Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 10 additions & 2 deletions packages/gittensory-miner/lib/attempt-runner.d.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,22 @@
import type {
CodingAgentDriver,
GovernorChokepointInput,
GovernorDecision,
IterateLoopInput,
IterateLoopResult,
LocalWriteActionSpec,
} 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<GovernorChokepointInput, "actionClass" | "repoFullName" | "nowMs" | "wouldBeAction">;
// 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<GovernorChokepointInputPersisted, "actionClass" | "repoFullName" | "nowMs" | "wouldBeAction">;

export type AttemptInput = {
loopInput: IterateLoopInput;
Expand All @@ -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<unknown>;
Expand Down
32 changes: 20 additions & 12 deletions packages/gittensory-miner/lib/attempt-runner.js
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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"]);
Expand Down Expand Up @@ -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<unknown>,
Expand Down Expand Up @@ -136,15 +141,18 @@ 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,
nowMs: deps.nowMs,
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 };
Expand Down
53 changes: 53 additions & 0 deletions test/unit/miner-attempt-runner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [];
Expand All @@ -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 });
Expand Down Expand Up @@ -105,6 +116,7 @@ function allowingGovernorContext(overrides: Record<string, unknown> = {}) {
function baseDeps(overrides: Record<string, unknown> = {}) {
const eventLedger = tempEventLedger();
const governorLedger = tempGovernorLedger();
const governorState = tempGovernorState();
return {
driver: driverReturning(okDriverResult()),
runSlopAssessment: () => noopSlop,
Expand All @@ -113,6 +125,7 @@ function baseDeps(overrides: Record<string, unknown> = {}) {
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,
Expand Down Expand Up @@ -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");
Expand Down