diff --git a/.github/workflows/sdk-watch.yml b/.github/workflows/sdk-watch.yml index 3dcd1da1..03ed617a 100644 --- a/.github/workflows/sdk-watch.yml +++ b/.github/workflows/sdk-watch.yml @@ -15,10 +15,12 @@ # issue per upstream that is behind. It runs every night and is a complete # no-op when nothing moved. # * `analyze` is the "have Opus read the SDK changes" half of #1063. It runs -# ONLY over issues the sweep just created — never over one that already -# existed — which is what keeps it to exactly one analysis per SDK release -# rather than a near-identical comment every night for as long as the issue -# stays open. +# over the issues the sweep just created, plus any OPEN issue it already +# filed that still carries no analysis comment — the retry path for an +# `analyze` job that failed or timed out. What it never does is re-analyze an +# issue that already has one, which is what keeps it to a single analysis per +# SDK release rather than a near-identical comment every night for as long as +# the issue stays open. # # ⚠️ Why this files an issue and not a PR, and why it is NOT the Copilot coding # agent that #1063's comment sketched. Assigning `copilot-swe-agent` is possible @@ -201,6 +203,25 @@ jobs: # What remains is read-only and takes no attacker-chosen file path. # `--disallowedTools` names the write tools anyway, so a later edit # that widens the whitelist cannot silently restore them. + # + # ⚠️ **A RESIDUAL EXFILTRATION CHANNEL REMAINS, AND IT IS NOT CLOSED + # HERE.** `claude-code-action` copies the action's environment into the + # model's, so `ANTHROPIC_API_KEY` and the job's `GITHUB_TOKEN` are + # readable by a `Read` this job genuinely needs — and `analysis` is + # model-controlled text this workflow publishes. An injection can + # therefore still read a credential and encode it in the write-up + # (Copilot, round 3). Two things narrow it and neither is a boundary: + # the posting step refuses any analysis containing a credential + # VERBATIM, and the job's own token is `contents: read` and dies with + # the job. `ANTHROPIC_API_KEY` is the asset that outlives the run. + # + # The real fix is to stop putting a long-lived key in the environment + # at all: this action supports workload identity federation + # (`anthropic_federation_rule_id` + `anthropic_organization_id` with + # `id-token: write`), which exchanges the workflow's OIDC token for a + # short-lived credential. That needs an Anthropic-side federation rule + # and org UUID that do not exist yet, so it cannot land in this change. + # Tracked separately — do not treat the scan below as having settled it. claude_args: | --model claude-opus-5 --max-turns 40 @@ -226,12 +247,31 @@ jobs: ISSUE: ${{ matrix.target.issue }} LABEL: ${{ matrix.target.label }} TO: ${{ matrix.target.to }} + # Read back ONLY to refuse publishing them; see the scan below. + SCAN_ANTHROPIC: ${{ secrets.ANTHROPIC_API_KEY }} + SCAN_GITHUB: ${{ secrets.GITHUB_TOKEN }} run: | set -euo pipefail if [ -z "${ANALYSIS//[[:space:]]/}" ]; then echo "sdk-watch: the analysis came back empty — posting nothing" >&2 exit 1 fi + # ⚠️ A BACKSTOP, NOT A BOUNDARY — read the security note above the + # analysis step before relying on it. `analysis` is model-controlled + # text and the model can still read files, so a verbatim credential in + # it is the one exfiltration shape that is cheap to refuse outright. + # Matched with shell pattern matching rather than `grep` so no secret + # ever reaches an argv that `ps` could show. + for scanned in "$SCAN_ANTHROPIC" "$SCAN_GITHUB"; do + if [ -n "$scanned" ]; then + case "$ANALYSIS" in + *"$scanned"*) + echo "sdk-watch: the analysis contains a credential verbatim — refusing to post" >&2 + exit 1 + ;; + esac + fi + done printf '%s\n\n## Automated review of %s %s\n\n%s\n\n---\n\n%s\n' \ '' \ "$LABEL" "$TO" "$ANALYSIS" \ diff --git a/AGENTS.md b/AGENTS.md index cccc7e12..58566331 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -134,9 +134,9 @@ The board write needs `organization projects: write`, which `GITHUB_TOKEN` canno - **It never boards, like the monthly sweep** — no `PROJECT_TOKEN` exists in this org — so the issue arrives labeled and milestoned and `/issue-triage` places it. - **It never closes an issue either.** A further release files its own issue and leaves a **supersession comment** on the older one; closing is a maintainer act, since the card may already have moved. An issue closed for the same target keeps suppressing it, so a maintainer's "not planned" is not re-argued nightly. -**The analysis half runs Claude, not Copilot, and that is deliberate.** #1063 sketched "a copilot agent running Opus"; neither half of that is reachable from a workflow. Assigning `copilot-swe-agent` produces a **pull request** — the artifact this whole section exists to remove — and its model cannot be selected programmatically at all (`replaceActorsForAssignable` takes no model parameter; absent an admin-configured picker it runs Sonnet). So the `analyze` job uses `anthropics/claude-code-action` with `--model claude-opus-5`, which is told to post **one comment** and is denied every file-writing tool. It runs on `ANTHROPIC_API_KEY`, an **organization** secret already available to this repo, and only over issues the sweep **just created** — never over one that already existed, which is what keeps it to one analysis per release instead of a near-identical comment every night. +**The analysis half runs Claude, not Copilot, and that is deliberate.** #1063 sketched "a copilot agent running Opus"; neither half of that is reachable from a workflow. Assigning `copilot-swe-agent` produces a **pull request** — the artifact this whole section exists to remove — and its model cannot be selected programmatically at all (`replaceActorsForAssignable` takes no model parameter; absent an admin-configured picker it runs Sonnet). So the `analyze` job uses `anthropics/claude-code-action` with `--model claude-opus-5`, which is told to post **one comment** and is denied every file-writing tool. It runs on `ANTHROPIC_API_KEY`, an **organization** secret already available to this repo, over the issues the sweep just created plus any **open** issue it already filed that still carries no analysis comment (the retry path below). What it never re-analyzes is an issue that already has one, or one that has been closed — which is what keeps it to a single analysis per release instead of a near-identical comment every night. -- **"An issue exists" and "the issue was analyzed" are different claims**, and the sweep must not equate them. The posting step stamps `ANALYSIS_MARKER` on its comment; a sweep that finds an existing issue for the current target but no such comment **re-queues it**, so a failed or timed-out `analyze` job is retried rather than silently never revisited. That marker is duplicated in the workflow because the posting step is shell — a test asserts the two strings match, since drift would make every issue read as unanalyzed forever. +- **"An issue exists" and "the issue was analyzed" are different claims**, and the sweep must not equate them. The posting step stamps `ANALYSIS_MARKER` on its comment; a sweep that finds an **open** existing issue for the current target but no such comment **re-queues it**, so a failed or timed-out `analyze` job is retried rather than silently never revisited. ⚠️ **Open only** — suppressing and re-queuing are different questions about the same match, and a closed issue must keep suppressing without being handed a fresh comment every night. That marker is duplicated in the workflow because the posting step is shell — a test asserts the two strings match, since drift would make every issue read as unanalyzed forever. ⚠️ **Upstream release notes are untrusted input to that job, and the model is granted nothing that can write anywhere.** The tool whitelist is the only real control — the prompt's prose constraints are not — and two revisions of it were wrong before this one landed: @@ -145,6 +145,8 @@ The board write needs `organization projects: write`, which `GITHUB_TOKEN` canno So the model returns its write-up as structured output (`--json-schema`) and a deterministic, model-free step posts it — that step is the only thing in the job holding a token that can write, and the body reaches it through the environment rather than the command line. `WebFetch` is denied for the same reason: an outbound fetch the model controls is the other end of the same exfiltration channel, and `gh release view` covers the need. The job's token carries `contents: read` only. **Keep every one of those properties when editing this job.** +⚠️ **One channel is still open, and the workflow says so rather than implying otherwise.** `claude-code-action` copies the action's environment into the model's, so `ANTHROPIC_API_KEY` is readable by a `Read` the analysis genuinely needs, and `analysis` is model-controlled text this workflow publishes. Two things narrow it and **neither is a boundary**: the posting step refuses any analysis containing a credential *verbatim*, and the job's own token expires with the job. The real fix is to stop putting a long-lived key in the environment — the action supports **workload identity federation** (`anthropic_federation_rule_id` + `id-token: write`), which needs an Anthropic-side federation rule this org does not have yet. Tracked in #2269. Do not read the verbatim scan as having settled it. + ## Contributing External contributions are accepted as **issues, not pull requests** — maintainers handle design and implementation through a prompt-driven workflow. diff --git a/scripts/sdk-watch.mjs b/scripts/sdk-watch.mjs index d41e30d4..26cc0d28 100644 --- a/scripts/sdk-watch.mjs +++ b/scripts/sdk-watch.mjs @@ -366,10 +366,15 @@ export function pickMilestone(milestones) { /** * The `$GITHUB_OUTPUT` line naming what was filed this run. * - * Only NEWLY CREATED issues appear here. That is what keeps the analysis job - * downstream to exactly one run per SDK version: an issue that already existed - * has already been analyzed, and re-running Opus against it nightly would add a - * near-identical comment every single night. + * What appears here is every issue this run created, plus any OPEN issue it had + * already filed that still carries no analysis comment. The second half is the + * retry path for an `analyze` job that failed or timed out. + * + * What is deliberately absent is an issue that already HAS its analysis — that + * is what keeps the job downstream to a single run per SDK version rather than a + * near-identical comment every night for as long as the issue stays open. A + * CLOSED issue is absent too, for the stronger reason that closing it was a + * decision and re-analyzing it nightly would re-argue that decision. * * @param {Array<{issue: number, label: string, repo: string, from: string, to: string}>} filed * @returns {string} @@ -450,20 +455,37 @@ function currentMilestone(repo, spawn) { return pickMilestone(JSON.parse(result.stdout || "[]")); } +/** + * Every comment body on an issue, as WHOLE strings. + * + * ⚠️ This used to be `--jq '.[].body'` split on newlines, which destroyed the + * comment boundaries: a body is multi-line, so every LINE became its own array + * element. Both callers then read a line as if it were a comment, and both + * checks are `startsWith` — so a maintainer who quoted `` at the start of any line of any comment would have permanently convinced + * the sweep that issue was analyzed, and the retry would never fire again + * (Copilot). The same held for the supersession marker. + * + * `--slurp` returns one array per page, hence the `flat()`. It cannot be + * combined with `--jq` — `gh` rejects the pair outright — which is exactly why + * the parsing moved here. + * + * @returns {string[]} one entry per comment + */ function issueComments(repo, number, spawn) { const result = gh(spawn, [ "api", "--paginate", + "--slurp", `repos/${repo}/issues/${number}/comments`, - "--jq", - ".[].body", ]); if (result.status !== 0) { throw new Error( `comment lookup for #${number} failed: ${(result.stderr ?? "").trim()}`, ); } - return (result.stdout ?? "").split("\n").filter(Boolean); + const pages = JSON.parse(result.stdout || "[]"); + return pages.flat().map((c) => c?.body ?? ""); } function comment(repo, number, body, spawn) { @@ -607,7 +629,15 @@ export function main( // can fail or time out, and equating the two left the promised analysis // silently never retried. A newly created issue has no comments, so // this only costs a lookup on the adopted path. - if (match) { + // + // ⚠️ OPEN only. `sweepIssues` deliberately reads `--state all`, because a + // CLOSED issue must keep suppressing its target — that is how a + // maintainer's "not planned" survives instead of being re-argued nightly. + // Re-queuing on state alone would have undone exactly that: the closed + // issue has no analysis marker, so it would be handed to the analyze job + // and receive a fresh automated comment every night (Copilot). Suppress + // and re-queue are different questions about the same match. + if (match && match.state === "OPEN") { if (hasAnalysis(issueComments(repo, number, spawn))) { console.log( `sdk-watch: ${state.group.label} ${state.target} already has an issue and an analysis — no-op`, @@ -618,6 +648,10 @@ export function main( `sdk-watch: #${number} has no analysis comment — re-queuing it for the analyze job`, ); } + } else if (match) { + console.log( + `sdk-watch: ${state.group.label} ${state.target} was filed as #${number} and closed — leaving it alone`, + ); } // Any OPEN issue of this group on an older target is now stale. Note it diff --git a/scripts/sdk-watch.test.mjs b/scripts/sdk-watch.test.mjs index 8b011364..5a40c85e 100644 --- a/scripts/sdk-watch.test.mjs +++ b/scripts/sdk-watch.test.mjs @@ -458,7 +458,14 @@ function fakeSpawn({ const path = args.find((a) => String(a).includes("/comments")) ?? ""; const number = Number(/issues\/(\d+)\/comments/.exec(path)?.[1]); const bodies = commentsByIssue[number] ?? comments; - return { status: 0, stdout: bodies.join("\n"), stderr: "" }; + // Shaped as `--paginate --slurp` really answers: an array of PAGES, each + // an array of comment objects. Faking it as newline-joined text was what + // let the boundary-destroying `--jq '.[].body'` split look correct. + return { + status: 0, + stdout: JSON.stringify([bodies.map((body) => ({ body }))]), + stderr: "", + }; } if (args[0] === "api") return { status: 0, stdout: JSON.stringify(milestones), stderr: "" }; @@ -672,6 +679,57 @@ test("main retries a supersession note that failed to post on an earlier run", ( assert.ok(posted.args[posted.args.indexOf("--body") + 1].includes("#500")); }); +test("main keeps a multi-line comment whole rather than splitting it into lines", () => { + // ⚠️ The comment lookup used to be `--jq '.[].body'` split on newlines, so + // every LINE of every comment became its own entry. Both marker checks are + // `startsWith`, so a maintainer quoting the analysis marker at the start of + // any line would have convinced the sweep this issue was analyzed — forever, + // since nothing would ever re-queue it (Copilot). + const spawn = fakeSpawn({ + latest: latestAt(SDK, "2.1.0"), + issues: [existingIssue(400, "2.1.0")], + commentsByIssue: { + 400: [ + `A maintainer writes:\n${ANALYSIS_MARKER}\nis the marker the sweep looks for.`, + ], + }, + }); + const output = outputFile(); + writeFileSync(output, ""); + + main("o/r", spawn, { readFile: fakeReadFile(), output }); + + assert.deepEqual( + readFiled(output).map((f) => f.issue), + [400], + "a quoted marker on a later line must not pass as the sweep's own comment", + ); +}); + +test("main does not re-queue a CLOSED issue that has no analysis", () => { + // Closing the issue was a decision. `sweepIssues` reads `--state all` so that + // decision keeps suppressing the target — but the closed issue naturally has + // no analysis marker, so re-queuing on the marker alone would have handed it + // to the analyze job and posted a fresh comment every night, re-arguing the + // decision the `--state all` read exists to respect (Copilot). + const spawn = fakeSpawn({ + latest: latestAt(SDK, "2.1.0"), + issues: [{ ...existingIssue(400, "2.1.0"), state: "CLOSED" }], + commentsByIssue: { 400: [] }, + }); + const output = outputFile(); + writeFileSync(output, ""); + + main("o/r", spawn, { readFile: fakeReadFile(), output }); + + assert.deepEqual(readFiled(output), []); + assert.equal( + spawn.calls.some((c) => c.args[0] === "issue" && c.args[1] === "create"), + false, + "and it must still suppress creation", + ); +}); + test("main does not treat the target issue as superseding itself", () => { const spawn = fakeSpawn({ latest: latestAt(SDK, "2.1.0"),