From bab8f82c1ba98b48be7526f7c13507c2043b5223 Mon Sep 17 00:00:00 2001 From: razbroc Date: Sun, 23 Aug 2026 11:24:51 +0300 Subject: [PATCH] feat: bound what one ticket and one day may cost MAPCO-11435. A per-ticket token and turn budget bounds one ticket, a per-day counter bounds volume, and both are config so a rollout can start slow. Going over is an outcome rather than an exception: the spend and what was tried go on the ticket, and the ticket is handed back. Spend is scoped per cycle, not per process. One accumulator for the guard's lifetime fed the per-cycle "cycle complete" field, so the second cycle reported the first one's tokens as well and any sum over those lines double-counted. A cycle is now the only way to reach a ticket meter, so the scope cannot be forgotten. The daily counter is the one thing that deliberately spans cycles. The clock is injected, so the day-rollover tests do not depend on the wall clock. The daily counter lives in memory, which makes it a per-process day: it resets on every redeploy, crash and eviction. That is a brake, not a billing guarantee. Not wired into runCycle yet, so nothing enforces these ceilings on the running worker and the run line still reports a hard-coded zero. --- src/budget/attempt.ts | 83 ++++++ src/budget/dailyCap.ts | 67 +++++ src/budget/enforce.ts | 155 +++++++++++ src/budget/guard.ts | 147 ++++++++++ src/budget/report.ts | 86 ++++++ src/budget/ticketLedger.ts | 70 +++++ src/budget/types.ts | 245 +++++++++++++++++ src/common/workerConfig.ts | 60 ++++- tests/unit/budget/attempt.spec.ts | 69 +++++ tests/unit/budget/dailyCap.spec.ts | 100 +++++++ tests/unit/budget/enforce.spec.ts | 355 +++++++++++++++++++++++++ tests/unit/budget/guard.spec.ts | 244 +++++++++++++++++ tests/unit/budget/report.spec.ts | 73 +++++ tests/unit/budget/ticketLedger.spec.ts | 157 +++++++++++ tests/unit/workerConfig.spec.ts | 48 +++- 15 files changed, 1957 insertions(+), 2 deletions(-) create mode 100644 src/budget/attempt.ts create mode 100644 src/budget/dailyCap.ts create mode 100644 src/budget/enforce.ts create mode 100644 src/budget/guard.ts create mode 100644 src/budget/report.ts create mode 100644 src/budget/ticketLedger.ts create mode 100644 src/budget/types.ts create mode 100644 tests/unit/budget/attempt.spec.ts create mode 100644 tests/unit/budget/dailyCap.spec.ts create mode 100644 tests/unit/budget/enforce.spec.ts create mode 100644 tests/unit/budget/guard.spec.ts create mode 100644 tests/unit/budget/report.spec.ts create mode 100644 tests/unit/budget/ticketLedger.spec.ts 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);