Skip to content

Commit 658b519

Browse files
committed
chore: audit cleanup — bg-shell kill result, diagnostics stderr, /mcp out
Closes four audit findings: - BackgroundShellStore.kill returns a structured result so callers can distinguish killed / not-found / already-exited / signal-failed. shell_kill tool surfaces signal-failed as an error so the model doesn't assume the process is gone when the OS refused our signal. - Diagnostics checker errors surface to stderr unconditionally instead of only under CODEBASE_DEBUG; a hanging checker is no longer a black box for end users running default settings. - /mcp stub command removed. Sitting on a placeholder that admits it's a placeholder feels worse than not having the command at all; tracked in docs/TECHNICAL_DEBT.md for when we ship real MCP. - Stale Phase-N references scrubbed from agent/agent.ts, agent/router.ts, tools/registry.ts, skills/loader.ts, skills/platform-loader.ts so docs match shipped code.
1 parent cad8441 commit 658b519

9 files changed

Lines changed: 81 additions & 52 deletions

File tree

src/agent/agent.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -194,8 +194,8 @@ export function createAgent(opts: CreateAgentOptions = {}): AgentBundle {
194194
tools: tools.map((t) => ({ name: t.name, description: t.description })),
195195
});
196196

197-
// MEMORY.md gets concatenated onto the system prompt at agent creation.
198-
// Reload-after-save is a Phase 11 polish item.
197+
// MEMORY.md gets concatenated onto the system prompt at agent creation;
198+
// edits during a session don't take effect until next launch.
199199
// Project-instruction file (first of AGENTS.md / CLAUDE.md / CODEX.md /
200200
// .cursorrules) gets pinned to the prompt so the agent sees the
201201
// project's conventions on every turn. Memory addendum is appended
@@ -314,15 +314,15 @@ export function createAgent(opts: CreateAgentOptions = {}): AgentBundle {
314314
});
315315
})
316316
.catch((err) => {
317-
// Diagnostics failures are non-fatal — surface only when the
318-
// user opted into debug. Silent before; that buried a real
317+
// Diagnostics failures are non-fatal — but always-stderr
318+
// instead of debug-only, because the previous behavior
319+
// (silent under default settings) hid a real production
319320
// bug where a checker hung and the user thought the tool
320-
// itself was slow.
321-
if (process.env.CODEBASE_DEBUG === "1") {
322-
process.stderr.write(
323-
`[diagnostics] ${absPath}: ${err instanceof Error ? err.message : String(err)}\n`,
324-
);
325-
}
321+
// itself was slow. Visible to anyone watching the terminal;
322+
// the agent keeps running.
323+
process.stderr.write(
324+
`[diagnostics] ${absPath}: ${err instanceof Error ? err.message : String(err)}\n`,
325+
);
326326
});
327327
}
328328
return undefined;

src/agent/router.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@ export async function routeUserInput(glue: GlueClient, text: string, options: Ro
2929
if (intent === "plan") {
3030
return { kind: "plan" };
3131
}
32-
// agent + clarify both go through the main agent for now; clarify-as-soft-hint
33-
// could surface a system reminder when we wire steering messages in Phase 11b.
32+
// agent + clarify both go through the main agent. A future "clarify" mode
33+
// could surface a system reminder via agent.steer() before running the turn.
3434
return { kind: "agent" };
3535
}
3636

src/commands/builtins.ts

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { execSync } from "node:child_process";
22
import { existsSync, writeFileSync } from "node:fs";
3-
import { homedir } from "node:os";
43
import { join } from "node:path";
54
import type { AgentMessage } from "@earendil-works/pi-agent-core";
65
import { CredentialsStore } from "../auth/credentials.js";
@@ -657,19 +656,6 @@ const projects: Command = {
657656
},
658657
};
659658

660-
const mcp: Command = {
661-
name: "mcp",
662-
description: "Manage MCP (Model Context Protocol) servers — placeholder until Phase 9 lands.",
663-
handler: (_args, ctx) => {
664-
const configPath = join(homedir(), ".codebase", "config.json");
665-
ctx.emit(
666-
"MCP support is on the Phase 9 roadmap; the runtime hasn't shipped it yet.\n" +
667-
`When it lands, server config will live at ${configPath} under "mcp_servers".`,
668-
);
669-
return { handled: true };
670-
},
671-
};
672-
673659
const redo: Command = {
674660
name: "redo",
675661
aliases: ["retry"],
@@ -729,7 +715,6 @@ export const BUILTIN_COMMANDS: readonly Command[] = [
729715
resume,
730716
init,
731717
projects,
732-
mcp,
733718
pwd,
734719
redo,
735720
debug,

src/skills/loader.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,11 @@ import type { Asset, AssetSource, PromptAsset, SkillAsset, TemplateAsset } from
66
* call, so loaders can come and go (e.g. PlatformLoader becomes
77
* available after the user runs `codebase auth login`).
88
*
9-
* Phase 7 ships LocalLoader (reads ~/.codebase/skills/*.md). Phase 7+
10-
* ships PlatformLoader (fetches from codebase.foundation/api/cli/...)
11-
* — see platform-loader.ts for the planned wire format.
9+
* Loaders today: BundledLoader (ships with the binary) and
10+
* PlatformLoader (fetches from codebase.foundation/api/cli/... when
11+
* signed in). LocalLoader (~/.codebase/skills/*.md) is queued.
12+
* Resolution order is platform > bundled, matching the "operator
13+
* overrides project" precedence the rest of the CLI uses.
1214
*/
1315
export interface AssetLoader {
1416
source: AssetSource;

src/skills/platform-loader.ts

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,10 @@ export interface PlatformLoaderOptions {
5757
* not signed in → returns [] without a network call
5858
* network error → returns the last cached body if any,
5959
* else [] (silent — never crashes a session)
60-
* 404 (endpoint TBD) → caches an empty bundle, returns []
61-
* (no warning — the backend half is
62-
* expected to land later, see
63-
* docs/plans/2026-05-09-codebase-cli-
64-
* oauth-server-side.md §3-4)
60+
* 404 → caches an empty bundle, returns []
61+
* (no warning — the user's tier may not
62+
* include skills, or the endpoint may not
63+
* be live yet on a particular deployment)
6564
* 200 → caches body + ETag, returns shaped list
6665
* 304 (ETag match) → bumps the cache timestamp, returns cached
6766
*/

src/tools/background-shell-store.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,14 +58,14 @@ describe("BackgroundShellStore", () => {
5858
expect(final?.endedAt).toBeGreaterThan(0);
5959
});
6060

61-
it("kill() on an unknown id is a no-op", async () => {
62-
await expect(store.kill("does-not-exist")).resolves.toBeUndefined();
61+
it("kill() on an unknown id reports not-found", async () => {
62+
await expect(store.kill("does-not-exist")).resolves.toEqual({ outcome: "not-found" });
6363
});
6464

65-
it("kill() on an already-exited shell is a no-op", async () => {
65+
it("kill() on an already-exited shell reports already-exited", async () => {
6666
const record = store.spawn("true", process.cwd());
6767
await waitUntil(() => (store.get(record.id)?.status !== "running" ? true : undefined));
68-
await expect(store.kill(record.id)).resolves.toBeUndefined();
68+
await expect(store.kill(record.id)).resolves.toEqual({ outcome: "already-exited" });
6969
});
7070

7171
it("killAllSync() SIGTERMs every running shell", async () => {

src/tools/background-shell-store.ts

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,18 @@ export interface BackgroundShellRecord {
1919

2020
export type BackgroundShellListener = (shells: readonly BackgroundShellRecord[]) => void;
2121

22+
/**
23+
* Result returned by `BackgroundShellStore.kill`. The caller needs to know
24+
* whether the process actually went down; a previous void return swallowed
25+
* the difference between "killed cleanly", "wasn't there", and "tried to
26+
* signal but the OS said no" — that hid real bugs in tool callers.
27+
*/
28+
export type BackgroundShellKillResult =
29+
| { outcome: "killed"; signal: "SIGTERM" | "SIGKILL" }
30+
| { outcome: "not-found" }
31+
| { outcome: "already-exited" }
32+
| { outcome: "signal-failed" };
33+
2234
/**
2335
* Tracks long-running shell processes that the agent spawned with
2436
* `shell({ background: true })`. The agent's tool turn returns
@@ -115,31 +127,50 @@ export class BackgroundShellStore {
115127
/**
116128
* Terminate a running shell. SIGTERM first; if the process is still
117129
* around after `gracePeriodMs`, SIGKILL. Resolves once the exit
118-
* handler has fired. No-op if the id is unknown or already exited.
130+
* handler has fired.
131+
*
132+
* Returns a structured result so the caller can act on the outcome:
133+
* - `"killed"` — process exited after our signal
134+
* - `"not-found"` — unknown id
135+
* - `"already-exited"` — id known but the process had already left
136+
* - `"signal-failed"` — both SIGTERM and SIGKILL threw (rare; e.g.
137+
* stale PID reuse). Likely already dead.
119138
*/
120-
async kill(id: string, gracePeriodMs = 2000): Promise<void> {
121-
const child = this.processes.get(id);
122-
if (!child) return;
139+
async kill(id: string, gracePeriodMs = 2000): Promise<BackgroundShellKillResult> {
140+
// Order matters: an exited process is dropped from `processes` but
141+
// stays in `records`. Check records first so we report
142+
// already-exited rather than not-found for shells we still know
143+
// about.
123144
const record = this.records.get(id);
124-
if (!record || record.status !== "running") return;
125-
return new Promise<void>((resolve) => {
145+
if (!record) return { outcome: "not-found" };
146+
if (record.status !== "running") return { outcome: "already-exited" };
147+
const child = this.processes.get(id);
148+
if (!child) return { outcome: "already-exited" };
149+
return new Promise<BackgroundShellKillResult>((resolve) => {
150+
let sigtermThrew = false;
151+
let sigkillThrew = false;
126152
const onExit = () => {
127153
clearTimeout(killTimer);
128-
resolve();
154+
if (sigtermThrew && sigkillThrew) {
155+
// Both threw but the process exited anyway — likely it was
156+
// already dying. Surface as signal-failed so the caller
157+
// can decide whether to retry differently.
158+
resolve({ outcome: "signal-failed" });
159+
return;
160+
}
161+
resolve({ outcome: "killed", signal: sigkillThrew ? "SIGTERM" : sigtermThrew ? "SIGKILL" : "SIGTERM" });
129162
};
130163
child.once("exit", onExit);
131164
try {
132165
child.kill("SIGTERM");
133166
} catch {
134-
// Process already gone; the exit handler may have fired
135-
// or be about to. Resolve once child emits or after the
136-
// timer trips.
167+
sigtermThrew = true;
137168
}
138169
const killTimer = setTimeout(() => {
139170
try {
140171
child.kill("SIGKILL");
141172
} catch {
142-
// Same — best effort.
173+
sigkillThrew = true;
143174
}
144175
}, gracePeriodMs);
145176
});

src/tools/registry.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import { createWriteFile } from "./write-file.js";
2828

2929
/**
3030
* Returns every built-in tool, configured against the given context.
31-
* Phase 2 commits append factories to this list one by one.
31+
* Add new tools by importing their factory and appending it here.
3232
*/
3333
export function buildTools(ctx: ToolContext): AgentTool<any>[] {
3434
return [

src/tools/shell-kill.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,19 @@ export function createShellKill(ctx: ToolContext): AgentTool<typeof Params> {
4545
],
4646
};
4747
}
48-
await ctx.backgroundShells.kill(params.task_id);
48+
const result = await ctx.backgroundShells.kill(params.task_id);
49+
if (result.outcome === "signal-failed") {
50+
return {
51+
details: undefined,
52+
content: [
53+
{
54+
type: "text",
55+
text: `Background shell ${params.task_id} did not respond to SIGTERM/SIGKILL — it may already be dead, or the OS refused the signal. Check with shell_output.`,
56+
},
57+
],
58+
isError: true,
59+
};
60+
}
4961
const after = ctx.backgroundShells.get(params.task_id);
5062
return {
5163
details: undefined,

0 commit comments

Comments
 (0)