fix(add): make --update reachable for reference-kind configs (todos 757cefdb)

src/cli/add-reference-update.test.ts

@@ -0,0 +1,147 @@
+// Regression cover for todos 757cefdb: `instructions add <path> --kind reference`
+// can NEVER find an existing row to update, at any `--update` setting, because
+// the collision check that backs `--update` is target-path-only
+// (findConfigsByTargetPath) and reference configs own no target_path by design
+// (see config-target-identity.ts). Every re-ingest of a reference-kind config —
+// the kind that carries global/managed operating-rules content, not a config
+// mirrored 1:1 onto one file — therefore silently mints a new row instead of
+// updating the one that already exists.
+//
+// Measured live 2026-08-04 by t42d493a5-driver (todos 757cefdb comment
+// 47307cda): reproduced three ways on disposable rows, and confirmed the
+// installed fleet store held 20 reference-kind configs with target_path=null
+// on 20/20. Re-confirmed independently here via `instructions list --json`
+// against the same live store before writing this test: 163 total configs,
+// kind counts {file: 143, reference: 20}, and target_path null on all 20
+// reference rows and non-null on all 143 file rows — the same shape, on a
+// fresh read, is what makes this a load-bearing regression rather than a
+// one-off.
+import { describe, expect, test } from "bun:test";
+import { dirname, join } from "node:path";
+import { spawnSync } from "node:child_process";
+import { fileURLToPath } from "node:url";
+import { writeFileSync } from "node:fs";
+import { makeTempRoot } from "../lib/test-temp-root";
+
+const repoRoot = join(dirname(fileURLToPath(import.meta.url)), "../..");
+
+function runCli(args: string[], env: Record<string, string | undefined> = {}) {
+  return spawnSync("bun", ["src/cli/index.tsx", ...args], {
+    cwd: repoRoot,
+    encoding: "utf8",
+    env: {
+      ...process.env,
+      HASNA_INSTRUCTIONS_API_URL: undefined,
+      HASNA_INSTRUCTIONS_API_KEY: undefined,
+      ...env,
+      NO_COLOR: "1",
+      FORCE_COLOR: "0",
+    },
+  });
+}
+
+function isolatedEnv(root: string) {
+  return { HASNA_INSTRUCTIONS_DB_PATH: join(root, "db.sqlite"), CONFIGS_HOME: root };
+}
+
+function referenceRowsNamed(root: string, name: string): Array<{ slug: string; content: string; version: number }> {
+  const listed = runCli(["list", "--json"], isolatedEnv(root));
+  expect(listed.status).toBe(0);
+  const all = JSON.parse(listed.stdout) as Array<{ slug: string; name: string; kind: string; content: string; version: number }>;
+  return all.filter((c) => c.kind === "reference" && c.name === name).map(({ slug, content, version }) => ({ slug, content, version }));
+}
+
+describe("instructions add --kind reference — one name, one row", () => {
+  test("refuses a second reference add for a name a config already owns, and names that config", () => {
+    const root = makeTempRoot("configs-add-ref-dup-");
+    const source = join(root, "rule.md");
+    writeFileSync(source, "alpha\n");
+
+    const first = runCli(["add", source, "--name", "sample-rule", "--kind", "reference"], isolatedEnv(root));
+    expect(first.status).toBe(0);
+    expect(referenceRowsNamed(root, "sample-rule").length).toBe(1);
+
+    const second = runCli(["add", source, "--name", "sample-rule", "--kind", "reference"], isolatedEnv(root));
+
+    // It must refuse rather than mint a twin — the same contract `add` already
+    // gives file-kind configs on a colliding target_path.
+    expect(second.status).not.toBe(0);
+    expect(`${second.stdout}${second.stderr}`).toContain("sample-rule");
+    const rows = referenceRowsNamed(root, "sample-rule");
+    expect(rows.length).toBe(1);
+  });
+
+  test("--update refreshes the existing reference row in place instead of adding one", () => {
+    const root = makeTempRoot("configs-add-ref-update-");
+    const source = join(root, "rule.md");
+    writeFileSync(source, "alpha\n");
+    expect(runCli(["add", source, "--name", "sample-rule", "--kind", "reference"], isolatedEnv(root)).status).toBe(0);
+
+    writeFileSync(source, "alpha\nbeta\n");
+    const updated = runCli(["add", source, "--name", "sample-rule", "--kind", "reference", "--update"], isolatedEnv(root));
+
+    expect(updated.status).toBe(0);
+    const rows = referenceRowsNamed(root, "sample-rule");
+    // The failure mode this guards: before the fix, this array has length 2
+    // (a fresh "sample-rule-1" twin) instead of one updated row.
+    expect(rows.length).toBe(1);
+    expect(rows[0]!.content).toBe("alpha\nbeta\n");
+    expect(rows[0]!.version).toBe(2);
+  });
+
+  test("--update on a reference row preserves the prior content as a snapshot", () => {
+    const root = makeTempRoot("configs-add-ref-snapshot-");
+    const source = join(root, "rule.md");
+    writeFileSync(source, "v1 content\n");
+    expect(runCli(["add", source, "--name", "sample-rule", "--kind", "reference"], isolatedEnv(root)).status).toBe(0);
+
+    writeFileSync(source, "v2 content\n");
+    expect(runCli(["add", source, "--name", "sample-rule", "--kind", "reference", "--update"], isolatedEnv(root)).status).toBe(0);
+
+    const rows = referenceRowsNamed(root, "sample-rule");
+    expect(rows.length).toBe(1);
+
+    // `snapshot list` has no --json output; it prints "  v<N> <created_at> <id>"
+    // per row (src/cli/index.tsx, snapshotCmd "list <config>"). Parse that
+    // contract directly rather than inventing a flag that does not exist.
+    const listed = runCli(["snapshot", "list", rows[0]!.slug], isolatedEnv(root));
+    expect(listed.status).toBe(0);
+    const v1Line = listed.stdout.split("\n").find((line) => /^\s*v1\s/.test(line));
+    expect(v1Line).toBeDefined();
+    const v1Id = v1Line!.trim().split(/\s+/)[2];
+    expect(v1Id).toBeTruthy();
+
+    const shown = runCli(["snapshot", "show", v1Id!], isolatedEnv(root));
+    expect(shown.status).toBe(0);
+    expect(shown.stdout.trimEnd()).toBe("v1 content");
+  });
+
+  test("still adds a genuinely new reference — the guard is per-name, not a blanket refusal", () => {
+    const root = makeTempRoot("configs-add-ref-distinct-");
+    const first = join(root, "one.md");
+    const second = join(root, "two.md");
+    writeFileSync(first, "one\n");
+    writeFileSync(second, "two\n");
+
+    expect(runCli(["add", first, "--name", "rule-one", "--kind", "reference"], isolatedEnv(root)).status).toBe(0);
+    const added = runCli(["add", second, "--name", "rule-two", "--kind", "reference"], isolatedEnv(root));
+
+    expect(added.status).toBe(0);
+    expect(referenceRowsNamed(root, "rule-one").length).toBe(1);
+    expect(referenceRowsNamed(root, "rule-two").length).toBe(1);
+  });
+
+  test("file-kind add/--update behavior is unchanged by this fix", () => {
+    const root = makeTempRoot("configs-add-file-unaffected-");
+    const target = join(root, "sample.md");
+    writeFileSync(target, "alpha\n");
+    expect(runCli(["add", target, "--name", "sample.md"], isolatedEnv(root)).status).toBe(0);
+
+    const dup = runCli(["add", target, "--name", "sample.md"], isolatedEnv(root));
+    expect(dup.status).not.toBe(0);
+
+    writeFileSync(target, "alpha\nbeta\n");
+    const updated = runCli(["add", target, "--name", "sample.md", "--update"], isolatedEnv(root));
+    expect(updated.status).toBe(0);
+  });
+});

src/cli/doctor-reference-duplicates.test.ts

@@ -0,0 +1,85 @@
+// CLI-level cover for the `instructions doctor` check added alongside todos
+// 757cefdb: a reference-kind config's identity is its name, not a target_path
+// (it has none — see config-target-identity.ts). Before that fix, `add <path>
+// --kind reference --update` had no way to find an existing row, so every
+// re-ingest minted a duplicate. This checks that `doctor` surfaces any such
+// duplicates that already exist, the same way it already surfaces duplicate
+// target-path rows.
+import { describe, expect, test } from "bun:test";
+import { dirname, join } from "node:path";
+import { spawnSync } from "node:child_process";
+import { fileURLToPath } from "node:url";
+import { writeFileSync } from "node:fs";
+import { makeTempRoot } from "../lib/test-temp-root";
+
+const repoRoot = join(dirname(fileURLToPath(import.meta.url)), "../..");
+
+function runCli(args: string[], env: Record<string, string | undefined> = {}) {
+  return spawnSync("bun", ["src/cli/index.tsx", ...args], {
+    cwd: repoRoot,
+    encoding: "utf8",
+    env: {
+      ...process.env,
+      // Isolation is the point of this file: an ambient cloud-mode credential
+      // silently overrides HASNA_INSTRUCTIONS_DB_PATH below and every write
+      // this test makes would land in the shared fleet store instead of the
+      // temp sqlite file. Clearing all three here, not just the DB path, is
+      // load-bearing — confirmed the hard way while writing this fix, on the
+      // live store, and cleaned up immediately (todos 757cefdb).
+      HASNA_INSTRUCTIONS_API_URL: undefined,
+      HASNA_INSTRUCTIONS_API_KEY: undefined,
+      HASNA_INSTRUCTIONS_STORAGE_MODE: undefined,
+      ...env,
+      NO_COLOR: "1",
+      FORCE_COLOR: "0",
+    },
+  });
+}
+
+function isolatedEnv(root: string) {
+  return { HASNA_INSTRUCTIONS_DB_PATH: join(root, "db.sqlite"), CONFIGS_HOME: root };
+}
+
+describe("instructions doctor — reference-name duplicates", () => {
+  test("passes clean when no reference name is claimed by more than one row", () => {
+    const root = makeTempRoot("doctor-ref-clean-");
+    const source = join(root, "rule.md");
+    writeFileSync(source, "alpha\n");
+    expect(runCli(["add", source, "--name", "solo-rule", "--kind", "reference"], isolatedEnv(root)).status).toBe(0);
+
+    const doctor = runCli(["doctor"], isolatedEnv(root));
+    expect(doctor.status).toBe(0);
+    expect(doctor.stdout).toContain("No reference config name is claimed by more than one row");
+  });
+
+  test("reports a pre-existing duplicate by name, id, and updated_at", () => {
+    const root = makeTempRoot("doctor-ref-dup-");
+
+    // `add`'s own guard now refuses to create this duplicate through the CLI —
+    // that is the fix. So to test `doctor`'s detection of a duplicate that
+    // ALREADY exists (e.g. left over from before this fix shipped), seed one
+    // directly at the DB layer, the same way the unit tests in
+    // config-target-identity.test.ts do, but out-of-process against the same
+    // isolated sqlite file `doctor` will then read.
+    const seed = spawnSync(
+      "bun",
+      [
+        "-e",
+        `
+        import { createConfig } from "./src/db/configs.ts";
+        createConfig({ name: "Twin Rule", category: "rules", content: "one", kind: "reference" });
+        createConfig({ name: "Twin Rule", category: "rules", content: "two", kind: "reference" });
+        `,
+      ],
+      { cwd: repoRoot, encoding: "utf8", env: { ...process.env, ...isolatedEnv(root), HASNA_INSTRUCTIONS_API_URL: undefined, HASNA_INSTRUCTIONS_API_KEY: undefined, HASNA_INSTRUCTIONS_STORAGE_MODE: undefined } },
+    );
+    expect(seed.status).toBe(0);
+
+    const doctor = runCli(["doctor"], isolatedEnv(root));
+    // `doctor` never fails the process on findings (see the existing
+    // duplicate-target-path check it mirrors) — it reports and continues.
+    expect(doctor.status).toBe(0);
+    expect(doctor.stdout).toContain("reference name(s) claimed by more than one row");
+    expect(doctor.stdout).toContain("Twin Rule");
+  });
+});

src/cli/index.tsx

@@ -6,7 +6,7 @@ import { existsSync, lstatSync, readFileSync, readSync, writeSync } from "node:f
 import { homedir } from "node:os";
 import { basename, join, resolve } from "node:path";
 import { applyConfigsWithReport, expandPath } from "../lib/apply.js";
-import { findConfigsByTargetPath, findDuplicateTargetPathGroups } from "../lib/config-target-identity.js";
+import { findConfigsByTargetPath, findDuplicateTargetPathGroups, findReferenceConfigsByName, findDuplicateReferenceNameGroups } from "../lib/config-target-identity.js";
 import { diffConfig, syncKnown, syncToDisk, syncProject, detectCategory, detectAgent, detectFormat, KNOWN_CONFIGS } from "../lib/sync.js";
 import { syncFromDir } from "../lib/sync-dir.js";
 import { redactContent, scanSecrets } from "../lib/redact.js";
@@ -448,31 +448,61 @@ program
     const name = opts.name || filePath.split("/").pop()!;
     const store = resolveConfigStore();
 
-    // One target path, one row. Without this, a second `add` of a file the store
-    // already tracks INSERTED a twin (uniqueSlug appending `-1`), and two rows on
-    // one path make `apply` race itself — last writer wins, silently. Refusing by
-    // default rather than updating is deliberate: the stored row may hold
-    // redacted or templateized content that the literal bytes on disk would
-    // flatten, so overwriting it is the operator's call, not a side effect of
-    // re-running `add`.
+    // One identity, one row. Without this, a second `add` of something the
+    // store already tracks INSERTED a twin (uniqueSlug appending `-1`), and two
+    // rows on one identity make `apply`/`session render` race each other — last
+    // writer wins, silently. Refusing by default rather than updating is
+    // deliberate: the stored row may hold redacted or templateized content that
+    // the literal bytes on disk would flatten, so overwriting it is the
+    // operator's call, not a side effect of re-running `add`.
+    //
+    // A file-kind config's identity is its target_path (one file, one owner).
+    // A reference-kind config owns no target_path — it is not mirrored 1:1 onto
+    // one file, so `findConfigsByTargetPath` never matches it, by design (see
+    // that function's doc comment) — its identity is its NAME instead (via
+    // slug). Before this fix `existingOwners` was hardcoded to `[]` for
+    // reference kind, so `--update` had no row to find at any setting and every
+    // re-ingest of a reference config silently minted a duplicate. Fixed per
+    // todos 757cefdb, evidenced 2026-08-04: 20/20 reference-kind rows in the
+    // fleet store had target_path=null, so this was not a corner case — it is
+    // the population that carries managed operating-rules content.
+    const allConfigs = await store.listConfigs();
     const existingOwners = opts.kind === "reference"
-      ? []
-      : findConfigsByTargetPath(await store.listConfigs(), targetPath);
+      ? findReferenceConfigsByName(allConfigs, name)
+      : findConfigsByTargetPath(allConfigs, targetPath);
+    const isReference = opts.kind === "reference";
+    const identityLabel = isReference ? `Reference config "${name}"` : targetPath;
+    const identityNoun = isReference ? "name" : "path";
 
     if (existingOwners.length > 0 && !opts.update) {
       const owners = existingOwners.map((owner) => `${owner.slug} (${owner.id})`).join(", ");
-      console.error(chalk.red(`${targetPath} is already tracked by: ${owners}`));
+      console.error(chalk.red(`${identityLabel} is already tracked by: ${owners}`));
       if (existingOwners.length > 1) {
-        console.error(chalk.red(`  ${existingOwners.length} rows already collide on this path — apply order between them is undefined.`));
+        console.error(chalk.red(`  ${existingOwners.length} rows already collide on this ${identityNoun} — apply order between them is undefined.`));
       }
       console.error(chalk.dim("  Use `instructions add <path> --update` to refresh that row in place,"));
-      console.error(chalk.dim("  `instructions sync` to pull disk changes in, or `instructions delete <id>` first."));
+      if (isReference) {
+        console.error(chalk.dim("  or `instructions delete <id>` first."));
+      } else {
+        console.error(chalk.dim("  `instructions sync` to pull disk changes in, or `instructions delete <id>` first."));
+      }
       process.exit(1);
     }
 
     let config: Config;
     if (existingOwners.length > 0) {
       const [target, ...rest] = existingOwners;
+      // Preserve provenance: capture the row's current content and version as a
+      // snapshot BEFORE it is overwritten, whenever the content is actually
+      // changing. This is the same primitive `apply.ts` already uses to protect
+      // a disk file's previous content right before a render overwrites it
+      // (`store.createSnapshot`) — applied here at the DB-write boundary
+      // instead, so a content edit via `add --update` is recoverable via
+      // `instructions snapshot list/restore` even before anything is ever
+      // applied or rendered again.
+      if (content !== target!.content) {
+        await store.createSnapshot(target!.id, target!.content, target!.version);
+      }
       config = await store.updateConfig(target!.id, {
         content,
         format: fmt,
@@ -482,7 +512,7 @@ program
       });
       console.log(chalk.green("✓") + ` Updated: ${chalk.bold(config.name)} ${chalk.dim(`(${config.slug})`)}`);
       if (rest.length > 0) {
-        console.log(chalk.yellow(`  ⚠ ${rest.length} other row(s) still target ${targetPath}: ${rest.map((r) => r.slug).join(", ")}`));
+        console.log(chalk.yellow(`  ⚠ ${rest.length} other row(s) still share this ${identityNoun}: ${rest.map((r) => r.slug).join(", ")}`));
         console.log(chalk.yellow("    Apply order between them is undefined. Delete the extras."));
       }
       if (redacted.length > 0) {
@@ -1695,6 +1725,26 @@ program
       console.log(chalk.dim("      Keep one row per path: `instructions delete <id>` for the extras."));
     }
 
+    // Same failure, the reference-kind identity axis: a reference config's
+    // identity is its NAME, not a target_path (it has none). Before todos
+    // 757cefdb, `add --update` had no way to find an existing reference row at
+    // all, so every re-ingest minted a fresh one — this reports rows that
+    // accumulated during that window so they can be reconciled by hand.
+    const duplicateReferenceNames = findDuplicateReferenceNameGroups(allConfigs);
+    if (duplicateReferenceNames.length === 0) {
+      pass("No reference config name is claimed by more than one row");
+    } else {
+      const rowCount = duplicateReferenceNames.reduce((total, group) => total + group.configs.length, 0);
+      fail(`${duplicateReferenceNames.length} reference name(s) claimed by more than one row (${rowCount} rows) — only one is live in the next render`);
+      for (const group of duplicateReferenceNames) {
+        console.log(chalk.yellow(`      ${group.name}`));
+        for (const c of group.configs) {
+          console.log(chalk.dim(`        ${c.slug}  (${c.id})  updated ${c.updated_at}`));
+        }
+      }
+      console.log(chalk.dim("      Keep one row per name: `instructions delete <id>` for the extras."));
+    }
+
     console.log(`\n${issues === 0 ? chalk.green("✓ All checks passed") : chalk.yellow(`${issues} issue(s) found`)}`);
   });