diff --git a/docs/decisions/adr-jira-write-plane.md b/docs/decisions/adr-jira-write-plane.md index fac3d22..3c3e912 100644 --- a/docs/decisions/adr-jira-write-plane.md +++ b/docs/decisions/adr-jira-write-plane.md @@ -122,32 +122,51 @@ dropped connection, or a create Jira accepted without naming — is reported as ## Where plans live -**In the server process, in memory, for ten minutes.** - -The alternative was a signed self-contained token: the plan travels through the -agent and comes back, verified by a signature. It loses on both counts that -matter here. - -The signing key has to come from somewhere. A key on disk is new secret -material to protect, for a feature whose whole point is not handling secrets -carelessly. A per-process key gives the token exactly the lifetime an in-memory -map already has, with more code and a larger attack surface. - -And the mutation itself would have to leave the process. Keeping it in memory -makes forgery impossible rather than computationally hard: `planId` is an -opaque handle, and what it names is never serialized anywhere an agent can -reach. A plan is also single-use, so an applied receipt cannot be replayed into -a second comment. - -The cost is that plans do not survive a restart, and are not shared between -concurrent JAM processes. Both are acceptable. A plan is only valid while the -issue has not moved, so a plan old enough to be affected was going to be -rejected on its own terms; and re-planning is one read. - -If plans ever need to outlive a process — a remote JAM, or a queued approval -step — this is the decision to revisit, and a signed token becomes the obvious -shape. Nothing in the tool contract changes if it does: an agent already treats -`planId` as opaque. +**In a file under `~/.jam/write-plans/`, for ten minutes, shared by both +transports.** + +This was in memory, in the server process, and the reason it moved is the +condition this section named for revisiting: plans had to outlive a process. + +`jam jira write-plan` and `jam jira write-apply` are two invocations. An +in-memory plan is gone before apply can see it, so writing from the shell was +impossible — and writing from the shell is what a session needs when its MCP +registry came up in a failed state and the tools cannot be re-registered +without restarting it. Sharing one store between MCP and the CLI also means a +plan made through the tools can be applied from the shell after they stop +answering, which is the case the whole change exists for. + +A signed token was still not the shape. The signing key has the same problem it +had before: a key on disk is new secret material to protect, and a per-process +key gives a token the lifetime an in-memory map already had. JAM does hold an +OS secret store, but its contract is the Jira credential triple, and widening +it to carry a signing key is a larger change than the one being made. + +**What replaces the in-process guarantee is re-derivation, not a signature.** +Apply reads the plan's own recorded input, derives the mutation from it again, +and refuses (`JAM_WRITE_PLAN_TAMPERED`) if the stored mutation is not what that +input produces. Editing the file to smuggle a different write means editing the +input to match — which is asking JAM to plan that write, which is what +`jira_write_plan` is. There is nothing to gain by forging a plan that could be +obtained by requesting one. + +Re-derivation reads the plan's recorded resolutions rather than asking Jira +again. Planning called Jira for a transition list and a user directory and +wrote down what it settled on; re-running those would cost a round trip and +would re-raise refusals that belong to planning ("already assigned", "not +assignable"), reporting a fact about the current state as though the plan were +malformed. What is checked is narrower and sufficient: the mutation must be the +one this plan's own parts describe. + +Single use is now enforced by the filesystem. `take` claims a plan by renaming +it, so two concurrent applies cannot both hold one — for `comment.add` the +alternative is two comments. + +The cost is that a mutation is now serialized where the machine's user can read +and edit it. That is a real reduction from "forgery is impossible" to "forgery +gains nothing", and it is the price of a write path that survives its +transport. Plans are still short-lived, still single-use, and still only valid +while the issue has not moved. ## Consequences diff --git a/packages/server/src/application/apply-create-issue.ts b/packages/server/src/application/apply-create-issue.ts index 98003ba..15d506f 100644 --- a/packages/server/src/application/apply-create-issue.ts +++ b/packages/server/src/application/apply-create-issue.ts @@ -3,8 +3,9 @@ import type { FullIssueContext } from "../domain/context.js"; import { JamError, toJamError } from "../domain/errors.js"; import type { CreateIssueInput, CreateIssueWritePlan, WriteApplyReceipt } from "../domain/write.js"; import { canonicalizePlainText } from "../domain/adf.js"; -import { assertSchemaUnchanged } from "../policy/create-policy.js"; +import { assertSchemaUnchanged, CREATE_FIELD_IDS } from "../policy/create-policy.js"; import { projectKeyOf } from "../policy/write-policy.js"; +import { toJiraCreateFields, validateCreateInput } from "./plan-create-issue.js"; import { readIssue } from "./plan-write.js"; /** @@ -32,6 +33,12 @@ export async function applyCreateIssue( throw new JamError("CONFIG_INVALID", "A create-issue plan must carry a create mutation."); } + // Plans come off disk now, so the fields being sent are checked against what + // this plan's own input produces before anything is created. Editing the + // stored mutation without editing the input is caught here; editing both is + // asking JAM to plan that create, which is the supported way to get one. + assertCreateMutationMatchesInput(plan); + await revalidateSchema(deps, plan); const created = await create(deps, plan); @@ -58,6 +65,61 @@ export async function applyCreateIssue( * not "is the schema identical" - on an active project it rarely is - but "are * this plan's premises still true". See assertSchemaUnchanged. */ +/** + * Check the stored create fields are the ones this plan's input produces. + * + * The existing-issue path does the same thing in apply-write.ts, and for the + * same reason: a plan is a file, and re-deriving is what makes editing it + * pointless. Creation can do it without touching Jira - `toJiraCreateFields` + * is pure once the issue type and resolved values are known, and those are in + * `schemaRequirements`, which `revalidateSchema` checks against Jira right + * after this. + * + * Nothing has been created when this refuses. + */ +function assertCreateMutationMatchesInput(plan: CreateIssueWritePlan): void { + const { schemaRequirements: schema } = plan; + // One entry per requested value, so a component list resolves item by item - + // matching on fieldId alone would give every component the first one's answer. + const resolvedFor = (fieldId: string, requested: string): string | undefined => + schema.resolvedValues.find((entry) => entry.fieldId === fieldId && entry.requested === requested) + ?.resolved; + + let derived: unknown; + try { + const input = validateCreateInput(plan.input); + const priority = + input.priority !== undefined + ? resolvedFor(CREATE_FIELD_IDS.priority, input.priority) + : undefined; + derived = toJiraCreateFields(plan.projectKey, schema.issueTypeId, input, { + ...(priority !== undefined ? { priority } : {}), + ...(input.components !== undefined + ? { + components: input.components.map( + (name) => resolvedFor(CREATE_FIELD_IDS.components, name) ?? name, + ), + } + : {}), + }); + } catch (err) { + throw new JamError( + "JAM_WRITE_PLAN_TAMPERED", + `This create plan does not describe an issue JAM would create in ${plan.projectKey}. Nothing was created - call jira_write_plan again.`, + { planId: plan.planId, project: plan.projectKey, reason: toJamError(err).code }, + ); + } + + const recorded = plan.mutation.kind === "create" ? plan.mutation.fields : undefined; + if (JSON.stringify(derived) !== JSON.stringify(recorded)) { + throw new JamError( + "JAM_WRITE_PLAN_TAMPERED", + `This create plan's recorded fields do not match what its input produces. Nothing was created - call jira_write_plan again.`, + { planId: plan.planId, project: plan.projectKey }, + ); + } +} + async function revalidateSchema(deps: JamDeps, plan: CreateIssueWritePlan): Promise { const { projectKey, schemaRequirements } = plan; const issueTypes = await deps.jiraCreateMetadata.getIssueTypes(projectKey); diff --git a/packages/server/src/application/apply-write.ts b/packages/server/src/application/apply-write.ts index e21397c..061b073 100644 --- a/packages/server/src/application/apply-write.ts +++ b/packages/server/src/application/apply-write.ts @@ -1,12 +1,18 @@ import type { JamDeps } from "../deps.js"; import type { FullIssueContext } from "../domain/context.js"; import { JamError, toJamError } from "../domain/errors.js"; -import type { ExistingIssueWritePlan, WriteApplyReceipt } from "../domain/write.js"; +import type { + ExistingIssueWritePlan, + FieldUpdateInput, + WriteApplyReceipt, + WriteInput, + WriteMutation, +} from "../domain/write.js"; import { readModeAfterWrite } from "../policy/consistency-policy.js"; import { assertAssignable } from "../policy/assignee-policy.js"; import { assertSameIssue, assertUnchanged } from "../policy/write-policy.js"; import { applyCreateIssue } from "./apply-create-issue.js"; -import { readIssue } from "./plan-write.js"; +import { readIssue, toJiraFields, validateInput } from "./plan-write.js"; export type ApplyWriteRequest = { planId: string }; @@ -53,6 +59,12 @@ export async function applyWritePlan( assertSameIssue(plan.issueKey, plan.issueId, current.issueId); assertUnchanged(plan.issueKey, plan.baseUpdated, current.issue.updated); + // The plan came off disk, so derive its mutation again and check it is the + // one stored. This is after the revision check on purpose: a moved issue is + // a conflict, not tampering, and saying so in that order keeps the two + // situations from being reported as each other. + assertMutationMatchesInput(plan); + // Whatever the plan depends on that the revision check cannot see, checked // again here. For an assignment that is the target's permission to hold this // issue: it can be revoked between planning and applying, and a plan that @@ -77,6 +89,91 @@ export async function applyWritePlan( }; } +/** + * Check the stored mutation is the one this plan's input produces. + * + * Plans used to live in the process that made them, so what apply sent could + * only have come from planning. They live in a file now - shared so that a + * session whose MCP channel died can still apply one from the shell - and a + * file can be edited. + * + * What replaces the in-process guarantee is this: run the same derivation + * planning ran, on the plan's own input, and refuse if the answer differs. + * Editing `mutation` alone is caught here. Editing `input` to match means + * asking JAM to derive that mutation, which is what `jira_write_plan` is - so + * there is nothing to gain by forging a plan that could not be obtained by + * asking for one. + * + * Nothing has been sent to Jira when this refuses. + */ +function assertMutationMatchesInput(plan: ExistingIssueWritePlan): void { + let derived: WriteMutation; + try { + // Through validateInput first: the whitelist is part of the derivation, so + // a plan carrying a field JAM does not write is refused here rather than + // sent. Re-deriving without it would let an edited input past the check + // planning applied to it. + const input = validateInput(plan.operation, plan.input); + derived = deriveMutation(plan, input); + } catch (err) { + // A plan whose input no longer derives anything is not a plan. Say that, + // rather than letting the original refusal read as a fresh request being + // rejected. + throw new JamError( + "JAM_WRITE_PLAN_TAMPERED", + `This write plan does not describe a change JAM would make for ${plan.issueKey}. Nothing was written - call jira_write_plan again.`, + { planId: plan.planId, issueKey: plan.issueKey, reason: toJamError(err).code }, + ); + } + + if (JSON.stringify(derived) !== JSON.stringify(plan.mutation)) { + throw new JamError( + "JAM_WRITE_PLAN_TAMPERED", + `This write plan's recorded change does not match what its input produces for ${plan.issueKey}. Nothing was written - call jira_write_plan again.`, + { planId: plan.planId, issueKey: plan.issueKey }, + ); + } +} + +/** + * The mutation this plan's input produces, without asking Jira anything. + * + * Planning calls Jira for two of these - the transition list, the user + * directory - and records what it settled on. Re-deriving here reads those + * recorded answers rather than fetching them again, for two reasons: apply + * would otherwise pay a round trip it does not need, and planning's lookups + * carry refusals that belong to planning (`already assigned`, `not + * assignable`). Running those a second time would report a check on the + * current state as if the plan were malformed. + * + * What is being checked is narrower and enough: the mutation must be the one + * this plan's own parts describe. An edit to `mutation` alone is caught. An + * edit that also rewrites the input and the recorded resolution is a different + * plan, obtainable by asking for one - and Jira still has to accept it. + */ +function deriveMutation(plan: ExistingIssueWritePlan, input: WriteInput): WriteMutation { + switch (plan.operation) { + case "comment.add": + return { kind: "comment", text: (input as { text: string }).text }; + case "field.update": + return { kind: "fields", fields: toJiraFields(input as FieldUpdateInput) }; + case "status.transition": { + // The id came from Jira at plan time and is recorded; what is verified is + // that the plan still points at its own resolution. + const transition = plan.transition; + if (!transition) throw new JamError("CONFIG_INVALID", "A transition plan must record its transition."); + return { kind: "transition", transitionId: transition.id }; + } + case "assignee.update": { + const target = plan.intendedAfter["assignee"] as { accountId?: string } | undefined; + if (!target?.accountId) { + throw new JamError("CONFIG_INVALID", "An assignment plan must record who it resolved to."); + } + return { kind: "assignee", accountId: target.accountId }; + } + } +} + /** * Re-derive the premises the revision check does not cover. * diff --git a/packages/server/src/application/plan-create-issue.ts b/packages/server/src/application/plan-create-issue.ts index dae93a0..9ee272f 100644 --- a/packages/server/src/application/plan-create-issue.ts +++ b/packages/server/src/application/plan-create-issue.ts @@ -111,6 +111,7 @@ export async function planCreateIssue( kind: "create", fields: toJiraCreateFields(projectKey, issueType.id, input, { priority, components }), }, + input: request.input, }) as CreateIssueWritePlan; return { @@ -217,7 +218,13 @@ function notAllowed(message: string): JamError { } /** Whitelisted values to the shapes Jira's create API expects. */ -function toJiraCreateFields( +/** + * Exported because apply derives these again and compares - see + * `assertCreateMutationMatchesInput` in apply-create-issue.ts. Planning and + * applying must produce the same fields from the same input, so they call one + * function rather than two that are supposed to agree. + */ +export function toJiraCreateFields( projectKey: string, issueTypeId: string, input: CreateIssueInput, diff --git a/packages/server/src/application/plan-write.ts b/packages/server/src/application/plan-write.ts index 3cc2034..c8cb091 100644 --- a/packages/server/src/application/plan-write.ts +++ b/packages/server/src/application/plan-write.ts @@ -98,6 +98,7 @@ export async function planWrite( ...(transition ? { transition } : {}), ...(baseAssigneeAccountId ? { baseAssigneeAccountId } : {}), mutation, + input: input as Record, }) as ExistingIssueWritePlan; return { @@ -221,7 +222,10 @@ function requireIssueKey(request: PlanWriteRequest): string { * is writable and whether the input is even the right shape are all knowable * without asking Jira anything, so they are answered first. */ -function validateInput(operation: ExistingIssueOperation, raw: Record): WriteInput { +export function validateInput( + operation: ExistingIssueOperation, + raw: Record, +): WriteInput { switch (operation) { case "comment.add": { const text = (raw as CommentAddInput).text; @@ -390,7 +394,8 @@ function currentValue(issue: FullIssueContext, field: string): unknown { } /** Whitelisted values to the shapes Jira's field API expects. */ -function toJiraFields(input: FieldUpdateInput): Record { +/** Exported so apply can derive the same fields again and compare. */ +export function toJiraFields(input: FieldUpdateInput): Record { const fields: Record = {}; if (input.summary !== undefined) fields["summary"] = input.summary; if (input.priority !== undefined) fields["priority"] = { name: input.priority }; diff --git a/packages/server/src/application/write-plan-store.ts b/packages/server/src/application/write-plan-store.ts index b0db637..3737fe1 100644 --- a/packages/server/src/application/write-plan-store.ts +++ b/packages/server/src/application/write-plan-store.ts @@ -1,56 +1,115 @@ import { randomUUID } from "node:crypto"; +import { + chmodSync, + existsSync, + mkdirSync, + readFileSync, + readdirSync, + renameSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { homedir } from "node:os"; +import { join } from "node:path"; import { JamError } from "../domain/errors.js"; import type { NewWritePlan, WritePlan } from "../domain/write.js"; import { planExpired } from "../policy/write-policy.js"; /** - * Where a plan lives between `jira_write_plan` and `jira_write_apply`. + * Where a plan lives between planning and applying. * - * In this process, and nowhere else. The alternative considered was a signed - * self-contained token, and it is worse here on both counts that matter: the - * signing key would have to come from somewhere (a new secret on disk, or a - * per-process key that gives the token exactly this lifetime anyway), and the - * mutation would have to travel through the agent to come back. Keeping the - * mutation in memory makes forgery impossible rather than merely hard - a - * `planId` is an opaque handle, and what it names never leaves this process. + * It used to live in the server process and nowhere else, and the ADR said why: + * a signed token needs a signing key from somewhere, and keeping the mutation + * in memory made forgery impossible rather than merely hard. * - * The cost is that plans do not survive a restart. That is acceptable: a plan - * is only valid while the issue has not moved, so a stale one was going to be - * rejected on its own terms, and re-planning is a single read. + * The same ADR named the condition for revisiting that - "if plans ever need to + * outlive a process". They do now. `jam jira write-plan` and `jam jira + * write-apply` are two processes, so an in-memory plan is gone before apply can + * see it, and a session whose MCP registry is stale has no way to write at all. + * + * So a plan is a file under `~/.jam/write-plans/`, shared by both transports. + * MCP and CLI read the same store, which means a plan made through the tools + * can be applied from the shell when the tools stop answering - that is the + * whole point of the change. + * + * What replaces the in-process guarantee is in apply, not here: it re-derives + * the mutation from the plan's own inputs and refuses if the stored mutation + * disagrees. Editing the file to smuggle a different write requires editing the + * inputs to match, which is just calling plan again. See apply-write.ts. * * See docs/decisions/adr-jira-write-plane.md. */ -export class WritePlanStore { - private readonly plans = new Map(); +/** One plan, one file. The suffix is the claim: see `take`. */ +const PENDING = ".json"; +const CLAIMED = ".claimed"; + +export type WritePlanStoreOptions = { /** Injected by tests so expiry does not depend on wall-clock timing. */ - constructor(private readonly now: () => Date = () => new Date()) {} + now?: () => Date; + /** Injected by tests so no suite writes into the real `~/.jam`. */ + root?: string; +}; + +export class WritePlanStore { + private readonly now: () => Date; + private readonly root: string; + + constructor(options: WritePlanStoreOptions | (() => Date) = {}) { + // The old signature took `now` positionally and tests still use it. + const opts = typeof options === "function" ? { now: options } : options; + this.now = opts.now ?? (() => new Date()); + this.root = opts.root ?? join(homedir(), ".jam", "write-plans"); + } create(plan: NewWritePlan): WritePlan { this.evictExpired(); const stored: WritePlan = { ...plan, planId: randomUUID() } as WritePlan; - this.plans.set(stored.planId, stored); + this.ensureRoot(); + const path = this.pathFor(stored.planId, PENDING); + writeFileSync(path, JSON.stringify(stored), "utf8"); + // Owner-only. A plan names an issue and what would be written to it, and + // nothing else on the machine needs to read that. + chmodSync(path, 0o600); return stored; } /** - * Resolve a plan for applying. + * Resolve a plan for applying, and claim it in the same step. + * + * The claim is a rename, because two processes can now reach the same plan + * and `rename` is the only thing here that is atomic. Whoever renames it owns + * the apply; everyone else sees the file already gone and is told there is no + * such plan - which is the truth, for them. + * + * Reading and then deleting would leave a window between the two, and for + * `comment.add` that window is a second comment. * * An expired plan is reported as expired rather than as missing: those are * different situations, and telling them apart is the difference between * "re-plan" and "you are calling this wrong". */ take(planId: string): WritePlan { - const plan = this.plans.get(planId); - if (!plan) { - throw new JamError( - "JAM_WRITE_PLAN_NOT_FOUND", - "No such write plan. Plans live in the running JAM server and do not survive a restart - call jira_write_plan again.", - { planId }, - ); + const pending = this.pathFor(planId, PENDING); + const claimed = this.pathFor(planId, CLAIMED); + try { + renameSync(pending, claimed); + } catch { + throw this.notFound(planId); } + + let plan: WritePlan; + try { + plan = JSON.parse(readFileSync(claimed, "utf8")) as WritePlan; + } catch { + // Unreadable is not "someone else has it" - the file is ours now and it + // is unusable, so retire it rather than leaving it to expire. + rmSync(claimed, { force: true }); + throw this.notFound(planId); + } + if (planExpired(plan.expiresAt, this.now())) { - this.plans.delete(planId); + rmSync(claimed, { force: true }); // What to re-plan against differs by plan: an existing issue has a // current state, a create has only the project's current create schema. // Naming an issue key here for a create would name an issue that has @@ -77,16 +136,53 @@ export class WritePlanStore { * * Single use, so a receipt cannot be turned into a second mutation by * calling apply again with the same id - which for `comment.add` would mean - * two comments. + * two comments. `take` already made that true by claiming the file; this + * removes the claim so it does not sit until expiry. */ consume(planId: string): void { - this.plans.delete(planId); + rmSync(this.pathFor(planId, CLAIMED), { force: true }); + } + + private notFound(planId: string): JamError { + return new JamError( + "JAM_WRITE_PLAN_NOT_FOUND", + "No such write plan. A plan is single-use and expires - call jira_write_plan (or `jam jira write-plan`) again.", + { planId }, + ); + } + + /** ``, with the id checked so it can only name a file here. */ + private pathFor(planId: string, suffix: string): string { + if (!/^[0-9a-fA-F-]{1,64}$/.test(planId)) { + throw this.notFound(planId); + } + return join(this.root, `${planId}${suffix}`); + } + + private ensureRoot(): void { + if (!existsSync(this.root)) mkdirSync(this.root, { recursive: true, mode: 0o700 }); } + /** + * Drop what has aged out, including claims abandoned by a process that died + * mid-apply. Not retrying such a claim is deliberate: apply may already have + * reached Jira, and that is `JAM_WRITE_UNCERTAIN`'s whole posture. + */ private evictExpired(): void { + if (!existsSync(this.root)) return; const now = this.now(); - for (const [id, plan] of this.plans) { - if (planExpired(plan.expiresAt, now)) this.plans.delete(id); + for (const name of readdirSync(this.root)) { + if (!name.endsWith(PENDING) && !name.endsWith(CLAIMED)) continue; + const path = join(this.root, name); + try { + const plan = JSON.parse(readFileSync(path, "utf8")) as WritePlan; + if (planExpired(plan.expiresAt, now)) rmSync(path, { force: true }); + } catch { + // Unparseable leftovers are not plans. Removing them keeps the + // directory from growing without bound, and nothing is lost: a plan + // that cannot be read cannot be applied either. + rmSync(path, { force: true }); + } } } } diff --git a/packages/server/src/cli-entry.ts b/packages/server/src/cli-entry.ts index 530dd8d..6c039d7 100644 --- a/packages/server/src/cli-entry.ts +++ b/packages/server/src/cli-entry.ts @@ -15,6 +15,7 @@ import { setupPlanCommand, } from "./cli/agent-api.js"; import { runJiraRead } from "./cli/jira-read.js"; +import { runJiraWrite } from "./cli/jira-write.js"; /** * Command dispatch for the JAM CLI, separated from the bin so other entry @@ -42,6 +43,11 @@ Jira jam jira full [KEY...] Read Jira from the shell - the same reads the MCP tools do, for a session that cannot see them yet + jam jira write-plan --operation [--key KEY] --input '' + jam jira write-apply + Write the same way the tools do: plan first, then + apply that plan by id. Nothing is written until + write-apply, and it takes no payload of its own Authentication jam auth status [--json] Whether Jira credentials are configured (never their value) @@ -168,9 +174,12 @@ export async function runJamCommand(argv: string[]): Promise { } case "jira": - // Reads addressed to the shell, for a session that cannot see the MCP - // tools yet. Same application path as the tools - see cli/jira-read.ts. - return runJiraRead(rest); + // Jira addressed to the shell, for a session that cannot see the MCP + // tools. Same application path as the tools, reads and writes alike - + // see cli/jira-read.ts and cli/jira-write.ts. + return rest[0] === "write-plan" || rest[0] === "write-apply" + ? runJiraWrite(rest) + : runJiraRead(rest); case "help": case "--help": diff --git a/packages/server/src/cli/jira-write.ts b/packages/server/src/cli/jira-write.ts new file mode 100644 index 0000000..e4a6c73 --- /dev/null +++ b/packages/server/src/cli/jira-write.ts @@ -0,0 +1,166 @@ +/** + * Writing Jira without the MCP channel. + * + * The companion to cli/jira-read.ts, for the same reason and with the same + * shape. A session whose MCP registry came up in a failed state cannot be shown + * the tools - Claude Code has no reload surface for a running session - so JAM, + * its credentials and its Jira access can all be healthy while `jira_write_plan` + * is simply unreachable. Reads were addressed to the shell first; this is the + * other half. + * + * Nothing here is a second write path. `write-plan` and `write-apply` call the + * same `planWrite` / `applyWritePlan` the tools call, with the same deps, the + * same project binding, the same whitelist and the same plan store - so a plan + * made through MCP can be applied here, which is the point when MCP is what + * broke. + * + * The two-call contract is not relaxed for the shell: + * + * write-plan reads Jira, decides what is possible, writes nothing + * write-apply takes a planId and nothing else, then reads the result back + * + * There is deliberately no `jam jira comment` or `jam jira close`. A shortcut + * that skips planning is exactly what the write plane exists to prevent, and + * being on a terminal does not change that. + * + * Contract, enforced by tests: + * stdout - one JSON document and nothing else, no ANSI, no prompts + * stderr - diagnostics only + * + * `write-apply` never asks for confirmation. What to write was settled when the + * plan was made; a prompt here would be a second approval step, which is a + * different feature and not this one. + */ + +import { applyWritePlan } from "../application/apply-write.js"; +import { planWrite } from "../application/plan-write.js"; +import { buildDeps, type BuildDepsOptions, type JamDeps } from "../deps.js"; +import { toJamError } from "../domain/errors.js"; +import { WRITE_OPERATIONS } from "../domain/write.js"; + +export const JIRA_WRITE_USAGE = `Usage: + jam jira write-plan --operation [--key KEY] --input '' + jam jira write-apply + +Operations: ${WRITE_OPERATIONS.join(", ")} + issue.create takes no --key; every other operation needs one. + +Two calls, always. write-plan changes nothing and returns a planId; write-apply +takes that planId and nothing else. Output is one JSON document on stdout - the +same receipt the MCP tools return, for a session that cannot see them. +`; + +export type JiraWriteOptions = BuildDepsOptions & { + /** Injected by tests so no test reaches a real Jira. */ + deps?: JamDeps; + /** Where the JSON document goes. Defaults to stdout. */ + write?: (text: string) => void; + /** Where diagnostics go. Defaults to stderr. */ + warn?: (text: string) => void; +}; + +/** `--flag value` / `--flag=value`, and nothing invented when absent. */ +function flagValue(argv: readonly string[], flag: string): string | undefined { + const index = argv.indexOf(flag); + if (index >= 0) return argv[index + 1]; + const inline = argv.find((arg) => arg.startsWith(`${flag}=`)); + return inline ? inline.slice(flag.length + 1) : undefined; +} + +const FLAGS_WITH_VALUES = ["--operation", "--key", "--input"]; + +const positional = (argv: readonly string[]): string[] => { + const out: string[] = []; + for (let i = 0; i < argv.length; i += 1) { + const arg = argv[i] as string; + if (FLAGS_WITH_VALUES.includes(arg)) { + i += 1; + continue; + } + if (arg.startsWith("--")) continue; + out.push(arg); + } + return out; +}; + +export async function runJiraWrite( + argv: readonly string[], + options: JiraWriteOptions = {}, +): Promise { + const write = options.write ?? ((text: string) => process.stdout.write(text)); + const warn = options.warn ?? ((text: string) => process.stderr.write(text)); + const [subcommand, ...rest] = argv; + + if (subcommand !== "write-plan" && subcommand !== "write-apply") { + warn(`Unknown jira command: ${subcommand ?? "(none)"}\n\n${JIRA_WRITE_USAGE}`); + return 1; + } + + // Argument shape is checked here; what the values mean is checked by the + // application, which is the same judgement the tools get. This file does not + // decide what a valid operation or input is. + let request: { kind: "plan"; key?: string; operation: string; input: Record } | { kind: "apply"; planId: string }; + + if (subcommand === "write-plan") { + const operation = flagValue(rest, "--operation"); + if (!operation) { + warn(`jam jira write-plan needs --operation.\n\n${JIRA_WRITE_USAGE}`); + return 1; + } + const raw = flagValue(rest, "--input"); + if (raw === undefined) { + warn(`jam jira write-plan needs --input ''.\n\n${JIRA_WRITE_USAGE}`); + return 1; + } + let input: unknown; + try { + input = JSON.parse(raw); + } catch { + warn("--input must be a JSON object.\n"); + return 1; + } + if (typeof input !== "object" || input === null || Array.isArray(input)) { + warn("--input must be a JSON object.\n"); + return 1; + } + const key = flagValue(rest, "--key"); + request = { + kind: "plan", + ...(key ? { key } : {}), + operation, + input: input as Record, + }; + } else { + // Exactly one positional and no way to pass anything else. There is no + // payload flag here, and adding one would be adding a way to write + // something the plan did not describe. + const [planId, ...extra] = positional(rest); + if (!planId || extra.length > 0) { + warn(`jam jira write-apply takes exactly one planId.\n\n${JIRA_WRITE_USAGE}`); + return 1; + } + request = { kind: "apply", planId }; + } + + try { + const { deps: injected, write: _w, warn: _n, ...depsOptions } = options; + const deps = injected ?? (await buildDeps(depsOptions)); + const result = + request.kind === "plan" + ? ( + await planWrite(deps, { + ...(request.key !== undefined ? { key: request.key } : {}), + operation: request.operation, + input: request.input, + }) + ).receipt + : await applyWritePlan(deps, { planId: request.planId }); + write(`${JSON.stringify(result)}\n`); + return 0; + } catch (err) { + // The same normalized codes the tools produce - an agent reads one contract, + // not two. + write(`${JSON.stringify(toJamError(err).toPayload())}\n`); + return 1; + } +} diff --git a/packages/server/src/domain/errors.ts b/packages/server/src/domain/errors.ts index 04082ff..343e278 100644 --- a/packages/server/src/domain/errors.ts +++ b/packages/server/src/domain/errors.ts @@ -45,6 +45,11 @@ export const JAM_ERROR_CODES = [ "JAM_WRITE_ASSIGNEE_ALREADY_SET", "JAM_WRITE_PLAN_NOT_FOUND", "JAM_WRITE_PLAN_EXPIRED", + // A stored plan that does not describe a change JAM would make. Plans live in + // a file so both transports can reach one, and apply re-derives the mutation + // from the plan's own input rather than trusting what the file says. Distinct + // from CONFLICT: nothing moved, the plan itself does not hold up. + "JAM_WRITE_PLAN_TAMPERED", "JAM_WRITE_CONFLICT", "JAM_WRITE_VERIFICATION_FAILED", "JAM_WRITE_UNCERTAIN", diff --git a/packages/server/src/domain/write.ts b/packages/server/src/domain/write.ts index f92faae..ab1ab80 100644 --- a/packages/server/src/domain/write.ts +++ b/packages/server/src/domain/write.ts @@ -185,6 +185,18 @@ type WritePlanCommon = { expiresAt: string; /** Normalized payload the apply step will send. Never supplied by a caller. */ mutation: WriteMutation; + /** + * The validated input the mutation was derived from. + * + * Kept so apply can derive the mutation again and compare. Plans live in a + * file now, and a file can be edited - re-deriving is what makes that + * pointless: changing the mutation without changing the input is caught, and + * changing both is just calling plan again. + * + * Validated, not raw: what a caller sent has already been checked against the + * operation's whitelist by the time it lands here. + */ + input: Record; }; /** diff --git a/packages/server/tests/helpers.ts b/packages/server/tests/helpers.ts index 903af12..36ef4a6 100644 --- a/packages/server/tests/helpers.ts +++ b/packages/server/tests/helpers.ts @@ -1,4 +1,5 @@ -import { readdirSync, readFileSync } from "node:fs"; +import { mkdtempSync, readdirSync, readFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import { join, relative } from "node:path"; import type { JamDeps } from "../src/deps.js"; import { ProjectConfigSchema, type ProjectConfig } from "../src/config/schema.js"; @@ -178,11 +179,25 @@ export class FakeJira implements JiraReadPort { } } +/** + * A plan store that never touches the real `~/.jam`. + * + * Plans became files so both transports can reach one, which means the default + * store now writes to the user's home. A suite that used the default would put + * test plans there - so every test gets its own throwaway directory instead. + */ +export function testWritePlanStore(now?: () => Date): WritePlanStore { + return new WritePlanStore({ + root: mkdtempSync(join(tmpdir(), "jam-plans-")), + ...(now ? { now } : {}), + }); +} + export function testDeps( jira: JiraReadPort, config: ProjectConfig = testConfig(), jiraWrite: JiraWritePort = new UnreachableJiraWrite(), - writePlans: WritePlanStore = new WritePlanStore(), + writePlans: WritePlanStore = testWritePlanStore(), jiraCreateMetadata: JiraCreateMetadataPort = new UnreachableCreateMetadata(), jiraAssignees: JiraAssigneeResolutionPort = new UnreachableAssignees(), ): JamDeps { diff --git a/packages/server/tests/unit/assignee.test.ts b/packages/server/tests/unit/assignee.test.ts index c3c89fb..5c7a339 100644 --- a/packages/server/tests/unit/assignee.test.ts +++ b/packages/server/tests/unit/assignee.test.ts @@ -10,8 +10,7 @@ import { issue, testConfig, testDeps, - UnreachableCreateMetadata, -} from "../helpers.js"; + UnreachableCreateMetadata, testWritePlanStore } from "../helpers.js"; /** * Assigning an issue to a person. @@ -42,7 +41,7 @@ function setup(options: { assignee?: { accountId: string; displayName: string } jira, testConfig({ project: { key: "PROJECT" } }), write, - new WritePlanStore(), + testWritePlanStore(), new UnreachableCreateMetadata(), assignees, ); diff --git a/packages/server/tests/unit/create-apply.test.ts b/packages/server/tests/unit/create-apply.test.ts index e8cbec6..a4b2e85 100644 --- a/packages/server/tests/unit/create-apply.test.ts +++ b/packages/server/tests/unit/create-apply.test.ts @@ -9,8 +9,7 @@ import { FakeJiraWrite, issue, testConfig, - testDeps, -} from "../helpers.js"; + testDeps, testWritePlanStore } from "../helpers.js"; /** * Applying a create. @@ -33,7 +32,7 @@ function setup(options: { now?: () => Date; created?: ReturnType } jira, testConfig({ project: { key: "PROJECT" } }), write, - new WritePlanStore(options.now ?? (() => new Date())), + testWritePlanStore(options.now ?? (() => new Date())), metadata, ); return { jam, metadata, write, jira }; diff --git a/packages/server/tests/unit/create-plan.test.ts b/packages/server/tests/unit/create-plan.test.ts index 86414de..4822b7a 100644 --- a/packages/server/tests/unit/create-plan.test.ts +++ b/packages/server/tests/unit/create-plan.test.ts @@ -8,8 +8,7 @@ import { FakeJiraWrite, testConfig, testDeps, - issue, -} from "../helpers.js"; + issue, testWritePlanStore } from "../helpers.js"; /** * Planning a create. @@ -25,7 +24,7 @@ function deps(metadata = new FakeCreateMetadata(), write = new FakeJiraWrite()) new FakeJira({ issues: [issue({ key: "PROJECT-1" })] }), testConfig({ project: { key: "PROJECT" } }), write, - new WritePlanStore(), + testWritePlanStore(), metadata, ), metadata, diff --git a/packages/server/tests/unit/jira-write-cli.test.ts b/packages/server/tests/unit/jira-write-cli.test.ts new file mode 100644 index 0000000..6ed3367 --- /dev/null +++ b/packages/server/tests/unit/jira-write-cli.test.ts @@ -0,0 +1,290 @@ +import { mkdtempSync, readFileSync, readdirSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { applyWritePlan } from "../../src/application/apply-write.js"; +import { planWrite } from "../../src/application/plan-write.js"; +import { WritePlanStore } from "../../src/application/write-plan-store.js"; +import { runJiraWrite } from "../../src/cli/jira-write.js"; +import type { JamDeps } from "../../src/deps.js"; +import { FakeJira, FakeJiraWrite, issue, testConfig, testDeps } from "../helpers.js"; + +/** + * Writing from the shell, when the MCP tools are not reachable. + * + * A session whose MCP registry came up failed cannot be shown the write tools - + * Claude Code has no reload surface for a running session - while JAM, its + * credentials and its Jira access are all fine. The reads got a shell address + * first (cli/jira-read.ts); this is the other half. + * + * Two things are fixed here. The transport contract: one JSON document, no + * prompts, no payload on apply. And the integrity that replaces the in-process + * plan store: a plan on disk is checked against its own input before anything + * is sent. + */ + +const capture = () => { + const out: string[] = []; + const err: string[] = []; + return { out, err, write: (t: string) => out.push(t), warn: (t: string) => err.push(t) }; +}; + +const storeRoot = () => mkdtempSync(join(tmpdir(), "jam-cli-write-")); + +const UPDATED = "2026-08-25T12:00:00.000+0900"; + +/** + * A read port that shows what the write port did. + * + * Apply confirms by reading, so a fake whose reads ignore its own writes can + * never produce an applied receipt - the verification step is doing its job. + */ +function linkedJira(jiraWrite: FakeJiraWrite): FakeJira { + const jira = new FakeJira({ + issues: [issue({ key: "PROJECT-1", issueId: "10001", updated: UPDATED })], + }); + const original = jira.getIssue.bind(jira); + jira.getIssue = async (req) => { + const result = await original(req); + if (result.issue) { + result.issue = { + ...result.issue, + comments: jiraWrite.comments.map((c, index) => ({ + id: String(index + 1), + author: "tester", + body: c.body, + created: UPDATED, + })), + ...(jiraWrite.updates.at(-1)?.fields ?? {}), + }; + } + return result; + }; + return jira; +} + +function writeDeps(root: string, jiraWrite = new FakeJiraWrite()): JamDeps { + return testDeps(linkedJira(jiraWrite), testConfig(), jiraWrite, new WritePlanStore({ root })); +} + +const planFile = (root: string): string => join(root, readdirSync(root)[0] as string); + +describe("jam jira write-plan - decides, changes nothing", () => { + it("returns the same receipt the tool returns, as one JSON document", async () => { + const root = storeRoot(); + const jiraWrite = new FakeJiraWrite(); + const io = capture(); + + const code = await runJiraWrite( + ["write-plan", "--key", "PROJECT-1", "--operation", "comment.add", "--input", '{"text":"from the shell"}'], + { deps: writeDeps(root, jiraWrite), write: io.write, warn: io.warn }, + ); + + expect(code).toBe(0); + expect(io.out).toHaveLength(1); + const receipt = JSON.parse(io.out[0] as string) as { status: string; planId: string }; + expect(receipt.status).toBe("planned"); + expect(receipt.planId).toBeTruthy(); + expect(io.err).toEqual([]); + // Planning reads. The whole point of the two-call shape is that this half + // cannot have written anything. + expect(jiraWrite.comments).toHaveLength(0); + }); + + it("a refusal is the same normalized code the tool produces", async () => { + const io = capture(); + const code = await runJiraWrite( + ["write-plan", "--key", "OTHER-1", "--operation", "comment.add", "--input", '{"text":"x"}'], + { deps: writeDeps(storeRoot()), write: io.write, warn: io.warn }, + ); + + expect(code).toBe(1); + expect(JSON.parse(io.out[0] as string)).toMatchObject({ + error: { code: "JAM_WRITE_SCOPE_VIOLATION" }, + }); + }); + + it("--input must be a JSON object, and says so on stderr", async () => { + const io = capture(); + const code = await runJiraWrite( + ["write-plan", "--key", "PROJECT-1", "--operation", "comment.add", "--input", "not json"], + { deps: writeDeps(storeRoot()), write: io.write, warn: io.warn }, + ); + expect(code).toBe(1); + expect(io.out).toEqual([]); + expect(io.err.join("")).toMatch(/JSON object/); + }); +}); + +describe("jam jira write-apply - takes a planId and nothing else", () => { + it("applies a plan the same process made", async () => { + const root = storeRoot(); + const jiraWrite = new FakeJiraWrite(); + const deps = writeDeps(root, jiraWrite); + const io = capture(); + + await runJiraWrite( + ["write-plan", "--key", "PROJECT-1", "--operation", "comment.add", "--input", '{"text":"hello"}'], + { deps, write: io.write, warn: io.warn }, + ); + const { planId } = JSON.parse(io.out[0] as string) as { planId: string }; + + const applied = capture(); + const code = await runJiraWrite(["write-apply", planId], { + deps, + write: applied.write, + warn: applied.warn, + }); + + expect(code).toBe(0); + expect(JSON.parse(applied.out[0] as string)).toMatchObject({ status: "applied", verified: true }); + expect(jiraWrite.comments).toHaveLength(1); + }); + + it("a plan planned in one process applies in another", async () => { + // The reason the store became a file. Two stores over one directory is what + // two `jam` invocations are. + const root = storeRoot(); + const jiraWrite = new FakeJiraWrite(); + const planning = writeDeps(root, jiraWrite); + const io = capture(); + await runJiraWrite( + ["write-plan", "--key", "PROJECT-1", "--operation", "comment.add", "--input", '{"text":"across"}'], + { deps: planning, write: io.write, warn: io.warn }, + ); + const { planId } = JSON.parse(io.out[0] as string) as { planId: string }; + + const applying = writeDeps(root, jiraWrite); + const applied = capture(); + const code = await runJiraWrite(["write-apply", planId], { + deps: applying, + write: applied.write, + warn: applied.warn, + }); + + expect(code).toBe(0); + expect(jiraWrite.comments).toHaveLength(1); + }); + + it("a plan made through the application layer applies from the shell", async () => { + // MCP calls planWrite directly. This is the case the change exists for: the + // tools stop answering after planning, and the shell finishes the write. + const root = storeRoot(); + const jiraWrite = new FakeJiraWrite(); + const deps = writeDeps(root, jiraWrite); + const { plan } = await planWrite(deps, { + key: "PROJECT-1", + operation: "comment.add", + input: { text: "planned through MCP" }, + }); + + const io = capture(); + const code = await runJiraWrite(["write-apply", plan.planId], { + deps: writeDeps(root, jiraWrite), + write: io.write, + warn: io.warn, + }); + + expect(code).toBe(0); + expect(jiraWrite.comments).toHaveLength(1); + }); + + it("takes exactly one planId and offers no way to pass a payload", async () => { + const io = capture(); + const code = await runJiraWrite(["write-apply"], { + deps: writeDeps(storeRoot()), + write: io.write, + warn: io.warn, + }); + expect(code).toBe(1); + expect(io.err.join("")).toMatch(/exactly one planId/); + + // Not an accident of parsing: the usage text is the contract, and it names + // no field, payload or override. + const source = readFileSync(new URL("../../src/cli/jira-write.ts", import.meta.url), "utf8"); + expect(source).not.toMatch(/--payload|--field|--override/); + }); +}); + +describe("a plan on disk is checked against itself", () => { + const tamper = (root: string, edit: (plan: Record) => void): void => { + const path = planFile(root); + const stored = JSON.parse(readFileSync(path, "utf8")) as Record; + edit(stored); + writeFileSync(path, JSON.stringify(stored), "utf8"); + }; + + it("an edited mutation is refused, and nothing is sent", async () => { + const root = storeRoot(); + const jiraWrite = new FakeJiraWrite(); + const deps = writeDeps(root, jiraWrite); + const { plan } = await planWrite(deps, { + key: "PROJECT-1", + operation: "comment.add", + input: { text: "what the plan says" }, + }); + + tamper(root, (stored) => { + stored["mutation"] = { kind: "comment", text: "something else entirely" }; + }); + + await expect( + applyWritePlan(writeDeps(root, jiraWrite), { planId: plan.planId }), + ).rejects.toMatchObject({ code: "JAM_WRITE_PLAN_TAMPERED" }); + expect(jiraWrite.comments).toHaveLength(0); + }); + + it("an input outside the whitelist is refused even when the mutation agrees", async () => { + const root = storeRoot(); + const jiraWrite = new FakeJiraWrite(); + const deps = writeDeps(root, jiraWrite); + const { plan } = await planWrite(deps, { + key: "PROJECT-1", + operation: "field.update", + input: { summary: "planned" }, + }); + + // Rewriting both is the interesting case: re-deriving has to run the + // whitelist too, or an edited plan gets to write a field planning refuses. + tamper(root, (stored) => { + stored["input"] = { reporter: "someone" }; + stored["mutation"] = { kind: "fields", fields: { reporter: "someone" } }; + }); + + await expect( + applyWritePlan(writeDeps(root, jiraWrite), { planId: plan.planId }), + ).rejects.toMatchObject({ code: "JAM_WRITE_PLAN_TAMPERED" }); + expect(jiraWrite.updates).toHaveLength(0); + }); + + it("a plan file nobody planned is refused", async () => { + const root = storeRoot(); + const jiraWrite = new FakeJiraWrite(); + const forged = "ffffffff-ffff-ffff-ffff-ffffffffffff"; + writeFileSync( + join(root, `${forged}.json`), + JSON.stringify({ + planId: forged, + kind: "existing-issue", + issueKey: "PROJECT-1", + issueId: "10001", + projectKey: "PROJECT", + operation: "comment.add", + before: { comments: 0 }, + intendedAfter: { commentAdded: "forged" }, + baseUpdated: UPDATED, + createdAt: "2026-01-01T00:00:00.000Z", + expiresAt: "2999-01-01T00:00:00.000Z", + // The mutation someone wants, with an input that does not produce it. + mutation: { kind: "comment", text: "forged" }, + input: { text: "not the same text" }, + }), + "utf8", + ); + + await expect( + applyWritePlan(writeDeps(root, jiraWrite), { planId: forged }), + ).rejects.toMatchObject({ code: "JAM_WRITE_PLAN_TAMPERED" }); + expect(jiraWrite.comments).toHaveLength(0); + }); +}); diff --git a/packages/server/tests/unit/read-adapter-direct-get.test.ts b/packages/server/tests/unit/read-adapter-direct-get.test.ts index da59251..496a5cc 100644 --- a/packages/server/tests/unit/read-adapter-direct-get.test.ts +++ b/packages/server/tests/unit/read-adapter-direct-get.test.ts @@ -10,8 +10,7 @@ import { FakeJiraWrite, issue, testConfig, - testDeps, -} from "../helpers.js"; + testDeps, testWritePlanStore } from "../helpers.js"; /** * ConsistencyPolicy calls for a direct issue GET around a write. @@ -80,7 +79,7 @@ describe("the write plane's reads", () => { jira, testConfig({ project: { key: "PROJECT" } }), write, - new WritePlanStore(), + testWritePlanStore(), metadata, ); diff --git a/packages/server/tests/unit/write-apply.test.ts b/packages/server/tests/unit/write-apply.test.ts index 7098382..b15b9b5 100644 --- a/packages/server/tests/unit/write-apply.test.ts +++ b/packages/server/tests/unit/write-apply.test.ts @@ -17,7 +17,7 @@ import type { SearchPageRequest, SearchPageResult, } from "../../src/ports/jira-read.port.js"; -import { FakeJiraWrite, issue, testConfig, testDeps } from "../helpers.js"; +import { FakeJiraWrite, issue, testConfig, testDeps, testWritePlanStore } from "../helpers.js"; /** * Applying is the half that changes something, so these tests are about the @@ -81,7 +81,7 @@ function setup(overrides: Partial = {}, transitions: FakeJiraW }), ); const jiraWrite = new FakeJiraWrite({ transitions }); - const deps: JamDeps = testDeps(jira, testConfig(), jiraWrite, new WritePlanStore()); + const deps: JamDeps = testDeps(jira, testConfig(), jiraWrite, testWritePlanStore()); return { jira, jiraWrite, deps }; } @@ -255,7 +255,7 @@ describe("a write that must not happen", () => { it("refuses an expired plan, and says to re-plan rather than that it is missing", async () => { const { jira, jiraWrite, deps } = setup(); let now = new Date(); - const store = new WritePlanStore(() => now); + const store = testWritePlanStore(() => now); const withStore: JamDeps = { ...deps, writePlans: store }; const { plan } = await planWrite(withStore, { diff --git a/packages/server/tests/unit/write-plan-store.test.ts b/packages/server/tests/unit/write-plan-store.test.ts new file mode 100644 index 0000000..2c0fe58 --- /dev/null +++ b/packages/server/tests/unit/write-plan-store.test.ts @@ -0,0 +1,119 @@ +import { mkdtempSync, readFileSync, readdirSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { WritePlanStore } from "../../src/application/write-plan-store.js"; +import { JamError } from "../../src/domain/errors.js"; +import type { NewWritePlan } from "../../src/domain/write.js"; + +/** + * Plans became files so both transports can reach one. + * + * The reason is `jam jira write-plan` and `jam jira write-apply`: two processes, + * so an in-memory plan is gone before apply can see it. Sharing the store also + * means a plan made through MCP survives the tools going stale, which is what + * the CLI write surface exists for. + * + * What that costs is the in-process guarantee, and these fix what replaces it: + * the file is claimed atomically, so two applies cannot both get it. + */ +const root = () => mkdtempSync(join(tmpdir(), "jam-plan-store-")); + +const plan = (overrides: Partial = {}): NewWritePlan => + ({ + kind: "existing-issue", + issueKey: "PROJECT-1", + issueId: "10001", + projectKey: "PROJECT", + operation: "comment.add", + before: { comments: 0 }, + intendedAfter: { commentAdded: "hello" }, + baseUpdated: "2026-01-01T00:00:00.000Z", + createdAt: "2026-01-01T00:00:00.000Z", + expiresAt: "2999-01-01T00:00:00.000Z", + mutation: { kind: "comment", text: "hello" }, + input: { text: "hello" }, + ...overrides, + }) as NewWritePlan; + +describe("a plan outlives the process that made it", () => { + it("a second store reads what the first wrote", () => { + const dir = root(); + const created = new WritePlanStore({ root: dir }).create(plan()); + + // The point of the change: `jam jira write-apply` is a different process + // from the one that planned. + const applied = new WritePlanStore({ root: dir }).take(created.planId); + expect(applied.planId).toBe(created.planId); + expect(applied.mutation).toEqual({ kind: "comment", text: "hello" }); + }); + + it("only one of two applies gets the plan", () => { + const dir = root(); + const created = new WritePlanStore({ root: dir }).create(plan()); + + const first = new WritePlanStore({ root: dir }); + const second = new WritePlanStore({ root: dir }); + expect(first.take(created.planId).planId).toBe(created.planId); + + // Whoever claimed it owns the apply. For comment.add the alternative is a + // second comment, which is exactly what single-use exists to prevent. + expect(() => second.take(created.planId)).toThrowError( + expect.objectContaining({ code: "JAM_WRITE_PLAN_NOT_FOUND" }) as unknown as Error, + ); + }); + + it("consume leaves nothing behind", () => { + const dir = root(); + const store = new WritePlanStore({ root: dir }); + const created = store.create(plan()); + store.take(created.planId); + store.consume(created.planId); + expect(readdirSync(dir)).toEqual([]); + }); + + it("an expired plan is reported as expired, not as missing", () => { + const dir = root(); + const store = new WritePlanStore({ + root: dir, + now: () => new Date("2026-01-01T00:11:00.000Z"), + }); + const created = store.create(plan({ expiresAt: "2026-01-01T00:10:00.000Z" })); + try { + store.take(created.planId); + throw new Error("expected a refusal"); + } catch (err) { + expect((err as JamError).code).toBe("JAM_WRITE_PLAN_EXPIRED"); + } + }); + + it("a planId cannot name a file outside the store", () => { + const store = new WritePlanStore({ root: root() }); + // Not a path traversal test for its own sake: planId reaches this from a + // command line now, so it has to be a name rather than a path. + expect(() => store.take("../../etc/passwd")).toThrowError( + expect.objectContaining({ code: "JAM_WRITE_PLAN_NOT_FOUND" }) as unknown as Error, + ); + }); + + it("what is written is the plan, and it is readable as one", () => { + const dir = root(); + const created = new WritePlanStore({ root: dir }).create(plan()); + const [file] = readdirSync(dir); + const onDisk = JSON.parse(readFileSync(join(dir, file as string), "utf8")) as { + planId: string; + input: unknown; + }; + // `input` is what apply re-derives from - see apply-write.ts. A plan + // without it could not be checked against itself. + expect(onDisk.planId).toBe(created.planId); + expect(onDisk.input).toEqual({ text: "hello" }); + }); + + it("leftovers that are not plans are cleared rather than accumulated", () => { + const dir = root(); + writeFileSync(join(dir, "not-a-plan.json"), "{ broken", "utf8"); + new WritePlanStore({ root: dir }).create(plan()); + expect(readdirSync(dir)).toHaveLength(1); + }); +});