fix: address Copilot review round 3 on the SDK watch
Four findings, including one suppressed. Three fixed outright; the fourth is fixed as far as this PR can reach, with the remainder tracked in #2269 rather than papered over. 1. (Suppressed finding, and the most consequential.) The comment lookup was `gh api --jq '.[].body'` split on newlines, which destroyed the comment boundaries — a body is multi-line, so every LINE became its own entry. Both consumers read a line as though it were a whole comment and both use `startsWith`, so a maintainer quoting `<!-- sdk-watch:analysis -->` at the start of any line would have permanently convinced the sweep that issue was analyzed, and the retry added in round 2 would never fire for it again. The same held for the supersession marker. `issueComments` now fetches `--paginate --slurp` and parses JSON, mapping whole `body` strings. The test fake was part of the problem — it returned newline-joined text, which made the broken parse look correct because it could never produce a multi-line body — so it now returns the real wire shape, an array of pages of comment objects. Note the round-2 test written for this class checked INLINE quoting and passed. The unit was correct; the transport feeding it was not. 2. A CLOSED matching issue was being re-queued for analysis. Two different questions about the same match had been collapsed into one: whether it suppresses creation (yes, whatever its state — that is why `sweepIssues` reads `--state all`) and whether it should be re-analyzed (only if open). A closed issue has no analysis marker almost by definition, so it was handed to the analyze job and would have drawn a fresh automated comment nightly, re-arguing the "not planned" decision that `--state all` exists to respect. Gated on `match.state === "OPEN"`, with the closed case logged rather than passed over in silence. 3. The docs contradicted the code in three places — AGENTS.md, the workflow header and `formatFiledOutput` all still said analysis runs only for newly created issues. All three now describe the same rule, including both exclusions and why they differ: an already-analyzed issue is excluded to avoid a nightly duplicate, a closed one because closing it was a decision. 4. The model retains a secret-exfiltration path: `claude-code-action` copies its environment into the model's, `Read` is genuinely needed to inspect how `core/` uses the SDK, and `analysis` is model-controlled text this workflow publishes. Removing the write-capable `gh` tools closed the posting channel, not the output channel. Landed here as a BACKSTOP, and labelled as one in the workflow so it is not read as settled: the posting step refuses any analysis containing ANTHROPIC_API_KEY or the job's GITHUB_TOKEN verbatim, matched with shell pattern matching so no secret reaches an argv. An injection that encodes the value defeats it. The real fix is workload identity federation (`anthropic_federation_rule_id` + `id-token: write`, and no `anthropic_api_key` at all), which needs an Anthropic-side federation rule and org UUID that do not exist and cannot be created from a pull request. Filed as #2269 with the exact YAML change and acceptance criteria, boarded at Todo/Medium in v2.6.0. AGENTS.md and the workflow both point at it rather than describing this job as secure. Mutation-checked, each failing exactly one test: newline splitting restored -> the multi-line-comment test `state === "OPEN"` guard removed -> the closed-issue test 49 tests in this file; `npm run local:gate` green end to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019JWUZxLDnPgLQzjBrtk1x9 Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
6711593a37
commit
e9a1571f42
2 changed files with 149 additions and 15 deletions
|
|
@ -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 `<!-- sdk-watch:analysis
|
||||
* -->` 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
|
||||
|
|
|
|||
|
|
@ -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"),
|
||||
|
|
|
|||
Loading…
Reference in a new issue