Skip to content

fix(anthropic): preserve response model metadata - #1122

Draft
giulioleone097 wants to merge 7 commits into
lidge-jun:devfrom
giulioleone097:agent/anthropic-response-model-metadata
Draft

fix(anthropic): preserve response model metadata#1122
giulioleone097 wants to merge 7 commits into
lidge-jun:devfrom
giulioleone097:agent/anthropic-response-model-metadata

Conversation

@giulioleone097

@giulioleone097 giulioleone097 commented Aug 6, 2026

Copy link
Copy Markdown

Summary

  • Keep client-facing Responses model metadata on a safe canonical public selector across bridge, passthrough JSON/SSE, image, web-search, and virtual-model paths while preserving the physical model upstream and in request logs. Slash aliases are emitted only when they decode uniquely; unknown or colliding encodings retain the raw qualified selector. Canonical OpenAI forward responses normalize unscoped selectors to the native bare slug; internal combo child dispatches retain their existing physical response identity.
  • Derive Anthropic bare-ID compatibility rows from the effective per-model adapter, preserve canonical owned_by, and mark compatibility/combo lifecycle ownership with explicit opencodex_catalog_kind values. Duplicate bare IDs select one deterministic alias owner; combo aliases suppress shadowed compatibility rows; fresh builds and transient sync/convergence fallback deduplicate legacy rows consistently; restore removes generated rows deterministically.
  • Capture registry-only per-model wire defaults in catalog-gather authority before async discovery, so response mapping cannot reread mutable registry state.
  • Document the public-response/physical-log and catalog lifecycle invariants, and add direct regression coverage for SSE, image, web search, passthrough, legacy/colliding selectors, cross-provider routing, virtual models, combos, per-model adapters, convergence, restore, and gather authority.

Closes #1117

Verification

  • Bun 1.3.14 (the repository/CI-pinned runtime)
  • bun run typecheck
  • bun run privacy:scan
  • Rebased final head 31ce1226: 409 focused tests passed, 0 failed, 1,818 assertions across response identity, image/web-search bridges, passthrough, catalog sync/restore/convergence, immutable gather authority, DeepSeek wire selection, request logs, combo failover, and virtual-model transports.
  • Full local final-head run: 9,473 passed, 8 skipped, 2 shared-state isolation flakes. The exact failing WebSocket pool-refresh and native-profile sideband tests each passed 3/3 immediately afterward (6/6 total). Final cross-platform/sharded workflows are waiting for maintainer approval to run on this fork head (action_required, zero jobs started).

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.
  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes

    • Preserved the client-selected model name in streamed and JSON responses while routing requests to the correct upstream model.
    • Improved request logs to retain the physically routed model identity.
    • Fixed model identity handling for image, web-search, passthrough, virtual-model, and combined routing scenarios.
    • Improved catalog synchronization, restoration, deduplication, and compatibility alias cleanup.
    • Preserved explicit model ownership metadata and provider adapter settings.
  • Documentation

    • Documented routed model identity and compatibility alias behavior.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The change separates physical routed model identity from the client-facing response identifier. It adds hidden Anthropic compatibility aliases, stores _responseModelId during route normalization, rewrites Responses payloads, updates bridges, and preserves routed models in request logs.

Changes

Routed model identity

Layer / File(s) Summary
Catalog compatibility aliases
src/codex/catalog/..., tests/codex-catalog*.test.ts, tests/codex-convergence-contract.test.ts, structure/03_catalog-and-subagents.md
Anthropic routed models receive hidden bare-ID aliases with routed metadata. Sync, convergence, fallback, and restore logic manage alias lifecycle and deduplication.
Normalized response identity
src/types.ts, src/server/responses/core.ts, src/server/responses-model-rewrite.ts, src/server/request-log.ts, src/server/index.ts, structure/04_transports-and-sidecars.md
Route normalization stores _responseModelId. JSON and SSE payloads rewrite differing model fields. Request logs preserve the physical routed model when required.
Bridge propagation and validation
src/images/loop.ts, src/web-search/loop.ts, tests/response-model-identity.test.ts, tests/images/loop.test.ts, tests/web-search.test.ts, tests/openai-api-virtual-models.test.ts, tests/openai-provider-option-e2e.test.ts, tests/passthrough-abort.test.ts, tests/request-log.test.ts
Response bridges use the canonical response identifier. Tests cover JSON, SSE, WebSocket, passthrough, virtual, combo, image, web-search, and request-log behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CodexClient
  participant ResponsesCore
  participant RoutedProvider
  participant ResponseBridge
  participant RequestLog
  CodexClient->>ResponsesCore: Send routed model selector
  ResponsesCore->>RoutedProvider: Send physical model identifier
  RoutedProvider-->>ResponsesCore: Return response payload
  ResponsesCore->>ResponseBridge: Provide _responseModelId
  ResponseBridge-->>CodexClient: Emit canonical response model
  ResponsesCore->>RequestLog: Preserve physical resolvedModel
Loading

Possibly related PRs

Suggested reviewers: lidge-jun, wibias, ingwannu

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy issue #1117 by preserving provider-qualified responses, maintaining physical routing identities, and managing compatibility aliases through synchronization and restore.
Out of Scope Changes check ✅ Passed The implementation, tests, and documentation remain within the linked issue scope of response identity, routing metadata, and Anthropic catalog alias lifecycle handling.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: preserving response model metadata for Anthropic-routed requests.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

⏳ DRAFT

  • review readiness checklist open (1/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 1/4).

Review readiness checklist

  • ⬜ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

1/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Aug 6, 2026
@giulioleone097
giulioleone097 marked this pull request as ready for review August 6, 2026 10:40
@github-actions
github-actions Bot marked this pull request as draft August 6, 2026 10:41

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@tests/response-model-identity.test.ts`:
- Around line 54-68: Add focused Bun regression tests near the existing
response-model identity tests that activate both the image-loop and
web-search-loop sidecar bridge paths. For each path, assert the upstream request
uses the bare model ID and every client-facing Responses event reports the
provider-qualified selector, covering streaming behavior rather than only the
non-streaming openai-chat adapter path.
🪄 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: 7ac23ed6-d419-4926-bf19-2e3f8b261210

📥 Commits

Reviewing files that changed from the base of the PR and between bbd82e7 and 7217bba.

📒 Files selected for processing (10)
  • src/codex/catalog/sync.ts
  • src/images/loop.ts
  • src/server/request-log.ts
  • src/server/responses/core.ts
  • src/types.ts
  • src/web-search/loop.ts
  • tests/codex-catalog-restore.test.ts
  • tests/codex-catalog.test.ts
  • tests/request-log.test.ts
  • tests/response-model-identity.test.ts

Comment thread tests/response-model-identity.test.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7217bba887

ℹ️ 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 thread src/server/responses/core.ts Outdated
Comment thread src/codex/catalog/sync.ts Outdated
Comment thread src/codex/catalog/sync.ts
Comment thread src/codex/catalog/sync.ts Outdated
Comment thread src/server/responses/core.ts Outdated
@Ingwannu

Ingwannu commented Aug 6, 2026

Copy link
Copy Markdown
Owner

This is a valuable implementation and the request-log preservation is an improvement over the first maintainer draft. I would prefer to keep the contributor PR if the remaining contract gaps are closed.

Before marking it ready, please address these points:

  • The reported failure is the streaming Responses path, but response-model-identity.test.ts exercises only stream:false. Add the exact SSE reproduction and keep JSON coverage.
  • Add direct regression coverage for the image bridge and web-search loop. Both read the new field, but neither path is currently pinned by this PR.
  • ROUTED_CONTEXT_COMPAT_OWNER is stored in owned_by, replacing the canonical upstream owner on the cloned row. owned_by is semantic provider metadata elsewhere in the catalog. Use a dedicated nonsemantic OpenCodex marker such as opencodex_catalog_kind for generated-row ownership, and preserve the canonical owned_by value.
  • _responseModelId currently changes bare-selector response identity for every non-openai routed provider, not just Anthropic. Either narrow that behavior to the affected contract or add cross-provider/virtual-model/combo tests and document why the generic change is intended.
  • Record the response-identity, log-identity, and cleanup invariants in structure/ so the Codex-facing selector and physical logged model cannot drift independently later.

Please keep the PR in draft until those are covered and all CI/review findings are resolved. I am leaving #1121 open only as a comparison/reference while this contributor PR is updated; I will not merge either duplicate in the current state.

@giulioleone097

Copy link
Copy Markdown
Author

All requested contract gaps are addressed on edec5764:

  • Added exact JSON and SSE response-identity coverage, including legacy full-slash canonicalization.
  • Added direct image-loop and web-search-loop streaming regressions; upstream stays bare and every client Responses snapshot is provider-qualified.
  • Replaced the semantic owned_by overload with opencodex_catalog_kind = routed-context-compat-v1 plus opencodex_routed_slug, preserving the canonical owner.
  • Propagated adapter identity through catalog discovery/custom rows, so custom Anthropic-wire provider names receive the compatibility alias.
  • Preserved only target-backed aliases on transient discovery, excluded them from native convergence, removed them on restore, and proved repeated convergence leaves one hidden alias with no visible duplicate.
  • Kept the generic routed response identity intentionally and pinned cross-provider, virtual OpenAI API, passthrough JSON/SSE, image, web-search, and combo behavior. Upstream and request-log identity remain physical; the documented internal combo exception remains unchanged.
  • Recorded the response/log and catalog lifecycle invariants in structure/03_catalog-and-subagents.md and structure/04_transports-and-sidecars.md.

Verification on the rebased head: 370 focused tests, 0 failures, 1,666 assertions; typecheck and privacy scan pass. The full local suite produced 9,431 passes / 8 skips with one unrelated intermittent native-profile-drain-server failure, and that exact test passed 3/3 in isolation. I am keeping the PR draft until GitHub CI and fresh automated review are clean.

@codex review
@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@giulioleone097 I will review the current PR state, including the new regression coverage and documented identity and catalog lifecycle invariants.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/codex/catalog/provider-fetch.ts`:
- Line 568: Resolve the effective per-model adapter before populating catalog
metadata, rather than copying the provider adapter. In
src/codex/catalog/provider-fetch.ts lines 568 and 1297-1302, update the
CatalogModel adapter assignments to use the existing resolver with provider
metadata and the relevant model.id or cm.modelId, preserving provider adapters
as the fallback when no model override exists.

In `@src/codex/catalog/sync.ts`:
- Around line 386-396: Update the compatibility-alias generation around the
existing routed-slug deduplication in the sync flow to track emitted hidden
aliases separately by bare Anthropic physical ID. When multiple providers share
the same m.id, deterministically retain one compatibility row and its routed
target instead of emitting duplicate slug values; preserve the existing
resolveSlugAliasCollisions behavior for routed-public slugs and satisfy the
two-provider coverage in tests/codex-catalog.test.ts.

In `@src/codex/convergence.ts`:
- Around line 181-185: Update the empty-discovery fallback in the convergence
logic around nativeSlugs to retain compatibility aliases whose
opencodex_routed_slug targets a preserved routed row, while keeping direct alias
exclusion from nativeSlugs. Ensure retained aliases preserve their context and
compaction metadata, and add a regression test covering convergence with an
empty provider discovery result.
🪄 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: 3f1fb371-df4c-4fc4-b177-2f42de34b131

📥 Commits

Reviewing files that changed from the base of the PR and between 3c24629 and edec576.

📒 Files selected for processing (24)
  • src/codex/catalog/effort.ts
  • src/codex/catalog/parsing.ts
  • src/codex/catalog/provider-fetch.ts
  • src/codex/catalog/sync.ts
  • src/codex/convergence.ts
  • src/images/loop.ts
  • src/server/index.ts
  • src/server/request-log.ts
  • src/server/responses-model-rewrite.ts
  • src/server/responses/core.ts
  • src/types.ts
  • src/web-search/loop.ts
  • structure/03_catalog-and-subagents.md
  • structure/04_transports-and-sidecars.md
  • tests/codex-catalog-restore.test.ts
  • tests/codex-catalog.test.ts
  • tests/codex-convergence-contract.test.ts
  • tests/images/loop.test.ts
  • tests/openai-api-virtual-models.test.ts
  • tests/openai-provider-option-e2e.test.ts
  • tests/passthrough-abort.test.ts
  • tests/request-log.test.ts
  • tests/response-model-identity.test.ts
  • tests/web-search.test.ts

Comment thread src/codex/catalog/provider-fetch.ts Outdated
Comment thread src/codex/catalog/sync.ts
Comment thread src/codex/convergence.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: edec576496

ℹ️ 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 thread src/server/responses/core.ts Outdated
Comment thread src/codex/convergence.ts
Comment thread src/codex/catalog/sync.ts Outdated
Comment thread src/server/responses/core.ts Outdated
@giulioleone097

Copy link
Copy Markdown
Author

Follow-up review fixes are on 538b0285 and be85b99b: effective per-model adapter metadata, deterministic bare-alias ownership, target-backed empty-convergence retention, post-virtual physical log locking, and reversible-only slug canonicalization. All seven new threads have exact replies and are resolved. Final focused proof: 373 passed, 0 failed, 1,677 assertions; typecheck and privacy scan pass. The PR remains draft and CI workflows remain action_required with zero jobs until a maintainer approves fork execution.

@codex review
@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== Current revision and recent commits ==='
git rev-parse --short HEAD
git show --no-patch --oneline HEAD
git show --no-patch --oneline 538b0285 2>/dev/null || true
git show --no-patch --oneline be85b99b 2>/dev/null || true

echo '=== Files changed from PR base (merge-base with origin default branch if available) ==='
base_ref=""
for ref in origin/main origin/master main master; do
  if git rev-parse --verify -q "$ref" >/dev/null; then
    base_ref="$ref"
    break
  fi
done
if [ -n "$base_ref" ]; then
  merge_base="$(git merge-base "$base_ref" HEAD)"
  git diff --name-status "$merge_base"...HEAD
else
  git show --stat --oneline HEAD
fi

echo '=== Follow-up commit diffs ==='
for commit in 538b0285 be85b99b; do
  if git rev-parse --verify -q "$commit" >/dev/null; then
    echo "--- $commit ---"
    git diff --unified=50 "${commit}^" "$commit" -- \
      src/codex src/server src/types.ts tests structure || true
  fi
done

echo '=== Relevant symbol map ==='
for file in \
  src/codex/catalog/parsing.ts \
  src/codex/catalog/provider-fetch.ts \
  src/codex/catalog/sync.ts \
  src/codex/convergence.ts \
  src/server/request-log.ts \
  src/server/responses/core.ts \
  src/server/responses-model-rewrite.ts \
  tests/response-model-identity.test.ts \
  tests/codex-catalog.test.ts \
  tests/codex-convergence-contract.test.ts \
  tests/request-log.test.ts; do
  if [ -f "$file" ]; then
    echo "--- $file ---"
    ast-grep outline "$file" --items all 2>/dev/null || true
  fi
done

Length of output: 138021


🏁 Script executed:

#!/bin/bash
set -euo pipefail

show() {
  local file="$1" start="$2" end="$3"
  echo "=== ${file}:${start}-${end} ==="
  nl -ba "$file" | sed -n "${start},${end}p"
}

show src/codex/catalog/parsing.ts 88 140
show src/codex/catalog/sync.ts 314 424
show src/codex/catalog/sync.ts 460 675
show src/codex/catalog/sync.ts 980 1110
show src/codex/convergence.ts 162 226
show src/codex/catalog/provider-fetch.ts 519 615
show src/server/adapter-resolve.ts 1 220
show src/providers/slug-codec.ts 1 260
show src/router.ts 1 260
show src/server/responses/core.ts 850 975
show src/providers/openai-virtual-models.ts 1 240
show src/server/request-log.ts 500 635
show src/server/request-log.ts 750 860

echo '=== All preservation/alias/adapter override references ==='
rg -n -C 4 \
  'preserveResolvedModelFromRoute|routedContextCompatTarget|isRoutedContextCompatEntry|ROUTED_CONTEXT_COMPAT_CATALOG_KIND|modelAdapters|resolveWireProtocolOverride' \
  src tests \
  -g '*.ts'

echo '=== Response identity test remainder ==='
nl -ba tests/response-model-identity.test.ts | sed -n '90,280p'

echo '=== Catalog alias and convergence test context ==='
nl -ba tests/codex-catalog.test.ts | sed -n '740,900p'
nl -ba tests/codex-convergence-contract.test.ts | sed -n '130,240p'

Length of output: 1645


🏁 Script executed:

#!/bin/bash
set -euo pipefail

show() {
  local file="$1" start="$2" end="$3"
  echo "=== ${file}:${start}-${end} ==="
  sed -n "${start},${end}p" "$file" | awk -v n="$start" '{printf "%6d\t%s\n", n++, $0}'
}

show src/codex/catalog/parsing.ts 88 140
show src/codex/catalog/sync.ts 314 424
show src/codex/catalog/sync.ts 460 675
show src/codex/catalog/sync.ts 980 1110
show src/codex/convergence.ts 162 226
show src/codex/catalog/provider-fetch.ts 519 615
show src/server/adapter-resolve.ts 1 220
show src/providers/slug-codec.ts 1 260
show src/router.ts 1 260
show src/server/responses/core.ts 850 975
show src/providers/openai-virtual-models.ts 1 240
show src/server/request-log.ts 500 635
show src/server/request-log.ts 750 860

echo '=== All preservation/alias/adapter override references ==='
rg -n -C 4 \
  'preserveResolvedModelFromRoute|routedContextCompatTarget|isRoutedContextCompatEntry|ROUTED_CONTEXT_COMPAT_CATALOG_KIND|modelAdapters|resolveWireProtocolOverride' \
  src tests \
  -g '*.ts'

echo '=== Response identity test remainder ==='
show tests/response-model-identity.test.ts 90 280

echo '=== Catalog alias and convergence test context ==='
show tests/codex-catalog.test.ts 740 900
show tests/codex-convergence-contract.test.ts 130 240

Length of output: 175307


@giulioleone097 I found one remaining catalog-sync issue.

src/codex/catalog/sync.ts Line 583 preserves empty-discovery compatibility aliases by target slug. It does not deduplicate by the public bare alias slug.

If the active catalog contains:

  • claude-sonnet-5a-anthropic/claude-sonnet-5
  • claude-sonnet-5z-anthropic/claude-sonnet-5

then both targets survive at Lines 585-592. The sync result contains two entries with slug: "claude-sonnet-5".

This conflicts with the invariant in structure/03_catalog-and-subagents.md: one hidden compatibility alias must exist for each bare model id. It also bypasses the deterministic winner logic in buildCatalogEntries() at Lines 336-406. Codex can then resolve the persisted bare selector to different context metadata based on catalog entry order.

Please deduplicate retained aliases by entry.slug. Select the target with the lexicographically smallest opencodex_routed_slug, which matches the fresh-build ownership rule. Add a regression test that starts with two target-backed aliases for the same bare slug, performs an empty-discovery sync, and asserts that exactly one alias remains with the a-anthropic/... target.

You are interacting with an AI system.

@giulioleone097

Copy link
Copy Markdown
Author

Addressed the remaining empty-discovery sync case in f121d324. Retained compatibility candidates are sorted by target then bare slug and deduplicated by both target and public slug, so legacy duplicate rows deterministically keep the lexicographically smallest routed target. The regression starts with two target-backed claude-sonnet-5 aliases plus an orphan and proves exactly the a-anthropic/... alias survives. Final-head proof: 373 passed, 0 failed, 1,678 assertions; typecheck and privacy scan pass.

@codex review
@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== Follow-up commit ==='
git show --no-patch --oneline f121d324
git diff --unified=35 'f121d324^' f121d324 -- \
  src/codex/catalog/sync.ts tests/codex-catalog.test.ts

echo '=== Relevant symbols ==='
ast-grep outline src/codex/catalog/sync.ts --items all --match 'buildCatalogEntries|sync|Catalog' || true
ast-grep outline tests/codex-catalog.test.ts --items all --match 'compat|sync|alias' || true

echo '=== Compatibility alias references with context ==='
rg -n -C 10 \
  'routedContextCompatTarget|isRoutedContextCompatEntry|retainedCompat|compatCandidates|ROUTED_CONTEXT_COMPAT_CATALOG_KIND' \
  src/codex/catalog/sync.ts tests/codex-catalog.test.ts

echo '=== Documentation invariant ==='
rg -n -C 5 'one hidden|compatibility alias|routed-context-compat' \
  structure/03_catalog-and-subagents.md

Length of output: 29770


@giulioleone097 The follow-up addresses the reported empty-discovery case.

src/codex/catalog/sync.ts Lines 583-600 now sorts retained compatibility aliases by routed target. It then deduplicates by both routed target and public bare slug. This retains a-anthropic/claude-sonnet-5 for duplicate claude-sonnet-5 aliases.

tests/codex-catalog.test.ts covers the required case. It includes two target-backed aliases, one orphan alias, and asserts that exactly one bare alias remains with the a-anthropic/... target.

This behavior matches the invariant in structure/03_catalog-and-subagents.md. I found no remaining issue in this follow-up.


If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@src/codex/convergence.ts`:
- Around line 205-211: Update the preservedCompat filter in the convergence
logic to deduplicate retained aliases by both routed target and slug, tracking
each accepted target alongside preservedAliasSlugs. Add a regression case
covering two distinct alias slugs referencing the same surviving
opencodex_routed_slug, ensuring convergence retains only one alias row.
🪄 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: 40558b1b-c37c-4f35-92b2-8eada959a0a5

📥 Commits

Reviewing files that changed from the base of the PR and between edec576 and f121d32.

📒 Files selected for processing (9)
  • src/codex/catalog/provider-fetch.ts
  • src/codex/catalog/sync.ts
  • src/codex/convergence.ts
  • src/server/responses/core.ts
  • structure/03_catalog-and-subagents.md
  • structure/04_transports-and-sidecars.md
  • tests/codex-catalog.test.ts
  • tests/codex-convergence-contract.test.ts
  • tests/response-model-identity.test.ts

Comment thread src/codex/convergence.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f121d324fc

ℹ️ 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 thread src/codex/convergence.ts Outdated
Comment thread src/codex/catalog/sync.ts
Comment thread src/codex/catalog/effort.ts
Comment thread src/server/responses/core.ts Outdated
@giulioleone097
giulioleone097 force-pushed the agent/anthropic-response-model-metadata branch from 23225b7 to 471356f Compare August 6, 2026 13:34

Copy link
Copy Markdown
Author

@codex review

Rebased on current dev at 471356f9; all previously valid inline findings are addressed with focused regression coverage.

Copy link
Copy Markdown
Author

@coderabbitai review

Please re-review rebased head 471356f9.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== Revision ==='
git rev-parse --short HEAD
git show --no-patch --format='%H%n%s%n%P' HEAD
git show --no-patch --oneline 471356f9 2>/dev/null || true

echo '=== Merge base and changed files ==='
base_ref=''
for ref in origin/dev dev origin/main main origin/master master; do
  if git rev-parse --verify -q "$ref" >/dev/null; then
    base_ref="$ref"
    break
  fi
done
if [ -n "$base_ref" ]; then
  merge_base="$(git merge-base "$base_ref" HEAD)"
  echo "base_ref=$base_ref"
  echo "merge_base=$merge_base"
  git diff --name-status "$merge_base"...HEAD
  echo '=== Diff stat ==='
  git diff --stat "$merge_base"...HEAD
else
  echo 'No base ref found; showing current commit stat.'
  git show --stat --oneline HEAD
fi

echo '=== Relevant symbol outlines ==='
for file in \
  src/codex/catalog/sync.ts \
  src/codex/catalog/parsing.ts \
  src/codex/convergence.ts \
  src/codex/catalog/provider-fetch.ts \
  src/server/responses/core.ts \
  src/server/responses-model-rewrite.ts \
  src/server/request-log.ts \
  src/images/loop.ts \
  src/web-search/loop.ts \
  src/types.ts \
  tests/response-model-identity.test.ts \
  tests/codex-catalog.test.ts \
  tests/codex-convergence-contract.test.ts \
  tests/request-log.test.ts \
  tests/images/loop.test.ts \
  tests/web-search.test.ts; do
  if [ -f "$file" ]; then
    echo "--- $file ---"
    ast-grep outline "$file" --items all 2>/dev/null || true
  fi
done

echo '=== Identity and compatibility-alias references ==='
rg -n -C 5 \
  '_responseModelId|preserveResolvedModelFromRoute|rewriteResponsesModelJson|createResponsesModelPayloadRewrite|isRoutedContextCompatEntry|routedContextCompatTarget|ROUTED_CONTEXT_COMPAT_CATALOG_KIND|opencodex_routed_slug' \
  src tests structure \
  -g '*.ts' -g '*.md'

Length of output: 128484


🏁 Script executed:

#!/bin/bash
set -euo pipefail

show() {
  local file="$1" start="$2" end="$3"
  echo "=== ${file}:${start}-${end} ==="
  nl -ba "$file" | sed -n "${start},${end}p"
}

echo '=== Rebased head commit diff ==='
git diff --unified=60 HEAD^ HEAD -- \
  src/codex/catalog/sync.ts \
  src/codex/catalog/parsing.ts \
  src/codex/convergence.ts \
  src/codex/catalog/provider-fetch.ts \
  src/server/responses/core.ts \
  src/server/responses-model-rewrite.ts \
  src/server/request-log.ts \
  src/images/loop.ts \
  src/web-search/loop.ts \
  src/types.ts \
  tests/response-model-identity.test.ts \
  tests/codex-catalog.test.ts \
  tests/codex-convergence-contract.test.ts \
  tests/request-log.test.ts \
  tests/images/loop.test.ts \
  tests/web-search.test.ts \
  structure/03_catalog-and-subagents.md \
  structure/04_transports-and-sidecars.md

echo '=== Final route normalization ==='
show src/server/responses/core.ts 850 950

echo '=== Responses model rewriter ==='
show src/server/responses-model-rewrite.ts 1 80

echo '=== Passthrough SSE and JSON paths ==='
show src/server/responses/core.ts 2020 2145
show src/server/responses/core.ts 2240 2320

echo '=== Bridge JSON and SSE paths ==='
show src/server/responses/core.ts 2550 2675
show src/server/responses/core.ts 3240 3375

echo '=== Catalog generation and empty-discovery retention ==='
show src/codex/catalog/sync.ts 319 425
show src/codex/catalog/sync.ts 570 625
show src/codex/convergence.ts 162 235

echo '=== Response identity test ==='
show tests/response-model-identity.test.ts 1 330

echo '=== Alias regressions ==='
show tests/codex-catalog.test.ts 780 945
show tests/codex-convergence-contract.test.ts 145 235

echo '=== Normalization call sites and direct upstream response returns ==='
rg -n -C 4 'applyFinalRouteRequestNormalization\s*\(|return upstreamResponse|new Response\(upstreamResponse\.body|rewriteResponsesModelJson|createResponsesModelPayloadRewrite' \
  src/server/responses/core.ts

Length of output: 120640


🏁 Script executed:

#!/bin/bash
set -euo pipefail

show() {
  local file="$1" start="$2" end="$3"
  echo "=== ${file}:${start}-${end} ==="
  sed -n "${start},${end}p" "$file" | awk -v line="$start" '{printf "%6d  %s\n", line++, $0}'
}

echo '=== Final route normalization ==='
show src/server/responses/core.ts 850 965

echo '=== Responses model rewriter ==='
show src/server/responses-model-rewrite.ts 1 80

echo '=== Passthrough SSE and JSON paths ==='
show src/server/responses/core.ts 2020 2160
show src/server/responses/core.ts 2240 2325

echo '=== Bridge JSON and SSE paths ==='
show src/server/responses/core.ts 2550 2675
show src/server/responses/core.ts 3240 3375

echo '=== Combo metadata producer and lifecycle consumers ==='
rg -n -C 8 'COMBO_CATALOG_KIND|opencodex_catalog_kind|isComboLifecycleEntry|deriveComboCatalogModel' \
  src/codex tests/codex-catalog.test.ts \
  -g '*.ts'

echo '=== Catalog generation and empty-discovery retention ==='
show src/codex/catalog/sync.ts 319 425
show src/codex/catalog/sync.ts 570 625
show src/codex/convergence.ts 162 235

echo '=== Full response-identity regression file ==='
show tests/response-model-identity.test.ts 1 340

echo '=== Image and web-search identity regression sections ==='
show tests/images/loop.test.ts 115 175
show tests/web-search.test.ts 390 470

echo '=== All normalization call sites and direct upstream returns ==='
rg -n -C 5 'applyFinalRouteRequestNormalization\s*\(|return upstreamResponse|new Response\(upstreamResponse\.body|rewriteResponsesModelJson|createResponsesModelPayloadRewrite' \
  src/server/responses/core.ts

Length of output: 121423


🏁 Script executed:

#!/bin/bash
set -euo pipefail

show() {
  local file="$1" start="$2" end="$3"
  echo "=== ${file}:${start}-${end} ==="
  sed -n "${start},${end}p" "$file" | awk -v line="$start" '{printf "%6d  %s\n", line++, $0}'
}

echo '=== Remaining convergence catalog assembly ==='
show src/codex/convergence.ts 175 260

echo '=== Combo marker and legacy-combo references in convergence ==='
rg -n -C 6 'combo-v1|COMBO_CATALOG_KIND|isComboCatalogEntry|Routed via opencodex → combo|isComboLifecycleEntry|exactComboSlugs' \
  src/codex/convergence.ts tests/codex-convergence-contract.test.ts \
  -g '*.ts'

echo '=== Convergence test setup and all catalog assertions ==='
show tests/codex-convergence-contract.test.ts 1 260

echo '=== Sync versus convergence native-row eligibility ==='
sed -n '496,508p' src/codex/catalog/sync.ts | awk -v line=496 '{printf "%6d  %s\n", line++, $0}'
sed -n '181,188p' src/codex/convergence.ts | awk -v line=181 '{printf "%6d  %s\n", line++, $0}'

echo '=== Build catalog behavior for supplied native slugs ==='
rg -n -C 4 'for \(const slug of gptSlugs\)|deriveEntry\(template, slug' src/codex/catalog/sync.ts

Length of output: 23046


@giulioleone097 I found one remaining issue.

src/codex/convergence.ts Line 184 promotes stale bare combo aliases to native catalog rows during convergence.

nativeSlugs excludes routed-context compatibility rows at Line 185. It does not exclude generated combo rows. If the active catalog contains this removed combo alias:

{
  "slug": "claude-sonnet-5",
  "opencodex_catalog_kind": "combo-v1"
}

then Lines 182-187 add claude-sonnet-5 to nativeSlugs. buildCatalogEntries() recreates it as a native entry at src/codex/catalog/sync.ts Lines 352-356. The recreated row has no combo marker because deriveEntry() removes generated-row markers at Lines 249-252.

The later filter at src/codex/convergence.ts Lines 232-240 only validates current exactComboSlugs. It does not remove this stale alias. The row therefore survives after the combo is deleted.

This differs from src/codex/catalog/sync.ts Lines 500-507, which excludes isComboLifecycleEntry() rows from native preservation. It also violates the documented combo cleanup invariant in structure/03_catalog-and-subagents.md.

Import a shared combo lifecycle predicate into src/codex/convergence.ts, or expose isComboLifecycleEntry() from src/codex/catalog/sync.ts. Exclude marked combo-v1 rows and legacy Routed via opencodex → combo ( rows when deriving nativeSlugs.

Add a convergence regression that starts with a stale bare combo alias, performs convergence after the combo is removed, and asserts that the alias is absent rather than recreated as a native row.

You are interacting with an AI system.

@giulioleone097
giulioleone097 force-pushed the agent/anthropic-response-model-metadata branch from 471356f to 31ce122 Compare August 6, 2026 13:52
@giulioleone097

Copy link
Copy Markdown
Author

Final rebased head 31ce1226 is current with dev. Focused proof: 409 passed, 0 failed, 1,818 assertions across 13 files; typecheck, privacy scan, and diff check pass; all 18 review threads are resolved. The PR remains draft while fork workflows await maintainer approval. @codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 31ce122644

ℹ️ 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 thread src/codex/catalog/sync.ts
Comment on lines +348 to +349
if (!current || slug.localeCompare(catalogModelSlug(current)) < 0) {
routedContextCompatWinnerByBareId.set(model.id, model);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Match compatibility aliases to bare routing precedence

When multiple enabled Anthropic providers expose the same bare model ID, this lexicographic winner can differ from the provider that actually handles that bare selector. For example, if anthropic-z precedes anthropic-a in config, routeByKnownModelPattern selects anthropic-z via Object.entries(...).find, but this code points the hidden alias at anthropic-a; persisted bare threads then receive context and compaction metadata for the wrong provider. Fresh evidence on the rebased head is that src/router.ts still resolves bare Claude IDs in config order while this comparison uses localeCompare. Select the alias using the router's precedence or suppress it when the bare selector is ambiguous.

Useful? React with 👍 / 👎.

Comment on lines +2296 to +2298
return parsed._responseModelId !== route.modelId
? rewriteResponsesModelJson(repaired, parsed._responseModelId!)
: repaired;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Rewrite successful JSON when Content-Type is absent

When an openai-responses passthrough gateway returns a successful non-streaming Responses JSON body without a Content-Type header, this new model rewrite is never reached because the enclosing branch requires application/json; execution instead falls through to the raw relay at line 2353. The client therefore still receives the bare physical model rather than the provider-qualified catalog selector. Treat a headerless successful non-streaming passthrough as bounded JSON, analogous to the existing headerless-SSE fallback, before applying this rewrite.

Useful? React with 👍 / 👎.

@lidge-jun

Copy link
Copy Markdown
Owner

Thank you for both the report and the implementation — this is landing as #1138, credited to you via Co-authored-by, and #1117 is closed by it.

Your mechanism is what I adopted, essentially as designed: _responseModelId on the parsed request, the 29-line responses-model-rewrite.ts helper, parsed._responseModelId ?? parsed.modelId in the image and web-search loops, and preserveResolvedModelFromRoute so request logs keep the physical routed model. That last detail is the one I would have missed — preserving observability of what was actually called, while the client sees the selector it asked for, is the correct split.

What I narrowed, and why. In #1122 _responseModelId is set for every routed provider. That changes response.model for any provider whose public and wire ids differ — which is a response-contract change rather than a bug fix, and it should not ride in on an Anthropic defect. In the stack the field is set only on the Anthropic route, so every other provider is byte-identical, pinned by a regression test that a non-Anthropic routed provider still emits exactly what it emitted before.

What I did not carry over: the catalog work (sync.ts, parsing.ts, provider-fetch.ts, effort.ts, convergence.ts), the hidden bare-selector compatibility rows, and the adapter-resolve.ts captured-default change. Generated catalog rows with restore/removal semantics have their own failure modes — user-owned row collision, restore deleting a generated row — and deserve a dedicated PR with that discussion, not a paragraph in a response-identity fix. If you want to pursue the bare-selector compatibility rows separately, that is a real gap and I would review it on its own terms.

That took the change from 25 files to 10.

Verified: 132 pass / 0 fail on the identity, image-loop, web-search and request-log suites; red-then-green confirmed (the four qualified Anthropic JSON/SSE cases failed before the fix while the non-Anthropic guard already passed); typecheck and privacy scan clean.

Leaving this PR open for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants