fix(harness): stop a settled identity failure from vetoing the graph cache [SAP-2984]

.changeset/graph-cache-settled-identity.md

@@ -0,0 +1,32 @@
+---
+"@sapiom/harness": patch
+---
+
+Stop an agent that can never be named from holding a whole project's graph at
+"Graph may be incomplete".
+
+An agent that does not declare a name has its source read to recover one. If
+even one agent in a project could not be read, the projection refused to cache,
+so the project reopened as degraded under the "Graph may be incomplete" banner
+every time. A companion package with no agent export in it has nothing to find
+no matter how often it is re-read, and a single such directory was enough to
+mark everything around it incomplete.
+
+The cache now depends on whether the name lookup can still produce a different
+answer, not on whether it found a name. Only one case is treated as final, the
+one that can be proven: a project with no TypeScript in it has nothing for an
+agent name to be declared in, and no install or re-run will invent one. Every
+other failure still blocks caching, so that project keeps its banner and its
+Retry button — the thing that clears it once dependencies are installed, or
+after a check that ran out of time succeeds on a second attempt.
+
+The affected agents keep their warnings either way. The graph stops calling
+itself incomplete over agents it was never going to resolve; it does not go
+quiet about what it could not resolve.
+
+Also stops one agent being registered twice when Studio reaches the same
+directory by two different paths — once as given and once with symlinks
+resolved. The duplicate pair made every reference between agents ambiguous,
+silently dropping those connections from the graph. This prevents new
+duplicates; a workspace that already contains a pair from an earlier version
+still needs them removed by hand.

packages/harness/src/core/definition-name.test.ts

@@ -1,4 +1,7 @@
-import { describe, expect, it, vi } from "vitest";
+import * as fs from "node:fs/promises";
+import * as os from "node:os";
+import * as path from "node:path";
+import { afterEach, describe, expect, it, vi } from "vitest";
 
 import { inspectManifestName, resolveManifestName } from "./definition-name.js";
 
@@ -72,14 +75,76 @@ describe("inspectManifestName", () => {
     ).resolves.toEqual({ status: "absent" });
     await expect(inspectManifestName("/proj/broken", failed)).resolves.toEqual({
       status: "failed",
+      retryable: false,
     });
   });
 
   it("reports a thrown extraction as failed", async () => {
     const extract = vi.fn().mockRejectedValue(new Error("boom"));
 
+    // The extractor told us nothing about why, so keep the retry affordance.
     await expect(inspectManifestName("/proj/broken", extract)).resolves.toEqual(
-      { status: "failed" },
+      { status: "failed", retryable: true },
     );
   });
+
+  describe("classifying a failure as still clearable", () => {
+    const roots: string[] = [];
+    const failed = () =>
+      vi.fn().mockResolvedValue({
+        result: { ok: false as const, reason: "run npm install first" },
+        cached: false,
+        fingerprint: "0:0",
+      });
+
+    async function project(files: Record<string, string>): Promise<string> {
+      const root = await fs.mkdtemp(path.join(os.tmpdir(), "definition-name-"));
+      roots.push(root);
+      for (const [relative, contents] of Object.entries(files)) {
+        const file = path.join(root, relative);
+        await fs.mkdir(path.dirname(file), { recursive: true });
+        await fs.writeFile(file, contents);
+      }
+      return root;
+    }
+
+    afterEach(async () => {
+      await Promise.all(
+        roots.splice(0).map((root) => fs.rm(root, { recursive: true, force: true })),
+      );
+    });
+
+    it("is retryable while the project still has TypeScript to name", async () => {
+      const root = await project({ "index.ts": "export {};\n" });
+      await expect(inspectManifestName(root, failed())).resolves.toEqual({
+        status: "failed",
+        retryable: true,
+      });
+    });
+
+    it("stays retryable with dependencies installed", async () => {
+      // Installed deps prove nothing: workspaces hoist `node_modules` to the
+      // repo root, and a check process that crashed or timed out under load
+      // succeeds on the next run. Only "no TypeScript at all" is provable.
+      const root = await project({
+        "index.ts": "export {};\n",
+        "node_modules/.keep": "",
+      });
+      await expect(inspectManifestName(root, failed())).resolves.toEqual({
+        status: "failed",
+        retryable: true,
+      });
+    });
+
+    it("is settled when the project has no TypeScript to name", async () => {
+      // A dashboard companion: no install and no re-run can produce a
+      // `defineAgent` that was never written, so this failure is the one we
+      // can prove has nowhere left to go.
+      const root = await project({ "server.js": "module.exports = {};\n" });
+      await expect(inspectManifestName(root, failed())).resolves.toEqual({
+        status: "failed",
+        retryable: false,
+      });
+    });
+  });
 });

packages/harness/src/core/definition-name.ts

@@ -13,27 +13,63 @@
  * caller falls back to a weaker name source.
  */
 import { extractWorkflowGraphCached } from "./canvas-cache.js";
+import { listSourceFiles } from "./canvas-interconnections.js";
 
 export type ManifestNameInspection =
   | { status: "found"; name: string }
-  | { status: "absent" | "failed" };
+  | { status: "absent" }
+  /** `retryable`: the same unchanged source could still name this agent. */
+  | { status: "failed"; retryable: boolean };
+
+/**
+ * Whether a later projection of the SAME unchanged source could still name
+ * this agent. Extraction failures are deliberately not cached
+ * (core/canvas-cache.ts), so a re-run is free to succeed — and several causes
+ * do resolve on their own terms: dependencies get installed, a check process
+ * that crashed or timed out under load succeeds on the next attempt. None of
+ * those fire the graph watcher, which only reacts to `.ts`/`.tsx` outside
+ * ignored directories, so the caller must keep offering a manual retry.
+ *
+ * `reason` cannot answer this — it is free-form text assembled from an agent's
+ * own error message, a stderr tail, or a timeout string — so the only claim
+ * made here is the one the filesystem proves outright: a project with no
+ * TypeScript in it has no `defineAgent` to find, and no install or re-run
+ * will invent one. Adding a source file changes the answer, and adding one is
+ * precisely what the watcher does see.
+ *
+ * Deliberately one-directional. Guessing "settled" wrongly caches a
+ * provisional label and removes the retry that would have fixed it; guessing
+ * "retryable" wrongly just leaves the project as it behaves today.
+ */
+async function couldStillBeNamed(projectDir: string): Promise<boolean> {
+  try {
+    return (await listSourceFiles(projectDir)).length > 0;
+  } catch {
+    return true;
+  }
+}
 
 /**
  * Inspect the declared manifest name while preserving the difference between
  * a valid unnamed agent and an extraction failure. Inventory uses the richer
- * result to avoid warning for the normal unnamed case.
+ * result to avoid warning for the normal unnamed case, and `retryable` to
+ * decide whether the failure can still be cleared without a source edit.
  */
 export async function inspectManifestName(
   projectDir: string,
   extract: typeof extractWorkflowGraphCached = extractWorkflowGraphCached,
 ): Promise<ManifestNameInspection> {
   try {
     const { result } = await extract(projectDir);
-    if (!result.ok) return { status: "failed" };
+    if (!result.ok) {
+      return { status: "failed", retryable: await couldStillBeNamed(projectDir) };
+    }
     const name = result.graph.manifestName.trim();
     return name === "" ? { status: "absent" } : { status: "found", name };
   } catch {
-    return { status: "failed" };
+    // The extractor itself misbehaved; we learned nothing about why. Assume
+    // recoverable so the caller keeps its retry affordance.
+    return { status: "failed", retryable: true };
   }
 }
 

packages/harness/src/core/system-graph-inventory.test.ts

@@ -168,7 +168,7 @@ describe("HarnessRegistryInventoryProvider", () => {
       started.push(sourceRoot);
       await gate;
       if (sourceRoot.endsWith("/broken")) {
-        return { status: "failed" as const };
+        return { status: "failed" as const, retryable: false };
       }
       if (sourceRoot.endsWith("/thrown")) {
         throw new Error(`unreadable ${sourceRoot}`);
@@ -220,6 +220,82 @@ describe("HarnessRegistryInventoryProvider", () => {
     expect(JSON.stringify(result.warnings)).not.toContain(WORKSPACE);
   });
 
+  it("caches a project whose extraction failures have all settled", async () => {
+    // A companion package with no `defineAgent` export, or one whose
+    // dependencies were never installed, fails identically on every open.
+    // Letting it veto the cache pinned real workspaces to `degraded` and its
+    // "graph may be incomplete" banner permanently.
+    const inspectManifestName = vi.fn(async (sourceRoot: string) =>
+      sourceRoot.endsWith("/dashboard")
+        ? ({ status: "failed", retryable: false } as const)
+        : ({ status: "found", name: "growth-manifest" } as const),
+    );
+
+    const result = await provider(
+      [workflow("Growth", "growth", null), workflow("Dashboard", "dashboard", null)],
+      { inspectManifestName },
+    ).listAgents(SCOPE);
+
+    expect(result.cacheable).toBe(true);
+    // The graph becomes cacheable; it does not go quiet about what it could
+    // not resolve.
+    expect(result.warnings).toEqual([
+      {
+        code: "inventory-extraction-failed",
+        agentKey: "local:dashboard",
+        message: "Could not inspect Dashboard; using its local identity.",
+      },
+    ]);
+  });
+
+  it("keeps a project uncacheable when a failure is still clearable", async () => {
+    // "Run install first" is the one failure a later projection of the SAME
+    // unchanged source can clear, and nothing fires the watcher when
+    // `node_modules` lands. Caching it would strip the degraded banner that
+    // carries the only Retry button, freezing a `local:` label on screen.
+    const inspectManifestName = vi.fn(async (sourceRoot: string) =>
+      sourceRoot.endsWith("/needs-install")
+        ? ({ status: "failed", retryable: true } as const)
+        : ({ status: "found", name: "growth-manifest" } as const),
+    );
+
+    const result = await provider(
+      [
+        workflow("Growth", "growth", null),
+        workflow("Needs install", "needs-install", null),
+      ],
+      { inspectManifestName },
+    ).listAgents(SCOPE);
+
+    expect(result.cacheable).toBe(false);
+    expect(
+      result.warnings.map(({ code, agentKey }) => [code, agentKey]),
+    ).toEqual([["inventory-extraction-failed", "local:needs-install"]]);
+  });
+
+  it("keeps a project uncacheable while an inspection is still in flight", async () => {
+    // The budget cut this inspection off mid-read. Its name is still
+    // recoverable, so caching the provisional local label would freeze the
+    // wrong text on screen until the next source edit.
+    const started: string[] = [];
+    const inspectManifestName = vi.fn(async (sourceRoot: string) => {
+      started.push(sourceRoot);
+      if (sourceRoot.endsWith("/settled")) {
+        return { status: "failed" as const, retryable: false };
+      }
+      await new Promise(() => {});
+      return { status: "absent" as const };
+    });
+
+    const result = await provider(
+      [workflow("Settled", "settled", null), workflow("Slow", "slow", null)],
+      { inspectManifestName, manifestInspectionBudgetMs: 20 },
+    ).listAgents(SCOPE);
+
+    expect(started).toEqual([`${WORKSPACE}/settled`, `${WORKSPACE}/slow`]);
+    expect(result.cacheable).toBe(false);
+  });
+
   it("returns partial inventory when the enrichment budget expires", async () => {
     let release!: () => void;
     const gate = new Promise<void>((resolve) => {

packages/harness/src/core/system-graph-inventory.ts

@@ -39,7 +39,14 @@ export interface AgentInventoryWarning {
 
 export interface AgentInventoryResult {
   agents: AgentInventoryItem[];
-  /** False when a later graph open should retry degraded enrichment. */
+  /**
+   * False while identity work could still produce a different answer — it is
+   * in flight, or it failed in a way a later projection of the same source
+   * can clear. An agent a re-projection cannot improve — no `defineAgent` to
+   * find — keeps its warning but does not veto the project's cache. Vetoing
+   * on any failure pinned whole workspaces to `degraded` and its "graph may
+   * be incomplete" banner permanently, over agents that were never nameable.
+   */
   cacheable: boolean;
   warnings: AgentInventoryWarning[];
 }
@@ -250,7 +257,10 @@ interface PreparedAgent {
   fallbackKey: AgentKey;
   definitionId: number | null;
   definitionSlug: string | null;
+  /** Identity work finished without a name; the agent keeps its warning. */
   extractionFailed: boolean;
+  /** Identity work never finished. Only this may veto the project's cache. */
+  identityPending: boolean;
   label: string;
   resolutionAliases: string[];
   sourceRoot: string;
@@ -397,7 +407,7 @@ export class HarnessRegistryInventoryProvider implements AgentInventoryProvider
     warnings.sort(warningOrder);
     return {
       agents,
-      cacheable: prepared.every((agent) => !agent.extractionFailed),
+      cacheable: prepared.every((agent) => !agent.identityPending),
       warnings,
     };
   }
@@ -410,16 +420,23 @@ export class HarnessRegistryInventoryProvider implements AgentInventoryProvider
     const definitionSlug = normalizedAlias(workflow.definitionSlug);
     let manifestName: string | null = null;
     let extractionFailed = false;
+    let identityPending = false;
     if (!definitionSlug && this.options.inspectManifestName) {
       try {
         const inspected = await this.options.inspectManifestName(sourceRoot);
         if (inspected.status === "found") {
           manifestName = normalizedAlias(inspected.name);
-        } else {
-          extractionFailed = inspected.status === "failed";
+        } else if (inspected.status === "failed") {
+          extractionFailed = true;
+          // Only a failure the inspector says is still clearable keeps the
+          // project uncacheable. A settled one has nowhere left to go: its
+          // warning stands, and the cache stops paying for it forever.
+          identityPending = inspected.retryable;
         }
       } catch {
+        // The inspector threw, so we learned nothing about which it was.
         extractionFailed = true;
+        identityPending = true;
       }
     }
 
@@ -438,6 +455,7 @@ export class HarnessRegistryInventoryProvider implements AgentInventoryProvider
       definitionId: workflow.definitionId,
       definitionSlug,
       extractionFailed,
+      identityPending,
       label: safeLabel(
         workflow.name,
         definitionSlug ??
@@ -461,7 +479,11 @@ export class HarnessRegistryInventoryProvider implements AgentInventoryProvider
       fallbackKey,
       definitionId: workflow.definitionId,
       definitionSlug,
+      // This agent's inspection never finished (deadline miss, or it never
+      // started). Re-projecting can still name it, so it must veto the cache:
+      // caching now would freeze a provisional local label on screen.
       extractionFailed: !definitionSlug,
+      identityPending: !definitionSlug,
       label: safeLabel(
         workflow.name,
         definitionSlug ?? (fallbackKey.slice("local:".length) || "Local agent"),