Skip to content

fix: document stream cleanup fallback intent#97701

Merged
vincentkoc merged 1 commit into
openclaw:mainfrom
ZengWen-DT:fix/97691-empty-catch-diagnostics
Jun 29, 2026
Merged

fix: document stream cleanup fallback intent#97701
vincentkoc merged 1 commit into
openclaw:mainfrom
ZengWen-DT:fix/97691-empty-catch-diagnostics

Conversation

@ZengWen-DT

@ZengWen-DT ZengWen-DT commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Closes #97691

What Problem This Solves

Fixes an issue where operators troubleshooting web response reads, proxy SSE streams, or MiniMax VLM host selection would find empty catch {} blocks with no indication whether cleanup and URL parse errors were intentionally ignored.

Why This Change Was Made

The catch sites now document the existing best-effort cleanup and fallback contracts in place, including the sibling provider error-body reader cleanup path that shares the same releaseLock() invariant. This preserves current stream handling and MiniMax malformed-host fallback behavior; it does not add logging, change provider routing, or turn cleanup failures into user-visible errors.

User Impact

Operators and maintainers can now tell why these errors are swallowed when reading the hot paths, while users keep the same runtime behavior for bounded web reads, proxy streams, provider error snippets, and MiniMax VLM endpoint fallback.

AI-assisted.

Evidence

Source precheck on current upstream/main showed the reported empty catches still present in src/agents/tools/web-shared.ts, src/agents/runtime/proxy.ts, and src/agents/minimax-vlm.ts; the sibling src/agents/provider-http-errors.ts cleanup catch used the same best-effort releaseLock() pattern. Existing MiniMax tests already cover malformed CN host fallback, so this patch keeps that behavior unchanged.

Focused tests:

node scripts/run-vitest.mjs src/agents/tools/web-shared.test.ts src/agents/runtime/proxy.test.ts src/agents/minimax-vlm.normalizes-api-key.test.ts src/agents/provider-http-errors.test.ts
[test] passed 1 Vitest shard in 4.55s
Test Files  4 passed (4)
Tests       45 passed (45)

Formatting and patch checks:

node_modules/.bin/oxfmt --check src/agents/tools/web-shared.ts src/agents/runtime/proxy.ts src/agents/minimax-vlm.ts src/agents/provider-http-errors.ts
All matched files use the correct format.

git diff --check
# no output

Build/CLI and live model proof:

pnpm openclaw --version
OpenClaw 2026.6.10 (b01e612)

pnpm openclaw agent --local --model deepseek/deepseek-v4-pro --session-key agent:default:issue-97691-proof --message "Reply exactly with: openclaw-proof-ok" --json --timeout 180
[provider-transport-fetch] response provider=deepseek api=openai-completions model=deepseek-v4-pro status=200
finalAssistantVisibleText: openclaw-proof-ok
stopReason: stop

Review:

.agents/skills/autoreview/scripts/autoreview --mode local
autoreview clean: no accepted/actionable findings reported
overall: patch is correct (0.92)

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS labels Jun 29, 2026
@clawsweeper

clawsweeper Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 29, 2026, 3:09 AM ET / 07:09 UTC.

Summary
The PR adds explanatory comments to four agent/runtime catch blocks covering web body reader cleanup, proxy stream cleanup, provider error-body cleanup, and MiniMax VLM host fallback without changing executable logic.

PR surface: Source +12. Total +12 across 4 files.

Reproducibility: yes. Current main has the exact unannotated catch sites from the linked source-repro issue, and the relevant tests show the cleanup and MiniMax fallback paths are real.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #97691
Summary: This PR is the candidate fix for the linked source-repro issue about unannotated stream/proxy/MiniMax catch blocks; the older empty-catch report is related context on other surfaces, not the canonical remaining work.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

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

Next step before merge

  • No ClawSweeper repair lane is needed because there are no actionable findings; the remaining action is normal maintainer review or merge.

Security
Cleared: The diff only adds comments inside existing catch blocks and does not touch dependencies, workflows, secrets, network targets, or executable code.

Review details

Best possible solution:

Land the comment-only annotations with existing focused validation so the linked issue closes without changing cleanup or MiniMax fallback semantics.

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

Yes. Current main has the exact unannotated catch sites from the linked source-repro issue, and the relevant tests show the cleanup and MiniMax fallback paths are real.

Is this the best way to solve the issue?

Yes. Commenting the intentional best-effort and fallback catches is the narrowest maintainable fix because the linked issue allowed comments and current tests make MiniMax fallback behavior intentional.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5327340a09ed.

Label changes

Label changes:

  • add P3: This is a low-risk code-clarity cleanup for troubleshooting intent with no runtime behavior, config, or compatibility change.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-patch terminal evidence for focused tests plus a live local agent command; for this comment-only runtime patch, that sufficiently shows the changed checkout was exercised without behavior drift.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-patch terminal evidence for focused tests plus a live local agent command; for this comment-only runtime patch, that sufficiently shows the changed checkout was exercised without behavior drift.

Label justifications:

  • P3: This is a low-risk code-clarity cleanup for troubleshooting intent with no runtime behavior, config, or compatibility change.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-patch terminal evidence for focused tests plus a live local agent command; for this comment-only runtime patch, that sufficiently shows the changed checkout was exercised without behavior drift.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-patch terminal evidence for focused tests plus a live local agent command; for this comment-only runtime patch, that sufficiently shows the changed checkout was exercised without behavior drift.
Evidence reviewed

PR surface:

Source +12. Total +12 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 4 16 4 +12
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 16 4 +12

What I checked:

  • Patch scope is comment-only: The PR diff replaces four empty catch bodies with explanatory comments and does not add logging, routing, config, dependency, or executable control-flow changes. (src/agents/tools/web-shared.ts:289, 259eda62af06)
  • Current main source reproduces the target pattern: Current main still has the unannotated releaseLock and MiniMax URL-parse catch sites that the linked issue reports, so this PR is addressing a real current source cleanup. (src/agents/minimax-vlm.ts:57, 5327340a09ed)
  • MiniMax fallback is already tested behavior: The MiniMax tests expect malformed CN hosts to fall back to the CN VLM host, supporting the PR's choice to document rather than change fallback semantics. (src/agents/minimax-vlm.normalizes-api-key.test.ts:109, 5327340a09ed)
  • Reader cleanup invariant has adjacent coverage: Existing web, proxy, and provider error tests cover releasing body readers after bounded reads and terminal streams, which matches the PR's documented cleanup invariant. (src/agents/runtime/proxy.test.ts:510, 5327340a09ed)
  • Repository comment policy supports this shape: Root policy requires short comments for non-obvious fallback behavior and cleanup/lifecycle invariants; the added two-line comments fit that guidance. (AGENTS.md:104, 5327340a09ed)
  • Live PR proof and checks are positive: The PR body includes focused test output plus a live local agent command returning the expected response, and live GitHub checks show Real behavior proof, lint, prod types, build artifacts, and QA Smoke CI passing for the PR. (259eda62af06)

Likely related people:

  • vincentkoc: Recent commits introduced or maintained the releaseLock cleanup behavior in the web, proxy, and provider error-body paths and touched MiniMax provider cleanup nearby. (role: recent area contributor; confidence: high; commits: 0dbac0d5f962, 3c01716c828c, e802fb8a9fbb; files: src/agents/tools/web-shared.ts, src/agents/runtime/proxy.ts, src/agents/provider-http-errors.ts)
  • zhangguiping-xydt: Recent proxy stream commits changed failure handling and bounded parsing in the same streamProxy surface adjacent to the cleanup catch. (role: recent adjacent contributor; confidence: medium; commits: cc757839d5e2, 27c1685f106d; files: src/agents/runtime/proxy.ts)
  • steipete: History shows MiniMax VLM routing and web helper maintenance in the affected MiniMax and web-tool surfaces. (role: feature-history owner; confidence: medium; commits: 5d1f7bf05894, b0319130311f, fbee4d56c4ca; files: src/agents/minimax-vlm.ts, src/agents/tools/web-shared.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: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels Jun 29, 2026
@vincentkoc
vincentkoc merged commit ac5af25 into openclaw:main Jun 29, 2026
138 of 146 checks passed
@vincentkoc vincentkoc mentioned this pull request Jun 29, 2026
Closed
7 tasks
This was referenced Jun 29, 2026
Closed
Closed
@lg320531124 lg320531124 mentioned this pull request Jun 29, 2026
Closed
@zw-xysk zw-xysk mentioned this pull request Jun 29, 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

agents Agent runtime and tooling P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor 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.

[Bug]: Empty catch blocks in streaming/proxy paths swallow I/O and URL errors without diagnostic trace

2 participants

@ZengWen-DT @vincentkoc