Repository navigation
docs+ci: run the issue-reference gate locally, and always record a mirror upstream title - #218
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @astubbs's task in 3m 26s —— View job Code Review
Overall: solid, low-risk change. Good instinct not reimplementing the regex — A couple of edge cases worth knowing about (none blocking):
Diff-mechanics check: Verification note: I wasn't able to execute the script in this sandbox (bash/node invocations required interactive approval that wasn't available), so the above is from static reading of Docs ( No bugs, security issues, or missing test coverage found that would block merge. The "N/A" test rationale is reasonable given the wrapper holds no independent rule logic. |
✅ 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). |
|
Claude finished @astubbs's task in 3m 36s —— View job Code Review
Summary: solid change, low risk. One finding worth the author's attention before merge; nothing blocking. Finding: PR description doesn't cover half the diffThis branch carries two commits, and only the first is described:
The PR body's Description section only talks about the CI-gate script; the mirror-format/ledger-retirement work isn't mentioned anywhere. That also makes the checked box "Title & body reflect the final content of this PR" inaccurate as the PR currently stands — worth either expanding the description to cover the second commit, or splitting it into its own PR, since the two changes are unrelated (CI tooling vs. issue-mirroring documentation convention). I checked the second commit isn't just scope creep dressed up — it's internally consistent:
So the content is fine; it's the description/title accuracy that's the actual gap. Technical review of
|
astubbs
changed the title
|
Both carried-over points are valid and both are now fixed in 1. Rewritten to say what the guarantee actually is, since "stricter in the safe direction" is only reassuring if you know which direction that is:
The same caveat is now in the script's own header, under 2. Added as a separate commit rather than amended, so the reviewed commits aren't rewritten underneath the review and this diff shows exactly what the feedback caused. Happy to fold it into Still open from the checklist and not silently dropped: whether this wants a |
…tion Both from review of #218. Neither changes behaviour; both are the docs describing the script more confidently than the script earns. "Reports exactly what the gate will" was not true in one narrow case. The *rule* is shared - same issue-ref-gate.js module, so it cannot drift - but the *input* is not. CI reads patches from GitHub's `pulls.listFiles`, which omits `patch` on a very large diff, and the gate skips a file it cannot see; this script builds its own patch with `git diff` and still checks it. The divergence is one-directional, which is worth stating rather than hiding: a red result here is always real, a green one is not a promise that CI examined every file. It can flag what CI would silently pass, never the reverse. The exit-code comment claimed 2 meant "cannot run (no node)", but there are two exit-2 sites - a missing node and a missing merge base (a shallow clone reaches the second). Someone scripting around the exit codes would have been told only half of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 1m 5s —— View job Code Review — follow-up on
|
The `PR Checklist` workflow fails a PR that adds an unqualified `#NN` below #1000, but the rule was only reachable from CI. The first sign you had written a bare `#118` was a red check minutes after pushing - a slow loop for a one-character fix. It cost two CI round-trips on #217 alone. bin/check-issue-refs.sh applies the same rule as the gate, because it calls the same .github/scripts/issue-ref-gate.js module. A bash re-implementation of the regex would drift, and a local check that disagrees with CI is worse than no local check: it teaches you to trust the wrong answer. Everything subtle - exempt paths, `{@link #close()}` false positives, the markdown/anchor/URL forms that already qualify a number - stays in the module, unit tested by issue-ref-gate.test.js. The rule is shared; the input is not, and the difference is documented rather than glossed. CI reads patches from GitHub's `pulls.listFiles`, which omits `patch` on a very large diff, and the gate skips a file it cannot see, while this script builds its own patch with `git diff` and still checks it. The divergence is one-directional: a red result here is always real, a green one is not a promise that CI examined every file. It can flag what CI would silently pass, never the reverse. It judges the working tree rather than HEAD, following bin/check-copyright-headers.sh, so uncommitted edits are caught before the commit rather than after the push. Exit 2 covers both ways it can fail to run - node missing, and no merge base with the base ref (a shallow clone reaches the second). Verified against the real failure: reintroducing the bare `#118` that failed CI on #217 reproduces the gate's finding locally - same file, same ref, same line. Both exit-2 paths, the optional base-ref argument, and running from an unrelated working directory were each exercised too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ledger entry Mirror titles started as upstream's own, and many of them name only where a failure surfaced - #118 was "Error in onPartitionsAssigned in parallel consumer", which contains no term anyone hitting it would search for. Those are ours to rewrite once the cause is known, and #118 now is. That makes the upstream title worth recording in the body. The rule is deliberately unconditional rather than "record it when you retitle": the conditional version needs whoever retitles to remember, and it leaves a reader unable to tell whether any given mirror's title is upstream's words or ours without opening upstream to compare. Recording it always makes the mirror self-describing. No bulk backfill - add the line when you touch a mirror. Also retires docs/inflight/parked-upstream-issue-mirroring.md. The work landed on 2026-08-05 (all 78 mirrored and backlinked), and inflight files do not outlive their work - AGENTS.md's own doc table says they are deleted, never archived into an index. Nothing is lost: it was a 27-line ledger entry pointing at the 459-line plan, which is kept untouched in docs/plans/ and holds every decision it summarised, including the bulk-vs-incremental rationale it existed to record. The format rules were the only content it uniquely held, and they move to AGENTS.md -> Mirror format, where someone editing a mirror will actually meet them. The plan's "Ledger entry:" link is repointed rather than left dangling. The mirror body template comes along with it. It had been living only in the plan, which is a dated record rather than durable reference, so the single written copy of "what a mirror looks like" was in a document that is allowed to go stale - and had: it proposes a confluentinc#NNN: title prefix, while the import shipped upstream #NNN:, which is what all 78 mirrors use. The live copy is now in AGENTS.md, with the upstream-title line included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
783d048 to
2124ea2
Compare
#218 landed a local runner for the gate while this branch was open. It is the natural way to reproduce the link-form false positives, so the note should name it rather than leaving the next reader to push and wait for CI.
Three shapes made stripQualified report a number as unqualified when the URL beside it already named the repo: - asciidoc `URL[link text]` - the bare-URL rule matches \S+, so it consumed ".../issues/65[Enhanced" and left "retry epic #65]" looking like loose prose. The new rule runs BEFORE the bare-URL one, or the URL eats the opening bracket first and there is nothing left to recognise. - asciidoc `{attr}/path[link text]` - no http prefix at all, so nothing stripped it. This is how the README writes upstream links. - an html anchor wrapped across two lines - suspectRefs works a line at a time, so the existing single-line rule never sees a complete element. Each half now strips only when its counterpart is absent, which leaves a complete anchor to the original rule and keeps a #NN sitting OUTSIDE the element visible. This was found by the tree-wide qualification sweep in this PR, which had to work around it: the qualifier had to sit immediately before the number, so the README gained "[PR confluentinc#291 ...]" where "[confluentinc PR #291 ...]" is the more natural house form and would have failed. With the gate fixed those six README references and one Java anchor are correctly seen as already qualified, so README.adoc, README_TEMPLATE.adoc and ParallelEoSStreamProcessorTest revert to untouched. The sweep now changes only genuinely ambiguous references - 366 of them. Stripping MORE is the direction that produces false negatives, and AGENTS.md is explicit that a wrong reference which resolves is worse than a broken one. So the fixtures cover both directions: link macros are accepted, and a bare ref elsewhere on the same line, or after a split anchor closes, is still flagged. 24 assertions pass. docs/inflight/ci-issue-ref-gate-link-forms.md is deleted - it recorded this as a follow-up while #218 was open against the gate's area. That PR has merged, so the reason to defer is gone.
1 participant
Description
Two conventions that existed only in places you had to already know about. Both are now where you meet them by working. Independent commits — rebase-merge, don't squash.
1.
bin/check-issue-refs.sh— run the issue-reference gate before pushingThe
PR Checklistworkflow fails a PR that adds an unqualified#NNbelow #1000, but the rule was only reachable from CI. The first sign you had written a bare#118was a red check minutes after pushing — a slow loop for a one-character fix. It cost two CI round-trips on #217 alone.No second copy of the rule. The script calls the same
.github/scripts/issue-ref-gate.jsmodule the workflow requires, rather than reimplementing the regex in bash. A bash version would drift, and a local check that disagrees with CI is worse than no local check: it teaches you to trust the wrong answer. Everything subtle — exempt paths,{@link #close()}false positives, the markdown/anchor/URL forms that already qualify a number — stays in the module, unit tested byissue-ref-gate.test.js.Judges the working tree, following
bin/check-copyright-headers.sh, so uncommitted edits are caught before the commit rather than after the push. Exit 2 when node is absent, matching the "cannot run" convention of the sibling scripts. Theissue-refs: N/Aopt-out lives in the PR body, which a local script cannot see — so it says so in its output rather than pretending to honour it.Verified against the real failure: reintroducing the bare
#118that failed CI on #217 reproduces the gate's finding locally — same file, same ref, same line.2. Mirrors record their upstream title, always
#118 was mirrored as "Error in onPartitionsAssigned in parallel consumer" — upstream's own words, naming where a failure surfaced and containing no term a sufferer would search for. Once #217 established the cause, it was retitled to lead with the error string.
That makes the upstream title worth recording in the body. Unconditionally, not "when you retitle": the conditional version needs whoever retitles to remember, and it leaves a reader unable to tell whether any given mirror's title is upstream's words or ours without opening upstream to compare. No bulk backfill of the other 77 — add the line when you touch one.
AGENTS.mdgains a Mirror format section carrying the live spec: title form, the header-block template, summary-not-verbatim, no@mentions, labels, and fully-qualified cross-repo refs.Retires
docs/inflight/parked-upstream-issue-mirroring.md. The work landed 2026-08-05 (all 78 mirrored and backlinked), andAGENTS.md's own doc table says inflight files are deleted when their work lands, never archived into an index. Nothing is lost — it was a 27-line ledger entry pointing at the 459-line plan, which is kept and holds every decision it summarised, including the bulk-vs-incremental rationale. Its format rules were the only content it uniquely held.The plan doc is updated rather than left stale. Its own Mirror format section was the single written copy of "what a mirror looks like", in a document explicitly allowed to go stale — and it had: it proposes a
confluentinc#NNN:title prefix, while the import shippedupstream #NNN:and all 78 mirrors carry that. That divergence was recorded nowhere. It is now annotated as superseded at point-of-use (following the plan's existing strikethrough/withdrawn pattern), logged in What actually happened, and itsLedger entry:link repointed instead of left dangling. The proposal text is left unedited — a dated plan records the decision, it is not a place to retcon it.Checklist
AGENTS.mdgains Mirror format and an Issue references pointer to the new script; the mirroring plan is annotated where it had gone stale.N/A. The wrapper holds no rule logic; the rule lives inissue-ref-gate.js, already unit tested byissue-ref-gate.test.js. Verified behaviourally by reproducing the fix(core) #118: foreign offset metadata no longer kills the consumer on assignment #217 failure locally. Happy to add abin/test-check-issue-refs.shharness to matchtest-check-copyright-headers.shif preferred.N/A, no CI runners or workflows touched; this adds a script CI does not invoke.🤖 Generated with Claude Code
https://claude.ai/code/session_01BK8ufGWcjXWJNyQ7WUzWer