From 570b6002b199f52c0f2db1c83d5179b82098653a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:23:04 -0700 Subject: [PATCH] feat(miner): make queue next WIP-cap-aware via --global-wip/--per-repo-wip queue next always called the naive, uncapped dequeueNext() directly, even though a WIP-cap-aware batch claimer (portfolio-queue-manager.js, used by queue claim-batch) already existed. Adds --global-wip/--per-repo-wip flags to queue next, mirroring claim-batch's own flag names. Both flags are omitted (unset/uncapped) by default, so every existing caller's behavior is byte-identical to before -- queue next only becomes cap-aware when an operator explicitly opts in, matching the "off by default" convention used elsewhere in this package (ledger retention, dry-run). When set, a new selectNextEligibleTarget() picks at most one atomically-claimable target via the store's existing batchClaim transaction, refusing to claim once the global or the target row's own per-repo in-progress count has reached its cap -- never claiming and then discarding extra rows, and never reusing the engine's diversifying batch-selection algorithm (that stays claim-batch's job). Scope note: the issue's ".gittensory-miner.yml" cap-source suggestion isn't wired here -- that file is resolved from an ALREADY-CLONED repo on disk (miner-goal-spec.js's resolveMinerGoalSpec), but queue next dequeues BEFORE any worktree is prepared for the claimed item, so no local copy of the target repo's yml exists yet at that point. Reading it would require new live-fetch plumbing this issue doesn't otherwise need. CLI flags (matching claim-batch's existing, established convention) deliver the same acceptance criterion -- "setting a WIP cap ... stops queue next from claiming once the cap is reached" -- without that new dependency. Advances #4850. --- packages/gittensory-miner/lib/cli.js | 3 +- .../lib/portfolio-queue-cli.d.ts | 11 +- .../lib/portfolio-queue-cli.js | 90 +++++++++- test/unit/miner-portfolio-queue-cli.test.ts | 159 ++++++++++++++++++ 4 files changed, 254 insertions(+), 9 deletions(-) diff --git a/packages/gittensory-miner/lib/cli.js b/packages/gittensory-miner/lib/cli.js index e7fc1742cb..a66676638b 100644 --- a/packages/gittensory-miner/lib/cli.js +++ b/packages/gittensory-miner/lib/cli.js @@ -30,7 +30,8 @@ export function printHelp(input) { " gittensory-miner loop --search --miner-login [--max-cycles ] [--cycle-delay-ms ] [--dry-run] [--json]", " Autonomous discover->claim->attempt->reenter loop", " gittensory-miner queue list [--repo ] [--json] List portfolio backlog rows", - " gittensory-miner queue next [--dry-run] [--json] Claim the highest-priority queued item", + " gittensory-miner queue next [--global-wip ] [--per-repo-wip ] [--dry-run] [--json]", + " Claim the highest-priority queued item, optionally WIP-cap-aware", " gittensory-miner queue claim-batch [--global-wip ] [--per-repo-wip ] [--dry-run] [--json]", " gittensory-miner queue done [--dry-run] [--json]", " gittensory-miner queue release [--dry-run] [--json] Return a claimed item to the queue", diff --git a/packages/gittensory-miner/lib/portfolio-queue-cli.d.ts b/packages/gittensory-miner/lib/portfolio-queue-cli.d.ts index 5fd7ae3ebc..798da1ad0e 100644 --- a/packages/gittensory-miner/lib/portfolio-queue-cli.d.ts +++ b/packages/gittensory-miner/lib/portfolio-queue-cli.d.ts @@ -8,7 +8,16 @@ export type ParsedQueueListArgs = } | { error: string }; -export type ParsedQueueNextArgs = { json: boolean; dryRun: boolean } | { error: string }; +export type ParsedQueueNextArgs = + | { json: boolean; dryRun: boolean; globalWipCap: number | undefined; perRepoWipCap: number | undefined } + | { error: string }; + +export type QueueClaimTarget = { repoFullName: string; identifier: string; apiBaseUrl: string }; + +export function selectNextEligibleTarget( + entries: Array<{ repoFullName: string; identifier: string; apiBaseUrl: string; status: string }>, + caps: { globalWipCap: number; perRepoWipCap: number } | null, +): QueueClaimTarget[]; export type ParsedQueueDoneArgs = | { diff --git a/packages/gittensory-miner/lib/portfolio-queue-cli.js b/packages/gittensory-miner/lib/portfolio-queue-cli.js index 3a76a1ee34..7a8003df60 100644 --- a/packages/gittensory-miner/lib/portfolio-queue-cli.js +++ b/packages/gittensory-miner/lib/portfolio-queue-cli.js @@ -4,7 +4,8 @@ import { runPortfolioDashboard } from "./portfolio-dashboard.js"; import { argsWantJson, describeCliError, reportCliFailure } from "./cli-error.js"; const QUEUE_LIST_USAGE = "Usage: gittensory-miner queue list [--repo ] [--json]"; -const QUEUE_NEXT_USAGE = "Usage: gittensory-miner queue next [--dry-run] [--json]"; +const QUEUE_NEXT_USAGE = + "Usage: gittensory-miner queue next [--global-wip ] [--per-repo-wip ] [--dry-run] [--json]"; const QUEUE_DONE_USAGE = "Usage: gittensory-miner queue done [--api-base-url ] [--dry-run] [--json]"; const QUEUE_RELEASE_USAGE = @@ -81,13 +82,70 @@ export function parseQueueListArgs(args) { return options; } +// #4850: --global-wip/--per-repo-wip are OMITTED (undefined) by default -- queue next stays uncapped, byte- +// identical to its pre-#4850 behavior, unless an operator explicitly opts in. Mirrors queue claim-batch's own +// flag names (portfolio-queue-manager.js's WIP-cap-aware claimer), but claim-batch's OWN default of 1/1 is not +// reused here: claim-batch's whole purpose is cap enforcement, while queue next has always been a plain +// highest-priority dequeue and must not silently start capping existing callers that never asked for it. export function parseQueueNextArgs(args) { - const parsed = parseJsonFlag(args); - if ("error" in parsed) return parsed; - if (parsed.positional.length > 0) { + const options = { json: false, dryRun: false, globalWipCap: undefined, perRepoWipCap: undefined }; + const positional = []; + + for (let index = 0; index < args.length; index += 1) { + const token = args[index]; + if (token === "--json") { + options.json = true; + continue; + } + if (token === "--dry-run") { + options.dryRun = true; + continue; + } + if (token === "--global-wip" || token === "--per-repo-wip") { + const value = Number(args[index + 1]); + if (args[index + 1] === undefined || !Number.isFinite(value) || value < 0) { + return { error: QUEUE_NEXT_USAGE }; + } + if (token === "--global-wip") options.globalWipCap = value; + else options.perRepoWipCap = value; + index += 1; + continue; + } + if (token.startsWith("-")) { + return { error: `Unknown option: ${token}` }; + } + positional.push(token); + } + + if (positional.length > 0) { return { error: QUEUE_NEXT_USAGE }; } - return { json: parsed.json, dryRun: parsed.dryRun }; + return options; +} + +/** + * Pick at most one atomically-claimable target from the store's already-priority-ordered active rows (queued + * AND in_progress interleaved, exactly `batchClaim`'s own `entries` shape). `caps` of `null` replicates the + * pre-#4850 behavior: the single highest-priority queued row, unconditionally. When caps are set, refuses to + * select anything once the global or the target row's own per-repo in-progress count has reached its cap -- + * "stops claiming once the cap is reached" (#4850), not a diversifying batch selection (that remains + * claim-batch's job via the engine's own `nextEligibleItems`). + * @param {Array<{ repoFullName: string, identifier: string, apiBaseUrl: string, status: string }>} entries + * @param {{ globalWipCap: number, perRepoWipCap: number } | null} caps + */ +export function selectNextEligibleTarget(entries, caps) { + const topQueued = entries.find((entry) => entry.status === "queued"); + if (!topQueued) return []; + if (!caps) { + return [{ repoFullName: topQueued.repoFullName, identifier: topQueued.identifier, apiBaseUrl: topQueued.apiBaseUrl }]; + } + const globalActiveCount = entries.filter((entry) => entry.status === "in_progress").length; + if (globalActiveCount >= caps.globalWipCap) return []; + const repoActiveCount = entries.filter( + (entry) => entry.status === "in_progress" && entry.repoFullName === topQueued.repoFullName, + ).length; + if (repoActiveCount >= caps.perRepoWipCap) return []; + return [{ repoFullName: topQueued.repoFullName, identifier: topQueued.identifier, apiBaseUrl: topQueued.apiBaseUrl }]; } /** Shared ` [--api-base-url ] [--json]` parse for the item-targeting subcommands @@ -220,10 +278,17 @@ export function runQueueNext(args, options = {}) { return reportCliFailure(argsWantJson(args), parsed.error); } + const capsRequested = parsed.globalWipCap !== undefined || parsed.perRepoWipCap !== undefined; if (parsed.dryRun) { - const dryRunResult = { outcome: "dry_run" }; + const dryRunResult = capsRequested + ? { outcome: "dry_run", globalWipCap: parsed.globalWipCap, perRepoWipCap: parsed.perRepoWipCap } + : { outcome: "dry_run" }; if (parsed.json) { console.log(JSON.stringify(dryRunResult, null, 2)); + } else if (capsRequested) { + console.log( + `DRY RUN: would dequeue the highest-priority queued item within WIP caps (global-wip: ${parsed.globalWipCap ?? "unset"}, per-repo-wip: ${parsed.perRepoWipCap ?? "unset"}). No portfolio-queue write was made.`, + ); } else { console.log("DRY RUN: would dequeue the highest-priority queued item. No portfolio-queue write was made."); } @@ -232,7 +297,18 @@ export function runQueueNext(args, options = {}) { try { return withPortfolioQueue(options, (portfolioQueue) => { - const entry = portfolioQueue.dequeueNext(); + let entry; + if (capsRequested) { + // Unset dimensions stay genuinely uncapped (Infinity), not silently defaulted to 1 like claim-batch. + const caps = { + globalWipCap: parsed.globalWipCap ?? Number.POSITIVE_INFINITY, + perRepoWipCap: parsed.perRepoWipCap ?? Number.POSITIVE_INFINITY, + }; + const claimed = portfolioQueue.batchClaim((entries) => selectNextEligibleTarget(entries, caps)); + entry = claimed[0] ?? null; + } else { + entry = portfolioQueue.dequeueNext(); + } if (parsed.json) { console.log(JSON.stringify({ entry }, null, 2)); } else { diff --git a/test/unit/miner-portfolio-queue-cli.test.ts b/test/unit/miner-portfolio-queue-cli.test.ts index b1b192bc5f..c1c88fec29 100644 --- a/test/unit/miner-portfolio-queue-cli.test.ts +++ b/test/unit/miner-portfolio-queue-cli.test.ts @@ -19,6 +19,7 @@ import { runQueueNext, runQueueRelease, runQueueRequeue, + selectNextEligibleTarget, } from "../../packages/gittensory-miner/lib/portfolio-queue-cli.js"; import type { QueueEntry } from "../../packages/gittensory-miner/lib/portfolio-queue.d.ts"; @@ -131,6 +132,164 @@ describe("gittensory-miner portfolio queue CLI (#2292)", () => { expect(log).toHaveBeenCalledWith("none"); }); + describe("selectNextEligibleTarget() (#4850)", () => { + function entry(overrides: Record = {}) { + return { + apiBaseUrl: "https://api.github.com", + repoFullName: "acme/widgets", + identifier: "issue:1", + status: "queued", + ...overrides, + }; + } + + it("null caps replicates the pre-#4850 unconditional highest-priority selection", () => { + const entries = [entry({ status: "in_progress", identifier: "in-flight" }), entry()]; + expect(selectNextEligibleTarget(entries, null)).toEqual([ + { apiBaseUrl: "https://api.github.com", repoFullName: "acme/widgets", identifier: "issue:1" }, + ]); + }); + + it("returns nothing when there is no queued row, regardless of caps", () => { + expect(selectNextEligibleTarget([], null)).toEqual([]); + expect(selectNextEligibleTarget([entry({ status: "in_progress" })], { globalWipCap: 5, perRepoWipCap: 5 })).toEqual([]); + }); + + it("refuses to select once the global cap is already reached", () => { + const entries = [ + entry({ status: "in_progress", identifier: "a", repoFullName: "acme/a" }), + entry({ status: "in_progress", identifier: "b", repoFullName: "acme/b" }), + entry({ status: "queued", identifier: "c", repoFullName: "acme/c" }), + ]; + expect(selectNextEligibleTarget(entries, { globalWipCap: 2, perRepoWipCap: 5 })).toEqual([]); + expect(selectNextEligibleTarget(entries, { globalWipCap: 3, perRepoWipCap: 5 })).toEqual([ + { apiBaseUrl: "https://api.github.com", repoFullName: "acme/c", identifier: "c" }, + ]); + }); + + it("refuses to select once the top queued row's own repo has reached its per-repo cap", () => { + const entries = [ + entry({ status: "in_progress", identifier: "a1" }), + entry({ status: "queued", identifier: "a2" }), + ]; + expect(selectNextEligibleTarget(entries, { globalWipCap: 5, perRepoWipCap: 1 })).toEqual([]); + // A different repo's own cap isn't saturated, so a queued row there is still eligible. + const otherRepo = [ + entry({ status: "in_progress", identifier: "a1" }), + entry({ status: "queued", identifier: "b1", repoFullName: "acme/other" }), + ]; + expect(selectNextEligibleTarget(otherRepo, { globalWipCap: 5, perRepoWipCap: 1 })).toEqual([ + { apiBaseUrl: "https://api.github.com", repoFullName: "acme/other", identifier: "b1" }, + ]); + }); + }); + + it("runQueueNext with --global-wip/--per-repo-wip claims only within the configured caps (#4850)", () => { + const portfolioQueue = tempQueueStore(); + portfolioQueue.enqueue({ repoFullName: "acme/widgets", identifier: "issue:1", priority: 1 }); + portfolioQueue.dequeueNext(); // one already in-flight, uncapped + + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + portfolioQueue.enqueue({ repoFullName: "acme/widgets", identifier: "issue:2", priority: 1 }); + + // global-wip 1 with one already in_progress -- refuses to claim a second. + expect( + runQueueNext(["--global-wip", "1", "--json"], { initPortfolioQueue: () => portfolioQueue }), + ).toBe(0); + expect(JSON.parse(String(log.mock.calls[0]?.[0]))).toEqual({ entry: null }); + expect(portfolioQueue.listQueue("acme/widgets").find((e) => e.identifier === "issue:2")?.status).toBe("queued"); + + // Raising the cap lets it claim. + log.mockClear(); + expect( + runQueueNext(["--global-wip", "2", "--json"], { initPortfolioQueue: () => portfolioQueue }), + ).toBe(0); + expect(JSON.parse(String(log.mock.calls[0]?.[0]))).toEqual({ + entry: expect.objectContaining({ identifier: "issue:2", status: "in_progress" }), + }); + }); + + it("runQueueNext with only --per-repo-wip set leaves the global dimension genuinely uncapped (#4850)", () => { + const portfolioQueue = tempQueueStore(); + portfolioQueue.enqueue({ repoFullName: "acme/alpha", identifier: "issue:1", priority: 1 }); + portfolioQueue.dequeueNext(); // acme/alpha already at 1 in-flight + portfolioQueue.enqueue({ repoFullName: "acme/beta", identifier: "issue:2", priority: 1 }); + + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + // Global left unset (uncapped): a different repo (acme/beta) is still claimable even though acme/alpha + // already has an in-flight item -- only the per-repo dimension is enforced here. + expect( + runQueueNext(["--per-repo-wip", "1", "--json"], { initPortfolioQueue: () => portfolioQueue }), + ).toBe(0); + expect(JSON.parse(String(log.mock.calls[0]?.[0]))).toEqual({ + entry: expect.objectContaining({ identifier: "issue:2", repoFullName: "acme/beta", status: "in_progress" }), + }); + }); + + it("runQueueNext without --global-wip/--per-repo-wip is unaffected by an already-saturated repo (#4850 backward compat)", () => { + const portfolioQueue = tempQueueStore(); + portfolioQueue.enqueue({ repoFullName: "acme/widgets", identifier: "issue:1", priority: 1 }); + portfolioQueue.dequeueNext(); + portfolioQueue.enqueue({ repoFullName: "acme/widgets", identifier: "issue:2", priority: 1 }); + + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + expect(runQueueNext(["--json"], { initPortfolioQueue: () => portfolioQueue })).toBe(0); + expect(JSON.parse(String(log.mock.calls[0]?.[0]))).toEqual({ + entry: expect.objectContaining({ identifier: "issue:2", status: "in_progress" }), + }); + }); + + it("parseQueueNextArgs accepts --global-wip/--per-repo-wip and rejects a malformed value (#4850)", () => { + expect(parseQueueNextArgs([])).toEqual({ + json: false, + dryRun: false, + globalWipCap: undefined, + perRepoWipCap: undefined, + }); + expect(parseQueueNextArgs(["--global-wip", "3", "--per-repo-wip", "2"])).toEqual({ + json: false, + dryRun: false, + globalWipCap: 3, + perRepoWipCap: 2, + }); + expect(parseQueueNextArgs(["--global-wip", "not-a-number"])).toEqual({ + error: expect.stringContaining("Usage: gittensory-miner queue next"), + }); + expect(parseQueueNextArgs(["--global-wip"])).toEqual({ + error: expect.stringContaining("Usage: gittensory-miner queue next"), + }); + expect(parseQueueNextArgs(["extra-positional"])).toEqual({ + error: expect.stringContaining("Usage: gittensory-miner queue next"), + }); + expect(parseQueueNextArgs(["--bogus"])).toEqual({ error: "Unknown option: --bogus" }); + }); + + it("runQueueNext --dry-run reports the requested caps when set (#4850)", () => { + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + const initPortfolioQueueSpy = vi.fn(); + + expect( + runQueueNext(["--dry-run", "--global-wip", "2", "--json"], { initPortfolioQueue: initPortfolioQueueSpy }), + ).toBe(0); + expect(initPortfolioQueueSpy).not.toHaveBeenCalled(); + expect(JSON.parse(String(log.mock.calls[0]?.[0]))).toEqual({ + outcome: "dry_run", + globalWipCap: 2, + perRepoWipCap: undefined, + }); + + log.mockClear(); + expect(runQueueNext(["--dry-run", "--per-repo-wip", "1"], { initPortfolioQueue: initPortfolioQueueSpy })).toBe(0); + expect(String(log.mock.calls[0]?.[0])).toContain("DRY RUN: would dequeue the highest-priority queued item within WIP caps"); + expect(String(log.mock.calls[0]?.[0])).toContain("global-wip: unset"); + expect(String(log.mock.calls[0]?.[0])).toContain("per-repo-wip: 1"); + + log.mockClear(); + expect(runQueueNext(["--dry-run", "--global-wip", "2"], { initPortfolioQueue: initPortfolioQueueSpy })).toBe(0); + expect(String(log.mock.calls[0]?.[0])).toContain("global-wip: 2"); + expect(String(log.mock.calls[0]?.[0])).toContain("per-repo-wip: unset"); + }); + it("#4847: --dry-run reports what next/done would do and returns 0 without opening the portfolio queue", () => { const log = vi.spyOn(console, "log").mockImplementation(() => undefined); const initPortfolioQueueSpy = vi.fn();