From 1383840a5a8149d0e16c8187846fd2a6b6c1ace0 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 10:21:50 -0700 Subject: [PATCH 1/6] chore(quest): claim quest/m0/helpers/move.md From 0e00ba7183baef216abe928b9f832b9d8912b27c Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 10:36:34 -0700 Subject: [PATCH 2/6] feat: move and delete quests with quest mv and quest rm `quest mv ` moves a quest or epic with git mv and rewrites every root-absolute link to it. `quest rm [-r] ` deletes one with git rm and drops each Required, Related, or Closes entry that linked to it, with any heading left empty. Both stage the change, report other mentions without editing them, and run quest check on the result. mv refuses while a moved quest has a claim branch on origin or an open PR on GitHub. The link edits are pure functions in the core (src/core/edit.ts); the CLI owns git. The GitHub client moves out of wait.ts into src/cli/github.ts so the claim lookup can share it. Co-Authored-By: Claude Opus 5.5 --- README.md | 26 +++ assets/skills/audit.md | 2 +- assets/skills/delete.md | 2 +- quest/m0/helpers/README.md | 1 - quest/m0/helpers/move.md | 45 ------ quest/m0/landed-quests.md | 6 +- quest/m0/milestone-links.md | 15 +- src/cli/claims.ts | 85 ++++++++++ src/cli/github.ts | 175 ++++++++++++++++++++ src/cli/main.ts | 57 ++++++- src/cli/move.ts | 176 ++++++++++++++++++++ src/cli/tree.ts | 13 +- src/cli/wait.ts | 151 ++++------------- src/core/doc.ts | 50 ++++-- src/core/edit.ts | 141 ++++++++++++++++ src/core/index.ts | 1 + src/core/rules.ts | 2 +- test/move.test.ts | 313 ++++++++++++++++++++++++++++++++++++ 18 files changed, 1061 insertions(+), 200 deletions(-) delete mode 100644 quest/m0/helpers/move.md create mode 100644 src/cli/claims.ts create mode 100644 src/cli/github.ts create mode 100644 src/cli/move.ts create mode 100644 src/core/edit.ts create mode 100644 test/move.test.ts diff --git a/README.md b/README.md index 70ce515a..8bf2c192 100644 --- a/README.md +++ b/README.md @@ -143,6 +143,32 @@ Invalid trees fail with diagnostics and no ranking. This command uses the same local readiness as `ready`; verify claims and outside conditions before starting. `quest-spawn` uses the ranking unless you explicitly select work in another order. +## Move and delete quests + +`quest mv` and `quest rm` move or delete a quest or an epic with `git mv` and +`git rm`, then fix the links so the tree still passes `quest check`: + +```sh +quest mv quest/m0/export.md quest/m1/export.md +quest mv quest/m0/drain quest/m1/drain +quest rm quest/m0/download.md +quest rm -r quest/m1/drain +``` + +`mv` rewrites every root-absolute link to the moved path, and to everything +under a moved epic. `rm` drops each `Required`, `Related`, or `Closes` entry +that links to what it deleted, and the heading if that was the last entry. +Both stage the change, print one line per file (`R from -> to`, `D path`, or +`M path`), and report on stderr any other mention of the path, such as prose +or files outside `quest/`, without editing it. They run `quest check` on the +result and exit 1 if it fails, for example when a quest moved out of its epic. + +An epic is its directory or README; removing one that holds other quests needs +`-r`. The root and milestones are never removed. `mv` refuses a destination +that exists, and refuses to move a quest while it has a claim branch on +`origin` or an open PR on GitHub, since the branch would no longer name it. +Neither command touches branches or PRs. + ## Wait for pull request activity `quest wait` blocks until a GitHub PR has a review, comment, review-thread diff --git a/assets/skills/audit.md b/assets/skills/audit.md index 33cca769..6608bda2 100644 --- a/assets/skills/audit.md +++ b/assets/skills/audit.md @@ -17,4 +17,4 @@ Split the reading across parallel sub-agents, each given a slice of the scope an Apply factual corrections and changes the user has already decided. Use `/quest-plan` only for unresolved scope, priority, API, or other consequential choices; recommendations are not decisions. Once those choices are settled, apply them without another approval round. -Update every reference and dependency affected by a move or deletion, run `quest check`, and follow the repository's PR workflow within the user's authorized scope. Report the changes with their evidence and any decisions still open. An explicit read-only audit request leaves the tree unchanged. +Move and delete quests with `quest mv` and `quest rm`, which update the links to them; edit any mentions they report, run `quest check`, and follow the repository's PR workflow within the user's authorized scope. Report the changes with their evidence and any decisions still open. An explicit read-only audit request leaves the tree unchanged. diff --git a/assets/skills/delete.md b/assets/skills/delete.md index 38ee3f3f..eee1a025 100644 --- a/assets/skills/delete.md +++ b/assets/skills/delete.md @@ -4,6 +4,6 @@ description: Abandon a quest. Before you begin, run `quest guide` and read its output completely. -Delete the specified quest file and make a PR. +Delete the specified quest with `quest rm`, edit any mentions it reports, and make a PR. If the quest has an open PR, repurpose it instead of making a new PR. Merge the PR; it should only consist of quest deletions. diff --git a/quest/m0/helpers/README.md b/quest/m0/helpers/README.md index c80bce7c..8a2e2a62 100644 --- a/quest/m0/helpers/README.md +++ b/quest/m0/helpers/README.md @@ -50,7 +50,6 @@ commands from Sep-Oct. Session counts in each child come from that window. ## Required - [PR status](/quest/m0/helpers/status.md) - one call for head, checks, merge state, reviews by head, and unresolved threads -- [Move and delete quests](/quest/m0/helpers/move.md) - `quest mv` and `quest rm` keep every root-absolute link valid - [CI checks in wait](/quest/m0/helpers/wait-checks.md) - `quest wait` wakes on required check results for the head - [quest start](/quest/m0/helpers/start.md) - claim a quest with a draft PR, the `## Decisions needed` hand-back, `quest unblocked --unclaimed`, and CI on ready PRs only - [PR state commands](/quest/m0/helpers/pr-states.md) - `quest iterate`, `ready`, `merge`, and `release` label the one owner of a PR diff --git a/quest/m0/helpers/move.md b/quest/m0/helpers/move.md deleted file mode 100644 index 429fe893..00000000 --- a/quest/m0/helpers/move.md +++ /dev/null @@ -1,45 +0,0 @@ -# [M] Move and delete quests with quest mv and quest rm - -## Goal - -`quest mv ` and `quest rm ` move or delete a quest or epic -and keep every root-absolute link in the tree valid. Agents stop following -`git rm` and `git mv` with `grep -r` and `sed -i` over Required links (203 -sessions since Oct 14). `quest check` already finds the broken links; these -commands avoid making them. - -## Plan - -- Decided 2026-10-19 (user): run `git mv` and `git rm`, so the change is - staged. Fail outside a git work tree rather than falling back to plain - file operations. -- `mv` rewrites every link to the moved path under `quest/`; moving an epic - directory rewrites links under it as a prefix. Moving a quest into or out - of an epic changes its structural membership; `quest check` reports what - the epic's Required still needs rather than `mv` guessing. -- `rm` drops list items that link to the deleted path (or under a deleted - epic) and deletes a section left empty, as the guide requires for an empty - Required. Removing an epic with children requires `-r`, as with `git rm`. -- Decided 2026-10-19 (user): `rm` also drops every `Required` link to a - finished milestone, meaning a milestone README with no `## Required` left - ([finished milestone links](/quest/m0/milestone-links.md)). That covers a - milestone this `rm` just emptied and any links that were already stale. As - in that check, the root's own entry for the milestone stays: it is - membership, not a blocker, and dropping it would leave the milestone - unlisted. It repeats until nothing changes, so `rm` never leaves a tree - that fails either check. Whichever of the two quests lands second wires it - up. -- Both report prose mentions of the path and mentions outside `quest/` - without editing them, and run `quest check` on the result. -- Neither touches branches or PRs, since renaming an open PR's head branch - closes the PR. `mv` refuses while the quest, or any quest under a moved - epic, has a claim branch or open PR, because the old branch name would no - longer be recognized as its claim. Use the same lookup as `quest claim`, - so whichever lands second reuses it. -- Put the link rewriting in the core as a pure edit of documents; the CLI - owns git. Update the `delete` and `audit` skills to use `quest rm` and - `quest mv`. -- Test moving a quest, moving an epic, deleting a quest listed under several - Required and Related sections, deleting a milestone's last child while - another quest requires that milestone (the root still lists it), refusing a non-empty epic without `-r`, and - a destination that already exists, and refusing to move a claimed quest. diff --git a/quest/m0/landed-quests.md b/quest/m0/landed-quests.md index d891be26..fe244e56 100644 --- a/quest/m0/landed-quests.md +++ b/quest/m0/landed-quests.md @@ -63,9 +63,8 @@ Decided 2026-10-20 (user, planned from moq-dev/moq): of their own. Rejected: refusing and reporting for `/quest-iterate`. - Repositories may also run the guard in CI on pull requests; document the invocation in `README.md`, but don't ship a workflow. -- [Move and delete quests](/quest/m0/helpers/move.md) gives `quest rm`, which - the fix-up step uses once it lands; until then the agent deletes and - unlinks by hand. +- The fix-up step deletes the quest with `quest rm`, which drops the links to + it. Tests: a quest-branch diff that deletes the quest, one that edits it, and one that leaves it untouched (the finding); an epic branch checked against its @@ -74,5 +73,4 @@ the base; and `--branch` overriding the current branch. ## Related -- [Move and delete quests](/quest/m0/helpers/move.md) - `quest rm` deletes a quest and every link to it - [Finished milestone links](/quest/m0/milestone-links.md) - the same "a finishing PR removes what depended on it" rule for milestones diff --git a/quest/m0/milestone-links.md b/quest/m0/milestone-links.md index bc8fbdbb..89dfce5d 100644 --- a/quest/m0/milestone-links.md +++ b/quest/m0/milestone-links.md @@ -47,13 +47,14 @@ Decided 2026-10-19 (user): `Required`, so the bump trips nothing. Four moq quests require `/quest/m1/README.md` (the release cut, the API-diff check, the API shape review, and the feature review); the PR that finishes m1 removes those links. -- `quest rm` ([move and delete quests](/quest/m0/helpers/move.md)) drops these - links as well; whichever of the two quests lands second wires that up. +- `quest rm` drops these links as well: every `Required` link to a finished + milestone, whether this `rm` emptied it or the link was already stale. The + root's own listing stays, as in the check. It repeats until nothing changes, + so `rm` never leaves a tree that fails the check. `quest rm` landed first, + so this quest wires that into `remove` in `src/core/edit.ts`. Tests: a quest requiring a finished milestone is flagged; one requiring a milestone with only non-child blockers left is not; the root's milestone -listing is not; and a fixed tree passes. - -## Related - -- [Move and delete quests](/quest/m0/helpers/move.md) - `quest rm` drops links to milestones it leaves finished +listing is not; and a fixed tree passes. `quest rm` of a milestone's last +child drops another quest's link to that milestone, while the root still +lists it. diff --git a/src/cli/claims.ts b/src/cli/claims.ts new file mode 100644 index 00000000..d097076e --- /dev/null +++ b/src/cli/claims.ts @@ -0,0 +1,85 @@ +// Who holds a quest: its claim branch on the remote, or an open PR from a +// branch of that name in any repository. Commands that must not disturb held +// work look here first. +import { execFileSync } from "node:child_process"; + +import { + type GitHubDependencies, + type Repository, + GitHub, + githubRepository, + object, + remoteUrl, + string, +} from "./github"; + +/** A quest's branch: its path without `.md`. */ +export function questBranch(path: string): string { + return path.replace(/\.md$/, ""); +} + +/** A held quest branch, and the open PR from it if there is one. */ +export interface Claim { + branch: string; + url: string | null; +} + +export interface ClaimDependencies { + /** Called only when the remote is on GitHub, so other forges need no token. */ + github(): GitHubDependencies; + warn(message: string): void; +} + +/** The `quest/*` branches on `remote`. Plain git, so it works with any forge. */ +export function remoteBranches(root: string, remote: string): string[] { + const out = execFileSync("git", ["-C", root, "ls-remote", remote, "refs/heads/quest/*"], { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); + return out + .split("\n") + .filter((line) => line !== "") + .map((line) => line.slice(line.indexOf("\t") + 1).replace(/^refs\/heads\//, "")); +} + +/** Every open PR in `repository`, from any head repository, by head branch. */ +export async function openPullRequests( + github: GitHub, + repository: Repository, +): Promise<{ branch: string; url: string }[]> { + const base = `/repos/${encodeURIComponent(repository.owner)}/${encodeURIComponent(repository.repo)}`; + const rows = await github.all(`${base}/pulls?state=open&per_page=100`); + return rows.map((row) => ({ branch: string(object(row.head).ref), url: string(row.html_url) })); +} + +/** + * Which of `branches` someone holds: pushed to `remote`, or the head of an + * open PR there. Without the remote it warns and checks nothing; off GitHub + * it checks branches only. + */ +export async function claims( + root: string, + remote: string, + branches: string[], + deps: ClaimDependencies, +): Promise { + let url: string; + try { + url = remoteUrl(root, remote); + } catch { + deps.warn(`no ${remote} remote, so claims were not checked`); + return []; + } + const wanted = new Set(branches); + const held = new Map(); + for (const branch of remoteBranches(root, remote)) if (wanted.has(branch)) held.set(branch, null); + const repository = githubRepository(url); + if (repository === null) { + deps.warn(`${remote} is not on GitHub, so only its branches were checked for claims`); + } else { + for (const pr of await openPullRequests(new GitHub(deps.github()), repository)) { + if (wanted.has(pr.branch)) held.set(pr.branch, pr.url); + } + } + return [...held].map(([branch, url]) => ({ branch, url })).sort((a, b) => (a.branch < b.branch ? -1 : 1)); +} diff --git a/src/cli/github.ts b/src/cli/github.ts new file mode 100644 index 00000000..600d4ec1 --- /dev/null +++ b/src/cli/github.ts @@ -0,0 +1,175 @@ +// GitHub requests for the commands that talk to GitHub, outside the +// forge-agnostic core: authentication, rate-limit retry, and same-origin +// pagination. +import { execFileSync } from "node:child_process"; +import { setTimeout as sleep } from "node:timers/promises"; + +const API = "https://api.github.com"; + +export type ObjectValue = Record; + +/** Network, time, and credentials, injected so tests replay GitHub. */ +export interface GitHubDependencies { + fetch(url: URL, init: RequestInit): Promise; + now(): number; + sleep(milliseconds: number): Promise; + token: string; + warn(message: string): void; +} + +/** A repository on github.com. */ +export interface Repository { + owner: string; + repo: string; +} + +/** The request outlived the caller's deadline. */ +export class GitHubDeadline extends Error { + constructor() { + super("deadline passed during a GitHub request"); + } +} + +/** Authenticate with GH_TOKEN, then GITHUB_TOKEN, then `gh auth token`. */ +export function githubDependencies(): GitHubDependencies { + let token = process.env.GH_TOKEN || process.env.GITHUB_TOKEN; + if (!token) { + try { + token = execFileSync("gh", ["auth", "token", "--hostname", "github.com"], { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }).trim(); + } catch { + throw new Error("set GH_TOKEN or GITHUB_TOKEN, or sign in with gh auth login"); + } + } + if (!token) throw new Error("GitHub authentication returned an empty token"); + return { fetch, now: Date.now, sleep, token, warn: (message) => process.stderr.write(`quest: ${message}\n`) }; +} + +/** The URL a git remote fetches from. */ +export function remoteUrl(root: string, remote: string): string { + return execFileSync("git", ["-C", root, "remote", "get-url", remote], { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }).trim(); +} + +/** The github.com repository a remote URL names, or `null` for any other host. */ +export function githubRepository(url: string): Repository | null { + const match = + /^(?:https:\/\/github\.com\/|git@github\.com:|ssh:\/\/git@github\.com\/)([^/]+)\/([^/]+?)(?:\.git)?$/.exec(url); + return match ? { owner: match[1], repo: match[2] } : null; +} + +/** REST and GraphQL requests that retry rate limits and give up at `deadline` (epoch milliseconds). */ +export class GitHub { + constructor( + private readonly deps: GitHubDependencies, + private readonly deadline = Infinity, + ) {} + + /** One request, returning a 200 or 304; `body` makes it a POST. */ + async request(path: string, options: { etag?: string; body?: unknown } = {}): Promise { + const { deps, deadline } = this; + // Follow only same-origin pagination links; never send a token to another host. + const url = new URL(path, API); + if (url.origin !== API || url.username || url.password) throw new Error("invalid GitHub pagination URL"); + let attempts = 0; + for (;;) { + const remaining = deadline - deps.now(); + if (remaining <= 0) throw new GitHubDeadline(); + let response: Response; + try { + response = await deps.fetch(url, { + ...(options.body === undefined ? {} : { method: "POST", body: JSON.stringify(options.body) }), + headers: { + Accept: "application/vnd.github+json", + Authorization: `Bearer ${deps.token}`, + "X-GitHub-Api-Version": "2022-11-28", + "User-Agent": "quest", + ...(options.etag ? { "If-None-Match": options.etag } : {}), + }, + ...(Number.isFinite(remaining) ? { signal: AbortSignal.timeout(Math.ceil(remaining)) } : {}), + }); + } catch (error) { + if (deps.now() >= deadline) throw new GitHubDeadline(); + throw error; + } + if (response.status === 200 || response.status === 304) return response; + const body = await response.text(); + const limited = + response.status === 429 || + (response.status === 403 && + (response.headers.has("retry-after") || + response.headers.get("x-ratelimit-remaining") === "0" || + /rate limit/i.test(body))); + if (!limited || attempts >= 6) + throw new Error( + `GitHub HTTP ${response.status} for ${url.pathname}; ${limited ? "rate limit persisted" : "check repository access and token permissions"}`, + ); + const retry = seconds(response.headers.get("retry-after")); + const reset = + response.headers.get("x-ratelimit-remaining") === "0" + ? seconds(response.headers.get("x-ratelimit-reset")) + : undefined; + const delay = retry ?? (reset === undefined ? 60_000 * 2 ** attempts : Math.max(1000, reset - deps.now())); + attempts++; + deps.warn(`GitHub rate limited this request; waiting ${Math.ceil(delay / 1000)} seconds`); + const left = deadline - deps.now(); + if (left <= 0) throw new GitHubDeadline(); + await deps.sleep(Math.min(delay, left, 2_147_483_647)); + if (deps.now() >= deadline) throw new GitHubDeadline(); + } + } + + /** One page of a REST listing, and its response for the next link. */ + async list(path: string): Promise<{ rows: ObjectValue[]; response: Response }> { + const response = await this.request(path); + if (response.status !== 200) throw new Error("unexpected GitHub 304 without a cached response"); + return { rows: array(await response.json()), response }; + } + + /** Every page of a REST listing. */ + async all(path: string): Promise { + const rows: ObjectValue[] = []; + for (let next: string | undefined = path; next;) { + const page = await this.list(next); + rows.push(...page.rows); + next = link(page.response, "next"); + } + return rows; + } +} + +/** The URL of a `Link` header relation, such as the next page. */ +export function link(response: Response, relation: string): string | undefined { + return response.headers + .get("link") + ?.split(",") + .map((entry) => /<([^>]+)>;\s*rel="([^"]+)"/.exec(entry.trim())) + .find((match) => match?.[2] === relation)?.[1]; +} + +/** Milliseconds from a header holding seconds. */ +export function seconds(value: string | null): number | undefined { + if (value === null || !/^\d+(?:\.\d+)?$/.test(value)) return undefined; + const result = Number(value) * 1000; + return Number.isFinite(result) ? result : undefined; +} + +export function object(value: unknown): ObjectValue { + if (typeof value !== "object" || value === null || Array.isArray(value)) + throw new Error("invalid GitHub response: expected an object"); + return value as ObjectValue; +} + +export function array(value: unknown): ObjectValue[] { + if (!Array.isArray(value)) throw new Error("invalid GitHub response: expected an array"); + return value.map(object); +} + +export function string(value: unknown): string { + if (typeof value !== "string" || !value) throw new Error("invalid GitHub response: expected a nonempty string"); + return value; +} diff --git a/src/cli/main.ts b/src/cli/main.ts index e75c0d52..22a9ee45 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -7,11 +7,13 @@ import { Command, CommanderError, InvalidArgumentError, Option } from "commander import { description, version } from "../../package.json"; import { blockers, check, formatFinding, label, lookup, ready, renderBlocker, unblocked } from "../core"; +import { githubDependencies } from "./github"; +import { type Report, moveQuest, removeQuest } from "./move"; import { init, uninstall } from "./setup"; import { GUIDE, SKILLS, skill } from "./skills"; import { type Terminal, Theme } from "./theme"; import { collect, exists, load, under } from "./tree"; -import { pullRequest, waitDependencies, waitForActivity, WaitInputError, WaitTimeout } from "./wait"; +import { pullRequest, waitForActivity, WaitInputError, WaitTimeout } from "./wait"; /** Where the command writes; the process streams, or a buffer under test. */ export interface Output { @@ -154,7 +156,7 @@ export function main(argv: string[], out: Output): number | Promise { .option("--timeout ", "Maximum wait in seconds", positiveSeconds) .action((value: string, options: { since?: string; timeout?: number }) => { const pr = pullRequest(value, root()); - const deps = waitDependencies(); + const deps = githubDependencies(); deps.warn = warn; pending = waitForActivity(pr, options, deps).then( (lines) => { @@ -169,6 +171,57 @@ export function main(argv: string[], out: Output): number | Promise { ); }); + /** Changes on stdout; mentions and check findings on stderr, which fail the command. */ + const report = (result: Report): number => { + for (const line of result.changes) print(line); + for (const line of result.warnings) warn(line); + for (const line of result.findings) warn(line); + return result.findings.length > 0 ? 1 : 0; + }; + + program + .command("mv") + .summary("Move a quest or epic and rewrite the links to it") + .description( + "Move a quest, or an epic directory and everything under it, with git mv, and rewrite every " + + "root-absolute link to it under quest/. The change is staged.\n\n" + + "Refuses outside a git work tree, when the destination exists, and while a moved quest has a claim " + + "branch on origin or an open PR on GitHub, since the old branch would no longer name it. Moving a " + + "quest into or out of an epic changes which epic lists it; the check afterwards reports what to fix.\n\n" + + "Prints R for the move and M for each edited document. Mentions of the old path that are not links, " + + "or are outside quest/, are reported on stderr and left alone. Runs quest check on the result and " + + "exits 1 if it fails.", + ) + .argument("", "Quest file or epic directory, as /quest/..., quest/..., or a filesystem path") + .argument("", "Its new path, which must not exist") + .action((from: string, to: string) => { + pending = moveQuest(root(), from, to, { github: githubDependencies, warn }).then( + report, + (error: unknown) => { + warn(error instanceof Error ? error.message : String(error)); + return 1; + }, + ); + }); + + program + .command("rm") + .summary("Delete a quest or epic and drop the links to it") + .description( + "Delete a quest, or an epic and the quests under it, with git rm, and drop every Required, Related, " + + "or Closes entry that links to it, with any heading left empty. The change is staged.\n\n" + + "Refuses outside a git work tree. An epic is its directory or README; removing one that holds other " + + "quests requires -r. The root epic and milestones are permanent.\n\n" + + "Prints D for each deleted quest and M for each edited document. Mentions that are not list entries, " + + "or are outside quest/, are reported on stderr and left alone. Runs quest check on the result and " + + "exits 1 if it fails.", + ) + .argument("", "Quest file, or epic directory or README") + .option("-r, --recursive", "Remove an epic with the quests under it") + .action((path: string, options: { recursive?: boolean }) => { + code = report(removeQuest(root(), path, options.recursive ?? false)); + }); + program .command("guide") .description("Print the quest guide: the format, workflow, and rules agents follow") diff --git a/src/cli/move.ts b/src/cli/move.ts new file mode 100644 index 00000000..e240af0a --- /dev/null +++ b/src/cli/move.ts @@ -0,0 +1,176 @@ +// `quest mv` and `quest rm`: git moves or deletes the files and stages the +// change, and the core edits the links in the documents that remain. +import { execFileSync } from "node:child_process"; +import { mkdirSync, statSync, writeFileSync } from "node:fs"; +import { join, posix, relative, resolve, sep } from "node:path"; + +import { ROOT, check, formatFinding, isQuest, move, normalize, permanent, relocate, remove } from "../core"; +import { type ClaimDependencies, claims, questBranch } from "./claims"; +import { exists, load, sources, under } from "./tree"; + +/** What a command changed, and what it left for the caller to look at. */ +export interface Report { + /** `R from -> to`, `D path`, or `M path`, one per change. */ + changes: string[]; + /** Mentions left for a person to edit, as `path:line: message`. */ + warnings: string[]; + /** `quest check` on the result. */ + findings: string[]; +} + +/** + * Move a quest, or an epic directory and everything under it, and rewrite + * every root-absolute link to it. Refuses while a moved quest is claimed, + * since its branch would no longer name it. + */ +export async function moveQuest( + root: string, + fromArg: string, + toArg: string, + deps: ClaimDependencies, +): Promise { + workTree(root); + const from = treePath(root, fromArg); + const to = treePath(root, toArg); + if (from === "quest" || from === ROOT) throw new Error("the root epic cannot move"); + if (!exists(root, from)) throw new Error(`${fromArg} does not exist`); + const directory = statSync(join(root, from)).isDirectory(); + if (!directory && !isQuest(from)) throw new Error(`${fromArg} is not a quest document`); + if (!directory && !isQuest(to)) throw new Error(`${toArg} must name a Markdown quest document`); + if (exists(root, to)) throw new Error(`${toArg} already exists`); + if (relocate(to, from, from) !== null) throw new Error(`cannot move ${fromArg} inside itself`); + + const docs = sources(root); + const moving = docs.filter((doc) => relocate(doc.path, from, to) !== null); + if (moving.length === 0) throw new Error(`${fromArg} holds no quest documents`); + const held = await claims( + root, + "origin", + moving.map((doc) => questBranch(doc.path)), + deps, + ); + if (held.length > 0) { + const list = held.map((claim) => (claim.url ? `${claim.branch} (${claim.url})` : claim.branch)).join(", "); + throw new Error( + `refusing to move claimed quests: ${list}; their branches would no longer name them, so move them once the work merges or the claim is released`, + ); + } + + const edited = move(docs, from, to); + mkdirSync(join(root, posix.dirname(to)), { recursive: true }); + git(root, "mv", "--", from, to); + write(root, edited.texts); + const warnings = edited.missed.map( + (link) => + `${relocate(link.path, from, to) ?? link.path}:${link.line}: links ${link.target} in a form mv cannot rewrite; update it by hand`, + ); + return { + changes: [`R ${from} -> ${to}`, ...modified(edited.texts)], + warnings: [...warnings, ...mentions(root, directory ? `${from}/` : from)], + findings: findings(root), + }; +} + +/** + * Delete a quest, or an epic with the quests under it, and drop every list + * entry that links to what was deleted. The root and milestones are permanent. + */ +export function removeQuest(root: string, arg: string, recursive: boolean): Report { + workTree(root); + const target = treePath(root, arg); + if (!exists(root, target)) throw new Error(`${arg} does not exist`); + const directory = statSync(join(root, target)).isDirectory(); + if (target === "quest" || permanent(directory ? `${target}/README.md` : target)) { + throw new Error(`${arg} is permanent; the root epic and milestones are never removed`); + } + if (!directory && !isQuest(target)) throw new Error(`${arg} is not a quest document`); + + // An epic is its directory, or its README while it still holds other quests. + const epic = directory ? target : posix.basename(target) === "README.md" ? posix.dirname(target) : null; + const docs = sources(root); + const inside = epic === null ? [] : docs.filter((doc) => relocate(doc.path, epic, epic) !== null); + const whole = epic !== null && (directory || inside.length > 1); + const doomed = whole ? inside : docs.filter((doc) => doc.path === target); + if (whole && doomed.length > 1 && !recursive) { + throw new Error(`${arg} is an epic holding ${doomed.length} quest documents; pass -r to remove them all`); + } + + const gone = new Set(doomed.map((doc) => doc.path)); + const edited = remove(docs, (path) => gone.has(path)); + git(root, "rm", "-q", ...(whole ? ["-r"] : []), "--", whole ? epic! : target); + write(root, edited); + return { + changes: [...doomed.map((doc) => `D ${doc.path}`), ...modified(edited)], + warnings: mentions(root, whole ? `${epic}/` : target), + findings: findings(root), + }; +} + +/** + * A quest path as the tree writes it (`/quest/m0/a.md`), as the shell + * completes it (`quest/m0/a.md`), or as a filesystem path to it. + */ +function treePath(root: string, arg: string): string { + const tree = normalize(arg.replace(/^\/(?=quest(\/|$))/, "")); + const path = + tree === "quest" || tree.startsWith("quest/") + ? tree + : relative(resolve(root), resolve(arg)).split(sep).join("/"); + if (path !== "quest" && !path.startsWith("quest/")) throw new Error(`${arg} is not under ${under(root, "quest")}`); + return path; +} + +/** Fail outside a git work tree: these commands stage their change. */ +function workTree(root: string) { + try { + git(root, "rev-parse", "--is-inside-work-tree"); + } catch { + throw new Error(`${root} is not in a git work tree; quest mv and rm stage their change with git`); + } +} + +function git(root: string, ...args: string[]): string { + try { + return execFileSync("git", ["-C", root, ...args], { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }); + } catch (error) { + const stderr = (error as { stderr?: string }).stderr?.trim(); + throw new Error(stderr || (error as Error).message); + } +} + +/** Write and stage each edited document. */ +function write(root: string, texts: Map) { + if (texts.size === 0) return; + for (const [path, text] of texts) writeFileSync(join(root, path), text); + git(root, "add", "--", ...texts.keys()); +} + +function modified(texts: Map): string[] { + return [...texts.keys()].sort().map((path) => `M ${path}`); +} + +/** Every tracked line that still names `path`, which no edit could safely change. */ +function mentions(root: string, path: string): string[] { + let out: string; + try { + out = execFileSync("git", ["-C", root, "grep", "-n", "-I", "-F", "-e", path], { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); + } catch (error) { + // git grep exits 1 when nothing matches. + if ((error as { status?: number }).status === 1) return []; + throw error; + } + return out + .split("\n") + .filter((line) => line !== "") + .map((line) => { + const [file, number] = line.split(":", 2); + return `${file}:${number}: mentions ${path}; update it by hand`; + }); +} + +function findings(root: string): string[] { + return check(load(root), (path) => exists(root, path)).map(formatFinding); +} diff --git a/src/cli/tree.ts b/src/cli/tree.ts index d4154853..8b6edef7 100644 --- a/src/cli/tree.ts +++ b/src/cli/tree.ts @@ -3,7 +3,7 @@ import { existsSync, readFileSync, readdirSync } from "node:fs"; import { join } from "node:path"; -import { type Doc, comparePaths, isQuest, parse } from "../core"; +import { type Doc, type Source, comparePaths, isQuest, parse } from "../core"; /** * `/` as a user would name it in an error: `./quest`, not the @@ -36,11 +36,16 @@ export function collect(root: string): string[] { return out.sort(comparePaths); } -/** Every quest document, parsed, in tree order. */ -export function load(root: string): Doc[] { +/** Every quest document's source text, in tree order. */ +export function sources(root: string): Source[] { const paths = collect(root); if (paths.length === 0) throw new Error(`no quest documents found under ${under(root, "quest")}`); - return paths.map((path) => parse(path, read(root, path))); + return paths.map((path) => ({ path, text: read(root, path) })); +} + +/** Every quest document, parsed, in tree order. */ +export function load(root: string): Doc[] { + return sources(root).map((source) => parse(source.path, source.text)); } /** Whether a repository-relative path exists under `root`. */ diff --git a/src/cli/wait.ts b/src/cli/wait.ts index cb2fe082..00b5d37f 100644 --- a/src/cli/wait.ts +++ b/src/cli/wait.ts @@ -1,8 +1,17 @@ // GitHub-specific PR activity waiting stays outside the repository-independent core. -import { execFileSync } from "node:child_process"; -import { setTimeout as sleep } from "node:timers/promises"; +import { + type GitHubDependencies, + type ObjectValue, + GitHub, + array, + githubRepository, + link, + object, + remoteUrl, + seconds, + string, +} from "./github"; -const API = "https://api.github.com"; const POLL = 5 * 60_000; const SHA = /^[a-f\d]{7,40}$/i; /** The timeline records a force push a few seconds after repository activity (1-2s observed). */ @@ -34,13 +43,7 @@ export interface WaitOptions { timeout?: number; } -export interface WaitDependencies { - fetch: typeof fetch; - now(): number; - sleep(milliseconds: number): Promise; - token: string; - warn(message: string): void; -} +export type WaitDependencies = GitHubDependencies; export class WaitInputError extends Error {} @@ -51,7 +54,6 @@ export class WaitTimeout extends Error { } } -type ObjectValue = Record; interface Activity { point: string; time: number; @@ -82,38 +84,15 @@ interface Cursor { /** Accept a GitHub PR URL, or a number in the origin repository. */ export function pullRequest(value: string, root: string): PullRequest { if (/^[1-9]\d*$/.test(value)) { - const origin = execFileSync("git", ["-C", root, "remote", "get-url", "origin"], { - encoding: "utf8", - stdio: ["ignore", "pipe", "pipe"], - }).trim(); - const match = - /^(?:https:\/\/github\.com\/|git@github\.com:|ssh:\/\/git@github\.com\/)([^/]+)\/([^/]+?)(?:\.git)?$/.exec( - origin, - ); - if (!match) throw new Error("origin must name a github.com repository; pass a GitHub PR URL instead"); - return { owner: match[1], repo: match[2], number: Number(value) }; + const repository = githubRepository(remoteUrl(root, "origin")); + if (!repository) throw new Error("origin must name a github.com repository; pass a GitHub PR URL instead"); + return { ...repository, number: Number(value) }; } const match = /^https:\/\/github\.com\/([\w.-]+)\/([\w.-]+)\/pull\/([1-9]\d*)\/?(?:[?#].*)?$/.exec(value); if (!match) throw new WaitInputError("expected a GitHub pull request URL or a PR number in origin"); return { owner: match[1], repo: match[2], number: Number(match[3]) }; } -export function waitDependencies(): WaitDependencies { - let token = process.env.GH_TOKEN || process.env.GITHUB_TOKEN; - if (!token) { - try { - token = execFileSync("gh", ["auth", "token", "--hostname", "github.com"], { - encoding: "utf8", - stdio: ["ignore", "pipe", "pipe"], - }).trim(); - } catch { - throw new Error("set GH_TOKEN or GITHUB_TOKEN, or sign in with gh auth login"); - } - } - if (!token) throw new Error("GitHub authentication returned an empty token"); - return { fetch, now: Date.now, sleep, token, warn: (message) => process.stderr.write(`quest: ${message}\n`) }; -} - /** Wait until activity, returning plain output lines. Network and time are injected. */ export async function waitForActivity( pr: PullRequest, @@ -157,60 +136,20 @@ async function watch( if (deps.now() >= deadline) throw new WaitTimeout(); }; let pollInterval = POLL; - const request = async (path: string, etag?: string, payload?: unknown): Promise => { - // Follow only same-origin pagination links; never send a token to another host. - const url = new URL(path, API); - if (url.origin !== API || url.username || url.password) throw new Error("invalid GitHub pagination URL"); - let attempts = 0; - for (;;) { - const remaining = deadline - deps.now(); - if (remaining <= 0) throw new WaitTimeout(); - let response: Response; - try { - response = await deps.fetch(url, { - ...(payload === undefined ? {} : { method: "POST", body: JSON.stringify(payload) }), - headers: { - Accept: "application/vnd.github+json", - Authorization: `Bearer ${deps.token}`, - "X-GitHub-Api-Version": "2022-11-28", - "User-Agent": "quest", - ...(etag ? { "If-None-Match": etag } : {}), - }, - ...(Number.isFinite(remaining) ? { signal: AbortSignal.timeout(Math.ceil(remaining)) } : {}), - }); - } catch (error) { - if (deps.now() >= deadline) throw new WaitTimeout(); - throw error; - } - pollInterval = Math.max(pollInterval, seconds(response.headers.get("x-poll-interval")) ?? 0); - if (response.status === 200 || response.status === 304) return response; - const body = await response.text(); - const limited = - response.status === 429 || - (response.status === 403 && - (response.headers.has("retry-after") || - response.headers.get("x-ratelimit-remaining") === "0" || - /rate limit/i.test(body))); - if (!limited || attempts >= 6) - throw new Error( - `GitHub HTTP ${response.status} for ${url.pathname}; ${limited ? "rate limit persisted" : "check repository access and token permissions"}`, - ); - const retry = seconds(response.headers.get("retry-after")); - const reset = - response.headers.get("x-ratelimit-remaining") === "0" - ? seconds(response.headers.get("x-ratelimit-reset")) - : undefined; - const delay = retry ?? (reset === undefined ? 60_000 * 2 ** attempts : Math.max(1000, reset - deps.now())); - attempts++; - deps.warn(`GitHub rate limited this request; waiting ${Math.ceil(delay / 1000)} seconds`); - await pause(delay); - } - }; - const list = async (path: string): Promise<{ rows: ObjectValue[]; response: Response }> => { - const response = await request(path); - if (response.status !== 200) throw new Error("unexpected GitHub 304 without a cached response"); - return { rows: array(await response.json()), response }; - }; + // Any failure past the deadline, the client's included, becomes a WaitTimeout in waitForActivity. + const github = new GitHub( + { + ...deps, + fetch: async (url, init) => { + const response = await deps.fetch(url, init); + pollInterval = Math.max(pollInterval, seconds(response.headers.get("x-poll-interval")) ?? 0); + return response; + }, + }, + deadline, + ); + const request = (path: string, etag?: string, body?: unknown) => github.request(path, { etag, body }); + const list = (path: string) => github.list(path); let cursor: Cursor; let historical: Activity[] = []; @@ -512,22 +451,6 @@ function actor(value: unknown): string { : "-"; } -function object(value: unknown): ObjectValue { - if (typeof value !== "object" || value === null || Array.isArray(value)) - throw new Error("invalid GitHub response: expected an object"); - return value as ObjectValue; -} - -function array(value: unknown): ObjectValue[] { - if (!Array.isArray(value)) throw new Error("invalid GitHub response: expected an array"); - return value.map(object); -} - -function string(value: unknown): string { - if (typeof value !== "string" || !value) throw new Error("invalid GitHub response: expected a nonempty string"); - return value; -} - function timestamp(value: string): number { const result = Date.parse(value); if (!/^\d{4}-\d\d-\d\dT\d\d:\d\d:\d\d(?:\.\d+)?(?:Z|[+-]\d\d:\d\d)$/.test(value) || !Number.isFinite(result)) @@ -535,20 +458,6 @@ function timestamp(value: string): number { return result; } -function seconds(value: string | null): number | undefined { - if (value === null || !/^\d+(?:\.\d+)?$/.test(value)) return undefined; - const result = Number(value) * 1000; - return Number.isFinite(result) ? result : undefined; -} - -function link(response: Response, relation: string): string | undefined { - return response.headers - .get("link") - ?.split(",") - .map((entry) => /<([^>]+)>;\s*rel="([^"]+)"/.exec(entry.trim())) - .find((match) => match?.[2] === relation)?.[1]; -} - function fullSha(value: unknown): boolean { return typeof value === "string" && /^[a-f\d]{40}$/i.test(value); } diff --git a/src/core/doc.ts b/src/core/doc.ts index 5cadcff9..8fcf921a 100644 --- a/src/core/doc.ts +++ b/src/core/doc.ts @@ -39,12 +39,20 @@ export interface Link { target: string; /** Where the link sits in its section. */ position: Position; + /** + * Source offsets bounding where the destination is written: after the + * link's text to the end of the link, or after the label of the + * definition a reference-style link uses. An edit searches it for `target`. + */ + span: [number, number]; } /** One top-level list entry: the shape a `Required` child or blocker has. */ export interface Entry { /** 1-based source line, for findings. */ line: number; + /** 1-based last source line of the item, so an edit can remove it. */ + end: number; /** The enclosing `## Heading`, or `null` above the first one. */ section: string | null; /** @@ -211,7 +219,7 @@ export function parse(path: string, text: string): Doc { // Lines as grep sees them: a bare `\r` does not end one, so a CR-only file is // one line and none of its headings are literal, which is what grep finds. const lines = text.split("\n").map((line) => (line.endsWith("\r") ? line.slice(0, -1) : line)); - const definitions = collectDefinitions(tree); + const definitions = collectDefinitions(tree, text); let title: Heading | null = null; const headings: Heading[] = []; @@ -234,6 +242,8 @@ export function parse(path: string, text: string): Doc { let heading: { depth: number; line: number; text: string } | null = null; const lineOf = (node: Nodes) => node.position?.start.line ?? 1; + const offsetOf = (node: Nodes) => node.position?.start.offset ?? 0; + const endOf = (node: Nodes) => node.position?.end.offset ?? text.length; const addText = (value: string) => { if (heading) { @@ -252,7 +262,7 @@ export function parse(path: string, text: string): Doc { if (itemDepth > 0) fresh = false; }; - const link = (node: Nodes, target: string) => { + const link = (node: Nodes, target: string, span: [number, number]) => { // Reference-style links resolve to their definition, so they carry an // edge like any other link. Only a top-level, unquoted item can hold an // entry. @@ -260,7 +270,7 @@ export function parse(path: string, text: string): Doc { itemDepth === 0 ? "prose" : fresh && itemDepth === 1 && quoteDepth === 0 ? "entry" : "inside"; fresh = false; if (position === "entry" && entry) entry.target = target; - links.push({ line: lineOf(node), section, target, position }); + links.push({ line: lineOf(node), section, target, position, span }); }; const enter = (node: Nodes) => { @@ -278,20 +288,29 @@ export function parse(path: string, text: string): Doc { break; case "listItem": if (itemDepth === 0 && quoteDepth === 0) { - entry = { line: lineOf(node), section, text: "", target: null }; + entry = { + line: lineOf(node), + end: node.position?.end.line ?? lineOf(node), + section, + text: "", + target: null, + }; } itemDepth++; fresh = true; break; - case "link": - link(node, node.url); + case "link": { + // The destination follows the text, which may mention it too. + const last = node.children.at(-1); + link(node, node.url, [last ? endOf(last) : offsetOf(node), endOf(node)]); break; + } case "linkReference": { - const url = definitions.get(node.identifier); - if (url === undefined) { + const definition = definitions.get(node.identifier); + if (definition === undefined) { other(); } else { - link(node, url); + link(node, definition.url, definition.span); } break; } @@ -405,12 +424,17 @@ export function parse(path: string, text: string): Doc { return { path, title, headings, links, entries: found }; } -/** The first definition of each reference label, which is the one CommonMark uses. */ -function collectDefinitions(tree: Root): Map { - const definitions = new Map(); +/** + * The first definition of each reference label, which is the one CommonMark + * uses, with the span its destination is written in: after the label's `]:`. + */ +function collectDefinitions(tree: Root, text: string): Map { + const definitions = new Map(); const visit = (node: Nodes) => { if (node.type === "definition" && !definitions.has(node.identifier)) { - definitions.set(node.identifier, node.url); + const start = node.position?.start.offset ?? 0; + const end = node.position?.end.offset ?? text.length; + definitions.set(node.identifier, { url: node.url, span: [text.indexOf("]:", start) + 2, end] }); } if ("children" in node) for (const child of node.children) visit(child); }; diff --git a/src/core/edit.ts b/src/core/edit.ts new file mode 100644 index 00000000..702bf160 --- /dev/null +++ b/src/core/edit.ts @@ -0,0 +1,141 @@ +// Moving and deleting quests, as pure edits of the documents that link to them. +// +// The caller owns the files: it moves or deletes them, then writes back what +// these return. Only root-absolute links are edited, since those are the +// dependency graph. Anything else that mentions a path is prose, which the +// caller reports rather than rewriting. + +import { type Doc, parse, rooted, withoutFragment } from "./doc"; +import { LIST_SECTIONS } from "./rules"; + +/** One quest document as loaded: its repository-relative path and source text. */ +export interface Source { + path: string; + text: string; +} + +/** A link that names a moved path but whose destination is not written as parsed, such as one with escapes. */ +export interface Missed { + path: string; + line: number; + target: string; +} + +/** What a move changes. */ +export interface Moved { + /** The new text of every document whose links changed, keyed by its path after the move. */ + texts: Map; + /** Links to a moved path that could not be rewritten in place. */ + missed: Missed[]; +} + +/** + * Where `path` lands when `from` moves to `to`, or `null` if it stays. `from` + * is a quest, or an epic directory that moves everything beneath it. + */ +export function relocate(path: string, from: string, to: string): string | null { + if (path === from) return to; + return path.startsWith(`${from}/`) ? to + path.slice(from.length) : null; +} + +/** + * Rewrite every root-absolute link to `from`, or to a document under it, to + * point at its new path under `to`. Fragments are kept; each path segment is + * percent-encoded the way a Markdown destination needs. + */ +export function move(sources: Source[], from: string, to: string): Moved { + const texts = new Map(); + const missed: Missed[] = []; + for (const source of sources) { + const doc = parse(source.path, source.text); + const edits = new Map(); + for (const link of doc.links) { + const target = rooted(link.target); + const moved = target === null ? null : relocate(target, from, to); + if (moved === null) continue; + const at = source.text.indexOf(link.target, link.span[0]); + if (at < 0 || at + link.target.length > link.span[1]) { + missed.push({ path: source.path, line: link.line, target: link.target }); + continue; + } + const fragment = link.target.slice(withoutFragment(link.target).length); + // Reference-style links share their definition, which is rewritten once. + edits.set(at, { length: link.target.length, text: `/${encode(moved)}${fragment}` }); + } + if (edits.size > 0) texts.set(relocate(source.path, from, to) ?? source.path, splice(source.text, edits)); + } + return { texts, missed }; +} + +/** + * Drop every list entry, in a list section, that opens with a link to a + * deleted document, and a section heading left with no entries. Returns the + * new text of every surviving document that changed. + */ +export function remove(sources: Source[], deleted: (path: string) => boolean): Map { + const texts = new Map(); + for (const source of sources) { + if (deleted(source.path)) continue; + const text = drop(source.text, parse(source.path, source.text), deleted); + if (text !== null) texts.set(source.path, text); + } + return texts; +} + +/** `doc` without its list entries whose link `deleted` accepts, or `null` if it has none. */ +function drop(text: string, doc: Doc, deleted: (path: string) => boolean): string | null { + const lines = text.split("\n"); + // A trailing newline leaves an empty last element, which is not a line. + const count = text.endsWith("\n") ? lines.length - 1 : lines.length; + const dropped = new Set(); + for (const entry of doc.entries) { + if (entry.section === null || !LIST_SECTIONS.includes(entry.section) || entry.target === null) continue; + const target = rooted(entry.target); + if (target === null || !deleted(target)) continue; + for (let line = entry.line; line <= entry.end; line++) dropped.add(line - 1); + } + if (dropped.size === 0) return null; + + // A section that lost entries and has nothing left but blank lines goes + // with its heading: an empty `Required` would block its quest forever. + doc.headings.forEach((heading, index) => { + if (!LIST_SECTIONS.includes(heading.text)) return; + const end = index + 1 < doc.headings.length ? doc.headings[index + 1].line - 1 : count; + const body = Array.from({ length: end - heading.line }, (_, i) => heading.line + i); + if (!body.some((i) => dropped.has(i))) return; + if (!body.every((i) => dropped.has(i) || lines[i].trim() === "")) return; + dropped.add(heading.line - 1); + for (const i of body) dropped.add(i); + }); + + // Removing a block between two blank lines would leave both; keep one. + const out: string[] = []; + let gap = false; + lines.forEach((line, i) => { + if (dropped.has(i)) { + gap = true; + return; + } + const blank = line.trim() === ""; + if (!(gap && blank && out.length > 0 && out[out.length - 1].trim() === "")) out.push(line); + gap = false; + }); + return out.join("\n"); +} + +/** A repository-relative path as a root-absolute Markdown destination's path. */ +function encode(path: string): string { + return path.split("/").map(encodeURIComponent).join("/"); +} + +/** `text` with each replacement, keyed by offset, applied. */ +function splice(text: string, edits: Map): string { + let out = ""; + let from = 0; + for (const at of [...edits.keys()].sort((a, b) => a - b)) { + const edit = edits.get(at)!; + out += text.slice(from, at) + edit.text; + from = at + edit.length; + } + return out + text.slice(from); +} diff --git a/src/core/index.ts b/src/core/index.ts index d92c07bb..005bbbe1 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -23,6 +23,7 @@ export { resolve, rooted, } from "./doc"; +export { type Missed, type Moved, type Source, move, relocate, remove } from "./edit"; export { comparePaths, normalize } from "./path"; export { type Blocker, blockers, label, lookup, ready, renderBlocker } from "./ready"; export { type Finding, ROOT, check, formatFinding } from "./rules"; diff --git a/src/core/rules.ts b/src/core/rules.ts index f1c1002d..9f207e6e 100644 --- a/src/core/rules.ts +++ b/src/core/rules.ts @@ -27,7 +27,7 @@ const SIZES = ["XS", "S", "M", "L", "XL"]; * last entry was removed is a bug: an empty `Required` blocks its quest * forever. */ -const LIST_SECTIONS = ["Required", "Closes", "Related"]; +export const LIST_SECTIONS = ["Required", "Closes", "Related"]; /** The permanent root epic; the one document nothing has to list. */ export const ROOT = "quest/README.md"; diff --git a/test/move.test.ts b/test/move.test.ts new file mode 100644 index 00000000..c3f284a4 --- /dev/null +++ b/test/move.test.ts @@ -0,0 +1,313 @@ +// `quest mv` and `quest rm`: the core edits, and the CLI's git and claim handling. + +import { execFileSync } from "node:child_process"; +import { existsSync } from "node:fs"; +import { join } from "node:path"; + +import { describe, expect, test } from "vitest"; + +import { move, remove } from "../src/core"; +import { openPullRequests } from "../src/cli/claims"; +import { GitHub, type GitHubDependencies } from "../src/cli/github"; +import { main } from "../src/cli/main"; +import { ROOT_README, Tree, tempDir } from "./fixture"; + +function git(dir: string, ...args: string[]): string { + return execFileSync("git", ["-C", dir, ...args], { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }); +} + +/** `tree` as a git repository with everything committed. */ +function committed(tree: Tree): Tree { + git(tree.path, "init", "-q", "-b", "main"); + git(tree.path, "add", "-A"); + git( + tree.path, + "-c", + "user.name=Test", + "-c", + "user.email=test@example.com", + "-c", + "commit.gpgsign=false", + "commit", + "-q", + "-m", + "init", + ); + return tree; +} + +async function quest(tree: Tree, ...args: string[]) { + let stdout = ""; + let stderr = ""; + const code = await main(["--root", tree.path, ...args], { + stdout: (text) => { + stdout += text; + }, + stderr: (text) => { + stderr += text; + }, + }); + return { code, stdout, stderr }; +} + +/** Staged changes, as `git status --short` prints them. */ +function staged(tree: Tree): string[] { + return git(tree.path, "status", "--short").split("\n").filter(Boolean).sort(); +} + +describe("move", () => { + test("rewrites inline and reference links, keeping fragments and encoding names", () => { + const text = + "# [S] A\n\n## Goal\n\nSee [b](/quest/b.md#plan) and [ref][b].\n\n" + + "## Related\n\n- [/quest/b.md](/quest/b.md)\n\n[b]: /quest/b.md\n"; + const moved = move([{ path: "quest/a.md", text }], "quest/b.md", "quest/m1/new name.md"); + expect(moved.missed).toEqual([]); + expect(moved.texts.get("quest/a.md")).toBe( + "# [S] A\n\n## Goal\n\nSee [b](/quest/m1/new%20name.md#plan) and [ref][b].\n\n" + + "## Related\n\n- [/quest/b.md](/quest/m1/new%20name.md)\n\n[b]: /quest/m1/new%20name.md\n", + ); + }); + + test("moves an epic's documents as a prefix and keys them by their new path", () => { + const moved = move( + [ + { + path: "quest/m0/README.md", + text: "# M0\n\n## Goal\n\nG.\n\n## Required\n\n- [E](/quest/m0/e/README.md)\n", + }, + { + path: "quest/m0/e/README.md", + text: "# E\n\n## Goal\n\nG.\n\n## Required\n\n- [A](/quest/m0/e/a.md)\n", + }, + { path: "quest/m0/e/a.md", text: "# [S] A\n\n## Goal\n\nG.\n" }, + { path: "quest/m0/elsewhere.md", text: "# [S] X\n\n## Related\n\n- [E2](/quest/m0/e2.md)\n" }, + ], + "quest/m0/e", + "quest/m1/e", + ); + expect([...moved.texts.keys()]).toEqual(["quest/m0/README.md", "quest/m1/e/README.md"]); + expect(moved.texts.get("quest/m0/README.md")).toContain("- [E](/quest/m1/e/README.md)\n"); + expect(moved.texts.get("quest/m1/e/README.md")).toContain("- [A](/quest/m1/e/a.md)\n"); + }); + + test("reports a link whose destination is not written as parsed", () => { + const text = "# [S] A\n\n## Goal\n\n[b](/quest/b.md)\n"; + const moved = move([{ path: "quest/a.md", text }], "quest/b.md", "quest/c.md"); + expect(moved.texts.size).toBe(0); + expect(moved.missed).toEqual([{ path: "quest/a.md", line: 5, target: "/quest/b.md" }]); + }); +}); + +describe("remove", () => { + const gone = (path: string) => path === "quest/b.md"; + + test("drops entries, and a section left empty with its heading", () => { + const text = + "# [S] A\n\n## Goal\n\nG.\n\n## Required\n\n- [B](/quest/b.md) - first\n\n" + + "## Related\n\n- [C](/quest/c.md)\n- [B](/quest/b.md)\n wrapped\n- [D](/quest/d.md)\n"; + expect(remove([{ path: "quest/a.md", text }], gone).get("quest/a.md")).toBe( + "# [S] A\n\n## Goal\n\nG.\n\n## Related\n\n- [C](/quest/c.md)\n- [D](/quest/d.md)\n", + ); + }); + + test("keeps one blank line in a loose list and at the end of the file", () => { + const loose = + "# [S] A\n\n## Goal\n\nG.\n\n## Related\n\n- [C](/quest/c.md)\n\n- [B](/quest/b.md)\n\n- [D](/quest/d.md)\n"; + expect(remove([{ path: "quest/a.md", text: loose }], gone).get("quest/a.md")).toBe( + "# [S] A\n\n## Goal\n\nG.\n\n## Related\n\n- [C](/quest/c.md)\n\n- [D](/quest/d.md)\n", + ); + const last = "# [S] A\n\n## Goal\n\nG.\n\n## Required\n\n- [B](/quest/b.md)\n"; + expect(remove([{ path: "quest/a.md", text: last }], gone).get("quest/a.md")).toBe("# [S] A\n\n## Goal\n\nG.\n"); + }); + + test("leaves prose, sections with other content, and other sections alone", () => { + const text = + "# [S] A\n\n## Goal\n\nAfter [B](/quest/b.md).\n\n- [B](/quest/b.md) in a goal list\n\n" + + "## Related\n\nContext.\n\n- [B](/quest/b.md)\n"; + expect(remove([{ path: "quest/a.md", text }], gone).get("quest/a.md")).toBe( + "# [S] A\n\n## Goal\n\nAfter [B](/quest/b.md).\n\n- [B](/quest/b.md) in a goal list\n\n## Related\n\nContext.\n", + ); + }); +}); + +describe("quest mv", () => { + test("moves a quest, rewrites its links, and stages the change", async () => { + const tree = committed(Tree.baseline()); + const out = await quest(tree, "mv", "/quest/m0/epic/one.md", "quest/m0/epic/first.md"); + expect(out).toEqual({ + code: 0, + stdout: "R quest/m0/epic/one.md -> quest/m0/epic/first.md\nM quest/m0/epic/README.md\nM quest/m0/epic/two.md\n", + stderr: "quest: no origin remote, so claims were not checked\n", + }); + expect(tree.read("quest/m0/epic/two.md")).toContain("- [One](/quest/m0/epic/first.md) - must finish first"); + expect(staged(tree)).toEqual([ + "M quest/m0/epic/README.md", + "M quest/m0/epic/two.md", + "R quest/m0/epic/one.md -> quest/m0/epic/first.md", + ]); + tree.accepts(); + }); + + test("moves an epic into a new directory", async () => { + const tree = committed(Tree.baseline()); + const out = await quest(tree, "mv", "quest/m0/epic", "quest/m1/later/epic"); + expect(out.stdout).toBe( + "R quest/m0/epic -> quest/m1/later/epic\nM quest/m0/README.md\nM quest/m1/later/epic/README.md\nM quest/m1/later/epic/two.md\n", + ); + expect(tree.read("quest/m1/later/epic/README.md")).toContain("- [One](/quest/m1/later/epic/one.md)\n"); + // The epic left m0 for a directory with no README, which check reports rather than mv guessing. + expect(out.code).toBe(1); + expect(out.stderr).toContain("quest/m1/later/epic/README.md: not listed in quest/m1/later/README.md"); + }); + + test("reports mentions it does not edit", async () => { + const tree = Tree.baseline() + .append("quest/m0/epic/two.md", "\n## Plan\n\nFollows `quest/m0/epic/one.md`.\n") + .write("notes.md", "See quest/m0/epic/one.md.\n"); + committed(tree); + const out = await quest(tree, "mv", "quest/m0/epic/one.md", "quest/m0/epic/first.md"); + expect(out.code).toBe(0); + expect(out.stderr).toContain("quest: notes.md:1: mentions quest/m0/epic/one.md; update it by hand\n"); + expect(out.stderr).toContain( + "quest: quest/m0/epic/two.md:13: mentions quest/m0/epic/one.md; update it by hand\n", + ); + }); + + test("refuses a destination that exists", async () => { + const tree = committed(Tree.baseline()); + const out = await quest(tree, "mv", "quest/m0/epic/one.md", "quest/m0/epic/two.md"); + expect(out).toEqual({ code: 1, stdout: "", stderr: "quest: quest/m0/epic/two.md already exists\n" }); + expect(staged(tree)).toEqual([]); + }); + + test("refuses a claimed quest", async () => { + const tree = committed(Tree.baseline()); + const remote = tempDir(); + git(remote, "init", "-q", "--bare"); + git(tree.path, "remote", "add", "origin", remote); + git(tree.path, "push", "-q", "origin", "HEAD:refs/heads/quest/m0/epic/two"); + const out = await quest(tree, "mv", "quest/m0/epic", "quest/m0/feature"); + expect(out.code).toBe(1); + expect(out.stdout).toBe(""); + expect(out.stderr).toContain("quest: origin is not on GitHub, so only its branches were checked for claims\n"); + expect(out.stderr).toContain("quest: refusing to move claimed quests: quest/m0/epic/two;"); + expect(existsSync(join(tree.path, "quest/m0/epic/two.md"))).toBe(true); + expect(staged(tree)).toEqual([]); + }); + + test("fails outside a git work tree", async () => { + const out = await quest(Tree.baseline(), "mv", "quest/m0/epic/one.md", "quest/m0/epic/first.md"); + expect(out.code).toBe(1); + expect(out.stderr).toContain("is not in a git work tree"); + }); +}); + +describe("quest rm", () => { + test("deletes a quest listed under several Required and Related sections", async () => { + const tree = Tree.baseline() + .append("quest/m0/epic/README.md", "- [Three](/quest/m0/epic/three.md)\n") + .write( + "quest/m0/epic/three.md", + "# [S] Three\n\n## Goal\n\nMore.\n\n## Required\n\n- [Two](/quest/m0/epic/two.md)\n- [One](/quest/m0/epic/one.md)\n\n" + + "## Related\n\n- [One](/quest/m0/epic/one.md) - similar\n", + ); + committed(tree); + const out = await quest(tree, "rm", "quest/m0/epic/one.md"); + expect(out).toEqual({ + code: 0, + stdout: "D quest/m0/epic/one.md\nM quest/m0/epic/README.md\nM quest/m0/epic/three.md\nM quest/m0/epic/two.md\n", + stderr: "", + }); + expect(tree.read("quest/m0/epic/two.md")).toBe("# [S] Two\n\n## Goal\n\nAnother quest.\n"); + expect(tree.read("quest/m0/epic/three.md")).toBe( + "# [S] Three\n\n## Goal\n\nMore.\n\n## Required\n\n- [Two](/quest/m0/epic/two.md)\n", + ); + expect(staged(tree)).toContain("D quest/m0/epic/one.md"); + tree.accepts(); + }); + + test("deletes a milestone's last child while the root still lists the milestone", async () => { + const tree = Tree.baseline() + .write("quest/README.md", `${ROOT_README}- [M1](/quest/m1/README.md)\n`) + .write("quest/m1/README.md", "# M1\n\n## Goal\n\nNext.\n\n## Required\n\n- [Later](/quest/m1/later.md)\n") + .write( + "quest/m1/later.md", + "# [S] Later\n\n## Goal\n\nAfter m0.\n\n## Required\n\n- [M0](/quest/m0/README.md)\n", + ); + committed(tree); + const out = await quest(tree, "rm", "-r", "quest/m0/epic"); + expect(out.code).toBe(0); + expect(out.stdout).toBe( + "D quest/m0/epic/README.md\nD quest/m0/epic/one.md\nD quest/m0/epic/two.md\nM quest/m0/README.md\n", + ); + expect(tree.read("quest/m0/README.md")).toBe("# M0\n\n## Goal\n\nA milestone.\n"); + expect(tree.read("quest/README.md")).toContain("- [M0](/quest/m0/README.md)\n"); + tree.accepts(); + }); + + test("refuses an epic that holds quests without -r, by directory or README", async () => { + const tree = committed(Tree.baseline()); + for (const path of ["quest/m0/epic", "quest/m0/epic/README.md"]) { + const out = await quest(tree, "rm", path); + expect(out.code).toBe(1); + expect(out.stderr).toBe( + `quest: ${path} is an epic holding 3 quest documents; pass -r to remove them all\n`, + ); + } + expect(staged(tree)).toEqual([]); + }); + + test("refuses the root epic and milestones", async () => { + const tree = committed(Tree.baseline()); + for (const path of ["quest/m0", "quest/m0/README.md", "quest/README.md"]) { + expect((await quest(tree, "rm", "-r", path)).stderr).toContain("is permanent"); + } + }); + + test("reports a prose link it does not edit, and the check that fails on it", async () => { + const tree = Tree.baseline().append( + "quest/m0/epic/two.md", + "\n## Plan\n\nReuse [one](/quest/m0/epic/one.md).\n", + ); + committed(tree); + const out = await quest(tree, "rm", "quest/m0/epic/one.md"); + expect(out.code).toBe(1); + expect(out.stderr).toContain( + "quest: quest/m0/epic/two.md:9: mentions quest/m0/epic/one.md; update it by hand\n", + ); + expect(out.stderr).toContain("quest: quest/m0/epic/two.md:9: link does not resolve: /quest/m0/epic/one.md\n"); + }); +}); + +test("open pull requests come from every page, by head branch", async () => { + const requested: string[] = []; + const deps: GitHubDependencies = { + token: "test-token", + now: () => 0, + sleep: async () => undefined, + warn: () => undefined, + fetch: async (url) => { + requested.push(url.pathname + url.search); + const page = url.searchParams.get("page"); + const head = (ref: string, n: number) => ({ + head: { ref }, + html_url: `https://github.com/example/project/pull/${n}`, + }); + return page === null + ? new Response(JSON.stringify([head("quest/m0/a", 1)]), { + headers: { + link: '; rel="next"', + }, + }) + : new Response(JSON.stringify([head("feature", 2)])); + }, + }; + expect(await openPullRequests(new GitHub(deps), { owner: "example", repo: "project" })).toEqual([ + { branch: "quest/m0/a", url: "https://github.com/example/project/pull/1" }, + { branch: "feature", url: "https://github.com/example/project/pull/2" }, + ]); + expect(requested).toEqual([ + "/repos/example/project/pulls?state=open&per_page=100", + "/repos/example/project/pulls?state=open&page=2", + ]); +}); From 8ebde166d872dd461e5b3d618aa0da509ac263aa Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 11:47:20 -0700 Subject: [PATCH 3/6] feat(rm): drop Required links to finished milestones quest rm now also drops every Required link to a finished milestone, including one it just emptied and links that were already stale, and repeats until nothing changes. The root's listing stays. It reuses the finished-milestone predicate from the check rule (#160). The guide's Deletion section points at quest rm instead of a grep. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 ++ assets/AGENTS.md | 2 +- src/cli/main.ts | 4 +++- src/core/edit.ts | 44 +++++++++++++++++++++++++++++--------------- src/core/rules.ts | 37 ++++++++++++++++++++++++++++++------- test/move.test.ts | 34 ++++++++++++++++++++++++++++++++-- 6 files changed, 97 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index b84f5941..62da3728 100644 --- a/README.md +++ b/README.md @@ -161,6 +161,8 @@ quest rm -r quest/m1/drain `mv` rewrites every root-absolute link to the moved path, and to everything under a moved epic. `rm` drops each `Required`, `Related`, or `Closes` entry that links to what it deleted, and the heading if that was the last entry. +It also drops every `Required` link to a finished milestone, including one it +just finished, but keeps the root's listing of the milestone. Both stage the change, print one line per file (`R from -> to`, `D path`, or `M path`), and report on stderr any other mention of the path, such as prose or files outside `quest/`, without editing it. They run `quest check` on the diff --git a/assets/AGENTS.md b/assets/AGENTS.md index c071251a..477f2595 100644 --- a/assets/AGENTS.md +++ b/assets/AGENTS.md @@ -147,6 +147,6 @@ Any findings that seem out-of-scope, or bugs encountered during the process, sho A quest that is no longer needed or cannot be completed is abandoned. Delete it and explain why in the PR. -Grep its absolute path and remove every reference. +Use `quest rm`, which deletes the quest and every link to it. Remove the `quest` label from issues no other quest tracks. diff --git a/src/cli/main.ts b/src/cli/main.ts index 22a9ee45..b7b0c45b 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -209,7 +209,9 @@ export function main(argv: string[], out: Output): number | Promise { .summary("Delete a quest or epic and drop the links to it") .description( "Delete a quest, or an epic and the quests under it, with git rm, and drop every Required, Related, " + - "or Closes entry that links to it, with any heading left empty. The change is staged.\n\n" + + "or Closes entry that links to it, with any heading left empty. It also drops every Required link to a " + + "finished milestone, one whose README has no Required left, except the root's own listing. The change " + + "is staged.\n\n" + "Refuses outside a git work tree. An epic is its directory or README; removing one that holds other " + "quests requires -r. The root epic and milestones are permanent.\n\n" + "Prints D for each deleted quest and M for each edited document. Mentions that are not list entries, " + diff --git a/src/core/edit.ts b/src/core/edit.ts index 702bf160..2bc2ed00 100644 --- a/src/core/edit.ts +++ b/src/core/edit.ts @@ -5,8 +5,8 @@ // dependency graph. Anything else that mentions a path is prose, which the // caller reports rather than rewriting. -import { type Doc, parse, rooted, withoutFragment } from "./doc"; -import { LIST_SECTIONS } from "./rules"; +import { type Doc, type Entry, parse, rooted, withoutFragment } from "./doc"; +import { LIST_SECTIONS, finishedMilestone } from "./rules"; /** One quest document as loaded: its repository-relative path and source text. */ export interface Source { @@ -69,29 +69,43 @@ export function move(sources: Source[], from: string, to: string): Moved { /** * Drop every list entry, in a list section, that opens with a link to a - * deleted document, and a section heading left with no entries. Returns the - * new text of every surviving document that changed. + * deleted document, and a section heading left with no entries. Then drop + * every `Required` link to a finished milestone, whether this removal emptied + * it or the link was already stale, repeating until nothing changes so the + * result passes `quest check`. Returns the new text of every surviving + * document that changed. */ export function remove(sources: Source[], deleted: (path: string) => boolean): Map { - const texts = new Map(); - for (const source of sources) { - if (deleted(source.path)) continue; - const text = drop(source.text, parse(source.path, source.text), deleted); - if (text !== null) texts.set(source.path, text); + const texts = new Map(sources.filter((s) => !deleted(s.path)).map((s) => [s.path, s.text])); + const changed = new Set(); + const linksDeleted = (entry: Entry) => { + if (entry.section === null || !LIST_SECTIONS.includes(entry.section) || entry.target === null) return false; + const target = rooted(entry.target); + return target !== null && deleted(target); + }; + for (let progress = true; progress;) { + progress = false; + const docs = [...texts].map(([path, text]) => parse(path, text)); + const finished = finishedMilestone(docs); + for (const doc of docs) { + const text = drop(texts.get(doc.path)!, doc, (entry) => linksDeleted(entry) || finished(doc, entry)); + if (text === null) continue; + texts.set(doc.path, text); + changed.add(doc.path); + progress = true; + } } - return texts; + return new Map([...changed].map((path) => [path, texts.get(path)!])); } -/** `doc` without its list entries whose link `deleted` accepts, or `null` if it has none. */ -function drop(text: string, doc: Doc, deleted: (path: string) => boolean): string | null { +/** `doc` without the list entries `dropping` accepts, or `null` if it has none. */ +function drop(text: string, doc: Doc, dropping: (entry: Entry) => boolean): string | null { const lines = text.split("\n"); // A trailing newline leaves an empty last element, which is not a line. const count = text.endsWith("\n") ? lines.length - 1 : lines.length; const dropped = new Set(); for (const entry of doc.entries) { - if (entry.section === null || !LIST_SECTIONS.includes(entry.section) || entry.target === null) continue; - const target = rooted(entry.target); - if (target === null || !deleted(target)) continue; + if (!dropping(entry)) continue; for (let line = entry.line; line <= entry.end; line++) dropped.add(line - 1); } if (dropped.size === 0) return null; diff --git a/src/core/rules.ts b/src/core/rules.ts index 19bc3291..8bfff295 100644 --- a/src/core/rules.ts +++ b/src/core/rules.ts @@ -11,7 +11,19 @@ // entry for a completed quest survived a rebase that produced no conflict at // all. -import { type Doc, entries, has, isEpic, children, owner, permanent, resolve, rooted, withoutFragment } from "./doc"; +import { + type Doc, + type Entry, + entries, + has, + isEpic, + children, + owner, + permanent, + resolve, + rooted, + withoutFragment, +} from "./doc"; import { comparePaths, join } from "./path"; /** @@ -231,14 +243,10 @@ function index(found: Finding[], known: Set, docs: Doc[]) { * membership, not a blocker, so it is exempt. */ function milestones(found: Finding[], docs: Doc[]) { - const byPath = new Map(docs.map((d) => [d.path, d])); + const finished = finishedMilestone(docs); for (const doc of docs) { - if (doc.path === ROOT) continue; for (const entry of entries(doc, "Required")) { - const target = entry.target === null ? null : rooted(entry.target); - if (target === null || target === ROOT || !permanent(target)) continue; - const milestone = byPath.get(target); - if (milestone === undefined || has(milestone, "Required")) continue; + if (!finished(doc, entry)) continue; found.push({ path: doc.path, line: entry.line, @@ -248,6 +256,21 @@ function milestones(found: Finding[], docs: Doc[]) { } } +/** + * Whether `entry`, in `doc`, is a `Required` link to a milestone of `docs` + * that is finished. Shared with `quest rm`, which drops these links. + */ +export function finishedMilestone(docs: Doc[]): (doc: Doc, entry: Entry) => boolean { + const byPath = new Map(docs.map((d) => [d.path, d])); + return (doc, entry) => { + if (doc.path === ROOT || entry.section !== "Required" || entry.target === null) return false; + const target = rooted(entry.target); + if (target === null || target === ROOT || !permanent(target)) return false; + const milestone = byPath.get(target); + return milestone !== undefined && !has(milestone, "Required"); + }; +} + /** * `Required` must be acyclic. A cycle is a set of quests none of which can ever * start, and walking the links to rule one out is exactly the manual step an diff --git a/test/move.test.ts b/test/move.test.ts index c3f284a4..f3d24797 100644 --- a/test/move.test.ts +++ b/test/move.test.ts @@ -128,6 +128,35 @@ describe("remove", () => { "# [S] A\n\n## Goal\n\nAfter [B](/quest/b.md).\n\n- [B](/quest/b.md) in a goal list\n\n## Related\n\nContext.\n", ); }); + test("drops links to finished milestones until nothing changes, keeping the root's listing", () => { + const root = + "# Quests\n\n## Goal\n\nG.\n\n## Required\n\n- [M0](/quest/m0/README.md)\n- [M1](/quest/m1/README.md)\n"; + const sources = [ + { path: "quest/README.md", text: root }, + // m0 finishes once b goes, which finishes m1, which only waited on m0. + { path: "quest/m0/README.md", text: "# M0\n\n## Goal\n\nG.\n\n## Required\n\n- [B](/quest/b.md)\n" }, + { + path: "quest/m1/README.md", + text: "# M1\n\n## Goal\n\nG.\n\n## Required\n\n- [M0](/quest/m0/README.md)\n", + }, + { path: "quest/b.md", text: "# [S] B\n\n## Goal\n\nG.\n" }, + { + path: "quest/m1/a.md", + text: + "# [S] A\n\n## Goal\n\nG.\n\n## Required\n\n- [M1](/quest/m1/README.md)\n- [M2](/quest/m2/README.md)\n\n" + + "## Related\n\n- [M0](/quest/m0/README.md)\n", + }, + // m2 was already finished, so the link to it was already stale. + { path: "quest/m2/README.md", text: "# M2\n\n## Goal\n\nG.\n" }, + ]; + const texts = remove(sources, gone); + expect([...texts.keys()].sort()).toEqual(["quest/m0/README.md", "quest/m1/README.md", "quest/m1/a.md"]); + expect(texts.get("quest/m0/README.md")).toBe("# M0\n\n## Goal\n\nG.\n"); + expect(texts.get("quest/m1/README.md")).toBe("# M1\n\n## Goal\n\nG.\n"); + expect(texts.get("quest/m1/a.md")).toBe( + "# [S] A\n\n## Goal\n\nG.\n\n## Related\n\n- [M0](/quest/m0/README.md)\n", + ); + }); }); describe("quest mv", () => { @@ -226,7 +255,7 @@ describe("quest rm", () => { tree.accepts(); }); - test("deletes a milestone's last child while the root still lists the milestone", async () => { + test("drops links to the milestone it finishes while the root still lists it", async () => { const tree = Tree.baseline() .write("quest/README.md", `${ROOT_README}- [M1](/quest/m1/README.md)\n`) .write("quest/m1/README.md", "# M1\n\n## Goal\n\nNext.\n\n## Required\n\n- [Later](/quest/m1/later.md)\n") @@ -238,9 +267,10 @@ describe("quest rm", () => { const out = await quest(tree, "rm", "-r", "quest/m0/epic"); expect(out.code).toBe(0); expect(out.stdout).toBe( - "D quest/m0/epic/README.md\nD quest/m0/epic/one.md\nD quest/m0/epic/two.md\nM quest/m0/README.md\n", + "D quest/m0/epic/README.md\nD quest/m0/epic/one.md\nD quest/m0/epic/two.md\nM quest/m0/README.md\nM quest/m1/later.md\n", ); expect(tree.read("quest/m0/README.md")).toBe("# M0\n\n## Goal\n\nA milestone.\n"); + expect(tree.read("quest/m1/later.md")).toBe("# [S] Later\n\n## Goal\n\nAfter m0.\n"); expect(tree.read("quest/README.md")).toContain("- [M0](/quest/m0/README.md)\n"); tree.accepts(); }); From 99a307122bb067207474942b1c1706981afc8ff8 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 11:53:50 -0700 Subject: [PATCH 4/6] fix(mv): keep milestones put, report epic mentions, check upstream claims - mv refuses milestones as well as the root, as rm does. - Mentions of a moved or removed epic match its directory path itself, not only paths under it, while skipping longer sibling names. - In a fork's clone, claims also count open PRs in the repository the fork came from. Co-Authored-By: Claude Opus 5.5 --- README.md | 7 ++++--- src/cli/claims.ts | 30 +++++++++++++++++++++++------ src/cli/main.ts | 3 ++- src/cli/move.ts | 19 ++++++++++++------ test/move.test.ts | 49 ++++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 91 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 62da3728..be88c9d3 100644 --- a/README.md +++ b/README.md @@ -169,9 +169,10 @@ or files outside `quest/`, without editing it. They run `quest check` on the result and exit 1 if it fails, for example when a quest moved out of its epic. An epic is its directory or README; removing one that holds other quests needs -`-r`. The root and milestones are never removed. `mv` refuses a destination -that exists, and refuses to move a quest while it has a claim branch on -`origin` or an open PR on GitHub, since the branch would no longer name it. +`-r`. The root and milestones are never moved or removed. `mv` refuses a +destination that exists, and refuses to move a quest while it has a claim +branch on `origin` or an open PR on GitHub, since the branch would no longer +name it. Neither command touches branches or PRs. ## Wait for pull request activity diff --git a/src/cli/claims.ts b/src/cli/claims.ts index d097076e..a82ac079 100644 --- a/src/cli/claims.ts +++ b/src/cli/claims.ts @@ -47,15 +47,30 @@ export async function openPullRequests( github: GitHub, repository: Repository, ): Promise<{ branch: string; url: string }[]> { - const base = `/repos/${encodeURIComponent(repository.owner)}/${encodeURIComponent(repository.repo)}`; - const rows = await github.all(`${base}/pulls?state=open&per_page=100`); + const rows = await github.all(`${path(repository)}/pulls?state=open&per_page=100`); return rows.map((row) => ({ branch: string(object(row.head).ref), url: string(row.html_url) })); } +/** + * The repositories whose open PRs can claim a quest: `repository`, and the + * one it was forked from, since a fork's clone names the fork `origin` while + * its quest PRs open upstream. + */ +export async function pullRequestRepositories(github: GitHub, repository: Repository): Promise { + const parent = object(await (await github.request(path(repository))).json()).parent; + if (parent === undefined || parent === null) return [repository]; + const [owner, repo] = string(object(parent).full_name).split("/"); + return [repository, { owner, repo }]; +} + +function path(repository: Repository): string { + return `/repos/${encodeURIComponent(repository.owner)}/${encodeURIComponent(repository.repo)}`; +} + /** * Which of `branches` someone holds: pushed to `remote`, or the head of an - * open PR there. Without the remote it warns and checks nothing; off GitHub - * it checks branches only. + * open PR there or in the repository `remote` was forked from. Without the + * remote it warns and checks nothing; off GitHub it checks branches only. */ export async function claims( root: string, @@ -77,8 +92,11 @@ export async function claims( if (repository === null) { deps.warn(`${remote} is not on GitHub, so only its branches were checked for claims`); } else { - for (const pr of await openPullRequests(new GitHub(deps.github()), repository)) { - if (wanted.has(pr.branch)) held.set(pr.branch, pr.url); + const github = new GitHub(deps.github()); + for (const each of await pullRequestRepositories(github, repository)) { + for (const pr of await openPullRequests(github, each)) { + if (wanted.has(pr.branch)) held.set(pr.branch, pr.url); + } } } return [...held].map(([branch, url]) => ({ branch, url })).sort((a, b) => (a.branch < b.branch ? -1 : 1)); diff --git a/src/cli/main.ts b/src/cli/main.ts index b7b0c45b..77ebb388 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -185,7 +185,8 @@ export function main(argv: string[], out: Output): number | Promise { .description( "Move a quest, or an epic directory and everything under it, with git mv, and rewrite every " + "root-absolute link to it under quest/. The change is staged.\n\n" + - "Refuses outside a git work tree, when the destination exists, and while a moved quest has a claim " + + "Refuses the root epic and milestones, which are permanent. Refuses outside a git work tree, when " + + "the destination exists, and while a moved quest has a claim " + "branch on origin or an open PR on GitHub, since the old branch would no longer name it. Moving a " + "quest into or out of an epic changes which epic lists it; the check afterwards reports what to fix.\n\n" + "Prints R for the move and M for each edited document. Mentions of the old path that are not links, " + diff --git a/src/cli/move.ts b/src/cli/move.ts index e240af0a..668ed2f1 100644 --- a/src/cli/move.ts +++ b/src/cli/move.ts @@ -4,7 +4,7 @@ import { execFileSync } from "node:child_process"; import { mkdirSync, statSync, writeFileSync } from "node:fs"; import { join, posix, relative, resolve, sep } from "node:path"; -import { ROOT, check, formatFinding, isQuest, move, normalize, permanent, relocate, remove } from "../core"; +import { check, formatFinding, isQuest, move, normalize, permanent, relocate, remove } from "../core"; import { type ClaimDependencies, claims, questBranch } from "./claims"; import { exists, load, sources, under } from "./tree"; @@ -32,9 +32,11 @@ export async function moveQuest( workTree(root); const from = treePath(root, fromArg); const to = treePath(root, toArg); - if (from === "quest" || from === ROOT) throw new Error("the root epic cannot move"); if (!exists(root, from)) throw new Error(`${fromArg} does not exist`); const directory = statSync(join(root, from)).isDirectory(); + if (from === "quest" || permanent(directory ? `${from}/README.md` : from)) { + throw new Error(`${fromArg} is permanent; the root epic and milestones never move`); + } if (!directory && !isQuest(from)) throw new Error(`${fromArg} is not a quest document`); if (!directory && !isQuest(to)) throw new Error(`${toArg} must name a Markdown quest document`); if (exists(root, to)) throw new Error(`${toArg} already exists`); @@ -66,7 +68,7 @@ export async function moveQuest( ); return { changes: [`R ${from} -> ${to}`, ...modified(edited.texts)], - warnings: [...warnings, ...mentions(root, directory ? `${from}/` : from)], + warnings: [...warnings, ...mentions(root, from)], findings: findings(root), }; } @@ -101,7 +103,7 @@ export function removeQuest(root: string, arg: string, recursive: boolean): Repo write(root, edited); return { changes: [...doomed.map((doc) => `D ${doc.path}`), ...modified(edited)], - warnings: mentions(root, whole ? `${epic}/` : target), + warnings: mentions(root, whole ? epic! : target), findings: findings(root), }; } @@ -149,11 +151,16 @@ function modified(texts: Map): string[] { return [...texts.keys()].sort().map((path) => `M ${path}`); } -/** Every tracked line that still names `path`, which no edit could safely change. */ +/** + * Every tracked line that still names `path`, or a path under it, which no + * edit could safely change. A longer sibling name, such as `drain.md` or + * `drain-2` for `drain`, is not a mention; a trailing full stop is. + */ function mentions(root: string, path: string): string[] { + const pattern = `${path.replace(/[.[\]()*+?{}|^$\\]/g, "\\$&")}\\.*([^A-Za-z0-9_.-]|$)`; let out: string; try { - out = execFileSync("git", ["-C", root, "grep", "-n", "-I", "-F", "-e", path], { + out = execFileSync("git", ["-C", root, "grep", "-n", "-I", "-E", "-e", pattern], { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"], }); diff --git a/test/move.test.ts b/test/move.test.ts index f3d24797..e3033dab 100644 --- a/test/move.test.ts +++ b/test/move.test.ts @@ -7,7 +7,7 @@ import { join } from "node:path"; import { describe, expect, test } from "vitest"; import { move, remove } from "../src/core"; -import { openPullRequests } from "../src/cli/claims"; +import { openPullRequests, pullRequestRepositories } from "../src/cli/claims"; import { GitHub, type GitHubDependencies } from "../src/cli/github"; import { main } from "../src/cli/main"; import { ROOT_README, Tree, tempDir } from "./fixture"; @@ -202,6 +202,32 @@ describe("quest mv", () => { ); }); + test("reports mentions of a moved epic's directory but not of its siblings", async () => { + const tree = Tree.baseline().write( + "notes.md", + "See quest/m0/epic.\nRun `quest rm -r quest/m0/epic` first.\nNot quest/m0/epic-2 or quest/m0/epic.md.\n", + ); + committed(tree); + const out = await quest(tree, "mv", "quest/m0/epic", "quest/m0/feature"); + expect(out.code).toBe(0); + expect(out.stderr).toBe( + "quest: no origin remote, so claims were not checked\n" + + "quest: notes.md:1: mentions quest/m0/epic; update it by hand\n" + + "quest: notes.md:2: mentions quest/m0/epic; update it by hand\n", + ); + }); + + test("refuses the root epic and milestones", async () => { + const tree = Tree.baseline().write("quest/m1/README.md", "# M1\n\n## Goal\n\nNext.\n"); + committed(tree); + for (const path of ["quest", "quest/README.md", "quest/m0", "quest/m0/README.md"]) { + const out = await quest(tree, "mv", path, "quest/m1/old"); + expect(out.code).toBe(1); + expect(out.stderr).toBe(`quest: ${path} is permanent; the root epic and milestones never move\n`); + } + expect(staged(tree)).toEqual([]); + }); + test("refuses a destination that exists", async () => { const tree = committed(Tree.baseline()); const out = await quest(tree, "mv", "quest/m0/epic/one.md", "quest/m0/epic/two.md"); @@ -341,3 +367,24 @@ test("open pull requests come from every page, by head branch", async () => { "/repos/example/project/pulls?state=open&page=2", ]); }); + +test("claims are checked in the repository a fork came from, too", async () => { + const repos: Record = { + "/repos/me/project": { fork: true, parent: { full_name: "example/project" } }, + "/repos/example/project": { fork: false }, + }; + const github = new GitHub({ + token: "test-token", + now: () => 0, + sleep: async () => undefined, + warn: () => undefined, + fetch: async (url) => new Response(JSON.stringify(repos[url.pathname])), + }); + expect(await pullRequestRepositories(github, { owner: "me", repo: "project" })).toEqual([ + { owner: "me", repo: "project" }, + { owner: "example", repo: "project" }, + ]); + expect(await pullRequestRepositories(github, { owner: "example", repo: "project" })).toEqual([ + { owner: "example", repo: "project" }, + ]); +}); From 73d51adac01a93c80a1df3701bf1fbb2acf29a7d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 12:06:33 -0700 Subject: [PATCH 5/6] fix(mv): count a fork parent's quest branches as claims In a fork's clone, a quest branch pushed upstream while its draft PR is opened is a claim too, so check the parent's quest/* branches through the GitHub API alongside its open PRs. Co-Authored-By: Claude Opus 5.5 --- README.md | 4 ++-- src/cli/claims.ts | 28 +++++++++++++++++++--------- src/cli/main.ts | 4 ++-- test/move.test.ts | 23 ++++++++++++----------- 4 files changed, 35 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index be88c9d3..d2398331 100644 --- a/README.md +++ b/README.md @@ -171,8 +171,8 @@ result and exit 1 if it fails, for example when a quest moved out of its epic. An epic is its directory or README; removing one that holds other quests needs `-r`. The root and milestones are never moved or removed. `mv` refuses a destination that exists, and refuses to move a quest while it has a claim -branch on `origin` or an open PR on GitHub, since the branch would no longer -name it. +branch or open PR on `origin`, or on the repository a GitHub fork came from, +since the branch would no longer name it. Neither command touches branches or PRs. ## Wait for pull request activity diff --git a/src/cli/claims.ts b/src/cli/claims.ts index a82ac079..17d07e10 100644 --- a/src/cli/claims.ts +++ b/src/cli/claims.ts @@ -52,15 +52,20 @@ export async function openPullRequests( } /** - * The repositories whose open PRs can claim a quest: `repository`, and the - * one it was forked from, since a fork's clone names the fork `origin` while - * its quest PRs open upstream. + * The repository `repository` was forked from, or `null` if it is not a fork. + * A fork's clone names the fork `origin`, while its quests are claimed upstream. */ -export async function pullRequestRepositories(github: GitHub, repository: Repository): Promise { +export async function forkParent(github: GitHub, repository: Repository): Promise { const parent = object(await (await github.request(path(repository))).json()).parent; - if (parent === undefined || parent === null) return [repository]; + if (parent === undefined || parent === null) return null; const [owner, repo] = string(object(parent).full_name).split("/"); - return [repository, { owner, repo }]; + return { owner, repo }; +} + +/** The `quest/*` branches in a GitHub `repository`, for one that is not a local remote. */ +export async function githubBranches(github: GitHub, repository: Repository): Promise { + const rows = await github.all(`${path(repository)}/git/matching-refs/heads/quest/`); + return rows.map((row) => string(row.ref).replace(/^refs\/heads\//, "")); } function path(repository: Repository): string { @@ -69,8 +74,9 @@ function path(repository: Repository): string { /** * Which of `branches` someone holds: pushed to `remote`, or the head of an - * open PR there or in the repository `remote` was forked from. Without the - * remote it warns and checks nothing; off GitHub it checks branches only. + * open PR there. When `remote` is a GitHub fork, its parent's branches and + * open PRs count too. Without the remote it warns and checks nothing; off + * GitHub it checks branches only. */ export async function claims( root: string, @@ -93,7 +99,11 @@ export async function claims( deps.warn(`${remote} is not on GitHub, so only its branches were checked for claims`); } else { const github = new GitHub(deps.github()); - for (const each of await pullRequestRepositories(github, repository)) { + const parent = await forkParent(github, repository); + if (parent !== null) { + for (const branch of await githubBranches(github, parent)) if (wanted.has(branch)) held.set(branch, null); + } + for (const each of parent === null ? [repository] : [repository, parent]) { for (const pr of await openPullRequests(github, each)) { if (wanted.has(pr.branch)) held.set(pr.branch, pr.url); } diff --git a/src/cli/main.ts b/src/cli/main.ts index 77ebb388..4a7556aa 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -186,8 +186,8 @@ export function main(argv: string[], out: Output): number | Promise { "Move a quest, or an epic directory and everything under it, with git mv, and rewrite every " + "root-absolute link to it under quest/. The change is staged.\n\n" + "Refuses the root epic and milestones, which are permanent. Refuses outside a git work tree, when " + - "the destination exists, and while a moved quest has a claim " + - "branch on origin or an open PR on GitHub, since the old branch would no longer name it. Moving a " + + "the destination exists, and while a moved quest has a claim branch or open PR on origin, or on " + + "the repository a GitHub fork came from, since the old branch would no longer name it. Moving a " + "quest into or out of an epic changes which epic lists it; the check afterwards reports what to fix.\n\n" + "Prints R for the move and M for each edited document. Mentions of the old path that are not links, " + "or are outside quest/, are reported on stderr and left alone. Runs quest check on the result and " + diff --git a/test/move.test.ts b/test/move.test.ts index e3033dab..d87ec216 100644 --- a/test/move.test.ts +++ b/test/move.test.ts @@ -7,7 +7,7 @@ import { join } from "node:path"; import { describe, expect, test } from "vitest"; import { move, remove } from "../src/core"; -import { openPullRequests, pullRequestRepositories } from "../src/cli/claims"; +import { forkParent, githubBranches, openPullRequests } from "../src/cli/claims"; import { GitHub, type GitHubDependencies } from "../src/cli/github"; import { main } from "../src/cli/main"; import { ROOT_README, Tree, tempDir } from "./fixture"; @@ -368,23 +368,24 @@ test("open pull requests come from every page, by head branch", async () => { ]); }); -test("claims are checked in the repository a fork came from, too", async () => { - const repos: Record = { +test("a fork's parent repository and its quest branches come from GitHub", async () => { + const responses: Record = { "/repos/me/project": { fork: true, parent: { full_name: "example/project" } }, "/repos/example/project": { fork: false }, + "/repos/example/project/git/matching-refs/heads/quest/": [ + { ref: "refs/heads/quest/m0/a" }, + { ref: "refs/heads/quest/m0/epic/b" }, + ], }; const github = new GitHub({ token: "test-token", now: () => 0, sleep: async () => undefined, warn: () => undefined, - fetch: async (url) => new Response(JSON.stringify(repos[url.pathname])), + fetch: async (url) => new Response(JSON.stringify(responses[url.pathname])), }); - expect(await pullRequestRepositories(github, { owner: "me", repo: "project" })).toEqual([ - { owner: "me", repo: "project" }, - { owner: "example", repo: "project" }, - ]); - expect(await pullRequestRepositories(github, { owner: "example", repo: "project" })).toEqual([ - { owner: "example", repo: "project" }, - ]); + const parent = { owner: "example", repo: "project" }; + expect(await forkParent(github, { owner: "me", repo: "project" })).toEqual(parent); + expect(await forkParent(github, parent)).toBeNull(); + expect(await githubBranches(github, parent)).toEqual(["quest/m0/a", "quest/m0/epic/b"]); }); From 322a3d12301d473a05ae7b00a29b9fa1f2e52851 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 12:43:37 -0700 Subject: [PATCH 6/6] fix(mv): encode parentheses and refuse rm of a directory without quests An unmatched parenthesis in a destination path ended the rewritten inline link early, so encode both as %28 and %29. rm -r on a directory holding only non-quest files would delete them all; refuse it instead. Co-Authored-By: Claude Opus 5.5 --- src/cli/move.ts | 1 + src/core/edit.ts | 10 ++++++++-- test/move.test.ts | 16 ++++++++++++---- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/cli/move.ts b/src/cli/move.ts index 67a549b2..150bbf83 100644 --- a/src/cli/move.ts +++ b/src/cli/move.ts @@ -93,6 +93,7 @@ export function removeQuest(root: string, arg: string, recursive: boolean): Repo const inside = epic === null ? [] : docs.filter((doc) => relocate(doc.path, epic, epic) !== null); const whole = epic !== null && (directory || inside.length > 1); const doomed = whole ? inside : docs.filter((doc) => doc.path === target); + if (doomed.length === 0) throw new Error(`${arg} holds no quest documents`); if (whole && doomed.length > 1 && !recursive) { throw new Error(`${arg} is an epic holding ${doomed.length} quest documents; pass -r to remove them all`); } diff --git a/src/core/edit.ts b/src/core/edit.ts index 2bc2ed00..8e9c0e3a 100644 --- a/src/core/edit.ts +++ b/src/core/edit.ts @@ -137,9 +137,15 @@ function drop(text: string, doc: Doc, dropping: (entry: Entry) => boolean): stri return out.join("\n"); } -/** A repository-relative path as a root-absolute Markdown destination's path. */ +/** + * A repository-relative path as a root-absolute Markdown destination's path. + * Parentheses are encoded too, since an unmatched one ends an inline link. + */ function encode(path: string): string { - return path.split("/").map(encodeURIComponent).join("/"); + return path + .split("/") + .map((name) => encodeURIComponent(name).replace(/\(/g, "%28").replace(/\)/g, "%29")) + .join("/"); } /** `text` with each replacement, keyed by offset, applied. */ diff --git a/test/move.test.ts b/test/move.test.ts index cfd3f7ab..750f796e 100644 --- a/test/move.test.ts +++ b/test/move.test.ts @@ -56,15 +56,15 @@ function staged(tree: Tree): string[] { } describe("move", () => { - test("rewrites inline and reference links, keeping fragments and encoding names", () => { + test("rewrites inline and reference links, keeping fragments and encoding names and parentheses", () => { const text = "# [S] A\n\n## Goal\n\nSee [b](/quest/b.md#plan) and [ref][b].\n\n" + "## Related\n\n- [/quest/b.md](/quest/b.md)\n\n[b]: /quest/b.md\n"; - const moved = move([{ path: "quest/a.md", text }], "quest/b.md", "quest/m1/new name.md"); + const moved = move([{ path: "quest/a.md", text }], "quest/b.md", "quest/m1/new (name.md"); expect(moved.missed).toEqual([]); expect(moved.texts.get("quest/a.md")).toBe( - "# [S] A\n\n## Goal\n\nSee [b](/quest/m1/new%20name.md#plan) and [ref][b].\n\n" + - "## Related\n\n- [/quest/b.md](/quest/m1/new%20name.md)\n\n[b]: /quest/m1/new%20name.md\n", + "# [S] A\n\n## Goal\n\nSee [b](/quest/m1/new%20%28name.md#plan) and [ref][b].\n\n" + + "## Related\n\n- [/quest/b.md](/quest/m1/new%20%28name.md)\n\n[b]: /quest/m1/new%20%28name.md\n", ); }); @@ -331,6 +331,14 @@ describe("quest rm", () => { } }); + test("refuses a directory that holds no quest documents", async () => { + const tree = committed(Tree.baseline().write("quest/m0/research/AGENTS.md", "Notes.\n")); + const out = await quest(tree, "rm", "-r", "quest/m0/research"); + expect(out).toEqual({ code: 1, stdout: "", stderr: "quest: quest/m0/research holds no quest documents\n" }); + expect(existsSync(join(tree.path, "quest/m0/research/AGENTS.md"))).toBe(true); + expect(staged(tree)).toEqual([]); + }); + test("reports a prose link it does not edit, and the check that fails on it", async () => { const tree = Tree.baseline().append( "quest/m0/epic/two.md",