diff --git a/devlog/_plan/260807_untouched_bug_stack/070_mimo_token_plan_preset.md b/devlog/_plan/260807_untouched_bug_stack/070_mimo_token_plan_preset.md new file mode 100644 index 000000000..bc9497e3b --- /dev/null +++ b/devlog/_plan/260807_untouched_bug_stack/070_mimo_token_plan_preset.md @@ -0,0 +1,100 @@ +# 070 — #1158: MiMo token-plan rejects Responses custom tools + +## Defect + +Xiaomi MiMo's paid token-plan endpoint (`https://token-plan-cn.xiaomimimo.com/v1`) +speaks the Responses wire for plain requests but rejects `type: "custom"` tools +with `400 responses_feature_not_supported`. Codex emits custom tools for +`apply_patch` and other freeform tools, so an agentic turn fails on this +provider while a plain chat turn succeeds. + +The reporter confirmed the same account works fully through `openai-chat`. + +Current state, verified on the tree: + +- `src/adapters/openai-responses.ts` filters only model-declared hosted tools; + arbitrary custom tools are serialized unchanged. +- `src/providers/registry.ts:2017` has `xiaomi` (Anthropic wire) and `:2020` + has `mimo-free` (free tier, own adapter). **No token-plan preset exists.** + +So a token-plan user hand-rolls the provider, and because MiMo documents +Responses support they naturally pick `openai-responses` — the one wire that +breaks. + +## Why a preset rather than tool stripping + +Two rejected alternatives, both worse: + +**Strip custom tools for this provider.** `apply_patch` IS a custom tool, so +stripping it disables the Codex agent loop. The user would get a provider that +no longer 400s and no longer edits files. Spark's specialized stripping +(`openai-responses.ts:248-353`) does exactly this and is not a model to follow +here. + +**`modelWireDefaults`.** That mechanism moves individual models between wires. +The known-good wire here is provider-wide, not per-model, so a preset states the +fact directly instead of repeating it per model. + +The Chat path already handles custom tools correctly: `src/responses/parser.ts` +lowers them to `{input: string}` functions and `src/bridge.ts` restores them as +`custom_tool_call`. Nothing needs building — the provider just has to be pointed +at the wire that works. + +## Change + +Add a registry entry beside the existing Xiaomi ones: + +``` +id: "mimo" +label: "Xiaomi MiMo (token plan)" +baseUrl: "https://token-plan-cn.xiaomimimo.com/v1" +adapter: "openai-chat" +authKind: "key" +models: mimo-v2.5-pro, mimo-v2.5 +efforts: low | medium | high per model +effortMap: xhigh/max/ultra -> high +preserveCustomDestination: true +``` + +`preserveCustomDestination` matters: someone may already have a hand-rolled +provider named `mimo`, and without it `routedProviderConfig()` would canonicalize +their base URL onto ours — silently retargeting their key at a different host. +The same hazard the `zhipu-bigmodel` comment documents at `registry.ts:1668`. + +The effort clamp is because MiMo's ladder stops at `high`; forwarding `ultra` +would send a value the provider rejects. + +## Tests + +**Preset shape** — `tests/provider-registry-parity.test.ts`, `MiMo token-plan +preset uses Chat and clamps extended efforts`. Assert the derived key-login +provider's adapter, base URL, and models, and that the effort map collapses the +three extended tiers to `high`. Add the id to `EXPECTED_KEY_PROVIDER_IDS`; the +parity test fails without that, which is the intended gate. + +**Collision preservation** — the shape test does NOT exercise the claim this +plan actually leans on. `preserveCustomDestination` is only consulted when the +configured endpoint, adapter, or auth differs +(`src/providers/registry.ts:2111-2124`), and only then does +`routedProviderConfig()` keep the user's row (`src/router.ts:254-258`). So the +regression has to route, not just inspect metadata: define a pre-existing +provider named `mimo` pointing somewhere else with a different adapter, route +through it, and assert its base URL, adapter, and key are untouched. Follow the +shape of `tests/cline-pass-provider.test.ts:163-183`. + +Without that second test the plan asserts a safety property it never checks — +and silently retargeting an existing user's key at another host is precisely the +failure the `zhipu-bigmodel` comment warns about. + +## Blast radius + +Registry-derived key login, `ocx init`, the provider picker, and catalog +metadata. `xiaomi` and `mimo-free` are untouched — different hosts, different +wires. + +## What this does not do + +It does not make the Responses wire work on this endpoint. If MiMo later accepts +custom tools there, the preset is the thing to revisit. The issue asked for +"preset or guidance"; this is the preset, and the note field carries the +guidance. diff --git a/devlog/_plan/260807_untouched_bug_stack/080_windows_acl_userdomain.md b/devlog/_plan/260807_untouched_bug_stack/080_windows_acl_userdomain.md new file mode 100644 index 000000000..ea786abea --- /dev/null +++ b/devlog/_plan/260807_untouched_bug_stack/080_windows_acl_userdomain.md @@ -0,0 +1,156 @@ +# 080 — #1149: ACL hardening trusts USERDOMAIN + +## Defect + +`currentWindowsUser()` in `src/lib/windows-secret-acl.ts` builds the icacls +principal as: + +```ts +return domain ? `${domain}\\${username}` : username; +``` + +`USERDOMAIN` is essentially always set — on a machine that is not domain-joined +it holds the COMPUTER NAME — so the `domain ? ... : username` branch never +takes the fallback the comment describes. Every machine gets +`DOMAIN\User`, and on a workgroup box that is `COMPUTERNAME\User`. + +That form is not always what the effective token accepts: a renamed computer, a +Microsoft-account login (where the local profile name and the account name +differ), or an AzureAD-joined machine can all produce a principal icacls cannot +resolve. The grant then fails, the harden fails closed, and every native request +returns 503. + +Both environment variables are also writable by the process that launched us, +which makes the principal attacker-influenceable in a permissions path. + +## Change + +Prefer the current user's SID. A SID sidesteps the naming question entirely: it +is what the token actually carries, it is identical in domain and workgroup +cases, it survives a computer rename, and icacls accepts `*S-1-5-21-...` +directly as a principal. + +**Reuse the existing resolver, do not write a new one.** +`src/codex/user-identity.ts:69-75` already resolves +`[WindowsIdentity]::GetCurrent().User.Value` and validates it against +`SID_PATTERN`. An earlier draft of this plan proposed a fresh `whoami /user` +lookup, which would have been strictly worse: an unqualified `whoami` is +resolvable through `PATH`, so a permissions path would have gained an +executable-substitution surface it does not currently have. Extract the shared +resolver rather than duplicating it, and keep the trusted-executable launch. + +Two constraints the extraction must honor: + +- **Charge the lookup against the harden deadline.** The 30s envelope from `030` + is per harden call and a spawn is not free; a lookup outside the budget could + push a call past it. +- **Cache only success.** A failed lookup must be retried, not memoized into a + permanent fallback. + +### Failure is fail-closed, not a guess + +Resolution order is deliberately short: + +1. Shared SID resolver -> `*`. +2. On failure, the existing `USERDOMAIN\USERNAME` form (current behavior). + +An earlier draft put bare `USERNAME` ahead of the qualified form. That is wrong +in a security-relevant way: a bare name resolves ambiguously when a local and a +domain account share it, which is the exact authority-confusion class this fix +exists to remove. The grant ACE is installed before inheritance is removed +(`src/lib/windows-secret-acl.ts:459-465`), so a wrong principal is not a +cosmetic error. + +On a `required: true` harden, a SID failure should fail closed rather than fall +back at all. The qualified fallback exists only for the optional read path, +where the current behavior is already the status quo. + +## What the principal resolves to + +| Case | Before | After | +|---|---|---| +| Domain-joined | `CORP\jane` | jane's SID | +| Workgroup | `DESKTOP-A1\jane` (may fail) | jane's SID | +| Microsoft account | `DESKTOP-A1\jane` (profile name may differ from account) | jane's SID | +| Renamed computer | stale `OLDNAME\jane` | jane's SID | + +## Security posture + +This must not become a way to grant to the WRONG principal. The SID comes from +the effective Windows token (`[WindowsIdentity]::GetCurrent().User.Value`), not +from environment. A malformed or unparseable result is rejected, never coerced. + +Net effect on the environment-variable exposure: the SID path does not read +`USERDOMAIN` or `USERNAME` at all, so the common case stops depending on +writable environment state. + +## Do not relocate the existing resolver — extract a neutral primitive + +`src/codex/user-identity.ts` has the right lookup but the wrong packaging for +this caller, in four specific ways: + +1. It throws `CodexUserIdentityRefusal` (`:33-44`), a domain-specific error the + ACL path has no business catching. +2. It launches an unqualified `powershell.exe` (`:46-60`) — PATH-resolvable, the + same substitution surface that disqualified the `whoami` idea. +3. It has no timeout and no `windowsHide`. +4. It is synchronous, so reusing it inside `hardenSecretPathAsync` + (`src/lib/windows-secret-acl.ts:700-748`) would block the event loop and + defeat the async path. + +So the shared piece is a neutral primitive: SID parsing/validation plus bounded +**sync and async** resolvers that launch an absolute System32 PowerShell path. +`user-identity.ts` keeps translating failures into its own refusal type; the ACL +owner applies its own required/optional policy. Cache successful values only. + +**Do not write a third System32 resolver.** +`resolveTrustedWindowsPowerShellExe()` in `src/lib/windows-elevation.ts:103-138` +already resolves and validates the executable through `GetSystemDirectoryW`. +Reuse it, or lift its trusted-path machinery into a neutral Windows +system-tools module — a fresh `SystemRoot`/PATH lookup would reintroduce exactly +the substitution surface this whole section exists to close. The SID primitive +still needs its own bounded sync/async execution and neutral error type; only +the executable resolution is shared. + +## Timeout must not poison the path memo + +A SID lookup that times out has to be classified distinctly. If it surfaces as +an ordinary `ETIMEDOUT`, `hardenEntry` records the path in `timedOutPaths` +(`src/lib/windows-secret-acl.ts:687-690`) and skips it for the rest of the +process — even though no `icacls` operation timed out and the path itself is +fine. Charge the lookup against the shared deadline, but keep its failure out of +that memo. + +## Test + +`tests/windows-secret-acl.test.ts`, using the existing seams plus an injected +SID resolver: + +- `resolves the ACL principal from the token SID, not USERDOMAIN` — resolver + returns a SID; assert the icacls invocation carries `*S-1-...` and that + `USERDOMAIN` is never consulted. +- `a malformed SID is rejected rather than passed to icacls` — resolver returns + garbage; assert no garbage principal reaches icacls. +- `a required harden fails closed when the SID cannot be resolved` — no bare + username, no guess. +- `an optional harden falls back to the qualified name` — the only place the + legacy `USERDOMAIN\USERNAME` form survives, and its boundary is explicit. +- `a SID lookup timeout does not mark the path as icacls-timed-out` — assert the + path is retryable rather than stuck in `timedOutPaths`. +- `only successful lookups are cached` — a failure must not memoize into a + permanent fallback. +- Both the sync and async harden entry points get coverage; the async one is the + reason a synchronous spawn is unacceptable. + +Note on the memo: `loadConfig` calls three harden wrappers +(`src/config.ts:1759-1764`), but SID resolution only runs on Windows for paths +that exist and are not already memoized, so "runs three times" is an upper +bound rather than the normal case. + +Env isolation follows the `previousAclTimeout` pattern already in +`beforeEach`/`afterEach`. + +## Security review gate + +Same class as `030`: this is credential-permission handling and needs explicit +security review plus `bun run privacy:scan` before the PR leaves draft. diff --git a/devlog/_plan/260807_untouched_bug_stack/090_deepseek_502_parked.md b/devlog/_plan/260807_untouched_bug_stack/090_deepseek_502_parked.md new file mode 100644 index 000000000..5d6fb53b6 --- /dev/null +++ b/devlog/_plan/260807_untouched_bug_stack/090_deepseek_502_parked.md @@ -0,0 +1,57 @@ +# 090 — #1176: DeepSeek V4 Flash 502 — parked, NEEDS-REPRO + +Recording why this is NOT being fixed in this stack, because "we looked and +chose not to act" is a different disposition from "nobody looked". + +## What is real + +The 502 is real and the local guard that produces it is identified: + +- `src/server/responses/core.ts` configures 180s total/first-byte and 30s + inter-chunk inactivity for the bounded JSON body read. +- `src/lib/bounded-body.ts` arms the first-byte deadline, then resets to the + 30s inter-chunk deadline after every non-empty chunk. +- A truncated bounded read becomes the reported local 502. +- `src/providers/registry.ts` deliberately routes DeepSeek Responses inbound + through bounded JSON, because native DeepSeek SSE previously omitted or + delayed terminal events (#875). + +## Why we are not fixing it + +The reporter is on v2.10.2, which already contains PR #1088 — the first-byte +deadline fix for the identical symptom in #1065. So this is not a stale package. + +Their trace shows `durationMs: 90241`. That rules out the 180s total deadline +and is consistent with a 30s inter-chunk timeout after an earlier chunk. What it +does NOT show is how many bytes arrived, when the last chunk landed, or whether +the upstream would eventually have completed. + +Three hypotheses remain live and the trace cannot separate them: + +1. A legitimate >30s inter-chunk pause from this model, which our deadline kills. +2. The bounded-JSON route is wrong for this case. +3. A genuine upstream stall that we correctly surface as 502. + +If (3), the "fix" is a regression: we would be removing a guard that is doing +its job. If (1), the fix is a model-scoped inactivity policy — not a bump to the +shared helper default, which has callers in Responses, upstream-error handling, +Kiro, Command Code, auth/quota, images, and web search. + +Raising a timeout because a timeout fired is how a real stall becomes a hang. + +## What would unpark it + +Either would settle it: + +- A reporter capture with byte counts and chunk timings, or +- A controlled direct-upstream run that waits past 30s and shows whether the + same body eventually reaches a valid EOF. + +## Cheap step that makes the next report decisive + +Independent of the fix, the failure is currently indistinguishable from other +truncations. Extending `BoundedBodyResult` with `timeoutPhase` +(`first_byte` | `inter_chunk` | `total`), `receivedBytes`, and `nonEmptyChunks`, +and using them in the 502 message, would mean the NEXT such report arrives +already diagnosed. That is a small observational change with no behavior risk, +and it belongs in its own PR rather than inside a bug-fix stack. diff --git a/src/providers/registry.ts b/src/providers/registry.ts index e1dc32e40..19d580565 100644 --- a/src/providers/registry.ts +++ b/src/providers/registry.ts @@ -2066,6 +2066,34 @@ export const PROVIDER_REGISTRY: readonly ProviderRegistryEntry[] = [ models: ["mimo-auto"], note: "No key needed — uses Xiaomi MiMo's free public tier (limited-time offer). A JWT is bootstrapped automatically with an anonymous random client id stored locally. The endpoint contract mirrors the official MiMoCode client and is not publicly documented — Xiaomi may change or restrict it at any time. Prompts may be processed/retained by Xiaomi; do not send confidential material.", }, + // Xiaomi MiMo paid token plan. Separate host and wire from both `xiaomi` (Anthropic) and + // `mimo-free` (free tier, bespoke adapter), so it needs its own entry rather than a variant. + // + // Pinned to openai-chat deliberately (#1158). The endpoint answers the Responses wire for + // plain turns, which is why users configuring it by hand pick `openai-responses` — MiMo + // documents Responses support. But its gateway rejects `type: "custom"` tools with + // `400 responses_feature_not_supported`, and `apply_patch` is a custom tool, so every agentic + // turn fails while chat turns succeed. The Chat path lowers custom tools to `{input: string}` + // functions and restores them as `custom_tool_call`, so the capability survives intact. + // Stripping the tools instead would stop the 400 and disable the agent loop. + { + id: "mimo", + label: "Xiaomi MiMo (token plan)", + baseUrl: "https://token-plan-cn.xiaomimimo.com/v1", + adapter: "openai-chat", + authKind: "key", + dashboardUrl: "https://xiaomimimo.com", + defaultModel: "mimo-v2.5-pro", + models: ["mimo-v2.5-pro", "mimo-v2.5"], + // The gateway validates the ladder strictly and rejects anything above `high`. + reasoningEfforts: ["low", "medium", "high"], + reasoningEffortMap: { xhigh: "high", max: "high", ultra: "high" }, + // A user may already have hand-rolled a provider under this id against a different host; + // without this, routedProviderConfig() would canonicalize their base URL onto ours and send + // their key somewhere they did not choose. + preserveCustomDestination: true, + note: "Xiaomi MiMo paid token plan. Pinned to the Chat wire: the Responses endpoint rejects freeform (custom) tools such as apply_patch with 400 responses_feature_not_supported, so agentic turns fail there while plain turns succeed. Reasoning tiers above high are clamped.", + }, { id: "cloudflare-ai-gateway", label: "Cloudflare AI Gateway", baseUrl: "https://gateway.ai.cloudflare.com/v1/{account-id}/{gateway}/anthropic", adapter: "anthropic", authKind: "key", dashboardUrl: "https://dash.cloudflare.com/?to=/:account/ai/ai-gateway" }, { // Cloudflare Workers AI: OpenAI-compatible endpoint. The base URL contains {account_id} diff --git a/tests/mimo-token-plan-provider.test.ts b/tests/mimo-token-plan-provider.test.ts new file mode 100644 index 000000000..17314d6e7 --- /dev/null +++ b/tests/mimo-token-plan-provider.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, test } from "bun:test"; +import { KEY_LOGIN_PROVIDERS } from "../src/oauth/key-providers"; +import { PROVIDER_REGISTRY } from "../src/providers/registry"; +import { routeModel } from "../src/router"; +import type { OcxConfig } from "../src/types"; + +describe("Xiaomi MiMo token plan (#1158)", () => { + const entry = PROVIDER_REGISTRY.find(row => row.id === "mimo"); + + test("is pinned to the Chat wire, which is the one that accepts custom tools", () => { + // The endpoint answers Responses for plain turns, so a hand-configured provider naturally + // picks `openai-responses` — and then every agentic turn 400s, because the gateway rejects + // `type: "custom"` tools and `apply_patch` is one. The preset exists to make the working + // wire the default. + expect(entry?.adapter).toBe("openai-chat"); + expect(entry?.baseUrl).toBe("https://token-plan-cn.xiaomimimo.com/v1"); + expect(entry?.models).toEqual(["mimo-v2.5-pro", "mimo-v2.5"]); + + const derived = KEY_LOGIN_PROVIDERS["mimo"]; + expect(derived?.adapter).toBe("openai-chat"); + expect(derived?.baseUrl).toBe("https://token-plan-cn.xiaomimimo.com/v1"); + }); + + test("clamps reasoning tiers above the ladder the gateway validates", () => { + expect(entry?.reasoningEfforts).toEqual(["low", "medium", "high"]); + for (const tier of ["xhigh", "max", "ultra"]) { + expect(entry?.reasoningEffortMap?.[tier]).toBe("high"); + } + }); + + test("a same-named custom provider keeps its own destination and credential", () => { + // The claim the preset actually leans on, and the one the shape assertions above do NOT + // exercise. Someone may already have a hand-rolled provider called `mimo` pointing + // elsewhere; without `preserveCustomDestination`, routing would canonicalize their base URL + // onto the registry's and send their key to a host they never chose. + const config: OcxConfig = { + port: 10100, + defaultProvider: "mimo", + providers: { + mimo: { + adapter: "openai-responses", + baseUrl: "https://private.example/v1", + apiKey: "user-key", + authMode: "key", + }, + }, + }; + + const route = routeModel(config, "mimo/custom-model"); + expect(route.provider.baseUrl).toBe("https://private.example/v1"); + expect(route.provider.apiKey).toBe("user-key"); + expect(route.provider.adapter).toBe("openai-responses"); + // The registry's effort clamp must not be applied to a row we do not own either. + expect(route.provider.reasoningEfforts).toBeUndefined(); + }); +}); diff --git a/tests/provider-registry-parity.test.ts b/tests/provider-registry-parity.test.ts index 94581a530..131ae199e 100644 --- a/tests/provider-registry-parity.test.ts +++ b/tests/provider-registry-parity.test.ts @@ -35,7 +35,7 @@ const EXPECTED_KEY_PROVIDER_IDS = [ "huggingface", "nvidia", "venice", "zai", "zhipu-bigmodel", "zhipu-bigmodel-coding", "nanogpt", "synthetic", "siliconflow", "qwen-cloud", "tencent-coding-plan", "volcengine", "volcengine-coding-plan", "volcengine-agent-plan", "qianfan", "alibaba", "alibaba-token-plan", "alibaba-token-plan-intl", "parallel", "zenmux", "litellm", "ollama-cloud", "mistral", "minimax", "minimax-cn", "kimi-code", "opencode-zen", "vercel-ai-gateway", - "opencode-free", "xiaomi", "kilo", "mimo-free", "cloudflare-ai-gateway", "cloudflare-workers-ai", "gitlab-duo", + "opencode-free", "xiaomi", "kilo", "mimo-free", "mimo", "cloudflare-ai-gateway", "cloudflare-workers-ai", "gitlab-duo", ]; describe("provider registry parity", () => {