Skip to content

fix(media-core): normalize GIF content type detection#96435

Merged
vincentkoc merged 3 commits into
openclaw:mainfrom
lin-hongkuan:codex/media-core-normalized-gif-content-type
Jun 24, 2026
Merged

fix(media-core): normalize GIF content type detection#96435
vincentkoc merged 3 commits into
openclaw:mainfrom
lin-hongkuan:codex/media-core-normalized-gif-content-type

Conversation

@lin-hongkuan

@lin-hongkuan lin-hongkuan commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

isGifMedia() only lowercased contentType before comparing it with image/gif. Real MIME headers can include parameters and surrounding whitespace, so values such as image/gif; charset=binary or IMAGE/GIF were not recognized as GIF media.

Other MIME helpers in this module already normalize content types by trimming, lowercasing, and removing parameters.

Why This Change Was Made

The GIF detector now uses the existing normalizeMimeType() helper instead of open-coding a narrower comparison. This keeps GIF detection aligned with the rest of media-core MIME handling while preserving the existing filename fallback.

A focused regression test covers parameterized content types, whitespace/case normalization, non-GIF content types, and .gif filename fallback.

User Impact

GIF attachments with parameterized or padded Content-Type metadata are classified correctly instead of being treated as non-GIF media. Existing .gif filename detection still works.

Evidence

Direct behavior probe:

$ node --import tsx -e "import('./packages/media-core/src/mime.ts').then(({ isGifMedia }) => console.log(JSON.stringify([{ contentType: 'image/gif; charset=binary' }, { contentType: ' IMAGE/GIF ' }, { contentType: 'image/png' }, { fileName: 'animation.GIF' }].map((input) => ({ input, isGif: isGifMedia(input) })))))"
[{"input":{"contentType":"image/gif; charset=binary"},"isGif":true},{"input":{"contentType":" IMAGE/GIF "},"isGif":true},{"input":{"contentType":"image/png"},"isGif":false},{"input":{"fileName":"animation.GIF"},"isGif":true}]

Targeted test:

$ node scripts/run-vitest.mjs run packages/media-core/src/mime.test.ts

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


 Test Files  1 passed (1)
      Tests  95 passed (95)
   Start at  21:08:54
   Duration  2.88s (transform 87ms, setup 0ms, import 1.28s, tests 650ms, environment 0ms)

[test] starting test/vitest/vitest.unit-fast.config.ts
[test] passed 1 Vitest shard in 8.88s

Formatting:

$ node_modules\.bin\oxfmt.cmd --check packages/media-core/src/mime.ts packages/media-core/src/mime.test.ts
Checking formatting...

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

Whitespace:

$ git diff --check

AI-assisted

Prepared with Codex. I reviewed the change, understand the touched code path, and kept the PR focused on the bug described above.

@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 24, 2026, 1:03 PM ET / 17:03 UTC.

Summary
The PR changes isGifMedia to use shared MIME normalization and adds regression coverage, but the current branch also carries unrelated workflow, docs, plugin, gateway/session, state schema, Telegram, TUI, and UI changes.

Reproducibility: yes. from source inspection: current main compares opts.contentType?.toLowerCase() directly to image/gif, so image/gif; charset=binary and padded IMAGE/GIF values miss without a .gif filename fallback.

Review metrics: 4 noteworthy metrics.

  • Unrelated second-commit surface: 143 files, +3680/-612. This separates the original two-file fix from the later cross-repo commit that makes the current head unreviewable as a media-core change.
  • Config/default surfaces: 1 response usage default surface added. messages.responseUsage changes user-visible session preference behavior and needs compatibility review outside a GIF MIME PR.
  • Persistent state schema: 1 cron_jobs column added. The current head changes stored SQLite schema, which requires upgrade and migration review that the PR body does not discuss.
  • Automation/setup surfaces: 1 workflow plus 3 Docker/setup/script files changed. CI and sandbox setup changes are supply-chain-sensitive and are not covered by the media-core proof.

Stored data model
Persistent data-model change detected: migration/backfill/repair: src/flows/doctor-core-checks.test.ts, migration/backfill/repair: src/flows/doctor-core-checks.ts, migration/backfill/repair: src/flows/doctor-health-contributions.ts, migration/backfill/repair: src/flows/doctor-lint-flow.test.ts, migration/backfill/repair: src/flows/doctor-lint-flow.ts, persistent cache schema: src/plugins/tools.optional.test.ts, and 17 more. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until stronger real behavior proof is added.

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

Rank-up moves:

  • Drop commit 41e2d19 or rebuild the branch so only the focused media-core files remain.
  • Re-run the focused media-core test and whitespace check on the narrowed head.
  • Open separate PRs for any workflow, config, state schema, Telegram, provider, gateway, TUI, or UI changes that maintainers still want.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR body includes terminal proof for the original media-core behavior, but that proof does not cover the current head's broad workflow, config, state, provider, Telegram, gateway, TUI, and UI changes; screenshots, terminal output, logs, recordings, or linked artifacts should be redacted for private data before posting, and updating the PR body should trigger a fresh ClawSweeper review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Merging the current head would intentionally include a second commit with 143 unrelated changed files across workflows, scripts, generated docs hashes, providers, Telegram behavior, session/config handling, persisted state schema, TUI, and UI under a media-core PR title.
  • [P1] The current head adds or changes compatibility-sensitive surfaces, including messages.responseUsage config/default behavior and a cron_jobs state column, without maintainer-visible upgrade proof scoped to those changes.
  • [P1] The proof in the PR body demonstrates the original media-core fix, but it does not prove the later workflow, Docker/script, state-schema, gateway/session, Telegram, provider, or UI changes now present at the current head.

Maintainer options:

  1. Restore the focused media-core branch (recommended)
    Drop the broad second commit and keep only the packages/media-core/src/mime.ts and packages/media-core/src/mime.test.ts changes before review resumes.
  2. Adopt it as a broad maintainer PR
    Maintainers could intentionally keep the second commit, but then the PR needs a new title/body, owner review, upgrade proof, and validation for every config, state, workflow, plugin, Telegram, gateway, and UI surface it changes.
  3. Close if it cannot be narrowed
    If the branch cannot be restored to the focused fix, close this PR and ask for a new narrow media-core PR so the useful bug fix is not tied to unrelated churn.
Copy recommended automerge instruction 
@clawsweeper automerge

Special instructions:
Reset or rebuild this PR branch so it preserves the focused GIF MIME normalization changes from 3524e1d3f5b013f8016df053982d386b90599bb4 and removes the broad unrelated commit 41e2d195f8c7f04a4c1191b539f8cbdf3ec541a0; then rerun the media-core test and whitespace check.

Next step before merge

  • [P1] The repair is concrete and mechanical: preserve the useful first commit and remove the unrelated second commit so maintainers can review the narrow media-core fix.

Security
Needs attention: The original media-core change is not security-sensitive, but the current head now carries unrelated workflow, Docker, and sandbox setup changes that are supply-chain-sensitive and not covered by the PR purpose or proof.

Review findings

  • [P1] Drop the unrelated second commit — .github/workflows/sandbox-common-smoke.yml:60-66
Review details

Best possible solution:

Preserve the two-file media-core fix, drop the unrelated second commit, rebase on current main, and review the unrelated changes only as separate purpose-built PRs if maintainers want them.

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

Yes, from source inspection: current main compares opts.contentType?.toLowerCase() directly to image/gif, so image/gif; charset=binary and padded IMAGE/GIF values miss without a .gif filename fallback.

Is this the best way to solve the issue?

No for the current head: the first commit is the right narrow fix, but the later unrelated cross-repo commit makes this branch the wrong merge vehicle until it is dropped or split.

Full review comments:

  • [P1] Drop the unrelated second commit — .github/workflows/sandbox-common-smoke.yml:60-66
    The current head is no longer the focused two-file media-core fix that was reviewed earlier. Commit 41e2d195f8c7f04a4c1191b539f8cbdf3ec541a0 adds 143 unrelated files across workflows, docs, plugins, gateway/session handling, persisted state, Telegram, TUI, and UI, so this PR cannot be reviewed or merged as a GIF content-type fix until that commit is dropped or split into separate PRs.
    Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add merge-risk: 🚨 compatibility: The current head changes config/default behavior, session preference handling, and persisted state schema in addition to the media-core fix.
  • add merge-risk: 🚨 automation: The current head changes a GitHub Actions workflow plus Docker and sandbox setup scripts unrelated to the stated GIF detection fix.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body includes terminal proof for the original media-core behavior, but that proof does not cover the current head's broad workflow, config, state, provider, Telegram, gateway, TUI, and UI changes; screenshots, terminal output, logs, recordings, or linked artifacts should be redacted for private data before posting, and updating the PR body should trigger a fresh ClawSweeper review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove proof: sufficient: Current real behavior proof status is insufficient, not sufficient.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🧂 unranked krab, so this older rating label is no longer current.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: 📣 needs proof.

Label justifications:

  • P2: The underlying GIF classification fix is a normal-priority user-visible media bug, but the current PR is blocked by broad unrelated branch churn rather than an emergency.
  • merge-risk: 🚨 compatibility: The current head changes config/default behavior, session preference handling, and persisted state schema in addition to the media-core fix.
  • merge-risk: 🚨 automation: The current head changes a GitHub Actions workflow plus Docker and sandbox setup scripts unrelated to the stated GIF detection fix.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body includes terminal proof for the original media-core behavior, but that proof does not cover the current head's broad workflow, config, state, provider, Telegram, gateway, TUI, and UI changes; screenshots, terminal output, logs, recordings, or linked artifacts should be redacted for private data before posting, and updating the PR body should trigger a fresh ClawSweeper review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The current head touches visible Telegram behavior such as rich table rendering, inline buttons, and progress truncation, so a Telegram-visible proof label is appropriate if those changes remain.
Evidence reviewed

Security concerns:

  • [medium] Unrelated automation changes ride along — .github/workflows/sandbox-common-smoke.yml:60
    The later commit changes the sandbox smoke workflow and setup path under a media-core title. Even if the changes are benign, workflow and setup-script changes need separate maintainer review and validation instead of riding along with a MIME helper fix.
    Confidence: 0.9

Acceptance criteria:

  • [P1] node scripts/run-vitest.mjs run packages/media-core/src/mime.test.ts.
  • [P1] git diff --check.

What I checked:

  • Repository policy read: Root and relevant scoped AGENTS.md files were read; the broad-diff, owner-boundary, proof, docs, plugin, gateway, TUI, UI, and Telegram review guidance applies to this PR. (AGENTS.md:24, 4ae0a5d958a0)
  • Original useful change was focused: The contributor's first commit changes only packages/media-core/src/mime.ts and packages/media-core/src/mime.test.ts, with +25/-1 for the GIF normalization bug fix. (packages/media-core/src/mime.ts:255, 3524e1d3f5b0)
  • Current head has unrelated broad churn: The later commit alone adds 143 files of unrelated changes across workflows, iOS docs/config, generated docs hashes, providers, Telegram, ACP, agents, cron, gateway, plugins, state schema, TUI, tests, and UI, with +3680/-612 under the same media-core title. (.github/workflows/sandbox-common-smoke.yml:60, 41e2d195f8c7)
  • Current-main comparison confirms the branch is not just stale-base noise: Comparing current main to the PR head still reports the branch as 2 commits ahead, 37 commits behind, and carrying the same 145 changed files, so the broad surface is branch-owned diff rather than only base drift. (41e2d195f8c7)
  • Current main reproduces the GIF bug by source inspection: Current main lowercases contentType directly before comparing to image/gif, so padded or parameterized MIME values do not match unless the filename fallback is .gif. (packages/media-core/src/mime.ts:255, 4ae0a5d958a0)
  • PR's narrow fix uses the existing helper: At the PR head, isGifMedia uses normalizeMimeType(opts.contentType) === "image/gif", which matches the existing helper contract for trimming, lowercasing, and parameter stripping. (packages/media-core/src/mime.ts:255, 41e2d195f8c7)

Likely related people:

  • steipete: Authored the media/ACP core extraction that moved MIME helpers into packages/media-core, and appears in later package history for the same files. (role: recent media-core contributor; confidence: high; commits: 77f1359612f6, 4df95d3c3fd6; files: packages/media-core/src/mime.ts, packages/media-core/src/mime.test.ts, src/media/mime.ts)
  • vincentkoc: Recent history includes MIME hardening work, and the current broad second commit on this PR was authored by this account. (role: recent media hardening contributor and branch updater; confidence: medium; commits: 306ca4d3ebd3, c070509b7f8f, 41e2d195f8c7; files: src/media/mime.ts, packages/media-core/src/mime.ts, .github/workflows/sandbox-common-smoke.yml)
  • Clever: Recent path history for packages/media-core/src/mime.ts includes validated text document media send work, making this account an adjacent owner for media-core behavior. (role: adjacent media-core contributor; confidence: medium; commits: 1e14f4400f10; files: packages/media-core/src/mime.ts, packages/media-core/src/mime.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.

@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. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. P2 Normal backlog priority with limited blast radius. labels Jun 24, 2026
@clawsweeper
clawsweeper Bot temporarily deployed to qa-live-shared June 24, 2026 13:23 Inactive
@lin-hongkuan
lin-hongkuan force-pushed the codex/media-core-normalized-gif-content-type branch from 55f9724 to 3524e1d Compare June 24, 2026 13:24
@vincentkoc
vincentkoc requested a review from a team as a code owner June 24, 2026 16:57
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation channel: msteams Channel integration: msteams channel: telegram Channel integration: telegram app: ios App: ios app: web-ui App: web-ui gateway Gateway runtime extensions: memory-core Extension: memory-core scripts Repository scripts commands Command implementations docker Docker and sandbox tooling agents Agent runtime and tooling extensions: duckduckgo extensions: qa-lab extensions: ollama extensions: parallel size: XL and removed size: XS labels Jun 24, 2026
@clawsweeper clawsweeper Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 24, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. 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. labels Jun 24, 2026
@vincentkoc
vincentkoc merged commit 47d3d1b into openclaw:main Jun 24, 2026
43 checks passed
@vincentkoc

vincentkoc commented Jun 24, 2026

Copy link
Copy Markdown
Member

Merged via squash.

Thanks @lin-hongkuan!

@github-actions github-actions Bot mentioned this pull request Jun 24, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 25, 2026
@lin-hongkuan @vincentkoc
fix(media-core): normalize GIF content type detection (openclaw#96435)
ecb9f07 
Merged via squash.

Prepared head SHA: 82b1396
Co-authored-by: lin-hongkuan <[email protected]>
Co-authored-by: vincentkoc <[email protected]>
Reviewed-by: @vincentkoc
QiuYuang pushed a commit to QiuYuang/openclaw that referenced this pull request Jul 1, 2026
@lin-hongkuan @vincentkoc @QiuYuang
fix(media-core): normalize GIF content type detection (openclaw#96435)
316d0ce 
Merged via squash.

Prepared head SHA: 82b1396
Co-authored-by: lin-hongkuan <[email protected]>
Co-authored-by: vincentkoc <[email protected]>
Reviewed-by: @vincentkoc
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

agents Agent runtime and tooling app: ios App: ios app: web-ui App: web-ui channel: msteams Channel integration: msteams channel: telegram Channel integration: telegram commands Command implementations docker Docker and sandbox tooling docs Improvements or additions to documentation extensions: duckduckgo extensions: memory-core Extension: memory-core extensions: ollama extensions: parallel extensions: qa-lab gateway Gateway runtime mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. 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. scripts Repository scripts size: XL status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lin-hongkuan @vincentkoc