Skip to content

fix(cron): warn when web_search allowlist has no provider#97673

Closed
ly-wang19 wants to merge 2 commits into
openclaw:mainfrom
ly-wang19:codex/fix-cron-web-search-toolsallow-diagnostic
Closed

fix(cron): warn when web_search allowlist has no provider#97673
ly-wang19 wants to merge 2 commits into
openclaw:mainfrom
ly-wang19:codex/fix-cron-web-search-toolsallow-diagnostic

Conversation

@ly-wang19

@ly-wang19 ly-wang19 commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #97654

AI-assisted change authored in Codex.

What Problem This Solves

Cron agentTurn jobs can explicitly set toolsAllow: ["web_search"] while no usable web search provider is selected or available. Before this change, the run could still finish as ok with a model apology and no persisted cron diagnostic, leaving operators without an actionable signal.

Why This Change Was Made

This adds a narrow, non-fatal cron preflight diagnostic for the explicit web_search allowlist case. The diagnostic uses the same provider-selection path as the web_search runtime (listWebSearchProviders plus resolveWebSearchProviderId) and passes the cron agent directory so auth-profile-backed provider auto-detection matches the actual tool runtime.

The warning is merged into normal cron run diagnostics, delivery diagnostics, and error results. If the diagnostic availability check itself fails, the cron run remains non-fatal and records that failure as a warning instead of bypassing cron cleanup.

Refinement (re-review): the predicate also checks resolveCodexNativeSearchActivation against the resolved live model. When native OpenAI/Codex hosted web_search would handle the request (state === "native_active"), the warning is suppressed so operators on direct OpenAI Responses/Codex traffic do not see a misleading diagnostic. This covers the CS finding that the original predicate could warn even when native hosted search was active.

User Impact

Operators get a persisted warning such as:

web_search is in toolsAllow but no web search provider is selected or available. Enable or configure one with: openclaw plugins enable duckduckgo

Healthy runs with a selected provider are unchanged. Operators using native OpenAI/Codex hosted web_search no longer see a false positive. Wildcard/default tool access is not newly warned.

Evidence

  • node scripts/run-vitest.mjs run src/cron/isolated-agent/run.tools-allow.test.ts src/cron/run-diagnostics.test.ts --maxWorkers=1 -> 18/18 pass (8 tools-allow incl. new native_active no-warning case + 10 run-diagnostics).
  • node scripts/run-vitest.mjs run src/agents/tools/web-tools.enabled-defaults.test.ts src/agents/tool-allowlist-guard.test.ts --maxWorkers=1 -> 12/12 pass (7 web-tools enabled-defaults + 5 tool-allowlist-guard).
  • node scripts/run-oxlint.mjs --tsconfig tsconfig.json src/cron/isolated-agent/run.ts -> clean.
  • git diff --check origin/main...HEAD.

New regression test does not warn when native OpenAI/Codex hosted web_search is active for the resolved model mocks resolveCodexNativeSearchActivation to return native_active and asserts no diagnostic fires. The existing adds a warning diagnostic when web_search is allowed but no provider is enabled test continues to verify the warning path when native search is managed_only.

Autoreview history:

  • Fixed accepted finding: provider presence was not enough; now requires resolveWebSearchProviderId to select a provider.
  • Fixed accepted finding: provider selection now receives the cron agentDir for agent-scoped auth-profile detection.
  • Fixed accepted finding: diagnostic generation now runs inside the protected cron path and degrades diagnostic failures to warning diagnostics.
  • CS re-review refinement: added native hosted search activation check to suppress false positives; normalized toolsAllow check to Set to satisfy unicorn(prefer-set-has).

@clawsweeper

clawsweeper Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close as superseded: the linked bug is real, but #97677 is the stronger canonical fix path with sufficient proof, cleaner provider-selection wording, catalog-backed native-search handling, and policy-matcher coverage.

Root-cause cluster
Relationship: superseded
Canonical: #97677
Summary: This PR and the sibling candidates target the same cron web_search toolsAllow diagnostic issue; the proof-positive sibling is the best canonical landing path.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Canonical path: Close this branch and let maintainers review or land #97677 as the canonical fix for #97654.

So I’m closing this here and keeping the remaining discussion on #97677 and #97654.

Review details

Best possible solution:

Close this branch and let maintainers review or land #97677 as the canonical fix for #97654.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection plus the linked issue show current main has no cron preflight diagnostic before execution, and runWebSearch only errors when the tool actually executes.

Is this the best way to solve the issue?

No. A cron preflight warning is the right boundary, but this branch is no longer the best landing path because #97677 covers the same issue more completely with sufficient proof.

Security review:

Security review cleared: The diff only changes cron diagnostic TypeScript and tests; it does not touch secrets, dependencies, workflows, package metadata, permissions, or downloaded code execution paths.

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy read and applied: Root AGENTS.md and scoped src/agents, src/agents/tools, and src/plugins policies were read; the review applied the whole-path, provider-routing, best-fix, and external proof requirements. (AGENTS.md:25, ff1e7e1305cc)
  • Current main still has the linked diagnostic gap: Current main reaches cron payload handling and delivery context without a web_search provider preflight diagnostic for explicit toolsAllow, so the linked issue is valid but still needs one canonical fix. (src/cron/isolated-agent/run.ts:756, ff1e7e1305cc)
  • Managed provider selection contract checked: resolveWebSearchProviderId returns an empty string when no explicit or credential-backed managed web_search provider is selected, matching the reported no-provider state. (src/web-search/runtime.ts:166, ff1e7e1305cc)
  • This PR still misses catalog-backed native-search detection: The PR passes modelApi from direct params or cfg.models only; cron model selection can resolve models from the catalog, and resolveCodexNativeSearchActivation needs modelApi to identify openai-chatgpt-responses native search. (src/cron/isolated-agent/run.ts:387, 240c80c9ba03)
  • This PR still misses existing glob allowlist semantics: The predicate checks a Set for exact web_search after group expansion, while current tool policy matching supports glob patterns such as web_* for runtime allowlists. (src/cron/isolated-agent/run.ts:359, 240c80c9ba03)
  • Current policy matcher supports glob allowlists: isToolAllowedByPolicyName compiles allow and deny entries as glob patterns and is covered by tests for web_* matching. (src/agents/tool-policy-match.ts:14, ff1e7e1305cc)

Likely related people:

  • vincentkoc: Recent main history for src/web-search/runtime.ts includes provider-selection and runtime helper work that this diagnostic calls. (role: recent web_search runtime contributor; confidence: high; commits: e122cd09ab0f, b962c53e785c, c48b36a2556e; files: src/web-search/runtime.ts)
  • fuller-stack-dev: Authored the merged Codex hosted web search feature that owns the native path this diagnostic must avoid false-warning. (role: native hosted web_search feature contributor; confidence: high; commits: 1e0062b44a51; files: src/agents/codex-native-web-search-core.ts)
  • eleqtrizit: Authored merged native web_search policy enforcement in the same activation helper this PR calls. (role: native web_search policy contributor; confidence: high; commits: f0d8048aa3b4; files: src/agents/codex-native-web-search-core.ts)
  • Jerry-Xin: Recent main history on src/cron/isolated-agent/run.ts touched the same prepare/finalize cron runner surface. (role: recent cron isolated-run contributor; confidence: medium; commits: 30b39c6eab43; files: src/cron/isolated-agent/run.ts)
  • hclsys: Authored the recent merged tool-policy matcher change in the glob-capable matcher used by the canonical sibling PR. (role: tool policy matcher contributor; confidence: medium; commits: 8da9d8c55fb7; files: src/agents/tool-policy-match.ts)

Codex review notes: model internal, reasoning high; reviewed against ff1e7e1305cc.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jun 29, 2026
This was referenced Jun 29, 2026
Closed
Merged
@ly-wang19

ly-wang19 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@ly-wang19

ly-wang19 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favor of #97677 by @LiLan0125, which covers the same cron web_search toolsAllow diagnostic more completely.

Per ClawSweeper re-review: this PR is superseded by #97677, which:

  • Uses the existing tool policy matcher so exact names, groups, and glob-style allowlist entries (e.g. web_*) are handled consistently — my Set-based exact-match check missed glob semantics.
  • Skips default-derived allowlists and provider-owned native search cases where managed web_search is intentionally suppressed, addressing the catalog-backed native-search detection gap CS flagged on my refinement.
  • Ships source-level tsx proof invoking the real provider-selection and diagnostic helpers from source.

Rather than expand scope here to chase glob semantics and catalog-backed native detection, the cleaner landing path is to consolidate review on #97677.

Thanks to @LiLan0125 for the more complete fix.

@ly-wang19 ly-wang19 closed this Jun 29, 2026
This was referenced Jun 29, 2026
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Silent failure when web_search is in toolsAllow but no search provider plugin is enabled

1 participant

@ly-wang19