diff --git a/.github/workflows/commands.yml b/.github/workflows/commands.yml index d1346a3f..1ff44eee 100644 --- a/.github/workflows/commands.yml +++ b/.github/workflows/commands.yml @@ -99,7 +99,7 @@ jobs: env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - gh workflow run review.yml -R "${{ github.repository }}" -f pr_number="${{ github.event.issue.number }}" + gh workflow run review.yml -R "${{ github.repository }}" -f pr_number="${{ github.event.issue.number }}" -f recheck=true explain: # Runs on the ephemeral x64-review pool (on the tailnet, reaches CLIProxyAPI); same lane as review. diff --git a/.github/workflows/hosted-review.yml b/.github/workflows/hosted-review.yml index ebd6de3e..7c6c7193 100644 --- a/.github/workflows/hosted-review.yml +++ b/.github/workflows/hosted-review.yml @@ -7,6 +7,11 @@ on: pr: description: PR number to review required: false + recheck: + description: Force a full review and reset the automatic recovery limit + type: boolean + required: false + default: false pr_number: description: PR number supplied by reconciliation required: false @@ -20,6 +25,7 @@ jobs: uses: ./.github/workflows/review.yml with: pr_number: ${{ inputs.pr_number || inputs.pr }} + recheck: ${{ inputs.recheck || false }} runner: codex # Self-hosted lane via CLIProxyAPI (frank_infra x64-review pool); eval #172. model: deepseek-v4.1-flash diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 2e5e3ab0..c787b9dd 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -13,6 +13,11 @@ on: pr_number: description: PR number to review (manual trigger; bypasses pull_request webhook) required: true + recheck: + description: Force a full review even when this head was already reviewed + type: boolean + required: false + default: false runner: description: "Model runner: codex, claude, opencode, grok, or pi" required: false @@ -51,6 +56,11 @@ on: description: PR number (optional; defaults to the event PR) type: string required: false + recheck: + description: Force a full review even when this head was already reviewed + type: boolean + required: false + default: false runner: description: "Model runner: codex, claude, opencode, grok, or pi" type: string @@ -177,6 +187,9 @@ jobs: -f head_sha="$head" \ -f status="completed" \ -f conclusion="neutral" \ + -f external_id="$(jq -cn --argjson pr "$PR_NUM" \ + --arg run "$GITHUB_RUN_ID" --arg round "skip-$GITHUB_RUN_ID-$GITHUB_RUN_ATTEMPT" \ + '{v: 2, prNumber: $pr, roundId: $round, runId: $run}')" \ -f "output[title]=Needlefish: skipped (author not trusted)" \ -f "output[summary]=Needlefish runs model CLIs unrestricted on trusted runners, so it reviews only PRs opened by an OWNER, MEMBER, or COLLABORATOR that is not a bot. $skip_reason. A maintainer who has read this PR can review it by running the review workflow manually with allow_untrusted_author: true." >/dev/null fi @@ -327,7 +340,8 @@ jobs: PI_AUTH_MODE: proxy # npm-global bin is not on the runner service PATH. PI_BIN: /home/termtek/.npm-global/bin/pi - NEEDLEFISH_RECHECK_INPUT: ${{ github.event_name == 'workflow_dispatch' && '1' || '' }} + NEEDLEFISH_CHECK_APP_ID: "" + NEEDLEFISH_RECHECK_INPUT: ${{ inputs.recheck && '1' || '' }} NEEDLEFISH_ALLOW_UNTRUSTED_AUTHOR: ${{ inputs.allow_untrusted_author && '1' || '' }} run: | if [ -z "${NEEDLEFISH_BIN:-}" ] || [ ! -x "$NEEDLEFISH_BIN" ]; then @@ -425,7 +439,22 @@ jobs: const checks = JSON.parse(input).check_runs; const previous = process.env.NEEDLEFISH_PREVIOUS_CHECK_ID ?? ""; if (!/^[0-9]+$/.test(previous)) process.exit(1); - const check = checks.filter(c => c.id > Number(previous) && c.name === "Needlefish" && c.external_id === process.env.GITHUB_RUN_ID && c.app?.slug === "github-actions").sort((a, b) => b.id - a.id)[0]; + const appIdInput = process.env.NEEDLEFISH_CHECK_APP_ID || ""; + const appId = appIdInput ? Number(appIdInput) : null; + if (appIdInput && (!/^[1-9][0-9]*$/.test(appIdInput) || !Number.isSafeInteger(appId))) process.exit(1); + const currentRun = externalId => { + if (externalId === process.env.GITHUB_RUN_ID) return true; + try { + const round = JSON.parse(externalId); + return round?.v === 2 && round.runId === process.env.GITHUB_RUN_ID && round.prNumber === Number(process.env.PR_NUM); + } catch { return false; } + }; + const check = checks.filter(c => + c.id > Number(previous) && + c.name === "Needlefish" && + currentRun(c.external_id) && + (appId === null ? c.app?.slug === "github-actions" : c.app?.id === appId) + ).sort((a, b) => b.id - a.id)[0]; const prefix = "Review errored and did NOT pass this PR.\n\n```\n"; const error = check?.output?.summary?.startsWith(prefix) ? check.output.summary.slice(prefix.length).split("\n")[0] : ""; const failed = check?.status === "completed" && check.conclusion === "failure" && check.output.title === "Needlefish: review failed"; @@ -478,6 +507,7 @@ jobs: PR_NUM: ${{ inputs.pr_number || github.event.inputs.pr_number || github.event.pull_request.number }} WORKFLOW_REF: ${{ github.workflow_ref }} RUN_ID: ${{ github.run_id }} + NEEDLEFISH_CHECK_APP_ID: "" run: | set -eu if [ -z "$PR_NUM" ]; then @@ -506,6 +536,9 @@ jobs: -f head_sha="$head" \ -f status="completed" \ -f conclusion="failure" \ + -f external_id="$(jq -cn --argjson pr "$PR_NUM" \ + --arg run "$RUN_ID" --arg round "retry-required-$RUN_ID-$GITHUB_RUN_ATTEMPT" \ + '{v: 2, prNumber: $pr, roundId: $round, runId: $run}')" \ -f "output[title]=Needlefish: caller retry required" \ -f "output[summary]=$summary" >/dev/null } @@ -513,7 +546,24 @@ jobs: # every review check is named "Needlefish", so the retry cap below would # otherwise never see more than one failure and could never trip. runs_url="repos/$REPO/commits/$head/check-runs?check_name=Needlefish&per_page=100&filter=all" - valid=$(gh api "$runs_url" --jq '[.check_runs[] | select(.conclusion == "success" or .conclusion == "neutral" or (.conclusion == "failure" and ((.output.title // "") | test("^Needlefish: review (failed|terminated)$") | not)))] | length') + app_id="${NEEDLEFISH_CHECK_APP_ID:-}" + if [ -n "$app_id" ] && { [[ ! "$app_id" =~ ^[1-9][0-9]*$ ]] || [ "${#app_id}" -gt 16 ] || [ "$app_id" -gt 9007199254740991 ]; }; then + echo "::error::NEEDLEFISH_CHECK_APP_ID must be a positive safe integer" + exit 1 + fi + app_filter='.app.slug == "github-actions"' + if [ -n "$app_id" ]; then app_filter=".app.id == $app_id"; fi + owned_checks=$(gh api "$runs_url" --jq "[ + .check_runs[] + | select(.name == \"Needlefish\" and ($app_filter)) + | ((.external_id // \"\" | fromjson?) // null) as \$round + | select(if (\$round | type) == \"object\" then + (\$round.v == 2 and \$round.prNumber == $PR_NUM + and (\$round.roundId | type) == \"string\" and (\$round.roundId | length) > 0 + and (\$round.runId | type) == \"string\") + else ((.external_id // \"\") | test(\"^[1-9][0-9]*$\")) end) + ]") + valid=$(printf '%s' "$owned_checks" | jq '[.[] | select(.conclusion == "success" or .conclusion == "neutral" or (.conclusion == "failure" and ((.output.title // "") | test("^Needlefish: review (failed|terminated)$") | not)))] | length') if [ "$valid" -gt 0 ]; then echo "head $head already has a terminal Needlefish verdict; nothing to reconcile" exit 0 @@ -546,7 +596,7 @@ jobs: echo "a $workflow_file run for PR #$PR_NUM is still active or queued; letting it finish" exit 0 fi - infra_fails=$(gh api "$runs_url" --jq '[.check_runs[] | select(.conclusion == "failure" and ((.output.title // "") | test("^Needlefish: review (failed|terminated)$")))] | length') + infra_fails=$(printf '%s' "$owned_checks" | jq '[.[] | select(.conclusion == "failure" and ((.output.title // "") | test("^Needlefish: review (failed|terminated)$")))] | length') if [ "$infra_fails" -ge 2 ]; then echo "::error::head $head has $infra_fails infra-failed reviews; retry cap reached, not re-dispatching" exit 0 diff --git a/action.yml b/action.yml index 50654cf7..d347989a 100644 --- a/action.yml +++ b/action.yml @@ -95,6 +95,9 @@ runs: -f head_sha="$head" \ -f status="completed" \ -f conclusion="neutral" \ + -f external_id="$(jq -cn --argjson pr "$pr" \ + --arg run "$GITHUB_RUN_ID" --arg round "skip-$GITHUB_RUN_ID-$GITHUB_RUN_ATTEMPT" \ + '{v: 2, prNumber: $pr, roundId: $round, runId: $run}')" \ -f "output[title]=Needlefish: skipped (author not trusted)" \ -f "output[summary]=Needlefish reviews only PRs opened by an OWNER, MEMBER, or COLLABORATOR that is not a bot. $reason. A maintainer who has read this PR can review it by running the workflow manually with allow_untrusted_author: true." >/dev/null fi diff --git a/docs/reference.md b/docs/reference.md index a9acf284..51c0bd09 100644 --- a/docs/reference.md +++ b/docs/reference.md @@ -280,6 +280,7 @@ jobs: uses: ./.github/workflows/needlefish-review-local.yml with: pr_number: ${{ inputs.pr_number || github.event.pull_request.number }} + recheck: ${{ inputs.recheck || false }} runner: codex model: gpt-5.6-terra codex_reasoning_effort: xhigh @@ -289,6 +290,20 @@ jobs: The caller must retain `workflow_dispatch.inputs.pr_number`, `actions: write`, and a run name ending in ` PR #` for bounded reconciliation. Reconcile runs on GitHub-hosted Ubuntu independently of the review runner. +Automatic dispatches use bounded recovery. To force a full manual review, declare +an optional boolean `workflow_dispatch.inputs.recheck` with default `false` in +the caller and forward it through `with.recheck` as shown above. + +For direct CLI use with a custom GitHub App, provide its installation token as +`GH_TOKEN` and set `NEEDLEFISH_CHECK_APP_ID` to that App's numeric ID. +Use the ID from its GitHub App settings or the `app.id` field of a check created +by that App. The pin validates check ownership; it does not select or mint a token. +The bundled reusable workflow uses `secrets.GITHUB_TOKEN` and does not accept a +custom App token. It clears `NEEDLEFISH_CHECK_APP_ID` for review and reconciliation, +so repository variables and runner environment pins cannot override its GitHub +Actions identity. Custom App pins apply only to direct CLI use, where a mismatched +token and App pin fails closed. +Without a pin, new checks and recovery accept only GitHub Actions checks. Set the repository variable `NEEDLEFISH_RUNS_ON` to an operator-owned runner label when direct PR triggers should use a dedicated fleet; otherwise the workflow keeps the `self-hosted` default. Explicit `runs_on` inputs still win. diff --git a/docs/reference.zh-TW.md b/docs/reference.zh-TW.md index e8eeb2ad..c01952cb 100644 --- a/docs/reference.zh-TW.md +++ b/docs/reference.zh-TW.md @@ -210,6 +210,7 @@ jobs: uses: ./.github/workflows/needlefish-review-local.yml with: pr_number: ${{ inputs.pr_number || github.event.pull_request.number }} + recheck: ${{ inputs.recheck || false }} runner: codex model: gpt-5.6-terra codex_reasoning_effort: xhigh @@ -219,6 +220,19 @@ jobs: Caller 保留 `workflow_dispatch.inputs.pr_number`、`actions: write`，以及以 ` PR #<編號>` 結尾的 run-name，供有次數上限的 reconciliation 使用。Reconcile 維持在 GitHub-hosted Ubuntu 執行，不依賴 review runner 是否正常。 +自動 dispatch 只做有次數上限的 recovery。若要手動強制完整 review，在 caller +宣告選填的 boolean `workflow_dispatch.inputs.recheck`（預設 `false`），並如上例 +經由 `with.recheck` 傳入。 + +直接以 CLI 搭配自訂 GitHub App 時，以該 App 的 installation token 作為 +`GH_TOKEN`，並把 `NEEDLEFISH_CHECK_APP_ID` 設為該 App 的數字 ID。ID 可從 +GitHub App 設定頁，或該 App 建立之 check 的 `app.id` 欄位取得。此 pin 只驗證 +check 的擁有者，不會選擇或產生 token。內建 reusable workflow 使用 +`secrets.GITHUB_TOKEN`，不接受自訂 App token；它在 review 與 reconciliation +時清除 `NEEDLEFISH_CHECK_APP_ID`，因此 repository variable 與 runner 環境的 +pin 都無法覆蓋其 GitHub Actions 身分。自訂 App pin 只適用於直接 CLI 使用； +token 與 App pin 不符時 fail closed。 +未設定 pin 時，新 check 與 recovery 只接受 GitHub Actions 建立的 check。 若 direct PR trigger 要使用專用 fleet，設定 repository variable `NEEDLEFISH_RUNS_ON` 為操作者持有的 runner label；未設定時維持 `self-hosted`。 明確傳入的 `runs_on` 仍優先。 diff --git a/scripts/commands-workflow-auth.test.mjs b/scripts/commands-workflow-auth.test.mjs index d61c3fb4..5ae262df 100644 --- a/scripts/commands-workflow-auth.test.mjs +++ b/scripts/commands-workflow-auth.test.mjs @@ -235,3 +235,17 @@ test("malformed permission payload is denied fail-closed", () => { assert.notEqual(lastOutput(result.githubOutput, "allowed"), "true"); assert.match(result.stderr, /Invalid permission payload/); }); + +test("manual recheck dispatch explicitly requests a full review", () => { + const dispatch = workflowScript("Dispatch recheck") + .replaceAll("${{ github.repository }}", "acme/widgets") + .replaceAll("${{ github.event.issue.number }}", "42"); + const result = spawnSync("bash", ["-c", `gh() { printf '%s\\n' "$@"; }\n${dispatch}`], { + encoding: "utf8", + }); + assert.equal(result.status, 0, result.stderr); + assert.deepEqual(result.stdout.trim().split("\n"), [ + "workflow", "run", "review.yml", "-R", "acme/widgets", + "-f", "pr_number=42", "-f", "recheck=true", + ]); +}); diff --git a/scripts/review-workflow-author-trust.test.mjs b/scripts/review-workflow-author-trust.test.mjs index 1f5223c5..95b6a700 100644 --- a/scripts/review-workflow-author-trust.test.mjs +++ b/scripts/review-workflow-author-trust.test.mjs @@ -100,6 +100,8 @@ esac ...process.env, GH_LOG: ghLog, GH_TOKEN: "test-token", + GITHUB_RUN_ID: "777", + GITHUB_RUN_ATTEMPT: "1", GITHUB_OUTPUT: output, PATH: `${fakeBin}:${process.env.PATH ?? ""}`, REPO, @@ -308,26 +310,34 @@ test("reconcile treats the posted skip check as terminal and never dispatches th undefined, ); - const skipped = runStep(resolveScript, eventEnv({ association: "NONE" })); - assert.equal(skipped.posts.length, 1, skipped.log); - const posted = postedCheck(skipped.posts[0]); - const r = runStep( - reconcileScript, - { - PR_NUM: "42", - WORKFLOW_REF: `${REPO}/.github/workflows/review.yml@refs/heads/main`, - RUN_ID: "777", - }, - { - pr: pull({ association: "NONE" }), - checkRuns: [ - { conclusion: posted.conclusion, output: { title: posted["output[title]"] } }, - ], - }, - ); - assert.equal(r.status, 0, r.stderr); - assert.match(r.stdout, /already has a terminal Needlefish verdict/); - assert.doesNotMatch(r.log, /^workflow run/m); + for (const [producer, env] of [ + [resolveScript, eventEnv({ association: "NONE" })], + [trustStep.run, actionEnv({ association: "NONE" })], + ]) { + const skipped = runStep(producer, env); + assert.equal(skipped.posts.length, 1, skipped.log); + const posted = postedCheck(skipped.posts[0]); + assert.deepEqual(JSON.parse(posted.external_id), { + v: 2, prNumber: 42, roundId: "skip-777-1", runId: "777", + }); + const r = runStep( + reconcileScript, + { + PR_NUM: "42", + WORKFLOW_REF: `${REPO}/.github/workflows/review.yml@refs/heads/main`, + RUN_ID: "777", + }, + { + pr: pull({ association: "NONE" }), + checkRuns: [ + { name: posted.name, app: { id: 15368, slug: "github-actions" }, external_id: posted.external_id, conclusion: posted.conclusion, output: { title: posted["output[title]"] } }, + ], + }, + ); + assert.equal(r.status, 0, r.stderr); + assert.match(r.stdout, /already has a terminal Needlefish verdict/); + assert.doesNotMatch(r.log, /^workflow run/m); + } }); function actionEnv({ association, userType = "User", allow = "false", eventHead = HEAD }) { diff --git a/scripts/review-workflow-fallback.test.mjs b/scripts/review-workflow-fallback.test.mjs index 28ac5a35..8109a27e 100644 --- a/scripts/review-workflow-fallback.test.mjs +++ b/scripts/review-workflow-fallback.test.mjs @@ -27,7 +27,10 @@ function shq(value) { } // primary/fallback: { out, err, code } the stub emits on that lane. -function runReviewScript(t, { primary, fallback, checks = [], checkResponse, checkExit = 0, runner = "codex", tier = "", previousCheckId = 0, snapshotExit = 0 }) { +function runReviewScript(t, { + primary, fallback, checks = [], checkResponse, checkExit = 0, runner = "codex", + tier = "", previousCheckId = 0, snapshotExit = 0, appId = "", +}) { const root = mkdtempSync(join(tmpdir(), "needlefish-workflow-fallback-")); t.after(() => rmSync(root, { recursive: true, force: true })); const write = (name, content) => { @@ -101,6 +104,7 @@ function runReviewScript(t, { primary, fallback, checks = [], checkResponse, che NEEDLEFISH_TIMEOUT_MS_INPUT: "", OPENCODE_IDLE_TIMEOUT_MS_INPUT: "", NEEDLEFISH_RECHECK_INPUT: "", + NEEDLEFISH_CHECK_APP_ID: appId, }, }); return { @@ -307,6 +311,9 @@ test("installed runner auth misclassification uses this run check and Sol medium test("check lookup stays closed for old runs, other apps, nonterminal checks and verdicts", t => { for (const checks of [ [runnerCheck({ external_id: "older-run" })], + [runnerCheck({ external_id: JSON.stringify({ v: 2, prNumber: 7, runId: "older-run", roundId: "round", recoveryAttempt: 0 }) })], + [runnerCheck({ external_id: JSON.stringify({ v: 1, runId: "123" }) })], + [runnerCheck({ external_id: JSON.stringify({ v: 2, prNumber: 8, runId: "123", roundId: "other", recoveryAttempt: 0 }) })], [runnerCheck({ app: { slug: "other-app" } })], [runnerCheck({ status: "in_progress" })], [runnerCheck({ output: { title: "Needlefish: changes requested", summary: "codex runner exited 1; 429" } })], @@ -357,3 +364,25 @@ test("compatibility fallback requires a check created after this invocation snap assert.equal(result.status, expected === 2 ? 0 : 1, result.stderr); } }); + +test("versioned delivery metadata preserves this workflow run fallback", t => { + const result = runReviewScript(t, { + primary: { out: "", err: "", code: 1 }, + fallback: { out: outcome({ outcome: "verdict", verdict: "pass" }), err: "", code: 0 }, + checks: [runnerCheck({ external_id: JSON.stringify({ v: 2, prNumber: 7, runId: "123", roundId: "round", recoveryAttempt: 0 }) })], + }); + assert.equal(result.status, 0, result.stderr); + assert.equal(result.invocations.length, 2); + assert.match(result.invocations[1], /--model gpt-6\.1-sol/); +}); + +test("compatibility fallback honors the configured check App ID", t => { + const primary = { out: "", err: "", code: 1 }; + const fallback = { out: "PASS", err: "", code: 0 }; + const custom = runnerCheck({ app: { id: 4812, slug: "private-review-app" } }); + for (const [appId, expected] of [["4812", 2], ["9999", 1], ["", 1], ["invalid", 1]]) { + const result = runReviewScript(t, { primary, fallback, checks: [custom], appId }); + assert.equal(result.invocations.length, expected, `App ID ${appId}: ${result.stderr}`); + assert.equal(result.status, expected === 2 ? 0 : 1, result.stderr); + } +}); diff --git a/scripts/review-workflow-pr-number.test.mjs b/scripts/review-workflow-pr-number.test.mjs index aa0e2967..e22a0a4b 100644 --- a/scripts/review-workflow-pr-number.test.mjs +++ b/scripts/review-workflow-pr-number.test.mjs @@ -13,6 +13,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import test from "node:test"; import { workflowRun } from "./workflow-test-helpers.mjs"; +import { parse } from "yaml"; // GitHub Actions interpolates ${{ }} into the run script before bash parses it. // Double quotes do not stop command substitution: $(...) and backticks in a @@ -28,11 +29,12 @@ const step = workflow.match( assert.ok(step, "Needlefish review step must exist"); const script = workflowRun(workflow, "review", "Needlefish review"); -function runReview(prNum, { runner = "", homeCodex = false, homeCodexVersion = "0.155.0", configuredCodex = false, codexBinValue } = {}) { +function runReview(prNum, { runner = "", homeCodex = false, homeCodexVersion = "0.155.0", configuredCodex = false, codexBinValue, recheck = "", appId = "" } = {}) { const root = mkdtempSync(join(tmpdir(), "needlefish-workflow-pr-")); const fakeBin = join(root, "fake bin"); const argvLog = join(root, "argv.log"); const codexBinLog = join(root, "codex-bin.log"); + const appIdLog = join(root, "app-id.log"); const needlefishBin = join(fakeBin, "needlefish"); const pathCodex = join(fakeBin, "codex"); mkdirSync(fakeBin); @@ -55,6 +57,7 @@ function runReview(prNum, { runner = "", homeCodex = false, homeCodexVersion = " set -euo pipefail for arg in "$@"; do printf '<%s>\\n' "$arg" >> "$ARGV_LOG"; done printf '%s' "\${CODEX_BIN:-}" > "$CODEX_BIN_LOG" +printf '%s' "\${NEEDLEFISH_CHECK_APP_ID:-}" > "$APP_ID_LOG" `, ); chmodSync(needlefishBin, 0o755); @@ -65,13 +68,15 @@ printf '%s' "\${CODEX_BIN:-}" > "$CODEX_BIN_LOG" env: { ...process.env, ARGV_LOG: argvLog, + APP_ID_LOG: appIdLog, + NEEDLEFISH_CHECK_APP_ID: parse(workflow).jobs.review.steps.find(s => s.name === "Needlefish review").env.NEEDLEFISH_CHECK_APP_ID === "" ? "" : appId, CODEX_BIN: codexBinValue === undefined ? (configuredCodex ? expectedConfiguredCodex : "") : codexBinValue(expectedConfiguredCodex), CODEX_BIN_LOG: codexBinLog, CODEX_REASONING_EFFORT: "", HOME: root, NEEDLEFISH_BIN: needlefishBin, NEEDLEFISH_MODEL_INPUT: "", - NEEDLEFISH_RECHECK_INPUT: "", + NEEDLEFISH_RECHECK_INPUT: recheck, NEEDLEFISH_RUNNER_INPUT: runner, NEEDLEFISH_TIMEOUT_MS_INPUT: "", OPENCODE_IDLE_TIMEOUT_MS_INPUT: "", @@ -81,6 +86,7 @@ printf '%s' "\${CODEX_BIN:-}" > "$CODEX_BIN_LOG" }); const output = { ...result, + appId: existsSync(appIdLog) ? readFileSync(appIdLog, "utf8") : null, argvLog: existsSync(argvLog) ? readFileSync(argvLog, "utf8") : "", codexBin: existsSync(codexBinLog) ? readFileSync(codexBinLog, "utf8") : "", expectedHomeCodex, @@ -250,3 +256,37 @@ test("review rejects a quote-breaking PR number before invoking needlefish", () assert.equal(result.argvLog, ""); assert.equal(result.pwned, false); }); + + +test("automatic dispatch stays bounded and only explicit recheck adds the flag", () => { + assert.match(step[1], /NEEDLEFISH_RECHECK_INPUT: \$\{\{ inputs\.recheck && '1' \|\| '' \}\}/); + const automatic = runReview("42"); + assert.equal(automatic.status, 0, automatic.stderr); + assert.doesNotMatch(automatic.argvLog, /<--recheck>/); + const manual = runReview("42", { recheck: "1" }); + assert.equal(manual.status, 0, manual.stderr); + assert.match(manual.argvLog, /<--recheck>/); +}); + + +test("both dispatch entry points forward an opt-in manual recheck", () => { + const reusable = parse(workflow); + const hosted = parse(readFileSync(".github/workflows/hosted-review.yml", "utf8")); + for (const trigger of ["workflow_dispatch", "workflow_call"]) { + assert.deepEqual(reusable.on[trigger].inputs.recheck, { + description: "Force a full review even when this head was already reviewed", + type: "boolean", required: false, default: false, + }); + } + assert.equal(hosted.on.workflow_dispatch.inputs.recheck.default, false); + assert.equal(hosted.jobs.review.with.recheck, "${{ inputs.recheck || false }}"); +}); + +for (const appId of ["4812", "15368", "invalid", ""]) { + test(`bundled review clears the repository or host App pin ${JSON.stringify(appId)}`, () => { + const result = runReview("42", { appId }); + assert.equal(result.status, 0, result.stderr); + assert.match(result.argvLog, /<--github>\n<--pr>\n<42>\n/); + assert.equal(result.appId, ""); + }); +} diff --git a/scripts/review-workflow-reconcile.test.mjs b/scripts/review-workflow-reconcile.test.mjs index 3d04cc1d..9a6e3481 100644 --- a/scripts/review-workflow-reconcile.test.mjs +++ b/scripts/review-workflow-reconcile.test.mjs @@ -12,6 +12,7 @@ import { import { tmpdir } from "node:os"; import { join } from "node:path"; import test from "node:test"; +import { parse } from "yaml"; import { workflowRun } from "./workflow-test-helpers.mjs"; // The reconcile job is the only thing that can re-dispatch a review on its @@ -34,7 +35,7 @@ const HEAD = "0123456789abcdef0123456789abcdef01234567"; const SELF_RUN_ID = "777"; function checkRun(title, conclusion = "failure") { - return { conclusion, output: { title } }; + return { name: "Needlefish", app: { id: 15368, slug: "github-actions" }, external_id: JSON.stringify({ v: 2, prNumber: 42, roundId: "fixture-round", runId: SELF_RUN_ID }), conclusion, output: { title } }; } function activeRun(id, title) { @@ -45,6 +46,7 @@ function activeRun(id, title) { // endpoint. Order matters: first match wins. function runReconcile({ prNum = "42", + appId = "", state = "open", headRepo = REPO, checkRunsAll = [], @@ -119,9 +121,11 @@ if [ -n "$jq_filter" ]; then printf '%s' "$body" | jq -r "$jq_filter"; else prin GH_TOKEN: "test-token", PATH: `${fakeBin}:${process.env.PATH ?? ""}`, PR_NUM: prNum, + NEEDLEFISH_CHECK_APP_ID: parse(workflow).jobs.reconcile.steps[0].env.NEEDLEFISH_CHECK_APP_ID === "" ? "" : appId, REPO, RESPONSES: join(root, "responses.json"), RUN_ID: SELF_RUN_ID, + GITHUB_RUN_ATTEMPT: "1", WORKFLOW_REF: workflowRef, WORKFLOW_MISSING: workflowMissing ? "1" : "0", }, @@ -153,6 +157,8 @@ test("reconcile re-dispatches once when the head has a single infra failure", () const r = runReconcile({ checkRunsAll: [INFRA] }); assert.equal(r.status, 0, r.stderr); assert.equal(r.dispatched, 1); + assert.match(r.log, /workflow run review.yml --repo acme\/widgets --ref main -f pr_number=42/); + assert.doesNotMatch(r.log, /recheck=true|recheck=1/); assert.match(r.stdout, /reconciliation dispatched for PR #42/); }); @@ -243,6 +249,27 @@ test("reconcile reports a missing caller workflow before querying its runs", () assert.equal(posts.length, 1); assert.match(posts[0], /check-runs .*output\[title\]=Needlefish: caller retry required/); assert.doesNotMatch(r.log, /actions\/workflows\/review.yml\/runs\?/); + const fields = Object.fromEntries( + [...posts[0].matchAll(/-f (output\[\w+\]|\w+)=(.*?)(?= -f |$)/g)] + .map((match) => [match[1], match[2]]), + ); + assert.deepEqual(JSON.parse(fields.external_id), { + v: 2, prNumber: 42, roundId: "retry-required-777-1", runId: "777", + }); + const repeated = runReconcile({ + workflowMissing: true, + checkRunsAll: [{ + name: fields.name, + app: { id: 15368, slug: "github-actions" }, + external_id: fields.external_id, + conclusion: fields.conclusion, + output: { title: fields["output[title]"] }, + }], + }); + assert.equal(repeated.status, 0, repeated.stderr); + assert.equal(repeated.dispatched, 0); + assert.match(repeated.stdout, /already has a terminal Needlefish verdict/); + assert.doesNotMatch(repeated.log, /api -X POST/); }); test("reconcile rejects a PR number containing regex syntax before any API call", () => { @@ -290,3 +317,51 @@ test("reconcile includes cancelled runs in the completed-run cap", () => { assert.equal(r.dispatched, 0); assert.match(r.stdout, /completed failed or cancelled runs; retry cap reached/); }); + + +for (const mode of ["foreign-app", "other-pr", "custom-pinned", "custom-mismatch", "legacy-actions"]) { + test(`reconcile terminal identity handles ${mode}`, () => { + const terminal = checkRun("Needlefish: pass", "success"); + if (mode.startsWith("custom") || mode === "foreign-app") terminal.app = { id: 4812, slug: "private-review-app" }; + if (mode === "other-pr") terminal.external_id = JSON.stringify({ v: 2, prNumber: 99 }); + if (mode === "legacy-actions") terminal.external_id = "123456"; + const r = runReconcile({ checkRunsAll: [terminal], appId: mode === "custom-pinned" ? "4812" : mode === "custom-mismatch" ? "9999" : "" }); + assert.equal(r.status, 0, r.stderr); + assert.equal(r.dispatched, mode === "legacy-actions" ? 0 : 1, r.stdout); + }); +} + +test("bundled reconcile sees the Actions verdict despite a repository or host custom App pin", () => { + const r = runReconcile({ appId: "4812", checkRunsAll: [checkRun("Needlefish: pass", "success")] }); + assert.equal(r.status, 0, r.stderr); + assert.equal(r.dispatched, 0); + assert.match(r.stdout, /already has a terminal Needlefish verdict/); + assert.doesNotMatch(r.log, /api -X POST/); +}); + +for (const externalId of [ + JSON.stringify({ v: 2, prNumber: 42 }), + JSON.stringify({ v: 2, prNumber: 42, roundId: "round" }), + JSON.stringify({ v: 2, prNumber: 42, roundId: "", runId: "777" }), + JSON.stringify({ v: 2, prNumber: 42, roundId: 5, runId: "777" }), + JSON.stringify({ v: 2, prNumber: 42, roundId: "round", runId: 777 }), + JSON.stringify({ v: 2, unrelated: true }), + JSON.stringify({ v: 1, prNumber: 42 }), + "[]", "true", "null", "unrelated", "", +]) { + test(`reconcile rejects incomplete or unrelated receipt ${externalId}`, () => { + const terminal = { ...checkRun("Needlefish: pass", "success"), external_id: externalId }; + const r = runReconcile({ checkRunsAll: [terminal] }); + assert.equal(r.status, 0, r.stderr); + assert.equal(r.dispatched, 1, r.stdout); + assert.match(r.log, /workflow run review.yml .*pr_number=42/); + }); +} + +test("reconcile accepts a complete v2 receipt with an empty run ID", () => { + const terminal = { ...checkRun("Needlefish: pass", "success"), external_id: JSON.stringify({ v: 2, prNumber: 42, roundId: "round", runId: "" }) }; + const r = runReconcile({ checkRunsAll: [terminal] }); + assert.equal(r.status, 0, r.stderr); + assert.equal(r.dispatched, 0); + assert.match(r.stdout, /already has a terminal Needlefish verdict/); +}); diff --git a/src/adapters/explain.test.ts b/src/adapters/explain.test.ts index 42857a9c..323ecef8 100644 --- a/src/adapters/explain.test.ts +++ b/src/adapters/explain.test.ts @@ -1,11 +1,12 @@ import assert from "node:assert/strict"; -import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { spawnSync } from "node:child_process"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import os from "node:os"; import path from "node:path"; import test from "node:test"; import { commitAll, gitText, headSha, initRepo } from "../shared/codex-runner-test-fixtures"; import { WITHHELD_MESSAGE } from "../shared/outbound-screen.js"; -import { runGithubExplain, screenExplanation } from "./explain.js"; +import { screenExplanation } from "./explain.js"; import { renderState } from "./github.js"; const KNOWN = "abc123def456ghi789jkl0"; @@ -27,7 +28,15 @@ test("screenExplanation returns clean text unchanged", () => { assert.equal(screenExplanation("plain explanation", {}), "plain explanation"); }); -test("runGithubExplain hands the model Needlefish's latest finding keys, not its review prose", async (t) => { +for (const { pin, receipt } of [ + { pin: undefined, receipt: "delivered" }, + { pin: "4812", receipt: "delivered" }, + { pin: "not-an-id", receipt: "delivered" }, + { pin: undefined, receipt: "legacy" }, + { pin: undefined, receipt: "in-progress" }, + { pin: undefined, receipt: "mismatched" }, +] as const) { +test(`explain CLI with App pin ${pin ?? "default"} and a ${receipt} state marker`, (t) => { const tmp = mkdtempSync(path.join(os.tmpdir(), "needlefish-explain-test-")); const repo = initRepo(tmp); const fakeBin = path.join(tmp, "bin"); @@ -38,6 +47,7 @@ test("runGithubExplain hands the model Needlefish's latest finding keys, not its repository: process.env.GITHUB_REPOSITORY, bin: process.env.CLAUDE_BIN, runner: process.env.NEEDLEFISH_RUNNER, + appId: process.env.NEEDLEFISH_CHECK_APP_ID, }; t.after(() => { if (previous.path === undefined) delete process.env.PATH; @@ -48,6 +58,8 @@ test("runGithubExplain hands the model Needlefish's latest finding keys, not its else process.env.CLAUDE_BIN = previous.bin; if (previous.runner === undefined) delete process.env.NEEDLEFISH_RUNNER; else process.env.NEEDLEFISH_RUNNER = previous.runner; + if (previous.appId === undefined) delete process.env.NEEDLEFISH_CHECK_APP_ID; + else process.env.NEEDLEFISH_CHECK_APP_ID = previous.appId; rmSync(tmp, { recursive: true, force: true }); }); @@ -58,7 +70,9 @@ test("runGithubExplain hands the model Needlefish's latest finding keys, not its commitAll(repo, "feature"); const head = headSha(repo); - const state = renderState(head, [ + const appId = pin === undefined ? 15368 : 4812; + const login = pin === undefined ? "github-actions[bot]" : "private-review-app[bot]"; + const delivered = renderState(head, [ { severity: "P2", title: "Doctor base check fails on a dirty worktree", @@ -71,7 +85,12 @@ test("runGithubExplain hands the model Needlefish's latest finding keys, not its suggestedFix: "fix", validation: "test", }, - ]); + ], { checkId: 1, appId, roundId: "explain-fixture", runId: "fixture" }); + const state = receipt === "legacy" + ? `` + : delivered; const reviewBody = `CHANGES REQUESTED ⚠️ — 1 blocking\n\nCoverage: full diff reviewed in one pass (1 file)\n\n## Findings\n\n| 1 | P2 | Doctor base check fails on a dirty worktree | app.ts:1 |\n\n${state}\n`; const prView = { number: 9, @@ -88,7 +107,12 @@ test("runGithubExplain hands the model Needlefish's latest finding keys, not its baseRefName: "main", headRefName: "feature", }; - const reviewsPage = [[{ id: 1, body: reviewBody, user: { login: "github-actions[bot]", type: "Bot" } }]]; + const reviewsPage = [[{ id: 1, body: reviewBody, user: { login, type: "Bot" } }]]; + const checks = [{ check_runs: [{ id: 1, head_sha: head, app: { id: appId, slug: pin === undefined ? "github-actions" : "private-review-app" }, + name: "Needlefish", status: receipt === "in-progress" ? "in_progress" : "completed", + conclusion: receipt === "in-progress" ? null : "failure", output: { title: "Needlefish: changes_requested" }, + external_id: JSON.stringify({ v: 2, prNumber: 9, roundId: receipt === "mismatched" ? "other-round" : "explain-fixture", runId: "fixture" }), + }] }]; mkdirSync(fakeBin); writeFileSync( path.join(fakeBin, "gh"), @@ -104,7 +128,7 @@ test("runGithubExplain hands the model Needlefish's latest finding keys, not its ` process.stdout.write(${JSON.stringify(JSON.stringify(reviewsPage))});`, " process.exit(0);", "}", - // An Actions token may not call GET /user; trust then rests on the bot identity. + `if (args.some(arg => arg.includes("/check-runs?"))) { process.stdout.write(${JSON.stringify(JSON.stringify(checks))}); process.exit(0); }`, "if (args[1] === 'user') { process.stderr.write('HTTP 403'); process.exit(1); }", "if (args.includes('--input')) {", ` fs.appendFileSync(${JSON.stringify(postLog)}, JSON.stringify({ args, payload: fs.readFileSync(0, 'utf8') }) + '\\n');`, @@ -131,7 +155,18 @@ test("runGithubExplain hands the model Needlefish's latest finding keys, not its process.env.CLAUDE_BIN = path.join(fakeBin, "claude"); process.env.NEEDLEFISH_RUNNER = "claude"; - await runGithubExplain(repo, 9, "dirty worktree", { timeoutMs: 1000 }); + if (pin === undefined) delete process.env.NEEDLEFISH_CHECK_APP_ID; + else process.env.NEEDLEFISH_CHECK_APP_ID = pin; + const result = spawnSync(process.execPath, ["--import", "tsx", "src/cli.ts", "explain", "9", + "--repo", repo, "--finding", "dirty worktree"], { encoding: "utf8", timeout: 20_000 }); + if (pin === "not-an-id") { + assert.equal(result.status, 1, result.stderr); + assert.match(result.stderr, /NEEDLEFISH_CHECK_APP_ID must be a positive safe integer/); + assert.equal(existsSync(promptLog), false); + assert.equal(existsSync(postLog), false); + return; + } + assert.equal(result.status, 0, result.stderr); const prompt = readFileSync(promptLog, "utf8"); const bundle = prompt.slice(prompt.indexOf("# Context bundle"), prompt.indexOf("# Diff")); @@ -139,11 +174,13 @@ test("runGithubExplain hands the model Needlefish's latest finding keys, not its latestReview?: { headSha: string; findings: { file: string; lineStart: number; title: string }[] }; prMeta: { reviews: string[] }; }; - assert.equal(context.latestReview?.headSha, head); - assert.deepEqual( - context.latestReview?.findings.map((f) => [f.file, f.lineStart, f.title]), - [["app.ts", 1, "doctor base check fails on a dirty worktree"]] - ); + if (receipt === "delivered") { + assert.equal(context.latestReview?.headSha, head); + assert.deepEqual( + context.latestReview?.findings.map((f) => [f.file, f.lineStart, f.title]), + [["app.ts", 1, "doctor base check fails on a dirty worktree"]] + ); + } else assert.equal(context.latestReview, undefined, "an undelivered marker gives explain no prior findings"); assert.deepEqual(context.prMeta.reviews, ["LGTM from me, one nit inline."]); assert.equal(prompt.includes("needlefish-state"), false); assert.equal(prompt.includes("Coverage: full diff"), false); @@ -155,3 +192,5 @@ test("runGithubExplain hands the model Needlefish's latest finding keys, not its assert.ok(body.startsWith("## 🔍 Needlefish explain\n\nThe guard runs after the write.")); assert.ok(body.endsWith("\n")); }); + +} diff --git a/src/adapters/explain.ts b/src/adapters/explain.ts index c39e1458..857e53fb 100644 --- a/src/adapters/explain.ts +++ b/src/adapters/explain.ts @@ -7,7 +7,7 @@ import { } from "../shared/outbound-screen.js"; import { ghText } from "../shared/repo.js"; import type { RunnerOptions } from "../shared/runner.js"; -import { findPreviousReview } from "./github.js"; +import { configuredCheckAppId, findDeliveredReview } from "./github.js"; import { prDiffBundle } from "./local.js"; // The explanation is model text posted publicly and outside ghPost, so it @@ -32,13 +32,14 @@ export async function runGithubExplain( findingKey: string, opts: RunnerOptions ): Promise { + const expectedAppId = configuredCheckAppId(); const repo = process.env.GITHUB_REPOSITORY; if (!repo) throw new Error("GITHUB_REPOSITORY not set (must run in Actions)"); const repoPath = path.resolve(cwd); const { bundle } = prDiffBundle(repoPath, prNumber, opts); - // Same trusted resolver as re-review: only a state marker written by the - // identity this run posts as names the findings. - const latestReview = findPreviousReview(repo, prNumber)?.state ?? null; + // Only a delivered round names the findings: a trusted author alone is not + // enough, since legacy and undelivered markers never reached a check. + const latestReview = findDeliveredReview(repo, prNumber, expectedAppId); const explanation = screenExplanation( await explainFinding(bundle, findingKey, opts, latestReview), process.env diff --git a/src/adapters/github-posting.test.ts b/src/adapters/github-posting.test.ts index 1a98e7f8..093c1203 100644 --- a/src/adapters/github-posting.test.ts +++ b/src/adapters/github-posting.test.ts @@ -43,6 +43,7 @@ type Post = { }; type Fixture = { + readonly prNumber: number; readonly postLog: string; readonly repo: string; readonly reviewOutput: string; @@ -72,6 +73,7 @@ type FixtureOptions = { // Make POSTs to the issue-comments endpoint exit 1 (after logging the // attempt) to exercise the fail-soft paths around cosmetic comments. readonly failIssueCommentPosts?: boolean; + readonly failIssueCommentReads?: boolean; // Make POSTs to the check-runs endpoint exit 1 (after logging the attempt) // to exercise independence of the failure check and the error comment. readonly failCheckRunPosts?: boolean; @@ -90,6 +92,16 @@ type FixtureOptions = { // Fail the first N POSTs to the reviews endpoint with a 502, then succeed. // Every attempt is still appended to postLog before the injected failure. readonly flakyReviewPosts?: number; + readonly storedReviewPost5xx?: boolean; + readonly storedCheckPost5xx?: boolean; + readonly foreignStoredCheck?: boolean; + readonly customCheckAppId?: number; + readonly duplicateStoredCheck?: boolean; + readonly storedIssuePost5xx?: boolean; + readonly failReviewReconcileReads?: boolean; + readonly failReviewPreRead?: boolean; + readonly failPreviousHeadCheckReads?: boolean; + readonly storedInlineLimit?: number; // Fail the first N PUTs to a review id with a 502, then succeed. readonly flakyReviewPuts?: number; // Fail every POST to the reviews endpoint with a 404 (non-retryable). @@ -105,6 +117,10 @@ type FixtureOptions = { // Make GET commits//check-runs fail, to exercise the fail-closed // behaviour of the completion-aware same-head gate. readonly failCheckRunReads?: boolean; + readonly failCheckRunReadsOn?: readonly number[]; + readonly lagStoredReviewRead?: boolean; + readonly lagStoredCheckRead?: boolean; + readonly lagStoredIssueRead?: boolean; // Make the runner stub wait this long before writing anything, so a low // timeoutMs produces a real process timeout (ETIMEDOUT). readonly runnerDelayMs?: number; @@ -190,6 +206,9 @@ function mkFinding(overrides: Partial = {}): Finding { } function setupFixture(t: TestContext, opts: FixtureOptions): Fixture { + const checkApp = opts.foreignStoredCheck ? { id: 999, slug: "foreign-app" } + : opts.customCheckAppId ? { id: opts.customCheckAppId, slug: "private-review-app" } + : { id: 15368, slug: "github-actions" }; const tmp = mkdtempSync( path.join(os.tmpdir(), "needlefish-github-posting-test-"), ); @@ -368,8 +387,8 @@ function setupFixture(t: TestContext, opts: FixtureOptions): Fixture { " fs.writeFileSync(reviewsPath, JSON.stringify(reviews));", ` const reviewCommentsPath = ${JSON.stringify(reviewCommentsState)};`, " const reviewComments = fs.existsSync(reviewCommentsPath) ? JSON.parse(fs.readFileSync(reviewCommentsPath, 'utf8')) : [];", - " for (const c of parsed.comments || []) {", - " reviewComments.push({ id: reviewComments.length + 1, pull_request_review_id: reviewId, in_reply_to_id: null, path: c.path, body: c.body, user: { login: AUTHOR_LOGIN, type: AUTHOR_TYPE } });", + ` for (const c of (parsed.comments || []).slice(0, fs.existsSync(reviewsPath + ".stored-5xx") ? 1000 : ${opts.storedInlineLimit ?? 1000})) {`, + " reviewComments.push({ id: reviewComments.length + 1, pull_request_review_id: reviewId, in_reply_to_id: null, path: c.path, line: c.line, start_line: c.start_line ?? null, side: c.side, body: c.body, user: { login: AUTHOR_LOGIN, type: AUTHOR_TYPE } });", " }", " fs.writeFileSync(reviewCommentsPath, JSON.stringify(reviewComments));", // Mirror the live API: Create-a-review returns the review object, @@ -377,6 +396,7 @@ function setupFixture(t: TestContext, opts: FixtureOptions): Fixture { // `return` (valid at CJS top level) instead of process.exit: exit // right after write drops queued pipe output above 64KB, which made // oversized-body tests fail as "GitHub returned invalid JSON". + ` if (${JSON.stringify(opts.storedReviewPost5xx === true)} && !fs.existsSync(reviewsPath + ".stored-5xx")) { fs.writeFileSync(reviewsPath + ".stored-5xx", "1"); process.stderr.write("gh: Server Error (HTTP 502)"); process.exit(1); }`, " process.stdout.write(JSON.stringify({ id: reviewId, body: parsed.body || '' }));", " process.exitCode = 0;", " return;", @@ -395,9 +415,11 @@ function setupFixture(t: TestContext, opts: FixtureOptions): Fixture { ` const checks = fs.existsSync(checksStatePath) ? JSON.parse(fs.readFileSync(checksStatePath, 'utf8')) : [];`, ` const parsedCheck = JSON.parse(payload);`, ` const nextId = checks.length + 1;`, - ` checks.push({ id: nextId, status: parsedCheck.status || 'completed', conclusion: parsedCheck.conclusion ?? null, output: parsedCheck.output ?? null });`, + ` checks.push({ id: nextId, name: parsedCheck.name, head_sha: parsedCheck.head_sha, external_id: parsedCheck.external_id, app: ${JSON.stringify(checkApp)}, status: parsedCheck.status || 'completed', conclusion: parsedCheck.conclusion ?? null, output: parsedCheck.output ?? null });`, + ` if (${opts.duplicateStoredCheck === true}) checks.push({ ...checks.at(-1), id: nextId + 1 });`, ` fs.writeFileSync(checksStatePath, JSON.stringify(checks));`, - ` process.stdout.write(JSON.stringify({ id: nextId }));`, + ` if (${JSON.stringify(opts.storedCheckPost5xx === true)} && !fs.existsSync(checksStatePath + ".stored-5xx")) { fs.writeFileSync(checksStatePath + ".stored-5xx", "1"); process.stderr.write("gh: Server Error (HTTP 502)"); process.exit(1); }`, + ` process.stdout.write(JSON.stringify({ id: nextId, app: ${JSON.stringify(checkApp)} }));`, ` process.exit(0);`, ` }`, ` if (apiPath && apiPath.startsWith('repos/frankekn/needlefish/check-runs/') && method === 'PATCH') {`, @@ -412,7 +434,7 @@ function setupFixture(t: TestContext, opts: FixtureOptions): Fixture { ` const checks = fs.existsSync(checksStatePath) ? JSON.parse(fs.readFileSync(checksStatePath, 'utf8')) : [];`, ` const parsedPatch = JSON.parse(payload);`, ` const check = checks.find((c) => c.id === id);`, - ` if (check) { check.status = parsedPatch.status || 'completed'; check.conclusion = parsedPatch.conclusion ?? null; check.output = parsedPatch.output ?? null; }`, + ` if (check) Object.assign(check, parsedPatch);`, ` fs.writeFileSync(checksStatePath, JSON.stringify(checks));`, ` process.stdout.write('{}');`, ` process.exit(0);`, @@ -424,6 +446,7 @@ function setupFixture(t: TestContext, opts: FixtureOptions): Fixture { " const nextId = issueComments.length + 1;", " issueComments.push({ id: nextId, node_id: 'IC_node_' + nextId, body: parsed.body || '', user: { login: AUTHOR_LOGIN, type: AUTHOR_TYPE } });", " fs.writeFileSync(issueCommentsPath, JSON.stringify(issueComments));", + ` if (${JSON.stringify(opts.storedIssuePost5xx === true)} && !fs.existsSync(issueCommentsPath + ".stored-5xx")) { fs.writeFileSync(issueCommentsPath + ".stored-5xx", "1"); process.stderr.write("gh: Server Error (HTTP 502)"); process.exit(1); }`, " }", " process.stdout.write('{}');", " process.exit(0);", @@ -447,10 +470,18 @@ function setupFixture(t: TestContext, opts: FixtureOptions): Fixture { `if (args[1] === '--paginate' && args[2] === '--slurp' && args[3] === ${JSON.stringify(`repos/frankekn/needlefish/pulls/${opts.prNumber}/reviews`)}) {`, ` const reviewsPath = ${JSON.stringify(reviewsState)};`, " const reviews = fs.existsSync(reviewsPath) ? JSON.parse(fs.readFileSync(reviewsPath, 'utf8')) : [];", + ` const reads = (fs.existsSync(reviewsPath + ".reads") ? Number(fs.readFileSync(reviewsPath + ".reads", "utf8")) : 0) + 1; fs.writeFileSync(reviewsPath + ".reads", String(reads));`, + ` if (${opts.failReviewPreRead === true} && reads === 2) { process.stderr.write("simulated review pre-read failure"); process.exit(1); }`, + ` if (${JSON.stringify(opts.failReviewReconcileReads === true)} && reviews.length > 0) { process.stderr.write("simulated review read failure"); process.exit(1); }`, + ` if (${JSON.stringify(opts.lagStoredReviewRead === true)} && fs.existsSync(reviewsPath + ".stored-5xx") && !fs.existsSync(reviewsPath + ".lagged-read")) { fs.writeFileSync(reviewsPath + ".lagged-read", "1"); process.stdout.write("[[]]"); process.exit(0); }`, ` const pages = ${JSON.stringify(opts.paginateEachReviewAsOwnPage === true)} ? reviews.map((r) => [r]) : ${JSON.stringify(opts.paginatePreviousReviewOnSecondPage === true)} ? [[], reviews] : [reviews];`, " process.stdout.write(JSON.stringify(pages));", " process.exit(0);", "}", + `if (args[1] === '--paginate' && args[2] === '--slurp' && /\\/reviews\\/\\d+\\/comments$/.test(args[3] || '')) {`, + ` const p = ${JSON.stringify(reviewCommentsState)}; const comments = fs.existsSync(p) ? JSON.parse(fs.readFileSync(p, 'utf8')) : [];`, + ` const id = Number(args[3].split('/').at(-2)); process.stdout.write(JSON.stringify([comments.filter(c => c.pull_request_review_id === id)])); process.exit(0);`, + `}`, // comments_url and review_comments_url serve what earlier rounds and // seeded humans posted, as the live API does; their bodies are what // reaches the model as prMeta. @@ -459,8 +490,10 @@ function setupFixture(t: TestContext, opts: FixtureOptions): Fixture { // Paginated GET must come with --slurp (page-wrapped array of arrays); // the stub emulates the slurped shape to pin the flat(1) handling. `if (args[1] === '--paginate' && args[2] === '--slurp' && args[3] === ${JSON.stringify(`repos/frankekn/needlefish/issues/${opts.prNumber}/comments`)}) {`, + ` if (${opts.failIssueCommentReads === true}) { process.stderr.write("simulated comment-list outage"); process.exit(1); }`, ` const issueCommentsPath = ${JSON.stringify(issueCommentsState)};`, " const issueComments = fs.existsSync(issueCommentsPath) ? JSON.parse(fs.readFileSync(issueCommentsPath, 'utf8')) : [];", + ` if (${JSON.stringify(opts.lagStoredIssueRead === true)} && fs.existsSync(issueCommentsPath + ".stored-5xx") && !fs.existsSync(issueCommentsPath + ".lagged-read")) { fs.writeFileSync(issueCommentsPath + ".lagged-read", "1"); process.stdout.write("[[]]"); process.exit(0); }`, " process.stdout.write(JSON.stringify([issueComments]));", " process.exit(0);", "}", @@ -473,10 +506,13 @@ function setupFixture(t: TestContext, opts: FixtureOptions): Fixture { // Dedupe probe: GET commits//check-runs. Returns the stored // checks verbatim so the completion-aware same-head gate can read // status/conclusion/output.title. Reads are never logged as posts. - `if (typeof args[1] === 'string' && args[1].startsWith('repos/frankekn/needlefish/commits/') && args[1].includes('/check-runs')) {`, - ` if (${JSON.stringify(opts.failCheckRunReads === true)} === true) { process.stderr.write('simulated check-run read failure'); process.exit(1); }`, + `if (args.some(a => a.startsWith('repos/frankekn/needlefish/commits/') && a.includes('/check-runs'))) {`, + ` const readCountPath = checksStatePath + ".reads"; const readCount = (fs.existsSync(readCountPath) ? Number(fs.readFileSync(readCountPath, 'utf8')) : 0) + 1; fs.writeFileSync(readCountPath, String(readCount));`, + ` if (${JSON.stringify(opts.failCheckRunReads === true)} === true || ${JSON.stringify(opts.failCheckRunReadsOn ?? [])}.includes(readCount)) { process.stderr.write('simulated check-run read failure'); process.exit(1); }`, + ` if (${opts.failPreviousHeadCheckReads === true} && args.some(a => a.includes(${JSON.stringify(`/commits/${baseTipSha}/check-runs`)}))) { process.stderr.write("simulated prior-head check read failure"); process.exit(1); }`, ` const checks = fs.existsSync(checksStatePath) ? JSON.parse(fs.readFileSync(checksStatePath, 'utf8')) : [];`, - ` process.stdout.write(JSON.stringify({ total_count: checks.length, check_runs: checks }));`, + ` if (${JSON.stringify(opts.lagStoredCheckRead === true)} && fs.existsSync(checksStatePath + ".stored-5xx") && !fs.existsSync(checksStatePath + ".lagged-read")) { fs.writeFileSync(checksStatePath + ".lagged-read", "1"); process.stdout.write(JSON.stringify([{ check_runs: [] }])); process.exit(0); }`, + ` process.stdout.write(JSON.stringify(args.includes("--slurp") ? [{ total_count: checks.length, check_runs: checks }] : { total_count: checks.length, check_runs: checks }));`, ` process.exit(0);`, `}`, `if (args[1] === 'user') { if (${JSON.stringify(opts.failUserApi === true)} === true) { process.stderr.write('simulated user lookup failure'); process.exit(1); } process.stdout.write(JSON.stringify({ login: AUTHOR_LOGIN, type: AUTHOR_TYPE })); process.exit(0); }`, @@ -534,6 +570,7 @@ function setupFixture(t: TestContext, opts: FixtureOptions): Fixture { process.env.NEEDLEFISH_RETRY_MS = "1"; delete process.env.NEEDLEFISH_ALLOW_UNTRUSTED_AUTHOR; return { + prNumber: opts.prNumber, postLog, repo, reviewOutput: reviewOutputFile, @@ -628,8 +665,10 @@ function spawnGithubCliMerged( return { status: result.status, output: result.stdout ?? "" }; } -function stateReviewBody(headSha: string): string { - return `# Needlefish review\n\n${renderState(headSha, [mkFinding({ title: "bug", lineStart: 1 })])}\n`; +const FIXTURE_RECEIPT = { checkId: 1, appId: 15368, roundId: "fixture-round", runId: "fixture-run" }; + +function stateReviewBody(headSha: string, checkId = 1): string { + return `# Needlefish review\n\n${renderState(headSha, [mkFinding({ title: "bug", lineStart: 1 })], { ...FIXTURE_RECEIPT, checkId })}\n`; } function seedReviews(reviewsState: string, reviews: readonly unknown[]): void { @@ -638,12 +677,13 @@ function seedReviews(reviewsState: string, reviews: readonly unknown[]): void { // A completed, non-infra Needlefish check for the head: the delivery proof the // completion-aware same-head gate requires before it may skip. -function seedCompletedVerdictCheck(fixture: Fixture): void { +function seedCompletedVerdictCheck(fixture: Fixture, head = fixture.headSha): void { writeFileSync( fixture.checksState, JSON.stringify([ { - id: 1, + id: 1, name: "Needlefish", head_sha: head, app: { id: 15368, slug: "github-actions" }, + external_id: JSON.stringify({ v: 2, prNumber: fixture.prNumber, roundId: "fixture-round", runId: "fixture-run", recoveryAttempt: 0 }), status: "completed", conclusion: "success", output: { title: "Needlefish: pass" }, @@ -810,7 +850,7 @@ test("capOutboundBody shrinks an oversized state marker rather than dropping it" const findings = Array.from({ length: 3000 }, (_, i) => mkFinding({ title: `finding ${i}`, lineStart: i + 1 }), ); - const body = `Review\n\n${renderState("a".repeat(40), findings)}\n`; + const body = `Review\n\n${renderState("a".repeat(40), findings, FIXTURE_RECEIPT)}\n`; assert.ok(body.length > 65_536); const capped = capOutboundBody(body); assert.ok(capped.length <= 65_536, `got ${capped.length}`); @@ -831,7 +871,7 @@ test("capOutboundBody shrinks an oversized state marker rather than dropping it" test("capOutboundBody keeps a quoted state marker as body text", () => { const quoted = renderState("b".repeat(40), [ mkFinding({ title: "quoted prior" }), - ]); + ], FIXTURE_RECEIPT); const body = [ "**P2** bug", "", @@ -1276,10 +1316,10 @@ test("runGithub inlines only P0/P1/P2 when more than 20 findings are anchorable" test("renderState produces an HTML comment with versioned JSON", () => { const marker = renderState("abc123", [ mkFinding({ title: "Bug", lineStart: 5 }), - ]); + ], FIXTURE_RECEIPT); assert.match(marker, /^$/); - assert.match(marker, /"v":1/); + assert.match(marker, /"v":2/); assert.match(marker, /"headSha":"abc123"/); assert.match(marker, /"title":"bug"/); }); @@ -1289,10 +1329,10 @@ test("parseState round-trips through renderState", () => { mkFinding({ title: "Null deref", lineStart: 42 }), mkFinding({ title: "Race", lineStart: 100, file: "other.ts" }), ]; - const marker = renderState("dead", findings); + const marker = renderState("dead", findings, FIXTURE_RECEIPT); const parsed = parseState(`# Review\n\nbody text\n\n${marker}\n`); assert.ok(parsed); - assert.equal(parsed!.v, 1); + assert.equal(parsed!.v, 2); assert.equal(parsed!.headSha, "dead"); assert.equal(parsed!.findings.length, 2); assert.equal(parsed!.findings[0].file, "README.md"); @@ -1305,7 +1345,7 @@ test("state marker round-trips a title containing -->", () => { const findings: Finding[] = [ mkFinding({ title: "stale comment x --> y kept", lineStart: 42 }), ]; - const marker = renderState("abc123", findings); + const marker = renderState("abc123", findings, FIXTURE_RECEIPT); // The JSON payload must not contain "-->", only the comment terminator may. assert.equal(marker.split("-->").length - 1, 1); const parsed = parseState(`# Review\n\nbody text\n\n${marker}\n`); @@ -2031,6 +2071,7 @@ test("minimizePreviousRoundComments still sweeps a loosely bot-shaped author", a ]), ); + seedCompletedVerdictCheck(fixture, "c".repeat(40)); await runGithub(fixture.repo, 65, { timeoutMs: 1000 }); const minimized = readPosts(fixture.postLog).some( @@ -2074,7 +2115,7 @@ test("runGithub ignores an untrusted review that quotes a trusted state marker", prNumber: 54, rawReview: defaultRawReview(), }); - const quoted = `Looks correct.\n\n> ${renderState(fixture.headSha, [mkFinding({ title: "bug", lineStart: 1 })])}\n`; + const quoted = `Looks correct.\n\n> ${renderState(fixture.headSha, [mkFinding({ title: "bug", lineStart: 1 })], FIXTURE_RECEIPT)}\n`; seedReviews(fixture.reviewsState, [ { id: 1, @@ -2141,6 +2182,7 @@ test("runGithub skips an untrusted newest marker and still uses an older trusted }, ]); + seedCompletedVerdictCheck(fixture, otherHead); await runGithub(fixture.repo, 56, { timeoutMs: 1000 }); assert.ok( @@ -2204,6 +2246,7 @@ test("runGithub still finds a trusted marker when an untrusted one is newest acr }, ]); + seedCompletedVerdictCheck(fixture, otherHead); await runGithub(fixture.repo, 58, { timeoutMs: 1000 }); assert.ok(runnerInvocationCount(fixture) >= 1); @@ -2223,6 +2266,7 @@ test("runGithub re-reviews a trusted same-head marker when recheck is true", asy }, ]); + seedCompletedVerdictCheck(fixture); await runGithub(fixture.repo, 59, { timeoutMs: 1000 }, true); assert.ok( @@ -3015,7 +3059,7 @@ function reviewPutAttempts(posts: readonly Post[], prNumber: number): number { // A state-bearing review on an older head: dedupe does not trigger, but this // round still PUTs the new body onto the existing review id — the re-review // path that exercises updateReviewBody in a single runGithub call. -function seedStaleHeadReview(fixture: { reviewsState: string }): void { +function seedStaleHeadReview(fixture: Fixture): void { seedReviews(fixture.reviewsState, [ { id: 1, @@ -3023,6 +3067,7 @@ function seedStaleHeadReview(fixture: { reviewsState: string }): void { user: { login: "github-actions[bot]", type: "Bot" }, }, ]); + seedCompletedVerdictCheck(fixture, "c".repeat(40)); } test("a transient 502 on the review PUT is retried once and the update lands", async (t) => { @@ -3067,7 +3112,7 @@ test("a transient 502 on the review PUT is retried once and the update lands", a ); }); -test("a 502 on the review POST is not retried — POSTs are not idempotent", async (t) => { +test("a 502 review POST defers recovery when no artifact is visible", async (t) => { const fixture = setupFixture(t, { prNumber: 83, rawReview: defaultRawReview(), @@ -3080,7 +3125,7 @@ test("a 502 on the review POST is not retried — POSTs are not idempotent", asy assert.equal( reviewPostAttempts(posts, 83), 1, - "a POST must keep single-attempt semantics even on a 5xx", + "a negative read is not proof a POST was not stored", ); const checkOps = posts.filter((p) => p.args.some((a) => a.includes("check-runs")), @@ -3228,10 +3273,11 @@ function seedPrevRound(fixture: Fixture, prevHead: string): void { seedReviews(fixture.reviewsState, [ { id: 1, - body: `# Needlefish review\n\n${renderState(prevHead, roundFateFindings())}\n`, + body: `# Needlefish review\n\n${renderState(prevHead, roundFateFindings(), FIXTURE_RECEIPT)}\n`, user: { login: "github-actions[bot]", type: "Bot" }, }, ]); + seedCompletedVerdictCheck(fixture, prevHead); } function roundCommentBody(posts: readonly Post[], prNumber: number): string { @@ -3771,6 +3817,7 @@ test("an inline-comment failure on a recheck leaves no marker for the next run t }, ]); + seedCompletedVerdictCheck(fixture); process.exitCode = undefined; await runGithub(fixture.repo, 91, { timeoutMs: 1000 }, true); assert.equal(process.exitCode, 1, "the inline POST failure must fail the run"); @@ -4060,21 +4107,11 @@ test("a same-head marker without a completed verdict check re-reviews", async (t "a marker with no delivered check must not suppress the review", ); const posts = readPosts(fixture.postLog); - const reviewPuts = posts.filter( - (p) => - p.args.includes("PUT") && - p.args.some((a) => - a.startsWith("repos/frankekn/needlefish/pulls/100/reviews/"), - ), - ); - assert.ok( - reviewPuts.length > 0, - "the recovered run updates the existing review", - ); - assert.match( - parseReviewPayload(reviewPuts.at(-1)!.payload).body, - /needlefish-state:/, - ); + const fresh = postedReview(posts, 100); + assert.ok(fresh, "an incomplete prior round must not hide fresh inline findings"); + assert.equal(parseReviewPayload(fresh.payload).comments.length, 1); + assert.equal(parseState(readReviewBodies(fixture.reviewsState).at(-1) ?? "")?.v, 2); + }); test("a same-head marker whose only check is an infra failure re-reviews", async (t) => { @@ -4125,7 +4162,8 @@ test("a same-head marker with a completed blocking verdict check still skips", a fixture.checksState, JSON.stringify([ { - id: 1, + id: 1, name: "Needlefish", head_sha: fixture.headSha, app: { id: 15368, slug: "github-actions" }, + external_id: JSON.stringify({ v: 2, prNumber: fixture.prNumber, roundId: "fixture-round", runId: "fixture-run", recoveryAttempt: 0 }), status: "completed", conclusion: "failure", output: { title: "Needlefish: changes_requested — bug" }, @@ -4144,7 +4182,7 @@ test("a same-head marker with a completed blocking verdict check still skips", a assert.equal(process.exitCode, 1); }); -test("an unreadable check list re-reviews instead of skipping", async (t) => { +test("an unreadable check list fails explicitly without skipping or spending model calls", async (t) => { const fixture = setupFixture(t, { prNumber: 103, rawReview: defaultRawReview(), @@ -4160,10 +4198,11 @@ test("an unreadable check list re-reviews instead of skipping", async (t) => { await runGithub(fixture.repo, 103, { timeoutMs: 1000 }); - assert.ok( - runnerInvocationCount(fixture) >= 1, - "an unreadable delivery proof must fail closed and re-review", - ); + assert.equal(runnerInvocationCount(fixture), 0); + assert.equal(lastCheckCompletion(readPosts(fixture.postLog)).conclusion, "failure"); + assert.equal(process.exitCode, 1); + assert.match(String(lastCheckCompletion(readPosts(fixture.postLog)).output?.summary), /history is unreadable/); + }); test("a same-head marker whose only check is a superseded neutral check re-reviews", async (t) => { @@ -4217,7 +4256,8 @@ test("a same-head marker with a failed needs_human check skips and exits 1", asy fixture.checksState, JSON.stringify([ { - id: 1, + id: 1, name: "Needlefish", head_sha: fixture.headSha, app: { id: 15368, slug: "github-actions" }, + external_id: JSON.stringify({ v: 2, prNumber: fixture.prNumber, roundId: "fixture-round", runId: "fixture-run", recoveryAttempt: 0 }), status: "completed", conclusion: "failure", output: { title: "Needlefish: needs_human — unverified migration" }, @@ -4281,12 +4321,14 @@ for (const scenario of [ { label: "old blocker before current pass", previous: "changes_requested", latest: "pass", reverse: false }, { label: "current pass before old blocker", previous: "changes_requested", latest: "pass", reverse: true }, ] as const) { - test(`same-head exit uses the newest completed verdict: ${scenario.label}`, (t) => { + test(`same-head exit uses the receipt-bound completed verdict: ${scenario.label}`, (t) => { const fixture = setupFixture(t, { prNumber: 108, rawReview: defaultRawReview() }); - seedReviews(fixture.reviewsState, [{ id: 1, body: stateReviewBody(fixture.headSha), + seedReviews(fixture.reviewsState, [{ id: 1, body: stateReviewBody(fixture.headSha, 2), user: { login: "github-actions[bot]", type: "Bot" } }]); const check = (id: number, verdict: typeof scenario.previous | typeof scenario.latest) => ({ - id, status: "completed", conclusion: verdict === "pass" ? "success" : verdict === "neutral" ? "neutral" : "failure", + id, name: "Needlefish", head_sha: fixture.headSha, app: { id: 15368, slug: "github-actions" }, + external_id: JSON.stringify({ v: 2, prNumber: fixture.prNumber, roundId: "fixture-round", runId: "fixture-run", recoveryAttempt: 0 }), + status: "completed", conclusion: verdict === "pass" ? "success" : verdict === "neutral" ? "neutral" : "failure", output: { title: `Needlefish: ${verdict === "neutral" ? "needs_human" : verdict}` }, }); const checks = [check(1, scenario.previous), check(2, scenario.latest)]; @@ -4299,33 +4341,610 @@ for (const scenario of [ }); } +for (const artifact of ["review", "check"] as const) { + test(`stored ${artifact} POST 5xx is reconciled without duplicate artifacts`, async (t) => { + const fixture = setupFixture(t, { prNumber: 120, rawReview: defaultRawReview(), + storedReviewPost5xx: artifact === "review", storedCheckPost5xx: artifact === "check" }); + await runGithub(fixture.repo, 120, { timeoutMs: 1000 }); + assert.equal(process.exitCode, 1); + const reviews: unknown = parseJson(readFileSync(fixture.reviewsState, "utf8")); + assert.ok(Array.isArray(reviews)); + assert.equal(reviews.length, 1); + assert.ok(typeof reviews[0] === "object" && reviews[0] !== null); + const state = parseState(String(Reflect.get(reviews[0], "body"))); + assert.equal(state?.v, 2); + const checks: unknown = parseJson(readFileSync(fixture.checksState, "utf8")); + assert.ok(Array.isArray(checks)); + assert.equal(checks.length, 1); + assert.equal(lastCheckCompletion(readPosts(fixture.postLog)).conclusion, "failure"); + const before = runnerInvocationCount(fixture); + await runGithub(fixture.repo, 120, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), before); + }); +} + +test("legacy state and unrelated same-name check cannot prove delivery", async (t) => { + const fixture = setupFixture(t, { prNumber: 121, rawReview: defaultRawReview() }); + seedReviews(fixture.reviewsState, [{ id: 1, body: ``, user: { login: "github-actions[bot]", type: "Bot" } }]); + seedCompletedVerdictCheck(fixture); + await runGithub(fixture.repo, 121, { timeoutMs: 1000 }); + assert.ok(runnerInvocationCount(fixture) > 0); + const fresh = postedReview(readPosts(fixture.postLog), 121); + assert.ok(fresh); + assert.equal(parseReviewPayload(fresh.payload).comments.length, 1); +}); + +for (const mismatch of ["app", "head", "round", "run", "id", "author", "pr"] as const) { + test(`receipt rejects an unrelated ${mismatch} before same-head skip`, async (t) => { + const fixture = setupFixture(t, { prNumber: 122, rawReview: defaultRawReview() }); + await runGithub(fixture.repo, 122, { timeoutMs: 1000 }); + const checks: unknown = parseJson(readFileSync(fixture.checksState, "utf8")); + const reviews: unknown = parseJson(readFileSync(fixture.reviewsState, "utf8")); + assert.ok(Array.isArray(checks) && isRecordLike(checks[0])); + assert.ok(Array.isArray(reviews) && isRecordLike(reviews[0])); + const check = checks[0]; + if (mismatch === "app") check.app = { id: 999 }; + if (mismatch === "head") check.head_sha = "a".repeat(40); + if (mismatch === "id") check.id = 999; + if (mismatch === "round" || mismatch === "run" || mismatch === "pr") { + const external: unknown = parseJson(String(check.external_id)); + assert.ok(isRecordLike(external)); + external[mismatch === "pr" ? "prNumber" : mismatch === "round" ? "roundId" : "runId"] = mismatch === "pr" ? 999 : "other"; + check.external_id = JSON.stringify(external); + } + if (mismatch === "author") reviews[0].user = { login: "other[bot]", type: "Bot" }; + writeFileSync(fixture.checksState, JSON.stringify(checks)); + writeFileSync(fixture.reviewsState, JSON.stringify(reviews)); + const before = readPosts(fixture.postLog).length; + await runGithub(fixture.repo, 122, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), 4); + const fresh = postedReview(readPosts(fixture.postLog).slice(before), 122); + assert.ok(fresh); + assert.equal(parseReviewPayload(fresh.payload).comments.length, 1); + }); +} + +test("failed check PATCHes cannot reset the recovery budget", async (t) => { + const fixture = setupFixture(t, { prNumber: 123, rawReview: defaultRawReview(), flakyCheckRunPatches: 99 }); + await runGithub(fixture.repo, 123, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), 2); + await runGithub(fixture.repo, 123, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), 4); + assert.equal(process.exitCode, 1); + await runGithub(fixture.repo, 123, { timeoutMs: 1000 }); + await runGithub(fixture.repo, 123, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), 4); + const completions = readPosts(fixture.postLog).filter(p => p.args.includes("PATCH")); + assert.match(completions.at(-1)?.payload ?? "", /recovery limit reached/); +}); + +test("recovery stops after one failed full re-review and recheck remains explicit", async (t) => { + const fixture = setupFixture(t, { prNumber: 124, rawReview: defaultRawReview(), reviewPost404: true }); + await runGithub(fixture.repo, 124, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), 2); + await runGithub(fixture.repo, 124, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), 4); + await runGithub(fixture.repo, 124, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), 4); + const posts = readPosts(fixture.postLog); + assert.match(String(lastCheckCompletion(posts).output?.summary), /plain rerun stops here again/); + const advicePost = posts.filter(p => p.args.includes("POST") && p.args.some(a => /\/issues\/\d+\/comments$/.test(a))).at(-1); + assert.ok(advicePost); + const advicePayload = parseJson(advicePost.payload); + assert.ok(isRecordLike(advicePayload)); + const advice = String(advicePayload.body).split("```\n\n").at(-1); + assert.equal(advice, "A plain rerun stops here again. Force a full review with the recheck input or comment `@needlefish recheck`.\n"); + const failureComment = posts.filter(p => p.args.includes("repos/frankekn/needlefish/issues/124/comments")).at(-1); + assert.ok(failureComment); + assert.match(failureComment.payload, /@needlefish recheck/); + assert.doesNotMatch(failureComment.payload, /Review did not complete\. Re-run the review\./); + await runGithub(fixture.repo, 124, { timeoutMs: 1000 }, true); + assert.equal(runnerInvocationCount(fixture), 6); +}); + +test("a stored round-comment 5xx with delayed visibility survives ordinary rerun without posting twice", async (t) => { + const fixture = setupFixture(t, { prNumber: 125, rawReview: defaultRawReview(), storedIssuePost5xx: true, lagStoredIssueRead: true }); + await runGithub(fixture.repo, 125, { timeoutMs: 1000 }); + await runGithub(fixture.repo, 125, { timeoutMs: 1000 }, true); + const comments: unknown = parseJson(readFileSync(fixture.issueCommentsState, "utf8")); + assert.ok(Array.isArray(comments)); + assert.equal(comments.length, 1); + assert.ok(isRecordLike(comments[0])); + assert.match(String(comments[0].body), /needlefish-round/); + assert.equal(lastCheckCompletion(readPosts(fixture.postLog)).conclusion, "failure"); + const before = readPosts(fixture.postLog).length; + const rerun = spawnGithubCli(fixture, 125); + assert.equal(rerun.status, 1, rerun.stderr); + assert.match(rerun.stdout, /"reason":"same_head"/); + assert.equal(runnerInvocationCount(fixture), 4); + assert.equal(readPosts(fixture.postLog).length, before); +}); + +test("an unreadable ambiguous review POST is not repeated", async (t) => { + const fixture = setupFixture(t, { prNumber: 126, rawReview: defaultRawReview(), storedReviewPost5xx: true, failReviewReconcileReads: true }); + await runGithub(fixture.repo, 126, { timeoutMs: 1000 }); + assert.equal(reviewPostAttempts(readPosts(fixture.postLog), 126), 1); + assert.match(String(lastCheckCompletion(readPosts(fixture.postLog)).output?.title), /review failed/); + assert.equal(parseState(readReviewBodies(fixture.reviewsState)[0] ?? ""), null); + assert.equal(process.exitCode, 1); +}); + +test("partial stored inline delivery remains fresh on ordinary recovery", async (t) => { + const fixture = setupFixture(t, { prNumber: 127, rawReview: JSON.stringify({ summary: "two findings", + findings: [mkFinding({ title: "first bug", lineStart: 1 }), mkFinding({ title: "second bug", lineStart: 25, lineEnd: 25 })], + checked: ["checked"], residual_risks: [] }), readmeContent: Array.from({ length: 30 }, (_, i) => `new line ${i + 1}\n`).join(""), + storedReviewPost5xx: true, storedInlineLimit: 1 }); + await runGithub(fixture.repo, 127, { timeoutMs: 1000 }); + assert.equal(parseState(readReviewBodies(fixture.reviewsState)[0] ?? ""), null); + assert.match(String(lastCheckCompletion(readPosts(fixture.postLog)).output?.title), /review failed/); + const before = readPosts(fixture.postLog).length; + await runGithub(fixture.repo, 127, { timeoutMs: 1000 }); + const recovered = postedReview(readPosts(fixture.postLog).slice(before), 127); + assert.ok(recovered); + assert.equal(parseReviewPayload(recovered.payload).comments.length, 2); + assert.match(String(parseReviewPayload(recovered.payload).comments[1]?.body ?? ""), /second bug/); + assert.equal(parseState(readReviewBodies(fixture.reviewsState).at(-1) ?? "")?.v, 2); +}); + +test("a newer owned failed recheck invalidates an older same-head receipt", async (t) => { + const fixture = setupFixture(t, { prNumber: 128, rawReview: defaultRawReview() }); + await runGithub(fixture.repo, 128, { timeoutMs: 1000 }); + writeFileSync(fixture.reviewOutput, "not JSON"); + await runGithub(fixture.repo, 128, { timeoutMs: 1000 }, true); + writeFileSync(fixture.reviewOutput, defaultRawReview()); + const before = runnerInvocationCount(fixture); + await runGithub(fixture.repo, 128, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), before + 2); + assert.match(String(lastCheckCompletion(readPosts(fixture.postLog)).output?.title), /changes_requested/); +}); + +test("a foreign app cannot supply the reconciled pending check", async (t) => { + const fixture = setupFixture(t, { prNumber: 129, rawReview: defaultRawReview(), storedCheckPost5xx: true, foreignStoredCheck: true }); + await runGithub(fixture.repo, 129, { timeoutMs: 1000 }); + const posts = readPosts(fixture.postLog); + const creates = posts.filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/check-runs")); + assert.equal(creates.length, 1); + assert.equal(runnerInvocationCount(fixture), 0); + assert.equal(posts.filter(p => p.args.includes("PATCH")).length, 0); + assert.equal(process.exitCode, 1); + const errors: unknown = parseJson(readFileSync(fixture.issueCommentsState, "utf8")); + assert.ok(Array.isArray(errors) && isRecordLike(errors[0])); + assert.match(String(errors[0].body), /delivery could not start/); +}); + +for (const artifact of ["review", "comment"] as const) { + test(`unreadable PAT identity does not repeat a stored ${artifact} POST`, async (t) => { + const fixture = setupFixture(t, { prNumber: 130, rawReview: artifact === "review" ? defaultRawReview() : "not JSON", + authorLogin: "review-pat", failUserApi: true, storedReviewPost5xx: artifact === "review", storedIssuePost5xx: artifact === "comment" }); + await runGithub(fixture.repo, 130, { timeoutMs: 1000 }); + const posts = readPosts(fixture.postLog); + const endpoint = artifact === "review" ? "repos/frankekn/needlefish/pulls/130/reviews" : "repos/frankekn/needlefish/issues/130/comments"; + assert.equal(posts.filter(p => p.args.includes("POST") && p.args.includes(endpoint)).length, 1); + const stored: unknown = parseJson(readFileSync(artifact === "review" ? fixture.reviewsState : fixture.issueCommentsState, "utf8")); + assert.ok(Array.isArray(stored)); + assert.equal(stored.length, 1); + assert.match(String(lastCheckCompletion(posts).output?.title), /review failed/); + }); +} + +test("stale recovery finalizes only its owned check", async (t) => { + const fixture = setupFixture(t, { prNumber: 131, rawReview: "not JSON", staleHeadAfterReview: true }); + writeFileSync(fixture.checksState, JSON.stringify([{ id: 1, name: "Needlefish", head_sha: fixture.headSha, + app: { id: 999, slug: "foreign-app" }, status: "in_progress", conclusion: null }])); + await runGithub(fixture.repo, 131, { timeoutMs: 1000 }); + const posts = readPosts(fixture.postLog); + const patches = posts.filter(p => p.args.includes("PATCH")); + assert.equal(patches.length, 1); + assert.ok(patches[0]?.args.includes("repos/frankekn/needlefish/check-runs/2")); + assert.equal(lastCheckCompletion(posts).conclusion, "neutral"); + assert.equal(reviewPostAttempts(posts, 131), 0); + const checks: unknown = parseJson(readFileSync(fixture.checksState, "utf8")); + assert.ok(Array.isArray(checks) && isRecordLike(checks[0]) && isRecordLike(checks[1])); + assert.equal(checks[0].status, "in_progress"); + assert.equal(checks[1].status, "completed"); +}); + +test("other PR rounds sharing the head cannot consume this PR recovery budget", async (t) => { + const fixture = setupFixture(t, { prNumber: 132, rawReview: defaultRawReview() }); + writeFileSync(fixture.checksState, JSON.stringify([1, 2].map(id => ({ id, name: "Needlefish", head_sha: fixture.headSha, + app: { id: 15368, slug: "github-actions" }, status: "completed", conclusion: "failure", + external_id: JSON.stringify({ v: 2, prNumber: 900, roundId: `other-${id}`, runId: "other-run", recoveryAttempt: 0, recheck: false }), + output: { title: "Needlefish: review failed" } })))); + await runGithub(fixture.repo, 132, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), 2); + assert.match(String(lastCheckCompletion(readPosts(fixture.postLog)).output?.title), /changes_requested/); + assert.equal(parseState(readReviewBodies(fixture.reviewsState).at(-1) ?? "")?.v, 2); +}); + +for (const [prNumber, kind, conclusion, title] of [ + [197, "skip", "neutral", "Needlefish: skipped (author not trusted)"], + [198, "retry-required", "failure", "Needlefish: caller retry required"], +] as const) { + test(`workflow ${kind} receipts on the head cannot consume the recovery budget`, async (t) => { + const fixture = untrustedFixture(t, prNumber, "FIRST_TIME_CONTRIBUTOR"); + process.env.NEEDLEFISH_ALLOW_UNTRUSTED_AUTHOR = "1"; + writeFileSync(fixture.checksState, JSON.stringify([1, 2].map(id => ({ id, name: "Needlefish", head_sha: fixture.headSha, + app: { id: 15368, slug: "github-actions" }, status: "completed", conclusion, output: { title }, + external_id: JSON.stringify({ v: 2, prNumber, roundId: `${kind}-run-${id}`, runId: "run" }) })))); + await runGithub(fixture.repo, prNumber, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), 2); + assert.equal(lastCheckCompletion(readPosts(fixture.postLog)).conclusion, "success"); + }); +} + +test("a newer round from another PR cannot invalidate this PR receipt", async (t) => { + const fixture = setupFixture(t, { prNumber: 133, rawReview: defaultRawReview() }); + await runGithub(fixture.repo, 133, { timeoutMs: 1000 }); + const raw: unknown = parseJson(readFileSync(fixture.checksState, "utf8")); + assert.ok(Array.isArray(raw)); + raw.push({ id: 2, name: "Needlefish", head_sha: fixture.headSha, app: { id: 15368, slug: "github-actions" }, + status: "completed", conclusion: "failure", output: { title: "Needlefish: review failed" }, + external_id: JSON.stringify({ v: 2, prNumber: 900, roundId: "other", runId: "other-run", recoveryAttempt: 0, recheck: false }) }); + writeFileSync(fixture.checksState, JSON.stringify(raw)); + const before = readPosts(fixture.postLog).length; + const result = spawnGithubCli(fixture, 133); + assert.equal(result.status, 1, result.stderr); + assert.match(result.stdout, /"reason":"same_head"/); + assert.equal(runnerInvocationCount(fixture), 2); + assert.equal(readPosts(fixture.postLog).length, before); +}); + +test("a transient history read after completed delivery preserves one recovery", async (t) => { + const fixture = setupFixture(t, { prNumber: 134, rawReview: JSON.stringify({ summary: "clean", findings: [], checked: ["checked"], residual_risks: [] }), failCheckRunReadsOn: [2] }); + await runGithub(fixture.repo, 134, { timeoutMs: 1000 }); + assert.equal(process.exitCode, 0); + await runGithub(fixture.repo, 134, { timeoutMs: 1000 }); + assert.equal(process.exitCode, 1); + assert.equal(runnerInvocationCount(fixture), 2); + await runGithub(fixture.repo, 134, { timeoutMs: 1000 }); + assert.equal(process.exitCode, 0); + assert.equal(runnerInvocationCount(fixture), 4); + assert.equal(lastCheckCompletion(readPosts(fixture.postLog)).conclusion, "success"); + const before = readPosts(fixture.postLog).length; + await runGithub(fixture.repo, 134, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), 4); + assert.equal(readPosts(fixture.postLog).length, before); +}); + +for (const artifact of ["review", "check"] as const) { + test(`a stored ${artifact} POST with delayed visibility is never resent`, async (t) => { + const fixture = setupFixture(t, { prNumber: 135, rawReview: defaultRawReview(), storedReviewPost5xx: artifact === "review", storedCheckPost5xx: artifact === "check", + lagStoredReviewRead: artifact === "review", lagStoredCheckRead: artifact === "check" }); + await runGithub(fixture.repo, 135, { timeoutMs: 1000 }); + const endpoint = artifact === "review" ? "repos/frankekn/needlefish/pulls/135/reviews" : "repos/frankekn/needlefish/check-runs"; + const posts = readPosts(fixture.postLog); + assert.equal(posts.filter(p => p.args.includes("POST") && p.args.includes(endpoint)).length, 1); + const rerun = spawnGithubCli(fixture, 135); + assert.equal(rerun.status, 1, rerun.stderr); + assert.match(rerun.stdout, /"reason":"same_head"/); + assert.equal(readPosts(fixture.postLog).filter(p => p.args.includes("POST") && p.args.includes(endpoint)).length, 1); + const stored: unknown = parseJson(readFileSync(artifact === "review" ? fixture.reviewsState : fixture.checksState, "utf8")); + assert.ok(Array.isArray(stored)); + assert.equal(stored.length, 1); + assert.equal(runnerInvocationCount(fixture), 2); + assert.equal(parseState(readReviewBodies(fixture.reviewsState).at(-1) ?? "")?.v, 2); + assert.equal(lastCheckCompletion(readPosts(fixture.postLog)).conclusion, "failure"); + assert.equal(process.exitCode, 1); + }); +} + +test("a trusted review quoting a v2 receipt in model prose cannot skip", async (t) => { + const fixture = setupFixture(t, { prNumber: 136, rawReview: defaultRawReview() }); + seedCompletedVerdictCheck(fixture); + const quoted = renderState(fixture.headSha, [mkFinding({ title: "bug" })], FIXTURE_RECEIPT); + seedReviews(fixture.reviewsState, [{ id: 1, body: `# Needlefish review\n\nQuoted source ${quoted}\n\n\n2 calls\n`, user: { login: "github-actions[bot]", type: "Bot" } }]); + await runGithub(fixture.repo, 136, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), 2); + const posted = postedReview(readPosts(fixture.postLog), 136); + assert.ok(posted); + assert.equal(parseReviewPayload(posted.payload).comments.length, 1); + assert.equal(parseState(readReviewBodies(fixture.reviewsState).at(-1) ?? "")?.v, 2); +}); + + +test("failure comments still post once when the comment-list pre-read fails", async (t) => { + const fixture = setupFixture(t, { prNumber: 137, rawReview: defaultRawReview(), reviewPost404: true, failIssueCommentReads: true }); + const result = spawnGithubCli(fixture, 137); + assert.equal(result.status, 1, result.stderr); + assert.equal(lastCheckCompletion(readPosts(fixture.postLog)).output?.title, "Needlefish: review failed"); + const comments = readPosts(fixture.postLog).filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/issues/137/comments")); + assert.equal(comments.length, 1); + assert.match(comments[0].payload, /needlefish-error/); + assert.equal(runnerInvocationCount(fixture), 2); +}); + +test("round history bounds recovery without a redundant counter", async (t) => { + const fixture = setupFixture(t, { prNumber: 138, rawReview: defaultRawReview(), flakyCheckRunPatches: 99 }); + await runGithub(fixture.repo, 138, { timeoutMs: 1000 }); + await runGithub(fixture.repo, 138, { timeoutMs: 1000 }); + const created = readPosts(fixture.postLog).filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/check-runs")); + const metadata = created.map(p => { const payload: unknown = parseJson(p.payload); assert.ok(isRecordLike(payload)); return parseJson(String(payload.external_id)); }); + assert.ok(isRecordLike(metadata[1])); + assert.equal(metadata[1].recoveryAttempt, undefined); + assert.equal(runnerInvocationCount(fixture), 4); +}); + +test("a newer incomplete old-head round preserves the completed cross-head baseline", async (t) => { + const fixture = setupFixture(t, { prNumber: 139, rawReview: defaultRawReview() }); + seedReviews(fixture.reviewsState, [{ id: 1, body: stateReviewBody(fixture.baseTipSha), user: { login: "github-actions[bot]", type: "Bot" } }]); + seedCompletedVerdictCheck(fixture, fixture.baseTipSha); + const checks: unknown = parseJson(readFileSync(fixture.checksState, "utf8")); + assert.ok(Array.isArray(checks) && isRecordLike(checks[0])); + checks.push({ ...checks[0], id: 2, status: "in_progress", conclusion: null, + external_id: JSON.stringify({ v: 2, prNumber: 139, roundId: "interrupted", runId: "later", recoveryAttempt: 0, recheck: true }) }); + writeFileSync(fixture.checksState, JSON.stringify(checks)); + await runGithub(fixture.repo, 139, { timeoutMs: 1000 }); + const posts = readPosts(fixture.postLog); + assert.equal(posts.filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/pulls/139/reviews")).length, 0); + assert.equal(posts.filter(p => p.args.includes("PUT") && p.args.includes("repos/frankekn/needlefish/pulls/139/reviews/1")).length, 2); + assert.equal(runnerInvocationCount(fixture), 2); +}); + + +for (const pin of ["4812", "9999", "", "bad", "0", "1.5", "9007199254740992"]) { + test(`custom App stored check recovery validates App ID pin ${pin}`, (t) => { + const fixture = setupFixture(t, { prNumber: 140, rawReview: defaultRawReview(), storedCheckPost5xx: true, customCheckAppId: 4812 }); + const old = process.env.NEEDLEFISH_CHECK_APP_ID; + process.env.NEEDLEFISH_CHECK_APP_ID = pin; + t.after(() => { if (old === undefined) delete process.env.NEEDLEFISH_CHECK_APP_ID; else process.env.NEEDLEFISH_CHECK_APP_ID = old; }); + const result = spawnGithubCli(fixture, 140); + const posts = readPosts(fixture.postLog); + const creates = posts.filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/check-runs")); + assert.equal(result.status, 1, result.stderr); + if (pin === "4812") { + assert.equal(creates.length, 1); + assert.equal(runnerInvocationCount(fixture), 2); + assert.equal(lastCheckCompletion(posts).output?.title, "Needlefish: changes_requested — bug"); + const marker = parseState(readReviewBodies(fixture.reviewsState).at(-1) ?? ""); + assert.ok(marker?.v === 2); + assert.equal(marker.delivery.appId, 4812); + } else if (pin === "9999" || pin === "") { + assert.equal(creates.length, 1); + assert.equal(runnerInvocationCount(fixture), 0); + assert.match(result.stderr, /ownership/); + } else { + assert.equal(creates.length, 0); + assert.equal(runnerInvocationCount(fixture), 0); + assert.match(result.stderr, /NEEDLEFISH_CHECK_APP_ID/); + } + }); +} + + +test("ambiguous duplicate check candidates never authorize completion", (t) => { + const fixture = setupFixture(t, { prNumber: 141, rawReview: defaultRawReview(), storedCheckPost5xx: true, duplicateStoredCheck: true }); + const result = spawnGithubCli(fixture, 141); + const posts = readPosts(fixture.postLog); + assert.equal(result.status, 1, result.stderr); + assert.match(result.stderr, /ownership/); + assert.equal(posts.filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/check-runs")).length, 1); + assert.equal(posts.filter(p => p.args.includes("PATCH")).length, 0); + assert.equal(runnerInvocationCount(fixture), 0); +}); + +test("a pinned App mismatch on successful create never authorizes model work or completion", (t) => { + const fixture = setupFixture(t, { prNumber: 142, rawReview: defaultRawReview(), customCheckAppId: 4812 }); + const old = process.env.NEEDLEFISH_CHECK_APP_ID; + process.env.NEEDLEFISH_CHECK_APP_ID = "9999"; + t.after(() => { if (old === undefined) delete process.env.NEEDLEFISH_CHECK_APP_ID; else process.env.NEEDLEFISH_CHECK_APP_ID = old; }); + const result = spawnGithubCli(fixture, 142); + const posts = readPosts(fixture.postLog); + assert.equal(result.status, 1, result.stderr); + assert.match(result.stderr, /no owned check id/); + assert.equal(posts.filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/check-runs")).length, 1); + assert.equal(posts.filter(p => p.args.includes("PATCH")).length, 0); + assert.equal(runnerInvocationCount(fixture), 0); +}); + +for (const priorHead of ["same", "previous"]) { +test(`changing the configured App creates a fresh review for ${priorHead} head receipt`, (t) => { + const fixture = setupFixture(t, { prNumber: 145, rawReview: defaultRawReview(), customCheckAppId: 4812, authorLogin: "private-review-app[bot]", failUserApi: true }); + seedReviews(fixture.reviewsState, [{ id: 1, body: stateReviewBody(priorHead === "same" ? fixture.headSha : fixture.baseTipSha), + user: { login: "github-actions[bot]", type: "Bot" } }]); + seedCompletedVerdictCheck(fixture, priorHead === "same" ? fixture.headSha : fixture.baseTipSha); + const old = process.env.NEEDLEFISH_CHECK_APP_ID; + process.env.NEEDLEFISH_CHECK_APP_ID = "4812"; + t.after(() => { if (old === undefined) delete process.env.NEEDLEFISH_CHECK_APP_ID; else process.env.NEEDLEFISH_CHECK_APP_ID = old; }); + const result = spawnGithubCli(fixture, 145); + assert.equal(result.status, 1, result.stderr); + assert.doesNotMatch(result.stdout, /"reason":"same_head"/); + assert.equal(runnerInvocationCount(fixture), 2); + const posts = readPosts(fixture.postLog); + assert.equal(posts.filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/pulls/145/reviews")).length, 1); + assert.equal(posts.filter(p => p.args.includes("PUT") && p.args.includes("repos/frankekn/needlefish/pulls/145/reviews/1")).length, 0); + assert.equal(lastCheckCompletion(readPosts(fixture.postLog)).output?.title, "Needlefish: changes_requested — bug"); + const marker = parseState(readReviewBodies(fixture.reviewsState).at(-1) ?? ""); + assert.ok(marker?.v === 2); + assert.equal(marker.delivery.appId, 4812); +}); +} + +test("an unpinned custom App check cannot start model work", (t) => { + const fixture = setupFixture(t, { prNumber: 146, rawReview: defaultRawReview(), customCheckAppId: 4812 }); + const old = process.env.NEEDLEFISH_CHECK_APP_ID; + delete process.env.NEEDLEFISH_CHECK_APP_ID; + t.after(() => { if (old === undefined) delete process.env.NEEDLEFISH_CHECK_APP_ID; else process.env.NEEDLEFISH_CHECK_APP_ID = old; }); + const result = spawnGithubCli(fixture, 146); + assert.equal(result.status, 1, result.stderr); + assert.match(result.stderr, /no owned check id/); + assert.equal(runnerInvocationCount(fixture), 0); + assert.equal(readPosts(fixture.postLog).filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/check-runs")).length, 1); +}); + +test("failure-comment pre-read and ambiguous POST failure never resend the comment", (t) => { + const fixture = setupFixture(t, { prNumber: 143, rawReview: defaultRawReview(), reviewPost404: true, failIssueCommentReads: true, storedIssuePost5xx: true }); + const result = spawnGithubCli(fixture, 143); + const posts = readPosts(fixture.postLog); + assert.equal(result.status, 1, result.stderr); + assert.equal(lastCheckCompletion(posts).output?.title, "Needlefish: review failed"); + assert.equal(posts.filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/issues/143/comments")).length, 1); + const comments: unknown = parseJson(readFileSync(fixture.issueCommentsState, "utf8")); + assert.ok(Array.isArray(comments) && isRecordLike(comments[0])); + assert.equal(comments.length, 1); + assert.match(String(comments[0].body), /needlefish-error/); + assert.equal(runnerInvocationCount(fixture), 2); +}); + +for (const postFailure of ["none", "stored", "not-stored"] as const) { + test(`review pre-read failure attempts one POST with ${postFailure} failure`, (t) => { + const fixture = setupFixture(t, { prNumber: 147, rawReview: defaultRawReview(), failReviewPreRead: true, + storedReviewPost5xx: postFailure === "stored", flakyReviewPosts: postFailure === "not-stored" ? 1 : 0 }); + const result = spawnGithubCli(fixture, 147); + const posts = readPosts(fixture.postLog); + assert.equal(result.status, 1, result.stderr); + const reviews = posts.filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/pulls/147/reviews")); + assert.equal(reviews.length, 1); + assert.equal(parseReviewPayload(reviews[0].payload).comments.length, 1); + assert.equal(runnerInvocationCount(fixture), 2); + assert.equal(readFileSync(fixture.reviewsState + ".reads", "utf8"), "2"); + assert.equal(lastCheckCompletion(posts).conclusion, "failure"); + assert.equal(lastCheckCompletion(posts).output?.title, postFailure === "none" ? "Needlefish: changes_requested — bug" : "Needlefish: review failed"); + if (postFailure === "none") { + assert.equal(parseState(readReviewBodies(fixture.reviewsState).at(-1) ?? "")?.v, 2); + assert.match(result.stdout, /"outcome":"verdict"/); + } else { + assert.match(result.stdout, /"outcome":"failure","operational":false/); + } + }); +} + +for (const unreadable of [false, true]) { + test(`cross-head receipt history ${unreadable ? "unreadable" : "available"} preserves prior inline delivery`, (t) => { + const fixture = setupFixture(t, { prNumber: 148, rawReview: defaultRawReview(), failPreviousHeadCheckReads: unreadable }); + seedReviews(fixture.reviewsState, [{ id: 1, body: stateReviewBody(fixture.baseTipSha), user: { login: "github-actions[bot]", type: "Bot" } }]); + seedCompletedVerdictCheck(fixture, fixture.baseTipSha); + const result = spawnGithubCli(fixture, 148); + const posts = readPosts(fixture.postLog); + assert.equal(result.status, 1, result.stderr); + assert.equal(posts.filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/pulls/148/reviews")).length, 0); + assert.equal(runnerInvocationCount(fixture), unreadable ? 0 : 2); + assert.equal(lastCheckCompletion(posts).conclusion, "failure"); + assert.equal(lastCheckCompletion(posts).output?.title, unreadable ? "Needlefish: review failed" : "Needlefish: changes_requested — bug"); + if (unreadable) { + assert.match(String(lastCheckCompletion(posts).output?.summary), /prior review delivery history is unreadable/i); + assert.match(result.stdout, /"outcome":"failure","operational":false/); + const comments = posts.filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/issues/148/comments")); + assert.equal(comments.length, 1); + assert.match(comments[0].payload, /needlefish-error/); + assert.equal(posts.filter(p => p.args.includes("PUT")).length, 0); + } else { + assert.equal(posts.filter(p => p.args.includes("PUT") && p.args.includes("repos/frankekn/needlefish/pulls/148/reviews/1")).length, 2); + } + }); +} + +for (const storedReviewPost5xx of [false, true]) { + test(`pinned custom App preserves its receipt with stored POST failure ${storedReviewPost5xx}`, (t) => { + const fixture = setupFixture(t, { + prNumber: 150, rawReview: defaultRawReview(), customCheckAppId: 4812, + authorLogin: "private-review-app[bot]", failUserApi: true, storedReviewPost5xx, + }); + const old = process.env.NEEDLEFISH_CHECK_APP_ID; + process.env.NEEDLEFISH_CHECK_APP_ID = "4812"; + t.after(() => { if (old === undefined) delete process.env.NEEDLEFISH_CHECK_APP_ID; else process.env.NEEDLEFISH_CHECK_APP_ID = old; }); + const first = spawnGithubCli(fixture, 150); + assert.equal(first.status, 1, first.stderr); + assert.equal(lastCheckCompletion(readPosts(fixture.postLog)).output?.title, "Needlefish: changes_requested — bug"); + const second = spawnGithubCli(fixture, 150); + assert.equal(second.status, 1, second.stderr); + assert.match(second.stdout, /"reason":"same_head"/); + assert.equal(runnerInvocationCount(fixture), 2); + assert.equal(readPosts(fixture.postLog).filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/pulls/150/reviews")).length, 1); + }); +} + +for (const mutation of ["actions-bot", "self-user", "other-bot", "user-type", "wrong-app", "missing-slug", "invalid-slug", "wrong-round"]) { + test(`pinned custom App rejects receipt identity ${mutation}`, (t) => { + const fixture = setupFixture(t, { prNumber: 151, rawReview: defaultRawReview(), customCheckAppId: 4812, + authorLogin: mutation === "self-user" ? "review-pat" : "private-review-app[bot]", failUserApi: mutation !== "self-user" }); + const old = process.env.NEEDLEFISH_CHECK_APP_ID; + process.env.NEEDLEFISH_CHECK_APP_ID = "4812"; + t.after(() => { if (old === undefined) delete process.env.NEEDLEFISH_CHECK_APP_ID; else process.env.NEEDLEFISH_CHECK_APP_ID = old; }); + assert.equal(spawnGithubCli(fixture, 151).status, 1); + const reviews: unknown = parseJson(readFileSync(fixture.reviewsState, "utf8")); + const checks: unknown = parseJson(readFileSync(fixture.checksState, "utf8")); + assert.ok(Array.isArray(reviews) && isRecordLike(reviews[0])); + assert.ok(Array.isArray(checks) && isRecordLike(checks[0])); + if (mutation === "actions-bot") reviews[0].user = { login: "github-actions[bot]", type: "Bot" }; + if (mutation === "self-user") reviews[0].user = { login: "review-pat", type: "User" }; + if (mutation === "other-bot") reviews[0].user = { login: "other-app[bot]", type: "Bot" }; + if (mutation === "user-type") reviews[0].user = { login: "private-review-app[bot]", type: "User" }; + if (mutation === "wrong-app") checks[0].app = { id: 9999, slug: "private-review-app" }; + if (mutation === "missing-slug") checks[0].app = { id: 4812 }; + if (mutation === "invalid-slug") checks[0].app = { id: 4812, slug: "private-review-app[bot]" }; + if (mutation === "wrong-round") checks[0].external_id = JSON.stringify({ v: 2, prNumber: 151, roundId: "other", runId: "" }); + writeFileSync(fixture.reviewsState, JSON.stringify(reviews)); + writeFileSync(fixture.checksState, JSON.stringify(checks)); + const result = spawnGithubCli(fixture, 151); + assert.equal(result.status, 1, result.stderr); + assert.doesNotMatch(result.stdout, /"reason":"same_head"/); + assert.equal(runnerInvocationCount(fixture), 4); + assert.equal(lastCheckCompletion(readPosts(fixture.postLog)).output?.title, "Needlefish: changes_requested — bug"); + }); +} + +test("pinned custom App unreadable receipt history fails with an owned terminal check", (t) => { + const fixture = setupFixture(t, { prNumber: 152, rawReview: defaultRawReview(), customCheckAppId: 4812, + authorLogin: "private-review-app[bot]", failUserApi: true, failCheckRunReadsOn: [2] }); + const old = process.env.NEEDLEFISH_CHECK_APP_ID; + process.env.NEEDLEFISH_CHECK_APP_ID = "4812"; + t.after(() => { if (old === undefined) delete process.env.NEEDLEFISH_CHECK_APP_ID; else process.env.NEEDLEFISH_CHECK_APP_ID = old; }); + assert.equal(spawnGithubCli(fixture, 152).status, 1); + const result = spawnGithubCli(fixture, 152); + assert.equal(result.status, 1, result.stderr); + assert.match(result.stderr, /Prior review identity is unreadable/); + assert.equal(runnerInvocationCount(fixture), 2); + assert.equal(readPosts(fixture.postLog).filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/pulls/152/reviews")).length, 1); + assert.equal(lastCheckCompletion(readPosts(fixture.postLog)).output?.title, "Needlefish: review failed"); +}); + +test("pinned custom App cannot reconcile an Actions-authored review POST", (t) => { + const fixture = setupFixture(t, { prNumber: 249, rawReview: defaultRawReview(), customCheckAppId: 4812, + authorLogin: "github-actions[bot]", failUserApi: true, storedReviewPost5xx: true }); + const old = process.env.NEEDLEFISH_CHECK_APP_ID; + process.env.NEEDLEFISH_CHECK_APP_ID = "4812"; + t.after(() => { if (old === undefined) delete process.env.NEEDLEFISH_CHECK_APP_ID; else process.env.NEEDLEFISH_CHECK_APP_ID = old; }); + const result = spawnGithubCli(fixture, 249); + assert.equal(result.status, 1, result.stderr); + const posts = readPosts(fixture.postLog); + assert.equal(posts.filter(p => p.args.includes("POST") && p.args.includes("repos/frankekn/needlefish/pulls/249/reviews")).length, 1); + assert.equal(lastCheckCompletion(posts).output?.title, "Needlefish: review failed"); + assert.match(result.stderr, /Cannot verify ownership of the stored review/); +}); + +function receiptRoundCheck(fixture: Fixture, id: number, conclusion: string, title: string, roundId = `round-${id}`): Record { + return { id, name: "Needlefish", head_sha: fixture.headSha, app: { id: 15368, slug: "github-actions" }, + external_id: JSON.stringify({ v: 2, prNumber: fixture.prNumber, roundId, runId: "fixture-run" }), + status: "completed", conclusion, output: { title } }; +} + for (const [conclusion, title] of [ - ["failure", "Needlefish: review terminated"], - ["failure", "Needlefish: review failed"], - ["neutral", "Needlefish: superseded"], - ["neutral", "Needlefish: needs_human"], + ["failure", "Needlefish: review terminated"], + ["failure", "Needlefish: review failed"], + ["neutral", "Needlefish: superseded"], + ["neutral", "Needlefish: needs_human"], ] as const) { - test(`a newer ${title} check is not covered by an older same-head pass`, async (t) => { - const fixture = setupFixture(t, { prNumber: 109, rawReview: defaultRawReview() }); - seedReviews(fixture.reviewsState, [{ id: 1, body: stateReviewBody(fixture.headSha), - user: { login: "github-actions[bot]", type: "Bot" } }]); - writeFileSync(fixture.checksState, JSON.stringify([ - { id: 1, status: "completed", conclusion: "success", output: { title: "Needlefish: pass" } }, - { id: 2, status: "completed", conclusion, output: { title } }, - ])); - await runGithub(fixture.repo, 109, { timeoutMs: 1000 }); - assert.ok(runnerInvocationCount(fixture) >= 1, "the newest completed check must decide"); - }); + test(`a newer ${title} check is not covered by an older same-head pass`, async (t) => { + const fixture = setupFixture(t, { prNumber: 109, rawReview: defaultRawReview() }); + seedReviews(fixture.reviewsState, [{ id: 1, body: stateReviewBody(fixture.headSha, 1), + user: { login: "github-actions[bot]", type: "Bot" } }]); + writeFileSync(fixture.checksState, JSON.stringify([ + receiptRoundCheck(fixture, 1, "success", "Needlefish: pass", "fixture-round"), + receiptRoundCheck(fixture, 2, conclusion, title), + ])); + await runGithub(fixture.repo, 109, { timeoutMs: 1000 }); + assert.ok(runnerInvocationCount(fixture) >= 1, "the newest completed check must decide"); + }); } test("a newer same-head pass still covers an older terminated check", async (t) => { - const fixture = setupFixture(t, { prNumber: 109, rawReview: defaultRawReview() }); - seedReviews(fixture.reviewsState, [{ id: 1, body: stateReviewBody(fixture.headSha), - user: { login: "github-actions[bot]", type: "Bot" } }]); - writeFileSync(fixture.checksState, JSON.stringify([ - { id: 1, status: "completed", conclusion: "failure", output: { title: "Needlefish: review terminated" } }, - { id: 2, status: "completed", conclusion: "success", output: { title: "Needlefish: pass" } }, - ])); - await runGithub(fixture.repo, 109, { timeoutMs: 1000 }); - assert.equal(runnerInvocationCount(fixture), 0, "a newer pass must still dedupe"); + const fixture = setupFixture(t, { prNumber: 109, rawReview: defaultRawReview() }); + seedReviews(fixture.reviewsState, [{ id: 1, body: stateReviewBody(fixture.headSha, 2), + user: { login: "github-actions[bot]", type: "Bot" } }]); + writeFileSync(fixture.checksState, JSON.stringify([ + receiptRoundCheck(fixture, 1, "failure", "Needlefish: review terminated"), + receiptRoundCheck(fixture, 2, "success", "Needlefish: pass", "fixture-round"), + ])); + await runGithub(fixture.repo, 109, { timeoutMs: 1000 }); + assert.equal(runnerInvocationCount(fixture), 0, "a newer pass must still dedupe"); }); diff --git a/src/adapters/github-termination.test.ts b/src/adapters/github-termination.test.ts index 22311d2a..be418c3f 100644 --- a/src/adapters/github-termination.test.ts +++ b/src/adapters/github-termination.test.ts @@ -136,9 +136,10 @@ function setupFixture(t: TestContext, opts: FixtureOptions): Fixture { ` setTimeout(() => { process.stdout.write(pr); process.exit(0); }, count > 0 ? ${opts.pullRereadDelayMs ?? 0} : 0);`, " return;", "}", + "if (args.some(a => /\\/commits\\/.*\\/check-runs/.test(a))) { process.stdout.write(JSON.stringify([{ check_runs: [] }])); process.exit(0); }", "if (args[1] === '--paginate') { process.stdout.write('[[]]'); process.exit(0); }", "if (args[1] === 'user') { process.stdout.write(JSON.stringify({ login: 'dev', type: 'User' })); process.exit(0); }", - `if (apiPath === ${JSON.stringify(`repos/${REPO}/check-runs`)} && method === 'POST') { process.stdout.write(JSON.stringify({ id: ${CHECK_ID} })); process.exit(0); }`, + `if (apiPath === ${JSON.stringify(`repos/${REPO}/check-runs`)} && method === 'POST') { process.stdout.write(JSON.stringify({ id: ${CHECK_ID}, app: { id: 15368, slug: 'github-actions' } })); process.exit(0); }`, "if (method === 'PATCH') {", ` const count = fs.existsSync(${JSON.stringify(patchCount)}) ? Number(fs.readFileSync(${JSON.stringify(patchCount)}, 'utf8')) : 0;`, ` fs.writeFileSync(${JSON.stringify(patchCount)}, String(count + 1));`, diff --git a/src/adapters/github.test.ts b/src/adapters/github.test.ts index 4e2a446f..ab25d415 100644 --- a/src/adapters/github.test.ts +++ b/src/adapters/github.test.ts @@ -57,7 +57,8 @@ test("runGithub normalizes relative repo paths before building prompts", async ( "const fs = require('node:fs');", "const args = process.argv.slice(2);", "if (args[0] !== 'api') process.exit(2);", - "if (args.includes('--input')) { fs.readFileSync(0, 'utf8'); process.stdout.write('{}'); process.exit(0); }", + "if (args.some(a => /\\/commits\\/.*\\/check-runs/.test(a))) { process.stdout.write(JSON.stringify([{ check_runs: [] }])); process.exit(0); }", + "if (args.includes('--input')) { fs.readFileSync(0, 'utf8'); process.stdout.write(JSON.stringify(args.includes('POST') && args.some(a => a.endsWith('/check-runs')) ? { id: 1, app: { id: 15368, slug: 'github-actions' } } : {})); process.exit(0); }", `if (args[1] === 'repos/frankekn/needlefish/pulls/7') { process.stdout.write(${JSON.stringify( JSON.stringify({ state: "open", @@ -208,7 +209,8 @@ test("runGithub reads a symlinked AGENTS.md as a Git blob, not the link target", "#!/usr/bin/env node", "const args = process.argv.slice(2);", "if (args[0] !== 'api') process.exit(2);", - "if (args.includes('--input')) { process.stdout.write('{}'); process.exit(0); }", + "if (args.some(a => /\\/commits\\/.*\\/check-runs/.test(a))) { process.stdout.write(JSON.stringify([{ check_runs: [] }])); process.exit(0); }", + "if (args.includes('--input')) { process.stdout.write(JSON.stringify(args.includes('POST') && args.some(a => a.endsWith('/check-runs')) ? { id: 1, app: { id: 15368, slug: 'github-actions' } } : {})); process.exit(0); }", `if (args[1] === 'repos/frankekn/needlefish/pulls/9') { process.stdout.write(${JSON.stringify( JSON.stringify({ state: "open", diff --git a/src/adapters/github.ts b/src/adapters/github.ts index 88cb0d80..ffb1a4a4 100644 --- a/src/adapters/github.ts +++ b/src/adapters/github.ts @@ -1,4 +1,5 @@ import path from "node:path"; +import { randomUUID } from "node:crypto"; import { setTimeout as sleep } from "node:timers/promises"; import { reviewExitCode } from "../core/verdict.js"; import { isDocsOnlyFastPath, review } from "../core/review.js"; @@ -393,10 +394,39 @@ export interface FindingKey { readonly title: string; } -export interface RoundState { - readonly v: 1; +interface DeliveryReceipt { + readonly checkId: number; + readonly appId: number; + readonly roundId: string; + readonly runId: string; +} + +export type RoundState = { readonly headSha: string; readonly findings: readonly FindingKey[]; +} & ({ readonly v: 1 } | { readonly v: 2; readonly delivery: DeliveryReceipt }); + +interface CheckRound { + readonly v: 2; + readonly prNumber: number; + readonly roundId: string; + readonly runId: string; + // Only adapter rounds write it; workflow receipts (author skip, caller retry) omit it. + readonly recheck?: boolean; +} + +function parseCheckRound(raw: unknown): CheckRound | null { + if (typeof raw !== "string") return null; + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return null; + } + if (!isRecord(parsed) || parsed.v !== 2 || typeof parsed.prNumber !== "number" || !Number.isSafeInteger(parsed.prNumber) || parsed.prNumber < 1 || typeof parsed.roundId !== "string" || + !parsed.roundId || typeof parsed.runId !== "string" || + (parsed.recheck !== undefined && typeof parsed.recheck !== "boolean")) return null; + return { v: 2, prNumber: parsed.prNumber, roundId: parsed.roundId, runId: parsed.runId, ...(parsed.recheck !== undefined ? { recheck: parsed.recheck } : {}) }; } function findingKey(f: Finding): FindingKey { @@ -411,20 +441,13 @@ function findingKey(f: Finding): FindingKey { export function renderState( headSha: string, findings: readonly Finding[], + delivery: DeliveryReceipt, ): string { - return renderStateKeys( - headSha, - findings.map(findingKey), - ); + return serializeState({ v: 2, headSha, findings: findings.map(findingKey), delivery }); } -function renderStateKeys( - headSha: string, - findings: readonly FindingKey[], -): string { - const state: RoundState = { v: 1, headSha, findings }; - // Escape ">" so a finding title like "x --> y" cannot terminate the HTML - // comment early and corrupt the marker (JSON.parse decodes \u003e back). +function serializeState(state: RoundState): string { + // Escape title text so it cannot terminate the HTML comment. const payload = JSON.stringify(state).replace(/>/g, "\\u003e"); return ``; } @@ -436,7 +459,7 @@ export function parseState(body: string): RoundState | null { if (start < 0) return null; const jsonStart = start + STATE_PREFIX.length; const end = body.indexOf("-->", jsonStart); - if (end < 0) return null; + if (end < 0 || body.slice(end + 3).trim() !== "") return null; let raw: unknown; try { raw = JSON.parse(body.slice(jsonStart, end)); @@ -444,7 +467,7 @@ export function parseState(body: string): RoundState | null { return null; } if (!isRecord(raw)) return null; - if (raw.v !== 1) return null; + if (raw.v !== 1 && raw.v !== 2) return null; const headSha = raw.headSha; if (typeof headSha !== "string") return null; const findingsRaw = raw.findings; @@ -463,7 +486,14 @@ export function parseState(body: string): RoundState | null { } keys.push({ file, lineStart, category, title }); } - return { v: 1, headSha, findings: keys }; + if (raw.v === 1) return { v: 1, headSha, findings: keys }; + const delivery = raw.delivery; + if (!isRecord(delivery) || typeof delivery.checkId !== "number" || !Number.isSafeInteger(delivery.checkId) || delivery.checkId < 1 || + typeof delivery.appId !== "number" || !Number.isSafeInteger(delivery.appId) || delivery.appId < 1 || + typeof delivery.roundId !== "string" || !delivery.roundId || typeof delivery.runId !== "string") return null; + return { v: 2, headSha, findings: keys, delivery: { + checkId: delivery.checkId, appId: delivery.appId, roundId: delivery.roundId, runId: delivery.runId, + } }; } // GitHub rejects a review or comment body over 65,536 characters with a 422 @@ -565,13 +595,13 @@ function fitStateMarker(state: RoundState, budget: number): string { while (lo < hi) { const mid = Math.ceil((lo + hi) / 2); if ( - renderStateKeys(state.headSha, state.findings.slice(0, mid)).length <= + serializeState({ ...state, findings: state.findings.slice(0, mid) }).length <= budget ) lo = mid; else hi = mid - 1; } - return renderStateKeys(state.headSha, state.findings.slice(0, lo)); + return serializeState({ ...state, findings: state.findings.slice(0, lo) }); } export interface MatchResult { @@ -784,6 +814,7 @@ function postReview( headSha: string, body: string, comments: readonly InlineComment[], + appBotLogin: string | null, ): number | null { const payload = JSON.stringify({ commit_id: headSha, @@ -796,16 +827,38 @@ function postReview( })), }); - const created = ghPost( - [ - "api", - "-X", - "POST", - `repos/${repo}/pulls/${prNumber}/reviews`, - "--input", - "-", - ], + const args = ["api", "-X", "POST", `repos/${repo}/pulls/${prNumber}/reviews`, "--input", "-"]; + let before: Set; + try { before = new Set(readReviews(repo, prNumber).map((item) => item.id)); } + catch { + const created = ghPost(args, payload); + return isRecord(created) && typeof created.id === "number" ? created.id : null; + } + const selfLogin = authenticatedLogin(); + const created = reconciledPost( + args, payload, + () => { + const matching = readReviews(repo, prNumber).filter((item) => !before.has(item.id) && + item.commit_id === headSha && item.body === screenAndCapOutboundBody(body)); + const candidate = matching.find((item) => isTrustedStateAuthor(item, selfLogin, appBotLogin)); + if (!candidate) { + if (matching.length > 0) throw new Error("Cannot verify ownership of the stored review; not retrying its POST"); + return null; + } + if (comments.length > 0) { + const raw = ghJson(["api", "--paginate", "--slurp", `repos/${repo}/pulls/${prNumber}/reviews/${String(candidate.id)}/comments`]); + if (!Array.isArray(raw)) throw new Error("Could not verify stored inline comments"); + const stored = raw.flat(1).filter(isRecord); + if (stored.length !== comments.length || !comments.every((comment) => stored.some((item) => + item.path === comment.path && item.line === comment.line && (item.start_line ?? undefined) === comment.start_line && + item.side === comment.side && + item.body === screenAndCapOutboundBody(comment.body) && isTrustedStateAuthor(item, selfLogin, appBotLogin)))) { + throw new Error("Review was stored but inline delivery is incomplete; rerun the review"); + } + } + return candidate; + }, ); // The id is needed later to append the dedupe state marker once delivery // has completed. A response without an id is tolerated: the body is @@ -813,22 +866,8 @@ function postReview( return isRecord(created) && typeof created.id === "number" ? created.id : null; } -// The state marker is the dedupe receipt: a same-head run skips only when a -// trusted review body carries it. It is written LAST — after the inline -// comments, the verdict label and the check run have all landed — so a failure -// anywhere in delivery can never leave a marker behind that makes the next run -// silently skip a head whose review never completed. -// -// Fail-soft on purpose: the verdict check is already posted, so a failed marker -// write must not turn a completed review red by falling through to the outer -// catch. The cost is one duplicate re-review (the safe direction, since the -// missing marker simply means "not delivered yet" to the next run). -// -// Legacy markers written before this ordering existed are still trusted and -// still parse (no format change), but they may have been written before their -// check run landed. The reconcile job covers exactly that: a head with no -// terminal non-infra check is re-dispatched as a workflow_dispatch, which sets -// NEEDLEFISH_RECHECK_INPUT=1 and so bypasses the same-head skip for one run. +// Marker writes fail soft because the review and verdict already reached GitHub. +// Reconciliation and same-head skipping require this receipt and its completed check. function persistStateMarker( repo: string, prNumber: number, @@ -873,18 +912,9 @@ function updateReviewBody( export function findPreviousReview( repo: string, prNumber: number, + expectedAppId: number | null = null, ): { id: number; state: RoundState } | null { - // --slurp: --paginate alone emits one JSON document PER PAGE (concatenated, - // unparseable); --slurp wraps the pages into a single outer array. - const raw = ghJson([ - "api", - "--paginate", - "--slurp", - `repos/${repo}/pulls/${prNumber}/reviews`, - ]); - if (!Array.isArray(raw)) return null; - // flat(1) also tolerates a plain review list from stubs or older gh versions. - const reviews = raw.flat(1); + const reviews = readReviews(repo, prNumber); // TRUST BOUNDARY. What this function returns decides two things in // runGithub: (a) the same-head dedupe, which returns *before* the model // run, the review post and the check-run post; and (b) the review id whose @@ -906,43 +936,72 @@ export function findPreviousReview( if (!state) continue; const id = item.id; if (typeof id !== "number") continue; - if (!isTrustedStateAuthor(item, selfLogin)) continue; + if (expectedAppId !== null) { + if (state.v !== 2 || state.delivery.appId !== expectedAppId || + nestedString(item, "user", "type") !== "Bot") continue; + const checks = readCheckRuns(repo, state.headSha); + if (completedCheckVerdict(state, checks, prNumber) === null) continue; + const check = checks.find((check) => check.id === state.delivery.checkId); + if (!isTrustedStateAuthor(item, selfLogin, customAppBotLogin(check?.app, expectedAppId))) continue; + } else if (!isTrustedStateAuthor(item, selfLogin)) continue; return { id, state }; } return null; } +// Explain trusts prior findings only from a delivered round: a v2 receipt whose +// owned check completed. Legacy and undelivered markers give it no context. +export function findDeliveredReview(repo: string, prNumber: number, expectedAppId: number | null): RoundState | null { + const previous = findPreviousReview(repo, prNumber, expectedAppId); + if (previous?.state.v !== 2) return null; + return deliveredVerdict(previous.state, readCheckRuns(repo, previous.state.headSha), prNumber, expectedAppId) !== null ? previous.state : null; +} + +function ownedCheckAppId(app: unknown, expectedAppId: number | null): number | null { + if (!isRecord(app) || typeof app.id !== "number" || !Number.isSafeInteger(app.id) || app.id <= 0) return null; + if (expectedAppId === null && app.slug !== "github-actions") return null; + if (expectedAppId !== null && app.id !== expectedAppId) return null; + return app.id; +} + +function customAppBotLogin(app: unknown, expectedAppId: number | null): string | null { + if (expectedAppId === null) return null; + if (ownedCheckAppId(app, expectedAppId) === null || !isRecord(app)) return ""; + const slug = stringField(app, "slug"); + return /^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$/.test(slug) ? `${slug}[bot]` : ""; +} + // S5a: create the check as in_progress BEFORE any model work so a PR always // shows whether a review is running on its latest head. All completions update -// this same check by id; a null id (creation failed) makes completions fall -// back to creating their own, preserving the old behavior. -function createPendingCheck(repo: string, headSha: string): number | null { +// this same check by id. An unknown id cannot authorize a completion. +function createPendingCheck(repo: string, headSha: string, round: CheckRound, expectedAppId: number | null): { id: number; appId: number; botLogin: string | null } | null { try { - const created = ghPost( + const created = reconciledPost( ["api", "-X", "POST", `repos/${repo}/check-runs`, "--input", "-"], JSON.stringify({ - name: "Needlefish", - head_sha: headSha, - status: "in_progress", - external_id: process.env.GITHUB_RUN_ID || undefined, - details_url: - process.env.GITHUB_SERVER_URL && - process.env.GITHUB_REPOSITORY && - process.env.GITHUB_RUN_ID - ? `${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/actions/runs/${process.env.GITHUB_RUN_ID}` - : undefined, + name: "Needlefish", head_sha: headSha, status: "in_progress", + external_id: JSON.stringify(round), + details_url: process.env.GITHUB_SERVER_URL && process.env.GITHUB_REPOSITORY && process.env.GITHUB_RUN_ID + ? `${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/actions/runs/${process.env.GITHUB_RUN_ID}` : undefined, }), + () => { + const matching = readCheckRuns(repo, headSha).filter((run) => run.head_sha === headSha && run.external_id === JSON.stringify(round)); + const owned = matching.filter((run) => run.name === "Needlefish" && ownedCheckAppId(run.app, expectedAppId) !== null); + if (owned.length === 1 && matching.length === 1) return owned[0]; + if (matching.length > 0) throw new Error("Cannot verify ownership of the stored check; not retrying its POST"); + return null; + }, ); - if (isRecord(created) && typeof created.id === "number") { - return created.id; + const appId = isRecord(created) ? ownedCheckAppId(created.app, expectedAppId) : null; + if (isRecord(created) && typeof created.id === "number" && Number.isSafeInteger(created.id) && created.id > 0 && appId !== null) { + return { id: created.id, appId, botLogin: customAppBotLogin(created.app, expectedAppId) }; } + throw new Error("GitHub returned no owned check id/app identity"); } catch (err) { const em = err instanceof Error ? err.message : String(err); - process.stderr.write( - `needlefish: could not create pending check: ${em}\n`, - ); + process.stderr.write(`needlefish: could not create pending check: ${em}\n`); + return null; } - return null; } type GhWrite = { readonly args: readonly string[]; readonly input: string }; @@ -1059,32 +1118,52 @@ function checkRunTitle(result: ReviewResult): string { return base; } -function postIssueComment(repo: string, prNumber: number, body: string): void { - ghPost( - [ - "api", - "-X", - "POST", - `repos/${repo}/issues/${prNumber}/comments`, - "--input", - "-", - ], - JSON.stringify({ body: screenAndCapOutboundBody(body) }), - ); +function postIssueComment(repo: string, prNumber: number, body: string, appBotLogin: string | null): void { + const endpoint = `repos/${repo}/issues/${prNumber}/comments`; + const read = (): JsonRecord[] => { + const raw = ghJson(["api", "--paginate", "--slurp", endpoint]); + if (!Array.isArray(raw)) throw new Error("GitHub returned no comment list"); + return raw.flat(1).filter(isRecord); + }; + const cappedBody = screenAndCapOutboundBody(body); + const args = ["api", "-X", "POST", endpoint, "--input", "-"]; + const payload = JSON.stringify({ body: cappedBody }); + let before: Set; + try { before = new Set(read().map((item) => item.id)); } + catch { + // A failed pre-read must not suppress the failure explanation. POST remains single-shot. + ghPost(args, payload); + return; + } + const selfLogin = authenticatedLogin(); + reconciledPost(args, payload, + () => { + const matching = read().filter((item) => !before.has(item.id) && item.body === cappedBody); + const owned = matching.find((item) => isTrustedStateAuthor(item, selfLogin, appBotLogin)); + if (owned) return owned; + if (matching.length > 0) throw new Error("Cannot verify ownership of the stored comment; not retrying its POST"); + return null; + }); +} + +class RecoveryLimitError extends Error { + constructor() { + super("Automatic review recovery limit reached. Delivery is incomplete; a plain rerun stops here again. Force a full review with the recheck input or comment `@needlefish recheck`."); + } } // S3: infra-failure comment posted alongside the red check so a red X always // has something to read in the PR timeline. Fail-soft: never blocks the check. -function postErrorComment(repo: string, prNumber: number, msg: string): void { +function postErrorComment(repo: string, prNumber: number, msg: string, advice = "Review did not complete. Re-run the review.", appBotLogin: string | null = null): void { const body = `⚠️ **Needlefish review FAILED TO RUN** — this red check is an infra failure, not a code verdict.\n\n` + "```\n" + `${msg.slice(0, 2000)}\n` + "```\n\n" + - "Re-trigger: push a new commit or re-run with --recheck.\n" + + `${advice}\n` + ""; try { - postIssueComment(repo, prNumber, body); + postIssueComment(repo, prNumber, body, appBotLogin); } catch (commentErr) { const cm = commentErr instanceof Error ? commentErr.message : String(commentErr); process.stderr.write(`needlefish: could not post error comment: ${cm}\n`); @@ -1123,8 +1202,9 @@ function postRoundComment( repo: string, prNumber: number, body: string, + appBotLogin: string | null, ): void { - postIssueComment(repo, prNumber, body); + postIssueComment(repo, prNumber, body, appBotLogin); } // GitHub usernames may contain the substring "github-actions"; @@ -1155,8 +1235,8 @@ const ACTIONS_BOT_LOGIN = "github-actions[bot]"; // boundary note in findPreviousReview for what rides on it. // // The rule is "the marker was written by the identity this run posts as", -// never "the marker was written by something bot-shaped". Two ways to -// establish that identity, both attested outside the author's control: +// never "the marker was written by something bot-shaped". Three ways to +// establish that identity, all attested outside the author's control: // // 1. login === selfLogin — the identity `gh api user` reports. Exact // equality only, never a prefix/substring, and never when selfLogin is @@ -1169,27 +1249,23 @@ const ACTIONS_BOT_LOGIN = "github-actions[bot]"; // cannot obtain type "Bot", and "[" / "]" are not legal in a human // username, so neither half is forgeable from a normal account. // +// 3. An explicit App pin replaces both defaults with its check API slug +// plus "[bot]", with user.type "Bot". A missing slug trusts nobody. +// Prior markers also need their matching completed check receipt. +// // Deliberately NOT trusted: // - a substring match on the login. That was the original bug: // "github-actions" is a substring of the registrable username // "z-github-actions-z", so any user could mint a trusted-looking marker. -// - any other Bot-typed "[bot]" account. A different GitHub App installed on +// - an unpinned Bot-typed "[bot]" account. A different GitHub App installed on // the repo is not us, and trusting it lets it suppress our review. // -// Consequence, accepted: a deployment that authenticates as a custom GitHub -// App (login "[bot]", and `GET /user` also unavailable to installation -// tokens) matches neither branch, so its markers are not trusted and every run -// re-reviews and posts a new review instead of updating. That is noisy but it -// is the safe direction — an unrecognized author costs an extra review, never -// a suppressed one. -export function isTrustedStateAuthor(item: JsonRecord, selfLogin: string): boolean { +export function isTrustedStateAuthor(item: JsonRecord, selfLogin: string, appBotLogin: string | null = null): boolean { const login = nestedString(item, "user", "login"); if (login === "") return false; + if (appBotLogin !== null) return login === appBotLogin && nestedString(item, "user", "type") === "Bot"; if (selfLogin !== "" && login === selfLogin) return true; - return ( - login === ACTIONS_BOT_LOGIN && - nestedString(item, "user", "type") === "Bot" - ); + return login === ACTIONS_BOT_LOGIN && nestedString(item, "user", "type") === "Bot"; } // Self-hosted runners often authenticate gh with a PAT, so Needlefish's own @@ -1423,52 +1499,76 @@ function skipReasonForPr(pr: unknown, prNumber: number, headSha: string): SkipRe return null; } -// Dedupe is a delivery gate, not a "we tried" gate. A same-head marker alone -// is not proof the head ever reached a verdict: any run that failed after -// posting the review body — a check-run POST/PATCH error, an inline-comment -// error, a crash — used to leave a marker behind. So the skip additionally -// requires a completed, non-infra Needlefish check run for that exact head: -// the same terminal state the reconcile job looks for. A marker with no such -// check (or only an infra-failure check, or a check still in progress) reruns. -// The newest completed check decides: any newer completed non-verdict check -// (failed, terminated, superseded, skipped or legacy neutral) is not covered by -// an older verdict on the same head. In-progress checks are ignored, so an -// orphaned in_progress check (runner killed before termination handling) does -// not stop the skip; recover it with --recheck (#102). -// -// Fail-closed on an unreadable check list: an extra review is the safe error, -// a silently skipped one is not. -function completedCheckVerdict(repo: string, headSha: string): Verdict | null { - let raw: unknown; - try { - raw = ghJson([ - "api", - `repos/${repo}/commits/${headSha}/check-runs?check_name=Needlefish&filter=all&per_page=100`, - ]); - } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - process.stderr.write( - `needlefish: could not read check runs for ${headSha}: ${msg}; re-reviewing instead of skipping.\n`, - ); - return null; - } - if (!isRecord(raw)) return null; - const runs = raw.check_runs; - if (!Array.isArray(runs)) return null; - const runId = (run: JsonRecord): number => typeof run.id === "number" ? run.id : 0; - const entries = runs.filter(isRecord).sort((a, b) => runId(b) - runId(a)); - for (const run of entries) { - if (stringField(run, "status") !== "completed") continue; - const conclusion = stringField(run, "conclusion"); - const title = nestedString(run, "output", "title"); - if (conclusion === "success" && title === "Needlefish: pass") return "pass"; - if (conclusion === "failure" && title.startsWith("Needlefish: needs_human")) return "needs_human"; - if (conclusion === "failure" && title.startsWith("Needlefish: changes_requested")) return "changes_requested"; - return null; +function readCheckRuns(repo: string, headSha: string): JsonRecord[] { + const raw = ghJson(["api", "--paginate", "--slurp", + `repos/${repo}/commits/${headSha}/check-runs?check_name=Needlefish&filter=all&per_page=100`]); + const pages = Array.isArray(raw) ? raw : [raw]; + const runs: JsonRecord[] = []; + for (const page of pages) { + if (!isRecord(page) || !Array.isArray(page.check_runs)) throw new Error("GitHub returned no check runs"); + runs.push(...page.check_runs.filter(isRecord)); } + return runs.sort((a, b) => Number(b.id) - Number(a.id)); +} + +function completedCheckVerdict(state: RoundState, checks: readonly JsonRecord[], prNumber: number): Verdict | null { + if (state.v !== 2) return null; + const receipt = state.delivery; + const run = checks.find((check) => check.id === receipt.checkId); + if (!run || run.head_sha !== state.headSha || !isRecord(run.app) || run.app.id !== receipt.appId || + run.name !== "Needlefish" || run.status !== "completed") return null; + const round = parseCheckRound(run.external_id); + if (!round || round.prNumber !== prNumber || round.roundId !== receipt.roundId || round.runId !== receipt.runId) return null; + const conclusion = stringField(run, "conclusion"); + const title = nestedString(run, "output", "title"); + if (conclusion === "success" && title === "Needlefish: pass") return "pass"; + if (conclusion !== "failure") return null; + if (title.startsWith("Needlefish: needs_human")) return "needs_human"; + if (title.startsWith("Needlefish: changes_requested")) return "changes_requested"; return null; } +function deliveredVerdict(state: RoundState, checks: readonly JsonRecord[], prNumber: number, expectedAppId: number | null): Verdict | null { + if (state.v !== 2) return null; + const receipt = state.delivery; + const owned = checks.some((check) => check.id === receipt.checkId && ownedCheckAppId(check.app, expectedAppId) === receipt.appId); + return owned ? completedCheckVerdict(state, checks, prNumber) : null; +} + +// Any newer owned round for this PR and head invalidates the receipt, including +// in_progress rounds (orphaned or concurrent). This replaces #257's "ignore +// in_progress" rule; the recovery budget, not the skip, bounds the rerun (#102). +function hasNewerRound(state: RoundState, checks: readonly JsonRecord[], prNumber: number): boolean { + if (state.v !== 2) return false; + const receipt = state.delivery; + return checks.some((check) => typeof check.id === "number" && check.id > receipt.checkId && + check.head_sha === state.headSha && isRecord(check.app) && check.app.id === receipt.appId && + check.name === "Needlefish" && parseCheckRound(check.external_id)?.prNumber === prNumber); +} + +function reconciledPost(args: readonly string[], input: string, read: () => unknown | null): unknown { + try { + return ghPost(args, input); + } catch (error) { + if (!isTransientGh5xx(error)) throw error; + for (let attempt = 1; attempt <= GH_POST_ATTEMPTS; attempt++) { + const existing = read(); + if (existing !== null) return existing; + if (attempt < GH_POST_ATTEMPTS) { + const delay = ghPostRetryBaseMs() * attempt; + if (delay > 0) Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, delay); + } + } + throw error; + } +} + +function readReviews(repo: string, prNumber: number): JsonRecord[] { + const raw = ghJson(["api", "--paginate", "--slurp", `repos/${repo}/pulls/${prNumber}/reviews`]); + if (!Array.isArray(raw)) throw new Error("GitHub returned no review list"); + return raw.flat(1).filter(isRecord); +} + // A job timeout or watchdog terminates the process while the owned check is // in_progress, and a consumer workflow has nothing else that completes it. // Runs inside the termination grace, off the event loop. Same stale-head rule @@ -1537,12 +1637,22 @@ async function completeTerminatedCheck( await ghJsonWhileTerminating(termination, write.args, input); } +export function configuredCheckAppId(): number | null { + const appIdInput = process.env.NEEDLEFISH_CHECK_APP_ID || undefined; + const expectedAppId = appIdInput === undefined ? null : Number(appIdInput); + if (appIdInput !== undefined && (!/^[1-9]\d*$/.test(appIdInput) || !Number.isSafeInteger(expectedAppId))) { + throw new Error("NEEDLEFISH_CHECK_APP_ID must be a positive safe integer"); + } + return expectedAppId; +} + export async function runGithub( cwd: string, prNumber: number, opts: RunnerOptions = {}, recheck = false, ): Promise { + const expectedAppId = configuredCheckAppId(); const repoPath = path.resolve(cwd); const repo = process.env.GITHUB_REPOSITORY; if (!repo) throw new Error("GITHUB_REPOSITORY not set (must run in Actions)"); @@ -1588,24 +1698,29 @@ export async function runGithub( nestedString(pr, "head", "sha") || git(["rev-parse", "HEAD"], repoPath); - // Same-head dedupe: fetch the previous review once, reuse for both the - // short-circuit and the post-review posting logic. A same-head marker only - // skips when the head also carries a completed Needlefish verdict check — - // the marker is the review-body receipt, the check is the delivery proof. - const prev = findPreviousReview(repo, prNumber); - if (prev && prev.state.headSha === headSha && !recheck) { - const verdict = completedCheckVerdict(repo, headSha); - if (verdict !== null) { - process.exitCode = reviewExitCode(verdict); - process.stderr.write( - `needlefish: head ${headSha} already reviewed; pass --recheck to force.\n`, - ); - emitSkip("same_head", prNumber, headSha); - return; - } - process.stderr.write( - `needlefish: head ${headSha} has a state marker but no completed Needlefish verdict check; re-reviewing.\n`, - ); + let previous: ReturnType | undefined; + try { previous = findPreviousReview(repo, prNumber, expectedAppId); } + catch (error) { + process.stderr.write(`needlefish: could not read prior review identity: ${error instanceof Error ? error.message : String(error)}\n`); + } + let checks: JsonRecord[] | null = null; + try { checks = readCheckRuns(repo, headSha); } + catch (error) { + process.stderr.write(`needlefish: could not read delivery checks: ${error instanceof Error ? error.message : String(error)}\n`); + } + let previousChecks = previous?.state.headSha === headSha ? checks : null; + if (previous && previous.state.headSha !== headSha) { + try { previousChecks = readCheckRuns(repo, previous.state.headSha); } + catch { previousChecks = null; } + } + const previousVerdict = previous && previousChecks ? deliveredVerdict(previous.state, previousChecks, prNumber, expectedAppId) : null; + const prev = previousVerdict !== null ? previous : null; + if (prev && previousVerdict !== null && prev.state.headSha === headSha && !recheck && + checks && !hasNewerRound(prev.state, checks, prNumber)) { + process.exitCode = reviewExitCode(previousVerdict); + process.stderr.write(`needlefish: head ${headSha} already reviewed; pass --recheck to force.\n`); + emitSkip("same_head", prNumber, headSha); + return; } const baseSha = process.env.PR_BASE_SHA || nestedString(pr, "base", "sha"); @@ -1648,7 +1763,9 @@ export async function runGithub( }); const resolvedOpts = resolveConnectionOptions(opts, repoPath, process.env, !isDocsOnlyFastPath(bundle)); - const pendingCheckId = createPendingCheck(repo, headSha); + const round: CheckRound = { v: 2, prNumber, roundId: randomUUID(), runId: process.env.GITHUB_RUN_ID || "", recheck }; + const pending = createPendingCheck(repo, headSha, round, expectedAppId); + const pendingCheckId = pending?.id ?? null; // The owned check reaches a terminal state exactly once, and whoever holds // the claim completes it. The result path claims before posting anything // and, if a post then throws, still completes the check from the catch. @@ -1672,6 +1789,24 @@ export async function runGithub( ); try { + if (!pending) throw new Error("Review delivery could not start; rerun the review"); + if (previous === undefined) throw new Error("Prior review identity is unreadable; rerun the review"); + if (!checks) throw new Error("Review delivery history is unreadable; rerun the review"); + if (previous && !previousChecks) throw new Error("Prior review delivery history is unreadable; rerun the review"); + const delivery: DeliveryReceipt = { checkId: pending.id, appId: pending.appId, roundId: round.roundId, runId: round.runId }; + const deliveredCheckId = prev?.state.v === 2 && prev.state.headSha === headSha ? prev.state.delivery.checkId : null; + const priorRounds: CheckRound[] = []; + for (const check of checks) { + if (deliveredCheckId !== null && typeof check.id === "number" && check.id <= deliveredCheckId) continue; + if (!isRecord(check.app) || check.app.id !== pending.appId || check.head_sha !== headSha || check.name !== "Needlefish") continue; + const prior = parseCheckRound(check.external_id); + // A workflow receipt is not a review attempt, so it spends no recovery budget. + if (!prior || prior.prNumber !== prNumber || prior.recheck === undefined) continue; + priorRounds.push(prior); + if (prior.recheck) break; + } + const recoveryAttempt = recheck ? 0 : deliveredCheckId !== null ? priorRounds.length : Math.max(priorRounds.length, previous?.state.headSha === headSha ? 1 : 0); + if (recoveryAttempt > 1) throw new RecoveryLimitError(); // Scope and base-tip fields are attached after review(): anything on // the bundle reaches the model via {{BUNDLE}}, so human-facing surface // data lives only on the result and the prompt stays byte-identical. @@ -1762,6 +1897,7 @@ export async function runGithub( open, renderOpts.newCount, ), + pending.botLogin, ); } catch (roundErr) { const rm = roundErr instanceof Error ? roundErr.message : String(roundErr); @@ -1770,7 +1906,7 @@ export async function runGithub( ); } if (freshComments.length > 0) { - postReview(repo, prNumber, headSha, "", freshComments); + postReview(repo, prNumber, headSha, "", freshComments, pending.botLogin); } await applyVerdictLabel(repo, prNumber, result.verdict); const summary = renderMarkdown(freshResult, renderOpts); @@ -1789,7 +1925,7 @@ export async function runGithub( prev.id, renderMarkdown(freshResult, { ...renderOpts, - stateMarker: renderState(headSha, result.findings), + stateMarker: renderState(headSha, result.findings, delivery), omitCoverageGaps: true, }), ); @@ -1813,7 +1949,7 @@ export async function runGithub( ...renderOpts, omitCoverageGaps: true, }); - const reviewId = postReview(repo, prNumber, headSha, body, comments); + const reviewId = postReview(repo, prNumber, headSha, body, comments, pending.botLogin); await applyVerdictLabel(repo, prNumber, result.verdict); const summary = renderMarkdown(result, renderOpts); postCheck( @@ -1831,7 +1967,7 @@ export async function runGithub( reviewId, renderMarkdown(result, { ...renderOpts, - stateMarker: renderState(headSha, result.findings), + stateMarker: renderState(headSha, result.findings, delivery), omitCoverageGaps: true, }), ); @@ -1878,7 +2014,7 @@ export async function runGithub( // the timeline — no verdict, no error comment — for a stale head. emitSkip(skipReason, prNumber, headSha); try { - postCheck( + if (pendingCheckId !== null) postCheck( repo, headSha, null, @@ -1899,7 +2035,7 @@ export async function runGithub( // failure of either GitHub endpoint must not suppress the other, nor // the stderr line and exit code below. try { - postCheck( + if (pendingCheckId !== null) postCheck( repo, headSha, null, @@ -1916,7 +2052,7 @@ export async function runGithub( ); } // S3: post a PR comment so a red X always has something to read. - if (!staleUnknown) postErrorComment(repo, prNumber, msg); + if (!staleUnknown) postErrorComment(repo, prNumber, msg, err instanceof RecoveryLimitError ? "A plain rerun stops here again. Force a full review with the recheck input or comment `@needlefish recheck`." : undefined, pending?.botLogin); } process.stderr.write(`needlefish review failed: ${msg}\n`); process.exitCode = 1; diff --git a/src/shared/connection-auth-integration.test.ts b/src/shared/connection-auth-integration.test.ts index 59bf54bf..9ca2ef24 100644 --- a/src/shared/connection-auth-integration.test.ts +++ b/src/shared/connection-auth-integration.test.ts @@ -312,10 +312,11 @@ const fs = require('node:fs'); const args = process.argv.slice(2); if (args.includes('--input')) { fs.appendFileSync(${JSON.stringify(writes)}, fs.readFileSync(0,'utf8') + '\\n'); - process.stdout.write('{"id":1}'); + process.stdout.write('{"id":1,"app":{"id":15368,"slug":"github-actions"}}'); } else if (args[1] === 'repos/frankekn/needlefish/pulls/7') { process.stdout.write(${JSON.stringify(JSON.stringify({ state: "open", author_association: "MEMBER", user: { login: "author", type: "User" }, head: { sha: f.head }, base: { sha: f.base } }))}); } else if (args[1] === 'user') process.stdout.write('{"login":"github-actions[bot]"}'); +else if (args.some(a => a.includes('/check-runs?'))) process.stdout.write('[{"check_runs":[]}]'); else if (args.includes('--paginate')) process.stdout.write('[[]]'); else process.stdout.write('[]'); `); diff --git a/src/shared/connections-integration.test.ts b/src/shared/connections-integration.test.ts index ddc0a359..5c29b8fb 100644 --- a/src/shared/connections-integration.test.ts +++ b/src/shared/connections-integration.test.ts @@ -106,11 +106,12 @@ process.stdout.write(${JSON.stringify(response)}); writeFileSync(path.join(bin, "gh"), `#!/usr/bin/env node const args = process.argv.slice(2); if (args[0] === 'api') { - if (args.includes('--input')) { require('node:fs').readFileSync(0, 'utf8'); process.stdout.write('{"id":1}'); } + if (args.includes('--input')) { require('node:fs').readFileSync(0, 'utf8'); process.stdout.write('{"id":1,"app":{"id":15368,"slug":"github-actions"}}'); } else if (args[1] === 'repos/frankekn/needlefish/pulls/7') process.stdout.write(${JSON.stringify(JSON.stringify({ state: options.github === "closed" ? "closed" : "open", head: { sha: head }, base: { sha: base }, author_association: options.github === "untrusted" ? "NONE" : "MEMBER", user: { login: "author", type: "User" }, }))}); + else if (args.some(a => a.includes('/check-runs?'))) process.stdout.write('[{"check_runs":[]}]'); else if (args.includes('--paginate')) process.stdout.write('[[]]'); else if (args[1] === 'user') process.stdout.write('{"login":"github-actions[bot]"}'); else process.exit(2);