Skip to content

feat(peek-cli): peek connect — supervised connector daemon (SP6b-2)#148

Merged
harry-harish merged 16 commits into
mainfrom
worktree-feat+connector-sp6b2-daemon
Jul 8, 2026
Merged

feat(peek-cli): peek connect — supervised connector daemon (SP6b-2)#148
harry-harish merged 16 commits into
mainfrom
worktree-feat+connector-sp6b2-daemon

Conversation

@harry-harish

@harry-harish harry-harish commented Jul 8, 2026

Copy link
Copy Markdown
Member

What

Adds peek connect to @peekdev/cli — a supervised local daemon that runs
connector 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

  • Registry (~/.peek/connect/connectors.json, zod-validated):
    peek connect add <surface> [--name] [--command] [--args=<a>], list, remove.
  • Descriptor registry maps a surface (slack) → a spawn command
    (peek-connector-slack), overridable per entry. peek-cli imports no
    connector code
    — connectors are spawned as subprocesses.
  • peek connect start spawns a detached __supervise child guarded by a
    long-held O_EXCL single-instance lock (PID/liveness + stale-takeover).
  • Supervisor core spawns each enabled connector with per-connector log
    routing (connect/logs/<name>.log), restarts on exit with exponential
    backoff (min(60s, 1s·2^n), reset after 30s stability), writes
    connect/status.json on transitions, and on SIGTERM/SIGINT stops restarting
    → SIGTERM → SIGKILL (grace) → releases the lock.
  • stop / status / logs [--follow] read the lock + status.json + the
    per-connector log tail.

Design & quality

  • Injectable seams throughout (fake spawn / fake clock / injected fs / lock /
    status / watch) → the Supervisor core, lock, registry, status, and log-tail
    are all unit-tested with no real processes, timers, or fs. 370 tests.
  • Built subagent-driven (fresh implementer + task review per task, then a
    whole-branch opus review). The final review flagged and fixed: a
    --follow tailer that resolved before streaming (now fs.watch + cursor +
    on('close') lifecycle), a shared-mutable registry sentinel (now a fresh
    object 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 reload verb.

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 — see
the plan's §10.

Changeset: @peekdev/cli minor.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added a new peek connect command to manage local connectors: add, list, remove, start, stop, status, and logs.
    • Implemented a supervised background connector supervisor with single-instance locking, automatic restart/backoff, and connector lifecycle status.
    • Added per-connector logs with tailing and line-limited output (including follow mode).
  • Bug Fixes
    • Improved resilience to missing or malformed connector registry, status data, and log files, with graceful messaging and safer shutdown/stop behavior.

harry-harish and others added 12 commits July 8, 2026 08:56
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>
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@harry-harish, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 50 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 213df42c-c015-40be-afd2-7ae6fda07319

📥 Commits

Reviewing files that changed from the base of the PR and between f817b62 and 2219e13.

📒 Files selected for processing (2)
  • packages/peek-cli/src/lib/connect/supervisor.test.ts
  • packages/peek-cli/src/lib/connect/supervisor.ts
📝 Walkthrough

Walkthrough

This PR adds a new peek connect CLI surface with registry-backed connector management, supervised daemon startup/shutdown, per-connector logs, persisted status, and top-level CLI routing.

Changes

peek connect daemon feature

Layer / File(s) Summary
Connector registry and descriptors
packages/peek-cli/src/lib/connect/registry.ts, packages/peek-cli/src/lib/connect/registry.test.ts, packages/peek-cli/src/lib/connect/descriptors.ts, packages/peek-cli/src/lib/connect/descriptors.test.ts
Defines connector registry types, validation, CRUD helpers, and descriptor-based spawn resolution with tests.
Supervisor lock
packages/peek-cli/src/lib/connect/supervisor-lock.ts, packages/peek-cli/src/lib/connect/supervisor-lock.test.ts
Implements single-instance lock acquisition, stale-lock takeover, liveness checks, and release handling.
Status and log helpers
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/lib/connect/logs.test.ts
Adds status persistence/readback plus per-connector log enumeration and tailing in follow and non-follow modes.
Supervisor process manager
packages/peek-cli/src/lib/connect/supervisor.ts, packages/peek-cli/src/lib/connect/supervisor.test.ts
Implements connector spawning, restart-with-backoff, and bounded shutdown with final status updates.
CLI command and wiring
packages/peek-cli/src/commands/connect.ts, packages/peek-cli/src/commands/connect.test.ts, packages/peek-cli/src/index.ts
Adds the peek connect subcommands, dispatcher, command help, and top-level CLI routing.
Changeset
.changeset/peek-cli-connect-daemon.md
Adds the release note entry for the new command surface.

Estimated code review effort: 4 (Complex) | ~60 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adding a supervised peek connect daemon for peek-cli.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch worktree-feat+connector-sp6b2-daemon

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (2)
packages/peek-cli/src/lib/connect/status.ts (1)

63-85: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value

Consider validating optional fields in readStatus shape guard.

The shape guard validates state and restarts but casts pid, lastExitCode, and nextRetryAtMs without type-checking. If status.json is corrupted with wrong types for these fields, downstream consumers (e.g., runStatus in connect.ts) could produce NaN in 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 win

Test gap: absent-file follow mode doesn't exercise the default watcher.

The absent-file follow-mode test uses an injected watch that never errors, so it doesn't catch the crash in the default fs.watch implementation when the file doesn't exist (flagged in logs.ts). Consider adding a test that uses the default watch dep (or a fake that simulates fs.watch emitting 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

📥 Commits

Reviewing files that changed from the base of the PR and between 6816b19 and 7a2e3b5.

📒 Files selected for processing (16)
  • .changeset/peek-cli-connect-daemon.md
  • packages/peek-cli/src/commands/connect.test.ts
  • packages/peek-cli/src/commands/connect.ts
  • packages/peek-cli/src/index.ts
  • packages/peek-cli/src/lib/connect/descriptors.test.ts
  • packages/peek-cli/src/lib/connect/descriptors.ts
  • packages/peek-cli/src/lib/connect/logs.test.ts
  • packages/peek-cli/src/lib/connect/logs.ts
  • packages/peek-cli/src/lib/connect/registry.test.ts
  • packages/peek-cli/src/lib/connect/registry.ts
  • packages/peek-cli/src/lib/connect/status.test.ts
  • packages/peek-cli/src/lib/connect/status.ts
  • packages/peek-cli/src/lib/connect/supervisor-lock.test.ts
  • packages/peek-cli/src/lib/connect/supervisor-lock.ts
  • packages/peek-cli/src/lib/connect/supervisor.test.ts
  • packages/peek-cli/src/lib/connect/supervisor.ts

Comment thread packages/peek-cli/src/commands/connect.ts
Comment thread packages/peek-cli/src/commands/connect.ts
Comment thread packages/peek-cli/src/commands/connect.ts
Comment thread packages/peek-cli/src/lib/connect/logs.ts
Comment thread packages/peek-cli/src/lib/connect/logs.ts
Comment thread packages/peek-cli/src/lib/connect/supervisor.ts Outdated
harry-harish and others added 2 commits July 8, 2026 10:45
…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>
@harry-harish

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…#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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 7a2e3b5 and f817b62.

📒 Files selected for processing (8)
  • packages/peek-cli/src/commands/connect.test.ts
  • packages/peek-cli/src/commands/connect.ts
  • packages/peek-cli/src/lib/connect/logs.test.ts
  • packages/peek-cli/src/lib/connect/logs.ts
  • packages/peek-cli/src/lib/connect/status.test.ts
  • packages/peek-cli/src/lib/connect/status.ts
  • packages/peek-cli/src/lib/connect/supervisor.test.ts
  • packages/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

Comment thread packages/peek-cli/src/lib/connect/supervisor.ts Outdated
…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>
@harry-harish harry-harish merged commit 4a22113 into main Jul 8, 2026
7 checks passed
@harry-harish harry-harish deleted the worktree-feat+connector-sp6b2-daemon branch July 8, 2026 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant