Skip to content

fix(install-cli): track mktemp paths and clean up on EXIT#103725

Merged
steipete merged 1 commit into
openclaw:mainfrom
SebTardif:fix/install-cli-tmpfile-cleanup
Jul 11, 2026
Merged

fix(install-cli): track mktemp paths and clean up on EXIT#103725
steipete merged 1 commit into
openclaw:mainfrom
SebTardif:fix/install-cli-tmpfile-cleanup

Conversation

@SebTardif

@SebTardif SebTardif commented Jul 10, 2026 •
edited by steipete
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

scripts/install-cli.sh creates temporary paths for user-space Node staging and pnpm workspace rewrites. Failure after mktemp could exit without removing those paths, leaving installer debris under the system temporary directory.

Why This Change Was Made

The installer now owns one EXIT cleanup list. Each direct caller registers its temporary path in the parent shell immediately after creation, avoiding command-substitution subshell state and giving every later failure path the same cleanup behavior.

User Impact

Failed CLI installs no longer leak Node staging directories or pnpm workspace rewrite files. Successful installs retain their existing behavior.

Evidence

No live Node tarball checksum failure was induced; the regressions cover both temporary-path owners at their production failure boundaries.

CI attempt 1 hit an unrelated apply-patch.test.ts fs-safe unhandled-rejection flake; the isolated rerun passed without code changes.

AI-assisted.

@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 10, 2026
@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. labels Jul 10, 2026
@clawsweeper

clawsweeper Bot commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 10, 2026, 8:34 PM ET / July 11, 2026, 00:34 UTC.

Summary
The branch adds caller-shell tracking and EXIT cleanup for the CLI installer’s Node staging directory and pnpm workspace rewrite file, with failure-path regression tests.

PR surface: Tests +61, Other +14. Total +75 across 2 files.

Reproducibility: yes. Current main visibly allocates both temporary paths before fallible operations without EXIT cleanup, and the contributor’s terminal proof exercises the production Node staging path and observes the registered directory removed after forced failure.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: serialized state: test/scripts/install-cli.test.ts. Confirm migration or upgrade compatibility proof before merge.

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.

Risk before merge

  • [P1] Two exact-head CI aggregation jobs were still running at review time; branch protection should require their completion before merge.

Maintainer options:

  1. Decide the mitigation before merge
    Merge the centralized parent-shell cleanup and focused regression tests once the required exact-head checks complete successfully.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No automated repair is needed; the remaining action is ordinary maintainer merge handling after required checks finish.

Security
Cleared: The patch adds no dependency, permission, credential, package-resolution, download-source, or remote code-execution surface, and deletion is limited to installer-owned mktemp results.

Review details

Best possible solution:

Merge the centralized parent-shell cleanup and focused regression tests once the required exact-head checks complete successfully.

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

Yes. Current main visibly allocates both temporary paths before fallible operations without EXIT cleanup, and the contributor’s terminal proof exercises the production Node staging path and observes the registered directory removed after forced failure.

Is this the best way to solve the issue?

Yes. Parent-shell registration is the narrowest maintainable solution because it matches scripts/install.sh, avoids command-substitution state loss, covers both known paths, and preserves the original failing exit status.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: The PR fixes a bounded installer failure-path temporary-resource leak with limited blast radius and no evidence of a core-runtime outage.
  • 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 provides after-fix terminal output from macOS Bash exercising the production install_node path and directly observing its registered staging directory removed after forced failure.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix terminal output from macOS Bash exercising the production install_node path and directly observing its registered staging directory removed after forced failure.
Evidence reviewed

PR surface:

Tests +61, Other +14. Total +75 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 1 61 0 +61
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 1 14 0 +14
Total 2 75 0 +75

What I checked:

  • Current-main behavior: Current main creates the Node staging directory before fallible download, checksum, and extraction operations but only removes it on the success path. (scripts/install-cli.sh:821, b7d6291bfd6e)
  • Second affected path: Current main creates a temporary workspace rewrite file before fallible awk and move operations without an installer EXIT cleanup registry. (scripts/install-cli.sh:1057, b7d6291bfd6e)
  • Established sibling pattern: The primary installer already owns a TMPFILES array and cleanup_tmpfiles EXIT trap, so the proposed ownership and lifecycle pattern is established in this repository. (scripts/install.sh:27, b7d6291bfd6e)
  • Exact-head cleanup: The latest head registers both installer-owned mktemp results in the parent shell and deletes only registered paths through the EXIT trap. (scripts/install-cli.sh:35, 2444c8a6b496)
  • Failure-status regression coverage: The tests assert original failure statuses 42 and 43 while independently verifying that the corresponding temporary directory or file has been removed. (test/scripts/install-cli.test.ts:760, 2444c8a6b496)
  • Re-review continuity: The installer and test blobs are unchanged since the previously reviewed head, while the latest force-push only rebased unrelated current-main changes and removed the release-owned changelog edit. (scripts/install-cli.sh:32, 2444c8a6b496)

Likely related people:

  • steipete: Current-main blame and history connect Peter Steinberger to both affected installer allocation paths, and GitHub shows him assigned to and updating this PR’s reviewed revisions. (role: recent installer contributor and assigned reviewer; confidence: high; commits: 92f3c35ee94e; files: scripts/install-cli.sh, test/scripts/install-cli.test.ts)
  • vincentkoc: Vincent Koc has several merged changes in the install-cli history, including package-manager and release-validation work adjacent to this installer surface. (role: recent installer contributor; confidence: medium; commits: 5a7aba94a2bf, 81ecae9d7aa6; files: scripts/install-cli.sh)
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.
Review history (5 earlier review cycles)
  • reviewed 2026-07-10T14:37:53.551Z sha cfcf018 :: needs real behavior proof before merge. :: [P1] Keep temp registration in the parent shell
  • reviewed 2026-07-10T15:45:41.034Z sha cc39ebe :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-10T17:03:56.947Z sha fd9a6b3 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-10T23:59:49.728Z sha c095c66 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-11T00:08:06.398Z sha c095c66 :: needs maintainer review before merge. :: none

@SebTardif

SebTardif commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Updated PR body with authored What Problem This Solves + Evidence, and fixed temp registration so TMPFILES+= runs in the parent shell (command-substitution helpers were a no-op).

@clawsweeper

clawsweeper Bot commented Jul 10, 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:

@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 10, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 10, 2026
@SebTardif

SebTardif commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Updated Evidence with forced failure through the real installer (OPENCLAW_INSTALL_CLI_SH_NO_RUN source + fail() after TMPFILES registration). Before leak vs after CLEANED terminal output included.

@clawsweeper

clawsweeper Bot commented Jul 10, 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.

@SebTardif

SebTardif commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Proof now forces failure on the production install_node path (source install-cli.sh, force download branch, stub download_file → installer fail() after TMPFILES registration). Terminal shows registered_temp=... then AFTER_RESULT=CLEANED for that same path.

@clawsweeper

clawsweeper Bot commented Jul 10, 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:

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. 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. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 10, 2026
@steipete steipete self-assigned this Jul 10, 2026
@steipete
steipete force-pushed the fix/install-cli-tmpfile-cleanup branch from fd9a6b3 to c095c66 Compare July 10, 2026 23:53
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 11, 2026
@steipete
steipete force-pushed the fix/install-cli-tmpfile-cleanup branch 2 times, most recently from 0bfe448 to 2444c8a Compare July 11, 2026 00:28
@steipete
steipete force-pushed the fix/install-cli-tmpfile-cleanup branch from 2444c8a to 63396f8 Compare July 11, 2026 00:40
@steipete

steipete commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Land-ready proof for 63396f851d7e4bb2827015145b42b7ac153e57f4.

What changed:

  • The installer owns one EXIT cleanup list for temporary paths.
  • Node staging and pnpm workspace rewrite callers register their paths in the parent shell immediately after creation.
  • Failure-path regressions cover both owners; the branch is one contributor-authored commit with no PR-local changelog.

Validation:

Known gap: no live checksum-mismatch download was induced. Both production temporary-path owners are covered at their failure boundary. CI attempt 1 hit an unrelated fs-safe test flake; the isolated rerun passed without code changes.

@steipete
steipete merged commit 95b205e into openclaw:main Jul 11, 2026
152 of 153 checks passed
@steipete

steipete commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

vincentkoc added a commit that referenced this pull request Jul 11, 2026
@vincentkoc
chore(release): refresh ledger proof from main
78f3dba 
* origin/main:
  fix(tui): preserve UTF-16 pairs in long tokens (#104024)
  fix(release): raise Telegram attestor buffer (#104025)
  fix(slack): apply sender policy to group DMs (#104007)
  feat(ui): add a background tasks pane to chat panes (#104010)
  docs(changelog): credit landed fixes
  fix(installer): clean temporary files on failure (#103725)
  fix(clickclack): sanitize outbound assistant text (#103142)
  fix(sms): normalize Twilio RCS webhook senders (#102373)
@steipete steipete mentioned this pull request Jul 11, 2026
Merged
@github-actions github-actions Bot mentioned this pull request Jul 11, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 11, 2026
@SebTardif
fix(installer): clean temporary files on failure (openclaw#103725)
67acd6e
vincentkoc pushed a commit that referenced this pull request Jul 13, 2026
@SebTardif @vincentkoc
fix(installer): clean temporary files on failure (#103725)
5bbf27e
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

P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. scripts Repository scripts size: S 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.

2 participants

@SebTardif @steipete