From 900e1911963405a42deea02efeaaef7d0dc7da89 Mon Sep 17 00:00:00 2001 From: Maxi Date: Mon, 7 Sep 2026 17:18:23 +0100 Subject: [PATCH] fix: share transition history across a pull request's events, and release 0.0.5 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A route on `pr_review_requested` matching `reviewersAdded` could never fire. Not intermittently — never, including the "Reviewer agent" template this ships in the route catalog. Observations were keyed `:`, so every trigger type kept its own transition baseline. But `pr_review_requested` is only emitted once a reviewer is outstanding, so that baseline came into existence at the exact moment of the transition it existed to detect: the cycle a reviewer is added is the row's first sight, which deliberately reports no changes, and by the next cycle the reviewer is no longer new. Every unit test passed because they all hand `changes` to the rule engine directly. None ran collect -> observe -> match, which is the only place the two halves disagree. - What changed belongs to the *item*, not to the trigger type it raised. Observations are keyed on `ref` alone and the poll loop observes once per item per cycle, so both events a pull request raises read one history. `pr_event` is emitted for every open PR whether or not a review is outstanding, which is what makes its baseline the right one to share. - `migrateObservationKeys` promotes existing `:` rows on boot, so an upgrade does not reseed a history the install already had and go quiet for a cycle. `pr_event` wins where both rows exist. - `test/routing/trigger-pipeline.test.ts` wires collection, observation and matching together as `main()` does, and covers the request, the outstanding request, the re-request, and first sight of a PR whose reviewer predates it. Debuggability, which is the reason this survived: the cycle summary's `no-route` is equally consistent with a typo'd login, a missing target, and a transition that was never recorded. - `explainRule` names the one clause that rejected an event, in the operator's vocabulary. `matchesRule` is now *defined as* `explainRule(...) === undefined` rather than written beside it, so the explanation cannot drift from the decision. - `sentinel0 explain [--project ] [--ref ]`, over a new `GET /routes/explain`, re-collects live triggers and reports every route's verdict per item. It reads history through the new `changesSince` and never `observe`: a diagnostic that advanced the baseline would consume the transition being asked about and answer differently the second time. - The output says plainly that a matching route may still not run — the dedupe ledger, one-run-per-agent and agent resolution all come after matching. CLAUDE.md and docs/routes.md carry both halves. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QNEqbFkMFoPiiVnJGxpjj2 --- CLAUDE.md | 19 +- docs/routes.md | 35 +++- package.json | 2 +- packages/cli/package.json | 6 +- packages/cli/src/args.ts | 6 + packages/cli/src/commands/explain.ts | 107 ++++++++++ packages/cli/src/index.ts | 4 + packages/cli/src/types.ts | 1 + packages/cli/src/usage.ts | 2 + packages/cloud-api/package.json | 2 +- packages/cloud-dashboard/package.json | 2 +- packages/common/package.json | 2 +- packages/orchestrator/package.json | 2 +- packages/orchestrator/src/database.ts | 95 ++++++++- packages/orchestrator/src/index.ts | 43 +++- .../orchestrator/src/routing/rule-engine.ts | 127 +++++++----- .../orchestrator/src/runtime/api-server.ts | 89 ++++++++ .../test/routing/explain-api.test.ts | 115 +++++++++++ .../test/routing/rule-engine.test.ts | 74 ++++++- .../test/routing/trigger-pipeline.test.ts | 192 ++++++++++++++++++ pnpm-lock.yaml | 4 +- 21 files changed, 851 insertions(+), 78 deletions(-) create mode 100644 packages/cli/src/commands/explain.ts create mode 100644 packages/orchestrator/test/routing/explain-api.test.ts create mode 100644 packages/orchestrator/test/routing/trigger-pipeline.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 2fd2d74..39c20b7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -139,7 +139,24 @@ by hand. `trigger → match → target → execution → outcome`. `rule-engine.ts` is pure — no I/O, no clock — so the whole "which agent starts, and when" decision is exhaustively -unit-testable. +unit-testable. `matchesRule` is *defined as* `explainRule(...) === undefined` rather +than written beside it: a route that silently does nothing is this system's hardest +failure, the cycle summary's `no-route` is equally consistent with a typo'd login and a +transition that was never recorded, and two implementations would drift. `sentinel0 +explain` (`GET /routes/explain`) re-collects live triggers and reports the one clause +that rejected each, reading history via `changesSince` and never `observe` — a +diagnostic that advanced the baseline would consume the transition being asked about. + +**Transition history belongs to the item, not to the trigger type.** One pull request +raises a `pr_event` and, while a review is outstanding, a `pr_review_requested`; +observations are keyed on `ref` alone and the poll loop observes once per item per +cycle. Keying per type gave the narrower event a baseline that began at the instant of +the transition it existed to detect — first sight reports no changes, and by the next +cycle the reviewer is no longer new — so `pr_review_requested` + `reviewersAdded` could +never fire, the shipped reviewer-agent template included. `pr_event` is emitted for +every open PR regardless, which is what makes its baseline the right one to share. +`migrateObservationKeys` promotes existing `:` rows so an upgrade does not +reseed a history the install already had. Two invariants the dispatcher enforces: diff --git a/docs/routes.md b/docs/routes.md index 961e3be..33d3987 100644 --- a/docs/routes.md +++ b/docs/routes.md @@ -76,6 +76,13 @@ every label would look newly added, and creating a route would fire it across yo entire backlog. A new route starts quiet and acts on what happens next — so to test one, add and remove the label once while the runner is up. +"First seen" means the *item*, not the trigger type. One pull request raises both a +`pr_event` and — while a review is outstanding — a `pr_review_requested`, and both read +the same history. This matters because `pr_review_requested` only exists once a +reviewer does: were it to keep its own baseline, that baseline would begin at the very +moment of the request, first sight would suppress it, and a `reviewersAdded` route on +that trigger could never fire at all. It used to work exactly that way. + `labels` asks *does it have this now*. `labelsAdded` asks *was it just added*. Pick the second when the act is the signal. @@ -267,7 +274,28 @@ pause, unlabel to resume. ## Troubleshooting -**Nothing fired.** Watch one poll cycle — the runner prints a summary line: +**Nothing fired.** Ask the runner directly: + +```bash +sentinel0 explain # every visible trigger, every route +sentinel0 explain --ref acme/www#42 # just this one +``` + +It re-collects triggers and prints, per item, the one clause that rejected it: + +``` + - acme/www#42 pr_review_requested Fix the export + changed nothing changed since the last poll + skips Reviewer agent: match.reviewersAdded wants any of [EomiAIBot]; nothing + changed there this cycle (nothing) +``` + +It reads history without recording any, so asking does not consume the transition you +are asking about, and the answer is the same every time until something really changes. +A *matching* route can still not run — the dedupe ledger, the one-run-per-agent rule and +agent resolution all come after matching, and the cycle summary reports those. + +For the cycle's own view, the runner prints a summary line: ``` poll: 12 event(s) (taplands 12) · dispatched 1 · skipped 11 (no-route 10, duplicate 1) @@ -281,8 +309,9 @@ matched. `unknown-agent` means the route names a profile absent from `sentinel0 `sentinel0:done` — remove it to re-arm, or switch to `per-change` with a transition clause. -**A transition route never fires.** It needs a prior observation. Add and remove the -label once while the runner is up. +**A transition route never fires.** It needs a prior observation. `sentinel0 explain` +says `never been observed before` when that is the reason; add and remove the label once +while the runner is up. **Everything fired at once when I created a route.** A state clause (`labels`) matches everything currently carrying the label. Use `labelsAdded` if you meant the act. diff --git a/package.json b/package.json index d8d2055..4b21245 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "sentinel0-monorepo", - "version": "0.0.4", + "version": "0.0.5", "private": true, "type": "module", "scripts": { diff --git a/packages/cli/package.json b/packages/cli/package.json index 99f56f7..3de24ec 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "sentinel0", - "version": "0.0.4", + "version": "0.0.5", "description": "Trigger Hermes agents from your tickets and pull requests.", "type": "module", "keywords": [ @@ -35,8 +35,8 @@ "dependencies": { "@clack/prompts": "1.4.0", "@fastify/cors": "11.2.0", - "@sentinel0/common": "0.0.4", - "@sentinel0/orchestrator": "0.0.4", + "@sentinel0/common": "0.0.5", + "@sentinel0/orchestrator": "0.0.5", "chalk": "4", "fastify": "5.7.4", "p-limit": "6.1.0", diff --git a/packages/cli/src/args.ts b/packages/cli/src/args.ts index 5bfae0a..7479c7d 100644 --- a/packages/cli/src/args.ts +++ b/packages/cli/src/args.ts @@ -12,6 +12,7 @@ import type { LogsCommandOptions, RunCommandOptions, RunnerCommandOptions, + ExplainCommandOptions, RunsCommandOptions, StartCommandOptions, } from './types.js' @@ -110,6 +111,11 @@ export function parseRunsOptions(args: string[]): RunsCommandOptions { } } +export function parseExplainOptions(args: string[]): ExplainCommandOptions { + assertKnownFlags(args, ['--project', '--ref'], 'explain') + return { projectId: valueOf(args, '--project'), ref: valueOf(args, '--ref') } +} + export function parseRunOptions(args: string[]): RunCommandOptions { assertKnownFlags(args, ['--agent', '--prompt', '--timeout'], 'run') const agent = valueOf(args, '--agent') diff --git a/packages/cli/src/commands/explain.ts b/packages/cli/src/commands/explain.ts new file mode 100644 index 0000000..b6682c4 --- /dev/null +++ b/packages/cli/src/commands/explain.ts @@ -0,0 +1,107 @@ +import chalk from 'chalk' +import type { TriggerChanges, TriggerType } from '@sentinel0/common' +import { ApiError, getJson, runnerUnreachable } from '../api.js' +import type { CliContext, ExplainCommandOptions } from '../types.js' + +interface Verdict { + routeId: string + routeName: string + matched: boolean + reason?: string +} + +interface ExplainItem { + ref: string + type: TriggerType + projectId: string + title: string + url?: string + labels: string[] + assignees: string[] + requestedReviewers: string[] + changes?: TriggerChanges + verdicts: Verdict[] +} + +/** Only the transitions that actually happened, so an empty cycle stays quiet. */ +function describeChanges(changes: TriggerChanges | undefined): string { + if (!changes) { + return chalk.yellow('never polled before — transition rules cannot match yet') + } + const parts = [ + ['+label', changes.labelsAdded], + ['-label', changes.labelsRemoved], + ['+assignee', changes.assigneesAdded], + ['-assignee', changes.assigneesRemoved], + ['+reviewer', changes.reviewersAdded], + ] as const + + const moved = parts + .filter(([, values]) => values.length > 0) + .map(([name, values]) => `${name} ${values.join(', ')}`) + + return moved.length > 0 ? moved.join(' ') : chalk.dim('nothing changed since the last poll') +} + +export async function runExplain( + context: CliContext, + options: ExplainCommandOptions +): Promise { + const apiBase = await context.resolveDefaultApiBase() + const query = new URLSearchParams() + if (options.projectId) { + query.set('project', options.projectId) + } + if (options.ref) { + query.set('ref', options.ref) + } + + const suffix = query.size > 0 ? `?${query}` : '' + const { items } = await getJson<{ items: ExplainItem[] }>( + `${apiBase}/routes/explain${suffix}` + ).catch((error: unknown) => { + // An unknown project, or a tracker that will not answer, is the runner + // explaining itself and worth passing through verbatim. Anything else means + // it is not there at all, which needs the other sentence entirely. + if (error instanceof ApiError) { + throw error + } + throw runnerUnreachable(apiBase) + }) + + if (items.length === 0) { + console.log(chalk.yellow('\n No triggers are visible right now.')) + console.log(chalk.dim(' sentinel0 projects check what this runner is polling\n')) + return + } + + console.log('') + for (const item of items) { + const matched = item.verdicts.filter((verdict) => verdict.matched) + const mark = matched.length > 0 ? chalk.green('*') : chalk.dim('-') + + console.log(` ${mark} ${chalk.bold(item.ref)} ${chalk.dim(item.type)} ${item.title}`) + console.log(chalk.dim(` changed ${describeChanges(item.changes)}`)) + if (item.requestedReviewers.length > 0) { + console.log(chalk.dim(` reviewers ${item.requestedReviewers.join(', ')}`)) + } + + for (const verdict of item.verdicts) { + if (verdict.matched) { + console.log( + chalk.green(` matches ${verdict.routeName} ${chalk.dim(`(${verdict.routeId})`)}`) + ) + } else { + console.log(chalk.dim(` skips ${verdict.routeName}: ${verdict.reason}`)) + } + } + console.log('') + } + + // A matching route is not a dispatched one, and conflating the two sends + // people back to the rule engine to debug a busy agent. + console.log( + chalk.dim(' A matching route still defers if its agent is busy, or is a no-op if it') + ) + console.log(chalk.dim(' already fired for this revision. sentinel0 logs shows which.\n')) +} diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index f06f0f3..09c7c5a 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -12,6 +12,7 @@ import { parseLogsOptions, parseRunOptions, parseRunnerOptions, + parseExplainOptions, parseRunsOptions, parseStartOptions, resolvePath, @@ -37,6 +38,7 @@ import { runRestart } from './commands/restart.js' import { runRoutes } from './commands/routes.js' import { runRunner } from './commands/runner.js' import { runRuns } from './commands/runs.js' +import { runExplain } from './commands/explain.js' import { runSmokeTest } from './commands/run.js' import { runStart } from './commands/start.js' import { runStatus } from './commands/status.js' @@ -160,6 +162,8 @@ async function dispatch(command: string | undefined, args: string[]): Promise [--deny] answer an agent waiting for permission', '', chalk.bold(' Debugging'), + ' explain [--project ] [--ref ]', + chalk.dim(' why each route did or did not match'), ' run --agent --prompt "..."', chalk.dim(' send one prompt straight to Hermes'), '', diff --git a/packages/cloud-api/package.json b/packages/cloud-api/package.json index 3e28e16..df8cd4d 100644 --- a/packages/cloud-api/package.json +++ b/packages/cloud-api/package.json @@ -1,6 +1,6 @@ { "name": "@sentinel0/cloud-api", - "version": "0.0.4", + "version": "0.0.5", "private": true, "type": "module", "main": "dist/index.js", diff --git a/packages/cloud-dashboard/package.json b/packages/cloud-dashboard/package.json index 2d63b65..28feef9 100644 --- a/packages/cloud-dashboard/package.json +++ b/packages/cloud-dashboard/package.json @@ -1,6 +1,6 @@ { "name": "@sentinel0/cloud-dashboard", - "version": "0.0.4", + "version": "0.0.5", "private": true, "type": "module", "scripts": { diff --git a/packages/common/package.json b/packages/common/package.json index 981b802..92e0a8e 100644 --- a/packages/common/package.json +++ b/packages/common/package.json @@ -1,6 +1,6 @@ { "name": "@sentinel0/common", - "version": "0.0.4", + "version": "0.0.5", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/orchestrator/package.json b/packages/orchestrator/package.json index e277067..4d59bcd 100644 --- a/packages/orchestrator/package.json +++ b/packages/orchestrator/package.json @@ -1,6 +1,6 @@ { "name": "@sentinel0/orchestrator", - "version": "0.0.4", + "version": "0.0.5", "private": true, "type": "module", "scripts": { diff --git a/packages/orchestrator/src/database.ts b/packages/orchestrator/src/database.ts index 9026f50..99b9f29 100644 --- a/packages/orchestrator/src/database.ts +++ b/packages/orchestrator/src/database.ts @@ -2,6 +2,7 @@ import { DatabaseSync } from 'node:sqlite' import path from 'node:path' import { RUN_STATUS, + TRIGGER_TYPE, isTerminalRunStatus, type RunApprovalDetail, type RunLogEntry, @@ -37,6 +38,57 @@ function addColumn(db: DatabaseSync, table: string, column: string, definition: } } +/** + * Rewrites observation rows that were keyed `:` onto the bare + * ref. + * + * The old key gave every trigger type its own baseline for the same item, which + * quietly made `pr_review_requested` transitions impossible to detect: that + * event is only emitted once a reviewer exists, so its very first row was + * written at the moment of the transition a route wanted to fire on — and + * first sight reports no changes. `pr_event` is emitted for every open pull + * request whether or not anyone has been asked to review, so its row is the one + * holding the true history; it wins where both exist, and the rest are dropped. + * + * Without this a running install would go quiet for one cycle after upgrading, + * reseeding a baseline it already had. + */ +function migrateObservationKeys(db: DatabaseSync): void { + const legacy = db + .prepare("SELECT projectId, ref FROM observations WHERE ref LIKE '%:%'") + .all() as { projectId: string; ref: string }[] + + const prefixes = Object.values(TRIGGER_TYPE).map((type) => `${type}:`) + + // pr_event is emitted for every open pull request whether or not a review was + // requested, so its row is the one holding real history. Promoting it last + // lets it overwrite whatever a narrower type seeded. + const rank = (ref: string): number => (ref.startsWith(`${TRIGGER_TYPE.PR_EVENT}:`) ? 1 : 0) + legacy.sort((a, b) => rank(a.ref) - rank(b.ref)) + + for (const row of legacy) { + const prefix = prefixes.find((candidate) => row.ref.startsWith(candidate)) + // A ref that merely contains a colon and was never one of ours. + if (prefix) { + db.prepare( + `INSERT INTO observations (projectId, ref, labels, assignees, reviewers, observedAt) + SELECT projectId, ?, labels, assignees, reviewers, observedAt + FROM observations WHERE projectId = ? AND ref = ? + ON CONFLICT (projectId, ref) DO UPDATE SET + labels = excluded.labels, + assignees = excluded.assignees, + reviewers = excluded.reviewers, + observedAt = excluded.observedAt` + ).run(row.ref.slice(prefix.length), row.projectId, row.ref) + + db.prepare('DELETE FROM observations WHERE projectId = ? AND ref = ?').run( + row.projectId, + row.ref + ) + } + } +} + function migrate(db: DatabaseSync): void { db.exec(` CREATE TABLE IF NOT EXISTS runs ( @@ -97,6 +149,10 @@ function migrate(db: DatabaseSync): void { // What each item looked like last cycle, so "label added" can mean added // rather than merely present. + // + // Keyed by the item, not by the trigger type it raised. One pull request can + // raise two events, and giving each its own baseline made the transition + // history of the narrower one useless -- see migrateObservationKeys. db.exec(` CREATE TABLE IF NOT EXISTS observations ( projectId TEXT NOT NULL, @@ -130,6 +186,7 @@ function migrate(db: DatabaseSync): void { // Existing installs predate these columns; `CREATE TABLE IF NOT EXISTS` alone // would leave them behind on every machine that has already run. addColumn(db, 'runs', 'approvalDetail', 'TEXT') + migrateObservationKeys(db) db.exec('CREATE INDEX IF NOT EXISTS idx_runs_updated ON runs(updatedAt DESC)') db.exec('CREATE INDEX IF NOT EXISTS idx_runs_status ON runs(status, updatedAt DESC)') @@ -423,11 +480,7 @@ export class Sentinel0Database { current: { labels: string[]; assignees: string[]; reviewers: string[] }, now: number = Date.now() ): TriggerChanges | undefined { - const row = this.db - .prepare( - 'SELECT labels, assignees, reviewers FROM observations WHERE projectId = ? AND ref = ?' - ) - .get(projectId, ref) as { labels: string; assignees: string; reviewers: string } | undefined + const changes = this.changesSince(projectId, ref, current) this.db .prepare( @@ -448,6 +501,27 @@ export class Sentinel0Database { now ) + return changes + } + + /** + * What changed, without recording anything. + * + * The read half of `observe`, split out for the explain endpoint: asking why + * a route did not fire must not move the baseline it is being asked about, + * or the question would answer itself differently every time it is put. + */ + changesSince( + projectId: string, + ref: string, + current: { labels: string[]; assignees: string[]; reviewers: string[] } + ): TriggerChanges | undefined { + const row = this.db + .prepare( + 'SELECT labels, assignees, reviewers FROM observations WHERE projectId = ? AND ref = ?' + ) + .get(projectId, ref) as { labels: string; assignees: string; reviewers: string } | undefined + if (!row) { return undefined } @@ -467,6 +541,17 @@ export class Sentinel0Database { } } + /** + * Re-runs the observation key migration. + * + * Exposed because a migration that only ever runs inside the constructor + * cannot be tested, and this one decides whether a live install keeps the + * history it already had or goes quiet for a cycle. It is idempotent. + */ + migrateObservationKeys(): void { + migrateObservationKeys(this.db) + } + pruneObservations(olderThan: number): number { const result = this.db.prepare('DELETE FROM observations WHERE observedAt < ?').run(olderThan) return Number(result.changes) diff --git a/packages/orchestrator/src/index.ts b/packages/orchestrator/src/index.ts index 4d80ab4..228d759 100644 --- a/packages/orchestrator/src/index.ts +++ b/packages/orchestrator/src/index.ts @@ -12,6 +12,7 @@ import { type RoutingRule, type RunRecord, type RunStatus, + type TriggerChanges, type TriggerEvent, } from '@sentinel0/common' import { loadConfig, resolveDataDir } from './config-loader.js' @@ -560,6 +561,15 @@ async function main(): Promise { approveRun, db, dataDir, + collectTriggers: async (projectId) => { + const projects = projectId + ? runtime.projects.filter((project) => project.id === projectId) + : runtime.projects + const collected = await Promise.all( + projects.map((project) => triggerSourceFor(project, services).collect(project)) + ) + return collected.flat() + }, }) await fastify.listen({ @@ -805,15 +815,32 @@ async function main(): Promise { tally.collected += events.length tally.perProject.push(`${project.id} ${events.length}`) + // Record what each item looks like now and attach what changed, so + // routes can match "label added" rather than merely "label present". + // + // Observed once per *item*, not per event. One pull request raises both + // a pr_event and -- when a reviewer is outstanding -- a + // pr_review_requested, and keying the baseline by trigger type gave the + // second one a history that began at the very moment of the transition + // it existed to detect. First sight reports nothing changed, and by the + // next cycle the reviewer is no longer new, so a `reviewersAdded` route + // on pr_review_requested could never fire once -- including the + // reviewer-agent route this ships in the catalog. pr_event is emitted + // for every open pull request, so sharing its baseline is what gives + // the narrower event real history. + const observed = new Map() for (const event of events) { - // Record what this item looks like now and attach what changed, so - // routes can match "label added" rather than merely "label present". - const changes = db.observe(event.projectId, `${event.type}:${event.ref}`, { - labels: event.labels, - assignees: event.assignees ?? [], - reviewers: event.requestedReviewers ?? [], - }) - void runDispatch(project, { ...event, changes }, tally) + if (!observed.has(event.ref)) { + observed.set( + event.ref, + db.observe(event.projectId, event.ref, { + labels: event.labels, + assignees: event.assignees ?? [], + reviewers: event.requestedReviewers ?? [], + }) + ) + } + void runDispatch(project, { ...event, changes: observed.get(event.ref) }, tally) } } diff --git a/packages/orchestrator/src/routing/rule-engine.ts b/packages/orchestrator/src/routing/rule-engine.ts index b26c888..cfba088 100644 --- a/packages/orchestrator/src/routing/rule-engine.ts +++ b/packages/orchestrator/src/routing/rule-engine.ts @@ -62,9 +62,20 @@ function matchesPattern(pattern: string | undefined, value: string): boolean { } } -export function matchesRule(rule: RoutingRule, event: TriggerEvent): boolean { +/** + * Why a rule did not select an event, in the operator's own vocabulary, or + * undefined when it did. + * + * `matchesRule` is defined in terms of this rather than beside it. A route that + * silently does nothing is the hardest failure this system has: the log says + * `no-route`, which is equally consistent with a typo'd login, a transition + * that was never recorded, and a route that is simply switched off. Two + * implementations -- one deciding, one explaining -- would eventually disagree, + * and the explanation is only worth having if it is the decision. + */ +export function explainRule(rule: RoutingRule, event: TriggerEvent): string | undefined { if (!rule.enabled) { - return false + return 'the route is disabled' } // Never start a second agent on something a run is already working on. This @@ -72,77 +83,74 @@ export function matchesRule(rule: RoutingRule, event: TriggerEvent): boolean { // the change would re-trigger the very route that started the work. const present = new Set(normalize(event.labels)) if (present.has(SENTINEL0_LABEL.IN_PROGRESS)) { - return false + return `${event.ref} carries ${SENTINEL0_LABEL.IN_PROGRESS}, so a run is already working on it` } // A route that fires once per item also declines anything it already // finished. Removing the marker by hand is how a human re-arms it. const guard = guardOf(rule) if (guard.refire === 'once' && guard.markers) { - if (present.has(SENTINEL0_LABEL.DONE) || present.has(SENTINEL0_LABEL.FAILED)) { - return false + const marker = [SENTINEL0_LABEL.DONE, SENTINEL0_LABEL.FAILED].find((label) => + present.has(label) + ) + if (marker) { + return `${event.ref} carries ${marker} and guard.refire is "once"; remove the label to re-arm it` } } if (rule.trigger.type !== event.type) { - return false + return `trigger.type is ${rule.trigger.type}, the event is ${event.type}` } if (rule.trigger.projectId !== event.projectId) { - return false + return `trigger.projectId is "${rule.trigger.projectId}", the event is from "${event.projectId}"` } if (rule.trigger.provider && rule.trigger.provider !== event.provider) { - return false + return `trigger.provider is ${rule.trigger.provider}, the event came from ${event.provider}` } - if (!matchesSet(rule.match.labels, event.labels)) { - return false - } - if (!matchesSet(rule.match.state, event.state ? [event.state] : [])) { - return false - } - if (!matchesSet(rule.match.assignees, event.assignees ?? [])) { - return false - } - if (!matchesSet(rule.match.reviewers, event.requestedReviewers ?? [])) { - return false - } - if (!matchesSet(rule.match.baseBranch, event.baseBranch ? [event.baseBranch] : [])) { - return false + const state = [ + ['match.labels', rule.match.labels, event.labels], + ['match.state', rule.match.state, event.state ? [event.state] : []], + ['match.assignees', rule.match.assignees, event.assignees ?? []], + ['match.reviewers', rule.match.reviewers, event.requestedReviewers ?? []], + ['match.baseBranch', rule.match.baseBranch, event.baseBranch ? [event.baseBranch] : []], + ] as const + + for (const [name, match, values] of state) { + if (!matchesSet(match, values)) { + return `${name} ${describeSet(match)}; the event has ${describeValues(values)}` + } } + if (rule.match.isDraft !== undefined && rule.match.isDraft !== (event.isDraft ?? false)) { - return false + return `match.isDraft is ${rule.match.isDraft}, the pull request is ${event.isDraft ?? false}` } // Transition clauses need history. On first sight there is none, so a route // keyed on "label added" stays quiet rather than firing across a backlog of // items that happen to already carry the label. - const wantsTransition = - rule.match.labelsAdded !== undefined || - rule.match.labelsRemoved !== undefined || - rule.match.assigneesAdded !== undefined || - rule.match.reviewersAdded !== undefined - + const transitions = [ + ['match.labelsAdded', rule.match.labelsAdded, event.changes?.labelsAdded], + ['match.labelsRemoved', rule.match.labelsRemoved, event.changes?.labelsRemoved], + ['match.assigneesAdded', rule.match.assigneesAdded, event.changes?.assigneesAdded], + ['match.reviewersAdded', rule.match.reviewersAdded, event.changes?.reviewersAdded], + ] as const + + const wantsTransition = transitions.some(([, match]) => match !== undefined) if (wantsTransition) { if (!event.changes) { - return false - } - if (!matchesSet(rule.match.labelsAdded, event.changes.labelsAdded)) { - return false - } - if (!matchesSet(rule.match.labelsRemoved, event.changes.labelsRemoved)) { - return false + return `${event.ref} has never been observed before, and a transition clause needs a previous sighting to compare against; it will be evaluated from the next poll onward` } - if (!matchesSet(rule.match.assigneesAdded, event.changes.assigneesAdded)) { - return false - } - if (!matchesSet(rule.match.reviewersAdded, event.changes.reviewersAdded)) { - return false + for (const [name, match, values] of transitions) { + if (!matchesSet(match, values ?? [])) { + return `${name} ${describeSet(match)}; nothing changed there this cycle (${describeValues(values ?? [])})` + } } } if (!matchesPattern(rule.match.titleMatches, event.title)) { - return false + return `match.titleMatches /${rule.match.titleMatches}/ does not match "${event.title}"` } if (!matchesPattern(rule.match.bodyMatches, event.body)) { - return false + return `match.bodyMatches /${rule.match.bodyMatches}/ does not match the body` } // A rule targeting an agent by GitHub identity only fires when that identity @@ -156,16 +164,35 @@ export function matchesRule(rule: RoutingRule, event: TriggerEvent): boolean { // review. A dead route that reports no error is worse than a rejected one. const githubLogin = rule.target.agentRef.githubLogin if (githubLogin) { - const named = new Set([ - ...normalize(event.requestedReviewers ?? []), - ...normalize(event.assignees ?? []), - ]) - if (!named.has(githubLogin.trim().toLowerCase())) { - return false + const named = [...(event.requestedReviewers ?? []), ...(event.assignees ?? [])] + if (!new Set(normalize(named)).has(githubLogin.trim().toLowerCase())) { + return `target.agentRef.githubLogin is "${githubLogin}", who is neither assigned to nor a requested reviewer on ${event.ref} (${describeValues(named)})` } } - return true + return undefined +} + +function describeSet(match: StringSetMatch | undefined): string { + const parts: string[] = [] + if (match?.any?.length) { + parts.push(`wants any of [${match.any.join(', ')}]`) + } + if (match?.all?.length) { + parts.push(`wants all of [${match.all.join(', ')}]`) + } + if (match?.none?.length) { + parts.push(`excludes [${match.none.join(', ')}]`) + } + return parts.join(' and ') || 'imposes no constraint' +} + +function describeValues(values: readonly string[]): string { + return values.length > 0 ? `[${values.join(', ')}]` : 'nothing' +} + +export function matchesRule(rule: RoutingRule, event: TriggerEvent): boolean { + return explainRule(rule, event) === undefined } /** diff --git a/packages/orchestrator/src/runtime/api-server.ts b/packages/orchestrator/src/runtime/api-server.ts index a72cd82..3838520 100644 --- a/packages/orchestrator/src/runtime/api-server.ts +++ b/packages/orchestrator/src/runtime/api-server.ts @@ -9,10 +9,12 @@ import { type ProjectConfig, type RoutingRule, type RunStatus, + type TriggerEvent, } from '@sentinel0/common' import type { Sentinel0Database } from '../database.js' import { readRunnerErrors } from './diagnostics.js' import { isAllowedBrowserOrigin } from './network-access.js' +import { explainRule } from '../routing/rule-engine.js' export interface ApiServerDeps { getConfig: () => AppConfig @@ -24,6 +26,71 @@ export interface ApiServerDeps { approveRun: (runId: string, choice: ApprovalChoice) => Promise<{ ok: boolean; reason?: string }> db: Sentinel0Database dataDir: string + /** + * Re-collects triggers for one project, or for all of them. + * + * Separate from the poll loop's own collection so that asking the question + * does not record an answer: see `explainRouting`. + */ + collectTriggers: (projectId?: string) => Promise +} + +/** + * Why every route did or did not select every trigger currently visible. + * + * Deliberately computed live rather than read from a log. The failure this + * addresses is a route that produces no output at all, and the operator's + * question is always about the item in front of them right now -- so the + * useful answer names that item, that route, and the one clause that rejected + * it, rather than a cycle summary that says `no-route` about a repository. + * + * It reads history through `changesSince` and never calls `observe`, because a + * diagnostic that advanced the baseline would consume the very transition the + * operator is asking about, and answer differently the second time it is asked. + */ +function explainRouting( + deps: ApiServerDeps, + events: readonly TriggerEvent[], + routes: readonly RoutingRule[] +) { + return events.map((event) => { + const changes = deps.db.changesSince(event.projectId, event.ref, { + labels: event.labels, + assignees: event.assignees ?? [], + reviewers: event.requestedReviewers ?? [], + }) + const observed = { ...event, changes } + + const verdicts = routes.map((route) => { + const reason = explainRule(route, observed) + return { + routeId: route.id, + routeName: route.name, + matched: reason === undefined, + reason, + } + }) + + return { + ref: event.ref, + type: event.type, + projectId: event.projectId, + title: event.title, + url: event.url, + labels: event.labels, + assignees: event.assignees ?? [], + requestedReviewers: event.requestedReviewers ?? [], + /** Absent means this item has never been polled before. */ + changes, + /** + * A matching route still may not run: the dispatch ledger, the one-run- + * per-agent rule and agent resolution all come after this point, and the + * cycle summary reports those as `duplicate`, `agent-busy` and + * `unknown-agent` respectively. + */ + verdicts, + } + }) } function parsePositiveInt(raw: unknown, label: string, fallback: number): number { @@ -88,6 +155,28 @@ export async function createApiServer(deps: ApiServerDeps): Promise ({ routes: deps.getRoutes() })) + app.get('/routes/explain', async (request, reply) => { + const query = request.query as Record + const projectId = query.project + if (projectId && !deps.getProjects().some((project) => project.id === projectId)) { + return reply.code(404).send({ error: `No project "${projectId}" is configured.` }) + } + + try { + const events = await deps.collectTriggers(projectId) + const wanted = query.ref + return { + items: explainRouting( + deps, + wanted ? events.filter((event) => event.ref === wanted) : events, + deps.getRoutes() + ), + } + } catch (error: unknown) { + return reply.code(502).send({ error: error instanceof Error ? error.message : String(error) }) + } + }) + app.get('/runs', async (request, reply) => { const query = request.query as Record try { diff --git a/packages/orchestrator/test/routing/explain-api.test.ts b/packages/orchestrator/test/routing/explain-api.test.ts new file mode 100644 index 0000000..f66e7fd --- /dev/null +++ b/packages/orchestrator/test/routing/explain-api.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it } from 'vitest' +import { + TICKET_PROVIDER, + TRIGGER_TYPE, + type AppConfig, + type ProjectConfig, + type RoutingRule, + type TriggerEvent, +} from '@sentinel0/common' +import { createApiServer, type ApiServerDeps } from '../../src/runtime/api-server.js' +import { openDatabase, type Sentinel0Database } from '../../src/database.js' + +const project: ProjectConfig = { + id: 'trackside', + provider: TICKET_PROVIDER.GITHUB, + filters: { owner: 'acme', repo: 'trackside' }, +} + +const route: RoutingRule = { + id: 'rt_review', + name: 'Reviewer agent', + priority: 100, + enabled: true, + trigger: { type: TRIGGER_TYPE.PR_REVIEW_REQUESTED, projectId: 'trackside' }, + guard: { refire: 'per-change', markers: true }, + match: { reviewersAdded: { any: ['EomiAIBot'] } }, + target: { agentRef: { githubLogin: 'EomiAIBot' } }, + execution: { prompt: 'Review it', requireApproval: false, timeoutSeconds: 60 }, + outcome: {}, +} + +function reviewRequested(reviewers: string[]): TriggerEvent { + return { + type: TRIGGER_TYPE.PR_REVIEW_REQUESTED, + projectId: 'trackside', + provider: TICKET_PROVIDER.GITHUB, + ref: 'acme/trackside#7', + revision: 'r1', + title: 'Fix the export', + body: '', + labels: [], + assignees: [], + prNumber: 7, + requestedReviewers: reviewers, + isDraft: false, + baseBranch: 'main', + } +} + +/** Only what the explain route touches; the rest throws if it is reached. */ +function server(db: Sentinel0Database, events: TriggerEvent[]) { + return createApiServer({ + getConfig: () => ({ server: { apiPort: 0, networkAccess: false } }) as AppConfig, + getProjects: () => [project], + getAgents: () => [], + getRoutes: () => [route], + db, + dataDir: '/tmp', + collectTriggers: async () => events, + } as unknown as ApiServerDeps) +} + +const seen = (db: Sentinel0Database, reviewers: string[]) => + db.observe('trackside', 'acme/trackside#7', { labels: [], assignees: [], reviewers }) + +describe('GET /routes/explain', () => { + it('reports the route that matches', async () => { + const db = openDatabase('memory') + seen(db, []) + + const app = await server(db, [reviewRequested(['EomiAIBot'])]) + const response = await app.inject({ method: 'GET', url: '/routes/explain' }) + + expect(response.json().items[0].verdicts[0]).toMatchObject({ + routeId: 'rt_review', + matched: true, + }) + await app.close() + }) + + it('answers the same way twice, because asking must not consume the transition', async () => { + const db = openDatabase('memory') + seen(db, []) + + const app = await server(db, [reviewRequested(['EomiAIBot'])]) + const first = await app.inject({ method: 'GET', url: '/routes/explain' }) + const second = await app.inject({ method: 'GET', url: '/routes/explain' }) + + expect(first.json().items[0].verdicts[0].matched).toBe(true) + expect(second.json().items[0].verdicts[0].matched).toBe(true) + await app.close() + }) + + it('names the clause that rejected', async () => { + const db = openDatabase('memory') + // The reviewer was already outstanding last cycle, so nothing was added. + seen(db, ['EomiAIBot']) + + const app = await server(db, [reviewRequested(['EomiAIBot'])]) + const response = await app.inject({ method: 'GET', url: '/routes/explain' }) + + const verdict = response.json().items[0].verdicts[0] + expect(verdict.matched).toBe(false) + expect(verdict.reason).toContain('match.reviewersAdded') + await app.close() + }) + + it('rejects a project this runner does not poll', async () => { + const app = await server(openDatabase('memory'), []) + const response = await app.inject({ method: 'GET', url: '/routes/explain?project=nope' }) + + expect(response.statusCode).toBe(404) + await app.close() + }) +}) diff --git a/packages/orchestrator/test/routing/rule-engine.test.ts b/packages/orchestrator/test/routing/rule-engine.test.ts index 86b076f..f8ee512 100644 --- a/packages/orchestrator/test/routing/rule-engine.test.ts +++ b/packages/orchestrator/test/routing/rule-engine.test.ts @@ -5,7 +5,13 @@ import { type RoutingRule, type TriggerEvent, } from '@sentinel0/common' -import { dedupeKey, evaluate, matchesRule, matchesSet } from '../../src/routing/rule-engine.js' +import { + dedupeKey, + evaluate, + explainRule, + matchesRule, + matchesSet, +} from '../../src/routing/rule-engine.js' function rule(overrides: Partial = {}): RoutingRule { return { @@ -182,3 +188,69 @@ describe('dedupeKey', () => { expect(dedupeKey(rule(), event())).not.toBe(dedupeKey(rule(), event({ ref: 'LIN-999' }))) }) }) + +describe('explainRule', () => { + it('says nothing when the rule matches', () => { + expect(explainRule(rule(), event())).toBeUndefined() + }) + + it('agrees with matchesRule, which is defined in terms of it', () => { + const cases: Array<[RoutingRule, TriggerEvent]> = [ + [rule(), event()], + [rule({ enabled: false }), event()], + [rule({ match: { labels: { any: ['nope'] } } }), event()], + [rule({ trigger: { type: TRIGGER_TYPE.PR_EVENT, projectId: 'taplands' } }), event()], + [rule({ match: { titleMatches: 'billing' } }), event()], + ] + for (const [candidate, incoming] of cases) { + expect(matchesRule(candidate, incoming)).toBe(explainRule(candidate, incoming) === undefined) + } + }) + + it('names the disabled route rather than a clause', () => { + expect(explainRule(rule({ enabled: false }), event())).toBe('the route is disabled') + }) + + it('reports the clause that rejected, and what the event actually had', () => { + const reason = explainRule(rule({ match: { labels: { any: ['urgent'] } } }), event()) + expect(reason).toContain('match.labels') + expect(reason).toContain('urgent') + expect(reason).toContain('feasibility') + }) + + it('distinguishes "no history yet" from "nothing changed"', () => { + const onLabel = rule({ match: { labelsAdded: { any: ['urgent'] } } }) + + expect(explainRule(onLabel, event())).toContain('never been observed before') + expect( + explainRule( + onLabel, + event({ + changes: { + labelsAdded: [], + labelsRemoved: [], + assigneesAdded: [], + assigneesRemoved: [], + reviewersAdded: [], + }, + }) + ) + ).toContain('nothing changed there') + }) + + it('explains an agent addressed by a login nobody named on the item', () => { + const targeted = rule({ + trigger: { type: TRIGGER_TYPE.PR_EVENT, projectId: 'taplands' }, + target: { agentRef: { githubLogin: 'EomiAIBot' } }, + }) + const reason = explainRule(targeted, event({ type: TRIGGER_TYPE.PR_EVENT, assignees: [] })) + + expect(reason).toContain('EomiAIBot') + expect(reason).toContain('neither assigned to nor a requested reviewer') + }) + + it('explains a marker label holding a once-route back', () => { + const once = rule({ guard: { refire: 'once', markers: true } }) + expect(explainRule(once, event({ labels: ['sentinel0:done'] }))).toContain('re-arm') + }) +}) diff --git a/packages/orchestrator/test/routing/trigger-pipeline.test.ts b/packages/orchestrator/test/routing/trigger-pipeline.test.ts new file mode 100644 index 0000000..05fa958 --- /dev/null +++ b/packages/orchestrator/test/routing/trigger-pipeline.test.ts @@ -0,0 +1,192 @@ +import { describe, expect, it } from 'vitest' +import { + TICKET_PROVIDER, + TRIGGER_TYPE, + type ProjectConfig, + type RoutingRule, + type TriggerChanges, + type TriggerEvent, +} from '@sentinel0/common' +import type { GitHubRestClient } from '@sentinel0/common/github' +import { GitHubService } from '../../src/github/service.js' +import { openDatabase, type Sentinel0Database } from '../../src/database.js' +import { explainRule, matchesRule } from '../../src/routing/rule-engine.js' + +/** + * Collection, observation and matching wired together exactly as the poll loop + * wires them. + * + * The unit tests hand `changes` to the rule engine directly, which is what let + * a dead route ship: a `reviewersAdded` rule on `pr_review_requested` passed + * every one of them while being structurally unable to fire against real + * GitHub data, because that trigger type is only emitted once a reviewer + * exists and so had no prior sighting to compare against. + */ + +const project: ProjectConfig = { + id: 'trackside', + provider: TICKET_PROVIDER.GITHUB, + filters: { owner: 'acme', repo: 'trackside' }, +} + +const reviewRoute: RoutingRule = { + id: 'rt_review', + name: 'Reviewer agent', + priority: 100, + enabled: true, + trigger: { + type: TRIGGER_TYPE.PR_REVIEW_REQUESTED, + provider: TICKET_PROVIDER.GITHUB, + projectId: 'trackside', + }, + guard: { refire: 'per-change', markers: true }, + match: { reviewersAdded: { any: ['EomiAIBot'] } }, + target: { agentRef: { githubLogin: 'EomiAIBot' } }, + execution: { prompt: 'Review {{pr.number}}', requireApproval: false, timeoutSeconds: 60 }, + outcome: {}, +} + +function api(pulls: Array>): GitHubRestClient { + return { + issues: async () => [], + pullRequests: async () => pulls, + } as unknown as GitHubRestClient +} + +function pull(overrides: Record = {}) { + return { + number: 7, + title: 'Fix the export', + body: '', + html_url: 'https://github.com/acme/trackside/pull/7', + state: 'open', + draft: false, + updated_at: '2026-09-01T10:00:00Z', + labels: [], + assignees: [], + requested_reviewers: [], + requested_teams: [], + base: { ref: 'main' }, + ...overrides, + } +} + +/** One poll cycle, as `main()` in src/index.ts runs it. */ +async function cycle( + db: Sentinel0Database, + pulls: Array> +): Promise { + const events = await new GitHubService(api(pulls)).collect(project) + const observed = new Map() + + return events.map((event) => { + if (!observed.has(event.ref)) { + observed.set( + event.ref, + db.observe(event.projectId, event.ref, { + labels: event.labels, + assignees: event.assignees ?? [], + reviewers: event.requestedReviewers ?? [], + }) + ) + } + return { ...event, changes: observed.get(event.ref) } + }) +} + +const reviewEvent = (events: TriggerEvent[]) => + events.find((event) => event.type === TRIGGER_TYPE.PR_REVIEW_REQUESTED) + +describe('a review request, end to end', () => { + const requested = [pull({ requested_reviewers: [{ login: 'EomiAIBot' }] })] + + it('fires the review route on the cycle the reviewer is added', async () => { + const db = openDatabase('memory') + + await cycle(db, [pull()]) + const event = reviewEvent(await cycle(db, requested)) + + expect(explainRule(reviewRoute, event!)).toBeUndefined() + }) + + it('shares the pull request’s history with both events it raises', async () => { + const db = openDatabase('memory') + + await cycle(db, [pull()]) + const events = await cycle(db, requested) + + // The narrower event exists only from this cycle; its history has to come + // from the pr_event baseline seeded a cycle earlier. + for (const event of events) { + expect(event.changes?.reviewersAdded).toEqual(['EomiAIBot']) + } + }) + + it('does not fire again while the request merely stays outstanding', async () => { + const db = openDatabase('memory') + + await cycle(db, [pull()]) + await cycle(db, requested) + const event = reviewEvent(await cycle(db, requested)) + + expect(matchesRule(reviewRoute, event!)).toBe(false) + }) + + it('fires again when the review is re-requested after being answered', async () => { + const db = openDatabase('memory') + + await cycle(db, [pull()]) + await cycle(db, requested) + // Answering a review clears the request on GitHub's side. + await cycle(db, [pull()]) + const event = reviewEvent(await cycle(db, requested)) + + expect(matchesRule(reviewRoute, event!)).toBe(true) + }) + + it('stays quiet for a pull request whose reviewer was requested before it was ever seen', async () => { + const db = openDatabase('memory') + + // First sight of the item, reviewer already attached: no history, so no + // transition. This is the backlog guard, and it is one cycle of latency + // rather than a route that can never fire. + const event = reviewEvent(await cycle(db, requested)) + + expect(matchesRule(reviewRoute, event!)).toBe(false) + expect(explainRule(reviewRoute, event!)).toContain('never been observed before') + }) +}) + +describe('observations are keyed by item, not by trigger type', () => { + it('migrates a database written under the old per-type keys', async () => { + const db = openDatabase('memory') + + // What the previous release wrote: the pr_event row holds real history, + // the pr_review_requested row is the one that was useless. + const seed = (ref: string, reviewers: string[]) => + db.observe('trackside', ref, { labels: [], assignees: [], reviewers }) + + seed('pr_event:acme/trackside#7', []) + seed('pr_review_requested:acme/trackside#7', ['EomiAIBot']) + + db.migrateObservationKeys() + + // Promoted onto the bare ref, and pr_event's empty reviewer set won -- so + // the pending request still reads as newly added rather than as history. + const changes = db.changesSince('trackside', 'acme/trackside#7', { + labels: [], + assignees: [], + reviewers: ['EomiAIBot'], + }) + expect(changes?.reviewersAdded).toEqual(['EomiAIBot']) + + // And the legacy rows are gone rather than lingering until the pruner. + expect( + db.changesSince('trackside', 'pr_event:acme/trackside#7', { + labels: [], + assignees: [], + reviewers: [], + }) + ).toBeUndefined() + }) +}) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 2caca31..a50f322 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -57,10 +57,10 @@ importers: specifier: 11.2.0 version: 11.2.0 '@sentinel0/common': - specifier: 0.0.4 + specifier: 0.0.5 version: link:../common '@sentinel0/orchestrator': - specifier: 0.0.4 + specifier: 0.0.5 version: link:../orchestrator chalk: specifier: '4'