fix: enforce immutable session configuration laws

[loganj]

🤖

Summary

A chat could previously drift between the provider and model shown in Berd and the configuration actually used by its session. This change makes each session keep one compatible, effective configuration and keeps the interface aligned with it.

Berd now preserves a user's explicit provider and model choice while authoritative model information is unavailable, then repairs or clears that choice only when a successful provider response proves it unsupported. The same rule applies when loading saved personas, choosing defaults, creating sessions, preparing a session, changing its model, and sending a prompt.

Details

• Adds durable product laws requiring one effective session configuration, a compatible harness/provider/model combination, and an interface that shows the configuration in use.
• Treats cached and runtime model candidates as suggestions; only a successful provider inventory response proves which models may be newly selected or persisted.
• Serializes configuration changes with session loading so stale or superseded work cannot overwrite newer user intent.
• Keeps external agent harnesses distinct from Goose model providers.
• Enforces provider policy synchronously during prompt dispatch without adding model-inventory network work to the send path.

Compatibility

The implementation follows the current session lifecycle and composer-queue dispatch laws on main. Session preparation retains the active dispatch lease and mutation intent while model changes are deferred or superseded.

Related issue

Supersedes squareup/berd#1114 after the repository migration. Builds on the provider-selection behavior introduced by squareup/berd#1085.

[morgmart]

🤖 Automated code review

REQUEST_CHANGES: five blocking findings remain in the exact three-dot comparison. The supplied GitHub evidence was inspected: all listed check runs for the exact head SHA completed successfully, while the combined commit status is pending with no status contexts; required checks still independently govern merge readiness. Final self-check covered every changed configuration flow (startup repair, persona loading/invocation, new and existing session preparation, picker/composer display, prompt dispatch, ACP connection invalidation), accessibility, localization, navigation and consent guards, async/error/never-completes/lifecycle/race behavior, test honesty, project laws/design-system rules, duplicate overlap, and concrete user effect. No additional supported accessibility, i18n, navigation, consent, or design-system findings survived. The five findings are distinct: invalid persona invocation, two different session/UI divergence scenarios, stale-client delivery, and failed-initialization transport leakage.

Deterministic publication result: 5 blocking and 0 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.

[morgmart]

🤖 Automated code review

REQUEST_CHANGES: four blocking findings remain after correcting the completed review against the exact three-dot comparison and current allowed anchors. The provider-only persona finding was removed because its cited behavior is not anchored to a current changed RIGHT-side line. The five prior automated threads were read in full and their underlying issues are fixed at this SHA, so they are neither findings nor duplicate suppressions. Supplied GitHub evidence was inspected: macOS, Windows, and clean-room checks succeeded; frontend/unit and Linux checks were still in progress; combined status was pending with no status contexts. Required checks independently govern merge readiness.

Deterministic publication result: 4 blocking and 0 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.

[morgmart]

🤖 Automated code review

REQUEST_CHANGES: three new blocking findings remain after a fresh full review of the exact three-dot comparison. Three additional still-valid concerns are suppressed because authoritative GitHub threads for the same underlying issues remain unresolved. The supplied GitHub evidence was inspected: all listed check runs for the exact head SHA completed successfully; combined status was pending with no status contexts. Required checks independently govern merge readiness. Final self-check covered provider setup and startup repair, persona navigation/invocation, picker and composer state, new/existing session preparation, target transitions, queue and berdctl dispatch, inventory refresh/invalidation, ACP connection lifecycle, accessibility, localization, navigation/consent guards, async/error/never-completes/races, test honesty, design-system/project laws, duplicate overlap, and concrete user effects. No additional supported accessibility, i18n, navigation, consent, or design-system findings survived.

Deterministic publication result: 3 blocking and 0 non-blocking finding(s) publishable; 3 duplicate(s) suppressed.

[loganj]

🤖 Carl — BLOCK (P1), 8/10 at exact head 197a9d5db43dd6add542fd2c9606376979b7c6f7.

The authoritative-inventory invalidation gate can reopen after any later failed refresh.

refreshProviderModels correctly records successful proof in provenModelIds (src/features/providers/stores/providerModelCacheStore.ts:315-337), but its error branch replaces the cache entry without carrying forward existing.provenModelIds (:338-355). That changes isModelInventoryAuthoritative from true to false after a transient refresh error. The consequences line up across all three gates:

• the active picker stops treating the missing model as disproved and reconstructs it from the session target (src/features/chat/hooks/useResolvedAgentModelPicker.ts:778-791);
• the composer stops invalidating the controlled target and enables Send again (src/shared/ui/GlobalComposerPill.tsx:667-681, :738-750);
• the synchronous transport gate treats missing/provisional proof as allowed (src/features/providers/stores/providerModelCacheStore.ts:164-177; src/shared/api/acpSessionRegistry.ts:470-490).

So this sequence regresses the new invariant: prepare model A → publish successful authoritative inventory excluding A → attempt a later refresh that fails (offline/expired token/transient ACP error) → stale model A is displayed and dispatchable again. A failed read is not evidence that the prior successful exclusion became false. Preserve the last successful proof across refresh failure (while recording the error/staleness separately), or otherwise keep the disproved pair synchronously blocked. Add the end-to-end discriminator covering picker display, composer admission, and zero prompt dispatch after the failed refresh.

I independently inspected the complete 65-file PR range and traced session preparation/registry serialization, picker/composer target derivation, provider-policy proof, cache publication/invalidation, preflight/load ordering, timeout isolation, persona targets, and synchronous Send admission. Review workstation: fresh Blox carl-berd-pr19-full-review (ID 1882713), clean exact-head checkout; no Blox-side agent. Existing CI and Larry's full-suite evidence were considered context, not proof. No branch movement or code changes by me.

[loganj]

🤖 Princess Donut — PASS (9.4/10) at exact head 197a9d5db43dd6add542fd2c9606376979b7c6f7 (tree 504d2310ac8922189792c542f823d43d1cc08a47).

I independently reviewed the complete 65-file PR range against LAWS/CHAT.md, not merely the latest repair. I traced the authoritative/provisional inventory split, managed-provider preflight and generation invalidation, serialized prepare/load ordering, registry acknowledgement materialization, active picker/composer reconciliation, and the synchronous prompt admission gate.

The design now closes the material invariant gaps:

• Prepared provider/model identity remains the transport authority; UI state reconciles from ACP acknowledgement rather than intent.
• A successful authoritative inventory can disprove a stale active model across picker, composer, and transport, while unavailable/provisional proof does not rewrite explicit intent.
• Send performs no inventory network work and synchronously rejects a cached-disproved pair before prompt setup or dispatch.
• Failed preflight restores a deferred authoritative load; a mutation attempt keeps that older load suppressed.
• Provider-only preparation materializes the already-acknowledged registry model, keeping coordinator, store, picker, and invocation aligned.
• Inventory-proof timeout does not invalidate the shared ACP client or abort an unrelated prompt.

Independent Blox verification on fresh read-only workstation donut-berd19-final-review (ID 1882721) passed 7 focused files / 246 tests at the exact head. I also confirmed all seven GitHub checks are terminal-green and the PR is mergeable. No blocking or non-blocking findings remain from my review.

Remaining risk: the change is broad and concurrency-heavy, and UI validation remains test-level rather than a live GUI workflow; however, the discriminating race/failure tests exercise the relevant state transitions directly, and the full author validation plus cross-platform CI provide adequate coverage for merge.

GitHub does not permit APPROVE on a PR authored by the same account, so I am recording this passing verdict as a COMMENT review.

[morgmart]

🤖 Automated code review

REQUEST_CHANGES: six blocking findings remain after a fresh full review of the exact three-dot comparison. Three resolved automation threads with no substantive human reply describe issues that are still present at this SHA, so those issues are publishable again under the duplicate policy; all other prior issues are fixed or unsupported. The supplied GitHub evidence was inspected: all listed check runs for the exact head SHA completed successfully; combined status was pending with no status contexts. Required checks independently govern merge readiness. Final self-check covered startup/default reconciliation, provider setup and inventory refresh, persona navigation/invocation, picker/composer state, new and existing session preparation, target transitions, queue and berdctl dispatch, ACP connection lifecycle, accessibility, localization, navigation/consent guards, async/error/never-completes/races, test honesty, design-system/project laws, duplicate overlap, and concrete user effects. No additional supported accessibility, i18n, navigation, consent, or design-system findings survived.

Deterministic publication result: 6 blocking and 0 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.

[loganj]

🤖 Review by Mongo (independent full-PR gate)

Verdict: APPROVE — 9/10. Reviewed the complete PR against LAWS/CHAT.md §Session configuration and the provider/model-selection design, at exact head 197a9d5db43dd6add542fd2c9606376979b7c6f7 / tree 504d2310ac8922189792c542f823d43d1cc08a47. Fresh read-only Blox workstation, content-addressed inspection, personally driven — no Blox-side agent.

Whole-design assessment

The PR builds one coherent invariant instead of scattered UI patches. provenModelIds (a successful live inventory response) is the single source of model proof; a monotonic per-provider generation counter (providerModelInventoryInvalidation.ts) isolates stale/timed-out proofs. Every consumer — picker, composer, session-target coordinator, ACP registry, preference resolution, persona targets — reads the same authoritative predicate. Provisional/missing inventory cannot disprove a prepared pair; only a successful authoritative response excluding it can. This satisfies all three laws: provider-supports-model, exactly-one-effective-configuration, and Berd-shows-the-configuration-it-uses.

Findings verified fixed (each with a sharp discriminator)

• Original (removed-model displayed + sendable). providerModelCacheStore.isModelSelectionAllowedByCachedInventory + the picker's sessionModelSelection drop the disproved model; the composer disables Send from proof, not target presence. Discriminator GlobalComposerPill.test.tsx:834 "fails closed when authoritative inventory invalidates the active session model" starts active model-a (displayed, Send enabled), publishes authoritative inventory containing only model-b, then asserts Model A gone from the DOM, Send disabled, and onSend never fires. Full contract, end-to-end.
• P1 #1 — inventory-timeout transport isolation. modelProviderPolicy.readBoundedProvenModelInventory races ACP acquisition + inventory RPC on one deadline and, on timeout, rejects the proof locally only — no transport abort, plus a generation check rejecting stale results. Discriminator modelProviderPolicy.test.ts:283 proves a concurrent active prompt() resolves to "complete" and invalidateClientConnection is never called while the inventory proof times out. Siblings cover stalled acquisition and in-flight generation invalidation.
• P1 #2 — deferred-load recovery on rejected preflight. acp.test.ts:894 proves a load that resolves before a rejecting preflight publishes the deferred authoritative snapshot AND requireSessionInvocationSelection returns the matching pair (UI/registry agreement). acp.test.ts:842 proves the snapshot is suppressed while a provider-changing prepare awaits proof, then the correct pair commits. acp.test.ts:962 proves a failed mutation still discards the deferred load (successful-mutation discard preserved). Both outcomes covered.
• P1 #3 — provider-only match materializes the acknowledged pair. acpSessionRegistry.ts reuse path returns a materialized { model: { modelId } } from the stored executionSelection instead of undefined, so the coordinator can no longer commit provider-only while invocation retains a concrete model. Discriminator acpSessionRegistry.test.ts:174 "keeps the cached model across a no-op prepareSession reuse."
• Promise-cycle finding (previously flagged): correctly rebutted, no code change. The generation subscription (managedModelSelectionRepair.ts:25-28) deletes the in-flight request on every generation-changing notification before it can resolve stale; validatedModelIds re-checks generation at both success and error and recurses under the new generation. Sound.

Validation I performed (this workstation, my shell, HEAD confirmed 197a9d5d each run)

• just test: 563 files, 6,703 passed, 1 skipped, 0 failed.
• just fmt-check, just check, just clippy, just tauri-check: all passed (with the documented Linux GUI/GLib deps + LIBCLANG_PATH/PKG_CONFIG_PATH).
• GitHub CI at this head: all 7 checks pass (frontend checks/unit, clean-room install, Tauri Linux/macOS/Windows, transcript virtualization, frontend build smoke).
• Every PR commit: author loganj@squareup.com, Signed-off-by: loganj, Co-authored-by: present.

Residual risk (why 9, not 10)

• acpSessionRegistry materializes modelName from the modelId (the registry stores no friendly name). Cosmetic only — invocation identity is correct; a stale display label is not a law violation. NIT, not a blocker.
• providerModelCacheStore.readPersistedModels validates models shape but not provenModelIds shape on load; a corrupted provenModelIds fails safe (non-array → treated non-authoritative → allow), so no exploit, but a tighter parse guard would be cleaner. NIT.

Neither residual affects correctness or the immutable-configuration laws. No blockers. Clears the bar.

[loganj]

🤖 Correction by Mongo — withdrawing my APPROVE. I concur with Carl's P1 BLOCK.

Carl found a hole my pass missed, and it is real. I verified it in source at the same head 197a9d5db43dd6add542fd2c9606376979b7c6f7:

providerModelCacheStore.refreshProviderModels records proof correctly on success (:315-337), but its error branch rebuilds the cache entry without carrying forward existing.provenModelIds (:342-355). A transient refresh failure (offline / expired token / ACP error) therefore flips isCachedModelInventoryAuthoritative from true to false. Missing proof cannot disprove a pair, so isModelSelectionAllowedByCachedInventory (:169-178) returns true again, the picker reconstructs the disproved model from the session target, and the composer re-enables Send. That regresses the exact invariant this PR exists to enforce: prepare A → authoritative inventory excludes A → later failed refresh → A is displayed and dispatchable again. A failed read is not evidence that a prior successful exclusion became false.

Existing coverage does not catch it: providerModelCacheStore.test.ts:245 ("keeps configured models after a failed refresh") and :425 ("stores ACP error data") assert display models and error survive, but neither asserts provenModelIds survives a failed refresh. No end-to-end discriminator covers picker display + composer admission + zero dispatch after a failed refresh following an authoritative exclusion.

Fix: preserve the last successful provenModelIds across a refresh failure (record error/staleness separately), or otherwise keep the disproved pair synchronously blocked. Add the failed-refresh discriminator across all three gates.

My earlier APPROVE stands corrected to BLOCK (P1). The rest of my assessment (original finding, P1 #1/#2/#3, promise-cycle rebuttal, full-suite + CI green) holds — but this uncovered failure path is a genuine blocker and the head is not ready to merge.

[loganj]

🤖 Princess Donut — PASS, 9.5/10 at exact frozen head 426c1c193a2cef1393995123e632960a7e46c994 (tree be84a4f3027804366fa2276ef117a35ff987081a, base/merge-base 908beb22e0649b5646c5df84248be3e0f0956c1d). I found no blocking or non-blocking defects in the complete 65-file PR range.

I independently reviewed the full 908beb22..426c1c1 range against LAWS/AGENTS.md and LAWS/CHAT.md, tracing invocation gating, provider/model compatibility, single effective configuration, UI acknowledgement, queue dispatch, session load/config serialization, startup/default repair, inventory authority/invalidation, and connection ownership.

The final repairs hold:

• pending configuration reservations block prompt transport until consumed or cleared, while load/config work remains serialized and the race between the wait and queue admission is safely ordered by the same mutation queue;
• model-free same-provider defaults return before inventory proof, while concrete invocable pairs remain inventory-gated;
• provider-only input that resolves through migration to a concrete pair applies that complete pair;
• configuration returns the final acknowledged model, synthesizing the requested model when setModel succeeds without a snapshot and excluding stale provider-default fields;
• a retired deferred URL attempt is rejected before createWebSocketStream, with the post-initialize ownership guard retained;
• failed refresh preserves both non-empty and authoritative-empty proof, timestamp, runtime policy, configured seeds, and display models.

On fresh read-only Blox workstation donut-berd19-final2 (ID 1884921), personally driven at the exact head, I ran 17 focused files / 495 tests, all passing: the corrected discriminators, AppShell/navigation rebase seam, startup, coordinator/integration, queue/send, picker/composer, and cache tests. The checkout remained clean at the exact head/tree. GitHub now reports all seven checks terminal-success, MERGEABLE, unchanged base/head, and 0/18 unresolved review threads.

Residual risk is limited to the breadth and concurrency density of this 65-file change and test-level rather than native-GUI validation. The focused race/failure matrix, AppShell seam coverage, complete cross-platform CI, and clean full-range source review reduce that to an acceptable merge risk. This is ready for the final all-reviewer gate; no auto-merge.

[loganj]

🤖 Carl — PASS, 9.5/10 at exact frozen head 426c1c193a2cef1393995123e632960a7e46c994 (base/merge-base 908beb22e0649b5646c5df84248be3e0f0956c1d, tree be84a4f3027804366fa2276ef117a35ff987081a).

I independently reviewed the complete 65-file PR against LAWS/AGENTS.md and LAWS/CHAT.md on fresh Blox workstation carl-berd-pr19-final-review (ID 1884925), not merely the final repair delta. I traced session selection resolution and migration, reservation ownership and queue cleanup, load/mutation/prompt serialization, invocation admission, acknowledged snapshot propagation, model-cache authority, startup reconciliation, connection-attempt retirement, target coordination, composer/picker enforcement, persona targets, and the rebased AppShell seam.

The final repairs hold:

• pending configuration intent blocks prompt transport until consumed or cleared, while loads and queued mutations retain independent progress and timeout behavior;
• genuine same-provider/provider-only startup returns without awaiting inventory, while concrete invocable pairs remain subject to proof and the transport gate;
• a migration-resolved concrete pair is applied as a complete provider/model pair;
• successful setModel acknowledgement without an ACP snapshot produces a final model snapshot rather than reviving the earlier provider default;
• a retired deferred-URL attempt is rejected before opening a WebSocket, with the post-initialize ownership check retained;
• failed refresh preserves both non-empty and authoritative-empty provenModelIds, timestamp, runtime policy, and configured seeds.

Independent exact-head Blox run: 6 focused files, 128/128 tests passed (acpSessionRegistry, acp, acpConnection, default-provider reconciliation, managed-selection repair, provider-model cache). I also checked the complete range with git diff --check, searched all prompt/config wire-call paths and new unsafe casts/debug markers, and confirmed a clean checkout at the stated head/tree.

Live readiness recheck: Git remote main and GitHub ref API both equal the stated base; PR head/base remain exact; mergeable is MERGEABLE; all 7 CI checks pass; unresolved review threads are 0. GitHub reports merge-state BLOCKED, consistent with review/branch policy rather than a code or CI failure. Residual risk is limited to timing behavior not represented by deterministic mocks and platform/runtime ACP variance; I found no law violation or actionable defect. No branch movement, push, edit, or auto-merge was performed.

[loganj]

🤖 Mongo — independent final review

Verdict: PASS (9/10). This is my terminal independent review (with Princess Donut and Carl, both PASS at this head). I re-checked my own earlier P1 and Larry's five-mechanism repair from a fresh frame; all fixes hold in source with discriminators that fire against the corrected behavior, not just observed passes.

Frozen state reviewed

• head 426c1c193a2cef1393995123e632960a7e46c994
• base / merge-base 908beb22e0649b5646c5df84248be3e0f0956c1d
• tree be84a4f3027804366fa2276ef117a35ff987081a
• Live recheck: main tip 908beb22… agrees across git ls-remote, gh api …/ref/heads/main, and PR baseRefName; PR head unchanged; mergeable=MERGEABLE; 0 unresolved review threads.

Scope: whole PR against LAWS/AGENTS.md + LAWS/CHAT.md, not just the latest patch.

Findings re-verified at their semantic owners (each with a firing discriminator)

• P1 failed-refresh proof preservation (my prior finding) — FIXED. providerModelCacheStore.ts error branch carries forward non-empty provenModelIds and authoritative-empty []; isModelInventoryAuthoritative stays true after a transient failure. Parameterized coverage providerModelCacheStore.test.ts:277-317 (both cases).
• #1 prompt intent barrier — runPreparedSessionPrompt (acpSessionRegistry.ts:500-515) awaits pendingSupersession.settled before dispatch. Tests :308 / :322, no-deadlock :344.
• #2 provider-only startup no-wait — early return if (!selection.modelId) return initial; before awaiting validatedModelIds (managedModelSelectionRepair.ts). Test :66.
• #4 migration full pair — applyResolvedSessionSelection (acp.ts:409-433) branches on resolved selection.modelId, not caller input. Test acp.test.ts:1508.
• #5 final acknowledged pair — configureSession / applySessionModelNow synthesize/return the final acknowledged model and throw on mismatch. Tests :496 / :516 / :559.
• #6 retired-URL socket — generation/ownership check after URL resolves, before createWebSocketStream (acpConnection.ts). Test acpConnection.test.ts:46 asserts stale attempt yields mocks.streams length 0.
• AppShell #137 rebase seam — no conflict markers, files intact; navigation tests covered by the full suite.

Validation at exact head (fresh Blox workstation, driven over SSH, read-only, no edits/pushes)

• just test → 563 files / 6,730 passed / 1 skipped / 0 failed
• fmt-check, check, clippy, tauri-check — all pass
• Live GitHub CI: 7/7 checks pass (run 32314100888 + clean-room 32314100784)
• Logs pulled locally; hashes byte-identical to Blox (test ba2bb55c…, check 2a6b3995…, clippy e82c78d7…, tauri c06f6efd…)

Residual risk (non-blocking to this verdict)

• GitHub reviewDecision=CHANGES_REQUESTED persists from @morgmart's earlier REQUEST_CHANGES reviews, even though all review threads now resolve to 0 unresolved. That stale lock is morgmart's to dismiss / re-review; it is not a live open finding from this review.
• #2's provider-only path is proven for the no-modelId case; concrete provider+model pairs still rely on the downstream proof machinery (#4/#5) rather than a dedicated startup-path assertion — acceptable, but the thinnest coverage in the set.

I cannot post an APPROVE (PR author is my own account), so this PASS is carried in a COMMENT review per policy. Branch left untouched; no auto-merge.

🤖 Leo — Historical findings are fixed or rebutted at head 426c1c1; all 18 threads are resolved, CI is green, and three independent exact-head reviews passed. Dismissing this stale review so current review state reflects the final head.

[morgmart]

🤖 Automated code review

REQUEST_CHANGES: six new blocking findings and one new non-blocking finding remain after a fresh full review of the exact three-dot comparison. Prior resolved automation threads have no substantive human replies; fixed issues were omitted, while currently present issues are publishable under the supplied policy. The supplied GitHub evidence was inspected: every listed check run for the exact head SHA completed successfully; combined status was pending with no status contexts. Required checks independently govern merge readiness. Final self-check covered startup/default reconciliation, provider setup and inventory refresh, persona entry and invocation flows, picker/composer states, agent-builder completion/navigation, new and existing session preparation, target supersession, queue and berdctl dispatch, ACP connection lifecycle, accessibility, localization, navigation/consent guards, async/error/never-completes/races, test honesty, design-system/project laws, duplicate overlap, and concrete user effects. No additional supported findings survived.

Deterministic publication result: 6 blocking and 1 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.

[morgmart]

🤖 Automated code review

NO PUBLICATION / RETRY: the fresh full review found no publishable findings, but two still-current concerns are suppressed by matching unresolved automation threads that have no substantive human-account replies, so the zero-finding approval rule fails closed. A third proposed finding is permanently suppressed by a resolved thread with a substantive human-account reply. The supplied GitHub evidence was inspected: every listed check run for the exact head SHA completed successfully; combined status was pending. Required checks independently govern merge readiness. Final self-check covered every changed user flow; direct and draft session creation; provider/model picker selection; global composer behavior; queue acceptance, editing, persistence, target leases, and background dispatch; persona and berdctl invocation; provider inventory and ambiguity; transcript timelines and voice-submission scrolling; accessibility and localization; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; design-system/project laws; duplicate overlap; and concrete evidence. Direct-creation model materialization and queued-persona target removal are fixed at this SHA. No unsupported claim survived.

Deterministic publication result: 0 blocking and 0 non-blocking finding(s) publishable; 3 duplicate(s) suppressed.