Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 45 additions & 26 deletions docs/decisions/adr-jira-write-plane.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
64 changes: 63 additions & 1 deletion packages/server/src/application/apply-create-issue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

/**
Expand Down Expand Up @@ -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);
Expand All @@ -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<void> {
const { projectKey, schemaRequirements } = plan;
const issueTypes = await deps.jiraCreateMetadata.getIssueTypes(projectKey);
Expand Down
101 changes: 99 additions & 2 deletions packages/server/src/application/apply-write.ts
Original file line number Diff line number Diff line change
@@ -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 };

Expand Down Expand Up @@ -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
Expand All @@ -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.
*
Expand Down
9 changes: 8 additions & 1 deletion packages/server/src/application/plan-create-issue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ export async function planCreateIssue(
kind: "create",
fields: toJiraCreateFields(projectKey, issueType.id, input, { priority, components }),
},
input: request.input,
}) as CreateIssueWritePlan;

return {
Expand Down Expand Up @@ -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,
Expand Down
9 changes: 7 additions & 2 deletions packages/server/src/application/plan-write.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export async function planWrite(
...(transition ? { transition } : {}),
...(baseAssigneeAccountId ? { baseAssigneeAccountId } : {}),
mutation,
input: input as Record<string, unknown>,
}) as ExistingIssueWritePlan;

return {
Expand Down Expand Up @@ -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<string, unknown>): WriteInput {
export function validateInput(
operation: ExistingIssueOperation,
raw: Record<string, unknown>,
): WriteInput {
switch (operation) {
case "comment.add": {
const text = (raw as CommentAddInput).text;
Expand Down Expand Up @@ -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<string, unknown> {
/** Exported so apply can derive the same fields again and compare. */
export function toJiraFields(input: FieldUpdateInput): Record<string, unknown> {
const fields: Record<string, unknown> = {};
if (input.summary !== undefined) fields["summary"] = input.summary;
if (input.priority !== undefined) fields["priority"] = { name: input.priority };
Expand Down
Loading
Loading