diff --git a/.claude/spec/gitea-axi.md b/.claude/spec/gitea-axi.md index 9af2465..76abd29 100644 --- a/.claude/spec/gitea-axi.md +++ b/.claude/spec/gitea-axi.md @@ -491,7 +491,7 @@ The dashboard's empty states are `prs: 0 open` / `issues: 0 open` (raw strings, Empty output is never silent. **Principle 6 — Structured errors, exit codes, idempotent mutations, no prompts.** -Errors are represented as a typed `AxiError` with one of ten named codes: `REPO_NOT_FOUND`, `ISSUE_NOT_FOUND`, `PR_NOT_FOUND`, `AUTH_REQUIRED`, `FORBIDDEN`, `RATE_LIMITED`, `TEA_NOT_INSTALLED`, `VALIDATION_ERROR`, `GIT_ERROR`, `UNKNOWN`. +Errors are represented as a typed `AxiError` with one of eleven named codes: `REPO_NOT_FOUND`, `ISSUE_NOT_FOUND`, `PR_NOT_FOUND`, `AUTH_REQUIRED`, `FORBIDDEN`, `RATE_LIMITED`, `TEA_NOT_INSTALLED`, `VALIDATION_ERROR`, `GIT_ERROR`, `TARGET_NOT_WRITABLE`, `UNKNOWN`. The `ISSUE_NOT_FOUND`/`PR_NOT_FOUND` split (vs gh-axi's single `NOT_FOUND`) is a deliberate divergence enabled by path-based 404 classification. API error responses are classified by HTTP status code and calling context: @@ -516,6 +516,8 @@ tea has logins but none match the detected hostname → `REPO_NOT_FOUND` (the re HTTP 401 from the API → `AUTH_REQUIRED` (token invalid or revoked), per the status table. A `--login` value naming a nonexistent profile is `VALIDATION_ERROR`, listing the available profile names. `GIT_ERROR` classifies non-zero git subprocess exits (currently only `pr checkout`), carrying git's first stderr line. +`TARGET_NOT_WRITABLE` classifies a `setup` target the filesystem refuses (`EACCES`, `EPERM`, `EROFS`), naming the file and pointing at the general remedy: it appears to be managed by another tool, so the skill or hook belongs in that tool's configuration. +It never names or infers a particular configuration manager — read-only is not diagnostic of one. Error output is TOON-encoded to stdout (not stderr): `error: `, `code: `, and optionally `help[N]:` with suggestion lines. The suggestions field is named `help`, not `hint`. Exit codes: 0 success, 1 error, 2 for `VALIDATION_ERROR` — covering unknown flags, missing required inputs, and server-side 422 rejections alike (the `axi-sdk-js` `exitCodeForError` mapping; see ADR 0004). diff --git a/.claude/tasks/0044-setup-fails-clean-on-unwritable-targets.md b/.claude/tasks/0044-setup-fails-clean-on-unwritable-targets.md index 1e27d12..f6504c6 100644 --- a/.claude/tasks/0044-setup-fails-clean-on-unwritable-targets.md +++ b/.claude/tasks/0044-setup-fails-clean-on-unwritable-targets.md @@ -21,9 +21,32 @@ The failure follows the CLI's existing error convention rather than inventing a ## Acceptance criteria -- [ ] An unwritable skill target produces a structured CLI error rather than an unhandled filesystem exception. -- [ ] An unwritable hook target produces the same class of error, with the same guidance. -- [ ] Both errors name the file that could not be written and state that it appears to be managed by another tool. -- [ ] Neither error names or infers a specific configuration manager. -- [ ] A skill target that is unwritable but already byte-identical to the bundled copy succeeds rather than failing, since nothing needs to be written. -- [ ] The errors carry a code and help lines consistent with the rest of the CLI's error surface. +- [x] An unwritable skill target produces a structured CLI error rather than an unhandled filesystem exception. +- [x] An unwritable hook target produces the same class of error, with the same guidance. +- [x] Both errors name the file that could not be written and state that it appears to be managed by another tool. +- [x] Neither error names or infers a specific configuration manager. +- [x] A skill target that is unwritable but already byte-identical to the bundled copy succeeds rather than failing, since nothing needs to be written. +- [x] The errors carry a code and help lines consistent with the rest of the CLI's error surface. + +## Implementation Notes + +The condition is `EACCES`, `EPERM`, or `EROFS` — the three ways a filesystem refuses a write for a reason the user has to settle outside this tool. +The new `TARGET_NOT_WRITABLE` code joins the spec's enumerated list, alongside a paragraph describing it. + +Two things came out of review and go slightly beyond the literal criteria. + +The skill half now guards its comparison read as well as its write. +A target the filesystem will not let us read is one it will not let us replace either — the same condition reached one call earlier — so a mode-`000` file reports the same error rather than the raw exception the criteria were written against. + +The hook half collects its failures as `{path, detail}` records rather than the agent SDK's flattened `: ` text. +The SDK reports through a string, so the two halves are separated once at that boundary and judged apart. +This matters for correctness, not just shape: testing the whole formatted string for an errno would misclassify an unrelated failure whose *path* happened to contain `EACCES`. + +Two known limits, both judged acceptable rather than fixed. + +The hook error names the target the SDK was writing, which is the intended path rather than necessarily the blocking one — if `~/.claude` were unwritable and `settings.json` absent, it would name the file rather than the directory. +The SDK discards the error object, so its `path` is not recoverable; the skill half, which catches its own errors, does report the blocking path and is tested for it. + +An unwritable `~/.claude/settings.json` still leaves the Codex and OpenCode integrations installed, because the SDK writes them before the failure surfaces. +The command exits 1 having done part of its work. +Making the hook install transactional across three integrations owned by the SDK is a larger change than this task, and re-running after fixing the permission converges correctly. diff --git a/src/commands/setup.ts b/src/commands/setup.ts index 61c8a8f..0d94997 100644 --- a/src/commands/setup.ts +++ b/src/commands/setup.ts @@ -4,7 +4,12 @@ import { join } from "node:path"; import { fileURLToPath } from "node:url"; import { installSessionStartHooks } from "axi-sdk-js"; import type { CliDeps } from "../deps.js"; -import { axiError } from "../errors.js"; +import { + axiError, + isNotWritableError, + isNotWritableMessage, + unwritableTargetError, +} from "../errors.js"; import { pruneDuplicateManagedHooks, resolveEntrypointOnPath } from "../hooks.js"; import { renderDetail } from "../render.js"; @@ -65,15 +70,30 @@ function installSkill(home: string): { skill: string; path: string; status: Skil const targetPath = join(targetDir, "SKILL.md"); let status: SkillStatus; - if (!existsSync(targetPath)) { - status = "installed"; - } else { - status = readFileSync(targetPath, "utf8") === source ? "unchanged" : "updated"; - } + try { + if (!existsSync(targetPath)) { + status = "installed"; + } else { + status = readFileSync(targetPath, "utf8") === source ? "unchanged" : "updated"; + } - if (status !== "unchanged") { - mkdirSync(targetDir, { recursive: true }); - writeFileSync(targetPath, source, "utf8"); + // A target that already holds these bytes needs no write, so its being + // read-only is beside the point and the run succeeds. + if (status !== "unchanged") { + mkdirSync(targetDir, { recursive: true }); + writeFileSync(targetPath, source, "utf8"); + } + } catch (error) { + // The comparison read is inside the guard because a target the filesystem + // will not let us read is one it will not let us replace either — the same + // condition, reached one call earlier. + if (isNotWritableError(error)) { + // The path the filesystem names is the directory when it is the directory + // that is read-only, so report that rather than assuming the file. + const blocked = (error as { path?: string }).path ?? targetPath; + throw unwritableTargetError(blocked, `the ${SKILL_NAME} skill`); + } + throw error; } return { skill: SKILL_NAME, path: collapseHome(targetPath, home), status }; @@ -99,6 +119,38 @@ const HOOK_SETTINGS_FILES = [ [".codex", "hooks.json"], ]; +/** One target the hook install could not write, and why. */ +interface TargetFailure { + /** The file being written when it failed. */ + path: string; + /** The underlying failure, with no path spliced into it. */ + detail: string; +} + +/** + * Recover a {@link TargetFailure} from the agent SDK's reporting. + * + * It hands its failures to `onError` as `: ` text rather than as + * errors, so the two halves have to be separated again before either can be + * judged on its own. Text in that shape is all it ever emits; anything else is + * returned whole as the detail, with no path to attribute it to. + */ +function parseReportedFailure(reported: string): TargetFailure { + const separator = reported.indexOf(": "); + if (separator === -1) { + return { path: "", detail: reported }; + } + return { + path: reported.slice(0, separator), + detail: reported.slice(separator + 2), + }; +} + +/** A failure rendered back as the `: ` line a reader sees. */ +function formatFailure({ path, detail }: TargetFailure): string { + return path === "" ? detail : `${path}: ${detail}`; +} + /** * Collapse any duplicate managed entry the SDK's own recognition missed. * @@ -109,7 +161,11 @@ const HOOK_SETTINGS_FILES = [ * command's shape — and cannot mistake another tool's hook for ours the way a * substring test can. */ -function pruneHookSettingsFiles(home: string, command: string, errors: string[]): void { +function pruneHookSettingsFiles( + home: string, + command: string, + errors: TargetFailure[], +): void { const isManaged = (recorded: string) => recorded === command; for (const segments of HOOK_SETTINGS_FILES) { @@ -124,14 +180,17 @@ function pruneHookSettingsFiles(home: string, command: string, errors: string[]) writeFileSync(target, `${JSON.stringify(settings, null, 2)}\n`, "utf8"); } } catch (error) { - errors.push(`${target}: ${error instanceof Error ? error.message : String(error)}`); + errors.push({ + path: target, + detail: error instanceof Error ? error.message : String(error), + }); } } } async function setupHooks(deps: CliDeps): Promise { const home = resolveHome(deps); - const errors: string[] = []; + const errors: TargetFailure[] = []; // ADR 0019: record a search-path name, not an install-tree path. Handing the // SDK where the binary resolves on PATH — rather than the module-relative @@ -156,13 +215,25 @@ async function setupHooks(deps: CliDeps): Promise { // deferring to the SDK's auto-install safety gate (which is tuned for the // inferred dist/bin/.js entrypoint layout gitea-axi does not use). shouldInstall: () => true, - onError: (message) => errors.push(message), + onError: (message) => errors.push(parseReportedFailure(message)), }); pruneHookSettingsFiles(home, command, errors); if (errors.length > 0) { - throw axiError(`Failed to install session hooks: ${errors.join("; ")}`, "UNKNOWN"); + // A read-only target is the most actionable thing that can be in here — it + // names something the user must settle elsewhere rather than a bug — so it + // is reported ahead of whatever else was collected. + const unwritable = errors.find( + (failure) => failure.path !== "" && isNotWritableMessage(failure.detail), + ); + if (unwritable) { + throw unwritableTargetError(unwritable.path, `the ${SKILL_NAME} session hook`); + } + throw axiError( + `Failed to install session hooks: ${errors.map(formatFailure).join("; ")}`, + "UNKNOWN", + ); } return renderDetail({ diff --git a/src/errors.ts b/src/errors.ts index 9b8bc96..affb1b3 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -10,6 +10,7 @@ export type AxiErrorCode = | "TEA_NOT_INSTALLED" | "VALIDATION_ERROR" | "GIT_ERROR" + | "TARGET_NOT_WRITABLE" | "UNKNOWN"; export function axiError( @@ -20,6 +21,50 @@ export function axiError( return new AxiError(message, code, suggestions); } +// The three ways a filesystem refuses a write for reasons the user must settle +// outside this tool: the permission bits deny it, the file is flagged immutable +// or otherwise protected, or the filesystem itself is mounted read-only. +const NOT_WRITABLE_ERRNOS = ["EACCES", "EPERM", "EROFS"]; + +/** Whether a caught filesystem error means the target cannot be written. */ +export function isNotWritableError(error: unknown): boolean { + const errno = (error as { code?: unknown } | null)?.code; + return typeof errno === "string" && NOT_WRITABLE_ERRNOS.includes(errno); +} + +/** + * The same judgement made from an error's *message*, for a caller handed the + * formatted text rather than the error object. + * + * This takes the message alone, never a string the target's path has been + * spliced into: a path is the user's to name, and one that happened to contain + * `EACCES` would otherwise misreport an unrelated failure as a read-only target. + */ +export function isNotWritableMessage(message: string): boolean { + return NOT_WRITABLE_ERRNOS.some((errno) => message.includes(errno)); +} + +/** + * A read-only target reported as something the user can act on. + * + * The remedy is deliberately general. Read-only is not diagnostic of any + * particular configuration manager, and every plausible cause — a declarative + * home manager, an immutable flag, a root-owned path — has the same answer: + * whatever renders the file read-only is where this belongs, not here. + * + * `subject` names what the caller was installing, for the remedy line. + */ +export function unwritableTargetError(path: string, subject: string): AxiError { + return axiError( + `Cannot write ${path}: it is not writable, so it appears to be managed by another tool`, + "TARGET_NOT_WRITABLE", + [ + `Declare ${subject} through that tool's configuration rather than installing it with this command`, + `Or make ${path} writable and re-run`, + ], + ); +} + interface HttpResponseLike { status: number; url: string; diff --git a/test/setup.test.ts b/test/setup.test.ts index 1edccef..c906819 100644 --- a/test/setup.test.ts +++ b/test/setup.test.ts @@ -1,4 +1,13 @@ -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + chmodSync, + existsSync, + mkdirSync, + mkdtempSync, + readdirSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { delimiter, isAbsolute, join } from "node:path"; import { fileURLToPath } from "node:url"; @@ -7,12 +16,49 @@ import { type CliResult, runCliTest } from "./harness.js"; let tempHome: string; +/** Restore write permission everywhere under `dir` so the tree can be removed. */ +function restorePermissions(dir: string): void { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const path = join(dir, entry.name); + chmodSync(path, entry.isDirectory() ? 0o700 : 0o600); + if (entry.isDirectory()) { + restorePermissions(path); + } + } +} + afterEach(() => { - if (tempHome) { + // Not every test creates a HOME, so this may be a directory an earlier test + // already removed. + if (tempHome && existsSync(tempHome)) { + // The read-only-target tests leave files and directories unwritable, and + // an unwritable directory cannot have its entries unlinked. + chmodSync(tempHome, 0o700); + restorePermissions(tempHome); rmSync(tempHome, { recursive: true, force: true }); } + tempHome = ""; }); +/** + * Permission bits are not enforced for root, so the read-only-target tests + * cannot express their premise there and are skipped rather than passing + * vacuously. + */ +const itUnlessRoot = process.getuid?.() === 0 ? it.skip : it; + +/** + * Assert the error's wording infers no particular configuration manager. + * + * Read-only is not diagnostic of one, so naming one would be wrong for most + * readers who hit this. The paths the error quotes are exempt — they are the + * user's own, and here the temp directory sits under a `nix-shell` TMPDIR. + */ +function expectNamesNoManager(stdout: string, home: string): void { + const wording = stdout.split(home).join(""); + expect(wording).not.toMatch(/\b(nix|home-manager|nixos|chezmoi|ansible|stow|guix)\b/i); +} + /** * Run `body` with `process.env.PATH` replaced. `setup hooks` reads PATH from the * process rather than the injected environment, because it has to agree with @@ -78,6 +124,60 @@ describe("setup", () => { expect(third.stdout).toContain("status: updated"); expect(readFileSync(installedPath, "utf8")).not.toBe("tampered"); }); + + itUnlessRoot("reports a read-only skill target as a structured error", async () => { + tempHome = mkdtempSync(join(tmpdir(), "gitea-axi-setup-")); + const installedPath = join(tempHome, ".claude", "skills", "gitea-axi", "SKILL.md"); + + // A declaratively managed install in miniature: the file is present, its + // content differs from the bundled copy, and it cannot be written. + mkdirSync(join(tempHome, ".claude", "skills", "gitea-axi"), { recursive: true }); + writeFileSync(installedPath, "managed elsewhere\n"); + chmodSync(installedPath, 0o444); + + const { stdout, exitCode } = await runCliTest(["setup"], { env: { HOME: tempHome } }); + + expect(exitCode).toBe(1); + expect(stdout).toContain("code: TARGET_NOT_WRITABLE"); + expect(stdout).toContain(installedPath); + expect(stdout).toContain("managed by another tool"); + expectNamesNoManager(stdout, tempHome); + // The bundled copy is untouched by a failed run. + expect(readFileSync(installedPath, "utf8")).toBe("managed elsewhere\n"); + }); + + itUnlessRoot("names the directory when it is the directory that is read-only", async () => { + tempHome = mkdtempSync(join(tmpdir(), "gitea-axi-setup-")); + const skillsDir = join(tempHome, ".claude", "skills"); + + // Nothing installed yet, and no new entry can be created here — so the + // blocked path is the directory, not the file that would have gone in it. + mkdirSync(skillsDir, { recursive: true }); + chmodSync(skillsDir, 0o555); + + const { stdout, exitCode } = await runCliTest(["setup"], { env: { HOME: tempHome } }); + + expect(exitCode).toBe(1); + expect(stdout).toContain("code: TARGET_NOT_WRITABLE"); + expect(stdout).toContain(join(skillsDir, "gitea-axi")); + expectNamesNoManager(stdout, tempHome); + }); + + itUnlessRoot("succeeds on a read-only skill target that is already up to date", async () => { + tempHome = mkdtempSync(join(tmpdir(), "gitea-axi-setup-")); + const installedPath = join(tempHome, ".claude", "skills", "gitea-axi", "SKILL.md"); + + const first = await runCliTest(["setup"], { env: { HOME: tempHome } }); + expect(first.exitCode).toBe(0); + + // Same bytes the command would write, so there is nothing to write and the + // target's being read-only is beside the point. + chmodSync(installedPath, 0o444); + + const { stdout, exitCode } = await runCliTest(["setup"], { env: { HOME: tempHome } }); + expect(exitCode).toBe(0); + expect(stdout).toContain("status: unchanged"); + }); }); describe("setup hooks", () => { @@ -179,6 +279,25 @@ describe("setup hooks", () => { expect(recordedHookCommand(tempHome)).toBe(entrypointPath()); }); + + itUnlessRoot("reports a read-only hook target as the same structured error", async () => { + tempHome = mkdtempSync(join(tmpdir(), "gitea-axi-setup-")); + const settingsPath = join(tempHome, ".claude", "settings.json"); + + mkdirSync(join(tempHome, ".claude"), { recursive: true }); + writeFileSync(settingsPath, "{}\n"); + chmodSync(settingsPath, 0o444); + + const { stdout, exitCode } = await runCliTest(["setup", "hooks"], { + env: { HOME: tempHome }, + }); + + expect(exitCode).toBe(1); + expect(stdout).toContain("code: TARGET_NOT_WRITABLE"); + expect(stdout).toContain(settingsPath); + expect(stdout).toContain("managed by another tool"); + expectNamesNoManager(stdout, tempHome); + }); }); describe("setup dispatch", () => {