5.2 KiB
5.2 KiB
CLI/MCP convergence cumulative code review — 2026-07-29
Scope and method
This review covers the cumulative uncommitted quality-harness, CLI/router,
report/query, MCP-policy/job, documentation-generation, and compatibility
changes relative to 669b4b4. The tracked diff was approximately 3,763
insertions and 2,100 deletions across 50 files before review fixes, plus the new
catalog/query/policy/harness files.
The review was findings-first and risk-weighted. It combined:
- targeted diff and history inspection;
- separate reviews of MCP/query, CLI/report, and quality/CI surfaces;
- schema/handler and report round-trip checks;
- focused regression tests followed by the complete Meson suite;
- Python harness tests, source-structure checks, diagnostics, generated-file parity, and whitespace validation.
Task 32 retains the separate bounded Fold self-scan so analyzer/tool failures do not get conflated with this manual differential review.
Confirmed findings resolved
| Severity | Finding | Resolution and regression evidence |
|---|---|---|
| High | MCP accepted workspace compile databases containing Clang/plugin load flags, permitting analyzer-process code execution outside the intended path policy. | Parse the compilation database with Clang, reject response files and plugin/pass-plugin arguments before dispatch, and cover -Xclang -load in mcp_input_policy. |
| High | Project-evaluation and optimization runs could consume stale report/profile/run artifacts from a prior invocation. | Remove every expected lane artifact before launch, require profiled runs to produce a profile, use a fresh optimization run directory, and add a stale-artifact regression. |
| Medium | CLI numeric options did not enforce the bounds encoded by the shared option catalog. | Add CLI11 checks for clone levels/thresholds, solver/process/time limits, and candidate/function caps; add CLI rejection coverage. |
| Medium | The Fold SARIF reader accepted malformed nested message/location shapes as empty actionable findings. | Validate Fold SARIF message and physical/logical location shapes and add malformed nested-field tests. |
| Medium | The binary query filter excluded granular binary-ir and binary-pcode-ssa provenance. |
Treat canonical backend filters as evidence-lane filters while preserving exact provenance; test P-Code matching. |
| Medium | Scan, grep, profile, FixIt, and embedded-skill writers did not consistently detect post-open stream failures. | Flush and check output state before returning a success/finding exit status. |
| Medium | SARIF round-trips omitted proof/bail evidence, failed to normalize ROP stackOffset, and did not declare every emitted rule. |
Emit and read the missing evidence, generate driver rules from the one canonical rule table, and add foreign/security rule plus round-trip tests. |
| Medium | Completed-job queries copied the full finding vector while holding the queue mutex and copied it again before bounded pagination. | Store immutable job findings behind a shared pointer, release the lock before querying, avoid the second copy, use overflow-safe paging, and enforce the server work-item ceiling. |
| Medium | Evidence queries omitted gadget, ROP-chain, and fact-provenance strings. | Search all string-bearing exploit and fact-provenance fields; cover gadget text. |
| Medium | Unknown future/foreign wire finding kinds aborted the entire report. | Map them to UnknownForeign, retain the original kind in correlation evidence, emit a warning, and add forward-compatibility coverage. |
| Medium | Newline/non-ASCII Git paths could be C-quoted and evade differential source/test selection. | Use NUL-delimited Git output and filesystem decoding; add an unusual-filename regression. |
| Medium | Lint comparison collapsed multiple same-identity warnings in one file into one accepted occurrence. | De-duplicate only identical source locations and compare identity multiplicity; test that an added occurrence is new debt. |
| Medium | fold quality inherited a hidden five-minute outer subprocess timeout despite two-hour CI tiers and tighter inner timeouts. |
Align the outer wrapper with the declared two-hour workflow ceiling while preserving inner test/scan limits and timeout classification. |
Reviewed decisions retained
- Scan exit status remains based on the filtered/deduplicated findings visible in
every selected report format.
findings-jsonis field-preserving and versioned; it is not a bypass around report visibility policy. fold grepintentionally requiresPATTERN REPORT...; filters-only invocation is outside the frozen CLI contract.- A positive fixture may also carry the
regressioncoverage label:positivedescribes expected detector behavior, whileregressionmarks the assertion as a locked prior behavior. Negative coverage remains a separate assertion. - Completed jobs remain session-scoped. The queue depth limits active work; immutable snapshots and query work ceilings bound per-query amplification.
Residual review limits
- The bounded production-only Fold self-scan is intentionally deferred to task 32 because the full workspace compilation database includes unsupported vendored/multi-command entries.
- This review did not claim whole-project sanitizer or fuzz coverage beyond the existing Meson suite.