src/commands/run.tsx (2)

1995-2003: gracefulShutdown is not re-entrant — add a guard flag.

process.removeListener at Line 2164 runs only after the quit promise resolves. A second SIGTERM arriving while gracefulShutdown is blocked inside await parallelExecutor.stop() will invoke gracefulShutdown concurrently, potentially calling interruptHandler.dispose() and renderer.destroy() twice.

♻️ Proposed fix

+  let isShuttingDown = false;
   const gracefulShutdown = async (): Promise<void> => {
+    if (isShuttingDown) return;
+    isShuttingDown = true;
     if (executionPromise) {
       // ...

Also applies to: 2027-2027, 2159-2164

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/run.tsx` around lines 1995 - 2003, The gracefulShutdown function
is not re-entrant and can be invoked concurrently (e.g., by multiple SIGTERM
signals), risking double disposal of resources like interruptHandler.dispose()
and renderer.destroy(); add a boolean guard (e.g., isShuttingDown) at the top of
gracefulShutdown to return early if already running, set it true immediately on
entry and false only if you truly want re-entry, and ensure all cleanup steps
(parallelExecutor.stop(), await executionPromise,
savePersistedSession(currentState), cleanup(), resolveQuitPromise?.()) run only
once; also guard any other shutdown entry points that call gracefulShutdown
(signals/handlers) to check this flag before invoking and avoid double-calling
interruptHandler.dispose() and renderer.destroy().

---

1996-2002: Capture executionPromise before await stop() to make intent explicit.

executionPromise can be set to null by the .finally() handler in startParallelExecution while await parallelExecutor.stop() is suspended, turning the subsequent await executionPromise into await null. Although functionally safe (execution has already unwound at that point), capturing the reference first makes the intent clear and prevents accidental no-ops if the pattern is extended.

♻️ Suggested refactor (both sites)

-  if (executionPromise) {
-    await parallelExecutor.stop();
-    await executionPromise;
-  }
+  const inFlight = executionPromise;
+  if (inFlight) {
+    await parallelExecutor.stop();
+    await inFlight;
+  }

Also applies to: 2098-2103

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/run.tsx` around lines 1996 - 2002, The code awaits
parallelExecutor.stop() and then awaits executionPromise, but executionPromise
can be nulled by the .finally() in startParallelExecution while stop() is
suspended; capture the current executionPromise reference into a local variable
(e.g., const exec = executionPromise) before calling await
parallelExecutor.stop(), then await that local (await exec) so the intent is
explicit and you avoid awaiting null; apply the same change for the other site
mentioned (the block near 2098-2103) and keep the subsequent
savePersistedSession(currentState) and cleanup() calls unchanged.

tests/commands/run.test.ts (1)

478-493: Missing test case: taskCompleted=false with existing entry.

The else branch of updateCompletedLocallyTaskIds deletes the task ID from the set, but only the (true, 0) variant is tested. Adding a (false, N) case (e.g., a retry that didn't complete) would complete the branch coverage.

➕ Suggested additional test

+    test('removes stale task ID when task did not complete', () => {
+      const next = updateCompletedLocallyTaskIds(
+        new Set<string>(['task-1', 'task-2']),
+        'task-1',
+        false,
+        3
+      );
+      expect(next).toEqual(new Set(['task-2']));
+    });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/commands/run.test.ts` around lines 478 - 493, The test suite is missing
a case exercising updateCompletedLocallyTaskIds when taskCompleted is false and
the task ID already exists; add a test that calls
updateCompletedLocallyTaskIds(new Set(['task-1','task-2']), 'task-1', false,
anyNumber) and assert the resulting set no longer contains 'task-1' (e.g.,
equals new Set(['task-2'])), so the branch that deletes on taskCompleted=false
is covered.

tests/commands/run.test.ts (1)

463-576: Good coverage for all new helpers — one boundary case missing.

All three logical branches of shouldMarkCompletedLocally, both directions of updateCompletedLocallyTaskIds, both executor-status paths of applyParallelCompletionState, and the key combinations of isParallelExecutionComplete are covered.

applyConflictResolvedTaskTracking tests use resolutionCount = 0 and 2, but the guard is < 1, meaning resolutionCount === 1 is the exact boundary. Consider adding a test at the boundary to make the contract explicit:

✅ Suggested boundary test

+    test('moves task from completed-locally to merged when a single conflict is resolved', () => {
+      const outcome = applyConflictResolvedTaskTracking(
+        new Set<string>(['task-1']),
+        new Set<string>(),
+        'task-1',
+        1
+      );
+
+      expect(outcome.completedLocallyTaskIds).toEqual(new Set());
+      expect(outcome.mergedTaskIds).toEqual(new Set(['task-1']));
+    });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/commands/run.test.ts` around lines 463 - 576, Add a boundary test for
applyConflictResolvedTaskTracking where resolutionCount === 1: call
applyConflictResolvedTaskTracking(new Set(['task-1','task-2']), new
Set(['task-3']), 'task-1', 1) and assert that completedLocallyTaskIds equals new
Set(['task-2']) and mergedTaskIds equals new Set(['task-3','task-1']), ensuring
the "< 1" guard treats 1 as a resolved count; place the test alongside the
existing applyConflictResolvedTaskTracking cases and use the same naming
pattern.

tests/engine/execution-engine.test.ts (1)

677-677: (engine as any).rateLimitedAgents directly tests the regression fix

Accessing the private rateLimitedAgents field via as any is acceptable here since the test is specifically verifying the new clearRateLimitedAgents() call introduced by this PR. Consider adding a package-internal getter (e.g. getRateLimitedAgentsCount(): number) if this field needs broader test coverage in future.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/engine/execution-engine.test.ts` at line 677, The test is reaching into
a private field rateLimitedAgents via (engine as any).rateLimitedAgents.size;
instead add a package-internal getter on the engine class (e.g.
getRateLimitedAgentsCount(): number) that returns rateLimitedAgents.size,
implement it alongside clearRateLimitedAgents(), and update the test to assert
expect(engine.getRateLimitedAgentsCount()).toBe(0) so the test no longer uses
type-unsafe casting.

tests/remote/remote.test.ts (1)

436-449: setTimeout(resolve, 0) is fragile for async-chain settling

The current implementation's .then(async () => { await resetTasksToOpen(); ... }) chain settles within the scheduled macrotask window, so the test passes. However, if an additional await is introduced in the chain (e.g. awaiting the send() call or an audit log), the test would silently miss assertions. A more robust approach is to flush all pending microtasks explicitly:

♻️ Proposed fix

-await new Promise((resolve) => setTimeout(resolve, 0));
+// Flush all pending microtask queues
+await Promise.resolve();
+await Promise.resolve();
+await Promise.resolve();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/remote/remote.test.ts` around lines 436 - 449, The test uses await new
Promise(resolve => setTimeout(resolve, 0)) which relies on a macrotask and is
fragile; replace that with an explicit microtask flush so pending promises in
the async chain reliably settle before assertions. Change the wait to an
explicit microtask flush (for example: await Promise.resolve(); await
Promise.resolve(); or use a shared flushPromises helper that awaits
Promise.resolve() twice or process.nextTick) right before the assertions that
check on, stop, resetTasksToOpen and ws.send (referencing the existing symbols
on, stop, resetTasksToOpen, ws.send and OperationResultMessage) so the async
chain is fully settled and the JSON-parsed response assertions are
deterministic.

tests/commands/run.test.ts (1)

506-535: Consider a narrower type for the partial state stub instead of as any.

The as any casts on lines 512 and 527 bypass type-checking for the stub objects. A typed partial avoids masking future type breakages in PersistedSessionState.

♻️ Suggested refinement

+import type { PersistedSessionState } from '../../src/session/index.js';

-      const state = {
+      const state: Partial<PersistedSessionState> & Pick<PersistedSessionState, 'status' | 'isPaused' | 'activeTaskIds' | 'updatedAt'> = {
         status: 'running',
         isPaused: true,
         activeTaskIds: ['task-1'],
         updatedAt: '2026-01-01T00:00:00.000Z',
-      } as any;
+      };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/commands/run.test.ts` around lines 506 - 535, The tests use loose `as
any` casts for the stubbed session state which bypass TypeScript checks; replace
those casts by typing the fixtures as Partial<PersistedSessionState> (or
Pick<...> of the required fields) so the test stubs match the real shape—locate
the two test cases in tests/commands/run.test.ts that call
applyParallelCompletionState and change the state declarations to use
Partial<PersistedSessionState> (preserving fields: status, isPaused,
activeTaskIds, updatedAt) to keep type safety while still allowing omitted
properties.

src/commands/run.tsx (2)

3222-3271: Headless signal handler doesn't guard against stop() rejection.

Unlike the TUI path (which wraps stop() in try/catch), the headless handleParallelSignal calls await parallelExecutor.stop() directly at Line 3231. Since this is an async signal handler, an uncaught rejection would surface as an unhandled promise rejection at the process level. The risk is low (the current stop() implementation uses Promise.allSettled), but it's inconsistent with the TUI path's defensive pattern.

🛡️ Wrap stop() for consistency with the TUI path

          console.log('\n[INFO] [parallel] Received signal, stopping parallel execution...');

          // Stop the parallel executor
-          await parallelExecutor.stop();
+          try {
+            await parallelExecutor.stop();
+          } catch {
+            // Best-effort stop; proceed with cleanup regardless
+          }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/run.tsx` around lines 3222 - 3271, The headless signal handler
handleParallelSignal calls await parallelExecutor.stop() without catching
rejections; wrap that call in a try/catch (same defensive pattern used in the
TUI path) so any thrown error from parallelExecutor.stop() is caught and handled
(e.g., log at info/error or ignore) to prevent unhandled promise rejections,
then continue the existing cleanup (awaiting parallelExecutionPromise, resetting
tasks, saving state); update references to parallelExecutor.stop() and
handleParallelSignal accordingly.

---

129-140: _commitCount is accepted but unused — intentional design seam?

The parameter is currently ignored (the function returns taskCompleted verbatim). The PR objectives specifically call out "tasks could be marked completed locally when no commit was produced" as a root cause. The actual fix lives in the executor (zero-commit tasks are treated as merge failures there), so this function serves as a UI-level extension point. The intent is clear, but the unused parameter could confuse future contributors.

Consider adding a brief inline comment explaining why the parameter exists but is unused, e.g. // commitCount reserved for future gating; merge outcome is tracked separately.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/run.tsx` around lines 129 - 140, The parameter _commitCount in
shouldMarkCompletedLocally is intentionally unused but confusing; add a brief
inline comment inside the function (near the parameter usage) explaining that
commit count is reserved for future UI gating and merge success/failure is
tracked separately by merge events, e.g., note that zero-commit handling happens
in the executor and this function is an extension point, so keep returning
taskCompleted while documenting the reason for the unused _commitCount.