From 208948dcd3146817b3c4b073b05960ab1ded8cc5 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 00:57:42 -0700 Subject: [PATCH] fix(replay,maintainability): thread the recorded decision clock into three unrecorded reads, delete a dead wire, and add a reachability check (#9539, #9540) --- .github/workflows/ci.yml | 7 + package.json | 3 +- scripts/check-dead-source-files.ts | 216 ++++++++++++++++++ src/mcp/server.ts | 23 ++ src/miner/soft-claim.ts | 4 +- src/queue/account-age-throttle.ts | 14 +- src/queue/processors.ts | 19 +- src/review/content-lane/index.ts | 135 ----------- src/review/decision-replay.ts | 16 ++ src/review/issue-rag-wire.ts | 10 - src/review/unlinked-issue-guardrail.ts | 23 +- test/unit/account-age-throttle.test.ts | 43 ++++ .../check-dead-source-files-script.test.ts | 181 +++++++++++++++ test/unit/decision-clock-threading.test.ts | 55 +++++ test/unit/issue-rag-wire.test.ts | 57 ----- test/unit/mcp-write-tools.test.ts | 38 +++ test/unit/unlinked-issue-guardrail.test.ts | 90 ++++++++ 17 files changed, 718 insertions(+), 216 deletions(-) create mode 100644 scripts/check-dead-source-files.ts delete mode 100644 src/review/content-lane/index.ts delete mode 100644 src/review/issue-rag-wire.ts create mode 100644 test/unit/check-dead-source-files-script.test.ts create mode 100644 test/unit/decision-clock-threading.test.ts delete mode 100644 test/unit/issue-rag-wire.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0ee5b9dd7e..ebb9175299 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -363,6 +363,13 @@ jobs: - name: Manifest drift check if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' }} run: npm run manifest:drift-check + # #9492: a src/** module with no production importer is a silently dead feature — issue-rag-wire.ts + # sat that way with a green test suite, because its OWN test imported it and nothing else did. + # Coverage cannot catch the class; only reachability can. Gated on `backend` alone: the check reads + # only src/**, test/** and scripts/** import specifiers, none of which the other filters cover. + - name: Dead source-file check + if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' }} + run: npm run dead-source-files:check # Mechanical drift tripwire for the hand-duplicated src/{review,settings,signals} <-> loopover-engine # twin files, plus a version-skew check on the installed @loopover/engine (#4260). Same # local-only-until-now gap as the drift checks above. Also gated on `miner`: the reverse-direction diff --git a/package.json b/package.json index 4348f973f6..4a0b37786d 100644 --- a/package.json +++ b/package.json @@ -70,6 +70,7 @@ "docs:drift-check": "node --experimental-strip-types scripts/check-docs-drift.ts", "coverage-boltons:check": "node --experimental-strip-types scripts/check-coverage-bolt-on-filenames.ts", "import-specifiers:check": "node --experimental-strip-types scripts/check-import-specifiers.ts", + "dead-source-files:check": "node --experimental-strip-types scripts/check-dead-source-files.ts", "replay-runner-manifest": "tsx scripts/replay-runner-image-manifest.ts", "replay-runner-manifest:write": "tsx scripts/replay-runner-image-manifest.ts --write", "replay-runner-manifest:check": "tsx scripts/replay-runner-image-manifest.ts --check", @@ -114,7 +115,7 @@ "test:smoke:browser:install": "playwright install chromium", "test:smoke:browser": "node --experimental-strip-types scripts/smoke-ui-browser.ts", "pretest:ci": "npm run check-node-version", - "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run command-reference:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", + "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run dead-source-files:check && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run command-reference:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", "test:release": "npm run test:ci && npm run changelog:check", "test:release:mcp": "npm run test:ci", "test:watch": "vitest", diff --git a/scripts/check-dead-source-files.ts b/scripts/check-dead-source-files.ts new file mode 100644 index 0000000000..d165b98c47 --- /dev/null +++ b/scripts/check-dead-source-files.ts @@ -0,0 +1,216 @@ +#!/usr/bin/env node +// Guards against a silently dead feature file (#9492): a `src/**` module with zero non-test importers is +// either a forgotten wire-up (issue-rag-wire.ts, deleted in the same PR that added this check — both real +// consumers imported packages/loopover-engine/src/issue-rag-query directly, and only its own test imported +// the wire) or straightforwardly stale. Coverage does not catch this class: the dead module's OWN test +// exercises it directly, so it reports green while contributing nothing to the running system. Commit +// 39cc9583c shows this exact class has bitten before. +// +// There are ~26 `*-wire.ts` files and no registration protocol (feature-activation.ts's total `Record` over +// CONVERGED_FEATURE_KEYS is the one drift-proof exception, by construction) — this check is the closest +// mechanical substitute: "does anything besides this file's own test import it at all". +// +// DELIBERATELY NARROW. This is not a general dead-code/knip-style analysis (no re-export chasing, no +// detection of a file that's imported but whose EXPORTS are all unused) — it only answers "is this file +// imported by anything outside its own test", which is exactly the shape of failure #9492 found and cheap +// enough to run on every PR. A real entry point (the worker's own `src/index.ts`, ambient `.d.ts` files, +// migrations, scripts) is never import-reached by design, so those roots are excluded rather than allowlisted +// file-by-file — an allowlist of individual dead files would defeat the check's own purpose. +import { readFileSync, readdirSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +const SOURCE_ROOT = "src"; +const TEST_ROOT = "test"; +/** Roots whose files are legitimate PRODUCTION importers of `src/**`, scanned for imports but never + * themselves checked for deadness. `scripts/**` earns this: several CLIs and drift-checkers are the sole + * consumer of a `src/**` module (check-migrations.ts -> src/db/migration-column-extraction.ts, + * draft-issue.ts -> src/services/issue-drafting.ts), and those modules are emphatically alive. */ +const IMPORTER_ROOTS = ["scripts"] as const; +// .d.ts: ambient/generated declaration files are never import-reached by a relative specifier — they're +// picked up by tsconfig's `include`/global ambient resolution instead. +const SOURCE_PATTERN = /(? these files), so no relative import can ever name them. + "src/selfhost/cf-workers-shim.ts", + "src/selfhost/stubs/agents-mcp.ts", +]); +// Path PREFIXES (not just filenames) that are structurally entry-point-shaped and would otherwise need one +// ENTRY_POINT_FILES line per file: each is invoked by its OWN runner (a CLI, a route table scanned by path, +// a migration runner reading the directory), never relatively imported by sibling source. +const ENTRY_POINT_PREFIXES = ["src/mcp/tools/", "src/mcp/cli/"]; + +/** + * Files that are legitimately unconsumed RIGHT NOW because they were landed deliberately ahead of their + * consumers — a staged seam, not rot. Each entry must name the tracking issue that migrates callers onto it, + * which is the whole difference between this and a silent allowlist: an unexplained dead file is exactly what + * this check exists to catch, so an exception has to state why and say where it ends. + * + * Remove the entry once the seam gains its first real consumer — from that point the check protects it for free. + */ +const STAGED_AHEAD_OF_CONSUMERS: ReadonlyMap = new Map([ + [ + "src/openapi/define-route.ts", + "The route-registration seam (#9519), landed by #9533 ahead of the 241-route migration it exists for; #9531 moves routes onto it incrementally.", + ], +]); + +function isEntryPoint(file: string): boolean { + return ENTRY_POINT_FILES.has(file) || ENTRY_POINT_PREFIXES.some((prefix) => file.startsWith(prefix)); +} + +function defaultListFiles(root: string, pattern: RegExp): string[] { + try { + return readdirSync(root, { recursive: true }) + .map(String) + .filter((entry) => pattern.test(entry) && !EXCLUDED_SEGMENT.test(entry)) + .map((entry) => `${root}/${entry}`); + } catch { + return []; + } +} + +export type DeadSourceFileViolation = { file: string; reason: string }; + +/** + * Pure over its inputs: resolves every `src/**` module's relative-import specifiers found across BOTH + * `src/**` and `test/**`, and reports a source file with zero importers outside its own test file. + * + * Resolution is specifier-text matching, not a real module resolver — it strips a trailing `.js`/`.ts` + * extension (see check-import-specifiers.ts's own doc for why both appear across the tree's two resolution + * zones) and normalizes `./`/`../` against the IMPORTING file's directory, then checks whether the result + * names a real `.ts` file (trying both the bare path and its `/index.ts` form). This is sufficient for every + * import in this codebase — none of which use path-mapped aliases or bundler-magic specifiers — without + * needing a full TypeScript program. + * + * `list*`/`readFile` are injectable so tests can simulate a fresh dead file without writing into the real + * tree. + */ +export function findDeadSourceFiles( + options: { + sourceRoot?: string; + testRoot?: string; + importerRoots?: readonly string[]; + stagedAheadOfConsumers?: ReadonlyMap; + listSourceFiles?: (root: string, pattern: RegExp) => string[]; + listTestFiles?: (root: string, pattern: RegExp) => string[]; + readFile?: (file: string) => string; + fileExists?: (file: string) => boolean; + } = {}, +): DeadSourceFileViolation[] { + const { + sourceRoot = SOURCE_ROOT, + testRoot = TEST_ROOT, + importerRoots = IMPORTER_ROOTS, + stagedAheadOfConsumers = STAGED_AHEAD_OF_CONSUMERS, + listSourceFiles = defaultListFiles, + listTestFiles = defaultListFiles, + readFile = (file: string) => readFileSync(file, "utf8"), + fileExists = (file: string) => { + try { + readFileSync(file, "utf8"); + return true; + } catch { + return false; + } + }, + } = options; + + const sourceFiles = listSourceFiles(sourceRoot, SOURCE_PATTERN); + const testFiles = listTestFiles(testRoot, /\.ts$/); + const importerFiles = importerRoots.flatMap((root) => listSourceFiles(root, /\.ts$/)); + const sourceFileSet = new Set(sourceFiles); + + // #9492: the one real per-file test pairing convention this repo has (test/unit/.test.ts mirroring + // src/**/.ts) — used to exclude a file's OWN test from counting as its sole importer. Matched by + // basename, not by directory mirroring (the tree doesn't mirror src/'s subdirectories under test/unit/), + // which is deliberately loose: it only needs to identify "this could plausibly be file X's own test", not + // prove it, since the real signal is "is anything ELSE importing it". + const basenameOf = (file: string): string => (file.split("/").pop() ?? file).replace(/\.ts$/, ""); + const ownTestOf = (sourceFile: string): string => `${basenameOf(sourceFile)}.test.ts`; + + function resolveSpecifier(fromFile: string, specifier: string): string | null { + const fromDir = fromFile.split("/").slice(0, -1).join("/"); + const parts = `${fromDir}/${specifier}`.split("/"); + const resolved: string[] = []; + for (const part of parts) { + if (part === "." || part === "") continue; + if (part === "..") resolved.pop(); + else resolved.push(part); + } + const base = resolved.join("/"); + const bare = `${base}.ts`; + if (sourceFileSet.has(bare)) return bare; + const indexed = `${base}/index.ts`; + if (sourceFileSet.has(indexed)) return indexed; + // A specifier resolving OUTSIDE src/** (e.g. into packages/**) is legitimate and simply not tracked by + // this checker — it only answers reachability WITHIN src/**. + if (fileExists(bare)) return null; + if (fileExists(indexed)) return null; + return null; + } + + const importedBy = new Map>(); // resolved source file -> set of importer files + const specifierPattern = /(?:from|import)\s*\(?\s*"((?:\.{1,2}\/)[^"]+?)(?:\.js|\.ts)?"/g; + for (const importerFile of [...sourceFiles, ...testFiles, ...importerFiles]) { + for (const match of readFile(importerFile).matchAll(specifierPattern)) { + const specifier = match[1]; + if (!specifier) continue; + const resolved = resolveSpecifier(importerFile, specifier); + if (!resolved) continue; + const importers = importedBy.get(resolved) ?? new Set(); + importers.add(importerFile); + importedBy.set(resolved, importers); + } + } + + const violations: DeadSourceFileViolation[] = []; + for (const file of sourceFiles) { + if (isEntryPoint(file)) continue; + if (stagedAheadOfConsumers.has(file)) continue; + const importers = importedBy.get(file) ?? new Set(); + const ownTest = ownTestOf(file); + const nonTestImporters = [...importers].filter((importer) => { + const isTestFile = importer.startsWith(`${testRoot}/`); + const isOwnTest = isTestFile && (importer.split("/").pop() ?? importer) === ownTest; + return !isOwnTest; + }); + if (nonTestImporters.length === 0) { + violations.push({ + file, + reason: importers.size === 0 ? "no importer at all (including its own test)" : "only its own test imports it — no production consumer", + }); + } + } + return violations.sort((a, b) => a.file.localeCompare(b.file)); +} + +function main(): void { + const violations = findDeadSourceFiles(); + if (violations.length === 0) { + process.stdout.write("dead source files: OK\n"); + return; + } + process.stderr.write(`Found ${violations.length} src/** file(s) with no production importer (#9492):\n`); + for (const violation of violations) { + process.stderr.write(` ${violation.file} — ${violation.reason}\n`); + } + process.stderr.write( + "\nEither wire the file up, delete it, or — if it's a genuine new entry point invoked outside a relative\n" + + "import (a new CLI, a route table, a migration runner) — add it to ENTRY_POINT_FILES/ENTRY_POINT_PREFIXES\n" + + "in scripts/check-dead-source-files.ts with a one-line reason. If it is a seam landed deliberately AHEAD\n" + + "of the callers it exists for, add it to STAGED_AHEAD_OF_CONSUMERS naming the issue that migrates them.\n", + ); + process.exit(1); +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) main(); diff --git a/src/mcp/server.ts b/src/mcp/server.ts index a46341b2a0..ff9bf2e3af 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -174,6 +174,10 @@ import { buildTestGenSpec, type LocalWriteActionSpec, } from "./local-write-tools"; +// #2315/#9492: buildSoftClaimSpec shipped in #2813 but was never registered as an MCP tool -- the exact +// silently-dead-feature shape this audit's check-dead-source-files.ts now guards against. Wired up here, +// following the identical local-write-tools pattern every sibling in this block already uses. +import { buildSoftClaimSpec } from "../miner/soft-claim"; import { buildTestEvidenceReport, classifyTestCoverage, hasLocalTestEvidence, isCodeFile, isTestPath, TEST_FRAMEWORKS } from "../signals/test-evidence"; import { applyStepResult, buildPlanDag, nextReadySteps, planProgress, validatePlanDag, type PlanDag } from "../services/plan-dag"; import { buildFocusManifestValidation } from "../services/focus-manifest-validation"; @@ -522,6 +526,14 @@ const postEligibilityCommentShape = { number: z.number().int().positive(), body: z.string().min(1).max(WRITE_TOOL_BODY_MAX), }; +// #2315/#9492: mirrors buildSoftClaimSpec's own input type exactly. +const postSoftClaimShape = { + repoFullName: z.string().min(3).max(SCENARIO_MAX_REPO_FULL_NAME_CHARS), + number: z.number().int().positive(), + minerId: z.string().min(1).max(200), + claimedAt: z.string().datetime({ offset: true }), + expiresAt: z.string().datetime({ offset: true }).optional(), +}; const createBranchShape = { branch: z.string().min(1).max(WRITE_TOOL_BRANCH_MAX), base: z.string().min(1).max(WRITE_TOOL_BRANCH_MAX).optional() }; const deleteBranchShape = { branch: z.string().min(1).max(WRITE_TOOL_BRANCH_MAX), remote: z.boolean().optional() }; // #2188: the framework list mirrors detectTestConvention's TEST_FRAMEWORKS (#2187) so a caller cannot request a @@ -2108,6 +2120,7 @@ export const MCP_TOOL_CATEGORIES: Record = { loopover_file_issue: "agent", loopover_apply_labels: "agent", loopover_post_eligibility_comment: "agent", + loopover_post_soft_claim: "agent", loopover_create_branch: "agent", loopover_delete_branch: "agent", loopover_generate_tests: "agent", @@ -2925,6 +2938,16 @@ export class LoopoverMcp { { description: "Build a LOCAL-execution spec to post an eligibility/context comment on an issue or PR (run it with your own gh creds; loopover never performs the write).", inputSchema: postEligibilityCommentShape, outputSchema: localWriteActionOutputSchema }, async (input) => this.toolResult(this.localWriteSpec(buildPostEligibilityCommentSpec(input))), ); + register( + "loopover_post_soft_claim", + { + description: + "Build a LOCAL-execution spec to post a soft-claim comment on an issue, signaling a miner is working on it to reduce duplicate work (run it with your own gh creds; loopover never performs the write). Not an assignment -- purely advisory.", + inputSchema: postSoftClaimShape, + outputSchema: localWriteActionOutputSchema, + }, + async (input) => this.toolResult(this.localWriteSpec(buildSoftClaimSpec(input))), + ); register( "loopover_create_branch", { description: "Build a LOCAL-execution spec to create a branch (run it locally; loopover never performs the write).", inputSchema: createBranchShape, outputSchema: localWriteActionOutputSchema }, diff --git a/src/miner/soft-claim.ts b/src/miner/soft-claim.ts index 67320d846f..2b4423b162 100644 --- a/src/miner/soft-claim.ts +++ b/src/miner/soft-claim.ts @@ -13,7 +13,7 @@ import { /** Format the deterministic Markdown body of a soft-claim comment: which miner claimed the issue, when, and — when * provided — when the claim lapses. Pure: the same inputs always produce the same string. The body's shell-safety * is guaranteed downstream by {@link buildSoftClaimSpec} reusing `local-write-tools`' single-quote escaping. */ -export function buildSoftClaimCommentBody(input: { minerId: string; claimedAt: string; expiresAt?: string }): string { +export function buildSoftClaimCommentBody(input: { minerId: string; claimedAt: string; expiresAt?: string | undefined }): string { const lines = [ `🤖 **Soft claim** — a Gittensor miner (\`${input.minerId}\`) is working on this issue, claimed at ${input.claimedAt}.`, ]; @@ -35,7 +35,7 @@ export function buildSoftClaimSpec(input: { number: number; minerId: string; claimedAt: string; - expiresAt?: string; + expiresAt?: string | undefined; }): LocalWriteActionSpec { const body = buildSoftClaimCommentBody(input); return buildPostEligibilityCommentSpec({ repoFullName: input.repoFullName, number: input.number, body }); diff --git a/src/queue/account-age-throttle.ts b/src/queue/account-age-throttle.ts index eda1f97c3e..c64cbfd3b7 100644 --- a/src/queue/account-age-throttle.ts +++ b/src/queue/account-age-throttle.ts @@ -1,16 +1,26 @@ import { getGithubUserCreatedAt } from "../github/app"; -/** Fail-open account-age check shared by issue cap tightening and issue-open labeling (#2561). */ +/** + * Fail-open account-age check shared by issue cap tightening and issue-open labeling (#2561). + * + * #9492: `nowMs` is a real seam, not a test convenience — this predicate HALVES a contributor cap + * (`effectiveIssueCapForAccountAge`) and can therefore flip a cap close, which makes it a decision INPUT. + * A caller inside a pass that captured a `decisionClock` must pass that instant so the decision stays + * replayable; the three callers that are NOT part of the replayed maintenance decision (the PR-open cap + * check, the issue cap check, and the issue webhook labeler — each its own pass with no recorded clock) + * legitimately omit it and read the live clock, exactly as before. + */ export async function isBelowAccountAgeThreshold( env: Env, installationId: number, authorLogin: string, accountAgeThresholdDays: number | null | undefined, + nowMs?: number | undefined, ): Promise { if (typeof accountAgeThresholdDays !== "number") return false; const createdAt = await getGithubUserCreatedAt(env, installationId, authorLogin); if (!createdAt) return false; - const ageDays = (Date.now() - Date.parse(createdAt)) / (24 * 60 * 60 * 1000); + const ageDays = ((nowMs ?? Date.now()) - Date.parse(createdAt)) / (24 * 60 * 60 * 1000); return ageDays < accountAgeThresholdDays; } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 18b8b6540d..74ae0c8567 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2793,6 +2793,13 @@ function buildAgentMaintenancePlanInput(args: { // existing test/caller that hasn't been updated to pass it keeps today's (pre-#9160) behavior exactly. scopedLinkedIssueClaimedAt?: string | null | undefined; manualReviewLockContentionResolved: AgentActionPlanInput["manualReviewLockContentionResolved"]; + // #9492: the decision pass's ONE captured wall-clock instant (runAgentMaintenancePlanAndExecute's + // `decisionClock`). Required, not optional: this builder's `mergeBlockedSha` used to call `Date.now()` + // itself, which was a SECOND, unrecorded clock read in the very same pass that captures decisionClock -- + // so the recorded instant and the instant that actually decided the merge-blocked reason code could differ, + // and a replay could never reproduce the latter. Typed as required so a future call site cannot reintroduce + // the gap by simply omitting it (the #9482 lesson: ad-hoc optional context fields are how these drift). + decisionNowMs: number; }): AgentActionPlanInput { const { gate, @@ -2922,7 +2929,8 @@ function buildAgentMaintenancePlanInput(args: { // token, exhausted rate-limit window) carries an expiry; once it lapses the planner must see no block at // all and re-probe the merge, so a fleet-wide token blip stops stranding green, approved PRs forever. // Resolved here rather than in the planner so the planner stays a pure function of its inputs, clock-free. - mergeBlockedSha: activeMergeBlockedSha(pr, pr.headSha, Date.now()), + // #9492: the pass's recorded instant, not a fresh read — see decisionNowMs's own doc above. + mergeBlockedSha: activeMergeBlockedSha(pr, pr.headSha, args.decisionNowMs), mergeBlockedReason: pr.mergeBlockedReason, approvedHeadSha: pr.approvedHeadSha, authorLogin: pr.authorLogin, @@ -3419,6 +3427,9 @@ async function runAgentMaintenancePlanAndExecute( changedPaths, diff: buildAiReviewDiff(changedFiles), prAuthorLogin: pr.authorLogin, + // #9492: the pass's recorded instant — the guardrail's velocity exception decides CLOSE vs HOLD off + // a wall-clock gap, so an unrecorded read there made that decision unreplayable. + nowMs: decisionClock.nowMs, }) : undefined; const unlinkedIssueMatchHold = unlinkedIssueMatchDisposition?.kind === "hold" ? unlinkedIssueMatchDisposition : undefined; @@ -3517,7 +3528,10 @@ async function runAgentMaintenancePlanAndExecute( if (typeof accountAgeThresholdDays === "number" && pr.authorLogin && !authorIsOwner && !authorIsAdmin && !authorIsAutomationBot) { const createdAt = await getGithubUserCreatedAt(env, installationId, pr.authorLogin); if (createdAt) { - const ageDays = (Date.now() - Date.parse(createdAt)) / (24 * 60 * 60 * 1000); + // #9492: the pass's recorded instant. This read decides `isNewAccount`, which HALVES the contributor + // open-PR cap and can therefore flip a cap CLOSE — a decision input, not telemetry. It sat inside a + // function that already had `decisionClock` in scope and simply didn't use it. + const ageDays = (decisionClock.nowMs - Date.parse(createdAt)) / (24 * 60 * 60 * 1000); isNewAccount = ageDays < accountAgeThresholdDays; } if (isNewAccount && resolveAutonomy(settings.autonomy, "review_state_label") === "auto") { @@ -3629,6 +3643,7 @@ async function runAgentMaintenancePlanAndExecute( duplicateWinnerEnabled, scopedLinkedIssueClaimedAt, manualReviewLockContentionResolved: hadPriorLockContentionHold && !aiReviewLockContentionThisPass, + decisionNowMs: decisionClock.nowMs, }), ); // Accuracy circuit-breakers (#self-improve / GAP-4): two INDEPENDENT, fail-open precision breakers, chained. diff --git a/src/review/content-lane/index.ts b/src/review/content-lane/index.ts deleted file mode 100644 index a3b1f6e96f..0000000000 --- a/src/review/content-lane/index.ts +++ /dev/null @@ -1,135 +0,0 @@ -// Content-lane public surface (reviewbot→loopover convergence). -// -// The native, flag-gated content-review primitives for the two CONTENT repos — awesome-claude (a -// curated list) and metagraphed (a registry) — a different domain from loopover's code-gate. The -// lane only runs when LOOPOVER_REVIEW_CONTENT_LANE is truthy (see ./flag); flag-off the host never reaches -// these modules. -// -// PORTED so far (the deterministic core — pure or fetch-only, no engine): -// - flag : LOOPOVER_REVIEW_CONTENT_LANE gate -// - safe-url : SSRF-safe URL guard (shared) -// - scope (awesome) : content-PR scope classification (ignore / close / deletion / review) -// - duplicates (awesome) : duplicate-detection + protected-edit gate -// - source-evidence (a.) : source-URL reachability gate (injectable fetch) -// - security-scan (a.) : embedded-secret + pipe-to-shell scan -// - registry-logic (meta): the GENERIC surface-model engine (RegistryLaneSpec, scope classification, duplicate -// detection) plus metagraphed's OWN domain-specific validators (candidate/provider -// gates, netuid GROUNDING, dedup keys, freshness) — the latter are metagraphed's own -// reference implementation, still exported here for a future registry to use as a -// template, not because they're generic. -// -// NOT re-exported here (metagraphed's own domain plumbing, no reason for a different registry to import it): -// taostats + public-registry netuid GROUNDING lookups (fail-open; taostats key optional) — import directly from -// ./netuid-verification if you're specifically working on metagraphed's own validators. -// -// DEFERRED / engine-entangled (NOT ported here — see the port report): the dual-AI review -// orchestration (needs the inference adapter + the gate engine), content-RAG (Vectorize/D1/Queue), -// and the GitHub/D1 I/O orchestrators that wire these primitives into a live review. - -export { isContentLaneEnabled, type ContentLaneEnv } from "./flag"; -export { isSafeHttpUrl, isSafeEndpointUrl } from "./safe-url"; - -// awesome-claude (curated list) primitives -export { - classifyContentFiles, - importContentPathParts, - touchesContentEntry, - SUPPORTED_CONTENT_CATEGORIES, - type ContentClassification, - type ContentFile, - type ContentScope, -} from "./scope"; -export { AWESOME_CLAUDE_CONTENT_SPEC, type ContentRepoSpec } from "./content-repo-spec"; -export { - buildContentDuplicateReview, - directoryIndexToSignals, - extractContentDuplicateSignals, - findContentDuplicateMatch, - findStrictContentDuplicateMatch, - findRelatedContentMatches, - findDuplicateFrontmatterKeys, - parseSimpleFrontmatter, - protectedFrontmatterChanges, - type ContentDuplicateMatch, - type ContentDuplicateReview, - type ContentDuplicateSignals, - type DirectoryIndexEntry, -} from "./duplicates"; -export { - checkSubmittedSourceEvidence, - extractSubmittedSourceUrls, - shouldHardCloseSourceEvidence, - sourceEvidenceCloseDecision, - sourceEvidenceSummary, - sourceEvidenceToDecisionEvidence, - type SourceEvidenceDecision, - type SourceEvidenceItem, - type SourceEvidenceReport, - type SubmittedSourceUrl, -} from "./source-evidence"; -export { - scanForSecrets, - scanSubmissionContent, - scanLinkedBodiesForSecrets, - EXECUTABLE_CATEGORIES, - type SecurityFinding, - type SecretScanResult, -} from "./security-scan"; - -// metagraphed (registry) primitives -export { - assessProviderDocument, - assessSurfaceEntry, - assessSubnetDocument, - assessFreshness, - classifyRegistryPrScope, - isRegistrySubmissionScope, - METAGRAPHED_LANE_SPEC, - SUBNET_ENTRY_PATTERN, - FLAT_PROVIDER_PATTERN, - type RegistryLaneSpec, - type RegistryPrScope, - type RegistryScopeResult, - computeGrounding, - containsSecretLikeText, - deriveRegistryIdentityTokens, - functionalRequired, - isAllowedChain, - isBaseLayerKind, - isInternalAutomationBranch, - isNonEmptyStructuredBody, - netuidGroundingRegex, - normalizePublicUrl, - probeFunctionalSurface, - registrableDomain, - surfaceMatchesRegistryIdentity, - toCoreVerdict, - ARTIFACT_PATTERN, - DEFAULT_PUBLIC_API_BASE, - STALE_REPO_DAYS, - type Assessment, - type CandidateLike, - type FreshnessSignals, - type GroundingSignals, - type MetaVerdict, - type ProviderAssessment, - type ProviderLike, - type Verdict, -} from "./registry-logic"; -export { - fetchSurfaceProbe, - makeSurfaceEntryVerifier, - probeToEvidence, - verifySurfaceEntry, - FUNCTIONAL_INCONCLUSIVE_REASON, - FUNCTIONAL_NOT_SERVED_REASON, - GROUNDING_INCONCLUSIVE_REASON, - GROUNDING_UNCONFIRMED_REASON, - MAX_PROBE_BODY_CHARS, - SURFACE_GROUNDING_MIN_STRONG, - type SurfaceCheckOutcome, - type SurfaceCheckResult, - type SurfaceEntryVerification, - type SurfaceProbe, -} from "./surface-verification"; -export { runSurfaceReview, diffAppendedSurfaceEntries, type SurfaceReviewInput, type SurfaceReviewResult } from "./orchestrator"; diff --git a/src/review/decision-replay.ts b/src/review/decision-replay.ts index d8ac9f0652..65f91d0a0a 100644 --- a/src/review/decision-replay.ts +++ b/src/review/decision-replay.ts @@ -32,6 +32,22 @@ // them is left to a follow-up widening this same input shape — documented here so the replay contract states // a narrower, honestly-achievable claim rather than an unqualified broad one. // +// #9492 SCOPE UPDATE (narrowing the gap #9135's note left open): the recorded `clock` instant now reaches +// EVERY clock-dependent read inside the maintenance decision pass, not just `maybeForceFreshRebase`. Newly +// threaded: `activeMergeBlockedSha` (which feeds the plan AND the recorded reason code — previously a +// SECOND, unrecorded `Date.now()` in the very pass that captures this instant), the account-age +// `isNewAccount` derivation (which halves the contributor cap and can flip a cap close), and +// `resolveUnlinkedIssueMatchDisposition`'s velocity exception (whose wall-clock gap chooses CLOSE vs HOLD). +// +// STILL LIVE-CLOCK, deliberately, and named here rather than left to be rediscovered: +// • The three `isBelowAccountAgeThreshold` callers OUTSIDE this pass (the PR-open cap check, the issue cap +// check, the issue webhook labeler). Each is its own pass with no captured instant and no replay record, +// so there is nothing to be consistent WITH; the seam exists on that helper for when one gains a record. +// • SQL-side `datetime('now', ?)` in src/review/submitter-reputation.ts — a class no JS-side clock seam can +// cover, since the instant is chosen by the database at statement time. +// • The precision-breaker flags, the live CI aggregate, and the global pause/freeze switches called out in +// the #9135 note above — unchanged, still unrecorded. +// // A divergence is a bug BY DEFINITION (the pipeline is supposed to be deterministic): callers exit non-zero // and file it; there is no "close enough" outcome. import { evaluateGateCheck, type GateCheckPolicy } from "../rules/advisory"; diff --git a/src/review/issue-rag-wire.ts b/src/review/issue-rag-wire.ts deleted file mode 100644 index b479b69158..0000000000 --- a/src/review/issue-rag-wire.ts +++ /dev/null @@ -1,10 +0,0 @@ -/** - * Issue-centric RAG query composition (#2320), extracted to `@loopover/engine` (#4254) so the - * miner analyze phase can compose the identical retrieval query from an issue's title/body/labels without - * importing the review stack. Retrieval wiring for the miner MCP tool lives in `./issue-rag-retrieval.ts` - * and `src/mcp/issue-rag.ts` (#4293); the Vectorize/D1 backend itself stays in `./rag`. - * - * packages/loopover-engine/src/issue-rag-query.ts (imported via relative source path, not the published - * module, matching the #2278/#2282 extraction shims) is the source of truth. - */ -export { buildIssueRagQuery, type IssueRagQueryInput } from "../../packages/loopover-engine/src/issue-rag-query"; diff --git a/src/review/unlinked-issue-guardrail.ts b/src/review/unlinked-issue-guardrail.ts index 8979ca7cf8..398a188f30 100644 --- a/src/review/unlinked-issue-guardrail.ts +++ b/src/review/unlinked-issue-guardrail.ts @@ -88,8 +88,8 @@ function hasUnlinkedIssueVerifyAiBinding(env: Env): boolean { /** Has this actor already run the AI verifier at or beyond the rate ceiling in the last window, across every * repo/PR? Fail-safe: a read error resolves to "not rate-limited," so the pre-#4515 unconditional- * verification behavior takes over rather than a DB hiccup silently disabling this guardrail. */ -async function isOverUnlinkedIssueVerifyRateCeiling(env: Env, authorLogin: string): Promise { - const sinceIso = new Date(Date.now() - VERIFY_RATE_CEILING_WINDOW_MS).toISOString(); +async function isOverUnlinkedIssueVerifyRateCeiling(env: Env, authorLogin: string, nowMs: number): Promise { + const sinceIso = new Date(nowMs - VERIFY_RATE_CEILING_WINDOW_MS).toISOString(); const count = await countRecentAuditEventsForActor(env, authorLogin, UNLINKED_ISSUE_VERIFY_ATTEMPT_AUDIT_EVENT_TYPE, sinceIso).catch(() => 0); return count >= VERIFY_RATE_CEILING_MAX_ATTEMPTS; } @@ -178,6 +178,12 @@ export type ResolveUnlinkedIssueMatchDispositionInput = { * a login is renameable and a freed one is reclaimable, so matching a `confirmed_miner`-gated exception on * username alone would let a squatter who claims a former miner's old login inherit it. */ prAuthorGithubId?: number | null | undefined; + /** #9492: the decision pass's ONE recorded wall-clock instant (processors.ts's `decisionClock.nowMs`). + * This module reads the clock in three places, and the one at the velocity exception below decides CLOSE + * vs HOLD — a decision input, not telemetry. Optional so every existing caller/test keeps today's + * behaviour byte-identically (`Date.now()` when omitted), which also keeps this module directly testable + * without threading a clock through fixtures that do not care about it. */ + nowMs?: number | undefined; }; function unlinkedIssueMatchTargetKey(repoFullName: string, pullNumber: number): string { @@ -188,8 +194,8 @@ function unlinkedIssueMatchTargetKey(repoFullName: string, pullNumber: number): * recency window, and if so when? Fail-safe: a read error resolves to "no prior match" (never wrongly * escalates on a DB hiccup). Timestamp (not just a boolean) so the caller can apply the #4512 velocity * exception. */ -async function priorUnlinkedIssueMatchTimestamp(env: Env, authorLogin: string, currentTargetKey: string): Promise { - const sinceIso = new Date(Date.now() - UNLINKED_ISSUE_MATCH_REPEAT_WINDOW_MS).toISOString(); +async function priorUnlinkedIssueMatchTimestamp(env: Env, authorLogin: string, currentTargetKey: string, nowMs: number): Promise { + const sinceIso = new Date(nowMs - UNLINKED_ISSUE_MATCH_REPEAT_WINDOW_MS).toISOString(); return mostRecentAuditEventForOtherTarget(env, authorLogin, UNLINKED_ISSUE_MATCH_AUDIT_EVENT_TYPE, currentTargetKey, sinceIso).catch(() => null); } @@ -228,6 +234,9 @@ async function recordUnlinkedIssueVerifyAttempt(env: Env, repoFullName: string, export async function resolveUnlinkedIssueMatchDisposition(env: Env, input: ResolveUnlinkedIssueMatchDispositionInput): Promise { if (input.config.mode !== "hold") return undefined; if (input.linkedIssueCount > 0) return undefined; + // #9492: resolved ONCE here so all three clock-dependent reads below share one instant — two of them + // bound DB lookback windows and the third decides close-vs-hold, and they must agree. + const nowMs = input.nowMs ?? Date.now(); const openIssues = await listOpenIssues(env, input.repoFullName); const candidateIssues: CandidateOpenIssue[] = openIssues.map((issue) => ({ number: issue.number, @@ -245,7 +254,7 @@ export async function resolveUnlinkedIssueMatchDisposition(env: Env, input: Reso const authorLogin = input.prAuthorLogin?.trim() || null; // #4515: cost-control gates ahead of the AI loop below. An unidentifiable author can't be rate-limited // individually (nothing to key the ceiling on), so only the shared budget check applies to them. - if (authorLogin && (await isOverUnlinkedIssueVerifyRateCeiling(env, authorLogin))) return unlinkedIssueVerifyCapacityHold("rate"); + if (authorLogin && (await isOverUnlinkedIssueVerifyRateCeiling(env, authorLogin, nowMs))) return unlinkedIssueVerifyCapacityHold("rate"); if (await isUnlinkedIssueVerifyBudgetExceeded(env, candidates.length)) return unlinkedIssueVerifyCapacityHold("budget"); for (const candidate of candidates) { // #8356: mirror the spend-budget gate below — missing/no-op AI bindings fail closed to NO_MATCH @@ -271,10 +280,10 @@ export async function resolveUnlinkedIssueMatchDisposition(env: Env, input: Reso }; } const currentTargetKey = unlinkedIssueMatchTargetKey(input.repoFullName, input.pullNumber); - const priorMatchIso = await priorUnlinkedIssueMatchTimestamp(env, authorLogin, currentTargetKey); + const priorMatchIso = await priorUnlinkedIssueMatchTimestamp(env, authorLogin, currentTargetKey, nowMs); await recordUnlinkedIssueMatchOccurrence(env, input.repoFullName, input.pullNumber, authorLogin, candidate.issue.number); if (priorMatchIso) { - const gapMs = Date.now() - new Date(priorMatchIso).getTime(); + const gapMs = nowMs - new Date(priorMatchIso).getTime(); // #4512 velocity exception: gated on CONFIRMED miner identity, not on speed alone -- an unverified // account repeating this fast is the MORE suspicious case, not less, and still escalates to close. const velocityExceptionApplies = gapMs >= 0 && gapMs < VELOCITY_EXCEPTION_MAX_GAP_MS && (await isConfirmedOfficialMiner(env, authorLogin, input.prAuthorGithubId).catch(() => false)); diff --git a/test/unit/account-age-throttle.test.ts b/test/unit/account-age-throttle.test.ts index 8461249ae2..6712ecff84 100644 --- a/test/unit/account-age-throttle.test.ts +++ b/test/unit/account-age-throttle.test.ts @@ -66,3 +66,46 @@ describe("account-age throttle helpers (#2561 issue path)", () => { expect(await isBelowAccountAgeThreshold(env, 123, "newbie", 30)).toBe(true); }); }); + +// #9492: `isNewAccount` HALVES the contributor open-PR cap (effectiveIssueCapForAccountAge) and can therefore +// flip a cap CLOSE — it is a decision INPUT, not telemetry. The maintenance decision pass captures ONE +// `decisionClock` instant and records it into the replay input; before this, the account-age comparison read +// the clock again, independently, so the recorded instant and the instant that actually decided the cap could +// differ and a replay could never reproduce the latter. This helper had no `nowMs` seam at all. +describe("account-age recorded decision clock (#9492)", () => { + const stubUserCreatedAt = (createdAt: string) => + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.includes("/users/")) return Response.json({ created_at: createdAt }); + return Response.json({}); + }); + + it("REGRESSION: evaluates the age against the SUPPLIED instant, not the live clock", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generatePrivateKeyPem() }); + // An account created 10 days ago against a 30-day threshold: NEW by the live clock. + stubUserCreatedAt(new Date(Date.now() - 10 * 24 * 60 * 60 * 1000).toISOString()); + + expect(await isBelowAccountAgeThreshold(env, 123, "newbie", 30)).toBe(true); + // The same account judged at an instant 60 days on is ESTABLISHED. The two answers differ, which is what + // makes the recorded instant load-bearing rather than cosmetic. + expect(await isBelowAccountAgeThreshold(env, 123, "newbie", 30, Date.now() + 60 * 24 * 60 * 60 * 1000)).toBe(false); + }); + + it("INVARIANT: omitting nowMs preserves the live-clock behaviour exactly — the three callers outside the decision pass are unaffected", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generatePrivateKeyPem() }); + stubUserCreatedAt(new Date(Date.now() - 100 * 24 * 60 * 60 * 1000).toISOString()); + expect(await isBelowAccountAgeThreshold(env, 123, "veteran", 30)).toBe(false); + }); + + it("INVARIANT: the threshold-off and unavailable-created_at fail-open paths ignore nowMs entirely", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generatePrivateKeyPem() }); + let fetched = false; + vi.stubGlobal("fetch", async () => { + fetched = true; + return Response.json({}); + }); + expect(await isBelowAccountAgeThreshold(env, 123, "newbie", null, 0)).toBe(false); + expect(fetched).toBe(false); + }); +}); diff --git a/test/unit/check-dead-source-files-script.test.ts b/test/unit/check-dead-source-files-script.test.ts new file mode 100644 index 0000000000..da744239a0 --- /dev/null +++ b/test/unit/check-dead-source-files-script.test.ts @@ -0,0 +1,181 @@ +import { describe, expect, it } from "vitest"; +import { findDeadSourceFiles } from "../../scripts/check-dead-source-files"; + +/** Simulates a tree without touching the real one: every injectable seam the checker exposes, driven off one + * `path -> contents` map. Mirrors check-import-specifiers-script.test.ts's own `fakeFiles` helper. */ +function fakeTree(byFile: Record) { + const files = Object.keys(byFile); + return { + listSourceFiles: (root: string, pattern: RegExp) => files.filter((f) => f.startsWith(`${root}/`) && pattern.test(f)), + listTestFiles: (root: string, pattern: RegExp) => files.filter((f) => f.startsWith(`${root}/`) && pattern.test(f)), + readFile: (file: string) => byFile[file] ?? "", + fileExists: (file: string) => file in byFile, + }; +} + +// #9492: issue-rag-wire.ts had ZERO production importers — both real consumers imported the engine module it +// re-exported, directly, and only its own test imported the wire. Coverage reported it green the whole time, +// because a dead module's own test exercises it perfectly well. Reachability is the only signal that catches +// this, and commit 39cc9583c shows the class has bitten before. +describe("check-dead-source-files script", () => { + it("REGRESSION: flags a src module whose ONLY importer is its own test — the issue-rag-wire shape", () => { + const violations = findDeadSourceFiles({ + importerRoots: [], + ...fakeTree({ + "src/review/thing-wire.ts": 'export { thing } from "../../packages/engine/src/thing";', + "test/unit/thing-wire.test.ts": 'import { thing } from "../../src/review/thing-wire";', + }), + }); + expect(violations).toEqual([{ file: "src/review/thing-wire.ts", reason: "only its own test imports it — no production consumer" }]); + }); + + it("REGRESSION: flags a src module with no importer at all, and says so distinctly", () => { + const violations = findDeadSourceFiles({ + importerRoots: [], + ...fakeTree({ "src/review/orphan.ts": "export const x = 1;" }), + }); + expect(violations).toEqual([{ file: "src/review/orphan.ts", reason: "no importer at all (including its own test)" }]); + }); + + it("INVARIANT: one production importer is enough — a module imported by sibling source is never flagged", () => { + const violations = findDeadSourceFiles({ + importerRoots: [], + ...fakeTree({ + "src/review/thing.ts": "export const thing = 1;", + "src/review/consumer.ts": 'import { thing } from "./thing";', + "test/unit/thing.test.ts": 'import { thing } from "../../src/review/thing";', + }), + }); + // Asserted on the file under test, not on global emptiness: `consumer.ts` has no importer of its own in + // this minimal fixture and is correctly flagged — that is the checker working, not a false positive. + expect(violations.map((violation) => violation.file)).not.toContain("src/review/thing.ts"); + }); + + it("INVARIANT: a test that is NOT the module's own test counts as a real importer — only the same-named test is discounted", () => { + // A shared helper imported by many suites is alive even with no src/** importer; discounting every test + // would flag it. Only `.test.ts` is treated as the module's own. + const violations = findDeadSourceFiles({ + importerRoots: [], + ...fakeTree({ + "src/review/helper.ts": "export const help = 1;", + "test/unit/something-else.test.ts": 'import { help } from "../../src/review/helper";', + }), + }); + expect(violations).toEqual([]); + }); + + it("INVARIANT: a scripts/** CLI counts as a production importer — the real check-migrations/draft-issue shape", () => { + const tree = fakeTree({ + "src/db/extraction.ts": "export const extract = 1;", + "scripts/check-migrations.ts": 'import { extract } from "../src/db/extraction";', + }); + expect(findDeadSourceFiles({ importerRoots: ["scripts"], ...tree })).toEqual([]); + // ...and with scripts NOT scanned, the same tree reports it dead — pinning that the root is what saves it. + expect(findDeadSourceFiles({ importerRoots: [], ...tree })).toEqual([ + { file: "src/db/extraction.ts", reason: "no importer at all (including its own test)" }, + ]); + }); + + it("INVARIANT: declared entry points are never flagged, however unreachable", () => { + const violations = findDeadSourceFiles({ + importerRoots: [], + ...fakeTree({ "src/index.ts": "export default {};", "src/server.ts": "export const serve = 1;" }), + }); + expect(violations).toEqual([]); + }); + + it("INVARIANT: resolves a directory specifier through its index.ts, and an extensioned specifier through either zone's form", () => { + const violations = findDeadSourceFiles({ + importerRoots: [], + ...fakeTree({ + "src/lane/index.ts": "export const lane = 1;", + "src/lane/part.ts": "export const part = 1;", + // `./lane` (directory -> index.ts), and `.js`/`.ts`-suffixed forms, all resolve to real files. + "src/consumer.ts": 'import { lane } from "./lane";\nimport { part } from "./lane/part.js";', + }), + }); + const dead = violations.map((violation) => violation.file); + expect(dead).not.toContain("src/lane/index.ts"); // reached via the bare directory specifier + expect(dead).not.toContain("src/lane/part.ts"); // reached via the `.js`-suffixed specifier + }); + + it("INVARIANT: a specifier reaching OUTSIDE src/** is ignored rather than miscounted as an importer", () => { + // The checker only answers reachability WITHIN src/**; an engine import must neither crash it nor + // accidentally mark some unrelated src file as imported. + const violations = findDeadSourceFiles({ + importerRoots: [], + ...fakeTree({ + "src/review/orphan.ts": "export const x = 1;", + "src/review/consumer.ts": 'import { thing } from "../../packages/loopover-engine/src/thing";', + "test/unit/consumer.test.ts": 'import { c } from "../../src/review/consumer";', + }), + }); + // The engine specifier neither crashes resolution nor marks anything in src/** as imported: the orphan + // is still reported, and consumer.ts is reported too (its only importer IS its own test). + expect(violations).toEqual([ + { file: "src/review/consumer.ts", reason: "only its own test imports it — no production consumer" }, + { file: "src/review/orphan.ts", reason: "no importer at all (including its own test)" }, + ]); + }); + + it("INVARIANT: ambient .d.ts files are not scanned as dead source", () => { + const violations = findDeadSourceFiles({ + importerRoots: [], + ...fakeTree({ "src/worker-configuration.d.ts": "declare const x: 1;" }), + }); + expect(violations).toEqual([]); + }); + + it("INVARIANT: an export-from re-export counts as an import, so a barrel keeps its members alive", () => { + const violations = findDeadSourceFiles({ + importerRoots: [], + ...fakeTree({ + "src/lane/part.ts": "export const part = 1;", + "src/lane/barrel.ts": 'export { part } from "./part";', + "src/consumer.ts": 'import { part } from "./lane/barrel";', + "test/unit/consumer.test.ts": 'import { c } from "../../src/consumer";', + }), + }); + const dead = violations.map((violation) => violation.file); + expect(dead).not.toContain("src/lane/part.ts"); // kept alive by the barrel's `export ... from` + expect(dead).not.toContain("src/lane/barrel.ts"); + }); + + it("reports violations sorted by path, so the failure output is stable across runs", () => { + const violations = findDeadSourceFiles({ + importerRoots: [], + ...fakeTree({ "src/z-last.ts": "export const z = 1;", "src/a-first.ts": "export const a = 1;" }), + }); + expect(violations.map((violation) => violation.file)).toEqual(["src/a-first.ts", "src/z-last.ts"]); + }); + + // A seam landed ahead of the callers it exists for is legitimate staging, not rot — but only when the + // exception is STATED. The distinction is the whole value of this check: an unexplained dead file is + // precisely what it catches, so the escape hatch has to name the issue that ends it. + it("INVARIANT: a staged-ahead-of-consumers file is exempt, but only when explicitly listed", () => { + const tree = fakeTree({ "src/openapi/seam.ts": "export const seam = 1;" }); + expect(findDeadSourceFiles({ importerRoots: [], ...tree })).toEqual([ + { file: "src/openapi/seam.ts", reason: "no importer at all (including its own test)" }, + ]); + expect( + findDeadSourceFiles({ + importerRoots: [], + stagedAheadOfConsumers: new Map([["src/openapi/seam.ts", "landed by #X ahead of the migration in #Y"]]), + ...tree, + }), + ).toEqual([]); + }); + + it("INVARIANT: the staged exemption is exact-path, so a sibling in the same directory is still caught", () => { + const violations = findDeadSourceFiles({ + importerRoots: [], + stagedAheadOfConsumers: new Map([["src/openapi/seam.ts", "reason"]]), + ...fakeTree({ "src/openapi/seam.ts": "export const a = 1;", "src/openapi/rot.ts": "export const b = 1;" }), + }); + expect(violations.map((violation) => violation.file)).toEqual(["src/openapi/rot.ts"]); + }); + + it("the REAL repo tree is clean — this check runs in CI and must stay green", () => { + expect(findDeadSourceFiles()).toEqual([]); + }); +}); diff --git a/test/unit/decision-clock-threading.test.ts b/test/unit/decision-clock-threading.test.ts new file mode 100644 index 0000000000..f055d31851 --- /dev/null +++ b/test/unit/decision-clock-threading.test.ts @@ -0,0 +1,55 @@ +import { readFileSync } from "node:fs"; +import { describe, expect, it } from "vitest"; + +// #9492 parity guard for the decision-clock threading, in the same spirit as db-parsers.test.ts's +// STALE_RECHECK_DENIAL_DETAIL_PATTERN producer-drift guard: the two remaining sites live inside +// `buildAgentMaintenancePlanInput` / `runAgentMaintenancePlanAndExecute`, neither of which is exported, so +// there is no seam to drive them through. Reading the producer source is the honest alternative to either +// exporting internals purely for a test, or leaving the regression unguarded. +// +// What this protects: #9028/#9256 shipped "replayable time" — ONE `Date.now()` per decision pass, recorded +// into the replay input. The value of that is entirely destroyed by a SECOND, unrecorded clock read in the +// same pass, because the recorded instant then is not the instant that decided. Worse, `replayDecision` pins +// `action` and only re-derives `evaluateGateCheck`, so replaying a decision made on an unrecorded read +// reports `verdict: "match"` — a FALSE CERTIFICATION rather than a caught divergence. +describe("decision-clock threading in the maintenance pass (#9492)", () => { + const source = () => readFileSync("src/queue/processors.ts", "utf8"); + + it("REGRESSION: the merge-blocked read uses the pass's recorded instant, never a fresh Date.now()", () => { + const text = source(); + expect(text).toContain("activeMergeBlockedSha(pr, pr.headSha, args.decisionNowMs)"); + expect(text).not.toContain("activeMergeBlockedSha(pr, pr.headSha, Date.now())"); + }); + + it("INVARIANT: decisionNowMs is a REQUIRED plan-input field, so a new call site cannot silently omit it", () => { + // Optionality is how this class of context field drifts (the #9482 lesson) — an optional clock would let + // a future caller reintroduce the gap by simply not passing it, with no type error. + expect(source()).toContain("decisionNowMs: number;"); + expect(source()).not.toMatch(/decisionNowMs\?\s*:/); + }); + + it("REGRESSION: the account-age derivation uses the recorded instant — it decides isNewAccount, which halves the contributor cap", () => { + const text = source(); + expect(text).toContain("const ageDays = (decisionClock.nowMs - Date.parse(createdAt))"); + expect(text).not.toContain("const ageDays = (Date.now() - Date.parse(createdAt))"); + }); + + it("REGRESSION: the unlinked-issue guardrail is called with the recorded instant — its velocity gap chooses CLOSE vs HOLD", () => { + expect(source()).toContain("nowMs: decisionClock.nowMs,"); + }); + + it("INVARIANT: the pass still captures exactly ONE clock instant to thread", () => { + // The whole mechanism rests on there being a single captured instant; two captures would be the same bug + // wearing a different shape. + const captures = source().match(/const decisionClock: DecisionClockCapture = \{ nowMs: Date\.now\(\) \}/g) ?? []; + expect(captures).toHaveLength(1); + }); + + it("INVARIANT: decision-replay's honest-scope note names the reads deliberately left on the live clock", () => { + // The note must stay accurate: a scope claim that silently widens is worse than a narrow one, because a + // reader trusts it. Pins that each still-live class is actually named. + const replaySource = readFileSync("src/review/decision-replay.ts", "utf8"); + expect(replaySource).toContain("isBelowAccountAgeThreshold"); + expect(replaySource).toContain("submitter-reputation.ts"); + }); +}); diff --git a/test/unit/issue-rag-wire.test.ts b/test/unit/issue-rag-wire.test.ts deleted file mode 100644 index a9df5b9bcd..0000000000 --- a/test/unit/issue-rag-wire.test.ts +++ /dev/null @@ -1,57 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { buildIssueRagQuery } from "../../src/review/issue-rag-wire"; - -describe("buildIssueRagQuery (#2320)", () => { - it("uses a sufficiently descriptive title when the issue has an empty body", () => { - expect( - buildIssueRagQuery({ - title: "Add observability context for self-hosted review planning failures", - body: "", - }), - ).toEqual({ - queryText: "Add observability context for self-hosted review planning failures", - }); - }); - - it("prepends the issue title, includes the body, and appends labels as a hint line", () => { - const { queryText } = buildIssueRagQuery({ - title: "Improve SQLite backup readiness checks", - body: "Operators need restore guidance tied to the existing self-host backup flow.", - labels: ["gittensor:feature", "selfhost", " "], - }); - - expect(queryText).toContain("Improve SQLite backup readiness checks"); - expect(queryText).toContain("Operators need restore guidance"); - expect(queryText).toContain("Labels: gittensor:feature, selfhost"); - expect(queryText.indexOf("Improve SQLite")).toBeLessThan(queryText.indexOf("Operators need")); - expect(queryText.indexOf("Operators need")).toBeLessThan(queryText.indexOf("Labels:")); - }); - - it("bounds long issue bodies without dropping the label hint", () => { - const { queryText } = buildIssueRagQuery({ - title: "Investigate flaky queue dispatch telemetry", - body: `${"a".repeat(4100)}SHOULD_NOT_APPEAR`, - labels: ["queue"], - }); - - expect(queryText).toContain("Investigate flaky queue dispatch telemetry"); - expect(queryText).toContain("Labels: queue"); - expect(queryText).not.toContain("SHOULD_NOT_APPEAR"); - }); - - it("returns an empty query for a one-line issue below the retrieval floor", () => { - expect(buildIssueRagQuery({ title: "Tiny" })).toEqual({ queryText: "" }); - }); - - it("uses a descriptive body when the title is blank and omits blank labels", () => { - const { queryText } = buildIssueRagQuery({ - title: " ", - body: "Document how the miner should build issue context before a pull request exists.", - labels: [" ", ""], - }); - - expect(queryText).toBe( - "Document how the miner should build issue context before a pull request exists.", - ); - }); -}); diff --git a/test/unit/mcp-write-tools.test.ts b/test/unit/mcp-write-tools.test.ts index 4b81ea312e..f158fcbd87 100644 --- a/test/unit/mcp-write-tools.test.ts +++ b/test/unit/mcp-write-tools.test.ts @@ -52,6 +52,44 @@ describe("MCP miner write-tools (#780)", () => { } }); + // #2315/#9492: buildSoftClaimSpec shipped in #2813 and was never registered as a tool — a silently dead + // feature, invisible to coverage because its own unit test exercised the builder perfectly well. It is the + // exact shape scripts/check-dead-source-files.ts now guards against; this test pins the wire-up itself. + it("post_soft_claim returns a runnable comment spec, and its body carries the advisory (not-an-assignment) framing", async () => { + const client = await connect(); + const result = await client.callTool({ + name: "loopover_post_soft_claim", + arguments: { repoFullName: "o/r", number: 7, minerId: "miner-a", claimedAt: "2026-07-28T00:00:00.000Z", expiresAt: "2026-07-28T06:00:00.000Z" }, + }); + expect(result.isError).toBeFalsy(); + const spec = result.structuredContent as Spec; + expect(spec.command).toContain("gh issue comment 7 --repo 'o/r'"); + // Reuses buildPostEligibilityCommentSpec's single-quote escaping rather than re-implementing it, so the + // whole body arrives inside one quoted argument. + expect(spec.command).toContain("miner-a"); + expect(spec.command).toContain("expires at 2026-07-28T06:00:00.000Z"); + expect(spec.command).toContain("not an assignment"); + }); + + it("post_soft_claim omits the expiry sentence entirely when no expiresAt is supplied", async () => { + const client = await connect(); + const result = await client.callTool({ + name: "loopover_post_soft_claim", + arguments: { repoFullName: "o/r", number: 7, minerId: "miner-a", claimedAt: "2026-07-28T00:00:00.000Z" }, + }); + expect(result.isError).toBeFalsy(); + expect((result.structuredContent as Spec).command).not.toContain("expires at"); + }); + + it("post_soft_claim rejects a non-ISO claimedAt rather than embedding it in a comment", async () => { + const client = await connect(); + const result = await client.callTool({ + name: "loopover_post_soft_claim", + arguments: { repoFullName: "o/r", number: 7, minerId: "miner-a", claimedAt: "yesterday" }, + }); + expect(result.isError).toBeTruthy(); + }); + // #2188 (boundary-safe test-generation slice of #1972). it("generate_tests returns a local-execution spec naming the framework and target files; loopover performs no write", async () => { const client = await connect(); diff --git a/test/unit/unlinked-issue-guardrail.test.ts b/test/unit/unlinked-issue-guardrail.test.ts index 90347ca6dc..e0031ce84b 100644 --- a/test/unit/unlinked-issue-guardrail.test.ts +++ b/test/unit/unlinked-issue-guardrail.test.ts @@ -213,6 +213,96 @@ describe("resolveUnlinkedIssueMatchDisposition", () => { expect(result?.kind).toBe("hold"); }); + // #9492: this module reads the wall clock in three places, and the velocity-exception gap at the third + // decides CLOSE vs HOLD. That made the choice a decision INPUT that the decision pass never recorded -- + // and because replayDecision pins `action` and only re-derives evaluateGateCheck, replaying such a + // decision reported `verdict: "match"`: a FALSE CERTIFICATION, not a caught divergence. + describe("recorded decision clock (#9492)", () => { + // Uses a CONFIRMED miner deliberately: the velocity exception only applies to one, so this is the only + // configuration in which the wall-clock GAP is what decides close-vs-hold. For an unconfirmed author a + // repeat closes regardless of gap, which would make the assertion pass without exercising the clock. + function stubConfirmedMinerFetch(githubUsername: string) { + return vi.fn(async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url === "https://api.gittensor.io/miners") return Response.json([{ githubUsername, githubId: "123", totalPrs: 2, totalMergedPrs: 2, isEligible: true, credibility: 1 }]); + if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]); + if (url === "https://api.gittensor.io/miners/123") return Response.json({}); + if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] }); + return Response.json({}); + }); + } + + it("REGRESSION: the velocity exception is evaluated at the SUPPLIED instant, not the live clock", async () => { + const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + await seedIssue(env, 7, "webhook retry duplicate bug", "retries duplicate events under load, needs a dedup key"); + vi.stubGlobal("fetch", stubConfirmedMinerFetch("contributor-a")); + try { + const first = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(first?.kind).toBe("hold"); + + // The LIVE clock would score this repeat as seconds later — inside the 1h window, so the velocity + // exception would HOLD it. The recorded instant places it 2 hours on, past the window, so it must + // CLOSE. The two answers differ, which is exactly what makes the recorded instant load-bearing. + const second = await resolveUnlinkedIssueMatchDisposition(env, { + ...BASE_INPUT, + pullNumber: 102, + config: config(), + nowMs: Date.now() + 2 * 60 * 60 * 1000, + }); + expect(second?.kind).toBe("close"); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("INVARIANT: with the SAME confirmed miner and no supplied instant, the live clock still holds — the pair pins the clock as the deciding input", async () => { + const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + await seedIssue(env, 7, "webhook retry duplicate bug", "retries duplicate events under load, needs a dedup key"); + vi.stubGlobal("fetch", stubConfirmedMinerFetch("contributor-a")); + try { + expect((await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }))?.kind).toBe("hold"); + const second = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, pullNumber: 102, config: config() }); + expect(second?.kind).toBe("hold"); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("INVARIANT: omitting nowMs preserves the live-clock behaviour exactly — every existing caller is unaffected", async () => { + const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + await seedIssue(env, 7, "webhook retry duplicate bug", "retries duplicate events under load, needs a dedup key"); + + const first = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(first?.kind).toBe("hold"); + // No nowMs: an immediate repeat is inside the window, and an UNCONFIRMED author escalates as before. + const second = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, pullNumber: 102, config: config() }); + expect(second?.kind).toBe("close"); + }); + + it("INVARIANT: one supplied instant governs the repeat LOOKBACK too, not just the velocity gap", async () => { + const run = vi.fn(async () => ({ response: JSON.stringify(aiVerdict()) })); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + await seedIssue(env, 7, "webhook retry duplicate bug", "retries duplicate events under load, needs a dedup key"); + + const first = await resolveUnlinkedIssueMatchDisposition(env, { ...BASE_INPUT, config: config() }); + expect(first?.kind).toBe("hold"); + + // Far past the repeat-detection window: the prior occurrence falls out of the lookback entirely, so + // this reads as a FIRST offence and holds. Pins that the lookback shares the same instant -- were it + // still on the live clock, the prior row would remain in range and this would close. + const second = await resolveUnlinkedIssueMatchDisposition(env, { + ...BASE_INPUT, + pullNumber: 102, + config: config(), + nowMs: Date.now() + 400 * 24 * 60 * 60 * 1000, + }); + expect(second?.kind).toBe("hold"); + }); + }); + describe("velocity exception for a CONFIRMED official miner (#4512)", () => { function stubMinerFetch(githubUsername: string) { return vi.fn(async (input: RequestInfo | URL) => {