fix: keep the provisioning marker tied to the run that wrote it - #388
fix: keep the provisioning marker tied to the run that wrote it#388KrasimirKralev wants to merge 1 commit into
Conversation
The marker file is the channel the flash host reads instead of parsing install.sh's stdout, so a run that could not write it left the previous run's STATUS=ok in place and that stale success was read as the current verdict. Every write in the helper ended in `|| true` with stderr discarded, so it happened without a word. The marker is now removed before provisioning starts — its presence at the end means this run wrote it — and the final record is written to a temp file and renamed, so a reader never sees a partial one. A marker that cannot be cleared or written is reported on stdout and turns the run's verdict into "incomplete" on the exit code and the sentinel line, because a result that cannot be published is not one to ship. The record also carries a RUN_ID that stdout repeats, for a reader that can correlate the two. The Hermes dashboard password file gets the same temp-file-and-rename treatment the config write already used: the previous in-place truncate let the proxy read an empty password mid-write, and a crash in between left it empty for good. The lost-write retry loop now backs off between attempts. It only runs while the competing writer is still inside its own read-modify-write window, so retrying immediately lost the same race again and all three attempts could finish before the other writer landed anything. Tests: the shell-source suites no longer assert over slices that degenerate silently — the extractor refuses a truncated function body when a caller makes a negative assertion, and the lock-ordering check uses anchored regexes and proves each marker was found before comparing their positions. The lock-wait test measures elapsed time in the test process against an uncontended baseline instead of `date +%s.%N`, which is a GNU extension the suite ran on macOS too.
📝 WalkthroughWalkthroughThe installer now publishes run-scoped provisioning markers atomically and reports publication failures. Dashboard credentials use secure atomic replacement with retry backoff. Tests cover marker lifecycle, concurrent writes, lock contention, and complete shell-function extraction. ChangesProvisioning and dashboard authentication
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
🦀 ClawReviewYour friendly reef crab, here with the lay of the land. Five targeted fixes to provisioning reliability, all follow-ups to the dashboard-auth work in #386/#387. The core change closes a false-healthy hole: the provisioning marker now removes any stale predecessor before the run starts and uses a temp-file-plus-rename so the flash host always sees a complete record from the current run — or no record at all. The same atomic-write treatment is applied to the dashboard password file, retry backoff is added to the lost-write path, and two test suites are hardened against slice-truncation bugs that let negative assertions pass silently. At a glance
Good to know
— ClawReview 🦀. I set the scene; CodeRabbit reviews the code; you decide. 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: 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 `@install.sh`:
- Around line 104-117: Update the provisioning status reader and writer contract
so a marker is valid only when it contains the current run’s expected RUN_ID,
preventing an uncleared prior STATUS=ok from being consumed. Integrate this
validation with invalidate_provision_status and the final replacement path,
preserving failure reporting when deletion or replacement fails. Add coverage
for a stale regular marker where both deletion and replacement fail, verifying
readers reject it as the current run’s verdict.
In `@src/tests/unit/hermes-config-lock.test.ts`:
- Around line 113-125: Update timeUncontendedRun to clean up the baseRoot
temporary directory in a finally block surrounding the spawnSync call and status
assertion. Use the existing filesystem cleanup APIs so cleanup runs both when
the process succeeds and when the assertion fails.
🪄 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: b2ff5076-a3c3-4459-a6dc-bcedc393d686
📒 Files selected for processing (6)
install.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.tssrc/tests/unit/install-provision-status-marker.test.ts
| # Drop any marker left behind by an earlier run. Called once, before the first | ||
| # provisioning step of a full install. | ||
| invalidate_provision_status() { | ||
| rm -f "$PROVISION_STATUS_FILE" 2>/dev/null || true | ||
| # `rm -f` reports success for an already-absent file and failure for one it | ||
| # could not remove, so test the outcome rather than its exit status. | ||
| if [ -e "$PROVISION_STATUS_FILE" ]; then | ||
| PROVISION_STATUS_UNPUBLISHED=1 | ||
| echo " WARNING: could not clear the previous provisioning marker" | ||
| echo " $PROVISION_STATUS_FILE — its contents describe an EARLIER" | ||
| echo " run and must not be read as this one's verdict." | ||
| return 1 | ||
| fi | ||
| return 0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not let an uncleared marker remain a valid verdict.
If rm -f cannot remove a prior regular marker, the code only sets PROVISION_STATUS_UNPUBLISHED and continues. If the later rename also fails, the prior STATUS=ok remains at $PROVISION_STATUS_FILE. The flash host reads that file instead of stdout, so the final INCOMPLETE output does not prevent a stale success from being consumed.
Change the marker-reader contract to require the expected current RUN_ID, or use another authoritative invalidation channel that the reader checks. Add a test with a stale regular marker where both deletion and replacement fail.
Based on learnings: “if provisioning cannot write its final status marker, readers must not interpret a previous successful marker as the current run's verdict.”
Also applies to: 3307-3312, 3474-3496
🤖 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 104 - 117, Update the provisioning status reader and
writer contract so a marker is valid only when it contains the current run’s
expected RUN_ID, preventing an uncleared prior STATUS=ok from being consumed.
Integrate this validation with invalidate_provision_status and the final
replacement path, preserving failure reporting when deletion or replacement
fails. Add coverage for a stale regular marker where both deletion and
replacement fail, verifying readers reject it as the current run’s verdict.
Source: Learnings
| const baseRoot = fs.mkdtempSync(path.join(os.tmpdir(), "clawbox-lockbase-")); | ||
| const baseConfig = path.join(baseRoot, "hermes", "config.yaml"); | ||
| fs.mkdirSync(path.dirname(baseConfig), { recursive: true }); | ||
| fs.writeFileSync(baseConfig, "mcp_servers:\n clawbox:\n enabled: true\n"); | ||
| const baseStart = Date.now(); | ||
| const baseProc = spawnSync("bash", [AUTH], { | ||
| encoding: "utf-8", | ||
| timeout: 20000, | ||
| env: { ...process.env, CLAWBOX_ROOT: baseRoot, HERMES_CONFIG: baseConfig }, | ||
| }); | ||
| const ms = Date.now() - baseStart; | ||
| expect(baseProc.status, baseProc.stderr).toBe(0); | ||
| return ms; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove each baseline temporary directory.
Each call to timeUncontendedRun creates baseRoot and leaves it in the system temporary directory. Remove it in a finally block after spawnSync completes. This also cleans up when the status assertion fails.
Proposed fix
const timeUncontendedRun = () => {
const baseRoot = fs.mkdtempSync(path.join(os.tmpdir(), "clawbox-lockbase-"));
- const baseConfig = path.join(baseRoot, "hermes", "config.yaml");
- fs.mkdirSync(path.dirname(baseConfig), { recursive: true });
- fs.writeFileSync(baseConfig, "mcp_servers:\n clawbox:\n enabled: true\n");
- const baseStart = Date.now();
- const baseProc = spawnSync("bash", [AUTH], {
- encoding: "utf-8",
- timeout: 20000,
- env: { ...process.env, CLAWBOX_ROOT: baseRoot, HERMES_CONFIG: baseConfig },
- });
- const ms = Date.now() - baseStart;
- expect(baseProc.status, baseProc.stderr).toBe(0);
- return ms;
+ try {
+ const baseConfig = path.join(baseRoot, "hermes", "config.yaml");
+ fs.mkdirSync(path.dirname(baseConfig), { recursive: true });
+ fs.writeFileSync(baseConfig, "mcp_servers:\n clawbox:\n enabled: true\n");
+ const baseStart = Date.now();
+ const baseProc = spawnSync("bash", [AUTH], {
+ encoding: "utf-8",
+ timeout: 20000,
+ env: { ...process.env, CLAWBOX_ROOT: baseRoot, HERMES_CONFIG: baseConfig },
+ });
+ const ms = Date.now() - baseStart;
+ expect(baseProc.status, baseProc.stderr).toBe(0);
+ return ms;
+ } finally {
+ fs.rmSync(baseRoot, { recursive: true, force: 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.
| const baseRoot = fs.mkdtempSync(path.join(os.tmpdir(), "clawbox-lockbase-")); | |
| const baseConfig = path.join(baseRoot, "hermes", "config.yaml"); | |
| fs.mkdirSync(path.dirname(baseConfig), { recursive: true }); | |
| fs.writeFileSync(baseConfig, "mcp_servers:\n clawbox:\n enabled: true\n"); | |
| const baseStart = Date.now(); | |
| const baseProc = spawnSync("bash", [AUTH], { | |
| encoding: "utf-8", | |
| timeout: 20000, | |
| env: { ...process.env, CLAWBOX_ROOT: baseRoot, HERMES_CONFIG: baseConfig }, | |
| }); | |
| const ms = Date.now() - baseStart; | |
| expect(baseProc.status, baseProc.stderr).toBe(0); | |
| return ms; | |
| const baseRoot = fs.mkdtempSync(path.join(os.tmpdir(), "clawbox-lockbase-")); | |
| try { | |
| const baseConfig = path.join(baseRoot, "hermes", "config.yaml"); | |
| fs.mkdirSync(path.dirname(baseConfig), { recursive: true }); | |
| fs.writeFileSync(baseConfig, "mcp_servers:\n clawbox:\n enabled: true\n"); | |
| const baseStart = Date.now(); | |
| const baseProc = spawnSync("bash", [AUTH], { | |
| encoding: "utf-8", | |
| timeout: 20000, | |
| env: { ...process.env, CLAWBOX_ROOT: baseRoot, HERMES_CONFIG: baseConfig }, | |
| }); | |
| const ms = Date.now() - baseStart; | |
| expect(baseProc.status, baseProc.stderr).toBe(0); | |
| return ms; | |
| } finally { | |
| fs.rmSync(baseRoot, { recursive: true, force: true }); | |
| } |
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 115-115: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(baseConfig, "mcp_servers:\n clawbox:\n enabled: true\n")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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 113 - 125, Update
timeUncontendedRun to clean up the baseRoot temporary directory in a finally
block surrounding the spawnSync call and status assertion. Use the existing
filesystem cleanup APIs so cleanup runs both when the process succeeds and when
the assertion fails.
1 participant
Follow-up to #386 / #387: the review of the merged dashboard-auth work found
five things, and the first one reopened the hole the change was written to
close. Devices are being prepared for shipping, so this is deliberately narrow —
five findings, nothing else.
1. The provisioning marker could speak for an earlier run
write_provision_statusended every operation in|| trueand discardedstderr. If
/etc/clawboxwas read-only, the file was owned by someone else, orthe disk was full, it returned 0 having written nothing — and the flash host,
which reads this file instead of parsing stdout, was left holding the previous
run's
STATUS=ok. A failing run therefore handed back a stale success, which isexactly the false-healthy outcome the marker exists to prevent.
What this does instead: the marker is removed before provisioning starts, so
its presence at the end means this run wrote it; the final record goes to a temp
file and is renamed into place, so a reader never sees a partial one; and if
either step fails the run says so on stdout and its verdict becomes
incompleteon the exit code and the
[provision-status]sentinel — a result that cannot bepublished is not one to ship. The record also carries a
RUN_IDthat stdoutrepeats on a
[provision-run]line, so a reader that can correlate the two getsa second, independent check.
The
[provision-status] OK/INCOMPLETElines keep their exact wording — theyare a stdout contract with the flash host, and the run id went on its own line
rather than onto the end of theirs.
--stepmode does not clear the marker: a single-step re-run is not aprovisioning run and must not destroy the last full install's verdict.
2. The dashboard password file was truncated in place
The config rewrite used a temp file and a rename; the password file did not.
printf '%s' "$pw" > "$PWFILE"truncates before it writes, so the proxy — whichre-reads that file on every session renewal — could observe an empty password
and answer 401, and a crash between the truncate and the write left it empty
permanently. It now uses the same temp-file-and-rename shape as the config, in
the same directory, created 0600 by umask, with each step's failure reported as
a write failure rather than as a credential verdict.
3. Lost-write retries had no backoff
That branch only runs when the lock did not hold the two writers apart — which
is precisely while the other writer is inside its own read-modify-write window.
Retrying immediately loses the same race again, and all three attempts could
finish before the competing writer landed anything. Now 1s, then 2s.
4 & 5. Two suites that could stop testing without saying so
Both asserted over slices of shell source located by
indexOfor a braceheuristic. When a marker moved the slice degenerated and the assertion silently
stopped checking what it named — worst for the negative ones, where a truncated
slice makes
not.toContainpass for the wrong reason.negative assertion, with a message naming the cause.
before comparing positions, and pins the lock as taken before both of the
registrar's writes (before-the-CLI-call alone was satisfied by taking it one
line above, leaving the PyYAML reconcile unprotected).
uncontended baseline, rather than
date +%s.%N—%Nis a GNU extension thatBSD/macOS
dateemits literally, and the suite gates only onplatform !== "win32". It also waits for proof that the holder owns the lockinstead of sleeping a guessed interval.
Verification
One regression test per finding, and each was run against the pre-fix code to
confirm it fails there:
[provision-status] OK+ exit 0 with an unwritable marker path}at column 0 inside the function truncated the slice and the negative assertion passedReproduced in a WSL Ubuntu 22.04 / Python 3.10 sandbox rather than on hardware —
there is no spare Hermes device, and the one that exists is being reset for
shipping.
Unit suite: 201 files / 2575 tests passing, against a beta baseline of
200 / 2561. Lint clean on the touched files;
tscerror count on the repo wentfrom 9 lines to 7 (the
run2helper's env parameter is now typed as a plainrecord, which also removes a pre-existing pair of errors).
The two timing-sensitive tests were run 15 times consecutively without a
failure, including from cold.
Deliberately not in this PR
Three other comments on #387 that are not in the five:
install.sh:77(propagating non-fatal failures out of
step_post_update/step_rebuild_rebootin
--stepmode), and the two remaininghermes-dashboard-auth-yamlsuggestions (
scryptHashshould assert the fixture generator succeeded, andrun2should be folded intorun). None of them affect the marker's integrity;they are worth a separate pass once devices are out the door.
Summary by CodeRabbit
Bug Fixes
Reliability