diff --git a/src/budget/attempt.ts b/src/budget/attempt.ts new file mode 100644 index 0000000..050a988 --- /dev/null +++ b/src/budget/attempt.ts @@ -0,0 +1,83 @@ +import { LABELS } from '@common/constants'; + +/** + * Counting an overspend as an attempt (MAPCO-11435). + * + * The attempt counter has no store but Jira labels: a ticket carries `agent-attempted-N` and + * the poll query excludes the one *at* the cap. That makes bumping it the only thing standing + * between an overspent ticket and being picked up next cycle and burnt for the same budget + * again — one ticket able to eat the whole daily allowance, which is the runaway the ceilings + * exist to bound. + * + * What lives here is the *policy*: given a ticket's labels, which labels count one more attempt. + * What does not live here is the write — that is `JiraPort.setLabels`, and the one place the two + * are put together is `handBackTicket` (src/tickets/handBack.ts), which counts before it releases + * so a ticket cannot become pollable while still uncounted. Keeping the rule pure is what lets + * every implementation of `AbortPort` and `ReleasePort` count the same way. + */ + +/** A counter label's suffix. Anchored and digits-only, so `agent-attempted-soon` is not one. */ +const COUNTER_SUFFIX = /^\d+$/u; + +/** + * The attempt this label records, or null if it is not one of the worker's counters. + * + * Anything under the prefix that is not a plain number is somebody else's label and is left + * alone rather than being read as a count of zero and quietly deleted. + */ +function counterValue(label: string): number | null { + if (!label.startsWith(LABELS.attemptedPrefix)) { + return null; + } + + const suffix = label.slice(LABELS.attemptedPrefix.length); + + return COUNTER_SUFFIX.test(suffix) ? Number(suffix) : null; +} + +/** + * How many attempts a ticket's labels already record. Zero when it carries no counter. + * + * Takes the highest rather than the count of counter labels: a ticket that somehow carries both + * `agent-attempted-1` and `agent-attempted-2` has two attempts behind it, not three, and reading + * it as three is how a ticket gets pushed past the cap. + */ +function attemptsSoFar(labels: readonly string[]): number { + let highest = 0; + + for (const label of labels) { + const counted = counterValue(label); + + if (counted !== null && counted > highest) { + highest = counted; + } + } + + return highest; +} + +/** + * The labels a ticket should carry once this attempt is counted against it. + * + * Replaces the old counter rather than adding to it, so a ticket carries exactly one, and + * **clamps at the cap**, which is the non-obvious half. The poll query excludes the label + * *exactly* at `attemptCap` (`labels not in ("agent-attempted-2")`) — so writing + * `agent-attempted-3` on a ticket already at a cap of 2 would not tighten anything, it would + * make the ticket poll-visible again and hand it straight back to the worker forever. A ticket + * at the cap is already invisible to the query, so leaving its counter where it is loses + * nothing. + * + * Every label that is not one of the worker's counters is preserved untouched, `agent-ready` + * included: enrolment is a human's decision and an overspend is not a reason to revoke it. + */ +function countAttempt(labels: readonly string[], attemptCap: number): readonly string[] { + // Never below 1: a cap of zero is not a thing the query can express, and an + // `agent-attempted-0` label would be a counter nothing ever excludes. + const counted = Math.max(Math.min(attemptsSoFar(labels) + 1, attemptCap), 1); + const next = `${LABELS.attemptedPrefix}${counted}`; + const kept = labels.filter((label) => counterValue(label) === null); + + return [...kept, next]; +} + +export { attemptsSoFar, countAttempt }; diff --git a/src/budget/dailyCap.ts b/src/budget/dailyCap.ts new file mode 100644 index 0000000..b5764ae --- /dev/null +++ b/src/budget/dailyCap.ts @@ -0,0 +1,67 @@ +import type { Clock, DailyCap, DailyCapState } from './types'; + +const MS_PER_DAY = 86_400_000; + +/** The UTC day a moment falls in, as a whole number of days since the epoch. */ +function dayOf(millis: number): number { + return Math.floor(millis / MS_PER_DAY); +} + +/** + * The per-day started-ticket counter (MAPCO-11435). + * + * ## The counter lives in memory, and that is a limitation, not an oversight + * + * Jira is the sole source of truth for this service: there is no database and nothing on disk + * outlives a run (MAPCO-11430). A counter is not a thing Jira can hold — a global daily total + * would mean a read-modify-write across every ticket the worker has ever touched — so it is a + * variable in a long-lived process instead. That is coherent, because the process outlives a + * cycle by design (MAPCO-11430 chose a Deployment over a CronJob for exactly this kind of + * reason), but it means the cap is **per process-day, not per calendar day globally**: a + * redeploy, a crash or an eviction resets it, so a pod that restarts three times in a morning + * can start three times the cap. Read it as a brake on a healthy process, not as a billing + * guarantee. A genuinely global cap needs a store, and inventing one here is out of scope. + * + * Day boundaries are UTC and are evaluated lazily: nothing schedules a reset, the window rolls + * forward the next time the counter is asked anything. An idle worker therefore holds no timer, + * and no count can change underneath a cycle that is already running. + * + * Asking (`state`) and counting (`recordStart`) are separate calls, and what is counted is + * tickets the worker *held*, not tickets it looked at. A claim lost to a human spends nothing, + * and burning a day's allowance on races would idle the worker for the rest of the day. The + * cost is that two tickets can read the same last slot as free when `maxConcurrentTickets` is + * above 1, so a day can end a ticket or two over the cap; the counter reports the real number + * rather than clamping it, and both knobs default to 1. + */ +function createDailyCap(limit: number, clock: Clock = Date.now): DailyCap { + let day = dayOf(clock()); + let started = 0; + + /** Roll the window forward if the clock has crossed midnight since the last question. */ + const rollover = (): void => { + const today = dayOf(clock()); + + if (today !== day) { + day = today; + started = 0; + } + }; + + const state = (): DailyCapState => { + rollover(); + + return { startedToday: started, limit, exhausted: started >= limit }; + }; + + return { + recordStart: (): void => { + // Records something that already happened, so it never refuses. Guarding here would + // hide the overshoot described above instead of reporting it. + rollover(); + started += 1; + }, + state, + }; +} + +export { createDailyCap, dayOf }; diff --git a/src/budget/enforce.ts b/src/budget/enforce.ts new file mode 100644 index 0000000..c13c9e4 --- /dev/null +++ b/src/budget/enforce.ts @@ -0,0 +1,155 @@ +import type { Logger } from '@map-colonies/js-logger'; +import type { JiraTicket } from '../jira/types'; +import type { ClaimOutcome } from '../tickets/claim'; +import { describeOverspend } from './report'; +import type { AbortPort, AttemptSummary, DailyCap, DailyCapState, Overspend, Spend, TicketLedger } from './types'; + +interface BudgetDeps { + readonly ledger: TicketLedger; + readonly abort: AbortPort; + readonly logger: Logger; +} + +interface DailyCapDeps { + readonly cap: DailyCap; + readonly logger: Logger; +} + +/** + * Whether the ticket may take another turn, and — when it may not — what the abort path + * actually managed. + * + * `released: false` means the ticket is still assigned to the bot and still In Progress. That + * is the same containment `releaseTicket` chooses on a stuck workflow: the poll query skips a + * ticket held by the bot, and the boot-time orphan sweep (MAPCO-11432) is what recovers it. A + * caller that sees it must stop working the ticket and must not release it a second time. + * + * `alreadyStopped: true` is the answer to a charge made after the budget had already run out. + * Nothing was written for it — the ticket was handed back by the charge that first refused — + * and there is nothing for the caller to do beyond stop, which it should already have done. + */ +type ChargeOutcome = + | { readonly ok: true; readonly spend: Spend } + | { + readonly ok: false; + readonly alreadyStopped: false; + readonly overspend: Overspend; + readonly released: boolean; + /** Whether the hand-back counted the attempt. See `AbortResult.attemptCounted`. */ + readonly attemptCounted: boolean; + } + | { readonly ok: false; readonly alreadyStopped: true; readonly overspend: Overspend }; + +/** Whether the day had room for this ticket, and what the claim said if it did. */ +type StartAttempt = + | { readonly ok: true; readonly claim: ClaimOutcome; readonly state: DailyCapState } + | { readonly ok: false; readonly reason: 'daily-cap'; readonly state: DailyCapState }; + +/** + * Charge spending to a ticket and, if that used the budget up, abort the ticket (MAPCO-11435). + * + * `spent` carries both halves because the caller is the only thing that knows them. A caller + * handed one usage figure for a whole hand-off charges that hand-off's real turn count, not `1` + * — see `TicketLedger.charge` for why the difference is a factor of `MAX_TURNS_PER_TICKET`. + * + * Going over is an outcome, not an exception: this never rejects, whatever the abort path does. + * A ticket that ran out of money is an ordinary day for the worker, and a throw here would + * surface as `ticket failed` in the run — indistinguishable from a bug, and one line of log + * instead of a comment on the ticket that spent the money. + * + * The hand-back happens at most once per ticket, guaranteed by the ledger's latch rather than + * by asking callers to stop: a second call after the budget has gone charges the money and + * writes nothing. + */ +async function chargeSpend(deps: BudgetDeps, ticket: JiraTicket, spent: Spend, attempt: AttemptSummary): Promise { + const { ledger, abort, logger } = deps; + const check = ledger.charge(spent); + + if (check.ok) { + return { ok: true, spend: check.spend }; + } + + const overspend: Overspend = { kind: check.kind, limit: check.limit, spend: check.spend, attempt }; + + if (check.alreadyStopped) { + // Worth a line, because it means a caller kept spending after being told to stop, but not + // worth a second comment on a ticket somebody else may already have picked up. + logger.warn({ + msg: 'charged spending to a ticket that had already run out of budget', + key: ticket.key, + tokensSpent: overspend.spend.tokens, + turnsSpent: overspend.spend.turns, + }); + + return { ok: false, alreadyStopped: true, overspend }; + } + + logger.warn({ + msg: 'budget exhausted', + key: ticket.key, + kind: overspend.kind, + limit: overspend.limit, + tokensSpent: overspend.spend.tokens, + turnsSpent: overspend.spend.turns, + }); + + try { + const result = await abort.abort(ticket, describeOverspend(overspend)); + + // A hand-back that returned but did not finish the job is the quiet failure mode: an + // unreleased ticket sits held until the orphan sweep, and an *uncounted* one goes straight + // back into the poll query and spends the same budget again next cycle. Neither shows up + // anywhere else, so it goes in the log next to what it cost. + if (!result.released || !result.attemptCounted) { + logger.warn({ + msg: 'overspent ticket was not fully handed back', + key: ticket.key, + released: result.released, + attemptCounted: result.attemptCounted, + }); + } + + return { ok: false, alreadyStopped: false, overspend, released: result.released, attemptCounted: result.attemptCounted }; + } catch (error) { + logger.error({ msg: 'overspent ticket could not be handed back', key: ticket.key, err: error }); + + return { ok: false, alreadyStopped: false, overspend, released: false, attemptCounted: false }; + } +} + +/** + * Start a ticket only if the day still has room for one (MAPCO-11435). + * + * The claim arrives as a callback rather than being done here, for two reasons. A run that has + * hit the cap must write nothing at all: a ticket claimed and then dropped for a spend ceiling + * has already put a bot's name on a human's ticket and notified its watchers, and "poll and + * start nothing" has to mean nothing. And the count must not drift apart from the claim it + * counts, which it would the moment a caller could do one without the other. + * + * The state returned is read *after* the claim, so it already includes the ticket just started + * and can go straight into the run line. + */ +async function startWithinDailyCap(deps: DailyCapDeps, ticket: JiraTicket, claim: () => Promise): Promise { + const { cap, logger } = deps; + const before = cap.state(); + + if (before.exhausted) { + // Not a warning: this is the ceiling doing its job, and the run line says so too. + logger.info({ msg: 'daily cap reached, starting nothing', key: ticket.key, startedToday: before.startedToday, limit: before.limit }); + + return { ok: false, reason: 'daily-cap', state: before }; + } + + const outcome = await claim(); + + // Counted only once the worker actually holds the ticket — a claim lost to a human spends + // nothing, and paying a day's allowance for a race would idle the worker until midnight. + if (outcome.ok) { + cap.recordStart(); + } + + return { ok: true, claim: outcome, state: cap.state() }; +} + +export { chargeSpend, startWithinDailyCap }; +export type { BudgetDeps, ChargeOutcome, DailyCapDeps, StartAttempt }; diff --git a/src/budget/guard.ts b/src/budget/guard.ts new file mode 100644 index 0000000..4e58c50 --- /dev/null +++ b/src/budget/guard.ts @@ -0,0 +1,147 @@ +import type { Logger } from '@map-colonies/js-logger'; +import type { JiraTicket } from '../jira/types'; +import type { ClaimOutcome } from '../tickets/claim'; +import { createDailyCap } from './dailyCap'; +import { chargeSpend, startWithinDailyCap, type ChargeOutcome } from './enforce'; +import { budgetRunLine, type BudgetRunLine } from './report'; +import { createTicketLedger } from './ticketLedger'; +import type { AbortPort, AttemptSummary, BudgetConfig, Clock, DailyCapState, Spend } from './types'; + +interface BudgetGuardDeps { + /** The configured ceilings, read from `WorkerConfig` through `budgetOf`. */ + readonly budget: BudgetConfig; + /** How an overspent ticket is given back. Not implemented here — see `AbortPort`. */ + readonly abort: AbortPort; + readonly logger: Logger; + /** Injected so a test can walk a process across midnight. Defaults to the wall clock. */ + readonly clock?: Clock; +} + +/** + * The meter for the one ticket the worker is holding. + * + * Only obtainable from a `start` that succeeded, which is the point: a ledger cannot be charged + * for a ticket the worker never claimed, and two tickets cannot end up sharing one. + */ +interface TicketGuard { + /** + * Charge spending to this ticket. A `false` outcome means stop working it — the ticket has + * been handed back, or the log says why it could not be. + * + * `spent.turns` is the caller's to get right: it means model turns, so a hand-off that took + * twelve of them charges twelve. See `TicketLedger.charge`. + */ + charge: (spent: Spend, attempt: AttemptSummary) => Promise; + /** What this ticket has cost so far. */ + spend: () => Spend; +} + +/** + * Whether the worker may work this ticket, and why not when it may not. + * + * `daily-cap` is the run that polled and started nothing: no Jira write happened, and the run + * line says so. `not-claimed` is the ordinary race — a human got there first — and costs the day + * nothing. + */ +type GuardedStart = + | { readonly ok: true; readonly ticket: TicketGuard; readonly state: DailyCapState } + | { readonly ok: false; readonly reason: 'daily-cap'; readonly state: DailyCapState } + | { readonly ok: false; readonly reason: 'not-claimed'; readonly claim: Extract; readonly state: DailyCapState }; + +/** + * One cycle's worth of metering. + * + * Its own scope because the two counters in this slice run on different clocks: the daily + * counter belongs to the process and has to survive every cycle, while the spend figure belongs + * to the single "cycle complete" line it is reported on. Holding both on one object was a real + * bug — the spend never reset, so a per-cycle field carried the process's lifetime total and the + * second cycle looked like it had cost the first one's tokens as well. Any sum over those lines + * would have double-counted. + * + * A cycle is the only way to reach `start`, so a caller cannot meter a ticket without having + * opened the cycle the cost is attributed to — the same structural argument as a ledger per + * ticket, one level up. + */ +interface CycleGuard { + /** Claim and start metering a ticket, if the day still has room for one. */ + start: (ticket: JiraTicket, claim: () => Promise) => Promise; + /** The budget half of the one structured "cycle complete" line, for this cycle alone. */ + runLine: () => BudgetRunLine; +} + +interface BudgetGuard { + /** + * Open a cycle. One per `runCycle` — the spend it reports is its own, the daily counter it + * consults is shared with every other cycle this process runs. + */ + cycle: () => CycleGuard; +} + +/** + * Everything the worker needs to hold itself to a spend ceiling, from one config object + * (MAPCO-11435). + * + * This exists so that wiring the ceilings into a cycle is one call rather than five things to + * remember, and so the three invariants that matter are structural rather than documented: a + * fresh ledger per ticket (cost lands on the ticket that caused it), no ledger at all for a + * ticket the worker does not hold, and a fresh spend total per cycle (the run line reports the + * cycle it is written on, not everything since boot). + * + * The lifetimes are nested and each is enforced by what hands out the next: one guard per + * process, because the daily counter is a property of the day and the process is what spans it; + * one cycle per run; one ledger per ticket. + */ +function createBudgetGuard(deps: BudgetGuardDeps): BudgetGuard { + const { budget, abort, logger, clock } = deps; + const cap = createDailyCap(budget.maxTicketsPerDay, clock); + + const openCycle = (): CycleGuard => { + let cycleTokens = 0; + let cycleTurns = 0; + + const meterFor = (ticket: JiraTicket): TicketGuard => { + const ledger = createTicketLedger(budget.ticket); + let counted: Spend = { tokens: 0, turns: 0 }; + + return { + charge: async (spent: Spend, attempt: AttemptSummary): Promise => { + const outcome = await chargeSpend({ ledger, abort, logger }, ticket, spent, attempt); + + // The cycle total is taken from the ledger as a delta rather than by adding the + // argument again, so the run line and the ticket's own comment can never disagree + // about what a turn cost — including the turn that went over the ceiling, which is + // still billed. + const total = ledger.spend(); + cycleTokens += total.tokens - counted.tokens; + cycleTurns += total.turns - counted.turns; + counted = total; + + return outcome; + }, + spend: (): Spend => ledger.spend(), + }; + }; + + return { + start: async (ticket: JiraTicket, claim: () => Promise): Promise => { + const attempt = await startWithinDailyCap({ cap, logger }, ticket, claim); + + if (!attempt.ok) { + return attempt; + } + + if (!attempt.claim.ok) { + return { ok: false, reason: 'not-claimed', claim: attempt.claim, state: attempt.state }; + } + + return { ok: true, ticket: meterFor(ticket), state: attempt.state }; + }, + runLine: (): BudgetRunLine => budgetRunLine({ tokens: cycleTokens, turns: cycleTurns }, cap.state()), + }; + }; + + return { cycle: openCycle }; +} + +export { createBudgetGuard }; +export type { BudgetGuard, BudgetGuardDeps, CycleGuard, GuardedStart, TicketGuard }; diff --git a/src/budget/report.ts b/src/budget/report.ts new file mode 100644 index 0000000..9094f94 --- /dev/null +++ b/src/budget/report.ts @@ -0,0 +1,86 @@ +import type { DailyCapState, Overspend, OverrunKind, Spend } from './types'; + +/** + * Numbers in a Jira comment are formatted with an explicit locale, never the ambient one: a + * pod's locale is not a thing anyone chooses, and the same overspend must not read as + * `212,431` on one worker and `212.431` on the next. + */ +const COUNT = new Intl.NumberFormat('en-GB'); + +/** The unit a limit is expressed in, for the sentence that names it. */ +const UNITS: Record = { tokens: 'tokens', turns: 'turns' }; + +/** + * What the worker says on a ticket it stopped for money (MAPCO-11435). + * + * Written for whoever finds the ticket back in Open: what it cost, which ceiling it hit, what + * it managed first, and which knob to turn. The spend is in the comment because the comment is + * where the cost becomes visible — the ticket that caused the spend is the only place the + * spend is attributable to, since there is no dashboard and no alerting stack (MAPCO-11437). + */ +function describeOverspend(overspend: Overspend): string { + const { kind, limit, spend, attempt } = overspend; + + const lines = [ + 'Stopped this ticket: it ran out of the budget I am allowed to spend on one ticket.', + '', + `Spent ${COUNT.format(spend.turns)} turns and ${COUNT.format(spend.tokens)} tokens. The ceiling I hit was ${COUNT.format(limit)} ${UNITS[kind]} per ticket.`, + '', + ]; + + if (attempt.tried.length > 0) { + lines.push('What I tried, in order:', ...attempt.tried.map((step) => `- ${step}`), ''); + } else { + lines.push('I never got as far as trying anything, which is itself worth a look — the budget went on getting started.', ''); + } + + if (attempt.reached !== null) { + lines.push(`How far it got: ${attempt.reached}.`, ''); + } + + // Says only what this module knows to be true, and the attempt count is deliberately not in + // it — for a reason of ordering rather than of policy. This note is an *argument* to + // `AbortPort.abort`: it is composed before the hand-back is attempted, so at the moment these + // words are written nothing yet knows whether the count, the transition or the unassign + // succeeded. Any sentence here about the attempt would be a prediction, and the prediction + // that matters is the one that goes wrong — a comment claiming the attempt was counted tells + // whoever reads it the ticket is safe from being picked up and re-burnt, exactly when it is + // not (`JiraPort` has no label write; see `countAttempt` and `AbortResult.attemptCounted`). + // What actually happened is reported in the log, where it can be written afterwards. (The + // write exists now — `JiraPort.setLabels`, applied by `handBackTicket` — but the ordering + // argument is unchanged: this note is composed before any of it is attempted.) + lines.push( + `Handing the ticket back. Raise \`${kind === 'turns' ? 'MAX_TURNS_PER_TICKET' : 'MAX_TOKENS_PER_TICKET'}\` if this ticket genuinely needs more room than that; if it should have been plenty, the list above is where the money went.` + ); + + return lines.join('\n'); +} + +/** + * The budget half of the one structured "cycle complete" line (MAPCO-11437 keeps that line the + * only alarm there is, so what a run spent has to be in it). + * + * `tokensSpent` keeps the name the run line already carries as a hard-coded zero, so wiring + * this in is a substitution rather than a rename of a field someone may already be querying. + */ +interface BudgetRunLine { + readonly tokensSpent: number; + readonly turnsSpent: number; + readonly ticketsStartedToday: number; + readonly dailyCapLimit: number; + /** True on a run that polled and deliberately started nothing. */ + readonly dailyCapReached: boolean; +} + +function budgetRunLine(spend: Spend, cap: DailyCapState): BudgetRunLine { + return { + tokensSpent: spend.tokens, + turnsSpent: spend.turns, + ticketsStartedToday: cap.startedToday, + dailyCapLimit: cap.limit, + dailyCapReached: cap.exhausted, + }; +} + +export { budgetRunLine, describeOverspend }; +export type { BudgetRunLine }; diff --git a/src/budget/ticketLedger.ts b/src/budget/ticketLedger.ts new file mode 100644 index 0000000..ab5b452 --- /dev/null +++ b/src/budget/ticketLedger.ts @@ -0,0 +1,70 @@ +import type { BudgetCheck, OverrunKind, Spend, TicketBudget, TicketLedger } from './types'; + +/** + * Meter one ticket against its budget (MAPCO-11435). + * + * The turn is charged *before* the verdict, which looks like an off-by-one and is not: a + * metered API only tells you what a turn cost once it has already cost it, so nothing here + * can refuse a turn in advance. The ceiling therefore means "no further turns once it is + * used up", not "never crossed", and the ticket's comment reports the real total — including + * the turn that went over — rather than a tidier number nobody was billed for. + * + * A ledger is per ticket. Reusing one across tickets would bill the second ticket for the + * first one's work, which is exactly the property this slice exists to get right. + * + * The verdict latches: once a ledger has refused, it goes on refusing with the same reason and + * says the refusal is not new. Only the first refusal can hand a ticket back, so a caller that + * charges one more turn before it reacts cannot produce a second comment and a second release + * on a ticket that is already in Open — an invariant in code rather than a rule in a comment. + */ +function createTicketLedger(budget: TicketBudget): TicketLedger { + let tokens = 0; + let turns = 0; + /** The ceiling this ledger already refused on, if it has refused. Set once, never cleared. */ + let stopped: { readonly kind: OverrunKind; readonly limit: number } | null = null; + + const spend = (): Spend => ({ tokens, turns }); + + return { + charge: (spent: Spend): BudgetCheck => { + // Charged even past the ceiling: the turns cost real money whether or not the ticket was + // supposed to take them, and the totals in the comment and the run line have to be the + // real ones. What does not repeat is the refusal being *new*. + // + // Both halves come from the caller. An earlier draft added `1` to the turn count per call, + // which quietly redefined the ceiling as one-per-charge: a caller reporting a whole + // hand-off's usage in one charge would have had forty hand-offs allowed by a + // `MAX_TURNS_PER_TICKET` of 40, not forty turns. + tokens += spent.tokens; + turns += spent.turns; + + if (stopped !== null) { + return { ok: false, kind: stopped.kind, limit: stopped.limit, spend: spend(), alreadyStopped: true }; + } + + // Both comparisons are `>=`, not `>`: a ticket that has spent exactly its allowance has + // none left, and the next turn would put it over. `readInt` refuses a limit below 1, so a + // ticket always gets to spend something before it can be stopped. + // + // Turns are checked first, which decides the tie when a single expensive turn uses up + // both halves at once. That is deliberate: the turn count is the bound on *looping*, and + // a loop wants a human to look at the agent rather than at the token knob. + if (turns >= budget.maxTurns) { + stopped = { kind: 'turns', limit: budget.maxTurns }; + + return { ok: false, kind: 'turns', limit: budget.maxTurns, spend: spend(), alreadyStopped: false }; + } + + if (tokens >= budget.maxTokens) { + stopped = { kind: 'tokens', limit: budget.maxTokens }; + + return { ok: false, kind: 'tokens', limit: budget.maxTokens, spend: spend(), alreadyStopped: false }; + } + + return { ok: true, spend: spend() }; + }, + spend, + }; +} + +export { createTicketLedger }; diff --git a/src/budget/types.ts b/src/budget/types.ts new file mode 100644 index 0000000..d3b553f --- /dev/null +++ b/src/budget/types.ts @@ -0,0 +1,245 @@ +/** + * The vocabulary of the worker's own cost ceiling (MAPCO-11435). + * + * A metered API key has no ceiling of its own, so the worker carries one. Two halves, because + * they bound different things: a per-ticket token/turn budget bounds how deep a single ticket + * may go, and a per-day counter bounds how many tickets get started at all. Both are config, + * so the rollout can start slow and ramp as quality is proven. + * + * Going over is an outcome, not an exception in a log. The ticket is aborted, commented with + * what it spent and how far it got, and handed back through `AbortPort` — so the cost lands on + * the ticket that caused it and overspend shows up in Jira rather than only on an invoice. + * Whether the hand-back also managed to count the attempt is reported, not assumed: see + * `AbortResult`. + */ + +import type { JiraTicket } from '../jira/types'; + +/** + * An amount of spending: either a running total for a ticket, or one increment charged to it. + * + * Both halves are counted in the units they are billed in. `turns` means *model* turns — one + * request and its response — and never a coarser unit like "one hand-off to the agent". A + * caller with a hand-off that took twelve turns charges twelve; charging one would make a + * `MAX_TURNS_PER_TICKET` of 40 mean forty hand-offs of up to forty turns each, which is a + * ceiling forty times higher than the one an operator set. See `TicketLedger.charge`. + */ +interface Spend { + /** Tokens billed to this ticket, prompt and completion together — the invoice's unit. */ + readonly tokens: number; + /** Model turns spent on this ticket. One turn is one request and its response. */ + readonly turns: number; +} + +/** + * The ceiling for one ticket. + * + * Both halves are needed, and neither implies the other: a stuck agent can burn forty turns + * on trivially cheap calls (a tool that keeps erroring), and a single turn on a huge context + * can cost more than forty small ones. + */ +interface TicketBudget { + readonly maxTokens: number; + readonly maxTurns: number; +} + +/** Every spend ceiling the worker has, as configured. Reached as `WorkerConfig.budget`. */ +interface BudgetConfig { + /** The ceiling applied to each ticket the worker works. */ + readonly ticket: TicketBudget; + /** + * How many tickets the worker may start in a day. Bounds volume where `ticket` bounds + * depth. Counted in memory, which makes it per process-day — see `createDailyCap`. + */ + readonly maxTicketsPerDay: number; +} + +/** Which half of the per-ticket budget ran out. Reported so a reader knows which knob to turn. */ +type OverrunKind = 'tokens' | 'turns'; + +interface BudgetExhausted { + readonly ok: false; + readonly kind: OverrunKind; + /** The limit that was reached, in the unit named by `kind`. */ + readonly limit: number; + /** What the ticket had cost by the time it stopped, including the turn that went over. */ + readonly spend: Spend; + /** + * False exactly once per ledger: on the charge that first ran the budget out. True on every + * charge after it. + * + * The distinction is what stops a ticket being handed back twice. A caller that keeps + * charging — because a turn was already in flight when the last one refused, or because it + * read a failed hand-back as retryable — would otherwise trip the whole abort path again and + * leave a second "budget exhausted" comment on a ticket already back in Open, possibly one a + * human has since picked up. The ledger latches so that cannot happen, rather than a comment + * asking callers not to do it. + */ + readonly alreadyStopped: boolean; +} + +/** Whether a ticket may keep going, and what it has cost either way. */ +type BudgetCheck = { readonly ok: true; readonly spend: Spend } | BudgetExhausted; + +/** + * What the ticket got for the money. + * + * Supplied by whatever was doing the work; the budget module never invents it, because only + * the caller knows how far it got. It exists so the comment on an aborted ticket says + * something a human can act on instead of only a number. + */ +interface AttemptSummary { + /** What was tried, in order, one short line each. Empty means it never got going. */ + readonly tried: readonly string[]; + /** How far it got, in a phrase — `branch pushed, no pull request`. Null when nowhere. */ + readonly reached: string | null; +} + +/** The whole story of a ticket that ran out of budget: what broke, what it cost, what it managed. */ +interface Overspend { + readonly kind: OverrunKind; + readonly limit: number; + readonly spend: Spend; + readonly attempt: AttemptSummary; +} + +/** + * The meter for one ticket. Not reusable: a ledger belongs to the ticket it was created for, + * and a fresh one per ticket is what makes "cost lands on the ticket that caused it" true. + */ +interface TicketLedger { + /** + * Charge some spending to the ticket and say whether it may spend any more. + * + * Takes both halves rather than counting a turn per call, and that is the whole reason this + * takes a `Spend` instead of a token count. A caller reporting usage once per hand-off — which + * is what `AgentPort.run` gives you today — would then have had its *hand-offs* counted against + * a ceiling named in turns, and a `MAX_TURNS_PER_TICKET` of 40 would have permitted forty + * hand-offs of forty turns apiece. Making the turn count an argument means a caller that does + * not know it has to decide what to say rather than have the ledger quietly say `1`. + * + * Keeps charging after the budget is gone — the money was spent either way, and the totals + * have to be the real ones — but reports every charge past the ceiling as `alreadyStopped`, + * so only the first one can trigger a hand-back. + */ + charge: (spent: Spend) => BudgetCheck; + /** What the ticket has cost so far. Keeps answering after the budget is gone. */ + spend: () => Spend; +} + +/** The daily counter as it stands, shaped for the run line. */ +interface DailyCapState { + /** Tickets started in the current window. */ + readonly startedToday: number; + readonly limit: number; + /** Whether the cap is reached, so a run can say why it started nothing. */ + readonly exhausted: boolean; +} + +/** + * The per-day started-ticket counter. + * + * Asking and counting are separate calls on purpose — see `createDailyCap` for why a ticket + * is counted only once the worker actually holds it. + */ +interface DailyCap { + /** Count a ticket the worker actually got hold of. */ + recordStart: () => void; + /** + * The counter as it stands, which is also how a caller asks whether it may start: a run + * reads `exhausted` and reports the numbers next to it, and one call answers both. Rolls + * the window forward if the day has changed since the last question. + */ + state: () => DailyCapState; +} + +/** + * Where the budget module gets the time from. + * + * Injected rather than reading the clock inline so a test can walk a process across a day + * boundary without waiting for one, and so no module-load-time `Date.now()` decides what + * "today" means for the lifetime of the process. + */ +type Clock = () => number; + +/** + * What the hand-back actually managed. + * + * Two separate booleans because they fail separately and one of them cannot currently succeed + * at all. Reported rather than assumed: the alternative is a comment on the ticket claiming + * something happened that did not, which is worse than no claim. + */ +interface AbortResult { + /** + * True when the ticket is back in Open and unassigned, so another run or a human can pick it + * up. False means the worker still holds it, In Progress — the containment `releaseTicket` + * chooses on a stuck workflow, recovered by the boot-time orphan sweep (MAPCO-11432). + */ + readonly released: boolean; + /** + * True when the attempt was counted, i.e. the ticket's `agent-attempted-N` label was bumped. + * + * Which labels that means is not left to the implementer to work out: `countAttempt` in + * `budget/attempt.ts` computes the exact label set, so the policy lives in this slice and + * every implementation of the port counts the same way. The write itself is + * `JiraPort.setLabels`, and `handBackTicket` (src/tickets/handBack.ts) is the binding that puts + * the two together — bind to that rather than writing a second one. + * + * `false` is still reachable, and deliberately so: the label write can fail. When it does the + * hand-back stops rather than releasing an uncounted ticket, so `released` comes back `false` + * with it. + * + * `false` matters because it is the runaway this slice exists to bound. An uncounted overspend + * still matches the poll query — `buildPollQuery` filters on `agent-ready`, an empty assignee + * and the label *at* `ATTEMPT_CAP` — so the next cycle picks the same ticket up and spends the + * same budget on it again, one ticket able to eat the whole daily allowance. `chargeSpend` + * logs that loudly rather than letting it pass, and the comment on the ticket never claims a + * count that did not happen. + */ + readonly attemptCounted: boolean; +} + +/** + * How the budget module gives an overspent ticket back. + * + * Deliberately a ticket and a note rather than a Jira client: the release path itself — + * comment, transition to Open, unassign last, bump the attempt label — is MAPCO-11431 and + * MAPCO-11432's to implement, and this slice must not grow a second implementation of it. + * The budget module's job ends at deciding to stop, saying what it cost, and reporting how + * much of the hand-back succeeded. + */ +interface AbortPort { + /** + * Comment `note` on the ticket, hand the ticket back, and count the attempt against it. + * + * An overspend should count as an attempt — unlike the hand-straight-back note in `runCycle`, + * which explicitly does not, because that one does no work and spends nothing. The labels to + * write are `countAttempt(ticket.labels)`; use it rather than deriving them, so an overspend + * counts the same way an implementation refusal does. + * + * The hand-back's own order is not this module's to choose either: `releaseTicket` comments, + * transitions to Open and unassigns *last*, because unassigning is what puts the ticket back + * in front of the poll query. An implementation must go through it rather than reproduce it. + * + * An implementation that cannot count the attempt must say so in its `AbortResult` rather than + * quietly skipping it; throwing is for a hand-back that achieved nothing at all. + */ + abort: (ticket: JiraTicket, note: string) => Promise; +} + +export type { + AbortPort, + AbortResult, + AttemptSummary, + BudgetCheck, + BudgetConfig, + BudgetExhausted, + Clock, + DailyCap, + DailyCapState, + Overspend, + OverrunKind, + Spend, + TicketBudget, + TicketLedger, +}; diff --git a/src/common/workerConfig.ts b/src/common/workerConfig.ts index 154d68c..b184707 100644 --- a/src/common/workerConfig.ts +++ b/src/common/workerConfig.ts @@ -8,6 +8,7 @@ * these knobs is follow-up work, not a blocker for the current slice. */ +import type { BudgetConfig } from '../budget/types'; import type { BotIdentity } from '../tickets/claim'; interface WorkerConfig { @@ -21,10 +22,43 @@ interface WorkerConfig { readonly mcpUrl: string; /** Who the worker claims tickets as. Both halves are required — see `BotIdentity`. */ readonly bot: BotIdentity; + /** + * The worker's own cost ceilings (MAPCO-11435): how much one ticket may spend, and how many + * tickets a day may start. Config rather than constants so the rollout can start slow and + * ramp as quality is proven. + * + * Optional on the *type* only, and never absent in practice: `loadWorkerConfig` always fills + * it in, so every deployed and dry-run worker has all three ceilings. It is optional because + * a required field would have broken every hand-written `WorkerConfig` literal in the suite + * the moment this slice landed, and a spend ceiling is not worth a fleet of unrelated + * compile errors. Read it through `budgetOf`, never directly: absence there means the + * conservative defaults below, and never "no ceiling". + */ + readonly budget?: BudgetConfig; } const DEFAULT_POLL_INTERVAL_MS = 300_000; +/** + * Conservative on purpose. A metered API key has no ceiling of its own, so these three are the + * only thing standing between a stuck agent and an invoice — a default that costs a few dollars + * to discover is the right default, and raising it is a deployment decision someone makes on + * purpose rather than one they inherit. + * + * There is deliberately no value meaning "unlimited": `readInt` refuses anything below 1, so a + * ceiling cannot be switched off with an env var. One that can is one that gets switched off + * during an incident and stays off. + */ +const DEFAULT_MAX_TOKENS_PER_TICKET = 200_000; +const DEFAULT_MAX_TURNS_PER_TICKET = 40; +const DEFAULT_MAX_TICKETS_PER_DAY = 5; + +/** The ceilings a worker gets when nothing configured any: the conservative ones, never none. */ +const DEFAULT_BUDGET: BudgetConfig = { + ticket: { maxTokens: DEFAULT_MAX_TOKENS_PER_TICKET, maxTurns: DEFAULT_MAX_TURNS_PER_TICKET }, + maxTicketsPerDay: DEFAULT_MAX_TICKETS_PER_DAY, +}; + class ConfigError extends Error { public constructor(message: string) { super(message); @@ -55,6 +89,18 @@ function readRequired(env: NodeJS.ProcessEnv, name: string, why: string): string return raw; } +/** + * The spend ceilings this config carries, or the conservative defaults if it carries none. + * + * The one way to read `WorkerConfig.budget`. An absent field is a config object written before + * the ceilings existed — a test literal, or an older caller — and the safe reading of that is + * "the defaults", not "unlimited". Going the other way would make a forgotten field the most + * expensive kind of typo there is. + */ +function budgetOf(config: WorkerConfig): BudgetConfig { + return config.budget ?? DEFAULT_BUDGET; +} + function loadWorkerConfig(env: NodeJS.ProcessEnv = process.env): WorkerConfig { const mcpUrl = readRequired(env, 'MCP_ATLASSIAN_URL', 'the worker has no Jira credentials of its own'); const bot: BotIdentity = { @@ -62,14 +108,26 @@ function loadWorkerConfig(env: NodeJS.ProcessEnv = process.env): WorkerConfig { displayName: readRequired(env, 'JIRA_BOT_DISPLAY_NAME', "the worker could not tell its own claims from a human's"), }; + // Both halves of the per-ticket ceiling are read, never derived from each other: a stuck + // agent can burn its turns on cheap calls, and one huge context can burn its tokens in a + // handful of turns. Whichever runs out first stops the ticket. + const budget: BudgetConfig = { + ticket: { + maxTokens: readInt(env, 'MAX_TOKENS_PER_TICKET', DEFAULT_BUDGET.ticket.maxTokens), + maxTurns: readInt(env, 'MAX_TURNS_PER_TICKET', DEFAULT_BUDGET.ticket.maxTurns), + }, + maxTicketsPerDay: readInt(env, 'MAX_TICKETS_PER_DAY', DEFAULT_BUDGET.maxTicketsPerDay), + }; + return { pollIntervalMs: readInt(env, 'POLL_INTERVAL_MS', DEFAULT_POLL_INTERVAL_MS), maxTicketsPerRun: readInt(env, 'MAX_TICKETS_PER_RUN', 1), maxConcurrentTickets: readInt(env, 'MAX_CONCURRENT_TICKETS', 1), mcpUrl, bot, + budget, }; } -export { ConfigError, loadWorkerConfig }; +export { budgetOf, ConfigError, DEFAULT_BUDGET, loadWorkerConfig }; export type { WorkerConfig }; diff --git a/tests/unit/budget/attempt.spec.ts b/tests/unit/budget/attempt.spec.ts new file mode 100644 index 0000000..b21a112 --- /dev/null +++ b/tests/unit/budget/attempt.spec.ts @@ -0,0 +1,69 @@ +import { describe, expect, it } from 'vitest'; +import { attemptsSoFar, countAttempt } from '@src/budget/attempt'; + +/** The cap the poll query is built with. Passed in, so this module never depends on the cycle. */ +const ATTEMPT_CAP = 2; + +describe('attemptsSoFar', () => { + it('should read a freshly enrolled ticket as having no attempts behind it.', () => { + expect(attemptsSoFar(['agent-ready'])).toBe(0); + }); + + it('should read the counter a ticket already carries.', () => { + expect(attemptsSoFar(['agent-ready', 'agent-attempted-1'])).toBe(1); + }); + + it('should take the highest counter rather than how many counters there are.', () => { + // A ticket that somehow carries both has two attempts behind it, not three. Counting the + // labels instead would push a ticket past the cap on the strength of a duplicate. + expect(attemptsSoFar(['agent-attempted-1', 'agent-attempted-2'])).toBe(2); + }); + + it('should ignore a label under the prefix that is not a number.', () => { + // Somebody else's label, not a counter. Reading it as zero would be harmless; reading it as + // a counter and deleting it in `countAttempt` would not be. + expect(attemptsSoFar(['agent-attempted-soon', 'agent-attempted-'])).toBe(0); + }); +}); + +describe('countAttempt', () => { + it('should put the first counter on a ticket that has never been attempted.', () => { + expect(countAttempt(['agent-ready'], ATTEMPT_CAP)).toEqual(['agent-ready', 'agent-attempted-1']); + }); + + it('should replace the old counter rather than leaving both on the ticket.', () => { + // One counter per ticket: the query excludes on an exact label, so a ticket wearing both + // `agent-attempted-1` and `agent-attempted-2` invites somebody to read the wrong one. + expect(countAttempt(['agent-ready', 'agent-attempted-1'], ATTEMPT_CAP)).toEqual(['agent-ready', 'agent-attempted-2']); + }); + + it('should stop counting at the cap instead of pushing a ticket past it.', () => { + // The non-obvious one, and the reason this clamps. `buildPollQuery` excludes the label + // *exactly* at the cap — `labels not in ("agent-attempted-2")` — so writing + // `agent-attempted-3` would not tighten anything, it would make an exhausted ticket visible + // to the poll again and hand it back to the worker forever. A ticket at the cap is already + // invisible, so leaving its counter alone costs nothing. + expect(countAttempt(['agent-ready', 'agent-attempted-2'], ATTEMPT_CAP)).toEqual(['agent-ready', 'agent-attempted-2']); + }); + + it('should leave every label that is not one of the worker’s counters alone.', () => { + // `agent-ready` above all: enrolment is a human's decision, and running out of budget is not + // a reason to take a ticket out of the programme. + expect(countAttempt(['agent-ready', 'sprint-42', 'agent-attempted-soon'], ATTEMPT_CAP)).toEqual([ + 'agent-ready', + 'sprint-42', + 'agent-attempted-soon', + 'agent-attempted-1', + ]); + }); + + it('should count an attempt on a ticket carrying no labels at all.', () => { + expect(countAttempt([], ATTEMPT_CAP)).toEqual(['agent-attempted-1']); + }); + + it('should never write a counter no query could exclude, whatever cap it is given.', () => { + // A cap below 1 is not a thing the query can express, and `agent-attempted-0` would be a + // counter that excludes nothing — a ticket wearing one is a ticket that never stops. + expect(countAttempt(['agent-ready'], 0)).toEqual(['agent-ready', 'agent-attempted-1']); + }); +}); diff --git a/tests/unit/budget/dailyCap.spec.ts b/tests/unit/budget/dailyCap.spec.ts new file mode 100644 index 0000000..6167813 --- /dev/null +++ b/tests/unit/budget/dailyCap.spec.ts @@ -0,0 +1,100 @@ +import { describe, expect, it } from 'vitest'; +import { createDailyCap, dayOf } from '@src/budget/dailyCap'; + +/** + * A fixed moment, mid-afternoon UTC. Every test here drives its own clock: the wall clock is + * never read, so none of this changes behaviour depending on when the suite runs — including + * when it runs a few minutes before midnight. + */ +const NOON = Date.UTC(2026, 7, 20, 12); +const MS_PER_HOUR = 3_600_000; +const MS_PER_DAY = 24 * MS_PER_HOUR; + +describe('createDailyCap', () => { + it('should let tickets start until the day is full.', () => { + const cap = createDailyCap(2, () => NOON); + + expect(cap.state()).toStrictEqual({ startedToday: 0, limit: 2, exhausted: false }); + + cap.recordStart(); + + expect(cap.state()).toStrictEqual({ startedToday: 1, limit: 2, exhausted: false }); + + cap.recordStart(); + + expect(cap.state()).toStrictEqual({ startedToday: 2, limit: 2, exhausted: true }); + }); + + it('should keep the day full for the rest of that day.', () => { + let now = NOON; + const cap = createDailyCap(1, () => now); + + cap.recordStart(); + now = NOON + 11 * MS_PER_HOUR; + + // 23:00 the same day. Anything that resets before midnight is a cap that does not cap. + expect(cap.state().exhausted).toBe(true); + }); + + it('should start a fresh count once the clock crosses into the next UTC day.', () => { + let now = NOON; + const cap = createDailyCap(1, () => now); + + cap.recordStart(); + now = NOON + 13 * MS_PER_HOUR; + + expect(cap.state()).toStrictEqual({ startedToday: 0, limit: 1, exhausted: false }); + }); + + it('should roll a window forward however long the worker sat idle, without a timer to do it.', () => { + let now = NOON; + const cap = createDailyCap(1, () => now); + + cap.recordStart(); + now = NOON + 5 * MS_PER_DAY; + + // Nothing schedules a reset; the window moves on the next question. Five days of silence + // must land on one clean window rather than needing five ticks to catch up. + expect(cap.state()).toStrictEqual({ startedToday: 0, limit: 1, exhausted: false }); + }); + + it('should count a ticket started after midnight against the new day.', () => { + let now = NOON; + const cap = createDailyCap(2, () => now); + + cap.recordStart(); + now = NOON + 13 * MS_PER_HOUR; + cap.recordStart(); + + expect(cap.state().startedToday).toBe(1); + }); + + it('should report the real count when concurrent starts overshoot the cap, rather than clamping it.', () => { + const cap = createDailyCap(1, () => NOON); + + cap.recordStart(); + cap.recordStart(); + + // Two tickets can read the same last slot as free when MAX_CONCURRENT_TICKETS is above 1. + // The counter reports what happened — clamping it would hide the overshoot instead. + expect(cap.state()).toStrictEqual({ startedToday: 2, limit: 1, exhausted: true }); + }); + + it('should take its idea of today from the injected clock and nothing else.', () => { + const cap = createDailyCap(1, () => 0); + + cap.recordStart(); + + // The epoch, decades before the suite runs: a cap that consulted the real clock anywhere + // would see a different day here and reset. + expect(cap.state().exhausted).toBe(true); + }); +}); + +describe('dayOf', () => { + it('should split days at UTC midnight rather than at a local one.', () => { + // The pod's timezone is not something anyone chooses, so the window must not move with it. + expect(dayOf(Date.UTC(2026, 7, 20, 23, 59, 59, 999))).toBe(dayOf(Date.UTC(2026, 7, 20, 0, 0, 0, 0))); + expect(dayOf(Date.UTC(2026, 7, 21, 0, 0, 0, 0))).toBe(dayOf(Date.UTC(2026, 7, 20, 12)) + 1); + }); +}); diff --git a/tests/unit/budget/enforce.spec.ts b/tests/unit/budget/enforce.spec.ts new file mode 100644 index 0000000..3b33e6e --- /dev/null +++ b/tests/unit/budget/enforce.spec.ts @@ -0,0 +1,355 @@ +import { describe, expect, it } from 'vitest'; +import { countAttempt } from '@src/budget/attempt'; +import { createDailyCap } from '@src/budget/dailyCap'; +import { chargeSpend, startWithinDailyCap } from '@src/budget/enforce'; +import { createTicketLedger } from '@src/budget/ticketLedger'; +import { claimTicket, releaseTicket } from '@src/tickets/claim'; +import { FakeJira, ticket } from '@tests/helpers/fakeJira'; +import { fakeLogger } from '@tests/helpers/fakeLogger'; +import type { AbortPort, AbortResult, AttemptSummary } from '@src/budget/types'; +import type { JiraPort, JiraTicket } from '@src/jira/types'; +import type { BotIdentity } from '@src/tickets/claim'; + +const BOT_ACCOUNT = 'developer-agent@mapcolonies.example'; +const BOT_DISPLAY_NAME = 'AGENT DEVELOPER'; + +const bot: BotIdentity = { account: BOT_ACCOUNT, displayName: BOT_DISPLAY_NAME }; +const displayNames = { [BOT_ACCOUNT]: BOT_DISPLAY_NAME }; + +/** A realistic workflow: transitions named as verbs, each reporting the status it lands in. */ +const workflow = { + 'MAPCO-1': [ + { id: '21', name: 'Start Progress', to: 'In Progress' }, + { id: '11', name: 'Reopen', to: 'Open' }, + ], +}; + +const NOON = Date.UTC(2026, 7, 20, 12); +const MS_PER_HOUR = 3_600_000; + +const attempt: AttemptSummary = { tried: ['read the ticket', 'cloned the repo'], reached: 'branch pushed, no pull request' }; + +/** + * The attempt cap the poll query is built with, restated rather than imported from `src/cycle`. + * + * `countAttempt` takes the cap as an argument precisely so the budget module does not depend on + * the cycle seam, and a test that imported the constant would put that dependency back. + */ +const ATTEMPT_CAP = 2; + +/** A ticket the worker is already holding, which is the only state a budget can run out in. */ +function held(): JiraTicket { + return ticket({ assignee: BOT_DISPLAY_NAME, status: 'In Progress' }); +} + +/** + * Records the abort instead of performing it. + * + * Used for the cases about the *contract* — that one exhaustion aborts exactly once, that a + * rejection is contained, that a partial hand-back is reported. `handBackThrough` below drives + * the real release path instead, for the cases about where the ticket actually ends up. + * + * It reports a fully successful hand-back by default. `managed` overrides that, because the + * partial outcomes are the interesting ones: today's `JiraPort` has no label write, so a real + * implementation would report `attemptCounted: false`. + */ +function fakeAbort(failWith?: Error, managed?: AbortResult): AbortPort & { aborted: { key: string; note: string }[] } { + const aborted: { key: string; note: string }[] = []; + + return { + aborted, + abort: async (target: JiraTicket, note: string): Promise => { + aborted.push({ key: target.key, note }); + + return failWith === undefined ? Promise.resolve(managed ?? { released: true, attemptCounted: true }) : Promise.reject(failWith); + }, + }; +} + +describe('chargeSpend', () => { + it('should let a ticket carry on while it still has budget, leaving the abort path alone.', async () => { + const abort = fakeAbort(); + const { logger } = fakeLogger(); + const deps = { ledger: createTicketLedger({ maxTokens: 1000, maxTurns: 10 }), abort, logger }; + + await expect(chargeSpend(deps, held(), { tokens: 100, turns: 1 }, attempt)).resolves.toEqual({ ok: true, spend: { tokens: 100, turns: 1 } }); + expect(abort.aborted).toEqual([]); + }); + + it('should take the release path when the budget runs out mid-ticket, rather than throwing.', async () => { + const abort = fakeAbort(); + const { logger } = fakeLogger(); + const deps = { ledger: createTicketLedger({ maxTokens: 1000, maxTurns: 10 }), abort, logger }; + + await chargeSpend(deps, held(), { tokens: 400, turns: 1 }, attempt); + const outcome = await chargeSpend(deps, held(), { tokens: 900, turns: 1 }, attempt); + + // Going over is an outcome, not an exception: the caller is told to stop and the ticket is + // handed back. A throw here would surface as `ticket failed` in the run, indistinguishable + // from a bug, and would leave the money spent with nothing on the ticket to show for it. + expect(outcome).toEqual({ + ok: false, + alreadyStopped: false, + released: true, + attemptCounted: true, + overspend: { kind: 'tokens', limit: 1000, spend: { tokens: 1300, turns: 2 }, attempt }, + }); + expect(abort.aborted).toHaveLength(1); + expect(abort.aborted[0]?.key).toBe('MAPCO-1'); + }); + + it('should hand the abort path a note carrying the cost and what was tried.', async () => { + const abort = fakeAbort(); + const { logger } = fakeLogger(); + const deps = { ledger: createTicketLedger({ maxTokens: 100, maxTurns: 10 }), abort, logger }; + + await chargeSpend(deps, held(), { tokens: 1500, turns: 1 }, attempt); + + const note = abort.aborted[0]?.note ?? ''; + + expect(note).toContain('1,500 tokens'); + expect(note).toContain('100 tokens per ticket'); + expect(note).toContain('- cloned the repo'); + expect(note).toContain('branch pushed, no pull request'); + }); + + it('should put the per-ticket cost in the log as well as on the ticket.', async () => { + const { logger, lines } = fakeLogger(); + const deps = { ledger: createTicketLedger({ maxTokens: 100, maxTurns: 10 }), abort: fakeAbort(), logger }; + + await chargeSpend(deps, held(), { tokens: 1500, turns: 1 }, attempt); + + expect(lines).toContainEqual({ + level: 'warn', + payload: { msg: 'budget exhausted', key: 'MAPCO-1', kind: 'tokens', limit: 100, tokensSpent: 1500, turnsSpent: 1 }, + }); + }); + + it('should keep holding a ticket whose hand-back failed, and say so instead of crashing.', async () => { + const abort = fakeAbort(new Error('MCP unreachable')); + const { logger, lines } = fakeLogger(); + const deps = { ledger: createTicketLedger({ maxTokens: 100, maxTurns: 10 }), abort, logger }; + + const outcome = await chargeSpend(deps, held(), { tokens: 1500, turns: 1 }, attempt); + + // Held-and-stuck is the recoverable state: the poll query skips a ticket assigned to the + // bot, and the boot-time orphan sweep (MAPCO-11432) is what gets it back. `released: false` + // is how the caller knows not to try to hand it back a second time. + expect(outcome).toMatchObject({ ok: false, released: false, attemptCounted: false }); + expect(lines).toContainEqual({ + level: 'error', + payload: { msg: 'overspent ticket could not be handed back', key: 'MAPCO-1', err: new Error('MCP unreachable') }, + }); + }); + + it('should hand a ticket back once however many times it is charged afterwards.', async () => { + const abort = fakeAbort(); + const { logger } = fakeLogger(); + const deps = { ledger: createTicketLedger({ maxTokens: 100, maxTurns: 10 }), abort, logger }; + const ticketHeld = held(); + + await chargeSpend(deps, ticketHeld, { tokens: 1500, turns: 1 }, attempt); + const second = await chargeSpend(deps, ticketHeld, { tokens: 200, turns: 1 }, attempt); + const third = await chargeSpend(deps, ticketHeld, { tokens: 200, turns: 1 }, attempt); + + // A caller can charge again for honest reasons — a turn was in flight when the last one + // refused, or it read a failed hand-back as retryable. A second comment on a ticket already + // back in Open, and a second unassign of one a human may have picked up in between, is not + // something the caller should have to be careful about. + expect(abort.aborted).toHaveLength(1); + expect(second).toMatchObject({ ok: false, alreadyStopped: true }); + expect(third).toMatchObject({ ok: false, alreadyStopped: true }); + }); + + it('should keep billing the turns charged after the budget went, so the run line stays honest.', async () => { + const { logger, lines } = fakeLogger(); + const deps = { ledger: createTicketLedger({ maxTokens: 100, maxTurns: 10 }), abort: fakeAbort(), logger }; + + await chargeSpend(deps, held(), { tokens: 1500, turns: 1 }, attempt); + const second = await chargeSpend(deps, held(), { tokens: 500, turns: 1 }, attempt); + + expect(second).toMatchObject({ overspend: { spend: { tokens: 2000, turns: 2 } } }); + expect(lines).toContainEqual({ + level: 'warn', + payload: { msg: 'charged spending to a ticket that had already run out of budget', key: 'MAPCO-1', tokensSpent: 2000, turnsSpent: 2 }, + }); + }); + + it('should say loudly when an overspent ticket went back without its attempt being counted.', async () => { + const abort = fakeAbort(undefined, { released: true, attemptCounted: false }); + const { logger, lines } = fakeLogger(); + const deps = { ledger: createTicketLedger({ maxTokens: 100, maxTurns: 10 }), abort, logger }; + + const outcome = await chargeSpend(deps, held(), { tokens: 1500, turns: 1 }, attempt); + + // The failure this reports is the runaway the slice exists to bound: an uncounted overspend + // still matches the poll query, so the same ticket comes back next cycle and burns the same + // budget again. Nothing in the worker can bump the label yet (MAPCO-11432), so the comment + // on the ticket must not claim it did — and the log must not be silent about it either. + expect(outcome).toMatchObject({ ok: false, released: true, attemptCounted: false }); + expect(abort.aborted[0]?.note ?? '').not.toContain('counting the attempt'); + expect(lines).toContainEqual({ + level: 'warn', + payload: { msg: 'overspent ticket was not fully handed back', key: 'MAPCO-1', released: true, attemptCounted: false }, + }); + }); +}); + +describe('startWithinDailyCap', () => { + it('should claim a ticket while the day still has room, and count it as started.', async () => { + const cap = createDailyCap(2, () => NOON); + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow, displayNames }); + const { logger } = fakeLogger(); + + const outcome = await startWithinDailyCap({ cap, logger }, ticket(), async () => claimTicket(ticket(), jira, bot)); + + expect(outcome).toEqual({ ok: true, claim: { ok: true }, state: { startedToday: 1, limit: 2, exhausted: false } }); + expect(jira.writes).toEqual([ + { kind: 'assign', key: 'MAPCO-1', assignee: BOT_ACCOUNT }, + { kind: 'transition', key: 'MAPCO-1', transitionId: '21' }, + ]); + }); + + it('should stop a ticket being claimed at all once the daily cap is hit.', async () => { + const cap = createDailyCap(1, () => NOON); + cap.recordStart(); + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow, displayNames }); + const { logger, lines } = fakeLogger(); + + const outcome = await startWithinDailyCap({ cap, logger }, ticket(), async () => claimTicket(ticket(), jira, bot)); + + expect(outcome).toEqual({ ok: false, reason: 'daily-cap', state: { startedToday: 1, limit: 1, exhausted: true } }); + // Not one write reached Jira: a ticket claimed and then dropped for a spend ceiling has + // already put a bot's name on a human's ticket and notified everyone watching it. + expect(jira.writes).toEqual([]); + expect(lines).toContainEqual({ + level: 'info', + payload: { msg: 'daily cap reached, starting nothing', key: 'MAPCO-1', startedToday: 1, limit: 1 }, + }); + }); + + it('should not spend a day slot on a ticket a human won.', async () => { + const cap = createDailyCap(1, () => NOON); + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow, displayNames, stealOnAssign: 'BROCHSTEIN RAZ' }); + const { logger } = fakeLogger(); + + const outcome = await startWithinDailyCap({ cap, logger }, ticket(), async () => claimTicket(ticket(), jira, bot)); + + // The counter bounds tickets the worker *worked*. A lost race spends nothing, and paying a + // day's allowance for one would idle the worker until midnight over other people's tickets. + expect(outcome).toMatchObject({ ok: true, claim: { ok: false, reason: 'lost-race' } }); + expect(cap.state()).toStrictEqual({ startedToday: 0, limit: 1, exhausted: false }); + }); + + it('should start tickets again once the day has turned over.', async () => { + let now = NOON; + const cap = createDailyCap(1, () => now); + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow, displayNames }); + const { logger } = fakeLogger(); + const claim = async (): ReturnType => claimTicket(ticket(), jira, bot); + + await startWithinDailyCap({ cap, logger }, ticket(), claim); + + await expect(startWithinDailyCap({ cap, logger }, ticket(), claim)).resolves.toMatchObject({ ok: false, reason: 'daily-cap' }); + + now = NOON + 13 * MS_PER_HOUR; + + await expect(startWithinDailyCap({ cap, logger }, ticket(), claim)).resolves.toMatchObject({ ok: true }); + }); +}); + +/** + * An `AbortPort` built out of the pieces that exist today, so the seam tests below prove where an + * overspent ticket really ends up rather than that a recorder was called. + * + * It is a *test* fixture on purpose. A production implementation of this port is MAPCO-11431's: + * this slice must not grow a second copy of the release path, whose ordering — comment, then + * transition to Open, then unassign last — is load-bearing and already correct in one place. + * What these tests pin is that the budget module hands that path the right note at the right + * moment and reads its outcome honestly. + * + * `attemptCounted` is `false` and not a shortcut: `countAttempt` says exactly which labels would + * count the attempt, and `JiraPort` has no label write to put them anywhere. + */ +function handBackThrough(jira: JiraPort): AbortPort & { wouldLabel: (readonly string[])[] } { + const wouldLabel: (readonly string[])[] = []; + + return { + wouldLabel, + abort: async (target: JiraTicket, note: string): Promise => { + const release = await releaseTicket(target, note, jira); + + wouldLabel.push(countAttempt(target.labels, ATTEMPT_CAP)); + + return { released: release.ok, attemptCounted: false }; + }, + }; +} + +describe('chargeSpend against the real release path', () => { + it('should leave an overspent ticket back in Open and unassigned, with the spend on it.', async () => { + const jira = new FakeJira({ tickets: [held()], transitions: workflow, displayNames }); + const abort = handBackThrough(jira); + const { logger } = fakeLogger(); + const deps = { ledger: createTicketLedger({ maxTokens: 100, maxTurns: 10 }), abort, logger }; + + const outcome = await chargeSpend(deps, held(), { tokens: 1500, turns: 3 }, attempt); + + // The ticket's own Expected Result, end to end through `releaseTicket`: the comment goes on + // first, the transition back to Open second, and the unassign *last* — because unassigning + // is what puts the ticket in front of the poll query again, so a part-way failure must leave + // it held rather than unassigned-and-In-Progress. + const first = jira.writes[0]; + + expect(outcome).toMatchObject({ ok: false, released: true }); + expect(jira.writes.map((write) => write.kind)).toEqual(['comment', 'transition', 'assign']); + expect(jira.writes.slice(1)).toEqual([ + { kind: 'transition', key: 'MAPCO-1', transitionId: '11' }, + { kind: 'assign', key: 'MAPCO-1', assignee: null }, + ]); + + // The cost lands on the ticket that caused it: the comment Jira actually received is the one + // carrying the spend, not a note composed and then dropped somewhere in the hand-back. + expect(first?.kind === 'comment' ? first.body : '').toContain('3 turns and 1,500 tokens'); + await expect(jira.getIssue('MAPCO-1')).resolves.toMatchObject({ assignee: null, labels: ['agent-ready'] }); + }); + + it('should keep holding an overspent ticket its workflow cannot get back to Open.', async () => { + const jira = new FakeJira({ + tickets: [held()], + transitions: { 'MAPCO-1': [{ id: '21', name: 'Start Progress', to: 'In Progress' }] }, + displayNames, + }); + const abort = handBackThrough(jira); + const { logger, lines } = fakeLogger(); + const deps = { ledger: createTicketLedger({ maxTokens: 100, maxTurns: 10 }), abort, logger }; + + const outcome = await chargeSpend(deps, held(), { tokens: 1500, turns: 1 }, attempt); + + // `releaseTicket` refuses to unassign a ticket it cannot move to Open: held-and-stuck is + // recoverable by the boot-time orphan sweep (MAPCO-11432), unassigned-and-stuck polls + // straight back in forever. The budget module reports that rather than retrying it. + expect(outcome).toMatchObject({ ok: false, released: false }); + expect(jira.writes.filter((write) => write.kind === 'assign')).toEqual([]); + expect(lines).toContainEqual({ + level: 'warn', + payload: { msg: 'overspent ticket was not fully handed back', key: 'MAPCO-1', released: false, attemptCounted: false }, + }); + }); + + it('should hand the ticket back without counting the attempt, since no label write exists.', async () => { + const jira = new FakeJira({ tickets: [held()], transitions: workflow, displayNames }); + const abort = handBackThrough(jira); + const { logger } = fakeLogger(); + const deps = { ledger: createTicketLedger({ maxTokens: 100, maxTurns: 10 }), abort, logger }; + + await chargeSpend(deps, held(), { tokens: 1500, turns: 1 }, attempt); + + // The runaway this slice exists to bound, pinned as the open gap it is. `countAttempt` knows + // the labels that would take the ticket out of the poll query; `JiraPort` has `assign`, + // `transition` and `addComment` and nothing that can write them, so every write below is a + // non-label one and the ticket goes back to Open still matching `buildPollQuery`. + expect(abort.wouldLabel).toEqual([['agent-ready', 'agent-attempted-1']]); + expect(jira.writes.map((write) => write.kind)).toEqual(['comment', 'transition', 'assign']); + }); +}); diff --git a/tests/unit/budget/guard.spec.ts b/tests/unit/budget/guard.spec.ts new file mode 100644 index 0000000..48d2631 --- /dev/null +++ b/tests/unit/budget/guard.spec.ts @@ -0,0 +1,244 @@ +import { describe, expect, it } from 'vitest'; +import { createBudgetGuard, type GuardedStart } from '@src/budget/guard'; +import { budgetOf, loadWorkerConfig } from '@src/common/workerConfig'; +import { claimTicket } from '@src/tickets/claim'; +import { FakeJira, ticket } from '@tests/helpers/fakeJira'; +import { fakeLogger } from '@tests/helpers/fakeLogger'; +import type { AbortPort, AbortResult, AttemptSummary, BudgetConfig } from '@src/budget/types'; +import type { JiraTicket } from '@src/jira/types'; +import type { BotIdentity } from '@src/tickets/claim'; + +const BOT_ACCOUNT = 'developer-agent@mapcolonies.example'; +const BOT_DISPLAY_NAME = 'AGENT DEVELOPER'; + +const bot: BotIdentity = { account: BOT_ACCOUNT, displayName: BOT_DISPLAY_NAME }; +const displayNames = { [BOT_ACCOUNT]: BOT_DISPLAY_NAME }; + +/** + * A realistic workflow: transitions named as verbs, each reporting the status it lands in. + * + * Two tickets everywhere, both claimable, so that a test asserting the *second* one was never + * touched is proving the cap held rather than that the fake had nothing to hand over. + */ +const workflow = { + 'MAPCO-1': [ + { id: '21', name: 'Start Progress', to: 'In Progress' }, + { id: '11', name: 'Reopen', to: 'Open' }, + ], + 'MAPCO-2': [ + { id: '21', name: 'Start Progress', to: 'In Progress' }, + { id: '11', name: 'Reopen', to: 'Open' }, + ], +}; + +const NOON = Date.UTC(2026, 7, 20, 12); +const MS_PER_HOUR = 3_600_000; + +const attempt: AttemptSummary = { tried: ['read the ticket'], reached: null }; +const budget: BudgetConfig = { ticket: { maxTokens: 1000, maxTurns: 5 }, maxTicketsPerDay: 1 }; + +function fakeAbort(managed?: AbortResult): AbortPort & { aborted: string[] } { + const aborted: string[] = []; + + return { + aborted, + abort: async (target: JiraTicket): Promise => { + aborted.push(target.key); + + return Promise.resolve(managed ?? { released: true, attemptCounted: false }); + }, + }; +} + +/** The guard hands out a meter only on a start that succeeded, so tests have to narrow. */ +function metered(start: GuardedStart): Extract { + if (!start.ok) { + throw new Error(`expected a started ticket, got ${start.reason}`); + } + + return start; +} + +describe('createBudgetGuard', () => { + it('should meter a ticket it started and hand it back when the budget runs out.', async () => { + const abort = fakeAbort(); + const { logger } = fakeLogger(); + const jira = new FakeJira({ tickets: [ticket(), ticket({ key: 'MAPCO-2' })], transitions: workflow, displayNames }); + const guard = createBudgetGuard({ budget, abort, logger, clock: () => NOON }); + const cycle = guard.cycle(); + + const started = metered(await cycle.start(ticket(), async () => claimTicket(ticket(), jira, bot))); + + await expect(started.ticket.charge({ tokens: 400, turns: 1 }, attempt)).resolves.toMatchObject({ ok: true }); + await expect(started.ticket.charge({ tokens: 900, turns: 1 }, attempt)).resolves.toMatchObject({ + ok: false, + alreadyStopped: false, + released: true, + }); + expect(abort.aborted).toEqual(['MAPCO-1']); + }); + + it('should start nothing and write nothing once the day is full, and say so in the run line.', async () => { + const { logger, lines } = fakeLogger(); + const jira = new FakeJira({ tickets: [ticket(), ticket({ key: 'MAPCO-2' })], transitions: workflow, displayNames }); + const guard = createBudgetGuard({ budget, abort: fakeAbort(), logger, clock: () => NOON }); + const cycle = guard.cycle(); + + await cycle.start(ticket(), async () => claimTicket(ticket(), jira, bot)); + const second = await cycle.start(ticket({ key: 'MAPCO-2' }), async () => claimTicket(ticket({ key: 'MAPCO-2' }), jira, bot)); + + // One ticket a day means the second one is never claimed: no assign, no transition, nothing + // on a human's ticket. The run line is the only alarm this service has (MAPCO-11437), so it + // has to carry the reason the run did nothing rather than leaving it to be inferred. + expect(second).toMatchObject({ ok: false, reason: 'daily-cap' }); + expect(jira.writes.filter((write) => write.key === 'MAPCO-2')).toEqual([]); + expect(cycle.runLine()).toMatchObject({ ticketsStartedToday: 1, dailyCapLimit: 1, dailyCapReached: true }); + expect(lines).toContainEqual({ + level: 'info', + payload: { msg: 'daily cap reached, starting nothing', key: 'MAPCO-2', startedToday: 1, limit: 1 }, + }); + }); + + it('should add up what a run spent across every ticket it worked.', async () => { + const { logger } = fakeLogger(); + const jira = new FakeJira({ tickets: [ticket(), ticket({ key: 'MAPCO-2' })], transitions: workflow, displayNames }); + const roomy: BudgetConfig = { ticket: { maxTokens: 10_000, maxTurns: 10 }, maxTicketsPerDay: 5 }; + let now = NOON; + const guard = createBudgetGuard({ budget: roomy, abort: fakeAbort(), logger, clock: () => now }); + const cycle = guard.cycle(); + + const first = metered(await cycle.start(ticket(), async () => claimTicket(ticket(), jira, bot))); + await first.ticket.charge({ tokens: 300, turns: 1 }, attempt); + await first.ticket.charge({ tokens: 200, turns: 1 }, attempt); + + const second = metered(await cycle.start(ticket({ key: 'MAPCO-2' }), async () => claimTicket(ticket({ key: 'MAPCO-2' }), jira, bot))); + await second.ticket.charge({ tokens: 700, turns: 1 }, attempt); + + // Per-ticket cost is what the comment carries; the run total is what the run line carries, + // and nothing but the guard is in a position to add it up. + expect(first.ticket.spend()).toStrictEqual({ tokens: 500, turns: 2 }); + expect(second.ticket.spend()).toStrictEqual({ tokens: 700, turns: 1 }); + expect(cycle.runLine()).toMatchObject({ tokensSpent: 1200, turnsSpent: 3, ticketsStartedToday: 2 }); + + now = NOON + 13 * MS_PER_HOUR; + + // The day rolls over; what the run has spent does not, because it is this run's cost. + expect(cycle.runLine()).toMatchObject({ tokensSpent: 1200, ticketsStartedToday: 0, dailyCapReached: false }); + }); + + it('should report each cycle only what that cycle spent, while the day keeps counting.', async () => { + const { logger } = fakeLogger(); + const jira = new FakeJira({ tickets: [ticket(), ticket({ key: 'MAPCO-2' })], transitions: workflow, displayNames }); + const roomy: BudgetConfig = { ticket: { maxTokens: 10_000, maxTurns: 10 }, maxTicketsPerDay: 5 }; + const guard = createBudgetGuard({ budget: roomy, abort: fakeAbort(), logger, clock: () => NOON }); + + const first = guard.cycle(); + const one = metered(await first.start(ticket(), async () => claimTicket(ticket(), jira, bot))); + await one.ticket.charge({ tokens: 500, turns: 2 }, attempt); + + const second = guard.cycle(); + const two = metered(await second.start(ticket({ key: 'MAPCO-2' }), async () => claimTicket(ticket({ key: 'MAPCO-2' }), jira, bot))); + await two.ticket.charge({ tokens: 300, turns: 1 }, attempt); + + // One accumulator for the whole process used to feed a per-cycle field, so the second cycle + // reported 800 tokens for work that cost 300 and any sum over the lines double-counted. The + // daily counter is the one thing that must carry across cycles, and still does. + expect(first.runLine()).toMatchObject({ tokensSpent: 500, turnsSpent: 2 }); + expect(second.runLine()).toMatchObject({ tokensSpent: 300, turnsSpent: 1, ticketsStartedToday: 2 }); + }); + + it('should give each ticket its own budget rather than one shared between them.', async () => { + const { logger } = fakeLogger(); + const jira = new FakeJira({ tickets: [ticket(), ticket({ key: 'MAPCO-2' })], transitions: workflow, displayNames }); + const shareable: BudgetConfig = { ticket: { maxTokens: 1000, maxTurns: 10 }, maxTicketsPerDay: 5 }; + const guard = createBudgetGuard({ budget: shareable, abort: fakeAbort(), logger, clock: () => NOON }); + const cycle = guard.cycle(); + + const first = metered(await cycle.start(ticket(), async () => claimTicket(ticket(), jira, bot))); + await first.ticket.charge({ tokens: 900, turns: 1 }, attempt); + + const second = metered(await cycle.start(ticket({ key: 'MAPCO-2' }), async () => claimTicket(ticket({ key: 'MAPCO-2' }), jira, bot))); + + // Cost landing on the ticket that caused it is the whole point of the slice. A guard that + // reused one ledger would stop the second ticket on the first one's spending. + await expect(second.ticket.charge({ tokens: 50, turns: 1 }, attempt)).resolves.toEqual({ ok: true, spend: { tokens: 50, turns: 1 } }); + }); + + it('should not spend a day slot, or hand out a meter, for a ticket a human won.', async () => { + const { logger } = fakeLogger(); + const jira = new FakeJira({ + tickets: [ticket(), ticket({ key: 'MAPCO-2' })], + transitions: workflow, + displayNames, + stealOnAssign: 'BROCHSTEIN RAZ', + }); + const guard = createBudgetGuard({ budget, abort: fakeAbort(), logger, clock: () => NOON }); + const cycle = guard.cycle(); + + const start = await cycle.start(ticket(), async () => claimTicket(ticket(), jira, bot)); + + // No claim, no meter: a ledger for a ticket the worker does not hold is an accident waiting + // to charge someone else's ticket. And a lost race costs the day nothing. + expect(start).toMatchObject({ ok: false, reason: 'not-claimed', claim: { reason: 'lost-race' } }); + expect(cycle.runLine()).toMatchObject({ ticketsStartedToday: 0, dailyCapReached: false }); + }); + + it('should hold itself to the ceilings the config was loaded with.', async () => { + const { logger } = fakeLogger(); + const jira = new FakeJira({ tickets: [ticket(), ticket({ key: 'MAPCO-2' })], transitions: workflow, displayNames }); + const config = loadWorkerConfig({ + /* eslint-disable @typescript-eslint/naming-convention -- these are environment variable names */ + MCP_ATLASSIAN_URL: 'http://mcp.invalid', + JIRA_BOT_ACCOUNT: BOT_ACCOUNT, + JIRA_BOT_DISPLAY_NAME: BOT_DISPLAY_NAME, + MAX_TOKENS_PER_TICKET: '100', + MAX_TURNS_PER_TICKET: '1', + MAX_TICKETS_PER_DAY: '1', + /* eslint-enable @typescript-eslint/naming-convention */ + }); + const abort = fakeAbort(); + const guard = createBudgetGuard({ budget: budgetOf(config), abort, logger, clock: () => NOON }); + const cycle = guard.cycle(); + + const started = metered(await cycle.start(ticket(), async () => claimTicket(ticket(), jira, bot))); + + // The ticket's own Expected Result, driven from env vars end to end: set the per-ticket + // budget very low and the ticket comes back with a comment saying what it spent, and the + // day is full after one ticket. + await expect(started.ticket.charge({ tokens: 50, turns: 1 }, attempt)).resolves.toMatchObject({ ok: false, released: true }); + expect(abort.aborted).toEqual(['MAPCO-1']); + await expect(cycle.start(ticket({ key: 'MAPCO-2' }), async () => claimTicket(ticket({ key: 'MAPCO-2' }), jira, bot))).resolves.toMatchObject({ + ok: false, + reason: 'daily-cap', + }); + }); + + it('should hold a ticket to the configured turn ceiling however few charges it arrives in.', async () => { + const { logger } = fakeLogger(); + const jira = new FakeJira({ tickets: [ticket(), ticket({ key: 'MAPCO-2' })], transitions: workflow, displayNames }); + const config = loadWorkerConfig({ + /* eslint-disable @typescript-eslint/naming-convention -- these are environment variable names */ + MCP_ATLASSIAN_URL: 'http://mcp.invalid', + JIRA_BOT_ACCOUNT: BOT_ACCOUNT, + JIRA_BOT_DISPLAY_NAME: BOT_DISPLAY_NAME, + MAX_TURNS_PER_TICKET: '8', + /* eslint-enable @typescript-eslint/naming-convention */ + }); + const abort = fakeAbort(); + const guard = createBudgetGuard({ budget: budgetOf(config), abort, logger, clock: () => NOON }); + const cycle = guard.cycle(); + + const started = metered(await cycle.start(ticket(), async () => claimTicket(ticket(), jira, bot))); + + // An operator ramping cautiously sets MAX_TURNS_PER_TICKET=8. The spend arrives one hand-off + // at a time, and the ceiling has to bound the *turns* inside those hand-offs rather than the + // number of hand-offs — otherwise 8 would permit eight hand-offs of forty turns each. + await expect(started.ticket.charge({ tokens: 900, turns: 5 }, attempt)).resolves.toMatchObject({ ok: true }); + await expect(started.ticket.charge({ tokens: 900, turns: 4 }, attempt)).resolves.toMatchObject({ + ok: false, + overspend: { kind: 'turns', limit: 8, spend: { tokens: 1800, turns: 9 } }, + }); + expect(abort.aborted).toEqual(['MAPCO-1']); + expect(cycle.runLine()).toMatchObject({ tokensSpent: 1800, turnsSpent: 9 }); + }); +}); diff --git a/tests/unit/budget/report.spec.ts b/tests/unit/budget/report.spec.ts new file mode 100644 index 0000000..4556db9 --- /dev/null +++ b/tests/unit/budget/report.spec.ts @@ -0,0 +1,73 @@ +import { describe, expect, it } from 'vitest'; +import { budgetRunLine, describeOverspend } from '@src/budget/report'; +import type { AttemptSummary } from '@src/budget/types'; + +const attempt: AttemptSummary = { + tried: ['resolved raster-shared from the title', 'cloned it and read the failing test'], + reached: 'branch pushed, no pull request', +}; + +describe('describeOverspend', () => { + it('should say what the ticket cost, which ceiling it hit and which knob to turn.', () => { + const comment = describeOverspend({ kind: 'tokens', limit: 200_000, spend: { tokens: 212_431, turns: 12 }, attempt }); + + // The spend is in the comment because the comment is where cost becomes attributable — + // there is no dashboard, so a ticket that does not say what it cost costs nothing visible. + expect(comment).toContain('12 turns and 212,431 tokens'); + expect(comment).toContain('200,000 tokens per ticket'); + expect(comment).toContain('MAX_TOKENS_PER_TICKET'); + expect(comment).toContain('- resolved raster-shared from the title'); + expect(comment).toContain('branch pushed, no pull request'); + }); + + it('should name the turn knob when it was the turns that ran out.', () => { + const comment = describeOverspend({ kind: 'turns', limit: 40, spend: { tokens: 6100, turns: 40 }, attempt }); + + // Pointing at the token knob here would send a reader to raise a limit that was never the + // one that stopped the ticket. + expect(comment).toContain('40 turns per ticket'); + expect(comment).toContain('MAX_TURNS_PER_TICKET'); + expect(comment).not.toContain('MAX_TOKENS_PER_TICKET'); + }); + + it('should say so plainly when the budget went before the ticket was even started.', () => { + const comment = describeOverspend({ kind: 'turns', limit: 1, spend: { tokens: 900, turns: 1 }, attempt: { tried: [], reached: null } }); + + expect(comment).toContain('never got as far as trying anything'); + expect(comment).not.toContain('How far it got'); + }); + + it('should say it is handing the ticket back without claiming the attempt was counted.', () => { + const comment = describeOverspend({ kind: 'tokens', limit: 200_000, spend: { tokens: 212_431, turns: 12 }, attempt }); + + // Bumping `agent-attempted-N` needs a label write nothing in the worker has yet + // (MAPCO-11432). A comment claiming the attempt was counted would tell whoever reads it + // that the ticket is safe from being picked up and re-burnt, when it is the opposite. + expect(comment).toContain('Handing the ticket back.'); + expect(comment).not.toContain('attempt'); + }); + + it('should group its numbers the same way whatever locale the pod happens to have.', () => { + const comment = describeOverspend({ kind: 'tokens', limit: 1_000_000, spend: { tokens: 1_234_567, turns: 3 }, attempt }); + + // Pinned to one locale on purpose: the same overspend must not read as 1,234,567 on one + // worker and 1.234.567 on the next. + expect(comment).toContain('1,234,567 tokens'); + }); +}); + +describe('budgetRunLine', () => { + it('should carry the run cost under the tokensSpent key the run line already has.', () => { + const line = budgetRunLine({ tokens: 900, turns: 3 }, { startedToday: 2, limit: 5, exhausted: false }); + + expect(line).toStrictEqual({ tokensSpent: 900, turnsSpent: 3, ticketsStartedToday: 2, dailyCapLimit: 5, dailyCapReached: false }); + }); + + it('should let a run that started nothing say why in one field.', () => { + const line = budgetRunLine({ tokens: 0, turns: 0 }, { startedToday: 5, limit: 5, exhausted: true }); + + // The run line is the only alarm this service has (MAPCO-11437), so "polled and started + // nothing on purpose" has to be readable from it rather than inferred from a zero. + expect(line).toMatchObject({ tokensSpent: 0, dailyCapReached: true, ticketsStartedToday: 5 }); + }); +}); diff --git a/tests/unit/budget/ticketLedger.spec.ts b/tests/unit/budget/ticketLedger.spec.ts new file mode 100644 index 0000000..5639c8a --- /dev/null +++ b/tests/unit/budget/ticketLedger.spec.ts @@ -0,0 +1,157 @@ +import { describe, expect, it } from 'vitest'; +import { createTicketLedger } from '@src/budget/ticketLedger'; + +describe('createTicketLedger', () => { + it('should let a ticket carry on while both halves of its budget have room.', () => { + const ledger = createTicketLedger({ maxTokens: 1000, maxTurns: 5 }); + + expect(ledger.charge({ tokens: 100, turns: 1 })).toEqual({ ok: true, spend: { tokens: 100, turns: 1 } }); + expect(ledger.charge({ tokens: 150, turns: 1 })).toEqual({ ok: true, spend: { tokens: 250, turns: 2 } }); + }); + + it('should stop the ticket on the turn that used up its tokens, and report what it really cost.', () => { + const ledger = createTicketLedger({ maxTokens: 1000, maxTurns: 100 }); + + ledger.charge({ tokens: 600, turns: 1 }); + + // The turn is charged before the verdict, so the total is over the limit. That is the + // honest number: a metered API only says what a turn cost once it has already cost it, and + // the comment on the ticket must report what was billed rather than a tidier figure. + expect(ledger.charge({ tokens: 700, turns: 1 })).toEqual({ + ok: false, + kind: 'tokens', + limit: 1000, + spend: { tokens: 1300, turns: 2 }, + alreadyStopped: false, + }); + }); + + it('should treat a budget spent exactly to its limit as spent.', () => { + const ledger = createTicketLedger({ maxTokens: 1000, maxTurns: 100 }); + + // Nothing left to spend means no further turn, even though this one did not go over. + expect(ledger.charge({ tokens: 1000, turns: 1 })).toEqual({ + ok: false, + kind: 'tokens', + limit: 1000, + spend: { tokens: 1000, turns: 1 }, + alreadyStopped: false, + }); + }); + + it('should stop the ticket on its last allowed turn however cheap the turns were.', () => { + const ledger = createTicketLedger({ maxTokens: 1_000_000, maxTurns: 3 }); + + // The failure this half exists for: a loop that burns turns on calls too cheap to ever + // trip the token ceiling. + expect(ledger.charge({ tokens: 1, turns: 1 }).ok).toBe(true); + expect(ledger.charge({ tokens: 1, turns: 1 }).ok).toBe(true); + expect(ledger.charge({ tokens: 1, turns: 1 })).toEqual({ + ok: false, + kind: 'turns', + limit: 3, + spend: { tokens: 3, turns: 3 }, + alreadyStopped: false, + }); + }); + + it('should report a turn overrun when one turn uses up both halves at once.', () => { + const ledger = createTicketLedger({ maxTokens: 10, maxTurns: 1 }); + + // A documented tie-break, not an accident: the turn count is the bound on looping, and a + // loop wants a human looking at the agent rather than at the token knob. + expect(ledger.charge({ tokens: 500, turns: 1 })).toEqual({ + ok: false, + kind: 'turns', + limit: 1, + spend: { tokens: 500, turns: 1 }, + alreadyStopped: false, + }); + }); + + it('should keep answering for the spend after the budget is gone, since the comment needs it.', () => { + const ledger = createTicketLedger({ maxTokens: 10, maxTurns: 10 }); + + ledger.charge({ tokens: 50, turns: 1 }); + ledger.charge({ tokens: 50, turns: 1 }); + + expect(ledger.spend()).toStrictEqual({ tokens: 100, turns: 2 }); + }); + + it('should latch its refusal, so only the first charge past the ceiling is a new one.', () => { + const ledger = createTicketLedger({ maxTokens: 100, maxTurns: 10 }); + + // The first refusal is the one that hands the ticket back. Anything charged after it — a + // turn already in flight, or a caller that read a failed hand-back as retryable — must not + // read as a fresh overrun, or the ticket gets a second comment and a second release. + expect(ledger.charge({ tokens: 150, turns: 1 })).toMatchObject({ ok: false, alreadyStopped: false }); + expect(ledger.charge({ tokens: 150, turns: 1 })).toMatchObject({ ok: false, alreadyStopped: true }); + expect(ledger.charge({ tokens: 1, turns: 1 })).toMatchObject({ ok: false, alreadyStopped: true }); + }); + + it('should keep reporting the ceiling it first stopped on, not whichever one it is furthest past.', () => { + const ledger = createTicketLedger({ maxTokens: 1_000_000, maxTurns: 2 }); + + ledger.charge({ tokens: 1, turns: 1 }); + + // It ran out of turns; charging on will eventually pass the token ceiling too. The reason + // the ticket stopped is still "turns", and the comment already sent said so. + expect(ledger.charge({ tokens: 2_000_000, turns: 1 })).toMatchObject({ kind: 'turns', limit: 2, alreadyStopped: false }); + expect(ledger.charge({ tokens: 2_000_000, turns: 1 })).toMatchObject({ kind: 'turns', limit: 2, alreadyStopped: true }); + }); + + it('should still bill the turns charged after it refused, since they cost real money.', () => { + const ledger = createTicketLedger({ maxTokens: 100, maxTurns: 10 }); + + ledger.charge({ tokens: 150, turns: 1 }); + ledger.charge({ tokens: 400, turns: 1 }); + + // Latching stops a second hand-back, not the accounting. The run line and the ticket must + // report what was actually billed rather than the total as of the refusal. + expect(ledger.spend()).toStrictEqual({ tokens: 550, turns: 2 }); + }); + + it('should count the turns it is told about rather than one per charge.', () => { + const ledger = createTicketLedger({ maxTokens: 1_000_000, maxTurns: 40 }); + + // The defect this shape exists to prevent. The only spend source in the repo reports usage + // once per hand-off, so a caller charges a whole hand-off at a time. If the ledger added a + // turn per call instead of taking the count, a ceiling of 40 turns would have permitted 40 + // hand-offs of up to 40 turns each — roughly 1,600 model turns against a limit named 40. + expect(ledger.charge({ tokens: 5000, turns: 38 })).toMatchObject({ ok: true, spend: { tokens: 5000, turns: 38 } }); + expect(ledger.charge({ tokens: 5000, turns: 4 })).toEqual({ + ok: false, + kind: 'turns', + limit: 40, + spend: { tokens: 10_000, turns: 42 }, + alreadyStopped: false, + }); + }); + + it('should stop a ticket whose very first hand-off overran the whole turn ceiling.', () => { + const ledger = createTicketLedger({ maxTokens: 1_000_000, maxTurns: 8 }); + + // An operator ramping cautiously sets MAX_TURNS_PER_TICKET=8 while the agent's own per-run + // turn bound is still higher. The overshoot is reported rather than clamped: 12 turns were + // billed, and the comment on the ticket has to say 12. + expect(ledger.charge({ tokens: 900, turns: 12 })).toEqual({ + ok: false, + kind: 'turns', + limit: 8, + spend: { tokens: 900, turns: 12 }, + alreadyStopped: false, + }); + }); + + it('should bill a ticket for its own work only.', () => { + const budget = { maxTokens: 1000, maxTurns: 10 }; + const first = createTicketLedger(budget); + const second = createTicketLedger(budget); + + first.charge({ tokens: 900, turns: 1 }); + + // Cost landing on the ticket that caused it is the whole point of the slice; a ledger + // shared between tickets would charge the second one for the first one's overrun. + expect(second.charge({ tokens: 50, turns: 1 })).toEqual({ ok: true, spend: { tokens: 50, turns: 1 } }); + }); +}); diff --git a/tests/unit/workerConfig.spec.ts b/tests/unit/workerConfig.spec.ts index 1be61b6..4d7381c 100644 --- a/tests/unit/workerConfig.spec.ts +++ b/tests/unit/workerConfig.spec.ts @@ -1,6 +1,6 @@ /* eslint-disable @typescript-eslint/naming-convention -- these are environment variable names */ import { describe, expect, it } from 'vitest'; -import { ConfigError, loadWorkerConfig } from '@src/common/workerConfig'; +import { budgetOf, ConfigError, DEFAULT_BUDGET, loadWorkerConfig } from '@src/common/workerConfig'; const minimal = { MCP_ATLASSIAN_URL: 'http://mcp-atlassian:8080/mcp', @@ -41,6 +41,52 @@ describe('loadWorkerConfig', () => { expect(() => loadWorkerConfig(noDisplayName)).toThrow(ConfigError); }); + it('should default the spend ceilings to something a runaway can be discovered on cheaply.', () => { + const config = loadWorkerConfig(minimal); + + expect(config.budget).toStrictEqual({ ticket: { maxTokens: 200_000, maxTurns: 40 }, maxTicketsPerDay: 5 }); + }); + + it('should read the spend ceilings it is given, so a rollout can ramp them.', () => { + const config = loadWorkerConfig({ + ...minimal, + MAX_TOKENS_PER_TICKET: '50000', + MAX_TURNS_PER_TICKET: '8', + MAX_TICKETS_PER_DAY: '2', + }); + + expect(config.budget).toStrictEqual({ ticket: { maxTokens: 50_000, maxTurns: 8 }, maxTicketsPerDay: 2 }); + }); + + it('should refuse a ceiling of zero rather than reading it as unlimited.', () => { + // A metered API key has no ceiling of its own, so these are the only ones there are. One + // that can be switched off with an env var is one that gets switched off during an + // incident and stays off — there is deliberately no way to express "no limit". + expect(() => loadWorkerConfig({ ...minimal, MAX_TOKENS_PER_TICKET: '0' })).toThrow(ConfigError); + + expect(() => loadWorkerConfig({ ...minimal, MAX_TURNS_PER_TICKET: '-1' })).toThrow(ConfigError); + + expect(() => loadWorkerConfig({ ...minimal, MAX_TICKETS_PER_DAY: 'none' })).toThrow(ConfigError); + }); + + it('should always fill the ceilings in, so nothing downstream has to cope with them missing.', () => { + const config = loadWorkerConfig(minimal); + + expect(config.budget).toBeDefined(); + expect(budgetOf(config)).toStrictEqual(config.budget); + }); + + it('should read a config with no ceilings at all as the default ones, never as unlimited.', () => { + const { budget, ...predatingTheCeilings } = loadWorkerConfig(minimal); + + // The field is optional on the type only, so that every hand-written WorkerConfig literal + // in the suite kept compiling when the ceilings landed. The reading of an absent one has to + // be the conservative default: a forgotten field must not be the cheapest way to switch the + // worker's only cost ceiling off. + expect(budgetOf(predatingTheCeilings)).toStrictEqual(DEFAULT_BUDGET); + expect(budgetOf(predatingTheCeilings)).toStrictEqual(budget); + }); + it('should keep the written identifier and the display name it reads back as separate.', () => { const config = loadWorkerConfig(minimal);