Skip to content

fix(agent): preserve media task success on delivery miss#88083

Merged
obviyus merged 2 commits into
mainfrom
codex/media-generation-delivery-completion
May 30, 2026
Merged

fix(agent): preserve media task success on delivery miss#88083
obviyus merged 2 commits into
mainfrom
codex/media-generation-delivery-completion

Conversation

@obviyus

@obviyus obviyus commented May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Keep successful media generation separate from requester completion delivery failures.
  • Recover known generated-media delivery misses with deterministic direct delivery instead of a second error wake.
  • Mark unrecovered delivery misses as blocked, and allow retry after blocked media delivery.

Proof

  • Distill pass: generation result, requester delivery, and retry blocking are separate states.
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main
  • Result: autoreview clean; no accepted/actionable findings.
  • git diff --check origin/main..HEAD
  • CI=1 OPENCLAW_VITEST_MAX_WORKERS=1 node --no-maglev ./node_modules/vitest/vitest.mjs run src/agents/tools/media-generate-background-shared.test.ts src/agents/image-generation-task-status.test.ts --reporter=dot --testTimeout=10000 --hookTimeout=10000
  • Result: 4 test files passed, 46 tests passed.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S maintainer Maintainer-authored PR labels May 29, 2026
@clawsweeper

clawsweeper Bot commented May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed May 29, 2026, 2:40 PM ET / 18:40 UTC.

Summary
The PR changes shared image, video, and music media-generation completion so successful generation can complete separately from requester delivery misses, with direct recovery and retry behavior for blocked delivery.

PR surface: Source +87, Tests +346. Total +433 across 4 files.

Reproducibility: yes. from source inspection: current main can be exercised with a media lifecycle whose wakeTaskCompletion returns false or throws, which currently turns successful generation into task failure. I did not run a live media-generation scenario in this read-only review.

Review metrics: 1 noteworthy metric.

  • Shared media completion callers: 3 callers affected. The helper is used by image, video, and music generation, so the delivery-miss behavior is shared rather than a single-tool tweak.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • [P1] Add redacted terminal/log/screenshot/video or linked-artifact proof from a real media-generation task showing the after-fix delivery-miss behavior.
  • Update the PR body after adding proof so ClawSweeper re-reviews automatically, or ask a maintainer to comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists autoreview, diff check, and targeted Vitest results, but no redacted real media-generation run or delivery-miss proof from a live setup. 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] The branch intentionally changes shared media delivery-miss semantics: successful generation can now complete as delivered or blocked instead of failing the task and sending the previous error wake.
  • [P1] The PR body has only autoreview, diff-check, and Vitest proof; external PR proof still needs a redacted real media-generation run showing the after-fix behavior.
  • [P1] The protected maintainer label means this should remain a human-handled PR even though no blocking source-level defect was found.

Maintainer options:

  1. Require real media delivery proof (recommended)
    Ask for redacted terminal, log, screenshot, recording, or linked-artifact proof from a real media-generation task showing successful generation and the delivery-miss behavior after this patch.
  2. Accept the delivery semantic change
    A maintainer can explicitly accept the new blocked-success semantics for unrecovered delivery misses after reviewing the focused regression tests and current source path.

Next step before merge

  • [P1] The remaining action is maintainer review plus contributor real behavior proof, not a narrow code defect for ClawSweeper to repair.

Security
Cleared: No concrete security or supply-chain concern was found; the diff changes local TypeScript runtime/tests and uses an existing internal delivery runtime.

Review details

Best possible solution:

Land the state split only after maintainer acceptance of the delivery-miss semantics and redacted real media-generation proof showing the requester-delivery path after the patch.

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

Yes from source inspection: current main can be exercised with a media lifecycle whose wakeTaskCompletion returns false or throws, which currently turns successful generation into task failure. I did not run a live media-generation scenario in this read-only review.

Is this the best way to solve the issue?

Likely yes: separating provider generation success from requester delivery failure is the narrow maintainable direction for this shared helper. The merge blocker is proof and maintainer acceptance of the delivery semantics, not a concrete code defect found in review.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 040eba1cdc6b.

Label changes

Label justifications:

  • P2: This is a bounded agent media-task delivery correctness fix without evidence of an emergency or broad runtime outage.
  • merge-risk: 🚨 message-delivery: The diff changes whether requesters receive direct media, blocked completion state, or the previous error wake when completion delivery is missed.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists autoreview, diff check, and targeted Vitest results, but no redacted real media-generation run or delivery-miss proof from a live setup. 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.
Evidence reviewed

PR surface:

Source +87, Tests +346. Total +433 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 2 122 35 +87
Tests 2 365 19 +346
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 487 54 +433

What I checked:

Likely related people:

  • Shakker: Git blame shows the current shared media-generation completion helper and delivery-miss failure branch coming from commit 40a9c38. (role: introduced current shared helper behavior; confidence: high; commits: 40a9c38736f8; files: src/agents/tools/media-generate-background-shared.ts, src/agents/media-generation-task-status-shared.ts)
  • Peter Steinberger: Recent history on subagent completion delivery includes timer and delivery-hardening work adjacent to the media completion path. (role: recent adjacent area contributor; confidence: medium; commits: bf3921dab762; files: src/agents/subagent-announce-delivery.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 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. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels May 29, 2026
@obviyus obviyus force-pushed the codex/media-generation-delivery-completion branch from c8ecabe to 9df7de0 Compare May 29, 2026 18:32
@obviyus obviyus self-assigned this May 30, 2026
@obviyus obviyus force-pushed the codex/media-generation-delivery-completion branch from 9df7de0 to 77a607b Compare May 30, 2026 03:37
@obviyus obviyus merged commit 1659b26 into main May 30, 2026
102 checks passed
@obviyus obviyus deleted the codex/media-generation-delivery-completion branch May 30, 2026 03:37
@obviyus

obviyus commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

Landed via rebase onto main.

  • Scoped tests: git diff --check origin/main..HEAD; node scripts/run-vitest.mjs src/agents/tools/media-generate-background-shared.test.ts src/agents/image-generation-task-status.test.ts --reporter=dot --testTimeout=10000 --hookTimeout=10000
  • Autoreview: .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main reported no accepted/actionable findings
  • Changelog: not updated; agent/media delivery bugfix only
  • Land commit: 77a607b
  • Merge commit: 1659b26

@steipete steipete mentioned this pull request May 30, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@obviyus obviyus

Labels

agents Agent runtime and tooling maintainer Maintainer-authored PR merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: L 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.

1 participant

@obviyus