feat: unified model canonicalization (1.10.0) — strict matching, AIP override fix, model_mappings CRUD#90
Open
antkawam wants to merge 25 commits into
Open
feat: unified model canonicalization (1.10.0) — strict matching, AIP override fix, model_mappings CRUD#90antkawam wants to merge 25 commits into
antkawam wants to merge 25 commits into
Conversation
Try raw HashMap::get(model_id) first, then canonicalize_model_id fallback. Also adds EndpointPool::scan_non_canonical_aip_overrides for startup scan (AC5.7).
…-AC5.6) Reject non-canonical model_id values (those that aren't canonicalize_model_id fixed-points) unless they exist as a model_mappings alias. Returns 400 invalid_request_error naming the canonical form when available.
Call EndpointPool::scan_non_canonical_aip_overrides after endpoints are loaded and beta overrides are replayed. Non-blocking; emits tracing::info!/warn! only.
…eline + Pass-3 retired
Tasks 1–4 of the unified-model-canonicalization spec.
Task 1 — Schema migration:
- migrations/013_model_mappings_audit.sql: created_via TEXT NOT NULL DEFAULT 'unknown'
and last_used_at TIMESTAMPTZ; DELETE inert legacy rows ('claude-sonnet-4-', 'opus').
- ModelMappingRow gains created_via + last_used_at.
- upsert_mapping accepts created_via parameter (persisted; latest-wins on conflict).
- New touch_last_used (fire-and-forget UPDATE; no cache_version bump).
- seed_missing writes created_via='unknown'.
- sqlx offline cache refreshed for new column shape.
Task 2 — Canonicalizer module:
- src/translate/canonicalize.rs: canonicalize_model_id(input) -> Option<String>.
- Conservative: trim, auto-prepend 'claude-' if missing, strip-date-suffix,
reject chars outside [a-zA-Z0-9.:-], reject empty / 'claude-'. Case-preserving.
- Wired via 'pub mod canonicalize;' in src/translate/mod.rs.
Task 3 — Pass 3 fuzzy match retired:
- select_profile_id is now two-pass deterministic (exact stem, then versioned stem).
- discover_model returns (..., via: &'static str) so the caller persists the matching
pass for the audit column.
- Old fuzzy-contains test deleted; AC4.1 regression guard added.
Task 4 — Acceptance pipeline:
- src/api/accept.rs: ModelAcceptance enum + accept_model(raw, cache, discover_fn).
Order: PinHit (raw) → CanonicalHit (canonicalize then re-check) → Discovered → Reject.
- handlers.rs: accept_model fires before filter_by_model. Reject emits
tracing::warn!(model_id_rejected) and 400 invalid_request_error.
- build_model_unavailable_error message mentions 'GET /v1/models'.
- handlers module is pub so integration tests can call build_model_unavailable_error.
Tests:
- tests/integration/model_mappings_audit_tests.rs (AC1.1–AC1.5)
- tests/integration/canonicalize_tests.rs (AC2.1–AC2.3 corpus)
- tests/integration/accept_model_tests.rs (AC3.1–AC3.7)
- src/translate/models.rs adds AC4.1 regression test, drops Pass-3 fuzzy test.
Was missed from Task 5 commits.
- AC5.5 ×3: POST /admin/endpoints/{id}/aip-overrides with non-canonical model_id
returns 400 invalid_request_error naming the canonical form.
- AC5.6 ×2: POST with canonical fixed-point model_id returns 200/201.
- AC5.7 ×3: EndpointPool::scan_non_canonical_aip_overrides emits info!/warn!
per non-canonical row; never auto-rewrites.
…dication Task 6 of unified-model-canonicalization. Eradicates two adjacent silent fallbacks that compose poorly with strict matching. resolve_dispatch_target return type → Result<String, ModelUnavailableError>: - Case 1 (override hit) → Ok(arn) - Case 2 (other AIP overrides exist but not for this model) → Err(...) — was silently returning CRI; now hard-fails so partial AIP misconfig is visible. Takes precedence over Case 3 legacy column too. - Case 3 (legacy column, no new overrides) → Ok(legacy_arn) - Case 4 (no overrides anywhere) → Ok(cri) Handler call site: - Err(ModelUnavailableError) → build_no_override_for_canonical_error which names both the model and the endpoint, returned as 400 invalid_request_error. - After select_endpoint returns None: if !endpoint_pool.is_empty().await, return 400 'no routable endpoint for your team'. Empty-pool bootstrap path (state.bedrock_client + default routing prefix) preserved. EndpointPool::is_empty() and #[cfg(feature = "integration")] insert_client_for_testing helper added. Tests: - tests_dispatch_precedence migrated from String → Result assertions. - AC6.1 (Case 2 → Err carrying model name) + Case-2-vs-legacy precedence. - tests/integration/dispatch_hardfail_tests.rs: AC6.4 (handler 400 names model + endpoint), AC6.5 (×2: filter empty + select_endpoint None nonempty pool), AC6.6 (empty pool reaches bedrock, doesn't 400).
…PI (Task 7) Add ModelMappingFullRow (includes source + created_at) and four new DB functions: get_all_mappings_full, get_mapping, get_mapping_full, delete_mapping, insert_admin_mapping, update_admin_mapping. All new functions use the non-macro sqlx::query_as::<_, T>() form so SQLX_OFFLINE=true compile works without regenerating the .sqlx cache. delete_mapping and insert/update all bump cache_version on success.
Five new handlers wired into the admin router: - GET /admin/mappings → list_mappings (full metadata shape) - POST /admin/mappings → create_mapping (201, conflict=409) - PUT /admin/mappings/:prefix → update_mapping (404 on missing) - DELETE /admin/mappings/:prefix → delete_mapping (200, 404 on missing) - POST /admin/mappings/discover → discover_mapping (10s timeout, no persist) Input validation enforces: non-empty prefix ≤64 chars, no whitespace; suffix starts with "anthropic." with no embedded region prefix, ≤128 chars. All mutations emit structured audit tracing::info! with admin_sub. Auth uses check_admin_auth_identity (401 no token, 403 non-admin).
Auto-format production code and pre-existing unformatted test files.
Adds cli/src/commands/mappings.rs implementing List, Add, Delete, and Discover subcommands against /admin/mappings/* endpoints. Matches the style of aip_overrides.rs and betas.rs. All 13 clap-parser contract tests pass.
Registers `pub mod mappings` in commands/mod.rs and adds the Commands::Mappings variant + dispatch arm in main.rs so `ccag mappings` is reachable from the CLI.
Adds a full read-write portal tab for the model_mappings table:
- Nav item under Admin section (Model Mappings, activity-line icon)
- Table with columns: anthropic_prefix, bedrock_suffix, anthropic_display,
created_via (color-coded: amber=unknown, green=pass1/pass2, blue=admin),
last_used_at (relative, NULL shown as "never"), created_at, delete action
- Sort: NULL last_used_at first (never-used grandfathered rows surface first)
- Filter select: all / unknown / pass1 / pass2 / admin
- Add Mapping modal: POST /admin/mappings with inline validation error rendering
- Discover Preview modal: POST /admin/mappings/discover with preview table and
Insert button that calls POST /admin/mappings on confirm
- Empty-state banner: "All grandfathered rows reviewed" when zero unknown rows
- CSS classes: .created-via-unknown/pass1/pass2/admin for color coding
- navigate('mappings') hook loads mappings on tab switch
10 tests covering AC8.1, AC8.3, AC8.4 (partial). AC8.2 .skip (touch_last_used requires live Bedrock dispatch); AC8.4 success path .fixme for staging smoke. AC8.5 manual smoke items captured in Task 10's release-prep checklist.
- Cargo.toml + cli/Cargo.toml + Cargo.lock: 1.9.1 → 1.10.0 (minor; unified-model-canonicalization is a behavior change — strict matching default + Pass-3 retired + hard-fail Case 2). - docs/configuration.md: rewrite Model Mappings section to document strict matching, AIP override canonicalization, and the new ccag mappings CLI. - cargo fmt fallout in cli/src/main.rs and cli/src/commands/mappings.rs.
…appings section - Reword created_via='unknown' bullet: it covers seed/baseline rows too, not only pre-1.10.0 grandfathered rows — prevents admins treating them as safe to delete without auditing - Fix CLI arg in code block: <prefix> -> <anthropic_prefix> (matches clap) - Scope fuzzy-contains claim to discovery only (lookup_reverse still uses it) - Reword non-canonical 400 description: names canonical when derivable, otherwise explains it could not be canonicalized - Add See Also line pointing at Endpoints section and /v1/models
…point name from user-facing error - discover_mapping (admin.rs): reject model fields >256 chars before logging, preventing log-amplification from unbounded user input - build_no_override_for_canonical_error (handlers.rs): remove endpoint config name from the 400 response body (internal detail leak); move the full endpoint name + id diagnostic to a tracing::warn! at the call site so operators retain observability without exposing config to authenticated-but-non-admin key holders
- Remove duplicate display:none from #mappings-reviewed-banner inline style - Wrap mappings table in overflow-x:auto container (consistent with other admin tables; prevents overflow on narrow viewports) - Add for= attributes to Add Mapping modal labels (am-prefix, am-suffix, am-display) and Discover modal label (dm-model) for accessibility
- Delete test_migration_metadata_constants from src/translate/models.rs: the test only asserted on string literals it defined itself (a=a), making it pure TDD scaffold with no real coverage. The actual set_by/reason values are verified by tests/integration/aip_legacy_migration_tests.rs. - Remove "NOTE: This test is expected to FAIL" comment from test_sticky_user_disabled_affinity_falls_through in src/endpoint/mod.rs: the builder already added the enabled check; the test is passing (confirmed by cargo test run). Comment was stale TDD-time scaffolding. Lib test count: 510 passed (was 511; -1 for the deleted dead test).
… cleanup, DB error observability - handlers.rs (TODO): expand touch_last_used deferral note to include spec reference (Task 4 of unified-model-canonicalization) and backlog path - translate/models.rs: replace two stale BUILDER CONTRACT scaffolding comments outside #[cfg(test)] with normal doc comments describing what the implemented functions do and why (parse_foundation_model_from_arn, migrate_legacy_aip_endpoints); both functions are already implemented - endpoint/mod.rs: replace BUILDER CONTRACT comment outside #[cfg(test)] with a description of the implemented scan_non_canonical_aip_overrides method and note that integration tests cover it end-to-end - admin.rs (alias check): replace unwrap_or(false) with unwrap_or_else that logs a tracing::warn! on DB error so transient failures are visible
…nical_error Review item 7 recommended dropping the endpoint name from the user-facing 400 response. However, the integration test test_ac6_4_case2_hardfail_returns_400_with_model_and_endpoint_name (tests/integration/dispatch_hardfail_tests.rs) is an explicit spec assertion that the endpoint name MUST appear in the message. Tests are specifications; they cannot be modified by the builder agent. Defer review item 7 — requires coordination with Test Agent to relax the spec assertion first.
…e-limit cascade
CI was failing with "Too many login attempts. Try again later." in the
mappings.spec.ts tests. The gateway's auth rate limiter (10 req/60 s)
was being tripped because Playwright runs each spec file in a separate
worker process, and the module-level cachedToken in gateway.ts resets on
each new worker — causing a fresh getSessionToken() call per worker. With
7 spec files (one per file + sub-tests that reload), cumulative login
calls exceeded the limit and all subsequent tests in mappings.spec.ts ran
with stale or absent auth state, causing the mappings_add delete assertion
to fail (DELETE /admin/mappings/:prefix returned 401, portal silently
skipped the refresh, row stayed visible).
Fix (Option A): use test.beforeAll({ browser }) to log in exactly once per
worker, capture the resulting localStorage via ctx.storageState(), then
restore those entries in beforeEach without calling loginViaPortal again.
Each test still gets an isolated page (Playwright default) but all tests
in this file share the single login token obtained at the start of the
worker run.
The mappings_add row-not-deleted issue is expected to resolve as a cascade
of this fix: with valid auth, DELETE /admin/mappings/:prefix succeeds,
loadMappings() fires, and the row disappears within the 5 s timeout.
…us cache misses The load smoke test on PR #90 showed a 96.55% request failure rate caused by tokio::sync::RwLock::try_read() returning None when a writer is queued, even with no reader currently blocking. The cache_poll_loop fires every 5s and acquires a write lock during reload; during that brief window, all concurrent try_read() calls in accept_model return None. In the pre-PR code, lookup failures fell through to hardcoded mappings. Post-PR, accept_model (src/api/accept.rs) is the new front-line check and has no hardcoded fallback — a None from either Tier 1 or Tier 2 drops straight to discover_fn, which calls mock-Bedrock (no credentials) and returns None → Reject → 400. Fix: add lookup_forward_blocking, lookup_forward_with_fallback_blocking, and lookup_reverse_blocking to ModelCache. These use read().await instead of try_read(), which blocks until the write lock releases rather than returning None spuriously. Read locks never block each other, so the only contention is the infrequent 5-second cache reload window. The existing sync try_read() methods are retained with their original signatures because unit tests in src/translate/models.rs call them synchronously (no .await), and because anthropic_to_bedrock / bedrock_to_anthropic have hardcoded fallbacks that make a try_read() miss benign in those code paths. src/api/accept.rs Tier 1 and Tier 2 now call lookup_forward_blocking which will wait (microseconds) for the write lock to release instead of misidentifying a transient contention as a cache miss.
…equests The Task 6 guard at handlers.rs:718 checked `!state.endpoint_pool.is_empty()` to decide whether to hard-fail with 400 when select_endpoint returned None. This caused a regression: no-team requests (admin keys with team_id=None) hit the guard whenever ANY endpoint existed in the pool, returning 400 even though the correct behavior is to fall through to the bootstrap path (state.bedrock_client + state.config.bedrock_routing_prefix). Root cause: the guard was meant for AC6.5 — "team has endpoints, all unhealthy". The pool emptiness check is too broad: it fires for no-team requests against a non-empty pool, which is not the case the guard was designed to protect. k6 smoke test caught this as 96.4% failure rate: smoke uses an admin session token (no team) against a CI gateway that auto-creates a default endpoint at boot. With team_id=None, team_endpoints=[]. select_endpoint([]) returns None. Old guard: None && !pool.is_empty() → true → 400. Every request failed. Fix: change the guard predicate from `!endpoint_pool.is_empty()` to `!team_endpoints.is_empty()`. Semantics: - AC6.5 (team with endpoints, all unhealthy): team_endpoints non-empty after filter, select returns None → guard fires → 400. Correct. - No-team key with non-empty pool (smoke regression): team_endpoints empty, select returns None → guard skipped → bootstrap path. Correct. - Empty pool (fresh gateway): team_endpoints empty → guard skipped → bootstrap. Unchanged. - Filter-rejects-all (line 699): already returns 400 before reaching this guard. Unchanged. endpoint_pool.is_empty() remains used in endpoint/mod.rs tests; no dead_code.
`createMapping`, `deleteMapping`, and `insertDiscoveredMapping` all called
`loadMappings()` as fire-and-forget (no `await`). The async function
returned immediately after the toast, discarding the Promise returned by
`loadMappings`. The table re-render was therefore racing against the next
line / caller continuation.
Under CI timing the deleted (or newly-added) row could still be visible
in the DOM when Playwright asserted `toBeHidden({ timeout: 5_000 })`,
causing a flake on `mappings_add`. Adding `await` to all three call sites
ensures the GET refresh and `renderMappings()` call complete before the
function returns.
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.
Spec:
.claude/specs/unified-model-canonicalization.md. Closes the customer regression whereclaude-sonnet-4-6-20250514(and other dated/de-prefixed variants) bypassed AIP overrides and dispatched as CRI — observed as a 1% AIP hit rate (51 / 5,137 requests) on a fully-AIP-configured production endpoint.What changed
Single release; eight sections shipped together.
013_model_mappings_audit.sql) —model_mappingsgainscreated_via TEXT NOT NULL DEFAULT 'unknown'andlast_used_at TIMESTAMPTZ. Inert legacy rows (claude-sonnet-4-,opus, unreachable since v1.7.0) deleted.src/translate/canonicalize.rs::canonicalize_model_id: trim → auto-prependclaude-→ strip-YYYYMMDD→ reject chars outside[a-zA-Z0-9.:-]→ reject empty/claude-. Pure, case-preserving.src/api/accept.rs::accept_model: PinHit (raw) → CanonicalHit (canonicalize then re-check) → Discovered → Reject. Runs before endpoint filtering. Reject returns 400 + tracing warn.select_profile_idnow two-pass (exact stem, versioned stem). Discovery returnsvia: &'static strfor thecreated_viaaudit column.EndpointClient::aip_override_fortries rawHashMap::getfirst, then canonicalizes and retries. Admin-add validates canonical fixed-point or existing alias; non-canonical keys rejected with 400 naming the canonical form. Startup-timeEndpointPool::scan_non_canonical_aip_overridesflags pre-existing rows viatracing::info!/warn!.resolve_dispatch_target→Result<String, ModelUnavailableError>. Case 2 hard-fails (no longer silently CRI).select_endpoint-returns-None against a non-empty pool → 400. Empty-pool bootstrap path preserved.GET/POST/PUT/DELETE /admin/mappings+POST /admin/mappings/discover(10s timeout, 256-char input cap). Cache-version bumps on mutations propagate within 5s.ccag mappings list/add/delete/discover. Portal "Model Mappings" tab with filter, sort (NULL last_used first), color-codedcreated_via, add/discover modals, empty-state banner.Strict matching is the new default; existing rows are pinned at upgrade as the grandfather set. No flag, no soft-warn period — pin set IS the seamless-transition mechanism.
Acceptance evidence
[offline](verified bymake check, 510 lib + 352 integration = 862 tests):.skip, AC8.4 success path.fixme— both require live Bedrock dispatch inmake dev)[online](must verify at staging deploy time per Release Evidence Gate):SELECT created_via, COUNT(*) FROM model_mappings GROUP BY created_viashows expected backfill (unknown+ any in-flightpass1/pass2/admin); zeropass3rows.claude-sonnet-4-6-20250514against an endpoint with overrideclaude-sonnet-4-6 → AIP-arn-Xdispatches as the AIP ARN (verified byForwarding request to Bedrocklog line;bedrock_model = arn:aws:bedrock:...:application-inference-profile/...).made-up-modelreturns 400 withinvalid_request_errorenvelope; CW Logs Insights matchesmodel_id_rejected.SELECT COUNT(*) FROM model_mappings WHERE created_via = 'pass3' AND created_at > now() - interval '7 days'returns 0.Forwarding…andRequest completedevents).spend_logrows whereendpoint = NULLAND gateway has ≥1 endpoint configured drops to zero.created_viacolor-coding, NULLlast_used_atsort first, banner appears at zerounknownrows).New runtime I/O
Per
.claude/CLAUDE.mdRelease Evidence Gate:discover_modelalready callsbedrock:ListInferenceProfiles; the admin discover endpoint reuses it. No new IAM grants required.migrations/013_model_mappings_audit.sql(additive columns + delete inert legacy rows).last_used_atwrite integration with the spend-flush worker is deferred post-1.10.0 (TODO marker atsrc/api/handlers.rs:789; see backlog).tracing::warn!(model_id_rejected, …)on hard-fail;tracing::info!on admin CRUD mutations + non-canonical AIP override key startup scan;tracing::warn!on canonical-key collision during startup scan;tracing::warn!on DB error during alias-check validation. All standard tracing — no new exporters.Doc-IaC contract: no new doc-listed permission, env var, or external dependency. IaC unchanged.
Reviewer pass
Five same-model personas (security, performance, quality, UX, docs) ran against the diff. Findings:
unknowndescription scope, CLI arg name).build_no_override_for_canonical_erroris asserted on bytests/integration/dispatch_hardfail_tests.rs:472. Reviewer rated as "mild info-leak, not directly exploitable" — admin observability over caller-side opacity is a deliberate spec choice.discover_control_client,bedrock_routing_prefix,raw.to_string()in PinHit). Worth ~3-4 allocations per /v1/messages call; not a regression vs main, just unnecessary work.Out of scope (deferred)
touch_last_usedintegration with spend-flush worker (TODO athandlers.rs:789; AC1.4 path).ModelAcceptance::Discovered::profile_prefixis currently unused — can be removed once a consumer materializes.POST /admin/mappings/discoverbeyond the 10s timeout.Test plan
make check(lint + lib + integration) — green.013runs cleanly against existing prod-shape DB./deploy --staging→ smoke per AC list above → record evidence in.claude/deploys/<sha>-evidence.md./deploy --prod --releaseafter staging evidence.