Skip to content

Commit 1fa3f89

Browse files
committed
refactor(logging): collapse double-log storms, fix levels, correlate ids
Second-layer duplicates (store settlement, submit sent-append) collapse to single entries; per-field trust warn storms become one summary warn, with a per-file error shape on the migration result. Mis-leveled errors demote to warn; warn churn during teardown demotes to debug. caughtDebug is the single append-failure helper for persist/teardown catches. The exec/exit error paths it guards carry sessionId from where the id is already in scope; the MCP late-connect/retry warns name the server. The collapsed per-entry absolute resolution uses an explicit lambda (paths.map((p) => resolve(p))) so map's (value, index, array) triple can never reach path.resolve — avoiding the plan draft's would-crash shape (probe-verified: restoring bare .map(resolve) fails the trust store suite with ERR_INVALID_ARG_TYPE). The parent commit always had the safe per-entry loop; nothing here fixes a shipped crash. The migration's one-summary-warn contract is pinned by a log-capture test listing each skipped store.
1 parent 1bfc073 commit 1fa3f89

20 files changed

Lines changed: 303 additions & 201 deletions

‎src/cost/pricing-fetcher.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -306,9 +306,7 @@ export function startPricingRefresh(
306306
const refreshIntervalMs =
307307
options.refreshIntervalMs ?? DEFAULT_REFRESH_INTERVAL_MS;
308308
const timer = setInterval(() => {
309-
loadPricing(options).catch((err: unknown) => {
310-
process.stderr.write(`pricing-fetcher: refresh error: ${err}\n`);
311-
});
309+
void loadPricing(options);
312310
}, refreshIntervalMs);
313311
return timer;
314312
}

‎src/cost/pricing-metadata.ts‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,9 @@ export function schedulePricingMetadataRefresh(
2929
): void {
3030
if (refreshScheduled) return;
3131
refreshScheduled = true;
32-
void loadPricing(options)
33-
.then((cache) => {
34-
if (cache !== null) applyPricingCacheMetadata(cache);
35-
})
36-
.catch((err: unknown) => {
37-
// Match pricing-fetcher: keep refresh best-effort, surface the failure.
38-
process.stderr.write(`pricing-metadata: refresh error: ${err}\n`);
39-
});
32+
void loadPricing(options).then((cache) => {
33+
if (cache !== null) applyPricingCacheMetadata(cache);
34+
});
4035
}
4136

4237
export async function bootstrapPricingMetadata(

‎src/exec/dispose.ts‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { getLogger } from "@intx/log";
22

33
import { LOG_NAMESPACE_ROOT } from "../branding.js";
4+
import { caughtDebug } from "../logging/caught.js";
45
import { awaitCloseWithoutHidingLeftover } from "../subagent/dispose.js";
56
import type { SubAgentSessionStore } from "../subagent/index.js";
67

@@ -59,9 +60,11 @@ async function runExecDispose(args: {
5960
try {
6061
await args.toolset.dispose();
6162
} catch (err: unknown) {
62-
logger.debug("toolset.dispose during exec finally failed: {error}", {
63-
error: (args.sanitizeDiagnostic ?? String)(formatCaughtError(err)),
64-
});
63+
caughtDebug(
64+
logger,
65+
"toolset.dispose during exec finally failed",
66+
args.sanitizeDiagnostic,
67+
)(err);
6568
failures.push(err);
6669
}
6770
}
@@ -74,9 +77,11 @@ async function runExecDispose(args: {
7477
try {
7578
await awaitCloseWithoutHidingLeftover(args.agent.close(), failures[0]);
7679
} catch (err: unknown) {
77-
logger.debug("agent.close during exec finally failed: {error}", {
78-
error: (args.sanitizeDiagnostic ?? String)(formatCaughtError(err)),
79-
});
80+
caughtDebug(
81+
logger,
82+
"agent.close during exec finally failed",
83+
args.sanitizeDiagnostic,
84+
)(err);
8085
failures.push(err);
8186
}
8287
}

‎src/exec/runner.ts‎

Lines changed: 50 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ import {
4242
} from "../subagent/index.js";
4343
import { getProcessAdmissionQueue } from "../subagent/admission.js";
4444
import { disposeExecRuntime, formatCaughtError } from "./dispose.js";
45+
import { caughtDebug } from "../logging/caught.js";
4546
import {
4647
EXEC_MCP_CONNECT_WAIT_MS,
4748
EXEC_MCP_HANDSHAKE_TIMEOUT_MS,
@@ -468,7 +469,10 @@ export async function runExec(config: Config): Promise<ExecResult> {
468469
priorState = prior?.kind === "ok" ? prior.state : undefined;
469470
} catch (err) {
470471
const message = formatCaughtError(err);
471-
logger.error("exec setup failed: {error}", { error: message });
472+
logger.error("exec setup failed: {error}", {
473+
sessionId,
474+
error: message,
475+
});
472476
stderr.write(`Error: ${message}\n`);
473477
liveTelemetry.capture(
474478
"session_end",
@@ -598,11 +602,13 @@ export async function runExec(config: Config): Promise<ExecResult> {
598602

599603
try {
600604
// Pricing seed is optional for exec; continue without rates.
601-
const inferenceDeps = await assembleInferenceBase((err: unknown) => {
602-
logger.debug("seedPricingMetadataFromCache failed: {error}", {
603-
error: sanitizeExecDiagnostic(formatCaughtError(err)),
604-
});
605-
});
605+
const inferenceDeps = await assembleInferenceBase(
606+
caughtDebug(
607+
logger,
608+
"seedPricingMetadataFromCache failed",
609+
sanitizeExecDiagnostic,
610+
),
611+
);
606612

607613
// One-shot migration only when the path-trust file is absent. Headless
608614
// exec has no frame to corrupt, so skipped members write straight to stderr.
@@ -1217,22 +1223,22 @@ export async function runExec(config: Config): Promise<ExecResult> {
12171223
// entry and would write the whole reply as a spurious partial.
12181224
// Dispose is a no-op on success and a real record only if a cycle
12191225
// died without a terminal event.
1220-
await activeAgent.close().catch((err: unknown) => {
1221-
logger.debug(
1222-
"agent.close during successful-send teardown failed: {error}",
1223-
{
1224-
error: sanitizeExecDiagnostic(formatCaughtError(err)),
1225-
},
1226-
);
1227-
});
1228-
await streamPromise.catch((err: unknown) => {
1229-
logger.debug(
1230-
"stream drain during successful-send teardown failed: {error}",
1231-
{
1232-
error: sanitizeExecDiagnostic(formatCaughtError(err)),
1233-
},
1226+
await activeAgent
1227+
.close()
1228+
.catch(
1229+
caughtDebug(
1230+
logger,
1231+
"agent.close during successful-send teardown failed",
1232+
sanitizeExecDiagnostic,
1233+
),
12341234
);
1235-
});
1235+
await streamPromise.catch(
1236+
caughtDebug(
1237+
logger,
1238+
"stream drain during successful-send teardown failed",
1239+
sanitizeExecDiagnostic,
1240+
),
1241+
);
12361242
await cycleRecorder.dispose("cancelled");
12371243
} else {
12381244
// Failed or aborted send: close() tears down stream consumers before
@@ -1241,22 +1247,22 @@ export async function runExec(config: Config): Promise<ExecResult> {
12411247
await cycleRecorder.dispose(
12421248
sendCompleted ? "cancelled" : "send-failed",
12431249
);
1244-
await activeAgent.close().catch((err: unknown) => {
1245-
logger.debug(
1246-
"agent.close during failed-send teardown failed: {error}",
1247-
{
1248-
error: sanitizeExecDiagnostic(formatCaughtError(err)),
1249-
},
1250-
);
1251-
});
1252-
await streamPromise.catch((err: unknown) => {
1253-
logger.debug(
1254-
"stream drain during failed-send teardown failed: {error}",
1255-
{
1256-
error: sanitizeExecDiagnostic(formatCaughtError(err)),
1257-
},
1250+
await activeAgent
1251+
.close()
1252+
.catch(
1253+
caughtDebug(
1254+
logger,
1255+
"agent.close during failed-send teardown failed",
1256+
sanitizeExecDiagnostic,
1257+
),
12581258
);
1259-
});
1259+
await streamPromise.catch(
1260+
caughtDebug(
1261+
logger,
1262+
"stream drain during failed-send teardown failed",
1263+
sanitizeExecDiagnostic,
1264+
),
1265+
);
12601266
}
12611267
}
12621268

@@ -1350,7 +1356,10 @@ export async function runExec(config: Config): Promise<ExecResult> {
13501356
return result;
13511357
} catch (err) {
13521358
const diagnosticMessage = sanitizeExecDiagnostic(formatCaughtError(err));
1353-
logger.error("exec failed: {error}", { error: diagnosticMessage });
1359+
logger.error("exec failed: {error}", {
1360+
sessionId,
1361+
error: diagnosticMessage,
1362+
});
13541363
const userMessage = sanitizeExecDiagnostic(
13551364
execUserFailureMessage(
13561365
config,
@@ -1391,7 +1400,10 @@ export async function runExec(config: Config): Promise<ExecResult> {
13911400
});
13921401
} catch (err: unknown) {
13931402
const message = sanitizeExecDiagnostic(formatCaughtError(err));
1394-
logger.error("runtime dispose failed: {error}", { error: message });
1403+
logger.error("runtime dispose failed: {error}", {
1404+
sessionId,
1405+
error: message,
1406+
});
13951407
stderr.write(`Error: runtime dispose failed: ${message}\n`);
13961408
if (result !== undefined) {
13971409
result.exitCode = 1;

‎src/logging/caught.test.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
import { describe, expect, test } from "bun:test";
2+
3+
import { caughtDebug } from "./caught.js";
4+
5+
// @intx/log renders the {error} template at sink level, so capture both the
6+
// message template and the properties to pin the normalization-and-append
7+
// contract.
8+
describe("caughtDebug", () => {
9+
interface Capture {
10+
message: string;
11+
properties: Record<string, unknown> | undefined;
12+
}
13+
const capture = {
14+
debug: (message: string, properties?: Record<string, unknown>) => {
15+
capture.last = { message, properties };
16+
},
17+
last: undefined as Capture | undefined,
18+
};
19+
20+
test("Error values feed {error} from .message", () => {
21+
caughtDebug(capture, "teardown failed")(new Error("boom"));
22+
expect(capture.last?.message).toBe("teardown failed: {error}");
23+
expect(capture.last?.properties).toEqual({ error: "boom" });
24+
});
25+
26+
test("non-Error values are String()ed", () => {
27+
caughtDebug(capture, "teardown failed")("boom-string");
28+
expect(capture.last?.properties).toEqual({ error: "boom-string" });
29+
});
30+
31+
test("a sanitizer runs on the normalized error message", () => {
32+
caughtDebug(capture, "teardown failed", (error) =>
33+
error.replaceAll("secret", "[redacted]"),
34+
)(new Error("secret leaked"));
35+
expect(capture.last?.properties).toEqual({
36+
error: "[redacted] leaked",
37+
});
38+
});
39+
});

‎src/logging/caught.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// Shared hunter for the "best-effort step failed" debug catch: callers keep
2+
// their message; the helper owns the caught-value normalization-and-append
3+
// shape so that boilerplate exists once, not per site.
4+
interface DebugSink {
5+
debug: (message: string, properties?: Record<string, unknown>) => void;
6+
}
7+
8+
export function caughtDebug(
9+
logger: DebugSink,
10+
message: string,
11+
sanitize?: (error: string) => string,
12+
): (err: unknown) => void {
13+
return (err) => {
14+
const error = err instanceof Error ? err.message : String(err);
15+
logger.debug(`${message}: {error}`, { error: (sanitize ?? String)(error) });
16+
};
17+
}

‎src/perf/otel-sink.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,9 @@ export async function flushPerfToOtel(
270270
try {
271271
const resolved = resolveOtelExportConfig(settings, env);
272272
if (!resolved.ok) {
273-
log.warn("OTEL export skipped: {message}", { message: resolved.message });
273+
log.debug("OTEL export skipped: {message}", {
274+
message: resolved.message,
275+
});
274276
return;
275277
}
276278
config = resolved.config;

‎src/permission/approval-store-migration.test.ts‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
} from "node:fs/promises";
1010
import { tmpdir } from "node:os";
1111
import { dirname, join } from "node:path";
12+
import { installFileLogSink } from "../logging/sink.js";
1213
import { generateSessionId, sessionDir } from "../session/index.js";
1314
import { loadSeededApprovals } from "../session/runtime-assembly.js";
1415
import { normalizeSeededApprovals } from "./authz-grants.js";
@@ -297,6 +298,56 @@ describe("migratePersistedApprovalStores", () => {
297298
}
298299
});
299300

301+
test("per-file migration failures surface once as a single summary warn", async () => {
302+
const logDir = await mkdtemp(join(tmpdir(), "approval-migration-log-"));
303+
installFileLogSink(join(logDir, "corbits.log"));
304+
const sessionOriginal = {
305+
approvals: [
306+
{ tool: "update_plan", pattern: "plan *" },
307+
{ tool: "run_shell", pattern: "npm *" },
308+
],
309+
};
310+
await writeFile(sessionStorePath(), JSON.stringify(sessionOriginal));
311+
await mkdir(join(cwd, ".corbits"), { recursive: true });
312+
await writeFile(
313+
projectStorePath(),
314+
JSON.stringify({
315+
approvals: [{ tool: "update_plan", pattern: "plan *" }],
316+
}),
317+
);
318+
// Read-only store dirs make the backup step fail, so both stores skip
319+
// untouched while the global path (absent) stays a clean no-op.
320+
const sessionStoreDir = dirname(sessionStorePath());
321+
const projectStoreDir = join(cwd, ".corbits");
322+
await chmod(sessionStoreDir, 0o555);
323+
await chmod(projectStoreDir, 0o555);
324+
try {
325+
const result = await migratePersistedApprovalStores(cwd, sessionId, home);
326+
expect(result.purged).toBe(0);
327+
const failed = result.files.filter((file) => file.error !== undefined);
328+
expect(failed).toHaveLength(2);
329+
for (const file of failed) {
330+
expect(file.error).toContain("backup failed");
331+
expect(file.purged).toBe(0);
332+
}
333+
334+
const lines = (await readFile(join(logDir, "corbits.log"), "utf8"))
335+
.split("\n")
336+
.filter((line) => line.includes("Skipped approval-store migration"));
337+
// One summary warn for the whole migration, not one per failing store.
338+
expect(lines).toHaveLength(1);
339+
const summary = lines[0];
340+
if (summary === undefined) throw new Error("summary warn missing");
341+
expect(summary).toMatch(/"level":"warning"/);
342+
expect(summary).toContain(sessionStorePath());
343+
expect(summary).toContain(projectStorePath());
344+
} finally {
345+
await chmod(sessionStoreDir, 0o755);
346+
await chmod(projectStoreDir, 0o755);
347+
await rm(logDir, { recursive: true, force: true });
348+
}
349+
});
350+
300351
test("existing torn .bak plus live update_plan still purges live", async () => {
301352
const path = sessionStorePath();
302353
const original = {

0 commit comments

Comments
 (0)