fix(catalog,bridge): write every slug once and scope declared-tool membership by wire (#4736, #4735) - #4799
Merged
Conversation
One 2.56.0 sync produced 507 catalog rows for 72 unique slugs: the model-alias slug (e.g. CC-x) and the canonical routed slug (command-code/x) of the same provider model both survive mergeCatalogEntriesFromObservedState, because slugEquivalenceKey treats a slash-less alias as an exact key and never unifies it with the routed key. Every duplicate row was byte-identical, so the guard keeps the first occurrence and leaves distinct slugs untouched. - dedupeCatalogEntriesBySlug runs on the merged list before the write and warns with the dropped count when it had to act - inert for catalogs that are already unique; order preserved; rows without a string slug pass through Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
#4730 Review follow-up on #4736: cover the write guard through the real syncCatalogModels path and surface content divergence instead of a silent first-win. - integration test runs the actual sync twice in isolated CODEX_HOME/OPENCODEX_HOME with the reported config shape (provider alias CC + modelAliases mappings) and asserts the written catalog has unique slugs, keeps the routed rows, and is idempotent - the dedup warning now names slugs whose dropped row differed from the kept row (safe-labeled, capped at 5) so disagreeing emit paths are visible instead of silently losing content Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
CodeRabbit follow-up: the integration fixture omitted the OpenAI forward provider, so includeNativeOpenAi was false and the merge dropped every slash-less baseline row before the write guard ran. With the forward surface present, the CC- alias row and its command-code/ canonical twin both survive — once each — and the test now proves it on both passes. Idempotence is asserted on the slug sequence: native row bodies refresh between passes, so full-row equality is not the invariant this suite owns. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
The dedupe ran before `clampCatalogModelsToCodexSupport`, which splices whole rows out of the array (src/codex/catalog/effort.ts:490) when an exact-reserve ladder clamps empty. With divergent duplicates that ordering can erase a slug entirely: first-win drops the row the clamp would have kept, then the clamp removes the one that survived. Moving the guard after the clamp and after `finalizeAutoReviewModelOverride` makes it the last mutation before serialization, so uniqueness holds for the exact bytes written. The divergence report also compared each row against the LAST occurrence of its slug, because the Map constructor keeps the last duplicate key, while the guard keeps the first. Build the baseline first-win so the reported divergence is measured against the row that actually lands on disk. Co-authored-by: rrmlima <rrmlima@users.noreply.github.com>
createCodexCatalogFixture wrote the POSIX launcher without the executable bit and had no Windows branch, so CODEX_CLI_PATH pointed at something that cannot run. The bundled-catalog loader catches the spawn failure and tries the next candidate, which means the test would go green while reading whatever Codex binary the host happens to have installed. Match the established sibling (tests/codex-integration/codex-catalog-sync-hardening.test.ts): chmod 0755 on POSIX, .cmd wrapper on Windows. Also drops the two unused node:fs imports. Co-authored-by: rrmlima <rrmlima@users.noreply.github.com>
AGENTS.md requires a new test file in scripts/test-layout/layout.json and tests/fixtures/test-layout-expected.json. The regex seed for catalog-* already resolves both files to codex-integration, so the guard was green without this, but the explicit table and its fixture oracle are what keep a renamed or moved file honest. Both maps stay byte-identical, which is what the membership oracle asserts. Co-authored-by: rrmlima <rrmlima@users.noreply.github.com>
…ries #4736 guarded \`writeRetainedCatalogSync\` only. \`buildConvergedCatalog\` (src/codex/convergence.ts) runs the same \`mergeCatalogEntriesFromObservedState\` and the same \`finalizeAutoReviewModelOverride\`, then commits through \`fixedCommit\` -> \`replaceActiveCodexCatalog\`. Every dashboard model toggle, combo edit, and Codex account login reaches that writer via \`convergeCodexCatalog\` and never touches the sync path, so the same \`source-invalid\` rejection stayed reachable through the management API. Move the guard into ./aggregation so both writers apply one rule, and keep the convergence call silent because that merge already runs under \`warningPolicy: "suppress"\`. The guard returns the input array untouched when the catalog is already unique, so the byte-identical no-op write optimisation in retained-sync is unaffected. Also drops the producer claim from the doc comment. \`modelAliases\` does not emit a second row: src/codex/catalog/routed-gather.ts builds \`aliasDisplayNames\` and stamps \`displayName\`, while the slug stays \`routedSlug(provider, id)\`, so the aliased/canonical pair the comment described is not something the merge can produce. The reported 507-rows-for-72-slugs catalog is real; the named cause is not, and a guard that only covered a guessed producer would leave the file corruptible by the next one. Stated as a write-boundary invariant instead. Co-authored-by: rrmlima <rrmlima@users.noreply.github.com>
…d endpoint regression tests Co-authored-by: Rafael Moreira <rrmlima@gmail.com>
…rmalization #4735 disabled the undeclared-tool guard on the chat and Anthropic wires by passing \`declaredToolNames: undefined\`. That set is not only the authorization boundary — it is also the catalog \`normalizeDeclaredToolName\` and \`declaresCodeModeExec\` read (src/types/tools.ts:84,138). Both return the input unchanged when it is undefined, so on those two wires a provider that invents \`default.lookup\` for a declared \`lookup\` would have reached the client under the invented name, and code-mode helper rewriting (#4176, #4412) would have stopped. That is a new failure on the happy path, not the removal of one. Carry the fix as an explicit \`enforceDeclaredToolNames\` flag instead. The set keeps flowing to the bridge on every wire; only the membership refusal is scoped. Chat and Anthropic relay the call and leave execution or refusal to the client runner, which is what those specs require; \`responses\` keeps failing closed exactly as #1700 left it. Also drops the \`declaredToolNames.size > 0\` condition the PR added. An explicitly empty catalog is a statement that no client tool may be called, which is how the passthrough guard already reads it (\`clientExplicitWireToolCatalog\`, src/server/responses/passthrough-dispatch.ts:441); making it mean "unrestricted" would have loosened the Responses wire too, well outside the reported failure. The passthrough scoping is reverted to today's behaviour and left out of this release. \`undeclaredToolGuardActive\` gates \`normalizeDefaultNamespaceInResponse\` and the continuation-state suppression as well as the 502 (src/server/responses/passthrough-dispatch.ts:500,546,557), so the same conflation applies there — and the reported failure is on the bridged path: a Command Code \`openai-chat\` provider never reaches a passthrough adapter. Fixing it needs the same split plus coverage, with no user report behind it yet. Co-authored-by: rrmlima <rrmlima@users.noreply.github.com>
…bership Scoping the membership refusal off the chat and Anthropic wires reverses part of #1700, so it is written down rather than left for the next reader to infer from a ternary. Both owning docs get the contract in the present tense. transports/responses.md states the whole rule: the declared set reaches the bridge on every wire because it is also the catalog normalizeDeclaredToolName and declaresCodeModeExec read, enforcement is the separate enforceDeclaredToolNames flag, only the responses wire enforces, an explicitly empty catalog still authorizes nothing where enforcement applies, and the passthrough guard is not wire-scoped because the same flag gates namespace normalization and continuation-state suppression there. adapters/compatibility-contracts.md states the consequence for a manifest: the same provider, base URL, adapter and auth mode answer an identical undeclared call differently per inbound protocol, so a tool-call disposition names its inbound protocol. That is the narrow-subject rule that doc already holds. Neither doc claims the duplicate-slug producer in #4730 is known, and nothing here closes an issue.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe change adds protocol-specific undeclared-tool enforcement and preserves tool-name normalization. It also deduplicates catalog rows by slug during synchronization and convergence, with regression tests for both behaviors. ChangesInbound tool enforcement
Catalog slug uniqueness
Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Client
participant ResponseExecution
participant ResponseBridge
participant ToolRunner
Client->>ResponseExecution: send request with inbound wire
ResponseExecution->>ResponseBridge: pass declaredToolNames and enforcement flag
ResponseBridge->>ToolRunner: normalize tool name
alt Responses inbound wire
ResponseBridge-->>Client: return undeclared client tool failure
else Chat or Anthropic inbound wire
ResponseBridge->>Client: relay undeclared tool call
end
✨ 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 |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Appending them to tests/responses/chat-completions-endpoint.test.ts pushed it to 3767 lines against its 3646 cap in tests/fixtures/file-size-baseline.json, and the ratchet only lowers a cap, so the number cannot be raised and the file cannot be exempted (scripts/file-size-ratchet.ts: GREW is an offender, SHRANK is not). The endpoint file returns to 3603. The two cases move verbatim, with the mock upstream, the isolated CODEX_HOME setup and the mock provider config they need, so the new file stands alone at 232 lines and is NEW_OK rather than a new baseline entry. Registered in scripts/test-layout/layout.json explicit and tests/fixtures/test-layout-expected.json; both maps stay byte-identical, which is what the membership oracle asserts. Co-authored-by: rrmlima <rrmlima@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This was referenced Sep 16, 2026
![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: e80d2edb33
ℹ️ 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".
| // in an isolated CODEX_HOME/OPENCODEX_HOME with the reporter's config shape: provider | ||
| // `alias: "CC"` plus `modelAliases` mappings. | ||
|
|
||
| const repoRoot = dirname(fileURLToPath(new URL("../../package.json", import.meta.url))); |
There was a problem hiding this comment.
Replace this location-dependent ../../package.json calculation with repoRoot() from tests/helpers/repo-root.ts. This integration test launches repository source in a child process, so moving it deeper during another test-layout reorganization would silently select the wrong working directory and break the regression; the repository explicitly requires source-oracle tests to use the shared root resolver for this reason.
AGENTS.md reference: AGENTS.md:L20-L23
Useful? React with 👍 / 👎.
Comment on lines
+467
to
+470
| The `chat` and `anthropic` inbound wires relay the call instead. This is a deliberate reversal of | ||
| #1700's scope for those two wires, not an oversight. Both vendor specs make the client's own runner | ||
| responsible for validating a tool call and then executing or denying it, and harnesses on those | ||
| endpoints defer part of their catalog to conserve prompt tokens and discover the rest at runtime. |
There was a problem hiding this comment.
Add this user-visible protocol change to docs-site/, not only to the maintainer-facing structure documents. Chat Completions and Claude Messages clients now receive undeclared calls that previously terminated with a 502, so users need the public adapter/API reference to explain the different enforcement semantics; otherwise the shipped behavior changes without corresponding user documentation.
AGENTS.md reference: AGENTS.md:L380-L381
Useful? React with 👍 / 👎.
Closed
2 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
2 participants
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
Two catalog and tool-guard failures users hit today, carried from #4736 and #4735 and reshaped where the original mechanism introduced a new failure.
Catalog slug uniqueness (#4736)
The duplicate-slug corruption is real, but the stated cause does not hold: routed-gather builds aliasDisplayNames and stamps displayName only, so modelAliases never becomes a second row's slug. The producer of the reported 507 rows for 72 unique slugs is still unidentified, so this carries the change as a write-boundary invariant rather than a repair of a guessed cause, and does not close the reporting issue.
Two defects mattered more than the wrong comment. The guard ran before clampCatalogModelsToCodexSupport, which splices whole rows out, so first-win could drop the row the clamp would have kept and then lose the slug entirely. And it covered only writeRetainedCatalogSync, while convergence.ts runs the same merge and finalize and commits through replaceActiveCodexCatalog, which is the writer every dashboard model toggle, combo edit and Codex account login reaches through convergeCodexCatalog. The same source-invalid rejection therefore stayed reachable through the management API. The guard now lives in aggregation.ts so both writers apply one rule, silent on the convergence path because that merge already runs under warningPolicy suppress, and it returns the input array untouched when the catalog is already unique so the byte-identical no-op write optimization is unaffected.
Declared-tool membership by inbound wire (#4735)
Providers in the muse family echo a namespaced tool by a spelling the guard does not hold, and the fail-closed refusal kills the stream mid-turn so the client re-sends the whole turn with the context re-billed. That failure is real.
The original fix disabled the guard by passing declaredToolNames undefined, and that set is not only the authorization boundary: it is also the catalog normalizeDeclaredToolName and declaresCodeModeExec read, both of which return their input unchanged when it is undefined. On chat and Anthropic a provider that invented default.lookup for a declared lookup would then reach the client under the invented name, and code-mode helper rewriting would stop. This carries it as an explicit enforceDeclaredToolNames flag instead, so the set keeps flowing on every wire and only the membership refusal is scoped. The original size > 0 condition is dropped: an explicitly empty catalog is a statement that no client tool may be called, which is how the passthrough guard already reads it, and making it mean unrestricted would have loosened the Responses wire too. The passthrough half is reverted to current behaviour.
Closes #4730 is deliberately absent: the producer is not identified.
Verification
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests