Review round 3 on #439.
- The attachRemoteSession branch of POST /api/sessions ran `ensureHostAwake`
before the multi-user gates, so a non-admin could have any configured
host's `wakeCommand` spawned (or a packet broadcast) and the request held
for the wake budget, then be refused for the workingDir. The admin gate
now comes first, before the host is even looked up; remote hosts are
admin-only infrastructure everywhere else. Route test: wake spy empty,
403.
- The non-wait input route answers `{buffered:true}` when the registry took
the chunk and `{buffered:true, dropped:true}` when it was over the cap
and is gone (`RemoteInputOutcome` gains 'dropped'); additive to the bare
`{}`.
- The send-and-wait path answers OPERATION_FAILED when the host never comes
back, like create and attach, instead of writing into the stalled pane
and reporting delivered:true plus a timeout.
- The flush writes with `fromUser: true`, so a first prompt buffered
through a wake can still name the tab.
Docs: api-reference (input route), remote-sessions.md (two invariants),
CLAUDE.md key pattern.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QdGP4jUTjc9J2RYYykDrCG
Review round 2 on #439.
1. The bare TCP probe connects to host:port, which a host behind a jump host
or SOCKS proxy does not answer even while ssh works. Acting on that
verdict drew a permanent banner over a healthy session, replaced a real
"needs tmux" error with "not reachable" in quick-start, and - with a wake
target - buffered every HTTP input for the life of the session, since the
readiness poll could never succeed. `WakeableRemote` now carries
`jumpHost`/`socksProxy`/`extraSshOptions`, and `isProbeable()` turns such
a host into reachability-UNKNOWN: input is delivered, `checkReachable` /
`checkHostReachable` answer `null` (never `false`), `ensureHostAwake`
returns `'unprobeable'` (handled like `'no-target'`), the quick-start gate
fires on `=== false` only, and `GET …/reachability` reports
`reachable: null, probeable: false` so the banner has nothing to key on.
A wake target can still be fired for it, blind: no readiness poll, no
reattach, no toast - the response says only whether the packet went out.
2. `'remote:'` joins the session-scoped SSE prefixes. The create/attach wake
has no session yet, so the registry names the requesting user
(`ensureHostAwake({ requestedBy })` -> `username` in the payload) and
`deriveSseHint` routes on it; with neither it fails closed to admins.
Single-user mode is unaffected.
Smaller, from the same review:
- A flush write that fails now drops the remaining buffer (logged) instead
of retaining it: the wake still resolved and marked the host reachable, so
the retained chunk waited for the NEXT wake and was replayed hours later,
after everything typed since. Same policy as the oversized paste.
- The banner polls on tab activation (a user action) and on its 30 s timer
only for a host with a wake target; a timer connecting to a host Codeman
cannot wake is the traffic invariant #2 rejects keepalives for. A proxied
host is never polled.
- `probeRemoteHostReachable`, `runRemoteWakeCommand` and the default UDP
socket refuse under VITEST, as remote-files.ts does. The guard caught a
leak on the spot: `createDefaultRemoteWakeDeps({ probe })` overrode the
probe but still polled readiness with the real one, so the shutdown test
had been connecting to a production address. The poll now uses the
injected probe.
- docs/remote-sessions.md is additions only again (the reformatting is
gone); the architecture-invariants overlap resolved itself in the merge.
Live, against a throwaway instance with a non-routable ghost host: proxied
-> no probe, no wake, the genuine ssh error after 10 s; direct (control) ->
probe, magic packet, "did not come back" after the 40 s budget.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QdGP4jUTjc9J2RYYykDrCG
Resolves CLAUDE.md count tables (route counts recounted on the merged
tree: 235 handlers, sessions 37) and keeps both the host-wake and the
reboot-restore banner in index.html.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QdGP4jUTjc9J2RYYykDrCG
Own review pass over the PR:
- `_flush` took the chunk out of the buffer only AFTER awaiting the write. Input
arriving during that await is enqueued (`waking` is still set, so it takes the
buffer path), and the 4 KB cap then drops the OLDEST chunk — which is the one
already on its way to the pane. The `shift()` that followed removed the NEXT
chunk instead, so the drop-oldest bookkeeping silently lost a chunk that was
never written, while the log line blamed the one that was. The chunk is now
removed before the await and re-inserted at the FRONT on a failed write, so the
order of the queue behind it is preserved. Regression test: a chunk enqueued
during the first write of a full buffer must still reach the pane (red against
the old order).
- `showCreateCaseModal()` reset the remote-host form fields but not the two new
wake inputs, so one host's MAC/command carried over into the next host that
form saved.
- The banner's pre-poll `wakeConfigured` labelled a command-only host as 'mac'.
Nothing reads the distinction, but the field is documented as which path is
configured, so it says the truth until the first poll corrects it.
- Stale `resolveRemote` comment ("only for sessions that have no usable target of
their own"): after the host config became authoritative in both directions it is
consulted on the TTL regardless.
- `host-wake-ui.js` joins the documented load order (12.2) and gets its
`@dependency`/`@loadorder` tags; the frontend module count is 33, not 32.
- `remote-wake` is not "(pure)" — the module uses `dgram`/`net`/`child_process`.
- SSE counts: 160 constants, and the category is "Remote auto-reconnect / wake
(5)"; the route table's per-file counts are refreshed (sessions 37, cases 34).
- The CLAUDE.md wake rule now names the create/attach wake, the 40 s request
budget, the whole-chunk paste drop, the registry's lifetime (drop on cleanup,
stop on shutdown) and the deliberately non-wake-aware WebSocket keystroke
path — that paragraph is what the next person reads.
- Reverted the eight lines of unrelated Prettier markdown churn in
`docs/architecture-invariants.md` (docs/ is not in the format glob, so it was
an editor): only the new wake paragraph remains in the diff.
Review follow-up on the wake-on-LAN PR (five findings, all of them about the
state the feature keeps and the budgets it inherits):
- Wake state is dropped by `WebServer.cleanupSession` instead of the two delete
routes, so it now goes with the session on EVERY cleanup path (cron, admin,
scheduled-run teardown, error paths) instead of surviving with up to 4 KB of
the user's buffered keystrokes. `registerSessionRoutes` returns the registry
so the server can own its lifetime without the wake-capable code living in
`server.ts`; the wiring guard is updated to allow that and gains a second
assertion that `server.ts` calls nothing but `drop`/`stop` on it.
- `_effectiveRemote` returns before `_state`, so a LOCAL session no longer gets
a wake-state entry — the input gate runs on every keystroke, so that entry
used to be allocated for every session the user types in.
- An input chunk larger than the 4 KB cap is dropped OUTRIGHT instead of being
head-trimmed and then written as a fragment: one paste is one `input` value
and was never typed character by character, so its tail is a partial command
the user never sent. The drop is logged.
- The manual wake button passes `REMOTE_WAKE_REQUEST_READY_TIMEOUT_MS` (40 s)
like the create/attach paths, instead of inheriting the 90 s session default
that the dashboard's reverse proxy cuts off at 60 s.
- `RemoteWakeRegistry.stop()` aborts in-flight readiness polls (abortable
sleep) and refuses new wakes, and `WebServer.stop()` calls it, so a restart
during a wake no longer waits the poll out.
- The banner/toast wording keys off a new `queuedInput` flag on the two SSE
events, which is true only when the server actually holds bytes: browser
keystrokes travel over the WebSocket, which never passes through the
registry, so the wake BUTTON must not promise queued input. The failed-wake
path also stops pattern-matching the error message (it re-asks the
reachability route) and the WoL dialog says "admin-only" instead of "host not
found" for a non-admin in multi-user mode.
refreshHostWakeBanner clears _hostWake before calling _hostWakeTick, so the
clear branch's `if (this._hostWake)` guard skipped the repaint: once the
banner had appeared for an unreachable remote session it stayed up on every
chat (local ones included) until a reload, and the 30s ticker never cleared
it either. Render unconditionally in that branch — _renderHostWakeBanner is
idempotent with a null state.
Reproduced in a real browser (Puppeteer, mobile viewport): state went null
but banner.hidden stayed false. Regression test added in
test/host-wake-banner.test.ts (red before, green after).
Found by driving the real UI: with a MAC configured in remote-hosts.json, removing it
(here: to reach the "Configure WoL" dialog) changed nothing for a running session —
_effectiveRemote short-circuited on the session's own snapshot whenever that snapshot
HAD a target, so the resolver was only ever consulted in the one direction where the
feature was missing. The documented "host config is authoritative" promise therefore
failed in the direction a user can actually observe, and a wake target could live on
invisibly after being deleted from the config.
The resolver is now consulted on the TTL regardless, and wins for the wake fields in
both directions. Also adds a route test for the browser's real input shape: one POST
per keystroke, all buffered during a wake, replayed IN ORDER.
Pressing Run on a remote case whose host was asleep failed with
`could not verify tmux on remote host 192.168.50.137: …` — an ssh error that
blames tmux for a machine that is merely suspended. The only wake paths were
typed input on an established session and the banner's Wake button, so OPENING a
session (the moment the user actually decides to use that host) had none.
`RemoteWakeRegistry.ensureHostAwake()` reuses the existing probe/wake/readiness
machinery for a host that has no session yet, and is wired into the two
user-initiated create paths: `POST /api/quick-start` for a remote case (before
the tmux prereq probe, which is what surfaced the misleading error) and
`POST /api/sessions` with `attachRemoteSession`. A host without a wake target is
not even probed, so its behavior and latency are byte-identical. The wake is
blocking — the caller gets the session or an error — but bounded by
REMOTE_WAKE_REQUEST_READY_TIMEOUT_MS (40 s) instead of the 90 s session default,
because the dashboard sits behind a reverse proxy whose default
`proxy_read_timeout` is 60 s: a longer wait would be cut off at the proxy while
the session was still being created. The budget has to cover the whole request
(40 s wake + 1.5 s probe + the tmux probe's own 15 s = 56.5 s worst case), which
is why it is 40 s and not 45. A timeout now says the host did not come back, and
an unreachable host without a wake target says so instead of pointing at tmux.
The wiring is deliberately in the HTTP ROUTE, never in the shared session
service: `cron-service.ts` builds sessions there with nobody waiting on the
answer, and a wake on that path would power the host on for every schedule —
the timer-driven re-wake invariant #1 exists to prevent. Both halves are asserted
(importers of `remote-wake`, and `ensureHostAwake` having exactly one caller
file), so a future caller has to come through the guard test. A rejection from
the wake IO is caught too: a broken target must fail the wake, not the route.
`remote:hostWaking`/`remote:hostWakeFailed` now carry `forNewSession` for the
session-less case, where "input is queued" would be untrue; the toast then reads
"the session starts when it is back".
Live wake numbers are unchanged (this reuses the measured ~12 s S3 path); the
route behavior is covered by new tests in session-routes.test.ts with an injected
registry, so no test opens a real socket or ssh.
Two findings from a final review pass over the wake-on-LAN feature.
`_onRemoteHostWaking` / `_onRemoteHostWakeFailed` were defined in BOTH
`panels-ui.js` (toasts) and `host-wake-ui.js` (banner). Both files mix into
`CodemanApp.prototype` and `host-wake-ui.js` loads later, so the panels-ui copies
were silently shadowed: the toast never fired, and a wake started for a BACKGROUND
session (input on a non-active tab) produced no notification at all, since the
banner handler only acts on the active session. The handlers now live only in
`host-wake-ui.js`, show the toast unconditionally, and update the banner when the
woken session is the active one.
`appendBoundedPending` dropped only WHOLE chunks, so a single input value over the
cap (one large paste is one `input` value, up to the 100 KB input schema) was kept
in full: "bounded at 4 KB" held per chunk, not per session, and nothing was logged.
The surviving chunk's head is now trimmed too, code-point aware so a multi-byte
character is never split into a replacement char.
Adds the guard that would have caught the first one: every SSE dispatch handler must
be defined in exactly ONE frontend module. The existing test only asserts a handler
EXISTS somewhere, which two modules both satisfy while one is shadowed.
Reported as 'the tab shows no banner' while the host was verifiably unreachable: the
banner only started polling from selectSession, which RETURNS EARLY for the tab you
are already on (so a page loaded with the remote tab active never polled), and a
long-lived tab keeps running the JS it loaded — the feature was invisible to anyone
who did not switch tabs after the deploy.
The poller is now page-wide: one interval (created on init and on the first session
switch), re-targeted whenever the active session changes, plus a visibilitychange
wake-up. It no longer depends on any single selection path running.
Also adds test/sse-dispatch-table.test.ts: a static guard that every
[SSE_EVENTS.X, '_onFoo'] entry names an event constants.js defines AND a handler some
module defines. Both halves fail silently (a typo'd constant is an undefined table
key; a renamed handler just never runs), which is exactly how a new banner can never
appear with no error anywhere.
A configured-but-broken target (host replaced NIC, command removed) had no way
out: the dialog hung off the 'no target configured' branch only, so the banner
would keep offering a Wake button that keeps failing.
setBroadcast() on an unbound dgram socket throws EBADF on Linux and the following
send fails with EACCES, so the magic packet silently never left the machine — the
feature reported a wake that never happened. Caught by waking a real sleeping host
(a unit test with a real UDP broadcast would not be welcome in CI, so the socket is
injectable and the bind-before-setBroadcast ORDER is asserted).
The reactive wake (typing into a session whose host slept) left the state invisible:
nothing told the user the machine was asleep, and with no wake target configured
there was nothing to do about it. Adds:
- RemoteHost.wakeMac (comma-separated) - Codeman builds and broadcasts the magic
packet itself (UDP port 9), so the common case needs no external script. The
existing wakeCommand stays as the explicit override.
- GET /api/sessions/:id/reachability - probes (throttled, cached, and it never
wakes) and reports HOW the host can be woken, or that nothing is configured.
- POST /api/sessions/:id/wake - wakes, waits, reattaches the pane and flushes
buffered input; 400 with a routable message when no target is configured.
- The amber host-unreachable banner + its 'Wake' / 'Configure WoL' action, and a
small config dialog that saves via PUT /api/remote-hosts/:id.
- RemoteWakeDeps.resolveRemote: host config is re-resolved for LIVE sessions
(throttled + cached), so saving the dialog takes effect without a restart.
Self-review pass: the input-ladder's two 'buffer' branches were the same three
lines, and bulk delete left a session's (bounded, per-random-uuid) wake state
behind. Documents the design where the code refers to it - remote-sessions.md
section, the architecture invariant, and the CLAUDE.md key pattern.
A session's remote block is persisted at launch time and recovery uses that
snapshot, so a wakeCommand added to remote-hosts.json afterwards never reached
an already-running session - not even across a Codeman restart (observed: the
live Hufflepuff session came back with no wakeCommand). Merge the host-level
field in on restore, with the host config authoritative.
A durable remote session survives SSH drops (COD-104/108), but nothing brought
the HOST back: after the remote machine suspended, the local tmux pane's ssh
child stalled silently and `send-keys` SUCCEEDS against it, so typed input
vanished with no error anywhere.
Add an optional per-host `wakeCommand` (Wake-on-LAN wrapper, e.g. whuff) that
the input route runs when a wake-enabled host is unreachable: input is buffered,
the host is woken, the pane is reattached, and the buffer is flushed in order.
Detection is a throttled bare TCP probe on wake-enabled hosts only, and only
REAL user input may wake a host - the auto-reconnect watcher and boot recovery
deliberately cannot, or the host would be re-woken seconds after every suspend
and could never stay asleep.
A clicked path that points OUTSIDE the case directory goes through the attachment
routes (the frontend's `_isExternalPreviewPath` sends every absolute path not under
`workingDir` to `POST /attachments`), and those had the same local-`fs` assumption
as file-raw: `realpathSync`/`fs.stat` on a path that only exists on the remote host,
so the file never opened — the case the #415 report was actually about.
- `registerExternalAttachment()` accepts `remote` and resolves through
`remoteProbePaths` (canonical path, size/mtime, kind, plus the workspace root for
the confinement check). Everything around it — blocklist, extension allowlist,
workspace confinement, registry/dedupe — is now shared by both branches, so the
remote path cannot drift from the local one.
- The by-id routes (`raw`, `preview`, `thumbnail`), the metadata poll and the
attachment history list resolve over ssh too. `raw` streams with the same
Range contract as file-raw; `preview` (office) and `thumbnail` answer 400 for a
remote record; an unreachable host answers 502, a vanished file 404.
- Which host a record is read from follows the SESSION, never the path string: the
same absolute path is a different file on each host, and a remote session never
falls back to a local file with that name.
- Codex generated artifacts keep force-workspace confinement for a remote case: the
well-known artifact directories are anchored at THIS host's home, so only a file
inside the remote workspace is trusted.
Still local-only by design: writes, office conversion, thumbnails, the file
tree/picker and tail-file.
A remote case's workingDir is an absolute path on the remote host, but the
file read routes resolved it with local `fs`: `validateSessionFilePath`'s
realpathSync fails for a path that does not exist on the Codeman host, so
every preview of an agent-written file answered "File not found" (#415).
Add src/remote-files.ts as the single remote-read layer, built on the same
buildSshConnectionArgs() the launch uses:
- remoteProbePaths(): ONE round trip returning realpath + stat for the
requested path AND the workspace root, so containment is checked against a
remotely canonicalized root (a symlinked remotePath is ordinary).
- remoteCreateReadStream(): streams the body (cat, or tail -c +N | head -c L
for a Range) with nothing buffered in memory, and reaps the ssh child when
the response ends so an aborted download cannot orphan it.
- remoteReadFile(): bounded read for file-content.
file-raw, file-content, file-preview and file-thumbnail now share one local/
remote target resolution. Guards keep their local strength: lexical pre-check,
remote realpath, workspace containment, sensitive-path blocklist, and the size
cap applied to the remote size before any bytes are read. An unreachable host
answers 502 with the remote reason instead of a misleading 404. Nothing is ever
copied to the Codeman host and there is NO local fallback (an sshfs mount of
the same tree must not shadow the remote bytes).
Deliberately unchanged: writes (edit=1 / PUT now answer 400 explicitly while
the viewer hides its Edit affordance), office previews, thumbnails, file tree,
picker, external attachment registration and tail-file stay local-only.
