From 3fabbd475ebefa2f214a2e6f9308158f45a634d2 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 01:36:57 -0700 Subject: [PATCH 1/4] fix(gate,review,ops): add glob exclusions, tier the public-comment vocabulary, and make inert config queryable (#9554, #9555, #9433) --- .../src/lib/selfhost-env-reference.ts | 15 +++ .../src/review/screenshot-table-gate.ts | 12 +- .../src/signals/change-guardrail.ts | 41 +++++++ src/queue-intelligence.ts | 51 ++++++-- src/selfhost/inert-config.ts | 109 ++++++++++++++++++ src/selfhost/metrics.ts | 1 + src/server.ts | 7 ++ src/signals/change-guardrail.ts | 1 + test/unit/change-guardrail.test.ts | 61 +++++++++- test/unit/queue-intelligence.test.ts | 42 ++++++- .../unit/screenshot-table-gate-engine.test.ts | 11 ++ test/unit/selfhost-inert-config.test.ts | 88 ++++++++++++++ 12 files changed, 423 insertions(+), 16 deletions(-) create mode 100644 src/selfhost/inert-config.ts create mode 100644 test/unit/selfhost-inert-config.test.ts diff --git a/apps/loopover-ui/src/lib/selfhost-env-reference.ts b/apps/loopover-ui/src/lib/selfhost-env-reference.ts index a1ee96f9ee..088ec6bd88 100644 --- a/apps/loopover-ui/src/lib/selfhost-env-reference.ts +++ b/apps/loopover-ui/src/lib/selfhost-env-reference.ts @@ -285,6 +285,14 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [ name: "LOOPOVER_METRICS_REPO_LABELS", firstReference: "src/server.ts", }, + { + name: "LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS", + firstReference: "src/selfhost/inert-config.ts", + }, + { + name: "LOOPOVER_PUBLIC_STATS_REPOS", + firstReference: "src/selfhost/inert-config.ts", + }, { name: "LOOPOVER_REPO_CONFIG_DIR", firstReference: "src/server.ts", @@ -297,6 +305,10 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [ name: "LOOPOVER_REVIEW_RAG", firstReference: "src/selfhost/ai.ts", }, + { + name: "LOOPOVER_REVIEW_SAFETY", + firstReference: "src/selfhost/inert-config.ts", + }, { name: "LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS", firstReference: "src/server.ts", @@ -744,9 +756,12 @@ export const SELFHOST_ENV_REFERENCE_MARKDOWN = [ "| `LOOPOVER_ENABLE_UNSAFE_CODEX_REVIEWER` | `src/selfhost/ai.ts` |", "| `LOOPOVER_MCP_TOKEN` | `src/selfhost/preflight.ts` |", "| `LOOPOVER_METRICS_REPO_LABELS` | `src/server.ts` |", + "| `LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS` | `src/selfhost/inert-config.ts` |", + "| `LOOPOVER_PUBLIC_STATS_REPOS` | `src/selfhost/inert-config.ts` |", "| `LOOPOVER_REPO_CONFIG_DIR` | `src/server.ts` |", "| `LOOPOVER_REVIEW_CONTINUOUS` | `src/queue/processors.ts` |", "| `LOOPOVER_REVIEW_RAG` | `src/selfhost/ai.ts` |", + "| `LOOPOVER_REVIEW_SAFETY` | `src/selfhost/inert-config.ts` |", "| `LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS` | `src/server.ts` |", "| `LOOPOVER_SINGLE_INSTANCE` | `src/selfhost/redis-cache.ts` |", "| `LOOPOVER_VERSION` | `src/selfhost/otel.ts` |", diff --git a/packages/loopover-engine/src/review/screenshot-table-gate.ts b/packages/loopover-engine/src/review/screenshot-table-gate.ts index 72ac0febf3..431a3df1c2 100644 --- a/packages/loopover-engine/src/review/screenshot-table-gate.ts +++ b/packages/loopover-engine/src/review/screenshot-table-gate.ts @@ -1,4 +1,4 @@ -import { matchesAny } from "../signals/change-guardrail.js"; +import { matchesAnyWithExclusions } from "../signals/change-guardrail.js"; import type { ScreenshotTableGateAction, ScreenshotTableGateConfig } from "../types/manifest-deps-types.js"; export type { ScreenshotTableGateAction, ScreenshotTableGateConfig } from "../types/manifest-deps-types.js"; @@ -174,7 +174,12 @@ export function hasCommittedImageFile(changedFiles: string[], scopedPaths: strin return changedFiles.some((file) => { const lower = file.toLowerCase(); if (!IMAGE_EXTENSIONS.some((ext) => lower.endsWith(ext))) return false; - return scopedPaths.length === 0 || matchesAny(file, scopedPaths); + // #9434: exclusion-aware, so an operator can scope "this directory except these generated files" + // (e.g. "apps/loopover-ui/public/**" minus "!apps/loopover-ui/public/openapi.json") instead of being + // forced to enumerate every safe subpath. Must stay the SAME matcher isScreenshotTableGateInScope uses + // below -- both read config.whenPaths, and disagreeing on which paths count as "scoped" would make a + // path excluded from gate SCOPE still count as a stray committed image, or vice versa. + return scopedPaths.length === 0 || matchesAnyWithExclusions(file, scopedPaths); }); } @@ -312,7 +317,8 @@ export function isScreenshotTableGateInScope(config: ScreenshotTableGateConfig, if (config.whenLabels.length === 0 && config.whenPaths.length === 0) return true; const wantedLabels = new Set(config.whenLabels.map((label) => label.toLowerCase())); const labelMatch = config.whenLabels.length > 0 && prLabels.some((label) => wantedLabels.has(label.toLowerCase())); - const pathMatch = config.whenPaths.length > 0 && changedFiles.some((file) => matchesAny(file, config.whenPaths)); + // #9434: exclusion-aware -- see hasCommittedImageFile's own comment on why the two must share one matcher. + const pathMatch = config.whenPaths.length > 0 && changedFiles.some((file) => matchesAnyWithExclusions(file, config.whenPaths)); return labelMatch || pathMatch; } diff --git a/packages/loopover-engine/src/signals/change-guardrail.ts b/packages/loopover-engine/src/signals/change-guardrail.ts index 5f0a1902aa..e6a14969fd 100644 --- a/packages/loopover-engine/src/signals/change-guardrail.ts +++ b/packages/loopover-engine/src/signals/change-guardrail.ts @@ -116,6 +116,47 @@ export function matchesAny(path: string, globs: string[]): boolean { return globs.some((g) => hasUnsafeWildcardCount(g) || globToRegExp(g).test(canonicalPath)); } +/** + * #9434: `matchesAny` is deliberately positive-only — there is no way to say "this directory, except these + * generated files" without enumerating every safe subpath by hand. That gap is exactly what let + * `apps/loopover-ui/public/**` (a directory that is overwhelmingly non-visual: a generated openapi.json, + * robots.txt, sitemap.xml, favicons) sit in a screenshotTableGate `whenPaths` list and auto-close 5 + * contributor PRs one-shot for regenerating a JSON spec file, with zero recovery — a one-shot close has none. + * Confirmed the identical glob shape independently present in two sibling repos' private configs. + * + * A glob prefixed with `!` here is an EXCLUSION: a path counts as matched only if it hits at least one + * INCLUDE glob and hits NO exclude glob. This is deliberately a SEPARATE function, not a change to + * `matchesAny` itself — `matchesAny` is also the hard-guardrail matcher, where an unrecognized/malformed + * glob shape failing towards "still matches" is the safety-correct default (see its own doc comment); adding + * `!`-parsing there would mean a maintainer's literal path starting with `!` (rare, but not impossible) is + * silently reinterpreted as an exclusion instead of guarding it. This function is opt-in for callers that + * explicitly want exclusion semantics (today: the screenshotTableGate `whenPaths` scope check) and leaves + * every existing `matchesAny` caller — including hardGuardrailGlobs — byte-identical. + * + * The INCLUDE half reuses `matchesAny` unchanged, so an over-complex include glob keeps its existing + * fail-toward-matching default. The EXCLUDE half deliberately does NOT reuse `matchesAny` for this: failing + * an unsafe glob toward "matches" is safe for an include (worst case, more paths are considered in scope) but + * WRONG for an exclude (an over-complex exclude glob resolving to "matches everything" would silently widen + * what gets excluded, shrinking a safety gate's coverage — the opposite failure direction from what the gate + * exists for). The exclude half therefore compiles each glob with `globToRegExp` directly, whose own + * NEVER_MATCHES fallback for an unsafe glob is exactly right here: a malformed/pathological exclude excludes + * NOTHING rather than excluding everything, so gate coverage can only ever be too WIDE from a bad exclude + * glob, never too narrow. + */ +export function matchesAnyWithExclusions(path: string, globs: string[]): boolean { + const includes: string[] = []; + const excludes: string[] = []; + for (const glob of globs) { + if (glob.startsWith("!") && glob.length > 1) excludes.push(glob.slice(1)); + else includes.push(glob); + } + if (includes.length === 0) return false; + if (!matchesAny(path, includes)) return false; + if (excludes.length === 0) return true; + const canonicalPath = canonicalize(path); + return !excludes.some((exclude) => globToRegExp(exclude).test(canonicalPath)); +} + /** * The changed paths (if any) that trip a hard guardrail. A non-empty result means the PR touches a guarded * path and MUST fall through to a human — loopover may neither auto-merge nor auto-close it. Pure. diff --git a/src/queue-intelligence.ts b/src/queue-intelligence.ts index 67838da668..51352273c5 100644 --- a/src/queue-intelligence.ts +++ b/src/queue-intelligence.ts @@ -40,7 +40,34 @@ const HIGH_ISSUE_QUALITY_THRESHOLD = 0.7; const PENDING_STALE_THRESHOLD_DAYS = 2; const MILLISECONDS_PER_DAY = 1000 * 60 * 60 * 24; -export const FORBIDDEN_PUBLIC_COMMENT_WORDS = [ +/** + * #9432: ordinary English that carries gittensor meaning ONLY in context. Each of these is a word a perfectly + * safe review of this codebase's own gate/scoring code uses naturally -- "updates the ranking comparator", + * "this improves reviewability", "the reward path". They are still matched by default (unchanged), but a repo + * that has POSITIVELY confirmed via {@link isPublicScoreTermSafeForRepo}'s allowlist that this vocabulary is + * safe public language for it can exempt them, exactly as bare "score" already could. + * + * WHY THIS IS SAFE, and why the split is where it is: the risk this filter exists to stop is a leaked private + * VALUE, and a value never appears as a bare noun -- it appears qualified ("reward estimate 12 TAO", "trust + * score 0.82", "score preview"). Every one of those QUALIFIED forms lives in + * {@link ALWAYS_FORBIDDEN_PUBLIC_COMMENT_WORDS} below and stays enforced for every repo, allowlisted or not. + * A bare "reward" or "ranking" with no number attached leaks nothing on its own. This generalizes the exact + * judgment `allowBareScoreTerm` already made for "score" to its siblings, rather than inventing a new one. + */ +export const AMBIGUOUS_PUBLIC_COMMENT_WORDS = [ + "rewards", + "reward", + "farming", + "rankings", + "ranking", + "cohort", + "reviewability", +] as const; + +/** Terms that name a private concept outright and are NEVER exemptible, for any repo. A leaked value is + * necessarily qualified (see AMBIGUOUS_PUBLIC_COMMENT_WORDS' rationale), so every qualified form belongs + * here -- this list is what actually holds the public/private boundary. */ +export const ALWAYS_FORBIDDEN_PUBLIC_COMMENT_WORDS = [ "wallet", "hotkey", "raw trust score", @@ -53,12 +80,8 @@ export const FORBIDDEN_PUBLIC_COMMENT_WORDS = [ "reward estimate", "estimated rewards", "estimated reward", - "rewards", - "reward", - "farming", "private reviewability", "reviewability internals", - "reviewability", "private scoreability", "scoreability", "score preview", @@ -67,15 +90,19 @@ export const FORBIDDEN_PUBLIC_COMMENT_WORDS = [ "score estimate", "private rankings", "private ranking", - "rankings", - "ranking", - "cohort", "miner-originated", "miner originated", "human-originated", "human originated", ] as const; +/** Every forbidden term, both tiers -- the default (non-exempt) matching set. Preserved as a single exported + * list so existing consumers and the redaction-parity test keep one canonical vocabulary to compare against. */ +export const FORBIDDEN_PUBLIC_COMMENT_WORDS = [ + ...ALWAYS_FORBIDDEN_PUBLIC_COMMENT_WORDS, + ...AMBIGUOUS_PUBLIC_COMMENT_WORDS, +] as const; + // A bare "score" is checked separately from the substring list above (not folded in as another entry): // FORBIDDEN_PUBLIC_COMMENT_WORDS is matched with a plain case-insensitive `.includes()`, and an unqualified // "score" substring also matches ordinary English words that carry no gittensor meaning at all ("underscore", @@ -218,7 +245,13 @@ export function shouldWarnPublicScoreTermsAllowlistUnset(env: Record; + +function unset(value: string | undefined): boolean { + return (value ?? "").trim() === ""; +} + +/** + * Every config-gated behaviour currently inert in `env`, in a stable order. + * + * PURE — no IO, no clock, no logging — so the `/metrics` gauge, a `/ready` field, and a test can all read the + * identical answer rather than three hand-maintained lists drifting apart (which is the same drift class this + * whole issue is about). + */ +export function inertConfigEntries(env: InertConfigEnv): InertConfigEntry[] { + const entries: InertConfigEntry[] = []; + + // The confirmed #9433 instance. Fail-closed by design, and the fail-closed direction is content-destroying: + // sanitizePublicComment THROWS on a bare "score" match and the caller degrades to a generic placeholder, so + // an unset allowlist silently strips narrative sentences on every repo. Correct for a deployment whose repos + // genuinely carry private trust/reward data, hence deployment-specific rather than always-wrong. + if (unset(env.LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS)) { + entries.push({ + key: "LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS", + impact: + "AI review narratives lose any sentence using ordinary scoring vocabulary (\"score\", \"ranking\", \"reward\", \"reviewability\"), silently — the published summary looks fine, just shorter.", + severity: "deployment-specific", + }); + } + + // Verified unset on the ORB self-host box and CORRECT there: the public-stats surface runs on the Cloudflare + // Worker, whose wrangler.jsonc sets it. Reported (never warned) precisely so an operator on a runtime that + // DOES serve /v1/public/stats can see that the own-ledger half is publishing zeros. + if (unset(env.LOOPOVER_PUBLIC_STATS_REPOS)) { + entries.push({ + key: "LOOPOVER_PUBLIC_STATS_REPOS", + impact: + "The own-ledger half of /v1/public/stats reports zero (disposition counts, reversal-grounded accuracy, weekly totals). Harmless on a runtime that does not serve public stats; silently wrong on one that does.", + severity: "deployment-specific", + }); + } + + // Redaction is flag-gated while DETECTION is not (verified: reviewInputHasPromptInjection and its hold run + // unconditionally), so an unset flag never lets a manipulated verdict through — it only means the reviewer + // sees the raw injected text rather than a defanged copy. Reported because the inconclusive-finding copy + // tells a reader the content "was redacted before review", which is untrue while this is off. + if (unset(env.LOOPOVER_REVIEW_SAFETY)) { + entries.push({ + key: "LOOPOVER_REVIEW_SAFETY", + impact: + "Prompt-injection text is still DETECTED and still holds the PR, but is not defanged before the model sees it — and the public finding claims it was redacted.", + severity: "deployment-specific", + }); + } + + return entries; +} + +/** Stable, bounded label values for the `/metrics` gauge — the key set is fixed in code, so cardinality is + * bounded by construction and an operator can alert on a specific key without a cardinality risk. */ +export function inertConfigGaugeSamples(env: InertConfigEnv): Array<{ labels: Record; value: number }> { + return inertConfigEntries(env).map((entry) => ({ + labels: { key: entry.key, severity: entry.severity }, + value: 1, + })); +} diff --git a/src/selfhost/metrics.ts b/src/selfhost/metrics.ts index 206ff96ca1..0b5beff5cf 100644 --- a/src/selfhost/metrics.ts +++ b/src/selfhost/metrics.ts @@ -57,6 +57,7 @@ export const DEFAULT_METRIC_META: readonly (readonly [string, MetricMeta])[] = [ ["loopover_backup_acknowledged", { help: "1 when SQLite backup is acknowledged or Postgres is in use; 0 when the boot backup advisory would fire.", type: "gauge" }], ["loopover_config_dir_empty_acknowledged", { help: "1 when LOOPOVER_REPO_CONFIG_DIR is unset, has entries, or is acknowledged; 0 when it's configured but the mounted directory is empty.", type: "gauge" }], ["loopover_http_requests_total", { help: "HTTP app requests by response status class.", type: "counter" }], + ["loopover_inert_config", { help: "One series per config-gated behaviour currently INERT on this instance (#9433) — labelled by env var key and whether an unset value is always wrong or deployment-specific. No series ⇒ nothing inert. See src/selfhost/inert-config.ts.", type: "gauge" }], ["loopover_http_request_duration_seconds", { help: "HTTP app request duration in seconds, labelled by bounded route group (see httpRouteGroup).", type: "histogram" }], ["loopover_visual_capture_total", { help: "Visual capture attempts by result -- a browserless outage is otherwise invisible while it silently degrades screenshots to dash cells, and the screenshot gate treats absent evidence as a close signal (#9487).", type: "counter" }], ["loopover_webhook_dedup_total", { help: "Webhook deliveries deduplicated before enqueue.", type: "counter" }], diff --git a/src/server.ts b/src/server.ts index b8bcd92c5c..0571639981 100644 --- a/src/server.ts +++ b/src/server.ts @@ -32,6 +32,7 @@ import { withAiGenerationCapture, } from "./selfhost/ai"; import { shouldWarnPublicScoreTermsAllowlistUnset } from "./queue-intelligence"; +import { inertConfigGaugeSamples } from "./selfhost/inert-config"; import { cookieValue, credentialsToEnv, @@ -1007,6 +1008,12 @@ async function main(): Promise { // when the probe is disabled or has never completed) so the metric names/HELP/TYPE lines are present on // the very first scrape, matching the seeded-counter convention below. gauge("loopover_d1_database_size_bytes", () => d1DatabaseSizeBytesSample()); + // #9433: which config-gated behaviours are currently INERT on this box. A gauge, not a boot warning: the + // same unset value is correct on one runtime and a defect on another (LOOPOVER_PUBLIC_STATS_REPOS is unset + // and CORRECT here, since the Worker serves public stats), so warning unconditionally would be steady-state + // noise -- the alert-fatigue failure this repo has already been bitten by. Bounded cardinality: the key set + // is a fixed list in code. Absent series ⇒ nothing inert. + gaugeVector("loopover_inert_config", () => inertConfigGaugeSamples(process.env)); gaugeVector("loopover_d1_table_row_count", () => d1TableRowCountSamples()); gauge("loopover_signal_snapshots_rows_per_key", () => d1SignalSnapshotsRowsPerKeySample()); // #9136: the generalizable fix — the NEXT review-source orphaning (a table a downstream module treats as diff --git a/src/signals/change-guardrail.ts b/src/signals/change-guardrail.ts index 6bd235d882..d32f2e5948 100644 --- a/src/signals/change-guardrail.ts +++ b/src/signals/change-guardrail.ts @@ -13,6 +13,7 @@ export { hasUnsafeWildcardCount, globToRegExp, matchesAny, + matchesAnyWithExclusions, changedPathsHittingGuardrail, type GuardrailPathMatch, guardrailPathMatches, diff --git a/test/unit/change-guardrail.test.ts b/test/unit/change-guardrail.test.ts index 17e44c84ca..efe8cc289a 100644 --- a/test/unit/change-guardrail.test.ts +++ b/test/unit/change-guardrail.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { changedPathsHittingGuardrail, globToRegExp, GUARDRAIL_UNVERIFIABLE_FILE_COUNT, guardrailPathMatches, isGuardrailHit, matchesAny } from "../../src/signals/change-guardrail"; +import { changedPathsHittingGuardrail, globToRegExp, GUARDRAIL_UNVERIFIABLE_FILE_COUNT, guardrailPathMatches, isGuardrailHit, matchesAny, matchesAnyWithExclusions } from "../../src/signals/change-guardrail"; describe("globToRegExp (the exported compiler itself — must be safe for ANY direct caller, not just matchesAny)", () => { it("compiles an ordinary glob to a working anchored RegExp", () => { @@ -212,3 +212,62 @@ describe("configured hard-guardrail glob examples", () => { } }); }); + +// #9434: matchesAny is positive-only, and that gap let "apps/loopover-ui/public/**" (overwhelmingly +// non-visual: a generated openapi.json, robots.txt, sitemap.xml, favicons) sit in a screenshotTableGate +// whenPaths list and auto-close 5 contributor PRs one-shot for regenerating a JSON spec file. There was no +// way to say "this directory except these generated files" without enumerating every safe subpath. +describe("matchesAnyWithExclusions (#9434)", () => { + it("REGRESSION: excludes a specific file from an otherwise-matching directory glob — the metagraphed#8038-adjacent shape", () => { + const globs = ["apps/loopover-ui/public/**", "!apps/loopover-ui/public/openapi.json"]; + expect(matchesAnyWithExclusions("apps/loopover-ui/public/openapi.json", globs)).toBe(false); + expect(matchesAnyWithExclusions("apps/loopover-ui/public/favicon.ico", globs)).toBe(true); + }); + + it("INVARIANT: with no exclusions at all, behaves identically to matchesAny", () => { + const globs = ["src/**", "apps/loopover-ui/src/**"]; + for (const path of ["src/a.ts", "apps/loopover-ui/src/routes/x.tsx", "unrelated/file.md"]) { + expect(matchesAnyWithExclusions(path, globs)).toBe(matchesAny(path, globs)); + } + }); + + it("INVARIANT: a path matching NO include glob is false, regardless of excludes", () => { + expect(matchesAnyWithExclusions("docs/readme.md", ["src/**", "!src/generated/**"])).toBe(false); + }); + + it("INVARIANT: excludes with no includes at all never match anything — an all-exclude list is not an implicit wildcard include", () => { + expect(matchesAnyWithExclusions("src/a.ts", ["!src/generated/**"])).toBe(false); + expect(matchesAnyWithExclusions("anything/at/all.ts", ["!**"])).toBe(false); + }); + + it("INVARIANT: multiple excludes all apply — matching ANY exclude removes the path", () => { + const globs = ["src/**", "!src/generated/**", "!src/vendor/**"]; + expect(matchesAnyWithExclusions("src/generated/x.ts", globs)).toBe(false); + expect(matchesAnyWithExclusions("src/vendor/y.ts", globs)).toBe(false); + expect(matchesAnyWithExclusions("src/real/z.ts", globs)).toBe(true); + }); + + it("INVARIANT: a bare '!' with nothing after it is treated as a literal include glob, not a malformed exclusion", () => { + // length > 1 guard: "!" alone can't be sliced into a meaningful exclude pattern, so it stays an include + // (and, being an unusual literal, simply won't match real paths -- never silently swallowed). + expect(matchesAnyWithExclusions("!", ["!"])).toBe(true); + }); + + it("SECURITY (ReDoS): an exclude glob is bound by the SAME wildcard cap as an include -- it cannot smuggle in a pathological pattern", () => { + const pathological = "!src/*-*-*-final.ts"; // 3 wildcard groups, over the safety cap + const adversarialPath = "src/" + "a-".repeat(2000) + "X"; + const start = Date.now(); + // hasUnsafeWildcardCount rejects it (NEVER_MATCHES for the exclude half), so it excludes nothing -- + // fails toward STILL SHOWING the path as matched (the include), not toward silently hiding it. + const result = matchesAnyWithExclusions(adversarialPath, ["src/**", pathological]); + expect(Date.now() - start).toBeLessThan(1000); + expect(result).toBe(true); + }); + + it("REGRESSION: the exact live shape — a directory scope with one generated-file exclusion — matches the fix applied to loopover/metagraphed/awesome-claude's private config", () => { + const globs = ["apps/ui/src/components/**", "apps/ui/src/routes/**", "apps/ui/src/styles.css", "apps/ui/public/**", "!apps/ui/public/openapi.json"]; + expect(matchesAnyWithExclusions("apps/ui/public/openapi.json", globs)).toBe(false); + expect(matchesAnyWithExclusions("apps/ui/src/components/Button.tsx", globs)).toBe(true); + expect(matchesAnyWithExclusions("apps/ui/public/logo.svg", globs)).toBe(true); + }); +}); diff --git a/test/unit/queue-intelligence.test.ts b/test/unit/queue-intelligence.test.ts index 29c5a4c63f..6cf04cb7dc 100644 --- a/test/unit/queue-intelligence.test.ts +++ b/test/unit/queue-intelligence.test.ts @@ -5,6 +5,8 @@ import { isPublicScoreTermSafeForRepo, shouldWarnPublicScoreTermsAllowlistUnset, sanitizePublicComment, + ALWAYS_FORBIDDEN_PUBLIC_COMMENT_WORDS, + AMBIGUOUS_PUBLIC_COMMENT_WORDS, FORBIDDEN_PUBLIC_COMMENT_WORDS, } from "../../src/queue-intelligence"; import type { ChecksStatus, PullRequestInput, RepoContext, Recommendation } from "../../src/queue-intelligence"; @@ -400,11 +402,45 @@ describe("sanitizePublicComment — allowBareScoreTerm option (#public-score-ter expect(() => sanitizePublicComment("The score looks good here.", { allowBareScoreTerm: false })).toThrow(/score/i); }); - it("still throws on every OTHER forbidden phrase even when allowBareScoreTerm is true — only the bare-word check is relaxable", () => { - for (const forbiddenWord of FORBIDDEN_PUBLIC_COMMENT_WORDS) { - expect(() => sanitizePublicComment(`This comment contains ${forbiddenWord} information`, { allowBareScoreTerm: true })).toThrow(); + // #9432: the exemption now covers the AMBIGUOUS tier (ordinary English) as well as bare "score" -- but the + // ALWAYS_FORBIDDEN tier, which is where every QUALIFIED private-value form lives, stays enforced for every + // repo. This is the security boundary of the whole feature, so it is asserted exhaustively over the real + // list rather than a hand-picked sample: a term moved into the wrong tier fails here immediately. + it("SECURITY: still throws on every ALWAYS-FORBIDDEN phrase even when allowBareScoreTerm is true — an allowlist never exempts a named private concept", () => { + for (const forbiddenWord of ALWAYS_FORBIDDEN_PUBLIC_COMMENT_WORDS) { + expect(() => sanitizePublicComment(`This comment contains ${forbiddenWord} information`, { allowBareScoreTerm: true }), forbiddenWord).toThrow(); + } + }); + + it("REGRESSION (#9432): an allowlisted repo keeps a sentence using ordinary review English — 'reviewability', 'ranking', 'reward'", () => { + // These are words a safe review of this codebase's own gate code uses naturally. Before the tiering they + // were matched unconditionally, so a sentence like "this improves reviewability" was dropped from the + // published narrative for every repo, allowlisted or not. + for (const word of AMBIGUOUS_PUBLIC_COMMENT_WORDS) { + expect(() => sanitizePublicComment(`This change improves ${word} in the comparator`, { allowBareScoreTerm: true }), word).not.toThrow(); } }); + + it("INVARIANT: WITHOUT the exemption the ambiguous tier is still enforced — the default is unchanged for every non-allowlisted repo", () => { + for (const word of AMBIGUOUS_PUBLIC_COMMENT_WORDS) { + expect(() => sanitizePublicComment(`This change improves ${word} in the comparator`), word).toThrow(); + } + }); + + it("INVARIANT: the two tiers are disjoint and together are exactly FORBIDDEN_PUBLIC_COMMENT_WORDS — no term can be silently lost or double-listed", () => { + const always = new Set(ALWAYS_FORBIDDEN_PUBLIC_COMMENT_WORDS); + const ambiguous = new Set(AMBIGUOUS_PUBLIC_COMMENT_WORDS); + for (const word of ambiguous) expect(always.has(word), word).toBe(false); + expect([...FORBIDDEN_PUBLIC_COMMENT_WORDS].sort()).toEqual([...always, ...ambiguous].sort()); + }); + + it("SECURITY: a QUALIFIED private value stays blocked for an allowlisted repo even though its bare noun is exempt", () => { + // The decisive property behind the tiering: a leaked value is always qualified, and the qualified form + // lives in the always-forbidden tier. "reward" is exempt; "reward estimate 12 TAO" is not. + expect(() => sanitizePublicComment("the reward estimate is 12 TAO", { allowBareScoreTerm: true })).toThrow(); + expect(() => sanitizePublicComment("raw trust score 0.82 for this miner", { allowBareScoreTerm: true })).toThrow(); + expect(() => sanitizePublicComment("wallet 5Gv... and hotkey", { allowBareScoreTerm: true })).toThrow(); + }); }); describe("isPublicScoreTermSafeForRepo (#public-score-terms-scoping)", () => { diff --git a/test/unit/screenshot-table-gate-engine.test.ts b/test/unit/screenshot-table-gate-engine.test.ts index a9deed4df9..8c0ae061da 100644 --- a/test/unit/screenshot-table-gate-engine.test.ts +++ b/test/unit/screenshot-table-gate-engine.test.ts @@ -172,6 +172,17 @@ describe("isScreenshotTableGateInScope", () => { it("only whenPaths configured (whenLabels empty) -- scope decided purely by path", () => { expect(isScreenshotTableGateInScope(config({ whenPaths: ["apps/ui/**"] }), ["frontend"], ["src/api/routes.ts"])).toBe(false); }); + + // #9434: "apps/loopover-ui/public/**" auto-closed 5 contributor PRs one-shot for regenerating a generated + // openapi.json -- the directory is overwhelmingly non-visual, and the old matcher had no way to exclude the + // one generated file inside it without dropping the whole directory from scope (which is what the loopover/ + // metagraphed/awesome-claude private configs all did as the actual fix). This pins the alternative: an + // operator CAN keep the directory in scope and carve out just the generated file. + it("REGRESSION (#9434): a whenPaths exclusion carves a generated file out of an otherwise-visual directory", () => { + const cfg = config({ whenPaths: ["apps/ui/public/**", "!apps/ui/public/openapi.json"] }); + expect(isScreenshotTableGateInScope(cfg, [], ["apps/ui/public/openapi.json"])).toBe(false); + expect(isScreenshotTableGateInScope(cfg, [], ["apps/ui/public/favicon.ico"])).toBe(true); + }); }); describe("normalizeScreenshotTableGateConfig", () => { diff --git a/test/unit/selfhost-inert-config.test.ts b/test/unit/selfhost-inert-config.test.ts new file mode 100644 index 0000000000..63790f1d7a --- /dev/null +++ b/test/unit/selfhost-inert-config.test.ts @@ -0,0 +1,88 @@ +import { describe, expect, it } from "vitest"; +import { inertConfigEntries, inertConfigGaugeSamples } from "../../src/selfhost/inert-config"; + +// #9433: a config-dependent fix that ships INERT behaves exactly like the pre-fix build, with a green test +// suite — tests verify the mechanism works WHEN ENABLED, never that an operator set the variable. The +// confirmed instance sat live for weeks. This report is the answerability layer: "which config-gated +// behaviours are currently inert on this box?", on demand, with zero steady-state noise. +describe("inertConfigEntries (#9433)", () => { + /** Everything set to a real value — the fully-configured deployment. */ + const configured = { + LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS: "JSONbored/loopover", + LOOPOVER_PUBLIC_STATS_REPOS: "JSONbored/loopover", + LOOPOVER_REVIEW_SAFETY: "true", + }; + + it("reports nothing when every tracked var is set — a fully-configured box is silent", () => { + expect(inertConfigEntries(configured)).toEqual([]); + expect(inertConfigGaugeSamples(configured)).toEqual([]); + }); + + it("REGRESSION: reports the confirmed #9433 instance — an unset score-terms allowlist silently strips narrative sentences", () => { + const entries = inertConfigEntries({ ...configured, LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS: undefined }); + expect(entries.map((entry) => entry.key)).toEqual(["LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS"]); + // The impact string is what an operator actually reads, so it must describe the OBSERVABLE effect rather + // than the mechanism — "the summary looks fine, just shorter" is the whole reason this went unnoticed. + expect(entries[0]?.impact).toMatch(/silently|shorter/i); + }); + + it("reports each tracked var independently, and all of them on a bare env", () => { + expect(inertConfigEntries({}).map((entry) => entry.key)).toEqual([ + "LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS", + "LOOPOVER_PUBLIC_STATS_REPOS", + "LOOPOVER_REVIEW_SAFETY", + ]); + }); + + it("treats whitespace-only and empty-string as unset — the shapes a half-written .env actually produces", () => { + for (const blank of ["", " ", "\t\n"]) { + expect(inertConfigEntries({ ...configured, LOOPOVER_PUBLIC_STATS_REPOS: blank }).map((e) => e.key)).toEqual([ + "LOOPOVER_PUBLIC_STATS_REPOS", + ]); + } + }); + + // The judgment that keeps this report from becoming the next ignored alert: LOOPOVER_PUBLIC_STATS_REPOS is + // unset on the ORB self-host box and that is CORRECT — the Worker serves public stats. Marking it + // deployment-specific is what says "do not warn about this unconditionally". + it("classifies every entry as deployment-specific, so nothing here is auto-escalated to a warning", () => { + for (const entry of inertConfigEntries({})) { + expect(entry.severity, entry.key).toBe("deployment-specific"); + } + }); + + it("INVARIANT: gauge samples carry a BOUNDED label set — key plus severity only, value always 1", () => { + // Cardinality is bounded by construction: the key set is a fixed list in code, never derived from input, + // so an operator can alert on a specific key with no cardinality risk. + const samples = inertConfigGaugeSamples({}); + expect(samples.length).toBeGreaterThan(0); + for (const sample of samples) { + expect(Object.keys(sample.labels).sort()).toEqual(["key", "severity"]); + expect(sample.value).toBe(1); + } + }); + + it("INVARIANT: gauge samples and entries stay in lockstep — one series per inert entry, same keys", () => { + // Three surfaces (metrics, a future /ready field, this test) must read ONE answer; hand-maintaining a + // second list is the same drift class #9433 is about. + for (const env of [{}, configured, { ...configured, LOOPOVER_REVIEW_SAFETY: "" }]) { + const entries = inertConfigEntries(env); + const samples = inertConfigGaugeSamples(env); + expect(samples.map((sample) => sample.labels.key)).toEqual(entries.map((entry) => entry.key)); + } + }); + + it("INVARIANT: every entry carries a non-empty, actionable impact string", () => { + for (const entry of inertConfigEntries({})) { + expect(entry.impact.length, entry.key).toBeGreaterThan(40); + expect(entry.key, entry.key).toMatch(/^[A-Z0-9_]+$/); // a real env var name, not prose + } + }); + + it("is PURE — repeated calls on the same env return an identical answer, and never mutate it", () => { + const env = { ...configured, LOOPOVER_PUBLIC_STATS_REPOS: undefined }; + const before = JSON.stringify(env); + expect(inertConfigEntries(env)).toEqual(inertConfigEntries(env)); + expect(JSON.stringify(env)).toBe(before); + }); +}); From 6a2bed097449c4d63d0f46acbebcca400bf2e0e6 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 02:05:53 -0700 Subject: [PATCH 2/4] test: cover the exclusion matcher from both import identities so shard attribution cannot miss it --- .../unit/screenshot-table-gate-engine.test.ts | 25 ++++++++++++++++ test/unit/screenshot-table-gate.test.ts | 30 +++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/test/unit/screenshot-table-gate-engine.test.ts b/test/unit/screenshot-table-gate-engine.test.ts index 8c0ae061da..242d99cb46 100644 --- a/test/unit/screenshot-table-gate-engine.test.ts +++ b/test/unit/screenshot-table-gate-engine.test.ts @@ -17,6 +17,7 @@ import { requiredScreenshotMatrixPairs, type ScreenshotMatrixPair, } from "../../packages/loopover-engine/src/review/screenshot-table-gate"; +import { matchesAnyWithExclusions } from "../../packages/loopover-engine/src/signals/change-guardrail"; import type { ScreenshotTableGateConfig } from "../../packages/loopover-engine/src/types/manifest-deps-types"; function config(overrides: Partial = {}): ScreenshotTableGateConfig { @@ -795,3 +796,27 @@ describe("extractTableRowImageUrls (#4366)", () => { ]); }); }); + +// #9434: the exclusion matcher exercised through the ENGINE source path (change-guardrail.test.ts reaches the +// same function through the `src/signals/change-guardrail` re-export shim, a distinct coverage identity). +// Both paths are asserted so the lines stay attributed under either identity regardless of how CI shards the +// suites — the misattribution class vitest.config.ts's coverage-include comment documents. +describe("matchesAnyWithExclusions via the engine path (#9434)", () => { + it("excludes a generated file from a directory glob, and leaves the rest matching", () => { + const globs = ["apps/ui/public/**", "!apps/ui/public/openapi.json"]; + expect(matchesAnyWithExclusions("apps/ui/public/openapi.json", globs)).toBe(false); + expect(matchesAnyWithExclusions("apps/ui/public/hero.png", globs)).toBe(true); + }); + + it("requires an include: an all-exclude list never matches, and a non-matching path stays false", () => { + expect(matchesAnyWithExclusions("apps/ui/public/x.png", ["!apps/ui/public/**"])).toBe(false); + expect(matchesAnyWithExclusions("docs/readme.md", ["apps/ui/**", "!apps/ui/public/**"])).toBe(false); + }); + + it("SECURITY: an over-complex exclude glob excludes NOTHING — it can only widen gate scope, never shrink it", () => { + // Opposite fail direction from an include: an unsafe exclude resolving to 'matches everything' would + // silently shrink a safety gate's coverage. + const result = matchesAnyWithExclusions("apps/ui/public/openapi.json", ["apps/ui/public/**", "!apps/*-*-*-x.json"]); + expect(result).toBe(true); + }); +}); diff --git a/test/unit/screenshot-table-gate.test.ts b/test/unit/screenshot-table-gate.test.ts index 477f40cb7f..13f27be1a4 100644 --- a/test/unit/screenshot-table-gate.test.ts +++ b/test/unit/screenshot-table-gate.test.ts @@ -792,3 +792,33 @@ describe("extractTableRowImageUrls (#4366)", () => { ]); }); }); + +// #9434: the same exclusion behaviour asserted in screenshot-table-gate-engine.test.ts, but reached through +// the `src/review/screenshot-table-gate` re-export shim rather than the engine source path. +// +// Not redundant: the two import paths are DISTINCT coverage identities (the shim is its own real file), and +// CI sharding means a given shard may run only one of the two suites. Exercising the exclusion from both +// sides keeps these lines attributed under either identity in whichever shard picks them up — the +// misattribution class vitest.config.ts's own coverage-include comment documents from the 2026-07-24 +// codecov/patch incident. +describe("whenPaths exclusions via the src re-export (#9434)", () => { + const scoped = ["apps/ui/public/**", "!apps/ui/public/openapi.json"]; + + it("carves a generated file out of an otherwise-in-scope directory", () => { + expect(isScreenshotTableGateInScope(config({ whenPaths: scoped }), [], ["apps/ui/public/openapi.json"])).toBe(false); + expect(isScreenshotTableGateInScope(config({ whenPaths: scoped }), [], ["apps/ui/public/hero.png"])).toBe(true); + }); + + it("applies the SAME exclusion to the committed-image check, so the two cannot disagree on what is scoped", () => { + // A path excluded from gate scope must not still count as a stray committed image — that disagreement is + // exactly what sharing one matcher prevents. + expect(hasCommittedImageFile(["apps/ui/public/openapi.json"], scoped)).toBe(false); + expect(hasCommittedImageFile(["apps/ui/public/hero.png"], scoped)).toBe(true); + }); + + it("INVARIANT: a plain whenPaths list with no exclusions behaves exactly as before", () => { + expect(isScreenshotTableGateInScope(config({ whenPaths: ["apps/ui/**"] }), [], ["apps/ui/src/App.tsx"])).toBe(true); + expect(hasCommittedImageFile(["apps/ui/src/logo.png"], ["apps/ui/**"])).toBe(true); + expect(hasCommittedImageFile(["docs/logo.png"], ["apps/ui/**"])).toBe(false); + }); +}); From 1ceec68991cf0feee0f33730efb26f1a94a19ec7 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 02:19:24 -0700 Subject: [PATCH 3/4] test(gate): cover the presence-mode staleness checkpoint and two long-untested reject arms Brings packages/loopover-engine/src/review/screenshot-table-gate.ts and src/signals/change-guardrail.ts (and both re-export shims) to 100% statement, branch and function coverage. Three branches had no test on any identity: - evaluateScreenshotTableGate's PRESENCE-mode freshness checkpoint. Matrix mode's identical correlation was pinned by #8866's tests, but presence mode runs it through a separate return site and never saw a headSha in a test. Presence mode is the DEFAULT shape, so the miss meant one table pasted on push #1 could hold the gate green for every later push -- the exact regression the checkpoint exists to stop. Six tests: first-push checkpoint, stale-on-new-head (asserting no checkpoint is re-issued, which would launder the staleness away after one extra push), re-affirmation with fresh URLs, custom message on the stale path, no-headSha degradation, and same-head replay. - guardrailPathMatches' empty-path skip, a deliberate divergence from matchesAny's fail-safe: the boolean form matches an empty path under an over-complex glob, the structured form must not, because its output is rendered verbatim into public review text and audit metadata. - extractTableRows' non-table reject arm, which is what stops ordinary PR prose (and a shell pipe inside backticks) from parsing as table rows. Mirrored across both import identities so a sharded, flag-merged coverage upload cannot report a branch as uncovered on one copy. --- test/unit/change-guardrail.test.ts | 19 +++ .../unit/screenshot-table-gate-engine.test.ts | 145 +++++++++++++++++- test/unit/screenshot-table-gate.test.ts | 81 ++++++++++ 3 files changed, 244 insertions(+), 1 deletion(-) diff --git a/test/unit/change-guardrail.test.ts b/test/unit/change-guardrail.test.ts index efe8cc289a..c8a92c7cd1 100644 --- a/test/unit/change-guardrail.test.ts +++ b/test/unit/change-guardrail.test.ts @@ -123,6 +123,25 @@ describe("change-guardrail glob matching", () => { expect(isGuardrailHit(["unrelated/file.ts"], [pathological])).toBe(true); }); + it("INVARIANT: guardrailPathMatches SKIPS an empty changed path — deliberately diverging from matchesAny's fail-safe", () => { + // matchesAny("") returns true for an over-complex glob (asserted above): fail-safe means "match everything + // rather than silently disable a maintainer's guardrail". The STRUCTURED form cannot inherit that, because + // its output is rendered verbatim into public review text and audit metadata — emitting `{ path: "" }` puts + // a blank filename in front of a maintainer, which is worse than useless. So an empty path is dropped here + // even though the boolean form would report it, and that asymmetry is intentional, not an oversight. + const pathological = `${"**/".repeat(12)}*.ts`; + expect(matchesAny("", [pathological])).toBe(true); // the boolean form still fails safe + expect(guardrailPathMatches([""], [pathological])).toEqual([]); // ...the structured one does not invent a row + + // The skip is per-path, not a whole-call bail: real paths alongside an empty one are still reported. + expect(guardrailPathMatches(["", "src/scoring/x.ts", ""], ["src/scoring/**"])).toEqual([ + { path: "src/scoring/x.ts", glob: "src/scoring/**" }, + ]); + + // ...and it holds for ordinary (safe) globs too, so this is a property of the path, not of the glob. + expect(guardrailPathMatches([""], ["**"])).toEqual([]); + }); + it("SECURITY (ReDoS): a glob AT the safe cap (2 wildcards) still compiles and matches NORMALLY (not the fail-safe path)", () => { // Exactly 2 stars: at the cap, still safely compiled/evaluated — proves the cap is inclusive, not exclusive, // and that ordinary (non-pathological) multi-wildcard globs keep their real matching semantics. This is also diff --git a/test/unit/screenshot-table-gate-engine.test.ts b/test/unit/screenshot-table-gate-engine.test.ts index 242d99cb46..e4c159d50b 100644 --- a/test/unit/screenshot-table-gate-engine.test.ts +++ b/test/unit/screenshot-table-gate-engine.test.ts @@ -17,7 +17,7 @@ import { requiredScreenshotMatrixPairs, type ScreenshotMatrixPair, } from "../../packages/loopover-engine/src/review/screenshot-table-gate"; -import { matchesAnyWithExclusions } from "../../packages/loopover-engine/src/signals/change-guardrail"; +import { guardrailPathMatches as engineGuardrailPathMatches, matchesAnyWithExclusions } from "../../packages/loopover-engine/src/signals/change-guardrail"; import type { ScreenshotTableGateConfig } from "../../packages/loopover-engine/src/types/manifest-deps-types"; function config(overrides: Partial = {}): ScreenshotTableGateConfig { @@ -341,6 +341,23 @@ describe("extractTableRows", () => { expect(extractTableRows(body)).toHaveLength(38); }); + + it("INVARIANT: a line pair that is not header+separator is skipped, so prose and pipe-bearing text are never rows", () => { + // The scan walks EVERY adjacent line pair, so the reject arm is what stops ordinary PR-description prose + // from being parsed as a table. Three distinct rejection shapes, each of which alone would produce a bogus + // row if the guard were dropped: + expect(extractTableRows("just prose\nmore prose")).toEqual([]); // neither line is pipe-delimited + expect(extractTableRows("| a | b |\nnot a separator\n| 1 | 2 |")).toEqual([]); // header, but no separator under it + expect(extractTableRows("intro text\n| --- | --- |\n| 1 | 2 |")).toEqual([]); // separator with no header above it + + // A pipe inside prose (a shell command, a regex alternation) is the realistic false positive this prevents. + expect(extractTableRows("run `cat x | grep y` to check\nand then rerun it")).toEqual([]); + + // ...and the reject arm does not consume a REAL table that follows the rejected prose. + expect(extractTableRows("some intro prose\n\n| before | after |\n| --- | --- |\n| a.png | b.png |")).toEqual([ + ["a.png", "b.png"], + ]); + }); }); describe("missingScreenshotMatrixPairs (#4535)", () => { @@ -748,6 +765,122 @@ describe("evaluateScreenshotTableGate", () => { expect(result).toEqual({ violated: false, reason: null }); }); }); + + // Matrix mode's staleness checkpoint is covered above; PRESENCE mode runs the identical + // evidenceFreshnessForHead correlation through a separate return site, and that site had no headSha test at + // all. The gap matters more here than in matrix mode: presence mode is the DEFAULT shape (any image-bearing + // table passes), so a stale-evidence miss means one table pasted on push #1 keeps the gate green for every + // later push — exactly the #stale-screenshot-table-fix regression, silently un-pinned. + describe("presence mode staleness (#stale-screenshot-table-fix)", () => { + it("issues a head-keyed checkpoint on the first satisfying push, so a later push has something to compare against", () => { + const push1 = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: TABLE_BODY, + prLabels: [], + changedFiles: ["apps/ui/src/App.tsx"], + headSha: "presence-push-1", + }); + expect(push1.violated).toBe(false); + expect(push1.presenceModeSatisfiedState?.headSha).toBe("presence-push-1"); + expect(push1.presenceModeSatisfiedState?.evidenceFingerprint).toEqual(expect.any(String)); + }); + + it("REGRESSION: the SAME table on a new head is stale — it must not keep passing the gate across pushes", () => { + const push1 = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: TABLE_BODY, + prLabels: [], + changedFiles: ["apps/ui/src/App.tsx"], + headSha: "presence-push-1", + }); + const staleOnPush2 = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: TABLE_BODY, // byte-identical body: no re-affirmation + prLabels: [], + changedFiles: ["apps/ui/src/App.tsx"], + headSha: "presence-push-2", + presenceModeSatisfied: push1.presenceModeSatisfiedState, + }); + expect(staleOnPush2.violated).toBe(true); + expect(staleOnPush2.reason).toContain(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE); + // No checkpoint is issued on a stale result — re-issuing one would let push #3 compare against push #2 + // and read as "fresh", laundering the staleness away after a single extra push. + expect(staleOnPush2.presenceModeSatisfiedState).toBeUndefined(); + }); + + it("a re-affirmed table (fresh image URLs) on the new head is NOT stale, and re-checkpoints to that head", () => { + const push1 = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: TABLE_BODY, + prLabels: [], + changedFiles: ["apps/ui/src/App.tsx"], + headSha: "presence-push-1", + }); + const push2 = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: TABLE_BODY.replaceAll(".png", "-v2.png"), // a fresh upload gets a fresh URL + prLabels: [], + changedFiles: ["apps/ui/src/App.tsx"], + headSha: "presence-push-2", + presenceModeSatisfied: push1.presenceModeSatisfiedState, + }); + expect(push2.violated).toBe(false); + expect(push2.presenceModeSatisfiedState?.headSha).toBe("presence-push-2"); + }); + + it("a custom config.message wins over the default contract text on the stale path too", () => { + const push1 = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: TABLE_BODY, + prLabels: [], + changedFiles: [], + headSha: "presence-push-1", + }); + const stale = evaluateScreenshotTableGate({ + config: config({ enabled: true, message: "custom contract text" }), + prBody: TABLE_BODY, + prLabels: [], + changedFiles: [], + headSha: "presence-push-2", + presenceModeSatisfied: push1.presenceModeSatisfiedState, + }); + expect(stale).toEqual({ violated: true, reason: "custom contract text" }); + }); + + it("INVARIANT: no headSha ⇒ byte-identical to pre-fix behavior — no checkpoint issued, no staleness possible", () => { + // The caller may have no persistence wired up. Degrading to the old always-pass shape is deliberate: + // without a head SHA there is nothing to correlate, and inventing a violation would fail closed on a + // deployment that never opted in. + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: TABLE_BODY, + prLabels: [], + changedFiles: [], + presenceModeSatisfied: { headSha: "old", evidenceFingerprint: "anything" }, + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("INVARIANT: the SAME head re-evaluated is never stale — a webhook replay must not flip a passing gate", () => { + const push1 = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: TABLE_BODY, + prLabels: [], + changedFiles: [], + headSha: "presence-push-1", + }); + const replay = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: TABLE_BODY, + prLabels: [], + changedFiles: [], + headSha: "presence-push-1", + presenceModeSatisfied: push1.presenceModeSatisfiedState, + }); + expect(replay.violated).toBe(false); + expect(replay.presenceModeSatisfiedState?.headSha).toBe("presence-push-1"); + }); + }); }); describe("extractTableRowImageUrls (#4366)", () => { @@ -820,3 +953,13 @@ describe("matchesAnyWithExclusions via the engine path (#9434)", () => { expect(result).toBe(true); }); }); + +// Mirrored from the app suite so BOTH import identities own this branch (CI shards + merges by flag). +describe("guardrailPathMatches empty-path skip via the engine path", () => { + it("INVARIANT: an empty changed path is dropped rather than rendered as a blank filename", () => { + expect(engineGuardrailPathMatches([""], ["**"])).toEqual([]); + expect(engineGuardrailPathMatches(["", "src/scoring/x.ts"], ["src/scoring/**"])).toEqual([ + { path: "src/scoring/x.ts", glob: "src/scoring/**" }, + ]); + }); +}); diff --git a/test/unit/screenshot-table-gate.test.ts b/test/unit/screenshot-table-gate.test.ts index 13f27be1a4..a31f7f5d03 100644 --- a/test/unit/screenshot-table-gate.test.ts +++ b/test/unit/screenshot-table-gate.test.ts @@ -822,3 +822,84 @@ describe("whenPaths exclusions via the src re-export (#9434)", () => { expect(hasCommittedImageFile(["docs/logo.png"], ["apps/ui/**"])).toBe(false); }); }); + +// Mirrored from the engine suite so BOTH import identities own these branches: CI shards the coverage run and +// merges by flag, so a branch proven only through one identity can read as uncovered on the other. +describe("extractTableRows rejects non-table line pairs", () => { + it("INVARIANT: a line pair that is not header+separator is skipped, so prose and pipe-bearing text are never rows", () => { + expect(extractTableRows("just prose\nmore prose")).toEqual([]); // neither line is pipe-delimited + expect(extractTableRows("| a | b |\nnot a separator\n| 1 | 2 |")).toEqual([]); // header, but no separator under it + expect(extractTableRows("intro text\n| --- | --- |\n| 1 | 2 |")).toEqual([]); // separator with no header above it + expect(extractTableRows("run `cat x | grep y` to check\nand then rerun it")).toEqual([]); // a pipe inside prose + expect(extractTableRows("some intro prose\n\n| before | after |\n| --- | --- |\n| a.png | b.png |")).toEqual([ + ["a.png", "b.png"], + ]); + }); +}); + +// Presence mode is the DEFAULT gate shape (any image-bearing table passes), and its staleness correlation ran +// through a separate return site from matrix mode's — one that had no headSha test at all. Untested, a stale +// -evidence miss means one table pasted on push #1 holds the gate green for every later push. +describe("evaluateScreenshotTableGate presence-mode staleness (#stale-screenshot-table-fix)", () => { + const satisfying = { config: config({ enabled: true }), prBody: TABLE_BODY, prLabels: [] as string[], changedFiles: ["apps/ui/src/App.tsx"] }; + + it("issues a head-keyed checkpoint on the first satisfying push", () => { + const push1 = evaluateScreenshotTableGate({ ...satisfying, headSha: "presence-push-1" }); + expect(push1.violated).toBe(false); + expect(push1.presenceModeSatisfiedState?.headSha).toBe("presence-push-1"); + expect(push1.presenceModeSatisfiedState?.evidenceFingerprint).toEqual(expect.any(String)); + }); + + it("REGRESSION: the SAME table on a new head is stale — it must not keep passing the gate across pushes", () => { + const push1 = evaluateScreenshotTableGate({ ...satisfying, headSha: "presence-push-1" }); + const stale = evaluateScreenshotTableGate({ + ...satisfying, // byte-identical body: no re-affirmation + headSha: "presence-push-2", + presenceModeSatisfied: push1.presenceModeSatisfiedState, + }); + expect(stale.violated).toBe(true); + expect(stale.reason).toContain(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE); + // No checkpoint on a stale result — re-issuing one would let push #3 compare against push #2 and read as + // fresh, laundering the staleness away after a single extra push. + expect(stale.presenceModeSatisfiedState).toBeUndefined(); + }); + + it("a re-affirmed table (fresh image URLs) on the new head is NOT stale, and re-checkpoints to that head", () => { + const push1 = evaluateScreenshotTableGate({ ...satisfying, headSha: "presence-push-1" }); + const push2 = evaluateScreenshotTableGate({ + ...satisfying, + prBody: TABLE_BODY.replaceAll(".png", "-v2.png"), // a fresh upload gets a fresh URL + headSha: "presence-push-2", + presenceModeSatisfied: push1.presenceModeSatisfiedState, + }); + expect(push2.violated).toBe(false); + expect(push2.presenceModeSatisfiedState?.headSha).toBe("presence-push-2"); + }); + + it("a custom config.message wins over the default contract text on the stale path too", () => { + const push1 = evaluateScreenshotTableGate({ ...satisfying, headSha: "presence-push-1" }); + const stale = evaluateScreenshotTableGate({ + ...satisfying, + config: config({ enabled: true, message: "custom contract text" }), + headSha: "presence-push-2", + presenceModeSatisfied: push1.presenceModeSatisfiedState, + }); + expect(stale).toEqual({ violated: true, reason: "custom contract text" }); + }); + + it("INVARIANT: no headSha ⇒ byte-identical to pre-fix behavior — no checkpoint issued, no staleness possible", () => { + const result = evaluateScreenshotTableGate({ ...satisfying, presenceModeSatisfied: { headSha: "old", evidenceFingerprint: "anything" } }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("INVARIANT: the SAME head re-evaluated is never stale — a webhook replay must not flip a passing gate", () => { + const push1 = evaluateScreenshotTableGate({ ...satisfying, headSha: "presence-push-1" }); + const replay = evaluateScreenshotTableGate({ + ...satisfying, + headSha: "presence-push-1", + presenceModeSatisfied: push1.presenceModeSatisfiedState, + }); + expect(replay.violated).toBe(false); + expect(replay.presenceModeSatisfiedState?.headSha).toBe("presence-push-1"); + }); +}); From 90513c71c7075ee09d6750666d21acd9c50576b3 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 02:30:25 -0700 Subject: [PATCH 4/4] fix(ci): declare @loopover/contract#build on the three typecheck tasks that transitively need it validate-code failed on this PR with 'Cannot find module @loopover/contract/tools' plus five downstream implicit-any errors in src/mcp/server.ts -- none of which this PR touches. #9530 added @loopover/contract, and src/mcp/server.ts imports @loopover/contract/tools. Three turbo typecheck tasks pull that file into their program without any build edge to the package: - @loopover/ui#typecheck: apps/loopover-ui/tsconfig.json includes $TURBO_ROOT$/worker-configuration.d.ts, which imports './src/index' -- so the ENTIRE Worker is in the UI's typecheck program (src/index.ts -> src/api/routes.ts -> src/mcp/server.ts -> @loopover/contract/tools). Confirmed with tsc --explainFiles, not inferred. apps/loopover-ui has no package.json dependency on contract, so ^build never builds it. - //#typecheck: a root task, where a root package.json dependency creates no build edge. - @loopover/ui-miner#typecheck: reaches packages/loopover-miner/lib/**, which imports the package, and miner-ui has no dependency on it either. Each already carries an explicit @loopover/engine#build edge for precisely this reason -- turbo.json's own comment there describes the same scheduling race, observed intermittently in validate-code, that bit here. Cache-dependent, which is why it looked like flakiness: turbo caches these tasks, so the failure only appears on a cache MISS. Other open PRs are green on cache hits. Verified both directions from a clean contract build state (dist/ and .tsbuildinfo both removed, --force): - with the edges: 4 tasks successful, contract built first, typecheck passes - without them: the identical six errors CI reported --- turbo.json | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/turbo.json b/turbo.json index 365f8f9ad0..16f11c81ed 100644 --- a/turbo.json +++ b/turbo.json @@ -2,7 +2,12 @@ "$schema": "https://turborepo.com/schema.json", "tasks": { "//#typecheck": { - "dependsOn": ["@loopover/engine#build"], + // @loopover/contract#build joins @loopover/engine#build for the identical reason spelled out at + // @loopover/ui-miner#typecheck below: src/mcp/server.ts imports "@loopover/contract/tools", whose + // "types" resolve to packages/loopover-contract/dist/tools/index.d.ts. The root package.json dependency + // does NOT create a build edge for a root ("//#") task, so without this turbo schedules the contract + // build CONCURRENTLY with this typecheck and a cache MISS on the build races the dist emit. + "dependsOn": ["@loopover/engine#build", "@loopover/contract#build"], // Root tsconfig.json's own "include" list ("src", "test", plus a handful of named config files) is // NOT a complete accounting of what tsc actually type-checks: "include" only seeds the starting file // set, and tsc's real surface expands to everything transitively imported from there, regardless of @@ -79,8 +84,15 @@ // driver-factory.ts as an input) since engine has no default inputs override -- its whole src/** is // already hashed by that task, so depending on it transitively covers this file (and any other engine // file a future test might import) without needing to track individual paths here too. + // @loopover/contract#build is a third explicit edge (added after #9530 introduced the package): this + // app's tsconfig includes $TURBO_ROOT$/worker-configuration.d.ts, which imports "./src/index" -- so the + // ENTIRE Worker is in this typecheck's program (src/index.ts -> src/api/routes.ts -> src/mcp/server.ts -> + // "@loopover/contract/tools"), and apps/loopover-ui has no package.json dependency on contract to create + // the edge via ^build. Confirmed with `tsc --explainFiles`, not inferred. This one bit in CI as a + // cache-dependent failure: turbo caches this task, so it passed on PRs that hit the cache and failed with + // "Cannot find module '@loopover/contract/tools'" (plus five downstream implicit-any errors) on a miss. "@loopover/ui#typecheck": { - "dependsOn": ["^build", "@loopover/engine#build"], + "dependsOn": ["^build", "@loopover/engine#build", "@loopover/contract#build"], "inputs": [ "$TURBO_DEFAULT$", "$TURBO_ROOT$/worker-configuration.d.ts", @@ -122,8 +134,11 @@ // fail with phantom "Cannot find module '@loopover/engine'" errors (observed intermittently in CI's // validate-code job -- a scheduling race, so it only bit when the engine cache missed AND the // scheduler interleaved the two tasks the wrong way). + // @loopover/contract#build is here for the same reason as the engine edge described just above: those + // reached-into packages/loopover-miner/lib/** files import @loopover/contract, and miner-ui has no + // package.json dependency on it either. "@loopover/ui-miner#typecheck": { - "dependsOn": ["^build", "@loopover/engine#build"], + "dependsOn": ["^build", "@loopover/engine#build", "@loopover/contract#build"], "inputs": ["$TURBO_DEFAULT$", "$TURBO_ROOT$/packages/loopover-miner/lib/**"], "outputs": [] },