Skip to content

fix: enforce immutable session configuration laws - #19

Open
loganj wants to merge 50 commits into
mainfrom
larry/provider-selection-laws
Open

fix: enforce immutable session configuration laws#19
loganj wants to merge 50 commits into
mainfrom
larry/provider-selection-laws

Conversation

@loganj

@loganj loganj commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

🤖 Other Brother Darryl

Summary

A chat session could previously show one provider or model while using a different configuration after asynchronous updates completed out of order. That made the model shown in the interface unreliable and could send a message through a stale target.

This PR makes the session's effective provider and model configuration authoritative and ordered. Newer user choices remain in control, stale responses are ignored, and the configuration shown in the interface stays aligned with the one used for session work and prompt dispatch.

Details

  • Preserves an explicit provider and model choice while authoritative model information is unavailable. A successful provider response can repair or clear the choice only when it proves that the selection is unsupported.
  • Applies the same configuration rules when loading saved personas, choosing defaults, creating or preparing sessions, changing a model, and sending a prompt.
  • Keeps external agent harnesses distinct from Goose model providers and enforces provider policy synchronously during prompt dispatch without adding model-inventory work to the send path.
  • Round 12 regression coverage drives provider and model responses out of order through the production chat-session snapshot-handler adapter, the real chat-session store, and the session registry. It proves that stale responses cannot overwrite newer store targets or registry selections. The coverage is test-only; production source bytes are unchanged.

Compatibility

The implementation follows the existing session lifecycle and composer-queue dispatch behavior 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.

@loganj
loganj force-pushed the larry/provider-selection-laws branch 4 times, most recently from 4970ced to 1d3682e Compare August 19, 2026 04:02
@loganj
loganj marked this pull request as ready for review August 19, 2026 11:20
@loganj
loganj requested a review from a team August 19, 2026 11:20

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Comment thread src/features/agents/lib/personaExecutionTarget.ts
Comment thread src/shared/api/acp.ts Outdated
Comment thread src/shared/api/acp.ts
Comment thread src/shared/api/acpConnection.ts Outdated
Comment thread src/shared/api/acpConnection.ts

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Comment thread src/features/providers/lib/managedModelSelectionRepair.ts
Comment thread src/shared/runtime-config/modelProviderPolicy.ts Outdated
Comment thread src/shared/api/acpSessionRegistry.ts
Comment thread src/features/chat/lib/sessionTargetCoordinator.ts
@loganj
loganj force-pushed the larry/provider-selection-laws branch from 9bd954e to af8f7da Compare August 19, 2026 21:40

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Comment thread src/features/providers/stores/providerModelCacheStore.ts
Comment thread src/shared/api/acp.ts Outdated
Comment thread src/features/chat/lib/sessionTargetCoordinator.ts
@loganj
loganj force-pushed the larry/provider-selection-laws branch from af8f7da to 197a9d5 Compare August 19, 2026 22:13

@loganj loganj left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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 loganj left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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 morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Comment thread src/shared/api/acpSessionRegistry.ts
Comment thread src/features/providers/lib/managedModelSelectionRepair.ts
Comment thread src/features/providers/stores/providerModelCacheStore.ts
Comment thread src/shared/api/acp.ts Outdated
Comment thread src/features/chat/lib/sessionTargetCoordinator.ts
Comment thread src/shared/api/acpConnection.ts

@loganj loganj left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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 loganj left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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
loganj force-pushed the larry/provider-selection-laws branch 3 times, most recently from cf918d3 to 426c1c1 Compare August 19, 2026 23:38

@loganj loganj left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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 loganj left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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 loganj left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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 barrierrunPreparedSessionPrompt (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 pairapplyResolvedSessionSelection (acp.ts:409-433) branches on resolved selection.modelId, not caller input. Test acp.test.ts:1508.
  • #5 final acknowledged pairconfigureSession / 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.

@loganj
loganj dismissed stale reviews from morgmart, morgmart, morgmart, and morgmart August 20, 2026 00:08

🤖 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 morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Comment thread src/features/chat/lib/sessionTargetCoordinator.ts Outdated
Comment thread src/shared/api/acpSessionRegistry.ts
Comment thread src/features/providers/defaultProviderConfig.ts
Comment thread src/app/AppShell.tsx
Comment thread src/features/chat/hooks/useChatSessionController.ts
Comment thread src/features/chat/hooks/useResolvedAgentModelPicker.ts Outdated
Comment thread src/shared/ui/GlobalComposerPill.tsx
loganj and others added 20 commits August 21, 2026 02:26
Rebasing onto main introduced reserveAcpSessionConfiguration (a new
5th argument threaded through acpPrepareSession) and provenModelIds
(the requirement that a default/configured model selection prove
itself via a live inventory response before being surfaced). Update
mocks and assertions across the affected suites to match both
contracts instead of weakening them.

Signed-off-by: loganj <loganj@squareup.com>
Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz>

Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz>

Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz>

Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz>
Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz>

Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <8cf5a83f590ec0955b11647d1c88f796a98e088c30a492c58e0e46c3026ae7a4@buzz.block.builderlab.xyz>

Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <larry@buzz.local>
Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <larry@buzz.invalid>
Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <larry@buzz.local>
Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <larry@buzz.local>

Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <larry@buzz.local>

Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <larry@buzz.local>
Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <larry@buzz.local>

Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <larry@buzz.local>

Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <larry@buzz.local>

Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <larry@buzz.local>
Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Larry <larry@buzz.local>

Signed-off-by: loganj <loganj@squareup.com>
Co-authored-by: Other Brother Darryl <other-brother-darryl@buzz.local>

Signed-off-by: loganj <loganj@squareup.com>
@loganj
loganj force-pushed the larry/provider-selection-laws branch from aeea72e to fa62199 Compare August 21, 2026 04:09
@loganj
loganj dismissed stale reviews from morgmart, morgmart, morgmart, morgmart, morgmart, morgmart, morgmart, and morgmart August 21, 2026 04:22

🤖 Leo: Dismissing this stale automated changes-requested review. Exact final head fa62199 addresses its findings; all 54 review threads are resolved with rendered evidence replies, and final-head CI is green. This dismissal does not approve or merge the PR.

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

REQUEST_CHANGES: two new blocking and two new non-blocking findings remain after a fresh full review of the exact three-dot comparison. Two additional proposed findings are permanently suppressed because resolved authoritative threads contain substantive human-account replies. There are no unresolved prior automated threads at this SHA. 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; ACP provider/model and generic configuration mutations; timeout, cancellation, late resolve/reject, lifecycle, and queue races; session creation and target reconciliation; berdctl creation; persona mention detection, dismissal, autocomplete, and invalid-agent recovery; accessibility and localization; navigation and consent guards; test honesty; design-system/project laws; duplicate overlap; and concrete user effects. The prior timed-out prepare success race and ineligible-provider ambiguity are fixed at this SHA. No additional supported finding survived.

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

isLatest: () => boolean,
sequence: number,
queue: SessionMutationQueue,
canPublish: () => boolean,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 P1 · Timed-out config still publishes (blocking)

The new timeout publication guard is threaded through provider and model mutations, but applySessionConfigOption still ignores the canPublish callback supplied by serializeSessionMutation and calls an API that unconditionally publishes its response snapshot. After this generic config request times out and newer queued work succeeds, a late response can still apply stale model or reasoning snapshots over the current session state.

User effect: After a timeout and successful retry, a delayed reasoning/config response can silently revert the visible or effective chat configuration.

Recommended fix: Pass the mutation-owned canPublish guard into setSessionConfigOption and gate its snapshot publication and any registry-side mutation exactly like setProvider and setModel.

Test: Let a generic session-config request time out, complete newer provider/model configuration, then resolve the old request and assert neither visible snapshots nor prepared invocation state changes.

snapshots = await acpApi.setModel(sessionId, modelId, {
providerId: executionSelection.providerId,
requestId: options.requestId,
canPublish,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 P1 · Late model rejection clears newer state (blocking)

applySessionModelNow passes canPublish to response publication, but its catch path always calls replaceExecutionSelection. If the model RPC times out, the queue admits a newer preparation; when the old RPC later rejects, this stale catch can clear the newer entry's model despite having lost publication ownership.

User effect: A chat successfully recovered after a model timeout can later become unprepared or fail its next send when the old request finally rejects.

Recommended fix: Guard catch-side registry mutation with canPublish and, ideally, verify entry identity or mutation generation before changing executionSelection.

Test: Time out model A, prepare model B successfully, then reject A's original promise and assert model B remains the prepared, displayed, and invokable selection.

type="button"
variant="alert"
size="sm"
onClick={() => handlePersonaChange(null)}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 P2 · Continue leaves invalid mention (non-blocking)

The Continue without this agent action clears persona intent but does not remove the completed @display Name text inserted when the invalid persona was chosen through autocomplete. That text is then sent as ordinary prompt content to the fallback assistant, so the visible recovery says the agent was removed while the prompt still references it.

User effect: A user who continues without an invalid agent can unknowingly send a stale agent mention to the default assistant.

Recommended fix: When clearing the invalid persona, also remove the completed persona mention owned by that selection, while preserving unrelated user-authored text.

Test: Select an invalid persona through autocomplete, choose Continue without this agent, and assert persona intent and only its inserted mention are removed before send.

Comment thread src/app/AppShell.tsx
catalogEntries: getProviderCatalog(),
});
if (personaResolution.status === "invalid") {
setActiveView("agents");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 P2 · Invalid-agent redirect skips cleanup (non-blocking)

The invalid target branch directly sets the Agents view and persona ID instead of using the coordinated agent-navigation path. It therefore skips cleanup such as clearing active session/settings route and dismissing or transitioning a centered global composer.

User effect: Launching an invalid agent from a chat or Settings can leave stale navigation state or a centered composer over the agent repair screen.

Recommended fix: Route invalid-agent repair through the shared agent navigation helper so all session, URL, and composer cleanup stays consistent.

Test: Attempt to start an invalid agent from an active chat and from Settings with the centered composer open; assert the repair screen opens with stale surfaces and session selection cleared.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants