From 39b1cd7d808e003796185cb7def310b1a05f0f37 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Tue, 4 Aug 2026 13:02:25 +0300 Subject: [PATCH] fix(add): make `--update` reachable for reference-kind configs (todos 757cefdb) `instructions add --kind reference --update` could never find an existing row, at any setting. `add`'s collision check (`--update`'s only identity signal) was `findConfigsByTargetPath`, which by design never matches a reference config -- reference configs own no target_path, because they are not mirrored 1:1 onto one file (they carry managed operating-rules content instead). `add` hardcoded `existingOwners = []` whenever `opts.kind === "reference"`, so every re-ingest of a reference config's content silently minted a fresh row instead of updating the one that already existed. Measured live in the fleet store before this fix: 8 reference rows named "Global Agent Rules Standard" (slugs -1 through -8), one identical SHA-256 content hash across all 8, ingested across 6 different days -- the exact failure this fixes, already caught in the wild rather than hypothetical. Fix: add `findReferenceConfigsByName`, using name (and its slug) as a reference config's identity, the same role target_path plays for file-kind configs. `uniqueSlug` only de-duplicates the SLUG column (which carries a DB UNIQUE constraint); it never touches `name` (which carries none), so the match is on EITHER an exact name match (to also catch legacy duplicates already sitting in the store, whose name is identical and whose slug is a `-1`/`-2`/... suffix) OR a slugified-name match (for a re-ingest whose `--name` differs only in case or punctuation). Also: - `add --update` now snapshots a row's content before overwriting it (reusing the same `store.createSnapshot` primitive `apply.ts` already uses before overwriting a rendered file), whenever content actually changes -- so a content edit is recoverable via `snapshot restore` even before anything is next applied or rendered. - `instructions doctor` gains a `findDuplicateReferenceNameGroups` check, mirroring the existing duplicate-target-path check, so existing collisions like the one above are visible instead of invisible. - The refusal message (no `--update` on a colliding identity) is now identity-aware: it names the config by path for file-kind and by name for reference-kind, and drops the `instructions sync` suggestion for reference-kind (sync only applies to disk-mirrored file-kind configs). Root cause and regression coverage per Fix Once: TDD -- the new CLI-level regression test (src/cli/add-reference-update.test.ts) reproduces the duplicate-row bug against the pre-fix code (3 of 5 cases fail with a fresh duplicate row and no snapshot); all 5 pass post-fix. Unit coverage for both new pure functions in src/lib/config-target-identity.test.ts, plus a CLI-level test for the new `doctor` check (src/cli/doctor-reference-duplicates.test.ts). Full suite: 593/593 pass, typecheck clean. Scope, stated so it isn't oversold: this closes the concretely blocking half of todos 757cefdb (a route that updates a managed instruction source without minting a duplicate, in place, avoiding hand-edited generated files -- the last of which was already true architecturally). It does NOT add a review gate before rollout, and it does NOT add an actor/provenance column to the configs table (none exists today) -- a full proposal -> review -> rollout state machine was built once for this exact row in 2026-07-17 across this repo and hasna/identities, fully tested, but never merged and is no longer recoverable (verified: no trace in this repo's git history, branches, or worktrees). That larger gap is filed separately rather than reattempted here, to avoid repeating the scope that stalled it before. Agent: t757cefdb-driver --- src/cli/add-reference-update.test.ts | 147 ++++++++++++++++++++ src/cli/doctor-reference-duplicates.test.ts | 85 +++++++++++ src/cli/index.tsx | 78 +++++++++-- src/lib/config-target-identity.test.ts | 94 ++++++++++++- src/lib/config-target-identity.ts | 58 ++++++++ 5 files changed, 447 insertions(+), 15 deletions(-) create mode 100644 src/cli/add-reference-update.test.ts create mode 100644 src/cli/doctor-reference-duplicates.test.ts diff --git a/src/cli/add-reference-update.test.ts b/src/cli/add-reference-update.test.ts new file mode 100644 index 0000000..9958f5c --- /dev/null +++ b/src/cli/add-reference-update.test.ts @@ -0,0 +1,147 @@ +// Regression cover for todos 757cefdb: `instructions add --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 = {}) { + 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 " + // per row (src/cli/index.tsx, snapshotCmd "list "). 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); + }); +}); diff --git a/src/cli/doctor-reference-duplicates.test.ts b/src/cli/doctor-reference-duplicates.test.ts new file mode 100644 index 0000000..64a132c --- /dev/null +++ b/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 +// --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 = {}) { + 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"); + }); +}); diff --git a/src/cli/index.tsx b/src/cli/index.tsx index 939df50..c4bc7cd 100644 --- a/src/cli/index.tsx +++ b/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 --update` to refresh that row in place,")); - console.error(chalk.dim(" `instructions sync` to pull disk changes in, or `instructions delete ` first.")); + if (isReference) { + console.error(chalk.dim(" or `instructions delete ` first.")); + } else { + console.error(chalk.dim(" `instructions sync` to pull disk changes in, or `instructions delete ` 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 ` 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 ` for the extras.")); + } + console.log(`\n${issues === 0 ? chalk.green("✓ All checks passed") : chalk.yellow(`${issues} issue(s) found`)}`); }); diff --git a/src/lib/config-target-identity.test.ts b/src/lib/config-target-identity.test.ts index 218b60b..d1553ff 100644 --- a/src/lib/config-target-identity.test.ts +++ b/src/lib/config-target-identity.test.ts @@ -3,7 +3,7 @@ import { mkdirSync, existsSync, rmSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { getDatabase, resetDatabase } from "../db/database"; import { createConfig } from "../db/configs"; -import { findConfigsByTargetPath } from "./config-target-identity"; +import { findConfigsByTargetPath, findReferenceConfigsByName, findDuplicateReferenceNameGroups } from "./config-target-identity"; import { tempRootPath } from "./test-temp-root"; let tmpDir: string; @@ -78,3 +78,95 @@ describe("findConfigsByTargetPath", () => { expect(new Set(found.map((c) => c.id))).toEqual(new Set([first.id, second.id])); }); }); + +describe("findReferenceConfigsByName", () => { + test("finds the reference row that already owns a name", () => { + const db = getDatabase(); + const existing = createConfig({ name: "sample-rule", category: "rules", content: "doc", kind: "reference" }, db); + + const found = findReferenceConfigsByName([existing], "sample-rule"); + + expect(found.map((c) => c.id)).toEqual([existing.id]); + }); + + test("matches on slug, so case/punctuation differences in --name still resolve to the same row", () => { + const db = getDatabase(); + const existing = createConfig({ name: "Sample Rule", category: "rules", content: "doc", kind: "reference" }, db); + + // uniqueSlug(slugify) turns "Sample Rule" into "sample-rule" at creation; + // a later re-ingest that types the name slightly differently but produces + // the same slug must still resolve to this row. + const found = findReferenceConfigsByName([existing], "sample rule"); + + expect(found.map((c) => c.id)).toEqual([existing.id]); + }); + + test("does not match a different name", () => { + const db = getDatabase(); + const existing = createConfig({ name: "sample-rule", category: "rules", content: "doc", kind: "reference" }, db); + + expect(findReferenceConfigsByName([existing], "other-rule")).toEqual([]); + }); + + test("ignores file configs, which are identified by target_path, not name", () => { + const db = getDatabase(); + const target = join(tmpDir, "sample-rule.md"); + writeFileSync(target, "body\n"); + const file = createConfig({ name: "sample-rule", category: "tools", content: "body\n", target_path: target }, db); + + // Same name a reference config might use, but this row is file-kind — + // its identity is target_path, so a reference-name lookup must not match it. + expect(findReferenceConfigsByName([file], "sample-rule")).toEqual([]); + }); + + test("reports EVERY row on a colliding name, not just the first", () => { + const db = getDatabase(); + // Same `name` twice: uniqueSlug (db/database.ts) de-duplicates the SLUG + // column only, so this reproduces exactly what the pre-fix bug already + // left behind live (measured 2026-08-04: 8 rows named "Global Agent Rules + // Standard", slugs suffixed -1..-8, one identical content hash across all + // 8) — two rows, same name, different slugs. + const first = createConfig({ name: "collision-rule", category: "rules", content: "one", kind: "reference" }, db); + const second = createConfig({ name: "collision-rule", category: "rules", content: "two", kind: "reference" }, db); + expect(first.slug).not.toBe(second.slug); + + const found = findReferenceConfigsByName([first, second], "collision-rule"); + + expect(found.length).toBe(2); + expect(new Set(found.map((c) => c.id))).toEqual(new Set([first.id, second.id])); + }); +}); + +describe("findDuplicateReferenceNameGroups", () => { + test("reports a name shared by more than one reference row", () => { + const db = getDatabase(); + const first = createConfig({ name: "Shared Name", category: "rules", content: "one", kind: "reference" }, db); + const second = createConfig({ name: "Shared Name", category: "rules", content: "two", kind: "reference" }, db); + + const groups = findDuplicateReferenceNameGroups([first, second]); + + expect(groups.length).toBe(1); + expect(groups[0]!.name).toBe("Shared Name"); + expect(new Set(groups[0]!.configs.map((c) => c.id))).toEqual(new Set([first.id, second.id])); + }); + + test("an empty result means the store is clean — no false positive on distinct names", () => { + const db = getDatabase(); + const first = createConfig({ name: "Rule One", category: "rules", content: "one", kind: "reference" }, db); + const second = createConfig({ name: "Rule Two", category: "rules", content: "two", kind: "reference" }, db); + + expect(findDuplicateReferenceNameGroups([first, second])).toEqual([]); + }); + + test("ignores file-kind configs even if their name collides with a reference config's", () => { + const db = getDatabase(); + const target = join(tmpDir, "shared.md"); + writeFileSync(target, "body\n"); + const file = createConfig({ name: "Shared Name", category: "tools", content: "body\n", target_path: target }, db); + const ref = createConfig({ name: "Shared Name", category: "rules", content: "doc", kind: "reference" }, db); + + // Only one reference row named "Shared Name" — the file-kind row is a + // different identity axis entirely and must not count toward this group. + expect(findDuplicateReferenceNameGroups([file, ref])).toEqual([]); + }); +}); diff --git a/src/lib/config-target-identity.ts b/src/lib/config-target-identity.ts index ec3798e..a1dcd3c 100644 --- a/src/lib/config-target-identity.ts +++ b/src/lib/config-target-identity.ts @@ -1,5 +1,6 @@ import type { Config } from "../types/index.js"; import { normalizeTargetPath } from "./apply.js"; +import { slugify } from "../db/database.js"; /** * Every config row that already writes to `targetPath`. @@ -25,6 +26,63 @@ export function findConfigsByTargetPath(configs: Config[], targetPath: string): }); } +/** + * Every reference-kind row already ingested under `name`. + * + * A reference config (`kind: "reference"`) owns no target_path — it is not + * mirrored 1:1 onto one file on disk, so `findConfigsByTargetPath` can never + * find it, by design (see that function's own doc comment). That left + * `add --kind reference --update` with no identity signal at all: every + * re-ingest of a reference config's content minted a fresh row instead of + * updating the one that already existed, at every `--update` setting, because + * there was nothing to match on. Measured live 2026-08-04, todos 757cefdb: 20 + * of 20 reference-kind rows in the fleet store had target_path=null. + * + * For a reference config, `name` IS the identity, in exactly the same sense a + * file-kind config's target_path is its identity: re-ingest with the same + * name, get the same row; re-ingest with a different name, get a different + * (or new) row. Two comparisons, both needed: + * + * - EXACT name match. `uniqueSlug` (db/database.ts) only de-duplicates the + * SLUG column, which carries a DB-level UNIQUE constraint — it never + * touches `name`, which carries no such constraint. So every prior + * duplicate this bug already produced still has the identical `name` and a + * `-1`, `-2`, ... suffixed slug. Measured live 2026-08-04 in the fleet + * store: 8 reference rows all named "Global Agent Rules Standard" + * (`global-agent-rules-standard-1` through `-8`), one identical SHA-256 + * content hash across all 8, ingested on 6 different days — the exact + * shape this function exists to stop. Matching on slugified name ALONE + * would find only the newest of the 9 and silently leave the other 8 + * orphaned on every future `--update` too. + * - Slugified name match, for a re-ingest whose `--name` differs only in + * case or punctuation from the name that produced the existing row's slug + * (slug is what the store already treats as the row's stable, human-typed + * identity — see `uniqueSlug`). + */ +export function findReferenceConfigsByName(configs: Config[], name: string): Config[] { + const wantedSlug = slugify(name); + return configs.filter( + (config) => config.kind === "reference" && (config.name === name || config.slug === wantedSlug) + ); +} + +/** + * Groups of reference-kind rows that collide on `name`, the mirror image of + * `findDuplicateTargetPathGroups` for the identity axis reference configs + * actually use. Only groups with more than one member are returned, so an + * empty result means the store is clean. + */ +export function findDuplicateReferenceNameGroups(configs: Config[]): Array<{ name: string; configs: Config[] }> { + const groups = new Map(); + for (const config of configs) { + if (config.kind !== "reference") continue; + groups.set(config.name, [...(groups.get(config.name) ?? []), config]); + } + return [...groups.entries()] + .filter(([, rows]) => rows.length > 1) + .map(([name, rows]) => ({ name, configs: rows })); +} + /** * Groups of rows that collide on one normalized target path. Only groups with * more than one member are returned, so an empty result means the store is