Skip to content

feat(harness): adapt invocations to graph evidence - #755

Open
ynadge wants to merge 4 commits into
yashnadge/sap-2985-api-define-and-validate-the-graph-evidence-protocolfrom
yashnadge/sap-2986-infrastructure-adapt-direct-invocations-to-graph-evidence
Open

feat(harness): adapt invocations to graph evidence#755
ynadge wants to merge 4 commits into
yashnadge/sap-2985-api-define-and-validate-the-graph-evidence-protocolfrom
yashnadge/sap-2986-infrastructure-adapt-direct-invocations-to-graph-evidence

Conversation

@ynadge

@ynadge ynadge commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Primary change type

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

Problem and motivation

The finalized direct-invocation scanner reports caller-scoped candidates, but
those candidates are not themselves durable package graph evidence: the source
endpoint is implicit, inventory scope and producer freshness are absent, and
scanner cache state is not evidence lifecycle state. SAP-2986 adapts that
bounded scanner into the package graph-evidence protocol introduced by
SAP-2985 without broadening the scanner or changing the System Graph payload.

Summary and scope

  • preserve the finalized caller-scoped, syntax-only scan boundary and attach a
    stable digest of the bounded source content actually analyzed
  • resolve literal targets against the exact package inventory, then emit
    explicit fromAgentKey / toAgentKey invokes / static-invocation
    evidence with typed call modes, producer/version, and opaque callsite refs
  • aggregate missing, pending, failed, dynamic, incomplete, and freshness-less
    caller scans conservatively; pending, failed, missing, or inconsistent scan
    sets retain last-good evidence, while every settled bounded scan atomically
    refreshes its proven literal subset even when structural limits or dynamic
    targets keep topology partial
  • translate invalid, unknown, ambiguous, self, and duplicate cases into shared
    diagnostics/quarantine plus the existing path-free public warnings
  • derive the existing StaticInvocationGraphEdge DTO from normalized accepted
    evidence so browser, mock, and server behavior stays compatible

Intentionally out of scope: TypeScript type checking, output-to-input data-flow
analysis, runtime/log evidence, persistence/replay, and UI redesign.

Related work

Related issue or discussion:

Validation

pnpm --filter @sapiom/harness exec vitest run src/core/system-graph-invocation-evidence.test.ts src/core/system-graph.test.ts src/core/system-graph-relationships.test.ts src/core/canvas-interconnections.test.ts — 4 files, 79 tests passed
pnpm --filter @sapiom/harness test — 172 files / 2,979 regular tests and 3 files / 10 performance tests passed
pnpm --filter @sapiom/harness build — passed
pnpm --filter @sapiom/harness typecheck — passed
pnpm --filter @sapiom/harness lint — passed
pnpm build — all workspace builds passed
pnpm typecheck — all workspace typechecks passed
pnpm lint — passed with pre-existing warnings in unrelated packages and no Harness warnings
pnpm test — every affected Agent/Harness suite passed; the workspace run later stopped on the pre-existing agent-core `bundle-error.spec.ts` unreadable-directory permission-classification failure in this sandbox (the same unrelated local-environment failure documented on #752)
pnpm provider-copy:check — passed (79 audited files)
pnpm terminology:check — passed (466 files)
pnpm examples:check — passed (12 templates)
pnpm exec changeset status --output=/tmp/sap-2986-changeset-status.json — parsed successfully and includes steady-agents-connect
git diff --check — passed

Tests and documentation

Added focused adapter fixtures for explicit endpoints, direct-only boundaries,
determinism, content freshness, opaque callsites, conservative per-caller
coverage, last-good retention, complete retraction, target resolution,
diagnostics/quarantine, and privacy-safe warning projection. Added an integrated
System Graph lifecycle tests, including cold start and a later source edit
through the production two-phase path, consecutive settled structural-partial
scans, and source-fingerprint tests proving that content—not mtimes or watcher
paths—drives evidence freshness. Updated the System Graph contract documentation
with the adapter and replacement semantics.

Compatibility and release impact

  • Breaking changes: None. Existing public System Graph node, edge, warning,
    server, mock, and browser payload shapes are unchanged.
  • Existing lifecycle behavior is preserved: a deterministic dynamic-target
    limitation remains settled/cacheable and keeps its path-free warning, while
    the internal evidence contract records topology coverage as partial.
  • Changeset: Added a patch changeset for @sapiom/harness.

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.

Codex implemented the bounded adapter from the approved Linear architecture,
added tests and documentation, and performed a full diff self-review. Every
affected test, the complete Harness suite including performance tests, and the
workspace build/typecheck/lint checks listed above were run after the changes.

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

Copy link
Copy Markdown
Contributor

Review — PR #755 (feat(harness): adapt invocations to graph evidence)

No confidentiality problems: the changeset, the workspace-system-graph.md edits, the new
JSDoc and the test fixtures are all provider-neutral and name no company, host, or person.

Findings

1. In the two-phase production path, one dynamic target permanently erases every static edge

packages/harness/src/core/system-graph-invocation-evidence.ts:1430 / system-graph.ts:405

advancePackageGraphStaticEvidenceState only replaces accepted on a complete result
(package-graph-evidence.ts:1136); a partial success with an existing non-failed state returns
{status: "stale", accepted: current.accepted}. In production StaticSystemGraphBuilder is always
two-phase, so build #1 for a workspace is all-pending (peekInvocations returns undefined
for every caller, system-graph.ts:367-381) → complete = false → coverage partial → state
{status: "partial", accepted: <empty result>}.

If any later build is also never complete, that empty first result stays pinned as accepted
forever, and projectEdges reads only accepted — so the graph shows zero edges.
complete is now forced to false by a single dynamic-target warning (line 1275), by
result.complete === false, and by observationsTruncated on large workspaces
(system-graph-relationships.ts:239-248).

Failure scenario: a workspace where one agent calls agents.run({ definition: someVar }) and also
statically calls two other agents. Before this PR the graph rendered both static edges plus a
dynamic-target warning. After it, on every server start the graph renders no edges at all and
stays degraded indefinitely. The new tests miss this because they drive the builder through a
provider with no peekInvocations, which takes the awaited legacy path where build #1 is
complete.

Fix direction: seed/refresh accepted from the latest attempt when the previous accepted result is
the empty cold-start placeholder, or don't let a first all-pending phase occupy the accepted slot
(pass previousState only once a real scan has landed).

2. Behavior change shipped as a patch changeset that claims none

.changeset/steady-agents-connect.md

A dynamic-target warning used to keep the build cacheable — the deleted test was literally named
"projects dynamic extraction warnings without degrading cacheability"; it is now asserted
false (system-graph.test.ts:1644). cacheable: false makes SystemGraphStore commit the
snapshot as degraded instead of ready (system-graph-store.ts:362-380), so a workspace with any
dynamic target now permanently shows the degraded state + Retry affordance and re-scans on every
open (one automatic retry per open, system-graph-store.ts:85-92). The changeset says only
"preserving the existing System Graph edge and warning payloads" and the PR states "externally
visible changes: None". The payload shape is preserved; the snapshot state a user sees is not. Say
so in the changeset body.

3. sourceFingerprint is optional but its absence silently degrades the graph forever

system-graph-relationships.ts:33 / system-graph-invocation-evidence.ts:1261

Any AgentInvocationProvider that does not set sourceFingerprint gets an invalid-fingerprint
diagnostic per caller, a Could not fully inspect <agent>. warning, and complete = false — i.e.
finding 1's failure mode — with nothing in the type telling the implementer. Either make the field
required on AgentInvocationProviderResult (it is internal, so this costs nothing) or document the
consequence on the JSDoc.

Nits

  • detectAgentInvocations (canvas-interconnections.ts:965) now has no production caller — only its
    own tests. Delete it or mark it test-only.
  • system-graph-invocation-evidence.ts has no file-level JSDoc explaining the adapter's role, unlike
    every sibling module in core/.
  • Scratch file packages/harness/src/core/__repro.test.ts was created while verifying finding 1 and
    the sandbox refused to delete it — please remove it before merging.

Verdict: Request changes — finding 1 is a silent, user-visible loss of all invocation edges for
any workspace containing a dynamic target, and it is invisible to the current test suite.

@ynadge
ynadge force-pushed the yashnadge/sap-2986-infrastructure-adapt-direct-invocations-to-graph-evidence branch from abae2f8 to 11cd19c Compare August 31, 2026 18:15
@github-actions

Copy link
Copy Markdown
Contributor

Review round 2 — PR #755

Delta: 11cd19c0 (fix(harness): preserve edges after partial cold scan). Round-1's sha
abae2f8a was force-pushed away, so commit 1 is taken as unchanged.

Resolved from round 1: finding 2 (changeset now states the degraded/retryable behavior
change), finding 3 (JSDoc documents the fingerprint consequence), all three nits. No
confidentiality issues in the new prose.

1. Finding 1 fixed only for cold start — edges now freeze at the first settled scan

system-graph.ts:411 clears state only when every scan is pending. invalidateSource
(system-graph-relationships.ts:153) makes exactly one caller pending, so state survives,
and advancePackageGraphStaticEvidenceState (package-graph-evidence.ts:1139-1147)
returns stale with the old accepted for every partial-after-partial. With any
dynamic-target present, coverage is never complete
(system-graph-invocation-evidence.ts:528), so once the first settled partial seeds
accepted, no later edit replaces it: add a static call in agent B and the edge never
appears; delete one and the stale edge stays. The producer-slot reset doesn't fire either —
the inventory revision hashes only the agent list (system-graph-inventory.ts:360), not
source content. Only adding/removing an agent, an invalidateScope, or a process restart
unsticks it. The new test stops one build too early to catch this.

2. The "retryable" state the changeset now advertises cannot be retried

retryFailed (system-graph-relationships.ts:173-182) re-arms only snapshots whose
scanner-level result.complete === false. A dynamic target leaves scanner complete === true — only the adapter downgrades it. So manual Retry (server/index.ts:2547) re-arms
nothing, rebuilds identical evidence, and stays degraded forever. Either re-arm on
adapter-reported incompleteness, or stop presenting a dynamic target as retryable.

Verdict: Request changes — a dynamic target still permanently pins the invocation
graph, now to the first settled scan rather than to empty.

Housekeeping: packages/harness/src/core/zz-scratch-verify.test.ts is a scratch file I
wrote to verify finding 1; the sandbox refused every delete. It is untracked — please
remove it.

ynadge commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

Copy link
Copy Markdown
Contributor

Review round 3 — PR #755

Delta since round 2's 11cd19c0: one commit, 5f344540 (fix(harness): refresh settled partial evidence). No confidentiality issues in the new changeset or doc prose.

Resolved: round-2 finding 2 — cacheable is now decoupled from topology complete, so a
dynamic target no longer forces a degraded/un-retryable snapshot, and the changeset text
matches. This restores main's cacheability rule exactly
(system-graph.ts:449 vs main's invocationsComplete).

1. Not fixed: edges still freeze permanently when incompleteness is not a dynamic target

system-graph-invocation-evidence.ts:742

The refresh escape hatch requires cacheable === true, but cacheable is cleared by every
non-dynamic incompleteness (lines 512, 524, 583) — including scan.result.complete !== true.
That flag is set for structural, permanent workspace properties, not transient ones: any
symlink in an agent's source tree, a directory past maxDepth, more than maxFiles sources,
an unreadable directory (canvas-interconnections.ts:125-195), or a workspace over
INVOCATION_OBSERVATION_MAX_PATHS (10k paths, system-graph-relationships.ts:245).

For such a workspace previousState is always passed, and
advancePackageGraphStaticEvidenceState returns {status: "stale", accepted: current.accepted}
(packages/agent/src/package-graph-evidence.ts:1139-1147) on every build after the first
settled one. projectEdges reads only accepted (line 302), so the first settled scan's
edges are pinned until the process restarts, an agent is added/removed, or invalidateScope
fires — a source edit never lands.

Failure scenario: an agent directory containing one symlink. On main the graph is
non-cacheable and re-projects fresh edges on every open. After this PR it is still
non-cacheable, re-scans every open, and renders the first scan's edges forever; adding a
static call to another agent never shows an edge, deleting one leaves a phantom edge.

The new system-graph.test.ts case only exercises the cacheable === true refresh path.
No test drives two consecutive settled complete: false scans through the adapter with
previousState, which is why this survives.

Fix direction: gate the refresh on transient incompleteness (pending/failed scans) rather
than on cacheable; a settled scan whose incompleteness is structural is still a fully
proven literal subset and should atomically replace accepted the same way a dynamic-target
scan now does.

Verdict: Request changes — one root cause remains from round 2, narrowed but not closed.

Housekeeping: packages/harness/src/core/tmp-verify-freeze.test.ts is a scratch file I
wrote while verifying the above; the sandbox blocked every delete/move. It is untracked —
please remove it.

ynadge commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

Copy link
Copy Markdown
Contributor

Review round 4 — PR #755

Delta since round 3's 5f344540: one commit, ee72cd56 (fix(harness): refresh structurally partial scans). No confidentiality issues — the changed doc prose and test
fixtures name no company, host, or person, and the changeset is untouched and still
accurate.

Round-3 finding 1 is fixed as directed: refresh is now gated on a new settled flag
(system-graph-invocation-evidence.ts:441) that is cleared only by transient or
inconsistent scan sets — pending, failed, missing, duplicate, and unexpected callers —
rather than by cacheable. A settled scan that is structurally incomplete (symlink,
maxDepth/maxFiles, unreadable directory, truncated observations) now passes undefined
as the current state (line 747), so advancePackageGraphStaticEvidenceState returns
{status: "partial", accepted: <fresh result>} and source edits land on every build. The
new adapter test drives two consecutive settled complete: false scans through
previousState (add and remove), which is the case round 3 said was missing, and the
system-graph.ts test now separates the retain-on-failure path from the refresh path.
Scratch files from earlier rounds are gone; git status is clean.

No new findings. Nothing from rounds 1–3 remains open, and I found nothing the earlier
rounds got wrong.

One nit, no action required: the new doc line "a structurally incomplete scan remains
non-cacheable and retryable" (workspace-system-graph.md:150) does not hold for the
observationsTruncated flavor — peekInvocations synthesizes complete: false without
mutating the stored snapshot, so retryFailed (system-graph-relationships.ts:178) re-arms
nothing there. Retrying a size-truncated workspace would not help anyway.

Verdict: Approve — the remaining root cause from rounds 2–3 is closed.

@sapiom-review

sapiom-review Bot commented Aug 31, 2026

Copy link
Copy Markdown

🔍 Advisory review of ee72cd5

The PR introduces a new boundary-adapter file and threads a package-graph-evidence cache through StaticSystemGraphBuilder, with end-to-end tests for freshness, content-based identity, and last-good retention. The architectural intent (opaque callsite references, content-only freshness, last-good replaced on settled partial scans) is internally consistent. However, the adapter and system-graph.ts both import several type aliases and helper functions from @sapiom/agent that are not present anywhere on trusted main — the trusted @sapiom/agent public surface ends at PACKAGE_INVENTORY_PROTOCOL/packageInventorySchema and buildManifest/validateGraph. As-is, harness will not typecheck and tests will not run. This is the only blocking issue; once the dependent @sapiom/agent symbols exist, the remaining changes are additive (new optional sourceFingerprint field, new optional internal cache map, retained public edge DTO) and do not regress existing public types or test behavior. No prompt-injection content was observed in the diff.

⛔ Blocking concerns (2)

  • Imports from @sapiom/agent symbols that do not exist on trusted mainpackages/harness/src/core/system-graph-invocation-evidence.ts:10 (confidence 0.95)
    The new adapter file imports 8 type aliases and 3 functions from @sapiom/agent (advancePackageGraphStaticEvidenceState, createPackageGraphEvidenceStaticResult, projectPackageGraphEvidence, PackageGraphEvidenceCoverageGap, PackageGraphEvidenceDiagnostic, PackageGraphEvidenceDigest, PackageGraphEvidenceProducer, PackageGraphEvidenceStaticResult, PackageGraphStaticEvidenceCandidate, PackageGraphStaticEvidenceState). None of these names exist anywhere on trusted main — verified by enumerating the full @sapiom/agent public surface in packages/agent/src/index.ts (which ends at PACKAGE_INVENTORY_PROTOCOL/packageInventorySchema and buildManifest/validateGraph/assertValidGraph) and by repo-wide grep for PackageGraph*/graph-evidence (zero matches). Harness typecheck will fail at module-resolution / type-evaluation time. The new test file (system-graph-invocation-evidence.test.ts) and the refactored system-graph.ts both depend on the same module, so the build will not run without a corresponding change to @sapiom/agent that is not part of this PR. Recommended action: ship the supporting @sapiom/agent package code in the same PR, or split this so the adapter and the agent-side protocol land together.
    Evidence: packages/agent/src/index.ts (only declares PACKAGE_INVENTORY_PROTOCOL, packageInventorySchema, buildManifest, validateGraph, assertValidGraph and their types); repo grep PackageGraph|graph-evidence|advancePackageGraph|createPackageGraphEvidence|projectPackageGraph returns nothing under /workspace/sapiom-js-main
  • Type imports from @sapiom/agent that do not exist (compile-time breakage of package-graph wiring)packages/harness/src/core/system-graph.ts:4 (confidence 0.90)
    system-graph.ts imports PackageGraphStaticEvidenceState and PackageInventory as types from @sapiom/agent, then threads them through StaticSystemGraphBuilder's new evidenceByWorkspace map. PackageInventory is real (used in consumeInventory) but PackageGraphStaticEvidenceState is not. The diff also narrows the AgentInvocationProviderResult contract by adding a required (in callers, optional in the type) sourceFingerprint: sha256:<...> field that the adapter relies on for cacheable and complete, and the new system-graph test file system-graph.test.ts rewrites EMPTY_INVOCATIONS and the test helper to pass sourceFingerprint: SOURCE_FINGERPRINT and complete: true. The diff is internally consistent but depends on the missing @sapiom/agent symbols.
    Evidence: packages/agent/src/index.ts (no PackageGraphStaticEvidenceState export); packages/harness/src/core/system-graph.ts lines 1-13 in the new file; system-graph.test.ts now sets EMPTY_INVOCATIONS to { invocations: [], warnings: [], complete: true, sourceFingerprint: SOURCE_FINGERPRINT } which the adapter requires for cacheable=true

Advisory only — never an approval. sapiom-review · run a4c2c891-2772-4f0c-824f-badbdfa27602

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.

1 participant