diff --git a/CHANGELOG.md b/CHANGELOG.md index 6be893183..d70ab8a28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable user-visible changes to CASCADE are documented here. The format is l ## Unreleased +### Documentation + +- **Trigger architecture docs now describe the migrated trigger contracts.** Added guidance for canonical `TRIGGER_EVENTS`, shared PM/GitHub result builders, first-match dispatch, structured skip vs bare `null`, no-agent results, deferred bare-job re-checks, router outcome decision reasons, PM coalescing, capacity scope, dispatch failure compensation, and wedged-lock diagnostics. Migration note for future trigger contributors: new handlers should import event constants, use the shared builders, return structured skips for claimed-but-non-dispatched events, and reserve bare `null` for "continue to later handlers." See Trello card [qUbPtALY](https://trello.com/c/69fe2a950699baaf91688a5b). + ### Fixed - **`resolve-conflicts` agent no longer silently skips when GitHub's async mergeability computation hasn't resolved by the time the `pull_request` webhook is processed** (spec 020). `PRConflictDetectedTrigger` previously exhausted a 2×2s synchronous retry budget and silently discarded the event when `mergeable === null` — because GitHub never sends a follow-up webhook once mergeability resolves, the `resolve-conflicts` agent never fired. The trigger now returns `TriggerResult.deferredRecheck`, which causes the router to schedule a bare BullMQ delayed re-check job ~45s later via `scheduleCoalescedJob` (deduped per PR). The worker re-dispatches via the trigger registry to get fresh mergeability state. Multiple rapid webhooks for the same PR coalesce to a single re-check job. If mergeability is still `null` after the re-check fires, a Sentry event is captured under tag `mergeability_recheck_exhausted` and a WARN log is emitted — not a silent discard. Observed live on ucho/PR #329 (2026-05-07). See [spec 020](docs/specs/020-github-mergeability-deferred-recheck.md). diff --git a/CLAUDE.md b/CLAUDE.md index 48e25e73e..aeab911e6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -123,7 +123,7 @@ Some triggers take params (e.g. `review` + `scm:check-suite-success` accepts `{" **Post-completion review dispatch** — when an implementation agent succeeds with a PR, the execution pipeline checks CI status and fires the review agent deterministically (before the container exits). This guarantees review dispatch within seconds of implementation completion, regardless of GitHub webhook timing. Uses the same `claimReviewDispatch` dedup key as the `check-suite-success` trigger, so the two paths cannot double-enqueue. -**Deferred re-check** — any trigger handler can return `TriggerResult.deferredRecheck: { delayMs, coalesceKey }` (with `agentType: null`) to schedule a bare delayed job that re-dispatches via the trigger registry when it fires. The router uses `scheduleCoalescedJob` for dedup. The worker detects re-check jobs via `GitHubJob.mergeabilityRecheckAttempt` and Sentry-captures under tag `mergeability_recheck_exhausted` when the trigger still cannot resolve state. Workers do not re-queue — a second `deferredRecheck` return in the worker handler exits gracefully. +**Deferred re-check** — a trigger handler can return `TriggerResult.deferredRecheck: { delayMs, coalesceKey }` (with `agentType: null`) to schedule a bare delayed job via `scheduleCoalescedJob`. The router scheduling is adapter-agnostic, but **bare re-dispatch is currently GitHub-only**: `GitHubRouterAdapter.buildJob()` strips `triggerResult` from the job so the GitHub worker re-dispatches through the trigger registry for fresh provider state. Non-GitHub adapters (Trello, JIRA, Linear, Sentry) embed `triggerResult` in the job; their workers pass it to `resolveTriggerResult()`, which returns the pre-resolved `agentType: null` result without re-dispatching — a non-GitHub handler using this field would schedule a job that reuses the same result rather than re-evaluating provider state. The worker detects GitHub re-check jobs via `GitHubJob.mergeabilityRecheckAttempt` and Sentry-captures under tag `mergeability_recheck_exhausted` when state still cannot resolve. Workers do not re-queue — a second `deferredRecheck` return exits gracefully. **Worker exit diagnostics** — when a worker container exits non-zero, the router calls `container.inspect()` *before* AutoRemove reaps it and stamps the run record's `error` field with a structured, grep-stable string: `Worker crashed with exit code N · OOMKilled= · reason=""`. The `OOMKilled=true` marker is the definitive cgroup-OOM signal (per Docker's own `State.OOMKilled`); a 137 exit *without* `OOMKilled=true` means the kill came from inside the container or from a non-cgroup signal — *not* memory. The `[WorkerManager] Resolved spawn settings` log emitted at every spawn includes both `projectWatchdogTimeoutMs` and `globalWorkerTimeoutMs` so post-mortems can confirm whether the per-project override actually won. See `src/router/active-workers.ts:formatCrashReason` for the format and `tests/unit/router/container-manager-diagnostics.test.ts` for regression pins. diff --git a/docs/architecture/02-webhook-pipeline.md b/docs/architecture/02-webhook-pipeline.md index f818f2003..fd87b5b48 100644 --- a/docs/architecture/02-webhook-pipeline.md +++ b/docs/architecture/02-webhook-pipeline.md @@ -102,7 +102,8 @@ flowchart TD F -->|Not found| SKIP4[Skip: no project config] F -->|Found| G[7. Dispatch triggers with credentials] G -->|No match| SKIP5[Skip: no trigger matched] - G -->|Matched| H{8. Work-item / agent-type lock} + G -->|Structured skip / no-agent / deferred| OUTCOME[Handle non-dispatch outcome] + G -->|Agent dispatch| H{8. Work-item / agent-type lock} H -->|Locked| SKIP6[Skip: concurrency limit] H -->|Free| I[9. Post ack comment] I --> J[10. Build job] @@ -118,13 +119,36 @@ flowchart TD 4. **Self-check** — Adapter's `isSelfAuthored()` detects bot's own actions (loop prevention) 5. **Reaction** — Fire-and-forget emoji reaction on the source event 6. **Resolve config** — Look up project by platform identifier (board ID, repo, etc.) -7. **Dispatch triggers** — Within credential scope, call `TriggerRegistry.dispatch()` to find a matching agent. PM router adapters also wrap dispatch in `withPMScopeForDispatch(fullProject, dispatch)` so shared PM gates can resolve the active provider. +7. **Dispatch triggers** — Within credential scope, call `TriggerRegistry.dispatch()` to find a matching result. PM router adapters also wrap dispatch in `withPMScopeForDispatch(fullProject, dispatch)` so shared PM gates can resolve the active provider. 8. **Concurrency** — Check work-item lock (`work-item-lock.ts`) and agent-type concurrency (`agent-type-lock.ts`) 9. **Ack comment** — Post an acknowledgment comment to the work item or PR 10. **Build job** — Package trigger result + payload + ack info into a `CascadeJob` 11. **Pre-actions** — Optional fire-and-forget actions (e.g., GitHub eyes reaction) 12. **Enqueue** — Add job to BullMQ Redis queue; mark work item and agent type as enqueued +### Router outcomes + +`src/router/webhook-trigger-outcomes.ts` normalizes trigger results into stable router decisions: + +| Trigger result | Router behavior | Decision reason shape | +|----------------|-----------------|-----------------------| +| `null` from registry | No handler claimed the event | `No trigger matched for event` | +| `agentType: null` + `skipReason` | Handler claimed the event but intentionally self-skipped | `Trigger skipped: ` | +| `agentType: null` + `deferredRecheck` | Schedule a coalesced delayed bare job and exit | `Deferred re-check scheduled: ` | +| `agentType: null` without skip/defer | Side-effect-only trigger completed | `Trigger completed without agent (PM operation)` | +| `agentType` + `coalesceKey` and coalescing enabled | Schedule a delayed coalesced dispatch | `Coalesced dispatch scheduled: agent for work item ` | +| `agentType` without coalescing | Post ack, build job, enqueue now | `Job queued: agent for work item ` | +| Immediate-dispatch or PM coalesced-dispatch Redis failure | Call `onBlocked` and leave a failure reason | `Failed to enqueue job to Redis` or `Failed to schedule coalesced job to Redis` | +| Deferred re-check Redis failure | Capture Sentry under `deferred_recheck_schedule_failure`; skip `onBlocked`; treat as if scheduled | `Deferred re-check scheduled: ` | + +Structured skip is intentionally different from bare `null`: it preserves the handler's reason in webhook logs instead of collapsing expected non-dispatch decisions into "no trigger matched." + +### Coalescing and deferred re-check + +PM status-change dispatches can include a `coalesceKey`, normally `${projectId}:${workItemId}`. When `PM_COALESCE_WINDOW_MS` is positive, the router schedules a delayed job via `scheduleCoalescedJob`; a newer dispatch with the same key supersedes the pending one and releases the superseded job's in-memory locks. PM ack comments are deferred to job fire time for coalesced jobs so superseded work does not leave orphan comments. + +Deferred re-check also uses `scheduleCoalescedJob` and exits without dispatch locks or an ack comment. The bare re-dispatch on job fire is currently **GitHub-only**: `GitHubRouterAdapter.buildJob()` strips `triggerResult` and sets `mergeabilityRecheckAttempt: 1`, so the GitHub worker re-dispatches through the trigger registry to evaluate fresh provider state. Non-GitHub adapters (Trello, JIRA, Linear, Sentry) embed `triggerResult` in the job regardless of `deferredRecheck`, so their workers return the pre-resolved `agentType: null` result directly without re-dispatching. If a deferred re-check schedule call fails, the router captures Sentry under `deferred_recheck_schedule_failure` and still returns `Deferred re-check scheduled` — it does not call `onBlocked`. GitHub mergeability uses this when `mergeable` is still `null` after the synchronous retry budget; if the re-check still cannot resolve state, the GitHub worker records `mergeability_recheck_exhausted` and stops rather than re-queueing indefinitely. + ### Concurrency controls | Mechanism | File | Purpose | @@ -136,6 +160,12 @@ flowchart TD All locks are in-memory with TTL expiry. Work-item locks are scoped by `(projectId, workItemId, agentType)`: duplicate runs of the same agent are blocked, but different agent types can run concurrently on the same work item. When a lock rejects a webhook, logs distinguish `Awaiting worker slot` from `Work item locked (no active dispatch)`; the latter is a wedged-lock canary and captures to Sentry. +The work-item lock decision vocabulary is stable by design: + +- `Job queued: ...` means the router successfully registered a dispatch and enqueued or scheduled work. +- `Awaiting worker slot: ...` means the same work item and agent type already have an active queued/waiting/running dispatch. +- `Work item locked (no active dispatch): ...` means the lock-state classifier could not correlate the lock with queued or running work. This is a wedged-lock canary, not normal backpressure. + ## Signature Verification `src/router/webhookVerification.ts` diff --git a/docs/architecture/03-trigger-system.md b/docs/architecture/03-trigger-system.md index 98cbea9f9..6352548be 100644 --- a/docs/architecture/03-trigger-system.md +++ b/docs/architecture/03-trigger-system.md @@ -21,6 +21,8 @@ class TriggerRegistry { 2. Call `handle(ctx)` — if it returns a `TriggerResult`, return it 3. If `handle()` returns `null`, continue to next handler +That makes dispatch first-match-wins for non-null results. A handler should return bare `null` only when it does not claim the event and later handlers should still get a chance. If a handler claims the event but decides not to run an agent, it returns a structured `TriggerResult` with `agentType: null` instead. + ## TriggerHandler `src/triggers/types.ts` @@ -58,6 +60,12 @@ interface TriggerResult { prUrl?: string; prTitle?: string; onBlocked?: () => void; // Cleanup if job can't be enqueued + skipReason?: { + handler: string; + message: string; + }; + lockKey?: string; // Optional work-item lock override + coalesceKey?: string; // Optional PM dispatch coalescing key deferredRecheck?: { delayMs: number; coalesceKey: string; @@ -135,15 +143,39 @@ function registerBuiltInTriggers(registry: TriggerRegistry): void { ### Event format -Triggers use category-prefixed events: `{category}:{event-name}` -- `pm:status-changed`, `pm:label-added` -- `scm:check-suite-success`, `scm:pr-review-submitted`, `scm:review-requested` -- `alerting:issue-alert`, `alerting:metric-alert` +Triggers use category-prefixed events from `src/triggers/shared/events.ts`. `TRIGGER_EVENTS` is the canonical catalog used by handlers, result builders, trigger configuration, and static tests: + +- PM: `pm:status-changed`, `pm:label-added`, `pm:comment-mention` +- SCM: `scm:check-suite-success`, `scm:check-suite-failure`, `scm:pr-review-submitted`, `scm:review-requested`, `scm:pr-opened`, `scm:pr-comment-mention`, `scm:pr-merged`, `scm:pr-ready-to-merge`, `scm:pr-conflict-detected` +- Alerting: `alerting:issue-alert`, `alerting:metric-alert` +- Internal: `internal:auto-chain` + +New handlers should import `TRIGGER_EVENTS` instead of adding raw string literals. The static guard in `tests/unit/triggers/trigger-event-consistency.test.ts` fails when a handler gates on one event string and emits a different `agentInput.triggerEvent`. + +### Result builders + +Shared builders live in `src/triggers/shared/result-builders.ts`, `src/triggers/shared/pm-status.ts`, `src/triggers/shared/pm-label.ts`, and `src/triggers/github/result-builders.ts`. + +Use them for new handlers unless there is a concrete reason not to: + +- `buildPMDispatchResult`, `buildPMStatusDispatchResult`, and `buildPMLabelDispatchResult` attach canonical PM trigger events, work-item fields, and PM coalescing keys. +- `buildGitHubPRDispatchResult` and the GitHub-specific wrappers attach PR metadata, optional linked PM work-item metadata, and normalized agent input for PR agents. +- `buildNoAgentResult` represents a matched trigger whose side effect is complete without spawning an agent, such as PM status updates after a PR merge. +- `buildSkipResult` or `skip()` represents a matched trigger that intentionally stops dispatch with a human-readable reason. +- `buildDeferredRecheckResult` represents a delayed bare-job re-dispatch. + +### Structured skip vs bare `null` + +Bare `null` means "this handler did not handle the event; continue registry dispatch." Structured skip means "this handler did handle the event; stop dispatch and record why no agent was queued." + +The router preserves structured skips in webhook logs with `Trigger skipped: `. Use structured skip for disabled trigger config, author-mode gates, self-loop gates, incomplete aggregate check-suite state, missing PR/work-item prerequisites, and similar expected non-dispatch outcomes. ### Deferred re-checks Handlers that cannot make a final decision yet can return `deferredRecheck: { delayMs, coalesceKey }` with `agentType: null`. The router schedules a coalesced delayed BullMQ job and exits without spawning an agent. GitHub mergeability checks use this path; the worker recognizes re-check jobs via `mergeabilityRecheckAttempt` and captures a Sentry diagnostic if the second pass still cannot resolve state. +The bare re-dispatch on job fire is currently **GitHub-only**: `GitHubRouterAdapter.buildJob()` strips `triggerResult` and sets `mergeabilityRecheckAttempt: 1`, so the GitHub worker re-dispatches through the trigger registry to evaluate fresh provider state. Non-GitHub adapters (Trello, JIRA, Linear, Sentry) embed `triggerResult` in the job regardless of `deferredRecheck`; `resolveTriggerResult()` returns the pre-resolved result directly, skipping registry dispatch. A non-GitHub handler returning `buildDeferredRecheckResult` would therefore schedule a job that reuses the same `agentType: null` result rather than re-evaluating provider state. See `src/triggers/README.md` for the full authoring contract. Workers do not schedule another re-check after exhaustion. + ### Config resolution `src/triggers/config-resolver.ts` diff --git a/docs/architecture/07-gadgets.md b/docs/architecture/07-gadgets.md index acfcdcfc7..ef213f217 100644 --- a/docs/architecture/07-gadgets.md +++ b/docs/architecture/07-gadgets.md @@ -108,6 +108,20 @@ Native-tool engines cannot invoke gadget classes directly (they run as subproces The `cascade-tools` binary uses a separate oclif config (`bin/cascade-tools.js`) that discovers all non-dashboard commands, while `cascade` discovers only dashboard commands. +`createCLICommand()` is the stable facade used by command files under `src/cli/**`. Shared CLI behavior lives in focused helper modules under `src/gadgets/shared/cli/`: + +| Helper | Role | +|--------|------| +| `commandNames.ts` | Command namespace/name derivation shared by the CLI factory and manifest generator | +| `examples.ts` | Tool example lookup, shell quoting, oclif example rendering, and JSON expected-shape hints | +| `flags.ts` | oclif flag construction and flag metadata collection | +| `booleanArgv.ts` | Boolean value-form normalization before oclif parsing | +| `parseErrors.ts` | oclif parse-error classification and unknown-flag suggestions | +| `params.ts` | File/stdin input, JSON parsing, direct parameter resolution, and git remote owner/repo resolution | +| `errorSink.ts` | Error-envelope routing through the active command instance | + +New domain commands should not add branches in these helpers. They declare behavior through their `ToolDefinition` metadata (`cliAliases`, examples, file input alternatives, auto-resolution), and the shared generators consume it. + ## Session State `src/gadgets/sessionState.ts` diff --git a/docs/architecture/10-resilience.md b/docs/architecture/10-resilience.md index 2436fed5e..04aa2dd7a 100644 --- a/docs/architecture/10-resilience.md +++ b/docs/architecture/10-resilience.md @@ -45,6 +45,8 @@ Configurable `max_concurrency` per agent type per project (set via `agent_config `projects.max_in_flight_items` — project-level cap on total concurrent agent runs. Checked during trigger dispatch. +This gate is PM-provider scoped. PM router adapters enter `withPMScopeForDispatch(fullProject, dispatch)` before calling `TriggerRegistry.dispatch()` so shared trigger gates can call `getPMProvider()` and count active PM work items. If the scope is missing, the gate fails closed: it blocks dispatch, logs at error level, and captures to Sentry under `pipeline_capacity_gate_no_pm_provider`. This protects the PM-source path where `maxInFlightItems` matters most. + ### BullMQ concurrency The router's worker manager limits how many Docker containers run in parallel via `routerConfig.maxWorkers`. @@ -72,7 +74,29 @@ The router queues `cascade-jobs` and `cascade-dashboard-jobs` with `attempts: 4` - Transient: Docker socket `ECONNREFUSED` / `ECONNRESET` / `ENOTFOUND`, registry HTTP 429, container-name HTTP 409, and `SLOT_WAIT_TIMEOUT`. - Terminal: validation errors (`TypeError`, `ZodError`) and image-not-found after fallback exhaustion. -Every failed dispatch path flows through the BullMQ `failed` event and calls `releaseLocksForFailedJob`, releasing the work-item lock, agent-type counter, and recently-dispatched mark. Webhook logs distinguish healthy backpressure (`Awaiting worker slot`) from the wedged-lock canary (`Work item locked (no active dispatch)`). +Post-enqueue dispatch failures (Docker socket errors, slot-wait timeouts, container failures) flow through the BullMQ `failed` event and call `releaseLocksForFailedJob`, releasing the work-item lock, agent-type counter, and recently-dispatched mark. Webhook logs distinguish healthy backpressure (`Awaiting worker slot`) from the wedged-lock canary (`Work item locked (no active dispatch)`). Enqueue/schedule failures that occur before a BullMQ job exists are handled differently — see the split below. + +The compensation operates at two distinct boundaries — not a single unified path: + +- **Enqueue/schedule failures** (`addJob` or `scheduleCoalescedJob` throws before any BullMQ job exists) — the router catches the error inline, calls `onBlocked()` to clear any pre-checked state, and returns a failure decision reason (`Failed to enqueue job to Redis` or `Failed to schedule coalesced job to Redis`). Lock protection here relies on the success-first ordering contract: `markImmediateDispatchEnqueued` and `markCoalescedDispatchEnqueued` are called only after a successful enqueue, so a Redis failure leaves no lock marked. There is no BullMQ job and therefore no `failed` event and no `releaseLocksForFailedJob` on this path. (Deferred re-check schedule failures are a special case: the router does not call `onBlocked` and still returns the scheduled decision reason; see the router outcomes table in `docs/architecture/02-webhook-pipeline.md`.) +- **Post-enqueue dispatch failures** (Docker socket errors, registry pull failures, container-name collisions, slot-wait timeouts) — a job already exists in BullMQ and locks are already marked. BullMQ retries under `attempts: 4` + exponential backoff. On exhaustion, `worker.on('failed')` calls `releaseLocksForFailedJob`, clearing the in-memory work-item lock, agent-type counter, and recently-dispatched dedup marker. + +This split prevents both classes of failure from wedging a work item for the lock TTL: enqueue/schedule failures never mark a lock in the first place; post-enqueue failures eventually flow through `releaseLocksForFailedJob` compensation. + +### Deferred re-check exhaustion + +Some provider state is eventually consistent and has no follow-up webhook. A trigger can return `TriggerResult.deferredRecheck` with `agentType: null`; the router schedules a coalesced delayed bare job and does not take normal dispatch locks. The bare re-dispatch on job fire is currently **GitHub-only**: `GitHubRouterAdapter.buildJob()` strips `triggerResult` and sets `mergeabilityRecheckAttempt: 1`, so the GitHub worker re-dispatches through the registry to get fresh provider state. Non-GitHub adapters (Trello, JIRA, Linear, Sentry) embed `triggerResult` in the job; their workers return the pre-resolved `agentType: null` result directly without re-dispatching through the registry. + +GitHub mergeability uses this for `pull_request` events where `mergeable === null`. If the deferred job still gets another deferred result, workers do not schedule a second re-check. The GitHub worker emits a WARN and captures to Sentry with tag `mergeability_recheck_exhausted`, making pathological provider latency visible without creating an infinite retry loop. + +### Wedged-lock canary + +The router classifies work-item lock rejections in `src/router/lock-state-classifier.ts`. + +- `Awaiting worker slot: ...` is healthy backpressure: the lock correlates with queued, waiting, or running dispatch state. +- `Work item locked (no active dispatch): ...` is a canary: the classifier found a lock but no matching active dispatch. This captures to Sentry under `wedged_lock_canary`. + +The canary should not appear during normal operation. Its presence means a path acquired a lock without completing registration or compensation. ### LLM/API retries diff --git a/src/agents/definitions/alerting.yaml b/src/agents/definitions/alerting.yaml index 9277e7f9f..b2ddeaca4 100644 --- a/src/agents/definitions/alerting.yaml +++ b/src/agents/definitions/alerting.yaml @@ -42,6 +42,12 @@ triggers: options: [critical, warning] defaultValue: critical contextPipeline: [directoryListing, contextFiles] + - event: alerting:issue-lifecycle + label: Sentry Issue Created (Internal Integration) + description: "Trigger when a Sentry issue is created via the Internal Integration / Custom Webhook surface (Sentry-Hook-Resource: issue)" + defaultEnabled: true + providers: [sentry] + contextPipeline: [alertingIssue, directoryListing, contextFiles] strategies: {} diff --git a/src/backends/codex/index.ts b/src/backends/codex/index.ts index ed6d194a8..61f62a6e0 100644 --- a/src/backends/codex/index.ts +++ b/src/backends/codex/index.ts @@ -27,6 +27,19 @@ import { const CODEX_AUTH_DIR = join(homedir(), '.codex'); const CODEX_AUTH_FILE = join(CODEX_AUTH_DIR, 'auth.json'); +/** + * Codex's persistent-bash-session corruption signal. When this stderr message + * appears, codex's `tools::router` has lost its long-lived bash session, and + * every subsequent command in the run inherits a stale stdout buffer. Treat + * any presence of this signal as a fatal run-level error so ops retry + * against a fresh session instead of trusting potentially-corrupted state. + * + * Source: prod runs 8b000cd6 + d8e31665 (cascade/implementation/codex, + * 2026-05-09). Both runs continued executing after the signal and produced + * silent failures with missing sidecars and bled-over command output. + */ +const SHELL_CORRUPTED_RE = /codex_core::tools::router:\s*error=write_stdin failed: stdin is closed/; + type JsonRecord = Record; /** * Accumulator for a single Codex turn (bounded by turn.started → turn.completed). @@ -572,6 +585,33 @@ export class CodexEngine extends NativeToolEngine { input.logWriter('WARN', 'Codex stderr output', { stderr: stderrOutput }); } + // Prod regression 2026-05-09 (runs 8b000cd6, d8e31665): codex's + // persistent bash session breaks with `ERROR + // codex_core::tools::router: error=write_stdin failed: stdin is + // closed for this session`. Once that signal fires, every + // subsequent command in the session inherits a corrupted stdout + // buffer (lint output from one command bleeding into the next, + // sidecar writes racing). We observed this leading to silent run + // failures with PR-creation evidence missing despite a real PR + // existing on GitHub. Codex itself exits cleanly (exit=0) — the + // stderr signal is the only evidence the session was corrupted. + // Surface it as a run failure so ops can retry against a fresh + // session rather than continuing in a corrupted state. + if (exitCode === 0 && SHELL_CORRUPTED_RE.test(stderrOutput)) { + input.logWriter('ERROR', 'Codex shell-state corrupted (write_stdin closed) — failing run', { + stderr: stderrOutput, + hint: 'codex_core::tools::router lost its persistent bash session; subsequent commands may have inherited stale state', + }); + return buildEngineResult({ + success: false, + output: finalOutput, + error: 'codex shell-state corrupted: write_stdin failed (stdin closed for session)', + cost, + prUrl, + prEvidence, + }); + } + if (exitCode !== 0) { return buildEngineResult({ success: false, diff --git a/src/backends/postProcess.ts b/src/backends/postProcess.ts index 761a5fb1e..e07b66491 100644 --- a/src/backends/postProcess.ts +++ b/src/backends/postProcess.ts @@ -1,3 +1,4 @@ +import { captureException } from '../sentry.js'; import { logger } from '../utils/logging.js'; import { COMPLETION_ERROR_NO_PM_WRITE, @@ -26,13 +27,31 @@ export function postProcessResult( hasPMWrite?: boolean; }, ): void { - // Validate PR creation for agents that require it (e.g., implementation) + // Validate PR creation for agents that require it (e.g., implementation). + // Prod regression 2026-05-09 (run d8e31665): the no-authoritative-PR failure + // surfaced only as a per-run record + a one-line WARN. Operators reading + // `cascade runs list` saw "failed" with no idea whether this is a recurring + // regression. Sentry capture under a stable tag makes prod frequency loud + // and gives ops a single dashboard to monitor. if (options?.requiresPR && result.success && !result.prEvidence?.authoritative) { + const prEvidenceSource = result.prEvidence?.source ?? null; logger.warn(`${agentType} agent completed without authoritative PR evidence`, { identifier, engine: engine.definition.id, prUrl: result.prUrl, - prEvidenceSource: result.prEvidence?.source ?? null, + prEvidenceSource, + }); + captureException(new Error(COMPLETION_ERROR_NO_PR), { + tags: { + source: 'pr_sidecar_invalid', + engine: engine.definition.id, + agentType, + }, + extra: { + identifier, + prUrl: result.prUrl, + prEvidenceSource, + }, }); result.success = false; result.error = COMPLETION_ERROR_NO_PR; diff --git a/src/backends/sidecarManager.ts b/src/backends/sidecarManager.ts index 3f1b5d902..c86806f36 100644 --- a/src/backends/sidecarManager.ts +++ b/src/backends/sidecarManager.ts @@ -1,4 +1,4 @@ -import { unlinkSync } from 'node:fs'; +import { existsSync, readFileSync, unlinkSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -136,8 +136,32 @@ export async function hydratePrSidecar(sidecarPath: string): Promise<{ }, }; } + // Prod regression 2026-05-09 (run d8e31665): "PR sidecar missing required + // fields { hasPrUrl: false }" was the only WARN — operators had no signal + // of whether the file was empty, malformed, or had a non-prUrl field. + // Re-read the raw payload and dump the keys + actual values so a single + // log line tells the whole story. The discriminated 'rawSidecarStatus' + // field makes empty vs malformed vs missing vs parsed-without-prUrl each + // produce a distinct log shape so no Loki archaeology is needed. + const rawSidecar = readRawPRSidecar(sidecarPath); + const parsedData = rawSidecar.status === 'parsed' ? rawSidecar.data : null; logger.warn('PR sidecar missing required fields', { + sidecarPath, hasPrUrl: !!sidecar.prUrl, + rawSidecarStatus: rawSidecar.status, + rawSidecarKeys: parsedData ? Object.keys(parsedData).sort() : null, + rawSidecarPrUrl: + parsedData !== null && typeof parsedData.prUrl !== 'undefined' ? parsedData.prUrl : null, + rawSidecarPrNumber: + parsedData !== null && typeof parsedData.prNumber !== 'undefined' + ? parsedData.prNumber + : null, + rawByteLength: + rawSidecar.status === 'empty' || rawSidecar.status === 'malformed' + ? rawSidecar.rawByteLength + : undefined, + parseError: rawSidecar.status === 'malformed' ? rawSidecar.parseError : undefined, + rawPreview: rawSidecar.status === 'malformed' ? rawSidecar.rawPreview : undefined, }); } catch (err) { logger.warn('Failed to read PR sidecar', { path: sidecarPath, error: String(err) }); @@ -146,6 +170,59 @@ export async function hydratePrSidecar(sidecarPath: string): Promise<{ return {}; } +/** + * Discriminated diagnostic result from reading the raw PR sidecar file. + * Returned by `readRawPRSidecar` so the WARN log can tell apart: + * - 'missing' — file never written (agent exited before calling cascade-tools) + * - 'empty' — file exists but is zero/whitespace bytes (truncated mid-write or race) + * - 'malformed' — JSON parse failed (partial write, encoding issue); rawPreview helps triage + * - 'parsed' — valid JSON object (normal path — prUrl simply absent from payload) + */ +type RawSidecarDiagnostic = + | { status: 'missing' } + | { status: 'empty'; rawByteLength: number } + | { status: 'malformed'; rawByteLength: number; parseError: string; rawPreview: string } + | { status: 'parsed'; data: Record }; + +/** + * Best-effort raw read of the sidecar JSON for diagnostic purposes. + * Returns a discriminated union instead of a nullable object so the WARN log + * can distinguish 'no file' / 'empty file' / 'malformed JSON' / 'valid JSON without prUrl'. + * Each case produces a distinct log shape — operators can triage from a single log line + * without Loki archaeology. + */ +function readRawPRSidecar(path: string): RawSidecarDiagnostic { + if (!existsSync(path)) return { status: 'missing' }; + let raw: string; + try { + raw = readFileSync(path, 'utf-8'); + } catch (err) { + // readFileSync itself threw (permissions, I/O error) — treat as malformed. + return { status: 'malformed', rawByteLength: 0, parseError: String(err), rawPreview: '' }; + } + if (!raw.trim()) return { status: 'empty', rawByteLength: raw.length }; + try { + const parsed = JSON.parse(raw); + if (parsed && typeof parsed === 'object') { + return { status: 'parsed', data: parsed as Record }; + } + // JSON.parse succeeded but returned a primitive (null, number, string) — not an object. + return { + status: 'malformed', + rawByteLength: raw.length, + parseError: 'parsed value is not an object', + rawPreview: raw.slice(0, 200), + }; + } catch (err) { + return { + status: 'malformed', + rawByteLength: raw.length, + parseError: String(err), + rawPreview: raw.slice(0, 200), + }; + } +} + /** * Hydrate native tool sidecars (PR and review) after engine execution. * Updates the result in-place with any authoritative PR evidence. diff --git a/src/gadgets/README.md b/src/gadgets/README.md index 28e620d43..e10803b95 100644 --- a/src/gadgets/README.md +++ b/src/gadgets/README.md @@ -116,11 +116,30 @@ You do not call `emitCliError` directly. The shared factory routes every failure --- +## Shared CLI helper layout + +`createCLICommand()` is intentionally the stable public facade for command files under `src/cli/**`. Its implementation delegates to focused helpers under `src/gadgets/shared/cli/`: + +| Helper | Responsibility | +|---|---| +| `commandNames.ts` | Kebab-case conversion and `cascade-tools ` derivation shared with manifest generation | +| `examples.ts` | Example lookup, shell quoting, oclif example rendering, and JSON expected-shape hints | +| `flags.ts` | oclif flag construction plus candidate and boolean-flag metadata collection | +| `booleanArgv.ts` | Natural boolean value forms such as `--flag true`, `--flag=false`, `yes/no`, and `1/0` | +| `parseErrors.ts` | oclif parse-error classification and unknown-flag suggestions | +| `params.ts` | File/stdin input, JSON parsing, direct parameter resolution, and git remote owner/repo resolution | +| `errorSink.ts` | Routing error envelopes through the active oclif command instance | + +These modules are shared infrastructure. A new gadget should still add or refine `ToolDefinition` metadata rather than branching inside a helper. + +--- + ## The single-entrypoint invariant Adding a gadget requires **zero edits** to: - `src/gadgets/shared/cliCommandFactory.ts` +- `src/gadgets/shared/cli/*.ts` - `src/gadgets/shared/manifestGenerator.ts` - `src/gadgets/shared/errorEnvelope.ts` - `src/backends/shared/nativeToolPrompts.ts` @@ -133,7 +152,7 @@ If you find yourself opening one of those files, stop — the right fix is almos The factory intercepts oclif's `NonExistentFlagsError`, runs a Levenshtein match against every declared canonical flag name + alias, and surfaces the closest canonical name as `error.hint`. No gadget work required — just declare your flags truthfully. -Two tuning constants live in the factory: `MAX_FLAG_SUGGESTION_DISTANCE` (default 2) and `MAX_FLAG_SUGGESTION_RATIO` (default 0.4). Wildly-off mistypes get no suggestion rather than a misleading one. +Two tuning constants live in `src/gadgets/shared/cli/parseErrors.ts`: `MAX_FLAG_SUGGESTION_DISTANCE` (default 2) and `MAX_FLAG_SUGGESTION_RATIO` (default 0.4). Wildly-off mistypes get no suggestion rather than a misleading one. --- diff --git a/src/gadgets/github/core/createPR.ts b/src/gadgets/github/core/createPR.ts index 9b81f5376..ae291bff0 100644 --- a/src/gadgets/github/core/createPR.ts +++ b/src/gadgets/github/core/createPR.ts @@ -33,6 +33,29 @@ export interface CreatePRResult { // see `[git-push] still running (Ns)` ticks during slow hooks. Setting // wallTimeoutMs + idleTimeoutMs to 0 disables them — see runCommand in utils/repo.ts. +/** + * Cap on captured hook output bytes that flow back into the agent's tool-result + * channel. Prod 2026-05-09 (run d8e31665, cascade/fe82YUKV): a successful PR + * creation returned 97 KB of `pushOutput` (lefthook's pre-push test:fast suite — + * 159 files, 2981 tests captured into the gadget result). Codex's tool-result + * parser couldn't extract the JSON envelope buried under that volume and + * retried the call; the resulting concurrent invocations raced against the same + * sidecar path leaving prUrl missing. Truncate here at the gadget result + * boundary; the FULL hook output already streams through `runCommand`'s + * heartbeat into the worker's engine log file (LLMIST_LOG_FILE) for operator + * visibility — only the agent-visible result-stream copy is capped. + */ +const HOOK_OUTPUT_MAX_BYTES = 4 * 1024; + +function truncateHookOutput(raw: string | undefined, label: 'commit' | 'push'): string | undefined { + if (!raw || raw.length <= HOOK_OUTPUT_MAX_BYTES) return raw; + const halfBudget = Math.floor(HOOK_OUTPUT_MAX_BYTES / 2); + const head = raw.slice(0, halfBudget); + const tail = raw.slice(-halfBudget); + const omitted = raw.length - head.length - tail.length; + return `${head}\n\n--- [${omitted} bytes truncated from ${label} hook output; full output in worker engine log] ---\n\n${tail}`; +} + async function detectOwnerRepo(): Promise<{ owner: string; repo: string }> { const result = await runCommand('git', ['remote', 'get-url', 'origin'], process.cwd()); if (result.exitCode !== 0) { @@ -85,8 +108,15 @@ async function stageAndCommit(commitMessage: string): Promise { ); if (commitResult.exitCode !== 0) { const output = [commitResult.stdout, commitResult.stderr].filter(Boolean).join('\n').trim(); + // Truncate before embedding in the error message — a failing pre-commit + // hook can emit the same volume of test output as a success (97 KB in the + // cascade prod incident). `createCLICommand` serialises err.message into + // the JSON error envelope, so unbounded output here hits the same + // Codex parser/retry bloat path as the success case. Full output stays + // in the worker engine log (LLMIST_LOG_FILE) for operator visibility. + const truncated = truncateHookOutput(output, 'commit') ?? output; throw new Error( - `COMMIT FAILED (pre-commit hooks may have failed)\n\n--- OUTPUT ---\n${output}`, + `COMMIT FAILED (pre-commit hooks may have failed)\n\n--- OUTPUT ---\n${truncated}`, ); } return [commitResult.stdout, commitResult.stderr].filter(Boolean).join('\n').trim(); @@ -106,8 +136,13 @@ async function pushBranch(branch: string): Promise { ); if (pushResult.exitCode !== 0) { const output = [pushResult.stdout, pushResult.stderr].filter(Boolean).join('\n').trim(); + // Truncate before embedding in the error message — same rationale as the + // commit failure path above: a failing pre-push hook can emit 97+ KB of + // test output, and err.message gets serialised into the JSON error + // envelope by createCLICommand. Full output stays in the engine log. + const truncated = truncateHookOutput(output, 'push') ?? output; throw new Error( - `PUSH FAILED for branch '${branch}' (pre-push hooks may have failed)\n\n--- OUTPUT ---\n${output}`, + `PUSH FAILED for branch '${branch}' (pre-push hooks may have failed)\n\n--- OUTPUT ---\n${truncated}`, ); } return [pushResult.stdout, pushResult.stderr].filter(Boolean).join('\n').trim(); @@ -143,6 +178,9 @@ export async function createPR(params: CreatePRParams): Promise const runLinkFooter = buildRunLinkFooterFromEnv(); const prBody = runLinkFooter ? params.body + runLinkFooter : params.body; + const truncatedPushOutput = truncateHookOutput(pushOutput, 'push'); + const truncatedCommitOutput = truncateHookOutput(commitOutput, 'commit'); + try { const pr = await githubClient.createPR(owner, repo, { title: params.title, @@ -157,8 +195,8 @@ export async function createPR(params: CreatePRParams): Promise prUrl: pr.htmlUrl, repoFullName: `${owner}/${repo}`, alreadyExisted: false, - pushOutput, - commitOutput, + pushOutput: truncatedPushOutput, + commitOutput: truncatedCommitOutput, }; } catch (error) { if ( @@ -174,8 +212,8 @@ export async function createPR(params: CreatePRParams): Promise prUrl: existingPR.htmlUrl, repoFullName: `${owner}/${repo}`, alreadyExisted: true, - pushOutput, - commitOutput, + pushOutput: truncatedPushOutput, + commitOutput: truncatedCommitOutput, }; } } diff --git a/src/gadgets/session/core/sidecar.ts b/src/gadgets/session/core/sidecar.ts index f1a1f5bfb..b6e86c85a 100644 --- a/src/gadgets/session/core/sidecar.ts +++ b/src/gadgets/session/core/sidecar.ts @@ -64,6 +64,18 @@ export function writeReviewSidecar( return false; } + // Spec 014/019 contract — same shape as writePRSidecar. Refuse to persist a + // half-baked sidecar so downstream gets either a complete record or a clear + // "no sidecar at all" signal, never a "missing required fields" mystery. + if (!reviewUrl || typeof reviewUrl !== 'string') { + logger.error('writeReviewSidecar: refusing to write — reviewUrl is missing or non-string', { + sidecarPath, + reviewUrl, + event, + }); + return false; + } + try { writeFileSync( sidecarPath, @@ -94,6 +106,32 @@ export function writePRSidecar( return false; } + // Spec 014/019 contract — sidecar is authoritative evidence of PR creation. + // Without prUrl + prNumber, the file is unusable downstream and the caller's + // run will fail with "no authoritative PR creation was recorded" — but with + // no signal of WHY the file was bad. Validate at the write boundary so the + // failure mode produces a precise log entry. Prod regression 2026-05-09 (run + // d8e31665) had `hasPrUrl: false` on the hydration side with no clue that + // the writer was the source. + if (!prUrl || typeof prUrl !== 'string') { + logger.error('writePRSidecar: refusing to write — prUrl is missing or non-string', { + sidecarPath, + prUrl, + prNumber, + alreadyExisted, + repoFullName, + }); + return false; + } + if (typeof prNumber !== 'number' || !Number.isFinite(prNumber)) { + logger.error('writePRSidecar: refusing to write — prNumber is missing or non-numeric', { + sidecarPath, + prUrl, + prNumber, + }); + return false; + } + try { writeFileSync( sidecarPath, diff --git a/src/gadgets/shared/cli/booleanArgv.ts b/src/gadgets/shared/cli/booleanArgv.ts new file mode 100644 index 000000000..f1a18e051 --- /dev/null +++ b/src/gadgets/shared/cli/booleanArgv.ts @@ -0,0 +1,93 @@ +import { emitCliError } from '../errorEnvelope.js'; +import type { ErrorSink } from './errorSink.js'; + +function normalizeBoolValue(raw: string): boolean | null { + const lc = raw.toLowerCase(); + if (lc === 'true' || lc === 'yes' || lc === '1') return true; + if (lc === 'false' || lc === 'no' || lc === '0') return false; + return null; +} + +/** + * Pre-process argv so boolean flags accept the natural value form. + */ +// biome-ignore lint/complexity/noExcessiveCognitiveComplexity: argv-shape taxonomy (--key=value, --key value, bare toggle) +export function massageBooleanFlagValues( + argv: readonly string[] | undefined, + booleanFlags: ReadonlyMap, + sink: ErrorSink, +): string[] | undefined { + if (argv === undefined) return undefined; + if (booleanFlags.size === 0) return [...argv]; + const result: string[] = []; + for (let i = 0; i < argv.length; i++) { + const tok = argv[i]; + + if (tok.startsWith('--') && tok.includes('=')) { + const eqIdx = tok.indexOf('='); + const name = tok.slice(2, eqIdx); + if (booleanFlags.has(name)) { + const allowNo = booleanFlags.get(name) ?? false; + const value = tok.slice(eqIdx + 1); + const normalized = normalizeBoolValue(value); + if (normalized === true) { + result.push(`--${name}`); + continue; + } + if (normalized === false) { + if (allowNo) result.push(`--no-${name}`); + continue; + } + emitCliError({ + type: 'flag-parse', + flag: name, + message: `Boolean flag --${name} got value '${value}'; accepts true|false|yes|no|1|0`, + got: value, + expected: 'true|false|yes|no|1|0', + hint: allowNo + ? `Use --${name} or --no-${name} for the canonical toggle form, or --${name}=true / --${name}=false.` + : `Use --${name} for true, or omit the flag for false.`, + stdout: sink.stdout, + stderr: sink.stderr, + exit: sink.exit, + }); + } + } + + if (tok.startsWith('--') && !tok.includes('=')) { + const name = tok.slice(2); + if (booleanFlags.has(name) && i + 1 < argv.length) { + const allowNo = booleanFlags.get(name) ?? false; + const next = argv[i + 1]; + const normalized = normalizeBoolValue(next); + if (normalized === true) { + result.push(`--${name}`); + i++; + continue; + } + if (normalized === false) { + if (allowNo) result.push(`--no-${name}`); + i++; + continue; + } + if (!next.startsWith('--')) { + emitCliError({ + type: 'flag-parse', + flag: name, + message: `Boolean flag --${name} got value '${next}'; accepts true|false|yes|no|1|0`, + got: next, + expected: 'true|false|yes|no|1|0', + hint: allowNo + ? `Use --${name} or --no-${name} for the canonical toggle form.` + : `Use --${name} for true, or omit the flag for false.`, + stdout: sink.stdout, + stderr: sink.stderr, + exit: sink.exit, + }); + } + } + } + result.push(tok); + } + return result; +} diff --git a/src/gadgets/shared/cli/commandNames.ts b/src/gadgets/shared/cli/commandNames.ts new file mode 100644 index 000000000..f6091bb26 --- /dev/null +++ b/src/gadgets/shared/cli/commandNames.ts @@ -0,0 +1,59 @@ +/** + * Convert a PascalCase or camelCase tool name to a kebab-case CLI command segment. + * + * Examples: + * - 'PostComment' -> 'post-comment' + * - 'ReadWorkItem' -> 'read-work-item' + * - 'CreatePR' -> 'create-pr' + */ +export function toKebabCase(name: string): string { + return name + .replace(/([A-Z]+)([A-Z][a-z])/g, '$1-$2') + .replace(/([a-z\d])([A-Z])/g, '$1-$2') + .toLowerCase(); +} + +/** + * Derive the CLI command prefix for a tool based on its category. + */ +export function deriveCLICommand(toolName: string, cliCommandOverride?: string): string { + if (cliCommandOverride) return cliCommandOverride; + + if (toolName === 'Finish') { + return `cascade-tools session ${toKebabCase(toolName)}`; + } + + const scmPrefixes = [ + 'createpr', + 'getpr', + 'postpr', + 'updatepr', + 'replytoreview', + 'createprreview', + 'getciru', + ]; + const lowerName = toolName.toLowerCase(); + if ( + scmPrefixes.some((p) => lowerName.startsWith(p)) || + lowerName.includes('pr') || + lowerName.includes('ci') + ) { + if ( + toolName.startsWith('CreatePR') || + toolName.startsWith('GetPR') || + toolName.startsWith('PostPR') || + toolName.startsWith('UpdatePR') || + toolName.startsWith('ReplyTo') || + toolName === 'GetCIRunLogs' + ) { + return `cascade-tools scm ${toKebabCase(toolName)}`; + } + } + + let commandName = toolName; + if (toolName.startsWith('PM') && toolName.length > 2 && /[A-Z]/.test(toolName[2])) { + commandName = toolName.slice(2); + } + + return `cascade-tools pm ${toKebabCase(commandName)}`; +} diff --git a/src/gadgets/shared/cli/errorSink.ts b/src/gadgets/shared/cli/errorSink.ts new file mode 100644 index 000000000..51cbcb798 --- /dev/null +++ b/src/gadgets/shared/cli/errorSink.ts @@ -0,0 +1,31 @@ +import type { CredentialScopedCommand } from '../../../cli/base.js'; + +/** + * Streams/exit delegate the factory hands to emitCliError so test spies on + * `instance.log`/`instance.exit` capture the envelope output. + */ +export interface ErrorSink { + stdout: NodeJS.WritableStream; + stderr: NodeJS.WritableStream; + exit: (code: number) => never; +} + +/** + * Build an error sink bound to a CredentialScopedCommand instance. + */ +export function buildSink(command: CredentialScopedCommand): ErrorSink { + const stdout: NodeJS.WritableStream = { + write: (chunk: string | Uint8Array): boolean => { + const text = typeof chunk === 'string' ? chunk : String(chunk); + if (typeof command.log === 'function') { + command.log(text.replace(/\n$/, '')); + } + return true; + }, + } as NodeJS.WritableStream; + const exit = + typeof command.exit === 'function' + ? (command.exit.bind(command) as (code: number) => never) + : (process.exit as (code: number) => never); + return { stdout, stderr: process.stderr, exit }; +} diff --git a/src/gadgets/shared/cli/examples.ts b/src/gadgets/shared/cli/examples.ts new file mode 100644 index 000000000..cb80b2ebe --- /dev/null +++ b/src/gadgets/shared/cli/examples.ts @@ -0,0 +1,67 @@ +import type { ParameterDefinition, ToolDefinition, ToolExample } from '../toolDefinition.js'; + +/** + * Pull the first concrete value for `paramName` out of the tool definition's examples block. + */ +export function findExampleForParam( + examples: readonly { params: Record }[] | undefined, + paramName: string, +): unknown { + if (!examples) return undefined; + for (const ex of examples) { + if (Object.hasOwn(ex.params, paramName) && ex.params[paramName] !== undefined) { + return ex.params[paramName]; + } + } + return undefined; +} + +/** + * Render the tool definition's `examples` block as oclif-flavored example strings. + * Each entry becomes a single shell-safe invocation line on `FactoryCommand.examples`. + */ +export function buildOclifExamples(def: ToolDefinition, cliCommand: string): string[] { + if (!def.examples || def.examples.length === 0) return []; + return def.examples.map((ex) => formatExampleLine(cliCommand, def, ex)); +} + +// biome-ignore lint/complexity/noExcessiveCognitiveComplexity: parameter-type taxonomy +function formatExampleLine(cliCommand: string, def: ToolDefinition, ex: ToolExample): string { + const parts: string[] = [cliCommand]; + for (const [key, value] of Object.entries(ex.params)) { + const paramDef = def.parameters[key]; + if (!paramDef || paramDef.gadgetOnly) continue; + if (value === undefined) continue; + + if (paramDef.type === 'boolean') { + if (value === true) parts.push(`--${key}`); + else parts.push(`--no-${key}`); + continue; + } + if (paramDef.type === 'array' && paramDef.items === 'string' && Array.isArray(value)) { + for (const v of value) parts.push(`--${key} ${shellQuote(String(v))}`); + continue; + } + if (paramDef.type === 'object' || (paramDef.type === 'array' && paramDef.items === 'object')) { + parts.push(`--${key} ${shellQuote(JSON.stringify(value))}`); + continue; + } + parts.push(`--${key} ${shellQuote(String(value))}`); + } + return parts.join(' '); +} + +export function shellQuote(s: string): string { + return `'${s.replace(/'/g, `'\\''`)}'`; +} + +/** + * Derive an `expected` shape hint for a JSON-parse failure envelope from the + * parameter's manifest example (primary source) or its `describe` text (fallback). + */ +export function expectedShapeFor(paramDef: ParameterDefinition, example?: unknown): string { + if (example !== undefined) { + return JSON.stringify(example); + } + return paramDef.describe; +} diff --git a/src/gadgets/shared/cli/flags.ts b/src/gadgets/shared/cli/flags.ts new file mode 100644 index 000000000..1656945cf --- /dev/null +++ b/src/gadgets/shared/cli/flags.ts @@ -0,0 +1,134 @@ +import { Flags } from '@oclif/core'; + +import type { ParameterDefinition, ToolDefinition } from '../toolDefinition.js'; +import type { AnyFlagsRecord } from './types.js'; + +/** + * Build a single oclif Flag from a ParameterDefinition. + * Returns undefined if the parameter is gadgetOnly (excluded from CLI). + */ +// biome-ignore lint/complexity/noExcessiveCognitiveComplexity: parameter-type taxonomy +export function buildOclifFlag( + def: ParameterDefinition, + isAutoResolved: boolean, + isFileInputParam: boolean, + // biome-ignore lint/suspicious/noExplicitAny: dynamic flag factory must accept heterogeneous oclif flag instances +): any { + if (def.gadgetOnly) return undefined; + + const isRequired = !isAutoResolved && !isFileInputParam && def.required === true && !def.optional; + + const baseOptions = { + description: def.describe, + required: isRequired, + ...(def.cliEnvVar ? { env: def.cliEnvVar } : {}), + ...(def.cliAliases && def.cliAliases.length > 0 ? { aliases: [...def.cliAliases] } : {}), + }; + + switch (def.type) { + case 'string': { + return Flags.string({ + ...baseOptions, + ...(def.default !== undefined ? { default: def.default } : {}), + }); + } + case 'number': { + return Flags.integer({ + ...baseOptions, + ...(def.default !== undefined ? { default: def.default } : {}), + }); + } + case 'boolean': { + return Flags.boolean({ + ...baseOptions, + ...(def.default !== undefined ? { default: def.default } : {}), + ...('allowNo' in def && def.allowNo ? { allowNo: true } : {}), + }); + } + case 'enum': { + return Flags.string({ + ...baseOptions, + options: [...def.options], + ...(def.default !== undefined ? { default: def.default } : {}), + }); + } + case 'array': { + if (def.items === 'object') { + return Flags.string({ ...baseOptions }); + } + return Flags.string({ ...baseOptions, multiple: true }); + } + case 'object': { + return Flags.string({ + ...baseOptions, + }); + } + default: { + const _exhaustive: never = def; + throw new Error(`Unknown parameter type: ${(_exhaustive as ParameterDefinition).type}`); + } + } +} + +/** + * Build the complete oclif flags record from a ToolDefinition. + * Includes file-input alternative flags and auto-resolved flags. + */ +export function buildFlagsRecord(def: ToolDefinition): AnyFlagsRecord { + const flags: AnyFlagsRecord = {}; + + const fileInputAlts = def.cli?.fileInputAlternatives ?? []; + const autoResolved = def.cli?.autoResolved ?? []; + + const fileInputParamNames = new Set(fileInputAlts.map((a) => a.paramName)); + const autoResolvedParamNames = new Set(autoResolved.map((a) => a.paramName)); + + for (const [name, paramDef] of Object.entries(def.parameters)) { + const isAutoResolved = autoResolvedParamNames.has(name); + const isFileInputParam = fileInputParamNames.has(name); + + const flag = buildOclifFlag(paramDef, isAutoResolved, isFileInputParam); + if (flag !== undefined) { + flags[name] = flag; + } + } + + for (const alt of fileInputAlts) { + flags[alt.fileFlag] = Flags.string({ + description: alt.description ?? `Read ${alt.paramName} from file (use - for stdin)`, + }); + } + + return flags; +} + +/** + * Collect canonical flag names + their declared aliases from a tool definition. + */ +export function collectCandidateFlags( + def: ToolDefinition, +): { canonical: string; aliases: readonly string[] }[] { + const list: { canonical: string; aliases: readonly string[] }[] = []; + for (const [name, paramDef] of Object.entries(def.parameters)) { + if (paramDef.gadgetOnly) continue; + list.push({ canonical: name, aliases: paramDef.cliAliases ?? [] }); + } + for (const alt of def.cli?.fileInputAlternatives ?? []) { + list.push({ canonical: alt.fileFlag, aliases: [] }); + } + return list; +} + +/** + * Collect boolean flag metadata for the argv preprocessor. + */ +export function collectBooleanFlagNames(def: ToolDefinition): Map { + const flags = new Map(); + for (const [name, paramDef] of Object.entries(def.parameters)) { + if (paramDef.gadgetOnly) continue; + if (paramDef.type === 'boolean') { + flags.set(name, paramDef.allowNo ?? false); + } + } + return flags; +} diff --git a/src/gadgets/shared/cli/params.ts b/src/gadgets/shared/cli/params.ts new file mode 100644 index 000000000..70d077826 --- /dev/null +++ b/src/gadgets/shared/cli/params.ts @@ -0,0 +1,204 @@ +import { readFileSync } from 'node:fs'; + +import { resolveOwnerRepo } from '../../../cli/base.js'; +import { emitCliError } from '../errorEnvelope.js'; +import type { + CLIAutoResolved, + FileInputAlternative, + ParameterDefinition, + ToolDefinition, +} from '../toolDefinition.js'; +import type { ErrorSink } from './errorSink.js'; +import { expectedShapeFor, findExampleForParam } from './examples.js'; +import type { ParsedFlags } from './types.js'; + +function readFileInput(fileFlagValue: string): string { + return fileFlagValue === '-' ? readFileSync(0, 'utf-8') : readFileSync(fileFlagValue, 'utf-8'); +} + +function parseJsonOrError( + raw: string, + flag: string, + paramDef: ParameterDefinition, + fileAlt: FileInputAlternative | undefined, + example: unknown, + sink: ErrorSink, +): unknown { + try { + return JSON.parse(raw); + } catch (err) { + const hint = fileAlt + ? `Use double-quoted JSON keys and values. For long payloads pass --${fileAlt.fileFlag} (or - for stdin).` + : 'Use double-quoted JSON keys and values.'; + return emitCliError({ + type: 'json-parse', + flag, + message: err instanceof Error ? err.message : String(err), + got: raw, + expected: expectedShapeFor(paramDef, example), + hint, + stdout: sink.stdout, + stderr: sink.stderr, + exit: sink.exit, + }); + } +} + +function resolveFileInputParam( + name: string, + paramDef: ParameterDefinition, + fileAlt: FileInputAlternative, + flags: ParsedFlags, + resolvedParams: Record, + example: unknown, + sink: ErrorSink, +): void { + const fileFlagValue = flags[fileAlt.fileFlag]; + const directValue = flags[name]; + + if (typeof fileFlagValue === 'string' && fileFlagValue.length > 0) { + const contents = readFileInput(fileFlagValue); + if (fileAlt.parseAs === 'json') { + resolvedParams[name] = parseJsonOrError(contents, name, paramDef, fileAlt, example, sink); + return; + } + resolvedParams[name] = contents; + return; + } + + if (directValue !== undefined && directValue !== null) { + if (paramDef.type === 'array' && paramDef.items === 'object') { + const asString = typeof directValue === 'string' ? directValue : JSON.stringify(directValue); + resolvedParams[name] = parseJsonOrError(asString, name, paramDef, fileAlt, example, sink); + return; + } + if (typeof directValue === 'string') { + resolvedParams[name] = directValue; + return; + } + } + + if (paramDef.required === true) { + emitCliError({ + type: 'missing-required', + flag: name, + message: `Either --${name} or --${fileAlt.fileFlag} is required`, + hint: `Pass --${name} '' or --${fileAlt.fileFlag} (use - for stdin).`, + stdout: sink.stdout, + stderr: sink.stderr, + exit: sink.exit, + }); + } +} + +function resolveObjectParam( + name: string, + flags: ParsedFlags, + resolvedParams: Record, + paramDef: ParameterDefinition, + example: unknown, + sink: ErrorSink, +): void { + const rawValue = flags[name]; + if (typeof rawValue !== 'string') { + return; + } + resolvedParams[name] = parseJsonOrError(rawValue, name, paramDef, undefined, example, sink); +} + +function resolveArrayOfObjectParam( + name: string, + flags: ParsedFlags, + resolvedParams: Record, + paramDef: ParameterDefinition, + fileAlt: FileInputAlternative | undefined, + example: unknown, + sink: ErrorSink, +): void { + const rawValue = flags[name]; + if (rawValue === undefined) return; + const asString = typeof rawValue === 'string' ? rawValue : JSON.stringify(rawValue); + resolvedParams[name] = parseJsonOrError(asString, name, paramDef, fileAlt, example, sink); +} + +function resolveStandardParam( + name: string, + flags: ParsedFlags, + resolvedParams: Record, +): void { + const value = flags[name]; + if (value !== undefined) { + resolvedParams[name] = value; + } +} + +export function resolveDirectParams( + def: ToolDefinition, + flags: ParsedFlags, + fileInputMap: Map, + autoResolvedMap: Map, + sink: ErrorSink, +): Record { + const resolvedParams: Record = {}; + + for (const [name, paramDef] of Object.entries(def.parameters)) { + if (paramDef.gadgetOnly) continue; + + const autoResolvedConfig = autoResolvedMap.get(name); + if (autoResolvedConfig?.resolvedFrom === 'git-remote') { + continue; + } + + const example = findExampleForParam(def.examples, name); + const fileAlt = fileInputMap.get(name); + if (fileAlt) { + resolveFileInputParam(name, paramDef, fileAlt, flags, resolvedParams, example, sink); + continue; + } + + if (paramDef.type === 'object') { + resolveObjectParam(name, flags, resolvedParams, paramDef, example, sink); + continue; + } + + if (paramDef.type === 'array' && paramDef.items === 'object') { + resolveArrayOfObjectParam(name, flags, resolvedParams, paramDef, undefined, example, sink); + continue; + } + + resolveStandardParam(name, flags, resolvedParams); + } + + return resolvedParams; +} + +export function resolveGitRemoteParams( + autoResolvedParams: CLIAutoResolved[], + flags: ParsedFlags, + resolvedParams: Record, +): void { + const gitRemoteParams = autoResolvedParams.filter((a) => a.resolvedFrom === 'git-remote'); + if (gitRemoteParams.length === 0) return; + + const ownerConfig = gitRemoteParams.find( + (a) => a.paramName === 'owner' || a.envVar?.includes('OWNER'), + ); + const repoConfig = gitRemoteParams.find( + (a) => a.paramName === 'repo' || a.envVar?.includes('NAME'), + ); + + if (!ownerConfig && !repoConfig) return; + + const ownerFlag = + ownerConfig && typeof flags[ownerConfig.paramName] === 'string' + ? (flags[ownerConfig.paramName] as string) + : undefined; + const repoFlag = + repoConfig && typeof flags[repoConfig.paramName] === 'string' + ? (flags[repoConfig.paramName] as string) + : undefined; + const { owner, repo } = resolveOwnerRepo(ownerFlag, repoFlag); + + if (ownerConfig) resolvedParams[ownerConfig.paramName] = owner; + if (repoConfig) resolvedParams[repoConfig.paramName] = repo; +} diff --git a/src/gadgets/shared/cli/parseErrors.ts b/src/gadgets/shared/cli/parseErrors.ts new file mode 100644 index 000000000..8754a9f92 --- /dev/null +++ b/src/gadgets/shared/cli/parseErrors.ts @@ -0,0 +1,100 @@ +import { distance } from 'fastest-levenshtein'; + +import type { EmitCliErrorOptions } from '../errorEnvelope.js'; + +const MAX_FLAG_SUGGESTION_DISTANCE = 2; +const MAX_FLAG_SUGGESTION_RATIO = 0.4; + +/** + * For the given unknown flag and the command's declared flag names + aliases, + * return the Levenshtein-closest canonical declared name if it passes the + * distance threshold; otherwise null. + */ +export function suggestFlag( + unknown: string, + candidates: { canonical: string; aliases: readonly string[] }[], +): string | null { + let best: { canonical: string; dist: number } | null = null; + for (const { canonical, aliases } of candidates) { + for (const candidate of [canonical, ...aliases]) { + const d = distance(unknown, candidate); + if (best === null || d < best.dist) { + best = { canonical, dist: d }; + } + } + } + if (best === null) return null; + const target = Math.max(unknown.length, best.canonical.length); + if (best.dist > MAX_FLAG_SUGGESTION_DISTANCE) return null; + if (target > 0 && best.dist / target > MAX_FLAG_SUGGESTION_RATIO) return null; + return best.canonical; +} + +/** + * Detect whether an error coming out of `this.parse()` is oclif's + * `NonExistentFlagsError`. + */ +export function isNonexistentFlagError(err: unknown): err is { flags: string[]; message: string } { + if (!err || typeof err !== 'object') return false; + const e = err as { name?: string; constructor?: { name?: string }; flags?: unknown }; + const ctorName = e.constructor?.name ?? ''; + const errName = e.name ?? ''; + const looksLikeCLIParse = + errName === 'CLIParseError' || + errName === 'NonExistentFlagsError' || + ctorName === 'NonExistentFlagsError'; + return looksLikeCLIParse && Array.isArray(e.flags); +} + +/** + * Classify oclif parse-time errors into the structured CLI envelope contract. + */ +export function classifyParseError( + err: unknown, +): Omit | null { + if (!err || typeof err !== 'object') return null; + const e = err as { name?: string; constructor?: { name?: string }; message?: string }; + const ctorName = e.constructor?.name ?? ''; + const message = typeof e.message === 'string' ? e.message : ''; + + if (ctorName === 'FailedFlagValidationError') { + const m = message.match(/Missing required flag\s+([\w-]+)/); + if (m) { + return { + type: 'missing-required', + flag: m[1], + message: `Missing required flag --${m[1]}`, + hint: `pass --${m[1]} (see --help for the full signature)`, + }; + } + } + + if (ctorName === 'FlagInvalidOptionError') { + const m = message.match(/Expected --([\w-]+)=(\S+) to be one of:\s+(.+?)(?:\n|$)/); + if (m) { + return { + type: 'enum-mismatch', + flag: m[1], + got: m[2], + expected: m[3].trim(), + message: `Flag --${m[1]} got '${m[2]}'; expected one of: ${m[3].trim()}`, + }; + } + } + + if (ctorName === 'UnexpectedArgsError') { + const m = message.match(/Unexpected argument:\s+(.+?)(?:\n|$)/); + if (m) { + return { + type: 'flag-parse', + got: m[1].trim(), + message, + }; + } + } + + if (ctorName.endsWith('Error') && /flag|argument|parse/i.test(message)) { + return { type: 'flag-parse', message }; + } + return null; +} diff --git a/src/gadgets/shared/cli/types.ts b/src/gadgets/shared/cli/types.ts new file mode 100644 index 000000000..2feaf1e10 --- /dev/null +++ b/src/gadgets/shared/cli/types.ts @@ -0,0 +1,4 @@ +// biome-ignore lint/suspicious/noExplicitAny: oclif flag generics do not compose safely for dynamic factories +export type AnyFlagsRecord = Record; + +export type ParsedFlags = Record; diff --git a/src/gadgets/shared/cliCommandFactory.ts b/src/gadgets/shared/cliCommandFactory.ts index 90a626f6f..548ae3727 100644 --- a/src/gadgets/shared/cliCommandFactory.ts +++ b/src/gadgets/shared/cliCommandFactory.ts @@ -9,769 +9,17 @@ * - An execute() method wired to the coreFn */ -import { readFileSync } from 'node:fs'; - -import { Flags } from '@oclif/core'; -import { distance } from 'fastest-levenshtein'; - -import { CredentialScopedCommand, resolveOwnerRepo } from '../../cli/base.js'; -import { type EmitCliErrorOptions, emitCliError } from './errorEnvelope.js'; -import type { - CLIAutoResolved, - FileInputAlternative, - ParameterDefinition, - ToolDefinition, - ToolExample, -} from './toolDefinition.js'; - -// biome-ignore lint/suspicious/noExplicitAny: oclif flag generics do not compose safely for dynamic factories -type AnyFlagsRecord = Record; -type ParsedFlags = Record; - -// --------------------------------------------------------------------------- -// Helpers -// --------------------------------------------------------------------------- - -/** - * Build a single oclif Flag from a ParameterDefinition. - * Returns undefined if the parameter is gadgetOnly (excluded from CLI). - * - * Branches on parameter type taxonomy (string / number / boolean / enum / - * array-with-object-items / array-with-string-items / object) — the complexity - * reflects the domain shape, not tangled logic. - */ -// biome-ignore lint/complexity/noExcessiveCognitiveComplexity: parameter-type taxonomy -function buildOclifFlag( - def: ParameterDefinition, - isAutoResolved: boolean, - isFileInputParam: boolean, - // biome-ignore lint/suspicious/noExplicitAny: dynamic flag factory must accept heterogeneous oclif flag instances -): any { - // gadgetOnly params (like `comment`) are excluded from CLI flags - if (def.gadgetOnly) return undefined; - - // File-input params that have a file alternative are optional in the CLI - // (since the value can come from the file flag instead) - const isRequired = !isAutoResolved && !isFileInputParam && def.required === true && !def.optional; - - const baseOptions = { - description: def.describe, - required: isRequired, - ...(def.cliEnvVar ? { env: def.cliEnvVar } : {}), - ...(def.cliAliases && def.cliAliases.length > 0 ? { aliases: [...def.cliAliases] } : {}), - }; - - switch (def.type) { - case 'string': { - return Flags.string({ - ...baseOptions, - ...(def.default !== undefined ? { default: def.default } : {}), - }); - } - case 'number': { - return Flags.integer({ - ...baseOptions, - ...(def.default !== undefined ? { default: def.default } : {}), - }); - } - case 'boolean': { - return Flags.boolean({ - ...baseOptions, - ...(def.default !== undefined ? { default: def.default } : {}), - ...('allowNo' in def && def.allowNo ? { allowNo: true } : {}), - }); - } - case 'enum': { - return Flags.string({ - ...baseOptions, - options: [...def.options], - ...(def.default !== undefined ? { default: def.default } : {}), - }); - } - case 'array': { - // Primitive arrays (items:'string') stay repeatable (--x a --x b). - // Object arrays (items:'object') take a single JSON-array string - // that the factory parses below. - if (def.items === 'object') { - return Flags.string({ ...baseOptions }); - } - return Flags.string({ ...baseOptions, multiple: true }); - } - case 'object': { - return Flags.string({ - ...baseOptions, - // Object params are passed as JSON string - }); - } - default: { - const _exhaustive: never = def; - throw new Error(`Unknown parameter type: ${(_exhaustive as ParameterDefinition).type}`); - } - } -} - -/** - * Build the complete oclif flags record from a ToolDefinition. - * Includes file-input alternative flags and auto-resolved flags. - */ -function buildFlagsRecord(def: ToolDefinition): AnyFlagsRecord { - const flags: AnyFlagsRecord = {}; - - const fileInputAlts = def.cli?.fileInputAlternatives ?? []; - const autoResolved = def.cli?.autoResolved ?? []; - - const fileInputParamNames = new Set(fileInputAlts.map((a) => a.paramName)); - const autoResolvedParamNames = new Set(autoResolved.map((a) => a.paramName)); - - // Generate flags for each parameter - for (const [name, paramDef] of Object.entries(def.parameters)) { - const isAutoResolved = autoResolvedParamNames.has(name); - const isFileInputParam = fileInputParamNames.has(name); - - const flag = buildOclifFlag(paramDef, isAutoResolved, isFileInputParam); - if (flag !== undefined) { - flags[name] = flag; - } - } - - // Add file-input alternative flags - for (const alt of fileInputAlts) { - flags[alt.fileFlag] = Flags.string({ - description: alt.description ?? `Read ${alt.paramName} from file (use - for stdin)`, - }); - } - - return flags; -} - -function readFileInput(fileFlagValue: string): string { - return fileFlagValue === '-' ? readFileSync(0, 'utf-8') : readFileSync(fileFlagValue, 'utf-8'); -} - -/** - * Derive an `expected` shape hint for a JSON-parse failure envelope from the - * parameter's manifest example (primary source) or its `describe` text (fallback). - */ -function expectedShapeFor(paramDef: ParameterDefinition, example?: unknown): string { - if (example !== undefined) { - return JSON.stringify(example); - } - // No example to lean on — give the agent the describe text so at least - // the shape hint is non-empty. - return paramDef.describe; -} - -/** - * JSON-parse a string and emit a structured envelope on failure. - * Returns the parsed value on success or never-returns on failure (emitCliError exits). - */ -/** - * Streams/exit delegate the factory hands to {@link emitCliError} so test spies - * on `instance.log`/`instance.exit` capture the envelope output instead of - * going directly to process.stdout / process.exit. - */ -interface ErrorSink { - stdout: NodeJS.WritableStream; - stderr: NodeJS.WritableStream; - exit: (code: number) => never; -} - -/** - * Maximum Levenshtein distance between an unknown flag and a declared name - * before we stop suggesting. Additionally bounded by `MAX_DISTANCE_RATIO` - * so that very short flags don't pick up wildly different suggestions. - */ -const MAX_FLAG_SUGGESTION_DISTANCE = 2; -const MAX_FLAG_SUGGESTION_RATIO = 0.4; - -/** - * For the given unknown flag and the command's declared flag names + aliases, - * return the Levenshtein-closest canonical declared name if it passes the - * distance threshold; otherwise null. Aliases are considered during the match - * but the returned value is always the canonical flag name. - */ -function suggestFlag( - unknown: string, - candidates: { canonical: string; aliases: readonly string[] }[], -): string | null { - let best: { canonical: string; dist: number } | null = null; - for (const { canonical, aliases } of candidates) { - for (const candidate of [canonical, ...aliases]) { - const d = distance(unknown, candidate); - if (best === null || d < best.dist) { - best = { canonical, dist: d }; - } - } - } - if (best === null) return null; - const target = Math.max(unknown.length, best.canonical.length); - if (best.dist > MAX_FLAG_SUGGESTION_DISTANCE) return null; - if (target > 0 && best.dist / target > MAX_FLAG_SUGGESTION_RATIO) return null; - return best.canonical; -} - -/** - * Collect canonical flag names + their declared aliases from a tool definition. - * Also includes any file-input alternative flags (which have no canonical counterpart; - * they stand on their own). Used by fuzzy-suggestion and by help rendering. - */ -function collectCandidateFlags( - def: ToolDefinition, -): { canonical: string; aliases: readonly string[] }[] { - const list: { canonical: string; aliases: readonly string[] }[] = []; - for (const [name, paramDef] of Object.entries(def.parameters)) { - if (paramDef.gadgetOnly) continue; - list.push({ canonical: name, aliases: paramDef.cliAliases ?? [] }); - } - for (const alt of def.cli?.fileInputAlternatives ?? []) { - list.push({ canonical: alt.fileFlag, aliases: [] }); - } - return list; -} - -/** - * Detect whether an error coming out of `this.parse()` is oclif's - * `NonExistentFlagsError`. We match by constructor name + the `flags` array - * shape to stay robust across oclif versions and avoid a deep import. - */ -function isNonexistentFlagError(err: unknown): err is { flags: string[]; message: string } { - if (!err || typeof err !== 'object') return false; - const e = err as { name?: string; constructor?: { name?: string }; flags?: unknown }; - const ctorName = e.constructor?.name ?? ''; - const errName = e.name ?? ''; - const looksLikeCLIParse = - errName === 'CLIParseError' || - errName === 'NonExistentFlagsError' || - ctorName === 'NonExistentFlagsError'; - return looksLikeCLIParse && Array.isArray(e.flags); -} - -/** - * Spec 014, prod regression 2026-05-09: oclif's parse-time errors (missing - * required, enum mismatch, unexpected positional from a boolean-value miss) - * historically threw past the existing unknown-flag catch with `exit code 2` - * and empty stdout — bypassing the structured envelope contract. Classify the - * error here so every parse failure reaches the agent through the same shape. - * - * Returns a ready-to-emit envelope (omitting only the sink fields). Returns - * `null` when the error doesn't match any oclif parse-time shape — caller - * re-throws so unexpected exceptions still surface. - */ -function classifyParseError( - err: unknown, -): Omit | null { - if (!err || typeof err !== 'object') return null; - const e = err as { name?: string; constructor?: { name?: string }; message?: string }; - const ctorName = e.constructor?.name ?? ''; - const message = typeof e.message === 'string' ? e.message : ''; - - // FailedFlagValidationError → "Missing required flag " - if (ctorName === 'FailedFlagValidationError') { - const m = message.match(/Missing required flag\s+([\w-]+)/); - if (m) { - return { - type: 'missing-required', - flag: m[1], - message: `Missing required flag --${m[1]}`, - hint: `pass --${m[1]} (see --help for the full signature)`, - }; - } - } - - // FlagInvalidOptionError → "Expected --= to be one of: " - if (ctorName === 'FlagInvalidOptionError') { - const m = message.match(/Expected --([\w-]+)=(\S+) to be one of:\s+(.+?)(?:\n|$)/); - if (m) { - return { - type: 'enum-mismatch', - flag: m[1], - got: m[2], - expected: m[3].trim(), - message: `Flag --${m[1]} got '${m[2]}'; expected one of: ${m[3].trim()}`, - }; - } - } - - // UnexpectedArgsError → fallback for boolean-value-form misses that escape - // the preprocessor (e.g. boolean toggle followed by a non-flag token we - // chose not to consume because it didn't look bool-shaped). - if (ctorName === 'UnexpectedArgsError') { - const m = message.match(/Unexpected argument:\s+(.+?)(?:\n|$)/); - if (m) { - return { - type: 'flag-parse', - got: m[1].trim(), - message, - }; - } - } - - // Generic CLIParseError fallback (rare). - if (ctorName.endsWith('Error') && /flag|argument|parse/i.test(message)) { - return { type: 'flag-parse', message }; - } - return null; -} - -/** - * Recognised string forms accepted as a value for boolean flags. Codex agents - * reach for `--includeComments true` (the dominant 2026-05-09 prod failure) - * even when the synopsis says `--[no-]includeComments`; widen the parser so - * both shapes work, then keep oclif's strict toggle semantics for everything - * else. Returns `true` / `false` for recognised values, `null` otherwise so - * the caller can treat the original token as a non-bool value. - */ -function normalizeBoolValue(raw: string): boolean | null { - const lc = raw.toLowerCase(); - if (lc === 'true' || lc === 'yes' || lc === '1') return true; - if (lc === 'false' || lc === 'no' || lc === '0') return false; - return null; -} - -/** - * Pre-process argv so boolean flags accept the natural value form. Each - * `--key true|false|...` (space- or equals-separated) is rewritten to oclif's - * canonical toggle (`--key` or `--no-key`); malformed values surface as a - * structured `flag-parse` envelope before oclif sees the argv. - * - * The preprocessor never consumes a token that LOOKS like another flag - * (starts with `--`) — that token belongs to a different flag, not to the - * preceding boolean. Bare-toggle invocations stay untouched. - */ -// biome-ignore lint/complexity/noExcessiveCognitiveComplexity: argv-shape taxonomy (--key=value, --key value, bare toggle) -function massageBooleanFlagValues( - argv: readonly string[] | undefined, - booleanFlags: ReadonlyMap, - sink: ErrorSink, -): string[] | undefined { - // Pass through `undefined` so oclif's `parse(Cmd)` (no argv arg) keeps - // working — some tests construct commands without seeded argv. - if (argv === undefined) return undefined; - if (booleanFlags.size === 0) return [...argv]; - const result: string[] = []; - for (let i = 0; i < argv.length; i++) { - const tok = argv[i]; - - // --flag=value form - if (tok.startsWith('--') && tok.includes('=')) { - const eqIdx = tok.indexOf('='); - const name = tok.slice(2, eqIdx); - if (booleanFlags.has(name)) { - const allowNo = booleanFlags.get(name) ?? false; - const value = tok.slice(eqIdx + 1); - const normalized = normalizeBoolValue(value); - if (normalized === true) { - result.push(`--${name}`); - continue; - } - if (normalized === false) { - // Only emit --no- when the flag supports negation. For - // non-negatable booleans (e.g. `draft`), omitting the flag - // produces the same `false` result without the unknown-flag error. - if (allowNo) result.push(`--no-${name}`); - continue; - } - emitCliError({ - type: 'flag-parse', - flag: name, - message: `Boolean flag --${name} got value '${value}'; accepts true|false|yes|no|1|0`, - got: value, - expected: 'true|false|yes|no|1|0', - hint: allowNo - ? `Use --${name} or --no-${name} for the canonical toggle form, or --${name}=true / --${name}=false.` - : `Use --${name} for true, or omit the flag for false.`, - stdout: sink.stdout, - stderr: sink.stderr, - exit: sink.exit, - }); - } - } - - // --flag form - if (tok.startsWith('--') && !tok.includes('=')) { - const name = tok.slice(2); - if (booleanFlags.has(name) && i + 1 < argv.length) { - const allowNo = booleanFlags.get(name) ?? false; - const next = argv[i + 1]; - const normalized = normalizeBoolValue(next); - if (normalized === true) { - result.push(`--${name}`); - i++; - continue; - } - if (normalized === false) { - // Only emit --no- when the flag supports negation. For - // non-negatable booleans, just consume the token — absence = false. - if (allowNo) result.push(`--no-${name}`); - i++; - continue; - } - // Next token is something else. If it doesn't start with `--`, the - // agent meant it as a value to this boolean — surface a precise - // envelope here so we don't bottom out as `Unexpected argument`. - if (!next.startsWith('--')) { - emitCliError({ - type: 'flag-parse', - flag: name, - message: `Boolean flag --${name} got value '${next}'; accepts true|false|yes|no|1|0`, - got: next, - expected: 'true|false|yes|no|1|0', - hint: allowNo - ? `Use --${name} or --no-${name} for the canonical toggle form.` - : `Use --${name} for true, or omit the flag for false.`, - stdout: sink.stdout, - stderr: sink.stderr, - exit: sink.exit, - }); - } - // next is another flag — leave the bare toggle as-is. - } - } - result.push(tok); - } - return result; -} - -/** - * Collect boolean flag metadata for the argv preprocessor. - * - * Returns a Map from flag name to whether it supports `--no-` negation - * (`allowNo`). The preprocessor uses this to decide: - * - `true` value → always rewrite to `--` - * - `false` value → `--no-` only when allowNo is set; otherwise drop the - * token (absence = false for non-negatable booleans, so this - * produces the correct oclif parse result without emitting an - * unknown flag). Fixes `--draft false` on `scm create-pr`. - */ -function collectBooleanFlagNames(def: ToolDefinition): Map { - const flags = new Map(); - for (const [name, paramDef] of Object.entries(def.parameters)) { - if (paramDef.gadgetOnly) continue; - if (paramDef.type === 'boolean') { - flags.set(name, paramDef.allowNo ?? false); - } - } - return flags; -} - -/** - * Build an error sink bound to a CredentialScopedCommand instance, so that - * emitCliError routes envelope output through `instance.log` (stripping the - * trailing newline oclif's `this.log` adds itself) and `instance.exit`. This - * keeps test spies honest. - * - * Note on stderr: Command instances don't expose a `stderr` method publicly, - * but tests don't assert on stderr from the factory surface (they assert on - * the prose summary via the errorEnvelope unit tests). Here we write directly - * to `process.stderr`. - */ -function buildSink(command: CredentialScopedCommand): ErrorSink { - const stdout: NodeJS.WritableStream = { - write: (chunk: string | Uint8Array): boolean => { - const text = typeof chunk === 'string' ? chunk : String(chunk); - // Some tests construct commands without a log spy installed; fall back - // to a no-op so envelope emission can't crash the test setup. - if (typeof command.log === 'function') { - command.log(text.replace(/\n$/, '')); - } - return true; - }, - } as NodeJS.WritableStream; - const exit = - typeof command.exit === 'function' - ? (command.exit.bind(command) as (code: number) => never) - : (process.exit as (code: number) => never); - return { stdout, stderr: process.stderr, exit }; -} - -function parseJsonOrError( - raw: string, - flag: string, - paramDef: ParameterDefinition, - fileAlt: FileInputAlternative | undefined, - example: unknown, - sink: ErrorSink, -): unknown { - try { - return JSON.parse(raw); - } catch (err) { - const hint = fileAlt - ? `Use double-quoted JSON keys and values. For long payloads pass --${fileAlt.fileFlag} (or - for stdin).` - : 'Use double-quoted JSON keys and values.'; - return emitCliError({ - type: 'json-parse', - flag, - message: err instanceof Error ? err.message : String(err), - got: raw, - expected: expectedShapeFor(paramDef, example), - hint, - stdout: sink.stdout, - stderr: sink.stderr, - exit: sink.exit, - }); - } -} - -function resolveFileInputParam( - name: string, - paramDef: ParameterDefinition, - fileAlt: FileInputAlternative, - flags: ParsedFlags, - resolvedParams: Record, - example: unknown, - sink: ErrorSink, -): void { - const fileFlagValue = flags[fileAlt.fileFlag]; - const directValue = flags[name]; - - if (typeof fileFlagValue === 'string' && fileFlagValue.length > 0) { - const contents = readFileInput(fileFlagValue); - if (fileAlt.parseAs === 'json') { - resolvedParams[name] = parseJsonOrError(contents, name, paramDef, fileAlt, example, sink); - return; - } - resolvedParams[name] = contents; - return; - } - - // Direct (non-file) value: for array-of-object we still need to JSON-parse; - // for primitive string params we pass through. - if (directValue !== undefined && directValue !== null) { - if (paramDef.type === 'array' && paramDef.items === 'object') { - const asString = typeof directValue === 'string' ? directValue : JSON.stringify(directValue); - resolvedParams[name] = parseJsonOrError(asString, name, paramDef, fileAlt, example, sink); - return; - } - if (typeof directValue === 'string') { - resolvedParams[name] = directValue; - return; - } - } - - if (paramDef.required === true) { - emitCliError({ - type: 'missing-required', - flag: name, - message: `Either --${name} or --${fileAlt.fileFlag} is required`, - hint: `Pass --${name} '' or --${fileAlt.fileFlag} (use - for stdin).`, - stdout: sink.stdout, - stderr: sink.stderr, - exit: sink.exit, - }); - } -} - -function resolveObjectParam( - name: string, - flags: ParsedFlags, - resolvedParams: Record, - paramDef: ParameterDefinition, - example: unknown, - sink: ErrorSink, -): void { - const rawValue = flags[name]; - if (typeof rawValue !== 'string') { - return; - } - resolvedParams[name] = parseJsonOrError(rawValue, name, paramDef, undefined, example, sink); -} - -/** - * Resolve a `type:'array', items:'object'` flag value: JSON-parse the single - * string form. oclif gives us a bare string because we set `multiple:false` - * for items:'object' in buildOclifFlag. - */ -function resolveArrayOfObjectParam( - name: string, - flags: ParsedFlags, - resolvedParams: Record, - paramDef: ParameterDefinition, - fileAlt: FileInputAlternative | undefined, - example: unknown, - sink: ErrorSink, -): void { - const rawValue = flags[name]; - if (rawValue === undefined) return; - const asString = typeof rawValue === 'string' ? rawValue : JSON.stringify(rawValue); - resolvedParams[name] = parseJsonOrError(asString, name, paramDef, fileAlt, example, sink); -} - -function resolveStandardParam( - name: string, - flags: ParsedFlags, - resolvedParams: Record, -): void { - const value = flags[name]; - if (value !== undefined) { - resolvedParams[name] = value; - } -} - -function resolveDirectParams( - def: ToolDefinition, - flags: ParsedFlags, - fileInputMap: Map, - autoResolvedMap: Map, - sink: ErrorSink, -): Record { - const resolvedParams: Record = {}; - - for (const [name, paramDef] of Object.entries(def.parameters)) { - if (paramDef.gadgetOnly) continue; - - const autoResolvedConfig = autoResolvedMap.get(name); - if (autoResolvedConfig?.resolvedFrom === 'git-remote') { - continue; - } - - // Derive one concrete example value per parameter from def.examples, used - // by JSON-parse error messages to show the agent the expected shape. - const example = findExampleForParam(def.examples, name); - const fileAlt = fileInputMap.get(name); - if (fileAlt) { - resolveFileInputParam(name, paramDef, fileAlt, flags, resolvedParams, example, sink); - continue; - } - - if (paramDef.type === 'object') { - resolveObjectParam(name, flags, resolvedParams, paramDef, example, sink); - continue; - } - - if (paramDef.type === 'array' && paramDef.items === 'object') { - resolveArrayOfObjectParam(name, flags, resolvedParams, paramDef, undefined, example, sink); - continue; - } - - resolveStandardParam(name, flags, resolvedParams); - } - - return resolvedParams; -} - -/** - * Pull the first concrete value for `paramName` out of the tool definition's - * examples block. Mirrors the manifest generator's `findExampleForParam` but - * kept local to avoid a cross-import cycle with the manifest module. - */ -function findExampleForParam( - examples: readonly ToolExample[] | undefined, - paramName: string, -): unknown { - if (!examples) return undefined; - for (const ex of examples) { - if (Object.hasOwn(ex.params, paramName) && ex.params[paramName] !== undefined) { - return ex.params[paramName]; - } - } - return undefined; -} - -/** - * Render the tool definition's `examples` block as oclif-flavored example strings. - * Each entry becomes a single shell-safe invocation line on `FactoryCommand.examples`. - */ -function buildOclifExamples(def: ToolDefinition, cliCommand: string): string[] { - if (!def.examples || def.examples.length === 0) return []; - return def.examples.map((ex) => formatExampleLine(cliCommand, def, ex)); -} - -// biome-ignore lint/complexity/noExcessiveCognitiveComplexity: parameter-type taxonomy -function formatExampleLine(cliCommand: string, def: ToolDefinition, ex: ToolExample): string { - const parts: string[] = [cliCommand]; - for (const [key, value] of Object.entries(ex.params)) { - const paramDef = def.parameters[key]; - if (!paramDef || paramDef.gadgetOnly) continue; - if (value === undefined) continue; - - if (paramDef.type === 'boolean') { - if (value === true) parts.push(`--${key}`); - else parts.push(`--no-${key}`); - continue; - } - if (paramDef.type === 'array' && paramDef.items === 'string' && Array.isArray(value)) { - for (const v of value) parts.push(`--${key} ${shellQuote(String(v))}`); - continue; - } - if (paramDef.type === 'object' || (paramDef.type === 'array' && paramDef.items === 'object')) { - parts.push(`--${key} ${shellQuote(JSON.stringify(value))}`); - continue; - } - parts.push(`--${key} ${shellQuote(String(value))}`); - } - return parts.join(' '); -} - -function shellQuote(s: string): string { - // Wrap with single quotes; escape any embedded single quote with the - // classic '\'' trick so copy/paste into a POSIX shell remains safe. - return `'${s.replace(/'/g, `'\\''`)}'`; -} - -/** - * Derive the `cascade-tools ` prefix for a tool, matching - * {@link ../shared/manifestGenerator.ts#deriveCLICommand}. Duplicated locally to - * avoid a cross-import cycle. - */ -function deriveCommandPrefix(toolName: string): string { - // Minimal duplicate of deriveCLICommand — enough to seed oclif examples. - if (toolName === 'Finish') { - return `cascade-tools session ${kebab(toolName)}`; - } - if ( - toolName.startsWith('CreatePR') || - toolName.startsWith('GetPR') || - toolName.startsWith('PostPR') || - toolName.startsWith('UpdatePR') || - toolName.startsWith('ReplyTo') || - toolName === 'GetCIRunLogs' - ) { - return `cascade-tools scm ${kebab(toolName)}`; - } - let commandName = toolName; - if (toolName.startsWith('PM') && toolName.length > 2 && /[A-Z]/.test(toolName[2])) { - commandName = toolName.slice(2); - } - return `cascade-tools pm ${kebab(commandName)}`; -} - -function kebab(name: string): string { - return name - .replace(/([A-Z]+)([A-Z][a-z])/g, '$1-$2') - .replace(/([a-z\d])([A-Z])/g, '$1-$2') - .toLowerCase(); -} - -function resolveGitRemoteParams( - autoResolvedParams: CLIAutoResolved[], - flags: ParsedFlags, - resolvedParams: Record, -): void { - const gitRemoteParams = autoResolvedParams.filter((a) => a.resolvedFrom === 'git-remote'); - if (gitRemoteParams.length === 0) return; - - const ownerConfig = gitRemoteParams.find( - (a) => a.paramName === 'owner' || a.envVar?.includes('OWNER'), - ); - const repoConfig = gitRemoteParams.find( - (a) => a.paramName === 'repo' || a.envVar?.includes('NAME'), - ); - - if (!ownerConfig && !repoConfig) return; - - const ownerFlag = - ownerConfig && typeof flags[ownerConfig.paramName] === 'string' - ? (flags[ownerConfig.paramName] as string) - : undefined; - const repoFlag = - repoConfig && typeof flags[repoConfig.paramName] === 'string' - ? (flags[repoConfig.paramName] as string) - : undefined; - const { owner, repo } = resolveOwnerRepo(ownerFlag, repoFlag); - - if (ownerConfig) resolvedParams[ownerConfig.paramName] = owner; - if (repoConfig) resolvedParams[repoConfig.paramName] = repo; -} +import { CredentialScopedCommand } from '../../cli/base.js'; +import { massageBooleanFlagValues } from './cli/booleanArgv.js'; +import { deriveCLICommand } from './cli/commandNames.js'; +import { buildSink } from './cli/errorSink.js'; +import { buildOclifExamples } from './cli/examples.js'; +import { buildFlagsRecord, collectBooleanFlagNames, collectCandidateFlags } from './cli/flags.js'; +import { resolveDirectParams, resolveGitRemoteParams } from './cli/params.js'; +import { classifyParseError, isNonexistentFlagError, suggestFlag } from './cli/parseErrors.js'; +import type { ParsedFlags } from './cli/types.js'; +import { emitCliError } from './errorEnvelope.js'; +import type { CLIAutoResolved, FileInputAlternative, ToolDefinition } from './toolDefinition.js'; // --------------------------------------------------------------------------- // Factory function @@ -827,7 +75,7 @@ export function createCLICommand( fileInputAlts.map((a) => [a.paramName, a]), ); - const commandPrefix = deriveCommandPrefix(def.name); + const commandPrefix = deriveCLICommand(def.name); const staticExamples = buildOclifExamples(def, commandPrefix); const booleanFlagNames = collectBooleanFlagNames(def); diff --git a/src/gadgets/shared/manifestGenerator.ts b/src/gadgets/shared/manifestGenerator.ts index 40658d438..1e2cd8faf 100644 --- a/src/gadgets/shared/manifestGenerator.ts +++ b/src/gadgets/shared/manifestGenerator.ts @@ -13,6 +13,8 @@ */ import type { ToolManifest, ToolManifestParameter } from '../../agents/contracts/index.js'; +import { deriveCLICommand } from './cli/commandNames.js'; +import { findExampleForParam } from './cli/examples.js'; import type { ParameterDefinition, ToolDefinition } from './toolDefinition.js'; // --------------------------------------------------------------------------- @@ -61,98 +63,6 @@ function buildManifestParam( return entry; } -/** - * Find the first example whose `params` object has a defined value for `paramName`, - * and return that value. Used by the manifest generator to surface one concrete - * shape per parameter to agents + help text. - */ -function findExampleForParam( - examples: readonly { params: Record }[] | undefined, - paramName: string, -): unknown { - if (!examples) return undefined; - for (const ex of examples) { - if (Object.hasOwn(ex.params, paramName) && ex.params[paramName] !== undefined) { - return ex.params[paramName]; - } - } - return undefined; -} - -/** - * Convert a PascalCase or camelCase tool name to a kebab-case CLI command segment. - * - * Examples: - * - 'PostComment' → 'post-comment' - * - 'ReadWorkItem' → 'read-work-item' - * - 'CreatePR' → 'create-pr' - */ -function toKebabCase(name: string): string { - return name - .replace(/([A-Z]+)([A-Z][a-z])/g, '$1-$2') - .replace(/([a-z\d])([A-Z])/g, '$1-$2') - .toLowerCase(); -} - -/** - * Derive the CLI command prefix for a tool based on its category. - * - * The tool name prefix determines whether it's a PM, SCM, or session tool: - * - PM tools: ReadWorkItem, PostComment, UpdateWorkItem, CreateWorkItem, ListWorkItems, - * AddChecklist, MoveWorkItem, PMUpdateChecklistItem, PMDeleteChecklistItem - * - SCM tools: CreatePR, GetPR*, PostPRComment, UpdatePRComment, ReplyToReviewComment, - * CreatePRReview, GetCIRunLogs - * - Session tools: Finish - * - * Falls back to 'cascade-tools pm' if the category cannot be determined. - */ -function deriveCLICommand(toolName: string, cliCommandOverride?: string): string { - if (cliCommandOverride) return cliCommandOverride; - - // Session tools - if (toolName === 'Finish') { - return `cascade-tools session ${toKebabCase(toolName)}`; - } - - // SCM tools: PR-related, CI-related - const scmPrefixes = [ - 'createpr', - 'getpr', - 'postpr', - 'updatepr', - 'replytoreview', - 'createprreview', - 'getciru', - ]; - const lowerName = toolName.toLowerCase(); - if ( - scmPrefixes.some((p) => lowerName.startsWith(p)) || - lowerName.includes('pr') || - lowerName.includes('ci') - ) { - // Verify it is truly an SCM tool - if ( - toolName.startsWith('CreatePR') || - toolName.startsWith('GetPR') || - toolName.startsWith('PostPR') || - toolName.startsWith('UpdatePR') || - toolName.startsWith('ReplyTo') || - toolName === 'GetCIRunLogs' - ) { - return `cascade-tools scm ${toKebabCase(toolName)}`; - } - } - - // PM tools: Strip "PM" prefix if present (e.g., PMUpdateChecklistItem → update-checklist-item) - // to avoid double "pm" prefix (cascade-tools pm pm-update-checklist-item) - let commandName = toolName; - if (toolName.startsWith('PM') && toolName.length > 2 && /[A-Z]/.test(toolName[2])) { - commandName = toolName.slice(2); - } - - return `cascade-tools pm ${toKebabCase(commandName)}`; -} - // --------------------------------------------------------------------------- // Factory function // --------------------------------------------------------------------------- diff --git a/src/integrations/alerting/_shared/format.ts b/src/integrations/alerting/_shared/format.ts index 28543393a..c2cb922b2 100644 --- a/src/integrations/alerting/_shared/format.ts +++ b/src/integrations/alerting/_shared/format.ts @@ -9,6 +9,7 @@ import type { SentryAugmentedPayload, SentryIssueAlertPayload, + SentryIssuePayload, SentryMetricAlertPayload, SentryStackFrame, } from '../../../sentry/types.js'; @@ -57,6 +58,38 @@ function findTopInAppFrame(frames?: SentryStackFrame[]): SentryStackFrame | unde return frames[frames.length - 1]; } +/** + * Build the PM card title and description body from a Sentry issue-lifecycle + * webhook (Sentry-Hook-Resource: issue — the Internal Integration default + * surface). Distinct from `formatSentryCardBody` (event_alert), which pulls + * fields from `data.event.{...}` instead of `data.issue.{...}`. + */ +export function formatSentryIssueLifecycleCardBody(augmented: SentryAugmentedPayload): AlertHints { + const payload = augmented.payload as SentryIssuePayload; + const issue = payload.data?.issue; + + const alertTitle = issue?.title?.trim() ? issue.title : 'Sentry Issue'; + const issueUrl = issue?.web_url ?? issue?.permalink ?? ''; + const lines: string[] = []; + + if (issueUrl) lines.push(`**Sentry issue:** ${issueUrl}`); + if (issue?.firstSeen) lines.push(`**First seen:** ${issue.firstSeen}`); + if (issue?.level) lines.push(`**Level:** ${issue.level}`); + if (issue?.shortId) lines.push(`**Short ID:** ${issue.shortId}`); + if (issue?.culprit) lines.push(`**Culprit:** \`${issue.culprit}\``); + + const md = issue?.metadata; + if (md?.filename || md?.function) { + const loc = [md.filename, md.function].filter(Boolean).join(':'); + lines.push(`**Top frame:** \`${loc}\``); + } + + return { + title: `[Sentry] ${alertTitle}`, + descriptionMarkdown: lines.join('\n'), + }; +} + /** Build the PM card title and description body from a Sentry metric_alert payload. */ export function formatSentryMetricCardBody(augmented: SentryAugmentedPayload): AlertHints { const payload = augmented.payload as SentryMetricAlertPayload; diff --git a/src/integrations/alerting/_shared/materialize.ts b/src/integrations/alerting/_shared/materialize.ts index adffa5a9f..0d9ca3d7a 100644 --- a/src/integrations/alerting/_shared/materialize.ts +++ b/src/integrations/alerting/_shared/materialize.ts @@ -47,6 +47,57 @@ function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } +/** + * Verify an existing pr_work_items mapping points to a live PM card and + * re-apply placement (idempotent for Trello, repairs failed prior moves for + * JIRA/Linear). Lazy-heals (creates a fresh card + replaces mapping) when the + * card returns 404. Re-throws any other PM error so BullMQ retries. + */ +async function reuseOrLazyHealMapping( + project: ProjectConfig, + source: AlertSource, + externalId: string, + containerId: string, + hints: AlertHints, + provider: ReturnType, + existing: { id: string; workItemId: string }, +): Promise { + try { + await provider.getWorkItem(existing.workItemId); + } catch (err) { + if (!is404Error(err)) throw err; + // Card deleted — create a replacement and CAS-swap the mapping row. + return createAndAttach(project, source, externalId, containerId, hints, provider, { + lazyHeal: { rowId: existing.id, oldWorkItemId: existing.workItemId }, + }); + } + // Re-apply placement so a prior failed moveWorkItem can be repaired on + // subsequent deliveries rather than staying permanently stuck. + const destination = getAlertsStatusDestination(project); + if (destination) { + await provider.moveWorkItem(existing.workItemId, destination); + } + return existing.workItemId; +} + +/** + * Lost the claim race — poll the winner's mapping row for its work_item_id. + * Returns the id when the winner attaches one; returns null after the polling + * budget is exhausted (caller throws MaterializationRetryExhausted). + */ +async function pollForConcurrentWinner( + project: ProjectConfig, + source: AlertSource, + externalId: string, +): Promise { + for (let attempt = 0; attempt < POLL_MAX_ATTEMPTS; attempt++) { + await sleep(POLL_DELAY_MS); + const row = await findByExternal(project.id, source, externalId); + if (row?.workItemId) return row.workItemId; + } + return null; +} + export async function materializeAlertWorkItem( source: AlertSource, externalId: string, @@ -60,36 +111,17 @@ export async function materializeAlertWorkItem( const provider = pmRegistry.createProvider(project); - // Step 1: check for an existing mapping + // Step 1: existing mapping? const existing = await findByExternal(project.id, source, externalId); if (existing?.workItemId) { - // Verify the PM card is still alive - try { - await provider.getWorkItem(existing.workItemId); - } catch (err) { - if (!is404Error(err)) throw err; - // Lazy-heal: card was deleted — fall through to create path - return await createAndAttach(project, source, externalId, containerId, hints, provider, { - lazyHeal: { rowId: existing.id, oldWorkItemId: existing.workItemId }, - }); - } - // Card exists — re-apply placement to repair any prior moveWorkItem failure. - // For JIRA/Linear this moves the issue into the configured alerts status; for - // Trello it is idempotent (card was created directly in the list). If the move - // throws a transient error the caller retries the full materialisation; if it - // throws a 404 the outer catch above handles it. This ensures that a - // moveWorkItem failure during the initial createAndAttach call is always - // repaired on subsequent deliveries rather than permanently stuck. - const destination = getAlertsStatusDestination(project); - if (destination) { - await provider.moveWorkItem(existing.workItemId, destination); - } - return existing.workItemId; + return reuseOrLazyHealMapping(project, source, externalId, containerId, hints, provider, { + id: existing.id, + workItemId: existing.workItemId, + }); } - // Step 2: atomically claim the mapping row + // Step 2: atomically claim the mapping row. const claim = await claimExternalMapping(project.id, source, externalId); - if (claim.ownedHere) { try { return await createAndAttach(project, source, externalId, containerId, hints, provider, { @@ -97,9 +129,8 @@ export async function materializeAlertWorkItem( rowId: claim.rowId, }); } catch (err) { - // createAndAttach failed — the claim row may still have work_item_id=NULL. - // Delete it (guarded by isNull) so the next Sentry delivery can reclaim it - // instead of polling to MaterializationRetryExhausted. + // createAndAttach failed — clear the NULL-work_item_id row so the next + // Sentry delivery can reclaim it instead of polling to exhaustion. await deleteExternalMappingClaim(claim.rowId).catch((deleteErr) => { logger.warn('[alert-materializer] failed to clean up stale claim row', { rowId: claim.rowId, @@ -113,15 +144,11 @@ export async function materializeAlertWorkItem( } } - // Lost to a concurrent winner — poll for its work_item_id + // Step 3: lost the claim race — return the winner's already-attached id, or + // poll until they attach one. if (claim.existing.workItemId) return claim.existing.workItemId; - - for (let attempt = 0; attempt < POLL_MAX_ATTEMPTS; attempt++) { - await sleep(POLL_DELAY_MS); - const row = await findByExternal(project.id, source, externalId); - if (row?.workItemId) return row.workItemId; - } - + const polled = await pollForConcurrentWinner(project, source, externalId); + if (polled) return polled; throw new MaterializationRetryExhausted(project.id, source, externalId); } diff --git a/src/integrations/alerting/_shared/types.ts b/src/integrations/alerting/_shared/types.ts index 2c123eeca..ca8e9dca0 100644 --- a/src/integrations/alerting/_shared/types.ts +++ b/src/integrations/alerting/_shared/types.ts @@ -17,8 +17,24 @@ * propagate this so BullMQ retries the job. */ -/** All supported alert source identifiers. Extend this union as new sources are added. */ -export type AlertSource = 'sentry' | 'sentry-metric' | 'pagerduty' | 'datadog' | 'github-alert'; +/** + * All supported alert source identifiers. Extend this union as new sources are added. + * + * Sentry has three surfaces, each given a distinct literal so the + * `(project_id, external_source, external_id)` partial-unique index on + * `pr_work_items` doesn't collide across surfaces (e.g. the same Sentry issue + * arriving via both `event_alert` and `issue` materializes two cards): + * - `'sentry'` — event_alert (Sentry Alert Rule firings) + * - `'sentry-metric'` — metric_alert (metric-based alert rules) + * - `'sentry-issue'` — issue lifecycle (Internal Integration default surface) + */ +export type AlertSource = + | 'sentry' + | 'sentry-metric' + | 'sentry-issue' + | 'pagerduty' + | 'datadog' + | 'github-alert'; /** Formatted card content produced by a per-source format helper. */ export interface AlertHints { diff --git a/src/router/adapters/sentry.ts b/src/router/adapters/sentry.ts index 87efe75a8..28b2de594 100644 --- a/src/router/adapters/sentry.ts +++ b/src/router/adapters/sentry.ts @@ -28,7 +28,14 @@ import { withPMScopeForDispatch } from './_shared.js'; // Processable resource types // ============================================================================ -const PROCESSABLE_RESOURCES = ['event_alert', 'metric_alert'] as const; +// Three Sentry webhook surfaces, each with a distinct trigger handler: +// - 'event_alert' → SentryIssueAlertTrigger (Sentry Alert Rule firings) +// - 'metric_alert' → SentryMetricAlertTrigger (metric-based alert rules) +// - 'issue' → SentryIssueLifecycleTrigger (Internal Integration default +// surface; new issue created/etc.). Captured live shape +// verified against 2026-05-09 prod webhook id +// fbdc6d87-b962-444c-8a2a-a9452a74ff71. +const PROCESSABLE_RESOURCES = ['event_alert', 'metric_alert', 'issue'] as const; // ============================================================================ // Extended parsed event diff --git a/src/sentry/types.ts b/src/sentry/types.ts index 512eeed38..672d1f55a 100644 --- a/src/sentry/types.ts +++ b/src/sentry/types.ts @@ -175,24 +175,49 @@ export interface SentryMetricAlertPayload { _cascadeProjectId?: string; } -/** Sentry issue lifecycle webhook payload (Sentry-Hook-Resource: issue) */ +/** + * Sentry issue lifecycle webhook payload (Sentry-Hook-Resource: issue). + * + * Sent by the Sentry "Internal Integration" surface (Custom Integrations → + * Webhook → Issue) when an issue's lifecycle changes. Distinct from + * `event_alert` (Sentry Alert Rule firings) — same Sentry issue can deliver + * via both surfaces if both are wired. Captured live shape verified against + * 2026-05-09 13:18:51 UTC prod webhook id `fbdc6d87-b962-444c-8a2a-a9452a74ff71`. + */ export interface SentryIssuePayload { action: 'created' | 'resolved' | 'assigned' | 'archived' | 'unresolved'; actor?: { id?: string; name?: string; type?: string }; + installation?: { uuid?: string }; data: { - id?: string; - title?: string; - url?: string; - web_url?: string; - project_url?: string; - status?: string; - substatus?: string; - issueCategory?: string; - count?: number; - userCount?: number; - firstSeen?: string; - lastSeen?: string; - assignedTo?: unknown; + issue: { + id: string; + title: string; + culprit?: string; + shortId?: string; + level?: string; + issueType?: string; + permalink?: string; + web_url?: string; + url?: string; + project_url?: string; + project?: { id?: string; name?: string; slug?: string; platform?: string }; + metadata?: { + type?: string; + value?: string; + filename?: string; + function?: string; + }; + platform?: string; + priority?: string; + firstSeen?: string; + lastSeen?: string; + status?: string; + substatus?: string; + issueCategory?: string; + count?: string | number; + userCount?: number; + assignedTo?: unknown; + }; }; /** Injected by CASCADE router to identify the project (from URL path param) */ _cascadeProjectId?: string; diff --git a/src/triggers/README.md b/src/triggers/README.md index f70b7270c..c2f61fc40 100644 --- a/src/triggers/README.md +++ b/src/triggers/README.md @@ -17,7 +17,7 @@ Webhook processing is split into two distinct tiers: | **Router** | `src/router/` | Receive, validate, acknowledge, enqueue | | **Worker** | `src/triggers/` | Resolve trigger, establish credentials, run agent | -**Router side is fully unified** — all four providers (Trello, JIRA, GitHub, Sentry) share `processRouterWebhook()` + `RouterPlatformAdapter`. No provider-specific branching in the router. +**Router side is fully unified** — Trello, JIRA, Linear, GitHub, and Sentry share `processRouterWebhook()` + `RouterPlatformAdapter`. No provider-specific branching in the router. **Worker side has intentional divergence** — see below. @@ -30,7 +30,7 @@ Webhook processing is split into two distinct tiers: | Trigger dispatch | ✅ Registry | ✅ Registry or pre-resolved | ✅ Registry or pre-resolved | | Ack comment (PR) | ❌ N/A | ✅ Posts to PR | ❌ N/A | | Ack comment (PM) | ✅ Via PM lifecycle | ✅ For PM-focused agents | ❌ N/A | -| CI check polling | ❌ N/A | ✅ `pollWaitForChecks()` | ❌ N/A | +| Check-suite decision | ❌ N/A | ✅ Aggregate check-run state in trigger handlers | ❌ N/A | | PM credential scope | ✅ `integration.withCredentials` | ✅ `withPMCredentials` | ✅ `withPMCredentials` | | PM lifecycle ops | ✅ prepareForAgent / handleFailure | ✅ For PM-focused agents | ❌ Skipped | | Persona token mgmt | ❌ N/A | ✅ Implementer / reviewer | ❌ N/A | @@ -51,7 +51,7 @@ Forcing GitHub or Sentry into this pipeline would require: ### GitHub-specific features (cannot be generalized) -1. **CI check polling** (`pollWaitForChecks`) — GitHub is the only provider with CI. No other source polls build status before running an agent. +1. **Check-suite aggregation** — GitHub is the only provider with CI. The check-suite triggers inspect aggregate check-run state for the PR head SHA and either dispatch `review`, dispatch `respond-to-ci`, or return a structured skip while checks are incomplete. Worker-side CI polling is intentionally not part of this path. 2. **PR acknowledgment comments** — GitHub PRs get a comment like "👀 Reviewing…" immediately. No other source has this flow. 3. **Dual-persona token management** — The implementer vs. reviewer persona selection is GitHub-specific. No Trello/JIRA/Sentry equivalent. 4. **PM-focused agent routing** — When a PM-focused agent (e.g. `backlog-manager`) fires from a GitHub PR event, it posts the ack to Trello/JIRA instead of the PR, and uses PM-appropriate lifecycle config. @@ -74,7 +74,7 @@ To reduce duplication across the three worker-side handlers, shared utilities ar | File | Purpose | Used By | |------|---------|---------| | `concurrency.ts` | `withAgentTypeConcurrency()` — wraps check→mark→execute→clear | GitHub, Sentry | -| `trigger-resolution.ts` | `resolveTriggerResult()` — pre-resolved or dispatch | Sentry (GitHub and PM use inline logic) | +| `trigger-resolution.ts` | `resolveTriggerResult()` — pre-resolved or dispatch | GitHub, PM, Sentry | | `credential-scope.ts` | `withPMScope()` — `withPMCredentials` + `withPMProvider` | GitHub, Sentry | | `pm-ack.ts` | `postPMAckComment()` — posts ack to Trello/JIRA | GitHub worker handler | | `events.ts` | `TRIGGER_EVENTS` — typed catalog of canonical trigger event names | Trigger handlers and tests | @@ -92,9 +92,67 @@ To reduce duplication across the three worker-side handlers, shared utilities ar --- +## Trigger Authoring Contracts + +### Canonical events + +Use `TRIGGER_EVENTS` from `src/triggers/shared/events.ts` for every new trigger event. The catalog is the source of truth for event IDs written into `agentInput.triggerEvent`, trigger configuration rows, and static consistency tests: + +| Category | Events | +|----------|--------| +| `PM` | `pm:status-changed`, `pm:label-added`, `pm:comment-mention` | +| `SCM` | `scm:check-suite-success`, `scm:check-suite-failure`, `scm:pr-review-submitted`, `scm:review-requested`, `scm:pr-opened`, `scm:pr-comment-mention`, `scm:pr-merged`, `scm:pr-ready-to-merge`, `scm:pr-conflict-detected` | +| `ALERTING` | `alerting:issue-alert`, `alerting:metric-alert` | +| `INTERNAL` | `internal:auto-chain` | + +Do not introduce raw event-string literals in new handlers. If a handler checks `checkTriggerEnabled(..., event, ...)`, the same event must be emitted as `agentInput.triggerEvent`; `tests/unit/triggers/trigger-event-consistency.test.ts` enforces that invariant because mismatches make enabled triggers silently fall back to YAML defaults. + +### Result builders + +Prefer the shared builders instead of hand-assembling `TriggerResult` objects: + +| Builder | Use | +|---------|-----| +| `buildPMDispatchResult` | Generic PM dispatch shape with `workItemId` and canonical trigger event | +| `buildPMStatusDispatchResult` | PM status-transition dispatch plus PM coalescing key | +| `buildPMLabelDispatchResult` | PM label-trigger dispatch | +| `buildGitHubPRDispatchResult` | Generic GitHub PR dispatch shape with `prNumber`, PR metadata, and optional PM work-item metadata | +| `buildReviewResult`, `buildRespondToCiResult`, `buildResolveConflictsResult` | GitHub-specific agent input shapes | +| `buildNoAgentResult` | Matched trigger completed a side effect but should not spawn an agent | +| `buildSkipResult` / `skip()` | Matched handler deliberately self-skipped and should stop registry dispatch with a structured reason | +| `buildDeferredRecheckResult` | **GitHub-only** — Router schedules a bare delayed job; the GitHub worker re-dispatches through the registry for fresh provider state. Non-GitHub adapters embed `triggerResult` in the job, so their workers return the same `agentType: null` result without re-dispatching. See the **Deferred re-check** section below. | + +### `null` vs structured skip + +`TriggerRegistry.dispatch()` is first-match dispatch with one important distinction: + +- Return bare `null` only when this handler does not claim the event and the registry should continue to later handlers. +- Return `buildSkipResult(handler, message)` when the handler did claim the event but chose not to run an agent, such as disabled config, loop-prevention gates, incomplete aggregate checks, or missing prerequisite state. The router logs a stable decision reason: `Trigger skipped: `. + +This distinction prevents "No trigger matched for event" from hiding real handler decisions. + +### Deferred re-check + +`buildDeferredRecheckResult` is currently a **GitHub-only** contract for bare re-dispatch. The router always schedules the delayed job via `scheduleCoalescedJob` and exits without taking dispatch locks or posting an ack — that part is generic. What differs is what the worker does when the job fires: + +- **GitHub adapter** — `GitHubRouterAdapter.buildJob()` strips `triggerResult` from the job and sets `mergeabilityRecheckAttempt: 1`. The GitHub worker detects the bare job and re-dispatches through the registry to evaluate fresh provider state. If the re-check still cannot resolve state, the GitHub worker emits `mergeability_recheck_exhausted` and stops without re-queueing. +- **Non-GitHub adapters (Trello, JIRA, Linear, Sentry)** — these adapters always embed `triggerResult` in the job regardless of `deferredRecheck`. When the job fires, `resolveTriggerResult()` receives the pre-resolved result and returns it directly, skipping registry dispatch. A non-GitHub handler returning `buildDeferredRecheckResult` would therefore schedule a job that re-uses the same `agentType: null` result instead of re-evaluating provider state. + +Use this builder only in GitHub handlers unless the adapter and worker for your provider have been updated to support bare re-dispatch. + +### Handler rules + +- `matches(ctx)` should be cheap, source-specific, and side-effect free. +- `handle(ctx)` owns expensive lookups, trigger-config checks, and the final dispatch decision. +- PM status and label handlers should use shared PM helpers so Trello, JIRA, and Linear stay behaviorally aligned. +- GitHub PR agent dispatch should use the GitHub result builders so PR metadata, work-item metadata, `headSha`, and `triggerType` remain consistent. +- Prefer structured skip over throwing for expected non-dispatch reasons. Throwing from router dispatch is treated as non-fatal by the router and loses handler-specific decision detail. + +--- + ## Flow Diagrams -### PM webhook (Trello / JIRA) +### PM webhook (Trello / JIRA / Linear) ``` processPMWebhook(integration, payload, registry) @@ -118,8 +176,8 @@ processPMWebhook(integration, payload, registry) processGitHubWebhook(payload, eventType, registry, ackCommentId, triggerResult) └─ integration.parseWebhookPayload(payload) → event └─ integration.lookupProject(event.repo) → project - └─ [inline] if triggerResult → use it, else dispatchTrigger(registry, payload, project) - └─ [optional] pollWaitForChecks(result, repo) → checksOk + └─ resolveTriggerResult(registry, ctx, triggerResult) → result + └─ check-suite handlers inspect aggregate check-run state before dispatch └─ maybePostAckComment(result, ...) → PR or PM ack └─ runGitHubAgent(result, project, config) └─ withAgentTypeConcurrency(projectId, agentType) diff --git a/src/triggers/sentry/alerting-issue-lifecycle.ts b/src/triggers/sentry/alerting-issue-lifecycle.ts new file mode 100644 index 000000000..2e60dbf09 --- /dev/null +++ b/src/triggers/sentry/alerting-issue-lifecycle.ts @@ -0,0 +1,129 @@ +/** + * Trigger handler: Sentry issue-lifecycle webhook (Sentry-Hook-Resource: issue). + * + * This is the default surface for Sentry "Internal Integrations" — when a + * project enables the `issue` webhook permission, Sentry sends one webhook + * per issue lifecycle event (created / resolved / archived / unresolved / + * assigned). Distinct from `event_alert` (Sentry Alert Rule firings, + * handled by SentryIssueAlertTrigger): the same Sentry issue can deliver + * via both surfaces if both are wired. + * + * First cut handles `action: 'created'` only — i.e. fire the alerting + * agent when a new issue is created in Sentry. Resolved/archived/etc. + * lifecycle actions are deferred (would auto-close the cascade work item; + * separate spec scope). + * + * PM card materialisation is deferred to the worker side (processSentryWebhook), + * mirroring the SentryIssueAlertTrigger pattern. This means transient PM + * failures surface as BullMQ retries rather than being silently swallowed + * as non-fatal dispatch errors. + * + * AlertSource literal is `'sentry-issue'` (distinct from `'sentry'` for + * event_alert) so the (project_id, external_source, external_id) partial- + * unique index on pr_work_items doesn't collide with future Alert-Rule + * webhooks for the same Sentry issue ID. + */ + +import { getAlertsContainerId } from '../../pm/config.js'; +import { getSentryIntegrationConfig } from '../../sentry/integration.js'; +import type { SentryAugmentedPayload, SentryIssuePayload } from '../../sentry/types.js'; +import type { TriggerContext, TriggerHandler, TriggerResult } from '../../types/index.js'; +import { logger } from '../../utils/logging.js'; +import { TRIGGER_EVENTS } from '../shared/events.js'; +import { checkTriggerEnabledWithParams } from '../shared/trigger-check.js'; + +export class SentryIssueLifecycleTrigger implements TriggerHandler { + name = 'sentry-issue-lifecycle'; + description = 'Triggers alerting agent when a Sentry issue is created (lifecycle webhook)'; + + matches(ctx: TriggerContext): boolean { + if (ctx.source !== 'sentry') return false; + const augmented = ctx.payload as SentryAugmentedPayload; + if (augmented.resource !== 'issue') return false; + const inner = augmented.payload as SentryIssuePayload; + // First cut: only fire on 'created'. Resolved/archived/unresolved/assigned + // would auto-close or annotate the cascade card — deferred to a future + // spec to keep the blast radius small. + return inner.action === 'created'; + } + + async handle(ctx: TriggerContext): Promise { + const triggerConfig = await checkTriggerEnabledWithParams( + ctx.project.id, + 'alerting', + TRIGGER_EVENTS.ALERTING.ISSUE_LIFECYCLE, + this.name, + ); + if (!triggerConfig.enabled) { + logger.debug('SentryIssueLifecycleTrigger: trigger disabled, skipping', { + projectId: ctx.project.id, + }); + return null; + } + + const augmented = ctx.payload as SentryAugmentedPayload; + const inner = augmented.payload as SentryIssuePayload; + const issue = inner.data?.issue; + + const issueId = issue?.id; + if (!issueId) { + logger.warn('SentryIssueLifecycleTrigger: cannot determine issue ID from payload', { + projectId: ctx.project.id, + }); + return null; + } + + const issueUrl = issue?.web_url ?? issue?.permalink ?? issue?.url; + const alertTitle = issue?.title?.trim() ? issue.title : 'Sentry Issue'; + + // Look up org slug from integration config (mirrors SentryIssueAlertTrigger). + const sentryConfig = await getSentryIntegrationConfig(ctx.project.id); + if (!sentryConfig) { + logger.warn('SentryIssueLifecycleTrigger: no Sentry integration config for project', { + projectId: ctx.project.id, + }); + return null; + } + + // Pre-flight: verify the alerts slot is configured before dispatching. + // Actual PM card creation is deferred to the worker (processSentryWebhook) + // so transient PM failures surface as BullMQ retries. + if (!getAlertsContainerId(ctx.project)) { + logger.warn('SentryIssueLifecycleTrigger: alerts slot not configured, skipping dispatch', { + projectId: ctx.project.id, + source: 'sentry', + reason: 'alerts_slot_missing', + }); + return null; + } + + logger.info('Alerting: Sentry issue-lifecycle event triggered', { + projectId: ctx.project.id, + issueId, + alertTitle, + orgId: sentryConfig.organizationSlug, + }); + + return { + agentType: 'alerting', + agentInput: { + triggerEvent: TRIGGER_EVENTS.ALERTING.ISSUE_LIFECYCLE, + // workItemId is intentionally absent — the worker (processSentryWebhook) + // materialises the PM card via materializeAlertWorkItem('sentry-issue', ...) + // and sets workItemId before running the agent. + alertIssueId: issueId, + alertOrgId: sentryConfig.organizationSlug, + alertTitle, + alertIssueUrl: issueUrl, + }, + // lockKey provides router-level work-item concurrency protection while + // the PM card ID is not yet known. The `sentry-issue:` namespace is + // distinct from `sentry:` (event_alert) so the same issue can arrive + // via both surfaces concurrently without lock contention. + lockKey: `sentry-issue:${issueId}`, + // coalesceKey shares the namespace so rapid re-fires of the same + // issue-lifecycle webhook are coalesced without needing a PM card ID. + coalesceKey: `${ctx.project.id}:sentry-issue:${issueId}`, + }; + } +} diff --git a/src/triggers/sentry/register.ts b/src/triggers/sentry/register.ts index b96d8d580..824b6498a 100644 --- a/src/triggers/sentry/register.ts +++ b/src/triggers/sentry/register.ts @@ -1,8 +1,10 @@ import type { TriggerRegistry } from '../registry.js'; import { SentryIssueAlertTrigger } from './alerting-issue.js'; +import { SentryIssueLifecycleTrigger } from './alerting-issue-lifecycle.js'; import { SentryMetricAlertTrigger } from './alerting-metric.js'; export function registerSentryTriggers(registry: TriggerRegistry): void { registry.register(new SentryIssueAlertTrigger()); registry.register(new SentryMetricAlertTrigger()); + registry.register(new SentryIssueLifecycleTrigger()); } diff --git a/src/triggers/sentry/webhook-handler.ts b/src/triggers/sentry/webhook-handler.ts index ddc5abd3a..3fb1390be 100644 --- a/src/triggers/sentry/webhook-handler.ts +++ b/src/triggers/sentry/webhook-handler.ts @@ -20,15 +20,97 @@ import { AlertSlotMissingError } from '../../integrations/alerting/_shared/types.js'; import type { SentryAugmentedPayload } from '../../sentry/types.js'; -import type { TriggerResult } from '../../types/index.js'; +import type { ProjectConfig, TriggerResult } from '../../types/index.js'; import { startWatchdog } from '../../utils/lifecycle.js'; import { logger } from '../../utils/logging.js'; import type { TriggerRegistry } from '../registry.js'; import { runAgentExecutionPipeline } from '../shared/agent-execution.js'; import { withAgentTypeConcurrency } from '../shared/concurrency.js'; import { withPMScope } from '../shared/credential-scope.js'; +import { TRIGGER_EVENTS } from '../shared/events.js'; import { resolveTriggerResult } from '../shared/trigger-resolution.js'; +/** + * Materialise the Sentry alert as a PM work item. Picks the right + * (AlertSource, format helper) pair from the trigger result's agentInput + * discriminators (`triggerEvent`, `alertIssueId`, `alertMetricKey`). + * + * Returns the resolved TriggerResult with `workItemId` populated, or `null` + * to signal the caller should skip the agent run (alerts slot was + * unconfigured between router dispatch and worker execution — graceful skip, + * not a retry; the operator must reconfigure). + * + * Re-throws any other error so BullMQ retries the job (transient PM failures + * — 5xx, DB errors, polling exhaustion). + */ +async function materializeSentryAlertWorkItem( + result: TriggerResult, + payload: unknown, + project: ProjectConfig, + projectId: string, +): Promise { + const triggerEvent = + typeof result.agentInput.triggerEvent === 'string' ? result.agentInput.triggerEvent : null; + const alertIssueId = + typeof result.agentInput.alertIssueId === 'string' ? result.agentInput.alertIssueId : null; + const alertMetricKey = + typeof result.agentInput.alertMetricKey === 'string' ? result.agentInput.alertMetricKey : null; + if (result.workItemId || (!alertIssueId && !alertMetricKey)) return result; + + const { materializeAlertWorkItem } = await import( + '../../integrations/alerting/_shared/materialize.js' + ); + const { formatSentryCardBody, formatSentryIssueLifecycleCardBody, formatSentryMetricCardBody } = + await import('../../integrations/alerting/_shared/format.js'); + + const augmented = payload as SentryAugmentedPayload; + try { + let workItemId: string; + if (triggerEvent === TRIGGER_EVENTS.ALERTING.ISSUE_LIFECYCLE && alertIssueId) { + // Sentry-Hook-Resource: issue (Internal Integration default surface). + // Distinct AlertSource ('sentry-issue') from event_alert ('sentry') so the + // partial-unique (project_id, external_source, external_id) index doesn't + // collide if the same Sentry issue arrives via both surfaces. + workItemId = await materializeAlertWorkItem( + 'sentry-issue', + alertIssueId, + project, + formatSentryIssueLifecycleCardBody(augmented), + ); + } else if (alertIssueId) { + // event_alert path (Sentry Alert Rule firings). + workItemId = await materializeAlertWorkItem( + 'sentry', + alertIssueId, + project, + formatSentryCardBody(augmented), + ); + } else { + // alertMetricKey is guaranteed non-null here (checked above). + workItemId = await materializeAlertWorkItem( + 'sentry-metric', + alertMetricKey as string, + project, + formatSentryMetricCardBody(augmented), + ); + } + return { + ...result, + workItemId, + agentInput: { ...result.agentInput, workItemId }, + }; + } catch (err) { + if (err instanceof AlertSlotMissingError) { + logger.warn('processSentryWebhook: alerts slot no longer configured, skipping agent run', { + projectId, + reason: 'alerts_slot_missing', + }); + return null; + } + throw err; + } +} + export async function processSentryWebhook( payload: unknown, projectId: string, @@ -83,66 +165,17 @@ export async function processSentryWebhook( // as BullMQ retries instead of being swallowed as non-fatal dispatch errors // by processRouterWebhook (which returns HTTP 200 to Sentry, preventing retries). // - // Both SentryIssueAlertTrigger (alertIssueId) and SentryMetricAlertTrigger - // (alertMetricKey) defer PM card creation to here. The trigger handlers only - // pre-check that the alerts slot is configured; actual PM card creation happens - // here where the error propagates to BullMQ's retry budget. - let resolvedResult = result; - const alertIssueId = - typeof result.agentInput.alertIssueId === 'string' - ? result.agentInput.alertIssueId - : null; - const alertMetricKey = - typeof result.agentInput.alertMetricKey === 'string' - ? result.agentInput.alertMetricKey - : null; - if (!result.workItemId && (alertIssueId || alertMetricKey)) { - const { materializeAlertWorkItem } = await import( - '../../integrations/alerting/_shared/materialize.js' - ); - const { formatSentryCardBody, formatSentryMetricCardBody } = await import( - '../../integrations/alerting/_shared/format.js' - ); - try { - let workItemId: string; - if (alertIssueId) { - const hints = formatSentryCardBody(payload as SentryAugmentedPayload); - workItemId = await materializeAlertWorkItem( - 'sentry', - alertIssueId, - pc.project, - hints, - ); - } else { - // alertMetricKey is guaranteed non-null here (checked above) - const hints = formatSentryMetricCardBody(payload as SentryAugmentedPayload); - workItemId = await materializeAlertWorkItem( - 'sentry-metric', - alertMetricKey as string, - pc.project, - hints, - ); - } - resolvedResult = { - ...result, - workItemId, - agentInput: { ...result.agentInput, workItemId }, - }; - } catch (err) { - if (err instanceof AlertSlotMissingError) { - // Slot was unconfigured between router dispatch and worker execution. - // Treat as a graceful skip (don't retry — the operator must reconfigure). - logger.warn( - 'processSentryWebhook: alerts slot no longer configured, skipping agent run', - { projectId, reason: 'alerts_slot_missing' }, - ); - return; - } - // Transient PM failure (5xx, DB error, polling exhaustion): re-throw so - // BullMQ retries the job rather than silently dropping the alert. - throw err; - } - } + // All three Sentry trigger handlers (issue-alert, metric-alert, + // issue-lifecycle) defer PM card creation to here. The trigger handlers + // only pre-check that the alerts slot is configured; actual creation + // happens here where the error propagates to BullMQ's retry budget. + const resolvedResult = await materializeSentryAlertWorkItem( + result, + payload, + pc.project, + projectId, + ); + if (resolvedResult === null) return; // alerts slot unconfigured — graceful skip return runAgentExecutionPipeline(resolvedResult, pc.project, pc.config, { logLabel: 'Sentry agent', diff --git a/src/triggers/shared/events.ts b/src/triggers/shared/events.ts index f2d421ac3..043a4e688 100644 --- a/src/triggers/shared/events.ts +++ b/src/triggers/shared/events.ts @@ -18,6 +18,7 @@ export const TRIGGER_EVENTS = { ALERTING: { ISSUE_ALERT: 'alerting:issue-alert', METRIC_ALERT: 'alerting:metric-alert', + ISSUE_LIFECYCLE: 'alerting:issue-lifecycle', }, INTERNAL: { AUTO_CHAIN: 'internal:auto-chain', diff --git a/src/types/index.ts b/src/types/index.ts index 2b9c07537..2a1db8fbb 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -162,11 +162,17 @@ export interface TriggerResult { message: string; }; /** - * When set and `agentType` is `null`, the router schedules a bare delayed - * job (no embedded trigger result) keyed by `coalesceKey`. The worker - * re-dispatches via the trigger registry when the job fires, obtaining fresh - * state. Any trigger handler can use this field; the router branch in - * `processRouterWebhook` is adapter-agnostic. + * When set and `agentType` is `null`, the router schedules a coalesced bare + * delayed job via `scheduleCoalescedJob`. The router scheduling is + * adapter-agnostic, but **bare re-dispatch is currently GitHub-only**: + * `GitHubRouterAdapter.buildJob()` omits `triggerResult` from the job so the + * GitHub worker re-dispatches through the trigger registry for fresh provider + * state. Non-GitHub adapters (Trello, JIRA, Linear, Sentry) embed + * `triggerResult` in the job; their workers pass it to + * `resolveTriggerResult()`, which returns the pre-resolved `agentType: null` + * result without dispatching. Use this field only in GitHub handlers unless + * the adapter and worker for your provider have been updated to support bare + * re-dispatch. */ deferredRecheck?: { delayMs: number; diff --git a/tests/helpers/factories.ts b/tests/helpers/factories.ts index 95fd5059e..91bef1334 100644 --- a/tests/helpers/factories.ts +++ b/tests/helpers/factories.ts @@ -1,4 +1,5 @@ import type { TRPCContext, TRPCUser } from '../../src/api/trpc.js'; +import type { CheckSuiteStatus } from '../../src/github/client.js'; import type { TrelloCard } from '../../src/trello/client.js'; import type { TrelloWebhookPayload } from '../../src/triggers/trello/types.js'; import type { ProjectConfig, TriggerContext } from '../../src/types/index.js'; @@ -215,6 +216,71 @@ export function createTrelloTriggerContext(overrides?: Partial): } as TriggerContext; } +export type PMStatusFixtureKind = 'create' | 'move'; + +export interface PMStatusFixture { + projectId: string; + kind: PMStatusFixtureKind; + agentType: string; + workItemId: string; + workItemUrl: string; + workItemTitle: string; + providerStatusId: string; + providerStatusName: string; + previousStatusId?: string; + previousStatusName?: string; +} + +/** + * Provider-neutral PM status-change fixture. + * + * Captures the contract-level data shared by Trello/JIRA/Linear without + * coupling conformance tests to each provider's webhook payload shape. + */ +export function createPMStatusFixture(overrides?: Partial): PMStatusFixture { + return { + projectId: 'project-1', + kind: 'move', + agentType: 'implementation', + workItemId: 'work-item-123', + workItemUrl: 'https://pm.example.test/work-item-123', + workItemTitle: 'Implement contract coverage', + providerStatusId: 'todo-list-id', + providerStatusName: 'Todo', + previousStatusId: 'planning-list-id', + previousStatusName: 'Planning', + ...overrides, + }; +} + +export interface PMLabelAddedFixture { + agentType: string; + workItemId: string; + workItemUrl: string; + workItemTitle: string; + labelId: string; + labelName: string; + containerId: string; +} + +/** + * Provider-neutral PM label-added fixture for ready-to-process style triggers. + */ +export function createPMLabelAddedFixture( + overrides?: Partial, +): PMLabelAddedFixture { + return { + agentType: 'splitting', + workItemId: 'work-item-456', + workItemUrl: 'https://pm.example.test/work-item-456', + workItemTitle: 'Split contract coverage', + labelId: 'ready-label-id', + labelName: 'Ready to process', + containerId: 'splitting-list-id', + ...overrides, + }; +} + // --------------------------------------------------------------------------- // GitHub payload factories // --------------------------------------------------------------------------- @@ -294,6 +360,32 @@ export function createCheckSuitePayload(overrides?: Partial): }; } +/** + * Creates aggregate check-suite state as returned by `githubClient.getCheckSuiteStatus()`. + */ +export function createCheckSuiteStatus( + checkRuns: CheckSuiteStatus['checkRuns'] = [ + { name: 'ci', status: 'completed', conclusion: 'success' }, + ], + overrides?: Partial, +): CheckSuiteStatus { + return { + totalCount: checkRuns.length, + checkRuns, + // Mirror production predicate: at least one check required; success/skipped/neutral are passing. + allPassing: + checkRuns.length > 0 && + checkRuns.every( + (checkRun) => + checkRun.status === 'completed' && + (checkRun.conclusion === 'success' || + checkRun.conclusion === 'skipped' || + checkRun.conclusion === 'neutral'), + ), + ...overrides, + }; +} + /** * Shape of a GitHub pull request review webhook payload. */ diff --git a/tests/unit/backends/codex.test.ts b/tests/unit/backends/codex.test.ts index a8e77ea4c..df2db8ad2 100644 --- a/tests/unit/backends/codex.test.ts +++ b/tests/unit/backends/codex.test.ts @@ -1218,6 +1218,80 @@ describe('Codex subscription auth', () => { { error: 'Error: DB write failed' }, ); }); + + // Prod regression 2026-05-09 (runs 8b000cd6 + d8e31665, both + // cascade/implementation/codex): codex's persistent bash session breaks + // with `ERROR codex_core::tools::router: error=write_stdin failed: stdin + // is closed for this session`. Once that signal fires, every subsequent + // command in the session inherits a corrupted stdout buffer (lint output + // from one command bleeding into the next, sidecar writes racing). We + // observed this leading to silent run failures with PR-creation evidence + // missing despite a real PR existing on GitHub. Fail loudly: when the + // signal appears in stderr, surface it as a run failure rather than + // continuing in a corrupted state. + describe('shell-state corruption (write_stdin closed)', () => { + it('marks the run as failed when stderr contains the persistent-shell stdin-closed signal', async () => { + mockSpawn.mockImplementation((_cmd: string, args: string[]) => { + const outputPath = args[args.indexOf('-o') + 1]; + return createMockChild({ + stdoutLines: [ + JSON.stringify({ type: 'turn.started' }), + JSON.stringify({ + type: 'turn.completed', + usage: { input_tokens: 5, output_tokens: 3 }, + }), + ], + stderr: + '2026-05-09T15:36:59.079680Z ERROR codex_core::tools::router: error=write_stdin failed: stdin is closed for this session; rerun exec_command with tty=true to keep stdin open\n', + onBeforeClose: () => { + writeFileSync(outputPath, 'PR created at https://github.com/o/r/pull/1', 'utf-8'); + }, + exitCode: 0, // <-- exit code 0 — codex itself doesn't fail; the shell-state signal is the only evidence + }); + }); + + const engine = new CodexEngine(); + const input = makeInput({ repoDir: workspaceDir }); + const result = await engine.execute(input); + + expect(result.success).toBe(false); + expect(result.error).toMatch(/codex shell-state corrupted/i); + expect(input.logWriter).toHaveBeenCalledWith( + 'ERROR', + expect.stringContaining('shell-state corrupted'), + expect.objectContaining({ + stderr: expect.stringContaining('write_stdin failed'), + }), + ); + }); + + it('passes through cleanly when stderr is benign (no false positives)', async () => { + mockSpawn.mockImplementation((_cmd: string, args: string[]) => { + const outputPath = args[args.indexOf('-o') + 1]; + return createMockChild({ + stdoutLines: [ + JSON.stringify({ type: 'turn.started' }), + JSON.stringify({ + type: 'turn.completed', + usage: { input_tokens: 5, output_tokens: 3 }, + }), + ], + stderr: 'some benign warning that is not the shell-state signal\n', + onBeforeClose: () => { + writeFileSync(outputPath, 'Finished. https://github.com/o/r/pull/1', 'utf-8'); + }, + exitCode: 0, + }); + }); + + const engine = new CodexEngine(); + const input = makeInput({ repoDir: workspaceDir }); + const result = await engine.execute(input); + + expect(result.success).toBe(true); + expect(result.error).toBeUndefined(); + }); + }); }); describe('CodexEngine lifecycle hooks', () => { diff --git a/tests/unit/backends/postProcess.test.ts b/tests/unit/backends/postProcess.test.ts index f9a3ba5f1..f63a27440 100644 --- a/tests/unit/backends/postProcess.test.ts +++ b/tests/unit/backends/postProcess.test.ts @@ -8,8 +8,13 @@ vi.mock('../../../src/utils/logging.js', () => ({ }, })); +vi.mock('../../../src/sentry.js', () => ({ + captureException: vi.fn(), +})); + import { postProcessResult } from '../../../src/backends/postProcess.js'; import type { AgentEngine, AgentEngineResult } from '../../../src/backends/types.js'; +import { captureException } from '../../../src/sentry.js'; import type { AgentInput, ProjectConfig } from '../../../src/types/index.js'; import { logger } from '../../../src/utils/logging.js'; @@ -122,6 +127,69 @@ describe('postProcessResult', () => { expect(result.error).toBe('Agent completed but no authoritative PR creation was recorded'); }); + // Prod regression 2026-05-09 (run d8e31665): the no-authoritative-PR + // failure surfaced only as a per-run record + a one-line WARN. Operators + // reading `cascade runs list` saw "failed" with no idea whether this is + // a recurring regression. Sentry capture under a stable tag makes prod + // frequency loud and gives ops a single dashboard to monitor. + it('emits a Sentry capture under tag pr_sidecar_invalid when authoritative PR evidence is missing', () => { + const captureSpy = vi.mocked(captureException); + captureSpy.mockClear(); + const result = makeResult({ + success: true, + prUrl: 'https://github.com/o/r/pull/1290', + prEvidence: { source: 'text', authoritative: false }, + }); + const engine = makeEngine('codex'); + const input = makeInput(); + + postProcessResult(result, 'implementation', engine, input, 'impl-card-fe82YUKV', { + requiresPR: true, + }); + + expect(captureSpy).toHaveBeenCalledTimes(1); + expect(captureSpy).toHaveBeenCalledWith( + expect.any(Error), + expect.objectContaining({ + tags: expect.objectContaining({ + source: 'pr_sidecar_invalid', + engine: 'codex', + agentType: 'implementation', + }), + extra: expect.objectContaining({ + identifier: 'impl-card-fe82YUKV', + prUrl: 'https://github.com/o/r/pull/1290', + prEvidenceSource: 'text', + }), + }), + ); + // Error message matches the user-visible failure string for grep parity. + const arg = captureSpy.mock.calls[0]![0] as Error; + expect(arg.message).toBe('Agent completed but no authoritative PR creation was recorded'); + }); + + it('does NOT Sentry-capture when authoritative PR evidence IS present', () => { + const captureSpy = vi.mocked(captureException); + captureSpy.mockClear(); + const result = makeResult({ + success: true, + prUrl: 'https://github.com/o/r/pull/1', + prEvidence: { + source: 'native-tool-sidecar', + authoritative: true, + command: 'cascade-tools scm create-pr', + }, + }); + const engine = makeEngine(); + const input = makeInput(); + + postProcessResult(result, 'implementation', engine, input, 'impl-id', { + requiresPR: true, + }); + + expect(captureSpy).not.toHaveBeenCalled(); + }); + it('passes through when requiresPR agent already failed', () => { const result = makeResult({ success: false, error: 'Budget exceeded' }); const engine = makeEngine(); diff --git a/tests/unit/backends/sidecarManager.test.ts b/tests/unit/backends/sidecarManager.test.ts index 2bd4ac5f9..8a87e1216 100644 --- a/tests/unit/backends/sidecarManager.test.ts +++ b/tests/unit/backends/sidecarManager.test.ts @@ -340,6 +340,103 @@ describe('hydratePrSidecar', () => { expect(result).toEqual({}); expect(mockRecordPRCreation).not.toHaveBeenCalled(); }); + + // Prod regression 2026-05-09 (run d8e31665): the WARN "PR sidecar missing + // required fields" surfaced no signal of WHY the file was bad. Operators + // hit Loki archaeology to figure out whether the file was empty, partial, + // or had a malformed prUrl. The hydration WARN now dumps the parsed keys + // + the actual prUrl/prNumber values + the file size so a single log line + // gives a precise cause-of-failure entry. + it('logs sidecar contents diagnostic when prUrl is missing (regression net)', async () => { + const { logger } = await import('../../../src/utils/logging.js'); + const warnSpy = vi.mocked(logger.warn); + warnSpy.mockClear(); + + const sidecarPath = makeSidecarPath('pr-diagnostic'); + writeFileSync( + sidecarPath, + JSON.stringify({ + source: 'cascade-tools scm create-pr', + prNumber: 1290, + alreadyExisted: true, + repoFullName: 'mongrel-intelligence/cascade', + // note: prUrl deliberately absent (the d8e31665 failure shape) + }), + ); + + const result = await hydratePrSidecar(sidecarPath); + + expect(result).toEqual({}); + // The WARN now carries enough context to debug from a single log line. + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('PR sidecar missing required fields'), + expect.objectContaining({ + sidecarPath, + hasPrUrl: false, + // Explicit 'parsed' status confirms the file was valid JSON — operator + // knows prUrl was simply absent, not that the file was truncated or missing. + rawSidecarStatus: 'parsed', + rawSidecarKeys: expect.arrayContaining(['source', 'prNumber', 'repoFullName']), + rawSidecarPrUrl: null, + rawSidecarPrNumber: 1290, + }), + ); + }); + + it('logs sidecar diagnostic when file is empty (truncated mid-write)', async () => { + const { logger } = await import('../../../src/utils/logging.js'); + const warnSpy = vi.mocked(logger.warn); + warnSpy.mockClear(); + + const sidecarPath = makeSidecarPath('pr-empty'); + writeFileSync(sidecarPath, ''); // empty file → JSON.parse throws → undefined + + const result = await hydratePrSidecar(sidecarPath); + + expect(result).toEqual({}); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('PR sidecar missing required fields'), + expect.objectContaining({ + sidecarPath, + hasPrUrl: false, + // 'empty' is distinguishable from 'malformed' — tells operator the file + // existed but had zero content (race/truncation), not a JSON syntax error. + rawSidecarStatus: 'empty', + rawSidecarKeys: null, + rawByteLength: 0, + }), + ); + }); + + it('logs malformed status with parseError and rawPreview when JSON is truncated mid-write', async () => { + const { logger } = await import('../../../src/utils/logging.js'); + const warnSpy = vi.mocked(logger.warn); + warnSpy.mockClear(); + + const sidecarPath = makeSidecarPath('pr-malformed'); + // Simulate a partial write: the file has content but JSON is incomplete. + const truncatedJson = '{"prUrl": "https://github.com/o/r/pull/1", "prNum'; + writeFileSync(sidecarPath, truncatedJson); + + const result = await hydratePrSidecar(sidecarPath); + + expect(result).toEqual({}); + // 'malformed' is distinguishable from 'empty' (has rawByteLength > 0, + // parseError, rawPreview) and from 'parsed' (no rawSidecarKeys). + // Operators can read the rawPreview to see how far the write got. + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('PR sidecar missing required fields'), + expect.objectContaining({ + sidecarPath, + hasPrUrl: false, + rawSidecarStatus: 'malformed', + rawSidecarKeys: null, + rawByteLength: truncatedJson.length, + parseError: expect.stringContaining('SyntaxError'), + rawPreview: expect.stringContaining('prUrl'), + }), + ); + }); }); describe('hydrateNativeToolSidecars', () => { diff --git a/tests/unit/gadgets/github/core/createPR.test.ts b/tests/unit/gadgets/github/core/createPR.test.ts index e46b2ee66..60d543391 100644 --- a/tests/unit/gadgets/github/core/createPR.test.ts +++ b/tests/unit/gadgets/github/core/createPR.test.ts @@ -599,6 +599,175 @@ describe('captured hook output preservation (spec 013)', () => { expect(result.commitOutput).toBeDefined(); expect(result.commitOutput).toContain('Pre-commit hook ran: biome OK'); }); + + // Prod regression 2026-05-09 (run d8e31665, cascade/fe82YUKV): a successful + // `cascade-tools scm create-pr` returned 97 KB of `pushOutput` (lefthook's + // pre-push test:fast suite — 159 files, 2981 tests captured into the gadget + // result). Codex's tool-result parser couldn't extract the JSON envelope + // buried under that volume, retried the call, and the resulting concurrent + // invocations raced against the same sidecar path leaving prUrl missing. + // The full hook output stays in the worker's engine log (LLMIST_LOG_FILE) + // for operator visibility — only the agent-visible result-stream copy is + // truncated. + describe('hook-output truncation (prod regression d8e31665)', () => { + it('truncates a 97 KB pushOutput to ~4 KB with a clear marker', async () => { + const huge = 'A'.repeat(97 * 1024); // mimic the cascade prod payload + mockRunCommand.mockImplementation(async (_cmd, args) => { + if (args?.[0] === 'remote') return { stdout: HTTPS_URL, stderr: '', exitCode: 0 }; + if (args?.[0] === 'push') return { stdout: huge, stderr: '', exitCode: 0 }; + if (args?.[0] === 'ls-remote') + return { stdout: 'abc\trefs/heads/feat', stderr: '', exitCode: 0 }; + return { stdout: '', stderr: '', exitCode: 0 }; + }); + mockGithub.createPR.mockResolvedValue({ + number: 1, + htmlUrl: 'https://github.com/test-owner/test-repo/pull/1', + } as Awaited>); + + const result = await createPR({ + title: 'Test', + body: 'Body', + head: 'feat', + base: 'main', + commit: false, + push: true, + }); + + // Result shape unchanged; pushOutput just smaller. + expect(result.prNumber).toBe(1); + expect(result.prUrl).toBe('https://github.com/test-owner/test-repo/pull/1'); + expect(result.pushOutput).toBeDefined(); + // Capped well under the original 97 KB. + expect(result.pushOutput!.length).toBeLessThan(10 * 1024); + // Marker tells operators where to find the full output. + expect(result.pushOutput).toMatch(/bytes truncated from push/i); + // Both ends of the output are preserved (head + tail) for context. + expect(result.pushOutput).toMatch(/^A+/); + expect(result.pushOutput).toMatch(/A+$/); + }); + + it('truncates an oversized commitOutput the same way', async () => { + const huge = 'C'.repeat(50 * 1024); + mockRunCommand.mockImplementation(async (_cmd, args) => { + if (args?.[0] === 'remote') return { stdout: HTTPS_URL, stderr: '', exitCode: 0 }; + if (args?.[0] === 'status') return { stdout: 'M foo.ts\n', stderr: '', exitCode: 0 }; + if (args?.[0] === 'commit') return { stdout: huge, stderr: '', exitCode: 0 }; + if (args?.[0] === 'ls-remote') + return { stdout: 'abc\trefs/heads/feat', stderr: '', exitCode: 0 }; + return { stdout: '', stderr: '', exitCode: 0 }; + }); + mockGithub.createPR.mockResolvedValue({ + number: 1, + htmlUrl: 'https://github.com/test-owner/test-repo/pull/1', + } as Awaited>); + + const result = await createPR({ + title: 'Test', + body: 'Body', + head: 'feat', + base: 'main', + commit: true, + push: false, + }); + + expect(result.commitOutput).toBeDefined(); + expect(result.commitOutput!.length).toBeLessThan(10 * 1024); + expect(result.commitOutput).toMatch(/bytes truncated from commit/i); + }); + + // Reviewer feedback (PR #1292): failing hooks embed the full captured + // output in err.message, which createCLICommand serialises into the JSON + // error envelope. A pre-push hook that fails after printing 97 KB of test + // output still hits the parser/retry bloat path unless the error message + // itself is truncated. Verify both commit and push failure paths. + it('truncates error message when pre-push hook fails with large output', async () => { + const huge = 'E'.repeat(50 * 1024); // 50 KB of hook failure output + mockRunCommand.mockImplementation(async (_cmd, args) => { + if (args?.[0] === 'remote') return { stdout: HTTPS_URL, stderr: '', exitCode: 0 }; + if (args?.[0] === 'push') return { stdout: huge, stderr: 'hook failed', exitCode: 1 }; + return { stdout: '', stderr: '', exitCode: 0 }; + }); + + let thrownError: Error | undefined; + try { + await createPR({ + title: 'Test', + body: 'Body', + head: 'feat', + base: 'main', + commit: false, + push: true, + }); + } catch (e) { + thrownError = e as Error; + } + expect(thrownError).toBeDefined(); + expect(thrownError?.message).toMatch(/PUSH FAILED/); + // Error message must be capped — same 4 KB limit as the success path. + expect((thrownError?.message ?? '').length).toBeLessThan(10 * 1024); + expect(thrownError?.message).toMatch(/bytes truncated from push/i); + }); + + it('truncates error message when pre-commit hook fails with large output', async () => { + const huge = 'F'.repeat(50 * 1024); // 50 KB of hook failure output + mockRunCommand.mockImplementation(async (_cmd, args) => { + if (args?.[0] === 'remote') return { stdout: HTTPS_URL, stderr: '', exitCode: 0 }; + if (args?.[0] === 'status') return { stdout: 'M foo.ts\n', stderr: '', exitCode: 0 }; + if (args?.[0] === 'commit') + return { stdout: huge, stderr: 'pre-commit hook failed', exitCode: 1 }; + return { stdout: '', stderr: '', exitCode: 0 }; + }); + + let thrownError: Error | undefined; + try { + await createPR({ + title: 'Test', + body: 'Body', + head: 'feat', + base: 'main', + commit: true, + push: false, + }); + } catch (e) { + thrownError = e as Error; + } + expect(thrownError).toBeDefined(); + expect(thrownError?.message).toMatch(/COMMIT FAILED/); + expect((thrownError?.message ?? '').length).toBeLessThan(10 * 1024); + expect(thrownError?.message).toMatch(/bytes truncated from commit/i); + }); + + it('leaves small hook output untouched (no truncation marker)', async () => { + mockRunCommand.mockImplementation(async (_cmd, args) => { + if (args?.[0] === 'remote') return { stdout: HTTPS_URL, stderr: '', exitCode: 0 }; + if (args?.[0] === 'push') + return { + stdout: 'Pre-push hook ran: typecheck OK\n', + stderr: 'To github.com...\n', + exitCode: 0, + }; + if (args?.[0] === 'ls-remote') + return { stdout: 'abc\trefs/heads/feat', stderr: '', exitCode: 0 }; + return { stdout: '', stderr: '', exitCode: 0 }; + }); + mockGithub.createPR.mockResolvedValue({ + number: 1, + htmlUrl: 'https://github.com/test-owner/test-repo/pull/1', + } as Awaited>); + + const result = await createPR({ + title: 'Test', + body: 'Body', + head: 'feat', + base: 'main', + commit: false, + push: true, + }); + + expect(result.pushOutput).toContain('Pre-push hook ran: typecheck OK'); + expect(result.pushOutput).not.toMatch(/bytes truncated/); + }); + }); }); // ─────────────────────────────────────────────────────────────────────────── diff --git a/tests/unit/gadgets/session/core/sidecar.test.ts b/tests/unit/gadgets/session/core/sidecar.test.ts index 532f3df8e..9aa224b3c 100644 --- a/tests/unit/gadgets/session/core/sidecar.test.ts +++ b/tests/unit/gadgets/session/core/sidecar.test.ts @@ -196,4 +196,58 @@ describe('writePRSidecar', () => { expect.stringContaining('Failed to write'), ); }); + + // Prod regression 2026-05-09 (run d8e31665): the sidecar contract is + // authoritative evidence of PR creation. JSON.stringify silently omits keys + // whose value is `undefined`, so a half-baked call leaves an unusable file + // AND the worker reports it as "missing required fields" with no signal of + // what the caller actually passed. Validate inputs at the write boundary + // and refuse the write loudly so future failures are diagnosable. + it('refuses to write when prUrl is empty (logs ERROR with context)', () => { + expect(writePRSidecar(sidecarPath, '', 42, false, 'owner/repo')).toBe(false); + expect(existsSync(sidecarPath)).toBe(false); + expect(vi.mocked(logger.error)).toHaveBeenCalledWith( + expect.stringContaining('refusing to write'), + expect.objectContaining({ sidecarPath, prUrl: '', prNumber: 42 }), + ); + }); + + it('refuses to write when prUrl is non-string (defensive, JS-runtime)', () => { + // Simulate a bug where the caller passes undefined despite the type. + expect( + writePRSidecar(sidecarPath, undefined as unknown as string, 42, false, 'owner/repo'), + ).toBe(false); + expect(existsSync(sidecarPath)).toBe(false); + expect(vi.mocked(logger.error)).toHaveBeenCalled(); + }); + + it('refuses to write when prNumber is not finite', () => { + expect(writePRSidecar(sidecarPath, 'https://x', NaN, false, 'o/r')).toBe(false); + expect(existsSync(sidecarPath)).toBe(false); + expect(vi.mocked(logger.error)).toHaveBeenCalledWith( + expect.stringContaining('prNumber'), + expect.any(Object), + ); + }); +}); + +describe('writeReviewSidecar — input validation (prod regression d8e31665)', () => { + let sidecarPath: string; + + beforeEach(() => { + sidecarPath = join(tmpdir(), `cascade-test-review-validation-${Date.now()}.json`); + }); + + afterEach(() => { + rmSync(sidecarPath, { force: true }); + }); + + it('refuses to write when reviewUrl is empty', () => { + expect(writeReviewSidecar(sidecarPath, '', 'APPROVE', 'lgtm')).toBe(false); + expect(existsSync(sidecarPath)).toBe(false); + expect(vi.mocked(logger.error)).toHaveBeenCalledWith( + expect.stringContaining('refusing to write'), + expect.any(Object), + ); + }); }); diff --git a/tests/unit/gadgets/shared/cli/booleanArgv.test.ts b/tests/unit/gadgets/shared/cli/booleanArgv.test.ts new file mode 100644 index 000000000..33361518c --- /dev/null +++ b/tests/unit/gadgets/shared/cli/booleanArgv.test.ts @@ -0,0 +1,55 @@ +import { Writable } from 'node:stream'; +import { describe, expect, it, vi } from 'vitest'; + +import { massageBooleanFlagValues } from '../../../../../src/gadgets/shared/cli/booleanArgv.js'; +import type { ErrorSink } from '../../../../../src/gadgets/shared/cli/errorSink.js'; + +function makeSink(): ErrorSink { + return { + stdout: new Writable({ + write(_chunk, _enc, cb) { + cb(); + }, + }), + stderr: new Writable({ + write(_chunk, _enc, cb) { + cb(); + }, + }), + exit: vi.fn<(code: number) => never>(() => { + throw new Error('exit'); + }), + }; +} + +describe('CLI boolean argv normalization', () => { + it('rewrites natural boolean value forms to oclif toggles', () => { + const flags = new Map([ + ['enabled', true], + ['draft', false], + ]); + expect( + massageBooleanFlagValues( + ['--enabled', 'true', '--enabled=false', '--draft', 'false', '--draft=true'], + flags, + makeSink(), + ), + ).toEqual(['--enabled', '--no-enabled', '--draft']); + }); + + it('leaves a bare toggle untouched when the next token is another flag', () => { + expect( + massageBooleanFlagValues( + ['--enabled', '--name', 'x'], + new Map([['enabled', true]]), + makeSink(), + ), + ).toEqual(['--enabled', '--name', 'x']); + }); + + it('emits through the sink on malformed boolean values', () => { + expect(() => + massageBooleanFlagValues(['--enabled', 'banana'], new Map([['enabled', true]]), makeSink()), + ).toThrow('exit'); + }); +}); diff --git a/tests/unit/gadgets/shared/cli/commandNames.test.ts b/tests/unit/gadgets/shared/cli/commandNames.test.ts new file mode 100644 index 000000000..dbab59bc4 --- /dev/null +++ b/tests/unit/gadgets/shared/cli/commandNames.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from 'vitest'; + +import { + deriveCLICommand, + toKebabCase, +} from '../../../../../src/gadgets/shared/cli/commandNames.js'; + +describe('CLI command names', () => { + it('converts PascalCase and acronym-heavy names to kebab case', () => { + expect(toKebabCase('PostComment')).toBe('post-comment'); + expect(toKebabCase('ReadWorkItem')).toBe('read-work-item'); + expect(toKebabCase('CreatePR')).toBe('create-pr'); + expect(toKebabCase('GetCIRunLogs')).toBe('get-ci-run-logs'); + }); + + it('derives PM, SCM, and session command prefixes from tool names', () => { + expect(deriveCLICommand('ReadWorkItem')).toBe('cascade-tools pm read-work-item'); + expect(deriveCLICommand('PMUpdateChecklistItem')).toBe( + 'cascade-tools pm update-checklist-item', + ); + expect(deriveCLICommand('CreatePR')).toBe('cascade-tools scm create-pr'); + expect(deriveCLICommand('GetCIRunLogs')).toBe('cascade-tools scm get-ci-run-logs'); + expect(deriveCLICommand('Finish')).toBe('cascade-tools session finish'); + }); + + it('honors explicit command overrides', () => { + expect(deriveCLICommand('ReadWorkItem', 'cascade-tools custom read')).toBe( + 'cascade-tools custom read', + ); + }); +}); diff --git a/tests/unit/gadgets/shared/cli/examples.test.ts b/tests/unit/gadgets/shared/cli/examples.test.ts new file mode 100644 index 000000000..ad2442b51 --- /dev/null +++ b/tests/unit/gadgets/shared/cli/examples.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from 'vitest'; + +import { + buildOclifExamples, + expectedShapeFor, + findExampleForParam, + shellQuote, +} from '../../../../../src/gadgets/shared/cli/examples.js'; +import type { ToolDefinition } from '../../../../../src/gadgets/shared/toolDefinition.js'; + +const reviewDef: ToolDefinition = { + name: 'CreatePRReview', + description: 'Review a PR', + parameters: { + body: { type: 'string', describe: 'Review body', required: true }, + draft: { type: 'boolean', describe: 'Draft review', optional: true, allowNo: true }, + labels: { type: 'array', items: 'string', describe: 'Labels', optional: true }, + comments: { type: 'array', items: 'object', describe: 'Inline comments', optional: true }, + comment: { type: 'string', describe: 'Internal rationale', gadgetOnly: true }, + }, + examples: [ + { + params: { + body: "Don't merge yet", + draft: false, + labels: ['needs-work', 'api'], + comments: [{ path: 'src/a.ts', line: 3, body: 'nit' }], + comment: 'hidden', + }, + }, + ], +}; + +describe('CLI examples', () => { + it('finds the first defined example value for a parameter', () => { + expect(findExampleForParam(reviewDef.examples, 'comments')).toEqual([ + { path: 'src/a.ts', line: 3, body: 'nit' }, + ]); + expect(findExampleForParam(reviewDef.examples, 'missing')).toBeUndefined(); + }); + + it('shell-quotes embedded single quotes safely', () => { + expect(shellQuote("don't")).toBe("'don'\\''t'"); + }); + + it('renders oclif examples while skipping gadget-only params', () => { + const [line] = buildOclifExamples(reviewDef, 'cascade-tools scm create-pr-review'); + expect(line).toContain("cascade-tools scm create-pr-review --body 'Don'\\''t merge yet'"); + expect(line).toContain('--no-draft'); + expect(line).toContain("--labels 'needs-work' --labels 'api'"); + expect(line).toContain('--comments \'[{"path":"src/a.ts","line":3,"body":"nit"}]\''); + expect(line).not.toContain('hidden'); + }); + + it('uses examples as JSON expected-shape hints with describe fallback', () => { + const paramDef = reviewDef.parameters.comments; + expect(expectedShapeFor(paramDef, [{ path: 'x.ts' }])).toBe('[{"path":"x.ts"}]'); + expect(expectedShapeFor(paramDef)).toBe('Inline comments'); + }); +}); diff --git a/tests/unit/gadgets/shared/cli/flags.test.ts b/tests/unit/gadgets/shared/cli/flags.test.ts new file mode 100644 index 000000000..fb8c20126 --- /dev/null +++ b/tests/unit/gadgets/shared/cli/flags.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from 'vitest'; + +import { + buildFlagsRecord, + collectBooleanFlagNames, + collectCandidateFlags, +} from '../../../../../src/gadgets/shared/cli/flags.js'; +import type { ToolDefinition } from '../../../../../src/gadgets/shared/toolDefinition.js'; + +const def: ToolDefinition = { + name: 'TestTool', + description: 'Test', + parameters: { + comment: { type: 'string', describe: 'Internal', gadgetOnly: true }, + owner: { type: 'string', describe: 'Owner', required: true }, + body: { type: 'string', describe: 'Body', required: true }, + draft: { type: 'boolean', describe: 'Draft', optional: true, allowNo: true }, + labels: { type: 'array', items: 'string', describe: 'Labels', optional: true }, + comments: { + type: 'array', + items: 'object', + describe: 'Inline comments', + optional: true, + cliAliases: ['comment'], + }, + }, + cli: { + autoResolved: [{ paramName: 'owner', resolvedFrom: 'git-remote' }], + fileInputAlternatives: [{ paramName: 'body', fileFlag: 'body-file' }], + }, +}; + +describe('CLI flags', () => { + it('builds flags while preserving optionality for auto-resolved and file-input params', () => { + const flags = buildFlagsRecord(def); + expect(flags.comment).toBeUndefined(); + expect(flags.owner).toBeDefined(); + expect(flags.owner.required).toBeFalsy(); + expect(flags.body.required).toBeFalsy(); + expect(flags['body-file']).toBeDefined(); + expect(flags.draft.allowNo).toBe(true); + expect(flags.labels.multiple).toBe(true); + expect(flags.comments.multiple).toBe(false); + expect(flags.comments.aliases).toEqual(['comment']); + }); + + it('collects fuzzy-match candidates and boolean metadata', () => { + expect(collectCandidateFlags(def)).toEqual([ + { canonical: 'owner', aliases: [] }, + { canonical: 'body', aliases: [] }, + { canonical: 'draft', aliases: [] }, + { canonical: 'labels', aliases: [] }, + { canonical: 'comments', aliases: ['comment'] }, + { canonical: 'body-file', aliases: [] }, + ]); + expect(collectBooleanFlagNames(def)).toEqual(new Map([['draft', true]])); + }); +}); diff --git a/tests/unit/gadgets/shared/cli/params.test.ts b/tests/unit/gadgets/shared/cli/params.test.ts new file mode 100644 index 000000000..288e76a01 --- /dev/null +++ b/tests/unit/gadgets/shared/cli/params.test.ts @@ -0,0 +1,103 @@ +import { mkdtempSync, writeFileSync } from 'node:fs'; +import { rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { Writable } from 'node:stream'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import type { ErrorSink } from '../../../../../src/gadgets/shared/cli/errorSink.js'; +import { resolveDirectParams } from '../../../../../src/gadgets/shared/cli/params.js'; +import type { + CLIAutoResolved, + FileInputAlternative, + ToolDefinition, +} from '../../../../../src/gadgets/shared/toolDefinition.js'; + +let tmpDir = ''; + +function makeSink(): ErrorSink { + return { + stdout: new Writable({ + write(_chunk, _enc, cb) { + cb(); + }, + }), + stderr: new Writable({ + write(_chunk, _enc, cb) { + cb(); + }, + }), + exit: vi.fn<(code: number) => never>(() => { + throw new Error('exit'); + }), + }; +} + +function writeTempFile(name: string, content: string): string { + tmpDir = tmpDir || mkdtempSync(join(tmpdir(), 'cascade-cli-params-test-')); + const path = join(tmpDir, name); + writeFileSync(path, content); + return path; +} + +afterEach(async () => { + await rm(tmpDir, { recursive: true, force: true }); + tmpDir = ''; +}); + +const fileAlt: FileInputAlternative = { + paramName: 'comments', + fileFlag: 'comments-file', + parseAs: 'json', +}; + +const autoResolved: CLIAutoResolved = { paramName: 'owner', resolvedFrom: 'git-remote' }; + +const def: ToolDefinition = { + name: 'TestTool', + description: 'Test', + parameters: { + owner: { type: 'string', describe: 'Owner', required: true }, + body: { type: 'string', describe: 'Body', required: true }, + config: { type: 'object', describe: 'Config', optional: true }, + comments: { type: 'array', items: 'object', describe: 'Comments', required: true }, + comment: { type: 'string', describe: 'Internal', gadgetOnly: true }, + }, + examples: [{ params: { comments: [{ path: 'x.ts' }] } }], +}; + +describe('CLI parameter resolution', () => { + it('prefers JSON file-input alternatives over inline values and skips auto-resolved params', () => { + const filePath = writeTempFile('comments.json', '[{"path":"from-file.ts"}]'); + const params = resolveDirectParams( + def, + { + owner: 'inline-owner', + body: 'hello', + config: '{"ok":true}', + comments: '[{"path":"inline.ts"}]', + 'comments-file': filePath, + }, + new Map([['comments', fileAlt]]), + new Map([['owner', autoResolved]]), + makeSink(), + ); + expect(params).toEqual({ + body: 'hello', + config: { ok: true }, + comments: [{ path: 'from-file.ts' }], + }); + }); + + it('emits missing-required when neither inline nor file value is present', () => { + expect(() => + resolveDirectParams( + def, + { body: 'hello' }, + new Map([['comments', fileAlt]]), + new Map([['owner', autoResolved]]), + makeSink(), + ), + ).toThrow('exit'); + }); +}); diff --git a/tests/unit/gadgets/shared/cli/parseErrors.test.ts b/tests/unit/gadgets/shared/cli/parseErrors.test.ts new file mode 100644 index 000000000..4270b784b --- /dev/null +++ b/tests/unit/gadgets/shared/cli/parseErrors.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, it } from 'vitest'; + +import { + classifyParseError, + isNonexistentFlagError, + suggestFlag, +} from '../../../../../src/gadgets/shared/cli/parseErrors.js'; + +describe('CLI parse errors', () => { + it('suggests the canonical flag when an alias is the closest match', () => { + expect( + suggestFlag('coment', [ + { canonical: 'comments', aliases: ['comment'] }, + { canonical: 'body', aliases: [] }, + ]), + ).toBe('comments'); + expect(suggestFlag('zzzzzzzz', [{ canonical: 'comments', aliases: ['comment'] }])).toBeNull(); + }); + + it('recognizes oclif nonexistent-flag error shapes', () => { + class NonExistentFlagsError extends Error { + public flags = ['coment']; + } + expect(isNonexistentFlagError(new NonExistentFlagsError('bad'))).toBe(true); + expect(isNonexistentFlagError(new Error('bad'))).toBe(false); + }); + + it('classifies required, enum, and positional parse failures', () => { + class FailedFlagValidationError extends Error {} + class FlagInvalidOptionError extends Error {} + class UnexpectedArgsError extends Error {} + + expect(classifyParseError(new FailedFlagValidationError('Missing required flag body'))).toEqual( + expect.objectContaining({ type: 'missing-required', flag: 'body' }), + ); + expect( + classifyParseError( + new FlagInvalidOptionError('Expected --state=bad to be one of: open, closed'), + ), + ).toEqual(expect.objectContaining({ type: 'enum-mismatch', flag: 'state', got: 'bad' })); + expect(classifyParseError(new UnexpectedArgsError('Unexpected argument: banana'))).toEqual( + expect.objectContaining({ type: 'flag-parse', got: 'banana' }), + ); + }); +}); diff --git a/tests/unit/router/adapters/sentry.test.ts b/tests/unit/router/adapters/sentry.test.ts index 9ffd17e25..3883b0cb2 100644 --- a/tests/unit/router/adapters/sentry.test.ts +++ b/tests/unit/router/adapters/sentry.test.ts @@ -124,17 +124,27 @@ describe('SentryRouterAdapter', () => { expect(mockLogger.warn).toHaveBeenCalled(); }); - it('returns null for non-processable resource type "issue"', async () => { + it('accepts "issue" resource (Sentry-Hook-Resource: issue, Internal Integration surface)', async () => { + // Updated 2026-05-09: SentryIssueLifecycleTrigger now handles + // `issue` resource webhooks (Internal Integration default surface — + // fires on issue lifecycle events like `created`). Previously this + // resource was dropped at parse time which silently skipped the + // alerting agent for any project that wired Sentry the natural way. const payload = { resource: 'issue', - payload: { action: 'created', data: {} }, + payload: { action: 'created', data: { issue: { id: '118723355' } } }, cascadeProjectId: 'p1', }; const result = await adapter.parseWebhook(payload); - expect(result).toBeNull(); - expect(mockLogger.debug).toHaveBeenCalled(); + expect(result).not.toBeNull(); + expect(result).toMatchObject({ + projectIdentifier: 'p1', + eventType: 'issue', + cascadeProjectId: 'p1', + resource: 'issue', + }); }); it('returns null for non-processable resource type "error"', async () => { @@ -171,14 +181,14 @@ describe('SentryRouterAdapter', () => { expect(adapter.isProcessableEvent(event)).toBe(true); }); - it('returns false for "issue" resource', () => { + it('returns true for "issue" resource (lifecycle webhook surface)', () => { const event = { projectIdentifier: 'p1', eventType: 'issue', isCommentEvent: false, }; - expect(adapter.isProcessableEvent(event)).toBe(false); + expect(adapter.isProcessableEvent(event)).toBe(true); }); it('returns false for unknown event type', () => { diff --git a/tests/unit/triggers/builtins.test.ts b/tests/unit/triggers/builtins.test.ts index 1d7a37fd1..ae77a6f48 100644 --- a/tests/unit/triggers/builtins.test.ts +++ b/tests/unit/triggers/builtins.test.ts @@ -139,8 +139,10 @@ describe('registerBuiltInTriggers', () => { registerBuiltInTriggers(registry as unknown as TriggerRegistry); - // Should have registered all 24 built-in triggers (19 + 2 Sentry alerting + 3 Linear triggers) - expect(registry.register).toHaveBeenCalledTimes(24); + // Should have registered all 25 built-in triggers (19 + 3 Sentry alerting + 3 Linear triggers). + // Sentry triggers: SentryIssueAlertTrigger (event_alert), SentryMetricAlertTrigger + // (metric_alert), SentryIssueLifecycleTrigger (issue lifecycle webhook). + expect(registry.register).toHaveBeenCalledTimes(25); }); it('registers TrelloCommentMentionTrigger first', () => { diff --git a/tests/unit/triggers/sentry-webhook-handler.test.ts b/tests/unit/triggers/sentry-webhook-handler.test.ts index 76c785ce7..5c06119f7 100644 --- a/tests/unit/triggers/sentry-webhook-handler.test.ts +++ b/tests/unit/triggers/sentry-webhook-handler.test.ts @@ -40,11 +40,16 @@ vi.mock('../../../src/integrations/alerting/_shared/format.js', () => ({ title: '[Sentry Metric] Error Rate High', descriptionMarkdown: 'metric desc', }), + formatSentryIssueLifecycleCardBody: vi.fn().mockReturnValue({ + title: '[Sentry] wedged work-item lock', + descriptionMarkdown: 'issue lifecycle desc', + }), })); import { loadProjectConfigById } from '../../../src/config/provider.js'; import { formatSentryCardBody, + formatSentryIssueLifecycleCardBody, formatSentryMetricCardBody, } from '../../../src/integrations/alerting/_shared/format.js'; import { materializeAlertWorkItem } from '../../../src/integrations/alerting/_shared/materialize.js'; @@ -85,6 +90,10 @@ describe('processSentryWebhook', () => { title: '[Sentry Metric] Error Rate High', descriptionMarkdown: 'metric desc', }); + vi.mocked(formatSentryIssueLifecycleCardBody).mockReturnValue({ + title: '[Sentry] wedged work-item lock', + descriptionMarkdown: 'issue lifecycle desc', + }); }); it('loads project config by projectId and calls resolveTriggerResult with sentry source', async () => { @@ -397,4 +406,139 @@ describe('processSentryWebhook', () => { expect(runAgentExecutionPipeline).not.toHaveBeenCalled(); }); + + // ── Issue-lifecycle PM card materialisation ────────────────────────────── + // + // Sentry-Hook-Resource: issue surface (Internal Integration default). + // The issue-lifecycle handler also passes `alertIssueId`, so the + // dispatcher discriminates on `agentInput.triggerEvent === 'alerting:issue-lifecycle'` + // to pick the lifecycle format helper + 'sentry-issue' AlertSource. + + it('materialises a PM work item for issue-lifecycle when triggerEvent is alerting:issue-lifecycle', async () => { + const payload = { resource: 'issue', cascadeProjectId: 'proj-sentry' }; + const triggerResult = { + agentType: 'alerting', + agentInput: { + triggerEvent: 'alerting:issue-lifecycle', + alertIssueId: '118723355', + }, + lockKey: 'sentry-issue:118723355', + } as never; + vi.mocked(resolveTriggerResult).mockResolvedValue(triggerResult); + vi.mocked(materializeAlertWorkItem).mockResolvedValue('issue-card-1'); + + await processSentryWebhook(payload, 'proj-sentry', mockRegistry as never); + + expect(materializeAlertWorkItem).toHaveBeenCalledWith( + 'sentry-issue', + '118723355', + mockProject, + expect.objectContaining({ title: '[Sentry] wedged work-item lock' }), + ); + expect(formatSentryIssueLifecycleCardBody).toHaveBeenCalledWith(payload); + // Existing event_alert formatter is NOT invoked for the lifecycle branch. + expect(formatSentryCardBody).not.toHaveBeenCalled(); + expect(runAgentExecutionPipeline).toHaveBeenCalledWith( + expect.objectContaining({ + workItemId: 'issue-card-1', + agentInput: expect.objectContaining({ workItemId: 'issue-card-1' }), + }), + mockProject, + expect.any(Object), + expect.objectContaining({ logLabel: 'Sentry agent' }), + ); + }); + + it('uses event_alert path (formatSentryCardBody + source=sentry) when triggerEvent is alerting:issue-alert', async () => { + // Regression net: the existing event_alert flow must keep using + // `formatSentryCardBody` and `'sentry'` AlertSource even though both + // surfaces pass `alertIssueId`. + const payload = { resource: 'event_alert', cascadeProjectId: 'proj-sentry' }; + const triggerResult = { + agentType: 'alerting', + agentInput: { + triggerEvent: 'alerting:issue-alert', + alertIssueId: 'sentry-issue-42', + }, + } as never; + vi.mocked(resolveTriggerResult).mockResolvedValue(triggerResult); + vi.mocked(materializeAlertWorkItem).mockResolvedValue('card-new'); + + await processSentryWebhook(payload, 'proj-sentry', mockRegistry as never); + + expect(materializeAlertWorkItem).toHaveBeenCalledWith( + 'sentry', + 'sentry-issue-42', + mockProject, + expect.objectContaining({ title: '[Sentry] Test' }), + ); + expect(formatSentryCardBody).toHaveBeenCalledWith(payload); + expect(formatSentryIssueLifecycleCardBody).not.toHaveBeenCalled(); + }); + + it('skips issue-lifecycle materialisation when workItemId is already set', async () => { + const payload = { resource: 'issue' }; + const triggerResult = { + agentType: 'alerting', + workItemId: 'issue-card-already', + agentInput: { + triggerEvent: 'alerting:issue-lifecycle', + alertIssueId: '118723355', + workItemId: 'issue-card-already', + }, + } as never; + vi.mocked(resolveTriggerResult).mockResolvedValue(triggerResult); + + await processSentryWebhook(payload, 'proj-sentry', mockRegistry as never); + + expect(materializeAlertWorkItem).not.toHaveBeenCalled(); + expect(runAgentExecutionPipeline).toHaveBeenCalledWith( + triggerResult, + mockProject, + expect.any(Object), + expect.any(Object), + ); + }); + + it('skips agent and warns when issue-lifecycle materialisation throws AlertSlotMissingError', async () => { + const payload = { resource: 'issue' }; + const triggerResult = { + agentType: 'alerting', + agentInput: { + triggerEvent: 'alerting:issue-lifecycle', + alertIssueId: '118723355', + }, + } as never; + vi.mocked(resolveTriggerResult).mockResolvedValue(triggerResult); + vi.mocked(materializeAlertWorkItem).mockRejectedValue( + new AlertSlotMissingError('proj-sentry', 'trello'), + ); + + await processSentryWebhook(payload, 'proj-sentry', mockRegistry as never); + + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('alerts slot no longer configured'), + expect.objectContaining({ reason: 'alerts_slot_missing' }), + ); + expect(runAgentExecutionPipeline).not.toHaveBeenCalled(); + }); + + it('re-throws transient PM errors for issue-lifecycle so BullMQ can retry', async () => { + const payload = { resource: 'issue' }; + const triggerResult = { + agentType: 'alerting', + agentInput: { + triggerEvent: 'alerting:issue-lifecycle', + alertIssueId: '118723355', + }, + } as never; + vi.mocked(resolveTriggerResult).mockResolvedValue(triggerResult); + vi.mocked(materializeAlertWorkItem).mockRejectedValue(new Error('PM 503')); + + await expect( + processSentryWebhook(payload, 'proj-sentry', mockRegistry as never), + ).rejects.toThrow('PM 503'); + + expect(runAgentExecutionPipeline).not.toHaveBeenCalled(); + }); }); diff --git a/tests/unit/triggers/sentry/issue-lifecycle-format.test.ts b/tests/unit/triggers/sentry/issue-lifecycle-format.test.ts new file mode 100644 index 000000000..30c126e4f --- /dev/null +++ b/tests/unit/triggers/sentry/issue-lifecycle-format.test.ts @@ -0,0 +1,104 @@ +/** + * Tests for `formatSentryIssueLifecycleCardBody`, the format helper for the + * `Sentry-Hook-Resource: issue` webhook surface (Internal Integration default). + * + * Mirrors the shape of `formatSentryCardBody` (event_alert) but pulls fields + * from `data.issue.{id, title, web_url, level, shortId, culprit, metadata}` + * instead of `data.event.{...}`. Captured shape from prod webhook id + * `fbdc6d87-b962-444c-8a2a-a9452a74ff71` (2026-05-09 13:18:51 UTC). + */ + +import { describe, expect, it } from 'vitest'; + +import { formatSentryIssueLifecycleCardBody } from '../../../../src/integrations/alerting/_shared/format.js'; +import type { SentryAugmentedPayload, SentryIssuePayload } from '../../../../src/sentry/types.js'; + +function makeAugmented( + overrides: Partial = {}, +): SentryAugmentedPayload { + const issue: SentryIssuePayload['data']['issue'] = { + id: '118723355', + title: + 'Error: wedged work-item lock: projectId=ucho workItemId=MNG-598 agentType=backlog-manager', + culprit: 'POST /github/webhook', + shortId: 'CASCADE-2T', + level: 'error', + issueType: 'error', + web_url: 'https://mongrel.sentry.io/issues/118723355/', + permalink: 'https://mongrel.sentry.io/issues/118723355/', + metadata: { + type: 'Error', + value: 'wedged work-item lock: projectId=ucho workItemId=MNG-598 agentType=backlog-manager', + filename: '/app/dist/router/webhook-dispatch-locks.js', + function: 'checkDispatchLocks', + }, + platform: 'node', + priority: 'high', + firstSeen: '2026-05-09T13:18:37.078000+00:00', + ...overrides, + }; + return { + resource: 'issue', + cascadeProjectId: 'cascade', + payload: { + action: 'created', + actor: { id: 'sentry', name: 'Sentry', type: 'application' }, + data: { issue }, + } as SentryIssuePayload, + }; +} + +describe('formatSentryIssueLifecycleCardBody', () => { + it('returns the prod-fixture title prefixed with [Sentry]', () => { + const hints = formatSentryIssueLifecycleCardBody(makeAugmented()); + expect(hints.title.startsWith('[Sentry] ')).toBe(true); + expect(hints.title).toContain('wedged work-item lock'); + }); + + it('includes the web_url, level, short ID, and culprit in the description', () => { + const hints = formatSentryIssueLifecycleCardBody(makeAugmented()); + expect(hints.descriptionMarkdown).toContain('https://mongrel.sentry.io/issues/118723355/'); + expect(hints.descriptionMarkdown).toContain('error'); // level + expect(hints.descriptionMarkdown).toContain('CASCADE-2T'); // shortId + expect(hints.descriptionMarkdown).toContain('POST /github/webhook'); // culprit + }); + + it('includes the top frame from metadata (filename:function)', () => { + const hints = formatSentryIssueLifecycleCardBody(makeAugmented()); + expect(hints.descriptionMarkdown).toContain('/app/dist/router/webhook-dispatch-locks.js'); + expect(hints.descriptionMarkdown).toContain('checkDispatchLocks'); + }); + + it('omits absent fields without leaving "undefined" placeholders', () => { + const hints = formatSentryIssueLifecycleCardBody( + makeAugmented({ + culprit: undefined, + shortId: undefined, + level: undefined, + metadata: undefined, + }), + ); + expect(hints.descriptionMarkdown).not.toContain('undefined'); + expect(hints.descriptionMarkdown).not.toMatch(/Top frame:.*undefined/); + expect(hints.descriptionMarkdown).not.toMatch(/Level:.*undefined/); + expect(hints.descriptionMarkdown).not.toMatch(/Short ID:.*undefined/); + expect(hints.descriptionMarkdown).not.toMatch(/Culprit:.*undefined/); + }); + + it('falls back to permalink when web_url is missing', () => { + const hints = formatSentryIssueLifecycleCardBody(makeAugmented({ web_url: undefined })); + expect(hints.descriptionMarkdown).toContain('https://mongrel.sentry.io/issues/118723355/'); + }); + + it('uses a default title when the issue has none', () => { + const hints = formatSentryIssueLifecycleCardBody(makeAugmented({ title: '' })); + expect(hints.title).toBe('[Sentry] Sentry Issue'); + }); + + it('returns a stable AlertHints shape', () => { + const hints = formatSentryIssueLifecycleCardBody(makeAugmented()); + expect(Object.keys(hints).sort()).toEqual(['descriptionMarkdown', 'title']); + expect(typeof hints.title).toBe('string'); + expect(typeof hints.descriptionMarkdown).toBe('string'); + }); +}); diff --git a/tests/unit/triggers/sentry/issue-lifecycle.test.ts b/tests/unit/triggers/sentry/issue-lifecycle.test.ts new file mode 100644 index 000000000..f14a5d708 --- /dev/null +++ b/tests/unit/triggers/sentry/issue-lifecycle.test.ts @@ -0,0 +1,238 @@ +/** + * Tests for `SentryIssueLifecycleTrigger` — the handler for the + * `Sentry-Hook-Resource: issue` surface. Mirrors the shape of + * `SentryIssueAlertTrigger` (event_alert) but matches on `resource: 'issue'` + * and the `'created'` lifecycle action. + * + * Captured shape from prod webhook id `fbdc6d87-b962-444c-8a2a-a9452a74ff71` + * (2026-05-09 13:18:51 UTC). + */ + +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { mockLogger, mockTriggerCheckModule } from '../../../helpers/sharedMocks.js'; + +vi.mock('../../../../src/utils/logging.js', () => ({ logger: mockLogger })); +vi.mock('../../../../src/triggers/shared/trigger-check.js', () => mockTriggerCheckModule); + +vi.mock('../../../../src/sentry/integration.js', () => ({ + getSentryIntegrationConfig: vi.fn(), +})); + +vi.mock('../../../../src/pm/config.js', () => ({ + getAlertsContainerId: vi.fn(), +})); + +import { getAlertsContainerId } from '../../../../src/pm/config.js'; +import { getSentryIntegrationConfig } from '../../../../src/sentry/integration.js'; +import { SentryIssueLifecycleTrigger } from '../../../../src/triggers/sentry/alerting-issue-lifecycle.js'; +import { checkTriggerEnabledWithParams } from '../../../../src/triggers/shared/trigger-check.js'; +import type { TriggerContext } from '../../../../src/types/index.js'; +import { createMockProject } from '../../../helpers/factories.js'; + +const mockProject = createMockProject({ + trello: { + boardId: 'board123', + lists: { + splitting: 'split-list', + planning: 'plan-list', + todo: 'todo-list', + alerts: 'alerts-list', + }, + labels: {}, + }, +}); + +const sentryConfig = { organizationSlug: 'mongrel' }; + +function makeIssueLifecycleCtx( + overrides: { + resource?: string; + action?: string; + issueOverrides?: Partial<{ + id: string; + title: string; + web_url: string; + permalink: string; + level: string; + shortId: string; + culprit: string; + }>; + } = {}, +): TriggerContext { + const issue = { + id: '118723355', + title: + 'Error: wedged work-item lock: projectId=ucho workItemId=MNG-598 agentType=backlog-manager', + web_url: 'https://mongrel.sentry.io/issues/118723355/', + permalink: 'https://mongrel.sentry.io/issues/118723355/', + shortId: 'CASCADE-2T', + level: 'error', + culprit: 'POST /github/webhook', + metadata: { + type: 'Error', + filename: '/app/dist/router/webhook-dispatch-locks.js', + function: 'checkDispatchLocks', + }, + firstSeen: '2026-05-09T13:18:37.078000+00:00', + ...(overrides.issueOverrides ?? {}), + }; + return { + project: mockProject, + source: 'sentry', + payload: { + resource: overrides.resource ?? 'issue', + payload: { + action: overrides.action ?? 'created', + actor: { id: 'sentry', name: 'Sentry', type: 'application' }, + data: { issue }, + }, + cascadeProjectId: 'test', + }, + } as TriggerContext; +} + +describe('SentryIssueLifecycleTrigger', () => { + let trigger: SentryIssueLifecycleTrigger; + + beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(checkTriggerEnabledWithParams).mockResolvedValue({ enabled: true, parameters: {} }); + vi.mocked(getSentryIntegrationConfig).mockResolvedValue(sentryConfig); + vi.mocked(getAlertsContainerId).mockReturnValue('alerts-list'); + trigger = new SentryIssueLifecycleTrigger(); + }); + + // ------------------------------------------------------------------------- + // matches() + // ------------------------------------------------------------------------- + + describe('matches()', () => { + it('returns true for sentry source with resource=issue + action=created', () => { + expect(trigger.matches(makeIssueLifecycleCtx())).toBe(true); + }); + + it('returns false for non-sentry source', () => { + const ctx = { ...makeIssueLifecycleCtx(), source: 'github' } as TriggerContext; + expect(trigger.matches(ctx)).toBe(false); + }); + + it('returns false when resource is event_alert (the SentryIssueAlertTrigger surface)', () => { + expect(trigger.matches(makeIssueLifecycleCtx({ resource: 'event_alert' }))).toBe(false); + }); + + it('returns false when resource is metric_alert', () => { + expect(trigger.matches(makeIssueLifecycleCtx({ resource: 'metric_alert' }))).toBe(false); + }); + + it('returns false for resolved action (lifecycle event we do not yet handle)', () => { + expect(trigger.matches(makeIssueLifecycleCtx({ action: 'resolved' }))).toBe(false); + }); + + it('returns false for archived action', () => { + expect(trigger.matches(makeIssueLifecycleCtx({ action: 'archived' }))).toBe(false); + }); + + it('returns false for assigned action', () => { + expect(trigger.matches(makeIssueLifecycleCtx({ action: 'assigned' }))).toBe(false); + }); + }); + + // ------------------------------------------------------------------------- + // handle() + // ------------------------------------------------------------------------- + + describe('handle()', () => { + it('returns null when trigger is disabled', async () => { + vi.mocked(checkTriggerEnabledWithParams).mockResolvedValue({ + enabled: false, + parameters: {}, + }); + const result = await trigger.handle(makeIssueLifecycleCtx()); + expect(result).toBeNull(); + }); + + it('returns null when issue ID is missing', async () => { + const result = await trigger.handle( + makeIssueLifecycleCtx({ issueOverrides: { id: '' as unknown as string } }), + ); + expect(result).toBeNull(); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('cannot determine issue ID'), + expect.any(Object), + ); + }); + + it('returns null when Sentry integration config is missing', async () => { + vi.mocked(getSentryIntegrationConfig).mockResolvedValue(null); + const result = await trigger.handle(makeIssueLifecycleCtx()); + expect(result).toBeNull(); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('no Sentry integration config'), + expect.any(Object), + ); + }); + + it('returns null when alerts slot is not configured (pre-flight skip)', async () => { + vi.mocked(getAlertsContainerId).mockReturnValue(undefined); + const result = await trigger.handle(makeIssueLifecycleCtx()); + expect(result).toBeNull(); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('alerts slot not configured'), + expect.any(Object), + ); + }); + + it('returns a TriggerResult with alertIssueId from data.issue.id', async () => { + const result = await trigger.handle(makeIssueLifecycleCtx()); + expect(result).toMatchObject({ + agentType: 'alerting', + agentInput: { + triggerEvent: 'alerting:issue-lifecycle', + alertIssueId: '118723355', + alertOrgId: 'mongrel', + alertIssueUrl: 'https://mongrel.sentry.io/issues/118723355/', + }, + }); + }); + + it('uses data.issue.title as alertTitle', async () => { + const result = await trigger.handle(makeIssueLifecycleCtx()); + expect(result?.agentInput?.alertTitle).toContain('wedged work-item lock'); + }); + + it('falls back to default alertTitle when title is empty', async () => { + const result = await trigger.handle(makeIssueLifecycleCtx({ issueOverrides: { title: '' } })); + expect(result?.agentInput?.alertTitle).toBe('Sentry Issue'); + }); + + it('falls back to permalink for alertIssueUrl when web_url is missing', async () => { + const result = await trigger.handle( + makeIssueLifecycleCtx({ + issueOverrides: { + web_url: undefined as unknown as string, + permalink: 'https://mongrel.sentry.io/perma/118723355/', + }, + }), + ); + expect(result?.agentInput?.alertIssueUrl).toBe('https://mongrel.sentry.io/perma/118723355/'); + }); + + it('does NOT set workItemId — materialisation is deferred to processSentryWebhook', async () => { + const result = await trigger.handle(makeIssueLifecycleCtx()); + expect(result?.workItemId).toBeUndefined(); + expect(result?.agentInput?.workItemId).toBeUndefined(); + }); + + it('uses a sentry-issue-namespaced lockKey distinct from event_alert', async () => { + const result = await trigger.handle(makeIssueLifecycleCtx()); + // Distinct from `sentry:` (event_alert) so the same issue can + // arrive via both surfaces concurrently without lock contention. + expect(result?.lockKey).toBe('sentry-issue:118723355'); + }); + + it('uses a sentry-issue-namespaced coalesceKey', async () => { + const result = await trigger.handle(makeIssueLifecycleCtx()); + expect(result?.coalesceKey).toBe(`${mockProject.id}:sentry-issue:118723355`); + }); + }); +}); diff --git a/tests/unit/triggers/shared/events.test.ts b/tests/unit/triggers/shared/events.test.ts index 2c5723e1e..70130e056 100644 --- a/tests/unit/triggers/shared/events.test.ts +++ b/tests/unit/triggers/shared/events.test.ts @@ -25,6 +25,7 @@ describe('TRIGGER_EVENTS', () => { 'scm:pr-conflict-detected', 'alerting:issue-alert', 'alerting:metric-alert', + 'alerting:issue-lifecycle', 'internal:auto-chain', ]); }); diff --git a/tests/unit/triggers/trigger-contracts.test.ts b/tests/unit/triggers/trigger-contracts.test.ts new file mode 100644 index 000000000..e44ea76bb --- /dev/null +++ b/tests/unit/triggers/trigger-contracts.test.ts @@ -0,0 +1,256 @@ +import { describe, expect, it, vi } from 'vitest'; +import { + type CheckSuiteDecision, + decideCheckSuiteOutcome, +} from '../../../src/triggers/github/check-suite-decision.js'; +import { + buildRespondToCiResult, + buildReviewResult, +} from '../../../src/triggers/github/result-builders.js'; +import { TRIGGER_EVENTS } from '../../../src/triggers/shared/events.js'; +import { buildPMLabelDispatchResult } from '../../../src/triggers/shared/pm-label.js'; +import { + buildPMStatusDispatchResult, + shouldFirePMStatusEvent, +} from '../../../src/triggers/shared/pm-status.js'; +import { + buildDeferredRecheckResult, + buildSkipResult, +} from '../../../src/triggers/shared/result-builders.js'; +import { + createCheckSuiteStatus, + createMockProject, + createPMLabelAddedFixture, + createPMStatusFixture, +} from '../../helpers/factories.js'; +import { mockPersonaIdentities } from '../../helpers/mockPersonas.js'; + +const prDetails = { + headRef: 'feature/contract-coverage', + htmlUrl: 'https://github.com/owner/repo/pull/42', + title: 'feat: contract coverage', +}; + +describe('trigger conformance contracts', () => { + describe('PM status-changed fixtures', () => { + it('preserves canonical event/result shape for PM create events', () => { + const fixture = createPMStatusFixture({ kind: 'create' }); + const result = buildPMStatusDispatchResult({ + projectId: fixture.projectId, + agentType: fixture.agentType, + workItemId: fixture.workItemId, + workItemUrl: fixture.workItemUrl, + workItemTitle: fixture.workItemTitle, + agentInput: { + providerStatusId: fixture.providerStatusId, + providerStatusName: fixture.providerStatusName, + }, + }); + + expect(shouldFirePMStatusEvent(true, { onCreate: true })).toBe(true); + expect(result).toMatchObject({ + agentType: 'implementation', + workItemId: fixture.workItemId, + workItemUrl: fixture.workItemUrl, + workItemTitle: fixture.workItemTitle, + coalesceKey: `${fixture.projectId}:${fixture.workItemId}`, + agentInput: { + workItemId: fixture.workItemId, + workItemUrl: fixture.workItemUrl, + workItemTitle: fixture.workItemTitle, + triggerEvent: TRIGGER_EVENTS.PM.STATUS_CHANGED, + providerStatusId: fixture.providerStatusId, + providerStatusName: fixture.providerStatusName, + }, + }); + }); + + it('preserves canonical event/result shape for PM move events', () => { + const fixture = createPMStatusFixture({ kind: 'move', agentType: 'planning' }); + const result = buildPMStatusDispatchResult({ + projectId: fixture.projectId, + agentType: fixture.agentType, + workItemId: fixture.workItemId, + workItemUrl: fixture.workItemUrl, + workItemTitle: fixture.workItemTitle, + agentInput: { + previousStatusId: fixture.previousStatusId, + previousStatusName: fixture.previousStatusName, + providerStatusId: fixture.providerStatusId, + providerStatusName: fixture.providerStatusName, + }, + }); + + expect(shouldFirePMStatusEvent(false, {})).toBe(true); + expect(result).toMatchObject({ + agentType: 'planning', + workItemId: fixture.workItemId, + coalesceKey: `${fixture.projectId}:${fixture.workItemId}`, + agentInput: { + workItemId: fixture.workItemId, + triggerEvent: TRIGGER_EVENTS.PM.STATUS_CHANGED, + previousStatusId: fixture.previousStatusId, + previousStatusName: fixture.previousStatusName, + providerStatusId: fixture.providerStatusId, + providerStatusName: fixture.providerStatusName, + }, + }); + }); + }); + + it('preserves canonical event/result shape for PM label-added events', () => { + const fixture = createPMLabelAddedFixture(); + const result = buildPMLabelDispatchResult({ + agentType: fixture.agentType, + workItemId: fixture.workItemId, + workItemUrl: fixture.workItemUrl, + workItemTitle: fixture.workItemTitle, + agentInput: { + labelId: fixture.labelId, + labelName: fixture.labelName, + containerId: fixture.containerId, + }, + }); + + expect(result).toMatchObject({ + agentType: 'splitting', + workItemId: fixture.workItemId, + workItemUrl: fixture.workItemUrl, + workItemTitle: fixture.workItemTitle, + agentInput: { + workItemId: fixture.workItemId, + workItemUrl: fixture.workItemUrl, + workItemTitle: fixture.workItemTitle, + triggerEvent: TRIGGER_EVENTS.PM.LABEL_ADDED, + labelId: fixture.labelId, + labelName: fixture.labelName, + containerId: fixture.containerId, + }, + }); + }); + + describe('GitHub check-suite aggregate outcomes', () => { + const project = createMockProject(); + const baseDecisionOptions = { + prNumber: 42, + prAuthorLogin: 'cascade-impl', + prBaseRef: 'main', + project, + personaIdentities: mockPersonaIdentities, + handlerName: 'check-suite-success', + mode: { kind: 'review', parameters: {} }, + } as const; + + it.each([ + { + name: 'all passing', + status: createCheckSuiteStatus([ + { name: 'lint', status: 'completed', conclusion: 'success' }, + { name: 'test', status: 'completed', conclusion: 'neutral' }, + ]), + expected: { action: 'review' } satisfies CheckSuiteDecision, + }, + { + name: 'mixed failure', + status: createCheckSuiteStatus([ + { name: 'lint', status: 'completed', conclusion: 'success' }, + { name: 'test', status: 'completed', conclusion: 'failure' }, + ]), + expected: { action: 'respond-to-ci' } satisfies CheckSuiteDecision, + }, + { + name: 'incomplete checks', + status: createCheckSuiteStatus([ + { name: 'lint', status: 'completed', conclusion: 'success' }, + { name: 'test', status: 'in_progress', conclusion: null }, + ]), + expected: { + action: 'defer', + incompleteChecks: ['test'], + message: 'Not all checks complete yet (1/2 still running): test', + } satisfies CheckSuiteDecision, + }, + ])('resolves $name aggregate state', ({ status, expected }) => { + expect(decideCheckSuiteOutcome({ ...baseDecisionOptions, checkStatus: status })).toEqual( + expected, + ); + }); + + it('builds canonical review and respond-to-ci dispatch results', () => { + const review = buildReviewResult({ + prNumber: 42, + prDetails, + repoFullName: 'owner/repo', + headSha: 'sha123', + workItemId: 'card-123', + workItemUrl: 'https://trello.com/c/card-123', + workItemTitle: 'Implement feature', + onBlocked: vi.fn(), + }); + const respondToCi = buildRespondToCiResult({ + prNumber: 42, + prDetails, + repoFullName: 'owner/repo', + headSha: 'sha123', + workItemId: 'card-123', + workItemUrl: 'https://trello.com/c/card-123', + workItemTitle: 'Implement feature', + }); + + expect(review).toMatchObject({ + agentType: 'review', + prNumber: 42, + workItemId: 'card-123', + agentInput: { + prNumber: 42, + workItemId: 'card-123', + triggerEvent: TRIGGER_EVENTS.SCM.CHECK_SUITE_SUCCESS, + triggerType: 'ci-success', + repoFullName: 'owner/repo', + headSha: 'sha123', + }, + }); + expect(respondToCi).toMatchObject({ + agentType: 'respond-to-ci', + prNumber: 42, + workItemId: 'card-123', + agentInput: { + prNumber: 42, + workItemId: 'card-123', + triggerEvent: TRIGGER_EVENTS.SCM.CHECK_SUITE_FAILURE, + triggerType: 'check-failure', + repoFullName: 'owner/repo', + headSha: 'sha123', + }, + }); + }); + }); + + it('builds deferred re-check results with no embedded agent dispatch', () => { + expect( + buildDeferredRecheckResult({ + delayMs: 45_000, + coalesceKey: 'project-1:pr-conflict-recheck:42', + agentInput: { prNumber: 42 }, + }), + ).toEqual({ + agentType: null, + agentInput: { prNumber: 42 }, + deferredRecheck: { + delayMs: 45_000, + coalesceKey: 'project-1:pr-conflict-recheck:42', + }, + }); + }); + + it('builds structured skip results that terminate dispatch with a handler reason', () => { + expect(buildSkipResult('check-suite-success', 'Not all checks complete yet')).toEqual({ + agentType: null, + agentInput: {}, + skipReason: { + handler: 'check-suite-success', + message: 'Not all checks complete yet', + }, + }); + }); +}); diff --git a/tests/unit/triggers/trigger-event-consistency.test.ts b/tests/unit/triggers/trigger-event-consistency.test.ts index b0bcfe90e..b40300478 100644 --- a/tests/unit/triggers/trigger-event-consistency.test.ts +++ b/tests/unit/triggers/trigger-event-consistency.test.ts @@ -25,6 +25,7 @@ import { readdirSync, readFileSync, statSync } from 'node:fs'; import { join } from 'node:path'; import { describe, expect, it } from 'vitest'; +import { ALL_TRIGGER_EVENTS } from '../../../src/triggers/shared/events.js'; const TRIGGERS_ROOT = join(__dirname, '..', '..', '..', 'src', 'triggers'); @@ -66,6 +67,13 @@ interface HandlerScan { emittedEvents: Set; } +interface RawTriggerLiteralOccurrence { + file: string; + lineNumber: number; + literal: string; + line: string; +} + function listHandlerFiles(dir: string): string[] { const out: string[] = []; for (const entry of readdirSync(dir)) { @@ -130,6 +138,70 @@ function scanHandler(file: string): HandlerScan { return { file, gatingEvents, emittedEvents }; } +const RAW_TRIGGER_LITERAL_EXEMPTIONS = new Set([ + // Existing handlers still pass literal event IDs into trigger-enabled gates + // or inline legacy AgentInput objects. Keep exemptions exact and line-scoped: + // new handlers or newly-added raw event literals must use TRIGGER_EVENTS + // unless a new entry is added here with a reason. + "src/triggers/trello/status-changed.ts :: 'pm:status-changed',", + "src/triggers/github/pr-review-submitted.ts :: 'scm:pr-review-submitted',", + "src/triggers/github/pr-review-submitted.ts :: triggerEvent: 'scm:pr-review-submitted',", + "src/triggers/github/pr-merged.ts :: if (await checkTriggerEnabled(ctx.project.id, 'backlog-manager', 'scm:pr-merged', this.name)) {", + "src/triggers/github/pr-merged.ts :: agentInput: { triggerEvent: 'scm:pr-merged', workItemId: workItemId },", + "src/triggers/github/check-suite-success.ts :: 'scm:check-suite-success',", + "src/triggers/github/check-suite-failure.ts :: 'scm:check-suite-failure',", + "src/triggers/github/pr-conflict-detected.ts :: 'scm:pr-conflict-detected',", + "src/triggers/github/review-requested.ts :: 'scm:review-requested',", + "src/triggers/github/review-requested.ts :: triggerEvent: 'scm:review-requested',", + "src/triggers/github/pr-comment-mention.ts :: 'scm:pr-comment-mention',", + "src/triggers/github/pr-comment-mention.ts :: triggerEvent: 'scm:pr-comment-mention',", + "src/triggers/github/pr-opened.ts :: 'scm:pr-opened',", + "src/triggers/github/pr-opened.ts :: triggerEvent: 'scm:pr-opened',", + "src/triggers/github/respond-to-ci-dispatch.ts :: 'scm:check-suite-failure',", + "src/triggers/trello/comment-mention.ts :: 'pm:comment-mention',", + "src/triggers/trello/comment-mention.ts :: triggerEvent: 'pm:comment-mention',", + "src/triggers/trello/label-added.ts :: if (!(await checkTriggerEnabled(ctx.project.id, agentType, 'pm:label-added', this.name))) {", + "src/triggers/config-resolver.ts :: /** Trigger event identifier (e.g., 'pm:status-changed') */", + "src/triggers/jira/status-changed.ts :: 'pm:status-changed',", + "src/triggers/jira/comment-mention.ts :: 'pm:comment-mention',", + "src/triggers/jira/comment-mention.ts :: triggerEvent: 'pm:comment-mention',", + "src/triggers/shared/splitting-auto-chain.ts :: 'internal:auto-chain',", + "src/triggers/shared/splitting-auto-chain.ts :: agentInput: { triggerEvent: 'internal:auto-chain', workItemId },", + "src/triggers/shared/post-completion-review.ts :: triggerEvent: 'scm:check-suite-success',", + "src/triggers/jira/label-added.ts :: if (!(await checkTriggerEnabled(ctx.project.id, agentType, 'pm:label-added', this.name))) {", + "src/triggers/linear/comment-mention.ts :: 'pm:comment-mention',", + "src/triggers/linear/comment-mention.ts :: triggerEvent: 'pm:comment-mention',", + "src/triggers/linear/label-added.ts :: if (!(await checkTriggerEnabled(ctx.project.id, agentType, 'pm:label-added', this.name))) {", + "src/triggers/linear/status-changed.ts :: 'pm:status-changed',", + "src/triggers/sentry/alerting-metric.ts :: 'alerting:metric-alert',", + "src/triggers/sentry/alerting-metric.ts :: triggerEvent: 'alerting:metric-alert',", + "src/triggers/sentry/alerting-issue.ts :: 'alerting:issue-alert',", + "src/triggers/sentry/alerting-issue.ts :: triggerEvent: 'alerting:issue-alert',", +]); + +function findRawTriggerLiteralOccurrences(files: string[]): RawTriggerLiteralOccurrence[] { + const escapedEvents = ALL_TRIGGER_EVENTS.map((event) => + event.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), + ).join('|'); + const rawLiteralPattern = new RegExp(`[\`'"](${escapedEvents})[\`'"]`, 'g'); + + return files + .filter((file) => !file.endsWith('/shared/events.ts')) + .flatMap((file) => { + const relPath = file.replace(`${TRIGGERS_ROOT}/`, 'src/triggers/'); + const lines = readFileSync(file, 'utf-8').split('\n'); + return lines.flatMap((line, index) => { + const matches = [...line.matchAll(rawLiteralPattern)]; + return matches.map((match) => ({ + file: relPath, + lineNumber: index + 1, + literal: match[1], + line: line.trim(), + })); + }); + }); +} + describe('trigger-event-string consistency (static guard)', () => { const allFiles = listHandlerFiles(TRIGGERS_ROOT).filter( (f) => !f.endsWith('/trigger-check.ts') && !EXEMPT_FILES.has(f), @@ -175,4 +247,22 @@ describe('trigger-event-string consistency (static guard)', () => { } }); } + + it('forbids new raw trigger-event literals outside the event catalog', () => { + const occurrences = findRawTriggerLiteralOccurrences(allFiles); + const unexpected = occurrences.filter( + (occurrence) => + !RAW_TRIGGER_LITERAL_EXEMPTIONS.has(`${occurrence.file} :: ${occurrence.line}`), + ); + + expect( + unexpected.map( + (occurrence) => + `${occurrence.file}:${occurrence.lineNumber} raw ${occurrence.literal} in: ${occurrence.line}`, + ), + `New trigger handlers must reference TRIGGER_EVENTS from src/triggers/shared/events.ts ` + + `instead of adding raw event strings. If a literal is unavoidable, add a narrow ` + + `RAW_TRIGGER_LITERAL_EXEMPTIONS entry with a reason. Unexpected raw literals:`, + ).toEqual([]); + }); });