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 }} 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' }); + }); });