From 5ea467aa3c2521dc09d3a16b34dbc2beacaee230 Mon Sep 17 00:00:00 2001 From: Dr Kennedy Umege Date: Fri, 26 Jun 2026 12:58:30 +0100 Subject: [PATCH] fix: address gc review edge cases --- packages/coding-agent/src/cli/gc-cli.ts | 52 +++++++- packages/coding-agent/src/config/settings.ts | 8 ++ .../test/discovery/opencode.test.ts | 3 +- packages/coding-agent/test/gc-cli.test.ts | 113 ++++++++++++++++++ .../test/issue-3461-repro.test.ts | 2 +- .../test/sdk-mcp-auto-discovery.test.ts | 6 +- 6 files changed, 175 insertions(+), 9 deletions(-) diff --git a/packages/coding-agent/src/cli/gc-cli.ts b/packages/coding-agent/src/cli/gc-cli.ts index 696da259f..a8db55a95 100644 --- a/packages/coding-agent/src/cli/gc-cli.ts +++ b/packages/coding-agent/src/cli/gc-cli.ts @@ -4,6 +4,7 @@ import * as path from "node:path"; import { gunzipSync, gzipSync } from "node:zlib"; import { getAgentDir, getBlobsDir, getHistoryDbPath, getModelDbPath, getSessionsDir } from "@oh-my-pi/pi-utils"; import { Settings } from "../config/settings"; +import { getDefault } from "../config/settings-schema"; import { listSessionsReadOnly, type SessionInfo, type SessionStatus } from "../session/session-listing"; import { FileSessionStorage } from "../session/session-storage"; @@ -127,16 +128,21 @@ interface GcLockSnapshot { text: string; } -function numberSetting(value: number | undefined, fallback: number): number { - if (value === undefined || !Number.isFinite(value)) return fallback; +function normalizeNumberSetting(value: unknown, defaultValue: number): number { + if (typeof value !== "number" || !Number.isFinite(value)) return defaultValue; return Math.max(0, Math.floor(value)); } +function numberSetting(value: number | undefined, fallback: unknown, defaultValue: number): number { + if (value !== undefined && Number.isFinite(value)) return Math.max(0, Math.floor(value)); + return normalizeNumberSetting(fallback, defaultValue); +} + async function resolveOptions(flags: GcCommandFlags): Promise { const agentDir = path.resolve(flags.agentDir ?? getAgentDir()); const selected = flags.blobs === true || flags.archive === true || flags.wal === true; const settings = - flags.apply === true ? await Settings.init({ agentDir }) : await Settings.loadReadOnly({ agentDir }); + flags.apply === true ? await Settings.loadIsolated({ agentDir }) : await Settings.loadReadOnly({ agentDir }); const getBoolean = (pathKey: "gc.blobs" | "gc.archive" | "gc.wal") => settings.get(pathKey); const getNumber = (pathKey: "gc.coldArchiveAfterDays" | "gc.retainNewestGlobal" | "gc.retainNewestPerCwd") => settings.get(pathKey); @@ -147,9 +153,21 @@ async function resolveOptions(flags: GcCommandFlags): Promise runBlobs: selected ? flags.blobs === true : getBoolean("gc.blobs"), runArchive: selected ? flags.archive === true : getBoolean("gc.archive"), runWal: selected ? flags.wal === true : getBoolean("gc.wal"), - coldArchiveAfterDays: numberSetting(flags.coldArchiveAfterDays, getNumber("gc.coldArchiveAfterDays")), - retainNewestGlobal: numberSetting(flags.retainNewestGlobal, getNumber("gc.retainNewestGlobal")), - retainNewestPerCwd: numberSetting(flags.retainNewestPerCwd, getNumber("gc.retainNewestPerCwd")), + coldArchiveAfterDays: numberSetting( + flags.coldArchiveAfterDays, + getNumber("gc.coldArchiveAfterDays"), + getDefault("gc.coldArchiveAfterDays"), + ), + retainNewestGlobal: numberSetting( + flags.retainNewestGlobal, + getNumber("gc.retainNewestGlobal"), + getDefault("gc.retainNewestGlobal"), + ), + retainNewestPerCwd: numberSetting( + flags.retainNewestPerCwd, + getNumber("gc.retainNewestPerCwd"), + getDefault("gc.retainNewestPerCwd"), + ), }; } @@ -337,6 +355,24 @@ async function listActiveSessions(sessionsRoot: string): Promise return sessions; } +async function listNestedSessionsReadOnly(artifactsRoot: string): Promise { + const files = await collectJsonlFiles(artifactsRoot); + const dirs = [...new Set(files.map(file => path.dirname(file)))].sort(); + const storage = new FileSessionStorage(); + const sessions: SessionInfo[] = []; + for (const dir of dirs) sessions.push(...(await listSessionsReadOnly(dir, storage))); + sessions.sort((a, b) => b.modified.getTime() - a.modified.getTime()); + return sessions; +} + +async function hasLiveNestedSessions(session: SessionInfo, archiveBeforeMs: number): Promise { + for (const nested of await listNestedSessionsReadOnly(sessionArtifactsPath(session.path))) { + if (nested.status && ACTIVE_STATUSES.has(nested.status)) return true; + if (nested.modified.getTime() > archiveBeforeMs) return true; + } + return false; +} + function archiveDestination( archiveRoot: string, sessionsRoot: string, @@ -580,6 +616,10 @@ async function runArchiveGc(options: ResolvedGcOptions, archiveRoot: string): Pr result.skippedActive += 1; continue; } + if (await hasLiveNestedSessions(session, archiveBeforeMs)) { + result.skippedActive += 1; + continue; + } const cwdKey = sessionCwdKey(sessionsRoot, session); const cwdSeen = inactiveSeenByCwd.get(cwdKey) ?? 0; const keepGlobal = inactiveSeen < options.retainNewestGlobal; diff --git a/packages/coding-agent/src/config/settings.ts b/packages/coding-agent/src/config/settings.ts index f41d6b135..ab60b0f5f 100644 --- a/packages/coding-agent/src/config/settings.ts +++ b/packages/coding-agent/src/config/settings.ts @@ -281,6 +281,14 @@ export class Settings { return instance.#loadReadOnly(); } + /** + * Load a persisted settings instance without touching the global singleton. + */ + static loadIsolated(options: SettingsOptions = {}): Promise { + const instance = new Settings(options); + return instance.#load(); + } + /** * Create an isolated instance for testing. * Does not affect the global singleton. diff --git a/packages/coding-agent/test/discovery/opencode.test.ts b/packages/coding-agent/test/discovery/opencode.test.ts index fa85f7654..6a9094201 100644 --- a/packages/coding-agent/test/discovery/opencode.test.ts +++ b/packages/coding-agent/test/discovery/opencode.test.ts @@ -88,7 +88,8 @@ describe("OpenCode MCP discovery", () => { }), ); - const [server] = await loadOpenCodeMcpConfig(tempDir); + const servers = await loadOpenCodeMcpConfig(tempDir); + const server = servers.find(item => item.name === "plain"); expect(server?.command).toBe("server-bin"); expect(server?.args).toBeUndefined(); diff --git a/packages/coding-agent/test/gc-cli.test.ts b/packages/coding-agent/test/gc-cli.test.ts index 97195bd39..190a3d770 100644 --- a/packages/coding-agent/test/gc-cli.test.ts +++ b/packages/coding-agent/test/gc-cli.test.ts @@ -5,6 +5,7 @@ import * as os from "node:os"; import * as path from "node:path"; import { gunzipSync } from "node:zlib"; import { runGcCommand } from "@oh-my-pi/pi-coding-agent/cli/gc-cli"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { getAgentDir, getBlobsDir, @@ -221,6 +222,85 @@ describe("runGcCommand blob sweep", () => { ); }); + test("--apply loads gc config from each requested agent dir", async () => { + const initializedAgentDir = path.join(root, "initialized-agent"); + const targetAgentDir = path.join(root, "target-agent"); + await writeConfig( + initializedAgentDir, + ["gc:", " blobs: false", " archive: false", " wal: false", ""].join("\n"), + ); + await Settings.init({ agentDir: initializedAgentDir }); + await writeSession(targetAgentDir, "project", "archive-me", "complete", { ageDays: 10 }); + await writeConfig( + targetAgentDir, + [ + "gc:", + " blobs: false", + " archive: true", + " wal: false", + " coldArchiveAfterDays: 7", + " retainNewestGlobal: 0", + " retainNewestPerCwd: 0", + "", + ].join("\n"), + ); + + const result = await runGcCommand({ flags: { agentDir: targetAgentDir, apply: true } }); + + expect(result.blobs).toBeUndefined(); + expect(result.wal).toBeUndefined(); + expect(result.archive?.archived).toBe(1); + expect( + await Bun.file(path.join(targetAgentDir, "archive", "sessions", "project", "archive-me.jsonl.gz")).exists(), + ).toBe(true); + }); + + test("invalid configured archive age falls back to schema default", async () => { + const session = await writeSession(root, "project", "too-new", "complete", { ageDays: 1 }); + await writeConfig( + root, + [ + "gc:", + " blobs: false", + " archive: true", + " wal: false", + " coldArchiveAfterDays: nope", + " retainNewestGlobal: 0", + " retainNewestPerCwd: 0", + "", + ].join("\n"), + ); + + const result = await runGcCommand({ flags: { agentDir: root, apply: true } }); + + expect(result.archive?.wouldArchive).toBe(0); + expect(result.archive?.archived).toBe(0); + expect(await Bun.file(session).exists()).toBe(true); + }); + + test("invalid configured retention counts fall back to schema defaults", async () => { + const session = await writeSession(root, "project", "kept-by-default", "complete", { ageDays: 90 }); + await writeConfig( + root, + [ + "gc:", + " blobs: false", + " archive: true", + " wal: false", + " coldArchiveAfterDays: 0", + " retainNewestGlobal: nope", + " retainNewestPerCwd: nope", + "", + ].join("\n"), + ); + + const result = await runGcCommand({ flags: { agentDir: root, apply: true } }); + + expect(result.archive?.keptNewestGlobal).toBe(1); + expect(result.archive?.archived).toBe(0); + expect(await Bun.file(session).exists()).toBe(true); + }); + test("explicit selectors override disabled gc config", async () => { const blob = await writeBlob(root, hashFor("orphan"), "orphan"); await agePath(blob); @@ -414,6 +494,39 @@ describe("runGcCommand cold-session archive", () => { expect(await Bun.file(interrupted).exists()).toBe(true); }); + test("skips archiving parent sessions with live nested sessions", async () => { + const parent = await writeSession(root, "project", "parent", "complete", { ageDays: 90 }); + const artifactsDir = parent.slice(0, -".jsonl".length); + const nested = path.join(artifactsDir, "Tan-nested.jsonl"); + await fs.mkdir(artifactsDir, { recursive: true }); + await Bun.write( + nested, + [ + JSON.stringify({ type: "session", version: 3, id: "Tan-nested", timestamp: "2026-01-01T00:00:00.000Z" }), + JSON.stringify({ type: "message", message: { role: "user", content: "waiting" } }), + "", + ].join("\n"), + ); + await agePath(nested, 90); + + const result = await runGcCommand({ + flags: { + agentDir: root, + archive: true, + coldArchiveAfterDays: 30, + retainNewestGlobal: 0, + retainNewestPerCwd: 0, + apply: true, + }, + }); + + expect(result.archive?.skippedActive).toBe(1); + expect(result.archive?.archived).toBe(0); + expect(await Bun.file(parent).exists()).toBe(true); + expect(await Bun.file(nested).exists()).toBe(true); + expect(await Bun.file(path.join(root, "archive", "sessions", "project", "parent.jsonl.gz")).exists()).toBe(false); + }); + test("removes archived session rows from history and rebuilds FTS", async () => { await writeSession(root, "project", "archive-me", "complete", { ageDays: 90 }); const dbPath = getHistoryDbPath(root); diff --git a/packages/coding-agent/test/issue-3461-repro.test.ts b/packages/coding-agent/test/issue-3461-repro.test.ts index 036e86c50..c6fc04c08 100644 --- a/packages/coding-agent/test/issue-3461-repro.test.ts +++ b/packages/coding-agent/test/issue-3461-repro.test.ts @@ -62,6 +62,6 @@ describe("issue #3461 — Ctrl+Z hangs after a command has been run", () => { it("MCP stdio servers spawn detached so terminal job-control signals cannot stop them", async () => { const src = await Bun.file(mcpStdioTransport).text(); expect(src).toMatch(/detached:\s*true/); - expect(src).toContain("no controlling terminal"); + expect(src).toMatch(/no(?:\s+\*\s+|\s+)controlling terminal/); }); }); diff --git a/packages/coding-agent/test/sdk-mcp-auto-discovery.test.ts b/packages/coding-agent/test/sdk-mcp-auto-discovery.test.ts index 654125252..84d4cc6ef 100644 --- a/packages/coding-agent/test/sdk-mcp-auto-discovery.test.ts +++ b/packages/coding-agent/test/sdk-mcp-auto-discovery.test.ts @@ -8,7 +8,7 @@ import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { createAgentSession } from "@oh-my-pi/pi-coding-agent/sdk"; import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; -import { Snowflake } from "@oh-my-pi/pi-utils"; +import { getAgentDir, Snowflake, setAgentDir } from "@oh-my-pi/pi-utils"; import { MANY_TOOL_COUNT } from "./fixtures/many-tools-mcp"; // Contracts for deferred (hasUI) MCP discovery follow-ups: @@ -30,6 +30,7 @@ describe("createAgentSession deferred MCP auto discovery", () => { let tempDir: string; let authStorage: AuthStorage; let modelRegistry: ModelRegistry; + let originalAgentDir: string; // Discovery resolves user-level MCP config from `os.homedir()`; redirect it // to an empty dir so the test connects ONLY to the fixture server and never // spawns the developer's real MCP servers. @@ -40,6 +41,7 @@ describe("createAgentSession deferred MCP auto discovery", () => { fs.mkdirSync(registryDir, { recursive: true }); isolatedHome = path.join(os.tmpdir(), `pi-sdk-mcp-auto-home-${Snowflake.next()}`); fs.mkdirSync(isolatedHome, { recursive: true }); + originalAgentDir = getAgentDir(); authStorage = await AuthStorage.create(path.join(registryDir, "auth.db")); modelRegistry = new ModelRegistry(authStorage); }); @@ -56,10 +58,12 @@ describe("createAgentSession deferred MCP auto discovery", () => { beforeEach(() => { tempDir = path.join(os.tmpdir(), `pi-sdk-mcp-auto-${Snowflake.next()}`); fs.mkdirSync(tempDir, { recursive: true }); + setAgentDir(tempDir); spyOn(os, "homedir").mockReturnValue(isolatedHome); }); afterEach(() => { + setAgentDir(originalAgentDir); if (tempDir && fs.existsSync(tempDir)) { fs.rmSync(tempDir, { recursive: true, force: true }); }