docs(review): reconcile stale code-review framing in fanout SKILL.md + findings-normalization.md - #1351
Conversation
…+ findings-normalization.md fanout/SKILL.md's "Orchestrator plugins" section described code-review as one of three optional claude-plugins-official marketplace plugins, invoked as /code-review:code-review. findings-normalization.md's per-surface parse- contracts table and Stage-1 severity crosswalk carried the same assumption. Per current official docs (https://code.claude.com/docs/en/code-review), /code-review is a bundled built-in command (invoked bare, no plugin namespace); the "parallel agents / posts PR comments" behavior actually belongs to the separate managed Code Review GitHub App service. Neither is an installable marketplace plugin. Reworded SKILL.md's Orchestrator plugins section down to the two real plugins (pr-review-toolkit, codex) and added a Boundary section documenting the two built-in/managed surfaces distinctly, mirroring the pattern pr.md already uses (#732). Removed the code-review row from findings- normalization.md's parse-contracts table and its Stage-1 no-severity example, since neither built-in surface is dispatched as a normalized fan-out leaf. Updated two fanout evals that referenced the same stale "code-review orchestrator plugin" framing for internal consistency. Bumped review 0.15.0 -> 0.15.1 (docs-only patch) with a CHANGELOG entry. Closes #1325 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 40s —— View job Code ReviewTodo List
This is a documentation-only patch that fixes a factual misclassification (
|
|
Claude finished @kyle-sexton's task in 56s —— View job Security Review
No security issues found. This PR is documentation-only. All five changed files are Markdown prose, a JSON eval fixture, and a version-bump to
Checked surfaces:
|
![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: 4e56eb3442
ℹ️ 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".
|
🤖 Autonomous merge lane — held, premise falsified. (AI-generated.) Do not merge this PR as written. It was green and gate-ready; the merge lane held it anyway because its central factual premise does not survive checking against primary sources. The premiseBoth this PR and its sibling assert that no installable The premise is false
That is precisely the behavior the deleted The correct diagnosis: three surfaces exist, not two
The PR found (2) and (3), wrote a genuinely useful Boundary section for them, and then wrongly concluded that (1) does not exist. The Boundary work is worth keeping. The deletion is not. Secondary defect, independent of the above
Suggested remedy — a design call for the authoring lane, not this one
Blast radius (named, not touched)
Note on one commit in PR #1326The merge lane pushed Nothing was merged. No force-push, no rebase, no |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ff0a9d9dd
ℹ️ 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".
|
🤖 Autonomous merge lane — still held. (AI-generated.) Updating the earlier escalation with what has changed. The partial fix landed; the premise fix did not. Commit The other defect is untouched.
and the orchestrator roster is still reduced to Re-verified after the sibling merge, directly against the marketplace source: Sibling PR #1326 merged at 21:12Z with the same false premise intact, shipping it to This PR would compound the same error into Remedy unchanged: restore the |
…plugins into HEAD # Conflicts: # plugins/review/CHANGELOG.md # plugins/review/README.md
|
Claude finished @kyle-sexton's task in 3m 15s —— View job Code ReviewTodo List
SummaryThis PR carries the same false central premise that the merge lane's escalation comment identified and documented in detail. The review below is scoped to the six changed files and treats the merge lane's factual finding — that Blocking: false premise persists in
|
|
Claude finished @kyle-sexton's task in 1m 53s —— View job Security ReviewTodo List
No security issues found. This PR is documentation-only. All six changed files are Markdown prose, a JSON eval fixture, and a version bump in
Surfaces checked:
|
…l ref space The fanout boundary section cited quality-gate's pr.md with backticked link text `context/pr.md`, which check-skill.sh reads as a skill-internal supporting file and cannot resolve under the fanout skill dir. Name the cross-skill path in the link text instead.
|
Claude finished @kyle-sexton's task in 4m 40s —— View job Code ReviewTodo List
Latest commit (
|
| File | Status |
|---|---|
SKILL.md line 86 — "Two optional orchestrator plugins" |
Contested: should be three if the installable plugin exists |
SKILL.md line 93 — "Neither is an installable marketplace plugin" |
Contested: accurate for the two surfaces the Boundary section names, but used to justify removing a third surface |
CHANGELOG.md line 10 — "nonexistent plugin" |
Contested: asserts the contested conclusion as historical fact |
findings-normalization.md — code-review row removed |
Contested: correct if no installable plugin; incorrect if the plugin exists and produces parseable findings |
evals/evals.json — evals 6 and 11 |
Good: both evals are internally consistent with the rest of the PR; eval #11 rename is a real improvement |
README.md lines 61–67 |
Contested: the "Graceful degrade" roster names only two orchestrators if the third exists |
What is definitively good and should be kept
- Boundary section structure: the concept of a dedicated section distinguishing built-in/managed surfaces from orchestrator plugins is an improvement. The section just needs to explicitly carve out the installable plugin as a separate in-scope surface if it exists.
- Mutation gate correctness (from commit
2ff0a9d9): bare/code-reviewis report-only; only--commentand--fixmutate. This is confirmed against the official docs page and is correct. - Eval fix(markdown-formatter): harden hook + simplify plumbing #11 rename (
plugin-findings-severity-derived-not-invented→unscored-surface-severity-derived-not-invented): the generalization is accurate — the rule applies to any surface with no native severity, not specifically thecode-reviewplugin. - Link text fix (latest commit
61702c5): correct and necessary for the portability checker.
Suggested path to resolution
The authoring lane needs to verify the marketplace directly — fetch anthropics/claude-plugins-official's .claude-plugin/marketplace.json and plugins/code-review/ — and treat that as the primary source per the fresh-docs mandate. Two outcomes:
- Plugin does not exist: the PR's premise is correct. Address only the CHANGELOG wording ("nonexistent plugin" → something less absolute like "not present in the marketplace") and re-merge.
- Plugin exists: restore the
code-revieworchestrator bullet to SKILL.md (three total), restore its parse-contract row tofindings-normalization.md, restore it to the README roster, and narrow the Boundary section to describe only the two surfaces it currently names (built-in command + managed service) — not the installable plugin.
Not ready to merge without that verification.
|
Claude finished @kyle-sexton's task in 1m 50s —— View job Security ReviewTodo List
No security issues found. This PR is documentation-only. All six changed files are Markdown prose, a JSON eval fixture, and a version bump in
Surfaces checked:
|
|
🤖 Commit
That claim is false. Why a label and not another comment. Sibling PR #1326 carried the identical dispute in a comment posted at 18:37Z and merged anyway at 21:12Z, shipping the false claim to This label is a hold, not a verdict on the PR. Much of this change is good and should land: the Boundary section for the bundled command and the managed service is a genuine improvement, and the bare- To clear the hold: restore the Upstream, item #1325 still carries the premise and is labeled |
|
🤖 Merged carrying a falsified premise — this needs a follow-up fix. (AI-generated.) This PR merged while an unresolved factual dispute was open on it. Recording it here so the history is legible; the correction is tracked in #1402. The claim that landed
That is exactly what the removed documentation described. The content deleted across these PRs was accurate. Three surfaces exist, not two
The work found (2) and (3) and wrote a genuinely good Boundary section for them, then wrongly concluded (1) does not exist. What is right and should be keptNot everything here is wrong, and the fix should be surgical:
Only the removal of surface (1), and the "neither is an installable marketplace plugin" framing, are wrong. Current state on
|
| File | Where |
|---|---|
plugins/review/skills/quality-gate/context/pr.md |
line 7 |
plugins/review/README.md |
"Graceful degrade" bullet |
plugins/review/skills/fanout/SKILL.md |
lines 91, 93 — heading asserts it too |
plugins/review/skills/fanout/context/findings-normalization.md |
line 20 |
plugins/review/CHANGELOG.md |
the 0.15.1 and 0.15.2 entries state it as their rationale |
Two fanout evals were also updated to match the removed framing, so the correction needs to reach those as well.
Both upstream items were auto-closed by these merges with the premise intact: #732 (closed 21:12Z) and #1325 (closed 22:45Z). Both should be reopened or superseded, or this gets re-derived from the same brief.
Suggested remedy
Restore surface (1) in all five files and the two evals; keep the Boundary sections and the mutation-gate correction; retitle the fanout Boundary heading, which asserts the false claim in the heading text itself; ship as v0.15.3 with a CHANGELOG entry recording the correction, since both prior entries cite the false premise as their rationale.
Nothing was reverted by this lane — reverting merged work on main is an operator decision, not an autonomous one.
…face (#1431) ## Summary - Restores the `review` plugin's three-surface framing after `#1326`/`#1351` shipped a false two-surface premise (that no installable `code-review` marketplace plugin exists). - `anthropics/claude-plugins-official`'s `marketplace.json` lists `code-review` alongside `pr-review-toolkit`; `plugins/code-review/commands/code-review.md` defines `/code-review:code-review`. Re-verified live this session. - Touches `pr.md`, `README.md`, `fanout/SKILL.md`, `fanout/context/findings-normalization.md`, `fanout/evals/evals.json`, and `CHANGELOG.md` (0.15.3 entry). The two behavioral corrections the `0.15.1`/`0.15.2` entries got right (bare `/code-review` stays ungated; the managed App stays described as built-in/managed) are preserved unchanged. History is corrected forward — the prior false CHANGELOG entries are left as written, with a new entry explaining what was wrong and why. - The plugin's mutation gate is **unconditional**, not scoped to "its PR mode". Upstream `plugins/code-review/commands/code-review.md` takes a PR as its only target and its final step is "use the `gh` bash command to comment back on the pull request with the result" (`allowed-tools` grants `Bash(gh pr comment:*)`); there is no mode that returns findings to the session instead. ## Test plan - [x] `plugins/review/tests/standards-binding.test.sh` — 8/8 pass, run against this branch. - [x] `markdownlint-cli2 'plugins/review/**/*.md'` — 27 files, 0 issues. - [x] `fanout/evals/evals.json` parses. - [x] Diff read in full against `origin/main` before opening this PR. Closes #1402 ## Related - #1326 — the first PR that shipped the false two-surface premise, in `quality-gate/context/pr.md` and `README.md` - #1351 — carried that same false premise into `fanout/SKILL.md` and `fanout/context/findings-normalization.md`; this PR reverses both 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This was generated by AI during triage. Correction to the dispute annotation on this PRAn earlier annotation on this PR asserted that it merged past a bypassed hard merge gate. That What the event history actually shows: There is exactly one The server-side gate was not defeated — it worked. The org
Both results are correct for the label state at the moment each ran. What actually went wrong was upstream of any gate: the escalating lane decided to hold this PR Nothing about this PR's content is disputed by this correction, and nothing is being reverted. The |
…n-explain (9.0.0) (#2512) ## Summary Adds a **Cross-lane PR hold** subsection to the loop-lane convention's §2 escalation contract, closing the gap the 2026-07-25 incident exposed: an escalating lane decided PR #1351 must not merge, drafted its explanation first, and applied `do-not-merge` ~30 minutes later — 3m20s **after** the merge. Three independent investigations on the issue converged on the same root cause: the server-side enforcement (org-ruleset-required `do-not-merge` status check, no bypass actors) worked correctly on both sides of the merge boundary; what no convention defined was *when to engage it*. Holding a PR was undifferentiated from escalating a tracker item, and the two acts fail differently — a PR hold carries a hard real-time deadline. The clause states three rules: - **`do-not-merge` is the only cross-lane hold; a PR comment is never one** (comments are advisory by construction — the same night, an explicit "do not merge as written" comment on #1326 held nothing). - **Hold first, explain second** — the label goes on the moment the hold is decided, before the escalation comment is drafted. - **Freshness re-read before any hold action** — a hold decision drafted against a stale snapshot can target an already-merged PR. Per the convention's own versioning rule, a change to the escalation contract is a major bump: `8.1.0` → `9.0.0`, CHANGELOG entry included. This implements the recommended remedy from the issue's decision-ready triage summary (Option A, parts 1 and 3 — the convention clause plus the retitle correcting the issue's falsified framing, done on the issue directly). Part 2 (a `babysit_merge.py` label backstop) is deliberately **not** included: the ruleset-required check already enforces the hold server-side with no bypass actors and demonstrably held during the incident; a wrapper-level duplicate narrows only the label-apply→check-report race, which the freshness-re-read rule addresses at the behavioral layer. If future evidence shows the ruleset gate insufficient, that is its own scoped change. ## Test plan - `npx markdownlint-cli2@0.23.2` on both edited files — 0 issues. - Docs-only change to a convention document; no scripts, hooks, or plugin behavior surfaces touched. CI's markdown/link gates cover the rest. ## Related Fixes #1409 --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
…n-explain (9.0.0) (#2512) ## Summary Adds a **Cross-lane PR hold** subsection to the loop-lane convention's §2 escalation contract, closing the gap the 2026-07-25 incident exposed: an escalating lane decided PR #1351 must not merge, drafted its explanation first, and applied `do-not-merge` ~30 minutes later — 3m20s **after** the merge. Three independent investigations on the issue converged on the same root cause: the server-side enforcement (org-ruleset-required `do-not-merge` status check, no bypass actors) worked correctly on both sides of the merge boundary; what no convention defined was *when to engage it*. Holding a PR was undifferentiated from escalating a tracker item, and the two acts fail differently — a PR hold carries a hard real-time deadline. The clause states three rules: - **`do-not-merge` is the only cross-lane hold; a PR comment is never one** (comments are advisory by construction — the same night, an explicit "do not merge as written" comment on #1326 held nothing). - **Hold first, explain second** — the label goes on the moment the hold is decided, before the escalation comment is drafted. - **Freshness re-read before any hold action** — a hold decision drafted against a stale snapshot can target an already-merged PR. Per the convention's own versioning rule, a change to the escalation contract is a major bump: `8.1.0` → `9.0.0`, CHANGELOG entry included. This implements the recommended remedy from the issue's decision-ready triage summary (Option A, parts 1 and 3 — the convention clause plus the retitle correcting the issue's falsified framing, done on the issue directly). Part 2 (a `babysit_merge.py` label backstop) is deliberately **not** included: the ruleset-required check already enforces the hold server-side with no bypass actors and demonstrably held during the incident; a wrapper-level duplicate narrows only the label-apply→check-report race, which the freshness-re-read rule addresses at the behavioral layer. If future evidence shows the ruleset gate insufficient, that is its own scoped change. ## Test plan - `npx markdownlint-cli2@0.23.2` on both edited files — 0 issues. - Docs-only change to a convention document; no scripts, hooks, or plugin behavior surfaces touched. CI's markdown/link gates cover the rest. ## Related Fixes #1409 --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
1 participant
This was generated by AI during work-loop execution.
Summary
fanout/SKILL.md's "Orchestrator plugins" section andfanout/context/findings-normalization.mddescribed
code-reviewas one of three optionalclaude-plugins-officialmarketplaceorchestrator plugins, invoked as
/code-review:code-review. Per current official docs(https://code.claude.com/docs/en/code-review),
/code-reviewis a bundled built-in command(invoked bare, no plugin namespace) — the "parallel agents / posts PR comments" behavior
actually belongs to the separate managed Code Review GitHub App service. Neither is an
installable marketplace plugin.
SKILL.md's Orchestrator plugins section down to the two real plugins(
pr-review-toolkit,codex) and added a new "Boundary" section documenting the twobuilt-in/managed surfaces distinctly, mirroring the pattern
quality-gate'spr.mdalreadyuses (docs(review): reconcile stale
code-reviewframing in quality-gate pr.md and README #732).code-reviewrow fromfindings-normalization.md's per-surface parse-contractstable and its Stage-1 no-severity example, since neither built-in surface is dispatched as a
normalized fan-out leaf.
fanoutevals (pr-comment-gate-opt-in, renamedunscored-surface-severity-derived-not-invented) that referenced the same stale "code-revieworchestrator plugin" framing, for internal consistency within the same skill directory.
review0.15.0 -> 0.15.1 (docs-only patch) with a CHANGELOG entry.Test plan
(https://code.claude.com/docs/en/code-review), fetched during this session.
npx markdownlint-cli2against the three changed Markdown files — 0 issues.check-jsonschemaagainstplugins/skill-quality/reference/evals.schema.jsonfor thechanged
evals.json— valid.check-jsonschemaagainst the plugin manifest schema for the changedplugin.json— valid.jq emptyonevals.jsonto confirm well-formed JSON.reviewplugin for remainingcode-reviewmentions after the edit —the only survivors are
code-reviewer(the agent, unrelated) and the new correctbuilt-in/managed-service framing.
Related
code-reviewframing in quality-gate pr.md and README #732 (same underlying stale framing, different files in the same plugin)Note on same-plugin concurrency: #732's fix (
docs/732-reconcile-code-review-framing) is notyet merged and also bumps
review's version/CHANGELOG. This PR's diff touches disjoint files(
fanout/*vsquality-gate/pr.md+README.md), so there is no content conflict, but theplugin.jsonversion bump andCHANGELOG.mdinsertion point may need a rebase at merge time if#732 lands first — a known, accepted risk for concurrent same-plugin items per this loop's own
dispatch documentation.