fix(claude-review): direct the review lane to post line-anchored inline comments - #338
Conversation
…ne comments
The lane has been granted
`mcp__github_inline_comment__create_inline_comment` since the lanes
landed, and the effective SDK `allowedTools` echo confirms it reaches the
agent — but the assembled prompt never mentioned it, while the action's
own base prompt routes every response into the single sticky comment
("Never create new comments. Only update the existing comment using
mcp__github_comment__update_claude_comment") and lists submitting a
formal PR review under what the agent cannot do. An unmentioned grant
under that steering reads as unusable, and the lane has produced zero
inline comments fleet-wide.
The security lane carries the identical grant and DOES produce
line-anchored comments; the only prompt-level difference is that its
criteria body says "Report findings as a pull request review". This adds
the equivalent instruction to the code-review lane, and names the tool
outright so the grant is discoverable rather than merely present.
Placed in the step's prompt wrapper rather than the `prompt` input
default: the input's documented contract is "just the instructions for
what to look for", so reporting mechanics belong with the structural
header and permalink form — and the wrapper survives a caller that
overrides the criteria body.
The pre-existing permalink guidance is scoped rather than removed: it now
covers findings no changed line can carry, so it no longer competes with
the inline-comment instruction for the same finding.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 2s —— View job Code review:
|
|
Claude finished @kyle-sexton's task in 2m 25s —— View job Security Review
Scope: the single changed file, What changed: prompt-text only. Both attempt copies (first + retry, byte-identical as claimed) gain a "report findings as a pull request review" instruction that names Deferred to zizmor (per instructions): action pin freshness, trigger-type safety, permission scoping, template-injection shape of the Finding posted inline (SUGGESTION / MEDIUM confidence) on lines 486–491: the tool grant itself predates this PR, but this change is what makes the agent actually call it on every finding, which meaningfully widens the practical trust surface for prompt-injection-driven inline comments (including one-click-appliable No CRITICAL or IMPORTANT issues found. No authorization/access-control gaps, injection surfaces, credential-handling changes, or dangerous trigger/permission patterns introduced by this diff. |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
![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: 2e7c5ed388
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…rides
The wrapper prompt names mcp__github_inline_comment__create_inline_comment
unconditionally and asserts it is granted to this lane, but the grant lived
in the `claude-args` input default — which a caller replaces wholesale. Any
consumer passing claude-args for an unrelated reason (selecting a model, say)
silently dropped it, leaving the prompt directing every line-anchorable
finding at a tool that was not there.
The failure is worse than an un-granted tool. In tag mode the action derives
which GitHub MCP servers to install from these same args
(`parseAllowedTools` -> `prepareMcpConfig({ allowedTools })`), so without the
grant the inline-comment server is never installed at all.
Fix: the compose step appends the grant unconditionally, and it comes out of
the overridable default. That step feeds both the first attempt and the
retry, so one edit covers both. Repeated --allowedTools flags accumulate and
dedupe rather than overwrite (parse-sdk-options.ts ACCUMULATING_FLAGS, with
tests), so the append only ever adds to whatever the caller passed — and tag
mode already relies on that union, emitting its own --allowedTools before
appending the caller's.
`Bash(gh pr diff:*)` deliberately stays in the overridable default: the
instruction that uses it lives in the equally overridable `prompt` input, so
the two co-vary correctly. The invariant is that whatever the
non-overridable wrapper prompt asserts must itself be non-overridable.
Verified against the pinned action's own parser: the default path yields the
same effective tool set as before this change, every override scenario keeps
the grant, and the pre-fix override yields an empty tool list.
Also records the widened trust surface in the SECURITY MODEL block. The
inline tool is now on the hot path, so untrusted diff content reaches a
rendered comment that can carry an appliable suggestion block. The action
sanitizes bodies for hidden markup and leaked tokens but not for meaning,
and its buffer-and-classify step targets test/probe comments and is skipped
when the agent passes confirmed: true — neither is an injection control.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 45s —— View job Security Review
Scope: this run reviews the full PR diff (base What changed: this commit is the fix for chatgpt-codex-connector's P2 finding on the prior revision — a caller supplying a custom
Verified:
Assessment: no new security issues in this delta. The change is a sound, narrowly-scoped fix — it doesn't widen the trust surface further (the tool was already granted and driven every finding as of the prior revision); it only makes that grant durable against caller overrides and documents the previously-flagged widened-surface tradeoff inline in the file. No CRITICAL or IMPORTANT issues found; no authorization/access-control gaps, injection surfaces, credential-handling changes, or dangerous trigger/permission patterns. |
…ents Independent verification of the previous commit turned up two statements that are not true, in a file where the comments are the contract. The SECURITY MODEL bullet said the action's buffer-and-classify step drops test/probe comments. It drops nothing in this lane: classification requires ANTHROPIC_API_KEY, and this lane passes only claude_code_oauth_token, so classifyComments returns null and every buffered comment posts. The error ran in the reassuring direction — it described a filter that is not there — so the note now says the step fails open here. The step comment claimed a granted tool the prompt never mentions "reads as unusable and goes uncalled". This repo's own security lane disproves it: that lane grants the tool, never names it in its prompt, runs in the same tag mode under the same base prompt, and does post inline comments. The load-bearing part is instructing the agent to report findings as a pull request review, which is what that lane has and this one lacked. Naming the tool is belt-and-braces, and the comment now says so. Also scoped the claude-args note: appending the grant defeats a caller replacing the default, not an explicit --disallowedTools for that tool. No behavior change; comments and input description only. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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. |
|
Claude finished @kyle-sexton's task in 58s —— View job Security Review
Scope: Claude has already reviewed this PR through commit What changed: two
Assessment: this delta is documentation-only — it corrects two previously-inaccurate explanatory comments to match the workflow's actual behavior, and does not itself change what the workflow does. It doesn't touch triggers, permissions, secrets, tool grants, or checkout behavior, and doesn't reopen or narrow anything from the prior review round (the durable-grant fix for the codex P2 finding, reviewed last round, is unchanged here). No CRITICAL or IMPORTANT issues found; no authorization/access-control gaps, injection surfaces, credential-handling changes, or dangerous trigger/permission patterns in this delta. |
1 participant
Summary
The
claude-reviewlane has posted zero line-anchored inline commentsfleet-wide (~10,563 all-time
claude-review*runs, 3,161 PRs), despitemcp__github_inline_comment__create_inline_commentbeing granted and reachingthe agent — the effective
SDK optionsecho lists it. The tool was installedand never called.
Why it was never called. The assembled prompt never mentioned the tool,
while the pinned action's own base prompt steers hard the other way:
Never create new comments. Only update the existing comment using mcp__github_comment__update_claude_comment.You communicate exclusively by editing your single comment - not through any other means.- Submit formal GitHub PR reviewsThat text is authored by
anthropics/claude-code-actionand cannot be changedhere. What can change is the lane's own prompt, which is appended after it and
gets the last word.
What made the difference.
claude-security-review.ymlcarries the identicaltool grant and does produce line-anchored comments. The only prompt-level
delta is that its criteria body says "Report findings as a pull request
review." This change gives the code-review lane the equivalent instruction and
additionally names the tool, so the grant is discoverable rather than merely
present.
Placement. In the step's prompt wrapper, not the
promptinput default. Theinput's documented contract is "just the instructions for what to look for", so
reporting mechanics belong beside the structural header and permalink form — and
the wrapper survives a caller that overrides the criteria body. Both the first
attempt and the retry carry byte-identical prompt blocks.
The pre-existing commit-permalink guidance is scoped, not removed: it now
covers findings no changed line can carry, so it no longer competes with the
inline-comment instruction for the same finding.
Deliberate non-changes
display-reportandshow_full_outputstay off. Public-repo log hygieneis untouched. (
show_full_output: falseis exactly why the logs cannotdistinguish "never called" from "called and failed" — an accepted limitation,
not a bug fixed here.)
reusable workflow, so the runner-policy contract (
allowedInputsfor thisworkflow) is unaffected.
skip-actorsandallowed_botsare untouched.melodic-software/standards.REVIEW.mdis the sharedreview-criteria prose and is also injected into Managed Code Review and local
review-plugin runs; it says nothing about output form today, and laneplumbing (which MCP tool to call) does not belong there. Its downstream
materialization in this repo is a managed sync target and was not touched.
instruction sits in its
promptinput default, this one's sits in the wrapper.That is deliberate scope discipline, not an oversight; the wrapper is the
correct home per the input's own contract, and normalizing the security lane is
a separate change.
Test plan
A prompt change cannot be proven by unit test, so this was demonstrated live.
Setup. Throwaway PR #337 (
demo/332-inline-comment-evidence) based on thisbranch, adding one Node script with genuine, line-anchorable defects.
claude-review-self.ymluses the local./ref, so the review ran against themodified reusable (
create_inline_commentappears 3x in the demo tree'sworkflow vs 1x on
main). The changed file matches none of the security lane'spathspatterns, sosecurity-reviewskipped — everyclaude[bot]inlinecomment on that PR is attributable to the code-review lane.
Result — 5 line-anchored inline comments where there were previously zero:
Attribution controls:
— agent
init21:53:04Z →resultsuccess21:55:13Z, 20 turns. All fivecomments landed 21:54:39Z–21:54:56Z, strictly inside that window.
security-review / security-reviewjob: skipped (run 30768850685).chatgpt-codex-connector[bot], adifferent reviewer, grouped under a single review id — not this lane.
create_inline_commentproduces.Note for future debugging: the action still logs
No buffered inline commentson this run even though five were posted. That step covers a different(batched) path;
create_inline_commentposts immediately. The log line is nota usable signal for whether the tool was called.
actionlintclean; both prompt blocks verified byte-identical.Review round
Two threads, both addressed with code.
P2 (codex) — the inline grant did not survive a caller overriding
claude-args. Real, and worse than reported. The grant lived in theclaude-argsinput default, which a caller replaces wholesale, so a consumer passingclaude-argsfor an unrelated reason (selecting a model) dropped it silently — while the non-overridable wrapper prompt still asserted the tool was granted. And because this lane runs in tag mode, the action derives which MCP servers to install from these same args (parseAllowedTools→prepareMcpConfig), so the inline-comment server would not even be installed.Fixed in 923a2d2:
compose-argsappends the grant unconditionally, and it comes out of the overridable default. That step feeds both the first attempt and the retry, so one edit covers both. The invariant, now written into the file: whatever the non-overridable wrapper prompt asserts must itself be non-overridable.Bash(gh pr diff:*)deliberately stays overridable, because the instruction that uses it lives in the equally overridablepromptinput.Safe because repeated
--allowedToolsflags accumulate and dedupe rather than overwrite (ACCUMULATING_FLAGS, both spellings, upstream-tested) — tag mode already relies on this, emitting its own--allowedToolsbefore appending the caller's. Verified by executing the pinned action's realparseAllowedToolsagainst the stringscompose-argsemits: the default path yields an effective tool set identical to pre-fix (no happy-path regression), every override scenario keeps the grant, and the pre-fix override reproduces the bug as an empty list.SUGGESTION (claude) — widened trust surface. Asked for no code change, only a
SECURITY MODELnote; added.Corrections this round (8d7ab14)
Independent verification caught two statements that were not true, in a file where the comments are the contract.
SECURITY MODELnote repeated the review thread's claim that the action buffers and classifies inline comments before posting. It filters nothing in this lane: classification requiresANTHROPIC_API_KEY, and this lane passes onlyclaude_code_oauth_token, soclassifyCommentsreturnsnulland every buffered comment posts. The error ran in the reassuring direction, describing a control that is not there.The empirical result the PR rests on is unaffected: the five line-anchored comments on #337 stand. Only the causal explanation was overstated.
Follow-ups filed, deliberately not folded in
claude-security-review.ymlhas the identical latent gap (same grant in its overridable default, forwarded verbatim at two call sites). It has no compose step, so closing it needs a new one.Related
Closes #332.
Follow-ups from review: #340, #341.
Related: #337 (throwaway evidence PR,
do-not-merge, deleted after review).The
Bash(gh pr diff:*)grant flagged during investigation is already fixed onmainby ef63101 (#319) and is present in the effectiveallowedTools— thestale
fix/309-claude-review-gh-pr-diff-grantremote branch is superseded.