Skip to content

fix(security): keep API keys out of curl probe argv - #5975

Merged
apurvvkumaria merged 46 commits into
mainfrom
fix/curl-probe-no-bearer-in-argv
Jul 1, 2026
Merged

fix(security): keep API keys out of curl probe argv#5975
apurvvkumaria merged 46 commits into
mainfrom
fix/curl-probe-no-bearer-in-argv

Conversation

@laitingsheng

@laitingsheng laitingsheng commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Probe-time curl spawns embedded the literal API key as an argv element (-H "Authorization: Bearer ...", -H "x-api-key: ...", ?key=... in URL), exposing it to host ps auxww on container runtimes that share /proc with the host (k3s/containerd, Docker Desktop on WSL2). Route every credential through a 0600 curl --config tmpfile so the secret never reaches argv.

Related Issue

Fixes #5966

Changes

  • New src/lib/adapters/http/auth-config.ts helper that writes a 0600 curl config tmpfile carrying header = "..." or url-query = "..." entries; returns the --config <path> argv pair plus a cleanup() for finally.
  • provider-models.ts: fetchNvidiaEndpointModels, fetchOpenAiLikeModels, fetchAnthropicModels route Bearer / x-api-key / query-param credentials via the new helper.
  • onboard-probes.ts: probeResponsesToolCalling, probeChatCompletionsToolCalling, probeOpenAiLikeEndpoint (Responses + Chat Completions + streaming + doubled-timeout retry), probeAnthropicEndpoint — all argv sites swapped to --config; bearer/x-api-key/?key= literals removed from URL and argv. Single authConfig per probe scope, cleaned up in finally.
  • Defence-in-depth in curl-args.ts: validateCurlProbeArgs refuses inline Authorization: / x-api-key: / x-goog-api-key: -H values and refuses URLs with ?key= / ?api_key= / ?apikey= / ?token= / ?access_token= query parameters. Trusted --config route remains the only legal credential channel.
  • Tests updated to invert "argv contains Bearer literal" → "argv contains --config <path> and the file is mode 0600 carrying the expected header/url-query entry; literal API key never appears in argv". New behavioural coverage for Anthropic. Existing onboard-smoke harness now reads the config file mid-probe to keep its auth assertion.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: internal probe transport; no user-visible behaviour change
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification:
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

  • New Features
    • Added curl probe auth via temporary on-disk config for Bearer, API key, and OpenAI-like “bearer vs query-param” modes.
  • Bug Fixes
    • Reduced credential exposure by rejecting inline auth headers and secret-bearing query parameters; curl requests now use --config with safe cleanup.
    • Improved onboarding/provider probing and standardized retry behavior for timeout and retriable HTTP failures.
  • Tests
    • Expanded coverage for auth config generation/escaping, curl-arg validation, onboarding flows, and tracing redaction.
  • Chores
    • Increased Node heap limit for integration subprocesses during Vitest.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Routes curl credentials through temp config files, rejects inline credential leakage in curl arguments, and updates inference/provider probe flows, tests, and harnesses to use --config-based auth handling with cleanup.

Changes

Credential Isolation via curl --config

Layer / File(s) Summary
auth-config adapter and helper tests
src/lib/adapters/http/auth-config.ts, src/lib/adapters/http/auth-config.test.ts, src/lib/adapters/http/auth-config-test-helpers.ts
Defines the auth-config types, temp-file creation, escaping, cleanup, and convenience helpers for bearer, query-param, x-api-key, and OpenAI-like auth. Shared tests assert config-file paths, contents, permissions, and trusted config tracking.
curl-args credential rejection
src/lib/adapters/http/curl-args.ts, src/lib/adapters/http/curl-args.test.ts, test/helpers/onboard-smoke-verifier-harness.ts
Adds rejection of inline auth headers and secret query parameters, and updates tests plus the smoke-verifier harness to read auth details from --config files instead of argv.
provider-models auth-config migration
src/lib/inference/provider-models.ts, src/lib/inference/provider-models.test.ts
Refactors NVIDIA, OpenAI-like, and Anthropic model fetches to use auth-config builders with trusted config files and cleanup, and updates tests to assert credentials only appear in the generated config file.
onboard-probes auth-config and retry wiring
src/lib/inference/onboard-probes.ts, src/lib/inference/onboard-probes.test.ts
Reworks onboarding probes and retry handling to use auth-config args, pass trusted config files through curl execution, and verify reused config paths, fallback behavior, and process-command-line secrecy.
shared retry helpers, trace redaction, and test runtime config
src/lib/inference/probe-retry.ts, src/lib/inference/health.ts, src/lib/trace.test.ts, vitest.config.ts, .github/workflows/pr.yaml
Adds shared retry primitives, switches a Kimi probe builder to the new credential-args shape, updates trace redaction coverage for a different secret query parameter, and increases worker heap and CI shard time limits.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

  • ericksoa
  • jyaunches
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR includes unrelated changes like the probe-retry module, Vitest heap tuning, and a workflow timeout bump beyond the linked security fix. Split the CI/runtime tuning and retry refactors into separate PRs so this change stays focused on removing API keys from process-visible arguments.
Docstring Coverage ⚠️ Warning Docstring coverage is 16.28% 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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: keeping API keys out of curl argv.
Linked Issues check ✅ Passed The NVIDIA probe now routes secrets through curl --config and tests verify the API key is absent from argv, matching the issue.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/curl-probe-no-bearer-in-argv

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/curl-probe-no-be... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/curl-probe-no-be... d39e2c6 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the fix/curl-probe-no-be... branch is 68%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/curl-probe-no-be... d39e2c6 +/-
src/lib/shields...nsition-lock.ts 86%
src/lib/actions...dbox/rebuild.ts 80%
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 80%
src/lib/state/sandbox.ts 72%
src/lib/shields/index.ts 69%
src/lib/onboard/preflight.ts 69%
src/lib/onboard...er-gpu-patch.ts 59%
src/lib/actions...licy-channel.ts 58%
src/lib/onboard.ts 20%

Updated July 01, 2026 21:22 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-1: Test file monolith grew +60 lines (now 927); fake curl harness duplicated across 8+ retry tests; then add or justify PRA-T1.
Open items: 1 required · 12 warnings · 2 suggestions · 8 test follow-ups
Since last review: 2 prior items resolved · 13 still apply · 4 new items found

Action checklist

  • PRA-1 Fix: Test file monolith grew +60 lines (now 927); fake curl harness duplicated across 8+ retry tests in src/lib/inference/onboard-probes.test.ts:1
  • PRA-2 Resolve or justify: Duplicate buildOpenAiLikeAuthConfig wrapper in onboard-probes.ts and provider-models.ts in src/lib/inference/onboard-probes.ts:53
  • PRA-3 Resolve or justify: @ts-nocheck without GitHub issue reference blocks type safety migration visibility in src/lib/inference/probe-retry.ts:1
  • PRA-4 Resolve or justify: @ts-nocheck without GitHub issue reference blocks type safety migration visibility in src/lib/inference/onboard-probes.ts:1
  • PRA-5 Resolve or justify: Env scrubbing integration tests belong in credential-env.test.ts, not probe.test.ts in src/lib/adapters/http/probe.test.ts:365
  • PRA-6 Resolve or justify: Local restoreEnv duplicates shared test/helpers/env-test-helpers.ts in src/lib/inference/onboard-probes.test.ts:28
  • PRA-7 Resolve or justify: CURL_FORBIDDEN_AUTH_HEADER_PREFIXES misnamed — should be CURL_CREDENTIAL_HEADER_PREFIXES in src/lib/adapters/http/curl-args.ts:97
  • PRA-8 Resolve or justify: Proxy-authorization test duplicates header validation logic instead of parameterizing in src/lib/adapters/http/curl-args.test.ts:57
  • PRA-9 Resolve or justify: probeOllamaAuthProxyHealth not extracted to ollama/probe.ts; bloats local.ts monolith in src/lib/inference/local.ts:420
  • PRA-10 Resolve or justify: Missing concurrent auth-config tmpfile creation test in src/lib/adapters/http/auth-config.test.ts:1
  • PRA-11 Resolve or justify: No test for valid dotted namespace segment against Ollama registry in src/lib/inference/ollama/model-size.test.ts:1
  • PRA-T1 Add or justify test follow-up: Mocked behavioral coverage
  • PRA-T2 Add or justify test follow-up: Mocked behavioral coverage
  • PRA-T3 Add or justify test follow-up: Mocked behavioral coverage
  • PRA-T4 Add or justify test follow-up: Env scrubbing integration tests belong in credential-env.test.ts, not probe.test.ts
  • PRA-T5 Add or justify test follow-up: Local restoreEnv duplicates shared test/helpers/env-test-helpers.ts
  • PRA-T6 Add or justify test follow-up: CURL_FORBIDDEN_AUTH_HEADER_PREFIXES misnamed — should be CURL_CREDENTIAL_HEADER_PREFIXES
  • PRA-T7 Add or justify test follow-up: Proxy-authorization test duplicates header validation logic instead of parameterizing
  • PRA-T8 Add or justify test follow-up: Missing concurrent auth-config tmpfile creation test
  • PRA-14 In-scope improvement: ollama/proxy.ts uses legacy stdin-config approach; should migrate to auth-config.ts in src/lib/inference/ollama/proxy.ts:85
  • PRA-15 In-scope improvement: assertHeaderCarriesNoSecret uses hardcoded prefix array; could reference shared constant in src/lib/adapters/http/curl-args.ts:109

Findings index

ID Severity Category Location Required action
PRA-1 Required architecture src/lib/inference/onboard-probes.test.ts:1 Complete the extraction started in onboard-probes-curl-harness.ts. Split test file into: onboard-probes-anthropic.test.ts, onboard-probes-responses.test.ts, onboard-probes-chat-completions.test.ts, onboard-probes-retry.test.ts, onboard-probes-sandbox-internal.test.ts. Ensure all tests use withFakeCurlProbe and makeFakeCurlScript from the shared harness.
PRA-2 Resolve/justify correctness src/lib/inference/onboard-probes.ts:53 Move the wrapper into auth-config.ts as an internal step of createOpenAiLikeAuthConfig, or export normalizeCredentialValue from auth-config.ts and remove both wrappers. Call createOpenAiLikeAuthConfig directly from both call sites.
PRA-3 Resolve/justify architecture src/lib/inference/probe-retry.ts:1 Add 'Tracked in #XXXX' to the @ts-nocheck comment referencing the GitHub issue that tracks migrating credentials/store, platform, and trace to typed ESM exports.
PRA-4 Resolve/justify architecture src/lib/inference/onboard-probes.ts:1 Add 'Tracked in #XXXX' to the @ts-nocheck comment referencing the GitHub issue that tracks migrating credentials/store, platform, and trace to typed ESM exports.
PRA-5 Resolve/justify tests src/lib/adapters/http/probe.test.ts:365 Move the four tests ('scrubs credential env vars when trustedConfigFiles is supplied', 'strips credential-shaped opts.env entries when trustedConfigFiles is supplied', 'strips credential-shaped opts.env while preserving PATH and NO_PROXY when replaceEnv is true', 'scrubs credential-shaped env even when trustedConfigFiles is not supplied') to credential-env.test.ts under a new describe('integration: curl probe env scrubbing').
PRA-6 Resolve/justify tests src/lib/inference/onboard-probes.test.ts:28 Import restoreEnv from test/helpers/env-test-helpers.ts and replace the local definition. Update all 4 call sites (lines 48, 73, 88, 103 in original).
PRA-7 Resolve/justify tests src/lib/adapters/http/curl-args.ts:97 Rename constant to CURL_CREDENTIAL_HEADER_PREFIXES. Update references in curl-args.ts (line 97, 109) and curl-args.test.ts (line 57, 61).
PRA-8 Resolve/justify tests src/lib/adapters/http/curl-args.test.ts:57 Add 'proxy-authorization:' to CURL_CREDENTIAL_HEADER_PREFIXES (already present) and remove the standalone test. The it.each at line 42 already covers all prefixes.
PRA-9 Resolve/justify architecture src/lib/inference/local.ts:420 Create src/lib/inference/ollama/probe.ts and move probeOllamaAuthProxyHealth, isValidOllamaTagsResponseBody, and related constants there. Update imports in local.ts and local.test.ts.
PRA-10 Resolve/justify tests src/lib/adapters/http/auth-config.test.ts:1 Add test: it('isolates concurrent createBearerAuthConfig calls with same prefix', async () => { const results = await Promise.all([...]); expect(results.map(r => r.args[1])).toHaveLength(3); /* verify all dirs exist then all cleaned up */ })
PRA-11 Resolve/justify tests src/lib/inference/ollama/model-size.test.ts:1 Verify against Ollama registry API documentation. Add test: it('accepts valid dotted namespace segment', () => { expect(buildManifestUrl('acme/foo.bar:tag')).toBe('https://registry.ollama.ai/v2/acme/foo.bar/manifests/tag'\); }) or expect null if not allowed.
PRA-12 Resolve/justify tests src/lib/adapters/http/curl-args.test.ts:1 Add opt-in integration test (e.g., test/integration/curl-argv-validation.test.ts) that spawns real curl with validated argv against a local test server (or httpbin) and verifies the request reaches the server without credentials in argv. Mark as opt-in with NEMOCLAW_RUN_CURL_INTEGRATION=1.
PRA-13 Resolve/justify architecture vitest.config.ts:1 Check vitest.config.ts in PR #6020 for conflicting changes. Coordinate with that PR author. If both target same base, rebase this PR onto #6020 or vice versa.
PRA-14 Improvement architecture src/lib/inference/ollama/proxy.ts:85 Migrate ollama/proxy.ts to use createBearerAuthConfig from auth-config.ts. Replace runCurlWithAuthConfig stdin approach with --config tmpfile. Remove @ts-nocheck once migrated.
PRA-15 Improvement architecture src/lib/adapters/http/curl-args.ts:109 After renaming to CURL_CREDENTIAL_HEADER_PREFIXES, ensure no other file defines its own credential header list.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-1 Required — Test file monolith grew +60 lines (now 927); fake curl harness duplicated across 8+ retry tests

  • Location: src/lib/inference/onboard-probes.test.ts:1
  • Category: architecture
  • Problem: onboard-probes.test.ts exceeded the growth guardrail threshold. The fake curl bash script fixture is duplicated in 8+ retry tests instead of using the newly extracted onboard-probes-curl-harness.ts. File should be split into focused test files.
  • Impact: Hard to maintain; test changes require editing massive file; harness duplication increases bug surface and drift risk.
  • Required action: Complete the extraction started in onboard-probes-curl-harness.ts. Split test file into: onboard-probes-anthropic.test.ts, onboard-probes-responses.test.ts, onboard-probes-chat-completions.test.ts, onboard-probes-retry.test.ts, onboard-probes-sandbox-internal.test.ts. Ensure all tests use withFakeCurlProbe and makeFakeCurlScript from the shared harness.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/inference/onboard-probes.test.ts; grep -c 'fakeBin.*curl' src/lib/inference/onboard-probes.test.ts; ls src/lib/inference/onboard-probes-*.test.ts
  • Missing regression test: Extracted harness should have its own unit tests; split files must preserve existing coverage (run vitest --project cli src/lib/inference/onboard-probes*.test.ts)
  • Done when: The required change is committed and verification passes: wc -l src/lib/inference/onboard-probes.test.ts; grep -c 'fakeBin.*curl' src/lib/inference/onboard-probes.test.ts; ls src/lib/inference/onboard-probes-*.test.ts.
  • Evidence: onboard-probes.test.ts grew from 867 to 927 lines (+60); 8+ fake curl harness copies visible in diff; onboard-probes-curl-harness.ts created but not fully adopted
Review findings by urgency: 1 required fix, 12 items to resolve/justify, 2 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-2 Resolve/justify — Duplicate buildOpenAiLikeAuthConfig wrapper in onboard-probes.ts and provider-models.ts

  • Location: src/lib/inference/onboard-probes.ts:53
  • Category: correctness
  • Problem: Both files define nearly identical buildOpenAiLikeAuthConfig functions that wrap normalizeCredentialValue + createOpenAiLikeAuthConfig. Divergence risk if one is updated without the other.
  • Impact: Maintenance burden; potential inconsistency in credential normalization across probe paths.
  • Recommended action: Move the wrapper into auth-config.ts as an internal step of createOpenAiLikeAuthConfig, or export normalizeCredentialValue from auth-config.ts and remove both wrappers. Call createOpenAiLikeAuthConfig directly from both call sites.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: diff src/lib/inference/onboard-probes.ts src/lib/inference/provider-models.ts | grep -A 10 'buildOpenAiLikeAuthConfig'
  • Missing regression test: Existing tests in onboard-probes.test.ts and provider-models.test.ts already verify credential routing; no new test needed if behavior unchanged.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: diff src/lib/inference/onboard-probes.ts src/lib/inference/provider-models.ts | grep -A 10 'buildOpenAiLikeAuthConfig'.
  • Evidence: onboard-probes.ts:53-55 and provider-models.ts:29-31 define identical 3-line wrappers

PRA-3 Resolve/justify — @ts-nocheck without GitHub issue reference blocks type safety migration visibility

  • Location: src/lib/inference/probe-retry.ts:1
  • Category: architecture
  • Problem: probe-retry.ts has @ts-nocheck with a comment explaining the CommonJS require() bridge but no GitHub issue number tracking the migration to typed ESM.
  • Impact: Type safety regression invisible to reviewers; migration work untracked; future changes may introduce type errors silently.
  • Recommended action: Add 'Tracked in #XXXX' to the @ts-nocheck comment referencing the GitHub issue that tracks migrating credentials/store, platform, and trace to typed ESM exports.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: head -5 src/lib/inference/probe-retry.ts | grep -E 'Tracked in #[0-9]+'
  • Missing regression test: N/A — documentation/tracking item; no test needed.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: head -5 src/lib/inference/probe-retry.ts | grep -E 'Tracked in #[0-9]+'.
  • Evidence: probe-retry.ts:1 has @ts-nocheck with detailed rationale but no issue number

PRA-4 Resolve/justify — @ts-nocheck without GitHub issue reference blocks type safety migration visibility

  • Location: src/lib/inference/onboard-probes.ts:1
  • Category: architecture
  • Problem: onboard-probes.ts has @ts-nocheck with a comment explaining the CommonJS require() bridge but no GitHub issue number tracking the migration to typed ESM.
  • Impact: Type safety regression invisible to reviewers; migration work untracked; future changes may introduce type errors silently.
  • Recommended action: Add 'Tracked in #XXXX' to the @ts-nocheck comment referencing the GitHub issue that tracks migrating credentials/store, platform, and trace to typed ESM exports.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: head -5 src/lib/inference/onboard-probes.ts | grep -E 'Tracked in #[0-9]+'
  • Missing regression test: N/A — documentation/tracking item; no test needed.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: head -5 src/lib/inference/onboard-probes.ts | grep -E 'Tracked in #[0-9]+'.
  • Evidence: onboard-probes.ts:1 has @ts-nocheck with detailed rationale but no issue number

PRA-5 Resolve/justify — Env scrubbing integration tests belong in credential-env.test.ts, not probe.test.ts

  • Location: src/lib/adapters/http/probe.test.ts:365
  • Category: tests
  • Problem: Four integration tests in probe.test.ts (lines 365-480) verify that credential-shaped env vars are stripped from curl child processes. These are integration tests for the security/credential-env boundary, not unit tests for probe.ts.
  • Impact: probe.test.ts monolith growth (+126 lines); test organization obscures security boundary coverage; credential-env.test.ts lacks integration coverage.
  • Recommended action: Move the four tests ('scrubs credential env vars when trustedConfigFiles is supplied', 'strips credential-shaped opts.env entries when trustedConfigFiles is supplied', 'strips credential-shaped opts.env while preserving PATH and NO_PROXY when replaceEnv is true', 'scrubs credential-shaped env even when trustedConfigFiles is not supplied') to credential-env.test.ts under a new describe('integration: curl probe env scrubbing').
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'scrubs credential env vars\|strips credential-shaped opts.env' src/lib/adapters/http/probe.test.ts; wc -l src/lib/security/credential-env.test.ts
  • Missing regression test: Moved tests preserve existing coverage; credential-env.test.ts should run in the same project.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'scrubs credential env vars\|strips credential-shaped opts.env' src/lib/adapters/http/probe.test.ts; wc -l src/lib/security/credential-env.test.ts.
  • Evidence: probe.test.ts lines 365-480 contain 4 integration tests for env scrubbing; credential-env.test.ts has only unit tests for isCredentialShapedName/scrubCredentialEnv

PRA-6 Resolve/justify — Local restoreEnv duplicates shared test/helpers/env-test-helpers.ts

  • Location: src/lib/inference/onboard-probes.test.ts:28
  • Category: tests
  • Problem: onboard-probes.test.ts defines its own restoreEnv function (line 28) identical to the one exported from test/helpers/env-test-helpers.ts, used at 4 call sites.
  • Impact: Inconsistent env restoration if shared helper is updated; code duplication; growth guardrail counts extra if-statements.
  • Recommended action: Import restoreEnv from test/helpers/env-test-helpers.ts and replace the local definition. Update all 4 call sites (lines 48, 73, 88, 103 in original).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'function restoreEnv' src/lib/inference/onboard-probes.test.ts; grep -n 'restoreEnv(' src/lib/inference/onboard-probes.test.ts
  • Missing regression test: N/A — refactoring; existing tests verify env restoration behavior.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'function restoreEnv' src/lib/inference/onboard-probes.test.ts; grep -n 'restoreEnv(' src/lib/inference/onboard-probes.test.ts.
  • Evidence: Line 28 defines local restoreEnv; test/helpers/env-test-helpers.ts exports identical function

PRA-7 Resolve/justify — CURL_FORBIDDEN_AUTH_HEADER_PREFIXES misnamed — should be CURL_CREDENTIAL_HEADER_PREFIXES

  • Location: src/lib/adapters/http/curl-args.ts:97
  • Category: tests
  • Problem: The constant name suggests it forbids auth headers, but it actually identifies credential-carrying headers that must be routed via --config. The name CURL_CREDENTIAL_HEADER_PREFIXES is more accurate.
  • Impact: Misleading naming reduces code clarity; future maintainers may misunderstand the purpose.
  • Recommended action: Rename constant to CURL_CREDENTIAL_HEADER_PREFIXES. Update references in curl-args.ts (line 97, 109) and curl-args.test.ts (line 57, 61).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'CURL_FORBIDDEN_AUTH_HEADER_PREFIXES' src/lib/adapters/http/curl-args.ts src/lib/adapters/http/curl-args.test.ts
  • Missing regression test: N/A — rename only; existing tests cover the behavior.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'CURL_FORBIDDEN_AUTH_HEADER_PREFIXES' src/lib/adapters/http/curl-args.ts src/lib/adapters/http/curl-args.test.ts.
  • Evidence: curl-args.ts:97 defines CURL_FORBIDDEN_AUTH_HEADER_PREFIXES; used in assertHeaderCarriesNoSecret at line 109

PRA-8 Resolve/justify — Proxy-authorization test duplicates header validation logic instead of parameterizing

  • Location: src/lib/adapters/http/curl-args.test.ts:57
  • Category: tests
  • Problem: The test 'rejects an inline proxy-authorization header so proxy credentials cannot reach argv' is a standalone test that duplicates the same assertion pattern used for Authorization and x-api-key headers. Should be merged into the existing it.each over credential header prefixes.
  • Impact: Test duplication; maintenance burden; inconsistent coverage if prefix array changes.
  • Recommended action: Add 'proxy-authorization:' to CURL_CREDENTIAL_HEADER_PREFIXES (already present) and remove the standalone test. The it.each at line 42 already covers all prefixes.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -A 20 'it.each.*credential-shaped.*query parameter' src/lib/adapters/http/curl-args.test.ts; verify proxy-authorization covered
  • Missing regression test: Existing it.each parameterized test covers all prefixes including proxy-authorization; no new test needed.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -A 20 'it.each.*credential-shaped.*query parameter' src/lib/adapters/http/curl-args.test.ts; verify proxy-authorization covered.
  • Evidence: curl-args.test.ts:57-65 is a standalone test for proxy-authorization; it.each at line 42 covers 10 credential-shaped query params but not headers

PRA-9 Resolve/justify — probeOllamaAuthProxyHealth not extracted to ollama/probe.ts; bloats local.ts monolith

  • Location: src/lib/inference/local.ts:420
  • Category: architecture
  • Problem: probeOllamaAuthProxyHealth (lines 420-495) is an Ollama-specific auth proxy health probe that belongs in src/lib/inference/ollama/probe.ts to reduce local.ts size (1258 lines, warning threshold).
  • Impact: local.ts continues growing; Ollama-specific logic mixed with generic local provider helpers; harder to maintain and test in isolation.
  • Recommended action: Create src/lib/inference/ollama/probe.ts and move probeOllamaAuthProxyHealth, isValidOllamaTagsResponseBody, and related constants there. Update imports in local.ts and local.test.ts.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: wc -l src/lib/inference/local.ts; ls src/lib/inference/ollama/probe.ts
  • Missing regression test: local.test.ts already tests probeOllamaAuthProxyHealth; moved tests should pass unchanged.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: wc -l src/lib/inference/local.ts; ls src/lib/inference/ollama/probe.ts.
  • Evidence: local.ts is 1258 lines (+13 delta, warning); probeOllamaAuthProxyHealth is 75 lines of Ollama-specific logic

PRA-10 Resolve/justify — Missing concurrent auth-config tmpfile creation test

  • Location: src/lib/adapters/http/auth-config.test.ts:1
  • Category: tests
  • Problem: No test verifies that concurrent createBearerAuthConfig calls with the same prefix create isolated tmpdirs and don't race on cleanup.
  • Impact: Potential race condition where concurrent probes could delete each other's tmpdirs or leave orphaned credential files.
  • Recommended action: Add test: it('isolates concurrent createBearerAuthConfig calls with same prefix', async () => { const results = await Promise.all([...]); expect(results.map(r => r.args[1])).toHaveLength(3); /* verify all dirs exist then all cleaned up */ })
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: npx vitest run src/lib/adapters/http/auth-config.test.ts -t 'concurrent'
  • Missing regression test: New test: concurrent createBearerAuthConfig calls with same prefix create distinct tmpdirs, all cleaned up on success/failure.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: npx vitest run src/lib/adapters/http/auth-config.test.ts -t 'concurrent'.
  • Evidence: auth-config.test.ts has 13 tests covering success, failure, cleanup, but no concurrency test

PRA-11 Resolve/justify — No test for valid dotted namespace segment against Ollama registry

  • Location: src/lib/inference/ollama/model-size.test.ts:1
  • Category: tests
  • Problem: The OLLAMA_REF_SEGMENT_PATTERN allows dots (^[a-zA-Z0-9][a-zA-Z0-9._-]*$), but no test verifies a valid dotted namespace like 'acme/foo.bar:tag' is accepted or rejected per actual Ollama registry API.
  • Impact: Pattern may over-allow (security) or under-allow (usability) without validation against upstream spec.
  • Recommended action: Verify against Ollama registry API documentation. Add test: it('accepts valid dotted namespace segment', () => { expect(buildManifestUrl('acme/foo.bar:tag')).toBe('https://registry.ollama.ai/v2/acme/foo.bar/manifests/tag'\); }) or expect null if not allowed.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: npx vitest run src/lib/inference/ollama/model-size.test.ts -t 'dotted'
  • Missing regression test: New test for dotted namespace segment acceptance/rejection per Ollama registry spec.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: npx vitest run src/lib/inference/ollama/model-size.test.ts -t 'dotted'.
  • Evidence: model-size.ts:27 pattern allows dots; model-size.test.ts has path traversal tests but no valid dotted segment test

PRA-12 Resolve/justify — No integration test spawns real curl to validate argv parsing matches curl behavior

  • Location: src/lib/adapters/http/curl-args.test.ts:1
  • Category: tests
  • Problem: All argv validation tests use fake-curl harnesses. No test spawns the real curl binary with validated args to confirm the validation matches curl's actual parsing.
  • Impact: Validator could diverge from curl's actual behavior; false confidence in security boundary.
  • Recommended action: Add opt-in integration test (e.g., test/integration/curl-argv-validation.test.ts) that spawns real curl with validated argv against a local test server (or httpbin) and verifies the request reaches the server without credentials in argv. Mark as opt-in with NEMOCLAW_RUN_CURL_INTEGRATION=1.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: ls test/integration/curl-argv-validation.test.ts; NEMOCLAW_RUN_CURL_INTEGRATION=1 npx vitest run test/integration/curl-argv-validation.test.ts
  • Missing regression test: New integration test: real curl spawn with validated argv; verify credentials absent from /proc/$$/cmdline and present in --config file.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: ls test/integration/curl-argv-validation.test.ts; NEMOCLAW_RUN_CURL_INTEGRATION=1 npx vitest run test/integration/curl-argv-validation.test.ts.
  • Evidence: curl-args.test.ts has 11 unit tests with fake argv; no real curl spawn

PRA-13 Resolve/justify — Potential conflict with overlapping PR #6020 on vitest.config.ts

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-14 Improvement — ollama/proxy.ts uses legacy stdin-config approach; should migrate to auth-config.ts

  • Location: src/lib/inference/ollama/proxy.ts:85
  • Category: architecture
  • Problem: ollama/proxy.ts:85 defines curlAuthHeaderConfig writing to stdin (--config -) instead of using createBearerAuthConfig. This is inconsistent with the new security pattern and the file carries @ts-nocheck.
  • Impact: Inconsistent credential routing; legacy code path bypasses new validation/scrubbing; technical debt.
  • Suggested action: Migrate ollama/proxy.ts to use createBearerAuthConfig from auth-config.ts. Replace runCurlWithAuthConfig stdin approach with --config tmpfile. Remove @ts-nocheck once migrated.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: grep -n 'createBearerAuthConfig' src/lib/inference/ollama/proxy.ts
  • Missing regression test: proxy.test.ts (if exists) or onboard-selection.test.ts should verify proxy token still works via --config.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: proxy.ts:85 curlAuthHeaderConfig writes to stdin; runCurlWithAuthConfig uses options.input; @ts-nocheck at top

PRA-15 Improvement — assertHeaderCarriesNoSecret uses hardcoded prefix array; could reference shared constant

  • Location: src/lib/adapters/http/curl-args.ts:109
  • Category: architecture
  • Problem: assertHeaderCarriesNoSecret iterates over CURL_FORBIDDEN_AUTH_HEADER_PREFIXES (to be renamed). The logic is sound but the constant should be shared with any other credential header validation.
  • Impact: Single source of truth for credential header prefixes; already achieved by the constant — just needs rename.
  • Suggested action: After renaming to CURL_CREDENTIAL_HEADER_PREFIXES, ensure no other file defines its own credential header list.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: grep -r 'authorization:\|x-api-key:' src/lib --include='*.ts' | grep -v test | grep -v '.d.ts'
  • Missing regression test: N/A — rename only.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: curl-args.ts:97-101 defines the prefix array; used only in assertHeaderCarriesNoSecret
Simplification opportunities: 8 possible cuts, net -344 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-1 delete (src/lib/inference/onboard-probes.test.ts:1): Inline fake curl bash script fixtures in 8+ retry tests
    • Replacement: Calls to withFakeCurlProbe({ script: makeFakeCurlScript(body), dirPrefix: ... })
    • Net: -200 lines
    • Safety boundary: Must preserve exact argv/env capture behavior for credential leakage tests (Linux /proc test)
  • PRA-2 delete (src/lib/inference/onboard-probes.ts:53): Two duplicate buildOpenAiLikeAuthConfig function definitions
    • Replacement: Single call to createOpenAiLikeAuthConfig(apiKey, options.authMode) after moving normalizeCredentialValue inside auth-config.ts or exporting it
    • Net: -10 lines
    • Safety boundary: Must preserve normalizeCredentialValue trimming behavior for all probe paths
  • PRA-5 delete (src/lib/adapters/http/probe.test.ts:365): Four integration test blocks (approx 120 lines) from probe.test.ts
    • Replacement: Same tests in credential-env.test.ts under describe('integration: curl probe env scrubbing')
    • Net: 0 lines
    • Safety boundary: Must keep the exact spawnSyncImpl mocking and env assertion logic unchanged
  • PRA-6 delete (src/lib/inference/onboard-probes.test.ts:28): Local restoreEnv function (6 lines) and its 4 call sites' inline conditionals
    • Replacement: import { restoreEnv } from '../../../../test/helpers/env-test-helpers'; restoreEnv(name, original)
    • Net: -15 lines
    • Safety boundary: Must preserve exact restore semantics: delete if undefined, else set value
  • PRA-7 shrink (src/lib/adapters/http/curl-args.ts:97): CURL_FORBIDDEN_AUTH_HEADER_PREFIXES constant name
    • Replacement: CURL_CREDENTIAL_HEADER_PREFIXES
    • Net: 0 lines
    • Safety boundary: Must not change the prefix array values or validation logic
  • PRA-8 shrink (src/lib/adapters/http/curl-args.test.ts:57): Standalone proxy-authorization test (9 lines)
    • Replacement: Add 'proxy-authorization:' to CURL_CREDENTIAL_HEADER_PREFIXES and rely on parameterized header test (or add header parameterization)
    • Net: -9 lines
    • Safety boundary: Must preserve the exact error message assertion for proxy-authorization
  • PRA-9 delete (src/lib/inference/local.ts:420): probeOllamaAuthProxyHealth function and isValidOllamaTagsResponseBody helper from local.ts (~80 lines)
    • Replacement: import { probeOllamaAuthProxyHealth } from './ollama/probe'
    • Net: -80 lines
    • Safety boundary: Must preserve exact CurlProbeOptions signature and LocalProviderHealthStatus return shape
  • PRA-14 stdlib (src/lib/inference/ollama/proxy.ts:85): curlAuthHeaderConfig function and stdin-config logic in runCurlWithAuthConfig/runCurlCaptureWithAuthConfig
    • Replacement: createBearerAuthConfig(token) returning { args: ['--config', path], trustedConfigFiles: [path], cleanup }
    • Net: -30 lines
    • Safety boundary: Must preserve proxy token validation flow (probeProxyToken, proxyOwnsPortWithToken) and 401/200 semantics
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Mocked behavioral coverage — Add concurrent createBearerAuthConfig tmpdir isolation test (auth-config.test.ts). Unit tests use fake-curl harnesses extensively for argv/env capture validation; missing real curl integration test and concurrency test for auth-config tmpfile isolation.
  • PRA-T2 Mocked behavioral coverage — Add opt-in real curl argv validation integration test (test/integration/curl-argv-validation.test.ts). Unit tests use fake-curl harnesses extensively for argv/env capture validation; missing real curl integration test and concurrency test for auth-config tmpfile isolation.
  • PRA-T3 Mocked behavioral coverage — Add dotted Ollama namespace segment test (model-size.test.ts). Unit tests use fake-curl harnesses extensively for argv/env capture validation; missing real curl integration test and concurrency test for auth-config tmpfile isolation.
  • PRA-T4 Env scrubbing integration tests belong in credential-env.test.ts, not probe.test.ts — Move the four tests ('scrubs credential env vars when trustedConfigFiles is supplied', 'strips credential-shaped opts.env entries when trustedConfigFiles is supplied', 'strips credential-shaped opts.env while preserving PATH and NO_PROXY when replaceEnv is true', 'scrubs credential-shaped env even when trustedConfigFiles is not supplied') to credential-env.test.ts under a new describe('integration: curl probe env scrubbing').
  • PRA-T5 Local restoreEnv duplicates shared test/helpers/env-test-helpers.ts — Import restoreEnv from test/helpers/env-test-helpers.ts and replace the local definition. Update all 4 call sites (lines 48, 73, 88, 103 in original).
  • PRA-T6 CURL_FORBIDDEN_AUTH_HEADER_PREFIXES misnamed — should be CURL_CREDENTIAL_HEADER_PREFIXES — Rename constant to CURL_CREDENTIAL_HEADER_PREFIXES. Update references in curl-args.ts (line 97, 109) and curl-args.test.ts (line 57, 61).
  • PRA-T7 Proxy-authorization test duplicates header validation logic instead of parameterizing — Add 'proxy-authorization:' to CURL_CREDENTIAL_HEADER_PREFIXES (already present) and remove the standalone test. The it.each at line 42 already covers all prefixes.
  • PRA-T8 Missing concurrent auth-config tmpfile creation test — Add test: it('isolates concurrent createBearerAuthConfig calls with same prefix', async () => { const results = await Promise.all([...]); expect(results.map(r => r.args[1])).toHaveLength(3); /* verify all dirs exist then all cleaned up */ })
Since last review details

Current findings, using the urgency labels above:

PRA-1 Required — Test file monolith grew +60 lines (now 927); fake curl harness duplicated across 8+ retry tests

  • Location: src/lib/inference/onboard-probes.test.ts:1
  • Category: architecture
  • Problem: onboard-probes.test.ts exceeded the growth guardrail threshold. The fake curl bash script fixture is duplicated in 8+ retry tests instead of using the newly extracted onboard-probes-curl-harness.ts. File should be split into focused test files.
  • Impact: Hard to maintain; test changes require editing massive file; harness duplication increases bug surface and drift risk.
  • Required action: Complete the extraction started in onboard-probes-curl-harness.ts. Split test file into: onboard-probes-anthropic.test.ts, onboard-probes-responses.test.ts, onboard-probes-chat-completions.test.ts, onboard-probes-retry.test.ts, onboard-probes-sandbox-internal.test.ts. Ensure all tests use withFakeCurlProbe and makeFakeCurlScript from the shared harness.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/inference/onboard-probes.test.ts; grep -c 'fakeBin.*curl' src/lib/inference/onboard-probes.test.ts; ls src/lib/inference/onboard-probes-*.test.ts
  • Missing regression test: Extracted harness should have its own unit tests; split files must preserve existing coverage (run vitest --project cli src/lib/inference/onboard-probes*.test.ts)
  • Done when: The required change is committed and verification passes: wc -l src/lib/inference/onboard-probes.test.ts; grep -c 'fakeBin.*curl' src/lib/inference/onboard-probes.test.ts; ls src/lib/inference/onboard-probes-*.test.ts.
  • Evidence: onboard-probes.test.ts grew from 867 to 927 lines (+60); 8+ fake curl harness copies visible in diff; onboard-probes-curl-harness.ts created but not fully adopted

PRA-2 Resolve/justify — Duplicate buildOpenAiLikeAuthConfig wrapper in onboard-probes.ts and provider-models.ts

  • Location: src/lib/inference/onboard-probes.ts:53
  • Category: correctness
  • Problem: Both files define nearly identical buildOpenAiLikeAuthConfig functions that wrap normalizeCredentialValue + createOpenAiLikeAuthConfig. Divergence risk if one is updated without the other.
  • Impact: Maintenance burden; potential inconsistency in credential normalization across probe paths.
  • Recommended action: Move the wrapper into auth-config.ts as an internal step of createOpenAiLikeAuthConfig, or export normalizeCredentialValue from auth-config.ts and remove both wrappers. Call createOpenAiLikeAuthConfig directly from both call sites.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: diff src/lib/inference/onboard-probes.ts src/lib/inference/provider-models.ts | grep -A 10 'buildOpenAiLikeAuthConfig'
  • Missing regression test: Existing tests in onboard-probes.test.ts and provider-models.test.ts already verify credential routing; no new test needed if behavior unchanged.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: diff src/lib/inference/onboard-probes.ts src/lib/inference/provider-models.ts | grep -A 10 'buildOpenAiLikeAuthConfig'.
  • Evidence: onboard-probes.ts:53-55 and provider-models.ts:29-31 define identical 3-line wrappers

PRA-3 Resolve/justify — @ts-nocheck without GitHub issue reference blocks type safety migration visibility

  • Location: src/lib/inference/probe-retry.ts:1
  • Category: architecture
  • Problem: probe-retry.ts has @ts-nocheck with a comment explaining the CommonJS require() bridge but no GitHub issue number tracking the migration to typed ESM.
  • Impact: Type safety regression invisible to reviewers; migration work untracked; future changes may introduce type errors silently.
  • Recommended action: Add 'Tracked in #XXXX' to the @ts-nocheck comment referencing the GitHub issue that tracks migrating credentials/store, platform, and trace to typed ESM exports.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: head -5 src/lib/inference/probe-retry.ts | grep -E 'Tracked in #[0-9]+'
  • Missing regression test: N/A — documentation/tracking item; no test needed.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: head -5 src/lib/inference/probe-retry.ts | grep -E 'Tracked in #[0-9]+'.
  • Evidence: probe-retry.ts:1 has @ts-nocheck with detailed rationale but no issue number

PRA-4 Resolve/justify — @ts-nocheck without GitHub issue reference blocks type safety migration visibility

  • Location: src/lib/inference/onboard-probes.ts:1
  • Category: architecture
  • Problem: onboard-probes.ts has @ts-nocheck with a comment explaining the CommonJS require() bridge but no GitHub issue number tracking the migration to typed ESM.
  • Impact: Type safety regression invisible to reviewers; migration work untracked; future changes may introduce type errors silently.
  • Recommended action: Add 'Tracked in #XXXX' to the @ts-nocheck comment referencing the GitHub issue that tracks migrating credentials/store, platform, and trace to typed ESM exports.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: head -5 src/lib/inference/onboard-probes.ts | grep -E 'Tracked in #[0-9]+'
  • Missing regression test: N/A — documentation/tracking item; no test needed.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: head -5 src/lib/inference/onboard-probes.ts | grep -E 'Tracked in #[0-9]+'.
  • Evidence: onboard-probes.ts:1 has @ts-nocheck with detailed rationale but no issue number

PRA-5 Resolve/justify — Env scrubbing integration tests belong in credential-env.test.ts, not probe.test.ts

  • Location: src/lib/adapters/http/probe.test.ts:365
  • Category: tests
  • Problem: Four integration tests in probe.test.ts (lines 365-480) verify that credential-shaped env vars are stripped from curl child processes. These are integration tests for the security/credential-env boundary, not unit tests for probe.ts.
  • Impact: probe.test.ts monolith growth (+126 lines); test organization obscures security boundary coverage; credential-env.test.ts lacks integration coverage.
  • Recommended action: Move the four tests ('scrubs credential env vars when trustedConfigFiles is supplied', 'strips credential-shaped opts.env entries when trustedConfigFiles is supplied', 'strips credential-shaped opts.env while preserving PATH and NO_PROXY when replaceEnv is true', 'scrubs credential-shaped env even when trustedConfigFiles is not supplied') to credential-env.test.ts under a new describe('integration: curl probe env scrubbing').
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'scrubs credential env vars\|strips credential-shaped opts.env' src/lib/adapters/http/probe.test.ts; wc -l src/lib/security/credential-env.test.ts
  • Missing regression test: Moved tests preserve existing coverage; credential-env.test.ts should run in the same project.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'scrubs credential env vars\|strips credential-shaped opts.env' src/lib/adapters/http/probe.test.ts; wc -l src/lib/security/credential-env.test.ts.
  • Evidence: probe.test.ts lines 365-480 contain 4 integration tests for env scrubbing; credential-env.test.ts has only unit tests for isCredentialShapedName/scrubCredentialEnv

PRA-6 Resolve/justify — Local restoreEnv duplicates shared test/helpers/env-test-helpers.ts

  • Location: src/lib/inference/onboard-probes.test.ts:28
  • Category: tests
  • Problem: onboard-probes.test.ts defines its own restoreEnv function (line 28) identical to the one exported from test/helpers/env-test-helpers.ts, used at 4 call sites.
  • Impact: Inconsistent env restoration if shared helper is updated; code duplication; growth guardrail counts extra if-statements.
  • Recommended action: Import restoreEnv from test/helpers/env-test-helpers.ts and replace the local definition. Update all 4 call sites (lines 48, 73, 88, 103 in original).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'function restoreEnv' src/lib/inference/onboard-probes.test.ts; grep -n 'restoreEnv(' src/lib/inference/onboard-probes.test.ts
  • Missing regression test: N/A — refactoring; existing tests verify env restoration behavior.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'function restoreEnv' src/lib/inference/onboard-probes.test.ts; grep -n 'restoreEnv(' src/lib/inference/onboard-probes.test.ts.
  • Evidence: Line 28 defines local restoreEnv; test/helpers/env-test-helpers.ts exports identical function

PRA-7 Resolve/justify — CURL_FORBIDDEN_AUTH_HEADER_PREFIXES misnamed — should be CURL_CREDENTIAL_HEADER_PREFIXES

  • Location: src/lib/adapters/http/curl-args.ts:97
  • Category: tests
  • Problem: The constant name suggests it forbids auth headers, but it actually identifies credential-carrying headers that must be routed via --config. The name CURL_CREDENTIAL_HEADER_PREFIXES is more accurate.
  • Impact: Misleading naming reduces code clarity; future maintainers may misunderstand the purpose.
  • Recommended action: Rename constant to CURL_CREDENTIAL_HEADER_PREFIXES. Update references in curl-args.ts (line 97, 109) and curl-args.test.ts (line 57, 61).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'CURL_FORBIDDEN_AUTH_HEADER_PREFIXES' src/lib/adapters/http/curl-args.ts src/lib/adapters/http/curl-args.test.ts
  • Missing regression test: N/A — rename only; existing tests cover the behavior.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'CURL_FORBIDDEN_AUTH_HEADER_PREFIXES' src/lib/adapters/http/curl-args.ts src/lib/adapters/http/curl-args.test.ts.
  • Evidence: curl-args.ts:97 defines CURL_FORBIDDEN_AUTH_HEADER_PREFIXES; used in assertHeaderCarriesNoSecret at line 109

PRA-8 Resolve/justify — Proxy-authorization test duplicates header validation logic instead of parameterizing

  • Location: src/lib/adapters/http/curl-args.test.ts:57
  • Category: tests
  • Problem: The test 'rejects an inline proxy-authorization header so proxy credentials cannot reach argv' is a standalone test that duplicates the same assertion pattern used for Authorization and x-api-key headers. Should be merged into the existing it.each over credential header prefixes.
  • Impact: Test duplication; maintenance burden; inconsistent coverage if prefix array changes.
  • Recommended action: Add 'proxy-authorization:' to CURL_CREDENTIAL_HEADER_PREFIXES (already present) and remove the standalone test. The it.each at line 42 already covers all prefixes.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -A 20 'it.each.*credential-shaped.*query parameter' src/lib/adapters/http/curl-args.test.ts; verify proxy-authorization covered
  • Missing regression test: Existing it.each parameterized test covers all prefixes including proxy-authorization; no new test needed.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -A 20 'it.each.*credential-shaped.*query parameter' src/lib/adapters/http/curl-args.test.ts; verify proxy-authorization covered.
  • Evidence: curl-args.test.ts:57-65 is a standalone test for proxy-authorization; it.each at line 42 covers 10 credential-shaped query params but not headers

PRA-9 Resolve/justify — probeOllamaAuthProxyHealth not extracted to ollama/probe.ts; bloats local.ts monolith

  • Location: src/lib/inference/local.ts:420
  • Category: architecture
  • Problem: probeOllamaAuthProxyHealth (lines 420-495) is an Ollama-specific auth proxy health probe that belongs in src/lib/inference/ollama/probe.ts to reduce local.ts size (1258 lines, warning threshold).
  • Impact: local.ts continues growing; Ollama-specific logic mixed with generic local provider helpers; harder to maintain and test in isolation.
  • Recommended action: Create src/lib/inference/ollama/probe.ts and move probeOllamaAuthProxyHealth, isValidOllamaTagsResponseBody, and related constants there. Update imports in local.ts and local.test.ts.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: wc -l src/lib/inference/local.ts; ls src/lib/inference/ollama/probe.ts
  • Missing regression test: local.test.ts already tests probeOllamaAuthProxyHealth; moved tests should pass unchanged.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: wc -l src/lib/inference/local.ts; ls src/lib/inference/ollama/probe.ts.
  • Evidence: local.ts is 1258 lines (+13 delta, warning); probeOllamaAuthProxyHealth is 75 lines of Ollama-specific logic

PRA-10 Resolve/justify — Missing concurrent auth-config tmpfile creation test

  • Location: src/lib/adapters/http/auth-config.test.ts:1
  • Category: tests
  • Problem: No test verifies that concurrent createBearerAuthConfig calls with the same prefix create isolated tmpdirs and don't race on cleanup.
  • Impact: Potential race condition where concurrent probes could delete each other's tmpdirs or leave orphaned credential files.
  • Recommended action: Add test: it('isolates concurrent createBearerAuthConfig calls with same prefix', async () => { const results = await Promise.all([...]); expect(results.map(r => r.args[1])).toHaveLength(3); /* verify all dirs exist then all cleaned up */ })
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: npx vitest run src/lib/adapters/http/auth-config.test.ts -t 'concurrent'
  • Missing regression test: New test: concurrent createBearerAuthConfig calls with same prefix create distinct tmpdirs, all cleaned up on success/failure.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: npx vitest run src/lib/adapters/http/auth-config.test.ts -t 'concurrent'.
  • Evidence: auth-config.test.ts has 13 tests covering success, failure, cleanup, but no concurrency test

PRA-11 Resolve/justify — No test for valid dotted namespace segment against Ollama registry

  • Location: src/lib/inference/ollama/model-size.test.ts:1
  • Category: tests
  • Problem: The OLLAMA_REF_SEGMENT_PATTERN allows dots (^[a-zA-Z0-9][a-zA-Z0-9._-]*$), but no test verifies a valid dotted namespace like 'acme/foo.bar:tag' is accepted or rejected per actual Ollama registry API.
  • Impact: Pattern may over-allow (security) or under-allow (usability) without validation against upstream spec.
  • Recommended action: Verify against Ollama registry API documentation. Add test: it('accepts valid dotted namespace segment', () => { expect(buildManifestUrl('acme/foo.bar:tag')).toBe('https://registry.ollama.ai/v2/acme/foo.bar/manifests/tag'\); }) or expect null if not allowed.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: npx vitest run src/lib/inference/ollama/model-size.test.ts -t 'dotted'
  • Missing regression test: New test for dotted namespace segment acceptance/rejection per Ollama registry spec.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: npx vitest run src/lib/inference/ollama/model-size.test.ts -t 'dotted'.
  • Evidence: model-size.ts:27 pattern allows dots; model-size.test.ts has path traversal tests but no valid dotted segment test

PRA-12 Resolve/justify — No integration test spawns real curl to validate argv parsing matches curl behavior

  • Location: src/lib/adapters/http/curl-args.test.ts:1
  • Category: tests
  • Problem: All argv validation tests use fake-curl harnesses. No test spawns the real curl binary with validated args to confirm the validation matches curl's actual parsing.
  • Impact: Validator could diverge from curl's actual behavior; false confidence in security boundary.
  • Recommended action: Add opt-in integration test (e.g., test/integration/curl-argv-validation.test.ts) that spawns real curl with validated argv against a local test server (or httpbin) and verifies the request reaches the server without credentials in argv. Mark as opt-in with NEMOCLAW_RUN_CURL_INTEGRATION=1.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: ls test/integration/curl-argv-validation.test.ts; NEMOCLAW_RUN_CURL_INTEGRATION=1 npx vitest run test/integration/curl-argv-validation.test.ts
  • Missing regression test: New integration test: real curl spawn with validated argv; verify credentials absent from /proc/$$/cmdline and present in --config file.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: ls test/integration/curl-argv-validation.test.ts; NEMOCLAW_RUN_CURL_INTEGRATION=1 npx vitest run test/integration/curl-argv-validation.test.ts.
  • Evidence: curl-args.test.ts has 11 unit tests with fake argv; no real curl spawn

PRA-13 Resolve/justify — Potential conflict with overlapping PR #6020 on vitest.config.ts

PRA-14 Improvement — ollama/proxy.ts uses legacy stdin-config approach; should migrate to auth-config.ts

  • Location: src/lib/inference/ollama/proxy.ts:85
  • Category: architecture
  • Problem: ollama/proxy.ts:85 defines curlAuthHeaderConfig writing to stdin (--config -) instead of using createBearerAuthConfig. This is inconsistent with the new security pattern and the file carries @ts-nocheck.
  • Impact: Inconsistent credential routing; legacy code path bypasses new validation/scrubbing; technical debt.
  • Suggested action: Migrate ollama/proxy.ts to use createBearerAuthConfig from auth-config.ts. Replace runCurlWithAuthConfig stdin approach with --config tmpfile. Remove @ts-nocheck once migrated.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: grep -n 'createBearerAuthConfig' src/lib/inference/ollama/proxy.ts
  • Missing regression test: proxy.test.ts (if exists) or onboard-selection.test.ts should verify proxy token still works via --config.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: proxy.ts:85 curlAuthHeaderConfig writes to stdin; runCurlWithAuthConfig uses options.input; @ts-nocheck at top

PRA-15 Improvement — assertHeaderCarriesNoSecret uses hardcoded prefix array; could reference shared constant

  • Location: src/lib/adapters/http/curl-args.ts:109
  • Category: architecture
  • Problem: assertHeaderCarriesNoSecret iterates over CURL_FORBIDDEN_AUTH_HEADER_PREFIXES (to be renamed). The logic is sound but the constant should be shared with any other credential header validation.
  • Impact: Single source of truth for credential header prefixes; already achieved by the constant — just needs rename.
  • Suggested action: After renaming to CURL_CREDENTIAL_HEADER_PREFIXES, ensure no other file defines its own credential header list.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: grep -r 'authorization:\|x-api-key:' src/lib --include='*.ts' | grep -v test | grep -v '.d.ts'
  • Missing regression test: N/A — rename only.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: curl-args.ts:97-101 defines the prefix array; used only in assertHeaderCarriesNoSecret

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@laitingsheng laitingsheng added security NV QA Bugs found by the NVIDIA QA Team area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression labels Jun 29, 2026
Comment thread src/lib/adapters/http/auth-config.test.ts Fixed
Comment thread src/lib/inference/provider-models.test.ts Fixed
@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: cloud-onboard, credential-sanitization, openclaw-inference-switch, security-posture, ollama-proxy-e2e
Optional E2E: model-router-provider-routed-inference-e2e, hermes-sandbox-secret-boundary

Dispatch hint: cloud-onboard,credential-sanitization,openclaw-inference-switch,security-posture

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • cloud-onboard (high): Required because hosted onboarding inference probe and provider-selection behavior changed. This exercises real install/onboard plus hosted inference readiness through the live cloud onboarding flow.
  • credential-sanitization (high): Required because the PR changes credential containment for curl argv/config files and spawned environments. This live E2E verifies credentials remain sanitized across install, onboarding, sandbox state, and artifact evidence.
  • openclaw-inference-switch (high): Required because inference routing/probe construction changed for OpenAI-compatible and Anthropic-compatible paths. This validates live OpenClaw inference switching and assistant responses through the real sandbox boundary.
  • security-posture (high): Required because the PR changes security-boundary behavior for credential-bearing HTTP probes and subprocess environments. This gives live sandbox posture coverage for OpenClaw/Hermes after the credential-boundary changes.
  • ollama-proxy-e2e (medium): Required because local Ollama auth-proxy health now uses the shared curl auth-config/trusted-config path. The dedicated Ollama Auth Proxy E2E validates token auth, real inference, persistence, recovery, and container reachability end-to-end.

Optional E2E

  • model-router-provider-routed-inference-e2e (high): Useful adjacent confidence for provider-routed inference after changes to provider model and onboard probe code, but less directly targeted than cloud-onboard and openclaw-inference-switch.
  • hermes-sandbox-secret-boundary (high): Optional additional confidence for secret-boundary behavior in a live Hermes sandbox after credential env-scrubbing changes. Credential-sanitization remains the more targeted required guard.

New E2E recommendations

  • curl-probe-credential-argv-env-boundary (high): Existing live credential E2E covers sandbox/state/artifact sanitization, but there does not appear to be a focused live guard that inspects a real spawned curl probe to prove hosted-provider secrets are absent from argv, query strings, and subprocess env while trusted --config tmpfiles are used.
    • Suggested test: Add a focused live E2E target, e.g. curl-probe-credential-boundary, that runs a fake compatible inference endpoint during onboarding and asserts probe requests succeed while process argv/env evidence contains no API key material.
  • kimi-health-probe-auth-config (medium): Kimi/NVIDIA health now has special behavior for credential lookup failures and auth-config tmpfile cleanup. Unit coverage is present, but a live targeted check would reduce risk of regressions in status rendering and credential handling for the real hosted Kimi route.
    • Suggested test: Add a Kimi health live or hermetic-compatible E2E that exercises nemoclaw status/health for moonshotai/kimi-k2.6, verifies unhealthy/not-probed labels, and asserts bearer credentials are never emitted inline.

Dispatch hint

  • Workflow: .github/workflows/e2e.yaml
  • jobs input: cloud-onboard,credential-sanitization,openclaw-inference-switch,security-posture

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: inference-routing-vitest, cloud-inference-vitest, bedrock-runtime-compatible-anthropic-vitest
Optional Vitest E2E scenarios: ubuntu-repo-cloud-openclaw

Dispatch required Vitest E2E scenarios:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=inference-routing-vitest
  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=cloud-inference-vitest
  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=bedrock-runtime-compatible-anthropic-vitest

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • inference-routing-vitest: The PR changes curl argument credential validation plus onboarding/provider-model inference probes. The inference-routing live Vitest job directly exercises onboarding inference-routing failure classification, credential non-exposure, and compatible endpoint validation paths that can be affected by moving provider secrets out of argv and into trusted curl --config files.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=inference-routing-vitest
  • cloud-inference-vitest: The PR changes successful cloud/compatible inference probing and provider model catalog auth handling. The cloud-inference live Vitest job runs install.sh/onboard with a hosted inference credential and verifies the resulting sandbox inference.local route, providing the primary live success-path coverage for the OpenAI-compatible/NVIDIA hosted probe changes.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=cloud-inference-vitest
  • bedrock-runtime-compatible-anthropic-vitest: The PR changes Anthropic-style x-api-key curl auth routing in onboarding probes and provider model fetches. The Bedrock Runtime compatible Anthropic live Vitest job is the targeted live Anthropic-compatible provider path and should verify that the Anthropic auth/probe changes still onboard and route through the adapter boundary.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=bedrock-runtime-compatible-anthropic-vitest

Optional Vitest E2E scenarios

  • ubuntu-repo-cloud-openclaw: Adjacent registry-scenario coverage for the standard Ubuntu Docker OpenClaw hosted onboarding path. Useful if maintainers want the typed scenario matrix path in addition to the targeted free-standing inference jobs.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw

Relevant changed files

  • src/lib/adapters/http/auth-config.ts
  • src/lib/adapters/http/curl-args.ts
  • src/lib/inference/onboard-probes.ts
  • src/lib/inference/provider-models.ts

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Source-of-truth review needed: OpenAI-like auth-config setup failure handling.
Open items: 0 required · 9 warnings · 1 suggestion · 8 test follow-ups
Since last review: 1 prior item resolved · 8 still apply · 2 new items found

Action checklist

  • PRA-1 Resolve or justify: Source-of-truth review needed: OpenAI-like auth-config setup failure handling
  • PRA-2 Resolve or justify: Credential-shaped camelCase names still bypass query/env scrubbing in src/lib/security/credential-env.ts:13
  • PRA-3 Resolve or justify: Inline auth-header validation misses whitespace-before-colon variants in src/lib/adapters/http/curl-args.ts:105
  • PRA-4 Resolve or justify: Query-param auth still lacks proof for reserved API-key characters in src/lib/adapters/http/auth-config.ts:48
  • PRA-5 Resolve or justify: Streaming curl wrappers still lack direct env-scrub coverage in src/lib/adapters/http/probe.test.ts:492
  • PRA-6 Resolve or justify: OpenAI-like auth-config setup failure path still lacks direct normal and strict-path coverage in src/lib/inference/onboard-probes.ts:856
  • PRA-7 Resolve or justify: Test-only helpers are still included in the production src build in src/lib/adapters/http/auth-config-test-helpers.ts:11
  • PRA-8 Resolve or justify: Security test additions still grow large test hotspots in src/lib/inference/onboard-probes.test.ts
  • PRA-9 Resolve or justify: Issue-specific runtime validation remains needed for visible host process boundaries
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Streaming curl wrappers still lack direct env-scrub coverage
  • PRA-T7 Add or justify test follow-up: OpenAI-like auth-config setup failure path still lacks direct normal and strict-path coverage
  • PRA-T8 Add or justify test follow-up: Issue-specific runtime validation remains needed for visible host process boundaries
  • PRA-10 In-scope improvement: New probe retry helper keeps a file-level @ts-nocheck in src/lib/inference/probe-retry.ts:9

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify security src/lib/security/credential-env.ts:13 Normalize names before matching, for example by checking a separator-free lowercase form and a camelCase-split form, or add explicit covered compounds such as `sessionToken` and `bearerToken` while keeping benign exceptions explicit.
PRA-3 Resolve/justify security src/lib/adapters/http/curl-args.ts:105 Parse the header name before the first colon, trim surrounding whitespace, normalize it to lowercase, and reject `authorization`, `proxy-authorization`, `x-api-key`, and `x-goog-api-key` regardless of whitespace around the colon.
PRA-4 Resolve/justify correctness src/lib/adapters/http/auth-config.ts:48 Either encode the query value before writing the `url-query` directive if that is the intended curl contract, or add a faithful behavioral test with a local capture server or fake curl-config parser proving the provider receives the exact intended `key` value and the key never appears in argv.
PRA-5 Resolve/justify tests src/lib/adapters/http/probe.test.ts:492 Add focused tests for both streaming wrappers that pass a trusted config file, set credential-shaped values in `process.env` and `opts.env`, capture `options.env` in `spawnSyncImpl`, and assert secrets are absent while benign variables remain.
PRA-6 Resolve/justify tests src/lib/inference/onboard-probes.ts:856 Add direct tests that force `createOpenAiLikeAuthConfig()` or its filesystem boundary to throw, call `probeOpenAiLikeEndpoint()` on both the normal path and with `requireChatCompletionsToolCalling: true`, assert no curl spawn occurs, and assert the returned failure entry uses name `curl auth config` with `httpStatus: 0` and `curlStatus: 0`.
PRA-7 Resolve/justify correctness src/lib/adapters/http/auth-config-test-helpers.ts:11 Move these helpers to a test-only location such as `test/helpers/`, or otherwise rename/exclude them so `tsconfig.src.json` does not emit them. Keep the assertions and fake-curl coverage, but keep the helper modules outside the production source graph.
PRA-8 Resolve/justify architecture src/lib/inference/onboard-probes.test.ts Move cohesive fake-curl/auth-config/process-list helpers and any standalone scenarios into focused behavior-specific test files or `test/helpers`, while preserving the real subprocess, trusted-config, env-scrub, and `/proc` assertions.
PRA-9 Resolve/justify tests Add or identify targeted runtime/integration validation for the changed behavior on at least one host/container runtime where the curl process is visible to the host. Keep the validation behavior-specific: onboard with a canary API key, inspect host process arguments while validation curl runs, and assert the canary is absent while `--config` is present.
PRA-10 Improvement architecture src/lib/inference/probe-retry.ts:9 Define minimal local types for the probe result, probe callback object, and trace module shape, then remove the file-level `@ts-nocheck` while preserving `module.exports` if callers still require it.
Review findings by urgency: 0 required fixes, 9 items to resolve/justify, 1 in-scope improvement

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: OpenAI-like auth-config setup failure handling

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Provider-model and Anthropic setup-failure tests exist, but direct OpenAI-like normal and strict Chat Completions setup-failure tests were not found.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `openAiLikeFailureFromError()` returns a `curl auth config` failure, but `onboard-probes.test.ts` does not directly force this boundary for both normal and `requireChatCompletionsToolCalling` paths.

PRA-2 Resolve/justify — Credential-shaped camelCase names still bypass query/env scrubbing

  • Location: src/lib/security/credential-env.ts:13
  • Category: security
  • Problem: The shared credential-name matcher catches separator-delimited names and several compounds, but common camelCase credential names such as `sessionToken` and `bearerToken` still do not match. The same helper gates both query-parameter rejection in curl argv validation and curl child environment scrubbing.
  • Impact: A future provider URL or environment variable using a missed camelCase credential name can still place a secret into the curl argv URL or into the spawned curl process environment, undermining this PR's primary credential-leak boundary.
  • Recommended action: Normalize names before matching, for example by checking a separator-free lowercase form and a camelCase-split form, or add explicit covered compounds such as `sessionToken` and `bearerToken` while keeping benign exceptions explicit.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/security/credential-env.ts` and evaluate `isCredentialShapedName("sessionToken")` and `isCredentialShapedName("bearerToken")`; both currently fail the regex shape because `token` is not preceded by start, `_`, or `-`.
  • Missing regression test: Add tests that `validateCurlProbeArgs` rejects `?sessionToken=...` and `?bearerToken=...`, and that `scrubCredentialEnv` / `buildScrubbedCurlProbeEnv` strip `sessionToken` and `bearerToken` while preserving benign variables such as `PATH` and `NO_PROXY`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/security/credential-env.ts` and evaluate `isCredentialShapedName("sessionToken")` and `isCredentialShapedName("bearerToken")`; both currently fail the regex shape because `token` is not preceded by start, `_`, or `-`.
  • Evidence: `CREDENTIAL_SHAPED_NAME_PATTERN` includes `authtoken`, `refreshtoken`, and `accesstoken`, but not `sessiontoken` or `bearertoken`; `credential-env.test.ts` covers `accessKey` and `refreshToken` but not these common token names.

PRA-3 Resolve/justify — Inline auth-header validation misses whitespace-before-colon variants

  • Location: src/lib/adapters/http/curl-args.ts:105
  • Category: security
  • Problem: `assertHeaderCarriesNoSecret()` lowercases and `trimStart()`s the full header value, then checks prefixes such as `authorization:`. Header names with optional whitespace before the colon, such as `Authorization : Bearer secret`, do not start with the forbidden prefix.
  • Impact: A caller regression can still pass a credential-bearing `-H` or `--proxy-header` value in argv if the header name is formatted with whitespace before the colon, exposing secrets to host process-list inspection.
  • Recommended action: Parse the header name before the first colon, trim surrounding whitespace, normalize it to lowercase, and reject `authorization`, `proxy-authorization`, `x-api-key`, and `x-goog-api-key` regardless of whitespace around the colon.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `assertHeaderCarriesNoSecret()` and check the path for `validateCurlProbeArgs(["-sS", "-H", "Authorization : Bearer secret", "https://example.test"\]\)\`; the current `startsWith("authorization:")` check does not catch it.
  • Missing regression test: Add negative tests for `Authorization : Bearer ...`, `Proxy-Authorization : Basic ...`, `x-api-key : ...`, and `x-goog-api-key : ...` through both `-H` and `--proxy-header` where applicable.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `assertHeaderCarriesNoSecret()` and check the path for `validateCurlProbeArgs(["-sS", "-H", "Authorization : Bearer secret", "https://example.test"\]\)\`; the current `startsWith("authorization:")` check does not catch it.
  • Evidence: `curl-args.test.ts` covers `Authorization: Bearer`, `x-api-key:`, and `Proxy-Authorization:` without whitespace before the colon, but not the whitespace-before-colon forms.

PRA-4 Resolve/justify — Query-param auth still lacks proof for reserved API-key characters

  • Location: src/lib/adapters/http/auth-config.ts:48
  • Category: correctness
  • Problem: `createQueryParamAuthConfig()` writes `url-query = "key=${value}"` after curl-config quoting, but does not URL-encode query values or prove curl preserves reserved characters as one exact parameter value.
  • Impact: API keys containing spaces, `&`, `=`, `%`, or other reserved characters could be split or transformed by curl query construction. That can break Gemini-style auth and may reintroduce surprising credential handling even though the key is no longer in argv.
  • Recommended action: Either encode the query value before writing the `url-query` directive if that is the intended curl contract, or add a faithful behavioral test with a local capture server or fake curl-config parser proving the provider receives the exact intended `key` value and the key never appears in argv.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `formatCurlConfigEntry()` in `src/lib/adapters/http/auth-config.ts` and the query-param tests in `provider-models.test.ts`; they only cover simple `AIzaFakeKey123` and `secret key`, not reserved separators such as `&` or `=`.
  • Missing regression test: Add a test named like `query-param auth preserves reserved API-key characters without putting them in argv`, using a value such as `secret key&scope=a=b` and asserting the received `key` parameter is exactly that value.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `formatCurlConfigEntry()` in `src/lib/adapters/http/auth-config.ts` and the query-param tests in `provider-models.test.ts`; they only cover simple `AIzaFakeKey123` and `secret key`, not reserved separators such as `&` or `=`.
  • Evidence: `provider-models.test.ts` asserts `url-query = "key=AIzaFakeKey123"`; `onboard-probes.test.ts` asserts a retry does not put `secret key` in argv, but neither proves reserved-character behavior at the curl query boundary.

PRA-5 Resolve/justify — Streaming curl wrappers still lack direct env-scrub coverage

  • Location: src/lib/adapters/http/probe.test.ts:492
  • Category: tests
  • Problem: The production code now routes `runChatCompletionsStreamingProbe()` and `runStreamingEventProbe()` through `resolveCurlProbeSpawnEnv()`, but the added env-scrub tests exercise only `runCurlProbe()`. The streaming sections still focus on SSE parsing, timeouts, and traces.
  • Impact: A future refactor can accidentally drop env scrubbing from one streaming wrapper while the current non-streaming env tests continue to pass, leaving streaming validation probes able to leak credential-shaped environment variables to curl.
  • Recommended action: Add focused tests for both streaming wrappers that pass a trusted config file, set credential-shaped values in `process.env` and `opts.env`, capture `options.env` in `spawnSyncImpl`, and assert secrets are absent while benign variables remain.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search `src/lib/adapters/http/probe.test.ts` for `spawnedEnv`; all current env assertions are in the `runCurlProbe` helper section, while later `runChatCompletionsStreamingProbe` and `runStreamingEventProbe` tests use `mockStreaming()` without inspecting `options.env`.
  • Missing regression test: Add tests named `chat streaming probe scrubs process and opts env while using trusted config` and `responses streaming probe scrubs process and opts env while using trusted config`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search `src/lib/adapters/http/probe.test.ts` for `spawnedEnv`; all current env assertions are in the `runCurlProbe` helper section, while later `runChatCompletionsStreamingProbe` and `runStreamingEventProbe` tests use `mockStreaming()` without inspecting `options.env`.
  • Evidence: Grep found direct `spawnedEnv` assertions around lines 365-485 for `runCurlProbe`, but no equivalent `spawnedEnv` assertions in the streaming wrapper test blocks beginning around lines 492 and 600.

PRA-6 Resolve/justify — OpenAI-like auth-config setup failure path still lacks direct normal and strict-path coverage

  • Location: src/lib/inference/onboard-probes.ts:856
  • Category: tests
  • Problem: `probeOpenAiLikeEndpoint()` catches auth-config setup errors and maps them through `openAiLikeFailureFromError()`, but the changed tests do not directly force that boundary to throw for both the normal OpenAI-like path and the `requireChatCompletionsToolCalling` path.
  • Impact: This is a source-boundary failure path for the credential tmpfile. Without direct tests, a future change can spawn curl after auth-config setup fails, throw uncaught into the onboard wizard, or return a failure shape that callers do not understand.
  • Recommended action: Add direct tests that force `createOpenAiLikeAuthConfig()` or its filesystem boundary to throw, call `probeOpenAiLikeEndpoint()` on both the normal path and with `requireChatCompletionsToolCalling: true`, assert no curl spawn occurs, and assert the returned failure entry uses name `curl auth config` with `httpStatus: 0` and `curlStatus: 0`.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search `src/lib/inference/onboard-probes.test.ts` for `mkdtemp`, `curl auth config`, or `auth-config setup`; provider-model and Anthropic setup-failure tests exist, but equivalent OpenAI-like normal/strict endpoint tests were not found.
  • Missing regression test: Add tests named `OpenAI-like probe reports curl auth config setup failure without spawning curl` and `strict Chat Completions probe reports curl auth config setup failure without spawning curl`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search `src/lib/inference/onboard-probes.test.ts` for `mkdtemp`, `curl auth config`, or `auth-config setup`; provider-model and Anthropic setup-failure tests exist, but equivalent OpenAI-like normal/strict endpoint tests were not found.
  • Evidence: `openAiLikeFailureFromError()` exists in `onboard-probes.ts`, and `probeOpenAiLikeEndpoint()` catches at line 856, but grep of `onboard-probes.test.ts` found strict retry behavior tests rather than auth-config setup-failure tests.

PRA-7 Resolve/justify — Test-only helpers are still included in the production src build

  • Location: src/lib/adapters/http/auth-config-test-helpers.ts:11
  • Category: correctness
  • Problem: `auth-config-test-helpers.ts` lives under `src/lib`, imports `expect` from `vitest`, and is imported only by tests. The new `onboard-probes-curl-harness.ts` is also a fake-curl test harness under `src/lib`. `tsconfig.src.json` includes `src/**/*.ts` and excludes only `src/**/*.test.ts`, so both helpers are part of the production build graph.
  • Impact: The published CLI can ship test-only modules, including a dev-only `vitest` dependency edge and a fake-curl shell-script harness. That increases production surface area and can break packaging, consumers, or static dependency checks despite no runtime code needing these helpers.
  • Recommended action: Move these helpers to a test-only location such as `test/helpers/`, or otherwise rename/exclude them so `tsconfig.src.json` does not emit them. Keep the assertions and fake-curl coverage, but keep the helper modules outside the production source graph.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `tsconfig.src.json` and confirm `include: ["src/**/*.ts"]` with only `src/**/*.test.ts` excluded; then grep for `auth-config-test-helpers` and `onboard-probes-curl-harness` to confirm they are only used by tests.
  • Missing regression test: Add or identify a package/build-shape check that verifies production `dist` does not contain `auth-config-test-helpers` or `onboard-probes-curl-harness`, and that emitted production files do not import `vitest`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `tsconfig.src.json` and confirm `include: ["src/**/*.ts"]` with only `src/**/*.test.ts` excluded; then grep for `auth-config-test-helpers` and `onboard-probes-curl-harness` to confirm they are only used by tests.
  • Evidence: `src/lib/adapters/http/auth-config-test-helpers.ts` imports `{ expect } from "vitest"`; `src/lib/inference/onboard-probes-curl-harness.ts` creates fake curl scripts and mutates test env; both are matched by `tsconfig.src.json`.

PRA-8 Resolve/justify — Security test additions still grow large test hotspots

  • Location: src/lib/inference/onboard-probes.test.ts
  • Category: architecture
  • Problem: The PR adds important security regression coverage, but the changed large test files still grow substantially: `probe.test.ts` by about 126 lines, `onboard-probes.test.ts` by about 60 lines, and `local.test.ts` by about 27 lines. Some extraction happened, but cohesive helpers were placed under `src` rather than a test-only helper area.
  • Impact: These already-large tests become harder to review and maintain, and the current extraction strategy creates production build-surface drift. Future security fixes in the same area will be harder to verify without accidentally weakening the real subprocess and `/proc` assertions.
  • Recommended action: Move cohesive fake-curl/auth-config/process-list helpers and any standalone scenarios into focused behavior-specific test files or `test/helpers`, while preserving the real subprocess, trusted-config, env-scrub, and `/proc` assertions.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Compare the monolith deltas for `src/lib/adapters/http/probe.test.ts`, `src/lib/inference/onboard-probes.test.ts`, and `src/lib/inference/local.test.ts`, then inspect whether the new helper modules live in a test-only path.
  • Missing regression test: Existing behavioral tests should be retained; add no new behavior-only test for this finding, but verify after refactor that the same named tests for process-list secrecy, auth-config cleanup, and env scrubbing still exist.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Compare the monolith deltas for `src/lib/adapters/http/probe.test.ts`, `src/lib/inference/onboard-probes.test.ts`, and `src/lib/inference/local.test.ts`, then inspect whether the new helper modules live in a test-only path.
  • Evidence: Synthetic drift context reports blocker-level growth for `probe.test.ts`, `onboard-probes.test.ts`, and `local.test.ts`; the new fake-curl harness was extracted to `src/lib/inference/onboard-probes-curl-harness.ts` instead of a test-only path.

PRA-9 Resolve/justify — Issue-specific runtime validation remains needed for visible host process boundaries

  • Location: not file-specific
  • Category: tests
  • Problem: The PR adds a strong Linux fake-curl test that records `/proc/$$/cmdline`, `/proc/$$/environ`, and `ps auxww`, but the linked issue is specifically about host-visible process lists on ubuntu24-gpu/k3s and WSL2/Docker Desktop runtime configurations.
  • Impact: A local fake-curl test proves the argv/env construction boundary, but it does not by itself prove the full onboard/runtime path on at least one affected process-visibility runtime where host `ps auxww` originally observed the key.
  • Recommended action: Add or identify targeted runtime/integration validation for the changed behavior on at least one host/container runtime where the curl process is visible to the host. Keep the validation behavior-specific: onboard with a canary API key, inspect host process arguments while validation curl runs, and assert the canary is absent while `--config` is present.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the PR for a behavior-specific runtime validation artifact or test definition that runs the changed onboard probe path on a visible-process host; do not rely on external job pass/fail status as review evidence.
  • Missing regression test: Add a runtime validation named `onboard NVIDIA cloud validation keeps API key out of host ps auxww on a visible-process runtime` or document an equivalent checked-in harness.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the PR for a behavior-specific runtime validation artifact or test definition that runs the changed onboard probe path on a visible-process host; do not rely on external job pass/fail status as review evidence.
  • Evidence: The linked issue names `ps auxww` on ubuntu24-gpu and WSL2; synthetic `testDepth` reports `runtime_validation_recommended` for `auth-config`, `curl-args`, `probe`, `health`, `local`, `model-size`, and onboarding helper surfaces.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-10 Improvement — New probe retry helper keeps a file-level @ts-nocheck

  • Location: src/lib/inference/probe-retry.ts:9
  • Category: architecture
  • Problem: `probe-retry.ts` is a new shared retry module but disables TypeScript checking for the entire file. The surface is small enough to type locally while preserving the CommonJS export shape used by `onboard-probes.ts`.
  • Impact: Type-checking is disabled around retry predicates and callback contracts that influence onboarding validation behavior. Future drift in result shape or trace module calls can go unnoticed.
  • Suggested action: Define minimal local types for the probe result, probe callback object, and trace module shape, then remove the file-level `@ts-nocheck` while preserving `module.exports` if callers still require it.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Open `src/lib/inference/probe-retry.ts` and confirm the leading `// @ts-nocheck` is removed and the exported functions still match existing CommonJS imports from `onboard-probes.ts`.
  • Missing regression test: Existing retry tests in `onboard-probes.test.ts` cover behavior; add or identify a type-safety hotspot check that fails if this new helper reintroduces file-level `@ts-nocheck`.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: `src/lib/inference/probe-retry.ts:9` contains `// @ts-nocheck — onboard-probes.ts is CommonJS-style and require()-based`.
Simplification opportunities: 1 possible cut

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-8 shrink (src/lib/inference/onboard-probes.test.ts): Test-only fake-curl/auth-config helper code from production `src/lib` and excess duplicated setup in large test files.
    • Replacement: Use `test/helpers` modules and focused behavior test files that import those helpers.
    • Safety boundary: Do not remove or weaken trust-boundary validation, credential redaction, real subprocess spawning, `--config` permission assertions, or `/proc`/`ps auxww` process-list coverage.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — validateCurlProbeArgs rejects Authorization, Proxy-Authorization, x-api-key, and x-goog-api-key headers with whitespace before the colon. Unit and fake-curl coverage is broad and useful, including `/proc` and `ps auxww` inspection, but the PR changes credential and process-boundary behavior that originally failed on host-visible container runtime configurations. Several edge-case unit tests are also still missing.
  • PRA-T2 Runtime validation — validateCurlProbeArgs and scrubCredentialEnv treat sessionToken and bearerToken as credential-shaped names. Unit and fake-curl coverage is broad and useful, including `/proc` and `ps auxww` inspection, but the PR changes credential and process-boundary behavior that originally failed on host-visible container runtime configurations. Several edge-case unit tests are also still missing.
  • PRA-T3 Runtime validation — query-param curl auth preserves reserved API-key characters as one exact key value without putting the key in argv. Unit and fake-curl coverage is broad and useful, including `/proc` and `ps auxww` inspection, but the PR changes credential and process-boundary behavior that originally failed on host-visible container runtime configurations. Several edge-case unit tests are also still missing.
  • PRA-T4 Runtime validation — runChatCompletionsStreamingProbe scrubs process.env and opts.env credential variables while using a trusted config file. Unit and fake-curl coverage is broad and useful, including `/proc` and `ps auxww` inspection, but the PR changes credential and process-boundary behavior that originally failed on host-visible container runtime configurations. Several edge-case unit tests are also still missing.
  • PRA-T5 Runtime validation — runStreamingEventProbe scrubs process.env and opts.env credential variables while using a trusted config file. Unit and fake-curl coverage is broad and useful, including `/proc` and `ps auxww` inspection, but the PR changes credential and process-boundary behavior that originally failed on host-visible container runtime configurations. Several edge-case unit tests are also still missing.
  • PRA-T6 Streaming curl wrappers still lack direct env-scrub coverage — Add focused tests for both streaming wrappers that pass a trusted config file, set credential-shaped values in `process.env` and `opts.env`, capture `options.env` in `spawnSyncImpl`, and assert secrets are absent while benign variables remain.
  • PRA-T7 OpenAI-like auth-config setup failure path still lacks direct normal and strict-path coverage — Add direct tests that force `createOpenAiLikeAuthConfig()` or its filesystem boundary to throw, call `probeOpenAiLikeEndpoint()` on both the normal path and with `requireChatCompletionsToolCalling: true`, assert no curl spawn occurs, and assert the returned failure entry uses name `curl auth config` with `httpStatus: 0` and `curlStatus: 0`.
  • PRA-T8 Issue-specific runtime validation remains needed for visible host process boundaries — Add or identify targeted runtime/integration validation for the changed behavior on at least one host/container runtime where the curl process is visible to the host. Keep the validation behavior-specific: onboard with a canary API key, inspect host process arguments while validation curl runs, and assert the canary is absent while `--config` is present.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: OpenAI-like auth-config setup failure handling

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Provider-model and Anthropic setup-failure tests exist, but direct OpenAI-like normal and strict Chat Completions setup-failure tests were not found.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `openAiLikeFailureFromError()` returns a `curl auth config` failure, but `onboard-probes.test.ts` does not directly force this boundary for both normal and `requireChatCompletionsToolCalling` paths.

PRA-2 Resolve/justify — Credential-shaped camelCase names still bypass query/env scrubbing

  • Location: src/lib/security/credential-env.ts:13
  • Category: security
  • Problem: The shared credential-name matcher catches separator-delimited names and several compounds, but common camelCase credential names such as `sessionToken` and `bearerToken` still do not match. The same helper gates both query-parameter rejection in curl argv validation and curl child environment scrubbing.
  • Impact: A future provider URL or environment variable using a missed camelCase credential name can still place a secret into the curl argv URL or into the spawned curl process environment, undermining this PR's primary credential-leak boundary.
  • Recommended action: Normalize names before matching, for example by checking a separator-free lowercase form and a camelCase-split form, or add explicit covered compounds such as `sessionToken` and `bearerToken` while keeping benign exceptions explicit.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/security/credential-env.ts` and evaluate `isCredentialShapedName("sessionToken")` and `isCredentialShapedName("bearerToken")`; both currently fail the regex shape because `token` is not preceded by start, `_`, or `-`.
  • Missing regression test: Add tests that `validateCurlProbeArgs` rejects `?sessionToken=...` and `?bearerToken=...`, and that `scrubCredentialEnv` / `buildScrubbedCurlProbeEnv` strip `sessionToken` and `bearerToken` while preserving benign variables such as `PATH` and `NO_PROXY`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/security/credential-env.ts` and evaluate `isCredentialShapedName("sessionToken")` and `isCredentialShapedName("bearerToken")`; both currently fail the regex shape because `token` is not preceded by start, `_`, or `-`.
  • Evidence: `CREDENTIAL_SHAPED_NAME_PATTERN` includes `authtoken`, `refreshtoken`, and `accesstoken`, but not `sessiontoken` or `bearertoken`; `credential-env.test.ts` covers `accessKey` and `refreshToken` but not these common token names.

PRA-3 Resolve/justify — Inline auth-header validation misses whitespace-before-colon variants

  • Location: src/lib/adapters/http/curl-args.ts:105
  • Category: security
  • Problem: `assertHeaderCarriesNoSecret()` lowercases and `trimStart()`s the full header value, then checks prefixes such as `authorization:`. Header names with optional whitespace before the colon, such as `Authorization : Bearer secret`, do not start with the forbidden prefix.
  • Impact: A caller regression can still pass a credential-bearing `-H` or `--proxy-header` value in argv if the header name is formatted with whitespace before the colon, exposing secrets to host process-list inspection.
  • Recommended action: Parse the header name before the first colon, trim surrounding whitespace, normalize it to lowercase, and reject `authorization`, `proxy-authorization`, `x-api-key`, and `x-goog-api-key` regardless of whitespace around the colon.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `assertHeaderCarriesNoSecret()` and check the path for `validateCurlProbeArgs(["-sS", "-H", "Authorization : Bearer secret", "https://example.test"\]\)\`; the current `startsWith("authorization:")` check does not catch it.
  • Missing regression test: Add negative tests for `Authorization : Bearer ...`, `Proxy-Authorization : Basic ...`, `x-api-key : ...`, and `x-goog-api-key : ...` through both `-H` and `--proxy-header` where applicable.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `assertHeaderCarriesNoSecret()` and check the path for `validateCurlProbeArgs(["-sS", "-H", "Authorization : Bearer secret", "https://example.test"\]\)\`; the current `startsWith("authorization:")` check does not catch it.
  • Evidence: `curl-args.test.ts` covers `Authorization: Bearer`, `x-api-key:`, and `Proxy-Authorization:` without whitespace before the colon, but not the whitespace-before-colon forms.

PRA-4 Resolve/justify — Query-param auth still lacks proof for reserved API-key characters

  • Location: src/lib/adapters/http/auth-config.ts:48
  • Category: correctness
  • Problem: `createQueryParamAuthConfig()` writes `url-query = "key=${value}"` after curl-config quoting, but does not URL-encode query values or prove curl preserves reserved characters as one exact parameter value.
  • Impact: API keys containing spaces, `&`, `=`, `%`, or other reserved characters could be split or transformed by curl query construction. That can break Gemini-style auth and may reintroduce surprising credential handling even though the key is no longer in argv.
  • Recommended action: Either encode the query value before writing the `url-query` directive if that is the intended curl contract, or add a faithful behavioral test with a local capture server or fake curl-config parser proving the provider receives the exact intended `key` value and the key never appears in argv.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `formatCurlConfigEntry()` in `src/lib/adapters/http/auth-config.ts` and the query-param tests in `provider-models.test.ts`; they only cover simple `AIzaFakeKey123` and `secret key`, not reserved separators such as `&` or `=`.
  • Missing regression test: Add a test named like `query-param auth preserves reserved API-key characters without putting them in argv`, using a value such as `secret key&scope=a=b` and asserting the received `key` parameter is exactly that value.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `formatCurlConfigEntry()` in `src/lib/adapters/http/auth-config.ts` and the query-param tests in `provider-models.test.ts`; they only cover simple `AIzaFakeKey123` and `secret key`, not reserved separators such as `&` or `=`.
  • Evidence: `provider-models.test.ts` asserts `url-query = "key=AIzaFakeKey123"`; `onboard-probes.test.ts` asserts a retry does not put `secret key` in argv, but neither proves reserved-character behavior at the curl query boundary.

PRA-5 Resolve/justify — Streaming curl wrappers still lack direct env-scrub coverage

  • Location: src/lib/adapters/http/probe.test.ts:492
  • Category: tests
  • Problem: The production code now routes `runChatCompletionsStreamingProbe()` and `runStreamingEventProbe()` through `resolveCurlProbeSpawnEnv()`, but the added env-scrub tests exercise only `runCurlProbe()`. The streaming sections still focus on SSE parsing, timeouts, and traces.
  • Impact: A future refactor can accidentally drop env scrubbing from one streaming wrapper while the current non-streaming env tests continue to pass, leaving streaming validation probes able to leak credential-shaped environment variables to curl.
  • Recommended action: Add focused tests for both streaming wrappers that pass a trusted config file, set credential-shaped values in `process.env` and `opts.env`, capture `options.env` in `spawnSyncImpl`, and assert secrets are absent while benign variables remain.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search `src/lib/adapters/http/probe.test.ts` for `spawnedEnv`; all current env assertions are in the `runCurlProbe` helper section, while later `runChatCompletionsStreamingProbe` and `runStreamingEventProbe` tests use `mockStreaming()` without inspecting `options.env`.
  • Missing regression test: Add tests named `chat streaming probe scrubs process and opts env while using trusted config` and `responses streaming probe scrubs process and opts env while using trusted config`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search `src/lib/adapters/http/probe.test.ts` for `spawnedEnv`; all current env assertions are in the `runCurlProbe` helper section, while later `runChatCompletionsStreamingProbe` and `runStreamingEventProbe` tests use `mockStreaming()` without inspecting `options.env`.
  • Evidence: Grep found direct `spawnedEnv` assertions around lines 365-485 for `runCurlProbe`, but no equivalent `spawnedEnv` assertions in the streaming wrapper test blocks beginning around lines 492 and 600.

PRA-6 Resolve/justify — OpenAI-like auth-config setup failure path still lacks direct normal and strict-path coverage

  • Location: src/lib/inference/onboard-probes.ts:856
  • Category: tests
  • Problem: `probeOpenAiLikeEndpoint()` catches auth-config setup errors and maps them through `openAiLikeFailureFromError()`, but the changed tests do not directly force that boundary to throw for both the normal OpenAI-like path and the `requireChatCompletionsToolCalling` path.
  • Impact: This is a source-boundary failure path for the credential tmpfile. Without direct tests, a future change can spawn curl after auth-config setup fails, throw uncaught into the onboard wizard, or return a failure shape that callers do not understand.
  • Recommended action: Add direct tests that force `createOpenAiLikeAuthConfig()` or its filesystem boundary to throw, call `probeOpenAiLikeEndpoint()` on both the normal path and with `requireChatCompletionsToolCalling: true`, assert no curl spawn occurs, and assert the returned failure entry uses name `curl auth config` with `httpStatus: 0` and `curlStatus: 0`.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search `src/lib/inference/onboard-probes.test.ts` for `mkdtemp`, `curl auth config`, or `auth-config setup`; provider-model and Anthropic setup-failure tests exist, but equivalent OpenAI-like normal/strict endpoint tests were not found.
  • Missing regression test: Add tests named `OpenAI-like probe reports curl auth config setup failure without spawning curl` and `strict Chat Completions probe reports curl auth config setup failure without spawning curl`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search `src/lib/inference/onboard-probes.test.ts` for `mkdtemp`, `curl auth config`, or `auth-config setup`; provider-model and Anthropic setup-failure tests exist, but equivalent OpenAI-like normal/strict endpoint tests were not found.
  • Evidence: `openAiLikeFailureFromError()` exists in `onboard-probes.ts`, and `probeOpenAiLikeEndpoint()` catches at line 856, but grep of `onboard-probes.test.ts` found strict retry behavior tests rather than auth-config setup-failure tests.

PRA-7 Resolve/justify — Test-only helpers are still included in the production src build

  • Location: src/lib/adapters/http/auth-config-test-helpers.ts:11
  • Category: correctness
  • Problem: `auth-config-test-helpers.ts` lives under `src/lib`, imports `expect` from `vitest`, and is imported only by tests. The new `onboard-probes-curl-harness.ts` is also a fake-curl test harness under `src/lib`. `tsconfig.src.json` includes `src/**/*.ts` and excludes only `src/**/*.test.ts`, so both helpers are part of the production build graph.
  • Impact: The published CLI can ship test-only modules, including a dev-only `vitest` dependency edge and a fake-curl shell-script harness. That increases production surface area and can break packaging, consumers, or static dependency checks despite no runtime code needing these helpers.
  • Recommended action: Move these helpers to a test-only location such as `test/helpers/`, or otherwise rename/exclude them so `tsconfig.src.json` does not emit them. Keep the assertions and fake-curl coverage, but keep the helper modules outside the production source graph.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `tsconfig.src.json` and confirm `include: ["src/**/*.ts"]` with only `src/**/*.test.ts` excluded; then grep for `auth-config-test-helpers` and `onboard-probes-curl-harness` to confirm they are only used by tests.
  • Missing regression test: Add or identify a package/build-shape check that verifies production `dist` does not contain `auth-config-test-helpers` or `onboard-probes-curl-harness`, and that emitted production files do not import `vitest`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `tsconfig.src.json` and confirm `include: ["src/**/*.ts"]` with only `src/**/*.test.ts` excluded; then grep for `auth-config-test-helpers` and `onboard-probes-curl-harness` to confirm they are only used by tests.
  • Evidence: `src/lib/adapters/http/auth-config-test-helpers.ts` imports `{ expect } from "vitest"`; `src/lib/inference/onboard-probes-curl-harness.ts` creates fake curl scripts and mutates test env; both are matched by `tsconfig.src.json`.

PRA-8 Resolve/justify — Security test additions still grow large test hotspots

  • Location: src/lib/inference/onboard-probes.test.ts
  • Category: architecture
  • Problem: The PR adds important security regression coverage, but the changed large test files still grow substantially: `probe.test.ts` by about 126 lines, `onboard-probes.test.ts` by about 60 lines, and `local.test.ts` by about 27 lines. Some extraction happened, but cohesive helpers were placed under `src` rather than a test-only helper area.
  • Impact: These already-large tests become harder to review and maintain, and the current extraction strategy creates production build-surface drift. Future security fixes in the same area will be harder to verify without accidentally weakening the real subprocess and `/proc` assertions.
  • Recommended action: Move cohesive fake-curl/auth-config/process-list helpers and any standalone scenarios into focused behavior-specific test files or `test/helpers`, while preserving the real subprocess, trusted-config, env-scrub, and `/proc` assertions.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Compare the monolith deltas for `src/lib/adapters/http/probe.test.ts`, `src/lib/inference/onboard-probes.test.ts`, and `src/lib/inference/local.test.ts`, then inspect whether the new helper modules live in a test-only path.
  • Missing regression test: Existing behavioral tests should be retained; add no new behavior-only test for this finding, but verify after refactor that the same named tests for process-list secrecy, auth-config cleanup, and env scrubbing still exist.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Compare the monolith deltas for `src/lib/adapters/http/probe.test.ts`, `src/lib/inference/onboard-probes.test.ts`, and `src/lib/inference/local.test.ts`, then inspect whether the new helper modules live in a test-only path.
  • Evidence: Synthetic drift context reports blocker-level growth for `probe.test.ts`, `onboard-probes.test.ts`, and `local.test.ts`; the new fake-curl harness was extracted to `src/lib/inference/onboard-probes-curl-harness.ts` instead of a test-only path.

PRA-9 Resolve/justify — Issue-specific runtime validation remains needed for visible host process boundaries

  • Location: not file-specific
  • Category: tests
  • Problem: The PR adds a strong Linux fake-curl test that records `/proc/$$/cmdline`, `/proc/$$/environ`, and `ps auxww`, but the linked issue is specifically about host-visible process lists on ubuntu24-gpu/k3s and WSL2/Docker Desktop runtime configurations.
  • Impact: A local fake-curl test proves the argv/env construction boundary, but it does not by itself prove the full onboard/runtime path on at least one affected process-visibility runtime where host `ps auxww` originally observed the key.
  • Recommended action: Add or identify targeted runtime/integration validation for the changed behavior on at least one host/container runtime where the curl process is visible to the host. Keep the validation behavior-specific: onboard with a canary API key, inspect host process arguments while validation curl runs, and assert the canary is absent while `--config` is present.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the PR for a behavior-specific runtime validation artifact or test definition that runs the changed onboard probe path on a visible-process host; do not rely on external job pass/fail status as review evidence.
  • Missing regression test: Add a runtime validation named `onboard NVIDIA cloud validation keeps API key out of host ps auxww on a visible-process runtime` or document an equivalent checked-in harness.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the PR for a behavior-specific runtime validation artifact or test definition that runs the changed onboard probe path on a visible-process host; do not rely on external job pass/fail status as review evidence.
  • Evidence: The linked issue names `ps auxww` on ubuntu24-gpu and WSL2; synthetic `testDepth` reports `runtime_validation_recommended` for `auth-config`, `curl-args`, `probe`, `health`, `local`, `model-size`, and onboarding helper surfaces.

PRA-10 Improvement — New probe retry helper keeps a file-level @ts-nocheck

  • Location: src/lib/inference/probe-retry.ts:9
  • Category: architecture
  • Problem: `probe-retry.ts` is a new shared retry module but disables TypeScript checking for the entire file. The surface is small enough to type locally while preserving the CommonJS export shape used by `onboard-probes.ts`.
  • Impact: Type-checking is disabled around retry predicates and callback contracts that influence onboarding validation behavior. Future drift in result shape or trace module calls can go unnoticed.
  • Suggested action: Define minimal local types for the probe result, probe callback object, and trace module shape, then remove the file-level `@ts-nocheck` while preserving `module.exports` if callers still require it.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Open `src/lib/inference/probe-retry.ts` and confirm the leading `// @ts-nocheck` is removed and the exported functions still match existing CommonJS imports from `onboard-probes.ts`.
  • Missing regression test: Existing retry tests in `onboard-probes.test.ts` cover behavior; add or identify a type-safety hotspot check that fails if this new helper reintroduces file-level `@ts-nocheck`.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: `src/lib/inference/probe-retry.ts:9` contains `// @ts-nocheck — onboard-probes.ts is CommonJS-style and require()-based`.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/lib/inference/provider-models.ts (1)

100-122: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Catch temp-config creation failures in the fetch-result path.

createBearerAuthConfig, buildOpenAiLikeAuthConfig, and createXApiKeyAuthConfig create temp files before the try, so filesystem failures bypass the existing ModelCatalogFetchResult error return and throw to callers. Move creation inside the try and cleanup conditionally. Based on learnings, filesystem boundaries are a realistic place for actionable error handling.

Proposed fix pattern
-  const authConfig = createBearerAuthConfig(normalizeCredentialValue(apiKey));
+  let authConfig: CurlAuthConfig | undefined;
   try {
+    authConfig = createBearerAuthConfig(normalizeCredentialValue(apiKey));
     const result = runCurlProbeImpl(
@@
   } finally {
-    authConfig.cleanup();
+    authConfig?.cleanup();
   }

Apply the same pattern in fetchOpenAiLikeModels and fetchAnthropicModels.

Also applies to: 159-173, 183-205

🤖 Prompt for 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.

In `@src/lib/inference/provider-models.ts` around lines 100 - 122, The
fetch-result paths in `fetchOpenAiLikeModels`, `fetchAnthropicModels`, and the
shown `fetch...` helper create temp auth config files before entering the `try`,
so failures in `createBearerAuthConfig`, `buildOpenAiLikeAuthConfig`, or
`createXApiKeyAuthConfig` bypass the `ModelCatalogFetchResult` error handling.
Move each auth-config creation into the `try`, keep the `runCurlProbeImpl` call
wrapped the same way, and make `authConfig.cleanup()` conditional in `finally`
so cleanup only runs after a config was successfully created.

Source: Learnings

🧹 Nitpick comments (1)
src/lib/inference/provider-models.ts (1)

28-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Centralize the OpenAI-like auth-mode builder.

buildOpenAiLikeAuthConfig is duplicated in src/lib/inference/onboard-probes.ts; keep one shared helper so bearer/query-param behavior cannot drift between model catalog and onboarding probes. As per path instructions, src/lib/{actions,domain,adapters,state}/** should flag duplicate sources of truth and keep host/process details in adapters.

🤖 Prompt for 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.

In `@src/lib/inference/provider-models.ts` around lines 28 - 35,
`buildOpenAiLikeAuthConfig` is duplicated and the bearer/query-param logic can
drift between `provider-models.ts` and the onboarding probe flow. Extract this
auth-mode builder into a single shared helper and reuse it from both
`buildOpenAiLikeAuthConfig` call sites, keeping the normalized credential
handling and `options.authMode === "query-param"` branch in one place.

Source: Path instructions

🤖 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 `@src/lib/adapters/http/curl-args.ts`:
- Around line 80-91: The inline credential guard in assertHeaderCarriesNoSecret
is missing the proxy credential case, so add proxy-authorization: to
CURL_FORBIDDEN_AUTH_HEADER_PREFIXES alongside authorization:, x-api-key:, and
x-goog-api-key:. This ensures --proxy-header values are treated the same as
other secret-bearing headers and rejected before they can reach argv.

In `@src/lib/inference/onboard-probes.test.ts`:
- Around line 669-677: The retry assertion in onboard-probes.test.ts is too weak
because it can accept a dangling --config with no value; update the test around
the args.split and configIndex check to first assert that the line after
--config exists before reading configPath, then compare that path across retries
using firstConfigPath so the test fails when the config argument is missing a
value.

In `@src/lib/inference/onboard-probes.ts`:
- Around line 327-378: The auth config is created outside the probe’s error
handling, so failures in buildOpenAiLikeAuthConfig can throw instead of
returning the structured probe failure shape. Move the auth-config setup for the
validation probe into a guarded boundary in onboard-probes and return the same {
ok: false, httpStatus: 0, curlStatus: 0, ... } result used for curl failures
when temp-file or filesystem work fails. Keep the existing try/finally cleanup
behavior, and apply the same pattern to the other probe helpers that create auth
config before their main request logic.

In `@src/lib/inference/provider-models.test.ts`:
- Around line 249-258: The auth-isolation tests for fetchOpenAiLikeModels,
fetchGeminiLikeModels, and fetchAnthropicLikeModels only validate argv/config
contents, so add assertions in each runCurlProbeImpl mock that
opts.trustedConfigFiles includes the same config path passed via --config.
Update the relevant test cases to verify the trusted config option alongside the
existing header/query-param checks so a missing trustedConfigFiles regression is
caught.

---

Outside diff comments:
In `@src/lib/inference/provider-models.ts`:
- Around line 100-122: The fetch-result paths in `fetchOpenAiLikeModels`,
`fetchAnthropicModels`, and the shown `fetch...` helper create temp auth config
files before entering the `try`, so failures in `createBearerAuthConfig`,
`buildOpenAiLikeAuthConfig`, or `createXApiKeyAuthConfig` bypass the
`ModelCatalogFetchResult` error handling. Move each auth-config creation into
the `try`, keep the `runCurlProbeImpl` call wrapped the same way, and make
`authConfig.cleanup()` conditional in `finally` so cleanup only runs after a
config was successfully created.

---

Nitpick comments:
In `@src/lib/inference/provider-models.ts`:
- Around line 28-35: `buildOpenAiLikeAuthConfig` is duplicated and the
bearer/query-param logic can drift between `provider-models.ts` and the
onboarding probe flow. Extract this auth-mode builder into a single shared
helper and reuse it from both `buildOpenAiLikeAuthConfig` call sites, keeping
the normalized credential handling and `options.authMode === "query-param"`
branch in one place.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7653dfea-8f36-44ae-911b-5aae955da820

📥 Commits

Reviewing files that changed from the base of the PR and between c6113be and 5cef290.

📒 Files selected for processing (9)
  • src/lib/adapters/http/auth-config.test.ts
  • src/lib/adapters/http/auth-config.ts
  • src/lib/adapters/http/curl-args.test.ts
  • src/lib/adapters/http/curl-args.ts
  • src/lib/inference/onboard-probes.test.ts
  • src/lib/inference/onboard-probes.ts
  • src/lib/inference/provider-models.test.ts
  • src/lib/inference/provider-models.ts
  • test/helpers/onboard-smoke-verifier-harness.ts

Comment thread src/lib/adapters/http/curl-args.ts Outdated
Comment thread src/lib/inference/onboard-probes.test.ts Outdated
Comment thread src/lib/inference/onboard-probes.ts Outdated
Comment thread src/lib/inference/provider-models.test.ts
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: ubuntu-repo-cloud-openclaw
Optional E2E targets: ubuntu-repo-cloud-langchain-deepagents-code

Dispatch required E2E targets:

  • gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-cloud-openclaw

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

  • ubuntu-repo-cloud-openclaw: The PR changes shared curl probe/auth-config credential handling plus cloud inference health/onboard probe/provider-model paths used during cloud OpenClaw onboarding and post-onboard inference/credential validation. This live-supported target is the primary Ubuntu Docker cloud OpenClaw path and exercises those changed surfaces end to end.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-cloud-openclaw

Optional E2E targets

  • ubuntu-repo-cloud-langchain-deepagents-code: Adjacent live-supported cloud onboarding path using the same shared inference/curl probe machinery, but with LangChain Deep Agents Code-specific onboarding and terminal-agent coverage. Useful if reviewers want additional confidence beyond the primary OpenClaw cloud target.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-cloud-langchain-deepagents-code

Relevant changed files

  • src/lib/adapters/http/auth-config.ts
  • src/lib/adapters/http/curl-args.ts
  • src/lib/adapters/http/probe.ts
  • src/lib/inference/health.ts
  • src/lib/inference/local.ts
  • src/lib/inference/ollama/model-size.ts
  • src/lib/inference/onboard-probes-curl-harness.ts
  • src/lib/inference/onboard-probes.ts
  • src/lib/inference/probe-anthropic.ts
  • src/lib/inference/probe-retry.ts
  • src/lib/inference/provider-models.ts
  • src/lib/security/credential-env.ts
  • test/helpers/onboard-smoke-verifier-harness.ts
  • test/onboard-selection.test.ts

Comment thread src/lib/inference/provider-models.test.ts Fixed
Comment thread src/lib/inference/provider-models.test.ts Fixed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/lib/adapters/http/auth-config.test.ts`:
- Around line 77-95: The auth routing tests for createOpenAiLikeAuthConfig only
assert the expected transport is present, so they do not prove exclusive mode
selection. Update both tests in auth-config.test.ts to add negative assertions
for the opposite auth transport: the default Bearer case should verify no
url-query entry is written, and the query-param case should verify no
Authorization header is written. This will ensure the helper cannot emit both
paths at once and that the new routing behavior is fully covered.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a280107b-0d71-410f-ad48-21414444d3d8

📥 Commits

Reviewing files that changed from the base of the PR and between 5cef290 and 53fffa8.

📒 Files selected for processing (11)
  • src/lib/adapters/http/auth-config-test-helpers.ts
  • src/lib/adapters/http/auth-config.test.ts
  • src/lib/adapters/http/auth-config.ts
  • src/lib/adapters/http/curl-args.ts
  • src/lib/inference/health.ts
  • src/lib/inference/onboard-probes.test.ts
  • src/lib/inference/onboard-probes.ts
  • src/lib/inference/probe-retry.ts
  • src/lib/inference/provider-models.test.ts
  • src/lib/inference/provider-models.ts
  • src/lib/trace.test.ts
✅ Files skipped from review due to trivial changes (1)
  • src/lib/inference/health.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/lib/adapters/http/curl-args.ts
  • src/lib/inference/provider-models.ts
  • src/lib/inference/onboard-probes.ts

Comment thread src/lib/adapters/http/auth-config.test.ts Outdated
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…llowRedirects opt-in

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…sed heap

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…n fake curl

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…rowing the file

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…th-config failure

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…env helpers

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…c probe; test auth-config helpers

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…nt env

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Target Results — ✅ All requested jobs passed

Run: 28543775914
Workflow ref: fix/curl-probe-no-bearer-in-argv
Requested targets: (default — all supported)
Requested jobs: cloud-onboard,inference-routing,kimi-inference-compat,ollama-auth-proxy,credential-sanitization
Summary: 5 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
cloud-onboard ✅ success
credential-sanitization ✅ success
inference-routing ✅ success
kimi-inference-compat ✅ success
ollama-auth-proxy ✅ success

prekshivyas and others added 2 commits July 1, 2026 14:08
…es.test.ts duplication

Addresses PR #5975 advisor item PRA-9 (test monolith + fake-curl harness
duplicated across ~7 tests). Move the repeated boilerplate — the -o/-w
arg-parsing curl-script header and the tmpdir/fakeBin/counter + PATH/
NEMOCLAW_TEST_NO_SLEEP/console.log save-set-restore plumbing — into a
co-located non-test helper (onboard-probes-curl-harness.ts), and route the
duplicated sites through makeFakeCurlScript()/withFakeCurlProbe().

Behavior is identical: each test keeps its exact bash body (via HARNESS_COUNTER/
HARNESS_TMPDIR placeholders substituted to the real absolute paths before the
script is written, so the emitted bash is byte-for-byte unchanged) and every
assertion verbatim. onboard-probes.test.ts drops 1056 -> 927 lines; suite is
unchanged (24 passed, 1 darwin-skipped). Keeping the branching in the .ts helper
also holds the test-file if-count flat.

SKIP=test-cli: the full cli+integration hook trips on pre-existing macOS bash
3.2 shell-harness failures unrelated to this change; CI runs bash 5.x green.
The onboard-probes suite, CLI typecheck, budget, and checks all pass.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
@prekshivyas

Copy link
Copy Markdown
Collaborator

All 5 CodeRabbit findings are stale:

  • curl-args.ts:91 (proxy-authorization header) — proxy-authorization: is already in CURL_FORBIDDEN_AUTH_HEADER_PREFIXES.
  • onboard-probes.test.ts:677 (--config path assertion) — comment at L631 explicitly cites this finding (PRA-9 / CodeRabbit); addressed.
  • onboard-probes.ts:378 (error not returned) — catch (error) { return probeFailureFromError(error); } already returns structured failure.
  • provider-models.test.ts:245 (auth isolation) — expectTrustedConfig present at L29, L220, L246, L275, L299 covering all auth-isolation tests.
  • auth-config.test.ts:95 (exclusive mode) — test at L88 asserts both url-query present AND Authorization absent.

No action needed on any of these.

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Target Results — ✅ All requested jobs passed

Run: 28550394810
Workflow ref: fix/curl-probe-no-bearer-in-argv
Requested targets: (default — all supported)
Requested jobs: cloud-onboard,inference-routing,kimi-inference-compat,ollama-auth-proxy,credential-sanitization
Summary: 5 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
cloud-onboard ✅ success
credential-sanitization ✅ success
inference-routing ✅ success
kimi-inference-compat ✅ success
ollama-auth-proxy ✅ success

@apurvvkumaria
apurvvkumaria merged commit f1e7d87 into main Jul 1, 2026
118 checks passed
@apurvvkumaria
apurvvkumaria deleted the fix/curl-probe-no-bearer-in-argv branch July 1, 2026 22:42
ericksoa pushed a commit that referenced this pull request Jul 2, 2026
## Summary
- Add the `v0.0.72` release-note section with links to the deeper docs
pages for installer recovery, command diagnostics, inference, policy,
and sandbox repair changes.
- Document the custom preset `allowed_ips` guard for user-authored
policy files.

## Related Issue
None.

## Source summary
- #6132 -> `docs/about/release-notes.mdx`: Summarizes installer and
upgrade recovery before generic onboarding, with links to quickstart and
lifecycle docs.
- #6087 -> `docs/network-policy/customize-network-policy.mdx`: Documents
that user-authored custom presets reject `allowed_ips` for ordinary
endpoints; also summarized in release notes.
- #5975 -> `docs/about/release-notes.mdx`: Summarizes safer curl-based
inference probes that keep API keys out of process arguments.
- #6044 -> `docs/about/release-notes.mdx`: Summarizes compact `channels
status` configuration reporting.
- #6096 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw EC2
metadata discovery disablement and links to security guidance.
- #5980 and #5991 -> `docs/about/release-notes.mdx`: Summarizes `exec`
multiline argument rejection and recovery guidance.
- #6023 -> `docs/about/release-notes.mdx`: Summarizes
registered-provider diagnostics for `inference set` failures.
- #6074 -> `docs/about/release-notes.mdx`: Summarizes the refreshed
NVIDIA Endpoints featured-model selection behavior.
- #5969 -> `docs/about/release-notes.mdx`: Summarizes `credentials add`
provider credential registration.
- #6060 -> `docs/about/release-notes.mdx`: Summarizes mutable OpenClaw
config permission restoration after `exec`.
- #6134 -> `docs/about/release-notes.mdx`: Summarizes restored Tavily
access for managed Python workflows.
- #6089 -> `docs/about/release-notes.mdx`: Summarizes Hermes runtime
version-scheme comparison during upgrade checks.
- #6131 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw gateway
watchdog recovery behavior.
- #5976 and #5990 -> `docs/about/release-notes.mdx`: Summarizes prompt
stdin EOF cancellation behavior during onboarding.
- #5540 -> `docs/about/release-notes.mdx`: Summarizes clarified
host-level and per-sandbox status command scope.
- #5978 and #6018 -> `docs/about/release-notes.mdx`: Summarizes
policy-denial log breadcrumbs in connect shells.

## Testing
- `npm run docs:sync-agent-variants`
- `npm run docs`
- Commit hooks passed during `git commit`, including commitlint and
gitleaks.
- Pre-push hook passed during `git push`, including TypeScript CLI and
package/tag version sync.

## Checklist
- [x] Documentation updated.
- [x] `npm run docs` completed with 0 errors and 1 existing Fern
warning.
- [x] No source code or generated build artifacts committed.

Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Added release notes for v0.0.72 covering improved installer recovery,
clearer CLI diagnostics, safer inference setup and provider switching,
better credential handling, stronger policy boundaries, and more robust
runtime repair behavior.
* Updated network policy guidance to clarify when `allowed_ips` can be
used, including a specific exception for the sandbox-to-host bridge
endpoint.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
## Summary
Probe-time curl spawns embedded the literal API key as an argv element
(`-H "Authorization: Bearer ..."`, `-H "x-api-key: ..."`, `?key=...` in
URL), exposing it to host `ps auxww` on container runtimes that share
`/proc` with the host (k3s/containerd, Docker Desktop on WSL2). Route
every credential through a 0600 `curl --config` tmpfile so the secret
never reaches argv.

## Related Issue
Fixes NVIDIA#5966

## Changes
- New `src/lib/adapters/http/auth-config.ts` helper that writes a 0600
curl config tmpfile carrying `header = "..."` or `url-query = "..."`
entries; returns the `--config <path>` argv pair plus a `cleanup()` for
`finally`.
- `provider-models.ts`: `fetchNvidiaEndpointModels`,
`fetchOpenAiLikeModels`, `fetchAnthropicModels` route Bearer / x-api-key
/ query-param credentials via the new helper.
- `onboard-probes.ts`: `probeResponsesToolCalling`,
`probeChatCompletionsToolCalling`, `probeOpenAiLikeEndpoint` (Responses
+ Chat Completions + streaming + doubled-timeout retry),
`probeAnthropicEndpoint` — all argv sites swapped to `--config`;
bearer/x-api-key/`?key=` literals removed from URL and argv. Single
`authConfig` per probe scope, cleaned up in `finally`.
- Defence-in-depth in `curl-args.ts`: `validateCurlProbeArgs` refuses
inline `Authorization:` / `x-api-key:` / `x-goog-api-key:` `-H` values
and refuses URLs with `?key=` / `?api_key=` / `?apikey=` / `?token=` /
`?access_token=` query parameters. Trusted `--config` route remains the
only legal credential channel.
- Tests updated to invert "argv contains Bearer literal" → "argv
contains `--config <path>` and the file is mode 0600 carrying the
expected header/url-query entry; literal API key never appears in argv".
New behavioural coverage for Anthropic. Existing onboard-smoke harness
now reads the config file mid-probe to keep its auth assertion.

## Type of Change

- [x] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [ ] Docs updated for user-facing behavior changes
- [x] Docs not applicable — justification: internal probe transport; no
user-visible behaviour change
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [ ] Doc pages follow the style guide (doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Added curl probe auth via temporary on-disk config for Bearer, API
key, and OpenAI-like “bearer vs query-param” modes.
* **Bug Fixes**
* Reduced credential exposure by rejecting inline auth headers and
secret-bearing query parameters; curl requests now use `--config` with
safe cleanup.
* Improved onboarding/provider probing and standardized retry behavior
for timeout and retriable HTTP failures.
* **Tests**
* Expanded coverage for auth config generation/escaping, curl-arg
validation, onboarding flows, and tracing redaction.
* **Chores**
* Increased Node heap limit for integration subprocesses during Vitest.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
## Summary
- Add the `v0.0.72` release-note section with links to the deeper docs
pages for installer recovery, command diagnostics, inference, policy,
and sandbox repair changes.
- Document the custom preset `allowed_ips` guard for user-authored
policy files.

## Related Issue
None.

## Source summary
- NVIDIA#6132 -> `docs/about/release-notes.mdx`: Summarizes installer and
upgrade recovery before generic onboarding, with links to quickstart and
lifecycle docs.
- NVIDIA#6087 -> `docs/network-policy/customize-network-policy.mdx`: Documents
that user-authored custom presets reject `allowed_ips` for ordinary
endpoints; also summarized in release notes.
- NVIDIA#5975 -> `docs/about/release-notes.mdx`: Summarizes safer curl-based
inference probes that keep API keys out of process arguments.
- NVIDIA#6044 -> `docs/about/release-notes.mdx`: Summarizes compact `channels
status` configuration reporting.
- NVIDIA#6096 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw EC2
metadata discovery disablement and links to security guidance.
- NVIDIA#5980 and NVIDIA#5991 -> `docs/about/release-notes.mdx`: Summarizes `exec`
multiline argument rejection and recovery guidance.
- NVIDIA#6023 -> `docs/about/release-notes.mdx`: Summarizes
registered-provider diagnostics for `inference set` failures.
- NVIDIA#6074 -> `docs/about/release-notes.mdx`: Summarizes the refreshed
NVIDIA Endpoints featured-model selection behavior.
- NVIDIA#5969 -> `docs/about/release-notes.mdx`: Summarizes `credentials add`
provider credential registration.
- NVIDIA#6060 -> `docs/about/release-notes.mdx`: Summarizes mutable OpenClaw
config permission restoration after `exec`.
- NVIDIA#6134 -> `docs/about/release-notes.mdx`: Summarizes restored Tavily
access for managed Python workflows.
- NVIDIA#6089 -> `docs/about/release-notes.mdx`: Summarizes Hermes runtime
version-scheme comparison during upgrade checks.
- NVIDIA#6131 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw gateway
watchdog recovery behavior.
- NVIDIA#5976 and NVIDIA#5990 -> `docs/about/release-notes.mdx`: Summarizes prompt
stdin EOF cancellation behavior during onboarding.
- NVIDIA#5540 -> `docs/about/release-notes.mdx`: Summarizes clarified
host-level and per-sandbox status command scope.
- NVIDIA#5978 and NVIDIA#6018 -> `docs/about/release-notes.mdx`: Summarizes
policy-denial log breadcrumbs in connect shells.

## Testing
- `npm run docs:sync-agent-variants`
- `npm run docs`
- Commit hooks passed during `git commit`, including commitlint and
gitleaks.
- Pre-push hook passed during `git push`, including TypeScript CLI and
package/tag version sync.

## Checklist
- [x] Documentation updated.
- [x] `npm run docs` completed with 0 errors and 1 existing Fern
warning.
- [x] No source code or generated build artifacts committed.

Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Added release notes for v0.0.72 covering improved installer recovery,
clearer CLI diagnostics, safer inference setup and provider switching,
better credential handling, stronger policy boundaries, and more robust
runtime repair behavior.
* Updated network policy guidance to clarify when `allowed_ips` can be
used, including a specific exception for the sandbox-to-host bridge
endpoint.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression NV QA Bugs found by the NVIDIA QA Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Ubuntu 24.04][Security] NVIDIA_API_KEY visible in host process list via ps auxww on ubuntu24-gpu and WSL2 runners

5 participants