refactor: consolidate core stream cleanup, watchdog locality, and approval text duplication by steipete · Pull Request #99901 · openclaw/openclaw

steipete · 2026-07-04T08:53:52Z

Related: #99841

What Problem This Solves

Second consolidation pass over the duplication clusters catalogued in #99841, this time centered on core src/:

src/agents/mcp-http-fetch.ts and src/agents/provider-transport-fetch.ts each hand-rolled the same delicate guarded-response stream wrapper (reader lifecycle, exactly-once finalize, FinalizationRegistry backstop for abandoned bodies) - a 29-line verbatim clone of leak-prevention code that must not drift.
src/agents/embedded-agent-runner/run/llm-idle-timeout.ts duplicated its 16-line model-endpoint locality classification between the idle and first-event watchdogs.
src/auto-reply/reply/commands-diagnostics.ts and commands-export-trajectory.ts duplicated the private-vs-origin exec-approval routing ternaries (the exact fallback cluster that must stay in sync across command handlers).
imessage and signal carried identical approval-hint insertion logic (insert...HintNearHeader + add-with-guard), a 30-line clone of the ID: header text surgery.
migrate-claude and migrate-hermes shipped byte-identical targets.ts files.
src/infra/backup-create.ts had a local sleep copy of src/utils/sleep.ts.

Why This Change Was Made

Same policy as #99850: one canonical owner per behavior.

New wrapGuardedBodyStream in src/infra/net/guarded-body-stream.ts (next to the fetchWithSsrFGuard contract both callers consume); the MCP and provider transports keep their caller-specific pieces (MCP global-fetch Response rewrap, provider local-service lease release) as small closures. A dedicated module keeps the transport test's explicit fetch-guard mock factory narrow while the real stream-cleanup behavior stays under test.
resolveRuntimeModelLocality now feeds both LLM watchdog resolvers; resolveCommandExecApprovalRoute lives in commands-private-route.ts (the existing owner of route reading helpers) and both command handlers spread it.
hasApprovalReactionHintText / insertApprovalReactionHintNearIdHeader / addApprovalReactionHintToText join the other reaction helpers in plugin-sdk/approval-reaction-runtime; imessage and signal shrink to wrappers/callers.
resolvePlannedMigrationTargets joins plugin-sdk/migration-runtime; both migration plugins re-export it. One deliberate alignment: a configured agentDir with a ~ prefix now resolves through the product's effective-home resolution (resolveHomeRelativePath) instead of a raw os.homedir() string replace, matching how the primary runtime resolver treats paths.
Surface budget pins updated per the exact-pin ratchet for the five new SDK exports (4 callable + 1 type).

User Impact

No user-visible behavior change. Internal hardening: stream-cleanup semantics are now provably identical across MCP and provider transports, and the watchdog locality rules cannot drift between the idle and first-event paths. Net negative production LOC.

Evidence

Focused tests, all green: mcp-http-fetch + provider-transport-fetch (89, including the abandoned-stream release/leak tests exercising the shared wrapper), llm-idle-timeout (100), commands-diagnostics + commands-export-trajectory (19), imessage + signal approval-reactions (45), migrate-claude + migrate-hermes provider tests (11), backup-create covered by its suite minus two failures that reproduce identically on unmodified main (late-appearing sqlite state tests; baseline noise, see PR discussion).
git diff origin/main --stat -- src/infra/net/fetch-guard.ts is empty: the guard contract file is untouched; the helper is a sibling module.
All four tsgo lanes, oxfmt/oxlint on changed files, plugin-sdk surface check with updated pins, and a fresh Codex autoreview run before merge (results in comments/checks).

…mers

…dogs

…mmand handlers

…pers

…n target helpers

clawsweeper · 2026-07-04T08:55:21Z

Codex review: needs real behavior proof before merge. Reviewed July 4, 2026, 5:00 AM ET / 09:00 UTC.

Summary
The PR consolidates duplicated helper logic for guarded fetch stream cleanup, LLM watchdog locality, command exec approval routing, approval reaction hints, migration target resolution, docs, and SDK surface budgets.

PR surface: Source -9, Docs 0, Other 0. Total -9 across 16 files.

Reproducibility: Do we have a high-confidence way to reproduce the issue? Not as a runtime bug, but yes as source-level cleanup: current main has duplicated stream cleanup and target/hint helper patterns that the PR centralizes.

Review metrics: 1 noteworthy metric.

Plugin SDK exports: 5 added (4 callable, 1 type). The PR expands public SDK surface and pins the export budget, so maintainers should notice the API commitment before merge.

Stored data model
Persistent data-model change detected: migration/backfill/repair: extensions/migrate-claude/targets.ts, migration/backfill/repair: extensions/migrate-hermes/targets.ts, migration/backfill/repair: src/plugin-sdk/migration-runtime.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
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:

[P2] Add or point to focused proof for configured agentDir: "~..." under OPENCLAW_HOME in the migration target helper.

Risk before merge

[P1] The PR adds five public Plugin SDK exports, so maintainers should explicitly accept the API commitment rather than treating this as purely private cleanup.
[P1] The shared migration target helper changes configured agentDir with a ~ prefix to use effective OpenClaw home, which appears aligned with runtime agent-dir resolution but should be covered or acknowledged as intentional upgrade behavior before merge.

Maintainer options:

Add focused migration-target proof (recommended)
Add or point to a focused test showing resolvePlannedMigrationTargets preserves runtime resolver precedence and intentionally resolves configured ~ agentDir through effective OpenClaw home.
Accept the API expansion
A maintainer can explicitly accept the five new public SDK exports and the target-path alignment as the desired permanent contract.
Pause if the SDK surface is too broad
If maintainers do not want these helpers public yet, keep the stream/watchdog cleanup and narrow or defer the SDK-facing migration and approval-hint exports.

Next step before merge

[P1] Protected maintainer label plus public SDK and migration target compatibility questions make maintainer review the right next action rather than cleanup close or automated repair.

Security
Cleared: The diff touches network stream cleanup and exec approval routing, but I found no new dependency, permission, secret-handling, workflow, or code-execution regression.

Review details

Best possible solution:

Land the consolidation only after maintainer confirmation that the new SDK exports and migration target ~ alignment are intended public behavior, with focused proof for that path rather than reintroducing per-plugin copies.

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

Do we have a high-confidence way to reproduce the issue? Not as a runtime bug, but yes as source-level cleanup: current main has duplicated stream cleanup and target/hint helper patterns that the PR centralizes.

Is this the best way to solve the issue?

Is this the best way to solve the issue? Mostly yes: the shared helper locations match the observed owner boundaries, but the best merge state needs explicit maintainer acceptance/proof for the public SDK exports and migration target path alignment.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

add P3: This is low-risk maintainability cleanup with compatibility-sensitive API surface, not a user-facing outage or urgent regression.
add merge-risk: 🚨 compatibility: The PR adds public Plugin SDK exports and changes migration target path resolution for configured ~ agentDir under effective OpenClaw home.
add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The PR carries the protected maintainer label for an internal refactor; the external contributor proof gate is not applied, though tests and CI remain relevant validation.

Label justifications:

P3: This is low-risk maintainability cleanup with compatibility-sensitive API surface, not a user-facing outage or urgent regression.
merge-risk: 🚨 compatibility: The PR adds public Plugin SDK exports and changes migration target path resolution for configured ~ agentDir under effective OpenClaw home.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The PR carries the protected maintainer label for an internal refactor; the external contributor proof gate is not applied, though tests and CI remain relevant validation.

Evidence reviewed

PR surface:

Source -9, Docs 0, Other 0. Total -9 across 16 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	14	262	271	-9
Tests	0	0	0	0
Docs	1	3	3	0
Config	0	0	0	0
Generated	0	0	0	0
Other	1	2	2	0
Total	16	267	276	-9

What I checked:

Repository policy read and applied: Root policy marks plugin APIs, migrations, provider routing, and fallback behavior as compatibility/upgrade-sensitive merge-risk surfaces. (AGENTS.md:31, eafe2a8d0bb6)
Scoped SDK policy read: The Plugin SDK guide requires public SDK subpath changes to keep docs, entrypoint lists, package exports, API baseline, and export checks aligned. (src/plugin-sdk/AGENTS.md:79, eafe2a8d0bb6)
Scoped extensions policy read: The extensions guide supports replacing duplicated bundled-plugin logic with a typed SDK subpath and moving bundled callers in the same change. (extensions/AGENTS.md:78, eafe2a8d0bb6)
Maintainer notes absent: The checkout has no .agents/maintainer-notes directory, so there were no scoped maintainer notes to apply.
Live PR state: Live GitHub metadata shows the PR is open, mergeable, and carries the protected maintainer label; review/cleanup should not close it automatically. (a32c6a547592)
Shared stream cleanup helper: The PR head adds wrapGuardedBodyStream with exactly-once cleanup, cancellation, timeout refresh, and FinalizationRegistry backstop behavior. (src/infra/net/guarded-body-stream.ts:20, a32c6a547592)

Likely related people:

steipete: Peter Steinberger authored the related merged SDK refactor on current main and appears in the touched-area history for provider/runtime cleanup. (role: recent SDK/refactor contributor; confidence: high; commits: eafe2a8d0bb6, 7740c4d53050; files: src/plugin-sdk/migration-runtime.ts, src/agents/provider-transport-fetch.ts, src/plugin-sdk/approval-reaction-runtime.ts)
Vincent Koc: Git history shows Vincent Koc as the dominant non-bot contributor over the central provider/MCP/approval runtime files sampled for this review. (role: feature-history contributor; confidence: medium; commits: e085fa1a3ffd; files: src/agents/provider-transport-fetch.ts, src/agents/mcp-http-fetch.ts, src/plugin-sdk/approval-reaction-runtime.ts)
Qasim Soomro: History on provider transport includes the private-network request policy work adjacent to the guarded fetch cleanup path. (role: adjacent network request contributor; confidence: low; commits: 71617ef2f056; files: src/agents/provider-transport-fetch.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.

steipete · 2026-07-04T09:08:39Z

Merged via squash.

Prepared head SHA: a32c6a547592c7662d72634579c7375e3544b542
Landed commit: be95bb72d4598e2e1728c9814b1c16fa60af05bd

…roval text duplication (openclaw#99901) * refactor(infra): share guarded body-stream cleanup across fetch consumers * refactor(agents): extract shared runtime model locality for llm watchdogs * refactor(auto-reply): share exec approval route resolution between command handlers * refactor(plugin-sdk): share approval reaction hint text helpers * refactor(plugin-sdk): share planned migration target resolution * refactor(infra): use canonical sleep helper in backup retry * docs(plugins): mention shared migration targets and reaction hint helpers * chore(plugin-sdk): pin surface budgets for reaction hint and migration target helpers

steipete added 8 commits July 4, 2026 09:51

refactor(infra): share guarded body-stream cleanup across fetch consu…

7308778

…mers

refactor(agents): extract shared runtime model locality for llm watch…

d8cad17

…dogs

refactor(auto-reply): share exec approval route resolution between co…

2a393d9

…mmand handlers

refactor(plugin-sdk): share approval reaction hint text helpers

ec6b551

refactor(plugin-sdk): share planned migration target resolution

b430918

refactor(infra): use canonical sleep helper in backup retry

7065279

docs(plugins): mention shared migration targets and reaction hint hel…

9686b07

…pers

chore(plugin-sdk): pin surface budgets for reaction hint and migratio…

a32c6a5

…n target helpers

steipete merged commit be95bb7 into main Jul 4, 2026
186 of 195 checks passed

steipete deleted the claude/dedup-round-2 branch July 4, 2026 09:08

github-actions Bot mentioned this pull request Jul 4, 2026

📡 Upstream Digest — 2026-07-04 10:01 UTC curtismercier/openclaw-mods#1203

Open

steipete mentioned this pull request Jul 4, 2026

refactor: consolidate markdown code fences, error coercion, and byte-identical helper pairs #99932

Merged

This was referenced Jul 4, 2026

Feature: Add per-model generation timeout config #8724

Open

Add rate limiting and throttling for external API calls #13615

Open

steipete mentioned this pull request Jul 19, 2026

fix(streams): release guarded response reader locks #111259

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

refactor: consolidate core stream cleanup, watchdog locality, and approval text duplication#99901

refactor: consolidate core stream cleanup, watchdog locality, and approval text duplication#99901
steipete merged 8 commits into
mainfrom
claude/dedup-round-2

steipete commented Jul 4, 2026

Uh oh!

clawsweeper Bot commented Jul 4, 2026 •

edited

Loading

Uh oh!

Uh oh!

steipete commented Jul 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

steipete commented Jul 4, 2026

What Problem This Solves

Why This Change Was Made

User Impact

Evidence

Uh oh!

clawsweeper Bot commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

steipete commented Jul 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

clawsweeper Bot commented Jul 4, 2026 •

edited

Loading