Fix locked plan transfer across worktrees - #214
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The new transfer path is not bound to admitted artifact identity, allowing workspace transitions to commit after copying stale or substituted product evidence. Model-level verification is not required before merge; targeted freshness and negative transaction tests should cover this defect.
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch leaves approval substitution undetected, prevents existing schema-4 installations from updating, and introduces a concrete workspace activation zero-progress path. Model-level verification is not required to establish these defects; targeted regression tests are sufficient before merge.
Findings without inline diff anchors
[P1] Preserve the approval fingerprint result during semantic validation
Invariant: approved or locked plans remain authoritative only while the approval bytes match the durable ApprovalFingerprint. After approval, replace the artifact with different but schema-valid JSON (for example, change Actor); line 528 makes valid false, but line 535 overwrites it with semantic validity, so observation still reports the plan approved. This patch introduced the fingerprint check without composing it with the existing validation. The substituted approval can authorize later delivery transitions. Add an observer test asserting that any strict-valid approval whose bytes differ from ApprovalFingerprint makes the plan and terminal stale.
Confidence: 0.99
Location: boatstack/internal/softwaredelivery/plant/observer.go:528-535 (RIGHT)
bigboateng
left a comment
bigboateng
left a comment
There was a problem hiding this comment.
Cycle-two update: approval-byte validation now composes with semantic validation and has an observer regression test. Existing schema-4 records remain readable; those without an approval fingerprint are observed stale and require reapproval. Focused durable, effects, plant, and helper tests passed.
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch introduces an authority/receipt violation at worktree installation, a resolver/apply disagreement, and an unversioned durable ABI change. Model-level verification is recommended after fixes, especially for interruption and workspace.reconcile behavior between worktree creation and artifact installation.
| planMutation, err := mutationFor(filepath.Join(destinationRoot, "plans", deliveryID+".source"), planRaw, 0o644, false, false) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| approvalMutation, err := mutationFor(filepath.Join(destinationRoot, "approvals", deliveryID+".json"), approvalRaw, 0o644, false, false) |
There was a problem hiding this comment.
[P1] Bind destination artifacts only after establishing their actual paths
Invariant: staged mutations must bind the actual prior resource and remain inside the admitted workspace. Prepare calls this while the destination does not yet exist, so mutationFor always records PriorExists=false. Then git worktree add can check out either a tracked artifact or a .boatstack/plans/approvals symlink before installation. The former is overwritten but receipted as a create and deleted on rollback; the latter redirects the write outside the workspace while verification still succeeds through the symlink. This is introduced by the new destination writes and can corrupt external files or produce false receipts/recovery data. A regression test should cut from a base ref containing both a tracked target and a symlinked artifact directory, asserting refusal, no external write, and exact prior facts.
Confidence: 0.99
| if workspacePath == "" || expectedApprovalFingerprint == "" { | ||
| return nil, fmt.Errorf("workspace plan transfer requires destination and exact approval for a bound plan") |
There was a problem hiding this comment.
[P2] Do not require approval for every bound plan
Invariant: any workspace.cut prescribed from an admissible snapshot must pass deterministic preparation for that same snapshot. The catalog permits cutting with a draft, valid, or stale plan, but those states have a nonempty plan fingerprint and legitimately no approval fingerprint. Resolution therefore prescribes a targeted workspace.cut, preparation rejects it here, state remains unchanged, and retry repeats the refusal. Legacy approved schema-4 states accepted by this patch provide another witness because their approval fingerprint is empty. The patch introduces this disagreement by treating every bound plan as approved. A regression test should resolve and apply workspace.cut from a draft plan, and separately verify that a legacy/stale plan is refused during resolution or has a defined transfer path.
Confidence: 0.99
| RuntimeFingerprint string `json:"runtime_fingerprint,omitempty"` | ||
| RuntimeSource string `json:"runtime_source_revision,omitempty"` | ||
| PlanFingerprint string `json:"plan_fingerprint,omitempty"` | ||
| ApprovalFingerprint string `json:"approval_fingerprint,omitempty"` |
There was a problem hiding this comment.
[P2] Version the durable state ABI for the new field
Invariant: equal state-schema identities must denote mutually compatible durable wire formats. Head now emits approval_fingerprint while retaining schema version 4; the base runtime's strict decoder rejects that unknown field. Thus after head commits an approval, an exact base candidate cannot even observe the state to perform an otherwise legal runtime downgrade/update, despite both pins declaring schema 4. The new legacy test checks only that the current decoder reads field-absent bytes and does not exercise the real old decoder. A regression test should use the actual base decoder against a head-emitted approved state and require either compatibility or a distinct schema identity with an explicit migration path.
Confidence: 0.96
1 participant
Summary
Verification
go test ./internal/softwaredelivery/effects ./cmd/boatstack-helperworkspace.cut, the target worktree retained the locked plan and selectedworkspace.activaterather than recreating a plan