diff --git a/src/jira/mcpJira.ts b/src/jira/mcpJira.ts index 8b187fc..06c14e8 100644 --- a/src/jira/mcpJira.ts +++ b/src/jira/mcpJira.ts @@ -70,6 +70,11 @@ class McpJira implements JiraPort { await this.call('jira_add_comment', { issue_key: issueKey, body }); } + public async setLabels(issueKey: string, labels: readonly string[]): Promise { + // eslint-disable-next-line @typescript-eslint/naming-convention -- MCP wire format + await this.call('jira_update_issue', { issue_key: issueKey, fields: labelsFields(labels) }); + } + public async getTransitions(issueKey: string): Promise { // eslint-disable-next-line @typescript-eslint/naming-convention -- MCP wire format const raw = await this.call('jira_get_transitions', { issue_key: issueKey }); @@ -144,6 +149,18 @@ function assigneeFields(assignee: string | null): string { return JSON.stringify({ assignee }); } +/** + * The `fields` argument for a label write. A JSON *string*, for the same reason as + * `assigneeFields` — an object updates nothing and still reports success. + * + * `jira_update_issue` overwrites the fields it is given, so this sends the complete set. The + * array is copied rather than passed through: `JSON.stringify` on a `readonly string[]` is + * fine, but the copy keeps the wire payload a plain array whatever the caller held. + */ +function labelsFields(labels: readonly string[]): string { + return JSON.stringify({ labels: [...labels] }); +} + /** The server reports a transition's target status as an object, or sometimes not at all. */ function toTransition(transition: McpTransition): JiraTransition { const to = typeof transition.to === 'string' ? transition.to : transition.to?.name; @@ -163,5 +180,5 @@ function toTicket(issue: McpTicket): JiraTicket { }; } -export { assigneeFields, McpJira, toTicket, toTransition }; +export { assigneeFields, labelsFields, McpJira, toTicket, toTransition }; export type { McpTicket, McpTransition }; diff --git a/src/jira/types.ts b/src/jira/types.ts index ec83ad5..ac99e17 100644 --- a/src/jira/types.ts +++ b/src/jira/types.ts @@ -46,4 +46,17 @@ export interface JiraPort { assign: (issueKey: string, assignee: string | null) => Promise; transition: (issueKey: string, transitionId: string) => Promise; addComment: (issueKey: string, body: string) => Promise; + /** + * Replace a ticket's labels with exactly this set. + * + * A whole-set write rather than an add/remove pair because that is what the underlying tool + * offers, and the caller has to have read the existing labels anyway to compute the next + * counter. The consequence is worth stating: anything absent from `labels` is *removed*, so a + * caller must pass the labels it wants kept, `agent-ready` included. `countAttempt` + * (src/budget/attempt.ts) is the function that computes the set; do not derive it by hand. + * + * This is what carries the attempt counter, and the counter is the only thing that stops a + * ticket the worker cannot finish from being picked up and paid for again on every tick. + */ + setLabels: (issueKey: string, labels: readonly string[]) => Promise; } diff --git a/src/tickets/handBack.ts b/src/tickets/handBack.ts new file mode 100644 index 0000000..ab99616 --- /dev/null +++ b/src/tickets/handBack.ts @@ -0,0 +1,95 @@ +import type { Logger } from '@map-colonies/js-logger'; +import { countAttempt } from '../budget/attempt'; +import type { JiraPort, JiraTicket } from '../jira/types'; +import { releaseTicket, type ReleaseOutcome } from './claim'; + +/** + * Giving a ticket back and counting it against the attempt cap, as one act. + * + * `releaseTicket` (./claim.ts) comments, transitions to Open and unassigns. That is the whole + * release and it is deliberately identity-free and counter-free, because a release is also what + * happens on paths that must *not* count — the hand-straight-back in `runCycle` does no work and + * spends nothing. This module is the other case: the worker tried, it failed or it ran out of + * budget, and the ticket must come back carrying the fact that it was tried. + * + * The counter matters because it is the only thing that ends a loop. `buildPollQuery` filters on + * `agent-ready`, an empty assignee and the label *at* the cap, so a ticket handed back without a + * bumped counter matches the poll again on the very next tick, is claimed again, and is paid for + * again — one ticket able to eat the whole daily allowance. `ATTEMPT_CAP` cannot express "tried + * twice" unless something writes it down. + * + * Both `AbortPort` (src/budget/types.ts) and `ReleasePort` (src/agent/types.ts) are contracts for + * exactly this, and both say the same thing: the count belongs *behind* the one call, so no path + * can release a ticket without counting it. This is the implementation both bind to, so there is + * one of it. + */ + +interface HandBackDeps { + readonly jira: JiraPort; + readonly logger: Logger; + /** + * The cap the poll query filters on — `ATTEMPT_CAP` from src/cycle.ts. + * + * Passed in rather than imported so this does not depend on the cycle it is called from, and + * because `countAttempt` clamps to it: writing a counter *past* the cap would make the ticket + * poll-visible again, which is the opposite of counting it. + */ + readonly attemptCap: number; +} + +interface HandBackOutcome { + /** True when the ticket is back in Open and unassigned. */ + readonly released: boolean; + /** True when the ticket now carries a bumped `agent-attempted-N`. */ + readonly attemptCounted: boolean; + /** Why the hand-back is incomplete, when it is. */ + readonly reason?: string; +} + +/** + * Count the attempt, then release. + * + * The order is the load-bearing part and it is the reverse of how it reads. Unassigning is the + * step that makes a ticket visible to the poll again, and it is the last thing `releaseTicket` + * does — so the counter has to be on the ticket *before* that happens. Counting afterwards leaves + * a window in which the ticket is available and uncounted, and if the count then fails the window + * never closes. + * + * So a label write that fails stops the hand-back entirely: the ticket stays held by the bot and + * In Progress, which the poll query skips, and the boot-time orphan sweep (MAPCO-11432) is what + * recovers it. Held-and-counted-nowhere is recoverable; available-and-uncounted is a re-burn loop. + */ +async function handBackTicket(ticket: JiraTicket, note: string, deps: HandBackDeps): Promise { + const { jira, logger, attemptCap } = deps; + const labels = countAttempt(ticket.labels, attemptCap); + + try { + await jira.setLabels(ticket.key, labels); + } catch (error) { + // Loudly: this is the failure that would otherwise cost real money on every tick. + logger.error({ + msg: 'could not count the attempt, so the ticket was not handed back', + err: error, + key: ticket.key, + labels, + reason: 'releasing an uncounted ticket would let the poll pick it up and re-burn it', + }); + + return { released: false, attemptCounted: false, reason: 'attempt-count-failed' }; + } + + const released: ReleaseOutcome = await releaseTicket(ticket, note, jira); + + if (!released.ok) { + logger.warn({ msg: 'attempt counted but the ticket is still held', key: ticket.key, reason: released.reason, offered: released.offered }); + + return { released: false, attemptCounted: true, reason: released.reason }; + } + + logger.info({ msg: 'ticket handed back', key: ticket.key, labels, attempts: labels.filter((label) => label.startsWith('agent-attempted-')) }); + + return { released: true, attemptCounted: true }; +} + +export { handBackTicket }; +export type { HandBackDeps, HandBackOutcome }; diff --git a/tests/helpers/fakeJira.ts b/tests/helpers/fakeJira.ts index ab28bc8..745e68d 100644 --- a/tests/helpers/fakeJira.ts +++ b/tests/helpers/fakeJira.ts @@ -4,7 +4,8 @@ import type { JiraPort, JiraTicket, JiraTransition } from '@src/jira/types'; export type FakeWrite = | { kind: 'assign'; key: string; assignee: string | null } | { kind: 'transition'; key: string; transitionId: string } - | { kind: 'comment'; key: string; body: string }; + | { kind: 'comment'; key: string; body: string } + | { kind: 'labels'; key: string; labels: readonly string[] }; export interface FakeJiraOptions { tickets?: JiraTicket[]; @@ -17,6 +18,8 @@ export interface FakeJiraOptions { displayNames?: Record; /** Makes the transition lookup fail, standing in for any mid-ticket Jira outage. */ transitionsFailWith?: Error; + /** Makes the label write fail — the one failure that must stop a hand-back. */ + setLabelsFailWith?: Error; /** * Simulates a human winning the race, applied the moment after the worker writes the * assignee — which is exactly the window the optimistic claim's re-read exists to catch. @@ -81,6 +84,24 @@ export class FakeJira implements JiraPort { return Promise.resolve(); } + public async setLabels(issueKey: string, labels: readonly string[]): Promise { + if (this.options.setLabelsFailWith) { + throw this.options.setLabelsFailWith; + } + + this.writes.push({ kind: 'labels', key: issueKey, labels }); + + // Visible to the next read, like the assignee write: a hand-back reads the labels back to + // check the counter landed, and a fake that dropped the write would make that untestable. + const current = this.state.get(issueKey); + + if (current) { + this.state.set(issueKey, { ...current, labels: [...labels] }); + } + + return Promise.resolve(); + } + public async addComment(issueKey: string, body: string): Promise { this.writes.push({ kind: 'comment', key: issueKey, body }); diff --git a/tests/unit/jira/mcpJira.spec.ts b/tests/unit/jira/mcpJira.spec.ts index 80afe64..3806f0c 100644 --- a/tests/unit/jira/mcpJira.spec.ts +++ b/tests/unit/jira/mcpJira.spec.ts @@ -1,6 +1,6 @@ /* eslint-disable @typescript-eslint/naming-convention -- these mirror the MCP server's wire format */ import { describe, expect, it } from 'vitest'; -import { assigneeFields, toTicket, toTransition } from '@src/jira/mcpJira'; +import { assigneeFields, labelsFields, toTicket, toTransition } from '@src/jira/mcpJira'; describe('toTicket', () => { it('should read an unassigned ticket as unclaimed, not as assigned to someone called Unassigned.', () => { @@ -57,6 +57,18 @@ describe('assigneeFields', () => { }); }); +describe('labelsFields', () => { + it('should send the labels as a JSON string, because that is what the tool takes.', () => { + expect(labelsFields(['agent-ready', 'agent-attempted-1'])).toBe('{"labels":["agent-ready","agent-attempted-1"]}'); + }); + + it('should send the complete set, because the update overwrites rather than adds.', () => { + // The attempt counter is the reason this matters: a write that dropped `agent-ready` would + // un-enrol the ticket, and enrolment is a human's decision. + expect(labelsFields([])).toBe('{"labels":[]}'); + }); +}); + describe('toTransition', () => { it('should keep the status a transition lands in, which is what the worker matches on.', () => { // Transition *names* are verbs on a real workflow — "Start Progress", not "In Progress" — diff --git a/tests/unit/tickets/handBack.spec.ts b/tests/unit/tickets/handBack.spec.ts new file mode 100644 index 0000000..3df8792 --- /dev/null +++ b/tests/unit/tickets/handBack.spec.ts @@ -0,0 +1,97 @@ +import { describe, expect, it } from 'vitest'; +import { handBackTicket } from '@src/tickets/handBack'; +import { FakeJira, ticket } from '@tests/helpers/fakeJira'; +import { fakeLogger } from '@tests/helpers/fakeLogger'; +import type { FakeWrite } from '@tests/helpers/fakeJira'; + +const ATTEMPT_CAP = 2; +const NOTE = 'Ran out of budget on this one.'; + +const workflow = { + 'MAPCO-1': [ + { id: '21', name: 'Start Progress', to: 'In Progress' }, + { id: '11', name: 'Reopen', to: 'Open' }, + ], +}; + +function kinds(writes: readonly FakeWrite[]): string[] { + return writes.map((write) => write.kind); +} + +describe('handBackTicket', () => { + it('should count the attempt and give the ticket back.', async () => { + const { logger } = fakeLogger(); + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow }); + + const outcome = await handBackTicket(ticket(), NOTE, { jira, logger, attemptCap: ATTEMPT_CAP }); + + expect(outcome).toStrictEqual({ released: true, attemptCounted: true }); + expect(jira.writes).toContainEqual({ kind: 'labels', key: 'MAPCO-1', labels: ['agent-ready', 'agent-attempted-1'] }); + expect(jira.writes).toContainEqual({ kind: 'assign', key: 'MAPCO-1', assignee: null }); + }); + + it('should write the counter before unassigning, because unassigning is what makes it pollable.', async () => { + const { logger } = fakeLogger(); + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow }); + + await handBackTicket(ticket(), NOTE, { jira, logger, attemptCap: ATTEMPT_CAP }); + + // Order is the whole correctness argument: an unassigned ticket matches the poll query, so a + // counter written after the unassign leaves a window where the ticket is available and + // uncounted — and if that write then fails, the window never closes. + expect(kinds(jira.writes)).toStrictEqual(['labels', 'comment', 'transition', 'assign']); + }); + + it('should keep holding the ticket when the counter cannot be written.', async () => { + const { logger, lines } = fakeLogger(); + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow, setLabelsFailWith: new Error('jira said no') }); + + const outcome = await handBackTicket(ticket(), NOTE, { jira, logger, attemptCap: ATTEMPT_CAP }); + + // Held-and-uncounted is recovered by the boot-time orphan sweep. Released-and-uncounted is a + // ticket that polls straight back in and is paid for again, for ever. + expect(outcome).toMatchObject({ released: false, attemptCounted: false, reason: 'attempt-count-failed' }); + expect(jira.writes).toStrictEqual([]); + expect(lines.filter((line) => line.level === 'error')).toHaveLength(1); + }); + + it('should bump an existing counter rather than adding a second one.', async () => { + const { logger } = fakeLogger(); + const held = ticket({ labels: ['agent-ready', 'agent-attempted-1', 'needs-discussion'] }); + const jira = new FakeJira({ tickets: [held], transitions: workflow }); + + await handBackTicket(held, NOTE, { jira, logger, attemptCap: ATTEMPT_CAP }); + + expect(jira.writes[0]).toStrictEqual({ + kind: 'labels', + key: 'MAPCO-1', + labels: ['agent-ready', 'needs-discussion', 'agent-attempted-2'], + }); + }); + + it('should never write a counter past the cap, which would make the ticket pollable again.', async () => { + const { logger } = fakeLogger(); + const capped = ticket({ labels: ['agent-ready', 'agent-attempted-2'] }); + const jira = new FakeJira({ tickets: [capped], transitions: workflow }); + + await handBackTicket(capped, NOTE, { jira, logger, attemptCap: ATTEMPT_CAP }); + + // The poll excludes the label *exactly* at the cap, so `agent-attempted-3` would not tighten + // anything — it would hand the ticket straight back to the worker. + expect(jira.writes[0]).toStrictEqual({ kind: 'labels', key: 'MAPCO-1', labels: ['agent-ready', 'agent-attempted-2'] }); + }); + + it('should report the attempt as counted even when the release cannot finish.', async () => { + const { logger } = fakeLogger(); + const stuck = { 'MAPCO-1': [{ id: '21', name: 'Start Progress', to: 'In Progress' }] }; + const jira = new FakeJira({ tickets: [ticket()], transitions: stuck }); + + const outcome = await handBackTicket(ticket(), NOTE, { jira, logger, attemptCap: ATTEMPT_CAP }); + + // A workflow with no way back to Open leaves the ticket held, which is the containment + // `releaseTicket` chooses on purpose. The count still stands, and saying so is what stops a + // caller counting it twice. + expect(outcome).toMatchObject({ released: false, attemptCounted: true, reason: 'no-transition' }); + expect(jira.writes.filter((write) => write.kind === 'assign')).toStrictEqual([]); + }); +});