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();