Skip to content

fix: split image setup and request timeout semantics#85415

Closed
hxy91819 wants to merge 4 commits into
openclaw:mainfrom
hxy91819:fix/image-timeout-pr1
Closed

fix: split image setup and request timeout semantics#85415
hxy91819 wants to merge 4 commits into
openclaw:mainfrom
hxy91819:fix/image-timeout-pr1

Conversation

@hxy91819

@hxy91819 hxy91819 commented May 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • split image-description timeout handling so setup and provider request both use the configured tools.media.image.timeoutSeconds budget instead of shrinking the request budget by elapsed setup time
  • preserve the full configured timeout for the provider request instead of silently subtracting setup latency down to a tiny remaining budget
  • add focused tests covering request timeout messaging, setup timeout messaging, and slow-setup request behavior
  • keep public docs user-facing: the image timeout still applies to the request itself and is not reduced by earlier preparation work
  • keep the heavier image-tool latency and runtime-loading reductions for follow-up PRs after this timeout-semantics fix

Verification

  • git diff --check
  • env CI=1 NODE_OPTIONS=--max-old-space-size=4096 OPENCLAW_VITEST_MAX_WORKERS=1 OPENCLAW_VITEST_NO_OUTPUT_TIMEOUT_MS=900000 node scripts/crabbox-wrapper.mjs run --provider docker --docker-image node:22-bookworm --timing-json --shell -- "bash -lc 'corepack enable >/dev/null 2>&1 && env CI=1 NODE_OPTIONS=--max-old-space-size=4096 OPENCLAW_VITEST_MAX_WORKERS=1 OPENCLAW_VITEST_NO_OUTPUT_TIMEOUT_MS=900000 corepack pnpm test src/media-understanding/image.test.ts'"

Real behavior proof

Behavior addressed: image timeout errors no longer collapse setup latency into a tiny remaining provider timeout; setup and request failures now report distinct phases.
Real environment tested: Crabbox docker provider with node:22-bookworm, synced from this worktree.
Exact steps or command run after this patch: run the targeted media-understanding Vitest file via Crabbox Docker using the command above.
Evidence after fix: src/media-understanding/image.test.ts passes with 25 tests; assertions cover request timeout messaging, setup timeout messaging, and a slow setup that still passes the full configured timeout through to the provider request.
Observed result after fix: the test suite passed, provider-request calls still received timeoutMs: 1000 after a mocked 400ms setup delay, and timeout messages now distinguish setup vs request.
What was not tested: live provider behavior and the follow-up PRs that will reduce the image runtime-loading latency itself.

@openclaw-barnacle openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels May 22, 2026
@clawsweeper

clawsweeper Bot commented May 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 11, 2026, 5:01 AM ET / 09:01 UTC.

Summary
Review failed before ClawSweeper could summarize the requested change.

PR surface: Source +39, Tests +65, Docs +4. Total +108 across 5 files.

Reproducibility: unclear. The review failed before ClawSweeper could establish a reproduction path.

Review metrics: none identified.

Merge readiness
Overall: 🌊 off-meta tidepool
Proof: 🌊 off-meta tidepool
Patch quality: 🌊 off-meta tidepool
Result: rating does not apply to this item.

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

Risk before merge

  • [P1] No close action taken because the review did not complete.

Maintainer options:

  1. Decide the mitigation before merge
    Retry the Codex review after fixing the execution failure.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • Review did not complete, so no work-lane recommendation was made.
Review details

Best possible solution:

Retry the Codex review after fixing the execution failure.

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

Unclear. The review failed before ClawSweeper could establish a reproduction path.

Is this the best way to solve the issue?

Unclear. Retry the review first so ClawSweeper can evaluate the actual issue and fix direction.

AGENTS.md: unclear because the file could not be read completely.

Codex review notes: model internal, reasoning high; reviewed against 0c4fc0a2e3b3.

Label changes

Label changes:

  • remove P2: Current review triage priority is none.
  • remove merge-risk: 🚨 compatibility: Current PR review selected no merge-risk labels.
  • remove merge-risk: 🚨 availability: Current PR review selected no merge-risk labels.

Label justifications:

  • rating: 🌊 off-meta tidepool: Overall readiness is 🌊 off-meta tidepool; proof is 🌊 off-meta tidepool and patch quality is 🌊 off-meta tidepool.
  • status: 🚀 automerge armed: This PR is in ClawSweeper's automerge lane. Not applicable: Real behavior proof was not assessed because the Codex review failed.
Evidence reviewed

PR surface:

Source +39, Tests +65, Docs +4. Total +108 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 1 45 6 +39
Tests 1 67 2 +65
Docs 3 8 4 +4
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 120 12 +108

What I checked:

  • failure reason: retryable codex transport failure.
  • codex failure detail: Codex review failed for this PR with exit 1.
  • codex stderr: ectness":"patch is incorrect","overallConfidenceScore":0.35,"fixedRelease":null,"fixedSha":null,"fixedAt":null,"closeComment":"","workCandidate":"manual_review","workConfidence":"medium","workPriority":"medium","workReason":"A maintainer must decide the permanent compatibility contract, and the contributor must add live provider proof before merge.","workPrompt":"","workClusterRefs":[],"workValidation":[],"workLikelyFiles":[]}.
  • codex stdout: No stdout captured.

Likely related people:

  • unknown: Codex failed before it could trace repository history. (role: review did not complete; confidence: low)
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: 🐚 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 May 22, 2026
@clawsweeper

clawsweeper Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

✨ Hatched: 🥚 common Clockwork Diff Drake

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🥚 common.
Trait: polishes edge cases.
Image traits: location branch lighthouse; accessory CI status badge; palette moss green and polished brass; mood celebratory; pose holding its accessory up for inspection; shell paper lantern shell; lighting calm overcast light; background subtle branch markers.
Share on X: post this hatch
Copy: My PR egg hatched a 🥚 common Clockwork Diff Drake in ClawSweeper.

What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@hxy91819

hxy91819 commented May 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

I checked the timeout contract here, and I think this PR is fixing an implementation bug rather than changing the config contract.

The reasoning is:

  1. The help text for tools.media.image.timeoutSeconds already says: Timeout in seconds for each image understanding request before it is aborted. The key phrase is each ... request, not a single shared budget for the whole image operation. See src/config/schema.help.ts:791.
  2. In the earlier timeout implementation, setup and request were already modeled as separate withImageDescriptionTimeout(...) boundaries. The later Math.max(1, timeoutMs - elapsed) subtraction looks much more like defensive handling for an already-expired request timeout than an intentional contract change to a whole-operation budget.
  3. The old behavior is exactly what made this confusing for users: docs and help text read like a request timeout, but in practice the provider request budget was silently reduced by setup latency, which could even surface as misleading 1ms timeout errors.

So the intended semantics of this patch are:

  • tools.media.image.timeoutSeconds still means the timeout for each image-understanding request or phase.
  • This PR brings the implementation back in line with that existing meaning.
  • We should not update the docs to describe a shared setup + request total budget, because that was the buggy behavior we are removing, not the contract we want to preserve.

To reduce future confusion, I am also tightening the wording in the docs so the image timeout is described more explicitly as a per-request or per-phase timeout rather than a whole-operation timeout.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation gateway Gateway runtime labels May 24, 2026
@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels May 24, 2026
@hxy91819
hxy91819 force-pushed the fix/image-timeout-pr1 branch from c379989 to 3f927f0 Compare May 24, 2026 02:29
@hxy91819

hxy91819 commented May 24, 2026

Copy link
Copy Markdown
Member Author

/clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@hxy91819

hxy91819 commented May 27, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper autofix

@clawsweeper

clawsweeper Bot commented May 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🔧
ClawSweeper autofix is enabled.

This is fix-only; I will not merge this PR.

@clawsweeper clawsweeper Bot added the clawsweeper:autofix Maintainer opted this PR into bounded ClawSweeper-reviewed autofix without merge label May 27, 2026
@clawsweeper

clawsweeper Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper 🐠 automerge status

No new branch changes from this pass. ClawSweeper left the branch untouched instead of making a noisy edit.

Executor outcome: Codex fix worker timed out after 1200000ms.
Worker summary: Make this PR merge-ready for ClawSweeper automerge. Rebase onto latest main, address PR comments and review findings, fix CI/check failures, preserve release-note context, and validate before returning.

Worker actions:

  • build_fix_artifact on this PR: planned - Maintainer opted this PR into ClawSweeper automerge/autofix repair; run the direct Codex edit loop after live hydration instead of a separate read-only planning pass.

No push, rebase, replacement PR, merge, or ClawSweeper re-review happened on this pass.

fish notes: model gpt-5.5, reasoning high.

Automerge progress:

@clawsweeper clawsweeper Bot added the clawsweeper:human-review Needs maintainer review before ClawSweeper can continue label May 27, 2026
@clawsweeper

clawsweeper Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

🦞✅
ClawSweeper is pausing this repair loop for human review.

Source: clawsweeper[bot]
Reason: No automated repair is needed; maintainers need to approve the per-phase timeout contract and merge or request an outer-cap adjustment.; Cleared: The diff is limited to media-understanding timeout control flow, focused tests, and docs; it does not change dependencies, CI, credentials, package resolution, or permissions. (sha=76443b2292b855aed042f19a8623cf219f530803)

Why human review is needed:
This item has security-sensitive risk. ClawSweeper is pausing instead of making an autonomous change that could affect trust, credentials, permissions, or exposure.

What the maintainer can do as a next step:
If the maintainer accepts the current risk and wants ClawSweeper to continue merge gates, comment @clawsweeper approve. If the security-sensitive detail still needs changes, describe the safe path or push the fix, then comment @clawsweeper automerge. If the risk should not be automated, keep the PR paused for manual review or comment @clawsweeper stop.

I added clawsweeper:human-review and left the final call with a maintainer.

@hxy91819
hxy91819 force-pushed the fix/image-timeout-pr1 branch from 76443b2 to 73a558c Compare June 11, 2026 03:47
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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 11, 2026
@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 11, 2026
@clawsweeper

clawsweeper Bot commented Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞✅
ClawSweeper is pausing this repair loop for human review.

Source: clawsweeper[bot]
Reason: - Review did not complete, so no work-lane recommendation was made. (sha=73a558c9d4897ed8987656257f9a5306baab2811)

Why human review is needed:
ClawSweeper found a blocker that should be resolved or accepted by a maintainer before the repair or automerge loop continues.

What the maintainer can do as a next step:
If the maintainer accepts the current risk and wants ClawSweeper to continue merge gates, comment @clawsweeper approve. If more work is needed, resolve the blocker first, then comment @clawsweeper automerge to re-review and continue. If automation should stay paused, leave clawsweeper:human-review in place or comment @clawsweeper stop.

I added clawsweeper:human-review and left the final call with a maintainer.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 11, 2026
@hxy91819

hxy91819 commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper automerge

@clawsweeper

clawsweeper Bot commented Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🔧
ClawSweeper automerge is enabled.

Draft PRs stay fix-only until GitHub marks them ready for review. Pause with /clawsweeper stop.

Automerge progress:

  • 2026-06-13 12:51:52 UTC review queued 73a558c9d489 (queued)

@clawsweeper clawsweeper Bot added clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane. clawsweeper:human-review Needs maintainer review before ClawSweeper can continue and removed clawsweeper:human-review Needs maintainer review before ClawSweeper can continue clawsweeper:autofix Maintainer opted this PR into bounded ClawSweeper-reviewed autofix without merge labels Jun 11, 2026
@hxy91819

hxy91819 commented Jun 13, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper approve

@clawsweeper clawsweeper Bot removed the clawsweeper:human-review Needs maintainer review before ClawSweeper can continue label Jun 13, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 13, 2026
Merged
@clawsweeper

clawsweeper Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper 🐠 reef update

Thanks for the contribution here. ClawSweeper tried the original lane first, but branch permissions blocked the push, so a replacement PR is carrying the fix forward.

Why replacement: ClawSweeper could not update the source PR branch directly; GitHub did not grant sufficient push rights to the bot for that branch.
Replacement PR: #92673
Why close: this run explicitly closes the superseded source PR after the credited replacement PR is open, so review continues in one place.
Closing this source PR because this run explicitly enabled source-PR closeout.
Attribution stays attached; the replacement just gives the fix a writable branch.
Co-author credit kept:

fish notes: reasoning high; reviewed against b84f60d.

@clawsweeper clawsweeper Bot closed this Jun 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge docs Improvements or additions to documentation gateway Gateway runtime maintainer Maintainer-authored PR merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. size: S status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hxy91819 @steipete