Skip to content

feat(FAFF-915): trim adversarial-review context to diff-relevant regions - #793

Merged
alechill merged 6 commits into
mainfrom
faff-915-trim-adversarial-review-context-to-diff-relevant-regions
Aug 31, 2026
Merged

feat(FAFF-915): trim adversarial-review context to diff-relevant regions#793
alechill merged 6 commits into
mainfrom
faff-915-trim-adversarial-review-context-to-diff-relevant-regions

Conversation

@alechill

Copy link
Copy Markdown
Contributor

Summary

Adds a diff-relevance filter to review-call.mjs that trims the --context bundle to the regions the diff references before the review payload is assembled. Reasoning-ON reviewers empty out on large payloads (FAFF‑906: the model spends its whole output budget reasoning and emits zero findings); the context bundle is most of that payload while the diff only touches a few regions. Trimming it roughly halves the payload so a reasoning-ON review fits under the empty-out knee. This is the targeted context-trim (the lighter fix), not the general decomposer (FAFF‑916, cancelled).

Closes FAFF‑915.

Approach

  • A pure, exported trimContextFiles({ contextFiles, diff, ... }) applied between reading the --context files and assembleUserMessage in main.
  • Relevance = a diff-adjacency + identifier-window heuristic: keep diff-touched ranges (always) plus lines naming a diff identifier, each expanded by a ±window and merged; drop the rest with an elision marker.
  • Gated by a byte threshold, so small payloads are byte-identical to today and every existing review test is unaffected; --context-trim-bytes 0 disables it entirely.
  • Conservative: a diff-touched line is never dropped. Identifier anchors are frequency-capped and a retained-fraction ceiling forces head-retention, so a recurring symbol cannot under-trim. Small no-anchor caller-selected files pass through untouched; large no-anchor files retain their head. Fails safe (passthrough) on any diff-parse ambiguity.
  • Pure and deterministic, so the trimmed context stays identical across the four spec-review lenses and the shared-prefix cache (FAFF‑903) holds.

Acceptance Criteria

  • AC 1 — Above the threshold, a large no-anchor file is head-reduced; a diff-touched file keeps the touched region and its window; a small no-anchor file passes through.
    Verified: test/adversarial-call.test.mjs FAFF‑915 tests 3, 5, 6 — passing
  • AC 2 — report.bytesAfter <= 0.6 * report.bytesBefore on the fixed fixture.
    Verified: FAFF‑915 test 4 — passing
  • AC 3 — Below the threshold, assembleUserMessage output is byte-identical and the existing suite passes unchanged.
    Verified: FAFF‑915 test 1 + full suite node --test test/adversarial-call.test.mjs → 284 pass
  • AC 4 — A line inside a diff-touched range is never dropped.
    Verified: FAFF‑915 tests 7 and "touched over the ceiling keeps all touched lines" — passing
  • AC 5 — --context-trim-bytes 0 disables the trim.
    Verified: FAFF‑915 test 2 — passing
  • AC 6 — Fail-safe passthrough on a malformed diff.
    Verified: FAFF‑915 test 11 — passing
  • AC 7 — Exactly one stderr note when a trim fires, none on the no-op paths.
    Verified: CLI end-to-end — [note] FAFF‑915 context trim: 4800 → 693 context bytes on fire, silent when disabled
  • AC 8 — Deterministic across repeated calls (shared-prefix cache holds).
    Verified: FAFF‑915 test 16 — passing
  • AC 9 — Frequency-cap and retained-ceiling fallback.
    Verified: FAFF‑915 tests 9, 10 — passing
  • AC 10 — faff validate-adapters, unit suite, faff lint-refs clean.
    Verified: validate-adapters PASS (20 slot skills), lint-refs PASS, gate ladder signal pass

Review

Adversarial code review: 4 rounds, terminal verdict pass. Resolved: full-path range matching, blank/CRLF context-line desync, hunk-length-aware parsing, headReduce clamp. Residual findings were non-gating (a refuted false positive, malformed-input-only edges, documented low-risk items).

Closes FAFF-915

alechill added a commit that referenced this pull request Aug 31, 2026
Signed-off-by: Alec Hill <alec@shftwst.dev>
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Ready to land. Run this locally in a real terminal:

faff effects declare --run-dir .faff/anchors/run-20260831-141101-graft-FAFF-915 --issue FAFF-915 --step merge <<'EOF'
[{"kind":"merge","target":"pr:793","reversible":true}]
EOF
faff merge-gate --pr 793 --issue FAFF-915 --run-dir .faff/anchors/run-20260831-141101-graft-FAFF-915 --level L2 \
  --execute --merge-args "--squash --delete-branch"

…relevant regions

Signed-off-by: Alec Hill <alec@shftwst.dev>
Add a diff-relevance filter to review-call.mjs that trims the --context bundle to
the regions the diff references before assembly, so a reasoning-ON review of a large
payload fits under the empty-out knee. Gated by a byte threshold (byte-identical
below it and when --context-trim-bytes 0 disables it); conservative (never drops a
diff-touched line); identifier anchors are frequency-capped and a retained-fraction
ceiling forces head-retention so a recurring symbol cannot under-trim. Pure and
deterministic, so the trimmed context stays identical across the four spec-review
lenses and the shared-prefix cache holds.

Signed-off-by: Alec Hill <alec@shftwst.dev>
…line desync, headReduce clamp

Adversarial code review flagged: (1) a basename-only touched-range key could
cross-apply one file's ranges to another same-basename context file — now keyed by
full path with exact-or-suffix pathsMatch; (2) a truly empty context line inside a
hunk did not advance the new-file line counter, desyncing later touched ranges — now
treated as a context line; (3) headReduce could emit a bogus elided count for a
negative headLines — now clamped. Tests added for each.

Signed-off-by: Alec Hill <alec@shftwst.dev>
…ormalization

Round-2 adversarial review flagged: (1) a CRLF-terminated diff's blank context line
arrived as a bare CR and did not advance the new-file counter; (2) an added line whose
content begins with '+++ ' was misread as a file header; (3) hunks were not bounded by
their declared line counts, so a truncated/overrun hunk shifted ranges. Rewrote
parseDiffTouched to strip a trailing CR and to bound each hunk by its declared -a,b +c,d
counts, so structural tokens are only recognised between hunks and in-hunk body content
is parsed correctly. Tests added for CRLF, an in-hunk '+++ ' body line, and multi-file
hunk-length attribution.

Signed-off-by: Alec Hill <alec@shftwst.dev>
…e ceiling + pure-insertion parse

Round-3 review re-raised the retained-ceiling 'drops touched lines' claim (a recurring
false positive: the !hasTouched guard exempts any touched file from the ceiling). Added
a test proving a touched file over the 0.8 ceiling keeps every touched line beyond
headLines, clarified the guard comment, and added a pure-insertion (@@ -0,0 +1,N @@)
parse test. Softened the pathsMatch comment to state the bare-basename suffix match is
keep-only-safe (over-retains, never under-retains).

Signed-off-by: Alec Hill <alec@shftwst.dev>
Signed-off-by: Alec Hill <alec@shftwst.dev>
@alechill
alechill force-pushed the faff-915-trim-adversarial-review-context-to-diff-relevant-regions branch from a7655f8 to 93e2869 Compare August 31, 2026 15:03
@alechill
alechill merged commit aef1d53 into main Aug 31, 2026
8 checks passed
@alechill
alechill deleted the faff-915-trim-adversarial-review-context-to-diff-relevant-regions branch August 31, 2026 15:12
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.

1 participant