fix(release): stop Windows package descendants after timeout#111956
Conversation
openclaw-barnacle
Bot
added
scripts
125a76d to
7f7b2f6
Compare
Current-main refresh and type-contract correctionExact head:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
|
Codex review: needs maintainer review before merge. Reviewed July 25, 2026, 5:03 AM ET / 09:03 UTC. ClawSweeper reviewWhat this changesThe branch resolves Windows npm and pnpm package commands through the shared runners, uses shared process-tree cleanup after package-step timeouts, and adds Windows coverage for the real command-shim descendant path. Merge readinessKeep this PR open for maintainer merge review. It is the active candidate fix for #111948, has maintainer engagement and current-head CI in progress, and its implementation reuses the repository’s existing Windows command-resolution and managed-child termination boundaries rather than adding a parallel process implementation. No concrete patch defect was found from the supplied exact-head diff and evidence. Likely related people: steipete (high confidence, recent area changes and assignee) and CassieMei (medium confidence, earlier implementation commits on this repair). Priority: P2 Review scores
Verification
How this fits togetherThe Docker package helper launches npm or pnpm while assembling OpenClaw’s package artifacts for release and CI. It must resolve the platform-specific command correctly, capture command output, and stop every managed descendant before returning a timeout failure. flowchart LR
A[Release or CI package step] --> B[Docker package helper]
B --> C[Windows npm or pnpm runner]
C --> D[Spawned command shim]
D --> E[Node descendant process]
E --> F{Package step times out?}
F -->|Yes| G[Managed process-tree termination]
F -->|No| H[Package output and completion]
G --> I[Timeout failure returned to CI]
Before merge
Agent review detailsSecurityNone. PR surfaceTests +104, Config 0, Other +5. Total +109 across 5 files. View PR surface stats
Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the narrow shared-runner and managed-child-helper fix after the running exact-head Windows CI check passes, retaining the real pnpm.cmd-to-Node timeout regression as the long-term contract. Do we have a high-confidence way to reproduce the issue? Yes, from source and supplied proof: on Windows, run the Docker package helper against a pnpm.cmd shim that starts a long-lived Node child, impose a short timeout, and verify the recorded descendant PID exits. The PR’s E2E coverage encodes that path, although this read-only review did not execute it. Is this the best way to solve the issue? Yes. Reusing the existing npm/pnpm runners and managed-child termination helper keeps command resolution and timeout cleanup at their established ownership boundaries, avoiding a separate Windows process-management implementation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against aba0b12014a3. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
7f7b2f6 to
c2770d5
Compare
c2770d5 to
04813a9
Compare
Exact-head publication update
The PR body now contains the bounded redacted agent transcript and full evidence map. No manual ClawSweeper re-review command was issued. |
04813a9 to
070527d
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
d4e05a6 to
d95611f
Compare
|
Maintainer rewrite and exact-head proof are complete for The defect is present on current main at the package helper's timeout boundary: Windows disables process groups, falls back to direct Proof:
ClawSweeper's current rank-up move and both merge-risk gates are satisfied by the exact-head Windows and aggregate CI results. Contributor authorship remains preserved in the original commits. |
|
Merged via squash.
|
3 participants
Closes #111948
What Problem This Solves
Windows release and CI workflows using the Docker package helper could resolve npm/pnpm command shims inconsistently or leave a descendant process alive after a timed-out package step.
Why This Change Was Made
The package helper uses OpenClaw's existing cross-platform npm/pnpm runners and canonical managed-child termination helper. This keeps command resolution and timeout cleanup at the existing ownership boundary without adding dependencies or another Windows process implementation.
The change is limited to the package helper and its E2E coverage. It does not alter package contents, release policy, timeout values, or the managed-child helper itself.
User Impact
Windows packaging honors its timeout contract across real
.cmdprocess trees. Developers and CI jobs should not retain apnpm.cmd -> nodedescendant after the package command times out.Evidence
mainatb9a28cde42872cfe13051ddd8f1667e34bf59b17.070527ddfe32f0156a6258a638e226f731458ecd.+130/-14; no dependency or lockfile changes.24.18.0, pnpm11.2.2.terminateManagedChildimports as a function.8 passed,1platform skip.30 passed,1platform skip.pnpm.cmd -> nodeprocess tree, records the descendant PID, triggers timeout, and proves that PID is absent afterward.taskkill /PID <pid> /T, forced/T /F, and child-only fallback only aftertaskkillfailure.tsgo:scriptsandtsgo:test:root: passed.241contracts matched.120stimeout followed byEPERM: the restricted process boundary prevented the real Windowstaskkill /Tcleanup from completing. Identical relevant source across the prior passing head and rebased head, followed by the passing real.cmd -> nodeE2E, classified this as proof-lane tooling rather than a product regression.patch is correct, confidence0.96.Proof level:
test_provenin a fresh exact-head repository clone. Public CI on the published head remains the merge gate; this does not claim package publication or operator-runtime proof.AI-assisted implementation and review. Maintainer edits are welcome.
Security Impact
No secret, permission, download, dependency, lockfile, workflow, or package-publishing surface is added. The change narrows timeout cleanup to existing shared process-management helpers.
Compatibility And Migration
No migration is required. npm/pnpm resolution remains platform-aware through existing repository helpers; timeout values and package output remain unchanged.
Review Conversations
terminateManagedChildis explicitly exported fromscripts/lib/managed-child-process.mjs, imported by the package helper, and imported successfully in a fresh-clone smoke test.child.kill(signal); it usestaskkill /PID <pid> /T, escalates with/F, and falls back tochild.killonly aftertaskkillfailure. Unit and real.cmd -> nodeE2E coverage prove the path.Agent Transcript
Bounded redacted operator/agent timeline
pnpm.cmd -> nodedescendant alive. Earlier implementation review also found resolver-provided environment changes needed to be preserved by the package helper.EPERMwas reproduced and isolated to the restricted process boundary, then corrected in a clean host-owned proof lane.taskkill /Twith/Fescalation before child-only fallback. Final frozen-head Auto-review returned no accepted/actionable findings on070527ddfe32f0156a6258a638e226f731458ecd.test_proven; public CI and maintainer review remain outstanding after publication. No package was published and no operator runtime was mutated.Generated from local session history with
openclaw-agent-transcript, then bounded and redacted. No secrets, credentials, raw logs, private prompts, local paths, or operator data are included.