diff --git a/TODO.md b/TODO.md new file mode 100644 index 000000000..324fb4ec6 --- /dev/null +++ b/TODO.md @@ -0,0 +1,32 @@ +# Deferred test coverage + +These cases were removed from the coding-agent server backend PR to keep its test scope focused. Reintroduce them in separate, ownership-specific PRs. + +## Agent session persistence + +Target: `packages/agent/test/harness/repo.test.ts` + +- Verify that opening a JSONL session with malformed entry JSON fails loudly instead of returning partial history. +- Verify that malformed JSONL session headers fail `list()` and `open()` instead of being omitted. +- Add one coding-agent backend boundary test with an injected `SessionRepository` that rejects during `list()` or `open()`, proving repository failures propagate without depending on JSONL internals. + +## Agent harness shutdown + +Target: `packages/agent/test/harness/agent-harness.test.ts` + +- Verify `requestShutdown({ discardPendingWrites: true })` aborts active work, drains shutdown, and does not persist assistant output produced after ownership is lost. + +## Agent shell environment + +Target: `packages/agent/test/harness/tools.test.ts` + +- Verify a bash preparation patch can set an environment variable to `undefined` to remove an inherited value from the child process. + +## Coding-agent server hardening + +Target: `packages/coding-agent/test/server/` + +- Verify an absolute cwd symlink whose target is a directory is accepted and preserved in snapshots. +- Verify reopening rejects a persisted cwd that no longer resolves to a directory. +- Verify non-file-backed repositories remove an inherited `PI_SESSION_FILE` from bash child processes. +- Verify transcript lifecycle events cannot publish an idle runtime phase before the originating prompt operation settles. diff --git a/packages/agent/test/harness/agent-harness.test.ts b/packages/agent/test/harness/agent-harness.test.ts index f78053475..ed7b55206 100644 --- a/packages/agent/test/harness/agent-harness.test.ts +++ b/packages/agent/test/harness/agent-harness.test.ts @@ -213,33 +213,6 @@ describe("AgentHarness", () => { }); }); - it("can discard active work when shutting down after ownership is lost", async () => { - const registration = newFaux(); - const entered = deferred(); - const release = deferred(); - registration.setResponses([ - async () => { - entered.resolve(); - await release.promise; - return fauxAssistantMessage("must not persist"); - }, - ]); - const session = await createInMemorySession(); - const harness = new AgentHarness({ models, session, model: registration.getModel() }); - const prompt = harness.prompt("hello"); - await entered.promise; - - harness.requestShutdown({ discardPendingWrites: true }); - release.resolve(); - - await expect(prompt).resolves.toMatchObject({ role: "assistant" }); - await expect(harness.waitForShutdown()).resolves.toBeUndefined(); - const messages = (await session.getEntries()).flatMap((entry) => - entry.type === "message" ? [entry.message] : [], - ); - expect(messages.map((message) => message.role)).toEqual(["user"]); - }); - it("allows a hook to request shutdown without deadlocking its operation", async () => { const registration = newFaux(); let providerCalls = 0; diff --git a/packages/agent/test/harness/tools.test.ts b/packages/agent/test/harness/tools.test.ts index 1af1f5d8b..a926051c7 100644 --- a/packages/agent/test/harness/tools.test.ts +++ b/packages/agent/test/harness/tools.test.ts @@ -572,28 +572,6 @@ describe("AgentHarness tools", () => { expect(textOutput(result)).toBe(`ready::explicit:${getOrThrow(await env.canonicalPath(context.workspace))}`); }); - it("removes inherited environment variables through the prepare patch", async () => { - const env = new NodeExecutionEnv({ - cwd: createTempDir(), - shellEnv: { PI_BASH_PREPARE_REMOVED: "remove-me" }, - }); - const tool = createBashTool({ - prepare: (execution) => { - execution.env.PI_BASH_PREPARE_REMOVED = undefined; - }, - }); - - const result = await tool.execute( - "bash-remove-env", - { command: `printf '%s' "\${PI_BASH_PREPARE_REMOVED-unset}"` }, - undefined, - undefined, - { env }, - ); - - expect(textOutput(result)).toBe("unset"); - }); - it("supports command prefixes", async () => { const context = createContext(); const result = await createBashTool({ commandPrefix: "value=hello" }).execute( diff --git a/packages/coding-agent/test/server/backend.test.ts b/packages/coding-agent/test/server/backend.test.ts index 3ca822625..8e07daeec 100644 --- a/packages/coding-agent/test/server/backend.test.ts +++ b/packages/coding-agent/test/server/backend.test.ts @@ -1,4 +1,3 @@ -import { rm, symlink, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { createReadTool, @@ -7,10 +6,10 @@ import { type SessionMetadata, type SessionRepository, } from "@earendil-works/pi-agent-core"; -import { fauxAssistantMessage, fauxToolCall } from "@earendil-works/pi-ai"; +import { fauxAssistantMessage } from "@earendil-works/pi-ai"; import type { TranscriptProgress } from "@earendil-works/pi-protocol"; import { describe, expect, test } from "vitest"; -import { CodingAgentServerBackend, toPiServerError } from "../../src/server/backend.ts"; +import { CodingAgentServerBackend } from "../../src/server/backend.ts"; import { createServerBackendFixture, removeServerBackendFixture } from "./fixture.ts"; function deferred(): { promise: Promise; resolve(): void } { @@ -231,77 +230,6 @@ describe("coding-agent server backend", () => { } }); - test("accepts a session cwd that is a symlink to a directory", async () => { - const fixture = await createServerBackendFixture(); - const linkedCwd = join(fixture.root, "linked-workspace"); - await symlink(fixture.cwd, linkedCwd, "dir"); - const runtime = await fixture.backend.createSession({ id: "symlink-cwd", cwd: linkedCwd }); - try { - expect(await runtime.snapshot()).toMatchObject({ cwd: linkedCwd }); - } finally { - await runtime.dispose(); - await removeServerBackendFixture(fixture); - } - }); - - test("rejects a persisted cwd that is no longer a directory", async () => { - const fixture = await createServerBackendFixture(); - const runtime = await fixture.backend.createSession({ id: "invalid-persisted-cwd", cwd: fixture.cwd }); - await runtime.dispose(); - await rm(fixture.cwd, { recursive: true }); - await writeFile(fixture.cwd, "not a directory"); - try { - await expect(fixture.backend.openSession("invalid-persisted-cwd")).rejects.toMatchObject({ - code: "invalid_request", - message: expect.stringContaining("Invalid session cwd"), - }); - } finally { - await removeServerBackendFixture(fixture); - } - }); - - test("removes inherited PI_SESSION_FILE for non-file-backed sessions", async () => { - const fixture = await createServerBackendFixture(); - const sessionRepository = new InMemorySessionRepository(); - const backend = await CodingAgentServerBackend.create({ - defaultCwd: fixture.cwd, - modelRuntime: fixture.modelRuntime, - settingsManager: fixture.settingsManager, - sessionRepository, - createSessionOptions: ({ id }) => ({ id }), - lockRoot: join(fixture.root, "environment-locks"), - }); - const previousSessionFile = process.env.PI_SESSION_FILE; - process.env.PI_SESSION_FILE = "inherited-session.jsonl"; - const runtime = await backend.createSession({ - id: "memory-environment", - cwd: fixture.cwd, - model: { provider: fixture.faux.provider.id, id: "faux-reasoning" }, - }); - try { - fixture.faux.setResponses([ - fauxAssistantMessage(fauxToolCall("bash", { command: `printf '%s' "\${PI_SESSION_FILE-unset}"` }), { - stopReason: "toolUse", - }), - fauxAssistantMessage("done"), - ]); - await runtime.prompt({ text: "inspect environment" }); - const tool = (await runtime.snapshot()).transcript.find((item) => item.role === "tool"); - if (!tool || tool.role !== "tool") throw new Error("Expected tool transcript item"); - expect(tool.content).toContainEqual({ type: "text", text: "unset" }); - } finally { - await runtime.dispose(); - if (previousSessionFile === undefined) delete process.env.PI_SESSION_FILE; - else process.env.PI_SESSION_FILE = previousSessionFile; - await sessionRepository[Symbol.asyncDispose](); - await removeServerBackendFixture(fixture); - } - }); - - test("preserves unexpected operational errors for boundary-safe handling", () => { - const operational = new Error("private filesystem detail"); - expect(toPiServerError(operational)).toBe(operational); - }); test("applies server-only defaults to newly created sessions", async () => { const fixture = await createServerBackendFixture(); fixture.backend.setDefaultSessionOptions({ diff --git a/packages/coding-agent/test/server/persistence.test.ts b/packages/coding-agent/test/server/persistence.test.ts index c4dad1284..674f7450b 100644 --- a/packages/coding-agent/test/server/persistence.test.ts +++ b/packages/coding-agent/test/server/persistence.test.ts @@ -6,41 +6,6 @@ import { describe, expect, test, vi } from "vitest"; import { createServerBackendFixture, removeServerBackendFixture } from "./fixture.ts"; describe("coding-agent server persistence", () => { - test("fails session listing when persisted JSONL is unreadable", async () => { - const fixture = await createServerBackendFixture(); - const runtime = await fixture.backend.createSession({ - id: "server-corrupt-session", - cwd: fixture.cwd, - }); - await runtime.dispose(); - const files = await readdir(join(fixture.root, "sessions"), { recursive: true }); - const sessionFile = files.find((file) => file.endsWith(".jsonl")); - if (!sessionFile) throw new Error("Expected persisted JSONL session"); - await appendFile(join(fixture.root, "sessions", sessionFile), "{invalid json\n"); - - try { - await expect(fixture.backend.listSessions()).rejects.toThrow(/Failed to read coding-agent session/); - } finally { - await removeServerBackendFixture(fixture); - } - }); - test("surfaces corrupt session headers instead of omitting them", async () => { - const fixture = await createServerBackendFixture(); - const runtime = await fixture.backend.createSession({ id: "server-corrupt-header", cwd: fixture.cwd }); - await runtime.dispose(); - const files = await readdir(join(fixture.root, "sessions"), { recursive: true }); - const sessionFile = files.find((file) => file.endsWith(".jsonl")); - if (!sessionFile) throw new Error("Expected persisted JSONL session"); - await writeFile(join(fixture.root, "sessions", sessionFile), "{invalid header\n"); - try { - await expect(fixture.backend.listSessions()).rejects.toThrow(/Invalid JSONL session file/); - await expect(fixture.backend.openSession("server-corrupt-header")).rejects.toThrow( - /Invalid JSONL session file/, - ); - } finally { - await removeServerBackendFixture(fixture); - } - }); test("rejects invalid persisted creation timestamps", async () => { const fixture = await createServerBackendFixture(); const runtime = await fixture.backend.createSession({ id: "server-invalid-created-at", cwd: fixture.cwd }); diff --git a/packages/coding-agent/test/server/runtime.test.ts b/packages/coding-agent/test/server/runtime.test.ts index 1500fa901..b59c94698 100644 --- a/packages/coding-agent/test/server/runtime.test.ts +++ b/packages/coding-agent/test/server/runtime.test.ts @@ -156,31 +156,6 @@ describe("coding-agent session runtime", () => { await removeServerBackendFixture(fixture); } }); - test("keeps runtime phase ownership until the prompt operation settles", async () => { - const fixture = await createServerBackendFixture(); - const runtime = await fixture.backend.createSession({ - id: "server-prompt-phase-owner", - cwd: fixture.cwd, - model: { provider: fixture.faux.provider.id, id: "faux-reasoning" }, - }); - try { - fixture.faux.setResponses([fauxAssistantMessage("done")]); - const snapshotPhases: string[] = []; - runtime.subscribe((event) => { - if (event.type === "snapshot") snapshotPhases.push(runtime.getPhase()); - }); - - await runtime.prompt({ text: "start" }); - - expect(snapshotPhases.length).toBeGreaterThan(0); - expect(snapshotPhases).not.toContain("idle"); - expect(runtime.getPhase()).toBe("idle"); - } finally { - await runtime.dispose(); - await removeServerBackendFixture(fixture); - } - }); - test("rejects malformed partial and final tool results", () => { const transcript = new LiveTranscript(); expect(() =>