refactor(agent-loop): dedup director tests, coordinator guard, error coercion

Summary:
- Drop per-director package-test assertions covered by the central
  registry loop; keep content-specific assertions per director.
- Extract the shared director test harness into one helper module.
- Merge the coordinator try/catch/rethrow/log/fallback skeletons into
  one generic helper preserving labels, fallbacks, rethrow semantics.
- Merge twin close blocks in disposeSubAgentSession into a loop.
- Add one shared errorMessage helper reused lane-wide.
- Collapse repeated skill-list literals in identity.test.ts into one
  shared const.

Verification:
- Baseline (pristine): bun test src/agent src/subagent src/workflows
  src/director.test.ts --randomize --seed 424242 -> 1471 pass, 0 fail.
- After: same command -> 1395 pass, 0 fail (delta is removed dupes).
- bun run typecheck -> exit 0. bun run lint -> clean.
  bun run check:dead-exports -> 0 violations.

Author: TheGreatAxios

src/agent/codex-read-raw-file.ts

@@ -32,6 +32,7 @@ import {
 } from "../plugins/secret-guard-plugin.js";
 import type { PermissionGate } from "../permission/gate.js";
 import type { CodexReadRawFile } from "./codex-tool-proxies.js";
+import { errorMessage } from "./error-message.js";
 
 /** `path` is workspace-relative (apply_patch's parser rejects absolute paths). */
 export function createCodexReadRawFile(
@@ -96,7 +97,7 @@ export function createCodexReadRawFile(
           return { content: `path is a directory: ${path}`, isError: true };
       }
       return {
-        content: err instanceof Error ? err.message : String(err),
+        content: errorMessage(err),
         isError: true,
       };
     }

src/agent/director.ts

@@ -23,6 +23,7 @@ import {
 } from "./compaction.js";
 import { onTurnBoundary } from "./reactor-events.js";
 import { isOperatorOriginated } from "./message-provenance.js";
+import { errorMessage } from "./error-message.js";
 import { type } from "arktype";
 import {
   applyManageTasks,
@@ -656,79 +657,89 @@ class ChatDirectorImpl extends DefaultDirector {
   // is the exception: its sole call site runs mid-turn (tool.done, never the
   // turn boundary), so it always degrades to plain inference on a
   // coordinator throw and takes no rethrow parameter.
-  private coordinatorIsActive(rethrowCoordinatorError: boolean): boolean {
+  //
+  // The four rethrow-capable consults below share one guard: on a coordinator
+  // throw, either rethrow (noting it so the turn drops queued notifications)
+  // or log under the consult's label and resolve the consult's fallback.
+  private withCoordinatorGuard<T>(
+    label: string,
+    fallback: T,
+    rethrowCoordinatorError: boolean,
+    consult: () => T,
+  ): T {
     try {
-      return this.workflowCoordinator?.isActive() === true;
+      return consult();
     } catch (err) {
       if (rethrowCoordinatorError) {
         this.coordinatorRethrowNoted = true;
         throw err;
       }
-      logger.warn`workflow-coordinator-isActive-threw error=${err instanceof Error ? err.message : String(err)}`;
-      return false;
+      logger.warn`${label} error=${errorMessage(err)}`;
+      return fallback;
     }
   }
 
+  private coordinatorIsActive(rethrowCoordinatorError: boolean): boolean {
+    return this.withCoordinatorGuard(
+      "workflow-coordinator-isActive-threw",
+      false,
+      rethrowCoordinatorError,
+      () => this.workflowCoordinator?.isActive() === true,
+    );
+  }
+
   private coordinatorDirective(
     rethrowCoordinatorError: boolean,
   ): string | null {
-    try {
-      const directive = this.workflowCoordinator?.directive() ?? null;
-      if (directive === null) return null;
-      if (typeof directive !== "string") {
-        logger.warn`workflow-coordinator-directive-not-string`;
-        return null;
-      }
-      if (directive.length === 0) return null;
-      if (directive.length > MAX_WORKFLOW_DIRECTIVE_CHARS) {
-        logger.warn`workflow-coordinator-directive-truncated chars=${String(directive.length)} max=${String(MAX_WORKFLOW_DIRECTIVE_CHARS)}`;
-        return `${directive.slice(0, MAX_WORKFLOW_DIRECTIVE_CHARS)}\n…[truncated]`;
-      }
-      return directive;
-    } catch (err) {
-      if (rethrowCoordinatorError) {
-        this.coordinatorRethrowNoted = true;
-        throw err;
-      }
-      logger.warn`workflow-coordinator-directive-threw error=${err instanceof Error ? err.message : String(err)}`;
-      return null;
-    }
+    return this.withCoordinatorGuard<string | null>(
+      "workflow-coordinator-directive-threw",
+      null,
+      rethrowCoordinatorError,
+      () => {
+        const directive = this.workflowCoordinator?.directive() ?? null;
+        if (directive === null) return null;
+        if (typeof directive !== "string") {
+          logger.warn`workflow-coordinator-directive-not-string`;
+          return null;
+        }
+        if (directive.length === 0) return null;
+        if (directive.length > MAX_WORKFLOW_DIRECTIVE_CHARS) {
+          logger.warn`workflow-coordinator-directive-truncated chars=${String(directive.length)} max=${String(MAX_WORKFLOW_DIRECTIVE_CHARS)}`;
+          return `${directive.slice(0, MAX_WORKFLOW_DIRECTIVE_CHARS)}\n…[truncated]`;
+        }
+        return directive;
+      },
+    );
   }
 
   private coordinatorCurrentStepIsGate(
     rethrowCoordinatorError: boolean,
   ): boolean {
-    try {
-      return this.workflowCoordinator?.currentStepIsGate() === true;
-    } catch (err) {
-      if (rethrowCoordinatorError) {
-        this.coordinatorRethrowNoted = true;
-        throw err;
-      }
-      logger.warn`workflow-coordinator-gate-threw error=${err instanceof Error ? err.message : String(err)}`;
-      return false;
-    }
+    return this.withCoordinatorGuard(
+      "workflow-coordinator-gate-threw",
+      false,
+      rethrowCoordinatorError,
+      () => this.workflowCoordinator?.currentStepIsGate() === true,
+    );
   }
 
   private coordinatorCurrentStepId(
     rethrowCoordinatorError: boolean,
   ): string | null {
-    try {
-      const stepId = this.workflowCoordinator?.currentStepId() ?? null;
-      if (stepId === null) return null;
-      if (typeof stepId !== "string" || stepId.length === 0) {
-        logger.warn`workflow-coordinator-step-id-not-string`;
-        return null;
-      }
-      return stepId;
-    } catch (err) {
-      if (rethrowCoordinatorError) {
-        this.coordinatorRethrowNoted = true;
-        throw err;
-      }
-      logger.warn`workflow-coordinator-step-id-threw error=${err instanceof Error ? err.message : String(err)}`;
-      return null;
-    }
+    return this.withCoordinatorGuard<string | null>(
+      "workflow-coordinator-step-id-threw",
+      null,
+      rethrowCoordinatorError,
+      () => {
+        const stepId = this.workflowCoordinator?.currentStepId() ?? null;
+        if (stepId === null) return null;
+        if (typeof stepId !== "string" || stepId.length === 0) {
+          logger.warn`workflow-coordinator-step-id-not-string`;
+          return null;
+        }
+        return stepId;
+      },
+    );
   }
 
   private coordinatorHandleToolDone(
@@ -743,7 +754,7 @@ class ChatDirectorImpl extends DefaultDirector {
     } catch (err) {
       // Mid-turn only (tool.done): a throwing coordinator degrades to plain
       // inference rather than failing the turn.
-      logger.warn`workflow-coordinator-handleToolDone-threw error=${err instanceof Error ? err.message : String(err)}`;
+      logger.warn`workflow-coordinator-handleToolDone-threw error=${errorMessage(err)}`;
       return false;
     }
   }

src/agent/directors/bruckheimer/package.test.ts

@@ -2,21 +2,6 @@ import { describe, expect, test } from "bun:test";
 import { bruckheimerPackage } from "./package.js";
 
 describe("bruckheimerPackage", () => {
-  test("id matches directory (keep bruckheimer path; identity is Bruckheimer)", () => {
-    expect(bruckheimerPackage.id).toBe("bruckheimer");
-  });
-
-  test("systemPrompt is real (not Placeholder)", () => {
-    expect(bruckheimerPackage.systemPrompt.length).toBeGreaterThan(0);
-    expect(bruckheimerPackage.systemPrompt.startsWith("Placeholder")).toBe(
-      false,
-    );
-  });
-
-  test("systemPrompt states PRIMARY INTENT", () => {
-    expect(bruckheimerPackage.systemPrompt).toMatch(/PRIMARY INTENT/i);
-  });
-
   test("systemPrompt identity is Bruckheimer / BruckheimerDirector", () => {
     const p = bruckheimerPackage.systemPrompt;
     expect(p).toMatch(/BruckheimerDirector \(Bruckheimer\)/);
@@ -101,26 +86,15 @@ describe("bruckheimerPackage", () => {
     expect(p).not.toMatch(/via run_shell/i);
   });
 
-  test("spawn.maySpawn is false", () => {
-    expect(bruckheimerPackage.spawn.maySpawn).toBe(false);
-  });
-
-  test("tools.allow is DOCS_TOOLS surface (writes, no shell)", () => {
+  test("tools.allow has no shell", () => {
     const allow = bruckheimerPackage.tools?.allow ?? [];
-    expect(allow).toContain("write_file");
-    expect(allow).toContain("edit_file");
-    expect(allow).toContain("delete_file");
     expect(allow).not.toContain("run_shell");
   });
 
   test("modelRole is docs", () => {
     expect(bruckheimerPackage.modelRole).toBe("docs");
   });
 
-  test("tier is leaf", () => {
-    expect(bruckheimerPackage.tier).toBe("leaf");
-  });
-
   test("primaryIntent and outOfLane match discovery lane", () => {
     expect(bruckheimerPackage.primaryIntent).toMatch(/product discovery/i);
     expect(bruckheimerPackage.outOfLane).toContain("shipping product code");

src/agent/directors/builder/package.test.ts

@@ -2,19 +2,6 @@ import { describe, expect, test } from "bun:test";
 import { builderPackage } from "./package.js";
 
 describe("builderPackage", () => {
-  test("id matches directory / registry id", () => {
-    expect(builderPackage.id).toBe("builder");
-  });
-
-  test("systemPrompt is non-empty and not a Placeholder", () => {
-    expect(builderPackage.systemPrompt.length).toBeGreaterThan(0);
-    expect(builderPackage.systemPrompt.startsWith("Placeholder")).toBe(false);
-  });
-
-  test("systemPrompt mentions PRIMARY INTENT", () => {
-    expect(builderPackage.systemPrompt).toContain("PRIMARY INTENT");
-  });
-
   test("systemPrompt identity is Builder / BuilderDirector (not job-title language)", () => {
     const p = builderPackage.systemPrompt;
     expect(p).toMatch(/BuilderDirector \(Builder\)/);
@@ -108,18 +95,6 @@ describe("builderPackage", () => {
     expect(p).not.toMatch(/scheduler/i);
   });
 
-  test("spawn.maySpawn is false (leaf)", () => {
-    expect(builderPackage.spawn.maySpawn).toBe(false);
-  });
-
-  test("tools.allow includes product write tools", () => {
-    const allow = builderPackage.tools?.allow ?? [];
-    expect(allow).toContain("write_file");
-    expect(allow).toContain("edit_file");
-    expect(allow).toContain("delete_file");
-    expect(allow).toContain("apply_patch");
-  });
-
   test("modelRole is implement", () => {
     expect(builderPackage.modelRole).toBe("implement");
   });

src/agent/directors/counsel/package.test.ts

@@ -2,19 +2,6 @@ import { describe, expect, test } from "bun:test";
 import { counselPackage } from "./package.js";
 
 describe("counselPackage", () => {
-  test("id matches directory", () => {
-    expect(counselPackage.id).toBe("counsel");
-  });
-
-  test("systemPrompt is real (not Placeholder)", () => {
-    expect(counselPackage.systemPrompt.length).toBeGreaterThan(0);
-    expect(counselPackage.systemPrompt.startsWith("Placeholder")).toBe(false);
-  });
-
-  test("systemPrompt states PRIMARY INTENT", () => {
-    expect(counselPackage.systemPrompt).toMatch(/PRIMARY INTENT/i);
-  });
-
   test("systemPrompt identity is Counsel / CounselDirector (not PlanDirector)", () => {
     const p = counselPackage.systemPrompt;
     expect(p).toMatch(/CounselDirector \(Counsel\)/);
@@ -63,16 +50,9 @@ describe("counselPackage", () => {
     expect(p).toContain("Headings-only Findings is not done");
   });
 
-  test("spawn.maySpawn is false", () => {
-    expect(counselPackage.spawn.maySpawn).toBe(false);
-  });
-
-  test("tools.allow is review surface with product writes", () => {
+  test("tools.allow mounts read (plan lane)", () => {
     const allow = counselPackage.tools?.allow ?? [];
     expect(allow).toContain("read_file");
-    expect(allow).toContain("write_file");
-    expect(allow).toContain("edit_file");
-    expect(allow).toContain("delete_file");
   });
 
   test("modelRole is plan", () => {

src/agent/directors/critic/package.test.ts

@@ -2,19 +2,6 @@ import { describe, expect, test } from "bun:test";
 import { criticPackage } from "./package.js";
 
 describe("criticPackage", () => {
-  test("id matches directory", () => {
-    expect(criticPackage.id).toBe("critic");
-  });
-
-  test("systemPrompt is real, not a placeholder", () => {
-    expect(criticPackage.systemPrompt.length).toBeGreaterThan(0);
-    expect(criticPackage.systemPrompt.startsWith("Placeholder")).toBe(false);
-  });
-
-  test("systemPrompt states PRIMARY INTENT", () => {
-    expect(criticPackage.systemPrompt).toMatch(/PRIMARY INTENT/i);
-  });
-
   test("systemPrompt identity is Critic / CriticDirector", () => {
     const p = criticPackage.systemPrompt;
     expect(p).toMatch(/CriticDirector \(Critic\)/);
@@ -103,18 +90,11 @@ describe("criticPackage", () => {
     expect(p).not.toMatch(/via run_shell/i);
   });
 
-  test("spawn.maySpawn is false", () => {
-    expect(criticPackage.spawn.maySpawn).toBe(false);
-  });
-
-  test("tools.allow is review surface with product writes", () => {
+  test("tools.allow mounts read plus skill discovery", () => {
     const allow = criticPackage.tools?.allow ?? [];
     expect(allow).toContain("read_file");
     expect(allow).toContain("skill_search");
     expect(allow).toContain("use_skill");
-    expect(allow).toContain("write_file");
-    expect(allow).toContain("edit_file");
-    expect(allow).toContain("delete_file");
   });
 
   test("modelRole is review", () => {

src/agent/directors/draper/package.test.ts

@@ -2,19 +2,6 @@ import { describe, expect, test } from "bun:test";
 import { draperPackage } from "./package.js";
 
 describe("draperPackage", () => {
-  test("id matches directory", () => {
-    expect(draperPackage.id).toBe("draper");
-  });
-
-  test("systemPrompt is real, not a placeholder", () => {
-    expect(draperPackage.systemPrompt.length).toBeGreaterThan(0);
-    expect(draperPackage.systemPrompt.startsWith("Placeholder")).toBe(false);
-  });
-
-  test("systemPrompt states PRIMARY INTENT", () => {
-    expect(draperPackage.systemPrompt).toMatch(/PRIMARY INTENT/i);
-  });
-
   test("systemPrompt identity is Draper / DraperDirector (package id stays draper)", () => {
     const p = draperPackage.systemPrompt;
     expect(p).toMatch(/DraperDirector \(Draper\)/);
@@ -126,23 +113,15 @@ describe("draperPackage", () => {
     expect(p).not.toMatch(/## Paths/);
   });
 
-  test("spawn.maySpawn is false", () => {
-    expect(draperPackage.spawn.maySpawn).toBe(false);
-  });
-
-  test("tools.allow is review surface with file writes for evidence tests", () => {
+  test("tools.allow mounts read plus skill discovery", () => {
     const allow = draperPackage.tools?.allow ?? [];
     expect(allow).toContain("read_file");
     expect(allow).toContain("skill_search");
     expect(allow).toContain("use_skill");
-    expect(allow).toContain("write_file");
-    expect(allow).toContain("edit_file");
-    expect(allow).toContain("delete_file");
   });
 
-  test("modelRole is review and tier is leaf", () => {
+  test("modelRole is review", () => {
     expect(draperPackage.modelRole).toBe("review");
-    expect(draperPackage.tier).toBe("leaf");
   });
 
   test("primaryIntent and outOfLane match the restored full-critique lane", () => {