From ebd37602b538c37cb15c973de62e8d113fae1672 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Thu, 9 Jul 2026 14:28:14 +0200 Subject: [PATCH 1/5] fix(state): bump updatedAt only when managed fields change (#52) --- src/state/state.ts | 30 +++++++++++++++++++++++++++-- tests/state.test.ts | 47 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/src/state/state.ts b/src/state/state.ts index 8c70961..d91d028 100644 --- a/src/state/state.ts +++ b/src/state/state.ts @@ -182,11 +182,17 @@ export function upsert(state: State, input: UpsertInput, now: string): UpsertAct if (existing.key !== input.key) { delete state.resources[existing.key]; } + // State is machine-only (#52): `updatedAt` bumps ONLY when the managed fields actually change, so + // an apply that writes an identical snapshot leaves the committed state file byte-for-byte + // unchanged (no churn). When unchanged, keep the EXISTING `fields` object (not the freshly-built + // input one) so serialization order is preserved too — a mere key-order difference must not churn + // the diff. `adoptedAt` is set once at first adoption and never touched again. + const unchanged = fieldsEqual(existing.fields, input.fields); state.resources[input.key] = { ...existing, key: input.key, - fields: input.fields, - updatedAt: now, + fields: unchanged ? existing.fields : input.fields, + updatedAt: unchanged ? existing.updatedAt : now, }; return "updated"; } @@ -205,3 +211,23 @@ export function upsert(state: State, input: UpsertInput, now: string): UpsertAct function isNotFound(err: unknown): boolean { return typeof err === "object" && err !== null && (err as { code?: string }).code === "ENOENT"; } + +/** + * Structural, order-independent equality for two managed-field bags (#52). Kept local so state — a + * foundational module that imports no engine code — stays self-contained (mirrors the engine's + * `deepEqual`, but a key-order difference between two structurally-identical snapshots must not be + * seen as a change here either). + */ +function fieldsEqual(a: unknown, b: unknown): boolean { + if (a === b) return true; + if (a === null || b === null || typeof a !== "object" || typeof b !== "object") return false; + if (Array.isArray(a) || Array.isArray(b)) { + if (!Array.isArray(a) || !Array.isArray(b) || a.length !== b.length) return false; + return a.every((v, i) => fieldsEqual(v, b[i])); + } + const ao = a as Record; + const bo = b as Record; + const aKeys = Object.keys(ao); + if (aKeys.length !== Object.keys(bo).length) return false; + return aKeys.every((k) => Object.prototype.hasOwnProperty.call(bo, k) && fieldsEqual(ao[k], bo[k])); +} diff --git a/tests/state.test.ts b/tests/state.test.ts index 1be8010..6f7ee1c 100644 --- a/tests/state.test.ts +++ b/tests/state.test.ts @@ -31,6 +31,53 @@ describe("state.upsert", () => { expect(state.resources.mainz?.updatedAt).toBe(LATER); }); + it("does not bump updatedAt when re-adopting identical fields (#52: quiet state)", () => { + const state = emptyState(HOST); + upsert(state, { type: "campus", id: 0, key: "mainz", fields: { name: "Mainz", shorty: "MZ" } }, NOW); + // Re-upsert with the SAME fields (a fresh object, but structurally equal) at a LATER time. + const action = upsert( + state, + { type: "campus", id: 0, key: "mainz", fields: { name: "Mainz", shorty: "MZ" } }, + LATER, + ); + expect(action).toBe("updated"); + // updatedAt must NOT churn: nothing about the resource actually changed. + expect(state.resources.mainz?.updatedAt).toBe(NOW); + expect(state.resources.mainz?.adoptedAt).toBe(NOW); + }); + + it("bumps updatedAt only when the fields actually change", () => { + const state = emptyState(HOST); + upsert(state, { type: "campus", id: 0, key: "mainz", fields: { name: "Mainz" } }, NOW); + upsert(state, { type: "campus", id: 0, key: "mainz", fields: { name: "Mainz HQ" } }, LATER); + expect(state.resources.mainz?.updatedAt).toBe(LATER); + expect(state.resources.mainz?.adoptedAt).toBe(NOW); + }); + + it("ignores field key ORDER when deciding whether fields changed (order-independent)", () => { + const state = emptyState(HOST); + upsert(state, { type: "group", id: 5, key: "g", fields: { name: "G", groupTypeId: 2 } }, NOW); + upsert(state, { type: "group", id: 5, key: "g", fields: { groupTypeId: 2, name: "G" } }, LATER); + expect(state.resources.g?.updatedAt).toBe(NOW); // reordered, but structurally identical + }); + + it("re-adopting identical fields leaves the saved state file byte-identical", async () => { + const state = emptyState(HOST); + upsert(state, { type: "campus", id: 0, key: "mainz", fields: { name: "Mainz", shorty: "MZ" } }, NOW); + const path = join(tmpdir(), `ct-cli-quiet-${process.pid}.json`); + await saveState(path, state); + const before = await import("node:fs/promises").then((fs) => fs.readFile(path, "utf8")); + upsert( + state, + { type: "campus", id: 0, key: "mainz", fields: { name: "Mainz", shorty: "MZ" } }, + LATER, + ); + await saveState(path, state); + const after = await import("node:fs/promises").then((fs) => fs.readFile(path, "utf8")); + await rm(path, { force: true }); + expect(after).toBe(before); + }); + it("handles id 0 without treating it as missing", () => { const state = emptyState(HOST); upsert(state, { type: "campus", id: 0, key: "mainz", fields: {} }, NOW); From 4566e7e90c84908dd86d678f6bfa5d806cff5ef2 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Thu, 9 Jul 2026 14:31:21 +0200 Subject: [PATCH 2/5] feat(config): located validation errors and unknown-field warnings (#52) --- src/config/context.ts | 159 ++++++++++++++++++++++++----------- tests/located-errors.test.ts | 121 ++++++++++++++++++++++++++ tests/state.test.ts | 6 +- 3 files changed, 234 insertions(+), 52 deletions(-) create mode 100644 tests/located-errors.test.ts diff --git a/src/config/context.ts b/src/config/context.ts index 34fb31d..e2b3bfd 100644 --- a/src/config/context.ts +++ b/src/config/context.ts @@ -11,6 +11,8 @@ * The context is injected (no global state), so blueprints are just functions * and loops, and the whole thing is trivially testable without file I/O. */ +import { basename } from "node:path"; +import { fileURLToPath } from "node:url"; import type { DesiredResource, DynamicSpec, DynamicStatus } from "../engine/types.js"; import type { DomainType } from "../permissions/grants.js"; import type { DesiredPermission, Grant } from "../permissions/types.js"; @@ -27,6 +29,54 @@ export { ref } from "../resolve/refs.js"; const DYNAMIC_STATUSES = ["active", "inactive", "manual", "none"] as const; +/** This module's own filesystem path — used to skip our own frames when locating a user call site. */ +const SELF_FILE = fileURLToPath(import.meta.url); + +/** + * Best-effort source location of the user's `ct.*(...)` call, for located config errors/warnings (#52). + * Walks `new Error().stack` for the first frame outside this module, node internals, and node_modules. + * jiti transpiles the user's `.ts` config but maps stack frames back to the ORIGINAL file + line + * (verified against the real loader), so the frame yields the author's actual location. Returns + * `basename:line` (e.g. `ct.config.ts:42`), or `undefined` when no user frame is identifiable — callers + * then omit the location rather than crash (V8-only `.stack`; a runtime without it degrades gracefully). + */ +function captureCallSite(): string | undefined { + const stack = new Error().stack; + if (typeof stack !== "string") return undefined; + for (const raw of stack.split("\n").slice(1)) { + const line = raw.trim(); + if (!line.startsWith("at ")) continue; + // `at fn (PATH:LINE:COL)` (function named) or `at PATH:LINE:COL` (top-level/anonymous). + const m = /\((.+):(\d+):(\d+)\)$/.exec(line) ?? /^at\s+(.+):(\d+):(\d+)$/.exec(line); + if (!m) continue; + let file = m[1]!; + if (file.startsWith("file://")) file = fileURLToPath(file); + if (file === SELF_FILE) continue; // our own wrapper / helper frames + if (file.startsWith("node:")) continue; // node internals + if (file.includes("/node_modules/")) continue; // jiti & other deps + return `${basename(file)}:${m[2]}`; + } + return undefined; +} + +/** Prefix a config error/warning message with its source location when known (#52). */ +function located(location: string | undefined, message: string): string { + return location ? `${location} — ${message}` : message; +} + +/** + * Prefix a thrown eval-time config error with its user call site (#52), once. Mutating `.message` + * (rather than wrapping) keeps the original stack; the `__ctLocated` marker guards against a second + * prefix if the same error somehow passes through another wrapper. + */ +function relocate(err: unknown, location: string | undefined): unknown { + if (location && err instanceof Error && !(err as { __ctLocated?: boolean }).__ctLocated) { + err.message = located(location, err.message); + (err as { __ctLocated?: boolean }).__ctLocated = true; + } + return err; +} + export interface ResourceInput { key: string; /** Ordering hint: apply this resource after `parent`. A dependency edge only — NOT managed hierarchy. */ @@ -129,7 +179,7 @@ export interface ConfigContext { export type ConfigModule = (ct: ConfigContext) => void | Promise; -function toDesired(type: string, input: ResourceInput): DesiredResource { +function toDesired(type: string, input: ResourceInput, location?: string): DesiredResource { const { key, parent, parents, dependsOn = [], preventDestroy, dynamic, ...fields } = input; if (!key || typeof key !== "string") { throw new Error(`${type} declaration is missing a string "key".`); @@ -177,12 +227,12 @@ function toDesired(type: string, input: ResourceInput): DesiredResource { // field still passes through into `fields` unchanged (unrecognised fields have always been sent // as-is); this only surfaces the mistake instead of leaving it silently un-diffed forever. The // allowlist comes from `knownFields` (the registry's own `managedFields`), never hand-copied, so - // it can't drift from what `adopt`/`plan`/`apply` actually read and write. Issue #52 will add - // file:line locations to this warning — not built here. + // it can't drift from what `adopt`/`plan`/`apply` actually read and write. The `location` prefix + // (#52) points the author at the exact config file + line of the offending declaration. const allowed = knownFields(type); for (const fieldKey of Object.keys(fields)) { if (!allowed.has(fieldKey)) { - warn(`${type} "${key}": unknown field "${fieldKey}" (ignored)`); + warn(located(location, `${type} "${key}": unknown field "${fieldKey}" (ignored)`)); } } // `dynamic` is a synthetic field for auto-groups, handled separately from the plain diffed @@ -261,55 +311,70 @@ export function createContext(): { const define = (type: string) => (input: ResourceInput): void => { - const resource = toDesired(type, input); - if (seen.has(resource.key)) { - throw new Error(`Duplicate logical key "${resource.key}" in config.`); + // Capture the user's call site FIRST (top of the wrapper = the frame is the author's + // `ct.({...})` call), then locate any eval-time error or unknown-field warning it raises (#52). + const location = captureCallSite(); + try { + const resource = toDesired(type, input, location); + if (seen.has(resource.key)) { + throw new Error(`Duplicate logical key "${resource.key}" in config.`); + } + seen.add(resource.key); + resources.push(resource); + } catch (err) { + throw relocate(err, location); } - seen.add(resource.key); - resources.push(resource); }; + const definePermissionInner = (domainType: DomainType, input: PermissionInput): void => { + if (typeof input.key !== "string" || !input.key) + throw new Error(`${domainType} declaration missing a string "key".`); + const domainId = resolveDomainInput(domainType, input); + if (!Array.isArray(input.grants)) + throw new Error(`${domainType} "${input.key}": "grants" must be an array.`); + for (const g of input.grants) { + const right = typeof g === "string" ? g : g?.right; + if (typeof right !== "string" || !right.includes(":")) + throw new Error( + `${domainType} "${input.key}": each grant must be a "module:right" string or { right, scope }.`, + ); + if (typeof g === "object") { + if (!Array.isArray(g.scope)) + throw new Error(`${domainType} "${input.key}": scoped grant needs "scope": (string | number)[].`); + // Each entry is a logical group key, or a raw numeric dataId (#49 escape hatch — for scope + // dimensions that aren't groups, e.g. security levels, which have no logical/managed form). + for (const s of g.scope) { + if (typeof s === "string" ? s.length === 0 : typeof s !== "number") + throw new Error( + `${domainType} "${input.key}": scope entries must be a non-empty string (logical group key) or a number (raw dataId), got ${JSON.stringify(s)}.`, + ); + } + } + } + if (seen.has(input.key)) throw new Error(`Duplicate logical key "${input.key}" in config.`); + seen.add(input.key); + // Duplicate-target guard, keyed by the canonical domain string (numeric id or Ref key). This + // catches obvious eval-time collisions early; the authoritative check runs post-resolution in + // buildPermissionPlan (two different refs, or a ref and a number, can resolve to the same id). + const domainKey = `${domainType}:${domainKeyPart(domainId)}`; + const existingKey = seenDomains.get(domainKey); + if (existingKey) { + const label = typeof domainId === "number" ? `#${domainId}` : refKey(domainId); + throw new Error( + `Duplicate permission target: ${domainType} ${label} is declared by both "${existingKey}" and "${input.key}". Merge their grants into one declaration.`, + ); + } + seenDomains.set(domainKey, input.key); + permissions.push({ key: input.key, domainType, domainId, grants: input.grants }); + }; const definePermission = (domainType: DomainType) => (input: PermissionInput): void => { - if (typeof input.key !== "string" || !input.key) - throw new Error(`${domainType} declaration missing a string "key".`); - const domainId = resolveDomainInput(domainType, input); - if (!Array.isArray(input.grants)) - throw new Error(`${domainType} "${input.key}": "grants" must be an array.`); - for (const g of input.grants) { - const right = typeof g === "string" ? g : g?.right; - if (typeof right !== "string" || !right.includes(":")) - throw new Error( - `${domainType} "${input.key}": each grant must be a "module:right" string or { right, scope }.`, - ); - if (typeof g === "object") { - if (!Array.isArray(g.scope)) - throw new Error(`${domainType} "${input.key}": scoped grant needs "scope": (string | number)[].`); - // Each entry is a logical group key, or a raw numeric dataId (#49 escape hatch — for scope - // dimensions that aren't groups, e.g. security levels, which have no logical/managed form). - for (const s of g.scope) { - if (typeof s === "string" ? s.length === 0 : typeof s !== "number") - throw new Error( - `${domainType} "${input.key}": scope entries must be a non-empty string (logical group key) or a number (raw dataId), got ${JSON.stringify(s)}.`, - ); - } - } - } - if (seen.has(input.key)) throw new Error(`Duplicate logical key "${input.key}" in config.`); - seen.add(input.key); - // Duplicate-target guard, keyed by the canonical domain string (numeric id or Ref key). This - // catches obvious eval-time collisions early; the authoritative check runs post-resolution in - // buildPermissionPlan (two different refs, or a ref and a number, can resolve to the same id). - const domainKey = `${domainType}:${domainKeyPart(domainId)}`; - const existingKey = seenDomains.get(domainKey); - if (existingKey) { - const label = typeof domainId === "number" ? `#${domainId}` : refKey(domainId); - throw new Error( - `Duplicate permission target: ${domainType} ${label} is declared by both "${existingKey}" and "${input.key}". Merge their grants into one declaration.`, - ); + const location = captureCallSite(); + try { + definePermissionInner(domainType, input); + } catch (err) { + throw relocate(err, location); } - seenDomains.set(domainKey, input.key); - permissions.push({ key: input.key, domainType, domainId, grants: input.grants }); }; // Every type emitted here MUST have an apply tier in engine/graph.ts TYPE_TIER // (locked by tests/context.test.ts), else computePlan rejects it at plan time. diff --git a/tests/located-errors.test.ts b/tests/located-errors.test.ts new file mode 100644 index 0000000..3660500 --- /dev/null +++ b/tests/located-errors.test.ts @@ -0,0 +1,121 @@ +/** + * Located config errors & warnings (#52 item C). Every fixture here is written to disk and evaluated + * through the REAL loader (jiti transpiling TS on the fly) so the `new Error().stack` frame mapping — + * the load-bearing part — is actually exercised end-to-end, not stubbed. + */ +import { describe, it, expect, vi, afterEach } from "vitest"; +import { mkdtempSync, writeFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { basename, join } from "node:path"; +import { loadConfig } from "../src/config/load.js"; +import { createContext } from "../src/config/context.js"; + +const dirs: string[] = []; +function writeConfig(body: string): string { + const dir = mkdtempSync(join(tmpdir(), "ct-located-")); + dirs.push(dir); + const path = join(dir, "ct.config.ts"); + writeFileSync(path, body); + return path; +} + +afterEach(() => { + while (dirs.length) rmSync(dirs.pop()!, { recursive: true, force: true }); +}); + +describe("located unknown-field warning (through the real jiti loader)", () => { + it("prefixes the warning with the config file basename + line of the declaration", async () => { + const writes: string[] = []; + const spy = vi.spyOn(process.stderr, "write").mockImplementation((s) => { + writes.push(String(s)); + return true; + }); + // Line 1 = comment, line 2 = export, line 3 = the campus call → location must be ct.config.ts:3. + const path = writeConfig( + [ + "// leading comment", + "export default (ct) => {", + ' ct.campus({ key: "mainz", name: "Mainz", shortName: "MZ" });', + "};", + "", + ].join("\n"), + ); + try { + await loadConfig(path); + } finally { + spy.mockRestore(); + } + const msg = writes.join(""); + expect(msg).toContain(`${basename(path)}:3 — campus "mainz": unknown field "shortName" (ignored)`); + }); +}); + +describe("located validation error (through the real jiti loader)", () => { + it("prefixes an eval-time error with the config file basename + line", async () => { + // The invalid declaration (both `campus` sugar and numeric `campusId`) sits on line 3. + const path = writeConfig( + [ + "// header", + "export default (ct) => {", + ' ct.group({ key: "g", name: "G", campus: "mainz", campusId: 4 });', + "};", + "", + ].join("\n"), + ); + await expect(loadConfig(path)).rejects.toThrow( + new RegExp(`${basename(path).replace(/\./g, "\\.")}:3 — group "g": declare either "campus"`), + ); + }); + + it("locates an error raised from a helper the config calls (first user frame wins)", async () => { + // ct.group is called from a helper on line 3; the throwing call site is line 3, not the loop. + const path = writeConfig( + [ + "export default (ct) => {", + " const mk = (k) => ct.group({ key: k, name: k, parents: 123 });", + ' mk("team");', + "};", + "", + ].join("\n"), + ); + await expect(loadConfig(path)).rejects.toThrow( + new RegExp(`${basename(path).replace(/\./g, "\\.")}:2 — group "team": "parents" must be`), + ); + }); +}); + +describe("graceful fallback when no user frame is identifiable", () => { + it("omits the location (never crashes) when the stack carries no frames", () => { + const writes: string[] = []; + const spy = vi.spyOn(process.stderr, "write").mockImplementation((s) => { + writes.push(String(s)); + return true; + }); + const originalLimit = Error.stackTraceLimit; + Error.stackTraceLimit = 0; // `new Error().stack` now has no frames → captureCallSite returns undefined + try { + const { ct } = createContext(); + ct.campus({ key: "mainz", name: "Mainz", shortName: "MZ" }); + } finally { + Error.stackTraceLimit = originalLimit; + spy.mockRestore(); + } + const msg = writes.join(""); + // Bare message, no `file:line — ` prefix, and no thrown error. + expect(msg).toContain('campus "mainz": unknown field "shortName" (ignored)'); + expect(msg).not.toMatch(/\.ts:\d+ — campus/); + }); + + it("throws a located-free (but intact) error when no frame is identifiable", () => { + const originalLimit = Error.stackTraceLimit; + Error.stackTraceLimit = 0; + try { + const { ct } = createContext(); + expect(() => ct.group({ key: "g", name: "G", campus: "mainz", campusId: 4 })).toThrow( + /^group "g": declare either "campus"/, + ); + } finally { + Error.stackTraceLimit = originalLimit; + } + }); +}); diff --git a/tests/state.test.ts b/tests/state.test.ts index 6f7ee1c..386a831 100644 --- a/tests/state.test.ts +++ b/tests/state.test.ts @@ -67,11 +67,7 @@ describe("state.upsert", () => { const path = join(tmpdir(), `ct-cli-quiet-${process.pid}.json`); await saveState(path, state); const before = await import("node:fs/promises").then((fs) => fs.readFile(path, "utf8")); - upsert( - state, - { type: "campus", id: 0, key: "mainz", fields: { name: "Mainz", shorty: "MZ" } }, - LATER, - ); + upsert(state, { type: "campus", id: 0, key: "mainz", fields: { name: "Mainz", shorty: "MZ" } }, LATER); await saveState(path, state); const after = await import("node:fs/promises").then((fs) => fs.readFile(path, "utf8")); await rm(path, { force: true }); From 9649b317c6457f95cc85fb8446a01efb2244400a Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Thu, 9 Jul 2026 14:32:47 +0200 Subject: [PATCH 3/5] =?UTF-8?q?feat(config):=20dynamic=20sugar=20=E2=80=94?= =?UTF-8?q?=20dynamic:=20true=20and=20dynamic:=20".json"=20(#52)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/config/context.ts | 68 ++++++++++++++++++++++++++++++++++--------- tests/context.test.ts | 54 ++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 13 deletions(-) diff --git a/src/config/context.ts b/src/config/context.ts index e2b3bfd..1a30e5c 100644 --- a/src/config/context.ts +++ b/src/config/context.ts @@ -77,10 +77,20 @@ function relocate(err: unknown, location: string | undefined): unknown { return err; } +/** + * A group's `dynamic` (auto-group) declaration (#52 item B). Three interchangeable forms: + * - `true` — dynamic with `status: "active"` and the conventional `./rulesets/.json` ruleset ref. + * - `".json"` — dynamic with `status: "active"` and an explicit ruleset-file ref. + * - `{ status, ruleset }` — the explicit form (a RuleSet object, a `{ ref }`, or a typed-query build). + */ +export type DynamicInput = boolean | string | { status: DynamicStatus; ruleset: unknown }; + export interface ResourceInput { key: string; /** Ordering hint: apply this resource after `parent`. A dependency edge only — NOT managed hierarchy. */ parent?: string; + /** Auto-group config (opt-in; omit for a plain group). Only valid on `ct.group(...)`. See {@link DynamicInput}. */ + dynamic?: DynamicInput; /** * Managed parent groups (group→group hierarchy). Opt-in: omit to leave a group's hierarchy * unmanaged; `[]` means "managed with no parents". Each key must reference a group declared @@ -179,6 +189,50 @@ export interface ConfigContext { export type ConfigModule = (ct: ConfigContext) => void | Promise; +/** + * Convention for the sugared ruleset path when `dynamic: true` — the same `rulesets/.json` + * layout `ct adopt group --with-dynamic` writes to, so an adopted dynamic group round-trips to the + * shortest form. Kept in sync with the emitter in registry.ts. + */ +export function conventionalRulesetRef(key: string): string { + return `./rulesets/${key}.json`; +} + +/** + * Eval-time desugaring of a group's `dynamic` field (#52 item B) — the engine is untouched; every + * form collapses to the same {@link DynamicSpec}. Three authoring forms: + * - `dynamic: true` → `{ status: "active", ruleset: { ref: "./rulesets/.json" } }` + * - `dynamic: ".json"` → `{ status: "active", ruleset: { ref: "" } }` + * - `dynamic: { status, ruleset }` (explicit) — validated as before. + * Returns `undefined` for `undefined` (opt-in: not a dynamic group). Anything else throws. + */ +function desugarDynamic(type: string, key: string, dynamic: unknown): DynamicSpec | undefined { + if (dynamic === undefined) return undefined; + if (type !== "group") throw new Error(`${type} "${key}": "dynamic" is only valid on a group.`); + if (dynamic === true) { + return { status: "active", ruleset: { ref: conventionalRulesetRef(key) } }; + } + if (typeof dynamic === "string") { + if (!dynamic.endsWith(".json")) + throw new Error( + `group "${key}": "dynamic" as a string must be a path to a .json ruleset file ` + + `(e.g. "./rulesets/${key}.json"), got ${JSON.stringify(dynamic)}.`, + ); + return { status: "active", ruleset: { ref: dynamic } }; + } + if (dynamic === null || typeof dynamic !== "object") { + throw new Error( + `group "${key}": "dynamic" must be true, a ".json" string, or an object with { status, ruleset }.`, + ); + } + const d = dynamic as Record; + if (!DYNAMIC_STATUSES.includes(d.status as DynamicStatus)) + throw new Error(`group "${key}": "dynamic.status" must be one of ${DYNAMIC_STATUSES.join(", ")}.`); + if (d.ruleset == null || typeof d.ruleset !== "object") + throw new Error(`group "${key}": "dynamic.ruleset" must be a RuleSet object or a { ref } reference.`); + return { status: d.status as DynamicStatus, ruleset: d.ruleset }; +} + function toDesired(type: string, input: ResourceInput, location?: string): DesiredResource { const { key, parent, parents, dependsOn = [], preventDestroy, dynamic, ...fields } = input; if (!key || typeof key !== "string") { @@ -237,19 +291,7 @@ function toDesired(type: string, input: ResourceInput, location?: string): Desir } // `dynamic` is a synthetic field for auto-groups, handled separately from the plain diffed // field bag. Opt-in: `undefined` means "not a dynamic group" (mirrors `parents`). - let dynamicSpec: DynamicSpec | undefined; - if (dynamic !== undefined) { - if (type !== "group") throw new Error(`${type} "${key}": "dynamic" is only valid on a group.`); - if (dynamic == null || typeof dynamic !== "object") { - throw new Error(`group "${key}": "dynamic" must be an object with { status, ruleset }.`); - } - const d = dynamic as Record; - if (!DYNAMIC_STATUSES.includes(d.status as DynamicStatus)) - throw new Error(`group "${key}": "dynamic.status" must be one of ${DYNAMIC_STATUSES.join(", ")}.`); - if (d.ruleset == null || typeof d.ruleset !== "object") - throw new Error(`group "${key}": "dynamic.ruleset" must be a RuleSet object or a { ref } reference.`); - dynamicSpec = { status: d.status as DynamicStatus, ruleset: d.ruleset }; - } + const dynamicSpec = desugarDynamic(type, key, dynamic); // `parent` is an ordering hint only — a dependency edge, never a diffed/managed field // (its pre-hierarchy meaning; a `parent` may point at a campus). Group hierarchy is // managed opt-in via `parents`: `undefined` → unmanaged, `[]` → managed with no parents. diff --git a/tests/context.test.ts b/tests/context.test.ts index db2c501..eae57df 100644 --- a/tests/context.test.ts +++ b/tests/context.test.ts @@ -225,6 +225,60 @@ describe("dynamic block", () => { }); }); +describe("dynamic sugar (#52 item B)", () => { + it("desugars `dynamic: true` to active + the conventional ./rulesets/.json ref", () => { + const { ct, resources } = createContext(); + ct.group({ key: "all_mainz", name: "Alle", groupTypeId: 1, dynamic: true }); + expect(resources[0]?.dynamic).toEqual({ + status: "active", + ruleset: { ref: "./rulesets/all_mainz.json" }, + }); + expect(resources[0]?.fields).not.toHaveProperty("dynamic"); + }); + + it('desugars a `dynamic: ".json"` string to active + that explicit ref', () => { + const { ct, resources } = createContext(); + ct.group({ key: "g", name: "G", dynamic: "./custom/rules.json" }); + expect(resources[0]?.dynamic).toEqual({ + status: "active", + ruleset: { ref: "./custom/rules.json" }, + }); + }); + + it("keeps the explicit object form working unchanged", () => { + const { ct, resources } = createContext(); + ct.group({ + key: "g", + name: "G", + dynamic: { status: "manual", ruleset: { description: "x", method: "ChurchQuery", params: {} } }, + }); + expect(resources[0]?.dynamic).toEqual({ + status: "manual", + ruleset: { description: "x", method: "ChurchQuery", params: {} }, + }); + }); + + it("rejects a string that is not a .json path", () => { + const { ct } = createContext(); + expect(() => ct.group({ key: "g", name: "G", dynamic: "rules.yaml" })).toThrow(/\.json ruleset file/); + }); + + it("rejects `dynamic: false` and other non-true / non-string / non-object values", () => { + const { ct } = createContext(); + expect(() => ct.group({ key: "a", name: "A", dynamic: false })).toThrow( + /must be true, a "\.json" string, or an object/, + ); + expect(() => ct.group({ key: "b", name: "B", dynamic: 42 as never })).toThrow( + /must be true, a "\.json" string, or an object/, + ); + }); + + it("rejects the sugar forms on a non-group type", () => { + const { ct } = createContext(); + expect(() => ct.campus({ key: "c", name: "C", dynamic: true } as never)).toThrow(/dynamic.*only.*group/i); + }); +}); + describe("unknown-field warning (#51)", () => { it("warns naming the resource key and the unknown field, but still keeps it in fields", () => { const spy = vi.spyOn(process.stderr, "write").mockImplementation(() => true); From dc392dce7d14189080e62f121be6d15a5c7d9b49 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Thu, 9 Jul 2026 14:48:20 +0200 Subject: [PATCH 4/5] feat(adopt): idiomatic multi-line config output with reverse-resolved logical sugar (#52) --- src/commands/adopt-group.ts | 22 ++++--- src/commands/adopt.ts | 6 +- src/config/context.ts | 11 +--- src/resolve/reverse.ts | 98 ++++++++++++++++++++++++++++ src/resources/registry.ts | 103 ++++++++++++++++++++++++++---- tests/adopt-group-command.test.ts | 64 +++++++++++++++++-- tests/registry.test.ts | 55 ++++++++++++++-- 7 files changed, 312 insertions(+), 47 deletions(-) create mode 100644 src/resolve/reverse.ts diff --git a/src/commands/adopt-group.ts b/src/commands/adopt-group.ts index 59622e8..1a010c7 100644 --- a/src/commands/adopt-group.ts +++ b/src/commands/adopt-group.ts @@ -18,6 +18,7 @@ import { prepareEnv } from "../env/context.js"; import { normalizeRuleset } from "../engine/dynamic.js"; import type { DynamicStatus } from "../engine/types.js"; import { RESOURCES, configSnippet, fromInformation, slug } from "../resources/registry.js"; +import { ReverseResolver } from "../resolve/reverse.js"; import { loadState, saveState, upsert, type State } from "../state/state.js"; import { success, info, warn, out } from "../ui.js"; @@ -234,6 +235,9 @@ export function adoptGroupCommand(): Command { } const now = new Date().toISOString(); + // One reverse resolver across the whole (possibly bulk) run — each master-data catalog is + // fetched at most once and reused for every group's numeric-id → logical-sugar rewrite (#52). + const reverse = new ReverseResolver(client); const results: ResolvedAdoption[] = []; const reports: Array<{ action: "created" | "updated"; id: number; key: string }> = []; @@ -246,7 +250,10 @@ export function adoptGroupCommand(): Command { } const fields = GROUP_SPEC.managedFields(resource); - let snippetFields: Record = fields; + // Reverse-resolve the group's numeric ids to logical sugar for the emitted snippet; the + // captured `dynamic` block (if any) is appended AFTER, so it is not treated as an id field. + const { fields: sugared, todos } = await reverse.sugarFields(fields); + const snippetFields: Record = sugared; if (opts.withDynamic) { const captured = await captureDynamic(id, client); if (captured) { @@ -259,13 +266,10 @@ export function adoptGroupCommand(): Command { "utf8", ); } - snippetFields = { - ...fields, - dynamic: { status: captured.status, ruleset: { ref: `./${relPath}` } }, - }; + snippetFields.dynamic = { status: captured.status, ruleset: { ref: `./${relPath}` } }; } } - const snippet = configSnippet("group", key, snippetFields); + const snippet = configSnippet("group", key, snippetFields, { todos }); if (opts.dryRun) { results.push({ id, key, fields, snippet }); @@ -304,9 +308,9 @@ export function adoptGroupCommand(): Command { } } - // Grouped, paste-ready config block. configSnippet's per-line FORMAT is unchanged (#52 reworks - // that later) — this only wraps the group of lines under a type comment header, ordered - // parents-before-children where hierarchy is known (--children-of's subtree walk). + // Grouped, paste-ready config block: each snippet is now idiomatic multi-line TS (#52 item A), + // wrapped under a type comment header and ordered parents-before-children where hierarchy is + // known (--children-of's subtree walk). info(results.length === 1 ? "Config entry:" : "Config entries (paste into your config):"); const block = [`// group`, ...results.map((r) => r.snippet)].join("\n"); process.stdout.write(`${block}\n`); diff --git a/src/commands/adopt.ts b/src/commands/adopt.ts index be38d23..9cceaf5 100644 --- a/src/commands/adopt.ts +++ b/src/commands/adopt.ts @@ -3,6 +3,7 @@ import { authedSession } from "../api/session.js"; import { resolveConfig } from "../config.js"; import { prepareEnv } from "../env/context.js"; import { resourceType, configSnippet } from "../resources/registry.js"; +import { ReverseResolver } from "../resolve/reverse.js"; import { loadState, saveState, upsert } from "../state/state.js"; import { success, info, warn, out } from "../ui.js"; import { adoptGrantsCommand } from "./adopt-grants.js"; @@ -47,7 +48,10 @@ export function adoptCommand(): Command { throw new Error("Could not derive a logical key — pass --key explicitly."); } const fields = spec.managedFields(resource); - const snippet = configSnippet(type, key, fields); + // Reverse-resolve numeric ids (campusId/groupTypeId/groupStatusId) to logical sugar so the + // emitted snippet is portable and human-readable; unresolved ids stay numeric + a TODO (#52). + const { fields: sugared, todos } = await new ReverseResolver(client).sugarFields(fields); + const snippet = configSnippet(type, key, sugared, { todos }); if (opts.dryRun) { info(`Would adopt ${type} #${id} as "${key}". Generated config entry:`); diff --git a/src/config/context.ts b/src/config/context.ts index 1a30e5c..89713dc 100644 --- a/src/config/context.ts +++ b/src/config/context.ts @@ -17,7 +17,7 @@ import type { DesiredResource, DynamicSpec, DynamicStatus } from "../engine/type import type { DomainType } from "../permissions/grants.js"; import type { DesiredPermission, Grant } from "../permissions/types.js"; import { isRef, ref, refKey, type Ref } from "../resolve/refs.js"; -import { knownFields } from "../resources/registry.js"; +import { conventionalRulesetRef, knownFields } from "../resources/registry.js"; import { warn } from "../ui.js"; // Re-exported so a config file can pull the query DSL from the same module as // `ConfigContext`: `import { q, churchQuery } from "../../src/config/context.js"`. @@ -189,15 +189,6 @@ export interface ConfigContext { export type ConfigModule = (ct: ConfigContext) => void | Promise; -/** - * Convention for the sugared ruleset path when `dynamic: true` — the same `rulesets/.json` - * layout `ct adopt group --with-dynamic` writes to, so an adopted dynamic group round-trips to the - * shortest form. Kept in sync with the emitter in registry.ts. - */ -export function conventionalRulesetRef(key: string): string { - return `./rulesets/${key}.json`; -} - /** * Eval-time desugaring of a group's `dynamic` field (#52 item B) — the engine is untouched; every * form collapses to the same {@link DynamicSpec}. Three authoring forms: diff --git a/src/resolve/reverse.ts b/src/resolve/reverse.ts new file mode 100644 index 0000000..1ea8aad --- /dev/null +++ b/src/resolve/reverse.ts @@ -0,0 +1,98 @@ +/** + * Reverse reference resolution for `ct adopt` (#52 item A): turn the numeric ChurchTools ids a + * fetched resource carries (`campusId`, `groupTypeId`, `groupStatusId`) into the logical sugar the + * DSL already accepts (`campus`/`groupType`/`status`), so an adopted snippet is portable and reads + * like something a human would author — not a wall of instance-specific integers. + * + * This is the mirror image of the forward {@link Resolver} (src/resolve/resolver.ts): it reads the + * SAME master-data catalogs, matched here BY ID instead of by name, and emits `slug(name)` — exactly + * the key the forward resolver's slug-primary match will map back to the same id on any host. Each + * catalog is fetched at most once and cached; a fetch that fails (endpoint unreachable, non-array + * body) degrades to "no catalog", so adopt never fails just because a lookup could not be resolved — + * the caller keeps the numeric id and flags it with a `// TODO: no logical match` comment. + */ +import type { CtClient } from "../api/ctClient.js"; +import { slug } from "../resources/registry.js"; + +/** + * The numeric id fields adopt can reverse-sugar, each mapped to its catalog path and the logical DSL + * field it sugars into. Mirrors context.ts `ID_SUGAR` (sugar → idField) and resolver.ts `CATALOG_PATH` + * (kind → path); kept explicit (three entries) rather than derived, with this comment as the sync note. + */ +const REVERSE_ID_FIELDS: Record = { + campusId: { catalog: "/campuses", sugar: "campus" }, + groupTypeId: { catalog: "/group/grouptypes", sugar: "groupType" }, + groupStatusId: { catalog: "/group/memberstatus", sugar: "status" }, +}; + +interface CatalogRecord { + id: number; + name?: string; + [k: string]: unknown; +} + +export class ReverseResolver { + private readonly client: Pick; + /** id → logical key, per catalog path, fetched at most once. A failed fetch caches an empty map. */ + private readonly catalogs = new Map>>(); + + constructor(client: Pick) { + this.client = client; + } + + private index(path: string): Promise> { + let p = this.catalogs.get(path); + if (!p) { + p = this.client + .get(path) + .then((rows) => { + const map = new Map(); + if (Array.isArray(rows)) { + for (const row of rows) { + if (typeof row?.id === "number" && typeof row.name === "string" && row.name.length > 0) { + map.set(row.id, slug(row.name)); + } + } + } + return map; + }) + // Adopt must not fail because a catalog is unreachable — degrade to "no matches" (→ TODO). + .catch(() => new Map()); + this.catalogs.set(path, p); + } + return p; + } + + /** The logical key for a numeric id in the given catalog, or `undefined` when unmatched. */ + private async keyForId(catalog: string, id: number): Promise { + return (await this.index(catalog)).get(id); + } + + /** + * Reverse-sugar a managed-field bag for emission (#52 item A): each numeric id field with a catalog + * match becomes its logical `campus`/`groupType`/`status` key (dropping the numeric field); an id + * with NO match stays numeric and is named in `todos` so the emitter can flag it. Every other field + * (and a `null` id — "no campus", omitted by the emitter) passes through in its original position. + */ + async sugarFields( + fields: Record, + ): Promise<{ fields: Record; todos: Set }> { + const out: Record = {}; + const todos = new Set(); + for (const [field, value] of Object.entries(fields)) { + const rule = REVERSE_ID_FIELDS[field]; + if (rule && typeof value === "number") { + const key = await this.keyForId(rule.catalog, value); + if (key !== undefined) { + out[rule.sugar] = key; + } else { + out[field] = value; + todos.add(field); + } + continue; + } + out[field] = value; + } + return { fields: out, todos }; + } +} diff --git a/src/resources/registry.ts b/src/resources/registry.ts index f29664b..11de1cb 100644 --- a/src/resources/registry.ts +++ b/src/resources/registry.ts @@ -163,23 +163,100 @@ function camelCase(type: string): string { } /** - * Render a config entry as a TS-as-code call, e.g. `campus({ key: "mainz", name: "Mainz" })`. - * The function name comes from the registry entry's `dslName` (default: camelCase of the type), - * so the emitted snippet always names an actual `ConfigContext` function — never a colliding one. + * The conventional ruleset-file path for a dynamic group's `dynamic: true` sugar (#52): the same + * `rulesets/.json` layout `ct adopt group --with-dynamic` writes to. Owned here (a low-level + * module) so both the config-DSL desugarer (context.ts) and the adopt emitter can share it without + * a registry↔context import cycle. */ -export function configSnippet(type: string, key: string, fields: Record): string { +export function conventionalRulesetRef(key: string): string { + return `./rulesets/${key}.json`; +} + +/** Prettier's `printWidth` (see .prettierrc.json) — the emitter mirrors it for array wrapping. */ +const PRINT_WIDTH = 110; + +/** Options for {@link configSnippet}. `todos` names fields to flag with a trailing `// TODO` comment. */ +export interface SnippetOptions { + /** Field keys that could not be reverse-resolved to logical sugar — annotated inline (#52 item A). */ + todos?: Set; +} + +/** + * Render a config entry as an idiomatic, prettier-compatible TS-as-code call (#52 item A): + * multi-line, 2-space indent, trailing commas, one field per line. The function name comes from the + * registry entry's `dslName` (default: camelCase of the type), so the emitted snippet always names an + * actual `ConfigContext` function — never a colliding one. `fields` should already be reverse-sugared + * (numeric ids → logical `campus`/`groupType`/`status` keys); anything left numeric that the caller + * couldn't resolve is passed in `opts.todos` to earn a `// TODO: no logical match` marker. A `dynamic` + * field is collapsed to its shortest sugar form (`true` / `""`) when it matches the convention. + */ +export function configSnippet( + type: string, + key: string, + fields: Record, + opts: SnippetOptions = {}, +): string { const fn = RESOURCES[type]?.dslName ?? camelCase(type); - return `${fn}(${tsObject({ key, ...fields })});`; + const prepared: Record = {}; + for (const [k, v] of Object.entries(fields)) { + prepared[k] = k === "dynamic" ? sugarDynamicValue(key, v) : v; + } + return `${fn}(${renderObject({ key, ...prepared }, "", opts.todos)});`; } -function tsObject(obj: Record): string { - // null-valued fields are omitted, not emitted: pasting `campusId: null` would actively - // MANAGE "no campus" (planning a later UI-assigned campus back to null), whereas omission - // leaves the field unmanaged — the safer default for a freshly adopted resource. - const parts = Object.entries(obj) - .filter(([, v]) => v !== undefined && v !== null) - .map(([k, v]) => `${isIdentifier(k) ? k : JSON.stringify(k)}: ${JSON.stringify(v)}`); - return `{ ${parts.join(", ")} }`; +/** + * Collapse an emitted `dynamic` value to its shortest DSL sugar (#52 item B round-trip): an + * `active` status whose ruleset is exactly `{ ref }` becomes `true` (when the ref matches the + * `./rulesets/.json` convention) or the bare `""` string. Any other shape (non-active + * status, an inline ruleset object) is emitted verbatim as the explicit object. + */ +function sugarDynamicValue(key: string, dynamic: unknown): unknown { + if (dynamic === null || typeof dynamic !== "object") return dynamic; + const d = dynamic as Record; + const ruleset = d.ruleset; + if (d.status !== "active" || ruleset === null || typeof ruleset !== "object") return dynamic; + const rs = ruleset as Record; + if (typeof rs.ref !== "string" || Object.keys(rs).length !== 1) return dynamic; + return rs.ref === conventionalRulesetRef(key) ? true : rs.ref; +} + +/** + * Render a plain object as multi-line TS. null/undefined-valued fields are OMITTED, not emitted: + * pasting `campusId: null` would actively MANAGE "no campus" (planning a later UI-assigned campus + * back to null), whereas omission leaves the field unmanaged — the safer default for a freshly + * adopted resource. `todos` (top-level only) appends a `// TODO` marker after the trailing comma. + */ +function renderObject(obj: Record, indent: string, todos?: Set): string { + const inner = `${indent} `; + const entries = Object.entries(obj).filter(([, v]) => v !== undefined && v !== null); + if (entries.length === 0) return "{}"; + const lines = entries.map(([k, v]) => { + const keyStr = isIdentifier(k) ? k : JSON.stringify(k); + const todo = todos?.has(k) ? " // TODO: no logical match" : ""; + return `${inner}${keyStr}: ${renderValue(v, inner)},${todo}`; + }); + return `{\n${lines.join("\n")}\n${indent}}`; +} + +/** Render any JSON value as prettier-style TS, indenting nested objects/arrays under `indent`. */ +function renderValue(value: unknown, indent: string): string { + if (value === null) return "null"; + if (typeof value === "string") return JSON.stringify(value); + if (typeof value === "number" || typeof value === "boolean") return String(value); + if (Array.isArray(value)) { + if (value.length === 0) return "[]"; + // Match prettier: a short all-primitive array stays on one line; anything longer or with a + // nested object/array breaks one element per line. (Adopt output never actually nests arrays — + // rulesets are emitted as a `{ ref }` — so this only keeps the emitter faithful in general.) + const allPrimitive = value.every((v) => v === null || typeof v !== "object"); + const inline = `[${value.map((v) => renderValue(v, indent)).join(", ")}]`; + if (allPrimitive && indent.length + inline.length <= PRINT_WIDTH) return inline; + const inner = `${indent} `; + const items = value.map((v) => `${inner}${renderValue(v, inner)},`).join("\n"); + return `[\n${items}\n${indent}]`; + } + if (typeof value === "object") return renderObject(value as Record, indent); + return JSON.stringify(value); } function isIdentifier(key: string): boolean { diff --git a/tests/adopt-group-command.test.ts b/tests/adopt-group-command.test.ts index 59265f2..be596f1 100644 --- a/tests/adopt-group-command.test.ts +++ b/tests/adopt-group-command.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { mkdtempSync, rmSync } from "node:fs"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { readFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -27,7 +27,8 @@ function makeClient() { 50: { id: 50, name: "Cycle A", information: { groupTypeId: 5, groupStatusId: 1 } }, 51: { id: 51, name: "Cycle B", information: { groupTypeId: 5, groupStatusId: 1 } }, 30: { id: 30, name: "All Mainz", information: { groupTypeId: 5, groupStatusId: 1 } }, - 31: { id: 31, name: "Static Group", information: { groupTypeId: 5, groupStatusId: 1 } }, + // campusId 0 (Mainz) exercises campus reverse-resolution — including the id-0 edge — end to end. + 31: { id: 31, name: "Static Group", information: { groupTypeId: 5, groupStatusId: 1, campusId: 0 } }, }; const children: Record = { 40: [41, 42], @@ -39,6 +40,9 @@ function makeClient() { { id: 5, name: "Team" }, { id: 9, name: "Other Type" }, ]; + // Master-data catalogs the reverse resolver reads to sugar numeric ids back to logical keys (#52). + const campuses = [{ id: 0, name: "Mainz" }]; + const memberStatuses = [{ id: 1, name: "Aktiv" }]; const rulesets: Record> = { 30: { description: "x", query: { "==": [{ var: "a" }, "1"] }, process: {} }, }; @@ -54,6 +58,8 @@ function makeClient() { m = /^\/groups\/(\d+)\/children$/.exec(path); if (m) return (children[Number(m[1])] ?? []).map((id) => ({ id })); if (path === "/group/grouptypes") return groupTypes; + if (path === "/campuses") return campuses; + if (path === "/group/memberstatus") return memberStatuses; m = /^\/dynamicgroups\/(\d+)\/ruleset$/.exec(path); if (m) { const rs = rulesets[Number(m[1])]; @@ -81,6 +87,7 @@ vi.mock("../src/api/session.js", () => ({ const { adoptCommand } = await import("../src/commands/adopt.js"); const { loadState } = await import("../src/state/state.js"); +const { loadConfig } = await import("../src/config/load.js"); const HOST = "https://eqrm.church.tools"; const originalHost = process.env.CT_HOST; @@ -130,8 +137,10 @@ describe("ct adopt group — multi-id list form", () => { } const block = writes.join(""); expect(block).toContain("// group"); - expect(block).toContain('group({ key: "area_a"'); - expect(block).toContain('group({ key: "area_b"'); + // Idiomatic multi-line snippets (#52 item A): the call opens on its own line, key first. + expect(block).toContain("group({"); + expect(block).toContain('key: "area_a"'); + expect(block).toContain('key: "area_b"'); // parents-before-children / declared order preserved: area_a's line precedes area_b's. expect(block.indexOf('key: "area_a"')).toBeLessThan(block.indexOf('key: "area_b"')); }); @@ -254,9 +263,12 @@ describe("ct adopt group --with-dynamic", () => { expect(written).toEqual({ description: "x", query: { "==": [{ var: "a" }, 1] }, process: {} }); // coerced "1" -> 1 const block = writes.join(""); - // configSnippet renders a nested-object field value via JSON.stringify (compact, quoted keys) — - // matches the existing tsObject behavior (see src/resources/registry.ts), unchanged by #51. - expect(block).toContain('dynamic: {"status":"active","ruleset":{"ref":"./rulesets/all_mainz.json"}}'); + // #52 item A+B: an active dynamic group whose ruleset matches the ./rulesets/.json convention + // is emitted with the shortest `dynamic: true` sugar (round-trips to the same spec on load). + expect(block).toContain("dynamic: true,"); + // And the numeric ids are reverse-sugared to their logical keys against the mocked catalogs. + expect(block).toContain('groupType: "team",'); + expect(block).toContain('status: "aktiv",'); // The plain group fields (state snapshot) never carry "dynamic" — it's synthetic, not a managed field. const state = await loadState(statePath, HOST); @@ -310,3 +322,41 @@ describe("ct adopt group --with-dynamic", () => { expect(plan.items.every((i) => i.action === "no-op")).toBe(true); }); }); + +describe("ct adopt group — idiomatic snippet round-trips to a no-op (#52 item A acceptance)", () => { + it("pasting the VERBATIM emitted snippet into a config plans as a no-op, zero hand edits", async () => { + // Adopt a real group, capturing exactly what the command prints to stdout. + const writes: string[] = []; + const spy = vi.spyOn(process.stdout, "write").mockImplementation((s) => { + writes.push(String(s)); + return true; + }); + try { + await run(["group", "31", "--state", statePath]); + } finally { + spy.mockRestore(); + } + + // The printed block is a `// group` header + one idiomatic multi-line `group({ ... });` snippet + // with campusId/groupTypeId/groupStatusId reverse-sugared to campus/groupType/status keys. + const block = writes.join(""); + const snippet = block.replace(/^\/\/ group\n/, "").trim(); + expect(snippet.startsWith("group({")).toBe(true); + expect(snippet).toContain('campus: "mainz"'); // id 0 reverse-resolved + expect(snippet).toContain('groupType: "team"'); + expect(snippet).toContain('status: "aktiv"'); + expect(snippet).not.toContain("TODO"); // everything resolved — a clean, hand-edit-free paste + + // Paste it VERBATIM into a config (only wrapping boilerplate + the `ct.` receiver added). + const configPath = join(workDir, "ct.config.ts"); + writeFileSync(configPath, `export default (ct) => {\n ct.${snippet}\n};\n`); + + // Load it through the real loader and plan against the state the adopt just wrote. + const { resources } = await loadConfig(configPath); + const state = await loadState(statePath, HOST); + const { plan } = await buildPlan(client as unknown as Pick, state, resources, { + configDir: workDir, + }); + expect(plan.items.every((i) => i.action === "no-op")).toBe(true); + }); +}); diff --git a/tests/registry.test.ts b/tests/registry.test.ts index d82c6b5..aedc390 100644 --- a/tests/registry.test.ts +++ b/tests/registry.test.ts @@ -69,30 +69,71 @@ describe("resourceType", () => { }); }); -describe("configSnippet", () => { - it("renders a TS-as-code call with the logical key first", () => { +describe("configSnippet — idiomatic multi-line output (#52 item A)", () => { + it("renders a prettier-compatible multi-line call with the logical key first", () => { expect(configSnippet("campus", "mainz", { name: "Mainz", shortName: "MZ" })).toBe( - 'campus({ key: "mainz", name: "Mainz", shortName: "MZ" });', + ["campus({", ' key: "mainz",', ' name: "Mainz",', ' shortName: "MZ",', "});"].join("\n"), ); }); it("camelCases a hyphenated type into the function name", () => { expect(configSnippet("group-type", "commitment", { name: "Commitment" })).toBe( - 'groupType({ key: "commitment", name: "Commitment" });', + ["groupType({", ' key: "commitment",', ' name: "Commitment",', "});"].join("\n"), ); }); it("omits undefined fields", () => { expect(configSnippet("group", "team", { name: "Team", groupTypeId: undefined })).toBe( - 'group({ key: "team", name: "Team" });', + ["group({", ' key: "team",', ' name: "Team",', "});"].join("\n"), ); }); it("emits the master-data role under roleDefinition, not the colliding permission name groupRole", () => { expect(configSnippet("group-role", "leiter", { name: "Leiter", groupTypeId: 2 })).toBe( - 'roleDefinition({ key: "leiter", name: "Leiter", groupTypeId: 2 });', + ["roleDefinition({", ' key: "leiter",', ' name: "Leiter",', " groupTypeId: 2,", "});"].join("\n"), ); }); + + it("flags a field named in `todos` with a trailing `// TODO: no logical match` comment", () => { + expect( + configSnippet("group", "team", { name: "Team", groupTypeId: 5 }, { todos: new Set(["groupTypeId"]) }), + ).toBe( + [ + "group({", + ' key: "team",', + ' name: "Team",', + " groupTypeId: 5, // TODO: no logical match", + "});", + ].join("\n"), + ); + }); + + it("collapses a `dynamic` block to `true` when it matches the ./rulesets/.json convention", () => { + expect( + configSnippet("group", "all_mainz", { + name: "Alle", + dynamic: { status: "active", ruleset: { ref: "./rulesets/all_mainz.json" } }, + }), + ).toContain(" dynamic: true,"); + }); + + it("collapses a `dynamic` block to the bare path string when active but off-convention", () => { + expect( + configSnippet("group", "g", { + name: "G", + dynamic: { status: "active", ruleset: { ref: "./custom/rules.json" } }, + }), + ).toContain(' dynamic: "./custom/rules.json",'); + }); + + it("keeps a non-active `dynamic` block as an explicit object", () => { + const snip = configSnippet("group", "g", { + name: "G", + dynamic: { status: "manual", ruleset: { ref: "./rulesets/g.json" } }, + }); + expect(snip).toContain(" dynamic: {"); + expect(snip).toContain(' status: "manual",'); + }); }); // Round-trip guarantee: whatever `adopt` prints for any adoptable type must be declarable. @@ -186,7 +227,7 @@ describe("knownFields (#51)", () => { describe("configSnippet null omission", () => { it("omits null-valued fields — a campus-less group adopts without managing 'no campus'", () => { expect(configSnippet("group", "team", { name: "Team", groupTypeId: 2, campusId: null })).toBe( - 'group({ key: "team", name: "Team", groupTypeId: 2 });', + ["group({", ' key: "team",', ' name: "Team",', " groupTypeId: 2,", "});"].join("\n"), ); }); }); From ef317f16e1d74c9abd38e806884ac9374ac18034 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Thu, 9 Jul 2026 14:57:40 +0200 Subject: [PATCH 5/5] fix(state): treat undefined-valued keys as missing in fieldsEqual (#52) managedFields for group-type/age-group/target-group/relationship-type/ group-role yields undefined-valued optional keys (nameTranslated, sortKey, degreeNameA/B) when the API omits them, but JSON.stringify drops those keys on save. Re-adopting compared the persisted {name} against a fresh {name, nameTranslated: undefined} by raw key count, saw a mismatch, and spuriously bumped updatedAt. fieldsEqual now filters undefined-valued keys before comparing, both top-level and recursively, mirroring JSON round-trip semantics. --- src/state/state.ts | 14 +++++++++++--- tests/state.test.ts | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/state/state.ts b/src/state/state.ts index d91d028..10a2fe1 100644 --- a/src/state/state.ts +++ b/src/state/state.ts @@ -217,6 +217,13 @@ function isNotFound(err: unknown): boolean { * foundational module that imports no engine code — stays self-contained (mirrors the engine's * `deepEqual`, but a key-order difference between two structurally-identical snapshots must not be * seen as a change here either). + * + * Undefined-valued keys are treated as absent (`undefined === missing`), on both sides and + * recursively at every level. This mirrors `JSON.stringify`/`JSON.parse` round-tripping, which is + * how state is actually persisted and reloaded: a fresh snapshot built from `managedFields` can carry + * explicit `foo: undefined` for an optional field the API omitted (e.g. group-type `nameTranslated`), + * while the persisted snapshot loaded from disk simply lacks the key. Without this, re-adopting an + * unchanged resource sees a key-count mismatch and spuriously bumps `updatedAt`. */ function fieldsEqual(a: unknown, b: unknown): boolean { if (a === b) return true; @@ -227,7 +234,8 @@ function fieldsEqual(a: unknown, b: unknown): boolean { } const ao = a as Record; const bo = b as Record; - const aKeys = Object.keys(ao); - if (aKeys.length !== Object.keys(bo).length) return false; - return aKeys.every((k) => Object.prototype.hasOwnProperty.call(bo, k) && fieldsEqual(ao[k], bo[k])); + const aKeys = Object.keys(ao).filter((k) => ao[k] !== undefined); + const bKeys = Object.keys(bo).filter((k) => bo[k] !== undefined); + if (aKeys.length !== bKeys.length) return false; + return aKeys.every((k) => bo[k] !== undefined && fieldsEqual(ao[k], bo[k])); } diff --git a/tests/state.test.ts b/tests/state.test.ts index 386a831..9f3ca21 100644 --- a/tests/state.test.ts +++ b/tests/state.test.ts @@ -46,6 +46,23 @@ describe("state.upsert", () => { expect(state.resources.mainz?.adoptedAt).toBe(NOW); }); + it("does not bump updatedAt when a fresh snapshot carries undefined-valued optional keys the persisted one lacks (#52: JSON round-trip parity)", () => { + // group-type's managedFields includes `nameTranslated` as an optional key: when the API omits + // it, the freshly-built fields object carries an explicit `nameTranslated: undefined`, while the + // snapshot loaded back from disk (via JSON.parse, after JSON.stringify dropped the undefined key + // on save) simply lacks the key. Re-adopting must see these as equal. + const state = emptyState(HOST); + const persisted = JSON.parse(JSON.stringify({ name: "Members", nameTranslated: undefined })); + upsert(state, { type: "group-type", id: 3, key: "gt-members", fields: persisted }, NOW); + expect(state.resources["gt-members"]?.fields).toEqual({ name: "Members" }); + + const fresh = { name: "Members", nameTranslated: undefined }; + const action = upsert(state, { type: "group-type", id: 3, key: "gt-members", fields: fresh }, LATER); + + expect(action).toBe("updated"); + expect(state.resources["gt-members"]?.updatedAt).toBe(NOW); + }); + it("bumps updatedAt only when the fields actually change", () => { const state = emptyState(HOST); upsert(state, { type: "campus", id: 0, key: "mainz", fields: { name: "Mainz" } }, NOW);