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

[gwitwer]

Primary change type

• Bug fix
• Documentation
• Feature
• Tests
• Dependency update
• Maintenance or refactor

Problem and motivation

An agent with no definitionSlug has its source parsed to recover a name. The
project's cache gate was prepared.every((agent) => !agent.extractionFailed),
so one agent whose source could not be read vetoed the entire project's
cache. SystemGraphStore.finishBuild then committed the projection as
degraded instead of ready — the "Graph may be incomplete" banner and the
degraded value of X-Sapiom-System-Graph-Cache.

Some of those agents have nothing to find: a dashboard companion with no
defineAgent written anywhere in it. Re-projecting cannot improve them, so the
banner was permanent.

Measured on a real 76-agent / 9-project workspace, not inferred. 21 of 76
registry rows have definitionSlug: null; 16 are named by manifest inspection
and 5 fail. Those 5 held all 76 at degraded, and an explicit "Retry"
(POST …/system-graph/refresh) came back degraded too.

One claim I could not reproduce and am therefore not making: that main
"re-projects on every open and never caches". It does not — the revision stayed
at 1 across repeated opens. What is permanent is the degraded label and the
banner, not repeated projection work.

Summary and scope

Cache on whether identity work can still produce a different answer, not on
whether it succeeded. PreparedAgent gains identityPending alongside
extractionFailed, and cacheable gates on the new field. Three cases:

Case	identityPending	Why
Inspection returned, nothing nameable in the project	false	Nowhere left to go. Stops vetoing.
Inspection returned failed for any other reason	true	An install, or a re-run of a check that timed out, could still name it — and nothing re-projects on its own.
Budget cut the inspection off mid-read (mapWithDeadline → prepareFallbackAgent)	true	Caching now freezes a provisional local: label on screen.

Rows 2 and 3 exist because two review rounds found the first version wrong,
and both were right. Round 1: I had treated every inspection that returned as
settled, but inspectManifestName also returns failed for "no node_modules
yet", which a later projection of the same unchanged source clears — extraction
failures are deliberately not cached for exactly that reason
(core/canvas-cache.ts). Nothing re-projects on its own: the graph watcher only
reacts to .ts/.tsx outside ignored directories, so node_modules appearing
fires nothing, and the degraded banner carries the only Retry button. That
version froze a local: label with no way back.

Round 2 then found my fix for it — probing for a local node_modules — wrong in
both directions: extractWorkflowGraph also returns { ok: false, reason }
for a check process that crashed or timed out, so installed-deps-plus-timeout
read as settled (round 1's bug at a different cause); and npm/yarn workspaces
hoist node_modules to the repo root, so a package directory has none even when
installed, making every real failure there read retryable and pinning the project
to degraded forever — the exact state this PR exists to remove.

reason cannot decide it either: it is free-form text assembled from an agent's
own error message, a stderr tail, or a timeout string. So the inference is gone,
and only the claim the filesystem proves outright remains — 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 that is exactly what
the watcher already sees. The classification is deliberately one-directional:
guessing "settled" wrongly costs a user the retry affordance, guessing
"retryable" wrongly costs nothing beyond today's behaviour. retryable is
required, not optional, so every construction site states its intent instead
of defaulting into the unsafe half.

extractionFailed is untouched and still drives the warnings. Affected agents
keep every inventory-extraction-failed warning: the graph becomes cacheable,
it does not go quiet about what it could not resolve.

The symlink defect — a distinct bug, see "Related work".
scanWorkflowsAndBroadcast resolves its scan root, and connectPath now
matches an existing row by resolved directory. The registry keys rows by path
and only the graph-refresh caller resolved symlinks, so booting under a
symlinked launch directory registered every agent under the symlinked path and
the first graph refresh registered them all again under the resolved one. The
duplicates collided into local: fallback keys, every cross-agent target became
ambiguous, and its edge silently disappeared.

What I did NOT do, and why — this contradicts a review suggestion. Round 1
asked me to move resolution inside WorkflowRegistry.scan/connectPath and
rewrite stored paths, covering every caller at once. I implemented that, and it
regresses session auto-bind: server/index.ts:1057 matches
workflow.path === session.cwd by exact string, session.cwd is never
resolved, and index.ts:1018 calls workflowRegistry.scan directly. Six
auto-bind specs plus two others failed (expected null to be '/var/folders/…').
Making that correct means resolving path identity across session binding too,
which is a much larger change than this PR should carry. So the scan root is
still resolved at the server, and connectPath matches canonically while
keeping the existing row's own spelling — that closes the "+ Connect
duplicates a scanned row" hole without touching path identity. Healing registries
that already contain both spellings is likewise deferred: any rule that picks a
winner changes a stored path, which is the same regression. The changeset now
says so outright rather than reading as an unconditional fix. Worth a follow-up
issue; not worth smuggling into this one.

Out of scope. No new contract beyond the retryable field, and no changes
to packages/agent. The broader package-inventory migration remains open in
#741 and is not merged or borrowed from here. relationshipsComplete still
participates in cacheable (system-graph.ts:413) with its old all-or-nothing
semantics; that is a separate judgement and I left it alone.

Related work

Related issue or discussion: SAP-2984. The branch name says sap-2983 for
historical reasons — Linear assigned SAP-2983 to a different issue after the
branch was pushed. The broader package-inventory migration that also fixes this
incidentally is #741 (not merged, not depended on here).

A correction to the premise this task was written from, stated plainly.
src/server/system-graph-freshness.test.ts fails deterministically on macOS on
clean main (18.2s, expected false to be true at line 190) and is green on
Linux CI. I was told it was catching this cache bug and should flip green with
the fix. It is not, and it does not. Evidence:

1. I instrumented the test and dumped every snapshot it reads. The lifecycle is
   ready/stale throughout — never degraded. Every agent in that fixture
   has a definitionSlug from its sapiom.json, so inspectManifestName is
   never called and extractionFailed is never set. The every(...) gate is not
   exercised by that test at all.
2. Mutation test. With cacheable: true hard-coded — the maximal version of
   this fix — the test still failed identically at line 190, 18.2s.
3. The real cause is in the dump: after the rename step the nodes are
   ["local:growth", "local:growth~2", "local:research", "local:research~2", …]
   with two duplicate-agent-key warnings. os.tmpdir() is /var/folders/…
   for /private/var/folders/…, the boot scan used the symlinked spelling and
   the graph refresh the resolved one, and the duplicate pair made
   agent:insights → agent:growth ambiguous. Re-running the test with a
   realpath'd tempRoot on unmodified main made it pass in 2.0s.

So it is not a flake, and it was worth chasing — but it was catching a different
bug. Rather than report the discrepancy and leave a deterministically red test
on macOS with a one-line cause identified, I fixed that too, as separate commits.
It is what turns the test green (18.2s red → 2.0s green).

Validation

pnpm --filter @sapiom/harness test          — 163 files / 2618 tests passed; +3 files / 10 perf passed
pnpm --filter @sapiom/harness typecheck     — clean (tsc --noEmit x2)
pnpm --filter @sapiom/harness lint          — 0 errors, 1 pre-existing warning in rest.test.ts (untouched)
pnpm terminology:check                      — passed (455 files)
pnpm examples:check                         — passed (12 templates)
E2E_PORT=5464 pnpm --filter @sapiom/harness test:ui  — 439/441; both failures re-run green,
                                                 session-tabs.spec.ts:268 passes in isolation (known flake band)

system-graph-freshness.test.ts on macOS     — RED 18.2s before / GREEN 2.0s after

Mutation-tested every guard. A count-only assertion passes when nothing
happened, so each new spec was proven to fail against the restored bug:

Mutation	Result
identityPending → extractionFailed in the cacheable gate	"caches a project whose extraction failures have all settled" FAILS
identityPending: !definitionSlug → false in prepareFallbackAgent	new in-flight spec and the pre-existing budget-expiry spec FAIL
identityPending = inspected.retryable → false	"keeps a project uncacheable when a failure is still clearable" FAILS
couldStillBeNamed forced to always-settled	both "is retryable while the project still has TypeScript" and "stays retryable with dependencies installed" FAIL
couldStillBeNamed forced to always-retryable	"is settled when the project has no TypeScript to name" FAILS
connectPath matches raw path instead of resolved	"connects a symlinked path onto the scanned row" FAILS
canonicalGraphPath(scanRoot) → scanRoot	symlink spec FAILS with expected [ 'local:growth', …(4) ]; freshness test back to 18.2s red

Reporting what did not work. My first version of the symlink spec survived
its own mutation — it passed in 181ms with the bug restored, because it
asserted only on the boot scan and the duplicate rows do not exist until a
second scan runs under the resolved spelling. Adding an agent after boot (which
makes the watcher trigger that second scan) is what gave it teeth. The table
above is the fixed version.

Real server

A 76-agent / 9-project workspace, on a dedicated port and --state-root. Of the
5 agents that fail inspection, 4 are settled and 1 is retryable — 4 of them
contain zero TypeScript files — measured directly:

4 × {"status":"failed","retryable":false}      1 × {"status":"failed","retryable":true}

So on that workspace the vetoing set drops 5 → 1, and it correctly stays
degraded — one project there really does need an install, and the banner is
telling the truth. My round-1 body claimed this workspace flipped to ready;
under the corrected rule it does not, and the earlier screenshot has been
replaced rather than left standing.

Scratch workspaces isolate each case (no real agent source was modified):

Workspace	main	this PR
companion with no TypeScript in it	degraded / degraded	ready / complete
agent with TypeScript that fails to extract	degraded / degraded	degraded / degraded (correct — keeps its Retry)

Warnings are identical in both rows; only the lifecycle differs.

What I did not prove end to end. I verified that the retryable project keeps
its banner and Retry, and that dropping an empty node_modules into it does
not flip it to ready (correctly — the extraction still fails). I did not
stand up a genuinely installable agent to watch Retry recover it to ready;
that path rests on canvas-cache.ts's documented contract that failures are
never cached, plus its own tests, not on a run of mine.

Edits mid-run keep re-projecting — the real risk of making this cacheable is a
frozen graph, so I drove it:

break dispatcher's target   → rev 5, state=ready, edges=[],  +unresolved-target:dispatcher
add a 2nd unnameable agent  → rev 7, state=ready, +inventory-extraction-failed:local:second-dashboard
restore dispatcher's target → rev 9, state=ready, edges=[agent:dispatcher>agent:reporter]

Every edit bumps the revision and the graph never sticks at a cached snapshot.

Before — a settled failure holds the project at "Graph may be incomplete"

After — banner gone, the warning still reported

After — a project whose dependencies are missing keeps its banner and Retry

Tests and documentation

Three new specs in system-graph-inventory.test.ts (settled failures cache and
keep their warnings; a clearable failure still vetoes; an in-flight inspection
still vetoes), three in definition-name.test.ts covering the retryable
classification against a real filesystem (including that installed dependencies
do not make a failure settled), one in workflow-registry.test.ts
(connecting a symlinked path lands on the scanned row), and one in
system-graph-freshness.test.ts (a symlinked launch directory registers each
agent once). The symlink spec creates the link explicitly rather than relying on
os.tmpdir(), so it reproduces on Linux CI too, where the macOS-only failure
never appears. Both pre-existing cacheable: false specs still pass unchanged.
The cacheable contract's doc comment and the new installCouldStillName helper
state the rule and the failure each prevents. No user-facing docs beyond the
changeset.

Compatibility and release impact

• Breaking or externally visible changes: None to any API. ManifestNameInspection
  is internal to this package; its failed variant now carries a required
  retryable. Behaviour change: a project whose enrichment failures all have
  nowhere left to go now reports ready / X-Sapiom-System-Graph-Cache: complete instead of degraded, and no longer shows the "Graph may be
  incomplete" banner. A project with any TypeScript that still fails to extract
  is unchanged, banner and Retry included. Warnings are unchanged in both cases.
• Changeset: Added — .changeset/graph-cache-settled-identity.md (patch,
  @sapiom/harness). Rewritten twice under review: the original claimed a
  package "whose dependencies were never installed can never be named" (false),
  and it now also states that the symlink fix prevents new duplicates rather
  than healing a workspace that already contains a pair.

Security

• I have not included secrets, credentials, private data, or unsanitized logs.
• This pull request does not publicly disclose a suspected vulnerability. I
  will follow the
  Security Policy for
  private reporting.

AI assistance

• I did not use AI assistance for this change.
• I used AI assistance and have described it below.

Claude Code wrote the diff, the specs, and this description. Verification was
not taken on trust: the red freshness test was reproduced and instrumented
before any code changed, the stated cause was disproved by hard-coding
cacheable: true and watching it stay red, every guard was mutation-tested (one
spec survived its own mutation and was rewritten — noted above), and the
before/after numbers come from driving real servers on a dedicated port and
state root rather than from the mock fixtures CI uses. Two review rounds each
found a real correctness bug — in the original rule, then in my first fix for it
— and both are described above rather than quietly corrected; so is the one
suggestion I did not take, with the failing test names as evidence.

Checklist

• I read CONTRIBUTING.md, and this contribution follows the direct-PR or issue-first policy.
• This pull request addresses one focused problem and contains no unrelated cleanup.
• I added or updated tests, or explained above why tests are not applicable.
• I ran the relevant build, typecheck, lint, and test commands, or explained
  any N/A checks above.
• I updated documentation for user-facing changes, or marked it N/A above.
• I added a Changeset for a published-package change, or explained why it is not applicable.
• I can explain and maintain every submitted change, including any AI-assisted work.

[github-actions]

Review — PR #744 (round 1)

🔒 CONFIDENTIALITY — PR body publishes an internal workspace inventory

The changeset and code comments are clean (generic roles, no customer or product
names) — the PR description is not. It names a maintainer's home directory
(/Users/…/sapiom/agents) and lists internal project/agent names from a private
tree: devtools/dependency-upgrade, murderbox/arena-app,
team-bots/backlog-action-handler, team-bots/mushroom-dashboard,
support/support-desk-dashboard. This repo is public. Internal codenames and
local paths belong in the Linear issue, not in a public PR body; the measurement
carries fine as "21 of 76 registry rows have definitionSlug: null; 5 fail
permanently". Unlike a changeset this surface is editable — please edit it.

Correctness — status: "failed" is not always permanent, and the fix removes the only recovery affordance

inspectManifestName returns failed for a transient condition too:
definition-name.ts:12 names "no node_modules yet" as a failure case, and
canvas-cache.ts:68 deliberately does not cache failures, so a later
projection of the same unchanged source would succeed once deps land. The PR
treats every returning inspection as settled, so that project now commits as
ready, and:

• The graph watcher only reacts to .ts/.tsx outside ignored dirs
  (system-graph-watcher.ts:16,19) — node_modules appearing does not
  re-project.
• installWatcher re-renders the canvas only (server/index.ts:1348); it does
  not touch the graph store.
• The Retry button renders only in the degraded branch
  (WorkspaceGraphView.tsx:200-219). Removing the banner removes the retry.

Failure scenario: user scaffolds/clones an agent, opens the graph before
pnpm install finishes → node shows its local: fallback label → install
completes → no watcher fires, no banner, no Retry, and the label stays wrong
until someone edits a .ts file in that project. On main the same user got a
banner with a working Retry.

This also makes the published prose inaccurate. The changeset says a package
"whose dependencies were never installed, can never be named no matter how often
it is re-read", and system-graph-inventory.ts:42-49 repeats it in the exported
AgentInventoryResult JSDoc. That is false for exactly the recoverable case,
and the changeset ships into CHANGELOG.md unretractably. Either narrow the
settled set (e.g. treat a deps-missing extraction as pending, or re-check on
node_modules landing), or keep the claim to what is true — "a lookup that
returned does not block the cache" — and drop the "can never be named" framing.

Correctness — canonicalization at one caller leaves the same bug reachable, and does not heal affected installs

canonicalGraphPath is applied in scanWorkflowsAndBroadcast
(server/index.ts:1224), but the registry is what keys rows by path, and it
still stores whatever it is given (workflow-registry.ts:376,419 use
path.resolve, never realpath). Two consequences:

1. "+ Connect" still reproduces it. connectPath on a symlinked directory
   already covered by a scan of its resolved form registers a second row for the
   same project → duplicate local: fallback keys → every cross-agent target
   ambiguous → edges silently dropped. Same defect, different entry point.
2. No cleanup for installs already broken. The registry persists across
   restarts. A user running under a symlinked launch dir today has rows under
   both spellings. After this upgrade the scan runs under the resolved root,
   so the symlinked rows are neither replaced (isCoveredByScan is false —
   workflow-registry.ts:181-185) nor pruned (partitionByPathExists stats the
   symlink, which exists — :245). The duplicates, ambiguity and missing edges
   survive the fix. The changeset promises "Also fixes duplicate agents when
   Studio is launched through a symlinked directory" — for those users it does
   not.

Resolving inside WorkflowRegistry.scan/connectPath (and dropping rows whose
realpath collides with a kept row on load) covers both and is one place instead
of N callers.

Verdict

Request changes. The confidentiality edit to the PR body is required before
merge; the transient-failed case needs either a narrower settled rule or a
recovery path, and the published claim corrected to match; the symlink fix
should move to the registry so connectPath and existing duplicated state are
covered.

[github-actions]

Review — PR #744 (round 2, delta since 6f39a199)

Round 1's confidentiality finding is resolved — the PR body no longer names internal
projects or a home directory. The transient-deps case and the connectPath duplicate hole
are addressed.

Correctness — retryability is inferred from the filesystem, not from why extraction failed

installCouldStillName (definition-name.ts:38) answers "can this change?" with "sources
exist && no local node_modules", ignoring the reason the extractor returned.
extractWorkflowGraph never throws — a check-process crash or timeout comes back as
{ ok: false, reason } (canvas-graph.ts:238-247), not as the thrown case the new
retryable: true catch covers. So:

• False settled. Deps installed, child check times out under load (plausible on the
  76-agent workspace motivating this PR) → retryable: false → project commits ready,
  the agent keeps its provisional local: label, and Retry — rendered only in the
  degraded branch — is gone, though a re-run would have succeeded. Round 1's failure
  mode narrowed to a different transient cause, not closed.
• False retryable. npm/yarn workspaces hoist node_modules to the root, so a package
  dir has none even with deps installed; any real failure there (bundle/type error) reads
  retryable → permanent degraded, the state this PR exists to remove.

reason is already available (the new fixture uses "run npm install first") — classify
on an explicit failure kind rather than probing the filesystem. No spec covers "installed,
but the check crashed".

Not fixed from round 1 — registries already holding both spellings

scan still stores paths verbatim; a symlinked row is neither covered
(workflow-registry.ts:387) nor pruned (:245 stats the symlink, which exists). Already-
duplicated users keep their ambiguity and dropped edges after upgrading, while the
changeset reads as an unconditional fix. One clause naming the limit keeps the published
text true.

Round 1 got this wrong

Resolving inside WorkflowRegistry.scan is unsound: auto-bind compares w.path === session.cwd by exact string (server/index.ts:1057) against an unresolved cwd. The
author's evidence stands.

Verdict: one correctness finding remains — the retryable heuristic still drops the
retry affordance for a transient failure, and inverts on hoisted workspaces.