Skip to content

fix(pm): a review of record on the card thread counts at the queue, and one constant holds both tools to it - #18738

Merged
os-justin merged 2 commits into
mainfrom
claude/issue-18701-review-of-record-thread
Sep 17, 2026
Merged

os-justin merged 2 commits into
mainfrom
claude/issue-18701-review-of-record-thread

Conversation

@os-justin

Copy link
Copy Markdown
Collaborator

Fixes #18701

Clause-②: no

The defect

check-clause2-carriers.mjs --template prints, and has always printed, that a review of record is ONE comment on the PR or its card; C6/C7 judge it on both threads through locateReviewOfRecord; the governed text says the same in four places. The Governed Surface Queue Guard's REFERENCES tier read the pull request's thread alone. The two tools already shared every recogniser — the heading marker, the head-sha span test, the newest-of resolution, the Reviewed-by: key line, the Served-tier: reader — and the one thing they did not share was the set of threads to run them over, which is the only fact a spelled-out copy of the filter could not copy. Measured cost, on one pull request in one afternoon: the skills seat posted its ## Contract review on carrier card #18426 (comment 5716694216, 15:10:43Z), --pair 18689 read it as C6-RECORD on the card thread, the merge_group leg (run 35238999808) refused with 0 comment(s) read on the PR thread, PR #18689 was dequeued CI_FAILURE at 15:36:18Z, and only a second copy of the same comment on the PR thread (5717108355) cured it.

The canonical sentence — the governed text is not silent

Read on origin/main (080688b561, the branch base). Quoted verbatim and untranslated, because rewriting a quoted ruling rewrites the ruling:

  • .claude/skills/pm-dispatch/references/contract-review.md :27 — 「复核记录 = 一条评论落 PR 或卡,达档与默认档同形;散文、dev 自评、os-dev-report 恒不算。」
  • same file :40 — 「落地前检三条:① 席内条款②复核 PASS 在案,即 PR 或卡上同形的复核记录(档位按实测)。」
  • same file :59 — 「PASS、FAIL 与作废都落 PR 或卡;……」
  • .claude/skills/pm-dispatch/SKILL.md 〈入队与落地〉 :640 — 「交付后复核归派发席:达档席内审,未达档循保险丝起子代理;记录 = 同形评论落 PR 或卡。」

(The claim comment's aside that references/contract-review.md carries no such sentence is the one premise this card's dispatch got wrong; :27 is that sentence, and it is on main. Nothing else in the premise moved — the guard, the template, the docblocks and the #18689 evidence all read as the card describes them.)

Which tool disagrees with it: scripts/pm/check-governed-queue-guard.mjs. So the GUARD moves, all of it. No governed file is touched by this PR.

Why the guard and not the template — the four axes

  • 实际业务需求. The record on the card is not a hypothesis: it is what a seat did, on 2026-09-17, following this repo's own --template. Both carriers are live on every pair — the clause-② declaration is hung on both, --pair judges both, the carrier label is stripped from both — and check-clause2-carriers has read both threads since finding(pm-dispatch): after #17285, default-tier lanes have no named "review of record" — five Clause-②: yes merges in one window carry none, and clearing the carrier is indistinguishable from never reviewing #17302. The population that would be served by moving the template instead is empty: nobody is asking for the card to stop counting.
  • 项目长远合理性. Contract-first. The governed text is the contract and the two tools are its consumers; a consumer reading a narrower set than the contract declares is the defect, and the repair belongs at the consumer, not at the contract. 基本裁决原则 puts 声明 above 实现 above 文档面 — moving the template would be narrowing the declaration to match an implementation gap, which is the one direction that rule forbids.
  • 防 AI 写代码犯错. This is a declared-but-not-honoured capability in its purest form: an instruction tells an author where to put an artifact, and a gate silently does not look there. The structural repair is not "say the same thing in more places" — it is ONE exported constant that the instruction and the reader both derive from, plus a pin that reds if either is edited alone. Moving the template would leave three more hand-written location sentences (the guard's option-3 text and two docblocks) free to drift the same way one layer along.
  • 创业阶段不扩散需求. Moving the guard is the SMALLER change and adds no capability: the helper already exists, the card number is read off the pull object the head read already fetches (zero extra requests for it), and at most one extra thread read is bought — only for a references-tier pull request with no authorized approval, a population refused outright today, so nothing that passes now pays anything. Moving the template would be a RETIREMENT of a carrier the protocol uses everywhere else, across governed files, which is a maintainer's ruling and not a dev PR.

No axis conflicts with another. A fifth, decisive fact: the template side cannot move from here at all — its sentences live in .claude/skills/pm-dispatch/**, off this card's file surface and governed.

The shared constant, and one derivation of "the record on this head"

check-clause2-carriers.mjs — the file that owns the reader — now declares the thread set once:

export const REVIEW_OF_RECORD_THREADS = Object.freeze([
  Object.freeze({ where: 'PR',   rows: 'prComments',   number: 'pr',   words: 'the PR' }),
  Object.freeze({ where: 'card', rows: 'cardComments', number: 'card', words: 'its card' }),
]);
export const REVIEW_OF_RECORD_LOCATION = REVIEW_OF_RECORD_THREADS.map((thread) => thread.words).join(' or ');

Three consumers, all derived from it and none spelling a set of its own:

  1. locateReviewOfRecord builds the rows it searches, its gap sentences and its per-thread read counts from the list. --template's and --pair's output is byte-identical to main's.
  2. contractReviewTemplateLines builds its printed sentence from REVIEW_OF_RECORD_LOCATION, so an instruction can never offer a thread no reader searches.
  3. check-governed-queue-guard.mjs fetches one thread per entry in the loaded list — and its whole record reading is now locateReviewOfRecord, imported through the existing lazy loader rather than re-spelled. The guard's own two-line filter is gone; what is left there is the tier gate on top of the located record (Served-tier: stands, and the [finding] The mandated Served-tier: top line puts a model identifier in a GitHub comment, which AGENTS.md:442 forbids in terms — and a p1 guard change would cement it into a gate #18060 model-identifier refusal), which is that leg's own fact.

The card the record may live on is derived, never configured: deliveredCardNumber(pull) enumerates the numbers the body or branch could be naming and asks deliveryEvidence — the one relation derivePairs, H8 and H31 already pair a PR with its card by — so it can never accept a card that relation rejects. A pull request naming no card, or two cards at equal strength, searches NO card thread and the refusal says so: the refusing direction, the only one a governed reading may be wrong in. The card's number and the head sha come from ONE GET /repos/{owner}/{repo}/pulls/{number} — the request the head read already made — so the card thread costs no extra call for the number, and the guard keeps its zero-write, event-payload-only posture with no checkout.

REVIEW_OF_RECORD_LOCATION is mirrored at the guard's module scope for the renderer only (renderGuardVerdict is pure and synchronous, and a module-scope import of the sibling is the measured exit-13 deadlock its own docblock records). The SET the leg acts on is never a mirror — it is the loaded list — and the mirror's words are pinned to the constant in the guard's battery.

The pin

check-clause2-carriers.mjs owns the reader, so the pin lives in its self-test — and it is a measurement, not two lists written side by side:

  • STATED — the locations read out of the text contractReviewTemplateLines() actually prints.
  • READ — obtained by driving check-governed-queue-guard.mjs's runGuard once per entry in REVIEW_OF_RECORD_THREADS, with a record built from this file's own contractReviewRecordLines() sitting on that thread alone, and collecting the threads whose run answers exitCode === EXIT_CLEAR with record.state === 'stands'.

The case asserts stated === read === the constant's own set, plus its own controls: the set has two locations (not vacuous); each run located the record on the thread it was posted to; the guard fetched exactly one thread per entry, no more and no fewer; and a run with the record on NEITHER thread is refused, so the pin can fail. selfTest() became async to take that lazy import — the dispatch uses .then(...), never a top-level await.

Counterfactual — the #18689 dequeue, offline

One fixture, driven through runGuard with no network and no git: a references-tier pair whose record sits only on the card thread. The same script runs against either generation (it passes both the old fetchPullHead and the new fetchPull).

exitCode conclusion record.state record.where
before (080688b561) 3 REFUSED refused absent
after 0 cleared stands card

Before, rendered: ⛔ NO review of record on this head either (1 comment(s) read on the PR thread) — the #18689 sentence, reproduced.
After, rendered: ✅ review of record on this head: comment 5716694216 (2026-09-16T15:10:43Z) on the card thread is a ## Contract review comment naming ...

Ablation

Two legs, each: on-disk proof of the mutation (grep -c of the removed text and of the injected text, plus the git hash-object blob before and after), then the self-tests, then restore, then a byte-identity check against the HEAD blob. Both scripts carry trap restore EXIT INT TERM with an absolute path. No build or dist/ is involved — these two files are run directly by node and by package.json, so the on-disk proof is the whole preflight.

Leg A — the loop that unifies the thread set (for (const thread of recognisers.threads) narrowed to the first entry). Blob 9a111f6cb57227d0; removed-text count 1 → 0, injected-text count 0 → 1. Cross-tool pin: 4 of 802 failed. Guard battery: 25 of 253 failed — the loop is load-bearing for the whole references leg, not only for the card.

Leg B — the line that supplies the card's number (card: card.cardcard: null), which reproduces the pre-#18701 thread set exactly. Blob 9a111f6c9d935d92; counts 1 → 0 and 0 → 1. This is the clean reading the card asks for:

  • new pin RED: check-clause2-carriers 3 of 802 failed, all three the new battery — stated=[PR,card] read=[PR], the card run locating nothing, and the fetch list [[4101],[4101]] instead of [[4101,4102],[4101,4102]].
  • new guard cases RED: check-governed-queue-guard 5 of 253 failed, all five in the #18701 battery (record on the card alone, both threads fetched, the rendering naming the thread, newest-governs across threads, the branch-name fallback).
  • old cases GREEN: 799 of 802 and 248 of 253 — every pre-existing battery in both files, #18020 included, passes under the ablation.

Restore, both legs: worktree blob 9a111f6c… equals the HEAD blob, git diff HEAD empty, git status --porcelain empty, and both self-tests back to exit 0.

Self-tests

family before after
node scripts/pm/check-governed-queue-guard.mjs --self-test 239 cases, 21 batteries declared 253 cases, 22 batteries — new: ⭐ #18701: the record lives on the PR or its card, and BOTH are read (14, floor 14); SELF_TEST_BATTERY_FLOOR 20 → 21
pnpm check:pm-clause2-carriers 786 cases, 28 batteries declared 802 cases, 29 batteries — new: #18701: ONE thread set -- what the template STATES is what the queue guard READS (16, floor 16); SELF_TEST_BATTERY_FLOOR 27 → 28

Both exit 0. --template, --help and the no-event refusal path (exit 1, its own message) all still answer.

Gates

node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack derived from the worktree with no hand-fed path list (change set: the two scripts, 2 committed / 0 working tree / 0 untracked, three-dot against merge base 080688b56). 36 derived, 36 run, every one exit 0, reconciled with --ran after a fresh git fetch origin main✓ dispatch-gates --ran: 36 derived famil(ies) accounted for — 36 run, 0 NOT-MEASURED (a DERIVED zero — all 36 recorded an exit code and none of them is 3).

node scripts/check-adr-0087-registration.mjs --base origin/main :: exit 0
node scripts/check-adr-0087-registration.mjs --self-test :: exit 0
node scripts/check-changeset-no-major.mjs --base origin/main :: exit 0
node scripts/check-changeset-no-major.mjs --self-test :: exit 0
node scripts/check-ci-filter-parity.mjs :: exit 0
node scripts/check-closing-keyword-parity.mjs :: exit 0
node scripts/check-closing-keyword-parity.mjs --self-test :: exit 0
node scripts/check-comment-mask-corpus.mjs :: exit 0
node scripts/check-declaration-mirrors.mjs :: exit 0
node scripts/check-declaration-mirrors.mjs --self-test :: exit 0
node scripts/check-scripts-symbol-anchors.mjs :: exit 0
node scripts/check-scripts-symbol-anchors.mjs --self-test :: exit 0
node scripts/check-self-test-wired.mjs :: exit 0
node scripts/check-self-test-wired.mjs --self-test :: exit 0
node scripts/check-self-test-workflow-commands.mjs :: exit 0
node scripts/check-self-test-workflow-commands.mjs --self-test :: exit 0
node scripts/check-whole-set-label-write.mjs :: exit 0
node scripts/check-whole-set-label-write.mjs --self-test :: exit 0
node scripts/pm/bare-root-worklist.mjs --self-test :: exit 0
node scripts/pm/check-governed-queue-guard.mjs --self-test :: exit 0
pnpm check:agent-test-spelling :: exit 0
pnpm check:bash32-floor :: exit 0
pnpm check:changeset-gate-self-tests :: exit 0
pnpm check:cli-command-ids :: exit 0
pnpm check:closing-target-claim :: exit 0
pnpm check:cross-package-test-inputs :: exit 0
pnpm check:driver-memory-census :: exit 0
pnpm check:entry-guard :: exit 0
pnpm check:nul-bytes :: exit 0
pnpm check:parse-guard :: exit 0
pnpm check:pm-clause2-carriers :: exit 0
pnpm check:pm-dispatch-gates :: exit 0
pnpm check:pnpm-filter-targets :: exit 0
pnpm check:ratchet-remedy-authority :: exit 0
pnpm check:refd-timer-probe :: exit 0
pnpm check:watch-hint-literal :: exit 0

Repo-wide pnpm lint (eslint . --no-inline-config, the whole tree, as PR #18654 did) :: exit 0, no output. Every exit code above was captured with redirect-then-$?, never across a pipe.

origin/main moved to 62bce5c297 while this ran; its three commits touch packages/** and .changeset/** only — nothing under scripts/** and nothing this diff reads — so no merge was taken and the derivation above is unaffected.

Scope

Two files, scripts/pm/check-governed-queue-guard.mjs and scripts/pm/check-clause2-carriers.mjs. No governed file. skip-changeset: nothing published moves — scripts/pm/** is not in any package's files[].

⚠️ The card read this PR adds is built on readCardComments's paged sibling, not on the un-paged path #18683 names. The guard's own makeCommentReader is paginated (100 per page, 10 pages, and it THROWS rather than truncating), and it is what serves both threads here — GET /repos/{owner}/{repo}/issues/{number}/comments is the same route for a pull request and for a card. #18683 remains exactly as it was, on check-clause2-carriers' own reader, untouched by this PR.

Not in scope, as dispatched: check-clause2-carriers' un-paged readCardComments (#18683 — read, not fixed); #18536's machine-side lane/tier re-keying; the REFERENCES-tier rule itself (which paths qualify), untouched.

Acceptance notes

  • deliveryEvidence's ranking had no exported ordering, so DELIVERY_EVIDENCE_PRECEDENCE is declared in check-clause2-carriers.mjs as a MIRROR and measured against that function pair by pair in the self-test rather than trusted. If check-half-states.mjs ever wants to own it, that is a one-line move; noted, not filed — no PR or person reaches for a ranking that today has exactly one caller.
  • The guard no longer loads check-half-states.mjs directly: every recogniser it reads now arrives through the file that composes them, and a rename upstream stops THAT module loading, which the leg already turns into a fail-closed refusal with the loader's own reason. RECOGNISER_SOURCES is down to one entry and the self-test pins that. Keeping the old direct load would have been a check on something this file no longer reads.

Generated by Claude Code

…d too

The governed text puts the review of record 「一条评论落 PR 或卡」; the queue
guard's references tier read the PR thread alone. Both tools now run one
reader over one thread set, derived from a single exported constant.

Claude-Session: https://claude.ai/code/session_01Gqi43smmqjJ5sUrhfoPeKu
Co-authored-by: Claude <noreply@anthropic.com>
The cross-tool pin drives `check-governed-queue-guard.mjs` once per entry in
`REVIEW_OF_RECORD_THREADS`, with a record copied from this file's own
`--template` sitting on that thread alone, so editing either side by itself
reds. Adds the guard-side card-thread battery and the card derivation cases.

Claude-Session: https://claude.ai/code/session_01Gqi43smmqjJ5sUrhfoPeKu
Co-authored-by: Claude <noreply@anthropic.com>
@os-justin os-justin added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Sep 17, 2026 — with Claude
@os-justin
os-justin marked this pull request as ready for review September 17, 2026 18:05
@os-justin
os-justin added this pull request to the merge queue Sep 17, 2026
Merged via the queue into main with commit a84f61a Sep 17, 2026
37 checks passed
@os-justin
os-justin deleted the claude/issue-18701-review-of-record-thread branch September 17, 2026 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/l skip-changeset PR has no user-facing published change; bypasses the changeset gate

Projects

None yet

2 participants