From fcdfcb5beecda827a9fd539b3616a7878114d856 Mon Sep 17 00:00:00 2001 From: razbroc Date: Sun, 23 Aug 2026 11:24:52 +0300 Subject: [PATCH] feat: count an attempt when a ticket is handed back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `JiraPort.setLabels` and `handBackTicket`, the one binding both `AbortPort` (MAPCO-11435) and `ReleasePort` (MAPCO-11434) are meant to use. The attempt counter is the only thing that ends a loop. The poll filters on agent-ready, an empty assignee and the label at ATTEMPT_CAP, so a ticket handed back without a bumped counter matches again on the next tick, is claimed again and is paid for again. Nothing could write that label before this. Counting happens before releasing, which is the reverse of how it reads. Unassigning is what makes a ticket visible to the poll and it is the last thing releaseTicket does, so the counter has to already be there. A label write that fails therefore stops the hand-back: held and In Progress is recovered by the boot-time orphan sweep, whereas available and uncounted is a re-burn loop. `setLabels` overwrites the whole set, so callers pass what they want kept. `countAttempt` clamps at the cap — writing past it would make the ticket pollable again rather than tightening anything. --- src/jira/mcpJira.ts | 19 +++++- src/jira/types.ts | 13 ++++ src/tickets/handBack.ts | 95 ++++++++++++++++++++++++++++ tests/helpers/fakeJira.ts | 23 ++++++- tests/unit/jira/mcpJira.spec.ts | 14 ++++- tests/unit/tickets/handBack.spec.ts | 97 +++++++++++++++++++++++++++++ 6 files changed, 258 insertions(+), 3 deletions(-) create mode 100644 src/tickets/handBack.ts create mode 100644 tests/unit/tickets/handBack.spec.ts 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([]); + }); +});