Procházet zdrojové kódy

fix(build): drop registry entries whose canonical source was deleted

Deleting a content/ source removed its .opencode/ output but left its
registry.json entry behind permanently, and the build reported "0 written,
41 unchanged" — stably wrong, so the drift gate could not see it. That is
the exact defect this emitter was built to cure and which its own header
condemns auto-detect-components.sh for: nothing ever re-derives, so entries
drift forever.

merge() carried any base entry whose id was not generated. The carry-through
is load-bearing — eval-runner ships in .opencode/agent/, sits in the
committed registry, and has no canonical source — but "never had a source"
and "source was deleted" are indistinguishable by id alone.

The manifest already encodes the distinction, so merge() now consults it:
an entry we previously wrote and no longer generate is dropped; one we never
wrote is carried. Of 34 registry agent entries, eval-runner is the only one
the ledger does not claim, so the populations separate cleanly. Every
ambiguity — missing or non-string path, no ledger at all — resolves toward
keeping data.

The ledger moves to its own module because BuildPipeline already imports
RegistryEmitter and reading it there would have closed an import cycle.
Both now provably consult the same ledger, so the two prune rules —
generated files and generated entries — cannot disagree.

Found by running the real command end to end; no test caught it. The new
tests were mutation-tested against the restored bug: exactly the drop cases
fail and the carry cases hold.
darrenhinde před 2 týdny
rodič
revize
9ca012e5ee

+ 99 - 0
packages/compatibility-layer/src/core/BuildManifest.ts

@@ -0,0 +1,99 @@
+/**
+ * The build's ledger: what `oac build` wrote, and where it was allowed to write it.
+ *
+ * ─── Why this is its own module ─────────────────────────────────────────────────────────
+ *
+ * The ledger answers one question — "did WE generate this path?" — and two callers need it:
+ *
+ *   - {@link BuildPipeline.write} prunes a generated FILE whose source is gone.
+ *   - {@link RegistryEmitter.emit} drops a generated registry ENTRY whose source is gone.
+ *
+ * Those are the same rule applied to two artefacts, so they must consult the same ledger or
+ * they will disagree — and a disagreement here means `.opencode/**` and `registry.json` drift
+ * apart, which is the exact class of bug this refactor exists to end. `BuildPipeline` already
+ * imports `RegistryEmitter` (it emits the registry as a build target), so the ledger cannot
+ * live in `BuildPipeline` without `RegistryEmitter` importing back into a cycle. It lives here
+ * instead, imported by both and owned by neither.
+ *
+ * ─── The ledger is what makes deletion safe ─────────────────────────────────────────────
+ *
+ * "Remove anything under `.opencode/agent/` without a `content/` source" would delete
+ * `.opencode/agent/eval-runner.md` — a real, shipped, hand-authored agent that has
+ * deliberately not been canonicalised. The rule is therefore inverted: the build removes only
+ * what IT PREVIOUSLY WROTE. A file the build has never generated is not in the ledger, cannot
+ * become a candidate, and is invisible to pruning no matter where it sits.
+ *
+ * That inversion is why an ABSENT or unreadable ledger prunes NOTHING rather than pruning
+ * everything: no record of having written a file is not evidence that we wrote it. The first
+ * build on a fresh clone therefore carries every pre-existing entry and deletes nothing.
+ */
+
+import { existsSync, readFileSync } from "node:fs";
+import { join, resolve } from "node:path";
+
+/** Where the ledger lives, relative to the repo root. */
+export const MANIFEST_FILE = ".oac/build-manifest.json";
+
+/** One line of the ledger: what the build wrote at a path, and where it was allowed to. */
+export interface ManifestEntry {
+  /** sha256 of the bytes the build wrote. */
+  sha256: string;
+  /** The target that produced it. */
+  target: string;
+  /**
+   * The output root this file was written under — `TARGET_ROOTS[target]`, rebased if the
+   * target was staged. Recorded rather than recomputed so pruning can bound itself without
+   * having to be told which staging layout a PREVIOUS build happened to use.
+   */
+  root: string;
+}
+
+/** The build's ledger of what it wrote. Deterministic: sorted keys, no timestamps. */
+export interface BuildManifest {
+  /** Ledger format version, so a future shape change is detectable rather than silent. */
+  version: 1;
+  /** Repo-relative POSIX path -> what the build wrote there. */
+  files: Record<string, ManifestEntry>;
+}
+
+/** Locale-independent ordering. `localeCompare` is locale-dependent — never use it here. */
+function compare(a: string, b: string): number {
+  return a < b ? -1 : a > b ? 1 : 0;
+}
+
+/** Read the previous manifest, or an empty one. An absent ledger prunes nothing — safe. */
+export function readManifest(root: string): BuildManifest {
+  const path = join(resolve(root), MANIFEST_FILE);
+  if (!existsSync(path)) return { version: 1, files: {} };
+
+  try {
+    const parsed = JSON.parse(readFileSync(path, "utf-8")) as BuildManifest;
+    // A ledger we cannot vouch for prunes nothing rather than pruning wrongly.
+    if (parsed.version !== 1 || typeof parsed.files !== "object" || parsed.files === null) {
+      return { version: 1, files: {} };
+    }
+    return parsed;
+  } catch {
+    return { version: 1, files: {} };
+  }
+}
+
+/**
+ * The set of repo-relative paths a previous build recorded generating.
+ *
+ * The discriminator both pruning rules turn on. A path in this set was emitted by us and may
+ * therefore be withdrawn when its source disappears; a path outside it is somebody else's file
+ * and is never ours to remove.
+ */
+export function generatedPaths(root: string): ReadonlySet<string> {
+  return new Set(Object.keys(readManifest(root).files));
+}
+
+/** Serialise a manifest with sorted keys and a trailing newline. No clock, by construction. */
+export function serializeManifest(manifest: BuildManifest): string {
+  const files: BuildManifest["files"] = {};
+  for (const path of Object.keys(manifest.files).sort(compare)) {
+    files[path] = manifest.files[path]!;
+  }
+  return `${JSON.stringify({ version: manifest.version, files }, null, 2)}\n`;
+}

+ 14 - 49
packages/compatibility-layer/src/core/BuildPipeline.ts

@@ -54,6 +54,13 @@ import {
 } from "node:fs";
 import { dirname, join, relative, resolve, sep } from "node:path";
 import { CanonicalAgentLoader, type CanonicalAgentFile } from "./AgentLoader.js";
+import {
+  MANIFEST_FILE,
+  readManifest,
+  serializeManifest,
+  type BuildManifest,
+  type ManifestEntry,
+} from "./BuildManifest.js";
 import { RegistryEmitter } from "./RegistryEmitter.js";
 import { ClaudeAdapter } from "../adapters/ClaudeAdapter.js";
 import { OpenCodeAdapter } from "../adapters/OpenCodeAdapter.js";
@@ -142,27 +149,12 @@ export interface WriteResult {
   kept: Array<{ path: string; reason: string }>;
 }
 
-/** One line of the ledger: what the build wrote at a path, and where it was allowed to. */
-export interface ManifestEntry {
-  /** sha256 of the bytes the build wrote. */
-  sha256: string;
-  /** The target that produced it. */
-  target: string;
-  /**
-   * The output root this file was written under — `TARGET_ROOTS[target]`, rebased if the
-   * target was staged. Recorded rather than recomputed so pruning can bound itself without
-   * having to be told which staging layout a PREVIOUS build happened to use.
-   */
-  root: string;
-}
-
-/** The build's ledger of what it wrote. Deterministic: sorted keys, no timestamps. */
-export interface BuildManifest {
-  /** Ledger format version, so a future shape change is detectable rather than silent. */
-  version: 1;
-  /** Repo-relative POSIX path -> what the build wrote there. */
-  files: Record<string, ManifestEntry>;
-}
+// The ledger lives in `./BuildManifest.js` because `RegistryEmitter` needs it too and this
+// module already imports `RegistryEmitter`. Re-exported here so it stays part of the build
+// pipeline's public surface — `readManifest`/`serializeManifest`/`BuildManifest` have always
+// been importable from this module and callers should not have to care that it moved.
+export { readManifest, serializeManifest } from "./BuildManifest.js";
+export type { BuildManifest, ManifestEntry } from "./BuildManifest.js";
 
 // ============================================================================
 // PATHS AND CONSTANTS
@@ -170,7 +162,6 @@ export interface BuildManifest {
 
 const DEFAULTS = {
   contentRoot: "content/agents",
-  manifestFile: ".oac/build-manifest.json",
 } as const;
 
 /**
@@ -341,32 +332,6 @@ export async function buildAgentIn(
 // MANIFEST
 // ============================================================================
 
-/** Read the previous manifest, or an empty one. An absent ledger prunes nothing — safe. */
-export function readManifest(root: string): BuildManifest {
-  const path = join(resolve(root), DEFAULTS.manifestFile);
-  if (!existsSync(path)) return { version: 1, files: {} };
-
-  try {
-    const parsed = JSON.parse(readFileSync(path, "utf-8")) as BuildManifest;
-    // A ledger we cannot vouch for prunes nothing rather than pruning wrongly.
-    if (parsed.version !== 1 || typeof parsed.files !== "object" || parsed.files === null) {
-      return { version: 1, files: {} };
-    }
-    return parsed;
-  } catch {
-    return { version: 1, files: {} };
-  }
-}
-
-/** Serialise a manifest with sorted keys and a trailing newline. No clock, by construction. */
-export function serializeManifest(manifest: BuildManifest): string {
-  const files: BuildManifest["files"] = {};
-  for (const path of Object.keys(manifest.files).sort(compare)) {
-    files[path] = manifest.files[path]!;
-  }
-  return `${JSON.stringify({ version: manifest.version, files }, null, 2)}\n`;
-}
-
 /** The manifest a plan implies, given where each target is actually written. */
 function manifestFor(files: readonly BuildFile[], outputRoots: OutputRoots): BuildManifest {
   const entries: BuildManifest["files"] = {};
@@ -519,7 +484,7 @@ export function write(plan: BuildPlan, options: WriteOptions): WriteResult {
     }
   }
 
-  const manifestPath = join(root, DEFAULTS.manifestFile);
+  const manifestPath = join(root, MANIFEST_FILE);
   mkdirSync(dirname(manifestPath), { recursive: true });
   writeFileSync(manifestPath, serializeManifest(next), "utf-8");
 

+ 71 - 4
packages/compatibility-layer/src/core/RegistryEmitter.ts

@@ -49,6 +49,30 @@
  * {@link ReferenceResolver.resolve} and pinned as a known dead ref in
  * `tests/unit/build/reference-resolution.test.ts`.
  *
+ * ─── Withdrawing an entry, and why it is manifest-gated ─────────────────────────────────
+ *
+ * Deleting a canonical source must delete its registry entry, or this emitter reproduces the
+ * exact defect it was built to cure: an entry nothing on disk agrees with, lingering forever.
+ * But entries are also CARRIED — `agent:eval-runner` ships in `.opencode/agent/eval-runner.md`
+ * and sits in the committed registry with no `content/` counterpart, and dropping it would
+ * uninstall the eval harness for everyone.
+ *
+ * By id alone the two cases are indistinguishable: both are "in the base, absent from the
+ * canonical tree". The discriminator is `.oac/build-manifest.json` — the same ledger
+ * {@link BuildPipeline} prunes generated FILES with. An entry whose `path` the ledger claims
+ * was written BY US, so its absence from the tree means its source was deleted and the entry is
+ * withdrawn. An entry whose `path` the ledger does not claim was never ours, so it is carried.
+ * `eval-runner.md` has never been in the ledger; every generated agent file always is.
+ *
+ * The ledger rather than `existsSync`, deliberately: at emit time the orphaned FILE has not
+ * been pruned yet ({@link BuildPipeline.write} plans before it reconciles), so an existence
+ * check would still see the file and carry the entry — the answer would depend on the order two
+ * phases happen to run in. The ledger is ordering-independent, and using the same mechanism as
+ * file pruning is what keeps `.opencode/**` and `registry.json` from drifting apart.
+ *
+ * No ledger prunes nothing, matching {@link BuildPipeline}'s rule: no record of having written
+ * a file is not evidence that we wrote it, so a first build carries everything.
+ *
  * ─── Determinism ────────────────────────────────────────────────────────────────────────
  *
  * The generated tree stays committed and CI gates drift with `oac build && git diff
@@ -66,6 +90,7 @@
 import { readFileSync } from "node:fs";
 import { resolve } from "node:path";
 import { CanonicalAgentLoader, type CanonicalAgentFile } from "./AgentLoader.js";
+import { generatedPaths } from "./BuildManifest.js";
 
 // ============================================================================
 // TYPES
@@ -110,6 +135,15 @@ export interface RegistryEmitterOptions {
   agentInstallRoot?: string;
   /** The committed registry to carry non-agent data through from. */
   registryFile?: string;
+  /**
+   * Repo-relative paths a previous build recorded generating — the discriminator between "this
+   * base entry was never ours, carry it" and "we wrote this and its source is gone, withdraw
+   * it". See the module header.
+   *
+   * Defaults to the paths `.oac/build-manifest.json` claims. An empty set carries everything,
+   * which is what a repo with no ledger yet gets.
+   */
+  generatedPaths?: ReadonlySet<string>;
 }
 
 const DEFAULTS = {
@@ -204,6 +238,7 @@ export class RegistryEmitter {
   private readonly contentRoot: string;
   private readonly agentInstallRoot: string;
   private readonly registryFile: string;
+  private readonly injectedGeneratedPaths?: ReadonlySet<string>;
 
   /**
    * @param root    - Repository root. Everything resolves relative to this, so the emitter is
@@ -215,6 +250,15 @@ export class RegistryEmitter {
     this.contentRoot = options.contentRoot ?? DEFAULTS.contentRoot;
     this.agentInstallRoot = options.agentInstallRoot ?? DEFAULTS.agentInstallRoot;
     this.registryFile = options.registryFile ?? DEFAULTS.registryFile;
+    this.injectedGeneratedPaths = options.generatedPaths;
+  }
+
+  /**
+   * The paths a previous build claims to have written. Read lazily rather than in the
+   * constructor so that constructing an emitter stays free of I/O, exactly like {@link base}.
+   */
+  private previouslyGenerated(): ReadonlySet<string> {
+    return this.injectedGeneratedPaths ?? generatedPaths(this.root);
   }
 
   /**
@@ -244,6 +288,7 @@ export class RegistryEmitter {
       .loadFromDirectory();
 
     const generated = this.generatedByCategory(agents);
+    const ours = this.previouslyGenerated();
     const components: Record<string, RegistryEntry[]> = {};
 
     // Iterate the BASE's keys, not the generated ones: an entry type the canonical tree does
@@ -251,14 +296,14 @@ export class RegistryEmitter {
     for (const category of Object.keys(base.components)) {
       const existing = base.components[category] ?? [];
       const owned = generated.get(category);
-      components[category] = owned === undefined ? existing : merge(owned, existing);
+      components[category] = owned === undefined ? existing : merge(owned, existing, ours);
     }
 
     // A canonical `oac.type` whose category the base has never seen. Not reachable today
     // (`AgentTypeSchema` is `agent | subagent` and both exist), but appending rather than
     // dropping means a new type surfaces in the diff instead of vanishing.
     for (const [category, owned] of generated) {
-      components[category] ??= merge(owned, []);
+      components[category] ??= merge(owned, [], ours);
     }
 
     const document: RegistryDocument = {} as RegistryDocument;
@@ -303,17 +348,39 @@ export class RegistryEmitter {
  * canonicalised is preserved verbatim, keeping its own key order and any fields this emitter
  * does not model.
  *
+ * An unclaimed entry we PREVIOUSLY GENERATED is the opposite case and must not be carried: its
+ * canonical source has been deleted, and carrying it would strand it in the registry forever —
+ * see {@link wasGeneratedByUs} and the module header.
+ *
  * The result is sorted by id: array order must be a property of the CONTENT, not of the
  * insertion history of a hand-edited file, or the diff gate cannot tell a real change from a
  * reshuffle.
  */
-function merge(generated: readonly RegistryEntry[], base: readonly RegistryEntry[]): RegistryEntry[] {
+function merge(
+  generated: readonly RegistryEntry[],
+  base: readonly RegistryEntry[],
+  previouslyGenerated: ReadonlySet<string>
+): RegistryEntry[] {
   const claimed = new Set(generated.map((entry) => entry.id));
-  const carried = base.filter((entry) => !claimed.has(entry.id));
+  const carried = base.filter(
+    (entry) => !claimed.has(entry.id) && !wasGeneratedByUs(entry, previouslyGenerated)
+  );
 
   return [...generated, ...carried].sort((a, b) => compare(a.id, b.id));
 }
 
+/**
+ * Whether a base entry names a file the build previously wrote.
+ *
+ * Matched on `path` because that is the only field tying an entry to a file on disk — and it is
+ * the field `install.sh` copies verbatim, so an entry without one installs nothing and cannot
+ * be something we emitted. A missing or non-string `path` therefore answers "not ours", which
+ * carries the entry: every ambiguity here resolves toward keeping data.
+ */
+function wasGeneratedByUs(entry: RegistryEntry, previouslyGenerated: ReadonlySet<string>): boolean {
+  return typeof entry.path === "string" && previouslyGenerated.has(entry.path);
+}
+
 /**
  * Generate `registry.json` for a repository, as the bytes to write.
  *

+ 124 - 0
packages/compatibility-layer/tests/unit/core/RegistryEmitter.test.ts

@@ -36,6 +36,7 @@ import {
   type RegistryDocument,
   type RegistryEntry,
 } from "../../../src/core/RegistryEmitter.js";
+import { generatedPaths } from "../../../src/core/BuildManifest.js";
 import { CanonicalAgentLoader } from "../../../src/core/AgentLoader.js";
 import { repoPath } from "../../support/pending.js";
 
@@ -303,6 +304,129 @@ describe("carry-through of everything the canonical tree does not own", () => {
   });
 });
 
+// ============================================================================
+// WITHDRAWAL — an entry we generated must not outlive its canonical source
+// ============================================================================
+
+/**
+ * The other half of carry-through, and the harder half.
+ *
+ * Carrying every unclaimed base entry is what keeps `eval-runner` alive, but applied blindly it
+ * recreates the defect this emitter exists to cure: delete `content/agents/x.md` and the build
+ * removes `.opencode/agent/x.md` while leaving `x` in the registry forever. The output is a
+ * stable fixed point, so `oac build && git diff --exit-code` stays green while being wrong —
+ * the drift gate cannot see it.
+ *
+ * By id alone "never had a source" and "source was deleted" are identical. The discriminator is
+ * `.oac/build-manifest.json`: an entry whose `path` the ledger claims was written by us.
+ */
+describe("withdrawal of entries the build itself generated", () => {
+  /** A path the ledger claims — i.e. one the build wrote on a previous run. */
+  const GENERATED_PATH = ".opencode/agent/subagents/utils/demo-agent.md";
+
+  /** `eval-runner`'s file: shipped, hand-authored, and never generated by us. */
+  const EVAL_RUNNER_PATH = ".opencode/agent/eval-runner.md";
+
+  /**
+   * The committed registry plus an entry for an agent the build generated and whose canonical
+   * source has since been deleted — the exact state a deletion build reads as its base.
+   */
+  function baseWithDeletedSource(): RegistryDocument {
+    const document = JSON.parse(COMMITTED_JSON) as RegistryDocument;
+    document.components.subagents = [
+      ...entries(document, "subagents"),
+      {
+        id: "demo-agent",
+        name: "Demo Agent",
+        type: "subagent",
+        path: GENERATED_PATH,
+        version: "1.0.0",
+        description: "Generated by a previous build; its canonical source is now gone.",
+        tags: ["demo"],
+        dependencies: [],
+        category: "subagents/utils",
+      },
+    ];
+    return document;
+  }
+
+  /** Emit against a scratch base and an explicit ledger, over the live canonical tree. */
+  async function emitWith(
+    base: RegistryDocument,
+    ledger: ReadonlySet<string>
+  ): Promise<RegistryDocument> {
+    const root = scratchRoot(serializeRegistry(base));
+
+    try {
+      return await new RegistryEmitter(root, {
+        contentRoot: repoPath("content/agents"),
+        generatedPaths: ledger,
+      }).emit();
+    } finally {
+      rmSync(root, { recursive: true, force: true });
+    }
+  }
+
+  it("drops an entry whose path the ledger claims once its canonical source is gone", async () => {
+    // The bug: `merge` carried this forever because `demo-agent` is simply "not in generated",
+    // which is also true of eval-runner. The ledger is what tells them apart.
+    const document = await emitWith(baseWithDeletedSource(), new Set([GENERATED_PATH]));
+
+    expect(byId(document, "subagents").has("demo-agent")).toBe(false);
+  });
+
+  it("keeps eval-runner while withdrawing a generated entry in the same merge", async () => {
+    // The regression that matters most: a fix that drops the deleted agent by also dropping
+    // every sourceless entry uninstalls the eval harness. Both must hold at once.
+    const document = await emitWith(baseWithDeletedSource(), new Set([GENERATED_PATH]));
+
+    expect(byId(document, "subagents").has("demo-agent")).toBe(false);
+    expect(byId(document, "agents").get("eval-runner")).toEqual(
+      byId(COMMITTED, "agents").get("eval-runner")
+    );
+  });
+
+  it("carries every entry when there is no ledger, so a first build deletes nothing", async () => {
+    // Matches BuildPipeline's rule: no record of having written a file is not evidence that we
+    // wrote it. A fresh clone has no manifest and must not withdraw anything.
+    const document = await emitWith(baseWithDeletedSource(), new Set());
+
+    expect(byId(document, "subagents").get("demo-agent")?.path).toBe(GENERATED_PATH);
+    expect(byId(document, "agents").has("eval-runner")).toBe(true);
+  });
+
+  it("carries an entry the ledger does not claim, however the ledger is populated", async () => {
+    // eval-runner survives against the REAL ledger, not just a hand-made one.
+    const document = await emitWith(baseWithDeletedSource(), generatedPaths(repoPath()));
+
+    expect(byId(document, "agents").get("eval-runner")).toEqual(
+      byId(COMMITTED, "agents").get("eval-runner")
+    );
+  });
+
+  it("has a ledger that claims every generated agent file and not eval-runner", async () => {
+    // The discriminator is only real if the live ledger actually separates the two populations.
+    // If a later subtask canonicalises eval-runner this turns red and forces a deliberate edit:
+    // the entry would then be generated by id and no longer need carrying at all.
+    const ledger = generatedPaths(repoPath());
+    const claimed = (id: string): boolean =>
+      ledger.has(byId(COMMITTED, "agents").get(id)?.path ?? "");
+
+    expect(ledger.has(EVAL_RUNNER_PATH), "eval-runner is in the ledger").toBe(false);
+    expect(claimed("openagent"), "a generated agent is missing from the ledger").toBe(true);
+
+    for (const category of GENERATED_CATEGORIES) {
+      const unclaimed = entries(COMMITTED, category)
+        .filter((entry) => !ledger.has(entry.path ?? ""))
+        .map((entry) => entry.id);
+
+      expect(unclaimed, `${category} entries the ledger does not claim`).toEqual(
+        category === "agents" ? ["eval-runner"] : []
+      );
+    }
+  });
+});
+
 // ============================================================================
 // GENERATION — entries come from the canonical files
 // ============================================================================