From eb05251825fd07bcfee6ac44d715d3ddda9b714c Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Fri, 1 May 2026 07:13:46 -0700 Subject: [PATCH 1/2] ci(claude): marker-based review-comment edit-in-place; fixes mention-collision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Background: PR #432's `--edit-last` mechanism filters by authenticated identity (`claude[bot]`) only. After a `@claude` mention, the most recent `claude[bot]` comment is the mention response — so the next review run's `--edit-last` clobbers it instead of editing the prior review. Observed on HarperFast/oauth#67. Fix — three coordinated changes: 1. **Sentinel marker.** Every review comment body now MUST begin with the literal first line ``. This is what distinguishes review comments from mention responses, which never carry the marker. 2. **Workflow lookup step + agent-side targeted edit.** A new `Find prior review comment` step runs before the Claude review step. It queries the issue-comments REST endpoint, filters for `claude[bot]` + marker-prefixed bodies, and exposes the integer database ID (or empty) as `steps.prior_review.outputs.id`. The agent reads this via the prompt's new `PRIOR_REVIEW_COMMENT_ID` field and: - if non-empty, edits via `gh api -X PATCH .../issues/comments/` - otherwise posts fresh via `gh pr comment ` Either way, the body starts with the marker. Adds `Bash(gh api:*)` to `--allowedTools`. Token only carries `pull-requests: write` / `contents: read` / `id-token: write`, so the broader API surface is contained to the same single PR. 3. **Log step now filters by marker too.** Same logic that the review uses — marker'd comment via raw API, claude[bot] login. Also switched the staleness guard from `created_at` to `updated_at`, since edit-in-place leaves `created_at` frozen at the original post time and we need the most recent activity to judge whether the body is fresh-this-run or stale. Existing un-marker'd review comments on active PRs will be orphaned (the next review run finds no prior, posts a marker'd fresh one). Acceptable; old comments stay as historical record. Mirror to oauth follows in a separate PR. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/claude-review.yml | 104 +++++++++++++++++++++++----- 1 file changed, 86 insertions(+), 18 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index d2c97d7b7..751263d9b 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -110,6 +110,29 @@ jobs: echo "${DELIM}" } >> "$GITHUB_OUTPUT" + - name: Find prior review comment + # `--edit-last` (the previous mechanism) filters by authenticated + # identity (`claude[bot]`) only — so after a `@claude` mention, + # the most recent claude[bot] comment is the mention response + # and `--edit-last` clobbers it. Marker-based lookup fixes that: + # every review comment starts with ``, + # which mention responses never carry. Mention responses are + # safe; only the marker'd review comment is editable here. + id: prior_review + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + set -uo pipefail + EXISTING_ID=$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ + --jq '[.[] | select(.user.login == "claude[bot]") | select(.body | startswith(""))] | last | .id // empty') + if [ -n "$EXISTING_ID" ]; then + echo "Prior review comment: $EXISTING_ID" + else + echo "No prior review comment found — agent will post fresh." + fi + echo "id=${EXISTING_ID}" >> "$GITHUB_OUTPUT" + - name: Claude review id: claude-review uses: anthropics/claude-code-action@c3d45e8e941e1b2ad7b278c57482d9c5bf1f35b3 # v1.0.99 @@ -139,10 +162,19 @@ jobs: # path-bound is enforced via prompt; deviations are # contained because the runner is ephemeral and has no # write credentials beyond `gh pr comment`. - --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr view:*),Read,Grep,Glob,Write,Bash(git diff:*),Bash(git log:*),Bash(git blame:*),Bash(git show:*)" + # `Bash(gh api:*)` is needed for editing a prior review + # comment by integer database ID (the only way to target a + # specific comment instead of "the most recent claude[bot] + # comment", which `--edit-last` defaults to and which + # collides with `@claude` mention responses). The token + # only carries `pull-requests: write` / `contents: read` / + # `id-token: write`, so blast radius from broader API + # access is the same single PR. + --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr view:*),Bash(gh api:*),Read,Grep,Glob,Write,Bash(git diff:*),Bash(git log:*),Bash(git blame:*),Bash(git show:*)" prompt: | REPO: ${{ github.repository }} PR NUMBER: ${{ github.event.pull_request.number }} + PRIOR_REVIEW_COMMENT_ID: ${{ steps.prior_review.outputs.id }} The PR branch is already checked out in the current working directory. @@ -265,15 +297,43 @@ jobs: fresh on each run; GitHub auto-marks superseded ones "outdated". - **Top-level PR comment** — a concise summary that - anchors the inline findings. Cap at ~5 lines. Edit - in place across runs so the PR never accumulates stacked - review comments: - `gh pr comment --edit-last --create-if-none --body ""`. - Use `--body-file` if the body is large enough to hit - shell argument limits. Don't wrap prior reviews in - `
` here — the related ai-review-log issue + anchors the inline findings. Cap at ~5 lines. **There + is at most one review comment per PR; edit the same + comment in place across runs.** Don't wrap prior reviews + in `
` here — the related ai-review-log issue keeps history across runs. + **Posting mechanism (read carefully).** `--edit-last` + is NOT safe — it would clobber a `@claude` mention + response that happens to be the most recent + `claude[bot]` comment. Instead, use the marker-based + flow: + + 1. **Every review comment body MUST begin with the + literal first line ``, + followed by a newline, followed by your review + content.** This sentinel is what distinguishes the + review comment from mention responses (which lack + it). Future runs find and edit this comment via the + marker. + + 2. **If `PRIOR_REVIEW_COMMENT_ID` (set above) is + non-empty**, edit that exact comment by integer + database ID: + ``` + gh api -X PATCH "repos/${{ github.repository }}/issues/comments/$PRIOR_REVIEW_COMMENT_ID" -f body="$BODY" + ``` + (where `$BODY` starts with the marker line). + + 3. **If empty**, post fresh: + ``` + gh pr comment ${{ github.event.pull_request.number }} --body "$BODY" + ``` + (where `$BODY` starts with the marker line). + + Use `--body-file` (or `gh api … -F body=@`) for + large bodies that hit shell argument limits. + **For "no blockers" runs, the PR comment is one sentence.** This overrides the universal scope's `summary is welcome during calibration` allowance — Harper @@ -373,32 +433,40 @@ jobs: fi # When this workflow job started. Used to filter out stale Claude - # comments from previous runs so a cancelled in-flight run (e.g. - # from a force-push) doesn't re-log the prior run's comment as a - # fresh finding. + # review comments from previous runs so a cancelled in-flight + # run (e.g. from a force-push) doesn't re-log a prior run's + # content as a fresh finding. JOB_STARTED=$(gh api "repos/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}" --jq '.run_started_at // empty') - # Fetch Claude's latest comment and its createdAt timestamp. - CLAUDE_JSON=$(gh pr view "$PR_NUMBER" --json comments \ - --jq '[.comments[] | select(.author.login == "claude")] | last // empty') + # Fetch the marker'd review comment via raw API. We can't use + # `gh pr view --json comments` because (a) it doesn't expose + # `updated_at` (which we need below for the staleness guard + # now that comments are edited in place), and (b) we need the + # marker filter to ignore `@claude` mention responses that + # share the `claude[bot]` identity. + CLAUDE_JSON=$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ + --jq '[.[] | select(.user.login == "claude[bot]") | select(.body | startswith(""))] | last // empty') if [ -z "$CLAUDE_JSON" ] || [ "$CLAUDE_JSON" = "null" ]; then - echo "No Claude comment found on PR #$PR_NUMBER (review_status=$REVIEW_STATUS); skipping log." + echo "No marker'd Claude review comment found on PR #$PR_NUMBER (review_status=$REVIEW_STATUS); skipping log." exit 0 fi CLAUDE_BODY=$(printf '%s' "$CLAUDE_JSON" | jq -r '.body // empty') - CLAUDE_AT=$(printf '%s' "$CLAUDE_JSON" | jq -r '.createdAt // empty') + # Prefer updated_at (reflects the most recent edit) over + # created_at (frozen at original post time) — comments are + # now edited in place across runs. + CLAUDE_AT=$(printf '%s' "$CLAUDE_JSON" | jq -r '.updated_at // .created_at // empty') if [ -z "$CLAUDE_BODY" ]; then - echo "Claude comment had empty body; skipping log." + echo "Claude review comment had empty body; skipping log." exit 0 fi # ISO-8601 lexicographic compare — both are UTC timestamps in the # same shape, so string comparison is sound. if [ -n "$JOB_STARTED" ] && [ -n "$CLAUDE_AT" ] && [ "$CLAUDE_AT" \< "$JOB_STARTED" ]; then - echo "::notice::Latest Claude comment ($CLAUDE_AT) predates this job's start ($JOB_STARTED); skipping to avoid re-logging a stale comment." + echo "::notice::Latest Claude review comment update ($CLAUDE_AT) predates this job's start ($JOB_STARTED); skipping to avoid re-logging stale content." exit 0 fi From ba4f7e051319b4c3a78b348237c2b1bef9efaff5 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Fri, 1 May 2026 11:33:06 -0700 Subject: [PATCH 2/2] =?UTF-8?q?ci(claude):=20tighten=20gh=20api=20allowlis?= =?UTF-8?q?t=20comment=20=E2=80=94=20repo-scoped,=20not=20PR-scoped?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on #444: the original comment said "blast radius from broader API access is the same single PR." That's overstated — `pull-requests: write` is repo-scoped, so an injected `gh api -X PATCH /repos///issues/comments/` could target any top-level PR/issue comment in the repo, not strictly this PR's. `contents: read` likewise grants read of any file in the repo, and `id-token: write` grants OIDC token issuance. Names what actually contains a successful injection: the prompt injection guard (the `Note:` paragraph treating PR-checked-out `CLAUDE.md`/`AGENTS.md` as untrusted for review-discipline overrides), the explicit tool discipline in `## Tools`, the runner's ephemerality, and branch-protection rules on protected refs. Comment-only change. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/claude-review.yml | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 751263d9b..a764e6aaa 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -166,10 +166,17 @@ jobs: # comment by integer database ID (the only way to target a # specific comment instead of "the most recent claude[bot] # comment", which `--edit-last` defaults to and which - # collides with `@claude` mention responses). The token - # only carries `pull-requests: write` / `contents: read` / - # `id-token: write`, so blast radius from broader API - # access is the same single PR. + # collides with `@claude` mention responses). The job's + # token (`pull-requests: write` / `contents: read` / + # `id-token: write`) is repo-scoped, not PR-scoped — a + # successful prompt injection could PATCH any top-level + # PR/issue comment in this repo, read repo contents, or + # fetch an OIDC token. The actual containment is the + # prompt's injection guard (the `Note:` paragraph treating + # PR-checked-out `CLAUDE.md` / `AGENTS.md` as untrusted + # for review-discipline overrides, plus the explicit tool + # discipline in `## Tools`), the runner's ephemerality, + # and branch-protection rules on protected refs. --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr view:*),Bash(gh api:*),Read,Grep,Glob,Write,Bash(git diff:*),Bash(git log:*),Bash(git blame:*),Bash(git show:*)" prompt: | REPO: ${{ github.repository }}