refactor(lib): harden shared-file-writer derivation against drift

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<string, Set<Feature>> => {
@@ -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<Feature, number>(

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/<name>/`).
+ *
+ * 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<string> = new Set([
@@ -54,7 +74,13 @@ type FactoryClass = {
 
 type FactoryMap = ReadonlyMap<ToolTarget, { readonly class: FactoryClass }>;
 
-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 }),
 ];
 
 /**