diff --git a/.changeset/graph-cache-settled-identity.md b/.changeset/graph-cache-settled-identity.md new file mode 100644 index 000000000..f921713f2 --- /dev/null +++ b/.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. diff --git a/packages/harness/src/core/definition-name.test.ts b/packages/harness/src/core/definition-name.test.ts index 9d0512409..b742c1d4a 100644 --- a/packages/harness/src/core/definition-name.test.ts +++ b/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): Promise { + 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, + }); + }); + }); }); diff --git a/packages/harness/src/core/definition-name.ts b/packages/harness/src/core/definition-name.ts index 4a9cdfd91..16ca0ff93 100644 --- a/packages/harness/src/core/definition-name.ts +++ b/packages/harness/src/core/definition-name.ts @@ -13,15 +13,47 @@ * 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 { + 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, @@ -29,11 +61,15 @@ export async function inspectManifestName( ): Promise { 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 }; } } diff --git a/packages/harness/src/core/system-graph-inventory.test.ts b/packages/harness/src/core/system-graph-inventory.test.ts index 044476adf..3d507e867 100644 --- a/packages/harness/src/core/system-graph-inventory.test.ts +++ b/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((resolve) => { diff --git a/packages/harness/src/core/system-graph-inventory.ts b/packages/harness/src/core/system-graph-inventory.ts index cbaf2041a..426275d06 100644 --- a/packages/harness/src/core/system-graph-inventory.ts +++ b/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"), diff --git a/packages/harness/src/core/workflow-registry.test.ts b/packages/harness/src/core/workflow-registry.test.ts index 8120fe102..797a0d925 100644 --- a/packages/harness/src/core/workflow-registry.test.ts +++ b/packages/harness/src/core/workflow-registry.test.ts @@ -394,6 +394,43 @@ describe("WorkflowRegistry", () => { * both halves of what replaced it: the reach, and the reconciliation rule that * keeps a *bounded* scan from mistaking "I didn't look there" for "it's gone". */ +describe("WorkflowRegistry path identity under symlinks", () => { + let tmpRoot: string; + let registryPath: string; + + beforeEach(async () => { + // Resolve up front: on macOS `os.tmpdir()` is itself a symlink, which + // would make every path in the test canonical by accident. + tmpRoot = await fs.realpath( + await fs.mkdtemp(path.join(os.tmpdir(), "harness-registry-symlink-")), + ); + registryPath = path.join(tmpRoot, "state", "workflows.json"); + }); + + afterEach(async () => { + await fs.rm(tmpRoot, { recursive: true, force: true }); + }); + + it("connects a symlinked path onto the scanned row instead of duplicating it", async () => { + const real = path.join(tmpRoot, "workspace"); + await writeMarker(path.join(real, "growth"), null, { name: "growth" }); + const link = path.join(tmpRoot, "linked-workspace"); + await fs.symlink(real, link, "dir"); + + const registry = new WorkflowRegistry(registryPath); + await registry.scan(real, new AgentProjectScanBudget()); + await registry.connectPath(path.join(link, "growth")); + + // One row, still under the spelling the scan stored: a second row for the + // same directory collides into `local:` keys and drops every edge, while + // rewriting the kept row's path would unbind a session matching on it. + expect((await registry.list()).map((workflow) => workflow.path)).toEqual([ + path.join(real, "growth"), + ]); + }); + +}); + describe("WorkflowRegistry scan rootedness (the 88-agent accumulation)", () => { let tmpRoot: string; let registry: WorkflowRegistry; diff --git a/packages/harness/src/core/workflow-registry.ts b/packages/harness/src/core/workflow-registry.ts index b460f7fbb..9b96701cc 100644 --- a/packages/harness/src/core/workflow-registry.ts +++ b/packages/harness/src/core/workflow-registry.ts @@ -29,6 +29,7 @@ import { walkAgentProjectTreeAsync, } from "./agent-project-discovery.js"; import { hasTraversalSegment, resolveWithinRoot } from "./path-safety.js"; +import { canonicalGraphPath } from "./system-graph-inventory.js"; function expandHome(inputPath: string): string { if (inputPath === "~") return os.homedir(); @@ -428,8 +429,18 @@ export class WorkflowRegistry { starterId: marker?.starterId ?? null, source: "connect", }; - const idx = this.workflows.findIndex((workflow) => workflow.path === absolutePath); - if (idx >= 0) this.workflows[idx] = info; + // Match on the resolved directory, not the spelling given: connecting a + // symlinked path to an already-scanned project otherwise registers a + // second row for one directory, and the pair collides into `local:` + // fallback keys that make every reference between agents ambiguous. + // The existing row keeps its own `path` — registry paths are compared by + // exact string elsewhere (a session auto-binds on `path === cwd`), so + // rewriting one silently unbinds whatever matched it. + const canonical = canonicalGraphPath(absolutePath); + const idx = this.workflows.findIndex( + (workflow) => canonicalGraphPath(workflow.path) === canonical, + ); + if (idx >= 0) this.workflows[idx] = { ...info, path: this.workflows[idx]!.path }; else this.workflows.push(info); await this.persist(); return info; diff --git a/packages/harness/src/server/index.ts b/packages/harness/src/server/index.ts index d21fa404e..d8ec944ed 100644 --- a/packages/harness/src/server/index.ts +++ b/packages/harness/src/server/index.ts @@ -1210,10 +1210,21 @@ export const startServer = async ( // archaeology session. Now every scan says its root, its reason, what it // found, what it cost, and what it declined to enter. const scanWorkflowsAndBroadcast = async ( - root: string, + scanRoot: string, reason: WorkflowScanReason, options: { refreshGraphs?: () => Promise } = {}, ): Promise => { + // The registry keys rows by path, so two spellings of one directory + // register the same agent twice. Only the graph-refresh caller resolved + // symlinks: booting under a symlinked launch dir (macOS `os.tmpdir()` is + // `/var/...` for `/private/var/...`) registered every agent under the + // symlinked path, then the first graph open registered them all again + // under the real one. The duplicates collided into `local:` fallback keys, + // so every cross-agent target became ambiguous and its edge vanished. + // Resolved here rather than inside the registry: registry paths are + // compared by exact string elsewhere (a session auto-binds on + // `path === cwd`, index.ts:1057), so rewriting stored paths unbinds them. + const root = canonicalGraphPath(scanRoot); const before = workflowsCache; const budget = new AgentProjectScanBudget(); const found = await workflowRegistry.scan(root, budget); diff --git a/packages/harness/src/server/system-graph-freshness.test.ts b/packages/harness/src/server/system-graph-freshness.test.ts index 9549464e9..940de47e0 100644 --- a/packages/harness/src/server/system-graph-freshness.test.ts +++ b/packages/harness/src/server/system-graph-freshness.test.ts @@ -237,4 +237,92 @@ describe("workspace graph freshness wiring", () => { expect(manualRetry.revision).toBeGreaterThan(beforeManualRetry.revision); }, ); + + it( + "registers each agent once when the launch directory is a symlink", + { timeout: 30_000 }, + async () => { + // The registry keys rows by path. Boot scanned the launch directory as + // given while the first graph open scanned its resolved form, so every + // agent registered twice; the duplicates collided into `local:` fallback + // keys and each cross-agent target became ambiguous, dropping its edge. + // macOS hits this on any `os.tmpdir()` path (`/var` -> `/private/var`). + await scaffoldAgent( + workspaceRoot, + "research", + 'ctx.sapiom.agents.run({ definition: "growth" });\n', + ); + await scaffoldAgent(workspaceRoot, "growth"); + const linkedRoot = path.join(tempRoot, "linked-workspace"); + await fs.symlink(workspaceRoot, linkedRoot, "dir"); + await fs.writeFile( + path.join(stateRoot, "settings.json"), + JSON.stringify({ recentDirs: [linkedRoot] }), + ); + + server = await startServer({ + port: 0, + bootToken: "test-token", + telemetryOptIn: false, + adapters: {}, + stateRoot, + launchDir: linkedRoot, + autoCreateSession: false, + }); + const baseUrl = `http://127.0.0.1:${server.port}`; + const headers = { "X-Harness-Token": "test-token" }; + + await vi.waitFor( + async () => { + const response = await fetch(`${baseUrl}/api/workflows`, { headers }); + const workflows = (await response.json()) as WorkflowInfo[]; + expect(workflows).toHaveLength(2); + }, + { timeout: 8_000, interval: 150 }, + ); + + const state = (await ( + await fetch(`${baseUrl}/api/state`, { headers }) + ).json()) as AppState; + const workspaceKey = state.workspaceScopes?.[0]?.workspaceKey; + expect(workspaceKey).toBeTruthy(); + + const readGraph = async (): Promise => + (await ( + await fetch(`${baseUrl}/api/workspaces/${workspaceKey}/system-graph`, { + headers, + }) + ).json()) as SystemGraphSnapshot; + + await vi.waitFor( + async () => { + expect((await readGraph()).state).toBe("ready"); + }, + { timeout: 8_000, interval: 150 }, + ); + + // The duplicate rows only appear once a SECOND scan runs under the + // resolved spelling, which is what a graph refresh does. Adding an agent + // is the cheapest way to make the watcher trigger one. + await scaffoldAgent(workspaceRoot, "reporting"); + await vi.waitFor( + async () => { + const graph = await readGraph(); + expect(graph.graph?.nodes.map((node) => node.agentKey)).toEqual([ + "growth", + "reporting", + "research", + ]); + expect(graph.graph?.warnings).toEqual([]); + expect(graph.graph?.edges).toEqual([ + expect.objectContaining({ + from: "agent:research", + to: "agent:growth", + }), + ]); + }, + { timeout: 10_000, interval: 150 }, + ); + }, + ); });