Skip to content

feat(source-control): judge a review lane by what it produced, not by its check row - #4106

Merged
kyle-sexton merged 4 commits into
mainfrom
claude/architecture-improvements-hqfc0i
Sep 11, 2026
Merged

feat(source-control): judge a review lane by what it produced, not by its check row#4106
kyle-sexton merged 4 commits into
mainfrom
claude/architecture-improvements-hqfc0i

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

No related issue: this is the agent-side compensation for the CI defect filed as
#4093, which it deliberately does not close. The lanes stay broken until that
issue's acceptance criteria are met; this only stops the PR monitor from reading
their green rows as evidence.

Summary

An AI-review lane reports on its session, not on its output. A session
that ends without error concludes success even when it posted no review body and
produced no finding, so the check row goes green and nothing downstream can tell
"reviewed and found nothing" apart from "never reviewed". The PR monitor's
per-iteration checklist classified such a lane PASS on its check row alone, and
the readiness gate cleared on that.

This reproduced on this pull request while it was open: both claude[bot] lanes
concluded success and posted zero reviews, on every head. Every external
finding here came from Codex.

Fix

New step B2 in the per-iteration monitoring checklist (skills/pull-request/SKILL.md),
and the same invariant as a Gate 5 item in reference/readiness.md, which is
the single source of truth both monitor.md (Phase 3.4) and merge.md
(Phase 4.1) rerun, so a direct merge and the final re-verification in full
enforce it too.

A lane that concluded success having produced nothing for the current round
reviewed nothing, whether it never started, could not load the skill it was told
to invoke, or stopped early inside its own session. It is classified ABSENT
rather than PASS, a local review stands in its place over the same diff, and
both are named in the verdict. A lane whose posted body admits a manual fallback
counts the same.

Four details are load-bearing:

  • The roster comes from gh pr checks, never from the authors who posted. A
    lane that posted nothing is missing from every author-derived roster by
    construction, so an author-derived roster would apply the rule to an empty set,
    which is exactly the lane it exists for.
  • Productivity is scoped to the head under review, per surface, with the
    field each surface actually has: original_commit_id on inline review
    comments, commit_id on the reviews endpoint, and created_at against push
    time on issue-level comments, which carry no commit field and are where
    AI-review summaries land.
  • A rerun of the current head is bounded by timestamp, not by commit field,
    because its artifacts carry the same SHA and no commit field can separate them.
  • Both readiness templates gained a Review lanes: line, so an absent lane
    and its substitute are reported rather than dropped from a verdict that
    otherwise reads all-clear.

The local fallback is /review:fanout for breadth, or the bundled /code-review
against an explicit target for a correctness lane, each conditioned on resolving
in the session. Not /review:code-review, whose own Routing section says it runs
only where the workflow invokes it and depends on that wrapper for the
repository, PR number, head SHA, and posting mechanics.

Because the rule is now stated on two surfaces, it is registered as a contract
clause (B2-lane-productivity) with readiness.md canonical and the monitor
checklist tagged as a restatement, putting its four qualifiers under
check-contract-clause-coverage.py.

Step E reads ALL checks terminal + every review lane productive or substituted per B2 + ALL comments addressed + 2-min cooldown. One gotcha
carries the reason, so the rule is not a bare instruction.

Verification

Branched fresh from main, then main moved and is merged in at 8d03719e,
conflict-free. Every gate re-run against that base on the final head:

check-contract-clause-coverage           6 canonical surfaces, 17 tagged restatements
check-purged-em-dashes                   383 declared paths, 1194 files, none found
check-changelog-parity --check           every versioned plugin has an entry
check-changelog-parity --check-bump      every changed plugin has its version entry
check-changelog-parity --check-order     93 changelogs newest-first, no duplicates
check-changelog-parity --check-preserved 250 existing version headings preserved
markdownlint-cli2                        0 issues in 4 files
affected-tests --run                     fetch-annotations 11/11, contract-clause
                                         coverage suite, 24 pytest cases, all pass

Two review rounds, both of which found real defects.

Codex, on the first head: three findings, all verified against the files
before being taken, all real. The productivity check was existential over the
whole PR rather than scoped to the round; the invariant was missing from the
authoritative merge gate; and the local fallback named the CI lane skill.

A local review, on the current head: run because both AI-review lanes
produced nothing and this PR's own new rule classes that ABSENT. Six findings,
five confirmed, and they are the four load-bearing details above plus the
Review lanes: line. The sixth claimed babysit-prs merges through
readiness.md; checked, and it does not, which made the changelog's "every merge
path" the actual overclaim, now corrected to name the readiness gate and say the
babysit tiers are unchanged.

The rule earned its place on its own pull request: it caught that this PR's
review lanes were absent, and the substitution it prescribes found a hole in the
PR's own headline guarantee.

This change is prose in a skill body and its references plus one registry entry;
it alters what an agent following the checklist does, and no shipped code path.

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP

… its check row

An AI-review lane reports on its session, not on its output: a session that
ends without error concludes success even when it posted no review body and no
finding, so the check row goes green and an evidence guard that reads posted
review bodies has nothing to judge when none was posted. Cost is no signal
either, since the session is billed whether or not it reviewed anything. The
monitor checklist classified such a lane PASS on its check row alone and the
readiness gate cleared.

New step B2 requires each review lane to have produced something readable, a
posted review body or at least one finding, and classifies a lane that produced
nothing as ABSENT rather than PASS, whether it never started, could not load
the skill it was told to invoke, or stopped early. An absent lane is
substituted with a local review over the same diff and named in the report, and
step E will not clear until every review lane is productive or substituted. A
lane whose posted body admits a manual fallback gets the same verdict.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
@kyle-sexton
kyle-sexton marked this pull request as ready for review September 11, 2026 16:59
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-11T17:02:50.068106Z 9ac42aa Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 9s —— View job


/review:code-review — CI code-review lane

  • Gather context (PR body, changed files)
  • Run /review:code-review skill
  • Check for anchorable findings (none — the one finding is cross-file)
  • Post summary comment

Scope: doc-only change to plugins/source-control/skills/pull-request/SKILL.md (+ changelog/version bump). No code path is affected; reviewed for correctness and internal consistency of the prescribed procedure per CLAUDE.md/AGENTS.md.

Finding: the new B2 gate isn't wired into the docs that operationalize "readiness"

SKILL.md's step E now reads "every review lane productive or substituted per B2" (SKILL.md#L185), but the three reference docs that actually operationalize "run the readiness gates" for the monitor and merge phases still describe that set as fixed at 6, with no mention of B2 or review-lane productivity:

Concretely: readiness.md Gate 4 ("All comments processed") is satisfied vacuously when a review lane posts zero comments, since there's nothing to process, exactly the false-green case this PR is closing. An agent that follows monitor.md 3.4 or merge.md 4.1 literally ("run all 6 gates from readiness.md") for the phase-specific procedure never encounters the new B2 check, so the merge-time re-verification pass (the one explicitly designed in merge.md to catch late-arriving/changed state before merge execution) doesn't re-check review-lane productivity at all. The fix as written only bites if the agent re-derives B2 from SKILL.md's inline checklist rather than following the phase docs it routes to.

Since the goal here is exactly "the readiness gate cleared on [a green check row alone]," leaving readiness.md/monitor.md/merge.md stating a stale gate count undercuts the fix at the phase docs' level, not just the top-level SKILL.md summary line.

Fix this →

No other high-signal findings. The prose itself is internally consistent (B2 checklist step, the E-gate line, and the new gotcha all agree with each other), follows the checklist's existing letter/decimal-substep convention (matches the D7.5 precedent), and the gotcha states the current rule with its reason in present tense with no incident/issue narration, per .claude/rules/skill-bodies-state-current-rules.md. Changelog entry and version bump are consistent with the diff and existing format.
· branch

@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 38s —— View job


/review:security-review — PR #4106

  • Gather context (PR diff, changed files)
  • Invoke /review:security-review per custom instructions
  • Post findings

Skip gate hit: no security-relevant surface.

This PR changes only:

No shell commands, tool invocations, data-handling logic, or trust boundaries are introduced or altered by this diff — it's exclusively markdown guidance changing how the PR-monitor checklist classifies an already-existing signal (posted review body / findings), not what it fetches or executes. There's no injection surface, no auth/access-control change, and no secret/token handling touched.

Per the skill's skip gate (item 2: "Change has no security-relevant surface after reading the diff"), stopping here with no findings.

@github-actions

Copy link
Copy Markdown

Last security-reviewed head: 9ac42aaae0db6d3b3b53dcce4236dde22bfc1542. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9ac42aaae0

ℹ️ 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".

Comment thread plugins/source-control/skills/pull-request/SKILL.md Outdated
Comment thread plugins/source-control/skills/pull-request/SKILL.md
Comment thread plugins/source-control/skills/pull-request/SKILL.md Outdated
@github-actions

Copy link
Copy Markdown

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

… enforce it on every merge path

Three review findings, each verified against the files before being taken.

The productivity check was existential over the whole pull request, so a review
body or finding left by an earlier head, or by an earlier rerun of the same
head, satisfied it while the newly green lane emitted nothing. That is the
false pass the step exists to catch. Productivity is now scoped to the head
under review through the per-surface commit fields Gate 5 already names, which
the step points at rather than restating.

The invariant lived only in the monitor checklist, but reference/readiness.md is
the single source of truth that both monitor.md and merge.md rerun, so a direct
merge invocation and the final re-verification in full did not enforce it. It is
now a Gate 5 item there as well, stated as what reaching that gate's bound hands
off to: the bound ends the wait for a silent reviewer, it does not supply the
review.

The local correctness fallback named the CI lane skill, which runs only where
the workflow invokes it and depends on that wrapper for the repository, pull
request number, head SHA and posting mechanics. A monitor session has no such
wrapper. The fallback is now the bundled code-review command against an explicit
target, conditioned on it resolving in the session.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
… and register the clause

Six findings from a local review, run because both AI-review lanes produced
nothing for this head and the new rule classes that ABSENT. Five were confirmed
against the files; the sixth, that babysit-prs merges through readiness.md, was
checked and is false in the direction that mattered: its merge path never reads
that file, so the changelog's "every merge path" was the overclaim instead.

The roster was the load-bearing hole. Both statements said "for every review
lane" without naming where the roster comes from, and the readiness item sat
beside a reviewer roster built from the bot authors who posted. A lane that
posted nothing is missing from that roster by construction, so on the merge path
the item applied to an empty set, which is precisely the lane it exists for.
Both surfaces now take the roster from the checks list.

The rerun claim was an overclaim. Commit-field scoping distinguishes an earlier
HEAD, not an earlier RERUN of the current head, whose artifacts carry the same
SHA. Both surfaces now bound that case by timestamp against the run's start
rather than implying the commit fields cover it.

The surface list was wrong by omission. Issue-level comments carry no commit
field and are where AI-review summaries land, so naming only "commit fields"
either false-ABSENTs a real review or falls back to the bare existence check.
All three surfaces are now named with the field each one actually has.

The substitution had nowhere to be reported: both rule statements said to record
it in the verdict, and neither readiness nor monitor template had a field, so it
would drop out of a report that otherwise reads all-clear. Both templates gain a
Review lanes line.

The rule is stated on two surfaces, so it is registered as a contract clause
with readiness.md canonical and the monitor checklist tagged as a restatement,
which puts its four qualifiers under the drift gate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011SQkHipoF2M8rTtnkbFKKP
@kyle-sexton
kyle-sexton merged commit 32eb246 into main Sep 11, 2026
18 checks passed
@kyle-sexton
kyle-sexton deleted the claude/architecture-improvements-hqfc0i branch September 11, 2026 17:40

Copy link
Copy Markdown
Contributor Author

Correction to this PR's description, for anyone reading it later.

The body says both claude[bot] lanes "concluded success and posted zero reviews, on every head" and that "every external finding here came from Codex". Both statements are wrong, and the merged commit message carries them.

I checked GET /pulls/4106/reviews and read 0 as "produced nothing". These lanes post to the issue-comment surface, not the reviews surface, which is the very distinction step B2 in this change tells an agent to make. Reading that surface:

  • claude-review completed in 3m09s with every item ticked and produced a real finding: that B2 was not wired into the reference docs operationalizing "run the readiness gates". Correct against the head it reviewed (9ac42aaa), and addressed before merge by the readiness.md Gate 5 item in 31976587.
  • claude-security-review completed in 38s with every item ticked, through its documented skip gate for a diff with no security surface, naming all three files and why. A reasoned no-findings verdict.

So this PR had three reviewers contributing, not one, and the "third reproduction of #4093" framing in the Related section is retracted; the correction is on #4093.

The change itself is unaffected. Nothing in the diff rested on that claim, every gate cited in Verification was really run, and both review rounds it describes happened. One residual worth a look by whoever picks up #4093: the productivity item landed as a bullet under ### Gate 5: Cooldown period, which does not advertise it by name, so an agent scanning gate titles for a productivity check will not find it there.


Generated by Claude Code

kyle-sexton added a commit that referenced this pull request Sep 11, 2026
…n prose across 36 skills (#4108)

No related issue: this closes the one decision the audit behind #4072
reclassified to a human, resolved by re-reading the sources; no issue
tracks it.

## Summary

Anthropic's [skill-authoring best-practices
page](https://platform.claude.com/docs/en/agents-and-tools/agent-skills/best-practices#writing-effective-descriptions)
rules out first and second person in a skill description. Its two Avoid
examples are "I can help you process Excel files" and "You can use this
to process Excel files", and its reason is that the text is injected
into the system prompt. Its own effective examples, the Claude Code
skills page's examples, and the bundled skill-creator's frontmatter are
all imperative ("Extract text and tables from PDF files", "Fix a GitHub
issue", "Create new skills..."), so imperative and third-person singular
both conform and this marketplace's imperative descriptions are not a
deviation. The real gap was description prose that addresses the reader.
A scripted scan that strips quoted trigger phrases and hyphenated or
slash-joined identifiers found it in 36 skills across 24 plugins.

## Fix

- **36 skill descriptions** (adhd, bugs, claude-config, claude-ops,
code-metrics, discipline, discovery, docs-hygiene, education, firecrawl,
improvement, instruction-placement, kindle-dedrm, mutation-testing,
overengineering, playgrounds, plugin-quality, prototype, session-flow,
songwriting, source-control, tdd): each clause that addressed the reader
now names the user, the session, the assistant, or the repository.
Examples: "so you can act on it" becomes "so the reader can act on it";
"a permission rule you cannot locate" becomes "a permission rule whose
source is unknown"; "books you own" becomes "books the user owns";
"synthesize where we stand" becomes "synthesize where the session
stands". Quoted trigger phrases (user utterances such as 'audit my
.claude folder'), backticked tokens, slash tokens, and skill names are
byte-identical. No description grew by more than 20 codepoints or
crossed 1,024; two that sat near the cap got shorter.
- **One clause stays as it was.** The native-surface presence gate
("resolves in your session") in the two claude-ops descriptions
(`audit-install-state`, `audit-skill-visibility`) is a literal token the
native-overlap registry self-check matches against every baked store
row, so `validate-plugins.sh` fails when it is reworded (that is what
turned `7cbf30ab8` red). Commit `af0e7b274` restores the token in both
and the claude-ops changelog entry says why. Changing the token itself
touches the checker, its tests, twelve skills, the seam-phrasing
convention, and the store evidence, and is tracked as #4112.
- **Two plugin manifests** (kindle-dedrm, session-flow) carried the same
wording in their own descriptions and follow; `docs/CATALOG.md`
regenerated.
- **playbooks 0.11.2**:
`skill-authoring/reference/authoring-guidance.md` states the rule as the
page defines it, with the Avoid examples, the system-prompt reason, and
the finding that imperative conforms, and its Record cites the three
imperative example surfaces with a recheck trigger for the page
rewriting its examples. `reference/authoring-checklist.md` gains the
matching judgment row. The previous text called the fleet's imperative
voice a deviation to leave alone, which the sources do not support.
- Each of the 24 touched plugins gets a patch bump and a CHANGELOG entry
naming its rewritten skills. Main moved four times while this was open;
the merge commits `2cc1102c0`, `f082ed2f8`, `7cbf30ab8`, and `0b10ccae7`
restack this change's entries above the newer main entries: claude-ops
to 0.54.1 (above #4099, #4107, and #4092), code-metrics to 0.2.3 (above
#4098), source-control to 0.55.76 (above #4106). Every other plugin's
bump is unchanged.

## Verification

Run on Linux at head `0b10ccae7`:

- Voice scan over all 258 skill descriptions (quoted phrases and
identifiers stripped, then first and second person pronouns matched): 36
hits before; after, only the two claude-ops descriptions that carry the
checked gate token (3 occurrences of that token, nothing else). An
uppercase variant scan finds 0.
- `scripts/validate-plugins.sh`: passes; the native-overlap self-check
reports degraded (stale-but-honest advisories, the same as main), not
broken.
- `scripts/check-changed-skills.sh origin/main`: 35 skills checked, 0
failed, no dropped-trigger-keyword WARN on any skill (check 3 compares
every trigger phrase against the base ref).
- `check-changelog-parity.sh --check`, `--check-bump origin/main`,
`--check-order`: pass across all 93 changelogs. `generate-catalog.mjs
--check`: in sync.
- `markdownlint-cli2` over every changed markdown file: 0 issues.
`typos` over the added lines: clean. `check-purged-em-dashes.sh`: no em
dashes.
- Diff shape: 36 SKILL.md files touched on their description line only,
23 plugin.json version bumps plus the two manifest description edits, 23
CHANGELOG.md entries, the two playbooks spokes, and the catalog. No
skill body, script, or test changed.
- `scripts/affected-tests.sh --run`: no suites selected (every changed
file is a recorded no-suite class).
- CI: `ci-status` and every lane green on `45af55489` (draft) and on
`2cc1102c0` (draft and after the ready flip). Codex review on
`2cc1102c0`: completed, no findings. `7cbf30ab8` went red on
`test-linux` (the gate-token self-check above); `af0e7b274` carries the
fix and `0b10ccae7` merges the latest main on top.

## Related

- Follow-up to #4069 and #4072. The two fresh-context validators behind
#4072 both reclassified the description-voice row to a human decision on
the premise that imperative and third person are two voices; re-reading
the page's examples shows they are one conforming class, so the decision
resolves on evidence rather than taste. The reasoning and the scan live
in the untracked
`.work/skill-authoring-best-practices/INTERVIEW-ACCEPTED.md` ("Audit
resolution", row I1.2) of the authoring checkout.
- #4112 tracks retokening the presence gate so the last two descriptions
lose their second person too.
- #4070 and #4071 remain the deferred follow-ups from #4069.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01MSGD4WGhUXepHq3LTmpQML

---------

Co-authored-by: Claude <noreply@anthropic.com>
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.

2 participants