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'