Skip to content

fix(plugins): ignore throwing provider policy hooks#88950

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

fix(plugins): ignore throwing provider policy hooks#88950
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-plugin-sdk-entrypoints-20260601b

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • wraps bundled provider-policy public-artifact hooks so a throwing hook is ignored instead of poisoning provider/config resolution
  • logs the plugin id and hook name when a bundled policy hook fails
  • preserves existing no-op short-circuit semantics, while allowing runtime provider hooks to run after an actual bundled-hook failure

Verification

  • node scripts/run-vitest.mjs src/plugins/provider-public-artifacts.test.ts src/plugins/provider-runtime.test.ts --reporter=dot
  • node_modules/.bin/oxfmt --check --threads=1 src/plugins/provider-public-artifacts.ts src/plugins/provider-public-artifacts.test.ts src/plugins/provider-runtime.ts src/plugins/provider-runtime.test.ts
  • node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.json src/plugins/provider-public-artifacts.ts src/plugins/provider-public-artifacts.test.ts src/plugins/provider-runtime.ts src/plugins/provider-runtime.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 the corrected provider-policy public-artifact hardening patch after addressing prior findings. Bundled provider policy hooks are wrapped to log and fail open; failure state is per current invocation and cleared before each hook call; provider-runtime callers continue to runtime provider hooks only when the current bundled hook failed, preserving no-op short-circuit semantics otherwise. Focus on fallback correctness, stale failure state, logging, cache behavior, and regression coverage.'
  • PR proof workflow rerun: current Real behavior proof completed successfully

Behavior addressed: A broken bundled provider-policy hook no longer crashes config/provider/thinking resolution, and runtime provider hooks are still available when the public-artifact hook is the broken part.

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 / 64 tests; oxfmt, oxlint, git diff --check, private-name scrub, autoreview, and PR proof workflow all passed.

Observed result after fix: Throwing synthetic fuzzplugin provider-policy hooks are logged and ignored; stale hook-failure state is cleared before later successful calls; runtime provider hooks still run after current bundled-hook failures.

What was not tested: Remote check:changed and pnpm build are blocked right now: Blacksmith Testbox auth is missing, AWS Crabbox is at the active lease limit, and direct local pnpm build in this Codex worktree attempted dependency reconciliation and aborted before build execution.

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

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 29, 2026, 8:24 PM ET / 00:24 UTC.

Summary
The PR wraps bundled provider-policy public-artifact hooks, logs thrown hook failures, and lets provider-runtime hooks run only after a bundled hook fails.

PR surface: Source +101, Tests +152. Total +253 across 4 files.

Reproducibility: yes. source-reproducible. Current main returns raw bundled provider-policy surfaces and provider-runtime invokes those hooks directly before runtime fallback, so a throwing hook propagates.

Review metrics: 1 noteworthy metric.

  • Provider-policy fail-open hooks: 4 changed. Config normalization, API-key resolution, config defaults, and thinking-profile hooks now ignore bundled-hook throws, so maintainers should review fallback semantics before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

  • Confirm provider-owner policy for fail-open behavior across config, API-key, defaults, and thinking hooks.
  • Run build or changed-gate validation against the current merge result before landing.

Risk before merge

  • [P1] Merging changes bundled provider-policy hook errors from hard failures into logged fail-open behavior across config normalization, API-key resolution, config defaults, and thinking-profile selection.
  • [P1] A broken API-key or config-default hook may now fall through to runtime hooks or no-op behavior, which can alter provider/auth resolution for existing setups.
  • [P1] Build or changed-gate validation against the current merge result is still missing for a public-artifact loader surface that imports new runtime logging/error helpers.

Maintainer options:

  1. Confirm Fail-Open Provider Policy (recommended)
    Have the provider/plugin owner explicitly accept logged fail-open behavior for config, API-key, defaults, and thinking hooks, then finish build or changed-gate validation before merge.
  2. Keep Auth And Defaults Stricter
    Rework the patch if API-key or config-default failures should remain fail-closed while lower-risk thinking or config-normalization hooks fail open.
  3. Pause If Policy Is Unsettled
    Keep the draft paused or close it if maintainers do not want provider-policy hook failures to move from hard errors to fail-open logging.

Next step before merge

  • [P2] Protected maintainer PR changes provider/auth fail-open semantics, so provider-owner review and final broad validation are the next action rather than an automated repair.

Security
Cleared: No dependency, workflow, package, or secret-handling regression was found; the new warning path uses the repository's error formatter, which redacts sensitive text before logging.

Review details

Best possible solution:

Land the centralized wrapper only after provider/plugin owners accept fail-open behavior for all four hook surfaces and build or changed-gate validation passes; split stricter auth/default behavior if that policy is not accepted.

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

Yes, source-reproducible. Current main returns raw bundled provider-policy surfaces and provider-runtime invokes those hooks directly before runtime fallback, so a throwing hook propagates.

Is this the best way to solve the issue?

Yes, conditionally. Central wrapping at the public-artifact seam is the narrowest inspected repair if fail-open is the intended policy; if auth/default hooks must remain fail-closed, the PR should split those before merge.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority provider/plugin hardening PR with limited blast radius but real provider-resolution and config impact.
  • merge-risk: 🚨 compatibility: The patch changes bundled provider-policy hook errors from thrown failures to logged fail-open behavior for existing provider setups.
  • merge-risk: 🚨 auth-provider: The changed hooks participate in provider config, API-key, defaults, and thinking-profile resolution, which can affect auth/provider behavior when a hook fails.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate does not apply to this maintainer-labeled MEMBER PR; the body still records local terminal validation and GitHub shows successful Real behavior proof runs.
Evidence reviewed

PR surface:

Source +101, Tests +152. Total +253 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 2 113 12 +101
Tests 2 152 0 +152
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 265 12 +253

What I checked:

  • Repository policy read: The root and scoped plugin policies were read; provider routing, fallback behavior, and public-artifact loading are compatibility-sensitive review surfaces. (AGENTS.md:31, e87b0df67575)
  • Scoped plugin validation policy: The scoped plugin guide says loader, registry, activation, or public-artifact changes that can change bundled plugin import fanout should run pnpm build; the PR body says build and remote changed-gate proof were not completed. (src/plugins/AGENTS.md:77, e87b0df67575)
  • Current main raw surface behavior: Current main caches and returns the raw bundled provider-policy module, so exported hook exceptions are not caught at the public-artifact boundary. (src/plugins/provider-public-artifacts.ts:58, e87b0df67575)
  • Current main runtime precedence: Current main calls bundled normalizeConfig, resolveConfigApiKey, resolveThinkingProfile, and applyConfigDefaults before runtime fallback, so a thrown bundled hook propagates through provider/config resolution. (src/plugins/provider-runtime.ts:443, e87b0df67575)
  • PR wrapper behavior: The PR head wraps each bundled provider-policy hook, records thrown failures in a WeakSet, logs a redacted warning, and returns undefined instead of rethrowing. (src/plugins/provider-public-artifacts.ts:46, f02b767441bd)
  • PR runtime fallback behavior: The PR head consumes the hook-failure marker before short-circuiting, so runtime provider hooks remain skipped on normal no-op returns but run after current bundled-hook failures. (src/plugins/provider-runtime.ts:420, f02b767441bd)

Likely related people:

  • vincentkoc: Git history ties the bundled provider-policy artifact seam, provider config policy contexts, and provider hook runtime split to commits by Vincent Koc in the touched files. (role: feature-history owner and recent area contributor; confidence: high; commits: 45663f287937, 10ee46c37387, a7436c8b4a93; files: src/plugins/provider-public-artifacts.ts, src/plugins/provider-runtime.ts, src/plugins/provider-config-context.types.ts)
  • Peter Steinberger: Earlier provider-policy and provider-defaults refactors moved related provider policy hooks into the plugin boundary near the same runtime/config surface. (role: adjacent provider-policy refactor owner; confidence: medium; commits: d042192c7c9c, b167ad052c76; files: src/plugins/provider-runtime.ts, src/config/provider-policy.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-21T12:58:29.266Z sha f02b767 :: needs maintainer review before merge. :: none

@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. 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. labels Jun 1, 2026
@vincentkoc vincentkoc mentioned this pull request Jun 1, 2026
Closed
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Closing this fuzz-only hardening PR. The only failing state is a synthetic fixture using hostile in-process objects, throwing getters or callbacks, or similarly malformed metadata; there is no linked production report or supported external contract requiring that state.

This adds broad fail-soft behavior and substantial regression surface across consumers. If a real plugin or provider demonstrates the failure, please open a focused issue with the observed payload and stack so validation can live at the single owning registration/parsing boundary. Thanks for exploring it.

@steipete steipete closed this Jul 5, 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: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vincentkoc @steipete