fix: refuse an unsupported edition transition and make validation honest - #377
Conversation
Re-running the installer with a different CLAWBOX_EDITION on a provisioned device completed every step and reported a healthy box, while leaving it in a state it cannot recover from on its own. The installer installs an edition; it does not migrate one. There is no step that removes the harness a device is leaving, and each harness keeps its AI provider credentials in its own config file, so a sign-in does not move across. The result is two harnesses running at once, an incoming harness with an empty provider registry so no model resolves, and setup_complete carried over so the wizard step that would repair it never runs again. Refuse the transition instead: * install.sh compares the requested edition against the root-owned lock (with the legacy drop-in as the fallback for older boxes) and stops before anything is changed. The check sits immediately after the edition resolves, so it runs ahead of the lock rewrite, ahead of any mask/unmask, and ahead of any unit being installed or started, on the full-install path and on --step dispatch alike. CLAWBOX_ALLOW_EDITION_CHANGE=1 is the single explicit override, and the refusal names it. * scripts/setup-hermes-edition.sh is the other writer of the lock and carries the same refusal. Without it, a standalone run could rewrite the lock and a later plain install.sh would then see no mismatch at all. * dual is treated as a distinct edition in both directions. The additive direction fails the same way as the destructive one, and the lock exists so the SKU cannot be selected from the environment. Fresh installs, same-edition re-installs and the updater's provisioning path are unaffected: they either have no lock yet or resolve the recorded value. Validation was blind in one direction as well. It only ever asserted that this edition's units are up, so a leftover harness from another edition scored zero failures. step_validate_services now also asserts the absence of foreign units and fails on any that are active or enabled, which is what stops the healthy summary from being printable on a device running two harnesses. Edition migration is deliberately not implemented.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe installer and standalone Hermes setup now enforce recorded edition consistency. They normalize edition values, refuse unsupported switches before changes, support an explicit override, detect foreign edition services, and document reflash requirements. ChangesEdition consistency enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant install_sh as install.sh
participant edition_lock as recorded edition lock
participant systemd as systemd services
Operator->>install_sh: request installation or edition
install_sh->>edition_lock: read and normalize recorded edition
install_sh->>install_sh: reject mismatched edition or apply override
install_sh->>systemd: validate edition-scoped units
systemd-->>install_sh: report active or enabled foreign units
install_sh-->>Operator: continue or fail validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
🦀 ClawReviewPoked my eyestalks out for this one. Quick tour: The installer previously let a At a glance
Good to know
— ClawReview 🦀, scuttling off. General info only — see CodeRabbit for the detailed review. 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: 3
🤖 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 `@install.sh`:
- Around line 120-136: The edition resolution case should apply the same
whitespace normalization as _normalise_edition before matching values. Update
the resolution flow using CLAWBOX_EDITION_RAW so inputs such as “ hermes ”
resolve identically to normalized editions, while preserving the existing
handling for empty and unrecognized values.
In `@scripts/setup-hermes-edition.sh`:
- Around line 49-52: Update the recorded-edition initialization before any
configuration or service operations to fall back to the legacy systemd drop-in
when EDITION_FILE is absent. Reuse the existing normalization and parsing
behavior so RECORDED_EDITION is populated from the legacy CLAWBOX_EDITION value,
allowing the later mismatch check to reject conflicting editions.
- Around line 45-53: Validate RECORDED_EDITION with normalizeEdition() before
the edition mismatch check, treating unsupported values such as openclw as
absent rather than rejecting the request. Preserve valid recorded editions and
the existing REQUESTED_EDITION handling.
🪄 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: 63223e0a-6fb5-4ca3-9f46-dbe04bb24721
📒 Files selected for processing (4)
docs-site/editions/overview.mdxinstall.shscripts/setup-hermes-edition.shsrc/tests/unit/install-edition-switch-refusal.test.ts
All three came out of review of the first commit. Read the legacy drop-in in setup-hermes-edition.sh. It resolved the recorded edition from /etc/clawbox/edition.env only, so on a device provisioned before that file existed the recorded value stayed empty and a conflicting CLAWBOX_EDITION walked straight past the refusal. That is the same gap the installer already covers, left open in the second writer. It now reads the drop-in as the fallback, using the same two-shape parser install.sh uses. Map an unrecognised recorded value to openclaw there too. install.sh already does this, so a typo'd lock described an openclaw device to one writer and an unknown edition to the other. Reading it as "absent" instead would be the dangerous direction: it would clear the refusal entirely and let a typo'd lock be provisioned straight over. Align the whitespace rule in install.sh. The recorded value was whitespace-stripped but the resolution was not, so a padded CLAWBOX_EDITION=" hermes " resolved to openclaw (unrecognised, warn, fall back) while the lock still read hermes — refusing on a device nobody was trying to change, and reporting a requested edition the operator never typed. The two rules are now identical. Tests cover each: the drop-in path, the typo'd lock in both directions, a padded value that matches, and a padded value that genuinely differs.
|
All three findings were valid and are fixed in 1. Legacy drop-in in 2. Unrecognised recorded value — correct that the two writers disagreed, and fixed, but not by treating the value as absent. 3. Whitespace alignment in New tests cover each: the drop-in path, the typo'd lock in both directions, a padded value that matches, and a padded value that genuinely differs. 47 tests in the file, full suite 2462 green. |
1 participant
Problem
Re-running the installer with a different
CLAWBOX_EDITIONon a device that is already provisioned completed all 26 steps and printed a healthy summary — for a device left in a state it cannot recover from on its own.The installer installs an edition; it does not migrate one. There is no step that stops, disables or masks the harness a device is leaving, and each harness keeps its AI provider credentials in its own config file, so a sign-in does not move across. What you get is:
setup_completecarried over, so the wizard step that would repair all of it never runs again.docs-site/editions/overview.mdxalready documented edition changes as reflash-only. The installer just did not enforce it.What this changes
1. The installer refuses the transition.
install.shcompares the requested edition against the root-owned lock — with the legacy systemd drop-in as the fallback, so boxes provisioned beforeedition.envexisted are covered too — and stops before anything is changed. The message states what the device currently is, what was requested, that changing edition requires a reflash, and names the single override.The check sits immediately after the edition resolves, during constant parsing. That is ahead of the lock and drop-in rewrite, ahead of any mask/unmask, and ahead of any unit being copied, enabled or started — on the full-install path and on the
--stepdispatch path the in-app updater uses, since both reach that point before any step function is even defined. A refusal that fires after the gateway has been unmasked would not be a refusal.CLAWBOX_ALLOW_EDITION_CHANGE=1is the one explicit escape hatch. It is named in the refusal and left out of the customer-facing docs on purpose.2. The other writer of the lock carries the same refusal.
scripts/setup-hermes-edition.shalso rewrites/etc/clawbox/edition.envand the legacy drop-in, and it read$CLAWBOX_EDITIONahead of the lock — the one place where the environment outranked the root-owned file, and the place that then rewrote it. Guarding onlyinstall.shwould have left that path open, and worse, it launders the change: once the lock reads the new edition, a later plaininstall.shsees no mismatch and provisions on top. Both writers now refuse, and this one reads the lock first like every other consumer.3.
dualis a distinct edition in both directions.openclaw → dualandhermes → dualare refused too. The additive direction fails the same way as the destructive one — the harness being added comes up with no provider registry and no credentials, andsetup_completesuppresses the step that would populate them — and the lock exists precisely so the SKU cannot be selected from the environment.4. Validation is honest about foreign units.
step_validate_servicesonly ever asserted that this edition's units are up; it appends the Hermes units insideif has_hermes_harness, so onopenclawa fully running Hermes stack was not merely tolerated, it was invisible. A newFOREIGN_EDITION_UNITSregistry — built from the existinghas_hermes_harness/has_openclaw_harnesspredicates, sodualaccumulates nothing and needs no special case — is now checked, and any foreign unit that is active or merely enabled fails the run. That is what stops the healthy summary being printable on a device running two harnesses.Edition migration is deliberately not implemented. Doing it properly means migrating credentials, tearing down the other harness and re-running the AI step; that is a feature, not this fix.
Unaffected
post_update/ the in-app updater —clawbox-root-update@.serviceexportsCLAWBOX_EDITIONfrom the lock, so requested and recorded are equal by construction on every step it dispatches.config/edition.txtis still a resolution source but is never treated as a record; it lives in a customer-writable tree.openclawbefore comparing, so a typo'd lock cannot brick a device's updates.Tests
src/tests/unit/install-edition-switch-refusal.test.ts(43 tests). Following the conventions ininstall-edition-lock.test.tsandregister-mcp-hermes.test.ts, these execute the shipped shell text rather than string-matching it: the edition-resolution/refusal region andstep_validate_servicesare extracted frominstall.shand run against a temp lock file and a fakesystemd.1opens it (yes,true,2,0all still refuse).step_*definition,DISPATCH_STEPS, and the installer banner.systemctl stop|disable|mask|unmaskand norm -rf, holding the "no migration" scope boundary.openclawbox passes; the same box with the Hermes stack alive fails 3 of 15; enabled-but-inactive fails; installed-but-disabled passes;hermesstill running the gateway fails;dualrunning both is healthy.Full suite green — 197 files, 2458 tests.
eslintclean on the changed files,shellcheck -S warningclean on both scripts, no newtscerrors.Summary by CodeRabbit
New Features
Documentation
Bug Fixes