Skip to content

fix(claude-review): summary-comment prompt half breaks when track-progress is false #343

Description

@kyle-sexton

PR #338 established an invariant in claude-review.yml: whatever the non-overridable wrapper prompt asserts must itself be non-overridable. It closed the claude-args leg of that invariant. The track-progress leg is still open.

The gap

The wrapper prompt now tells the agent where non-line-anchorable findings go:

Keep your summary comment for the overview.

A finding no changed line can carry — cross-file, or about the PR as a whole — goes in the summary comment instead

That prompt is not overridable. But track-progress is a supported input, and setting it false removes the artifact the prompt points at:

  • track-progress: false on a pull_request event means src/modes/detector.ts no longer forces tag mode, so the lane runs in agent mode.
  • In agent mode install-mcp-server.ts:95 computes shouldIncludeCommentServer = !isAgentMode || hasGitHubCommentTools. This lane grants no mcp__github_comment__* tool, so that is false and the comment server is not installed.

The agent is then instructed to put a class of findings into a summary comment it has no tool to create. Inline comments still work — the inline server installs on its own hasInlineCommentTools check, which #338 made durable — so the failure is partial: line-anchorable findings land, everything else has nowhere to go.

Severity: low

track-progress's own description already concedes that turning it off means clean reviews produce no visible output (it exists to mitigate upstream #1071). So a consumer setting it false has already accepted reduced visibility. This is a sharpening of a known tradeoff, not a surprise.

It is worth closing anyway because it is the same defect class as the P2 that #338 fixed — a non-overridable prompt asserting something a supported input can falsify — and because the failure is silent.

Options

  1. Condition the summary-comment half of the prompt on track-progress. Keeps both modes honest, at the cost of a conditional prompt (and the prompt block is duplicated across the first attempt and the retry, so it doubles — see test(claude-review): enforce first-attempt/retry prompt-block parity #341, which would enforce the two stay identical).
  2. Grant mcp__github_comment__update_claude_comment durably the same way fix(claude-review): direct the review lane to post line-anchored inline comments #338 grants the inline tool, so the summary comment exists in both modes.
  3. Document track-progress: false as unsupported for this lane and reject it, making the input's contract explicit.

Not obvious which is right; option 2 is the smallest change but widens the tool grant, and option 1 is the most honest but fights the byte-identical-prompt invariant.

Found by the independent pre-merge verifier on #338, outside the scope of the threads it was resolving.

Context: #338, #341.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-triageNot yet classified. Floor until a type and one priority tier are set.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions