Skip to content

feat(harness): discover unlinked agents (SAP-2956) - #732

Merged
ynadge merged 7 commits into
mainfrom
yashnadge/sap-2956-discover-unlinked-agents-and-reconcile-studio-inventory
Aug 31, 2026
Merged

feat(harness): discover unlinked agents (SAP-2956)#732
ynadge merged 7 commits into
mainfrom
yashnadge/sap-2956-discover-unlinked-agents-and-reconcile-studio-inventory

Conversation

@ynadge

@ynadge ynadge commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Primary change type

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

Problem and motivation

Studio only knew agent projects that already carried sapiom.json or had been connected manually. Markerless defineAgent and legacy defineOrchestration projects therefore stayed absent from the Workflows rail and workspace graph, while broad legacy extraction could execute project code merely to recover identity.

The deterministic graph path also described its per-caller literal-call scanner as a general relationship provider. That contract could only express caller-to-target invocations, so presenting it as the future relationship model would misrepresent what its evidence proves.

Summary and scope

  • Discover current and legacy agents with bounded TypeScript syntax analysis only, including local alias and re-export resolution, without bundling, type-checking, importing, or executing project code.
  • Apply marker-first precedence, repository/symlink/ignore confinement, exact traversal and byte/module/lookup budgets, fail-closed unresolved export handling, and bounded LRU memoization.
  • Reconcile source-only, marker-backed, connected, and newly scaffolded registry rows without losing cloud metadata or deleting evidence outside a proven scan envelope.
  • Compose with PR feat(harness): migrate graphs to package inventory [SAP-2955] #750's package-inventory architecture while keeping workspace discovery completeness separate from identity settlement and direct-invocation completion.
  • Publish rail rows, discovery lifecycle, session contexts, graph inventory, and revision-matched navigation through one accepted generation.
  • Share one canonical workspace watcher across session and graph consumers, with bounded polling fingerprints and source/dependency observations.
  • Render cached inventory immediately, then extract literal direct source invocations in a bounded background queue with retry and stale-generation retirement.
  • Specialize the provider surface as AgentInvocationProvider, AgentInvocationCandidate, and related invocation-specific cache/lifecycle contracts.
  • Emit StaticInvocationGraphEdge with kind: "invokes" and basis: "static-invocation"; strict browser parsing rejects the obsolete "static" basis.
  • Keep invocation evidence internal and preserve blocking/async modes, target resolution, duplicate warnings, graph layout, navigation, and stale/degraded behavior.
  • Prevent unprompted legacy Canvas or identity extraction for source-only rows and revalidate authorized marker/cloud evidence at the actual extraction boundary.
  • Add latest-successful browser workflow ordering, production/mock graph parity, Polsia-style coverage, docs, and a Harness minor changeset.

Direct invocation analysis scans each inventoried agent source root independently. It does not create a TypeScript Program or TypeChecker, inspect invocation-input provenance, follow outputs through formatter/helper/router code, or scan arbitrary workspace router modules.

Intentionally out of scope: package-wide output-to-input data-flow evidence, feeds edges, runtime lineage, LLM inference, and backend inventory ingestion. Cross-agent data flow will use a separate package-level evidence provider.

Related work

Related issue or discussion: SAP-2956

SAP-2955 replacement / PR #750 is merged and defines the current package-inventory architecture.

Validation

Verified on the published PR head 9d0e29eebb0d48892e6f88eb97fe13b033e92055 after rebasing onto the merged PR #750 result:

  • pnpm --filter @sapiom/harness exec vitest run — 171 files, 2,968/2,968 tests passed
  • Focused invocation, inventory, graph, parser, grouping, server, and freshness suites — 247/247 passed; post-terminology affected-suite rerun — 90/90 passed
  • pnpm --filter @sapiom/harness test:perf — 10/10 passed
  • pnpm --filter @sapiom/harness typecheck — passed
  • pnpm --filter @sapiom/harness lint — passed with zero warnings
  • pnpm --filter @sapiom/harness build — passed
  • Polsia workspace Playwright scenarios — 2/2 passed
  • pnpm terminology:check — 465 files passed with no stale allowlist entries
  • pnpm terminology:check:test — 10/10 passed
  • pnpm changeset status --since=origin/main — passed
  • git diff --check and changed-file Prettier audit — passed

Tests and documentation

Added analyzer, registry, watcher, invocation-provider, publication, no-execution, browser-ordering, mock-parity, and production-server integration coverage. The direct-invocation boundary regression proves coordinator source yields Coordinator → Research and Coordinator → Growth without manufacturing Research → Growth data flow. Cache regressions separately prove that settled provisional identity is cacheable while incomplete workspace discovery is not. Updated the agent-discovery and workspace-system-graph contracts.

Compatibility and release impact

  • Breaking or externally visible changes: Markerless local agents now appear in Studio. Public invocation-edge JSON changes basis: "static" to basis: "static-invocation"; clients validating the exact edge shape must accept the new value.
  • Public graph JSON remains path-free, source file/line/column evidence stays internal, and public workflow JSON remains unchanged.
  • Changeset: Added for a minor release of @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 and reviewed the discovery, reconciliation, watcher, direct-invocation, graph, browser, test, and documentation changes. The final branch was rebased onto the merged PR #750 result and verified with the commands and results above.

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.
  • I ran the relevant build, typecheck, lint, and test commands.
  • I updated documentation for user-facing changes.
  • I added a Changeset for the published-package change.
  • I can explain and maintain every submitted change, including AI-assisted work.

@github-actions

Copy link
Copy Markdown
Contributor

Review — PR #732 feat(harness): discover unlinked agents (SAP-2956)

Public-copy check: clean. Changeset, both docs, JSDoc, tests and fixtures are provider-neutral;
every company-ish string in the diff is fictional (@acme/billing, /Users/demo/…,
slack-notifier as a generic example, Polsia — already the repo's public example workspace).
No new dependency, no package.json/exports/files change; typescript was already a runtime
dep. sourceDefinitionName / markerPresent are stripped in both publicWorkflowInfo
implementations and never reach /api/workflows, /api/state, or harness-context.json. Minor
changeset level is right.

Findings

1. git init in a markerless agent silently deletes it from the rail — and the doc says the opposite

stopsAtRepositoryBoundary runs after onDirectory (marker) but before onAdmittedDirectory
(source proof) in both walks (packages/harness/src/core/agent-project-discovery.ts:646-660). So the
two "ordered proofs" §1 now advertises as equals are not equal at a nested checkout: a marker-backed
agent survives (workflow-registry.test.ts:637, "the marker outranks the boundary"), while a
source-proven one is not merely undiscovered but reconciled away
workflow-registry.test.ts:1418 asserts await registry.list() becomes [] after mkdir .git and
a parent rescan.

Failure scenario: user has a markerless defineAgent project at ~/work/leasing, opens ~/work as
the Project, sees it on the rail, then runs git init in it (or clones a markerless agent repo into
the workspace). Next scan of ~/work removes the row. Only re-selecting ~/work/leasing itself
brings it back.

packages/harness/docs/agent-discovery.md:171-179 still states, unqualified, "It is still
registered… so 'one git repo per agent' works exactly as it did… a scaffold that runs git init in
its own new directory registers within the same second as one that does not." That paragraph is now
true only of markers, and it sits three sections below the §1 text this PR rewrote to promise
markerless parity. Per the doc's own rule ("if the code and this document disagree… this document is
a bug"), qualify it — and either mention the caveat in the changeset or explain why the asymmetry is
intended. The agent-project-discovery.ts:420 line reference in that paragraph is also stale
(stopsAtRepositoryBoundary is at :560).

2. Watcher observation probes re-lstat every ancestor on every pass

admittedObservedStat (packages/harness/src/core/workspace-watcher.ts:331-386) walks the full
ancestor chain per probe, doing one lstat on the directory and one on <dir>/.git at each
level, with no memoization across probes.
snapshotWorkflowSourceRootsAsync calls it up to WORKFLOW_SOURCE_OBSERVATION_MAX_PROBES = 10_000
times per pass (:61, :531-546), and that pass runs on every 250 ms debounce and on the 2 s poll
fallback (:598, :760) — on Linux and on any macOS session whose fs.watch errored, the poll is
the steady state. A probe 8 levels deep costs ~17 lstats, so a workspace with a few hundred
observations burns thousands of syscalls every two seconds, on top of a directory walk whose budget
this PR raises from 2,500 to 10,000 nodes (agent-project-discovery.ts:85) and which now also
lstats up to 10,000 source files (workspace-watcher.ts:59).

The analyzer already solves exactly this: isAdmittedModulePath memoizes admission in
state.admittedDirectory (agent-source-discovery.ts:998). Give the snapshot pass the same
per-pass ancestor cache.

3. Two JSDoc blocks now describe code that no longer exists

snapshotWorkspaceWorkflows (workspace-watcher.ts:169-172) says "The synchronous form remains for
the session watcher, whose constructor requires an immediate baseline" — SessionWorkspaceWatcher
now uses snapshotWorkspaceWorkflowsAsync (:626), and :592 says so explicitly. watchBudget()
(:70-72) says "tighter than a registry scan's, because this walk is synchronous" while returning
AGENT_PROJECT_SCAN_MAX_NODES. The sync export has no production caller left (only
workspace-watcher.test.ts and agent-project-scan.perf.test.ts); mark it test/compat-only like
AGENT_PROJECT_WATCH_MAX_NODES correctly does, or drop it.

Nits

  • definition-name.ts:36-40: the options.authorizeBeforeLaunch || options.beforeLaunchAuthorization
    ternary silently drops any future CachedExtractionOptions field; pass options unconditionally
    and let extractWorkflowGraphCached default it.
  • server/index.ts:1729-1737: JSON.stringify([...map].sort()) sorts [k,v] pairs by string
    coercion — works today, breaks quietly if a value ever contains a comma.
  • acceptedCanonicalScopeByLexicalRoot (server/index.ts:1712) grows one entry per lexical root
    ever observed and is never pruned in a long-lived server.

Verdict: Approve after fixing the doc claim in finding 1 (or the behavior); findings 2–3 are
follow-up-safe. The confidentiality, dependency, tarball and API-surface surfaces are clean.

@github-actions

Copy link
Copy Markdown
Contributor

Follow-up review — PR #732 (round 2)

Delta since 65bbc346: one commit, 20e3dd23, touching only
.changeset/soft-agents-discover.md and packages/harness/docs/agent-discovery.md. No code
moved. Public-copy check on the new prose: clean, provider-neutral, no named third party.

Finding 1 — fixed. The "If the new agent is its own git repository" paragraph now states the
marker/source asymmetry and the parent-scan retirement explicitly, matching
workflow-registry.test.ts ("retires a parent-owned source row when that candidate becomes a
foreign repository"), and the stale agent-project-discovery.ts:420 reference is gone. The
changeset names the nested-repo caveat (positive framing only — it does not say a
parent-discovered row is retired after git init — acceptable, but that is the surprising half).

Not fixed

  • Finding 2admittedObservedStat (packages/harness/src/core/workspace-watcher.ts:331)
    still re-lstats the whole ancestor chain per probe, up to 10,000 probes per pass, on every
    250 ms debounce and 2 s poll. No per-pass ancestor cache added.
  • Finding 3 — both stale JSDoc blocks remain: workspace-watcher.ts:169-172 still says the
    sync form exists because "the session watcher['s] constructor requires an immediate baseline"
    while that constructor calls checkNowAsync() (:596), and watchBudget() (:70) still
    says "this walk is synchronous". snapshotWorkspaceWorkflows still has no production caller.

New

  • Doc line references broken by this PR, in the file the push edited.
    agent-project-discovery.ts grows +202 lines here, and only the one flagged reference was
    updated. docs/agent-discovery.md:36 cites inspectAgentProjectMarkerSync at :237 (exact in
    main, now :293:237 is an unrelated O_RDONLY constant), and :67 cites
    walkAgentProjectTree/Async at :446/:479 (now :588/:629). The doc's own preamble tells
    readers these refs are how to check its claims, so wrong ones actively mislead. Round 1 caught
    only :420; fix the sibling refs in the same pass.

Verdict: Approve — the round-1 blocker is resolved; the remaining items (stale doc refs, stale
JSDoc, watcher probe cost) are all follow-up-safe and none affect published behavior or copy.

@ynadge
ynadge force-pushed the yashnadge/sap-2956-discover-unlinked-agents-and-reconcile-studio-inventory branch from 20e3dd2 to 5deea09 Compare August 30, 2026 08:35

ynadge commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

/claude-review

@github-actions

Copy link
Copy Markdown
Contributor

Follow-up review — PR #732 (round 3)

Delta since round 2: one commit, refactor(harness): specialize direct invocation evidence — the RelationshipInvocation rename, basis: "static"
"static-invocation", two doc updates, two new tests. New prose is
provider-neutral; public-copy check clean.

New

  • Released public JSON value changed; the changeset does not call it breaking.
    basis: "static" shipped in @sapiom/harness 0.9.0 (CHANGELOG.md:61-62,
    b66ff0e) and is live in 0.10.0. This commit renames it and rejects the old
    spelling — and parseSystemGraph
    (packages/harness/web/src/lib/system-graph.ts:248-256) throws
    "Invalid system graph response" for the whole payload on one stale edge, so a
    third-party consumer of the documented GET /api/workspaces/:key/system-graph
    loses the entire graph, not one connector. .changeset/soft-agents-discover.md
    only mentions the new value in passing; it needs a Breaking callout and a
    migration line ("edges previously carrying basis: \"static\" now carry
    basis: \"static-invocation\""). The PR body already labels this externally
    visible — the changelog is the surface that has to. In fairness:
    SystemGraphEdge is not exported from the package entry and server + web ship
    together, so no in-tree consumer skews.

Not fixed from earlier rounds

  • R1 update SDKs to use payment protocol data #2admittedObservedStat (core/workspace-watcher.ts:331) still re-lstats
    the full ancestor chain per probe, up to 10,000 probes per pass.
  • R1 langchain SDK config cleanup #3 — stale JSDoc remains at workspace-watcher.ts:169-172 and watchBudget()
    (:69-70); the sync form still has no production caller.
  • R2 new — doc line refs still stale in the file this commit edited:
    docs/agent-discovery.md:36 cites inspectAgentProjectMarkerSync at :237
    (actual :293), :67 cites the walk at :446 (actual :588), :44 :210:212,
    :82 :154:149.

Nothing from rounds 1–2 looks wrong on re-check.

Verdict: Approve after adding the Breaking callout + migration note to the
changeset; the rest is follow-up-safe.

ynadge commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

/claude-review

@github-actions

Copy link
Copy Markdown
Contributor

Follow-up review — PR #732 (round 4)

Delta since round 3 (5deea097): one commit, 222ffb49, touching only
.changeset/soft-agents-discover.md (+4 lines). No code moved. Public-copy check on the new
prose: clean — provider-neutral, no company named, no internal context.

Round-3 finding — fixed. The changeset now carries a Breaking: callout for
basis: "static""static-invocation" and a Migration: line naming the affected
endpoint. That is the right level of detail for a body-level breaking note on a minor
bump, per the repo's changeset convention.

New

None.

Not fixed from earlier rounds (all follow-up-safe, none affect published copy or behavior)

  • R1 update SDKs to use payment protocol data #2admittedObservedStat (packages/harness/src/core/workspace-watcher.ts:331)
    still re-lstats the whole ancestor chain per probe, up to 10,000 probes per pass, on
    every 250 ms debounce and 2 s poll.
  • R1 langchain SDK config cleanup #3 — stale JSDoc persists: workspace-watcher.ts:169-170 still says the sync form
    exists for "the session watcher, whose constructor requires an immediate baseline"; the
    constructor uses the async form. watchBudget() (:68-69) still says "this walk is
    synchronous". snapshotWorkspaceWorkflows still has no production caller.
  • R2 new — stale doc line refs in packages/harness/docs/agent-discovery.md: :36
    cites inspectAgentProjectMarkerSync at :237 (actual :293), :67 cites the walk at
    :446 (actual :588), :44 :210:212, :82 :154:149.

Nothing from rounds 1–3 looks wrong on re-check.

Verdict: Approve — the round-3 blocker is resolved and the remaining items are
follow-up-safe.

@ynadge
ynadge force-pushed the yashnadge/sap-2956-discover-unlinked-agents-and-reconcile-studio-inventory branch from 222ffb4 to d377464 Compare August 31, 2026 07:56
@ynadge
ynadge changed the base branch from yashnadge/sap-2955-define-the-package-inventory-contract-and-migrate-the-v0 to codex/sap-2955-package-inventory-replacement August 31, 2026 07:56

ynadge commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

/claude-review

@github-actions

Copy link
Copy Markdown
Contributor

Follow-up review — PR #732 (round 5)

Delta since round 4 (222ffb49, now fe17150e after a restack): one commit,
test(harness): cover composed graph cache state — comment rewrites in definition-name.ts /
system-graph-store.ts, Prettier formatting, and test changes. Public-copy check: clean; the
changeset is untouched and no new prose surface ships.

New

  • The tree was red at rounds 3–4, and the PR body's Validation block still reports the
    pre-rename numbers.
    At fe17150e, src/core/system-graph.test.ts:1340 called
    relationshipProvider(async () => EMPTY_RELATIONSHIPS) — both identifiers were deleted by the
    round-3 rename commit (invocationProvider is defined at :46), so that test threw and
    typecheck could not have passed. src/server/system-graph-freshness.test.ts:321 likewise
    asserted state: "ready" for POST …/system-graph/refresh where the code now returns
    degraded; this commit flips it with no production code change, i.e. the assertion was failing.
    Both are fixed here, so nothing broken ships — but "171 files, 2,968/2,968 passed / typecheck
    passed" in the PR body was measured against a tree two commits back and should be re-run and
    updated before merge. (I could not execute the suite in this sandbox; the claim rests on the
    deleted-identifier evidence above.)

Not fixed from earlier rounds (all follow-up-safe, none affect published copy or behavior)

  • R1 update SDKs to use payment protocol data #2admittedObservedStat (src/core/workspace-watcher.ts:331) still re-lstats the
    whole ancestor chain per probe, up to 10,000 probes per pass, on every 250 ms debounce and 2 s poll.
  • R1 langchain SDK config cleanup #3 — stale JSDoc persists: workspace-watcher.ts:169-170 ("the session watcher, whose
    constructor requires an immediate baseline") and watchBudget() (:68-69, "this walk is
    synchronous"); snapshotWorkspaceWorkflows still has no production caller.
  • R2 — stale doc refs in docs/agent-discovery.md: :36 :237:293, :44 :210:212,
    :67 :446:588, :82 :154:149.

Nothing from rounds 1–4 looks wrong on re-check. New discoveryComplete field is internal
(AgentInventoryResult, not exported from the package entry) with one implementer, and
degraded-after-refresh is already documented (docs/workspace-system-graph.md:160).

Verdict: Approve after re-running and updating the Validation block; the remaining items are
follow-up-safe.

ynadge commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

/claude-review

@github-actions

Copy link
Copy Markdown
Contributor

Follow-up review — PR #732 (round 6)

No delta. The PR head is still d377464c — the exact sha reviewed in round 5. No commits,
no code, no prose moved. Nothing new to review; the public-copy verdict from round 5 stands
unchanged.

Correction to round 5

Round 5's only new finding said the Validation block "was measured against a tree two commits
back". The body now reads Verified on the published PR head d377464cbf30…, which is the
current head, so that item reads as addressed. The underlying observation was still real —
system-graph.test.ts referenced identifiers deleted by the round-3 rename until this commit
fixed them — but I can't execute the suite in this sandbox to independently confirm the numbers
now attached to d377464c.

Still open from earlier rounds (all follow-up-safe; none affect published copy or behavior)

  • R1 update SDKs to use payment protocol data #2admittedObservedStat (packages/harness/src/core/workspace-watcher.ts:331)
    re-lstats the whole ancestor chain per probe, up to 10,000 probes per pass, on every 250 ms
    debounce and 2 s poll.
  • R1 langchain SDK config cleanup #3 — stale JSDoc: workspace-watcher.ts:169-170 still says the sync form exists for
    "the session watcher, whose constructor requires an immediate baseline" (the constructor uses
    the async form); watchBudget() (:68-69) still says "this walk is synchronous".
    snapshotWorkspaceWorkflows still has no production caller.
  • R2 — stale doc refs in packages/harness/docs/agent-discovery.md (re-verified against the
    current file): :36 :237:293, :44 :210:212, :67 :446:588, :82
    :154:149.

Verdict: Approve — unchanged head, round-1/2 leftovers are all follow-up-safe.

Base automatically changed from codex/sap-2955-package-inventory-replacement to main August 31, 2026 08:39
ynadge added 5 commits August 31, 2026 08:44
Narrow the V0 relationship provider to its actual direct-call semantics and make invocation edges explicit before package-level data-flow evidence is introduced.

Refs: SAP-2956
Call out the released system-graph wire value change and give consumers an explicit migration step.

Refs: SAP-2956
Keep identity settlement independent from workspace discovery completeness, preserve background invocation refresh semantics, and format the restacked integration.

Refs: SAP-2956
@ynadge
ynadge force-pushed the yashnadge/sap-2956-discover-unlinked-agents-and-reconcile-studio-inventory branch from d377464 to f008f5b Compare August 31, 2026 08:47

ynadge commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

/claude-review

let canonicalCandidateRoot: string;
let canonicalWorkspaceRoot: string;
try {
canonicalCandidateRoot = await fs.realpath(absoluteRoot);
let canonicalWorkspaceRoot: string;
try {
canonicalCandidateRoot = await fs.realpath(absoluteRoot);
canonicalWorkspaceRoot = await fs.realpath(absoluteWorkspaceRoot);
const raw = await fs.readFile(path.join(dir, "package.json"), "utf8");
const pkg = JSON.parse(raw) as { name?: string };
if (pkg.name) return pkg.name;
const initialDirectory = await fs.lstat(dir);
if (!initialDirectory.isDirectory() || initialDirectory.isSymbolicLink()) {
return fallback;
}
const canonicalDirectoryPath = await fs.realpath(dir);
return fallback;
}
const canonicalDirectoryPath = await fs.realpath(dir);
const initial = await fs.lstat(packagePath);
let raw: string;
try {
const opened = await handle.stat();
const beforeReadDirectory = await fs.lstat(dir);
try {
const opened = await handle.stat();
const beforeReadDirectory = await fs.lstat(dir);
const beforeReadCanonicalDirectory = await fs.realpath(dir);
const opened = await handle.stat();
const beforeReadDirectory = await fs.lstat(dir);
const beforeReadCanonicalDirectory = await fs.realpath(dir);
const beforeReadPath = await fs.lstat(packagePath);
offset += bytesRead;
}
const finalHandle = await handle.stat();
const finalPath = await fs.lstat(packagePath);
): Promise<CanonicalDirectoryEvidence> {
const absolute = path.resolve(input);
try {
const key = await fs.realpath(absolute);
@github-actions

Copy link
Copy Markdown
Contributor

Follow-up review — PR #732 (round 6)

No content delta. Head moved d377464cf008f5b6, but that is a restack: same five
commit subjects, same merge base (917c9306), same 53 files / 15,354 insertions / 1,591
deletions as the round-5/6 snapshot. The force-push dropped d377464c from the branch, so I
could not diff the two trees byte-for-byte in this sandbox — the equivalence above is
inferred from commit subjects and diffstat, not proven.

Public-copy check re-run on the current prose: clean. .changeset/soft-agents-discover.md is
unchanged and still carries the Breaking: / Migration: pair for
basis: "static""static-invocation". No company named, no internal context.

New

None.

Still open from earlier rounds (all follow-up-safe; none affect published copy or behavior)

  • R1 update SDKs to use payment protocol data #2admittedObservedStat (packages/harness/src/core/workspace-watcher.ts:331)
    still walks and lstats the full ancestor chain per probe, up to
    WORKFLOW_SOURCE_OBSERVATION_MAX_PROBES = 10_000 probes per pass, on every 250 ms debounce
    and 2 s poll. No per-pass ancestor cache.
  • R1 langchain SDK config cleanup #3 — stale JSDoc persists verbatim: workspace-watcher.ts:169-170 still says the
    sync form exists for "the session watcher, whose constructor requires an immediate
    baseline" (the constructor uses the async form); watchBudget() (:68-69) still says
    "this walk is synchronous". snapshotWorkspaceWorkflows (:180) still has zero production
    callers — only workspace-watcher.test.ts and agent-project-scan.perf.test.ts.
  • R2 — stale doc refs in packages/harness/docs/agent-discovery.md, re-verified against
    the current source: :36 cites inspectAgentProjectMarkerSync at :237 (actual :293),
    :44 :210:212, :67 cites walkAgentProjectTree at :446 (actual :588), :82
    :154:149.

Nothing from rounds 1–5 looks wrong on re-check. The round-5 Validation-block item now reads
addressed — the body cites f008f5b6, the current head — though I still cannot execute the
suite here to confirm the attached numbers. The lone surviving basis: "static"
(web/src/lib/system-graph.test.ts:39) is the rejection regression, which is correct.

Verdict: Approve — restack only; the three leftovers are follow-up-safe.

ynadge commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

/claude-review

@github-actions

Copy link
Copy Markdown
Contributor

Follow-up review — PR #732 (round 7)

Delta since round 6 (f008f5b6): one commit, 8cb843cb — internal Error message copy
(Workflow …Agent …) in canvas-cache.ts, workflow-registry.ts, server/index.ts,
plus two occurrence-count fixes in scripts/agent-studio-terminology-allowlist.json.
Public-copy check on the new prose: clean — no company named, no internal context, no
changeset or published-doc surface touched. No behavior change: nothing matches these
messages by text (workflow-registry.test.ts:475 asserts /superseded/, still matching),
and no old string survives anywhere in the tree.

New

  • Nothing blocking. Worth recording: the two allowlist counts this commit fixes were
    wrong at f008f5b6server-workflow-change-event declared 3 against 1 actual code
    occurrence (server/index.ts:1892; the other three are // comments, which the checker
    does not scan) and web-api-workflow-change-event declared 2 against 3. auditSources
    fails on any used !== occurrences mismatch (agent-studio-terminology-check.mjs:476),
    so pnpm terminology:check could not have passed at the sha the round-6 body claimed it
    did. Both counts are correct at head. This is the third round where the Validation block
    was attached to a tree that could not have produced it (round 5: deleted identifiers in
    system-graph.test.ts); re-run the block against 8cb843cb before merge rather than
    editing the sha.

Still open from earlier rounds (all follow-up-safe; none affect published copy or behavior)

  • R1 update SDKs to use payment protocol data #2admittedObservedStat (src/core/workspace-watcher.ts:331) still lstats
    every ancestor segment twice (dir + .git) per probe, uncached, up to
    WORKFLOW_SOURCE_OBSERVATION_MAX_PROBES = 10_000 probes per pass.
  • R1 langchain SDK config cleanup #3 — stale JSDoc verbatim: workspace-watcher.ts:169-172 ("the session watcher,
    whose constructor requires an immediate baseline" — it uses the async form) and
    watchBudget() (:69-70, "this walk is synchronous"); snapshotWorkspaceWorkflows
    still has no production caller.
  • R2 — stale doc refs in docs/agent-discovery.md, re-verified: :36 :237:293,
    :44 :210:212, :67 :446:588 (and :479:629), :82 :154:149.

Nothing from rounds 1–6 looks wrong on re-check.

Nit: production errors now read "Agent registry write was superseded" while the thrower
is WorkflowRegistry in workflow-registry.ts — an operator with the message in hand can
no longer grep to the source.

Verdict: Approve — copy-only delta, correctly scoped; re-run the Validation block at
head, and the three leftovers are fine as follow-ups.

ynadge commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

/claude-review

Comment thread packages/harness/src/core/agent-source-discovery.ts Fixed
Comment thread packages/harness/src/core/workflow-registry.ts Fixed
Comment thread packages/harness/src/core/workflow-registry.ts Fixed
Comment thread packages/harness/src/core/workflow-registry.ts Fixed
@github-actions

Copy link
Copy Markdown
Contributor

Follow-up review — PR #732 (round 8)

Delta since round 7 (8cb843cb): one commit, 9765c54d — eleven
// codeql[js/path-injection] suppression comments plus explanatory prose in
core/agent-source-discovery.ts (2) and core/workflow-registry.ts (9), and one vitest
timeout bump. No behavior change, no changeset/README/doc surface touched. Public-copy check
on the new comment prose: clean — no company named, no internal hostname or ticket, product
terms only ("Connect flow", boot token). The assertions the comments make are accurate: the
scan call sites go through resolveWithinRoot (workflow-registry.ts:613) and the third is
the explicit Connect path (:1782).

New

  • The suppressions probably don't suppress, so "resolve discovery CI findings" may resolve
    nothing.
    These are the only codeql[...] comments in the repo and there is no
    .github/workflows/codeql*.yml or .github/codeql/ config — i.e. code scanning runs under
    default setup, which does not apply CodeQL/LGTM inline suppression comments; separately,
    every comment sits on the line above its sink, and LGTM suppression semantics require the
    same line. Timing is consistent with that: a github-advanced-security review was submitted
    at 09:09:00Z, two minutes after this commit (09:07:01Z). I could not read the inline alert
    comments or check runs in this sandbox, so this is unconfirmed — but verify the alerts
    actually closed before merging. If they didn't, the only shipped effect is eleven comments
    asserting a security property that nothing enforces, and the alerts need a UI dismissal or a
    paths-ignore/query-filter config instead.

Still open from earlier rounds (all follow-up-safe)

  • R1 update SDKs to use payment protocol data #2admittedObservedStat (src/core/workspace-watcher.ts:331) still lstats every
    ancestor segment twice per probe, uncached, up to 10,000 probes per pass.
  • R1 langchain SDK config cleanup #3 — stale JSDoc verbatim at workspace-watcher.ts:169-172 and watchBudget()
    (:69-70); snapshotWorkspaceWorkflows still has no production caller.
  • R2 — stale doc refs in docs/agent-discovery.md: :36 :237:293, :44 :210:212,
    :67 :446:588, :82 :154:149.

Nothing from rounds 1–7 looks wrong on re-check.

Nit: agent-source-discovery.test.ts:1313 raises the max-modules scan test to a 20 s
timeout (4× default) in the standard unit suite rather than moving it to test:perf; it now
absorbs a real 4× slowdown silently.

Verdict: Approve once the CodeQL alerts are confirmed closed at head — the delta is
comment-only and correctly scoped, but its entire purpose is unverified.

@ynadge
ynadge force-pushed the yashnadge/sap-2956-discover-unlinked-agents-and-reconcile-studio-inventory branch from 9765c54 to d095c22 Compare August 31, 2026 09:15
Comment thread packages/harness/src/core/agent-source-discovery.ts Fixed
Comment thread packages/harness/src/core/workflow-registry.ts Fixed
Comment thread packages/harness/src/core/workflow-registry.ts Fixed
Comment thread packages/harness/src/core/workflow-registry.ts Fixed
@ynadge
ynadge force-pushed the yashnadge/sap-2956-discover-unlinked-agents-and-reconcile-studio-inventory branch from d095c22 to 9d0e29e Compare August 31, 2026 09:21

ynadge commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

/claude-review

@github-actions

Copy link
Copy Markdown
Contributor

Follow-up review — PR #732 (round 9)

Delta since round 8 (9765c54d, force-dropped from the branch): head is now 9d0e29ee,
which is 8cb843cb plus a test-only commit changing one line —
agent-source-discovery.test.ts:1313 gains a 20_000 timeout. The eleven
// codeql[js/path-injection] comments round 8 flagged are gone from the tree (grep -rn codeql over .ts/.tsx/.yml returns nothing), so the round-8 finding is resolved by
removal rather than by config. Public-copy check: no prose surface moved;
.changeset/soft-agents-discover.md is byte-identical and still carries the Breaking: /
Migration: pair for basis: "static""static-invocation".

New

  • PR body Validation block is attached to 8cb843cb, not head 9d0e29ee — fourth round
    running. The delta is one test timeout, so nothing substantive is unverified, but the block
    should be re-run at head rather than left citing a stale sha.

Still open from earlier rounds (all follow-up-safe; none affect published copy or behavior)

  • R1 update SDKs to use payment protocol data #2admittedObservedStat (packages/harness/src/core/workspace-watcher.ts:331)
    still resolves and lstats the full ancestor chain per probe, uncached, up to
    WORKFLOW_SOURCE_OBSERVATION_MAX_PROBES = 10_000 probes per pass.
  • R1 langchain SDK config cleanup #3 — stale JSDoc verbatim: workspace-watcher.ts:169-172 ("the session watcher, whose
    constructor requires an immediate baseline" — it uses the async form) and watchBudget()
    (:69-70, "this walk is synchronous"); snapshotWorkspaceWorkflows (:180) still has zero
    production callers, only tests and the perf suite.
  • R2 — stale doc refs in packages/harness/docs/agent-discovery.md, re-verified against
    source: :36 :237:293, :44 :210:212, :67 :446:588 (and :479:629),
    :82 :154:149.
  • R7 nit — errors read "Agent registry write was superseded" while the thrower is
    WorkflowRegistry; not greppable from the message.

Nothing from rounds 1–8 looks wrong on re-check.

Nit: the surviving 20 s timeout keeps a 2,001-candidate scan test in the standard unit
suite at 4× the default budget instead of in test:perf; it will absorb a real regression
silently.

Verdict: Approve — the round-8 blocker is gone with the comments; leftovers are
follow-up-safe.

@ynadge
ynadge merged commit 1107764 into main Aug 31, 2026
10 of 11 checks passed
@ynadge
ynadge deleted the yashnadge/sap-2956-discover-unlinked-agents-and-reconcile-studio-inventory branch August 31, 2026 10:19
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