From 1138d6955d776cd885235b381f057e7087faaf25 Mon Sep 17 00:00:00 2001 From: razbroc Date: Thu, 20 Aug 2026 17:40:34 +0300 Subject: [PATCH 1/2] feat: claim a ticket and release it, with nothing in between MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Walks the whole Jira state machine with an empty middle: take the oldest ready ticket, assign the bot, move it to In Progress, then comment, return it to Open and unassign. Claiming is optimistic because Jira is the only state store. After writing the assignee the worker re-reads the issue, and a name that is not its own means a human got there first: back off, write nothing further, and do not clear the field — that would take the ticket off them. Release runs comment, transition, unassign, in that order. Unassigning is what makes a ticket visible to the poll query again, so it goes last: a failure part-way through leaves the ticket held and In Progress, which the query skips and the boot-time sweep recovers. Unassigning first risks leaving a ticket that polls straight back in and gets commented on every cycle. Transitions are matched on their target status rather than their own name, because transition names are verbs on a real workflow (Start Progress). Both jira_get_transitions and expand=transitions are rejected by the write-pilot MCP server, so the actual vocabulary could not be verified the way the poll query was; a no-transition refusal logs the names it was offered, so the first real run reports them instead of refusing every ticket in silence. The bot's identity is configured rather than discovered. The MCP server runs under a shared service account with no per-user attribution, and Jira takes an identifier on write but returns a surname-first display name on read, so both halves are required and neither can be derived from the other. Refs: MAPCO-11431 --- README.md | 59 ++++++++++-- helm/templates/deployment.yaml | 4 + helm/values.yaml | 6 ++ src/common/constants.ts | 11 +++ src/common/workerConfig.ts | 25 ++++- src/cycle.ts | 101 ++++++++++++++++++-- src/dryRun.ts | 3 +- src/jira/mcpJira.ts | 57 +++++++++-- src/jira/types.ts | 25 +++++ src/tickets/claim.ts | 132 ++++++++++++++++++++++++++ tests/helpers/fakeJira.ts | 82 +++++++++++++++- tests/helpers/fakeLogger.ts | 4 +- tests/integration/cycle.spec.ts | 158 ++++++++++++++++++++++++++++--- tests/unit/jira/mcpJira.spec.ts | 34 ++++++- tests/unit/scheduler.spec.ts | 19 +++- tests/unit/tickets/claim.spec.ts | 128 +++++++++++++++++++++++++ tests/unit/workerConfig.spec.ts | 22 ++++- 17 files changed, 816 insertions(+), 54 deletions(-) create mode 100644 src/tickets/claim.ts create mode 100644 tests/unit/tickets/claim.spec.ts diff --git a/README.md b/README.md index e01454a..90a0041 100644 --- a/README.md +++ b/README.md @@ -4,8 +4,9 @@ Pulls `agent-ready` Jira tickets from the MAPCO project, implements them, and op requests. Designed in [MAPCO-11374](https://mapcolonies.atlassian.net/browse/MAPCO-11374), substrate decided in [MAPCO-11377](https://mapcolonies.atlassian.net/browse/MAPCO-11377). -**Current slice: MAPCO-11429.** It polls and reports. It claims nothing, writes nothing to -Jira, and touches no repository. +**Current slice: MAPCO-11431.** It walks the whole Jira state machine with nothing in the +middle — it claims a ticket and hands it straight back with a comment. It touches no +repository and writes no code yet. ## Shape @@ -71,6 +72,8 @@ so refusal is the common path until the convention spreads. | Variable | Default | Meaning | |---|---|---| | `MCP_ATLASSIAN_URL` | *required* | Address of the `atlassian-write` MCP server. Transport is picked from the path: `/sse` gets SSE, anything else Streamable HTTP | +| `JIRA_BOT_ACCOUNT` | *required* | Identifier written to a ticket's assignee field — an email or accountId | +| `JIRA_BOT_DISPLAY_NAME` | *required* | What `JIRA_BOT_ACCOUNT` reads back as, surname-first. The claim re-read compares against this | | `POLL_INTERVAL_MS` | `300000` | How often a cycle runs | | `MAX_TICKETS_PER_RUN` | `1` | Tickets one cycle may start | | `MAX_CONCURRENT_TICKETS` | `1` | Tickets in flight at once | @@ -78,24 +81,64 @@ so refusal is the common path until the convention spreads. Raise `MAX_TICKETS_PER_RUN` before ever raising `MAX_CONCURRENT_TICKETS`. +The two bot-identity variables look redundant and are not: Jira takes an *identifier* on +write and hands back a *display name* on read, and neither is derivable from the other in +this instance. Set them inconsistently and every claim reads as lost. + +## Claiming and releasing + +Jira is the only state store — no database, no files that outlive a run — so there is no +lock to take. Claiming is **optimistic**: + +1. Look up the transition into `In Progress` *before* writing anything. A workflow with no + route in means the ticket can never be worked, and that is a refusal, not a half-claim. + The lookup matches a transition's **target status**, not its name — transition names are + verbs (`Start Progress`), so matching by name alone would refuse every ticket. +2. Write the assignee, then **read the issue back**. If the assignee is not the bot, a human + got there first: back off, write nothing further, and do not try to take it back off + them. Our write is already overwritten and is not ours to undo. +3. Transition to `In Progress`. + +Releasing runs in the order comment → transition to `Open` → **unassign last**. That order +is deliberate and is the reverse of how it reads. Unassigning is what makes a ticket visible +to the poll query again (it filters `assignee is EMPTY`), so it goes last: if anything fails +part-way, the ticket is left held by the bot and `In Progress`, which the query skips and the +boot-time orphan sweep (MAPCO-11432) recovers. Unassigning first risks leaving a ticket +unassigned and `In Progress` — which polls straight back in, forever. + ## Known gaps - The worker knobs are read from the environment rather than `@map-colonies/config`, which needs a schema published in `@map-colonies/schemas`. Telemetry still goes through the library. Registering a real schema is follow-up work. -- **The Jira identity is the shared MCP service account.** It has no per-user attribution, - so the optimistic claim check (MAPCO-11431) cannot distinguish this worker from any other - session using the same MCP, and boot-time orphan release (MAPCO-11432) could release a - ticket someone else is working. A dedicated Jira account is the recommendation. +- **The Jira identity is configured, not discovered.** The MCP server runs under a shared + service account with no per-user attribution, so the worker cannot ask Jira who it is — + hence `JIRA_BOT_ACCOUNT` / `JIRA_BOT_DISPLAY_NAME`. The claim re-read can therefore tell + the bot apart from a *human*, but not from a second worker configured with the same + account. A dedicated Jira account per deployment is still the recommendation, and it is + what makes boot-time orphan release (MAPCO-11432) safe. +- **The real transition vocabulary is unverified.** `jira_get_transitions` and + `expand=transitions` are both rejected by the write-pilot MCP server, so the MAPCO + workflow's actual transition names and target statuses could not be read the way the poll + query was verified in MAPCO-11427. The lookup matches on target status with the + transition name as a fallback, which covers both shapes, and a `no-transition` refusal + logs the `offered` names — so the first real run reports the vocabulary rather than + refusing in silence. Confirm it from that log line before trusting a deployment. - `helm lint` needs the private `mclabels` dependency and fails without registry access. ## Dry run -One cycle against the real MCP server, from a laptop. Read-only. Requires the corporate -VPN — the server is not reachable from outside it. +One cycle against the real MCP server, from a laptop. Requires the corporate VPN — the +server is not reachable from outside it. + +**This writes to real tickets.** It claims the oldest `agent-ready` ticket and hands it +straight back, leaving a comment behind. That is the point: label a ticket `agent-ready` and +watch it get claimed and returned. ```sh MCP_ATLASSIAN_URL="https://atlassian-mcp-write.mapcolonies.net/sse" \ + JIRA_BOT_ACCOUNT="developer-agent@mapcolonies.net" \ + JIRA_BOT_DISPLAY_NAME="AGENT DEVELOPER" \ GITHUB_TOKEN="$(gh auth token)" npm run dry-run ``` diff --git a/helm/templates/deployment.yaml b/helm/templates/deployment.yaml index 9254719..815ecc3 100644 --- a/helm/templates/deployment.yaml +++ b/helm/templates/deployment.yaml @@ -73,6 +73,10 @@ spec: fieldPath: metadata.uid - name: MCP_ATLASSIAN_URL value: {{ .Values.worker.mcpUrl | quote }} + - name: JIRA_BOT_ACCOUNT + value: {{ .Values.worker.botAccount | quote }} + - name: JIRA_BOT_DISPLAY_NAME + value: {{ .Values.worker.botDisplayName | quote }} - name: POLL_INTERVAL_MS value: {{ .Values.worker.pollIntervalMs | quote }} - name: MAX_TICKETS_PER_RUN diff --git a/helm/values.yaml b/helm/values.yaml index 4f98ae0..fa8e035 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -59,6 +59,12 @@ worker: # In-cluster address of the self-hosted atlassian-write MCP server. The worker holds no # Jira credentials of its own — that server already has them. mcpUrl: 'http://mcp-atlassian:8080/mcp' + # Who the worker claims tickets as. Two values because Jira takes an identifier on write + # and returns a display name on read, and display names here are surname-first — the + # optimistic claim's re-read compares against botDisplayName, so a mismatch between + # these two makes every claim look lost. + botAccount: '' + botDisplayName: '' pollIntervalMs: 300000 maxTicketsPerRun: 1 maxConcurrentTickets: 1 diff --git a/src/common/constants.ts b/src/common/constants.ts index 1aace74..90b3325 100644 --- a/src/common/constants.ts +++ b/src/common/constants.ts @@ -17,6 +17,17 @@ export const LABELS = { attemptedPrefix: 'agent-attempted-', } as const; +/** + * The two statuses the claim/release cycle moves a ticket between. + * + * Matched against a transition's *target status*, not against transition names — those are + * verbs on a real workflow (`Start Progress`). + */ +export const STATUS_NAMES = { + inProgress: 'In Progress', + open: 'Open', +} as const; + /** * Status *names* that mean the ticket is finished. * diff --git a/src/common/workerConfig.ts b/src/common/workerConfig.ts index 2cff7cc..154d68c 100644 --- a/src/common/workerConfig.ts +++ b/src/common/workerConfig.ts @@ -5,9 +5,11 @@ * schema published in `@map-colonies/schemas`, and no schema exists for this service yet; * the telemetry half still goes through it (see `common/config.ts`) because * `commonBoilerplateV3` already covers logger and tracing. Registering a real schema for - * these knobs is follow-up work, not a blocker for the read-only slice. + * these knobs is follow-up work, not a blocker for the current slice. */ +import type { BotIdentity } from '../tickets/claim'; + interface WorkerConfig { /** How often the internal scheduler runs a cycle. */ readonly pollIntervalMs: number; @@ -17,6 +19,8 @@ interface WorkerConfig { readonly maxConcurrentTickets: number; /** Base URL of the in-cluster `atlassian-write` MCP server. */ readonly mcpUrl: string; + /** Who the worker claims tickets as. Both halves are required — see `BotIdentity`. */ + readonly bot: BotIdentity; } const DEFAULT_POLL_INTERVAL_MS = 300_000; @@ -42,17 +46,28 @@ function readInt(env: NodeJS.ProcessEnv, name: string, fallback: number): number return parsed; } -function loadWorkerConfig(env: NodeJS.ProcessEnv = process.env): WorkerConfig { - const mcpUrl = env.MCP_ATLASSIAN_URL; - if (mcpUrl === undefined || mcpUrl === '') { - throw new ConfigError('MCP_ATLASSIAN_URL must be set — the worker has no Jira credentials of its own'); +function readRequired(env: NodeJS.ProcessEnv, name: string, why: string): string { + const raw = env[name]; + if (raw === undefined || raw === '') { + throw new ConfigError(`${name} must be set — ${why}`); } + return raw; +} + +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 = { + account: readRequired(env, 'JIRA_BOT_ACCOUNT', 'the worker has no way to put its own name on a ticket'), + displayName: readRequired(env, 'JIRA_BOT_DISPLAY_NAME', "the worker could not tell its own claims from a human's"), + }; + 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, }; } diff --git a/src/cycle.ts b/src/cycle.ts index fdcc176..21d2fa9 100644 --- a/src/cycle.ts +++ b/src/cycle.ts @@ -2,17 +2,32 @@ import type { Logger } from '@map-colonies/js-logger'; import type { WorkerConfig } from '@common/workerConfig'; import { buildPollQuery } from './jira/query'; import type { JiraPort, JiraTicket } from './jira/types'; +import { claimTicket, releaseTicket } from './tickets/claim'; /** The attempt cap from MAPCO-11432. Enforced in the query so a capped ticket is never even seen. */ -export const ATTEMPT_CAP = 2; +const ATTEMPT_CAP = 2; -export interface CycleDeps { +/** + * What the worker says on a ticket it hands straight back. + * + * Written to be read by whoever finds the ticket back in Open and wonders what touched it. + * It names what was tried, which in this slice is nothing at all. + */ +const HANDED_BACK_NOTE = [ + 'Picked this up automatically and handed it straight back.', + '', + 'This build of the developer agent can claim a ticket and release it, but there is nothing in between yet (MAPCO-11431) — no branch, no changes, no pull request. Nothing was modified, and this does not count as an attempt.', + '', + 'The ticket is available again for whoever wants it next.', +].join('\n'); + +interface CycleDeps { readonly jira: JiraPort; readonly logger: Logger; readonly config: WorkerConfig; } -export interface CycleResult { +interface CycleResult { readonly found: number; readonly started: number; readonly skipped: number; @@ -20,6 +35,69 @@ export interface CycleResult { readonly outcome: 'ok' | 'failed'; } +/** + * Work through the run's tickets, no more than `maxConcurrentTickets` at a time. + * + * Both caps default to 1, which makes this sequential — the interesting case is a + * deployment that raises them, where the concurrency cap is what keeps the worker from + * claiming a whole page of tickets at once. + */ +async function handleTickets(tickets: JiraTicket[], deps: CycleDeps): Promise { + const { maxConcurrentTickets } = deps.config; + let started = 0; + + for (let index = 0; index < tickets.length; index += maxConcurrentTickets) { + const batch = tickets.slice(index, index + maxConcurrentTickets); + const outcomes = await Promise.all(batch.map(async (ticket) => handleTicket(ticket, deps))); + + started += outcomes.filter((outcome) => outcome).length; + } + + return started; +} + +/** + * Claim one ticket and hand it back. Returns whether the worker actually held it. + * + * Nothing in here is allowed to end the run: one ticket the worker cannot take says + * nothing about the next one, and a run that dies on the first refusal would stall the + * whole queue behind it. + */ +async function handleTicket(ticket: JiraTicket, deps: CycleDeps): Promise { + const { jira, logger, config } = deps; + + try { + const claim = await claimTicket(ticket, jira, config.bot); + + if (!claim.ok) { + // `saw` and `offered` are the diagnostic half of a refusal. A `lost-race` reporting + // the bot's own name means JIRA_BOT_DISPLAY_NAME is wrong, not that a human raced; + // `offered` names the transitions a workflow actually has. + logger.warn({ msg: 'not claimed', key: ticket.key, reason: claim.reason, saw: claim.saw, offered: claim.offered }); + + return false; + } + + logger.info({ msg: 'claimed', key: ticket.key }); + + const release = await releaseTicket(ticket, HANDED_BACK_NOTE, jira); + + if (!release.ok) { + // Still assigned to the bot and still In Progress, on purpose — see `releaseTicket`. + // The orphan sweep on boot (MAPCO-11432) is what gets it back. + logger.error({ msg: 'held ticket could not be released', key: ticket.key, reason: release.reason, offered: release.offered }); + } else { + logger.info({ msg: 'released', key: ticket.key }); + } + + return true; + } catch (error) { + logger.error({ msg: 'ticket failed', key: ticket.key, err: error }); + + return false; + } +} + /** * One full run of the worker, start to finish. * @@ -27,9 +105,10 @@ export interface CycleResult { * scheduler calls it, and so do the tests. Later slices add cases here rather than * standing up harnesses of their own. * - * In this slice it polls and reports. It starts nothing and writes nothing to Jira. + * In this slice it walks the whole Jira state machine with nothing in the middle: claim a + * ticket, then release it again (MAPCO-11431). */ -export async function runCycle(deps: CycleDeps): Promise { +async function runCycle(deps: CycleDeps): Promise { const { jira, logger, config } = deps; const jql = buildPollQuery(ATTEMPT_CAP); @@ -47,10 +126,12 @@ export async function runCycle(deps: CycleDeps): Promise { const more = tickets.length > config.maxTicketsPerRun; const eligible = tickets.slice(0, config.maxTicketsPerRun); + const started = await handleTickets(eligible, deps); + const result: CycleResult = { found: eligible.length, - started: 0, - skipped: eligible.length, + started, + skipped: eligible.length - started, more, outcome: 'ok', }; @@ -58,12 +139,14 @@ export async function runCycle(deps: CycleDeps): Promise { // One structured line per run, shaped so Loki can ingest it later. A missing run line // is the alarm — there is no alerting stack by decision (MAPCO-11437). logger.info({ - msg: 'poll complete', + msg: 'cycle complete', ...result, - reason: 'read-only slice: this worker cannot claim yet', keys: eligible.map((ticket) => ticket.key), tokensSpent: 0, }); return result; } + +export { ATTEMPT_CAP, HANDED_BACK_NOTE, runCycle }; +export type { CycleDeps, CycleResult }; diff --git a/src/dryRun.ts b/src/dryRun.ts index dff76a7..73756e7 100644 --- a/src/dryRun.ts +++ b/src/dryRun.ts @@ -6,7 +6,8 @@ * It exercises the same `runCycle` seam the deployed worker runs, so what it proves is * about the worker and not about the harness. * - * Read-only, like the slice it belongs to. + * Not read-only: this walks the same claim-and-release path the deployed worker walks, so + * it comments on, assigns and transitions a real ticket (MAPCO-11431). */ import 'reflect-metadata'; import { jsLogger } from '@map-colonies/js-logger'; diff --git a/src/jira/mcpJira.ts b/src/jira/mcpJira.ts index fc249c5..8b187fc 100644 --- a/src/jira/mcpJira.ts +++ b/src/jira/mcpJira.ts @@ -9,6 +9,12 @@ import type { JiraPort, JiraTicket, JiraTransition } from './types'; const POLL_FIELDS = 'summary,status,labels,assignee,issuetype,created'; /* eslint-disable @typescript-eslint/naming-convention -- these mirror the MCP server's wire format */ +interface McpTransition { + id: number | string; + name: string; + to?: { name?: string } | string; +} + interface McpTicket { key: string; summary?: string; @@ -18,14 +24,14 @@ interface McpTicket { assignee?: { display_name?: string } | null; } +/* eslint-enable @typescript-eslint/naming-convention */ + /** * Jira access via the org's self-hosted `atlassian-write` MCP server (MAPCO-11427). * * The worker holds no Jira credentials — that server already has them. It is reached * in-cluster, which is also why the worker needs no inbound Route of its own. */ -/* eslint-enable @typescript-eslint/naming-convention */ - class McpJira implements JiraPort { private client: Client | undefined; @@ -41,12 +47,35 @@ class McpJira implements JiraPort { return (parsed.issues ?? []).map(toTicket); } + public async getIssue(issueKey: string): Promise { + // eslint-disable-next-line @typescript-eslint/naming-convention -- MCP wire format + const raw = await this.call('jira_get_issue', { issue_key: issueKey, fields: POLL_FIELDS, comment_limit: 0 }); + const parsed = JSON.parse(raw) as McpTicket | null; + + return parsed?.key === undefined ? null : toTicket(parsed); + } + + public async assign(issueKey: string, assignee: string | null): Promise { + // eslint-disable-next-line @typescript-eslint/naming-convention -- MCP wire format + await this.call('jira_update_issue', { issue_key: issueKey, fields: assigneeFields(assignee) }); + } + + public async transition(issueKey: string, transitionId: string): Promise { + // eslint-disable-next-line @typescript-eslint/naming-convention -- MCP wire format + await this.call('jira_transition_issue', { issue_key: issueKey, transition_id: transitionId }); + } + + public async addComment(issueKey: string, body: string): Promise { + // eslint-disable-next-line @typescript-eslint/naming-convention -- MCP wire format + await this.call('jira_add_comment', { issue_key: issueKey, body }); + } + 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 }); - const parsed = JSON.parse(raw) as { id: number | string; name: string }[]; + const parsed = JSON.parse(raw) as McpTransition[]; - return parsed.map((transition) => ({ id: String(transition.id), name: transition.name })); + return parsed.map(toTransition); } public async close(): Promise { @@ -106,6 +135,22 @@ function toAssignee(displayName: string | undefined): string | null { return displayName; } +/** + * The `fields` argument of `jira_update_issue`, which takes a JSON *string* rather than an + * object. Passing an object updates nothing and still reports success, which would make a + * failed claim look like it held — so this is its own named, tested function. + */ +function assigneeFields(assignee: string | null): string { + return JSON.stringify({ assignee }); +} + +/** 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; + + return { id: String(transition.id), name: transition.name, to }; +} + function toTicket(issue: McpTicket): JiraTicket { return { key: issue.key, @@ -118,5 +163,5 @@ function toTicket(issue: McpTicket): JiraTicket { }; } -export { McpJira, toTicket }; -export type { McpTicket }; +export { assigneeFields, McpJira, toTicket, toTransition }; +export type { McpTicket, McpTransition }; diff --git a/src/jira/types.ts b/src/jira/types.ts index fe58491..ec83ad5 100644 --- a/src/jira/types.ts +++ b/src/jira/types.ts @@ -9,7 +9,16 @@ export interface JiraTicket { export interface JiraTransition { readonly id: string; + /** The transition's own name — a verb on a real workflow, like `Start Progress`. */ readonly name: string; + /** + * The status this transition lands the ticket in, when the server reports one. + * + * This is what the worker matches on. Asking for "the transition into In Progress" by + * *name* only works on a workflow whose transitions happen to be named after their + * target status, which is not the common case. + */ + readonly to?: string; } /** @@ -21,4 +30,20 @@ export interface JiraTransition { export interface JiraPort { search: (jql: string, limit: number) => Promise; getTransitions: (issueKey: string) => Promise; + /** + * Read one issue back. This is the confirmation half of the optimistic claim: Jira is + * the only state store, so the only way to know a claim held is to ask again. + */ + getIssue: (issueKey: string) => Promise; + /** + * Write the assignee. `null` unassigns. + * + * Note the asymmetry with what reads return: a write takes an *identifier* (email or + * accountId) while a read returns a *display name*, and in this instance display names + * are surname-first, so the two cannot be assumed equal. That is why the worker is + * configured with both (see `WorkerConfig.botAccount` / `botDisplayName`). + */ + assign: (issueKey: string, assignee: string | null) => Promise; + transition: (issueKey: string, transitionId: string) => Promise; + addComment: (issueKey: string, body: string) => Promise; } diff --git a/src/tickets/claim.ts b/src/tickets/claim.ts new file mode 100644 index 0000000..56cdb11 --- /dev/null +++ b/src/tickets/claim.ts @@ -0,0 +1,132 @@ +import { STATUS_NAMES } from '@common/constants'; +import type { JiraPort, JiraTicket, JiraTransition } from '../jira/types'; + +/** + * Who the worker claims tickets as. + * + * Two values, because Jira is asymmetric: a write takes an identifier and a read hands back + * a display name, surname-first in this instance, and neither is derivable from the other. + * The worker cannot ask Jira who it is — the MCP server runs under a shared service account + * — so it has to be told both. + */ +interface BotIdentity { + /** Written to the assignee field: an email or accountId. */ + readonly account: string; + /** What `account` reads back as. The re-read compares against this. */ + readonly displayName: string; +} + +/** + * Why a claim or release did not happen. Each one is a normal outcome, not an error — the + * worker moves on and the ticket stays available to whoever does hold it. + */ +type Refusal = 'already-assigned' | 'lost-race' | 'no-transition'; + +interface Refused { + readonly ok: false; + readonly reason: Refusal; + /** + * Who the re-read actually found, on `lost-race`. Worth reporting: if this comes back as + * the bot's own name, the configured `displayName` is wrong rather than a human having + * raced — a failure mode that otherwise looks identical and drains the queue silently. + */ + readonly saw?: string | null; + /** + * The transition names the workflow did offer, on `no-transition`. The real vocabulary is + * unverified (`jira_get_transitions` is not reachable from the write pilot), so the first + * run against a real workflow needs to report what it was given. + */ + readonly offered?: readonly string[]; +} + +type ClaimOutcome = { readonly ok: true } | Refused; +type ReleaseOutcome = { readonly ok: true } | Refused; + +/** + * Find the transition that lands a ticket in `status`. + * + * Target status first, transition name second. Jira transition names are verbs on a real + * workflow — `Start Progress`, not `In Progress` — so matching on the name alone would find + * nothing and refuse every ticket. The name is kept only as a fallback for a server that + * reports no target status. + */ +function findTransition(transitions: readonly JiraTransition[], status: string): JiraTransition | undefined { + return transitions.find((candidate) => candidate.to === status) ?? transitions.find((candidate) => candidate.name === status); +} + +function refuseNoTransition(transitions: readonly JiraTransition[]): Refused { + return { ok: false, reason: 'no-transition', offered: transitions.map((candidate) => candidate.name) }; +} + +/** + * Take a ticket, optimistically. + * + * Jira is the only state store (MAPCO-11430), so there is no lock to take — the worker + * writes the assignee and then asks Jira who actually holds it. + */ +async function claimTicket(ticket: JiraTicket, jira: JiraPort, bot: BotIdentity): Promise { + // The poll query already filters `assignee is EMPTY`, but a search result is a snapshot + // and this is the last look before a write. Checked here so the guard exists even when + // the caller found the ticket some other way. + if (ticket.assignee !== null) { + return { ok: false, reason: 'already-assigned' }; + } + + const transitions = await jira.getTransitions(ticket.key); + const inProgress = findTransition(transitions, STATUS_NAMES.inProgress); + + // Read before write. A workflow with no route into In Progress means this ticket can + // never be worked, and finding that out after the assign would leave it held by a bot + // that cannot start it. + if (!inProgress) { + return refuseNoTransition(transitions); + } + + await jira.assign(ticket.key, bot.account); + + // The confirmation. Between the search and the write above, a human may have taken the + // ticket; Jira's last write wins, so the only way to know whose name is on it is to ask. + const confirmed = await jira.getIssue(ticket.key); + if (confirmed?.assignee !== bot.displayName) { + // Deliberately no unwind: if someone else's name is on the field, clearing it would + // take the ticket off them, which is worse than leaving our write overwritten. + return { ok: false, reason: 'lost-race', saw: confirmed?.assignee ?? null }; + } + + await jira.transition(ticket.key, inProgress.id); + + return { ok: true }; +} + +/** + * Give a ticket back, saying what was tried. + * + * The order — comment, transition, unassign — is deliberate and is the opposite of the way + * it reads naturally. Unassigning is the step that makes a ticket visible to the poll query + * again (it filters `assignee is EMPTY`), so it goes last: if anything fails part-way, the + * ticket is left held by the bot and In Progress, which the query skips, and the orphan + * sweep on boot (MAPCO-11432) is what recovers it. Unassigning first would risk leaving a + * ticket unassigned and In Progress — which polls straight back in, forever. + * + * Takes no identity: releasing is the same act whoever holds the ticket. + */ +async function releaseTicket(ticket: JiraTicket, note: string, jira: JiraPort): Promise { + await jira.addComment(ticket.key, note); + + const transitions = await jira.getTransitions(ticket.key); + const open = findTransition(transitions, STATUS_NAMES.open); + + if (!open) { + // Cannot get it back to Open, so do not unassign either — held-and-stuck is recoverable + // by the orphan sweep, unassigned-and-stuck is a re-claim loop. + return refuseNoTransition(transitions); + } + + await jira.transition(ticket.key, open.id); + await jira.assign(ticket.key, null); + + return { ok: true }; +} + +export { claimTicket, releaseTicket }; +export type { BotIdentity, ClaimOutcome, Refusal, ReleaseOutcome }; diff --git a/tests/helpers/fakeJira.ts b/tests/helpers/fakeJira.ts index 774dcc1..ab28bc8 100644 --- a/tests/helpers/fakeJira.ts +++ b/tests/helpers/fakeJira.ts @@ -1,20 +1,47 @@ import type { JiraPort, JiraTicket, JiraTransition } from '@src/jira/types'; +/** Every write the worker made, in order. Order is behaviour here, so tests assert on it. */ +export type FakeWrite = + | { kind: 'assign'; key: string; assignee: string | null } + | { kind: 'transition'; key: string; transitionId: string } + | { kind: 'comment'; key: string; body: string }; + export interface FakeJiraOptions { tickets?: JiraTicket[]; transitions?: Record; failWith?: Error; + /** + * What a written assignee identifier reads back as. Models the real asymmetry: the + * worker writes an email or accountId, Jira reports a surname-first display name. + */ + displayNames?: Record; + /** Makes the transition lookup fail, standing in for any mid-ticket Jira outage. */ + transitionsFailWith?: 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. + */ + stealOnAssign?: string; } /** - * A double for the Jira side of the seam. Records the queries it was asked, so tests can - * assert on what the worker *asked for* without asserting on how it phrased anything the - * worker is free to change. + * A double for the Jira side of the seam. + * + * It models exactly one piece of Jira's behaviour: an assignee write is visible to the next + * read. That is not decoration — the optimistic claim's whole correctness rests on it. It + * models nothing else, so a test can only ever assert on what the worker asked for. */ export class FakeJira implements JiraPort { public readonly queries: { jql: string; limit: number }[] = []; + public readonly writes: FakeWrite[] = []; + + private readonly state = new Map(); - public constructor(private readonly options: FakeJiraOptions = {}) {} + public constructor(private readonly options: FakeJiraOptions = {}) { + for (const seed of options.tickets ?? []) { + this.state.set(seed.key, seed); + } + } public async search(jql: string, limit: number): Promise { this.queries.push({ jql, limit }); @@ -23,12 +50,57 @@ export class FakeJira implements JiraPort { throw this.options.failWith; } - return Promise.resolve((this.options.tickets ?? []).slice(0, limit)); + return Promise.resolve([...this.state.values()].slice(0, limit)); } public async getTransitions(issueKey: string): Promise { + if (this.options.transitionsFailWith) { + throw this.options.transitionsFailWith; + } + return Promise.resolve(this.options.transitions?.[issueKey] ?? []); } + + public async getIssue(issueKey: string): Promise { + return Promise.resolve(this.state.get(issueKey) ?? null); + } + + public async assign(issueKey: string, assignee: string | null): Promise { + this.writes.push({ kind: 'assign', key: issueKey, assignee }); + this.patch(issueKey, { assignee: this.readsBackAs(assignee) }); + + return Promise.resolve(); + } + + public async transition(issueKey: string, transitionId: string): Promise { + this.writes.push({ kind: 'transition', key: issueKey, transitionId }); + + // Deliberately does not move the ticket's status. Modelling that would invent a rule + // Jira does not promise, and tests asserting on it would be checking this fake rather + // than the worker — the recorded writes already say which transition was asked for. + return Promise.resolve(); + } + + public async addComment(issueKey: string, body: string): Promise { + this.writes.push({ kind: 'comment', key: issueKey, body }); + + return Promise.resolve(); + } + + private readsBackAs(assignee: string | null): string | null { + if (assignee === null) { + return null; + } + + return this.options.stealOnAssign ?? this.options.displayNames?.[assignee] ?? assignee; + } + + private patch(issueKey: string, changes: Partial): void { + const current = this.state.get(issueKey); + if (current) { + this.state.set(issueKey, { ...current, ...changes }); + } + } } export function ticket(overrides: Partial = {}): JiraTicket { diff --git a/tests/helpers/fakeLogger.ts b/tests/helpers/fakeLogger.ts index 84d8d24..36cf2f7 100644 --- a/tests/helpers/fakeLogger.ts +++ b/tests/helpers/fakeLogger.ts @@ -2,7 +2,7 @@ import type { Logger } from '@map-colonies/js-logger'; import { vi } from 'vitest'; export interface RecordedLine { - level: 'info' | 'error'; + level: 'info' | 'warn' | 'error'; payload: Record; } @@ -18,7 +18,7 @@ export function fakeLogger(): { logger: Logger; lines: RecordedLine[] } { const logger = { info: record('info'), error: record('error'), - warn: vi.fn(), + warn: record('warn'), debug: vi.fn(), trace: vi.fn(), fatal: vi.fn(), diff --git a/tests/integration/cycle.spec.ts b/tests/integration/cycle.spec.ts index e945493..cded6c7 100644 --- a/tests/integration/cycle.spec.ts +++ b/tests/integration/cycle.spec.ts @@ -1,58 +1,160 @@ import { describe, expect, it } from 'vitest'; import { runCycle, type CycleDeps } from '@src/cycle'; -import { FakeJira, ticket } from '@tests/helpers/fakeJira'; +import { FakeJira, ticket, type FakeWrite } from '@tests/helpers/fakeJira'; import { fakeLogger, type RecordedLine } from '@tests/helpers/fakeLogger'; import type { WorkerConfig } from '@src/common/workerConfig'; +import type { JiraTransition } from '@src/jira/types'; + +const BOT_ACCOUNT = 'developer-agent@mapcolonies.example'; +const BOT_DISPLAY_NAME = 'AGENT DEVELOPER'; const baseConfig: WorkerConfig = { pollIntervalMs: 1000, maxTicketsPerRun: 1, maxConcurrentTickets: 1, mcpUrl: 'http://mcp.invalid', + bot: { 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. */ +function workflowFor(...keys: string[]): Record { + return Object.fromEntries( + keys.map((key) => [ + key, + [ + { id: '21', name: 'Start Progress', to: 'In Progress' }, + { id: '11', name: 'Reopen', to: 'Open' }, + ], + ]) + ); +} + function makeCycle(jira: FakeJira, overrides: Partial = {}): { deps: CycleDeps; lines: RecordedLine[] } { const { logger, lines } = fakeLogger(); return { deps: { jira, logger, config: { ...baseConfig, ...overrides } }, lines }; } +function kindsOf(writes: FakeWrite[]): string[] { + return writes.map((write) => write.kind); +} + +/** The order tickets appear in the write log, collapsed — `[a, b]` means a finished before b started. */ +function ticketRuns(writes: FakeWrite[]): string[] { + return writes.map((write) => write.key).filter((key, index, keys) => key !== keys[index - 1]); +} + describe('runCycle', () => { - it('should report what it found and that it started nothing', async () => { - const jira = new FakeJira({ tickets: [ticket({ key: 'MAPCO-100' })] }); + it('should claim the ticket it found and hand it straight back.', async () => { + const jira = new FakeJira({ tickets: [ticket({ key: 'MAPCO-100' })], transitions: workflowFor('MAPCO-100'), displayNames }); const { deps, lines } = makeCycle(jira); const result = await runCycle(deps); - expect(result).toMatchObject({ found: 1, started: 0, skipped: 1, outcome: 'ok' }); - expect(lines).toHaveLength(1); - expect(lines[0]?.payload).toMatchObject({ found: 1, started: 0, keys: ['MAPCO-100'] }); + expect(result).toMatchObject({ found: 1, started: 1, skipped: 0, outcome: 'ok' }); + expect(jira.writes).toEqual([ + { kind: 'assign', key: 'MAPCO-100', assignee: BOT_ACCOUNT }, + { kind: 'transition', key: 'MAPCO-100', transitionId: '21' }, + { kind: 'comment', key: 'MAPCO-100', body: expect.stringContaining('MAPCO-11431') as unknown as string }, + { kind: 'transition', key: 'MAPCO-100', transitionId: '11' }, + { kind: 'assign', key: 'MAPCO-100', assignee: null }, + ]); + expect(lines.at(-1)?.payload).toMatchObject({ found: 1, started: 1, keys: ['MAPCO-100'] }); }); - it('should write nothing to Jira.', async () => { - const jira = new FakeJira({ tickets: [ticket()] }); + it('should leave the ticket unassigned again, so the next run can pick it up.', async () => { + const jira = new FakeJira({ tickets: [ticket()], transitions: workflowFor('MAPCO-1'), displayNames }); const { deps } = makeCycle(jira); await runCycle(deps); - // The only thing this slice is permitted to do is ask. - expect(jira.queries).toHaveLength(1); + await expect(jira.getIssue('MAPCO-1')).resolves.toMatchObject({ assignee: null }); + }); + + it('should not touch a ticket that turns out to be assigned by the time it is reached.', async () => { + // The poll query filters `assignee is EMPTY`, so this is the snapshot-went-stale case + // rather than something the query would hand over — it exercises the guard before the write. + const jira = new FakeJira({ tickets: [ticket({ assignee: 'BROCHSTEIN RAZ' })], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps } = makeCycle(jira); + + const result = await runCycle(deps); + + expect(result).toMatchObject({ found: 1, started: 0, skipped: 1 }); + expect(jira.writes).toEqual([]); + }); + + it('should back off cleanly when a human claims the ticket mid-claim.', async () => { + const jira = new FakeJira({ + tickets: [ticket()], + transitions: workflowFor('MAPCO-1'), + displayNames, + stealOnAssign: 'BROCHSTEIN RAZ', + }); + const { deps, lines } = makeCycle(jira); + + const result = await runCycle(deps); + + expect(result).toMatchObject({ started: 0, skipped: 1, outcome: 'ok' }); + // The assign is already out there; the point is that nothing follows it — no transition, + // no comment, and no attempt to take it back off the human. + expect(kindsOf(jira.writes)).toEqual(['assign']); + expect(lines.some((line) => line.level === 'warn' && line.payload.reason === 'lost-race' && line.payload.saw === 'BROCHSTEIN RAZ')).toBe(true); + }); + + it('should report the transitions a workflow did offer when it cannot claim.', async () => { + // The real transition vocabulary is unverified, so a refusal has to say what it saw + // rather than leaving a silent no-op to be discovered by a drained queue. + const jira = new FakeJira({ tickets: [ticket()], transitions: { 'MAPCO-1': [{ id: '31', name: 'Reject', to: 'Rejected' }] }, displayNames }); + const { deps, lines } = makeCycle(jira); + + const result = await runCycle(deps); + + expect(result).toMatchObject({ started: 0, skipped: 1 }); + expect(lines.some((line) => line.payload.reason === 'no-transition' && (line.payload.offered as string[])[0] === 'Reject')).toBe(true); }); it('should honour the per-run ticket limit and say there is more waiting.', async () => { - const jira = new FakeJira({ tickets: [ticket({ key: 'MAPCO-1' }), ticket({ key: 'MAPCO-2' }), ticket({ key: 'MAPCO-3' })] }); - const { deps } = makeCycle(jira, { maxTicketsPerRun: 2 }); + const keys = ['MAPCO-1', 'MAPCO-2', 'MAPCO-3']; + const jira = new FakeJira({ tickets: keys.map((key) => ticket({ key })), transitions: workflowFor(...keys), displayNames }); + const { deps } = makeCycle(jira, { maxTicketsPerRun: 2, maxConcurrentTickets: 2 }); const result = await runCycle(deps); expect(result.found).toBe(2); + expect(result.started).toBe(2); expect(result.more).toBe(true); // One over the limit, because the server's `total` is always -1 and cannot be trusted. expect(jira.queries[0]?.limit).toBe(3); + expect(jira.writes.some((write) => write.key === 'MAPCO-3')).toBe(false); + }); + + it('should hold tickets to one at a time at the default concurrency.', async () => { + const keys = ['MAPCO-1', 'MAPCO-2']; + const jira = new FakeJira({ tickets: keys.map((key) => ticket({ key })), transitions: workflowFor(...keys), displayNames }); + const { deps } = makeCycle(jira, { maxTicketsPerRun: 2, maxConcurrentTickets: 1 }); + + await runCycle(deps); + + // Each ticket is finished with before the next is touched: the cap is what stops the + // worker holding a whole page of tickets at once. + expect(ticketRuns(jira.writes)).toEqual(['MAPCO-1', 'MAPCO-2']); + }); + + it('should overlap tickets once concurrency is raised.', async () => { + const keys = ['MAPCO-1', 'MAPCO-2']; + const jira = new FakeJira({ tickets: keys.map((key) => ticket({ key })), transitions: workflowFor(...keys), displayNames }); + const { deps } = makeCycle(jira, { maxTicketsPerRun: 2, maxConcurrentTickets: 2 }); + + await runCycle(deps); + + // Interleaved rather than one-then-the-other, which is the difference the cap makes. + expect(ticketRuns(jira.writes).length).toBeGreaterThan(2); }); it('should not claim there is more waiting when the queue is exhausted.', async () => { - const jira = new FakeJira({ tickets: [ticket()] }); + const jira = new FakeJira({ tickets: [ticket()], transitions: workflowFor('MAPCO-1'), displayNames }); const { deps } = makeCycle(jira); expect((await runCycle(deps)).more).toBe(false); @@ -64,7 +166,7 @@ describe('runCycle', () => { const result = await runCycle(deps); - expect(result).toMatchObject({ found: 0, outcome: 'ok' }); + expect(result).toMatchObject({ found: 0, started: 0, outcome: 'ok' }); expect(lines[0]?.level).toBe('info'); }); @@ -77,4 +179,32 @@ describe('runCycle', () => { expect(result.outcome).toBe('failed'); expect(lines[0]?.level).toBe('error'); }); + + it('should keep hold of a ticket it cannot return to Open, and say so loudly.', async () => { + const jira = new FakeJira({ + tickets: [ticket()], + transitions: { 'MAPCO-1': [{ id: '21', name: 'Start Progress', to: 'In Progress' }] }, + displayNames, + }); + const { deps, lines } = makeCycle(jira); + + const result = await runCycle(deps); + + // It did hold the ticket, so it counts as started — but it is now stuck, and the run + // line has to make that findable. + expect(result).toMatchObject({ started: 1, outcome: 'ok' }); + await expect(jira.getIssue('MAPCO-1')).resolves.toMatchObject({ assignee: BOT_DISPLAY_NAME }); + expect(lines.some((line) => line.level === 'error' && line.payload.msg === 'held ticket could not be released')).toBe(true); + }); + + it('should let one broken ticket fail without taking the run down with it.', async () => { + const keys = ['MAPCO-1', 'MAPCO-2']; + const jira = new FakeJira({ tickets: keys.map((key) => ticket({ key })), transitionsFailWith: new Error('jira exploded'), displayNames }); + const { deps, lines } = makeCycle(jira, { maxTicketsPerRun: 2 }); + + const result = await runCycle(deps); + + expect(result).toMatchObject({ found: 2, started: 0, skipped: 2, outcome: 'ok' }); + expect(lines.filter((line) => line.payload.msg === 'ticket failed')).toHaveLength(2); + }); }); diff --git a/tests/unit/jira/mcpJira.spec.ts b/tests/unit/jira/mcpJira.spec.ts index 4855ce0..80afe64 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 { toTicket } from '@src/jira/mcpJira'; +import { assigneeFields, toTicket, toTransition } from '@src/jira/mcpJira'; describe('toTicket', () => { it('should read an unassigned ticket as unclaimed, not as assigned to someone called Unassigned.', () => { @@ -44,3 +44,35 @@ describe('toTicket', () => { }); }); }); + +describe('assigneeFields', () => { + it('should send the assignee as a JSON string, because that is what the tool takes.', () => { + // `fields` is a *stringified* object on this API, not an object. Passing an object + // silently updates nothing, which would make a claim look like it succeeded. + expect(assigneeFields('developer-agent@mapcolonies.example')).toBe('{"assignee":"developer-agent@mapcolonies.example"}'); + }); + + it('should send an explicit null to unassign, not an omitted field.', () => { + expect(assigneeFields(null)).toBe('{"assignee":null}'); + }); +}); + +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" — + // so the target status is the only reliable way to ask for "get me to In Progress". + expect(toTransition({ id: 21, name: 'Start Progress', to: { name: 'In Progress' } })).toStrictEqual({ + id: '21', + name: 'Start Progress', + to: 'In Progress', + }); + }); + + it('should accept a target status reported as a bare string.', () => { + expect(toTransition({ id: '11', name: 'Reopen', to: 'Open' }).to).toBe('Open'); + }); + + it('should leave the target status absent when the server does not report one.', () => { + expect(toTransition({ id: '11', name: 'Open' }).to).toBeUndefined(); + }); +}); diff --git a/tests/unit/scheduler.spec.ts b/tests/unit/scheduler.spec.ts index 7e0d8ca..9e33c7b 100644 --- a/tests/unit/scheduler.spec.ts +++ b/tests/unit/scheduler.spec.ts @@ -4,7 +4,22 @@ import { FakeJira, ticket } from '@tests/helpers/fakeJira'; import { fakeLogger } from '@tests/helpers/fakeLogger'; import type { WorkerConfig } from '@src/common/workerConfig'; -const config: WorkerConfig = { pollIntervalMs: 1000, maxTicketsPerRun: 1, maxConcurrentTickets: 1, mcpUrl: 'http://mcp.invalid' }; +/** A claimable ticket, so these timing tests exercise a real cycle rather than a refusal. */ +const displayNames = { 'developer-agent@mapcolonies.example': 'AGENT DEVELOPER' }; +const workflow = { + 'MAPCO-1': [ + { id: '21', name: 'Start Progress', to: 'In Progress' }, + { id: '11', name: 'Reopen', to: 'Open' }, + ], +}; + +const config: WorkerConfig = { + pollIntervalMs: 1000, + maxTicketsPerRun: 1, + maxConcurrentTickets: 1, + mcpUrl: 'http://mcp.invalid', + bot: { account: 'developer-agent@mapcolonies.example', displayName: 'AGENT DEVELOPER' }, +}; describe('createScheduler', () => { beforeEach(() => { @@ -16,7 +31,7 @@ describe('createScheduler', () => { }); it('should run a cycle immediately and again after the interval.', async () => { - const jira = new FakeJira({ tickets: [ticket()] }); + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow, displayNames }); const { logger } = fakeLogger(); const scheduler = createScheduler({ jira, logger, config }, 1000, logger); diff --git a/tests/unit/tickets/claim.spec.ts b/tests/unit/tickets/claim.spec.ts new file mode 100644 index 0000000..eb790d5 --- /dev/null +++ b/tests/unit/tickets/claim.spec.ts @@ -0,0 +1,128 @@ +import { describe, expect, it } from 'vitest'; +import { claimTicket, releaseTicket } from '@src/tickets/claim'; +import { FakeJira, ticket } from '@tests/helpers/fakeJira'; +import type { BotIdentity } from '@src/tickets/claim'; + +/** + * The two halves of the bot's identity. They differ on purpose: a write takes an + * identifier, a read comes back as a surname-first display name. + */ +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' }, + ], +}; + +describe('claimTicket', () => { + it('should claim a ready ticket by assigning the bot and moving it to In Progress.', async () => { + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow, displayNames }); + + const outcome = await claimTicket(ticket(), jira, bot); + + expect(outcome).toEqual({ ok: true }); + expect(jira.writes).toEqual([ + { kind: 'assign', key: 'MAPCO-1', assignee: BOT_ACCOUNT }, + { kind: 'transition', key: 'MAPCO-1', transitionId: '21' }, + ]); + }); + + it('should find the transition by the status it lands in, not by its own name.', async () => { + // The real reason this matters: a workflow whose transitions are verbs would otherwise + // never match, and every claim would refuse for a reason that looks like a workflow + // problem rather than a bug here. + const jira = new FakeJira({ + tickets: [ticket()], + transitions: { 'MAPCO-1': [{ id: '77', name: 'Begin working on this', to: 'In Progress' }] }, + displayNames, + }); + + await expect(claimTicket(ticket(), jira, bot)).resolves.toEqual({ ok: true }); + expect(jira.writes).toContainEqual({ kind: 'transition', key: 'MAPCO-1', transitionId: '77' }); + }); + + it('should still match on the transition name when the server reports no target status.', async () => { + const jira = new FakeJira({ tickets: [ticket()], transitions: { 'MAPCO-1': [{ id: '21', name: 'In Progress' }] }, displayNames }); + + await expect(claimTicket(ticket(), jira, bot)).resolves.toEqual({ ok: true }); + }); + + it('should never pick up a ticket someone already holds, and write nothing at all.', async () => { + const held = ticket({ assignee: 'BROCHSTEIN RAZ' }); + const jira = new FakeJira({ tickets: [held], transitions: workflow, displayNames }); + + const outcome = await claimTicket(held, jira, bot); + + expect(outcome).toEqual({ ok: false, reason: 'already-assigned' }); + expect(jira.writes).toEqual([]); + }); + + it('should back off when the re-read shows a human got there first, writing nothing further.', async () => { + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow, displayNames, stealOnAssign: 'BROCHSTEIN RAZ' }); + + const outcome = await claimTicket(ticket(), jira, bot); + + // `saw` is what makes a misconfigured JIRA_BOT_DISPLAY_NAME tell itself apart from a + // genuine race: the same refusal, but the name in it is the bot's own. + expect(outcome).toEqual({ ok: false, reason: 'lost-race', saw: 'BROCHSTEIN RAZ' }); + // The assignee write is already out there and is not ours to undo — the human owns the + // field now, so unwinding it would take the ticket off them. + expect(jira.writes).toEqual([{ kind: 'assign', key: 'MAPCO-1', assignee: BOT_ACCOUNT }]); + }); + + it('should treat a ticket that vanished between the write and the re-read as lost.', async () => { + const jira = new FakeJira({ transitions: workflow, displayNames }); + + await expect(claimTicket(ticket(), jira, bot)).resolves.toEqual({ ok: false, reason: 'lost-race', saw: null }); + }); + + it('should refuse before writing when the workflow offers no way into In Progress.', async () => { + const jira = new FakeJira({ tickets: [ticket()], transitions: { 'MAPCO-1': [{ id: '31', name: 'Reject', to: 'Rejected' }] }, displayNames }); + + const outcome = await claimTicket(ticket(), jira, bot); + + // The offered names are reported so the first run against a real workflow says what it + // was actually given, instead of refusing every ticket in silence. + expect(outcome).toEqual({ ok: false, reason: 'no-transition', offered: ['Reject'] }); + // Looked before it leapt: refusing after the assign would leave a ticket held by a bot + // that cannot start it. + expect(jira.writes).toEqual([]); + }); +}); + +describe('releaseTicket', () => { + const held = (): ReturnType => ticket({ assignee: BOT_DISPLAY_NAME, status: 'In Progress' }); + + it('should hand a ticket back by commenting, returning it to Open, then unassigning.', async () => { + const jira = new FakeJira({ tickets: [held()], transitions: workflow, displayNames }); + + const outcome = await releaseTicket(held(), 'Tried nothing, and it did not work.', jira); + + expect(outcome).toEqual({ ok: true }); + // Order is the whole design: unassigning last means a failure part-way through leaves + // the ticket held by the bot — invisible to the poll query — rather than unassigned and + // still In Progress, which the query would happily hand straight back. + expect(jira.writes).toEqual([ + { kind: 'comment', key: 'MAPCO-1', body: 'Tried nothing, and it did not work.' }, + { kind: 'transition', key: 'MAPCO-1', transitionId: '11' }, + { kind: 'assign', key: 'MAPCO-1', assignee: null }, + ]); + }); + + it('should keep holding a ticket it cannot return to Open, rather than unassigning it into a re-claim loop.', async () => { + const jira = new FakeJira({ tickets: [held()], transitions: { 'MAPCO-1': [{ id: '31', name: 'Reject', to: 'Rejected' }] }, displayNames }); + + const outcome = await releaseTicket(held(), 'Tried nothing.', jira); + + expect(outcome).toEqual({ ok: false, reason: 'no-transition', offered: ['Reject'] }); + // Still the bot's problem, which is what makes the orphan sweep on boot able to find it. + expect(jira.writes.some((write) => write.kind === 'assign')).toBe(false); + }); +}); diff --git a/tests/unit/workerConfig.spec.ts b/tests/unit/workerConfig.spec.ts index 5a9fa6c..1be61b6 100644 --- a/tests/unit/workerConfig.spec.ts +++ b/tests/unit/workerConfig.spec.ts @@ -2,7 +2,11 @@ import { describe, expect, it } from 'vitest'; import { ConfigError, loadWorkerConfig } from '@src/common/workerConfig'; -const minimal = { MCP_ATLASSIAN_URL: 'http://mcp-atlassian:8080/mcp' }; +const minimal = { + MCP_ATLASSIAN_URL: 'http://mcp-atlassian:8080/mcp', + JIRA_BOT_ACCOUNT: 'developer-agent@mapcolonies.example', + JIRA_BOT_DISPLAY_NAME: 'AGENT DEVELOPER', +}; describe('loadWorkerConfig', () => { it('should default both concurrency knobs to 1.', () => { @@ -28,4 +32,20 @@ describe('loadWorkerConfig', () => { expect(config.maxTicketsPerRun).toBe(3); expect(config.pollIntervalMs).toBe(60000); }); + + it('should refuse to start without a bot identity, since it could not tell its own claims apart.', () => { + const { JIRA_BOT_ACCOUNT, ...noAccount } = minimal; + const { JIRA_BOT_DISPLAY_NAME, ...noDisplayName } = minimal; + + expect(() => loadWorkerConfig(noAccount)).toThrow(ConfigError); + expect(() => loadWorkerConfig(noDisplayName)).toThrow(ConfigError); + }); + + it('should keep the written identifier and the display name it reads back as separate.', () => { + const config = loadWorkerConfig(minimal); + + // Two knobs, not one: a write takes an email or accountId, a read returns a + // surname-first display name, and neither can be derived from the other. + expect(config.bot).toStrictEqual({ account: 'developer-agent@mapcolonies.example', displayName: 'AGENT DEVELOPER' }); + }); }); From a88de873002a7f62c095b60a8286ee59cec9d964 Mon Sep 17 00:00:00 2001 From: razbroc Date: Thu, 20 Aug 2026 17:50:38 +0300 Subject: [PATCH 2/2] ci: take the author-assign token from the github context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The secrets-context fallback in 908f19a still resolved to an empty input, so the action kept failing with 'Input required and not supplied: repo-token' — neither GH_PAT nor secrets.GITHUB_TOKEN came through. github.token is always populated, and the job already holds the pull-requests: write permission. This only takes effect for pull requests whose base branch carries it, because pull_request_target resolves the workflow from the base. Refs: MAPCO-11431 --- .github/workflows/auto-author-assign.yml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/workflows/auto-author-assign.yml b/.github/workflows/auto-author-assign.yml index e8ce2c5..43da753 100644 --- a/.github/workflows/auto-author-assign.yml +++ b/.github/workflows/auto-author-assign.yml @@ -13,6 +13,8 @@ jobs: steps: - uses: toshimaru/auto-author-assign@v3.1.0 with: - # Falls back to the built-in token: GH_PAT is not set on this repo, and the job - # already has the pull-requests: write permission it needs. - repo-token: ${{ secrets.GH_PAT || secrets.GITHUB_TOKEN }} + # `github.token` rather than `secrets.GITHUB_TOKEN`: the secrets-context fallback + # added in 908f19a still resolved to an empty input, so the action failed with + # 'Input required and not supplied'. The github context is always populated, and + # the job already holds the pull-requests: write permission the action needs. + repo-token: ${{ github.token }}