Skip to content

feat(harness): unify project agents and restore map navigation - #804

Closed
ynadge wants to merge 3 commits into
mainfrom
conductor/sap-3148-unified-project-agents
Closed

feat(harness): unify project agents and restore map navigation#804
ynadge wants to merge 3 commits into
mainfrom
conductor/sap-3148-unified-project-agents

Conversation

@ynadge

@ynadge ynadge commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Primary change type

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

Problem and motivation

Agent Studio treated the project map as a planner-owned workspace and persisted planner/builder origins as authorization-bearing identity. That made Plan Agents behave unlike every other session, coupled a map click to session lifecycle, and left the new-project map seed tied to a planner-only greeting path.

SAP-3148 establishes the role-neutral foundation for the replacement stack: every project session is an ordinary writable coding session with the same server-derived project scope, shared Agent Map tools, and project-agent guidance. The project name now opens the durable production Agent Map without touching a session, while every tab opens its exact conversation and Canvas/Steps experience.

Summary and scope

  • Replaced live planner/builder authorization with ProjectAgentSession { projectId, userId, sessionId }, re-derived from trusted project roots on create and resume.
  • Added a tolerant one-way migration for legacy planner/manual session metadata. Valid records keep their session/provider linkage, cwd, title, transcript, and canvas; malformed or conflicting authority fails closed without deleting or duplicating the session.
  • Composed one common project-agent appendix on top of the ordinary served writable coding prompt for every project session. All such sessions retain the same E2 agent_map_read, agent_map_validate, and agent_map_propose surface and existing capability rotation/revocation/expiry checks.
  • Moved Agent Map navigation to the project name, removed the pinned planning row and planner-only tab filtering, and made Plan Agents the initial title of the first ordinary session only.
  • Added a durable project-keyed bootstrap outbox/coordinator with first-root admission, readiness and model-turn timeouts, unique correlated attempts, bounded retry/restart recovery, tombstones, empty-map checks, and real-input preemption. The visible evidence-first turn uses the existing structured Agent Map tools and cannot block a direct user build request.
  • Mapped terminal input isolation consistently to the existing retryable 409 contract on both the canonical input route and the bounded compatibility route, with no duplicate dispatch.
  • Shared the canonical project-root derivation between server and browser so project scope, session tabs, and navigation agree for descendants and deterministic multi-root projects.
  • Preserved one tab per live session ID, cross-project selection containment, per-agent Canvas/Steps, and fix(harness): keep new-agent prompts in builder sessions #802's merged explicit prompt/session ownership guarantee.

Intentionally out of scope: build-plan version contracts, neutral persisted proposal actors, focused briefs, new delegation orchestration, legacy PR disposition, and any approval/consent/mode gate. The existing E2 ProposalActor persistence shape remains readable and is deferred to SAP-3149 as required.

Related work

Related issue or discussion: SAP-3148 — Unify project agents and restore map navigation

Successor stack: SAP-3149 -> SAP-3150 -> SAP-3151 -> SAP-3152. Each successor PR must target its frozen predecessor feature branch rather than main.

Current review pair: base 73e7f46c0aa3044bba0f2d43bedfeb9d94469e02, head 5d00c55925d1907670bed6e885384b25eb775a73.

Validation

Fresh focused convergence evidence on the exact source committed as head 5d00c55925d1907670bed6e885384b25eb775a73:

pnpm --filter @sapiom/harness exec vitest run src/server/rest.test.ts src/server/agent-map.test.ts src/server/terminal-ws.test.ts --reporter=verbose — passed: 3 files / 120 tests
pnpm --filter @sapiom/harness typecheck — passed
pnpm --filter @sapiom/harness lint — passed
git diff --check — passed

The complete local publication matrix below passed on predecessor head b931e79898c22b2d56ea00a9e92ccaf9c67048ed. The focused lifecycle and route/ingest convergence suites additionally passed on predecessor head 036a73c68667af86742ffc63726b69dd70144894 (2 files / 233 tests with zero unhandled errors, and 5 files / 148 tests respectively). Exact-head hosted CI and review are rerunning for the bounded route fix; historical results are not presented as fresh evidence for the current head.

pnpm --filter @sapiom/harness test — passed: 207 files / 3,377 tests; performance suites 3 files / 10 tests
pnpm --filter @sapiom/harness test:ui — passed: 500/500 Playwright tests
pnpm --filter @sapiom/harness test:canvas — passed: 11/11 tests
setpriv --inh-caps=-all --ambient-caps=-all --bounding-set=-all --no-new-privs pnpm test — passed: all 18 workspace package test tasks
pnpm typecheck — passed: 18/18 workspace projects
pnpm lint — passed: zero errors (only existing warnings)
pnpm build — passed: 18/18 workspace projects
pnpm terminology:check — passed: 512 files
pnpm terminology:check:test — passed: 10/10 tests
pnpm provider-copy:check — passed: 128 files
pnpm provider-copy:check:test — passed: 6/6 tests
pnpm examples:check — passed: 12 templates/manifests
pnpm examples:check:test — passed: 165/165 tests
pnpm examples:test — passed: 22 passed, 0 failed, 12 examples without suites
pnpm pr-labeler:check — passed: 21/21 tests and workflow/template formatting
pnpm pr-ci-security:check — passed: 3/3 tests and workflow formatting
pnpm --filter "@sapiom/harness-desktop..." build — passed: 9-project dependency build
node packages/harness-desktop/scripts/pack.mjs --linux — passed: AppImage and DEB produced
packages/harness-desktop/scripts/smoke.sh — passed: 13 checks, 1 Windows-only check skipped
git diff --check origin/main...HEAD — passed
pnpm exec prettier --check <all changed source files> — passed

The sandbox user carries an ambient filesystem-read capability that invalidates one permission-denied fixture in @sapiom/agent-core; the complete root test gate was therefore rerun with ambient/inheritable/bounding capabilities cleared, matching normal CI filesystem semantics. No agent-core source changed. Linux packaging required the Amazon Linux workflow equivalents libxcrypt-compat (for libcrypt.so.1) and xorg-x11-server-Xvfb; packaged smoke then completed normally.

Tests and documentation

Added/rewrote unit, integration, migration, UI, and browser coverage for neutral identity equality, legacy resume and malformed migration, common prompt/tool discovery, capability lifecycle and project containment, deterministic root scope, ordinary create/resume/adopt/input, bootstrap admission/readiness/retry/restart/tombstones/user races, a real scripted MCP bootstrap (read -> validate -> propose) with exactly-once restart/replay assertions, project-name/session-tab navigation, one-tab-per-session, direct-build ownership, Canvas/Steps, and the #802 explicit-prompt regression.

Updated the harness README for the neutral identity, navigation, bootstrap lifecycle, and bounded compatibility aliases. UI behavior is covered by the 500-test Playwright run; no separate recording was captured in this cloud workspace.

Compatibility and release impact

  • Breaking or externally visible changes: Project Agent navigation changes as intended: the project name opens the Agent Map and Plan Agents is an ordinary first session tab. Valid planner-era session metadata migrates in place. The planner-named HTTP session/message/retry routes, TypeScript service/coordinator exports, and legacy planner-state directory reader remain bounded aliases into neutral services for rolling persisted-client compatibility; removal is deferred to SAP-3152.
  • Changeset: Added for @sapiom/harness (minor) and @sapiom/harness-desktop (patch).

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.

OpenAI Codex implemented and reviewed the change with bounded parallel research/review agents. Every reviewer was read-only; findings were reproduced and fixed before publication. Verification included the complete package, browser, build, lint, typecheck, documentation/copy, workflow-policy, Linux packaging, and packaged-smoke gates listed 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, 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

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Review — PR #804 (feat(harness): unify project agents and restore map navigation)

1. GET /api/state can spawn sessions in a loop when a bootstrap session dies early

listWorkflowsannotateStudioSelectionsconvergeReconciledProjectLifecycle
ensureProjectFirstSession (src/server/index.ts:3432, :3115, :2845-ish). That chain runs on
every GET /api/state and GET /api/workflows, so two read-only endpoints now create PTY sessions
as a side effect.

needsProjectSession (src/core/planner-greeting.ts:814) treats a claimed intent as replaceable
whenever the target session is exited with agentSessionId === null, and explicitly clears the
in-memory provisionalProjectClaims guard in that case. There is no bound on replacements —
MAX_RETRIES only bounds prompt attempts inside one claimed session (:1539).

Failure scenario: a new project's Plan Agents session spawns, but the agent binary exits before
the SessionStart hook posts an agentSessionId (unauthenticated claude, crash on startup, bad
config — AdapterNotFoundError/ExternalHarnessError are caught at :2870, this case is not).
The session exits with agentSessionId: null. The next /api/state read returns needsProjectSession = true and spawns a replacement; that create broadcasts an event, the SPA refetches state, and the
cycle repeats. The user sees an ever-growing tab strip of dead sessions and repeated process spawns
until they remove the project.

Needs either a durable attempt counter on the intent (mirroring retryCount) or a rule that a
replacement is only minted from an explicit lifecycle boundary, not from a read path.

2. Breaking public-type change shipped as minor with no migration note

HarnessSession is public (src/index.ts:6 re-exports all of shared/types.ts).
agentMapIdentity narrows from PlanningSessionIdentity (union carrying role and assignment)
to ProjectAgentSession ({projectId, userId, sessionId}), planning is deprecated and deleted
from sessions.json on first boot (src/core/session-manager.ts:901), and
PlannerSessionMetadataResponse.metadata changes shape and gains | null. A consumer reading
session.agentMapIdentity.role stops compiling; one reading it at runtime gets undefined.

.changeset/unified-project-agents.md is one sentence at minor with no Breaking heading and
no migration note. Per the repo's changeset policy this needs the breaking callout naming the
dropped role/assignment fields, the planning removal, and the deprecated planner routes. The
changeset is the one surface that can't be edited after publish.

Also missing from the docs: CreateSessionRequest.initialUserInputPending is a new public request
field (src/shared/types.ts:1108, validated in rest.ts:97) with no README coverage.

3. onRecentDirAdded fires for every recent dir, not just new ones

rest.ts:423 iterates the whole submitted recentDirs array; the option's own JSDoc two hunks
earlier says "Called only for roots newly added to the user's durable project list." The SPA
PATCHes the full capped list on every project open (web/src/lib/use-harness-state.ts:1503,
cap 8), so one click runs up to 8 sequential initializeOpenedProject passes. Each does a full
studioWorkspaceScopeCatalog.list() (stat every candidate root plus identity resolution for every
durable project) and a catalog reconcile, all serialized on projectScopeResolutionQueue — the same
queue that gates resolveAgentMapIdentity, so session creation stalls behind it. The PATCH response
is held until all of them finish.

Worse, the settings write has already committed before the loop. Any error that escapes
ensureProjectFirstSession — e.g. SpawnTargetError on Windows, which is not in the caught set at
src/server/index.ts:2870 — turns a durable, successful settings save into a 500.

The inline comment justifies the fan-out as retry convergence, but three idempotent convergence
points already exist (boot outbox recovery, convergeReconciledProjectLifecycle, onRootBound).
Filter to newly-added roots, and wrap the callback so lifecycle failure can't fail the settings
response.


Verdict: Request changes — finding 1 is a user-visible session-spawn loop reachable from a
read-only endpoint; findings 2 and 3 are a mislabeled public-API break and a hot-path contract
violation. The confidentiality/public-copy surfaces (changeset text, README, new prose) are clean:
no non-Sapiom company names, business arrangements, or internal hostnames. No dependency,
files/exports, or tarball changes.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — PR #804 (delta b931e798036a73c6)

Earlier findings

  1. Session-spawn loop from GET /api/state — fixed. needsProjectSession
    (src/core/planner-greeting.ts:1391) now only returns true when the target session row is
    absent and the provisional claim has been released, so an exited session with
    agentSessionId: null can no longer mint an unbounded chain of replacements.
  2. Breaking public-type change shipped as a bare minor — fixed.
    .changeset/unified-project-agents.md now carries the Breaking for embedders callout,
    names the dropped agentMapIdentity.role/.assignment, the planningprojectBootstrap
    migration and the metadata: null alias shape, and gives a migration paragraph;
    initialUserInputPending is documented in the harness README.
  3. onRecentDirAdded fan-out — fixed. rest.ts:432 diffs the committed list against the
    prior one and only calls back for newly-added roots, and each call is wrapped so a lifecycle
    failure can no longer turn a durable settings write into a 500. Both sides of the diff come
    from sanitizeRecentDirs, so the comparison is normalized-vs-normalized.

New finding

SessionInputIsolationError is unreachable by any HTTP status mapping — a fenced composer
returns 500 instead of a retryable 409.
The new error (src/core/session-manager.ts:93,
thrown at :1639, :1670, :1762) carries code: "SESSION_INPUT_ISOLATION_REQUIRED", but
neither rest.ts:948 (POST /api/sessions/:id/input) nor sendProjectSessionError
(agent-map.ts:99) lists it, so it falls through to the generic handler. Failure scenario: a
pty.write fails mid-line (EPIPE/EAGAIN on a dying pty), markComposerUnsafe fences the handle,
and the user's next send returns an opaque 500 — the SPA has no way to distinguish "reset and
retry" from a server fault, and the sibling case (SessionBackgroundInputPreemptedError) was
explicitly added to the 409 set in this same commit.

Corrections to the earlier round

None.

Verdict: Comment — all three earlier findings are resolved; the one new finding is a missing
error-status mapping, not a data-loss or leak. Delta prose (changeset, README, comments) is clean:
no non-Sapiom company names, internal hosts, or ticket links in shipped text. No dependency,
files, or exports changes.

ynadge commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — PR #804 (delta 036a73c65d00c559)

The push resolves the only open finding, and I found nothing new.

  • Round 2's finding — fixed. SessionInputIsolationError is now in the 409 set on both
    POST /api/sessions/:id/input (src/server/rest.ts:952) and sendProjectSessionError
    (src/server/agent-map.ts:105), with a test per route asserting the
    SESSION_INPUT_ISOLATION_REQUIRED body. The remaining throw site is write()
    (session-manager.ts:1670), which is the terminal WS path and already closes with the
    dedicated input-failure code (terminal-ws.ts:85-95); the bootstrap coordinator path
    (:1762) calls onNotSubmitted first, so planner-greeting.ts:3408 classifies it as a
    retryable pre-Enter failure. All three sites are covered.
  • No new findings. Delta is 4 lines of error mapping plus two tests; no prose, dependency,
    files/exports, or public-type changes. Test-only strings are provider-neutral with no
    company names or internal references.
  • Corrections to earlier rounds: none.

Verdict: Approve.

ynadge commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by the maintainer-authorized 15-part Agent Map review stack. This PR is closed without merging; its branch and review history remain preserved.

Replacement increments:

The complete testing branch remains fix/studio-onboarding-followups. Full stack: #819 through #834. Final hosted-review corrections and Mac journey validation are continuing on those same replacement PRs.

@ynadge ynadge closed this Sep 5, 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.

1 participant