fix(gateway): stored disabled tool calling survives endpoint moves — with the contract-seam lift and LiteLLM production repairs - #3042
Conversation
GitHub Actions
Inactive
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR centralizes gateway protocol contracts, fixes provider URL normalization, refines gateway setup preservation and error classification, strengthens UI validation, expands package exports, and adds LiteLLM troubleshooting documentation. ChangesGateway contracts, exports, and URL handling
Gateway setup and validation
LiteLLM troubleshooting
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ❌ 2❌ Failed checks (2 warnings)
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 |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![keiko-for-quality[bot]](https://avatars.githubusercontent.com/in/4290143?s=60&v=4){kind=small}
|
Keiko for Quality — run summary
Budget: 2440464 tokens allotted, 133088 reported Recent runs
|
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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: 2e7c233f4c
ℹ️ 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".
…ckage surface (#3042) PROVIDER_ENDPOINT_STYLES and REALTIME_AUTH_MODES reach the root runtime and declaration surface through the contracts re-export; the expectation update was lost in the cherry-pick onto fresh dev and turned the whole CI matrix red on the surface gate. Set-diff against the previous contract: exactly these two additions, zero removals (the file rewrite is the checker's deterministic sort). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d0bbcd98f
ℹ️ 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".
…antics; honest LiteLLM diagnostics (#3042) - A fresh replacement (preserveExisting omitted) no longer inherits a stored disabled toolCalling onto a new endpoint — the restriction now rides the preserve flag through the setup request like every stored-capability carry-over; pin red first (Codex finding). - Troubleshooting doc: the proxy diagnostic lists variable NAMES only (proxy URLs embed credentials), and the discovery-cap check counts JSON entries instead of lines — a capped response also never exceeds the cap, so the 'above 100' reading was unreachable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0537be7333
ℹ️ 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".
…er in history or argv (#3042) The placeholder assignment put the real LiteLLM hostname into shell history once an operator substituted it, and the expanded URL was visible in curl's process arguments for the duration of each request. Both blocks now read the host and the key without echo into a 0600 mktemp curl config file (url + header), so curl's argv carries only --config; the separate header files are gone with it. Executed against an unreachable host: status 7 propagates out of the subshell, the parent shell survives. All three bash blocks syntax-checked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ives-endpoint-move
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a16fc48303
ℹ️ 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".
…unts require an array (#3042) - curl -S printed 'Could not resolve host: <hostname>' on stderr, putting the production endpoint into scrollback and support logs even though --config had removed it from argv. The probes run with -s only and map a non-zero exit to 'transport failure (curl exit N)' — 6 DNS, 7 connect, 35 TLS, 28 timeout. Verified against an unresolvable host: the name appears nowhere in stdout or stderr. - The count guarded only JSON.parse, so a valid document whose data member is a string printed a character count, and {"data":{"length":"..."}} printed attacker-controlled text. Array.isArray is required now; verified against object-with-length, string data, a proper array and non-JSON. All three bash blocks syntax-checked; sonar PASS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c89ac81b8
ℹ️ 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".
…xit status (#3042) Three review findings on the diagnostics, each verified against a local proxy stand-in rather than argued: - The model-list download is bounded in bytes and time (--max-filesize 2MB, --max-time 30) and the reader refuses an oversized file, so a hostile or misconfigured proxy can fill neither the disk nor the reader's memory. A 2MB+ body now ends as 'transport failure (curl exit 63)' with status 63. - The count probe asks which key header the proxy reads; hardcoding Authorization made the diagnostic useless on exactly the proxies the page tells operators to configure. Both header choices return the real count (137 against the stand-in). - probe() returns curl's status instead of swallowing it behind the echo, so a pasted script can tell a transport failure from a completed diagnostic; the auth block exits with the first non-zero status. Additionally the unreadable-body message carries the HTTP status (a number, still body-free): a key on the ignored header now reads 'unreadable response (HTTP 401, ...)' instead of looking like a malformed model list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbdbbd3ff4
ℹ️ 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".
…inct ids from a 2xx body (#3042) - P1: curl reads ~/.curlrc even when --config is supplied, so a default 'verbose' or 'trace' there would print the Authorization header this block exists to hide. All three keyed invocations now start with -q. - The count claimed to predict Keiko's truncation but used the RAW array length, while parseModelDiscovery rejects non-2xx, drops entries it cannot classify, and deduplicates by id BEFORE applying MAX_DISCOVERED_MODELS. It now counts distinct ids from a 2xx body only, and the guidance states the number is an upper bound on what Keiko keeps, not an exact prediction. Verified against a local stand-in: 137 distinct → 137; 50 entries with 5 distinct ids → 5; a body mixing a valid entry with a malformed record and a bare string → 1; HTTP 500 carrying a data array → 'unreadable response (HTTP 500, ...)' instead of a count. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
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 `@docs/troubleshooting/litellm-production-gateway.md`:
- Around line 57-62: In both configuration-generation sites in
docs/troubleshooting/litellm-production-gateway.md (lines 57-62 and 209-216),
parse HDR using an explicit allowlist before prompting for the key, then escape
HOST, KEY, and HDR for their double-quoted curl configuration values before
writing the files. Apply the same validation and escaping behavior at both sites
so malformed input cannot alter the generated authorization header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b2642759-2d87-4839-86dc-0bb138c066b2
📒 Files selected for processing (2)
docs/troubleshooting/litellm-production-gateway.mdpackages/keiko-server/src/gateway-setup.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: Cross-platform smoke (windows-latest)
- GitHub Check: Cross-platform smoke (ubuntu-latest)
- GitHub Check: Cross-platform smoke (macos-latest)
- GitHub Check: Coverage suite (keiko-ui)
- GitHub Check: Build, scan, SBOM, smoke
- GitHub Check: Coverage suite (scripts)
- GitHub Check: Core quality
- GitHub Check: Coverage shard (packages 2/3)
- GitHub Check: ui
- GitHub Check: Coverage shard (packages 3/3)
- GitHub Check: Coverage shard (packages 1/3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (6)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Use English in code, comments, identifiers, documentation, commit messages, issues, and pull requests.
Respect the inward dependency direction:keiko-contractsis the leaf,keiko-securitydepends only on contracts, and cross-package types belong in contracts.
Inspect and reuse existing helpers, contracts, and subsystems; do not create parallel workspace, graph, policy, evidence, memory, connector, workflow, or UI subsystems without a documented capability gap.
Keep manifests, audit exports, diagnostics, logs, configuration, tests, and evidence redacted: never expose secrets, keys, raw content, endpoints, PII, memory bodies, or customer data.
Never weaken trust boundaries, evidence redaction, governance gates, required checks, or authority limits to make a change pass; fail closed.
Never directly push todev, force-push, bypass required checks, dismiss findings to obtain green status, or merge outside the approved direct-check path.
Use branch names in the formtype/short-slug, never work directly ondev, and use imperative conventional commit subjects with an issue or PR reference and no secrets.
Run the applicable local typecheck, lint, format check, tests, architecture checks, Sonar gate, and touched-area gates before claiming a change or pull request is green; report only commands actually executed.
Files:
docs/troubleshooting/litellm-production-gateway.mdpackages/keiko-server/src/gateway-setup.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use strict TypeScript settings, forbidany, require explicit function return types, and model states with discriminated unions rather than casts.
Keep cyclomatic complexity at or below 10, keep functions within 50 non-comment lines, and extract helpers when necessary.
Use the intended logger or diagnostic sink instead ofconsole.*in product code.
Validate hostile inputs before processing, fail closed at trust boundaries, and never route generated or model-produced code around the sandbox egress boundary.
Do not swallow errors with empty catches; surface sufficient diagnostic context, including a correlation ID for server-visible opaque 500 errors.
Files:
packages/keiko-server/src/gateway-setup.ts
**/*.{ts,tsx,js,mjs,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier formatting: 2-space indentation, double quotes, semicolons, 100-column width, trailing commas, and LF endings.
Files:
packages/keiko-server/src/gateway-setup.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Delete dead code instead of commenting it out and do not leave scaffolding or TODOs hiding unfinished branches.
Files:
packages/keiko-server/src/gateway-setup.ts
packages/keiko-server/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
The server may use domain packages but must not depend on
keiko-editor; domain packages must not depend on the server.
Files:
packages/keiko-server/src/gateway-setup.ts
packages/**/src/**
⚙️ CodeRabbit configuration file
packages/**/src/**: Enforce ADR-0019 package direction and the owning trust boundary. Flag provider SDK imports
outside keiko-model-gateway, cross-package wire types outside contracts, workspace escape,
raw evidence bodies, silent failures, and parallel subsystems that should extend an owner.
Files:
packages/keiko-server/src/gateway-setup.ts
🧠 Learnings (4)
📚 Learning: 2026-07-24T19:25:13.892Z
Learnt from: oscharko
Repo: oscharko-dev/Keiko PR: 2697
File: packages/keiko-server/src/editor/inlineCompletionRoutes.ts:616-616
Timestamp: 2026-07-24T19:25:13.892Z
Learning: In `packages/keiko-server`, follow ADR-0141 D5’s AppSession/request-authorization model for workspace root resolution. When reviewing code that performs the `resolveRequestRoot`-style authorization gate, ensure it (1) re-proves persisted workspace identity, (2) derives the request root path, (3) enforces canonical realpath containment within the expected workspace/root to prevent path traversal, (4) verifies workspace presence/validity, and (5) requires a live AppSession (launcher-attested, process-scoped authority per BFF). Do not require a task identity for this gate if AppSession does not model task identity; authorization should be uniform and based on the same-user local threat model rather than per-task/per-workspace session semantics.
Applied to files:
packages/keiko-server/src/gateway-setup.ts
📚 Learning: 2026-07-26T21:23:56.288Z
Learnt from: oscharko
Repo: oscharko-dev/Keiko PR: 2755
File: packages/keiko-server/src/grounded-qa-hybrid.ts:1006-1012
Timestamp: 2026-07-26T21:23:56.288Z
Learning: In Keiko’s internal “stable-ID” hash helper functions (e.g., where stable identifiers are computed for persistence/comparison), preserve the existing UTF-16 code-unit iteration semantics when modernizing. Specifically, if the current implementation iterates with `charCodeAt(i)` (code units) rather than `codePointAt(i)` (Unicode code points), keep `charCodeAt(i)` to avoid changing the resulting stable IDs. If changing from `charCodeAt` to `codePointAt` is ever desired, it must be an explicitly planned compatibility/migration decision (e.g., versioning, backfill, or dual-read/write) rather than a mechanical refactor.
Applied to files:
packages/keiko-server/src/gateway-setup.ts
📚 Learning: 2026-07-27T10:55:30.485Z
Learnt from: oscharko
Repo: oscharko-dev/Keiko PR: 2766
File: packages/keiko-server/src/qualityIntelligence/figmaSnapshotRoutes.test.ts:1738-1738
Timestamp: 2026-07-27T10:55:30.485Z
Learning: When reviewing TypeScript files, do not flag single-quoted string literals as a violation if they match Prettier’s formatter-approved output. Specifically, if Prettier is configured to prefer double quotes but retains a single-quoted literal solely because switching to double quotes would require escaping embedded double quotes, allow the single quotes (i.e., don’t “fix” it beyond what Prettier would produce).
Applied to files:
packages/keiko-server/src/gateway-setup.ts
📚 Learning: 2026-07-27T18:08:22.461Z
Learnt from: oscharko
Repo: oscharko-dev/Keiko PR: 2780
File: packages/keiko-server/src/store/migrations.test.ts:78-95
Timestamp: 2026-07-27T18:08:22.461Z
Learning: When reviewing TypeScript lint findings (e.g., from typescript-eslint/no-unsafe-assignment), ensure the issue is reproducible using the repository’s configured ESLint setup/TS project settings. Do not treat diagnostics observed under a mismatched TypeScript project/tsconfig as authoritative; only accept them as real if you can reproduce them with the same ESLint configuration the repo uses (e.g., via its full/targeted lint scripts). In related test code, prefer deriving branded identity types from the production function return types to keep RootIdentity/brand fields type-compatible and avoid unsafe assignments.
Applied to files:
packages/keiko-server/src/gateway-setup.ts
🔇 Additional comments (4)
packages/keiko-server/src/gateway-setup.ts (1)
32-33: LGTM!Also applies to: 302-303, 316-319, 352-352, 403-417, 439-445, 1188-1193, 1396-1452, 1467-1471, 1480-1493, 1508-1511, 1612-1612, 1835-1838, 3151-3167, 3423-3423, 3530-3530, 3726-3726, 4168-4168
docs/troubleshooting/litellm-production-gateway.md (3)
44-56: LGTM!Also applies to: 63-85
204-208: LGTM!Also applies to: 217-222, 225-237
199-203: 🩺 Stability & AvailabilityNo change needed.
--max-filesizeis applied to chunked or ongoing transfers, so the documented bounded behavior is not invalidated by unknown response sizes in this script.> Likely an incorrect or invalid review comment.
…ues (#3042) curl config values are double-quoted, so a key or host containing a quote or backslash silently produced a DIFFERENT header than the operator intended — an unexplained auth failure with no way to see the cause. Both blocks escape those two characters before writing (the escaper keeps the value out of any argv: printf is a builtin, sed reads stdin). The header NAME is allowlisted to the two headers this proxy family supports, so a typo fails fast with exit 2 instead of sending a header nothing reads. Byte-exact verification against a local stand-in: the key sk-a"b\\c arrives as 'Bearer sk-a"b\\c' unchanged; both allowlisted headers return the count; a bogus header name exits 2 without issuing a request. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e83fa885c
ℹ️ 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".
… pin requires (#3042) The regenerated contract re-sorted runtimeExports with localeCompare, while tests/architecture/root-package-surface-contract.test.ts asserts the list equals [...].sort() — code-unit order, uppercase before lowercase. The gate accepted a file the pin rejected, which is how a green local check:package-surface still failed the coverage shard. The generator now sorts with an explicit code-unit comparator (proven to reproduce default-sort output exactly), the committed list is reordered to match with an unchanged SET (288 entries, verified), and the neighbouring multi-push Sonar smell in the same file is folded into one call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d claims only what it proves (#3042) modelIdFromKnownFields takes the first usable of id / model_name / model / deployment_name / deploymentName, TRIMS it and bounds its length before parseModelDiscovery deduplicates — so counting raw `id` strings both missed entries the gateway does read and double-counted ids differing by whitespace. The one-liner mirrors that normalization now. The guidance is corrected in the direction that matters rather than overclaiming: 100 or fewer proves nothing is truncated; above 100 is a strong indication only, because entries whose declared mode the gateway rejects are still dropped before the cap. Confirming truncation means comparing with the deployment list Keiko shows after a save. Verified against a stand-in: whitespace-differing ids → 2, alternative id fields → 3, unusable/absent ids → 1, a plain 137-entry list → 137. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
1 participant
Summary
Three review-complete work packages from the 0.3.0 hardening cycle, deferred off the collector (#3037) as agreed there:
toolCalling: falseno longer flips to enabled when a preserve-mode rebuild moves the endpoint or the capability entry is missing from the rebuilt set, and the Mistral limitation note travels with the disabled state instead of being stripped byapplyMistralSetupDefaultson every preserve rebuild. ThereplaceModelCapabilityre-enable path stays intact.ProviderEndpointStyle,RealtimeAuthMode+ value arrays withRecord<Union, true>-derived exhaustiveness). The UI upload parser and the server setup route now validate against the same wire values without importing the gateway package (ADR-0019: keiko-ui never imports keiko-model-gateway); keiko-model-gateway re-exports from contracts so every existing consumer keeps its import path.sameBaseUrlIdentitygating, 401 classification on setup failures, upload-parser LiteLLM tolerances, plus docs/troubleshooting/litellm-production-gateway.md (x-litellm-key header configuration, NO_PROXY for loopback proxies, outputTokenParameter for reasoning aliases, discovery cap) with the realtime-voice boundary stated as by-design (realtime is not offered over LiteLLM; the product detects the model and withholds the surface).Reuse / No-Duplication
No new subsystem. The unions moved to the existing contract seam (
keiko-contracts/src/gateway.ts); the gateway package re-exports them; the UI reads them through the existing@/lib/typesseam. The toolCalling preservation extends the existing capability-preservation path ingateway-setup.ts; the LiteLLM repairs extend existing validators.Verification
gateway-setup.test.ts204/204, keiko-model-gateway 1041/1041, modals suite 225/225 — all on this branch atop current dev.tsc -bacross contracts/model-gateway/server/ui, repo ESLint (zero warnings), keiko-ui workspace lint, prettier.npm run arch:check(contract boundaries green — the new edges point inward to contracts).npm run check:package-surface:assembledfor the new contracts exports.npm run gates:sonarPASS on the final diff.Update-Impact
Behavioral: preserve-mode rebuilds no longer silently re-enable stored-disabled tool calling; LiteLLM chat URLs tolerate trailing slashes; moved voice endpoints no longer inherit the previous endpoint's protocol shape. No wire-format change (the unions are identical values, now single-sourced). No migration required.
🤖 Generated with Claude Code