feat(peek-cli): peek connect — supervised connector daemon (SP6b-2)#148
Conversation
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Adds descriptors.ts with ConnectorDescriptor interface, DESCRIPTORS map (slack entry), getDescriptor(), and resolveSpawn() which merges per-entry overrides from connectors.json with descriptor defaults. 5/5 tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Implements Task 4: the Supervisor class that spawns each enabled connector exactly once, tracks it in a Map<name,Slot>, and writes status.json on every state change (running on spawn, stopped on exit). All side-effects are injected via SupervisorDeps; shutdown() is a stub for Task 5. 8 unit tests cover spawn/disabled/writeStatus/two-connectors/mixed/on-exit/null-exit-code. Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Extends the Task-4 Supervisor with exponential-backoff restart and graceful shutdown. On child exit, attempts counter increments (resets when the child was stable ≥30 s), delay = min(60 s, 1 s × 2^attempts), status goes backing-off with nextRetryAtMs, and a setTimer fires the respawn. shutdown() sets the down flag, clears pending restart timers, SIGTERMs each live child (with SIGKILL escalation via injected timer), marks all stopped, and writes a final status snapshot. An exit received after shutdown is recorded as stopped only — no restart scheduled. 21/21 tests pass; typecheck clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Add `runConnect(argv)` command shell mirroring retention.ts's switch-on-sub pattern. Verbs: add (validates descriptor-or-command, writes registry, prints interactive-setup guidance), list (reads registry, prints or "no connectors configured"), remove (deletes entry, no-op if absent). Lifecycle stubs (start/stop/status/logs/__supervise) return 0 with "not implemented yet" notes for Tasks 7-9. Wires `case 'connect'` into index.ts run() + adds connect line to HELP. 19 tests pass; typecheck + biome clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
- runStart: detached-supervisor spawn with injectable deps (isRunning /
readLock / openLogFd / spawnDetached / cliEntry); prints "already running
(pid N)" when lock is live, otherwise opens supervisor.log fd and spawns
process.execPath [cliEntry, 'connect', '__supervise'] {detached:true} +
unref().
- runSupervise: acquires supervisor lock (null → quiet exit), builds a
Supervisor from readConnectors() with REAL deps (per-connector log routing
via connectorLogPath(name) — the name arg SupervisorDeps.spawn was designed
for), calls start(), registers SIGTERM+SIGINT → shutdown+release+exit,
installs an unref'd keep-alive interval so the daemon survives an empty
registry.
- writeStatusInline: inline status.json writer (atomicWriteFileSync); noted
for Task 8 consolidation into status.ts.
- logs.ts: minimal log-path helpers (supervisorLogPath, connectorLogPath);
Task 9 extends this module.
- Tests: 8 new tests covering the decision logic (no real detached spawn or
signal delivery); old lifecycle-stubs group trimmed to stop/status/logs.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
- Add src/lib/connect/status.ts with statusPath(), readStatus() (null-safe
try/catch + shape guard → {} on absent/malformed), and writeStatus()
(atomic via atomicWriteFileSync, all fs ops injectable for tests).
- Remove inline writeStatusInline/statusFilePath from connect.ts and rewire
buildRealDeps() to use the shared writeStatus from status.ts.
- Implement runStop(): read supervisor lock, SIGTERM if alive, poll until
lock clears or 5 s timeout; idempotent (returns 0 on not-running too).
- Implement runStatus(): read lock (pid + uptime) + readStatus(); prints
a table of connector state/pid/restarts/lastExitCode/nextRetryAt.
- Route stop and status verbs in runConnect() to the new functions; logs
stub remains for Task 9.
- Export RunStopDeps and RunStatusDeps for test injection.
- 44 new tests (status.test.ts × 14, connect.test.ts stop/status × 15 new
+ 1 logs stub) — all 352 package tests green; typecheck clean.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Implements Task 9: `peek connect logs [name] [--follow] [--lines N]`. - Extends `src/lib/connect/logs.ts` with `tailLog` (injectable fs deps; reads last N lines; streams appended bytes via `fs.createReadStream` in follow mode) and `listLogs` (connector names from logs dir). - Exports `runLogs` from `connect.ts`; wires the `logs` verb (replacing the Task-9 stub). No-name → lists available logs. With name → delegates to `tailLog`. `--follow` keeps streaming until interrupted. - Adds `src/lib/connect/logs.test.ts` (10 tests: path helpers, listLogs, tailLog no-follow + follow, absent-file handling). - Extends `connect.test.ts` with 5 logs verb tests covering no-name, no-file guidance, tailLog delegation, --follow injection, routing. - All 366 tests pass; typecheck clean. Write path (buildRealDeps / supervisor / openConnectorLogFd) not touched. Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
…review)
- Extend LogWatcher with on('close', cb) + wire _watchStream to resolve
only on that event (promise was resolving immediately, exiting before streaming)
- Replace default watchFn createReadStream with fs.watch + cursor-based
approach so newly-appended bytes are actually seen
- Guard --lines against NaN/<=0 values: write error to stderr, return 1
- Rewrite follow-mode tests to assert real lifecycle: PENDING before close,
chunk reaches stdout, close() settles the promise
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
…upervise lock-release-on-throw, USAGE --args= (SP6b-2) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a new Changespeek connect daemon feature
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
packages/peek-cli/src/lib/connect/status.ts (1)
63-85: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider validating optional fields in
readStatusshape guard.The shape guard validates
stateandrestartsbut castspid,lastExitCode, andnextRetryAtMswithout type-checking. Ifstatus.jsonis corrupted with wrong types for these fields, downstream consumers (e.g.,runStatusinconnect.ts) could produceNaNin arithmetic or unexpected string interpolation. The risk is low since the supervisor is the trusted writer, but adding type guards for optional numeric fields would make the reader fully defensive.🤖 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 `@packages/peek-cli/src/lib/connect/status.ts` around lines 63 - 85, The shape guard in readStatus currently validates only state and restarts, then trusts optional fields like pid, lastExitCode, and nextRetryAtMs. Tighten the validation in the Object.entries loop by type-checking those optional fields when present before assigning to result, so ConnectorStatus is only populated with fully valid numeric values and downstream consumers such as runStatus in connect.ts won’t see malformed data.packages/peek-cli/src/lib/connect/logs.test.ts (1)
247-290: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest gap: absent-file follow mode doesn't exercise the default watcher.
The absent-file follow-mode test uses an injected
watchthat never errors, so it doesn't catch the crash in the defaultfs.watchimplementation when the file doesn't exist (flagged inlogs.ts). Consider adding a test that uses the defaultwatchdep (or a fake that simulatesfs.watchemitting an'error'event) to verify the absent-file path doesn't crash.🤖 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 `@packages/peek-cli/src/lib/connect/logs.test.ts` around lines 247 - 290, The follow-mode absent-file test in tailLog is only covering a custom watch stub that never fails, so it misses the default fs.watch error path. Update the logs.test.ts coverage around tailLog to exercise the default watch behavior (or a fake that emits an 'error' event) when readFile throws ENOENT, and verify the absent-file follow flow still prints the “no logs yet” message and does not crash. Use the tailLog and watch dependency setup in the existing test block to locate the change.
🤖 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 `@packages/peek-cli/src/commands/connect.ts`:
- Around line 494-522: runConnect currently returns promises from the async
subcommands without awaiting them, so its try-catch cannot intercept rejections
from runStart, runStop, runStatus, runLogs, or runSupervise. Update the switch
in runConnect so the async handlers are awaited before returning, while leaving
the sync handlers (runAdd, runList, runRemove) unchanged; this will ensure any
rejection is caught and reported through the existing peek connect error path.
- Around line 269-273: The shutdown flow in shutdown() exits the process too
early, which prevents the delayed SIGKILL escalation from ever firing. Update
the shutdown logic in connect.ts so it still calls sup.shutdown() and
lock.release(), but does not immediately call process.exit(0); let the existing
setTimer-based escalation complete so stubborn child processes can be terminated
properly.
- Around line 205-225: buildRealDeps().spawn leaks the log file descriptor
returned by openConnectorLogFd because the parent never closes its copy after
_realSpawn starts the child. Update the spawn wrapper to close the fd
immediately after the child process is created, while still passing it through
stdio to the child, so repeated reconnects do not accumulate descriptors. Keep
the change localized to buildRealDeps and the spawn path that uses
openConnectorLogFd/_realSpawn.
In `@packages/peek-cli/src/lib/connect/logs.ts`:
- Around line 140-148: The absent-file follow path in _watchStream currently
relies on the default watchFn, which can trigger fs.watch on a missing file and
crash from an unhandled error. Update _watchStream and/or the default watchFn in
logs.ts to handle the non-existent-file case by registering an error listener
that safely retries or waits for creation, or by switching to a polling fallback
such as fs.watchFile when fileAbsent is true. Ensure the follow-mode logic for
fileAbsent no longer lets the watcher emit an uncaught error.
- Around line 92-121: The default watchFn in logs.ts can start multiple
createReadStream reads from the same pos before the prior stream advances it,
causing duplicate log chunks during bursty fs.watch events. Update the watchFn
implementation to guard the fsWatch callback with an in-flight/read-in-progress
flag (and clear it on end/error) so only one stream is active at a time, while
still advancing pos from the data handler and keeping the existing LogWatcher
close behavior intact.
In `@packages/peek-cli/src/lib/connect/supervisor.ts`:
- Around line 125-133: The shutdown flow in supervisor shutdown handling should
not call process.exit(0) immediately after sup.shutdown(), because that cancels
the SIGKILL grace timer and can release the lock while connectors are still
running. Update the logic around sup.shutdown() in supervisor.ts to let the
process exit naturally after shutdown completes, or make shutdown async and
await it before exiting, so the existing SIGKILL timer and child cleanup in the
supervisor lifecycle can finish.
---
Nitpick comments:
In `@packages/peek-cli/src/lib/connect/logs.test.ts`:
- Around line 247-290: The follow-mode absent-file test in tailLog is only
covering a custom watch stub that never fails, so it misses the default fs.watch
error path. Update the logs.test.ts coverage around tailLog to exercise the
default watch behavior (or a fake that emits an 'error' event) when readFile
throws ENOENT, and verify the absent-file follow flow still prints the “no logs
yet” message and does not crash. Use the tailLog and watch dependency setup in
the existing test block to locate the change.
In `@packages/peek-cli/src/lib/connect/status.ts`:
- Around line 63-85: The shape guard in readStatus currently validates only
state and restarts, then trusts optional fields like pid, lastExitCode, and
nextRetryAtMs. Tighten the validation in the Object.entries loop by
type-checking those optional fields when present before assigning to result, so
ConnectorStatus is only populated with fully valid numeric values and downstream
consumers such as runStatus in connect.ts won’t see malformed data.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4563d203-be18-4094-be98-66109f9cede8
📒 Files selected for processing (16)
.changeset/peek-cli-connect-daemon.mdpackages/peek-cli/src/commands/connect.test.tspackages/peek-cli/src/commands/connect.tspackages/peek-cli/src/index.tspackages/peek-cli/src/lib/connect/descriptors.test.tspackages/peek-cli/src/lib/connect/descriptors.tspackages/peek-cli/src/lib/connect/logs.test.tspackages/peek-cli/src/lib/connect/logs.tspackages/peek-cli/src/lib/connect/registry.test.tspackages/peek-cli/src/lib/connect/registry.tspackages/peek-cli/src/lib/connect/status.test.tspackages/peek-cli/src/lib/connect/status.tspackages/peek-cli/src/lib/connect/supervisor-lock.test.tspackages/peek-cli/src/lib/connect/supervisor-lock.tspackages/peek-cli/src/lib/connect/supervisor.test.tspackages/peek-cli/src/lib/connect/supervisor.ts
…L, log-tail race + absent-file crash, status shape-guard (SP6b-2 #148) Fix A (connect.ts buildRealDeps): close the parent's copy of the connector log fd immediately after spawn(). The child inherits its own dup (dup2 at fork) so closing the parent copy right after spawn is safe and prevents an unbounded fd leak across restarts. Fix B (supervisor.ts shutdown + connect.ts runSupervise): make shutdown() return Promise<void> that resolves when all tracked children have emitted 'exit' (clean SIGTERM path) or after the SIGKILL grace elapses and a bounded fallback tick fires (never hangs). Uses injected setTimer/clearTimer so tests drive it entirely with the fake clock. The signal handler in runSupervise is now async (void sup.shutdown().then(release+exit)) so the lock is released and the process exits only after children are confirmed down. This also closes the "SIGKILL escalation never fires" gap — the grace timer now actually fires via the fake clock in the new tests. Fix C (logs.ts default watchFn): add reading/pending flags so concurrent fs.watch change events do not open overlapping read streams from the same byte cursor. A new read only starts when the previous stream ends; if a change arrived while reading, one more pass is done. This serialises reads and prevents duplicate output on burst writes. Fix D (logs.ts default watchFn): register an 'error' listener on the fs.watch watcher. On ENOENT (file absent in follow mode) the error is swallowed so the process does not crash. The follow promise stays pending until close() is called. Fix E (status.ts readStatus): tighten the shape guard for optional numeric fields. pid/lastExitCode/nextRetryAtMs are now only included in the result when typeof === 'number' (conditional-spread); malformed values are silently dropped rather than cast. Satisfies exactOptionalPropertyTypes. Fix F (logs.test.ts): add test for the absent-file follow path through a fake watcher that exposes an 'error' listener — asserts the listener is registered, calling it does not throw, the promise stays pending, and 'no logs yet' is printed. Tests 378/378 green, no hangs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
…+ shutdown .catch (SP6b-2 #148 review) - Add _setFsWatch seam to logs.ts so tests can inject a fake fs.watch into the DEFAULT watchFn (not the outer watch dep), making the FIX-D try/catch + on('error') code actually execute. The old tautological test injected the entire watch dep, bypassing FIX-D entirely. - Harden default watchFn for both error modes: try/catch for synchronous throws (Linux ENOENT behaviour) + existing on('error') for async events (macOS/Windows). Both return/stay a valid watcher handle. - Replace the tautological absent-file test with two _fsWatch-seam tests (one per failure mode) that FAIL when FIX-D is reverted and PASS with it. - Add .catch(() => { lock.release(); process.exit(1) }) to the shutdown promise chain in runSupervise so a rejection never leaves the lock held. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
…#148) The five async subcommands (start/__supervise/stop/status/logs) were returned from runConnect without await, so a rejection settled outside the try/catch and would escape as an unhandled rejection instead of the clean `peek connect: <msg>` + exit 1. Route through resolved handler refs (injectable, matching the file's deps? convention) and `return await` each. Adds a guard test that fails with a bare return. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/peek-cli/src/lib/connect/supervisor.ts`:
- Around line 132-133: The shutdown path in supervisor.ts is incorrectly
treating connectors in `backing-off` as alive, which leaves `waitForExit`
waiting on children that have already exited. Update the alive-set check in the
supervisor logic so only truly running connectors are added to `alive`, using
the existing `slot.status.state` handling around the `alive.add(name)` path.
Keep the restart-timer cancellation behavior intact, and ensure the stop flow
resolves immediately once no running children remain; the related test in
`supervisor.test.ts` should still pass without needing the extra timer waits.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d19dc64-ad67-44dc-a525-7727d02c5b49
📒 Files selected for processing (8)
packages/peek-cli/src/commands/connect.test.tspackages/peek-cli/src/commands/connect.tspackages/peek-cli/src/lib/connect/logs.test.tspackages/peek-cli/src/lib/connect/logs.tspackages/peek-cli/src/lib/connect/status.test.tspackages/peek-cli/src/lib/connect/status.tspackages/peek-cli/src/lib/connect/supervisor.test.tspackages/peek-cli/src/lib/connect/supervisor.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/peek-cli/src/lib/connect/status.ts
- packages/peek-cli/src/lib/connect/status.test.ts
- packages/peek-cli/src/lib/connect/logs.ts
- packages/peek-cli/src/commands/connect.test.ts
- packages/peek-cli/src/commands/connect.ts
…148) shutdown() added backing-off connectors to the `alive` set and awaited an 'exit' that never comes — a backing-off connector's child already exited (that's why it's backing off), and a dead ChildProcess won't emit 'exit' again. So the promise only resolved via the 5s SIGKILL grace + 0.5s fallback timers, stalling `peek connect stop` ~5.5s whenever any connector was mid-backoff. Only await genuinely 'running' children; their restart timers are already cancelled. Adds a guard test asserting shutdown arms no grace timer and resolves immediately. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
What
Adds
peek connectto@peekdev/cli— a supervised local daemon that runsconnector subprocesses (SP6b-2 of the connector platform). Completes the
study's "CLI-daemon-first" architecture. Builds on SP6b-1 (token capture), SP6a
(secret store), SP4 (pairing), SP1.
The command
~/.peek/connect/connectors.json, zod-validated):peek connect add <surface> [--name] [--command] [--args=<a>],list,remove.slack) → a spawn command(
peek-connector-slack), overridable per entry. peek-cli imports noconnector code — connectors are spawned as subprocesses.
peek connect startspawns a detached__supervisechild guarded by along-held O_EXCL single-instance lock (PID/liveness + stale-takeover).
routing (
connect/logs/<name>.log), restarts on exit with exponentialbackoff (
min(60s, 1s·2^n), reset after 30s stability), writesconnect/status.jsonon transitions, and on SIGTERM/SIGINT stops restarting→ SIGTERM → SIGKILL (grace) → releases the lock.
stop/status/logs [--follow]read the lock + status.json + theper-connector log tail.
Design & quality
status / watch) → the Supervisor core, lock, registry, status, and log-tail
are all unit-tested with no real processes, timers, or fs. 370 tests.
whole-branch opus review). The final review flagged and fixed: a
--followtailer that resolved before streaming (now fs.watch + cursor +on('close')lifecycle), a shared-mutable registry sentinel (now a freshobject per call), a lock leak if
start()throws (now try/catch release),and the USAGE
--args=syntax.Not in scope (deferred)
Autostart / boot-persistence, the localhost dashboard, connector-internal
Socket-Mode reconnect hardening, a
reloadverb.Testing
Full local CI green:
pnpm build && pnpm typecheck && pnpm lint && pnpm test(peek-cli 370 tests / 35 files). The end-to-end daemon smoke (real Slack
connector, live pairing, crash-restart,
--follow) is maintainer-gated — seethe plan's §10.
Changeset:
@peekdev/climinor.🤖 Generated with Claude Code
Summary by CodeRabbit
peek connectcommand to manage local connectors: add, list, remove, start, stop, status, and logs.