fix(runtime-policy): release the default target instead of rejecting the selection that invalidates it - #3126
Conversation
…he selection that invalidates it Unchecking the model a catalog's default target names failed the whole update with `invalid_default_target`, so a user could never disable their own default model. Settings surfaced it as "模型连接服务暂时不可用，请稍后重试", which no retry could clear. The catalog already treated the target as a pointer that follows its selection: removal cleared it, discovery and onboarding moved it. Only `update` read the same consequence as a caller error. Collapse those four paths into one next-version constructor that reconciles the target by construction — it keeps the target a caller prefers, falls back inside that target's own connection, and releases to null so the bootstrap layer keeps owning which connection carries the default. `invalid_default_target` now belongs to `setDefaultTarget` alone, the one call that states a target itself rather than changing what one points at, and the update result contract narrows to match. Generated-by: Claude Code
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review. 📝 WalkthroughProblem solvedThe PR fixes runtime-policy updates when a catalog’s default target is unchecked. The catalog now keeps a default only when it references an enabled model on an enabled connection. Otherwise, the update releases the default to DesignThe PR extends the existing catalog document source of truth. Catalog mutations use This path covers connection updates, removals, default-target changes, onboarding, model-fetch patches, and test-clearing writes. It does not create a parallel update path. Discovery can move defaults across provider model renames. Onboarding can seed an initial default but does not replace an existing target during model-selection changes. Scope and complexityThe change is the smallest coherent solution shown by the diff. Centralized reconciliation removes duplicated and inconsistent default-target decisions. The protocol result type and coordinator handling are simpler because connection updates no longer return No code or tests can be deleted without weakening behavior or regression coverage. Tests and validationRegression tests cover removal of the selected model, disabling the connection, and rejection of explicitly unselected targets without catalog changes. The compatibility test verifies that the decoder rejects the retired result variant when the compatibility epoch is greater than 21. Affected package tests, type checks, and formatting checks passed locally. Repository-wide tests and Playwright E2E tests were not run. Required-check status remains unverified without direct CI evidence. Review-relevant risksThe change affects user-visible default-target selection and persistence. Material behavior changes require independent human review under repository policy. The change narrows the public No security, licensing, or governance effect was identified in the current diff. The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughCatalog mutations now centralize default-target reconciliation. Invalidated targets are cleared instead of replaced. Explicit invalid targets remain caller-facing errors. Runtime update results no longer include ChangesDefault target reconciliation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to This change releases invalidated defaults and updates the host compatibility epoch while affected tests and typechecks pass. Mixed-version handshake rejection is not directly exercised, so merge is reasonable with explicit owner awareness and follow-up coverage for that compatibility path. Sequence Diagram(s)sequenceDiagram
participant CatalogMutation
participant nextDocument
participant retainedDefaultTarget
CatalogMutation->>nextDocument: apply catalog mutation
nextDocument->>retainedDefaultTarget: validate target
retainedDefaultTarget-->>nextDocument: retain target or clear target
nextDocument-->>CatalogMutation: write revised catalog
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…to another model Adversarial review caught the repair contradicting a rule this codebase already states: `reconcileConnectionAfterEnabledModelsChange` says that dropping the model which happens to be the default drops the default with it, rather than moving it to some other member of the set. Which model a new chat starts on is 设置 · 通用's one control, and picking a replacement from the connection page answers that question where it is no longer asked. Config import already obeyed this; the catalog now does too, so the two entries stop disagreeing. Releasing rather than repairing also collapses the rule to what it always was — a target is either absent or names an enabled model of an enabled connection — so `create` and `clearAllConnectionLastTests` can join the single next-version constructor and make its claim true. Two paths keep a say the constructor cannot make for them: - Discovery MOVES a target rather than keeping or dropping it, carrying the default across a provider's model rename by alias. That is a migration of the same choice, so `writeModelFetchResult` fails closed when the reconciler hands it a default outside the selection it just decided, instead of letting the constructor silently release a user's default on an upstream bug. - Onboarding seeds the first default when the catalog has none, and its no-op short-circuit now asks the same question the constructor would, so an already doomed target still counts as a write to make rather than resting on the caller having read a validated document. Generated-by: Claude Code
Astro-Han
changed the title
…e code cannot say The longest of them restated the rule `reconcileConnectionAfterEnabledModelsChange` already documents, which puts one rule in two places to keep in sync. Point at it instead, and cut the rest to the part a reader would otherwise have to reconstruct: why the catalog releases a default rather than choosing a replacement, why discovery still fails closed, and why the onboarding short-circuit asks what the constructor would. Generated-by: Claude Code
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
[Important] Preserve compatibility with epoch-21 Hosts
This PR removes invalid_default_target from UpdateCatalogConnectionResult and changes decodeUpdateConnectionResult() to decode every non-committed response as connection_stale. However, RUNTIME_HOST_COMPATIBILITY_EPOCH remains 21.
An older epoch-21 Host can still legitimately return:
{ kind: "invalid_default_target", target: ... }This happens, for example, when the user disables the current default connection or removes its selected model. A new Client accepts that Host during the handshake, but then attempts to decode the response as connection_stale.
I reproduced the resulting failure using the codec built from this PR:
RuntimeHostProtocolError: Unknown connection stale conflict field
The response decoder failure subsequently fails the Client connection. Therefore, this is a wire-compatibility regression rather than only a TypeScript API simplification.
The most compatible fix would be to:
- Keep accepting the legacy
invalid_default_targetvariant in the wire result union and decoder. - Let the new Host implementation stop producing that variant.
- Add a codec regression test covering an epoch-21 legacy response.
This preserves interoperability with existing local or remote Hosts without forcing their replacement. If removing the wire variant is intentional, the compatibility epoch must instead be incremented to 22 so that incompatible Client/Host combinations are rejected during the handshake.
The new default-target reconciliation itself looks well designed: invalidated defaults are cleared atomically, valid defaults are retained, and explicit invalid setDefaultTarget requests remain rejected. I did not find another blocking issue in that behavior.
中文
[重要] 需要保留与 epoch-21 Host 的兼容性
该 PR 从 UpdateCatalogConnectionResult 中删除了 invalid_default_target，并让 decodeUpdateConnectionResult() 将所有非 committed 响应都按 connection_stale 解码，但 RUNTIME_HOST_COMPATIBILITY_EPOCH 仍然是 21。
旧的 epoch-21 Host 仍可能合法返回：
{ kind: "invalid_default_target", target: ... }例如，用户禁用当前默认连接，或从启用模型中移除当前默认模型时就会触发该路径。新 Client 会在握手阶段接受这个 Host，但随后会尝试把该响应解码为 connection_stale。
我使用该 PR 构建出的 codec 进行了复现，结果为：
RuntimeHostProtocolError: Unknown connection stale conflict field
响应解码失败随后会使 Client 连接整体失败。因此，这不仅是 TypeScript API 的简化，而是实际的 wire compatibility 回归。
兼容性最好的修复方式是：
- 在 wire result union 和 decoder 中继续接受 legacy
invalid_default_target。 - 新 Host 的实现不再产生该分支。
- 增加一个覆盖 epoch-21 legacy 响应的 codec 回归测试。
这样可以继续兼容现有的本地或远程 Host，而无需强制替换它们。如果确实有意删除该 wire variant，则应把 compatibility epoch 提升到 22，让不兼容的 Client/Host 组合在握手阶段就被拒绝。
除此之外，新的 default-target reconciliation 设计是合理的：失效的默认目标会被原子清空，有效目标会被保留，显式设置无效默认目标的请求也仍然会被拒绝。我没有在这部分发现其他阻塞问题。
…d default target Narrowing `UpdateCatalogConnectionResult` changed which values may cross the wire, but left the compatibility epoch at 21. An epoch-21 Host answering a connection update with `invalid_default_target` — exactly what it does when a selection strands the default — passes this Client's handshake and then fails its decoder with `Unknown connection stale conflict field`, taking the whole connection down. Keeping the retired variant decodable would need the coordinator and IPC branches back with it, which is the dead path this change exists to remove. The epoch is the mechanism for a wire set that no longer matches: incompatible pairs are rejected at the handshake, where the existing replacement and upgrade paths already handle them. Reported in review by M4n5ter, who reproduced the decoder failure. Generated-by: Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e7b77a16-4637-4776-949f-d6fca6af55d1
📒 Files selected for processing (2)
packages/runtime-host/src/__tests__/protocol.test.tspackages/runtime-host/src/protocol/index.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
… to a literal Asserting the constant equals 22 restated the implementation and would collide with the next legitimate increment. What this change actually guarantees is a pair: a wire set that no longer accepts `invalid_default_target` must have left behind the epoch whose Hosts still answer that way. `> 21` alongside the decoder assertion holds both halves, and either one regressing alone fails. Generated-by: Claude Code
|
@M4n5ter Confirmed — fixed in Went with your second option (epoch 21 → 22) rather than keeping the legacy variant decodable. The coordinator and IPC branches that consumed Your third point is in Investigation and fix here were AI-assisted (Claude Code), as noted in the PR description. |
Adversarial review: Approve (merge recommended), with two non-blocking cleanupsFor the problem this PR solves — the connection-catalog default-target invariant (when a selection invalidates a target, the default-target should be released, not have the selection rejected) — the core change is the optimal solution and is good to merge. Why it's mergeable
Worth folding in before merge (code quality, not correctness-blocking)
VerdictDirection and implementation are both correct — mergeable. If the team allows "merge now, follow-up later" → Approve. If "zero dead code into main" is required → remove the two dead-code blocks and strengthen that test within this PR first. 中文（点击展开）对抗性审查结论：建议合并（Approve），附两处非阻塞清理针对本 PR 要解决的问题（连接目录 default-target 不变量：当选择使某目标失效时，应释放该 default-target 而非拒绝选择），核心改动是最优解，可以合入。 为什么可以合
合并前建议一并收掉（不阻塞正确性，属代码质量）
结论：方向与实现都正确，可合。若团队流程允许「先合再跟 follow-up」→ Approve；若要求「零死代码入主干」→ 请在本 PR 内删掉两段死代码并补强该测试后再合。 |
The onboarding short-circuit's `retainedDefaultTarget(...) === defaultTarget` check is only reached after `isDeepStrictEqual(current.defaultTarget, defaultTarget)` passes, at which point the target is the current document's own — and every document in memory already satisfies the target invariant: read() rejects invalid targets on load and nextDocument reconciles them on write. The guard could never be false, and its comment described a doomed target that cannot arrive here. Review finding by liugddx on #3126. Generated-by: Claude Code
|
@liugddx Thanks for the adversarial pass. Verdict on the three cleanups: Onboarding guard — confirmed, fixed in Discovery throw — keeping it. It is unreachable only while the core reconciler upholds its documented contract, which is the point: discovery is the one path allowed to move a target, and Test rewrite — keeping as is. All quadrants have real assertions: the retain branch is covered by the pre-existing update test (a still-valid target survives an unrelated update), the two new release tests cover both Adjudication and fix were AI-assisted (Claude Code), as disclosed in the PR description. |
3 participants
Summary
Unchecking the model a catalog's default target names failed the whole update with
invalid_default_target, so a user could never disable their own default model. Settings surfaced it as "模型连接服务暂时不可用，请稍后重试" — a message no retry could clear, because nothing was temporarily unavailable.The rule this should have followed was already written down.
reconcileConnectionAfterEnabledModelsChangeinpackages/core/src/llm-connections.tsstates it for a connection: a default is either absent or a member of the enabled set, and dropping the model that happens to be the default drops the default with it rather than moving it to some other member — which model a new chat starts on is 设置 · 通用's one control, and picking a replacement from the connection page would answer that question where it is no longer asked. Config import already obeyed this; the catalog document did not, and instead rejected the write.So the catalog now states the same rule for itself, in one place:
nextDocumentis the only next-version constructor, and it keeps a default target exactly when the target still names an enabled model of an enabled connection. Callers state the target they want kept and never the one they have to police, which retires the four different answersupdate,remove,writeModelFetchResult, andprepareOnboardingUpsertused to give the same question.Two paths keep a say the constructor cannot make for them:
writeModelFetchResultfails closed if the reconciler ever hands it a default outside the selection it just decided, so an upstream bug surfaces where it is attributable instead of silently releasing a user's default.invalid_default_targetnow belongs tosetDefaultTargetalone — the one call that states a target itself rather than changing what one points at — andUpdateCatalogConnectionResultnarrows to match, retiring the unreachable branches in the coordinator and the protocol decoder.Verification
Replayed the reported scenario against a copy of a real workspace document (default target
grok-4.5, selection["grok-4.5","grok-4.6"]), uncheckinggrok-4.5:maininvalid_default_target— the reported failurecommitted, default target released tonull, persisted to diskRan locally:
packages/storage—runtime-policy-stores.test.js44/44, including three new regressions: release on selection change, release on connection disable, andsetDefaultTargetrejecting a stated target that names an unselected model (that path had no coverage before)packages/runtime-host—runtime-policy-coordinator15/15,bootstrap-runtime-policy5/5,connection-effect-coordinator13/13apps/desktop—runtime-host-connections-ipc-main10/10typecheckfor@maka/storage,@maka/runtime-host,@maka/desktop;npm run format:checkNot run: the repository-wide suite and Playwright E2E — left to CI. Note that
npm run astryx:surface-inventoryfails on this branch, but the drift is inpackages/ui/src/chat-turn.tsxand pre-exists onmain; it is untouched by these four files.Review focus
No default Session model is configured, Daily Review raisesAuxiliaryModelCallConfigurationError, and task submission reportsmissing_default. Recovery is the user re-picking a default in 设置 · 通用 —ensureBootstrapRuntimePolicyreturns early once the catalog is non-empty, so nothing re-seeds it. That is the existing product rule, followed rather than introduced here, and it is why the readiness gate reportsmissing_model.read()continues to reject a document whose stored target is invalid. Releasing is what a mutation does to a consequence it caused; a persisted invalid target means the document is already corrupt, which is a different claim and stays fatal.Breaking change
UpdateCatalogConnectionResultno longer includesinvalid_default_target. A connection update can no longer fail that way, so callers that branched on it were handling an outcome the store stopped producing.That is a change to the set of values which may cross the wire, so
RUNTIME_HOST_COMPATIBILITY_EPOCHgoes 21 → 22. Without it, an epoch-21 Host answering a connection update with the retired variant passes this Client's handshake and then fails its decoder withUnknown connection stale conflict field, taking the connection down. At epoch 22 the pair is rejected at the handshake instead, where the existing replacement and upgrade paths already handle it. An older Host still running after an upgrade therefore needs one restart.Behavior change beyond the fix: disabling a connection that carries the default target now commits and releases the default, where it previously failed the update outright.
AI use
Select exactly one:
Tool(s) and scope: Claude Code — diagnosis, implementation, tests, and this description. Adversarial review by Codex and an independent Claude subagent found that the first commit moved the default to another model instead of releasing it, contradicting the stated rule; the second commit corrects that. The human contributor of record reviews the final diff and owns the merge decision.
Checklist
Does this PR entail a change in behavior?