feat(config): bind catalog and route resolution - #5766
Conversation
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. |
|
Claude finished @Hmbown's task in 2m 43s —— View job Review: feat(config): bind catalog and route resolution
SummaryThis is a clean, well-scoped Phase 1 addition: Correctness finding
let receipt = if request.base_url_override.is_some() {
CatalogOfferingReceipt::CustomEndpoint { .. }
} ...The resolver does not treat every This isn't a hypothetical: Suggested fix: reuse the resolver's own notion of "custom" instead of re-deriving it. let receipt = if request.base_url_override.as_deref().is_some_and(|url| {
crate::provider_preserves_custom_base_url_model(candidate.provider_kind(), url)
}) { ... }(or better, have the resolver surface its Minor / test coverage
Provider/route safety (EPIC #2608)No prefix-based provider/model inference introduced. Note on verificationI was not able to execute |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 860f5228d3
ℹ️ 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".
| /// produce an executable candidate. | ||
| pub fn resolve(&self, request: &RouteRequest) -> Result<AuthorityResolution, RouteError> { | ||
| let candidate = self.resolver.resolve(request)?; | ||
| let receipt = if request.base_url_override.is_some() { |
There was a problem hiding this comment.
Classify only genuinely custom endpoint overrides
When base_url_override names an official endpoint—such as https://api.deepseek.com/v1 or another accepted URL in the provider's official endpoint family—the resolver treats it as non-custom and retains the matched catalog facts, but this unconditional is_some() returns CustomEndpoint and hides the exact offering. Use the resolver's official/custom endpoint classification here as well so the receipt agrees with the candidate's provenance.
Useful? React with 👍 / 👎.
| /// Bind a compiled catalog to the resolver that consumes its offerings. | ||
| #[must_use] | ||
| pub fn new(catalog: CatalogSnapshot) -> Self { | ||
| let resolver = RouteResolver::from_offerings(catalog.to_offerings()); |
There was a problem hiding this comment.
Scope live offerings to the resolved endpoint
When a snapshot contains live rows for the same provider from multiple base-URL fingerprints—which ProviderCatalogCache explicitly supports—projecting every row into one resolver drops that endpoint scope. A request for the provider's default endpoint can consequently resolve using a row fetched from a different custom endpoint and inherit its endpoint key, limits, capabilities, and pricing; the authority should filter live offerings to the request's base-URL fingerprint (while retaining endpoint-independent layers) before resolution.
Useful? React with 👍 / 👎.
| /// Bind a compiled catalog to the resolver that consumes its offerings. | ||
| #[must_use] | ||
| pub fn new(catalog: CatalogSnapshot) -> Self { | ||
| let resolver = RouteResolver::from_offerings(catalog.to_offerings()); |
There was a problem hiding this comment.
Preserve curated transport offerings in the authority
When this authority is built from the normal bundled catalog snapshot, catalog.to_offerings() omits the curated route::bundled_offerings() seam that RouteResolver::new() deliberately gives precedence for transport facts. For example, the Models.dev-shaped DeepSeek Flash row is projected with endpoint_key = "chat" instead of its required Responses route, and the bundled asset has no OpenCode Zen provider rows at all, so migrating those routes to this advertised replacement either selects the wrong protocol or rejects supported Zen models. Merge the curated transport offerings with the catalog projection using the same precedence as the default resolver.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Codewhale review
PR #5766 adds RouteAuthoritySnapshot to pair a compiled CatalogSnapshot with its RouteResolver, plus provenance receipts for resolved routes. The core direct/custom/pass-through tests are present and the API is generally coherent.
Findings
- [WARNING] Deserialize bypasses RouteCatalogScope normalization (
crates/config/src/route/authority.rs:34)
RouteCatalogScope::new trims both fields, but the derived Deserialize implementation populates the private fields directly and does not call new. JSON containing whitespace such as {"provider":" deepseek ","base_url_fingerprint":" fingerprint-a "} will produce a key that does not compare equal to the same scope created via new, causing scope_status BTreeMap lookups to miss. If deserialization is part of the public contract, implement a custom deserializer or validate/normalize after deserialization; otherwise remove Deserialize. - [WARNING] Catalog receipt matching ignores endpoint_key (
crates/config/src/route/authority.rs:181)
resolve() determines the Catalog receipt by matching only provider and wire_model_id. If the compiled catalog can contain more than one offering for the same provider/wire pair with different endpoint_key values, the first match may not be the offering the resolver actually selected. This can make the receipt point at different limits/pricing/source than the executable candidate. Prefer using an effective-offering lookup/index from the resolver, or include endpoint_key in the comparison, and add a multi-endpoint test. - [INFO] Missing tests for public serde shape and status replacement (
crates/config/src/route/authority.rs:291)
CatalogOfferingReceipt is a public Serialize type with an internal tag, but there is no test asserting the kind strings ("catalog", "custom_endpoint", "not_cataloged") or that a later with_scope_status call overwrites an earlier value. These are low-cost tests for a public contract.
Assessment
The API is a reasonable phase 1 seam and the included tests cover the primary direct/custom/pass-through cases. Address the deserialization normalization and offering-lookup precision issues before exposing the receipt in production; otherwise it may report a coherent-looking but incorrect receipt.
Advisory review by Codewhale (codewhale review --pr 5766 --post, head 860f5228d364c58f92f2519d0c172b137c35517c). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| /// The base URL is represented only by its already-redacted fingerprint. The | ||
| /// provider remains an open catalog string because a catalog can include a | ||
| /// discoverable provider that is not yet a built-in [`crate::ProviderKind`]. | ||
| #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] |
There was a problem hiding this comment.
[WARNING] Deserialize bypasses RouteCatalogScope normalization
RouteCatalogScope::new trims both fields, but the derived Deserialize implementation populates the private fields directly and does not call new. JSON containing whitespace such as {"provider":" deepseek ","base_url_fingerprint":" fingerprint-a "} will produce a key that does not compare equal to the same scope created via new, causing scope_status BTreeMap lookups to miss. If deserialization is part of the public contract, implement a custom deserializer or validate/normalize after deserialization; otherwise remove Deserialize.
| provider: candidate.provider_id().clone(), | ||
| wire_model_id: candidate.wire_model_id().clone(), | ||
| } | ||
| } else if let Some(offering) = self.catalog.offerings.iter().find(|offering| { |
There was a problem hiding this comment.
[WARNING] Catalog receipt matching ignores endpoint_key
resolve() determines the Catalog receipt by matching only provider and wire_model_id. If the compiled catalog can contain more than one offering for the same provider/wire pair with different endpoint_key values, the first match may not be the offering the resolver actually selected. This can make the receipt point at different limits/pricing/source than the executable candidate. Prefer using an effective-offering lookup/index from the resolver, or include endpoint_key in the comparison, and add a multi-endpoint test.
| assert!(resolved.receipt().offering().is_none()); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
[INFO] Missing tests for public serde shape and status replacement
CatalogOfferingReceipt is a public Serialize type with an internal tag, but there is no test asserting the kind strings ("catalog", "custom_endpoint", "not_cataloged") or that a later with_scope_status call overwrites an earlier value. These are low-cost tests for a public contract.
A required per-PR check should assert a property of the change. Five of ours assert a property of the whole repository or of already-merged history, so a branch fails for debt it did not add and the fix is rebasing rather than editing code. That is how the queue got stuck, and it trains people to read a red check as noise. Demoted to advisory on `pull_request` only, still blocking on pushes to main: - Check dead-code budget (absolute #[allow(dead_code)] total) - Check runtime-contract budget - Check persistence-backlog budget - Check harvested contributor credit check-versions.sh keeps every tree-state check blocking everywhere. Only its two range audits -- check 12 (feature release-note receipts) and check 7 (contributor credit), both scanning previous-tag..HEAD -- become advisory, and only for the per-PR CI job, via a new --range-audit-advisory flag. Every release path still runs them blocking: release-candidate.yml, auto-tag.yml, release.yml, and prepare-release.sh. Combining the new flag with --require-dated-release is refused outright so publication can never skip them. Measured on the merge of origin/main + pr/5740, a tree with the real missing #5766 receipt: $ ./scripts/release/check-versions.sh # exit 1 ::error::Feature commit 79ed88f references #5766, but no release-note receipt exists in CHANGELOG.md docs/CHANGELOG_ARCHIVE.md. $ ./scripts/release/check-versions.sh --range-audit-advisory # exit 0 ::warning::Missing feature release-note receipt(s) above. Advisory here because this audits already-merged commits in v0.9.10..HEAD, not this change. Version state OK: workspace=0.9.11, npm=0.9.11, npm-binary=0.9.11. actionlint is clean on the edited workflow. Signed-off-by: CodeWhale Bot <bot@codewhale.net> Entire-Checkpoint: 01M1D74JXS2F91WDY9DSKQ787E
`scripts/release/check-feature-release-notes.sh` requires every issue-linked `feat:` commit in the release range to leave a durable changelog receipt. 79ed88f ("feat(config): bind catalog and route resolution (#5766)") landed without one, so `scripts/release/check-versions.sh` now exits 1 on plain main: $ git worktree add --detach wt origin/main # 6ea1003 $ ./scripts/release/check-versions.sh ::error::Feature commit 79ed88f references #5766, but no release-note receipt exists in CHANGELOG.md docs/CHANGELOG_ARCHIVE.md. exit=1 "Version drift" is a required status check, so this failed every pull request opened or re-run against current main, not just the one that surfaced it. The entry says plainly that #5766 is additive plumbing with no call-site or user-visible change, rather than inventing a user-facing feature to satisfy the gate. `crates/tui/CHANGELOG.md` is regenerated with `./scripts/sync-changelog.sh` so the slice check passes too. After this commit `./scripts/release/check-versions.sh` exits 0: Feature release-note receipts OK: 47 linked issue reference(s) checked. Version state OK: workspace=0.9.11, npm=0.9.11, npm-binary=0.9.11. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
A required per-PR check should assert a property of the change. Five of ours assert a property of the whole repository or of already-merged history, so a branch fails for debt it did not add and the fix is rebasing rather than editing code. That is how the queue got stuck, and it trains people to read a red check as noise. Demoted to advisory on `pull_request` only, still blocking on pushes to main: - Check dead-code budget (absolute #[allow(dead_code)] total) - Check runtime-contract budget - Check persistence-backlog budget - Check harvested contributor credit check-versions.sh keeps every tree-state check blocking everywhere. Only its two range audits -- check 12 (feature release-note receipts) and check 7 (contributor credit), both scanning previous-tag..HEAD -- become advisory, and only for the per-PR CI job, via a new --range-audit-advisory flag. Every release path still runs them blocking: release-candidate.yml, auto-tag.yml, release.yml, and prepare-release.sh. Combining the new flag with --require-dated-release is refused outright so publication can never skip them. Measured on the merge of origin/main + pr/5740, a tree with the real missing #5766 receipt: $ ./scripts/release/check-versions.sh # exit 1 ::error::Feature commit 79ed88f references #5766, but no release-note receipt exists in CHANGELOG.md docs/CHANGELOG_ARCHIVE.md. $ ./scripts/release/check-versions.sh --range-audit-advisory # exit 0 ::warning::Missing feature release-note receipt(s) above. Advisory here because this audits already-merged commits in v0.9.10..HEAD, not this change. Version state OK: workspace=0.9.11, npm=0.9.11, npm-binary=0.9.11. actionlint is clean on the edited workflow. Signed-off-by: CodeWhale Bot <bot@codewhale.net> Entire-Checkpoint: 01M1D74JXS2F91WDY9DSKQ787E
…wide audits failing innocent PRs (#5786) * docs(changelog): add the missing release-note receipt for #5766 `scripts/release/check-feature-release-notes.sh` requires every issue-linked `feat:` commit in the release range to leave a durable changelog receipt. 79ed88f ("feat(config): bind catalog and route resolution (#5766)") landed without one, so `scripts/release/check-versions.sh` now exits 1 on plain main: $ git worktree add --detach wt origin/main # 6ea1003 $ ./scripts/release/check-versions.sh ::error::Feature commit 79ed88f references #5766, but no release-note receipt exists in CHANGELOG.md docs/CHANGELOG_ARCHIVE.md. exit=1 "Version drift" is a required status check, so this failed every pull request opened or re-run against current main, not just the one that surfaced it. The entry says plainly that #5766 is additive plumbing with no call-site or user-visible change, rather than inventing a user-facing feature to satisfy the gate. `crates/tui/CHANGELOG.md` is regenerated with `./scripts/sync-changelog.sh` so the slice check passes too. After this commit `./scripts/release/check-versions.sh` exits 0: Feature release-note receipts OK: 47 linked issue reference(s) checked. Version state OK: workspace=0.9.11, npm=0.9.11, npm-binary=0.9.11. Signed-off-by: CodeWhale Bot <bot@codewhale.net> * ci: stop repo-wide and history-wide audits from failing innocent PRs A required per-PR check should assert a property of the change. Five of ours assert a property of the whole repository or of already-merged history, so a branch fails for debt it did not add and the fix is rebasing rather than editing code. That is how the queue got stuck, and it trains people to read a red check as noise. Demoted to advisory on `pull_request` only, still blocking on pushes to main: - Check dead-code budget (absolute #[allow(dead_code)] total) - Check runtime-contract budget - Check persistence-backlog budget - Check harvested contributor credit check-versions.sh keeps every tree-state check blocking everywhere. Only its two range audits -- check 12 (feature release-note receipts) and check 7 (contributor credit), both scanning previous-tag..HEAD -- become advisory, and only for the per-PR CI job, via a new --range-audit-advisory flag. Every release path still runs them blocking: release-candidate.yml, auto-tag.yml, release.yml, and prepare-release.sh. Combining the new flag with --require-dated-release is refused outright so publication can never skip them. Measured on the merge of origin/main + pr/5740, a tree with the real missing #5766 receipt: $ ./scripts/release/check-versions.sh # exit 1 ::error::Feature commit 79ed88f references #5766, but no release-note receipt exists in CHANGELOG.md docs/CHANGELOG_ARCHIVE.md. $ ./scripts/release/check-versions.sh --range-audit-advisory # exit 0 ::warning::Missing feature release-note receipt(s) above. Advisory here because this audits already-merged commits in v0.9.10..HEAD, not this change. Version state OK: workspace=0.9.11, npm=0.9.11, npm-binary=0.9.11. actionlint is clean on the edited workflow. Signed-off-by: CodeWhale Bot <bot@codewhale.net> Entire-Checkpoint: 01M1D74JXS2F91WDY9DSKQ787E * fix(tui): silence clippy::explicit_counter_loop in the startup mark rust 1.98's clippy added `explicit_counter_loop` coverage for this shape, and CI's `dtolnay/rust-toolchain@master` picked it up. `Lint` is a required check, so main is currently red on it and every pull request inherits the failure: error: the variable `x` is used as a loop counter --> crates/tui/src/tui/mark.rs:182:9 | 182 | for glyph in line.chars() { | help: consider using: `for (x, glyph) in (x0..).zip(line.chars())` = note: `-D clippy::explicit-counter-loop` implied by `-D warnings` error: could not compile `codewhale-tui` (lib) due to 1 previous error Applied clippy's own suggestion. Behaviour is identical: `x` still starts at `x0` and advances one cell per glyph, and the `x >= area.right()` break still stops the row at the viewport edge -- `zip` only advances as far as `line.chars()` yields, so the open-ended range cannot run away. Signed-off-by: CodeWhale Bot <bot@codewhale.net> Entire-Checkpoint: 01M1D7F82ZEN4JVYBN0W1YDPRX --------- Signed-off-by: CodeWhale Bot <bot@codewhale.net> Co-authored-by: CodeWhale Bot <bot@codewhale.net>
Summary
Closes #5755 (Phase 1 contract only). This does not claim provider readiness, credential validity, endpoint health, or UI/runtime migration.
Verification