Generalize accepted work packages - #232
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.95
The generic work-package split introduces two concrete stale-approval gaps: observation can accept substituted approval bytes, and workspace transfer can copy an approval different from the promotion lineage. Model-level verification is recommended before merge, specifically to check whether approval substitution or recovery around promotion/workspace transfer can create additional blocking cycles.
Findings without inline diff anchors
[P1] Revalidate the approval bytes read after package verification
Invariant: an approved work package may remain known only while the currently observed approval bytes are canonical and exactly match the durable approval identity. State with approval fingerprint A → Verify reads a valid approval A → another process replaces approval.json with noncanonical or malformed bytes that retain "fingerprint": A → the ignored read/decode errors and field-only comparison still return valid → resolution can report the approved-work target terminal despite tampered authority evidence. This patch introduces the failure by replacing the prior full-byte digest comparison with trust in the decoded, self-declared fingerprint. A regression test should swap the approval immediately after Verify returns and assert observeWorkPackage returns false; the post-verification read must check errors, canonical encoding, self-fingerprint, and manifest lineage.
Confidence: 0.97
Location: boatstack/internal/softwaredelivery/plant/observer.go:718-721 (RIGHT)
| var promotion planPromotionReceipt | ||
| if decodeStrictArtifact(approvalRaw, &promotion) == nil && promotion.SchemaVersion == 2 { | ||
| packageMutations, packageErr := prepareWorkspaceWorkPackageTransfer(repositoryRoot, workspacePath, deliveryID, promotion.WorkPackageFingerprint) |
There was a problem hiding this comment.
[P2] Bind workspace package transfer to the promoted approval
Invariant: workspace transfer must copy the exact work-package approval named by the committed plan-promotion receipt. Promotion P binds package F and approval A → P is validated → approval.json under F is concurrently replaced by another structurally valid approval A' → this call passes only F, so the helper verifies and copies A' → the workspace receives P(A) alongside package approval A'. The new transfer path therefore loses the already available approval identity and can commit inconsistent artifacts, after which fresh observation marks the workspace stale and blocks continuation. A regression test should replace the sidecar between promotion validation and package capture and require refusal with no mutations; pass promotion.WorkPackageApprovalFingerprint into the helper and validate the captured sidecar against it.
Confidence: 0.93
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.94
The new work-package state can enter a concrete zero-progress stale state with no normal recovery transition. Model-level verification is recommended before merge to enumerate stale valid/approved package states across objective phases and confirm each has a reachable repair path.
| if !valid { | ||
| workPackageFact.Status = model.FactStale | ||
| artifactTerminal = model.TerminalStale |
There was a problem hiding this comment.
[P2] Provide a recovery path for stale work packages
Invariant: every observed stale package state must retain a transition that can restore durable progress. After work.package.admit or terminal work.package.approve, deleting or corrupting any package member makes observation set the work-package fact and terminal status to stale. All new package transitions require the work-package fact to be known (absent, valid, or approved), and an approved-work objective may remain in TERMINAL, so neither targeted nor untargeted resolution can select re-admission, approval, or promotion. This patch introduced the separate stale facet without the prior planning-package behavior that projected corruption to recoverable plan=stale. Normal operation is therefore wedged until files are manually restored. Add an integration test that admits/approves a generic package, corrupts an output, observes and resolves, and verifies a repair/re-admission transition can restore a valid package.
Confidence: 0.96
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.97
The new durable work-package domain is not reset on the workspace-cut source-parking path, creating a concrete blocking state for subsequent deliveries. Model-level verification is recommended before merge: check every reset, transfer, and objective-rebind path for retained work-package lineage.
Findings without inline diff anchors
[P1] Clear work-package state when parking the source worktree
Invariant: parking a source worktree must clear all delivery-owned state. Starting with an approved work package for delivery A, workspace.cut clears the objective and plan here but retains WorkPackage, WorkPackageFingerprint, and WorkPackageApprovalFingerprint. Binding an approved-plan objective for delivery B then makes the observer verify A's fingerprint under B, project a stale package, and force phase ACTIVE/terminal stale while engagement remains dormant; engagement.begin, work.package.admit, and plan.create all reject that state, leaving no progress path. This is introduced by the new work-package fields not being added to parkedSourceState, making the parked checkout unusable for another delivery without manual state repair. A regression test should park a state with an approved package and assert the package state and both fingerprints are cleared, then verify a new objective can select engagement.begin.
Confidence: 0.98
Location: boatstack/internal/softwaredelivery/effects/driver.go:468-470 (RIGHT)
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.96
The kernel-facing Flow compiler accepts incomplete package lifecycles that normal resolution cannot complete. Model-level verification is recommended after fixing this: verify that priorities cannot shadow the required admit→approve→promote sequence or create a stale-package recovery cycle.
| if promotionPlanOutput != "" { | ||
| if admittedPackageWork == nil { | ||
| return delivery.ProgramRuntimeManifest{}, fmt.Errorf("%s requires %s with foreground work", PlanningPackagePromote, WorkPackageAdmit) | ||
| } | ||
| if err := validatePlanningPackageWorkContract(*admittedPackageWork, promotionPlanOutput); err != nil { | ||
| return delivery.ProgramRuntimeManifest{}, fmt.Errorf("%s: %w", PlanningPackagePromote, err) | ||
| } |
There was a problem hiding this comment.
[P1] Reject incomplete work-package lifecycles
Invariant: every admitted marked objective must have a reachable transition chain from default durable state. A raw Flow containing only work.package.approve can declare the new approved-work-package target and pass compilation, as the added TestApprovedWorkPackageEntryResolvesTrustedObjective demonstrates. After objective.bind and engagement.begin, the work-package facet is absent; approval requires valid, admission is undeclared, and untargeted resolution can never progress. Similarly, planning promotion is accepted with admission but no approval, then blocks after admission. This patch introduces the new objective/transitions but validates only promotion→admission, while the TypeScript helper’s admit+approve requirement is bypassable by other frontends. The observable result is a permanently nonterminal admitted run. Add a runtime-manifest test asserting that approve-only and admit+promote manifests are rejected; require the complete admit→approve chain whenever these package objectives are exposed.
Confidence: 0.98
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.99
The new stale-plan recovery route is blocked by an inherited terminal predicate. Model-level verification is recommended before merge to check all new stale work-package/plan combinations retain a selectable recovery transition.
| promote.SourceConditions = withFacetStatuses( | ||
| withFacet(withoutFacet(promote.SourceConditions, model.FacetPlan), model.FacetWorkPackage, string(model.WorkPackageApproved)), | ||
| model.FacetPlan, | ||
| []model.FactStatus{model.FactKnown}, | ||
| string(model.PlanAbsent), string(model.PlanStale), | ||
| ) |
There was a problem hiding this comment.
[P1] Stale promoted plans cannot reach the promotion recovery path
Invariant: a stale canonical plan backed by a valid approved work package must remain admissible to planning.package.promote. After a successful promotion, deleting or corrupting .boatstack/plans/<delivery>.source (or its promotion receipt) yields PlanStale, TerminalStale, PhaseActive, and WorkPackageApproved. Although these lines add PlanStale as a promotion source, they retain plan.activate's inherited terminal condition allowing only nonterminal, so targeted and untargeted resolution reject promotion; admission also rejects because the work package is not absent. This split introduces a durable zero-progress state where normal operation cannot restore the plan. A regression test should promote a package, corrupt the canonical plan, observe production state, and assert untargeted resolution selects planning.package.promote.
Confidence: 0.99
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.91
The new revoked-delegation preflight exception encloses stateful parameter materialization, violating the intended zero-effect admission boundary. Model-level verification is not required before merge once this ordering defect is covered by a targeted regression test.
| } | ||
| if !reprojected { | ||
| return commandOptions{}, fmt.Errorf("DELEGATION_DRIFT: current Flow context does not match the authorized request (bundle %s, authorized %s)", delegationRequest.ControlBundleFingerprint, bound.ControlBundleFingerprint) | ||
| if !revokedDelegationCanReachProgramPreflight(record, delegationRequest) { |
There was a problem hiding this comment.
[P2] Keep revoked candidates read-only until program preflight
Invariant: an unadmitted program with revoked delegation must produce no durable mutations before program-change preflight. With a revoked exact run, change the Flow fingerprint, then invoke a targeted repository transition requiring host input. This exception permits the candidate through; bindFlowEntry subsequently calls materializeFlowInvocation, whose SaveRequest persists an input request before preflightFlowAuthorizationProgramChange runs. Previously this path stopped at DELEGATION_DRIFT. The observable impact is kernel-owned request state created or superseded by an unadmitted program under revoked authority. A regression test should invoke that sequence, assert the program-change suspension, and assert the input-request store remains byte-for-byte unchanged.
Confidence: 0.96
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The new plan-neutral work-package program surface is accepted by compilation but cannot be started through the production Flow binder without plan-specific input. Model-level verification is recommended before merge to check that stale work-package and plan states retain reachable admission and promotion paths.
Findings without inline diff anchors
[P1] Plan-only entry binding makes generic work-package flows unreachable
Invariant: every valid work-package Flow accepted by the compiler must be invocable through the production Flow path. A generic workPackage with an approved-work-package target, no entry inputs, and no planning promotion compiles, but bindFlowEntry unconditionally calls resolveBoundPlan; PlanInboxForEntry then rejects the entry because it does not contain exactly one plan input, so execution stops with FLOW_INPUT_INVALID before objective.bind or work.package.admit. The patch introduces plan-neutral work packages and permits such entries while retaining the old plan-only binder, making valid generic flows require an artificial plan. The smallest regression test should run a compiled no-input generic work-package Flow through bindFlowEntry/continuation and assert it reaches work.package.admit without a plan inbox.
Confidence: 0.99
Location: boatstack/cmd/boatstack-helper/flow_runtime.go:152-152 (RIGHT)
1 participant
Summary
Breaking alpha change
This removes planning.package.admit, planning.package.approve, the planning-package verifier and storage layout, and the old planning package fingerprints. Consumers must update directly; there are no aliases, compatibility readers, migrations, or dual paths.
Boundaries
The kernel, base SDK, production compiler schema, and base Control Program IR gain no package or planning vocabulary. Core objective and engagement transitions remove prior software-delivery target IDs. Generic package approval binds the whole package; only planning.package.promote binds an output to plan semantics.
Verification
Consumer proof: bigboateng/boatstack-test#18.