diff --git a/src/commands/auth.ts b/src/commands/auth.ts index de1eda4..5b3e274 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -191,7 +191,8 @@ export async function runWhoami(opts: CommonOptions, deps: AuthDeps = {}): Promi // displayed URL always matches where requests actually go (dogfood L1788). let resolvedEndpoint: string; if (opts.dryRun) { - resolvedEndpoint = opts.endpointUrl ?? env.TESTSPRITE_API_URL ?? 'https://api.testsprite.com'; + const envApiUrl = env.TESTSPRITE_API_URL?.trim() || undefined; + resolvedEndpoint = opts.endpointUrl ?? envApiUrl ?? 'https://api.testsprite.com'; } else { const credentialsPath = deps.credentialsPath ?? defaultCredentialsPath(); const config = loadConfig({ diff --git a/src/lib/bundle.commit.test.ts b/src/lib/bundle.commit.test.ts new file mode 100644 index 0000000..a832096 --- /dev/null +++ b/src/lib/bundle.commit.test.ts @@ -0,0 +1,64 @@ +import type * as NodeFsPromises from 'node:fs/promises'; +import { mkdirSync, mkdtempSync, readdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const renameMock = vi.hoisted(() => vi.fn()); + +vi.mock('node:fs/promises', async importOriginal => { + const actual = (await importOriginal()) as typeof NodeFsPromises; + return { + ...actual, + rename: renameMock, + }; +}); + +const { commitBundle } = await import('./bundle.js'); + +describe('commitBundle', () => { + let realRename: NodeFsPromises.rename; + + beforeEach(async () => { + const actual = (await vi.importActual('node:fs/promises')) as typeof NodeFsPromises; + realRename = actual.rename; + renameMock.mockImplementation(realRename); + }); + + afterEach(() => { + renameMock.mockReset(); + }); + + it('rolls back to the prior complete bundle when the staging swap fails', async () => { + const parent = mkdtempSync(join(tmpdir(), 'bundle-commit-parent-')); + const dir = join(parent, 'bundle'); + const tmpDir = join(dir, '.tmp'); + + mkdirSync(join(dir, 'steps'), { recursive: true }); + writeFileSync(join(dir, 'meta.json'), '{"snapshotId":"snap_old"}\n', 'utf8'); + writeFileSync(join(dir, 'steps', '01-evidence.json'), '{"step":1}\n', 'utf8'); + + mkdirSync(join(tmpDir, 'steps'), { recursive: true }); + writeFileSync(join(tmpDir, 'meta.json'), '{"snapshotId":"snap_new"}\n', 'utf8'); + writeFileSync(join(tmpDir, 'result.json'), '{}\n', 'utf8'); + writeFileSync(join(tmpDir, 'steps', '01-evidence.json'), '{"step":9}\n', 'utf8'); + + renameMock.mockImplementation(async (oldPath, newPath) => { + const src = String(oldPath); + const dest = String(newPath); + if (src.includes('.staging.') && !dest.includes('.prior.')) { + throw Object.assign(new Error('simulated atomic swap failure'), { code: 'EACCES' }); + } + return realRename(oldPath, newPath); + }); + + await expect(commitBundle(tmpDir, dir)).rejects.toThrow('simulated atomic swap failure'); + + expect(readFileSync(join(dir, 'meta.json'), 'utf8')).toBe('{"snapshotId":"snap_old"}\n'); + expect(readFileSync(join(dir, 'steps', '01-evidence.json'), 'utf8')).toBe('{"step":1}\n'); + const leftovers = readdirSync(parent).filter( + name => name.includes('.staging.') || name.includes('.prior.'), + ); + expect(leftovers).toEqual([]); + }); +}); \ No newline at end of file diff --git a/src/lib/bundle.ts b/src/lib/bundle.ts index d50948c..fa795bb 100644 --- a/src/lib/bundle.ts +++ b/src/lib/bundle.ts @@ -11,9 +11,11 @@ * `snapshotId`. We refuse a forged response where * `bundle.snapshotId !== result.snapshotId` or steps disagree on * `runIdIfAvailable` (`assertContextIntegrity`). - * 2. **Atomic on disk** — we write to `/.tmp/...` first and - * `rename()` each file into place. `meta.json` is renamed last so - * the presence of `/meta.json` ⇔ "bundle is complete and + * 2. **Atomic on disk** — we write to `/.tmp/...` first, then + * swap the complete staging tree into place via directory rename. + * The prior bundle is moved aside and only deleted after the swap + * succeeds, so a failed re-commit never destroys a complete bundle. + * `meta.json` in the final tree ⇔ "bundle is complete and * self-consistent". * 3. **`.partial` on failure** — any download or fs failure leaves * a `/.partial` marker (with `requestId`, error summary, @@ -33,12 +35,13 @@ * when the agent re-runs. M3 may add resume. */ -import { mkdir, mkdtemp, readdir, rename, rm, stat, unlink, writeFile } from 'node:fs/promises'; +import { randomUUID } from 'node:crypto'; +import { mkdir, mkdtemp, rename, rm, stat, unlink, writeFile } from 'node:fs/promises'; import type { Writable } from 'node:stream'; import { pipeline } from 'node:stream/promises'; import type { ReadableStream as NodeReadableStream } from 'node:stream/web'; import { createWriteStream } from 'node:fs'; -import { dirname, isAbsolute, join, resolve } from 'node:path'; +import { basename, dirname, isAbsolute, join, resolve } from 'node:path'; import type { CliFailureContext, CliTestStep } from '../commands/test.js'; import { ApiError, TransportError } from './errors.js'; import type { FetchImpl } from './http.js'; @@ -465,10 +468,10 @@ export async function writeBundle( await writeFile(join(tmpDir, 'meta.json'), JSON.stringify(meta, null, 2) + '\n', 'utf8'); filesWritten.push('meta.json'); - // Atomic rename: move every file from /.tmp/* into /*. - // Steps subdir gets renamed wholesale; meta.json renames last so - // its visibility implies "bundle is complete and atomic on disk." - await commitBundle(tmpDir, dir, filesWritten); + // Atomic directory swap: the complete bundle in /.tmp/ replaces + // only after the staging tree is fully built. A failed swap + // rolls back to the prior bundle instead of leaving meta.json gone. + await commitBundle(tmpDir, dir); return { meta, dir, files: filesWritten }; } catch (err) { @@ -492,83 +495,43 @@ async function freshTmpDir(dir: string): Promise { } /** - * Rename `/` → `/` for every file in `files`. + * Atomically replace `` with the complete bundle staged in `tmpDir`. * - * Critical ordering for atomicity (the §3 "agent-safe" contract): - * - * 1. **Remove the OLD `meta.json` first.** The bundle's completion - * signal is `meta.json`'s presence; an agent reading `` while - * we're mutating it must see "no meta → bundle absent or - * mid-write" rather than "meta points at a snapshot that's already - * been partially overwritten." Removing the old meta is what - * makes the rest of the swap safe to do in place. - * 2. Wipe stale top-level files (e.g. an old `video.mp4` when the new - * bundle has no video). Without this, a fresh bundle could ship - * with a stale video lingering at the top level. - * 3. Replace `/steps/` wholesale. - * 4. Rename top-level files into place. - * 5. **Rename `meta.json` LAST.** Its visible presence is the atomic - * completion signal; until step 5 lands, agents see "incomplete." - * - * The window between (1) and (5) is bounded by a handful of `rename` - * syscalls — small enough that a SIGKILL there is rare, and any agent - * caught reading the dir during it sees no meta and refuses to consume - * (per §7.3). That's what we want. + * `tmpDir` already contains the full §7 layout (including `meta.json`). + * We move it to a sibling staging directory, swap it into place via + * directory rename, and only delete the prior bundle after the swap + * succeeds. If the swap fails, the prior bundle is restored so a + * re-commit never leaves `meta.json` deleted with no replacement. */ -async function commitBundle( - tmpDir: string, - dir: string, - files: ReadonlyArray, -): Promise { - // (1) Remove the prior bundle's completion signal FIRST. - await unlink(join(dir, 'meta.json')).catch(() => undefined); - - // (2) Sweep stale top-level files that the new bundle won't write. - // If the prior run wrote `video.mp4` and the new run has no video, - // an in-place rename leaves the old video lingering. Enumerate - // current top-level entries and remove anything that isn't being - // freshly renamed in. - const topLevel = files.filter(f => !f.startsWith('steps/')); - const newTopLevelSet = new Set(topLevel); - newTopLevelSet.add('meta.json'); // about to land last, do not delete - const existing = await readdir(dir).catch(() => [] as string[]); - for (const entry of existing) { - // Preserve the writer's own scratch dir + the .partial marker - // (we'll re-evaluate .partial at the end of commit). Anything else - // not-listed in the new bundle is stale. - if (entry === '.tmp' || entry === '.partial') continue; - if (newTopLevelSet.has(entry)) continue; - if (entry === 'steps') continue; // handled below - await rm(join(dir, entry), { recursive: true, force: true }); - } - - // (3) Replace `/steps/` with `/steps/`. - const stepsTmp = join(tmpDir, 'steps'); - const stepsDir = join(dir, 'steps'); - await rm(stepsDir, { recursive: true, force: true }); - if (await dirExists(stepsTmp)) { - await rename(stepsTmp, stepsDir); - } +export async function commitBundle(tmpDir: string, dir: string): Promise { + const parent = dirname(dir); + const base = basename(dir); + const stagingDir = join(parent, `.${base}.staging.${randomUUID()}`); + const priorDir = join(parent, `.${base}.prior.${randomUUID()}`); - // (4) Top-level files (result/failure/code/video). meta.json renames - // LAST; track it separately. - const metaIdx = topLevel.indexOf('meta.json'); - const beforeMeta = metaIdx >= 0 ? topLevel.filter((_, i) => i !== metaIdx) : topLevel; - for (const file of beforeMeta) { - await rename(join(tmpDir, file), join(dir, file)); - } + // Move the complete new bundle out of /.tmp/ without mutating + // the existing bundle tree yet. + await rename(tmpDir, stagingDir); - // (5) meta.json LAST → atomic completion signal. - if (metaIdx >= 0) { - await rename(join(tmpDir, 'meta.json'), join(dir, 'meta.json')); + try { + if (await dirExists(dir)) { + await rename(dir, priorDir); + } + await rename(stagingDir, dir); + await rm(priorDir, { recursive: true, force: true }); + await unlink(join(dir, '.partial')).catch(() => undefined); + } catch (err) { + const priorPresent = await dirExists(priorDir); + const dirPresent = await dirExists(dir); + if (priorPresent && !dirPresent) { + await rename(priorDir, dir); + } else if (priorPresent && dirPresent) { + await rm(dir, { recursive: true, force: true }); + await rename(priorDir, dir); + } + await rm(stagingDir, { recursive: true, force: true }).catch(() => undefined); + throw err; } - - // .partial from a prior aborted run is now stale. Remove it so an - // agent inspecting the dir sees only the fresh bundle. - await unlink(join(dir, '.partial')).catch(() => undefined); - - // Clean up the now-empty tmp dir. - await rm(tmpDir, { recursive: true, force: true }); } async function dirExists(path: string): Promise { @@ -796,7 +759,5 @@ function bundleIntegrityError( }); } -// Avoid unused-import lint warning for the `mkdtemp` and `readdir` -// helpers reserved for forensics + future resume work. +// Avoid unused-import lint warning for `mkdtemp` (reserved for resume work). void mkdtemp; -void readdir; diff --git a/src/lib/config.test.ts b/src/lib/config.test.ts index 463ddcc..599fc92 100644 --- a/src/lib/config.test.ts +++ b/src/lib/config.test.ts @@ -76,6 +76,28 @@ describe('loadConfig', () => { expect(config.apiUrl).toBe('https://from-file.example.com'); }); + it('treats empty / whitespace TESTSPRITE_API_URL as unset (falls through to profile)', () => { + writeProfile( + 'default', + { apiKey: 'sk-file', apiUrl: 'https://api.example.com:8443' }, + { path: credentialsPath }, + ); + const config = loadConfig({ + env: { TESTSPRITE_API_URL: ' ' }, + credentialsPath, + }); + expect(config.apiUrl).toBe('https://api.example.com:8443'); + }); + + it('treats empty / whitespace TESTSPRITE_API_KEY as unset (falls through to profile)', () => { + writeProfile('default', { apiKey: 'sk-file' }, { path: credentialsPath }); + const config = loadConfig({ + env: { TESTSPRITE_API_KEY: '' }, + credentialsPath, + }); + expect(config.apiKey).toBe('sk-file'); + }); + it('reads the requested profile, not just default', () => { writeProfile('dev', { apiKey: 'sk-dev' }, { path: credentialsPath }); const config = loadConfig({ profile: 'dev', env: {}, credentialsPath }); diff --git a/src/lib/config.ts b/src/lib/config.ts index 363c394..da0a23c 100644 --- a/src/lib/config.ts +++ b/src/lib/config.ts @@ -38,9 +38,15 @@ export function loadConfig(options: LoadConfigOptions = {}): Config { const credentialsPath = options.credentialsPath ?? defaultCredentialsPath(); const fileEntry = readProfile(profile, { path: credentialsPath }); + // Empty / whitespace-only env vars are treated as unset so they do not + // short-circuit the `??` chain (e.g. `export TESTSPRITE_API_URL=` in a shell + // profile). Matches the normalization in auth configure and init/setup. + const envApiUrl = env.TESTSPRITE_API_URL?.trim() || undefined; + const envApiKey = env.TESTSPRITE_API_KEY?.trim() || undefined; + return { - apiUrl: options.endpointUrl ?? env.TESTSPRITE_API_URL ?? fileEntry?.apiUrl ?? DEFAULT_API_URL, - apiKey: env.TESTSPRITE_API_KEY ?? fileEntry?.apiKey, + apiUrl: options.endpointUrl ?? envApiUrl ?? fileEntry?.apiUrl ?? DEFAULT_API_URL, + apiKey: envApiKey ?? fileEntry?.apiKey, profile, }; }