Skip to content

fix(plugins): skip broken web provider factories#88936

Closed
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-plugin-sdk-entrypoints-proof-20260601
Closed

fix(plugins): skip broken web provider factories#88936
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-plugin-sdk-entrypoints-proof-20260601

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • skips bundled web provider factories that throw during public-artifact discovery
  • keeps healthy sibling providers active when another selected plugin's factory is broken
  • preserves the existing null fallback path for modules that simply do not expose a matching provider factory

Verification

  • node scripts/run-vitest.mjs src/plugins/web-provider-public-artifacts.explicit-fast-path.test.ts --reporter=dot
  • node_modules/.bin/oxfmt --check --threads=1 src/plugins/web-provider-public-artifacts.explicit.ts src/plugins/web-provider-public-artifacts.explicit-fast-path.test.ts
  • node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.json src/plugins/web-provider-public-artifacts.explicit.ts src/plugins/web-provider-public-artifacts.explicit-fast-path.test.ts
  • git diff --check
  • private reported-name scrub over the local spec and changed files
  • .agents/skills/autoreview/scripts/autoreview --mode local --prompt 'Review this focused plugin public-artifact provider hardening patch. It changes bundled web provider factory exceptions from fatal errors into per-plugin unavailability so one broken provider factory cannot poison assistant startup or sibling providers. Focus on runtime behavior, fallback semantics, diagnostics risk, and whether the tests cover the crash surface.'
  • AWS Crabbox changed gate: node scripts/crabbox-wrapper.mjs run --provider aws --idle-timeout 90m --ttl 240m --timing-json --shell -- "corepack pnpm check:changed" (run_70b582d21a6b, lease cbx_ec1a78220a74)

Behavior addressed: A bundled plugin public-artifact provider factory can throw during discovery without poisoning sibling web providers or the assistant startup path.

Real environment tested: Local Codex gwt worktree on macOS using the repo Vitest node wrapper and repo lint/format wrappers.

Exact steps or command run after this patch: See Verification above.

Evidence after fix: Focused Vitest passed 1 shard / 6 tests; oxfmt, oxlint, git diff --check, private-name scrub, autoreview, and AWS Crabbox check:changed all passed.

Observed result after fix: A plugin whose matching web provider factories all throw contributes no provider entries; a healthy sibling provider still resolves from the same selected provider set.

What was not tested: Full repo test suite was not run; this patch is limited to the plugin web-provider public-artifact loader and focused regression coverage plus changed-gate proof.

@vincentkoc vincentkoc self-assigned this Jun 1, 2026
@openclaw-barnacle openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels Jun 1, 2026
@clawsweeper

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed June 29, 2026, 6:03 PM ET / 22:03 UTC.

Summary
The PR changes bundled web-provider public-artifact factory failures from fatal all-fail initialization errors into per-plugin unavailability and updates fast-path tests for all-failing and mixed selected-provider cases.

PR surface: Source -9, Tests +20. Total +11 across 2 files.

Reproducibility: yes. Current main and v2026.6.10 throw for an all-failing selected bundled web-provider factory, while PR head changes that same source path to return an empty provider list.

Review metrics: 1 noteworthy metric.

  • Provider failure behavior: 1 all-failing factory path changed. The PR changes what existing selected bundled provider setups observe when every matching public-artifact factory throws.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦞 diamond lobster
Patch quality: 🦪 silver shellfish
Result: blocked by patch quality or review findings.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Add or confirm an observable diagnostic for all-failing selected provider artifacts, or get explicit maintainer approval that silent unavailability is the desired contract.
  • Refresh the draft branch against current main after the failure-policy adjustment.

Risk before merge

  • [P1] Existing setups that currently see a clear initialization failure for a selected broken bundled web-provider artifact may instead see that provider silently disappear.
  • [P2] Setup-mode callers return any bundled artifact array, so an empty array can suppress runtime fallback/loading and diagnostics for the selected provider set.
  • [P1] The web-provider loader would diverge from sibling public-artifact loaders that still fail loudly when every matching factory fails.
  • [P1] The draft branch is currently conflicting against main, so the policy fix needs refresh on the current public-artifact loader shape before merge.

Maintainer options:

  1. Add an observable failure path before merge (recommended)
    Preserve sibling-provider isolation while keeping all-failing selected provider artifacts loud, diagnostic-backed, or otherwise explicitly approved before returning an empty provider set.
  2. Approve silent provider unavailability
    Maintainers can intentionally accept that broken selected bundled web-provider artifacts disappear from availability instead of failing setup/startup loudly.
  3. Pause for sibling loader policy
    Hold the PR if web providers should stay aligned with sibling public-artifact loader failure semantics before choosing the narrower implementation.

Next step before merge

  • [P2] Protected draft maintainer PR is currently conflicting and blocked on provider-failure policy, so this needs maintainer/author resolution rather than an automated repair lane.

Security
Cleared: The diff only changes in-repo plugin public-artifact factory error handling and tests; it adds no dependency, workflow, secret, permission, or supply-chain surface.

Review findings

  • [P1] Keep all-failing provider artifacts observable — src/plugins/web-provider-public-artifacts.explicit.ts:139
Review details

Best possible solution:

Keep healthy sibling-provider isolation, but preserve an observable all-failing provider path through a thrown error, diagnostic-backed failure result, or explicit maintainer-approved policy applied consistently across public-artifact loaders.

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

Yes. Current main and v2026.6.10 throw for an all-failing selected bundled web-provider factory, while PR head changes that same source path to return an empty provider list.

Is this the best way to solve the issue?

No, not as written. Sibling-provider isolation is a plausible bug fix, but the all-failing selected provider path should stay observable or receive explicit maintainer approval before becoming fail-open.

Full review comments:

  • [P1] Keep all-failing provider artifacts observable — src/plugins/web-provider-public-artifacts.explicit.ts:139
    Returning [] here makes a selected plugin whose matching public-artifact factories all throw look like an authoritative empty provider result. Setup callers return any array from this loader, so the current loud initialization failure and fallback/diagnostic path are skipped; preserve the loud error or add an explicit diagnostic/failure result before fail-open.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a focused plugin provider hardening PR with limited blast radius, but it changes provider-routing failure semantics.
  • merge-risk: 🚨 compatibility: Existing setups that currently fail loudly for a broken selected provider factory may instead see the provider disappear during upgrade.
  • merge-risk: 🚨 auth-provider: The affected loader feeds web provider selection and setup paths, so fail-open behavior can alter provider availability and credential/setup diagnostics.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦞 diamond lobster and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The external contributor proof gate does not apply to this maintainer-labeled MEMBER PR, though the body lists focused local checks and an AWS Crabbox changed gate.
Evidence reviewed

PR surface:

Source -9, Tests +20. Total +11 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 11 20 -9
Tests 1 23 3 +20
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 34 23 +11

What I checked:

Likely related people:

  • vincentkoc: Authored the explicit bundled web-provider artifact split and the merged predecessor that introduced per-factory isolation while keeping all-failing artifacts loud. (role: feature owner and recent area contributor; confidence: high; commits: 447ab8102a33, 53990d5bbf00, aa69b12d0086; files: src/plugins/web-provider-public-artifacts.explicit.ts, src/plugins/web-provider-public-artifacts.explicit-fast-path.test.ts)
  • steipete: Authored the shared web-provider runtime helper that decides when setup-mode public-artifact arrays are authoritative versus when plugin runtime loading should continue. (role: adjacent runtime helper author; confidence: medium; commits: 8838fdc91625; files: src/plugins/web-provider-runtime-shared.ts, src/plugins/web-search-providers.runtime.ts, src/plugins/web-fetch-providers.runtime.ts)
  • luoyanglang: Merged current-main rewritten/grafted plugin metadata-check work that now owns the exact loader, runtime helper, and fast-path test snapshots this PR conflicts with. (role: recent area contributor; confidence: medium; commits: 68ddb9744f78; files: src/plugins/web-provider-public-artifacts.explicit.ts, src/plugins/web-provider-runtime-shared.ts, src/plugins/web-provider-public-artifacts.explicit-fast-path.test.ts)
  • gumadeiras: Added recent fast-path test coverage around web-search contract runtime loads in the same explicit public-artifact test file. (role: adjacent test contributor; confidence: medium; commits: 2482e70fb8c0; files: src/plugins/web-provider-public-artifacts.explicit-fast-path.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-06-21T07:41:20.749Z sha 84449cf :: found issues before merge. :: [P1] Keep all-failing provider artifacts observable

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jun 1, 2026
@vincentkoc vincentkoc mentioned this pull request Jun 1, 2026
Closed
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 19, 2026
@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Closing this old provider fallback PR. A plugin factory or normalization hook can genuinely throw, but silently ignoring that failure and continuing with a manifest, sibling provider, or alternate transport changes routing and fail-closed behavior. There is no linked production report or explicit fallback contract justifying that compatibility/security tradeoff.

The right fix needs an observed failure, an owner decision about whether startup/request routing should fail or degrade, and diagnostics at the provider boundary. It should not turn arbitrary plugin exceptions into implicit fallback policy. Thanks for investigating the failure mode.

@steipete steipete closed this Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@vincentkoc vincentkoc

Labels

maintainer Maintainer-authored PR merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vincentkoc @steipete