From de16205c01b00367f59ce836a0593949af0c573b Mon Sep 17 00:00:00 2001 From: Relayflow Lead Date: Sat, 29 Aug 2026 14:02:15 -0400 Subject: [PATCH 1/2] gate 3: refuse a work package that scopes files which do not exist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recovered work. Run fe94b247 built this and the capture fault ate it — its build log quotes the diff verbatim and the delivered patch contained none of it. The design was sound and visible, so rather than let a good change die to a platform defect, it is reimplemented here with its tests. Why it matters: the picker derives files_in_scope from prose in ops/BACKLOG.md, so a stale or mistyped entry yields a package that reads as perfectly actionable and sends whoever picks it up hunting for a file that was never there. For the component that proposes the system's own next task, a confidently wrong scope is worse than no scope. pathExists is injected and optional — absence means 'not my job', never 'assume missing' — so existing callers do not start failing because a new check exists. A test pins that specifically. Confirmed the tests FAIL against main before trusting them: × refuses a package scoping files that do not exist → expected true to be false × refuses when only some scoped files exist → expected true to be false Verified: sdk 175 passed across 13 files, tsc --noEmit clean. Co-Authored-By: Claude Fable 5 --- sdk/src/work-package-consumer.ts | 23 +++++++++++++++-- sdk/tests/work-package-consumer.test.ts | 33 +++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/sdk/src/work-package-consumer.ts b/sdk/src/work-package-consumer.ts index 1611a1b7c..38eb04a6f 100644 --- a/sdk/src/work-package-consumer.ts +++ b/sdk/src/work-package-consumer.ts @@ -10,23 +10,42 @@ export interface EmittedWorkPackage { export type WorkPackageRefusalReason = | 'missing_title' | 'missing_scope' - | 'missing_definition_of_done'; + | 'missing_definition_of_done' + | 'nonexistent_files'; export type WorkPackageConsumption = | { accepted: true; work: EmittedWorkPackage } | { accepted: false; reason: WorkPackageRefusalReason }; +/** Resolve a scoped path against the repository root; used to check existence. */ +export type PathExists = (path: string) => boolean; + /** * Validate an emitted package before admitting it as runnable work. * Refusals are data so callers must handle an unverifiable package explicitly. + * + * `pathExists` is optional and injected. When supplied, a package scoping files + * that are not there is refused: the picker derives files_in_scope from prose + * in ops/BACKLOG.md, so a stale or mistyped entry produces a package that reads + * as actionable and sends whoever picks it up looking for something that does + * not exist. Checking is cheap; a wrong scope is not. */ -export function consumeWorkPackage(input: unknown): WorkPackageConsumption { +export function consumeWorkPackage( + input: unknown, + pathExists?: PathExists, +): WorkPackageConsumption { if (!isRecord(input) || !isNonEmptyString(input['title'])) { return { accepted: false, reason: 'missing_title' }; } if (!isNonEmptyStringArray(input['files_in_scope'])) { return { accepted: false, reason: 'missing_scope' }; } + if (pathExists && isNonEmptyStringArray(input['files_in_scope'])) { + const missing = input['files_in_scope'].filter((p) => !pathExists(p)); + if (missing.length > 0) { + return { accepted: false, reason: 'nonexistent_files' }; + } + } if (!isNonEmptyStringArray(input['definition_of_done'])) { return { accepted: false, reason: 'missing_definition_of_done' }; } diff --git a/sdk/tests/work-package-consumer.test.ts b/sdk/tests/work-package-consumer.test.ts index 4b8d48846..219814cdc 100644 --- a/sdk/tests/work-package-consumer.test.ts +++ b/sdk/tests/work-package-consumer.test.ts @@ -106,3 +106,36 @@ describe('the Garden join: picker output feeds the consumer', () => { } }); }); + +describe('scope existence', () => { + const scoped = { + title: 'Real entry', + files_in_scope: ['sdk/src/a.ts', 'sdk/src/b.ts'], + definition_of_done: ['npm test'], + }; + + it('refuses a package scoping files that do not exist', () => { + // The picker derives files_in_scope from prose in ops/BACKLOG.md, so a + // stale or mistyped entry yields a package that reads as actionable and + // sends whoever picks it up hunting for a file that was never there. + const verdict = consumeWorkPackage(scoped, () => false); + expect(verdict.accepted).toBe(false); + expect(verdict.accepted === false && verdict.reason).toBe('nonexistent_files'); + }); + + it('refuses when only some scoped files exist', () => { + const verdict = consumeWorkPackage(scoped, (p) => p === 'sdk/src/a.ts'); + expect(verdict.accepted).toBe(false); + expect(verdict.accepted === false && verdict.reason).toBe('nonexistent_files'); + }); + + it('accepts when every scoped file exists', () => { + expect(consumeWorkPackage(scoped, () => true).accepted).toBe(true); + }); + + it('skips the check entirely when no pathExists is supplied', () => { + // Existing callers must not start failing because a new optional check + // exists. Absence of the checker means "not my job", not "assume missing". + expect(consumeWorkPackage(scoped).accepted).toBe(true); + }); +}); From 4e00fdb433e1608415564523f996242b0c44f92c Mon Sep 17 00:00:00 2001 From: Relayflow Lead Date: Sat, 29 Aug 2026 14:43:41 -0400 Subject: [PATCH 2/2] fix: the existence check is on by default, injectable only for testing (PR #28 P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review was right and my design was wrong. Making pathExists optional meant a caller using the one-argument API silently skipped the check — the guard did not guard, which is the exact failure this repo has hit five other times today. But the alternative the drive runs kept producing (import existsSync, check unconditionally, no seam) is untestable without a filesystem and resolves paths against whatever the CWD happens to be. So: default to the real filesystem, keep the seam for injection. The check is on for every caller that does nothing, and testable for callers that need it. Making it default-on broke three existing tests, which is the honest cost: they use fictional fixture paths and were being accepted only because nothing checked. They now inject a permissive checker explicitly, so each test isolates the refusal reason it actually exercises rather than depending on what happens to exist on disk. Verified: sdk 176 passed across 13 files, tsc --noEmit clean. Co-Authored-By: Claude Fable 5 --- sdk/src/work-package-consumer.ts | 18 ++++++++++++----- sdk/tests/work-package-consumer.test.ts | 27 ++++++++++++++++++------- 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/sdk/src/work-package-consumer.ts b/sdk/src/work-package-consumer.ts index 38eb04a6f..1f68b10fa 100644 --- a/sdk/src/work-package-consumer.ts +++ b/sdk/src/work-package-consumer.ts @@ -1,3 +1,5 @@ +import { existsSync } from 'node:fs'; + /** The work-package shape emitted at the SDK boundary. */ export interface EmittedWorkPackage { title: string; @@ -17,22 +19,28 @@ export type WorkPackageConsumption = | { accepted: true; work: EmittedWorkPackage } | { accepted: false; reason: WorkPackageRefusalReason }; -/** Resolve a scoped path against the repository root; used to check existence. */ +/** Resolve a scoped path; used to check that scoped files are really there. */ export type PathExists = (path: string) => boolean; +/** Default: ask the filesystem. Injectable so the check is testable. */ +const defaultPathExists: PathExists = (path) => existsSync(path); + /** * Validate an emitted package before admitting it as runnable work. * Refusals are data so callers must handle an unverifiable package explicitly. * - * `pathExists` is optional and injected. When supplied, a package scoping files - * that are not there is refused: the picker derives files_in_scope from prose + * The existence check is ON by default — review rejected making it opt-in + * (PR #28, P1): a caller using the one-argument API would silently skip it, so + * the guard would not guard. `pathExists` defaults to the real filesystem and + * is injectable purely so the behaviour can be tested without one. A package + * scoping files that are not there is refused: the picker derives files_in_scope from prose * in ops/BACKLOG.md, so a stale or mistyped entry produces a package that reads * as actionable and sends whoever picks it up looking for something that does * not exist. Checking is cheap; a wrong scope is not. */ export function consumeWorkPackage( input: unknown, - pathExists?: PathExists, + pathExists: PathExists = defaultPathExists, ): WorkPackageConsumption { if (!isRecord(input) || !isNonEmptyString(input['title'])) { return { accepted: false, reason: 'missing_title' }; @@ -40,7 +48,7 @@ export function consumeWorkPackage( if (!isNonEmptyStringArray(input['files_in_scope'])) { return { accepted: false, reason: 'missing_scope' }; } - if (pathExists && isNonEmptyStringArray(input['files_in_scope'])) { + if (isNonEmptyStringArray(input['files_in_scope'])) { const missing = input['files_in_scope'].filter((p) => !pathExists(p)); if (missing.length > 0) { return { accepted: false, reason: 'nonexistent_files' }; diff --git a/sdk/tests/work-package-consumer.test.ts b/sdk/tests/work-package-consumer.test.ts index 219814cdc..9dcef21c4 100644 --- a/sdk/tests/work-package-consumer.test.ts +++ b/sdk/tests/work-package-consumer.test.ts @@ -9,7 +9,10 @@ const validPackage = { async function consume(input: unknown) { const module = await import('../src/work-package-consumer.js'); - return module.consumeWorkPackage(input); + // Inject a permissive existence check: these tests exercise the OTHER + // refusal reasons, and should not also depend on whether fixture paths + // happen to exist on disk. + return module.consumeWorkPackage(input, () => true); } describe('work package consumer', () => { @@ -88,7 +91,7 @@ describe('the Garden join: picker output feeds the consumer', () => { run(step('read-backlog'), dir); run(step('select-entry'), dir); const accepted = JSON.parse(run(step('emit-package'), dir)); - expect(consumeWorkPackage(accepted).accepted, JSON.stringify(accepted)).toBe(true); + expect(consumeWorkPackage(accepted, () => true).accepted, JSON.stringify(accepted)).toBe(true); // An entry with no command names no way to verify itself. writeFileSync( @@ -98,7 +101,7 @@ describe('the Garden join: picker output feeds the consumer', () => { run(step('read-backlog'), dir); run(step('select-entry'), dir); const refused = JSON.parse(run(step('emit-package'), dir)); - const verdict = consumeWorkPackage(refused); + const verdict = consumeWorkPackage(refused, () => true); expect(verdict.accepted).toBe(false); expect(verdict.accepted === false && verdict.reason).toBe('missing_definition_of_done'); } finally { @@ -133,9 +136,19 @@ describe('scope existence', () => { expect(consumeWorkPackage(scoped, () => true).accepted).toBe(true); }); - it('skips the check entirely when no pathExists is supplied', () => { - // Existing callers must not start failing because a new optional check - // exists. Absence of the checker means "not my job", not "assume missing". - expect(consumeWorkPackage(scoped).accepted).toBe(true); + it('checks the real filesystem when no checker is injected', () => { + // Review rejected making this opt-in (PR #28, P1): a caller using the + // one-argument API would silently skip the check, so the guard would not + // guard. The default must reach the filesystem — these paths do not exist, + // so the package is refused without anyone passing a checker. + const verdict = consumeWorkPackage(scoped); + expect(verdict.accepted).toBe(false); + expect(verdict.accepted === false && verdict.reason).toBe('nonexistent_files'); + }); + + it('accepts a package scoping files that really are present', () => { + // Injection exists to make the check testable, not to disable it. + const real = { ...scoped, files_in_scope: ['package.json'] }; + expect(consumeWorkPackage(real).accepted).toBe(true); }); });