Add portable dashboard lifecycle commands#75
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe packaged CLI adds detached lifecycle commands with protected configuration, locking, ownership checks, health readiness, recovery, and process cleanup. Doctor diagnostics can validate and probe URLs through machine-local network interfaces. ChangesPortable dashboard lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Operator
participant LifecycleCLI
participant LifecycleChild
participant Dashboard
Operator->>LifecycleCLI: start
LifecycleCLI->>LifecycleChild: spawn detached instance
LifecycleChild->>Dashboard: launch server
LifecycleCLI->>Dashboard: poll /api/health
Dashboard-->>LifecycleCLI: readiness and coordination status
LifecycleCLI-->>Operator: lifecycle result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c62e0d3df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review: Portable dashboard lifecycle commandsOverviewThis PR adds Code quality
Issues found (posted inline)
Other observations (not blocking)
Overall this is a thorough, well-tested addition; the one substantive finding is the env-file TOCTOU race, which is worth fixing given the explicit anti-symlink security intent of that check. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@bin/agent-coordination-dashboard.js`:
- Around line 100-109: Update parseLocalInterfaceDashboardUrl() so valid HTTP
URLs explicitly using the default port :80 are accepted. Adjust the
normalizedRaw versus parsed.origin validation to canonicalize or otherwise
account for the default HTTP port, while preserving rejection of non-root paths,
credentials, query strings, fragments, and unsupported protocols.
In `@bin/lifecycle.js`:
- Around line 481-486: Update terminateDetachedGroup to retain and propagate the
result of the final waitForProcessExit call after SIGKILL. Ensure callers such
as stopDashboard do not delete runtime.json or report success when the process
remains alive, preserving runtime metadata for subsequent lifecycle commands.
- Around line 266-290: Update readProtectedEnv to open envFile once with a
no-follow read-only descriptor using O_RDONLY | O_NOFOLLOW, then perform
ownership/mode/regular-file validation via fstat on that descriptor and read
contents from the same handle. Remove the separate lstat/readFile path while
preserving the existing optional-file behavior, validation errors, parsing, and
reliable descriptor cleanup.
In `@scripts/lifecycle.test.ts`:
- Around line 507-569: The simultaneous-start test currently always expects a
fake ps failure, but Linux resolves process identity through /proc instead.
Separate the cross-platform concurrency assertions from the injected
process-identity failure scenario in the test, and only run the fake ps failure
setup and psFailureLog assertion on platforms that use the ps fallback (or
provide a deterministic fallback seam).
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56dabc4a-1d5d-44cc-9b4c-45178a1bdd1a
📒 Files selected for processing (5)
README.mdbin/agent-coordination-dashboard.jsbin/lifecycle.jsscripts/cli.test.tsscripts/lifecycle.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ed4c8eaae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review summaryReviewed the diff ( OverviewAdds a tmux-free, detached lifecycle CLI (
Code quality
Other observations (no action required)
Security
Overall this is a well-engineered, heavily-tested feature. The two inline points are minor robustness/maintainability suggestions rather than blocking issues. |
Code ReviewOverviewThis PR adds a portable, tmux-free lifecycle CLI ( Code qualityThe lifecycle module is unusually careful for process-management code:
Overall the design matches what's described in the PR body (generation-specific lock records, bounded identity retries, fail-closed behavior), and the test suite exercises the gnarly cases well: concurrent starts, killed wrapper with orphaned server group, Suggestions (posted inline)
Minor / non-blocking observations
SecurityToken handling (excluded from argv/metadata/logs, explicit clearing of API env vars before file values are applied), env-file protections (0600, owned, non-symlink via Test coverageVery strong — concurrency, recovery from a killed wrapper, lock staleness/reuse, restart-config-validation-before-stop, token rotation, and degraded-backend reporting are all covered. One small gap: no test exercises the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffde9ec3e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b46ed8e42c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review: Add portable dashboard lifecycle commands (#75) Overview Strengths
Issues found (posted inline)
Other observations (not blocking)
Security |
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 `@bin/lifecycle.js`:
- Around line 668-672: Update the ALLOWED_HOSTS validation near bindHost to
parse each comma-separated entry, trim it, and require every non-empty entry to
be an explicit valid browser hostname or IP address. Reject catch-all values
such as "*" and treat missing or invalid entries as disallowed when bindHost is
"0.0.0.0" or "::"; preserve the existing error behavior for wildcard bindings.
- Around line 481-499: The health probe in the non-loopback branch must bind its
source connection to the configured target host instead of forcing loopback
addresses. Update the localAddress calculation used by httpRequest to use
targetUrl.hostname, while preserving the existing loopback-derived Host header
construction unchanged.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f530089-7379-4f27-8a26-c42d2a23f126
📒 Files selected for processing (5)
README.mdbin/agent-coordination-dashboard.jsbin/lifecycle.jsscripts/cli.test.tsscripts/lifecycle.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- bin/agent-coordination-dashboard.js
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6234131954
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review summaryThis is a large, exceptionally well-hardened PR (visible from the ~30 iterative "harden lifecycle X" commits) that adds detached Strengths:
Findings posted inline (roughly in order of what I'd want addressed before merge):
Nothing found rises to a hard security bypass or data-corruption bug — the codebase's existing hardening already closes the classic TOCTOU/race patterns I looked for. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fcb383843
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review summaryThis is a large, well-engineered PR (1663-line new I verified the write-access boundary changes in I found no critical bugs. A few concrete issues worth addressing, posted as inline comments:
One more inconsistency that isn't part of this diff so I couldn't attach it inline: Performance: no new synchronous/blocking work was added to hot GET request paths; the machine-local check is only invoked on write routes. The lifecycle log writer's Security: the Overall: solid work, small polish items above rather than blockers. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39793c603c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10b217af5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address-review summaryScan scope: full PR history through exact head Mattered
Optional
Skipped
Next default scan starts after this comment. Say |
|
|
||
| function resetLifecycleLogWithTail(fd, chunk, maxBytes) { | ||
| const marker = Buffer.from(LIFECYCLE_LOG_ROLLOVER_MARKER); | ||
| const boundedMarker = marker.subarray(0, Math.min(marker.length, maxBytes)); | ||
| const tailCapacity = maxBytes - boundedMarker.length; | ||
| const tail = chunk.subarray(Math.max(0, chunk.length - tailCapacity)); | ||
| // Lifecycle startup opens this shared inherited descriptor with O_APPEND. | ||
| // Reset before writing the newest tail so the stale shared offset cannot | ||
| // create a sparse hole and the file never exceeds the cap at write return. | ||
| fsFtruncateSync(fd, 0); | ||
| writeAllToLifecycleLog(fd, boundedMarker); | ||
| writeAllToLifecycleLog(fd, tail); | ||
| } | ||
|
|
||
| function writeBoundedLifecycleLogChunk(fd, chunk, maxBytes) { | ||
| const stats = validateLifecycleLogStats(fsFstatSync(fd)); | ||
| if (stats.size + chunk.length <= maxBytes) { | ||
| writeAllToLifecycleLog(fd, chunk); | ||
| return; | ||
| } | ||
| // Rollover discards the previous log and any oldest bytes from an oversized | ||
| // chunk. Only the fixed marker and newest chunk suffix are retained. | ||
| resetLifecycleLogWithTail(fd, chunk, maxBytes); | ||
| } |
There was a problem hiding this comment.
Bug: runtime log rollover discards nearly all log history instead of retaining a bounded tail
writeBoundedLifecycleLogChunk calls resetLifecycleLogWithTail(fd, chunk, maxBytes) on rollover, passing only the new chunk currently being written — it never reads back any of the previous on-disk content before truncating. Since a single write is typically a short line (tens/hundreds of bytes), the effect is that the entire multi-megabyte log is truncated to just the rollover marker plus that one small line, rather than retaining a "newest bounded tail" as the PR description claims.
This is inconsistent with the startup path, resetOversizedLifecycleLog (below, lines 563–572), which correctly reads back up to MAX_LIFECYCLE_LOG_STARTUP_TAIL_BYTES (1 MiB) of existing file content before calling this same reset helper.
Failure scenario: the dashboard runs for a while, the log grows to just under the 8 MiB cap, and the write that finally crosses it is an ordinary small line. logs/the raw log file then shows only the rollover marker plus that single line — megabytes of recent debugging context vanish silently, right when a user is likely to run logs to investigate an incident.
Consider mirroring the startup path: read back maxBytes - chunk.length bytes of existing tail content before truncating, so live rollover behaves the same as the startup rollover.
|
|
||
| export function localSourceAddressForDashboardHost(host) { | ||
| return isIP(host) === 4 && host.startsWith("127.") ? "127.0.0.1" : null; | ||
| } |
There was a problem hiding this comment.
Minor robustness gap: this only special-cases IPv4 loopback (isIP(host) === 4 && host.startsWith("127.")). For an IPv6 loopback hostname the caller passes the bracketed form (e.g. "[::1]"), and isIP("[::1]") returns 0, so this always returns null for IPv6 loopback and falls through to the interface-enumeration fallback instead of recognizing it directly. That fallback happens to work today because Node conventionally reports an ::1 entry for lo/lo0, but in an environment where IPv6 is disabled or that entry is absent, doctor --deep --url http://[::1]/-style probes would fail closed unnecessarily. Not a security issue (this is the diagnostic probing path, not the write-gating boundary), just a correctness/availability edge case worth a follow-up.
Review: Add portable dashboard lifecycle commands (#75)OverviewAdds This was reviewed by splitting the diff across three focused passes: the lifecycle core (locking/reaping/logs/env handling), the network/loopback security boundary, and the CLI/app integration + docs + tests. Findings below are from direct code reading, not just the PR description text. Findings1. Runtime log rollover discards almost all log history ( 2. Minor: IPv6 loopback probing gap in What held up well
SummaryThis is a large, security-conscious addition, and most of the security-critical logic (locking, env-file validation, loopback/link-local gating) is solid and fails closed on error paths. The one substantive bug is the log-rollover behavior above — low security impact but real operational impact (loses debugging context right when it's needed). Recommend fixing that before merge; the IPv6 loopback probe gap can be a fast follow. |
| const logHandle = await openLifecycleLogForAppend(paths); | ||
| try { | ||
| if (await stopDashboard(context, paths, { quiet: true })) { | ||
| await startDashboard(options, context, paths, preparedStart, logHandle); |
There was a problem hiding this comment.
Possible race: log rotation runs before the old server is stopped, while it can still be writing to the same fd.
In the restart branch, openLifecycleLogForAppend(paths) (which calls resetOversizedLifecycleLog → resetLifecycleLogWithTail → fsFtruncateSync + raw fsWriteSync on the shared fd when the log exceeds MAX_LIFECYCLE_LOG_BYTES) runs before stopDashboard(...) is called. The currently-running detached server process holds its own duplicated fd to the same log file (opened O_APPEND) via installLifecycleLogWriter, and it is not synchronized by withLifecycleLock — that lock only serializes lifecycle CLI invocations against each other, not against the long-running server process itself.
So if the log happens to be oversized at the moment a restart is issued (e.g., due to prior external tampering, or logs written before installLifecycleLogWriter bounded every write), there's a window where the still-alive old server can call its own bounded writer (fsFtruncateSync/fsWriteSync on the same fd) concurrently with this reset, interleaving with the rollover marker + tail write and potentially producing a corrupted/garbled log around the rollover boundary or defeating the “never exceed the cap at write return” invariant that the rest of this file goes to great lengths to guarantee.
This is intentionally ordered this way so that a bad replacement config aborts before the running dashboard is touched (per the README: “A restart validates the protected file... before stopping the running dashboard”), so moving the log-cap check after stopDashboard isn't a free fix — but as written the log-integrity guarantee has a gap specifically on the restart path that start doesn't have (since start only opens/rotates the log once ownership has already been confirmed as not running). Worth confirming this is an accepted/understood tradeoff, since the existing tests (e.g. "marks and retains the newest tail when capping an oversized log before restart") inflate the log via writeFile while the old server is idle, not while it's actively writing — so this interleaving path isn't covered.
| import { isIP } from "node:net"; | ||
|
|
||
| export function isNonLinkLocalInterfaceAddress(address) { | ||
| if (isIP(address) === 4) return !address.startsWith("169.254."); | ||
| if (isIP(address) !== 6) return false; | ||
| try { | ||
| const normalized = new URL(`http://[${address}]`).hostname.slice(1, -1); | ||
| const mapped = normalized.match(/^::ffff:([a-f0-9]+):([a-f0-9]+)$/); | ||
| if (mapped) { | ||
| const high = Number.parseInt(mapped[1], 16); | ||
| const low = Number.parseInt(mapped[2], 16); | ||
| return isNonLinkLocalInterfaceAddress( | ||
| `${high >> 8}.${high & 0xff}.${low >> 8}.${low & 0xff}` | ||
| ); | ||
| } | ||
| const firstHextet = Number.parseInt(normalized.split(":")[0], 16); | ||
| return (firstHextet & 0xffc0) !== 0xfe80; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Minor: link-local / IPv4-mapped canonicalization logic is duplicated between here and src/server/security/machineLocal.ts.
isNonLinkLocalInterfaceAddress here and canonicalAddress/the link-local check in src/server/security/machineLocal.ts independently re-implement the same IPv4-mapped-IPv6 unpacking and 0xfe80/0xffc0 link-local bitmask logic (and bin/lifecycle.js has its own isIpv6LinkLocal doing the same bitmask a third time). They currently agree, but since this logic is security-relevant (it gates which addresses are trusted as "this machine" for write access and for HOST binding), any future tweak to one copy without the others risks silently diverging the trust boundary between the CLI lifecycle code and the server's request-auth code. Consider consolidating into a single shared helper (e.g. exported from interface-address.js and imported by machineLocal.ts) rather than three parallel implementations.
Review summaryThis is a large, security-focused addition (detached lifecycle start/stop/restart/status/logs/open commands, a protected env-file loader, a hardened lifecycle lock, bounded log rotation, and a widened but carefully-scoped machine-local write-auth check). The code is unusually defensive: extensive input validation, fail-closed behavior on ambiguous states, symlink/FIFO/ownership checks on every file it touches, and a large accompanying test suite (scripts/lifecycle.test.ts, scripts/interface-address.test.js, src/server/security/machineLocal.test.ts). I left two inline comments:
Other things I checked and did not flag:
Overall this looks like solid, careful work; my findings are one plausible-but-narrow race condition and one maintainability nit, not correctness or security blockers. |
1 participant
Closes #63
Summary
start,stop,restart,status,logs, andopencommands without tmuxDecision log
--config-env-filebecause Node 24 reserves--env-file; the documented protected default remains availablelocalhostruntimes so later resolver changes do not misreport or open the wrong endpointlocalhostDNS resolution to remain within IPv4/IPv6 loopbackValidation
39793c603cf1a0b821336b16b57ef97932bea270.agents/bin/validate, andgit diff --check127.0.0.1:4317remained healthy and untouchedSecurity
HOSTvalues requireALLOWED_HOSTScontaining only specific hostnames or IP addressesSECURITY_PREFLIGHT_OK