diff --git a/packages/contracts/src/replay.ts b/packages/contracts/src/replay.ts index 4a610131fd..c523c3a2e6 100644 --- a/packages/contracts/src/replay.ts +++ b/packages/contracts/src/replay.ts @@ -141,6 +141,13 @@ export type ReplaySuiteResult = { durationMs: number; failures: ReplaySuiteTestFailed[]; tests: ReplaySuiteTestResult[]; + /** + * The suite's own artifacts root (the parent of every test's `artifactsDir`), as resolved on + * the host that ran the suite. Absent when the suite produced no attempt (e.g. every source + * was filtered out). #2246: a remote daemon rewrites this to the caller-local path once the + * directory has been transferred back, so it always names a path the caller can open. + */ + artifactsDir?: string; snapshotDiagnostics?: SnapshotDiagnosticsSummary; }; diff --git a/packages/kernel/src/contracts.ts b/packages/kernel/src/contracts.ts index a42a4154be..7581203438 100644 --- a/packages/kernel/src/contracts.ts +++ b/packages/kernel/src/contracts.ts @@ -114,7 +114,8 @@ export type DaemonArtifactKnownType = | 'screen-recording' | 'screen-recording-chunk' | 'screen-recording-telemetry' - | 'trace-log'; + | 'trace-log' + | 'test-artifacts'; export type DaemonArtifactType = DaemonArtifactKnownType | (string & {}); diff --git a/packages/replay-test/src/internal/__tests__/session-test-artifacts.test.ts b/packages/replay-test/src/internal/__tests__/session-test-artifacts.test.ts index c74b0e9abd..14516e636b 100644 --- a/packages/replay-test/src/internal/__tests__/session-test-artifacts.test.ts +++ b/packages/replay-test/src/internal/__tests__/session-test-artifacts.test.ts @@ -4,11 +4,27 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { + DEFAULT_TEST_ARTIFACTS_ROOT, materializeReplayTestAttemptArtifacts, prepareReplayTestAttemptArtifacts, + resolveReplayTestArtifactsDir, } from '../session-test-artifacts.ts'; import type { ReplayTestAttemptOutcome } from '../session-test-types.ts'; +test('resolveReplayTestArtifactsDir falls back to the default root when artifactsDir is omitted', () => { + const dir = resolveReplayTestArtifactsDir({ cwd: '/repo', suiteInvocationId: 'abc123' }); + assert.equal(dir, path.resolve('/repo', DEFAULT_TEST_ARTIFACTS_ROOT, 'abc123')); +}); + +test('resolveReplayTestArtifactsDir resolves an explicit relative artifactsDir against cwd', () => { + const dir = resolveReplayTestArtifactsDir({ + artifactsDir: 'remote-device-artifacts/ad-test', + cwd: '/repo', + suiteInvocationId: 'abc123', + }); + assert.equal(dir, path.resolve('/repo', 'remote-device-artifacts/ad-test', 'abc123')); +}); + // Building outcomes from a DaemonResponse is the adapter's job and is pinned on that side; a // package test states the neutral outcome directly (#1478 P3b). const passedOutcome = ( diff --git a/packages/replay-test/src/internal/session-test-artifacts.ts b/packages/replay-test/src/internal/session-test-artifacts.ts index 1336e18188..b6a9be04b4 100644 --- a/packages/replay-test/src/internal/session-test-artifacts.ts +++ b/packages/replay-test/src/internal/session-test-artifacts.ts @@ -3,7 +3,15 @@ import path from 'node:path'; import { trimEdgeDashes } from '@agent-device/kernel/collections'; import type { ReplayTestAttemptOutcome } from '@agent-device/replay-test'; -const DEFAULT_TEST_ARTIFACTS_ROOT = '.agent-device/test-artifacts'; +/** + * `test`'s default artifacts root when `--artifacts-dir` is not given. `src/remote/daemon-artifacts.ts` + * keeps its own copy of this literal (to redirect a remote request without resolving anything + * against the daemon's `cwd`, #2246): pulling it from `@agent-device/contracts` instead grew this + * package's eager import closure by 3 modules for one string, past its pinned budget + * (`scripts/__tests__/eager-closure-budgets.test.ts`) — not worth it for a value that changes + * only if this line does. + */ +export const DEFAULT_TEST_ARTIFACTS_ROOT = '.agent-device/test-artifacts'; export function resolveReplayTestArtifactsDir(params: { artifactsDir?: string; diff --git a/packages/replay-test/src/internal/session-test.ts b/packages/replay-test/src/internal/session-test.ts index e2319f7694..6fb785ebff 100644 --- a/packages/replay-test/src/internal/session-test.ts +++ b/packages/replay-test/src/internal/session-test.ts @@ -124,7 +124,12 @@ export async function runReplayTestSuite( ); } - const data = summarizeReplayTestResults(plan.total, results, Date.now() - suiteStartedAt); + const data = summarizeReplayTestResults( + plan.total, + results, + Date.now() - suiteStartedAt, + plan.suiteArtifactsDir, + ); return { status: 'completed', data }; } catch (error) { const appErr = asAppError(error); @@ -467,6 +472,7 @@ function summarizeReplayTestResults( total: number, results: ReplaySuiteTestResult[], durationMs: number, + artifactsDir: string, ): ReplaySuiteResult { const passed = results.filter((result) => result.status === 'passed').length; const failedResults = results.filter( @@ -488,6 +494,7 @@ function summarizeReplayTestResults( durationMs, failures: failedResults, tests: results, + artifactsDir, ...(snapshotDiagnostics ? { snapshotDiagnostics } : {}), }; } diff --git a/src/daemon/replay/internal/__tests__/session-test-suite-command-remote-artifacts.test.ts b/src/daemon/replay/internal/__tests__/session-test-suite-command-remote-artifacts.test.ts new file mode 100644 index 0000000000..dff7ccbcc2 --- /dev/null +++ b/src/daemon/replay/internal/__tests__/session-test-suite-command-remote-artifacts.test.ts @@ -0,0 +1,172 @@ +/** + * #2246: once a remote daemon writes suite artifacts under a temp directory it owns (the client + * redirects `--artifacts-dir` there — see `daemon-artifacts-test-command.test.ts`), the response + * this handler builds must point the caller back at the REAL local root the client will download + * that directory into (`req.meta.clientArtifactPaths.artifactsDir`), and must register the + * directory as one downloadable artifact so the client's existing artifact transport can pull it. + * A local daemon never sets that hint, so the response must come back byte-for-byte unchanged. + * + * `daemonRoot` here is deliberately two levels (`/`), matching what + * `resolveReplayTestArtifactsDir` actually produces: the redirected temp ROOT the client sent + * joined with the daemon-generated invocation id. The caller-local mirror of that is + * `clientRoot/` — NOT bare `clientRoot`, which is only the root the client + * resolved from `--artifacts-dir` *before* the suite ran and could not yet know the invocation id. + */ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { test } from 'vitest'; +import type { ReplaySuiteResult } from '@agent-device/contracts/replay'; +import type { DaemonRequest } from '../../../types.ts'; +import { attachRemoteReplayTestArtifacts } from '../test-command.ts'; +import { mkdtempForTestSync } from '../../../../__tests__/test-utils/tmp-dir.ts'; + +const SUITE_INVOCATION_ID = 'cd5f9c01feec8d70'; + +function suiteResult(daemonRoot: string): ReplaySuiteResult { + return { + total: 2, + executed: 2, + passed: 1, + failed: 1, + skipped: 1, + notRun: 0, + durationMs: 10, + tests: [ + { + file: path.join(daemonRoot, '..', '01-open.ad'), + session: 'default:test:1', + status: 'passed', + durationMs: 5, + attempts: 1, + artifactsDir: path.join(daemonRoot, 'qa-flows__open.ad', 'attempt-1'), + replayed: 1, + healed: 0, + }, + { + file: path.join(daemonRoot, '..', '02-checkout.ad'), + session: 'default:test:2', + status: 'failed', + durationMs: 8, + attempts: 1, + artifactsDir: path.join(daemonRoot, 'qa-flows__checkout.ad', 'attempt-1'), + error: { code: 'COMMAND_FAILED', message: 'boom' }, + }, + { + file: 'skipped.ad', + status: 'skipped', + durationMs: 0, + reason: 'skipped-by-filter', + message: 'platform mismatch', + }, + ], + // The failed test above is intentionally the SAME object reference here, mirroring + // `summarizeReplayTestResults` (`failures: results.filter(...)`, `tests: results`) — a fix + // that only rewrites `tests` and forgets `failures` would leave this one unrewritten. + get failures() { + return this.tests.filter( + (t): t is Extract => + t.status === 'failed', + ); + }, + artifactsDir: daemonRoot, + }; +} + +function req(clientArtifactsRoot: string | undefined): DaemonRequest { + return { + token: 't', + session: 'default', + command: 'test', + positionals: [], + meta: clientArtifactsRoot ? { clientArtifactPaths: { artifactsDir: clientArtifactsRoot } } : {}, + }; +} + +test('rewrites the suite, every test, and every failure to the caller-local root, and registers the directory for download', () => { + const tempRoot = mkdtempForTestSync('agent-device-remote-test-artifacts-daemon-'); + const daemonRoot = path.join(tempRoot, SUITE_INVOCATION_ID); + fs.mkdirSync(path.join(daemonRoot, 'qa-flows__open.ad', 'attempt-1'), { recursive: true }); + const clientRoot = '/Users/ci/work/installer-app/remote-device-artifacts/ad-test'; + const clientSuiteRoot = path.join(clientRoot, SUITE_INVOCATION_ID); + + const data = attachRemoteReplayTestArtifacts(suiteResult(daemonRoot), req(clientRoot)); + + assert.equal(data.artifactsDir, clientSuiteRoot); + const tests = data.tests as ReplaySuiteResult['tests']; + assert.equal( + (tests[0] as { artifactsDir?: string }).artifactsDir, + path.join(clientSuiteRoot, 'qa-flows__open.ad', 'attempt-1'), + ); + assert.equal( + (tests[1] as { artifactsDir?: string }).artifactsDir, + path.join(clientSuiteRoot, 'qa-flows__checkout.ad', 'attempt-1'), + ); + // A skipped test never had an artifactsDir; it must not gain one. + const skippedTest = tests[2]; + assert.ok(skippedTest); + assert.equal('artifactsDir' in skippedTest, false); + + const failures = data.failures as ReplaySuiteResult['failures']; + assert.equal(failures.length, 1); + assert.equal( + (failures[0] as { artifactsDir?: string }).artifactsDir, + path.join(clientSuiteRoot, 'qa-flows__checkout.ad', 'attempt-1'), + ); + + assert.deepEqual(data.artifacts, [ + { + field: 'artifactsDir', + artifactType: 'test-artifacts', + path: daemonRoot, + // The download destination is the ROOT the client redirected to, not `clientSuiteRoot`: + // extracting the archive reproduces the invocation-id segment on its own. + localPath: clientRoot, + fileName: SUITE_INVOCATION_ID, + }, + ]); +}); + +test('a local daemon (no clientArtifactPaths hint) returns the suite result unchanged', () => { + const tempRoot = mkdtempForTestSync('agent-device-remote-test-artifacts-local-'); + const daemonRoot = path.join(tempRoot, SUITE_INVOCATION_ID); + fs.mkdirSync(path.join(daemonRoot, 'qa-flows__open.ad', 'attempt-1'), { recursive: true }); + const result = suiteResult(daemonRoot); + + const data = attachRemoteReplayTestArtifacts(result, req(undefined)); + + assert.equal(data, result); +}); + +test('a suite directory that was never created (all sources skipped) still reports the caller-local root, but registers no download', () => { + const tempRoot = mkdtempForTestSync('agent-device-remote-test-artifacts-empty-'); + const daemonRoot = path.join(tempRoot, SUITE_INVOCATION_ID); + const clientRoot = '/Users/ci/work/installer-app/remote-device-artifacts/ad-test'; + const result: ReplaySuiteResult = { + total: 1, + executed: 0, + passed: 0, + failed: 0, + skipped: 1, + notRun: 0, + durationMs: 1, + failures: [], + artifactsDir: daemonRoot, + tests: [ + { + file: 'skipped.ad', + status: 'skipped', + durationMs: 0, + reason: 'skipped-by-filter', + message: 'x', + }, + ], + }; + + const data = attachRemoteReplayTestArtifacts(result, req(clientRoot)); + + // Nothing to download, but the reported root is still the caller-local one the suite WOULD + // have used — not the unreachable daemon-local temp path. + assert.equal(data.artifactsDir, path.join(clientRoot, SUITE_INVOCATION_ID)); + assert.equal(data.artifacts, undefined); +}); diff --git a/src/daemon/replay/internal/test-command.ts b/src/daemon/replay/internal/test-command.ts index 12c38aae05..f635a8166e 100644 --- a/src/daemon/replay/internal/test-command.ts +++ b/src/daemon/replay/internal/test-command.ts @@ -1,10 +1,12 @@ /** Runs the replay-test scheduler and its nested replay attempts. */ +import fs from 'node:fs'; +import path from 'node:path'; import type { CommandFlags } from '@agent-device/contracts/command'; -import type { ReplayScriptSourceBundle } from '@agent-device/contracts/replay'; +import type { ReplaySuiteResult, ReplayScriptSourceBundle } from '@agent-device/contracts/replay'; import { REPLAY_SCRIPT_SOURCE_REQUIRED_MESSAGE } from '../../replay-script-source.ts'; import type { ReplayScriptMetadata } from '@agent-device/ad-script'; -import type { DaemonRequest, DaemonResponse } from '../../types.ts'; +import type { DaemonRequest, DaemonResponse, DaemonResponseData } from '../../types.ts'; import { expandSessionPath } from '../../session-paths.ts'; import type { ReplayTestCommand } from './command-types.ts'; import { @@ -279,10 +281,89 @@ export async function runReplayTestCommand(command: ReplayTestCommand): Promise< cleanupSession, }); return outcome.status === 'completed' - ? { ok: true, data: outcome.data } + ? { ok: true, data: attachRemoteReplayTestArtifacts(outcome.data, req) } : errorResponse(outcome.error.code, outcome.error.message); } +/** + * #2246: against a remote daemon, the client redirects `--artifacts-dir` to a temp directory on + * THIS host before the suite runs (`prepareRemoteRequestArtifacts` in + * `src/remote/daemon-artifacts.ts`) — the caller's real `cwd` names no path here, so the + * scheduler above must never resolve suite artifacts against it. `req.meta.clientArtifactPaths` + * is how that client tells the daemon the real caller-local ROOT (e.g. `--artifacts-dir`, or its + * default) it will pull the directory back into once the request returns — that root is resolved + * client-side *before* the suite runs, so it never contains the suite's own invocation id. + * + * `data.artifactsDir` (the daemon-local suite directory) is always `/`: `createDirectoryArchive` (`src/daemon/artifact-tracking.ts`) tars it as + * `-C dirname(daemonRoot) -- basename(daemonRoot)`, so extracting that archive under the + * caller-local root reproduces the SAME invocation-id segment there. The caller-facing root is + * therefore `path.join(clientRoot, basename(daemonRoot))`, not `clientRoot` itself — every path + * this rewrites must land under that, matching where the client will actually put the files. + * + * Rewriting every artifact path in the response to that root, and registering the directory + * itself as one downloadable artifact (the same generic `data.artifacts` -> `trackArtifact` + * mechanism `screenshot`/`record` already use), are host concerns: the scheduler only ever + * reports paths on its own filesystem. + * + * A local daemon never sets `clientArtifactPaths` (the client only redirects for a remote one), + * so this is a no-op there and the response is returned unchanged. + */ +export function attachRemoteReplayTestArtifacts( + data: ReplaySuiteResult, + req: DaemonRequest, +): DaemonResponseData { + const clientRoot = req.meta?.clientArtifactPaths?.artifactsDir; + const daemonRoot = data.artifactsDir; + if (!clientRoot || !daemonRoot) return data; + + // The directory the client will actually extract the download into, on ITS filesystem — + // `clientRoot` plus the same invocation-id segment `daemonRoot` ends in (see doc comment). + const clientSuiteRoot = path.join(clientRoot, path.basename(daemonRoot)); + const remapArtifactsDir = (candidate: string): string => + candidate === daemonRoot || candidate.startsWith(daemonRoot + path.sep) + ? clientSuiteRoot + candidate.slice(daemonRoot.length) + : candidate; + // `tests` and `failures` both come from the scheduler's own `results` array + // (`summarizeReplayTestResults`) and share object references for every failed entry — each + // array is rewritten independently so both, not just whichever a caller happens to read, carry + // the caller-local path. + const rewritten: DaemonResponseData = { + ...data, + artifactsDir: clientSuiteRoot, + tests: data.tests.map((test) => + 'artifactsDir' in test && test.artifactsDir + ? { ...test, artifactsDir: remapArtifactsDir(test.artifactsDir) } + : test, + ), + failures: data.failures.map((failure) => + failure.artifactsDir + ? { ...failure, artifactsDir: remapArtifactsDir(failure.artifactsDir) } + : failure, + ), + }; + + // No attempt ever ran (e.g. every source was filtered out), so the directory the client would + // try to download was never created — the rewritten paths above are still correct (they name + // where the suite WOULD have put its output), just nothing to register a download for. + if (!fs.existsSync(daemonRoot)) return rewritten; + + return { + ...rewritten, + artifacts: [ + { + field: 'artifactsDir', + artifactType: 'test-artifacts', + path: daemonRoot, + // The download destination is the ROOT, not `clientSuiteRoot`: the archive's own + // top-level entry already supplies the invocation-id segment on extraction. + localPath: clientRoot, + fileName: path.basename(daemonRoot), + }, + ], + }; +} + /** * Translates a daemon `test` request into the scheduler's neutral request (#1478 P3b). * diff --git a/src/remote/__tests__/daemon-artifacts-test-command.test.ts b/src/remote/__tests__/daemon-artifacts-test-command.test.ts new file mode 100644 index 0000000000..b804135b5c --- /dev/null +++ b/src/remote/__tests__/daemon-artifacts-test-command.test.ts @@ -0,0 +1,75 @@ +/** + * #2246: a remote daemon's suite artifacts must never be resolved against the caller's `cwd` on + * the daemon's own filesystem (that's what produced the reported `ENOENT` — `mkdir` on a path + * that only exists on the caller's machine). This mirrors #1802's read-side fix for the same + * command: the client resolves the real local artifacts root itself and redirects the daemon to + * a temp path it owns, exactly as it already does for `screenshot`/`record start`. + */ +import assert from 'node:assert/strict'; +import path from 'node:path'; +import { test } from 'vitest'; +import { prepareRemoteRequestArtifacts } from '../daemon-artifacts.ts'; + +const REMOTE = { baseUrl: 'http://remote-mac.example.test:7777/agent-device', token: 'secret' }; +const LOCAL = { token: 'secret' }; + +function testRequest(artifactsDir: string | undefined, cwd = '/repo') { + return { + session: 'default', + command: 'test', + positionals: ['./qa-flows/open-grid-settings.ad'], + flags: artifactsDir === undefined ? {} : { artifactsDir }, + meta: { cwd }, + }; +} + +test('a remote daemon redirects an explicit --artifacts-dir to a temp path it owns', async () => { + const prepared = await prepareRemoteRequestArtifacts( + testRequest('remote-device-artifacts/ad-test'), + REMOTE, + ); + + const redirected = (prepared.flags as Record | undefined)?.artifactsDir; + assert.equal(typeof redirected, 'string'); + assert.ok((redirected as string).startsWith('/tmp/agent-device-test-artifacts-')); + assert.equal( + prepared.clientArtifactPaths?.artifactsDir, + path.resolve('/repo', 'remote-device-artifacts/ad-test'), + ); +}); + +test('a remote daemon redirects the default artifacts directory too', async () => { + const prepared = await prepareRemoteRequestArtifacts(testRequest(undefined), REMOTE); + + const redirected = (prepared.flags as Record | undefined)?.artifactsDir; + assert.equal(typeof redirected, 'string'); + assert.ok((redirected as string).startsWith('/tmp/agent-device-test-artifacts-')); + assert.equal( + prepared.clientArtifactPaths?.artifactsDir, + path.resolve('/repo', '.agent-device/test-artifacts'), + ); +}); + +test('a remote daemon leaves an already-absolute --artifacts-dir as the download target', async () => { + const prepared = await prepareRemoteRequestArtifacts( + testRequest('/ci/artifacts/ad-test'), + REMOTE, + ); + + const redirected = (prepared.flags as Record | undefined)?.artifactsDir; + assert.notEqual(redirected, '/ci/artifacts/ad-test'); + assert.equal(prepared.clientArtifactPaths?.artifactsDir, '/ci/artifacts/ad-test'); +}); + +test('a local daemon leaves --artifacts-dir untouched', async () => { + const prepared = await prepareRemoteRequestArtifacts( + testRequest('remote-device-artifacts/ad-test'), + LOCAL, + ); + + assert.equal( + (prepared.flags as Record | undefined)?.artifactsDir, + 'remote-device-artifacts/ad-test', + ); + assert.equal(prepared.clientArtifactPaths, undefined); +}); diff --git a/src/remote/artifact-download.ts b/src/remote/artifact-download.ts new file mode 100644 index 0000000000..47473564a2 --- /dev/null +++ b/src/remote/artifact-download.ts @@ -0,0 +1,191 @@ +import fs from 'node:fs'; +// Type-only: a runtime `node:http` import would load it eagerly for every CLI command. The actual +// HTTP stack loads through `loadNodeHttpRequester` only when a remote daemon returns an artifact. +import type http from 'node:http'; +import path from 'node:path'; +import { pipeline } from 'node:stream/promises'; +import { AppError } from '@agent-device/kernel/errors'; +import { loadNodeHttpRequester } from '@agent-device/host-kit/transport'; +import type { DaemonRequestMeta } from '@agent-device/kernel/contracts'; +import { + buildDaemonHttpAuthHeaders, + buildDaemonHttpTenantHeaders, +} from '../daemon/http-contract.ts'; + +const REMOTE_ARTIFACT_DOWNLOAD_TIMEOUT_MS = 90_000; + +export type RemoteArtifactDownload = { + artifactUrl: URL; + token: string; + artifactId: string; + destinationPath: string; + requestScope: DaemonRequestMeta | undefined; + timeoutMs?: number; + /** Whether `destinationPath` is a caller-owned root rather than one file to create. */ + isDirectory?: boolean; +}; + +type DownloadDestination = { + prepare(): Promise; + cleanupOnError(): Promise; + materialize(res: http.IncomingMessage, signal: AbortSignal): Promise; +}; + +function buildDownloadDestination(params: RemoteArtifactDownload): DownloadDestination { + const { destinationPath } = params; + if (params.isDirectory) { + return { + prepare: async () => { + await fs.promises.mkdir(destinationPath, { recursive: true }); + }, + // The root belongs to the caller and may contain earlier runs. Directory materialization + // owns and cleans its hidden staging directory, so the root itself is never removed. + cleanupOnError: async () => {}, + materialize: (res, signal) => materializeDirectoryArtifact(res, destinationPath, signal), + }; + } + return { + prepare: async () => { + await fs.promises.mkdir(path.dirname(destinationPath), { recursive: true }); + }, + cleanupOnError: () => fs.promises.rm(destinationPath, { force: true }), + materialize: async (res, signal) => { + await pipeline(res, fs.createWriteStream(destinationPath), { signal }); + return destinationPath; + }, + }; +} + +/** Downloads an artifact and returns the exact file or directory published on the client. */ +export async function downloadRemoteArtifactFromUrl( + params: RemoteArtifactDownload, +): Promise { + const transport = await loadNodeHttpRequester(params.artifactUrl.protocol); + const destination = buildDownloadDestination(params); + await destination.prepare(); + return await new Promise((resolve, reject) => { + let settled = false; + let materializationStarted = false; + let timeoutError: AppError | undefined; + const operation = new AbortController(); + const timeoutMs = params.timeoutMs ?? REMOTE_ARTIFACT_DOWNLOAD_TIMEOUT_MS; + const settle = (result?: string, error?: Error) => { + if (settled) return; + settled = true; + clearTimeout(timeoutHandle); + if (error) { + void destination.cleanupOnError().finally(() => reject(error)); + return; + } + resolve(result ?? params.destinationPath); + }; + const request = transport.request( + { + protocol: params.artifactUrl.protocol, + host: params.artifactUrl.hostname, + port: params.artifactUrl.port, + method: 'GET', + path: params.artifactUrl.pathname + params.artifactUrl.search, + headers: { + ...buildDaemonHttpAuthHeaders(params.token), + ...buildDaemonHttpTenantHeaders(params.requestScope?.tenantId), + }, + }, + (res) => { + if ((res.statusCode ?? 500) >= 400) { + let body = ''; + res.setEncoding('utf8'); + res.on('data', (chunk) => { + body += chunk; + }); + res.on('end', () => { + settle( + undefined, + new AppError('COMMAND_FAILED', 'Failed to download remote artifact', { + artifactId: params.artifactId, + statusCode: res.statusCode, + requestId: params.requestScope?.requestId, + body, + }), + ); + }); + return; + } + materializationStarted = true; + void destination.materialize(res, operation.signal).then( + (materializedPath) => settle(materializedPath), + (error: unknown) => + settle( + undefined, + timeoutError ?? (error instanceof Error ? error : new Error(String(error))), + ), + ); + }, + ); + const timeoutHandle = setTimeout(() => { + timeoutError = new AppError('COMMAND_FAILED', 'Remote artifact download timed out', { + artifactId: params.artifactId, + requestId: params.requestScope?.requestId, + timeoutMs, + }); + operation.abort(timeoutError); + request.destroy(timeoutError); + // The abort-aware materializer cleans its staging area. Do not reject before it finishes, + // or extraction could keep writing after the caller observes the timeout. + if (!materializationStarted) settle(undefined, timeoutError); + }, timeoutMs); + request.on('error', (error) => { + if (materializationStarted) return; + if (error instanceof AppError) { + settle(undefined, timeoutError ?? error); + return; + } + settle( + undefined, + new AppError( + 'COMMAND_FAILED', + 'Failed to download remote artifact', + { + artifactId: params.artifactId, + requestId: params.requestScope?.requestId, + timeoutMs, + }, + error instanceof Error ? error : undefined, + ), + ); + }); + request.end(); + }); +} + +/** Safely extracts one directory and atomically publishes it under the caller-owned root. */ +async function materializeDirectoryArtifact( + res: http.IncomingMessage, + destinationRoot: string, + signal: AbortSignal, +): Promise { + // Staging beside the final entry guarantees one filesystem, so there is no EXDEV copy fallback + // that could expose a partially copied suite. + const tempDir = await fs.promises.mkdtemp(path.join(destinationRoot, '.agent-device-download-')); + const archivePath = path.join(tempDir, 'artifact.tar.gz'); + const stagingRoot = path.join(tempDir, 'extracted'); + try { + await pipeline(res, fs.createWriteStream(archivePath), { signal }); + const { extractArchiveSafely } = await import('@agent-device/host-kit/archive'); + await extractArchiveSafely({ archivePath, outputRoot: stagingRoot, type: 'tgz', signal }); + signal.throwIfAborted(); + const [entryName, ...extraEntries] = await fs.promises.readdir(stagingRoot); + if (!entryName || extraEntries.length > 0) { + throw new AppError( + 'COMMAND_FAILED', + `Downloaded directory artifact has an unexpected shape: expected exactly one top-level entry, found ${extraEntries.length + (entryName ? 1 : 0)}`, + ); + } + signal.throwIfAborted(); + const materializedPath = path.join(destinationRoot, entryName); + await fs.promises.rename(path.join(stagingRoot, entryName), materializedPath); + return materializedPath; + } finally { + await fs.promises.rm(tempDir, { recursive: true, force: true }); + } +} diff --git a/src/remote/daemon-artifacts.ts b/src/remote/daemon-artifacts.ts index b58fd9ec61..417dc5c59f 100644 --- a/src/remote/daemon-artifacts.ts +++ b/src/remote/daemon-artifacts.ts @@ -1,13 +1,7 @@ import fs from 'node:fs'; import path from 'node:path'; -import { pipeline } from 'node:stream/promises'; import { AppError } from '@agent-device/kernel/errors'; -import { loadNodeHttpRequester } from '@agent-device/host-kit/transport'; import type { DaemonArtifact, DaemonRequest, DaemonResponse } from '../daemon/types.ts'; -import { - buildDaemonHttpAuthHeaders, - buildDaemonHttpTenantHeaders, -} from '../daemon/http-contract.ts'; import { appendRecordingExtensionWhenMissing, recordingExtensionForPlatform, @@ -15,8 +9,11 @@ import { import { uploadArtifact } from './upload-client.ts'; import { createStderrUploadProgressReporter, type UploadProgressSink } from './upload-progress.ts'; -// Mirrors the current daemon RPC timeout, but artifact download timeouts may diverge. -const REMOTE_ARTIFACT_DOWNLOAD_TIMEOUT_MS = 90_000; +// Mirrors `DEFAULT_TEST_ARTIFACTS_ROOT` in `packages/replay-test/src/internal/session-test-artifacts.ts` +// (the daemon's own default). Duplicated rather than imported: pulling it from +// `@agent-device/contracts` grew that package's pinned eager-import closure by 3 modules for one +// string (#2246) — not worth it for a literal that only ever changes alongside this comment. +const DEFAULT_TEST_ARTIFACTS_ROOT = '.agent-device/test-artifacts'; export type DaemonArtifactEndpoint = { baseUrl?: string; @@ -135,20 +132,25 @@ function applyRemoteArtifactCommand( ): DaemonRequest['flags'] | undefined { const remoteArtifact = prepareRemoteArtifactCommand(req, positionals); if (!remoteArtifact) return flags; - if (remoteArtifact.positionalPath !== undefined) { + if (remoteArtifact.positionalIndex !== undefined && remoteArtifact.positionalPath !== undefined) { positionals[remoteArtifact.positionalIndex] = remoteArtifact.positionalPath; } - const nextFlags = applyRemoteArtifactOutFlag(flags, remoteArtifact.flagPath); + const nextFlags = applyRemoteArtifactFlag( + flags, + remoteArtifact.flagKey ?? 'out', + remoteArtifact.flagPath, + ); clientArtifactPaths[remoteArtifact.field] = remoteArtifact.localPath; return nextFlags; } -function applyRemoteArtifactOutFlag( +function applyRemoteArtifactFlag( flags: DaemonRequest['flags'] | undefined, + flagKey: string, flagPath: string | undefined, ): DaemonRequest['flags'] | undefined { if (flagPath === undefined) return flags; - return { ...(flags ?? {}), out: flagPath }; + return { ...(flags ?? {}), [flagKey]: flagPath }; } function resolveLocalInstallPath(rawPath: string, cwd: string | undefined): string | undefined { @@ -232,8 +234,9 @@ function prepareRemoteArtifactCommand( ): { field: string; localPath: string; - positionalIndex: number; + positionalIndex?: number; positionalPath?: string; + flagKey?: string; flagPath?: string; } | null { if (req.command === 'screenshot') { @@ -272,9 +275,27 @@ function prepareRemoteArtifactCommand( ), }; } + if (req.command === 'test') { + // #2246: `test` always materializes a suite artifacts directory, whether or not the caller + // passed `--artifacts-dir` — unlike `record`, there is no "nothing to redirect" case here. + // Resolving the caller-local root here (not on the daemon) mirrors #1802's read-side fix for + // the same command: the daemon must never resolve a path against the caller's `cwd`. + return { + field: 'artifactsDir', + localPath: resolveClientArtifactOutputRoot(req), + flagKey: 'artifactsDir', + flagPath: buildRemoteTempArtifactDirPath('test-artifacts'), + }; + } return null; } +function resolveClientArtifactOutputRoot(req: Omit): string { + const requested = req.flags?.artifactsDir; + const rawPath = hasNonEmptyString(requested) ? requested : DEFAULT_TEST_ARTIFACTS_ROOT; + return resolveAbsoluteClientPath(rawPath, req.meta?.cwd); +} + function recordingFallbackExtension(req: Omit): string { return recordingExtensionForPlatform(req.flags?.platform); } @@ -299,8 +320,15 @@ function resolveClientArtifactOutputPath( ): string { const requested = req.positionals?.[positionalIndex] ?? req.flags?.out; const fallbackName = `${field === 'path' ? 'screenshot' : 'recording'}-${Date.now()}${fallbackExtension}`; - const rawPath = hasNonEmptyString(requested) ? requested : fallbackName; - return path.isAbsolute(rawPath) ? rawPath : path.resolve(req.meta?.cwd ?? process.cwd(), rawPath); + return resolveAbsoluteClientPath( + hasNonEmptyString(requested) ? requested : fallbackName, + req.meta?.cwd, + ); +} + +/** Shared tail of every "what local path did the caller mean" resolution in this file. */ +function resolveAbsoluteClientPath(rawPath: string, cwd: string | undefined): string { + return path.isAbsolute(rawPath) ? rawPath : path.resolve(cwd ?? process.cwd(), rawPath); } function hasNonEmptyString(value: unknown): value is string { @@ -315,6 +343,14 @@ function buildRemoteTempArtifactPath(prefix: string, extension: string): string ); } +/** A directory temp path — unlike `buildRemoteTempArtifactPath`, no extension is ever appended. */ +function buildRemoteTempArtifactDirPath(prefix: string): string { + return path.posix.join( + '/tmp', + `agent-device-${prefix}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`, + ); +} + export async function materializeRemoteArtifacts( info: DaemonArtifactEndpoint, req: DaemonRequest, @@ -329,18 +365,26 @@ export async function materializeRemoteArtifacts( nextArtifacts.push(artifact); continue; } - const localPath = resolveMaterializedArtifactPath(artifact, req); - await downloadRemoteArtifact({ + const downloadPath = resolveMaterializedArtifactPath(artifact, req); + const materializedPath = await downloadRemoteArtifact({ baseUrl: info.baseUrl, token: info.token, artifactId: artifact.artifactId, - destinationPath: localPath, + destinationPath: downloadPath, requestScope: req.meta, + // #2246: `test-artifacts` is the one directory-shaped artifact type today — known from the + // response, before any bytes arrive, so cleanup-on-error can be decided up front instead of + // racing the response headers (a directory destination must never be `rm`'d wholesale; see + // `downloadRemoteArtifact`). + isDirectory: artifact.artifactType === 'test-artifacts', }); - nextData[artifact.field] = localPath; + // Directory artifacts download into a caller-owned root, then atomically publish their one + // top-level entry beneath it. Use the path the download actually published rather than the + // root hint sent over the wire; otherwise `artifactsDir` loses its suite invocation segment. + nextData[artifact.field] = materializedPath; nextArtifacts.push({ ...artifact, - localPath, + localPath: materializedPath, }); } nextData.artifacts = nextArtifacts; @@ -366,100 +410,24 @@ type DownloadRemoteArtifactParams = { destinationPath: string; requestScope: DaemonRequest['meta']; timeoutMs?: number; + /** Whether `destinationPath` is a caller-owned root rather than one file to create. */ + isDirectory?: boolean; }; -export async function downloadRemoteArtifact(params: DownloadRemoteArtifactParams): Promise { - const artifactUrl = new URL(buildDaemonArtifactUrl(params.baseUrl, params.artifactId)); - // `prepareRemoteRequestArtifacts` runs on every CLI request, but only a - // remote daemon ever downloads an artifact, so the HTTP stack loads here. - const transport = await loadNodeHttpRequester(artifactUrl.protocol); - await fs.promises.mkdir(path.dirname(params.destinationPath), { recursive: true }); - await new Promise((resolve, reject) => { - let settled = false; - const timeoutMs = params.timeoutMs ?? REMOTE_ARTIFACT_DOWNLOAD_TIMEOUT_MS; - const settle = (error?: Error) => { - if (settled) return; - settled = true; - clearTimeout(timeoutHandle); - if (error) { - void fs.promises.rm(params.destinationPath, { force: true }).finally(() => reject(error)); - return; - } - resolve(); - }; - const request = transport.request( - { - protocol: artifactUrl.protocol, - host: artifactUrl.hostname, - port: artifactUrl.port, - method: 'GET', - path: artifactUrl.pathname + artifactUrl.search, - headers: { - ...buildDaemonHttpAuthHeaders(params.token), - ...buildDaemonHttpTenantHeaders(params.requestScope?.tenantId), - }, - }, - (res) => { - if ((res.statusCode ?? 500) >= 400) { - let body = ''; - res.setEncoding('utf8'); - res.on('data', (chunk) => { - body += chunk; - }); - res.on('end', () => { - settle( - new AppError('COMMAND_FAILED', 'Failed to download remote artifact', { - artifactId: params.artifactId, - statusCode: res.statusCode, - requestId: params.requestScope?.requestId, - body, - }), - ); - }); - return; - } - res.on('aborted', () => { - settle( - new AppError('COMMAND_FAILED', 'Remote artifact download was interrupted', { - artifactId: params.artifactId, - requestId: params.requestScope?.requestId, - }), - ); - }); - void pipeline(res, fs.createWriteStream(params.destinationPath)).then( - () => settle(), - (error: unknown) => settle(error instanceof Error ? error : new Error(String(error))), - ); - }, - ); - const timeoutHandle = setTimeout(() => { - const timeoutError = new AppError('COMMAND_FAILED', 'Remote artifact download timed out', { - artifactId: params.artifactId, - requestId: params.requestScope?.requestId, - timeoutMs, - }); - settle(timeoutError); - request.destroy(timeoutError); - }, timeoutMs); - request.on('error', (error) => { - if (error instanceof AppError) { - settle(error); - return; - } - settle( - new AppError( - 'COMMAND_FAILED', - 'Failed to download remote artifact', - { - artifactId: params.artifactId, - requestId: params.requestScope?.requestId, - timeoutMs, - }, - error instanceof Error ? error : undefined, - ), - ); - }); - request.end(); +export async function downloadRemoteArtifact( + params: DownloadRemoteArtifactParams, +): Promise { + // Remote artifact transfer is not part of routine CLI startup. Keep the HTTP/archive owner out + // of the eager command graph and load it only after a remote response names an artifact. + const { downloadRemoteArtifactFromUrl } = await import('./artifact-download.ts'); + return await downloadRemoteArtifactFromUrl({ + artifactUrl: new URL(buildDaemonArtifactUrl(params.baseUrl, params.artifactId)), + token: params.token, + artifactId: params.artifactId, + destinationPath: params.destinationPath, + requestScope: params.requestScope, + ...(params.timeoutMs === undefined ? {} : { timeoutMs: params.timeoutMs }), + ...(params.isDirectory === undefined ? {} : { isDirectory: params.isDirectory }), }); } diff --git a/test/integration/provider-scenarios/remote-test-artifacts-directory.test.ts b/test/integration/provider-scenarios/remote-test-artifacts-directory.test.ts new file mode 100644 index 0000000000..89ae020c27 --- /dev/null +++ b/test/integration/provider-scenarios/remote-test-artifacts-directory.test.ts @@ -0,0 +1,214 @@ +/** + * #2246: the download half of the fix. The daemon's artifact transport already tar's a + * directory artifact on the fly (`artifact-tracking.ts`'s `ensureDirectoryArchive`) — this test + * is the first to exercise that path all the way through `downloadRemoteArtifact` on the client, + * which used to only ever write a response body straight to one file (screenshot/recording). + * `test`'s suite artifacts are a whole directory tree, so the client must detect the + * `application/gzip` directory-archive response and extract it instead. + */ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { test } from 'vitest'; +import { AppError } from '@agent-device/kernel/errors'; +import { + cleanupDownloadableArtifact, + trackDownloadableArtifact, +} from '../../../src/daemon/artifact-tracking.ts'; +import { createDaemonHttpServer } from '../../../src/daemon/server/http-server.ts'; +import { downloadRemoteArtifact } from '../../../src/remote/daemon-artifacts.ts'; +import { + closeLoopbackServer, + listenOnLoopback, + skipWhenLoopbackUnavailable, +} from '../../../src/__tests__/test-utils/loopback.ts'; + +const SUITE_INVOCATION_ID = 'cd5f9c01feec8d70'; + +test('downloadRemoteArtifact extracts a directory artifact into the caller-local artifacts root', async (t) => { + if (await skipWhenLoopbackUnavailable(t, 'remote test-artifacts directory download coverage')) + return; + + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-remote-test-artifacts-')); + // Mirrors what the daemon actually produces: `//...`. + const daemonSuiteDir = path.join(tempDir, 'daemon-side', SUITE_INVOCATION_ID); + const attemptDir = path.join(daemonSuiteDir, 'qa-flows__open-grid-settings.ad', 'attempt-1'); + fs.mkdirSync(attemptDir, { recursive: true }); + fs.writeFileSync(path.join(attemptDir, 'replay.ad'), 'context platform=ios\nopen "Demo"\n'); + fs.writeFileSync(path.join(attemptDir, 'result.txt'), 'status: passed\n'); + + // The caller-local artifacts root the client resolved before the request was sent + // (`resolveClientArtifactOutputRoot` in `daemon-artifacts.ts`) — a plain directory that, in + // the real CLI, may already hold earlier suite runs. + const callerArtifactsRoot = path.join( + tempDir, + 'caller-side', + 'remote-device-artifacts', + 'ad-test', + ); + fs.mkdirSync(callerArtifactsRoot, { recursive: true }); + fs.writeFileSync(path.join(callerArtifactsRoot, 'previous-run-marker.txt'), 'keep me'); + + const artifactId = trackDownloadableArtifact({ + artifactPath: daemonSuiteDir, + artifactType: 'test-artifacts', + fileName: SUITE_INVOCATION_ID, + }); + const server = await createDaemonHttpServer({ + token: 'daemon-token', + handleRequest: async () => { + throw new Error('not exercised: this test only hits the /artifacts route'); + }, + }); + + try { + const port = await listenOnLoopback(server); + await downloadRemoteArtifact({ + baseUrl: `http://127.0.0.1:${port}`, + token: 'daemon-token', + artifactId, + destinationPath: callerArtifactsRoot, + requestScope: {}, + isDirectory: true, + }); + + const downloadedResult = path.join( + callerArtifactsRoot, + SUITE_INVOCATION_ID, + 'qa-flows__open-grid-settings.ad', + 'attempt-1', + 'result.txt', + ); + assert.equal(fs.readFileSync(downloadedResult, 'utf8'), 'status: passed\n'); + assert.equal( + fs.readFileSync( + path.join( + callerArtifactsRoot, + SUITE_INVOCATION_ID, + 'qa-flows__open-grid-settings.ad', + 'attempt-1', + 'replay.ad', + ), + 'utf8', + ), + 'context platform=ios\nopen "Demo"\n', + ); + // Earlier runs already in the caller's artifacts root must survive untouched. + assert.equal( + fs.readFileSync(path.join(callerArtifactsRoot, 'previous-run-marker.txt'), 'utf8'), + 'keep me', + ); + } finally { + cleanupDownloadableArtifact(artifactId); + await closeLoopbackServer(server); + fs.rmSync(tempDir, { recursive: true, force: true }); + } +}); + +test('a failed directory download leaves the caller-local artifacts root untouched', async (t) => { + if (await skipWhenLoopbackUnavailable(t, 'remote test-artifacts directory download coverage')) + return; + + const tempDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'agent-device-remote-test-artifacts-fail-'), + ); + const callerArtifactsRoot = path.join(tempDir, 'remote-device-artifacts', 'ad-test'); + fs.mkdirSync(callerArtifactsRoot, { recursive: true }); + fs.writeFileSync(path.join(callerArtifactsRoot, 'previous-run-marker.txt'), 'keep me'); + + const server = await createDaemonHttpServer({ + token: 'daemon-token', + handleRequest: async () => { + throw new Error('not exercised: this test only hits the /artifacts route'); + }, + }); + + try { + const port = await listenOnLoopback(server); + // An id nothing tracked: the server answers 404, exercising the error path before any + // directory-archive bytes ever arrive. + await assert.rejects( + async () => + await downloadRemoteArtifact({ + baseUrl: `http://127.0.0.1:${port}`, + token: 'daemon-token', + artifactId: 'does-not-exist', + destinationPath: callerArtifactsRoot, + requestScope: {}, + isDirectory: true, + }), + ); + + // The pre-existing directory — and everything already in it — must survive: it is the + // caller's own `--artifacts-dir` root, not a file this download owns. + assert.equal(fs.existsSync(callerArtifactsRoot), true); + assert.equal( + fs.readFileSync(path.join(callerArtifactsRoot, 'previous-run-marker.txt'), 'utf8'), + 'keep me', + ); + } finally { + await closeLoopbackServer(server); + fs.rmSync(tempDir, { recursive: true, force: true }); + } +}); + +test('a directory artifact containing a symlink is rejected, not extracted', async (t) => { + if (await skipWhenLoopbackUnavailable(t, 'remote test-artifacts directory download coverage')) + return; + + const tempDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'agent-device-remote-test-artifacts-evil-'), + ); + // A remote (possibly compromised) daemon returning an archive with a symlink entry — the + // real-world shape `extractArchiveSafely` guards against, since raw `tar xzf` would otherwise + // follow it wherever it points on extraction. + const daemonSuiteDir = path.join(tempDir, 'daemon-side', SUITE_INVOCATION_ID); + fs.mkdirSync(daemonSuiteDir, { recursive: true }); + const escapeTarget = path.join(tempDir, 'outside-destination.txt'); + fs.writeFileSync(escapeTarget, 'should never be linked to'); + fs.symlinkSync(escapeTarget, path.join(daemonSuiteDir, 'evil-link')); + + const callerArtifactsRoot = path.join( + tempDir, + 'caller-side', + 'remote-device-artifacts', + 'ad-test', + ); + fs.mkdirSync(callerArtifactsRoot, { recursive: true }); + + const artifactId = trackDownloadableArtifact({ + artifactPath: daemonSuiteDir, + artifactType: 'test-artifacts', + fileName: SUITE_INVOCATION_ID, + }); + const server = await createDaemonHttpServer({ + token: 'daemon-token', + handleRequest: async () => { + throw new Error('not exercised: this test only hits the /artifacts route'); + }, + }); + + try { + const port = await listenOnLoopback(server); + await assert.rejects( + async () => + await downloadRemoteArtifact({ + baseUrl: `http://127.0.0.1:${port}`, + token: 'daemon-token', + artifactId, + destinationPath: callerArtifactsRoot, + requestScope: {}, + isDirectory: true, + }), + (error: unknown) => + error instanceof AppError && error.details?.reason === 'ARCHIVE_UNSAFE_ENTRY', + ); + + assert.equal(fs.existsSync(path.join(callerArtifactsRoot, SUITE_INVOCATION_ID)), false); + } finally { + cleanupDownloadableArtifact(artifactId); + await closeLoopbackServer(server); + fs.rmSync(tempDir, { recursive: true, force: true }); + } +}); diff --git a/test/integration/provider-scenarios/remote-test-artifacts-materialization.test.ts b/test/integration/provider-scenarios/remote-test-artifacts-materialization.test.ts new file mode 100644 index 0000000000..1d504f1f0a --- /dev/null +++ b/test/integration/provider-scenarios/remote-test-artifacts-materialization.test.ts @@ -0,0 +1,154 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import http from 'node:http'; +import os from 'node:os'; +import path from 'node:path'; +import { test } from 'vitest'; +import type { ReplaySuiteResult } from '@agent-device/contracts/replay'; +import { + cleanupDownloadableArtifact, + trackDownloadableArtifact, +} from '../../../src/daemon/artifact-tracking.ts'; +import type { DaemonRequest } from '../../../src/daemon/types.ts'; +import { createDaemonHttpServer } from '../../../src/daemon/server/http-server.ts'; +import { attachRemoteReplayTestArtifacts } from '../../../src/daemon/replay/internal/test-command.ts'; +import { + downloadRemoteArtifact, + materializeRemoteArtifacts, +} from '../../../src/remote/daemon-artifacts.ts'; +import { + closeLoopbackServer, + listenOnLoopback, + skipWhenLoopbackUnavailable, +} from '../../../src/__tests__/test-utils/loopback.ts'; + +const SUITE_INVOCATION_ID = 'cd5f9c01feec8d70'; + +test('the composed daemon and client path owners report the directory that was materialized', async (t) => { + if (await skipWhenLoopbackUnavailable(t, 'remote test-artifacts materialization coverage')) + return; + + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-remote-suite-response-')); + const daemonSuiteDir = path.join(tempDir, 'daemon-side', SUITE_INVOCATION_ID); + const daemonAttemptDir = path.join(daemonSuiteDir, 'flow.ad', 'attempt-1'); + const clientRoot = path.join(tempDir, 'caller-side', 'artifacts'); + const clientSuiteDir = path.join(clientRoot, SUITE_INVOCATION_ID); + fs.mkdirSync(daemonAttemptDir, { recursive: true }); + fs.writeFileSync(path.join(daemonAttemptDir, 'replay.ad'), 'open "Demo"\n'); + fs.writeFileSync(path.join(daemonAttemptDir, 'result.txt'), 'status: passed\n'); + + const request: DaemonRequest = { + token: 'daemon-token', + session: 'default', + command: 'test', + positionals: [], + meta: { clientArtifactPaths: { artifactsDir: clientRoot } }, + }; + const suite: ReplaySuiteResult = { + total: 1, + executed: 1, + passed: 1, + failed: 0, + skipped: 0, + notRun: 0, + durationMs: 1, + failures: [], + artifactsDir: daemonSuiteDir, + tests: [ + { + file: 'flow.ad', + session: 'default:test:1', + status: 'passed', + durationMs: 1, + attempts: 1, + artifactsDir: daemonAttemptDir, + replayed: 1, + healed: 0, + }, + ], + }; + const response = { + ok: true as const, + data: attachRemoteReplayTestArtifacts(suite, request), + }; + const artifact = response.data.artifacts?.[0]; + assert.ok(artifact?.path); + const artifactId = trackDownloadableArtifact({ + artifactPath: artifact.path, + artifactType: artifact.artifactType, + fileName: artifact.fileName, + }); + response.data.artifacts = [{ ...artifact, artifactId }]; + + const server = await createDaemonHttpServer({ + token: 'daemon-token', + handleRequest: async () => { + throw new Error('not exercised: this test only hits the /artifacts route'); + }, + }); + + try { + const port = await listenOnLoopback(server); + const materialized = await materializeRemoteArtifacts( + { baseUrl: `http://127.0.0.1:${port}`, token: 'daemon-token' }, + request, + response, + ); + + assert.equal(materialized.ok, true); + if (!materialized.ok) return; + assert.equal(materialized.data?.artifactsDir, clientSuiteDir); + assert.equal(materialized.data?.artifacts?.[0]?.localPath, clientSuiteDir); + assert.equal( + fs.readFileSync(path.join(clientSuiteDir, 'flow.ad', 'attempt-1', 'replay.ad'), 'utf8'), + 'open "Demo"\n', + ); + assert.equal( + fs.readFileSync(path.join(clientSuiteDir, 'flow.ad', 'attempt-1', 'result.txt'), 'utf8'), + 'status: passed\n', + ); + } finally { + cleanupDownloadableArtifact(artifactId); + await closeLoopbackServer(server); + fs.rmSync(tempDir, { recursive: true, force: true }); + } +}); + +test('directory downloads stage beside the destination and finish timeout cleanup before rejecting', async (t) => { + if (await skipWhenLoopbackUnavailable(t, 'remote test-artifacts atomic staging coverage')) return; + + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-remote-suite-timeout-')); + const clientRoot = path.join(tempDir, 'caller-side', 'artifacts'); + const server = http.createServer((_req, res) => { + res.statusCode = 200; + res.write('partial archive'); + }); + + try { + const port = await listenOnLoopback(server); + const download = downloadRemoteArtifact({ + baseUrl: `http://127.0.0.1:${port}`, + token: 'daemon-token', + artifactId: 'stalled-directory', + destinationPath: clientRoot, + requestScope: {}, + isDirectory: true, + timeoutMs: 80, + }); + await new Promise((resolve) => setTimeout(resolve, 30)); + const stagingWhileActive = fs.existsSync(clientRoot) + ? fs.readdirSync(clientRoot).filter((entry) => entry.startsWith('.agent-device-download-')) + : []; + await assert.rejects(download, /timed out/); + assert.equal(stagingWhileActive.length, 1); + assert.deepEqual( + fs.existsSync(clientRoot) + ? fs.readdirSync(clientRoot).filter((entry) => entry.startsWith('.agent-device-download-')) + : [], + [], + ); + } finally { + await closeLoopbackServer(server); + fs.rmSync(tempDir, { recursive: true, force: true }); + } +}); diff --git a/test/wire-compat/ledger.json b/test/wire-compat/ledger.json index 7667d5a130..9d186e6528 100644 --- a/test/wire-compat/ledger.json +++ b/test/wire-compat/ledger.json @@ -8,7 +8,7 @@ "packages/kernel/src/contracts.ts#CommandRpcParams": "sha256:3a400b0d815920d9cf4c6236531931f6ba6f9debf7ac26915e0116b493f1c97a", "packages/kernel/src/contracts.ts#DAEMON_LOCK_POLICIES": "sha256:625cc33bb574aa6119add45878a0f6adc445244edc2957737da975f91605c786", "packages/kernel/src/contracts.ts#DaemonArtifact": "sha256:967c919d878b83b6e0324c56c1bf41e63b0c31c2e42cf41cd5a12902c2b5034a", - "packages/kernel/src/contracts.ts#DaemonArtifactKnownType": "sha256:fecca9e18bc5e84ca9c4341d28be2fc16c228d92cde1b0746014e18dfad9568d", + "packages/kernel/src/contracts.ts#DaemonArtifactKnownType": "sha256:21886407c3fbc30d8c0c544503b831c78db2408aeaef1b9b2c6a4139144aef28", "packages/kernel/src/contracts.ts#DaemonArtifactType": "sha256:5e5017276fefe880b8ab32001888011d2e02a8670f0978afdb9b56a77ab37f09", "packages/kernel/src/contracts.ts#DaemonInstallSource": "sha256:fe26e202e62d997cdd3855fd96002511783a6c26a9ce27210d7c6cc084c66e4e", "packages/kernel/src/contracts.ts#DaemonLockPolicy": "sha256:f33d0ce7200a7d97c11e91bd2889b5d9bc7f35935a5370a29930118d091468f4", @@ -136,12 +136,14 @@ "src/daemon/upload-http.ts#resolveUploadHttpRoute": "sha256:8837e62933cf57111a8070454877c5859aed65b4dcdba755f145346e30f82835", "src/daemon/upload-http.ts#sendJson": "sha256:c2317b4b14a10731684c9160038005965856150f05f8923765a3fa833d2ecae9", "src/daemon/upload-http.ts#sendUploadedArtifactResponse": "sha256:9e418403448d58a96284fed605762fa71f9349bf2c70ad79df69b2b3e27487d6", + "src/remote/artifact-download.ts#RemoteArtifactDownload": "sha256:f73a888f4f8c9d603701c505bb342787b1ca6559a13b15da2385e60349ecea8e", + "src/remote/artifact-download.ts#downloadRemoteArtifactFromUrl": "sha256:5176697267907e3f5e4383a881969721e4ea3f6776128d28c313bdc210128834", "src/remote/daemon-artifacts.ts#DaemonArtifactEndpoint": "sha256:45226717ac52c0af2a62f74e91549582d95f4a4abc49be87989e026291a88c82", - "src/remote/daemon-artifacts.ts#DownloadRemoteArtifactParams": "sha256:24f3e577471874ae9147373d6ea0a9108e5a21a39742d2e3931ff1a6a323cd8e", + "src/remote/daemon-artifacts.ts#DownloadRemoteArtifactParams": "sha256:3ec63be2dbdb542e30b19d1500bc16874607823afb7ae392e92b0b535865859e", "src/remote/daemon-artifacts.ts#buildDaemonArtifactUrl": "sha256:06dcde057809b507f70f7bb5cdef9bde6515be84b67e45865cf19e70b8636208", - "src/remote/daemon-artifacts.ts#downloadRemoteArtifact": "sha256:3db8ad2b4660f813583aa9b87e1c3927adbc3ca1e84cdd0d181d82fdc85d747a", + "src/remote/daemon-artifacts.ts#downloadRemoteArtifact": "sha256:f3ab09ce387577073ecc5bcd2528963e45f91b2c1e4cc57847c3e2732ccbb300", "src/remote/daemon-artifacts.ts#isRemoteDaemon": "sha256:0f6a714c5f60ce4e8548d811b516d8b6241eca9f7e5414733732da395b8a00e7", - "src/remote/daemon-artifacts.ts#materializeRemoteArtifacts": "sha256:1dcab98fba2674f87053ae888c2235b6a6780405aa70648dc57eb6b9077a02fd", + "src/remote/daemon-artifacts.ts#materializeRemoteArtifacts": "sha256:6a62429723f52231951ef263950f4b304529bad984ab216b73db1750f58c7f84", "src/remote/daemon-artifacts.ts#resolveMaterializedArtifactPath": "sha256:db5c44effc41ff9fa40bd094945e56ac60fac89a42fa68c65c9293e6de6fb1dc", "src/remote/remote-request-diagnostics.ts#RemoteDaemonErrorPayload": "sha256:8fbcbcdf4bae7b66a08e465a72593794f36364759f18f1a814cd39dfdb76f309", "src/remote/remote-request-diagnostics.ts#RemoteDiagnosticsEndpoint": "sha256:bdf63219ecea77d1290f0f5eeef6d5adc55077fa2fa6750ba4608aa3efd540f0", @@ -173,6 +175,26 @@ "src/remote/upload-stream.ts#streamFileToHttpRequestAttempt": "sha256:da39a79fa7c1f81e55caf613eedc347c0f3a9a9711b265a4db185677532d9552" }, "compatibleChanges": [ + { + "declaration": "packages/kernel/src/contracts.ts#DaemonArtifactKnownType", + "digest": "sha256:21886407c3fbc30d8c0c544503b831c78db2408aeaef1b9b2c6a4139144aef28", + "rationale": "#2246 adds the 'test-artifacts' literal for the new remote `test` suite-directory artifact. DaemonArtifactType already widens to `(string & {})`, so an artifactType value outside the known set was always valid on the wire; the literal only sharpens the type hint for code that opts into recognizing it. A released peer that does not know it either never receives it (an old client's daemon-artifacts.ts has no 'test' redirect, so it never sets clientArtifactPaths.artifactsDir, so a new daemon's attachRemoteReplayTestArtifacts never attaches this artifact) or, receiving it from an old daemon that cannot produce it, is a case that cannot occur." + }, + { + "declaration": "src/remote/daemon-artifacts.ts#DownloadRemoteArtifactParams", + "digest": "sha256:3ec63be2dbdb542e30b19d1500bc16874607823afb7ae392e92b0b535865859e", + "rationale": "#2246 adds the optional isDirectory field. It is pure client-local state — never serialized, never sent to the daemon — that tells the CLIENT'S OWN download logic to extract a tar body instead of writing it verbatim; the GET /artifacts/:id request and response framing are unchanged. Every existing call site (screenshot, recording) omits it and keeps writing a single file exactly as before." + }, + { + "declaration": "src/remote/daemon-artifacts.ts#downloadRemoteArtifact", + "digest": "sha256:f3ab09ce387577073ecc5bcd2528963e45f91b2c1e4cc57847c3e2732ccbb300", + "rationale": "#2246 selects a file or directory destination once. Directory downloads stage and safely extract beside the destination, propagate timeout cancellation through both operations, and atomically rename the complete top-level entry; returning that local path is client-only state. The GET /artifacts/:id request and response framing, and every existing file artifact call site's behavior, are unchanged." + }, + { + "declaration": "src/remote/daemon-artifacts.ts#materializeRemoteArtifacts", + "digest": "sha256:6a62429723f52231951ef263950f4b304529bad984ab216b73db1750f58c7f84", + "rationale": "#2246 passes isDirectory only for the new test-artifacts type and uses the exact path the client materializer published. Existing screenshot and recording artifacts still report their requested file path; only the new directory response gains its suite-invocation segment." + }, { "declaration": "src/daemon/client/daemon-client-rpc.ts#buildHttpRpcPayload", "digest": "sha256:efa9a5da4c7288cceae5656b468946ad3a2af0f0f0fb940f8a37987f922aa4b8", diff --git a/test/wire-compat/surface.ts b/test/wire-compat/surface.ts index e7f0c534e2..6e45a744df 100644 --- a/test/wire-compat/surface.ts +++ b/test/wire-compat/surface.ts @@ -57,6 +57,7 @@ const CLIENT_PROGRESS = 'src/daemon/client/daemon-client-progress.ts'; const CLIENT_TRANSPORT = 'src/daemon/client/daemon-client-transport.ts'; const UPLOAD_CLIENT = 'src/remote/upload-client.ts'; const REMOTE_ARTIFACTS = 'src/remote/daemon-artifacts.ts'; +const ARTIFACT_DOWNLOAD = 'src/remote/artifact-download.ts'; const UPLOAD_STREAM = 'src/remote/upload-stream.ts'; function from(file: string, ...names: string[]): WireDeclarationRef[] { @@ -384,6 +385,7 @@ export const WIRE_SURFACE: readonly WireSurfaceGroup[] = [ 'materializeRemoteArtifacts', 'resolveMaterializedArtifactPath', ), + ...from(ARTIFACT_DOWNLOAD, 'RemoteArtifactDownload', 'downloadRemoteArtifactFromUrl'), ], }, ]; diff --git a/test/wire-compat/wire-mutations.test.ts b/test/wire-compat/wire-mutations.test.ts index e9309e1de6..6337001dca 100644 --- a/test/wire-compat/wire-mutations.test.ts +++ b/test/wire-compat/wire-mutations.test.ts @@ -201,8 +201,8 @@ const MUTATIONS: readonly WireMutation[] = [ }, { breakClass: 'artifact consumer: the download request drops its tenant header', - file: 'src/remote/daemon-artifacts.ts', - name: 'downloadRemoteArtifact', + file: 'src/remote/artifact-download.ts', + name: 'downloadRemoteArtifactFromUrl', from: '...buildDaemonHttpTenantHeaders(params.requestScope?.tenantId),', to: '', },