fix(review): address coderabbit findings on dispatch collapse PR

- Strict TriageResponseSchema rejects legacy {mode,complexity} fields
- Scaler rollbackSpawn() clears reservation only if it still matches
- Router uses rollbackSpawn on K8s failure to avoid trampling concurrent reservations
- Migration 004 UPDATEs now guard with IS DISTINCT FROM to be idempotent
- job-queue getQueueLength() moves valkey client acquisition inside try/catch
- Docs: fix fallback-reason count, parameterize RBAC namespaces, scope daemon-secrets,
  correct architecture scale-up diagram edges
- Tests: align router mock contract with EphemeralSpawnErrorKind taxonomy,
  add daemon-mode DAEMON_AUTH_TOKEN coverage, cover legacy triage shape rejection

Author: chrisleekr

docs/ARCHITECTURE.md

@@ -12,7 +12,7 @@ flowchart TD
     ROUTE["Router<br/>idempotency + allowlist + concurrency"]:::guard
     TR["Haiku triage<br/>binary heavy classifier"]:::decide
     QUEUE["Orchestrator job queue<br/>Valkey list"]:::store
-    SCALE{{"Scale-up decision<br/>heavy OR queue ≥ threshold?<br/>and cooldown elapsed?"}}:::fork
+    SCALE{{"Scale-up decision<br/>heavy OR queue ≥ threshold<br/>AND no persistent slots?<br/>and cooldown elapsed?"}}:::fork
     SPAWN["K8s API<br/>create bare Pod<br/>DAEMON_EPHEMERAL=true"]:::decide
     FLEET["Daemon fleet<br/>persistent + ephemeral<br/>WebSocket connections"]:::target
     PIPE["runPipeline<br/>clone → prompt → Claude Agent SDK"]:::work
@@ -22,7 +22,7 @@ flowchart TD
     ACK -. async .-> ROUTE
     ROUTE --> TR
     TR --> QUEUE
-    ROUTE --> SCALE
+    QUEUE --> SCALE
     SCALE -->|yes| SPAWN
     SPAWN --> FLEET
     SCALE -->|no, or cooldown active| FLEET

docs/CONFIGURATION.md

@@ -104,7 +104,7 @@ Required whenever the orchestrator role is active (i.e. the webhook server proce
 | `TRIAGE_TIMEOUT_MS`           | `5000`      | Per-call wall clock. Beyond this, the circuit-breaker counter increments.                          |
 | `DEFAULT_MAXTURNS`            | `30`        | Agent turn cap. Applied on every execution — triage no longer influences `maxTurns`.               |
 
-See [Triage](TRIAGE.md) for the binary `heavy` signal, circuit breaker, and the five fallback reasons that appear in logs.
+See [Triage](TRIAGE.md) for the binary `heavy` signal, circuit breaker, and the six fallback reasons that appear in logs.
 
 ## Mode matrix — what's required when
 

docs/DEPLOYMENT.md

@@ -314,7 +314,8 @@ apiVersion: rbac.authorization.k8s.io/v1
 kind: Role
 metadata:
   name: github-app-playground-ephemeral-spawner
-  namespace: default
+  # Role lives in the namespace where ephemeral daemon Pods will be created.
+  namespace: ${EPHEMERAL_DAEMON_NAMESPACE}
 rules:
   - apiGroups: [""]
     resources: ["pods"]
@@ -324,11 +325,13 @@ apiVersion: rbac.authorization.k8s.io/v1
 kind: RoleBinding
 metadata:
   name: github-app-playground-ephemeral-spawner
-  namespace: default
+  # Must match the Role namespace above.
+  namespace: ${EPHEMERAL_DAEMON_NAMESPACE}
 subjects:
   - kind: ServiceAccount
     name: github-app-playground
-    namespace: default
+    # Namespace where the orchestrator ServiceAccount actually lives.
+    namespace: ${ORCHESTRATOR_NAMESPACE}
 roleRef:
   kind: Role
   name: github-app-playground-ephemeral-spawner
@@ -339,4 +342,4 @@ Without these verbs, every scale-up attempt yields `dispatch_reason=ephemeral-sp
 
 ### `daemon-secrets` Secret
 
-Spawned ephemeral daemon Pods receive their configuration via `envFrom: secretRef: daemon-secrets`. Create this Secret once in `EPHEMERAL_DAEMON_NAMESPACE` with the GitHub App credentials used by the orchestrator (so the daemon can reuse the installation-token path), Claude provider keys, and the data-layer URLs (`DATABASE_URL`, `VALKEY_URL`). See [DAEMON.md](DAEMON.md) for the full key list.
+Spawned ephemeral daemon Pods receive their configuration via `envFrom: secretRef: daemon-secrets`. Create this Secret once in `EPHEMERAL_DAEMON_NAMESPACE` with only the daemon runtime values it needs — `DAEMON_AUTH_TOKEN`, Claude provider keys, and the daemon-side data-layer URLs (`DATABASE_URL`, `VALKEY_URL`). Do **not** copy GitHub App private-key material into this Secret: the orchestrator mints installation tokens and hands them to the daemon per job, so expanding the blast radius to every ephemeral Pod is unnecessary. See [DAEMON.md](DAEMON.md) for the full key list.

src/db/migrations/004_collapse_dispatch_to_daemon.sql

@@ -19,10 +19,16 @@ ALTER TABLE executions DROP CONSTRAINT IF EXISTS executions_dispatch_reason_chec
 ALTER TABLE executions DROP CONSTRAINT IF EXISTS executions_dispatch_mode_check;
 ALTER TABLE triage_results DROP CONSTRAINT IF EXISTS triage_results_mode_check;
 
+-- Mirror the open-ended guard used for executions.dispatch_reason below:
+-- rewrite any row whose target/mode is not already 'daemon', regardless of
+-- whether it comes from the documented legacy set. Without this, a stray
+-- value from an operator backfill or an older-branch deploy would survive
+-- the UPDATE and fail the CHECK (= 'daemon') added later in this file,
+-- aborting the whole transaction.
 UPDATE executions SET dispatch_target = 'daemon'
- WHERE dispatch_target IN ('inline', 'shared-runner', 'isolated-job');
+ WHERE dispatch_target IS DISTINCT FROM 'daemon';
 UPDATE executions SET dispatch_mode = 'daemon'
- WHERE dispatch_mode IN ('inline', 'shared-runner', 'isolated-job');
+ WHERE dispatch_mode IS DISTINCT FROM 'daemon';
 
 -- Rewrite any reason not already in the post-collapse set. Keeping this
 -- unconditional (rather than enumerating legacy values) defends against

src/orchestrator/ephemeral-daemon-scaler.ts

@@ -68,6 +68,19 @@ export function markSpawn(now: number): void {
   lastSpawnAtMs = now;
 }
 
+/**
+ * Release a cooldown reservation *only* if it still matches the caller's
+ * attempt timestamp. If a newer reservation has already overtaken it (e.g.
+ * a later spawn won the race while this one's K8s call was still in
+ * flight), leave the newer timestamp in place. An unconditional reset
+ * would reopen the thundering-herd window the cooldown is meant to close.
+ */
+export function rollbackSpawn(expectedTimestampMs: number): void {
+  if (lastSpawnAtMs === expectedTimestampMs) {
+    lastSpawnAtMs = 0;
+  }
+}
+
 /** Test-only: reset the module-level cooldown state between test cases. */
 export function _resetEphemeralScalerForTests(): void {
   lastSpawnAtMs = 0;

src/orchestrator/job-queue.ts

@@ -41,8 +41,11 @@ export async function enqueueJob(job: QueuedJob): Promise<void> {
  * persistent-daemon routing instead of trapping a job.
  */
 export async function getQueueLength(): Promise<number> {
-  const valkey = requireValkeyClient();
   try {
+    // `requireValkeyClient()` itself throws when the client isn't ready, so
+    // acquire inside the try block — otherwise the "return 0 on any read
+    // error" contract silently loses to the client acquisition throw.
+    const valkey = requireValkeyClient();
     // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment -- Valkey LLEN returns number
     const len: number = await valkey.send("LLEN", [QUEUE_KEY]);
     return typeof len === "number" && Number.isFinite(len) ? len : 0;

src/orchestrator/triage.ts

@@ -32,12 +32,22 @@ import { logger } from "../logger";
 import { CircuitBreaker } from "../utils/circuit-breaker";
 import { sanitizeContent } from "../utils/sanitize";
 
-/** Zod schema — binary heavy classifier. */
-export const TriageResponseSchema = z.object({
-  heavy: z.boolean(),
-  confidence: z.number().min(0).max(1),
-  rationale: z.string().min(1).max(500),
-});
+/**
+ * Zod schema — binary heavy classifier.
+ *
+ * `.strict()` so legacy pre-collapse fields (`mode`, `complexity`) on a
+ * response from a stale provider/proxy are a hard parse error instead of
+ * being silently stripped. That forces the `parse-error` fallback branch
+ * in `triageRequest`, which is safer than silently absorbing a shape we
+ * no longer model.
+ */
+export const TriageResponseSchema = z
+  .object({
+    heavy: z.boolean(),
+    confidence: z.number().min(0).max(1),
+    rationale: z.string().min(1).max(500),
+  })
+  .strict();
 export type TriageResponse = z.infer<typeof TriageResponseSchema>;
 
 export interface TriageResult extends TriageResponse {

src/webhook/router.ts

@@ -9,7 +9,11 @@ import {
   isAtCapacity,
 } from "../orchestrator/concurrency";
 import { getPersistentPoolFreeSlots } from "../orchestrator/daemon-registry";
-import { decideEphemeralSpawn, markSpawn } from "../orchestrator/ephemeral-daemon-scaler";
+import {
+  decideEphemeralSpawn,
+  markSpawn,
+  rollbackSpawn,
+} from "../orchestrator/ephemeral-daemon-scaler";
 import { createExecution } from "../orchestrator/history";
 import { dispatchJob } from "../orchestrator/job-dispatcher";
 import { enqueueJob, getQueueLength, type QueuedJob } from "../orchestrator/job-queue";
@@ -240,8 +244,11 @@ export async function decideDispatch(ctx: BotContext): Promise<DispatchDecision>
       ...(triageAttempted && { triageAttempted: true }),
     };
   } catch (err) {
-    // Spawn failed — release the cooldown so the next request can retry.
-    markSpawn(0);
+    // Spawn failed — release only *our* reservation. Unconditionally
+    // zeroing the timestamp would stomp on a newer concurrent spawn
+    // that already won the cooldown race while this call was in flight,
+    // reopening the thundering-herd window the cooldown exists to close.
+    rollbackSpawn(spawnAttemptAt);
     const kind = err instanceof EphemeralSpawnError ? err.kind : undefined;
     const message =
       err instanceof EphemeralSpawnError

test/config.test.ts

@@ -109,6 +109,19 @@ describe("configSchema — data layer validation", () => {
     });
     expect(result.success).toBe(true);
   });
+
+  it("still requires DAEMON_AUTH_TOKEN in daemon mode (ORCHESTRATOR_URL set)", () => {
+    // The DB/Valkey waiver must not cascade into waiving daemon auth —
+    // an orchestrator-connected daemon without a shared token would
+    // accept unauthenticated connections on restart.
+    const { daemonAuthToken, ...withoutToken } = ANTHROPIC_BASE;
+    expect(daemonAuthToken).toBeDefined();
+    const result = configSchema.safeParse({
+      ...withoutToken,
+      orchestratorUrl: "wss://orchestrator.example.com",
+    });
+    expect(result.success).toBe(false);
+  });
 });
 
 describe("configSchema — ephemeral-daemon defaults", () => {
@@ -194,7 +207,12 @@ describe("assertOauthRequiresAllowlist", () => {
   });
 
   it("throws when OAuth is set without an allowlist", () => {
-    const cfg = { ...baseOauthCfg, allowedOwners: undefined };
+    // With `exactOptionalPropertyTypes`, the absence of a property is
+    // distinct from an explicit `undefined`. Destructure the property
+    // out so this test actually models "no allowlist configured" — not
+    // "allowlist is undefined-valued".
+    const { allowedOwners, ...cfg } = baseOauthCfg;
+    expect(allowedOwners).toBeDefined();
     expect(() => {
       assertOauthRequiresAllowlist(cfg);
     }).toThrow(/ALLOWED_OWNERS/);

test/contract/triage-response.test.ts

@@ -56,13 +56,25 @@ describe("TriageResponse schema — invalid inputs", () => {
   });
 
   it("rejects legacy pre-collapse shape (mode/complexity)", () => {
-    const r = TriageResponseSchema.safeParse({
+    // Schema is `.strict()` — missing required `heavy` *or* extra
+    // legacy keys is enough to fail. Cover both so future contributors
+    // know the unknown-key rejection is intentional.
+    const missingHeavy = TriageResponseSchema.safeParse({
       mode: "daemon",
       confidence: 0.5,
       complexity: "trivial",
       rationale: "legacy shape",
     });
-    expect(r.success).toBe(false);
+    expect(missingHeavy.success).toBe(false);
+
+    const legacyKeysPresent = TriageResponseSchema.safeParse({
+      heavy: true,
+      confidence: 0.5,
+      rationale: "legacy fields present",
+      mode: "daemon",
+      complexity: "trivial",
+    });
+    expect(legacyKeysPresent.success).toBe(false);
   });
 
   it("rejects confidence outside [0, 1]", () => {