Skip to content

Commit afbe7ff

Browse files
chrisleekrchrisleekr
committed
fix(review): address copilot comments on dispatch collapse PR
- docs: add missing 'disabled' triage fallback reason (6 not 5) - docs: document ephemeral-daemon-overflow dual-condition trigger - docs: align triageRate(days) description with query semantics - daemon: guard DAEMON_MAX_CONCURRENT_JOBS parse against NaN - history: hardcode dispatch_mode/dispatch_target to 'daemon' for invariant safety
1 parent e5ab5f0 commit afbe7ff

4 files changed

Lines changed: 35 additions & 26 deletions

File tree

‎docs/OBSERVABILITY.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ Structured JSON logs via [pino](https://getpino.io) are the primary signal. Ever
1212
| `dispatch_target` | Always `daemon` (singleton — kept as a field for DB/log stability). |
1313
| `dispatch_reason` | Why the job landed where it did. See below. |
1414
| `isEphemeral` | Present on daemon-originating log lines. `true` if emitted by an ephemeral daemon, `false` otherwise. |
15-
| `triage_fallback_reason` | Only present on triage fallbacks — one of the five values in [Triage](TRIAGE.md#fallback-reasons). |
15+
| `triage_fallback_reason` | Only present on triage fallbacks — one of the six values in [Triage](TRIAGE.md#fallback-reasons). |
1616
| `confidence` | Triage confidence (0–1), only when the decision came from triage. |
1717
| `heavy` | Triage binary signal (`true`/`false`) — only on triage-success. |
1818
| `rationale` | Free-text rationale from the triage LLM. Only on triage-success. |
@@ -26,7 +26,7 @@ Canonical source: `src/shared/dispatch-types.ts`. Four values, all landing on `d
2626
| --------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------- |
2727
| `persistent-daemon` | Routed to an existing persistent daemon. The default, hot path. Also used on cooldown — when a scale-up was warranted but blocked by the cooldown window. |
2828
| `ephemeral-daemon-triage` | Triage returned `heavy=true` and an ephemeral daemon Pod was spawned to claim the job. |
29-
| `ephemeral-daemon-overflow` | Queue length ≥ `EPHEMERAL_DAEMON_SPAWN_QUEUE_THRESHOLD` and an ephemeral daemon Pod was spawned to drain the overflow. |
29+
| `ephemeral-daemon-overflow` | Queue length ≥ `EPHEMERAL_DAEMON_SPAWN_QUEUE_THRESHOLD` **and** the persistent pool is saturated (zero free slots); a spawn drains the overflow. |
3030
| `ephemeral-spawn-failed` | A spawn was required but the K8s API call failed. The job is rejected with a tracking-comment infra error. |
3131

3232
## Aggregate reporting
@@ -36,7 +36,7 @@ When `DATABASE_URL` is set, helpers in `src/db/queries/dispatch-stats.ts` expose
3636
| Helper | Returns |
3737
| -------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
3838
| `eventsPerTarget(days)` | Count of executions grouped by `dispatch_target`. Post-collapse this is always a single `daemon` row — useful only as a liveness counter; query `dispatch_reason` directly for the per-reason split. |
39-
| `triageRate(days)` | Share of events that hit triage vs. short-circuited. |
39+
| `triageRate(days)` | Share of events whose `dispatch_reason` is `ephemeral-daemon-triage` (i.e. triage drove an ephemeral spawn) vs. all events. |
4040
| `avgConfidenceAndFallback(days)` | Mean triage confidence plus fallback counts by reason. |
4141
| `triageSpend(days)` | Cumulative `cost_usd` for triage-reached executions. |
4242

‎docs/TRIAGE.md‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -22,17 +22,18 @@ Triage wraps the LLM call in a circuit breaker (see `src/orchestrator/triage.ts`
2222

2323
## Fallback reasons
2424

25-
Five distinct reasons cause triage to fall back to `heavy=false` (i.e. route to `persistent-daemon`):
26-
27-
| Reason | Trigger |
28-
| --------------- | --------------------------------------------------------------------- |
29-
| `circuit-open` | The circuit breaker tripped after consecutive failures. |
30-
| `timeout` | The call exceeded `TRIAGE_TIMEOUT_MS`. |
31-
| `llm-error` | The provider returned an error. |
32-
| `parse-error` | The JSON response could not be validated against the expected schema. |
33-
| `sub-threshold` | Parsed successfully but `confidence < TRIAGE_CONFIDENCE_THRESHOLD`. |
34-
35-
All five appear in Pino logs as `triage_fallback_reason`. Canonical values live in `src/orchestrator/triage.ts`.
25+
Six distinct reasons cause triage to fall back to `heavy=false` (i.e. route to `persistent-daemon`):
26+
27+
| Reason | Trigger |
28+
| --------------- | ----------------------------------------------------------------------- |
29+
| `disabled` | `TRIAGE_ENABLED=false` — triage short-circuits without calling the LLM. |
30+
| `circuit-open` | The circuit breaker tripped after consecutive failures. |
31+
| `timeout` | The call exceeded `TRIAGE_TIMEOUT_MS`. |
32+
| `llm-error` | The provider returned an error. |
33+
| `parse-error` | The JSON response could not be validated against the expected schema. |
34+
| `sub-threshold` | Parsed successfully but `confidence < TRIAGE_CONFIDENCE_THRESHOLD`. |
35+
36+
All six appear in Pino logs as `triage_fallback_reason`. Canonical values live in `src/orchestrator/triage.ts`.
3637

3738
## Cost implications
3839

‎src/daemon/tool-discovery.ts‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,18 @@ function detectEphemeral(): boolean {
239239
return raw === "true" || raw === "1" || raw === "yes";
240240
}
241241

242+
const DEFAULT_MAX_CONCURRENT_JOBS = 3;
243+
244+
// parseInt returns NaN for empty strings or non-numeric input, and
245+
// `??` only falls back when undefined — so a malformed env var would
246+
// propagate NaN into the Zod schema (`int().positive()`) and fail
247+
// `daemon:register`. Guard explicitly on Number.isInteger and >0.
248+
function parseMaxConcurrentJobs(raw: string | undefined): number {
249+
if (raw === undefined) return DEFAULT_MAX_CONCURRENT_JOBS;
250+
const parsed = Number.parseInt(raw, 10);
251+
return Number.isInteger(parsed) && parsed > 0 ? parsed : DEFAULT_MAX_CONCURRENT_JOBS;
252+
}
253+
242254
// Auth + repo probes
243255

244256
async function probeAuthContexts(): Promise<string[]> {
@@ -358,10 +370,7 @@ export async function discoverCapabilities(cloneBaseDir: string): Promise<Daemon
358370
cachedRepos,
359371
ephemeral,
360372
maxUptimeMs: ephemeral ? 3_600_000 : null,
361-
maxConcurrentJobs: Math.max(
362-
1,
363-
Number.parseInt(process.env["DAEMON_MAX_CONCURRENT_JOBS"] ?? "3", 10),
364-
),
373+
maxConcurrentJobs: parseMaxConcurrentJobs(process.env["DAEMON_MAX_CONCURRENT_JOBS"]),
365374
};
366375

367376
logger.debug(

‎src/orchestrator/history.ts‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,10 @@ export async function createExecution(params: CreateExecutionParams): Promise<st
5959
throw new Error("createExecution: dispatchReason is required when triage fields are provided");
6060
}
6161

62-
// Migration 003 denormalizes dispatch onto `executions` with
63-
// `dispatch_target` as the canonical column for new rows; `dispatch_mode`
64-
// stays populated for backward compat (the migration note calls for a
65-
// future consolidation). Callers pass the resolved DispatchTarget via
66-
// `dispatchMode` — we write it to both columns.
62+
// Post migration 004 both `dispatch_mode` and `dispatch_target` carry a
63+
// CHECK (= 'daemon'). Hardcoding the literal here makes the invariant
64+
// unbreakable at the data-layer boundary — a stray caller passing any
65+
// other `dispatchMode` value cannot fail the INSERT at runtime.
6766
let rows: { id: string }[];
6867
if (hasTriageFields) {
6968
rows = await db`
@@ -75,7 +74,7 @@ export async function createExecution(params: CreateExecutionParams): Promise<st
7574
) VALUES (
7675
${params.deliveryId}, ${params.repoOwner}, ${params.repoName},
7776
${params.entityNumber}, ${params.entityType}, ${params.eventName},
78-
${params.triggerUsername}, ${params.dispatchMode}, ${params.dispatchMode},
77+
${params.triggerUsername}, 'daemon', 'daemon',
7978
${params.dispatchReason ?? "persistent-daemon"},
8079
${params.triageConfidence ?? null}, ${params.triageCostUsd ?? null},
8180
'queued', ${params.contextJson ?? null}
@@ -91,7 +90,7 @@ export async function createExecution(params: CreateExecutionParams): Promise<st
9190
) VALUES (
9291
${params.deliveryId}, ${params.repoOwner}, ${params.repoName},
9392
${params.entityNumber}, ${params.entityType}, ${params.eventName},
94-
${params.triggerUsername}, ${params.dispatchMode}, ${params.dispatchMode}, ${params.dispatchReason},
93+
${params.triggerUsername}, 'daemon', 'daemon', ${params.dispatchReason},
9594
'queued', ${params.contextJson ?? null}
9695
)
9796
RETURNING id
@@ -104,7 +103,7 @@ export async function createExecution(params: CreateExecutionParams): Promise<st
104103
) VALUES (
105104
${params.deliveryId}, ${params.repoOwner}, ${params.repoName},
106105
${params.entityNumber}, ${params.entityType}, ${params.eventName},
107-
${params.triggerUsername}, ${params.dispatchMode}, ${params.dispatchMode}, 'queued',
106+
${params.triggerUsername}, 'daemon', 'daemon', 'queued',
108107
${params.contextJson ?? null}
109108
)
110109
RETURNING id

0 commit comments

Comments
 (0)