Handoff reliability + read_output screen view (plan 3712ddc)

README.md

@@ -170,6 +170,11 @@ GitHub Copilot を選んだ場合:
 - シェルエラーが出てしまった場合、それを収拾しようとして空入力や Ctrl+C 相当の入力を送ってはいけません。継続入力待ちのシェルに対しては EOF のように作用し、**シェルプロセスごと終了させてしまうことがあります** (実際に一度そうなりました)。`get_tab_status` で `exited: true` を確認したら、そのタブは諦めて `close_tab` → `open_tab` で作り直してください。
 - 新規に開いたタブに何かを送る前には、`read_output` で実際にアプリの TUI が描画されていることを確認してから送ってください。
 
+#### control MCP ツールの信頼性保証 (handoff と read_output)
+
+- **ハンドオフは失われません**: `wait_for_handoff` はタイムアウト (`{timedOut:true}`) 時に**そのままもう一度呼ぶだけで安全**です。誰も待っていない間に届いたハンドオフはキューに残り、また**待機中に接続が切れても**イベントを消費しないため、再接続後の次の `wait_for_handoff` が必ず受け取ります。サーバー再起動後も未受信ハンドオフは残っています。
+- **`read_output` の `screen` / `screenAlt` / `screenIdleMs` を使う**: ワーカーのスピナー等の動的描画はカーソル移動と行消去でその場を書き換えるため、生のバイト列 (`raw` / `text`) からは「今見えている画面」を復元できません。サーバーはセッションごとに軽量な仮想画面 (ANSI 解釈) を維持しており、`screen` が現在の可視画面、`screenIdleMs` が**画面が最後に変化してからの経過** (スピナーが回っていれば小さい値、静止プロンプトなら大きい値) です。stuck/busy 判定は `text` や `idleForMs` (バイトベース) よりこれらを優先してください。`get_tab_status` の `screenIdleMs` も同様です。
+
 #### オーケストレーターから見えるのは repo_info の基本情報だけ
 
 オーケストレーターのサンドボックスにワーカーのディレクトリは**マウントされません** (プロジェクトファイルへの直接アクセスは不可)。代わりに、control MCP サーバーのツール `repo_info` がグループのプロジェクト (cwd) の**基本情報だけ**を返します: トップレベルの構成 (ディレクトリ/ファイル名のみ、100 エントリ上限)、README の先頭 ~8KB、`package.json` の要約 (name/version/description と scripts/dependencies/devDependencies の**キー一覧のみ**、値は返さない、各 50 キー上限)、git 状態 (現在ブランチ / short HEAD / 直近 5 コミットの件名 / 変更ファイル数)。
@@ -319,6 +324,7 @@ ccserver/
 │       ├── mcpServer.js            # control / handoff / notify 各 MCP サーバー (SocketTransport 含む)
 │       ├── mcpBroker.js            # Unix-socket MCP ブローカー (control/handoff はグループ毎、notify はプロセス毎 1 つ)
 │       ├── mcpTools.js             # control/handoff ツールの実装 (deps 注入)
+│       ├── screenModel.js          # read_output 用の軽量仮想画面 (ANSI 解釈 + 変化検知)
 │       ├── sandbox.js              # bwrap + rootless docker サンドボックス構築
 │       ├── sandbox-entrypoint.sh
 │       ├── sandbox-gh-wrapper.cjs         # サンドボックス内 gh をブローカー中継に差し替え

server/routes/groups.js

@@ -58,11 +58,16 @@ the MCP server "ccserver" that is already configured in this session.
 Each worker is a full terminal session you can inspect and control:
 
 - list_group_sessions -- see the members of this group.
-- read_output -- read a member's recent terminal output (fallback for
-  inspecting a stuck member; avoid polling it).
+- read_output -- read a member's current screen / recent terminal output
+  (fallback for inspecting a stuck member; avoid polling it). Use its
+  \`screen\` and \`screenIdleMs\` fields for stuck/busy judgments -- a static
+  screen (large screenIdleMs) means the member is idle even if its byte
+  stream is noisy; a small screenIdleMs means it is actively redrawing
+  (spinner or progress).
 - send_input -- type text into a member's terminal (submit defaults to true).
 - open_tab / close_tab -- add or terminate worker sessions.
-- get_tab_status -- quick status of a member.
+- get_tab_status -- quick status of a member (including screenIdleMs, the
+  screen-change-based idle signal).
 - repo_info -- the repository's basic facts (top-level layout, README,
   package.json summary, git state). Shallow by design: it never returns
   source-file contents, takes no path arguments, and is capped in size.
@@ -130,6 +135,12 @@ orchestrator should catch this itself.
 - Every instruction sent via \`send_input\` MUST end with an explicit
   reminder to call \`handoff_to_orchestrator\` once done, blocked, or in
   need of input.
+- \`wait_for_handoff\` returning \`{timedOut:true}\` is NOT an error: it
+  simply means no handoff arrived within the timeout. Call it again. A
+  handoff is never lost to a timeout or a disconnect -- an event that
+  arrives while nobody is waiting stays queued, and even if your
+  connection dies mid-wait, the next \`wait_for_handoff\` (after the
+  reconnect) receives it.
 - After sending a step, don't just trust \`wait_for_handoff\` to eventually
   notify you -- it only returns once the worker actually calls the tool,
   and nothing forces that to happen. When you get any other opportunity to

server/ws/groupManager.js

@@ -619,48 +619,110 @@ export function pushHandoff(groupId, event) {
 // Resolves with the next handoff event, or { timedOut: true } when timeoutMs
 // elapses with the queue still empty (timeoutMs <= 0 means never).
 //
+// Reliability contract (the orchestrator's wait_for_handoff depends on it):
+// an event is only ever dequeued by a waiter that can be reasonably expected
+// to deliver it. A waiter whose client connection is dead or whose request
+// was cancelled must not remove an event from the queue -- the event stays
+// queued and the next wait_for_handoff receives it.
+//
 // Only one waiter per group is ever meaningful (the orchestrator calls
 // wait_for_handoff one at a time). A client-side cancelled MCP request leaves
 // its takeHandoff promise -- and its listener -- alive server-side for up to
 // timeoutMs (15 min by default), and such a "zombie" listener, being older,
 // would consume the next pushHandoff before the real waiter ever sees it.
 // So a new takeHandoff first settles every still-pending waiter for the same
-// group as { orphaned: true } (each finish tears its own listener/timer down),
-// then registers the fresh waiter as the sole consumer.
-export function takeHandoff(groupId, timeoutMs) {
+// group as { timedOut: true } (each finish tears its own listener/timer
+// down), then registers the fresh waiter as the sole consumer.
+//
+// opts.isAlive (a function, optional): checked right before a dequeue. When
+// it returns false the waiter leaves the queue alone -- the event belongs to
+// the next waiter whose connection is actually alive. The waiter itself is
+// left pending (it cannot consume anything) until superseded or timed out.
+//
+// Dequeue is not the same as delivery: the waiter claims an event, then
+// commits the delivery on the next macrotask. A supersede arriving in the
+// same turn can still reclaim the claimed event (its connection may have died
+// or its request been cancelled between the claim and the send), so the
+// event is re-queued instead of being lost with the stale waiter. The same
+// reclaim runs when the orchestrator exits (onOrchestratorExit) or a timeout
+// fires while an event is claimed.
+export function takeHandoff(groupId, timeoutMs, opts = {}) {
   const group = groups.get(groupId);
   if (!group) return Promise.resolve({ error: 'group-not-found' });
-  for (const stale of [...group.pendingTakes]) {
-    console.warn(`[groupManager] takeHandoff(${groupId}): superseding a still-pending waiter`);
-    stale({ orphaned: true });
+  if (group.pendingTakes.size > 0) {
+    console.warn(`[groupManager] takeHandoff(${groupId}): superseding ${group.pendingTakes.size} still-pending waiter(s)`);
   }
+  settlePendingTakes(group, { timedOut: true });
   return new Promise((resolve) => {
+    const waiter = { consumed: null, finish: null, onHandoff: null };
     let settled = false;
     const finish = (val) => {
       if (settled) return;
       settled = true;
       clearTimeout(timer);
-      group.pendingTakes.delete(finish);
-      group.handoffEmitter.off('handoff', onHandoff);
+      group.pendingTakes.delete(waiter);
+      group.handoffEmitter.off('handoff', waiter.onHandoff);
       resolve(val);
     };
-    const onHandoff = () => {
-      if (group.handoffQueue.length > 0) finish(group.handoffQueue.shift());
+    waiter.finish = finish;
+    waiter.onHandoff = () => {
+      if (group.handoffQueue.length === 0 || waiter.consumed) return;
+      if (opts.isAlive && !opts.isAlive()) return;
+      waiter.consumed = group.handoffQueue.shift();
+      // Commit the delivery on the next macrotask, not inline: a supersede
+      // (a newer takeHandoff in the same turn) must be able to reclaim the
+      // event from this waiter, so it is never delivered to a connection
+      // whose request may already be gone.
+      setTimeout(() => finish(waiter.consumed), 0);
     };
     const timer = timeoutMs > 0
-      ? setTimeout(() => finish({ timedOut: true }), timeoutMs)
+      ? setTimeout(() => {
+          reclaimConsumed(group, waiter);
+          finish({ timedOut: true });
+        }, timeoutMs)
       : null;
-    group.pendingTakes.add(finish);
-    group.handoffEmitter.on('handoff', onHandoff);
-    onHandoff();
+    group.pendingTakes.add(waiter);
+    group.handoffEmitter.on('handoff', waiter.onHandoff);
+    waiter.onHandoff();
   });
 }
 
+// Give back an event a (still-pending) waiter claimed but has not committed:
+// its delivery is suspect (dead connection, cancelled request), so the event
+// must reach the next waiter. Reference-guarded against re-queueing an event
+// that already sits in the queue.
+function reclaimConsumed(group, waiter) {
+  if (!waiter.consumed) return;
+  if (!group.handoffQueue.includes(waiter.consumed)) {
+    group.handoffQueue.unshift(waiter.consumed);
+  }
+  waiter.consumed = null;
+}
+
+// Settle every pending waiter for the group with `val`, reclaiming any event
+// a waiter already claimed. Used by supersede (a newer takeHandoff) and by
+// onOrchestratorExit (the control broker went away: no zombie waiter may
+// linger for the full timeout).
+function settlePendingTakes(group, val) {
+  for (const stale of [...group.pendingTakes]) {
+    reclaimConsumed(group, stale);
+    stale.finish(val);
+  }
+}
+
 // Stop only the control broker (orchestrator exited) -- the workers stay
-// alive so the human can keep working in them.
+// alive so the human can keep working in them. Pending wait_for_handoff
+// waiters (created by the now-destroyed control connections) are settled
+// with { timedOut: true } so no 15-minute zombie survives the broker
+// teardown; the events themselves stay in the queue (any claimed-but-
+// undelivered event is reclaimed by settlePendingTakes), so the next
+// orchestrator's wait_for_handoff still receives them.
 export function onOrchestratorExit(groupId) {
   const group = groups.get(groupId);
   if (!group) return;
+  if (group.pendingTakes.size > 0) {
+    settlePendingTakes(group, { timedOut: true });
+  }
   if (group.controlBroker) {
     stopBroker(group.controlBroker);
     group.controlBroker = null;
@@ -675,8 +737,8 @@ export function onOrchestratorExit(groupId) {
 export function destroyGroup(groupId) {
   const group = groups.get(groupId);
   if (!group) return;
-  for (const finish of [...group.pendingTakes]) {
-    finish({ error: 'group-destroyed' });
+  for (const waiter of [...group.pendingTakes]) {
+    waiter.finish({ error: 'group-destroyed' });
   }
   group.pendingTakes.clear();
   for (const sessionId of [...group.members.values()]) {
@@ -754,7 +816,17 @@ function cleanupMemberChannels(group, sessionId) {
 }
 
 // Public facade passed into broker servers (avoids exposing the module
-// namespace's internals / keeps tool deps explicit).
+// namespace's internals / keeps tool deps explicit). This IS the shape the
+// production MCP tools receive -- keep it in sync with what mcpTools.js
+// calls: a function used by a tool but missing here fails in production
+// (TypeError) while the full-module tests stay green. Every tool added to
+// mcpServer/mcpTools must have its backing groupManager function in this
+// facade, and tests must inject this facade (getGroupManagerApi), not the
+// full module.
+// Deliberately NOT in this facade: getGroup (the raw group object carries
+// controlBroker socket paths, handoff channels, the handoff queue and
+// allowedCwds -- internals LLM-facing tools must never reach; repo_info
+// needs only the project dir, which getGroupCwd provides).
 const groupManagerApi = {
   listGroupMembers,
   isSessionInGroup,
@@ -767,6 +839,13 @@ const groupManagerApi = {
   removeMember,
 };
 
+// Test seam: returns the exact facade the broker servers receive. Unit tests
+// must inject this -- never the full module -- so a facade/real mismatch
+// (a missing function) is caught by the tests, not only in production.
+export function getGroupManagerApi() {
+  return groupManagerApi;
+}
+
 // Session-manager facade. A `let` so tests can swap in fakes (see
 // setSessionApiForTests) to exercise addMember's spawn/teardown paths without
 // real ptys. All references go through this binding (function bodies only),

server/ws/groupManager.test.js

@@ -170,14 +170,15 @@ test('a newer takeHandoff supersedes a still-pending one (no zombie listener)',
   const waitA = groupManager.takeHandoff(gid, 0);
   // Call B: the real waiter arriving while A is still unresolved. Under the
   // pre-fix implementation A's listener would stay attached and consume the
-  // next pushHandoff, leaving B stuck until timeout; now A is orphaned first.
+  // next pushHandoff, leaving B stuck until timeout; now A is superseded
+  // first.
   const waitB = groupManager.takeHandoff(gid, 0);
 
   const event = { type: 'done', from: 'workerA' };
   assert.equal(groupManager.pushHandoff(gid, event), true);
 
   const [resA, resB] = await Promise.all([waitA, waitB]);
-  assert.deepEqual(resA, { orphaned: true }, 'superseded waiter settles as orphaned, not by stealing the event');
+  assert.deepEqual(resA, { timedOut: true }, 'superseded waiter settles as timedOut, not by stealing the event');
   assert.deepEqual(resB, event, 'the latest waiter receives the pushed event');
 });
 
@@ -190,14 +191,66 @@ test('a superseded waiter is removed from pendingTakes (no zombie listener left
   // The newer waiter supersedes A, which must not linger in pendingTakes --
   // otherwise its listener would consume the next pushHandoff before waitB.
   const waitB = groupManager.takeHandoff(gid, 0);
-  assert.equal(groupManager.getGroup(gid).pendingTakes.size, 1, 'orphaned A must not linger');
-  assert.deepEqual(await waitA, { orphaned: true });
+  assert.equal(groupManager.getGroup(gid).pendingTakes.size, 1, 'superseded A must not linger');
+  assert.deepEqual(await waitA, { timedOut: true });
 
   groupManager.pushHandoff(gid, { type: 'first' });
   assert.deepEqual(await waitB, { type: 'first' });
   assert.equal(groupManager.getGroup(gid).pendingTakes.size, 0, 'resolved waiter cleans up');
 });
 
+// The supersede reclaim: a waiter that already dequeued an event (its
+// delivery is committed only on the next macrotask) gives it back to the
+// queue when superseded -- the event must reach the fresh waiter instead of
+// being lost with the stale one.
+test('supersede reclaims an event a stale waiter already consumed', async () => {
+  const gid = await makeGroup();
+
+  const waitA = groupManager.takeHandoff(gid, 0);
+  const event = { type: 'done', from: 'workerA', summary: 'E1' };
+  groupManager.pushHandoff(gid, event); // A dequeues it (delivery not yet committed)
+
+  const waitB = groupManager.takeHandoff(gid, 0); // supersedes A, reclaiming the event
+  const [resA, resB] = await Promise.all([waitA, waitB]);
+  assert.deepEqual(resA, { timedOut: true }, 'the stale waiter settles as timedOut without the event');
+  assert.deepEqual(resB, event, 'the reclaimed event reaches the new waiter');
+});
+
+// The core no-loss guarantee: a waiter whose connection is dead must not
+// dequeue anything -- the event stays queued for the next (live) waiter.
+test('a dead (isAlive:false) waiter never consumes; the next live waiter receives the event', async () => {
+  const gid = await makeGroup();
+
+  const deadWait = groupManager.takeHandoff(gid, 0, { isAlive: () => false });
+  groupManager.pushHandoff(gid, { type: 'done', from: 'workerA', summary: 'survives death' });
+  // The dead waiter has not consumed: the queue still holds the event and a
+  // live waiter supersedes the dead one and receives it.
+  const liveWait = groupManager.takeHandoff(gid, 0, { isAlive: () => true });
+  const [resDead, resLive] = await Promise.all([deadWait, liveWait]);
+  assert.deepEqual(resDead, { timedOut: true });
+  assert.deepEqual(resLive, { type: 'done', from: 'workerA', summary: 'survives death' });
+});
+
+test('onOrchestratorExit settles pending waiters as timedOut (no 15-min zombie)', async () => {
+  const gid = await makeGroup();
+
+  const wait = groupManager.takeHandoff(gid, 0); // never times out on its own
+  assert.equal(groupManager.getGroup(gid).pendingTakes.size, 1);
+  groupManager.onOrchestratorExit(gid);
+  assert.equal(groupManager.getGroup(gid).pendingTakes.size, 0, 'waiters settled on orchestrator exit');
+  const res = await Promise.race([
+    wait,
+    new Promise((r) => setTimeout(() => r('still-pending'), 500)),
+  ]);
+  assert.deepEqual(res, { timedOut: true });
+
+  // The queue is untouched: a worker handoff after the exit is still
+  // received by the next waiter.
+  groupManager.pushHandoff(gid, { summary: 'after exit' });
+  const next = await groupManager.takeHandoff(gid, 200);
+  assert.deepEqual(next, { summary: 'after exit' });
+});
+
 test('destroyGroup settles pending takeHandoff waiters', async () => {
   const gid = randomUUID();
   await groupManager.createGroup({ groupId: gid, cwd: '/srv/proj', orchestratorDir: '/srv/orch' });