Merge branch 'main' into feat/guardrail-mode

Author: loopover-orb[bot]

packages/loopover-engine/src/index.ts

@@ -465,6 +465,7 @@ export {
   buildSlopAssessment,
   buildTrivialWhitespaceChurnFinding,
   hasClearNoIssueRationale,
+  isTestableCodePath,
   type SlopAssessment,
   type SlopAssessmentInput,
   type SlopBand,

packages/loopover-engine/src/signals/slop.ts

@@ -110,6 +110,16 @@ const PADDING_DOMINANCE_SHARE = 0.5;
 // (where it exempts codegen-only diffs from the missing-test-evidence signal).
 const PADDING_CATEGORIES = new Set(["minified", "generated", "vendored"]);
 
+/** The canonical "this path is code that ought to carry tests" predicate: a source file that is NOT
+ *  mechanically-produced padding (generated/vendored/minified output carries source extensions — e.g.
+ *  protoc's `.pb.go` stubs — but nobody hand-writes tests for it). Exported as the single source of truth so
+ *  every test-evidence consumer (buildMissingTestEvidenceFinding here, plus the improvement and
+ *  contributor-open-pr-monitor signals in src/) applies the SAME rule instead of a hand-copied `isCodeFile`
+ *  that disagrees at exactly the codegen boundary (#9696). */
+export function isTestableCodePath(path: string): boolean {
+  return isCodeFile(path) && !PADDING_CATEGORIES.has(classifyChangedFile(path));
+}
+
 export function buildSlopAssessment(input: SlopAssessmentInput): SlopAssessment {
   const findings: AdvisoryFinding[] = [];
   const trivialChurnFinding = buildTrivialWhitespaceChurnFinding(input);
@@ -282,7 +292,7 @@ export function buildMissingTestEvidenceFinding(input: SlopAssessmentInput): Adv
   // so passes the plain isCodeFile check, but nobody hand-writes tests for mechanically regenerated code.
   // Mirror buildNonSubstantivePaddingFinding's classifyChangedFile-based exemption so this signal can't
   // fire on a codegen-only diff.
-  const codePaths = changedPaths.filter((path) => isCodeFile(path) && !PADDING_CATEGORIES.has(classifyChangedFile(path)));
+  const codePaths = changedPaths.filter(isTestableCodePath);
   if (codePaths.length === 0) return null;
 
   // A changed test FILE only counts as real test evidence when it carries substantive content. An empty or

packages/loopover-engine/test/slop.test.ts

@@ -0,0 +1,28 @@
+import assert from "node:assert/strict";
+import { test } from "node:test";
+
+import { buildMissingTestEvidenceFinding, isTestableCodePath } from "../dist/index.js";
+
+// #9696: isTestableCodePath is the single source-of-truth "code that ought to carry tests" predicate — a
+// source file that is NOT mechanically-produced padding — and buildMissingTestEvidenceFinding filters on it,
+// so a codegen-only diff never trips the missing-test-evidence signal.
+
+test("isTestableCodePath is true for hand-authored source and false for padding or non-code", () => {
+  assert.equal(isTestableCodePath("src/app/service.ts"), true);
+  // Generated/vendored/minified output carries source extensions but is not hand-authored, so it is exempt.
+  assert.equal(isTestableCodePath("api/service.pb.go"), false); // generated
+  assert.equal(isTestableCodePath("vendor/dep/util.go"), false); // vendored
+  assert.equal(isTestableCodePath("dist/app.min.js"), false); // minified
+  assert.equal(isTestableCodePath("README.md"), false); // not a code file at all
+});
+
+test("buildMissingTestEvidenceFinding exempts a codegen-only diff but still fires on real code without tests", () => {
+  // A diff of only generated output has no testable code → the missing-test-evidence signal must not fire.
+  assert.equal(
+    buildMissingTestEvidenceFinding({ changedFiles: [{ path: "api/service.pb.go", additions: 500, deletions: 0 }], tests: [], testFiles: [] }),
+    null,
+  );
+  // Real hand-authored code with zero tests → the finding fires.
+  const finding = buildMissingTestEvidenceFinding({ changedFiles: [{ path: "src/app/service.ts", additions: 120, deletions: 0 }], tests: [], testFiles: [] });
+  assert.ok(finding && finding.code === "missing_test_evidence", "expected a missing-test-evidence finding for uncovered hand-authored code");
+});

packages/loopover-miner/lib/attempt-cli.ts

@@ -1097,8 +1097,16 @@ export async function runAttempt(args: string[], options: RunAttemptOptions = {}
     // the worktree to postmortem -- those are the cases that default to `true` (nothing to retain), matching
     // cleanupAttemptWorktree's own retention policy (a failed REAL attempt is what gets retained).
     if (worktreeResult?.ok) {
-      const cleanupWorktree = options.cleanupAttemptWorktree ?? cleanupAttemptWorktree;
-      await cleanupWorktree(worktreeResult.repoPath, worktreeResult.worktreePath, worktreeResult.attemptOk ?? true);
+      // #9677: cleanupWorktree spawns `git worktree remove` -- a locked worktree, a git failure, or a timeout must
+      // never abort the rest of this teardown (the claim release, allocator release, and every close() below).
+      // Catch-and-capture, mirroring the DB-fork discard block below; an unremoved worktree is a lesser harm than
+      // a stranded `active` claim that blocks a sibling miner for the ledger's full expiry window.
+      try {
+        const cleanupWorktree = options.cleanupAttemptWorktree ?? cleanupAttemptWorktree;
+        await cleanupWorktree(worktreeResult.repoPath, worktreeResult.worktreePath, worktreeResult.attemptOk ?? true);
+      } catch (error) {
+        captureMinerError(error, { kind: "attempt_worktree_cleanup_failed", repoFullName: parsed.repoFullName, attemptId });
+      }
     }
     // Every terminal outcome past the claim point (submitted/abandon/stale/blocked/governed, or an
     // unexpected throw) releases the soft-claim -- a claim that outlives its own attempt process would
@@ -1108,8 +1116,15 @@ export async function runAttempt(args: string[], options: RunAttemptOptions = {}
     // the initial claim submission actually ran (claimRecord is only set once claimedIssue is), so a run that
     // never reached the claim point (e.g. blocked_max_concurrent_claims) has nothing to release remotely.
     if (claimedIssue && claimRecord && isDiscoveryPlaneEnabled(env)) {
-      const submitClaim = options.submitSoftClaim ?? submitSoftClaim;
-      await submitClaim({ ...claimRecord, status: "released" } as Parameters<typeof SubmitSoftClaimFn>[0], { env });
+      // #9677: the hosted release is a network POST -- a down discovery plane or a non-2xx response must not
+      // strand the local claim release / allocator release / close()s that follow. Catch-and-capture like the
+      // DB-fork discard block below rather than letting the finally throw.
+      try {
+        const submitClaim = options.submitSoftClaim ?? submitSoftClaim;
+        await submitClaim({ ...claimRecord, status: "released" } as Parameters<typeof SubmitSoftClaimFn>[0], { env });
+      } catch (error) {
+        captureMinerError(error, { kind: "attempt_hosted_claim_release_failed", repoFullName: parsed.repoFullName, attemptId });
+      }
     }
     if (allocation && allocator) allocator.release(attemptId);
     // #7858: discard the disposable DB fork on every terminal outcome, mirroring the worktree release above.

packages/loopover-miner/lib/attempt-worktree.ts

@@ -81,8 +81,10 @@ export async function prepareAttemptWorktree(
   });
   // ensureRepoCloned's own EnsureRepoClonedResult declares `error` optional, but every one of its real ok:false
   // return sites (repo-clone.ts) sets a real, non-empty error string -- a non-null assertion here rather than a
-  // fake fallback string, since that fallback would be genuinely unreachable dead code.
-  if (!cloneResult.ok) return { ok: false, error: cloneResult.error! };
+  // fake fallback string, since that fallback would be genuinely unreachable dead code. repoPath is always set on
+  // its failures too, so forward it (matching the worktree-add failure below) -- a clone/checkout failure and a
+  // worktree-add failure now surface the same {ok:false, repoPath, error} shape to callers.
+  if (!cloneResult.ok) return { ok: false, repoPath: cloneResult.repoPath, error: cloneResult.error! };
 
   const exec = options.exec ?? createRealWorktreeExec(options.timeoutMs);
   const baseBranch = typeof options.baseBranch === "string" && options.baseBranch.trim() ? options.baseBranch.trim() : "main";

packages/loopover-miner/lib/discover-cli.ts

@@ -1,5 +1,6 @@
 /** `discover` CLI command (#4247): wires the existing fanout -> rank -> enqueue pipeline together so a miner
  * can actually run it. Every piece already exists and is independently tested; this module only composes them. */
+import { existsSync } from "node:fs";
 import { resolveForgeConfig } from "./forge-config.js";
 import type { ForgeConfig } from "./forge-config.js";
 import {
@@ -22,6 +23,7 @@ import { initPolicyDocCacheStore } from "./policy-doc-cache.js";
 import type { PolicyDocCacheStore } from "./policy-doc-cache.js";
 import { initPolicyVerdictCacheStore } from "./policy-verdict-cache.js";
 import type { PolicyVerdictCacheStore } from "./policy-verdict-cache.js";
+import { isValidRepoSegment } from "./repo-clone.js";
 import { enqueueRankedDiscovery } from "./portfolio-discovery.js";
 import { AMS_MIN_RANK_SHIPPED, readMinRankAutotuneEnabled, readMinRankOverride } from "./ams-calibration.js";
 import { initEventLedger, resolveEventLedgerDbPath } from "./event-ledger.js";
@@ -185,11 +187,19 @@ async function supplementWithDiscoveryIndex(
   if (!isDiscoveryPlaneEnabled(env)) return fanOut;
   const queryIndex = options.queryDiscoveryIndex ?? queryDiscoveryIndex;
   const response = await queryIndex(queryScope, { env });
-  recordDiscoveryTelemetry("discover_query", response.candidates.length > 0 ? "supplemented" : "empty", { env });
-  if (response.candidates.length === 0) return fanOut;
+  // #9680: the hosted index preserves a repo's AI-contribution ban (normalizeDiscoveryIndexCandidate keeps
+  // aiPolicyAllowed:false), but nothing downstream re-checks it -- the `as RawCandidateIssue` cast below would
+  // launder a `false` through a type declared as the literal `true`, so an AI-banned repo's issue would be ranked
+  // and enqueued. Enforce the ban here exactly as the local fetchTargetIssues does (`if (!verdict.allowed) return
+  // []`): drop it, not down-rank or warn. A candidate that omits the field is kept (`!== false`, matching
+  // normalizeDiscoveryIndexCandidate's own default).
+  const aiAllowed = response.candidates.filter((candidate) => candidate.aiPolicyAllowed !== false);
+  const droppedAiBanned = response.candidates.length - aiAllowed.length;
+  recordDiscoveryTelemetry("discover_query", response.candidates.length > 0 ? "supplemented" : "empty", { env, droppedAiBanned });
+  if (aiAllowed.length === 0) return fanOut;
 
   const seen = new Set(fanOut.issues.map((issue) => dedupeKey(issue.repoFullName, issue.issueNumber)));
-  const supplemented = response.candidates
+  const supplemented = aiAllowed
     .filter((candidate) => !seen.has(dedupeKey(candidate.repoFullName, candidate.issueNumber)))
     // DiscoveryIndexCandidate is a near-superset of RawCandidateIssue; copy the real assignees through when the
     // hosted contract carried them (#7442), falling back to [] only when the served response genuinely omitted the
@@ -203,6 +213,7 @@ function parseRepoTarget(value: string): FanoutTarget | null {
   const trimmed = value.trim();
   const [owner, repo, extra] = trimmed.split("/");
   if (!owner || !repo || extra !== undefined) return null;
+  if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) return null;
   return { owner, repo };
 }
 
@@ -516,8 +527,17 @@ export async function runDiscover(args: string[], options: RunDiscoverOptions =
     let overrideLedger = null;
     try {
       const ledgerEnv = options.env ?? process.env;
-      overrideLedger = initEventLedger(resolveEventLedgerDbPath(ledgerEnv));
-      minRankScore = readMinRankOverride(overrideLedger, { enabled: readMinRankAutotuneEnabled(ledgerEnv) }) ?? AMS_MIN_RANK_SHIPPED;
+      const ledgerDbPath = resolveEventLedgerDbPath(ledgerEnv);
+      // #9679: --dry-run must make ZERO filesystem writes, but initEventLedger creates + migrates + prunes the
+      // ledger file. On the dry-run path only read the override when the ledger file ALREADY exists (opening a
+      // not-yet-existing SQLite file is itself a write, and retention pruning can delete rows) -- a missing file
+      // falls back to the shipped default, exactly the value an empty/new ledger would yield, so the preview is
+      // unchanged when it exists. Same "skip a file that doesn't exist yet" discipline as migrate-cli.ts /
+      // store-maintenance.ts. The real (non-dry-run) run is unchanged: it opens unconditionally.
+      if (!parsed.dryRun || existsSync(ledgerDbPath)) {
+        overrideLedger = initEventLedger(ledgerDbPath);
+        minRankScore = readMinRankOverride(overrideLedger, { enabled: readMinRankAutotuneEnabled(ledgerEnv) }) ?? AMS_MIN_RANK_SHIPPED;
+      }
     } catch {
       minRankScore = AMS_MIN_RANK_SHIPPED;
     } finally {

packages/loopover-miner/lib/discovery-index-client.ts

@@ -171,9 +171,16 @@ export async function submitSoftClaim(
 export function recordDiscoveryTelemetry(
   event: string,
   outcome: string,
-  options: { env?: Record<string, string | undefined> } = {},
+  options: { env?: Record<string, string | undefined>; droppedAiBanned?: number } = {},
 ): void {
   const env = options.env ?? process.env;
   if (!isDiscoveryPlaneEnabled(env) || !isDiscoveryTelemetryEnabled(env)) return;
-  getLogger().info("discovery_plane_telemetry", { event, outcome });
+  // #9680: low-cardinality count of index candidates dropped for an AI-contribution ban this query, when the
+  // caller supplies it. Spread conditionally so callers that don't pass it keep the exact { event, outcome }
+  // shape (and payload) they emitted before.
+  getLogger().info("discovery_plane_telemetry", {
+    event,
+    outcome,
+    ...(options.droppedAiBanned !== undefined ? { droppedAiBanned: options.droppedAiBanned } : {}),
+  });
 }

packages/loopover-miner/lib/event-ledger.ts

@@ -2,6 +2,7 @@ import type { DatabaseSync, SQLOutputValue } from "node:sqlite";
 import { isDeepStrictEqual } from "node:util";
 import { normalizeLocalStoreDbPath, openLocalStoreDb, resolveLocalStoreDbPath } from "./local-store.js";
 import { applySchemaMigrations } from "./schema-version.js";
+import { isValidRepoSegment } from "./repo-clone.js";
 import {
   EVENT_LEDGER_PURGE_SPEC,
   EVENT_LEDGER_RETENTION_SPEC,
@@ -83,6 +84,7 @@ function normalizeOptionalRepoFullName(repoFullName: unknown): string | null {
   if (typeof repoFullName !== "string") throw new Error("invalid_repo_full_name");
   const [owner, repo, extra] = repoFullName.trim().split("/");
   if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name");
+  if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name");
   return `${owner}/${repo}`;
 }
 

packages/loopover-miner/lib/loop-cli.ts

@@ -49,6 +49,7 @@ import { buildLoopClosureSummary } from "./loop-closure.js";
 import { attemptLoopReentry } from "./loop-reentry.js";
 import { parsePrNumberFromExecResult } from "./pr-number-parse.js";
 import { resolveGitHubToken } from "./github-token-resolution.js";
+import { isValidRepoSegment } from "./repo-clone.js";
 import { DEFAULT_AMS_POLICY_SPEC } from "@loopover/engine";
 import type { GovernorCapUsage } from "@loopover/engine";
 
@@ -118,6 +119,7 @@ function parseRepoTarget(value: string): string | null {
   const trimmed = value.trim();
   const [owner, repo, extra] = trimmed.split("/");
   if (!owner || !repo || extra !== undefined) return null;
+  if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) return null;
   return `${owner}/${repo}`;
 }
 

packages/loopover-miner/lib/manage-poll.ts

@@ -11,6 +11,7 @@ import type { PortfolioQueueStore } from "./portfolio-queue.js";
 import { DEFAULT_FORGE_CONFIG } from "./forge-config.js";
 import { argsWantJson, describeCliError, reportCliFailure } from "./cli-error.js";
 import { resolveGitHubToken } from "./github-token-resolution.js";
+import { isValidRepoSegment } from "./repo-clone.js";
 
 const MANAGE_POLL_USAGE =
   "Usage: loopover-miner manage poll <owner/repo> <pr#> [--branch <name>] [--dry-run] [--json]";
@@ -54,6 +55,9 @@ function parseRepoArg(value: string): { repoFullName: string } | { error: string
   if (!owner || !repo || extra !== undefined) {
     return { error: "Repository must be in owner/repo form." };
   }
+  if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) {
+    return { error: "Repository must be in owner/repo form." };
+  }
   return { repoFullName: `${owner}/${repo}` };
 }