feat(reliability): per-fallback API keys for custom endpoints#2571
Merged
theonlyhennygod merged 5 commits intomainfrom Mar 4, 2026
Merged
feat(reliability): per-fallback API keys for custom endpoints#2571theonlyhennygod merged 5 commits intomainfrom
theonlyhennygod merged 5 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
CI Tooling scripts/ci/ensure_cargo_component.sh |
Adds probe_rustfmt(), probe_rustdoc(), ensure_required_tooling(), and default_required_components(); auto-detects required components by job, installs missing components via rustup, and aborts when tooling is unavailable. |
Config Schema & Encryption src/config/schema.rs |
Adds fallback_api_keys: std::collections::HashMap<String,String> to ReliabilityConfig (default empty), introduces encrypt_map_secrets/decrypt_map_secrets helpers, wires encrypt/decrypt for the new map, and extends validation to require non-empty mapped keys and mapping entries. |
Provider Wiring src/providers/..., src/providers/mod.rs |
When constructing resilient/fallback providers, looks up per-fallback API keys by full fallback entry or by provider name from reliability.fallback_api_keys and passes the resolved key into create_provider_with_options; falls back to env-var resolution if none provided. |
Integration Test tests/reliability_fallback_api_keys.rs |
New integration test that starts mock primary + fallback servers, configures fallback_providers and fallback_api_keys, forces primary failure, asserts Authorization headers and final success from the keyed fallback. |
Config Usage Sites / Defaults src/config.rs, tests/... |
Updates call sites, defaults, and tests to initialize and persist the new fallback_api_keys field (encryption/decryption and validation exercised). |
Sequence Diagram(s)
sequenceDiagram
participant Config as Config (reliability)
participant Factory as ResilientFactory
participant Primary as PrimaryProvider
participant FallbackA as FallbackProviderA
participant FallbackB as FallbackProviderB
participant Client as Client
Config->>Factory: provide fallback_providers + fallback_api_keys
Factory->>Primary: create primary provider (no per-entry key)
Factory->>FallbackA: lookup key -> create with key?/None
Factory->>FallbackB: lookup key -> create with key?/None
Client->>Primary: request
Primary-->>Client: error
Client->>Factory: retry -> select FallbackA
FallbackA-->>Factory: unauthorized/error
Factory->>FallbackB: call with per-entry API key (from map)
FallbackB-->>Client: success response
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- fix(providers): warn on shared API key for fallbacks and warm up all providers #130: Modifies fallback provider handling in providers (overlaps with per-entry
fallback_api_keyswiring).
Suggested reviewers
- chumyin
🚥 Pre-merge checks | ✅ 3 | ❌ 2
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check | The PR includes changes to CI tooling verification (ensure_cargo_component.sh) that appear unrelated to the core reliability fallback API keys feature. | Review and clarify the necessity of changes to scripts/ci/ensure_cargo_component.sh (+92 lines). If unrelated to issue #2517, consider moving to a separate PR to maintain scope clarity. |
|
| Docstring Coverage | Docstring coverage is 79.41% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly summarizes the main change: adding per-fallback API keys support for custom endpoints in the reliability configuration. |
| Description check | ✅ Passed | The description covers key aspects (feature addition, wiring, encryption support, test coverage) and includes validation commands and issue reference, though some template sections are not filled. |
| Linked Issues check | ✅ Passed | The PR fully implements the acceptance criteria from issue #2517: distinct API keys per fallback provider, no regression for existing configs, and includes regression test coverage. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
issue-2517-fallback-provider-keys
Comment @coderabbitai help to get the list of available commands and usage tips.
PR intake checks found warnings (non-blocking)Fast safe checks found advisory issues. CI lint/test/build gates still enforce merge quality.
Action items:
Detected Linear keys: none Run logs: https://github.com/zeroclaw-labs/zeroclaw/actions/runs/22666014480 Detected blocking line issues (sample):
Detected advisory line issues (sample):
|
github-actions
Bot
added
config
|
Thanks for contributing to ZeroClaw. For faster review, please ensure:
See |
github-actions
Bot
added
size: S
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| .fallback_api_keys | ||
| .get(fallback) | ||
| .or_else(|| reliability.fallback_api_keys.get(provider_name)) | ||
| .map(String::as_str); |
Check failure
Code scanning / CodeQL
Cleartext logging of sensitive information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
In general, sensitive values like API keys should never be logged in cleartext. If some internal functions log inputs, the caller should avoid passing raw secrets directly or should wrap them in a type that prevents or redacts logging.
In this specific case, the problem is that fallback_api_key is a plain Option<&str> derived directly from reliability.fallback_api_keys. CodeQL indicates that this value is later written to logs. A minimal fix, without changing visible behavior, is to wrap the API key in a lightweight “secret” wrapper type that implements Debug/Display in a redacting way (e.g., printing "<redacted>") so that if any downstream code logs it, the actual key is not exposed. We then pass this wrapper instead of a bare &str.
Concretely, within src/providers/mod.rs:
- Define a small
SecretStr<'a>(&'a str)type near the top of the file (or near helper types), withDebugandDisplaythat do not reveal the underlying data, e.g. always print<redacted>. - Change the type of
fallback_api_keyat the call site to beOption<SecretStr<'_>>by mappingString::as_strintoSecretStr. - Adjust the call to
create_provider_with_optionsto pass this newOption<SecretStr<'_>>. This assumescreate_provider_with_options’s parameter is generic over anything implementingAsRef<str>or can be updated accordingly; but since we are constrained to this file only, we should keep the interface nominally compatible. To avoid changing its signature, we can ensureSecretStrimplementsAsRef<str>, so existing function signatures acceptingOption<impl AsRef<str>>orOption<&str>via generic bounds continue to work without behavior change, except that any logging viaDebug/Displaywill now be redacted.
This keeps functional behavior (the raw key is still available via AsRef<str> for HTTP headers, etc.) but prevents accidental cleartext logging of the key through formatted output.
Suggested changeset
1
src/providers/mod.rs
| @@ -28,6 +28,30 @@ | ||
| pub mod openai; | ||
| pub mod openai_codex; | ||
| pub mod openrouter; | ||
|
|
||
| /// Wrapper for sensitive strings (like API keys) to avoid cleartext logging. | ||
| /// Implements `Debug`/`Display` in a redacting way while still allowing use | ||
| /// of the underlying value via `AsRef<str>`. | ||
| #[derive(Clone, Copy)] | ||
| struct SecretStr<'a>(&'a str); | ||
|
|
||
| impl<'a> core::fmt::Debug for SecretStr<'a> { | ||
| fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
| f.write_str("<redacted>") | ||
| } | ||
| } | ||
|
|
||
| impl<'a> core::fmt::Display for SecretStr<'a> { | ||
| fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
| f.write_str("<redacted>") | ||
| } | ||
| } | ||
|
|
||
| impl<'a> AsRef<str> for SecretStr<'a> { | ||
| fn as_ref(&self) -> &str { | ||
| self.0 | ||
| } | ||
| } | ||
| pub mod quota_adapter; | ||
| pub mod quota_cli; | ||
| pub mod quota_types; | ||
| @@ -1600,7 +1624,7 @@ | ||
| .fallback_api_keys | ||
| .get(fallback) | ||
| .or_else(|| reliability.fallback_api_keys.get(provider_name)) | ||
| .map(String::as_str); | ||
| .map(|s| SecretStr(s.as_str())); | ||
|
|
||
| let fallback_options = match profile_override { | ||
| Some(profile) => { |
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
github-actions
Bot
added
config
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/providers/mod.rs (1)
1599-1604: Add a focused precedence test for entry-key vs provider-key resolution.This branch has two explicit lookup tiers; adding a direct test that entry-specific key wins over provider-level key would lock behavior and prevent regressions.
Based on learnings: "Implement
Providertrait insrc/providers/, register insrc/providers/mod.rsfactory, add focused tests for factory wiring and error paths, avoid provider-specific behavior leaks into shared orchestration code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/mod.rs` around lines 1599 - 1604, Add a focused unit test that verifies entry-specific fallback key lookup wins over provider-level fallback key: exercise the lookup logic that computes fallback_api_key (the map lookups on reliability.fallback_api_keys using the fallback key and provider_name) and assert that when both an entry-key (fallback) and a provider-key (provider_name) exist, the value from the entry-key is returned; implement the test by constructing a Reliability-like test fixture with both keys present, calling the factory/function that performs this resolution (the code that sets fallback_api_key) and asserting the resolved string equals the entry-specific value to lock the precedence behavior.src/config/schema.rs (1)
3997-4000: Clarify the new config key’s public contract in field docs.Please make the
fallback_api_keysdocs explicit about default ({}), backward compatibility (fully additive), and rollback/migration guidance (remove the key/table to revert behavior).✍️ Suggested doc patch
- /// Optional per-fallback provider API keys keyed by fallback entry name. - /// This allows distinct credentials for multiple `custom:<url>` endpoints. + /// Optional per-fallback provider API keys keyed by exact fallback entry string + /// (for example: `"custom:https://api-a.example.com/v1"`). + /// + /// Default: empty map (`{}`), so existing configurations are unchanged. + /// Compatibility: additive key; omitting this field preserves legacy key resolution. + /// Rollback: remove `[reliability.fallback_api_keys]` (or set it empty) to revert. + /// + /// This allows distinct credentials for multiple `custom:<url>` endpoints. #[serde(default)] pub fallback_api_keys: std::collections::HashMap<String, String>,As per coding guidelines, “For config/schema changes, treat keys as public contract: document defaults, compatibility impact, and migration/rollback path”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/schema.rs` around lines 3997 - 4000, The field doc for fallback_api_keys is incomplete about its public contract; update the doc comment on the fallback_api_keys field in schema.rs to state its default value is an empty map ({}), that adding this key is fully additive and backward-compatible (old configs continue to work), and that reverting the behavior is done by removing the key or table from the config (migration/rollback guidance); keep the #[serde(default)] behavior noted and reference the field name fallback_api_keys in the comment so readers can find it easily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/reliability_fallback_api_keys.rs`:
- Around line 93-95: The test currently only verifies
fallback_server_one.verify().await and fallback_server_two.verify().await and
misses asserting the primary mock; add a call to primary_server.verify().await
alongside the existing verifications so the primary hop is explicitly asserted
(i.e., call primary_server.verify().await before or after the fallback_server_*
verifications to ensure the primary mock expectations are checked).
---
Nitpick comments:
In `@src/config/schema.rs`:
- Around line 3997-4000: The field doc for fallback_api_keys is incomplete about
its public contract; update the doc comment on the fallback_api_keys field in
schema.rs to state its default value is an empty map ({}), that adding this key
is fully additive and backward-compatible (old configs continue to work), and
that reverting the behavior is done by removing the key or table from the config
(migration/rollback guidance); keep the #[serde(default)] behavior noted and
reference the field name fallback_api_keys in the comment so readers can find it
easily.
In `@src/providers/mod.rs`:
- Around line 1599-1604: Add a focused unit test that verifies entry-specific
fallback key lookup wins over provider-level fallback key: exercise the lookup
logic that computes fallback_api_key (the map lookups on
reliability.fallback_api_keys using the fallback key and provider_name) and
assert that when both an entry-key (fallback) and a provider-key (provider_name)
exist, the value from the entry-key is returned; implement the test by
constructing a Reliability-like test fixture with both keys present, calling the
factory/function that performs this resolution (the code that sets
fallback_api_key) and asserting the resolved string equals the entry-specific
value to lock the precedence behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
scripts/ci/ensure_cargo_component.shsrc/config/schema.rssrc/providers/mod.rstests/reliability_fallback_api_keys.rs
github-actions
Bot
added
config
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/ci/ensure_cargo_component.sh (1)
68-75: Auto-mapping drops one role when job names match multiple patterns.With the current
case, a job name containing both “lint” and “test” only gets the first branch. Consider composing the list so both components can be required when applicable.Suggested refactor
default_required_components() { local normalized_job_name="${1:-}" - case "${normalized_job_name}" in - *lint*) echo "rustfmt" ;; - *test*) echo "rust-docs" ;; - *) echo "" ;; - esac + local components=() + [[ "${normalized_job_name}" == *lint* ]] && components+=("rustfmt") + [[ "${normalized_job_name}" == *test* ]] && components+=("rust-docs") + echo "${components[*]}" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/ensure_cargo_component.sh` around lines 68 - 75, The function default_required_components currently returns only the first matching component because the case branches echo and exit; modify default_required_components to accumulate matches into a local variable (e.g., components="") by checking both patterns (use shell pattern tests like [[ "${normalized_job_name}" == *lint* ]] and [[ "${normalized_job_name}" == *test* ]]) and append "rustfmt" and "rust-docs" as applicable, then echo the final components string; keep the function name default_required_components and the same pattern checks but replace the case block with explicit tests that compose the full list before echoing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ci/ensure_cargo_component.sh`:
- Around line 34-37: The probe currently named probe_rustdoc calls "rustdoc
--version" which only proves the rustdoc binary (part of rustc) exists and will
miss the separate "rust-docs" component; update probe_rustdoc to check rustup
for the installed component (e.g. run "rustup component list --toolchain
\"${toolchain}\" --installed" and test for "rust-docs") and then use that probe
where the script checks/installs rust-docs (the logic around the rust-docs check
currently at lines 59-63) so the script correctly detects and installs the
rust-docs component when missing.
- Around line 47-49: The loop that installs components (iterating over
required_components and running rustup component add --toolchain "${toolchain}"
"${component}") uses "|| true" which hides failures; remove the silent
suppression and instead check each rustup component add exit code and
accumulate/report failures (or exit immediately) so missing/unknown components
(including those from ENSURE_RUST_COMPONENTS) cause the script to fail; also
keep or extend the existing post-install validation pattern used for rustfmt and
rust-docs to verify each installed component actually exists after installation
and log clear error messages referencing the component and toolchain if
validation fails.
---
Nitpick comments:
In `@scripts/ci/ensure_cargo_component.sh`:
- Around line 68-75: The function default_required_components currently returns
only the first matching component because the case branches echo and exit;
modify default_required_components to accumulate matches into a local variable
(e.g., components="") by checking both patterns (use shell pattern tests like [[
"${normalized_job_name}" == *lint* ]] and [[ "${normalized_job_name}" == *test*
]]) and append "rustfmt" and "rust-docs" as applicable, then echo the final
components string; keep the function name default_required_components and the
same pattern checks but replace the case block with explicit tests that compose
the full list before echoing.
theonlyhennygod
force-pushed
the
issue-2517-fallback-provider-keys
branch
from
44de520 to
b6b1412
Compare
github-actions
Bot
added
config
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
scripts/ci/ensure_cargo_component.sh (2)
34-37:⚠️ Potential issue | 🟠 Major
rust-docsverification is checking the wrong signal.Line 34 currently probes
rustdoc --version, which validates therustdocexecutable, not necessarily therust-docscomponent requested at Line 59. This can let missingrust-docspass undetected.Proposed fix
+component_installed() { + local toolchain="$1" + local component="$2" + rustup component list --toolchain "${toolchain}" \ + | grep -Eq "^${component}(-[[:alnum:]_:-]+)? \(installed\)$" +} + probe_rustdoc() { local toolchain="$1" - rustup run "${toolchain}" rustdoc --version >/dev/null 2>&1 + component_installed "${toolchain}" "rust-docs" }#!/usr/bin/env bash set -euo pipefail rg -n -C2 'probe_rustdoc|rust-docs' scripts/ci/ensure_cargo_component.shDoes `rustup component add rust-docs` install the `rustdoc` executable, or is `rustdoc` provided by the `rustc` component?As per coding guidelines: “Prefer reproducible commands and locked dependency behavior in CI-sensitive paths.”
Also applies to: 59-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/ensure_cargo_component.sh` around lines 34 - 37, The probe_rustdoc check is validating the rustdoc executable instead of the actual rust-docs component; update the probe_rustdoc function to query rustup for the component on the given toolchain (e.g. run rustup component list --toolchain "${toolchain}" --installed and check for rust-docs) and return non-zero if rust-docs is not present so the later rustup component add rust-docs logic behaves correctly.
47-49:⚠️ Potential issue | 🟠 MajorRequired component installation failures are silently ignored.
Line 48 uses
|| true, so unknown/unavailable components inENSURE_RUST_COMPONENTSwon’t fail CI even when required.Proposed fix
for component in ${required_components}; do - rustup component add --toolchain "${toolchain}" "${component}" || true + if ! rustup component add --toolchain "${toolchain}" "${component}"; then + echo "::error::failed to install required component '${component}' for ${toolchain}." + return 1 + fi done#!/usr/bin/env bash set -euo pipefail rg -n 'rustup component add .*\\|\\| true' scripts/ci/ensure_cargo_component.shBased on learnings: “Do not bypass failing checks without explicit explanation.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/ensure_cargo_component.sh` around lines 47 - 49, The loop that installs required_components currently silences failures by appending "|| true" to the rustup command; remove that suppression and instead let failures fail the script or explicitly handle unavailable components: replace the line using rustup component add --toolchain "${toolchain}" "${component}" (refer to the required_components variable and the installation loop) so it does not append "|| true", and optionally pre-validate components by querying rustup component list --toolchain "${toolchain}" to skip or report unknown components with a clear error and non-zero exit code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/ci/ensure_cargo_component.sh`:
- Around line 34-37: The probe_rustdoc check is validating the rustdoc
executable instead of the actual rust-docs component; update the probe_rustdoc
function to query rustup for the component on the given toolchain (e.g. run
rustup component list --toolchain "${toolchain}" --installed and check for
rust-docs) and return non-zero if rust-docs is not present so the later rustup
component add rust-docs logic behaves correctly.
- Around line 47-49: The loop that installs required_components currently
silences failures by appending "|| true" to the rustup command; remove that
suppression and instead let failures fail the script or explicitly handle
unavailable components: replace the line using rustup component add --toolchain
"${toolchain}" "${component}" (refer to the required_components variable and the
installation loop) so it does not append "|| true", and optionally pre-validate
components by querying rustup component list --toolchain "${toolchain}" to skip
or report unknown components with a clear error and non-zero exit code.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
scripts/ci/ensure_cargo_component.shsrc/config/schema.rssrc/providers/mod.rstests/reliability_fallback_api_keys.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/reliability_fallback_api_keys.rs
- src/config/schema.rs
This was referenced Mar 3, 2026
theonlyhennygod
force-pushed
the
issue-2517-fallback-provider-keys
branch
from
b6b1412 to
c6911f6
Compare
github-actions
Bot
added
config
Collaborator
Author
|
Status update after rebase refresh:
Fresh CI is running on the rebased head. External merge gate still applies:
Continuing to monitor CI and overall queue health; reviewer approval is required to complete merge. |
github-actions
Bot
added
config
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/ci/ensure_cargo_component.sh (2)
47-49:⚠️ Potential issue | 🟠 MajorRequired component install failures are currently swallowed.
Line 48 (and retry lines Line 53 and Line 61) uses
|| true, so missing/unknown components can fail without failing the script.Suggested fix
- for component in ${required_components}; do - rustup component add --toolchain "${toolchain}" "${component}" || true - done + for component in ${required_components}; do + if ! rustup component add --toolchain "${toolchain}" "${component}"; then + echo "::error::failed to install required component '${component}' for ${toolchain}." + return 1 + fi + done @@ - rustup component add --toolchain "${toolchain}" rustfmt || true + rustup component add --toolchain "${toolchain}" rustfmt @@ - rustup component add --toolchain "${toolchain}" rust-docs || true + rustup component add --toolchain "${toolchain}" rust-docs#!/usr/bin/env bash set -euo pipefail # Verify silent suppression points and their context. rg -n -C2 'rustup component add.*\|\| true' scripts/ci/ensure_cargo_component.shBased on learnings: Do not bypass failing checks without explicit explanation.
Also applies to: 53-53, 61-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/ensure_cargo_component.sh` around lines 47 - 49, The loop that runs rustup component add (the for loop iterating over required_components calling rustup component add --toolchain "${toolchain}" "${component}") currently swallows failures via "|| true"; remove the "|| true" and instead propagate errors or implement explicit retry/failure handling: replace the silent suppression with a retry loop that attempts rustup component add a few times with short backoff and exits non‑zero on final failure, or if certain components are known-optional, explicitly check and log that case before continuing; ensure the unique call site "rustup component add --toolchain \"${toolchain}\" \"${component}\"" is updated so missing/unknown component errors cause the CI script to fail rather than be ignored.
34-37:⚠️ Potential issue | 🟠 Major
rust-docsis validated with the wrong probe.Line 36 checks
rustdoc --version, but Line 59-Line 63 is meant to enforce therust-docscomponent.rustdocavailability is not a reliable proof thatrust-docsis installed, so this can silently pass when the required component is missing.Suggested fix
+component_installed() { + local toolchain="$1" + local component="$2" + rustup component list --toolchain "${toolchain}" \ + | grep -Eq "^${component}(-[[:alnum:]_:-]+)? \(installed\)$" +} @@ - if [[ " ${required_components} " == *" rust-docs "* ]] && ! probe_rustdoc "${toolchain}"; then + if [[ " ${required_components} " == *" rust-docs "* ]] && ! component_installed "${toolchain}" "rust-docs"; then echo "::error::rustdoc is unavailable for toolchain ${toolchain}." rustup component add --toolchain "${toolchain}" rust-docs || true - if ! probe_rustdoc "${toolchain}"; then + if ! component_installed "${toolchain}" "rust-docs"; then return 1 fi fiDoes `rustup component add rust-docs` install the `rustdoc` binary, or is `rustdoc` provided by a different component?Also applies to: 59-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/ensure_cargo_component.sh` around lines 34 - 37, The probe currently checks for the rustdoc binary (probe_rustdoc uses `rustdoc --version`) which doesn't prove the rust-docs component is installed; update probe_rustdoc to query rustup for the installed components for the given toolchain (e.g., run `rustup component list --toolchain "${toolchain}" --installed` and test for `rust-docs` with grep/exit status) and use that check in the enforcement block that adds/validates `rust-docs` so the script fails/installs correctly when the component is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/schema.rs`:
- Around line 4012-4015: Add validation that every key in
reliability.fallback_api_keys matches a configured entry in
reliability.fallback_providers and that each associated value is non-empty:
iterate the map in the configuration validation routine (e.g., wherever
config/schema validation occurs), for each (name, key) ensure
reliability.fallback_providers contains name and that key.trim().is_empty() is
false; if either check fails, return or bail! with a clear error mentioning the
invalid fallback key name so startup fails fast rather than silently falling
back to generic credentials.
---
Duplicate comments:
In `@scripts/ci/ensure_cargo_component.sh`:
- Around line 47-49: The loop that runs rustup component add (the for loop
iterating over required_components calling rustup component add --toolchain
"${toolchain}" "${component}") currently swallows failures via "|| true"; remove
the "|| true" and instead propagate errors or implement explicit retry/failure
handling: replace the silent suppression with a retry loop that attempts rustup
component add a few times with short backoff and exits non‑zero on final
failure, or if certain components are known-optional, explicitly check and log
that case before continuing; ensure the unique call site "rustup component add
--toolchain \"${toolchain}\" \"${component}\"" is updated so missing/unknown
component errors cause the CI script to fail rather than be ignored.
- Around line 34-37: The probe currently checks for the rustdoc binary
(probe_rustdoc uses `rustdoc --version`) which doesn't prove the rust-docs
component is installed; update probe_rustdoc to query rustup for the installed
components for the given toolchain (e.g., run `rustup component list --toolchain
"${toolchain}" --installed` and test for `rust-docs` with grep/exit status) and
use that check in the enforcement block that adds/validates `rust-docs` so the
script fails/installs correctly when the component is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 979c3b0c-7ffb-476c-a735-2145603ac4a4
📒 Files selected for processing (4)
scripts/ci/ensure_cargo_component.shsrc/config/schema.rssrc/providers/mod.rstests/reliability_fallback_api_keys.rs
- validate reliability.fallback_api_keys mappings and values - assert primary mock in fallback API key regression test - avoid logging tainted fallback entries
github-actions
Bot
added
config
Collaborator
Author
|
Addressed the open review/code-scanning points in
Local validation on this head:
|
github-actions
Bot
added
config
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/providers/mod.rs (1)
1617-1619:⚠️ Potential issue | 🟠 MajorAvoid logging raw fallback identifiers for custom endpoints.
Line 1618 logs
provider_namedirectly. Forcustom:/anthropic-custom:entries, this can include full URLs and potentially embedded credentials or tokenized query params.As per coding guidelines `src/{security,gateway,tools,runtime,providers}/**/*.rs`: "never log secrets, raw tokens, or sensitive payloads."🔒 Suggested hardening
match create_provider_with_options(provider_name, fallback_api_key, &fallback_options) { Ok(provider) => providers.push((fallback.clone(), provider)), Err(_error) => { + let fallback_provider_label = if provider_name.starts_with("custom:") + || provider_name.starts_with("anthropic-custom:") + { + provider_name + .split_once(':') + .map(|(kind, _)| kind) + .unwrap_or(provider_name) + } else { + provider_name + }; tracing::warn!( - fallback_provider = provider_name, + fallback_provider = fallback_provider_label, "Ignoring invalid fallback provider during initialization" ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/mod.rs` around lines 1617 - 1619, The warn call logs raw provider_name; instead sanitize or redact custom endpoint identifiers before passing them into tracing::warn! — detect entries starting with "custom:" or "anthropic-custom:" (or containing "://"), parse the URL (e.g., via url::Url::parse) to strip username/password and query or otherwise replace with a safe token like "<redacted-custom-endpoint>" and then log that sanitized_provider variable in the tracing::warn! invocation instead of provider_name.
🧹 Nitpick comments (1)
tests/reliability_fallback_api_keys.rs (1)
65-73: Consider one additional compatibility-path regression test.This test covers per-entry mapping well; add a companion case that also exercises provider-name lookup and confirms explicit entry mapping still wins when both are present (the branch at Line 1599-Line 1603 in
src/providers/mod.rs).Based on learnings: "Add focused tests for factory wiring and error paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/reliability_fallback_api_keys.rs` around lines 65 - 73, Add a new test case alongside the existing one in tests/reliability_fallback_api_keys.rs that exercises the provider-name lookup path and the per-entry mapping conflict: create a map that includes both a per-entry key (the existing fallback_api_keys entries) and a fallback key under the provider name, then assert that the explicit per-entry mapping in ReliabilityConfig (the fallback_api_keys HashMap used with fallback_providers) takes precedence over the provider-name lookup branch in the provider resolution logic (the branch around the match in src/providers/mod.rs that checks provider-name vs per-entry). Ensure the test uses the same identifiers (fallback_provider_one, fallback_provider_two, fallback_api_keys, ReliabilityConfig) so it exercises the factory wiring and confirms explicit entry mapping wins when both mappings are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/schema.rs`:
- Around line 4012-4015: Update the doc comment for the public field
`fallback_api_keys` (the `reliability.fallback_api_keys` config entry) to
explicitly state the contract: indicate the explicit default is an empty map
`{}` (matching the #[serde(default)]), note this change is additive and
non-breaking for existing configs, and document rollback guidance (remove the
key or the specific map entry to revert behavior). Add the short inline
sentence(s) above the `pub fallback_api_keys: std::collections::HashMap<String,
String>,` field so readers treating config keys as public contract see default,
compatibility impact, and migration/rollback path.
---
Duplicate comments:
In `@src/providers/mod.rs`:
- Around line 1617-1619: The warn call logs raw provider_name; instead sanitize
or redact custom endpoint identifiers before passing them into tracing::warn! —
detect entries starting with "custom:" or "anthropic-custom:" (or containing
"://"), parse the URL (e.g., via url::Url::parse) to strip username/password and
query or otherwise replace with a safe token like "<redacted-custom-endpoint>"
and then log that sanitized_provider variable in the tracing::warn! invocation
instead of provider_name.
---
Nitpick comments:
In `@tests/reliability_fallback_api_keys.rs`:
- Around line 65-73: Add a new test case alongside the existing one in
tests/reliability_fallback_api_keys.rs that exercises the provider-name lookup
path and the per-entry mapping conflict: create a map that includes both a
per-entry key (the existing fallback_api_keys entries) and a fallback key under
the provider name, then assert that the explicit per-entry mapping in
ReliabilityConfig (the fallback_api_keys HashMap used with fallback_providers)
takes precedence over the provider-name lookup branch in the provider resolution
logic (the branch around the match in src/providers/mod.rs that checks
provider-name vs per-entry). Ensure the test uses the same identifiers
(fallback_provider_one, fallback_provider_two, fallback_api_keys,
ReliabilityConfig) so it exercises the factory wiring and confirms explicit
entry mapping wins when both mappings are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6f2e10f-d595-4a54-a1cc-54b1e3491524
📒 Files selected for processing (4)
scripts/ci/ensure_cargo_component.shsrc/config/schema.rssrc/providers/mod.rstests/reliability_fallback_api_keys.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/ci/ensure_cargo_component.sh
Collaborator
Author
|
Follow-up update in
Local validation on this head:
Current remaining unresolved item is the GitHub Advanced Security PR thread ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
reliability.fallback_api_keysfor per-fallback credentials keyed by fallback entry stringreliability.fallback_api_keyscustom:<url>fallback endpoints can use different API keysValidation
cargo fmt --all -- --checkcargo test --lib config_save_encrypts_nested_credentials -- --nocapturecargo test --test reliability_fallback_api_keys -- --nocaptureCloses #2517
Summary by CodeRabbit
New Features
Chores
Tests