Skip to content

fix(harness): reach the graph fast path on a real install — take over #731 [SAP-2978] - #741

Closed
gwitwer wants to merge 14 commits into
mainfrom
georgewitwer/sap-2978-package-inventory-takeover
Closed

fix(harness): reach the graph fast path on a real install — take over #731 [SAP-2978]#741
gwitwer wants to merge 14 commits into
mainfrom
georgewitwer/sap-2978-package-inventory-takeover

Conversation

@gwitwer

@gwitwer gwitwer commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Primary change type

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

Problem and motivation

Studio's project dependency graph never reached its fast path on a real install.

Most registry rows carry no definitionSlug, so the projector fell back to parsing each agent's
source to recover its name, bounded at 4 concurrent / 5s for the whole project. Anything that missed
emitted inventory-extraction-failed, and cacheable was prepared.every((agent) => !agent.extractionFailed).
One agent that could not be read made the entire graph non-cacheable — it re-projected from
scratch on every open and stayed degraded, which is the "Graph may be incomplete" banner.

Measured on a 76-agent / 9-project tree, main leaves the whole workspace and three of its projects
permanently degraded, and pays 176–784ms of blocking source extraction on every cold open.

This takes over #731 (SAP-2955, @ynadge), which is the fix for that, and had gone stale with merge
conflicts.

before
after

Same workspace, same 76 agents, same 11 projection warnings. Above: main. Below: this branch.

Opening the project, recorded

main — the banner is up the whole time:

https://raw.githubusercontent.com/sapiom/sapiom-js/pr-media/media/741/open-project-before.webm

This branch — first open renders provisional nodes at once, settles to ready, and the return visit
is served from cache:

https://raw.githubusercontent.com/sapiom/sapiom-js/pr-media/media/741/open-project-after.webm

Full-window stills of the same two runs:
before ·
after

Summary and scope

Carried in from #731 (Yash's six commits, rebased onto main with authorship preserved):

  • A package-inventory contract in @sapiom/agent (PACKAGE_INVENTORY_PROTOCOL, packageInventorySchema)
    that separates a canonical identity from a provisional one, and names why an identity is
    provisional (identity-pending, identity-unavailable, identity-invalid, duplicate-agent-key).
  • Graphs project from provisional identities immediately and enrich source names in the background,
    bounded and batched, cached by source fingerprint.
  • GET /api/workspaces/:key/system-graph/navigation — revision-matched navigation targets, so the
    browser stops reconstructing paths from mutable state.

Added here, because the defect survived the takeover:

  • The every(...) cliff had moved, not gone. feat(harness): migrate graphs to package inventory (SAP-2955) #731 replaced it with cacheable: inventory.status === "complete",
    and the package-inventory contract requires status: "degraded" whenever any identity is
    provisional. So one permanently unidentifiable agent still vetoed the whole project's cache — the
    original bug with a new spelling. Cacheability now depends on whether identity work has finished,
    not on how it finished: identity-unavailable, identity-invalid and duplicate-agent-key are
    settled and cacheable; only identity-pending refuses the cache, because caching mid-enrichment
    would freeze provisional names on screen.
  • The freshness spec's stale assertion had become unsatisfiable under feat(harness): migrate graphs to package inventory (SAP-2955) #731 (a degraded projection
    refreshes with its lifecycle preserved and never re-enters stale), and its lifecycle expectations
    were relaxed to degraded. Both are restored to ready, and the bus assertion is now bound to the
    revision the read observed rather than to the state alone.

Not in scope: retargeting #732 (follow-up). No change to relationship detection, the Group axis,
or the rail.

One commit from #731 was dropped: 4ce82ae1 test(harness): use checkout-invariant root graph key
resolved to empty. It renamed a graph-node test id used by a rail→graph→node navigation detour that
#733 deleted; nothing of it survived the rebase.

Related work

Related issue or discussion: SAP-2978. Takes over #731 (SAP-2955).

Validation

# command — result
pnpm --filter @sapiom/harness test        — 164 files / 2682 tests passed; perf 3 files / 10 passed
pnpm --filter @sapiom/agent  test         — 10 suites / 181 tests passed
pnpm --filter @sapiom/harness typecheck   — clean (tsc --noEmit x2)
pnpm --filter @sapiom/harness lint        — 0 errors, 1 pre-existing warning (rest.test.ts unused import)
pnpm terminology:check                    — passed (456 files)
pnpm examples:check                       — passed (12 templates)
E2E_PORT=5461 pnpm --filter @sapiom/harness test:ui — 439-440 passed, 1-2 failed
    session-tabs.spec.ts:268 fails on every full-suite run and on clean `main` too, and
    passes in isolation — pre-existing, verified on both. canvas-inspector.spec.ts flaked
    in 2 of 5 full-suite runs, on two *different* tests within the file (:130 and :106),
    both passing in isolation; that rotation is the known flake band, not a fixed failure.
    CI `playwright-mock` is green.

Real-server verification

CI runs VITE_MOCK=1, whose fixture has a handful of uniquely-named agents. Everything below was
measured against a local 76-agent / 9-project tree — duplicate names, agents with no defineAgent,
agents with no node_modules — on main and on this branch, each on its own port and its own
--state-root. Project names below are anonymised.

GET /api/workspaces/:key/system-graph, read after the projection settles:

project agents main this branch
entire workspace 76 degraded / X-…-Cache: degraded ready / complete
project A 12 degraded ready
project B 10 degraded ready
project C 4 degraded ready
project D 8 ready ready
projects E / F / G 9 / 7 / 7 ready ready

Every project is served from cache on the second open (revision stable, no re-projection). Cold-open
latency drops from blocking extraction to a provisional read: 784ms → 42ms for the 76-agent
workspace, 176–494ms → 11–51ms per project.

E1.3, forced mid-run. A 7-agent copy of one project, all canonical and ready on both builds.
Break one agent's source (invalid TypeScript in its entrypoint), then wait for the watcher:

main    → state=degraded  hdr=degraded  warn={'inventory-extraction-failed': 1}
branch  → state=ready     hdr=complete  warn={'inventory-extraction-failed': 1}
                          failed=['local:<project>/<agent>']

Same broken agent, same warning. On main it takes the whole project down with it; here the other
six stay cached and ready, and the broken one is named. Restoring the source returns it to a
canonical identity and clears the warning (revision 11 → 15), so the cache does not go stale forever.

E1.4. GET …/system-graph/navigation returns revision: 5 against a graph at revision: 5, 12
targets covering all 12 nodes, Cache-Control: no-store. On main that path falls through to the
SPA and returns text/html.

E1.5. system-graph-freshness.test.ts fails deterministically on macOS on clean main (18.2s,
vi.waitFor timeout on the directory-rename step). It passes here, 5 runs for 5, in ~1.7s. It is not
quarantined and no assertion was weakened — the opposite: it asserts ready where #731 had relaxed it
to degraded. Reverting only the settled gate reproduces main's 18s timeout, which is the evidence
that this change is what fixes it.

Mutation tests. Every guard added here was proven by breaking the code under it:

mutation expected to fail result
settled: inventory.status === "complete" (the old cliff) new cacheable spec + freshness spec both failed ✓ (freshness at 16.4s)
settled: true (ignore pending) pending-refuses-cache spec + 2 others 3 failed ✓
onChange → no-op (bus goes quiet) freshness bus assertion failed at that line ✓
drop the packageInventorySchema.parse boundary contract-rejection spec failed ✓

A mutation that did not fail, and what I did about it. #731 added
"uses the normalized inventory status even if a provider mutates its result", which mutates the
inventory object after listAgents resolves. build consumes the inventory in the same tick it
receives it, so that mutation can never land in between — removing the re-parse entirely left the
test green. It is replaced with one that hands the builder an inventory whose status contradicts its
own identities and asserts the build is refused; that one does fail when the boundary is removed.

Tests and documentation

  • system-graph.test.ts: settled-but-provisional agent is cacheable and still warns; a pending
    identity refuses the cache; the contract boundary rejects a self-contradicting inventory.
  • system-graph-freshness.test.ts: first read is provisional and degraded, settles to ready
    without the source ever becoming identifiable; bus announcement bound to its revision.
  • Plus the suites carried in from feat(harness): migrate graphs to package inventory (SAP-2955) #731 (package-inventory.spec.ts, system-graph-inventory.test.ts,
    WorkspaceGraphView.test.ts, api.test.ts, system-graph-navigation.test.ts).
  • packages/harness/docs/workspace-system-graph.md updated in the carried-in work.

Compatibility and release impact

  • Breaking or externally visible changes: None breaking. @sapiom/agent gains the package-inventory
    types and schema; @sapiom/harness gains the navigation route. A graph with an unreadable agent now
    reports ready with a per-agent warning instead of degraded for the whole project — that is the fix.
  • Changeset: Added — .changeset/calm-graphs-navigate.md (@sapiom/harness) and
    .changeset/bright-graphs-navigate.md (@sapiom/agent).

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 performed the rebase and conflict resolution, wrote the settled cacheability change and
its specs, and ran the verification above. Every claim in this description is a measured result: the
before/after tables come from paired runs of the same script against main and this branch, and each
new guard was mutation-tested by breaking the code under it and confirming the spec failed.

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.

Review round 1 — what changed

Finding 1 (afterCommit dropped on a superseded build) — fixed, and it was real.
reportRefreshFailure bumps the generation with refreshPending already false and
automaticRetryUsed already true, so a superseded build's enrichment callback was discarded with
nothing left to schedule a follow-up: every identity stays identity-pending, nothing can be
cacheable, stuck degraded. Enrichment is now armed on both exits.

The suggested placement did not work. Arming it before the canCommit branch fails
system-graph.test.ts › serves the current accepted revision when afterCommit immediately refreshes
— a synchronous refresh inside the callback bumps the generation out from under the result being
committed. It is armed after the commit decision on both paths instead. Both orderings now have a
spec that fails when reversed.

Finding 2 (unnameable contract types) — fixed. PackageInventoryIdentityIssue and the JSON
payload type (exported as PackageInventoryJsonValue) are now public from @sapiom/agent, with a
spec that uses both through the package entrypoint.

Finding 3 (duplicate slugs settled before enrichment) — fixed, independently. Found and pushed
before the review landed, same diagnosis. Duplicate detection now considers only agents whose
identity has resolved: two agents sharing a marker are not a collision until their sources are read.

Nit — workspaceRelativeLocalKey re-export: removed; the one test imports it from the shared
module.

Nit — navigation retry with no delay: fixed. Back-off between attempts, injectable so the specs
stay fast, with a spec that fails when the wait is removed.

Nit — serve() uses peek() instead of the awaited snapshot: kept deliberately, and it is
covered. afterCommit may refresh synchronously, so the awaited snapshot can already be superseded
by the time the response is written; peek() returns the accepted revision, which is what
serves the current accepted revision when afterCommit immediately refreshes pins. Body and header
are derived from the same snapshot, so they cannot disagree.

Confidentiality: the local path and the project names are removed from this description.

Every fix above was mutation-tested — the guard was confirmed to fail with the fix reverted.

ynadge and others added 10 commits August 30, 2026 00:33
Define the versioned @sapiom/agent inventory contract, move Studio graph identity enrichment behind it, and join path-bearing navigation through an atomic revisioned sidecar.

Closes: SAP-2955
…they settled

Gating the graph cache on the inventory's `status` reproduced the all-or-
nothing cliff it was meant to remove. The package-inventory contract requires
`status: "degraded"` whenever any identity is provisional, so one agent that
can never be named — a dashboard with no `defineAgent`, a package with no
`node_modules` — vetoed the whole project's fast path. The graph re-projected
on every open and never left `degraded`, which is the "Graph may be
incomplete" banner.

Cache on whether identity work has finished instead. `identity-unavailable`,
`identity-invalid` and `duplicate-agent-key` have all settled: re-projecting
cannot improve them, and a source edit re-projects through the watcher anyway.
Only `identity-pending` still has enrichment in flight, and caching that would
freeze provisional names on screen.

Measured against a 76-agent, 9-project tree: the whole workspace and every
project reach `ready` and serve `X-Sapiom-System-Graph-Cache: complete`, where
`main` leaves four of them `degraded`. Breaking one agent's source mid-run
takes that agent to a provisional key and leaves the other six cached and
`ready`; on `main` the same edit degrades the whole project. The affected
agents keep their `inventory-extraction-failed` warnings, so the graph is
cached without going quiet about what it could not resolve.

This is also what makes `system-graph-freshness.test.ts` pass on macOS, where
it has been failing on `main`. Restoring the `status` gate reproduces its 18s
timeout. The spec's bus assertion is now bound to the revision the read
observed rather than to the state alone, so it fails when the broadcast goes
quiet: stubbing `onChange` to a no-op times the wait out.

Co-authored-by: Yash <yash@sapiom.ai>
The contract doc said an unresolved agent makes the snapshot degraded, which
is now true only while its inspection is still pending.

Co-authored-by: Yash <yash@sapiom.ai>
…ision

Two agents that share a registry marker look identical only until their
sources are read. Counting them as a duplicate while both are still pending
reports `duplicate-agent-key` — an issue the contract treats as settled — so
the projection above it declared the identities finished and cached a graph
whose names were still guesses, then enrichment retracted the warning a moment
later.

Duplicate detection now considers only agents whose identity has resolved.
Restoring pending agents to the counts fails the new spec.

Co-authored-by: Yash <yash@sapiom.ai>
@github-actions

Copy link
Copy Markdown
Contributor

Review — PR #741 (round 1)

🔒 Confidentiality

Published surfaces are clean. Both changesets, packages/harness/docs/workspace-system-graph.md,
every new JSDoc block and every fixture use only generic vocabulary (acme, demo, @sapiom/*,
workspace-acme). No customer, partner, sibling-product or codename; no billing/reseller framing;
no private hostnames. .changeset/calm-graphs-navigate.md in particular reads correctly as text for
an anonymous npm consumer.

One item outside the shipped surfaces: the PR body pastes /Users/gwitwer/sapiom/agents and the
names of nine private local agent projects. That leaks a personal account name and an internal
project list into a public PR page. Nothing in the tarball is affected — worth trimming to
"a 76-agent / 9-project local tree" on the next edit, not a blocker.

Findings

1. A superseded build silently drops afterCommit, so enrichment is never armed

packages/harness/src/core/system-graph-store.ts:277finishSuccess only calls
this.afterCommit(result.afterCommit) on the commit path. When canCommit is false it hands off to
continueAfterSupersededBuild (line 335), which starts a follow-up build only if
entry.refreshPending
is true. reportRefreshFailure (line 139) is the one caller that bumps
entry.generation and sets refreshPending = false — and it also sets
automaticRetryUsed = true, which disables the recovery refresh in get() (line 87).

Failure scenario: a cold projection is in flight; the registry-refresh prerequisite fails and the
server calls reportRefreshFailure; the in-flight build then resolves, fails canCommit, and its
startEnrichment closure is discarded with refreshPending === false. No follow-up build is
scheduled and get() will not queue one. Every identity stays identity-pending, so
consumed.settled is false, cacheable is false, and the project sits at degraded — re-projecting
on every open — until an unrelated watcher event or an explicit Retry arrives. That is precisely the
stuck-degraded behaviour this PR exists to remove, reachable through a different door.

Cheapest fix: call this.afterCommit(result.afterCommit) in finishSuccess before the canCommit
branch (enrichment is idempotent — enqueueInspections already drops non-current tasks by
epoch/generation, and inspectSource short-circuits on a fingerprint hit), or have
continueAfterSupersededBuild forward it.

2. @sapiom/agent exports a contract whose two supporting types cannot be named

packages/agent/src/package-inventory.ts:107 and :6PackageInventoryIdentityIssue and
JsonValue are module-local, but both are reachable from exported types:
PackageInventoryAgent is defined in terms of Exclude<PackageInventoryIdentityIssue, …>, and
PackageInventoryStaticSignals.payload is a JsonValue.

Failure scenario: a consumer writes function explain(issue: PackageInventoryIdentityIssue) to
switch over the four provisional reasons — the whole point of the new contract — and cannot import
the name. They are forced to re-declare the union by hand, which then silently diverges when
protocol 2 adds a reason. Same for anyone building a staticSignals.payload. Export both from
package-inventory.ts and index.ts while this is still the first minor that introduces them;
adding them later is a second minor bump for no reason.

3. Duplicate registry slugs produce a wrong warning in a graph reported ready

packages/harness/src/core/system-graph-inventory.ts:530 / system-graph.ts:487 — on the very first
projection (no identity cache), two agents sharing a definitionSlug both get
candidateKey = markerAlias, so provisionalCount > 1 fires the duplicate-agent-key path. That
issue is not identity-pending, so settled is true and the graph commits cacheable: true
state: "ready", header complete, stored as lastGood.

Failure scenario: two agents whose sources declare distinct defineAgent names, both registered
under the same slug. The user's first open shows a ready graph — no "may be incomplete" hedge —
carrying "Multiple agents use X; kept each with a local identity." and two local: node labels, both
of which are wrong. It self-heals when enrichment flushes (~250 ms), but it is committed and served
as a settled result in the meantime. Treating duplicate-agent-key as settled is only sound once
identity work has actually run; deriving duplicate from a marker alias while every identity is
still pending is not.

Nits

  • packages/harness/src/core/system-graph-inventory.ts:21workspaceRelativeLocalKey is now
    re-exported for tests only; no production caller survives the rewrite.
  • packages/harness/web/src/lib/system-graph-navigation.ts:36 — the 3-attempt loop retries a
    behind-revision sidecar with no delay, so all three requests land in the same tick and the "wait
    for the commit to land" intent never has time to hold.
  • packages/harness/src/server/system-graph.ts:57serve() now discards the awaited snapshot for
    store.peek(); an enrichment flush queued as a microtask can flip the entry to stale in between,
    reporting X-Sapiom-System-Graph-Cache: degraded for a graph that committed ready.

Verdict

Request changes — finding 1 is the same stuck-degraded outcome the PR is fixing, reachable via
reportRefreshFailure; findings 2 and 3 are cheap to settle before this contract ships.

David Witwer added 3 commits August 30, 2026 01:28
A build that loses its generation carries the only callback that starts
identity work, and `reportRefreshFailure` supersedes an in-flight build with
`refreshPending` already false and `automaticRetryUsed` already true — so
nothing schedules a follow-up and no later read queues one. Dropping the
callback there left every identity `identity-pending`, which can never be
cacheable: the stuck-`degraded` state through another door.

Arm it on both exits, after the commit decision rather than before it. A
callback that refreshes synchronously would otherwise bump the generation out
from under the very result being committed; both orderings are now guarded by
a spec that fails when it is reversed.
`PackageInventoryAgent` is defined in terms of the identity-issue union and
`staticSignals.payload` in terms of the JSON type, but neither name was
exported. A consumer switching over the four provisional reasons — the point
of the contract — had to re-declare the union by hand, and that copy would
diverge silently the first time a protocol adds a reason.

Export both while this is still the minor that introduces them.
The bounded retry existed to survive a read that straddled a graph commit,
but it re-read as fast as the network answered, so all three attempts could
land on the same pre-commit snapshot and the wait never happened. Back off
between attempts, injectably so the specs stay fast.

Also drops the `workspaceRelativeLocalKey` re-export that no production
caller survived; its one test imports it from the shared module.
@github-actions

Copy link
Copy Markdown
Contributor

Review — PR #741 (round 2)

Delta reviewed: cc382338eaa81a (5 commits, 11 files).

Earlier findings

  1. afterCommit dropped on a superseded build — fixed. finishBuild now arms enrichment on
    the superseded exit too (system-graph-store.ts:286), with a spec that drives it through
    reportRefreshFailure. The arming is after continueAfterSupersededBuild, not before the
    canCommit branch as round 1 suggested — the author is right that the earlier placement breaks
    the synchronous-refresh commit; the recovery path still closes
    (enrichment → onIdentityChangerequestRefreshautomaticRetryUsed = false).
  2. Unnameable contract types — fixed. PackageInventoryIdentityIssue and
    PackageInventoryJsonValue are exported from package-inventory.ts and index.ts, exercised
    through the package entrypoint. Both are load-bearing (the issue union is reachable from
    PackageInventoryAgent, the JSON type from staticSignals.payload), and the existing
    @sapiom/agent minor changeset already covers them.
  3. Duplicate slugs settled before enrichment — fixed. Pending agents are excluded from the
    collision counts (system-graph-inventory.ts:521), so a shared marker no longer yields a settled
    duplicate-agent-key; the two agents fall out as identity-pending (second one disambiguated by
    the existing ~2 suffix, still schema-valid), the graph stays non-cacheable, and no warning is
    flashed then retracted. Alias ambiguity keeps the edges unresolved as before.

Round 1 got one thing wrong: the serve()/peek() nit. Body and header are derived from the
same peeked snapshot (server/system-graph.ts:60-67), so they cannot disagree — at worst the
response is a newer accepted revision than the awaited one, which is what the store test pins.

New findings

None.

Nits

  • web/src/lib/system-graph-navigation.ts:22 — the resolver's JSDoc now sits above
    backOffBeforeRetry; the exported function lost its doc block.
  • core/system-graph-inventory.ts:20 — double blank line left by the removed re-export.

Verdict

Approve — all three round-1 findings are fixed, published prose in the delta is clean.

@gwitwer
gwitwer marked this pull request as draft August 30, 2026 15:41
@gwitwer

gwitwer commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator Author

Converting to draft — not because anything is wrong with it, but because it should not merge in this shape.

95% of this PR is one commit: 5549b15b, 6477 lines across 36 files, written by @ynadge as a single atomic migration of the graph onto the package-inventory contract. There is no seam to split it on, and re-cutting someone else's commit into pieces they didn't write is worse than leaving it whole. The other 13 commits (~660 lines) are the rebase and review fixes on top, and they depend on that base.

The user-visible defect this PR incidentally fixes — the permanent "Graph may be incomplete" banner — is being landed separately as a narrow fix on main (SAP-2983), touching only the cacheable rule in system-graph-inventory.ts and its one consumer. That way the visible win doesn't require reviewing a 6.5k-line migration.

What's genuinely useful here, kept for whoever lands the migration:

  • 5040e36c found that system-graph-freshness.test.ts is not the macOS filesystem-watcher flake it has been written off as — it catches a real caching bug. Restoring the status gate reproduces its 18s timeout exactly.
  • Round 1 and 2 review findings are all resolved here: afterCommit dropped on a superseded build, unnameable contract types, duplicate slugs settling before enrichment, and the navigation resolver falling behind its commit.
  • The claim that main "re-projects on every open, never caches" could not be reproduced. Measured behaviour: degraded projections bump revision once and stabilise. What is permanent is the label and banner, not unbounded re-projection.

@ynadge — this is yours to land whenever you're ready. Nothing has been force-pushed to your branch; #731 is untouched.

ynadge commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Thanks for pushing this investigation and its follow-up fixes forward. SAP-2955 is being consolidated from current main in #750.

The replacement keeps the useful fixes established here—settled-identity caching, pending-collision handling, enrichment after superseded builds, the public identity-issue type, and bounded navigation backoff—while keeping package inventory limited to identity/location and preserving the current right-pane, group-aware map.

Per Yash's direction, I am leaving #741 open so he can coordinate with you directly. I have not changed this branch or its history.

ynadge commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Yash has now confirmed that #750 supersedes this PR as well. Closing #741 in favor of the consolidated replacement; this branch and its history remain untouched.

@ynadge ynadge closed this Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants