Browse Source

fix(herdr): treat exit code 1 as successful close and parse NDJSON line-by-line

- closePane: accept exit code 1 (pane already closed / not found) as
  success, matching the codemap's documented behavior. Only non-zero
  codes other than 1 now surface as real failures.
- parsePaneId: scan newline-delimited JSON line-by-line and return the
  first pane_id found, instead of JSON.parse on the whole stdout. Extra
  progress/diagnostic lines no longer cause spawnPane to silently report
  failure when the pane was actually created.
- Add 4 tests covering exit code 1 (already closed), exit code 2 (real
  failure), extra NDJSON lines, and only non-pane JSON lines.

Addresses P1 review comments on PR #617.
Shank 1 month ago
parent
commit
7c4d8fea15
2 changed files with 109 additions and 7 deletions
  1. 95 0
      src/multiplexer/herdr/index.test.ts
  2. 14 7
      src/multiplexer/herdr/index.ts

+ 95 - 0
src/multiplexer/herdr/index.test.ts

@@ -176,6 +176,101 @@ describe('HerdrMultiplexer', () => {
     expect(success).toBe(true);
   });
 
+  test('returns true when pane close exits with code 1 (already closed)', async () => {
+    const { HerdrMultiplexer } = await importFreshHerdr();
+    const herdr = new HerdrMultiplexer('main-vertical', 60);
+
+    crossSpawnMock.mockImplementation((command: string[]) => {
+      if (command[0] === 'which') {
+        return createSpawnResult(0, '/usr/bin/herdr\n');
+      }
+      if (command.includes('close')) {
+        return createSpawnResult(1, '', 'pane not found');
+      }
+      return createSpawnResult();
+    });
+
+    const success = await herdr.closePane('w1:p2');
+    expect(success).toBe(true);
+  });
+
+  test('returns false when pane close exits with code 2 (real failure)', async () => {
+    const { HerdrMultiplexer } = await importFreshHerdr();
+    const herdr = new HerdrMultiplexer('main-vertical', 60);
+
+    crossSpawnMock.mockImplementation((command: string[]) => {
+      if (command[0] === 'which') {
+        return createSpawnResult(0, '/usr/bin/herdr\n');
+      }
+      if (command.includes('close')) {
+        return createSpawnResult(2, '', 'fatal error');
+      }
+      return createSpawnResult();
+    });
+
+    const success = await herdr.closePane('w1:p2');
+    expect(success).toBe(false);
+  });
+
+  test('parses pane_id when split output has extra NDJSON lines', async () => {
+    const { HerdrMultiplexer } = await importFreshHerdr();
+    const herdr = new HerdrMultiplexer('main-vertical', 60);
+
+    const extraLine = JSON.stringify({
+      id: 'cli:event',
+      result: { type: 'progress', message: 'splitting...' },
+    });
+    const paneLine = createSplitResponse('w1:p9');
+
+    crossSpawnMock.mockImplementation((command: string[]) => {
+      if (command[0] === 'which') {
+        return createSpawnResult(0, '/usr/bin/herdr\n');
+      }
+      if (command.includes('split')) {
+        return createSpawnResult(0, `${extraLine}\n${paneLine}\n`);
+      }
+      return createSpawnResult();
+    });
+
+    const result = await herdr.spawnPane(
+      'session-1',
+      'Herdr worker',
+      'http://localhost:4096',
+      '/repo',
+    );
+
+    expect(result).toEqual({ success: true, paneId: 'w1:p9' });
+  });
+
+  test('reports failure when split output has only non-pane JSON lines', async () => {
+    const { HerdrMultiplexer } = await importFreshHerdr();
+    const herdr = new HerdrMultiplexer('main-vertical', 60);
+
+    const progressOnly = JSON.stringify({
+      id: 'cli:event',
+      result: { type: 'progress', message: 'working...' },
+    });
+
+    crossSpawnMock.mockImplementation((command: string[]) => {
+      if (command[0] === 'which') {
+        return createSpawnResult(0, '/usr/bin/herdr\n');
+      }
+      if (command.includes('split')) {
+        return createSpawnResult(0, `${progressOnly}\n`);
+      }
+      return createSpawnResult();
+    });
+
+    const result = await herdr.spawnPane(
+      'session-1',
+      'Herdr worker',
+      'http://localhost:4096',
+      '/repo',
+    );
+
+    expect(result).toEqual({ success: false });
+  });
+
   test('reports failure when split returns non-zero exit code', async () => {
     const { HerdrMultiplexer } = await importFreshHerdr();
     const herdr = new HerdrMultiplexer('main-vertical', 60);

+ 14 - 7
src/multiplexer/herdr/index.ts

@@ -183,7 +183,7 @@ export class HerdrMultiplexer implements Multiplexer {
 
       log('[herdr] closePane: result', { exitCode, stderr: stderr.trim() });
 
-      if (exitCode === 0) {
+      if (exitCode === 0 || exitCode === 1) {
         return true;
       }
 
@@ -257,13 +257,20 @@ function parsePaneId(stdout: string): string | null {
   const trimmed = stdout.trim();
   if (!trimmed) return null;
 
-  try {
-    const response = JSON.parse(trimmed) as HerdrCliResponse;
-    return response.result?.pane?.pane_id ?? null;
-  } catch {
-    log('[herdr] parsePaneId: failed to parse JSON', { stdout: trimmed });
-    return null;
+  for (const line of trimmed.split('\n')) {
+    const candidate = line.trim();
+    if (!candidate) continue;
+    try {
+      const response = JSON.parse(candidate) as HerdrCliResponse;
+      const paneId = response.result?.pane?.pane_id;
+      if (paneId) return paneId;
+    } catch {
+      // Not a JSON line (e.g. progress/diagnostic); skip and keep scanning.
+    }
   }
+
+  log('[herdr] parsePaneId: no pane_id found in output', { stdout: trimmed });
+  return null;
 }
 
 function getPaneDirection(layout: MultiplexerLayout): HerdrPaneDirection {