Skip to content

Commit 4f6d5f6

Browse files
committed
fix(session): surface swallowed store/resume errors instead of silent defaults
A torn permissions approvals file read back as an empty store and any subsequent grant write permanently discarded prior allow-always grants with nothing surfaced; reads now distinguish ENOENT from corruption and a corrupt file aborts before the read-modify-write can clobber. Stall-after-interrupt approvals collapsed every resume-path throw into a bare 'refused' that looked identical to a system decline; the refusal now carries a scrubbed cause and a structured warn. The seven silent swallow sites (aged image markers, dropped handoffs, compaction evidence, corrupt MCP oauth backup, evidence-archive skips, drain failures, stale model picker) each log with their identifier.
1 parent 8b3cf2c commit 4f6d5f6

18 files changed

Lines changed: 390 additions & 52 deletions

‎src/auth/oauth-scope-check.test.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,24 @@ describe("checkOAuthProviderScope", () => {
181181
commandName,
182182
);
183183
expect(result.status).toBe("unavailable");
184+
// The inconclusive outcome names the inconclusive evidence: the HTTP
185+
// status is the only thing this check ever surfaces about the body.
186+
if (result.status === "unavailable")
187+
expect(result.message).toContain("provider HTTP 500");
188+
});
189+
190+
test("a timed-out probe says timed out, not network down", async () => {
191+
stubFetch(() => {
192+
throw new DOMException("The operation timed out.", "TimeoutError");
193+
});
194+
const result = await checkOAuthProviderScope(
195+
"codex",
196+
codexTokens,
197+
commandName,
198+
);
199+
expect(result.status).toBe("unavailable");
200+
if (result.status === "unavailable")
201+
expect(result.message).toContain("probe timed out");
184202
});
185203

186204
test("xai: builds the probe from staged tokens", async () => {

‎src/auth/oauth-scope-check.ts‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,13 +46,22 @@ function insufficientScope(providerLabel: string): OAuthScopeCheckResult {
4646
};
4747
}
4848

49-
function unavailable(providerLabel: string): OAuthScopeCheckResult {
49+
function unavailable(
50+
providerLabel: string,
51+
cause?: string,
52+
): OAuthScopeCheckResult {
53+
const why = cause === undefined ? "" : ` (${cause})`;
5054
return {
5155
status: "unavailable",
52-
message: `Couldn't confirm ${providerLabel} API access right now — continuing without blocking setup.`,
56+
message: `Couldn't confirm ${providerLabel} API access right now${why} — continuing without blocking setup.`,
5357
};
5458
}
5559

60+
/** AbortSignal.timeout's rejection, so "network down" and "probe hung" differ. */
61+
function isTimeout(err: unknown): boolean {
62+
return err instanceof Error && err.name === "TimeoutError";
63+
}
64+
5665
function invalidCredentials(providerLabel: string): OAuthScopeCheckResult {
5766
return {
5867
status: "blocked",
@@ -94,7 +103,7 @@ function classifyStatus(
94103
providerLabel: string,
95104
): OAuthScopeCheckResult {
96105
if (status === 401 || status === 403) return insufficientScope(providerLabel);
97-
return unavailable(providerLabel);
106+
return unavailable(providerLabel, `provider HTTP ${status}`);
98107
}
99108

100109
async function checkCodexScope(
@@ -117,7 +126,10 @@ async function checkCodexScope(
117126
} catch (err) {
118127
if (isDefinitiveRefreshAuthRejection(err))
119128
return invalidCredentials(providerLabel);
120-
return unavailable(providerLabel);
129+
return unavailable(
130+
providerLabel,
131+
isTimeout(err) ? "probe timed out" : undefined,
132+
);
121133
}
122134
}
123135

@@ -138,7 +150,10 @@ async function checkXaiScope(
138150
} catch (err) {
139151
if (isDefinitiveRefreshAuthRejection(err))
140152
return invalidCredentials(providerLabel);
141-
return unavailable(providerLabel);
153+
return unavailable(
154+
providerLabel,
155+
isTimeout(err) ? "probe timed out" : undefined,
156+
);
142157
}
143158
}
144159

‎src/mcp/auth-store.test.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,4 +321,29 @@ describe("mcp auth-store", () => {
321321
final.tokens === undefined || final.tokens.access_token === "v1",
322322
).toBe(true);
323323
});
324+
325+
test("a corrupt auth file is backed up before the next overwrite", async () => {
326+
const home = await tempHome();
327+
const path = authFilePath(linear, home);
328+
await mkdir(dirname(path), { recursive: true });
329+
await writeFile(path, "{corrupt", { mode: 0o600 });
330+
331+
// Corrupt state reads as absent (a fresh authorization is offered), but
332+
// the corrupt bytes survive for diagnosis.
333+
expect(await loadAuthState(linear, home)).toEqual({});
334+
expect(await readFile(`${path}.corrupt`, "utf8")).toBe("{corrupt");
335+
336+
// The overwrite behind a fresh authorization cannot consume the last copy.
337+
await updateAuthState(
338+
linear,
339+
(state) => {
340+
state.tokens = { access_token: "tok", token_type: "bearer" };
341+
},
342+
home,
343+
);
344+
expect((await loadAuthState(linear, home)).tokens?.access_token).toBe(
345+
"tok",
346+
);
347+
expect(await readFile(`${path}.corrupt`, "utf8")).toBe("{corrupt");
348+
});
324349
});

‎src/mcp/auth-store.ts‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,20 @@ import {
88
unlink,
99
writeFile,
1010
} from "node:fs/promises";
11-
import { readFileSync } from "node:fs";
11+
import { readFileSync, writeFileSync } from "node:fs";
1212
import { homedir } from "node:os";
1313
import { dirname, join } from "node:path";
1414
import { setTimeout as delay } from "node:timers/promises";
15+
import { getLogger } from "@intx/log";
1516
import type {
1617
OAuthClientInformationFull,
1718
OAuthTokens,
1819
} from "@modelcontextprotocol/sdk/shared/auth.js";
20+
import { LOG_NAMESPACE_ROOT } from "../branding.js";
1921
import { SETTINGS_DIR_NAME } from "../branding.js";
2022

23+
const logger = getLogger([LOG_NAMESPACE_ROOT, "mcp", "auth-store"]);
24+
2125
// Per-server OAuth state. The durable pair — registered client info and
2226
// tokens — reaches disk; the PKCE verifier stays instance-local in the
2327
// provider. Legacy codeVerifier keys are stripped on write.
@@ -74,20 +78,32 @@ export function isEnoent(err: unknown): boolean {
7478
);
7579
}
7680

77-
function parseAuthState(raw: string): MCPAuthState | undefined {
81+
// Corrupt credentials are backed up (never logged and never discarded with
82+
// the next overwrite) so an operator can diagnose a half-written tokens file.
83+
// The backup keeps owner-only mode like the auth file itself.
84+
function preserveCorruptAuthFile(path: string, raw: string): void {
85+
try {
86+
writeFileSync(`${path}.corrupt`, raw, { mode: 0o600 });
87+
} catch {
88+
// Backing up is best-effort; the fresh authorization proceeds either way.
89+
}
90+
logger.warn`corrupt MCP auth file read as no state path=${path}`;
91+
}
92+
93+
function parseAuthState(raw: string, path: string): MCPAuthState | undefined {
7894
try {
7995
const parsed: unknown = JSON.parse(raw);
8096
if (typeof parsed === "object" && parsed !== null)
8197
return parsed as MCPAuthState;
8298
} catch {
83-
// Corrupt auth file: treat as no state; a fresh authorization overwrites it.
99+
preserveCorruptAuthFile(path, raw);
84100
}
85101
return undefined;
86102
}
87103

88-
function stateFromRaw(raw: string | undefined): MCPAuthState {
104+
function stateFromRaw(raw: string | undefined, path: string): MCPAuthState {
89105
if (raw === undefined) return {};
90-
return parseAuthState(raw) ?? {};
106+
return parseAuthState(raw, path) ?? {};
91107
}
92108

93109
function readAuthFileSync(path: string): string | undefined {
@@ -112,7 +128,8 @@ export async function loadAuthState(
112128
identity: MCPAuthIdentity,
113129
home: string = homedir(),
114130
): Promise<MCPAuthState> {
115-
return stateFromRaw(await readAuthFile(authFilePath(identity, home)));
131+
const path = authFilePath(identity, home);
132+
return stateFromRaw(await readAuthFile(path), path);
116133
}
117134

118135
// Cache refresh: missing/unreadable/corrupt files return undefined so the
@@ -121,14 +138,15 @@ export function tryLoadAuthStateSync(
121138
identity: MCPAuthIdentity,
122139
home: string = homedir(),
123140
): MCPAuthState | undefined {
141+
const path = authFilePath(identity, home);
124142
let raw: string | undefined;
125143
try {
126-
raw = readAuthFileSync(authFilePath(identity, home));
144+
raw = readAuthFileSync(path);
127145
} catch {
128146
return undefined;
129147
}
130148
if (raw === undefined) return undefined;
131-
return parseAuthState(raw);
149+
return parseAuthState(raw, path);
132150
}
133151

134152
function isEexist(err: unknown): boolean {

‎src/mcp/plugin.ts‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type { AgentTool } from "@intx/agent";
22
import type { ToolCall, ToolResult } from "@intx/types/runtime";
33
import type { PermissionGate } from "../permission/gate.js";
44
import { gateAgentTools } from "../plugins/permission-plugin.js";
5+
import { getLogger } from "@intx/log";
56
import {
67
scrubSecretShapedContent,
78
scrubSecretShapedValue,
@@ -20,10 +21,13 @@ import {
2021
} from "./client.js";
2122
import { applyMcpResultProjection } from "./result-projection.js";
2223
import { mcpToolName } from "./tool-name.js";
24+
import { LOG_NAMESPACE_ROOT } from "../branding.js";
2325

2426
export const MCP_RECONNECTING_TOOL_ERROR =
2527
"MCP server is reconnecting; retry the call once it reports connected.";
2628

29+
const logger = getLogger([LOG_NAMESPACE_ROOT, "mcp", "plugin"]);
30+
2731
/**
2832
* Stable marker prefixing JSON-serialized `structuredContent` (alone or
2933
* supplementing an empty flatten); distinguishes server data from free text.
@@ -156,8 +160,12 @@ export function mcpClientTools(
156160
provenance: "mcp:post-policy-pre-flatten",
157161
});
158162
archivedFullEnvelope = true;
159-
} catch {
160-
// Archive write must not fail a successful tool result.
163+
} catch (err) {
164+
// Archive write must not fail a successful tool result; the
165+
// skip still gets a receipt with the call it dropped. The
166+
// result path is secret scrubbed throughout — the receipt is too.
167+
const message = err instanceof Error ? err.message : String(err);
168+
logger.warn`evidence archive skipped for successful MCP tool result call=${call.id} error=${scrubSecretShapedContent(message)}`;
161169
}
162170
}
163171
const projected = isError
@@ -229,8 +237,15 @@ export function mcpClientTools(
229237
callId: call.id,
230238
provenance: "mcp:error",
231239
});
232-
} catch {
233-
// Archive write must not fail a successful tool result.
240+
} catch (archiveErr) {
241+
// archiveErr here comes from the archive writer, never the
242+
// payload, but the result path is secret scrubbed throughout —
243+
// the receipt is too.
244+
const message =
245+
archiveErr instanceof Error
246+
? archiveErr.message
247+
: String(archiveErr);
248+
logger.warn`evidence archive skipped for failed MCP tool result call=${call.id} underlying=${scrubbed} error=${scrubSecretShapedContent(message)}`;
234249
}
235250
}
236251
return {

‎src/permission/store.test.ts‎

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
2-
import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises";
2+
import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
33
import { tmpdir } from "node:os";
44
import { join } from "node:path";
5+
import { SETTINGS_DIR_NAME } from "../branding.js";
56
import {
67
loadGlobalApprovals,
78
loadPendingProjectApprovals,
@@ -188,3 +189,58 @@ describe("provider-model store", () => {
188189
]);
189190
});
190191
});
192+
193+
// A torn (non-JSON) store file must surface, never silently report an empty
194+
// store: the empty read is the first half of read-modify-write, so a swallowed
195+
// corruption reconciles the file's grants away on the next write. Repairs
196+
// always work because the write path refuses to touch the corrupt bytes.
197+
describe("torn store recovery", () => {
198+
const globalStorePathFor = (): string =>
199+
join(home, SETTINGS_DIR_NAME, "permissions.json");
200+
201+
test("a corrupt global store surfaces on load instead of reporting zero grants", async () => {
202+
await saveGlobalApproval({ tool: "run_shell", pattern: "git *" }, home);
203+
await writeFile(globalStorePathFor(), "{torn");
204+
await expect(loadGlobalApprovals(home)).rejects.toThrow(
205+
/permissions store is corrupt/,
206+
);
207+
});
208+
209+
test("a corrupt provider-model store surfaces on load instead of reporting zero grants", async () => {
210+
await saveProviderModelApproval(
211+
"openai:gpt-5",
212+
{ tool: "run_shell", pattern: "npm *" },
213+
home,
214+
);
215+
await writeFile(globalStorePathFor(), "{torn");
216+
await expect(loadProviderModelApprovals(home)).rejects.toThrow(
217+
/permissions store is corrupt/,
218+
);
219+
});
220+
221+
test("a later grant write cannot clobber a torn store's prior grants", async () => {
222+
await saveGlobalApproval({ tool: "run_shell", pattern: "git *" }, home);
223+
await saveGlobalApproval({ tool: "run_shell", pattern: "npm *" }, home);
224+
const path = globalStorePathFor();
225+
const original = await readFile(path, "utf-8");
226+
// Simulate a torn write: the bytes are a shortened prefix of the store.
227+
const torn = original.slice(0, Math.max(original.length - 4, 0));
228+
await writeFile(path, torn);
229+
230+
await expect(
231+
saveGlobalApproval({ tool: "run_shell", pattern: "cargo *" }, home),
232+
).rejects.toThrow(/permissions store is corrupt/);
233+
234+
// The failed write left the corrupt bytes untouched for an operator repair.
235+
expect(await readFile(path, "utf-8")).toBe(torn);
236+
237+
// Repair regains the prior grants; the deferred grant re-applies on retry.
238+
await writeFile(path, original);
239+
await saveGlobalApproval({ tool: "run_shell", pattern: "cargo *" }, home);
240+
expect(await loadGlobalApprovals(home)).toEqual([
241+
{ tool: "run_shell", pattern: "git *" },
242+
{ tool: "run_shell", pattern: "npm *" },
243+
{ tool: "run_shell", pattern: "cargo *" },
244+
]);
245+
});
246+
});

0 commit comments

Comments
 (0)