From cce331f26ae148eed9da25783be55e6e82879c55 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 02:30:07 +0000 Subject: [PATCH 1/7] feat(upgrade): one-screen check, plan grouped by fix, and hack upgrade apply (HACK-2220) `hack upgrade check` printed 1,202 lines and a 266-step plan on a machine with 174 checkouts. The text report now fits one screen: - first line names the version, channel, and a source build or candidate launcher; - machine findings, project counts, and dead registry entries as one line with `hack projects prune --dry-run` (names only with --verbose/--all); - only projects with a finding, one line each, worktrees folded under their project, capped with a count; - a plan with one step per kind of change and the number of checkouts it covers, then what blocks, what could not be checked and why, and the next command. The JSON report (version 2) keeps the full detail: each plan step lists its checkouts in `targets`, with `count` and `applies`; project reports name their registered `project` and `project_dir`; `cli.build` says whether a source build ran. `hack upgrade apply [--only ] [--project ] [--dry-run]` runs the plan's apply steps for every checkout they cover: refreshing outdated `.hack/.gitignore` files and removing stale standalone agent files through the plugin migration (backed up to its own manifest, refused unless the hack plugin is verified). It works from a fresh check, skips checkouts with a blocker or an incomplete check, and keeps an upgrade snapshot before the first change. A journal beside the snapshot manifest records the snapshot's coverage and gaps and each checkout's outcome, rewritten after every target. The after state of every written file is recorded, so rollback needs no --overwrite. It prints the rollback commands and then rechecks. Co-Authored-By: Claude Opus 5.5 --- docs/cli.md | 2 +- docs/reference/cli.md | 39 +- docs/versions/v5/guides/upgrade-from-v4.md | 35 +- src/commands/upgrade.ts | 343 ++++++++---- src/lib/upgrade-apply.ts | 538 +++++++++++++++++++ src/lib/upgrade-check-project.ts | 4 + src/lib/upgrade-check-render.ts | 277 ++++++++++ src/lib/upgrade-check.ts | 150 +----- src/lib/upgrade-plan.ts | 326 ++++++++++++ src/lib/upgrade-snapshot.ts | 62 ++- tests/upgrade-apply.test.ts | 578 +++++++++++++++++++++ tests/upgrade-check.test.ts | 3 +- 12 files changed, 2132 insertions(+), 225 deletions(-) create mode 100644 src/lib/upgrade-apply.ts create mode 100644 src/lib/upgrade-check-render.ts create mode 100644 src/lib/upgrade-plan.ts create mode 100644 tests/upgrade-apply.test.ts diff --git a/docs/cli.md b/docs/cli.md index 60667e2f7..a4bfef917 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -22,7 +22,7 @@ terminal). - `hack exec -- ` — command in an already-running service container - `hack session` — persistent project workspaces (tmux-first) - `hack doctor` / `hack doctor --fix` — validate and repair local setup -- `hack upgrade check` — read-only upgrade check of this machine and every registered project, ending in an ordered plan and the next command; `hack upgrade snapshot` and `hack upgrade rollback --rollback-manifest ` keep and restore the files the upgrade changes +- `hack upgrade check` — read-only upgrade check of this machine and every registered project: a one-screen summary, the projects with findings (worktrees folded; `--verbose` for every checkout), and a plan with one step per kind of change, then the next command; `hack upgrade apply [--only ] [--project ] [--dry-run]` runs the plan's apply steps (`.hack/.gitignore` refresh, stale agent files) for every checkout after keeping a snapshot and a journal; `hack upgrade snapshot` and `hack upgrade rollback --rollback-manifest ` keep and restore the files the upgrade changes - `hack daemon` — optional local daemon for faster JSON project status; `hack daemon install` starts it at login (launchd on macOS, a systemd user unit on Linux). On Linux without linger it still stops at your last logout and does not start at boot; `loginctl enable-linger` changes that Candidate `hack-next doctor` and `hack-v5 doctor` report a missing hackd as optional diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 1168f63ee..522da4a56 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -1868,7 +1868,7 @@ Notes: ## `hack upgrade` -Guided upgrade: check, snapshot and roll back +Guided upgrade: check, apply, snapshot and roll back ### Usage @@ -1881,6 +1881,7 @@ hack upgrade [options] | Command | Summary | | --- | --- | | `hack upgrade check` | Check this machine and every registered project for the upgrade (read-only) | +| `hack upgrade apply` | Run the plan's apply steps for every checkout (snapshot and journal first) | | `hack upgrade snapshot` | Keep a private copy of the files the upgrade changes | | `hack upgrade rollback` | Put back the files kept by an upgrade snapshot | @@ -1905,13 +1906,45 @@ hack upgrade check [options] Reads the installed versions, the daemon's service, the projects registry (dead entries apart from unreadable ones), Hack's global services, proxy ports and DNS, the global secret store, and for each live project its env format and key, tracked files, ignore rules, agent integrations -and (Apple Silicon) native preflight. Ends with an ordered plan and the next command. Changes -nothing and prints no value. Exits 1 when something blocks the upgrade, naming it. +and (Apple Silicon) native preflight. Prints a one-screen summary, the projects with findings +(worktrees folded under their project) and a plan with one step per kind of change, then the +next command. Changes nothing and prints no value. Exits 1 when something blocks the upgrade. ### Options | Option | Description | | --- | --- | +| `--verbose` | Also list every checkout's findings and the dead registry entries (--json always has everything) | +| `--all` | Same as --verbose | +| `--json` | Output JSON (machine-readable) | +| `--no-interactive` | Never prompt: apply documented defaults or fail with E_INTERACTIVE_REQUIRED (also via HACK_NO_INTERACTIVE=1) | +| `--help, -h` | Show help | +| `--version, -v` | Show version | + +## `hack upgrade apply` + +Run the plan's apply steps for every checkout (snapshot and journal first) + +### Usage + +```bash +hack upgrade apply [options] +``` + +Checks first, then runs the plan steps marked (apply) for every checkout they cover: refreshing +outdated .hack/.gitignore files and removing stale standalone agent files the hack plugin replaces +(each removal backed up by the plugin migration). Before changing anything it keeps an upgrade +snapshot and writes a journal beside it with the snapshot's coverage and gaps and each checkout's +outcome. Checkouts with a blocker or a check that could not run are skipped. Prints the rollback +command. --dry-run lists what would change. Exits 1 when anything selected was not done. + +### Options + +| Option | Description | +| --- | --- | +| `--only ` | Run only this plan step: ignore-rules or agent-integrations (comma-separated for both) | +| `--project ` | Target a registered project by name (from ~/.hack/projects.json) | +| `--dry-run` | List what would change and change nothing | | `--json` | Output JSON (machine-readable) | | `--no-interactive` | Never prompt: apply documented defaults or fail with E_INTERACTIVE_REQUIRED (also via HACK_NO_INTERACTIVE=1) | | `--help, -h` | Show help | diff --git a/docs/versions/v5/guides/upgrade-from-v4.md b/docs/versions/v5/guides/upgrade-from-v4.md index 837e8981f..6821a4b61 100644 --- a/docs/versions/v5/guides/upgrade-from-v4.md +++ b/docs/versions/v5/guides/upgrade-from-v4.md @@ -83,10 +83,19 @@ Silicon, the native preflight. It changes nothing and prints no secret value. - **Dead registry entries** (the checkout or its config is gone) are counted apart from live projects that cannot be read. Dead entries only need `hack projects prune`; unreadable ones need repair. -- It ends with an ordered plan and the next command. A step either has a - command or is marked manual, with the action to take; manual text is never - printed as a command. Commands run the CLI you ran the check with (the - `hack-next` launcher during the prerelease). +- The text report fits one screen: the build that ran (a source build or the + candidate launcher is named on the first line), the machine, project counts, + the projects that have a finding (worktrees folded under their project), + then the plan. `--verbose` (or `--all`) adds every checkout's findings and + the dead entries; `--json` always has the full detail. +- The plan has one step per kind of change, with the number of checkouts it + covers, then the next command. A step either has a command or is marked + manual, with the action to take; manual text is never printed as a command. + Commands run the CLI you ran the check with (the `hack-next` launcher during + the prerelease). +- Steps marked `(apply)` are safe hygiene that `hack upgrade apply` runs for + every checkout they cover: refreshing outdated `.hack/.gitignore` files and + removing stale standalone agent files that the hack plugin replaces. - It exits 1 when something blocks the upgrade, naming the blockers. Examples are a global secret store whose key is gone or refused while a project reads it, a tracked secret key, an env layer conflict, or a live project @@ -96,7 +105,23 @@ Silicon, the native preflight. It changes nothing and prints no secret value. may hold the key; the check never asks the keychain, because that can prompt. -Before you follow the plan, keep a private copy of the files it changes: +To run the apply steps: + +```sh +hack upgrade apply --dry-run # what would change; changes nothing +hack upgrade apply # every apply step, every checkout +hack upgrade apply --only ignore-rules --project my-app +``` + +Apply checks first and skips any checkout with a blocker or a check that could +not run. Before it changes anything it keeps an upgrade snapshot (below) and +writes a journal beside the snapshot manifest: the snapshot's coverage (whether +it is complete, and its gaps), and each checkout's outcome. Agent files are +removed only when the hack plugin that replaces them is verified, and each +removal is backed up by `hack setup migrate`'s manifest. Apply prints the +rollback commands and then rechecks. + +Before you follow the rest of the plan, keep a private copy of the files it changes: ```sh hack upgrade snapshot diff --git a/src/commands/upgrade.ts b/src/commands/upgrade.ts index 9e4007469..98ecfaa56 100644 --- a/src/commands/upgrade.ts +++ b/src/commands/upgrade.ts @@ -7,18 +7,22 @@ import { defineOption, withHandler, } from "../cli/command.ts"; -import { optJson } from "../cli/options.ts"; +import { optJson, optProject } from "../cli/options.ts"; import { emitCliResult, okResult } from "../lib/cli-result.ts"; import { resolveUserHomeDir } from "../lib/home-dir.ts"; import { escapeTerminalControls } from "../lib/terminal-text.ts"; +import { + runUpgradeApply, + type UpgradeApplyResult, +} from "../lib/upgrade-apply.ts"; import { renderUpgradeCommand, runUpgradeCheck, type UpgradeCheckReport, - type UpgradeFinding, upgradeCommandPrefix, } from "../lib/upgrade-check.ts"; import { defaultUpgradeCheckProbes } from "../lib/upgrade-check-probes.ts"; +import { renderUpgradeCheck } from "../lib/upgrade-check-render.ts"; import { createUpgradeSnapshot, listUpgradeSnapshots, @@ -33,7 +37,22 @@ import { collectUpgradeSnapshotTargets } from "../lib/upgrade-snapshot-targets.t * later apply steps build on. */ -const checkOptions = [optJson] as const; +const optVerbose = defineOption({ + name: "verbose", + type: "boolean", + long: "--verbose", + description: + "Also list every checkout's findings and the dead registry entries (--json always has everything)", +} as const); + +const optAll = defineOption({ + name: "all", + type: "boolean", + long: "--all", + description: "Same as --verbose", +} as const); + +const checkOptions = [optVerbose, optAll, optJson] as const; const checkSpec = defineCommand({ name: "check", @@ -44,8 +63,9 @@ const checkSpec = defineCommand({ "Reads the installed versions, the daemon's service, the projects registry (dead entries apart", "from unreadable ones), Hack's global services, proxy ports and DNS, the global secret store, and", "for each live project its env format and key, tracked files, ignore rules, agent integrations", - "and (Apple Silicon) native preflight. Ends with an ordered plan and the next command. Changes", - "nothing and prints no value. Exits 1 when something blocks the upgrade, naming it.", + "and (Apple Silicon) native preflight. Prints a one-screen summary, the projects with findings", + "(worktrees folded under their project) and a plan with one step per kind of change, then the", + "next command. Changes nothing and prints no value. Exits 1 when something blocks the upgrade.", ].join("\n"), options: checkOptions, positionals: [], @@ -102,14 +122,51 @@ const rollbackSpec = defineCommand({ subcommands: [], } as const); +const optOnly = defineOption({ + name: "only", + type: "string", + long: "--only", + valueHint: "", + description: + "Run only this plan step: ignore-rules or agent-integrations (comma-separated for both)", +} as const); + +const optDryRun = defineOption({ + name: "dryRun", + type: "boolean", + long: "--dry-run", + description: "List what would change and change nothing", +} as const); + +const applyOptions = [optOnly, optProject, optDryRun, optJson] as const; + +const applySpec = defineCommand({ + name: "apply", + summary: + "Run the plan's apply steps for every checkout (snapshot and journal first)", + group: "Diagnostics", + description: [ + "Checks first, then runs the plan steps marked (apply) for every checkout they cover: refreshing", + "outdated .hack/.gitignore files and removing stale standalone agent files the hack plugin replaces", + "(each removal backed up by the plugin migration). Before changing anything it keeps an upgrade", + "snapshot and writes a journal beside it with the snapshot's coverage and gaps and each checkout's", + "outcome. Checkouts with a blocker or a check that could not run are skipped. Prints the rollback", + "command. --dry-run lists what would change. Exits 1 when anything selected was not done.", + ].join("\n"), + options: applyOptions, + positionals: [], + subcommands: [], +} as const); + export const upgradeCommand = defineCommand({ name: "upgrade", - summary: "Guided upgrade: check, snapshot and roll back", + summary: "Guided upgrade: check, apply, snapshot and roll back", group: "Diagnostics", options: [], positionals: [], subcommands: [ withHandler(checkSpec, handleUpgradeCheck), + withHandler(applySpec, handleUpgradeApply), withHandler(snapshotSpec, handleUpgradeSnapshot), withHandler(rollbackSpec, handleUpgradeRollback), ], @@ -134,99 +191,31 @@ async function handleUpgradeCheck({ readonly ctx: CliContext; readonly args: CommandArgs; }): Promise { - const report = await runUpgradeCheck({ - cliVersion: ctx.cli.version, - homeDir: resolveUserHomeDir(), - probes: defaultUpgradeCheckProbes(), - }); + const report = await check(ctx); if (args.options.json === true) { emitCliResult({ result: okResult({ data: report }) }); } else { - process.stdout.write(renderUpgradeCheck(report)); - } - return report.ready ? 0 : 1; -} - -const MARK: Readonly> = { - ok: "ok ", - info: "info ", - warn: "warn ", - incomplete: "UNCHK", - blocker: "BLOCK", -}; - -/** Plain-text report: every line escaped, no values. */ -export function renderUpgradeCheck(report: UpgradeCheckReport): string { - const lines: string[] = []; - const line = (text: string) => lines.push(escapeTerminalControls(text)); - line( - `hack ${report.cli.version} (${report.cli.channel}${report.cli.candidate_run ? ", candidate launcher" : ""})` - ); - line(""); - line("Machine"); - for (const finding of report.machine) { - line(` ${MARK[finding.status]} ${finding.summary}`); - } - if (report.registry.missing.length > 0) { - line( - ` Dead entries: ${report.registry.missing.map((entry) => entry.name).join(", ")}` + process.stdout.write( + renderUpgradeCheck(report, { + verbose: args.options.verbose === true || args.options.all === true, + }) ); } - for (const project of report.projects) { - line(""); - line(`Project ${project.name} (${project.root})`); - for (const finding of project.findings) { - line(` ${MARK[finding.status]} ${finding.summary}`); - } - } - line(""); - for (const text of renderPlan(report)) { - line(text); - } - return `${lines.join("\n")}\n`; + return report.ready ? 0 : 1; } -function renderPlan(report: UpgradeCheckReport): string[] { - if (report.plan.length === 0) { - return report.ready - ? ["Nothing to do: this machine and its projects are ready."] - : ["No step to suggest, but the check is not complete; see above."]; - } - const lines = ["Plan", ...report.plan.flatMap(renderStep), ""]; - if (report.next_command !== null) { - lines.push(`Next: ${report.next_command}`); - } else if (report.next_action !== null) { - lines.push(`Next (manual): ${report.next_action}`); - } - if (report.blockers.length > 0) { - lines.push( - "", - `Blocked by: ${report.blockers.map((blocker) => `${blocker.scope} ${blocker.id}`).join(", ")}` - ); - } - if (report.incomplete.length > 0) { - lines.push( - "", - `Not fully checked: ${report.incomplete.map((item) => `${item.scope} ${item.id}`).join(", ")}` - ); - } - return lines; +async function check(ctx: CliContext): Promise { + return await runUpgradeCheck({ + cliVersion: ctx.cli.version, + homeDir: resolveUserHomeDir(), + probes: defaultUpgradeCheckProbes(), + build: runningBuild(), + }); } -function renderStep(step: UpgradeCheckReport["plan"][number]): string[] { - const tag = - { - blocker: "[blocker] ", - incomplete: "[not checked] ", - }[step.status as string] ?? ""; - const scope = step.scope === "machine" ? "" : ` (${step.scope})`; - const head = - step.command === null - ? ` ${step.order}. ${tag}manual: ${step.action ?? step.why}${scope}` - : ` ${step.order}. ${tag}${step.command}${scope}`; - return step.command !== null && step.action !== null - ? [head, ` ${step.action}`] - : [head]; +/** A compiled binary runs from Bun's embedded filesystem; anything else is a source build. */ +function runningBuild(): "release" | "source" { + return Bun.main.startsWith("/$bunfs/") ? "release" : "source"; } // --------------------------------------------------------------------------- @@ -403,3 +392,177 @@ function renderRollback(result: RestoreUpgradeSnapshotResult): string { ); return `${lines.map((text) => escapeTerminalControls(text)).join("\n")}\n`; } + +// --------------------------------------------------------------------------- +// apply +// --------------------------------------------------------------------------- + +async function handleUpgradeApply({ + ctx, + args, +}: { + readonly ctx: CliContext; + readonly args: CommandArgs; +}): Promise { + const json = args.options.json === true; + const only = (args.options.only ?? "") + .split(",") + .map((id) => id.trim()) + .filter((id) => id.length > 0); + if (args.options.only !== undefined && only.length === 0) { + throw new CliUsageError("--only needs ignore-rules or agent-integrations."); + } + const project = args.options.project?.trim(); + if (args.options.project !== undefined && !project) { + throw new CliUsageError("--project needs a registered project name."); + } + const dryRun = args.options.dryRun === true; + const report = await check(ctx); + const result = await runUpgradeApply({ + report, + only, + project, + dryRun, + home: resolveUserHomeDir(), + collect: collectUpgradeSnapshotTargets, + }); + const ran = result.status === "applied" || result.status === "partial"; + // Fresh evidence: what the check finds after the run. + const after = ran ? await check(ctx) : null; + const rollback = ran + ? command([ + "upgrade", + "rollback", + "--rollback-manifest", + result.coverage.manifest_path, + ]) + : null; + if (json) { + emitCliResult({ + result: okResult({ data: applyJson({ result, after, rollback }) }), + }); + } else { + process.stdout.write(renderApply({ result, after, rollback })); + } + return result.status === "refused" || result.status === "partial" ? 1 : 0; +} + +function applyJson(opts: { + readonly result: UpgradeApplyResult; + readonly after: UpgradeCheckReport | null; + readonly rollback: string | null; +}) { + const { result, after } = opts; + if (result.status === "refused") { + return result; + } + return { + ...result, + rollback_command: opts.rollback, + after: + after === null + ? null + : { + ready: after.ready, + plan: after.plan.map((step) => ({ + id: step.id, + status: step.status, + count: step.count, + applies: step.applies, + })), + next_command: after.next_command, + next_action: after.next_action, + }, + }; +} + +const APPLY_LABEL: Readonly> = { + "ignore-rules": ".hack/.gitignore", + "agent-integrations": "agent files", +}; + +/** Text result: outcomes counted per fix and status, a few names, closed messages. */ +export function renderApply(opts: { + readonly result: UpgradeApplyResult; + readonly after: UpgradeCheckReport | null; + readonly rollback: string | null; +}): string { + const { result } = opts; + const lines = + result.status === "refused" + ? [result.message] + : [ + ...outcomeLines(result), + ...recoveryLines({ result, rollback: opts.rollback }), + ...afterLines(opts.after), + ]; + return `${lines.map((text) => escapeTerminalControls(text)).join("\n")}\n`; +} + +function outcomeLines( + result: Exclude +): string[] { + if (result.targets.length === 0) { + return ["Nothing for hack upgrade apply to do."]; + } + const groups = new Map(); + for (const target of result.targets) { + const key = `${target.fix}\u0000${target.status}\u0000${target.message ?? ""}`; + groups.set(key, [...(groups.get(key) ?? []), target]); + } + const verb = result.status === "dry-run" ? "would change" : "changed"; + const lines: string[] = []; + for (const targets of groups.values()) { + const first = targets[0] as (typeof targets)[number]; + const names = targets.slice(0, 3).map((t) => t.scope); + const more = targets.length > 3 ? ` and ${targets.length - 3} more` : ""; + const status = first.status === "planned" ? verb : first.status; + lines.push( + `${APPLY_LABEL[first.fix] ?? first.fix}: ${status} in ${targets.length} (${names.join(", ")}${more})` + ); + if (first.message !== null) { + lines.push(` ${first.message}`); + } + } + return lines; +} + +function recoveryLines(opts: { + readonly result: Exclude; + readonly rollback: string | null; +}): string[] { + const { result } = opts; + if (result.status !== "applied" && result.status !== "partial") { + return []; + } + const { coverage } = result; + const gaps = coverage.complete + ? "" + : `; not a complete recovery point (${coverage.gaps.length} gap(s), listed in the journal)`; + return [ + "", + `Snapshot ${coverage.snapshot_id}: ${coverage.files} file(s)${gaps}.`, + `Journal: ${result.journal_path}`, + ...(opts.rollback === null + ? [] + : [`To put the files back: ${opts.rollback}`]), + ...result.agent_backups.map( + (manifest) => + `To put removed agent files back: ${command(["setup", "migrate", "--rollback-manifest", manifest])}` + ), + ]; +} + +function afterLines(after: UpgradeCheckReport | null): string[] { + if (after === null) { + return []; + } + const remaining = after.plan.filter((step) => step.id !== "snapshot").length; + return [ + "", + after.ready + ? "Check after apply: ready." + : `Check after apply: ${remaining} step(s) remain.`, + ...(after.next_command === null ? [] : [`Next: ${after.next_command}`]), + ]; +} diff --git a/src/lib/upgrade-apply.ts b/src/lib/upgrade-apply.ts new file mode 100644 index 000000000..f6d113020 --- /dev/null +++ b/src/lib/upgrade-apply.ts @@ -0,0 +1,538 @@ +import { lstat } from "node:fs/promises"; +import { join, normalize, resolve } from "node:path"; + +import { + applyPluginMigration, + type PluginMigrationApplyResult, +} from "../agents/plugin-migration.ts"; +import { MIGRATION_CLIENTS } from "../agents/plugin-replacement.ts"; +import { resolveGlobalHackDir } from "./config-paths.ts"; +import { ensureHackDirGitignore } from "./project-env-config.ts"; +import type { UpgradeCheckReport } from "./upgrade-check.ts"; +import { + isUpgradeApplyFix, + UPGRADE_APPLY_FIXES, + type UpgradeApplyFix, +} from "./upgrade-plan.ts"; +import { + createUpgradeSnapshot, + recordUpgradeSnapshotAfter, + type UpgradeSnapshot, + type UpgradeSnapshotTarget, + writeUpgradeSnapshotJournal, +} from "./upgrade-snapshot.ts"; + +/** + * `hack upgrade apply` (HACK-2188, HACK-2220): runs the plan's apply steps + * ({@link UPGRADE_APPLY_FIXES}) for every checkout they cover. + * + * Contract: + * - Works from a fresh `hack upgrade check` report; never guesses. A + * checkout with a blocker or an incomplete check is skipped (named, with + * why); `--project` and `--only` narrow the work and an unknown name is + * refused before anything is written. + * - `dryRun` writes nothing and lists what would run. + * - Otherwise it first keeps an upgrade snapshot of the files the upgrade + * changes (the `hack upgrade snapshot` set, plus every file this run + * writes). If the snapshot is refused, nothing is changed. A file this + * run writes is changed only when the snapshot holds it, and its after + * state is recorded right after the write, so `hack upgrade rollback` + * can tell the step's output from a later edit. + * - A journal beside the snapshot manifest records the run: the snapshot's + * coverage (`complete`, `gaps`, file count), each target's outcome and + * any backup manifest it made. It is rewritten atomically before the + * first change and after every target, so an interrupted run says how + * far it got. Paths, ids, statuses, counts and closed messages only: + * never a value. + * - Stale agent files are removed only by the plugin migration, which backs + * up each file to its own manifest first and refuses when the hack plugin + * that replaces them is not verified. Machine-wide agent files (under + * HOME) are migrated only without `--project`. + * - Nothing is deleted that current resolution reads: `.hack/.gitignore` is + * merged (never shrunk), and the migration removes only Hack-owned + * standalone files the verified plugin replaces. + */ + +export const UPGRADE_APPLY_JOURNAL_VERSION = 1; +export const UPGRADE_APPLY_JOURNAL_KIND = "hack-upgrade-apply"; + +export type UpgradeApplyTargetStatus = + /** Would run (dry run). */ + | "planned" + /** Changed. */ + | "applied" + /** Already current; nothing written. */ + | "unchanged" + /** Not run: the checkout has a blocker or an incomplete check, or it is out of scope. */ + | "skipped" + /** Not run: a safety condition refused it (plugin not verified, a link, a read-only file, not in the snapshot). */ + | "refused" + /** Started and failed. */ + | "failed"; + +export type UpgradeApplyTarget = { + readonly fix: UpgradeApplyFix; + /** The checkout's name, or `machine` for machine-wide agent files. */ + readonly scope: string; + readonly project: string | null; + readonly root: string; + readonly status: UpgradeApplyTargetStatus; + /** A closed explanation; never a value. */ + readonly message: string | null; + /** The plugin migration's backup manifest, when it removed files. */ + readonly backup_manifest: string | null; +}; + +export type UpgradeApplyCoverage = { + readonly snapshot_id: string; + readonly manifest_path: string; + readonly files: number; + /** True when the snapshot is a complete recovery point for the files the upgrade changes. */ + readonly complete: boolean; + readonly gaps: readonly { readonly name: string; readonly reason: string }[]; +}; + +export type UpgradeApplyJournal = { + readonly version: typeof UPGRADE_APPLY_JOURNAL_VERSION; + readonly kind: typeof UPGRADE_APPLY_JOURNAL_KIND; + readonly started_at: string; + readonly updated_at: string; + readonly state: "applying" | "applied" | "failed"; + readonly cli_version: string; + readonly filters: { + readonly only: readonly UpgradeApplyFix[]; + readonly project: string | null; + }; + readonly snapshot: UpgradeApplyCoverage; + readonly targets: readonly UpgradeApplyTarget[]; +}; + +export type UpgradeApplyResult = + | { + /** Refused before anything was written. */ + readonly status: "refused"; + readonly reason: "unknown-project" | "unknown-fix" | "snapshot"; + readonly message: string; + } + | { + readonly status: "nothing-to-do" | "dry-run"; + readonly targets: readonly UpgradeApplyTarget[]; + } + | { + /** `applied`: every selected target is applied or unchanged. */ + readonly status: "applied" | "partial"; + readonly targets: readonly UpgradeApplyTarget[]; + readonly coverage: UpgradeApplyCoverage; + readonly journal_path: string; + readonly agent_backups: readonly string[]; + }; + +/** Collected snapshot targets, as {@link collectUpgradeSnapshotTargets} returns them. */ +export type UpgradeApplySnapshotSet = { + readonly targets: readonly UpgradeSnapshotTarget[]; + readonly gaps: readonly { readonly name: string; readonly reason: string }[]; +}; + +type Planned = { + readonly fix: UpgradeApplyFix; + readonly scope: string; + readonly project: string | null; + readonly root: string; + /** The checkout's Hack directory; null for machine-wide agent files. */ + readonly projectDir: string | null; + readonly skip: string | null; +}; + +const ERROR_CODE = /^[A-Z0-9_]{1,40}$/; +const SKIP_BLOCKED = + "Not run: this checkout has a blocker or a check that could not be performed; fix that first (hack upgrade check lists it)."; +const SKIP_MACHINE_WITH_PROJECT = + "Not run with --project: these agent files are under HOME and serve every project; run hack upgrade apply --only agent-integrations without --project."; +const NOT_IN_SNAPSHOT = + "Not changed: the file is not in this run's snapshot, so a rollback could not restore it."; + +/** + * Runs (or with `dryRun` lists) the apply steps of `report`. `collect` + * returns the upgrade snapshot set (production: + * `collectUpgradeSnapshotTargets`). + */ +export async function runUpgradeApply(input: { + readonly report: UpgradeCheckReport; + readonly only?: readonly string[]; + readonly project?: string; + readonly dryRun: boolean; + readonly home: string; + readonly collect: () => Promise; + /** Test seams. */ + readonly snapshotRoot?: string; + readonly agentBackupDir?: string; + readonly now?: () => Date; +}): Promise { + const only = input.only ?? []; + const unknownFix = only.find((id) => !isUpgradeApplyFix(id)); + if (unknownFix !== undefined) { + return { + status: "refused", + reason: "unknown-fix", + message: `--only takes ${UPGRADE_APPLY_FIXES.join(" or ")}; nothing was changed.`, + }; + } + const known = new Set(input.report.projects.map((p) => p.project)); + if (input.project !== undefined && !known.has(input.project)) { + return { + status: "refused", + reason: "unknown-project", + message: + "No live registered project has that name; nothing was changed. hack upgrade check lists them.", + }; + } + const planned = planTargets({ + report: input.report, + only: only.filter(isUpgradeApplyFix), + project: input.project ?? null, + home: input.home, + }); + const runnable = planned.filter((target) => target.skip === null); + const skipped = planned + .filter((target) => target.skip !== null) + .map((target) => outcome(target, "skipped", target.skip)); + if (input.dryRun) { + return { + status: + runnable.length === 0 && skipped.length === 0 + ? "nothing-to-do" + : "dry-run", + targets: [ + ...runnable.map((target) => outcome(target, "planned", null)), + ...skipped, + ], + }; + } + if (runnable.length === 0) { + return { status: "nothing-to-do", targets: skipped }; + } + + // The snapshot comes first: nothing is written unless it is kept. + const collected = await input.collect(); + const targets = await snapshotTargets({ runnable, collected }); + const created = await createUpgradeSnapshot({ + purpose: "hack upgrade apply", + targets, + root: input.snapshotRoot, + now: input.now?.(), + }); + if (created.status !== "created") { + return { + status: "refused", + reason: "snapshot", + message: + created.reason === "root" + ? `The snapshot directory ${created.root} cannot be used (${created.rootReason}); nothing was changed.` + : `These files cannot be snapshotted, so nothing was changed: ${created.targets.map((t) => `${t.label} (${t.reason})`).join(", ")}.`, + }; + } + const { snapshot } = created; + const coverage: UpgradeApplyCoverage = { + snapshot_id: snapshot.id, + manifest_path: snapshot.manifestPath, + files: snapshot.manifest.entries.length, + complete: collected.gaps.length === 0, + gaps: collected.gaps, + }; + const now = () => (input.now?.() ?? new Date()).toISOString(); + const startedAt = now(); + const results: UpgradeApplyTarget[] = [...skipped]; + const journal = ( + state: UpgradeApplyJournal["state"] + ): UpgradeApplyJournal => ({ + version: UPGRADE_APPLY_JOURNAL_VERSION, + kind: UPGRADE_APPLY_JOURNAL_KIND, + started_at: startedAt, + updated_at: now(), + state, + cli_version: input.report.cli.version, + filters: { + only: only.filter(isUpgradeApplyFix), + project: input.project ?? null, + }, + snapshot: coverage, + targets: [ + ...results, + ...runnable + .slice(results.length - skipped.length) + .map((target) => outcome(target, "planned", null)), + ], + }); + // Commit point: the journal names the snapshot before the first change. + const journalPath = await writeUpgradeSnapshotJournal({ + snapshot, + journal: journal("applying"), + }); + const inSnapshot = new Set(snapshot.manifest.entries.map((e) => e.path)); + for (const target of runnable) { + results.push( + await applyTarget({ + target, + snapshot, + inSnapshot, + home: input.home, + agentBackupDir: + input.agentBackupDir ?? + join(resolveGlobalHackDir(), "agent-migrations"), + }) + ); + await writeUpgradeSnapshotJournal({ + snapshot, + journal: journal("applying"), + }); + } + const done = results.every( + (r) => r.status === "applied" || r.status === "unchanged" + ); + await writeUpgradeSnapshotJournal({ + snapshot, + journal: journal(done ? "applied" : "failed"), + }); + return { + status: done ? "applied" : "partial", + targets: results, + coverage, + journal_path: journalPath, + agent_backups: results.flatMap((r) => + r.backup_manifest === null ? [] : [r.backup_manifest] + ), + }; +} + +function planTargets(opts: { + readonly report: UpgradeCheckReport; + readonly only: readonly UpgradeApplyFix[]; + readonly project: string | null; + readonly home: string; +}): Planned[] { + const wanted = new Set( + opts.only.length > 0 ? opts.only : UPGRADE_APPLY_FIXES + ); + const unsafe = unsafeCheckouts(opts.report); + const byName = new Map(opts.report.projects.map((p) => [p.name, p])); + const out: Planned[] = []; + for (const fix of UPGRADE_APPLY_FIXES) { + const step = wanted.has(fix) + ? opts.report.plan.find((c) => c.id === fix && c.applies) + : undefined; + if (step === undefined) { + continue; + } + if (fix === "agent-integrations") { + // Files under HOME make every checkout stale; they are migrated once. + out.push({ + fix, + scope: "machine", + project: null, + root: opts.home, + projectDir: null, + skip: opts.project === null ? null : SKIP_MACHINE_WITH_PROJECT, + }); + } + out.push( + ...step.targets + .filter((t) => opts.project === null || t.project === opts.project) + .map( + (target): Planned => ({ + fix, + scope: target.scope, + project: target.project, + root: target.root, + projectDir: byName.get(target.scope)?.project_dir ?? null, + skip: unsafe.has(target.scope) ? SKIP_BLOCKED : null, + }) + ) + ); + } + return out; +} + +/** Checkouts with a blocker or a check that could not be performed. */ +function unsafeCheckouts(report: UpgradeCheckReport): ReadonlySet { + return new Set( + report.projects + .filter((project) => + project.findings.some( + (f) => f.status === "blocker" || f.status === "incomplete" + ) + ) + .map((project) => project.name) + ); +} + +async function applyTarget(opts: { + readonly target: Planned; + readonly snapshot: UpgradeSnapshot; + readonly inSnapshot: ReadonlySet; + readonly home: string; + readonly agentBackupDir: string; +}): Promise { + const { target } = opts; + if (target.fix === "ignore-rules") { + if (target.projectDir === null) { + return outcome( + target, + "refused", + "Not changed: the checkout's Hack directory is unknown." + ); + } + const path = gitignorePath(target.projectDir); + if (!opts.inSnapshot.has(path)) { + return outcome(target, "refused", NOT_IN_SNAPSHOT); + } + let result: Awaited> | null = + null; + let failure: unknown = null; + try { + result = await ensureHackDirGitignore({ projectDir: target.projectDir }); + } catch (error) { + failure = error; + } + // Whatever is there now is this step's output, even after a failure. + await recordUpgradeSnapshotAfter({ + manifestPath: opts.snapshot.manifestPath, + paths: [path], + }); + if (result === null) { + return outcome(target, "failed", closedError(failure)); + } + if (result.readOnly) { + return outcome( + target, + "refused", + "Not changed: .hack/.gitignore is read-only; make it writable or update it yourself." + ); + } + return outcome(target, result.changed ? "applied" : "unchanged", null); + } + let migrated: PluginMigrationApplyResult; + try { + migrated = await applyPluginMigration({ + scopes: [ + target.projectDir === null + ? { scope: "global", root: opts.home } + : { scope: "project", root: target.root }, + ], + home: opts.home, + clients: MIGRATION_CLIENTS, + skipPluginCheck: [], + backupDir: opts.agentBackupDir, + }); + } catch (error) { + return outcome(target, "failed", closedError(error)); + } + return agentOutcome({ target, migrated }); +} + +function agentOutcome(opts: { + readonly target: Planned; + readonly migrated: PluginMigrationApplyResult; +}): UpgradeApplyTarget { + const { target, migrated } = opts; + const preview = + target.projectDir === null + ? "hack setup migrate --global" + : "hack setup migrate --path "; + if (migrated.status === "nothing-to-migrate") { + return outcome( + target, + "unchanged", + "No standalone Hack agent file to remove here; a stale AGENTS.md or CLAUDE.md section is refreshed by hack setup, not by apply." + ); + } + if (migrated.status === "refused") { + const why = { + blocked: + "the hack plugin that replaces these files is not verified for a client", + "scope-error": "the scope could not be resolved", + "backup-failed": "a file could not be read for its backup", + }[migrated.reason]; + return outcome( + target, + "refused", + `Not changed: ${why}. ${preview} shows the details.` + ); + } + const failed = migrated.results.some( + (entry) => entry.status === "error" || entry.status === "changed" + ); + return { + ...outcome( + target, + failed ? "failed" : "applied", + failed + ? `Some files were not removed; ${preview} shows them. Removed files are backed up in the manifest.` + : null + ), + backup_manifest: migrated.manifestPath, + }; +} + +function outcome( + target: Planned, + status: UpgradeApplyTargetStatus, + message: string | null +): UpgradeApplyTarget { + return { + fix: target.fix, + scope: target.scope, + project: target.project, + root: target.root, + status, + message, + backup_manifest: null, + }; +} + +/** + * The snapshot's targets: the upgrade set plus every file this run writes. + * A write-set path the collection did not keep is added only when it is a + * regular file or absent: a link there would refuse the whole snapshot, so + * its checkout is refused alone (not in the snapshot) instead. + */ +async function snapshotTargets(opts: { + readonly runnable: readonly Planned[]; + readonly collected: UpgradeApplySnapshotSet; +}): Promise { + const collectedPaths = new Set( + opts.collected.targets.map((t) => normalize(t.path)) + ); + const extra: UpgradeSnapshotTarget[] = []; + for (const target of opts.runnable) { + if (target.fix !== "ignore-rules" || target.projectDir === null) { + continue; + } + const path = gitignorePath(target.projectDir); + if (!collectedPaths.has(path) && (await snapshotable(path))) { + extra.push({ path, label: `project ${target.scope}: .hack/.gitignore` }); + } + } + return [...opts.collected.targets, ...extra]; +} + +/** A regular file or an absent path (not a link or other entry). */ +async function snapshotable(path: string): Promise { + try { + return (await lstat(path)).isFile(); + } catch (error) { + return (error as { code?: unknown }).code === "ENOENT"; + } +} + +function gitignorePath(projectDir: string): string { + return normalize(resolve(projectDir, ".gitignore")); +} + +/** A closed message for a failure: the error's code, never its text. */ +function closedError(error: unknown): string { + const code = + typeof error === "object" && error !== null && "code" in error + ? String((error as { code: unknown }).code) + : "error"; + return `Failed (${ERROR_CODE.test(code) ? code : "error"}).`; +} diff --git a/src/lib/upgrade-check-project.ts b/src/lib/upgrade-check-project.ts index e22c9ca7a..244feada4 100644 --- a/src/lib/upgrade-check-project.ts +++ b/src/lib/upgrade-check-project.ts @@ -95,6 +95,8 @@ export type RegistryEntryState = export type RegisteredCheckout = { /** The project name, with ` (worktree )` for a linked worktree. */ readonly name: string; + /** The registered project's name, the same for all its worktrees. */ + readonly project: string; readonly repoRoot: string; readonly projectDir: string; readonly projectDirName: string; @@ -107,12 +109,14 @@ export function registeredCheckouts( return [ { name: project.name, + project: project.name, repoRoot: project.repoRoot, projectDir: project.projectDir, projectDirName: project.projectDirName, }, ...(project.worktrees ?? []).map((worktree) => ({ name: `${project.name} (worktree ${basename(worktree.path)})`, + project: project.name, repoRoot: worktree.path, projectDir: resolve(worktree.path, project.projectDirName), projectDirName: project.projectDirName, diff --git a/src/lib/upgrade-check-render.ts b/src/lib/upgrade-check-render.ts new file mode 100644 index 000000000..f6018b4e9 --- /dev/null +++ b/src/lib/upgrade-check-render.ts @@ -0,0 +1,277 @@ +import { escapeTerminalControls } from "./terminal-text.ts"; +import type { + UpgradeCheckReport, + UpgradeFinding, + UpgradePlanStep, + UpgradeProjectReport, +} from "./upgrade-check.ts"; + +/** + * Text output of `hack upgrade check` (HACK-2220): one screen whatever the + * number of checkouts. The summary comes first (build, machine, counts, + * what blocks, what could not be checked), then the projects that have a + * finding (worktrees folded under their project), then the plan grouped + * by kind of change, then the single next command. `verbose` adds every + * checkout's findings and the dead registry entries. Every line is escaped; + * no value is ever printed. `--json` keeps the full detail. + */ + +const MARK: Readonly> = { + ok: "ok ", + info: "info ", + warn: "warn ", + incomplete: "UNCHK", + blocker: "BLOCK", +}; + +/** Projects listed in the summary before the rest are counted. */ +const MAX_PROJECT_LINES = 12; +/** Blockers or unchecked items named before the rest are counted. */ +const MAX_NAMED = 5; + +const NEEDS_WORK: ReadonlySet = new Set([ + "blocker", + "incomplete", + "warn", +]); + +/** Short names for finding ids in the folded project lines and the plan. */ +const LABEL: Readonly> = { + "project-config": "unreadable config", + "project-store": "project secret store", + "env-secrets": "env secrets", + "env-format": "legacy env format", + "env-key": "env key", + "tracked-files": "tracked machine-local files", + "ignore-rules": "outdated .hack/.gitignore", + "agent-integrations": "stale agent files", +}; + +export function renderUpgradeCheck( + report: UpgradeCheckReport, + opts: { readonly verbose?: boolean } = {} +): string { + const lines = [ + buildLine(report), + "", + ...machineLines(report), + "", + ...projectSummaryLines(report), + ...(opts.verbose === true ? verboseLines(report) : []), + "", + ...planLines(report), + ]; + return `${lines.map((text) => escapeTerminalControls(text)).join("\n")}\n`; +} + +/** First line: version, channel, and whether a source build or a candidate launcher ran. */ +function buildLine(report: UpgradeCheckReport): string { + const how: string[] = [`${report.cli.channel} channel`]; + if (report.cli.build === "source") { + how.push("SOURCE BUILD, not an installed release"); + } + if (report.cli.candidate_run) { + how.push(`candidate launcher: ${report.cli.command}`); + } + return `hack ${report.cli.version} (${how.join("; ")})`; +} + +function machineLines(report: UpgradeCheckReport): string[] { + const lines = ["Machine"]; + for (const finding of report.machine) { + if (finding.id === "registry" && finding.status === "ok") { + continue; // The project counts below say it. + } + if (finding.id === "registry-dead") { + lines.push( + ` ${MARK.warn} ${report.registry.missing.length} dead registry entr${report.registry.missing.length === 1 ? "y" : "ies"} (checkout or config gone): ${finding.command ?? ""}` + ); + continue; + } + lines.push(` ${MARK[finding.status]} ${finding.summary}`); + } + return lines; +} + +type ProjectGroup = { + readonly project: string; + readonly checkouts: readonly UpgradeProjectReport[]; +}; + +function groupProjects( + projects: readonly UpgradeProjectReport[] +): ProjectGroup[] { + const groups = new Map(); + for (const checkout of projects) { + const list = groups.get(checkout.project) ?? []; + list.push(checkout); + groups.set(checkout.project, list); + } + return [...groups.entries()].map(([project, checkouts]) => ({ + project, + checkouts, + })); +} + +function projectSummaryLines(report: UpgradeCheckReport): string[] { + const r = report.registry; + const lines = [ + `Projects: ${r.projects} registered, ${r.total} checkout(s) with worktrees: ${r.live} live, ${r.missing.length} dead, ${r.unreadable.length} unreadable${r.unchecked.length > 0 ? `, ${r.unchecked.length} not checked` : ""}.`, + ]; + const groups = groupProjects(report.projects); + const worst = (group: ProjectGroup) => { + const statuses = group.checkouts.flatMap((c) => + c.findings.map((f) => f.status) + ); + if (statuses.includes("blocker")) { + return 0; + } + return statuses.includes("incomplete") ? 1 : 2; + }; + const withFindings = groups + .filter((group) => + group.checkouts.some((c) => + c.findings.some((f) => NEEDS_WORK.has(f.status)) + ) + ) + .sort((a, b) => worst(a) - worst(b) || a.project.localeCompare(b.project)); + if (withFindings.length === 0) { + return lines; + } + lines.push( + `With findings: ${withFindings.length} of ${groups.length} project(s) (hack upgrade check --verbose shows every checkout):` + ); + for (const group of withFindings.slice(0, MAX_PROJECT_LINES)) { + lines.push(projectLine(group)); + } + const rest = withFindings.length - MAX_PROJECT_LINES; + if (rest > 0) { + lines.push(` … and ${rest} more project(s); the plan below covers them.`); + } + return lines; +} + +/** One line per project: its worst status and its finding kinds, with checkout counts. */ +function projectLine(group: ProjectGroup): string { + const counts = new Map(); + let mark = MARK.warn; + for (const checkout of group.checkouts) { + for (const finding of checkout.findings) { + if (!NEEDS_WORK.has(finding.status)) { + continue; + } + if (finding.status === "blocker") { + mark = MARK.blocker; + } else if (finding.status === "incomplete" && mark !== MARK.blocker) { + mark = MARK.incomplete; + } + const label = LABEL[finding.id] ?? finding.id; + counts.set(label, (counts.get(label) ?? 0) + 1); + } + } + const worktrees = group.checkouts.length - 1; + const name = + worktrees > 0 + ? `${group.project} (+${worktrees} worktree${worktrees === 1 ? "" : "s"})` + : group.project; + const kinds = [...counts.entries()] + .map(([label, n]) => (n > 1 ? `${label} ×${n}` : label)) + .join(", "); + return ` ${mark} ${name}: ${kinds}`; +} + +function verboseLines(report: UpgradeCheckReport): string[] { + const lines: string[] = []; + if (report.registry.missing.length > 0) { + lines.push( + "", + `Dead entries: ${report.registry.missing.map((entry) => entry.name).join(", ")}` + ); + } + for (const project of report.projects) { + lines.push("", `Checkout ${project.name} (${project.root})`); + for (const finding of project.findings) { + lines.push(` ${MARK[finding.status]} ${finding.summary}`); + if (NEEDS_WORK.has(finding.status) && finding.command !== null) { + lines.push(` ${finding.command}`); + } + } + } + return lines; +} + +function planLines(report: UpgradeCheckReport): string[] { + if (report.plan.length === 0) { + return report.ready + ? ["Nothing to do: this machine and its projects are ready."] + : ["No step to suggest, but the check is not complete; see above."]; + } + const lines = ["Plan", ...report.plan.flatMap(stepLines)]; + if (report.plan.some((step) => step.applies)) { + lines.push( + " Steps marked (apply) run for all their checkouts with hack upgrade apply (--dry-run to preview, --project for one project)." + ); + } + lines.push(""); + lines.push(...namedLines("Blocked by", report.blockers)); + lines.push(...namedLines("Not checked", report.incomplete, true)); + if (report.next_command !== null) { + lines.push(`Next: ${report.next_command}`); + } else if (report.next_action !== null) { + lines.push(`Next (manual): ${report.next_action}`); + } + return lines; +} + +function stepLines(step: UpgradePlanStep): string[] { + const tag = + { + blocker: "[blocker] ", + incomplete: "[not checked] ", + }[step.status as string] ?? ""; + const label = LABEL[step.id]; + const what = + step.count > 1 + ? `${label ?? step.id}, ${step.count} checkouts${step.applies ? " (apply)" : ""}: ` + : ""; + let scope = ""; + if (step.count === 1) { + scope = ` (${step.scope}${step.applies ? ", apply" : ""})`; + } else if (step.count === 0 && step.scope !== "machine") { + scope = ` (${step.scope})`; + } + const head = + step.command === null + ? ` ${step.order}. ${tag}${what}manual: ${step.action ?? step.why}${scope}` + : ` ${step.order}. ${tag}${what}${step.command}${scope}`; + return step.count <= 1 && step.command !== null && step.action !== null + ? [head, ` ${step.action}`] + : [head]; +} + +/** Blockers or unchecked items: a few named (with why for unchecked), the rest counted. */ +function namedLines( + title: string, + items: UpgradeCheckReport["blockers"], + why = false +): string[] { + if (items.length === 0) { + return []; + } + const shown = items.slice(0, MAX_NAMED); + const lines = why + ? [ + `${title} (${items.length}):`, + ...shown.map((item) => ` ${item.scope} ${item.id}: ${item.summary}`), + ] + : [ + `${title}: ${shown.map((item) => `${item.scope} ${item.id}`).join(", ")}${items.length > MAX_NAMED ? ` and ${items.length - MAX_NAMED} more` : ""}`, + ]; + if (why && items.length > MAX_NAMED) { + lines.push( + ` … and ${items.length - MAX_NAMED} more (hack upgrade check --verbose).` + ); + } + lines.push(""); + return lines; +} diff --git a/src/lib/upgrade-check.ts b/src/lib/upgrade-check.ts index 8dbc23cf5..7d6899050 100644 --- a/src/lib/upgrade-check.ts +++ b/src/lib/upgrade-check.ts @@ -26,11 +26,13 @@ import { type UpgradeFinding, type UpgradeFindingStatus, } from "./upgrade-check-project.ts"; +import { buildUpgradePlan, type UpgradePlanStep } from "./upgrade-plan.ts"; export type { UpgradeFinding, UpgradeFindingStatus, } from "./upgrade-check-project.ts"; +export type { UpgradePlanStep, UpgradePlanTarget } from "./upgrade-plan.ts"; /** * `hack upgrade check` (HACK-2189, the upgrade assistant HACK-2188): a @@ -54,7 +56,12 @@ export type { * action, never prose labelled as a command. */ -export const UPGRADE_CHECK_REPORT_VERSION = 1; +/** + * 2 (HACK-2220): the plan has one step per kind of change (`count`, + * `targets`, `applies`), project reports name their registered `project` + * and `project_dir`, and `cli.build` says whether a source build ran. + */ +export const UPGRADE_CHECK_REPORT_VERSION = 2; /** How printed commands run the CLI that produced the report. */ export type UpgradeCommandPrefix = { @@ -110,26 +117,14 @@ export type UpgradeReportFinding = UpgradeFinding & { readonly command: string | null; }; -export type UpgradePlanStep = { - readonly order: number; - /** The finding this step resolves, or `snapshot`. */ - readonly id: string; - /** `machine`, or the checkout's registered name. */ - readonly scope: string; - readonly status: UpgradeFindingStatus | "step"; - /** An executable command line; null for a manual step. */ - readonly command: string | null; - /** Its Hack CLI arguments (after `cli.command`); null for a manual step. */ - readonly args: readonly string[] | null; - /** A manual action: what to do when there is no command, or instead of it. */ - readonly action: string | null; - readonly why: string; - readonly blocker: boolean; -}; - export type UpgradeProjectReport = { + /** The checkout's name: the project's, or `project (worktree dir)`. */ readonly name: string; + /** The registered project this checkout belongs to. */ + readonly project: string; readonly root: string; + /** Its Hack directory (`.hack`), absolute. */ + readonly project_dir: string | null; readonly state: "live" | "unreadable"; readonly reason: string | null; readonly findings: readonly UpgradeReportFinding[]; @@ -149,6 +144,8 @@ export type UpgradeCheckReport = { readonly version: string; readonly channel: "stable" | "next"; readonly candidate_run: boolean; + /** `source` when the CLI ran from a source checkout, not a built binary. */ + readonly build: "release" | "source"; /** How printed commands run this CLI, as a command-line prefix. */ readonly command: string; }; @@ -185,6 +182,7 @@ export type UpgradeCheckReport = { type RegistryEntrySummary = { readonly name: string; + readonly project: string; readonly root: string; readonly reason: string; }; @@ -201,6 +199,8 @@ export async function runUpgradeCheck(opts: { readonly homeDir: string; readonly probes: UpgradeCheckProbes; readonly env?: Readonly>; + /** How the running CLI was built (default `release`). */ + readonly build?: "release" | "source"; readonly now?: () => number; /** Test seams: the whole budget and the per-check limit. */ readonly budgetMs?: number; @@ -230,7 +230,9 @@ export async function runUpgradeCheck(opts: { PROJECT_CONCURRENCY, async (project): Promise => ({ name: project.name, + project: project.project, root: project.repoRoot, + project_dir: project.projectDir, state: "live", reason: null, findings: ( @@ -247,7 +249,9 @@ export async function runUpgradeCheck(opts: { const unreadableProjects: UpgradeProjectReport[] = registry.report.unreadable.map((entry) => ({ name: entry.name, + project: entry.project, root: entry.root, + project_dir: null, state: "unreadable", reason: entry.reason, findings: [ @@ -285,10 +289,10 @@ export async function runUpgradeCheck(opts: { ]; const blockers = scoped("blocker"); const incomplete = scoped("incomplete"); - const plan = buildPlan({ + const plan: UpgradePlanStep[] = buildUpgradePlan({ machine: machineFindings, projects: allProjects, - prefix, + render: (args) => renderUpgradeCommand(prefix, args), }); const first = plan[0]; const version = opts.cliVersion; @@ -300,6 +304,7 @@ export async function runUpgradeCheck(opts: { version, channel: version.includes("-next.") ? "next" : "stable", candidate_run: isCandidateRun(env), + build: opts.build ?? "release", command: renderUpgradeCommand(prefix, []), }, host_probes: opts.probes.disabled === true ? "disabled" : "enabled", @@ -368,6 +373,7 @@ async function inspectRegistry(opts: { readonly deadline: number }): Promise<{ ); const summary = (checkout: RegisteredCheckout, reason: string) => ({ name: checkout.name, + project: checkout.project, root: checkout.repoRoot, reason, }); @@ -1070,110 +1076,6 @@ function uncertainReadersFinding(readers: ReaderKeyStates): UpgradeFinding { // Plan // --------------------------------------------------------------------------- -/** Machine fixes in the order they should run. */ -const MACHINE_ORDER = [ - "host-probes", - "registry", - "registry-unchecked", - "secret-store", - "registry-dead", - "registry-rejected", - "docker", - "global-infra", - "linux-dns", - "proxy-ports", - "daemon", -] as const; - -/** Checkout fixes in the order they should run. */ -const PROJECT_ORDER = [ - "project-config", - "project-store", - "env-secrets", - "env-format", - "env-key", - "tracked-files", - "ignore-rules", - "agent-integrations", -] as const; - -const ACTIONABLE: ReadonlySet = new Set([ - "blocker", - "incomplete", - "warn", -]); - -function buildPlan(opts: { - readonly machine: readonly UpgradeReportFinding[]; - readonly projects: readonly UpgradeProjectReport[]; - readonly prefix: UpgradeCommandPrefix; -}): UpgradePlanStep[] { - type Draft = Omit; - const actionable = (finding: UpgradeReportFinding) => - ACTIONABLE.has(finding.status) && - (finding.args !== null || finding.action !== null); - const rank = - (order: readonly string[]) => (finding: UpgradeReportFinding) => { - const index = order.indexOf(finding.id); - return index === -1 ? order.length : index; - }; - const toStep = - (scope: string) => - (finding: UpgradeReportFinding): Draft => ({ - id: finding.id, - scope, - status: finding.status, - command: finding.command, - args: finding.args, - action: finding.action, - why: finding.summary, - blocker: finding.status === "blocker", - }); - const machineSteps = [...opts.machine] - .filter(actionable) - .sort((a, b) => rank(MACHINE_ORDER)(a) - rank(MACHINE_ORDER)(b)) - .map(toStep("machine")); - const projectSteps = opts.projects.flatMap((project) => - [...project.findings] - .filter(actionable) - .sort((a, b) => rank(PROJECT_ORDER)(a) - rank(PROJECT_ORDER)(b)) - .map(toStep(project.name)) - ); - const steps = [...machineSteps, ...projectSteps]; - const severity = (step: Draft) => { - if (step.status === "blocker") { - return 0; - } - return step.status === "incomplete" ? 1 : 2; - }; - const ordered = steps - .map((step, index) => ({ step, index })) - .sort((a, b) => severity(a.step) - severity(b.step) || a.index - b.index) - .map(({ step }) => step); - // A snapshot first, before any step changes a file, so `hack upgrade - // rollback` can put back what the following steps change. It covers - // files only: not the Git index, services or containers. - const snapshot: Draft[] = - ordered.length > 0 - ? [ - { - id: "snapshot", - scope: "machine", - status: "step", - command: renderUpgradeCommand(opts.prefix, ["upgrade", "snapshot"]), - args: ["upgrade", "snapshot"], - action: null, - why: "Keep a private copy of the files the following steps change, so the upgrade rollback command can put them back.", - blocker: false, - }, - ] - : []; - return [...snapshot, ...ordered].map((step, index) => ({ - order: index + 1, - ...step, - })); -} - // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- diff --git a/src/lib/upgrade-plan.ts b/src/lib/upgrade-plan.ts new file mode 100644 index 000000000..73c965f34 --- /dev/null +++ b/src/lib/upgrade-plan.ts @@ -0,0 +1,326 @@ +import type { + UpgradeProjectReport, + UpgradeReportFinding, +} from "./upgrade-check.ts"; +import type { UpgradeFindingStatus } from "./upgrade-check-project.ts"; + +/** + * The upgrade plan (HACK-2188, HACK-2220): one step per kind of change, not + * one per checkout. A machine finding is its own step. Checkout findings of + * the same id and status are one step that lists every checkout it covers + * (`targets`, each with its own command), so a machine with 170 checkouts + * gets a plan of about ten steps. + * + * The fixes `hack upgrade apply` runs for every checkout of a step + * ({@link UPGRADE_APPLY_FIXES}) are the ones whose writes a snapshot or a + * backup manifest can undo: refreshing `.hack/.gitignore` (a file in the + * upgrade snapshot) and removing stale standalone agent files (each + * removal backed up by the plugin migration's own manifest). Every other + * step keeps its per-checkout command or manual action. + */ + +/** Checkout fixes `hack upgrade apply` runs, in the order it runs them. */ +export const UPGRADE_APPLY_FIXES = [ + "ignore-rules", + "agent-integrations", +] as const; + +export type UpgradeApplyFix = (typeof UPGRADE_APPLY_FIXES)[number]; + +export function isUpgradeApplyFix(id: string): id is UpgradeApplyFix { + return (UPGRADE_APPLY_FIXES as readonly string[]).includes(id); +} + +/** One checkout a plan step covers. */ +export type UpgradePlanTarget = { + /** The checkout's name (`project (worktree dir)` for a worktree). */ + readonly scope: string; + /** The registered project it belongs to. */ + readonly project: string; + readonly root: string; + /** This checkout's own command line; null when only `action` applies. */ + readonly command: string | null; + readonly args: readonly string[] | null; + readonly action: string | null; +}; + +export type UpgradePlanStep = { + readonly order: number; + /** The finding id this step resolves, or `snapshot`. */ + readonly id: string; + /** + * `machine`, the checkout's name when the step covers one checkout, or + * `projects` when it covers several (listed in `targets`). + */ + readonly scope: string; + readonly status: UpgradeFindingStatus | "step"; + /** How many checkouts the step covers (0 for a machine step). */ + readonly count: number; + /** The checkouts it covers; empty for a machine step. */ + readonly targets: readonly UpgradePlanTarget[]; + /** One executable command line for the whole step; null for a manual step. */ + readonly command: string | null; + /** Its Hack CLI arguments (after `cli.command`); null for a manual step. */ + readonly args: readonly string[] | null; + /** A manual action: what to do when there is no command, or instead of it. */ + readonly action: string | null; + readonly why: string; + readonly blocker: boolean; + /** `hack upgrade apply` runs this step for every checkout it covers. */ + readonly applies: boolean; +}; + +/** Machine fixes in the order they should run. */ +const MACHINE_ORDER = [ + "host-probes", + "registry", + "registry-unchecked", + "secret-store", + "registry-dead", + "registry-rejected", + "docker", + "global-infra", + "linux-dns", + "proxy-ports", + "daemon", +] as const; + +/** Checkout fixes in the order they should run. */ +const PROJECT_ORDER = [ + "project-config", + "project-store", + "env-secrets", + "env-format", + "env-key", + "tracked-files", + "ignore-rules", + "agent-integrations", +] as const; + +/** What a step covering several checkouts is about (no counts, no values). */ +const GROUP_WHY: Readonly> = { + "project-config": "These live checkouts cannot be read.", + "project-store": "The project's own secret store or its key needs attention.", + "env-secrets": "Env layer secrets must open under the project env key.", + "env-format": "Env uses the legacy format; migrate it to YAML layers.", + "env-key": "The env key is missing, not private, or tracked.", + "tracked-files": + "Machine-local or sensitive files are tracked or not ignored.", + "ignore-rules": "The committed .hack/.gitignore is missing or out of date.", + "agent-integrations": + "Standalone Hack agent files are stale; the hack plugin replaces them.", +}; + +const ACTIONABLE: ReadonlySet = new Set([ + "blocker", + "incomplete", + "warn", +]); + +const PATH_PLACEHOLDER = ""; + +/** + * Builds the grouped plan. `render` turns Hack CLI arguments into a command + * line for the CLI that produced the report. + */ +export function buildUpgradePlan(opts: { + readonly machine: readonly UpgradeReportFinding[]; + readonly projects: readonly UpgradeProjectReport[]; + readonly render: (args: readonly string[]) => string; +}): UpgradePlanStep[] { + type Draft = Omit; + const actionable = (finding: UpgradeReportFinding) => + ACTIONABLE.has(finding.status) && + (finding.args !== null || finding.action !== null); + const rank = (order: readonly string[], id: string) => { + const index = order.indexOf(id); + return index === -1 ? order.length : index; + }; + const machineSteps: Draft[] = [...opts.machine] + .filter(actionable) + .sort((a, b) => rank(MACHINE_ORDER, a.id) - rank(MACHINE_ORDER, b.id)) + .map((finding) => ({ + id: finding.id, + scope: "machine", + status: finding.status, + count: 0, + targets: [], + command: finding.command, + args: finding.args, + action: finding.action, + why: finding.summary, + blocker: finding.status === "blocker", + applies: false, + })); + const groups = new Map< + string, + { + id: string; + status: UpgradeFindingStatus; + findings: { target: UpgradePlanTarget; summary: string }[]; + } + >(); + for (const project of opts.projects) { + for (const finding of project.findings) { + if (!actionable(finding)) { + continue; + } + const key = `${finding.id}\u0000${finding.status}`; + const group = groups.get(key) ?? { + id: finding.id, + status: finding.status, + findings: [], + }; + group.findings.push({ + target: { + scope: project.name, + project: project.project, + root: project.root, + command: finding.command, + args: finding.args, + action: finding.action, + }, + summary: finding.summary, + }); + groups.set(key, group); + } + } + const projectSteps: Draft[] = [...groups.values()] + .sort((a, b) => rank(PROJECT_ORDER, a.id) - rank(PROJECT_ORDER, b.id)) + .map((group) => groupStep({ group, render: opts.render })); + const severity = (step: Draft) => { + if (step.status === "blocker") { + return 0; + } + return step.status === "incomplete" ? 1 : 2; + }; + const ordered = [...machineSteps, ...projectSteps] + .map((step, index) => ({ step, index })) + .sort((a, b) => severity(a.step) - severity(b.step) || a.index - b.index) + .map(({ step }) => step); + // A snapshot first, before any step changes a file, so `hack upgrade + // rollback` can put back what the following steps change. It covers + // files only: not the Git index, services or containers. + const snapshot: Draft[] = + ordered.length > 0 + ? [ + { + id: "snapshot", + scope: "machine", + status: "step", + count: 0, + targets: [], + command: opts.render(["upgrade", "snapshot"]), + args: ["upgrade", "snapshot"], + action: null, + why: "Keep a private copy of the files the following steps change, so the upgrade rollback command can put them back.", + blocker: false, + applies: false, + }, + ] + : []; + return [...snapshot, ...ordered].map((step, index) => ({ + order: index + 1, + ...step, + })); +} + +function groupStep(opts: { + readonly group: { + readonly id: string; + readonly status: UpgradeFindingStatus; + readonly findings: readonly { + readonly target: UpgradePlanTarget; + readonly summary: string; + }[]; + }; + readonly render: (args: readonly string[]) => string; +}): Omit { + const { id, status, findings } = opts.group; + const targets = findings.map((finding) => finding.target); + const base = { + id, + status, + count: targets.length, + targets, + blocker: status === "blocker", + }; + if (status === "warn" && isUpgradeApplyFix(id)) { + const args = ["upgrade", "apply", "--only", id]; + return { + ...base, + scope: + targets.length === 1 ? (targets[0]?.scope ?? "projects") : "projects", + command: opts.render(args), + args, + action: null, + why: GROUP_WHY[id] ?? findings[0]?.summary ?? id, + applies: true, + }; + } + const only = findings.length === 1 ? findings[0] : undefined; + if (only !== undefined) { + return { + ...base, + scope: only.target.scope, + command: only.target.command, + args: only.target.args, + action: only.target.action, + why: only.summary, + applies: false, + }; + } + return { + ...base, + scope: "projects", + command: null, + args: null, + action: groupAction({ targets, render: opts.render }), + why: GROUP_WHY[id] ?? findings[0]?.summary ?? id, + applies: false, + }; +} + +/** + * A manual action for a step over several checkouts: the shared command + * with the checkout as a placeholder when every target runs the same + * command for its own path, else a pointer to the per-checkout findings. + */ +function groupAction(opts: { + readonly targets: readonly UpgradePlanTarget[]; + readonly render: (args: readonly string[]) => string; +}): string { + const count = opts.targets.length; + const shapes = new Set( + opts.targets.map((target) => + target.args === null + ? null + : JSON.stringify(withoutPath(target.args, target.root)) + ) + ); + const first = opts.targets[0]; + const shared = + shapes.size === 1 && first?.args + ? withoutPath(first.args, first.root) + : null; + const advice = opts.targets.some((target) => target.action !== null) + ? " Each checkout's finding says what else to do." + : ""; + if (shared !== null && first?.args?.includes(first.root)) { + return `Run ${opts.render(shared)} --path ${PATH_PLACEHOLDER} for each of the ${count} checkouts (hack upgrade check --verbose lists them).${advice}`; + } + return `See the finding in each of the ${count} checkouts (hack upgrade check --verbose lists them).`; +} + +/** `args` without its `--path ` pair. */ +function withoutPath(args: readonly string[], root: string): readonly string[] { + const out: string[] = []; + for (let index = 0; index < args.length; index += 1) { + if (args[index] === "--path" && args[index + 1] === root) { + index += 1; + continue; + } + out.push(args[index] as string); + } + return out; +} diff --git a/src/lib/upgrade-snapshot.ts b/src/lib/upgrade-snapshot.ts index ce2eed179..32de59e58 100644 --- a/src/lib/upgrade-snapshot.ts +++ b/src/lib/upgrade-snapshot.ts @@ -511,6 +511,58 @@ export async function listUpgradeSnapshots( ); } +// --------------------------------------------------------------------------- +// Journal +// --------------------------------------------------------------------------- + +const JOURNAL_NAME = "journal.json"; +const MAX_JOURNAL_BYTES = 16 * 1024 * 1024; + +/** + * Writes the step journal of the step that owns this snapshot (for example + * `hack upgrade apply`) beside its manifest, atomically and privately. + * The journal is caller data and, like labels, must hold no value: paths, + * ids, statuses, counts and closed messages only. Returns its path. + */ +export async function writeUpgradeSnapshotJournal(input: { + readonly snapshot: Pick; + readonly journal: unknown; +}): Promise { + if ((await privateDirectoryProblem(input.snapshot.dir)) !== null) { + throw new Error( + "The upgrade snapshot directory is not private to this user." + ); + } + const path = join(input.snapshot.dir, JOURNAL_NAME); + await writePrivateJson({ path, value: input.journal }); + return path; +} + +/** + * The journal beside a snapshot manifest, parsed; null when there is none + * or it is not a private file of this user holding JSON. + */ +export async function readUpgradeSnapshotJournal(input: { + readonly manifestPath: string; +}): Promise { + const dir = dirname(resolve(input.manifestPath)); + if ((await privateDirectoryProblem(dir)) !== null) { + return null; + } + const text = await readPrivateFile({ + path: join(dir, JOURNAL_NAME), + maxBytes: MAX_JOURNAL_BYTES, + }); + if (text === null) { + return null; + } + try { + return JSON.parse(text.toString("utf8")) as unknown; + } catch { + return null; + } +} + // --------------------------------------------------------------------------- // Reading targets // --------------------------------------------------------------------------- @@ -1107,6 +1159,14 @@ async function readBlob(input: { async function writeManifest(input: { readonly path: string; readonly manifest: UpgradeSnapshotManifest; +}): Promise { + await writePrivateJson({ path: input.path, value: input.manifest }); +} + +/** Atomic private JSON write: no-follow exclusive temporary file, fsync, rename. */ +async function writePrivateJson(input: { + readonly path: string; + readonly value: unknown; }): Promise { const temporary = `${input.path}.${randomBytes(6).toString("hex")}.tmp`; const handle = await open( @@ -1118,7 +1178,7 @@ async function writeManifest(input: { 0o600 ); try { - await handle.writeFile(`${JSON.stringify(input.manifest, null, 2)}\n`); + await handle.writeFile(`${JSON.stringify(input.value, null, 2)}\n`); await handle.sync(); } finally { await handle.close(); diff --git a/tests/upgrade-apply.test.ts b/tests/upgrade-apply.test.ts new file mode 100644 index 000000000..50e58873f --- /dev/null +++ b/tests/upgrade-apply.test.ts @@ -0,0 +1,578 @@ +import { afterEach, beforeEach, expect, test } from "bun:test"; +import { createHash } from "node:crypto"; +import { + lstat, + mkdir, + mkdtemp, + readdir, + readFile, + realpath, + rm, + stat, + symlink, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; + +import { runUpgradeApply } from "../src/lib/upgrade-apply.ts"; +import { + runUpgradeCheck, + type UpgradeCheckReport, +} from "../src/lib/upgrade-check.ts"; +import type { UpgradeCheckProbes } from "../src/lib/upgrade-check-probes.ts"; +import { renderUpgradeCheck } from "../src/lib/upgrade-check-render.ts"; +import { + readUpgradeSnapshotJournal, + restoreUpgradeSnapshot, +} from "../src/lib/upgrade-snapshot.ts"; +import { collectUpgradeSnapshotTargets } from "../src/lib/upgrade-snapshot-targets.ts"; +import { restoreEnv } from "./helpers/env.ts"; +import { fixtureGitEnv } from "./helpers/fixture-git.ts"; +import { privateCliEnv } from "./helpers/private-cli-env.ts"; + +// Values that must never appear in any report, plan, journal or output. +const SENTINEL = ["sentinel", "upgrade", "apply", "7b2e19"].join("-"); +const ENTRYPOINT = resolve(import.meta.dir, "..", "index.ts"); + +const ISOLATED_ENV = [ + "HOME", + "HACK_HOME", + "HACK_GLOBAL_CONFIG_PATH", + "HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK", + "HACK_SECRETS_FILE_KEY", + "HACK_ENV_SECRET_KEY", +] as const; +const saved = new Map(); +let root = ""; +let home = ""; +let hackHome = ""; + +beforeEach(async () => { + for (const key of ISOLATED_ENV) { + saved.set(key, process.env[key]); + } + root = await realpath(await mkdtemp(join(tmpdir(), "hack-upgrade-apply-"))); + home = join(root, "user-home"); + hackHome = join(home, ".hack"); + await mkdir(hackHome, { recursive: true }); + process.env.HOME = home; + process.env.HACK_HOME = hackHome; + process.env.HACK_GLOBAL_CONFIG_PATH = join(hackHome, "hack.config.json"); + process.env.HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK = "1"; + Reflect.deleteProperty(process.env, "HACK_SECRETS_FILE_KEY"); + Reflect.deleteProperty(process.env, "HACK_ENV_SECRET_KEY"); +}); + +afterEach(async () => { + for (const key of ISOLATED_ENV) { + restoreEnv(key, saved.get(key)); + } + await rm(root, { recursive: true, force: true }); +}); + +type Worktree = { path: string; branch: string; lastSeenAt: string }; +type Entry = { + id: string; + name: string; + repoRoot: string; + projectDirName: ".hack"; + projectDir: string; + createdAt: string; + worktrees?: Worktree[]; +}; + +function git(dir: string, args: readonly string[]) { + const result = Bun.spawnSync(["git", "-C", dir, ...args], { + env: fixtureGitEnv({ env: { HOME: home } }), + }); + if (result.exitCode !== 0) { + throw new Error(`fixture git ${args[0]} failed`); + } +} + +/** A Git checkout with a Hack config and no .hack/.gitignore. */ +async function checkout(dir: string, name: string): Promise { + await mkdir(join(dir, ".hack"), { recursive: true }); + await writeFile( + join(dir, ".hack", "hack.config.json"), + `${JSON.stringify({ name, dev_host: `${name}.hack` })}\n` + ); + git(dir, ["init", "-q"]); +} + +async function project( + name: string, + worktrees = 0 +): Promise { + const repoRoot = join(root, "repos", name); + await checkout(repoRoot, name); + const trees: Worktree[] = []; + for (let i = 0; i < worktrees; i += 1) { + const path = join(root, "worktrees", name, `wt${i}`); + await checkout(path, name); + trees.push({ + path, + branch: `wt${i}`, + lastSeenAt: "2026-01-01T00:00:00.000Z", + }); + } + return { + id: `id-${name}`, + name, + repoRoot, + projectDirName: ".hack", + projectDir: join(repoRoot, ".hack"), + createdAt: "2026-01-01T00:00:00.000Z", + ...(trees.length > 0 ? { worktrees: trees } : {}), + checkouts: [repoRoot, ...trees.map((t) => t.path)], + }; +} + +function dead(name: string): Entry { + const repoRoot = join(root, "gone", name); + return { + id: `id-${name}`, + name, + repoRoot, + projectDirName: ".hack", + projectDir: join(repoRoot, ".hack"), + createdAt: "2026-01-01T00:00:00.000Z", + }; +} + +async function writeRegistry( + projects: readonly (Entry & { checkouts?: string[] })[] +): Promise { + const entries = projects.map(({ checkouts: _unused, ...entry }) => entry); + await writeFile( + join(hackHome, "projects.json"), + `${JSON.stringify({ version: 1, projects: entries })}\n` + ); +} + +function healthyProbes(): UpgradeCheckProbes { + return { + pathHack: async () => ({ + path: "/usr/local/bin/hack", + install: "standalone", + version: "4.3.0", + }), + daemon: async () => ({ + manager: "systemd", + registered: true, + enabled: true, + running: true, + managed: true, + summary: "installed, enabled, active (pid 42)", + severity: "ok", + nextStep: null, + }), + docker: async () => "reachable", + globalInfra: async () => ({ installed: true, running: true }), + linuxDns: async () => null, + proxyPorts: async () => ({ + name: "proxy ports", + status: "ok", + message: "Ports 80 and 443 reach Hack's proxy.", + verified: true, + reachable: true, + }), + nativePreflight: async () => null, + }; +} + +async function check(): Promise { + return await runUpgradeCheck({ + cliVersion: "5.0.0-next.17", + homeDir: home, + probes: healthyProbes(), + build: "source", + budgetMs: 180_000, + }); +} + +/** Every file under `dir`: path, mode and digest, for a no-change proof. */ +async function tree(dir: string): Promise { + const out: string[] = []; + const walk = async (current: string) => { + for (const name of (await readdir(current)).sort()) { + const path = join(current, name); + const info = await lstat(path); + if (info.isDirectory()) { + out.push(`${path}/ ${info.mode & 0o7777}`); + await walk(path); + } else if (info.isFile()) { + const digest = createHash("sha256") + .update(await readFile(path)) + .digest("hex"); + out.push(`${path} ${info.mode & 0o7777} ${digest}`); + } else { + out.push(`${path} link`); + } + } + }; + await walk(dir); + return out; +} + +/** A stale standalone Hack skill under HOME: every checkout reports stale agent files. */ +async function staleHomeAgentFile(): Promise { + const path = join(home, ".codex", "skills", "hack-cli", "SKILL.md"); + await mkdir(join(path, ".."), { recursive: true }); + await writeFile(path, "# hack-cli\nan old standalone skill\n"); + return path; +} + +test("100 checkouts fit one screen: summary first, worktrees folded, plan grouped by kind", async () => { + await staleHomeAgentFile(); + const projects: (Entry & { checkouts: string[] })[] = []; + for (let i = 0; i < 25; i += 1) { + const name = `app-${String(i).padStart(2, "0")}`; + const made = await project(name, 3); + // Mixed findings: legacy env in some, a tracked env file in others. + if (i % 3 === 0) { + for (const dir of made.checkouts) { + await writeFile(join(dir, ".hack", "hack.env.json"), "{}\n"); + await writeFile(join(dir, ".hack", ".env"), `TOKEN=${SENTINEL}\n`); + } + } + if (i % 5 === 0) { + git(made.repoRoot, ["add", "-f", ".hack/hack.config.json"]); + git(made.repoRoot, ["commit", "-q", "-m", "fixture"]); + await writeFile(join(made.projectDir, ".env"), `TOKEN=${SENTINEL}\n`); + git(made.repoRoot, ["add", "-f", ".hack/.env"]); + git(made.repoRoot, ["commit", "-q", "-m", "tracked"]); + } + projects.push(made); + } + // One blocker: an env layer that is a dangling link. + await symlink( + join(root, "nowhere.yaml"), + join(projects[7]?.checkouts[2] as string, ".hack", "hack.env.default.yaml") + ); + const deadEntries = Array.from({ length: 12 }, (_, i) => + dead(`old-${String(i).padStart(2, "0")}`) + ); + await writeRegistry([...projects, ...deadEntries]); + + const report = await check(); + expect(report.report_version).toBe(2); + expect(report.registry.total).toBe(112); + expect(report.registry.live).toBe(100); + // Stale agent files and the missing .gitignore are one step each. + const agent = report.plan.find((s) => s.id === "agent-integrations"); + expect(agent?.count).toBe(100); + expect(agent?.applies).toBe(true); + expect(agent?.command).toBe("hack upgrade apply --only agent-integrations"); + expect(agent?.targets).toHaveLength(100); + const ignore = report.plan.find( + (s) => s.id === "ignore-rules" && s.status === "warn" + ); + expect(ignore?.applies).toBe(true); + expect(ignore?.count).toBeGreaterThan(90); + // A manual step over several checkouts names one command pattern. + const legacy = report.plan.find((s) => s.id === "env-format" && !s.blocker); + expect(legacy?.command).toBeNull(); + expect(legacy?.action).toContain( + "hack doctor --migrate-env-config --path " + ); + expect(report.plan.length).toBeLessThanOrEqual(10); + + const text = renderUpgradeCheck(report); + const lines = text.trimEnd().split("\n"); + expect(lines.length).toBeLessThanOrEqual(60); + expect(lines[0]).toBe( + "hack 5.0.0-next.17 (next channel; SOURCE BUILD, not an installed release)" + ); + // Dead entries are one line with the prune preview, never listed. + expect(text).toContain( + "12 dead registry entries (checkout or config gone): hack projects prune --dry-run" + ); + expect(text).not.toContain("old-03"); + // Worktrees fold under their project. + expect(text).toContain("app-07 (+3 worktrees): "); + const projectLines = lines.filter((line) => + /^ {2}(BLOCK|warn |UNCHK) app-/.test(line) + ); + expect(projectLines.length).toBeLessThanOrEqual(13); + expect(projectLines.some((line) => line.includes("(worktree"))).toBe(false); + expect(text).toContain("stale agent files, 100 checkouts (apply): "); + expect(text).toContain("Blocked by: app-07 (worktree wt1) env-format"); + expect(text).toContain("Next: hack upgrade snapshot"); + expect(text).not.toContain(SENTINEL); + expect(JSON.stringify(report)).not.toContain(SENTINEL); + + // --verbose has every checkout and the dead entries. + const verbose = renderUpgradeCheck(report, { verbose: true }); + expect(verbose).toContain("old-03"); + expect(verbose).toContain("Checkout app-07 (worktree wt1)"); + expect(verbose).not.toContain(SENTINEL); +}, 180_000); + +test("apply --dry-run changes nothing; apply snapshots, journals, refreshes .gitignore and skips blocked checkouts", async () => { + const agentFile = await staleHomeAgentFile(); + const a = await project("app-a", 1); + const b = await project("app-b"); + const blocked = await project("app-blocked"); + await writeFile(join(a.projectDir, ".env"), `TOKEN=${SENTINEL}\n`); + await symlink( + join(root, "nowhere.yaml"), + join(blocked.projectDir, "hack.env.default.yaml") + ); + await writeRegistry([a, b, blocked]); + + const report = await check(); + const before = await tree(root); + const dry = await runUpgradeApply({ + report, + dryRun: true, + home, + collect: collectUpgradeSnapshotTargets, + }); + expect(dry.status).toBe("dry-run"); + expect(await tree(root)).toEqual(before); + if (dry.status !== "dry-run") { + throw new Error("unreachable"); + } + const ignorePlanned = dry.targets.filter((t) => t.fix === "ignore-rules"); + expect(ignorePlanned.map((t) => `${t.scope}: ${t.status}`).sort()).toEqual([ + "app-a (worktree wt0): planned", + "app-a: planned", + "app-b: planned", + "app-blocked: skipped", + ]); + + // --project narrows to one project and its worktrees. + const one = await runUpgradeApply({ + report, + dryRun: true, + project: "app-a", + only: ["ignore-rules"], + home, + collect: collectUpgradeSnapshotTargets, + }); + expect( + one.status === "dry-run" ? one.targets.map((t) => t.scope).sort() : [] + ).toEqual(["app-a", "app-a (worktree wt0)"]); + expect( + await runUpgradeApply({ + report, + dryRun: true, + project: "nope", + home, + collect: collectUpgradeSnapshotTargets, + }) + ).toMatchObject({ status: "refused", reason: "unknown-project" }); + expect(await tree(root)).toEqual(before); + + const applied = await runUpgradeApply({ + report, + dryRun: false, + home, + collect: collectUpgradeSnapshotTargets, + agentBackupDir: join(hackHome, "agent-migrations"), + }); + if (applied.status !== "applied" && applied.status !== "partial") { + throw new Error(`apply did not run: ${applied.status}`); + } + const byKey = new Map( + applied.targets.map((t) => [`${t.fix} ${t.scope}`, t.status]) + ); + expect(byKey.get("ignore-rules app-a")).toBe("applied"); + expect(byKey.get("ignore-rules app-a (worktree wt0)")).toBe("applied"); + expect(byKey.get("ignore-rules app-b")).toBe("applied"); + expect(byKey.get("ignore-rules app-blocked")).toBe("skipped"); + // The blocked checkout was not touched. + expect( + await lstat(join(blocked.projectDir, ".gitignore")).catch(() => null) + ).toBeNull(); + const gitignore = join(a.projectDir, ".gitignore"); + expect(await readFile(gitignore, "utf8")).toContain(".env"); + // The stale agent file under HOME is never removed without a verified plugin. + expect(byKey.get("agent-integrations machine")).toMatch( + /^(refused|unchanged)$/ + ); + expect(await readFile(agentFile, "utf8")).toContain("old standalone"); + // A skipped checkout makes the run partial. + expect(applied.status).toBe("partial"); + + // The journal sits beside the snapshot manifest, private, with coverage. + const journalInfo = await stat(applied.journal_path); + expect(journalInfo.mode & 0o777).toBe(0o600); + const journal = (await readUpgradeSnapshotJournal({ + manifestPath: applied.coverage.manifest_path, + })) as { + kind: string; + state: string; + snapshot: { complete: boolean; gaps: unknown; files: number }; + targets: { scope: string; status: string }[]; + }; + expect(journal.kind).toBe("hack-upgrade-apply"); + expect(journal.state).toBe("failed"); + // The blocked checkout's linked env layer is not kept: a named gap. + expect(journal.snapshot.complete).toBe(false); + expect(journal.snapshot.gaps).toEqual(applied.coverage.gaps); + expect( + applied.coverage.gaps.some((gap) => gap.name.includes("app-blocked")) + ).toBe(true); + expect(journal.snapshot.files).toBe(applied.coverage.files); + expect(journal.targets).toHaveLength(applied.targets.length); + expect(journal.targets.some((t) => t.status === "planned")).toBe(false); + const journalText = await readFile(applied.journal_path, "utf8"); + expect(journalText).not.toContain(SENTINEL); + expect(JSON.stringify(applied)).not.toContain(SENTINEL); + + // The snapshot recorded what apply wrote, so rollback needs no --overwrite. + const restored = await restoreUpgradeSnapshot({ + manifestPath: applied.coverage.manifest_path, + overwrite: false, + }); + expect(restored.status).toBe("restored"); + expect(await lstat(gitignore).catch(() => null)).toBeNull(); +}, 120_000); + +test("a snapshot with gaps is recorded in the journal as not complete", async () => { + const a = await project("app-a"); + // A link at an env path the snapshot keeps: skipped, so a gap. + await symlink(join(root, "elsewhere"), join(a.projectDir, ".env")); + await writeRegistry([a]); + const report = await check(); + const applied = await runUpgradeApply({ + report, + dryRun: false, + only: ["ignore-rules"], + home, + collect: collectUpgradeSnapshotTargets, + }); + if (applied.status !== "applied") { + throw new Error(`apply did not run: ${applied.status}`); + } + expect(applied.coverage.complete).toBe(false); + expect(applied.coverage.gaps.length).toBeGreaterThan(0); + const journal = (await readUpgradeSnapshotJournal({ + manifestPath: applied.coverage.manifest_path, + })) as { snapshot: { complete: boolean; gaps: unknown } }; + expect(journal.snapshot.complete).toBe(false); + expect(journal.snapshot.gaps).toEqual(applied.coverage.gaps); +}, 60_000); + +test("--only refuses an unknown step before anything is written", async () => { + const a = await project("app-a"); + await writeRegistry([a]); + const report = await check(); + const before = await tree(root); + expect( + await runUpgradeApply({ + report, + dryRun: false, + only: ["env-format"], + home, + collect: collectUpgradeSnapshotTargets, + }) + ).toMatchObject({ status: "refused", reason: "unknown-fix" }); + expect(await tree(root)).toEqual(before); +}, 60_000); + +// --------------------------------------------------------------------------- +// CLI +// --------------------------------------------------------------------------- + +async function cli(args: readonly string[]) { + const child = Bun.spawn([process.execPath, ENTRYPOINT, ...args], { + cwd: root, + env: privateCliEnv({ + root, + home, + env: { + HACK_HOME: hackHome, + HACK_GLOBAL_CONFIG_PATH: join(hackHome, "hack.config.json"), + HACK_SYSTEMD_DISABLE: "1", + HACK_HOST_PROBES_DISABLE: "1", + HACK_SECRETS_DISABLE_KEYCHAIN_FALLBACK: "1", + HACK_NO_INTERACTIVE: "1", + }, + }), + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, code] = await Promise.all([ + new Response(child.stdout).text(), + new Response(child.stderr).text(), + child.exited, + ]); + return { code, stdout, stderr, output: `${stdout}${stderr}` }; +} + +test("hack upgrade apply: --dry-run lists, apply prints the journal and rollback, check names the source build", async () => { + const a = await project("app-a"); + await writeFile(join(a.projectDir, ".env"), `TOKEN=${SENTINEL}\n`); + await writeRegistry([a, dead("old-one")]); + + const checked = await cli(["upgrade", "check"]); + expect(checked.stdout.split("\n")[0]).toContain("SOURCE BUILD"); + expect(checked.stdout).toContain("1 dead registry entry"); + expect(checked.stdout).not.toContain("old-one"); + const verbose = await cli(["upgrade", "check", "--verbose"]); + expect(verbose.stdout).toContain("old-one"); + + const before = await tree(root); + const dry = await cli(["upgrade", "apply", "--dry-run"]); + expect(dry.code).toBe(0); + expect(dry.stdout).toContain(".hack/.gitignore: would change in 1 (app-a)"); + expect(await tree(root)).toEqual(before); + + const bad = await cli(["upgrade", "apply", "--only", "bogus", "--json"]); + expect(bad.code).toBe(1); + expect(JSON.parse(bad.stdout).data.reason).toBe("unknown-fix"); + + const applied = await cli([ + "upgrade", + "apply", + "--only", + "ignore-rules", + "--json", + ]); + expect(applied.code).toBe(0); + const data = JSON.parse(applied.stdout).data as { + status: string; + journal_path: string; + rollback_command: string; + coverage: { manifest_path: string; complete: boolean }; + after: { plan: { id: string }[] }; + }; + expect(data.status).toBe("applied"); + expect(data.rollback_command).toBe( + `hack upgrade rollback --rollback-manifest ${data.coverage.manifest_path}` + ); + expect(data.after.plan.some((s) => s.id === "ignore-rules")).toBe(false); + expect(applied.output).not.toContain(SENTINEL); + expect(await readFile(data.journal_path, "utf8")).not.toContain(SENTINEL); +}, 120_000); + +test("a .hack/.gitignore that is a link refuses only its own checkout", async () => { + const a = await project("app-a"); + const b = await project("app-b"); + await writeFile(join(root, "elsewhere-ignore"), "x\n"); + await symlink( + join(root, "elsewhere-ignore"), + join(a.projectDir, ".gitignore") + ); + await writeRegistry([a, b]); + const report = await check(); + const applied = await runUpgradeApply({ + report, + dryRun: false, + only: ["ignore-rules"], + home, + collect: collectUpgradeSnapshotTargets, + }); + if (applied.status !== "partial" && applied.status !== "applied") { + throw new Error(`apply did not run: ${applied.status}`); + } + const byScope = new Map(applied.targets.map((t) => [t.scope, t.status])); + expect(byScope.get("app-b")).toBe("applied"); + expect(byScope.get("app-a")).toMatch(/^(refused|failed|skipped)$/); + expect(await readFile(join(root, "elsewhere-ignore"), "utf8")).toBe("x\n"); +}, 60_000); diff --git a/tests/upgrade-check.test.ts b/tests/upgrade-check.test.ts index 42f319a52..ddb44247b 100644 --- a/tests/upgrade-check.test.ts +++ b/tests/upgrade-check.test.ts @@ -239,7 +239,7 @@ test("once the time budget is spent, project checks report not checked without r const project = await liveProject("app-late"); let ran = false; const findings = await inspectUpgradeProject({ - project, + project: { ...project, project: project.name }, homeDir: home, deadline: Date.now() - 1, nativePreflight: async () => { @@ -325,6 +325,7 @@ test("a host with nothing to do is ready, with no plan and no next command", asy version: "5.0.0-next.17", channel: "next", candidate_run: false, + build: "release", command: "hack", }); }); From f7f385c685faaa68e98f59f4e840cc72ff73a576 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 02:46:55 +0000 Subject: [PATCH 2/7] fix(upgrade): apply by checkout root, exit 1 when blocked, journal backups before removal (HACK-2220) Review fixes for hack upgrade apply: - Resolve each target's Hack directory and safety findings by checkout root, not display name (duplicate registrations or worktrees with the same directory name no longer cross over); --project naming several registered projects is refused as ambiguous. - When every selected checkout is skipped, the result is `blocked` and the command exits 1, not success. - With --project, HOME agent files are left out and noted instead of counted as a skipped target, so a successful scoped run exits 0. - A target is applied only once its after state is recorded; a refused, invalid or failed recording makes it failed with rollback guidance. - The plugin migration's backup manifest id is chosen up front and the journal marks the target `started` with that path before the migration can remove anything; after a failure the manifest is kept in the result and the journal. - --verbose shows each finding's manual action. Co-Authored-By: Claude Opus 5.5 --- src/commands/upgrade.ts | 18 +- src/lib/upgrade-apply.ts | 455 +++++++++++++++++++++----------- src/lib/upgrade-check-render.ts | 3 + tests/upgrade-apply.test.ts | 221 ++++++++++++++++ 4 files changed, 546 insertions(+), 151 deletions(-) diff --git a/src/commands/upgrade.ts b/src/commands/upgrade.ts index 98ecfaa56..dfa890e07 100644 --- a/src/commands/upgrade.ts +++ b/src/commands/upgrade.ts @@ -444,7 +444,13 @@ async function handleUpgradeApply({ } else { process.stdout.write(renderApply({ result, after, rollback })); } - return result.status === "refused" || result.status === "partial" ? 1 : 0; + // Exit 1 when selected work did not all get done: refused, blocked + // (every selected checkout skipped) or partial. + return result.status === "refused" || + result.status === "blocked" || + result.status === "partial" + ? 1 + : 0; } function applyJson(opts: { @@ -502,8 +508,9 @@ export function renderApply(opts: { function outcomeLines( result: Exclude ): string[] { + const notes = result.notes.map((note) => `Note: ${note}`); if (result.targets.length === 0) { - return ["Nothing for hack upgrade apply to do."]; + return ["Nothing for hack upgrade apply to do.", ...notes]; } const groups = new Map(); for (const target of result.targets) { @@ -524,7 +531,12 @@ function outcomeLines( lines.push(` ${first.message}`); } } - return lines; + if (result.status === "blocked") { + lines.push( + "Nothing ran: every selected checkout has a blocker or a check that could not be performed." + ); + } + return [...lines, ...notes]; } function recoveryLines(opts: { diff --git a/src/lib/upgrade-apply.ts b/src/lib/upgrade-apply.ts index f6d113020..4cd4029de 100644 --- a/src/lib/upgrade-apply.ts +++ b/src/lib/upgrade-apply.ts @@ -29,25 +29,28 @@ import { * Contract: * - Works from a fresh `hack upgrade check` report; never guesses. A * checkout with a blocker or an incomplete check is skipped (named, with - * why); `--project` and `--only` narrow the work and an unknown name is - * refused before anything is written. + * why); `--project` and `--only` narrow the work, and an unknown or + * ambiguous name is refused before anything is written. Checkouts are + * identified by their root, never by their display name. * - `dryRun` writes nothing and lists what would run. * - Otherwise it first keeps an upgrade snapshot of the files the upgrade * changes (the `hack upgrade snapshot` set, plus every file this run * writes). If the snapshot is refused, nothing is changed. A file this - * run writes is changed only when the snapshot holds it, and its after - * state is recorded right after the write, so `hack upgrade rollback` - * can tell the step's output from a later edit. + * run writes is changed only when the snapshot holds it, and a target is + * reported applied only once its after state is recorded, so + * `hack upgrade rollback` can tell the step's output from a later edit. * - A journal beside the snapshot manifest records the run: the snapshot's - * coverage (`complete`, `gaps`, file count), each target's outcome and - * any backup manifest it made. It is rewritten atomically before the - * first change and after every target, so an interrupted run says how - * far it got. Paths, ids, statuses, counts and closed messages only: - * never a value. + * coverage (`complete`, `gaps`, file count) and each target's outcome. It + * is rewritten atomically before the first change, when each target + * starts (`started`, with the backup manifest it will write, before the + * migration can remove anything) and when it ends. After an interruption + * a `started` target may have changed files; `planned` ones have not. + * Paths, ids, statuses, counts and closed messages only: never a value. * - Stale agent files are removed only by the plugin migration, which backs * up each file to its own manifest first and refuses when the hack plugin * that replaces them is not verified. Machine-wide agent files (under - * HOME) are migrated only without `--project`. + * HOME) are migrated only without `--project`; with it they are left out + * and noted, not counted as unfinished work. * - Nothing is deleted that current resolution reads: `.hack/.gitignore` is * merged (never shrunk), and the migration removes only Hack-owned * standalone files the verified plugin replaces. @@ -57,17 +60,19 @@ export const UPGRADE_APPLY_JOURNAL_VERSION = 1; export const UPGRADE_APPLY_JOURNAL_KIND = "hack-upgrade-apply"; export type UpgradeApplyTargetStatus = - /** Would run (dry run). */ + /** Would run (dry run), or not reached yet (journal). */ | "planned" - /** Changed. */ + /** In progress (journal only): it may have changed files. */ + | "started" + /** Changed, and the change is recorded for rollback. */ | "applied" /** Already current; nothing written. */ | "unchanged" - /** Not run: the checkout has a blocker or an incomplete check, or it is out of scope. */ + /** Not run: the checkout has a blocker or an incomplete check. */ | "skipped" /** Not run: a safety condition refused it (plugin not verified, a link, a read-only file, not in the snapshot). */ | "refused" - /** Started and failed. */ + /** Started and failed, or changed without a recorded after state. */ | "failed"; export type UpgradeApplyTarget = { @@ -79,7 +84,7 @@ export type UpgradeApplyTarget = { readonly status: UpgradeApplyTargetStatus; /** A closed explanation; never a value. */ readonly message: string | null; - /** The plugin migration's backup manifest, when it removed files. */ + /** The plugin migration's backup manifest: set before it can remove a file. */ readonly backup_manifest: string | null; }; @@ -105,23 +110,35 @@ export type UpgradeApplyJournal = { }; readonly snapshot: UpgradeApplyCoverage; readonly targets: readonly UpgradeApplyTarget[]; + readonly notes: readonly string[]; }; export type UpgradeApplyResult = | { /** Refused before anything was written. */ readonly status: "refused"; - readonly reason: "unknown-project" | "unknown-fix" | "snapshot"; + readonly reason: + | "unknown-project" + | "ambiguous-project" + | "unknown-fix" + | "snapshot"; readonly message: string; } | { - readonly status: "nothing-to-do" | "dry-run"; + /** + * `nothing-to-do`: no selected work. `blocked`: every selected target + * was skipped (a blocker or an incomplete check), so nothing ran. + */ + readonly status: "nothing-to-do" | "dry-run" | "blocked"; readonly targets: readonly UpgradeApplyTarget[]; + /** Work left out on purpose (for example HOME files with `--project`). */ + readonly notes: readonly string[]; } | { /** `applied`: every selected target is applied or unchanged. */ readonly status: "applied" | "partial"; readonly targets: readonly UpgradeApplyTarget[]; + readonly notes: readonly string[]; readonly coverage: UpgradeApplyCoverage; readonly journal_path: string; readonly agent_backups: readonly string[]; @@ -146,10 +163,12 @@ type Planned = { const ERROR_CODE = /^[A-Z0-9_]{1,40}$/; const SKIP_BLOCKED = "Not run: this checkout has a blocker or a check that could not be performed; fix that first (hack upgrade check lists it)."; -const SKIP_MACHINE_WITH_PROJECT = - "Not run with --project: these agent files are under HOME and serve every project; run hack upgrade apply --only agent-integrations without --project."; +const NOTE_HOME_WITH_PROJECT = + "Agent files under HOME serve every project and are left out with --project; run hack upgrade apply --only agent-integrations without --project for them."; const NOT_IN_SNAPSHOT = "Not changed: the file is not in this run's snapshot, so a rollback could not restore it."; +const NOT_RECORDED = + "Changed, but the snapshot could not record what was written; hack upgrade rollback needs --overwrite for this file."; /** * Runs (or with `dryRun` lists) the apply steps of `report`. `collect` @@ -167,28 +186,22 @@ export async function runUpgradeApply(input: { readonly snapshotRoot?: string; readonly agentBackupDir?: string; readonly now?: () => Date; + readonly applyPluginMigration?: typeof applyPluginMigration; + readonly recordAfter?: typeof recordUpgradeSnapshotAfter; }): Promise { const only = input.only ?? []; - const unknownFix = only.find((id) => !isUpgradeApplyFix(id)); - if (unknownFix !== undefined) { - return { - status: "refused", - reason: "unknown-fix", - message: `--only takes ${UPGRADE_APPLY_FIXES.join(" or ")}; nothing was changed.`, - }; - } - const known = new Set(input.report.projects.map((p) => p.project)); - if (input.project !== undefined && !known.has(input.project)) { - return { - status: "refused", - reason: "unknown-project", - message: - "No live registered project has that name; nothing was changed. hack upgrade check lists them.", - }; + const refusal = selectionRefusal({ + report: input.report, + only, + project: input.project, + }); + if (refusal !== null) { + return refusal; } - const planned = planTargets({ + const fixes = only.filter(isUpgradeApplyFix); + const { planned, notes } = planTargets({ report: input.report, - only: only.filter(isUpgradeApplyFix), + only: fixes, project: input.project ?? null, home: input.home, }); @@ -196,28 +209,21 @@ export async function runUpgradeApply(input: { const skipped = planned .filter((target) => target.skip !== null) .map((target) => outcome(target, "skipped", target.skip)); - if (input.dryRun) { - return { - status: - runnable.length === 0 && skipped.length === 0 - ? "nothing-to-do" - : "dry-run", - targets: [ - ...runnable.map((target) => outcome(target, "planned", null)), - ...skipped, - ], - }; - } - if (runnable.length === 0) { - return { status: "nothing-to-do", targets: skipped }; + const early = resultBeforeSnapshot({ + dryRun: input.dryRun, + runnable, + skipped, + notes, + }); + if (early !== null) { + return early; } // The snapshot comes first: nothing is written unless it is kept. const collected = await input.collect(); - const targets = await snapshotTargets({ runnable, collected }); const created = await createUpgradeSnapshot({ purpose: "hack upgrade apply", - targets, + targets: await snapshotTargets({ runnable, collected }), root: input.snapshotRoot, now: input.now?.(), }); @@ -243,47 +249,65 @@ export async function runUpgradeApply(input: { const startedAt = now(); const results: UpgradeApplyTarget[] = [...skipped]; const journal = ( - state: UpgradeApplyJournal["state"] - ): UpgradeApplyJournal => ({ - version: UPGRADE_APPLY_JOURNAL_VERSION, - kind: UPGRADE_APPLY_JOURNAL_KIND, - started_at: startedAt, - updated_at: now(), - state, - cli_version: input.report.cli.version, - filters: { - only: only.filter(isUpgradeApplyFix), - project: input.project ?? null, - }, - snapshot: coverage, - targets: [ - ...results, - ...runnable - .slice(results.length - skipped.length) - .map((target) => outcome(target, "planned", null)), - ], - }); + state: UpgradeApplyJournal["state"], + current: UpgradeApplyTarget | null + ): UpgradeApplyJournal => { + const done = results.length - skipped.length; + const pending = runnable + .slice(done + (current === null ? 0 : 1)) + .map((target) => outcome(target, "planned", null)); + return { + version: UPGRADE_APPLY_JOURNAL_VERSION, + kind: UPGRADE_APPLY_JOURNAL_KIND, + started_at: startedAt, + updated_at: now(), + state, + cli_version: input.report.cli.version, + filters: { only: fixes, project: input.project ?? null }, + snapshot: coverage, + targets: [...results, ...(current === null ? [] : [current]), ...pending], + notes, + }; + }; // Commit point: the journal names the snapshot before the first change. const journalPath = await writeUpgradeSnapshotJournal({ snapshot, - journal: journal("applying"), + journal: journal("applying", null), }); const inSnapshot = new Set(snapshot.manifest.entries.map((e) => e.path)); - for (const target of runnable) { + const backupDir = + input.agentBackupDir ?? join(resolveGlobalHackDir(), "agent-migrations"); + for (const [index, target] of runnable.entries()) { + const backup = + target.fix === "agent-integrations" + ? { + id: `upgrade-apply-${snapshot.id}-${index}`, + path: join(backupDir, `upgrade-apply-${snapshot.id}-${index}.json`), + } + : null; + // Before anything can change: this target started, with its backup. + await writeUpgradeSnapshotJournal({ + snapshot, + journal: journal("applying", { + ...outcome(target, "started", null), + backup_manifest: backup?.path ?? null, + }), + }); results.push( await applyTarget({ target, snapshot, inSnapshot, home: input.home, - agentBackupDir: - input.agentBackupDir ?? - join(resolveGlobalHackDir(), "agent-migrations"), + backupDir, + backup, + migrate: input.applyPluginMigration ?? applyPluginMigration, + recordAfter: input.recordAfter ?? recordUpgradeSnapshotAfter, }) ); await writeUpgradeSnapshotJournal({ snapshot, - journal: journal("applying"), + journal: journal("applying", null), }); } const done = results.every( @@ -291,11 +315,12 @@ export async function runUpgradeApply(input: { ); await writeUpgradeSnapshotJournal({ snapshot, - journal: journal(done ? "applied" : "failed"), + journal: journal(done ? "applied" : "failed", null), }); return { status: done ? "applied" : "partial", targets: results, + notes, coverage, journal_path: journalPath, agent_backups: results.flatMap((r) => @@ -304,18 +329,97 @@ export async function runUpgradeApply(input: { }; } +/** The dry-run, nothing-to-do and all-blocked results; null when there is work to run. */ +function resultBeforeSnapshot(opts: { + readonly dryRun: boolean; + readonly runnable: readonly Planned[]; + readonly skipped: readonly UpgradeApplyTarget[]; + readonly notes: readonly string[]; +}): Extract< + UpgradeApplyResult, + { status: "dry-run" | "nothing-to-do" | "blocked" } +> | null { + const { runnable, skipped, notes } = opts; + if (opts.dryRun) { + return { + status: + runnable.length + skipped.length === 0 ? "nothing-to-do" : "dry-run", + targets: [ + ...runnable.map((target) => outcome(target, "planned", null)), + ...skipped, + ], + notes, + }; + } + if (runnable.length > 0) { + return null; + } + return { + status: skipped.length === 0 ? "nothing-to-do" : "blocked", + targets: skipped, + notes, + }; +} + +/** An unknown `--only`, or an unknown or ambiguous `--project`; null when the selection is valid. */ +function selectionRefusal(opts: { + readonly report: UpgradeCheckReport; + readonly only: readonly string[]; + readonly project: string | undefined; +}): Extract | null { + if (opts.only.some((id) => !isUpgradeApplyFix(id))) { + return { + status: "refused", + reason: "unknown-fix", + message: `--only takes ${UPGRADE_APPLY_FIXES.join(" or ")}; nothing was changed.`, + }; + } + if (opts.project === undefined) { + return null; + } + const checkouts = opts.report.projects.filter( + (p) => p.project === opts.project + ); + if (checkouts.length === 0) { + return { + status: "refused", + reason: "unknown-project", + message: + "No live registered project has that name; nothing was changed. hack upgrade check lists them.", + }; + } + // A project's primary checkout carries the project's own name. + const primaries = new Set( + checkouts.filter((p) => p.name === p.project).map((p) => p.root) + ); + if (primaries.size > 1) { + return { + status: "refused", + reason: "ambiguous-project", + message: + "Several registered projects have that name; nothing was changed. Run without --project, or rename one of them.", + }; + } + return null; +} + function planTargets(opts: { readonly report: UpgradeCheckReport; readonly only: readonly UpgradeApplyFix[]; readonly project: string | null; readonly home: string; -}): Planned[] { +}): { readonly planned: Planned[]; readonly notes: string[] } { const wanted = new Set( opts.only.length > 0 ? opts.only : UPGRADE_APPLY_FIXES ); - const unsafe = unsafeCheckouts(opts.report); - const byName = new Map(opts.report.projects.map((p) => [p.name, p])); - const out: Planned[] = []; + const unsafe = unsafeRoots(opts.report); + // By root: display names are not unique (duplicate registrations, + // worktrees with the same directory name). + const byRoot = new Map( + opts.report.projects.map((p) => [normalize(p.root), p]) + ); + const planned: Planned[] = []; + const notes: string[] = []; for (const fix of UPGRADE_APPLY_FIXES) { const step = wanted.has(fix) ? opts.report.plan.find((c) => c.id === fix && c.applies) @@ -325,35 +429,44 @@ function planTargets(opts: { } if (fix === "agent-integrations") { // Files under HOME make every checkout stale; they are migrated once. - out.push({ + if (opts.project === null) { + planned.push({ + fix, + scope: "machine", + project: null, + root: opts.home, + projectDir: null, + skip: null, + }); + } else { + notes.push(NOTE_HOME_WITH_PROJECT); + } + } + const seen = new Set(); + for (const target of step.targets) { + const root = normalize(target.root); + if ( + seen.has(root) || + (opts.project !== null && target.project !== opts.project) + ) { + continue; + } + seen.add(root); + planned.push({ fix, - scope: "machine", - project: null, - root: opts.home, - projectDir: null, - skip: opts.project === null ? null : SKIP_MACHINE_WITH_PROJECT, + scope: target.scope, + project: target.project, + root: target.root, + projectDir: byRoot.get(root)?.project_dir ?? null, + skip: unsafe.has(root) ? SKIP_BLOCKED : null, }); } - out.push( - ...step.targets - .filter((t) => opts.project === null || t.project === opts.project) - .map( - (target): Planned => ({ - fix, - scope: target.scope, - project: target.project, - root: target.root, - projectDir: byName.get(target.scope)?.project_dir ?? null, - skip: unsafe.has(target.scope) ? SKIP_BLOCKED : null, - }) - ) - ); } - return out; + return { planned, notes }; } -/** Checkouts with a blocker or a check that could not be performed. */ -function unsafeCheckouts(report: UpgradeCheckReport): ReadonlySet { +/** Roots of checkouts with a blocker or a check that could not be performed. */ +function unsafeRoots(report: UpgradeCheckReport): ReadonlySet { return new Set( report.projects .filter((project) => @@ -361,7 +474,7 @@ function unsafeCheckouts(report: UpgradeCheckReport): ReadonlySet { (f) => f.status === "blocker" || f.status === "incomplete" ) ) - .map((project) => project.name) + .map((project) => normalize(project.root)) ); } @@ -370,49 +483,19 @@ async function applyTarget(opts: { readonly snapshot: UpgradeSnapshot; readonly inSnapshot: ReadonlySet; readonly home: string; - readonly agentBackupDir: string; + readonly backupDir: string; + readonly backup: { readonly id: string; readonly path: string } | null; + readonly migrate: typeof applyPluginMigration; + readonly recordAfter: typeof recordUpgradeSnapshotAfter; }): Promise { const { target } = opts; if (target.fix === "ignore-rules") { - if (target.projectDir === null) { - return outcome( - target, - "refused", - "Not changed: the checkout's Hack directory is unknown." - ); - } - const path = gitignorePath(target.projectDir); - if (!opts.inSnapshot.has(path)) { - return outcome(target, "refused", NOT_IN_SNAPSHOT); - } - let result: Awaited> | null = - null; - let failure: unknown = null; - try { - result = await ensureHackDirGitignore({ projectDir: target.projectDir }); - } catch (error) { - failure = error; - } - // Whatever is there now is this step's output, even after a failure. - await recordUpgradeSnapshotAfter({ - manifestPath: opts.snapshot.manifestPath, - paths: [path], - }); - if (result === null) { - return outcome(target, "failed", closedError(failure)); - } - if (result.readOnly) { - return outcome( - target, - "refused", - "Not changed: .hack/.gitignore is read-only; make it writable or update it yourself." - ); - } - return outcome(target, result.changed ? "applied" : "unchanged", null); + return await applyIgnoreRules(opts); } + const backup = opts.backup as { readonly id: string; readonly path: string }; let migrated: PluginMigrationApplyResult; try { - migrated = await applyPluginMigration({ + migrated = await opts.migrate({ scopes: [ target.projectDir === null ? { scope: "global", root: opts.home } @@ -421,14 +504,80 @@ async function applyTarget(opts: { home: opts.home, clients: MIGRATION_CLIENTS, skipPluginCheck: [], - backupDir: opts.agentBackupDir, + backupDir: opts.backupDir, + hooks: { id: backup.id }, }); } catch (error) { - return outcome(target, "failed", closedError(error)); + // Files may already be removed: keep the backup's name when it exists. + const kept = (await exists(backup.path)) ? backup.path : null; + return { + ...outcome( + target, + "failed", + `${closedError(error)}${kept === null ? "" : " Files it removed are kept in the backup manifest."}` + ), + backup_manifest: kept, + }; } return agentOutcome({ target, migrated }); } +async function applyIgnoreRules(opts: { + readonly target: Planned; + readonly snapshot: UpgradeSnapshot; + readonly inSnapshot: ReadonlySet; + readonly recordAfter: typeof recordUpgradeSnapshotAfter; +}): Promise { + const { target } = opts; + if (target.projectDir === null) { + return outcome( + target, + "refused", + "Not changed: the checkout's Hack directory is unknown." + ); + } + const path = gitignorePath(target.projectDir); + if (!opts.inSnapshot.has(path)) { + return outcome(target, "refused", NOT_IN_SNAPSHOT); + } + let result: Awaited> | null = null; + let failure: unknown = null; + try { + result = await ensureHackDirGitignore({ projectDir: target.projectDir }); + } catch (error) { + failure = error; + } + // Whatever is there now is this step's output, even after a failure. + let recorded = false; + try { + const record = await opts.recordAfter({ + manifestPath: opts.snapshot.manifestPath, + paths: [path], + }); + recorded = record.status === "recorded"; + } catch { + recorded = false; + } + if (result === null) { + return outcome( + target, + "failed", + `${closedError(failure)}${recorded ? "" : ` ${NOT_RECORDED}`}` + ); + } + if (result.changed && !recorded) { + return outcome(target, "failed", NOT_RECORDED); + } + if (result.readOnly) { + return outcome( + target, + "refused", + "Not changed: .hack/.gitignore is read-only; make it writable or update it yourself." + ); + } + return outcome(target, result.changed ? "applied" : "unchanged", null); +} + function agentOutcome(opts: { readonly target: Planned; readonly migrated: PluginMigrationApplyResult; @@ -509,6 +658,7 @@ async function snapshotTargets(opts: { } const path = gitignorePath(target.projectDir); if (!collectedPaths.has(path) && (await snapshotable(path))) { + collectedPaths.add(path); extra.push({ path, label: `project ${target.scope}: .hack/.gitignore` }); } } @@ -524,6 +674,15 @@ async function snapshotable(path: string): Promise { } } +async function exists(path: string): Promise { + try { + await lstat(path); + return true; + } catch { + return false; + } +} + function gitignorePath(projectDir: string): string { return normalize(resolve(projectDir, ".gitignore")); } diff --git a/src/lib/upgrade-check-render.ts b/src/lib/upgrade-check-render.ts index f6018b4e9..5e1ab7d59 100644 --- a/src/lib/upgrade-check-render.ts +++ b/src/lib/upgrade-check-render.ts @@ -195,6 +195,9 @@ function verboseLines(report: UpgradeCheckReport): string[] { if (NEEDS_WORK.has(finding.status) && finding.command !== null) { lines.push(` ${finding.command}`); } + if (NEEDS_WORK.has(finding.status) && finding.action !== null) { + lines.push(` manual: ${finding.action}`); + } } } return lines; diff --git a/tests/upgrade-apply.test.ts b/tests/upgrade-apply.test.ts index 50e58873f..70a1b8223 100644 --- a/tests/upgrade-apply.test.ts +++ b/tests/upgrade-apply.test.ts @@ -23,6 +23,7 @@ import { import type { UpgradeCheckProbes } from "../src/lib/upgrade-check-probes.ts"; import { renderUpgradeCheck } from "../src/lib/upgrade-check-render.ts"; import { + listUpgradeSnapshots, readUpgradeSnapshotJournal, restoreUpgradeSnapshot, } from "../src/lib/upgrade-snapshot.ts"; @@ -576,3 +577,223 @@ test("a .hack/.gitignore that is a link refuses only its own checkout", async () expect(byScope.get("app-a")).toMatch(/^(refused|failed|skipped)$/); expect(await readFile(join(root, "elsewhere-ignore"), "utf8")).toBe("x\n"); }, 60_000); + +// --------------------------------------------------------------------------- +// Review regressions +// --------------------------------------------------------------------------- + +test("checkouts with the same display name are applied by their own root", async () => { + const a = await project("app-a"); + // Two linked worktrees with the same directory name: the same display name. + const first = join(root, "wt-one", "feature"); + const second = join(root, "wt-two", "feature"); + await checkout(first, "app-a"); + await checkout(second, "app-a"); + // The second one is blocked; the first must still get its own fix. + await symlink( + join(root, "nowhere.yaml"), + join(second, ".hack", "hack.env.default.yaml") + ); + const lastSeenAt = "2026-01-01T00:00:00.000Z"; + await writeRegistry([ + { + ...a, + worktrees: [ + { path: first, branch: "f1", lastSeenAt }, + { path: second, branch: "f2", lastSeenAt }, + ], + }, + ]); + const report = await check(); + const applied = await runUpgradeApply({ + report, + dryRun: false, + only: ["ignore-rules"], + home, + collect: collectUpgradeSnapshotTargets, + }); + if (applied.status !== "partial") { + throw new Error(`unexpected status ${applied.status}`); + } + const byRoot = new Map(applied.targets.map((t) => [t.root, t.status])); + expect(byRoot.get(first)).toBe("applied"); + expect(byRoot.get(second)).toBe("skipped"); + expect(await readFile(join(first, ".hack", ".gitignore"), "utf8")).toContain( + ".env" + ); + expect( + await lstat(join(second, ".hack", ".gitignore")).catch(() => null) + ).toBeNull(); +}, 60_000); + +test("--project naming two registered projects is refused as ambiguous", async () => { + const one = await project("dup"); + const other = { + ...(await project("dup-2")), + id: "id-dup-other", + name: "dup", + }; + await writeRegistry([one, other]); + const report = await check(); + const before = await tree(root); + expect( + await runUpgradeApply({ + report, + dryRun: false, + project: "dup", + home, + collect: collectUpgradeSnapshotTargets, + }) + ).toMatchObject({ status: "refused", reason: "ambiguous-project" }); + expect(await tree(root)).toEqual(before); +}, 60_000); + +test("--project leaves HOME agent files out without counting them as unfinished", async () => { + const agentFile = await staleHomeAgentFile(); + const a = await project("app-a"); + await writeRegistry([a]); + const report = await check(); + const scopes: string[] = []; + const applied = await runUpgradeApply({ + report, + dryRun: false, + project: "app-a", + only: ["agent-integrations"], + home, + collect: collectUpgradeSnapshotTargets, + applyPluginMigration: async (input) => { + scopes.push(...input.scopes.map((s) => s.scope)); + return { + status: "nothing-to-migrate", + plan: { checks: [], entries: [] }, + }; + }, + }); + expect(applied.status).toBe("applied"); + expect(scopes).toEqual(["project"]); + if (applied.status !== "applied") { + throw new Error("unreachable"); + } + expect(applied.targets.map((t) => t.scope)).toEqual(["app-a"]); + expect(applied.notes[0]).toContain("left out with --project"); + expect(await readFile(agentFile, "utf8")).toContain("old standalone"); +}, 60_000); + +test("a write whose after state cannot be recorded is failed, not applied", async () => { + const a = await project("app-a"); + await writeRegistry([a]); + const report = await check(); + for (const record of [ + async () => ({ status: "refused" as const, paths: [] }), + async () => ({ status: "invalid-manifest" as const, message: "x" }), + async () => { + throw new Error("boom"); + }, + ]) { + await rm(join(a.projectDir, ".gitignore"), { force: true }); + const applied = await runUpgradeApply({ + report, + dryRun: false, + only: ["ignore-rules"], + home, + collect: collectUpgradeSnapshotTargets, + recordAfter: record, + }); + if (applied.status !== "partial") { + throw new Error(`unexpected status ${applied.status}`); + } + expect(applied.targets[0]?.status).toBe("failed"); + expect(applied.targets[0]?.message).toContain("needs --overwrite"); + const journal = (await readUpgradeSnapshotJournal({ + manifestPath: applied.coverage.manifest_path, + })) as { state: string }; + expect(journal.state).toBe("failed"); + } +}, 60_000); + +test("the agent backup manifest is journaled before removal and kept when the migration fails after it", async () => { + await staleHomeAgentFile(); + const a = await project("app-a"); + await writeRegistry([a]); + const report = await check(); + const backupDir = join(hackHome, "agent-migrations"); + const snapshotRoot = join(hackHome, "upgrade", "snapshots"); + const seenDuring: (string | null)[] = []; + const applied = await runUpgradeApply({ + report, + dryRun: false, + only: ["agent-integrations"], + home, + collect: collectUpgradeSnapshotTargets, + agentBackupDir: backupDir, + snapshotRoot, + applyPluginMigration: async (input) => { + // Mid-migration (an interruption here): the journal already names + // this target as started, with the backup it is about to write. + const [snapshot] = await listUpgradeSnapshots({ root: snapshotRoot }); + const journal = (await readUpgradeSnapshotJournal({ + manifestPath: snapshot?.manifestPath ?? "", + })) as { + targets: { status: string; backup_manifest: string | null }[]; + }; + const current = journal.targets.find((t) => t.status === "started"); + seenDuring.push(current?.backup_manifest ?? null); + expect(current?.backup_manifest).toBe( + join(input.backupDir, `${input.hooks?.id}.json`) + ); + // The migration committed its backup and removed files, then failed. + await mkdir(input.backupDir, { recursive: true }); + await writeFile(join(input.backupDir, `${input.hooks?.id}.json`), "{}\n"); + throw Object.assign(new Error("rewrite failed"), { code: "EIO" }); + }, + }); + // manifestPath is only known after the run; replay the check here. + if (applied.status !== "partial") { + throw new Error(`unexpected status ${applied.status}`); + } + const manifestPath = applied.coverage.manifest_path; + const failed = applied.targets.filter((t) => t.status === "failed"); + expect(failed.length).toBeGreaterThan(0); + for (const target of failed) { + expect(target.backup_manifest).toStartWith(backupDir); + expect(target.message).toContain("Failed (EIO)"); + } + expect(applied.agent_backups).toHaveLength(failed.length); + const journal = (await readUpgradeSnapshotJournal({ manifestPath })) as { + targets: { status: string; backup_manifest: string | null }[]; + }; + expect( + journal.targets + .filter((t) => t.status === "failed") + .every((t) => t.backup_manifest?.startsWith(backupDir)) + ).toBe(true); + expect(seenDuring).toEqual(failed.map((t) => t.backup_manifest)); +}, 60_000); + +test("hack upgrade apply exits 1 when every selected checkout is blocked", async () => { + const blocked = await project("app-blocked"); + await symlink( + join(root, "nowhere.yaml"), + join(blocked.projectDir, "hack.env.default.yaml") + ); + await writeRegistry([blocked]); + // The CLI harness itself prepares the private home; compare what apply could touch. + const touched = async () => [ + ...(await tree(join(root, "repos"))), + ...(await tree(hackHome)), + ]; + const before = await touched(); + const run = await cli(["upgrade", "apply", "--only", "ignore-rules"]); + expect(run.code).toBe(1); + expect(run.stdout).toContain("Nothing ran"); + const json = await cli([ + "upgrade", + "apply", + "--only", + "ignore-rules", + "--json", + ]); + expect(json.code).toBe(1); + expect(JSON.parse(json.stdout).data.status).toBe("blocked"); + expect(await touched()).toEqual(before); +}, 120_000); From de9d4d2d26f6f291dfc1d1dbc7b0e819b31c79e6 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 02:59:36 +0000 Subject: [PATCH 3/7] fix(upgrade): count dead primaries as registrations; keep an uninspectable backup path (HACK-2220) - --project is ambiguous when the name matches more than one registration, counting primaries that are dead or not checked (their worktrees can still be live), not only live ones. - After a failed agent migration, the precommitted backup manifest path is kept unless it is confirmed absent; an unreadable path is not absence. Co-Authored-By: Claude Opus 5.5 --- src/lib/upgrade-apply.ts | 21 ++++++---- tests/upgrade-apply.test.ts | 78 +++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 7 deletions(-) diff --git a/src/lib/upgrade-apply.ts b/src/lib/upgrade-apply.ts index 4cd4029de..1578c42b7 100644 --- a/src/lib/upgrade-apply.ts +++ b/src/lib/upgrade-apply.ts @@ -388,9 +388,14 @@ function selectionRefusal(opts: { "No live registered project has that name; nothing was changed. hack upgrade check lists them.", }; } - // A project's primary checkout carries the project's own name. + // A registration is identified by its primary checkout, which carries + // the project's own name, whether that checkout is live, unreadable, + // dead or cut short (its worktrees can be live either way). + const { missing, unchecked } = opts.report.registry; const primaries = new Set( - checkouts.filter((p) => p.name === p.project).map((p) => p.root) + [...checkouts, ...missing, ...unchecked] + .filter((p) => p.project === opts.project && p.name === p.project) + .map((p) => normalize(p.root)) ); if (primaries.size > 1) { return { @@ -508,8 +513,9 @@ async function applyTarget(opts: { hooks: { id: backup.id }, }); } catch (error) { - // Files may already be removed: keep the backup's name when it exists. - const kept = (await exists(backup.path)) ? backup.path : null; + // Files may already be removed: keep the backup's name unless it is + // confirmed absent (an unreadable path is not absence). + const kept = (await confirmedAbsent(backup.path)) ? null : backup.path; return { ...outcome( target, @@ -674,12 +680,13 @@ async function snapshotable(path: string): Promise { } } -async function exists(path: string): Promise { +/** True only when `path` is known not to exist (ENOENT); any other error is unknown. */ +async function confirmedAbsent(path: string): Promise { try { await lstat(path); - return true; - } catch { return false; + } catch (error) { + return (error as { code?: unknown }).code === "ENOENT"; } } diff --git a/tests/upgrade-apply.test.ts b/tests/upgrade-apply.test.ts index 70a1b8223..c653f430f 100644 --- a/tests/upgrade-apply.test.ts +++ b/tests/upgrade-apply.test.ts @@ -1,6 +1,7 @@ import { afterEach, beforeEach, expect, test } from "bun:test"; import { createHash } from "node:crypto"; import { + chmod, lstat, mkdir, mkdtemp, @@ -797,3 +798,80 @@ test("hack upgrade apply exits 1 when every selected checkout is blocked", async expect(JSON.parse(json.stdout).data.status).toBe("blocked"); expect(await touched()).toEqual(before); }, 120_000); + +test("--project is ambiguous when another registration's primary is dead but its worktree lives", async () => { + const live = await project("dup"); + const worktree = join(root, "other-wt", "feature"); + await checkout(worktree, "dup"); + const other = { + ...dead("dup"), + id: "id-dup-dead", + worktrees: [ + { path: worktree, branch: "f", lastSeenAt: "2026-01-01T00:00:00.000Z" }, + ], + }; + await writeRegistry([live, other]); + const report = await check(); + // The dead primary is not a live checkout, but its worktree is. + expect(report.projects.map((p) => p.root).sort()).toEqual( + [live.repoRoot, worktree].sort() + ); + const before = await tree(root); + expect( + await runUpgradeApply({ + report, + dryRun: false, + project: "dup", + home, + collect: collectUpgradeSnapshotTargets, + }) + ).toMatchObject({ status: "refused", reason: "ambiguous-project" }); + expect(await tree(root)).toEqual(before); +}, 60_000); + +test("a backup that cannot be inspected after a failed migration keeps its journaled path", async () => { + await staleHomeAgentFile(); + const a = await project("app-a"); + await writeRegistry([a]); + const report = await check(); + const backupDir = join(hackHome, "agent-migrations"); + try { + const applied = await runUpgradeApply({ + report, + dryRun: false, + only: ["agent-integrations"], + home, + collect: collectUpgradeSnapshotTargets, + agentBackupDir: backupDir, + applyPluginMigration: async (input) => { + await mkdir(input.backupDir, { recursive: true }); + await writeFile( + join(input.backupDir, `${input.hooks?.id}.json`), + "{}\n" + ); + // The backup directory becomes unreadable: existence is unknown. + await chmod(input.backupDir, 0o000); + throw Object.assign(new Error("rewrite failed"), { code: "EIO" }); + }, + }); + if (applied.status !== "partial") { + throw new Error(`unexpected status ${applied.status}`); + } + const failed = applied.targets.filter((t) => t.status === "failed"); + expect(failed.length).toBeGreaterThan(0); + expect(failed.every((t) => t.backup_manifest?.startsWith(backupDir))).toBe( + true + ); + expect(applied.agent_backups).toHaveLength(failed.length); + const journal = (await readUpgradeSnapshotJournal({ + manifestPath: applied.coverage.manifest_path, + })) as { targets: { status: string; backup_manifest: string | null }[] }; + expect( + journal.targets + .filter((t) => t.status === "failed") + .every((t) => t.backup_manifest?.startsWith(backupDir)) + ).toBe(true); + } finally { + await chmod(backupDir, 0o700).catch(() => undefined); + } +}, 60_000); From e0dd5f6ed2b0a99a57a21717ef3e60e77e5e80a4 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 03:34:28 +0000 Subject: [PATCH 4/7] feat(upgrade): resume snapshot-backed apply and restore agent files together --- docs/reference/cli.md | 4 +- docs/reference/upgrade-apply.mmd | 20 ++ docs/reference/upgrade-apply.svg | 66 ++++ docs/reference/v5-model-coverage.md | 20 ++ docs/versions/v5/guides/upgrade-from-v4.md | 27 +- src/agents/plugin-migration.ts | 56 ++- src/commands/upgrade.ts | 59 +++- src/lib/private-file.ts | 9 +- src/lib/project-env-config.ts | 11 +- src/lib/upgrade-apply-recovery.ts | 238 +++++++++++++ src/lib/upgrade-apply.ts | 382 ++++++++++++++++++--- src/lib/upgrade-check-render.ts | 30 +- src/lib/upgrade-plan.ts | 8 +- src/lib/upgrade-snapshot.ts | 35 ++ tests/fixtures/upgrade-apply-worker.ts | 26 ++ tests/upgrade-apply.test.ts | 264 +++++++++++++- tests/upgrade-check.test.ts | 21 +- 17 files changed, 1197 insertions(+), 79 deletions(-) create mode 100644 docs/reference/upgrade-apply.mmd create mode 100644 docs/reference/upgrade-apply.svg create mode 100644 src/lib/upgrade-apply-recovery.ts create mode 100644 tests/fixtures/upgrade-apply-worker.ts diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 4ae3e9ef2..2e6c63df3 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -1934,10 +1934,11 @@ hack upgrade apply [options] Checks first, then runs the plan steps marked (apply) for every checkout they cover: refreshing outdated .hack/.gitignore files and removing stale standalone agent files the hack plugin replaces -(each removal backed up by the plugin migration). Before changing anything it keeps an upgrade +(each removal covered by the same snapshot). Before changing anything it keeps an upgrade snapshot and writes a journal beside it with the snapshot's coverage and gaps and each checkout's outcome. Checkouts with a blocker or a check that could not run are skipped. Prints the rollback command. --dry-run lists what would change. Exits 1 when anything selected was not done. +After interruption, --resume reuses that run's selection and snapshot; unexpected edits refuse. ### Options @@ -1946,6 +1947,7 @@ command. --dry-run lists what would change. Exits 1 when anything selected was n | `--only ` | Run only this plan step: ignore-rules or agent-integrations (comma-separated for both) | | `--project ` | Target a registered project by name (from ~/.hack/projects.json) | | `--dry-run` | List what would change and change nothing | +| `--resume ` | Resume the apply journal beside this explicit snapshot manifest | | `--json` | Output JSON (machine-readable) | | `--no-interactive` | Never prompt: apply documented defaults or fail with E_INTERACTIVE_REQUIRED (also via HACK_NO_INTERACTIVE=1) | | `--help, -h` | Show help | diff --git a/docs/reference/upgrade-apply.mmd b/docs/reference/upgrade-apply.mmd new file mode 100644 index 000000000..437a1783d --- /dev/null +++ b/docs/reference/upgrade-apply.mmd @@ -0,0 +1,20 @@ +stateDiagram-v2 + direction TB + [*] --> checked: fresh check + checked --> planned: snapshot and journal + planned --> started: journal target start + started --> applied: record output then outcome + started --> failed: closed failure + started --> interrupted: process lost + interrupted --> checked: explicit resume manifest + applied --> restored: guarded rollback + failed --> checked: explicit resume manifest + restored --> [*] + note right of planned + Coverage and gaps persisted + before target writes + end note + note right of interrupted + Unexpected edits refuse resume + No implicit run selection + end note diff --git a/docs/reference/upgrade-apply.svg b/docs/reference/upgrade-apply.svg new file mode 100644 index 000000000..efdd23dc7 --- /dev/null +++ b/docs/reference/upgrade-apply.svg @@ -0,0 +1,66 @@ +fresh checksnapshot and journaljournal target startrecord output then outcomeclosed failureprocess lostexplicit resume manifestguarded rollbackexplicit resume manifestcheckedstartedappliedfailedrestoredplannedinterruptedCoverage and gaps persistedbefore target writesUnexpected edits refuse resumeNo implicit run selection \ No newline at end of file diff --git a/docs/reference/v5-model-coverage.md b/docs/reference/v5-model-coverage.md index e9c611693..9acb764f7 100644 --- a/docs/reference/v5-model-coverage.md +++ b/docs/reference/v5-model-coverage.md @@ -168,3 +168,23 @@ the inspected immutable ID. Uncovered: real VM/foreground signals, external Dock writers, fsync failures, native publication retirement, boot rollover recovery, application flush behavior and live container absence. Linux portable checks do not qualify those macOS behaviors. + +### Upgrade apply recovery (HACK-2220) + +![Upgrade apply recovery](upgrade-apply.svg) ([source](upgrade-apply.mmd)) + +The apply journal records `planned → started → applied|unchanged|refused|failed` +per target, with snapshot coverage persisted before the first target starts. +`--resume` requires an explicit manifest and admits only the original input or +that run's deterministic ignore-rule output; agent removal rechecks ownership, +plugin replacement and snapshot coverage. A per-snapshot lock serializes writers. + +[Upgrade apply regressions](../../tests/upgrade-apply.test.ts) exercise seven real +SIGKILL boundaries over two checkouts, plus interruption after a real agent-file +removal. Each resumes the original snapshot and compares restored file bytes, +modes and directory entries. The metadata-only manifest checks include encoded +sentinels. This is implementation evidence, not a maintained finite TLA+ model. +Uncovered: power-loss durability, arbitrary filesystem-call interruption, +unsynchronized external writers, multiple independent snapshots writing the same +checkout, service-manager changes and env migration's separate journal. No +whole-machine upgrade or native runtime qualification follows from these tests. diff --git a/docs/versions/v5/guides/upgrade-from-v4.md b/docs/versions/v5/guides/upgrade-from-v4.md index 6821a4b61..413e1b60d 100644 --- a/docs/versions/v5/guides/upgrade-from-v4.md +++ b/docs/versions/v5/guides/upgrade-from-v4.md @@ -118,8 +118,31 @@ not run. Before it changes anything it keeps an upgrade snapshot (below) and writes a journal beside the snapshot manifest: the snapshot's coverage (whether it is complete, and its gaps), and each checkout's outcome. Agent files are removed only when the hack plugin that replaces them is verified, and each -removal is backed up by `hack setup migrate`'s manifest. Apply prints the -rollback commands and then rechecks. +removal is covered by the same upgrade snapshot. File bytes live only in private +snapshot blobs; the manifest and journal contain metadata. Apply prints the +rollback command and then rechecks. Ignore-rule writes stage inside the snapshot, +so they leave no staging files in the checkout. + +After an interruption, choose the run explicitly: + +```sh +hack upgrade apply --resume --dry-run +hack upgrade apply --resume +hack upgrade rollback --rollback-manifest +``` + +Resume uses the journal's selection and original snapshot, including its coverage +and gaps; it cannot be combined with `--only` or `--project`. It rechecks the +registered checkouts, refuses unexpected file edits, and verifies the snapshot's +original blobs before writing. It never chooses the latest run automatically. +Rollback restores ignore rules and removed agent files together, with their exact +bytes and modes. Files edited after apply require an explicit `--overwrite` to +restore; their current copies are retained. Service state and installed versions +are outside this file rollback. + +Env migration remains a separate grouped command until its journal owner is +integrated. Apply does not perform env cleanup or accept tracked-file edits on +your behalf. Before you follow the rest of the plan, keep a private copy of the files it changes: diff --git a/src/agents/plugin-migration.ts b/src/agents/plugin-migration.ts index a3fc2ae46..6da658222 100644 --- a/src/agents/plugin-migration.ts +++ b/src/agents/plugin-migration.ts @@ -12,6 +12,10 @@ import { dirname, isAbsolute, join, resolve } from "node:path"; import { readOptionalTextFile } from "../lib/fs.ts"; import { isRecord } from "../lib/guards.ts"; +import { + readUpgradeSnapshot, + verifyUpgradeSnapshotRecovery, +} from "../lib/upgrade-snapshot.ts"; import { classifyIntegrationContent, classifyIntegrationFile, @@ -414,6 +418,8 @@ export type PluginMigrationApplyHooks = { */ export async function applyPluginMigration( input: PluginMigrationInput & { + /** Existing metadata-only upgrade snapshot covering every removal. */ + readonly upgradeSnapshotManifest?: string; readonly backupDir: string; readonly hooks?: PluginMigrationApplyHooks; } @@ -456,7 +462,8 @@ export async function applyPluginMigration( const now = input.hooks?.now?.() ?? new Date(); const id = input.hooks?.id ?? manifestId(now); - const manifestPath = join(input.backupDir, `${id}.json`); + const manifestPath = + input.upgradeSnapshotManifest ?? join(input.backupDir, `${id}.json`); const manifest: MigrationManifest = { version: MIGRATION_MANIFEST_VERSION, kind: "hack-plugin-migration", @@ -466,13 +473,43 @@ export async function applyPluginMigration( entries: backups, }; // Commit point: nothing is removed before this write succeeds. - await writeManifest({ path: manifestPath, manifest }); + if (input.upgradeSnapshotManifest === undefined) { + await writeManifest({ path: manifestPath, manifest }); + } else { + if ( + !(await verifyUpgradeSnapshotRecovery({ + manifestPath, + paths: backups.map((backup) => backup.path), + })) + ) { + return { status: "refused", reason: "backup-failed", plan }; + } + const loaded = await readUpgradeSnapshot({ manifestPath }); + if ( + loaded.status !== "ok" || + backups.some((backup) => { + const entry = loaded.snapshot.manifest.entries.find( + (e) => e.path === backup.path + ); + return ( + entry?.before.kind !== "file" || + entry.before.sha256 !== backup.sha256 || + entry.before.mode !== backup.mode + ); + }) + ) { + return { status: "refused", reason: "backup-failed", plan }; + } + } const results: MigrationApplyEntry[] = []; const recorded: ManifestEntry[] = []; for (const [index, backup] of backups.entries()) { await input.hooks?.beforeRemove?.({ path: backup.path, index }); - const status = await removeBackedUpFile({ backup }); + const status = await removeBackedUpFile({ + backup, + keepDirectory: input.upgradeSnapshotManifest !== undefined, + }); results.push({ scope: backup.scope, target: backup.target, @@ -481,10 +518,12 @@ export async function applyPluginMigration( }); recorded.push({ ...backup, outcome: status.status }); } - await writeManifest({ - path: manifestPath, - manifest: { ...manifest, state: "applied", entries: recorded }, - }); + if (input.upgradeSnapshotManifest === undefined) { + await writeManifest({ + path: manifestPath, + manifest: { ...manifest, state: "applied", entries: recorded }, + }); + } return { status: "applied", plan, manifestPath, results }; } @@ -538,6 +577,7 @@ async function writeManifest(opts: { async function removeBackedUpFile(opts: { readonly backup: ManifestEntry; + readonly keepDirectory?: boolean; }): Promise> { const { backup } = opts; const target = listOwnedFileTargets(backup.scope).find( @@ -569,7 +609,7 @@ async function removeBackedUpFile(opts: { surface: target.surface, path: backup.path, currentRender: target.render(), - removeEmptyDirectory: target.removeEmptyDirectory, + removeEmptyDirectory: !opts.keepDirectory && target.removeEmptyDirectory, }); if (outcome.status === "preserved") { return { diff --git a/src/commands/upgrade.ts b/src/commands/upgrade.ts index dfa890e07..7d665042b 100644 --- a/src/commands/upgrade.ts +++ b/src/commands/upgrade.ts @@ -138,7 +138,21 @@ const optDryRun = defineOption({ description: "List what would change and change nothing", } as const); -const applyOptions = [optOnly, optProject, optDryRun, optJson] as const; +const optResume = defineOption({ + name: "resume", + type: "string", + long: "--resume", + valueHint: "", + description: + "Resume the apply journal beside this explicit snapshot manifest", +} as const); +const applyOptions = [ + optOnly, + optProject, + optDryRun, + optResume, + optJson, +] as const; const applySpec = defineCommand({ name: "apply", @@ -148,10 +162,11 @@ const applySpec = defineCommand({ description: [ "Checks first, then runs the plan steps marked (apply) for every checkout they cover: refreshing", "outdated .hack/.gitignore files and removing stale standalone agent files the hack plugin replaces", - "(each removal backed up by the plugin migration). Before changing anything it keeps an upgrade", + "(each removal covered by the same snapshot). Before changing anything it keeps an upgrade", "snapshot and writes a journal beside it with the snapshot's coverage and gaps and each checkout's", "outcome. Checkouts with a blocker or a check that could not run are skipped. Prints the rollback", "command. --dry-run lists what would change. Exits 1 when anything selected was not done.", + "After interruption, --resume reuses that run's selection and snapshot; unexpected edits refuse.", ].join("\n"), options: applyOptions, positionals: [], @@ -417,14 +432,21 @@ async function handleUpgradeApply({ throw new CliUsageError("--project needs a registered project name."); } const dryRun = args.options.dryRun === true; + if (args.options.resume !== undefined && !args.options.resume.trim()) { + throw new CliUsageError("--resume needs a snapshot manifest path."); + } const report = await check(ctx); const result = await runUpgradeApply({ report, - only, + only: args.options.only === undefined ? undefined : only, project, dryRun, home: resolveUserHomeDir(), collect: collectUpgradeSnapshotTargets, + resumeManifest: + args.options.resume === undefined + ? undefined + : resolve(ctx.cwd, args.options.resume), }); const ran = result.status === "applied" || result.status === "partial"; // Fresh evidence: what the check finds after the run. @@ -465,6 +487,15 @@ function applyJson(opts: { return { ...result, rollback_command: opts.rollback, + resume_command: + result.status === "applied" || result.status === "partial" + ? command([ + "upgrade", + "apply", + "--resume", + result.coverage.manifest_path, + ]) + : null, after: after === null ? null @@ -531,6 +562,13 @@ function outcomeLines( lines.push(` ${first.message}`); } } + if (result.status === "dry-run") { + lines.push( + ...result.targets.flatMap((target) => + (target.files ?? []).map((path) => ` ${path}`) + ) + ); + } if (result.status === "blocked") { lines.push( "Nothing ran: every selected checkout has a blocker or a check that could not be performed." @@ -555,13 +593,20 @@ function recoveryLines(opts: { "", `Snapshot ${coverage.snapshot_id}: ${coverage.files} file(s)${gaps}.`, `Journal: ${result.journal_path}`, + ...(result.status === "partial" + ? [ + `To resume: ${command(["upgrade", "apply", "--resume", coverage.manifest_path])}`, + ] + : []), ...(opts.rollback === null ? [] : [`To put the files back: ${opts.rollback}`]), - ...result.agent_backups.map( - (manifest) => - `To put removed agent files back: ${command(["setup", "migrate", "--rollback-manifest", manifest])}` - ), + ...result.agent_backups + .filter((manifest) => manifest !== coverage.manifest_path) + .map( + (manifest) => + `To put removed agent files back: ${command(["setup", "migrate", "--rollback-manifest", manifest])}` + ), ]; } diff --git a/src/lib/private-file.ts b/src/lib/private-file.ts index 1392e838f..43763a04e 100644 --- a/src/lib/private-file.ts +++ b/src/lib/private-file.ts @@ -218,8 +218,10 @@ export type ReplaceablePrivateFileOptions = { * the target (see `private-staging.ts`), so a copy left by a crash is * never visible to git; `"sibling"` uses a `...tmp` sibling, * for files outside any checkout whose directory must hold nothing else. + * A directory option keeps ignored staging under an existing private recovery + * directory instead. It must share the target filesystem for atomic rename. */ - readonly staging?: "ignored" | "sibling"; + readonly staging?: "ignored" | "sibling" | { readonly directory: string }; /** * Directories (normally the file's parent inside the project) that must be * real directories rather than symlinks, with a stable identity across the @@ -284,7 +286,10 @@ async function stagingFor(opts: ReplaceablePrivateFileOptions): Promise<{ } try { return await preparePrivateStaging({ - target: opts.path, + target: + typeof opts.staging === "object" + ? join(opts.staging.directory, basename(opts.path)) + : opts.path, refusal: opts.refusal, }); } catch (error: unknown) { diff --git a/src/lib/project-env-config.ts b/src/lib/project-env-config.ts index 5b8a92279..a38723b0d 100644 --- a/src/lib/project-env-config.ts +++ b/src/lib/project-env-config.ts @@ -531,6 +531,10 @@ export async function resolveProjectEnvSharedKeyPath(opts: { */ export async function ensureHackDirGitignore(opts: { readonly projectDir: string; + /** Private same-filesystem staging for snapshot-owned upgrade writes. */ + readonly stagingDirectory?: string; + /** Last snapshot identity check before atomic publication. */ + readonly beforePublish?: () => Promise; /** Test seam; see {@link HackDirGitignoreWriteAccess}. */ readonly checkWriteAccess?: HackDirGitignoreWriteAccess; }): Promise { @@ -555,8 +559,13 @@ export async function ensureHackDirGitignore(opts: { } await replacePrivateFileAtomic({ ...options, + staging: + opts.stagingDirectory === undefined + ? undefined + : { directory: opts.stagingDirectory }, text: next, mode: current ? current.mode & 0o777 : 0o644, + beforeRename: opts.beforePublish, }); return { changed: true, readOnly: false }; } @@ -686,7 +695,7 @@ function hackDirGitignoreFileOptions(opts: { }; } -function mergeHackDirGitignore(opts: { +export function mergeHackDirGitignore(opts: { readonly existing: string | null; }): string { return mergeManagedGitignoreBlock({ diff --git a/src/lib/upgrade-apply-recovery.ts b/src/lib/upgrade-apply-recovery.ts new file mode 100644 index 000000000..e493946e5 --- /dev/null +++ b/src/lib/upgrade-apply-recovery.ts @@ -0,0 +1,238 @@ +import { createHash } from "node:crypto"; +import { readFile } from "node:fs/promises"; +import { dirname, join, resolve } from "node:path"; +import { z } from "zod"; +import { readReplaceablePrivateFile } from "./private-file.ts"; +import { mergeHackDirGitignore } from "./project-env-config.ts"; +import type { + Planned, + UpgradeApplyJournal, + UpgradeApplyTarget, +} from "./upgrade-apply.ts"; +import { UPGRADE_APPLY_FIXES } from "./upgrade-plan.ts"; +import { + MAX_SNAPSHOT_FILE_BYTES, + readUpgradeSnapshot, + readUpgradeSnapshotJournal, + type UpgradeSnapshot, + type UpgradeSnapshotState, + verifyUpgradeSnapshotRecovery, +} from "./upgrade-snapshot.ts"; +export const UPGRADE_APPLY_JOURNAL_VERSION = 1; +export const UPGRADE_APPLY_JOURNAL_KIND = "hack-upgrade-apply"; + +const applyJournalSchema = z.object({ + version: z.literal(UPGRADE_APPLY_JOURNAL_VERSION), + kind: z.literal(UPGRADE_APPLY_JOURNAL_KIND), + started_at: z.string(), + updated_at: z.string(), + state: z.enum(["applying", "applied", "failed"]), + cli_version: z.string(), + filters: z.object({ + only: z.array(z.enum(UPGRADE_APPLY_FIXES)), + project: z.string().nullable(), + }), + snapshot: z.object({ + snapshot_id: z.string(), + manifest_path: z.string(), + files: z.number().int().nonnegative(), + complete: z.boolean(), + gaps: z.array(z.object({ name: z.string(), reason: z.string() })), + }), + notes: z.array(z.string()), + targets: z.array( + z.object({ + fix: z.enum(UPGRADE_APPLY_FIXES), + root: z.string().refine((path) => resolve(path) === path), + scope: z.string(), + project: z.string().nullable(), + status: z.enum([ + "planned", + "started", + "applied", + "unchanged", + "skipped", + "refused", + "failed", + ]), + message: z.string().nullable(), + backup_manifest: z.string().nullable(), + }) + ), +}); + +/** Read only explicit, private recovery points; never infer a run from current findings. */ +export async function loadApplyRecovery( + manifestPath: string +): Promise<{ snapshot: UpgradeSnapshot; journal: UpgradeApplyJournal } | null> { + const loaded = await readUpgradeSnapshot({ manifestPath }); + const parsed = applyJournalSchema.safeParse( + await readUpgradeSnapshotJournal({ manifestPath }) + ); + if (loaded.status !== "ok" || !parsed.success) { + return null; + } + const journal = parsed.data; + const coverage = journal.snapshot; + const snapshot = loaded.snapshot; + const keys = journal.targets.map((target) => `${target.fix}\0${target.root}`); + if ( + coverage.snapshot_id !== snapshot.id || + coverage.manifest_path !== snapshot.manifestPath || + coverage.files !== snapshot.manifest.entries.length || + new Set(keys).size !== keys.length + ) { + return null; + } + return { snapshot, journal }; +} + +/** + * Recovery admits the saved input or the deterministic ignore-rule output. + * An interrupted agent removal may be absent; it may never become a different file. + * Hashes are compared in memory; file content never enters the journal. + */ +export async function verifyApplyInput(opts: { + snapshot: UpgradeSnapshot; + target: Planned; + previous: UpgradeApplyTarget | undefined; +}): Promise<"safe" | "conflict"> { + if (opts.target.fix === "agent-integrations") { + return await verifyAgentInput(opts); + } + if (opts.target.projectDir === null) { + return "conflict"; + } + const path = resolve(opts.target.projectDir, ".gitignore"); + if ( + !(await verifyUpgradeSnapshotRecovery({ + manifestPath: opts.snapshot.manifestPath, + paths: [path], + })) + ) { + return "conflict"; + } + const entry = opts.snapshot.manifest.entries.find((e) => e.path === path); + if (entry === undefined) { + return "conflict"; + } + try { + const current = await readApplyState(path); + if ( + opts.previous?.status === "applied" || + opts.previous?.status === "unchanged" + ) { + return sameApplyState(current.state, entry.after ?? entry.before) + ? "safe" + : "conflict"; + } + if (sameApplyState(current.state, entry.before)) { + return "safe"; + } + if (opts.previous === undefined || current.state.kind === "absent") { + return "conflict"; + } + const original = + entry.before.kind === "absent" + ? null + : await readFile(join(opts.snapshot.dir, "blobs", entry.before.sha256)); + if ( + original !== null && + entry.before.kind === "file" && + hashBytes(original) !== entry.before.sha256 + ) { + return "conflict"; + } + const expected = mergeHackDirGitignore({ + existing: original?.toString("utf8") ?? null, + }); + const mode = entry.before.kind === "file" ? entry.before.mode : 0o644; + return current.state.sha256 === hashBytes(Buffer.from(expected)) && + current.state.mode === mode + ? "safe" + : "conflict"; + } catch { + return "conflict"; + } +} + +export function agentPaths( + snapshot: UpgradeSnapshot, + target: Planned +): string[] { + return snapshot.manifest.entries + .filter((e) => e.label.startsWith(`agent ${target.root}: `)) + .map((e) => e.path); +} + +export async function verifyAgentInput(opts: { + snapshot: UpgradeSnapshot; + target: Planned; + previous?: UpgradeApplyTarget; +}): Promise<"safe" | "conflict"> { + try { + for (const path of agentPaths(opts.snapshot, opts.target)) { + const entry = opts.snapshot.manifest.entries.find((e) => e.path === path); + if (entry === undefined) { + return "conflict"; + } + const current = await readApplyState(path); + if ( + (opts.previous?.status === "applied" || + opts.previous?.status === "unchanged") && + entry.after !== null + ) { + if (!sameApplyState(current.state, entry.after)) { + return "conflict"; + } + continue; + } + if ( + current.state.kind !== "absent" && + !sameApplyState(current.state, entry.before) + ) { + return "conflict"; + } + } + return "safe"; + } catch { + return "conflict"; + } +} + +async function readApplyState( + path: string +): Promise<{ state: UpgradeSnapshotState }> { + const file = await readReplaceablePrivateFile({ + path, + directories: [dirname(path)], + maxBytes: MAX_SNAPSHOT_FILE_BYTES, + refusal: () => new Error("Unsafe upgrade target."), + }); + if (file === null) { + return { state: { kind: "absent" } }; + } + const bytes = Buffer.from(file.text); + return { + state: { + kind: "file", + sha256: hashBytes(bytes), + size: bytes.length, + mode: file.mode, + }, + }; +} + +function hashBytes(bytes: Uint8Array): string { + return createHash("sha256").update(bytes).digest("hex"); +} + +function sameApplyState( + a: UpgradeSnapshotState, + b: UpgradeSnapshotState +): boolean { + if (a.kind === "absent" || b.kind === "absent") { + return a.kind === b.kind; + } + return a.sha256 === b.sha256 && a.mode === b.mode && a.size === b.size; +} diff --git a/src/lib/upgrade-apply.ts b/src/lib/upgrade-apply.ts index 1578c42b7..5be332b30 100644 --- a/src/lib/upgrade-apply.ts +++ b/src/lib/upgrade-apply.ts @@ -4,10 +4,12 @@ import { join, normalize, resolve } from "node:path"; import { applyPluginMigration, type PluginMigrationApplyResult, + planPluginMigration, } from "../agents/plugin-migration.ts"; import { MIGRATION_CLIENTS } from "../agents/plugin-replacement.ts"; import { resolveGlobalHackDir } from "./config-paths.ts"; import { ensureHackDirGitignore } from "./project-env-config.ts"; +import { withProjectsRegistryLock } from "./projects-registry-lock.ts"; import type { UpgradeCheckReport } from "./upgrade-check.ts"; import { isUpgradeApplyFix, @@ -46,8 +48,8 @@ import { * migration can remove anything) and when it ends. After an interruption * a `started` target may have changed files; `planned` ones have not. * Paths, ids, statuses, counts and closed messages only: never a value. - * - Stale agent files are removed only by the plugin migration, which backs - * up each file to its own manifest first and refuses when the hack plugin + * - Stale agent files are removed only by the plugin migration, with each + * original kept in this snapshot's blobs. It refuses when the hack plugin * that replaces them is not verified. Machine-wide agent files (under * HOME) are migrated only without `--project`; with it they are left out * and noted, not counted as unfinished work. @@ -56,8 +58,17 @@ import { * standalone files the verified plugin replaces. */ -export const UPGRADE_APPLY_JOURNAL_VERSION = 1; -export const UPGRADE_APPLY_JOURNAL_KIND = "hack-upgrade-apply"; +import { + agentPaths, + UPGRADE_APPLY_JOURNAL_KIND as JOURNAL_KIND, + UPGRADE_APPLY_JOURNAL_VERSION as JOURNAL_VERSION, + loadApplyRecovery, + verifyAgentInput, + verifyApplyInput, +} from "./upgrade-apply-recovery.ts"; + +export const UPGRADE_APPLY_JOURNAL_KIND = JOURNAL_KIND; +export const UPGRADE_APPLY_JOURNAL_VERSION = JOURNAL_VERSION; export type UpgradeApplyTargetStatus = /** Would run (dry run), or not reached yet (journal). */ @@ -86,6 +97,8 @@ export type UpgradeApplyTarget = { readonly message: string | null; /** The plugin migration's backup manifest: set before it can remove a file. */ readonly backup_manifest: string | null; + /** Candidate file paths, supplied by read-only dry-run previews. */ + readonly files?: readonly string[]; }; export type UpgradeApplyCoverage = { @@ -150,7 +163,7 @@ export type UpgradeApplySnapshotSet = { readonly gaps: readonly { readonly name: string; readonly reason: string }[]; }; -type Planned = { +export type Planned = { readonly fix: UpgradeApplyFix; readonly scope: string; readonly project: string | null; @@ -175,7 +188,7 @@ const NOT_RECORDED = * returns the upgrade snapshot set (production: * `collectUpgradeSnapshotTargets`). */ -export async function runUpgradeApply(input: { +export type RunUpgradeApplyInput = { readonly report: UpgradeCheckReport; readonly only?: readonly string[]; readonly project?: string; @@ -188,23 +201,64 @@ export async function runUpgradeApply(input: { readonly now?: () => Date; readonly applyPluginMigration?: typeof applyPluginMigration; readonly recordAfter?: typeof recordUpgradeSnapshotAfter; -}): Promise { - const only = input.only ?? []; + /** Explicit recovery point; never choose the newest run implicitly. */ + readonly resumeManifest?: string; + /** Test seam for real process interruption at durable boundaries. */ + readonly checkpoint?: (boundary: string) => Promise; +}; + +export async function runUpgradeApply( + input: RunUpgradeApplyInput +): Promise { + const recovery = + input.resumeManifest === undefined + ? null + : await loadApplyRecovery(input.resumeManifest); + if (input.resumeManifest !== undefined && recovery === null) { + return { + status: "refused", + reason: "snapshot", + message: "The apply journal or snapshot is invalid; nothing was changed.", + }; + } + if ( + recovery !== null && + (input.only !== undefined || input.project !== undefined) + ) { + return { + status: "refused", + reason: "snapshot", + message: + "Resume uses the saved selection; do not combine --resume with --only or --project.", + }; + } + const only = recovery?.journal.filters.only ?? input.only ?? []; const refusal = selectionRefusal({ report: input.report, only, - project: input.project, + project: recovery?.journal.filters.project ?? input.project, }); if (refusal !== null) { return refusal; } const fixes = only.filter(isUpgradeApplyFix); - const { planned, notes } = planTargets({ + const fresh = planTargets({ report: input.report, only: fixes, project: input.project ?? null, home: input.home, }); + const notes = recovery?.journal.notes ?? fresh.notes; + const planned: Planned[] = + recovery === null + ? fresh.planned + : recovery.journal.targets.map((target) => + recoverPlannedTarget({ + target, + report: input.report, + home: input.home, + }) + ); const runnable = planned.filter((target) => target.skip === null); const skipped = planned .filter((target) => target.skip !== null) @@ -215,18 +269,32 @@ export async function runUpgradeApply(input: { skipped, notes, }); - if (early !== null) { - return early; + if (early !== null && shouldReturnEarly(recovery, input.dryRun)) { + return await previewEarlyResult({ + result: early, + planned, + home: input.home, + }); } // The snapshot comes first: nothing is written unless it is kept. - const collected = await input.collect(); - const created = await createUpgradeSnapshot({ - purpose: "hack upgrade apply", - targets: await snapshotTargets({ runnable, collected }), - root: input.snapshotRoot, - now: input.now?.(), - }); + const collected = + recovery === null + ? await input.collect() + : { targets: [], gaps: recovery.journal.snapshot.gaps }; + const created = + recovery === null + ? await createUpgradeSnapshot({ + purpose: "hack upgrade apply", + targets: await snapshotTargets({ + runnable, + collected, + home: input.home, + }), + root: input.snapshotRoot, + now: input.now?.(), + }) + : { status: "created" as const, snapshot: recovery.snapshot }; if (created.status !== "created") { return { status: "refused", @@ -238,15 +306,53 @@ export async function runUpgradeApply(input: { }; } const { snapshot } = created; - const coverage: UpgradeApplyCoverage = { + const coverage: UpgradeApplyCoverage = recovery?.journal.snapshot ?? { snapshot_id: snapshot.id, manifest_path: snapshot.manifestPath, files: snapshot.manifest.entries.length, complete: collected.gaps.length === 0, gaps: collected.gaps, }; + return await withProjectsRegistryLock({ + lockPath: join(snapshot.dir, "apply.lock"), + label: "upgrade apply", + timeoutMs: 1000, + run: () => + executeApplyRun({ + input, + recovery, + notes, + fixes, + runnable, + skipped, + snapshot, + coverage, + }), + }); +} + +async function executeApplyRun(opts: { + input: RunUpgradeApplyInput; + recovery: Awaited>; + notes: readonly string[]; + fixes: readonly UpgradeApplyFix[]; + runnable: readonly Planned[]; + skipped: readonly UpgradeApplyTarget[]; + snapshot: UpgradeSnapshot; + coverage: UpgradeApplyCoverage; +}): Promise { + const { + input, + recovery, + notes, + fixes, + runnable, + skipped, + snapshot, + coverage, + } = opts; const now = () => (input.now?.() ?? new Date()).toISOString(); - const startedAt = now(); + const startedAt = recovery?.journal.started_at ?? now(); const results: UpgradeApplyTarget[] = [...skipped]; const journal = ( state: UpgradeApplyJournal["state"], @@ -263,7 +369,10 @@ export async function runUpgradeApply(input: { updated_at: now(), state, cli_version: input.report.cli.version, - filters: { only: fixes, project: input.project ?? null }, + filters: { + only: fixes, + project: recovery?.journal.filters.project ?? input.project ?? null, + }, snapshot: coverage, targets: [...results, ...(current === null ? [] : [current]), ...pending], notes, @@ -274,6 +383,7 @@ export async function runUpgradeApply(input: { snapshot, journal: journal("applying", null), }); + await input.checkpoint?.("journal-created"); const inSnapshot = new Set(snapshot.manifest.entries.map((e) => e.path)); const backupDir = input.agentBackupDir ?? join(resolveGlobalHackDir(), "agent-migrations"); @@ -282,7 +392,10 @@ export async function runUpgradeApply(input: { target.fix === "agent-integrations" ? { id: `upgrade-apply-${snapshot.id}-${index}`, - path: join(backupDir, `upgrade-apply-${snapshot.id}-${index}.json`), + path: + input.applyPluginMigration === undefined + ? snapshot.manifestPath + : join(backupDir, `upgrade-apply-${snapshot.id}-${index}.json`), } : null; // Before anything can change: this target started, with its backup. @@ -293,18 +406,40 @@ export async function runUpgradeApply(input: { backup_manifest: backup?.path ?? null, }), }); - results.push( - await applyTarget({ - target, - snapshot, - inSnapshot, - home: input.home, - backupDir, - backup, - migrate: input.applyPluginMigration ?? applyPluginMigration, - recordAfter: input.recordAfter ?? recordUpgradeSnapshotAfter, - }) + await input.checkpoint?.("target-started"); + const previous = recovery?.journal.targets.find( + (t) => t.fix === target.fix && t.root === target.root ); + const safe = await verifyApplyInput({ snapshot, target, previous }); + if (safe === "conflict") { + results.push( + outcome( + target, + "refused", + "The target changed outside this run; restore or review it before resuming." + ) + ); + } else if ( + previous?.status === "applied" || + previous?.status === "unchanged" + ) { + results.push(previous); + } else { + results.push( + await applyTarget({ + target, + snapshot, + inSnapshot, + home: input.home, + backupDir, + backup, + migrate: input.applyPluginMigration ?? applyPluginMigration, + recordAfter: input.recordAfter ?? recordUpgradeSnapshotAfter, + checkpoint: input.checkpoint, + }) + ); + } + await input.checkpoint?.("target-recorded"); await writeUpgradeSnapshotJournal({ snapshot, journal: journal("applying", null), @@ -323,9 +458,13 @@ export async function runUpgradeApply(input: { notes, coverage, journal_path: journalPath, - agent_backups: results.flatMap((r) => - r.backup_manifest === null ? [] : [r.backup_manifest] - ), + agent_backups: [ + ...new Set( + results.flatMap((r) => + r.backup_manifest === null ? [] : [r.backup_manifest] + ) + ), + ], }; } @@ -492,6 +631,7 @@ async function applyTarget(opts: { readonly backup: { readonly id: string; readonly path: string } | null; readonly migrate: typeof applyPluginMigration; readonly recordAfter: typeof recordUpgradeSnapshotAfter; + readonly checkpoint?: (boundary: string) => Promise; }): Promise { const { target } = opts; if (target.fix === "ignore-rules") { @@ -510,6 +650,7 @@ async function applyTarget(opts: { clients: MIGRATION_CLIENTS, skipPluginCheck: [], backupDir: opts.backupDir, + upgradeSnapshotManifest: opts.snapshot.manifestPath, hooks: { id: backup.id }, }); } catch (error) { @@ -525,7 +666,35 @@ async function applyTarget(opts: { backup_manifest: kept, }; } - return agentOutcome({ target, migrated }); + const result = agentOutcome({ target, migrated }); + await opts.checkpoint?.("agent-written"); + if ( + migrated.status === "applied" || + migrated.status === "nothing-to-migrate" + ) { + const paths = agentPaths(opts.snapshot, target); + if ( + (await verifyAgentInput({ snapshot: opts.snapshot, target })) !== "safe" + ) { + return { ...result, status: "failed", message: NOT_RECORDED }; + } + let recorded = false; + try { + recorded = + ( + await opts.recordAfter({ + manifestPath: opts.snapshot.manifestPath, + paths, + }) + ).status === "recorded"; + } catch { + recorded = false; + } + if (!recorded) { + return { ...result, status: "failed", message: NOT_RECORDED }; + } + } + return result; } async function applyIgnoreRules(opts: { @@ -533,6 +702,7 @@ async function applyIgnoreRules(opts: { readonly snapshot: UpgradeSnapshot; readonly inSnapshot: ReadonlySet; readonly recordAfter: typeof recordUpgradeSnapshotAfter; + readonly checkpoint?: (boundary: string) => Promise; }): Promise { const { target } = opts; if (target.projectDir === null) { @@ -549,13 +719,41 @@ async function applyIgnoreRules(opts: { let result: Awaited> | null = null; let failure: unknown = null; try { - result = await ensureHackDirGitignore({ projectDir: target.projectDir }); + result = await ensureHackDirGitignore({ + projectDir: target.projectDir, + stagingDirectory: opts.snapshot.dir, + beforePublish: async () => { + if ( + (await verifyApplyInput({ + snapshot: opts.snapshot, + target, + previous: outcome(target, "started", null), + })) !== "safe" + ) { + throw new Error("Upgrade target changed before publication."); + } + }, + }); } catch (error) { failure = error; } + await opts.checkpoint?.("target-written"); // Whatever is there now is this step's output, even after a failure. let recorded = false; try { + if ( + (await verifyApplyInput({ + snapshot: opts.snapshot, + target, + previous: outcome(target, "started", null), + })) !== "safe" + ) { + return outcome( + target, + "failed", + "The target changed outside this run; its after state was not adopted." + ); + } const record = await opts.recordAfter({ manifestPath: opts.snapshot.manifestPath, paths: [path], @@ -571,7 +769,7 @@ async function applyIgnoreRules(opts: { `${closedError(failure)}${recorded ? "" : ` ${NOT_RECORDED}`}` ); } - if (result.changed && !recorded) { + if (!recorded) { return outcome(target, "failed", NOT_RECORDED); } if (result.readOnly) { @@ -653,12 +851,34 @@ function outcome( async function snapshotTargets(opts: { readonly runnable: readonly Planned[]; readonly collected: UpgradeApplySnapshotSet; + readonly home: string; }): Promise { const collectedPaths = new Set( opts.collected.targets.map((t) => normalize(t.path)) ); const extra: UpgradeSnapshotTarget[] = []; for (const target of opts.runnable) { + if (target.fix === "agent-integrations") { + const plan = await planPluginMigration({ + scopes: [ + target.projectDir === null + ? { scope: "global", root: opts.home } + : { scope: "project", root: target.root }, + ], + home: opts.home, + clients: MIGRATION_CLIENTS, + skipPluginCheck: [], + }); + for (const entry of plan.entries) { + if (entry.action === "remove" && !collectedPaths.has(entry.path)) { + collectedPaths.add(entry.path); + extra.push({ + path: entry.path, + label: `agent ${target.root}: ${entry.target}`, + }); + } + } + } if (target.fix !== "ignore-rules" || target.projectDir === null) { continue; } @@ -702,3 +922,85 @@ function closedError(error: unknown): string { : "error"; return `Failed (${ERROR_CODE.test(code) ? code : "error"}).`; } + +function recoverPlannedTarget(opts: { + target: UpgradeApplyTarget; + report: UpgradeCheckReport; + home: string; +}): Planned { + const { target, report, home } = opts; + const checkout = report.projects.find( + (p) => + normalize(p.root) === normalize(target.root) && + p.project === target.project + ); + let skip: string | null = null; + if (target.project === null) { + skip = target.root === home ? null : SKIP_BLOCKED; + } else if ( + checkout === undefined || + unsafeRoots(report).has(normalize(target.root)) + ) { + skip = SKIP_BLOCKED; + } + return { + fix: target.fix, + scope: target.scope, + root: target.root, + project: target.project, + projectDir: checkout?.project_dir ?? null, + skip, + }; +} + +function shouldReturnEarly(recovery: unknown, dryRun: boolean): boolean { + return recovery === null || dryRun; +} + +async function previewTargetFiles( + target: Planned, + home: string +): Promise { + if (target.fix === "ignore-rules") { + return target.projectDir === null ? [] : [gitignorePath(target.projectDir)]; + } + const plan = await planPluginMigration({ + scopes: [ + target.projectDir === null + ? { scope: "global", root: home } + : { scope: "project", root: target.root }, + ], + home, + clients: MIGRATION_CLIENTS, + skipPluginCheck: [], + }); + return plan.entries + .filter((entry) => entry.action === "remove" || entry.action === "blocked") + .map((entry) => entry.path); +} + +async function previewEarlyResult(opts: { + result: Extract< + UpgradeApplyResult, + { status: "nothing-to-do" | "dry-run" | "blocked" } + >; + planned: readonly Planned[]; + home: string; +}): Promise { + if (opts.result.status !== "dry-run") { + return opts.result; + } + const targets = await Promise.all( + opts.result.targets.map(async (target) => { + const planned = opts.planned.find( + (p) => p.root === target.root && p.fix === target.fix + ); + const files = + planned === undefined + ? [] + : await previewTargetFiles(planned, opts.home); + return { ...target, files }; + }) + ); + return { ...opts.result, targets }; +} diff --git a/src/lib/upgrade-check-render.ts b/src/lib/upgrade-check-render.ts index 5e1ab7d59..176f68a70 100644 --- a/src/lib/upgrade-check-render.ts +++ b/src/lib/upgrade-check-render.ts @@ -55,6 +55,7 @@ export function renderUpgradeCheck( buildLine(report), "", ...machineLines(report), + ...findingCounts(report), "", ...projectSummaryLines(report), ...(opts.verbose === true ? verboseLines(report) : []), @@ -64,6 +65,21 @@ export function renderUpgradeCheck( return `${lines.map((text) => escapeTerminalControls(text)).join("\n")}\n`; } +/** Count finding kinds across all live checkouts, including folded-away projects. */ +function findingCounts(report: UpgradeCheckReport): string[] { + const counts = new Map(); + for (const checkout of report.projects) { + for (const finding of checkout.findings) { + if (NEEDS_WORK.has(finding.status)) { + counts.set(finding.id, (counts.get(finding.id) ?? 0) + 1); + } + } + } + return [...counts].map( + ([id, count]) => ` ${count} checkout(s): ${LABEL[id] ?? id}` + ); +} + /** First line: version, channel, and whether a source build or a candidate launcher ran. */ function buildLine(report: UpgradeCheckReport): string { const how: string[] = [`${report.cli.channel} channel`]; @@ -204,12 +220,14 @@ function verboseLines(report: UpgradeCheckReport): string[] { } function planLines(report: UpgradeCheckReport): string[] { - if (report.plan.length === 0) { - return report.ready - ? ["Nothing to do: this machine and its projects are ready."] - : ["No step to suggest, but the check is not complete; see above."]; - } - const lines = ["Plan", ...report.plan.flatMap(stepLines)]; + const lines = + report.plan.length === 0 + ? [ + report.ready + ? "Nothing to do: this machine and its projects are ready." + : "No step to suggest, but the check is not complete; see below.", + ] + : ["Plan", ...report.plan.flatMap(stepLines)]; if (report.plan.some((step) => step.applies)) { lines.push( " Steps marked (apply) run for all their checkouts with hack upgrade apply (--dry-run to preview, --project for one project)." diff --git a/src/lib/upgrade-plan.ts b/src/lib/upgrade-plan.ts index 73c965f34..3c0af16ef 100644 --- a/src/lib/upgrade-plan.ts +++ b/src/lib/upgrade-plan.ts @@ -165,12 +165,16 @@ export function buildUpgradePlan(opts: { if (!actionable(finding)) { continue; } - const key = `${finding.id}\u0000${finding.status}`; + const key = finding.id; const group = groups.get(key) ?? { id: finding.id, status: finding.status, findings: [], }; + const severity = { blocker: 0, incomplete: 1, warn: 2, info: 3, ok: 4 }; + if (severity[finding.status] < severity[group.status]) { + group.status = finding.status; + } group.findings.push({ target: { scope: project.name, @@ -245,7 +249,7 @@ function groupStep(opts: { targets, blocker: status === "blocker", }; - if (status === "warn" && isUpgradeApplyFix(id)) { + if (isUpgradeApplyFix(id)) { const args = ["upgrade", "apply", "--only", id]; return { ...base, diff --git a/src/lib/upgrade-snapshot.ts b/src/lib/upgrade-snapshot.ts index 32de59e58..024d951a8 100644 --- a/src/lib/upgrade-snapshot.ts +++ b/src/lib/upgrade-snapshot.ts @@ -518,6 +518,41 @@ export async function listUpgradeSnapshots( const JOURNAL_NAME = "journal.json"; const MAX_JOURNAL_BYTES = 16 * 1024 * 1024; +/** + * Read-only admission for a snapshot-backed writer. Every named path must be + * covered, its original blob intact, and its parent on the snapshot filesystem. + * This does not promise a filesystem transaction or exclude later writers. + */ +export async function verifyUpgradeSnapshotRecovery(input: { + readonly manifestPath: string; + readonly paths: readonly string[]; +}): Promise { + const loaded = await readUpgradeSnapshot(input); + if (loaded.status !== "ok") { + return false; + } + const { snapshot } = loaded; + try { + const device = (await stat(snapshot.dir)).dev; + for (const path of input.paths) { + const entry = snapshot.manifest.entries.find((e) => e.path === path); + if (entry === undefined || (await stat(dirname(path))).dev !== device) { + return false; + } + if ( + entry.before.kind === "file" && + (await readBlob({ dir: snapshot.dir, sha256: entry.before.sha256 })) === + null + ) { + return false; + } + } + return true; + } catch { + return false; + } +} + /** * Writes the step journal of the step that owns this snapshot (for example * `hack upgrade apply`) beside its manifest, atomically and privately. diff --git a/tests/fixtures/upgrade-apply-worker.ts b/tests/fixtures/upgrade-apply-worker.ts new file mode 100644 index 000000000..f62deec17 --- /dev/null +++ b/tests/fixtures/upgrade-apply-worker.ts @@ -0,0 +1,26 @@ +import { readFile } from "node:fs/promises"; +import { runUpgradeApply } from "../../src/lib/upgrade-apply.ts"; +import type { UpgradeCheckReport } from "../../src/lib/upgrade-check.ts"; +import { collectUpgradeSnapshotTargets } from "../../src/lib/upgrade-snapshot-targets.ts"; + +// Only the parent fixture supplies these paths and a metadata-only report. +const report = JSON.parse( + await readFile(process.argv[2] ?? "", "utf8") +) as UpgradeCheckReport; +const boundary = Number(process.argv[3]); +let checkpoint = 0; +await runUpgradeApply({ + report, + home: process.env.HOME ?? "", + only: [process.argv[4] ?? "ignore-rules"], + dryRun: false, + collect: collectUpgradeSnapshotTargets, + checkpoint: async () => { + checkpoint += 1; + if (checkpoint === boundary) { + process.stdout.write("checkpoint\n"); + await new Promise(() => {}); + } + }, +}); +throw new Error("The requested interruption boundary was not reached."); diff --git a/tests/upgrade-apply.test.ts b/tests/upgrade-apply.test.ts index c653f430f..5ae092261 100644 --- a/tests/upgrade-apply.test.ts +++ b/tests/upgrade-apply.test.ts @@ -15,7 +15,8 @@ import { } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; - +import { renderCodexSkill } from "../src/agents/codex-skill.ts"; +import { HACK_PLUGIN_ID } from "../src/agents/plugin-replacement.ts"; import { runUpgradeApply } from "../src/lib/upgrade-apply.ts"; import { runUpgradeCheck, @@ -274,11 +275,16 @@ test("100 checkouts fit one screen: summary first, worktrees folded, plan groupe expect(ignore?.applies).toBe(true); expect(ignore?.count).toBeGreaterThan(90); // A manual step over several checkouts names one command pattern. - const legacy = report.plan.find((s) => s.id === "env-format" && !s.blocker); + const legacy = report.plan.find((s) => s.id === "env-format"); + expect(legacy?.blocker).toBe(true); + expect(report.plan.filter((s) => s.id === "env-format")).toHaveLength(1); expect(legacy?.command).toBeNull(); - expect(legacy?.action).toContain( - "hack doctor --migrate-env-config --path " - ); + expect(legacy?.action).toContain("checkout"); + expect( + legacy?.targets.some((target) => + target.command?.includes("doctor --migrate-env-config") + ) + ).toBe(true); expect(report.plan.length).toBeLessThanOrEqual(10); const text = renderUpgradeCheck(report); @@ -875,3 +881,251 @@ test("a backup that cannot be inspected after a failed migration keeps its journ await chmod(backupDir, 0o700).catch(() => undefined); } }, 60_000); + +test("SIGKILL at every apply boundary resumes the same snapshot and rolls back exact bytes and modes", async () => { + const a = await project("app-a", 1); + await writeRegistry([a]); + const ignore = join(a.projectDir, ".gitignore"); + await writeFile(ignore, "# keep custom rules\ncustom\n"); + await chmod(ignore, 0o640); + const report = await check(); + const reportPath = join(root, "report.json"); + await writeFile(reportPath, JSON.stringify(report)); + const before = [ + ...(await tree(join(root, "repos"))), + ...(await tree(join(root, "worktrees"))), + ]; + // Initial journal, then started / published / recorded for both targets. + for (let boundary = 1; boundary <= 7; boundary += 1) { + const child = Bun.spawn( + [ + process.execPath, + resolve(import.meta.dir, "fixtures/upgrade-apply-worker.ts"), + reportPath, + String(boundary), + ], + { + cwd: root, + env: privateCliEnv({ + root, + home, + env: { + HACK_HOME: hackHome, + HACK_GLOBAL_CONFIG_PATH: join(hackHome, "hack.config.json"), + HACK_HOST_PROBES_DISABLE: "1", + HACK_SYSTEMD_DISABLE: "1", + HACK_NO_INTERACTIVE: "1", + }, + }), + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + } + ); + const reader = child.stdout.getReader(); + try { + const reached = await Promise.race([ + reader.read(), + new Promise((_resolve, reject) => { + setTimeout( + () => reject(new Error("No interruption checkpoint")), + 15_000 + ).unref(); + }), + ]); + expect(new TextDecoder().decode(reached.value)).toContain("checkpoint"); + } finally { + child.kill("SIGKILL"); + await child.exited; + reader.releaseLock(); + } + const [snapshot] = await listUpgradeSnapshots(); + if (snapshot === undefined) { + throw new Error("Missing interrupted snapshot"); + } + const resumed = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + resumeManifest: snapshot.manifestPath, + collect: async () => { + throw new Error("Resume must reuse coverage"); + }, + }); + expect(resumed.status).toBe("applied"); + if (resumed.status !== "applied") { + throw new Error(JSON.stringify(resumed)); + } + expect(resumed.coverage.manifest_path).toBe(snapshot.manifestPath); + expect(await readFile(ignore, "utf8")).toContain("# keep custom rules"); + expect(await readFile(resumed.journal_path, "utf8")).not.toContain( + SENTINEL + ); + expect( + (await restoreUpgradeSnapshot({ manifestPath: snapshot.manifestPath })) + .status + ).toBe("restored"); + expect([ + ...(await tree(join(root, "repos"))), + ...(await tree(join(root, "worktrees"))), + ]).toEqual(before); + } +}, 120_000); + +test("resume refuses an unknown journal and preserves edits after apply", async () => { + const a = await project("app-a"); + await writeRegistry([a]); + const result = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + only: ["ignore-rules"], + collect: collectUpgradeSnapshotTargets, + }); + if (result.status !== "applied") { + throw new Error("Expected apply"); + } + const path = join(a.projectDir, ".gitignore"); + await writeFile(path, "later edit\n"); + const resumed = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + resumeManifest: result.coverage.manifest_path, + collect: collectUpgradeSnapshotTargets, + }); + expect(resumed.status).toBe("partial"); + expect(await readFile(path, "utf8")).toBe("later edit\n"); + await writeFile(result.journal_path, '{"version":999}\n', { mode: 0o600 }); + const before = await tree(root); + expect( + ( + await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + resumeManifest: result.coverage.manifest_path, + collect: collectUpgradeSnapshotTargets, + }) + ).status + ).toBe("refused"); + expect(await tree(root)).toEqual(before); +}, 60_000); + +test("agent removals keep metadata-only snapshots and rollback exact files and directories after interruption", async () => { + const a = await project("app-a"); + await writeRegistry([a]); + const agent = await staleHomeAgentFile(); + await writeFile(agent, renderCodexSkill()); + await chmod(agent, 0o640); + await writeFile( + join(home, ".codex/config.toml"), + `[plugins."${HACK_PLUGIN_ID}"]\nenabled = true\n` + ); + await writeFile( + join(a.repoRoot, "AGENTS.md"), + "\nold\n\n" + ); + const beforeAgentTree = await tree(join(home, ".codex")); + const report = await check(); + expect(report.plan.some((step) => step.id === "agent-integrations")).toBe( + true + ); + const reportPath = join(root, "report.json"); + await writeFile(reportPath, JSON.stringify(report)); + // Interrupt after the real plugin migration has removed the global file, + // before apply records its after state or commits the target outcome. + const child = Bun.spawn( + [ + process.execPath, + resolve(import.meta.dir, "fixtures/upgrade-apply-worker.ts"), + reportPath, + "3", + "agent-integrations", + ], + { + cwd: root, + env: privateCliEnv({ + root, + home, + env: { + HACK_HOME: hackHome, + HACK_GLOBAL_CONFIG_PATH: join(hackHome, "hack.config.json"), + HACK_HOST_PROBES_DISABLE: "1", + HACK_SYSTEMD_DISABLE: "1", + }, + }), + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + } + ); + const reader = child.stdout.getReader(); + try { + const reached = await reader.read(); + expect(new TextDecoder().decode(reached.value)).toContain("checkpoint"); + } finally { + child.kill("SIGKILL"); + await child.exited; + reader.releaseLock(); + } + expect(await lstat(agent).catch(() => null)).toBeNull(); + const [snapshot] = await listUpgradeSnapshots(); + if (snapshot === undefined) { + throw new Error("Missing snapshot"); + } + const resumed = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + resumeManifest: snapshot.manifestPath, + collect: collectUpgradeSnapshotTargets, + }); + expect(resumed.status).toBe("applied"); + const manifest = await readFile(snapshot.manifestPath, "utf8"); + expect(manifest).not.toContain("contentBase64"); + expect(manifest).not.toContain(Buffer.from(SENTINEL).toString("base64")); + expect( + (await restoreUpgradeSnapshot({ manifestPath: snapshot.manifestPath })) + .status + ).toBe("restored"); + expect(await tree(join(home, ".codex"))).toEqual(beforeAgentTree); +}, 60_000); + +test("resume refuses damaged snapshot originals before applying another fix", async () => { + const a = await project("app-a"); + await writeRegistry([a]); + await writeFile(join(a.projectDir, ".gitignore"), "keep\n"); + const result = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + only: ["ignore-rules"], + collect: collectUpgradeSnapshotTargets, + }); + if (result.status !== "applied") { + throw new Error("Expected apply"); + } + const [snapshot] = await listUpgradeSnapshots(); + const original = snapshot?.manifest.entries.find( + (entry) => entry.path === join(a.projectDir, ".gitignore") + ); + if (snapshot === undefined || original?.before.kind !== "file") { + throw new Error("Missing original"); + } + await writeFile( + join(snapshot.dir, "blobs", original.before.sha256), + "damaged\n", + { mode: 0o600 } + ); + const before = await tree(join(root, "repos")); + const resumed = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + resumeManifest: result.coverage.manifest_path, + collect: collectUpgradeSnapshotTargets, + }); + expect(resumed.status).toBe("partial"); + expect(await tree(join(root, "repos"))).toEqual(before); +}, 60_000); diff --git a/tests/upgrade-check.test.ts b/tests/upgrade-check.test.ts index ddb44247b..655d4d81d 100644 --- a/tests/upgrade-check.test.ts +++ b/tests/upgrade-check.test.ts @@ -287,19 +287,30 @@ test("the plan starts with the snapshot, then blockers, then machine and project report.plan.map((step) => [step.id, step.scope, step.blocker]) ).toEqual([ ["snapshot", "machine", false], - ["env-format", "app-blocked", true], + ["env-format", "projects", true], // Its dangling layer link cannot be inspected for secrets either. ["env-secrets", "app-blocked", false], ["registry-dead", "machine", false], ["global-infra", "machine", false], - ["env-format", "app-legacy", false], ]); expect(report.next_command).toBe("hack upgrade snapshot"); - const legacyStep = report.plan.find((step) => step.scope === "app-legacy"); - expect(legacyStep?.command).toBe( + const grouped = report.plan.filter((step) => step.id === "env-format"); + expect(grouped).toHaveLength(1); + expect(grouped[0]?.targets.map((target) => target.scope).sort()).toEqual([ + "app-blocked", + "app-legacy", + ]); + const legacyTarget = grouped[0]?.targets.find( + (target) => target.scope === "app-legacy" + ); + expect(legacyTarget?.command).toBe( `hack doctor --migrate-env-config --path ${legacy.repoRoot}` ); - expect(legacyStep?.why).toContain("legacy-json, dotenv"); + expect( + report.projects + .find((project) => project.name === "app-legacy") + ?.findings.find((finding) => finding.id === "env-format")?.summary + ).toContain("legacy-json, dotenv"); }); test("a host with nothing to do is ready, with no plan and no next command", async () => { From 9f1ffb43da1f2cba3884ea349a7621b8e307e39e Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 03:50:21 +0000 Subject: [PATCH 5/7] fix(upgrade): plan cross-volume refusals and serialize rollback --- docs/reference/cli.md | 3 + docs/reference/upgrade-apply.mmd | 4 +- docs/reference/upgrade-apply.svg | 4 +- docs/reference/v5-model-coverage.md | 5 + docs/versions/v5/guides/upgrade-from-v4.md | 21 +- src/commands/upgrade.ts | 71 +++++- src/lib/upgrade-apply-filesystems.ts | 71 ++++++ src/lib/upgrade-apply-recovery.ts | 4 + src/lib/upgrade-apply.ts | 67 ++++-- src/lib/upgrade-check-render.ts | 11 +- src/lib/upgrade-plan.ts | 10 +- tests/upgrade-apply.test.ts | 250 +++++++++++++++++---- tests/upgrade-check.test.ts | 6 + 13 files changed, 451 insertions(+), 76 deletions(-) create mode 100644 src/lib/upgrade-apply-filesystems.ts diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 2e6c63df3..b04010cd6 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -1939,6 +1939,8 @@ snapshot and writes a journal beside it with the snapshot's coverage and gaps an outcome. Checkouts with a blocker or a check that could not run are skipped. Prints the rollback command. --dry-run lists what would change. Exits 1 when anything selected was not done. After interruption, --resume reuses that run's selection and snapshot; unexpected edits refuse. +Other-volume checkouts are skipped before writing. Select one project with --snapshot-root on its volume +to keep recovery bytes in an explicit private directory outside every Git working tree. ### Options @@ -1948,6 +1950,7 @@ After interruption, --resume reuses that run's selection and snapshot | `--project ` | Target a registered project by name (from ~/.hack/projects.json) | | `--dry-run` | List what would change and change nothing | | `--resume ` | Resume the apply journal beside this explicit snapshot manifest | +| `--snapshot-root ` | Keep recovery bytes in this private directory outside Git, on the selected targets' filesystem | | `--json` | Output JSON (machine-readable) | | `--no-interactive` | Never prompt: apply documented defaults or fail with E_INTERACTIVE_REQUIRED (also via HACK_NO_INTERACTIVE=1) | | `--help, -h` | Show help | diff --git a/docs/reference/upgrade-apply.mmd b/docs/reference/upgrade-apply.mmd index 437a1783d..6806c4bf3 100644 --- a/docs/reference/upgrade-apply.mmd +++ b/docs/reference/upgrade-apply.mmd @@ -1,7 +1,9 @@ stateDiagram-v2 direction TB [*] --> checked: fresh check - checked --> planned: snapshot and journal + checked --> planned: same filesystem admitted + checked --> skipped: other filesystem + skipped --> checked: explicit snapshot root planned --> started: journal target start started --> applied: record output then outcome started --> failed: closed failure diff --git a/docs/reference/upgrade-apply.svg b/docs/reference/upgrade-apply.svg index efdd23dc7..ae8817ef9 100644 --- a/docs/reference/upgrade-apply.svg +++ b/docs/reference/upgrade-apply.svg @@ -1,4 +1,4 @@ -fresh checksnapshot and journaljournal target startrecord output then outcomeclosed failureprocess lostexplicit resume manifestguarded rollbackexplicit resume manifestcheckedstartedappliedfailedrestoredplannedinterruptedCoverage and gaps persistedbefore target writesUnexpected edits refuse resumeNo implicit run selection \ No newline at end of file +]]>fresh checksame filesystem admittedother filesystemexplicit snapshot rootjournal target startrecord output then outcomeclosed failureprocess lostexplicit resume manifestguarded rollbackexplicit resume manifestcheckedskippedstartedappliedfailedrestoredplannedinterruptedCoverage and gaps persistedbefore target writesUnexpected edits refuse resumeNo implicit run selection \ No newline at end of file diff --git a/docs/reference/v5-model-coverage.md b/docs/reference/v5-model-coverage.md index 9acb764f7..923bf80be 100644 --- a/docs/reference/v5-model-coverage.md +++ b/docs/reference/v5-model-coverage.md @@ -184,6 +184,11 @@ SIGKILL boundaries over two checkouts, plus interruption after a real agent-file removal. Each resumes the original snapshot and compares restored file bytes, modes and directory entries. The metadata-only manifest checks include encoded sentinels. This is implementation evidence, not a maintained finite TLA+ model. +An injected two-device regression proves default skips before snapshot creation, +continued same-device work and explicit-root selection refusal when devices still +differ. CLI rollback shares the apply lock; a live-owner regression proves a +bounded busy refusal without changing the fixture tree. These checks do not +qualify real external-volume, network-filesystem or macOS mount semantics. Uncovered: power-loss durability, arbitrary filesystem-call interruption, unsynchronized external writers, multiple independent snapshots writing the same checkout, service-manager changes and env migration's separate journal. No diff --git a/docs/versions/v5/guides/upgrade-from-v4.md b/docs/versions/v5/guides/upgrade-from-v4.md index 413e1b60d..0188bb7e2 100644 --- a/docs/versions/v5/guides/upgrade-from-v4.md +++ b/docs/versions/v5/guides/upgrade-from-v4.md @@ -111,6 +111,7 @@ To run the apply steps: hack upgrade apply --dry-run # what would change; changes nothing hack upgrade apply # every apply step, every checkout hack upgrade apply --only ignore-rules --project my-app +hack upgrade apply --only ignore-rules --project my-app --snapshot-root /path/on/project-volume/private-snapshots ``` Apply checks first and skips any checkout with a blocker or a check that could @@ -132,7 +133,8 @@ hack upgrade rollback --rollback-manifest ``` Resume uses the journal's selection and original snapshot, including its coverage -and gaps; it cannot be combined with `--only` or `--project`. It rechecks the +and gaps; it cannot be combined with `--only`, `--project` or `--snapshot-root`. +It rechecks the registered checkouts, refuses unexpected file edits, and verifies the snapshot's original blobs before writing. It never chooses the latest run automatically. Rollback restores ignore rules and removed agent files together, with their exact @@ -140,6 +142,23 @@ bytes and modes. Files edited after apply require an explicit `--overwrite` to restore; their current copies are retained. Service state and installed versions are outside this file rollback. +The snapshot and changed files must share a filesystem: rollback uses atomic +rename and hard-link publication. Checkouts on another volume are skipped during +planning, before any snapshot or journal is written, with `other-filesystem` and +the `--snapshot-root` remedy. Other eligible checkouts still run. An explicit root +must be private, owned by you, outside every Git working tree and on the selected +targets' volume; a selection that still spans filesystems is refused. Hack does +not discover or create snapshot roots on external volumes automatically. The +chosen directory holds original file bytes, including the broader upgrade +snapshot set; choose its storage and backup policy accordingly. + +The apply JSON field `agent_backups` lists recovery manifests and can equal +`coverage.manifest_path`: it no longer implies a separate plugin backup. A grouped +plan target keeps its own `status` and `apply_eligible`; the text plan counts +blocked or unchecked checkouts that apply will skip. `--dry-run` lists candidate +paths and can be longer than the one-screen check summary. Rollback returns +`busy` without restoring anything while apply or resume holds that snapshot's lock. + Env migration remains a separate grouped command until its journal owner is integrated. Apply does not perform env cleanup or accept tracked-file edits on your behalf. diff --git a/src/commands/upgrade.ts b/src/commands/upgrade.ts index 7d665042b..e9754bc2f 100644 --- a/src/commands/upgrade.ts +++ b/src/commands/upgrade.ts @@ -1,4 +1,4 @@ -import { resolve } from "node:path"; +import { dirname, join, resolve } from "node:path"; import type { CliContext, CommandArgs } from "../cli/command.ts"; import { @@ -10,6 +10,10 @@ import { import { optJson, optProject } from "../cli/options.ts"; import { emitCliResult, okResult } from "../lib/cli-result.ts"; import { resolveUserHomeDir } from "../lib/home-dir.ts"; +import { + ProjectsRegistryLockTimeoutError, + withProjectsRegistryLock, +} from "../lib/projects-registry-lock.ts"; import { escapeTerminalControls } from "../lib/terminal-text.ts"; import { runUpgradeApply, @@ -27,6 +31,7 @@ import { createUpgradeSnapshot, listUpgradeSnapshots, type RestoreUpgradeSnapshotResult, + readUpgradeSnapshot, restoreUpgradeSnapshot, } from "../lib/upgrade-snapshot.ts"; import { collectUpgradeSnapshotTargets } from "../lib/upgrade-snapshot-targets.ts"; @@ -146,11 +151,20 @@ const optResume = defineOption({ description: "Resume the apply journal beside this explicit snapshot manifest", } as const); +const optSnapshotRoot = defineOption({ + name: "snapshotRoot", + type: "string", + long: "--snapshot-root", + valueHint: "", + description: + "Keep recovery bytes in this private directory outside Git, on the selected targets' filesystem", +} as const); const applyOptions = [ optOnly, optProject, optDryRun, optResume, + optSnapshotRoot, optJson, ] as const; @@ -167,6 +181,8 @@ const applySpec = defineCommand({ "outcome. Checkouts with a blocker or a check that could not run are skipped. Prints the rollback", "command. --dry-run lists what would change. Exits 1 when anything selected was not done.", "After interruption, --resume reuses that run's selection and snapshot; unexpected edits refuse.", + "Other-volume checkouts are skipped before writing. Select one project with --snapshot-root on its volume", + "to keep recovery bytes in an explicit private directory outside every Git working tree.", ].join("\n"), options: applyOptions, positionals: [], @@ -364,10 +380,19 @@ async function handleUpgradeRollback({ } return 1; } - const result = await restoreUpgradeSnapshot({ - manifestPath: resolve(ctx.cwd, manifest), + const manifestPath = resolve(ctx.cwd, manifest); + const result = await restoreWithApplyLock({ + manifestPath, overwrite: args.options.overwrite === true, }); + if (result.status === "busy") { + if (json) { + emitCliResult({ result: okResult({ data: result }) }); + } else { + process.stdout.write(`${result.message}\n`); + } + return 1; + } if (json) { emitCliResult({ result: okResult({ data: rollbackJson(result) }) }); } else { @@ -376,6 +401,34 @@ async function handleUpgradeRollback({ return result.status === "restored" ? 0 : 1; } +/** CLI writers share one lock; direct snapshot-library callers own their serialization. */ +async function restoreWithApplyLock(opts: { + manifestPath: string; + overwrite: boolean; +}) { + const loaded = await readUpgradeSnapshot(opts); + if (loaded.status !== "ok") { + return { status: "invalid-manifest" as const, message: loaded.message }; + } + try { + return await withProjectsRegistryLock({ + lockPath: join(dirname(opts.manifestPath), "apply.lock"), + label: "upgrade snapshot", + timeoutMs: 1000, + run: () => restoreUpgradeSnapshot(opts), + }); + } catch (error) { + if (!(error instanceof ProjectsRegistryLockTimeoutError)) { + throw error; + } + return { + status: "busy" as const, + message: + "The snapshot is in use; wait for apply or resume to finish before rolling back.", + }; + } +} + function rollbackJson(result: RestoreUpgradeSnapshotResult) { if (result.status === "invalid-manifest") { return { status: result.status, message: result.message }; @@ -435,6 +488,12 @@ async function handleUpgradeApply({ if (args.options.resume !== undefined && !args.options.resume.trim()) { throw new CliUsageError("--resume needs a snapshot manifest path."); } + if ( + args.options.snapshotRoot !== undefined && + !args.options.snapshotRoot.trim() + ) { + throw new CliUsageError("--snapshot-root needs a directory path."); + } const report = await check(ctx); const result = await runUpgradeApply({ report, @@ -447,6 +506,10 @@ async function handleUpgradeApply({ args.options.resume === undefined ? undefined : resolve(ctx.cwd, args.options.resume), + snapshotRoot: + args.options.snapshotRoot === undefined + ? undefined + : resolve(ctx.cwd, args.options.snapshotRoot), }); const ran = result.status === "applied" || result.status === "partial"; // Fresh evidence: what the check finds after the run. @@ -571,7 +634,7 @@ function outcomeLines( } if (result.status === "blocked") { lines.push( - "Nothing ran: every selected checkout has a blocker or a check that could not be performed." + "Nothing ran: every selected target was skipped; see the reasons above." ); } return [...lines, ...notes]; diff --git a/src/lib/upgrade-apply-filesystems.ts b/src/lib/upgrade-apply-filesystems.ts new file mode 100644 index 000000000..ddcf00292 --- /dev/null +++ b/src/lib/upgrade-apply-filesystems.ts @@ -0,0 +1,71 @@ +import { stat } from "node:fs/promises"; +import { dirname, resolve } from "node:path"; +import { isRecord } from "./guards.ts"; +import type { Planned } from "./upgrade-apply.ts"; + +const OTHER_FILESYSTEM = + "Not run: the checkout and snapshot are on different filesystems. Rerun with --project --snapshot-root , or run the checkout's manual fix."; +const UNKNOWN_FILESYSTEM = + "Not run: the target or snapshot filesystem could not be checked; inspect its directory permissions before retrying."; + +/** Read-only: nonexistent snapshot roots are checked through their nearest existing parent. */ +export async function upgradeFilesystemDevice(path: string): Promise { + let current = resolve(path); + for (;;) { + try { + return (await stat(current)).dev; + } catch (error: unknown) { + const parent = dirname(current); + if (!isRecord(error) || error.code !== "ENOENT" || parent === current) { + throw new Error("The upgrade filesystem could not be checked."); + } + current = parent; + } + } +} + +/** Skip unsupported recovery layouts before creating the snapshot or writing a journal. */ +export async function planUpgradeFilesystems(opts: { + targets: readonly Planned[]; + snapshotRoot: string; + files: (target: Planned) => Promise; + device?: (path: string) => Promise; +}): Promise { + const device = opts.device ?? upgradeFilesystemDevice; + const snapshotDevice = await device(opts.snapshotRoot).catch(() => null); + return await Promise.all( + opts.targets.map(async (target) => { + if (target.skip !== null) { + return target; + } + try { + if (snapshotDevice === null) { + throw new Error("Unknown snapshot filesystem."); + } + const files = await opts.files(target); + const paths = + files.length === 0 ? [target.projectDir ?? target.root] : files; + for (const path of paths) { + if ((await device(path)) !== snapshotDevice) { + return { + ...target, + skip: OTHER_FILESYSTEM, + skipReason: "other-filesystem" as const, + }; + } + } + return target; + } catch { + return { + ...target, + skip: UNKNOWN_FILESYSTEM, + skipReason: "filesystem-unknown" as const, + }; + } + }) + ); +} + +export function hasOtherFilesystem(targets: readonly Planned[]): boolean { + return targets.some((target) => target.skipReason === "other-filesystem"); +} diff --git a/src/lib/upgrade-apply-recovery.ts b/src/lib/upgrade-apply-recovery.ts index e493946e5..b1df65f2e 100644 --- a/src/lib/upgrade-apply-recovery.ts +++ b/src/lib/upgrade-apply-recovery.ts @@ -57,6 +57,10 @@ const applyJournalSchema = z.object({ ]), message: z.string().nullable(), backup_manifest: z.string().nullable(), + reason: z + .enum(["other-filesystem", "filesystem-unknown"]) + .nullable() + .optional(), }) ), }); diff --git a/src/lib/upgrade-apply.ts b/src/lib/upgrade-apply.ts index 5be332b30..95d273a98 100644 --- a/src/lib/upgrade-apply.ts +++ b/src/lib/upgrade-apply.ts @@ -10,6 +10,10 @@ import { MIGRATION_CLIENTS } from "../agents/plugin-replacement.ts"; import { resolveGlobalHackDir } from "./config-paths.ts"; import { ensureHackDirGitignore } from "./project-env-config.ts"; import { withProjectsRegistryLock } from "./projects-registry-lock.ts"; +import { + hasOtherFilesystem, + planUpgradeFilesystems, +} from "./upgrade-apply-filesystems.ts"; import type { UpgradeCheckReport } from "./upgrade-check.ts"; import { isUpgradeApplyFix, @@ -18,6 +22,7 @@ import { } from "./upgrade-plan.ts"; import { createUpgradeSnapshot, + defaultUpgradeSnapshotRoot, recordUpgradeSnapshotAfter, type UpgradeSnapshot, type UpgradeSnapshotTarget, @@ -99,6 +104,7 @@ export type UpgradeApplyTarget = { readonly backup_manifest: string | null; /** Candidate file paths, supplied by read-only dry-run previews. */ readonly files?: readonly string[]; + readonly reason?: "other-filesystem" | "filesystem-unknown" | null; }; export type UpgradeApplyCoverage = { @@ -134,6 +140,7 @@ export type UpgradeApplyResult = | "unknown-project" | "ambiguous-project" | "unknown-fix" + | "filesystem" | "snapshot"; readonly message: string; } @@ -154,6 +161,7 @@ export type UpgradeApplyResult = readonly notes: readonly string[]; readonly coverage: UpgradeApplyCoverage; readonly journal_path: string; + /** Agent recovery manifests; may equal coverage.manifest_path. No file content. */ readonly agent_backups: readonly string[]; }; @@ -171,6 +179,7 @@ export type Planned = { /** The checkout's Hack directory; null for machine-wide agent files. */ readonly projectDir: string | null; readonly skip: string | null; + readonly skipReason?: "other-filesystem" | "filesystem-unknown"; }; const ERROR_CODE = /^[A-Z0-9_]{1,40}$/; @@ -197,6 +206,8 @@ export type RunUpgradeApplyInput = { readonly collect: () => Promise; /** Test seams. */ readonly snapshotRoot?: string; + /** Read-only device probe seam for portable multi-filesystem regressions. */ + readonly filesystemDevice?: (path: string) => Promise; readonly agentBackupDir?: string; readonly now?: () => Date; readonly applyPluginMigration?: typeof applyPluginMigration; @@ -221,15 +232,12 @@ export async function runUpgradeApply( message: "The apply journal or snapshot is invalid; nothing was changed.", }; } - if ( - recovery !== null && - (input.only !== undefined || input.project !== undefined) - ) { + if (recovery !== null && hasResumeOverrides(input)) { return { status: "refused", reason: "snapshot", message: - "Resume uses the saved selection; do not combine --resume with --only or --project.", + "Resume uses the saved selection and snapshot; do not combine --resume with --only, --project or --snapshot-root.", }; } const only = recovery?.journal.filters.only ?? input.only ?? []; @@ -249,7 +257,7 @@ export async function runUpgradeApply( home: input.home, }); const notes = recovery?.journal.notes ?? fresh.notes; - const planned: Planned[] = + const selected: Planned[] = recovery === null ? fresh.planned : recovery.journal.targets.map((target) => @@ -259,6 +267,20 @@ export async function runUpgradeApply( home: input.home, }) ); + const planned = await planUpgradeFilesystems({ + targets: selected, + snapshotRoot: selectedSnapshotRoot(input, recovery), + files: (target) => previewTargetFiles(target, input.home), + device: input.filesystemDevice, + }); + if (input.snapshotRoot !== undefined && hasOtherFilesystem(planned)) { + return { + status: "refused", + reason: "filesystem", + message: + "The selected targets span filesystems or do not share the chosen snapshot root's filesystem. Select one project on that volume; nothing was written.", + }; + } const runnable = planned.filter((target) => target.skip === null); const skipped = planned .filter((target) => target.skip !== null) @@ -269,7 +291,7 @@ export async function runUpgradeApply( skipped, notes, }); - if (early !== null && shouldReturnEarly(recovery, input.dryRun)) { + if (early !== null) { return await previewEarlyResult({ result: early, planned, @@ -331,6 +353,23 @@ export async function runUpgradeApply( }); } +function hasResumeOverrides(input: RunUpgradeApplyInput): boolean { + return ( + input.only !== undefined || + input.project !== undefined || + input.snapshotRoot !== undefined + ); +} + +function selectedSnapshotRoot( + input: RunUpgradeApplyInput, + recovery: Awaited> +): string { + return ( + recovery?.snapshot.dir ?? input.snapshotRoot ?? defaultUpgradeSnapshotRoot() + ); +} + async function executeApplyRun(opts: { input: RunUpgradeApplyInput; recovery: Awaited>; @@ -392,10 +431,7 @@ async function executeApplyRun(opts: { target.fix === "agent-integrations" ? { id: `upgrade-apply-${snapshot.id}-${index}`, - path: - input.applyPluginMigration === undefined - ? snapshot.manifestPath - : join(backupDir, `upgrade-apply-${snapshot.id}-${index}.json`), + path: snapshot.manifestPath, } : null; // Before anything can change: this target started, with its backup. @@ -416,7 +452,7 @@ async function executeApplyRun(opts: { outcome( target, "refused", - "The target changed outside this run; restore or review it before resuming." + "Not changed: target identity or recoverable snapshot coverage could not be verified; inspect the target and snapshot before retrying." ) ); } else if ( @@ -839,6 +875,7 @@ function outcome( status, message, backup_manifest: null, + reason: target.skipReason ?? null, }; } @@ -936,7 +973,7 @@ function recoverPlannedTarget(opts: { ); let skip: string | null = null; if (target.project === null) { - skip = target.root === home ? null : SKIP_BLOCKED; + skip = normalize(target.root) === normalize(home) ? null : SKIP_BLOCKED; } else if ( checkout === undefined || unsafeRoots(report).has(normalize(target.root)) @@ -953,10 +990,6 @@ function recoverPlannedTarget(opts: { }; } -function shouldReturnEarly(recovery: unknown, dryRun: boolean): boolean { - return recovery === null || dryRun; -} - async function previewTargetFiles( target: Planned, home: string diff --git a/src/lib/upgrade-check-render.ts b/src/lib/upgrade-check-render.ts index 176f68a70..ce792a7a4 100644 --- a/src/lib/upgrade-check-render.ts +++ b/src/lib/upgrade-check-render.ts @@ -265,9 +265,16 @@ function stepLines(step: UpgradePlanStep): string[] { step.command === null ? ` ${step.order}. ${tag}${what}manual: ${step.action ?? step.why}${scope}` : ` ${step.order}. ${tag}${what}${step.command}${scope}`; + const skipped = step.applies + ? step.targets.filter((target) => !target.apply_eligible).length + : 0; + const line = + skipped === 0 + ? head + : `${head} (${skipped} blocked/unchecked; apply skips them)`; return step.count <= 1 && step.command !== null && step.action !== null - ? [head, ` ${step.action}`] - : [head]; + ? [line, ` ${step.action}`] + : [line]; } /** Blockers or unchecked items: a few named (with why for unchecked), the rest counted. */ diff --git a/src/lib/upgrade-plan.ts b/src/lib/upgrade-plan.ts index 3c0af16ef..6c34be01e 100644 --- a/src/lib/upgrade-plan.ts +++ b/src/lib/upgrade-plan.ts @@ -7,7 +7,7 @@ import type { UpgradeFindingStatus } from "./upgrade-check-project.ts"; /** * The upgrade plan (HACK-2188, HACK-2220): one step per kind of change, not * one per checkout. A machine finding is its own step. Checkout findings of - * the same id and status are one step that lists every checkout it covers + * the same id are one step that lists every checkout it covers * (`targets`, each with its own command), so a machine with 170 checkouts * gets a plan of about ten steps. * @@ -38,6 +38,10 @@ export type UpgradePlanTarget = { /** The registered project it belongs to. */ readonly project: string; readonly root: string; + /** This checkout's finding severity, before group escalation. */ + readonly status: UpgradeFindingStatus; + /** False when any required checkout check is blocked or incomplete. */ + readonly apply_eligible: boolean; /** This checkout's own command line; null when only `action` applies. */ readonly command: string | null; readonly args: readonly string[] | null; @@ -180,6 +184,10 @@ export function buildUpgradePlan(opts: { scope: project.name, project: project.project, root: project.root, + status: finding.status, + apply_eligible: !project.findings.some( + (f) => f.status === "blocker" || f.status === "incomplete" + ), command: finding.command, args: finding.args, action: finding.action, diff --git a/tests/upgrade-apply.test.ts b/tests/upgrade-apply.test.ts index 5ae092261..b1797f8bd 100644 --- a/tests/upgrade-apply.test.ts +++ b/tests/upgrade-apply.test.ts @@ -17,6 +17,7 @@ import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { renderCodexSkill } from "../src/agents/codex-skill.ts"; import { HACK_PLUGIN_ID } from "../src/agents/plugin-replacement.ts"; +import { withProjectsRegistryLock } from "../src/lib/projects-registry-lock.ts"; import { runUpgradeApply } from "../src/lib/upgrade-apply.ts"; import { runUpgradeCheck, @@ -745,12 +746,9 @@ test("the agent backup manifest is journaled before removal and kept when the mi }; const current = journal.targets.find((t) => t.status === "started"); seenDuring.push(current?.backup_manifest ?? null); - expect(current?.backup_manifest).toBe( - join(input.backupDir, `${input.hooks?.id}.json`) - ); - // The migration committed its backup and removed files, then failed. - await mkdir(input.backupDir, { recursive: true }); - await writeFile(join(input.backupDir, `${input.hooks?.id}.json`), "{}\n"); + expect(current?.backup_manifest).toBe(input.upgradeSnapshotManifest); + // Production and injected migrations receive the same snapshot mode. + expect(input.upgradeSnapshotManifest).toBe(snapshot?.manifestPath); throw Object.assign(new Error("rewrite failed"), { code: "EIO" }); }, }); @@ -762,17 +760,17 @@ test("the agent backup manifest is journaled before removal and kept when the mi const failed = applied.targets.filter((t) => t.status === "failed"); expect(failed.length).toBeGreaterThan(0); for (const target of failed) { - expect(target.backup_manifest).toStartWith(backupDir); + expect(target.backup_manifest).toBe(manifestPath); expect(target.message).toContain("Failed (EIO)"); } - expect(applied.agent_backups).toHaveLength(failed.length); + expect(applied.agent_backups).toEqual([manifestPath]); const journal = (await readUpgradeSnapshotJournal({ manifestPath })) as { targets: { status: string; backup_manifest: string | null }[]; }; expect( journal.targets .filter((t) => t.status === "failed") - .every((t) => t.backup_manifest?.startsWith(backupDir)) + .every((t) => t.backup_manifest === manifestPath) ).toBe(true); expect(seenDuring).toEqual(failed.map((t) => t.backup_manifest)); }, 60_000); @@ -835,51 +833,44 @@ test("--project is ambiguous when another registration's primary is dead but its expect(await tree(root)).toEqual(before); }, 60_000); -test("a backup that cannot be inspected after a failed migration keeps its journaled path", async () => { +test("snapshot-backed failures retain their receipt without using a legacy backup directory", async () => { await staleHomeAgentFile(); const a = await project("app-a"); await writeRegistry([a]); const report = await check(); const backupDir = join(hackHome, "agent-migrations"); - try { - const applied = await runUpgradeApply({ - report, - dryRun: false, - only: ["agent-integrations"], - home, - collect: collectUpgradeSnapshotTargets, - agentBackupDir: backupDir, - applyPluginMigration: async (input) => { - await mkdir(input.backupDir, { recursive: true }); - await writeFile( - join(input.backupDir, `${input.hooks?.id}.json`), - "{}\n" - ); - // The backup directory becomes unreadable: existence is unknown. - await chmod(input.backupDir, 0o000); - throw Object.assign(new Error("rewrite failed"), { code: "EIO" }); - }, - }); - if (applied.status !== "partial") { - throw new Error(`unexpected status ${applied.status}`); - } - const failed = applied.targets.filter((t) => t.status === "failed"); - expect(failed.length).toBeGreaterThan(0); - expect(failed.every((t) => t.backup_manifest?.startsWith(backupDir))).toBe( - true - ); - expect(applied.agent_backups).toHaveLength(failed.length); - const journal = (await readUpgradeSnapshotJournal({ - manifestPath: applied.coverage.manifest_path, - })) as { targets: { status: string; backup_manifest: string | null }[] }; - expect( - journal.targets - .filter((t) => t.status === "failed") - .every((t) => t.backup_manifest?.startsWith(backupDir)) - ).toBe(true); - } finally { - await chmod(backupDir, 0o700).catch(() => undefined); + await writeFile(backupDir, "not a directory\n"); + const applied = await runUpgradeApply({ + report, + dryRun: false, + only: ["agent-integrations"], + home, + collect: collectUpgradeSnapshotTargets, + agentBackupDir: backupDir, + applyPluginMigration: async (input) => { + expect(input.upgradeSnapshotManifest).toBeString(); + expect(await lstat(input.upgradeSnapshotManifest ?? "")).toBeDefined(); + throw Object.assign(new Error("rewrite failed"), { code: "EIO" }); + }, + }); + if (applied.status !== "partial") { + throw new Error(`unexpected status ${applied.status}`); } + const failed = applied.targets.filter((t) => t.status === "failed"); + expect(failed.length).toBeGreaterThan(0); + expect( + failed.every((t) => t.backup_manifest === applied.coverage.manifest_path) + ).toBe(true); + expect(applied.agent_backups).toEqual([applied.coverage.manifest_path]); + const journal = (await readUpgradeSnapshotJournal({ + manifestPath: applied.coverage.manifest_path, + })) as { targets: { status: string; backup_manifest: string | null }[] }; + expect( + journal.targets + .filter((t) => t.status === "failed") + .every((t) => t.backup_manifest === applied.coverage.manifest_path) + ).toBe(true); + expect(await readFile(backupDir, "utf8")).toBe("not a directory\n"); }, 60_000); test("SIGKILL at every apply boundary resumes the same snapshot and rolls back exact bytes and modes", async () => { @@ -1129,3 +1120,166 @@ test("resume refuses damaged snapshot originals before applying another fix", as expect(resumed.status).toBe("partial"); expect(await tree(join(root, "repos"))).toEqual(before); }, 60_000); + +test("other-volume checkouts are skipped before any write with an accurate remedy, while local work applies", async () => { + const a = await project("app-a"); + const b = await project("app-b"); + await writeRegistry([a, b]); + const report = await check(); + const device = async (path: string) => (path.startsWith(b.repoRoot) ? 2 : 1); + const before = await tree(root); + const onlyRemote = await runUpgradeApply({ + report, + home, + dryRun: false, + only: ["ignore-rules"], + project: "app-b", + filesystemDevice: device, + collect: async () => { + throw new Error("Blocked planning must not collect or create a snapshot"); + }, + }); + expect(onlyRemote.status).toBe("blocked"); + if (onlyRemote.status !== "blocked") { + throw new Error("Expected skipped layout"); + } + expect(onlyRemote.targets[0]?.reason).toBe("other-filesystem"); + expect(onlyRemote.targets[0]?.message).toContain("--snapshot-root"); + expect(onlyRemote.targets[0]?.message).not.toContain("changed outside"); + expect(await tree(root)).toEqual(before); + const remoteBefore = await tree(b.repoRoot); + const applied = await runUpgradeApply({ + report, + home, + dryRun: false, + only: ["ignore-rules"], + filesystemDevice: device, + collect: collectUpgradeSnapshotTargets, + }); + expect(applied.status).toBe("partial"); + if (applied.status !== "partial") { + throw new Error("Expected partial apply"); + } + expect( + applied.targets.find((target) => target.root === a.repoRoot)?.status + ).toBe("applied"); + expect( + applied.targets.find((target) => target.root === b.repoRoot)?.reason + ).toBe("other-filesystem"); + expect(await tree(b.repoRoot)).toEqual(remoteBefore); + const refused = await runUpgradeApply({ + report, + home, + dryRun: false, + only: ["ignore-rules"], + snapshotRoot: join(root, "chosen-snapshots"), + filesystemDevice: device, + collect: async () => { + throw new Error( + "Mixed explicit-root selection must not create a snapshot" + ); + }, + }); + expect(refused).toMatchObject({ status: "refused", reason: "filesystem" }); +}, 60_000); + +test("an explicitly selected snapshot root admits its volume and is retained by resume", async () => { + const a = await project("app-a"); + await writeRegistry([a]); + const chosenRoot = join(root, "chosen-snapshots"); + const device = async (path: string) => + path.startsWith(a.repoRoot) || path.startsWith(chosenRoot) ? 2 : 1; + const result = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + only: ["ignore-rules"], + project: "app-a", + snapshotRoot: chosenRoot, + filesystemDevice: device, + collect: collectUpgradeSnapshotTargets, + }); + expect(result.status).toBe("applied"); + if (result.status !== "applied") { + throw new Error("Expected apply"); + } + expect(result.coverage.manifest_path).toStartWith(chosenRoot); + const blockedReport = await check(); + const target = blockedReport.projects[0]; + if (target === undefined) { + throw new Error("Missing checkout"); + } + const report = { + ...blockedReport, + projects: [ + { + ...target, + findings: [ + ...target.findings, + { + id: "fixture-blocker", + status: "blocker" as const, + summary: "Blocked fixture", + command: null, + args: null, + action: null, + }, + ], + }, + ], + }; + const before = await tree(root); + const blocked = await runUpgradeApply({ + report, + home, + dryRun: false, + resumeManifest: result.coverage.manifest_path, + filesystemDevice: device, + collect: collectUpgradeSnapshotTargets, + }); + expect(blocked.status).toBe("blocked"); + expect(await tree(root)).toEqual(before); + expect( + ( + await restoreUpgradeSnapshot({ + manifestPath: result.coverage.manifest_path, + }) + ).status + ).toBe("restored"); +}, 60_000); + +test("CLI rollback refuses a snapshot held by apply's lock without touching files", async () => { + const a = await project("app-a"); + await writeRegistry([a]); + const applied = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + only: ["ignore-rules"], + collect: collectUpgradeSnapshotTargets, + }); + if (applied.status !== "applied") { + throw new Error("Expected apply"); + } + // Prepare the harness-owned Docker config and HOME mode before the oracle. + privateCliEnv({ root, home }); + await withProjectsRegistryLock({ + lockPath: join(applied.coverage.manifest_path, "..", "apply.lock"), + run: async () => { + const before = await tree(root); + const started = Date.now(); + const result = await cli([ + "upgrade", + "rollback", + "--rollback-manifest", + applied.coverage.manifest_path, + "--json", + ]); + expect(result.code).toBe(1); + expect(JSON.parse(result.stdout).data.status).toBe("busy"); + expect(result.output).not.toContain(SENTINEL); + expect(Date.now() - started).toBeLessThan(10_000); + expect(await tree(root)).toEqual(before); + }, + }); +}, 60_000); diff --git a/tests/upgrade-check.test.ts b/tests/upgrade-check.test.ts index 655d4d81d..a4ac6d7a8 100644 --- a/tests/upgrade-check.test.ts +++ b/tests/upgrade-check.test.ts @@ -296,6 +296,12 @@ test("the plan starts with the snapshot, then blockers, then machine and project expect(report.next_command).toBe("hack upgrade snapshot"); const grouped = report.plan.filter((step) => step.id === "env-format"); expect(grouped).toHaveLength(1); + expect( + grouped[0]?.targets.find((target) => target.scope === "app-blocked")?.status + ).toBe("blocker"); + expect( + grouped[0]?.targets.find((target) => target.scope === "app-legacy")?.status + ).toBe("warn"); expect(grouped[0]?.targets.map((target) => target.scope).sort()).toEqual([ "app-blocked", "app-legacy", From 571611065f43caeda0e76a26d07bc7f5330fc336 Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 03:56:48 +0000 Subject: [PATCH 6/7] fix(upgrade): retain recovery evidence across interrupted resumes --- docs/reference/v5-model-coverage.md | 6 + docs/versions/v5/guides/upgrade-from-v4.md | 5 + src/lib/upgrade-apply-recovery.ts | 26 ++-- src/lib/upgrade-apply.ts | 25 ++-- tests/fixtures/upgrade-apply-worker.ts | 7 +- tests/upgrade-apply.test.ts | 135 +++++++++++++++++++++ 6 files changed, 182 insertions(+), 22 deletions(-) diff --git a/docs/reference/v5-model-coverage.md b/docs/reference/v5-model-coverage.md index 923bf80be..8343cff12 100644 --- a/docs/reference/v5-model-coverage.md +++ b/docs/reference/v5-model-coverage.md @@ -193,3 +193,9 @@ Uncovered: power-loss durability, arbitrary filesystem-call interruption, unsynchronized external writers, multiple independent snapshots writing the same checkout, service-manager changes and env migration's separate journal. No whole-machine upgrade or native runtime qualification follows from these tests. + +Resume publication retains saved target outcomes until verification, and any +recorded snapshot after-state remains authoritative after a refused retry. Real +SIGKILL regressions at both journal publication boundaries preserve a user's +revert through a subsequent resume. Missing or damaged original agent blobs refuse +resume even when the removed target is absent; absence alone is not recovery proof. diff --git a/docs/versions/v5/guides/upgrade-from-v4.md b/docs/versions/v5/guides/upgrade-from-v4.md index 0188bb7e2..ebcd04fbd 100644 --- a/docs/versions/v5/guides/upgrade-from-v4.md +++ b/docs/versions/v5/guides/upgrade-from-v4.md @@ -928,3 +928,8 @@ The remaining record must fill these gaps, each **to be confirmed in the dogfood - Full application/data checks beyond the three-project start/restart/stop trials, and restored v4 data. - Linux DNS, host/browser trust and daemon service availability at the tested revision. - Explicit agent-client loading, native setup eligibility, and the actual rollback transcript. + +Keep the printed resume and rollback commands for a custom `--snapshot-root`: +ordinary snapshot listings scan only the default root. Resume preserves completed +outcomes across interruption and refuses a damaged original blob even when its +agent file has already been removed. diff --git a/src/lib/upgrade-apply-recovery.ts b/src/lib/upgrade-apply-recovery.ts index b1df65f2e..1e6ba6f3a 100644 --- a/src/lib/upgrade-apply-recovery.ts +++ b/src/lib/upgrade-apply-recovery.ts @@ -122,13 +122,8 @@ export async function verifyApplyInput(opts: { } try { const current = await readApplyState(path); - if ( - opts.previous?.status === "applied" || - opts.previous?.status === "unchanged" - ) { - return sameApplyState(current.state, entry.after ?? entry.before) - ? "safe" - : "conflict"; + if (entry.after !== null) { + return sameApplyState(current.state, entry.after) ? "safe" : "conflict"; } if (sameApplyState(current.state, entry.before)) { return "safe"; @@ -175,17 +170,22 @@ export async function verifyAgentInput(opts: { previous?: UpgradeApplyTarget; }): Promise<"safe" | "conflict"> { try { - for (const path of agentPaths(opts.snapshot, opts.target)) { + const paths = agentPaths(opts.snapshot, opts.target); + if ( + !(await verifyUpgradeSnapshotRecovery({ + manifestPath: opts.snapshot.manifestPath, + paths, + })) + ) { + return "conflict"; + } + for (const path of paths) { const entry = opts.snapshot.manifest.entries.find((e) => e.path === path); if (entry === undefined) { return "conflict"; } const current = await readApplyState(path); - if ( - (opts.previous?.status === "applied" || - opts.previous?.status === "unchanged") && - entry.after !== null - ) { + if (entry.after !== null) { if (!sameApplyState(current.state, entry.after)) { return "conflict"; } diff --git a/src/lib/upgrade-apply.ts b/src/lib/upgrade-apply.ts index 95d273a98..fd6344ab8 100644 --- a/src/lib/upgrade-apply.ts +++ b/src/lib/upgrade-apply.ts @@ -393,6 +393,10 @@ async function executeApplyRun(opts: { const now = () => (input.now?.() ?? new Date()).toISOString(); const startedAt = recovery?.journal.started_at ?? now(); const results: UpgradeApplyTarget[] = [...skipped]; + const previousTarget = (target: Pick) => + recovery?.journal.targets.find( + (saved) => saved.fix === target.fix && saved.root === target.root + ); const journal = ( state: UpgradeApplyJournal["state"], current: UpgradeApplyTarget | null @@ -400,7 +404,9 @@ async function executeApplyRun(opts: { const done = results.length - skipped.length; const pending = runnable .slice(done + (current === null ? 0 : 1)) - .map((target) => outcome(target, "planned", null)); + .map( + (target) => previousTarget(target) ?? outcome(target, "planned", null) + ); return { version: UPGRADE_APPLY_JOURNAL_VERSION, kind: UPGRADE_APPLY_JOURNAL_KIND, @@ -427,6 +433,7 @@ async function executeApplyRun(opts: { const backupDir = input.agentBackupDir ?? join(resolveGlobalHackDir(), "agent-migrations"); for (const [index, target] of runnable.entries()) { + const previous = previousTarget(target); const backup = target.fix === "agent-integrations" ? { @@ -437,15 +444,17 @@ async function executeApplyRun(opts: { // Before anything can change: this target started, with its backup. await writeUpgradeSnapshotJournal({ snapshot, - journal: journal("applying", { - ...outcome(target, "started", null), - backup_manifest: backup?.path ?? null, - }), + journal: journal( + "applying", + previous?.status === "applied" || previous?.status === "unchanged" + ? previous + : { + ...outcome(target, "started", null), + backup_manifest: backup?.path ?? null, + } + ), }); await input.checkpoint?.("target-started"); - const previous = recovery?.journal.targets.find( - (t) => t.fix === target.fix && t.root === target.root - ); const safe = await verifyApplyInput({ snapshot, target, previous }); if (safe === "conflict") { results.push( diff --git a/tests/fixtures/upgrade-apply-worker.ts b/tests/fixtures/upgrade-apply-worker.ts index f62deec17..2f2944f73 100644 --- a/tests/fixtures/upgrade-apply-worker.ts +++ b/tests/fixtures/upgrade-apply-worker.ts @@ -8,11 +8,16 @@ const report = JSON.parse( await readFile(process.argv[2] ?? "", "utf8") ) as UpgradeCheckReport; const boundary = Number(process.argv[3]); +const resumeManifest = process.argv[5]; let checkpoint = 0; await runUpgradeApply({ report, home: process.env.HOME ?? "", - only: [process.argv[4] ?? "ignore-rules"], + only: + resumeManifest === undefined + ? [process.argv[4] ?? "ignore-rules"] + : undefined, + resumeManifest, dryRun: false, collect: collectUpgradeSnapshotTargets, checkpoint: async () => { diff --git a/tests/upgrade-apply.test.ts b/tests/upgrade-apply.test.ts index b1797f8bd..ce1dafef9 100644 --- a/tests/upgrade-apply.test.ts +++ b/tests/upgrade-apply.test.ts @@ -1283,3 +1283,138 @@ test("CLI rollback refuses a snapshot held by apply's lock without touching file }, }); }, 60_000); + +test("a resume killed while republishing its journal never adopts a user's revert", async () => { + const a = await project("app-a"); + await writeRegistry([a]); + const ignore = join(a.projectDir, ".gitignore"); + await writeFile(ignore, "original\n"); + const applied = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + only: ["ignore-rules"], + collect: collectUpgradeSnapshotTargets, + }); + if (applied.status !== "applied") { + throw new Error("Expected apply"); + } + await writeFile(ignore, "original\n"); + const reportPath = join(root, "resume-report.json"); + await writeFile(reportPath, JSON.stringify(await check())); + for (const boundary of [1, 2]) { + const child = Bun.spawn( + [ + process.execPath, + resolve(import.meta.dir, "fixtures/upgrade-apply-worker.ts"), + reportPath, + String(boundary), + "ignore-rules", + applied.coverage.manifest_path, + ], + { + cwd: root, + env: privateCliEnv({ + root, + home, + env: { + HACK_HOME: hackHome, + HACK_GLOBAL_CONFIG_PATH: join(hackHome, "hack.config.json"), + HACK_HOST_PROBES_DISABLE: "1", + HACK_SYSTEMD_DISABLE: "1", + }, + }), + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + } + ); + const reader = child.stdout.getReader(); + try { + const reached = await reader.read(); + expect(new TextDecoder().decode(reached.value)).toContain("checkpoint"); + } finally { + child.kill("SIGKILL"); + await child.exited; + reader.releaseLock(); + } + const journal = (await readUpgradeSnapshotJournal({ + manifestPath: applied.coverage.manifest_path, + })) as { targets: { status: string }[] }; + expect(journal.targets[0]?.status).toBe("applied"); + expect(await readFile(ignore, "utf8")).toBe("original\n"); + } + const resumed = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + resumeManifest: applied.coverage.manifest_path, + collect: collectUpgradeSnapshotTargets, + }); + expect(resumed.status).toBe("partial"); + expect(await readFile(ignore, "utf8")).toBe("original\n"); + // Even another resume after the refusal must retain the after-state guard. + expect( + ( + await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + resumeManifest: applied.coverage.manifest_path, + collect: collectUpgradeSnapshotTargets, + }) + ).status + ).toBe("partial"); + expect(await readFile(ignore, "utf8")).toBe("original\n"); +}, 60_000); + +test("an absent agent target cannot resume with a damaged original blob", async () => { + const a = await project("app-a"); + await writeRegistry([a]); + const agent = await staleHomeAgentFile(); + await writeFile(agent, renderCodexSkill()); + await writeFile( + join(home, ".codex/config.toml"), + `[plugins."${HACK_PLUGIN_ID}"]\nenabled = true\n` + ); + await writeFile( + join(a.repoRoot, "AGENTS.md"), + "\nold\n\n" + ); + const applied = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + only: ["agent-integrations"], + collect: collectUpgradeSnapshotTargets, + }); + if (applied.status !== "applied") { + throw new Error("Expected agent apply"); + } + const [snapshot] = await listUpgradeSnapshots(); + const entry = snapshot?.manifest.entries.find((item) => item.path === agent); + if (snapshot === undefined || entry?.before.kind !== "file") { + throw new Error("Missing blob"); + } + await writeFile( + join(snapshot.dir, "blobs", entry.before.sha256), + "damaged\n", + { mode: 0o600 } + ); + expect(await lstat(agent).catch(() => null)).toBeNull(); + const resumed = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + resumeManifest: applied.coverage.manifest_path, + collect: collectUpgradeSnapshotTargets, + }); + expect(resumed.status).toBe("partial"); + if (resumed.status !== "partial") { + throw new Error("Expected refusal"); + } + expect(resumed.targets.find((target) => target.root === home)?.status).toBe( + "refused" + ); + expect(await lstat(agent).catch(() => null)).toBeNull(); +}, 60_000); From 2fb92304db0b3213ea6ca949b5ab49ef8d32e64f Mon Sep 17 00:00:00 2001 From: Kit <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 04:05:46 +0000 Subject: [PATCH 7/7] fix(upgrade): record successful agent removal retries --- docs/reference/v5-model-coverage.md | 5 ++ docs/versions/v5/guides/upgrade-from-v4.md | 2 + src/lib/upgrade-apply-recovery.ts | 27 ++++++++- src/lib/upgrade-apply.ts | 16 +++++- tests/upgrade-apply.test.ts | 67 ++++++++++++++++++++++ 5 files changed, 113 insertions(+), 4 deletions(-) diff --git a/docs/reference/v5-model-coverage.md b/docs/reference/v5-model-coverage.md index 8343cff12..373c7444b 100644 --- a/docs/reference/v5-model-coverage.md +++ b/docs/reference/v5-model-coverage.md @@ -199,3 +199,8 @@ recorded snapshot after-state remains authoritative after a refused retry. Real SIGKILL regressions at both journal publication boundaries preserve a user's revert through a subsequent resume. Missing or damaged original agent blobs refuse resume even when the removed target is absent; absence alone is not recovery proof. +Pre-write agent admission enforces the saved after-state; post-write verification +admits only the owner's authorized removal output before advancing that state. +An unchanged injected failure followed by a real removal/restore regression +covers successful retry without `--overwrite`; it does not qualify OS permission +failures or arbitrary concurrent writers. diff --git a/docs/versions/v5/guides/upgrade-from-v4.md b/docs/versions/v5/guides/upgrade-from-v4.md index ebcd04fbd..826d67279 100644 --- a/docs/versions/v5/guides/upgrade-from-v4.md +++ b/docs/versions/v5/guides/upgrade-from-v4.md @@ -137,6 +137,8 @@ and gaps; it cannot be combined with `--only`, `--project` or `--snapshot-root`. It rechecks the registered checkouts, refuses unexpected file edits, and verifies the snapshot's original blobs before writing. It never chooses the latest run automatically. +If an agent removal failed without changing its file, a later retry can record +the successful removal while keeping ordinary rollback available. Rollback restores ignore rules and removed agent files together, with their exact bytes and modes. Files edited after apply require an explicit `--overwrite` to restore; their current copies are retained. Service state and installed versions diff --git a/src/lib/upgrade-apply-recovery.ts b/src/lib/upgrade-apply-recovery.ts index 1e6ba6f3a..75d26d596 100644 --- a/src/lib/upgrade-apply-recovery.ts +++ b/src/lib/upgrade-apply-recovery.ts @@ -167,7 +167,24 @@ export function agentPaths( export async function verifyAgentInput(opts: { snapshot: UpgradeSnapshot; target: Planned; - previous?: UpgradeApplyTarget; +}): Promise<"safe" | "conflict"> { + return await verifyAgentState({ ...opts, output: false, removedPaths: [] }); +} + +/** Authorized removals may advance an earlier attempt's unchanged after-state. */ +export async function verifyAgentOutput(opts: { + snapshot: UpgradeSnapshot; + target: Planned; + removedPaths: readonly string[]; +}): Promise<"safe" | "conflict"> { + return await verifyAgentState({ ...opts, output: true }); +} + +async function verifyAgentState(opts: { + snapshot: UpgradeSnapshot; + target: Planned; + output: boolean; + removedPaths: readonly string[]; }): Promise<"safe" | "conflict"> { try { const paths = agentPaths(opts.snapshot, opts.target); @@ -185,7 +202,13 @@ export async function verifyAgentInput(opts: { return "conflict"; } const current = await readApplyState(path); - if (entry.after !== null) { + if (opts.output && opts.removedPaths.includes(path)) { + if (current.state.kind !== "absent") { + return "conflict"; + } + continue; + } + if (!opts.output && entry.after !== null) { if (!sameApplyState(current.state, entry.after)) { return "conflict"; } diff --git a/src/lib/upgrade-apply.ts b/src/lib/upgrade-apply.ts index fd6344ab8..d66688724 100644 --- a/src/lib/upgrade-apply.ts +++ b/src/lib/upgrade-apply.ts @@ -68,7 +68,7 @@ import { UPGRADE_APPLY_JOURNAL_KIND as JOURNAL_KIND, UPGRADE_APPLY_JOURNAL_VERSION as JOURNAL_VERSION, loadApplyRecovery, - verifyAgentInput, + verifyAgentOutput, verifyApplyInput, } from "./upgrade-apply-recovery.ts"; @@ -719,7 +719,19 @@ async function applyTarget(opts: { ) { const paths = agentPaths(opts.snapshot, target); if ( - (await verifyAgentInput({ snapshot: opts.snapshot, target })) !== "safe" + (await verifyAgentOutput({ + snapshot: opts.snapshot, + target, + removedPaths: + migrated.status === "applied" + ? migrated.results + .filter( + (entry) => + entry.status === "removed" || entry.status === "missing" + ) + .map((entry) => entry.path) + : [], + })) !== "safe" ) { return { ...result, status: "failed", message: NOT_RECORDED }; } diff --git a/tests/upgrade-apply.test.ts b/tests/upgrade-apply.test.ts index ce1dafef9..6a3955045 100644 --- a/tests/upgrade-apply.test.ts +++ b/tests/upgrade-apply.test.ts @@ -16,6 +16,7 @@ import { import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { renderCodexSkill } from "../src/agents/codex-skill.ts"; +import { applyPluginMigration } from "../src/agents/plugin-migration.ts"; import { HACK_PLUGIN_ID } from "../src/agents/plugin-replacement.ts"; import { withProjectsRegistryLock } from "../src/lib/projects-registry-lock.ts"; import { runUpgradeApply } from "../src/lib/upgrade-apply.ts"; @@ -1418,3 +1419,69 @@ test("an absent agent target cannot resume with a damaged original blob", async ); expect(await lstat(agent).catch(() => null)).toBeNull(); }, 60_000); + +test("a failed unchanged agent removal can retry successfully and roll back without overwrite", async () => { + const a = await project("app-a"); + await writeRegistry([a]); + const agent = await staleHomeAgentFile(); + const original = renderCodexSkill(); + await writeFile(agent, original); + await chmod(agent, 0o640); + await writeFile( + join(home, ".codex/config.toml"), + `[plugins."${HACK_PLUGIN_ID}"]\nenabled = true\n` + ); + await writeFile( + join(a.repoRoot, "AGENTS.md"), + "\nold\n\n" + ); + const first = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + only: ["agent-integrations"], + collect: collectUpgradeSnapshotTargets, + applyPluginMigration: async (input) => { + if (input.scopes[0]?.scope !== "global") { + return await applyPluginMigration(input); + } + return { + status: "applied", + plan: { checks: [], entries: [] }, + manifestPath: input.upgradeSnapshotManifest ?? "", + results: [ + { + scope: "global", + target: "codex-hack-cli-skill", + path: agent, + status: "error", + message: "Fixture removal refused without changing its input.", + }, + ], + }; + }, + }); + expect(first.status).toBe("partial"); + if (first.status !== "partial") { + throw new Error("Expected first-attempt failure"); + } + expect(await readFile(agent, "utf8")).toBe(original); + const resumed = await runUpgradeApply({ + report: await check(), + home, + dryRun: false, + resumeManifest: first.coverage.manifest_path, + collect: collectUpgradeSnapshotTargets, + }); + expect(resumed.status).toBe("applied"); + expect(await lstat(agent).catch(() => null)).toBeNull(); + expect( + ( + await restoreUpgradeSnapshot({ + manifestPath: first.coverage.manifest_path, + }) + ).status + ).toBe("restored"); + expect(await readFile(agent, "utf8")).toBe(original); + expect((await stat(agent)).mode & 0o777).toBe(0o640); +}, 60_000);