Docs: amend P5 session-isolation design (v1.1) — server.ts:174 is on the /turn path

[cwiklik]

Summary

Amends docs/specs/2026-09-06-p5-session-isolation-design.md to v1.1. Docs only — no code, no test changes. Every file:line in the spec was re-verified against main@02109bc / pi-fork@2c28be50 before writing; all citations check out except the one corrected below.

Groundwork for #239. The amendments are the facts a fresh implementation session would otherwise have to rediscover, one of which would have shipped a bypass.

The blocking correction: server.ts:174 is on the /turn path

The spec classified :174 two different ways, and both were wrong: §2.3 called it "async dispatch" and §3.5 listed it among "the non-turn buildConfig() call sites [that] keep ambient config". It is neither — it is the SSE branch of POST /turn:

• :174 sits inside handleTurnStream (:150)
• whose only caller is handleTurn:108 — :107 computes wantsStream from req.headers.accept, then if (wantsStream) return handleTurnStream(...)
• and handleTurn is the POST /turn route handler (:564)

Implemented as written, step 1 would convert only :111, and a client sending Accept: text/event-stream would keep the ambient credential and never reach the 401 — making §3.1 clause 1 false on a shipped code path while the sync two-tenant test in §5 passed green.

Step 1 must convert both :111 and :174. handleTurnStream already receives req (:153), so no signature change is needed. The genuinely non-turn sites are :411/:415 (runLeaf) and leaf-job.ts:77.

Also amended

• The bypass would fail open, not loudly. run-turn.ts:314 (if (!gatewayBase && !authToken) return baseModel;) returns the model unchanged rather than throwing, and model-gateway.test.ts:27 pins that deliberately. So removing the ambient fallbacks in step 2 adds no enforcement of its own — step 1's explicit 401 is the only thing enforcing §3.1 clause 3. §3.1 and §3.2 now say so.
• Scrub location settled — the exported startServer() (:576), not the isMainModule guard (:592-593). This decides whether §5's Sentinel identity row is assertable in-process or needs a subprocess; the cost is env save/restore in every test that starts a server.
• TurnConfig gains a subject field. Otherwise the tenant is knowable only by reverse-mapping the placeholder, which puts the logging path in contact with the credential mapping.
• The cwd probe was run — §4's "one item that could change the verdict". Verdict holds and scope does not grow, but §4's existing pin was slightly too strong, so it now states the narrower claim: SessionManager is proven inert on the backend path (sessionDir="" guards the mkdir and append off; cwd survives only as a header value), while SettingsManager does compute a cwd-derived projectSettingsPath and is merely read-only from a server turn. Two residuals are now pinned rather than assumed — projectTrusted defaults to true, and the settings lock busy-waits synchronously (10 × 20 ms) on the read path, which stalls the event loop for every multiplexed session.
• Three existing tests assert exactly what step 2 deletes and must be inverted, not updated: model-gateway.test.ts:50, plus :42 and :78 which need splitting because their ANTHROPIC_BASE_URL half survives (§3.5 keeps anthropicBaseUrl deployment-level).
• New §5 row: a two-tenant interleaved test on the SSE path. The sync-only version passes while /turn + Accept: text/event-stream leaks.

New in the second commit: cross-tenant session resumption (§6)

§6 filed this under "per-tenant sandbox or data isolation, untouched here", which is true but reads as YAGNI. It is not, and §3.1's guarantees do not cover it.

/turn takes sessionId straight from the request body (server.ts:101) and passes it to SessionManager.openFromCheckpoint (run-turn.ts:439), and the Redis keyspace is flat — session:${sid} and session:${sid}:seq (redis-backend.ts:6-7). A caller supplying another subject's session id therefore resumes that conversation and reads its history, while the upstream call carries the caller's own placeholder. So §3.1 clause 1 holds exactly as specified and the leak happens anyway; the credential invariant and the data boundary are orthogonal, which is why this needed stating.

The leaf path already closes it — leafSessionId prefixes tenant/sessionId before sanitizing (run-leaf.ts:158-159) — so the turn path lacking an equivalent is the same dual-path asymmetry §2.3 records for credentials, one layer up.

Kept in §6 because it genuinely stays out of scope, but stated as a precondition on the deployment: until a subject→session binding exists, /turn must not be reachable by mutually untrusted callers, and §7's tenancy-neutral claim assumes a gateway that has already partitioned them.

One decision still needs an owner

The spec names the subject header X-SH-Subject (§3.2) but describes the placeholder config source only as "mounted non-secret configuration" — no env var name. There is no existing X-SH-* convention in the repo to follow (the server reads only req.headers.accept), and both names become a cross-repo contract as soon as #241 / cortex#905 or a manifest references them: renaming later is a coordinated change plus a deploy window.

This genuinely gates the code — no step can land ahead of step 1, since step 3 deletes ANTHROPIC_AUTH_TOKEN which step 2's fallback still reads, so §2.3's ordering makes step 1 strictly first.

Testing

Docs only. prettier --check clean on the amended file; the pristine file was confirmed prettier-clean first, so the formatting pass reflects only these edits.

Related issue(s)

Groundwork for #239. Spec merged in #228.

Assisted-By: Claude (Anthropic AI) noreply@anthropic.com

[pdettori]

Verified every file:line in this amendment against the PR's base (main@1deacac4, which is current main) and pi-fork@2c28be50 (confirmed to match the submodule pin), fetching each file from GitHub rather than a local clone.

The central correction is right. server.ts:174 is the SSE branch of POST /turn: :174 sits in handleTurnStream:150, whose only caller is the if (wantsStream) at :108 (computed from req.headers.accept at :107), inside the POST /turn handler at :564 — and handleTurnStream already takes req at :153, so no signature change. Line 148's own comment reads "SSE representation of /turn". The v1.0 "async dispatch" label was wrong and the bypass reasoning follows. Roughly forty other citations across server.ts, run-turn.ts, model-gateway.test.ts, run-leaf.ts, redis-backend.ts, session-manager.ts and settings-manager.ts check out exactly, including all four test line numbers and their titles.

Requesting changes on one newly-introduced citation in §4 (inline): the three lines cited as "project-scope mutators" are setProjectTrusted() calls, which write nothing — the audit names a set that cannot write and omits the set that can, and residual #1 is built on it. One-line fix. Flagging it as blocking rather than a suggestion only because citation accuracy is this PR's entire deliverable, and its description claims all citations check out except the corrected one.

Two nits also inline. Docs-only, all 12 checks green, both commits signed off, Assisted-By used correctly.

[pdettori]

must-fix — this names the wrong set of functions.

All three cited lines are setProjectTrusted(...) calls (settings-manager.ts:447), which flip an in-memory boolean and clear the modified-field sets. Nothing is written to disk, so they are not project-scope mutators.

The functions that actually reach updateProjectSettings → assertProjectTrustedForWrite (:527) all live in settings-manager.ts itself:

• setProjectPackages (:942-943)
• setProjectExtensionPaths (:958-959)
• setProjectSkillPaths (:974-975)
• setProjectPromptTemplatePaths (:990-991)
• setProjectThemePaths (:1006-1007)

plus saveProjectSettings (:618-619) and the project branch of enqueueWrite (:552-553).

So the sentence names a set that cannot write and omits the set that can, which means "none reachable from a server turn" is asserted about the wrong functions. That matters here specifically because residual #1 below ("a future project-scope write would succeed, not throw") is aimed at exactly those write paths — as written it points the next session at package-manager-cli.ts instead of the five setProject* methods, in the section whose stated purpose is that a clean session not rediscover this.

One detail worth folding in while you are here: the three trust-flag calls you cite are unreachable from a server turn, but for a reason the spec does not give — resource-loader.ts:328/:338 are only reached via reload({ resolveProjectTrust }) (resource-loader.ts:335-338), and run-turn.ts:493 calls reload() with no options, so that branch never runs.

I could not settle whether the five real writers are turn-reachable: GitHub code search returns 0 hits for kagenti/pi even on definitions that demonstrably exist, so the repo is not indexed and I am not going to assert it either way. Re-running that audit against the five setProject* methods is the ask.

[cwiklik]

Fixed in a982ffd, and the audit you asked for came back your conclusion, my wrong evidence — so the section's claim stands but on a different set.

You're right that all three cited lines are setProjectTrusted(...) (settings-manager.ts:447), which flips projectTrusted and clears the modified-field sets without touching disk. The spec now names the writers that do reach updateProjectSettings (:635) → assertProjectTrustedForWrite (:527): your five setProject* methods (:942, :958, :974, :990, :1006), plus saveProjectSettings (:618, reached at :640) and the "project" branch of enqueueWrite (:552-553).

Re-ran the reachability audit against those five, in the pinned fork (pi-fork@2c28be50), by tracing callers rather than by search. They have exactly two non-test caller sites:

1. The interactive TUI — config-selector.ts:484/:486/:488/:490/:558, under src/modes/interactive/. Server mode never enters it.
2. pi packages — package-manager.ts:798/:806/:824, inside addSourceToSettings (:782) and removeSourceFromSettings (:813), reached only from installAndPersist (:989) and removeAndPersist (:1014), whose sole non-test callers are package-manager-cli.ts:601/:606. Both also need an explicit local: true to select project scope at all (scope = options?.local ? "project" : "user" at :783, :814, :994), which the CLI only sets from the --local flag.

Neither entry point exists in a server turn: sdk.ts contains no PackageManager reference, so createAgentSession never constructs one, and the harness passes only sessionManager/model/resourceLoader/settingsManager (run-turn.ts:499-504). So no project-scope write is reachable from a server turn — same verdict, now about the functions that can actually write.

Three things folded in while there:

• Your reload({ resolveProjectTrust }) detail is in the spec, as the reason the trust-flag calls are unreachable: resource-loader.ts:328/:338 run only under that option (:335-338) and run-turn.ts:493 calls reload() with none.
• Residual harness: tsc fails on cli.ts gateway-header cast (x-api-key: null → Record<string, string>) #1 retargeted — the guard belongs on the five setProject* methods, not on the setProjectTrusted call sites, which cannot write.
• A short "Superseded citation" paragraph records that v1.1 named a set that cannot write and omitted the set that can, so the next reader doesn't have to re-derive why the line numbers moved. That is the section's stated purpose, and it applies to its own history.

One correction back: package-manager-cli.ts is at packages/coding-agent/src/, not src/core/ — noted in the spec so the path is right if anyone re-runs this.

[pdettori]

nit — the quoted code elides a branch. session-manager.ts:1421 is:

const dir = backend ? "" : sessionDir ? normalizePath(sessionDir) : getDefaultSessionDir(cwd);

The middle sessionDir ? normalizePath(sessionDir) : is dropped. Harmless for the argument — the harness passes sessionDir === undefined (run-turn.ts:432, :442) and always passes a backend, so dir is "" either way and the "proven inert" conclusion holds. Raising it only because it is presented in backticks as verbatim source in a document whose value is exact citation.

[cwiklik]

Fixed in a982ffd — the ternary is now quoted in full:

const dir = backend ? "" : sessionDir ? normalizePath(sessionDir) : getDefaultSessionDir(cwd);

Fair catch on the grounds you give: in a document whose value is exact citation, an elided branch inside backticks is a defect even when the argument is unaffected. The spec now also states why it's unaffected rather than leaving it implicit — the harness passes a backend and no sessionDir (run-turn.ts:419-420, :432, :442), so dir is "" on both branches that could otherwise reach cwd.

[pdettori]

nit — redis-backend.ts:6-7 is cited as a bare filename. The lines are correct (packages/session-backend/src/redis-backend.ts:6-7 is exactly streamKey/seqKey, and the keyspace is flat as described), but the repo also has harness/src/buffered-redis-backend.ts, and neighbouring citations in this document use full paths. Worth qualifying for consistency.

[cwiklik]

Fixed in a982ffd — now packages/session-backend/src/redis-backend.ts:6-7, with the reason for the full path stated inline (harness/src/buffered-redis-backend.ts also exists), so it doesn't get shortened again on the next pass.

[pdettori]

Reviewing from the MU1 / P6 side rather than on #239's readiness goals, which look right to me — the :174 catch in particular is the kind of thing that would have shipped a green test suite over a live bypass.

Framing up front: these are four additive scope-boundary notes, not corrections. Every claim below is true of P5 considered alone; each becomes wrong only once MU1 lands. I'm raising them here because MU1 §3.6 opens with:

P5's design is merged and should not be rewritten to accommodate a later spec, so the three interactions between them are carried here.

That was sound when it was written — but this PR reopens P5 for implementation readiness, which makes "carry it in MU1" the more expensive option for all three. This PR mentions MU1 zero times, so I suspect the two tracks just haven't crossed yet.

---

1. The tagged credential — this PR currently pins the opposite

The new §3.2 step 1 text says:

The placeholder rides in on the existing TurnConfig.anthropicAuthToken field — applyModelGateway installs Authorization: Bearer ${authToken} (run-turn.ts:336) from it, so no new credential field is needed.

MU1 §3.6 item 1 requires a tagged union instead, and gives the reason:

type UpstreamCredential =
  | { mode: 'placeholder'; value: string }  // P5 + an injector in the egress path
  | { mode: 'direct'; value: string }       // MU1 interim, control-plane resolved

Same field, incompatible contents. Left implicit, a P5 implementation that unconditionally sets the placeholder would silently overwrite MU1's token, and requests would fail with no injector configured to swap it.

Both failure directions are silent: placeholder mode with a misconfigured injector sends the placeholder upstream and gets an opaque auth error; direct mode that later grows an injector has its real key rewritten. MU1 also points out this is the same argument P5 §3.4 already makes about ambient placeholders — the injector faithfully swaps in whichever tenant a placeholder names — applied to a mislabelled one.

Resolution rule is MU1's, so P5 need not adopt it, only leave room for it: the control plane declares the mode at exchange time, placeholder mode wins wherever an injector exists, and MU3 deletes direct mode.

Cost of doing it here: a paragraph in a docs-only PR. Cost of not: MU1's implementation reopens run-turn.ts and P5's tests to widen a field P5 had just pinned as sufficient.

2. The §5 Lock-down invariant, in-process row over-asserts

It reads "no real provider credential is reachable from the harness process in server mode." MU1's interim direct mode puts one there every turn — a divergence ADR-0033 explicitly accepts. MU1 §3.6 item 2 states the amendment, and it's the one thing in that section that can only live in P5:

P5's lock-down assertion wants scoping to the environment (which MU1 never writes) rather than to the whole process, or gating on direct mode being disabled.

As written, MU1 makes this test fail, and the tempting repair at that point — weakening the assertion — discards precisely the pin P5 §2.4 exists to create ("documented prose with nothing pinning it"). Env-scoping costs nothing today and stays true through MU3.

3. Fail-closed: no subject needs one more clause

The row asserts "401 before a session is created, and no upstream request made." Under MU1 §3.5 the subject is token.sub and an inbound X-SH-Subject is ignored when a session token is present, so a token-bearing request carrying no header is legitimate and must not 401:

When a session token is present, the subject is token.sub, and any inbound X-SH-Subject is ignored. A request carrying both a session token and a conflicting X-SH-Subject is rejected with subject_conflict (400) rather than resolved by precedence.

Phrasing the assertion as "no subject resolvable from any source" is free today and survives MU1. Phrased as-is, MU1 has to invert it — and this PR makes that exact argument better than I can, in its own new paragraph about the three model-gateway.test.ts assertions that "must be inverted rather than updated … do not let a green run be achieved by weakening them."

Relatedly, MU1 §3.6 item 3 asks that the implementation not pin "inbound X-SH-Subject is always honoured" — the operator and leaf paths keep that behaviour, the token-bearing path does not.

4. The scrub location settles a question P6 needs one step further

Settling the scrub at the exported startServer() (:576) rather than the isMainModule guard is right, and the in-process assertability argument is convincing.

One consequence for P6 (#244, VM process manager): its worker is a third entry point that builds createServer(handler) and never calls listen() — so it never calls startServer() and would run unscrubbed, with W×S multiplexed sessions, which is the exposure the scrub exists for. P6 §3.6's ask is therefore that the scrub land as a shared function startServer() calls, rather than inline within it. Same one-line factoring, and it keeps this PR's assertability argument intact.

---

Happy to open a PR against docs/239-p5-spec-amendments with concrete wording for any of these, or to just hand over the text — whichever is less disruptive to #239's sequencing. If you'd rather keep P5 v1.1 focused and land these as a v1.2, that works too; item 1 is the only one I'd argue shouldn't wait, since this PR is actively pinning the narrower contract.

cc MU1 (#238) · P6 (#244)

[pdettori]

Second pass, scoped to P6 (#244) rather than MU1 — separate comment because it's a different category from my earlier review, and item 1 below is a category question about §4 rather than a wording fix.

Three additions, plus one correction to something I initially suspected and then disproved, and one note about a change in this PR that actively helps.

---

1. §4's verdicts are correctness-only. P6 needs the liveness half named.

This is the substantive one, and this PR already found the evidence for it — it's the classification I'd push back on, not the finding.

The amendment records the settings lock as a residual: "the settings lock busy-waits synchronously (10 × 20 ms) on the read path, which stalls the event loop for every multiplexed session." Verified, and it's settings-manager.ts:192-217:

const maxAttempts = 10;
const delayMs = 20;
// …
const start = Date.now();
while (Date.now() - start < delayMs) {
  // Sleep synchronously to avoid changing callers to async.
}

SettingsManager has no async variant of withLock, so contention always spins, and it takes the lock on reads too whenever the file exists (:226-229).

What makes this P6's ceiling rather than a curiosity is the key it locks on: join(resolvedCwd, CONFIG_DIR_NAME, "settings.json") (:189) — derived from cwd, which is process-global, i.e. precisely the item §4 calls "the one item that could change this slice's verdict." So every session in a worker resolves to one lockfile, contention is structural rather than incidental, and each contended acquisition stalls every other session in the process for up to 200 ms of pure CPU.

§4's verdict on cwd — inert — is correct for isolation and silent on liveness. The same shape appears in §2.1 item 4, which dismisses registrationQueue as "throughput, not correctness." Both are the right call for P5, whose property is isolation. They are the exact dismissal P6 cannot inherit, because throughput is P6's subject: a global that cannot corrupt a session but can stall S of them is inert by P5's test and fatal by P6's.

Ask: one sentence in §4 scoping its verdicts to correctness and pointing the liveness question at the deployment-model slice. Not a new pin — P6 §5.2 already instruments event-loop lag p99 per worker (monitorEventLoopDelay), which is exactly the instrument that catches this. The value of the sentence is that a fresh P6 session doesn't re-derive the cwd → shared-lockfile chain from scratch, which is this PR's whole stated purpose.

2. The output-guard pin's phrasing excludes P6's entry point

§4 pins "isStdoutTakenOver() stays false across a server turn." P6's worker is a third entry point — it builds createServer(handler) and never calls listen(), serving sockets handed over IPC — so a pin worded around "a server turn" leaves it uncovered.

Phrased as "in any non-CLI entry point" it covers worker.ts for free and needs no fourth amendment. Worth doing precisely because §4's own note makes the stakes higher there: output-guard.ts:91's process.exit(1) costs one session at 1:1 and S sessions in a multiplexed worker.

3. The two-tenant interleaved test needs a parameterized entry point

P6 §7 inherits this test and runs it at the worker level — round one claims no isolation property, but it must not break P5's unknowingly, and the cheapest way to know is to run P5's own test where W×S sessions actually share a process.

That only works if the test can be pointed at an entry point. If it stands up startServer() directly, P6 has to fork it. Deciding it at authoring time costs nothing and is the same category as everything else in this PR.

---

Correction to something I suspected

I initially thought auth-storage carried this hazard onto the credential path, since it has the identical spin at :77-96. It doesn't, and the distinction is worth recording so nobody else chases it: getApiKey → OAuth refresh goes through withLockAsync (:124), which uses lockfile.lock with async retries and does not block the loop. Only the sync withLock callers (:221, :266, :284 — the write paths) spin, and a server turn shouldn't reach them. So SettingsManager is the live instance, not AuthStorage.

A change here that helps P6

TurnConfig gaining an explicit subject field is more valuable at W×S than the amendment's own rationale suggests. The stated reason is keeping the logging path away from the credential mapping, which holds at 1:1 — but in a process multiplexing S sessions, per-session attribution is otherwise recoverable only by reverse-mapping a placeholder, and doing that per log line across S concurrent sessions is both a hot path and the thing §3.4 warns about. Please keep it.

---

Same offer as before: I can open a PR against docs/239-p5-spec-amendments with wording for items 1–3, or hand over the text. All three are one or two sentences; none changes the three ordered steps or the shape of the implementation.

Prior review on MU1 composition: #242 (review above) · P6 spec: #244 · related ST follow-up: #245

[pdettori]

Re-verified every item from my three earlier passes against a982ffd, fetching the pinned fork (kagenti/pi@2c28be50) from GitHub rather than a local clone. All ten are addressed, and the audit reproduces independently.

The must-fix is resolved, and your correction of my evidence is the right way round. §4 now names the writers that actually reach updateProjectSettings (:635) → assertProjectTrustedForWrite (:527), and every line checks out exactly: the five setProject* methods at :942/:958/:974/:990/:1006 (each calling updateProjectSettings on the next line), saveProjectSettings (:618, reached at :640), and enqueueWrite's "project" branch (:552-553).

I re-ran the reachability audit rather than take the trace on faith, and it holds. A repo-wide grep of the pinned tree finds exactly two non-test call sites for the five methods — package-manager.ts:798/:806/:824 and modes/interactive/components/config-selector.ts:484/:486/:488/:490/:558 — with every other hit under test/. installAndPersist/removeAndPersist (defined :987/:1012) have exactly two non-test callers, package-manager-cli.ts:601/:606, both passing { local: options.local }. sdk.ts contains zero PackageManager references. Your path correction is right: package-manager-cli.ts is at packages/coding-agent/src/.

Worth noting the disambiguation earned its keep — src/cli/config-selector.ts also exists and is not a caller, so the src/modes/interactive/ qualifier is load-bearing, the same class of hazard as the redis-backend.ts nit.

Also spot-checked and exact: :189 as the cwd-derived lock key, :192-217 (the method spans precisely those lines), the read-path lock at :226-228, the write guard at :232-241, loadFromStorage at :349-352, projectTrusted defaulting true at :313, and every auth-storage.ts citation (:76-98, :124, :409, :417, :464, :490) — including :76-98, which is a better range than the :77-96 I originally gave.

Both nits fixed, and the ternary now carries why it is inert rather than leaving it implicit.

All four MU1 notes and all three P6 asks absorbed, several better than I asked for. no new credential _path_ rather than field is the precise repair for item 1. The §5 row's "gate it on direct mode being disabled rather than loosen it" closes the failure mode I was actually worried about — an implementer reaching for the weaker assertion when MU1 reddens the test. And §6 naming P6 as the consumer of all three forward references means the next reader does not have to guess whether they are hypothetical.

Two nits from this pass, both introduced by the fixes themselves. Neither blocks.

§3.6 now contradicts the §5 row it cites. Line 311 still reads "§5's lock-down row is scoped to the process for exactly this reason" — but MU1 item 2 re-scoped that row to the environment, and the row now cites §3.6 as one of its own two reasons. The pod-delivery argument is untouched; only the word is stale. In the body rather than inline because the line falls outside every diff hunk.

The second is inline, on the version label.

Areas reviewed: Docs (single spec file)
Agent/IDE config (.claude/.vscode): none
Commits: 3, all signed off
CI: all 12 checks green

[pdettori]

nit — this labels the current version as the erroneous one. The header reads Version: 1.1 (line 3), and this paragraph says "v1.1 named package-manager-cli.ts:487 and resource-loader.ts:328/:338 as the project-scope mutators."

Those citations never existed in a v1.1 anyone will read. I checked main: it is Version: 1.0 and contains no package-manager-cli, resource-loader.ts:328, or setProjectTrusted reference at all. They were introduced by this PR's c2e9fe1 and corrected in a982ffd, so they only ever existed on this branch.

As written, a fresh reader hits this paragraph, goes looking for those line numbers in the v1.1 in front of them, and finds nothing — the opposite of the orienting function the paragraph exists to serve. It also reads oddly against line 356, which correctly attributes the other superseded pin to "the 1.0 pin above".

Cheapest fix is a reword ("an earlier draft of this amendment", or "v1.1 as first drafted"); bumping the header to 1.2 also works and would make the note literally true. Your call which — the paragraph is worth keeping either way, and the reload({ resolveProjectTrust }) detail folded into it is the part I would most want a fresh session to find.