fix: bring the other edition's harness down instead of only reporting it - #381
Merged
Conversation
A device converted in place from hermes to openclaw came up with the OpenClaw
gateway healthy and the entire Hermes stack still running: clawbox-gateway,
clawbox-hermes-dashboard, clawbox-hermes-dashboard-proxy and hermes-gateway all
active at once. Both harnesses long-poll getUpdates on the same Telegram bot
token, so each terminated the other's request and the box could not receive a
message for hours.
FOREIGN_EDITION_UNITS could already see that state and step_validate_services
failed the install over it, but nothing acted on it — the appliance finished
loudly broken and waited for an operator who knew which units to name.
step_edition_foreign_teardown closes the gap. The mechanism is not new:
step_edition_gateway_state has always stopped, disabled, removed and masked
clawbox-gateway on hermes; only the openclaw/dual direction was missing. It
drives the same FOREIGN_EDITION_UNITS list the validator uses, so the two cannot
drift, and it runs from step_edition_lock — reaching the full install and the
in-app updater's --step dispatch alike, before any unit of this edition starts.
Deliberately conservative:
- stop + disable only. No mask, no unit file removed, so everything returns
with one `systemctl enable --now`. The mask in step_edition_gateway_state
exists because config/clawbox-sudoers grants the clawbox user NOPASSWD
`systemctl start clawbox-gateway`, which undoes a plain disable; no Hermes
unit has an equivalent grant. hermes-gateway.service is written by the
upstream Hermes installer, so its file is not ours to delete.
- dual is untouched by construction, not by a special case: its foreign list
is empty because both harness predicates are negated.
- never silent — every unit is named with the state it was in and the command
that restores it.
- CLAWBOX_KEEP_FOREIGN_UNITS=1 skips it entirely for an operator mid-diagnosis.
Detection is unaffected either way.
Detection stays as the last line of defence, since a unit can come back under
its own Restart= and the opt-out skips the teardown. Its messages now carry the
exact command to run, and the failure list ends with the one command that redoes
the whole teardown.
The refusal from #377 still precedes all of this: it is a top-level exit during
constant parsing, so a refused transition reaches no step function and "nothing
has been changed" stays literally true. The three places that promised the old
harness would be left running are corrected — install.sh's escape-hatch warning
and refusal text, the same passage in docs-site/editions/overview.mdx, and the
matching note in scripts/setup-hermes-edition.sh. What is still true, and is
what the refusal now rests on, is that the sign-in does not move: each harness
keeps its credentials in its own config and setup_complete carries over, so a
converted device comes up quiet rather than conflicted and still has no usable
model.
Follows install-edition-switch-refusal.test.ts: the real shell function is extracted from install.sh and run against a stubbed systemctl, so the code under test is the code that ships. The stub records every state-changing verb to a file, because install.sh sends those calls to /dev/null — a file is the only way to assert that stop and disable were the only things done, and that mask, unmask and rm were not. Covered: the exact observed device state (openclaw box, whole Hermes stack up); dual left completely alone, with zero systemctl calls; hermes silent when step_edition_gateway_state has already masked the gateway, and still backstopping it if it has not; enabled-but-inactive disabled; masked-but-running stopped; activating and reloading treated as running; idempotence; the CLAWBOX_KEEP_FOREIGN_UNITS opt-out taking only an explicit 1. The wiring tests pin that the teardown is called from step_edition_lock after the gateway state, that the updater path reaches it, that it is dispatchable so the printed repair command is real, that it runs before any unit of this edition starts, and that the refusal still precedes it in the file. Two tests guard the judgement rather than the behaviour: one reads config/clawbox-sudoers and fails if a Hermes unit ever gains the NOPASSWD start grant that justifies masking the gateway, and one fails if hermes-gateway.service ever becomes a unit this repo ships. Either would mean stop+disable is no longer the right amount of force. The validator tests pin the new per-unit fix commands, the single command that redoes the whole teardown, that it is offered only for edition failures, and that a clean device still reports the same 15 checks.
📝 WalkthroughWalkthroughThe installer now detects foreign edition units, stops active units, disables enabled units, supports an opt-out variable, and exposes manual teardown. Edition transition messages and service validation now describe credential boundaries and remediation commands. Tests cover teardown, wiring, validation, and non-destructive behavior. ChangesEdition transition contracts
Foreign-unit teardown
Validation and tests
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EditionLock
participant Teardown
participant ServiceRegistry
participant Systemctl
EditionLock->>Teardown: Run foreign-unit teardown
Teardown->>ServiceRegistry: Find foreign edition units
ServiceRegistry-->>Teardown: Return matching units
Teardown->>Systemctl: Stop active units
Teardown->>Systemctl: Disable enabled units
Teardown-->>EditionLock: Report affected units and recovery commands
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Note This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. Track spend and usage in your billing settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🦀 ClawReviewFresh PR washed in with the tide — here's the gist. Fills the gap where re-running install.sh to switch editions (or using CLAWBOX_ALLOW_EDITION_CHANGE=1) left the old harness's systemd units fully running alongside the new one. On a real device this meant both harnesses long-polling the same Telegram bot token, looping forever on 'Conflict: terminated by other getUpdates request'. The fix is a new step_edition_foreign_teardown that stops and disables whatever is in FOREIGN_EDITION_UNITS before any unit of the incoming edition starts; it is wired into step_edition_lock so it reaches the full install and the in-app updater alike. At a glance
Good to know
— ClawReview 🦀, your resident reef crab. Just orientation — CodeRabbit does the line-by-line, humans do the merge. Conventions: docs. |
github-actions
Bot
added
area: docs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/editions/overview.mdx`:
- Around line 96-103: Clarify the edition-mismatch behavior: in
docs-site/editions/overview.mdx (96-103), state that refusal is a no-op
requiring a reflash, and limit foreign-unit teardown to same-edition installs or
manual repair; update install.sh (166-175) to distinguish refusal from cleanup
and (221-225) to remove the stop/disable claim; change
scripts/setup-hermes-edition.sh (109-111) to say install.sh does not migrate;
update src/tests/unit/install-foreign-edition-teardown.test.ts (349-359) to
execute the refusal path and assert its diagnostic says no units changed.
In `@install.sh`:
- Around line 1220-1226: Update the foreign-unit teardown flow around
needs_stop, needs_disable, and brought_down so each requested systemctl stop or
disable is followed by a state check. Return nonzero and skip adding the unit to
brought_down when it remains active or enabled, and add coverage for stop and
disable failures.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ff82d3cc-5500-4920-86d9-1837d1712131
📒 Files selected for processing (4)
docs-site/editions/overview.mdxinstall.shscripts/setup-hermes-edition.shsrc/tests/unit/install-foreign-edition-teardown.test.ts
Comment on lines
+96
to
+103
| The installer does not migrate an edition. It will stop and disable the harness | ||
| the device is leaving — two agents on one box conflict over shared resources such | ||
| as the Telegram bot token, so that half is not left to chance — but each harness | ||
| keeps its AI provider sign-in in its own configuration file, and signing in does | ||
| not carry across. A device installed over the top of another edition ends up with | ||
| no usable model while still reporting that setup is complete, which is why the | ||
| transition is refused rather than half performed. Changing a device's edition is | ||
| a reflash. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not describe a refused edition change as a teardown.
The edition-mismatch path exits before any step function runs. It does not stop or disable units. The current text can cause an operator to believe that a rejected migration changed service state.
docs-site/editions/overview.mdx#L96-L103: state that a refused edition change is a no-op and requires a reflash; describe foreign-unit teardown only for same-edition installs or manual repair.install.sh#L166-L175: update the implementation comment to distinguish refused edition changes from foreign-unit cleanup.install.sh#L221-L225: remove the claim that the departing harness “will stop and disable” from the refusal diagnostic.scripts/setup-hermes-edition.sh#L109-L111: change “install.sh migrates” to “install.sh does not migrate.”src/tests/unit/install-foreign-edition-teardown.test.ts#L349-L359: execute the refusal path and assert that its diagnostic states that no units changed.
📍 Affects 4 files
docs-site/editions/overview.mdx#L96-L103(this comment)install.sh#L166-L175install.sh#L221-L225scripts/setup-hermes-edition.sh#L109-L111src/tests/unit/install-foreign-edition-teardown.test.ts#L349-L359
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/editions/overview.mdx` around lines 96 - 103, Clarify the
edition-mismatch behavior: in docs-site/editions/overview.mdx (96-103), state
that refusal is a no-op requiring a reflash, and limit foreign-unit teardown to
same-edition installs or manual repair; update install.sh (166-175) to
distinguish refusal from cleanup and (221-225) to remove the stop/disable claim;
change scripts/setup-hermes-edition.sh (109-111) to say install.sh does not
migrate; update src/tests/unit/install-foreign-edition-teardown.test.ts
(349-359) to execute the refusal path and assert its diagnostic says no units
changed.
Comment on lines
+1220
to
+1226
| if [ "$needs_stop" = true ]; then | ||
| systemctl stop "$funit" >/dev/null 2>&1 || true | ||
| fi | ||
| if [ "$needs_disable" = true ]; then | ||
| systemctl disable "$funit" >/dev/null 2>&1 || true | ||
| fi | ||
| brought_down+=("$funit (was active=$f_active enabled=$f_enabled)") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail when foreign-unit teardown fails.
Lines 1221 and 1224 discard systemctl failures. Line 1226 then reports the unit as brought down. A direct --step edition_foreign_teardown run can return success while a foreign unit remains active or enabled.
Re-check the required state after each operation. Return nonzero and do not add the unit to brought_down if it remains active or enabled. Add tests where systemctl stop and systemctl disable fail.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` around lines 1220 - 1226, Update the foreign-unit teardown flow
around needs_stop, needs_disable, and brought_down so each requested systemctl
stop or disable is followed by a state check. Return nonzero and skip adding the
unit to brought_down when it remains active or enabled, and add coverage for
stop and disable failures.
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
1 participant
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 happened on a device
A box was converted in place from the
hermesedition toopenclawby re-runninginstall.sh. The OpenClaw gateway came up healthy — and the Hermes stack was still running alongside it:Two harnesses long-polling
getUpdateson one Telegram bot token, each knocking the other off:Telegram was dead on that box for hours.
The decision: act, don't just report
FOREIGN_EDITION_UNITSalready saw this —step_validate_servicesfailed the install over it. But nothing acted on it, so the appliance finished loudly broken and waited for an operator who knew which units to name.Three things decided it:
1. The repo already accepts this mechanism — it was only written in one direction.
step_edition_gateway_statestops, disables, removes and masksclawbox-gatewayonhermes. "install.sh brings a foreign harness down" is shipped behaviour; the openclaw/dual half was simply missing. The old comment "Nothing in install.sh brings these down" described the Hermes-side units, not the mechanism.2. #377's refusal does not cover every route into this state. It fires only when a recorded edition exists and differs. Two gaps remain:
CLAWBOX_ALLOW_EDITION_CHANGE=1is a documented escape hatch whose own warning text promised the old harness would be left running — that is this state, by design./etc/clawbox/edition.envand the legacy drop-in are both absent (wiped, restored from backup, provisioned by a route that never wrote them), the refusal is skipped entirely and no flag is needed.3. It is a shared exclusive resource, not a preference. One bot token, one long-poll. Two pollers is not "degraded" — neither side wins.
Rejected: leave it at detection. The argument against acting is that stopping services an operator may be mid-diagnosis on is its own hazard. That is answered rather than dismissed — the teardown is never silent, is reversible with one command, and
CLAWBOX_KEEP_FOREIGN_UNITS=1opts out entirely while leaving detection untouched. Detection alone leaves an appliance broken at the end of a run the operator started.Rejected: mask the Hermes units the way the gateway is masked. The gateway's mask is justified by a specific fact —
config/clawbox-sudoersgrants the clawbox user NOPASSWDsystemctl start clawbox-gateway, so a plain disable is undone from the in-UI terminal. No Hermes unit has an equivalent grant, so a mask would only cost reversibility.hermes-gateway.serviceis written by the upstream Hermes installer, so deleting its unit file is not ours to do either. A test reads the sudoers file and fails if that ever stops being true.What this adds
step_edition_foreign_teardown, driven by the sameFOREIGN_EDITION_UNITSlist the validator uses so the two cannot drift. Called fromstep_edition_lockafterstep_edition_gateway_state, so it reaches the full install and the in-app updater's--stepdispatch alike, and runs before any unit of this edition starts.systemctl enable --now.dualuntouched by construction — its foreign list is empty because both harness predicates are negated. Zero systemctl calls, no output.CLAWBOX_KEEP_FOREIGN_UNITS=1skips it; detection still fails the install.sudo bash install.sh --step edition_foreign_teardown.Detection stays as the last line of defence (a unit can return under its own
Restart=, and the opt-out skips the teardown). Its messages now carry the exact command per unit, and the failure list ends with the one command that redoes the whole teardown.The refusal from #377 still precedes all of it — a top-level exit during constant parsing, so a refused transition reaches no step function and "nothing has been changed" stays literally true.
Corrected claims
Three places promised the old harness would be left running and no longer can: install.sh's escape-hatch warning and refusal text, the same passage in
docs-site/editions/overview.mdx, and the note inscripts/setup-hermes-edition.sh. What is still true — and what the refusal now rests on — is that the sign-in does not move: each harness keeps its credentials in its own config andsetup_completecarries over, so a converted device comes up quiet rather than conflicted and still has no usable model. Changing edition is still a reflash.Tests
src/tests/unit/install-foreign-edition-teardown.test.ts(29 tests), followinginstall-edition-switch-refusal.test.ts: the real shell function is extracted frominstall.shand run against a stubbedsystemctlthat records every state-changing verb to a file — install.sh sends those calls to/dev/null, so a file is the only way to assert stop and disable were the only things done, and mask/unmask/rm were not.Covered: the exact observed device state;
dualwith zero calls;hermessilent after the gateway state already masked, and still backstopping if it did not; enabled-but-inactive; masked-but-running; activating/reloading; idempotence; the opt-out taking only an explicit1; the wiring and ordering; and the new validator hints.(new file +
install-edition-switch-refusal.test.ts, run on Linux — the bash-backed suites skip on win32)Full unit project: 174 files / 2406 tests passed. ESLint and
tsc --noEmitclean;bash -nclean on both scripts.Summary by CodeRabbit
CLAWBOX_KEEP_FOREIGN_UNITS=1.