Skip to content

CS-10623: Reimplement boxel realm watch command#4554

Open
FadhlanR wants to merge 10 commits intomainfrom
cs-10623-reimplement-boxel-realm-watch-command
Open

CS-10623: Reimplement boxel realm watch command#4554
FadhlanR wants to merge 10 commits intomainfrom
cs-10623-reimplement-boxel-realm-watch-command

Conversation

@FadhlanR
Copy link
Copy Markdown
Contributor

@FadhlanR FadhlanR commented Apr 28, 2026

Summary

  • Port boxel watch from the standalone cardstack/boxel-cli into packages/boxel-cli/src/commands/realm/watch.ts
  • Namespaced under realm group (was top-level boxel watch in the legacy CLI). The Claude Code plugin's skill copy needs to use boxel realm watch.
  • Options: -i, --interval <seconds> (default 30), -d, --debounce <seconds> (default 5). Silencing uses the program-level -q, --quiet (boxel -q realm watch …), introduced in lib/cli-log.ts on main; the watch command does not declare its own -q.
  • Single realm at the CLI surface (<realm-url> <local-dir>). The internal watchRealms() API takes an array of specs to keep room for a future multi-realm CLI; today's CLI passes a single spec and the resolved authenticator is shared, so multi-realm callers must use realms that share a profile / secret seed.

boxel realm stop — out of scope, deferred to CS-10624

This PR ships only the single-terminal Ctrl+C path. SIGINT cleanly tears down the timers, releases the lock, and exits — same shape as the legacy CLI, which also ran watch in the foreground.

boxel realm stop (CS-10624) solves a different problem and is still wanted once boxel realm track (CS-10622) lands: a cross-terminal kill switch that finds every running watch/track process (the legacy walked a process registry and SIGINTed each registered PID) and shuts them all down at once. Out of scope here.

What this PR does add for safety is a concurrency guard, not a stop mechanism: .boxel-watch.lock is written into the local-dir while a watch is active, so a second boxel realm watch against the same dir refuses to start (and overwrites a stale lock from a non-running pid). The lock records { pid, startedAt, realmUrl } so CS-10624 can later use it as one discovery source.

Cross-command coordination (pull/push/sync warning when a watch is active against the same dir) is also out of scope here — separate ticket if/when wanted.

Test plan

  • pnpm --filter @cardstack/boxel-cli test:integration passes (12 watch cases: add/modify/delete/burst/loop/abort/error paths/poll-error-doesn't-delete/pending-modify→delete-supersedes/lock-blocks-second-watch/stale-lock-overwrite)
  • pnpm --filter @cardstack/boxel-cli build succeeds
  • boxel realm watch --help documents -i, -d (silencing is via the program-level boxel -q …)
  • Manual: boxel realm watch <staging-url>, edit a card via Boxel web UI, confirm local pull within ~30s and a [remote] checkpoint is created.

Depends on

  • CS-10625 for checkpoint creation on detected changes — already merged when this implementation landed; uses CheckpointManager directly.

🤖 Generated with Claude Code

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a new boxel realm watch command implementation in the monorepo Boxel CLI, along with integration tests, to continuously poll a realm for remote changes and pull them into a local directory (with checkpoint creation).

Changes:

  • Added packages/boxel-cli/src/commands/realm/watch.ts implementing RealmWatcher, watchRealms(), and CLI registration for boxel realm watch.
  • Added integration tests covering initial sync, modify/delete detection, debounced batching, abort handling, and error cases.
  • Wired the new watch subcommand into the realm command group.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
packages/boxel-cli/src/commands/realm/watch.ts Implements polling-based realm watcher, debounced apply/flush, checkpointing, and CLI command registration.
packages/boxel-cli/tests/integration/realm-watch.test.ts Adds end-to-end integration coverage for watcher behaviors and error handling.
packages/boxel-cli/src/commands/realm/index.ts Registers the new watch subcommand under boxel realm.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/boxel-cli/src/commands/realm/watch.ts Outdated
Comment thread packages/boxel-cli/src/commands/realm/watch.ts Outdated
Comment thread packages/boxel-cli/src/commands/realm/watch.ts
Comment thread packages/boxel-cli/src/commands/realm/watch.ts
Comment thread packages/boxel-cli/src/commands/realm/watch.ts Outdated
Comment thread packages/boxel-cli/src/commands/realm/watch.ts Outdated
FadhlanR added a commit that referenced this pull request May 5, 2026
Captures the five-commit plan for addressing the review feedback on
PR #4554: correctness fixes (poll-error swallowing, flush/poll race,
setInterval reentrancy, pending→delete transition), minimal lock file,
PR description cleanup, code cleanups (option typing, persistManifest
incrementality, duplicate _mtimes probe, TTY colors), and nits.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
FadhlanR added a commit that referenced this pull request May 5, 2026
- Override `getRemoteMtimes` so poll failures throw instead of
  returning an empty map. The base swallow-and-empty behavior is fine
  for `pull` but in the watcher it would be read as "every file was
  deleted remotely" and wipe the local directory on a transient
  network blip.
- Snapshot `pendingChanges` and clear it before any I/O in
  `flushPending`. Anything an interleaved `poll()` records during the
  flush now rolls into the next flush instead of being dropped by the
  trailing `clear()`.
- Replace `setInterval(tickAll, intervalMs)` with a self-scheduling
  `setTimeout` chain. Two ticks can no longer overlap, eliminating a
  reentrancy that compounded the flush/poll race above.
- When a previously-known file is missing from `_mtimes`, override
  any non-`deleted` pending entry to `deleted` instead of skipping
  the deletion sweep. Previously, a pending add/modify followed by a
  remote delete would try to download a 404'd file at flush time.

Adds two integration tests: one verifying a poll error doesn't
delete local files, and one verifying that a remote delete supersedes
a pending modify.

Addresses Copilot review comments on PR #4554 lines 144, 161, 208,
and 417.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
FadhlanR and others added 9 commits May 5, 2026 18:55
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Polls a realm's `_mtimes` endpoint, accumulates changes between ticks,
and applies them in a debounced batch — downloading new/modified files,
removing locally what's gone remote, and writing a checkpoint. Reuses
the `RealmSyncBase` + `CheckpointManager` + sync-manifest plumbing the
other realm commands share, and accepts `RealmAuthenticator` so both
the profile flow and `--realm-secret-seed` work.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Captures the five-commit plan for addressing the review feedback on
PR #4554: correctness fixes (poll-error swallowing, flush/poll race,
setInterval reentrancy, pending→delete transition), minimal lock file,
PR description cleanup, code cleanups (option typing, persistManifest
incrementality, duplicate _mtimes probe, TTY colors), and nits.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Override `getRemoteMtimes` so poll failures throw instead of
  returning an empty map. The base swallow-and-empty behavior is fine
  for `pull` but in the watcher it would be read as "every file was
  deleted remotely" and wipe the local directory on a transient
  network blip.
- Snapshot `pendingChanges` and clear it before any I/O in
  `flushPending`. Anything an interleaved `poll()` records during the
  flush now rolls into the next flush instead of being dropped by the
  trailing `clear()`.
- Replace `setInterval(tickAll, intervalMs)` with a self-scheduling
  `setTimeout` chain. Two ticks can no longer overlap, eliminating a
  reentrancy that compounded the flush/poll race above.
- When a previously-known file is missing from `_mtimes`, override
  any non-`deleted` pending entry to `deleted` instead of skipping
  the deletion sweep. Previously, a pending add/modify followed by a
  remote delete would try to download a 404'd file at flush time.

Adds two integration tests: one verifying a poll error doesn't
delete local files, and one verifying that a remote delete supersedes
a pending modify.

Addresses Copilot review comments on PR #4554 lines 144, 161, 208,
and 417.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Each `watchRealms()` call now writes `.boxel-watch.lock` (containing
pid, start time, realm URL) into every spec.localDir before constructing
watchers, and removes it during shutdown. A second `watchRealms()`
against the same localDir returns an error referencing the running pid;
a stale lock from a non-existent pid is detected via process.kill(pid, 0)
and overwritten with a notice.

Cross-command coordination (pull/push/sync warning when watch is active)
is intentionally out of scope of this PR — that's a separate ticket.

Two integration tests cover both the live-pid block and the stale-lock
overwrite paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The CLI passes a single spec; the array-of-specs shape on
`watchRealms()` exists for programmatic / test use. Make the
single-tenant authenticator resolution explicit in the doc comment so
future multi-realm callers know they must use realms that share a
profile / secret seed.

Companion change: PR description updated via `gh pr edit` to drop the
"Multi-realm support" claim and resolve the `boxel stop` open question
now that the lock file landed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four small cleanups, no behavior change at the CLI surface:

- Drop `WatcherInternalOptions`. `RealmWatcher` now passes plain
  `{ realmUrl, localDir }` to `super()` and keeps `debounceMs`/`quiet`
  only as instance fields, matching the `pullOptions`/`pushOptions`
  shape used by the sibling sync commands.
- Make `persistManifest` O(changed files): load the prior manifest,
  delete the entries for `deleted`, rehash `pulled`, copy
  `lastKnownMtimes` for `remoteMtimes`. Previously every applied
  batch rehashed every file in `lastKnownMtimes`.
- Replace the explicit `_mtimes` probe in `initialize()` with a
  call to `getRemoteMtimes()` — the override added in commit 1
  already throws on access failure, so the duplicated probe code
  was redundant.
- Make `lib/colors.ts` TTY-aware. Constants resolve to empty strings
  when `process.stdout.isTTY` is false or `NO_COLOR` is set, so
  `boxel realm watch ... > log.txt` no longer captures raw ANSI
  escapes. Affects every command that imports from `lib/colors.ts`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- `localDirs: string[]` is `const` (the binding never gets reassigned).
- Reflow two prettier complaints introduced by the cleanup commit.
- Remove the plan doc per the project's plan-then-implement convention
  (matches how the original plan was rolled into c6076cf).

`pnpm --filter @cardstack/boxel-cli lint` now exits 0.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Boxel-cli now has a global `-q, --quiet` (lib/cli-log.ts + src/index.ts)
that monkey-patches console.log/info/debug. Drop the duplicate watch
subcommand flag — `boxel -q realm watch` now drives silencing for the
CLI path. Also remove the `if (!quiet)` wrappers around console.error so
poll/flush errors always reach the operator. Programmatic `quiet`
plumbing on RealmWatcher / WatchRealmsOptions is kept for the
integration tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@FadhlanR FadhlanR force-pushed the cs-10623-reimplement-boxel-realm-watch-command branch from f97a3cf to ed81a91 Compare May 5, 2026 12:23
@FadhlanR FadhlanR marked this pull request as ready for review May 5, 2026 12:49
@FadhlanR FadhlanR requested a review from a team May 5, 2026 12:49
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c258448ca1

ℹ️ 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".

startedAt: new Date().toISOString(),
realmUrl,
};
await fs.writeFile(lockPath(localDir), JSON.stringify(info, null, 2) + '\n');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Acquire watch lock atomically

This lock write is not atomic because acquireWatchLock() first checks for an existing lock and then unconditionally writes with default flags; two boxel realm watch processes started at nearly the same time can both pass the read check and both start watching the same directory. That defeats the concurrency guard and can lead to competing pulls/checkpoints in one workspace.

Useful? React with 👍 / 👎.

for (const [fileUrl, mtime] of Object.entries(
data.data?.attributes?.mtimes ?? {},
)) {
mtimes.set(fileUrl.replace(this.normalizedRealmUrl, ''), mtime);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Decode _mtimes file paths before queuing changes

The _mtimes endpoint returns URL-encoded file keys, but this watcher stores them directly as relative paths. When filenames contain spaces or other encoded characters, the watcher will pull into incorrectly encoded local paths (for example %20 in names) and subsequent modify/delete matching will diverge from real filesystem paths, causing incorrect sync behavior for those files.

Useful? React with 👍 / 👎.

@habdelra
Copy link
Copy Markdown
Contributor

habdelra commented May 5, 2026

One thing we need to be careful for anytime we wire up subscription logic is teardown. this is one of the most common avenues for memory leaks, as failing to teardown properly means that you retain references to things that are now out of context. these can be really hard to spot, can you do me a favor and ask claude to "audit for proper teardown of all subscriptions to ensure that we have no memory leaks"--thanks!

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.

4 participants