release: promote beta to main (hermes dashboard-auth fix) - #387
Merged
Conversation
A failed Hermes provision could not be told apart from a good one. Three independent defects made a box that provisioned badly look healthy: - The dashboard-auth check returned a bare exit 1 for every failure — a real password/hash mismatch, a config another writer had just rewritten, an unreadable file, or an interpreter without scrypt — and then blamed the credentials, which were correct. It now classifies the outcome: 'could not run the check' and 'the password does not match the hash' are distinct outcomes with distinct messages and exit codes, and an environment failure is never reported as a credential fault. Exposed as a reusable '--check' mode. - Root cause of the recurring failure: ~/.hermes/config.yaml has more than one writer (this script and register-mcp.sh, run seconds apart at install time), and a lost update erased the dashboard block between the write and the verify. Both scripts now take one shared flock over the config so their read-modify-write cycles serialise. - A non-fatal step_hermes_edition failure never reached the summary: install.sh printed 'Setup Complete' and exited 0 even after provisioning reported errors. It now records the failure, prints an INCOMPLETE summary, writes a provision-status marker, and exits non-zero so the flash host cannot report success over it. - step_validate_services could report every check healthy right after auth failed, because its only auth probe accepted the failure symptom. It now verifies the auth provider directly via the auth script's --check. Adds regression tests for each and keeps the existing behavioural tests green.
fix: make hermes dashboard-auth failures honest and propagate them
📝 WalkthroughWalkthroughHermes authentication provisioning now uses shared locking, credential-state classification, atomic writes, and read-only checks. Installer validation records provisioning failures, verifies authentication directly, rejects proxy HTTP 401 responses, writes machine-readable status, and returns failure when provisioning or validation is incomplete. ChangesHermes provisioning reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant install.sh
participant setup-hermes-dashboard-auth.sh
participant HermesConfig
install.sh->>setup-hermes-dashboard-auth.sh: run --check
setup-hermes-dashboard-auth.sh->>HermesConfig: read dashboard credentials
HermesConfig-->>setup-hermes-dashboard-auth.sh: classified credential state
setup-hermes-dashboard-auth.sh-->>install.sh: return check result
install.sh->>install.sh: combine authentication and proxy probes
install.sh-->>install.sh: write status and exit code
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: A release promotion (beta → main) shipping the fix for a Hermes provisioning race: two concurrent writers of ~/.hermes/config.yaml — the dashboard auth script and register-mcp.sh — could silently erase each other's block between write and verify, causing a confusing credential-mismatch error when the credentials were always correct. Both scripts now take an exclusive flock on a shared lock file, the credential classifier distinguishes environment failures from actual mismatches (exit codes 3–7), install.sh now exits non-zero when a provisioning step fails, and a machine-readable /etc/clawbox/provision-status marker is written for flash-host tooling. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 79-93: Update write_provision_status so it detects and reports
failures creating the directory or writing the status file instead of
suppressing them with unconditional “|| true” handling. Emit a clear failure
message on stdout and return a nonzero status when the marker cannot be
persisted, while preserving successful writes and chmod behavior.
- Around line 64-77: Update the non-fatal wrappers in step_post_update and
step_rebuild_reboot to call record_provision_failure when their wrapped
operations fail, ensuring --step dispatch propagates a non-zero result and does
not report success without the status marker. Alternatively, narrow the
provisioning-status comment and behavior to full-install mode if these
update-step failures are intentionally excluded.
In `@scripts/setup-hermes-dashboard-auth.sh`:
- Around line 360-391: Add a short delay before the `continue` in the
`CREDS_NOT_CONFIGURED`/`CREDS_PW_MISSING` retry branch of the main attempt loop,
while preserving the existing retry limit and logging. The delay should occur
only when another attempt remains, allowing the competing writer time to finish
before `mint_credentials` runs again.
- Around line 289-292: Update the password-file write in the setup flow around
PWFILE to write the new password to a securely permissioned temporary file in
the same directory, then atomically replace PWFILE with mv. Preserve 0600
permissions and cleanup/error handling, and avoid truncating PWFILE in place.
In `@src/tests/unit/hermes-config-lock.test.ts`:
- Around line 46-50: Harden both shell-source extraction tests against missing
or malformed markers. In src/tests/unit/hermes-config-lock.test.ts#L46-L50,
assert both indexOf results are greater than -1 before comparing their order,
and identify the acquire_config_lock call with an anchored regex rather than
formatting-dependent text. In
src/tests/unit/install-hermes-edition-step.test.ts#L183-L191, verify the
extracted step_validate_services body reaches the function end before asserting
it does not contain the service pattern.
- Around line 96-111: Replace the non-portable `date +%s.%N` timing inside the
`script` used by the hermes config lock test with elapsed-time measurement
performed by the test process, avoiding shell or platform-specific timer
dependencies. Preserve the lock-holder setup and the `elapsed > 0.5` assertion
while ensuring the measured value remains numeric and portable on macOS and
other non-Windows platforms.
In `@src/tests/unit/hermes-dashboard-auth-yaml.test.ts`:
- Around line 77-93: Update scryptHash to validate the spawnSync result before
reading stdout: fail fast when Python cannot start or exits with a non-zero
status, and only trim and return stdout after successful execution. Keep
seedBlock and the fixture tests unchanged so setup failures report the broken
hash generator directly.
- Around line 315-324: Consolidate the duplicated environment setup by adding an
optional extraEnv parameter to run, merging it into the spawned process
environment, and declaring run before its first use. Update the current run2
call site to pass the environment override through run, then remove the run2
helper while leaving check and existing run behavior unchanged.
In `@src/tests/unit/install-hermes-edition-step.test.ts`:
- Around line 183-191: Strengthen the test for step_validate_services by
asserting that extractShellFunction returns the complete function slice before
checking its status patterns. Add a guard using the function’s expected ending
or another reliable end-of-function marker, so truncation caused by an embedded
column-zero “}” fails the test rather than allowing the negative 401 assertion
to pass incorrectly.
🪄 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: 88a1b79f-6a0c-4fb8-9b7b-1c7258c563a2
📒 Files selected for processing (6)
install.shscripts/register-mcp.shscripts/setup-hermes-dashboard-auth.shsrc/tests/unit/hermes-config-lock.test.tssrc/tests/unit/hermes-dashboard-auth-yaml.test.tssrc/tests/unit/install-hermes-edition-step.test.ts
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.
Second promotion of the day, cut so that a Hermes device can be reset onto a
release build that contains the fix written for it.
What this adds over
505cab6The Hermes dashboard-auth failure is fixed at its cause. Provisioning a
Hermes device could report that a freshly written password and hash did not
verify, while both were correct. The cause was a lost update: the installer
starts the web service — which asynchronously rewrites the Hermes config — and
then, a few steps later, writes the dashboard block and re-reads it to check.
A write landing between those two points removed the block, and the check
reported it as a credential mismatch.
The same race had a second, quieter phase: when the competing write landed
after the check instead of before it, provisioning reported success on a device
that had no auth provider at all.
This change closes the race, separates "the check could not run" from "the
credentials disagree" so the message names the real condition, and makes a
failed provisioning step visible in the summary rather than reported as
success.
Why now
Devices are being prepared for shipping. A Hermes unit reset onto
mainwithout this would carry a provisioning step that can fail confusingly, or
succeed misleadingly — neither is something to hand to a customer.
Verification
Beyond CI: the failure was reproduced deterministically before the fix, and the
OpenClaw devices flashed from
505cab6earlier today are unaffected by it, sothis narrows rather than widens what is already in the field.
Summary by CodeRabbit
Bug Fixes
Improvements
STATUS=okorSTATUS=incomplete).