Skip to content

Commit cf787b4

Browse files
committed
chore(audit-polish): optional improvements from verification audit
Verification audit of the previous audit-fixes commit (be1a68a) passed with no blocking issues. Applying the 3 optional low-priority suggestions from council synthesis: - `messages-transform.ts`: treat SQLITE_LOCKED as transient alongside SQLITE_BUSY (shared-cache conflicts are rare in our setup but covered defensively). Add write-if-changed guard for `lastTransformError`: persistent errors repeating on every transform pass no longer cause redundant WAL writes — we only persist when the summary changes. - `inject-compartments.ts`: update stale comment on the memory-token estimate. Old comment referenced the pre-tokenizer "22-char overhead" formula; new comment explains the real tokenizer pipeline and why the `+ 6` XML-tag allowance keeps sidebar math consistent. - `clear-message-tokens-cache.test.ts`: new focused tests covering the per-message cache invalidation path (previously only the session-wide path had test coverage). Exercises per-message delete, session-wide clear, no-op behavior on uncached sessions, and per-session isolation. Adds a test-only `__getMessageTokensCacheForTest` accessor on transform.ts (not exported from any barrel). Verified: 540 tests pass (up from 535 — 5 new), typecheck clean, build clean, lint clean.
1 parent be1a68a commit cf787b4

4 files changed

Lines changed: 110 additions & 10 deletions

File tree

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
/// <reference types="bun-types" />
2+
3+
/**
4+
* Focused tests for clearMessageTokensCache. The cache is consumed by the
5+
* transform path (see transform.ts) and invalidated from event-handler.ts on
6+
* message.removed / message.updated / session.compacted / session.deleted.
7+
*
8+
* These tests exercise the two invalidation modes directly so the per-message
9+
* path has coverage separate from the session-wide clear path.
10+
*/
11+
12+
import { describe, expect, it } from "bun:test";
13+
import { __getMessageTokensCacheForTest, clearMessageTokensCache } from "./transform";
14+
15+
describe("clearMessageTokensCache", () => {
16+
describe("#given cached tokens for two messages in one session", () => {
17+
it("#when called with a messageId, then only that entry is removed", () => {
18+
const sessionId = "ses-clear-per-message";
19+
const cache = __getMessageTokensCacheForTest(sessionId);
20+
cache.set("msg-a", { conversation: 100, toolCall: 0 });
21+
cache.set("msg-b", { conversation: 50, toolCall: 25 });
22+
23+
clearMessageTokensCache(sessionId, "msg-a");
24+
25+
expect(cache.has("msg-a")).toBe(false);
26+
expect(cache.has("msg-b")).toBe(true);
27+
expect(cache.get("msg-b")).toEqual({ conversation: 50, toolCall: 25 });
28+
});
29+
30+
it("#when called without a messageId, then the entire session cache is cleared", () => {
31+
const sessionId = "ses-clear-session-wide";
32+
const cache = __getMessageTokensCacheForTest(sessionId);
33+
cache.set("msg-a", { conversation: 100, toolCall: 0 });
34+
cache.set("msg-b", { conversation: 50, toolCall: 25 });
35+
36+
clearMessageTokensCache(sessionId);
37+
38+
const after = __getMessageTokensCacheForTest(sessionId);
39+
expect(after.size).toBe(0);
40+
});
41+
});
42+
43+
describe("#given no cached tokens for a session", () => {
44+
it("#when called with a messageId, then it is a no-op (no throw)", () => {
45+
expect(() => clearMessageTokensCache("ses-never-cached", "msg-x")).not.toThrow();
46+
});
47+
48+
it("#when called without a messageId, then it is a no-op (no throw)", () => {
49+
expect(() => clearMessageTokensCache("ses-never-cached")).not.toThrow();
50+
});
51+
});
52+
53+
describe("#given cached tokens for two sessions", () => {
54+
it("#when one session is cleared, then the other session's cache is untouched", () => {
55+
const s1 = "ses-isolation-1";
56+
const s2 = "ses-isolation-2";
57+
const c1 = __getMessageTokensCacheForTest(s1);
58+
const c2 = __getMessageTokensCacheForTest(s2);
59+
c1.set("m1", { conversation: 10, toolCall: 0 });
60+
c2.set("m2", { conversation: 20, toolCall: 0 });
61+
62+
clearMessageTokensCache(s1);
63+
64+
expect(__getMessageTokensCacheForTest(s1).size).toBe(0);
65+
expect(__getMessageTokensCacheForTest(s2).size).toBe(1);
66+
expect(__getMessageTokensCacheForTest(s2).get("m2")).toEqual({
67+
conversation: 20,
68+
toolCall: 0,
69+
});
70+
});
71+
});
72+
});

packages/plugin/src/hooks/magic-context/inject-compartments.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -131,9 +131,12 @@ function trimMemoriesToBudget(
131131
let usedTokens = 0;
132132

133133
for (const memory of sorted) {
134-
// Estimate the rendered memory line ("- {content}") plus category-tag
135-
// overhead using the real tokenizer. The 22-char overhead models the
136-
// opening/closing XML tags amortized per item.
134+
// Estimate the rendered memory line ("- {content}") with the real
135+
// Claude tokenizer, plus a fixed ~6-token allowance for opening and
136+
// closing XML category tags amortized per item. Keeps units
137+
// consistent with rpc-handlers.ts / transform.ts / system-prompt-hash.ts
138+
// so the sidebar's "Memories" segment matches what actually lands in
139+
// the injection block.
137140
const memoryTokens = estimateTokens(`- ${memory.content}`) + 6;
138141
if (usedTokens + memoryTokens > budgetTokens) {
139142
break;

packages/plugin/src/hooks/magic-context/transform.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,17 @@ export function clearMessageTokensCache(sessionId: string, messageId?: string):
8585
if (cache) cache.delete(messageId);
8686
}
8787

88+
/**
89+
* Test-only accessor that returns (and lazily creates) the per-session token
90+
* cache map so tests can seed and inspect entries without running the full
91+
* transform pipeline. Not exported from any barrel.
92+
*/
93+
export function __getMessageTokensCacheForTest(
94+
sessionId: string,
95+
): Map<string, { conversation: number; toolCall: number }> {
96+
return getMessageTokensCache(sessionId);
97+
}
98+
8899
/**
89100
* Extract the provider/model from the last assistant message in the array.
90101
* Used for early model-change detection before loadContextUsage.

packages/plugin/src/plugin/messages-transform.ts

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,14 @@
1-
import { openDatabase } from "../features/magic-context/storage";
1+
import { getOrCreateSessionMeta, openDatabase } from "../features/magic-context/storage";
22
import { updateSessionMeta } from "../features/magic-context/storage-meta-session";
33
import { log } from "../shared/logger";
44

5+
// Error codes that SQLite raises for transient contention — should be retried
6+
// on next transform pass rather than surfaced as persistent failures. BUSY is
7+
// by far the most common in WAL mode; LOCKED is theoretically possible when a
8+
// shared-cache conflict occurs (extremely rare in our single-DB setup but
9+
// covered defensively).
10+
const TRANSIENT_SQLITE_CODES = new Set(["SQLITE_BUSY", "SQLITE_LOCKED"]);
11+
512
type MessageWithParts = {
613
info: import("@opencode-ai/sdk").Message;
714
parts: import("@opencode-ai/sdk").Part[];
@@ -56,20 +63,20 @@ export function createMessagesTransformHandler(args: {
5663
const code = (error as { code?: string } | null)?.code;
5764
const name = (error as { name?: string } | null)?.name;
5865
const message = error instanceof Error ? error.message : String(error);
59-
const isBusy = code === "SQLITE_BUSY";
66+
const isTransient = typeof code === "string" && TRANSIENT_SQLITE_CODES.has(code);
6067

61-
if (isBusy) {
68+
if (isTransient) {
6269
log(
63-
`[magic-context] transform skipped this pass — SQLITE_BUSY (another writer holds lock, retrying next pass): ${message}`,
70+
`[magic-context] transform skipped this pass — ${code} (transient; retrying next pass): ${message}`,
6471
);
6572
return;
6673
}
6774

68-
// Persistent non-BUSY errors are the real risk: silent forever
75+
// Persistent non-transient errors are the real risk: silent forever
6976
// disable unless we surface them. Persist to session_meta so the
7077
// sidebar shows an obvious failure indicator.
7178
log(
72-
`[magic-context] transform FAILED (non-BUSY) code=${code ?? "none"} name=${name ?? "none"}: ${message}. Continuing with unmodified messages for this pass.`,
79+
`[magic-context] transform FAILED code=${code ?? "none"} name=${name ?? "none"}: ${message}. Continuing with unmodified messages for this pass.`,
7380
error,
7481
);
7582

@@ -81,7 +88,14 @@ export function createMessagesTransformHandler(args: {
8188
try {
8289
const db = openDatabase();
8390
const summary = truncateError(name, code, message);
84-
updateSessionMeta(db, sessionId, { lastTransformError: summary });
91+
// Write-if-changed guard: when the same error repeats on
92+
// every transform pass (e.g. persistent schema corruption),
93+
// skip the DB write if lastTransformError already matches.
94+
// Prevents needless WAL churn during degraded operation.
95+
const current = getOrCreateSessionMeta(db, sessionId).lastTransformError;
96+
if (current !== summary) {
97+
updateSessionMeta(db, sessionId, { lastTransformError: summary });
98+
}
8599
} catch (persistError) {
86100
// Swallow — if we can't even write the error, we definitely
87101
// can't recover. Next pass may succeed.

0 commit comments

Comments
 (0)