smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: address gc review edge cases

Browse Source
This commit is contained in:
Dr Kennedy Umege
2026-06-26 12:58:30 +01:00
parent 1912cf93ab
commit 5ea467aa3c
6 changed files with 175 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/src/cli/gc-cli.ts
+46 -6
View File
@@ -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<ResolvedGcOptions> {
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<ResolvedGcOptions>
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<SessionInfo[]>
return sessions;
}
async function listNestedSessionsReadOnly(artifactsRoot: string): Promise<SessionInfo[]> {
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<boolean> {
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;
packages/coding-agent/src/config/settings.ts
+8
View File
@@ -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<Settings> {
const instance = new Settings(options);
return instance.#load();
}
/**
* Create an isolated instance for testing.
* Does not affect the global singleton.
packages/coding-agent/test/discovery/opencode.test.ts
+2 -1
View File
@@ -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();
packages/coding-agent/test/gc-cli.test.ts
+113
View File
@@ -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);
packages/coding-agent/test/issue-3461-repro.test.ts
+1 -1
View File
@@ -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/);
});
});
packages/coding-agent/test/sdk-mcp-auto-discovery.test.ts
+5 -1
View File
@@ -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 });
}