feat(agents): add finding-triage, a read-only PR-comment checker - #4
Merged
Conversation
Design approved before Gate A. Records the platform facts that were read rather than assumed (agents/ directory, frontmatter schema, tools inherits-all-if-omitted, permissionMode/hooks ignored for plugin agents), because two of them drive real decisions: there is no per-agent way to constrain Bash, and prose must use the scoped name. Notes the asymmetry between the two agents — finding-triage is mechanically read-only, task-verifier is partly instruction-backed because it must run the test command — and states that rather than implying a guarantee the format cannot give. ledger-scribe is scoped out with its reasoning preserved: motivation real, the concern already carried elsewhere, constraint expressible but redundant.
Gate A pass 1 returned 34 findings. Two were structural. task-verifier duplicated superpowers:subagent-driven-development, which already dispatches a per-task spec-compliance + quality reviewer. The other execution path, executing-plans, tells you to use subagent-driven-development when subagents are available — so on the platform where task-verifier could run the reviewer already exists, and the path lacking one cannot run subagents. Dropped, with the residual distinction and its trigger condition recorded rather than the reasoning discarded. The spec had also claimed a rogue subagent write 'cannot be invisible' because the content hash catches it. todos.md in this same repo lists the false-✓ paths that refute it. Downgraded to best-effort detection with the gaps named — a claim in one document must not overstate what another already refutes. Adding agents/ makes invariant 11 and prompt-standards' enumeration of governed prompt artifacts incomplete, so the change that opens that gap now closes it. Also: separated factual validity from actionability in the verdicts, defined the input contract's missing-field behaviour, SHA-staleness and moved-code handling, stop conditions, and dropped the invented getting-started line cap.
Pass 2 returned 1 blocker + 13 majors. The blocker was structural: the agent was asked to decide whether a fix 'belongs in this PR', which needs a base/head comparison — and Read/Grep/Glob cannot derive a git range. The contract was unimplementable, and the example output escalated on a condition the agent could not observe. Resolved by making the split explicit: the subagent judges whether a claim is TRUE, the main command decides what to DO. Actionability was already Step 3.3's job. This also resolves the duplicate-detection contradiction (the one-claim contract withholds the other comments the agent was told to compare against). Other majors: caller now passes both SHAs since the agent cannot resolve git state; search budget widened from 'file and immediate callers' (which would falsely accept a missing-validation claim when validation sits in middleware) with an explicit bound; untrusted-input rule added — bot comments are attacker-influencable on a public repo and the output may be posted back to it; snapshot barrier and subagent-failure handling defined; location shape allows file-level and range comments. The 'no residual' read-only claim was false — user-configured hooks can run on this agent's tool calls. Replaced with the precise boundary. And the section correcting an overstated cross-document claim contained one: it said all four hash gaps are in todos.md; only two are. Now each is cited where it is actually documented.
… boundary (pass 3) Pass 3 returned 10 majors. The most important was the blocker's tail: I split truth from actionability in the agent but left Step 3 implementing every accepted finding BEFORE checking whether it belonged. Actionability classification is now its own step ahead of implementation, and accept explicitly never authorizes a fix on its own. Verified against the docs rather than assumed: custom subagents load the full CLAUDE.md hierarchy and there is no per-agent opt-out — only built-in Explore and Plan skip it. Since this repo's CLAUDE.md imports AGENTS.md, a PR editing an instruction file changes the rules triage runs under, before its data-only rule applies. Not closable at the agent level, so the command now surfaces instruction-file changes for review before dispatching any triage. Also: equal SHAs do not identify a snapshot (uncommitted edits change content while both SHAs hold), so the caller passes and re-checks a worktree fingerprint; the search budget is now countable (25 tool calls, repeats counted) instead of 'roughly a dozen file reads'; an output grammar the caller validates; paths confined to the checkout root because locations derive from attacker-influencable text; dedup by claim rather than location; and claim-level tracking with thread-level replies for compound comments.
…tree Gate A pass 4 answered 'not implementable without guessing' and 'not internally consistent'. Nearly every open finding traced to one root cause: the agent has no shell, so each git-derived fact had to be passed in, and each passed fact needed its own trust, validation and failure rule. Pass 3 added a fingerprint; pass 4 showed the fingerprint recipe was undefined, unreachable from an installed command, and detected net change rather than identity. So the contract is cut rather than patched: given one claim and where to look, is it true of the code readable right now? Staleness, actionability, dedup and replies all belong to the caller, which has git. Triage is explicitly best-effort — stated, not engineered around, since every verdict was already advisory. Injection gets the blunt rule: custom subagents load the CLAUDE.md hierarchy with no per-agent opt-out, so a PR touching any instruction-bearing path skips triage entirely. That path class is deliberately broader than the hook's is_prompt_path, with the reason stated — a missed reminder and an injected instruction are not the same failure. No hook change: plugins/**/agents/*.md already matches is_prompt_path's plugins/ segment and .claude/agents/*.md matches .claude/, so both real locations already fire Gate B. Carries a kill condition: if this does not converge within two Gate A passes, the feature is dropped.
Nine findings, no blocker, every fix local — the narrowing converged. Two were regressions the narrowing itself introduced, which is what a review pass after a big cut is for: AGENTS.md fell out of the instruction-bearing path class even though the agent reads it as authority, and path confinement was left to an agent that cannot resolve symlinks. Both restored, the second assigned to the caller where it belongs. The injection boundary gains a fail-closed import rule (unresolvable import => skip triage; a boundary that is only sometimes complete is not one) and a precheck attestation as a required input. That attestation is a real agent-side check — an invocation that skipped the precheck cannot supply it — and the spec says plainly it is not proof against a caller that lies, because nothing at this layer would be. Also: an actionability truth table so a valid pre-existing finding is recorded rather than silently widening the PR or vanishing; one cheap caller rule replacing the removed snapshot barrier, with external edits named as accepted residual risk; claim canonicalization, since a real bot comment can be multiline and can contain the word VERDICT; and bounded dispatch batches against spawn limits.
Pass 6 returned no Blocker/Major — the narrowed design converged inside its declared two-pass kill budget, so the feature ships rather than being dropped. Five minors applied. The one worth naming: the attestation paragraph overstated what a caller-authored field can prove, which is the fourth instance of the unsupported-enforcement pattern in this document — written, this time, into the same spec whose §10 records that pattern as a harden-finding. It now says exactly what the field does (fail-closed on omission, catches accidental invocation) and what it cannot do (establish the precheck ran). Also: the mutation freeze now spans every queued claim rather than the current batch, since batching made 'dispatched' ambiguous; a concrete batch size of 4, chosen as a conservative constant rather than citing a configurable environment variable nobody verified; a third REASON form so diagnostic escalations need not invent a file:line citation; and superseded claims excluded before the tracked set is formed, so Done stays satisfiable.
Seven tasks: the definition, the enumeration sweep across AGENTS.md and the repo docs, the scaffolded inline templates, process-pr-review's Step 3 and Done, the user-facing docs and version, then verify-and-commit. Every edit carries verbatim find-text and replace-text, and all 17 find-texts were checked against the working tree before committing — a plan whose 'before' string is wrong fails on contact with the file. Global constraints carry the spec's settled decisions into every task: one commit (overriding this skill's commit-per-task default, with the Gate-B-cycle reason stated), the hook untouched, nothing added to plugin.json but the version, and the no-unsupported-enforcement-claim rule Gate A caught four times in the spec.
…n pass 1) Gate A on the plan: 1 blocker, 10 majors, 8 minors. The blocker was in my own gate mechanics. mcp__codex__review reads the COMMITTED range, so a fix left in the working tree between passes is invisible to it: the next pass re-reviews the same stale diff, reports the same findings, and the final commit ships without the fixes. The loop now stages and amends the WIP commit after every fix, with that reason written down because the step is easy to skip. Verification also moved inside the loop — a review fix can break validation, touch the hook, or add an unsupported claim, and the plan previously checked all of that only once, before Gate B ever ran. Task 4's caller contract had quietly lost detail the spec settled: the @path expansion rule and the ambiguous-import failure state, the "repository" token for claims with no canonical file, the branch for a project with no AGENTS.md, and symlink-following confinement. All restored — a plan written for someone with no context cannot leave those to be re-derived. Two honest hits: the agent body I drafted contained a fifth instance of the unsupported-enforcement pattern ("an altered claim means the verdict lands on the wrong one" — which the caller's own equality check is designed to prevent), and the invariant-11 review covered only the new agent when Tasks 2-5 change five other shipped prompts.
… opened Gate A plan pass 2: 8 majors, no blocker — the pass-1 blocker is resolved. Three findings were consequences of my own pass-1 fixes, which is what a review after a large edit is for. The confinement rule I added told the executor to "skip a claim whose locations cannot be proved confined", leaving a tracked claim with no verdict that Done could never accept; such a claim now stays tracked, spawns nothing, and takes an escalate-to-user disposition naming the failed check. The Gate-B loop had no defined exit: step 5 said "go to 1" unconditionally while the floor paragraph described two exit conditions elsewhere. The decision now sits immediately after each review, before any fix work, with both exits stated there. It also gained CLAUDE.md §5's one-retry timeout rule, and an abort path — a stuck loop previously left the branch on a WIP commit, which is the one outcome the WIP naming convention exists to prevent. Re-verification after a fix covered quality, hook and enforcement but not the path scope check or the 11-item prompt review, so a fix could amend an unrelated path or invalidate the recorded review that ships in the commit message. Both now re-run. Task 4 regained the claim/thread tracking and dedup-by-claim rules the spec settled and the plan had dropped. Tasks 2-3 now begin with AGENTS.md's mandatory pre-edit grep and a read of plugin.json, since these edits change statements about what loads by convention — the exact path behind the 0.2.1 failure. And my rationale for amending claimed it "keeps the reviewed range and the eventual commit the same object". Amend creates a new object; the range keeps its parent, not its identity. Corrected, since reasoning from a false mechanism is what the plan's own enforcement-claim rule forbids.
…rence Gate A plan pass 3: 1 major, 1 minor. The major is about how the plan actually gets executed. I put AGENTS.md's mandatory pre-edit grep in Task 2 only, but the plan recommends a fresh subagent per task and Task 3 declares it consumes nothing — so the worker holding Task 3 would edit docs/architecture.md's manifest and convention-loading claims having never read the manifest. Task 3 now carries the same check, with the duplication justified in place rather than left looking redundant. The minor is a cross-reference I broke in pass 2: splitting re-verification and staging into 6.4 and 6.5 left Step 7 still pointing at 6.4 as the amend step.
Adds the plugin's first agent definition and wires it into the PR processor.
Convention-loaded from agents/, so plugin.json gains only the version.
WHAT IT DOES, AND DELIBERATELY DOES NOT
Given one claim and where to look, it answers whether that claim is true of the
code — nothing else. Actionability, staleness and deduplication stay with the
command, which has Bash, git and the other comments; the agent has none of those.
That split came out of Gate A: an earlier design had the agent decide whether a
fix "belongs in this PR", which needs a diff range it cannot derive, so the
contract was unimplementable as written.
Its verdicts are accept / dismiss / escalate-to-user, factual only. It never
counts as a Gate A or B pass — same model as its caller, so no cross-model
independence — and it says so in its own text, because the definition is what the
agent reads.
TWO BOUNDARIES STATED RATHER THAN OVERSOLD
Read-only means exactly: it cannot directly invoke a Claude Code write or shell
tool, enforced by the tools allowlist. It does not mean nothing changes on disk —
hooks in the user's own settings can run on its tool calls.
Custom subagents load the whole CLAUDE.md hierarchy with no per-agent opt-out
(verified against the docs). A PR editing an instruction file would therefore
rewrite the rules its own reviewer runs under, so process-pr-review skips subagent
triage entirely for such PRs and validates those comments itself.
process-pr-review also gained an actionability step before implementation:
accept establishes truth, not permission to change code.
PROMPT-STANDARDS REVIEW (invariant 11, all 11 items, per changed artifact)
agents/finding-triage.md — passes. 1: names Claude via Claude Code with the
check date. 2/3: 25-tool-call budget plus an immediate-stop list. 4: three-field
block with three worked verdicts. 5: sectioned by concern. 6: every rule carries
its reason. 7: no contradiction with the command. 8: no duplication of AGENTS.md.
9: positively framed ("read widely enough to be right"); the remaining
prohibitions are boundaries whose subject is the prohibition. 10: each escalation
cause paired with what the caller must supply. 11: 9 bold uses inventoried — 7 are
structural list labels, 2 are load-bearing rules; no ALL-CAPS, no MUST.
process-pr-review.md — passes; item 4 was failing until a worked report example
was added showing two claims on one thread, both terminals, hardening, per-claim
verification and final-head CI.
workflow-init.md (both inline templates), CLAUDE.md, AGENTS.md,
harden-finding/SKILL.md — one-line enumeration changes; items unaffected, checked
rather than assumed.
VERIFICATION
Canonical quality command from AGENTS.md § Commands, run verbatim: shellcheck on
all four shell files, hook suite, 61 invariant assertions, invariant scan, and
claude plugin validate --strict — all green. plugins/dev-workflow/hooks/ is
byte-identical; plugin.json changed by one line.
Gate B: three passes, final clean (no Critical, no Important). Gate A ran six
passes on the spec and four on the plan.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a read-only ChangesFinding triage workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PRProcessor
participant FindingTriage
participant Repository
participant ReviewThread
PRProcessor->>PRProcessor: Split and normalize review claims
PRProcessor->>FindingTriage: Dispatch up to four claims per batch
FindingTriage->>Repository: Read repository evidence
Repository-->>FindingTriage: Matching or contradictory code
FindingTriage-->>PRProcessor: Verdict and reason
PRProcessor->>ReviewThread: Post claim disposition
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryAdds
Confidence Score: 4/5Safe to merge; all changes are prompt/documentation artifacts with no application code, and the functional behaviour of the hook and invariant checker is unchanged except for the The agent definition is carefully written — explicit No files require special attention; Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant U as User
participant C as process-pr-review
participant FT as finding-triage
participant GH as GitHub / gh CLI
U->>C: /dev-workflow:process-pr-review [pr-number]
C->>GH: read bot matrix (docs/pr-review-bots.md)
C->>GH: gh pr checks (CI gate)
C->>GH: wait for findings bots
C->>C: Step 3.0 instruction-path precheck
alt PR touches instruction-bearing paths
C->>C: validate all claims manually (no subagent)
else precheck passes
C->>C: form tracked claim set (split + normalize)
loop batches of 4
C->>FT: claim + locations + AGENTS.md path + attestation
FT->>FT: Read/Grep/Glob up to 25 calls
FT-->>C: CLAIM / VERDICT / REASON
C->>C: validate output block
end
C->>C: Step 2 decide actionability per accepted claim
C->>C: Step 3 implement actionable findings
C->>C: Step 4 escalate unresolved to user
C->>C: Step 5 harden-finding for fixed classes
end
C->>GH: gh pr comment (one reply per thread)
C-->>U: report (verdict + action + verification per claim)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant U as User
participant C as process-pr-review
participant FT as finding-triage
participant GH as GitHub / gh CLI
U->>C: /dev-workflow:process-pr-review [pr-number]
C->>GH: read bot matrix (docs/pr-review-bots.md)
C->>GH: gh pr checks (CI gate)
C->>GH: wait for findings bots
C->>C: Step 3.0 instruction-path precheck
alt PR touches instruction-bearing paths
C->>C: validate all claims manually (no subagent)
else precheck passes
C->>C: form tracked claim set (split + normalize)
loop batches of 4
C->>FT: claim + locations + AGENTS.md path + attestation
FT->>FT: Read/Grep/Glob up to 25 calls
FT-->>C: CLAIM / VERDICT / REASON
C->>C: validate output block
end
C->>C: Step 2 decide actionability per accepted claim
C->>C: Step 3 implement actionable findings
C->>C: Step 4 escalate unresolved to user
C->>C: Step 5 harden-finding for fixed classes
end
C->>GH: gh pr comment (one reply per thread)
C-->>U: report (verdict + action + verification per claim)
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
plugins/dev-workflow/agents/finding-triage.md:79-84
**Self-enforced tool-call budget has no backstop**
The 25-call ceiling ("Nothing counts these for you; this is a rule you keep") relies entirely on the model following the instruction. A deeply nested claim — or one whose evidence path branches repeatedly — could quietly exhaust the budget before the model recognises it has reached 25, producing a verdict based on partial evidence with no indication to the caller. The acknowledged limit is an accepted trade-off, but it may be worth noting in the output spec that an agent which ran many tool calls without settling should prefer `escalate-to-user` over a low-confidence `accept`/`dismiss`.
### Issue 2 of 2
plugins/dev-workflow/commands/process-pr-review.md:43-48
**Bare `agents/` in the precheck is broader than stated agent locations**
The instruction-path precheck skips subagent triage for any PR that touches anything under an `agents/` directory at any depth. However, the standard agent locations are `.claude/agents/` and `plugins/*/agents/` — both already covered by the `.claude/` and `plugins/` entries in this same list. A project with an `agents/` directory used for application code (not Claude instructions) would unnecessarily fall back to manual validation for every PR touching it. The comment "deliberately wider than the gate hook's" explains the conservatism but doesn't note this overlap. A brief clarifying note that bare `agents/` is extra conservatism covering a non-standard layout would help future maintainers avoid over-widening the list further.
Reviews (1): Last reviewed commit: "feat(agents): add finding-triage, a read..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+79
to
+84
| **Stop at 25 tool calls** — Read, Grep and Glob counted alike, repeats included — or at | ||
| your first verdict, whichever comes first. The number is a deliberate ceiling: a claim | ||
| that needs more than about two dozen reads is one that reading cannot settle, and saying | ||
| so is more useful than a fortieth file. Nothing counts these for you; this is a rule you | ||
| keep. On reaching 25 without settling the claim, return `escalate-to-user` and name the | ||
| evidence that would settle it. |
There was a problem hiding this comment.
The 25-call ceiling ("Nothing counts these for you; this is a rule you keep") relies entirely on the model following the instruction. A deeply nested claim — or one whose evidence path branches repeatedly — could quietly exhaust the budget before the model recognises it has reached 25, producing a verdict based on partial evidence with no indication to the caller. The acknowledged limit is an accepted trade-off, but it may be worth noting in the output spec that an agent which ran many tool calls without settling should prefer escalate-to-user over a low-confidence accept/dismiss.
Prompt To Fix With AI
This is a comment left during a code review.
Path: plugins/dev-workflow/agents/finding-triage.md
Line: 79-84
Comment:
**Self-enforced tool-call budget has no backstop**
The 25-call ceiling ("Nothing counts these for you; this is a rule you keep") relies entirely on the model following the instruction. A deeply nested claim — or one whose evidence path branches repeatedly — could quietly exhaust the budget before the model recognises it has reached 25, producing a verdict based on partial evidence with no indication to the caller. The acknowledged limit is an accepted trade-off, but it may be worth noting in the output spec that an agent which ran many tool calls without settling should prefer `escalate-to-user` over a low-confidence `accept`/`dismiss`.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Owner
Author
There was a problem hiding this comment.
Accepted — fixed in bf875b1.
Right that the ceiling has no backstop, and the failure you describe is the dangerous one: a verdict formed on partial evidence looks identical to a confident one from the caller's side. The definition now says to prefer escalate-to-user over a low-confidence accept/dismiss when the search has run long, which is the part the caller can actually act on.
Keeping it instruction-level rather than adding a mechanism: nothing in the agent format counts tool calls, and maxTurns caps turns rather than calls, so it would truncate mid-check without producing a verdict. Stated as a rule the agent keeps, not as something enforced.
Comment on lines
+43
to
+48
| subagent triage for this PR entirely: validate the comments yourself and say so in | ||
| each reply. The paths are `CLAUDE.md`, `CLAUDE.local.md` and `AGENTS.md` at any | ||
| depth, anything under `.claude/`, `plugins/`, `skills/`, `commands/` or `agents/`, | ||
| and every file reached by expanding `@path` imports from those, transitively. Skip | ||
| triage — never proceed on a partial set — whenever an import is malformed, missing, | ||
| resolves outside the checkout, or resolves more than one way. |
There was a problem hiding this comment.
agents/ in the precheck is broader than stated agent locations
The instruction-path precheck skips subagent triage for any PR that touches anything under an agents/ directory at any depth. However, the standard agent locations are .claude/agents/ and plugins/*/agents/ — both already covered by the .claude/ and plugins/ entries in this same list. A project with an agents/ directory used for application code (not Claude instructions) would unnecessarily fall back to manual validation for every PR touching it. The comment "deliberately wider than the gate hook's" explains the conservatism but doesn't note this overlap. A brief clarifying note that bare agents/ is extra conservatism covering a non-standard layout would help future maintainers avoid over-widening the list further.
Prompt To Fix With AI
This is a comment left during a code review.
Path: plugins/dev-workflow/commands/process-pr-review.md
Line: 43-48
Comment:
**Bare `agents/` in the precheck is broader than stated agent locations**
The instruction-path precheck skips subagent triage for any PR that touches anything under an `agents/` directory at any depth. However, the standard agent locations are `.claude/agents/` and `plugins/*/agents/` — both already covered by the `.claude/` and `plugins/` entries in this same list. A project with an `agents/` directory used for application code (not Claude instructions) would unnecessarily fall back to manual validation for every PR touching it. The comment "deliberately wider than the gate hook's" explains the conservatism but doesn't note this overlap. A brief clarifying note that bare `agents/` is extra conservatism covering a non-standard layout would help future maintainers avoid over-widening the list further.
How can I resolve this? If you propose a fix, please make it concise.
Owner
Author
There was a problem hiding this comment.
Accepted — clarifying note added in bf875b1.
Correct on the overlap: .claude/agents/ and plugins/*/agents/ are both already covered, so bare agents/ only adds a non-standard layout, at the cost you name — a project using agents/ for application code falls back to manual validation on those PRs.
Kept rather than removed, because for this list over-skipping is cheap and under-skipping means an attacker-influencable PR rewrites the verifier's instructions. But your real point stands: without the reasoning written down the next maintainer widens it again. That cost is now in the file, along with the note that further widening needs the same justification.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/coding-workflow.md`:
- Line 71: Update the cross-reference sentence in coding-workflow.md to remove
the “agent definitions” claim, leaving only the skills, commands, and hook
messages covered by getting-started.md. Preserve the existing reference wording
and point agent-definition readers to the separate established reference if
needed.
In `@docs/getting-started.md`:
- Around line 52-56: Update the documentation paragraph describing
dev-workflow:finding-triage so it explicitly separates factual-validity triage
from the command’s actionability and scope checks. State that an accepted
finding is only eligible for implementation after those checks pass, while
preserving the existing review and Gate B behavior.
In `@docs/superpowers/plans/2026-07-18-finding-triage-agent.md`:
- Around line 784-793: Update the enforcement-claim scan in the Tasks 1–6 review
instructions to pass the artifacts changed by those tasks as explicit grep
operands, or pipe a verified changed-file list into grep. Preserve the existing
pattern and follow-up review of absolute claims, while ensuring the command
scans files instead of reading indefinitely from standard input.
In `@docs/superpowers/specs/2026-07-18-subagent-definitions-design.md`:
- Around line 276-295: Align the actionability contract in
docs/superpowers/specs/2026-07-18-subagent-definitions-design.md:276-295 with
the command by placing settled-decision handling first, mapping it to the
correct command step, and adding the large pre-existing finding recorded in
todos.md as a terminal out-of-scope outcome under ## Done; preserve the rule
that accept alone never authorizes a fix. In
plugins/dev-workflow/commands/process-pr-review.md:104-121, make no direct
change and retain the existing settled-decision-first ordering and terminal
handling.
- Line 87: Clear the Markdown quality-gate violations by adding appropriate
language identifiers to the fences at
docs/superpowers/specs/2026-07-18-subagent-definitions-design.md:87 and :212,
and adding blank lines around the table at :286; add languages to all reported
fences in docs/superpowers/plans/2026-07-18-finding-triage-agent.md at :236,
:242, :250, :257, :267, :273, :281, :288, :297, :305, :351, :357, :365, :372,
:381, :390, :402, :410, :421, :427, :435, :441, :465, :480, :580, :587, :618,
:626, :641, :649, :685, :691, :699, :707, :720, and :726; add languages to both
example fences in plugins/dev-workflow/agents/finding-triage.md at :105 and
:128, and to the report example fence in
plugins/dev-workflow/commands/process-pr-review.md at :143.
In `@plugins/dev-workflow/commands/workflow-init.md`:
- Around line 417-420: Update the prompt-scope paragraph to include both
.claude/agents/ and plugins/*/agents/ as agent-definition locations, matching
Gate B coverage. Replace the narrower “skill specs” wording with “prompt specs”
so the checklist consistently applies to all listed prompt types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be877bc3-9113-4f54-bf73-3f38d0be6c47
📒 Files selected for processing (16)
.claude-plugin/marketplace.jsonAGENTS.mdCLAUDE.mdREADME.mddocs/architecture.mddocs/coding-workflow.mddocs/getting-started.mddocs/prompt-standards.mddocs/superpowers/plans/2026-07-18-finding-triage-agent.mddocs/superpowers/specs/2026-07-18-subagent-definitions-design.mdplugins/dev-workflow/.claude-plugin/plugin.jsonplugins/dev-workflow/agents/finding-triage.mdplugins/dev-workflow/commands/process-pr-review.mdplugins/dev-workflow/commands/workflow-init.mdplugins/dev-workflow/skills/harden-finding/SKILL.mdscripts/check-invariants.sh
…arify scope Processing PR #4 reviewer comments: 8 findings, 7 accepted, 1 dismissed. CodeRabbit (major) — the spec and the command disagreed on actionability. Gate B made settled-decision take precedence and made the out-of-scope row terminal, and added that terminal to Done; I changed the command and never propagated it to the spec. That is docs-drift, this repo's own class, created between the artifact that was reviewed and the one that was implemented. Spec now matches, with a note that Gate B is what found the ordering problem. CodeRabbit (major) — the plan's enforcement-claim scan was `grep -nE 'pattern'` with no file operands, so it reads standard input and waits. An implementer would have seen a hung step, not a scan. Now takes the changed-file list. CodeRabbit (major) — the scaffolded prompt-standards template listed only .claude/agents/, so a downstream plugin agent could be Gate-B covered but outside the checklist's stated scope. Now covers plugins/*/agents/ too, and says "prompt specs" rather than "skill specs". Greptile — the 25-call budget is instruction-level with nothing counting for the agent, so it may notice late that it has read widely. The definition now says to prefer escalate-to-user over a low-confidence verdict, since the caller cannot distinguish a hesitant accept from a confident one. Greptile — bare `agents/` in the precheck is redundant against .claude/ and plugins/ and only covers a non-standard layout. Kept (over-skipping is the safe direction for injection) with that cost now written down, so the list is not widened further without the same justification. CodeRabbit (minor) x2 — getting-started now says triage judges truth only and the command decides scope separately; two spec code fences gained a language. DISMISSED — CodeRabbit said coding-workflow.md's cross-reference wrongly claims getting-started.md covers agent definitions. It does: getting-started.md:52 walks through dev-workflow:finding-triage in step 8. Gate B skipped per CLAUDE.md §5 (trivial): clarifying clauses and a doc alignment, no behaviour change to any prompt's contract. Battery green — shellcheck x4, hook suite, 61 invariant assertions, invariant scan, plugin validate --strict.
dsnger
added a commit
that referenced
this pull request
Jul 18, 2026
…to rung P (#5) * chore(harden): enforcement-claim checklist item; escalate docs-drift to rung P Two reactive hardenings, one ledger row each, both surfaced by the gates or a bot. 1. unverified-enforcement-claim (new class) -> rung P Six citable instances across one spec, its plan and its diff where something was described as enforced, caught or guaranteed and no mechanism did it. Each was caught by a review gate; none by me. New docs/prompt-standards.md item 11 requires such a claim to name its mechanism and the author to verify the mechanism exists first. Mirrored into the scaffolded template. The class is minted rather than filed as docs-drift: drift is two artifacts disagreeing, this is one claim untrue on its own terms even with nothing to contradict it. Different fix, so different grep. How the count was settled is the best argument for the item. It was asserted as six without checking, "corrected" to five from memory, and only settled at six by reading the commits — two unverified claims in the course of hardening against unverified claims. 2. docs-drift -> rung P (escalated from the 2026-07-18 `1 prose` row) A Gate-B fix changed behaviour the approved spec specified and the spec was never updated, so the two disagreed until a PR bot found it. The prior rung was an AGENTS.md rule about manifest claims — too narrow to reach this. The rule now lives in the Gate-B instructions themselves: a fix that changes specified behaviour is not complete until the spec is updated in the same commit. No mechanical rung: deciding whether a fix altered behaviour a spec pins down needs both artifacts read, and the crude proxies fire on every change no spec covers. BOT MATRIX Greptile posted summary-only findings on PR #1 and two valid inline comments on PR #4. Its column now reads `inconsistent` with both observations dated, rather than being reclassified from one more data point. That exposed a routing bug: `inconsistent` was not consumable by a command that knew only line-findings-vs-summary. The Wait-for list is now the routing authority in all four copies — the command's Steps 0 and 2, the matrix, AGENTS.md's tree comment, and the scaffolded template — with the table descriptive. The scaffold no longer derives Wait-for from a column that stopped holding yes/no. Also corrected: a fabricated "four days apart" (the observations are hours apart on the same day, timestamps cited), and "both post a check" (the API returns only quality and CodeRabbit for that head, so Greptile's completion is watched via its comment instead). Gate B: five passes, final clean on both reviews. Battery green — shellcheck x4, hook suite, 61 invariant assertions, invariant scan, plugin validate --strict. * docs(review): wait on completion signals, not on a bot posting Processing PR #5's comments. Step 0 fired — the PR touches AGENTS.md, CLAUDE.md and two commands — so subagent triage was skipped and these were validated here. First live use of that rule, working as designed. CodeRabbit (major) — the scaffolded template said processing starts once each bot "has posted, even with zero comments". A bot that finds nothing can complete via its status check without posting anything, so that instruction hangs forever on a clean review. It is also the exact hang docs/pr-review-bots.md exists to prevent, reintroduced by my own wording while fixing the routing model. Now waits on the per-bot completion signal recorded in Notes, and says why a post is not it. Greptile (P2) — item 11's actionable remedy was appended mid-line to the anecdote paragraph, so a reader skimming for the rule had to read the story first. The remedy is now its own paragraph ahead of the anecdote, matching how the scaffolded copy already reads. CodeRabbit (minor) — MD028: the plan's historical note sat directly above the pre-existing agentic-workers blockquote with a bare blank line between, which renderers may merge. Joined with a quoted separator. Checked the spec for the same pattern; its blockquote is followed by prose, so it does not apply. Battery green. Gate B skipped per CLAUDE.md §5 (trivial): three prose edits, no change to any contract.
dsnger
added a commit
that referenced
this pull request
Jul 18, 2026
…ic category Folding in the third and fourth data points, and correcting what the earlier rows claimed. The verification revised the premise. My earlier queries filtered issue comments on the login `greptile-apps`, but they are authored by `greptile-apps[bot]`, so they returned zero and looked like absence. Across #1, #2, #4 and #5 the summary comment was posted EVERY time; it is the inline comments that varied — present on #2, #4 and #5, absent only on #1. So the row now reads "summary always; inline usually", not "inconsistent findings channel". The login trap is written down, since it is what produced the wrong reading. The completion signal is the genuinely unreliable part: `gh pr checks` displayed a Greptile entry for #4 and #5, while the check-runs and statuses APIs return none for any observed head. Two tools, two answers, so neither is proof. That forced a third routing category. Greptile is a real findings source with no signal you can block on — blocking hangs the loop, dropping it loses findings, and the file exists to prevent exactly those two failures. Routing is now: wait for (block), process opportunistically (never block), ignore. Propagated to process-pr-review Steps 0 and 2 and the scaffolded template in the same commit, rather than leaving copies disagreeing — which is the drift this repo hardened against two commits ago. Docs-only; no executable changed. Battery green.
dsnger
added a commit
that referenced
this pull request
Jul 19, 2026
…#7) * feat(ci): require a version bump when the shipped plugin changes (invariant 12) An installed plugin lives under a version-keyed cache path, so a change that merges without a bump never reaches an installed copy. That happened twice: a machine ran 0.1.0 while main was at 0.4.0, and the 0.4.0 bump had to be asked for during PR #4. main was still carrying two un-released plugin commits when this was written. scripts/check-version-bump.sh fails a PR that changes any path under a plugins/<name>/ directory still present at HEAD without changing that plugin's manifest version. Both versions are read from commits, every git call's status is checked, and it fails closed on anything unexpected — a false skip would be a silent green on exactly the defect it exists to catch. 36 assertions, mutation-verified. Also here: a backfilled plugin CHANGELOG (0.1.0-0.4.1), the artifact-version-not-bumped taxonomy class and its ledger row, and the version bump this very check now requires. Rung 2 per dev-workflow:harden-finding. * test(version-bump): fail loudly when the O5/O6 rebase does not happen Both fixtures rebased `feature` onto main with `|| true`. A failed rebase left the branch at its old base, so the malformed manifest never reached the merge-base and the row passed for the wrong reason — a suite lying about what it covers. `rebase_onto_main` now names the setup in the failure and aborts a partial rebase. Verified by pointing it at a non-existent branch: two setup failures plus both row failures, 36/36 again after restore. Also drops the IFS/`set -f` restore before two `die` calls. `die` exits, so it was dead code that implied the cleanup was sometimes load-bearing. Both raised by Greptile on #7 (P2), validated before applying.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the plugin's first agent definition and wires it into
/dev-workflow:process-pr-review. Convention-loaded fromagents/, soplugin.jsongains only the version. 0.4.0.What it does — and deliberately does not
Given one claim and where to look: is that claim true of the code? Nothing else.
Actionability, staleness and deduplication stay with the command, which has
Bash, git and the other comments. The agent has none of those, and the split is the point:finding-triageprocess-pr-reviewalso gained an actionability step before implementation:acceptestablishes truth, not permission to change code.Two boundaries stated rather than oversold
Read-only means exactly: it cannot directly invoke a Claude Code write or shell tool, enforced by the
toolsallowlist. It does not mean nothing changes on disk — hooks in the user's own settings can run on its tool calls.Instruction injection. Custom subagents load the whole
CLAUDE.mdhierarchy with no per-agent opt-out (verified against the docs, not assumed). A PR editing an instruction file would rewrite the rules its own reviewer runs under — so triage is skipped entirely for such PRs and the command validates those comments itself.What the gates actually changed
This is the part worth reading. The gates didn't rubber-stamp a design; they deleted half of it and rebuilt the rest.
Gate A dropped a whole agent. The original brief scoped two. Pass 1 showed
task-verifierduplicatedsuperpowers:subagent-driven-development, which already dispatches a per-task spec-compliance reviewer — andexecuting-planstells you to use that skill when subagents are available. So on the platform where it could run, the reviewer already existed; the path lacking one can't run subagents. The niche collapsed. Its residual value and a trigger condition for revisiting are recorded in the spec rather than discarded.Gate A rewrote the survivor's contract. Pass 2 found a blocker: the agent was asked to decide whether a fix "belongs in this PR", which needs a base/head comparison it cannot derive with
Read/Grep/Glob. Unimplementable as written. Hence the truth/actionability split above.Gate A caused a deliberate simplification. By pass 4 the design had accumulated a SHA pair, a worktree fingerprint and a snapshot barrier — each closing a gap opened by the last. The spec carried a kill condition: converge in two passes or drop the feature. It converged by cutting the contract, not patching it.
Gate B caught me over-claiming against my own hook. I'd added a bare
agents/to the Gate-B path lists while keeping "the hook classifies paths the same way" — butis_prompt_pathmatches.claude|plugins|skills|commands, notagents. Verified empirically:agents/x.mddoes not match;plugins/*/agents/and.claude/agents/do. Removed, with a note so nobody creates a top-levelagents/assuming the gate sees it. The hook is byte-identical.A pattern this PR exposed
Six times across the spec, plan and diff, I wrote that something was enforced, caught or guaranteed when no mechanism did it — "a rogue write cannot be invisible", "all four gaps are in todos.md", "output validation catches a budget overrun", and three more. Each was caught only by the gate.
That's a pattern in how specs get written here, not six coincidences. It gets its own
harden-findingrun as a follow-up: rungP, adocs/prompt-standards.mditem requiring every enforcement claim to name its mechanism, and the mechanism to be verified before the claim is written.Verification
Canonical quality command from
AGENTS.md § Commands, run verbatim — shellcheck ×4, hook suite, 61 invariant assertions, invariant scan,plugin validate --strict: green.plugins/dev-workflow/hooks/byte-identical;plugin.jsonchanged by one line.Spec:
docs/superpowers/specs/2026-07-18-subagent-definitions-design.mdPlan:
docs/superpowers/plans/2026-07-18-finding-triage-agent.mdSummary by CodeRabbit
New Features
Documentation
Chores