mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-11-15 02:49:33 +01:00
fix(adopt): never drive, write to, or answer for a session we did not start
Review items 3, 4 and the smaller ones on #510. An adopted session wraps a process someone else launched, and its mode is whatever the probe saw — very often `claude`. Every guard below is therefore its own `isAdopted` check rather than a side effect of the external-CLI rule: the two ask different questions, and merging them means the day the first loosens, Codeman starts acting on a live conversation that is not ours. - The boot workspace-hook sweep skipped only non-claude and `session.remote`. An adopted session keeps its connection on `adopt.remote` / `adopt.docker`, so a tab wrapping someone's in-container or over-ssh claude looked local and Codeman wrote `.claude/settings.local.json` into their repo on every restart. - The workspace-trust auto-accept read the pane and pressed keys for 90s after `startInteractive()`. In an adopted pane that is someone else's dialog, and boot recovery re-opens the window on every restart. - Auto-clear, auto-compact, auto-resume and the Ralph auto-enable in `POST /interactive` all drove the pane. Each now refuses with its own gate. - The wrapper's `workingDir` was the foreign pane's cwd. That cwd can carry characters outside SAFE_PATH_PATTERN (`~/c++`, `Program Files (x86)`), which makes `createSession` throw so the tab never starts, and for a container or ssh candidate it is not a path on this host at all. It is now a neutral local `/tmp`, the same cwd `resolveMuxAttachCwd` already gives the pane; the foreign cwd stays on `adopt.paneCurrentPath` for display. - The CLI signature table hardcoded ids, which CLAUDE.md forbids outside `stock.ts`, and had already gone stale: `omp` was missing, so a hand-started omp pane read as `shell`. It is now derived from each entry's `discovery.binaries` at call time, regex-escaped, longest name first. Stock entries only, since `SessionMode` is the closed union of stock ids. - The descendant walk now passes `onTruncated`. A capped walk returns "nothing matched", which is byte-identical to the answer for a real shell. - The home screen's wide scan is one-shot. Riding `docker exec` and a full ssh handshake per target, each with a ~12s timeout, on the 8s poll stacked connections on a slow host. Wide rows are kept apart and merged, because a local-only payload cannot contain them. - Adoption failures report the server's own message. `_apiJson` folds a non-2xx to null, which collapsed "could not reach it just now" into "that session is gone" and reported a live remote session as deleted on every link flicker. Tests: route tests for the refusals, the admin gate, the 404 re-resolve, the one-wrapper-per-target reuse and the adopt round-trip; unit tests for the two non-HTTP guards, the registry-derived signatures, truncation reporting and the frontend's scan and error paths. Each guard was removed in turn to confirm its own tests go red.
This commit is contained in:
1 parent
aec4867821
commit
09ccffd32e
11 files changed
+954
-46
No files matched your search
@@ -21,11 +21,17 @@
|
||||
* tab keeps sweeping tmux sockets forever. `startForeignPolling` /
|
||||
* `stopForeignPolling` are called from `showWelcome`/`hideWelcome`.
|
||||
*
|
||||
* ⚠️ Docker and remote locations are NOT polled. Each costs a `docker exec` or a
|
||||
* full ssh handshake per target, and doing that on every home-screen load is the
|
||||
* one cost the server-side design explicitly refuses. They are fetched only when
|
||||
* the user asks, via the "scan containers & hosts" toggle, which then rides along
|
||||
* with the same poll.
|
||||
* ⚠️ Docker and remote locations are NOT in that poll. Each costs a `docker exec`
|
||||
* or a full ssh handshake per target, each with a ~12s timeout, so riding them
|
||||
* along on an 8s tick stacks connections on a slow host — the one cost the
|
||||
* server-side design explicitly refuses. The wide scan is therefore ONE-SHOT:
|
||||
* turning the toggle on fetches once, and after that it is the user's Rescan
|
||||
* button. The periodic poll stays local-only forever.
|
||||
*
|
||||
* The wide rows are kept separately (`_foreignWideSessions`) and merged into
|
||||
* every local render, because a local-only poll's payload does not contain them
|
||||
* and would otherwise wipe the container/remote rows off the list one tick after
|
||||
* they appeared. Turning the toggle off discards them.
|
||||
*
|
||||
* ⚠️ Adoption creates a REAL session, so the button locks while in flight and the
|
||||
* result goes through the app's normal idempotent create path
|
||||
@@ -74,6 +80,7 @@ Object.assign(CodemanApp.prototype, {
|
||||
if (this._foreignPollTimer) return;
|
||||
void this.loadForeignSessions();
|
||||
this._foreignPollTimer = setInterval(() => {
|
||||
// Local only — see @fileoverview. The wide scan never rides the timer.
|
||||
void this.loadForeignSessions();
|
||||
}, this._foreignPollMs || FOREIGN_POLL_FALLBACK_MS);
|
||||
},
|
||||
@@ -90,19 +97,46 @@ Object.assign(CodemanApp.prototype, {
|
||||
return this._foreignScanRemote === true;
|
||||
},
|
||||
|
||||
/**
|
||||
* Flip the wide scan. ON fetches containers and hosts ONCE; OFF discards what
|
||||
* that fetch found, because those rows are not re-confirmed by any later poll
|
||||
* and leaving them up would show a container session that may be long gone.
|
||||
*/
|
||||
toggleForeignScanRemote() {
|
||||
this._foreignScanRemote = !this._foreignScanRemote;
|
||||
void this.loadForeignSessions();
|
||||
if (!this._foreignScanRemote) {
|
||||
this._foreignWideSessions = [];
|
||||
this._foreignWideNotes = [];
|
||||
this.renderAllForeignSessions();
|
||||
return;
|
||||
}
|
||||
void this.loadForeignSessions({ wide: true });
|
||||
},
|
||||
|
||||
/** Re-run the expensive scan on demand. The only way wide rows ever refresh. */
|
||||
rescanForeignWide() {
|
||||
if (!this.foreignScanRemote()) return;
|
||||
void this.loadForeignSessions({ wide: true });
|
||||
},
|
||||
|
||||
// ═══════════════════════════════════════════════════════════════
|
||||
// Data
|
||||
// ═══════════════════════════════════════════════════════════════
|
||||
|
||||
async loadForeignSessions() {
|
||||
const wide = this.foreignScanRemote();
|
||||
/**
|
||||
* Fetch the list. `wide` asks the server for containers and ssh hosts too, and
|
||||
* ONLY an explicit user action ever passes it — never the interval.
|
||||
*/
|
||||
async loadForeignSessions({ wide = false } = {}) {
|
||||
const qs = wide ? '?docker=1&remote=1' : '';
|
||||
const data = await this._apiJson(`/api/mux/foreign${qs}`);
|
||||
if (wide) this._foreignWideLoading = true;
|
||||
this.renderAllForeignSessions();
|
||||
let data;
|
||||
try {
|
||||
data = await this._apiJson(`/api/mux/foreign${qs}`);
|
||||
} finally {
|
||||
if (wide) this._foreignWideLoading = false;
|
||||
}
|
||||
if (!data) {
|
||||
// A failed poll must not blank a list the user is looking at: keep the
|
||||
// last good result and let the next tick recover.
|
||||
@@ -111,13 +145,34 @@ Object.assign(CodemanApp.prototype, {
|
||||
return;
|
||||
}
|
||||
this._foreignError = false;
|
||||
this._foreignSessions = Array.isArray(data.sessions) ? data.sessions : [];
|
||||
this._foreignNotes = Array.isArray(data.notes) ? data.notes : [];
|
||||
const rows = Array.isArray(data.sessions) ? data.sessions : [];
|
||||
const notes = Array.isArray(data.notes) ? data.notes : [];
|
||||
if (wide) {
|
||||
// Split by location: the local rows refresh on every tick, the rest only
|
||||
// here, so they have to be stored apart or the next local-only payload
|
||||
// (which cannot contain them) would read as "those sessions are gone".
|
||||
this._foreignSessions = rows.filter((r) => (r.location || 'local') === 'local');
|
||||
this._foreignWideSessions = rows.filter((r) => (r.location || 'local') !== 'local');
|
||||
this._foreignNotes = notes;
|
||||
this._foreignWideNotes = [];
|
||||
this._foreignWideAt = Date.now();
|
||||
} else {
|
||||
this._foreignSessions = rows;
|
||||
this._foreignNotes = notes;
|
||||
}
|
||||
this._foreignCanScanWide = data.canScanWide === true;
|
||||
if (Number.isFinite(data.pollIntervalMs)) this._foreignPollMs = data.pollIntervalMs;
|
||||
this.renderAllForeignSessions();
|
||||
},
|
||||
|
||||
/** Local rows plus whatever the last wide scan found. The render's one input. */
|
||||
_allForeignRows() {
|
||||
const local = Array.isArray(this._foreignSessions) ? this._foreignSessions : [];
|
||||
if (!this.foreignScanRemote()) return local;
|
||||
const wide = Array.isArray(this._foreignWideSessions) ? this._foreignWideSessions : [];
|
||||
return wide.length ? local.concat(wide) : local;
|
||||
},
|
||||
|
||||
/**
|
||||
* Adopt one candidate.
|
||||
*
|
||||
@@ -138,22 +193,30 @@ Object.assign(CodemanApp.prototype, {
|
||||
body.docker = true;
|
||||
body.remote = true;
|
||||
}
|
||||
const data = await this._apiJson('/api/sessions/adopt', { method: 'POST', body });
|
||||
if (!data || !data.session) {
|
||||
this.showToast?.(
|
||||
codemanT
|
||||
? codemanT('That session is gone. Refreshing the list.')
|
||||
: 'That session is gone. Refreshing the list.',
|
||||
'error'
|
||||
);
|
||||
// ⚠️ `_api`, not `_apiJson`: the latter folds a network failure, a non-2xx
|
||||
// and `success:false` all into null, and the server deliberately tells
|
||||
// two different stories here — "that tmux session is no longer there" vs
|
||||
// "could not reach it just now (…) — try again". Collapsing them showed a
|
||||
// live remote session as deleted every time the link flickered.
|
||||
const res = await this._api('/api/sessions/adopt', { method: 'POST', body });
|
||||
const payload = await this._readJsonBody(res);
|
||||
const session = payload && payload.success !== false ? payload.data?.session : null;
|
||||
if (!session) {
|
||||
const fallback = codemanT
|
||||
? codemanT('Could not open that session. Refreshing the list.')
|
||||
: 'Could not open that session. Refreshing the list.';
|
||||
this.showToast?.(payload?.error || fallback, 'error');
|
||||
await this.loadForeignSessions();
|
||||
return;
|
||||
}
|
||||
const data = { session };
|
||||
// Go through the app's normal create path so tab order, lineage lines and
|
||||
// SSE-vs-POST ordering behave exactly as they do for a Run.
|
||||
this._onSessionCreated?.(data.session);
|
||||
await this.selectSession(data.session.id);
|
||||
await this.loadForeignSessions();
|
||||
// Still one explicit user action, so a wide row's "Open" refreshes the
|
||||
// wide list too — otherwise the row just adopted keeps reading as free.
|
||||
await this.loadForeignSessions({ wide: this.foreignScanRemote() });
|
||||
} finally {
|
||||
this._foreignAdoptInFlight = false;
|
||||
if (buttonEl) {
|
||||
@@ -163,6 +226,16 @@ Object.assign(CodemanApp.prototype, {
|
||||
}
|
||||
},
|
||||
|
||||
/** The error envelope, or null when there was no readable body at all. */
|
||||
async _readJsonBody(res) {
|
||||
if (!res) return null;
|
||||
try {
|
||||
return await res.json();
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
},
|
||||
|
||||
// ═══════════════════════════════════════════════════════════════
|
||||
// Render
|
||||
// ═══════════════════════════════════════════════════════════════
|
||||
@@ -183,7 +256,7 @@ Object.assign(CodemanApp.prototype, {
|
||||
* near `innerHTML`.
|
||||
*/
|
||||
renderForeignSessions(container) {
|
||||
const rows = Array.isArray(this._foreignSessions) ? this._foreignSessions : [];
|
||||
const rows = this._allForeignRows();
|
||||
container.innerHTML = '';
|
||||
|
||||
if (!rows.length) {
|
||||
@@ -192,7 +265,9 @@ Object.assign(CodemanApp.prototype, {
|
||||
// A note is a REASON the list is empty, so a block carrying one must stay
|
||||
// visible — hiding it is exactly how "where did my session go" becomes
|
||||
// unanswerable.
|
||||
const hasNotes = Array.isArray(this._foreignNotes) && this._foreignNotes.length > 0;
|
||||
const hasNotes =
|
||||
(Array.isArray(this._foreignNotes) && this._foreignNotes.length > 0) ||
|
||||
(Array.isArray(this._foreignWideNotes) && this._foreignWideNotes.length > 0);
|
||||
// ⚠️ `canScanWide` must keep the block visible even with nothing to show.
|
||||
// The scan toggle lives in the header, so hiding an empty block also hides
|
||||
// the only control that could fill it — on a host with containers but no
|
||||
@@ -242,7 +317,9 @@ Object.assign(CodemanApp.prototype, {
|
||||
* as text.
|
||||
*/
|
||||
_appendForeignNotes(container) {
|
||||
const notes = Array.isArray(this._foreignNotes) ? this._foreignNotes : [];
|
||||
const local = Array.isArray(this._foreignNotes) ? this._foreignNotes : [];
|
||||
const wide = this.foreignScanRemote() && Array.isArray(this._foreignWideNotes) ? this._foreignWideNotes : [];
|
||||
const notes = wide.length ? local.concat(wide) : local;
|
||||
if (!notes.length) return;
|
||||
const box = document.createElement('div');
|
||||
box.className = 'foreign-notes';
|
||||
@@ -284,6 +361,27 @@ Object.assign(CodemanApp.prototype, {
|
||||
scan.textContent = this.foreignScanRemote() ? '⟳ all' : '⟳ local';
|
||||
header.appendChild(scan);
|
||||
|
||||
// The wide scan is one-shot, so without this button its rows could only
|
||||
// ever be refreshed by toggling off and on again.
|
||||
if (this.foreignScanRemote()) {
|
||||
const rescan = document.createElement('button');
|
||||
rescan.type = 'button';
|
||||
rescan.className = 'foreign-scan-toggle';
|
||||
rescan.dataset.foreignAction = 'rescan-wide';
|
||||
rescan.disabled = this._foreignWideLoading === true;
|
||||
rescan.title = codemanT
|
||||
? codemanT('Scan containers and remote hosts again')
|
||||
: 'Scan containers and remote hosts again';
|
||||
rescan.textContent = this._foreignWideLoading
|
||||
? codemanT
|
||||
? codemanT('Scanning…')
|
||||
: 'Scanning…'
|
||||
: codemanT
|
||||
? codemanT('Rescan')
|
||||
: 'Rescan';
|
||||
header.appendChild(rescan);
|
||||
}
|
||||
|
||||
return header;
|
||||
},
|
||||
|
||||
@@ -353,6 +451,8 @@ Object.assign(CodemanApp.prototype, {
|
||||
void this.selectSession(el.dataset.foreignSession);
|
||||
} else if (action === 'toggle-scan') {
|
||||
this.toggleForeignScanRemote();
|
||||
} else if (action === 'rescan-wide') {
|
||||
this.rescanForeignWide();
|
||||
}
|
||||
});
|
||||
},
|
||||
|
||||
@@ -1191,6 +1191,13 @@ export function registerSessionRoutes(
|
||||
* grant gates elsewhere. The admin gate subsumes it, so there is deliberately
|
||||
* no second grant check here.
|
||||
*/
|
||||
/**
|
||||
* The wrapper pane for an adopted session runs `tmux attach`, never the agent,
|
||||
* so it needs no workspace — and must not borrow the foreign one. Same value
|
||||
* `resolveMuxAttachCwd()` gives remote and docker panes.
|
||||
*/
|
||||
const ADOPTED_WRAPPER_WORKING_DIR = '/tmp';
|
||||
|
||||
app.post('/api/sessions/adopt', async (req, reply) => {
|
||||
if (isMultiUserMode() && !requireAdmin(req, reply)) return;
|
||||
|
||||
@@ -1277,13 +1284,16 @@ export function registerSessionRoutes(
|
||||
|
||||
const adoptHistoryConfig = await ctx.getTerminalHistoryConfig();
|
||||
|
||||
// ⚠️ `workingDir` for an adopted session is the FOREIGN pane's cwd, which may
|
||||
// not exist on this host (a container path, a remote path). It is recorded as
|
||||
// an observation for display; the wrapper pane is never `cd`'d into it, and
|
||||
// the case-space confinement that guards a real workingDir does not apply
|
||||
// because nothing is created there.
|
||||
// ⚠️ The wrapper's `workingDir` is a NEUTRAL LOCAL path, never the foreign
|
||||
// pane's cwd. That cwd is an observation about another machine: it can carry
|
||||
// characters outside SAFE_PATH_PATTERN (`~/c++`, `Program Files (x86)`), which
|
||||
// makes `createSession` throw so the tab never starts at all, and when it is a
|
||||
// container or ssh path every local consumer of `workingDir` — the Files panel,
|
||||
// the boot hook sweep, the fs watchers — reads it as a path on THIS host. The
|
||||
// foreign cwd stays on `adopt.paneCurrentPath`, which is for display only.
|
||||
// `/tmp` is the same neutral cwd `resolveMuxAttachCwd` already gives the pane.
|
||||
const session = new Session({
|
||||
workingDir: target.workingDir || process.cwd(),
|
||||
workingDir: ADOPTED_WRAPPER_WORKING_DIR,
|
||||
mode: target.mode,
|
||||
name: body.name || target.sessionName,
|
||||
mux: ctx.mux,
|
||||
@@ -1712,6 +1722,10 @@ export function registerSessionRoutes(
|
||||
// and belongs in its own PR, not in a refactor that is meant to change nothing.
|
||||
if (
|
||||
!isExternalCliMode(session.mode) &&
|
||||
// ⚠️ And never for an adopted session, whose mode can be claude while the
|
||||
// agent in that pane belongs to someone else. Kept as its own clause for
|
||||
// the same reason the respawn and Ralph routes keep theirs separate.
|
||||
!session.isAdopted &&
|
||||
ctx.store.getConfig().ralphEnabled &&
|
||||
!session.ralphTracker.autoEnableDisabled
|
||||
) {
|
||||
@@ -3303,6 +3317,16 @@ export function registerSessionRoutes(
|
||||
const { id } = req.params as { id: string };
|
||||
const body = parseBody(AutoClearSchema, req.body, 'Invalid request body');
|
||||
const session = findSessionOrFail(ctx, id, req);
|
||||
// ⚠️ Separate from the external-CLI gate on purpose: an adopted session's mode
|
||||
// can perfectly well BE claude, and this drives the pane of a session someone
|
||||
// else is using. Merging the two checks means a future change to the
|
||||
// external-CLI rule silently reopens this one.
|
||||
if (session.isAdopted) {
|
||||
return createErrorResponse(
|
||||
ApiErrorCode.INVALID_INPUT,
|
||||
'Auto-clear is not available for adopted sessions: Codeman did not start this agent and must not drive it'
|
||||
);
|
||||
}
|
||||
|
||||
session.setAutoClear(body.enabled, body.threshold);
|
||||
persistAndBroadcastSession(ctx, session);
|
||||
@@ -3324,6 +3348,16 @@ export function registerSessionRoutes(
|
||||
const { id } = req.params as { id: string };
|
||||
const body = parseBody(AutoCompactSchema, req.body, 'Invalid request body');
|
||||
const session = findSessionOrFail(ctx, id, req);
|
||||
// ⚠️ Separate from the external-CLI gate on purpose: an adopted session's mode
|
||||
// can perfectly well BE claude, and this drives the pane of a session someone
|
||||
// else is using. Merging the two checks means a future change to the
|
||||
// external-CLI rule silently reopens this one.
|
||||
if (session.isAdopted) {
|
||||
return createErrorResponse(
|
||||
ApiErrorCode.INVALID_INPUT,
|
||||
'Auto-compact is not available for adopted sessions: Codeman did not start this agent and must not drive it'
|
||||
);
|
||||
}
|
||||
|
||||
session.setAutoCompact(body.enabled, body.threshold, body.prompt);
|
||||
persistAndBroadcastSession(ctx, session);
|
||||
@@ -3346,6 +3380,16 @@ export function registerSessionRoutes(
|
||||
const { id } = req.params as { id: string };
|
||||
const body = parseBody(AutoResumeSchema, req.body, 'Invalid request body');
|
||||
const session = findSessionOrFail(ctx, id, req);
|
||||
// ⚠️ Separate from the external-CLI gate on purpose: an adopted session's mode
|
||||
// can perfectly well BE claude, and this drives the pane of a session someone
|
||||
// else is using. Merging the two checks means a future change to the
|
||||
// external-CLI rule silently reopens this one.
|
||||
if (session.isAdopted) {
|
||||
return createErrorResponse(
|
||||
ApiErrorCode.INVALID_INPUT,
|
||||
'Auto-resume is not available for adopted sessions: Codeman did not start this agent and must not drive it'
|
||||
);
|
||||
}
|
||||
|
||||
session.setAutoResume(body.enabled);
|
||||
persistAndBroadcastSession(ctx, session);
|
||||
|
||||
@@ -3796,6 +3796,13 @@ export class WebServer extends EventEmitter {
|
||||
const workspaces = new Set<string>();
|
||||
for (const session of this.sessions.values()) {
|
||||
if (session.mode !== 'claude' || session.remote) continue;
|
||||
// ⚠️ An ADOPTED session is not caught by the `remote` check above: its
|
||||
// connection facts live on `adopt.remote` / `adopt.docker`, so a session
|
||||
// wrapping someone's in-container or over-ssh claude looks local here.
|
||||
// Its workspace was never Codeman's to begin with, so installing hooks
|
||||
// there writes `.claude/settings.local.json` into the human's own repo —
|
||||
// on EVERY restart, since this sweep runs from boot recovery.
|
||||
if (session.isAdopted) continue;
|
||||
if (session.docker && !session.docker.hooksEnabled) continue;
|
||||
if (session.workingDir) workspaces.add(session.workingDir);
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user