fix(review,work-items): close the two blockers the #2933 close-out review found - #3083
Conversation
…t found, and four recorded gaps The full close-out review of container #2933 (the real 20/20 pass, not the 19/20 dry run) returned FAIL with two `wrong` findings against stated acceptance criteria. Both are closed here, along with four non-blocking gaps the standing instruction says to work rather than file. BLOCKER — close-out.md's own basis rule dropped code-shipping sub-items. Rung 1's empty-result branch read "a successful query returning zero merged PRs means that sub-item closed without shipping code ... Only a failed query falls to rung 2." But an empty rung 1 means only that no PR named the item with a CLOSING KEYWORD, and two very different situations produce that: the item shipped nothing, or it shipped under `Refs #N` — which work-items' own work/SKILL.md explicitly sanctions, and which is the normal shape when one PR advances several items while closing only the spin-offs it resolves. Everything in the second case was classified no-code and silently dropped while the report still claimed to cover the shipped whole. An empty rung 1 now falls to rung 2 as well; no-code requires both rungs empty; the verdict names which rung resolved each item. The mode found this by reviewing the container that shipped it — #3027's dogfood criterion working exactly as intended. On #2933's own close-out, #3056 carried Closes for three spin-offs only and #3067/#3071 carried no closing keyword at all, so three sub-items that between them shipped 83 file-touches of adapter and generator code would have been dropped from the basis of the review deciding whether that container could close. BLOCKER — the container's record was a sub-item behind. #2946 closed today with a fourth disposition, but aihero-shipping-course.md still asserted "Issue #2946 remains open", which under archival-by-closure means closing on a record that contradicts the tracker. Corrected, and deliberately as a correction-in-place rather than a silent overwrite, since that section is the durable record of the adapter track's scope decisions. Recorded rather than filed, per the standing no-follow-up-issues rule: The Linear schema check is now committed at adapters/linear/schema-check/. It justified descoping #2946's live conformance and existed only as a session artifact, so the claim could not be re-run by anyone. Ships validate.mjs, the negative control that makes a green run mean something, fidelity.sh proving the checked operations are the adapter's own text, and a fetch-schema.sh that pulls the SDL rather than vendoring 1.2 MB. Verified by running it: 18/18 operations valid, 10/10 injected faults caught, 11/11 strings verbatim. fidelity.sh earned its place immediately by catching that the harness still expected the pre-0.39.9 team.labels query. tracker-seam.md now names the item-content-trust boundary where it teaches body reads — no live surface was unguarded, but the document a NEW surface consults did not mention that what comes back is untrusted. CONTRACT.md records the #2945 role-split topology decision. #2951 was closed not_planned on the strength of it while it lived only in a sub-issue comment, which this plugin's own disposable-tickets doctrine says is the wrong place. States plainly that nothing implements `sources` today. The README's synonym claim is scoped to the skills it is true of. Stuffing ticket/issue into an adapter generator's triggers to satisfy a fleet-wide sentence would buy a tidier claim at the cost of worse routing. work-items 0.39.9 → 0.39.10; review 0.26.2 → 0.26.3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
|
Claude finished @kyle-sexton's task in 1m 51s —— View job Claude is working on this
Reviewed the diff directly (no 
|
|
Last security-reviewed head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef6a92c6f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Claude finished @kyle-sexton's task in 4m 11s —— View job Claude's code review
This repo has a 4 findings posted inline, all on lines this PR touched or added:
Everything else — the two blocker fixes ( Branch: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…ncile rung 2 Five review findings on #3083, all correct, and three of them are the same mistake I have spent this branch calling out in other people's code. Every script in the schema-check harness printed FAIL / MISSED / MISMATCH and then EXITED 0. No caller — a shell, CI, a future close-out — could tell a passing run from a failing one. I committed three checks that cannot go red, ran them, saw green, and reported that as verification. A check that cannot fail is exactly the vacuous green this seam has spent three PRs eliminating. All three now return 1 on failure. fidelity.sh had a deeper hole: it matched each operation against the ADAPTER only, never against validate.mjs. So validate.mjs could have held a different — and still schema-valid — query while both scripts stayed green and the adapter's real request went unvalidated, which defeats the entire guarantee the script advertises ("the thing validated IS the thing sent"). It now requires each operation on BOTH sides, and covers the multi-line operations whitespace-normalized rather than merely printing them. That raised coverage from 11 single-line literals to 16 operations, and needed an unescaping step because the adapter writes its sigils as \$ inside double-quoted shell strings while validate.mjs holds them bare. close-out.md's rung 2 contradicted the rung-1 fix from the previous commit. Rung 1 now says no-code when both rungs come back empty; rung 2 still said no surviving hit means unresolved, which escalates to rung 3 and can stop a close-out. Two sections prescribing opposite outcomes for the exact case the fallback exists to preserve. Rung 2 now classifies by WHY rung 1 was empty: succeeded-and-empty plus rung 2 empty is no-code and does not escalate; FAILED plus rung 2 empty is unresolved and does, because then nothing has actually looked successfully. Verified by breaking each check deliberately: an invalid field in validate.mjs, a neutered fault in negative.mjs, and a validate.mjs query that no longer matches the adapter — each returns 1, and a clean run still returns 0. That is the check I skipped the first time. Versions unchanged: 0.39.10 and 0.26.3 have not shipped, so their entries are extended rather than superseded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
|
Claude finished @kyle-sexton's task in 46s —— View job Claude's security review
Skip gate hit — no findings to post. Per
None of this touches authentication, authorization, secrets/credentials, network input handling, or GitHub Actions workflow config. The new Per skip gate condition 2 (no security-relevant surface in the diff under review), no inline findings are posted. |
|
Claude finished @kyle-sexton's task in 3m 42s —— View job Claude's code review
This lane is scoped to correctness/design/conventions/docs (security is a separate lane per What
4 findings posted inline, all repeats of findings from the prior review pass that the follow-up commit didn't address:
Everything else (the rung-1 basis-rule fix, the SSOT correction-in-place, Branch: |
…count in the drift paragraph Round three on #3083. All four correct. The SSOT said "all 17 operations validate clean" where the harness now has 18 — written in the very paragraph arguing that a durable record which contradicts reality is what archival-by-closure cannot survive. Same failure mode, one paragraph later, in my own prose. The 17 in the 0.39.9 CHANGELOG entry is left alone: that entry describes the state at that version, where 17 was correct. close-out.md's rung 2 ENTRY sentence still read "The rung-1 query failed:" while the rule two paragraphs up now says an empty rung 1 falls through too. The previous commit fixed the classification bullets and left the sentence that introduces them, so a reader landing on Rung 2 met the pre-fix framing immediately below the fix. fetch-schema.sh pointed a failing contributor at /root/.ccr/README.md — a path from this authoring session's own sandbox, which exists in no contributor checkout or CI runner. Replaced with advice that is true anywhere. fidelity.sh extracted WIT_LINEAR_ISSUE_FIELDS, printed it, and compared it to nothing. That block is the shared field selection which fetch_issue, list-items and list-sub-items all interpolate rather than spelling out, so the three highest-traffic reads rested on a human noticing a difference between two echo blocks — while the README claimed every operation was checked. Now asserted against validate.mjs's F and contributing to ok/bad: 17 checks, and verified discriminating by drifting F on one side only (exit 1, restored exit 0). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
…g-breakdown-ukohlx # Conflicts: # plugins/review/CHANGELOG.md
|
Claude finished @kyle-sexton's task in 1m 12s —— View job Claude's security review
Skip gate hit — no findings to post. Per
None of the in-scope changes touch authentication, authorization, secrets/credentials, network input handling, or GitHub Actions workflow config. This is a documentation-reconciliation and offline-validation-tooling change only. Per skip gate condition 2 (no security-relevant surface in the diff under review), no inline findings are posted. Branch: |
|
Claude finished @kyle-sexton's task in 2m 15s —— View job Claude's code review
This lane is scoped to correctness/design/conventions/docs (security is the separate The only substantive new content is commit
No new findings. Plugin versions and CHANGELOG headers are consistent ( Branch: |
2 participants
No linked issue
Summary
The full close-out review of container #2933 — the real 20/20 cumulative pass, which the
earlier 19/20 run was gated out of by
close-out.md's own Step 4 — returned FAIL with twowrongfindings against stated acceptance criteria. Both are closed here, together with fournon-blocking gaps that the standing no-follow-up-issues rule says to work rather than file.
work-items0.39.9 → 0.39.10;review0.26.2 → 0.26.3.Fix
Blocker 1 —
close-out.md's own basis rule dropped code-shipping sub-items from the basis.Rung 1's empty-result branch read: "a successful query returning zero merged PRs means that
sub-item closed without shipping code … Only a failed query (non-zero exit) falls to rung 2."
But an empty rung 1 means only that no PR named the item with a closing keyword, and two very
different situations produce that: the item genuinely shipped nothing, or it shipped under a
Refs #Nreference. The second is sanctioned, not an oversight —work-items' ownwork/SKILL.mdstates that "an intentionalRefs #Nopt-out does not exclude its issue" — and itis the normal shape whenever one PR advances several items while closing only the spin-offs it
fully resolves. Everything in that case was classified
no-codeand silently excluded while thereport still claimed to cover the shipped whole.
An empty rung 1 now falls to rung 2 as well;
no-codeis reached only when both rungs come backempty; and the verdict names which rung resolved each item, with rung 2 still flagged heuristic.
The mode found this by reviewing the container that shipped it — #3027's dogfood criterion
("the container can run it against itself") working exactly as intended. On #2933's own close-out,
PR #3056 carried
Closesfor three spin-offs only and PRs #3067 and #3071 carried no closingkeyword at all, so rung 1 came back successful-and-empty for three sub-items (#2946, #2950, #2952)
that between them shipped 83 file-touches of adapter and generator code — which would have
been dropped from the basis of the review deciding whether that container could close.
Blocker 2 — the container's durable record was one sub-item behind. #2946 closed with a fourth
disposition, but
docs/upstream/aihero-shipping-course.mdstill asserted "Issue #2946 remainsopen". Under archival-by-closure the container body and its SSOT are the archive, so closing
on a record that contradicts the tracker is precisely what that model cannot survive. Corrected —
and deliberately as a correction-in-place rather than a silent overwrite, since that section is
the durable record of the adapter track's scope decisions.
Recorded rather than filed (these would normally be follow-up issues):
adapters/linear/schema-check/. It is what justifieddescoping Linear adapter with full verb parity #2946's live conformance, and it existed only as a session artifact — so the claim
could not be re-run or regression-guarded by anyone. Ships
validate.mjs, thenegative.mjscontrol that makes a green run mean something,
fidelity.shproving the checked operations arethe adapter's own text rather than a paraphrase, and
fetch-schema.shthat pulls the SDL ondemand rather than vendoring 1.2 MB of upstream text. Verified by running it: 18/18 operations
valid, 10/10 injected faults caught, 11/11 strings verbatim.
fidelity.shearned its placeimmediately — it caught that the harness still expected the pre-0.39.9
team.labelsquery.tracker-seam.mdnames the item-content-trust boundary where it teaches body reads. No livesurface was unguarded, but the document a new surface consults when adding a body read never
mentioned that what comes back is untrusted, so the link ran one way only.
CONTRACT.mdrecords the Investigation: multi-provider topology — source-of-record + coordination split #2945 role-split topology decision. Jira write support — guarded, opt-in #2951 was closednot_plannedon the strength of it while it lived only in a sub-issue comment — which this plugin's own
disposable-tickets doctrine says is the wrong place for a decision. The new section states
plainly that nothing implements
sourcestoday and marks building it demand-gated.ship,onboard-adapterand
setupcarry neither token; stuffing ticket/issue into an adapter generator's triggers tosatisfy a fleet-wide sentence would buy a tidier claim at the cost of worse routing.
Verification
close-out.md'srung-1 paragraph, and the SSOT's "remains open" line. The linkage claim that makes blocker 1 bite
was confirmed directly —
#3056's squash body carries onlyCloses #3046/#3047/#3048, and#3067/#3071carry no closing keyword.a bound without a test, so shipping an unexercised check would have repeated the mistake.
.gitignoreverified by staging: only the five source files are tracked; the 1.2 MB SDL andnode_modulesare not.shellcheck -x,shfmt -d,typos,markdownlint-cli2, changelog parity and--check-bump origin/mainall clean.#NNNNat linestart turns it into an H1. Issue refs in the affected paragraphs are backticked, with a note
saying why.
Related
Refs-linked sub-items blocker 1 would have droppedCONTRACT.md, and the item closed on it41f88c02; the schema check now committed here is what validated it🤖 Generated with Claude Code
https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
Generated by Claude Code