diff --git a/packages/loopover-engine/src/calibration/signal-tracking.ts b/packages/loopover-engine/src/calibration/signal-tracking.ts new file mode 100644 index 0000000000..a6a8f12858 --- /dev/null +++ b/packages/loopover-engine/src/calibration/signal-tracking.ts @@ -0,0 +1,113 @@ +// Shared deterministic-rule signal tracking (#7982) -- the deployment-agnostic primitive both ORB's gate +// blockers and AMS's eligibility/policy heuristics record through, so a systematically-wrong rule can be +// detected the same way in both subsystems instead of ORB alone having a self-correction story. +// +// SELF-CONTAINED, STORAGE-AGNOSTIC: every type + function here is pure -- no DB, no env, no host-specific +// event vocabulary. Mirrors src/review/auto-tune.ts's own FlagStore-injection precedent (see that file's +// header comment): the pure calibration math lives here, and each host (ORB, AMS) supplies its own +// `SignalStore` implementation wired to whatever it already uses for durable storage (ORB: audit_events over +// D1/Postgres; AMS: the local append-only event ledger). This module does NOT replace either of those -- +// see outcomes-wire.ts (ORB) and event-ledger.ts (AMS), which this wraps. +// +// DEFERRED (out of scope here -- foundation only, no behavior change for either consumer until #7983/#7984/ +// #7986 actually consume it): +// • the live SignalStore implementations (the ORB and AMS adapters, wired at the host layer). +// • any circuit-breaker / alerting action taken FROM a RulePrecisionReport or repeat count -- this module +// only computes the numbers, exactly like auto-tune.ts's GateEvalReport is computed elsewhere and only +// READ by the breaker logic. + +/** A single instance of a deterministic rule firing against a target -- the shared "the system made a call" + * primitive. `ruleId` is host-defined (an ORB gate-blocker code like `missing_linked_issue`, or an AMS + * eligibility-exclusion reason like `missing_eligibility_label`); `targetKey` is host-defined too (ORB: + * `owner/repo#123`; AMS: `owner/repo#issue-456`) -- this module never parses or interprets either string. */ +export type RuleFiredEvent = { + ruleId: string; + targetKey: string; + outcome: string; + occurredAt: string; + metadata?: Record; +}; + +/** A human's later, explicit judgment on a specific prior rule firing: `"reversed"` means the target should + * NOT have been blocked/excluded (the rule was wrong this time); `"confirmed"` means it should have been + * (the rule was right). Absence of an override is NOT itself a signal either way -- most fired rules never + * get an explicit human judgment, and {@link computeRulePrecision} only scores the ones that do (mirrors + * auto-tune.ts's GateEvalRow: `decided` is always <= `fired`/`wouldMerge`, never assumed equal to it). */ +export type HumanOverrideEvent = { + ruleId: string; + targetKey: string; + verdict: "reversed" | "confirmed"; + occurredAt: string; + metadata?: Record; +}; + +/** The minimal storage seam a host implements. Every method is async so a real implementation can hit a DB; + * a pure in-memory test double satisfies this trivially. Mirrors FlagStore's shape (auto-tune.ts): a small, + * named set of operations, not a generic read/write-anything interface. */ +export interface SignalStore { + recordRuleFired(event: RuleFiredEvent): Promise; + recordHumanOverride(event: HumanOverrideEvent): Promise; + /** Every fired + override event for `ruleId` at or after `sinceMs` (epoch millis), oldest first. A host MAY + * scope this further (e.g. to one repo) internally; the interface itself is unscoped beyond `ruleId`. */ + queryRuleHistory(ruleId: string, sinceMs: number): Promise<{ fired: RuleFiredEvent[]; overrides: HumanOverrideEvent[] }>; +} + +/** Per-rule confusion-style report over a window: how many times it fired, how many of those got an explicit + * human verdict, and the resulting precision. Mirrors auto-tune.ts's GateEvalRow shape (fired ~ wouldMerge/ + * wouldClose, reversed ~ mergeFalse/closeFalse) at a per-RULE grain instead of per-project -- the same + * "confirmed / decided, decided <= fired" relationship, just keyed differently. */ +export type RulePrecisionReport = { + ruleId: string; + fired: number; + reversed: number; + confirmed: number; + decided: number; + /** confirmed / decided, or null when decided === 0 (no human verdict yet -- never coerced to 0 or 1, same + * "unknown stays unknown" discipline as GateEvalRow's null precision fields). */ + precision: number | null; +}; + +/** True for a override event that targets the same rule as `ruleId` -- the shared filter both + * {@link computeRulePrecision} and any future per-target lookup would need. */ +function overrideMatchesRule(event: HumanOverrideEvent, ruleId: string): boolean { + return event.ruleId === ruleId; +} + +/** + * Compute a {@link RulePrecisionReport} for `ruleId` from its fired + override events. Only overrides whose + * `ruleId` matches are counted (a caller MAY pass a mixed-rule event list without filtering first); a + * `targetKey` that never fired but has an override is impossible by construction upstream and is simply + * counted as a decided verdict with no matching fire (does not affect `fired`, only `reversed`/`confirmed`/ + * `decided`) -- this function does not attempt to cross-validate the two lists against each other, mirroring + * computeGateEval's own "trust the caller's already-joined rows" posture. + */ +export function computeRulePrecision(ruleId: string, fired: readonly RuleFiredEvent[], overrides: readonly HumanOverrideEvent[]): RulePrecisionReport { + const firedCount = fired.reduce((count, event) => (event.ruleId === ruleId ? count + 1 : count), 0); + let reversed = 0; + let confirmed = 0; + for (const event of overrides) { + if (!overrideMatchesRule(event, ruleId)) continue; + if (event.verdict === "reversed") reversed += 1; + else confirmed += 1; + } + const decided = reversed + confirmed; + return { + ruleId, + fired: firedCount, + reversed, + confirmed, + decided, + precision: decided > 0 ? confirmed / decided : null, + }; +} + +/** + * Count how many times `ruleId` fired against the exact same `targetKey` within `fired` -- the #7983 + * "same-rule repeat alarm" primitive (a rule re-firing against a target it already fired against once is a + * stronger signal than a bare one-off fire, independent of whether either fire has been overridden yet). + * Pure counting, no time-windowing here -- a caller windows `fired` itself before calling this (e.g. via + * `queryRuleHistory`'s own `sinceMs`), matching how this whole module leaves all storage/scoping to the host. + */ +export function computeRuleRepeatCount(ruleId: string, targetKey: string, fired: readonly RuleFiredEvent[]): number { + return fired.reduce((count, event) => (event.ruleId === ruleId && event.targetKey === targetKey ? count + 1 : count), 0); +} diff --git a/packages/loopover-engine/src/index.ts b/packages/loopover-engine/src/index.ts index 72cb858ff4..609b2f2c10 100644 --- a/packages/loopover-engine/src/index.ts +++ b/packages/loopover-engine/src/index.ts @@ -162,6 +162,7 @@ export * from "./governor/run-halt.js"; export * from "./governor/kill-switch.js"; export * from "./governor/action-mode.js"; export * from "./governor/chokepoint.js"; +export * from "./calibration/signal-tracking.js"; export { GOVERNOR_LEDGER_EVENT_TYPES, normalizeGovernorLedgerEvent, diff --git a/packages/loopover-engine/test/signal-tracking.test.ts b/packages/loopover-engine/test/signal-tracking.test.ts new file mode 100644 index 0000000000..71e0d24fb6 --- /dev/null +++ b/packages/loopover-engine/test/signal-tracking.test.ts @@ -0,0 +1,92 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { computeRulePrecision, computeRuleRepeatCount, type HumanOverrideEvent, type RuleFiredEvent } from "../dist/index.js"; + +function fired(ruleId: string, targetKey: string, overrides: Partial = {}): RuleFiredEvent { + return { ruleId, targetKey, outcome: "block", occurredAt: "2026-07-22T00:00:00.000Z", ...overrides }; +} + +function override( + ruleId: string, + targetKey: string, + verdict: HumanOverrideEvent["verdict"], + overrides: Partial = {}, +): HumanOverrideEvent { + return { ruleId, targetKey, verdict, occurredAt: "2026-07-22T00:00:00.000Z", ...overrides }; +} + +test("barrel: the public entrypoint re-exports the signal-tracking primitives (#7982)", () => { + assert.equal(typeof computeRulePrecision, "function"); + assert.equal(typeof computeRuleRepeatCount, "function"); +}); + +test("computeRulePrecision: no overrides -> decided is 0 and precision is null (unknown stays unknown, never coerced)", () => { + const report = computeRulePrecision("missing_linked_issue", [fired("missing_linked_issue", "a#1"), fired("missing_linked_issue", "a#2")], []); + assert.deepEqual(report, { + ruleId: "missing_linked_issue", + fired: 2, + reversed: 0, + confirmed: 0, + decided: 0, + precision: null, + }); +}); + +test("computeRulePrecision: mixes confirmed and reversed verdicts into a real precision", () => { + const report = computeRulePrecision( + "missing_linked_issue", + [fired("missing_linked_issue", "a#1"), fired("missing_linked_issue", "a#2"), fired("missing_linked_issue", "a#3")], + [ + override("missing_linked_issue", "a#1", "confirmed"), + override("missing_linked_issue", "a#2", "confirmed"), + override("missing_linked_issue", "a#3", "reversed"), + ], + ); + assert.equal(report.fired, 3); + assert.equal(report.confirmed, 2); + assert.equal(report.reversed, 1); + assert.equal(report.decided, 3); + assert.equal(report.precision, 2 / 3); +}); + +test("computeRulePrecision: 100% reversed yields precision 0, not null (a real, scored bad outcome, not an unknown one)", () => { + const report = computeRulePrecision("bad_rule", [fired("bad_rule", "a#1")], [override("bad_rule", "a#1", "reversed")]); + assert.equal(report.decided, 1); + assert.equal(report.precision, 0); +}); + +test("computeRulePrecision: ignores fired/override events for a DIFFERENT ruleId entirely", () => { + const report = computeRulePrecision( + "rule_a", + [fired("rule_a", "a#1"), fired("rule_b", "a#2")], + [override("rule_a", "a#1", "confirmed"), override("rule_b", "a#2", "reversed")], + ); + assert.equal(report.fired, 1); + assert.equal(report.confirmed, 1); + assert.equal(report.reversed, 0); +}); + +test("computeRulePrecision: an override with no matching fired event still counts toward decided (no cross-validation between the two lists)", () => { + const report = computeRulePrecision("rule_a", [], [override("rule_a", "a#1", "confirmed")]); + assert.equal(report.fired, 0); + assert.equal(report.decided, 1); + assert.equal(report.precision, 1); +}); + +test("computeRuleRepeatCount: counts only fires matching BOTH ruleId and targetKey", () => { + const events = [ + fired("rule_a", "a#1"), + fired("rule_a", "a#1"), + fired("rule_a", "a#2"), + fired("rule_b", "a#1"), + ]; + assert.equal(computeRuleRepeatCount("rule_a", "a#1", events), 2); + assert.equal(computeRuleRepeatCount("rule_a", "a#2", events), 1); + assert.equal(computeRuleRepeatCount("rule_b", "a#1", events), 1); + assert.equal(computeRuleRepeatCount("rule_a", "a#3", events), 0); +}); + +test("computeRuleRepeatCount: zero fired events yields 0, not an error", () => { + assert.equal(computeRuleRepeatCount("rule_a", "a#1", []), 0); +}); diff --git a/packages/loopover-miner/docs/ams-signal-tracking-design.md b/packages/loopover-miner/docs/ams-signal-tracking-design.md new file mode 100644 index 0000000000..1f509264c5 --- /dev/null +++ b/packages/loopover-miner/docs/ams-signal-tracking-design.md @@ -0,0 +1,81 @@ +# AMS signal-tracking design — the #7982 shared calibration module and what AMS records into it + +Design doc for **#7982**, the "extract a shared, deployment-agnostic calibration/signal-tracking module for +ORB + AMS" foundation issue under the false-positive/self-correction roadmap (#7980). Covers: where the shared +primitive lives, the exact shape of what AMS now writes into it, and — the explicit gap this doc exists to +name — what AMS still does **not** write, and why. + +## The shared primitive + +`packages/loopover-engine/src/calibration/signal-tracking.ts` — pure, storage-agnostic, mirrors +`src/review/auto-tune.ts`'s own `FlagStore`-injection precedent: + +- `RuleFiredEvent` — a deterministic rule firing against a target (`ruleId`, `targetKey`, `outcome`, + `occurredAt`, optional `metadata`). Host-defined strings throughout; the engine module never parses either. +- `HumanOverrideEvent` — a human's later, explicit judgment on a specific prior firing (`"reversed"` — the + rule was wrong — or `"confirmed"` — it was right). +- `SignalStore` — the injected storage seam (`recordRuleFired`, `recordHumanOverride`, `queryRuleHistory`). +- `computeRulePrecision` / `computeRuleRepeatCount` — pure functions over already-fetched event lists; the + primitives #7983 (same-rule repeat alarm) and #7984 (per-rule precision tracking) build on directly. + +Two adapters implement `SignalStore`, each wrapping existing storage rather than inventing new tables: + +- **ORB**: `src/review/signal-tracking-wire.ts`, wrapping `audit_events` (via `recordAuditEvent` / + `listAuditEventsByType`, `src/db/repositories.ts`). `ruleId` is folded into `event_type` as + `signal.rule_fired:` / `signal.human_override:`, keeping a per-rule history query an + efficient index range scan (`audit_events_type_created_idx`) instead of a metadata scan. +- **AMS**: `packages/loopover-miner/lib/signal-tracking-store.ts`, wrapping the miner's local append-only + `event-ledger.ts` under two new event types (`signal_rule_fired`, `signal_human_override`). No indexed + per-rule query exists on this store, so `queryRuleHistory` scans the whole local ledger and filters + client-side — the same pattern `calibration-cli.ts`'s `toOutcomeRecords` already uses for that same ledger. + Fine at AMS's bounded, single-operator local volume; not a hosted-scale query shape. + +## What AMS now records (live, not deferred) + +`packages/miner-lib/discover-cli.ts`'s real (non-`--dry-run`) run wires the eligibility filter +(`contribution-profile-filter.ts`'s `filterCandidatesByProfiles`) to `recordRuleFired`: every candidate the +filter excludes writes one event, `ruleId` = the exclusion reason (`exclusion_label`, +`missing_eligibility_label`, `conflicting_signals`, `excluded_assignee` — see +`ELIGIBILITY_EXCLUSION_REASONS`), `targetKey` = `/#issue-`, `outcome` = `"exclude"`. + +Deliberately **not** wired on `--dry-run`: a dry run previews what a real run would do (it already uses a +no-op portfolio-queue store for the same reason) and must not itself contribute real data to a future +precision report. Deliberately best-effort: a store-open failure or a single event's write failure never +aborts discovery, matching every other optional store in this file (policy caches, ranked-candidates +snapshot). + +This closes the exact gap #7982's own audit found: `contribution-profile-filter.ts` was previously a 100% +pure function with zero persistence — when a rule excluded a candidate, nothing recorded that decision at +all, so there was no way to later ask "how often was this exclusion actually right?" + +## What AMS still does not record — the human-override gap + +`recordHumanOverride` exists on the interface and the AMS adapter implements it correctly, but **nothing in +AMS calls it yet.** This is a real, known gap, not an oversight in scope: + +ORB's human-override signal (see `src/review/outcomes-wire.ts`'s `recordReversalSignals`) has a natural +trigger: a human directly acts on the exact artifact the bot produced (reopens a bot-closed PR, reverts a +bot-merged one) — the same PR, a GitHub-native action, unambiguous provenance. + +AMS's eligibility exclusion has no equivalent natural trigger today. Excluding a candidate means AMS never +even attempts the issue — there is no AMS-authored PR, no AMS-facing artifact a human could act on to signal +"you were wrong to skip this." Discovering that an exclusion was wrong currently requires an operator to +notice, out-of-band, that AMS skipped a genuinely-eligible issue (e.g. by reading `discover --json`'s +`excluded` field themselves) — and nothing today captures that observation back into the ledger. + +**This is explicitly out of scope for #7982** (foundation only) but is the concrete, actionable follow-up a +future sub-issue should own. Two candidate designs, neither implemented here: + +1. **Operator-driven**: a `loopover-miner discover mark-eligible #` command that looks up the + most recent `signal_rule_fired` event for that target and writes a matching `recordHumanOverride("reversed")` + — cheap, but requires an operator to actually run it. +2. **Signal-driven**: if a repo's `ContributionProfile` is later re-extracted (profiles are re-resolved + periodically) and an issue that was previously excluded would now be *kept* under the fresh profile, treat + that transition as an implicit `"reversed"` signal for the original exclusion. Requires diffing two + `filterCandidatesByProfiles` runs over time, which discover-cli.ts does not currently retain. + +Either design is a real, scoped follow-up issue, not a blocking dependency of #7983/#7984/#7986 — both of +those can compute meaningful repeat-count/precision reports from `recordRuleFired` data alone; precision +reports will simply show `decided: 0, precision: null` for every AMS rule until an override path exists, +which `computeRulePrecision`'s own contract already represents correctly (unknown stays unknown, never +coerced to "always right"). diff --git a/packages/loopover-miner/lib/discover-cli.ts b/packages/loopover-miner/lib/discover-cli.ts index 3f0225fe94..17e831933c 100644 --- a/packages/loopover-miner/lib/discover-cli.ts +++ b/packages/loopover-miner/lib/discover-cli.ts @@ -35,7 +35,9 @@ import type { ContributionProfile } from "./contribution-profile.js"; import { argsWantJson, describeCliError, reportCliFailure } from "./cli-error.js"; import { isDiscoveryPlaneEnabled, queryDiscoveryIndex, recordDiscoveryTelemetry } from "./discovery-index-client.js"; import type { queryDiscoveryIndex as QueryDiscoveryIndexFn } from "./discovery-index-client.js"; -import type { DiscoveryIndexQuery } from "@loopover/engine"; +import type { DiscoveryIndexQuery, SignalStore } from "@loopover/engine"; +import { appendEvent, readEvents } from "./event-ledger.js"; +import { createSignalTrackingStore } from "./signal-tracking-store.js"; export type ParsedDiscoverArgs = @@ -100,6 +102,10 @@ export type RunDiscoverOptions = { initPolicyDocCache?: () => PolicyDocCacheStore; initPolicyVerdictCache?: () => PolicyVerdictCacheStore; initRankedCandidatesStore?: () => RankedCandidatesStore; + /** #7982: records each real-run eligibility exclusion as a rule-fired signal, so it can later be scored for + * precision the same way ORB's own gate blockers will be. Same "nice to have, own try/catch, degrade to a + * no-op" discipline as the caches/stores above -- a signal-tracking write failure must never abort discovery. */ + initSignalTrackingStore?: () => SignalStore; fetchCandidateIssuesWithSummary?: ( targets: FanoutTarget[], githubToken: string, @@ -277,6 +283,44 @@ function renderRateLimitLine(result: Pick, + options: Pick, +): Promise { + if (excluded.length === 0) return; + let store: SignalStore | null = null; + try { + store = (options.initSignalTrackingStore ?? initDefaultSignalTrackingStore)(); + } catch { + store = null; + } + if (!store) return; + const occurredAt = new Date(options.nowMs ?? Date.now()).toISOString(); + for (const entry of excluded) { + await store + .recordRuleFired({ + ruleId: entry.reason, + targetKey: `${entry.candidate.repoFullName}#issue-${entry.candidate.issueNumber}`, + outcome: "exclude", + occurredAt, + }) + .catch(() => undefined); + } +} + export function renderDiscoverSummary(result: DiscoverResult): string { const lines = [ `fanned out: ${result.fanOutCount} candidate issue(s)`, @@ -547,6 +591,7 @@ export async function runDiscover(args: string[], options: RunDiscoverOptions = fanOut.issues, profilesByRepo as Map, ); + await recordEligibilityExclusionSignals(excluded, options); // Pass any caller-supplied per-tenant goal specs through to the ranker so lane fit uses the tenant's // conventions instead of silently falling back to loopover's defaults (#4784); the fallback is surfaced via diff --git a/packages/loopover-miner/lib/signal-tracking-store.ts b/packages/loopover-miner/lib/signal-tracking-store.ts new file mode 100644 index 0000000000..6deb90190a --- /dev/null +++ b/packages/loopover-miner/lib/signal-tracking-store.ts @@ -0,0 +1,133 @@ +// AMS adapter for @loopover/engine's shared signal-tracking primitive (#7982). WRAPS the miner's existing +// local, append-only event-ledger.js -- no new table, no new storage mechanism -- the same "reuse, don't +// rewrite" contract ORB's own adapter (src/review/signal-tracking-wire.ts) follows for audit_events. +// +// Event-ledger vocabulary: two typed event kinds, mirroring MINER_PR_OUTCOME_EVENT's own naming convention +// (pr-outcome.ts). ruleId/outcome/verdict/extra metadata live in the ledger's `payload` (a plain JSON object, +// the ledger's own storage unit) -- there is no indexed column to fold ruleId into the way ORB's audit_events +// event_type affords, so queryRuleHistory reads the WHOLE ledger and filters client-side, mirroring +// calibration-cli.ts's toOutcomeRecords (the ledger's only other "scan + typed filter" reader). Fine for AMS's +// bounded, single-operator local volume; not a hosted-scale query pattern -- a future issue can index this if +// it ever needs to be. + +import type { HumanOverrideEvent, RuleFiredEvent, SignalStore } from "@loopover/engine"; + +import type { AppendEventInput, LedgerEntry, ReadEventsFilter } from "./event-ledger.js"; + +export const SIGNAL_RULE_FIRED_EVENT = "signal_rule_fired" as const; +export const SIGNAL_HUMAN_OVERRIDE_EVENT = "signal_human_override" as const; + +/** The minimal event-ledger surface this adapter needs -- same "reuse the real interface, don't invent a + * narrower one" shape as pr-outcome.ts's own RecordPrOutcomeOptions.eventLedger, so a genuine EventLedger + * (not just a same-shaped stub) satisfies this without a cast. */ +export type SignalTrackingLedger = { + appendEvent(event: AppendEventInput): LedgerEntry; + readEvents(filter?: ReadEventsFilter): LedgerEntry[]; +}; + +type RuleFiredPayload = { ruleId: string; targetKey: string; outcome: string; occurredAt: string; metadata?: Record }; +type HumanOverridePayload = { ruleId: string; targetKey: string; verdict: "reversed" | "confirmed"; occurredAt: string; metadata?: Record }; + +function toRuleFiredPayload(event: RuleFiredEvent): RuleFiredPayload { + return { + ruleId: event.ruleId, + targetKey: event.targetKey, + outcome: event.outcome, + occurredAt: event.occurredAt, + ...(event.metadata ? { metadata: event.metadata } : {}), + }; +} + +function toHumanOverridePayload(event: HumanOverrideEvent): HumanOverridePayload { + return { + ruleId: event.ruleId, + targetKey: event.targetKey, + verdict: event.verdict, + occurredAt: event.occurredAt, + ...(event.metadata ? { metadata: event.metadata } : {}), + }; +} + +/** Best-effort `owner/repo` scope for the ledger row, parsed from targetKey's `owner/repo#...` convention (the + * same shape ORB's own targetKey uses, e.g. `owner/repo#123`). A targetKey that doesn't match this shape + * stays UNSCOPED (repoFullName omitted) rather than guessing wrong -- a wrong scope would make the row + * permanently invisible to a repo-filtered read, which is worse than just being unscoped. */ +function repoFullNameFromTargetKey(targetKey: string): string | undefined { + const match = /^([^/]+\/[^/#]+)#/.exec(targetKey); + return match?.[1]; +} + +/** True when `payload` is a well-formed {@link RuleFiredPayload} for exactly `ruleId` -- both the type guard + * AND the ruleId filter in one check, since every caller of this immediately wants both. A payload that + * doesn't match (wrong ruleId, or missing/wrong-typed fields from some other event this adapter didn't + * write) is silently skipped by the caller, never thrown on -- the ledger holds every miner event type, not + * just this adapter's own. */ +function isRuleFiredPayload(payload: Record, ruleId: string): payload is RuleFiredPayload { + return payload.ruleId === ruleId && typeof payload.targetKey === "string" && typeof payload.outcome === "string" && typeof payload.occurredAt === "string"; +} + +/** The override-side mirror of {@link isRuleFiredPayload}. */ +function isHumanOverridePayload(payload: Record, ruleId: string): payload is HumanOverridePayload { + return ( + payload.ruleId === ruleId && + typeof payload.targetKey === "string" && + (payload.verdict === "reversed" || payload.verdict === "confirmed") && + typeof payload.occurredAt === "string" + ); +} + +/** + * Local, event-ledger-backed {@link SignalStore} for AMS. `eventLedger` is REQUIRED (not defaulted to a + * module-level singleton) — same discipline as pr-outcome.ts's own `RecordPrOutcomeOptions.eventLedger`: the + * caller already owns the ledger's open/close lifecycle (a real SQLite file handle), so this adapter never + * opens or closes one itself. + */ +export function createSignalTrackingStore(eventLedger: SignalTrackingLedger): SignalStore { + return { + async recordRuleFired(event: RuleFiredEvent): Promise { + const repoFullName = repoFullNameFromTargetKey(event.targetKey); + eventLedger.appendEvent({ + type: SIGNAL_RULE_FIRED_EVENT, + ...(repoFullName ? { repoFullName } : {}), + payload: toRuleFiredPayload(event), + }); + }, + async recordHumanOverride(event: HumanOverrideEvent): Promise { + const repoFullName = repoFullNameFromTargetKey(event.targetKey); + eventLedger.appendEvent({ + type: SIGNAL_HUMAN_OVERRIDE_EVENT, + ...(repoFullName ? { repoFullName } : {}), + payload: toHumanOverridePayload(event), + }); + }, + async queryRuleHistory(ruleId: string, sinceMs: number): Promise<{ fired: RuleFiredEvent[]; overrides: HumanOverrideEvent[] }> { + const sinceIso = new Date(sinceMs).toISOString(); + const fired: RuleFiredEvent[] = []; + const overrides: HumanOverrideEvent[] = []; + // ISO 8601 UTC timestamps (every occurredAt/createdAt in this module) compare correctly as plain + // strings -- same assumption the SQL `created_at >= ?` comparisons elsewhere in this codebase already + // rely on -- so no Date parsing is needed just to filter the window. + for (const entry of eventLedger.readEvents()) { + if (entry.createdAt < sinceIso) continue; + if (entry.type === SIGNAL_RULE_FIRED_EVENT && isRuleFiredPayload(entry.payload, ruleId)) { + fired.push({ + ruleId, + targetKey: entry.payload.targetKey, + outcome: entry.payload.outcome, + occurredAt: entry.payload.occurredAt, + ...(entry.payload.metadata ? { metadata: entry.payload.metadata } : {}), + }); + } else if (entry.type === SIGNAL_HUMAN_OVERRIDE_EVENT && isHumanOverridePayload(entry.payload, ruleId)) { + overrides.push({ + ruleId, + targetKey: entry.payload.targetKey, + verdict: entry.payload.verdict, + occurredAt: entry.payload.occurredAt, + ...(entry.payload.metadata ? { metadata: entry.payload.metadata } : {}), + }); + } + } + return { fired, overrides }; + }, + }; +} diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 65b7ec979e..5c2a7ee91c 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -3521,6 +3521,41 @@ export async function listAuditEventsForTarget( })); } +/** A raw `audit_events` row projected for a caller that keys its own domain data inside `metadataJson` rather + * than the fixed `outcome`/`detail` columns — #7982's `signal.rule_fired:*`/`signal.human_override:*` event + * types are the first consumer (see `src/review/signal-tracking-wire.ts`). `metadata` is best-effort parsed: + * a corrupt row (should never happen — every writer round-trips through `jsonString`) degrades to `{}` rather + * than throwing, since a single bad row must never break a whole precision report. */ +export type AuditEventByType = { + targetKey: string | null; + detail: string | null; + metadata: Record; + createdAt: string; +}; + +/** Every `audit_events` row for an EXACT `eventType`, at or after `sinceIso`, oldest first (the read order a + * precision/repeat-count report over time needs — unlike {@link listAuditEventsForTarget}'s newest-first + * "recent activity" order). Unscoped by target/actor: matches this table's `audit_events_type_created_idx` + * index exactly, so a caller querying one event type over a window stays an efficient index range scan. */ +export async function listAuditEventsByType(env: Env, eventType: string, sinceIso: string, limit = 500): Promise { + const rows = await getDb(env.DB) + .select({ targetKey: auditEvents.targetKey, detail: auditEvents.detail, metadataJson: auditEvents.metadataJson, createdAt: auditEvents.createdAt }) + .from(auditEvents) + .where(and(eq(auditEvents.eventType, eventType), gte(auditEvents.createdAt, sinceIso))) + .orderBy(asc(auditEvents.createdAt), asc(auditEvents.id)) + .limit(clampInteger(limit, 1, 2000)); + return rows.map((row) => { + let metadata: Record = {}; + try { + const parsed: unknown = JSON.parse(row.metadataJson); + if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) metadata = parsed as Record; + } catch { + /* corrupt row -- fail open to {} per this function's own doc comment */ + } + return { targetKey: row.targetKey, detail: row.detail, metadata, createdAt: row.createdAt }; + }); +} + export async function getFreshOfficialMinerDetection(env: Env, login: string, now = nowIso()): Promise { const [row] = await getDb(env.DB).select().from(officialMinerDetections).where(and(eq(officialMinerDetections.login, login.toLowerCase()), gte(officialMinerDetections.expiresAt, now))).limit(1); return row ? toOfficialMinerDetection(row) : null; diff --git a/src/review/signal-tracking-wire.ts b/src/review/signal-tracking-wire.ts new file mode 100644 index 0000000000..60d27c10d7 --- /dev/null +++ b/src/review/signal-tracking-wire.ts @@ -0,0 +1,108 @@ +// ORB adapter for @loopover/engine's shared signal-tracking primitive (#7982). WRAPS the existing audit_events +// store (via recordAuditEvent/listAuditEventsByType, db/repositories.ts) — this file intentionally contains no +// new schema, no new table, and no gate-decision logic of its own. It does NOT replace outcomes-wire.ts's +// pr_outcome/reversal system; that stays the ground-truth source for ORB's existing merge/close precision +// breaker (auto-tune.ts). This adapter exists so a NEW rule-level signal (starting with #7983/#7984/#7986) can +// be recorded the same way AMS's own adapter (packages/loopover-miner/lib/signal-tracking-store.ts) records +// its eligibility/policy calls, without either side reinventing storage. +// +// Event-type encoding: `ruleId` is folded directly into audit_events.event_type (`signal.rule_fired:`, +// `signal.human_override:`) rather than left in metadata — audit_events already carries a +// (event_type, created_at) index, so a per-rule history query stays an efficient index range scan instead of a +// metadata JSON scan. The rest of the event (target, domain-specific outcome/verdict, extra metadata) lives in +// metadataJson, read back via listAuditEventsByType. + +import type { HumanOverrideEvent, RuleFiredEvent, SignalStore } from "@loopover/engine"; + +import { listAuditEventsByType, recordAuditEvent } from "../db/repositories"; +import { nowIso } from "../utils/json"; + +const RULE_FIRED_EVENT_TYPE_PREFIX = "signal.rule_fired:"; +const HUMAN_OVERRIDE_EVENT_TYPE_PREFIX = "signal.human_override:"; + +function ruleFiredEventType(ruleId: string): string { + return `${RULE_FIRED_EVENT_TYPE_PREFIX}${ruleId}`; +} + +function humanOverrideEventType(ruleId: string): string { + return `${HUMAN_OVERRIDE_EVENT_TYPE_PREFIX}${ruleId}`; +} + +/** Reconstruct a {@link RuleFiredEvent} from an `audit_events` row written by {@link createSignalStore}'s + * `recordRuleFired`. `ruleId` comes from the CALLER (the query was already scoped to one rule's event_type), + * not re-parsed from the row — mirrors how the row itself never duplicates it into metadata. A row with a + * missing/non-string `outcome` in its metadata (should never happen — see the doc comment on + * {@link listAuditEventsByType}) degrades to an empty string rather than throwing, keeping a report over a + * large window resilient to one bad row. */ +function toRuleFiredEvent(ruleId: string, row: { targetKey: string | null; metadata: Record; createdAt: string }): RuleFiredEvent { + const outcome = typeof row.metadata.outcome === "string" ? row.metadata.outcome : ""; + const extraMetadata = { ...row.metadata }; + delete extraMetadata.outcome; + return { + ruleId, + targetKey: row.targetKey ?? "", + outcome, + occurredAt: row.createdAt, + ...(Object.keys(extraMetadata).length > 0 ? { metadata: extraMetadata } : {}), + }; +} + +/** Reconstruct a {@link HumanOverrideEvent}, the override-side mirror of {@link toRuleFiredEvent}. A row whose + * metadata `verdict` isn't exactly `"reversed"`/`"confirmed"` degrades to `"confirmed"` (fail toward NOT + * inflating the reversal count on corrupt data) rather than throwing. */ +function toHumanOverrideEvent(ruleId: string, row: { targetKey: string | null; metadata: Record; createdAt: string }): HumanOverrideEvent { + const verdict = row.metadata.verdict === "reversed" ? "reversed" : "confirmed"; + const extraMetadata = { ...row.metadata }; + delete extraMetadata.verdict; + return { + ruleId, + targetKey: row.targetKey ?? "", + verdict, + occurredAt: row.createdAt, + ...(Object.keys(extraMetadata).length > 0 ? { metadata: extraMetadata } : {}), + }; +} + +/** Live, D1/Postgres-backed {@link SignalStore} for ORB. Every write is best-effort (`.catch(() => undefined)`, + * matching every other audit-event write in this codebase, e.g. outcomes-wire.ts's `recordAuditEvent` calls) — + * a failure to record a signal must never fail the review pass that produced it. Reads (`queryRuleHistory`) + * are NOT fail-open the same way: a read error propagates, since a caller computing a precision report needs + * to know its input is incomplete rather than silently scoring against a partial (possibly empty) history. + */ +export function createSignalStore(env: Env): SignalStore { + return { + async recordRuleFired(event: RuleFiredEvent): Promise { + await recordAuditEvent(env, { + eventType: ruleFiredEventType(event.ruleId), + actor: "loopover", + targetKey: event.targetKey, + outcome: "completed", + detail: `rule ${event.ruleId} fired (${event.outcome}) against ${event.targetKey}`, + metadata: { outcome: event.outcome, ...(event.metadata ?? {}) }, + createdAt: event.occurredAt || nowIso(), + }).catch(() => undefined); + }, + async recordHumanOverride(event: HumanOverrideEvent): Promise { + await recordAuditEvent(env, { + eventType: humanOverrideEventType(event.ruleId), + actor: "human", + targetKey: event.targetKey, + outcome: "completed", + detail: `human ${event.verdict} rule ${event.ruleId} against ${event.targetKey}`, + metadata: { verdict: event.verdict, ...(event.metadata ?? {}) }, + createdAt: event.occurredAt || nowIso(), + }).catch(() => undefined); + }, + async queryRuleHistory(ruleId: string, sinceMs: number): Promise<{ fired: RuleFiredEvent[]; overrides: HumanOverrideEvent[] }> { + const sinceIso = new Date(sinceMs).toISOString(); + const [firedRows, overrideRows] = await Promise.all([ + listAuditEventsByType(env, ruleFiredEventType(ruleId), sinceIso), + listAuditEventsByType(env, humanOverrideEventType(ruleId), sinceIso), + ]); + return { + fired: firedRows.map((row) => toRuleFiredEvent(ruleId, row)), + overrides: overrideRows.map((row) => toHumanOverrideEvent(ruleId, row)), + }; + }, + }; +} diff --git a/test/unit/miner-discover-cli.test.ts b/test/unit/miner-discover-cli.test.ts index edcedf880d..ffdada3a22 100644 --- a/test/unit/miner-discover-cli.test.ts +++ b/test/unit/miner-discover-cli.test.ts @@ -1,7 +1,8 @@ import { existsSync, mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { afterEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { closeDefaultEventLedger } from "../../packages/loopover-miner/lib/event-ledger.js"; import { initPolicyDocCacheStore } from "../../packages/loopover-miner/lib/policy-doc-cache.js"; import { initPolicyVerdictCacheStore } from "../../packages/loopover-miner/lib/policy-verdict-cache.js"; import { @@ -22,6 +23,21 @@ const NOW = Date.parse("2026-07-09T12:00:00.000Z"); const roots: string[] = []; const stores: Array<{ close(): void }> = []; +// #7982: none of this file's runDiscover calls inject initSignalTrackingStore (most don't produce any +// excluded candidates, and the ones that do only assert on payload.excluded/portfolioQueue, not on +// signal-tracking itself -- that gets its own dedicated coverage below). Without this redirect, the DEFAULT +// SignalStore falls back to the real on-disk event ledger under ~/.config/loopover-miner, exactly the +// LOOPOVER_MINER_CONFIG_DIR redirect tempPolicyDocCacheStore's own comment already warns about for the OTHER +// default-store fallbacks -- same class of leak, same fix. closeDefaultEventLedger() resets the module-level +// singleton each test so a later test's redirected path is never masked by an earlier one's cached handle. +let previousConfigDir: string | undefined; +beforeEach(() => { + const root = mkdtempSync(join(tmpdir(), "loopover-miner-discover-cli-ledger-")); + roots.push(root); + previousConfigDir = process.env.LOOPOVER_MINER_CONFIG_DIR; + process.env.LOOPOVER_MINER_CONFIG_DIR = root; +}); + function tempQueueStore() { const root = mkdtempSync(join(tmpdir(), "loopover-miner-discover-cli-")); roots.push(root); @@ -105,6 +121,9 @@ function indexCandidate(overrides: Record = {}) { afterEach(() => { for (const store of stores.splice(0)) store.close(); closeDefaultPortfolioQueueStore(); + closeDefaultEventLedger(); + if (previousConfigDir === undefined) delete process.env.LOOPOVER_MINER_CONFIG_DIR; + else process.env.LOOPOVER_MINER_CONFIG_DIR = previousConfigDir; vi.restoreAllMocks(); for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); @@ -1623,6 +1642,108 @@ describe("runDiscover onResult hook (#6522)", () => { ).toEqual(["issue:1"]); }); + describe("eligibility-exclusion signal tracking (#7982)", () => { + function fakeSignalStore() { + const fired: Array<{ ruleId: string; targetKey: string; outcome: string }> = []; + return { + fired, + store: { + recordRuleFired: vi.fn(async (event: { ruleId: string; targetKey: string; outcome: string }) => { + fired.push({ ruleId: event.ruleId, targetKey: event.targetKey, outcome: event.outcome }); + }), + recordHumanOverride: vi.fn(async () => undefined), + queryRuleHistory: vi.fn(async () => ({ fired: [], overrides: [] })), + }, + }; + } + + it("records a rule-fired signal for every real-run exclusion, keyed by reason + repo#issue-N", async () => { + const issues = [ + fanOutIssue({ issueNumber: 1, labels: ["help wanted"] }), + fanOutIssue({ issueNumber: 2, labels: ["blocked"] }), + fanOutIssue({ issueNumber: 3, labels: ["bug"] }), + ]; + const { opts } = discoverWith(issues, new Map([["acme/widgets", trustworthyProfile]])); + const { fired, store } = fakeSignalStore(); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + const exitCode = await runDiscover(["acme/widgets", "--json"], { ...opts, initSignalTrackingStore: () => store }); + expect(exitCode).toBe(0); + expect(fired).toEqual([ + { ruleId: "exclusion_label", targetKey: "acme/widgets#issue-2", outcome: "exclude" }, + { ruleId: "missing_eligibility_label", targetKey: "acme/widgets#issue-3", outcome: "exclude" }, + ]); + }); + + it("records nothing when nothing was excluded", async () => { + const issues = [fanOutIssue({ issueNumber: 1, labels: ["help wanted"] })]; + const { opts } = discoverWith(issues, new Map([["acme/widgets", trustworthyProfile]])); + const { fired, store } = fakeSignalStore(); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + await runDiscover(["acme/widgets", "--json"], { ...opts, initSignalTrackingStore: () => store }); + expect(fired).toEqual([]); + expect(store.recordRuleFired).not.toHaveBeenCalled(); + }); + + it("never records anything on a --dry-run, even when the same run would exclude candidates for real", async () => { + const issues = [ + fanOutIssue({ issueNumber: 1, labels: ["help wanted"] }), + fanOutIssue({ issueNumber: 2, labels: ["blocked"] }), + ]; + const { opts } = discoverWith(issues, new Map([["acme/widgets", trustworthyProfile]])); + const { fired, store } = fakeSignalStore(); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + const exitCode = await runDiscover(["acme/widgets", "--json", "--dry-run"], { ...opts, initSignalTrackingStore: () => store }); + expect(exitCode).toBe(0); + const payload = JSON.parse(String(log.mock.calls[0]?.[0])); + expect(payload.excluded).toHaveLength(1); + expect(fired).toEqual([]); + expect(store.recordRuleFired).not.toHaveBeenCalled(); + }); + + it("a store-open failure degrades to a no-op rather than aborting discovery", async () => { + const issues = [ + fanOutIssue({ issueNumber: 1, labels: ["help wanted"] }), + fanOutIssue({ issueNumber: 2, labels: ["blocked"] }), + ]; + const { opts } = discoverWith(issues, new Map([["acme/widgets", trustworthyProfile]])); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + const exitCode = await runDiscover(["acme/widgets", "--json"], { + ...opts, + initSignalTrackingStore: () => { + throw new Error("store unavailable"); + }, + }); + expect(exitCode).toBe(0); + const payload = JSON.parse(String(log.mock.calls[0]?.[0])); + expect(payload.excluded).toHaveLength(1); + }); + + it("a per-event recording failure is swallowed and does not stop the remaining events from being recorded", async () => { + const issues = [ + fanOutIssue({ issueNumber: 1, labels: ["help wanted"] }), + fanOutIssue({ issueNumber: 2, labels: ["blocked"] }), + fanOutIssue({ issueNumber: 3, labels: ["bug"] }), + ]; + const { opts } = discoverWith(issues, new Map([["acme/widgets", trustworthyProfile]])); + let calls = 0; + const recordRuleFired = vi.fn(async () => { + calls += 1; + if (calls === 1) throw new Error("write failed"); + }); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + const exitCode = await runDiscover(["acme/widgets", "--json"], { + ...opts, + initSignalTrackingStore: () => ({ + recordRuleFired, + recordHumanOverride: vi.fn(async () => undefined), + queryRuleHistory: vi.fn(async () => ({ fired: [], overrides: [] })), + }), + }); + expect(exitCode).toBe(0); + expect(recordRuleFired).toHaveBeenCalledTimes(2); + }); + }); + it("SAFE DEFAULT: filters nothing for a low-confidence/empty profile", async () => { const emptyProfile = { ...trustworthyProfile, diff --git a/test/unit/miner-signal-tracking-store.test.ts b/test/unit/miner-signal-tracking-store.test.ts new file mode 100644 index 0000000000..a015545b2c --- /dev/null +++ b/test/unit/miner-signal-tracking-store.test.ts @@ -0,0 +1,182 @@ +import { describe, expect, it } from "vitest"; +import { + SIGNAL_HUMAN_OVERRIDE_EVENT, + SIGNAL_RULE_FIRED_EVENT, + createSignalTrackingStore, +} from "../../packages/loopover-miner/lib/signal-tracking-store.js"; +import type { AppendEventInput, LedgerEntry } from "../../packages/loopover-miner/lib/event-ledger.js"; + +const ONE_HOUR_MS = 60 * 60 * 1000; + +// Same mock-ledger shape as test/unit/miner-pr-outcome.test.ts's own mockLedger, so these stay pure unit tests +// with no SQLite file. Typed against the real EventLedger contract so it can't silently drift. +function mockLedger(): { + appendEvent: (e: AppendEventInput) => LedgerEntry; + readEvents: () => LedgerEntry[]; + _events: LedgerEntry[]; +} { + const events: LedgerEntry[] = []; + let seq = 0; + return { + appendEvent: (e) => { + const entry: LedgerEntry = { id: ++seq, seq, type: e.type, repoFullName: e.repoFullName ?? null, payload: e.payload, createdAt: new Date().toISOString() }; + events.push(entry); + return entry; + }, + readEvents: () => events, + _events: events, + }; +} + +describe("createSignalTrackingStore (#7982) — recordRuleFired + queryRuleHistory round-trip", () => { + it("records a rule-fired event and reads it back with its outcome and target intact", async () => { + const ledger = mockLedger(); + const store = createSignalTrackingStore(ledger); + const now = Date.now(); + const occurredAt = new Date(now).toISOString(); + await store.recordRuleFired({ ruleId: "missing_eligibility_label", targetKey: "owner/repo#issue-5", outcome: "exclude", occurredAt }); + const history = await store.queryRuleHistory("missing_eligibility_label", now - ONE_HOUR_MS); + expect(history.fired).toEqual([ + { ruleId: "missing_eligibility_label", targetKey: "owner/repo#issue-5", outcome: "exclude", occurredAt }, + ]); + expect(history.overrides).toEqual([]); + }); + + it("preserves extra metadata on a fired event, separately from the domain outcome", async () => { + const ledger = mockLedger(); + const store = createSignalTrackingStore(ledger); + const now = Date.now(); + const occurredAt = new Date(now).toISOString(); + await store.recordRuleFired({ + ruleId: "missing_eligibility_label", + targetKey: "owner/repo#issue-5", + outcome: "exclude", + occurredAt, + metadata: { profileConfidence: "explicit" }, + }); + const history = await store.queryRuleHistory("missing_eligibility_label", now - ONE_HOUR_MS); + expect(history.fired[0]?.metadata).toEqual({ profileConfidence: "explicit" }); + }); + + it("records a human-override event and reads it back with its verdict intact", async () => { + const ledger = mockLedger(); + const store = createSignalTrackingStore(ledger); + const now = Date.now(); + const occurredAt = new Date(now).toISOString(); + await store.recordHumanOverride({ ruleId: "missing_eligibility_label", targetKey: "owner/repo#issue-5", verdict: "reversed", occurredAt }); + const history = await store.queryRuleHistory("missing_eligibility_label", now - ONE_HOUR_MS); + expect(history.overrides).toEqual([ + { ruleId: "missing_eligibility_label", targetKey: "owner/repo#issue-5", verdict: "reversed", occurredAt }, + ]); + expect(history.fired).toEqual([]); + }); + + it("writes each event under the correct event-ledger type constant", async () => { + const ledger = mockLedger(); + const store = createSignalTrackingStore(ledger); + const occurredAt = new Date().toISOString(); + await store.recordRuleFired({ ruleId: "rule_a", targetKey: "owner/repo#1", outcome: "block", occurredAt }); + await store.recordHumanOverride({ ruleId: "rule_a", targetKey: "owner/repo#1", verdict: "confirmed", occurredAt }); + expect(ledger._events.map((e) => e.type)).toEqual([SIGNAL_RULE_FIRED_EVENT, SIGNAL_HUMAN_OVERRIDE_EVENT]); + }); + + it("scopes the ledger row's repoFullName from a well-formed owner/repo#N targetKey", async () => { + const ledger = mockLedger(); + const store = createSignalTrackingStore(ledger); + await store.recordRuleFired({ ruleId: "rule_a", targetKey: "owner/repo#123", outcome: "block", occurredAt: new Date().toISOString() }); + expect(ledger._events[0]?.repoFullName).toBe("owner/repo"); + }); + + it("leaves the ledger row unscoped (null repoFullName) when targetKey doesn't match the owner/repo#N shape", async () => { + const ledger = mockLedger(); + const store = createSignalTrackingStore(ledger); + await store.recordRuleFired({ ruleId: "rule_a", targetKey: "not-a-real-target", outcome: "block", occurredAt: new Date().toISOString() }); + expect(ledger._events[0]?.repoFullName).toBeNull(); + }); + + it("never mixes events for a DIFFERENT ruleId into the query result", async () => { + const ledger = mockLedger(); + const store = createSignalTrackingStore(ledger); + const occurredAt = new Date().toISOString(); + await store.recordRuleFired({ ruleId: "rule_a", targetKey: "owner/repo#1", outcome: "block", occurredAt }); + await store.recordRuleFired({ ruleId: "rule_b", targetKey: "owner/repo#2", outcome: "block", occurredAt }); + const history = await store.queryRuleHistory("rule_a", Date.now() - ONE_HOUR_MS); + expect(history.fired).toHaveLength(1); + expect(history.fired[0]?.ruleId).toBe("rule_a"); + }); + + it("keeps fired and override events for the SAME ruleId in separate buckets, never cross-contaminating", async () => { + const ledger = mockLedger(); + const store = createSignalTrackingStore(ledger); + const occurredAt = new Date().toISOString(); + await store.recordRuleFired({ ruleId: "rule_a", targetKey: "owner/repo#1", outcome: "block", occurredAt }); + await store.recordHumanOverride({ ruleId: "rule_a", targetKey: "owner/repo#1", verdict: "confirmed", occurredAt }); + const history = await store.queryRuleHistory("rule_a", Date.now() - ONE_HOUR_MS); + expect(history.fired).toHaveLength(1); + expect(history.overrides).toHaveLength(1); + }); + + it("excludes an event older than sinceMs", async () => { + const ledger = mockLedger(); + // Manually seed an old event -- the mock's appendEvent always stamps "now", so an explicit direct push is + // how this test controls createdAt, same technique test/unit/miner-pr-outcome.test.ts's own suite uses via + // its exposed `_events` array. + ledger._events.push({ + id: 1, + seq: 1, + type: SIGNAL_RULE_FIRED_EVENT, + repoFullName: "owner/repo", + payload: { ruleId: "rule_a", targetKey: "owner/repo#1", outcome: "block", occurredAt: new Date(Date.now() - 2 * ONE_HOUR_MS).toISOString() }, + createdAt: new Date(Date.now() - 2 * ONE_HOUR_MS).toISOString(), + }); + const store = createSignalTrackingStore(ledger); + const history = await store.queryRuleHistory("rule_a", Date.now() - ONE_HOUR_MS); + expect(history.fired).toEqual([]); + }); + + it("skips a ledger row for the target ruleId whose payload is malformed, without throwing", async () => { + const ledger = mockLedger(); + ledger._events.push({ + id: 1, + seq: 1, + type: SIGNAL_RULE_FIRED_EVENT, + repoFullName: "owner/repo", + payload: { ruleId: "rule_a", targetKey: "owner/repo#1" /* missing outcome/occurredAt */ }, + createdAt: new Date().toISOString(), + }); + const store = createSignalTrackingStore(ledger); + const history = await store.queryRuleHistory("rule_a", Date.now() - ONE_HOUR_MS); + expect(history.fired).toEqual([]); + }); + + it("skips a ledger row whose verdict isn't 'reversed' or 'confirmed', without throwing", async () => { + const ledger = mockLedger(); + ledger._events.push({ + id: 1, + seq: 1, + type: SIGNAL_HUMAN_OVERRIDE_EVENT, + repoFullName: "owner/repo", + payload: { ruleId: "rule_a", targetKey: "owner/repo#1", verdict: "maybe", occurredAt: new Date().toISOString() }, + createdAt: new Date().toISOString(), + }); + const store = createSignalTrackingStore(ledger); + const history = await store.queryRuleHistory("rule_a", Date.now() - ONE_HOUR_MS); + expect(history.overrides).toEqual([]); + }); + + it("ignores an unrelated event type entirely (the ledger holds every miner event kind, not just this adapter's own)", async () => { + const ledger = mockLedger(); + ledger._events.push({ + id: 1, + seq: 1, + type: "pr_outcome", + repoFullName: "owner/repo", + payload: { prNumber: 1, decision: "merged" }, + createdAt: new Date().toISOString(), + }); + const store = createSignalTrackingStore(ledger); + const history = await store.queryRuleHistory("rule_a", Date.now() - ONE_HOUR_MS); + expect(history.fired).toEqual([]); + expect(history.overrides).toEqual([]); + }); +}); diff --git a/test/unit/signal-tracking-wire.test.ts b/test/unit/signal-tracking-wire.test.ts new file mode 100644 index 0000000000..246a74cc15 --- /dev/null +++ b/test/unit/signal-tracking-wire.test.ts @@ -0,0 +1,273 @@ +import { describe, expect, it } from "vitest"; + +import { listAuditEventsByType, recordAuditEvent } from "../../src/db/repositories"; +import { createSignalStore } from "../../src/review/signal-tracking-wire"; +import { createTestEnv } from "../helpers/d1"; + +const ONE_HOUR_MS = 60 * 60 * 1000; + +// Fixed reference point (not Date.now() at test-file-load time) so every event's occurredAt/sinceMs stays +// relative to the SAME instant throughout a single test, regardless of how long a real session has been +// running when this file executes. +function isoOffset(baseMs: number, deltaMs: number): string { + return new Date(baseMs + deltaMs).toISOString(); +} + +describe("createSignalStore (#7982) — recordRuleFired + queryRuleHistory round-trip", () => { + it("records a rule-fired event and reads it back with its outcome and target intact", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const now = Date.now(); + const occurredAt = isoOffset(now, 0); + await store.recordRuleFired({ + ruleId: "missing_linked_issue", + targetKey: "owner/repo#123", + outcome: "block", + occurredAt, + }); + const history = await store.queryRuleHistory("missing_linked_issue", now - ONE_HOUR_MS); + expect(history.fired).toEqual([ + { + ruleId: "missing_linked_issue", + targetKey: "owner/repo#123", + outcome: "block", + occurredAt, + }, + ]); + expect(history.overrides).toEqual([]); + }); + + it("preserves extra metadata on a fired event, separately from the domain outcome", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const now = Date.now(); + const occurredAt = isoOffset(now, 0); + await store.recordRuleFired({ + ruleId: "missing_eligibility_label", + targetKey: "owner/repo#issue-5", + outcome: "exclude", + occurredAt, + metadata: { profileConfidence: "explicit" }, + }); + const history = await store.queryRuleHistory("missing_eligibility_label", now - ONE_HOUR_MS); + expect(history.fired).toEqual([ + { + ruleId: "missing_eligibility_label", + targetKey: "owner/repo#issue-5", + outcome: "exclude", + occurredAt, + metadata: { profileConfidence: "explicit" }, + }, + ]); + }); + + it("preserves extra metadata on an override event, separately from the verdict", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const now = Date.now(); + const occurredAt = isoOffset(now, 0); + await store.recordHumanOverride({ + ruleId: "missing_linked_issue", + targetKey: "owner/repo#123", + verdict: "confirmed", + occurredAt, + metadata: { reviewer: "maintainer" }, + }); + const history = await store.queryRuleHistory("missing_linked_issue", now - ONE_HOUR_MS); + expect(history.overrides).toEqual([ + { + ruleId: "missing_linked_issue", + targetKey: "owner/repo#123", + verdict: "confirmed", + occurredAt, + metadata: { reviewer: "maintainer" }, + }, + ]); + }); + + it("records a human-override event and reads it back with its verdict intact", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const now = Date.now(); + const occurredAt = isoOffset(now, 5 * 60 * 1000); + await store.recordHumanOverride({ + ruleId: "missing_linked_issue", + targetKey: "owner/repo#123", + verdict: "reversed", + occurredAt, + }); + const history = await store.queryRuleHistory("missing_linked_issue", now - ONE_HOUR_MS); + expect(history.overrides).toEqual([ + { + ruleId: "missing_linked_issue", + targetKey: "owner/repo#123", + verdict: "reversed", + occurredAt, + }, + ]); + expect(history.fired).toEqual([]); + }); + + it("keeps fired and override events for the SAME ruleId in separate buckets, never cross-contaminating", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const now = Date.now(); + await store.recordRuleFired({ ruleId: "rule_a", targetKey: "owner/repo#1", outcome: "block", occurredAt: isoOffset(now, 0) }); + await store.recordHumanOverride({ ruleId: "rule_a", targetKey: "owner/repo#1", verdict: "confirmed", occurredAt: isoOffset(now, 60_000) }); + const history = await store.queryRuleHistory("rule_a", now - ONE_HOUR_MS); + expect(history.fired).toHaveLength(1); + expect(history.overrides).toHaveLength(1); + expect(history.overrides[0]?.verdict).toBe("confirmed"); + }); + + it("never mixes events for a DIFFERENT ruleId into the query result", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const now = Date.now(); + await store.recordRuleFired({ ruleId: "rule_a", targetKey: "owner/repo#1", outcome: "block", occurredAt: isoOffset(now, 0) }); + await store.recordRuleFired({ ruleId: "rule_b", targetKey: "owner/repo#2", outcome: "block", occurredAt: isoOffset(now, 0) }); + const history = await store.queryRuleHistory("rule_a", now - ONE_HOUR_MS); + expect(history.fired).toHaveLength(1); + expect(history.fired[0]?.ruleId).toBe("rule_a"); + }); + + it("excludes an event older than sinceMs", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const now = Date.now(); + await store.recordRuleFired({ ruleId: "rule_a", targetKey: "owner/repo#1", outcome: "block", occurredAt: isoOffset(now, -2 * ONE_HOUR_MS) }); + const history = await store.queryRuleHistory("rule_a", now - ONE_HOUR_MS); + expect(history.fired).toEqual([]); + }); + + it("returns fired events oldest-first, matching a precision/trend report's natural read order", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const now = Date.now(); + await store.recordRuleFired({ ruleId: "rule_a", targetKey: "owner/repo#1", outcome: "block", occurredAt: isoOffset(now, 2 * 60_000) }); + await store.recordRuleFired({ ruleId: "rule_a", targetKey: "owner/repo#2", outcome: "block", occurredAt: isoOffset(now, 0) }); + await store.recordRuleFired({ ruleId: "rule_a", targetKey: "owner/repo#3", outcome: "block", occurredAt: isoOffset(now, 60_000) }); + const history = await store.queryRuleHistory("rule_a", now - 24 * ONE_HOUR_MS); + expect(history.fired.map((event) => event.targetKey)).toEqual(["owner/repo#2", "owner/repo#3", "owner/repo#1"]); + }); + + it("a recording failure is swallowed, never thrown into the caller (best-effort, matching every other audit-event write)", async () => { + const env = { ...createTestEnv(), DB: null } as unknown as ReturnType; + const store = createSignalStore(env); + const occurredAt = new Date().toISOString(); + await expect( + store.recordRuleFired({ ruleId: "rule_a", targetKey: "owner/repo#1", outcome: "block", occurredAt }), + ).resolves.toBeUndefined(); + await expect( + store.recordHumanOverride({ ruleId: "rule_a", targetKey: "owner/repo#1", verdict: "confirmed", occurredAt }), + ).resolves.toBeUndefined(); + }); + + it("a read failure PROPAGATES rather than failing open — a caller must know its history is incomplete, not silently score against a partial one", async () => { + const env = { ...createTestEnv(), DB: null } as unknown as ReturnType; + const store = createSignalStore(env); + await expect(store.queryRuleHistory("rule_a", Date.now() - ONE_HOUR_MS)).rejects.toBeDefined(); + }); + + it("an empty occurredAt falls back to the current time instead of writing a blank createdAt", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const before = Date.now(); + await store.recordRuleFired({ ruleId: "rule_a", targetKey: "owner/repo#1", outcome: "block", occurredAt: "" }); + await store.recordHumanOverride({ ruleId: "rule_a", targetKey: "owner/repo#1", verdict: "confirmed", occurredAt: "" }); + const after = Date.now(); + const history = await store.queryRuleHistory("rule_a", before - ONE_HOUR_MS); + expect(history.fired).toHaveLength(1); + expect(history.overrides).toHaveLength(1); + const firedAtMs = new Date(history.fired[0]?.occurredAt ?? "").getTime(); + const overrideAtMs = new Date(history.overrides[0]?.occurredAt ?? "").getTime(); + expect(firedAtMs).toBeGreaterThanOrEqual(before); + expect(firedAtMs).toBeLessThanOrEqual(after); + expect(overrideAtMs).toBeGreaterThanOrEqual(before); + expect(overrideAtMs).toBeLessThanOrEqual(after); + }); + + // #7982-defensive: these three simulate a row that never should exist (every real writer round-trips + // through recordRuleFired/recordHumanOverride, which always produce a string outcome/valid verdict and a + // real targetKey) by writing directly through recordAuditEvent -- the same "a single bad row must never + // break a whole precision report" contract listAuditEventsByType's own doc comment states. + it("a fired row with a non-string/missing metadata.outcome degrades to an empty string rather than throwing", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const now = Date.now(); + await recordAuditEvent(env, { + eventType: "signal.rule_fired:rule_a", + actor: "loopover", + targetKey: "owner/repo#1", + outcome: "completed", + metadata: {}, + createdAt: isoOffset(now, 0), + }); + const history = await store.queryRuleHistory("rule_a", now - ONE_HOUR_MS); + expect(history.fired).toEqual([{ ruleId: "rule_a", targetKey: "owner/repo#1", outcome: "", occurredAt: isoOffset(now, 0) }]); + }); + + it("a row with a null targetKey degrades to an empty string rather than null", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const now = Date.now(); + await recordAuditEvent(env, { + eventType: "signal.rule_fired:rule_a", + actor: "loopover", + targetKey: null, + outcome: "completed", + metadata: { outcome: "block" }, + createdAt: isoOffset(now, 0), + }); + const history = await store.queryRuleHistory("rule_a", now - ONE_HOUR_MS); + expect(history.fired[0]?.targetKey).toBe(""); + }); + + it("an override row with a verdict other than 'reversed' and a null targetKey degrades to 'confirmed' + '' -- fails toward NOT inflating the reversal count", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const now = Date.now(); + await recordAuditEvent(env, { + eventType: "signal.human_override:rule_a", + actor: "human", + targetKey: null, + outcome: "completed", + metadata: {}, + createdAt: isoOffset(now, 0), + }); + const history = await store.queryRuleHistory("rule_a", now - ONE_HOUR_MS); + expect(history.overrides[0]?.verdict).toBe("confirmed"); + expect(history.overrides[0]?.targetKey).toBe(""); + }); +}); + +// db/repositories.ts's listAuditEventsByType, tested directly (not through the adapter above) — a corrupt +// metadata_json value can only ever reach a real row via something OTHER than recordAuditEvent (which always +// round-trips through jsonString, producing valid JSON object text), so these simulate that with a raw INSERT. +describe("listAuditEventsByType (#7982) — corrupt-row resilience", () => { + it("a metadata_json that parses but isn't an object degrades to {} rather than throwing", async () => { + const env = createTestEnv(); + const nowIso = new Date().toISOString(); + await env.DB.prepare( + "INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?)", + ) + .bind("corrupt-array", "signal.rule_fired:rule_a", "loopover", "owner/repo#1", "completed", null, "[1,2,3]", nowIso) + .run(); + const rows = await listAuditEventsByType(env, "signal.rule_fired:rule_a", new Date(Date.now() - ONE_HOUR_MS).toISOString()); + expect(rows).toHaveLength(1); + expect(rows[0]?.metadata).toEqual({}); + }); + + it("an invalid (unparseable) metadata_json degrades to {} rather than throwing", async () => { + const env = createTestEnv(); + const nowIso = new Date().toISOString(); + await env.DB.prepare( + "INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?)", + ) + .bind("corrupt-json", "signal.rule_fired:rule_a", "loopover", "owner/repo#1", "completed", null, "{not valid json", nowIso) + .run(); + const rows = await listAuditEventsByType(env, "signal.rule_fired:rule_a", new Date(Date.now() - ONE_HOUR_MS).toISOString()); + expect(rows).toHaveLength(1); + expect(rows[0]?.metadata).toEqual({}); + }); +});