Skip to content

fix(omp,remote): thread remote-omp resume/continue through respawn and reattach - #362

Merged
Ark0N merged 3 commits into
Ark0N:masterfrom
timkjr:feat/omp-remote-continuation
Sep 12, 2026
Merged

fix(omp,remote): thread remote-omp resume/continue through respawn and reattach#362
Ark0N merged 3 commits into
Ark0N:masterfrom
timkjr:feat/omp-remote-continuation

Conversation

@timkjr

@timkjr timkjr commented Aug 31, 2026

Copy link
Copy Markdown

Summary

Follow-up to #353 (OMP backend) — this is the remote-continuation threading work that was deliberately split out at the time because it can't apply without OmpConfig existing upstream.

Two fixes, found live 2026-08-29 on a remote OMP/Claude node, both stemming from the same root cause: a dead/dropped remote pane relaunched the agent as a brand-new conversation instead of resuming.

  1. fix(omp,remote): pin remote conversations on respawn

    • SSH-remote claude was launched as bare claude --dangerously-skip-permissions, so any remote-respawn (COD-108 reattachRemote re-running the idempotent launch command) started a fresh conversation every time. Pinned it to the deterministic Codeman session id (--session-id <id> to create, || --resume <id> fallback so the idempotent re-run resumes instead of erroring "already in use") — mirrors the existing docker-claude shape.
    • OMP --resume pinning silently degraded to ambiguous --continue whenever a case path ended in a trailing slash (a remote case's remotePath is stored verbatim, e.g. /home/user/dotfiles/). mangleOmpWorkingDir produced -dotfiles- while omp persists sessions under -dotfiles, so lookup never matched. Normalized the trailing slash before mangling.
  2. fix(remote): never auto-revive a remote session after a clean agent exit

    • The COD-108 reconnect watcher treated any dead local pane as a dropped transport and re-ran the pane command — so a normal ctrl-c/ctrl-d on a remote omp/opencode/claude session auto-spawned a fresh agent (claude only looked correct because its --session-id || --resume fallback happened to resume it, with a loud "already in use" error first).
    • Now distinguishes a transport drop from an intentional exit: only reconnects when the durable remote tmux session (codeman-ssh-*) is verifiably still alive on the remote host, probed via ssh has-session, fails closed (skips) when gone or unknown. The probe is cached per-session and fired async so the 5s watcher tick never blocks on ssh.
    • Threads ompConfig/resumeSessionId into the remote command builders so a dead-pane respawn of an omp session resumes (--resume <id>) or continues (--continue) instead of launching bare omp.

Test plan

  • npm run typecheck — clean
  • Full suite: 6358 tests, 6345 passed, 12 skipped, 0 failed (one unrelated environment failure — missing node_modules/.bin/prettier in a fresh worktree — resolved by npm install, confirmed passing after)
  • test/remote-shared-sessions.test.ts (most directly touched) — all passing, including the omp --resume/--continue command-building cases
  • test/remote-auto-reconnect.test.ts — new cases for remote-gone / remote-unknown (fail closed) / remote-alive (legitimate reconnect still works)
  • test/omp-session-resolver.test.ts, test/tmux-manager.test.ts — trailing-slash normalization and SSH-remote claude session-id pinning

🤖 Generated with Claude Code

@Ark0N

Ark0N commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Thanks for this, and sorry it has sat. The branch is conflicting, which means GitHub has run no CI on it at all, and the conflicts are not mechanical: master has moved under two of the three things this PR does.

The reconnect-watcher half is already on master, in a form that has since been fixed. #355 (your commit, same title) merged on 2026-09-04. This version's remoteTmuxSessionAlive() returns stdout.trim().length > 0, and tmux has-session prints nothing and exits 0 when the session exists, so every live remote session reads as gone and transport-drop reconnects are silently disabled along with the clean-exit revives. Master classifies by exit status instead (classifyRemoteAliveExit()), invalidates the cache whenever the pane is seen alive (a stale true revives the next clean exit, a stale false leaves a manually restarted session with reconnect permanently off), and caps probes at one in flight. Every merge conflict in this PR is in that region, and every one resolves to master's side. On rebase, please drop the probe, the remoteAlive field and remote-gone reason, the cache wiring and the reconnect test edits.

The remote omp branch calls buildOmpCommand(), which master deleted (src/tmux-manager.ts:1148). The CLI-registry refactor (4830e662) removed it: omp's --model/--resume/--continue are now launch.variants data on the OMP entry in src/config/cli-registry/stock.ts, rendered by renderCliCommand(), and the remote default comes from overlayCliCommand(mode, 'remote'). test/cli-registry-no-id-branching.test.ts also forbids mode === '<id>' outside the catalog, and only mode === 'claude' is allowlisted in tmux-manager.ts. On a scratch merge with conflicts resolved to master's side: tsc gives TS2552 at the omp branch, the guard fails on tmux-manager.ts:794, and both new remote-omp tests throw ReferenceError. Please render the remote invocation through the registry instead: legacyConfigForMode() in session-cli-registry-bridge.ts to map the session config, renderCliCommand() to render, then remoteLoginShellCommand() to wrap. That is mode-agnostic and gives every registry CLI with a resume form the same behaviour for free. appendResumeFlag() will not work here: appending after the quoted -c 'omp' hands the id to the login shell as $0.

The host-local omp resolver now feeds --resume to the remote host (src/session.ts:1687). reattachRemote() calls _pinOmpRespawnId(), and first idle calls _maybeCaptureOmpSessionId(); both run resolveAndClaimOmpSessionId(this.workingDir), which reads the Codeman host's ~/.omp/agent/sessions/<mangled workingDir>. For a remote case workingDir is remoteCase.remotePath, a path on the other machine. Before this PR the resolved id was inert for remote sessions because the remote launch was always bare omp. Now, when the local host happens to have omp sessions under that path, and your own /home/user/dotfiles example is the archetype (a checkout at the same absolute path on both machines), the remote is told to resume a uuid its ~/.omp has never seen, and _claudeSessionId gets aliased to the wrong host's id. When the local host has none, the resolver returns null, continueSession is set and the remote runs omp --continue, which is correct. Which means: on a true remote, the trailing-slash fix cannot be what made a relaunch resume, --continue is. Please skip host-local resolution when this._remote is set in both methods and set continueSession: true directly for a remote respawn, with a test for a remote session whose local ~/.omp holds a matching directory. Resolving over ssh is the fuller fix and belongs in its own PR.

Smaller: docs/remote-sessions.md still describes the old remote behaviour, the header-cwd half of the trailing-slash fix has no test, and the remote claude branch mirrors claudeDockerPaneCommand only partly.

The claude session pin and the trailing-slash normalisation are good and I want them. If you would rather land those quickly, a45c80e7 on its own is a clean cherry-pick onto master, and the omp threading can follow in a PR that also decides whether remote omp resolution should happen over ssh. Your call which way round.

timkjr and others added 3 commits September 7, 2026 21:20
… resumes instead of relaunching fresh

Two independent defects made ANY clean exit from a remote SSH session (user
ctrl-d or ctrl-c, or a dropped pane) relaunch the agent as a NEW conversation:

1. SSH-remote claude was launched as a bare `claude --dangerously-skip-permissions`,
   so the remote-respawn path (COD-108 reattachRemote re-running the idempotent
   launch command) started a fresh conversation every time. Pin it to the
   deterministic Codeman session id, mirroring the docker-claude shape
   (claudeDockerPaneCommand): `--session-id <id>` to create, with the
   `|| --resume <id>` fallback so the idempotent re-run resumes instead of
   erroring with "already in use". A per-host commands.claude override still wins.

2. OMP --resume pinning silently degraded to ambiguous `--continue` whenever a
   case path ended in a trailing slash (e.g. remote `remotePath` stored verbatim
   as `/home/user/dotfiles/`): mangleOmpWorkingDir produced `-dotfiles-` while
   omp persists sessions under `-dotfiles`, readdirSync returned null for an
   existing dir, and findLatestOmpSessionId/resolveAndClaimOmpSessionId never
   matched. Normalize the trailing slash before mangling (new exported
   stripTrailingSlash) and compare the session header cwd against the same
   normalized value.

Both were found live 2026-08-29 on a remote OMP/Claude node: ctrl-c and ctrl-d
behaved identically, both relaunching a fresh session.
The COD-108 reconnect watcher treated any dead local pane as a dropped
transport and re-ran the pane command — so a normal ctrl-c/ctrl-d on a
remote omp/opencode/claude auto-spawned a FRESH agent (claude only
looked correct because its '--session-id || --resume' fallback resumed,
with a loud 'already in use' error first).

Distinguish a transport drop from an intentional exit: only reconnect
when the durable remote tmux session (codeman-ssh-*) is verifiably
still alive on the remote host. A clean exit tears that session down;
the watcher now probes it via ssh has-session and skips (remote-gone)
when it is gone OR unknown (fail closed). The probe is cached
per-session and fired async so the 5s tick never blocks on ssh.

Also thread ompConfig/resumeSessionId into the remote builders so a
dead-pane respawn of an omp session resumes (--resume <id>) or
continues (--continue) instead of launching bare omp.

Tests: 3 new cases pinning remote-gone / unknown / alive decisions;
remote omp resume + --continue fallback. Verified live: all three
remote CLIs stay dead after exit.
- Remote omp command now renders through buildSpawnCommandFromRegistry
  (the mode-agnostic engine local/docker spawns use) instead of the
  buildOmpCommand() the CLI-registry refactor deleted.
- Session._pinOmpRespawnId()/_maybeCaptureOmpSessionId() now skip
  host-local ~/.omp resolution entirely for a remote session and fall
  back to --continue: that resolver only ever reads THIS host's
  filesystem, which is meaningless (and could wrongly alias an
  unrelated local conversation) for a conversation that lives on the
  remote host.
- Remote-claude launch now honors an explicit resumeSessionId distinct
  from sessionId (mirrors claudeDockerPaneCommand's shape), and
  validates sessionId the same way that sibling does before
  interpolating it into the remote shell command.
- Add the still-missing header-cwd half of the trailing-slash test,
  and document respawn/reattach continuation + auto-reconnect-vs-
  clean-exit in docs/remote-sessions.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@timkjr
timkjr force-pushed the feat/omp-remote-continuation branch from 5041bc6 to 797f0d3 Compare September 8, 2026 03:12
@timkjr

timkjr commented Sep 8, 2026

Copy link
Copy Markdown
Author

Thanks for the detailed review — rebased and addressed all four points.

Reconnect-watcher half: dropped entirely, as you said. Confirmed the merge resolved every conflict in that region to master's side (classifyRemoteAliveExit(), the invalidate-on-alive cache, the single-in-flight probe cap) and that test/remote-auto-reconnect.test.ts came out byte-identical to master's copy — nothing of the old version survived.

Remote omp branch: rendered through the registry now. I used buildSpawnCommandFromRegistry() (the same generic engine buildCodexCommand() already uses for local codex spawns) rather than hand-calling legacyConfigForMode()/renderCliCommand() directly — same effect, one fewer call site to keep in sync. mode === 'omp' still exists as a branch (it has to, to thread resumeSessionId/ompConfig through), so I added it to ALLOWED_BRANCHES in the guard test with the same justification style as the existing claude entry — the branch only picks which config to pass in, the command itself is fully registry-driven.

Host-local omp resolver on a remote respawn: you're right, and your dotfiles example is exactly the failure case I reproduced. _pinOmpRespawnId()/_maybeCaptureOmpSessionId() now skip resolveAndClaimOmpSessionId() entirely when this._remote is set and fall back to continueSession: true directly, with a regression test that seeds a same-mangled-path local session file and asserts it's never touched. Agreed resolving over ssh is a separate PR.

Smaller items: docs/remote-sessions.md now has sections on respawn/reattach continuation and auto-reconnect-vs-clean-exit. Added the missing header-cwd trailing-slash test. And you're right about the claude branch only partly mirroring claudeDockerPaneCommand — it was missing the distinct-resumeId case (--resume <rid> || --session-id <sessionId> when a resume-from-history launch names an id different from the session id); fixed, plus a sessionId validation check to match that function's own defense-in-depth.

On the cherry-pick-vs-one-PR question: I looked at splitting it back out, but by the time everything's rebased and fixed, the omp threading and the claude pinning share too much of the same code path (both go through buildRemoteLaunchCommand, and the session.ts remote-guard fix serves both) to split cleanly into two independently-mergeable PRs — it'd just be a stacked pair with a merge-order dependency. Left it as one PR.

CI is green and it's rebased clean on current master.

@Ark0N
Ark0N merged commit a28b04c into Ark0N:master Sep 12, 2026
2 checks passed
Ark0N pushed a commit that referenced this pull request Sep 12, 2026
Each item is from the pre-merge review of the PR it names, applied on master
rather than by pushing to a contributor branch.

#400 (response viewer, shenlvkang-collab)
- The brief view opened at `scrollTop = 0`, right when it was a single card
  holding the last row. Now that it renders the whole turn, the top is the
  turn's first narration line and the answer can be screens below it, while
  loadFullContext already scrolls to the bottom of the same turn. A multi-row
  turn now opens at its newest text; a single card still opens at the top.

#401 (loopback links as web tabs, shenlvkang-collab)
- Drop `*.localhost` from the auto-route set. Every other member is an address
  literal that can only mean this box; a `*.localhost` DNS name is not one, and
  a resolver with a search domain retries `evil.localhost` as
  `evil.localhost.<search domain>`. The link source is agent-written terminal
  output, so that set is the whole confinement on a tap that makes Codeman
  fetch a URL server-side and persist it. The page-side test stays broader
  (`isOnBoxHostname`), where a false positive only declines to proxy.
- A link to the origin root navigated nothing: the path was flattened to '',
  which openWebview reads as "no deep link", leaving an open frame where it was.
- `this.webviews` being set does not mean it is loaded. initWebviews() assigns a
  truthy empty map and only then awaits the list, so a tap during page load
  found nothing to reuse and POSTed a duplicate record. Join the in-flight
  refresh instead.
- One dashboard per dev server rather than per host spelling, which is what the
  method's own comment already promised.
- Toast on the auto-create: it writes webviews.json, broadcasts over SSE and
  adds a Run-dropdown row on every signed-in device, with a new tab as its only
  previous signal.

#362 (remote omp continuation, timkjr)
- Accept the allowlisted `mode === 'omp'` arm as-is; a blanket registry render
  would hand deepseek a locally-resolved --profile and bypass claude's own
  overlay. A registry-declared switch is the follow-up if a third mode needs it.
- Revert the whole-file Prettier reformat of docs/remote-sessions.md (docs/ is
  hand-formatted and outside `npm run format`), keeping only the two new
  sections.
- Correct three stale passages: architecture-invariants' `exec claude
  --dangerously-skip-permissions`, the `exec <cli>` paragraph (claude and omp
  now have their own arms, and the claude pane's PID is the login shell), and
  omp-integration's `-c 'omp'`. RemoteCommandMode gains deepseek and omp.
- Add the missing `_maybeCaptureOmpSessionId` remote-guard test; the sibling
  guard in `_pinOmpRespawnId` had one and this path runs earlier, on the first
  idle turn.

#388 (keyCode 229 recovery, aakhter)
- Gate notifyCanonicalData on shouldSuppressTerminalQueryResponse and
  isTerminalFocusOrMouseReport. onData also carries the DA/DSR/CPR/OSC replies
  xterm answers during Ink redraws and its SGR mouse and focus reports; any of
  those landing between the keydown and the candidate's resolution was read as
  "xterm spoke for this keystroke", standing the recovery down and leaving the
  character dropped, worst on a busy agent pane. Reached through
  window.CodemanTerminalInput: the predicates live in a module IIFE that closes
  long before this call site, so bare references would throw into the
  surrounding try/catch and stop the notify from ever running.

Every fix has a test that fails without it (verified by reverting each).
Full gate green on the combined tree: 358 files, 6849 tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Ark0N

Ark0N commented Sep 12, 2026

Copy link
Copy Markdown
Owner

Merged, thank you, and thanks for the thorough rework. Dropping the reconnect-watcher half entirely and then verifying the merge resolved that whole region to master's side, byte-identical test file included, is exactly the right way to handle a branch that overlapped something already landed. The host-local-resolver bug you reproduced from my dotfiles example is the one that would have bitten people quietly, and the regression test that seeds a same-mangled-path local session file and asserts it is never touched is the right shape for it.

On the design question you left open: I accepted the allowlisted mode === 'omp' arm as-is. A blanket registry render is not the fix, for the reason the guard exists: deepseek would gain a --profile resolved from the LOCAL launcher tree, and claude's overlays.remote.command would be bypassed. The data-driven form would be a registry field (overlays.remote.renderLaunch, or a capability) checked generically after the override and claude arms, and that is worth doing the first time a third mode needs it, not for one. Your entry has an honest reason string and only omp uses it, which is what the guard's own header asks for. Please leave it as-is.

Three things applied on master at merge time (02b0e278):

  • Reverted the whole-file Prettier reformat of docs/remote-sessions.md, keeping your two new sections. docs/ is outside npm run format's globs and hand-formatted on purpose (several other files there fail prettier --check on master too), so the table re-padding and *emphasis* to _emphasis_ was about 40 of the 90 changed lines and buried the real content. The diff is now the two sections plus the corrections below.
  • Three stale doc passages, which your change made wrong rather than introduced: architecture-invariants.md still said remote claude defaults to exec claude --dangerously-skip-permissions, the exec <cli> paragraph still said the pane PID is the agent (with an a || b pair under -c it is the login shell), and omp-integration.md still said remote omp launches -c 'omp'. Also added deepseek and omp to the RemoteCommandMode row, which had drifted from src/types/session.ts independently of this PR.
  • The missing _maybeCaptureOmpSessionId remote-guard test. Its sibling in _pinOmpRespawnId got one and this one did not, and it actually runs earlier, on the first turn going idle, so without the guard it would claim an unrelated local conversation's uuid and every later respawn would inherit the wrong pin. Verified it fails with the guard removed.

Agreed on keeping this as one PR. Your point that the omp threading and the claude pinning share buildRemoteLaunchCommand and the session.ts remote guard is right; splitting would have produced a stacked pair with a merge-order dependency for no benefit. And agreed that resolving omp session ids over ssh is a separate PR.

Shipping in the next release.

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.

2 participants