Repository navigation
fix(cli): preserve plugin manifests when builds fail - #140740
Merged
obviyus merged 4 commits intoSep 7, 2026
Merged
Conversation
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Contributor
|
Codex review: blocked before merge. Reviewed September 7, 2026, 1:45 AM ET / 05:45 UTC. ClawSweeper reviewWhat this changesThe PR publishes plugin package metadata atomically, preserves permission bits, and adds failure-preservation and symlink-root coverage. Merge readiness⛔ Blocked before merge - 5 items remain The repair remains necessary: current main and v2026.9.2 still overwrite package.json directly. The earlier permission-mask concern is addressed, and no concrete introduced defect was established. Priority: P2 Review scores
Verification
How this fits togetherThe plugin authoring CLI reads a plugin entry and existing package metadata, then generates manifests used to load the plugin. This change protects the package manifest when filesystem publication fails. flowchart TD
A[Plugin entry] --> C[Build plugin metadata]
B[Existing package manifest] --> C
C --> D[Stage package manifest]
D --> E{Publication succeeds?}
E -->|Yes| F[Publish plugin manifest]
E -->|No| G[Preserve original and report error]
Decision needed
Why: The permission-mask correction is visible and runtime evidence is positive, but this environment could not inspect the exact dependency version requested by the previous review. Before merge
Agent review detailsSecurityNone. PR surfaceSource +9, Tests +107. Total +116 across 2 files. View PR surface stats
Review metricsNone. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep atomic publication at the existing manifest writer boundary, preserving project permissions and symlink-root behavior, with the pinned helper contract confirmed before landing. Do we have a high-confidence way to reproduce the issue? Yes: current main directly overwrites package.json, and the supplied real CLI comparison demonstrates truncation under a file-size limit. This review inspected source without executing the reproduction. Is this the best way to solve the issue? Yes: reusing the existing atomic replacement helper is a focused repair that preserves generated JSON semantics; exact dependency verification remains the outstanding review limitation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6ca2ef59b8e5. LabelsLabel changes:
Label 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)
|
Collaborator
|
Thanks @zenglingbiao! Merged as 3be8b66. Proof: Real
Owned temporary files are removed. Substituted, multiply linked, or relocated entries remain protected when cleanup ownership cannot be verified. The original literal cleanup failure, its clarified ownership interpretation, and all raw observations are retained; no unconditional deletion or cross-manifest transaction is claimed. |
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Sep 8, 2026
## What Problem This Solves A failed `openclaw plugins build` could truncate the user's hand-maintained `package.json`, leaving invalid JSON and breaking the plugin project. ## Solution Publish the complete package manifest through the existing atomic replacement helper. Preserve the file mode and all parent-directory permission bits, including setgid and sticky bits. A failed rename reports an error without falling back to an in-place copy. Resolve a selected symlink project root for publication while preserving entry validation and displayed paths. ## User Impact A failed package-manifest write preserves the original file. After the filesystem problem is resolved, the same build can succeed. Successful JSON output, `--check`, initialization, and adjacent plugin-manifest publication retain their behavior. ## Evidence - Real CLI under a file-size limit: the original publication code truncated a 5,164-byte manifest to 2,048 bytes of invalid JSON. The repaired runtime preserved all 5,164 bytes through both physical and symlink project roots. - Partial disk-full writes and rename permission failures preserve bytes, file mode `02640`, directory mode `03770`, and remove owned temporary files. Successful retries generate both manifests. - Staged-file symlink, hardlink, file replacement, and directory replacement are rejected without modifying unrelated targets. Entries whose ownership can no longer be verified remain protected instead of being deleted. - Fresh independent CLI checks cover relative and absolute entries through symlink roots, failures, retries, modes, checks, and initialization. Adjacent-manifest failure remains visible; no transaction across both manifests is promised. - All 28 focused authoring tests passed, including the symlink-root regression. The two interrupted-publication cases fail against the original publication code for the expected reasons. - Formatting, lint, source review, build, type checks, and [CI for this head](https://github.com/openclaw/openclaw/actions/runs/34087634188) passed. Runtime proof used the compiled merge artifact from that run. Co-authored-by: Ayaan Zaidi <hi@obviy.us>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
A failed
openclaw plugins buildcould truncate the user's hand-maintainedpackage.json, leaving invalid JSON and breaking the plugin project.Solution
Publish the complete package manifest through the existing atomic replacement helper. Preserve the file mode and all parent-directory permission bits, including setgid and sticky bits. A failed rename reports an error without falling back to an in-place copy. Resolve a selected symlink project root for publication while preserving entry validation and displayed paths.
User Impact
A failed package-manifest write preserves the original file. After the filesystem problem is resolved, the same build can succeed. Successful JSON output,
--check, initialization, and adjacent plugin-manifest publication retain their behavior.Evidence
02640, directory mode03770, and remove owned temporary files. Successful retries generate both manifests.