Add runtime-bound sessions and runtime UI#43
Conversation
ashwin-pc
left a comment
ashwin-pc
left a comment
There was a problem hiding this comment.
Reviewed the whole branch. Verdict: the foundation layer is on the right track — the integration layer needs a course correction before this grows further.
Keep as-is (this part is good)
server/runner.tsas a standalone stdio JSON-RPC agent host is exactly the right shape: one runner that works identically on host, in Docker, viadocker exec, or over SSH. Best decision in the branch.- The NDJSON protocol (
protocol.ts,stdioClient.ts) is simple, debuggable, and transport-agnostic. RuntimeBindingStorewith atomic writes + serialized write queue matches the runtime-binding design doc.- Docker security posture (
--network nonedefault, env allowlist, read-only/app, no arbitrary host mounts from API params) and the honest limitations section indocs/docker-runtime.md. - Degraded
runtimeUnavailable*states so the UI survives a missing runtime. - Layered tests (protocol / provider / runner / API / e2e).
Core problem: runner sessions are a parallel session universe
Runner sessions bypass liveSessions, viewer leases, the realtime pipeline, tool cards, streaming, the tree, compaction, models, thinking, and images. To compensate, 5+ core routes (/api/state, /api/messages, /api/prompt, /api/abort, /api/sessions/open, plus git/fs/artifacts/new-chat/cwd) each grew an if (runnerBinding...) fork with its own error handling. This is the mockMode branching pattern, multiplied.
User-visible symptoms:
- No streaming: the runner only emits
session.prompt.start/done/error, so sandboxed sessions show nothing until the full response completes — no deltas, no tool cards, no thinking. Big enough UX cliff that people will avoid sandboxed sessions. /api/modelsreturnsmodels: [], thinking is hardcodedoff, and prompt images are silently dropped (see inline comment — that one is a bug, not a gap).
The design doc says: “Make existing API routes runtime-aware without duplicating UI behavior.” The current shape duplicates behavior.
Suggested inversion
- In
runner.ts, addsessions.subscribe: callsession.subscribe(...)and forward every agent event as{ event: "session.event", data: { sessionId, event } }. ~15 lines, and it is the same event shapebroadcast()+ frontendrealtime.tsalready consume — streaming, tool cards, and unread tracking then work for free. - Define a narrow
SessionHostinterface (state, messages, prompt(+images), abort, setModel, subscribe, …). Local = today's in-process path; remote = an adapter overStdioRuntimeClient. Routes resolvesessionId → hostonce, in one place, instead of per-route forks. - Forward
models.list/setModel/ thinking / images (base64) through the protocol so the runner protocol becomes “everything a session can do”, not “the subset forked so far”.
This is also the first slice of the server refactor we discussed: SessionHost is the same seam that lets mock mode become a third host implementation, killing both sets of if forks together. I'd do steps 1–2 before adding any more proxied routes — each new fork raises the cost of the inversion.
Other structural points (details inline)
- The three providers (
StdioRunnerProvider/DockerRunnerProvider/CommandRunnerProvider) are ~90% copy-paste; collapse to one class + a Docker config factory. runner.tsre-implementssafeArtifactName,textFromContent, git status/diff fromserver.ts— extract sharedserver/git.ts/server/artifacts.tson main first so runner and server import the same code instead of drifting.- Rename “experimental” — it's load-bearing now — and consider gating the always-on local
StdioRunnerProviderbehind an env flag; it spawns a secondtsxprocess to do what the in-process path already does.
Suggested merge sequencing
- Mergeable nearly as-is:
protocol.ts,stdioClient.ts(with the non-JSON-line fix),bindings.ts,runtimeStore.ts, runtime panel UI, docs. - Collapse the three providers.
- Full event forwarding in
runner.ts. SessionHost+ remove per-route forks (the big one).- Inline bug fixes: bindings growth/caching, image drop, token-into-container, stdout parse fragility.
| try { | ||
| message = parseRuntimeLine(line) as RuntimeResponse | RuntimeEvent | undefined; | ||
| } catch (error) { | ||
| this.failAll(error instanceof Error ? error : new Error(String(error))); |
There was a problem hiding this comment.
Bug risk: a single non-JSON line on stdout calls failAll, killing every pending request and effectively bricking the client. The example configs in runtimePanel.ts run npm exec --yes tsx …, and npm happily prints banners/progress to stdout, so this will happen in practice.
Suggest: ignore unparseable lines (log them to stderr), or require the ready handshake and drop anything received before it. failAll should be reserved for process exit/error.
| "-w", "/app", | ||
| "-e", `PI_RUNNER_CWD=${this.containerWorkspace}`, | ||
| ]; | ||
| if (this.token) args.push("-e", "PI_WEB_TOKEN"); |
There was a problem hiding this comment.
This ships the pi-web bearer token into the sandboxed container, but runner.ts never uses PI_WEB_TOKEN. Unnecessary secret inside the trust boundary you're building — drop it.
| kind?: "local" | "container" | "ssh"; | ||
| }; | ||
|
|
||
| export class CommandRunnerProvider { |
There was a problem hiding this comment.
CommandRunnerProvider, StdioRunnerProvider, and DockerRunnerProvider are ~90% identical — same sessionFiles map, same request wrappers, same rememberSession/sessionParams. They differ only in how the spawn command is computed.
Suggest one provider class taking { id, label, kind, command, args, cwd, processCwd }, with Docker reduced to a factory function that turns DockerRunnerOptions into that config (dockerArgs() already is that computation). One class, one test suite, and runtimeKindForProvider/defaultCwdForRunnerProvider/isDockerRunnerProvider type-sniffing in server.ts disappears too.
| const session = await resolveSession(params); | ||
| const message = String(params.message || "").trim(); | ||
| if (!message) throw new Error("message is required"); | ||
| send({ event: "session.prompt.start", data: { ...sessionState(session), isStreaming: true } }); |
There was a problem hiding this comment.
This is the root of the biggest gap in the branch: only prompt.start/done/error are emitted, so runner sessions have no streaming — no assistant deltas, no tool cards, no thinking indicator until the entire response finishes.
Suggest adding a sessions.subscribe method that calls session.subscribe(...) and forwards every agent event as { event: "session.event", data: { sessionId, event } }. It's the same event shape broadcast() and the frontend realtime.ts already consume, so streaming/tool cards/unread tracking come for free on the server side once the events flow.
| const loader = new DefaultResourceLoader({ cwd, agentDir: getAgentDir() }); | ||
| await loader.reload(); | ||
| const result = await createAgentSession({ cwd, sessionManager, authStorage, modelRegistry, resourceLoader: loader }); | ||
| liveSessions.set(result.session.sessionId, result.session); |
There was a problem hiding this comment.
runner.ts re-implements safeArtifactName, textFromContent, and git status/diff that already exist in server.ts (the two safeArtifactNames are only coincidentally identical today and will drift). Worth extracting shared server/git.ts / server/artifacts.ts modules on main first so both import the same code — this dovetails with the server.ts refactor anyway.
| envAllowlist: process.env.PI_WEB_DOCKER_ENV_ALLOWLIST?.split(",").map((item) => item.trim()).filter(Boolean), | ||
| }) | ||
| : undefined; | ||
| const experimentalRunnerSessionIds = new Set<string>(); |
There was a problem hiding this comment.
experimentalRunnerSessionIds (Set) and runnerSessionRuntimeIds (Map) encode the same fact and are updated in tandem in ~6 places — they can drift. A single Map<sessionId, runtimeId> covers both (has() replaces the Set). Also suggest renaming away from "experimental" — this is load-bearing routing state now, and the name leaks into runnerProviderForSession's fallback logic.
| const runnerProvider = runnerProviderForSession(requestedSessionId); | ||
| const runnerState = await runnerProvider.state(requestedSessionId) as any; | ||
| const status = await runnerProvider.gitStatus(runnerState.cwd) as any; | ||
| return sendJson(res, 200, { ok: true, cwd: runnerState.cwd, branch: status.branch || "", files: [], isRepo: Boolean(status.isRepo), raw: status.porcelain || "" }); |
There was a problem hiding this comment.
This returns files: [] + raw porcelain while the local path returns parsed files, so the git panel renders differently depending on runtime. You already have parseStatusLine in this file — parse the porcelain server-side so the response shape is uniform regardless of where git ran.
| return sendJson(res, 200, { ok: true, runtimes: [...runtimeRegistry.list(), ...runtimeRunnerProviders.map(runtimeSummaryForProvider)] }); | ||
| } | ||
|
|
||
| if (method === "POST" && url.pathname === "/api/runtimes/connect") { |
There was a problem hiding this comment.
Worth flagging explicitly: this endpoint is authenticated arbitrary command execution on the host, and unlike a shell in a session it persists across restarts via runtimeStore and is invisible afterwards. pi-web is inherently RCE-by-design behind the token, but this deserves (a) a callout in the docs like you did for Docker, and (b) possibly an env opt-in (PI_WEB_ALLOW_CUSTOM_RUNTIMES=1) so a token leak doesn't silently gain a persistence mechanism.
| try { | ||
| const artifactProvider = runnerProviderForSession(sessionId); | ||
| const runnerState = await artifactProvider.state(sessionId) as any; | ||
| const artifact = await artifactProvider.readArtifactBase64(runnerState.cwd, name); |
There was a problem hiding this comment.
No size cap: a large artifact is fully buffered in the runner, JSON/base64-encoded (×1.33), then buffered again here. Add a max-bytes check in the runner's artifacts.readBase64 (reject over, say, 10–20 MB) — chunked transfer can come later if needed.
| messages: number; | ||
| }; | ||
|
|
||
| export class StdioRunnerProvider { |
There was a problem hiding this comment.
This provider is registered unconditionally and spawns a second tsx subprocess to do what the in-process path already does. Great as a test/dev vehicle, but as a default user-selectable runtime it doubles memory and confuses "Local machine" vs "Local runner process". Suggest gating it behind an env flag (e.g. PI_WEB_LOCAL_RUNNER=1).
|
Pushed follow-up review fixes through |
ashwin-pc
left a comment
ashwin-pc
left a comment
There was a problem hiding this comment.
Re-review after 9427fce + b351082
This round resolved almost everything from the last review, and resolved it well:
- ✅ stdioClient no longer bricks on non-protocol stdout lines
- ✅
PI_WEB_TOKENno longer shipped into the container (+ docs updated) - ✅ Provider triplication collapsed — Docker/Stdio are now thin subclasses of
CommandRunnerProvider - ✅ Full pi event stream forwarded (
sessions.subscribe→session.event→pi_eventbroadcast) — streaming works - ✅ Images passed through to runner sessions (the silent-drop bug is gone)
- ✅
ensureLocalno longer persists unbounded local rows; binding store has a write-through cache - ✅
experimentalRunnerSessionIdsSet removed; singlerunnerSessionRuntimeIdsMap - ✅ Runner git status parsed with
parseStatusLine+ ahead/behind - ✅ Artifact size cap (20MB, env-tunable)
- ✅ Local runner gated behind
PI_WEB_LOCAL_RUNNER=1 - ✅
/api/runtimes/connectgated behindPI_WEB_ALLOW_CUSTOM_RUNTIMESin prod, with docs callout - ✅ Bonus: host-only routes now return explicit 501s instead of silently operating on the host cwd, and model switching actually works for runner sessions
Why the remaining items are blocking, not follow-ups
Main has no runtime concept today. This PR introduces it, so whatever shape merges becomes the foundation everything else builds on. Deferring the structural items means merging an architecture we already know we want to replace, and re-reviewing the same surface twice. Since nothing has shipped, there's no churn cost to doing it right the first time — so I'm marking the structural items as blocking.
Blocking:
SessionHostinversion — the per-routeruntimeBindingIfAnyforks have grown to ~15 call sites. Details inline.- Event forwarding parity — forwarded runner events skip the host-side enrichment pipeline (tool
startedAt,lastActivityAt, unread recovery, stats), andruntimeForEventis keyed on container paths. Details inline. - Runner-side session/subscription leak —
liveSessions/subscriptionsgrow forever. models.setstate-merge hack in/api/model— have the runner return full session state instead.- Helper duplication between
runner.tsandserver.ts— extract a shared module before the copies drift. - Frontend fetch/error-parsing duplication —
sessionDrawerandruntimePaneleach hand-roll runtime fetching and error parsing; add the shared API helper now rather than after 10 more call sites exist. - Test coverage for the two headline fixes — event forwarding and image passthrough are currently untested.
Nits (genuinely fine to skip): rename experimentalRunnerWebState, malformed-protocol-line hang, prompt timeout with large images, document.execCommand fallback.
The protocol, runner, provider hierarchy, binding store, security posture, and UI are all in good shape — the remaining work is integration shape, not rework.
| const binding = await runnerBindingForSession(sessionId); | ||
| return binding.binding || binding.provider ? binding : undefined; | ||
| } | ||
| function runtimeUnsupported(res: ServerResponse, feature: string, binding?: { binding?: SessionRuntimeBinding; provider?: RunnerProvider }) { |
There was a problem hiding this comment.
Blocking: invert to a SessionHost interface instead of per-route forks.
runtimeUnsupported + runtimeBindingIfAny guards now appear at ~15 route call sites (git/repos, git/log, git/commit, git/image, git/sync, web-header-action, session/stats, session/tree ×3, commands, command, shell, compaction/abort, session/name, delete), plus full forks in messages, model ×2, prompt, git/status, git/diff, artifacts, and the WS hello. Every new route added to pi-web from now on has to remember to add a guard, and every new runtime capability means touching N routes.
Since this PR is what introduces the runtime concept to main, this is the moment to shape it. Define one interface both session kinds implement:
interface SessionHost {
state(): Promise<WebState>;
messages(): Promise<SimplifiedMessage[]>;
prompt(message: string, images?: ImagePart[]): Promise<void>;
abort(): Promise<void>;
gitStatus?(): Promise<GitStatus>; // optional = capability
listModels?(): Promise<ModelState>;
// ...
}
function hostForSession(sessionId: string): Promise<SessionHost>;Routes then call host.gitStatus?.() ?? unsupported(res, "Git status") — the 501 behavior you added falls out of optional methods for free, unsupported capabilities are impossible to forget, and mock mode becomes a third host instead of its own special case. The LocalSessionHost wraps the existing live-session code; the RunnerSessionHost wraps CommandRunnerProvider. This is mostly moving code you already wrote, not new logic — but doing it after merge means re-reviewing all 15 sites twice.
| const piEvent = data?.event; | ||
| if (sessionId) runnerSessionRuntimeIds.set(sessionId, provider.id); | ||
| broadcast({ type: "pi_event", sessionId, sessionFile, event: piEvent }); | ||
| broadcast({ type: "session_runtime_changed", sessionId, sessionFile, runtime: runtimeForEvent(sessionFile, piEvent) }); |
There was a problem hiding this comment.
Blocking: forwarded runner events bypass the host-side enrichment pipeline, and runtimeForEvent is keyed on container paths.
Compare with the local event path (~line 2280): before broadcasting, it stamps startedAt on tool_execution_start, propagates it to tool_execution_update/end, sets lastActivityAt via markRuntimeActivity, and additionally emits session_stats_changed/models_updated. Runner events are broadcast raw, so for runtime sessions the UI loses tool durations, activity timestamps, and stats updates — a quiet quality gap that will read as "runtime sessions feel broken".
Separately, runtimeForEvent(sessionFile, piEvent) and noteRuntimeEventForUnreadRecovery key host-side maps (runtimeStartedAts, runtimeLastActivityAts) by sessionFile — but here sessionFile is a container path that will never match host session listings, so unread recovery and runtime status for these sessions are silently wrong.
Fix: extract the enrichment block from the local subscribe handler into a shared enrichAndBroadcastPiEvent(sessionId, sessionFile, event) and call it from both paths, and key the runtime-activity maps by sessionId (or a normalized key) rather than raw file path. This pairs naturally with the SessionHost change.
| const authStorage = AuthStorage.create(); | ||
| const modelRegistry = ModelRegistry.create(authStorage); | ||
| const liveSessions = new Map<string, any>(); | ||
| const subscriptions = new Map<string, () => void>(); |
There was a problem hiding this comment.
Blocking: liveSessions and subscriptions grow without bound.
Every sessions.state/sessions.prompt/sessions.subscribe on a new session adds an entry to both maps and nothing ever removes them — each entry holds a full agent session (messages, model registry hooks) plus a live subscription. A long-lived Docker runner serving a user who touches many sessions will accumulate all of them in memory.
Minimum fix: an LRU cap (e.g. keep the N most recently used sessions; on eviction call the stored unsubscribe and drop the session). Also consider a sessions.release RPC so the server can proactively evict when a binding is deleted or a session goes idle.
| const id = String(body.id || "").trim(); | ||
| if (!provider || !id) return sendJson(res, 400, { ok: false, error: "provider and id are required" }); | ||
|
|
||
| const runnerBinding = await runnerBindingForSession(requestedSessionId); |
There was a problem hiding this comment.
Blocking: replace the state-merge hack with a full state return from the runner.
experimentalRunnerWebState({ ...runnerState, ...models }, runnerProvider) merges two differently-shaped objects (models has current/models/cwd, runnerState has model/sessionId/...), then patches model/thinkingLevel back on top. It works, but it's exactly the kind of shape-coupling that breaks silently when either side changes.
Simpler: have the runner's models.set handler return { ...sessionState(session), models: modelState(session) } (it already has the session in hand), so the server gets one authoritative post-change state and this route becomes broadcast(state); sendJson(res, 200, state). Also saves the second round-trip (setModel then state).
| return { path, parent: dirname(path), dirs }; | ||
| } | ||
|
|
||
| async function gitStatus(cwdValue: unknown) { |
There was a problem hiding this comment.
Blocking: extract shared helpers instead of duplicating server.ts logic.
gitStatus, gitDiff, safeArtifactName, the artifact path convention (.pi/web/artifacts), and listDirectories here are near-copies of code in server.ts. They've already drifted once this PR (this side gained -b porcelain headers; the artifact side gained a size cap the host route doesn't have).
Move them to something like server/shared/{git,artifacts,fsList}.ts imported by both server.ts and runner.ts. The runner is spawned via tsx from the same source tree in every deployment mode (local, Docker mounts /app, custom commands run the same file), so sharing modules is safe — and it guarantees host sessions and runner sessions return identical shapes for the same operations.
| }); | ||
|
|
||
| describe("runtime runner spike", () => { | ||
| it("serves health, filesystem, git, artifacts, and events over stdio", async () => { |
There was a problem hiding this comment.
Blocking: the two headline fixes from this revision are untested.
This test exercises health/fs/git/artifacts/session CRUD, but not:
- Event forwarding —
sessions.subscribereturning ok, and asession.eventenvelope arriving on the client with{ sessionId, sessionFile, event }shape (you can trigger at leastsession.createddeterministically without a model key). - Image passthrough —
sessions.promptwith animagesarray accepting an image-only prompt (no message), and rejecting a prompt with neither.
Both were the fixes for real bugs (no streaming; images silently dropped), so they're the regressions most worth guarding. A third cheap one: artifacts.readBase64 rejecting a file over PI_RUNNER_MAX_ARTIFACT_BYTES with a tiny env override.
| webHeaderActions: [], | ||
| }; | ||
| } | ||
| function experimentalRunnerWebState(runnerState: any, provider: RunnerProvider) { |
There was a problem hiding this comment.
Nit: experimentalRunnerWebState is now a misnomer — it renders state for all runtime providers including the non-experimental Docker one. Rename to runnerWebState (falls out naturally if the SessionHost refactor lands).
| message = parseRuntimeLine(line) as RuntimeResponse | RuntimeEvent | undefined; | ||
| } catch (error) { | ||
| const detail = error instanceof Error ? error.message : String(error); | ||
| process.stderr.write(`[runtime] ignored non-protocol stdout line: ${line.slice(0, 500)}${line.length > 500 ? "…" : ""} (${detail})\n`); |
There was a problem hiding this comment.
Nit: ignoring unparseable lines is the right default (fixes the npm-banner problem), but note the trade-off: a truncated/corrupt protocol line now means the matching request silently hangs until its timeout instead of failing fast. If you want the best of both: when the ignored line starts with {"id":, log at a louder level — that's almost certainly a mangled response, not banner noise.
|
|
||
| messages(sessionId: string) { return this.start().request("sessions.messages", this.sessionParams(sessionId)); } | ||
|
|
||
| async prompt(sessionId: string, message: string, images?: unknown[]) { |
There was a problem hiding this comment.
Nit: sessions.prompt uses a 30s request timeout, and the request line now carries base64 image payloads. Over a slow transport (SSH runner, cold Docker) a few multi-MB images could plausibly blow the window. Consider a larger timeout for prompt specifically, or scale it when images.length > 0.
| } | ||
| } | ||
|
|
||
| async function connectRuntime() { |
There was a problem hiding this comment.
Nit: document.execCommand("copy") is deprecated; fine as a fallback, but the navigator.clipboard branch already covers every browser that can run this app (secure context is required for the token flow anyway). Could drop the fallback entirely and simplify.
|
Pushed |
ashwin-pc
left a comment
ashwin-pc
left a comment
There was a problem hiding this comment.
Re-review after 1a42c8f "Introduce session host runtime routing"
All seven blocking items from the previous review are genuinely resolved, and resolved well:
- ✅
SessionHostinversion — capability-based interface with three implementations (local/runner/unavailable), onesessionHostForSession()factory, and all ~25 routes uniformly usinghost?.capability+unsupportedHostCapability. The 501 behavior now falls out of optional methods; the per-route forks are gone. - ✅ Event parity —
enrichPiEventForBroadcast/broadcastPiEventshared between local and runner paths (toolstartedAt,lastActivityAt, stats,models_updated), and runtime-activity maps re-keyed byruntimeMapKey(sessionId, sessionFile)— the container-path bug is fixed. - ✅ Runner memory — LRU cap with unsubscribe+dispose on eviction, plus
sessions.release. - ✅
models.setreturns full session state; the route is one call, one broadcast. - ✅ Shared helpers —
server/shared/{git,artifacts,fsList}.ts; runner git status even gained full parity (untracked, upstream, ahead/behind, fetchRemote). - ✅ Frontend API consolidation —
src/runtimes/api.ts, one normalizer + one error parser. - ✅ Tests for forwarding, image passthrough, artifact cap. All four nits fixed too.
I verified this empirically in a clean worktree: typecheck passes, and the unit/API/runtime suites pass — with one exception that is the first of two remaining blockers.
Blocking
tests/runtime-runner.test.tsimage-prompt test is not hermetic — it depends on a live model API key + network and fails deterministically without one (details inline). This will fail in CI and for any contributor without credentials.- Runner session deletion must be implemented, not deferred — I initially called the unused
provider.release()/runtimeBindingStore.remove()surface a non-blocking observation, but tracing the data flow shows deletion is a required part of this feature, not a follow-up (details inline onlistSessionInfos).
Once these two land, this is mergeable as far as I'm concerned — the architecture is now the one we wanted on day one.
| } | ||
| }, 60_000); | ||
|
|
||
| it("accepts image prompts and rejects empty prompts", async () => { |
There was a problem hiding this comment.
Blocking: this test requires live model credentials and fails without them.
The session.event assertion depends on the agent loop actually starting. I traced it with a debug harness against the runner: with no API key available, session.prompt fails before the agent loop (session.prompt.error: "No API key found for the selected model..."), so no session.event is ever forwarded and waitForEvent times out after 15s. In my environment this fails 4/5 runs; in keyless CI it will fail 100% of the time.
Two hermetic options:
- Assert the deterministic contract instead:
sessions.promptresolves{ ok: true }(proving the image-accepting path), and eithersession.prompt.startorsession.prompt.errorarrives (proving the event plumbing) — no model call required:
const promptEvent = waitForEvent(client, (e) => e.event === "session.prompt.start" || e.event === "session.prompt.error");- Or keep the stronger
session.eventassertion but gate it:it.skipIf(!process.env.ANTHROPIC_API_KEY)(...), keeping the empty-prompt rejection assertion in an unconditional test.
Option 1 is better — it tests your code rather than Anthropic's uptime.
| if (mockMode) return mockSessions.map((info) => simplifySessionInfo(info as any, info.cwd || piCwd)); | ||
| const bindings = (await runtimeBindingStore.read()).bindings; | ||
| if (noSession) return runtimeBoundSessionInfos(bindings); |
There was a problem hiding this comment.
Blocking: implement runner session deletion — it's required, not deferrable.
This line is why: the bindings file is the source of truth for the session drawer (runtimeBoundSessionInfos(bindings) feeds /api/sessions directly). Combined with two other facts:
runtimeBindingStore.set()is called on every runner session creation, andruntimeBindingStore.remove()is never called anywhere;- the runner host defines no
deleteSessioncapability, so/api/sessions/deletereturns 501 for runner sessions;
…every runner session ever created becomes a permanent, undeletable row in every user's session drawer, and the bindings file grows without bound. That's the same class of unbounded-growth bug as the ensureLocal issue fixed in 9427fce — fixed for local sessions, reintroduced for runner sessions.
So provider.release() and bindings.remove() aren't optional scaffolding — they're the two halves of a half-implemented feature. And thanks to the SessionHost refactor, finishing it is small (~40 lines), because the delete route is already host-agnostic (UI-state removal + session_deleted broadcast happen in the route; the host only supplies the deletion):
- Runner:
sessions.deleteRPC →releaseRunnerSession(sessionId)+unlink(sessionFile). - Provider:
deleteSession(sessionId)→ RPC + drop fromsessionFiles/subscribedSessionIds(reusingrelease()). makeRunnerSessionHost: adddeleteSession→provider.deleteSession(...), thenruntimeBindingStore.remove(sessionId)+runnerSessionRuntimeIds.delete(sessionId), return{ id: sessionId, disposition: "deleted" }.
If the runtime is unavailable, deletion of the binding should still work (remove the row so the drawer entry goes away) — arguably via the unavailable host too, so users can clean up sessions whose runtime is gone for good.
Alternative considered and rejected: if deletion really were deferrable, the dead release/remove surface should be deleted rather than shipped unused — but it isn't deferrable, because the drawer fills with immortal rows from day one.
| localStorage.setItem(knownSessionCwdsStorageKey, JSON.stringify(Array.from(cwds))); | ||
| } | ||
|
|
||
| async function responseError(response: Response) { |
There was a problem hiding this comment.
Nit: responseError and fetchRuntimeOptions are now one-line aliases for parseApiError/listRuntimes. Fine as churn-reducers, but consider inlining the imports at call sites so there's exactly one name for each operation.
|
Pushed |
ashwin-pc
force-pushed
the
runtime-first-sessions
branch
from
cfbc138 to
36c1911
Compare
|
Runtime/workbench UX follow-up plan After testing the current runtime-first branch, I think the right next step is to keep the current safe primitive (sessions are runtime-bound) but change the product UX to a VS Code-like active runtime/workbench context. Proposed model:
Important architecture point:
Implementation plan:
This means the current branch has the right lower-level invariant (runtime-bound sessions, no silent fallback), but the next UX iteration should make runtime selection a workbench context rather than a repeated per-session choice. |
|
Thoughts on the workbench pivot — the product model is right (VS Code Remote is the correct frame, and "runtime-authoritative sessions, host keeps connection config + a locator/cache" is the correct inversion; the bindings file today is a cache pretending to be truth). Two pushbacks on the plan, then edge cases — with how VS Code Remote answers each, since it has hit all of them. Pushback 1: composite identity
|
|
Confirming
No outstanding blockers from my side on this branch. Remaining discussion is about the workbench follow-up plan (see previous comment) — which should land as a separate PR. |
|
Implemented the revised VS Code-style workbench model in Key changes:
Also fixed the
Fixes:
I cleared the corrupted local locator cache after backing it up. The live server now reports 188 local sessions, all with Validation:
|
|
Correction to my previous status update: live testing against a real Apple container exposed remaining runtime integration blockers, so the PR is not merge-ready despite the passing automated suites. The agreed scope in the PR explicitly says runtime model/auth APIs must be first-class. We currently proxy The same live pass confirmed several local-only capabilities are only rejected rather than represented clearly in the runtime workbench (slash/shell commands, rename, full stats/context, git sync, extension header/footer actions, compaction cancellation). I will audit these against the intended parity contract, route the capabilities that should work, and make any intentionally unsupported capability explicit/disabled in the UI with tests. Credential isolation itself remains correct: host credentials must not be copied into a runtime silently. What is missing is runtime-scoped auth status/onboarding. |
|
Adding a security requirement from live runtime review: provider-only egress is a merge blocker for managed container runtimes. Proposed fail-closed topology for Apple container, Docker, and Podman:
This prevents tools from bypassing the proxy by clearing environment variables because the runtime network itself has no external route. Dependencies/container images are provisioned before isolation; arbitrary npm/git/curl access remains blocked at runtime. Policy modes should be explicit: |
|
Architecture decision before the next implementation pass: replace copied container auth + provider egress proxy with a host-side model broker for managed local containers.
This supersedes the proposed proxy-sidecar and automatic auth-copy design for managed local containers. Implementation and threat-model tests remain blockers for PR #43. |
|
Implemented and pushed the managed-container host model broker in Architecture now in the branch
Network policy
Real Apple-container proof
The manually copied runtime
Validation
Container auth copying and the proxy sidecar proposal are superseded by this design. PR remains draft for the separate runtime capability audit/contract work. |
1 participant
Summary
This draft PR moves the runtime-bound session work onto a reviewable branch.
It adds:
Current status
Validation from the working branch before opening this PR:
npm run typecheckpassednpm run test:unitpassednpm testpassednpm run buildpassedKnown follow-ups / review focus
This is intentionally still draft/WIP. Areas needing review and follow-up:
Notes
The current implementation avoids silent fallback for core runtime-bound chat routes, but the broader UI still needs a stricter sandbox-first policy before this should be considered ready for merge.