fold/docs/architecture/code-quality-and-yams-benchmark.md
trvon 8ac9ddb175
docs: expand fold-sast skill; document call-graph decomposition + budgets
- skills/fold-sast: rename to skill-fold-sast and expand the scan/precision
  guidance for C++/Rust/Swift lanes
- docs/architecture: record call-graph phase decomposition and the
  optimization-loop TP/FP/TN/FN evidence policy
- docs/mcp-tools + README: update budget/per-query defaults (1800s/6000ms)
  and note the OpenGrep pre-push hook
2026-08-16 16:41:33 -06:00

339 lines
19 KiB
Markdown

# Code quality and Yams benchmark
This document maps Fold's source-side code-quality surface and defines how to
measure it on the sibling Yams repository. Security, binary, and DAST findings
remain separate lanes; this workflow is about maintainability, structural
redundancy, and reliability of the source analyzer.
## Capability map
| Goal | Detectors | Evidence | Cost / limitation |
| --- | --- | --- | --- |
| Find copy/paste implementations | `clone` | Syntactic Type-2/Type-3 hashing and similarity | Cheap after summaries. `--min-body-stmts` counts source-level executable statements; project headers can still dominate results. Tune `--min-body-stmts`, `--clone-type`, and `--clone-min-score`. |
| Find equivalent implementations | `equiv` | Bounded Z3 proof | Pairwise within compatible signatures; expensive and intentionally filters trivial constant-return pairs. |
| Find overlapping classes | `class-dup` | Heuristic class match backed by bounded method equivalence | Requires at least two eligible methods and matching method-signature fingerprints; use as design-review evidence, not an automatic deletion command. |
| Find large control-flow surfaces | `complexity` | Syntactic McCabe score from Clang CFG | Cheap and deterministic. The threshold is inclusive and configurable with `--complexity-threshold`. |
| Find unreachable or unused code | `dead-statement`, `unused-function`, `dead`, `unreachable`, `always-condition` | CFG or bounded proof, depending on detector | `unused-function` is intentionally limited to internal-linkage functions. Symbolic dead-code passes cost materially more than CFG checks. |
| Find redundant state changes | `redundant-store` | Conservative scalar data-flow, reported as heuristic | Unsupported control flow and non-scalar lifetime objects are suppressed. It is deliberately not labelled proved without an all-path proof. |
| Find C++ lifetime/API hygiene issues | `use-after-move`, `iterator-invalidation`, `static-ptr-asymmetry`, `deprecated-api` | SAT-confirmed, heuristic, or syntactic by detector | `use-after-move` checks source order and path feasibility; imprecisely modelled control flow is suppressed. Review confidence and evidence per result. |
`--scope quality` selects complexity, CFG/API, and C++ lifetime hygiene.
`--scope redundancy` adds clones, semantic equivalence, duplicate classes,
dead paths, constant conditions, and redundant stores. Use both scopes for a
complete structural review.
## Recommended Yams rollout
Start with two lanes so high-volume redundancy results do not block useful
correctness checks.
1. PR advisory lane:
```sh
fold scan --mode source \
--compile-db ../yams/build/release/compile_commands.json \
--detectors complexity,dead-statement,unused-function,use-after-move,iterator-invalidation \
--complexity-threshold 20 --processes 4 \
--format sarif --output yams-quality.sarif
```
2. Nightly design-review lane:
```sh
fold scan --mode source \
--compile-db ../yams/build/release/compile_commands.json \
--scope quality --scope redundancy \
--min-body-stmts 6 --clone-min-score 0.85 \
--complexity-threshold 20 --processes 4 \
--format sarif --output yams-structural.sarif
```
The two-TU metadata slice now has a reviewed baseline and gates only new stable
finding IDs. Keep broader Yams paths advisory until their findings receive the
same source review and per-finding rationale. The CI clang-tidy job is likewise
advisory while it uploads a per-commit baseline candidate; promote it only after
a reviewer has classified the captured diagnostics and accepted the baseline.
## Benchmark contract
The real-project runner is `scripts/benchmarks/run_code_quality.py`. It:
- selects a stable, sorted subset from an existing compile database;
- runs repeated serial scans with Fold's internal profile enabled;
- runs the same workload through subprocess fanout;
- fingerprints canonical finding content across every run and lane;
- checks physical locations, backend provenance, crashes, wall-clock timeout,
and a configurable Z3-timeout budget;
- reports per-detector median timings, peak RSS, and the shared
frontend/orchestration remainder;
- separates function fingerprinting, standalone call-graph construction,
cross-TU property collection, and symbolic-summary priming from the still
unattributed frontend/orchestration remainder;
- decomposes call-graph work into cache load, dirty-TU contribution collection,
contribution merge/SCC work, exhaustive fallback, index writes, and security
candidate expansion;
- writes hash-addressed `RunArtifact` records with exact commands, inputs,
outputs, environment/tool facts, measurements, and exit status;
- optionally compares wall time and finding identity with a prior baseline,
using paired repetition samples, effect, and a sign test when the baseline
carries matching raw samples;
- optionally compares stable finding IDs with a reviewed disposition baseline,
reporting new, persisting, and resolved findings, accepted/actionable trends,
changed-file scope, review coverage, and alert density per KLOC.
Run the current Yams metadata slice with:
```sh
scripts/benchmarks/run_code_quality.py \
--compile-db ../yams/build/release/compile_commands.json \
--project-root ../yams \
--path-regex '/src/metadata/' --max-tus 12 \
--repeats 3 --processes 4 \
--dispositions tests/benchmarks/yams/metadata_dispositions.json \
--out-dir /tmp/fold-yams-quality
```
For an A/B optimization, preserve the first `summary.json`, repeat the same TU
selection, and pass it back as `--baseline`. Finding drift fails by default;
`--allow-finding-drift` is reserved for intentional detector changes.
### Optimization harness
Use the wrapper when the goal is to collect one concise optimization artifact
rather than inspect each project-evaluation run separately:
```sh
scripts/benchmarks/run_optimization_harness.py \
--manifest tests/benchmarks/projects/yams_source.json \
--out-dir /tmp/fold-optimization
```
It preserves the project evaluator's profile/stability gates, turns launch and
timeout failures into recorded results, and reports a model-independent SARIF
size estimate (`ceil(UTF-8 bytes / 4)`) plus findings per estimated 1K tokens.
This is a comparison metric, not a claim of exact tokens for a particular model.
Use `--max-report-tokens` only after reviewing a workload-specific baseline.
The broader selection, ablation, and evidence policy is
[`docs/architecture/optimization-loop.md`](optimization-loop.md). In
particular, an unlabelled Yams or Chromium alert count is not a TP/FP/TN/FN
measurement; retain the project artifact and add an explicit oracle before
claiming precision or recall.
### Local quality artifact contract
`fold quality` keeps `summary.json` and terminal output compact. Each executed
step writes its complete stdout/stderr under `logs/`; the differential scan
keeps its complete SARIF report, and project-policy runs retain their summary,
markdown, and immutable run artifacts. The compact summary contains status,
bounded tail evidence, artifact paths, and only new/resolved finding details;
persisting reviewed findings remain in the full policy artifact. Finding detail
lists are capped for rendering and report the omitted count.
The summary records deterministic `ceil(UTF-8 bytes / 4)` estimates for both its
compact pre-metric payload and complete text artifacts. These measurements are
observational: no token threshold is enforced until a workload-specific
baseline is reviewed.
The synthetic precision guard is
`tests/benchmarks/source/code_quality_structural.json`. It pins positive and
clean cases for clones, semantic equivalence, duplicate classes, complexity,
dead statements, and unused functions. The standard quality gate now runs it.
Adjacent executable surfaces protect the analyzer that supplies the Yams
results. `run_source_metamorphic.py` requires invariant findings across source
renaming, harmless insertion, equivalent guard forms, TU order, and worker
count. `run_sanitizer_differential.py` compares vulnerable/fixed division and
signed-overflow programs against UBSan and OOB/UAF programs against ASan, with
deterministic replay. `run_clang_sa_differential.py` retains Fold/Clang SA
agreement on division and null-dereference pairs. `run_organic_pairs.py` keeps
three provenance- and hash-pinned upstream CVE adaptations separate from the
synthetic score. These do not turn comparator or runtime evidence into source
proof; they test classifications against independent controlled oracles.
The optimization contract is deliberately multi-dimensional:
| Property | Gate / metric | Reject when |
| --- | --- | --- |
| Precision | Vulnerable and clean-near-miss goldens; one-to-one matching; source metamorphic, UBSan/ASan/Clang SA differential cases, and origin-separated organic pairs | A clean case gains a finding, a required positive disappears, a semantics-preserving transform drifts, or a controlled comparator pair disagrees |
| Determinism | Canonical finding fingerprint, intersection/union, churn, pairwise Jaccard | Serial repetitions or worker counts change finding identity |
| Reliability | Exit status, crashes, subprocess timeout, Z3 timeout count, physical location, backend provenance | A scan crashes, hangs, exceeds its timeout budget, or emits malformed evidence |
| Speed | Median/IQR wall time, detector timers, source-phase timers | An A/B run exceeds the configured baseline regression budget |
| Resource use | Peak driver RSS | Memory exceeds the configured baseline regression budget |
| Structure | Per-production-file line budget | A new C++ monolith exceeds 1,000 lines or a tracked exception grows |
Every precision defect discovered during Yams review should enter the nearest
unit or golden corpus before its fix. Every performance change must preserve
the finding fingerprint unless the semantic change is explicit and reviewed.
## Initial Yams baseline (2026-07-13)
A four-TU metadata slice (`connection_pool.cpp`, `path_utils.cpp`,
`query_helpers.cpp`, and `tree_differ.cpp`) produced the following local
release-build baseline:
| Metric | Result |
| --- | ---: |
| Visible functions, including project headers | 949 |
| Stable post-filter findings | 171 |
| Profiled serial median (2 runs) | 14.011 s |
| Two-worker throughput median (2 runs) | 8.954 s |
| Peak driver RSS | 644.188 MiB |
| Z3 timeouts | 0 |
| Finding stability | Identical across all 4 runs |
Finding distribution was 72 clones, 81 redundant stores, 7 dead statements,
6 use-after-move results, 4 complexity hotspots, and 1 unused function. These
counts are a workload fingerprint, not a claim that all 171 results are true
design defects.
## Calibrated Yams result
Reviewing every non-clone result exposed systematic precision defects rather
than 95 independent Yams defects. After fixing the detectors and pinning each
failure mode with a focused regression, the exact same four-TU workload
produced:
| Metric | Before | Calibrated |
| --- | ---: | ---: |
| Stable post-filter findings | 171 | 32 |
| Clone | 72 | 28 |
| Redundant store | 81 | 0 |
| Dead statement | 7 | 0 |
| Use after move | 6 | 0 |
| High complexity | 4 | 4 |
| Unused function | 1 | 0 |
| Profiled serial median | 14.011 s | 12.391 s |
| Two-worker median | 8.954 s | 7.487 s |
| Peak driver RSS | 644.188 MiB | 649.555 MiB |
| Z3 timeouts | 0 | 0 |
The calibrated fingerprint was identical across two serial and two parallel
runs. Serial time improved 11.6%, two-worker time improved 16.4%, and peak RSS
increased 0.8%.
The 28 remaining clone reports are structurally valid candidates, not automatic
defects: 23 are configuration-family boilerplate in `TuneAdvisor.h`, two are
document-metadata enum mappers, one is repair-tuning boilerplate, one groups
connection-pool trace-environment helpers, and one pairs two closely related
query builders. The four complexity metrics are also arithmetically correct.
One (`errorToString`, complexity 37) is primarily a switch mapping table, so a
reviewer may reasonably baseline it while retaining the metric.
The precision fixes covered same-expression and reordered use-after-move
events, unsupported-control-flow redundant stores, RAII objects mistaken for
scalar stores, dependent-template calls mistaken for unused helpers,
configuration-pruned branches mistaken for actionable dead statements, and AST
node counts mistaken for source-level statement counts. Serial and fanout runs
now share the same clone/equivalence thresholds and cross-TU wire data.
Building the executable evaluation foundation found two further source-proof
defects. Shared evidence annotation was promoting local findings to
summary-backed/cross-TU corroborated and overwriting detector proof metadata;
detectors now own those flags at the actual callsite. Summary encoding also
used the wrong fallthrough condition after an early-return `if` without an
`else`, producing a guarded-division false positive. Both failures are retained
as unit, cross-TU, metamorphic, and sanitizer-differential regressions.
## Optimization surface
The calibrated run identifies the next measurable targets:
1. **Shared frontend/orchestration remainder — 6.623 s in the calibrated
baseline.** This was roughly half of profiled analysis time. Profiles now
expose `function_fingerprint_ms`, `call_graph_ms`, `cross_tu_property_ms`,
and `symbolic_summary_ms`, so the next run can distinguish reusable-fact
passes from the main traversal and genuinely unattributed setup. Remove
duplicate frontend work before micro-optimizing cheap detectors.
2. **Expensive passes with few solver queries.** `always-condition` takes
1.322 s for two queries, `equiv` 1.064 s for none, `dead` 0.994 s for one,
`unreachable` 0.974 s for none, and `use-after-move` 0.751 s for none on this
corpus. Candidate construction, serialization, and CFG/state setup are a
larger surface than Z3 itself. Shared facts are the likely optimization
boundary; detector semantics must remain independently tested.
3. **Header amplification — 4 TUs expose 949 functions.** Project headers are
legitimate analysis inputs but inflate clone candidates and memory. Add a
report-scope benchmark (main-file-only versus project-header-inclusive)
before changing coverage defaults.
4. **Memory — 650 MiB peak RSS.** Track this alongside time; an optimization
that retains every AST/summary to save recomputation may regress practical
Yams throughput.
5. **Reviewed change-time precision.** The two-TU Yams baseline now classifies
every result and blocks new unreviewed IDs. Extend the reviewed slice only
with the same per-finding rationale and stable configuration/TU contract.
6. **Organic recall gaps.** The pinned libssh2 slice is detected and all fixed
slices are clean; nullable-return propagation for the GStreamer `strchr`
case and OpenSSL public-key flow remains explicit backlog. DART-style
predicate flipping belongs only behind targeted confirmation tasks after a
concrete campaign plateaus.
The serial/parallel reliability benchmark already found and now guards one
class of drift: fanout workers previously ignored complexity and clone tuning
options, and same-file equivalence was incorrectly promoted as cross-TU
corroboration. `fold:subprocess_fanout` pins parity across those lanes.
### Current phase-attributed Yams slice
After adding source-phase timers, a two-TU metadata slice exposed 916 visible
functions through project headers. Across two profiled serial runs and two
two-worker runs, all four result sets contained the same 32 findings (pairwise
Jaccard 1.0) with zero Z3 timeouts.
| Surface | Median | Analyze share |
| --- | ---: | ---: |
| Function fingerprint pass | 1.188 s | 20.38% |
| Largest detector (`equiv`) | 0.620 s | 10.64% |
| All named detector timers | 2.468 s | 42.35% |
| Still-unattributed frontend/orchestration | 2.172 s | 37.26% |
| Total analyzed time | 5.827 s | 100% |
This makes the duplicate fingerprint parse the first shared-fact optimization
target. The extraction into `incremental_fingerprint.cpp` gives that work a
narrow contract; the deterministic transitive-hash test and the stable Yams
fingerprint are the reliability guard for any later pass fusion or cache reuse.
The post-decomposition rerun kept the same TU-set fingerprint and the same 32
findings across two serial and two two-worker runs: Jaccard 1.0, zero churn,
zero Z3 timeouts. Median serial wall time was 5.890 s, two-worker wall time was
3.091 s, and peak driver RSS was 504.797 MiB. Against the immediately preceding
baseline, those are reductions of about 8.8%, 9.5%, and 12.9%, respectively;
they are local measurements, not universal speedup claims.
The reviewed evaluation-foundation rerun selected
`connection_pool.cpp`/`content_index_writer.cpp`. Suppressing explicit
`while (true)` loop controls as non-actionable reduced the result to 31
findings: 27 accepted-risk and 4 actionable. Across two serial and two
two-worker runs the IDs were identical (Jaccard 1.0, zero churn, zero Z3
timeouts), with zero new and zero resolved relative to the reviewed baseline.
Local medians were 6.887 s serial and 3.429 s at two workers, with 523.109 MiB
median peak driver RSS. Alert density was 4.493 per KLOC over the 6.900 KLOC
union of selected TUs and finding-bearing project headers; new-alert density
was zero. These are workload measurements,
not universal performance claims.
The reviewed file is
`tests/benchmarks/yams/metadata_dispositions.json`. Twenty-seven accepted-risk
records cover typed tuning-accessor families, enum/string mappers, and the
exhaustive error-code mapping. Four actionable records cover the three repeated
trace-toggle helpers and the multi-responsibility query/acquire/configuration
functions. Each record has a rationale; the baseline pins Yams revision, TU-set
fingerprint, and analysis configuration fingerprint.
## Production structure guard
The source-size ratchet now has no exceptions. The former coordination and
analysis monoliths have narrow modules with every production C++ file below
1,000 lines:
| Former concentration | Facade now | Extracted ownership |
| --- | ---: | --- |
| Source scan runner | 850 lines | backend dispatch, fingerprints, call graph, summaries, TU execution, evidence, quarantine |
| Security walker | 355 lines | analysis, contracts, refinement, engine core/control, batch observer |
| Div-by-zero detector | 633 lines | guard facts and context |
| Null / overflow / OOB detectors | 962 / 934 / 926 lines | candidate and proof-analysis helpers |
| Symbolic encoder | 215 lines | foundations, operators, expression dispatch and arms |
| radare2 backend | 827 lines | session transport, serialization/classification, disassembly state, extraction |
`fold:source_structure` rejects a new production `.cpp` above the limit. This
is a maintainability ratchet, not a quality score: module cohesion, behavior
tests, and the benchmark contract remain the semantic guards.