Skip to content

feat(harness): expose scoped Agent Map planning tools - #776

Open
ynadge wants to merge 4 commits into
yashnadge/sap-3059-backend-persist-one-shared-versioned-map-proposalfrom
yashnadge/sap-3060-infrastructure-expose-scoped-agent-map-planning-tools
Open

feat(harness): expose scoped Agent Map planning tools#776
ynadge wants to merge 4 commits into
yashnadge/sap-3059-backend-persist-one-shared-versioned-map-proposalfrom
yashnadge/sap-3060-infrastructure-expose-scoped-agent-map-planning-tools

Conversation

@ynadge

@ynadge ynadge commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Expose the one shared Agent Map proposal service through a capability-authenticated Streamable HTTP MCP endpoint embedded in Studio. Every trusted project session receives the same strict read, validate, and propose tools, while Studio derives project/user/session/role/assignment identity server-side.

This PR is stacked on PR #775, which is stacked on PR #774. Its base is intentionally the SAP-3059 branch so this diff contains only SAP-3060.

Changes

  • add a direct @modelcontextprotocol/sdk harness dependency and one embedded /mcp/agent-map stateful Streamable HTTP router
  • issue digest-only 256-bit session capabilities with a sliding inactivity lease, generation rotation, revocation, bounded tombstones, and redacted lifecycle events
  • pin MCP transports to one live capability generation and close them on DELETE, rotation, session exit, principal changes, idle eviction, failed initialization, and server shutdown
  • register identical strict agent_map_read, agent_map_validate, and agent_map_propose tools for planner, planned-builder, and unplanned-builder identities
  • call the transport-neutral proposal service for pure validation and idempotent mutation, returning concise structured recovery errors without raw content
  • return a bounded project_unavailable / reread result when a capability project can no longer be resolved
  • resolve session cwd against the canonical most-specific active Studio project root and persist only trusted path-free Agent Map identity hints
  • generate owner-only Claude MCP config and inject Codex URL/token-env overrides per process, with bearer material only in private config or child env
  • publish the actual ephemeral server port before any session launch options are built
  • preserve original create/resume persistence failures when best-effort exited-state reconciliation also fails
  • add bounded tool/auth telemetry and focused planner tool-use context without proposal payloads, prompts, paths, tokens, or raw errors
  • document the endpoint, bearer lifecycle, and three project-wide tools in the public harness README
  • add package changeset and real SDK client transport plus lifecycle/wiring regression coverage

Key refinements from automated review

  • renew active capabilities on each successful authenticated resolution without weakening exit, revoke, rotate, or principal-change boundaries
  • exercise successful SDK initialize, listTools, and read through the full startServer({ port: 0 }) middleware stack before asserting exit revocation
  • close both the bound MCP server and transport when initialization fails before session registration
  • preserve the initiating SessionManager error if durable cleanup also rejects

Testing

  • ticket-focused Vitest coverage: 9 files, 248 tests
  • real SDK client initialize/list/call coverage for all three roles
  • actual port: 0, successful full-server list/read, private config mode, and exit revocation wiring coverage
  • failed-initialize resource cleanup regression coverage
  • injectable-clock active/inactive capability lease coverage
  • pnpm --filter @sapiom/harness typecheck
  • pnpm --filter @sapiom/harness lint
  • pnpm --filter @sapiom/harness build:server
  • git diff --check

Related

Checklist

  • Code follows project guidelines
  • No caller-controlled authority fields
  • No boot token, ingest token, bearer value, path, prompt, or proposal content in telemetry
  • No sidecar, confirmation tool, build tool, or legacy SystemGraph change
  • Self-reviewed
  • No merge performed

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Findings

1. Capabilities expire after 12h with no renewal path — long-lived sessions silently lose all Agent Map tools

packages/harness/src/core/agent-map-capability-registry.ts:48,81,96

expiresAt is stamped once at issue() and resolve() never extends it. Nothing re-issues
a capability except SessionManager.create() / resume(). So a Studio session left open past
DEFAULT_TTL_MS (12h — routine for a planner session) starts getting 401 on every
agent_map_read/validate/propose call, while the generated Claude MCP config file and the
Codex SAPIOM_AGENT_MAP_CAPABILITY env var still hold the now-dead token. The agent sees hard
tool failures with no in-band recovery; the only fix is exiting and resuming the session, which
is not discoverable from the error ("Agent Map capability rejected").

Either slide expiresAt forward on successful resolve(), or add a rotation trigger that
rewrites the private config/env for a live session. As written the TTL is a scheduled outage,
not a security boundary — the token is already revoked on exit, principal change and rotation.

2. internal_error / recovery: "retry" is returned for a terminal condition

packages/harness/src/server/agent-map-mcp-tools.ts:51, thrown at
packages/harness/src/server/index.ts:2705

The only error readSnapshotFor raises in production is a plain
Error("Agent Map project is unavailable") when the catalog can no longer resolve the
capability's projectId. It falls through the errorResult chain to
{ code: "internal_error", recovery: "retry" }, so the planner is explicitly told to retry a
condition that will never clear (the capability pins a projectId that is gone). Give the
unresolvable-project case its own terminal code with recovery: "reread"/"forbidden", the way
AgentMapProposalProjectError is already handled two branches up.

3. The wiring test's only HTTP assertion is non-discriminating

packages/harness/src/server/agent-map-mcp-wiring.test.ts

The test makes exactly one request to the endpoint and asserts 401 after kill(). A 401
is also what you get from a misrouted path, a stray auth middleware in front of the mount, or a
body-parser conflict — so this test passes even if the endpoint never worked. The protocol tests
in agent-map-mcp.test.ts mount mcp.router on a bare express app, so nothing exercises the
real mount at index.ts:3520 behind the full middleware stack (/api boot-token middleware,
express.json({ limit: JSON_BODY_LIMIT_BYTES }), canvas router, static/SPA fallback).

Add a successful initialize + listTools against server.port before the kill; that is the
assertion that actually protects the feature.

4. Transport and McpServer leak when initialize fails before session registration

packages/harness/src/server/agent-map-mcp.ts:143-146

The catch only cleans up when transport.sessionId is already set. If handleRequest rejects
before onsessioninitialized fires, the StreamableHTTPServerTransport and its connected
McpServer are never closed and were never entered into sessions, so they are also invisible
to maxSessions and to close()/revokeSession(). A holder of a valid capability that repeats
failing initializes grows the heap without bound. Close bound unconditionally in the catch.

5. Moving persist() inside the try in create()/resume() left a stale comment and masks errors

packages/harness/src/core/session-manager.ts:699-717 (and the equivalent in resume())

The catch comment still reads "The record was already persisted as starting above" — no longer
true now that await this.persist() is the first statement inside the try. Worse, when
persist() is the failure, the catch calls transitionExited(), which persists again; if that
rejects, its error replaces the original and throw err never runs. This is also an unrelated
behavior change riding along in a feature PR with no mention in the changeset.

6. New public HTTP surface is undocumented

packages/harness/README.md

The README documents the harness's HTTP surfaces (planner-session routes, workflow/graph
endpoints) and even carries a migration note for POST /api/sessions. This PR adds
POST/GET/DELETE /mcp/agent-map — a route deliberately outside boot-token auth — plus three
agent-facing tools, and documents none of it. packages/harness ships to npm; the endpoint and
its auth model are part of what consumers get.

Verdict

Solid capability design — digest-only storage, generation pinning, fail-closed resolution,
server-derived identity, token kept out of argv, and no path/prompt/proposal content in
telemetry. Public-copy hygiene is clean: the changeset and all new prose describe generic
roles only, no company names, no business arrangements, no internal hosts. minor is the right
level and @modelcontextprotocol/sdk is a justified runtime dep. Fix #1 and #3 before merge;
#2, #4, #5 and #6 are small and worth folding in.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — no PR content changed

Since the reviewed commit (f425a2bc) the only new commits are a merge of the base
SAP-3059 branch (c868852b, "fix(harness): clarify idempotency recovery"). It touches
.changeset/shared-proposals-persist.md, agent-map-proposal-service.ts and its test —
none of which are in PR #776's own diff. Every file this PR owns is byte-identical to
the previously reviewed state.

Earlier findings the push did NOT fix (all six)

  1. Capabilities expire after 12h with no renewal path — agent-map-capability-registry.ts:48,81,96; resolve() still never extends expiresAt.
  2. Unresolvable projectId still maps to { code: "internal_error", recovery: "retry" }agent-map-mcp-tools.ts:51, telling the planner to retry a terminal condition.
  3. agent-map-mcp-wiring.test.ts still asserts only a post-kill() 401; nothing exercises the real mount behind the full middleware stack.
  4. agent-map-mcp.ts:143-146 catch still cleans up only when transport.sessionId is set — pre-onsessioninitialized failures leak the transport + McpServer outside sessions and maxSessions.
  5. session-manager.ts:699-717 still carries the stale "already persisted as starting above" comment, and a failing persist() still lets transitionExited() mask the original error.
  6. packages/harness/README.md still documents none of POST/GET/DELETE /mcp/agent-map or the three agent-facing tools.

New findings

None. I checked the one plausible cross-boundary effect of the merge: the base now returns
recovery: "new_request" for a reused request ID, and errorResult spreads
error.conflict verbatim with no outputSchema to conflict with, so the new code flows
through unchanged. @modelcontextprotocol/sdk: ^1.26.0 matches the package's existing
caret policy. No confidentiality issues in the merged changeset text either — it moved
from a specific "latest 256 accepted batches" claim to "a bounded retry window", which is
strictly safer copy.

Corrections to round 1

None found on re-check; findings 1 and 4 were re-verified against current HEAD.

Verdict: Nothing was addressed. Findings 1 and 3 still block merge.

Renew live-session capabilities on authenticated use, return bounded project recovery, close failed MCP initializations, and preserve original session persistence failures. Extend full-server SDK coverage and document the embedded endpoint.

Refs SAP-3060

ynadge commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — all prior findings resolved

Delta since the reviewed commit (12edc582): one commit, 839cd7cb "fix(harness): harden
Agent Map MCP lifecycle" (10 files, +299/−26). It addresses all six earlier findings.

Earlier findings — status

  1. Fixed. agent-map-capability-registry.ts:105-117 now slides expiresAt forward on every
    successful resolve(), turning the TTL into an inactivity lease; exit, rotation, principal
    change and explicit revocation still hard-revoke. Covered by two injectable-clock tests
    (sliding renewal + full-inactivity expiry).
  2. Fixed. New AgentMapMcpProjectUnavailableError (agent-map-mcp-tools.ts:36) thrown from
    index.ts:2706 maps to a terminal { code: "project_unavailable", recovery: "reread" }
    instead of internal_error/retry, with a protocol-level test.
  3. Fixed. agent-map-mcp-wiring.test.ts now connects a real SDK Client to metadata.url
    through the full startServer({ port: 0 }) stack, asserts the three tool names, and asserts a
    successful agent_map_read payload before the post-kill() 401.
  4. Fixed. agent-map-mcp.ts:163-169 wraps connect + handleRequest and calls
    closeBound(transport.sessionId, bound) unconditionally; closeBound tolerates an undefined
    session id. The regression test asserts both server.close and transport.close fire when
    handleRequest rejects before registration.
  5. Fixed. session-manager.ts:709-717,873-882 — comments rewritten to match the moved
    persist(), and transitionExited(...).catch(() => {}) preserves the original error. In-memory
    repair to exited still happens synchronously inside transitionExited before the persist
    promise, so the ghost-tab guarantee holds; both paths have tests.
  6. Fixed. README.md:156-176 documents the /mcp/agent-map route, its separation from the
    /api browser-token surface, the capability lifecycle, and the three tools. Copy is generic —
    no company names, hosts, or business arrangements.

New findings

None. The two new createTransport/createToolServer test seams sit on
AgentMapMcpRouterOptions, which is not reachable from the package's public entry
(src/index.ts), so no npm API surface is widened. SessionManager is likewise not exported,
so the error-preservation change is internal and needs no changeset line. The changeset is
unchanged and still clean.

Corrections to earlier rounds

None.

Verdict: Everything raised in rounds 1–2 is addressed with tests. No blockers.

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