Implement planning-package contract v2. - #231
Conversation
![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.97
The patch introduces false approval provenance and two verifier soundness/completeness failures. Model-level verification is not required to establish these local counterexamples, though transaction/recovery modeling may be useful after correction.
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch permits a verification-to-promotion race that can commit different plan bytes, accepts internally contradictory package identities, and rejects valid nested paths on Windows. Model-level verification is recommended before merge, especially to check crash recovery across atomic tree installation, immutable approval creation, and state commit for zero-progress cycles.
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The new approval format diverges from the kernel authority contract, can enter a deterministic approval/refusal loop, and records incorrect identity-provider facts; the all-package verifier can also silently skip packages. Model-level verification is recommended before merge, particularly to check whether every partial or pre-existing package/approval state has a reachable recovery path and whether any other immutable-sidecar state can cycle without durable progress.
Findings without inline diff anchors
[P2] Do not treat an unadmitted approval sidecar as a valid package observation
Invariant: normal resolution must not prescribe approval when the immutable effect boundary already knows it cannot install that approval. Minimal failure: admit package P → repository adds a structurally valid, self-sealed but unadmitted approval.json → observation keeps state package-valid because approval status is ignored here → resolver selects planning.package.approve → immutablePlanningMutation refuses the different admitted approval → state remains unchanged and the same transition repeats. The new content-addressed immutable sidecar behavior creates this apply/resolver disagreement; previously approval overwrote the path. The package cannot progress without out-of-band deletion. Add a production-path test that inserts a valid foreign sidecar while state is package-valid and asserts resolution becomes stale/recoverable rather than prescribing an approval that preparation rejects.
Confidence: 0.98
Location: boatstack/internal/softwaredelivery/plant/observer.go:687-690 (RIGHT)
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch can commit false approval provenance, promote approval bytes different from those verified, and certify contracts outside the executable ABI. Model-level verification is recommended before merge, particularly: can interrupted atomic-tree admission or recovery enter a zero-progress state after partial filesystem failure?
Findings without inline diff anchors
[P1] Promote the same approval bytes that were verified
Invariant: promotion must publish the exact approval snapshot that passed verification. After verifyPlanningPackage returns valid, another process can replace only approval.json inside the still-identical directory; the directory SameFile check passes, and these lines reread and accept the replacement using only basic package fields, without checking its fingerprint, authority sources, manifest reference, plan-output ID, or durable ApprovalFingerprint. Promotion then copies the unverified bytes to the canonical approval, hashes them into state, and later workspaces inherit them. This is introduced by the new verify-then-reread V2 path. A regression test should mutate approval.json in the verification hook without replacing the directory and assert promotion refuses with zero mutations.
Confidence: 0.99
Location: boatstack/internal/softwaredelivery/effects/artifacts.go:272-277 (RIGHT)
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch introduces an approval-to-promotion TOCTOU violation, an invalid recovery receipt path, and an unsound approval verifier. Model-level verification is recommended before merge, particularly to check every atomic-tree crash point and whether recovery can commit facts matching its staged mutation manifest.
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch introduces an authority-freshness bypass, a concrete atomic-tree recovery refusal, and an unsound portable-lineage check. Model-level verification is recommended for the newly added atomic-tree recovery states, particularly whether every crash point retains a non-cycling resume or rollback path.
Findings without inline diff anchors
[P1] Bind identity provenance into authority freshness
Invariant: every authority field consumed by an effect must be bound to prescription/admission freshness. For a package awaiting approval, resolve with a human receipt using role developer and provider A, then apply the prescription with the same ID/class/subject/fingerprint but provider B. These lines omit both new provenance fields, so the authority fingerprint and prescription remain unchanged; apply succeeds and records provider B. Conversely, a receipt with no provenance passes resolution but is deterministically rejected during effect preparation, producing a repeatable zero-progress loop. This patch introduces both effect-relevant fields and the hidden approval precondition without fingerprinting or checking them during applicability. The committed approval can therefore differ from resolved authority, while receipts omit that difference. A regression test should assert that changing either provenance field changes the authority fingerprint and makes an existing prescription stale, and that provenance-free approval is not prescribed.
Confidence: 0.99
Location: boatstack/internal/softwaredelivery/protocol/authority.go:89-91 (RIGHT)
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.94
The new approval preflight diverges from the transition's admitted authority contract and creates a concrete zero-progress path. Model-level verification is recommended before merge to check all new deterministic planning-package refusals remain aligned with resolution and recovery.
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.94
The new approval provenance logic depends on authority-receipt order, causing deterministic refusal or false actor attribution for otherwise valid authority bundles. Model-level verification is recommended before merge, particularly to check whether the new atomic-tree recovery paths can commit any immediately stale package state or enter zero-progress retries.
|
Codex review cycle 1 resolved at ddf1c89. Fixed receipt-order-independent approval provenance and also hardened bounded package reads plus cross-platform path validation. Locus: faithful advisory result res-3e0f7f2e0350196b41b679dd10ab9ff8b6f2cafa9dcd7090efba02797d5a11ed, zero obligations, target met. Verification: SDK and docs checks, 63 Python tests, all 574 isolated Go tests, go test -race ./..., go vet ./..., go build ./..., release-note validation, and git diff --check. |
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.96
The patch introduces actionable authority-freshness, receipt-lineage, update-recovery, verifier-agreement, and partial-effect defects. Model-level verification is recommended before merge, especially to check progress through package-valid/package-approved states across program updates and interrupted atomic-tree recovery.
|
Codex review cycle 2 completed at 82e5923 and this is the requested stopping point; no third review cycle will be processed. Four valid findings were fixed at their owning boundaries: authority provenance freshness/receipts, work-result Flow lineage, portable contract metadata admission, and atomic-tree rollback bookkeeping before parent sync. The schema-2 foreground-record request was declined because the accepted v2 plan explicitly forbids v1 migrations and compatibility readers. Locus result res-2e5cdf7380d4424fc85290dd80bccd6234eff9966f684daaf8464c0b86ddd47b was faithful with zero obligations and advisory target met. Verification passed: SDK/docs, 63 Python tests, all 579 isolated Go tests, go test -race ./..., go vet ./..., go build ./..., release-note validation, and git diff --check. Consumer PR #17 is pinned to this exact candidate; package B remains integrity/contract/approval valid with current_program match. |
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.97
The patch introduces an admission/receipt canonicalization mismatch that can fail after effects, and its new offline verifier accepts approval artifacts outside the production approval contract. Model-level verification is recommended before merge for the new atomic-tree recovery paths: verify that every interruption point converges through resume or rollback without duplicate effects or zero-progress cycles.
| if err := humanidentity.ValidateRole(r.IdentityRole); err != nil || len(r.IdentityProviderFingerprint) != 64 { | ||
| return fmt.Errorf("authority receipt has invalid identity provenance") | ||
| } | ||
| if _, err := hex.DecodeString(r.IdentityProviderFingerprint); err != nil { | ||
| return fmt.Errorf("authority receipt has invalid identity provenance") |
There was a problem hiding this comment.
[P1] Canonicalize identity fingerprints before admitting authority
Invariant: every authority bundle accepted before effects must be representable in the committed receipt. A 64-character uppercase hexadecimal IdentityProviderFingerprint passes AuthorityReceipt.Validate here, so plan.approve can execute and verify its state mutation; NewReceipt then rejects the same fingerprint because receipt validation requires lowercase SHA-256, leaving the transaction recovery-required after effects. Planning-package approval similarly rolls back after its verifier rejects the uppercase value. This mismatch is introduced by the new provenance validation. Require canonical lowercase here (or consistently accept uppercase everywhere). A regression test should apply a local authority transition with an uppercase provider fingerprint and assert either pre-effect refusal or a successful committed receipt, never post-effect failure.
Confidence: 0.98
| matchingActor := false | ||
| for _, source := range approval.AuthoritySources { | ||
| if source.Subject == approval.Actor { | ||
| matchingActor = true | ||
| } | ||
| } | ||
| if !matchingActor { | ||
| return fmt.Errorf("authority does not match actor") |
There was a problem hiding this comment.
[P2] Require the approval actor to come from approval-capable authority
Invariant: approval=valid must establish the structural authority contract produced by planning.package.approve. ValidateApproval currently matches only Subject, so a canonically sealed approval containing one repository-policy source whose subject equals Actor, plus an arbitrary role/provider fingerprint, is accepted; the same is true when the role/provider does not correspond to any human/autonomy source. Such artifacts cannot be produced by the approval effect, which selects an identity-bearing human or autonomy receipt, yet Verify reports approval=valid and --require-approval exits successfully. Require a matching human/autonomy source and bind its identity provenance to the selected actor. A regression test should reseal an otherwise valid package approval with only a repository-policy actor source and assert Approval=Invalid and CLI failure.
Confidence: 0.96
1 participant
Problem
Planning packages were mutable schema-1 projections with an implicit plan artifact and no portable verifier. Artifact-specific generation guidance was not part of the generic foreground-work contract.
Contract changes
This intentionally breaking alpha change adds generic content-bound artifact guidance and replaces the legacy planning field with an explicit
planningPackagedefinition containingworkandplanOutput. It bumps Control Program schema/artifact 6 to 7, compiler identity 8 to 9, foreground-work record 2 to 3, planning manifest 1 to 2, and approval 1 to 2. Contract snapshot and work receipt begin at schema 1.Immutable evidence model
The software-delivery runtime now installs complete fingerprint-addressed package trees atomically under
.boatstack/planning-packages/<delivery>/<fingerprint>/. The exported Go owner validates canonical manifest, contract, receipt, output, approval, inventory, path, mode, content, and fingerprint edges. Approval and promotion revalidate the exact durable snapshot; workspace transfer requires the schema-2 approval projection to be byte-identical to that snapshot.Important non-change
Guidance is descriptive content only. It grants no authority, capability, execution, or verification semantics. Generic work contracts gain no software-delivery field, and no compatibility reader, migration, alias, fallback, or generic bundle framework is introduced.
Consumer proof
The ready boatstack-test PR #17 compiles against exact Boatstack candidate
82e5923387d6706382802c075a0a087e7756a977and contains package B only.planning-snapshot-proof849a4e6239b61fda7597ba0baa5fc32fc2b4ea55b212d0638cd53babdc9ccd47(local proof branch; byte-stable after B)ca5595048d84dd449add17a24a7d4e20b334c00dc2e2e85ac0b303c77b4fbab6implementation-plan--require-current-programfailsVerification
npm run test:flow-sdknpm run docs:checkpython3 .github/scripts/run_go_tests.pygo test -race ./...go vet ./...go build ./...git diff --checkOwnership
controlprogramand base TypeScript SDKflow/softwaredelivery/planningpackageboatstack-helperRelease note:
release-notes/2026-08-18-planning-package-contract-v2.md