feat(codex): add routed tool-discovery compatibility profiles - #1607
Open
lidge-jun wants to merge 13 commits into
Open
feat(codex): add routed tool-discovery compatibility profiles#1607lidge-jun wants to merge 13 commits into
lidge-jun wants to merge 13 commits into
Conversation
Replaces the hard-coded Cursor/non-Cursor boolean behind routed `supports_search_tool` with a resolved, route-scoped policy. Defaults are unchanged: with no configuration, non-Cursor routed rows stay deferred (`supports_search_tool: true` + `web_search_tool_type: "text_and_image"`) and Cursor stays direct, exactly as PR #1596 shipped. Every pre-existing catalog test passes unmodified, which is the proof that nothing moved. New config, both optional: routedToolDiscovery?: "auto" | "deferred" | "direct" modelRoutedToolDiscovery?: Record<string, "auto" | "deferred" | "direct"> Precedence is Cursor hard fence > model override > provider override > auto. `auto` resolves to deferred for non-Cursor rows and never reaches serialization: `CatalogModel.toolDiscoveryMode` carries only the resolved value, resolved once in applyProviderConfigHints() so configured, live-discovered, cached and combo-derived rows agree. Combos resolve conservatively (one direct member forces direct) because a single public row cannot vary after target selection, and the field is emitted only when it departs from the default. Both the explicit fingerprint and the provider-graph identity see the new fields, so two different policies cannot share a stale gather. Closes the unresolved P2 from #1596 rather than reproducing it: both catalog construction paths now share one fence, isCursorRoute(), which decides on provider identity and falls back to the `cursor/` slug prefix only for callers with no CatalogModel. Previously parsing.ts tested the slug while sync.ts tested the provider, so a `cursor/`-aliased combo whose canonical provider is `combo` was classified differently depending on whether a template happened to exist. Config admission mirrors the house pattern: the load path degrades a malformed value with `.catch(undefined)` so a typo cannot cost a user their providers or credentials, while the write boundary rejects it outright with a path-specific `schema_invalid: providers.<name>.<field>: ...`. The validator reads only own data properties via Object.getOwnPropertyDescriptor, so an accessor- or prototype-polluted candidate is rejected without ever invoking the getter. What `direct` is not: under code mode Codex installs nested MCP tools on the `tools`/`ALL_TOOLS` globals in BOTH exposures, so for an eligible tool `direct` buys no reachability — it moves full schemas into `exec.description` at the measured 2.7x turn-1 cost and changes `tool_search` construction. It cannot repair a tool removed by `direct_only_tool_namespaces`, `excluded_tool_namespaces`, or MCP/App policy filtering. It is a comprehension and compatibility lever, documented as such. Also corrects structure/03: Cursor emits `supports_search_tool: false` and omits `web_search_tool_type`; it does not "advertise neither flag" (CodeRabbit's unresolved note on #1596). Plan, evidence and upstream citations: devlog/_plan/260813_routed_tool_discovery_profiles. Verification: bun x tsc --noEmit clean; bun test on the 7 affected suites = 392 pass / 0 fail (25 new cases covering the precedence matrix, Cursor fence, combo derivation, propagation isolation and config admission); privacy:scan passed. Full suite runs on the Linux CI host.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1d655debf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+654
to
+656
| // Resolve routed discovery here so configured, live-discovered, cached and combo-derived | ||
| // rows all carry the same value and `auto` never reaches serialization. | ||
| toolDiscoveryMode: resolveConfiguredRoutedToolDiscoveryMode(name, prov, model.id).mode, |
There was a problem hiding this comment.
When the configured model is represented by customModels, the custom row replaces the hinted provider row but does not inherit toolDiscoveryMode; similarly, augmentRoutedModelsWithCapturedOpenAiApiRows() reconstructs trusted openai-apikey rows without this field. In either case a configured direct policy becomes undefined, serialization silently falls back to deferred, and combos derived from these rows also get the wrong mode. Apply the policy in a final common derivation pass or propagate it through both replacement paths.
AGENTS.md reference: src/AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
Comment on lines
+419
to
+421
| // Provider identity first, slug prefix only as a fallback — one shared fence for both the | ||
| // template and template-less paths (see tool-discovery.ts isCursorRoute). | ||
| const isCursorEntry = isCursorRoute(entry.slug, options.providerId); |
There was a problem hiding this comment.
For a provider configured under a custom name with adapter: "cursor", the resolver correctly hard-fences discovery to direct, but deriveEntry() passes only model.provider here, so isCursorRoute() classifies it as non-Cursor. The resulting row incorrectly retains web_search_tool_type even though this transport bypasses the sidecar, and it also loses Cursor's automatic parallel-tool-call advertisement. Carry the adapter-derived Cursor identity through CatalogModel or pass an explicit Cursor flag to normalization.
Useful? React with 👍 / 👎.
Comment on lines
+2286
to
+2288
| const mode = ownDataProperty(provider, "routedToolDiscovery"); | ||
| if (mode !== undefined && !isRoutedToolDiscoveryMode(mode)) { | ||
| return `schema_invalid: providers.${name}.routedToolDiscovery: ${ROUTED_TOOL_DISCOVERY_EXPECTED}`; |
There was a problem hiding this comment.
When validateConfigCandidate() receives a provider whose own routedToolDiscovery property is an accessor, ownDataProperty() returns undefined, but this branch does not reject the present non-data descriptor as the model-map branch does. Validation then reaches configSchema.safeParse(), which reads the property, invokes the getter, and can accept its result, defeating the stated non-invoking admission boundary. Check Object.hasOwn(provider, "routedToolDiscovery") and reject an undefined descriptor result before invoking Zod.
Useful? React with 👍 / 👎.
An independent code review of c1d655d returned FAIL on four blockers, each reproduced before fixing. All four are closed. 1. Default preservation for `cursor/`-aliased combos. Reconciling the two historical fences by preferring provider identity UNFENCED a real row: a combo aliased `cursor/gpt-5.5` with canonical provider `combo` went from `supports_search_tool: false` (template path, fenced on the slug) to `true`. isCursorRoute() now UNIONS the available signals instead of ranking them. Reconciliation can only ever add fencing, never remove it, because an under-fenced Cursor row advertises a deferred surface its runTurn transport cannot serve, while over-fencing costs only payload. The test that codified the wrong default now pins the original one. 2. Getter-safe write boundary. ownDataProperty() collapsed "absent" and "accessor" into undefined, so an accessor-backed `routedToolDiscovery` looked like a missing field: the validator passed and Zod invoked the getter moments later, accepting "direct". It now returns a tagged absent/accessor/data result and rejects accessors on `providers`, on each provider entry, on both policy fields and on every model-map entry, before `configSchema.safeParse()`. 3. Propagation completeness. Custom rows (`customModels`) and trusted `openai-apikey` rows are rebuilt without passing through applyProviderConfigHints, so a provider-level `direct` silently degraded to deferred for exactly the models an operator hand-declared. Both now resolve the policy explicitly, and custom rows inherit `toolDiscoveryMode`/ `cursorRoute` from the provider-derived row they replace, matching how the other capability fields already inherit. 4. Fence agreement between resolution and serialization. The resolver fences on provider name OR adapter; serialization saw only the provider id, so a Cursor-adapter gateway under a custom provider name was hard-fenced to direct yet still emitted `web_search_tool_type` and missed Cursor's parallel-tool advertisement. The resolver now stamps `CatalogModel.cursorRoute` where both signals are known, and serialization honors it. Test gap from the same review: the new suite exercised normalizeRoutedCatalogEntry directly and never drove sync.ts's template-less branch. Added three buildCatalogEntries(null, ...) cases covering fallback direct, fallback default and the fallback Cursor fence, plus accessor cases for the provider entry and the providers map. Verification: bun x tsc --noEmit clean; bun test across the 8 affected suites = 399 pass / 0 fail (32 in the focused file, up from 25); bun run privacy:scan passed; all four blockers re-run and confirmed closed, including a gathered custom model with a provider-level override now resolving to direct.
…e write boundary Round-2 review found the descriptor guard still had a hole. `ownDataProperty()` returned "absent" for a key reachable only through the prototype chain, so `routedToolDiscoveryError()` treated it as missing and let the candidate through to `configSchema.safeParse()`, which then read it. Reproduced three ways: an inherited `routedToolDiscovery: "direct"` was accepted and persisted into the admitted config, an inherited getter for that field was invoked once and its value accepted, and an inherited getter for the root `providers` key was invoked once and accepted. `ownDataProperty()` now returns a fourth result, "inherited", determined with `key in target` — which walks the prototype chain WITHOUT reading, so an inherited accessor is classified rather than invoked. Inherited keys are rejected alongside own accessors on `providers`, on each provider entry, and on both policy fields. The previous test was weak for the reason the reviewer identified: it used an inherited value of `"eager"`, which Zod's `.catch(undefined)` discards anyway, so it passed for the wrong reason. It now uses a VALID inherited value, which is the case that actually leaked, plus two inherited-getter cases asserting the getter call count stays at zero. Also corrects an overstated comment in tool-discovery.ts. The union fence was justified as "over-fencing only costs payload"; the reviewer showed that is incomplete, since a combo may legally be aliased `cursor/<name>` (only `combo/` is reserved) and fencing it also removes hosted-search metadata and applies Cursor's parallel-tool advertisement. That row's fenced shape is exactly what shipped before this PR, so the union preserves behavior rather than regressing it, but the comment now says so honestly and names the real fix: reserve the `cursor/` alias prefix as a deliberate, documented compatibility break. Verification: bun x tsc --noEmit clean; 401 pass / 0 fail across 8 suites (37 in the focused file); privacy:scan passed; all three inherited-property reproductions re-run and confirmed closed.
Owner
Author
Full suite verification — Linux x86_64Run on a Linux host matching CI, at the exact PR head
Review historyThis branch went through three rounds of independent adversarial code review. Every finding was reproduced before being fixed, and the fixes are separate commits so the progression is auditable:
Final verdict: PASS, with no remaining blockers. One tradeoff worth a reviewer's attention
The cost is real and documented in the source comment: a combo may legally be aliased |
Adds two end-to-end cases: two configs differing only in `routedToolDiscovery` gather distinct results, and an identical policy still reuses the flight. Both are labeled honestly, because ablation proved the obvious claim false. Deleting `rtd`/`mrtd` from `providerCatalogFingerprint` leaves both cases GREEN: `providerGraphIdentity` already hashes the whole admitted provider row and refuses the join on its own. So these guard end-to-end behavior; they do not prove the fingerprint carries the policy, and the comment says so rather than letting a future reader mistake them for that proof. The fingerprint fields stay regardless. It is an explicit allow-list whose omissions have leaked flights twice before — credentials until `authIdentity` landed, then `reasoningEfforts` — both reproduced against real routes. Keeping it semantically complete is defense in depth, per the existing comment at provider-fetch.ts:198-208. Verification: bun x tsc --noEmit clean; 403 pass / 0 fail across 8 suites; privacy:scan passed; ablation run recorded above.
Owner
Author
|
Deferred from today's landing round with a concrete blocker: this branch now conflicts with #1602 (routed passthrough deferred-tool promotion) and #1604 (Daybreak Blue capability preservation) both landed a few hours ago and touch the same regions this PR rewrites — the merge stops on I resolved conflicts by hand for #1581 during this round, but that was a four-line alignment prop. This is 798 lines replacing the hard-coded routed Current |
…r test An independent audit of 524b649 returned FAIL. Two findings were correct and are fixed here; the doc translations are unrelated-looking but in scope. The "still reuses the gather for an identical policy" test never observed reuse. Its two calls were sequential and awaited, while a completed flight is removed from the in-flight map immediately (provider-fetch.ts finally-block), so equal results proved nothing about flight behavior. Replaced with a genuinely concurrent Promise.all case that asserts two different policies do not serve one another's rows, and the surrounding comment now says these pin the admission invariant rather than the fingerprint field list. The audit also showed WP3's "little left to do" claim was overstated. Added the planned coverage that was actually missing: - 023: zero-config byte comparison, an absolute emitted-key-set pin, the Cursor row shape, and assertions that the internal `toolDiscoveryMode`/`cursorRoute` fields never reach the Codex catalog on either construction path. - 025: the remaining combo compositions (direct in any position, arity 1..3, undefined members) and confirmation the field is omitted entirely for an all-deferred combo. Every new guard was ablation-verified rather than assumed. That mattered twice: the first byte-comparison test passed with a stray field injected, because two rows built by the same path both carry the leak; and the key-set pin initially appeared to pass an ablation of normalizeRoutedCatalogEntry, which turned out to be because buildCatalogEntries(null, ...) exercises the template-less branch that never calls it. Both paths now have their own guard, and injecting a stray field into either one fails a test. Also propagates the two config rows to the ja/ko/ru/zh-cn/zh-tw provider reference pages so the translated docs do not contradict the English source. Verification: bun x tsc --noEmit clean; 411 pass / 0 fail across 8 suites (44 in the focused file, up from 36); privacy:scan passed; all ablations restored and `git diff --quiet` confirmed clean on the touched source files.
…ocklist Re-audit of 9315ad0 found my previous commit message overstated its own coverage, and the reviewer was right. The template-path guard rejected three named policy fields plus one hard-coded sentinel and required lowercase keys. Injecting the anticipated `leaked_internal_field` failed it, which is what I tested; injecting `another_internal_field` passed straight through. So "injecting a stray field into either path fails a test" was true of the fallback path and false of the template path. It now asserts the exact emitted key set, matching the fallback guard, and is verified with the reviewer's own ablation: an arbitrarily named field injected into normalizeRoutedCatalogEntry fails it. Also records Phase 2 as NOT fully closed in 029, rather than letting the shipped config half imply the whole phase landed. Two items stay open as named debt: the 020 single-variable code-mode differential, which needs a running Codex client and keeps the direct-is-not-a-reachability-fix claim source-derived until it runs, and 024's model-map divergence plus warm-cache refresh cases. Verification: bun x tsc --noEmit clean; 414 pass / 0 fail across 8 suites (47 in the focused file); privacy:scan passed; SHA256SUMS regenerated and fully verified; ablation restored with git diff --quiet confirmed clean.
…outed discovery An independent audit of the WP3 work-phase returned FAIL and was right: the previous gather-identity pair could not observe flight behavior at all. A completed flight is removed from `gatherInflight` in its finally block, so two sequential awaited calls compare equal even when nothing is shared. Replaces them with a latched harness that injects the provider's own `fetch` executor and counts upstream discovery, then asserts the real contract: - two concurrent identical-policy callers cause ONE discovery (they joined); - two concurrent differing-policy callers cause TWO (joining would serve one config the other's catalog); - flights differing only in the per-model map also split; - the policy is re-resolved against a warm model cache rather than inherited. Ablation matrix, recorded in the test file so nobody has to guess what these pin: removing `rtd`/`mrtd` from the fingerprint leaves them green, and so does neutralizing `discoveryPolicyIdentity` or `providerGraphIdentity` individually — but neutralizing all three fails 2 of 3. They are not vacuous; they detect a real collision. No single mechanism is what they pin, because the three are redundant by design, and the comment now says exactly that instead of implying fingerprint proof. Also from the audit: - combo matrix completed to all five documented rows (devlog 025), plus an explicit assertion that an all-deferred combo OMITS the key rather than merely not being direct; - backward-compat cases from devlog 023: a pre-field config does not gain persisted fields on read, and a configured pair survives an unrelated save (downgrade tolerance through `.passthrough()`); - two overclaims softened. `structure/03` no longer says `auto` is "byte-for-byte" or that every row shape "agrees" automatically — it now says the shape is asserted per-key on both paths, and that custom-model and trusted `openai-apikey` rows resolve the policy explicitly because they are rebuilt outside `applyProviderConfigHints`. - The reachability claim is relabeled as source-derived rather than executed proof, in the English docs and all five translated locales, since the single-variable code-mode differential (devlog 020) has not been run. Locale sync: `routedToolDiscovery` and `modelRoutedToolDiscovery` are now documented in ko, ja, ru, zh-cn and zh-tw, which AGENTS.md requires so translations do not contradict the English source by omission. Verification: bun x tsc --noEmit clean; 414 pass / 0 fail across 8 suites (47 in the focused file); privacy:scan passed; ablation runs recorded above.
The previous revision of 029 listed 024's model-map divergence and warm-cache policy refresh as open debt. They had already landed in the latched-fetch concurrency harness, so the note contradicted the tests sitting beside it. An audit caught the contradiction. Only the 020 single-variable code-mode differential remains open. It needs a running Codex client, and until it runs the direct-is-not-a-reachability-fix claim in 004/094 stays source-derived. The correction is stated in place rather than silently rewritten, so the record shows the debt list was wrong and why. Verification: SHA256SUMS regenerated and fully verified; privacy:scan passed; 47 focused tests pass at this head.
…P3 gaps A second audit round found that my own "closures" were partly overstated. Fixed by doing the work, not by rewording it. Malformed-load warning (devlog 020, previously missing entirely). The schema degrades these fields with `.catch(undefined)` so a typo cannot cost a user their providers or credentials, but silent degradation is its own failure: an operator whose emergency escape hatch was dropped would never learn it is inactive — the #1529 observability failure at smaller scope. Adds `warnDegradedRoutedToolDiscoveryForLoad`, wired into both load paths beside the existing `retryOn429` sanitizer and following its conventions: runs before schema validation, redacts secret-shaped provider/model names, and logs only the received TYPE, never the value, since provider config can hold secrets. Backward compatibility, previously claimed but not proven. The old tests called `validateConfigCandidate()`, which cannot demonstrate a file-level promise. They now drive the real on-disk round trip through `loadConfig`/`saveConfig` against a temp `OPENCODEX_HOME`: - a pre-field config gains no persisted fields on read, and an unrelated save does not introduce them; - a config carrying both fields survives an unrelated save, which is the `.passthrough()` downgrade contract. Ablation: with the fields removed from the schema AND `.passthrough()` switched to `.strict()`, the downgrade test fails. It is not vacuous. Combo alias coverage (devlog 025), which the audit correctly refused to let me defer since it sits inside WP3's declared combo scope. A direct policy is now asserted through every alias shape — default `combo/<id>`, bare, slashed, and explicit native alias — because the alias is what Codex sees, and a shape-dependent policy is exactly the defect class the unified Cursor fence already had to fix once. 029 exit gate corrected. It had been edited to claim the `020` differential was the only open item. That was false: the `023` prior-build comparison and the `025` forcing-member diagnostic remain open too. The retracted claim is left visible in the note on purpose — this unit exists because a plan asserted more verification than it had, and quietly editing that away would repeat it. Verification: bun x tsc --noEmit clean; 427 pass / 0 fail across 9 suites (49 in the focused file); privacy:scan passed; SHA256SUMS regenerated and verified.
The audit caught that my alias tests could not fail. They re-attached `alias`
onto the derived model before serializing, and fell back to `rows[0]` when the
expected slug was missing — so deleting alias propagation from
`deriveComboCatalogModel()` left them green. That is the same false-confidence
shape as the gather test this work-phase already replaced once.
Now they build rows from the derived object only, assert `combo.alias` directly,
and require a row at the EXACT expected slug with no fallback. Ablation:
removing `...(combo.alias ? { alias: combo.alias } : {})` from
`deriveComboCatalogModel()` turns both alias tests red.
Also qualifies the last two categorical statements of the reachability
conclusion. The public docs were already labeled source-derived, but
`structure/03` and the `tool-discovery.ts` header still asserted it flatly,
contradicting `029`'s own record that devlog `020` still owes the executed
single-variable differential. Both now say the conclusion comes from reading the
upstream source and is well-grounded but unproven.
`029` no longer claims alias coverage without qualification: it states the
shapes are pinned to exact emitted slugs so deleting alias propagation turns
them red.
Verification: bun x tsc --noEmit clean; 427 pass / 0 fail across 9 suites;
privacy:scan passed; alias mutant confirmed red; SHA256SUMS regenerated.
Final audit sweep found a real functional gap behind the warning added last
commit. `modelRoutedToolDiscovery` keys go through `z.string().min(1)`, so a
hand-edited `{"": "direct"}` fails the key check and takes the ENTIRE map with
it — but the warning helper only inspected values, so the map vanished without a
word. The write boundary already rejected it; only the tolerant load path was
silent. Reproduced, then fixed with its own warning and a disk-load test that
asserts the provider and credential survive, the map is dropped, and the warning
names the blank key. Ablation: removing the blank-key branch turns the test red.
Wording sweep, all from the same audit:
- `tool-discovery.ts` opening sentence still stated the reachability conclusion
categorically while its own later paragraph called it unproven. Now "appears
not to decide".
- `000_master_plan.md` and `094_landing_verification_pass.md` led with the
categorical claim; both now say a source reading indicates it and name the
differential `020` still owes, matching `029`.
- `029` no longer implies the malformed-load warning is fully closed without the
blank-key case; it names it.
- Renamed "emits byte-identical routed rows with zero configuration" to "emits
identical rows whether the deferred default is implicit or explicit". The test
compares two outputs of the SAME build, so the old name implied the
prior-build comparison that `029` correctly records as still open.
Verification: bun x tsc --noEmit clean; 428 pass / 0 fail across 9 suites (50 in
the focused file); privacy:scan passed; blank-key mutant confirmed red;
SHA256SUMS regenerated.
The blank-key warning added last commit could lie. The load schema used
`z.string().min(1)`, which accepts a whitespace-only key, while the write
boundary and the warning helper both treat `" "` as blank via `trim()`. So a
config carrying `{" ": "direct"}` printed "ignoring the whole map" and then
kept the map — a false warning, worse than no warning, because it tells an
operator their override was dropped when it is still live.
The load path now uses the same nonblank rule as the write boundary, so the
three agree. The regression test switched from `""` to `" "` precisely because
the empty-string case passed under both rules and could not catch this.
Ablation: reverting the refine to `.min(1)` turns the test red.
Verification: bun x tsc --noEmit clean; 428 pass / 0 fail across 9 suites;
privacy:scan passed; SHA256SUMS regenerated.
Three stale claims left behind by the previous two commits. Both the helper comment and the test comment still said a blank key fails `z.string().min(1)`. That became false the moment I changed the rule: `.min(1)` is precisely the mutant that ACCEPTS a whitespace-only key and made the warning lie. They now name the trimmed-nonblank refinement, and the test comment records why the fixture is whitespace-only rather than empty — an empty string passes under both rules and so cannot catch the load/write mismatch. `000_master_plan.md` still claimed default behavior is "byte-for-byte equivalent" to #1596 while `029` correctly records the prior-build normalized comparison as open. It now says equivalence is intended and asserted per-key on both construction paths, and points at `029` for what remains unproven. Verification: bun x tsc --noEmit clean; 399 pass / 0 fail across the config and catalog suites; SHA256SUMS regenerated.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the hard-coded Cursor/non-Cursor boolean behind routed
supports_search_toolwith a resolved, route-scoped policy, and closes the unresolved P2 review finding from #1596.Defaults do not move. With no configuration, non-Cursor routed rows stay deferred (
supports_search_tool: true+web_search_tool_type: "text_and_image") and Cursor stays direct, exactly as #1596 shipped. Every pre-existing catalog test passes unmodified — that is the proof, not an assertion.Two optional provider settings:
Precedence is Cursor hard fence > model override > provider override > auto.
autoresolves to deferred for non-Cursor rows and never reaches serialization:CatalogModel.toolDiscoveryModecarries only the resolved value, resolved once inapplyProviderConfigHints()so configured, live-discovered, cached and combo-derived rows all agree. Combos resolve conservatively — one direct member forces the combo direct, since a single public row cannot vary after target selection — and the field is emitted only when it departs from the default. Both the explicit fingerprint and the provider-graph identity see the new fields, so two different policies cannot share a stale gather.The defect this closes
parsing.tsfenced Cursor onslug.startsWith("cursor/")whilesync.tsfenced onmodel?.provider === "cursor". Acursor/-aliased combo whose canonical provider iscombowas therefore classified differently depending on whether a template happened to be available, making discovery mode and payload size depend on template availability. Both paths now share oneisCursorRoute()helper: provider identity decides, and the slug prefix is only a fallback for callers with noCatalogModel.What
directis notUnder code mode Codex installs nested MCP tools on the
tools/ALL_TOOLSglobals in both exposures, so for an eligible tooldirectbuys no reachability. It moves full schemas intoexec.descriptionat the measured 2.7x turn-1 cost and changestool_searchconstruction, and it cannot repair a tool removed bydirect_only_tool_namespaces,excluded_tool_namespaces, or MCP/App policy filtering. The types, docs andstructure/03all say so, so nobody reaches for this expecting a reachability fix. Full analysis with upstream citations is in the parent PR's devlog unit.Config admission
The load path degrades a malformed value with
.catch(undefined)so a typo cannot cost a user their providers or credentials; the write boundary rejects it outright with a path-specificschema_invalid: providers.<name>.<field>: must be auto, deferred, or direct. The validator reads only own data properties throughObject.getOwnPropertyDescriptor, so an accessor- or prototype-polluted candidate is rejected without ever invoking the getter — covered by a test that asserts the getter call count stays at zero.Also corrects
structure/03: Cursor emitssupports_search_tool: falseand omitsweb_search_tool_type; it does not "advertise neither flag" (CodeRabbit's unresolved note on #1596).Verification
Full
bun run typecheckandbun run testwere run on a Linux x86_64 host, matching CI.bun x tsc --noEmit— clean.codex-tool-discovery-mode,catalog-cursor-search,codex-catalog,parallel-tool-calls-optin,e2e-style/phase100-native-parity,config,config-user-edits.tests/codex-tool-discovery-mode.test.tsis new — 25 cases covering the full precedence matrix, the Cursor hard fence by name and by adapter, thecursor/-aliased combo case,modelRecordValuematching including the deliberate dated-variant miss, sibling isolation on a mixed gateway, combo derivation, and config admission including the accessor and prototype-pollution cases.bun run privacy:scan— passed.One pre-existing test caught a real mistake during development: an unconditional
toolDiscoveryModekey brokecodex-catalog's combo-shape assertion. Making the field conditional matches theparallelToolCallsprecedent and keeps default rows byte-identical — exactly the regression fence that test exists to be.Checklist
Security-relevant surface: this PR touches config admission. The write-boundary validator is deliberately descriptor-first so a hostile config candidate cannot execute a getter during validation, and the load path degrades rather than discarding a provider's credentials. No secrets, tokens, or request bodies are logged;
privacy:scanis green.Stacked PR. Based on
codex/routed-tool-discovery-devlog(#1606), which carries the plan and verification record.enforce-targetexempts children whose base is an open PR's head branch. Retarget todevonce #1606 lands.