Repository navigation
ci(issue-refs): in-file opt-outs at line, block and file scope; honour the PR-body opt-out locally; repeat guidance after the hits - #307
Conversation
…r the PR-body opt-out locally; repeat guidance after the hits
Four defects in one opt-out mechanism, found when an agent hit the gate with
25 refs in a generated document:
1. The guidance was invisible. The failure message printed the opt-out
instructions ABOVE the hits list, and a failing tool is read from its tail
- often literally, through `| tail`. On 2026-08-17 an agent hand-qualified
25 refs, some inside quotations, never learning any opt-out existed; the
header had said so all along, 23 hits earlier. formatFailure now repeats a
fix/opt-out reminder AFTER the hits, with a unit test pinning the position.
2. There was no in-file opt-out at all - the only escape hatch was the
whole-PR hammer, and it lived in the PR body. New, narrowest first, all
written in the file's own comment syntax (invisible when rendered in
markdown/HTML):
issue-refs: exempt that line only
issue-refs: exempt-begin/ a pasted block - for quoted material too
issue-refs: exempt-end dense to mark per line without being a mess
issue-refs: exempt-file a whole file (a generated doc, an imported
archive) - the in-file equivalent of an
EXEMPT_PATHS entry, without editing the gate
These travel with the file: they hold for future PRs and for local runs
with no PR body. Line/block markers govern added patch lines (same-line/
same-patch by necessity - a marker outside the patch is invisible to a
patch-based gate); the file marker is judged against file CONTENT by both
callers (local: disk; CI: contents API, fetched only for files with hits),
so it holds however small the diff. An unclosed exempt-begin swallows the
rest of the file's added lines, mirroring the unclosed-fence rule. A
backslash escape (\confluentinc#857) was rejected: it edits the text it is meant to
preserve, renders visibly outside markdown, and is an illegal escape in a
Java string literal.
3. The PR-body opt-out was one-sided. bin/check-issue-refs.sh never read
bodies, so an opt-out CI accepted left every local run red on the same
file forever - a "local mirror of CI" permanently disagreeing with CI. The
script now resolves the current branch's PR via `gh pr view` (repo derived
from origin), honours the opt-out exactly as pr-checklist.yml does, and
scans the body as a synthetic file too, pre-empting the CI round-trip for
body refs. Fetch failure (no PR yet, no gh, offline) degrades to the old
body-less behaviour, and the message now says "could not read the PR body
(no PR yet, or gh unavailable)" instead of implying it never does.
4. Nothing tested the local script's plumbing. New bin/test-check-issue-refs.sh
stubs gh on PATH with eight end-to-end cases (no-PR fallback both ways,
line and file opt-outs from disk, PR-body opt-out honoured, body ref
flagged, clean pass naming the body, the reminder surviving `| tail -3`),
runs in pr-checklist.yml beside the gate module's unit tests, and dogfoods
the line marker on its own fixtures.
Verified red against master's code with the new tests kept in place: 7 of 8
harness cases fail and the unit-test run (59 assertions on the new code)
aborts on the first missing feature. All checks green on the new code:
unit tests, harness, sigpipe guard, actionlint, and the gate itself run
against this branch. docs/issue-references.md rewritten where this change
falsified it ("purely textual, no API calls"; "the PR body ... does not
exist when you run the script") and now documents all four opt-out scopes,
narrowest first.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
…d/checkout silent-loss lesson Captures the process failure from this branch's own development as a durable learning: three git commands (stash pop, commit --amend against an emptied index, checkout <ref> -- <paths>) each reported success while doing something other than intended, costing the same uncommitted work twice and voiding one red-proof entirely. The doc records the correct red-proof shape (old CODE + new TESTS; throwaway worktree as the safest form) and the prove-the-capture habit (status clean + grep the new symbol in git show HEAD:<file>). Also adds "Red-proof" to CONCEPTS.md's Test reliability cluster - the practice bin/AGENTS.md mandates now has a name and a definition. Grounding: mechanical claims validator clean (4 paths, 3 links); semantic validator verified 10 claims including the git-stash man-page semantics, and its one finding (the quoted bin/AGENTS.md phrase line-wraps, so a literal grep fails) is fixed in the doc's wording. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
…the gate module, and fix PR resolution under -R
ce-simplify pass over the branch, three reviewers. Applied:
- The file-opt-out drop loop existed twice - bin/check-issue-refs.sh's node
block and pr-checklist.yml's workflow step - identical control flow
(dedupe, PR-body skip, fail-open-keeps-hit) differing only in the reader.
That is the exact NO-SECOND-COPY violation the module's own doctrine names.
Now one exported gate.dropFileOptedOutHits(hits, readFile, {onDrop,
onError}); each caller supplies its reader (fs vs contents API) and its
logging. Unit-tested (dedupe, body skip, fail-open, callbacks) - 61
assertions total.
- Dropped the dead exports LINE_OPT_OUT / FILE_OPT_OUT - nothing imported
them; hasFileOptOut is the intended surface, matching the unexported
BLOCK_BEGIN/BLOCK_END.
- gh pr view gained timeout: 3000 - this script runs in the pre-commit hook,
whose documented budget is ~1.5s, and execFileSync otherwise blocks forever
on a hung network. Measured: ~0.7s total with a live PR, ~0.2s without.
- FIXED A REAL BUG the timing run exposed: `gh pr view -R <repo>` refuses to
infer the branch ("argument required when using the --repo flag"), so the
body fetch silently failed on every run - the stubbed-gh harness could not
catch a real gh CLI contract. The branch is now passed explicitly
(detached HEAD skips), verified live: the success line names the PR body.
- Deleted two dead fixture lines in the harness (written state overwritten
before any check observed it).
- docs/issue-references.md's backslash-rejection rationale trimmed to a
pointer at the LINE_OPT_OUT design comment, which owns it (never state a
fact twice).
Skipped, deliberately: formatFailure's header/trailer repetition (the
truncation-resilience feature itself, position-pinned by test); the harness's
hand-rolled check() helper (bin/AGENTS.md documents copying, not sharing, as
the convention); two pre-existing rationale duplications in
docs/issue-references.md dating from before this branch (out of scope).
Verified: 61 unit assertions, 8/8 harness cases, sigpipe guard, actionlint,
the gate on this branch's own diff (now confirming "and PR 307's body").
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
… activates it, and a body fence closes an open block
Applies all six findings from this branch's code review (validated 6/6 by
independent repro). The two P1s were unintended false-pass paths that broke
the PR's own contract:
- MENTION IS NOT USE. Marker tests now run on a span-stripped view
(stripCodeSpans): a marker quoted in a backtick code span is documentation.
Before this, five of this branch's own files - including
docs/issue-references.md, the convention's defining doc - were permanently
whole-file exempt, and a doc line quoting the block markers opened a real,
unclosed block. An unquoted prose mention still counts (indistinguishable
from use); the docs now say to backtick marker names when writing about
them. A line carrying BOTH block markers is treated as discussion: no
state change.
- A FENCE CLOSES ANY OPEN BLOCK in the PR body. prBodyEntry blanks fenced
lines, so an exempt-end inside a fence was invisible and the block never
closed - everything after silently escaped (fail-open). The fence boundary
now emits a synthetic end marker, flipping the failure mode to fail-closed.
- bin/test-check-issue-refs.sh joins EXEMPT_PATHS with the same rationale as
the gate's unit-test file: its fixtures write real markers and bare refs
deliberately.
- The harness truncation case now uses tail -4 and asserts the trailer's
LEAD line ("Fix: qualify each ref") as well as the opt-out substring -
tail -3 was provably cutting the lead while the test passed.
- The unit-test file's async block is captured and awaited before the
summary line, so the printed count (now 67) is accurate and prints last.
- AGENTS.md's issue-references entry updated: both the local script and CI
scan the PR body when one is reachable (the old line implied CI-only).
Regression tests: quoted file/line/block markers are inert at every scope;
unquoted both-markers lines change no state; the fence repro flags both the
ref before the block and the ref after the fence; a fence with no open block
is unchanged. 67 unit assertions, 8/8 harness cases, sigpipe + actionlint +
the gate itself all green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
|
@claude review this |
|
Claude finished @astubbs's task in 4m 40s —— View job Review
On "doesnt look dry" (2026-08-18T00:43:07Z): already addressed. Finding:
|
…s actual budget The cloud review caught the arithmetic the earlier bound ignored: timeout 3000ms justified itself against .githooks/pre-commit's "~1.5s" budget while being 2x that budget on its own, for one of SIX sequential gates. Now 1000ms, with the comment rewritten to carry the real reasoning: a slow gh answer must lose to the hook staying fast, and a body the pre-commit run missed is still caught at CI - a deferred check, never a lost one. Reviewer's optional note (a second owner/repo parser vs bin/check-quarantine-owners.sh's sed version) deliberately not taken: the two live in different languages (bash+sed vs a JS regex inside a node heredoc), so sharing an implementation buys drift risk in glue code instead of removing it - the reviewer's own assessment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
|
Response to the review:
Both test suites green after the fix (67 unit assertions, 8/8 harness cases). |
…ail-trailer # Conflicts: # .github/workflows/pr-checklist.yml
…t-outs #307 landed the mechanism this branch was working around. It adds in-file opt-out markers at line, block and file scope, written in the file's own comment syntax, and names quoted source material as the case they exist for - which is exactly the case here. So the whole-file `EXEMPT_PATHS` entry this branch added is reverted, along with its test and its sentence in docs/issue-references.md: those three files now match master exactly. A whole-file exemption was always coarser than the problem - only three lines needed it, and it would have left every future bare ref in that document unchecked. In its place, four `issue-refs: exempt-begin`/`-end` pairs around the indented blocks of quoted material in mechanical-issue-ref-sweep-falsified-a-verbatim-log-quote.md. Block scope rather than per-line because those blocks are markdown code blocks: a same-line marker, or the inline code span this branch tried first, renders literally inside the quotation - which would be a smaller version of the falsification the document is about. The markers sit outside the quoted lines, so all of them are now byte-identical to the source; the only other change is the prose ordinals (`run #1` and friends), which are not in a code block and take inline spans correctly. Master's own new EXEMPT_PATHS entry, bin/test-check-issue-refs.sh, is kept - the conflict was purely that both sides appended to the same list.
…p, and amend the sweep learning its own extraction refuted Three corrections, all found at merge prep rather than during the work. **The two batching items are different kinds of defect and were billed as one.** The arithmetic in `calculateQuantityToRequest` needs no caller error: `target - modulo` where `batchSize - modulo` was meant affects every correct batching configuration, continuously. The absent `batchSize` bound in `validate()` needs somebody to pass a zero. Filing them at equal weight made the weaker one inflate the stronger. They are now one note each, per this directory's one-item-per-file rule, and remain a single issue, since the same field and the same one-PR fix cover both. **The `batchSize = 0` claim was over-broad.** It said the consumer starts cleanly and processes nothing. That holds on a default configuration, but `PCModule.initDynamicLoadFactor()` divides `messageBufferSize` by the same zeroed target, so the same misconfiguration throws `ArithmeticException` at construction whenever a buffer size is set - and NPEs on `null`, the field being boxed. Silent, fatal, or an NPE depending on unrelated settings is the argument for a bound rather than for documentation. Found by the merge-prep search for other instances of the class, which also confirmed the arithmetic half is a single site: `PCModule` is the only other modulo in main code and its ceiling division is correct. **The extraction reproduced the failure the extracted learning documents.** `mechanical-issue-ref-sweep-falsified-a-verbatim-log-quote.md` recommends an inline code span as the remedy for a quoted artifact that trips the reference gate. Applying it here rendered the backticks as part of the quotation, because the quote sat in an indented code block - a smaller copy of the forgery the document exists to record. The remedy is container-dependent, and the document did not say so. It now carries that as guidance item 8, pointing at #307's block markers as the general answer: they sit outside the quoted material, so every quoted byte survives, which is the property the whole document is about. Item 2 gains a forward reference so a reader who stops there is not misled. Amending an extracted document rather than copying it verbatim is a deliberate exception: the correction is the direct result of this branch's own attempt to follow it, and holding it back to a follow-up would leave the next reader to repeat the failure.
…es, and the batching defect the recovery found (#312) Committed planning documents are project content even when their implementation does not exist yet. Knowledge left on a branch is invisible to any agent that does not already know the branch exists, and the branch can vanish: while this sweep was being assembled, docs/proxy-http-ideation was merged into #293 and deleted, and its note did not travel with it. That note is recovered here from the commit object alone. Every remote branch was surveyed against master's current tree rather than merge-base diffs, which over-report. Most doc paths that exist on a branch and not on master are carried by a branch with an open PR and land when it does; those were left alone. What remained was triaged by reading each note, not by bucketing it on its filename prefix - which would have been wrong in both directions. Brought over: the 2026-08-17 ideation wave, five notes plus the three HTML deliverables they only point at, since the notes are one-page summaries and the substance is in the HTML; one open design proposal, recoverable producer fencing (#225); twelve docs/solutions/ learnings, whose home is master by definition, adding the architecture-patterns, best-practices, runtime-errors and logic-errors categories; and the chaos pain suite design plan, which master's shipped phase-1 plan names in its own origin: frontmatter and cannot resolve, and which is the only definition of phases 2 and 3 that docs/inflight/test-chaos-phase2.md tracks as open. Also brought over, as a deliberate exception to the rule above: six files from #306, which is not scheduled to merge until after v6. One of them records a project-wide gap that would otherwise stay invisible through the release it could affect - there is no japicmp or revapi equivalent, so a source- or binary-breaking public API change rides through CI unflagged. Left behind as spent rather than stranded: the whole v6 documentation-ideas cluster, whose output already shipped as docs/data/roadmap.yaml, docs/data/module-maturity.yaml and the docs/features/ catalogue; the package-rename dress rehearsal, since the real rename merged as #294; and a stale outside-collaborator grant, since the grant is gone and its branch is deleted. Left with their branches: pr- notes, which the PR that resolves them tracks, and the ks-streams unit plans, which document code that exists on those branches. 2026-08-14-language-proxy-interaction-model-ideation.html is deliberately not taken. Two sessions extended it in parallel; #293 has since absorbed the HTTP half and carries none of the polyglot half, and its copy is still growing. A master copy would fork the document a third time. The note recording the collision is here instead, and the reconciliation belongs on that PR. A survey is a photograph, and this one aged inside the hour. The actor-revival pair was triaged as stranded and then removed again once #310 opened to land it. Every remaining file was re-checked against the open PRs at push time rather than at survey time. The batching ideation note carried an unreported defect, confirmed against master and filed as #311, recorded as two notes because docs/inflight/ is one item per file. The arithmetic one needs no caller error: calculateQuantityToRequest rounds up with target - modulo where filling the tail batch needs batchSize - modulo, and the variable is named extraToFillBatch. target is always a multiple of batchSize, so it fires whenever current is not, and in-flight settles near twice the configured target. The second-order effect is the one worth keeping: the inflated delta makes lastWorkRequestWasFulfilled false, which gates the adaptive load factor, so under batching the load factor stops stepping up precisely when the pool queue is low. The second note covers an absent bound on batchSize, where zero is silent on a default configuration, throws from PCModule.initDynamicLoadFactor() when a message buffer size is set, and NPEs when null. Default configurations are unaffected, batchSize defaults to 1, and there is no data-safety consequence. Searching the class found PCModule to be the only other modulo in main code, and its ceiling division is correct, so the arithmetic defect is a single site rather than a pattern. Extracted docs predate the issue-reference gate, so bare numbers below #1000 were qualified against the repo they actually mean. One document could not be: the write-up of a sweep that once rewrote a verbatim log line into a string no program ever emitted. Its remaining bare refs are the gate matcher's quoted inputs beside its quoted outputs, and qualifying them rewrites the demonstration into a false one. It carries #307's block opt-out markers around those blocks, which sit outside the quoted material so every quoted line stays byte-identical. That document is also the one file amended rather than copied. Its own recommended remedy is an inline code span, and applying it here reproduced the failure it records, because backticks are literal inside a code block. It now carries that as guidance item 8, recommending the block markers as the general answer, with a forward reference on item 2. The correction came out of this branch's own attempt to follow the document, so deferring it would have left the next reader to repeat it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ions Master landed #310's ranked revival of the 2022 micro-actor family the same day this branch wrote up the interrupt-overload finding, and the two were not pointed at each other. Forward: this branch's design read as greenfield - "add a nudge variant" - when a framework already exists, 537 lines in 4 files coupled to PC by one 16-line marker interface, with six ranked directions for reviving it. Both the solution write-up and the contract-debts note now say to read next-actor-revival.md first, and name survivor 5 (skeleton-first strangler) as the shape a payload-free nudge should land in rather than a rewrite. Back: survivor 6 is a concurrency mass budget - an ArchUnit ratchet on primitive counts, conversions graded on locks removed - and this branch has already taken a reading it can start from. Four meanings on one bit, four hand-clears, one of which does not clear and only warns that it cannot tell which meaning it got. That is a mass measurement with a documented failure behind it, which is more useful to a ratchet than a count taken cold. Also merges master: #310 and, in the previous merge, #304, #305, #307, #286 and #312. Core unit suite: Tests run 372, Failures 0, Errors 0, BUILD SUCCESS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ahSzHD72EvTGTYkmpTsx6
…ponse-timeout Eight commits, all docs and CI tooling - no product code, and no conflicts. The two files both sides touched, docs/refactoring.md and the upstream manifest, auto-merged cleanly. Checked rather than assumed, because a clean merge proves nothing about content: this branch's refactoring.md entry and upstream-map entry both survive, the inflight note this branch deletes stayed deleted, and both halves of the poller-death handoff are intact. Worth knowing from what came in: #307 reworks the issue-reference gate - in-file opt-outs at line, block and file scope, and the local mirror now reads the PR body the way CI already did. Re-run here against the new gate and clean, now reporting "77 changed file(s) vs cae88c7 and PR 204's body". Verified: full unit suite green across all 11 modules, plus copyright, issue-refs, todo-index, docs-data, quarantine-registry and the new .claude/hooks self-tests.
…ponse-timeout Eleven commits, one conflict, in docs/inflight/bug-857-family.md. Master had grown that file from four sightings to nine while this branch carried one insertion of its own with two entries renumbered around it. Resolved by taking master's numbering wholesale and placing this branch's sighting chronologically - 2026-08-17, between the 08-14 and 08-18 entries - as the eighth, which shifts master's two 08-18 entries to ninth and tenth. Checked before renumbering: no prose in the file cross-references those ordinals, only the second, third and fourth. Two claims in that entry were softened because master overtook them. It no longer calls itself the first replayed seed - unverifiable against nine entries - and it drops the "seeds are perishable" framing, because #307's sibling now carries the replay seed inside the autopsy block itself. Also arriving, and the reason this merge is worth reading rather than just taking: #209 is a 241-line change to AbstractParallelEoSStreamProcessor - the close and pool-shutdown race - in the same file this branch modifies, and it makes failureReason volatile and narrows the state-machine setter to package. The close path it hardens is the same one this branch's commitSync now throws on. Verified: full unit suite green across 11 modules, core 381 tests (up from 359 - #209 brought 22 with it), 0 failures. All gates green.
Description
Four defects in the issue-reference gate's opt-out mechanism, found when an agent hit the gate with 25 bare refs in a generated document and never learned an opt-out existed:
Guidance was invisible in practice. The failure message printed opt-out instructions ABOVE the hits list, but a failing tool is read from its tail - often literally, through
| tail.formatFailurenow repeats a fix/opt-out reminder AFTER the hits, with a unit test pinning its position.No in-file opt-out existed - the only escape hatch was the whole-PR hammer in the PR body. New, narrowest first, written in the file's own comment syntax (invisible when rendered in markdown/HTML):
issue-refs: exempt- that line onlyissue-refs: exempt-begin/issue-refs: exempt-end- a pasted block too dense to mark per lineissue-refs: exempt-file- a whole file (generated docs, imported archives); judged against file content (local: disk; CI: contents API, fetched only for files with hits), so it holds however small a later diffLine/block markers govern added patch lines by necessity (a patch-based gate cannot see markers outside the patch); an unclosed
exempt-beginswallows the rest of the file's added lines, mirroring the unclosed-fence rule. A backslash escape (\#NNN) was rejected: it edits the text it is meant to preserve, renders visibly outside markdown, and is an illegal escape in a Java string literal.The PR-body opt-out was one-sided.
bin/check-issue-refs.shnever read bodies, so an opt-out CI accepted left every local run red on the same file forever. The script now resolves the branch's PR viagh pr view, honours the opt-out exactly as the workflow does, and scans the body as a synthetic file too. Fetch failure (no PR yet, no gh, offline) degrades to the old body-less behaviour, and the message says so.Nothing tested the local script's plumbing. New
bin/test-check-issue-refs.shstubsghon PATH with eight end-to-end cases, runs inpr-checklist.ymlbeside the gate module's unit tests, and dogfoods the line marker on its own fixtures.Also carries the process learning this branch's own development produced:
docs/solutions/workflow-issues/red-proof-old-code-new-tests-2026-08-18.md(red-proofing without destroying uncommitted work - stash pop unstages, amend then captures only the message, checkout-pathspec clobbers silently), plus a "Red-proof" entry inCONCEPTS.md's Test reliability cluster.Defect-class check (same-defect search): the class is "guidance printed only where truncated reads never look". Checked the sibling gate:
changelog-ref-gate.js's failure message is short (no long hits list between guidance and tail), so it does not exhibit the failure at meaningful scale - left alone. The other checkers' guidance (bin/check-*.sh) print short failures; none found with a long-list-then-silence shape.Verification: gate module 59 unit assertions green (trailer position, line/block/file opt-out scope, case-insensitivity, body-adapter cases); harness 8/8 green; red-proof done properly - master's code against the new tests: 7 of 8 harness cases fail, unit run aborts; sigpipe guard green; actionlint green;
bin/check-issue-refs.shgreen on this branch's own diff; solution-doc claims validated mechanically and semantically.docs/issue-references.mdrewritten where this change falsified it ("purely textual, no API calls"; "the PR body ... does not exist when you run the script") and now documents all four opt-out scopes, narrowest first.Checklist
docs/features/- N/A - developer tooling, not a product feature🤖 Generated with Claude Code
https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn