Skip to content

fix(security): stop reporting incomplete scans as clean - #705

Merged
proffesor-for-testing merged 5 commits into
proffesor-for-testing:mainfrom
rudycelekli:fix/security-scan-receipts
Sep 22, 2026
Merged

proffesor-for-testing merged 5 commits into
proffesor-for-testing:mainfrom
rudycelekli:fix/security-scan-receipts

Conversation

@rudycelekli

@rudycelekli rudycelekli commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

aqe security --sast currently passes string paths to an API expecting FilePath values, so an eval(userInput) fixture produces no findings and exits 0. Missing targets also exit 0, while comprehensive MCP reports missing/unsupported inputs as completed. 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.

  • Record per-file source digests/dispositions and per-engine status, scope, actual built-in rule IDs/digest, and limitations. Count completed analysis; never count findings as executed rules.
  • Preserve discovery failures/truncation, unsupported files, partial findings, optional Semgrep absence/failure, and unverified legacy results. Validate Semgrep output while retaining valid findings and all nine supported severity values.
  • Run default CLI SAST, return nonzero for incomplete requested checks, and retain evidence in JSON/Markdown/SARIF. MCP retains all findings and execution status through saved artifacts.
  • Replace the exported audit protocol's placeholder secret success with unavailable, and prevent failed/unverified requested checks from authorizing its deployment recommendation. Honor trigger scope.

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 in docs/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 build passed.

  • 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, and protocol-server.ts; no new lint errors.

Focused public-route reproduction:

AQE_PROJECT_ROOT=$(mktemp -d) TMPDIR=$(mktemp -d) npx vitest run tests/integration/security-scan-execution-receipts.test.ts --maxWorkers=1

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.

  • Every failure mode mentioned in this PR description has either (a) a test that exercises it, or (b) a linked tracking issue.

@proffesor-for-testing proffesor-for-testing left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@proffesor-for-testing
proffesor-for-testing merged commit d47ec76 into proffesor-for-testing:main Sep 22, 2026
29 checks passed
@proffesor-for-testing

Copy link
Copy Markdown
Owner

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 unsupported, the scan is partial, and aqe security --sast can no longer exit 0, while the MCP path runs generic patterns on those files and reports completed. Neither direction is a false clean, but the two surfaces disagree. Either running generic patterns in the CLI or scoping CLI discovery to supported extensions would close that gap. Also noting for downstream users: --dast now exits 1 as not-run, rulesApplied now counts unique evaluated patterns, and SARIF sets executionSuccessful: false for legacy receipt-less results, all documented in docs/security/scan-execution-evidence.md.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants