diff --git a/scripts/build-diff-data.mjs b/scripts/build-diff-data.mjs index 795e802..37c0fe7 100644 --- a/scripts/build-diff-data.mjs +++ b/scripts/build-diff-data.mjs @@ -439,6 +439,15 @@ function bareCache(remoteUrl) { return { path, run }; } +class RemoteConnectionError extends Error {} + +function remoteLookupError(message, error) { + const detail = error?.stderr?.toString().trim() || ''; + const transient = /(?:failed to connect|couldn't connect|could not resolve host|could not resolve hostname|temporary failure in name resolution|connection (?:timed out|reset|refused|closed)|network is unreachable|operation timed out|i\/o timeout|TLS connection was non-properly terminated|error connecting to|HTTP 50[0234]|requested URL returned error: 50[0234])/i.test(detail); + const ErrorType = transient ? RemoteConnectionError : Error; + return new ErrorType(message); +} + function fetchInto(cache, remoteUrl, refspecs) { try { runGit( @@ -456,8 +465,9 @@ function fetchInto(cache, remoteUrl, refspecs) { ); } catch (error) { const detail = error?.stderr?.toString().trim(); - throw new Error( + throw remoteLookupError( `Could not fetch the remote target${detail ? `: ${detail}` : ''}`, + error, ); } } @@ -484,8 +494,8 @@ function remoteDefaultBranchInfo(remoteUrl) { let raw; try { raw = runGit(['ls-remote', '--symref', remoteUrl, 'HEAD'], { remoteUrl }); - } catch { - throw new Error('Could not read the remote default branch'); + } catch (error) { + throw remoteLookupError('Could not read the remote default branch', error); } const match = raw.match(/^ref:\s+refs\/heads\/([^\t\n]+)\s+HEAD$/m); if (!match) { @@ -659,8 +669,9 @@ function pullRequestInfo(pr, remote) { ) ? ' Check gh auth status.' : ''; - throw new Error( + throw remoteLookupError( `Could not read pull request ${pr} with gh${detail ? `: ${detail}` : ''}.${authHint}`, + error, ); } } @@ -1467,13 +1478,21 @@ function fingerprint() { ].join('|'); } -const refresh = () => { +let remoteRefreshFailed = false; +const refresh = ({ retainSnapshot = false } = {}) => { try { const wrote = build(); + if (remoteRefreshFailed) console.error('Remote refresh recovered.'); + remoteRefreshFailed = false; console.log(wrote ? `Wrote ${output}` : 'No diff-data changes'); return true; } catch (error) { console.error(error.message); + if (retainSnapshot && error instanceof RemoteConnectionError) { + remoteRefreshFailed = true; + console.error('Keeping the last valid review; it has not been refreshed. Will retry the remote refresh.'); + return true; + } process.exitCode = 1; return false; } @@ -1504,10 +1523,11 @@ if (watching && started) { const next = fingerprint(); remoteWait += watchInterval; const remoteDue = remoteMode && remoteWait >= remoteRefreshInterval; + if (remoteRefreshFailed && !remoteDue) return true; if (next !== last || remoteDue || watchContent) { last = next; remoteWait = 0; - if (!refresh()) { + if (!refresh({ retainSnapshot: true })) { clearInterval(watcher); return false; } diff --git a/tests/presenter-recovery.test.mjs b/tests/presenter-recovery.test.mjs index cc565b1..2ce6816 100644 --- a/tests/presenter-recovery.test.mjs +++ b/tests/presenter-recovery.test.mjs @@ -61,14 +61,14 @@ function stop(child) { }); } -function present(repo, summaries, output, codex, environment = {}) { +function present(repo, summaries, output, codex, environment = {}, target = ['--worktree']) { return spawn( process.execPath, [ script, '--repo', repo, - '--worktree', + ...target, '--agent', 'codex', '--summaries', @@ -240,3 +240,114 @@ process.stdout.write(JSON.stringify(response)); await rm(root, { recursive: true, force: true }); } }); + + +test('serves completed notes and lets an active agent finish during a remote outage', async () => { + const root = await mkdtemp(join(tmpdir(), 'diffsplain-remote-recovery-')); + const repo = await makeRepo(root, ['changed.txt']); + const remote = join(root, 'remote.git'); + const bin = join(root, 'bin'); + const failure = join(root, 'offline'); + const release = join(root, 'release-agent'); + const active = join(root, 'agent-active'); + const calls = join(root, 'calls.jsonl'); + const summaries = join(root, 'notes.json'); + const output = join(root, 'snapshot.json'); + const codex = join(root, 'codex.mjs'); + let presenter; + let logs = ''; + try { + git(repo, 'branch', '-M', 'main'); + git(repo, 'switch', '-qc', 'feature'); + git(repo, 'commit', '-qam', 'feature'); + execFileSync('git', ['init', '--bare', '-q', remote]); + git(repo, 'remote', 'add', 'origin', remote); + git(repo, 'push', '-q', 'origin', 'main', 'feature'); + git(repo, 'switch', '-q', 'main'); + await mkdir(bin); + await writeFile(join(bin, 'git'), `#!/usr/bin/env node +const { existsSync } = require('node:fs'); +const { spawnSync } = require('node:child_process'); +if (process.argv.includes('fetch') && existsSync(${JSON.stringify(failure)})) { + process.stderr.write('fatal: Failed to connect to github.com port 443'); + process.exit(128); +} +const result = spawnSync('git', process.argv.slice(2), { + env: { ...process.env, PATH: process.env.RECOVERY_REAL_PATH }, stdio: 'inherit', +}); +process.exit(result.status ?? 1); +`); + await chmod(join(bin, 'git'), 0o755); + await writeFile(codex, `#!/usr/bin/env node +import { appendFileSync, existsSync, readFileSync, writeFileSync } from 'node:fs'; +const input = JSON.parse(readFileSync(0, 'utf8')); +const paths = input.files.map((file) => file.path); +appendFileSync(${JSON.stringify(calls)}, JSON.stringify(paths) + '\\n'); +if (!paths.length) { + writeFileSync(${JSON.stringify(active)}, 'active'); + while (!existsSync(${JSON.stringify(release)})) { + await new Promise((resolve) => setTimeout(resolve, 25)); + } +} +process.stdout.write(JSON.stringify({ + change: { title: 'Feature', summary: 'Updates text.', why: 'Tests recovery.', highlights: [], risks: [] }, + files: paths.map((path) => ({ path, title: 'Updated text', what: 'Changes text.', why: 'Tests recovery.', details: [], risks: [] })), +})); +`); + await chmod(codex, 0o755); + presenter = present(repo, summaries, output, codex, { + PATH: `${bin}:${process.env.PATH}`, + RECOVERY_REAL_PATH: process.env.PATH, + XDG_CACHE_HOME: join(root, 'cache'), + XDG_CONFIG_HOME: join(root, 'config'), + DIFFSPLAIN_WATCH_INTERVAL_MS: '50', + DIFFSPLAIN_REMOTE_REFRESH_INTERVAL_MS: '200', + }, ['--branch', 'feature', '--base', 'main']); + presenter.stdout.on('data', (chunk) => { logs += chunk; }); + presenter.stderr.on('data', (chunk) => { logs += chunk; }); + const ready = await waitFor(() => { + const line = logs.split('\n').find((value) => value.startsWith('{"event":"ready"')); + return line && JSON.parse(line); + }); + const url = new URL('diff-data.json', ready.url); + url.searchParams.set('access', ready.access); + const served = async () => { + const response = await fetch(url); + assert.equal(response.status, 200); + return response.json(); + }; + await waitFor(() => readFile(active, 'utf8')); + const before = await served(); + assert.equal(before.files[0].noteReady, true); + assert.equal(before.notes.status, 'generating'); + await writeFile(failure, 'offline'); + await waitFor(() => (logs.match(/Keeping the last valid review/g) || []).length >= 2); + assert.equal(presenter.exitCode, null, logs); + assert.deepEqual(await served(), before); + await writeFile(release, 'finish'); + const completed = await waitFor(async () => { + const snapshot = await served(); + return snapshot.notes.complete && snapshot; + }); + assert.equal(completed.files[0].noteReady, true); + const callsBefore = await readFile(calls, 'utf8'); + const notesBefore = await readFile(summaries, 'utf8'); + await rm(failure); + await waitFor(() => logs.includes('Remote refresh recovered')); + const recovered = await served(); + assert.deepEqual(recovered.files, completed.files); + assert.deepEqual(recovered.change, completed.change); + assert.deepEqual(recovered.notes, completed.notes); + assert.deepEqual(recovered.repo, completed.repo); + assert.equal(await readFile(calls, 'utf8'), callsBefore); + assert.equal(await readFile(summaries, 'utf8'), notesBefore); + const stopped = await stop(presenter); + assert.equal(stopped.code, 0); + await assert.rejects(fetch(url)); + } catch (error) { + throw new Error(`${error.message}\n${logs}`, { cause: error }); + } finally { + await stopIfRunning(presenter); + await rm(root, { recursive: true, force: true }); + } +}); diff --git a/tests/remote-targets.test.mjs b/tests/remote-targets.test.mjs index 2733ec5..805f51b 100644 --- a/tests/remote-targets.test.mjs +++ b/tests/remote-targets.test.mjs @@ -34,7 +34,7 @@ function run(repo, args, options = {}) { ); } -async function proxyRemote(fixture, remoteUrl) { +async function proxyRemote(fixture, remoteUrl, failurePath) { const bin = join(fixture.root, "git-proxy"); const proxy = join(bin, "git"); await mkdir(bin); @@ -42,6 +42,16 @@ async function proxyRemote(fixture, remoteUrl) { proxy, `#!/usr/bin/env node const { spawnSync } = require("node:child_process"); +const { existsSync, readFileSync, appendFileSync } = require("node:fs"); +const failurePath = ${JSON.stringify(failurePath) || "undefined"}; +if (failurePath && existsSync(failurePath)) { + const failure = JSON.parse(readFileSync(failurePath, "utf8")); + if (process.argv.includes(failure.command)) { + appendFileSync(failurePath + ".attempts", Date.now() + "\\n"); + process.stderr.write(failure.message); + process.exit(128); + } +} const args = process.argv.slice(2).map((arg) => arg === ${JSON.stringify(remoteUrl)} ? ${JSON.stringify(fixture.remote)} @@ -1344,3 +1354,101 @@ test("rejects conflicting remote target flags", async () => { await rm(fixture.root, { recursive: true, force: true }); } }); + +async function remoteWatchTargetArgs(fixture, target, failure) { + if (!target.startsWith("pr")) return ["--branch", "feature"]; + const gh = join(fixture.root, "git-proxy", "gh"); + await writeFile(gh, `#!/usr/bin/env node +const { existsSync, readFileSync, appendFileSync } = require("node:fs"); +const failurePath = ${JSON.stringify(failure)}; +if (existsSync(failurePath)) { + const failure = JSON.parse(readFileSync(failurePath, "utf8")); + if (failure.command === "gh") { + appendFileSync(failurePath + ".attempts", Date.now() + "\\n"); + process.stderr.write(failure.message); + process.exit(1); + } +} +process.stdout.write(JSON.stringify({ + number: 7, title: "Feature", url: "https://github.com/example/project/pull/7", + baseRefName: "main", baseRefOid: ${JSON.stringify(fixture.mainOid)}, + headRefName: "feature", headRefOid: ${JSON.stringify(fixture.featureOid)} +})); +`); + await chmod(gh, 0o755); + execFileSync("git", ["--git-dir", fixture.remote, "update-ref", "refs/pull/7/head", fixture.featureOid]); + return ["--pr", "7"]; +} + +for (const [target, failureMessage] of [ + ["branch fetch", "fatal: Failed to connect to github.com port 443: Couldn't connect to server"], + ["branch ls-remote", "fatal: Could not resolve host: github.com"], + ["pr fetch", "fatal: The requested URL returned error: 500"], + ["pr gh", "HTTP 500: Internal Server Error (https://api.github.com/graphql)"], +]) { + test(`retains the snapshot and retries after transient ${target} failures`, async () => { + const fixture = await makeRemoteRepo(); + const output = join(fixture.root, "watch.json"); + const failure = join(fixture.root, "failure.json"); + const remoteUrl = "https://github.com/example/project.git"; + let watched; + try { + const env = await proxyRemote(fixture, remoteUrl, failure); + git(fixture.repo, "remote", "set-url", "origin", remoteUrl); + const args = [ + ...await remoteWatchTargetArgs(fixture, target, failure), + "--cache-dir", join(fixture.root, "cache"), "--watch", "--output", output, + ]; + watched = startWatcher(fixture.repo, args, { env }); + await waitForSnapshot(output, watched, (value) => value.repo.head === fixture.featureOid); + const original = await readFile(output, "utf8"); + await writeFile(failure, JSON.stringify({ + command: target.split(" ")[1], + message: failureMessage, + })); + await waitFor(() => watched.logs().includes("Keeping the last valid review") || watched.child.exitCode !== null); + assert.equal(watched.child.exitCode, null, watched.logs()); + assert.match(watched.logs(), /Keeping the last valid review.*retry/i); + await waitFor(async () => (await readFile(`${failure}.attempts`, "utf8")).trim().split("\n").length >= 3); + const attempts = (await readFile(`${failure}.attempts`, "utf8")).trim().split("\n").map(Number); + assert.ok(attempts[2] - attempts[0] >= 150, "retries respect the remote refresh interval"); + assert.equal(await readFile(output, "utf8"), original); + await rm(failure); + await waitFor(() => watched.logs().includes("Remote refresh recovered")); + assert.equal(await readFile(output, "utf8"), original); + if (!target.startsWith("pr")) { + const head = await publishFeatureUpdate(fixture, "recovered.txt", "recovered\n"); + await waitForSnapshot(output, watched, (value) => value.repo.head === head); + } + await writeFile(failure, JSON.stringify({ command: "fetch", message: "fatal: couldn't find remote ref refs/heads/feature" })); + await waitFor(() => watched.child.exitCode !== null); + assert.equal(watched.child.exitCode, 1, "permanent target errors still stop the watcher"); + } finally { + await stopIfRunning(watched); + await rm(fixture.root, { recursive: true, force: true }); + } + }); +} + +test("fails initial transient remote lookup without serving an old snapshot", async () => { + const fixture = await makeRemoteRepo(); + const output = join(fixture.root, "old.json"); + const failure = join(fixture.root, "failure.json"); + const remoteUrl = "https://github.com/example/project.git"; + try { + const env = await proxyRemote(fixture, remoteUrl, failure); + git(fixture.repo, "remote", "set-url", "origin", remoteUrl); + await writeFile(output, '{"old":true}\n'); + await writeFile(failure, JSON.stringify({ command: "fetch", message: "fatal: Failed to connect to github.com port 443" })); + const result = spawnSync(process.execPath, [script, "--repo", fixture.repo, + "--branch", "feature", "--watch", "--cache-dir", join(fixture.root, "cache"), "--output", output], + { env, encoding: "utf8", timeout: 10_000 }); + assert.equal(result.status, 1); + assert.match(result.stderr, /Could not fetch the remote target/); + assert.doesNotMatch(result.stdout, /Wrote|No diff-data changes/); + assert.doesNotMatch(result.stderr, /Keeping the last valid review/); + assert.equal(await readFile(output, "utf8"), '{"old":true}\n'); + } finally { + await rm(fixture.root, { recursive: true, force: true }); + } +});