fix(update): classify unreadable plugin manifests separately from parse and migration failures#110200
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 17, 2026, 7:30 PM ET / 23:30 UTC. Summary PR surface: Source +55, Tests +78. Total +133 across 6 files. Reproducibility: yes. The supplied real Linux path creates an installed plugin with an unreadable package.json, launches the built gateway as an unprivileged user, and observes the startup refusal; the changed source directly owns classification and formatting of that result. Review metrics: none identified. Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Keep the diagnostic-only scope, review the merge result against current main, and merge if the existing fail-closed plugin-verification policy remains the intended behavior; leave the separate degrade-and-boot decision with the canonical report. Do we have a high-confidence way to reproduce the issue? Yes. The supplied real Linux path creates an installed plugin with an unreadable package.json, launches the built gateway as an unprivileged user, and observes the startup refusal; the changed source directly owns classification and formatting of that result. Is this the best way to solve the issue? Yes for the stated defect. Separating filesystem read errors from JSON parse errors and preserving the existing fail-closed behavior is the narrowest maintainable correction; changing whether the gateway boots is a separate policy decision. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b42dc728510b. Label changesLabel justifications:
Evidence reviewedPR surface: Source +55, Tests +78. Total +133 across 6 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
|
…readable-classification-109833
…readable-classification-109833
…readable-classification-109833
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Merged via squash.
|
Co-authored-by: VACInc <3279061+VACInc@users.noreply.github.com>
What Problem This Solves
Issue #109833: an unreadable (e.g. root-owned, mode-600) plugin
package.jsonblocks the entire gateway with a misleading diagnosis. Three defects compound:readPackagePayloadManifestlumpsEACCES/EPERM/IO read failures into the same catch as JSON parse errors, reportinginvalid-package-json/ "Could not parse package.json: EACCES…" for a file that was never read.openclaw update repair, which cannot fix file ownership/permissions.Why This Change Was Made
Maintainer-instructed mechanical scope only:
src/cli/update-cli/plugin-payload-validation.ts: permission/IO read failures now classify as a distinctunreadable-package-jsonreason instead ofinvalid-package-json.src/commands/doctor-config-preflight.ts: plugin-verification blockers get their own accurate headline ("OpenClaw plugin verification failed; refusing to report the gateway ready.") while genuine state-migration failures keep the existing migration wording. Precedence and fail-closed behavior are unchanged (migration warnings → invalid config → plugin convergence, exactly as before).src/cli/update-cli/post-core-plugin-convergence.ts: permission failures now point at the offending path with ownership/permission remediation instead ofopenclaw update repair.Explicitly out of scope: whether a configured/enabled plugin with an unreadable payload should degrade-and-boot instead of fail-closed. That is a pending product decision (ClawSweeper
needs-product-decision); current deliberate fail-closed behavior is preserved and deferred to the plugin-surface owner.User Impact
Operators hitting an unreadable plugin file during upgrade now see the true cause (permission failure on a named path) with actionable remediation, instead of a false "migration failure" pointing at repair steps that cannot work. No behavior change for whether startup blocks.
Evidence
Real behavior proof
Environment: Linux, Node 24, built CLI from this worktree. The real run used PR head
8b1c51f1921after mergingorigin/mainat9c2d38a596e. Before publishing this evidence, the branch was refreshed again to currentorigin/main(781913025b6) at final PR head28a64a12325; the intervening main commits did not touch any of this PR's six files, and the focused tests were rerun on that final head. Two fresh temporary state roots used the same minimal configured plugin, a canonical tracked install record, andplugins.entries.proof-unreadable-109833.enabled=true. The pluginpackage.jsonwas made unreadable by the same non-root user that launched OpenClaw. Paths and the local username/group are redacted below.For the before run, the three changed production files were restored to byte-exact current-main contents (
git diff --exit-code origin/main -- <three production files>produced no output), then the CLI was built and launched:The branch sources were restored (
git diff --exit-code HEAD -- <three production files>produced no output), a fresh forced CLI build completed, and the same scenario ran against a second fresh state root:Both invocations exited 1 before readiness; a listener check returned
NO_GATEWAY_LISTENER_ON_29187_OR_29188. This proves the diagnostic changed while fail-closed behavior did not. The temporary fixture, config, state databases, installed payloads, and mode-000 files were removed afterward (TEMP_PROOF_STATE_REMOVED).Verification
OPENCLAW_HEAVY_CHECK_LOCK_SCOPE=worktree node scripts/run-vitest.mjs src/cli/update-cli/plugin-payload-validation.test.ts src/cli/update-cli/post-core-plugin-convergence.test.ts src/commands/doctor-config-preflight.state-migration.test.tsOPENCLAW_FORCE_BUILD=1 pnpm dev ...completed separately for the byte-exact current-main and restored branch source trees, supplying the built CLIs used for the two runs.git diff --checkandgit diff origin/main...HEAD --checkpassed.autoreview --mode uncommittedwith no actionable findings. This feedback pass changed no code; it only merged current main, reran proof, and updated PR evidence.Upgrade compatibility and scope
The stored-data-model flag is a false positive from touching startup/doctor and plugin-record consumers: this patch changes only transient discriminated-union values and rendered diagnostics. It does not change config shape, installed-plugin-index serialization, SQLite schema/version, migration inputs/outputs, or persisted state. The same existing state/record shape was accepted by both the current-main and branch runs, so no migration or backfill is required.
Not tested: a live Windows
EPERMsetup or a self-update that mutates the installed OpenClaw package. The real Linux startup run exercises the shared post-core convergence path with an actualEACCES; focused tests cover both startup headline separation and update-path outcome coherence. The configured-plugin fatality question is intentionally not changed or tested as a new behavior; it remains deferred to the plugin-surface owner.Refs #109833.
Maintainer Crabbox proof (exact refreshed head)
A separate maintainer-run before/after proof exercised the real built gateway startup path on fresh sanitized direct-AWS Crabbox leases. The PR run was bound to exact head
28a64a123254ca6e101914a2b4de4374b0302adc; the head was rechecked unchanged after the run.a9dc393f7eb84f515994789b8eefafa206e96914, AWS leasecbx_51173939c1a3, runrun_f62039878a59.28a64a123254ca6e101914a2b4de4374b0302adc, AWS leasecbx_244518ab556e, runrun_c9c0cacb919d.--fresh-pr openclaw/openclaw#110200,--no-hydrate,CRABBOX_ENV_ALLOW=CI, public networking, no Tailscale state, empty AWS instance profile, and trustedscripts/crabbox-untrusted-bootstrap.sh.plugins.allow;package.jsonowned byroot:rootwith mode0600; gateway launched as unprivileged usercrabbox.node dist/entry.js gateway run.Redacted current-main result:
Redacted PR-head result:
All four target claims passed: the failure is classified as
unreadable-package-json; the top-level error identifies plugin verification rather than startup migration; the message names the affected path and gives ownership/permission remediation without update-repair advice; and startup remains deliberately fail-closed with exit 1.Supporting focused tests passed: 3 files / 77 tests across
plugin-payload-validation.test.ts,post-core-plugin-convergence.test.ts, anddoctor-config-preflight.state-migration.test.ts.Setup limitation: the canonical installed-plugin record was seeded with OpenClaw's storage helper to reproduce the already-installed/corrupted state deterministically. The observed behavior used the real built CLI, real Linux file permissions, and the unprivileged gateway user; it did not add a systemd or Docker service-manager layer.
Both task-owned Crabbox leases were released after proof.