From e502d569a99f10084ddf8686354983fdc7a36349 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 23:15:09 +0200 Subject: [PATCH] test(sessiond): make the concurrent-refresh note's wiring verifiable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One optional line in sessiond.ts passed the model catalog refresher into PiSessionService, and it was the only thing making "· provider model lists are refreshing" reachable in the product. Deleting it left typecheck, lint, Knip, and all 1860 tests green while the note silently disappeared — confirmed by actually deleting it and running each check. sessiond.ts starts a daemon as an import side effect, so nothing in it could be tested. Extract the dependency assembly into a pure sessionServiceDependencies() function, following the sessionDaemonStartup.ts precedent, and assert what a user is actually told: a service built by the real assembly, with a refresh in flight, reports the note on its startup phases. A companion test pins the note to the refresher's answer so the first cannot pass on unconditional wording. Only the object literal moved. Everything createRuntime() constructs before it stays in place and in order — the provider freeze must still precede any real session, for the reason its comment gives — and the extracted function performs no side effect, so construction order, side effects, and routes are unchanged. The wiring is now guarded at both hops. Dropping the field from the assembly fails the new test; dropping it from sessiond.ts fails typecheck, because the assembly's input type requires every collaborator the daemon constructs. PiSessionServiceDependencies.catalogRefreshStatus stays optional, so the 98 existing service constructions in the suite are untouched, and the refresher's getter stays read-only: the test injects a fake in-flight status rather than touching its cadence, timeout, or coalescing. subsessionsEnabled's spawn-capability conjunction moved into the assembly with the literal it lived in. The semantics are identical, and it is now covered by a test instead of being another untested decision in an untestable file. --- src/server/sessiond.ts | 9 +- .../sessionServiceDependencies.test.ts | 90 +++++++++++++++++++ .../sessiond/sessionServiceDependencies.ts | 53 +++++++++++ 3 files changed, 147 insertions(+), 5 deletions(-) create mode 100644 src/server/sessiond/sessionServiceDependencies.test.ts create mode 100644 src/server/sessiond/sessionServiceDependencies.ts diff --git a/src/server/sessiond.ts b/src/server/sessiond.ts index 0a67a5e..87f78d7 100644 --- a/src/server/sessiond.ts +++ b/src/server/sessiond.ts @@ -27,6 +27,7 @@ import { SESSIOND_RUNTIME_CAPABILITIES } from "../shared/capabilities.js"; import { agentSessionDirEnvKeys, effectivePiWebConfig, maxUploadBytes, offlineModeEnabled } from "../config.js"; import { createActiveAgentProfileDescriptor } from "../sessiond/activeAgentProfile.js"; import { runSessionDaemonStartup } from "./sessiond/sessionDaemonStartup.js"; +import { sessionServiceDependencies } from "./sessiond/sessionServiceDependencies.js"; const daemonEnvironment: NodeJS.ProcessEnv = Object.freeze({ ...process.env }); const { config } = effectivePiWebConfig({ env: daemonEnvironment }); @@ -70,24 +71,22 @@ await runSessionDaemonStartup({ const spawnTargets = config.spawnSessions ? new ProjectScopedSpawnTargetResolver({ projects: new ProjectService(new ProjectStore()), workspaces: new WorkspaceService() }) : undefined; - const sessions = new PiSessionService(eventHub, { + const sessions = new PiSessionService(eventHub, sessionServiceDependencies({ modelRuntime: auth.runtime, agentDir: activeAgentProfile.dir, workspaceActivity, logger: app.log, ...(spawnTargets === undefined ? {} : { spawnTargets }), - subsessionsEnabled: spawnTargets !== undefined && config.subsessions, + subsessionsEnabled: config.subsessions, notificationStore, unreadStore, - // Read-only, so session startup can tell a waiting user that provider - // model lists are refreshing at the same time. catalogRefreshStatus: catalogRefresher, sessionManager: createPiSessionManagerGateway({ agentDir: activeAgentProfile.dir, env: daemonEnvironment, sessionDirEnvKeys: activeAgentProfile.sessionDirEnvKeys, }), - }); + })); auth.subscribe((change) => { sessions.applyAuthChange(change); }); const terminals = new TerminalService(eventHub, workspaceActivity); const runtimeComponent = Object.freeze({ diff --git a/src/server/sessiond/sessionServiceDependencies.test.ts b/src/server/sessiond/sessionServiceDependencies.test.ts new file mode 100644 index 0000000..fcc52c9 --- /dev/null +++ b/src/server/sessiond/sessionServiceDependencies.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it } from "vitest"; +import { WorkspaceActivityService } from "../activity/workspaceActivityService.js"; +import { SessionNotificationStore } from "../sessions/sessionNotificationStore.js"; +import { SessionUnreadStore } from "../sessions/sessionUnreadStore.js"; +import { PiSessionService, type PiSessionServiceDependencies } from "../sessions/piSessionService.js"; +import { CapturingSessionEventHub, emptyArchiveStore, fakeRuntime, sessionGateway, testModelRuntime } from "../sessions/piSessionService.testSupport.js"; +import { sessionServiceDependencies, type SessionServiceDependencyInput } from "./sessionServiceDependencies.js"; + +const AGENT_DIR = "/tmp/pi-web-test-agent"; + +/** + * The collaborators sessiond hands the assembly, with only the process-facing + * ones (agent dir, session store gateway) replaced. Everything the assembly + * decides is left to the assembly. + */ +function daemonCollaborators(patch: Partial = {}): SessionServiceDependencyInput { + return { + agentDir: AGENT_DIR, + modelRuntime: testModelRuntime, + sessionManager: sessionGateway([]), + workspaceActivity: new WorkspaceActivityService(), + logger: { info() { /* no-op */ } }, + notificationStore: new SessionNotificationStore(), + unreadStore: new SessionUnreadStore(), + catalogRefreshStatus: { isRefreshInFlight: () => false }, + subsessionsEnabled: false, + ...patch, + }; +} + +/** + * Start a session through a service built by the real assembly and collect what + * the user is told while waiting. Only test-local seams are patched onto the + * assembled dependencies, never anything the assembly is responsible for + * supplying, so a dependency the assembly stops passing cannot be masked here. + */ +async function startupDetails(deps: PiSessionServiceDependencies): Promise { + const hub = new CapturingSessionEventHub(); + const fake = fakeRuntime(); + const service = new PiSessionService(hub, { + ...deps, + archiveStore: emptyArchiveStore(), + createAgentRuntime: () => Promise.resolve(fake.runtime), + heartbeatIntervalMs: 60_000, + }); + try { + await service.start("/workspace"); + } finally { + await service.dispose(); + } + return hub.globalEvents.flatMap((event) => + event.type === "session.startup" && event.activity.detail !== undefined ? [event.activity.detail] : [], + ); +} + +describe("sessiond session service dependency assembly", () => { + it("reports a concurrent provider model list refresh to a waiting user", async () => { + // The note is only reachable in the product because the assembly hands the + // refresher to the session service. Asserting the note rather than the + // property means dropping that line fails here instead of passing silently. + const details = await startupDetails(sessionServiceDependencies(daemonCollaborators({ + catalogRefreshStatus: { isRefreshInFlight: () => true }, + }))); + + expect(details).toEqual([ + "Starting the Pi session · provider model lists are refreshing", + "Loading session extensions · provider model lists are refreshing", + ]); + }); + + it("states the startup phase alone when no refresh is running", async () => { + const details = await startupDetails(sessionServiceDependencies(daemonCollaborators())); + + // Pins the note to the refresher's answer, so the test above cannot pass on + // wording that is always appended. + expect(details).toEqual(["Starting the Pi session", "Loading session extensions"]); + }); + + it("keeps tracked subsessions off unless spawning is configured as well", () => { + const spawnTargets = { resolveSpawnTarget: () => Promise.reject(new Error("not used")) }; + + const withoutSpawnTargets = sessionServiceDependencies(daemonCollaborators({ subsessionsEnabled: true })); + const withSpawnTargets = sessionServiceDependencies(daemonCollaborators({ subsessionsEnabled: true, spawnTargets })); + + expect(withoutSpawnTargets.spawnTargets).toBeUndefined(); + expect(withoutSpawnTargets.subsessionsEnabled).toBe(false); + expect(withSpawnTargets.spawnTargets).toBe(spawnTargets); + expect(withSpawnTargets.subsessionsEnabled).toBe(true); + }); +}); diff --git a/src/server/sessiond/sessionServiceDependencies.ts b/src/server/sessiond/sessionServiceDependencies.ts new file mode 100644 index 0000000..a2dcfbb --- /dev/null +++ b/src/server/sessiond/sessionServiceDependencies.ts @@ -0,0 +1,53 @@ +import type { PiSessionServiceDependencies } from "../sessions/piSessionService.js"; + +/** + * The collaborators sessiond constructs, in the shape the assembly needs them. + * + * Every field is required unless the capability itself is optional, so a + * collaborator the daemon stops constructing cannot silently vanish from the + * session service. + */ +export interface SessionServiceDependencyInput { + agentDir: string; + sessionManager: PiSessionServiceDependencies["sessionManager"]; + modelRuntime: PiSessionServiceDependencies["modelRuntime"]; + workspaceActivity: NonNullable; + logger: NonNullable; + notificationStore: NonNullable; + unreadStore: NonNullable; + /** Read-only view of the background refresher; see the assembly below. */ + catalogRefreshStatus: NonNullable; + /** Omitted when the operator has not enabled session spawning. */ + spawnTargets?: NonNullable; + /** The operator's subsessions preference, which also requires spawning. */ + subsessionsEnabled: boolean; +} + +/** + * Map sessiond's constructed collaborators onto the session service's + * dependencies. + * + * Extracted from `sessiond.ts`, which starts a daemon as an import side effect + * and so cannot be loaded by a test. This function performs no side effects and + * constructs nothing, so lifting it changes no startup ordering; it exists only + * so a test can build a service the way the daemon does and assert what a user + * is actually told. + */ +export function sessionServiceDependencies(input: SessionServiceDependencyInput): PiSessionServiceDependencies { + return { + modelRuntime: input.modelRuntime, + agentDir: input.agentDir, + workspaceActivity: input.workspaceActivity, + logger: input.logger, + ...(input.spawnTargets === undefined ? {} : { spawnTargets: input.spawnTargets }), + // Tracked subsessions share the spawn capability's project-scope resolver, + // so they stay off unless spawning is configured too. + subsessionsEnabled: input.spawnTargets !== undefined && input.subsessionsEnabled, + notificationStore: input.notificationStore, + unreadStore: input.unreadStore, + // Read-only, so session startup can tell a waiting user that provider + // model lists are refreshing at the same time. + catalogRefreshStatus: input.catalogRefreshStatus, + sessionManager: input.sessionManager, + }; +}