From 09252bf45925780dbaf75afc400c5f8f59572e1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Sat, 5 Sep 2026 09:09:50 +0200 Subject: [PATCH 01/11] docs(workflow): materialize issue 184 artifacts --- ...utral-planning-pull-request-foundations.md | 1247 +++++++++++++++++ ...lanning-pull-request-foundations-design.md | 527 +++++++ 2 files changed, 1774 insertions(+) create mode 100644 docs/plans/2026-09-01-issue-184-provider-neutral-planning-pull-request-foundations.md create mode 100644 docs/specs/2026-09-01-issue-184-provider-neutral-planning-pull-request-foundations-design.md diff --git a/docs/plans/2026-09-01-issue-184-provider-neutral-planning-pull-request-foundations.md b/docs/plans/2026-09-01-issue-184-provider-neutral-planning-pull-request-foundations.md new file mode 100644 index 00000000..1ad30205 --- /dev/null +++ b/docs/plans/2026-09-01-issue-184-provider-neutral-planning-pull-request-foundations.md @@ -0,0 +1,1247 @@ +# Issue 184 Provider-Neutral Planning Pull Request Foundations Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use +> superpowers:subagent-driven-development (recommended) or +> superpowers:executing-plans to implement this plan task-by-task. Steps use +> checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add stable provider-neutral contracts and pure planning rules for +later planning pull request work. + +**Architecture:** Add one host contract module, one pure workflow module, and +one Git workspace contract module. Keep the current host factory and run-once +pipeline unchanged. + +**Tech Stack:** TypeScript, Node.js 24, NodeNext modules, `node:test`, strict +TypeScript checks, ESLint, Prettier, and dependency-cruiser. + +**Spec:** +`docs/specs/2026-09-01-issue-184-provider-neutral-planning-pull-request-foundations-design.md` + +## Global Constraints + +- Treat the issue #180 workflow decisions in the spec as fixed constraints. +- Use `RepositoryIdentity`, not `CanonicalRepositoryIdentity`. +- Keep `PLANNING_PR_WORKFLOW_VERSION` equal to `planning-pr-v1`. +- Keep the marker format exact: + ``. +- A successful pull request search means that the search is exhaustive. +- Keep incomplete search, proven absence, and identity mismatch as distinct + errors. +- Keep the new `PullRequestHost` separate from `RunOnceHostProvider`. +- Do not modify the current host factory, provider adapters, pipeline, run + state, labels, or publication code. +- Do not add production methods that throw `not implemented` errors. +- Do not implement Git commands in this issue. +- Keep the host and workspace fake adapters inside their colocated test files. +- Keep each production module below 200 meaningful lines. +- Do not add dependencies. +- Existing runtime and CLI behavior must stay unchanged. + +--- + +## File and Module Map + +### New production modules + +- `src/host/pull-requests.ts` owns repository identity, normalized pull request + contracts, host search semantics, and host contract errors. +- `src/workflow/planning-pull-requests.ts` owns phase derivation, phase + workspace names, markers, planning titles, and planning bodies. +- `src/git/planning-workspaces.ts` owns local workspace snapshots, lifecycle + operations, and workspace conflict errors. + +### New test modules + +- `src/host/pull-requests.test.ts` protects host contract behavior and contains + one fake host adapter. +- `src/workflow/planning-pull-requests.test.ts` protects all pure workflow + behavior. +- `src/git/planning-workspaces.test.ts` protects workspace contract behavior and + contains one fake workspace adapter. + +No existing source file changes in this issue. + +--- + +### Task 1: Define the provider-neutral pull request host contract + +**Files:** + +- Create: `src/host/pull-requests.ts` +- Create: `src/host/pull-requests.test.ts` + +**Interfaces:** + +- Consumes: `PatchmillHostProviderId` from `src/config/types.ts`. +- Produces: `RepositoryIdentity`, `PullRequestStatus`, `PullRequestReference`, + `PullRequestSummary`, `CreatePullRequestInput`, and `FindPullRequestsQuery`. +- Produces: `PullRequestHost` for later GitHub, Forgejo, and coordinator + adapters. +- Produces: `sameRepositoryIdentity` for all repository identity comparisons. +- Produces: `IncompletePullRequestSearchError`, `PullRequestNotFoundError`, and + `PullRequestIdentityError`. +- A `merged` summary requires `mergeCommit`. Other statuses prohibit it. +- A successful `findPullRequests` result is exhaustive by contract. + +- [ ] **Step 1: Write the repository identity and error tests** + +Create `src/host/pull-requests.test.ts` with these initial tests: + +```ts +import assert from "node:assert/strict"; +import test from "node:test"; +import { + IncompletePullRequestSearchError, + PullRequestIdentityError, + PullRequestNotFoundError, + sameRepositoryIdentity, + type FindPullRequestsQuery, + type RepositoryIdentity, +} from "./pull-requests.ts"; + +const githubRepository: RepositoryIdentity = { + provider: "github-gh", + host: "github.com", + owner: "rochecompaan", + repository: "patchmill", +}; + +test("repository identity comparison normalizes host, owner, and repository case", () => { + assert.equal( + sameRepositoryIdentity(githubRepository, { + provider: "github-gh", + host: "GITHUB.COM", + owner: "RocheCompaan", + repository: "Patchmill", + }), + true, + ); +}); + +test("repository identity comparison requires the same provider", () => { + assert.equal( + sameRepositoryIdentity(githubRepository, { + ...githubRepository, + provider: "forgejo-tea", + }), + false, + ); +}); + +test("incomplete search error keeps the exact query and provider limit", () => { + const query: FindPullRequestsQuery = { + targetRepository: githubRepository, + baseBranch: "main", + headRepository: githubRepository, + headBranch: "agent/issue-184-foundations-spec", + }; + const error = new IncompletePullRequestSearchError(query, 100); + + assert.equal(error.name, "IncompletePullRequestSearchError"); + assert.deepEqual(error.query, query); + assert.equal(error.limit, 100); +}); + +test("not found error keeps the exact pull request reference", () => { + const reference = { + targetRepository: githubRepository, + number: 404, + }; + const error = new PullRequestNotFoundError(reference); + + assert.equal(error.name, "PullRequestNotFoundError"); + assert.deepEqual(error.reference, reference); +}); + +test("identity error keeps expected and partial actual identity", () => { + const error = new PullRequestIdentityError("repository mismatch", { + expected: githubRepository, + actual: { host: "forge.example.test" }, + }); + + assert.equal(error.name, "PullRequestIdentityError"); + assert.equal(error.reason, "repository mismatch"); + assert.deepEqual(error.expected, githubRepository); + assert.deepEqual(error.actual, { host: "forge.example.test" }); +}); +``` + +- [ ] **Step 2: Add a fake host contract test** + +Add these imports and fixtures to the same test file: + +```ts +import type { + CreatePullRequestInput, + PullRequestHost, + PullRequestReference, + PullRequestSummary, +} from "./pull-requests.ts"; + +const openPullRequest = { + number: 12, + url: "https://github.com/rochecompaan/patchmill/pull/12", + status: "open", + targetRepository: githubRepository, + baseBranch: "main", + headRepository: githubRepository, + headBranch: "agent/issue-184-foundations-spec", + headSha: "abc123", + body: "Refs #184", +} satisfies PullRequestSummary; + +function fakePullRequestHost(summary: PullRequestSummary): PullRequestHost { + let body = summary.body; + return { + id: "github-gh", + resolveTargetRepositoryIdentity: async () => githubRepository, + resolveRemoteRepositoryIdentity: async () => githubRepository, + createPullRequest: async (_input: CreatePullRequestInput) => summary, + findPullRequests: async (_query: FindPullRequestsQuery) => [summary], + getPullRequest: async (_reference: PullRequestReference) => summary, + readPullRequestBody: async (_reference: PullRequestReference) => body, + updatePullRequestBody: async ( + _reference: PullRequestReference, + nextBody: string, + ) => { + body = nextBody; + }, + }; +} + +test("a fake host satisfies the same contract as production adapters", async () => { + const host = fakePullRequestHost(openPullRequest); + const reference = { + targetRepository: githubRepository, + number: openPullRequest.number, + }; + const query: FindPullRequestsQuery = { + targetRepository: githubRepository, + baseBranch: "main", + headRepository: githubRepository, + headBranch: openPullRequest.headBranch, + }; + + assert.deepEqual(await host.findPullRequests(query), [openPullRequest]); + assert.deepEqual(await host.getPullRequest(reference), openPullRequest); + await host.updatePullRequestBody(reference, "updated body"); + assert.equal(await host.readPullRequestBody(reference), "updated body"); +}); +``` + +This test adapter is local to this test file. Do not create a shared test +helper. + +- [ ] **Step 3: Run the focused test to prove RED** + +Run: + +```sh +node --test src/host/pull-requests.test.ts +``` + +Expected: FAIL because `src/host/pull-requests.ts` does not exist. + +- [ ] **Step 4: Implement the complete host contract module** + +Create `src/host/pull-requests.ts` with this public surface and implementation: + +```ts +import type { PatchmillHostProviderId } from "../config/types.ts"; + +export type RepositoryIdentity = { + provider: PatchmillHostProviderId; + host: string; + owner: string; + repository: string; +}; + +export type PullRequestStatus = "open" | "merged" | "closed-unmerged"; + +export type PullRequestReference = { + targetRepository: RepositoryIdentity; + number: number; +}; + +type PullRequestSummaryIdentity = { + number: number; + url: string; + targetRepository: RepositoryIdentity; + baseBranch: string; + headRepository: RepositoryIdentity; + headBranch: string; + headSha: string; + body: string; +}; + +export type PullRequestSummary = PullRequestSummaryIdentity & + ( + | { status: "open"; mergeCommit?: undefined } + | { status: "merged"; mergeCommit: string } + | { status: "closed-unmerged"; mergeCommit?: undefined } + ); + +export type CreatePullRequestInput = { + title: string; + body: string; + baseBranch: string; + headBranch: string; +}; + +export type FindPullRequestsQuery = { + targetRepository: RepositoryIdentity; + baseBranch: string; + headRepository: RepositoryIdentity; + headBranch: string; +}; + +function sameCaseInsensitiveValue(left: string, right: string): boolean { + return left.toLowerCase() === right.toLowerCase(); +} + +export function sameRepositoryIdentity( + left: RepositoryIdentity, + right: RepositoryIdentity, +): boolean { + return ( + left.provider === right.provider && + sameCaseInsensitiveValue(left.host, right.host) && + sameCaseInsensitiveValue(left.owner, right.owner) && + sameCaseInsensitiveValue(left.repository, right.repository) + ); +} + +export class IncompletePullRequestSearchError extends Error { + readonly query: FindPullRequestsQuery; + readonly limit?: number; + + constructor(query: FindPullRequestsQuery, limit?: number) { + super( + limit === undefined + ? "Pull request search was incomplete" + : `Pull request search stopped at provider limit ${limit}`, + ); + this.name = "IncompletePullRequestSearchError"; + this.query = query; + if (limit !== undefined) this.limit = limit; + } +} + +export class PullRequestNotFoundError extends Error { + readonly reference: PullRequestReference; + + constructor(reference: PullRequestReference) { + super(`Pull request #${reference.number} was not found`); + this.name = "PullRequestNotFoundError"; + this.reference = reference; + } +} + +export class PullRequestIdentityError extends Error { + readonly reason: string; + readonly expected?: RepositoryIdentity; + readonly actual?: Partial; + + constructor( + reason: string, + identities: { + expected?: RepositoryIdentity; + actual?: Partial; + } = {}, + ) { + super(`Pull request identity is invalid: ${reason}`); + this.name = "PullRequestIdentityError"; + this.reason = reason; + if (identities.expected !== undefined) { + this.expected = identities.expected; + } + if (identities.actual !== undefined) { + this.actual = identities.actual; + } + } +} + +export interface PullRequestHost { + readonly id: PatchmillHostProviderId; + + resolveTargetRepositoryIdentity(): Promise; + + resolveRemoteRepositoryIdentity(remote: string): Promise; + + createPullRequest(input: CreatePullRequestInput): Promise; + + findPullRequests( + query: FindPullRequestsQuery, + ): Promise; + + getPullRequest(reference: PullRequestReference): Promise; + + readPullRequestBody(reference: PullRequestReference): Promise; + + updatePullRequestBody( + reference: PullRequestReference, + body: string, + ): Promise; +} +``` + +Do not import or extend `RunOnceHostProvider`. Do not modify either production +provider. + +- [ ] **Step 5: Format and run focused validation** + +Run: + +```sh +npx --no-install prettier --write \ + src/host/pull-requests.ts \ + src/host/pull-requests.test.ts +node --test src/host/pull-requests.test.ts +npm run check:types +``` + +Expected: the focused tests and strict TypeScript checks PASS. + +- [ ] **Step 6: Commit the host contract** + +```sh +git add src/host/pull-requests.ts src/host/pull-requests.test.ts +git commit -m "feat(host): define pull request contract" +``` + +--- + +### Task 2: Implement pure planning phase and pull request rules + +**Files:** + +- Create: `src/workflow/planning-pull-requests.ts` +- Create: `src/workflow/planning-pull-requests.test.ts` + +**Interfaces:** + +- Consumes: `GitWorktreeStrategyConfig` from `src/git/types.ts`. +- Consumes: `buildIssueBranchName` and `buildIssueWorktreePath` from + `src/git/worktree-strategy.ts`. +- Produces: `PlanningPhaseKind`, `PlanningArtifactKind`, `PlanningGateSnapshot`, + and `PlannedPhase`. +- Produces: `planningPhasePlan` with the exact four-row workflow matrix. +- Produces: `phaseWorkspaceIdentity` with the phase suffix outside `slugLength`. +- Produces: strict marker rendering and parsing for `planning-pr-v1`. +- Produces: fixed non-closing planning titles and bodies. +- Produces: `PlanningPullRequestMarkerError` for malformed identity markers. + +- [ ] **Step 1: Write the four-combination phase planner test** + +Create `src/workflow/planning-pull-requests.test.ts` with this table-driven +test: + +```ts +import assert from "node:assert/strict"; +import test from "node:test"; +import { + planningPhasePlan, + type PlannedPhase, + type PlanningGateSnapshot, +} from "./planning-pull-requests.ts"; + +const phaseCases: Array<{ + gates: PlanningGateSnapshot; + expected: readonly PlannedPhase[]; +}> = [ + { + gates: { specRequired: false, planRequired: false }, + expected: [ + { + kind: "implementation", + artifactKinds: ["spec", "plan"], + pullRequestRequired: true, + }, + ], + }, + { + gates: { specRequired: true, planRequired: false }, + expected: [ + { + kind: "spec", + artifactKinds: ["spec"], + pullRequestRequired: true, + }, + { + kind: "implementation", + artifactKinds: ["plan"], + pullRequestRequired: true, + }, + ], + }, + { + gates: { specRequired: false, planRequired: true }, + expected: [ + { + kind: "plan", + artifactKinds: ["spec", "plan"], + pullRequestRequired: true, + }, + { + kind: "implementation", + artifactKinds: [], + pullRequestRequired: true, + }, + ], + }, + { + gates: { specRequired: true, planRequired: true }, + expected: [ + { + kind: "spec", + artifactKinds: ["spec"], + pullRequestRequired: true, + }, + { + kind: "plan", + artifactKinds: ["plan"], + pullRequestRequired: true, + }, + { + kind: "implementation", + artifactKinds: [], + pullRequestRequired: true, + }, + ], + }, +]; + +test("planning phase plan covers all spec and plan gate combinations", () => { + for (const entry of phaseCases) { + assert.deepEqual(planningPhasePlan(entry.gates), entry.expected); + } +}); +``` + +- [ ] **Step 2: Add workspace identity and marker tests** + +Add these imports and tests to the same file: + +```ts +import { + PlanningPullRequestMarkerError, + parsePlanningPullRequestMarker, + phaseWorkspaceIdentity, + renderPlanningPullRequestMarker, +} from "./planning-pull-requests.ts"; +import type { GitWorktreeStrategyConfig } from "../git/types.ts"; + +const strategy: GitWorktreeStrategyConfig = { + baseBranch: "main", + baseRef: "HEAD", + remote: "origin", + branchPrefix: "agent/issue-", + worktreeDir: ".worktrees", + worktreePrefix: "patchmill-issue-", + slugLength: 48, + allowDirectLand: false, +}; + +test("phase workspace identity appends the phase outside the issue slug", () => { + assert.deepEqual( + phaseWorkspaceIdentity({ + issueNumber: 184, + title: "Define provider-neutral planning pull request foundations", + phase: "plan", + strategy, + }), + { + branch: + "agent/issue-184-define-provider-neutral-planning-pull-request-fo-plan", + worktreePath: + ".worktrees/patchmill-issue-184-define-provider-neutral-planning-pull-request-fo-plan", + }, + ); +}); + +test("planning pull request marker renders and parses exact identity", () => { + const marker = renderPlanningPullRequestMarker({ + issueNumber: 184, + phase: "spec", + }); + + assert.equal( + marker, + "", + ); + assert.deepEqual(parsePlanningPullRequestMarker(`Header\n\n${marker}\n`), { + workflowVersion: "planning-pr-v1", + issueNumber: 184, + phase: "spec", + }); +}); + +test("marker parser returns undefined when no planning marker exists", () => { + assert.equal(parsePlanningPullRequestMarker("Refs #184"), undefined); +}); + +test("marker parser rejects duplicate and invalid planning markers", () => { + const invalidBodies = [ + [ + "", + "", + ].join("\n"), + "", + "", + "", + "/u); + assert.doesNotMatch(body, closingKeyword); +}); +``` + +- [ ] **Step 4: Run the focused test to prove RED** + +Run: + +```sh +node --test src/workflow/planning-pull-requests.test.ts +``` + +Expected: FAIL because `src/workflow/planning-pull-requests.ts` does not exist. + +- [ ] **Step 5: Implement the pure workflow module** + +Create `src/workflow/planning-pull-requests.ts` with this implementation: + +```ts +import { + buildIssueBranchName, + buildIssueWorktreePath, +} from "../git/worktree-strategy.ts"; +import type { GitWorktreeStrategyConfig } from "../git/types.ts"; + +export const PLANNING_PR_WORKFLOW_VERSION = "planning-pr-v1" as const; + +export type PlanningPhaseKind = "spec" | "plan" | "implementation"; +export type PlanningArtifactKind = "spec" | "plan"; + +export type PlanningGateSnapshot = { + specRequired: boolean; + planRequired: boolean; +}; + +export type PlannedPhase = { + kind: PlanningPhaseKind; + artifactKinds: readonly PlanningArtifactKind[]; + pullRequestRequired: true; +}; + +export function planningPhasePlan( + gates: PlanningGateSnapshot, +): readonly PlannedPhase[] { + if (gates.specRequired && gates.planRequired) { + return [ + { + kind: "spec", + artifactKinds: ["spec"], + pullRequestRequired: true, + }, + { + kind: "plan", + artifactKinds: ["plan"], + pullRequestRequired: true, + }, + { + kind: "implementation", + artifactKinds: [], + pullRequestRequired: true, + }, + ]; + } + if (gates.specRequired) { + return [ + { + kind: "spec", + artifactKinds: ["spec"], + pullRequestRequired: true, + }, + { + kind: "implementation", + artifactKinds: ["plan"], + pullRequestRequired: true, + }, + ]; + } + if (gates.planRequired) { + return [ + { + kind: "plan", + artifactKinds: ["spec", "plan"], + pullRequestRequired: true, + }, + { + kind: "implementation", + artifactKinds: [], + pullRequestRequired: true, + }, + ]; + } + return [ + { + kind: "implementation", + artifactKinds: ["spec", "plan"], + pullRequestRequired: true, + }, + ]; +} + +export function phaseWorkspaceIdentity(input: { + issueNumber: number; + title: string; + phase: PlanningPhaseKind; + strategy: GitWorktreeStrategyConfig; +}): { branch: string; worktreePath: string } { + const branch = buildIssueBranchName( + input.issueNumber, + input.title, + input.strategy, + ); + const worktreePath = buildIssueWorktreePath( + input.issueNumber, + input.title, + input.strategy, + ); + return { + branch: `${branch}-${input.phase}`, + worktreePath: `${worktreePath}-${input.phase}`, + }; +} + +const markerCandidatePattern = //gu; +const validMarkerPattern = + /^$/u; +const markerPrefix = "patchmill:planning-pr-"; + +export class PlanningPullRequestMarkerError extends Error { + readonly reason: string; + readonly marker: string; + + constructor(reason: string, marker: string) { + super(`Planning pull request marker is invalid: ${reason}`); + this.name = "PlanningPullRequestMarkerError"; + this.reason = reason; + this.marker = marker; + } +} + +function assertPositiveIssueNumber(issueNumber: number): void { + if (!Number.isSafeInteger(issueNumber) || issueNumber < 1) { + throw new RangeError("Issue number must be a positive safe integer"); + } +} + +export function renderPlanningPullRequestMarker(input: { + issueNumber: number; + phase: PlanningPhaseKind; +}): string { + assertPositiveIssueNumber(input.issueNumber); + return ``; +} + +export function parsePlanningPullRequestMarker(body: string): + | { + workflowVersion: typeof PLANNING_PR_WORKFLOW_VERSION; + issueNumber: number; + phase: PlanningPhaseKind; + } + | undefined { + const candidates = body.match(markerCandidatePattern) ?? []; + if (candidates.length === 0) { + if (body.includes(markerPrefix)) { + throw new PlanningPullRequestMarkerError("malformed marker", body); + } + return undefined; + } + if (candidates.length !== 1) { + throw new PlanningPullRequestMarkerError( + "multiple markers", + candidates.join("\n"), + ); + } + const marker = candidates[0]!; + const match = validMarkerPattern.exec(marker); + if (!match) { + throw new PlanningPullRequestMarkerError("unsupported marker", marker); + } + const issueNumber = Number(match[2]); + if (!Number.isSafeInteger(issueNumber)) { + throw new PlanningPullRequestMarkerError("invalid issue number", marker); + } + return { + workflowVersion: PLANNING_PR_WORKFLOW_VERSION, + issueNumber, + phase: match[3] as PlanningPhaseKind, + }; +} + +export function planningPullRequestTitle(input: { + issueNumber: number; + phase: PlanningArtifactKind; +}): string { + assertPositiveIssueNumber(input.issueNumber); + const label = input.phase === "spec" ? "Spec" : "Plan"; + return `${label} for #${input.issueNumber}`; +} + +export function planningPullRequestBody(input: { + issueNumber: number; + phase: PlanningArtifactKind; + artifactPaths: readonly string[]; +}): string { + assertPositiveIssueNumber(input.issueNumber); + const artifactPaths = [...new Set(input.artifactPaths)]; + if (artifactPaths.length === 0) { + throw new RangeError("Planning pull request requires an artifact path"); + } + const label = input.phase === "spec" ? "Spec" : "Plan"; + return [ + `Refs #${input.issueNumber}`, + "", + "## Planning phase", + "", + label, + "", + "## Artifacts", + "", + ...artifactPaths.map((path) => `- \`${path}\``), + "", + "Merge this pull request to unlock the next phase.", + "", + renderPlanningPullRequestMarker(input), + ].join("\n"); +} +``` + +Keep marker parsing strict. Do not accept unknown versions or phases. + +- [ ] **Step 6: Format and run focused validation** + +Run: + +```sh +npx --no-install prettier --write \ + src/workflow/planning-pull-requests.ts \ + src/workflow/planning-pull-requests.test.ts +node --test src/workflow/planning-pull-requests.test.ts +npm run check:types +npm run check:architecture +``` + +Expected: the focused tests, strict TypeScript checks, and architecture check +PASS. + +- [ ] **Step 7: Commit the pure workflow rules** + +```sh +git add \ + src/workflow/planning-pull-requests.ts \ + src/workflow/planning-pull-requests.test.ts +git commit -m "feat(workflow): define planning pull request phases" +``` + +--- + +### Task 3: Define the planning workspace lifecycle contract + +**Files:** + +- Create: `src/git/planning-workspaces.ts` +- Create: `src/git/planning-workspaces.test.ts` + +**Interfaces:** + +- Produces: `PlanningWorkspaceIdentity` for an expected branch and worktree + path. +- Produces: a discriminated `PlanningWorkspaceSnapshot` with `missing`, + `branch-only`, and `ready` states. +- Produces: `PreparedPlanningWorkspace` with the fetched remote base and ready + snapshot. +- Produces: `PlanningWorkspaceLifecycle` with `prepare`, `inspect`, + `removeWorktree`, and `removeBranch`. +- Produces: `PlanningWorkspaceConflictReason` and + `PlanningWorkspaceConflictError` for fail-closed state. +- The interface binds each concrete adapter to one repository root. +- The interface does not expose `CommandRunner` or Git command details. + +- [ ] **Step 1: Write the fake workspace contract test** + +Create `src/git/planning-workspaces.test.ts` with this in-memory adapter and +contract test: + +```ts +import assert from "node:assert/strict"; +import test from "node:test"; +import { + PlanningWorkspaceConflictError, + type PlanningWorkspaceIdentity, + type PlanningWorkspaceLifecycle, + type PlanningWorkspaceSnapshot, + type PreparedPlanningWorkspace, +} from "./planning-workspaces.ts"; + +const identity: PlanningWorkspaceIdentity = { + branch: "agent/issue-184-foundations-spec", + worktreePath: ".worktrees/patchmill-issue-184-foundations-spec", +}; + +class FakePlanningWorkspace implements PlanningWorkspaceLifecycle { + readonly events: string[] = []; + private snapshot: PlanningWorkspaceSnapshot = { + state: "missing", + identity, + }; + + async prepare(input: { + identity: PlanningWorkspaceIdentity; + remote: string; + baseBranch: string; + resume?: { baseSha: string; headSha: string }; + }): Promise { + this.events.push("prepare"); + const headSha = input.resume?.headSha ?? "head-1"; + const snapshot: Extract = { + state: "ready", + identity: input.identity, + headSha, + clean: true, + }; + this.snapshot = snapshot; + return { + created: input.resume === undefined, + remote: input.remote, + baseBranch: input.baseBranch, + baseSha: input.resume?.baseSha ?? "base-1", + snapshot, + }; + } + + async inspect( + _identity: PlanningWorkspaceIdentity, + ): Promise { + this.events.push("inspect"); + return this.snapshot; + } + + async removeWorktree( + _identity: PlanningWorkspaceIdentity, + ): Promise> { + this.events.push("remove-worktree"); + if (this.snapshot.state !== "ready") { + throw new PlanningWorkspaceConflictError( + "branch-owned-by-other-worktree", + identity, + ); + } + if (!this.snapshot.clean) { + throw new PlanningWorkspaceConflictError("dirty-worktree", identity); + } + this.snapshot = { + state: "branch-only", + identity, + headSha: this.snapshot.headSha, + }; + return this.snapshot; + } + + async removeBranch(input: { + identity: PlanningWorkspaceIdentity; + pushedHeadSha: string; + }): Promise> { + this.events.push("remove-branch"); + if ( + this.snapshot.state !== "branch-only" || + this.snapshot.headSha !== input.pushedHeadSha + ) { + throw new PlanningWorkspaceConflictError("head-not-pushed", identity); + } + this.snapshot = { state: "missing", identity: input.identity }; + return this.snapshot; + } +} + +test("a coordinator can use the workspace lifecycle without Git commands", async () => { + const workspace = new FakePlanningWorkspace(); + const prepared = await workspace.prepare({ + identity, + remote: "origin", + baseBranch: "main", + }); + + assert.equal(prepared.created, true); + assert.equal(prepared.baseSha, "base-1"); + assert.deepEqual(await workspace.inspect(identity), prepared.snapshot); + + const branchOnly = await workspace.removeWorktree(identity); + assert.equal(branchOnly.state, "branch-only"); + + const missing = await workspace.removeBranch({ + identity, + pushedHeadSha: "head-1", + }); + assert.equal(missing.state, "missing"); + assert.deepEqual(workspace.events, [ + "prepare", + "inspect", + "remove-worktree", + "remove-branch", + ]); +}); + +test("workspace conflict error keeps the reason and identity", () => { + const error = new PlanningWorkspaceConflictError( + "unregistered-path", + identity, + ); + + assert.equal(error.name, "PlanningWorkspaceConflictError"); + assert.equal(error.reason, "unregistered-path"); + assert.deepEqual(error.identity, identity); +}); +``` + +Do not move this adapter to `test-support`. Later issue #188 can extract a +shared adapter after it has a second consumer. + +- [ ] **Step 2: Run the focused test to prove RED** + +Run: + +```sh +node --test src/git/planning-workspaces.test.ts +``` + +Expected: FAIL because `src/git/planning-workspaces.ts` does not exist. + +- [ ] **Step 3: Implement the workspace lifecycle types and interface** + +Create `src/git/planning-workspaces.ts` with this code: + +```ts +export type PlanningWorkspaceIdentity = { + readonly branch: string; + readonly worktreePath: string; +}; + +export type PlanningWorkspaceSnapshot = + | { + readonly state: "missing"; + readonly identity: PlanningWorkspaceIdentity; + } + | { + readonly state: "branch-only"; + readonly identity: PlanningWorkspaceIdentity; + readonly headSha: string; + } + | { + readonly state: "ready"; + readonly identity: PlanningWorkspaceIdentity; + readonly headSha: string; + readonly clean: boolean; + }; + +export type PreparedPlanningWorkspace = { + readonly created: boolean; + readonly remote: string; + readonly baseBranch: string; + readonly baseSha: string; + readonly snapshot: Extract; +}; + +export type PlanningWorkspaceConflictReason = + | "path-owned-by-other-branch" + | "branch-owned-by-other-worktree" + | "unregistered-path" + | "detached-worktree" + | "base-sha-mismatch" + | "head-sha-mismatch" + | "dirty-worktree" + | "head-not-pushed"; + +export class PlanningWorkspaceConflictError extends Error { + readonly reason: PlanningWorkspaceConflictReason; + readonly identity: PlanningWorkspaceIdentity; + + constructor( + reason: PlanningWorkspaceConflictReason, + identity: PlanningWorkspaceIdentity, + ) { + super(`Planning workspace is unsafe: ${reason}`); + this.name = "PlanningWorkspaceConflictError"; + this.reason = reason; + this.identity = identity; + } +} + +export interface PlanningWorkspaceLifecycle { + prepare(input: { + identity: PlanningWorkspaceIdentity; + remote: string; + baseBranch: string; + resume?: { + baseSha: string; + headSha: string; + }; + }): Promise; + + inspect( + identity: PlanningWorkspaceIdentity, + ): Promise; + + removeWorktree( + identity: PlanningWorkspaceIdentity, + ): Promise>; + + removeBranch(input: { + identity: PlanningWorkspaceIdentity; + pushedHeadSha: string; + }): Promise>; +} +``` + +Do not import `CommandRunner`. Do not add a production Git adapter in this task. + +- [ ] **Step 4: Format and run focused validation** + +Run: + +```sh +npx --no-install prettier --write \ + src/git/planning-workspaces.ts \ + src/git/planning-workspaces.test.ts +node --test src/git/planning-workspaces.test.ts +npm run check:types +npm run check:architecture +``` + +Expected: the focused tests, strict TypeScript checks, and architecture check +PASS. + +- [ ] **Step 5: Commit the workspace contract** + +```sh +git add \ + src/git/planning-workspaces.ts \ + src/git/planning-workspaces.test.ts +git commit -m "feat(git): define planning workspace lifecycle" +``` + +--- + +## Final Validation + +After Task 3, run all focused contract tests together: + +```sh +node --test \ + src/host/pull-requests.test.ts \ + src/workflow/planning-pull-requests.test.ts \ + src/git/planning-workspaces.test.ts +``` + +Expected: all focused tests PASS. + +Run the repository validation commands: + +```sh +npm run check:types +npm run build +npm run check:architecture +npm run lint +npm test +git diff --check +``` + +Expected: every command exits with status 0. + +Inspect the branch file list: + +```sh +git diff --name-only main...HEAD +``` + +Expected branch files: + +```text +docs/plans/2026-09-01-issue-184-provider-neutral-planning-pull-request-foundations.md +docs/specs/2026-09-01-issue-184-provider-neutral-planning-pull-request-foundations-design.md +src/git/planning-workspaces.test.ts +src/git/planning-workspaces.ts +src/host/pull-requests.test.ts +src/host/pull-requests.ts +src/workflow/planning-pull-requests.test.ts +src/workflow/planning-pull-requests.ts +``` + +No other production file changes belong in issue #184. + +No Nix build is required because this issue does not change npm dependencies. + +## Implementation Completion Report + +The implementation worker reports: + +- The three implementation commit hashes. +- The focused test result. +- The strict TypeScript, build, architecture, lint, and full test results. +- Any deviation from the six planned implementation files. +- Any residual risk in the contracts for issues #185, #186, or #187. diff --git a/docs/specs/2026-09-01-issue-184-provider-neutral-planning-pull-request-foundations-design.md b/docs/specs/2026-09-01-issue-184-provider-neutral-planning-pull-request-foundations-design.md new file mode 100644 index 00000000..d894c663 --- /dev/null +++ b/docs/specs/2026-09-01-issue-184-provider-neutral-planning-pull-request-foundations-design.md @@ -0,0 +1,527 @@ +# Issue 184 provider-neutral planning pull request foundations design + +## Status + +The design decisions are approved in chat. This written specification awaits +document review. + +Issue #184 is the first delivery slice of issue #180. This design treats the +approved workflow decisions from issue #180 as constraints. + +## Summary + +Patchmill needs stable foundations for planning pull requests before it adds +GitHub, Forgejo, and Git workspace implementations. + +This change defines three focused modules: + +1. A provider-neutral pull request host contract. +2. Pure planning phase and pull request rules. +3. A planning workspace lifecycle contract. + +This change does not connect the new contracts to the current run-once workflow. +Existing runtime behavior stays unchanged. + +## Goals + +- Define complete repository and pull request identities. +- Define pull request search completeness and error semantics. +- Define the phase sequence for all four planning gate combinations. +- Define deterministic planning pull request markers, titles, and bodies. +- Define deterministic phase branch and worktree names. +- Hide Git command sequences behind a small workspace interface. +- Prove the host and workspace seams with test adapters. +- Give later issues stable types and functions to implement and consume. + +## Non-goals + +- Add GitHub commands. +- Add Forgejo commands. +- Implement Git worktree operations. +- Change the host factory or `RunOnceHostProvider`. +- Add production methods that throw `not implemented` errors. +- Add run recovery state, locks, phase coordination, or publication. +- Change issue labels, artifact comments, or pipeline behavior. +- Publish or merge a planning pull request. + +## Module map + +### `src/host/pull-requests.ts` + +This module owns provider-neutral repository and pull request contracts. It +contains no CLI commands and no provider response fields. + +### `src/workflow/planning-pull-requests.ts` + +This module owns pure planning workflow rules. It derives phases and renders or +parses deterministic identity text. + +### `src/git/planning-workspaces.ts` + +This module owns the seam for the local phase workspace lifecycle. It defines +observable snapshots and high-level lifecycle operations. + +Each module has one colocated test file. The test adapters stay in those test +files until another test needs the same implementation. + +## Repository identity + +The public type is `RepositoryIdentity`. The word `Canonical` is not part of the +identifier. + +```ts +export type RepositoryIdentity = { + provider: PatchmillHostProviderId; + host: string; + owner: string; + repository: string; +}; +``` + +A `RepositoryIdentity` is complete and provider-normalized: + +- `provider` identifies the active host adapter. +- `host` contains the canonical host name without a URL scheme or path. +- `owner` contains the provider-resolved repository owner. +- `repository` contains the provider-resolved repository name. + +An owner and repository slug without a host is not a complete identity. A raw +Git remote URL is not a `RepositoryIdentity`. + +`sameRepositoryIdentity(left, right)` compares `provider` exactly. It compares +`host`, `owner`, and `repository` without case sensitivity. + +## Pull request contract + +### Types + +The module defines these public types: + +```ts +export type PullRequestStatus = "open" | "merged" | "closed-unmerged"; + +export type PullRequestReference = { + targetRepository: RepositoryIdentity; + number: number; +}; + +export type PullRequestSummary = { + number: number; + url: string; + targetRepository: RepositoryIdentity; + baseBranch: string; + headRepository: RepositoryIdentity; + headBranch: string; + headSha: string; + body: string; +} & ( + | { status: "open"; mergeCommit?: undefined } + | { status: "merged"; mergeCommit: string } + | { status: "closed-unmerged"; mergeCommit?: undefined } +); + +export type CreatePullRequestInput = { + title: string; + body: string; + baseBranch: string; + headBranch: string; +}; + +export type FindPullRequestsQuery = { + targetRepository: RepositoryIdentity; + baseBranch: string; + headRepository: RepositoryIdentity; + headBranch: string; +}; +``` + +A merged summary always has a merge commit. An open or closed-unmerged summary +does not have a merge commit. + +### Host interface + +```ts +export interface PullRequestHost { + readonly id: PatchmillHostProviderId; + + resolveTargetRepositoryIdentity(): Promise; + + resolveRemoteRepositoryIdentity(remote: string): Promise; + + createPullRequest(input: CreatePullRequestInput): Promise; + + findPullRequests( + query: FindPullRequestsQuery, + ): Promise; + + getPullRequest(reference: PullRequestReference): Promise; + + readPullRequestBody(reference: PullRequestReference): Promise; + + updatePullRequestBody( + reference: PullRequestReference, + body: string, + ): Promise; +} +``` + +The interface is separate from `RunOnceHostProvider`. Issues #185 and #186 can +implement this interface without temporary production stubs. + +### Completeness semantics + +A successful `findPullRequests` call means that the result is exhaustive. The +adapter must search all pull request states and all available pages. + +The adapter throws `IncompletePullRequestSearchError` if it cannot prove that +the result is complete. A partial result must not appear as a successful result. + +The adapter throws `PullRequestNotFoundError` only after it proves that the +requested pull request does not exist. + +The adapter throws `PullRequestIdentityError` for incomplete identity data or a +repository mismatch. + +The errors expose these stable fields: + +```ts +export class IncompletePullRequestSearchError extends Error { + readonly query: FindPullRequestsQuery; + readonly limit?: number; +} + +export class PullRequestNotFoundError extends Error { + readonly reference: PullRequestReference; +} + +export class PullRequestIdentityError extends Error { + readonly reason: string; + readonly expected?: RepositoryIdentity; + readonly actual?: Partial; +} +``` + +Authentication, transport, rate-limit, and malformed-response errors retain +their original error type. The host contract does not misclassify these errors +as a missing pull request. + +## Planning phases + +The workflow version is `planning-pr-v1`. + +```ts +export const PLANNING_PR_WORKFLOW_VERSION = "planning-pr-v1" as const; + +export type PlanningPhaseKind = "spec" | "plan" | "implementation"; +export type PlanningArtifactKind = "spec" | "plan"; + +export type PlanningGateSnapshot = { + specRequired: boolean; + planRequired: boolean; +}; + +export type PlannedPhase = { + kind: PlanningPhaseKind; + artifactKinds: readonly PlanningArtifactKind[]; + pullRequestRequired: true; +}; + +export function planningPhasePlan( + gates: PlanningGateSnapshot, +): readonly PlannedPhase[]; +``` + +The function returns these exact phase sequences: + +| Spec gate | Plan gate | Phase sequence | +| --------- | --------- | ----------------------------------------------------------------------- | +| Disabled | Disabled | Implementation contains the spec and plan. | +| Enabled | Disabled | Spec contains the spec. Implementation contains the plan. | +| Disabled | Enabled | Plan contains the spec and plan. Implementation follows. | +| Enabled | Enabled | Spec contains the spec. Plan contains the plan. Implementation follows. | + +Every listed phase ends in a pull request. The implementation phase also +contains code, but `artifactKinds` lists planning artifacts only. + +A remote-base artifact can satisfy a planning gate without a new pull request. +Later reconciliation logic owns that decision. The pure phase plan does not +inspect repositories or run state. + +## Phase workspace identity + +`phaseWorkspaceIdentity` uses the existing issue slug and strategy helpers. It +appends the phase suffix after the complete issue branch and worktree name. + +```ts +export function phaseWorkspaceIdentity(input: { + issueNumber: number; + title: string; + phase: PlanningPhaseKind; + strategy: GitWorktreeStrategyConfig; +}): { + branch: string; + worktreePath: string; +}; +``` + +The phase suffix does not count against `slugLength`. + +For issue #184, the names have this form: + +```text +agent/issue-184--spec +.worktrees/patchmill-issue-184--spec +``` + +The `plan` and `implementation` phases use their matching suffix. + +## Planning pull request identity + +### Marker + +The marker format is: + +```html + +``` + +For the spec phase of issue #184, the marker is: + +```html + +``` + +The marker is an invisible pull request body comment. It is not part of the +committed spec or plan. + +The marker helps Patchmill recover from an uncertain create response. A later +run can find the pull request by repository and branch identity. The marker then +proves the workflow version, issue, and phase. + +The parser returns `undefined` when the body has no Patchmill planning marker. +It throws `PlanningPullRequestMarkerError` for these conditions: + +- More than one marker exists. +- A marker is malformed. +- The workflow version is unsupported. +- The issue number is not a positive integer. +- The phase is not `spec`, `plan`, or `implementation`. + +Human-readable title or body text is not an identity source. + +### Titles + +Planning pull request titles use these forms: + +```text +Spec for # +Plan for # +``` + +The title identifies the issue and phase without untrusted issue text. It does +not contain an issue-closing keyword. + +### Bodies + +A planning pull request body contains these items in order: + +1. `Refs #` as a non-closing reference. +2. The planning phase. +3. The repository-relative artifact paths. +4. A statement that the merge unlocks the next phase. +5. The planning pull request marker. + +The renderer preserves the first occurrence of each artifact path. It omits +later duplicate occurrences. It does not inspect the filesystem. + +Only the implementation pull request can use `Closes #`. Implementation +pull request body rules remain outside issue #184. + +## Planning workspace lifecycle + +The workspace module defines an interface. Issue #187 will add the Git +implementation. + +### Identity and snapshots + +```ts +export type PlanningWorkspaceIdentity = { + branch: string; + worktreePath: string; +}; + +export type PlanningWorkspaceSnapshot = + | { + state: "missing"; + identity: PlanningWorkspaceIdentity; + } + | { + state: "branch-only"; + identity: PlanningWorkspaceIdentity; + headSha: string; + } + | { + state: "ready"; + identity: PlanningWorkspaceIdentity; + headSha: string; + clean: boolean; + }; + +export type PreparedPlanningWorkspace = { + created: boolean; + remote: string; + baseBranch: string; + baseSha: string; + snapshot: Extract; +}; +``` + +A valid snapshot has one of three states. Unexpected filesystem and Git +combinations do not create more snapshot variants. + +### Interface + +```ts +export interface PlanningWorkspaceLifecycle { + prepare(input: { + identity: PlanningWorkspaceIdentity; + remote: string; + baseBranch: string; + resume?: { + baseSha: string; + headSha: string; + }; + }): Promise; + + inspect( + identity: PlanningWorkspaceIdentity, + ): Promise; + + removeWorktree( + identity: PlanningWorkspaceIdentity, + ): Promise>; + + removeBranch(input: { + identity: PlanningWorkspaceIdentity; + pushedHeadSha: string; + }): Promise>; +} +``` + +A concrete adapter binds this interface to one repository root. + +`prepare` fetches the configured remote base before it creates a new workspace. +It creates the branch from the fetched remote base commit. It can resume only +when the expected identity and saved commits agree with live Git state. + +`removeWorktree` does not force removal. A dirty worktree blocks the operation. + +`removeBranch` proves that the exact local head exists on the remote branch. It +then removes the local branch. + +The two cleanup operations stay separate. Later run recovery state can record +worktree removal before branch removal. + +Push and pull request creation are not workspace responsibilities. Later +publication code owns those operations. + +### Workspace errors + +`PlanningWorkspaceConflictError` reports unsafe or inconsistent state. Its +reason identifies at least these conditions: + +- The expected path belongs to another branch. +- The expected branch belongs to another worktree. +- An unregistered directory exists at the expected path. +- A worktree is detached. +- A saved base or head commit does not match live state. +- A worktree is dirty during removal. +- The local branch head is not proven on the remote branch. + +The adapter blocks instead of using force flags or deleting uncertain state. + +## Test design + +### Pull request contract tests + +`src/host/pull-requests.test.ts` covers: + +- Case-insensitive comparison of host, owner, and repository. +- Exact comparison of provider IDs. +- The status and merge commit invariants. +- Error fields for incomplete search, missing PR, and identity mismatch. +- A small fake host that satisfies `PullRequestHost`. + +### Pure workflow tests + +`src/workflow/planning-pull-requests.test.ts` covers: + +- The exact phase sequence for all four gate combinations. +- Phase suffix placement outside the configured slug limit. +- Exact marker rendering. +- Valid marker parsing. +- Rejection of duplicate, malformed, and unsupported markers. +- Non-closing planning titles and bodies. +- One rendered list item for each artifact path. + +### Workspace contract tests + +`src/git/planning-workspaces.test.ts` covers: + +- The three snapshot states. +- The return state for each lifecycle operation. +- A small in-memory adapter that satisfies `PlanningWorkspaceLifecycle`. +- Coordinator-style calls through the interface without Git command details. + +The host and workspace test adapters stay local to their test files. A later +issue can extract shared adapters after a second test needs the same behavior. + +### Validation commands + +Implementation work will use these commands: + +```sh +node --test src/host/pull-requests.test.ts +node --test src/workflow/planning-pull-requests.test.ts +node --test src/git/planning-workspaces.test.ts +npm run build +npm run lint +npm test +``` + +These tests pass the Testing Value Gate. They protect reusable contracts, +workflow decisions, parsing, validation, and fail-closed errors. + +No dependency changes are planned. A Nix build is not required for this slice. + +## Module size + +Each production module has one reason to change: + +- Host contract changes affect `src/host/pull-requests.ts`. +- Planning policy changes affect `src/workflow/planning-pull-requests.ts`. +- Workspace lifecycle changes affect `src/git/planning-workspaces.ts`. + +Each production module targets fewer than 200 meaningful lines. If the workflow +module exceeds that target, marker parsing is the first private implementation +to extract. + +## Compatibility + +This slice adds contracts and pure functions only. It does not change current +host adapters, factories, pipeline stages, run state, labels, or publication. + +Existing runtime behavior and public CLI behavior stay unchanged. + +## Acceptance criteria + +- `RepositoryIdentity` contains provider, host, owner, and repository identity. +- Pull request summaries use normalized status and complete repository identity. +- Successful pull request searches are exhaustive. +- Incomplete searches, proven absence, and identity mismatches remain distinct. +- The phase planner covers all four planning gate combinations. +- Planning markers, titles, bodies, and workspace names are deterministic. +- The workspace interface hides Git command sequencing. +- Workspace cleanup blocks unsafe or uncertain deletion. +- Fake host and workspace adapters satisfy the production interfaces. +- Existing runtime behavior stays unchanged. From 928489fb8869c780721b57eb9c7a3bccb50711d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Sat, 5 Sep 2026 09:21:24 +0200 Subject: [PATCH 02/11] feat(host): define pull request contract --- src/host/pull-requests.test.ts | 130 +++++++++++++++++++++++++++++++ src/host/pull-requests.ts | 136 +++++++++++++++++++++++++++++++++ 2 files changed, 266 insertions(+) create mode 100644 src/host/pull-requests.test.ts create mode 100644 src/host/pull-requests.ts diff --git a/src/host/pull-requests.test.ts b/src/host/pull-requests.test.ts new file mode 100644 index 00000000..a34d8525 --- /dev/null +++ b/src/host/pull-requests.test.ts @@ -0,0 +1,130 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { + IncompletePullRequestSearchError, + PullRequestIdentityError, + PullRequestNotFoundError, + sameRepositoryIdentity, + type CreatePullRequestInput, + type FindPullRequestsQuery, + type PullRequestHost, + type PullRequestReference, + type PullRequestSummary, + type RepositoryIdentity, +} from "./pull-requests.ts"; + +const githubRepository: RepositoryIdentity = { + provider: "github-gh", + host: "github.com", + owner: "rochecompaan", + repository: "patchmill", +}; + +test("repository identity comparison normalizes host, owner, and repository case", () => { + assert.equal( + sameRepositoryIdentity(githubRepository, { + provider: "github-gh", + host: "GITHUB.COM", + owner: "RocheCompaan", + repository: "Patchmill", + }), + true, + ); +}); + +test("repository identity comparison requires the same provider", () => { + assert.equal( + sameRepositoryIdentity(githubRepository, { + ...githubRepository, + provider: "forgejo-tea", + }), + false, + ); +}); + +test("incomplete search error keeps the exact query and provider limit", () => { + const query: FindPullRequestsQuery = { + targetRepository: githubRepository, + baseBranch: "main", + headRepository: githubRepository, + headBranch: "agent/issue-184-foundations-spec", + }; + const error = new IncompletePullRequestSearchError(query, 100); + + assert.equal(error.name, "IncompletePullRequestSearchError"); + assert.deepEqual(error.query, query); + assert.equal(error.limit, 100); +}); + +test("not found error keeps the exact pull request reference", () => { + const reference = { + targetRepository: githubRepository, + number: 404, + }; + const error = new PullRequestNotFoundError(reference); + + assert.equal(error.name, "PullRequestNotFoundError"); + assert.deepEqual(error.reference, reference); +}); + +test("identity error keeps expected and partial actual identity", () => { + const error = new PullRequestIdentityError("repository mismatch", { + expected: githubRepository, + actual: { host: "forge.example.test" }, + }); + + assert.equal(error.name, "PullRequestIdentityError"); + assert.equal(error.reason, "repository mismatch"); + assert.deepEqual(error.expected, githubRepository); + assert.deepEqual(error.actual, { host: "forge.example.test" }); +}); + +const openPullRequest = { + number: 12, + url: "https://github.com/rochecompaan/patchmill/pull/12", + status: "open", + targetRepository: githubRepository, + baseBranch: "main", + headRepository: githubRepository, + headBranch: "agent/issue-184-foundations-spec", + headSha: "abc123", + body: "Refs #184", +} satisfies PullRequestSummary; + +function fakePullRequestHost(summary: PullRequestSummary): PullRequestHost { + let body = summary.body; + return { + id: "github-gh", + resolveTargetRepositoryIdentity: async () => githubRepository, + resolveRemoteRepositoryIdentity: async () => githubRepository, + createPullRequest: async (_input: CreatePullRequestInput) => summary, + findPullRequests: async (_query: FindPullRequestsQuery) => [summary], + getPullRequest: async (_reference: PullRequestReference) => summary, + readPullRequestBody: async (_reference: PullRequestReference) => body, + updatePullRequestBody: async ( + _reference: PullRequestReference, + nextBody: string, + ) => { + body = nextBody; + }, + }; +} + +test("a fake host satisfies the same contract as production adapters", async () => { + const host = fakePullRequestHost(openPullRequest); + const reference = { + targetRepository: githubRepository, + number: openPullRequest.number, + }; + const query: FindPullRequestsQuery = { + targetRepository: githubRepository, + baseBranch: "main", + headRepository: githubRepository, + headBranch: openPullRequest.headBranch, + }; + + assert.deepEqual(await host.findPullRequests(query), [openPullRequest]); + assert.deepEqual(await host.getPullRequest(reference), openPullRequest); + await host.updatePullRequestBody(reference, "updated body"); + assert.equal(await host.readPullRequestBody(reference), "updated body"); +}); diff --git a/src/host/pull-requests.ts b/src/host/pull-requests.ts new file mode 100644 index 00000000..521b5522 --- /dev/null +++ b/src/host/pull-requests.ts @@ -0,0 +1,136 @@ +import type { PatchmillHostProviderId } from "../config/types.ts"; + +export type RepositoryIdentity = { + provider: PatchmillHostProviderId; + host: string; + owner: string; + repository: string; +}; + +export type PullRequestStatus = "open" | "merged" | "closed-unmerged"; + +export type PullRequestReference = { + targetRepository: RepositoryIdentity; + number: number; +}; + +type PullRequestSummaryIdentity = { + number: number; + url: string; + targetRepository: RepositoryIdentity; + baseBranch: string; + headRepository: RepositoryIdentity; + headBranch: string; + headSha: string; + body: string; +}; + +export type PullRequestSummary = PullRequestSummaryIdentity & + ( + | { status: "open"; mergeCommit?: undefined } + | { status: "merged"; mergeCommit: string } + | { status: "closed-unmerged"; mergeCommit?: undefined } + ); + +export type CreatePullRequestInput = { + title: string; + body: string; + baseBranch: string; + headBranch: string; +}; + +export type FindPullRequestsQuery = { + targetRepository: RepositoryIdentity; + baseBranch: string; + headRepository: RepositoryIdentity; + headBranch: string; +}; + +function sameCaseInsensitiveValue(left: string, right: string): boolean { + return left.toLowerCase() === right.toLowerCase(); +} + +export function sameRepositoryIdentity( + left: RepositoryIdentity, + right: RepositoryIdentity, +): boolean { + return ( + left.provider === right.provider && + sameCaseInsensitiveValue(left.host, right.host) && + sameCaseInsensitiveValue(left.owner, right.owner) && + sameCaseInsensitiveValue(left.repository, right.repository) + ); +} + +export class IncompletePullRequestSearchError extends Error { + readonly query: FindPullRequestsQuery; + readonly limit?: number; + + constructor(query: FindPullRequestsQuery, limit?: number) { + super( + limit === undefined + ? "Pull request search was incomplete" + : `Pull request search stopped at provider limit ${limit}`, + ); + this.name = "IncompletePullRequestSearchError"; + this.query = query; + if (limit !== undefined) this.limit = limit; + } +} + +export class PullRequestNotFoundError extends Error { + readonly reference: PullRequestReference; + + constructor(reference: PullRequestReference) { + super(`Pull request #${reference.number} was not found`); + this.name = "PullRequestNotFoundError"; + this.reference = reference; + } +} + +export class PullRequestIdentityError extends Error { + readonly reason: string; + readonly expected?: RepositoryIdentity; + readonly actual?: Partial; + + constructor( + reason: string, + identities: { + expected?: RepositoryIdentity; + actual?: Partial; + } = {}, + ) { + super(`Pull request identity is invalid: ${reason}`); + this.name = "PullRequestIdentityError"; + this.reason = reason; + if (identities.expected !== undefined) { + this.expected = identities.expected; + } + if (identities.actual !== undefined) { + this.actual = identities.actual; + } + } +} + +export interface PullRequestHost { + readonly id: PatchmillHostProviderId; + + resolveTargetRepositoryIdentity(): Promise; + + resolveRemoteRepositoryIdentity(remote: string): Promise; + + createPullRequest(input: CreatePullRequestInput): Promise; + + findPullRequests( + query: FindPullRequestsQuery, + ): Promise; + + getPullRequest(reference: PullRequestReference): Promise; + + readPullRequestBody(reference: PullRequestReference): Promise; + + updatePullRequestBody( + reference: PullRequestReference, + body: string, + ): Promise; +} From 38d9487f7f0cffe4167d4519e869d7166cb1c087 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Sat, 5 Sep 2026 09:22:46 +0200 Subject: [PATCH 03/11] feat(workflow): define planning pull request phases --- src/workflow/planning-pull-requests.test.ts | 191 ++++++++++++++++++ src/workflow/planning-pull-requests.ts | 206 ++++++++++++++++++++ 2 files changed, 397 insertions(+) create mode 100644 src/workflow/planning-pull-requests.test.ts create mode 100644 src/workflow/planning-pull-requests.ts diff --git a/src/workflow/planning-pull-requests.test.ts b/src/workflow/planning-pull-requests.test.ts new file mode 100644 index 00000000..b54c2a17 --- /dev/null +++ b/src/workflow/planning-pull-requests.test.ts @@ -0,0 +1,191 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import type { GitWorktreeStrategyConfig } from "../git/types.ts"; +import { + PlanningPullRequestMarkerError, + parsePlanningPullRequestMarker, + phaseWorkspaceIdentity, + planningPhasePlan, + planningPullRequestBody, + planningPullRequestTitle, + renderPlanningPullRequestMarker, + type PlannedPhase, + type PlanningGateSnapshot, +} from "./planning-pull-requests.ts"; + +const phaseCases: Array<{ + gates: PlanningGateSnapshot; + expected: readonly PlannedPhase[]; +}> = [ + { + gates: { specRequired: false, planRequired: false }, + expected: [ + { + kind: "implementation", + artifactKinds: ["spec", "plan"], + pullRequestRequired: true, + }, + ], + }, + { + gates: { specRequired: true, planRequired: false }, + expected: [ + { + kind: "spec", + artifactKinds: ["spec"], + pullRequestRequired: true, + }, + { + kind: "implementation", + artifactKinds: ["plan"], + pullRequestRequired: true, + }, + ], + }, + { + gates: { specRequired: false, planRequired: true }, + expected: [ + { + kind: "plan", + artifactKinds: ["spec", "plan"], + pullRequestRequired: true, + }, + { + kind: "implementation", + artifactKinds: [], + pullRequestRequired: true, + }, + ], + }, + { + gates: { specRequired: true, planRequired: true }, + expected: [ + { + kind: "spec", + artifactKinds: ["spec"], + pullRequestRequired: true, + }, + { + kind: "plan", + artifactKinds: ["plan"], + pullRequestRequired: true, + }, + { + kind: "implementation", + artifactKinds: [], + pullRequestRequired: true, + }, + ], + }, +]; + +test("planning phase plan covers all spec and plan gate combinations", () => { + for (const entry of phaseCases) { + assert.deepEqual(planningPhasePlan(entry.gates), entry.expected); + } +}); + +const strategy: GitWorktreeStrategyConfig = { + baseBranch: "main", + baseRef: "HEAD", + remote: "origin", + branchPrefix: "agent/issue-", + worktreeDir: ".worktrees", + worktreePrefix: "patchmill-issue-", + slugLength: 48, + allowDirectLand: false, +}; + +test("phase workspace identity appends the phase outside the issue slug", () => { + assert.deepEqual( + phaseWorkspaceIdentity({ + issueNumber: 184, + title: "Define provider-neutral planning pull request foundations", + phase: "plan", + strategy, + }), + { + branch: + "agent/issue-184-define-provider-neutral-planning-pull-request-fo-plan", + worktreePath: + ".worktrees/patchmill-issue-184-define-provider-neutral-planning-pull-request-fo-plan", + }, + ); +}); + +test("planning pull request marker renders and parses exact identity", () => { + const marker = renderPlanningPullRequestMarker({ + issueNumber: 184, + phase: "spec", + }); + + assert.equal( + marker, + "", + ); + assert.deepEqual(parsePlanningPullRequestMarker(`Header\n\n${marker}\n`), { + workflowVersion: "planning-pr-v1", + issueNumber: 184, + phase: "spec", + }); +}); + +test("marker parser returns undefined when no planning marker exists", () => { + assert.equal(parsePlanningPullRequestMarker("Refs #184"), undefined); +}); + +test("marker parser rejects duplicate and invalid planning markers", () => { + const invalidBodies = [ + [ + "", + "", + ].join("\n"), + "", + "", + "", + "/u); + assert.doesNotMatch(body, closingKeyword); +}); diff --git a/src/workflow/planning-pull-requests.ts b/src/workflow/planning-pull-requests.ts new file mode 100644 index 00000000..ec447cb5 --- /dev/null +++ b/src/workflow/planning-pull-requests.ts @@ -0,0 +1,206 @@ +import type { GitWorktreeStrategyConfig } from "../git/types.ts"; +import { + buildIssueBranchName, + buildIssueWorktreePath, +} from "../git/worktree-strategy.ts"; + +export const PLANNING_PR_WORKFLOW_VERSION = "planning-pr-v1" as const; + +export type PlanningPhaseKind = "spec" | "plan" | "implementation"; +export type PlanningArtifactKind = "spec" | "plan"; + +export type PlanningGateSnapshot = { + specRequired: boolean; + planRequired: boolean; +}; + +export type PlannedPhase = { + kind: PlanningPhaseKind; + artifactKinds: readonly PlanningArtifactKind[]; + pullRequestRequired: true; +}; + +export function planningPhasePlan( + gates: PlanningGateSnapshot, +): readonly PlannedPhase[] { + if (gates.specRequired && gates.planRequired) { + return [ + { + kind: "spec", + artifactKinds: ["spec"], + pullRequestRequired: true, + }, + { + kind: "plan", + artifactKinds: ["plan"], + pullRequestRequired: true, + }, + { + kind: "implementation", + artifactKinds: [], + pullRequestRequired: true, + }, + ]; + } + if (gates.specRequired) { + return [ + { + kind: "spec", + artifactKinds: ["spec"], + pullRequestRequired: true, + }, + { + kind: "implementation", + artifactKinds: ["plan"], + pullRequestRequired: true, + }, + ]; + } + if (gates.planRequired) { + return [ + { + kind: "plan", + artifactKinds: ["spec", "plan"], + pullRequestRequired: true, + }, + { + kind: "implementation", + artifactKinds: [], + pullRequestRequired: true, + }, + ]; + } + return [ + { + kind: "implementation", + artifactKinds: ["spec", "plan"], + pullRequestRequired: true, + }, + ]; +} + +export function phaseWorkspaceIdentity(input: { + issueNumber: number; + title: string; + phase: PlanningPhaseKind; + strategy: GitWorktreeStrategyConfig; +}): { branch: string; worktreePath: string } { + const branch = buildIssueBranchName( + input.issueNumber, + input.title, + input.strategy, + ); + const worktreePath = buildIssueWorktreePath( + input.issueNumber, + input.title, + input.strategy, + ); + return { + branch: `${branch}-${input.phase}`, + worktreePath: `${worktreePath}-${input.phase}`, + }; +} + +const markerCandidatePattern = //gu; +const validMarkerPattern = + /^$/u; +const markerPrefix = "patchmill:planning-pr-"; + +export class PlanningPullRequestMarkerError extends Error { + readonly reason: string; + readonly marker: string; + + constructor(reason: string, marker: string) { + super(`Planning pull request marker is invalid: ${reason}`); + this.name = "PlanningPullRequestMarkerError"; + this.reason = reason; + this.marker = marker; + } +} + +function assertPositiveIssueNumber(issueNumber: number): void { + if (!Number.isSafeInteger(issueNumber) || issueNumber < 1) { + throw new RangeError("Issue number must be a positive safe integer"); + } +} + +export function renderPlanningPullRequestMarker(input: { + issueNumber: number; + phase: PlanningPhaseKind; +}): string { + assertPositiveIssueNumber(input.issueNumber); + return ``; +} + +export function parsePlanningPullRequestMarker(body: string): + | { + workflowVersion: typeof PLANNING_PR_WORKFLOW_VERSION; + issueNumber: number; + phase: PlanningPhaseKind; + } + | undefined { + const candidates = body.match(markerCandidatePattern) ?? []; + if (candidates.length === 0) { + if (body.includes(markerPrefix)) { + throw new PlanningPullRequestMarkerError("malformed marker", body); + } + return undefined; + } + if (candidates.length !== 1) { + throw new PlanningPullRequestMarkerError( + "multiple markers", + candidates.join("\n"), + ); + } + const marker = candidates[0]!; + const match = validMarkerPattern.exec(marker); + if (!match) { + throw new PlanningPullRequestMarkerError("unsupported marker", marker); + } + const issueNumber = Number(match[2]); + if (!Number.isSafeInteger(issueNumber)) { + throw new PlanningPullRequestMarkerError("invalid issue number", marker); + } + return { + workflowVersion: PLANNING_PR_WORKFLOW_VERSION, + issueNumber, + phase: match[3] as PlanningPhaseKind, + }; +} + +export function planningPullRequestTitle(input: { + issueNumber: number; + phase: PlanningArtifactKind; +}): string { + assertPositiveIssueNumber(input.issueNumber); + const label = input.phase === "spec" ? "Spec" : "Plan"; + return `${label} for #${input.issueNumber}`; +} + +export function planningPullRequestBody(input: { + issueNumber: number; + phase: PlanningArtifactKind; + artifactPaths: readonly string[]; +}): string { + assertPositiveIssueNumber(input.issueNumber); + const artifactPaths = [...new Set(input.artifactPaths)]; + if (artifactPaths.length === 0) { + throw new RangeError("Planning pull request requires an artifact path"); + } + const label = input.phase === "spec" ? "Spec" : "Plan"; + return [ + `Refs #${input.issueNumber}`, + "", + "## Planning phase", + "", + label, + "", + "## Artifacts", + "", + ...artifactPaths.map((path) => `- \`${path}\``), + "", + "Merge this pull request to unlock the next phase.", + "", + renderPlanningPullRequestMarker(input), + ].join("\n"); +} From e6c910dda06481ac34a31ad12b25a28ec0695d38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Sat, 5 Sep 2026 09:23:35 +0200 Subject: [PATCH 04/11] feat(git): define planning workspace lifecycle --- src/git/planning-workspaces.test.ts | 128 ++++++++++++++++++++++++++++ src/git/planning-workspaces.ts | 79 +++++++++++++++++ 2 files changed, 207 insertions(+) create mode 100644 src/git/planning-workspaces.test.ts create mode 100644 src/git/planning-workspaces.ts diff --git a/src/git/planning-workspaces.test.ts b/src/git/planning-workspaces.test.ts new file mode 100644 index 00000000..c4a614a1 --- /dev/null +++ b/src/git/planning-workspaces.test.ts @@ -0,0 +1,128 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { + PlanningWorkspaceConflictError, + type PlanningWorkspaceIdentity, + type PlanningWorkspaceLifecycle, + type PlanningWorkspaceSnapshot, + type PreparedPlanningWorkspace, +} from "./planning-workspaces.ts"; + +const identity: PlanningWorkspaceIdentity = { + branch: "agent/issue-184-foundations-spec", + worktreePath: ".worktrees/patchmill-issue-184-foundations-spec", +}; + +class FakePlanningWorkspace implements PlanningWorkspaceLifecycle { + readonly events: string[] = []; + private snapshot: PlanningWorkspaceSnapshot = { + state: "missing", + identity, + }; + + async prepare(input: { + identity: PlanningWorkspaceIdentity; + remote: string; + baseBranch: string; + resume?: { baseSha: string; headSha: string }; + }): Promise { + this.events.push("prepare"); + const headSha = input.resume?.headSha ?? "head-1"; + const snapshot: Extract = { + state: "ready", + identity: input.identity, + headSha, + clean: true, + }; + this.snapshot = snapshot; + return { + created: input.resume === undefined, + remote: input.remote, + baseBranch: input.baseBranch, + baseSha: input.resume?.baseSha ?? "base-1", + snapshot, + }; + } + + async inspect( + _identity: PlanningWorkspaceIdentity, + ): Promise { + this.events.push("inspect"); + return this.snapshot; + } + + async removeWorktree( + _identity: PlanningWorkspaceIdentity, + ): Promise> { + this.events.push("remove-worktree"); + if (this.snapshot.state !== "ready") { + throw new PlanningWorkspaceConflictError( + "branch-owned-by-other-worktree", + identity, + ); + } + if (!this.snapshot.clean) { + throw new PlanningWorkspaceConflictError("dirty-worktree", identity); + } + this.snapshot = { + state: "branch-only", + identity, + headSha: this.snapshot.headSha, + }; + return this.snapshot; + } + + async removeBranch(input: { + identity: PlanningWorkspaceIdentity; + pushedHeadSha: string; + }): Promise> { + this.events.push("remove-branch"); + if ( + this.snapshot.state !== "branch-only" || + this.snapshot.headSha !== input.pushedHeadSha + ) { + throw new PlanningWorkspaceConflictError("head-not-pushed", identity); + } + this.snapshot = { state: "missing", identity: input.identity }; + return this.snapshot; + } +} + +test("a coordinator can use the workspace lifecycle without Git commands", async () => { + const workspace = new FakePlanningWorkspace(); + const prepared = await workspace.prepare({ + identity, + remote: "origin", + baseBranch: "main", + }); + + assert.equal(prepared.created, true); + assert.equal(prepared.baseSha, "base-1"); + assert.deepEqual(await workspace.inspect(identity), prepared.snapshot); + + const branchOnly = await workspace.removeWorktree(identity); + assert.equal(branchOnly.state, "branch-only"); + + const missing = await workspace.removeBranch({ + identity, + pushedHeadSha: "head-1", + }); + assert.equal(missing.state, "missing"); + assert.deepEqual(workspace.events, [ + "prepare", + "inspect", + "remove-worktree", + "remove-branch", + ]); +}); + +test("workspace conflict error keeps the reason and identity", () => { + const error = new PlanningWorkspaceConflictError( + "unregistered-path", + identity, + ); + + assert.equal(error.name, "PlanningWorkspaceConflictError"); + assert.equal(error.reason, "unregistered-path"); + assert.deepEqual(error.identity, identity); +}); diff --git a/src/git/planning-workspaces.ts b/src/git/planning-workspaces.ts new file mode 100644 index 00000000..547fd8a8 --- /dev/null +++ b/src/git/planning-workspaces.ts @@ -0,0 +1,79 @@ +export type PlanningWorkspaceIdentity = { + readonly branch: string; + readonly worktreePath: string; +}; + +export type PlanningWorkspaceSnapshot = + | { + readonly state: "missing"; + readonly identity: PlanningWorkspaceIdentity; + } + | { + readonly state: "branch-only"; + readonly identity: PlanningWorkspaceIdentity; + readonly headSha: string; + } + | { + readonly state: "ready"; + readonly identity: PlanningWorkspaceIdentity; + readonly headSha: string; + readonly clean: boolean; + }; + +export type PreparedPlanningWorkspace = { + readonly created: boolean; + readonly remote: string; + readonly baseBranch: string; + readonly baseSha: string; + readonly snapshot: Extract; +}; + +export type PlanningWorkspaceConflictReason = + | "path-owned-by-other-branch" + | "branch-owned-by-other-worktree" + | "unregistered-path" + | "detached-worktree" + | "base-sha-mismatch" + | "head-sha-mismatch" + | "dirty-worktree" + | "head-not-pushed"; + +export class PlanningWorkspaceConflictError extends Error { + readonly reason: PlanningWorkspaceConflictReason; + readonly identity: PlanningWorkspaceIdentity; + + constructor( + reason: PlanningWorkspaceConflictReason, + identity: PlanningWorkspaceIdentity, + ) { + super(`Planning workspace is unsafe: ${reason}`); + this.name = "PlanningWorkspaceConflictError"; + this.reason = reason; + this.identity = identity; + } +} + +export interface PlanningWorkspaceLifecycle { + prepare(input: { + identity: PlanningWorkspaceIdentity; + remote: string; + baseBranch: string; + resume?: { + baseSha: string; + headSha: string; + }; + }): Promise; + + inspect( + identity: PlanningWorkspaceIdentity, + ): Promise; + + removeWorktree( + identity: PlanningWorkspaceIdentity, + ): Promise>; + + removeBranch(input: { + identity: PlanningWorkspaceIdentity; + pushedHeadSha: string; + }): Promise>; +} From 7b00b63b998c80cc94dee4387804600ea6359e41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Sat, 5 Sep 2026 09:39:11 +0200 Subject: [PATCH 05/11] fix(workflow): harden planning pull request bodies --- package.json | 3 +- src/workflow/planning-pull-requests.test.ts | 34 +++++++++++++++++++++ src/workflow/planning-pull-requests.ts | 32 ++++++++++++++----- tsconfig.contract-tests.json | 14 +++++++++ 4 files changed, 74 insertions(+), 9 deletions(-) create mode 100644 tsconfig.contract-tests.json diff --git a/package.json b/package.json index 926a0d19..ed485168 100644 --- a/package.json +++ b/package.json @@ -54,7 +54,7 @@ "format:check": "prettier --check .", "prepare": "husky", "pretest": "npm run check:dependencies", - "test": "node --test \"bin/*.test.ts\" \"src/**/*.test.ts\" \"test-support/*.test.ts\" \"scripts/*.test.mjs\"", + "test": "npm run check:contract-tests && node --test \"bin/*.test.ts\" \"src/**/*.test.ts\" \"test-support/*.test.ts\" \"scripts/*.test.mjs\"", "test:coverage": "node --test --experimental-test-coverage --test-coverage-include='bin/**/*.ts' --test-coverage-include='src/**/*.ts' --test-coverage-include='test-support/**/*.ts' --test-coverage-exclude='**/*.test.ts' \"bin/*.test.ts\" \"src/**/*.test.ts\" \"test-support/*.test.ts\"", "check:architecture": "depcruise --config dependency-cruiser.config.mjs bin src extensions", "check:coverage": "c8 node --test \"bin/*.test.ts\" \"src/**/*.test.ts\" \"test-support/*.test.ts\"", @@ -65,6 +65,7 @@ "test:mutation": "node --test src/policy/todo-statuses.test.ts quality/property/todo-statuses.property.test.ts", "check:static": "eslint \"{bin,src,test-support,quality}/**/*.ts\" --max-warnings=0", "check:types": "tsc -p tsconfig.quality.json", + "check:contract-tests": "tsc -p tsconfig.contract-tests.json", "test:cli": "node --test bin/*.test.ts src/cli/main.test.ts", "test:triage": "node --test src/cli/commands/triage/*.test.ts", "test:run-once": "node --test src/cli/commands/run-once/*.test.ts", diff --git a/src/workflow/planning-pull-requests.test.ts b/src/workflow/planning-pull-requests.test.ts index b54c2a17..3b627d7e 100644 --- a/src/workflow/planning-pull-requests.test.ts +++ b/src/workflow/planning-pull-requests.test.ts @@ -154,6 +154,18 @@ test("marker parser rejects duplicate and invalid planning markers", () => { } }); +test("marker parser rejects a malformed marker alongside a valid marker", () => { + const body = [ + "", + "/u); assert.doesNotMatch(body, closingKeyword); }); + +test("planning body contains adversarial artifact paths in a single code span", () => { + const artifactPath = + "docs/specs/report`\nCloses #184\n.md"; + const body = planningPullRequestBody({ + issueNumber: 184, + phase: "spec", + artifactPaths: [artifactPath], + }); + + assert.match(body, /^- ``docs\/specs\/report`$/mu); + assert.match(body, /^Closes #184$/mu); + assert.match( + body, + /^\.md``$/mu, + ); + assert.deepEqual(parsePlanningPullRequestMarker(body), { + workflowVersion: "planning-pr-v1", + issueNumber: 184, + phase: "spec", + }); +}); diff --git a/src/workflow/planning-pull-requests.ts b/src/workflow/planning-pull-requests.ts index ec447cb5..5deea9a9 100644 --- a/src/workflow/planning-pull-requests.ts +++ b/src/workflow/planning-pull-requests.ts @@ -102,9 +102,9 @@ export function phaseWorkspaceIdentity(input: { } const markerCandidatePattern = //gu; +const markerStartPattern = /$/u; -const markerPrefix = "patchmill:planning-pr-"; export class PlanningPullRequestMarkerError extends Error { readonly reason: string; @@ -132,6 +132,15 @@ export function renderPlanningPullRequestMarker(input: { return ``; } +function withoutMarkdownCodeSpans(body: string): string { + let codeSpan = /(`+)[\s\S]*?\1/u.exec(body); + while (codeSpan) { + body = body.replace(codeSpan[0], ""); + codeSpan = /(`+)[\s\S]*?\1/u.exec(body); + } + return body; +} + export function parsePlanningPullRequestMarker(body: string): | { workflowVersion: typeof PLANNING_PR_WORKFLOW_VERSION; @@ -139,12 +148,12 @@ export function parsePlanningPullRequestMarker(body: string): phase: PlanningPhaseKind; } | undefined { - const candidates = body.match(markerCandidatePattern) ?? []; - if (candidates.length === 0) { - if (body.includes(markerPrefix)) { - throw new PlanningPullRequestMarkerError("malformed marker", body); - } - return undefined; + const markerBody = withoutMarkdownCodeSpans(body); + const candidates = markerBody.match(markerCandidatePattern) ?? []; + const starts = markerBody.match(markerStartPattern) ?? []; + if (starts.length === 0) return undefined; + if (starts.length !== candidates.length) { + throw new PlanningPullRequestMarkerError("malformed marker", markerBody); } if (candidates.length !== 1) { throw new PlanningPullRequestMarkerError( @@ -177,6 +186,13 @@ export function planningPullRequestTitle(input: { return `${label} for #${input.issueNumber}`; } +function markdownCodeSpan(value: string): string { + const delimiter = "`".repeat( + 1 + Math.max(0, ...(value.match(/`+/gu) ?? []).map((run) => run.length)), + ); + return `${delimiter}${value}${delimiter}`; +} + export function planningPullRequestBody(input: { issueNumber: number; phase: PlanningArtifactKind; @@ -197,7 +213,7 @@ export function planningPullRequestBody(input: { "", "## Artifacts", "", - ...artifactPaths.map((path) => `- \`${path}\``), + ...artifactPaths.map((path) => `- ${markdownCodeSpan(path)}`), "", "Merge this pull request to unlock the next phase.", "", diff --git a/tsconfig.contract-tests.json b/tsconfig.contract-tests.json new file mode 100644 index 00000000..1ca001c8 --- /dev/null +++ b/tsconfig.contract-tests.json @@ -0,0 +1,14 @@ +{ + "extends": "./tsconfig.quality.json", + "compilerOptions": { + "typeRoots": [ + "./node_modules/@earendil-works/pi-coding-agent/node_modules/@types" + ], + "types": ["node"] + }, + "include": [ + "src/git/planning-workspaces.test.ts", + "src/host/pull-requests.test.ts" + ], + "exclude": [] +} From dd546d656cf8c0122017accd504083080a76ae7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Sat, 5 Sep 2026 09:55:48 +0200 Subject: [PATCH 06/11] fix(workflow): harden planning marker parsing --- src/host/pull-requests.test.ts | 31 ++++++++++++ src/workflow/planning-pull-requests.test.ts | 43 +++++++++++++--- src/workflow/planning-pull-requests.ts | 55 ++++++++++++++++++--- 3 files changed, 116 insertions(+), 13 deletions(-) diff --git a/src/host/pull-requests.test.ts b/src/host/pull-requests.test.ts index a34d8525..e3b2ba18 100644 --- a/src/host/pull-requests.test.ts +++ b/src/host/pull-requests.test.ts @@ -91,6 +91,37 @@ const openPullRequest = { body: "Refs #184", } satisfies PullRequestSummary; +const mergedPullRequest = { + ...openPullRequest, + status: "merged", + mergeCommit: "def456", +} satisfies PullRequestSummary; + +const closedPullRequest = { + ...openPullRequest, + status: "closed-unmerged", +} satisfies PullRequestSummary; + +// @ts-expect-error A merged pull request always has a merge commit. +void ({ ...openPullRequest, status: "merged" } satisfies PullRequestSummary); +void ({ + ...openPullRequest, + mergeCommit: "def456", + // @ts-expect-error An open pull request never has a merge commit. +} satisfies PullRequestSummary); +void ({ + ...openPullRequest, + status: "closed-unmerged", + mergeCommit: "def456", + // @ts-expect-error A closed-unmerged pull request never has a merge commit. +} satisfies PullRequestSummary); + +test("pull request summary status preserves merge commit invariants", () => { + assert.equal(mergedPullRequest.mergeCommit, "def456"); + assert.equal("mergeCommit" in openPullRequest, false); + assert.equal("mergeCommit" in closedPullRequest, false); +}); + function fakePullRequestHost(summary: PullRequestSummary): PullRequestHost { let body = summary.body; return { diff --git a/src/workflow/planning-pull-requests.test.ts b/src/workflow/planning-pull-requests.test.ts index 3b627d7e..d8d1d084 100644 --- a/src/workflow/planning-pull-requests.test.ts +++ b/src/workflow/planning-pull-requests.test.ts @@ -134,6 +134,31 @@ test("marker parser returns undefined when no planning marker exists", () => { assert.equal(parsePlanningPullRequestMarker("Refs #184"), undefined); }); +test("marker parser skips only valid unescaped code spans", () => { + const marker = renderPlanningPullRequestMarker({ + issueNumber: 184, + phase: "spec", + }); + const identity = { + workflowVersion: "planning-pr-v1", + issueNumber: 184, + phase: "spec", + } as const; + + const escapedBacktick = "\\`"; + assert.equal(parsePlanningPullRequestMarker(`\`${marker}\``), undefined); + assert.deepEqual( + parsePlanningPullRequestMarker( + `${escapedBacktick}${marker}${escapedBacktick}`, + ), + identity, + ); + assert.deepEqual( + parsePlanningPullRequestMarker("`" + marker + "```"), + identity, + ); +}); + test("marker parser rejects duplicate and invalid planning markers", () => { const invalidBodies = [ [ @@ -202,20 +227,24 @@ test("planning body is non-closing and lists each artifact path once", () => { assert.doesNotMatch(body, closingKeyword); }); -test("planning body contains adversarial artifact paths in a single code span", () => { - const artifactPath = - "docs/specs/report`\nCloses #184\n.md"; +test("planning body safely contains leading, trailing, and multi-backtick artifact paths", () => { + const artifactPaths = [ + "`docs/specs/leading.md\nCloses #184\ntrailing.md`", + "``docs/specs/multi.md\nCloses #184\ntrailing.md``", + ]; const body = planningPullRequestBody({ issueNumber: 184, phase: "spec", - artifactPaths: [artifactPath], + artifactPaths, }); - assert.match(body, /^- ``docs\/specs\/report`$/mu); - assert.match(body, /^Closes #184$/mu); assert.match( body, - /^\.md``$/mu, + /^- `` `docs\/specs\/leading\.md\nCloses #184\ntrailing\.md` ``$/mu, + ); + assert.match( + body, + /^- ``` ``docs\/specs\/multi\.md\nCloses #184\ntrailing\.md`` ```$/mu, ); assert.deepEqual(parsePlanningPullRequestMarker(body), { workflowVersion: "planning-pr-v1", diff --git a/src/workflow/planning-pull-requests.ts b/src/workflow/planning-pull-requests.ts index 5deea9a9..f9df70d4 100644 --- a/src/workflow/planning-pull-requests.ts +++ b/src/workflow/planning-pull-requests.ts @@ -132,13 +132,56 @@ export function renderPlanningPullRequestMarker(input: { return ``; } +function isEscaped(value: string, index: number): boolean { + let backslashCount = 0; + for ( + let cursor = index - 1; + cursor >= 0 && value[cursor] === "\\"; + cursor -= 1 + ) { + backslashCount += 1; + } + return backslashCount % 2 === 1; +} + +function backtickRunEnd(value: string, start: number): number { + let end = start; + while (value[end] === "`") end += 1; + return end; +} + +function codeSpanEnd( + value: string, + start: number, + delimiterLength: number, +): number | undefined { + for (let cursor = start; cursor < value.length; cursor += 1) { + if (value[cursor] !== "`" || isEscaped(value, cursor)) continue; + const end = backtickRunEnd(value, cursor); + if (end - cursor === delimiterLength) return end; + cursor = end - 1; + } + return undefined; +} + function withoutMarkdownCodeSpans(body: string): string { - let codeSpan = /(`+)[\s\S]*?\1/u.exec(body); - while (codeSpan) { - body = body.replace(codeSpan[0], ""); - codeSpan = /(`+)[\s\S]*?\1/u.exec(body); + let visibleText = ""; + for (let cursor = 0; cursor < body.length; cursor += 1) { + if (body[cursor] !== "`" || isEscaped(body, cursor)) { + visibleText += body[cursor]; + continue; + } + const openerEnd = backtickRunEnd(body, cursor); + const closerEnd = codeSpanEnd(body, openerEnd, openerEnd - cursor); + if (closerEnd === undefined) { + visibleText += body.slice(cursor, openerEnd); + cursor = openerEnd - 1; + continue; + } + visibleText += " "; + cursor = closerEnd - 1; } - return body; + return visibleText; } export function parsePlanningPullRequestMarker(body: string): @@ -190,7 +233,7 @@ function markdownCodeSpan(value: string): string { const delimiter = "`".repeat( 1 + Math.max(0, ...(value.match(/`+/gu) ?? []).map((run) => run.length)), ); - return `${delimiter}${value}${delimiter}`; + return `${delimiter} ${value} ${delimiter}`; } export function planningPullRequestBody(input: { From b926c0bdcbf33c55d9f4d0cdf94a541bb4ae48f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Sat, 5 Sep 2026 10:28:40 +0200 Subject: [PATCH 07/11] fix(workflow): ignore markers in code blocks --- src/workflow/planning-pull-request-markers.ts | 151 ++++++++++++++++++ src/workflow/planning-pull-requests.test.ts | 25 +++ src/workflow/planning-pull-requests.ts | 128 ++------------- 3 files changed, 188 insertions(+), 116 deletions(-) create mode 100644 src/workflow/planning-pull-request-markers.ts diff --git a/src/workflow/planning-pull-request-markers.ts b/src/workflow/planning-pull-request-markers.ts new file mode 100644 index 00000000..2ea5b054 --- /dev/null +++ b/src/workflow/planning-pull-request-markers.ts @@ -0,0 +1,151 @@ +export const PLANNING_PR_WORKFLOW_VERSION = "planning-pr-v1" as const; + +export type PlanningPhaseKind = "spec" | "plan" | "implementation"; + +export class PlanningPullRequestMarkerError extends Error { + readonly reason: string; + readonly marker: string; + + constructor(reason: string, marker: string) { + super(`Planning pull request marker is invalid: ${reason}`); + this.name = "PlanningPullRequestMarkerError"; + this.reason = reason; + this.marker = marker; + } +} + +function assertPositiveIssueNumber(issueNumber: number): void { + if (!Number.isSafeInteger(issueNumber) || issueNumber < 1) { + throw new RangeError("Issue number must be a positive safe integer"); + } +} + +export function renderPlanningPullRequestMarker(input: { + issueNumber: number; + phase: PlanningPhaseKind; +}): string { + assertPositiveIssueNumber(input.issueNumber); + return ``; +} + +const markerCandidatePattern = //gu; +const markerStartPattern = /$/u; +const fencedCodeStartPattern = /^ {0,3}(`{3,}|~{3,})/u; +const fencedCodeEndPattern = /^ {0,3}(`+|~+)\s*$/u; +const indentedCodePattern = /^(?: {4}|\t)/u; + +function withoutMarkdownCodeBlocks(body: string): string { + let fence: string | undefined; + return body + .split("\n") + .map((line) => { + if (fence !== undefined) { + const closingFence = line.match(fencedCodeEndPattern)?.[1]; + if ( + closingFence !== undefined && + closingFence[0] === fence[0] && + closingFence.length >= fence.length + ) { + fence = undefined; + } + return ""; + } + const openingFence = line.match(fencedCodeStartPattern)?.[1]; + if (openingFence !== undefined) { + fence = openingFence; + return ""; + } + return indentedCodePattern.test(line) ? "" : line; + }) + .join("\n"); +} + +function isEscaped(value: string, index: number): boolean { + let backslashCount = 0; + for ( + let cursor = index - 1; + cursor >= 0 && value[cursor] === "\\"; + cursor -= 1 + ) { + backslashCount += 1; + } + return backslashCount % 2 === 1; +} + +function backtickRunEnd(value: string, start: number): number { + let end = start; + while (value[end] === "`") end += 1; + return end; +} + +function codeSpanEnd( + value: string, + start: number, + delimiterLength: number, +): number | undefined { + for (let cursor = start; cursor < value.length; cursor += 1) { + if (value[cursor] !== "`" || isEscaped(value, cursor)) continue; + const end = backtickRunEnd(value, cursor); + if (end - cursor === delimiterLength) return end; + cursor = end - 1; + } + return undefined; +} + +function withoutMarkdownCodeSpans(body: string): string { + let visibleText = ""; + for (let cursor = 0; cursor < body.length; cursor += 1) { + if (body[cursor] !== "`" || isEscaped(body, cursor)) { + visibleText += body[cursor]; + continue; + } + const openerEnd = backtickRunEnd(body, cursor); + const closerEnd = codeSpanEnd(body, openerEnd, openerEnd - cursor); + if (closerEnd === undefined) { + visibleText += body.slice(cursor, openerEnd); + cursor = openerEnd - 1; + continue; + } + visibleText += " "; + cursor = closerEnd - 1; + } + return visibleText; +} + +export function parsePlanningPullRequestMarker(body: string): + | { + workflowVersion: typeof PLANNING_PR_WORKFLOW_VERSION; + issueNumber: number; + phase: PlanningPhaseKind; + } + | undefined { + const markerBody = withoutMarkdownCodeSpans(withoutMarkdownCodeBlocks(body)); + const candidates = markerBody.match(markerCandidatePattern) ?? []; + const starts = markerBody.match(markerStartPattern) ?? []; + if (starts.length === 0) return undefined; + if (starts.length !== candidates.length) { + throw new PlanningPullRequestMarkerError("malformed marker", markerBody); + } + if (candidates.length !== 1) { + throw new PlanningPullRequestMarkerError( + "multiple markers", + candidates.join("\n"), + ); + } + const marker = candidates[0]!; + const match = validMarkerPattern.exec(marker); + if (!match) { + throw new PlanningPullRequestMarkerError("unsupported marker", marker); + } + const issueNumber = Number(match[2]); + if (!Number.isSafeInteger(issueNumber)) { + throw new PlanningPullRequestMarkerError("invalid issue number", marker); + } + return { + workflowVersion: PLANNING_PR_WORKFLOW_VERSION, + issueNumber, + phase: match[3] as PlanningPhaseKind, + }; +} diff --git a/src/workflow/planning-pull-requests.test.ts b/src/workflow/planning-pull-requests.test.ts index d8d1d084..694fbeca 100644 --- a/src/workflow/planning-pull-requests.test.ts +++ b/src/workflow/planning-pull-requests.test.ts @@ -159,6 +159,31 @@ test("marker parser skips only valid unescaped code spans", () => { ); }); +test("marker parser ignores fenced and indented Markdown code blocks", () => { + const marker = renderPlanningPullRequestMarker({ + issueNumber: 184, + phase: "spec", + }); + + const codeBodies = [ + ["```html", marker, "```"].join("\n"), + ["~~~html", marker, "~~~"].join("\n"), + ` ${marker}`, + `\t${marker}`, + ]; + for (const body of codeBodies) { + assert.equal(parsePlanningPullRequestMarker(body), undefined); + } + assert.deepEqual( + parsePlanningPullRequestMarker(["```", marker, "```", marker].join("\n")), + { + workflowVersion: "planning-pr-v1", + issueNumber: 184, + phase: "spec", + }, + ); +}); + test("marker parser rejects duplicate and invalid planning markers", () => { const invalidBodies = [ [ diff --git a/src/workflow/planning-pull-requests.ts b/src/workflow/planning-pull-requests.ts index f9df70d4..7724fe44 100644 --- a/src/workflow/planning-pull-requests.ts +++ b/src/workflow/planning-pull-requests.ts @@ -3,10 +3,19 @@ import { buildIssueBranchName, buildIssueWorktreePath, } from "../git/worktree-strategy.ts"; +import { + renderPlanningPullRequestMarker, + type PlanningPhaseKind, +} from "./planning-pull-request-markers.ts"; + +export { + PLANNING_PR_WORKFLOW_VERSION, + PlanningPullRequestMarkerError, + parsePlanningPullRequestMarker, + renderPlanningPullRequestMarker, +} from "./planning-pull-request-markers.ts"; +export type { PlanningPhaseKind } from "./planning-pull-request-markers.ts"; -export const PLANNING_PR_WORKFLOW_VERSION = "planning-pr-v1" as const; - -export type PlanningPhaseKind = "spec" | "plan" | "implementation"; export type PlanningArtifactKind = "spec" | "plan"; export type PlanningGateSnapshot = { @@ -101,125 +110,12 @@ export function phaseWorkspaceIdentity(input: { }; } -const markerCandidatePattern = //gu; -const markerStartPattern = /$/u; - -export class PlanningPullRequestMarkerError extends Error { - readonly reason: string; - readonly marker: string; - - constructor(reason: string, marker: string) { - super(`Planning pull request marker is invalid: ${reason}`); - this.name = "PlanningPullRequestMarkerError"; - this.reason = reason; - this.marker = marker; - } -} - function assertPositiveIssueNumber(issueNumber: number): void { if (!Number.isSafeInteger(issueNumber) || issueNumber < 1) { throw new RangeError("Issue number must be a positive safe integer"); } } -export function renderPlanningPullRequestMarker(input: { - issueNumber: number; - phase: PlanningPhaseKind; -}): string { - assertPositiveIssueNumber(input.issueNumber); - return ``; -} - -function isEscaped(value: string, index: number): boolean { - let backslashCount = 0; - for ( - let cursor = index - 1; - cursor >= 0 && value[cursor] === "\\"; - cursor -= 1 - ) { - backslashCount += 1; - } - return backslashCount % 2 === 1; -} - -function backtickRunEnd(value: string, start: number): number { - let end = start; - while (value[end] === "`") end += 1; - return end; -} - -function codeSpanEnd( - value: string, - start: number, - delimiterLength: number, -): number | undefined { - for (let cursor = start; cursor < value.length; cursor += 1) { - if (value[cursor] !== "`" || isEscaped(value, cursor)) continue; - const end = backtickRunEnd(value, cursor); - if (end - cursor === delimiterLength) return end; - cursor = end - 1; - } - return undefined; -} - -function withoutMarkdownCodeSpans(body: string): string { - let visibleText = ""; - for (let cursor = 0; cursor < body.length; cursor += 1) { - if (body[cursor] !== "`" || isEscaped(body, cursor)) { - visibleText += body[cursor]; - continue; - } - const openerEnd = backtickRunEnd(body, cursor); - const closerEnd = codeSpanEnd(body, openerEnd, openerEnd - cursor); - if (closerEnd === undefined) { - visibleText += body.slice(cursor, openerEnd); - cursor = openerEnd - 1; - continue; - } - visibleText += " "; - cursor = closerEnd - 1; - } - return visibleText; -} - -export function parsePlanningPullRequestMarker(body: string): - | { - workflowVersion: typeof PLANNING_PR_WORKFLOW_VERSION; - issueNumber: number; - phase: PlanningPhaseKind; - } - | undefined { - const markerBody = withoutMarkdownCodeSpans(body); - const candidates = markerBody.match(markerCandidatePattern) ?? []; - const starts = markerBody.match(markerStartPattern) ?? []; - if (starts.length === 0) return undefined; - if (starts.length !== candidates.length) { - throw new PlanningPullRequestMarkerError("malformed marker", markerBody); - } - if (candidates.length !== 1) { - throw new PlanningPullRequestMarkerError( - "multiple markers", - candidates.join("\n"), - ); - } - const marker = candidates[0]!; - const match = validMarkerPattern.exec(marker); - if (!match) { - throw new PlanningPullRequestMarkerError("unsupported marker", marker); - } - const issueNumber = Number(match[2]); - if (!Number.isSafeInteger(issueNumber)) { - throw new PlanningPullRequestMarkerError("invalid issue number", marker); - } - return { - workflowVersion: PLANNING_PR_WORKFLOW_VERSION, - issueNumber, - phase: match[3] as PlanningPhaseKind, - }; -} - export function planningPullRequestTitle(input: { issueNumber: number; phase: PlanningArtifactKind; From ddb8c60ed8ad7161b9757967a2f2b2e799d77c42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Sat, 5 Sep 2026 11:07:04 +0200 Subject: [PATCH 08/11] fix(workflow): preserve planning marker boundaries --- src/workflow/planning-pull-request-markers.ts | 18 +++++- src/workflow/planning-pull-requests.test.ts | 57 +++++++++++-------- src/workflow/planning-pull-requests.ts | 7 +++ 3 files changed, 55 insertions(+), 27 deletions(-) diff --git a/src/workflow/planning-pull-request-markers.ts b/src/workflow/planning-pull-request-markers.ts index 2ea5b054..fcb19b83 100644 --- a/src/workflow/planning-pull-request-markers.ts +++ b/src/workflow/planning-pull-request-markers.ts @@ -35,14 +35,25 @@ const validMarkerPattern = const fencedCodeStartPattern = /^ {0,3}(`{3,}|~{3,})/u; const fencedCodeEndPattern = /^ {0,3}(`+|~+)\s*$/u; const indentedCodePattern = /^(?: {4}|\t)/u; +const containerPrefixPattern = /^(?: {0,3}> ?| {0,3}(?:[-+*]|\d+[.)])[ \t]+)/u; + +function withoutMarkdownContainerPrefix(line: string): string { + let prefix = containerPrefixPattern.exec(line); + while (prefix !== null) { + line = line.slice(prefix[0].length); + prefix = containerPrefixPattern.exec(line); + } + return line; +} function withoutMarkdownCodeBlocks(body: string): string { let fence: string | undefined; return body .split("\n") .map((line) => { + const content = withoutMarkdownContainerPrefix(line); if (fence !== undefined) { - const closingFence = line.match(fencedCodeEndPattern)?.[1]; + const closingFence = content.match(fencedCodeEndPattern)?.[1]; if ( closingFence !== undefined && closingFence[0] === fence[0] && @@ -52,12 +63,12 @@ function withoutMarkdownCodeBlocks(body: string): string { } return ""; } - const openingFence = line.match(fencedCodeStartPattern)?.[1]; + const openingFence = content.match(fencedCodeStartPattern)?.[1]; if (openingFence !== undefined) { fence = openingFence; return ""; } - return indentedCodePattern.test(line) ? "" : line; + return indentedCodePattern.test(content) ? "" : line; }) .join("\n"); } @@ -86,6 +97,7 @@ function codeSpanEnd( delimiterLength: number, ): number | undefined { for (let cursor = start; cursor < value.length; cursor += 1) { + if (/^\r?\n[ \t]*\r?\n/u.test(value.slice(cursor))) return undefined; if (value[cursor] !== "`" || isEscaped(value, cursor)) continue; const end = backtickRunEnd(value, cursor); if (end - cursor === delimiterLength) return end; diff --git a/src/workflow/planning-pull-requests.test.ts b/src/workflow/planning-pull-requests.test.ts index 694fbeca..c4f9aa6d 100644 --- a/src/workflow/planning-pull-requests.test.ts +++ b/src/workflow/planning-pull-requests.test.ts @@ -168,6 +168,8 @@ test("marker parser ignores fenced and indented Markdown code blocks", () => { const codeBodies = [ ["```html", marker, "```"].join("\n"), ["~~~html", marker, "~~~"].join("\n"), + ["> ~~~html", `> ${marker}`, "> ~~~"].join("\n"), + ["- ~~~html", ` ${marker}`, " ~~~"].join("\n"), ` ${marker}`, `\t${marker}`, ]; @@ -184,6 +186,22 @@ test("marker parser ignores fenced and indented Markdown code blocks", () => { ); }); +test("marker parser does not span blank-line-separated Markdown blocks", () => { + const marker = renderPlanningPullRequestMarker({ + issueNumber: 184, + phase: "spec", + }); + + assert.deepEqual( + parsePlanningPullRequestMarker(["`", "", marker, "", "`"].join("\n")), + { + workflowVersion: "planning-pr-v1", + issueNumber: 184, + phase: "spec", + }, + ); +}); + test("marker parser rejects duplicate and invalid planning markers", () => { const invalidBodies = [ [ @@ -252,28 +270,19 @@ test("planning body is non-closing and lists each artifact path once", () => { assert.doesNotMatch(body, closingKeyword); }); -test("planning body safely contains leading, trailing, and multi-backtick artifact paths", () => { - const artifactPaths = [ - "`docs/specs/leading.md\nCloses #184\ntrailing.md`", - "``docs/specs/multi.md\nCloses #184\ntrailing.md``", - ]; - const body = planningPullRequestBody({ - issueNumber: 184, - phase: "spec", - artifactPaths, - }); - - assert.match( - body, - /^- `` `docs\/specs\/leading\.md\nCloses #184\ntrailing\.md` ``$/mu, - ); - assert.match( - body, - /^- ``` ``docs\/specs\/multi\.md\nCloses #184\ntrailing\.md`` ```$/mu, - ); - assert.deepEqual(parsePlanningPullRequestMarker(body), { - workflowVersion: "planning-pr-v1", - issueNumber: 184, - phase: "spec", - }); +test("planning body rejects artifact paths that can create Markdown blocks", () => { + for (const artifactPath of [ + "docs/specs/issue-184.md\n\nCloses #184", + "docs/specs/issue-184.md\n~~~", + ]) { + assert.throws( + () => + planningPullRequestBody({ + issueNumber: 184, + phase: "spec", + artifactPaths: [artifactPath], + }), + RangeError, + ); + } }); diff --git a/src/workflow/planning-pull-requests.ts b/src/workflow/planning-pull-requests.ts index 7724fe44..1e4ad91a 100644 --- a/src/workflow/planning-pull-requests.ts +++ b/src/workflow/planning-pull-requests.ts @@ -132,6 +132,12 @@ function markdownCodeSpan(value: string): string { return `${delimiter} ${value} ${delimiter}`; } +function assertSingleLineArtifactPath(path: string): void { + if (/\r|\n/u.test(path)) { + throw new RangeError("Planning artifact paths must be single-line"); + } +} + export function planningPullRequestBody(input: { issueNumber: number; phase: PlanningArtifactKind; @@ -142,6 +148,7 @@ export function planningPullRequestBody(input: { if (artifactPaths.length === 0) { throw new RangeError("Planning pull request requires an artifact path"); } + for (const path of artifactPaths) assertSingleLineArtifactPath(path); const label = input.phase === "spec" ? "Spec" : "Plan"; return [ `Refs #${input.issueNumber}`, From a8093c642976906c53d9fa20207a98653c661a2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Sat, 5 Sep 2026 11:19:53 +0200 Subject: [PATCH 09/11] fix(workflow): preserve planning marker code contexts --- src/workflow/planning-pull-request-markers.ts | 4 +- src/workflow/planning-pull-requests.test.ts | 2 + test-support/contract-test-node.d.ts | 42 +++++++++++++++++++ tsconfig.contract-tests.json | 10 ++--- 4 files changed, 49 insertions(+), 9 deletions(-) create mode 100644 test-support/contract-test-node.d.ts diff --git a/src/workflow/planning-pull-request-markers.ts b/src/workflow/planning-pull-request-markers.ts index fcb19b83..44bb7def 100644 --- a/src/workflow/planning-pull-request-markers.ts +++ b/src/workflow/planning-pull-request-markers.ts @@ -35,7 +35,7 @@ const validMarkerPattern = const fencedCodeStartPattern = /^ {0,3}(`{3,}|~{3,})/u; const fencedCodeEndPattern = /^ {0,3}(`+|~+)\s*$/u; const indentedCodePattern = /^(?: {4}|\t)/u; -const containerPrefixPattern = /^(?: {0,3}> ?| {0,3}(?:[-+*]|\d+[.)])[ \t]+)/u; +const containerPrefixPattern = /^(?: {0,3}> ?| {0,3}(?:[-+*]|\d+[.)])[ \t])/u; function withoutMarkdownContainerPrefix(line: string): string { let prefix = containerPrefixPattern.exec(line); @@ -98,7 +98,7 @@ function codeSpanEnd( ): number | undefined { for (let cursor = start; cursor < value.length; cursor += 1) { if (/^\r?\n[ \t]*\r?\n/u.test(value.slice(cursor))) return undefined; - if (value[cursor] !== "`" || isEscaped(value, cursor)) continue; + if (value[cursor] !== "`") continue; const end = backtickRunEnd(value, cursor); if (end - cursor === delimiterLength) return end; cursor = end - 1; diff --git a/src/workflow/planning-pull-requests.test.ts b/src/workflow/planning-pull-requests.test.ts index c4f9aa6d..bed1f682 100644 --- a/src/workflow/planning-pull-requests.test.ts +++ b/src/workflow/planning-pull-requests.test.ts @@ -157,6 +157,7 @@ test("marker parser skips only valid unescaped code spans", () => { parsePlanningPullRequestMarker("`" + marker + "```"), identity, ); + assert.equal(parsePlanningPullRequestMarker(`\`${marker}\\\``), undefined); }); test("marker parser ignores fenced and indented Markdown code blocks", () => { @@ -170,6 +171,7 @@ test("marker parser ignores fenced and indented Markdown code blocks", () => { ["~~~html", marker, "~~~"].join("\n"), ["> ~~~html", `> ${marker}`, "> ~~~"].join("\n"), ["- ~~~html", ` ${marker}`, " ~~~"].join("\n"), + `- ${marker}`, ` ${marker}`, `\t${marker}`, ]; diff --git a/test-support/contract-test-node.d.ts b/test-support/contract-test-node.d.ts new file mode 100644 index 00000000..5c222fef --- /dev/null +++ b/test-support/contract-test-node.d.ts @@ -0,0 +1,42 @@ +type ContractTestAssertion = (...arguments_: readonly any[]) => void; + +declare module "node:assert/strict" { + const assert: { + deepEqual: ContractTestAssertion; + doesNotMatch: ContractTestAssertion; + equal: ContractTestAssertion; + match: ContractTestAssertion; + throws: ContractTestAssertion; + }; + export default assert; +} + +declare module "node:test" { + type TestFunction = () => void | Promise; + + function test(name: string, fn: TestFunction): void; + + export default test; +} + +declare module "node:crypto" { + export type BinaryLike = any; + export const createHash: (...arguments_: readonly any[]) => any; +} + +declare module "node:fs" { + export const existsSync: (...arguments_: readonly any[]) => any; + export const statSync: (...arguments_: readonly any[]) => any; +} + +declare module "node:path" { + export const dirname: (...arguments_: readonly any[]) => any; + export const isAbsolute: (...arguments_: readonly any[]) => any; + export const join: (...arguments_: readonly any[]) => any; + export const resolve: (...arguments_: readonly any[]) => any; + export const win32: any; +} + +declare module "node:url" { + export const fileURLToPath: (...arguments_: readonly any[]) => any; +} diff --git a/tsconfig.contract-tests.json b/tsconfig.contract-tests.json index 1ca001c8..0d47c461 100644 --- a/tsconfig.contract-tests.json +++ b/tsconfig.contract-tests.json @@ -1,14 +1,10 @@ { "extends": "./tsconfig.quality.json", - "compilerOptions": { - "typeRoots": [ - "./node_modules/@earendil-works/pi-coding-agent/node_modules/@types" - ], - "types": ["node"] - }, "include": [ "src/git/planning-workspaces.test.ts", - "src/host/pull-requests.test.ts" + "src/host/pull-requests.test.ts", + "src/workflow/planning-pull-requests.test.ts", + "test-support/contract-test-node.d.ts" ], "exclude": [] } From a1451463e415f1f5c13485f86da81e8baee513a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Sat, 5 Sep 2026 11:44:12 +0200 Subject: [PATCH 10/11] fix(workflow): preserve planning marker boundaries --- src/workflow/planning-pull-request-markers.ts | 90 +++++++++++++++---- src/workflow/planning-pull-requests.test.ts | 43 +++++++-- 2 files changed, 109 insertions(+), 24 deletions(-) diff --git a/src/workflow/planning-pull-request-markers.ts b/src/workflow/planning-pull-request-markers.ts index 44bb7def..994efde1 100644 --- a/src/workflow/planning-pull-request-markers.ts +++ b/src/workflow/planning-pull-request-markers.ts @@ -32,43 +32,95 @@ const markerCandidatePattern = //gu; const markerStartPattern = /$/u; -const fencedCodeStartPattern = /^ {0,3}(`{3,}|~{3,})/u; -const fencedCodeEndPattern = /^ {0,3}(`+|~+)\s*$/u; +const fencedCodePattern = /^ {0,3}(`{3,}|~{3,})(.*)$/u; +const fencedCodeEndPattern = /^ {0,3}(`+|~+)[ \t]*$/u; const indentedCodePattern = /^(?: {4}|\t)/u; -const containerPrefixPattern = /^(?: {0,3}> ?| {0,3}(?:[-+*]|\d+[.)])[ \t])/u; +const blockQuotePrefixPattern = /^ {0,3}> ?/u; +const listPrefixPattern = /^ {0,3}(?:[-+*]|\d+[.)])[ \t]/u; -function withoutMarkdownContainerPrefix(line: string): string { - let prefix = containerPrefixPattern.exec(line); - while (prefix !== null) { - line = line.slice(prefix[0].length); - prefix = containerPrefixPattern.exec(line); +type MarkdownContainer = { + blockQuoteDepth: number; + listIndent?: number; +}; + +type MarkdownFence = MarkdownContainer & { + delimiter: string; +}; + +function stripMarkdownContainer( + line: string, + container?: MarkdownContainer, +): { content: string; container: MarkdownContainer } | undefined { + let content = line; + let blockQuoteDepth = 0; + let blockQuotePrefix = blockQuotePrefixPattern.exec(content); + while (blockQuotePrefix !== null) { + content = content.slice(blockQuotePrefix[0].length); + blockQuoteDepth += 1; + blockQuotePrefix = blockQuotePrefixPattern.exec(content); + } + if ( + container !== undefined && + blockQuoteDepth !== container.blockQuoteDepth + ) { + return undefined; + } + const listPrefix = listPrefixPattern.exec(content); + const listIndent = listPrefix?.[0].length; + if (container?.listIndent !== undefined) { + if (listIndent !== undefined) { + content = content.slice(listIndent); + } else if (content.startsWith(" ".repeat(container.listIndent))) { + content = content.slice(container.listIndent); + } else { + return undefined; + } + } else if (listIndent !== undefined) { + content = content.slice(listIndent); } - return line; + return { + content, + container: { + blockQuoteDepth, + ...(listIndent === undefined ? {} : { listIndent }), + }, + }; +} + +function parseOpeningFence(content: string): string | undefined { + const match = content.match(fencedCodePattern); + if (match === null) return undefined; + const delimiter = match[1]!; + return delimiter[0] === "`" && match[2]!.includes("`") + ? undefined + : delimiter; } function withoutMarkdownCodeBlocks(body: string): string { - let fence: string | undefined; + let fence: MarkdownFence | undefined; return body .split("\n") .map((line) => { - const content = withoutMarkdownContainerPrefix(line); + const stripped = stripMarkdownContainer(line, fence); if (fence !== undefined) { - const closingFence = content.match(fencedCodeEndPattern)?.[1]; + if (stripped === undefined) return ""; + const closingFence = stripped.content.match(fencedCodeEndPattern)?.[1]; if ( closingFence !== undefined && - closingFence[0] === fence[0] && - closingFence.length >= fence.length + closingFence[0] === fence.delimiter[0] && + closingFence.length >= fence.delimiter.length ) { fence = undefined; } return ""; } - const openingFence = content.match(fencedCodeStartPattern)?.[1]; + if (stripped === undefined) return line; + const openingFence = parseOpeningFence(stripped.content); if (openingFence !== undefined) { - fence = openingFence; + fence = { delimiter: openingFence, ...stripped.container }; return ""; } - return indentedCodePattern.test(content) ? "" : line; + return indentedCodePattern.test(stripped.content) ? "" : line; }) .join("\n"); } @@ -97,7 +149,9 @@ function codeSpanEnd( delimiterLength: number, ): number | undefined { for (let cursor = start; cursor < value.length; cursor += 1) { - if (/^\r?\n[ \t]*\r?\n/u.test(value.slice(cursor))) return undefined; + if (/^\r?\n[ \t]*(?:\r?\n|`; } -const markerCandidatePattern = //gu; -const markerStartPattern = /$/u; -const fencedCodePattern = /^ {0,3}(`{3,}|~{3,})(.*)$/u; -const fencedCodeEndPattern = /^ {0,3}(`+|~+)[ \t]*$/u; -const indentedCodePattern = /^(?: {4}|\t)/u; -const blockQuotePrefixPattern = /^ {0,3}> ?/u; -const listPrefixPattern = /^ {0,3}(?:[-+*]|\d+[.)])[ \t]/u; +const openingFencePattern = /^(`{3,}|~{3,})(.*)$/u; +const closingFencePattern = /^(`+|~+)[ \t]*$/u; -type MarkdownContainer = { - blockQuoteDepth: number; - listIndent?: number; -}; +type MarkerLine = { line: string; index: number }; -type MarkdownFence = MarkdownContainer & { - delimiter: string; -}; - -function stripMarkdownContainer( - line: string, - container?: MarkdownContainer, -): { content: string; container: MarkdownContainer } | undefined { - let content = line; - let blockQuoteDepth = 0; - let blockQuotePrefix = blockQuotePrefixPattern.exec(content); - while (blockQuotePrefix !== null) { - content = content.slice(blockQuotePrefix[0].length); - blockQuoteDepth += 1; - blockQuotePrefix = blockQuotePrefixPattern.exec(content); - } - if ( - container !== undefined && - blockQuoteDepth !== container.blockQuoteDepth - ) { - return undefined; - } - const listPrefix = listPrefixPattern.exec(content); - const listIndent = listPrefix?.[0].length; - if (container?.listIndent !== undefined) { - if (listIndent !== undefined) { - content = content.slice(listIndent); - } else if (content.startsWith(" ".repeat(container.listIndent))) { - content = content.slice(container.listIndent); - } else { - return undefined; - } - } else if (listIndent !== undefined) { - content = content.slice(listIndent); - } - return { - content, - container: { - blockQuoteDepth, - ...(listIndent === undefined ? {} : { listIndent }), - }, - }; -} - -function parseOpeningFence(content: string): string | undefined { - const match = content.match(fencedCodePattern); +function openingFence(line: string): string | undefined { + const match = line.match(openingFencePattern); if (match === null) return undefined; const delimiter = match[1]!; return delimiter[0] === "`" && match[2]!.includes("`") @@ -96,88 +45,37 @@ function parseOpeningFence(content: string): string | undefined { : delimiter; } -function withoutMarkdownCodeBlocks(body: string): string { - let fence: MarkdownFence | undefined; - return body - .split("\n") - .map((line) => { - const stripped = stripMarkdownContainer(line, fence); - if (fence !== undefined) { - if (stripped === undefined) return ""; - const closingFence = stripped.content.match(fencedCodeEndPattern)?.[1]; - if ( - closingFence !== undefined && - closingFence[0] === fence.delimiter[0] && - closingFence.length >= fence.delimiter.length - ) { - fence = undefined; - } - return ""; +function topLevelMarkerLines(lines: readonly string[]): MarkerLine[] { + const markers: MarkerLine[] = []; + let fence: string | undefined; + for (let index = 0; index < lines.length; index += 1) { + const line = lines[index]!; + if (fence !== undefined) { + const closingFence = line.match(closingFencePattern)?.[1]; + if ( + closingFence !== undefined && + closingFence[0] === fence[0] && + closingFence.length >= fence.length + ) { + fence = undefined; } - if (stripped === undefined) return line; - const openingFence = parseOpeningFence(stripped.content); - if (openingFence !== undefined) { - fence = { delimiter: openingFence, ...stripped.container }; - return ""; - } - return indentedCodePattern.test(stripped.content) ? "" : line; - }) - .join("\n"); -} - -function isEscaped(value: string, index: number): boolean { - let backslashCount = 0; - for ( - let cursor = index - 1; - cursor >= 0 && value[cursor] === "\\"; - cursor -= 1 - ) { - backslashCount += 1; - } - return backslashCount % 2 === 1; -} - -function backtickRunEnd(value: string, start: number): number { - let end = start; - while (value[end] === "`") end += 1; - return end; -} - -function codeSpanEnd( - value: string, - start: number, - delimiterLength: number, -): number | undefined { - for (let cursor = start; cursor < value.length; cursor += 1) { - if (/^\r?\n[ \t]*(?:\r?\n|