diff --git a/.agents/skills/iterate-pr/SKILL.md b/.agents/skills/iterate-pr/SKILL.md index cfa1a0c2..43f1cd6b 100644 --- a/.agents/skills/iterate-pr/SKILL.md +++ b/.agents/skills/iterate-pr/SKILL.md @@ -69,6 +69,8 @@ Returns JSON with feedback categorized as: Review bot feedback (from Sentry, Warden, Copilot, Cursor, Bugbot, CodeQL, etc.) appears in `high`/`medium`/`low` with `review_bot: true` — it is NOT placed in the `bot` bucket. +That includes Claude posting as `github-actions`, which happens when a workflow hands claude-code-action `github_token`. That login is shared with informational bots, so it is decided by content: inline review threads from it, and top-level comments carrying a claude-code-action signature (`Claude finished @…`, a placeholder, a `[View job](…/actions/runs/N)` link), are review feedback. Other `github-actions` comments stay in `bot`. + **A review bot's comment existing is not a completion signal, and neither is its `created_at` or a green check run.** The Claude review bot posts a comment the instant it is triggered and edits that same comment in place as it works — a run has been observed to report `success` while the body still read "Review in progress" with unchecked boxes. `fetch_pr_feedback.cjs` detects this by checking whether the body still **opens** with the in-progress marker (a review that happens to discuss this behaviour mid-body is not mistaken for an unfinished one) and reports the count as `summary.review_in_progress` instead of bucketing the placeholder as ordinary feedback. **Before treating `needs_attention: 0` as "reviewed, nothing found," check `summary.review_in_progress` — a nonzero count means the review hasn't finished, not that it found nothing.** When one or more are present, `action_required` says so and outranks every other bucket, since a caller must not stop just because the buckets it can already see look clean. **Self-review feedback** (from the PR author) appears in `high`/`medium`/`low` with `self_review: true`. Because you can't "Request changes" on your own PR, a self-review lands as `COMMENTED` review summaries and ordinary review threads rather than changes-requested items — these are surfaced (not dropped). A self-review **thread** (an inline comment) is bucketed by content like any other, defaulting to `medium` absent a `h:/m:/l:` prefix. A self-review **summary** is bucketed structurally, not by content (see below), landing in `review_summary` absent a marker. Treat `self_review` items the same as any other human feedback in step 3. diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs index 321b8855..decb040b 100755 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs @@ -33,6 +33,9 @@ * with a `review_bot: true` flag — they are NOT placed in the `bot` bucket. * - Info bots (Codecov, Dependabot, Renovate, etc.) post status reports and are * placed in the `bot` bucket for silent skipping. + * - `github-actions` is decided by content, since claude-code-action posts as + * it when given `github_token`: its inline threads, and its top-level + * comments carrying a claude-code-action signature, are review feedback. * * Self-review feedback: * - You can't formally "Request changes" on your own PR, so a PR author's own @@ -150,6 +153,45 @@ const isReviewBot = (username) => const isInfoBot = (username) => INFO_BOT_PATTERNS.some((pattern) => pattern.test(username ?? "")); +/** + * `github-actions` is a SHARED identity, so its login alone cannot decide a + * bucket. claude-code-action posts as it whenever a workflow passes + * `github_token: ${{ github.token }}` (this repo's review and focus workflows + * do), and the login is spelled `github-actions[bot]` over REST and + * `github-actions` over GraphQL. Filing those as info-bot noise is how + * taskless/marketing#73 reported `needs_attention: 0` over four open findings. + */ +const SHARED_ACTIONS_BOT = /^github-actions(\[bot\])?$/i; + +/** + * Openings and links that only claude-code-action's comments carry. The + * in-progress placeholders count too: a placeholder posted as `github-actions` + * must still hold the wait loop open. + */ +const ACTION_SIGNATURES = [ + /^[*_]*\s*claude finished @/i, + /\[View job(?: run)?\]\(https:\/\/[^)\s]+\/actions\/runs\/\d+/i, +]; + +/** + * Whether a comment is a review bot's, deciding `github-actions` by content. + * + * An INLINE thread from `github-actions` is review feedback by default: + * informational bots post top-level status comments, not line-anchored review + * threads. A top-level `github-actions` comment counts only when it carries a + * claude-code-action signature; every other one stays informational. + */ +const isReviewComment = (author, body, { inline = false } = {}) => { + if (isReviewBot(author)) return true; + if (!SHARED_ACTIONS_BOT.test(author ?? "")) return false; + if (inline) return true; + const opening = body.trimStart(); + return ( + isReviewInProgress(opening) || + ACTION_SIGNATURES.some((signature) => signature.test(opening)) + ); +}; + /** * The placeholders a review bot posts before it has anything to say. * @@ -328,8 +370,17 @@ const matchesUnnegated = (patterns, body) => const categorizeComment = (comment, body) => { const author = comment?.author?.login || comment?.user?.login || ""; - if (isInfoBot(author) && !isReviewBot(author)) return "bot"; + if (isInfoBot(author) && !isReviewComment(author, body)) return "bot"; + return categorizeContent(comment, body); +}; + +/** + * The content half of `categorizeComment`, for a caller that has already + * decided the author is not an info bot (`bucketByAuthor`), so a review + * comment posted as `github-actions` is not sent back to `bot` by its login. + */ +const categorizeContent = (_comment, body) => { const logaf = detectLogaf(body); if (logaf) return logaf; @@ -382,10 +433,11 @@ const categorizeReviewSummary = (_comment, body) => * an info bot's noise (`bot`), or ordinary human feedback, from ANY of the * three sources. * - * `categorize` defaults to the inline-comment classifier; the review-summary - * call site passes `categorizeReviewSummary` instead, so the bot/info-bot - * split and the in-progress check stay one rule in one place while what - * happens to the *content* differs by source. + * `categorize` defaults to the inline-comment content classifier; the + * review-summary call site passes `categorizeReviewSummary` instead, so the + * bot/info-bot split and the in-progress check stay one rule in one place while + * what happens to the *content* differs by source. `inline` marks a review + * thread, which decides a `github-actions` author (see `isReviewComment`). */ const bucketByAuthor = ( feedback, @@ -393,17 +445,18 @@ const bucketByAuthor = ( comment, body, author, - categorize = categorizeComment + { categorize = categorizeContent, inline = false } = {} ) => { + const reviewer = isReviewComment(author, body, { inline }); // The author gate is load-bearing, not belt and braces. A human writing // "Review in progress on my end, back by EOD" would otherwise be filed as an // unfinished review, vanish from `needs_attention`, and hang the wait loop // forever: a person's comment never gets edited into a finished form the way // the bot's placeholder does, so the count never drops. Only a review bot has // the lifecycle this bucket describes. - if (isReviewBot(author) && isReviewInProgress(body)) { + if (reviewer && isReviewInProgress(body)) { feedback.review_in_progress.push(item); - } else if (isReviewBot(author)) { + } else if (reviewer) { item.review_bot = true; feedback[categorize(comment, body)].push(item); } else if (isInfoBot(author)) { @@ -628,14 +681,9 @@ const buildFeedback = (client, { owner, repo, prInfo }) => { ) { feedback.high.push(item); } else { - bucketByAuthor( - feedback, - item, - review, - body, - author, - categorizeReviewSummary - ); + bucketByAuthor(feedback, item, review, body, author, { + categorize: categorizeReviewSummary, + }); } } @@ -671,7 +719,7 @@ const buildFeedback = (client, { owner, repo, prInfo }) => { if (isResolved) { feedback.resolved.push(item); } else { - bucketByAuthor(feedback, item, first, body, author); + bucketByAuthor(feedback, item, first, body, author, { inline: true }); } } @@ -822,6 +870,7 @@ module.exports = { extractFeedbackItem, isInfoBot, isReviewBot, + isReviewComment, isReviewInProgress, main, }; diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs index 2859ec3f..342427a1 100644 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs @@ -21,6 +21,7 @@ const { extractFeedbackItem, isInfoBot, isReviewBot, + isReviewComment, isReviewInProgress, main, } = require("./fetch_pr_feedback.cjs"); @@ -1072,3 +1073,106 @@ test("an explicit marker in a summary still reaches its priority bucket", () => assert.equal(output.summary.high, 1); assert.equal(output.summary.review_summaries, 0); }); + +// claude-code-action posts as `github-actions` when a workflow hands it +// `github_token`: `github-actions[bot]` over REST, `github-actions` over +// GraphQL. Measured on taskless/marketing#73, where four unresolved findings +// were filed as info-bot noise and the script reported needs_attention: 0. +test("an inline thread from github-actions is review feedback, in both spellings", () => { + for (const login of ["github-actions", "github-actions[bot]"]) { + const output = build( + fakeClient({ + threads: [ + thread({ + comments: { + nodes: [ + { body: "This will break on empty input", author: { login } }, + ], + }, + }), + ], + }), + {} + ); + assert.equal(output.summary.bot_comments, 0, login); + assert.equal(output.summary.high, 1, login); + assert.equal(output.summary.review_bot_feedback, 1, login); + assert.equal(output.feedback.high[0].review_bot, true, login); + } +}); + +test("a finished claude-code-action summary from github-actions is review feedback", () => { + for (const login of ["github-actions", "github-actions[bot]"]) { + const output = build( + fakeClient({ + comments: [ + { + id: 1, + body: `${LIVE_FINISHED}\n\nThis will break on empty input.`, + user: { login }, + }, + ], + }), + {} + ); + assert.equal(output.summary.bot_comments, 0, login); + assert.equal(output.summary.high, 1, login); + } +}); + +// The wait loop must not exit while a review posted as github-actions is still +// running: that is the silent-success shape `review_in_progress` exists for. +test("a placeholder posted as github-actions is an unfinished review", () => { + for (const body of [LIVE_CREATED, LIVE_IN_PROGRESS]) { + const output = build( + fakeClient({ + comments: [{ id: 1, body, user: { login: "github-actions[bot]" } }], + }), + {} + ); + assert.equal(output.summary.review_in_progress, 1); + assert.equal(output.summary.bot_comments, 0); + assert.match(output.action_required, /still in progress/); + } +}); + +// The login is shared, so every OTHER top-level github-actions comment stays +// informational: a deploy preview or a coverage table is not a finding. +test("an ordinary top-level github-actions comment stays in bot", () => { + const output = build( + fakeClient({ + comments: [ + { + id: 1, + body: "Preview deployed. This will break nothing, see the logs.", + user: { login: "github-actions[bot]" }, + }, + ], + }), + {} + ); + assert.equal(output.summary.bot_comments, 1); + assert.equal(output.summary.needs_attention, 0); +}); + +test("isReviewComment decides github-actions by signature, and only github-actions", () => { + assert.ok(isReviewComment("github-actions", LIVE_FINISHED)); + assert.ok( + isReviewComment( + "github-actions[bot]", + "Done.\n\n[View job](https://github.com/o/r/actions/runs/123)" + ) + ); + assert.ok(!isReviewComment("github-actions[bot]", "Coverage dropped 2%")); + assert.ok(isReviewComment("github-actions", "anything", { inline: true })); + // Another info bot carrying the same signature is still not a reviewer. + assert.ok(!isReviewComment("codecov[bot]", LIVE_FINISHED)); + assert.ok(!isReviewComment("codecov[bot]", "anything", { inline: true })); + assert.equal( + categorizeComment( + { user: { login: "github-actions[bot]" } }, + `${LIVE_FINISHED}\n\nThis will break at runtime` + ), + "high" + ); +}); diff --git a/.github/workflows/claude-code-focus-on-demand.yml b/.github/workflows/claude-code-focus-on-demand.yml index 44157466..58aca616 100644 --- a/.github/workflows/claude-code-focus-on-demand.yml +++ b/.github/workflows/claude-code-focus-on-demand.yml @@ -129,6 +129,15 @@ jobs: echo "threads: $(jq '.data.repository.pullRequest.reviewThreads.nodes | length' "${GITHUB_WORKSPACE}/.prior-review.json")" + # The action's tag mode fetches the PR branch before it authenticates + # origin, which fails on a private repo. See the review workflow's step + # of the same name (issue #512) for why, and why not + # `persist-credentials: true`. + - name: Authenticate origin for the action's PR-branch fetch + env: + GH_TOKEN: ${{ github.token }} + run: git remote set-url origin "https://x-access-token:${GH_TOKEN}@${GITHUB_SERVER_URL#https://}/${GITHUB_REPOSITORY}.git" + - name: Run Claude Code Focus id: focus uses: anthropics/claude-code-action@0a8d3c9443bbff909ab973b6a17a340b913f229f # v1.0.221 diff --git a/.github/workflows/claude-code-review-on-demand.yml b/.github/workflows/claude-code-review-on-demand.yml index afe6a615..21ce08f6 100644 --- a/.github/workflows/claude-code-review-on-demand.yml +++ b/.github/workflows/claude-code-review-on-demand.yml @@ -64,7 +64,9 @@ name: Claude Code Review (on demand) # - that is NOT sufficient on its own. The action rewrites the origin remote # URL to embed ITS token (`https://x-access-token:@github.com/...`, # logged as "Updated remote URL with authentication token") after checkout, -# whatever the checkout did. So `--disallowedTools` denies `Read(.git/**)`; +# whatever the checkout did, and a step just before the action writes +# the same URL early so private repos work (issue #512). So +# `--disallowedTools` denies `Read(.git/**)`; # - the token also sits in the Claude process's environment, which is how # `gh pr comment` authenticates, so `Read(//proc/**)` is denied as well. # Do not remove any of these without removing `Read`: they are safe together @@ -231,6 +233,27 @@ jobs: # that had nothing to classify. echo "threads: $(jq '.data.repository.pullRequest.reviewThreads.nodes | length' "${GITHUB_WORKSPACE}/.prior-review.json")" + # The action fetches the PR branch BEFORE it authenticates origin. + # `track_progress: true` selects tag mode, whose `setupBranch` runs + # `git fetch origin ` (or `pull/N/head` for a fork), and only + # then does `configureGitAuth` write the token into the origin URL. With + # `persist-credentials: false` that fetch is anonymous: fine on this + # public repo, `could not read Username` on a private one, which is how a + # vendored copy in taskless/marketing died (issue #512, checked against + # v1.0.221). This writes the value the action writes moments later, from + # the same token as `github_token`, so it exposes nothing new and + # `Read(.git/**)` already covers it. + # + # Do NOT fix this with `persist-credentials: true`. Checkout v7 keeps its + # credential in a `RUNNER_TEMP` file loaded through `includeIf.gitdir:*`; + # the action's `replaceCheckoutCredentials` only strips `include.path` + # entries, so that credential would survive, outside `.git/**`, where the + # `Read` deny rule does not reach. + - name: Authenticate origin for the action's PR-branch fetch + env: + GH_TOKEN: ${{ github.token }} + run: git remote set-url origin "https://x-access-token:${GH_TOKEN}@${GITHUB_SERVER_URL#https://}/${GITHUB_REPOSITORY}.git" + # The 👀 on the triggering comment is not from this workflow or the # action. The Claude GitHub App adds it, as `claude[bot]`, to ANY comment # mentioning `@claude`, before this workflow has started (measured on PR