60 lines
5.2 KiB
Markdown
60 lines
5.2 KiB
Markdown
# 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-json` is field-preserving and versioned;
|
|
it is not a bypass around report visibility policy.
|
|
- `fold grep` intentionally requires `PATTERN REPORT...`; filters-only invocation
|
|
is outside the frozen CLI contract.
|
|
- A positive fixture may also carry the `regression` coverage label: `positive`
|
|
describes expected detector behavior, while `regression` marks 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.
|