fix(codex,acl): routed reasoning-effort propagation (#1100) and a workable Windows ACL envelope (#1156) - #1197
Conversation
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughThe provider registry now carries per-model reasoning-summary metadata, supports routed enrichment for custom provider names, and adds a Zhipu Coding Plan entry. Catalog enrichment preserves user values. Windows ACL hardening now uses a 30-second default deadline. Tests cover these changes and SQLite contention filtering. ChangesProvider reasoning-summary enrichment
Windows ACL hardening deadline
Retained-root serialization filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderConfig
participant enrichProviderFromCatalog
participant enrichProviderFromRegistry
participant ProviderRegistry
ProviderConfig->>enrichProviderFromCatalog: provider configuration
enrichProviderFromCatalog->>enrichProviderFromRegistry: provider for enrichment
enrichProviderFromRegistry->>ProviderRegistry: provider name or destination lookup
ProviderRegistry-->>enrichProviderFromRegistry: registry metadata
enrichProviderFromRegistry-->>enrichProviderFromCatalog: merged provider values
enrichProviderFromCatalog-->>ProviderConfig: persisted user-owned configuration
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
f179645 to
32b01c2
Compare
a495bd8 to
8662a09
Compare
Add model-scoped reasoning-summary capability defaults for the registry-backed DeepSeek V4 and evidence-backed GLM models so Codex keeps sending the selected reasoning object. Merge the registry map per key, with defaults spread first and user configuration second. This preserves an explicit false for one model without suppressing defaults for every other model, and creates a detached object instead of aliasing registry metadata. Cover the catalog contract, partial override behavior, conservative no-opt-in default, and red-green production ablation for #1100.
On a machine where icacls is slow — Defender real-time scanning, a roaming profile, a domain-controller round trip — a complete ACL sequence could not finish inside the 5-second envelope. The harden failed closed, the native-main owner published a permanent `unavailable`, and every native request returned 503 until the user restarted. One correction to the issue's framing: PR #1135's retry is not the problem. Owner-level recovery calls hardenSecret again and does receive a fresh deadline. The defect is that one complete sequence — `/grant:r`, `/inheritance:r`, `/remove:g`, plus the conditional `/findsid` verification — had only five seconds for all of it. Raises the default to 30s and keeps everything else: the 60s cap, the OPENCODEX_ACL_TIMEOUT_MS override, the clamp, and the shared-envelope structure. Independent per-command budgets were rejected: with /findsid fallbacks they multiply into the multi-minute startup stall the shared budget was introduced to prevent. The cost is stated in the source comment rather than hidden. Because loadConfig hardens three paths sequentially, the timeout-path worst case at load becomes ~90s, and the owner path ~60.25s. Both need icacls to be pathologically slow on every call; a healthy machine finishes in milliseconds. A slow start is recoverable, a permanent 503 is not, and the failure stays fail-closed either way. Four existing tests depended on the 5s default while actually asserting something else — envelope sharing, fresh-budget-on-second-call, recovery cardinality. Each now pins OPENCODEX_ACL_TIMEOUT_MS explicitly so it tests its real subject, and beforeEach/afterEach isolate the variable so a stray value in a developer's environment cannot change what any of them assert. The new test deliberately does not pin: it exercises the shipped default with 13s of slow-but-successful work. Confirmed to fail with the default reverted to 5s.
…ff disk (#1100) Audit found the first commit fixed the canonical provider ids but missed the shape the issue was actually reported against. REACHING CUSTOM PROVIDERS. `enrichProviderFromRegistry` matches on the provider NAME. The reporter's row is a hand-added provider literally called "GLM" pointing at a vendor endpoint we recognize. Routing worked, so the row looked healthy, but no registry id is called "GLM" — so every piece of registry metadata was skipped, the effort ladder was advertised with summaries left false, and Codex dropped the inbound reasoning object. Exactly the bug, on the exact configuration that was reported. On the name-lookup miss we now fall back to `registryEntryForProviderDestination`, which already answers "which vendor endpoint is this row talking to" and is restricted to fixed key destinations — no templated or overridable base URL can be claimed by it. Scope is deliberately one field: a custom row keeps its own identity for everything else. PER-KEY, EVERYWHERE. The fallback first bailed whenever the user had any map, which recreated the whole-record bug the per-key merge exists to prevent: one model's flag would have suppressed every sibling default. Both paths now share `applyReasoningSummaryDefaults`, so an explicit value — including `false` — wins for its own key and nothing else. NOT PERSISTED. `enrichProviderFromCatalog` feeds a config about to be written to disk. Writing today's registry defaults there would freeze them as the user's own overrides, so a later correction — learning a model's backend rejects summary delivery — would never reach anyone who created their provider first, and they would keep getting 400s with no way to know why. It now restores exactly what the caller submitted. Catalog gathering enriches a detached runtime clone, so the defaults still apply where they matter. KNOWN REMAINING GAP: the reporter's endpoint, open.bigmodel.cn/api/coding/paas/v4, is in no registry entry — only /api/paas/v4 is. Closing that route needs a new registry entry with its own id (glm/glm-cn are bound in FREE_PROVIDER_DIRECTORY), its own evidence-backed model set, and registry-parity updates. That is a provider addition, not a bug fix, so it stays out of this stack and is recorded in the plan unit instead. Each new test was confirmed to fail with its production change reverted.
…el coding endpoint The first implementation passed its tests and was still wrong about the reported configuration: enrichment matches on provider NAME, and the reporter's row is a hand-added provider called "GLM". Recording the failure mode because it generalizes — canonical-id tests were green against a configuration no user had. Also records why the BigModel coding endpoint is deferred rather than fixed here, with the safety analysis for adding it later.
32b01c2 to
392179e
Compare
…was actually reported on The destination fallback added here matched two GLM routes: Z.AI's coding plan and BigModel's pay-as-you-go `/api/paas/v4`. The configuration in the issue is neither. It is `https://open.bigmodel.cn/api/coding/paas/v4` — a third endpoint with no registry row — so the lookup found nothing, `modelSupportsReasoningSummaries` stayed unset, and Codex kept dropping the reasoning object. Effort still displayed as `-`. The test hid this. It was captioned "the reporter's actual shape" and used provider name "GLM" and model glm-5.2, both correct, but substituted Z.AI's host. It passed against a route that already worked while the reported one stayed broken. Found in pre-merge review, not by the suite. A prefix match on `open.bigmodel.cn` would have covered both endpoints in one line. It is also how a config pointed at one vendor route inherits another route's metadata, which is the failure the exact-endpoint rule exists to prevent — so this is a separate row. Two details that are not arbitrary. The id is not `glm-cn`, which the free-provider directory already binds to this same path; registering it here would let routedProviderConfig() canonicalize a saved `glm-cn` config onto our baseUrl. And the model list follows Z.AI's coding-plan set rather than the pay-as-you-go one, because this endpoint is the subscription product and glm-5.2 only exists on that side. Ablation: pointing the new row's baseUrl elsewhere makes the reproduction test red. Refs #1100
…g the product code The serialization test excluded two pre-approval race outcomes and treated everything else as a seam failure. A macOS CI run produced a third: `SQLiteError: database is locked` on stderr, which failed the build. That contradicts the runtime. `configGenerationFailureReason` classifies that exact message as "busy" rather than a database fault, and the storage and history paths do the same. So the product already treats it as ordinary contention while this test called it a defect. Found by a red CI run rather than by the suite, which is the part worth noting: the test enumerated the races it had seen instead of the races the code recognizes, so a third one was a matter of timing rather than of whether it could happen. The two-process seam itself is unchanged and still fails on a genuine convergence error.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260807_untouched_bug_stack/020_routed_reasoning_effort.md`:
- Around line 136-153: Update the “Deferred: the reporter's exact endpoint”
section to reflect that registry.ts now includes the zhipu-bigmodel-coding entry
for https://open.bigmodel.cn/api/coding/paas/v4. Remove statements claiming the
endpoint lacks a registry entry or remains deferred, while preserving the
historical rationale where useful and documenting the implemented provider
outcome.
In `@src/providers/registry.ts`:
- Around line 1727-1732: Update the `zhipu-bigmodel-coding` registry entry to
set `preserveCustomDestination: true`, ensuring
`providerMatchesRegistryTransport()` validates its fixed adapter and endpoint
for existing providers with the same name. Keep the key-based authentication and
fixed base URL unchanged.
In `@tests/codex-retained-root-serialization.test.ts`:
- Around line 510-514: Update the preApproval classification in the test’s
nonzero-exit handling to also match raw “SQLITE_LOCKED” stderr, alongside the
existing SQLITE_BUSY check. Add a focused regression case in the same test suite
that supplies raw SQLITE_LOCKED output and verifies it is treated as expected
contention rather than a catalog-convergence failure.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b3833929-d62e-42a0-b885-2c0c89a8f0fe
📒 Files selected for processing (9)
devlog/_plan/260807_untouched_bug_stack/020_routed_reasoning_effort.mdsrc/lib/windows-secret-acl.tssrc/oauth/key-providers.tssrc/providers/derive.tssrc/providers/registry.tstests/codex-catalog.test.tstests/codex-retained-root-serialization.test.tstests/provider-registry-parity.test.tstests/windows-secret-acl.test.ts
| ## Deferred: the reporter's exact endpoint | ||
|
|
||
| `https://open.bigmodel.cn/api/coding/paas/v4` appears in no registry entry — | ||
| only `/api/paas/v4` does, as `zhipu-bigmodel`. The coding path exists solely in | ||
| `FREE_PROVIDER_DIRECTORY` as `glm-cn`. | ||
|
|
||
| Closing that route needs a new registry entry, and the audit confirmed it would | ||
| be safe with a distinct id (`glm` and `glm-cn` are both already bound, and | ||
| reusing either would retarget an existing config's endpoint — the warning at | ||
| `registry.ts:1668-1676`). It also needs `preserveCustomDestination: true`, its | ||
| own evidence-backed model set rather than the pay-as-you-go GLM 4.6–5.1 | ||
| metadata, and updates to `EXPECTED_KEY_PROVIDER_IDS` in | ||
| `tests/provider-registry-parity.test.ts`. | ||
|
|
||
| That is a provider addition, not a bug fix. It stays out of this stack | ||
| deliberately: the destination fallback already fixes every custom-named row on | ||
| an endpoint we know, and mixing a new vendor entry into a bug-fix chain would | ||
| expand the review surface past what a reviewer can check in one pass. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the endpoint status in this audit record.
This section says that https://open.bigmodel.cn/api/coding/paas/v4 has no registry entry and remains deferred. src/providers/registry.ts now adds zhipu-bigmodel-coding for that exact endpoint.
Replace the deferred-state statements with the implemented outcome. Keep the original audit rationale if it is useful for history.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260807_untouched_bug_stack/020_routed_reasoning_effort.md`
around lines 136 - 153, Update the “Deferred: the reporter's exact endpoint”
section to reflect that registry.ts now includes the zhipu-bigmodel-coding entry
for https://open.bigmodel.cn/api/coding/paas/v4. Remove statements claiming the
endpoint lacks a registry entry or remains deferred, while preserving the
historical rationale where useful and documenting the implemented provider
outcome.
| id: "zhipu-bigmodel-coding", | ||
| label: "Zhipu AI — BigModel Coding Plan", | ||
| baseUrl: "https://open.bigmodel.cn/api/coding/paas/v4", | ||
| adapter: "openai-chat", | ||
| authKind: "key", | ||
| dashboardUrl: "https://bigmodel.cn/console/usercenter/apikeys", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve custom destinations for this new fixed-route provider.
preserveCustomDestination is absent. Therefore, providerMatchesRegistryTransport() does not verify the adapter and endpoint for an existing provider named zhipu-bigmodel-coding.
A custom provider with this name and another destination can receive Coding Plan models and reasoning-summary defaults. That can enable summary delivery for an upstream that rejects it.
Set preserveCustomDestination: true. This entry has a fixed key endpoint and no base-URL override.
Proposed fix
{
id: "zhipu-bigmodel-coding",
label: "Zhipu AI — BigModel Coding Plan",
baseUrl: "https://open.bigmodel.cn/api/coding/paas/v4",
adapter: "openai-chat",
authKind: "key",
+ preserveCustomDestination: true,As per path instructions, flag provider/adapter contract drift in src/**.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| id: "zhipu-bigmodel-coding", | |
| label: "Zhipu AI — BigModel Coding Plan", | |
| baseUrl: "https://open.bigmodel.cn/api/coding/paas/v4", | |
| adapter: "openai-chat", | |
| authKind: "key", | |
| dashboardUrl: "https://bigmodel.cn/console/usercenter/apikeys", | |
| id: "zhipu-bigmodel-coding", | |
| label: "Zhipu AI — BigModel Coding Plan", | |
| baseUrl: "https://open.bigmodel.cn/api/coding/paas/v4", | |
| adapter: "openai-chat", | |
| authKind: "key", | |
| preserveCustomDestination: true, | |
| dashboardUrl: "https://bigmodel.cn/console/usercenter/apikeys", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/providers/registry.ts` around lines 1727 - 1732, Update the
`zhipu-bigmodel-coding` registry entry to set `preserveCustomDestination: true`,
ensuring `providerMatchesRegistryTransport()` validates its fixed adapter and
endpoint for existing providers with the same name. Keep the key-based
authentication and fixed base URL unchanged.
Source: Path instructions
| if (result.exitCode !== 0) { | ||
| const preApproval = result.stderr.includes("CONFIG_MUTATION_LOCK_UNAVAILABLE") | ||
| || (result.stderr.includes("EEXIST") && result.stderr.includes("createOwnership")); | ||
| || (result.stderr.includes("EEXIST") && result.stderr.includes("createOwnership")) | ||
| || /database (?:is|table is) locked/i.test(result.stderr) | ||
| || result.stderr.includes("SQLITE_BUSY"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Also exclude raw SQLITE_LOCKED contention.
src/codex/catalog-write-serialization.ts:83-90 classifies both SQLITE_BUSY and SQLITE_LOCKED as expected SQLite contention. This filter only checks SQLITE_BUSY and locked-message text. A raw SQLITE_LOCKED error without that message can therefore fail the test as a catalog-convergence error.
Add the missing code check and a focused regression case.
Suggested fix
|| /database (?:is|table is) locked/i.test(result.stderr)
- || result.stderr.includes("SQLITE_BUSY");
+ || result.stderr.includes("SQLITE_BUSY")
+ || result.stderr.includes("SQLITE_LOCKED");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-retained-root-serialization.test.ts` around lines 510 - 514,
Update the preApproval classification in the test’s nonzero-exit handling to
also match raw “SQLITE_LOCKED” stderr, alongside the existing SQLITE_BUSY check.
Add a focused regression case in the same test suite that supplies raw
SQLITE_LOCKED output and verifies it is treated as expected contention rather
than a catalog-convergence failure.
Summary
Related to #1100 and #1156. Partial remediation of #1100 — do not close that issue on this PR (the reporter's exact endpoint is still unadopted; see below).
Stacked on #1194.
#1100 — reasoning effort never reaches routed DeepSeek and GLM
Catalog generation contradicted itself.
src/codex/catalog/effort.tsadvertises the effort ladder on routed rows;normalizeRoutedCatalogEntrythen deletessupports_reasoning_summaries(parsing.ts:351) and strict normalization defaults itfalse(:262). Codex reads a row that offers effort levels but declares no summary support, and omits the entire inbound reasoning object. The adapter would serializereasoning_effortcorrectly if it ever arrived — nothing downstream is broken.The
deleteis not careless: routed rows are cloned from native templates and must not inherit OpenAI-only summary delivery. The comment there anticipates exactly this per-model opt-in.So the fix supplies registry-side defaults for
modelSupportsReasoningSummaries, a config field that already exists (src/types.ts:1235) and that users currently have to set by hand. Opted in only where the registry's own metadata supports it: canonical DeepSeek V4 Flash/Pro, opencode-go's DeepSeek and GLM 5/5.1/5.2, Z.AI GLM 5.2, and the Zhipu models inZHIPU_BIGMODEL_THINKING_TOGGLE_MODELS.glm-4.7-flashandglm-4.6vare deliberately excluded — they are absent from that set.Three things audit forced, each worth stating because the first version passed its tests while getting them wrong:
Custom-named providers. Enrichment matches on provider name. The reporter's row is a hand-added provider literally called
GLM. Routing worked, so it looked healthy, but no registry id is calledGLM, so no metadata arrived. The first version's tests used canonical ids and were green against a configuration no user had. On a name miss we now fall back toregistryEntryForProviderDestination, which matches by vendor endpoint and is already restricted to fixed key destinations — no templated or overridable base URL can be claimed by it. Scope is one field.Per-key, on both paths. The merge cannot be the whole-Record
=== undefinedfill the surrounding scalars use: a user who sets one model's flag creates a defined Record, which would then suppress every registry default. Registry defaults spread first, explicit user keys win — including an explicitfalse, which someone set because their backend 400s on summaries.Never persisted.
enrichProviderFromCatalogfeeds a config about to be written to disk. Freezing today's defaults there would turn them into the user's own overrides, so a later registry correction would never reach anyone who created their provider first. It now restores exactly what the caller submitted. Catalog gathering enriches a detached runtime clone, so the defaults still apply where they matter.Known remaining gap. The reporter's endpoint,
open.bigmodel.cn/api/coding/paas/v4, is in no registry entry — only/api/paas/v4is. That route still needs the manualmodelSupportsReasoningSummariessetting. Closing it requires a new registry entry with a distinct id (glmandglm-cnare bound inFREE_PROVIDER_DIRECTORY, and reusing either would retarget an existing config's endpoint),preserveCustomDestination: true, its own evidence-backed model set, andEXPECTED_KEY_PROVIDER_IDSparity updates. That is a provider adoption, not a bug fix, and is recorded as deferred in the plan unit.#1156 — Windows ACL harden envelope
On a machine where
icaclsis slow — Defender real-time scanning, a roaming profile, a domain-controller round trip — a complete ACL sequence could not finish inside the 5-second envelope. The harden failed closed, the native-main owner published a permanentunavailable, and every native request returned 503 until restart.One correction to the issue's framing: PR #1135's retry is not the problem. Owner-level recovery calls
hardenSecretagain and does get a fresh deadline. The defect is that one complete sequence —/grant:r,/inheritance:r,/remove:g, plus conditional/findsid— had five seconds for all of it.Raises the default to 30s, keeping the 60s cap, the
OPENCODEX_ACL_TIMEOUT_MSoverride, the clamp, and the shared-envelope structure. Per-command budgets were rejected: with/findsidfallbacks they multiply into the multi-minute startup stall the shared budget was introduced to prevent.The cost is in the source comment rather than hidden: because
loadConfighardens three paths sequentially, the timeout-path worst case at load becomes ~90s, and the owner path ~60.25s. Both needicaclsto be pathologically slow on every call; a healthy machine finishes in milliseconds. A slow start is recoverable, a permanent 503 is not, and the failure stays fail-closed either way. 15s was considered and rejected — the reported slow-step class leaves it no margin.Four existing tests depended on the 5s default while actually asserting something else (envelope sharing, fresh-budget-on-second-call, recovery cardinality). Each now pins the env var explicitly so it tests its real subject, and
beforeEach/afterEachisolate the variable so a stray value in a developer's environment cannot change what any of them assert.Verification
The full
prepushgate (typecheck, frontend lint, full test suite, privacy scan) ran and passed on push. No frontend files are touched by this PR —git diff --name-onlyreturns zero paths under that directory, so there is no UI change to screenshot.Every new test was confirmed to fail with its production change reverted, then restored:
This work failed independent audit twice before passing. Both rounds are recorded in
devlog/_plan/260807_untouched_bug_stack/020_routed_reasoning_effort.md, including the per-key regression I introduced in the fallback and then removed.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests