diff --git a/packages/opencode/src/cli/cmd/tui/context/sync.tsx b/packages/opencode/src/cli/cmd/tui/context/sync.tsx index 1dab4874e..53bb7f4de 100644 --- a/packages/opencode/src/cli/cmd/tui/context/sync.tsx +++ b/packages/opencode/src/cli/cmd/tui/context/sync.tsx @@ -172,6 +172,41 @@ export function nextSessionStatus(status: SessionStatus) { return reconcile(status) } +// Pick the bucket the session view should render. `main` is the normal case; a +// peer child (spawn.ts) runs its turns under agentID == its own sessionID, so +// attaching to one lands on agentID "main" with an empty main bucket and must +// fall back to the self-id bucket. A session whose turns ran under an ACTOR id +// has neither key — its bucket is "build-1" / "compose-1" / "general-1" — so +// without the last arm it renders a blank pane over a full transcript. +// +// ⚠️Do not delete the last arm again. An earlier revision of this branch removed +// it on the reasoning that its only population was internal machinery. That +// inference is now backwards: the route refuses a machinery session BEFORE the +// transcript is selected (routes/session/index.tsx → session/visibility.ts), so +// this fallback can no longer be the thing that renders a checkpoint-writer +// transcript. Everything that still reaches it is a session the product has +// already decided to show. Measured on the live DB, the 1313 sessions this arm +// serves split 1302 checkpoint-writer hosts (refused upstream, never arrive +// here) and 11 `session ask` fork-query hosts whose buckets are build-1 ×7, +// compose-1 ×3, general-1 ×1 — those 11 are model-spawned read-only transcripts +// and a blank pane for them is the original bug (#1964). Those counts are one +// read-only local-DB snapshot and they drift — this arm's population grew +// 1294 → 1313 across this branch's own revisions — so trust the split's shape, +// not the absolute numbers. +export function selectMessages( + buckets: Record | undefined, + agentID: string, + sessionID: string, +): M[] { + if (agentID !== "main" || buckets?.["main"]?.length) return buckets?.[agentID] ?? [] + if (buckets?.[sessionID]?.length) return buckets[sessionID] + const newest = Object.entries(buckets ?? {}) + .filter(([key, msgs]) => key !== "main" && msgs.length > 0) + .sort(([, a], [, b]) => (b.at(-1)?.id ?? "").localeCompare(a.at(-1)?.id ?? "")) + .at(0) + return newest?.[1] ?? [] +} + export const { use: useSync, provider: SyncProvider } = createSimpleContext({ name: "Sync", init: () => { @@ -935,6 +970,14 @@ export const { use: useSync, provider: SyncProvider } = createSimpleContext({ if (fullSyncedSessions.has(sessionID)) return const [session, messages, todo, diff, actors, task, children] = await Promise.all([ sdk.client.session.get({ sessionID }, { throwOnError: true }), + // ⚠️`limit` is ONE budget shared across every agent bucket, not a + // per-bucket limit. A session whose real `main` history is crowded out + // of the newest 100 therefore arrives with an empty `main` and falls + // through to a non-main bucket in selectMessages above. Measured on the + // live DB: 1 of 4613 sessions with messages. Left as-is deliberately — + // a separate concern from the render prohibition — and no server work + // is needed to fix it, since this endpoint already returns up to 1000 + // when `limit` is omitted. sdk.client.session.messages({ sessionID, limit: 100, agent_id: "*" }), sdk.client.session.todo({ sessionID }), sdk.client.session.diff({ sessionID }), @@ -942,9 +985,11 @@ export const { use: useSync, provider: SyncProvider } = createSimpleContext({ sdk.client.session.task({ sessionID }), // children aren't in the root-only session list; fetch them so the // session dialog can show the current session's child sessions. - // visible: true hides internal machinery children (checkpoint-writer - // hosts, ask-tool forks, workflow subagent sessions) — only peer - // sessions the user should see are returned. + // visible: true returns only peer children, dropping the two other + // kinds of child session that exist — the checkpoint-writer host + // (session/checkpoint.ts:851) and the `session ask` fork-query host + // (tool/session.ts:128). See Session.children for why "workflow + // subagent sessions" is not a third kind. sdk.client.session.children({ sessionID, visible: true }).catch(() => undefined), ]) setStore( diff --git a/packages/opencode/src/cli/cmd/tui/routes/session/index.tsx b/packages/opencode/src/cli/cmd/tui/routes/session/index.tsx index d8c11138f..2c2bde7c4 100644 --- a/packages/opencode/src/cli/cmd/tui/routes/session/index.tsx +++ b/packages/opencode/src/cli/cmd/tui/routes/session/index.tsx @@ -16,7 +16,7 @@ import { Dynamic } from "solid-js/web" import path from "path" import { useCurrentAgentID, useRoute, useRouteData } from "@tui/context/route" import { useProject } from "@tui/context/project" -import { useSync } from "@tui/context/sync" +import { selectMessages, useSync } from "@tui/context/sync" import { useEvent } from "@tui/context/event" import { SplitBorder } from "@tui/component/border" import { Spinner } from "@tui/component/spinner" @@ -34,6 +34,7 @@ import type { } from "@mimo-ai/sdk/v2" import { useLocal } from "@tui/context/local" import { Locale } from "@/util" +import { verifySessionRenderable, type SessionActorInput } from "@/session/visibility" import type { Tool } from "@/tool" import type { ReadTool } from "@/tool/read" import type { WriteTool } from "@/tool/write" @@ -172,16 +173,9 @@ export function Session() { const session = createMemo(() => sync.session.get(route.sessionID)) const currentAgentID = useCurrentAgentID() const actors = createMemo(() => sync.data.actor[route.sessionID] ?? []) - const messages = createMemo(() => { - const buckets = sync.data.message[route.sessionID] - const agentID = currentAgentID() - // A peer child runs its own turns under agentID == its own sessionID - // (spawn.ts), so its messages bucket under [sessionID] not ["main"]. When - // attaching to such a child at "main", fall back to its own-id bucket so the - // full session renders instead of an empty "main" view. - if (agentID === "main" && !buckets?.["main"]?.length) return buckets?.[route.sessionID] ?? [] - return buckets?.[agentID] ?? [] - }) + const messages = createMemo(() => + selectMessages(sync.data.message[route.sessionID], currentAgentID(), route.sessionID), + ) const permissions = createMemo(() => sync.data.permission[route.sessionID] ?? []) const questions = createMemo(() => sync.data.question[route.sessionID] ?? []) const visible = createMemo( @@ -260,6 +254,37 @@ export function Session() { return } + // The prohibition. Every way of reaching this route hands a raw session id + // straight to the renderer and bypasses both hiding layers: -s/--session + // (thread.ts → app.tsx), `attach --session`, POST /tui/select-session, POST + // /tui/event, the session tool's `switch`, MIMOCODE_ROUTE, plugin + // navigate("session", …) and the session-list dialog's child injection. This + // effect is the one point all of them must pass, so the refusal lives here + // rather than on any single entry point. What counts as forbidden lives in + // session/visibility.ts: a host for a RUNTIME-spawned agent, which today + // means the checkpoint writer. It reads the session's own actor rows, so no + // parent round-trip is needed. + const verdict = await verifySessionRenderable(result.data, (sessionID) => + // `throwOnError` is load-bearing, not tidiness: without it this client + // RESOLVES `{ data: undefined }` on an HTTP error, which the classifier + // reads as "this session has no actor rows" and renders. The failure has to + // arrive as a rejection for the gate to see it as unverified rather than as + // verified-absent. + // SessionActorsResponses[200] is generated as `unknown`, so the shape is + // asserted here exactly as sync.tsx does for the same endpoint. + sdk.client.session + .actors({ sessionID }, { throwOnError: true }) + .then((res) => res.data as SessionActorInput[] | undefined), + ) + if (!verdict.renderable) { + toast.show({ + message: `Cannot open session: ${verdict.reason}`, + variant: "error", + }) + navigate({ type: "home" }) + return + } + if (result.data.workspaceID !== previousWorkspace) { project.workspace.set(result.data.workspaceID) diff --git a/packages/opencode/src/session/session.ts b/packages/opencode/src/session/session.ts index 367c12189..319436b9d 100644 --- a/packages/opencode/src/session/session.ts +++ b/packages/opencode/src/session/session.ts @@ -527,10 +527,22 @@ export const layer: Layer.Layer d .select({ session_id: ActorRegistryTable.session_id }) diff --git a/packages/opencode/src/session/visibility.ts b/packages/opencode/src/session/visibility.ts new file mode 100644 index 000000000..656bb6358 --- /dev/null +++ b/packages/opencode/src/session/visibility.ts @@ -0,0 +1,201 @@ +import { SYSTEM_SPAWNED_AGENT_TYPES } from "@/agent/config" +import { Log } from "@/util" + +const log = Log.create({ service: "session.visibility" }) + +/** + * Which sessions the UI is allowed to display. + * + * Product prohibition: a session that exists only to host a RUNTIME-spawned + * agent must never be rendered — not merely omitted from lists. The scope is + * narrow on purpose: only the runtime-spawned writer / dream / distill hosts are + * clearly not conversations, and everything else a user can reach should render. + * The prohibition exists because navigation was landing inside a writer host. + * + * ## Exactly three code paths create a child session + * + * Enumerated by grepping every `create({ parentID })` in src/ (non-test): + * + * 1. `actor/spawn.ts:674` — a PEER child (`session create`, `mode: "peer"`, + * `session_id === actor_id === child.id`). A real conversation the + * subagent dialog already shows. Renderable. + * 2. `tool/session.ts:128` — the `session ask` fork-query host (`forkQuery`, + * title `ask: `, `mode: "subagent"`, and `agentType` is the + * TARGET's own last-assistant agent, so `build` / `compose` / `general`). + * MODEL-spawned and read-only. Renderable — if a compose or workflow run + * goes wrong, its side-question transcript is exactly what you want. + * 3. `session/checkpoint.ts:851` — the checkpoint-writer host, spawned with + * `agentType: "checkpoint-writer"` (`checkpoint.ts:878`). RUNTIME-spawned + * bookkeeping. The one population this file exists to refuse. + * + * ⚠️"workflow subagent sessions" is NOT a fourth path: a workflow's `agent()` + * calls `actor.spawn({ mode: "subagent", sessionID: input.sessionID })` + * (`workflow/runtime.ts:814-816`, `:945-948`) — it registers an actor under the + * workflow's OWN session and creates no child session at all. An earlier + * revision of this comment (and of `Session.children`'s) named it as a hidden + * session category; it never was one. + * + * ## The criterion: the agent-type set, not `mode !== "peer"` + * + * `SYSTEM_SPAWNED_AGENT_TYPES` (`agent/config.ts`) already means "spawned by the + * runtime, NOT by the model", and is already the discriminator for the same kind + * of decision elsewhere — permission routing (`agent/agent.ts`: a system agent + * auto-denies because there is no human to answer) and the + * prune/bootstrap/memory/recall scan skips. + * + * The predicate this file used to carry — *renderable iff root, or present among + * the parent's `visible: true` children* — was a PROXY for "internal machinery", + * because `visible: true` resolves to `ActorRegistry.mode === "peer"` + * (`session/session.ts` `children()`). Measured on the live DB the proxy + * over-blocked by 28 sessions: it refused all 11 `ask:` forks and all 17 + * legacy no-actor-row `@explore`/`@general` subagent transcripts, none of which + * is machinery. + * + * ## Fail OPEN, and why the live DB settles it + * + * Keying on an agent-type set inverts the old fail-closed default: a session + * with no actor row is not in the set, so it renders. That is the intended + * answer, not an accident: + * + * - All 17 child sessions in the live DB with NO actor row are pre-registry + * (Jan–Feb 2026) `@explore` / `@general` mention subagents. 17/17 hold + * their messages under `main`; 0/17 has a non-main bucket, i.e. not one of + * them is an actor-hosted machinery session. Fail-closed refused all 17. + * - A checkpoint-writer host cannot present as "no actor row" once it has + * anything to leak: `spawnSubagent` registers the row (`actor/spawn.ts:731`) + * BEFORE it forks the work that writes the first message (`:762`), and the + * row is `ON DELETE CASCADE` on the session, so it cannot be outlived. + * 1302/1302 writer hosts in the live DB carry their row. The residual race + * window — created, not yet registered — has zero messages, so fail-open's + * worst case there is an EMPTY pane, not a leaked machinery transcript. + * + * A prohibition that fails open is still a prohibition when the population it + * targets provably cannot arrive un-annotated; over-refusing real transcripts is + * the more expensive error, and it is the one the user actually hit. + * + * ## "Unreadable" is NOT "absent" — only the first of the two fails open + * + * The argument above is about rows that are genuinely ABSENT, and it rests on a + * measured population: all 17 no-row children are real transcripts. It does not + * transfer to rows that EXIST but could not be READ. That is a different + * population — every child is a candidate, and of the 1504 children in the live + * DB 1304 carry a system-spawned row — so a read failure that fell through to + * the fail-open arm would render a checkpoint-writer host in roughly six cases + * out of seven. A filter may fail open; a GATE must fail closed, and this is a + * gate. + * + * So `classifySession` never learns about read failures: `undefined` means "this + * session has no rows" and nothing else. A failed read goes to + * `classifyUnreadableActors` instead, which refuses with a DISTINCT reason + * ("could not verify") so an operator can tell a broken read from the product + * prohibition, and logs the session id and the cause — the swallowed + * `catch(() => undefined)` this replaced reported a state it had not verified + * and left no trace of having failed. + * + * Failing closed costs a legitimate session that is briefly unopenable, which is + * the OTHER error this file was narrowed to avoid. Two things bound that cost: + * the refusal is only reached after one retry, which is what a single dropped + * request costs; and a root never reaches it at all, because a root's verdict + * does not depend on its rows, so an unreadable read cannot change it. + */ + +/** Minimal shape needed to classify — `parentID` is a nullable DB column. */ +export interface SessionVisibilityInput { + readonly id: string + readonly parentID?: string | null +} + +/** The two `ActorRegistry` fields the verdict reads. */ +export interface SessionActorInput { + readonly mode: string + readonly agent: string +} + +export type RenderVerdict = { readonly renderable: true } | { readonly renderable: false; readonly reason: string } + +const RENDERABLE: RenderVerdict = { renderable: true } + +/** + * `actors` must be the session's OWN `ActorRegistry` rows + * (`ActorRegistry.listBySession` / `GET /session/:id/actors`) — the rows keyed by + * `session_id === info.id`. `undefined` means the session HAS no rows, which is + * renderable: see the fail-open note above. A read that FAILED must never be + * passed here as `undefined`; route it to `classifyUnreadableActors`, which is + * the whole point of keeping the two states apart. + */ +export function classifySession( + info: SessionVisibilityInput, + actors: readonly SessionActorInput[] | undefined, +): RenderVerdict { + // Roots are what the session list shows, and user-initiated forks are roots + // too (Session.fork → createNext, no parentID). Checked first and not merely + // as an optimisation: one real root in the live DB carries a + // `checkpoint-writer` actor row, because before the writer got its own child + // session it registered under the session it was checkpointing. Reading the + // agent set without this guard would refuse that user's own conversation. + if (!info.parentID) return RENDERABLE + // A peer child is a conversation by construction (spawn.ts registers + // session_id === actor_id === child.id), and it is what the subagent dialog + // lists. Checked before the agent set for the same reason as the root guard: + // whatever a peer child happened to RUN must not decide what it IS. Because the + // default below is renderable, this arm only changes an outcome for a peer that + // ALSO carries a system-spawned row — narrow, but that is precisely the overlap + // the one writer-carrying root in the live DB exhibits, one level up. + if (actors?.some((actor) => actor.mode === "peer")) return RENDERABLE + const system = actors?.find((actor) => SYSTEM_SPAWNED_AGENT_TYPES.has(actor.agent)) + if (system) + return { + renderable: false, + reason: `${info.id} hosts the runtime-spawned ${system.agent} agent, not a conversation`, + } + return RENDERABLE +} + +/** + * The verdict for a session whose own actor rows could not be READ, as opposed to + * a session that genuinely has none. Fails closed for a child, and logs the + * session id with the underlying cause so the failure is never silent. + * + * A root stays renderable: `classifySession` decides a root without looking at + * its rows at all, so an unreadable read cannot change its verdict, and routing + * roots through here keeps the two enforcement points from disagreeing — the + * transport wrapper below returns before it ever fetches, while the session + * tool's `switch` reads rows unconditionally and so does reach this function. + * + * The reason deliberately says "could not verify" and never names an agent: the + * user-visible string has to distinguish a broken read from the prohibition. + */ +export function classifyUnreadableActors(info: SessionVisibilityInput, cause: unknown): RenderVerdict { + if (!info.parentID) return RENDERABLE + log.error("actor rows unreadable, refusing to render", { sessionID: info.id, cause }) + return { + renderable: false, + reason: `could not verify whether ${info.id} is a conversation: reading its actor rows failed`, + } +} + +/** + * Same rule, for callers that reach the actor registry over a transport rather + * than in-process. `fetchActors` MUST REJECT when the read fails — a client that + * resolves `{ data: undefined }` on an HTTP error arrives here as "no rows" and + * fails open, so the caller passes `throwOnError`. + */ +export async function verifySessionRenderable( + info: SessionVisibilityInput, + fetchActors: (sessionID: string) => Promise, +): Promise { + if (!info.parentID) return RENDERABLE + // One retry, then refuse. A single dropped request is the common failure and is + // not worth making a real transcript unopenable; a failure that survives a + // second attempt is not transient. Bounded on purpose — a gate that retries + // until it succeeds is a gate that never closes. + let cause: unknown + for (let attempt = 0; attempt < 2; attempt++) { + try { + return classifySession(info, await fetchActors(info.id)) + } catch (error) { + cause = error + } + } + return classifyUnreadableActors(info, cause) +} diff --git a/packages/opencode/src/tool/session.ts b/packages/opencode/src/tool/session.ts index 173fc7164..2f1ad183f 100644 --- a/packages/opencode/src/tool/session.ts +++ b/packages/opencode/src/tool/session.ts @@ -5,8 +5,9 @@ import DESCRIPTION from "./session.txt" import SHELL_DESCRIPTION from "./session.shell.txt" import { tokenize } from "./shell-tokenize" import z from "zod" -import { Effect, Deferred } from "effect" +import { Cause, Effect, Deferred } from "effect" import { Session } from "@/session" +import { classifySession, classifyUnreadableActors } from "@/session/visibility" import { Worktree } from "@/worktree" import { Instance } from "@/project/instance" import { InstanceRef } from "@/effect/instance-ref" @@ -929,6 +930,47 @@ export const SessionTool = Tool.define( } if (op.action === "switch") { + // Same prohibition the renderer enforces (cli/cmd/tui/routes/session/index.tsx). + // The renderer is the choke point, but refusing here too is what reaches + // the model mid-turn: a silent no-op would just make it retry. + // NotFoundError is a synchronous throw inside an Effect.fn (a DEFECT, not + // a typed failure — see the Worktree.create note above), so Effect.catch + // can't see it; Effect.exit captures any non-success. + const targetExit = yield* Effect.exit(sessions.get(op.sessionID as SessionID)) + if (targetExit._tag !== "Success") + return { + title: `Refused switch to ${op.sessionID}`, + output: `Refused to move the UI to ${op.sessionID}: no such session. Run \`session list\` to see the child sessions you can switch to.`, + metadata: { sessionID: op.sessionID } as Metadata, + } + const target = targetExit.value + // Same shared helpers the renderer uses, so the criterion cannot drift + // between the two enforcement points: they read the TARGET's own actor + // rows, not its parent's child list. + // + // listBySession is typed as never-failing, so a DB error surfaces as a + // defect — the same shape as the NotFoundError above, and equally + // invisible to Effect.catch. Left unwrapped it would abort the whole tool + // call, which reaches the model as a crash rather than as a decision; and + // "rows could not be read" must NOT reach classifySession, because there + // it would be indistinguishable from "this session has no rows" and would + // fail open onto exactly the population the prohibition exists to refuse. + const actorsExit = yield* Effect.exit(actorReg.listBySession(target.id as SessionID)) + const verdict = + actorsExit._tag === "Success" + ? classifySession(target, actorsExit.value) + : classifyUnreadableActors(target, Cause.pretty(actorsExit.cause)) + if (!verdict.renderable) + return { + title: `Refused switch to ${op.sessionID}`, + output: + `Refused to move the UI to ${op.sessionID}: ${verdict.reason}. ` + + (actorsExit._tag === "Success" + ? `A session hosting a runtime-spawned agent is never rendered. ` + : `That is a read failure, not a prohibition: retry the switch, and if it keeps failing the actor registry is broken. `) + + `Run \`session list\` to see the child sessions you can switch to, or switch to this session's parent instead.`, + metadata: { sessionID: op.sessionID } as Metadata, + } yield* Effect.promise(() => Bus.publish(TuiEvent.SessionSelect, { sessionID: op.sessionID as SessionID })) return { title: `Switched to ${op.sessionID}`, diff --git a/packages/opencode/test/cli/tui/select-messages.test.ts b/packages/opencode/test/cli/tui/select-messages.test.ts new file mode 100644 index 000000000..8e5f59f63 --- /dev/null +++ b/packages/opencode/test/cli/tui/select-messages.test.ts @@ -0,0 +1,76 @@ +import { describe, test, expect } from "bun:test" +import { bucketMessages, selectMessages } from "../../../src/cli/cmd/tui/context/sync" + +const msg = (id: string, agentID?: string) => ({ id, agentID }) as any + +describe("selectMessages", () => { + test("renders the main bucket for a normal session", () => { + const buckets = bucketMessages([msg("m1"), msg("m2", "explore-1")]) + expect(selectMessages(buckets, "main", "ses_root")).toEqual([msg("m1")]) + }) + + test("renders the requested subagent bucket when the route carries an agentID", () => { + const buckets = bucketMessages([msg("m1"), msg("m2", "explore-1")]) + expect(selectMessages(buckets, "explore-1", "ses_root")).toEqual([msg("m2", "explore-1")]) + }) + + test("falls back to the self-id bucket for a peer child (spawn.ts)", () => { + const buckets = bucketMessages([msg("m1", "ses_peer"), msg("m2", "ses_peer")]) + expect(selectMessages(buckets, "main", "ses_peer")).toEqual([msg("m1", "ses_peer"), msg("m2", "ses_peer")]) + }) + + // REWRITTEN TWICE — read the history before touching these, they have flipped + // once already. + // + // Originally they asserted that an actor-bucketed session renders (the + // blank-transcript fix). A later commit on this same branch INVERTED them to + // `toEqual([])` and deleted the fallback, on the reasoning that arm 4's only + // population was internal machinery which the new render prohibition made + // unreachable anyway. + // + // That reasoning has been narrowed and these are back to asserting rendering. + // The prohibition no longer keys on "not a peer child" but on the session + // hosting a RUNTIME-spawned agent (session/visibility.ts → + // SYSTEM_SPAWNED_AGENT_TYPES). Measured on the live DB, the 1313 sessions this + // arm serves are 1302 checkpoint-writer hosts — still refused, upstream at the + // route, before the selector ever runs — plus 11 `session ask` fork-query hosts + // (buckets build-1 ×7, compose-1 ×3, general-1 ×1) which are model-spawned + // read-only transcripts the product does display. Those 11 are precisely the + // blank pane #1964 was opened to fix, so the arm is load-bearing again. + // + // The inversion that makes it safe: machinery is refused BEFORE bucket + // selection, so this fallback can no longer be what renders a checkpoint-writer + // transcript. + test("renders an actor-hosted session whose only bucket is its actor id", () => { + const buckets = bucketMessages([msg("m1", "build-1"), msg("m2", "build-1"), msg("m3", "build-1")]) + expect(selectMessages(buckets, "main", "ses_askfork")).toEqual([ + msg("m1", "build-1"), + msg("m2", "build-1"), + msg("m3", "build-1"), + ]) + }) + + test("picks the newest bucket when an empty-main session has several actor buckets", () => { + const buckets = bucketMessages([msg("m1", "general-1"), msg("m9", "general-2")]) + expect(selectMessages(buckets, "main", "ses_actorhost")).toEqual([msg("m9", "general-2")]) + }) + + // The self-id bucket must still win over a newer actor bucket: a peer child that + // spawned subagents has both, and its own conversation is what to show. + test("prefers the peer self-id bucket over a newer actor bucket", () => { + const buckets = bucketMessages([msg("m1", "ses_peer"), msg("m9", "explore-1")]) + expect(selectMessages(buckets, "main", "ses_peer")).toEqual([msg("m1", "ses_peer")]) + }) + + test("an explicit agentID still reaches an actor bucket (subagent dialog is unaffected)", () => { + const buckets = bucketMessages([msg("m1", "checkpoint-writer-1")]) + expect(selectMessages(buckets, "checkpoint-writer-1", "ses_actorhost")).toEqual([ + msg("m1", "checkpoint-writer-1"), + ]) + }) + + test("stays empty when the session genuinely has no messages", () => { + expect(selectMessages(undefined, "main", "ses_new")).toEqual([]) + expect(selectMessages({}, "main", "ses_new")).toEqual([]) + }) +}) diff --git a/packages/opencode/test/session/internal-session-prohibition.test.ts b/packages/opencode/test/session/internal-session-prohibition.test.ts new file mode 100644 index 000000000..739a89b9c --- /dev/null +++ b/packages/opencode/test/session/internal-session-prohibition.test.ts @@ -0,0 +1,563 @@ +import { afterEach, describe, expect, setDefaultTimeout } from "bun:test" +import { Effect, Layer } from "effect" + +// Live tests: real sessions + the session tool's full layer stack. +setDefaultTimeout(30_000) + +import { Agent } from "../../src/agent/agent" +import { Actor } from "../../src/actor/spawn" +import { ActorRegistry } from "../../src/actor/registry" +import { Bus } from "../../src/bus" +import { Config } from "../../src/config" +import { Git } from "../../src/git" +import { Instance } from "../../src/project/instance" +import { Provider } from "../../src/provider" +import { Session } from "../../src/session" +import { classifySession, classifyUnreadableActors, verifySessionRenderable } from "../../src/session/visibility" +import { MessageID, SessionID } from "../../src/session/schema" +import { Truncate } from "../../src/tool" +import { SessionTool } from "../../src/tool/session" +import { TuiEvent } from "../../src/cli/cmd/tui/event" +import { Worktree } from "../../src/worktree" +import * as CrossSpawnSpawner from "../../src/effect/cross-spawn-spawner" +import { Log } from "../../src/util" +import { provideTmpdirInstance } from "../fixture/fixture" +import { testEffect } from "../lib/effect" + +void Log.init({ print: false }) + +afterEach(async () => { + await Instance.disposeAll() +}) + +const env = Layer.mergeAll( + Session.defaultLayer, + ActorRegistry.defaultLayer, + Provider.defaultLayer, + Truncate.defaultLayer, + Agent.defaultLayer, + CrossSpawnSpawner.defaultLayer, + Bus.defaultLayer, + Config.defaultLayer, + Worktree.defaultLayer, + Git.defaultLayer, + Actor.defaultLayer, +) + +const it = testEffect(env) + +const ctx = (sessionID: string) => ({ + sessionID: SessionID.make(sessionID), + messageID: MessageID.ascending(), + agent: "build", + actorID: "main", + abort: new AbortController().signal, + extra: {}, + messages: [], + metadata: () => Effect.void, + ask: () => Effect.void, +}) + +/** + * Builds the five shapes that matter, exactly as they exist in the real DB: + * - peer child → actor row keyed (session_id = child.id, actor_id = child.id), mode "peer" + * - writer host → actor row keyed (session_id = child.id, actor_id = "checkpoint-writer-1"), mode "subagent" + * - ask fork → tool/session.ts:128's forkQuery host: mode "subagent" whose + * agent is the TARGET's agent ("build"), title `ask: …` + * - unregistered → child session with no actor row at all (17 such children + * exist in the live DB: pre-registry @explore/@general subagents) + * - writerRoot → a ROOT that carries a checkpoint-writer row, because + * before the writer got its own child session it registered + * under the session it was checkpointing. One such root + * exists in the live DB and it is a real conversation. + */ +const scaffold = Effect.gen(function* () { + const sessions = yield* Session.Service + const actorReg = yield* ActorRegistry.Service + + const root = yield* sessions.create({ title: "root" }) + + const peer = yield* sessions.create({ parentID: root.id as SessionID, title: "general: do a thing" }) + yield* actorReg.register({ + sessionID: peer.id as SessionID, + actorID: peer.id, + mode: "peer", + agent: "general", + description: "peer child", + contextMode: "none", + contextWatermark: undefined, + background: false, + lifecycle: "persistent", + tools: undefined, + }) + + const writerHost = yield* sessions.create({ parentID: root.id as SessionID, title: "checkpoint-writer: root" }) + yield* actorReg.register({ + sessionID: writerHost.id as SessionID, + actorID: "checkpoint-writer-1", + mode: "subagent", + agent: "checkpoint-writer", + description: "writer", + contextMode: "none", + contextWatermark: undefined, + background: true, + lifecycle: "ephemeral", + tools: undefined, + }) + + const askFork = yield* sessions.create({ parentID: root.id as SessionID, title: "ask: what is the status" }) + yield* actorReg.register({ + sessionID: askFork.id as SessionID, + actorID: "build-1", + mode: "subagent", + agent: "build", + description: "fork-query", + contextMode: "full", + contextWatermark: undefined, + background: false, + lifecycle: "ephemeral", + tools: undefined, + }) + + const unregistered = yield* sessions.create({ + parentID: root.id as SessionID, + title: "Explore codebase structure (@explore subagent)", + }) + + const writerRoot = yield* sessions.create({ title: "a real conversation that got checkpointed" }) + yield* actorReg.register({ + sessionID: writerRoot.id as SessionID, + actorID: "checkpoint-writer-1", + mode: "subagent", + agent: "checkpoint-writer", + description: "writer registered under the session it checkpointed", + contextMode: "none", + contextWatermark: undefined, + background: true, + lifecycle: "ephemeral", + tools: undefined, + }) + + return { sessions, root, peer, writerHost, askFork, unregistered, writerRoot } +}) + +describe("runtime-spawned agent hosts are never rendered — the rule", () => { + it.live("a root is renderable without consulting its actor rows at all", () => + Effect.gen(function* () { + let asked = 0 + const verdict = yield* Effect.promise(() => + verifySessionRenderable({ id: "ses_root" }, async () => { + asked++ + return [] + }), + ) + expect(verdict.renderable).toBe(true) + expect(asked).toBe(0) + }), + ) + + it.live("parent_id arriving as SQL NULL is still a root (nullable-column rule)", () => + Effect.sync(() => { + expect(classifySession({ id: "ses_root", parentID: null }, undefined).renderable).toBe(true) + }), + ) + + // REWRITTEN (second time). Was, at 0b458f634: "a child whose actor rows cannot + // be read is still rendered — the prohibition fails open", whose first + // assertion was `verifySessionRenderable(child, () => { throw }).renderable === + // true`. That single test asserted BOTH states at once, because + // `.catch(() => undefined)` made a failed read arrive at the classifier as "no + // rows" — so it pinned the collapse it was meant to describe. + // + // The fail-open evidence only ever covered rows that are genuinely ABSENT: all + // 17 no-actor-row children in the live DB are real pre-registry + // @explore/@general transcripts stored under `main`. It says nothing about rows + // that exist and could not be READ, where the population is every child — 1304 + // of the 1504 live children carry a system-spawned row. So the two states are + // now split across this test (absent, still fails open, assertions kept + // verbatim) and the two that follow (unreadable, fails closed). Nothing was + // relaxed: the `throw` case moved from asserting `renderable === true` to + // asserting `renderable === false` plus a distinct reason. + it.live("absent actor rows still fail open — a child with no rows renders", () => + Effect.sync(() => { + expect(classifySession({ id: "ses_kid", parentID: "ses_root" }, undefined).renderable).toBe(true) + expect(classifySession({ id: "ses_kid", parentID: "ses_root" }, []).renderable).toBe(true) + }), + ) + + it.live("an UNREADABLE actor read is refused, and never with the prohibition's reason", () => + Effect.sync(() => { + const verdict = classifyUnreadableActors({ id: "ses_kid", parentID: "ses_root" }, new Error("boom")) + expect(verdict.renderable).toBe(false) + if (!verdict.renderable) { + expect(verdict.reason).toContain("ses_kid") + // An operator has to be able to tell a broken read from the product + // prohibition, so these two reasons must never converge. + expect(verdict.reason).toContain("could not verify") + expect(verdict.reason).not.toContain("runtime-spawned") + } + // A root is decided without reading rows at all, so an unreadable read + // cannot make one unopenable. This is also what keeps the switch path — which + // reads rows unconditionally — in step with the renderer, which returns + // before it ever fetches. + expect(classifyUnreadableActors({ id: "ses_root" }, new Error("boom")).renderable).toBe(true) + }), + ) + + it.live("renderer path: a read that keeps failing is refused after exactly one retry", () => + Effect.gen(function* () { + let attempts = 0 + const verdict = yield* Effect.promise(() => + verifySessionRenderable({ id: "ses_kid", parentID: "ses_root" }, async () => { + attempts++ + throw new Error("network") + }), + ) + expect(verdict.renderable).toBe(false) + if (!verdict.renderable) expect(verdict.reason).toContain("could not verify") + // Bounded: one retry, not a loop. A gate that retries until it succeeds is a + // gate that never closes. + expect(attempts).toBe(2) + }), + ) + + // The other half of the retry decision: failing closed must not punish the + // transient blip the branch was narrowed to avoid, and the rows recovered by + // the retry must be classified normally rather than as unverified. + it.live("renderer path: one transient failure is retried, and the recovered rows decide", () => + Effect.gen(function* () { + let attempts = 0 + const verdict = yield* Effect.promise(() => + verifySessionRenderable({ id: "ses_kid", parentID: "ses_root" }, async () => { + attempts++ + if (attempts === 1) throw new Error("blip") + return [{ mode: "subagent", agent: "checkpoint-writer" }] + }), + ) + expect(attempts).toBe(2) + expect(verdict.renderable).toBe(false) + if (!verdict.renderable) { + expect(verdict.reason).toContain("checkpoint-writer") + expect(verdict.reason).not.toContain("could not verify") + } + }), + ) + + it.live("the refusal is logged with the session id and the underlying error", () => + Effect.promise(async () => { + // Log.init({ print: false }) opens a real file sink and Log.file() names it, + // so the record is asserted rather than assumed. The swallowed catch this + // replaced left no trace at all. + await Log.init({ print: false }) + const logfile = Log.file() + expect(logfile).not.toBe("") + await verifySessionRenderable({ id: "ses_logged", parentID: "ses_root" }, async () => { + throw new Error("actors-endpoint-exploded") + }) + await Log.flush() + const written = await Bun.file(logfile).text() + expect(written).toContain("actor rows unreadable") + expect(written).toContain("ses_logged") + expect(written).toContain("actors-endpoint-exploded") + }), + ) + + it.live("dream and distill are refused for the same reason as checkpoint-writer", () => + Effect.sync(() => { + for (const agent of ["checkpoint-writer", "dream", "distill"]) { + const verdict = classifySession({ id: "ses_kid", parentID: "ses_root" }, [{ mode: "subagent", agent }]) + expect(verdict.renderable).toBe(false) + if (!verdict.renderable) expect(verdict.reason).toContain(agent) + } + }), + ) + + // Ordering matters, not just membership: a peer child that RAN a system agent + // must stay renderable, so the peer arm has to be reached before the agent set. + it.live("a peer row wins over a system-spawned row on the same session", () => + Effect.sync(() => { + expect( + classifySession({ id: "ses_kid", parentID: "ses_root" }, [ + { mode: "peer", agent: "general" }, + { mode: "subagent", agent: "checkpoint-writer" }, + ]).renderable, + ).toBe(true) + }), + ) +}) + +describe("runtime-spawned agent hosts are never rendered — renderer path", () => { + it.live("refuses only the checkpoint-writer host; admits root, peer, ask fork and unregistered child", () => + provideTmpdirInstance(() => + Effect.gen(function* () { + const { root, peer, writerHost, askFork, unregistered, writerRoot } = yield* scaffold + const actorReg = yield* ActorRegistry.Service + + // The renderer resolves the verdict from the session's own actor rows, + // which over the SDK is GET /session/:id/actors → listBySession. + const fetchActors = (sessionID: string) => + Effect.runPromise( + actorReg + .listBySession(sessionID as SessionID) + .pipe(Effect.map((rows) => rows.map((r) => ({ mode: r.mode, agent: r.agent })))) as Effect.Effect< + { mode: string; agent: string }[] + >, + ) + + const check = (info: { id: string; parentID?: string }) => + Effect.promise(() => verifySessionRenderable(info, fetchActors)) + + expect((yield* check(root)).renderable).toBe(true) + expect((yield* check(peer)).renderable).toBe(true) + + const writerVerdict = yield* check(writerHost) + expect(writerVerdict.renderable).toBe(false) + if (!writerVerdict.renderable) { + expect(writerVerdict.reason).toContain(writerHost.id) + expect(writerVerdict.reason).toContain("checkpoint-writer") + } + + // The narrowing. Both of these were refused by the previous criterion: + // neither owns a mode:"peer" row, so neither appeared among its parent's + // visible children. Both are real transcripts. + expect((yield* check(askFork)).renderable).toBe(true) + expect((yield* check(unregistered)).renderable).toBe(true) + + // A root is never classified by its actor rows, so the real conversation + // that carries a checkpoint-writer row stays renderable. + expect((yield* check(writerRoot)).renderable).toBe(true) + }), + ), + ) + + // There is no Solid render harness for the session route, so the wiring of the + // guard into the route effect is asserted at the source level. Narrow on + // purpose: it pins only that the refusal runs, and runs before the transcript + // is synced. Without it, deleting the guard block in index.tsx would break the + // prohibition while every behavioural test above still passed. + it.live("the session route wires the guard in before it syncs the transcript", () => + Effect.promise(async () => { + const src = await Bun.file( + new URL("../../src/cli/cmd/tui/routes/session/index.tsx", import.meta.url).pathname, + ).text() + const guardAt = src.indexOf("verifySessionRenderable(") + const syncAt = src.indexOf("sync.session.sync(route.sessionID)") + expect(guardAt).toBeGreaterThan(-1) + expect(syncAt).toBeGreaterThan(-1) + expect(guardAt).toBeLessThan(syncAt) + }), + ) + // Source-level for the same reason as the wiring assertion above — no Solid + // render harness — and additionally because what it pins is a property of the + // SDK CALL, not of any function under test. Load-bearing: without + // `throwOnError` this client RESOLVES `{ data: undefined }` on an HTTP error + // (gen/client/client.gen.ts:167-177), so a 500 from /session/:id/actors reaches + // classifySession as "this session has no rows" and renders. That reopens the + // fail-open leak with visibility.ts completely untouched, which is why the + // separation cannot be enforced in visibility.ts alone. + it.live("the route's actor fetch rejects on an HTTP error rather than resolving undefined", () => + Effect.promise(async () => { + const src = await Bun.file( + new URL("../../src/cli/cmd/tui/routes/session/index.tsx", import.meta.url).pathname, + ).text() + const guardAt = src.indexOf("verifySessionRenderable(") + expect(guardAt).toBeGreaterThan(-1) + const call = src.slice(guardAt, src.indexOf("if (!verdict.renderable)", guardAt)) + expect(call).toContain("sdk.client.session") + expect(call).toContain(".actors({ sessionID }, { throwOnError: true })") + }), + ) +}) + +describe("runtime-spawned agent hosts are never rendered — session tool switch path", () => { + it.live("switch refuses a checkpoint-writer host without publishing SessionSelect", () => + provideTmpdirInstance(() => + Effect.gen(function* () { + const { root, writerHost } = yield* scaffold + + const seen: string[] = [] + const unsub = Bus.subscribe(TuiEvent.SessionSelect, (event) => seen.push(event.properties.sessionID)) + + const info = yield* SessionTool + const tool = yield* info.init() + const result = yield* tool.execute({ operation: { action: "switch", sessionID: writerHost.id } }, ctx(root.id)) + + unsub() + expect(seen).toEqual([]) + expect(result.title).toContain("Refused") + expect(result.output).toContain("checkpoint-writer") + // The refusal must be actionable for the model mid-turn. + expect(result.output).toContain("session list") + }), + ), + ) + + // REWRITTEN, was: "switch refuses an unregistered child fork without + // publishing", asserting `seen === []` and a "Refused" title. Same criterion + // change as the fail-open rewrite above — a child with no actor row is no + // longer machinery by default, so `switch` must now move the UI there. Kept as + // a test rather than deleted because it is the discriminator for the two + // enforcement points staying in step: if only the renderer had been narrowed, + // the model would still be refused here and the UI would still be reachable + // by -s, which is the split the shared helper exists to prevent. + it.live("switch now publishes for an unregistered child and for an ask fork", () => + provideTmpdirInstance(() => + Effect.gen(function* () { + const { root, unregistered, askFork } = yield* scaffold + + const seen: string[] = [] + const unsub = Bus.subscribe(TuiEvent.SessionSelect, (event) => seen.push(event.properties.sessionID)) + + const info = yield* SessionTool + const tool = yield* info.init() + const bare = yield* tool.execute({ operation: { action: "switch", sessionID: unregistered.id } }, ctx(root.id)) + const ask = yield* tool.execute({ operation: { action: "switch", sessionID: askFork.id } }, ctx(root.id)) + + unsub() + expect(seen).toEqual([unregistered.id, askFork.id]) + expect(bare.title).toContain("Switched to") + expect(ask.title).toContain("Switched to") + }), + ), + ) + + it.live("switch refuses an id with no session row without publishing", () => + provideTmpdirInstance(() => + Effect.gen(function* () { + const { root } = yield* scaffold + + const seen: string[] = [] + const unsub = Bus.subscribe(TuiEvent.SessionSelect, (event) => seen.push(event.properties.sessionID)) + + const info = yield* SessionTool + const tool = yield* info.init() + const result = yield* tool.execute( + { operation: { action: "switch", sessionID: "ses_doesnotexist" } }, + ctx(root.id), + ) + + unsub() + expect(seen).toEqual([]) + expect(result.output).toContain("no such session") + }), + ), + ) + + it.live("switch still publishes for a peer child and for a root", () => + provideTmpdirInstance(() => + Effect.gen(function* () { + const { root, peer } = yield* scaffold + + const seen: string[] = [] + const unsub = Bus.subscribe(TuiEvent.SessionSelect, (event) => seen.push(event.properties.sessionID)) + + const info = yield* SessionTool + const tool = yield* info.init() + const peerResult = yield* tool.execute({ operation: { action: "switch", sessionID: peer.id } }, ctx(root.id)) + const rootResult = yield* tool.execute({ operation: { action: "switch", sessionID: root.id } }, ctx(root.id)) + + unsub() + expect(seen).toEqual([peer.id, root.id]) + expect(peerResult.title).toContain("Switched to") + expect(rootResult.title).toContain("Switched to") + }), + ), + ) + + // The switch path is where classifySession's OWN root guard is load-bearing: + // it calls the helper unconditionally with listBySession's rows, whereas + // verifySessionRenderable returns early for a root and never fetches any. So + // only this test fails if the agent-set check is moved above the root guard — + // and getting that wrong refuses a real user conversation, which is what the + // one such root in the live DB is. + // The discriminator that keeps the two enforcement points in step for the NEW + // state. `unregistered` is chosen deliberately: the test above publishes it when + // the rows read cleanly (absent rows fail open), and this one refuses the very + // same session when the read FAILS. If the two states were ever collapsed + // again — by restoring a swallowing catch here or by handing the failure to + // classifySession — this test would publish and go green. + it.live("switch refuses when the actor rows cannot be READ, with the unverifiable reason", () => + provideTmpdirInstance(() => + Effect.gen(function* () { + const { root, unregistered } = yield* scaffold + const real = yield* ActorRegistry.Service + // listBySession is typed as never-failing, so the only way it breaks is a + // defect — exactly the shape Effect.catch cannot see and Effect.exit can. + const broken = Layer.succeed(ActorRegistry.Service, { + ...real, + listBySession: () => Effect.die(new Error("actor_registry read failed")), + }) + + const seen: string[] = [] + const unsub = Bus.subscribe(TuiEvent.SessionSelect, (event) => seen.push(event.properties.sessionID)) + + const result = yield* Effect.gen(function* () { + const info = yield* SessionTool + const tool = yield* info.init() + return yield* tool.execute({ operation: { action: "switch", sessionID: unregistered.id } }, ctx(root.id)) + }).pipe(Effect.provide(broken)) + + unsub() + expect(seen).toEqual([]) + expect(result.title).toContain("Refused") + expect(result.output).toContain("could not verify") + // Not the prohibition's wording, so the model is not told a product rule + // when what happened was a broken read. + expect(result.output).not.toContain("runtime-spawned") + // Still model-actionable: a silent no-op or a crashed tool call just makes + // the model retry blind. + expect(result.output).toContain("retry the switch") + expect(result.output).toContain("session list") + }), + ), + ) + + // A root's verdict never depends on its rows, so an unreadable read must not make + // one unopenable — and this path is the only one that can regress it, because + // verifySessionRenderable returns before it fetches while switch reads rows + // unconditionally. + it.live("switch still publishes for a root when the actor rows cannot be read", () => + provideTmpdirInstance(() => + Effect.gen(function* () { + const { root } = yield* scaffold + const real = yield* ActorRegistry.Service + const broken = Layer.succeed(ActorRegistry.Service, { + ...real, + listBySession: () => Effect.die(new Error("actor_registry read failed")), + }) + + const seen: string[] = [] + const unsub = Bus.subscribe(TuiEvent.SessionSelect, (event) => seen.push(event.properties.sessionID)) + + const result = yield* Effect.gen(function* () { + const info = yield* SessionTool + const tool = yield* info.init() + return yield* tool.execute({ operation: { action: "switch", sessionID: root.id } }, ctx(root.id)) + }).pipe(Effect.provide(broken)) + + unsub() + expect(seen).toEqual([root.id]) + expect(result.title).toContain("Switched to") + }), + ), + ) + + it.live("switch still publishes for a root that carries a checkpoint-writer row", () => + provideTmpdirInstance(() => + Effect.gen(function* () { + const { root, writerRoot } = yield* scaffold + + const seen: string[] = [] + const unsub = Bus.subscribe(TuiEvent.SessionSelect, (event) => seen.push(event.properties.sessionID)) + + const info = yield* SessionTool + const tool = yield* info.init() + const result = yield* tool.execute({ operation: { action: "switch", sessionID: writerRoot.id } }, ctx(root.id)) + + unsub() + expect(seen).toEqual([writerRoot.id]) + expect(result.title).toContain("Switched to") + }), + ), + ) +})