Skip to content

fix(plugins): ignore throwing provider runtime hooks#88959

Closed
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-provider-runtime-hooks-20260601
Closed

fix(plugins): ignore throwing provider runtime hooks#88959
vincentkoc wants to merge 1 commit into
mainfrom
fuzz-provider-runtime-hooks-20260601

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 1, 2026

Copy link
Copy Markdown
Member

Summary

  • catch throwing provider runtime normalizeModelId and normalizeTransport hooks so model/transport normalization keeps its existing fallback paths
  • log sanitized provider hook failures instead of letting a bad plugin poison provider lookup
  • keep transport fallback scanning sibling providers, including same-package provider surfaces, while avoiding rerunning the same failed provider hook

Verification

  • node scripts/run-vitest.mjs src/plugins/provider-runtime.test.ts --reporter=dot
  • node_modules/.bin/oxfmt --check --threads=1 src/plugins/provider-runtime.ts src/plugins/provider-runtime.test.ts
  • OPENCLAW_OXLINT_SKIP_LOCK=1 node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.core.json src/plugins/provider-runtime.ts src/plugins/provider-runtime.test.ts
  • git diff --check HEAD~1..HEAD
  • .agents/skills/autoreview/scripts/autoreview --mode local --prompt "Review the updated OpenClaw provider-runtime patch after changing the transport skip key to plugin package plus provider id. Focus on whether throwing normalizeModelId/normalizeTransport hooks preserve manifest and sibling transport fallback, including same-package multi-provider plugins."
  • node scripts/crabbox-wrapper.mjs run --provider aws --idle-timeout 90m --ttl 240m --timing-json --shell -- "corepack pnpm check:changed" (run_8c7e8041e188, lease cbx_739f083002ff, exit 0; lanes core, coreTests)

Real behavior proof

Behavior addressed: provider runtime normalization hooks that throw no longer abort model-id or transport fallback resolution.

Real environment tested: local focused Vitest/format/lint/autoreview plus AWS Crabbox Linux changed gate.

Exact steps or command run after this patch: node scripts/run-vitest.mjs src/plugins/provider-runtime.test.ts --reporter=dot; node scripts/crabbox-wrapper.mjs run --provider aws --idle-timeout 90m --ttl 240m --timing-json --shell -- "corepack pnpm check:changed".

Evidence after fix: focused provider-runtime tests passed 58/58; autoreview reported no accepted/actionable findings; AWS Crabbox run run_8c7e8041e188 exited 0.

Observed result after fix: throwing normalizeModelId returns to manifest fallback, and throwing matched normalizeTransport keeps scanning sibling transport hooks without duplicate same-provider invocation.

What was not tested: direct Blacksmith Testbox was not available because blacksmith testbox list reported missing auth.

@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 27, 2026, 9:16 AM ET / 13:16 UTC.

Summary
The PR wraps provider normalizeModelId and normalizeTransport hooks with catch-and-warn handling and adds provider-runtime regression tests for thrown hook fallback.

PR surface: Source +41, Tests +76. Total +117 across 2 files.

Reproducibility: yes. from source inspection: create a provider plugin whose normalizeModelId or normalizeTransport throws, and current main invokes that hook directly before fallback handling. I did not run tests because this was a read-only review.

Review metrics: 1 noteworthy metric.

  • Provider isolation invariants: 1 guard + 1 model-id scoped lookup to preserve. Both current-main protections constrain provider hook fallback and must survive the rebase before this provider-runtime change is safe to merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🌊 off-meta tidepool
Patch quality: 🧂 unranked krab
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:

  • Rebase onto current main while preserving the configured-provider guard and model-id scoped transport lookup.
  • Rerun the focused provider-runtime test and changed gate after the refresh.

Risk before merge

  • [P1] The branch is conflicting in src/plugins/provider-runtime.test.ts, and its proof predates current-main configured-provider and model-id scoped transport safeguards.
  • [P2] Provider hook fallback changes can alter existing custom provider API/baseUrl selection if the refresh broad-scans unrelated transport hooks for explicitly configured providers.

Maintainer options:

  1. Rebase preserving provider isolation (recommended)
    Refresh the branch onto current main, keep the configured-provider guard and model-id scoped transport lookup, and rerun the provider-runtime focused tests plus changed gate.
  2. Accept broader transport fallback deliberately
    Maintainers could intentionally allow broader fallback, but that should come with explicit provider-routing approval and upgrade coverage for custom configured providers.
  3. Replace the stale draft
    If the conflict is no longer worth resolving, close this draft after opening a narrow replacement PR carrying only the throwing-hook hardening on current main.

Next step before merge

  • [P2] This protected maintainer draft needs an author or maintainer refresh and routing review rather than an autonomous repair lane.

Security
Cleared: No concrete security or supply-chain issue was found; the diff changes provider-runtime exception handling/tests and uses existing sanitization patterns.

Review findings

  • [P1] Preserve the configured-provider transport guard — src/plugins/provider-runtime.ts:427
Review details

Best possible solution:

Land a refreshed provider-runtime hardening patch that catches optional hook failures while preserving configured-provider isolation, model-id scoped lookup, and focused regression coverage.

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

Yes from source inspection: create a provider plugin whose normalizeModelId or normalizeTransport throws, and current main invokes that hook directly before fallback handling. I did not run tests because this was a read-only review.

Is this the best way to solve the issue?

No for the current PR head. Catch-and-warn handling is the right layer for optional provider hooks, but this branch needs a rebase that preserves current configured-provider and model-id scoping safeguards.

Full review comments:

  • [P1] Preserve the configured-provider transport guard — src/plugins/provider-runtime.ts:427
    Current main returns before broad provider-hook scanning when models.providers explicitly configures the provider. This PR head falls through to resolveProviderPluginsForHooks(params), so the refresh must keep that guard or explicitly configured custom providers can have their API/baseUrl rewritten by unrelated transport hooks.
    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 c5d34c8376f8.

Label changes

Label justifications:

  • P2: This is a bounded provider-runtime bugfix with limited blast radius, but it still needs compatibility-preserving refresh before merge.
  • merge-risk: 🚨 compatibility: An incorrect refresh could change existing custom provider API/baseUrl behavior by broad-scanning unrelated transport hooks.
  • merge-risk: 🚨 auth-provider: The touched runtime participates in provider routing and model transport selection, so stale fallback behavior can route configured providers through the wrong provider-family hook.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • 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-authored draft; the PR body proof is useful but stale relative to current-main provider-isolation changes.
Evidence reviewed

PR surface:

Source +41, Tests +76. Total +117 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 46 5 +41
Tests 1 76 0 +76
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 122 5 +117

What I checked:

  • Root and scoped policy read: Root AGENTS.md and src/plugins/AGENTS.md were read; plugin provider routing, fallback behavior, and provider/plugin compatibility are explicitly review-sensitive for this surface. (AGENTS.md:26, c5d34c8376f8)
  • Current main still has the throwing-hook failure path: Current main calls plugin?.normalizeModelId?.(...) and matchedPlugin?.normalizeTransport?.(...) directly before fallback handling, so a thrown provider hook can still abort normalization. (src/plugins/provider-runtime.ts:395, c5d34c8376f8)
  • Current main has provider-isolation invariants to preserve: Current main returns before broad transport-hook scanning for explicitly configured providers and has tests for both configured-provider isolation and model-id scoped transport lookup. (src/plugins/provider-runtime.ts:416, c5d34c8376f8)
  • PR head predates those current-main safeguards: The PR head wraps hook calls but its normalizeProviderTransportWithPlugin hunk falls through to broad resolveProviderPluginsForHooks(params) without the current-main configured-provider guard, and applyProviderResolvedTransportWithPlugin does not pass the scoped model id. (src/plugins/provider-runtime.ts:427, 42398b15f272)
  • Branch is currently unmergeable against main: Live GitHub reports the draft PR as mergeable: CONFLICTING / mergeStateStatus: DIRTY, and git merge-tree shows provider-runtime test conflicts around the new throwing-transport test and current-main isolation tests. (src/plugins/provider-runtime.test.ts:1768, 42398b15f272)
  • Provider hook contract checked: The provider plugin contract documents normalizeModelId and normalizeTransport as provider-owned model-id and transport-family cleanup hooks, so changing failure and fallback behavior affects provider routing semantics. (src/plugins/types.ts:1354, c5d34c8376f8)

Likely related people:

  • vincentkoc: Authored this PR and recent current-main provider-runtime commits that added custom-provider runtime fanout and exact custom-provider model safeguards. (role: recent area contributor; confidence: high; commits: 42398b15f272, d07ba5f2656d, 160aad6fb359; files: src/plugins/provider-runtime.ts, src/plugins/provider-runtime.test.ts)
  • fuller-stack-dev: Recent provider hook work changed structured provider hook dispatch and broad fallback constraints in the same runtime/test area. (role: adjacent provider-hook scoping contributor; confidence: medium; commits: 89975eea24a5; files: src/plugins/provider-runtime.ts, src/plugins/provider-runtime.test.ts)
  • steipete: Recent provider identity and provider support commits touched the same provider runtime and plugin normalization surface. (role: adjacent provider-runtime contributor; confidence: medium; commits: 8c7e5c691891, 4c33aaa86c16, d92b3b5cc2bd; files: src/plugins/provider-runtime.ts, src/plugins/provider-runtime.test.ts, src/plugins/types.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.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. 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. labels Jun 1, 2026
@vincentkoc vincentkoc mentioned this pull request Jun 1, 2026
Closed
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. 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. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. proof: sufficient ClawSweeper judged the real behavior proof convincing. labels Jun 19, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 21, 2026
Closed
@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: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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