fix(update): make owner refusals and restart recovery actionable - #136995
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
openclaw-barnacle
Bot
added
docs
clawsweeper
Bot
added
P1
|
Codex review: blocked before merge. Reviewed September 3, 2026, 8:30 AM ET / 12:30 UTC. ClawSweeper reviewWhat this changesThis PR adds update/restart diagnostics, owner-aware recovery guidance, health checks, and related tests for Gateway update and restart flows. Merge readiness⛔ Blocked before merge - 7 items remain Keep open: the branch improves update and restart diagnostics, but its introduced recovery guidance still makes terminal commands the only resolution for disabled LaunchAgent and update-failure states, contrary to the stated in-product recovery direction. Priority: P1 Review scores
Verification
How this fits togetherGateway update and restart owners report failures through notices, Doctor diagnostics, and the Control UI. These outputs guide an operator from a failed update or disabled service toward recovery actions. flowchart LR
A[Update or restart failure] --> B[Gateway update and restart owners]
B --> C[Failure notices and diagnostics]
C --> D[Doctor recovery guidance]
C --> E[Control UI triage]
D --> F[Operator recovery]
E --> F
Decision needed
Why: The required recovery action changes product and authorization UX: controls must expose only safe, currently authorized operations and report their real outcome. Before merge
Findings
Agent review detailsSecurityNone. PR surfaceSource +100, Tests +610, Docs +23. Total +733 across 43 files. View PR surface stats
Review metrics
Stored data modelPersistent data-model change detected: Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Hold the PR until failures expose capability-checked, idempotent Control UI recovery actions backed by truthful Gateway outcomes, with CLI guidance retained only as developer-facing fallback information. Do we have a high-confidence way to reproduce the issue? No high-confidence runtime reproduction was executed; source inspection directly shows the introduced terminal-only guidance and the existing UI's navigation-only action. Is this the best way to solve the issue? No: diagnostics alone do not meet the stated recovery contract because the product lacks typed, authorized in-app actions for the reported failure states. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 6b1d8119e189. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What 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 (6 earlier review cycles)
|
ff22c0c to
7b95302
Compare
openclaw-barnacle
Bot
removed
app: web-ui
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
7b95302 to
57720ed
Compare
clawsweeper
Bot
added
rating: 🧂 unranked krab
Give authorized non-owners a sender-specific operator command across text/native commands, update tool refusals, and Gateway revalidation. Preserve unauthorized-sender behavior and correct pairing ownership guidance. Follow-up to #136588.
Use the existing local CLI shared-auth and loopback backend auth-none contracts for readiness. Preserve health diagnostics and served identity without device writes or Gateway auth changes. Follow-up to #136588.
Report an installed but disabled and unloaded selected LaunchAgent with its profile-exact recovery command. Preserve stopped-service behavior in doctor --fix and explain interrupted-update triage. Follow-up to #136588.
81d6ed0 to
cd4c3c0
Compare
|
Still blocked at The new head improves restart verification, skipped-versus-failed status, and user-facing notices, but it does not satisfy the in-app recovery contract:
Revision mismatch, disconnect, timeout, unconfirmed completion, and unknown outcome must keep the user inside the product: automatically converge when safe, or offer capability-checked, idempotent Retry/Restore/Report controls backed by truthful typed outcomes. CLI triage can remain available for developers, but it cannot be the required resolution shown to users. Please make every terminal-instruction occurrence a regression assertion, not accepted UI copy. This PR also remains downstream of repaired #137071 and the residual lifecycle/custody PR; it must not land ahead of them. It contains the |
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Rebased the existing updater follow-ups onto main without conflicts. Main contains Exact head: The six-file run passed all 189 tests. The changed gate exited 0; the existing Doctor temp-helper advisory remains warning-only. Fresh autoreview was explicitly waived for this conflict-free rebase. Disposition of the latest review and recovery-controls request: this maintainer-directed landing retains the existing operator-run CLI diagnosis. Typed, capability-checked Retry/Restore/Report controls and their UI proof remain a named follow-up; this PR does not claim to provide them. The cited Control UI navigation surface is unchanged. The owner-ID visibility tradeoff remains as already documented in the body; update authority remains owner-revalidated. The cloud-lifecycle repair in #137071 remains open. Its overlapping Rank-up proof gaps remain explicit: no fresh Telegram Test Server run or full package-staging update roundtrip here; the previously recorded isolated live restart and notice evidence is retained. No schema, stored representation, migration, configuration, environment-variable, or protocol change is introduced. Existing owner normalization/current-config revalidation and sentinel round-trip tests cover the retained contracts. Production is +188/-88 (net +100), tests/support +1034/-424 (net +610), and docs +24/-1; the rebase adds no production code. Native artifacts include all 15 validator-classified runtime files, including both test helpers. |
|
Merged via squash.
|
Join update discovery and native-auth preparation before shared Gateway teardown. Capture scoped positive and negative auth facts before handing work to catalog workers, while retaining the synchronous plugin hook contract. Use generation-owned workspace allocations for remote skill inputs, derive one location for writes and mounts, and keep the private marker until payload cleanup succeeds. Surface bootstrap network and cleanup failures explicitly. Retain ownership through cancelled Git worktree creation and independent managed-update handoff. No configuration, database, or protocol changes. Refs: #137071, #136084, #136995.
* fix(gateway): own cloud startup and teardown resources Join update discovery and native-auth preparation before shared Gateway teardown. Capture scoped positive and negative auth facts before handing work to catalog workers, while retaining the synchronous plugin hook contract. Use generation-owned workspace allocations for remote skill inputs, derive one location for writes and mounts, and keep the private marker until payload cleanup succeeds. Surface bootstrap network and cleanup failures explicitly. Retain ownership through cancelled Git worktree creation and independent managed-update handoff. No configuration, database, or protocol changes. Refs: #137071, #136084, #136995. * test(auth): align OAuth fixture with async synthetic lookup Preserve the no-refresh assertions while following the async absence contract. Reproduced two missing-export failures before the one-line fixture repair; all 209 owning and sibling tests plus the changed-file gate pass. Refs: #137071.
…nclaw#136995) * fix(commands): explain owner setup in authorized refusals Give authorized non-owners a sender-specific operator command across text/native commands, update tool refusals, and Gateway revalidation. Preserve unauthorized-sender behavior and correct pairing ownership guidance. Follow-up to openclaw#136588. * fix(update): verify restarts through local control clients Use the existing local CLI shared-auth and loopback backend auth-none contracts for readiness. Preserve health diagnostics and served identity without device writes or Gateway auth changes. Follow-up to openclaw#136588. * fix(doctor): diagnose disabled Gateway LaunchAgents Report an installed but disabled and unloaded selected LaunchAgent with its profile-exact recovery command. Preserve stopped-service behavior in doctor --fix and explain interrupted-update triage. Follow-up to openclaw#136588. * fix(gateway): send human-readable update outcome notices * test(update): accept the operator role actor stamped on in-process update dispatch * fix(commands): keep existing owners in the owner setup hint * fix(gateway): report skipped updates as skipped, not failed * test(update): align readiness fixtures with the local control client
* fix(gateway): own cloud startup and teardown resources Join update discovery and native-auth preparation before shared Gateway teardown. Capture scoped positive and negative auth facts before handing work to catalog workers, while retaining the synchronous plugin hook contract. Use generation-owned workspace allocations for remote skill inputs, derive one location for writes and mounts, and keep the private marker until payload cleanup succeeds. Surface bootstrap network and cleanup failures explicitly. Retain ownership through cancelled Git worktree creation and independent managed-update handoff. No configuration, database, or protocol changes. Refs: openclaw#137071, openclaw#136084, openclaw#136995. * test(auth): align OAuth fixture with async synthetic lookup Preserve the no-refresh assertions while following the async absence contract. Reproduced two missing-export failures before the one-line fixture repair; all 209 owning and sibling tests plus the changed-file gate pass. Refs: openclaw#137071.
…nclaw#136995) * fix(commands): explain owner setup in authorized refusals Give authorized non-owners a sender-specific operator command across text/native commands, update tool refusals, and Gateway revalidation. Preserve unauthorized-sender behavior and correct pairing ownership guidance. Follow-up to openclaw#136588. * fix(update): verify restarts through local control clients Use the existing local CLI shared-auth and loopback backend auth-none contracts for readiness. Preserve health diagnostics and served identity without device writes or Gateway auth changes. Follow-up to openclaw#136588. * fix(doctor): diagnose disabled Gateway LaunchAgents Report an installed but disabled and unloaded selected LaunchAgent with its profile-exact recovery command. Preserve stopped-service behavior in doctor --fix and explain interrupted-update triage. Follow-up to openclaw#136588. * fix(gateway): send human-readable update outcome notices * test(update): accept the operator role actor stamped on in-process update dispatch * fix(commands): keep existing owners in the owner setup hint * fix(gateway): report skipped updates as skipped, not failed * test(update): align readiness fixtures with the local control client
* fix(gateway): own cloud startup and teardown resources Join update discovery and native-auth preparation before shared Gateway teardown. Capture scoped positive and negative auth facts before handing work to catalog workers, while retaining the synchronous plugin hook contract. Use generation-owned workspace allocations for remote skill inputs, derive one location for writes and mounts, and keep the private marker until payload cleanup succeeds. Surface bootstrap network and cleanup failures explicitly. Retain ownership through cancelled Git worktree creation and independent managed-update handoff. No configuration, database, or protocol changes. Refs: openclaw#137071, openclaw#136084, openclaw#136995. * test(auth): align OAuth fixture with async synthetic lookup Preserve the no-refresh assertions while following the async absence contract. Reproduced two missing-export failures before the one-line fixture repair; all 209 owning and sibling tests plus the changed-file gate pass. Refs: openclaw#137071.
2 participants
What Problem This Solves
Fixes an issue where authorized chat users receive no actionable response to owner-only commands on an ownerless install, a healthy local Gateway can fail updater readiness checks, and Doctor overlooks a LaunchAgent left disabled by an interrupted update helper.
Follow-up to merged #136588. This branch includes the owner-refusal, local restart-verification, disabled-service diagnosis, and human-readable update-notice repairs.
Why This Change Was Made
Owner refusals now share one sender-specific recovery hint across command replies, tool results, and Gateway requester revalidation. Restart verification reuses the existing local control client and its established CLI/shared-secret and loopback backend/auth-none contracts. Doctor reports persistent LaunchAgent disablement through its existing Gateway health contribution without taking activation authority from the operator.
User Impact
Authorized non-owners get an operator-run command that preserves existing configured owners, removes duplicates and wildcard entries, and adds the requesting sender; tools without config ask the operator to add the sender instead of replacing the owner list; unauthorized senders retain their existing behavior. CLI pairing and Control UI ownership guidance now matches the implemented setup paths. Local restart checks retain plugin/channel failures and served version/build information without creating a device identity or changing Gateway authentication rules. Doctor names the selected profile or custom LaunchAgent label and gives its exact start command, with update/triage advice when installation safety is uncertain.
Skipped update outcomes now read
ℹ️ OpenClaw update skipped: <recorded reason>.; a missing reason readsunknown reason. Error outcomes retain their failure line and running-version detail. No configuration, environment-variable, SQLite, or protocol surface changes. The general remote diagnostic probe is unchanged.Evidence
W15c resolves both P1 findings from the 08:17 UTC revision of the ClawSweeper review:
slack:ownerand newtelegram:123, the command isopenclaw config set commands.ownerAllowFrom '["slack:owner","telegram:123"]'.already-currentremains the recorded reason: no canonical reason constant exists for translating it to a latest-version phrase.node scripts/run-vitest.mjs src/agents/tools/gateway-tool.test.ts src/auto-reply/reply/commands-update.test.ts src/commands/doctor-command-owner.test.ts src/auto-reply/reply/commands-session-restart.test.ts src/gateway/server-methods/update-owner.test.ts src/infra/restart-sentinel.test.ts src/gateway/server-restart-sentinel.test.ts.pnpm check:changed: exit 0 on final W15c tree81d6ed0c0bdacb4c293f342bc8aa792df100fb92, including typechecks, lint, formatting, and selected runtime guards.origin/main: exit 0, scoped-clean at the requested helper's default P0 threshold. No accepted/actionable findings.Latest-review disposition (08:45 UTC, reviewed head 80c3b52): the full-array command exposes configured owner IDs to an authorized non-owner. This is the explicitly requested maintainer design for preserving existing owners; it is retained intentionally. The new redaction/non-disclosure suggestion is not applied because it would replace the specified full-array guidance with a different recovery contract. Owner authority still requires the existing sender check and an operator-run config change. Treat this as an acknowledged visibility tradeoff, not a claim that the reply conceals owner membership. Requester controls are documented as defense in depth within a trusted Gateway domain, not hostile multi-user isolation. The optional Telegram Test Server rank-up remains unperformed under this work order's focused proof scope; the compatibility request is addressed above.
CI follow-up: run 33734359177 exposed 66 updater-owned failures in
checks-node-compact-small-20. The production readiness path already usedcallGateway, but two CLI fixtures still mocked the retiredprobeGatewaycall. The repair moves the existing pure hello/health fixture into shared test support and aligns the transport mocks and health-request assertions. Real service ownership, version/build rejection, sentinel consumption, and plugin-error guards remain exercised. A local rerun also found an unmocked port-availability check in the absent-service fixture; that I/O is now fixture-owned. No production changes were needed. The repaired six-file CLI/restart suite passed all 531 tests in 34.94 seconds.Landing stopped on the next exact-head CI run: checks-node-compact-small-5, job 100600155339 failed
src/commands/agent-exec.test.ts/ “bounds blocked service-relay construction through the shipped CLI command”. The one-second command deadline elapsed before the fake CLI wrote its PID. This test is absent from the PR head and was added to main by 693c16dbccc / #136507; it remains unchanged on fetched main5a9168fea34a6b62b5a97673fb5416e7504ced09. It is outside the updater stack, so the requested unrelated-main-failure stop rule applies. The four W15c commits are pushed; native prepare/merge has not run.Earlier stack proof (retained for context):
git revert --no-commit, retaining the new tests while reverting production fixes. All five lanes failed for the intended old behavior: 19 failing assertions, with 45 controls passing. Restoring the fixes made the same behavior cases pass.pnpm check:changed: exit 0, including the inherited parent stack's macOS validation.pnpm prompt:snapshots:check: current, 7 files.pnpm build: exit 0. All OpenClaw commands used temporary state/config paths.autoreview scoped-clean: no accepted/actionable findings in the selected Git scope and priority(P0 threshold).missing scope: operator.readlines. The temporary LaunchAgent was removed. The socket tests cover token, password, and auth-none, preserve plugin/channel errors, reject remote credential leakage, and verify that no device files are created.Full PR LOC against main: production +188 / -88 (net +100), tests/support +1036 / -424 (net +612), docs +24 / -1 (net +23). W15c including the CI fixture repair adds only +7 net production lines and +36 net test/support lines: current-config owner preservation and explicit skipped status require the small additional decision surface. The restart-client change is net -4 production lines. The two positive production deltas add the shared executable ownership hint and the selected-service diagnosis; no duplicate probe path or handshake exception was added.
The live proof exercises a real managed restart and readiness verification; it does not repeat the full package-staging update roundtrip or send live channel messages. No configuration, baseline, changelog, workflow, protocol, or SQLite changes are included. The changed-check temp-directory warning names an earlier Doctor test with explicit afterEach removal; it is advisory, not an unclean test or gate failure. Landing is authorized after exact-head CI and native prepare/merge gates pass.