fix(security): stop reporting incomplete scans as clean - #705
proffesor-for-testing merged 5 commits into
Conversation
proffesor-for-testing
left a comment
There was a problem hiding this comment.
Verified end-to-end on a fixture with a chmod-000 file, an unsupported extension, and a clean file: the CLI (aqe security --sast) reports status partial with exit 1 and per-file receipts (analyzed/unsupported/unreadable with sanitized errno), and the MCP security_scan_comprehensive tool on the same directory reports partial completeness with the unreadable file recorded per engine; the control directory reports completed/complete. Saved JSON/SARIF/Markdown carry the execution evidence. Exceptions never leak into output. Build passes; PR suites pass (185/185 with the receipts integration suite green in CI; its 60s fleet-init hook times out only in this sandbox); on the tree merged with current main (which now includes #702's edits to the same handler files) 21 files / 478 tests pass. Codex adversarial pass produced no findings against the PR code. Partial resolution of #694 as the PR states; keeping that issue open.
d47ec76
into
proffesor-for-testing:main
|
Thank you, @rudycelekli! This lands the hard part of #694: every security scan now carries per-file and per-engine execution receipts, so an unreadable file, an unsupported language, or a missing or failed Semgrep can no longer be mistaken for a clean result, in the CLI, the MCP tool, and the saved JSON/SARIF/Markdown reports. I reproduced it end-to-end with a chmod-000 fixture on both surfaces. Merged. One follow-up worth a look: the CLI runs only the SAST domain, so on a polyglot repo every non-JS/TS file becomes |
Summary
aqe security --sastcurrently passes string paths to an API expectingFilePathvalues, so aneval(userInput)fixture produces no findings and exits 0. Missing targets also exit 0, while comprehensive MCP reports missing/unsupported inputs ascompleted. This change preserves execution evidence from scanner to CLI, registered MCP, and saved reports, and makes incomplete analysis distinguishable from a completed scan with no findings.Refs #694. This is the execution-truth portion of that issue, not full SAST assurance or completion of all its acceptance criteria.
The security type imports are also placed separately from #702's quality import so the two contributions merge without a conflict. This changes no runtime behavior.
The schema and compatibility changes, including CLI exit behavior and the narrower meaning of
complete, are documented indocs/security/scan-execution-evidence.md. Missing optional Semgrep remains visible and does not invalidate completed required built-in pattern checks.CI follow-up
Includes the fork-comment guard from #701 and the runner/reporting correction from #704. Optional fork comments are skipped while artifacts remain available; nonzero runner exits and timeouts remain failures. Coverage uses valid reporters and produces a JSON summary, while JUnit remains a test report.
The 12 fork-comment regressions and nine runner-exit regressions pass. A real 18-test coverage control produces the expected reports with exit 0; an invalid-reporter control retains exit 1 despite all 18 tests passing. Earlier green coverage jobs did not establish completed coverage reporting: the old wrapper could mask the invalid coverage reporter's error. The current full run has now completed successfully with the corrected wrapper.
These reporting and exit-status failure modes are exercised by
tests/unit/scripts/fork-pr-comments.test.ts,tests/unit/scripts/ci-vitest-run.test.ts, and the real reporter controls described in #704.Verification
Final published head
3aec246e: all 25 checks pass. Full coverage job: 23,716 tests passed / 62 skipped; 982 files passed / six skipped. Coverage artifacts upload successfully; measured line coverage is 65.75% (the existing 80% comparison is advisory). No reporter errors or exit normalization occur.npm run buildpassed.The original dashboard and MCP-summary HTTP 403 comment failures are addressed by the included fix(ci): preserve reports without unauthorized fork PR comments #701 guard.
Relevant scanner, CLI, executor, protocol, report-saver, and MCP suites: 441 passed, 4 existing skips across 20 files. This includes 16 tests exercising the registered MCP tool, real discovery/scanner/task execution, and saved artifacts.
Replayed the six new consumer/producer regression files on unchanged upstream
29f0ed4f13b83632a394147046780f446a087223: 103 failed, 5 passed. Those tests all pass with this patch.Executed both built CLI and stdio MCP bundles in fresh temporary projects. The seeded vulnerable CLI fixture changes from exit 0/zero findings to exit 1/a finding; the clean control remains exit 0. Missing, unsupported, and empty targets no longer establish completed analysis. Repeating the same MCP call after changing source, with session caching enabled, returns a new digest and findings.
Changed-source ESLint reports the same three unused-variable errors reproduced on the baseline in
result-saver.ts,sast-scanner.ts, andprotocol-server.ts; no new lint errors.Focused public-route reproduction:
Failure modes
Missing/read-failed/unsupported inputs, discovery limits, duplicate inputs, partial runs, failed/malformed Semgrep output, legacy results, failed requested checks, saved-report propagation, and source mutation are exercised by the new tests. Semgrep's process boundary is mocked; the built CLI/MCP probes use an unavailable Semgrep executable and a loopback embedder. No native Semgrep integration or external security target was exercised.
Atomic repository snapshots, alias identity resolution, per-file external-engine coverage, mandatory Semgrep policy, implemented DAST/secret receipts, cross-run receipt reuse, and release-gate integration remain tracked in #694. The existing comprehensive MCP tool already bypasses session result caching; the freshness test verifies that behavior without introducing a cache mechanism.