diff --git a/sdk/typescript/src/cli.ts b/sdk/typescript/src/cli.ts index 33bf8aca..a4aa8c0e 100644 --- a/sdk/typescript/src/cli.ts +++ b/sdk/typescript/src/cli.ts @@ -1132,6 +1132,7 @@ interface CliDependencies { deduplicateScan?: typeof deduplicateScanInternal; publishFindingsCsvToCloud?: typeof publishFindingsCsvToCloud; publishScanToCloud?: typeof publishScanToCloud; + cloudFetch?: (url: string, options: RequestInit) => Promise; publishScanToCustom?: typeof publishScanToCustom; confirmPatchReview?: (question: string) => Promise; patchEditor?: ( @@ -2139,6 +2140,21 @@ export async function main( output: z.record(z.string(), z.unknown()).optional(), async run({ args, format, formatExplicit, options }) { const controller = new AbortController(); + let cloudRequestStarted = false; + const cloudFetch = ( + url: string, + options: RequestInit, + ): Promise => { + options.signal?.throwIfAborted(); + cloudRequestStarted = true; + return (dependencies.cloudFetch ?? globalThis.fetch)(url, options); + }; + const publicationErrorMessage = (error: unknown): string => + cloudRequestStarted && + controller.signal.aborted && + error === controller.signal.reason + ? "Any upload already in flight may have been accepted. Check Cloud before retrying." + : safeErrorMessage(error); let presentation: PublicationProgressPresenter | undefined; let firstSignalAt = 0; let observingSignals = false; @@ -2182,9 +2198,9 @@ export async function main( ? "Publication canceled by Ctrl-C." : "Publication terminated by SIGTERM."; const recovery = - error === undefined || error === signal + error === undefined || (error === signal && !cloudRequestStarted) ? "" - : ` ${diagnosticValue(safeErrorMessage(error))}`; + : ` ${diagnosticValue(publicationErrorMessage(error))}`; errorOutput.write(`codex-security: ${reason}${recovery}\n`); exitCode = signal === "SIGINT" ? 130 : 143; return true; @@ -2290,6 +2306,7 @@ export async function main( environment: dependencies.environment, dryRun: options.dryRun, signal: controller.signal, + fetch: cloudFetch, }); return { ...result }; } @@ -2533,6 +2550,7 @@ export async function main( break; } cloudBatch.notAttempted.shift(); + cloudRequestStarted = false; try { const result = await ( dependencies.publishScanToCloud ?? publishScanToCloud @@ -2540,11 +2558,12 @@ export async function main( environment: dependencies.environment, dryRun: options.dryRun, signal: controller.signal, + fetch: cloudFetch, ...(scanId === undefined ? {} : { expectedScanId: scanId }), }); cloudBatch.results.push({ scanDir: directory, ...result }); } catch (error) { - const message = safeErrorMessage(error); + const message = publicationErrorMessage(error); cloudBatch.failed.push({ scanDir: directory, ...(scanId === undefined ? {} : { scanId }), @@ -2565,6 +2584,7 @@ export async function main( environment: dependencies.environment, dryRun: options.dryRun, signal: controller.signal, + fetch: cloudFetch, ...(selectedScans[0]?.scanId === undefined ? {} : { expectedScanId: selectedScans[0].scanId }), diff --git a/sdk/typescript/tests-ts/cli-cloud-publish.test.ts b/sdk/typescript/tests-ts/cli-cloud-publish.test.ts index 803c5ad9..c021114d 100644 --- a/sdk/typescript/tests-ts/cli-cloud-publish.test.ts +++ b/sdk/typescript/tests-ts/cli-cloud-publish.test.ts @@ -1,8 +1,21 @@ -import { mkdir, mkdtemp, realpath, rm, symlink } from "node:fs/promises"; +import { + chmod, + cp, + mkdir, + mkdtemp, + realpath, + rm, + symlink, + writeFile, +} from "node:fs/promises"; import { homedir, tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { afterEach, describe, expect, test } from "bun:test"; import { main } from "../src/cli.js"; +import { + publishFindingsCsvToCloud, + publishScanToCloud, +} from "../src/cloud-publish.js"; import type { JsonObject } from "../src/index.js"; import { capture, @@ -10,6 +23,7 @@ import { FakeSignals, SYNTHETIC_CREDENTIALS, } from "./cli-fixtures.js"; +import { PLUGIN_ROOT } from "./plugin-root.js"; const receipt = { scanId: "scan-1", @@ -342,15 +356,18 @@ describe("publish scan to Cloud", () => { uploads++; return receipt; }; + const stderr = capture(); expect( await main( ["publish", "scan", "--to", "cloud"], capture().stream, - capture().stream, + stderr.stream, deps, ), ).toBe(130); expect(uploads).toBe(0); + expect(stderr.text()).toContain("Publication canceled"); + expect(stderr.text()).not.toMatch(/accepted|retry/i); expect( [...signals.listeners.values()].every( (listeners) => listeners.size === 0, @@ -474,6 +491,7 @@ describe("publish scan to Cloud", () => { environment: deps.environment, dryRun, signal: expect.any(AbortSignal), + fetch: expect.any(Function), }); const { scanDir: _, ...result } = results[calls.length]!; calls.push(directory); @@ -631,6 +649,191 @@ describe("publish scan to Cloud", () => { }, ); + test.each([ + ["single preflight", "preflight", false], + ["single request", "request", false], + ["single receipt", "receipt", false], + ["batch preflight", "preflight", true], + ["batch request", "request", true], + ["batch receipt", "receipt", true], + ] as const)( + "handles cancellation from the real Cloud publisher: %s", + async (_scenario, stage, batch) => { + for (const [signal, code] of [ + ["SIGINT", 130], + ["SIGTERM", 143], + ] as const) { + const root = await realpath( + await mkdtemp(join(tmpdir(), "cloud-publication-cancel-")), + ); + temporaryDirectories.push(root); + const credentialHome = join(root, "credentials"); + await mkdir(credentialHome, { mode: 0o700 }); + await writeFile( + join(credentialHome, "auth.json"), + JSON.stringify({ + auth_mode: "chatgpt", + tokens: { + access_token: "synthetic-access-token", + account_id: "synthetic-account", + }, + }), + { mode: 0o600 }, + ); + await writeFile( + join(credentialHome, "config.toml"), + 'cli_auth_credentials_store = "file"\n', + ); + const scanDirectories = await Promise.all( + ["scan-one", "scan-two"].map(async (name) => { + const directory = join(root, name); + await cp( + join(PLUGIN_ROOT, "examples", "completed-scan"), + directory, + { + recursive: true, + }, + ); + if (process.platform !== "win32") await chmod(directory, 0o700); + return directory; + }), + ); + const directories = batch + ? [...scanDirectories, join(root, "not-attempted")] + : [scanDirectories[0]!]; + const signals = new FakeSignals(); + const deps = dependencies({ + signals, + currentDirectory: root, + environment: { + CODEX_HOME: credentialHome, + CODEX_SECURITY_STATE_DIR: join(root, "state"), + }, + }); + let requests = 0; + deps.cloudFetch = async (_url, request) => { + requests++; + if (batch && requests === 1) { + return Response.json({ + status: "accepted", + finding_ids: ["accepted-finding"], + finding_count: 1, + }); + } + expect(request.signal).toBeInstanceOf(AbortSignal); + if (stage === "request") { + signals.emit(signal); + throw request.signal!.reason; + } + return new Response( + new ReadableStream({ + pull(body) { + signals.emit(signal); + body.error(request.signal!.reason); + }, + }), + { status: 201 }, + ); + }; + let publications = 0; + deps.publishScanToCloud = (directory, options) => { + const result = publishScanToCloud(directory, options); + publications++; + if (stage === "preflight" && publications === (batch ? 2 : 1)) { + signals.emit(signal); + } + return result; + }; + const stdout = capture(); + const stderr = capture(); + expect( + await main( + [ + "publish", + "scan", + ...directories.flatMap((directory) => ["--scan-dir", directory]), + "--to", + "cloud", + "--json", + ], + stdout.stream, + stderr.stream, + deps, + ), + ).toBe(code); + expect(requests).toBe( + (batch ? 1 : 0) + (stage === "preflight" ? 0 : 1), + ); + if (batch) { + expect(JSON.parse(stdout.text())).toEqual({ + results: [ + expect.objectContaining({ + scanDir: directories[0], + findingIds: ["accepted-finding"], + }), + ], + failed: [ + { + scanDir: directories[1], + error: + stage === "preflight" + ? signal + : expect.stringMatching(/accepted.*check.*retry/i), + }, + ], + notAttempted: [directories[2]], + }); + } else { + expect(stdout.text()).toBe(""); + } + if (stage === "preflight") { + expect(stderr.text()).not.toMatch(/accepted|retry/i); + } else { + expect(stderr.text()).toMatch(/accepted.*check.*retry/i); + } + } + }, + ); + + test("cancels during CSV reading without suggesting an upload was accepted", async () => { + const root = await mkdtemp(join(tmpdir(), "cloud-csv-cancel-")); + temporaryDirectories.push(root); + const csv = join(root, "findings.csv"); + await writeFile(csv, ""); + const signals = new FakeSignals(); + const deps = dependencies({ + signals, + currentDirectory: root, + environment: { + CODEX_HOME: join(root, "credentials"), + CODEX_SECURITY_STATE_DIR: join(root, "state"), + }, + }); + let requests = 0; + deps.cloudFetch = async () => { + requests++; + throw new Error("unexpected upload"); + }; + deps.publishFindingsCsvToCloud = (path, options) => { + const result = publishFindingsCsvToCloud(path, options); + signals.emit("SIGINT"); + return result; + }; + const stdout = capture(); + const stderr = capture(); + expect( + await main( + ["publish", "scan", "--csv", csv, "--to", "cloud"], + stdout.stream, + stderr.stream, + deps, + ), + ).toBe(130); + expect(requests).toBe(0); + expect(stdout.text()).toBe(""); + expect(stderr.text()).not.toMatch(/accepted|retry/i); + }); + test("rejects multiple scans for Linear before publishing any findings", async () => { const deps = dependencies(); let calls = 0; @@ -699,6 +902,7 @@ describe("publish scan to Cloud", () => { environment: deps.environment, dryRun, signal: expect.any(AbortSignal), + fetch: expect.any(Function), }); return result; }; @@ -1034,6 +1238,27 @@ describe("publish scan to Cloud", () => { ).toBe(true); }); + test("does not suggest retrying an upload when a Cloud dry run is canceled", async () => { + const signals = new FakeSignals(); + const deps = dependencies({ signals }); + deps.publishScanToCloud = async (_directory, options) => { + signals.emit("SIGINT"); + throw options!.signal!.reason; + }; + const stdout = capture(); + const stderr = capture(); + expect( + await main( + ["publish", "scan", "completed-scan", "--to", "cloud", "--dry-run"], + stdout.stream, + stderr.stream, + deps, + ), + ).toBe(130); + expect(stdout.text()).toBe(""); + expect(stderr.text()).not.toMatch(/accepted|retry/i); + }); + test("aborts Cloud publication without activating Linear recovery signal handling", async () => { for (const [signal, code] of [ ["SIGINT", 130], diff --git a/sdk/typescript/tests-ts/patch-tui.test.ts b/sdk/typescript/tests-ts/patch-tui.test.ts index 58dd67f7..59a2424b 100644 --- a/sdk/typescript/tests-ts/patch-tui.test.ts +++ b/sdk/typescript/tests-ts/patch-tui.test.ts @@ -3,8 +3,9 @@ import { spawnSync } from "node:child_process"; import { mkdir, mkdtemp, rm, symlink, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { setImmediate as nextTurn } from "node:timers/promises"; import { cleanup, render } from "ink-testing-library"; -import { createElement } from "react"; +import { act, createElement } from "react"; import type { Finding, SeverityLevel } from "../src/index.js"; import { PatchTui, type PatchSelection } from "../src/patch-tui.js"; import { fakeResult } from "./cli-fixtures.js"; @@ -80,8 +81,22 @@ function findings(severities: readonly SeverityLevel[]): Finding[] { return result.findings.findings; } -async function settle(): Promise { - await new Promise((resolve) => setTimeout(resolve, 60)); +async function press( + app: ReturnType, + input: string, +): Promise { + const environment = globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }; + const previous = environment.IS_REACT_ACT_ENVIRONMENT; + environment.IS_REACT_ACT_ENVIRONMENT = true; + try { + await act(async () => { + app.stdin.write(input); + await nextTurn(); + }); + } finally { + if (previous === undefined) delete environment.IS_REACT_ACT_ENVIRONMENT; + else environment.IS_REACT_ACT_ENVIRONMENT = previous; + } } describe("interactive patch finding browser", () => { @@ -112,11 +127,9 @@ describe("interactive patch finding browser", () => { expect(app.lastFrame()).not.toContain('"rationale"'); const frames = [app.lastFrame() ?? ""]; - app.stdin.write("\t"); - await settle(); + await press(app, "\t"); for (let page = 0; page < 12; page += 1) { - app.stdin.write("\u001B[6~"); - await settle(); + await press(app, "\u001B[6~"); frames.push(app.lastFrame() ?? ""); } @@ -200,8 +213,7 @@ describe("interactive patch finding browser", () => { const frames = [app.lastFrame() ?? ""]; for (let page = 0; page < 12; page += 1) { - app.stdin.write("\u001B[6~"); - await settle(); + await press(app, "\u001B[6~"); frames.push(app.lastFrame() ?? ""); } const reviewed = frames.join("\n"); @@ -234,18 +246,15 @@ describe("interactive patch finding browser", () => { }), ); - app.stdin.write("2"); - await settle(); + await press(app, "2"); expect(app.lastFrame()).toContain("1/3 selected"); expect(app.lastFrame()).toContain("high and above"); - app.stdin.write("\u001B[B "); - await settle(); + await press(app, "\u001B[B "); expect(app.lastFrame()).toContain("2/3 selected"); expect(app.lastFrame()).toContain("custom"); - app.stdin.write("\r"); - await settle(); + await press(app, "\r"); expect(selected).toEqual([ { severity: "medium", occurrenceIds: ["occ_1", "occ_2"] }, ]); @@ -262,36 +271,27 @@ describe("interactive patch finding browser", () => { }), ); - app.stdin.write("i"); - await settle(); + await press(app, "i"); expect(app.lastFrame()).toContain("Enter save"); - app.stdin.write("Use the shared 2FA helper, not a new dependency."); - await settle(); + await press(app, "Use the shared 2FA helper, not a new dependency."); expect(app.lastFrame()).toContain("Use the shared 2FA helper"); expect(app.lastFrame()).toContain("2/2 selected"); - app.stdin.write("\r"); - await settle(); + await press(app, "\r"); expect(app.lastFrame()).toContain("PATCH INSTRUCTIONS"); expect(app.lastFrame()).toContain("Use the shared 2FA helper"); expect(app.lastFrame()).toContain("✎"); expect(app.lastFrame()?.match(/PATCH INSTRUCTIONS/gu)).toHaveLength(1); - app.stdin.write("\u001B[B"); - await settle(); - app.stdin.write("i"); - await settle(); - app.stdin.write("Keep the existing middleware."); - await settle(); - app.stdin.write("\r"); - await settle(); + await press(app, "\u001B[B"); + await press(app, "i"); + await press(app, "Keep the existing middleware."); + await press(app, "\r"); expect(app.lastFrame()).toContain("Keep the existing middleware."); - app.stdin.write(" "); - await settle(); - app.stdin.write("\r"); - await settle(); + await press(app, " "); + await press(app, "\r"); expect(selected).toEqual([ { @@ -318,13 +318,11 @@ describe("interactive patch finding browser", () => { expect(app.lastFrame()).toContain( "[ ] Create draft GitHub pull request after patching", ); - app.stdin.write("r"); - await settle(); + await press(app, "r"); expect(app.lastFrame()).toContain( "[✓] Create draft GitHub pull request after patching", ); - app.stdin.write("\r"); - await settle(); + await press(app, "\r"); expect(selected).toEqual([ { @@ -346,25 +344,17 @@ describe("interactive patch finding browser", () => { }), ); - app.stdin.write("i"); - await settle(); - app.stdin.write("Discard this guidance."); - await settle(); - app.stdin.write("\u001B"); - await settle(); + await press(app, "i"); + await press(app, "Discard this guidance."); + await press(app, "\u001B"); expect(selected).toEqual([]); expect(app.lastFrame()).not.toContain("Discard this guidance."); - app.stdin.write("i"); - await settle(); - app.stdin.write("x"); - await settle(); - app.stdin.write("\u007F"); - await settle(); - app.stdin.write("\r"); - await settle(); - app.stdin.write("\r"); - await settle(); + await press(app, "i"); + await press(app, "x"); + await press(app, "\u007F"); + await press(app, "\r"); + await press(app, "\r"); expect(selected).toEqual([{ severity: "high", occurrenceIds: ["occ_1"] }]); }); @@ -381,12 +371,10 @@ describe("interactive patch finding browser", () => { }), ); if (input === "\r") { - app.stdin.write("n"); - await settle(); + await press(app, "n"); expect(app.lastFrame()).toContain("0/1 selected"); } - app.stdin.write(input); - await settle(); + await press(app, input); expect(selected).toEqual([null]); app.unmount(); }