feat(runtime): retire the Claude subscription OAuth provider - #3183
Conversation
|
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:
📝 WalkthroughWhat problem this solvesThis PR retires the The provider remains registered as Source of truthThe PR extends the existing provider registry, readiness, authentication-contract, onboarding, storage-policy, runtime, and UI paths. It does not create a parallel retirement path. The provider registry is the source of truth for retirement. Runtime and UI behavior derive from that state. Scope and necessityThis is the smallest coherent solution for provider retirement:
The removed settings component, usage module, bridge methods, protocol operations, provider-specific helpers, fixtures, and stale export cannot be removed further without weakening compatibility or regression coverage. The Complexity deltaThe PR removes:
The PR adds:
Total maintenance complexity decreases. The added retirement state is necessary to preserve stored-connection decoding while preventing use. Validation and risksTests cover readiness rejection, provider contracts, onboarding, connection status, OAuth protocol behavior, storage policy, session admission, runtime rejection without network requests, and supported-provider OAuth flows. The author reports passing build, typecheck, lint, formatting, and other listed checks. Environment-related failures match clean Concrete risks include:
Review-relevant risksThe current diff affects user-visible settings and onboarding, public bridge and protocol contracts, runtime compatibility, authentication behavior, and stored-connection handling. Material changes in these areas require independent human review under repository policy. The follow-up for The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughClaude subscription OAuth support is retired across core, runtime, runtime-host, storage, and desktop. Existing connections remain decodable but cannot authenticate, execute models, refresh quota, or initiate OAuth login. The UI directs users to replacement connections. ChangesClaude subscription retirement
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to The change retires Claude subscription connections, but a retained retired connection can still be selected as the default and then cause new sessions to fail instead of running. Merge should wait for default-target validation to reject retired providers and for the corresponding mutation-path test. Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoRetire Claude subscription OAuth across runtime and desktop
AI Description
Diagram
High-Level Assessment
Files changed (65)
|
16f779f to
16c9576
Compare
Code Review by Qodo
1.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/runtime-host/src/server/oauth-coordinator.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete the unused authorization-code exchange dependency.
Disposition: optional.
After Line 333,
#runLogincalls only#runXaiLoginor#runCodexDeviceLogin. Neither method calls#exchangeCode. DeleteexchangeOAuthAuthorizationCode,HostOAuthCoordinatorInput.exchangeCode, and the private#exchangeCodefield.As per path instructions, “Flag concrete cases where code can be deleted or simplified.”
Also applies to: 333-333
Source: Path instructions
packages/runtime-host/src/__tests__/oauth-execution-authority.test.ts (1)
439-439: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated union member.
'openai-codex' | 'openai-codex'is equivalent to one literal type. DeclareproviderTypeas'openai-codex'.Disposition: optional.
Proposed simplification
- providerType: 'openai-codex' | 'openai-codex', + providerType: 'openai-codex',As per path instructions, “Flag concrete cases where code can be deleted or simplified.”
Source: Path instructions
packages/runtime/src/__tests__/oauth-login.test.ts (1)
104-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete the unused Claude environment entry.
MAKA_CLAUDE_SUBSCRIPTION_EXPERIMENTALaffects neither assertion. The test only evaluatesopenai-codexandxai-oauth, and xAI ignores environment flags. Remove this entry to keep the test limited to active enrollment behavior. Disposition: optional.As per path instructions, delete an unnecessary path before adding behavior.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 000f121d-fee4-4262-991f-407da136b587
📒 Files selected for processing (71)
apps/desktop/src/main/__tests__/chat-readiness.test.tsapps/desktop/src/main/__tests__/runtime-host-account-connection.test.tsapps/desktop/src/main/__tests__/runtime-host-oauth-ipc-main.test.tsapps/desktop/src/main/chat-readiness.tsapps/desktop/src/main/oauth-connection-identities.tsapps/desktop/src/main/runtime-host-client.tsapps/desktop/src/main/runtime-host-oauth-ipc-main.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/locales/conversation-copy.tsapps/desktop/src/renderer/locales/onboarding-copy.tsapps/desktop/src/renderer/locales/settings-provider-copy.tsapps/desktop/src/renderer/onboarding-hero-copy.tsapps/desktop/src/renderer/settings/claude-subscription-card.tsxapps/desktop/src/renderer/settings/provider-connection-detail.tsxapps/desktop/src/renderer/settings/provider-oauth-section.tsxapps/desktop/src/renderer/settings/providers-panel.tsxapps/desktop/src/renderer/settings/use-connection-detail.tsapps/desktop/stories/settings/provider-settings.stories.tsxapps/desktop/stories/settings/settings-pages.stories.tsxdocs/astryx-surface-file-inventory.mddocs/astryx-surface-file-inventory.pathspackages/core/src/__tests__/connection-readiness.test.tspackages/core/src/__tests__/oauth-subscription.test.tspackages/core/src/__tests__/onboarding.test.tspackages/core/src/__tests__/provider-auth.test.tspackages/core/src/__tests__/provider-catalog-contract.test.tspackages/core/src/connection-error-copy.tspackages/core/src/connection-readiness.tspackages/core/src/model-catalog.tspackages/core/src/model-web-search.tspackages/core/src/oauth-subscription.tspackages/core/src/onboarding.tspackages/core/src/provider-auth.tspackages/core/src/provider-registry.tspackages/core/src/task-submission-readiness.tspackages/runtime-host/src/__tests__/execution-model-composition.test.tspackages/runtime-host/src/__tests__/oauth-coordinator.test.tspackages/runtime-host/src/__tests__/oauth-execution-authority.test.tspackages/runtime-host/src/__tests__/oauth-protocol.test.tspackages/runtime-host/src/__tests__/oauth-two-client-uds.test.tspackages/runtime-host/src/protocol/index.tspackages/runtime-host/src/protocol/oauth.tspackages/runtime-host/src/protocol/operations.tspackages/runtime-host/src/server/execution-composition.tspackages/runtime-host/src/server/execution-model-authority.tspackages/runtime-host/src/server/execution-model-composition.tspackages/runtime-host/src/server/oauth-coordinator.tspackages/runtime-host/src/server/oauth-execution-authority.tspackages/runtime/src/__tests__/claude-subscription-runtime.test.tspackages/runtime/src/__tests__/claude-subscription-usage.test.tspackages/runtime/src/__tests__/computer-use-model-loop.test.tspackages/runtime/src/__tests__/model-factory-thinking.test.tspackages/runtime/src/__tests__/oauth-login.test.tspackages/runtime/src/__tests__/provider-contract-matrix.tspackages/runtime/src/__tests__/subscription-credentials.test.tspackages/runtime/src/__tests__/subscription-model-fetch.test.tspackages/runtime/src/claude-subscription-usage.tspackages/runtime/src/model-factory.tspackages/runtime/src/model-fetcher.tspackages/runtime/src/model-runtime.tspackages/runtime/src/oauth-login.tspackages/runtime/src/oauth-provider-contracts.tspackages/runtime/src/subscription-auth.tspackages/runtime/src/subscription-cloaked-request.tspackages/runtime/src/subscription-credentials.tspackages/runtime/src/subscription-model-fetch.tspackages/runtime/src/test-connection.tspackages/storage/src/__tests__/runtime-policy-stores.test.tspackages/storage/src/runtime-policy/coordinator.tspackages/storage/src/runtime-policy/operations.ts
💤 Files with no reviewable changes (21)
- docs/astryx-surface-file-inventory.paths
- apps/desktop/src/main/oauth-connection-identities.ts
- apps/desktop/stories/settings/provider-settings.stories.tsx
- apps/desktop/stories/settings/settings-pages.stories.tsx
- apps/desktop/src/renderer/settings/claude-subscription-card.tsx
- packages/runtime/src/claude-subscription-usage.ts
- packages/runtime/src/tests/computer-use-model-loop.test.ts
- packages/runtime-host/src/protocol/operations.ts
- packages/runtime/src/tests/subscription-model-fetch.test.ts
- packages/runtime-host/src/server/execution-model-composition.ts
- apps/desktop/src/preload/bridge-contract.d.ts
- packages/core/src/model-web-search.ts
- packages/runtime/src/subscription-cloaked-request.ts
- packages/core/src/tests/oauth-subscription.test.ts
- packages/runtime-host/src/server/execution-composition.ts
- packages/runtime-host/src/server/execution-model-authority.ts
- packages/runtime/src/tests/claude-subscription-usage.test.ts
- apps/desktop/src/main/runtime-host-client.ts
- packages/runtime-host/src/server/oauth-execution-authority.ts
- packages/runtime/src/subscription-auth.ts
- packages/runtime/src/subscription-model-fetch.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
16c9576 to
2ddd92d
Compare
|
All three are real. Fixed in the amended commit. 1. Stale 2. Orphaned Claude settings copy. Removed the whole 3. Login branch not future-exhaustive. Agreed — the ternary was the one place in this PR that fails open. Replaced with a On the alternative approach: agreed, and the reason is stronger than compatibility. |
fd810dd to
1d978df
Compare
|
Thanks for the removal — the impersonation is unambiguous and the decision is right: the deleted Conclusion: PASS with one P2 (epoch) — and the UI/UX screenshot gate must be satisfied before merge. P2 — the epoch bump the PR claims ("RUNTIME_HOST_COMPATIBILITY_EPOCH moves to 23") is not in the diff. This PR changes wire protocol (removes P3-1 (merge gate): no before/after screenshots for the UI/UX changes. This PR visibly changes Settings (retirement banner, removed test/set-default buttons, list-row retired state), onboarding ( P3 (optional): one mutation-claim is not independently reproducible — the storage allow-list test ( AI-assisted review disclosure: this review was produced with AI assistance (two independent pi review subagents on 中文摘要（AI 辅助审查）结论：PASS（1 个 P2 + UI/UX 截图 gate 阻塞）。冒充确凿、移除决定正确（subscription-cloaked-request.ts 全套伪造 Claude Code 身份——UA/x-app:cli/会话头/假系统提示/计费标记/借用的 client_id，风险在用户自己的 Anthropic 账户，gating 只是把违规路径藏起来，移除才消除风险载体）。7 个 enforcement point 全部真实且相互独立（注册表真相源→storage 准入→send 准入→picker→OAuth 登录→wire 枚举→runtime 兜底），逐一核实存在且单独 mutation 会红。删除干净（4 文件整删、共享基建正确裁剪、无悬空 import、stale export 已修）。retained-but-retired 的技术理由基本成立（decodeProviderType 对未注册类型 throw、整个 catalog 文档失败，unregister 会让用户失去全部连接）——但有 gemini-cli 先例（按原始 providerType 字符串先过滤）证明完全 unregister 可行，"不可能"略夸大；真实理由（保留可见可删、让用户清理本机凭据、否则 onboarding 退回 needs_connection）是合理产品选择，建议在 PR 中诚实陈述。breaking-change 叙述除 epoch 段外诚实。P2：PR 声称的 epoch bump（22→23）不在 diff 里——protocol/index.ts 未动，base 已是 23（#3145 升的），main 当前 24（#3165 升的）；"旧客户端被 epoch gate 拒绝"是巧合保护——#3145 与本 PR 之间构建的 client（epoch 23、旧协议）会通过精确相等握手（connection.ts:1290）然后中途在 oauth.account.usage.fetch/login 上报 unknown-operation 而非握手期干净拒绝。实际影响低（无已发布 epoch-23 client），但违反仓库"每个 breaking PR 自己 bump"惯例且描述误导。建议本 PR bump 25（与 #3103 协调）并修正描述，或显式声明骑 #3145 的 bump 并 release-atomic。P3-1（合并 gate）：大量 UI/UX 变化（Settings 退休横幅/移除 test 与 set-default 按钮/list 行 retired 状态/onboarding all_connections_retired 卡片/删 494 行卡片）但无任何截图或录屏——按仓库规则 UI/UX 变化合并前必须 before/after 截图。P3（可选）：storage allow-list 的 mutation 声称不可独立复现（contract 门独立产生同结果，防御纵深生效但表格高估该行判别力）；oauth-coordinator.ts 残留死代码（exchangeOAuthAuthorizationCode 导入、#exchangeCode 字段、HostOAuthCoordinatorInput.exchangeCode 永不被调用）；oauth-login.test.ts:106 仍设置已删除的 MAKA_CLAUDE_SUBSCRIPTION_EXPERIMENTAL env flag；新增 retired UI 状态无 Storybook stories、E2E 未跑（仓库惯例 UI 状态要确定性 Storybook/Playwright 覆盖）。 |
There was a problem hiding this comment.
English
Requesting changes.
Correction: the compatibility-epoch issue was already reported in the top-level review comment from Astro-Han, so I removed my duplicate inline comment. That earlier comment also captures the current-main coordination correctly: main already uses epoch 24, so this PR needs the next distinct epoch rather than another bump to 24.
This review retains two independent actionable findings:
- Host session admission can still persist sessions targeting a retired connection when its credential remains stored.
- The authorization-code/paste-code OAuth path has lost its last production producer but remains across protocol, Host, runtime, and Desktop layers.
The Host admission gap independently warrants changes. The authorization-code comment intentionally covers the full producerless demand chain, extending beyond the narrower unused-#exchangeCode observation in the earlier review.
简体中文
建议 Request changes。
更正：compatibility epoch 问题已经由 Astro-Han 的顶层 review comment先行报告，因此我删除了自己的重复 inline comment。该评论对当前 main 的协调也更准确：main 已经使用 epoch 24，所以本 PR 需要下一个独立 epoch，而不是再次提升到 24。
本 review 保留两项独立且可执行的 finding：
- 当旧 credential 仍被保留时，Host session admission 仍能持久化指向退役 connection 的 session。
- authorization-code/paste-code OAuth 路径已经失去最后一个生产 producer，却仍横跨 protocol、Host、runtime 和 Desktop 层存在。
Host admission 缺口本身足以支持 Request changes。authorization-code 评论针对的是完整的无 producer demand chain，范围超过先前 review 中较窄的 unused #exchangeCode 观察。
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
对抗性复核结论：当前 revision 不可合并。移除 Claude subscription OAuth 的方向正确，删除量也总体干净，但执行准入和协议边界还没有闭合。
- P1：breaking protocol 没有分配新的 compatibility epoch。
PR head 的 packages/runtime-host/src/protocol/index.ts:74 仍是 epoch 23；当前 main 已是 24。这个 PR 又删除了 OAuth provider/operation，所以合并结果会继续使用 24，epoch-24 的旧 Client/Host 能通过握手，再在 removed operation/provider 上中途失败。这里需要占用一个新的独立 epoch，并与其它并行 breaking PR 协调，不能依赖别的 PR 的 bump。
- P1：保留的旧 credential 会让 retired connection 通过 Host session admission。
packages/core/src/provider-auth.ts:93-100 对 retired provider 仍返回 requiresSecret: true；packages/storage/src/runtime-policy/coordinator.ts:562-587 只据此准备 credential，存在旧 token 时返回 ready。packages/runtime-host/src/server/session-catalog-coordinator.ts:653-679 信任该结果，所以 default/explicit session.create 或 configuration update 可以持久化指向 retired Claude connection 的 Session，真正执行时才在更深层失败。
请把 retired/unavailable 作为 execution resolver 的一等结果，在 create/update commit 前拒绝，并覆盖 retained credential 下的 default 与 explicit target。
- P2：升级前的 retired Claude default target 没有释放。
packages/storage/src/runtime-policy/connection-catalog-document.ts:127-135 只清理旧 Gemini preview；Claude connection 保留后，其 defaultTarget 也原样保留。session-catalog-coordinator.ts:716-752 会继续选择它；apps/desktop/src/renderer/settings/providers-panel.tsx:266-270 还会显示“默认”，但不给用户切换该连接的操作。即使另有健康连接，新 Session 默认仍落到退役目标。迁移/读取时应释放 retired default，或提供等价的统一修复路径。
- P2：
request_authorization_code/ paste-code OAuth 链路已无生产 producer，却仍横跨 protocol、Host、runtime 和 Desktop 存在。
当前 Codex 与 xAI 都只产生 open_external，但 packages/runtime-host/src/protocol/oauth.ts:53-74、server/oauth-coordinator.ts:69,124,452-476、Desktop pending-code/IPC、client presenter 以及 packages/runtime/src/oauth-login.ts:139 的 authorization-code exchange 仍被保留。应端到端删除这条无需求的敏感鉴权表面；未来有真实 provider 需要时再随 producer 引入。
对应地，apps/desktop/src/main/__tests__/runtime-host-oauth-ipc-main.test.ts:90 把 Codex fixture 伪造成 paste-code 流程，未覆盖真实 open_external 行为，应替换而不是继续维持死接口。
结论：方案目前不最优；需要局部根治，不需要推翻 retained-but-retired registry 设计。可删除完整 paste-code 链路、无调用的 exchange 注入、废弃 Claude env fixture 和重复 union member。CI 全绿，但没有覆盖 retained credential 的 Session admission、retired default migration 和合并后 epoch；这些是 merge blockers。
|
@M4n5ter @hqhq1025 — on the Why split. This PR's claim is "a provider that violates Anthropic's Consumer Terms no longer exists." The paste-code teardown is cleanup that removal enables — it is producerless because of this PR — but it is not needed for that claim to hold, and nothing in the retirement is incorrect while the dead path exists. Verifying them together also means one reviewer has to check both "the provider is gone from seven authorities" and "codex and xai device login still work after an OAuth-presentation teardown," which are unrelated failure modes. It matters for revert granularity too: if the teardown breaks a device flow in a way the tests miss, reverting it should not drag the Consumer Terms fix back with it. This PR is already 73 files; the teardown adds 14 across four packages. I am taking the cost of that honestly: the teardown is another wire change and will need its own epoch, so splitting spends two epochs instead of one. PR B scope — exactly this, nothing else. Removed end to end:
Kept, because the live device flows import them: Also in PR B: Everything else you both raised — the epoch, Host session admission with a retained credential, the retired default target, the override-ordering bypass @coderabbitai found, and the If either of you would rather have the teardown here despite the above, say so and I will fold it in. |
|
Tip For best results, initiate chat on the files or code changes.
The current PR can remain focused on retiring For PR B, remove the wire variants, desktop IPC channel, coordinator exchange path, runtime authorization-code helpers, and path-only tests in the same change. Keep the device-flow helpers that Codex and xAI still import. Please open a tracked follow-up issue for PR B, or link the follow-up PR here when it exists. This will make the remaining protocol cleanup explicit and reviewable. I will review the current-PR fixes after they are pushed. You are interacting with an AI system. |
1d978df to
91c504e
Compare
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: e1ff2d6a-840f-468f-a678-dadabaee4355
📒 Files selected for processing (14)
apps/desktop/src/main/__tests__/runtime-host-oauth-ipc-main.test.tsapps/desktop/src/main/runtime-host-client.tsapps/desktop/src/renderer/settings/providers-panel.tsxpackages/runtime-host/src/__tests__/session-catalog-coordinator.test.tspackages/runtime-host/src/protocol/index.tspackages/runtime-host/src/server/session-catalog-coordinator.tspackages/runtime/src/__tests__/claude-subscription-runtime.test.tspackages/runtime/src/__tests__/oauth-login.test.tspackages/runtime/src/__tests__/provider-contract-matrix.tspackages/runtime/src/model-runtime.tspackages/storage/src/__tests__/runtime-policy-stores.test.tspackages/storage/src/runtime-policy/connection-catalog-document.tspackages/storage/src/runtime-policy/coordinator.tspackages/storage/src/runtime-policy/operations.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/main/runtime-host-client.ts
- packages/runtime/src/tests/claude-subscription-runtime.test.ts
- apps/desktop/src/renderer/settings/providers-panel.tsx
- apps/desktop/src/main/tests/runtime-host-oauth-ipc-main.test.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
|
Pushed. Point by point, and where I was wrong I have said so rather than quietly fixed it. @Astro-Han / @hqhq1025 — P1/P2, the epoch. Fixed: 24 → 25 in @M4n5ter / @hqhq1025 — P1/P2, Host session admission with a retained credential. Fixed, and you were right that my earlier reasoning was too convenient — I had considered guarding
@hqhq1025 — P2, the retired default target. Fixed, and thank you for catching the second half of it, which was mine: I had hidden "set as default" for retired connections but left the branch order such that a connection which already was the default still rendered the 默认 Badge with no control to move it — I removed a dead end and added a new one. Loading the catalog now releases a @coderabbitai — Major, the override-ordering bypass. Fixed: @coderabbitai — the contract matrix and the Codex IPC fixture. Both fixed here. @Astro-Han — P3, the mutation table overstated one row. Correct, and now stated in the description: reverting only the storage interactive-login allow list leaves the suite green, because the auth-contract gate independently produces the same refusal. That is defense in depth working, but the row claimed discriminating power it does not have. @Astro-Han — P3, "impossible to unregister" was overstated. Also corrected in the description. Also in this push: Still deliberately not done here: the Not covered: the new retired UI states have no Storybook stories and I have not run the Playwright E2E suite; both are stated in the description rather than left implied. Before/after screenshots for the UI/UX gate are in the PR body. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files relevant to reported fixes ==='
git diff --name-only origin/main...HEAD | rg \
'(^packages/runtime-host/src/protocol/index\.ts$|^packages/storage/src/runtime-policy/|^packages/runtime/src/model-runtime\.ts$|^packages/runtime/src/__tests__/provider-contract-matrix\.ts$|^apps/desktop/src/main/__tests__/runtime-host-oauth-ipc-main\.test\.ts$|^packages/runtime-host/src/server/session-catalog-coordinator\.ts$|^packages/runtime-host/src/__tests__/session-catalog-coordinator\.test\.ts$|^packages/storage/src/__tests__/runtime-policy-stores\.test\.ts$|^packages/storage/src/runtime-policy/connection-catalog-document\.ts$|^apps/desktop/src/renderer/settings/providers-panel\.tsx$|^packages/runtime/src/__tests__/claude-subscription-runtime\.test\.ts$|^packages/runtime/src/__tests__/oauth-login\.test\.ts$)' || true
echo '=== Targeted outlines ==='
ast-grep outline packages/runtime/src/model-runtime.ts --items all --view expanded
ast-grep outline packages/storage/src/runtime-policy/coordinator.ts --items all --view expanded
ast-grep outline packages/runtime-host/src/server/session-catalog-coordinator.ts --items all --view expanded
echo '=== Epoch and retirement enforcement references ==='
rg -n -C 4 \
'RUNTIME_HOST_COMPATIBILITY_EPOCH|provider_retired|isRetiredProvider|lookupModelProviderOverride|wireDimensionCell|reasoning.*[Rr]eplay|openExternal|requestAuthorizationCode|MAKA_CLAUDE_SUBSCRIPTION_EXPERIMENTAL' \
packages/runtime-host/src/protocol/index.ts \
packages/runtime/src/model-runtime.ts \
packages/runtime/src/__tests__/provider-contract-matrix.ts \
packages/storage/src/runtime-policy \
packages/storage/src/__tests__/runtime-policy-stores.test.ts \
packages/runtime-host/src/server/session-catalog-coordinator.ts \
packages/runtime-host/src/__tests__/session-catalog-coordinator.test.ts \
apps/desktop/src/main/__tests__/runtime-host-oauth-ipc-main.test.ts \
apps/desktop/src/renderer/settings/providers-panel.tsx \
packages/runtime/src/__tests__/claude-subscription-runtime.test.ts \
packages/runtime/src/__tests__/oauth-login.test.ts || true
echo '=== Remaining removed experimental flag references ==='
rg -n -C 2 'MAKA_CLAUDE_SUBSCRIPTION_EXPERIMENTAL' . -g '!node_modules' -g '!dist' -g '!build' || true
echo '=== Exact diff for relevant files ==='
git diff --unified=35 origin/main...HEAD -- \
packages/runtime-host/src/protocol/index.ts \
packages/runtime/src/model-runtime.ts \
packages/runtime/src/__tests__/provider-contract-matrix.ts \
packages/storage/src/runtime-policy/coordinator.ts \
packages/storage/src/runtime-policy/operations.ts \
packages/storage/src/runtime-policy/connection-catalog-document.ts \
packages/storage/src/__tests__/runtime-policy-stores.test.ts \
packages/runtime-host/src/server/session-catalog-coordinator.ts \
packages/runtime-host/src/__tests__/session-catalog-coordinator.test.ts \
apps/desktop/src/main/__tests__/runtime-host-oauth-ipc-main.test.ts \
apps/desktop/src/renderer/settings/providers-panel.tsx \
packages/runtime/src/__tests__/claude-subscription-runtime.test.ts \
packages/runtime/src/__tests__/oauth-login.test.tsLength of output: 156942 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR body ==='
gh pr view 3183 --repo maka-agent/maka-agent --json body,url --jq '"URL: \(.url)\n\n\(.body)"'
echo '=== Candidate tracked follow-ups ==='
gh search issues \
--repo maka-agent/maka-agent \
--state open \
--limit 30 \
--json number,title,url,body \
--jq '.[] | select(
((.title + "\n" + .body) | test(
"request_authorization_code|complete-authorization|exchangeOAuthAuthorizationCode|paste-code";
"i"
))
) | "#\(.number) \(.title)\n\(.url)"' || trueLength of output: 8507
The Comment image satisfies the before/after UI screenshot requirement. It shows the retired composer state, model-list status, and connection-detail banner with unavailable actions removed. One follow-up remains: the PR body links the teardown scope comment, but not a tracked issue or follow-up PR for ✏️ Learnings added
You are interacting with an AI system. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for taking on the retirement as an end-to-end product change. The core direction is sound: keep the provider identity only so existing rows remain decodable and deletable, remove its OAuth/runtime implementation, and fail closed at execution. Four independent reviewer passes plus a read-only DeepSeek V4 Flash high-effort pass converged on the remaining boundary gap below.
The first-principles invariant should be: a retired provider may be decoded and deleted, but it cannot enter any ordinary mutation or action path. Today the read path clears a retired default and the execution path rejects it, while the catalog mutation API can still create a new claude-subscription row or commit it as a default because the provider remains a valid ProviderType; enabled retired rows can also still surface generic test actions. That creates states which a later read silently rewrites and actions which are guaranteed to fail.
The smallest coherent fix is to enforce retirement once at the storage mutation authority (create, setDefaultTarget, and any update that would make the row executable), then expose the same capability to shared action construction so test/default actions are omitted. Please keep decode/delete as the explicit exceptions. This is simpler than adding more renderer-specific guards and makes the retained registry entry serve exactly one compatibility purpose.
I did not run a local test suite; this review is based on the latest head, focused static tracing, the existing CI results, four independent @reviewer reviews, and a read-only ollama-cloud/deepseek-v4-flash high-effort review. AI-assisted review; I verified the reported control flow and discarded findings that were only artifacts of comparing this old branch head directly with current main.
中文评论
感谢把 Claude subscription 的退役做成端到端收口。核心方向是对的：只保留 provider 身份用于解码和删除旧连接，移除 OAuth/Runtime 实现，并在执行边界 fail closed。
剩余问题是 authority 没有完全统一：退休 provider 仍是合法 ProviderType，所以 mutation API 仍可新建该连接、把它设为默认，通用 action 层也仍可能提供测试入口；但读取时又会清空默认值，执行时必然拒绝。这会制造“写入成功、读回被改写”以及必然失败的操作。
更符合第一性原理和奥卡姆剃刀的方案，是在 storage mutation authority 一次性禁止退休 provider 的新建、设默认和重新启用，并让共享 action 构造消费同一 capability；仅把解码和删除保留为明确例外。这样不需要继续在各个 renderer 补零散判断。
本次未运行本地测试；结论来自最新 head 的静态追踪、现有 CI、四个独立 reviewer 和一次只读的 DeepSeek V4 Flash high-effort 审查。AI 辅助审查；我已人工复核控制流，并剔除了由旧分支与当前 main 直接比较产生的误报。
| protocol: 'anthropic', | ||
| runtimeAdapter: { kind: 'claude-subscription' }, | ||
| runtimeAdapter: { kind: 'unavailable' }, | ||
| retired: true, |
There was a problem hiding this comment.
P2 — Reject retired providers at the catalog mutation authority. Keeping this registry entry is necessary for decoding existing rows, but it also keeps claude-subscription valid for CreateCatalogConnectionInput. ConnectionCatalogDocumentOwner.create() does not check isRetiredProvider, so an IPC/protocol caller can still create a brand-new enabled connection that can never execute. The same gap lets setDefaultTarget() return committed for a retired row, after which read() silently normalizes that default back to null. Please make decode/delete the explicit exceptions and reject create/default/re-enable mutations for retired providers in the storage owner; add focused tests for create and set-default rejection.
ace2ca2 to
330cc83
Compare
Maka could sign in with a Claude Pro/Max subscription and send inference through it. Anthropic's Consumer Terms permit programmatic access to the consumer Claude services only through an API key or explicit permission, and this path had neither: it presented itself as Claude Code — borrowing that client_id, its User-Agent, its beta header set and an `x-app: cli` marker — to get requests accepted. The account carrying that risk is the user's, not Maka's. Remove the capability rather than gate it. `claude-subscription` keeps its registry entry so a stored connection still decodes and renders, and is marked `retired`, which is distinct from a provider that was never wired: both have no Runtime adapter, but only one used to work. Retirement is refused at each authority that could otherwise admit the connection, so no single revert makes it sendable again: - the auth contract hides every action, which is what makes the storage layer refuse a model fetch or a connection test - the readiness gate reports `provider_retired` before the send is admitted, instead of letting it fail inside model construction - the model catalog resolves every model to `provider_removed`, so the pickers stop offering them - the interactive-login allow list and the Host wire enum no longer name it - `getAIModel` and `resolveModelRuntime` throw as the last backstop Settings explains the state instead of pointing at a sign-in that no longer exists, and stops offering "set as default" and "test connection" for a connection that cannot perform either. Deleting the connection is what clears the credential this machine still holds. The impersonation code goes with it: the cloaked request builder, the Claude token endpoint and its client identity, the cloaked model-fetch headers, and the subscription usage/quota path that needed that same identity to read. `RUNTIME_HOST_COMPATIBILITY_EPOCH` moves to 23: the OAuth login provider enum and the account-usage operation both changed. Generated-by: Claude Code
330cc83 to
66f94be
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The latest head closes the prior retirement gaps cleanly. The compatibility epoch is now 26; legacy rows remain decodable/deletable but cannot be created, enabled, or selected as defaults; default targets pointing at retired connections are released on read; Host admission rejects provider_retired before credentials or Session persistence; and the auth/model/runtime backstops prevent an override bypass. Current-main integration is clean and all live checks are green.
From first principles, provider retirement now has one enforceable storage/execution contract rather than relying on UI removal. The remaining producerless request_authorization_code / paste-code path can be deleted as a separate, tracked cleanup; it is no longer reachable through a live provider and does not block this retirement. Please also update the PR description's epoch note from 24→25 to the actual 25→26.
Approved. Reviewed with Codex using two independent reviewer agents and an external DeepSeek review; I verified the latest head, mutation/admission/model backstops, compatibility epoch, current-main merge, and live CI.
中文
最新 head 已完整闭合此前的退休缺口：compatibility epoch 现为 26；旧行仍可解码/删除，但不能新建、启用或设为默认；读取时会释放指向退休连接的默认目标；Host admission 在读取凭据或持久化 Session 前返回 provider_retired；auth/model/runtime backstop 也阻止 override 绕过。与当前 main 可干净合并，实时检查全绿。
按第一性原理，provider retirement 现在由统一的存储/执行契约强制，而不是依赖 UI 删除。剩余没有 producer 的 request_authorization_code / paste-code 路径可以作为独立、可跟踪的清理删除；它已无法通过现有 provider 到达，不阻塞本次退休。另请把 PR 描述中的 epoch 从 24→25 修正为实际的 25→26。
批准。本次由 Codex 配合两个独立 reviewer agent 和外部 DeepSeek 审查；我核验了最新 head、mutation/admission/model backstop、compatibility epoch、与当前 main 的合并和实时 CI。
|
复核最新 head 还剩两个合并前建议修正的点：
|
The command palette filtered per-connection commands on `enabled` alone, so a retained retired connection — which stays enabled so its credential remains visible and deletable — still offered "set as default" and "test connection", both refused downstream by the storage default-target gate and the hidden auth actions. The palette now checks the same isRetiredProvider capability Settings uses, including the diag test-default entry for a stale in-memory default. Regression covers a retained-enabled retired row next to a live positive control; reverting the loop filter turns it red. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
|
@likun666661 — both fixed in P2, the palette. You were right, and it was the one shared surface the sweep missed: P3, the epoch note. The description now states the actual diff, 25 → 26, verified against The teardown promise is now #3219 rather than a comment: full file-by-file scope, what stays because the live device flows import it, and the epoch cost of the split. @M4n5ter @hqhq1025 — your review requests are addressed as of this head (Host admission with a retained credential, the retired default target, the storage mutation gates, and the fixture replacement), and @Astro-Han has approved the retirement chain. Could you take another look when you have a moment? |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The latest delta closes the remaining Command Palette action surface for retained retired providers. It reuses the existing retirement authority, covers all three connection command paths, and includes a discriminating live-connection positive control. I found no new P0–P3 issue in this head.
The prior epoch, Host admission, retired-default, mutation-authority, and runtime/auth backstop findings remain resolved. The producerless authorization-code path is now explicitly tracked as separate follow-up work.
Reviewed with Codex using two independent review passes and DeepSeek V4 Flash as an external adversarial pass; I verified the latest delta and prior-finding state against this exact head and current main.
中文
最新 delta 已关闭 retained retired provider 在 Command Palette 中最后的 action surface。实现复用了现有 retirement authority，覆盖三类 connection command，并用 live connection 正向对照避免过度过滤。当前 head 未发现新的 P0–P3。
此前的 epoch、Host admission、retired default、mutation authority 和 runtime/auth backstop 问题仍保持已解决；producerless authorization-code 路径已明确拆为后续工作。
本次由 Codex 两轮独立审查，并使用 DeepSeek V4 Flash 做外部对抗审查；我核对了当前 head、最新 delta 与 prior findings。
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
Requesting changes on the latest head.
The earlier epoch, default-target, general session-admission, mutation-authority, runtime-backstop, and Command Palette findings remain fixed. This pass found three non-duplicate residual paths:
- Removing
oauth.account.usage.fetchmakes previously issued Remote Host access files fail decoding, preventing the Host from starting. The known Remote Host population is almost nonexistent, so its practical reach is negligible, but the upgrade failure is deterministic for affected profiles. - Configured subagent admission still considers retained retired connections available. Direct spawn and graph provisioning can consequently persist a child/operator that can never execute.
- The CLI
/modelprojection still presents models belonging to the retired connection and only rejects them after selection.
The configured-subagent persistence gap independently warrants changes. Both remaining selection gaps can be closed by consuming the new canonical isRetiredProvider() authority rather than adding renderer- or caller-specific state.
The simplify audit found no additional merge blocker. The producerless authorization-code/paste-code path is already tracked by an existing review and is intentionally not duplicated here. Making retired connections fully read-only tombstones would be a reasonable follow-up simplification.
I verified these paths against head 6fc4447. Relevant targeted suites pass and all current GitHub checks are green; the missing retired-provider cases are not covered by those tests.
I’ll respond quickly to follow-ups on this PR, so please feel free to ping me directly.
简体中文
建议在最新 head 上 Request changes。
此前关于 compatibility epoch、default target、通用 session admission、mutation authority、runtime backstop 和 Command Palette 的问题仍保持已修复。本轮发现三个未与现有 review 重复的剩余入口：
- 删除
oauth.account.usage.fetch会使此前签发的 Remote Host access file 解码失败，阻止 Host 启动。已知 Remote Host 用户几乎不存在，实际影响面可以忽略不计，但对命中的 profile 来说升级故障是确定性的。 - Configured subagent admission 仍把保留的 retired connection 视为可用，因此直接 spawn 和 graph provisioning 可以持久化一个永远无法执行的 child/operator。
- CLI
/model投影仍会展示属于 retired connection 的模型，直到用户选择后才拒绝。
Configured-subagent 的持久化缺口本身足以支持 Request changes。两个剩余选择入口都可以通过消费新的统一 isRetiredProvider() authority 来关闭，不需要增加 renderer 或 caller 专属状态。
简化审计没有发现额外 merge blocker。没有 producer 的 authorization-code/paste-code 路径已经由现有 review 跟踪，本次不重复评论。将 retired connection 进一步收敛为完全只读的 tombstone，可以作为后续简化。
我在 head 6fc4447 上验证了这些路径。相关 targeted suites 通过，当前 GitHub checks 全绿；但现有测试没有覆盖上述 retired-provider 场景。
对于这个 PR，我会尽快回复后续讨论；有需要请直接 ping 我。
The list badge said provider_retired, but the editor still judged a connection by enabled alone in usableConnections, validConnection, and connectionOptions, so a retained retired row could be selected and saved into a preset the runtime admission is guaranteed to refuse. One predicate — isSelectableSubagentConnection, enabled and not retired — now feeds all of them plus the model derivations, and the option row for an enabled-but-retired connection says why it cannot be picked instead of graying out silently. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
|
@Astro-Han — the editor projection is closed in |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The latest head correctly closes the Desktop Subagent-editor gap: one isSelectableSubagentConnection() predicate now excludes retired providers from usable models, validity, and options, with focused retained/disabled/live coverage.
The existing catalog-mutation authority thread is still only partially closed. Create/default/session admission now reject retired providers, but connection.catalog.update can take a retained disabled claude-subscription row and commit enabled: true; runtime then predictably rejects it as provider_retired. I am not duplicating that inline thread. The smallest coherent fix is to reject re-enabling retired rows at the storage mutation authority (or reject all ordinary updates if retained rows are decode/delete-only), with no revision change on rejection.
All live checks are green, but merge state remains blocked by unresolved review state.
Reviewed with Codex using two independent reviewer agents and OpenCode Go DeepSeek V4 Flash (high); I verified the exact delta, the prior Desktop finding, storage/runtime retirement invariants, focused tests, and live CI.
中文
最新 head 已正确修复 Desktop Subagent editor 缺口：统一的 isSelectableSubagentConnection() predicate 会在 usable models、validity 和 options 中排除 retired provider，并有 retained/disabled/live 聚焦覆盖。
但现有 catalog mutation authority 线程仍只部分关闭。create/default/session admission 已拒绝 retired provider；connection.catalog.update 仍可把 retained disabled claude-subscription row 提交为 enabled: true，随后 runtime 必然以 provider_retired 拒绝。我不重复发布该行内线程。最小一致修复是在 storage mutation authority 拒绝重新启用 retired row；若 retained row 只用于 decode/delete，则可直接拒绝所有普通 update，并保证 rejection 不增加 revision。
实时检查全绿，但 unresolved review state 仍使 merge state blocked。
本次由 Codex 配合两个独立 reviewer agent，以及 OpenCode Go DeepSeek V4 Flash（high）审查；我核验了精确增量、既有 Desktop finding、storage/runtime retirement invariant、聚焦测试和实时 CI。
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
LGTM. The latest update closes the remaining subagent editor path, and the previously reported findings are resolved. All current checks pass.
简体中文
LGTM。最新更新已关闭剩余的 subagent editor 路径，之前提出的问题均已解决。当前检查全部通过。
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
One new merge-blocking finding is attached inline.
The earlier hqhq1025 blockers around the compatibility epoch, retained-credential Session admission, and retired default targets are fixed on this branch. I did not duplicate the existing unresolved retired-provider update/re-enable thread.
This exact head also conflicts with current main, whose Runtime Host compatibility epoch is already 28. The required rebase must allocate the next distinct epoch for this wire removal, preserve both sides' authority changes, and receive a complete new-head review.
I reviewed the complete revision; required checks are green, build:test passed, and 228 focused local tests passed.
Codex-assisted review performed under the maintainer-approved review workflow.
Config export carries every catalog connection, and this retirement deliberately retains existing claude-subscription rows so the user can see and delete them. A fresh-profile import saves connections one at a time, so that row reached createConnection(), which refuses it — and the refusal aborted the restore partway, after whichever live connections had already committed and before settings, credentials, and memory. That is the supported backup path for exactly the users this retirement affects. The storage guard stays: creating a retired connection is still refused, because it could never execute. Import planning no longer hands it one — the row is planned as skipped, and its credential is skipped with it, since only a created or overwritten slug gets a secret written. The regression restores a full bundle whose live connection is ordered before the retained retired one, so an aborted import would look like a partial success rather than a clean failure; reverting the filter turns it red. Reported by @hqhq1025. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
The previous config-restore P1 is fixed on this revision. Retired-provider connections are removed from the import plan before any connection save, their credentials remain unapplied, and the rest of the selected bundle continues normally. Exact-head build:test, 149 focused retirement/import tests, lint, format, and the Astryx surface inventory all passed.
I am leaving a COMMENT rather than approving because both hosted GitHub Actions suites are currently action_required, and the existing current-code update/re-enable thread remains unresolved. I found no new unique finding and did not duplicate that thread.
Codex-assisted review performed under the maintainer-approved review workflow.
|
Could a committer approve the workflow runs on What changed since the last review, so the new head is not a black box: @hqhq1025's P1 — config restore. You were right, and it was the worst kind of break: the users this retirement affects are exactly the ones whose backup carries a The guard stays, because creating a retired connection is still wrong. Import planning no longer hands it one: the row is planned as skipped, and its credential is skipped with it, since only a created or overwritten slug gets a secret written. The regression restores a full bundle whose live connection is ordered before the retained retired one — so a regression would look like a partial success rather than a clean failure — and reverting the filter turns it red. The rebase you asked for. Current
Epoch is 30, not the 24→25 an earlier description claimed and not the 26 this branch carried before the rebase. Main reached 29 while this sat, and main also adopted a changelog comment format for that constant, which the new entry follows. Verified with Local state: build, typecheck, lint, format clean; the retirement contract suite is 9/9; the runtime suite's failing set is byte-identical to the same suite at the merge base on this machine (environmental ripgrep/PTY/filesystem cases), and eval's is the Docker preflight. |
create() and setDefaultTarget() already refused a retired provider, but update() did not — so a disabled retired connection could be re-enabled and become a default candidate again, leaving every downstream refusal as the only thing between it and a Session. That was the half of the mutation-authority finding this branch had not closed. The whole update is refused rather than the `enabled` field alone: a retired connection has no edit worth committing, and a field-by-field allow list would have to be revisited for every field added later. The Desktop UI offers no enable toggle for a connection, so nothing legible is being taken away. Read and delete stay the exceptions, and the regression proves it — the refused row is still readable and still deletable afterwards, which is the whole reason it is retained. Reported by @Astro-Han; @hqhq1025 named the open thread. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
|
@hqhq1025 — thank you for naming the open thread rather than re-deriving it; I had closed half of @Astro-Han's mutation-authority finding and not noticed.
Read and delete stay the exceptions, and the regression asserts exactly that: after the refused edit the row is still readable and still deletable, which is the entire reason this retirement keeps it. Reverting the guard turns that test red. On your other reason for not approving: the workflow runs were approved and are green on the rebased head — the consolidated For the record on approvals: @M4n5ter approved at |
|
@M4n5ter — your approval at |
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
LGTM. The latest changes resolve the remaining catalog-update and config-restore issues, and I found no path that can restore authentication or execution for the retired provider.
Two user-facing inconsistencies remain:
- The retained connection detail still shows model management and advanced request editing. Model and request-body changes are rejected by the storage boundary, while request headers can be saved but can never affect execution.
- Because the retired default target is released during catalog loading, Health Center reports “Select a default model” instead of explaining that the provider has been retired.
Both are reachable for users with a legacy connection, but neither bypasses the storage, session-admission, OAuth, or runtime protections. I consider them non-blocking UI follow-ups. All current checks pass.
简体中文
LGTM。最新修改已解决剩余的 catalog update 和配置恢复问题，也没有发现能够恢复退休 provider 的认证或执行能力的路径。
目前仍有两处用户界面不一致：
- 保留的旧连接详情页仍显示模型管理和高级请求编辑。模型与 request body 修改会被存储边界拒绝；request headers 虽然可以保存，但永远不会参与执行。
- catalog 加载时会释放指向退休连接的默认目标，因此 Health Center 会提示“请选择默认模型”，而不是说明该 provider 已经退休。
拥有旧连接的用户确实可以遇到这些问题，但它们都无法绕过存储、Session admission、OAuth 或 Runtime 的保护。我认为可以作为非阻塞的 UI follow-up。当前检查全部通过。
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Requesting changes on head 46d9efffe73220642c177d7d31c9cf7d27a61936.
Problem reconstruction
This PR correctly retires the Claude subscription OAuth path: it removes the Claude client identity and request-cloaking implementation, keeps the provider registered only so existing rows remain decodable and deletable, and blocks auth, defaults, selection, Session admission, model resolution, testing, CLI choices, and subagent execution. The problem definition is correct, and the retained-tombstone direction follows first principles.
Findings
-
P2: connection-owned credential mutations still accept a retired row.
credential.vault.setwith arequest_headerslocator andconnection.request-headers.replaceboth remain remotely granted and both commit against a retainedclaude-subscriptionconnection. I reproduced both on this exact head. The catalog update guard therefore does not establish the stated read/delete-only invariant. -
P2: the Desktop detail still exposes mutating controls that conflict with the new storage contract. Request-body saves and enabled-model changes call
connection.catalog.update, which now deterministically throwsinvalid_connection_inputfor the retired row. The request-header editor is worse: it remains writable through the credential path above. A user sees a retirement alert beside controls that either fail or mutate a connection described as inert. -
P2: existing Sessions bound to this provider are not projected as stale in the task rail.
deriveStaleSessionIds()recognizes onlyconnection_missingandfake_backend, soprovider_retiredtasks look healthy until opened even though they can never send. The new active-task health copy does not cover that sidebar projection.
First-principles and Occam assessment
The simplest coherent final structure is one tombstone invariant: retired rows may be decoded, queried, exported if required for visibility, and deleted; every connection-owned write is refused centrally. The renderer should consume that same capability and render a read-only retirement detail with deletion as the only action. provider_retired should join the existing stale-session projection. Continuing to add isolated guards while leaving sibling mutation and presentation paths open is not yet the optimal architecture.
Deletion and tests
No merge-blocking low-quality tests need deletion. Add behavioral coverage for retired request-header/credential writes, a rendered read-only detail, and provider_retired stale-session classification. The producerless request_authorization_code path is real removable code, but it is unreachable and already tracked separately in #3219, so it does not block this PR.
Merge readiness
Not ready to merge. Clean build:test, workspace typecheck, 138 focused tests, Astryx inventory, and live GitHub CI/audit all pass. Residual verification gaps are live provider login and local Windows/Linux packaging; neither changes the three deterministic findings above. git diff --check also reports one extra blank line at EOF in onboarding-hero-copy.ts.
Codex-assisted review; I verified the complete diff, current main integration, exact live head, production call chains, focused tests, and the two credential-mutation reproducers before publication.
There was a problem hiding this comment.
Current-main update after the review above:
main advanced to 92da51d995fa80ab3629769b2af782c2cb3e8afb at 2026-08-21 06:08:59 UTC while this review was being submitted. GitHub now reports the PR as conflicting. A fresh git merge-tree shows content conflicts in packages/runtime-host/src/protocol/index.ts and docs/astryx-surface-file-inventory.md.
The protocol conflict is substantive: current main independently uses compatibility epoch 30 for managed Runtime Host credential pairing, while this branch also uses epoch 30 for the Claude OAuth wire removal. The required rebase must preserve both wire changes and allocate the next distinct epoch, 31, then regenerate the Astryx inventory and receive a complete new-head review.
This is an additional merge blocker; it does not replace the three P2 findings in the request-changes review.
Review reproduced two writes that still committed against a retained retired row: `credential.vault.set` with a `request_headers` locator, and `connection.request-headers.replace`. Guarding the catalog update alone had not established the invariant it claimed, because those are sibling writes to the same connection — and guarding entry points one at a time is what left them open. Every connection-owned write now passes one assertion. The credential vault and the request-header replacement share it with the catalog update, so a path added later inherits the refusal instead of needing to remember it. Reading, querying and deleting stay the exceptions, and the regression proves the row survives its refusals: after all three writes are rejected it is still readable and still deletable, which is the only reason it is retained. That refusal point moved earlier than an existing expectation: a client-supplied OAuth token for the retired provider used to be rejected for its credential kind and is now rejected for its connection, so that test asserts the connection-level code with the reason stated. The Desktop detail no longer offers what the storage layer refuses. The advanced-request, model-management and capability sections are hidden for a retired connection, leaving the retirement notice and deletion — the request-header editor was the worst of them, since before the vault refused it a user could save a header that could never reach a request. Sessions bound to a retired provider now project as stale in the task rail. `provider_retired` joins `connection_missing` and `fake_backend` there: the connection still exists and is still enabled, so nothing else about the row looks wrong and the task read as healthy until opened. Also in this push: current main is merged, and since main independently took epoch 30 for access-credential pairing, this wire removal takes 31. The Astryx inventory is regenerated and `git diff --check` is clean. Reported by @hqhq1025; the detail-page half was also observed by @M4n5ter. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
|
@hqhq1025 — all three P2s and the rebase are in P2-1 — sibling writes. You were right that the catalog guard did not establish the invariant, and right about why: I had been guarding entry points one at a time, and the credential vault and the request-header replacement are siblings of the update I had just closed. There is now one assertion — One consequence worth flagging rather than burying: the refusal now happens earlier than an existing test expected. A client-supplied OAuth token for the retired provider used to be rejected for its credential kind ( P2-2 — the detail page. Advanced request, model management and capabilities are hidden for a retired connection; what remains is the retirement notice and deletion. The request-header editor was the worst of the three, exactly as you said: before the vault refused it, a user could save a header that could never reach a request. @M4n5ter's two observations are the same surface and are covered by this. P2-3 — the rail. Epoch. Main took 30 for access-credential pairing while your review was in flight, so this takes 31, with both changelog entries preserved. Inventory regenerated (189 files); Local: build, typecheck, lint, format, Astryx inventory clean; |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Requesting changes on head 38814c64854ff2a74aabfdc8b40eaed3528663b7.
Problem reconstruction
This PR correctly removes the unsupported Claude subscription OAuth execution path while retaining legacy rows only so users can understand and delete them. The latest delta fixes the previously reported credential/request-header writes, makes the Desktop detail read-only, marks bound tasks stale, integrates current main, and assigns compatibility epoch 31. The problem definition and retained-tombstone direction are correct and follow first principles.
Findings
-
P2: a normal network-proxy mutation still rewrites retired rows.
mutatePolicy()callsclearAllConnectionLastTests()whenever the effective proxy changes. That helper removeslastTestand increments the revision of every tested connection, including a retained retired one. I reproduced a proxy change advancing the retired row revision and clearing its test fact. This contradicts the new read/delete-only invariant and can make a concurrent deletion fail as stale even though the user edited only global proxy settings. -
P3:
compareAndSetOAuthCredential()still bypasses the tombstone guard. With an existing retained OAuth credential, the internal store operation commits a refreshed secret because it validates onlyauthKind. I reproduced a credential revision advancing from 1 to 2. The current production caller is gated byresolveExecutionConnection(), so this is not presently a remote execution bypass, but it leaves the claimed storage invariant incomplete.
First-principles and Occam assessment
The smallest coherent final structure is to make every connection mutation consume one retirement capability, including indirect catalog invalidations and OAuth compare-and-set. Global proxy changes should clear verification only for writable/live rows; retired tombstones should remain byte-stable until deletion. Adding more renderer or caller-specific guards is unnecessary.
Deletion and tests
No merge-blocking low-quality test should be deleted. Add one proxy-mutation regression that proves a retired row's revision and lastTest remain unchanged, plus one compare-and-set regression. The producerless authorization-code path remains removable code but is already tracked separately in #3219 and does not block this PR.
Merge readiness
Not ready to merge. Focused retirement/runtime tests passed (24 tests), Storage/Runtime/Runtime Host typechecks passed, formatting and Astryx inventory passed, and git diff --check is clean. The full build reached and passed the affected Core, Storage, Runtime, and Runtime Host packages, then stopped in unchanged packages/ui type mismatches; Desktop typecheck hit the same existing UI surface. GitHub currently reports no checks for this new head, so hosted CI is also still outstanding.
Codex-assisted adversarial re-review; I verified the exact live head, current-main merge, prior-finding fixes, production mutation paths, and both reproducers before publication.
Two indirect writes still reached the tombstone, both reproduced in review. A network-proxy change invalidates every tested connection's verification, and that swept the retired row too — clearing a `lastTest` for a provider that can no longer be tested, and bumping a revision that a deletion started elsewhere then failed against as stale. A global setting has no business mutating this row, so the invalidation now skips it. One predicate decides both the early return and the rewrite, because disagreeing there would write an identical document with a higher revision, which is the same defect wearing a different shape. The regression pins both directions: the live row loses its verification and gains a revision, the retired one keeps both. `compareAndSetOAuthCredential` validated only the auth kind, which a retired provider still declares, so a refresh could rotate the very credential the user is being asked to delete. It now passes the same assertion as the other three connection writes. No production caller reaches it — execution resolution refuses first — which is exactly why it would have stayed open. Its regression seeds the credential the way the row itself is seeded, since the guard now refuses to write one. Reported by @hqhq1025, both with reproducers. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
|
@hqhq1025 — both are in P2 — global invalidation. The proxy change now leaves retired rows alone. One predicate decides both the early return and the rewrite: had I guarded only the map, the early return would still have seen work to do and written an identical document with a higher revision — the same defect in a different shape, and the kind that survives a shallow test. The regression pins both directions, because a version that invalidates nothing at all would otherwise pass it: the live row loses its P3 — compare-and-set. Now behind the same assertion as the other three connection writes. Worth stating plainly: it is unreachable in production today, and that is precisely why it was still open — nothing exercised it, so nothing failed. Its regression has to seed the credential by writing the vault document directly, because the guard added earlier refuses to create one; that is the same shape as seeding the connection row, and it reproduces the real case, which is a credential written before the provider was retired. After the refusal the credential is still at revision 1 — the point being that the thing the user came to delete has not been quietly rewritten. That makes four direct write paths through one assertion (catalog update, credential vault, request headers, OAuth refresh) plus one global operation that now excludes tombstones rather than refusing — the distinction being that a proxy edit is legitimate and simply has no business touching this row. Local: build, typecheck, lint, format, Astryx inventory clean; |
|
@hqhq1025 — when you have a moment, could you take another look at |
Requested changes were addressed in subsequent commits; dismissing this stale blocking review on the newer head.
5 participants
Summary
Maka could sign in with a Claude Pro/Max subscription and send inference through it. Anthropic's Consumer Terms permit programmatic access to the consumer Claude services only through an API key or explicit permission, and this path had neither — it presented itself as Claude Code (borrowing that
client_id, its User-Agent, its beta header set, and anx-app: climarker) to get requests accepted. The account carrying that risk is the user's, not Maka's.This removes the capability rather than gating it further.
claude-subscriptionkeeps its registry entry, and the entry is markedretired— deliberately distinct from a provider that was never wired, since both end up with no Runtime adapter but only one of them used to work and needs to explain itself.Keeping it registered is a product choice, not a technical necessity. Unregistering is possible:
isRetiredGeminiCliConnectionis the precedent, filtering by rawproviderTypestring before canonical decode. It is avoided here because a user who signed in has a credential on this machine, and the connection is what makes that visible and deletable — unregister it and onboarding reportsneeds_connection, with the credential still on disk and nothing pointing at it. (decodeProviderTypedoes throw on unknown types andconnection-catalog-codec.tswould fail the whole document, so unregistering without the filtering precedent would cost the user every other connection — but that is a reason to use the precedent, not a reason the choice was forced.)Retirement is refused at each authority that could otherwise admit the connection, so no single revert makes it sendable again:
deriveProviderAuthContracthidden— this is what makes the storage layer refuse a model fetch or a connection testresolveExecutionConnectionprovider_retired, a first-class result, so a retained credential can no longer read asreadySessionCatalogCoordinatorisConnectionReadyprovider_retired, before a Desktop send is admittedbuildConnectionModelCatalogEntriesprovider_removed, so the pickers stop offering themresolveModelRuntimeOAUTH_LOGIN_PROVIDERS(Host wire)getAIModelLoading the catalog also releases a
defaultTargetthat points at a retained retired connection, so new Sessions stop defaulting to something that cannot run. Settings no longer shows a 默认 Badge on one either, since retirement is checked before both default states — a Badge with no control to move the default off it would be exactly the read-only label that slot exists to avoid.Settings explains the state instead of pointing at a sign-in that no longer exists, and stops offering "set as default" and "test connection". Onboarding gets its own
all_connections_retiredcopy — the generic blocked text tells the user to re-check credentials, sign-in status, and network, and for a retired provider all three lead nowhere. Deleting the connection is what clears the credential this machine still holds.The impersonation code goes with it: the cloaked request builder, the Claude token endpoint and its client identity, the cloaked model-fetch headers, and the subscription usage/quota path that needed that same identity to read.
RUNTIME_HOST_COMPATIBILITY_EPOCHmoves 25 → 26 (main reached 25 while this PR was in review, so the rebase re-took the next slot; the description tracks the actual diff). This PR changes the wire contract (removesclaude-subscriptionfromOAUTH_LOGIN_PROVIDERS, deletes theoauth.account.usage.fetchoperation), so it takes its own epoch rather than riding one raised elsewhere. An earlier revision of this description claimed a 22 → 23 bump that a rebase had turned into a no-op; that was wrong, and @Astro-Han and @hqhq1025 both caught it.Refs #3143
Verification
Built from clean (
npm run cleanfirst) — an incremental build hid a real error during this work, so every gate below was re-run after a full rebuild.npm run build,npm run typecheck— 0 errorslint,format:check,astryx:surface-inventory,check:stale,check:third-party-notices,check:windows-cargo-notices— passnpm test— the failing set is identical to the same suite on a cleanorigin/mainworktree on this machine (environment-related ripgrep / PTY / filesystem-worker cases). Diffed programmatically rather than eyeballed.Each enforcement point was mutation-checked: reverting it individually turns a test red.
runtimeAdapter: 'unavailable'retired: trueproviderAvailableretirement branchresolveExecutionConnectionrefusaldefaultTargetreleaseOAUTH_LOGIN_PROVIDERSisConnectionReadygateOne correction to an earlier revision of this table: the storage interactive-login allow-list row does not discriminate on its own — reverting only that line leaves the suite green, because the auth-contract gate independently produces the same refusal. That is defense in depth working as intended, but the row overstated its own power and @Astro-Han was right to flag it.
End-to-end, against a real
claude-subscriptionconnection already stored on this machine (packaged app, installed, launched) — see the before/after screenshots below.Not run: Windows and Linux packaging, and the Playwright E2E suite. The new retired UI states have no Storybook stories.
Breaking change
A user signed in through the Claude subscription can no longer send with it. The connection and its stored credential are left in place and readable so the state is explainable; an Anthropic API key connection is the replacement path. Clients built against an earlier epoch are rejected at the handshake by the bump above.
Follow-up
The
request_authorization_code/ paste-code presentation has no producer left once this lands, and both @M4n5ter and @hqhq1025 asked for its removal. It is the immediate next PR rather than part of this one; the exact file-by-file scope, what stays because the live device flows import it, and why the unused#exchangeCodefield is deliberately left in place here are in this comment.AI use
Select exactly one:
Tool(s) and scope: Claude Code — drafted the removal, the enforcement points and their tests, and ran the verification described above. Reviewed and submitted by the contributor of record.
Generated-by: Claude Codeis on the commit.Checklist
Does this PR entail a change in behavior?