From 53e578b8d1d1a37ad1a8cec0b2fbb2a61c2f2dcc Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 01:15:44 -0700 Subject: [PATCH 1/3] feat(miner): run the target repo's own test/lint/build commands before opening a PR (#8807) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing independently verified a coding agent's work before submission: the only verification module (engine lint-guard) is hardcoded to loopover's own monorepo commands and never passed in production, and coding-task-spec's validation guidance only TELLS the agent which commands to run. A known-bad change passed every AMS-side gate on the agent's self-attestation alone. - target-repo-verification.ts: runs stack-detection's already-inferred commands (test → lint → build, highest signal first) from the attempt's worktree, per-command timeout (10 min), stop at first failure, bounded output tail. An undetected stack or empty command set SKIPS (recorded, never a failure) — the gate is only as smart as detection, and empty detection must not block repos with unconventional tooling. - attempt-runner: the gate runs after handoff + kill-switch recheck and BEFORE the freshness read (a failing build never spends GitHub budget); a failure returns the new verification_failed outcome — the attempt never submits, the worktree is retained for postmortem, and the existing not-submitted notification plumbing carries the reason. Deliberately not re-entering the iterate loop in this change (the loop's own self-review iterations already ran; never-submit-known-bad is the trust win) — loop feedback is the tracked follow-up on the issue. - attempt-cli binds the worktree-scoped thunk (same stack detection the agent's guidance rendered), with MINER_SKIP_TARGET_REPO_VERIFICATION as the escape hatch for suites exceeding the per-command bound. --- packages/loopover-miner/lib/attempt-cli.ts | 19 ++++ packages/loopover-miner/lib/attempt-runner.ts | 19 ++++ .../lib/target-repo-verification.ts | 100 +++++++++++++++++ test/unit/miner-attempt-cli.test.ts | 74 ++++++++++++ test/unit/miner-attempt-runner.test.ts | 30 +++++ .../miner-target-repo-verification.test.ts | 105 ++++++++++++++++++ 6 files changed, 347 insertions(+) create mode 100644 packages/loopover-miner/lib/target-repo-verification.ts create mode 100644 test/unit/miner-target-repo-verification.test.ts diff --git a/packages/loopover-miner/lib/attempt-cli.ts b/packages/loopover-miner/lib/attempt-cli.ts index 6569a6be64..f8356cceb6 100644 --- a/packages/loopover-miner/lib/attempt-cli.ts +++ b/packages/loopover-miner/lib/attempt-cli.ts @@ -46,6 +46,8 @@ import { isValidRepoSegment } from "./repo-clone.js"; import { REJECTION_REASON_AI_USAGE_POLICY_BAN, REJECTION_REASON_OWN_SUBMISSION_REJECTED, resolveOwnOpenPrForIssue, resolveRejectionSignaled } from "./rejection-signal.js"; import { initDenyHookSynthesisStore } from "./deny-hook-synthesis.js"; import type { DenyRule } from "@loopover/engine"; +import { runTargetRepoVerification } from "./target-repo-verification.js"; +import { detectRepoStack } from "./stack-detection.js"; import type { resolveRejectionSignaled as ResolveRejectionSignaledFn } from "./rejection-signal.js"; import { cleanupAttemptWorktree, prepareAttemptWorktree } from "./attempt-worktree.js"; import type { @@ -145,6 +147,8 @@ export type RunAttemptOptions = { resolveRejectionSignaled?: typeof ResolveRejectionSignaledFn; // #8808: injection seam for the own-open-PR idempotency guard, mirroring resolveRejectionSignaled above. resolveOwnOpenPrForIssue?: typeof resolveOwnOpenPrForIssue; + // #8807: injection seam for the target-repo verification gate, mirroring the resolver seams above. + runTargetRepoVerification?: typeof runTargetRepoVerification; fetchImpl?: SelfReviewContextFetch; prepareAttemptWorktree?: typeof PrepareAttemptWorktreeFn; cleanupAttemptWorktree?: typeof CleanupAttemptWorktreeFn; @@ -739,6 +743,9 @@ export async function runAttempt(args: string[], options: RunAttemptOptions = {} }; }; + // #8807: captured as a const here (where the !ok early-return has already narrowed the union) because + // the verification thunk below closes over it — TS drops narrowing on a mutable binding inside closures. + const attemptWorktreePath = worktreeResult.worktreePath; const loopInput = buildAttemptLoopInput({ codingTaskSpec, reviewContext, @@ -872,6 +879,18 @@ export async function runAttempt(args: string[], options: RunAttemptOptions = {} ...deps, shouldAbort, resolveKillSwitchScope: () => resolveLiveKillSwitch().scope, + // #8807: pre-bound target-repo verification against THIS attempt's worktree, using the same stack + // detection the agent's own validation guidance rendered. Opt-out escape hatch for repos whose + // suites exceed the per-command bound; the gate itself skips (never fails) on an undetected stack. + ...(/^(1|true|yes|on)$/i.test((env.MINER_SKIP_TARGET_REPO_VERIFICATION ?? "").trim()) + ? {} + : { + verifyTargetRepo: () => + (options.runTargetRepoVerification ?? runTargetRepoVerification)({ + worktreeDir: attemptWorktreePath, + stack: detectRepoStack(attemptWorktreePath), + }), + }), }, ); } catch (error) { diff --git a/packages/loopover-miner/lib/attempt-runner.ts b/packages/loopover-miner/lib/attempt-runner.ts index 3484911788..50e9e3f484 100644 --- a/packages/loopover-miner/lib/attempt-runner.ts +++ b/packages/loopover-miner/lib/attempt-runner.ts @@ -60,6 +60,12 @@ export type AttemptInput = { }; export type AttemptDeps = { + /** #8807: pre-bound target-repo verification (worktree + detected stack captured by the caller). Runs the + * TARGET repo's own test/lint/build commands after handoff and BEFORE any submission read/write — a + * failed verification blocks the PR instead of trusting the coding agent's self-attestation. Optional: + * absent (older callers, tests) preserves the pre-#8807 flow byte-identically. Loosely typed at this + * public boundary like runSlopAssessment above; the real shape is TargetRepoVerificationResult. */ + verifyTargetRepo?: () => Promise<{ status: string } & Record>; driver: CodingAgentDriver; runSlopAssessment: (input: unknown) => unknown; appendAttemptLogEvent: (event: unknown) => void; @@ -83,6 +89,7 @@ export type AttemptDeps = { export type AttemptResult = | { outcome: "abandon"; loopResult: IterateLoopResult } + | { outcome: "verification_failed"; verification: unknown; loopResult: IterateLoopResult } | { outcome: "stale"; reason: FreshnessAbortReason; loopResult: IterateLoopResult } | { outcome: "blocked"; decision: HarnessSubmissionDecision; loopResult: IterateLoopResult } | { outcome: "governed"; decision: GovernorDecision; loopResult: IterateLoopResult } @@ -213,6 +220,18 @@ export async function runMinerAttempt(input: AttemptInput, deps: AttemptDeps): P } } + // #8807: the independent quality gate — the target repo's own commands against the worktree. Placed + // BEFORE the freshness read so a failing build never spends GitHub API budget. A "skipped" or "passed" + // result proceeds; only a real command failure blocks. Deliberately NOT re-entering the iterate loop in + // this change: the loop's internal self-review iterations already ran, and never-submit-known-bad is the + // trust win — feeding the failure back as loop input is the tracked follow-up on the issue. + if (typeof deps.verifyTargetRepo === "function") { + const verification = await deps.verifyTargetRepo(); + if (verification.status === "failed") { + return { outcome: "verification_failed", verification, loopResult }; + } + } + const freshness = await checkSubmissionFreshness( { repoFullName: input.loopInput.repoFullName, issueNumber: input.issueNumber, minerLogin: input.minerLogin }, { claimLedger: deps.claimLedger, fetchLiveIssueSnapshot: deps.fetchLiveIssueSnapshot, eventLedger: deps.eventLedger }, diff --git a/packages/loopover-miner/lib/target-repo-verification.ts b/packages/loopover-miner/lib/target-repo-verification.ts new file mode 100644 index 0000000000..10b3df985f --- /dev/null +++ b/packages/loopover-miner/lib/target-repo-verification.ts @@ -0,0 +1,100 @@ +// Target-repo verification gate (#8807): run the TARGET repository's own detected test/lint/build commands +// against the attempt's worktree BEFORE a PR opens — the independent check the audit found missing: the +// only verification module that existed (engine lint-guard.ts) is hardcoded to loopover's own monorepo +// commands and never passed in production, and coding-task-spec's validation guidance only TELLS the agent +// which commands to run, trusting its self-attestation. A coding agent that skips or fakes its own test run +// previously produced a PR that passed every AMS-side gate and still broke the target repo's build. +// +// Commands come from stack-detection.js's already-inferred RepoStackResult (the same source the agent's own +// guidance renders), run in test → lint → build order (highest signal first), stop at the first failure, +// with a per-command timeout and a bounded output tail (the postmortem detail, never an unbounded dump). +// An UNDETECTED stack or a stack with no inferred commands SKIPS (recorded, never a failure): this gate can +// only ever be as smart as detection, and refusing to submit because detection came up empty would block +// legitimate work on repos with unconventional tooling. +import { spawn as nodeSpawn } from "node:child_process"; +import type { RepoStackResult } from "./stack-detection.js"; + +export type TargetRepoVerificationSpawn = ( + command: string, + options: { cwd: string; timeoutMs: number }, +) => Promise<{ code: number | null; output: string }>; + +export type TargetRepoVerificationCheck = { + kind: "test" | "lint" | "build"; + command: string; + ok: boolean; + exitCode: number | null; + outputTail: string; +}; + +export type TargetRepoVerificationResult = + | { status: "passed"; checks: TargetRepoVerificationCheck[] } + | { status: "failed"; checks: TargetRepoVerificationCheck[]; firstFailure: TargetRepoVerificationCheck } + | { status: "skipped"; reason: "stack_undetected" | "no_commands_detected" | "disabled" }; + +/** Per-command wall-clock bound. A target repo's test suite legitimately runs minutes; 10 is the ceiling + * before the gate itself becomes the attempt's bottleneck — a suite slower than this is skipped territory + * for a future per-repo override, not something to silently wait out. */ +export const DEFAULT_VERIFICATION_TIMEOUT_MS = 10 * 60 * 1000; +/** Postmortem detail bound — enough tail to show the failing assertion, never an unbounded log dump. */ +export const VERIFICATION_OUTPUT_TAIL_CHARS = 4000; + +/** Default spawn: shell-executed (detected commands are shell strings like "npm test" / "ruff check ."), + * merged stdout+stderr, killed at the timeout (a killed process reports code null → treated as failure). */ +export const defaultVerificationSpawn: TargetRepoVerificationSpawn = (command, options) => + new Promise((resolve) => { + const child = nodeSpawn(command, { cwd: options.cwd, shell: true, stdio: ["ignore", "pipe", "pipe"] }); + let output = ""; + const capture = (chunk: Buffer) => { + output = (output + chunk.toString()).slice(-VERIFICATION_OUTPUT_TAIL_CHARS * 4); + }; + child.stdout?.on("data", capture); + child.stderr?.on("data", capture); + const timer = setTimeout(() => { + child.kill("SIGKILL"); + }, options.timeoutMs); + child.on("error", (error) => { + clearTimeout(timer); + resolve({ code: null, output: `${output}\n${String(error)}` }); + }); + child.on("close", (code) => { + clearTimeout(timer); + resolve({ code, output }); + }); + }); + +export async function runTargetRepoVerification(options: { + worktreeDir: string; + stack: RepoStackResult; + spawn?: TargetRepoVerificationSpawn; + timeoutMsPerCommand?: number; +}): Promise { + const stack = options.stack; + if (stack.detected !== true) return { status: "skipped", reason: "stack_undetected" }; + const commands: Array<{ kind: TargetRepoVerificationCheck["kind"]; command: string | null }> = [ + { kind: "test", command: stack.testCommand }, + { kind: "lint", command: stack.lintCommand }, + { kind: "build", command: stack.buildCommand }, + ]; + const runnable = commands.filter((entry): entry is { kind: TargetRepoVerificationCheck["kind"]; command: string } => entry.command !== null); + if (runnable.length === 0) return { status: "skipped", reason: "no_commands_detected" }; + + const spawn = options.spawn ?? defaultVerificationSpawn; + const timeoutMs = options.timeoutMsPerCommand ?? DEFAULT_VERIFICATION_TIMEOUT_MS; + const checks: TargetRepoVerificationCheck[] = []; + for (const { kind, command } of runnable) { + const { code, output } = await spawn(command, { cwd: options.worktreeDir, timeoutMs }); + const check: TargetRepoVerificationCheck = { + kind, + command, + ok: code === 0, + exitCode: code, + outputTail: output.slice(-VERIFICATION_OUTPUT_TAIL_CHARS), + }; + checks.push(check); + // Stop at the first failure: the remaining commands' results would only pile noise onto an attempt that + // is already not submitting, and a broken build often cascades into misleading downstream failures. + if (!check.ok) return { status: "failed", checks, firstFailure: check }; + } + return { status: "passed", checks }; +} diff --git a/test/unit/miner-attempt-cli.test.ts b/test/unit/miner-attempt-cli.test.ts index 98cf9a25c4..7c497e74b8 100644 --- a/test/unit/miner-attempt-cli.test.ts +++ b/test/unit/miner-attempt-cli.test.ts @@ -2670,3 +2670,77 @@ describe("resolveAttemptHouseRulesConfig (#8806)", () => { expect(close).toHaveBeenCalled(); }); }); + +describe("target-repo verification wiring (#8807)", () => { + it("binds verifyTargetRepo into the runner deps (worktree-scoped thunk over the injected verifier)", async () => { + const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); + vi.spyOn(console, "log").mockImplementation(() => undefined); + const runMinerAttemptSpy = vi.fn(async (_input: unknown, deps: { verifyTargetRepo?: () => Promise }) => { + // The thunk exists and resolves through the injected verifier when invoked. + expect(typeof deps.verifyTargetRepo).toBe("function"); + const verification = await deps.verifyTargetRepo!(); + expect(verification).toEqual({ status: "skipped", reason: "stack_undetected" }); + return { outcome: "abandon", loopResult: { outcome: "abandon", iterations: [], finalMeterTotals: { tokens: 0 } } }; + }); + const runVerifier = vi.fn(async (opts: { worktreeDir: string }) => { + expect(opts.worktreeDir).toBeTruthy(); // bound to THIS attempt's worktree + return { status: "skipped" as const, reason: "stack_undetected" as const }; + }); + + await runAttempt(["acme/widgets", "7", "--miner-login", "alice", "--json"], { + env: { MINER_CODING_AGENT_PROVIDER: "noop" }, + openWorktreeAllocator: () => allocator, + openClaimLedger: () => claimLedger, + initEventLedger: () => eventLedger, + initAttemptLog: () => attemptLog, + initGovernorLedger: () => governorLedger, + ...readyPipelineOptions({ runMinerAttempt: runMinerAttemptSpy, runTargetRepoVerification: runVerifier }), + }); + + expect(runMinerAttemptSpy).toHaveBeenCalled(); + expect(runVerifier).toHaveBeenCalled(); + }); + + it("without an injected verifier the thunk runs the REAL verification (default arm) — an unmarked temp worktree skips", async () => { + const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); + vi.spyOn(console, "log").mockImplementation(() => undefined); + const runMinerAttemptSpy = vi.fn(async (_input: unknown, deps: { verifyTargetRepo?: () => Promise<{ status: string }> }) => { + const verification = await deps.verifyTargetRepo!(); + expect(verification.status).toBe("skipped"); // no stack markers in the fixture worktree + return { outcome: "abandon", loopResult: { outcome: "abandon", iterations: [], finalMeterTotals: { tokens: 0 } } }; + }); + + await runAttempt(["acme/widgets", "7", "--miner-login", "alice", "--json"], { + env: { MINER_CODING_AGENT_PROVIDER: "noop" }, + openWorktreeAllocator: () => allocator, + openClaimLedger: () => claimLedger, + initEventLedger: () => eventLedger, + initAttemptLog: () => attemptLog, + initGovernorLedger: () => governorLedger, + ...readyPipelineOptions({ runMinerAttempt: runMinerAttemptSpy }), + }); + + expect(runMinerAttemptSpy).toHaveBeenCalled(); + }); + + it("MINER_SKIP_TARGET_REPO_VERIFICATION omits the thunk entirely — the documented escape hatch", async () => { + const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); + vi.spyOn(console, "log").mockImplementation(() => undefined); + const runMinerAttemptSpy = vi.fn(async (_input: unknown, deps: { verifyTargetRepo?: unknown }) => { + expect(deps.verifyTargetRepo).toBeUndefined(); + return { outcome: "abandon", loopResult: { outcome: "abandon", iterations: [], finalMeterTotals: { tokens: 0 } } }; + }); + + await runAttempt(["acme/widgets", "7", "--miner-login", "alice", "--json"], { + env: { MINER_CODING_AGENT_PROVIDER: "noop", MINER_SKIP_TARGET_REPO_VERIFICATION: "1" }, + openWorktreeAllocator: () => allocator, + openClaimLedger: () => claimLedger, + initEventLedger: () => eventLedger, + initAttemptLog: () => attemptLog, + initGovernorLedger: () => governorLedger, + ...readyPipelineOptions({ runMinerAttempt: runMinerAttemptSpy }), + }); + + expect(runMinerAttemptSpy).toHaveBeenCalled(); + }); +}); diff --git a/test/unit/miner-attempt-runner.test.ts b/test/unit/miner-attempt-runner.test.ts index 48be8acde9..76af9d2b7b 100644 --- a/test/unit/miner-attempt-runner.test.ts +++ b/test/unit/miner-attempt-runner.test.ts @@ -184,6 +184,36 @@ describe("runMinerAttempt (#2337) — the real create->review->gate->submit pipe expect(result.loopResult.finalMeterTotals.tokens).toBe(1234); }); + it("#8807: a FAILED target-repo verification blocks the submission after handoff — no freshness read, no PR", async () => { + const executeLocalWrite = vi.fn(); + const fetchLiveIssueSnapshot = vi.fn(); + const verification = { + status: "failed", + checks: [{ kind: "test", command: "npm test", ok: false, exitCode: 1, outputTail: "1 failing" }], + firstFailure: { kind: "test", command: "npm test", ok: false, exitCode: 1, outputTail: "1 failing" }, + }; + const deps = baseDeps({ executeLocalWrite, fetchLiveIssueSnapshot, verifyTargetRepo: async () => verification }); + const result = await runMinerAttempt(baseAttemptInput(), deps); + + expect(result.outcome).toBe("verification_failed"); + if (result.outcome !== "verification_failed") throw new Error("expected verification_failed"); + expect(result.verification).toEqual(verification); + expect(result.loopResult.outcome).toBe("handoff"); // the agent DID hand off — the gate caught it after + expect(fetchLiveIssueSnapshot).not.toHaveBeenCalled(); // blocked BEFORE spending the freshness read + expect(executeLocalWrite).not.toHaveBeenCalled(); // and no PR was opened + }); + + it("#8807: a PASSED or SKIPPED verification proceeds to submit exactly as before; absent dep is byte-identical pre-#8807 flow", async () => { + for (const verification of [{ status: "passed", checks: [] }, { status: "skipped", reason: "stack_undetected" }]) { + const deps = baseDeps({ verifyTargetRepo: async () => verification }); + const result = await runMinerAttempt(baseAttemptInput(), deps); + expect(result.outcome, JSON.stringify(verification)).toBe("submitted"); + } + // No dep at all (older callers): the happy path above already pins this; assert explicitly anyway. + const result = await runMinerAttempt(baseAttemptInput(), baseDeps()); + expect(result.outcome).toBe("submitted"); + }); + it("defaults the open_pr body to an empty string when the loop input never set one", async () => { const deps = baseDeps(); const result = await runMinerAttempt(baseAttemptInput({ loopInput: passingLoopInput({ body: undefined }) }), deps); diff --git a/test/unit/miner-target-repo-verification.test.ts b/test/unit/miner-target-repo-verification.test.ts new file mode 100644 index 0000000000..c9e6c33d64 --- /dev/null +++ b/test/unit/miner-target-repo-verification.test.ts @@ -0,0 +1,105 @@ +import { describe, expect, it, vi } from "vitest"; +import { + DEFAULT_VERIFICATION_TIMEOUT_MS, + defaultVerificationSpawn, + runTargetRepoVerification, + VERIFICATION_OUTPUT_TAIL_CHARS, + type TargetRepoVerificationSpawn, +} from "../../packages/loopover-miner/lib/target-repo-verification.js"; +import type { RepoStackResult } from "../../packages/loopover-miner/lib/stack-detection.js"; + +// #8807: the independent quality gate — the target repo's own commands, never the agent's self-attestation. +function detectedStack(over: Partial> = {}): RepoStackResult { + return { + detected: true, + language: "javascript", + packageManager: "npm", + testCommand: "npm test", + lintCommand: null, + buildCommand: null, + formatCommand: null, + summary: "js", + ...over, + } as unknown as RepoStackResult; +} + +describe("runTargetRepoVerification (#8807)", () => { + it("runs detected commands in test → lint → build order from the worktree and passes when all succeed", async () => { + const calls: Array<{ command: string; cwd: string }> = []; + const spawn: TargetRepoVerificationSpawn = async (command, options) => { + calls.push({ command, cwd: options.cwd }); + return { code: 0, output: "ok" }; + }; + const result = await runTargetRepoVerification({ + worktreeDir: "/wt", + stack: detectedStack({ testCommand: "npm test", lintCommand: "npm run lint", buildCommand: "npm run build" }), + spawn, + }); + expect(result.status).toBe("passed"); + expect(calls.map((c) => c.command)).toEqual(["npm test", "npm run lint", "npm run build"]); + expect(calls.every((c) => c.cwd === "/wt")).toBe(true); + }); + + it("STOPS at the first failure with the failing command, exit code, and a BOUNDED output tail", async () => { + const spawn: TargetRepoVerificationSpawn = async (command) => + command === "npm test" ? { code: 1, output: "x".repeat(VERIFICATION_OUTPUT_TAIL_CHARS * 3) + "FAIL tail" } : { code: 0, output: "ok" }; + const result = await runTargetRepoVerification({ + worktreeDir: "/wt", + stack: detectedStack({ testCommand: "npm test", lintCommand: "npm run lint" }), + spawn, + }); + expect(result.status).toBe("failed"); + if (result.status !== "failed") throw new Error("unreachable"); + expect(result.firstFailure.command).toBe("npm test"); + expect(result.firstFailure.exitCode).toBe(1); + expect(result.firstFailure.outputTail.endsWith("FAIL tail")).toBe(true); + expect(result.firstFailure.outputTail.length).toBeLessThanOrEqual(VERIFICATION_OUTPUT_TAIL_CHARS); + expect(result.checks).toHaveLength(1); // lint never ran + }); + + it("a timeout-killed command (code null) is a failure, never a silent pass", async () => { + const spawn: TargetRepoVerificationSpawn = async () => ({ code: null, output: "killed" }); + const result = await runTargetRepoVerification({ worktreeDir: "/wt", stack: detectedStack(), spawn }); + expect(result.status).toBe("failed"); + }); + + it("SKIPS (recorded, never a failure) on an undetected stack or a stack with no inferred commands", async () => { + const spawn = vi.fn(); + const undetected = await runTargetRepoVerification({ + worktreeDir: "/wt", + stack: { detected: false, reason: "no markers" } as unknown as RepoStackResult, + spawn: spawn as never, + }); + expect(undetected).toEqual({ status: "skipped", reason: "stack_undetected" }); + const empty = await runTargetRepoVerification({ + worktreeDir: "/wt", + stack: detectedStack({ testCommand: null, lintCommand: null, buildCommand: null }), + spawn: spawn as never, + }); + expect(empty).toEqual({ status: "skipped", reason: "no_commands_detected" }); + expect(spawn).not.toHaveBeenCalled(); + }); + + it("the default spawn runs shell commands from the cwd, merges output, and enforces the timeout", async () => { + const ok = await defaultVerificationSpawn("echo hello && echo err 1>&2", { cwd: process.cwd(), timeoutMs: DEFAULT_VERIFICATION_TIMEOUT_MS }); + expect(ok.code).toBe(0); + expect(ok.output).toContain("hello"); + expect(ok.output).toContain("err"); + const fail = await defaultVerificationSpawn("exit 3", { cwd: process.cwd(), timeoutMs: DEFAULT_VERIFICATION_TIMEOUT_MS }); + expect(fail.code).toBe(3); + const killed = await defaultVerificationSpawn("sleep 30", { cwd: process.cwd(), timeoutMs: 200 }); + expect(killed.code).not.toBe(0); // SIGKILL → non-zero/null, treated as failure upstream + // Spawn-level error (nonexistent cwd): resolves code null with the error text — a failure, never a hang. + const errored = await defaultVerificationSpawn("echo hi", { cwd: "/nonexistent-dir-8807", timeoutMs: 5000 }); + expect(errored.code).toBeNull(); + expect(errored.output).toContain("ENOENT"); + }, 15_000); + + it("uses the default spawn when none is injected (the production arm) — a real fast command passes", async () => { + const result = await runTargetRepoVerification({ + worktreeDir: process.cwd(), + stack: detectedStack({ testCommand: "true", lintCommand: null, buildCommand: null }), + }); + expect(result.status).toBe("passed"); + }, 15_000); +}); From 7127ce4780d50a5b50d556cc5c0f94083439b515 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 01:26:00 -0700 Subject: [PATCH 2/3] chore(miner): regenerate env reference for MINER_SKIP_TARGET_REPO_VERIFICATION --- apps/loopover-ui/src/lib/ams-env-reference.ts | 6 ++++++ packages/loopover-miner/docs/env-reference.md | 1 + 2 files changed, 7 insertions(+) diff --git a/apps/loopover-ui/src/lib/ams-env-reference.ts b/apps/loopover-ui/src/lib/ams-env-reference.ts index 67f29e1f9a..3e1ffaf34f 100644 --- a/apps/loopover-ui/src/lib/ams-env-reference.ts +++ b/apps/loopover-ui/src/lib/ams-env-reference.ts @@ -241,6 +241,11 @@ export const AMS_ENV_REFERENCE_ROWS: MinerEnvReferenceRow[] = [ firstReference: "packages/loopover-engine/src/miner/driver-factory.ts", defaultValue: null, }, + { + name: "MINER_SKIP_TARGET_REPO_VERIFICATION", + firstReference: "lib/attempt-cli.ts", + defaultValue: "", + }, ]; export const AMS_ENV_REFERENCE_MARKDOWN = [ @@ -297,5 +302,6 @@ export const AMS_ENV_REFERENCE_MARKDOWN = [ '| `MINER_CODING_AGENT_PAUSED` | `packages/loopover-engine/src/miner/coding-agent-mode.ts` | `""` |', '| `MINER_CODING_AGENT_PROVIDER` | `lib/laptop-init.ts` | `""` |', "| `MINER_CODING_AGENT_TIMEOUT_MS` | `packages/loopover-engine/src/miner/driver-factory.ts` | (none) |", + '| `MINER_SKIP_TARGET_REPO_VERIFICATION` | `lib/attempt-cli.ts` | `""` |', "", ].join("\n"); diff --git a/packages/loopover-miner/docs/env-reference.md b/packages/loopover-miner/docs/env-reference.md index 8d25d3fda0..a512fe19a0 100644 --- a/packages/loopover-miner/docs/env-reference.md +++ b/packages/loopover-miner/docs/env-reference.md @@ -51,3 +51,4 @@ Generated by `npm run miner:env-reference`. Do not edit manually. | `MINER_CODING_AGENT_PAUSED` | `packages/loopover-engine/src/miner/coding-agent-mode.ts` | `""` | | `MINER_CODING_AGENT_PROVIDER` | `lib/laptop-init.ts` | `""` | | `MINER_CODING_AGENT_TIMEOUT_MS` | `packages/loopover-engine/src/miner/driver-factory.ts` | (none) | +| `MINER_SKIP_TARGET_REPO_VERIFICATION` | `lib/attempt-cli.ts` | `""` | From d344d90ba7969e17bfb944e996a16005f392c802 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 01:49:00 -0700 Subject: [PATCH 3/3] fix(miner): kill the whole verification process group on timeout and settle bounded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The default verification spawn killed only the shell at the timeout; on Linux the command's descendants survive, keep the stdio pipes open, and stall the close event (observed as a 30s hang in CI). Spawn detached, kill the negative pid, and add a bounded post-kill settle so the gate can never outlive its per-command timeout even if an orphan escapes the group. Also fix the queue.test.ts/queue-2.test.ts fetch stubs whose generic {} fallback broke the close-explanation marker search (the comment list endpoint must return an array) — a latent mock-shape gap surfaced by routing enforced closes through createOrUpdateCloseExplanationComment. --- .../lib/target-repo-verification.ts | 64 ++++++++++++++++--- .../miner-target-repo-verification.test.ts | 30 +++++++++ test/unit/queue-2.test.ts | 5 ++ test/unit/queue.test.ts | 5 ++ 4 files changed, 95 insertions(+), 9 deletions(-) diff --git a/packages/loopover-miner/lib/target-repo-verification.ts b/packages/loopover-miner/lib/target-repo-verification.ts index 10b3df985f..2cf695af4c 100644 --- a/packages/loopover-miner/lib/target-repo-verification.ts +++ b/packages/loopover-miner/lib/target-repo-verification.ts @@ -14,6 +14,9 @@ import { spawn as nodeSpawn } from "node:child_process"; import type { RepoStackResult } from "./stack-detection.js"; +/** The slice of ChildProcess the tree-kill needs — narrow so tests can drive both arms with plain fakes. */ +export type KillableChild = { pid?: number | undefined; kill: (signal: NodeJS.Signals) => boolean }; + export type TargetRepoVerificationSpawn = ( command: string, options: { cwd: string; timeoutMs: number }, @@ -39,29 +42,72 @@ export const DEFAULT_VERIFICATION_TIMEOUT_MS = 10 * 60 * 1000; /** Postmortem detail bound — enough tail to show the failing assertion, never an unbounded log dump. */ export const VERIFICATION_OUTPUT_TAIL_CHARS = 4000; +/** Post-timeout grace before giving up on the `close` event: a killed process group's pipes close nearly + * instantly, so this only fires when something double-forked out of the group and kept the pipes open — + * the gate resolves as failed rather than hanging on that orphan. */ +export const VERIFICATION_KILL_SETTLE_MS = 5000; + +/** Kill the command's whole detached process group via the NEGATIVE pid: a test command is routinely a tree + * (`npm test` → node → workers), and killing only the shell leaves grandchildren holding the stdio pipes — + * the `close` event then waits on THEM, stalling the gate far past its own timeout (observed as the 30s + * hang on Linux CI). Falls back to the plain single-process kill when the group kill isn't possible + * (no pid, or the group is already gone and the signal throws). */ +export function killVerificationProcessTree(child: KillableChild, killGroup: (pid: number, signal: NodeJS.Signals) => void = (pid, signal) => process.kill(-pid, signal)): void { + try { + if (typeof child.pid !== "number") throw new Error("child has no pid"); + killGroup(child.pid, "SIGKILL"); + } catch { + child.kill("SIGKILL"); + } +} + /** Default spawn: shell-executed (detected commands are shell strings like "npm test" / "ruff check ."), - * merged stdout+stderr, killed at the timeout (a killed process reports code null → treated as failure). */ -export const defaultVerificationSpawn: TargetRepoVerificationSpawn = (command, options) => - new Promise((resolve) => { - const child = nodeSpawn(command, { cwd: options.cwd, shell: true, stdio: ["ignore", "pipe", "pipe"] }); + * merged stdout+stderr, killed at the timeout (a killed/timed-out command reports code null → treated as + * failure upstream). `internals` exists ONLY for tests to reach the timeout/settle arms deterministically; + * production callers always take the defaults. */ +export function runShellCommandWithTreeKill( + command: string, + options: { cwd: string; timeoutMs: number }, + internals: { killTree?: (child: KillableChild) => void; settleMs?: number } = {}, +): Promise<{ code: number | null; output: string }> { + const killTree = internals.killTree ?? killVerificationProcessTree; + const settleMs = internals.settleMs ?? VERIFICATION_KILL_SETTLE_MS; + return new Promise((resolve) => { + // detached: its own process group, so the timeout can kill the entire tree, not just the shell. + const child = nodeSpawn(command, { cwd: options.cwd, shell: true, stdio: ["ignore", "pipe", "pipe"], detached: true }); let output = ""; + let settled = false; + let settleTimer: ReturnType | undefined; + const finish = (result: { code: number | null; output: string }) => { + if (settled) return; + settled = true; + clearTimeout(timer); + if (settleTimer !== undefined) clearTimeout(settleTimer); + resolve(result); + }; const capture = (chunk: Buffer) => { output = (output + chunk.toString()).slice(-VERIFICATION_OUTPUT_TAIL_CHARS * 4); }; child.stdout?.on("data", capture); child.stderr?.on("data", capture); const timer = setTimeout(() => { - child.kill("SIGKILL"); + killTree(child); + // Bounded settle: if some orphan still holds the pipes open after the group kill, resolve as a + // timeout failure anyway — the verification gate must never outlive its own per-command bound. + settleTimer = setTimeout(() => { + finish({ code: null, output: `${output}\n[verification timeout after ${options.timeoutMs}ms — process tree killed]` }); + }, settleMs); }, options.timeoutMs); child.on("error", (error) => { - clearTimeout(timer); - resolve({ code: null, output: `${output}\n${String(error)}` }); + finish({ code: null, output: `${output}\n${String(error)}` }); }); child.on("close", (code) => { - clearTimeout(timer); - resolve({ code, output }); + finish({ code, output }); }); }); +} + +export const defaultVerificationSpawn: TargetRepoVerificationSpawn = (command, options) => runShellCommandWithTreeKill(command, options); export async function runTargetRepoVerification(options: { worktreeDir: string; diff --git a/test/unit/miner-target-repo-verification.test.ts b/test/unit/miner-target-repo-verification.test.ts index c9e6c33d64..f0dd79c35c 100644 --- a/test/unit/miner-target-repo-verification.test.ts +++ b/test/unit/miner-target-repo-verification.test.ts @@ -2,6 +2,8 @@ import { describe, expect, it, vi } from "vitest"; import { DEFAULT_VERIFICATION_TIMEOUT_MS, defaultVerificationSpawn, + killVerificationProcessTree, + runShellCommandWithTreeKill, runTargetRepoVerification, VERIFICATION_OUTPUT_TAIL_CHARS, type TargetRepoVerificationSpawn, @@ -102,4 +104,32 @@ describe("runTargetRepoVerification (#8807)", () => { }); expect(result.status).toBe("passed"); }, 15_000); + + it("killVerificationProcessTree kills the GROUP via negative pid, falling back to the single-process kill on no-pid or a thrown group kill", () => { + const groupKills: Array<[number, string]> = []; + const kill = vi.fn().mockReturnValue(true); + // Happy arm: pid present → group kill, no single-process fallback. + killVerificationProcessTree({ pid: 4242, kill }, (pid, signal) => void groupKills.push([pid, signal])); + expect(groupKills).toEqual([[4242, "SIGKILL"]]); + expect(kill).not.toHaveBeenCalled(); + // No-pid arm (spawn failed before assigning one) → single-process fallback. + killVerificationProcessTree({ pid: undefined, kill }); + expect(kill).toHaveBeenCalledWith("SIGKILL"); + // Thrown-group-kill arm (group already reaped → ESRCH) → single-process fallback. + const killAfterThrow = vi.fn().mockReturnValue(true); + killVerificationProcessTree({ pid: 4242, kill: killAfterThrow }, () => { + throw new Error("ESRCH"); + }); + expect(killAfterThrow).toHaveBeenCalledWith("SIGKILL"); + }); + + it("the bounded settle resolves a timeout even when the kill leaves an orphan holding the pipes (the gate can never hang)", async () => { + // A no-op killTree simulates a survivor that keeps stdout open past the kill: the settle timer must + // still resolve the spawn as a timeout failure instead of waiting for the orphan's own exit. + const result = await runShellCommandWithTreeKill("sleep 1", { cwd: process.cwd(), timeoutMs: 150 }, { killTree: () => undefined, settleMs: 100 }); + expect(result.code).toBeNull(); + expect(result.output).toContain("verification timeout after 150ms"); + // Let the (never-killed) command's own close event fire afterwards: the settled guard must ignore it. + await new Promise((r) => setTimeout(r, 1100)); + }, 15_000); }); diff --git a/test/unit/queue-2.test.ts b/test/unit/queue-2.test.ts index 54a3aaac35..8d99d954c9 100644 --- a/test/unit/queue-2.test.ts +++ b/test/unit/queue-2.test.ts @@ -341,6 +341,11 @@ describe("queue processors", () => { if (url.includes("/commits/c9/status")) return Response.json({ state: "success", statuses: [] }); if (url.endsWith("/pulls/9/reviews") && init?.method === "POST") return Response.json({ id: 1 }); if (url.endsWith("/pulls/9/reviews")) return Response.json([]); + // The one-shot close posts a close-explanation comment (createOrUpdateCloseExplanationComment): its + // marker search LISTS the PR's comments first, and the list endpoint must return an ARRAY -- the + // generic `Response.json({})` fallback below would make the close action itself fail. + if (url.includes("/issues/9/comments") && init?.method === "POST") return Response.json({ id: 91 }); + if (url.includes("/issues/9/comments")) return Response.json([]); if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); return Response.json({}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 383c06b872..fba4e25b58 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -2729,6 +2729,11 @@ describe("queue processors", () => { if (index >= 0) liveLabels.splice(index, 1); return new Response(null, { status: 204 }); } + // The enforced close posts a close-explanation comment (createOrUpdateCloseExplanationComment): + // its marker search LISTS the PR's comments first, and the list endpoint must return an ARRAY -- + // the generic `Response.json({})` fallback below would make the close action itself fail. + if (url.includes("/issues/7/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/7/comments") && method === "POST") return Response.json({ id: 71 }); // The `.loopover.yml`/`.json` content fetch (raw.githubusercontent.com) is the ONLY place // linkedIssueHardRules can be turned on (see the comment above). Batch-A settings ride along on BOTH // arms so their effective value stays "off" regardless of whether the hard rule itself is enabled.