Skip to content

fix(media-generation): preserve trimmed default model flag#96430

Merged
vincentkoc merged 1 commit into
openclaw:mainfrom
lin-hongkuan:codex/media-generation-trimmed-default
Jun 25, 2026
Merged

fix(media-generation): preserve trimmed default model flag#96430
vincentkoc merged 1 commit into
openclaw:mainfrom
lin-hongkuan:codex/media-generation-trimmed-default

Conversation

@lin-hongkuan

@lin-hongkuan lin-hongkuan commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Static media-generation catalog synthesis trims and de-duplicates provider model ids before emitting rows. However, the default flag compared each trimmed row against the raw provider.defaultModel string.

If provider metadata supplied a default model with harmless surrounding whitespace, the synthesized row used the trimmed model id but lost default: true.

Why This Change Was Made

The default comparison should use the same normalized model value as the generated rows. This patch normalizes defaultModel through the same unique/trim helper before marking catalog entries.

User Impact

Media generation provider catalogs keep the intended default model marker even when provider metadata includes surrounding whitespace on defaultModel.

Evidence

Live behavior proof from this branch:

{"kind":"video_generation","provider":"example","model":"video-default","source":"static","capabilities":{},"default":true}

Focused validation:

RUN  v4.1.8 C:/Users/lin20/Documents/New project 3/openclaw

Test Files  1 passed (1)
     Tests  3 passed (3)
  Start at  20:53:45
  Duration  940ms (transform 647ms, setup 499ms, import 14ms, tests 130ms, environment 0ms)

Checking formatting...

All matched files use the correct format.
Finished in 1367ms on 2 files using 32 threads.

git diff --check produced no output.

AI-assisted: OpenAI Codex helped inspect media-generation catalog normalization and add focused regression coverage.

@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 25, 2026, 7:10 AM ET / 11:10 UTC.

Summary
The PR normalizes the media-generation provider defaultModel before comparing it to synthesized catalog row model ids and adds a regression test for whitespace-padded defaults.

PR surface: Source +1, Tests +23. Total +24 across 2 files.

Reproducibility: yes. Source inspection on current main shows the trimmed row model is compared against raw defaultModel; the added whitespace-padded test case is a high-confidence reproduction, though I did not run tests in this read-only review.

Review metrics: none identified.

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:

  • none.

Next step before merge

  • No automated repair is needed because the PR is already a focused implementation with no actionable findings; maintainers should review and land it after required CI.

Security
Cleared: The diff touches only a pure TypeScript catalog helper and colocated Vitest test, with no dependency, workflow, secret, network, install, or generated-artifact surface.

Review details

Best possible solution:

Land this narrow normalization fix after required CI so media-generation catalog row ids and default-model comparison share the same helper.

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

Yes. Source inspection on current main shows the trimmed row model is compared against raw defaultModel; the added whitespace-padded test case is a high-confidence reproduction, though I did not run tests in this read-only review.

Is this the best way to solve the issue?

Yes. Normalizing defaultModel through the same trimmed-string helper used for row model ids is the narrowest maintainable fix at the shared media-generation catalog synthesis boundary.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority media-generation catalog correctness fix with limited blast radius in static provider model rows.
  • 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. Sufficient (live_output): The PR body includes copied after-fix live output showing the synthesized video catalog row with default: true, plus focused test and formatting output.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied after-fix live output showing the synthesized video catalog row with default: true, plus focused test and formatting output.
Evidence reviewed

PR surface:

Source +1, Tests +23. Total +24 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 2 1 +1
Tests 1 23 0 +23
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 25 1 +24

What I checked:

  • Repository policy read: Root AGENTS.md was read fully, no package-scoped AGENTS.md exists under packages/, and only a Telegram maintainer note exists, so root PR-review and package-boundary guidance applied. (AGENTS.md:1, 4ecb45bf7729)
  • Current-main bug path: Current main trims provider defaults and models through uniqueModels, but marks a row as default only when the trimmed row model equals raw params.provider.defaultModel, so surrounding whitespace prevents default: true. (packages/media-generation-core/src/catalog.ts:65, 4ecb45bf7729)
  • Shared normalization contract: uniqueTrimmedStrings trims strings, drops empty values, and preserves first-seen unique values, matching the row-model normalization the default comparison should use. (packages/media-generation-core/src/string.ts:13, 4ecb45bf7729)
  • PR diff: The head diff computes a normalized defaultModel once, compares synthesized row models to that normalized value, and adds a test where defaultModel: " video-default " emits default: true. (packages/media-generation-core/src/catalog.ts:53, b2bae26b77dc)
  • Runtime caller boundary: Static media-generation model catalog providers call synthesizeMediaGenerationCatalogEntries from the plugin registration boundary, so the helper-level fix reaches registered media catalog rows without provider-specific patches. (src/plugins/model-catalog-registration.ts:182, 4ecb45bf7729)
  • Tool-list caller: Media generation list actions use both listMediaGenerationProviderModels and synthesizeMediaGenerationCatalogEntries, so the PR keeps listed models and catalog rows aligned on the same normalization owner. (src/agents/tools/media-generate-tool-actions-shared.ts:72, 4ecb45bf7729)

Likely related people:

  • steipete: Merged history ties the unified model catalog registration and the extraction of the media-generation core catalog helper to this account. (role: introduced behavior and recent area owner; confidence: high; commits: 311e4608d173, 7c1484d637c8; files: packages/media-generation-core/src/catalog.ts, packages/media-generation-core/src/catalog.test.ts, src/plugins/model-catalog-registration.ts)
  • vincentkoc: Recent adjacent work touched provider catalog projection and voice catalog/default-model registration patterns near the same model catalog surface. (role: adjacent provider/model-catalog contributor; confidence: medium; commits: 2fef80aee5dc, 27b15a19e84c; files: src/plugins/model-catalog-registration.ts, packages/speech-core/voice-models.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 24, 2026
@vincentkoc
vincentkoc force-pushed the codex/media-generation-trimmed-default branch from 2dc49de to b2bae26 Compare June 25, 2026 10:57
@vincentkoc
vincentkoc merged commit 2e6e17f into openclaw:main Jun 25, 2026
130 of 131 checks passed
@vincentkoc

vincentkoc commented Jun 25, 2026

Copy link
Copy Markdown
Member

Merged via squash.

@clawsweeper clawsweeper Bot mentioned this pull request Jun 25, 2026
Merged
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 26, 2026
@lin-hongkuan
fix(media-generation): preserve trimmed default model flag (openclaw#…
b6ff345 
…96430)
QiuYuang pushed a commit to QiuYuang/openclaw that referenced this pull request Jul 1, 2026
@lin-hongkuan @QiuYuang
fix(media-generation): preserve trimmed default model flag (openclaw#…
cfb5eac 
…96430)
@lsr911 lsr911 mentioned this pull request Jul 8, 2026
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

P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS 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

@lin-hongkuan @vincentkoc