fix(cli): resolve web command SecretRefs#82819
Conversation
|
Codex review: needs changes before merge. Summary Reproducibility: yes. for review purposes: configure a non-bundled web provider with a plugin-scoped webSearch/webFetch SecretRef and select it with infer web ... --provider. Source inspection shows the PR's override matchers use bundled-only owner lookup, so that selected path is treated as inactive instead of returned. Real behavior proof Next step before merge Security Review findings
Review detailsBest possible solution: Keep the provider-aware command SecretRef design, but align every selected-provider owner check with the registry-aware, origin-agnostic lookup used by command target selection before landing. Do we have a high-confidence way to reproduce the issue? Yes for review purposes: configure a non-bundled web provider with a plugin-scoped webSearch/webFetch SecretRef and select it with infer web ... --provider. Source inspection shows the PR's override matchers use bundled-only owner lookup, so that selected path is treated as inactive instead of returned. Is this the best way to solve the issue? No: the overall provider-aware approach is the right boundary, and the latest head fixed the secret-prefix repro leak, but the override owner checks still need to match current main's origin-agnostic registry contract. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 2a7f9f354644. |
f03421f to
8cbc656
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f03421f1fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| targetIds: getCapabilityWebCommandSecretTargetIds(), | ||
| ...(provider | ||
| ? { | ||
| providerOverrides: | ||
| params.webKind === "search" ? { webSearch: provider } : { webFetch: provider }, |
There was a problem hiding this comment.
Scope web secret targets to the active web command
resolveCapabilityCommandConfig always resolves the full getCapabilityWebCommandSecretTargetIds() set (search + fetch + plugin web refs), but only sends providerOverrides when --provider is explicitly passed. For default infer web search/infer web fetch runs, unrelated web surfaces remain in-scope, so an unresolved SecretRef on the other surface (for example, a broken tools.web.fetch... ref while running search) can still trigger enforced unresolved-secret failures and block the command even though that provider path is not used.
Useful? React with 👍 / 👎.
8cbc656 to
d0af5da
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0af5da09b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| resolveManifestContractOwnerPluginId({ | ||
| contract: "webSearchProviders", | ||
| value: webSearch, | ||
| origin: "bundled", | ||
| config: params.config, |
There was a problem hiding this comment.
Resolve override owners across all plugin origins
The provider-override matching path is hard-coded to origin: "bundled", so infer web search/fetch --provider ... cannot activate SecretRefs for installed external providers even when getWeb*CommandSecretTargets selected those plugin paths. In that case isProviderOverridePath returns false for the selected plugin credential path, it gets treated as inactive, and command-secret resolution skips the credential for the provider the user explicitly chose. This is a regression introduced by the override flow and affects any non-bundled web provider with SecretRefs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0343bb3e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| commandSecretGatewayDeps.resolveManifestContractOwnerPluginId({ | ||
| contract: "webSearchProviders", | ||
| value: webSearch, | ||
| origin: "bundled", | ||
| config: params.config, |
There was a problem hiding this comment.
Resolve override owners without bundled-only filtering
When resolveCommandSecretRefsViaGateway falls back to resolveCommandSecretRefsLocally (for example, gateway unavailable/unsupported), isProviderOverridePath resolves override owners with origin: "bundled", so external web providers selected via --provider are treated as inactive and their SecretRefs are skipped. This means local CLI runs can silently miss the selected provider credential (or degrade to unresolved behavior) even though allowedPaths selected that provider; the fresh evidence here is that this bundled-only filter is present in the new local-fallback override path, not just the server-side resolver already discussed.
Useful? React with 👍 / 👎.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: 吴杨帆 <39647285+leno23@users.noreply.github.com>
bd167de to
3fc6660
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44fb025ade
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const directProvider = searchProviderFromDirectWebPath(params.path); | ||
| if (directProvider) { | ||
| return directProvider === webSearch; |
There was a problem hiding this comment.
Canonicalize provider overrides before path matching
getWebSearchCommandSecretTargets/getWebFetchCommandSecretTargets normalize provider ids to lowercase, but isProviderOverridePath compares override values with normalizeOptionalString output, so mixed-case CLI input like --provider Gemini can select lowercase credential paths while this matcher treats them as non-selected and inactive. In that case the selected provider SecretRef can be reported unresolved (or skipped) even though the credential is configured, because direct-path checks and owner resolution run against the non-canonical override string.
Useful? React with 👍 / 👎.
|
Verification for replacement fix for #82699 / #82621:
Known gaps: live third-party provider calls proved SecretRef resolution and plugin install paths, but available Firecrawl/Gemini/xAI keys did not all authenticate successfully at the provider boundary. Raw AWS Crabbox image lacked Node, so E2E used Actions-hydrated Crabbox instead. |
Summary
Verification
Real behavior proof
Behavior addressed: infer web search and infer web fetch now resolve only the selected web provider SecretRefs, including --provider overrides, and leave unrelated plugin secrets inactive.
Real environment tested: local source checkout plus AWS Crabbox cbx_aebcfe04e4bc with live provider keys injected through an allowlisted env profile.
Exact steps or command run after this patch: node scripts/run-vitest.mjs src/secrets/runtime-command-secrets.test.ts src/cli/command-secret-gateway.test.ts src/cli/command-secret-targets.test.ts src/gateway/server-methods/secrets.test.ts src/cli/capability-cli.test.ts src/cli/command-config-resolution.test.ts; TAVILY_API_KEY=resolved-live-proof pnpm exec tsx scripts/repro/cli-web-search-secret-refs-live-proof.mjs; pnpm tsgo:core; git diff --check; pnpm crabbox:run -- --provider aws --id cbx_aebcfe04e4bc --no-sync --env-from-profile /tmp/openclaw-82699-live-web.env --allow-env FIRECRAWL_API_KEY,GEMINI_API_KEY,XAI_API_KEY --script /tmp/openclaw-82699-crabbox-live.sh.
Evidence after fix: local focused suite passed 7 files and 137 tests; live repro converted a Tavily env SecretRef to a string; Crabbox installed and inspected the Firecrawl plugin, DuckDuckGo config and CLI override searches returned results, and Grok, Gemini, and Firecrawl config and CLI override cases reached the expected live provider auth responses with redacted key presence.
Observed result after fix: selected web provider credentials are resolved for command execution, unrelated voice-call/Twilio SecretRefs remain inactive, and legacy Firecrawl fetch refs remain covered.
What was not tested: valid paid-provider success for Grok, Gemini, or Firecrawl because the available live keys currently return provider auth errors; those errors still prove the selected SecretRefs were resolved and used.
Fixes #82621.
Supersedes #82699.