ci(claude): marker-based review-comment edit-in-place; fixes mention-collision#444
Merged
ci(claude): marker-based review-comment edit-in-place; fixes mention-collision#444
Conversation
…collision 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 `<!-- claude-review:v1 -->`. 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/<id>` - otherwise posts fresh via `gh pr comment <N>` 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) <noreply@anthropic.com>
3 tasks
…oped 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/<owner>/<repo>/issues/comments/<id>` 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) <noreply@anthropic.com>
heskew
added a commit
that referenced
this pull request
May 1, 2026
The inline `run: |` blocks in claude-review.yml and claude-mention.yml have grown past the point of being reviewable inside YAML — the log step alone was 142 lines. Pulls them out to standalone bash scripts: - .github/scripts/compose-review-scope.sh (was 36-line inline) - .github/scripts/find-prior-review-comment.sh (was 10-line inline) - .github/scripts/log-review-to-ai-review-log.sh (was 142-line inline) - .github/scripts/parse-claude-mention.sh (was 17-line inline) Each script documents its inputs, outputs, and the rationale for non-obvious mechanics inline. Workflows invoke via `bash .github/scripts/<name>.sh` rather than direct path execution — sidesteps the +x bit being dropped on Windows checkouts and the "forgot to chmod the new script" footgun. The `#!/usr/bin/env bash` shebangs are now informational only. Stacked on top of #444 (marker-based review-comment edit), which introduces the "Find prior review comment" step and the marker'd log-step lookup. Will rebase to main once #444 merges. Tests for these scripts are deliberately out of scope — Nathan's preference per discussion. A separate PR will add coverage via `npm run test:workflows` (or similar) once we settle on the test runner shape. #417 (auth-gate work) carries its own inline scripts and is not touched here. When that branch updates, its scripts adopt the same .github/scripts/ structure as part of that PR's review fixup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
pushed a commit
that referenced
this pull request
May 5, 2026
…oped 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/<owner>/<repo>/issues/comments/<id>` 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) <noreply@anthropic.com>
kriszyp
pushed a commit
that referenced
this pull request
May 5, 2026
The inline `run: |` blocks in claude-review.yml and claude-mention.yml have grown past the point of being reviewable inside YAML — the log step alone was 142 lines. Pulls them out to standalone bash scripts: - .github/scripts/compose-review-scope.sh (was 36-line inline) - .github/scripts/find-prior-review-comment.sh (was 10-line inline) - .github/scripts/log-review-to-ai-review-log.sh (was 142-line inline) - .github/scripts/parse-claude-mention.sh (was 17-line inline) Each script documents its inputs, outputs, and the rationale for non-obvious mechanics inline. Workflows invoke via `bash .github/scripts/<name>.sh` rather than direct path execution — sidesteps the +x bit being dropped on Windows checkouts and the "forgot to chmod the new script" footgun. The `#!/usr/bin/env bash` shebangs are now informational only. Stacked on top of #444 (marker-based review-comment edit), which introduces the "Find prior review comment" step and the marker'd log-step lookup. Will rebase to main once #444 merges. Tests for these scripts are deliberately out of scope — Nathan's preference per discussion. A separate PR will add coverage via `npm run test:workflows` (or similar) once we settle on the test runner shape. #417 (auth-gate work) carries its own inline scripts and is not touched here. When that branch updates, its scripts adopt the same .github/scripts/ structure as part of that PR's review fixup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
pushed a commit
that referenced
this pull request
May 5, 2026
…heck Symptom: harper PR #411 from a real org member was silently skipped — `Claude PR Review` evaluated its job-level `if:` to false and ran zero steps. Cause: GitHub's webhook `author_association` is unreliable. It reports `CONTRIBUTOR` (or `NONE`) for org members with private membership AND for users whose repo access comes via team membership rather than direct collaborator status. Real HarperFast team members fall into both buckets. Forcing visibility changes is hostile UX, and the collaborators API would admit a broader population (read-only collaborators, default-org-permission users) than we want. Fix: two-job pattern with team-membership check. Each workflow has an `authorize` job that runs first, mints an installation token from a HarperFast-org-owned GitHub App (Members:Read scope), and checks team membership. The work/review job has ONE `if: needs.authorize.outputs.authorized == 'true'`. No step-level guards, no individual user list to maintain. The App token lives in the authorize job ONLY — the work job uses the default GITHUB_TOKEN, so the org-read capability never reaches the agent step. CODEOWNERS-driven trust set: - The auth check reads `.github/CODEOWNERS` via the default token and extracts every `@HarperFast/<team>` handle as the trust set. - Same set as people we trust to review code; alignment by construction. New owner team in CODEOWNERS automatically extends trigger trust. New consumer repo inherits its own CODEOWNERS. - External-org handles are deliberately ignored — only HarperFast teams. - If CODEOWNERS is missing, empty, or has no @HarperFast handles, falls back to @HarperFast/developers. Per-workflow specifics: - claude-review.yml: checks BOTH the PR author (`pull_request.user.login`) AND the event actor (`github.actor`). A non-trusted user pushing to a trusted user's PR branch changes the actor without changing the PR author; refusing those events closes that loophole. claude[bot] is admitted explicitly so AI-authored PRs from the issue-to-PR pipeline get reviewed (ADMIT_CLAUDE_BOT=true). - claude-mention.yml: checks the commenter. claude[bot] not admitted here (only humans trigger mentions). - claude-issue-to-pr.yml: checks the LABELER (github.actor), not the issue author. The labeler must already have at least triage permission; a maintainer labeling an external-author issue is a legitimate way to invoke the agent on community reports. Per the post-#447 convention, the auth-check bash lives in `.github/scripts/authorize-claude-workflow.sh` (shared across all three workflows; parameterized by env vars). Workflows invoke via `bash .github/scripts/...`. Defense-in-depth lint: - New `.github/workflows/auth-gate-invariants.yml` runs on any PR touching a `claude-*.yml` workflow file. Validates structurally via `bash .github/scripts/validate-auth-gate-invariants.sh`: * `authorize` job exists. * `authorize.outputs.authorized` wired to a step output. * `actions/create-github-app-token` present and pinned to a SHA. * `authorize.permissions` has no `write` scopes. * `HARPERFAST_AI_CLIENT_ID` and `HARPERFAST_AI_APP_PRIVATE_KEY` secrets referenced. * Every non-authorize job has `needs: authorize` and an exact `if: needs.authorize.outputs.authorized == 'true'` (no compound expressions, no tautologies). Make this a REQUIRED status check on `main` via branch protection. Subtle attacks on the bash logic are caught by CODEOWNERS review on `.github/`. Required (organization-level) secrets — must be set on the HarperFast org for any consumer repo to authorize a Claude run: - HARPERFAST_AI_CLIENT_ID (the App's Client ID, like Iv23li…) - HARPERFAST_AI_APP_PRIVATE_KEY (.pem file contents) Replaces #417's two earlier commits (which were on a stale base that pre-dated #432, #437, #438, #439, #442, #444, #447). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
PR #432's
--edit-lastmechanism filters by authenticated identity (claude[bot]) only. After a@claudemention, the most recentclaude[bot]comment is the mention response — so the next review run's--edit-lastclobbers it instead of editing the prior review. Observed onHarperFast/oauth#67.Fix is marker-based: every review comment starts with
<!-- claude-review:v1 -->, which mention responses never carry. The marker is what makes "edit only my own prior review comment" possible.What changes
Find prior review commentworkflow step — runs before the Claude review step. Queries the issue-comments REST endpoint, filters forclaude[bot]+ marker-prefixed bodies, exposes the integer database ID (or empty) assteps.prior_review.outputs.id.PRIOR_REVIEW_COMMENT_IDfield is the ID to edit by. If empty, agent posts fresh viagh pr comment. Either way, body starts with the marker.Bash(gh api:*)added to--allowedTools— needed forgh api -X PATCH .../issues/comments/<id>. Token only carriespull-requests: write/contents: read/id-token: write, so the broader API surface is contained to the same single PR.created_attoupdated_atsince edit-in-place leavescreated_atfrozen.Migration
Existing un-marker'd review comments on active PRs (e.g. on the 11 open harper PRs we left in the log) will be orphaned. Next review run on each finds no prior marker'd comment, posts a fresh marker'd one. The old un-marker'd comments stay as historical record. Acceptable cost for the cleaner mechanism going forward.
Test plan
<!-- claude-review:v1 -->as its first line.@claudemention the PR → confirm the review still in place; mention response is a separate comment.Followup
Mirror to oauth in a separate PR — same shape, adapted in the existing worktree.
🤖 Generated with Claude Code