Run launched agent sessions in the project's checkout with the session plugin; delete the context assembler

[malibio]

Closes #3547. Implements ADR-093 §8 and the launched-session part of §9.

A session NodeSpace launches now runs in the project's folder on this machine, with an environment that points nodespace at the database it was launched from. Nothing is written at launch: the harness plugin delivers the context, as it does for a session started from a terminal.

What changed

Working directory

• LaunchSession takes project_id, project_folder and task_id. A session launched for a project runs in that project's checkout_path.
• A launch that names a folder has it checked (absolute, an existing directory) and stored on the project, so later launches name none. A project with no folder refuses the launch until one is named.
• The app's launch form has a project picker and, for a chosen project, a folder field with a folder dialog. The folder is also editable in the project's properties.
• nodespace session launch takes --project, --folder and --task.

Environment

• The allowlist is unchanged in kind. On top of it a session gets NODESPACE_DATABASE (the database's id), NODESPACED_SOCKET, NODESPACE_SESSION (the PTY session's id) and NODESPACE_LAUNCHED_FOR (the task, otherwise the chat node). A value that is not known is left out.
• CLAUDE_CONFIG_DIR is forwarded to Claude Code when the daemon has it.

Plugin

• New ReportHarnessSession RPC and nodespace session report-harness-session <id>. The Claude Code plugin calls it at session start (and again after a /clear); the daemon writes the id to the chat node when the session ends.
• When a task was named, the plugin adds its context to the first prompt and watches it. A session launched for no task is launched for its own chat node, which the plugin does not open or watch.

Removed and fixed

• Deleted GraphContextAssembler, ContextFile, ContextError, the embedded SKILL.md and reference copies, the catalog's resume flag and session-store field, and harness_session.rs (finding the id by a unique working directory).
• Detection and launch resolve the agent binary on one search path, held by PtySessionManager. The session gets that path as its PATH.
• One set of agent ids: claude-code, codex, antigravity, pi, opencode. The daemon's antigravity-cli and open-code are gone. A test reads the skill installer's AgentName and compares it with AgentType.

Node type change

• Type: project, a flat core type (category unchanged).
• Field added: checkout_path, string, optional, local_only, user-editable. Wire name checkoutPath. It is in ProjectNode and in the typed update (set and clear).
• No relationship or rule changes.
• Docs: components/node-types.md (the project sheet). The docs commit is local in nodespace-docs and is pushed just before bun run merge, as the node type sequence requires.

Decisions to review

• Launches with no project still create a private folder at ~/.nodespace/agent-sessions/<session id>/, as before. It is still never deleted. Launches for a project create nothing.
• A harness with no plugin records no session id. Codex sessions used to get one from the directory lookup; ADR-093 §8 removes that lookup, so session_id stays empty for them.
• The harness session id is kept in memory and written at session end, as it was. A daemon that dies mid-session loses it, along with the session's end status.
• Stored agent values of antigravity-cli or open-code on existing chat nodes no longer match an agent. No migration, per the pre-release rule.

Tests

• packages/cli/tests/it/launched_session.rs: a daemon serving two databases launches a stand-in claude (a shell script on a search path the test owns) in the second one. It asserts the working directory, that nothing is written there, the environment, that nodespace node get with no flags reads the second database, that the folder is remembered, and that two sessions in one project each record their own harness session id. With the database variable removed from the launch, two of the three tests fail.
• Unit tests for the session environment, the allowlist, CLAUDE_CONFIG_DIR, the catalog and the CLI's arguments; the typed update of checkout_path; the launch form; and the plugin (claude plugin test, 39 passing, not part of the merge gate).
• bun run test:changed passes on every tier; bun run quality:fix is clean.

Not verified

• I did not launch a real Claude Code session from the running app. The orientation and first-turn context are covered by the plugin's own tests and the daemon-side environment by the end-to-end test, but not the two together in a live session.

Docs

nodespace-docs: architecture/agent-session.md, decisions/032-pty-agent-session-architecture.md (context-file, temp-directory and shim provisions marked as amended by ADR-093), components/node-types.md.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3647 (issue #3547)

Review Type: Initial Review (no prior reviews or comments on the PR; one commit, 7772c942)

Summary

The change does what the issue asks and is a clear net improvement: about 2,100 lines of dead or wrong code go (the assembler that always wrote an empty context file, the directory-based session id lookup, the resume flag), and the launch path gets a typed SessionLaunch, one search path and one agent id vocabulary. The node type change (project.checkout_path) covers every layer of the node type sequence. No blockers. Four things are worth fixing before merge; the rest is polish.

What I did not do: I read the code and docs but ran no tests, skill:check or quality:fix. The last three acceptance criteria are marked on the PR's own statement.

---

Requirements Check

Working directory

• ✅ A project launch starts in the project's folder. project_folder() resolves checkout_path; asserted end to end by CWD= in launched_session.rs and by working_dir in the response.
• ✅ First launch asks, validates, remembers; changeable in settings. The daemon refuses with FailedPrecondition, the form disables Launch until a folder is given, and the daemon checks absolute + is_dir before storing. "Changeable in the project's settings" relies on the schema-driven properties panel; no test covers that path.
• ✅ Never written where another machine reads. checkout_path is local_only, added through schema, wire type, promoted fields, generated TS, typed update (proto, daemon, Tauri, dev-proxy, adapter) and node-types.md.
• ✅ A launch with no project still works, in a private folder with the environment set (third e2e test). See finding 1 on where that folder lands.
• ✅ nodespace session launch takes --project, --folder, --task.

Environment

• ✅ Names the database and socket, with a test that proves the read. node get <task> with no flags succeeds only because the task exists in the second database alone, so the test fails if NODESPACE_DATABASE is dropped.
• ✅ Names the task or chat node (NODESPACE_LAUNCHED_FOR). See finding 3 on the cost of one variable carrying both.
• ✅ CLAUDE_CONFIG_DIR is forwarded for Claude Code and not for Codex (unit test).
• ✅ The allowlist stays an allowlist: env_clear() then named variables only; the test sets an unlisted variable and asserts it is absent.

Plugin

• ⚠️ Same orientation and skill list as a terminal session. True by construction (same plugin, same load, CLAUDE_CONFIG_DIR forwarded), but not verified in a live session, as the PR says.
• ✅ First turn has the task's context when a task was named (plugin test: delivered once, then the task is watched).
• ✅ The plugin reports the harness id at session start and it is stored at exit (e2e asserts session_id on the chat node).
• ✅ Two sessions in one project each record their own id (e2e). See finding 2 for a case that still overwrites.
• ✅ A harness with no plugin still launches in the folder with the environment set. Nothing in the launch depends on a plugin.

Removal and fixes

• ✅ GraphContextAssembler, ContextFile, ContextError, the embedded skill copies and their daemon wiring are deleted; nothing writes CLAUDE.md or AGENTS.md. One stale comment still names context_assembly.rs (finding 7).
• ✅ harness_session.rs and find_harness_session_id are gone.
• ✅ Launch resolves on the search path detection uses (resolve_binary for both, via PtySessionManager).
• ✅ One set of agent ids. AgentType::id / from_id, CLI PossibleValuesParser from AgentType::ALL, and a test that reads the installer's AgentName. No antigravity-cli or open-code id remains outside the absence assertions (the ~/.gemini/antigravity-cli hits are a real directory name).
• ✅ The resume flag and SessionStore are removed from the catalog.
• ✅ references/cli.md documents the changed session launch and the new subcommand. skill:check not re-run by me.
• ✅ Docs updated: agent-session.md, ADR-032 amendments, node-types.md (uncommitted in nodespace-docs, to be pushed just before bun run merge). They match the code I read.
• ➖ quality:fix and test:changed: per the PR; not re-run.

---

Code Review Findings

🟡 Important

1. The private session folder ignores NODESPACE_HOME, and the new test writes into the developer's real home
📁 packages/agent/src/pty/session.rs:248-256, packages/cli/tests/it/launched_session.rs:105, 438-465

let dir = dirs::home_dir()
    .context("HOME not set")?
    .join(".nodespace")
    .join("agent-sessions")

The test sets NODESPACE_HOME to a temp directory, but the launch resolves the folder from the real home, so a_launch_with_no_project_… creates ~/.nodespace/agent-sessions/<uuid>/ on whoever runs the gate. The cleanup is a last-line remove_dir, so any earlier assertion failure leaves the folder behind. The same applies to a daemon run in isolation: it serves a temp database and still writes session folders into the user's home, which is the inconsistency assembly.rs already calls out for daemon.toml.

Recommendation: have the daemon pass the base folder (nodespace_dir()?.join("agent-sessions")) in SessionLaunch, or hold it on PtySessionManager beside the socket and search path, so the agent crate stops deciding where NodeSpace state lives. The test then needs no cleanup.
Principle: test isolation; one definition of where state lives.

2. A harness started inside a launched session overwrites the recorded id
📁 packages/skill/plugins/claude-code/hooks/register.ts:203-212, packages/agent/src/pty/session.rs:349-355

NODESPACE_SESSION is inherited by everything the agent runs. If the agent runs claude -p … from its shell tool (this repository's own scripts and evals do), that inner harness loads the same plugin, sees the variable and reports its own id. "The latest report stands", so the chat node ends up with the inner conversation's id, and the outer one is lost. NODESPACE_LAUNCHED_FOR is inherited too, so the inner session also opens with the task and sets a watch.

"Latest stands" is needed for /clear, so first-report-wins is not the fix. Options: the plugin reports only when its harness is the PTY's direct child (send the harness pid, and the daemon compares it with the child it spawned); or the plugin strips the two variables from the environment of the commands the harness runs. If neither is worth it now, state the limit in agent-session.md beside the other deliberate ones.
Principle: correctness of identity; an ambient credential should name its holder.

3. One variable for "task or chat node" makes the plugin depend on a second call succeeding
📁 packages/daemon/src/services/agent_session_service.rs:243-247, packages/skill/plugins/claude-code/hooks/register.ts:219-229

The daemon sets NODESPACE_LAUNCHED_FOR to the task, otherwise the chat node. The plugin tells them apart only by comparing with the node_id the report call answers. When that call fails (CLI older than the daemon, a transient error), chatNode is null, the comparison fails, and the plugin treats the session's own chat node as work: it injects its context and watches it. The app writes agent to that node right after launch, so the watch can then refuse tool calls with "changed under it".

Recommendation: in launchedItem, when NODESPACE_SESSION is set and the report did not answer, open with nothing. One line, plus a plugin test with the report failing. The variable can keep both meanings, as the issue asks.
Principle: fail closed; do not derive a decision from the absence of an answer.

4. A stored folder is trusted without the checks a named one gets, and the folder is stored before the launch is known to be valid
📁 packages/daemon/src/services/agent_session_service.rs:118-160, 218-236

• A folder named by the launch must be absolute. A stored one (typed update, properties panel, node update) is only checked with is_dir(), so a relative value resolves against the daemon's working directory. Apply the absolute check to both, with the same message.
• project_folder() writes checkout_path before task_id is looked up. A launch refused for an unknown task still changes the project. Validate the task first, then resolve and store the folder.
• A version conflict on that write surfaces as Status::internal; aborted or a re-read would describe it better.

Principle: validate at the trust boundary, not at one of its entrances; no side effects before validation completes.

🟢 Suggestions

5. DATABASE_ENV_VAR is defined but the CLI does not use it
📁 packages/proto/src/lib.rs:68-72, packages/cli/src/lib.rs:116
The constant's doc says it is the variable the CLI reads, but the CLI still has env = "NODESPACE_DATABASE" as a literal. Use the constant there, as --session does with SESSION_ENV_VAR. (DRY)

6. Double cast where a guard exists
📁 packages/desktop-app/src/lib/components/viewers/ai-chat-pty-session.svelte:167
nodes as unknown as ProjectNode[] bypasses the type system; isProjectNode is exported from $lib/types/project-node. nodes.filter(isProjectNode) gives the same result with a real check. The component test mocks queryNodes with the typed shape, so it would not notice a change in what the adapter returns.

7. Leftover references

• 📁 packages/skill/src/tests/installer.test.ts:251-253: the comment still lists "context_assembly.rs (PTY)" as a fourth place that enumerates the skill's files. It is three now.
• 📁 packages/agent/src/pty/session.rs:749: test still named session_dir_persists_….

8. Tests that pin less than their comments say

• 📁 launched_session.rs:459-460: LAUNCHED_FOR=\r\n cannot tell an absent variable from an empty one, which is the distinction the comment claims. Print ${NODESPACE_LAUNCHED_FOR+set} in the stand-in script.
• The session's PATH being the search path is not asserted anywhere (launch_for_test_as passes search_path: None; the script uses builtins and an absolute nodespace). Add echo "PATH=$PATH" to the stand-in and assert the test's bin leads it.
• 📁 launched_session.rs:349-354: the "again" session is never ended; harmless, but it holds a PTY until the test process exits.

9. The search path is rebuilt on every call
📁 packages/agent/src/pty/manager.rs:82-87
Outside tests the OnceLock is never set, so each launch runs path_helper twice (detection, then launch) and the two results are only equal by assumption. Either cache the first result, or resolve once in launch_session and pass it to both. Not a bug today.

• Nit: 📁 packages/agent/src/pty/session.rs:1010: NODESPACE_TEST_NOT_LISTED_3547 puts an issue number in code, which CLAUDE.md asks to avoid.
• Nit: 📁 agent_session_service.rs:108-112: checkout_path is read from the raw property bucket by name. It works; a typed read through the ProjectNode conversion would keep the field name in one place.

On the stated decisions

• Private folder kept and never deleted: acceptable; the folder is empty now that nothing is written at launch, which also means it could simply be removed when the session ends if it is still empty. Worth a line in a follow-up, not this PR. Finding 1 is about where it is created, not whether.
• No id for a harness with no plugin, id held in memory until exit, no migration of stored agent ids: all consistent with ADR-093 §8 and the pre-release rule. No objection.

Security

• The PTY environment stays an allowlist; added values are daemon-chosen names with validated or daemon-generated values. Fine.
• Graph text in the first prompt is wrapped in the nodespace-graph-data marker with the marker stripped from the body and capped at 16,000 characters, the same treatment the skill list gets, and the consent rules stay in the plugin. Fine.
• ReportHarnessSession accepts any caller on the local socket with a session id. That matches the trust model of the other session RPCs.

---

Recommendation: APPROVE

No blocker. Findings 1 to 4 are small and worth doing in this PR: 1 because the gate itself writes into a real home, 3 because it is a one-line fail-closed fix, 2 at least as a documented limit.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Review addressed

All four important findings and suggestions 5 to 9 are fixed. One nit is skipped.

Important

1. Private session folder ignored NODESPACE_HOME. The daemon now tells the launch where to create it (<NodeSpace state dir>/agent-sessions/<id>/), and the agent crate no longer reads the home directory for it. The end-to-end test asserts the folder is under the test's own NODESPACE_HOME and needs no cleanup.
2. A harness started inside a launched session overwrote the recorded id. The plugin reads NODESPACE_SESSION and NODESPACE_LAUNCHED_FOR once, keeps them in its state, and unsets them, so nothing the agent starts inherits them. The report names the session with --session. A harness with no plugin still leaves them in place; nothing there reads them.
3. A failed report made the plugin treat the chat node as work. With no answer to the report the plugin opens with nothing. New plugin test.
4. Stored folder and write order. A stored folder gets the same checks as a named one (absolute, an existing directory). The task is validated before the folder is stored, so a refused launch changes nothing. A version conflict on the write is ABORTED, not INTERNAL. New tests for the relative stored folder and for an unknown task storing nothing.

Suggestions

5. The CLI's --database reads its variable name from nodespace_proto::DATABASE_ENV_VAR.
6. The launch form filters with isProjectNode instead of casting.
7. The stale context_assembly.rs comment and the session_dir_persists_… test name are fixed.
8. The stand-in agent prints ${NODESPACE_LAUNCHED_FOR-unset} and its PATH; the test asserts the variable is absent and that the test's bin leads the path. The extra session is terminated.
9. The search path is built once per launch and used for both detection and launch.

• The issue number is gone from the test variable name.

Skipped

• Reading checkout_path through the typed ProjectNode conversion. The handler reads the declaring type's bucket by field name, which is how capture_service reads its fields; converting the whole node for one string adds a serialization round trip and no safety.

Checks: bun run quality:fix clean; bun run test:changed passes on every tier; plugin tests 41 passing.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: Findings 1 to 4 changed behaviour, not wording: where the session folder is created, the order of validation and the write in the launch handler, and how the plugin holds and removes the launch variables. Those deserve a second look.

[malibio]

Pragmatic Code Review: PR #3647 (issue #3547)

Review Type: Re-Review (commit bde8c300, on top of 7772c942)

Previous Review Summary: 11 recommendations (findings 1 to 9 and 2 nits). 10 addressed, 1 not addressed (a nit, skipped on purpose with a sound reason). 4 new findings, none blocking.

What I did not do: I read the diff, the surrounding code and the uncommitted docs. I ran no tests, and I did not check in a live Claude Code session that unsetting a variable through the plugin API removes it from what the shell tool's children inherit. The fix for finding 2 depends on that.

Earlier findings

#	Finding	Status	Reason
1	Private folder ignored NODESPACE_HOME	Fixed	The daemon passes nodespace_dir()/agent-sessions in SessionLaunch.session_folders; the agent crate no longer reads the home directory. The e2e test asserts the exact path under its own home and the cleanup is gone.
2	An inner harness overwrote the recorded id	Fixed in the plugin, not verified live	takeLaunch reads both variables once, keeps them in the session state and unsets them; the report passes --session. See new findings 1 and 2 for two gaps in how it is held and tested.
3	A failed report made the chat node look like work	Fixed	reportSession answers null for no answer and '' for no chat node; launchedItem returns early on null. The new test fails the report with NODESPACE_LAUNCHED_FOR equal to the chat node and asserts no context read.
4	Stored folder unchecked; folder stored before validation	Fixed	checked_folder serves both paths. The no-project check and the task lookup now run before project_folder, which holds the only write. A VersionConflict maps to ABORTED.
5	DATABASE_ENV_VAR unused by the CLI	Fixed	env = nodespace_proto::DATABASE_ENV_VAR.
6	Double cast in the launch form	Fixed	nodes.filter(isProjectNode).
7	Leftover references	Fixed	Comment says three places; test renamed.
8	Tests pinning less than they claimed	Fixed	${NODESPACE_LAUNCHED_FOR-unset} tells absent from empty; PATH={bin}: is asserted; the second session is terminated.
9	Search path rebuilt per call	Fixed	detect_agents_with_path returns the path it looked on and launch takes it.
Nit	Issue number in a test variable	Fixed
Nit	checkout_path read from the raw bucket	Not changed	Deliberate; the reason given (one string, the same pattern capture_service uses) holds.

The new e2e assertions pin what they claim. The unknown-task launch would have stored the folder under the old order, so the is_null() check fails without the fix. A stored "." passes is_dir(), so the relative-folder test fails without the absolute check.

The docs in nodespace-docs/architecture/agent-session.md match the code: the launch signature, the folder checks and their order, the private folder's location, the four variables, the plugin taking two of them out of the environment, --session, and opening with nothing when the report is not answered.

New findings

1. 🟡 Important: no test pins that the launch survives in state once the variables are gone
📁 packages/skill/plugins/claude-code/tests/register.test.ts:155-162, 430-441

The test host's env.set handler records the unset and returns without passing it on, so the mocked environment still holds both variables afterwards. The /clear test therefore passes whether or not takeLaunch reads held.launch: with that branch deleted, the second load would read the variables from the environment again. The behaviour the fix exists for (after the unset, a /clear still reports and reopens from state) is untested. I inferred this from the handler order and did not run the mutation.

Recommendation: make the handler remove the variable from the mocked environment (or pass the call on to mock.env), then check the /clear test fails with the held?.launch branch removed. Add the same for a compaction: one more report, no second opening.
Principle: a regression test must fail without its fix.

2. 🟡 Important: the variables are unset before the launch is stored
📁 packages/skill/plugins/claude-code/hooks/register.ts:205-223, 407-428, 984-995

takeLaunch unsets the variables at the top of load. The launch reaches the session state only when load returns and the caller runs update($, session, …). If anything between the two throws, quietly swallows it, the state stays null, and the next current() calls takeLaunch with no held state and an empty environment. The session is then treated as terminal-started for the rest of its life: no report, no opening. Before this commit the next prompt would have read the variables again and recovered. run catches its own errors, so this needs a throw from $.session.id(), $.clock.now() or an update, or the host cancelling the hook mid-load: unlikely, but silent and permanent when it happens.

A related ordering point: held.launch wins over the environment. If session state is ever restored into a new harness process (a resume in a new PTY), the old PTY session id would be reported and the new variables never unset. I did not check whether state persists that way.

Recommendation: read the environment first and fall back to held.launch; unset only after the state holding the launch is written (split the unset out of takeLaunch and call it after update($, session, () => loaded) at both call sites).
Principle: make the durable write before the destructive one.

3. 🟢 Suggestion: a session that cannot reach the CLI or daemon at start never opens with its task unless the user clears
📁 packages/skill/plugins/claude-code/hooks/register.ts:320-334, 416-422

Not introduced by this commit, but now more visible. When nodespace --version or diagnostics fails at start, load returns with the launch held and stale: null, so nothing is read again until a compaction or a /clear. A compaction reports the id but passes isOpening: false, so the task's context is never delivered; only a /clear brings it. The daemon launched the session, so this needs a nodespace binary missing from the session's PATH or a daemon restart. If it is worth handling, keep a "not yet opened" marker so the first load that reaches the daemon opens. Otherwise add one line to agent-session.md.

4. 🟢 Nit: the spawn now runs on the async executor
📁 packages/agent/src/pty/manager.rs:109-116

With the search path passed in, launch dropped its spawn_blocking, so resolve_binary, create_dir_all, openpty and the fork run on a Tokio worker. That matches main before this PR and each is short; noted only because the first commit had moved it off the executor.

Security

No change to the trust model. The private folder is created under the daemon's owner-only state directory. Removing the two session-naming variables from the harness's environment narrows what child processes hold. NODESPACE_DATABASE and NODESPACED_SOCKET stay, as intended.

Recommendation: APPROVE

Every earlier finding is fixed or deliberately declined. New findings 1 and 2 are small, sit in the plugin (outside the merge gate), and are worth doing in this PR: 1 because the fix for finding 2 is currently unpinned, 2 because it is a reorder of two steps. The unverified part remains the one the PR already states: no live Claude Code session was launched, which is also the only way to confirm the unset reaches the shell tool's children.

[malibio]

Automated pragmatic-code-review (re-review): APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Re-review addressed

Fixed

1. The state fallback was unpinned. The test host now holds its own environment, so an unset variable is gone for later reads. The /clear test and a new compaction test (one more report, no second opening) both fail with the held.launch fallback removed; I ran that mutation.
2. Variables were unset before the launch was stored. readLaunch reads the environment first and falls back to the session's state; dropLaunchVariables runs only after the state holding the launch is written, at both call sites. A load that throws now leaves the variables for the next one, and state carried into another process never outranks what that process was launched with.
3. The spawn ran on the async executor. PtySessionManager::launch runs the launch under spawn_blocking again.

Documented, not changed

3. A session that cannot reach the CLI or daemon at start opens without its task until a /clear. This predates the commit and needs a missing nodespace binary or a daemon restart. agent-session.md now states it and what the agent does meanwhile (nodespace node context <id>). A "not yet opened" marker is worth adding if it shows up in use.

Checks: bun run quality:fix clean; bun run test:changed passes on every tier; plugin tests 42 passing.

Still not verified: no live Claude Code session was launched from the app, so it is unconfirmed that unsetting a variable through the plugin API removes it from what the shell tool's children inherit.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: The changes are a reorder of two steps in the plugin, a test host that models the environment, one spawn_blocking, and a docs line. The plugin change is covered by tests that fail without it.