Skip to content

fix(harness): Studio never creates a planner session; the map is a read-only view [SAP-3143] - #837

Closed
gwitwer wants to merge 5 commits into
mainfrom
gwitwer/no-planner-sessions
Closed

fix(harness): Studio never creates a planner session; the map is a read-only view [SAP-3143]#837
gwitwer wants to merge 5 commits into
mainfrom
gwitwer/no-planner-sessions

Conversation

@gwitwer

@gwitwer gwitwer commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Primary change type

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

Problem and motivation

Selecting a project, or its Plan Agents row, opened a session automatically: a map-planner session launched behind the user's back that owned the centre pane and could not build anything (#835 unblocked what that session can do). There is no planning session type. The Agent Map is a view of durable state, and a project's sessions are ordinary sessions the user starts.

Summary and scope

  • Opening a project starts nothing. The map renders from the workspace store. The centre names the project with no running session and offers a Start that goes through the same createSessionAt path as the tab strip's plus.
  • Existing planner sessions become ordinary sessions. The registry load drops the persisted planning key, and a persisted map-planner identity is no longer honored on resume, so those sessions come back as ordinary agent-builder sessions. No data loss: the session, its cwd, and its vendor conversation id are untouched.
  • Planner routes answer 410. POST /api/projects/:id/planner-sessions and its message and greeting-retry children return planner_sessions_removed with a sentence telling a stale client to start an ordinary session. The boot-token gate still comes first.
  • The composer no longer routes a stable project through the planner. handleSelectWorkspace resolves a session for the focus first; a Studio project with none shows its map and waits.
  • Closing a tab prefers another live session in the same folder. The deleted planner fallback expressed this for planner sessions only; without it, closing a tab at map altitude jumped the conversation to an unrelated project. Caught by the re-pointed e2e spec, not by review.
  • Deleted (see below). The Agent Map store, its MCP tools, the proposal service and the renderer are untouched.

Out of scope: the Agent Map itself, proposals, and the E2 store.

deleted

File Lines
src/core/planner-greeting.ts 1489
src/core/planner-greeting.test.ts 962
src/core/planning-session.ts 614
src/core/planning-session.test.ts 817
src/profiles/agent-map-planner.ts 28

Also removed in place: the planner routes and their schemas in server/agent-map.ts, the planner wiring in server/index.ts, the three planner_session_requires_scoped_route fences in server/rest.ts, setPlanningMetadata and the trusted planning options in core/session-manager.ts, HarnessSession.planning, the planner analytics event types, the planner-only types in shared/agent-map.ts, paths.plannerSessions, the client openPlannerSession / sendPlannerMessage / retryPlannerGreeting and their mock, and the planner pane in use-agent-map-entry.ts. Net non-test change: 254 added, 3421 removed.

PlanningSessionIdentity and the map-planner actor role stay: durable proposal history records the actor that wrote it, and old rows name that role.

Related work

Related issue or discussion: SAP-3143. Follows #835 (the stopgap that made the planner session able to build). Superseded by Yash's #820 / #826 stack; the deletion list mirrors #826's.

Validation

Real server, node dist/cli/bin.js <tree> --port 4131 --state-root <scratch>, Playwright with page.on("pageerror") attached, zero page errors in every run.

Before (branch gwitwer/planner-sessions-are-ordinary): clicking Plan Agents created a session unasked. Measured through /api/state: 0 sessions at load, 1 session after the click, role map-planner.

Before: Plan Agents auto-starts a planner session

After (this branch): 0 sessions at load, 0 sessions after the click. The map is on the right, the honest empty state in the centre.

After: Plan Agents shows the map and starts nothing

After, explicit Start: one session, planning: null, agentMapIdentity.role: agent-builder, assignment.kind: unplanned. It scaffolded, typechecked and locally checked a new agent through sapiom-dev on request, and the agent appeared in the rail. Nothing was deployed.

After: the ordinary project session scaffolds an agent

pnpm install --frozen-lockfile && pnpm --filter "@sapiom/harness..." build : pass
pnpm typecheck : pass
pnpm lint (eslint src) : 0 errors, 0 warnings
pnpm test : 3261 passed, 12 failed in 6 files; the same 6 files fail on unmodified
  origin/main in a clean worktree (agent-source-discovery, workspace-watcher,
  workflow-registry, system-graph-relationships, system-graph-watcher,
  system-graph-freshness): filesystem-watcher tests, none touch sessions or the map
pnpm test:ui (VITE_MOCK=1) : final run 543 passed, 3 failed, all 3 pass on
  --last-failed (rich-step-detail x2, step-macros; known flake band). An earlier
  full run on the same tree was 546 passed, 0 failed.
real server: before/after session counts and screenshots above

Tests and documentation

agent-map-planning.spec.ts renamed to agent-map.spec.ts and re-pointed test by test: first open starts no session and the explicit Start creates exactly one, a project session keeps the ordinary chrome (Copy path and Open in editor, which the planner hid), ending the last session returns to the empty state. open-project, project-axis and new-session-composer re-pointed the same way. No test deleted except the ones whose subject no longer exists: the four rest.test.ts planner-fence cases, the two agent-map.test.ts planner-route cases (replaced by one asserting the 410), the ingest.test.ts planner-redaction case, and the api.test.ts planner-mutation case.

Documentation: changeset only.

Compatibility and release impact

  • Breaking or externally visible changes: the planner routes now answer 410. A client on an older build that calls them shows its own error rather than opening a session. Persisted planner sessions resume as ordinary sessions; nothing on disk is deleted.
  • Changeset: added, @sapiom/harness minor, marked Breaking with the migration note. HarnessSession.planning is removed from the published types; the eight planner AnalyticsEventType members stay in the union for one release, deprecated and no longer emitted (five of them shipped 0.14.0 already deprecated), and are removed in the next minor.

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 (Fable 5.1) made the change and re-pointed the tests under direction, ran the real-server verification, and drafted this body. Verified by the build, typecheck, lint, unit and mock UI suites, and the before/after measurements 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.

Review rounds

Round 1.

All three findings addressed in ba1ce3f8, plus the advisory hygiene item. The changeset now says Breaking and carries the migration note, and the deprecated analytics members stay for one release. The registry-load migration has the mutation-tested unit test described above. The map-altitude Start spinner keys to the project's own root rather than to any start in flight. A Studio project's root now comes from one shared rule, studioProjectRoot, next to mostSpecificStudioScope, so the tab strip and the Start target cannot key to different folders under nested scopes.

Round 2, verdict approve with comments. Both addressed in a49b5207. The identity predicate that runs against a user's persisted sessions on first boot is now core/project-session-identity.ts, called by the server, with its own tests: a persisted map-planner row is never honored, a server-authored planned assignment is, and a row from another session, project or principal is re-issued. It is mutation-tested against dropping the assignment.kind clause, which is the regression the review named; the earlier session-manager test asserted a hand-written mock's branch instead. mostSpecificStudioScope has its doc comment back. On the nit: the conversation subject keys to the innermost scope while the tabs key to the project root deliberately, and now says so where the root is derived.

did_not_work

  • The scaffold run stopped halfway on a Claude Code usage-limit prompt ("You've reached your Fable limit"). I answered it and the run finished; the screenshot is of the completed run, not one uninterrupted run.
  • The first re-pointed e2e run failed three specs for real reasons, not flake: the tab-close fallback jumping projects (fixed in source), a mobile assertion that assumed exactly one running session dot, and a project-axis test that assumed opening the map shows a terminal. All three are re-pointed or fixed above.
  • pnpm test is not green on this machine even on unmodified origin/main (12 to 13 watcher-test failures, count varies by run). Reported, not investigated here.
  • The migration now has a unit test that loads a 0.14.0-shaped sessions.json and asserts the session comes back ordinary and resumable, and that test fails when the delete is removed. I still have not run a real upgrade over a user's live state root: every real-server run used a fresh one.
  • The mock UI suite is not deterministic on this machine: the same tree gave 0 failures on one full run and 3 on another, all passing individually. I am reporting both runs rather than the better one.
  • ?mockGreeting and ?mockPlanner fixtures are dead now. I left the query-string handling removed with the mock rather than keeping stubs, so a stale bookmark carrying them simply opens a normal project.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DUq2aqxS5DQtG6aPvoYtn7

…ad-only view [SAP-3143]

Selecting a project or its Plan Agents row opened a session automatically:
a `map-planner` session, launched behind the user's back, that owned the
centre pane. There is no planning session type. The map is a view of durable
state, and a project's sessions are ordinary sessions the user starts.

- Opening a project or its Agent Map row reads the workspace and starts
  nothing. The centre says which project has no running session and offers a
  Start that goes through the same createSession path as the + tab.
- Sessions persisted with planner metadata load as ordinary sessions; the
  stale key is dropped on registry load, and a persisted `map-planner`
  identity is no longer honored on resume.
- The planner routes (create, message, greeting retry) answer 410 with
  `planner_sessions_removed`.
- Closing a tab now prefers another live session in the same folder, which is
  what the planner-specific fallback expressed for planner sessions only.
- Deleted: planner-greeting, planning-session, the agent-map-planner profile
  and their tests; the planner client API, its mock, and the client planning
  state. The Agent Map store, MCP tools and renderer are untouched.

Mirrors the deletions in #826, which supersedes this.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DUq2aqxS5DQtG6aPvoYtn7
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review — PR #837 (round 1)

Deleting the planner is the right call and the deletion itself is clean: the 410 route, the MCP-identity narrowing and the e2e rewrites all have tests. Three findings, ordered by severity.

1. Breaking removals from a released public surface, shipped as minor with no Breaking callout

.changeset/no-planner-sessions.md is minor and reads as a feature note. What it actually removes was published in @sapiom/harness@0.14.0 (packages/harness/CHANGELOG.md, version-packages commit f0933e5f; the planner types landed in ac9a35f3, an ancestor of it):

  • HarnessSession.planning (src/shared/types.ts:217 before this PR) — re-exported to consumers by src/index.ts:6 (export * from "./shared/types.js").
  • Eight AnalyticsEventType members (planner_session.*, planner_greeting.*) — same export path. A consumer with an exhaustive switch over AnalyticsEventType, or one that constructs an event of those types, fails to compile after a ^0.14.0 bump.
  • Three REST routes that now answer 410 rather than 404/200.

Worse for the deprecation contract: the five planner_greeting.* members carried @deprecated Compatibility-only; new planner sessions do not inject synthetic greetings. in the released 0.14.0 types. Removing them in the very next release gives consumers zero versions of deprecation window, which is the opposite of what the annotation promised.

Fix: keep the changeset level, but state Breaking in the changeset body with the migration note (drop session.planning; those analytics types are no longer emitted; the planner routes answer 410 planner_sessions_removed). If you want to honour the @deprecated markers, leave the five greeting members in the union for one release — nothing else in the diff depends on their absence, and AnalyticsEventType has no runtime allowlist that would reject them.

2. The one path that touches users' persisted state has no test

The PR's compatibility claim — "Sessions persisted with planner metadata resume as ordinary sessions" — rests on two changes with zero coverage:

  • src/core/session-manager.ts:593delete (session as {planning?: unknown}).planning at registry load, which also sets dirty and rewrites sessions.json.
  • src/server/index.ts:1178-1182 — a persisted map-planner identity is no longer honoured.

No test loads a 0.14.0-shaped sessions.json (a session with planning plus agentMapIdentity.role === "map-planner") and asserts it comes back as an ordinary, resumable session. grep planning src/core/session-manager.test.ts is empty. Every other branch of this PR got its test rewritten, so this is the gap, and it is the branch that runs against real user data on first boot after upgrade. Type-checking will not catch a regression here either, since planning no longer exists on the type.

3. startingProject is a global flag, but starts are keyed by root

web/src/App.tsx:3008 (newSessionPending) and :3144 (disabled) both use startingProject != null. startProjectSession is deliberately guarded by root (startingProjectRootsRef, :1679) precisely because two projects can start concurrently. Failure: start a session in project A from the rail, switch to project B's map before A's POST resolves — B's "Start a session" button is disabled and spinning even though nothing is starting for B. Use startingProject?.root === selectedStudioRoot.

HYGIENE (advisory)

A second derivation of "this Studio project's root". web/src/App.tsx:1348 computes selectedStudioRoot as the shortest cwd among the project's bound scopes; the same component already answers that question 38 lines earlier via mostSpecificStudioScope (web/src/lib/agent-map.ts:327) to build selectedStudioScope. With nested bound scopes the two disagree, and then the map-altitude tab strip (projectSessions), the Start target, and conversation (:1389, keyed to selectedStudioScope?.cwd) are keyed to different folders. Next step: put the project-root rule in lib/agent-map.ts beside mostSpecificStudioScope and have both call sites read it.


Verdict: Request changes — fix the changeset (#1) before merge; #2 and #3 are small and worth landing in the same push. No confidentiality problems: the changeset, the 410 copy and the new empty-state strings name no company or internal system. (Separately, the PR body is still Placeholder; full body follows once media is pushed. — the deterministic labeler will flag it.)

…he state migration

Review round 1:
1. The changeset now says Breaking and carries the migration note. The eight
   planner analytics event types stay in AnalyticsEventType for one release,
   deprecated and no longer emitted, so a consumer's exhaustive switch still
   compiles; five of them shipped 0.14.0 already marked deprecated.
2. The registry-load migration has a test: a 0.14.0-shaped session with
   planner metadata and a map-planner identity loads as an ordinary session,
   the rewrite is durable, and it resumes as an agent-builder. Mutation-tested.
3. The map-altitude Start spinner keys to the project's own root, not to any
   start in flight, so two projects starting concurrently do not disable each
   other's button.

Hygiene: one shared rule for a Studio project's root (studioProjectRoot,
beside mostSpecificStudioScope) so the tab strip, the Start target and the
conversation subject cannot key to different folders under nested scopes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DUq2aqxS5DQtG6aPvoYtn7
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review — PR #837 (round 2, follow-up)

Delta since 4ca6aa8f (one commit, ba1ce3f8). Round-1 #1 and #3 are fixed:
the changeset now says Breaking, carries the migration note and holds the
eight analytics members in the union with @deprecated (nothing emits them —
grep planner_session\. is empty outside shared/types.ts), and the map-altitude
Start now keys to startingProject?.root === selectedStudioRoot. Changeset copy is
clean: no company, no internal system, no arrangement.

1. Round-1 #2 is only half covered — the server-side half still asserts a fake

session-manager.test.ts:1700 genuinely covers the registry-load delete and the
durable rewrite. It does not cover the other change the finding named,
src/server/index.ts:1174-1183. The test hand-writes a resolveAgentMapIdentity
mock, and that mock is not the real resolver: it re-issues whenever
prior?.role !== "agent-builder", while the real one also requires
persisted.sessionId === sessionId, a matching projectId/userId, and
assignment.kind === "planned". So expect(resumed.agentMapIdentity).toMatchObject({role: "agent-builder"})
asserts the mock's branch, not the narrowing that runs on a user's first boot after
upgrade. A regression in that real predicate — e.g. dropping the assignment.kind
clause and honouring a persisted map-planner row — leaves this test green.
Lift the resolver out of the createServer closure so a test can call it, or assert
it through an existing server-level test.

2. New: the inserted function stole mostSpecificStudioScope's doc comment

web/src/lib/agent-map.ts:322-325 — the old block ("The durable Studio project
owning a path. Nested opened projects use the nearest containing root…") now sits
directly above studioProjectRoot's own block, so two JSDocs stack on the new
function and mostSpecificStudioScope has none. Hover and completion on the
function that actually answers "which scope contains this path" now show nothing.
Move the old block back down to line 349.

Nits

  • App.tsx:1385conversation still keys to selectedStudioScope?.cwd (innermost
    scope), not studioProjectRoot. The unification covers the tab strip and the Start
    target only; the body's "cannot key to different folders under nested scopes"
    overstates it. If the split is deliberate — conversation follows the focused agent's
    scope — say so where selectedStudioRoot is defined.
  • startingProject is still a single slot, so starting project B re-enables A's Start
    button mid-flight. startingProjectRootsRef prevents the double-create, so this is
    cosmetic.

Nothing from round 1 was wrong.

Verdict: Approve with comments — #2 is a one-line move; #1 is the only one worth
holding for, and it is a test-strength gap on an already-shipping-safe change, not a
defect in the code.

gwitwer and others added 3 commits September 5, 2026 16:03
Review round 2:
1. The resolver that runs against a user's persisted sessions.json is now
   core/project-session-identity.ts, called by the server, with its own tests:
   a persisted map-planner row is never honored, a server-authored planned
   assignment is, and a row from another session, project, or principal is
   re-issued. Mutation-tested against dropping the assignment.kind clause,
   which is the regression the review named.
2. mostSpecificStudioScope has its doc comment back; studioProjectRoot keeps
   only its own.

Nit: the conversation subject keying to the innermost scope while the tabs key
to the project root is deliberate, and now says so where the root is derived.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DUq2aqxS5DQtG6aPvoYtn7
…menu

The ⋮ on every project row opened a 248px card to hold one item. Its create
item is suppressed wherever the Agent Map owns creation, which is every
plan-first project, so on the product path the popover existed to carry a
single "Remove <project> from the rail" — a card, an anchor and a min-width
whose stated purpose in the CSS was fitting that one sentence on one line.

Remove is now an X beside the session shortcut, hover-revealed like every other
row action, opening the same RemoveProjectConfirm as before: the project named,
the count of running sessions it ends, and the statement that nothing on disk is
touched. The legacy create action, where a server still offers one, keeps its
own glyph rather than a menu row.

design-eng D33 (2026-09-04) already ruled this way and was not followed: "a
project row's verbs are hover actions on the header ... a per-row menu would be
a new idiom", rejecting "a ⋯ menu on every project row". Reversibility: low.

Focus survives the change rather than degrading. The menu item that opened the
confirm unmounted with its popover, so focus was released to <body>; the X is
still mounted when the dialog closes, so focus returns to the control that
opened it. dialog-shell's CreateAgentDialog case gains the trigger it could not
have before.

The collapsed-row exclusion in styles.css goes with the menu: it existed only
because a menu trigger always carries aria-expanded, which made every collapsed
project row wear a standing ⋮. Plain row actions carry no such attribute.

The hover-reveal spec now asserts the contract across every action on the row
instead of a hard-coded pair, so adding or removing a verb cannot leave one
standing at full strength unnoticed.

Verified: 544 playwright mock specs pass; web typecheck clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KQCZymBi59bqzQ1dEKQYbw
Creating an agent inside an existing project had no door left. `mapOwnsCreation`
is `studioProject != null`, and the comment above it says current servers issue
a durable Studio project for every scope — so it is true everywhere. It gated
both the row's create action and the empty project's create row, on the stated
grounds that "its pinned Agent Map is the only route to generating agents".

AgentMapPane has no create control. The route was the planner session driving
the map's MCP tools, which SAP-3143 deletes in this same branch. A project that
already holds agents was therefore left with nothing: the row offered a session
and a remove, the empty-project row never rendered, and the rail's top CTA opens
the composer with no project context, so it cannot create INTO a project.

The `+` is New agent again, which is also what the design has said since
2026-09-04 — design-eng IA.md 219 and D34(a) put `project-create-agent-{label}`
on the row itself, hover-revealed. A bare project keeps its distinct scaffold
verb, which grows an agent inside the session already running there.

`project-start-session-{label}` comes off the row. D34(e) and D35 item 6 give a
plain session to the tab strip, and SAP-3143 added the project pane's own Start
a session, which is the map-altitude path. `startProjectSession` in App.tsx is
untouched and still serves both.

The empty project still gets no create row of its own: D36 makes its Agent Map
row the CTA, and that gate is the one thing `mapOwnsCreation` still owns.

Verified: 544 playwright mock specs pass; web typecheck clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KQCZymBi59bqzQ1dEKQYbw
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