refactor: retire 'fake' from the live backend surface - #3249
Conversation
📝 WalkthroughProblem solvedThis PR retires
Design and scopeThe PR extends the existing backend type and decoding rules. It does not create a parallel live-backend path. The change is the smallest coherent solution shown by the diff. It separates live protocol values from persisted compatibility values without requiring data migration. The remaining fake-session handling supports reads, search, stale-session processing, and activation refusal. The deleted ValidationAdded or updated coverage verifies:
Required check results are unverified from the supplied evidence. Complexity deltaThe PR removes:
The PR adds:
The PR reduces live backend states and public protocol surface. It adds one explicit persisted-data state and validation boundary. The added compatibility complexity is necessary to avoid data migration and preserve existing records. Total maintenance complexity decreases, based on the removal of obsolete code and fake-specific product branches. Review-relevant risks
The person performing the merge must review the final diff. A maintainer makes the final determination. WalkthroughThe change retires FakeBackend as a live backend, preserves legacy ChangesBackend compatibility and session readiness
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR retires the live 'fake' backend, but the current implementation still permits new sessions and scheduled tasks to be created with the retired value and may incorrectly recognize reserved provider names such as 'proto'. This can produce unusable records or incorrect model/connection behavior, so those bounded correctness issues should be fixed before merge. Sequence Diagram(s)sequenceDiagram
participant AppShell
participant pendingSessionView
participant NewChatModel
participant SessionSummary
AppShell->>NewChatModel: select model and connection
AppShell->>pendingSessionView: pass session metadata and fallback connection
pendingSessionView->>SessionSummary: create transient active session summary
SessionSummary-->>AppShell: return view data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoRetire fake from live backend types while preserving persisted data
AI Description
Diagram
High-Level Assessment
Files changed (30)
|
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 975a4dd6-1095-44b6-b539-813433b64b02
📒 Files selected for processing (32)
apps/desktop/src/main/__tests__/chat-readiness.test.tsapps/desktop/src/main/__tests__/pending-session-view.test.tsapps/desktop/src/main/chat-readiness.tsapps/desktop/src/main/main-window.tsapps/desktop/src/main/search/thread-search.tsapps/desktop/src/renderer/app-shell-session-start-actions.tsapps/desktop/src/renderer/app-shell.tsxapps/desktop/src/renderer/model-catalog-choices.tsapps/desktop/src/renderer/pending-session-view.tsapps/desktop/src/renderer/settings/provider-connection-detail.tsxpackages/core/src/__tests__/llm-connections.test.tspackages/core/src/agent-run.tspackages/core/src/backend-types.tspackages/core/src/chat-model-choice.tspackages/core/src/connection-readiness.tspackages/core/src/llm-connections.tspackages/core/src/model-catalog.tspackages/core/src/provider-auth.tspackages/core/src/runtime-inputs.tspackages/core/src/scheduled-task.tspackages/core/src/session-send-projection.tspackages/core/src/session.tspackages/core/src/workspace.tspackages/runtime-host/src/protocol/scheduled-task.tspackages/runtime-host/src/protocol/session-catalog.tspackages/runtime/src/model-fetcher.tspackages/runtime/src/model-runtime.tspackages/runtime/src/session-manager.tspackages/runtime/src/test-connection.tspackages/runtime/src/test-only/fake-backend.tspackages/storage/src/__tests__/session-store.test.tspackages/storage/src/session-store.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/main/tests/chat-readiness.test.ts
- apps/desktop/src/main/chat-readiness.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 1 remains after this review.
Code Review by Qodo
1. Automation mutations accept fake
|
|
Qodo: all three verified, all three acted on. 1. Automation mutations accept 2. New sessions accept 3. Loading sessions show wrong model — you were right, and it reversed my previous commit. I had replaced Separately, tracing why the placeholder mattered surfaced something neither review flagged: |
0e37426 to
3d5cb0c
Compare
`BackendKind` was doing two jobs: naming the backend a build may select, and typing the backend value read back from durable state. The in-process FakeBackend stopped shipping in #3226, so the first job narrows to `'ai-sdk'`; the second becomes `PersistedBackendKind`, which still admits `'fake'` and now types the session header, the session-catalog wire projection, the run header, the Automation template, the workspace defaults, and the backend registry that dispatches off them. No data migration. Sessions, runs and Automations written by builds that shipped FakeBackend keep `'fake'` on disk: narrowing the decode guards would make those rows read back as malformed, and rewriting them to `'ai-sdk'` would make an unrunnable task look runnable, since their `llmConnectionSlug` still points at nothing. Activation already refuses them with the product's `fake_backend` reason. `backendKindOf` no longer answers `'fake'` for an unrecognized `providerType` — it throws. That fallback was the last live producer of the value, and there is no honest backend to name for a provider this build cannot describe; the non-throwing question ("can this connection be used?") is `isRealConnection` / `isConnectionReady`. With no provider declaring `'fake'`, the former `isFakeBackend` collapses to "is this providerType one the build knows", so it is named for what it tests. The send projection's dead `slug === 'fake'` branches go with it: a session carrying that slug is refused one line earlier by its backend, and a connection carrying it is refused by the readiness gate. Also deletes `apps/desktop/src/main/chat-readiness.ts`. Its send gate — `requireReadyConnection`, `assertSessionCanSend`, `ensureSessionCanSendOrRebind`, and a third copy of the Chinese connection error copy — had no caller left once the gate moved to Runtime Host; the only live import was the one-line `errorMessage` helper, now inlined at its single use site. Part of #3211 Generated-by: Claude Code
The placeholder `SessionSummary` the chat view shows between "a session id became active" and "its real summary arrived" was hardcoded to `backend: 'fake'` / `model: 'fake-model'` / `llmConnectionSlug: 'default'`, directly above a comment claiming it matched the configured default. It did not. The composer reads those fields straight off this object — the model switcher's current value and the quote companion's inherited-model line — so the switcher matched no offered choice and the companion named a model no session ever had. The `'fake'` there was also the last live write of the retired backend value in product code, standing in for "not loaded yet". The placeholder now carries what the next new task would start on, falling back to the default connection slug and then to no model, which is what the comment always claimed. Extracted to `pending-session-view.ts` so the shape is testable outside the shell component. Closes #3211 Generated-by: Claude Code
`PROVIDER_DEFAULTS` is an object literal, so `PROVIDER_DEFAULTS[providerType]` resolves inherited members: `'__proto__'`, `'toString'` and `'constructor'` all read back truthy and pass as registered providers. Every recognition site was doing that lookup itself, and two of them had been holding the leak closed by accident. `backendKindOf` ended `?? 'fake'`, and the model-choice gates tested `backendKind !== 'ai-sdk'` — both fell through to "not usable" for an inherited member. Narrowing those in the previous commit removed the accident without replacing it: `backendKindOf` began returning `undefined` typed as `BackendKind`, and an inherited member started reading as a model-consumer connection. Calling that check tautological was wrong; it was tautological in the type system only. `providerDefaultsOf` now owns the question and answers it with `Object.hasOwn`. The connection-catalog codec's `PROVIDER_TYPES` set — until now the only site that got this right, via `Object.keys` — folds into it, so provider recognition has one implementation rather than a correct one and several approximations. Part of #3211 Generated-by: Claude Code
`shouldRebindSessionToDefault` listed `fake_backend`, so an unlocked session on
the retired backend projected as `{kind:'rebind'}`. Nothing performs that
rebind — the only executor was `ensureSessionCanSendOrRebind`, which had no
caller and is now deleted, and neither Runtime nor Runtime Host has an
equivalent. The two consumers of `'rebind'` only use it to suppress a notice.
It could not have worked anyway. Every other reason on that list names a broken
*connection*, which another connection can stand in for. A retired backend is
not: activation dispatches off the session header's own `backend`, so pointing
the session at a healthy connection still leaves `'fake'` in the header and
still gets refused.
The cost was paid elsewhere. Because the projection said "recoverable" for rows
that are not, the surfaces that must answer "is this task usable?" bypassed it
and read `session.backend` themselves — the rail's stale marker and the
composer's connection/model labels. Removing `fake_backend` from the list makes
the projection answer `blocked` for these rows, and both surfaces drop their
workaround and read the reason like any other. The
`'任务已过期 · 请先配置真实模型'` notice, written long ago and suppressed for
unlocked rows ever since, now shows.
Part of #3211
Generated-by: Claude Code
…t know The previous commit replaced the placeholder's `model: 'fake-model'` with the connection and model the next new task would start on. That was wrong in a way the literal was not. This fallback covers every active id whose summary has not arrived, not only a freshly created task, so the session behind it is usually an existing one bound to some other model. Naming the new-chat default made the composer assert that configuration as the session's own: the model switcher shows it as current, and its no-op guard compares against it — so a user switching onto that very model has the change silently dropped against a session that was never on it. `'fake-model'` avoided this by accident: it matched no offered choice, which is exactly how the switcher spells "not known yet". The placeholder keeps that property and states it deliberately, with an empty connection/model pair, and without borrowing a retired backend name to mean "not loaded". Withholding in-session model selection entirely until the summary lands is the better answer, but it belongs to the switcher's own loading contract in `@maka/ui`, not to this cleanup. Part of #3211 Generated-by: Claude Code
…ivation Two validators accepted `'fake'` under a comment claiming they were decoders keeping frozen Automations readable. Neither is a decoder. `normalizeExecution` in core is reached only from `normalizeCreateScheduledTaskInput` / `normalizeUpdateScheduledTaskInput`; stored Automations are read back with `JSON.parse` in `scheduled-task-store.ts` and never pass through it. It now refuses `'fake'` outright. The protocol's `decodeExecution` genuinely serves both directions — reading a stored task and validating an inbound create/update — but those carry different backend invariants, so one shared answer had to be wrong for one of them. It takes the direction as an argument: `'stored'` still accepts the retired value, `'mutation'` does not. Without this a client could write a brand new Automation guaranteed to fail later at activation. The storage regression test seeded its legacy row through `store.create`, which is the write path this is closing. It now writes the row underneath the store, which is also the only way such a row was ever produced — and the test fixture stops defaulting every session in the file to the retired backend, so narrowing the header guard fails one named test instead of all seventeen. Part of #3211 Generated-by: Claude Code
3d5cb0c to
0d7048c
Compare
|
@CxHsin @me2seeks — no obligation, but I'd value your eyes on this one if you have time, since it lands right on top of work you each own. @CxHsin: #3096 made @me2seeks: two things touch your ground. Context is in the PR body and the per-commit messages. Reviews from committers are already requested, so treat this purely as an invitation, not a queue. 中文版@CxHsin @me2seeks —— 没有任何义务，但这个 PR 正好压在你们各自的地盘上，有空的话我很希望听听你们的意见。 @CxHsin：#3096 把 @me2seeks：有两处落在你熟悉的地方。 背景在 PR 正文和各个 commit message 里。committer 的 review 已经另外请了，所以这条纯粹是邀请，不是排队。 |
|
Both claims check out, with one correction to the framing. T1 gate: safe to remove, but not because "activation refuses those headers before the kernel sees them." Placeholder: endorse. The placeholder's backend/connection/model fields have exactly one consumer — the switcher's current-value and no-op comparison. One housekeeping note: your reply to Qodo's finding #2 says "declined, deferred" — but 中文版两条判断都成立，但第一条的论证措辞需要修正。 T1 gate： 可以删，但理由不是「activation 在 kernel 见到这些 header 之前就拒绝了它们」。 占位符： 认可。占位符的 backend/connection/model 字段只有一个消费者——切换器的 current-value 和 no-op 比较。 一件收尾的事：你对 Qodo 第二条 finding 的回复说「declined, deferred」——但 |
M4n5ter
left a comment
There was a problem hiding this comment.
English
LGTM — I found no blocking correctness or design issues.
Two non-blocking follow-ups may be worth considering separately:
-
SessionHeaderPatchstill inheritsbackendfromSessionHeader, so trusted internal code usingSessionStore.updateHeader()can technically write the retiredfakevalue. Current product protocols expose closed patch shapes and no existing caller updates this field, so this is not reachable through a supported user flow. A future cleanup could makebackendimmutable in the patch type and reject it at the storage writer boundary. -
ScheduledTaskExecutionTemplate.backendremains durable state, but the fire path does not use it when creating the execution session. This behavior predates this PR, and normal product flows did not createfakeAutomations, so it should not block this change. Longer term, the field should either be enforced at fire time—rejecting retired legacy values—or removed from new canonical writes while remaining tolerated by the legacy decoder.
The PR otherwise achieves its goal: fake is removed from the live backend surface, new retired-backend writes are closed through supported paths, and legacy durable records remain readable and fail activation through the existing product reason.
中文
LGTM——没有发现需要阻塞合并的正确性或设计问题。
有两个非阻塞事项可以后续单独处理：
-
SessionHeaderPatch仍从SessionHeader继承了backend，因此受信任的内部代码理论上可以通过SessionStore.updateHeader()写入已经退役的fake。目前产品协议使用封闭的 patch 结构，也没有现有调用方更新该字段，因此正常用户路径无法触发。后续可以在 patch 类型中将backend设为不可变，并在 storage writer 边界拒绝该字段。 -
ScheduledTaskExecutionTemplate.backend仍会被持久化，但任务触发路径在创建执行 Session 时并不读取它。该行为早于本 PR，正常产品流程也不会创建fakeAutomation，因此不应阻塞本次修改。长期来看，应当选择一种明确语义：在触发时校验该字段并拒绝退役值，或者停止在新记录中写入它，仅由 legacy decoder 容忍历史字段。
除此之外，本 PR 已实现其核心目标：从 live backend surface 移除 fake，关闭受支持路径中的新退役值写入，同时保持历史持久化记录可读取，并通过现有产品原因拒绝激活。
|
Thanks — both verified, both left out of this PR.
中文两条都查证了，都不进本 PR。
🤖 Drafted with Claude Code |
EnglishI think this direction is sound. In the current live build, The write model and persisted model should remain distinct, however. Existing data may still contain In short, I support removing caller-facing 中文我认为这个方向是合理的。当前线上构建中， 不过，写入模型和持久化模型仍应保持区分。旧数据中可能仍然包含 简而言之，我支持删除面向调用方的 |
`CreateSessionInput.backend` was the last live path that could write the
retired `'fake'` into a new session. Narrowing it to `BackendKind` would only
move the question: four derivation sites in `session-manager.ts` plus the
conversation copy in `session-revision-coordinator.ts` inherit the source
header's backend, so each would need an answer for what deriving from a legacy
row means.
Deleting the field dissolves that question. A live build has exactly one
backend, so the field carried no choice — the store stamps `'ai-sdk'` on every
new header and no caller names a backend at all. A session derived from a
legacy row is now a real session whose connection slug resolves to nothing,
which is what the readiness projection already says about that row.
Two backend-kind guards go with it, for separate reasons:
- `runtimeToolBoundaryProtocol` withheld the T1 durable tool boundary from
non-`ai-sdk` headers. The branch was reachable: nothing checks the backend
kind between `sendMessage` and the kernel, and `AgentRun.begin()` persists the
run record, user message, turn state and the `connectionLocked` flip before
the refusal fires inside `reserveRun`. What the gate withheld was only the
protocol marker on the initial event of a run that dies before tool dispatch,
and every marker consumer treats a marked run with zero tool operations the
same as an unmarked one. The marker stays truthful; the gate bought nothing.
- `sessions:create` rejected a non-`ai-sdk` backend arriving over IPC. That
one really is unrepresentable now: the field is gone from the request type.
Tests that dispatched through a `'fake'` registry key now register the live
one, which is how they would be written today. The activation-refusal
regression test seeded its legacy row through `createSession({backend:
'fake'})` — the very write this removes — and now seeds it under the writer,
which is the only way such a row was ever produced.
Generated-by: Claude Code
0d7048c to
6295359
Compare
|
@me2seeks — correction accepted, and verified: On Qodo #2: already reversed. @CxHsin — that is exactly the split shipped: 中文@me2seeks —— 纠正接受，我也核对过： 关于 Qodo 第二条：其实已经反转过了。那个线程里的 @CxHsin —— 这正是本 PR 采取的切分： 🤖 Drafted with Claude Code |
…tests apache#3249 removed `backend` from `CreateSessionInput`, so the four Session fixtures this branch adds no longer compile against `main`. The field carried no choice for a live build, and the store stamps every new header itself, so the fixtures need nothing in its place. Generated-by: Claude Code
Summary
Second half of #3211. #3226 stopped shipping FakeBackend; this retires
'fake'from the live surface.The value was carrying three jobs: a selector for a runnable backend, a literal in durable records, and a sentinel meaning "not real / not known". #3226 killed the first. The second is permanent. The third is what this removes — so
'fake'now appears only in decode guards and in the one function translating it to thefake_backendproduct reason. Everything else reads that reason.BackendKindnarrows to'ai-sdk';PersistedBackendKind = BackendKind | 'fake'types everything durable (session header/summary, catalog wire projection, run header, Automation template, workspace defaults, and the registry dispatching off them).CreateSessionInput.backendis gone rather than narrowed: a live build has exactly one backend, so the field carried no choice — only the chance of writing the retired value. The store stamps every new header. Two backend-kind guards go with it: thesessions:createIPC check, now unrepresentable, and the T1 tool-boundary gate onheader.backend— that one was reachable, but all it withheld was the protocol marker on the initial event of a run that dies atreserveRunbefore any tool dispatch, and every marker consumer treats a marked run with zero tool operations the same as an unmarked one (thanks @me2seeks for correcting my original framing).'fake'on disk. Narrowing the decode guards makes them read back as malformed; rewriting them to'ai-sdk'makes an unrunnable task look runnable, sincellmConnectionSlugstill points at nothing. Activation refuses them with the product reason, as of fix(release): stop shipping FakeBackend and desktop E2E material in production artifacts #3226.backendKindOfnow throws for an unrecognizedproviderTypeinstead of answering'fake'. This changes@maka/core's public contract; it has no caller in this repo.Per-commit messages carry the reasoning for each change.
Closes #3211
Behavior change
shouldRebindSessionToDefaultlistedfake_backend, but nothing performs that rebind — the only executor was in the deleted module, and activation dispatches off the header's ownbackendanyway, so no connection swap could have helped. That false promise is why the rail and composer bypassed the projection and readsession.backenddirectly.Removing it makes the projection answer
blocked, both surfaces drop their workaround, and the existing'任务已过期 · 请先配置真实模型'notice — suppressed for unlocked rows until now — appears. That is the one user-visible change beyond the type work.A session derived from a legacy row (branch, revision, subagent, conversation copy) no longer inherits
'fake'. It is a real session whose connection slug resolves to nothing, so the projection reportsconnection_missing— which is what that row actually is.Verification
npm run format/npm run lintclean; typecheck clean across all affected workspaces.Object.hasOwnfrom provider recognition fails the inherited-member test.New tests pin each decision: a retired backend never rebinds even when unlocked with a ready connection available; inherited object members (
__proto__,toString) are not providers; an Automation cannot be created on the retired backend while a stored one still decodes; the pending chat view matches no offered model choice.Two legacy-row tests — the storage decode test and the activation-refusal regression — used to seed
'fake'through the writer, demonstrating the write this PR removes. Both now seed under the writer, which is the only way such a row was ever produced.Not run locally, left to CI: Playwright E2E and the repository-wide run.
AI use
Tool(s) and scope: Claude Code (Opus 5) wrote the changes and this description. CodeRabbit and Qodo reviewed; findings were verified independently before acting. AI review is not independent human review.
Generated-bytrailers are on every commit and must survive squash.Checklist
Does this PR entail a change in behavior?