Skip to content

feat(miner): capture feasibility-verdict rule-fired signals in attempt-cli (#8543) #8557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
philluiz2323 wants to merge 1 commit into from
Closed

feat(miner): capture feasibility-verdict rule-fired signals in attempt-cli (#8543) #8557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 48 additions & 2 deletions packages/loopover-miner/lib/attempt-cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
resolveCodingAgentModeFromConfig,
resolveFirstConfiguredCodingAgentDriverName,
} from "@loopover/engine";
import type { AttemptDbFork, AttemptDbForkConfig, CodingAgentExecutionMode, FeasibilityVerdict, LocalWriteActionSpec } from "@loopover/engine";
import type { AttemptDbFork, AttemptDbForkConfig, CodingAgentExecutionMode, FeasibilityVerdict, LocalWriteActionSpec, SignalStore } from "@loopover/engine";
import { argsWantJson, describeCliError, reportCliFailure } from "./cli-error.js";
import { resolveAttemptDbForkConfig } from "./attempt-db-fork-config.js";
import { constructProductionCodingAgentDriver } from "./coding-agent-construction.js";
Expand All @@ -33,8 +33,9 @@ import { resolveMinerGoalSpec } from "./miner-goal-spec.js";
import { resolveClaimConflict } from "./claim-conflict-resolver.js";
import type { ClaimConflictResult, resolveClaimConflict as ResolveClaimConflictFn } from "./claim-conflict-resolver.js";
import { parsePrNumberFromExecResult } from "./pr-number-parse.js";
import { initEventLedger } from "./event-ledger.js";
import { appendEvent, initEventLedger, readEvents } from "./event-ledger.js";
import type { EventLedger } from "./event-ledger.js";
import { createSignalTrackingStore } from "./signal-tracking-store.js";
import { initAttemptLog } from "./attempt-log.js";
import type { AttemptLog } from "./attempt-log.js";
import { initGovernorLedger } from "./governor-ledger.js";
Expand Down Expand Up @@ -137,6 +138,7 @@ export type RunAttemptOptions = {
initEventLedger?: () => EventLedger;
initAttemptLog?: () => AttemptLog;
initGovernorLedger?: () => GovernorLedger;
initSignalTrackingStore?: () => SignalStore;
buildAttemptDeps?: typeof buildAttemptDeps;
resolveRejectionSignaled?: typeof ResolveRejectionSignaledFn;
fetchImpl?: SelfReviewContextFetch;
Expand Down Expand Up @@ -291,6 +293,44 @@ export function buildAttemptDeps(
};
}

// #8543: the default SignalStore, backed by the miner's own shared local event ledger -- same singleton
// discover-cli.ts's initDefaultSignalTrackingStore already uses, no extra lifecycle management needed here.
function initDefaultSignalTrackingStore(): SignalStore {
return createSignalTrackingStore({ appendEvent, readEvents });
}

// #8543: records each avoid/raise reason a feasibility verdict fired as a rule-fired signal, so AMS can
// precision-score feasibility rules the same way discover-cli.ts's recordEligibilityExclusionSignals already
// does for eligibility exclusions. Best-effort, mirrored verbatim from that sibling: a store-open failure or a
// single write failure never changes the CLI's console output, JSON result shape, or exit code -- the
// infeasible branch still returns 4 either way. Every ready:false occurrence records, no dedup across repeated
// attempts at the same issue -- each attempt is a distinct decision instance.
async function recordFeasibilityVerdictSignals(
feasibility: { avoidReasons: readonly string[]; raiseReasons: readonly string[] },
target: { repoFullName: string; issueNumber: number },
options: Pick<RunAttemptOptions, "initSignalTrackingStore">,
nowMs: number,
): Promise<void> {
let store: SignalStore | null = null;
try {
store = (options.initSignalTrackingStore ?? initDefaultSignalTrackingStore)();
} catch {
store = null;
}
if (!store) return;
const occurredAt = new Date(nowMs).toISOString();
const targetKey = `${target.repoFullName}#issue-${target.issueNumber}`;
const fired: Array<{ ruleId: string; outcome: "avoid" | "raise" }> = [
...feasibility.avoidReasons.map((ruleId) => ({ ruleId, outcome: "avoid" as const })),
...feasibility.raiseReasons.map((ruleId) => ({ ruleId, outcome: "raise" as const })),
];
for (const entry of fired) {
await store
.recordRuleFired({ ruleId: entry.ruleId, targetKey, outcome: entry.outcome, occurredAt })
.catch(() => undefined);
}
}

/**
* Run the `attempt` CLI subcommand end to end: resolveRejectionSignaled (before consuming a worktree slot) ->
* acquire a concurrency slot -> assemble real AttemptDeps -> prepare a REAL git worktree -> fetch a real
Expand Down Expand Up @@ -520,6 +560,12 @@ export async function runAttempt(args: string[], options: RunAttemptOptions = {}

if (!codingTaskSpec.ready) {
const reason = `infeasible_${codingTaskSpec.verdict}`;
await recordFeasibilityVerdictSignals(
codingTaskSpec.feasibility,
{ repoFullName: parsed.repoFullName, issueNumber: parsed.issueNumber },
options,
nowMs,
);
attemptLog.appendAttemptLogEvent({
eventType: "attempt_aborted",
attemptId,
Expand Down
165 changes: 165 additions & 0 deletions test/unit/miner-attempt-cli.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,17 @@ function fakeLoopResult(overrides: Record<string, unknown> = {}) {
};
}

/** A no-op SignalStore double (#8543), for tests that reach the infeasible branch but don't themselves assert
* on signal capture -- keeps them off the real on-disk event-ledger fallback under ~/.config, the same leak
* class discover-cli.ts's own initDefaultSignalTrackingStore comment already documents. */
function fakeSignalStore() {
return {
recordRuleFired: vi.fn(async (_event: { ruleId: string; targetKey: string; outcome: string; occurredAt: string }) => undefined),
recordHumanOverride: vi.fn(async () => undefined),
queryRuleHistory: vi.fn(async () => ({ fired: [], overrides: [] })),
};
}

/** The default set of injected options a test needs to reach past every real dependency and into (or
* through) the final runMinerAttempt call, without doing any real network/git/subprocess work. */
function readyPipelineOptions(overrides: Record<string, unknown> = {}) {
Expand Down Expand Up @@ -1192,6 +1203,7 @@ describe("runAttempt (#5132)", () => {
initEventLedger: () => eventLedger,
initAttemptLog: () => attemptLog,
initGovernorLedger: () => governorLedger,
initSignalTrackingStore: () => fakeSignalStore(),
...readyPipelineOptions({
buildCodingTaskSpec: () => ({
ready: false,
Expand Down Expand Up @@ -1225,6 +1237,157 @@ describe("runAttempt (#5132)", () => {
expect(cleanupAttemptWorktreeSpy).toHaveBeenCalledWith(expect.any(String), expect.any(String), true);
});

describe("feasibility-verdict signal capture (#8543)", () => {
it("records one rule-fired signal per avoid AND raise reason, with the exact ruleId/outcome/targetKey shape", async () => {
const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers();
vi.spyOn(console, "log").mockImplementation(() => undefined);
const store = fakeSignalStore();

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,
initSignalTrackingStore: () => store,
...readyPipelineOptions({
buildCodingTaskSpec: () => ({
ready: false,
verdict: "avoid",
feasibility: {
verdict: "avoid",
avoidReasons: ["claim_status_solved", "issue_quality_do_not_use"],
raiseReasons: ["duplicate_cluster_high"],
summary: "not feasible",
},
}),
}),
});

expect(store.recordRuleFired).toHaveBeenCalledTimes(3);
expect(store.recordRuleFired).toHaveBeenCalledWith(
expect.objectContaining({ ruleId: "claim_status_solved", outcome: "avoid", targetKey: "acme/widgets#issue-7" }),
);
expect(store.recordRuleFired).toHaveBeenCalledWith(
expect.objectContaining({ ruleId: "issue_quality_do_not_use", outcome: "avoid", targetKey: "acme/widgets#issue-7" }),
);
expect(store.recordRuleFired).toHaveBeenCalledWith(
expect.objectContaining({ ruleId: "duplicate_cluster_high", outcome: "raise", targetKey: "acme/widgets#issue-7" }),
);
// No metadata (raw-context capture is a separate issue -- not this one) and a well-formed ISO timestamp.
for (const [event] of store.recordRuleFired.mock.calls) {
expect(event).not.toHaveProperty("metadata");
expect(new Date(event.occurredAt).toISOString()).toBe(event.occurredAt);
}
});

it("records zero fired events on the ready:true path", async () => {
const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers();
vi.spyOn(console, "log").mockImplementation(() => undefined);
const store = fakeSignalStore();

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,
initSignalTrackingStore: () => store,
...readyPipelineOptions({ runMinerAttempt: vi.fn(async () => ({ outcome: "submitted", loopResult: fakeLoopResult() }) as never) }),
});

expect(store.recordRuleFired).not.toHaveBeenCalled();
});

it("a throwing initSignalTrackingStore never changes the exit code, console output, or JSON result", async () => {
const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers();
const log = vi.spyOn(console, "log").mockImplementation(() => undefined);

const exitCode = await runAttempt(["acme/widgets", "7", "--miner-login", "alice", "--json"], {
env: { MINER_CODING_AGENT_PROVIDER: "noop" },
attemptId: "infeasible-attempt",
openWorktreeAllocator: () => allocator,
openClaimLedger: () => claimLedger,
initEventLedger: () => eventLedger,
initAttemptLog: () => attemptLog,
initGovernorLedger: () => governorLedger,
initSignalTrackingStore: () => {
throw new Error("store unavailable");
},
...readyPipelineOptions({
buildCodingTaskSpec: () => ({
ready: false,
verdict: "raise",
feasibility: { verdict: "raise", avoidReasons: [], raiseReasons: ["target_not_found"], summary: "issue not found" },
}),
}),
});

expect(exitCode).toBe(4);
expect(JSON.parse(String(log.mock.calls[0]?.[0]))).toEqual({
outcome: "blocked_infeasible",
reason: "infeasible_raise",
verdict: "raise",
avoidReasons: [],
raiseReasons: ["target_not_found"],
repoFullName: "acme/widgets",
issueNumber: 7,
minerLogin: "alice",
base: "main",
mode: "dry_run",
attemptId: "infeasible-attempt",
});
});

it("a store whose recordRuleFired rejects never changes the exit code, console output, or JSON result", async () => {
const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers();
const log = vi.spyOn(console, "log").mockImplementation(() => undefined);
const rejectingStore = {
recordRuleFired: vi.fn(async () => {
throw new Error("write failed");
}),
recordHumanOverride: vi.fn(async () => undefined),
queryRuleHistory: vi.fn(async () => ({ fired: [], overrides: [] })),
};

const exitCode = await runAttempt(["acme/widgets", "7", "--miner-login", "alice", "--json"], {
env: { MINER_CODING_AGENT_PROVIDER: "noop" },
attemptId: "infeasible-attempt",
openWorktreeAllocator: () => allocator,
openClaimLedger: () => claimLedger,
initEventLedger: () => eventLedger,
initAttemptLog: () => attemptLog,
initGovernorLedger: () => governorLedger,
initSignalTrackingStore: () => rejectingStore,
...readyPipelineOptions({
buildCodingTaskSpec: () => ({
ready: false,
verdict: "raise",
feasibility: { verdict: "raise", avoidReasons: [], raiseReasons: ["target_not_found"], summary: "issue not found" },
}),
}),
});

expect(exitCode).toBe(4);
expect(JSON.parse(String(log.mock.calls[0]?.[0]))).toEqual({
outcome: "blocked_infeasible",
reason: "infeasible_raise",
verdict: "raise",
avoidReasons: [],
raiseReasons: ["target_not_found"],
repoFullName: "acme/widgets",
issueNumber: 7,
minerLogin: "alice",
base: "main",
mode: "dry_run",
attemptId: "infeasible-attempt",
});
expect(rejectingStore.recordRuleFired).toHaveBeenCalledTimes(1);
});
});

it("REGRESSION: infeasible WITHOUT --json prints the feasibility verdict on stderr and exits 4", async () => {
const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers();
const error = vi.spyOn(console, "error").mockImplementation(() => undefined);
Expand All @@ -1236,6 +1399,7 @@ describe("runAttempt (#5132)", () => {
initEventLedger: () => eventLedger,
initAttemptLog: () => attemptLog,
initGovernorLedger: () => governorLedger,
initSignalTrackingStore: () => fakeSignalStore(),
...readyPipelineOptions({
buildCodingTaskSpec: () => ({
ready: false,
Expand Down Expand Up @@ -1879,6 +2043,7 @@ describe("runAttempt: real claim-ledger wiring (#5393)", () => {
initEventLedger: () => eventLedger,
initAttemptLog: () => attemptLog,
initGovernorLedger: () => governorLedger,
initSignalTrackingStore: () => fakeSignalStore(),
...readyPipelineOptions({
buildCodingTaskSpec: () => ({
ready: false,
Expand Down