fix(cron): add Authorization header to provider preflight probe#83648
fix(cron): add Authorization header to provider preflight probe#83648samson1357924 wants to merge 9 commits into
Conversation
openclaw-barnacle
Bot
added
size: XS
triage: needs-real-behavior-proof
|
Codex review: needs maintainer review before merge. Reviewed July 3, 2026, 3:05 PM ET / 19:05 UTC. Summary PR surface: Source +80, Tests +1505. Total +1585 across 3 files. Reproducibility: yes. at source level: current main and v2026.6.11 send the local cron preflight GET without configured provider/request headers. The inspected LiteLLM screenshot shows the bearer-header path changing from unauthenticated 401 to authenticated 200, but I did not run a live LiteLLM setup in this read-only review. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the shared-resolver approach once maintainers explicitly accept credentialed local/private preflight scope and normal merge gates stay green. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main and v2026.6.11 send the local cron preflight GET without configured provider/request headers. The inspected LiteLLM screenshot shows the bearer-header path changing from unauthenticated 401 to authenticated 200, but I did not run a live LiteLLM setup in this read-only review. Is this the best way to solve the issue? Yes, with maintainer acceptance of the credential scope. Delegating to the shared provider request/header resolver is the narrowest maintainable fix for parity with normal model requests, and the latest head covers provider-default fallback and request/header preservation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 6011c9e16d21. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +80, Tests +1505. Total +1585 across 3 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
proof: sufficient
openclaw-barnacle
Bot
removed
the
proof: sufficient
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
the
proof: sufficient
openclaw-barnacle
Bot
removed
the
proof: sufficient
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
d5f9a78 to
bbacf9f
Compare
openclaw-barnacle
Bot
added
proof: supplied
clawsweeper
Bot
added
the
proof: sufficient
openclaw-barnacle
Bot
removed
the
proof: sufficient
clawsweeper
Bot
added
the
proof: sufficient
|
ClawSweeper PR egg 🔥 Warming up: real-behavior proof passed; findings, security review, or rank-up moves are still in progress. Hatch commandComment Hatchability rules:
What is this egg doing here?
|
The cron job preflight check sends GET {baseUrl}/models to verify local provider connectivity, but was sending the request without an Authorization header, causing spurious 401/ECONNRESET errors.
Changes:
- Add resolveProbeApiKey() with three-source fallback chain (config.apiKey -> auth-profiles.json -> env var)
- Modify probeLocalProviderEndpoint() to accept optional apiKey and send Bearer header
- Wire apiKey resolution into preflightCronModelProvider()
- Use static fs/path imports instead of dynamic import()
- Add console.warn for auth-profiles.json read errors (no silent catch)
Tests added:
- sends Authorization Bearer header when provider config has apiKey
- does not send Authorization header when no apiKey is available
Closes openclaw#83648
The cron job preflight check sends GET {baseUrl}/models to verify local provider connectivity, but was sending the request without an Authorization header, causing spurious 401/ECONNRESET errors.
Changes:
- Add resolveProbeApiKey() with three-source fallback chain (config.apiKey -> auth-profiles.json -> env var)
- Modify probeLocalProviderEndpoint() to accept optional apiKey and send Bearer header
- Wire apiKey resolution into preflightCronModelProvider()
- Use static fs/path imports instead of dynamic import()
- Add console.warn for auth-profiles.json read errors (no silent catch)
Tests added:
- sends Authorization Bearer header when provider config has apiKey
- does not send Authorization header when no apiKey is available
Closes openclaw#83648
ee38079 to
e5180bd
Compare
- Integrated resolveApiKeyForProvider from the shared SDK auth runtime for full credential chain resolution (config → auth profiles → env vars → plugin hooks → SecretRef) - Added agentDir/workspaceDir params for cron agent context scoping - Added isNonSecretApiKeyMarker guard to prevent sentinel markers (custom-local, ollama-local, env-var names, etc.) from being sent as Bearer tokens - Added request.auth.token override tier (authorization-bearer mode) - Added OAuth exclusion and auth leak redaction in error messages - Guarded empty-string request.auth.token to prevent bare Bearer header - Removed evidence screenshot from source tree (proof in PR comment) - Expanded tests from 4 to 13 covering all auth paths + marker filtering Closes openclaw#83648
|
@clawsweeper re-review Changes since last review:
All 13 tests pass. Code reviewer approved. Test engineer verified coverage. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
removed
the
proof: sufficient
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Changes since last review:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
The cron job preflight check sends GET {baseUrl}/models to verify local provider connectivity, but was sending the request without an Authorization header, causing spurious 401/ECONNRESET errors.
Changes:
- Add resolveProbeApiKey() with three-source fallback chain (config.apiKey -> auth-profiles.json -> env var)
- Modify probeLocalProviderEndpoint() to accept optional apiKey and send Bearer header
- Wire apiKey resolution into preflightCronModelProvider()
- Use static fs/path imports instead of dynamic import()
- Add console.warn for auth-profiles.json read errors (no silent catch)
Tests added:
- sends Authorization Bearer header when provider config has apiKey
- does not send Authorization header when no apiKey is available
Closes openclaw#83648
- Error redaction: use formatErrorMessage() to mask auth secrets in logs - Type contract: add agentDir/workspaceDir to preflightCronModelProvider params - Wire agentDir/workspaceDir from run.ts to preflight call - Support request.auth.mode: 'header' for custom header auth - Log debug-level message on resolveApiKeyForProvider failure
P1 fix: when request.auth.mode is 'header' with a valid custom auth header, skip fall-through to resolveApiKeyForProvider() which would inject an unwanted Authorization: Bearer header alongside the custom header. This matches the normal provider request path (provider-request-config.ts) contract where header mode suppresses Bearer injection. Test coverage: - verify resolveApiKeyForProvider is NOT called in header mode - verify ONLY custom header sent (no Authorization Bearer) - verify custom Authorization header with non-Bearer prefix works
Refactor model-preflight.runtime.ts to delegate auth header construction to the shared resolveProviderRequestHeaders/sanitizeConfiguredProviderRequest from provider-request-config.ts, eliminating the hand-rolled auth logic. ClawSweeper P1: custom-header auth path now reuses shared normalization. ClawSweeper P2: cross-path auth header parity tests added. Changes: - Replace manual Authorization/header construction with resolveProviderRequestHeaders delegation - Redact errors at cache-write time via formatErrorMessage - Add debug log for header mode misconfiguration - Add 4 auth parity tests (P0) + 4 edge case tests (P1) - Remove unused apiKey/extraHeaders params from probeLocalProviderEndpoint
The preflight now calls resolveProviderRequestHeaders unconditionally, not only when requestOverrides?.auth exists. This ensures request.headers (configured via request.headers in provider config) are always passed through to the shared auth header resolver, matching the normal model request path. Also fixes eslint(curly) in test file (single-line if must use braces).
|
@clawsweeper re-review Changes since last review:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
- Replace headerModeConfigured (raw config check) with !requestOverrides?.auth (sanitized state) so misconfigured mode:header no longer blocks fallback - Remove stale debug-log block that re-read raw config - Update test expectations: empty headerName/value now correctly falls through to resolveApiKeyForProvider instead of silently dropping auth - Add fallback+providerHeaders integration test - Update comment to accurately describe fallback scope
|
@clawsweeper re-review Changes since last review:
All 39 preflight tests + 30 regression tests pass (69/69). |
|
🦞🧹 I asked ClawSweeper to review this item again. |
- Spread sanitized requestOverrides into fallback resolveProviderRequestHeaders call so request.headers survive when env/profile credentials are used - Update comment to reflect fixed behavior - Add focused test: request.headers + fallback bearer auth coexisting - Fix sanitizeConfiguredProviderRequest mock to handle request.headers - Fix resolveProviderRequestHeaders mock to merge request.headers ClawSweeper P1: src/cron/isolated-agent/model-preflight.runtime.ts:276
|
@clawsweeper re-review Changes since last review :
All 40 preflight tests + 30 regression tests pass (70/70). |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Fixed ClawSweeper P1 finding: "Treat provider-default auth as fallback auth" Problem: When request.auth.mode is provider-default, the sanitizer returns an auth object, which makes requestOverrides?.auth truthy and blocks the resolveApiKeyForProvider fallback. The shared resolver does not inject headers for provider-default, leaving preflight unauthenticated. Fix (model-preflight.runtime.ts):
Tests (2 new, 42 total preflight):
Mock fix: Added provider-default handling to sanitizeConfiguredProviderRequestMock (was missing, returned undefined instead of matching real sanitizer output) All 72/72 tests pass (42 preflight + 30 regression). |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
This pull request has been automatically marked as stale due to inactivity. |
2 participants
Summary
Replaced the hand-rolled Authorization header logic in the cron preflight probe with the shared
resolveProviderRequestHeaders()fromprovider-request-config.ts, and extended it to passproviderConfig.headersthrough the same pipeline. This ensures the preflight and normal model request paths build identical auth headers for the same config, including provider-level static headers and request-level custom headers.Changes
P1: Reuse shared auth normalization (ClawSweeper finding)
if (params.apiKey) { headers["Authorization"] = ... }/ custom-header construction with delegation toresolveProviderRequestHeaders()viasanitizeConfiguredProviderRequest()request.authoverride →resolveApiKeyForProviderfallback → no authrequest.headersare always passed through (even withoutrequest.auth), fixing a pre-existing gap where provider-level and request-level headers were silently droppedP1: Provider-level header parity (ClawSweeper finding)
sanitizeModelHeaders(providerConfig.headers)and passes the result asdefaultHeaderstoresolveProviderRequestHeaders, matching the normal model request path inmodel.ts:675models.providers.*.headers) now flow into preflight probes — previously onlyrequest.authandrequest.headerswere forwardedresolveProviderRequestHeaderscalls (primary andresolveApiKeyForProviderfallback) includedefaultHeadersfor defense-in-depthP1: Preserve request.headers in fallback auth call (ClawSweeper finding)
resolveProviderRequestHeaderscall fromrequest: { auth: { ... } }torequest: { ...requestOverrides, auth: { ... } }so sanitizedrequest.headerssurvive when env/profile credentials are usedmodels.providers.*.request.headersrequest.headersandrequest.authcoexistP1: Treat provider-default auth mode as fallback-triggering (ClawSweeper finding)
!requestOverrides?.authto also skipprovider-defaultmode — whenrequest.auth.mode: "provider-default", the sanitizer returns an auth object that blocks the fallback, but the shared resolver does not inject headers for provider-default, leaving preflight unauthenticatedhasExplicitAuth = requestOverrides?.auth && requestOverrides.auth.mode !== "provider-default"— provider-default now falls through to credential lookupprovider-defaulthandling tosanitizeConfiguredProviderRequestMockto match real sanitizer outputP1: Sanitized auth state for fallback gate
headerModeConfiguredfromproviderConfig?.request?.auth?.mode) with sanitized check (!requestOverrides?.auth)mode:header(empty headerName/value) no longer incorrectly blocks theresolveApiKeyForProviderfallbackheaderstruthy and erroneously skip fallbackP0: Error redaction at cache-write time
formatErrorMessage()to errors before storing inpreflightCache, preventing unredacted error objects from remaining in heap between cache write and readP0: Header prefix trimming consistency
sanitizeConfiguredProviderRequest, matching the behavior ofresolveAuthOverridein the normal request pathCross-path auth + provider header tests
Files Changed
src/cron/isolated-agent/model-preflight.runtime.tssrc/cron/isolated-agent/model-preflight.runtime.test.tssrc/cron/isolated-agent/run.tsReal behavior proof
Behavior or issue addressed: Cron job preflight was building custom auth headers (
request.auth.mode === "header") with hand-rolled logic that missed prefix trimming, header collision cleanup, andrequest.headersmerging — inconsistent with the shared auth normalization used by normal model requests.The fix replaces the entire preflight auth resolution with delegation to
resolveProviderRequestHeaders(), the same shared function used by the normal model request path. This ensures:request.headersare always forwarded (even withoutrequest.auth)models.providers.*.headers) now pass through viadefaultHeaders, matching the normal model pathrequest.headersare preserved in the fallback credential path, so proxy/tenant headers are not lostReal environment tested:
Exact steps or command run after this patch (original base auth case):
node -e "const m = require('./dist/index.cjs'); m.probeCronModelProvider({ provider: 'litellm', model: 'gpt-4o' }).then(r => console.log(JSON.stringify(r)))"Evidence after fix: LiteLLM proxy logs before (top) and after (bottom) the patch show GET /models now gets 200 OK instead of 401:
Before — no auth header, cron preflight triggers 401:
After — auth header sent, preflight succeeds:
The comprehensive auth normalization refactoring (prefix trim, header merging, parity with normal path, provider-header support) is covered by the test suite below.
Additional verification (unit test suite):
Coverage includes:
Observed result after fix:
providerConfig.headers) pass throughsanitizeModelHeaderswith secret-ref marker filtering, matching normal model pathWhat was not tested:
fetchWithSsrFGuard)prepareProviderRuntimeAuthplugin auth exchange path (documented gap — preflight intentionally skips this for local provider probes)SecretRefobjects — these are non-string config shapes thatsanitizeModelHeaderssilently skips, and preflight runs only after config validation