fix(http_request): stabilize credential_profile env resolution

[theonlyhennygod]

Closes #2562

Summary

• add env-backed credential caching for http_request credential profiles
• fall back to cached secrets when env lookup intermittently fails at runtime
• refresh cache on successful reads to support secret rotation

Validation

• cargo test resolve_credential_profile_injects_env_backed_header
• cargo test resolve_credential_profile_missing_env_var_fails
• cargo test resolve_credential_profile_uses_cached_secret_when_env_temporarily_missing
• cargo test resolve_credential_profile_refreshes_cached_secret_after_rotation
• cargo fmt --all -- --check

Summary by CodeRabbit

• Improvements
  • Credential profile names are normalized to lowercase for consistent handling.
  • Credential caching implemented to improve performance and availability; cached credentials are used when environment variables are temporarily missing and refreshed after rotation.
• Bug Fixes
  • Empty environment variables no longer incorrectly fall back to cached secrets; warnings emitted when cached secrets are used.
• Tests
  • Added tests covering caching, refresh after rotation, and empty-env behavior.

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)

Validation error: Unrecognized key(s) in object: 'tools', 'path_filters', 'review_instructions'

⚙️ Configuration instructions

• Please see the configuration documentation for more information.
• You can also validate your configuration using the online YAML validator.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

📝 Walkthrough

Walkthrough

Adds a credential caching layer to the http_request tool: a Mutex-backed HashMap, helper methods to read/cache secrets, preload of non-empty env vars on init, and use of the cache during credential resolution; tests added for caching and rotation behaviors.

Changes

Cohort / File(s)	Summary
HttpRequest tool core src/tools/http_request.rs	Adds credential_cache: Mutex<HashMap<String, String>>, helper methods read_non_empty_env_var, cache_secret, cached_secret, and resolve_secret_for_profile. Normalizes profile names to lowercase, preloads cache from env, and replaces direct env reads with cache-backed resolution and warnings when falling back to cached secrets.
Tests (http_request) src/tools/.../http_request_tests.rs, tests/...	Adds tests covering: caching when env var becomes temporarily missing, cache refresh after secret rotation, and behavior when env var is empty (should not fall back to cache). Duplicated/parallel test blocks present for these scenarios.
Metadata / deps Cargo.toml	Minor dependency/declaration adjustments referenced by the diff (lines added/removed related to implementation).

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant HttpRequestTool
    participant Env as Environment
    participant Cache as CredentialCache

    Client->>HttpRequestTool: request with credential_profile "clawhub"
    HttpRequestTool->>Cache: lookup("CLAWHUB_TOKEN")
    alt cache hit
        Cache-->>HttpRequestTool: return cached_secret
    else cache miss
        HttpRequestTool->>Env: read "CLAWHUB_TOKEN"
        alt env has non-empty value
            Env-->>HttpRequestTool: return secret
            HttpRequestTool->>Cache: store secret
        else env missing/empty
            alt cache has entry
                Cache-->>HttpRequestTool: return cached_secret (warn)
            else
                HttpRequestTool-->>Client: error (missing credential)
            end
        end
    end
    HttpRequestTool->>Client: attach header and perform request

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• feat(http_request): add env-backed credential profiles and onboarding policy presets #2148: Introduced env-backed credential_profiles and resolve_credential_profile in src/tools/http_request.rs; this PR extends that work by adding caching and cache-backed secret resolution.

Suggested reviewers

• chumyin

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description provides a clear summary of changes (3 bullets), links to issue #2562, and includes validation commands. However, it lacks the comprehensive metadata required by the template, including risk/size labels, change type, security/privacy assessments, and detailed validation evidence.	Complete the PR description to match the template structure by adding Label Snapshot, Change Metadata, Security Impact assessment, Privacy review, Compatibility details, and Human Verification sections.
Docstring Coverage	⚠️ Warning	Docstring coverage is 45.45% 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 PR title 'fix(http_request): stabilize credential_profile env resolution' directly describes the main change: adding credential caching and fallback mechanisms to stabilize environment variable resolution for credential profiles.
Linked Issues check	✅ Passed	The PR implementation directly addresses issue #2562's objectives: adds credential caching mechanism, implements fallback to cached secrets on env var lookup failures, supports secret rotation, and includes tests covering all scenarios including the empty env var edge case.
Out of Scope Changes check	✅ Passed	All changes in src/tools/http_request.rs are scoped to credential profile env resolution: credential caching, helper methods for cache management, and updated credential resolution flow with new tests. No unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch issue-2562-http-request-credential-profile

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

PR intake checks found warnings (non-blocking)

Fast safe checks found advisory issues. CI lint/test/build gates still enforce merge quality.

• Missing required PR template sections: ## Validation Evidence (required), ## Security Impact (required), ## Privacy and Data Hygiene (required), ## Rollback Plan (required)
• Incomplete required PR template fields: summary problem, summary why it matters, summary what changed, validation commands, security risk/mitigation, privacy status, rollback plan
• Missing Linear issue key reference (RMN-<id>, CDV-<id>, or COM-<id>) in PR title/body (recommended for traceability, non-blocking).

Action items:

1. Complete required PR template sections/fields.
2. (Recommended) Link this PR to one active Linear issue key (RMN-xxx/CDV-xxx/COM-xxx) for traceability.
3. Remove tabs, trailing whitespace, and merge conflict markers from added lines.
4. Re-run local checks before pushing:
   • ./scripts/ci/rust_quality_gate.sh
   • ./scripts/ci/rust_strict_delta_gate.sh
   • ./scripts/ci/docs_quality_gate.sh

Detected Linear keys: none

Run logs: https://github.com/zeroclaw-labs/zeroclaw/actions/runs/22647234298

Detected blocking line issues (sample):

• none

Detected advisory line issues (sample):

• none

[github-actions]

Thanks for contributing to ZeroClaw.

For faster review, please ensure:

• PR template sections are fully completed
• cargo fmt --all -- --check, cargo clippy --all-targets -- -D warnings, and cargo test are included
• If automation/agents were used heavily, add brief workflow notes
• Scope is focused (prefer one concern per PR)

See CONTRIBUTING.md and docs/pr-workflow.md for full collaboration rules.

[MrSuddenJoy]

@theonlyhennygod You are on the right path, although there is still some work that needs to be done. Keep going.

[alexl83]

Out of curiosity I locally pulled this PR, it compiles perfectly but still env vars for configured credential_profiles are not found

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/tools/http_request.rs (1)

954-1032: Add a boundary test for the empty-env branch in credential resolution.

Lines 954-1032 cover missing-env and rotation well, but the new empty-value branch (Line 58 onward) is not explicitly tested. Please add one deterministic test to lock intended behavior for that failure mode.

As per coding guidelines: "For security/runtime/gateway/tools changes, include at least one boundary/failure-mode validation."

🤖 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/tools/http_request.rs`:
- Around line 58-66: The current logic treats an empty env value
(secret.is_empty()) as safe to reuse a cached secret via
self.cached_secret(env_var) which must be removed; instead, when
secret.is_empty() is detected, do not fall back to cached secrets—log a
warning/error referencing requested_name and env_var and return a hard error
(e.g., bail! or Err) from the surrounding method so credential revocation is
honored. Update the branch containing secret.is_empty(), remove the return
Ok(cached) path that calls self.cached_secret(env_var), and ensure callers
handle the propagated error; keep tracing fields (profile=requested_name,
env_var) in the log for context.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0a0433b and cade681.

📒 Files selected for processing (1)

• src/tools/http_request.rs

[theonlyhennygod]

Addressing the remaining review point in commit 1989de3b on branch issue-2562-http-request-credential-profile.

What was fixed

• credential_profile resolution now treats explicitly empty env var values as hard failure and does not fall back to cached secrets.
  • src/tools/http_request.rs (resolve_secret_for_profile, empty-value branch).

Regression coverage added

• tools::http_request::tests::resolve_credential_profile_empty_env_var_does_not_fallback_to_cached_secret

Targeted validation run (all passed)

• cargo fmt --all -- --check
• cargo test --lib resolve_credential_profile_missing_env_var_fails
• cargo test --lib resolve_credential_profile_uses_cached_secret_when_env_temporarily_missing
• cargo test --lib resolve_credential_profile_refreshes_cached_secret_after_rotation
• cargo test --lib resolve_credential_profile_empty_env_var_does_not_fallback_to_cached_secret

Broader safety pass

• ./scripts/ci/rust_quality_gate.sh
• ./scripts/ci/rust_strict_delta_gate.sh
• Gate summary: No blocking strict lint issues on changed Rust lines.
• Note: script output still includes large repo-wide baseline Clippy findings outside this PR’s changed lines.