diff --git a/src/lib/shared-file-derive.test.ts b/src/lib/shared-file-derive.test.ts index 577db75fd..eb991f553 100644 --- a/src/lib/shared-file-derive.test.ts +++ b/src/lib/shared-file-derive.test.ts @@ -1,11 +1,13 @@ import { describe, expect, it } from "vitest"; -import type { Feature } from "../types/features.js"; +import { ALL_FEATURES, type Feature } from "../types/features.js"; import { GENERATION_STEP_GRAPH } from "./generate.js"; import { deriveSharedFileWriters, deriveSharedWriteSteps, + NON_SHARED_WRITE_FEATURES, SHARED_WRITE_FEATURE_ORDER, + settablePathsForScope, } from "./shared-file-derive.js"; const graphWritersByFile = (): Map> => { @@ -53,6 +55,34 @@ describe("shared-file write derivation", () => { expect(() => deriveSharedWriteSteps()).not.toThrow(); }); + it("classifies every Feature as either a shared writer or an explicit non-shared-writer", () => { + // Guards #2110 mid#1: SHARED_WRITE_FEATURE_ORDER is hand-maintained, so a + // feature that later starts writing a shared file could be silently + // forgotten. Requiring the two lists to partition ALL_FEATURES exactly means + // adding a Feature forces classifying it in one of them (a duplicate would + // make `classified` longer than ALL_FEATURES, so this also rejects overlap). + const classified = [...SHARED_WRITE_FEATURE_ORDER, ...NON_SHARED_WRITE_FEATURES].toSorted(); + expect(classified).toEqual([...ALL_FEATURES].toSorted()); + }); + + it("collects getExtraSharedWritePaths even when getSettablePaths throws", () => { + // Guards #2110 mid#2: the two hooks are independent, so a throwing + // getSettablePaths must not suppress a tool's (possibly scope-only) extra + // shared path. + const cls = { + getSettablePaths: () => { + throw new Error("boom"); + }, + getExtraSharedWritePaths: () => [ + { relativeDirPath: ".config/example", relativeFilePath: "example.json" }, + ], + }; + + expect(settablePathsForScope({ cls, global: true })).toEqual([ + { relativeDirPath: ".config/example", relativeFilePath: "example.json" }, + ]); + }); + it("orders every writer pair of every shared file by dependsOn edges", () => { const steps = deriveSharedWriteSteps(); const orderIndex = new Map( diff --git a/src/lib/shared-file-derive.ts b/src/lib/shared-file-derive.ts index a1072a4fb..51f3f5310 100644 --- a/src/lib/shared-file-derive.ts +++ b/src/lib/shared-file-derive.ts @@ -34,6 +34,26 @@ export const SHARED_WRITE_FEATURE_ORDER = [ "rules", ] as const satisfies readonly Feature[]; +/** + * Features that deliberately never participate in shared read-modify-write + * config files: each writes its own dedicated per-item artifact, never a file + * another feature also writes. + * + * - `commands`: standalone per-command files (e.g. `.claude/commands/*.md`). + * - `skills`: standalone per-skill directories (e.g. `.claude/skills//`). + * + * Kept as an explicit list — not an implicit "everything not in + * {@link SHARED_WRITE_FEATURE_ORDER}" — so that adding a new `Feature` forces a + * conscious classification: the exhaustiveness test in + * `shared-file-derive.test.ts` fails until the new feature is placed in either + * this list or `SHARED_WRITE_FEATURE_ORDER`. That closes the drift where a + * feature which later starts writing a shared file is silently forgotten. + */ +export const NON_SHARED_WRITE_FEATURES = [ + "commands", + "skills", +] as const satisfies readonly Feature[]; + // Deprecated aliases; a guard for the day one diverges from its canonical // target's paths. A no-op today since they reuse the canonical class and paths. const TARGETS_NOT_DERIVED: ReadonlySet = new Set([ @@ -54,7 +74,13 @@ type FactoryClass = { type FactoryMap = ReadonlyMap; -const settablePathsForScope = (cls: FactoryClass, global: boolean): SharedWritePath[] => { +export const settablePathsForScope = ({ + cls, + global, +}: { + cls: FactoryClass; + global: boolean; +}): SharedWritePath[] => { const paths: SharedWritePath[] = []; let settable: | { @@ -67,7 +93,12 @@ const settablePathsForScope = (cls: FactoryClass, global: boolean): SharedWriteP try { settable = cls.getSettablePaths?.({ global }) as typeof settable; } catch { - return paths; + // Fail-open (see the extra-paths note below), but do NOT early-return: the + // two hooks are independent, so a throwing getSettablePaths must not also + // suppress this tool's getExtraSharedWritePaths — otherwise a tool whose + // global getSettablePaths throws while it has a global-only extra shared + // path would silently drop that path from the write order. + settable = undefined; } if (settable?.relativeFilePath) { paths.push({ @@ -85,11 +116,11 @@ const settablePathsForScope = (cls: FactoryClass, global: boolean): SharedWriteP // trade-off is that a broken getExtraSharedWritePaths silently drops that // tool's extra shared paths from the write order — acceptable because every // current implementation returns a static constant that cannot throw. - let extra: SharedWritePath[]; + let extra: SharedWritePath[] = []; try { extra = cls.getExtraSharedWritePaths?.({ global }) ?? []; } catch { - return paths; + extra = []; } for (const path of extra) { if (path.relativeFilePath) paths.push(path); @@ -101,8 +132,8 @@ const settablePathsForScope = (cls: FactoryClass, global: boolean): SharedWriteP // `.config/opencode/opencode.json`), so a feature's writes are the union across // both scopes; the step graph declares that union. const collectFactoryPaths = (factory: { class: FactoryClass }): SharedWritePath[] => [ - ...settablePathsForScope(factory.class, false), - ...settablePathsForScope(factory.class, true), + ...settablePathsForScope({ cls: factory.class, global: false }), + ...settablePathsForScope({ cls: factory.class, global: true }), ]; /**