From 6990e5538a031bc86f5ced1c55474e31077c9618 Mon Sep 17 00:00:00 2001 From: Relayflow Date: Sun, 20 Sep 2026 17:10:03 +0000 Subject: [PATCH] fix: journal agent artifacts written under dot-directories (#512) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The worker's artifact scan skipped every entry whose name began with a dot, so nothing an agent wrote under `.workflow-artifacts/` — the conventional artifact directory — ever reached `output.artifacts`. An `artifact_exists` gate naming such a path could therefore never pass: it failed as a bare `retries_exhausted exit=1` on a file sitting on disk, with nothing in the completion to say why. Replace the blanket dot-prefix skip with an exact-name set (`.git`, `.relayflowd`, `node_modules`) applied at any depth before the entry's type is consulted, so a linked worktree's regular `.git` file is skipped too while `.github` and `.relayflowd-notes` stay eligible. The gate still reads the journal and never the disk; the fix is that the journal now describes what the agent wrote. Co-Authored-By: Claude --- docs/SURFACE.md | 63 +++++++++++++------ packages/sdk/src/agent-artifacts.ts | 40 ++++++++---- .../sdk/tests/agent-artifacts-live.test.ts | 23 ++++--- packages/sdk/tests/agent-artifacts.test.ts | 56 +++++++++++++++-- packages/sdk/tests/artifact-gates.test.ts | 33 ++++++++-- 5 files changed, 166 insertions(+), 49 deletions(-) diff --git a/docs/SURFACE.md b/docs/SURFACE.md index 4620b56ea..b1c7f62ac 100644 --- a/docs/SURFACE.md +++ b/docs/SURFACE.md @@ -1126,32 +1126,55 @@ whose final message is a JSON object owns its output shape and journals no artifacts; gate such a step on a deterministic check instead. The relay transport journals none, because the agent ran on another host. +What the scan reports is every **regular file** under the working directory +that is new, or whose content changed, between the two snapshots — content +(size + sha256), not mtime, so a rewrite inside the filesystem's timestamp +resolution still counts. Symlinks are not followed. Three entry names are +skipped, matched **exactly**, at any depth, before the entry's type is +consulted: `.git` (a directory, or the regular file a linked worktree has), +`.relayflowd` (the default data dir) and `node_modules`. Exact names, not +prefixes — `.github`, `.relayflowd-notes` and every other author-chosen name +that merely starts the same way is scanned normally. + +**Dot-directories are artifacts.** `.workflow-artifacts/` is the conventional +place a flow tells its agents to write, so +`.gate({ type: 'artifact_exists', path: '.workflow-artifacts/x/y.md' })` is an +ordinary gate and the path appears verbatim in `output.artifacts`. An earlier +scanner skipped every entry whose name began with a dot; that made a whole +class of author-chosen paths invisible to the journal, and a gate naming one +could never pass — it failed on a file that was sitting on disk, with nothing +in the completion to say why (flows#512). + The diff is of the working directory, not of what the agent did, so anything written under it during the attempt is an artifact by default — including files -the runtime itself writes. The worker's own per-attempt evidence lives under +the runtime itself writes, and writes by any unrelated process that happens to +touch the tree. A content hash proves a change during the interval; it does not +prove the agent made it. The worker's own per-attempt evidence lives under `/runs//steps//` (the transcript file, the `attempt-..tail` files and the `.tmp` each tail is staged as), and a -local `--data-dir` inside the project puts all of it inside the scanned tree. -Every one of those paths is excluded from the diff by name in -`packages/sdk/src/worker-cli.ts`'s `ownEvidencePaths`, derived from the attempt -identity — the same input that decides where each file is written, so the -exclusion cannot drift from the files. Deriving it from anything the run -*produces* is a mistake worth naming: an earlier version read the transcript's -path off `result.transcript.file`, which `finish` omits when the close outruns -its deadline or the attempt aborts, so the exclusion lapsed on exactly the paths -where the file is slowest to finish and most likely to still be sitting there. - -Anyone adding a new runtime-written file under the run's data dir has to add it -to `ownEvidencePaths` in the same change. **Missing one is silent by default.** -`step.complete` bounds `trajectory_tail` and passes `output` through verbatim -(`kernel/relayflowd/src/server.rs`), so the kernel accepts the polluted list and -the run succeeds with the worker's own bookkeeping journaled as the agent's +local `--data-dir` inside the project puts all of it inside the scanned tree — +under any name the caller chose, which is usually not one of the three skipped +above. So `packages/sdk/src/worker-cli.ts` drops the **entire configured data +directory subtree** from the diff, comparing symlink-resolved paths against the +data dir the step was dispatched with. That is the attempt's own identity, the +same input that decides where each file is written, so the exclusion cannot +drift from the files. Deriving it from anything the run *produces* is a mistake +worth naming: an earlier version listed each runtime-written file by name and +read the transcript's path off `result.transcript.file`, which `finish` omits +when the close outruns its deadline or the attempt aborts, so the exclusion +lapsed on exactly the paths where the file is slowest to finish and most likely +to still be sitting there. A subtree exclusion has nothing to enumerate and so +nothing to forget. + +Pollution of that list is silent by default. `step.complete` bounds +`trajectory_tail` and passes `output` through verbatim +(`kernel/relayflowd/src/server.rs`), so the kernel accepts whatever list the +worker sends and the run succeeds with it journaled as the agent's `output.artifacts`. It only becomes loud where something reads that list: an `artifact_exists` gate on a path that is now crowded, or a flow body that -asserts on `AgentResult.artifacts` — which is how this was caught at all, by -`packages/sdk/tests/agent-transcript-live.test.ts` failing its own -`artifacts.length !== 0` check. Dotfiles and dotdirs are skipped by the walk, so -`.relayflowd` is already invisible; a data dir under any other name is not. +asserts on `AgentResult.artifacts` — which is how the data-dir case was caught +at all, by `packages/sdk/tests/agent-transcript-live.test.ts` failing its own +`artifacts.length !== 0` check. - Are YAML helper verbs (`slack:`, `mcp:`) core spec vocabulary or compile-time expansion into `run`/effect steps? Leaning: expansion — the kernel spec stays seven words; helpers stay a surface concern. - Helper generation cadence: generated from relayfile adapter manifests at build time vs published per-adapter packages. Leaning: generated, with hand-tuned verb names for the top providers. diff --git a/packages/sdk/src/agent-artifacts.ts b/packages/sdk/src/agent-artifacts.ts index c7f02b50a..3b34910ad 100644 --- a/packages/sdk/src/agent-artifacts.ts +++ b/packages/sdk/src/agent-artifacts.ts @@ -6,7 +6,14 @@ function isEnoent(error: unknown): boolean { return error instanceof Error && (error as NodeJS.ErrnoException).code === 'ENOENT'; } -const SKIPPED_DIR_NAMES = new Set(['node_modules']); +/** + * Entry names never walked and never reported, matched exactly, at any depth, + * before the entry's type is consulted — so a `.git` *file* (a worktree's + * gitdir pointer) is skipped as surely as a `.git` directory. Exact names, not + * prefixes: `.github`, `.relayflowd-notes` and every other author-chosen name + * that merely starts the same way stays eligible. + */ +const SKIPPED_ENTRY_NAMES = new Set(['.git', '.relayflowd', 'node_modules']); /** * A recursive snapshot of every regular file under `dir`, keyed by its @@ -14,17 +21,24 @@ const SKIPPED_DIR_NAMES = new Set(['node_modules']); * Content, not mtime: a fast rewrite inside the filesystem's timestamp * resolution must still count as a change — the same rule * `examples/research/shims/agent-cli.ts`'s `snapshot()` uses for the same - * "did the agent actually write something" question. Dotfiles/dotdirs - * (`.git`, a default `.relayflowd` data dir, editor swap files, ...) are never - * agent-authored content and are skipped, as is `node_modules`. A data dir the - * caller named explicitly is not a dotdir and so is not covered here; the - * caller excludes it from the diff instead (`worker-cli.ts`). - * A missing `dir` (an agent step whose cwd does not exist yet) yields an - * empty snapshot rather than throwing. Only a vanished path (`ENOENT`) is - * ever swallowed this way; any other filesystem error (permissions, - * `ENOTDIR`, `EISDIR`, ...) propagates, because a step whose artifact scan - * silently dropped files it could not read must not report a successful, - * incomplete `artifacts` list as if it were the truth. + * "did the agent actually write something" question. + * + * Dotfiles and dot-directories ARE eligible. `.workflow-artifacts/` is the + * conventional place a flow tells its agents to write, and a blanket dot-prefix + * skip made every review, report and evidence file written there invisible to + * the journal — and so to an `artifact_exists` gate naming one, which could + * then never pass. Only the three names in `SKIPPED_ENTRY_NAMES` are excluded: + * `.git`, the default `.relayflowd` data dir, and `node_modules`. A data dir + * the caller named explicitly is not one of those; the caller drops that whole + * subtree from the diff instead (`worker-cli.ts`). + * + * Only regular files are signed; symlinks are not followed and directories are + * descended, not recorded. A missing `dir` (an agent step whose cwd does not + * exist yet) yields an empty snapshot rather than throwing. Only a vanished + * path (`ENOENT`) is ever swallowed this way; any other filesystem error + * (permissions, `ENOTDIR`, `EISDIR`, ...) propagates, because a step whose + * artifact scan silently dropped files it could not read must not report a + * successful, incomplete `artifacts` list as if it were the truth. */ export async function snapshotWorkspaceFiles(dir: string): Promise> { const out = new Map(); @@ -41,7 +55,7 @@ async function walk(root: string, current: string, out: Map): Pr throw error; } for (const entry of entries) { - if (entry.name.startsWith('.') || SKIPPED_DIR_NAMES.has(entry.name)) continue; + if (SKIPPED_ENTRY_NAMES.has(entry.name)) continue; const path = join(current, entry.name); if (entry.isDirectory()) { await walk(root, path, out); diff --git a/packages/sdk/tests/agent-artifacts-live.test.ts b/packages/sdk/tests/agent-artifacts-live.test.ts index 3dbf90f3a..bb441a04b 100644 --- a/packages/sdk/tests/agent-artifacts-live.test.ts +++ b/packages/sdk/tests/agent-artifacts-live.test.ts @@ -118,16 +118,19 @@ function reportedRunIds(report: { runId?: string; diagnostics: Array<{ message: } describe('agent artifacts and gates through the built CLI, a real daemon and the local agent', () => { - it('journals the files the agent wrote, and both artifact gates pass on that journal', async () => { + it('journals the files the agent wrote, including under a dot-directory, and every artifact gate passes on that journal', async () => { const f = fixture(` const security = await f.agent('security-reviewer', { task: 'review write:review/security.md' }) .gate({ type: 'artifact_exists', path: 'review/security.md' }); + const evidence = await f.agent('evidence-writer', { task: 'review write:.workflow-artifacts/x/y.md' }) + .gate({ type: 'artifact_exists', path: '.workflow-artifacts/x/y.md' }); const correctness = await f.agent('correctness-reviewer', { task: 'review write:review/correctness.md' }) .gate(r => r.artifacts.includes('review/correctness.md'), 'the reviewer must write its findings'); - const both = await f.run('ls review'); + const both = await f.run('ls review .workflow-artifacts/x'); if (!security.artifacts.includes('review/security.md')) throw new Error('security artifacts missing'); + if (!evidence.artifacts.includes('.workflow-artifacts/x/y.md')) throw new Error('evidence artifacts missing'); if (!correctness.artifacts.includes('review/correctness.md')) throw new Error('correctness artifacts missing'); - if (!both.includes('security.md') || !both.includes('correctness.md')) throw new Error('files not on disk'); + if (!both.includes('security.md') || !both.includes('correctness.md') || !both.includes('y.md')) throw new Error('files not on disk'); f.done('success');`); const result = f.invoke(); expect(result.status, result.stderr + result.stdout).toBe(0); @@ -138,22 +141,28 @@ describe('agent artifacts and gates through the built CLI, a real daemon and the // The named gate is lowered INTO the agent's own kernel run (`agent-1` + // `agent-1.gate` in one spec); the predicate gate is its own lowered run; // then the ls and the terminal marker. Every one is a journaled kernel step. - expect(journalSteps.map(s => s.id)).toEqual(['agent-1', 'agent-2', 'agent-2.gate', 'run-3', 'complete-4']); + expect(journalSteps.map(s => s.id)).toEqual(['agent-1', 'agent-2', 'agent-3', 'agent-3.gate', 'run-4', 'complete-5']); expect(completed.has('agent-1.gate')).toBe(true); // The worker journaled the artifacts in the step output — the fact the gates read. const agent1 = completed.get('agent-1')!.payload.output as { artifacts?: string[]; stdout_tail?: string }; expect(agent1.artifacts).toEqual(['review/security.md']); expect(agent1.stdout_tail).toContain('reviewed'); - expect((completed.get('agent-2')!.payload.output as { artifacts?: string[] }).artifacts).toEqual(['review/correctness.md']); + // A dot-directory artifact reaches the journal under its exact relative + // POSIX path. It used to be dropped by the scanner, so an + // `artifact_exists` gate naming one could never pass. + expect((completed.get('agent-2')!.payload.output as { artifacts?: string[] }).artifacts) + .toEqual(['.workflow-artifacts/x/y.md']); + expect(completed.get('agent-2.gate')!.payload.completionReason).toBe('success'); + expect((completed.get('agent-3')!.payload.output as { artifacts?: string[] }).artifacts).toEqual(['review/correctness.md']); // Named gate: a deterministic step reading FLOWS_INPUT, passed. expect(completed.get('agent-1.gate')!.payload.completionReason).toBe('success'); // Predicate gate: the recorded verdict, with the author's reason. - const predicate = completed.get('agent-2.gate')!.payload; + const predicate = completed.get('agent-3.gate')!.payload; expect(predicate.completionReason).toBe('success'); expect((predicate.output as { stdout_tail: string }).stdout_tail) - .toBe(JSON.stringify({ gate: 'predicate', step: 'agent-2', verdict: 'pass', because: 'the reviewer must write its findings' })); + .toBe(JSON.stringify({ gate: 'predicate', step: 'agent-3', verdict: 'pass', because: 'the reviewer must write its findings' })); }, 120_000); it('fails the run when the artifact_exists gate names a file the agent did not write', async () => { diff --git a/packages/sdk/tests/agent-artifacts.test.ts b/packages/sdk/tests/agent-artifacts.test.ts index 59c0d6c62..52be9df62 100644 --- a/packages/sdk/tests/agent-artifacts.test.ts +++ b/packages/sdk/tests/agent-artifacts.test.ts @@ -41,13 +41,59 @@ describe('snapshotWorkspaceFiles / diffWorkspaceFiles', () => { expect(diffWorkspaceFiles(before, after)).toEqual([]); }); - it('skips dotdirs and node_modules', async () => { + it('reports a file the agent wrote under a dot-directory', async () => { + // `.workflow-artifacts/` is the conventional artifact directory; a blanket + // dot-prefix skip made every file written there invisible to the journal, + // so an `artifact_exists` gate naming one could never pass. const dir = tempDir(); + mkdirSync(join(dir, '.workflow-artifacts/reviews'), { recursive: true }); + writeFileSync(join(dir, '.workflow-artifacts/reviews/edited.md'), 'aaaa'); + writeFileSync(join(dir, '.workflow-artifacts/untouched.md'), 'same'); const before = await snapshotWorkspaceFiles(dir); - mkdirSync(join(dir, '.relayflowd')); - writeFileSync(join(dir, '.relayflowd', 'kernel.log'), 'noise'); - mkdirSync(join(dir, 'node_modules')); - writeFileSync(join(dir, 'node_modules', 'pkg.js'), 'noise'); + mkdirSync(join(dir, '.workflow-artifacts/x'), { recursive: true }); + writeFileSync(join(dir, '.workflow-artifacts/x/y.md'), 'findings'); + writeFileSync(join(dir, '.workflow-artifacts/reviews/edited.md'), 'bbbb'); + const after = await snapshotWorkspaceFiles(dir); + expect(diffWorkspaceFiles(before, after)) + .toEqual(['.workflow-artifacts/reviews/edited.md', '.workflow-artifacts/x/y.md']); + }); + + it('keeps ordinary dotfiles and author dot-directories eligible, including names that merely resemble exclusions', async () => { + const dir = tempDir(); + const before = await snapshotWorkspaceFiles(dir); + writeFileSync(join(dir, '.env.example'), 'KEY='); + mkdirSync(join(dir, '.github/workflows'), { recursive: true }); + writeFileSync(join(dir, '.github/workflows/ci.yml'), 'on: push'); + mkdirSync(join(dir, '.relayflowd-notes')); + writeFileSync(join(dir, '.relayflowd-notes/todo.md'), 'later'); + mkdirSync(join(dir, 'evidence/.nested/.deeper'), { recursive: true }); + writeFileSync(join(dir, 'evidence/.nested/.deeper/proof.md'), 'proof'); + const after = await snapshotWorkspaceFiles(dir); + expect(diffWorkspaceFiles(before, after)).toEqual([ + '.env.example', + '.github/workflows/ci.yml', + '.relayflowd-notes/todo.md', + 'evidence/.nested/.deeper/proof.md', + ]); + }); + + it('skips exactly .git, .relayflowd and node_modules, at the root and nested', async () => { + const dir = tempDir(); + const before = await snapshotWorkspaceFiles(dir); + for (const name of ['.git', '.relayflowd', 'node_modules']) { + mkdirSync(join(dir, name)); + writeFileSync(join(dir, name, 'noise.txt'), 'noise'); + mkdirSync(join(dir, 'sub', name), { recursive: true }); + writeFileSync(join(dir, 'sub', name, 'noise.txt'), 'noise'); + } + const after = await snapshotWorkspaceFiles(dir); + expect(diffWorkspaceFiles(before, after)).toEqual([]); + }); + + it('skips a regular .git file, as a linked worktree has', async () => { + const dir = tempDir(); + const before = await snapshotWorkspaceFiles(dir); + writeFileSync(join(dir, '.git'), 'gitdir: /elsewhere/.git/worktrees/w\n'); const after = await snapshotWorkspaceFiles(dir); expect(diffWorkspaceFiles(before, after)).toEqual([]); }); diff --git a/packages/sdk/tests/artifact-gates.test.ts b/packages/sdk/tests/artifact-gates.test.ts index 4983684ad..ebdd0a5cb 100644 --- a/packages/sdk/tests/artifact-gates.test.ts +++ b/packages/sdk/tests/artifact-gates.test.ts @@ -47,7 +47,7 @@ afterEach(() => { onSpawn = undefined; for (const d of dirs.splice(0)) rmSync(d, function tempDir(): string { const d = mkdtempSync(join(tmpdir(), 'artifact-gates-')); dirs.push(d); return d; } describe('worker-side artifacts', () => { - it('journals the files the CLI created or changed in its cwd, content-hashed, dotdirs and node_modules excluded', async () => { + it('journals the files the CLI created or changed in its cwd, content-hashed, including dot-directories, with .git and node_modules excluded', async () => { const cwd = tempDir(); mkdirSync(join(cwd, 'review')); writeFileSync(join(cwd, 'review/existing.md'), 'v1'); @@ -56,14 +56,16 @@ describe('worker-side artifacts', () => { mkdirSync(join(dir!, 'review'), { recursive: true }); writeFileSync(join(dir!, 'review/security.md'), 'findings'); writeFileSync(join(dir!, 'review/existing.md'), 'v2'); - mkdirSync(join(dir!, '.cache'), { recursive: true }); - writeFileSync(join(dir!, '.cache/tmp'), 'x'); + mkdirSync(join(dir!, '.workflow-artifacts/x'), { recursive: true }); + writeFileSync(join(dir!, '.workflow-artifacts/x/y.md'), 'evidence'); + mkdirSync(join(dir!, '.git/objects'), { recursive: true }); + writeFileSync(join(dir!, '.git/objects/abc'), 'x'); mkdirSync(join(dir!, 'node_modules/dep'), { recursive: true }); writeFileSync(join(dir!, 'node_modules/dep/index.js'), 'x'); }; const result = await runAgentCli('claude', 'review', undefined, 'claude-opus-5', undefined, undefined, 'agent', undefined, cwd); expect(result.exit_code, result.stderr_tail).toBe(0); - expect(result.artifacts).toEqual(['review/existing.md', 'review/security.md']); + expect(result.artifacts).toEqual(['.workflow-artifacts/x/y.md', 'review/existing.md', 'review/security.md']); }); it('reports an empty list when nothing changed, and none at all for llm mode', async () => { @@ -98,6 +100,9 @@ describe('worker-side artifacts: concurrency', () => { describe('artifact_exists named gate', () => { it('validates a relative POSIX path and refuses escapes, absolute paths and NUL', () => { expect(namedGateErrors({ type: 'artifact_exists', path: 'review/security.md' }, undefined, 'g')).toEqual([]); + // The conventional artifact directory is a dot-directory; it is a legal + // gate path, and the scanner journals what an agent writes there. + expect(namedGateErrors({ type: 'artifact_exists', path: '.workflow-artifacts/x/y.md' }, undefined, 'g')).toEqual([]); for (const path of ['', ' ', '/etc/passwd', '../x', 'a/../b', './x', 'a//b', 'a\0b', 42]) { const errors = namedGateErrors({ type: 'artifact_exists', path }, undefined, 'g'); expect(errors, String(path)).toHaveLength(1); @@ -137,6 +142,26 @@ describe('artifact_exists named gate', () => { expect(() => run({ message: 'a JSON-speaking agent owns its output' })).toThrow(); }); + it('judges a dot-directory path by the journal alone: present in input passes with no such file, absent fails with one on disk', () => { + const spec = compileSpec({ version: '0.1.0', name: 'x', steps: [ + { id: 'shadow-rust', type: 'agent', instruction: 'i', cli: 'c', + verification: { type: 'artifact_exists', path: '.workflow-artifacts/reviews/shadow-rust.md' } }, + ] }); + const command = (lowerNamedGates(spec.steps)[1] as { command: string }).command; + const cwd = tempDir(); + const run = (artifacts: unknown) => execFileSync('sh', ['-c', command], { + cwd, env: { ...process.env, FLOWS_INPUT: JSON.stringify({ output: { exit_code: 0, stdout_tail: '', artifacts } }) }, stdio: 'pipe', + }); + // Journaled, never written here: the gate passes anyway. + expect(() => run(['.workflow-artifacts/reviews/shadow-rust.md'])).not.toThrow(); + // On disk, absent from the journal: the gate still fails. This is the + // shape the ticket reported, and it stays correct — the fix is that the + // scanner now journals the path, not that the gate consults the disk. + mkdirSync(join(cwd, '.workflow-artifacts/reviews'), { recursive: true }); + writeFileSync(join(cwd, '.workflow-artifacts/reviews/shadow-rust.md'), 'findings'); + expect(() => run([])).toThrow(); + }); + it('is preflightable: flows check prints it as a kernel exit_code gate', () => { const dir = tempDir(); writeFileSync(join(dir, 'flows.json'), JSON.stringify({ cli: 'true' }));