diff --git a/packages/gittensory-miner/lib/attempt-runner.d.ts b/packages/gittensory-miner/lib/attempt-runner.d.ts index 4a32762421..82e5071b2c 100644 --- a/packages/gittensory-miner/lib/attempt-runner.d.ts +++ b/packages/gittensory-miner/lib/attempt-runner.d.ts @@ -1,6 +1,5 @@ import type { CodingAgentDriver, - GovernorChokepointInput, GovernorDecision, IterateLoopInput, IterateLoopResult, @@ -8,10 +7,16 @@ import type { } from "@jsonbored/gittensory-engine"; import type { HarnessSubmissionDecision, HarnessSubmissionEventLedger } from "./harness-submission-trigger.js"; import type { SubmissionFreshnessClaimLedger, LiveIssueSnapshot, FreshnessAbortReason } from "./submission-freshness-check.js"; +import type { GovernorChokepointInputPersisted } from "./governor-chokepoint-persisted.js"; +import type { GovernorState } from "./governor-state.js"; export const ATTEMPT_OUTCOMES: readonly ["abandon", "stale", "blocked", "governed", "submitted"]; -export type AttemptGovernorContext = Omit; +// rateLimitBuckets/rateLimitBackoffAttempts/capUsage are optional here (via GovernorChokepointInputPersisted, +// not the engine's own GovernorChokepointInput) so a caller can omit them and let evaluateGovernorChokepointGatePersisted +// (#5134) auto-supply real persisted state -- forcing them required at this layer would make every caller +// hand-thread honest-but-stale zero defaults on every invocation, silently defeating that persistence. +export type AttemptGovernorContext = Omit; export type AttemptInput = { loopInput: IterateLoopInput; @@ -36,6 +41,9 @@ export type AttemptDeps = { /** Injected governor-ledger append (mirrors evaluateGovernorChokepointGate's own `options.append`); omitted * falls back to that function's own default (the real default governor ledger). */ governorLedgerAppend?: (event: unknown) => unknown; + /** Injected governor-state store (#5134); omitted falls back to evaluateGovernorChokepointGatePersisted's + * own default (opens + closes the real default governor-state store for this one call). */ + governorState?: GovernorState; sessionStartMs?: number; nowMs: number; executeLocalWrite: (spec: LocalWriteActionSpec) => Promise; diff --git a/packages/gittensory-miner/lib/attempt-runner.js b/packages/gittensory-miner/lib/attempt-runner.js index e349bb66c4..d87ee5045d 100644 --- a/packages/gittensory-miner/lib/attempt-runner.js +++ b/packages/gittensory-miner/lib/attempt-runner.js @@ -1,7 +1,7 @@ import { buildOpenPrSpec } from "@jsonbored/gittensory-engine"; import { runIterateLoop } from "@jsonbored/gittensory-engine"; import { checkSubmissionFreshness } from "./submission-freshness-check.js"; -import { evaluateGovernorChokepointGate } from "./governor-chokepoint.js"; +import { evaluateGovernorChokepointGatePersisted } from "./governor-chokepoint-persisted.js"; import { prepareOpenPrSubmission } from "./harness-submission-trigger.js"; // The real driving-loop entrypoint (#2337): the missing link between #2333's iterate-loop orchestrator and an @@ -18,20 +18,23 @@ import { prepareOpenPrSubmission } from "./harness-submission-trigger.js"; // (worktree-allocator.js, #4297) -- this module composes the create/review/gate/submit sequence #2337 is // actually about, not worktree allocation policy, which is a separate, already-solved concern. // -// KNOWN, DELIBERATE GAPS (not silently papered over -- both were injected-but-unwired seams before this module -// existed, and remain so here): +// KNOWN, DELIBERATE GAP (not silently papered over -- was an injected-but-unwired seam before this module +// existed, and remains so here): // - `deps.runSlopAssessment` has no production implementation anywhere in this package. The real slop scorer // (src/signals/slop.ts, 518 lines, 5 sibling src/signals/** dependencies) is far larger and more // interconnected than local-write-tools.ts was, so extracting it is separate, substantial scope -- this // function requires a real one be injected rather than silently stubbing a result that would either always // pass (unsafe) or always fail (useless). -// - `input.governor`'s cross-attempt state (rate-limit buckets, budget-cap usage, convergence input, -// reputation history, self-plagiarism recent-submissions) has no persistence wiring anywhere in this -// package either -- every existing governor-*.js wrapper is a pure in/out transform over caller-supplied -// state (confirmed by reading governor-write-rate-limit.js: it returns an updated bucket store, but nothing -// persists it). A caller with no prior history should pass honest empty/zero defaults, not fabricated ones; -// durable cross-attempt tracking is portfolio-loop-level scope (the outer "process the queue" loop this -// issue does not build), not a single attempt's. +// +// `input.governor`'s cross-attempt state (rate-limit buckets, backoff attempts, budget-cap usage) DOES now +// persist across separate process invocations (#5134, governor-state.js), via +// evaluateGovernorChokepointGatePersisted -- callers no longer need to hand-thread honest empty/zero defaults +// on every invocation; `capUsage` is loaded from that same store but its post-attempt save stays the caller's +// job (see governor-chokepoint-persisted.js's own header for why: nothing computes "the next capUsage" from a +// verdict, only the attempt's real outcome does). Reputation/self-plagiarism state also has real persistence +// primitives (governor-state.js) but isn't auto-loaded here yet -- `input.governor.reputationHistory`/ +// `selfPlagiarismCandidate`/`selfPlagiarismRecentSubmissions` are still caller-supplied optional fields on +// GovernorChokepointInput, same as before. /** True once the loop reaches handoff AND every downstream gate (freshness, submission, governor) allows. */ export const ATTEMPT_OUTCOMES = Object.freeze(["abandon", "stale", "blocked", "governed", "submitted"]); @@ -88,6 +91,8 @@ function assertInput(input) { * claimLedger: object, * fetchLiveIssueSnapshot: Function, * eventLedger: object, + * governorLedgerAppend?: Function, + * governorState?: import("./governor-state.js").GovernorState, * sessionStartMs?: number, * nowMs: number, * executeLocalWrite: (spec: import("@jsonbored/gittensory-engine").LocalWriteActionSpec) => Promise, @@ -136,7 +141,7 @@ export async function runMinerAttempt(input, deps) { return { outcome: "blocked", decision: submission.decision, loopResult }; } - const governed = evaluateGovernorChokepointGate( + const governed = evaluateGovernorChokepointGatePersisted( { actionClass: "open_pr", repoFullName: input.loopInput.repoFullName, @@ -144,7 +149,10 @@ export async function runMinerAttempt(input, deps) { wouldBeAction: submission.openPrInput, ...input.governor, }, - deps.governorLedgerAppend ? { append: deps.governorLedgerAppend } : {}, + { + ...(deps.governorLedgerAppend ? { append: deps.governorLedgerAppend } : {}), + ...(deps.governorState ? { governorState: deps.governorState } : {}), + }, ); if (!governed.decision.allowed) { return { outcome: "governed", decision: governed.decision, loopResult }; diff --git a/test/unit/miner-attempt-runner.test.ts b/test/unit/miner-attempt-runner.test.ts index 8c4941da2f..3af3379534 100644 --- a/test/unit/miner-attempt-runner.test.ts +++ b/test/unit/miner-attempt-runner.test.ts @@ -10,6 +10,7 @@ vi.mock("@jsonbored/gittensory-engine", async () => { import { runMinerAttempt } from "../../packages/gittensory-miner/lib/attempt-runner.js"; import { initEventLedger } from "../../packages/gittensory-miner/lib/event-ledger.js"; import { initGovernorLedger } from "../../packages/gittensory-miner/lib/governor-ledger.js"; +import { openGovernorState } from "../../packages/gittensory-miner/lib/governor-state.js"; import { parseFocusManifest, type CodingAgentDriver, type CodingAgentDriverResult } from "../../packages/gittensory-engine/src/index"; const roots: string[] = []; @@ -31,6 +32,16 @@ function tempGovernorLedger() { return ledger; } +// Isolated per test: without this, evaluateGovernorChokepointGatePersisted's own default-store fallback +// would open the REAL ~/.config/gittensory-miner/governor-state.sqlite3 on whatever machine runs these tests. +function tempGovernorState() { + const root = mkdtempSync(join(tmpdir(), "gittensory-miner-attempt-runner-governor-state-")); + roots.push(root); + const state = openGovernorState(join(root, "governor-state.sqlite3")); + closers.push(state); + return state; +} + afterEach(() => { for (const closer of closers.splice(0)) closer.close(); for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); @@ -105,6 +116,7 @@ function allowingGovernorContext(overrides: Record = {}) { function baseDeps(overrides: Record = {}) { const eventLedger = tempEventLedger(); const governorLedger = tempGovernorLedger(); + const governorState = tempGovernorState(); return { driver: driverReturning(okDriverResult()), runSlopAssessment: () => noopSlop, @@ -113,6 +125,7 @@ function baseDeps(overrides: Record = {}) { fetchLiveIssueSnapshot: async () => ({ state: "open" as const, referencingPrs: [] }), eventLedger, governorLedgerAppend: (event: unknown) => governorLedger.appendGovernorEvent(event as never), + governorState, nowMs: 10_000, executeLocalWrite: async () => ({ ranAt: 10_000 }), ...overrides, @@ -218,6 +231,46 @@ describe("runMinerAttempt (#2337) — the real create->review->gate->submit pipe vi.unstubAllEnvs(); }); + it("falls back to the real default governor-state store when governorState is omitted", async () => { + const root = mkdtempSync(join(tmpdir(), "gittensory-miner-attempt-runner-default-governor-state-")); + roots.push(root); + vi.stubEnv("GITTENSORY_MINER_GOVERNOR_STATE_DB", join(root, "default-governor-state.sqlite3")); + const deps = baseDeps(); + delete (deps as { governorState?: unknown }).governorState; + + const result = await runMinerAttempt(baseAttemptInput(), deps); + + expect(result.outcome).toBe("submitted"); + vi.unstubAllEnvs(); + }); + + it("REGRESSION (#5134/#5203): a rate limit consumed by one runMinerAttempt call is honored by the next, via the shared governor-state store -- this is what the missing wiring bug looked like", async () => { + const deps = baseDeps(); + const policies = { + global: { open_pr: { limit: 1, windowMs: 60_000 } }, + perRepo: { open_pr: { limit: 5, windowMs: 60_000 } }, + backoffBaseMs: 100, + }; + // rateLimitBuckets/rateLimitBackoffAttempts are DELIBERATELY omitted here (unlike allowingGovernorContext's + // own explicit empty defaults) -- an explicit value on the input always wins over persisted state, so + // omitting them is what actually exercises evaluateGovernorChokepointGatePersisted's auto-load/save path + // through the real runMinerAttempt entrypoint, not just the lower-level wrapper tested elsewhere. + const governorWithoutRateLimitState = allowingGovernorContext({ rateLimitPolicies: policies }); + delete (governorWithoutRateLimitState as { rateLimitBuckets?: unknown }).rateLimitBuckets; + delete (governorWithoutRateLimitState as { rateLimitBackoffAttempts?: unknown }).rateLimitBackoffAttempts; + + const first = await runMinerAttempt(baseAttemptInput({ governor: governorWithoutRateLimitState }), deps); + expect(first.outcome).toBe("submitted"); + + const second = await runMinerAttempt( + baseAttemptInput({ loopInput: passingLoopInput({ attemptId: "attempt-2" }), governor: governorWithoutRateLimitState }), + { ...deps, nowMs: deps.nowMs + 100 }, + ); + expect(second.outcome).toBe("governed"); + if (second.outcome !== "governed") throw new Error("expected governed"); + expect(second.decision.stage).toBe("rate_limit"); + }); + it("fails closed on malformed input", async () => { const deps = baseDeps(); await expect(runMinerAttempt(null as never, deps)).rejects.toThrow("invalid_attempt_input");