feat(adopt): adopt tmux sessions a human started, in all three locations - #364
feat(adopt): adopt tmux sessions a human started, in all three locations#364dignfei wants to merge 15 commits into
Conversation
Docker cases could only run in a container Codeman created itself. Attaching to one the user already built and runs means Codeman must leave that container's lifecycle completely alone, which the launch chain could not do: it was `image inspect` -> `inspect || create` -> `start` -> `exec`. Adds `DockerCase.owned`, mirroring the `owned:false` contract remote-SSH already uses for attached sessions. Absent (every existing case) means owned, so current behaviour is byte-identical. `false` means the container belongs to the user and Codeman may only exec into it. The launch chain for an attached container only looks, then execs: no image gate (the image is theirs), no create, and no `start` — starting a container we do not own is the very mutation attaching promises not to perform. A missing or stopped container fails closed with an actionable message instead. Credential seeding is skipped too: those copies read from create-time read-only mounts that do not exist here, and writing host credentials into someone's container is not ours to do, so its CLIs must already be authenticated inside it. Four fail-closed guards. buildDockerStopCommand and buildDockerRemoveCommand throw during pure string construction, so no caller bug can turn into a `docker stop`/`rm` on a container we do not own; removeDockerContainer refuses again at the lowest layer; drift reports "none" for an attached container, which carries no `codeman.confighash` label and would otherwise always look drifted and 409 the launch gate forever; and the orphan reaper skips attached containers through a check deliberately independent of the two conditions already covering them. `owned` is applied AFTER the config hash is computed. dockerConfigHash takes an explicit field list, so ownership can never shift an existing case's hash — if it did, every pre-existing case would trip the drift gate at once, and the remedy the UI offers is "recreate the container". Adds POST /api/cases/docker-adopt and a read-only POST /api/docker-cases/adopt-preflight. The preflight refuses at LINK time rather than at session launch, where the only ways out would be a dead pane or starting a container we do not own. Tests assert the negative guarantee directly — that create, start, stop, rm, restart and kill are absent from the generated commands while `docker exec -it` and `new-session -A` remain — since it cannot be observed by using the feature.
The Docker tab gains an "Attach to an existing container" toggle. Ticking it swaps the create-time fields (image, network, advanced) — which describe a `docker create` attaching never runs — for the container name, and routes the submit to the adopt endpoint. Reuses the existing linkDockerCase flow end to end: only the final call differs. The docker-host upsert still applies, since it is what resolves the engine/context/daemon for `docker exec`; its create-time fields are simply never read for an attached case.
Two defects that only a real container exposes. The probe chained `command -v X && echo X` with semicolons, and a script's exit status is its last command's. A container without the last probed CLI made the whole `sh -lc` exit 1, so a perfectly healthy container with tmux and claude was reported as "could not exec into the container". A missing CLI is data here, not failure, so the script now ends with `exit 0`. containerWorkdir defaulted to hostWorkspacePath. That default holds for an owned container only because the create-time bind mount puts the host directory at that exact path; attaching mounts nothing, so the two are independent facts. A host path absent inside the container makes `docker exec --workdir` fail with an OCI chdir error that surfaces in the pane as a bare "execvp failed". The preflight now proves the directory exists inside the container and refuses at link time.
Attaching lived only on the Docker tab, but the place users look for anything container-shaped is the "Run in an isolated Docker container" checkbox on Create New. A feature nobody can find is a feature nobody has. Adds a one-click link there that switches to the Docker tab, turns the toggle on and focuses the container field. Reuses switchCaseModalTab and the existing sync helper; no new CSS.
The new strings were English only. Adding entries surfaced a deeper problem: the
translator matches whole text nodes and skips `code`/`pre`, so an inline `<code>`
mid-sentence splits a hint into fragments that can never match an entry — which is
why the panel's existing "Build it once with <code>...</code>" hint was never
translated either.
Drops the inline markup from the new hints so each is a single text node, then
adds the zh-CN entries. The brand name goes through the existing {name}
placeholder.
Server-side error bodies are deliberately not added: the client receives them
already interpolated with a concrete container name, so a template key could
never match.
Typing a container name from memory is error-prone. The field becomes a native datalist: pick from the engine's containers, type to filter, or type a name that is not listed (the engine may be remote, or the container may not exist yet). A datalist gives all three natively, so no dropdown state machine is introduced. Adds listDockerContainers and GET /api/docker-hosts/:hostId/containers, following the listRemoteCodemanSessions discovery precedent: read-only and never throwing, so an unreachable daemon returns an empty list and the field degrades to plain text instead of erroring. Stopped containers stay in the list, sorted after running ones and labelled. Attaching does require a running container, but hiding stopped ones turns "my container is not in the list" into a dead end, while showing `Exited (137) 8 days ago` says exactly what to fix.
The run-mode dropdown hides CLIs that are not installed on the HOST (Ark0N#201). That is right for local sessions and wrong for a container case, whose agents run inside the container: a host with no claude installed hides the mode while the container ships one, which is exactly what happened on a real deployment. The adoption preflight already probes what the container has, so that result is persisted on the case and surfaced through CaseInfo. Docker cases gate on it; every other case keeps the host probe unchanged. An absent list reads as "do not gate" rather than "nothing available": an owned container runs our base image, which ships every CLI, and treating unknown as empty would leave the menu with Shell alone.
The adoption preflight used the mode name as the binary name. claude, codex, opencode, gemini and pi happen to match, so it never showed — but antigravity ships as `agy` and deepseek as `dsh`, so a container that has either was reported as not having it, and the mode was silently dropped from the case. Adds a MODE_BINARIES map, single-sourced with defaultDockerCommandForMode, which launches those same binaries. Probing and result filtering share one `binaryFor` so the two cannot drift apart.
…ch time Storing the container's CLIs on the case at attach time left two gaps: a case linked before that field existed has none at all, and a container's CLIs can be installed or removed long after it was linked. A real deployment hit the first one — the host had only codex, the container only claude, and with no stored list the menu still gated on the host and hid the mode that actually worked. The probe now runs when a container case is selected, reusing the existing adopt-preflight endpoint, so there is no new backend surface. Results are cached per case for the page's lifetime, since the menu opens often and the probe is a `docker exec` round trip; a concurrent probe for the same case is deduplicated with an in-flight marker. A failed probe leaves the cache empty, which the caller reads as "unknown" and therefore does not gate. Hiding every mode because one probe failed is worse than offering one that turns out to be missing, which the launch path already refuses with a specific message. The repaint only happens while the menu is still open, so a late answer cannot make the list jump under a user who already closed it.
Attaching a container, picking claude and hitting Run gave one line — `execvp(3) failed.: No such file or directory` — and the run-mode menu offered every mode. Three separate defects, found on a real deployment. TmuxManager.createSession resolved the CLI directory without distinguishing a docker session, so a host with no claude threw, the catch fell back to a direct PTY, and that PTY exec'd the CLI on the HOST. The failure surfaced as a bare execvp error naming nothing. A docker session runs its CLI inside the container; the host does not need it. All eight modes now sit behind a cliRunsInContainer guard, and whether the container has the CLI is settled by the adoption preflight or the image gate before launch. The running check used a bare double quote and command substitution. The whole chain is embedded in an outer `bash -c "…"`, so the unescaped quote closed that string early and the remainder was re-tokenized. It is now a `grep -qx` pipeline using only the single-quote form every other line in the builder already uses. Claude Code refuses --dangerously-skip-permissions as root. Our base image runs a non-root user, so an owned container never hit this; an adopted container's user belongs to its owner and is frequently root, and keeping the flag killed the pane with a message visible only inside the container. The preflight now reports runsAsRoot and the launch chain drops the flag for it. The menu also showed every mode because the container CLI probe only started when the menu opened. It is warmed when the case is selected instead.
Link Existing's Browse did nothing: GET /api/filesystem/browse answered 403 "No filesystem browse roots are available". Two rules were fighting. /root is a default blocked tree in the attachment guard, and Codeman running as root — containers, plenty of servers — makes homedir() exactly /root, so the picker's own allowlisted Home root was blocked; the other candidates live under it or do not exist. The root list came out empty and there was nothing the user could open. The blocked trees exist to keep ~/.ssh and friends out of reach, not to seal off the user's own home. Only trees that would swallow a configured root whole are dropped now: /root goes when Home is it (or sits inside it), /etc holds no configured root and is untouched. Secrets stay protected — isSensitivePath independently matches .ssh/, .env and credentials* at any depth, and it is what the directory probe asks about.⚠️ Navigation must reuse the same narrowed list the roots were chosen with. Handing the raw trees downstream admits a root and then refuses every path inside it, which reads as a picker that opens and does nothing.
Both paths in the adoption form had to be typed. Each gets a Browse button using the same path-input-group markup Link Existing uses, so the two look and behave alike. What they can browse differs, and that is the point. The host workspace path reuses the existing host picker. The container workdir cannot: an adopted container has nothing mounted at a matching host path, so a host listing would be a different filesystem — and getting this field wrong is the source of the opaque OCI chdir error at launch, which makes it the field that most needs to be clickable. Adds a read-only POST /api/docker-cases/browse: one `ls` through docker exec, no writes, no lifecycle, path shell-escaped like every other value. `ls -Ap` marks directories with a trailing slash and keeps names with spaces intact. PathPicker takes an optional fetchListing source rather than being forked: the container variant only swaps where the rows come from, and reuses the rendering, navigation, Up and Choose/Select unchanged.
…able container
The run menu still offered every mode for an attached container. The browser's
actual request showed why:
POST /api/docker-cases/adopt-preflight -> 400
{"error":"Invalid input: expected object, received string"}
_api serializes `body` and sets Content-Type itself, and three call sites each
passed an already-stringified body, so it was encoded twice and the server saw a
JSON string where it expects an object. curl was fine throughout, so nothing in
the server logs pointed at it.
Also fixes the design defect underneath: a failed probe fell through to "do not
gate", which silently offered every mode. When the container has been recreated,
is stopped, or the engine is unreachable, the user sees claude, clicks it, and
it can only fail — with the reason visible nowhere. A failed probe now hides
every agent mode (Shell needs no CLI and stays) and shows the server's own
reason at the top of the menu.
Two static guards switched from a character window to brace matching. They
sliced between two call sites, and _loadRunModeHistory's call appears above its
definition, so the slice came out empty and the assertion verified nothing —
the same trap twice in one file.
Format check failed twice, on different files each time, because three prettier versions were in play: package.json says ^3.4.0, package-lock pins 3.8.3 (CI runs npm ci, so that is the one CI uses), and the local node_modules had 3.9.6. Files formatted with 3.9.6 were then "fixed" with 3.4.2, pushing session-routes and system-routes onto a third style — every version change moved the failure to a different set of files. Line-break placement in `await import` and a union type only; no logic changes.
The home screen now lists tmux sessions Codeman did not create — a `claude`
or `codex` someone started inside `tmux new -s work`, or just a shell — and
one click turns one into a tab you can keep working in.
Adoption is a FOURTH location overlay, structurally identical to remote-SSH
and Docker, and deliberately NOT a new `SessionMode`: the outer session is
still an ordinary `codeman-<8hex>` on this instance's own socket, and only
what runs inside its pane differs. That indirection is what leaves the mux
name allowlist, capture, input and recovery paths completely untouched, and
it makes "detach, never kill" structural rather than a rule to remember —
`killSession` can only ever reach the wrapper we created.
One probe script, one parser and one classifier serve all three locations;
only the transport differs (direct / `docker exec` / `ssh`). The mode comes
from a bounded process-tree walk over the pane's descendants, because
`#{pane_current_command}` is `node` for BOTH claude and codex. Anything not
recognised is `shell`, which is also the honest answer for the case this was
built for.
Three things were settled by measurement rather than by reasoning, and each
is recorded where the code would otherwise invite the wrong change:
- A grouped session buys an independent `status off` and current window, but
NOT an independent size. Measured on tmux 3.3a against a target held by a
200x49 client: a bare attach and a grouped session BOTH shrink it to
80x23; only `window-size largest` protects it, and that is a WINDOW option
on a SHARED window which survives our detach — so it is deliberately not
set, and attaching resizes like any second tmux client does.
- View collection differs per location: a local client dies with its pane and
ssh propagates SIGHUP, but a `docker exec` outlives its client, so the
in-container view has to be collected explicitly or every adoption leaks
one plus its exec process.
- A foreign session name is chosen by someone else, and the local launch
chain ends at `bash -c ${JSON.stringify(cmd)}`. `JSON.stringify` escapes
`"` and `\` but not `$` or a backtick, and the outer shell substitutes
before the inner single quotes apply. Names and socket paths therefore go
through a character allowlist and are dropped during DISCOVERY, so an
unusable candidate never gets an id for a caller to send.
Capabilities degrade on "who launched this process": an adopted session has
no hooks, no envOverrides, no effort, and a working directory that is merely
the foreign pane's cwd — a path that need not exist on this host at all. So
respawn, Ralph, the orchestrator, hook-backed waits and every watcher keyed
on a local workingDir refuse or skip, and the close dialog no longer offers
a "kill the session" option it cannot honour.
|
#357 is merged — details and the conflict resolutions are in #375 (comment). This branch needs the same rebase onto master. Since #375 already contains the tmux adoption commit, it may be simplest to close this one in favour of that PR and keep the discussion in one place — your call, though; if you'd rather land the tmux adoption on its own first, a rebased #364 is the easier review of the two and I'm happy to take it that way. |
|
Thanks for this, and sorry it has taken a while. Adopting a tmux session a human started, in all three locations, is a genuinely good idea and the design fits the codebase: a wrapper session on Codeman's own socket, a probe/parse/classify core, capability gating. The full gate passes on your head. Five things block it though, and four of them are a few lines each. 1. 2. Docker discovery passes shell-quoted engine flags to 3. The one-wrapper-per-target key ignores location ( 4. The boot-time hook sweep writes Codeman hooks into an adopted local session's workspace on every restart ( 5. Then two more that are about process rather than behaviour: 6. Rebase. 7. Tests outside the pure core. The 33 unit tests pin the probe, parser, classifier, allowlist and builders well. Nothing covers Smaller: Please rebase first, that alone should shrink this a lot, then the five majors are each a few lines. The docs I can do at merge time if you would rather. The feature is well built and speaks the codebase's own design language, and I want it in. One coordination note: #375 from you carries an overlapping foreign-adoption implementation on top of commits master already has. Rather than reviewing both, tell me which one you want to be the adoption PR and I will close the other. |
|
Closing this in favour of #375, which carries the same foreign-adoption work plus the multi-case adopted-container extension. To be clear about what that does and does not mean: this is a routing decision, not a rejection. The review above still stands and none of it is wasted, because the same code is in #375 and the same seven items apply to it there. Carry them across:
One thing worth saying plainly, since it makes #375 the harder branch to land rather than the easier one: #364 needed only a rebase and five small fixes, while #375 needs a rebase that drops 14 commits AND a split into three PRs. The adoption feature is split 1 there. If, once you start, you find it is less work to reopen this branch and land adoption from here instead, say so and I will reopen it. I would rather land the feature than defend a filing decision. Thanks for both, and sorry it took this long to get you a written answer on either. |
2 participants
Adds adoption: a tmux session a human started outside Codeman shows up on the home screen, and one click turns it into a tab you can keep working in. Works for sessions on this host, inside a container, and across ssh.
Why it is not a new
SessionModeAdoption is the fourth location overlay, structurally identical to remote-SSH and Docker. The mode is still
claude/codex/shell, decided by a probe rather than by the user.The outer session stays an ordinary
codeman-<8hex>on this instance's own socket; only what runs inside its pane differs. That indirection is the whole design:killSessioncan only address our own socket, so no shape of caller bug reaches the foreign server — the same reasoning behind the non-owned remote early return it sits next toOne probe, three transports
A single POSIX-sh probe (
tmux list-panesper socket + onepssnapshot, no logic), one parser and one classifier serve all three locations; only the shell around it differs (direct /docker exec/ssh). The mode comes from a bounded process-tree walk, because#{pane_current_command}isnodefor both claude and codex. Anything unrecognised isshell— which is also the honest answer for the case this was built for.Three things settled by measurement
Each is recorded next to the code that would otherwise invite the wrong change.
Grouping does not give an independent window size. Measured on tmux 3.3a against a target held open by a 200x49 client, with ours at 80x24:
attachwindow-size largestA window is one object with one size and a group shares it.
window-size largestis deliberately not set: it is a WINDOW option on a SHARED window and survives our detach, so it would permanently rewrite the owner's configuration. Grouping is still used — it is what keeps ourstatus offand current-window off the owner's session. Attaching therefore resizes like any second tmux client does, and reverses on detach.View collection differs per location. A local client dies with its pane; ssh propagates SIGHUP. A
docker execprocess outlives its client, so the in-container view has to be collected explicitly — without it every adoption leaked a view session plus an exec process (measured).A foreign session name is attacker-influenced. The local launch chain ends at
bash -c ${JSON.stringify(cmd)}.JSON.stringifyescapes"and\but not$or a backtick, and the outer shell substitutes before the inner single quotes apply:Names and socket paths now go through a character allowlist and are dropped during discovery, so an unusable candidate never gets an id for a caller to send, and the adopt endpoint (which re-resolves through the same code) fails closed for free. Skipped candidates are reported to the user rather than silently missing.
Capability degradation
Keyed on who launched the process, deliberately separate from
isExternalCliMode()(which answers does this CLI draw its own TUI). An adopted session has no hooks, noenvOverrides, no effort, and aworkingDirthat is merely the foreign pane's cwd — a path that need not exist on this host. So respawn, Ralph, the orchestrator, hook-backed waits and every watcher keyed on a local working directory refuse or skip, and the close dialog no longer offers a "kill the session" option it cannot honour.Cost policy
Local discovery is TTL-cached and polled by the browser only while the home screen is open. Docker and remote are on demand — opening the home page must never fan out one ssh connection per saved host.
Verification
Driven through a real browser, plus direct
tmux/docker exec/sshchecks that never trust Codeman's own self-report.typecheck,lint,format:check,check:frontend-syntax,check:public-assetsclean; full suite green (6363 passed). 33 new unit tests pin the probe/parse/classify core, the injection gate, and the three attach-command builders.Known limits
tmux -S /custom/pathis out of scope; only$TMUX_TMPDIR/tmux-<uid>/is swept, which covers everytmux -L <name>server.buildSshConnectionArgs'BatchMode=yes, so a password-only host cannot be used — it needs anidentityFile, like every other remote path here.