diff --git a/packages/gittensory-miner/lib/attempt-cli.js b/packages/gittensory-miner/lib/attempt-cli.js index 75ffa7c153..0465a40667 100644 --- a/packages/gittensory-miner/lib/attempt-cli.js +++ b/packages/gittensory-miner/lib/attempt-cli.js @@ -399,7 +399,10 @@ export async function runAttempt(args, options = {}) { amsPolicySpec: amsPolicy.spec, branchRef: worktreeResult.branchName, }); - const governor = buildAttemptGovernorContext(env, amsPolicy.spec, repoPaused); + // Real per-issue attempt history (#5654): the queue-driven caller (loop-cli) reads it from the portfolio-queue + // store and passes it through; a one-off direct attempt has none, so buildAttemptGovernorContext fails open to a + // fresh, never-attempted item rather than fabricating history here. + const governor = buildAttemptGovernorContext(env, amsPolicy.spec, repoPaused, options.convergenceInput); // Real soft-claim (#5393): recorded once we've committed to a real attempt (past feasibility), so a // sibling miner process on this machine sees it via claimLedger.listClaims/listActiveClaims while this diff --git a/packages/gittensory-miner/lib/attempt-input-builder.d.ts b/packages/gittensory-miner/lib/attempt-input-builder.d.ts index 9df919cc5e..770b8fdf4f 100644 --- a/packages/gittensory-miner/lib/attempt-input-builder.d.ts +++ b/packages/gittensory-miner/lib/attempt-input-builder.d.ts @@ -1,4 +1,10 @@ -import type { AmsPolicySpec, CodingAgentExecutionMode, IterateLoopInput, SelfReviewContext } from "@loopover/engine"; +import type { + AmsPolicySpec, + CodingAgentExecutionMode, + IterateLoopInput, + PortfolioConvergenceInput, + SelfReviewContext, +} from "@loopover/engine"; import type { AttemptGovernorContext } from "./attempt-runner.js"; import type { CodingTaskSpecResult } from "./coding-task-spec.js"; @@ -6,6 +12,7 @@ export function buildAttemptGovernorContext( env: Record, amsPolicySpec: AmsPolicySpec, repoPaused?: boolean, + convergenceInput?: PortfolioConvergenceInput, ): AttemptGovernorContext; export type BuildAttemptLoopInputInput = { diff --git a/packages/gittensory-miner/lib/attempt-input-builder.js b/packages/gittensory-miner/lib/attempt-input-builder.js index 3b2f11c8ac..b5aa35efe2 100644 --- a/packages/gittensory-miner/lib/attempt-input-builder.js +++ b/packages/gittensory-miner/lib/attempt-input-builder.js @@ -6,13 +6,6 @@ import { isGlobalMinerKillSwitch, isGlobalMinerLiveModeOptIn } from "@loopover/e // same discipline as coding-task-spec.js's own composers. // // KNOWN, DOCUMENTED GAPS (not fabricated -- explicitly left as real, narrow follow-ups): -// - governor.convergenceInput is a first-attempt-shaped literal ({ attempts: 0, consecutiveFailures: 0, -// reenqueues: 0, reachedDone: false }), not a real per-issue query. attempt-log.js's schema has no -// repo+issue index (attemptId embeds a timestamp, so it's not a stable group key), and reenqueue counts -// aren't tracked ANYWHERE yet (non-convergence.ts's own header says that belongs on the portfolio-queue -// table once it grows attempt-history columns -- a real, separate schema change, not something to fake -// here). This literal is only ever an UNDER-estimate (reads "fresh, no prior failures" even on a real -// Nth attempt), which fails toward LETTING an attempt through, not blocking one -- documented, not silent. // - governor.reputationHistory/selfPlagiarismCandidate/selfPlagiarismRecentSubmissions are omitted, which // chokepoint.ts's own design treats as "skip that stage entirely" -- an honest absence, not a fabricated // "clean" verdict. @@ -26,18 +19,25 @@ import { isGlobalMinerKillSwitch, isGlobalMinerLiveModeOptIn } from "@loopover/e * (miner-goal-spec.js's resolveMinerGoalSpec) -- this composer stays pure and just threads whatever the * caller already resolved through; passing nothing keeps the prior fails-open-on-that-axis-only behavior. * + * `convergenceInput` (#5654) is the caller's real per-issue attempt history read from the portfolio-queue store + * (portfolio-queue.js `getAttemptHistory`). This composer stays pure and just forwards whatever the caller + * resolved; when the caller has none (e.g. a one-off direct attempt not driven by the queue) it defaults to a + * fresh, never-attempted item -- an UNDER-estimate that fails toward LETTING an attempt through, the same + * fail-open posture this axis had before real history existed. + * * @param {Record} env * @param {import("@loopover/engine").AmsPolicySpec} amsPolicySpec * @param {boolean} [repoPaused] + * @param {import("@loopover/engine").PortfolioConvergenceInput} [convergenceInput] * @returns {import("./attempt-runner.js").AttemptGovernorContext} */ -export function buildAttemptGovernorContext(env, amsPolicySpec, repoPaused) { +export function buildAttemptGovernorContext(env, amsPolicySpec, repoPaused, convergenceInput) { return { killSwitchGlobal: isGlobalMinerKillSwitch(env), killSwitchRepoPaused: repoPaused, liveModeGlobalOptIn: isGlobalMinerLiveModeOptIn(env), capLimits: amsPolicySpec.capLimits, - convergenceInput: { attempts: 0, consecutiveFailures: 0, reenqueues: 0, reachedDone: false }, + convergenceInput: convergenceInput ?? { attempts: 0, consecutiveFailures: 0, reenqueues: 0, reachedDone: false }, }; } diff --git a/packages/gittensory-miner/lib/loop-cli.js b/packages/gittensory-miner/lib/loop-cli.js index 9b6ff15131..82dc69ba67 100644 --- a/packages/gittensory-miner/lib/loop-cli.js +++ b/packages/gittensory-miner/lib/loop-cli.js @@ -394,6 +394,10 @@ export async function runLoop(args, options = {}) { await runAttemptFn(attemptArgv, { ...(options.attemptOptions ?? {}), env, + // Real per-issue attempt history (#5654): the durable portfolio-queue counters (bumped by the claim above + // and every prior requeue/reclaim) feed this attempt's Governor chokepoint context, so its non-convergence + // detector sees genuine "Nth attempt, M prior re-enqueues" state instead of the old hardcoded "fresh" literal. + convergenceInput: portfolioQueue.getAttemptHistory(claimed.repoFullName, claimed.identifier, claimed.apiBaseUrl), onResult: (result) => { lastResult = result; }, diff --git a/packages/gittensory-miner/lib/portfolio-queue.d.ts b/packages/gittensory-miner/lib/portfolio-queue.d.ts index 15c9ff9d57..3633938086 100644 --- a/packages/gittensory-miner/lib/portfolio-queue.d.ts +++ b/packages/gittensory-miner/lib/portfolio-queue.d.ts @@ -1,3 +1,5 @@ +import type { PortfolioConvergenceInput } from "@loopover/engine"; + export type QueueStatus = "queued" | "in_progress" | "done"; export type QueueEntry = { @@ -35,6 +37,12 @@ export type PortfolioQueueStore = { markFailed(repoFullName: string, identifier: string, apiBaseUrl?: string): QueueEntry | null; reclaimStuckItem(repoFullName: string, identifier: string, apiBaseUrl?: string): QueueEntry | null; requeueItem(repoFullName: string, identifier: string, apiBaseUrl?: string): QueueEntry | null; + /** Real per-item attempt history (#5654) for the Governor's non-convergence detector. */ + getAttemptHistory( + repoFullName: string, + identifier: string, + apiBaseUrl?: string, + ): PortfolioConvergenceInput; batchClaim( selectFn: ( entries: QueueEntry[], diff --git a/packages/gittensory-miner/lib/portfolio-queue.js b/packages/gittensory-miner/lib/portfolio-queue.js index d86069241e..7929ec4dd0 100644 --- a/packages/gittensory-miner/lib/portfolio-queue.js +++ b/packages/gittensory-miner/lib/portfolio-queue.js @@ -147,6 +147,23 @@ export function initPortfolioQueueStore(dbPath = resolvePortfolioQueueDbPath()) migrationDb.exec("DROP TABLE miner_portfolio_queue"); migrationDb.exec("ALTER TABLE miner_portfolio_queue_v3 RENAME TO miner_portfolio_queue"); }, + // v3 -> v4 (#5654): grow the attempt-history columns the Governor's non-convergence detector needs + // (packages/gittensory-engine/src/portfolio/non-convergence.ts, whose header names the portfolio-queue table + // as where these belong). `attempts` counts claims, `reenqueues` counts in_progress -> queued transitions that + // never reached done, `consecutive_failures` is those same transitions since the last markDone. All three are + // NOT NULL DEFAULT 0, so every pre-existing row reads as "fresh, no history yet" until next claimed/requeued -- + // an additive migration. Defensive per-column table_info check (same idiom as the `leased_at` migration above) + // so a store already carrying them is never re-altered into a duplicate-column error. + (migrationDb) => { + const columns = migrationDb.prepare("PRAGMA table_info(miner_portfolio_queue)").all(); + const hasColumn = (name) => columns.some((column) => column.name === name); + if (!hasColumn("attempts")) + migrationDb.exec("ALTER TABLE miner_portfolio_queue ADD COLUMN attempts INTEGER NOT NULL DEFAULT 0"); + if (!hasColumn("consecutive_failures")) + migrationDb.exec("ALTER TABLE miner_portfolio_queue ADD COLUMN consecutive_failures INTEGER NOT NULL DEFAULT 0"); + if (!hasColumn("reenqueues")) + migrationDb.exec("ALTER TABLE miner_portfolio_queue ADD COLUMN reenqueues INTEGER NOT NULL DEFAULT 0"); + }, ]); // `rowid` is a stable, unique key assigned once at first insert (re-enqueue updates in place, never re-inserts), @@ -174,8 +191,10 @@ export function initPortfolioQueueStore(dbPath = resolvePortfolioQueueDbPath()) // cross-host priority ordering, not a per-host one. // Claiming stamps `leased_at` with the caller-supplied claim time; leaving 'in_progress' (done/failed/reclaim) // clears it back to NULL so only genuinely in-flight rows carry a lease. + // Claiming counts an attempt (#5654): every queued -> in_progress flip is one more try on the item, so the + // Governor's non-convergence detector can tell a genuine Nth attempt from a first one. const dequeueStatement = db.prepare(` - UPDATE miner_portfolio_queue SET status = 'in_progress', leased_at = ? + UPDATE miner_portfolio_queue SET status = 'in_progress', leased_at = ?, attempts = attempts + 1 WHERE rowid = ( SELECT rowid FROM miner_portfolio_queue WHERE status = 'queued' ${ORDER} LIMIT 1 ) @@ -183,13 +202,19 @@ export function initPortfolioQueueStore(dbPath = resolvePortfolioQueueDbPath()) `); // RETURNING (rather than a separate post-UPDATE SELECT) makes the "nothing to mark done" case observable // directly from one atomic statement. + // Reaching done resets the consecutive-failure streak to 0 (#5654): progress was made, so the item is no longer + // "stuck". `attempts`/`reenqueues` are cumulative lifetime totals and deliberately survive a done. const markDoneStatement = db.prepare(` - UPDATE miner_portfolio_queue SET status = 'done', leased_at = NULL + UPDATE miner_portfolio_queue SET status = 'done', leased_at = NULL, consecutive_failures = 0 WHERE api_base_url = ? AND repo_full_name = ? AND identifier = ? AND status <> 'done' RETURNING * `); + // A failed/halted attempt cycles in_progress -> queued WITHOUT reaching done (#5654) -- exactly non-convergence.ts's + // reenqueue trigger -- so bump both the lifetime reenqueue count and the consecutive-failure streak. (requeueItem, + // which cycles a COMPLETED done row, is deliberately excluded: it DID reach done, so it is not a stuck loop.) const markFailedStatement = db.prepare(` - UPDATE miner_portfolio_queue SET status = 'queued', leased_at = NULL + UPDATE miner_portfolio_queue + SET status = 'queued', leased_at = NULL, reenqueues = reenqueues + 1, consecutive_failures = consecutive_failures + 1 WHERE api_base_url = ? AND repo_full_name = ? AND identifier = ? AND status = 'in_progress' RETURNING * `); @@ -203,8 +228,12 @@ export function initPortfolioQueueStore(dbPath = resolvePortfolioQueueDbPath()) const listInProgressStatement = db.prepare( `SELECT * FROM miner_portfolio_queue WHERE status = 'in_progress' ${ORDER}`, ); + // The stuck-lease reclaim sweep is the other in_progress -> queued-without-done cycle (#5654): a crashed/killed + // run's lease is swept back to queued, which is the same non-convergence signal as an explicit failure, so it + // bumps the same two counters. const reclaimStatement = db.prepare(` - UPDATE miner_portfolio_queue SET status = 'queued', leased_at = NULL + UPDATE miner_portfolio_queue + SET status = 'queued', leased_at = NULL, reenqueues = reenqueues + 1, consecutive_failures = consecutive_failures + 1 WHERE api_base_url = ? AND repo_full_name = ? AND identifier = ? AND status = 'in_progress' RETURNING * `); @@ -216,11 +245,15 @@ export function initPortfolioQueueStore(dbPath = resolvePortfolioQueueDbPath()) WHERE api_base_url = ? AND repo_full_name = ? AND identifier = ? AND status = 'done' RETURNING * `); + // Batch claim is the same queued -> in_progress attempt as dequeueNext, so it counts an attempt too (#5654). const claimTargetStatement = db.prepare(` - UPDATE miner_portfolio_queue SET status = 'in_progress', leased_at = ? + UPDATE miner_portfolio_queue SET status = 'in_progress', leased_at = ?, attempts = attempts + 1 WHERE api_base_url = ? AND repo_full_name = ? AND identifier = ? AND status = 'queued' RETURNING * `); + const attemptHistoryStatement = db.prepare( + "SELECT attempts, consecutive_failures, reenqueues, status FROM miner_portfolio_queue WHERE api_base_url = ? AND repo_full_name = ? AND identifier = ?", + ); return { dbPath: resolvedPath, @@ -286,6 +319,24 @@ export function initPortfolioQueueStore(dbPath = resolvePortfolioQueueDbPath()) ); return row ? rowToEntry(row) : null; }, + /** Real per-item attempt history for the Governor's non-convergence detector (#5654): a genuine + * `PortfolioConvergenceInput` (packages/gittensory-engine/src/portfolio/non-convergence.ts) instead of the + * first-attempt-shaped literal `buildAttemptGovernorContext` used to hardcode. `reachedDone` is strictly + * `status === 'done'`, never fabricated. An item never enqueued reads all-zero and not-done -- a fresh, + * never-attempted item the detector treats as `converging`, matching the prior literal's fail-open posture. */ + getAttemptHistory(repoFullName, identifier, apiBaseUrl) { + const row = attemptHistoryStatement.get( + normalizeApiBaseUrl(apiBaseUrl), + normalizeRepoFullName(repoFullName), + normalizeIdentifier(identifier), + ); + return { + attempts: row?.attempts ?? 0, + consecutiveFailures: row?.consecutive_failures ?? 0, + reenqueues: row?.reenqueues ?? 0, + reachedDone: row?.status === "done", + }; + }, /** * Transactional caps-aware batch claim hook used by portfolio-queue-manager.js: re-read active rows under an * exclusive lock, let the caller pick targets, then atomically flip each still-queued row to `in_progress`. diff --git a/test/unit/miner-portfolio-queue-attempt-history.test.ts b/test/unit/miner-portfolio-queue-attempt-history.test.ts new file mode 100644 index 0000000000..2d392f169e --- /dev/null +++ b/test/unit/miner-portfolio-queue-attempt-history.test.ts @@ -0,0 +1,177 @@ +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { DatabaseSync } from "node:sqlite"; +import { describe, expect, it } from "vitest"; +import { buildAttemptGovernorContext } from "../../packages/gittensory-miner/lib/attempt-input-builder.js"; +import { initPortfolioQueueStore } from "../../packages/gittensory-miner/lib/portfolio-queue.js"; +import { + DEFAULT_PORTFOLIO_CONVERGENCE_THRESHOLDS, + classifyPortfolioConvergence, +} from "../../packages/gittensory-engine/src/portfolio/non-convergence"; + +// #5654: real per-issue attempt history on the portfolio queue, feeding the Governor's non-convergence detector. +const REPO = "owner/repo"; +const ID = "issue:42"; +const memStore = () => initPortfolioQueueStore(":memory:"); + +/** A pre-#5654 on-disk store: the v3 table shape (post api_base_url rebuild) stamped at schema version 3, so + * re-opening runs exactly the 3->4 attempt-history migration. `withColumns` seeds an already-migrated file to + * exercise the migration's defensive "column already present" branch. */ +function seedV3File(withColumns: boolean, seed?: { attempts: number; consecutive: number; reenqueues: number; status: string }) { + const file = join(mkdtempSync(join(tmpdir(), "pq-attempt-")), "portfolio-queue.sqlite3"); + const raw = new DatabaseSync(file); + raw.exec(`CREATE TABLE miner_portfolio_queue ( + api_base_url TEXT NOT NULL, repo_full_name TEXT NOT NULL, identifier TEXT NOT NULL, + priority REAL NOT NULL DEFAULT 0, + status TEXT NOT NULL DEFAULT 'queued' CHECK (status IN ('queued', 'in_progress', 'done')), + enqueued_at TEXT NOT NULL, leased_at TEXT${ + withColumns + ? ",\n attempts INTEGER NOT NULL DEFAULT 0, consecutive_failures INTEGER NOT NULL DEFAULT 0, reenqueues INTEGER NOT NULL DEFAULT 0" + : "" + }, + PRIMARY KEY (api_base_url, repo_full_name, identifier))`); + raw.exec("PRAGMA user_version = 3"); + const cols = withColumns + ? "(api_base_url,repo_full_name,identifier,priority,status,enqueued_at,attempts,consecutive_failures,reenqueues)" + : "(api_base_url,repo_full_name,identifier,priority,status,enqueued_at)"; + const vals = withColumns ? "?,?,?,0,?,?,?,?,?" : "?,?,?,0,?,?"; + const args = withColumns + ? ["https://api.github.com", REPO, ID, seed!.status, "2026-01-01T00:00:00Z", seed!.attempts, seed!.consecutive, seed!.reenqueues] + : ["https://api.github.com", REPO, ID, seed?.status ?? "queued", "2026-01-01T00:00:00Z"]; + raw.prepare(`INSERT INTO miner_portfolio_queue ${cols} VALUES (${vals})`).run(...args); + raw.close(); + return file; +} + +describe("portfolio-queue attempt-history migration (#5654)", () => { + it("adds the attempt-history columns to a pre-existing store that lacks them; old rows read fresh-zero", () => { + const store = initPortfolioQueueStore(seedV3File(false, { attempts: 0, consecutive: 0, reenqueues: 0, status: "done" })); + expect(store.getAttemptHistory(REPO, ID, "https://api.github.com")).toEqual({ + attempts: 0, + consecutiveFailures: 0, + reenqueues: 0, + reachedDone: true, + }); + store.close(); + }); + + it("is defensive: a store already carrying the columns is not re-altered, and its values survive", () => { + const store = initPortfolioQueueStore(seedV3File(true, { attempts: 5, consecutive: 1, reenqueues: 2, status: "queued" })); + expect(store.getAttemptHistory(REPO, ID, "https://api.github.com")).toEqual({ + attempts: 5, + consecutiveFailures: 1, + reenqueues: 2, + reachedDone: false, + }); + store.close(); + }); +}); + +describe("portfolio-queue attempt-history counters (#5654)", () => { + it("counts attempts on claim, re-enqueues + consecutive failures on failure, and resets consecutive on done", () => { + const store = memStore(); + store.enqueue({ repoFullName: REPO, identifier: ID }); + expect(store.getAttemptHistory(REPO, ID)).toEqual({ attempts: 0, consecutiveFailures: 0, reenqueues: 0, reachedDone: false }); + + store.dequeueNext(); + store.markFailed(REPO, ID); + expect(store.getAttemptHistory(REPO, ID)).toEqual({ attempts: 1, consecutiveFailures: 1, reenqueues: 1, reachedDone: false }); + + store.dequeueNext(); + store.markFailed(REPO, ID); + expect(store.getAttemptHistory(REPO, ID)).toEqual({ attempts: 2, consecutiveFailures: 2, reenqueues: 2, reachedDone: false }); + + store.dequeueNext(); + store.markDone(REPO, ID); + // consecutive failures reset, but the lifetime attempts/reenqueues totals survive the done. + expect(store.getAttemptHistory(REPO, ID)).toEqual({ attempts: 3, consecutiveFailures: 0, reenqueues: 2, reachedDone: true }); + store.close(); + }); + + it("counts a batch claim as an attempt", () => { + const store = memStore(); + store.enqueue({ repoFullName: REPO, identifier: ID }); + store.batchClaim((entries) => entries); + expect(store.getAttemptHistory(REPO, ID).attempts).toBe(1); + store.close(); + }); + + it("treats the stuck-lease reclaim sweep as a re-enqueue + consecutive failure", () => { + const store = memStore(); + store.enqueue({ repoFullName: REPO, identifier: ID }); + store.dequeueNext(); + store.reclaimStuckItem(REPO, ID); + expect(store.getAttemptHistory(REPO, ID)).toEqual({ attempts: 1, consecutiveFailures: 1, reenqueues: 1, reachedDone: false }); + store.close(); + }); + + it("does NOT count requeueItem (a completed done row re-run) as a re-enqueue or failure", () => { + const store = memStore(); + store.enqueue({ repoFullName: REPO, identifier: ID }); + store.dequeueNext(); + store.markDone(REPO, ID); + store.requeueItem(REPO, ID); + expect(store.getAttemptHistory(REPO, ID)).toEqual({ attempts: 1, consecutiveFailures: 0, reenqueues: 0, reachedDone: false }); + store.close(); + }); + + it("reads an unknown (never-enqueued) item as a fresh, never-attempted item", () => { + const store = memStore(); + expect(store.getAttemptHistory("no/thing", "issue:1")).toEqual({ attempts: 0, consecutiveFailures: 0, reenqueues: 0, reachedDone: false }); + store.close(); + }); + + it("only ever reports reachedDone from a real 'done' status, never fabricated", () => { + const store = memStore(); + store.enqueue({ repoFullName: REPO, identifier: ID }); + expect(store.getAttemptHistory(REPO, ID).reachedDone).toBe(false); // queued + store.dequeueNext(); + expect(store.getAttemptHistory(REPO, ID).reachedDone).toBe(false); // in_progress + store.markDone(REPO, ID); + expect(store.getAttemptHistory(REPO, ID).reachedDone).toBe(true); // done + store.close(); + }); +}); + +describe("portfolio-queue attempt-history feeds the non-convergence detector (#5654)", () => { + it("REGRESSION: a repeatedly re-enqueued item that never reaches done now classifies as non_convergent", () => { + const store = memStore(); + store.enqueue({ repoFullName: REPO, identifier: ID }); + for (let i = 0; i < DEFAULT_PORTFOLIO_CONVERGENCE_THRESHOLDS.maxReenqueues; i++) { + store.dequeueNext(); + store.markFailed(REPO, ID); + } + const history = store.getAttemptHistory(REPO, ID); + const verdict = classifyPortfolioConvergence(history, DEFAULT_PORTFOLIO_CONVERGENCE_THRESHOLDS); + expect(verdict.status).toBe("non_convergent"); + store.close(); + }); + + it("a fresh single attempt still classifies as converging (fail-open, not a stuck loop)", () => { + const store = memStore(); + store.enqueue({ repoFullName: REPO, identifier: ID }); + store.dequeueNext(); + expect(classifyPortfolioConvergence(store.getAttemptHistory(REPO, ID)).status).toBe("converging"); + store.close(); + }); +}); + +describe("buildAttemptGovernorContext convergenceInput (#5654)", () => { + const env = {} as Record; + const amsPolicySpec = { capLimits: { budget: 1 } } as never; + + it("forwards a real convergenceInput the caller resolved from the portfolio queue", () => { + const real = { attempts: 5, consecutiveFailures: 2, reenqueues: 4, reachedDone: false }; + expect(buildAttemptGovernorContext(env, amsPolicySpec, false, real).convergenceInput).toEqual(real); + }); + + it("defaults to a fresh, never-attempted item when the caller passes none (fail-open)", () => { + expect(buildAttemptGovernorContext(env, amsPolicySpec, false).convergenceInput).toEqual({ + attempts: 0, + consecutiveFailures: 0, + reenqueues: 0, + reachedDone: false, + }); + }); +});