Require complete Control Program invocations - #222
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 declarative runtime bypasses authority and transition-closure checks, while invocation evidence and producer completeness are not fully related to the parameters and facts used at commit. Model-level verification is recommended before merge. Questions: after these defects are corrected, can any declarative nonterminal state still block or cycle under priority ordering, and does every producer kind retain a recovery path after its source becomes stale?
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch introduces authority/verification bypasses, false invocation receipts, invalid declarative state writes, and an unreachable publication recovery path. Model-level verification is recommended, particularly for declarative priority shadowing and whether every new invocation refusal retains a recovery path.
Findings without inline diff anchors
[P1] Persist publication identity before post-create failures
Invariant: output needed to reconcile an external effect must be durable before any later failure can require recovery. If gh pr create succeeds but its output is unparsable—or state/journal commit subsequently fails—the PR exists while no committed receipt or durable publication_id exists. Recovery selects publication.reconcile, whose new producer requires durable publication_id, so materialization blocks and normal operation cannot recover or safely retry. Add an integration test with successful PR creation followed by unparsable output or injected post-effect failure; the next continuation must reconcile the same PR without recreating it.
Confidence: 0.97
Location: boatstack/internal/softwaredelivery/effects/command_boundary.go:333-337 (RIGHT)
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch breaks durable compatibility, idempotent replay, and resolver/apply agreement for the new declarative runtime. Model-level verification is recommended before merge. Question for model-level verification: can any other declarative predicate/status or producer combination admitted by the generic compiler enter a permanent nonterminal blocker?
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch introduces concrete authority-loss, cross-worktree isolation, transition-selection, and durable-fact defects. Model-level verification is recommended before merge for the new declarative runtime, particularly whether any newly reachable nonterminal state is blocking and whether priority composition can create zero-progress cycles. Tests could not be executed because the review environment is read-only and Go could not create its module/cache directories.
| if (operation == surfaces.OperationApply || operation == surfaces.OperationRecover) && request.ProgramID != "" { | ||
| request, options, err = refreshFlowInvocation(context.Background(), operation, request, options) |
There was a problem hiding this comment.
[P1] Rematerialization drops admitted delegation authority
Invariant: rematerializing invocation evidence must preserve the externally admitted authority for the same apply. Minimal sequence: authorize an autonomy-only Flow → invoke the direct CLI apply path → prepareDelegation appends its authority receipt → refreshFlowInvocation rebuilds the request from commandOptions → the receipt is lost → resolution refuses/frontiers the otherwise authorized transition. This is introduced by rebuilding the request after delegation admission. The observable impact is that direct CLI apply/recover cannot use valid delegation, despite continuation and RPC paths working. A regression test should authorize a delegated run, execute direct CLI apply for an autonomy-required transition, and assert it commits rather than reporting missing authority.
Confidence: 0.98
| if state.SchemaRevision != declarativeRunSchemaRevision || state.RunID != runID || state.ProgramFingerprint != compiled.Fingerprint || state.EntryID != entry.ID || state.TargetID != entry.Target || state.StateRevision == 0 || state.EntryInputs == nil || state.Facts == nil { | ||
| return declarativeRuntimeContext{}, fmt.Errorf("FLOW_CONTEXT_MISMATCH: declarative run identity or schema changed") |
There was a problem hiding this comment.
[P1] Declarative runs are not bound to their originating worktree
Invariant: durable run state must remain bound to its exact repository execution scope. Minimal sequence: worktree A starts a declarative run and receives its run ID → worktree B from the same Git common directory supplies that ID → the shared FlowRoot record passes validation because no execution-scope identity is stored or compared → B can advance A's effect-free state transitions. Although generated run IDs include the scope, the explicit resume path bypasses that derivation. This patch introduces the declarative shared-state runtime, making cross-worktree state mutation reachable. Persist and validate the execution-scope fingerprint; a regression test should create a nonterminal run in one worktree and prove that resuming its ID from another worktree is refused with no revision change.
Confidence: 0.99
| return transitions[i].Priority < transitions[j].Priority | ||
| } | ||
| return transitions[i].ID < transitions[j].ID | ||
| }) | ||
| for _, transition := range transitions { | ||
| if predicateSatisfied(transition.Guard, facts) && !predicateSatisfied(transition.Target, facts) { | ||
| return transition, operators[transition.Operator], true |
There was a problem hiding this comment.
[P2] Equal-priority declarative transitions bypass the canonical frontier
Invariant: multiple equally preferred admissible transitions must produce a selection frontier, not an arbitrary commit. Minimal sequence: empty facts → two transitions with true guards, unsatisfied targets, and the same priority → this selector sorts by ID and applies the first transition → state is mutated without explicit selection. The new declarative runtime bypasses kernel.Relate, whose contract returns a frontier for this case; it also lacks the kernel's positive-priority validation. The impact is repository-ID-dependent control behavior and potentially the wrong durable product state. A regression test should compile two simultaneous equal-priority transitions and assert an ambiguity result and unchanged revision; also reject zero/negative priorities.
Confidence: 0.99
| receipt.ID = "receipt-" + digestDeclarative(receipt)[:24] | ||
| receipt.Fingerprint = digestDeclarative(receipt) | ||
| candidate.LastReceipt = &receipt | ||
| if err := saveDeclarativeRun(runtimeContext.statePath, candidate); err != nil { |
There was a problem hiding this comment.
[P2] Each declarative commit erases the preceding receipt fact
Invariant: a committed transition receipt is an immutable durable fact, not replaceable bookkeeping. Minimal sequence: declarative transition A commits receipt RA → transition B commits receipt RB → LastReceipt is overwritten with RB → no durable record of RA remains. This storage model is introduced with the new declarative runtime. Multi-step runs therefore lose their audit history and cannot prove the prior revision chain or authority used for earlier transitions. A regression test should execute a two-transition run, restart the runtime, and verify that both immutable receipts remain discoverable and validate as a contiguous revision chain.
Confidence: 0.96
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The new invocation boundary can discard declared parameter authority, and the declarative verifier is unsound for parameter-derived assignments, creating a reachable deterministic zero-progress loop. Model-level verification is recommended before merge, particularly to ask whether every dynamically assigned facet invalidates pre-state target proofs and whether every producer kind preserves full any_of/all_of authority requirements.
| for entryID, inputs := range entries { | ||
| input, ok := inputs[producer.Input] | ||
| if !ok || !compatibleEntryInputType(input.Type, contract.Type.Kind) { | ||
| return nil, invocationIncomplete(transition.ID, contract.ID, fmt.Sprintf("entry-input %q is unavailable or incompatible for reachable entry %q", producer.Input, entryID)) | ||
| } |
There was a problem hiding this comment.
[P1] Reject authority-weakening parameter producers
Invariant: every operator-parameter authority requirement must be backed by external authority evidence; a producer declaration cannot grant that authority. Here entry-input producers are accepted based only on type compatibility, while state, receipt, and work-output producers likewise receive no authority check. A declarative operator can therefore require human for parameter p, bind p to an entry input, require no operator/transition authority, and commit an assignment without --human; the resulting invocation even records no authority receipt. This is introduced by the new producer compiler/materializer path and makes parameter-level authority ineffective. Add a regression test where an entry-input producer feeds a human-required parameter and assert compilation rejects it (or materialization blocks without a human authority receipt); cover all_of and the other non-trusted producer kinds as adjacent cases.
Confidence: 0.98
| if value, ok := assigned[fact.Facet]; ok { | ||
| if len(fact.Statuses) != 0 && !containsString(fact.Statuses, "known") { | ||
| return false | ||
| } | ||
| return len(fact.Values) == 0 || containsString(fact.Values, value) | ||
| } | ||
| return predicateRequiresFacetValue(guard, fact.Facet, fact.Values) |
There was a problem hiding this comment.
[P1] Account for parameter assignments when proving postconditions
Invariant: a declarative transition must not be admitted when its assignment can falsify a target fact that the verifier claims is preserved from the guard. assigned contains only literal assignments, so for a facet assigned through value_from.parameter, this branch incorrectly falls back to the pre-state guard. Concrete sequence: an initialization transition establishes phase=old; the next transition guards on phase=old, dynamically assigns phase from entry input new, literally assigns done=yes, and targets phase=old AND done=yes. Validation accepts it because the guard allegedly establishes phase=old; production selects it, applies phase=new, fails target verification, discards the candidate, and repeats the same transition forever. The new declarative verifier/runtime introduces this zero-progress path. Add a regression test compiling that two-step flow and require rejection (or prove the parameter's value); running twice must never reproduce the same deterministic verification failure from unchanged durable state.
Confidence: 0.99
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch introduces one wrong-transition commit path and one concrete zero-progress input-authority path. Model-level verification is recommended before merge. Question for model-level verification: can any other newly admitted producer/priority combination leave a nonterminal invocation permanently blocked?
| "receipt": lastDeclarativeReceipt(runtimeContext.state), "receipts": runtimeContext.state.Receipts, | ||
| }, options.format) | ||
| } | ||
| frontier := selectDeclarativeTransitions(compiled.Document, runtimeContext.state.Facts) |
There was a problem hiding this comment.
[P1] Declarative runs ignore targeted transition requests
Invariant: an explicit transition request must execute that transition or be refused. flow run --transition B reaches this code, but selection always considers the untargeted frontier. With two simultaneously enabled transitions A(priority 10) and B(priority 20), requesting B commits A and records an A receipt. This is introduced by the new declarative driver and observably mutates the wrong state. Add a test invoking a two-candidate declarative flow with --transition B and assert B alone commits (or the request is refused).
Confidence: 0.99
| ProducerFingerprint: invocation.ProducerFingerprint(producer), RequestFingerprint: request.Fingerprint, | ||
| StateRevision: request.StateRevision, ContextFingerprint: request.ContextFingerprint, ControlBundleFingerprint: request.ControlBundleFingerprint, | ||
| ExecutionScopeFingerprint: runtimeContext.executionScopeFingerprint, | ||
| Actor: actor, Host: host, AuthorityReceipts: []string{"human:" + actor}, Scope: "transition", |
There was a problem hiding this comment.
[P1] Non-human host-input authority can never be satisfied
Invariant: every authority configuration accepted by invocation compilation must have a production path that supplies matching externally granted evidence. A valid flow can request an autonomy-only host parameter, but flow input answer always records only human:<actor>. On resume, ValidateCurrent rejects that receipt for missing autonomy, and repeated runs remain blocked; no command path records the delegated autonomy evidence. This behavior is introduced by the new input command. Add an end-to-end test with an autonomy-only host request plus a valid autonomy delegation and verify it can resume, or reject that unsupported authority during compilation.
Confidence: 0.97
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.99
The new durable receipt/admission ABI breaks existing journals and strands an active legacy publication boundary. Model-level verification is recommended before merge: verify that every newly admissible receipt/work/state producer is available in all production-selected states and that no new priority or recovery path can enter a zero-progress cycle.
| ) | ||
|
|
||
| const ReceiptSchemaVersion = 12 | ||
| const ReceiptSchemaVersion = 13 |
There was a problem hiding this comment.
[P1] Preserve and migrate pre-upgrade committed histories
Invariant: a runtime upgrade must keep durable journals readable and every previously reachable nonterminal state resumable. A repository with any base-version committed journal contains admission schema 8 and receipt schema 12; after upgrading, readJournal invokes the new exact validators and rejects that history because this patch requires admission 9/receipt 13 while leaving the enclosing journal schema unchanged. Receipt scans used for active-run lookup, recovery, and sequence allocation then fail; sequence allocation can even fail after a new effect has executed. Additionally, a base run paused after publication.execute has no effect_outputs, while the new publication.observe producer requires publication_id from that receipt, leaving published-not-landed unable to progress even if legacy schemas are accepted. The patch introduces both incompatibilities without migration or an old-version fixture. Add a regression that creates a real base-version committed history—including a run paused after publication.execute—opens it with head, and proves it can resume without effects occurring before migration failure.
Confidence: 0.99
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The new declarative runtime can commit a superseded input and cannot initialize valid zero-input entries. Model-level verification is recommended before merge, particularly whether accepted declarative guards and priority rules can create additional blocking nonterminal states.
| materializationContext := invocation.Context{ | ||
| RunID: runtimeContext.state.RunID, ProgramFingerprint: runtimeContext.compiled.Fingerprint, | ||
| ExecutionProgramFingerprint: runtimeContext.compiled.Fingerprint, | ||
| EntryID: runtimeContext.entry.ID, TargetID: runtimeContext.entry.Target, TransitionID: transition.ID, | ||
| StateRevision: runtimeContext.state.StateRevision, ContextFingerprint: contextFingerprint, | ||
| ExecutionScopeFingerprint: runtimeContext.executionScopeFingerprint, EntryInputs: entryInputs, | ||
| State: stateValues, Receipts: map[string]invocation.Value{}, WorkOutputs: map[string]invocation.Value{}, InputReceipts: receipts, | ||
| } | ||
| result, err := invocation.Materialize(operator.Parameters, transition.Parameters, materializationContext, nil) |
There was a problem hiding this comment.
[P1] Honor superseded input generations in declarative runs
Invariant: a superseded host-input receipt must never authorize a later transition. Sequence: suspend generation 1 → answer A → supersede and answer generation 2 with B → resume; this path constructs a generation-1 materialization context and consumes A, committing the explicitly rejected value. The software-delivery path loads LatestRequest, but this new declarative path does not. Add a regression that supersedes an answered declarative request and verifies only generation 2's value reaches state and the receipt.
Confidence: 0.99
| if len(provided) == 0 { | ||
| return declarativeRuntimeContext{}, fmt.Errorf("FLOW_INPUT_REQUIRED: a new declarative run requires its entry inputs") |
There was a problem hiding this comment.
[P2] Permit declarative runs without entry inputs
Invariant: a valid entry with no required inputs must be startable. Sequence: compile an assignment-only declarative Flow whose entry has no inputs → run it without --input → the runtime returns FLOW_INPUT_REQUIRED before creating state, despite validation having established that no input is missing. This unconditional new check also blocks entries containing only omitted optional inputs. Add a regression that starts and completes a zero-input declarative entry.
Confidence: 0.99
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.99
The patch introduces a concrete recovery dead end and two declarative-runtime correctness failures. Model-level verification is recommended before merge, particularly to verify that every new explicit producer remains available in all interrupted states and that priority rules do not shadow recovery prerequisites.
| case "workspace.reconcile": | ||
| return { transaction_id: durableValue("transaction_id") }; | ||
| case "publication.reconcile": | ||
| return { | ||
| publication_id: durableValue("publication_id"), | ||
| transaction_id: durableValue("transaction_id"), |
There was a problem hiding this comment.
[P1] Source recovery parameters from committed recovery evidence
Invariant: every prescribed recovery transition must be materializable from facts that survive the interrupted effect. If a workspace command returns an unknown outcome, or gh pr create succeeds but returns unparsable output, preparedEffect.Execute exits before installing the candidate state; durable transaction_id and usually publication_id remain absent. Resolution then selects workspace.reconcile or publication.reconcile, but these new bindings read those required values only from durable state, so invocation materialization returns TRANSITION_INPUT_UNAVAILABLE and retries cannot progress. The patch introduces this by replacing the prior parameter surface with state-only producers. This can strand a run in recovery after an external effect may already have occurred. A regression test should force an unknown workspace/publication boundary result, then assert the production continuation path can materialize and apply the prescribed reconcile transition from journal/provider evidence without repeating the original effect.
Confidence: 0.99
| if len(provided) == 0 { | ||
| return declarativeRuntimeContext{}, fmt.Errorf("FLOW_INPUT_REQUIRED: a new declarative run requires its entry inputs") |
There was a problem hiding this comment.
[P2] Allow declarative entries that require no inputs
Invariant: a valid entry with no required inputs must be able to create its initial run. For an entry with zero inputs, or only omitted optional inputs, validation succeeds and derives a run ID from the empty input map, but the new-state branch unconditionally rejects len(provided) == 0 with FLOW_INPUT_REQUIRED. No alternative input can be supplied because undeclared inputs are rejected, so the entry is permanently unstartable. A regression test should run a declarative flow whose entry has no inputs and verify its initial transition executes.
Confidence: 0.99
| for _, assignment := range operator.StateEffect.Assignments { | ||
| if assignment.ValueFrom == nil { | ||
| continue | ||
| } | ||
| if assignment.ValueFrom.Parameter == "" || assignment.ValueFrom.Admission != "" || assignment.ValueFrom.Invocation != "" { | ||
| return fmt.Errorf("FLOW_RUNTIME_INVALID: declarative assignment %q has an unsupported value source", assignment.Facet) | ||
| } | ||
| contract, declared := contracts[assignment.ValueFrom.Parameter] | ||
| if !declared || !bindings[assignment.ValueFrom.Parameter] { | ||
| return fmt.Errorf("FLOW_RUNTIME_INVALID: declarative assignment %q requires bound operator parameter %q", assignment.Facet, assignment.ValueFrom.Parameter) | ||
| } | ||
| if facets[assignment.Facet].Kind == "enum" || contract.Type.Kind != "string" { | ||
| return fmt.Errorf("FLOW_RUNTIME_INVALID: declarative assignment %q cannot prove parameter %q belongs to the facet", assignment.Facet, assignment.ValueFrom.Parameter) |
There was a problem hiding this comment.
[P2] Reject ill-typed declarative state assignments
Invariant: committed declarative facts must conform to their declared facet type. The validator skips all literal assignments and treats every non-enum parameter assignment as requiring a string, so a boolean facet can be assigned literal "yes" or an arbitrary string parameter. The runtime writes that value, its predicate evaluator accepts the same string as the boolean target, and it emits a successful terminal receipt for invalid typed state. This becomes reachable through the newly added declarative executor. A regression test should declare a boolean facet and verify both a non-boolean literal and a string-sourced assignment are rejected before run creation.
Confidence: 0.98
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.96
The new invocation-completeness checks admit producer relationships that the production materializer deterministically refuses, including a reachable publication-recovery loop. Model-level verification is recommended before merge: check whether every required producer is available for every newly selectable transition and whether producer priority can shadow its prerequisite.
|
Review cycle 1 is settled at bf3b209.
Verification: all 482 isolated Go tests, 54 Python tests (one expected failure), Flow SDK TypeScript build, Go build, Go vet, and the two-pass local boundary review passed. |
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.97
The new invocation-completeness rule permits a concrete zero-progress publication recovery path. Model-level verification is recommended for whether every new receipt/work-output producer remains available after recovery transitions.
|
Cycle 2 settled at
Verification: 485 Go tests passed; 54 repository-contract tests passed with 1 expected failure; Flow SDK tests, documentation checks, Go build/vet, shell syntax, and the tree-bound local review all passed. |
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.97
The patch introduces concrete recovery and invocation-completeness dead ends. Model-level verification is recommended before merge. Questions: does every pending-journal recovery input come from the observed recovery context, and can any newly accepted producer graph remain indefinitely in candidate/suspended state without durable progress? Tests could not be executed because the read-only environment prevented Go from creating its temporary build directory.
| for _, parameter := range operator.Parameters { | ||
| if !declarativeAuthoritySupported(parameter.Authority.AnyOf) || !declarativeAuthoritySupported(parameter.Authority.AllOf) { | ||
| return fmt.Errorf("FLOW_RUNTIME_INVALID: declarative operator %q parameter %q uses unsupported authority", operator.ID, parameter.ID) | ||
| } | ||
| } |
There was a problem hiding this comment.
[P2] Reject secret parameters until the declarative runtime can answer them
Invariant: a Flow accepted by runtime validation must have a resumable path for every required input. The declarative validator accepts secret parameters and their host-input bindings, but flow input answer always supplies plaintext and deterministically rejects every secret request with FLOW_SECRET_STORE_UNAVAILABLE; entry/state sources likewise cannot supply an opaque secret reference. Thus an accepted Flow suspends forever on its first required secret. Reject such contracts during flow check until a secret-store adapter exists. A minimal regression test should check a declarative Flow with one required secret host parameter and assert validation fails rather than producing an unanswerable request.
Confidence: 0.96
| if len(producerTransitions) != 1 { | ||
| return nil, invocationIncomplete(transition.ID, contract.ID, "work output does not have exactly one producer transition") | ||
| } | ||
| prior := producerTransitions[0] | ||
| if prior.ID != transition.ID && (prior.Priority >= transition.Priority || !predicateImplies(transition.Guard, prior.Target)) { | ||
| return nil, invocationIncomplete(transition.ID, contract.ID, "work output is not guaranteed before the consuming transition") |
There was a problem hiding this comment.
[P2] Close untargeted selection for same-transition work outputs
Invariant: if apply can legally consume a producer, normal untargeted resolution must be able to reach the same invocation. The compiler now accepts a required parameter sourced from foreground work owned by that same transition. Initially resolution returns a candidate and starts the work; after completion it still resolves with no parameters and returns the same missing-parameter candidate, while prescription stabilization only handles responses that already contain a prescription. Targeting the transition explicitly materializes the completed output and succeeds, so targeted and untargeted resolution diverge. Either reject this producer graph or rebind completed-work candidates before resolving again. Add a regression test that completes same-transition work and expects an untargeted RPC resolve to return an invocation-bound prescription.
Confidence: 0.91
|
P1-only follow-up at 36cec03: reconciliation now binds transaction_id to the canonical pending-journal recovery observation, including the case where durable transaction state was never written. Added a real-journal regression for untargeted publication.reconcile. Local evidence: 487/487 Go tests, 54 repository contracts, SDK/docs checks, build, vet, and the exact-tree Boatstack review all pass. The explicit P1 thread has an individual reply and is resolved; lower-priority threads remain open for later work. |
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch introduces invalid typed-state commits and several concrete zero-progress or broken-replay paths in the new Flow runtime. Model-level verification is recommended before merge, particularly to ask whether global declarative priority selection can let transitions serving another entry shadow all progress toward the selected entry’s target.
| if os.IsNotExist(err) { | ||
| if len(provided) == 0 { | ||
| return declarativeRuntimeContext{}, fmt.Errorf("FLOW_INPUT_REQUIRED: a new declarative run requires its entry inputs") | ||
| } |
There was a problem hiding this comment.
[P2] Allow declarative entries with no initial inputs to start
Invariant: a valid entry with no required inputs must be able to create its initial run. For an entry whose inputs is empty, provided is necessarily empty; the missing-state branch therefore always returns FLOW_INPUT_REQUIRED. Supplying any value is also rejected as undeclared, so the entry can never start. This affects the new declarative runtime, including the zero-input form documented by the TypeScript API. Add a regression test that runs a valid zero-input declarative entry and observes its first transition or terminal result.
Confidence: 0.99
| if options.transitionID != "" { | ||
| _, repositoryTransition := findCompiledTransition(compiled.Document.Transitions, options.transitionID) | ||
| if repositoryTransition { | ||
| options, err = materializeFlowInvocation(ctx, compiled, entry, options, options.controlBundle) | ||
| if err != nil || options.inputRequest != nil { | ||
| return options, err | ||
| } |
There was a problem hiding this comment.
[P2] Check committed CLI replays before rematerializing producers
Invariant: an exact idempotent retry must be resolved from its committed receipt before volatile producer inputs are read again. The CLI path calls materializeFlowInvocation before the engine can look up the idempotency receipt, while only the RPC binder uses committedFlowReplay. After gate.review.record commits, deleting its consumed review.input.json and repeating the exact CLI apply therefore fails producer resolution instead of returning the prior receipt; workspace.cut similarly fails once its branch or destination exists. This behavior is introduced by the new producer materialization path. Add the existing RPC consumed-input replay scenario through the CLI and assert the second apply returns the identical receipt with replayed=true.
Confidence: 0.98
| if requestedParameter.Secret { | ||
| return nil, fmt.Errorf("FLOW_SECRET_STORE_UNAVAILABLE: parameter %s requires a trusted secret store", parameterID) |
There was a problem hiding this comment.
[P2] Reject secret host-input contracts without a secret-store adapter
Invariant: every invocation contract accepted as executable must have a reachable materialization path. Declarative validation accepts a required secret host-input parameter, flow run emits an input request, but every answer deterministically returns FLOW_SECRET_STORE_UNAVAILABLE; no command can create the required secret reference, so the same transition remains suspended forever. The new validator/runtime composition introduces this zero-progress path. Until a trusted secret-store producer exists, add a regression test requiring flow check to reject such a declarative contract rather than emitting an unanswerable request.
Confidence: 0.97
|
P1-only follow-up at 643ac06: declarative assignment validation now rejects parameter/facet type mismatches and noncanonical boolean literals before run state is loaded or created. Added an execution-path regression proving the invalid Flow creates no controller state or receipt. Local evidence: 488/488 Go tests, 54 repository contracts, build, vet, and the exact-tree Boatstack review pass. The explicit P1 thread has an individual reply and is resolved; P2 threads remain open and unchanged. |
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.97
The patch introduces two concrete resolver/apply and targeted-selection failures. Model-level verification is recommended for the new declarative runtime, especially priority shadowing and nonterminal progress.
| state.Verification, state.Phase = model.VerificationStale, model.PhaseActive | ||
| if state.RequiredGateEvidenceCurrent() { | ||
| state.Verification = model.VerificationCurrent | ||
| } |
There was a problem hiding this comment.
[P2] Supplemental gates can be prescribed but cannot commit
Invariant: every prescribed transition must satisfy its own deterministic apply postcondition. With a locked active delivery lacking one of build/test/review, explicitly request gate.change.record or gate.journey.record; targeted resolution admits it, but this handler leaves verification stale, while both transitions require current, so verification fails, rollback occurs, state remains unchanged, and retry repeats. This patch introduced conditional verification without widening these two targets. Impact: valid targeted gate recording makes no progress. Add a targeted engine test proving each supplemental gate is either refused before effects until prerequisites exist or commits successfully.
Confidence: 0.98
| if options.transitionID != "" { | ||
| var requested *controlprogram.Transition | ||
| for index := range frontier { | ||
| if frontier[index].ID == options.transitionID { | ||
| requested = &frontier[index] | ||
| break | ||
| } | ||
| } | ||
| if requested == nil { |
There was a problem hiding this comment.
[P2] Targeted declarative resolution incorrectly applies untargeted priority
Invariant: an explicit request filters admissible transitions before priority ordering. With two guard-satisfied transitions at priorities 10 and 20, explicitly requesting the priority-20 transition is rejected because the code searches only the already-prioritized frontier. The new declarative runtime therefore disagrees with Boatstack's canonical targeted relation and makes a legal alternative unreachable until the higher-priority transition changes state. Add a declarative runtime test where the lower-priority requested transition has a satisfied guard and assert it executes despite another untargeted preference.
Confidence: 0.97
|
P1 saturation reached at 643ac06. The exact-head Codex review completed with zero P1 findings; its two P2 findings remain open, unreplied, and unresolved under the agreed post-cycle-2 policy. All exact-head CI checks pass, and no further code change is being made in this pass. |
1 participant
Summary
Control boundaries
Verification
python3 .github/scripts/run_go_tests.py— 465 tests passed across 10 isolated shardspython3 -m unittest discover -s .github/tests -p test_*.py -v— 54 tests passed with one expected failurenpm run test:flow-sdknpm run docs:checkcd boatstack && go build ./... && go vet ./...published-prpython3 .github/scripts/release_notes.py preflight --repo . --base-branch mainThe release note is
release-notes/2026-08-15-invocation-completeness.md.