fix: make hermes dashboard-auth failures honest and propagate them - #386
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.
📝 WalkthroughWalkthroughHermes dashboard authentication now supports classified checks, secure retries, and shared configuration locking. Install validation checks authentication-provider state and proxy responses. Install failures are recorded in summaries, status files, sentinels, and exit codes. ChangesHermes provisioning reliability
Estimated code review effort: 4 (Complex) | ~45 minutes 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. Three independent fixes so a broken Hermes provisioning can never masquerade as a healthy install. The auth script now classifies failures by kind (environment error vs. genuine mismatch vs. lost write) instead of collapsing everything into exit 1; a shared flock on At a glance
Good to know
— ClawReview 🦀, scuttling off. General info only — see CodeRabbit for the detailed review. 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: 7
🤖 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 81-93: Update write_provision_status so failures creating the
directory, writing PROVISION_STATUS_FILE, or applying chmod are no longer
silently ignored; report the marker-write failure to the operator while
preserving the install failure status and ensuring a stale STATUS=ok is not
treated as authoritative.
In `@scripts/register-mcp.sh`:
- Around line 47-55: Set HERMES_CONFIG explicitly to
/home/clawbox/.hermes/config.yaml after the EnvironmentFile entries in
clawbox-setup.service, ensuring register-mcp.sh uses the same configuration and
lock path as setup-hermes-edition.sh regardless of imported environment
overrides.
In `@scripts/setup-hermes-dashboard-auth.sh`:
- Around line 72-90: Extract acquire_config_lock and the CONFIG_LOCK derivation
from scripts/setup-hermes-dashboard-auth.sh lines 72-90 into a shared file such
as scripts/lib/hermes-config-lock.sh, then source it from that script. In
scripts/register-mcp.sh lines 64-79, remove the duplicated helper and
CONFIG_LOCK assignment and source the same shared file, preserving the existing
locking behavior in both writers.
- Around line 279-292: Update mint_credentials to explicitly check and return
failures from the PWFILE write and the mkdir -p calls near the start of the
function. Ensure each failed filesystem operation returns a nonzero status so
the caller reports a direct write or directory-creation error instead of
classifying it as a concurrent rewrite.
In `@src/tests/unit/hermes-config-lock.test.ts`:
- Around line 41-53: Guard all source-marker lookups before slicing or
comparing. In src/tests/unit/hermes-config-lock.test.ts lines 41-53, capture the
REGISTER_SRC marker index, assert it is greater than -1, prefer a regex anchored
to the acquire_config_lock call, then reuse the validated index for slicing and
ordering checks. In src/tests/unit/install-hermes-edition-step.test.ts lines
170-176, capture probe.indexOf("case"), assert it is greater than -1, and only
then slice with that index.
- Around line 96-111: Update the lock timing test around the shell script and
spawnSync call to measure the blocking AUTH execution with Node’s timing API
rather than bash date arithmetic. Record the start time immediately before
spawnSync and compute elapsed after it returns, while preserving the existing
lock-holder setup and greater-than-0.5-second assertion.
In `@src/tests/unit/hermes-dashboard-auth-yaml.test.ts`:
- Around line 77-93: Update scryptHash to validate the spawnSync result before
returning stdout: assert or throw when proc.status indicates python3 failed,
including stderr or another useful failure detail, then return the trimmed
stdout only on success. Keep the existing fixture-generation output unchanged
for successful executions.
🪄 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: a10bdcaa-0cc0-4c6f-9e3d-1b9a4b1d67ce
📒 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
| write_provision_status() { | ||
| local status="$1"; shift | ||
| local dir | ||
| dir="$(dirname "$PROVISION_STATUS_FILE")" | ||
| mkdir -p "$dir" 2>/dev/null || true | ||
| { | ||
| echo "# Written by install.sh at the end of a full install. Machine-readable." | ||
| echo "STATUS=$status" | ||
| echo "FAILED_STEPS=$*" | ||
| echo "TIMESTAMP=$(date -u +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" | ||
| } > "$PROVISION_STATUS_FILE" 2>/dev/null || true | ||
| chmod 644 "$PROVISION_STATUS_FILE" 2>/dev/null || true | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
A failed marker write leaves a stale STATUS=ok that contradicts the exit code.
mkdir -p, the redirect, and chmod all end in || true. If the redirect fails, for example on a read-only /etc or without write permission, the previous file content survives unchanged. A device that installed successfully once and then fails keeps STATUS=ok on disk while exit 1 and the [provision-status] INCOMPLETE sentinel report the failure. The three signals must agree, as stated on Lines 3383-3385.
Report the write failure so the operator sees that the marker is unreliable.
🐛 Proposed fix
{
echo "# Written by install.sh at the end of a full install. Machine-readable."
echo "STATUS=$status"
echo "FAILED_STEPS=$*"
echo "TIMESTAMP=$(date -u +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)"
- } > "$PROVISION_STATUS_FILE" 2>/dev/null || true
+ } > "$PROVISION_STATUS_FILE" 2>/dev/null || {
+ echo " Warning: could not write $PROVISION_STATUS_FILE — any existing marker there is STALE and does not describe this run (STATUS=$status)."
+ return 0
+ }
chmod 644 "$PROVISION_STATUS_FILE" 2>/dev/null || true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| write_provision_status() { | |
| local status="$1"; shift | |
| local dir | |
| dir="$(dirname "$PROVISION_STATUS_FILE")" | |
| mkdir -p "$dir" 2>/dev/null || true | |
| { | |
| echo "# Written by install.sh at the end of a full install. Machine-readable." | |
| echo "STATUS=$status" | |
| echo "FAILED_STEPS=$*" | |
| echo "TIMESTAMP=$(date -u +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" | |
| } > "$PROVISION_STATUS_FILE" 2>/dev/null || true | |
| chmod 644 "$PROVISION_STATUS_FILE" 2>/dev/null || true | |
| } | |
| write_provision_status() { | |
| local status="$1"; shift | |
| local dir | |
| dir="$(dirname "$PROVISION_STATUS_FILE")" | |
| mkdir -p "$dir" 2>/dev/null || true | |
| { | |
| echo "# Written by install.sh at the end of a full install. Machine-readable." | |
| echo "STATUS=$status" | |
| echo "FAILED_STEPS=$*" | |
| echo "TIMESTAMP=$(date -u +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" | |
| } > "$PROVISION_STATUS_FILE" 2>/dev/null || { | |
| echo " Warning: could not write $PROVISION_STATUS_FILE — any existing marker there is STALE and does not describe this run (STATUS=$status)." | |
| return 0 | |
| } | |
| chmod 644 "$PROVISION_STATUS_FILE" 2>/dev/null || true | |
| } |
🤖 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 81 - 93, Update write_provision_status so failures
creating the directory, writing PROVISION_STATUS_FILE, or applying chmod are no
longer silently ignored; report the marker-write failure to the operator while
preserving the install failure status and ensuring a stale STATUS=ok is not
treated as authoritative.
| # Shared with setup-hermes-dashboard-auth.sh: BOTH scripts read-modify-write | ||
| # ~/.hermes/config.yaml, and at install time they run seconds apart | ||
| # (production-server.js fire-and-forgets this script on the clawbox-setup | ||
| # restart in step_start_services; setup-hermes-edition.sh runs the auth script | ||
| # right after). Without a shared lock, whichever one snapshotted the file first | ||
| # and wrote last silently erased the other's block — the auth script's dashboard | ||
| # block vanished and its verify failed, blaming credentials that were correct. | ||
| # Same path derivation on both sides (HERMES_CONFIG + ".lock") so they collide. | ||
| CONFIG_LOCK="${HERMES_CONFIG}.lock" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare HERMES_CONFIG resolution across the two cooperating writers and the unit that spawns register-mcp.sh.
set -euo pipefail
rg -n -C 3 'HERMES_CONFIG' --glob 'scripts/*.sh' --glob 'install.sh' || true
# How is register-mcp.sh spawned, and with which user/HOME?
rg -n -C 6 'register-mcp' --glob '!**/node_modules/**' || true
# Service user and environment of the units involved.
fd -e service . --exec rg -n -e '^User=' -e '^Environment=' -e '^WorkingDirectory=' {} +Repository: ID-Robots/clawbox
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant files ---'
git ls-files | rg '(^|/)(register-mcp\.sh|setup-hermes-dashboard-auth\.sh|production-server\.js|install\.sh|.*\.service)$' || true
printf '%s\n' '--- HERMES_CONFIG references ---'
rg -n -C 8 'HERMES_CONFIG|register-mcp|setup-hermes-dashboard-auth' --glob 'scripts/*.sh' --glob 'install.sh' --glob '*.js' --glob '*.service' . || true
printf '%s\n' '--- service identity and environment ---'
fd -e service . --exec sh -c 'printf "\n--- %s ---\n" "$1"; rg -n -e "^(User|Group|Environment|WorkingDirectory|ExecStart)=" "$1" || true' sh {} +Repository: ID-Robots/clawbox
Length of output: 49287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- install-time home and auth invocation ---'
sed -n '1,90p' install.sh
sed -n '3070,3120p' install.sh
printf '%s\n' '--- Hermes edition variables and cooperating invocations ---'
sed -n '1,75p' scripts/setup-hermes-edition.sh
sed -n '155,195p' scripts/setup-hermes-edition.sh
printf '%s\n' '--- setup service environment and install flow references ---'
sed -n '1,45p' config/clawbox-setup.service
rg -n -C 5 'step_start_services|setup-hermes-edition|CLAWBOX_HOME|HOME=' install.sh scripts production-server.js config
printf '%s\n' '--- shell expansion probe for the exact assignments ---'
python3 - <<'PY'
from pathlib import Path
import re
register = Path("scripts/register-mcp.sh").read_text()
auth = Path("scripts/setup-hermes-dashboard-auth.sh").read_text()
service = Path("config/clawbox-setup.service").read_text()
for name, text, patterns in [
("register-mcp.sh", register, [
r'^HOME_DIR=.*$',
r'^HERMES_CONFIG=.*$',
r'^CONFIG_LOCK=.*$',
]),
("setup-hermes-dashboard-auth.sh", auth, [
r'^HERMES_CONFIG=.*$',
r'^CONFIG_LOCK=.*$',
]),
("clawbox-setup.service", service, [
r'^User=.*$',
r'^Environment=HOME=.*$',
r'^ExecStart=.*$',
]),
]:
print(f"--- {name} ---")
for pattern in patterns:
for line in text.splitlines():
if re.match(pattern, line):
print(line)
PYRepository: ID-Robots/clawbox
Length of output: 50373
🌐 Web query:
systemd.exec HOME environment variable User= system services documentation
💡 Result:
In systemd services, the HOME environment variable is controlled by the SetLoginEnvironment= directive within the unit file's [Service] section [1][2]. Key behaviors regarding the HOME environment variable: 1. Automatic Setting: By default, systemd automatically sets the $HOME, $LOGNAME, and $SHELL environment variables when User=, DynamicUser=, or PAMName= are configured for a system service [1][3][2]. 2. Configuration via SetLoginEnvironment=: You can explicitly control this behavior using the SetLoginEnvironment= boolean option [1][2]: - If set to true, $HOME, $LOGNAME, and $SHELL are set regardless of whether User=, DynamicUser=, or PAMName= are used (this includes the root user) [1][3]. - If set to false, these variables are not set by the service manager, even if User=, DynamicUser=, or PAMName= are specified [1][2]. 3. Per-User Service Manager: The SetLoginEnvironment= option generally has no effect on services run by the per-user service manager (systemd --user), as these variables are typically inherited from the user manager's own environment [1][2]. 4. User Home Directory Path: While systemd can set the $HOME environment variable to the user's home directory (as defined in the system's user database), it does not provide a direct specifier (like %h) that resolves to the home directory of the user specified by User= for use in other configuration lines (such as Environment=) [4]. The %h specifier in system unit files resolves to the home directory of the user running the service manager (typically root), not the user specified in the User= directive [4]. For services needing to store data in a user's home directory, it is often recommended to use systemd's built-in directory management directives—such as StateDirectory=, CacheDirectory=, or ConfigurationDirectory=—which are specifically designed to provide predictable paths for service data [3][5][6]. These directories are automatically created and associated with environment variables (e.g., $STATE_DIRECTORY) that point to the correct locations [3][6].
Citations:
- 1: https://freedesktop.org/software/systemd/man/latest/systemd.exec.html
- 2: https://www.man7.org/linux/man-pages/man5/systemd.exec.5.html
- 3: https://man.archlinux.org/man/systemd.exec.5
- 4: Add specifier for user home directory that respects
User=setting of unit systemd/systemd#30782 - 5: https://manpages.debian.org/unstable/systemd/systemd.exec.5.en.html
- 6: https://freedesktop.org/software/systemd/man/247/systemd.exec.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- environment-file loading and HERMES_CONFIG overrides ---'
rg -n -C 4 'source .*\.env|EnvironmentFile|dotenv|HERMES_CONFIG|HOME=' install.sh production-server.js config/clawbox-setup.service scripts/setup-hermes-edition.sh scripts/setup-hermes-dashboard-auth.sh scripts/register-mcp.sh
printf '%s\n' '--- tracked environment templates ---'
git ls-files | rg '(^|/)(\.env|.*\.env.*|environment.*)$' || true
printf '%s\n' '--- concise source facts ---'
python3 - <<'PY'
from pathlib import Path
for file, ranges in {
"config/clawbox-setup.service": [(1, 45)],
"scripts/register-mcp.sh": [(32, 58)],
"scripts/setup-hermes-edition.sh": [(10, 22), (160, 188)],
"scripts/setup-hermes-dashboard-auth.sh": [(48, 61)],
"install.sh": [(280, 305), (1318, 1331)],
}.items():
print(f"--- {file} ---")
lines = Path(file).read_text().splitlines()
for start, end in ranges:
for i in range(start, min(end, len(lines)) + 1):
print(f"{i}:{lines[i-1]}")
PY
printf '%s\n' '--- deterministic environment model ---'
python3 - <<'PY'
# Model the relevant precedence without executing repository code.
def register_path(env):
home_dir = env.get("HOME", "/home/clawbox")
return env.get("HERMES_CONFIG", f"{home_dir}/.hermes/config.yaml")
def auth_path(env):
return env.get("HERMES_CONFIG", f"{env.get('HOME', '')}/.hermes/config.yaml")
cases = [
("systemd default HOME", {"HOME": "/home/clawbox"}),
("custom HOME from .env", {"HOME": "/tmp/other"}),
("custom HERMES_CONFIG from .env", {
"HOME": "/home/clawbox",
"HERMES_CONFIG": "/tmp/other/config.yaml",
}),
]
for label, env in cases:
print(label)
print(" register:", register_path(env))
print(" auth:", auth_path(env))
print(" same:", register_path(env) == auth_path(env))
PYRepository: ID-Robots/clawbox
Length of output: 36254
Set HERMES_CONFIG explicitly after the EnvironmentFile= entries. clawbox-setup.service imports the writable .env, and register-mcp.sh honors HOME and HERMES_CONFIG from that environment. setup-hermes-edition.sh explicitly uses /home/clawbox/.hermes/config.yaml, so an override can make the boot-time writer use a different config and lock path.
🤖 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 `@scripts/register-mcp.sh` around lines 47 - 55, Set HERMES_CONFIG explicitly
to /home/clawbox/.hermes/config.yaml after the EnvironmentFile entries in
clawbox-setup.service, ensuring register-mcp.sh uses the same configuration and
lock path as setup-hermes-edition.sh regardless of imported environment
overrides.
| acquire_config_lock() { | ||
| command -v flock >/dev/null 2>&1 || { | ||
| log "flock unavailable — proceeding without the config lock" | ||
| return 0 | ||
| } | ||
| mkdir -p "$(dirname "$CONFIG_LOCK")" 2>/dev/null || true | ||
| # Probe writability in a SUBSHELL first (its redirect is scoped, so a failure | ||
| # can't abort the install and can't leak past this line). Only then open fd 9. | ||
| if ! ( : > "$CONFIG_LOCK" ) 2>/dev/null; then | ||
| log "could not create $CONFIG_LOCK — proceeding without the config lock" | ||
| return 0 | ||
| fi | ||
| # Open fd 9 for the LIFE of the script (that permanence is the point — the lock | ||
| # must outlive this function). Redirections on `exec` are permanent, so this | ||
| # line must carry NO other redirect: `exec 9>file 2>/dev/null` would silence | ||
| # the whole script's stderr, hiding every error message below. | ||
| exec 9>"$CONFIG_LOCK" | ||
| flock -w 120 9 || log "could not acquire $CONFIG_LOCK within 120s — proceeding without it" | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
One lock helper is copied into both writers. acquire_config_lock and the CONFIG_LOCK="${HERMES_CONFIG}.lock" derivation exist twice with identical bodies. The mutual exclusion holds only while both copies stay byte-identical, so any edit to one silently removes the serialisation this PR adds. Define the helper once and source it from both scripts.
scripts/setup-hermes-dashboard-auth.sh#L72-L90: moveacquire_config_lockand theCONFIG_LOCKderivation into a shared file, for examplescripts/lib/hermes-config-lock.sh, then source that file here.scripts/register-mcp.sh#L64-L79: delete the duplicated function and the duplicatedCONFIG_LOCKline, then source the same shared file.
📍 Affects 2 files
scripts/setup-hermes-dashboard-auth.sh#L72-L90(this comment)scripts/register-mcp.sh#L64-L79
🤖 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 `@scripts/setup-hermes-dashboard-auth.sh` around lines 72 - 90, Extract
acquire_config_lock and the CONFIG_LOCK derivation from
scripts/setup-hermes-dashboard-auth.sh lines 72-90 into a shared file such as
scripts/lib/hermes-config-lock.sh, then source it from that script. In
scripts/register-mcp.sh lines 64-79, remove the duplicated helper and
CONFIG_LOCK assignment and source the same shared file, preserving the existing
locking behavior in both writers.
| log "ERROR: failed to write the dashboard block to $HERMES_CONFIG" >&2 | ||
| return 1 | ||
| fi | ||
|
|
||
| # config.yaml now holds the session-signing secret and the password hash. | ||
| chmod 600 "$HERMES_CONFIG" | ||
| for bak in "$HERMES_CONFIG".bak*; do | ||
| [ -f "$bak" ] && chmod 600 "$bak" 2>/dev/null || true | ||
| done | ||
| log "wrote dashboard.basic_auth to $HERMES_CONFIG (username=$USERNAME, ttl=7d, mode 600)" | ||
|
|
||
| # Store the plaintext password for the proxy ONLY (clawbox-owned, 0600). | ||
| umask 077 | ||
| printf '%s' "$PW" > "$PWFILE" | ||
| chmod 600 "$PWFILE" | ||
| log "wrote $PWFILE (0600)" | ||
|
|
||
| # Prove the pair we just installed actually verifies, so a bug here surfaces | ||
| # now rather than as a permanent 401 loop at the proxy. | ||
| if ! creds_are_consistent; then | ||
| log "ERROR: freshly written password and hash do not verify — refusing to report success" >&2 | ||
| exit 1 | ||
| # config.yaml now holds the session-signing secret and the password hash. | ||
| chmod 600 "$HERMES_CONFIG" | ||
| for bak in "$HERMES_CONFIG".bak*; do | ||
| [ -f "$bak" ] && chmod 600 "$bak" 2>/dev/null || true | ||
| done | ||
|
|
||
| # Store the plaintext password for the proxy ONLY (clawbox-owned, 0600). | ||
| ( umask 077; printf '%s' "$pw" > "$PWFILE" ) | ||
| chmod 600 "$PWFILE" | ||
| return 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check the password-file write; an unchecked failure is reported as another process's lost write.
mint_credentials is always called as mint_credentials || mint_rc=$? on Line 366, so set -e is disabled for the whole function body. The write on Line 290 and the mkdir -p calls on Lines 179-180 therefore cannot fail the function.
If $PWFILE cannot be created, mint_credentials still returns 0. The verify on Line 375 then classifies PW_MISSING, and Lines 385-391 report "a concurrent process rewrote the file" and exit 3 with "another process keeps rewriting the file and is not honouring $CONFIG_LOCK". A local write failure is then blamed on a cooperating writer, which is the class of dishonest verdict this change removes elsewhere.
Return the write failure explicitly so the caller reports a write error.
🐛 Proposed fix: fail on a real write failure
# Store the plaintext password for the proxy ONLY (clawbox-owned, 0600).
- ( umask 077; printf '%s' "$pw" > "$PWFILE" )
- chmod 600 "$PWFILE"
+ if ! ( umask 077; printf '%s' "$pw" > "$PWFILE" ); then
+ log "ERROR: failed to write the dashboard password file $PWFILE" >&2
+ return 1
+ fi
+ chmod 600 "$PWFILE" 2>/dev/null || true
return 0Apply the same treatment to the directory creation at the top of the function:
- mkdir -p "$(dirname "$PWFILE")"
- mkdir -p "$(dirname "$HERMES_CONFIG")"
+ if ! mkdir -p "$(dirname "$PWFILE")" || ! mkdir -p "$(dirname "$HERMES_CONFIG")"; then
+ log "ERROR: could not create the directories for $PWFILE / $HERMES_CONFIG" >&2
+ return 1
+ fiAs per path instructions: "Bash scripts managing NetworkManager and iptables. Review for proper error handling, quoting, and idempotency".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log "ERROR: failed to write the dashboard block to $HERMES_CONFIG" >&2 | |
| return 1 | |
| fi | |
| # config.yaml now holds the session-signing secret and the password hash. | |
| chmod 600 "$HERMES_CONFIG" | |
| for bak in "$HERMES_CONFIG".bak*; do | |
| [ -f "$bak" ] && chmod 600 "$bak" 2>/dev/null || true | |
| done | |
| log "wrote dashboard.basic_auth to $HERMES_CONFIG (username=$USERNAME, ttl=7d, mode 600)" | |
| # Store the plaintext password for the proxy ONLY (clawbox-owned, 0600). | |
| umask 077 | |
| printf '%s' "$PW" > "$PWFILE" | |
| chmod 600 "$PWFILE" | |
| log "wrote $PWFILE (0600)" | |
| # Prove the pair we just installed actually verifies, so a bug here surfaces | |
| # now rather than as a permanent 401 loop at the proxy. | |
| if ! creds_are_consistent; then | |
| log "ERROR: freshly written password and hash do not verify — refusing to report success" >&2 | |
| exit 1 | |
| # config.yaml now holds the session-signing secret and the password hash. | |
| chmod 600 "$HERMES_CONFIG" | |
| for bak in "$HERMES_CONFIG".bak*; do | |
| [ -f "$bak" ] && chmod 600 "$bak" 2>/dev/null || true | |
| done | |
| # Store the plaintext password for the proxy ONLY (clawbox-owned, 0600). | |
| ( umask 077; printf '%s' "$pw" > "$PWFILE" ) | |
| chmod 600 "$PWFILE" | |
| return 0 | |
| log "ERROR: failed to write the dashboard block to $HERMES_CONFIG" >&2 | |
| return 1 | |
| fi | |
| # config.yaml now holds the session-signing secret and the password hash. | |
| chmod 600 "$HERMES_CONFIG" | |
| for bak in "$HERMES_CONFIG".bak*; do | |
| [ -f "$bak" ] && chmod 600 "$bak" 2>/dev/null || true | |
| done | |
| # Store the plaintext password for the proxy ONLY (clawbox-owned, 0600). | |
| if ! ( umask 077; printf '%s' "$pw" > "$PWFILE" ); then | |
| log "ERROR: failed to write the dashboard password file $PWFILE" >&2 | |
| return 1 | |
| fi | |
| chmod 600 "$PWFILE" 2>/dev/null || true | |
| return 0 |
🤖 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 `@scripts/setup-hermes-dashboard-auth.sh` around lines 279 - 292, Update
mint_credentials to explicitly check and return failures from the PWFILE write
and the mkdir -p calls near the start of the function. Ensure each failed
filesystem operation returns a nonzero status so the caller reports a direct
write or directory-creation error instead of classifying it as a concurrent
rewrite.
Source: Path instructions
| it("both take the lock before they touch config.yaml", () => { | ||
| // The auth script acquires before its mint; the registrar acquires before | ||
| // its PyYAML reconcile AND holds it across the `hermes tools disable` CLI | ||
| // call (which does its own wide load->save_config on the same file). | ||
| expect(AUTH_SRC).toMatch(/acquire_config_lock[\s\S]*mint_credentials/); | ||
| const regTail = REGISTER_SRC.slice(REGISTER_SRC.indexOf("acquire_config_lock\n\nexport")); | ||
| expect(regTail).toContain("acquire_config_lock"); | ||
| expect(REGISTER_SRC.indexOf("acquire_config_lock\n\nexport")).toBeLessThan( | ||
| REGISTER_SRC.indexOf("tools disable browser"), | ||
| ); | ||
| expect(AUTH_SRC).toContain("flock -w 120 9"); | ||
| expect(REGISTER_SRC).toContain("flock -w 120 9"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unguarded indexOf markers make these assertions pass without checking anything. Both tests slice the shell source at a string marker and never verify that the marker was found. When indexOf returns -1, the derived offset still compares or slices successfully, so the test stays green while the contract it protects is gone. Assert that each marker index is greater than -1 before you use it.
src/tests/unit/hermes-config-lock.test.ts#L41-L53: captureREGISTER_SRC.indexOf("acquire_config_lock\n\nexport")in a variable, assert it is greater than -1, then use it for the slice and the ordering comparison. Prefer a regex anchored on the call itself over the trailingexportliteral.src/tests/unit/install-hermes-edition-step.test.ts#L170-L176: captureprobe.indexOf("case"), assert it is greater than -1, then slice with it.
📍 Affects 2 files
src/tests/unit/hermes-config-lock.test.ts#L41-L53(this comment)src/tests/unit/install-hermes-edition-step.test.ts#L170-L176
🤖 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 `@src/tests/unit/hermes-config-lock.test.ts` around lines 41 - 53, Guard all
source-marker lookups before slicing or comparing. In
src/tests/unit/hermes-config-lock.test.ts lines 41-53, capture the REGISTER_SRC
marker index, assert it is greater than -1, prefer a regex anchored to the
acquire_config_lock call, then reuse the validated index for slicing and
ordering checks. In src/tests/unit/install-hermes-edition-step.test.ts lines
170-176, capture probe.indexOf("case"), assert it is greater than -1, and only
then slice with that index.
| const script = ` | ||
| set -e | ||
| LOCK="${lockFile}" | ||
| ( exec 9>"$LOCK"; flock 9; sleep 0.8 ) & | ||
| hold=$! | ||
| sleep 0.15 # ensure the holder has the lock first | ||
| start=$(date +%s.%N) | ||
| CLAWBOX_ROOT="${root}" HERMES_CONFIG="${configPath}" bash "${AUTH}" >/dev/null 2>&1 | ||
| end=$(date +%s.%N) | ||
| wait $hold | ||
| awk -v s="$start" -v e="$end" 'BEGIN{printf "%.3f", e - s}' | ||
| `; | ||
| const proc = spawnSync("bash", ["-c", script], { encoding: "utf-8", timeout: 20000 }); | ||
| const elapsed = parseFloat(proc.stdout.trim() || "0"); | ||
| // Held ~0.8s, started ~0.15s in, so the auth script should wait ≳0.5s. | ||
| expect(elapsed).toBeGreaterThan(0.5); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
date +%s.%N is not portable; this test fails on macOS while the lock works.
BSD date on macOS does not support %N. It prints a literal N, so awk subtracts two equal integer second values and elapsed becomes 0. The assertion on Line 111 then fails on a machine where the lock behaves correctly. RUNNABLE excludes only win32, so darwin reaches this test.
Measure the elapsed time in the test process instead of in bash. That also removes the dependency on GNU date.
🐛 Proposed fix: time the blocking run from Node
const script = `
set -e
LOCK="${lockFile}"
( exec 9>"$LOCK"; flock 9; sleep 0.8 ) &
hold=$!
sleep 0.15 # ensure the holder has the lock first
- start=$(date +%s.%N)
CLAWBOX_ROOT="${root}" HERMES_CONFIG="${configPath}" bash "${AUTH}" >/dev/null 2>&1
- end=$(date +%s.%N)
wait $hold
- awk -v s="$start" -v e="$end" 'BEGIN{printf "%.3f", e - s}'
`;
- const proc = spawnSync("bash", ["-c", script], { encoding: "utf-8", timeout: 20000 });
- const elapsed = parseFloat(proc.stdout.trim() || "0");
+ const startedAt = Date.now();
+ spawnSync("bash", ["-c", script], { encoding: "utf-8", timeout: 20000 });
+ const elapsed = (Date.now() - startedAt) / 1000;
// Held ~0.8s, started ~0.15s in, so the auth script should wait ≳0.5s.
expect(elapsed).toBeGreaterThan(0.5);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const script = ` | |
| set -e | |
| LOCK="${lockFile}" | |
| ( exec 9>"$LOCK"; flock 9; sleep 0.8 ) & | |
| hold=$! | |
| sleep 0.15 # ensure the holder has the lock first | |
| start=$(date +%s.%N) | |
| CLAWBOX_ROOT="${root}" HERMES_CONFIG="${configPath}" bash "${AUTH}" >/dev/null 2>&1 | |
| end=$(date +%s.%N) | |
| wait $hold | |
| awk -v s="$start" -v e="$end" 'BEGIN{printf "%.3f", e - s}' | |
| `; | |
| const proc = spawnSync("bash", ["-c", script], { encoding: "utf-8", timeout: 20000 }); | |
| const elapsed = parseFloat(proc.stdout.trim() || "0"); | |
| // Held ~0.8s, started ~0.15s in, so the auth script should wait ≳0.5s. | |
| expect(elapsed).toBeGreaterThan(0.5); | |
| const script = ` | |
| set -e | |
| LOCK="${lockFile}" | |
| ( exec 9>"$LOCK"; flock 9; sleep 0.8 ) & | |
| hold=$! | |
| sleep 0.15 # ensure the holder has the lock first | |
| CLAWBOX_ROOT="${root}" HERMES_CONFIG="${configPath}" bash "${AUTH}" >/dev/null 2>&1 | |
| wait $hold | |
| `; | |
| const startedAt = Date.now(); | |
| spawnSync("bash", ["-c", script], { encoding: "utf-8", timeout: 20000 }); | |
| const elapsed = (Date.now() - startedAt) / 1000; | |
| // Held ~0.8s, started ~0.15s in, so the auth script should wait ≳0.5s. | |
| expect(elapsed).toBeGreaterThan(0.5); |
🤖 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 `@src/tests/unit/hermes-config-lock.test.ts` around lines 96 - 111, Update the
lock timing test around the shell script and spawnSync call to measure the
blocking AUTH execution with Node’s timing API rather than bash date arithmetic.
Record the start time immediately before spawnSync and compute elapsed after it
returns, while preserving the existing lock-holder setup and
greater-than-0.5-second assertion.
| function scryptHash(plaintext: string): string { | ||
| const proc = spawnSync( | ||
| "python3", | ||
| [ | ||
| "-c", | ||
| [ | ||
| "import base64,hashlib,sys", | ||
| "pw=sys.argv[1].encode(); salt=bytes(range(16))", | ||
| "dk=hashlib.scrypt(pw,salt=salt,n=2**14,r=8,p=1,dklen=32,maxmem=0)", | ||
| "print('scrypt$%d$%d$%d$%s$%s'%(2**14,8,1,base64.b64encode(salt).decode(),base64.b64encode(dk).decode()))", | ||
| ].join("\n"), | ||
| plaintext, | ||
| ], | ||
| { encoding: "utf-8" }, | ||
| ); | ||
| return proc.stdout.trim(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the fixture generator succeeded.
scryptHash returns proc.stdout.trim() without inspecting proc.status. If python3 fails, the helper returns an empty string, seedBlock writes password_hash: "", and the classifier answers 4 (NOT_CONFIGURED). The mismatch test on Line 256 then fails with a message that points at the classifier instead of at the broken fixture.
♻️ Proposed fix
{ encoding: "utf-8" },
);
- return proc.stdout.trim();
+ if (proc.status !== 0) {
+ throw new Error(`scryptHash fixture failed (status ${proc.status}): ${proc.stderr}`);
+ }
+ const hash = proc.stdout.trim();
+ if (!hash.startsWith("scrypt$")) throw new Error(`scryptHash produced: ${hash}`);
+ return hash;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function scryptHash(plaintext: string): string { | |
| const proc = spawnSync( | |
| "python3", | |
| [ | |
| "-c", | |
| [ | |
| "import base64,hashlib,sys", | |
| "pw=sys.argv[1].encode(); salt=bytes(range(16))", | |
| "dk=hashlib.scrypt(pw,salt=salt,n=2**14,r=8,p=1,dklen=32,maxmem=0)", | |
| "print('scrypt$%d$%d$%d$%s$%s'%(2**14,8,1,base64.b64encode(salt).decode(),base64.b64encode(dk).decode()))", | |
| ].join("\n"), | |
| plaintext, | |
| ], | |
| { encoding: "utf-8" }, | |
| ); | |
| return proc.stdout.trim(); | |
| } | |
| function scryptHash(plaintext: string): string { | |
| const proc = spawnSync( | |
| "python3", | |
| [ | |
| "-c", | |
| [ | |
| "import base64,hashlib,sys", | |
| "pw=sys.argv[1].encode(); salt=bytes(range(16))", | |
| "dk=hashlib.scrypt(pw,salt=salt,n=2**14,r=8,p=1,dklen=32,maxmem=0)", | |
| "print('scrypt$%d$%d$%d$%s$%s'%(2**14,8,1,base64.b64encode(salt).decode(),base64.b64encode(dk).decode()))", | |
| ].join("\n"), | |
| plaintext, | |
| ], | |
| { encoding: "utf-8" }, | |
| ); | |
| if (proc.status !== 0) { | |
| throw new Error(`scryptHash fixture failed (status ${proc.status}): ${proc.stderr}`); | |
| } | |
| const hash = proc.stdout.trim(); | |
| if (!hash.startsWith("scrypt$")) throw new Error(`scryptHash produced: ${hash}`); | |
| return hash; | |
| } |
🤖 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 `@src/tests/unit/hermes-dashboard-auth-yaml.test.ts` around lines 77 - 93,
Update scryptHash to validate the spawnSync result before returning stdout:
assert or throw when proc.status indicates python3 failed, including stderr or
another useful failure detail, then return the trimmed stdout only on success.
Keep the existing fixture-generation output unchanged for successful executions.
|
Worked through all seven review comments. Six applied, one declined with a reason — plus a root-cause gap the review did not raise that I think was the important one. The gap: the race was closed for cooperating writers only
Reproduced deterministically (shim a Three things followed:
Applied
DeclinedSetting The underlying concern is real but narrower than stated. The two writers only need to agree when they are writing the same file — if TestsFull unit suite green: 2437 passing across 176 files (one worker-startup timeout under WSL, confirmed passing standalone). New coverage: the lock is defined once and both writers source it; one lock file from a symlinked/dotted path; an unserialised run does not exit 0; the block being erased right after the verify never yields success; an unwritable password file is not blamed on another writer; a marker that cannot be rewritten is removed rather than left stale; config writes are locked and reads are not. On-device confirmation on the Hermes box is still owed — that box is disconnected at the moment. |
|
Heads-up on timing: this merged at
It is a clean descendant of the merged head and merges into current beta ( I have not opened a new PR for it and have not pushed to beta — say which you'd prefer and I'll land it. |
|
One more commit on the branch — a regression I introduced and caught before it could ship: Deriving the lock path called Branch is now 2 commits ahead of beta, still conflict-free. Full unit suite: 2448 passing across 177 files. |
1 participant
Why
A Hermes provision could report itself broken and still be indistinguishable from a healthy one. Two real provisions on different boards printed
dashboard auth setup returned non-zeroandHermes is not fully provisioned, thenAll 20 checks healthy, then the flash host printedSetup: 1/1 succeeded. The credentials written were always correct — the check failed at install time.The cause is a lost update on
~/.hermes/config.yaml, which has several writers, each doing an unlocked read-modify-write.install.sh'sstep_start_servicesrestartsclawbox-setup;production-server.jsfire-and-forgetsregister-mcp.sh; thirteen lines laterstep_hermes_editionruns the dashboard-auth script. The auth script's critical section is write config → write password file → re-read to verify. A writer that snapshotted before the write and lands itsos.replaceinside that window erases the dashboard block, and the verify then reports a credential fault for credentials that are provably correct.What
Serialise the writers. One
flockover the config, defined once inscripts/lib/hermes-config-lock.shand sourced by both shell writers. The lock path is derived from the config path after canonicalising it, so two spellings of one file (a symlinked home, a..) cannot produce two lock files and quietly stop excluding each other.A lock rather than ordering the install steps, because ordering fixes install time only: the same two writers race again on every boot from two independent units (
clawbox-setupandclawbox-hermes-dashboard), where there is no single caller to order. The Hermes CLI is a writer we do not control and cannot ask to cooperate — it is covered by never being allowed to run outside a critical section:register-mcp.shholds the lock across its invocation, and the Settings routes'hermes config set/unsetnow run under the same lock viaflock(1), applied at the one chokepoint (runHermesCli) so all five call sites are covered. Read-only calls stay unlocked; a lock conflict is a bounded 30s wait and a distinct exit code, so "the device is busy" is not reported as "the command failed".Both phases of the race are closed. Phase one — a writer landing between the write and the verify — makes the verify fail loudly, and is retried. Phase two is the dangerous half: a writer landing after the verify left the script printing
doneand exiting 0 over a config with no dashboard block at all, a clean success reported over a box with no auth provider. Holding the lock until exit closes it for cooperating writers; a run that could not take the lock at all now exits 8 rather than reporting success. The work is still done first, so the dashboard keeps its auth provider and only the certification is withheld.Honest failure classes (
scripts/setup-hermes-dashboard-auth.sh). The old check returned a bare exit 1 for a genuine mismatch, a config another writer had rewritten, an unreadable file, and an interpreter withoutscryptalike — and the message blamed the credentials. Each is now its own outcome with its own message and exit code: "could not run the check", "the block is missing", "the hash does not match the password", "verified but not serialised". Local write failures are reported as local write failures, not blamed on a competing writer. The classifier is exposed as a read-only--checkreused by validation, so the validator and the provisioner agree on what healthy means.Failures propagate (
install.sh). A non-fatalstep_hermes_editionfailure never reached the summary. The step stays non-fatal (a half-provisioned box should still come up reachable), but the failure is recorded, the summary printsPROVISIONING INCOMPLETE, aprovision-statusmarker is written, and the script exits non-zero — so the flash host cannot report success over it.step_validate_servicesnow verifies the auth provider directly instead of accepting a probe that answers the same whether or not the provider works. The exit code, the[provision-status]line and the marker file are kept in agreement: a marker that cannot be rewritten is removed rather than left behind asserting a staleSTATUS=ok.Tests
Full unit suite green: 2437 passing across 176 files.
Phase two was reproduced deterministically before the fix — shim a
flockthat cannot acquire, and fire one competing read-modify-write the instant the verify returns: 8/8 trials exited 0 with no dashboard block on disk. After the fix: 0/8, all honest non-zero.New regression coverage: the lock is defined once and both writers source it; one lock file from a symlinked or dotted path; a held lock is waited on rather than raced through; an unserialised run does not exit 0; the block being erased right after the verify never yields success; a compute failure is not reported as a mismatch; a genuine mismatch is still caught; a vanished block is classified as not-configured rather than a credential fault; an unwritable password file is not blamed on another writer; a failed provisioning step does not produce a success summary; a marker that cannot be rewritten is not left stale; config writes are locked and reads are not.
Two bash behaviours were verified against the shell rather than assumed, and both changed the implementation: a redirection failure on a compound command does not propagate through
if ! { ...; } > file(so that guard would have been inert), andflock -w -Ereturns the chosen code only on timeout, the command's own status otherwise.Follow-up
On-device confirmation on the Hermes box is still owed (
sudo bash ~/clawbox/install.sh --step hermes_editionplus the--checkprobe); that box is disconnected at the moment.