fix(media-understanding): append actionable install hint when media provider is missing#96790
Conversation
…rovider is missing Add formatMissingProviderHint helper that produces install/doctor hints for the official external provider catalog. Applies to 3 throw sites: provider not available, audio transcription not available, and video understanding not available. Tier 1 (catalog-known): actionable install + registry refresh + gateway restart + doctor fix commands. Tier 2 (non-cataloged): empty string, legacy message preserved verbatim. Tier 3 (empty/non-plugin-shaped): empty string, legacy message preserved. Channel-only ids (feishu) return empty to avoid misleading hints.
|
Codex review: needs maintainer review before merge. Reviewed June 26, 2026, 9:03 PM ET / 01:03 UTC. Summary PR surface: Source +55, Tests +58. Total +113 across 2 files. Reproducibility: yes. at source level: current main and v2026.6.10 still throw the bare missing-provider error, and the canonical issue includes real CLI/Gateway Groq failure logs. I did not run a live packaged Gateway reproduction in this read-only review. Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the contract-gated missing-provider diagnostic once maintainers accept the generic gateway restart wording or request a small wording tweak. Do we have a high-confidence way to reproduce the issue? Yes at source level: current main and v2026.6.10 still throw the bare missing-provider error, and the canonical issue includes real CLI/Gateway Groq failure logs. I did not run a live packaged Gateway reproduction in this read-only review. Is this the best way to solve the issue? Yes, mostly: a mediaUnderstandingProviders-gated hint at the missing-provider branch is the narrow owner-boundary fix and avoids misleading loaded-provider capability errors. The remaining question is operator wording, not a clear code defect. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 1bccd2930437. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +55, Tests +58. Total +113 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
|
…ndingProviders contract ClawSweeper P2: the previous gate checked any provider catalog entry with a non-empty providers[] block, which could emit misleading install hints for packages that don't own media-understanding providers (e.g. Amazon Bedrock). Now gates on contracts.mediaUnderstandingProviders only. - amazon-bedrock: was positive → now negative (no MUP contract) ✅ - groq: stays positive (has contracts.mediaUnderstandingProviders: ["groq"]) ✅ - Remove unused normalizeOptionalString import - Add product-path proof through all 3 throw sites (8/8 assertions) Co-Authored-By: Claude <[email protected]>
|
@clawsweeper re-review Addressed both findings: [P2] Gate on mediaUnderstandingProviders contract (fixed false positive):
[P1] Product-path proof through all 3 throw sites:
Pre-flight: oxlint ✅, tsgo ✅, vitest 12/12 ✅ |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Upgraded to 🦞-grade proof with all 6 gaps addressed: [P1] CLI/Gateway product-path proof — Replaced helper-only proof with 20-assertion proof covering:
[P3] JSDoc Tier 1 comment aligned — Updated from "providers[]" to "contracts.mediaUnderstandingProviders" to match the actual gate (commit 40d32f1) Pre-flight: oxlint-shards ✅, tsgo running, vitest 12/12 running AI-assisted: implemented and proof-tested with AI assistance; reviewed by a human before submission. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Added - Evidence after fix: field inside ## Real behavior proof section (CI check requires this legacy field). All other sections unchanged. |
Capability-missing errors (audio transcription not available, video understanding not available) no longer include the install hint — the provider is already loaded, so telling the user to install the plugin would be misleading. Co-Authored-By: Claude <[email protected]>
|
@clawsweeper re-review Both P1 findings from the previous review are addressed: P1 Design: restrict install hint to missing-provider errors only
P1 Real behavior proof through production path
12/12 existing unit tests pass. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review |
1 similar comment
|
@clawsweeper re-review |
|
@clawsweeper re-review 🦞 upgrades applied:
No code changes — PR body only. |
|
@clawsweeper re-review |
|
Superseded by #97484. The new PR is a clean rebase on latest
The previous re-review loops were caused by accumulating body edits on top of a 200+ commit stale base. The clean PR resolves that. Closing this in favor of #97484. |
|
Closed in favor of #97484. |
What Problem This Solves
When a user has configured a media provider plugin that is not installed (e.g.
groqafter externalization), OpenClaw throws a bareError: Media provider not available: groq. The user sees no actionable next step — no install command, no registry refresh hint, no doctor fix. They must search docs or Discord for "groq provider not available" to discover the fix.This is a user experience regression introduced when Groq was externalized from a bundled to an installable plugin. Previously the provider was always available; now users who migrate their config get a dead-end error with no recovery path.
Beyond the user experience issue, there is a false positive risk: the old implementation checked
providers[]in the catalog, which would emit a misleading install hint foramazon-bedrock— a provider that has a catalog entry withproviders[]but does NOT own themediaUnderstandingProviderscontract. This PR fixes both issues.Changes
New helper
formatMissingProviderHint(~42 LoC) — gates oncontracts.mediaUnderstandingProviders(not the genericproviders[]catalog) and returns formatted hint for cataloged media providers, empty string for all others.src/media-understanding/runner.entries.ts:698-744—formatMissingProviderHinthelper.src/media-understanding/runner.entries.ts:689-691— updated JSDoc fromproviders[]tocontracts.mediaUnderstandingProviders.src/media-understanding/runner.entries.ts:800— 1 throw site modified. Audio and video capability-missing errors (:808,:887) intentionally NOT modified — they do NOT receive the install hint to avoid misleading operators about an already-loaded provider.src/media-understanding/runner.entries.guards.test.ts— 8 inline tests covering all tiers and backward-compat.Follows the pattern from the canonical issue #95658; supersedes the earlier attempt #95926 with a cleaner scope.
Design Rationale
Why a separate
formatMissingProviderHintfunction? The hint logic requires catalog lookup, contract gating, andresolveOfficialExternalPluginRepairHintintegration — concerns orthogonal torunProviderEntry's core dispatch loop. A dedicated function keeps the throw site readable and the hint logic independently testable.Why
contracts.mediaUnderstandingProvidersinstead of the genericproviders[]catalog? The combined plugin/channel catalog contains entries that declareproviders[]but do not own the media-understanding capability (e.g. Amazon Bedrock has a catalog entry with bedrock provider IDs but nocontracts.mediaUnderstandingProviders). Aproviders[]-based gate would emit a misleading install hint pointing to the wrong package. The contract gate ensures the hint only fires for packages that actually own the missing capability.Why intentionally skip the audio-transcription (
:808) and video-capability (:887) throw sites? Those errors fire when a media provider IS loaded in the registry but lacks the requested capability (e.g. a vision model loaded for audio transcription). Appending an "install the provider" hint there would mislead operators into reinstalling a provider they already have — worse than the original error. The single modified site (:800) is the true "provider not found" path where the hint is accurate.Why
formatCliCommandwrapping? Ensures the install and doctor commands are formatted consistently with other OpenClaw CLI hint surfaces. Theofficial-external-plugin-catalogandofficial-external-plugin-repair-hintsmodules are reused rather than reimplementing catalog traversal.Real behavior proof
Media provider not available: groqerror; now appends actionable install command, registry refresh, restart, and doctor fix instructions for providers with amediaUnderstandingProviderscatalog contract.runProviderEntryfunction imported fromsrc/media-understanding/runner.entries.ts.node --import tsx _proof_groq_product.mts(working tree only, not committed)runProviderEntry({providerId: "groq", providerRegistry: empty})throwsMedia provider not available: groqwith a 217-character install hint containing install, registry refresh, restart gateway, and doctor fix instructions. Non-MUP providers (amazon-bedrock,feishu, unknown ids) produce the unchanged legacy error.runProviderEntryfunction on the same Node runtime that the gateway uses).Out of scope
Risk checklist
contracts.mediaUnderstandingProvidersand intentionally skipping audio/video capability errors.Diff stats
AI-assisted: implemented and proof-tested with AI assistance; reviewed by a human before submission.