trevon/fold
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions
fold/DIFFERENTIAL_REVIEW_REPORT.md
trvon 50230c809b
 checkpoint
2026-06-04 20:20:29 -04:00

4.8 KiB
Raw Permalink Blame History

Differential Review Report — Fold symbolic/assertion changes

Date: 2026-06-04 Scope: unstaged Fold diff covering symbolic path modeling, P-Code path pruning, assertion layer/DBUILD checks, reporting evidence, router/build options, and tests.

Verdict

Major review completed with 3 agents plus local verification. Several concrete issues were fixed during review. Four larger design findings remain as follow-up tasks because they require broader modeling changes.

Fixed during review

  1. Fold assertions were masked as recoverable TU crashes

    • Risk: FOLD_ASSERT/FOLD_PRECONDITION inside source analysis raised SIGABRT, and runClangShardTranslationUnit could quarantine the TU instead of failing the scan.
    • Fix: include/fold/core/assert.hpp now tracks assertion-failure-in-flight; src/cli/clang_shard_tu.cpp resets/checks this flag and aborts on Fold assertion failures instead of treating them as ordinary TU signals.

  2. FOLD_DCHECK test did not match macro semantics

    • Risk: test assumed only FOLD_ENABLE_DCHECK controls FOLD_DCHECK, but macro also enables under !NDEBUG.
    • Fix: tests/unit/test_signal_isolation.cpp now requires FOLD_ENABLE_DCHECK for the test build and always expects SIGABRT.

  3. Debug-only abort conflicted with guarded-fact invalid-subject skip policy

    • Risk: GuardedFactStore::add aborted in test/canary builds for invalid AccessPath while release silently skipped.
    • Fix: src/symbolic/guarded_facts.cpp now consistently treats invalid subjects as skipped facts, then asserts only scalar/boolean payload invariants for valid facts.

  4. P-Code path invariant was debug-only before release dereference

    • Risk: path_ was checked with FOLD_DCHECK before release dereference.
    • Fix: src/symbolic/pcode_encoder.cpp now uses always-on FOLD_ASSERT for initialized/boolean P-Code path before feasibility/observer paths.

  5. Disabled Rust/Swift frontend options broke compilation

    • Risk: -Drust-frontend=false / -Dswift-frontend=false omitted backend sources but scan_runner.cpp still instantiated them.
    • Fix: src/cli/scan_runner.cpp now gates backend construction with FOLD_HAVE_RUST / FOLD_HAVE_SWIFT and throws clear disabled-frontend errors.

  6. DCHECK canary docs reused another build dir's native file

    • Risk: BUILD.md example pointed build/dcheck at build/release/conan_meson_native.ini.
    • Fix: docs now use independent build/dcheck Conan/Meson files and show meson configure ... -Denable-dcheck=true for existing builds.

Remaining findings / follow-up tasks

  1. High — P-Code CBRANCH assumes all later ops are fallthrough

    • Ref: src/symbolic/pcode_encoder.cpp:437-444.
    • Scenario: if lifted P-Code is ordered taken-block-first, global path && !cond can suppress real hazards in the taken path. Unconditional branch/return boundaries are also not modeled.
    • Follow-up: task #12.

  2. Medium — unknown-call havoc does not clear AccessPath-backed facts

    • Ref: src/symbolic/encoder.cpp:1038-1069.
    • Scenario: mutable calls clear member-value cache but not AccessPath ranges/non-null/guarded facts, so facts like obj.v.size() > 0 may survive mutate(&obj).
    • Follow-up: task #13.

  3. Low/Medium — latent/precondition evidence is serialized but not produced by detectors

    • Refs: src/report/finding_json.cpp:104-107, src/report/sarif.cpp:283-290.
    • Scenario: real raw-pointer/config-derived precondition findings still appear as ordinary findings until detector emission is wired.
    • Follow-up: task #14.

  4. Low — hybrid profile merge drops timing/counter fields

    • Ref: src/cli/scan_router.cpp:454-476.
    • Scenario: hybrid profiles merge detector rows and counts but omit lane timing/prefilter counters, reducing benchmark/profile fidelity.
    • Follow-up: task #15.

Validation run after fixes

Passed:

ninja -C build/release \
  tests/unit/test_signal_isolation tests/unit/test_guarded_facts \
  tests/unit/test_pcode_encoder tests/integration/test_scan_e2e \
  tests/integration/test_subprocess_fanout

meson test -C build/release \
  fold:signal_isolation fold:guarded_facts fold:pcode_encoder \
  fold:scan_e2e fold:subprocess_fanout --print-errorlogs

git diff --check

Result: 5/5 OK, 0 failures.

Also passed no-frontends build smoke:

meson setup build/review-no-frontends ... \
  -Dbuild-tests=false -Drust-frontend=false -Dswift-frontend=false \
  -Dghidra=disabled -Dcapstone=disabled -Dradare2=disabled \
  -Dbuild-mcp-server=false -Dbuild-lsp-server=false
ninja -C build/review-no-frontends libfold-core.a

Coverage limits

This was a differential review over the current unstaged diff, not a full architectural audit. The largest remaining risks are path-model semantics and fact invalidation, which need dedicated implementation/testing passes.