From 0633988cb528cb2ea27bcd66bb08452e1c425f60 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Tue, 7 Jul 2026 13:21:53 +0200 Subject: [PATCH 1/3] =?UTF-8?q?feat:=20Phase=202=20=E2=80=94=20ct=20adopt?= =?UTF-8?q?=20+=20JSON=20state=20file?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Selective, opt-in management: bring one existing ChurchTools resource under management and record it in a committable JSON state file. Nothing outside the state file is visible to the tool. - src/state/state.ts: State schema (version, host, resources keyed by logical key), load/save, and an idempotent upsert (re-adopting the same type+id updates in place, re-keys on key change, rejects key collisions). All id comparisons are null-checked — CT ids can be 0 (Mainz campus). - src/resources/registry.ts: adoptable-type registry (campus, group, group-type) with item paths from the Phase 0 matrix, key derivation, managed-field snapshots, and TS-as-code config-snippet rendering. - ct adopt [--key] [--state] [--dry-run]: fetch by id, snapshot, upsert into state; --dry-run previews the generated config entry without writing. Host guard refuses to mix instances. - ct state list: show the managed set. - 17 new tests (state upsert/idempotency/id-0/rekey/collision, registry slug/paths/snippet, adopt write/idempotency/dry-run/unknown-type) = 34. Refs #4 --- .gitignore | 1 + src/commands/adopt.ts | 65 ++++++++++++++++++ src/commands/placeholders.ts | 1 - src/commands/state.ts | 26 ++++++++ src/index.ts | 4 ++ src/resources/registry.ts | 78 ++++++++++++++++++++++ src/state/state.ts | 123 +++++++++++++++++++++++++++++++++++ tests/adopt.test.ts | 65 ++++++++++++++++++ tests/registry.test.ts | 44 +++++++++++++ tests/state.test.ts | 53 +++++++++++++++ 10 files changed, 459 insertions(+), 1 deletion(-) create mode 100644 src/commands/adopt.ts create mode 100644 src/commands/state.ts create mode 100644 src/resources/registry.ts create mode 100644 src/state/state.ts create mode 100644 tests/adopt.test.ts create mode 100644 tests/registry.test.ts create mode 100644 tests/state.test.ts diff --git a/.gitignore b/.gitignore index e884388..8098f6f 100644 --- a/.gitignore +++ b/.gitignore @@ -10,6 +10,7 @@ dist/ # Local ChurchTools state / adopted config (belongs in eqrm/ct-structure, never here) *.ctstate.json +ct-state.json # Generated typed client (regenerated from the live OpenAPI spec) src/api/schema.d.ts diff --git a/src/commands/adopt.ts b/src/commands/adopt.ts new file mode 100644 index 0000000..9511d0b --- /dev/null +++ b/src/commands/adopt.ts @@ -0,0 +1,65 @@ +import { Command } from "commander"; +import { authedSession } from "../api/session.js"; +import { resolveConfig } from "../config.js"; +import { resourceType, configSnippet } from "../resources/registry.js"; +import { loadState, saveState, resolveStatePath, upsert } from "../state/state.js"; +import { success, info, warn, out } from "../ui.js"; + +interface AdoptOptions { + key?: string; + state?: string; + dryRun?: boolean; +} + +export function adoptCommand(): Command { + return new Command("adopt") + .description("Put one existing ChurchTools resource under management (adds it to the state file)") + .argument("", "resource type, e.g. campus | group | group-type") + .argument("", "ChurchTools id of the resource") + .option("-k, --key ", "logical key (defaults to a slug of the resource name)") + .option("-s, --state ", "state file path (or set CT_STATE)") + .option("--dry-run", "preview the config entry and state change without writing") + .action(async (type: string, rawId: string, opts: AdoptOptions) => { + const spec = resourceType(type); + const id = Number.parseInt(rawId, 10); + if (!Number.isInteger(id)) { + throw new Error(`Invalid id "${rawId}" — expected an integer.`); + } + + const { client } = await authedSession(); + const resource = await client.get>(spec.itemPath(id)); + + const key = opts.key?.trim() || spec.deriveKey(resource); + if (!key) { + throw new Error("Could not derive a logical key — pass --key explicitly."); + } + const fields = spec.managedFields(resource); + const snippet = configSnippet(type, key, fields); + + if (opts.dryRun) { + info(`Would adopt ${type} #${id} as "${key}". Generated config entry:`); + out({ key, type, id, fields, config: snippet }); + return; + } + + const config = resolveConfig(); + const statePath = resolveStatePath(opts.state); + const state = await loadState(statePath, config.host); + if (state.host !== config.host) { + throw new Error( + `State file host (${state.host}) does not match CT_HOST (${config.host}). ` + + `Refusing to mix instances.`, + ); + } + + const now = new Date().toISOString(); + const action = upsert(state, { type, id, key, fields }, now); + await saveState(statePath, state); + + success(`${action === "created" ? "Adopted" : "Updated"} ${type} #${id} as "${key}" → ${statePath}`); + info(`Config entry: ${snippet}`); + if (action === "updated") { + warn("This resource was already managed — its snapshot was refreshed."); + } + }); +} diff --git a/src/commands/placeholders.ts b/src/commands/placeholders.ts index 1f44726..e076242 100644 --- a/src/commands/placeholders.ts +++ b/src/commands/placeholders.ts @@ -12,7 +12,6 @@ interface Planned { } const PLANNED: Planned[] = [ - { name: "adopt", description: "Put one existing resource under management", issue: "Phase 2 (#4)" }, { name: "plan", description: "Show the diff between desired state and ChurchTools", issue: "Phase 3 (#5)" }, { name: "apply", description: "Apply the plan (idempotent, in dependency order)", issue: "Phase 4 (#6)" }, { name: "destroy", description: "Explicitly remove managed resources (protected)", issue: "Phase 4 (#6)" }, diff --git a/src/commands/state.ts b/src/commands/state.ts new file mode 100644 index 0000000..14cc4c7 --- /dev/null +++ b/src/commands/state.ts @@ -0,0 +1,26 @@ +import { Command } from "commander"; +import { resolveConfig } from "../config.js"; +import { loadState, resolveStatePath } from "../state/state.js"; +import { info, out } from "../ui.js"; + +interface StateOptions { + state?: string; +} + +export function stateCommand(): Command { + const cmd = new Command("state").description("Inspect the managed-resource state file"); + + cmd + .command("list") + .description("List every resource under management (JSON to stdout)") + .option("-s, --state ", "state file path (or set CT_STATE)") + .action(async (opts: StateOptions) => { + const statePath = resolveStatePath(opts.state); + const state = await loadState(statePath, resolveConfig().host); + const resources = Object.values(state.resources); + info(`${resources.length} managed resource(s) in ${statePath} (host ${state.host}).`); + out(resources); + }); + + return cmd; +} diff --git a/src/index.ts b/src/index.ts index d1dc413..5101afd 100644 --- a/src/index.ts +++ b/src/index.ts @@ -2,6 +2,8 @@ import { Command } from "commander"; import { authCommand } from "./commands/auth.js"; import { getCommand } from "./commands/get.js"; +import { adoptCommand } from "./commands/adopt.js"; +import { stateCommand } from "./commands/state.js"; import { plannedCommands } from "./commands/placeholders.js"; import { error } from "./ui.js"; @@ -17,6 +19,8 @@ export function buildProgram(): Command { program.addCommand(authCommand()); program.addCommand(getCommand()); + program.addCommand(adoptCommand()); + program.addCommand(stateCommand()); for (const cmd of plannedCommands()) { program.addCommand(cmd); } diff --git a/src/resources/registry.ts b/src/resources/registry.ts new file mode 100644 index 0000000..ed1a856 --- /dev/null +++ b/src/resources/registry.ts @@ -0,0 +1,78 @@ +/** + * Registry of adoptable ChurchTools resource types. + * + * Each entry knows how to fetch one resource by id, derive a stable logical key, + * snapshot the fields we manage, and render a config snippet (the TS-as-code + * form the Phase 3 engine will consume). Paths come from the Phase 0 coverage + * matrix (docs/api-coverage.md). Adding a type = adding an entry here. + */ + +export interface AdoptableResource { + /** GET path for a single resource by id. */ + itemPath: (id: number) => string; + /** Stable logical key derived from the fetched resource. */ + deriveKey: (resource: Record) => string; + /** The subset of fields we manage — the desired-state baseline. */ + managedFields: (resource: Record) => Record; +} + +/** kebab/underscore slug: "Kids Leitung" → "kids_leitung". */ +export function slug(value: string): string { + return value + .toLowerCase() + .normalize("NFKD") + .replace(/[^a-z0-9]+/g, "_") + .replace(/^_+|_+$/g, ""); +} + +function str(resource: Record, key: string): string { + const value = resource[key]; + return typeof value === "string" ? value : ""; +} + +export const RESOURCES: Record = { + campus: { + itemPath: (id) => `/campuses/${id}`, + deriveKey: (r) => slug(str(r, "shortName") || str(r, "name")), + managedFields: (r) => ({ name: r.name, shortName: r.shortName }), + }, + group: { + itemPath: (id) => `/groups/${id}`, + deriveKey: (r) => slug(str(r, "name")), + managedFields: (r) => { + const information = (r.information as Record | undefined) ?? {}; + return { name: r.name, groupTypeId: information.groupTypeId, groupStatusId: information.groupStatusId }; + }, + }, + "group-type": { + itemPath: (id) => `/group/grouptypes/${id}`, + deriveKey: (r) => slug(str(r, "name")), + managedFields: (r) => ({ name: r.name, nameTranslated: r.nameTranslated }), + }, +}; + +export function resourceType(type: string): AdoptableResource { + const entry = RESOURCES[type]; + if (!entry) { + const known = Object.keys(RESOURCES).join(", "); + throw new Error(`Unknown resource type "${type}". Adoptable types: ${known}.`); + } + return entry; +} + +/** Render a config entry as a TS-as-code call, e.g. `campus({ key: "mainz", name: "Mainz" })`. */ +export function configSnippet(type: string, key: string, fields: Record): string { + const fn = type.replace(/-([a-z])/g, (_, c: string) => c.toUpperCase()); + return `${fn}(${tsObject({ key, ...fields })});`; +} + +function tsObject(obj: Record): string { + const parts = Object.entries(obj) + .filter(([, v]) => v !== undefined) + .map(([k, v]) => `${isIdentifier(k) ? k : JSON.stringify(k)}: ${JSON.stringify(v)}`); + return `{ ${parts.join(", ")} }`; +} + +function isIdentifier(key: string): boolean { + return /^[A-Za-z_$][A-Za-z0-9_$]*$/.test(key); +} diff --git a/src/state/state.ts b/src/state/state.ts new file mode 100644 index 0000000..8ac93f1 --- /dev/null +++ b/src/state/state.ts @@ -0,0 +1,123 @@ +/** + * The state file: the set of **explicitly managed** resources. + * + * Everything not in here is invisible to the tool — never shown, never changed, + * never proposed for deletion. It maps a logical key → CT id + the last-known + * snapshot of the fields we manage (the desired-state baseline for diffing). + * + * The file belongs to the config repo (eqrm/ct-structure) and is meant to be + * committed. Default path is `ct-state.json` in the cwd; override with + * `--state ` or `CT_STATE`. + * + * CT ids can be `0` (the Mainz campus is `id: 0`), so every id comparison here + * uses explicit null/undefined checks — never truthiness. + */ +import { readFile, writeFile } from "node:fs/promises"; + +export interface ManagedResource { + type: string; + id: number; + key: string; + /** Snapshot of the managed fields — the desired-state baseline. */ + fields: Record; + adoptedAt: string; + updatedAt: string; +} + +export interface State { + version: 1; + host: string; + /** Keyed by logical key (e.g. "mainz", "mainz_kids_lead"). */ + resources: Record; +} + +export const DEFAULT_STATE_PATH = "ct-state.json"; + +export function resolveStatePath(explicit?: string, env: NodeJS.ProcessEnv = process.env): string { + return explicit?.trim() || env.CT_STATE?.trim() || DEFAULT_STATE_PATH; +} + +export function emptyState(host: string): State { + return { version: 1, host, resources: {} }; +} + +export async function loadState(path: string, host: string): Promise { + try { + const raw = await readFile(path, "utf8"); + const parsed = JSON.parse(raw) as State; + if (parsed.version !== 1) { + throw new Error(`Unsupported state file version ${parsed.version} in ${path}`); + } + return parsed; + } catch (err) { + if (isNotFound(err)) { + return emptyState(host); + } + throw err; + } +} + +export async function saveState(path: string, state: State): Promise { + await writeFile(path, `${JSON.stringify(state, null, 2)}\n`, "utf8"); +} + +/** Find a managed entry by CT type + id (id may legitimately be 0). */ +export function findByTypeId(state: State, type: string, id: number): ManagedResource | undefined { + return Object.values(state.resources).find((r) => r.type === type && r.id === id); +} + +export function isManaged(state: State, type: string, id: number): boolean { + return findByTypeId(state, type, id) !== undefined; +} + +export interface UpsertInput { + type: string; + id: number; + key: string; + fields: Record; +} + +export type UpsertAction = "created" | "updated"; + +/** + * Idempotently place a resource under management. Re-adopting the same + * (type, id) updates it in place — and re-keys it if the logical key changed. + * A key already taken by a *different* resource is a conflict, not an overwrite. + */ +export function upsert(state: State, input: UpsertInput, now: string): UpsertAction { + const existing = findByTypeId(state, input.type, input.id); + const collision = state.resources[input.key]; + if (collision && !(collision.type === input.type && collision.id === input.id)) { + throw new Error( + `Logical key "${input.key}" is already used by ${collision.type} #${collision.id}. ` + + `Pass a different --key.`, + ); + } + + if (existing) { + if (existing.key !== input.key) { + delete state.resources[existing.key]; + } + state.resources[input.key] = { + ...existing, + key: input.key, + fields: input.fields, + updatedAt: now, + }; + return "updated"; + } + + state.resources[input.key] = { + type: input.type, + id: input.id, + key: input.key, + fields: input.fields, + adoptedAt: now, + updatedAt: now, + }; + return "created"; +} + +function isNotFound(err: unknown): boolean { + return typeof err === "object" && err !== null && (err as { code?: string }).code === "ENOENT"; +} diff --git a/tests/adopt.test.ts b/tests/adopt.test.ts new file mode 100644 index 0000000..01ee4e9 --- /dev/null +++ b/tests/adopt.test.ts @@ -0,0 +1,65 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { readFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +const fakeCampus = { id: 0, name: "Mainz", shortName: "MZ" }; +const getMock = vi.fn(async () => fakeCampus); + +vi.mock("../src/api/session.js", () => ({ + authedSession: vi.fn(async () => ({ client: { get: getMock }, me: { id: 1 } })), +})); + +const { adoptCommand } = await import("../src/commands/adopt.js"); +const { loadState } = await import("../src/state/state.js"); + +const statePath = join(tmpdir(), `ct-cli-adopt-${process.pid}.json`); +const HOST = "https://eqrm.church.tools"; + +async function runAdopt(args: string[]): Promise { + await adoptCommand().parseAsync(args, { from: "user" }); +} + +beforeEach(() => { + getMock.mockClear(); +}); + +afterEach(async () => { + await rm(statePath, { force: true }); +}); + +describe("ct adopt", () => { + it("adopts a campus (id 0) into the state file", async () => { + await runAdopt(["campus", "0", "--state", statePath]); + const state = await loadState(statePath, HOST); + expect(state.resources.mz).toMatchObject({ + type: "campus", + id: 0, + key: "mz", + fields: { name: "Mainz", shortName: "MZ" }, + }); + }); + + it("is idempotent — re-adopting keeps a single entry", async () => { + await runAdopt(["campus", "0", "--state", statePath]); + await runAdopt(["campus", "0", "--state", statePath]); + const state = await loadState(statePath, HOST); + expect(Object.keys(state.resources)).toHaveLength(1); + }); + + it("honours an explicit --key", async () => { + await runAdopt(["campus", "0", "--key", "mainz", "--state", statePath]); + const state = await loadState(statePath, HOST); + expect(state.resources.mainz?.id).toBe(0); + }); + + it("--dry-run does not write the state file", async () => { + await runAdopt(["campus", "0", "--dry-run", "--state", statePath]); + await expect(readFile(statePath, "utf8")).rejects.toMatchObject({ code: "ENOENT" }); + }); + + it("rejects an unknown resource type before any API call", async () => { + await expect(runAdopt(["widget", "1", "--state", statePath])).rejects.toThrow(/Adoptable types/); + expect(getMock).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/registry.test.ts b/tests/registry.test.ts new file mode 100644 index 0000000..e967df3 --- /dev/null +++ b/tests/registry.test.ts @@ -0,0 +1,44 @@ +import { describe, it, expect } from "vitest"; +import { slug, resourceType, configSnippet, RESOURCES } from "../src/resources/registry.js"; + +describe("slug", () => { + it("normalises names to underscore keys", () => { + expect(slug("Kids Leitung")).toBe("kids_leitung"); + expect(slug("Kids 0–3")).toBe("kids_0_3"); + expect(slug(" MZ ")).toBe("mz"); + }); +}); + +describe("resourceType", () => { + it("returns the campus spec with the right item path", () => { + expect(resourceType("campus").itemPath(0)).toBe("/campuses/0"); + }); + + it("throws for an unknown type, listing the known ones", () => { + expect(() => resourceType("nope")).toThrow(/Adoptable types/); + }); + + it("derives a key from campus shortName", () => { + expect(RESOURCES.campus?.deriveKey({ name: "Mainz", shortName: "MZ" })).toBe("mz"); + }); +}); + +describe("configSnippet", () => { + it("renders a TS-as-code call with the logical key first", () => { + expect(configSnippet("campus", "mainz", { name: "Mainz", shortName: "MZ" })).toBe( + 'campus({ key: "mainz", name: "Mainz", shortName: "MZ" });', + ); + }); + + it("camelCases a hyphenated type into the function name", () => { + expect(configSnippet("group-type", "commitment", { name: "Commitment" })).toBe( + 'groupType({ key: "commitment", name: "Commitment" });', + ); + }); + + it("omits undefined fields", () => { + expect(configSnippet("group", "team", { name: "Team", groupTypeId: undefined })).toBe( + 'group({ key: "team", name: "Team" });', + ); + }); +}); diff --git a/tests/state.test.ts b/tests/state.test.ts new file mode 100644 index 0000000..543a0bc --- /dev/null +++ b/tests/state.test.ts @@ -0,0 +1,53 @@ +import { describe, it, expect } from "vitest"; +import { emptyState, upsert, findByTypeId, isManaged } from "../src/state/state.js"; + +const HOST = "https://eqrm.church.tools"; +const NOW = "2026-07-07T00:00:00.000Z"; +const LATER = "2026-07-08T00:00:00.000Z"; + +describe("state.upsert", () => { + it("creates a new managed resource", () => { + const state = emptyState(HOST); + const action = upsert(state, { type: "campus", id: 0, key: "mainz", fields: { name: "Mainz" } }, NOW); + expect(action).toBe("created"); + expect(state.resources.mainz).toMatchObject({ type: "campus", id: 0, key: "mainz", adoptedAt: NOW }); + }); + + it("is idempotent for the same (type, id) — updates in place, no duplicate", () => { + const state = emptyState(HOST); + upsert(state, { type: "campus", id: 0, key: "mainz", fields: { name: "Mainz" } }, NOW); + const action = upsert( + state, + { type: "campus", id: 0, key: "mainz", fields: { name: "Mainz HQ" } }, + LATER, + ); + expect(action).toBe("updated"); + expect(Object.keys(state.resources)).toHaveLength(1); + expect(state.resources.mainz?.fields).toEqual({ name: "Mainz HQ" }); + expect(state.resources.mainz?.adoptedAt).toBe(NOW); + expect(state.resources.mainz?.updatedAt).toBe(LATER); + }); + + it("handles id 0 without treating it as missing", () => { + const state = emptyState(HOST); + upsert(state, { type: "campus", id: 0, key: "mainz", fields: {} }, NOW); + expect(isManaged(state, "campus", 0)).toBe(true); + expect(findByTypeId(state, "campus", 0)?.id).toBe(0); + }); + + it("re-keys when the same resource is adopted under a new key", () => { + const state = emptyState(HOST); + upsert(state, { type: "campus", id: 0, key: "mainz", fields: {} }, NOW); + upsert(state, { type: "campus", id: 0, key: "mz", fields: {} }, LATER); + expect(Object.keys(state.resources)).toEqual(["mz"]); + expect(findByTypeId(state, "campus", 0)?.key).toBe("mz"); + }); + + it("rejects a key already taken by a different resource", () => { + const state = emptyState(HOST); + upsert(state, { type: "campus", id: 0, key: "shared", fields: {} }, NOW); + expect(() => upsert(state, { type: "group", id: 5, key: "shared", fields: {} }, NOW)).toThrow( + /already used/, + ); + }); +}); From cd2a909df087fccdc325d331e151186672a6e842 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Tue, 7 Jul 2026 13:31:12 +0200 Subject: [PATCH 2/3] fix: run main() when invoked via a symlink (global/brew ct) The entrypoint guard compared import.meta.url (the module realpath) against file://+argv[1] (the symlink path). Invoked through the npm-link / Homebrew `ct` symlink these never matched, so `ct --help` exited 0 printing nothing. Resolve both sides to their realpath via a tested isMainModule() helper. Refs #4 --- src/index.ts | 3 ++- src/isMain.ts | 28 ++++++++++++++++++++++++++++ tests/isMain.test.ts | 30 ++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 src/isMain.ts create mode 100644 tests/isMain.test.ts diff --git a/src/index.ts b/src/index.ts index 5101afd..754e6e8 100644 --- a/src/index.ts +++ b/src/index.ts @@ -5,6 +5,7 @@ import { getCommand } from "./commands/get.js"; import { adoptCommand } from "./commands/adopt.js"; import { stateCommand } from "./commands/state.js"; import { plannedCommands } from "./commands/placeholders.js"; +import { isMainModule } from "./isMain.js"; import { error } from "./ui.js"; export function buildProgram(): Command { @@ -39,6 +40,6 @@ async function main(): Promise { } /** Only run when invoked as the binary — importing this module (tests) must not parse argv. */ -if (process.argv[1] && import.meta.url === `file://${process.argv[1]}`) { +if (isMainModule(process.argv[1], import.meta.url)) { void main(); } diff --git a/src/isMain.ts b/src/isMain.ts new file mode 100644 index 0000000..88df0f4 --- /dev/null +++ b/src/isMain.ts @@ -0,0 +1,28 @@ +/** + * Whether this module is the process entrypoint (i.e. run as the `ct` binary), + * as opposed to being imported (e.g. by tests). + * + * The naive `import.meta.url === "file://" + process.argv[1]` check breaks when + * the binary is invoked through a symlink — npm link, a global install, or a + * Homebrew shim all leave `argv[1]` as the symlink path while `import.meta.url` + * is the module's realpath. Resolving both to their realpath makes the + * comparison robust; a resolution failure falls back to a plain comparison. + */ +import { realpathSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +export function isMainModule( + argv1: string | undefined, + metaUrl: string, + realpath: (p: string) => string = realpathSync, +): boolean { + if (!argv1) { + return false; + } + const modulePath = fileURLToPath(metaUrl); + try { + return realpath(argv1) === realpath(modulePath); + } catch { + return argv1 === modulePath; + } +} diff --git a/tests/isMain.test.ts b/tests/isMain.test.ts new file mode 100644 index 0000000..82343e9 --- /dev/null +++ b/tests/isMain.test.ts @@ -0,0 +1,30 @@ +import { describe, it, expect } from "vitest"; +import { isMainModule } from "../src/isMain.js"; + +const moduleUrl = "file:///repo/dist/index.js"; // → /repo/dist/index.js + +describe("isMainModule", () => { + it("is true when invoked directly (paths already equal)", () => { + expect(isMainModule("/repo/dist/index.js", moduleUrl, (p) => p)).toBe(true); + }); + + it("is true when invoked via a symlink that resolves to the module (the ct/brew case)", () => { + const realpath = (p: string) => (p === "/opt/homebrew/bin/ct" ? "/repo/dist/index.js" : p); + expect(isMainModule("/opt/homebrew/bin/ct", moduleUrl, realpath)).toBe(true); + }); + + it("is false when imported by another entrypoint (e.g. the test runner)", () => { + expect(isMainModule("/repo/node_modules/.bin/vitest", moduleUrl, (p) => p)).toBe(false); + }); + + it("is false when argv1 is undefined", () => { + expect(isMainModule(undefined, moduleUrl)).toBe(false); + }); + + it("falls back to a plain comparison when realpath throws", () => { + const realpath = () => { + throw new Error("ENOENT"); + }; + expect(isMainModule("/repo/dist/index.js", moduleUrl, realpath)).toBe(true); + }); +}); From ad1d11cfff246fddff529c497c7dfaff57743e87 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Tue, 7 Jul 2026 14:13:11 +0200 Subject: [PATCH 3/3] fix: address Phase 2 code review (#10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correctness: - adopt: reject non-integer ids strictly (/^\d+$/) — Number.parseInt accepted "0x10"→0, "3abc"→3, "2.9"→2, silently adopting the wrong id. - state: validate the state file's shape (top-level object, version, host, resources) and surface friendly errors instead of cryptic TypeErrors on a hand-edited/committed file (e.g. missing "resources", top-level null, invalid JSON). - state: move the host-mismatch guard into loadState so every command inherits it — `ct state list` previously listed another instance's resources with no warning. - adopt: load + validate the state file (host guard included) BEFORE authedSession()/GET, so a mismatched instance never triggers a live authenticated request against the wrong host. - registry: strip NFKD combining marks in slug() so German names slug to their base letters ("Zürich"→"zurich") instead of gaining underscores. - registry: snapshot group ids from a nested `information` object OR the top level, so the baseline isn't silently empty if the API shape differs. - isMain: resolve each path independently in the realpath fallback so an unresolvable argv1 still compares against the module's realpath. Cleanup: - adopt: drop the redundant second resolveConfig() (single source now). - registry: derive itemPath from a single collection-path literal per entry. Refs #4 --- src/commands/adopt.ts | 23 ++++++++--------- src/isMain.ts | 16 +++++++----- src/resources/registry.ts | 31 +++++++++++++++++------ src/state/state.ts | 48 +++++++++++++++++++++++++++++++----- tests/adopt.test.ts | 10 ++++++++ tests/registry.test.ts | 21 ++++++++++++++++ tests/state.test.ts | 52 +++++++++++++++++++++++++++++++++++++-- 7 files changed, 166 insertions(+), 35 deletions(-) diff --git a/src/commands/adopt.ts b/src/commands/adopt.ts index 9511d0b..1034e22 100644 --- a/src/commands/adopt.ts +++ b/src/commands/adopt.ts @@ -21,10 +21,17 @@ export function adoptCommand(): Command { .option("--dry-run", "preview the config entry and state change without writing") .action(async (type: string, rawId: string, opts: AdoptOptions) => { const spec = resourceType(type); - const id = Number.parseInt(rawId, 10); - if (!Number.isInteger(id)) { - throw new Error(`Invalid id "${rawId}" — expected an integer.`); + if (!/^\d+$/.test(rawId.trim())) { + throw new Error(`Invalid id "${rawId}" — expected a non-negative integer.`); } + const id = Number.parseInt(rawId, 10); + + // Load + validate the state file (host guard included) BEFORE any network + // call, so a state file recorded against another instance never triggers a + // live authenticated request against the wrong ChurchTools host. + const config = resolveConfig(); + const statePath = resolveStatePath(opts.state); + const state = await loadState(statePath, config.host); const { client } = await authedSession(); const resource = await client.get>(spec.itemPath(id)); @@ -42,16 +49,6 @@ export function adoptCommand(): Command { return; } - const config = resolveConfig(); - const statePath = resolveStatePath(opts.state); - const state = await loadState(statePath, config.host); - if (state.host !== config.host) { - throw new Error( - `State file host (${state.host}) does not match CT_HOST (${config.host}). ` + - `Refusing to mix instances.`, - ); - } - const now = new Date().toISOString(); const action = upsert(state, { type, id, key, fields }, now); await saveState(statePath, state); diff --git a/src/isMain.ts b/src/isMain.ts index 88df0f4..d9c325b 100644 --- a/src/isMain.ts +++ b/src/isMain.ts @@ -6,7 +6,8 @@ * the binary is invoked through a symlink — npm link, a global install, or a * Homebrew shim all leave `argv[1]` as the symlink path while `import.meta.url` * is the module's realpath. Resolving both to their realpath makes the - * comparison robust; a resolution failure falls back to a plain comparison. + * comparison robust; each side falls back to its raw path if resolution fails, + * so an unresolvable argv1 still gets compared against the module's realpath. */ import { realpathSync } from "node:fs"; import { fileURLToPath } from "node:url"; @@ -20,9 +21,12 @@ export function isMainModule( return false; } const modulePath = fileURLToPath(metaUrl); - try { - return realpath(argv1) === realpath(modulePath); - } catch { - return argv1 === modulePath; - } + const resolve = (p: string): string => { + try { + return realpath(p); + } catch { + return p; + } + }; + return resolve(argv1) === resolve(modulePath); } diff --git a/src/resources/registry.ts b/src/resources/registry.ts index ed1a856..d7289e2 100644 --- a/src/resources/registry.ts +++ b/src/resources/registry.ts @@ -16,11 +16,19 @@ export interface AdoptableResource { managedFields: (resource: Record) => Record; } -/** kebab/underscore slug: "Kids Leitung" → "kids_leitung". */ +/** Build an `itemPath` from a collection path, so each entry names its path once. */ +const item = (collectionPath: string) => (id: number) => `${collectionPath}/${id}`; + +/** + * kebab/underscore slug: "Kids Leitung" → "kids_leitung", "Zürich" → "zurich". + * NFKD splits accented letters into base + combining mark; we drop the marks so + * German names (ü/ö/ä/…) slug to their base letters rather than gaining a `_`. + */ export function slug(value: string): string { return value .toLowerCase() .normalize("NFKD") + .replace(/[\u0300-\u036f]/g, "") .replace(/[^a-z0-9]+/g, "_") .replace(/^_+|_+$/g, ""); } @@ -30,22 +38,29 @@ function str(resource: Record, key: string): string { return typeof value === "string" ? value : ""; } +/** Read a field, preferring a nested `information` object but falling back to the top level. */ +function fromInformation(resource: Record, key: string): unknown { + const information = (resource.information as Record | undefined) ?? {}; + return information[key] ?? resource[key]; +} + export const RESOURCES: Record = { campus: { - itemPath: (id) => `/campuses/${id}`, + itemPath: item("/campuses"), deriveKey: (r) => slug(str(r, "shortName") || str(r, "name")), managedFields: (r) => ({ name: r.name, shortName: r.shortName }), }, group: { - itemPath: (id) => `/groups/${id}`, + itemPath: item("/groups"), deriveKey: (r) => slug(str(r, "name")), - managedFields: (r) => { - const information = (r.information as Record | undefined) ?? {}; - return { name: r.name, groupTypeId: information.groupTypeId, groupStatusId: information.groupStatusId }; - }, + managedFields: (r) => ({ + name: r.name, + groupTypeId: fromInformation(r, "groupTypeId"), + groupStatusId: fromInformation(r, "groupStatusId"), + }), }, "group-type": { - itemPath: (id) => `/group/grouptypes/${id}`, + itemPath: item("/group/grouptypes"), deriveKey: (r) => slug(str(r, "name")), managedFields: (r) => ({ name: r.name, nameTranslated: r.nameTranslated }), }, diff --git a/src/state/state.ts b/src/state/state.ts index 8ac93f1..cfad233 100644 --- a/src/state/state.ts +++ b/src/state/state.ts @@ -41,20 +41,56 @@ export function emptyState(host: string): State { return { version: 1, host, resources: {} }; } +/** + * Load the state file at `path`, validating its shape and asserting it belongs + * to `host`. A missing file yields an empty state for `host`. `host` is the + * instance the caller intends to operate on: a file recorded against a + * different host is rejected here so no command (adopt, state list, …) can + * silently mix instances. + */ export async function loadState(path: string, host: string): Promise { + let raw: string; try { - const raw = await readFile(path, "utf8"); - const parsed = JSON.parse(raw) as State; - if (parsed.version !== 1) { - throw new Error(`Unsupported state file version ${parsed.version} in ${path}`); - } - return parsed; + raw = await readFile(path, "utf8"); } catch (err) { if (isNotFound(err)) { return emptyState(host); } throw err; } + + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch (err) { + throw new Error(`Malformed state file ${path}: not valid JSON (${(err as Error).message}).`); + } + + const state = validateState(parsed, path); + if (state.host !== host) { + throw new Error( + `State file host (${state.host}) does not match CT_HOST (${host}). Refusing to mix instances.`, + ); + } + return state; +} + +/** Assert the parsed JSON has the shape of a State; throw a friendly error otherwise. */ +function validateState(parsed: unknown, path: string): State { + if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { + throw new Error(`Malformed state file ${path}: expected a JSON object at the top level.`); + } + const obj = parsed as Record; + if (obj.version !== 1) { + throw new Error(`Unsupported state file version ${String(obj.version)} in ${path}`); + } + if (typeof obj.host !== "string" || obj.host === "") { + throw new Error(`Malformed state file ${path}: missing or empty "host".`); + } + if (typeof obj.resources !== "object" || obj.resources === null || Array.isArray(obj.resources)) { + throw new Error(`Malformed state file ${path}: "resources" must be an object.`); + } + return obj as unknown as State; } export async function saveState(path: string, state: State): Promise { diff --git a/tests/adopt.test.ts b/tests/adopt.test.ts index 01ee4e9..92a548f 100644 --- a/tests/adopt.test.ts +++ b/tests/adopt.test.ts @@ -62,4 +62,14 @@ describe("ct adopt", () => { await expect(runAdopt(["widget", "1", "--state", statePath])).rejects.toThrow(/Adoptable types/); expect(getMock).not.toHaveBeenCalled(); }); + + it("rejects a non-integer id (trailing garbage) before any API call", async () => { + await expect(runAdopt(["campus", "3abc", "--state", statePath])).rejects.toThrow( + /expected a non-negative integer/, + ); + await expect(runAdopt(["campus", "0x10", "--state", statePath])).rejects.toThrow( + /expected a non-negative integer/, + ); + expect(getMock).not.toHaveBeenCalled(); + }); }); diff --git a/tests/registry.test.ts b/tests/registry.test.ts index e967df3..6371e81 100644 --- a/tests/registry.test.ts +++ b/tests/registry.test.ts @@ -7,6 +7,12 @@ describe("slug", () => { expect(slug("Kids 0–3")).toBe("kids_0_3"); expect(slug(" MZ ")).toBe("mz"); }); + + it("strips German diacritics to their base letters instead of adding underscores", () => { + expect(slug("Zürich")).toBe("zurich"); + expect(slug("Jugendküche")).toBe("jugendkuche"); + expect(slug("Gebärdensprache")).toBe("gebardensprache"); + }); }); describe("resourceType", () => { @@ -14,6 +20,10 @@ describe("resourceType", () => { expect(resourceType("campus").itemPath(0)).toBe("/campuses/0"); }); + it("builds the group-type item path from its collection path", () => { + expect(resourceType("group-type").itemPath(7)).toBe("/group/grouptypes/7"); + }); + it("throws for an unknown type, listing the known ones", () => { expect(() => resourceType("nope")).toThrow(/Adoptable types/); }); @@ -21,6 +31,17 @@ describe("resourceType", () => { it("derives a key from campus shortName", () => { expect(RESOURCES.campus?.deriveKey({ name: "Mainz", shortName: "MZ" })).toBe("mz"); }); + + it("snapshots group ids whether they are nested under information or top-level", () => { + expect( + RESOURCES.group?.managedFields({ name: "Team", information: { groupTypeId: 2, groupStatusId: 1 } }), + ).toEqual({ name: "Team", groupTypeId: 2, groupStatusId: 1 }); + expect(RESOURCES.group?.managedFields({ name: "Team", groupTypeId: 2, groupStatusId: 1 })).toEqual({ + name: "Team", + groupTypeId: 2, + groupStatusId: 1, + }); + }); }); describe("configSnippet", () => { diff --git a/tests/state.test.ts b/tests/state.test.ts index 543a0bc..6578bca 100644 --- a/tests/state.test.ts +++ b/tests/state.test.ts @@ -1,5 +1,8 @@ -import { describe, it, expect } from "vitest"; -import { emptyState, upsert, findByTypeId, isManaged } from "../src/state/state.js"; +import { describe, it, expect, afterEach } from "vitest"; +import { writeFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { emptyState, loadState, saveState, upsert, findByTypeId, isManaged } from "../src/state/state.js"; const HOST = "https://eqrm.church.tools"; const NOW = "2026-07-07T00:00:00.000Z"; @@ -51,3 +54,48 @@ describe("state.upsert", () => { ); }); }); + +describe("state.loadState", () => { + const statePath = join(tmpdir(), `ct-cli-loadstate-${process.pid}.json`); + + afterEach(async () => { + await rm(statePath, { force: true }); + }); + + it("returns an empty state for the given host when the file is missing", async () => { + const state = await loadState(statePath, HOST); + expect(state).toEqual(emptyState(HOST)); + }); + + it("round-trips a saved state", async () => { + const original = emptyState(HOST); + upsert(original, { type: "campus", id: 0, key: "mainz", fields: { name: "Mainz" } }, NOW); + await saveState(statePath, original); + expect(await loadState(statePath, HOST)).toEqual(original); + }); + + it("refuses to load a state file recorded against a different host", async () => { + await saveState(statePath, emptyState("https://other.church.tools")); + await expect(loadState(statePath, HOST)).rejects.toThrow(/Refusing to mix instances/); + }); + + it("rejects a structurally invalid state file (missing resources) with a friendly error", async () => { + await writeFile(statePath, JSON.stringify({ version: 1, host: HOST }), "utf8"); + await expect(loadState(statePath, HOST)).rejects.toThrow(/"resources" must be an object/); + }); + + it("rejects a top-level non-object state file", async () => { + await writeFile(statePath, "null", "utf8"); + await expect(loadState(statePath, HOST)).rejects.toThrow(/expected a JSON object/); + }); + + it("rejects invalid JSON with a friendly error", async () => { + await writeFile(statePath, "{ not json", "utf8"); + await expect(loadState(statePath, HOST)).rejects.toThrow(/not valid JSON/); + }); + + it("rejects an unsupported version", async () => { + await writeFile(statePath, JSON.stringify({ version: 2, host: HOST, resources: {} }), "utf8"); + await expect(loadState(statePath, HOST)).rejects.toThrow(/Unsupported state file version/); + }); +});