fix(runtime-host): import external sessions without a configured default model - #4227
fix(runtime-host): import external sessions without a configured default model#4227liuxiaocs7 wants to merge 1 commit into
Conversation
97de1f6 to
c432c0d
Compare
2e04bab to
5f8b37f
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The bug is real and it is worse than the issue says. retainedDefaultTarget (packages/storage/src/runtime-policy/connection-catalog-document.ts:926) drops defaultTarget to null whenever the connection it names is disabled or the model it names is unchecked, and its own comment says nothing picks a replacement. ensureBootstrapRuntimePolicy only sets a default when the catalog is empty. So defaultTarget: null is not a self-configured profile edge case; it is where any user lands the first time they disable their default connection, and from then on external import fails for every conversation with a banner that tells them to check the source. Worth fixing.
The fix lands on the right owner. #resolveImportModel only enumerates candidates and hands each to the unchanged #resolveModel, so resolveExecutionConnection and authorizeConnectionModel stay the sole readiness authority. I enumerated what those can return and the skip list is complete: under an explicit target the only reachable codes are operation_conflict, invalid_request and operation_unavailable, and the loop skips exactly the two that mean "this candidate is unusable". The whole loop is read-only, so abandoning a candidate leaves no partial authorization, and #resolveTarget runs before any commit, so a total failure leaves no half-created session.
The best decision in the diff is the one-line change in execution-composition.ts. Splitting resolveDefaultCreateTarget out keeps WorkHub's autonomous create fail-closed, so the fallback only applies where a person explicitly asked to import. That is the one place where silently picking a model would be genuinely dangerous, and you found it yourself and pinned it with a test. It also retires a method name that no longer described what it did.
Before merge
The banner points at a settings section that does not exist. importFailedNoModel says 设置 › 服务商 and Settings › Providers. The section is models, labelled 模型 and Models; there is no 服务商 or Providers entry anywhere in settings navigation. Every one of the dozen existing remedy strings in conversation-copy.ts:467-478 says 设置 · 模型 and Settings · Models, with that separator. This is the PR's one actionable instruction, in the PR whose whole point is making the failure actionable, and it sends the user somewhere they cannot find.
The epoch has already been taken. Main is at 107 for token_usage anchors; this branch bumps 106 to 107 with its own note. The conflict is exactly that line. On rebase the value is 108, and it needs re-deriving at the moment you rebase rather than assumed, since another open PR may claim it first, which the file's own // 103: note records happening before. In the same edit, protocol.test.ts's new assert.ok(RUNTIME_HOST_COMPATIBILITY_EPOCH > 101) needs to move: the epoch was already 106 at the merge base, so that assertion passes with no bump at all and cannot catch the epoch being lost in the conflict, which is the precise failure a conflicted epoch line invites. The adjacent assertion uses > 99 for 100.
The candidate list multiplies connections by models. importModelCandidates emits one candidate per enabled model per enabled connection, and a connection-level failure such as a missing credential or a retired provider returns the same verdict for every one of that connection's models. A connection may enable up to 512 models, so an unusable connection early in catalog order costs up to 512 resolveExecutionConnection calls before the loop reaches the next connection, each one a full runtime-policy lane round trip that reads the catalog document and prepares connection material. That lane is shared with every other runtime-policy read and write, so the connections settings page queues behind an import. Filtering at enumeration with the pure authorizeConnectionModel and isModelExplicitlyUnsupportedForChat gives at most one candidate per connection and keeps #resolveModel as the only readiness arbiter, which is what the comment at :196 already promises.
Nothing tests the sentence the summary rests on. "Behavior is unchanged when a default is set and ready" is the one precision claim in the change, and deleting the entire preferred block from importModelCandidates leaves all six new tests green: two run with defaultTarget: null, one has an unready default, and two go through resolveDefaultCreateTarget and never reach the enumerator. A case with a ready default that is not first in catalog order, asserting it wins, is the missing one.
Smaller
The comment at :283-284 justifies keeping WorkHub fail-closed by saying its 配置默认模型 startup banner depends on the throw. That banner comes from the missing_default_connection readiness verdict in session-send-projection.ts:110 and does not depend on this throw at all. Keeping WorkHub fail-closed is right; the stated reason is not, and this is exactly the comment someone will read before deciding to merge the two resolvers back together.
The batch paragraph in the summary is backwards. Before this PR, operation_unavailable was rethrown, so the batch path already recorded it as failed. It is this PR that starts returning {ok: false} with a reason, which is what would sweep it into the maybe-landed bucket and disable the whole page's import buttons. So that change is required to prevent a regression this PR introduces, not a fix for an existing bug, and describing it the other way makes the next reader think it is optional polish.
sawNoModel and ImportBatchOutcome.noModel hold one fact in two places; the local can go and the branch can set the field directly.
source_unreadable is broader than its copy. The coordinator maps every pre-commit throw other than not-found into it, so an EACCES, an EMFILE or an adapter bug all tell the user the conversation may be too large or malformed. The wording hedges with 可能 and may be, so nobody is led into a dead end, but listing causes rather than asserting one would be truer.
The "Review follow-ups" section describes feedback that is not on this PR: no reviews, no comments, no inline threads. Say where it came from or fold it into the summary, because a reader cannot check it. And the four screenshots are hosted on a release tag in your own fork, so they can change or disappear independently of this PR; GitHub's own attachment upload keeps them fixed.
Scope
This is really three changes: resolve a model when there is no default, stop redacting import errors and give them typed reasons, and reclassify batch failures. Only the first fixes #4226, and it can merge alone: it needs no new protocol code, so no epoch at all, and without the typed results the batch path keeps its existing behavior, so there is nothing to reclassify. That is roughly seventy production lines plus the two coordinator tests that fail without them, against +718/-53 across fifteen files and three packages.
I am not asking you to split it on principle. I am asking because the epoch is the expensive part: bumping it refuses every older peer at the handshake, permanently, and the file already records contention over the counter. Spending that on a better error banner, inside a PR whose title is about a missing default model, is what makes the size disproportionate. The diagnosability half stands on its own merits and would be an easy review as its own PR, and it would not be holding the actual bug fix behind an epoch conflict.
There is also a question the PR has not answered. The codebase already has a designed policy for this state: missing_default_connection in connection-readiness.ts:63, with copy in three places telling the user to go configure a default. That policy says the state is legitimate and the remedy is to prompt. This PR introduces a second, divergent policy for the same state, picking a connection the user never chose. I think leniency is defensible here, since import is a one-off explicit action with no model picker, but the divergence should be argued in the body rather than left implicit, because the next path that hits a null default will have two precedents to choose from.
Value
The first part must exist. The feature is completely unusable in a state ordinary users reach, and the UI actively misdirects them. There is no case for closing it.
The second part is a real improvement to a real problem, and I would take it, just not attached to this one.
Evidence boundary: read at 5f8b37f0 against main b9748a77; the null-default reachability, the epoch collision, the settings section labels, the candidate enumeration and the six new test fixtures verified from source; no build, no test run, no Electron launch, and I did not reproduce the import failure against a real catalog. The screenshots were not opened, so I cannot say whether they show the current copy. Conflict resolution will change the head, and everything here needs re-checking against it.
AI-assisted review: drafted with Maka. I verified the epoch collision, the settings labels and the null-default reachability against source myself.
简体中文
这个 bug 是真的，而且比 issue 说的更严重。retainedDefaultTarget（packages/storage/src/runtime-policy/connection-catalog-document.ts:926）在它指向的连接被禁用、或它指向的模型被取消勾选时，直接把 defaultTarget 置 null，而它自己的注释写着这里不会挑替代者。ensureBootstrapRuntimePolicy 只在 catalog 为空时设过一次默认。所以 defaultTarget: null 不是「自配置 profile」的边角情况，而是任何用户第一次禁用自己的默认连接之后就会落到的状态，从那以后外部导入对每个对话都失败，而横幅只会让他去检查来源。值得修。
修改落在了对的归属上。#resolveImportModel 只做候选枚举，逐个交给未改动的 #resolveModel，所以 resolveExecutionConnection 和 authorizeConnectionModel 仍是唯一的 readiness 权威。我枚举了它们能返回的全部错误码，跳过清单是完备的：explicit target 下可达的只有 operation_conflict、invalid_request、operation_unavailable 三个，循环跳过的正是「这个候选不可用」的那两个。整个循环只读，所以中途放弃某个候选不留半授权状态；#resolveTarget 又在任何 commit 之前，所以整体失败不会留下半个 session。
整个 diff 里最好的决定是 execution-composition.ts 那一行。把 resolveDefaultCreateTarget 拆出来，让 WorkHub 的自主创建继续 fail-closed，于是 fallback 只作用在有人明确点了导入的地方。那正是「悄悄挑一个模型」真正危险的唯一位置，你自己识别到了，还配了测试钉住。顺带也退掉了一个名不副实的方法名。
合并前
横幅指向一个不存在的设置分区。 importFailedNoModel 写的是「设置 › 服务商」和 Settings › Providers。那一栏是 models，标签是「模型」和 Models；设置导航里根本没有「服务商」或 Providers。conversation-copy.ts:467-478 里现有的十几条同类提示统一写「设置 · 模型」和 Settings · Models，分隔符也是那个。这是本 PR 唯一那句可执行的指引，而本 PR 的全部意义就是让失败变得可执行，结果它把用户指向一个找不到的地方。
epoch 已经被占了。 main 上 107 给了 token_usage anchors，这个分支把 106 抬到 107 并写上自己的说明，冲突就是这一行。rebase 之后应该是 108，而且要在 rebase 的那一刻重新确认而不是直接假定，因为可能有别的 open PR 先占了，文件自己 // 103: 那条说明就记录过这种事。同一次编辑里 protocol.test.ts 新加的 assert.ok(RUNTIME_HOST_COMPATIBILITY_EPOCH > 101) 也要跟着动：merge-base 上 epoch 已经是 106，所以这条断言在完全不 bump 的情况下也会过，抓不到「解冲突时把 epoch 丢了」这种失败，而这恰恰是冲突行最容易导致的失败。旁边那条的写法是 > 99 对应 100。
候选列表是连接乘以模型。 importModelCandidates 为每个启用连接的每个启用模型各出一个候选，而连接级的失败（缺凭据、provider 已退休）对这个连接的所有模型给出同一个结论。一个连接最多可以启用 512 个模型，所以 catalog 顺序靠前的一个不可用连接，要空转最多 512 次 resolveExecutionConnection 才能走到下一个连接，每一次都是一整轮 runtime-policy lane：读 catalog 文档、准备连接材料。那条 lane 和其它所有 runtime-policy 读写共用，所以导入期间连接设置页会排在后面。在枚举阶段用纯函数 authorizeConnectionModel 和 isModelExplicitlyUnsupportedForChat 先筛，每个连接最多出一个候选，#resolveModel 仍是唯一的 readiness 裁判，也正是 :196 那条注释已经承诺的。
正文赖以成立的那句话没有测试。 「Behavior is unchanged when a default is set and ready」是这次改动唯一的精度声明，而把 importModelCandidates 里整个 preferred 块删掉，六个新测试全绿：两个跑在 defaultTarget: null 下，一个的 default 不 ready，两个走 resolveDefaultCreateTarget 根本不经过枚举器。缺的是一个 ready 的 default 排在 catalog 顺序之后、断言它仍然胜出的用例。
可以更小
:283-284 的注释说保留 WorkHub fail-closed 是因为它的「配置默认模型」启动横幅依赖这次抛出。那条横幅来自 session-send-projection.ts:110 的 missing_default_connection readiness verdict，和这里抛不抛没有关系。保留 fail-closed 是对的，写进注释的依据不对，而这正是以后有人想把两条解析合回去时会先读的那条注释。
正文里 batch 那段是反的。改动前 operation_unavailable 是被 rethrow 的，batch 路径本来就记成 failed。是这个 PR 开始返回带 reason 的 {ok: false}，才会把它扫进 maybe-landed 桶并禁用整页的导入按钮。所以那段改动是防止本 PR 自身引入回归的必需部分，不是修既有 bug 的收尾；反过来写会让下一个读者以为它可选。
sawNoModel 和 ImportBatchOutcome.noModel 是同一个事实的两处表示，局部变量可以删，分支里直接写字段即可。
source_unreadable 的覆盖面大于它的文案。协调器把除 not-found 外的所有 pre-commit 抛出都归到它，所以 EACCES、EMFILE 或者适配器自身的 bug，都会告诉用户这个对话可能过大或已损坏。文案用了「可能 / may be」，不至于把人带进死路，但把原因写成例举而不是断言会更真实。
「Review follow-ups」这一节描述的反馈不在这个 PR 上：没有 review、没有评论、没有 inline 串。说明它来自哪里，或者并进 Summary，因为读者无法核实。另外四张截图托管在你自己 fork 的 release tag 下，可以独立于这个 PR 变更或消失；用 GitHub 自己的附件上传能把它们固定住。
范围
这实际上是三件事：没有默认时解析一个模型、停止脱敏导入错误并给出 typed reason、重新分类批量失败。只有第一件在修 #4226，而且它可以单独合：它不需要任何新协议码，所以完全不用动 epoch；没有 typed result，batch 路径保持原样，也就没有什么要重新分类。那大约是七十行生产代码加两个没有修复就会红的协调器测试，对比现在的 +718/-53、十五个文件、三个包。
我不是为了原则而要求拆。我在意的是 epoch 是这里最贵的东西：抬一次就在握手层永久拒绝所有更旧的 peer，而这个文件本身已经记录过这个计数器的争用。把它花在一个更好的错误横幅上，还是在一个标题讲「缺少默认模型」的 PR 里，这才是体量不成比例的地方。可诊断性那一半自身立得住，单独成 PR 也很好评审，而且不会把真正的 bug 修复卡在一个 epoch 冲突后面。
还有一个 PR 没有回答的问题。仓库对这个状态已经有一套设计好的策略：connection-readiness.ts:63 的 missing_default_connection，配套文案在三处告诉用户去配一个默认。那套策略认定这个状态是合法的，补救办法是提示。这个 PR 为同一个状态引入了第二套、且方向相反的策略：替用户挑一个他从没选过的连接。我认为在导入这条路上宽松一点是讲得通的，它是一次性的显式动作，也没有模型选择器，但这个分歧应该写进正文而不是留成隐含，因为下一条撞到空默认的路径会有两个先例可以挑。
价值
第一部分必须存在。功能在普通用户能到达的状态下完全不可用，而 UI 还在主动误导他们。没有关掉它的理由。
第二部分是对一个真实问题的真实改进，我会要，只是不要挂在这一个上。
|
Severities for the review above, which I should have included with it. P2 — the new banner points at a settings section that does not exist. Normal user path, and it is the one actionable instruction this PR adds: 设置 › 服务商 and Settings › Providers, where the section is P2 — the candidate list multiplies connections by models. Normal path: one candidate per enabled model per enabled connection, while a connection-level failure returns the same verdict for all of that connection's models, and a connection may enable up to 512. An unusable connection early in catalog order costs up to 512 P2 — the summary's precision claim has no test. Deleting the whole P3 — the epoch ratchet test does not ratchet. Reasonable recovery path, and this PR now requires a rebase: P3 — the comment at P3 — P3 — P3 — the summary's account of the batch change is backwards. Before this PR No finding on the owner (model resolution already lives in this coordinator), on the reuse of No P0 and no P1. Merge conditions rather than findings: the rebase, and re-deriving the epoch to 108 at the moment you rebase, since main took 107 for 简体中文上面那份 review 的分级，应该跟着一起给的。 P2 —— 新横幅指向一个不存在的设置分区。 正常用户路径，而且这是本 PR 新增的唯一一句可执行指引：写的是「设置 › 服务商」和 Settings › Providers，而那一栏是 P2 —— 候选列表是连接乘以模型。 正常路径：每个启用连接的每个启用模型各出一个候选，而连接级失败对这个连接的所有模型给出同一结论，一个连接最多可启用 512 个模型。catalog 顺序靠前的不可用连接要空转最多 512 次 P2 —— 正文的精度声明没有测试。 把 P3 —— epoch 的 ratchet 测试并不 ratchet。 合理的恢复路径，而这个 PR 现在正需要 rebase： P3 —— P3 —— P3 —— P3 —— 正文对 batch 改动的叙述是反的。 这个 PR 之前 无发现：归属（模型解析本来就在这个 coordinator 里）、对 没有 P0、没有 P1。属于合并条件而不是 finding 的：rebase，以及在 rebase 的那一刻把 epoch 重推成 108（main 已经把 107 给了 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Putting the local ones inline, each with its grade, so it is clear which are worth fixing before merge and which can wait. The reasoning behind each is in my review above.
…ult model
External-session import resolved its task's model with a hardcoded
`{ kind: 'default' }` target, so it threw "No default Session model is
configured" whenever the workspace had no default set — a legitimate state for
a self-configured profile with otherwise-usable connections. The throw happened
before the source was read, so every import failed, surfaced as the generic
"该对话无法转换或保存" banner.
Split model resolution by caller intent:
- `resolveExternalSessionImportTarget` (import, an explicit user action) prefers
the default but falls back to the first ready connection+model.
- `resolveDefaultCreateTarget` (autonomous WorkHub/scheduled/root create) keeps
the strict, fail-closed default — there is no user in the loop to pick one, and
WorkHub's "配置默认模型" startup banner depends on that throw. This is the path
the WorkHub coordination E2E exercises.
Give import failures stable, code-based classification: publish dedicated
`model_unavailable` and `source_unreadable` operation error codes (compatibility
epoch 109 -> 110), so Desktop Main maps them to typed IPC reasons by code rather
than by the redacted error message. An unsupported adapter (`invalid_request`)
is no longer mislabeled as a too-large/corrupt source, and the oversized-rollout
(> 64 MB) failure gets an honest "too large" banner.
Two follow-ups from review:
- Batch import classifies `no_model` / `source_unreadable` as definite failures
(counted as failed, no recovery offered) instead of folding them into the
maybe-landed "unconfirmed" bucket; a batch blocked for lack of a model raises
the actionable banner once for the whole run.
- The import model fallback only skips genuinely unusable candidates
(`invalid_request` / `operation_unavailable`); a mid-selection identity race
(`operation_conflict`, e.g. a connection deleted or renamed after the snapshot)
now surfaces instead of silently choosing another connection or reporting no
usable model.
Refs apache#4226
Generated-by: Claude (claude-opus-4.8)
5f8b37f to
966fcbb
Compare
|
Thanks for the exceptionally thorough review. The null-default reachability point — that All four before-merge items are fixed on the current head (
Smaller ones are done too: corrected the WorkHub comment (the 配置默认模型 banner is On the divergent policy vs On scope: your reasoning about the epoch is fair — it is a permanent handshake cost, and this PR spends it partly on the error banner. I've kept it unified for now because the typed reasons and the batch reclassification are coupled (the reasons are exactly what make the batch path need reclassifying, so splitting them would land a self-introduced regression in between). But if you'd rather see the null-default resolution merge on its own as the standalone #4226 fix (no epoch) with the diagnosability + batch work as a follow-up, I'm happy to reshape it that way — just say the word. Re-verified locally (built the dependency chain, ran the affected suites) and CI |
2 participants
Summary
External-session import (Settings → 活动 → 导入任务) failed for every conversation with the generic banner "该对话无法转换或保存", regardless of size or source.
Root cause:
resolveExternalSessionImportTargetresolved the imported task's model with a hardcoded{ kind: 'default' }target, so it threw "No default Session model is configured" whenever the workspace had no default set. That is not a rare edge case:retainedDefaultTargetdropsdefaultTargettonullthe moment a user disables the connection it names or unchecks its model, and nothing picks a replacement — so any user who disables their default lands there, and from then on every import fails with a banner telling them to check the source. The throw happened before the source was read; at the IPC boundary it became aRuntimeHostOperationErrorthe renderer redacted down to the generic fallback banner.This resolves the import model by preferring the configured default but falling back to the first ready connection+model in the catalog, reusing the existing explicit-target readiness path (
resolveExecutionConnection+authorizeConnectionModel) so disabled / retired / credential-less / non-chat candidates are skipped. Behavior is unchanged when a default is set and ready.Why fall back rather than prompt. The codebase already has a policy for
defaultTarget: null—missing_default_connectionreadiness, which prompts the user to go configure a default. Import deliberately diverges: it is a one-off, explicit user action on a specific conversation, with no model picker in its flow, so prompting-and-bouncing the user to settings mid-import is worse than quietly attaching a ready model. The divergence is confined to this one explicit path — autonomous creation (WorkHub coordination, scheduled, root) keeps the strict fail-closed default via a separateresolveDefaultCreateTarget, so nothing binds a model the user never chose without a user in the loop.It also makes import errors actionable: because Electron strips the error
codebefore the renderer sees it, Desktop Main maps the intact Host operation code to typed IPC reasons (no_model,source_unreadable) so the page shows a specific banner instead of the redacted fallback. This also gives the oversized-rollout (> 64 MB) failure an honest "too large" message.Because the typed results introduce
{ ok: false, reason }where the page previously saw only thrown errors orcommit_outcome_unknown, the batch path is updated in the same change to keep classifying them correctly:no_model/source_unreadableare counted as definite failures (no recovery offered, per-row import buttons stay live), and onlycommit_outcome_unknownstill enters the maybe-landed recovery bucket. Without this, the new reasons would have swept into that bucket and disabled the page's import buttons — a regression this PR would otherwise introduce, not a pre-existing bug.Fixes #4226
Addressing review
Changes made in response to @Astro-Han's review:
importFailedNoModelnow says 设置 · 模型 / Settings · Models (the section ismodels; there is no 服务商 / Providers entry), matching the dozen existing remedy strings inconversation-copy.ts.importModelCandidatesnow emits the connection's first chat-capable enabled model (pureauthorizeConnectionModel+!isModelExplicitlyUnsupportedForChat), not every enabled model. An unusable connection early in catalog order now costs oneresolveExecutionConnectionround trip on the shared runtime-policy lane instead of up to 512.#resolveModelstays the sole arbiter of connection-level readiness.protocol.test.tsthreshold moved to> 109so it actually detects a lost epoch.source_unreadablecopy widened to "too large, malformed, or temporarily unreadable" — the coordinator maps every pre-commit throw except not-found into it.sawNoModellocal (the summary field is the single source of truth).Verification
Built the dependency chain and ran the affected suites locally (Node test runner), plus Biome and the renderer-architecture snapshot +
--baseratchet:session-catalog-coordinator.test.ts— 45/45, including: fallback when no default is set; a ready default not first in catalog order wins; skip an unready default; clean failure when nothing is usable; surface a mid-selection identity race (operation_conflict) instead of masking it. The fallback/identity-race cases fail without the fix.protocol.test.ts— epoch cases pass at 110.import-tasks-settings-page.test.ts— 34/34, including the code→reason mapping, both single-import banners, and the batch path countingno_model/source_unreadableas failed (not unconfirmed) and raising the model banner.testjob): lint, format, build, typecheck, knip, and the affected suites.User-visible change: the import error banner. zh
importFailedNoModel= "没有可用的模型连接，无法为导入的任务选择模型。请先在 设置 · 模型 中配置并启用一个模型后再导入。";importFailedSourceUnreadable= "无法读取或转换该对话，它可能过大、已损坏或暂时无法读取。请检查来源后重试。" (English equivalents alongside). No layout change; both banners are asserted by the page test.Root cause
See #4226 for the full trace. The redacted banner (
generalizedErrorMessageChinese→importFailedFallback) hid the real error, so the failure was undiagnosable from the UI — the typed-reason mapping above is the durable fix for that.AI use
Select exactly one:
Tool(s) and scope: Claude Code (claude-opus-4.8) diagnosed the root cause against real local data, implemented the fix and tests, and drafted this PR. The commit carries a
Generated-bytrailer.Checklist
Does this PR entail a change in behavior?