Skip to content

Commit be1a68a

Browse files
committed
fix(audit): address Athena council findings from solo v0.10.1+ audit
Council flagged concrete correctness and observability bugs introduced (or left untouched) by the recent tokenizer/sidebar work. All fixes apply to uncommitted changes so no shipped behavior regresses. Correctness fixes: - `strip-content.ts` — `stripReasoningFromMergedAssistants` now strips `thinking` and `redacted_thinking` part types in addition to OpenCode's internal `reasoning`. Opus 4.7 emits wire-format `thinking` parts, and the workaround's whole purpose (keep thinking at position 0 in the merged Anthropic block) requires handling every reasoning-like type. Without this, two consecutive assistants each carrying a `thinking` block pass through unchanged and produce the exact "thinking blocks … cannot be modified" 400 the function was written to prevent. Adds 3 new tests covering `thinking`-typed consecutive runs and mixed type sequences. - `messages-transform.ts` — distinguish SQLITE_BUSY (transient, log and skip) from persistent non-BUSY errors (log with full detail and persist a summary into `session_meta.last_transform_error`). The sidebar already reads that field, so persistent schema/programming failures now surface as a visible failure indicator instead of disabling magic-context silently forever. - `event-handler.ts` + `event-payloads.ts` — invalidate the per-message token cache on `message.updated` (per-message, falls back to session-wide when the event lacks a message id) and on `session.compacted` (session-wide, since native compaction restructures messages). `MessageUpdatedAssistantInfo` gains an optional `messageID` sourced from `info.id`. Hardening fixes: - `inject-compartments.ts` — memory trim-to-budget now uses `estimateTokens` instead of chars/4, matching the rest of the plugin's token math. Removes the last unit-mismatched budget path in the injection pipeline. - `image-token-estimate.ts` — `readUint32BE` now coerces via `>>> 0` so PNG headers with MSB-set bytes produce the correct unsigned value instead of a negative int that bypasses the `< 1` fallback. Removes dead `|| 0` in the WebP lossy parser; the `& 0x3fff` mask already produces a non-negative result. Tests: - `transform-index-staleness.test.ts` — the "clears reasoning before dropped messages" regression expected `m-reason-b`'s `thinking` to survive after pruning, but that expectation was only valid while `stripReasoningFromMergedAssistants` ignored `thinking` parts (the bug fixed in #2 above). Updated the assertion and comment to reflect the correct interaction: after pruning collapses adjacent assistants, the merge-strip correctly removes `thinking` from every assistant past the first in the run, even when the watermark wouldn't reach it. Verified: 535 plugin tests pass, typecheck clean, build clean, lint clean (pre-existing Intentional: warnings only). Skipped findings (documented in synthesis.md): - #10 self-heal oscillation (sticky-date already stabilizes main variance) - #11 non-image attachments counted as 0 (would require document tokenization) - #12 residual clamp masks drift (clamp-to-0 is more user-friendly than negative)
1 parent 4fbeba6 commit be1a68a

8 files changed

Lines changed: 398 additions & 47 deletions

File tree

packages/plugin/src/hooks/magic-context/event-handler.ts

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ import {
3939
resolveSessionId,
4040
} from "./event-resolvers";
4141
import { clearNoteNudgeState } from "./note-nudger";
42-
import type { NudgePlacementStore } from "./transform";
42+
import { clearMessageTokensCache, type NudgePlacementStore } from "./transform";
4343
import { clearCompressorCooldown } from "./transform-compartment-phase";
4444

4545
const CONTEXT_USAGE_TTL_MS = 60 * 60 * 1000;
@@ -215,6 +215,17 @@ export function createEventHandler(deps: EventHandlerDeps) {
215215
return;
216216
}
217217

218+
// Invalidate this message's cached token contribution. The message
219+
// content is finalized at this event — if a prior transform pass
220+
// happened to cache partial/streaming content (or the message is
221+
// being edited/retried), the next pass must recompute. We fall
222+
// back to session-wide clear when the event lacks a message id.
223+
if (info.messageID) {
224+
clearMessageTokensCache(info.sessionID, info.messageID);
225+
} else {
226+
clearMessageTokensCache(info.sessionID);
227+
}
228+
218229
const now = Date.now();
219230
const usageTokens = [
220231
info.tokens?.input,
@@ -400,6 +411,10 @@ export function createEventHandler(deps: EventHandlerDeps) {
400411
);
401412
}
402413

414+
// Invalidate this message's cached token contribution so the
415+
// next transform pass recomputes without stale data.
416+
clearMessageTokensCache(info.sessionID, info.messageID);
417+
403418
deps.onSessionCacheInvalidated?.(info.sessionID);
404419
sessionLog(
405420
info.sessionID,
@@ -429,6 +444,10 @@ export function createEventHandler(deps: EventHandlerDeps) {
429444
} catch (error) {
430445
sessionLog(sessionId, "event session.compacted marker cleanup failed:", error);
431446
}
447+
// Compaction restructures messages (deletes/replaces some). Clear the
448+
// per-message token cache for the whole session so the next transform
449+
// pass recomputes against the new shape instead of serving stale counts.
450+
clearMessageTokensCache(sessionId);
432451
deps.onSessionCacheInvalidated?.(sessionId);
433452
return;
434453
}
@@ -453,6 +472,7 @@ export function createEventHandler(deps: EventHandlerDeps) {
453472
deps.contextUsageMap.delete(sessionId);
454473
deps.tagger.cleanup(sessionId);
455474
clearCompressorCooldown(sessionId);
475+
clearMessageTokensCache(sessionId);
456476
return;
457477
}
458478
};

packages/plugin/src/hooks/magic-context/event-payloads.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ export interface MessageUpdatedAssistantInfo {
2323
role: "assistant";
2424
finish?: string;
2525
sessionID: string;
26+
/** OpenCode assistant message id. Undefined only when the event payload
27+
* doesn't include one (older SDK versions or malformed events). */
28+
messageID?: string;
2629
providerID?: string;
2730
modelID?: string;
2831
tokens?: {
@@ -92,6 +95,7 @@ export function getMessageUpdatedAssistantInfo(
9295
role: "assistant",
9396
finish: typeof info.finish === "string" ? info.finish : undefined,
9497
sessionID: info.sessionID,
98+
messageID: typeof info.id === "string" ? info.id : undefined,
9599
providerID: typeof info.providerID === "string" ? info.providerID : undefined,
96100
modelID: typeof info.modelID === "string" ? info.modelID : undefined,
97101
tokens: {

packages/plugin/src/hooks/magic-context/image-token-estimate.ts

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -133,9 +133,11 @@ function parseWebpDimensions(b: Uint8Array): { w: number; h: number } | null {
133133
if (b[8] !== 0x57 || b[9] !== 0x45 || b[10] !== 0x42 || b[11] !== 0x50) return null; // WEBP
134134
const variant = String.fromCharCode(b[12]!, b[13]!, b[14]!, b[15]!);
135135
if (variant === "VP8 ") {
136-
// Lossy: width/height at bytes 26-29 (14-bit each, little-endian)
137-
const w = ((b[26]! | (b[27]! << 8)) & 0x3fff) || 0;
138-
const h = ((b[28]! | (b[29]! << 8)) & 0x3fff) || 0;
136+
// Lossy: width/height at bytes 26-29 (14-bit each, little-endian).
137+
// The `& 0x3fff` already produces a non-negative 14-bit result; no
138+
// extra `|| 0` fallback is needed.
139+
const w = (b[26]! | (b[27]! << 8)) & 0x3fff;
140+
const h = (b[28]! | (b[29]! << 8)) & 0x3fff;
139141
if (w && h) return { w, h };
140142
} else if (variant === "VP8L") {
141143
// Lossless: 14-bit width/height starting byte 21
@@ -166,5 +168,11 @@ function parseGifDimensions(b: Uint8Array): { w: number; h: number } | null {
166168
}
167169

168170
function readUint32BE(b: Uint8Array, offset: number): number {
169-
return (b[offset]! << 24) | (b[offset + 1]! << 16) | (b[offset + 2]! << 8) | b[offset + 3]!;
171+
// `>>> 0` coerces to an unsigned 32-bit integer. Without it, a byte with
172+
// the MSB set produces a negative value (JS bitwise ops are 32-bit
173+
// signed), which would bypass downstream `< 1` guards and produce
174+
// wrong token counts for malformed/untrusted PNG headers.
175+
return (
176+
((b[offset]! << 24) | (b[offset + 1]! << 16) | (b[offset + 2]! << 8) | b[offset + 3]!) >>> 0
177+
);
170178
}

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

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { CATEGORY_PRIORITY } from "../../features/magic-context/memory/constants
99
import { getMemoriesByProject } from "../../features/magic-context/memory/storage-memory";
1010
import type { Memory, MemoryCategory } from "../../features/magic-context/memory/types";
1111
import { sessionLog } from "../../shared/logger";
12+
import { estimateTokens } from "./read-session-formatting";
1213
import type { MessageLike } from "./tag-messages";
1314

1415
export interface PreparedCompartmentInjection {
@@ -72,8 +73,6 @@ export function renderMemoryBlock(memories: Memory[]): string | null {
7273
return `<project-memory>\n${sections.join("\n")}\n</project-memory>`;
7374
}
7475

75-
const CHARS_PER_TOKEN_ESTIMATE = 4;
76-
7776
/** Constraint keywords that signal a memory encodes a rule rather than a description. */
7877
const CONSTRAINT_KEYWORDS = /\b(must|never|always|cannot|should not|must not)\b/i;
7978

@@ -101,7 +100,10 @@ function utilityTier(m: Memory): number {
101100
* 4. shorter content first (fit more memories in budget)
102101
* 5. deterministic id tiebreaker for cache stability
103102
*
104-
* Estimates ~4 chars per token for budget enforcement.
103+
* Uses the real Claude tokenizer (via estimateTokens) so the trim stays
104+
* consistent with the rest of the plugin's token math — mismatching units
105+
* (chars/4 here vs real tokens elsewhere) caused either under- or
106+
* over-injection of memories, depending on memory content shape.
105107
*/
106108
function trimMemoriesToBudget(
107109
sessionId: string,
@@ -129,8 +131,10 @@ function trimMemoriesToBudget(
129131
let usedTokens = 0;
130132

131133
for (const memory of sorted) {
132-
// Estimate: category tag overhead (~20 chars) + "- " prefix + content
133-
const memoryTokens = Math.ceil((memory.content.length + 22) / CHARS_PER_TOKEN_ESTIMATE);
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.
137+
const memoryTokens = estimateTokens(`- ${memory.content}`) + 6;
134138
if (usedTokens + memoryTokens > budgetTokens) {
135139
break;
136140
}

packages/plugin/src/hooks/magic-context/strip-content.test.ts

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -724,5 +724,177 @@ describe("strip-content", () => {
724724
expect(a2.parts).toHaveLength(1);
725725
});
726726
});
727+
728+
describe("#given a single assistant with reasoning NOT at content position 0", () => {
729+
it("#then strips the reasoning (would land at non-zero in merged block)", () => {
730+
const u = message("u", "user", [{ type: "text", text: "q" }]);
731+
const a1 = message("a1", "assistant", [
732+
{ type: "text", text: "t1" },
733+
{ type: "reasoning", text: "r1" },
734+
{ type: "text", text: "t2" },
735+
]);
736+
const messages = [u, a1];
737+
738+
const stripped = stripReasoningFromMergedAssistants(messages);
739+
740+
expect(stripped).toBe(1);
741+
expect(a1.parts.map((p) => (p as { type: string }).type)).toEqual(["text", "text"]);
742+
});
743+
});
744+
745+
describe("#given a single assistant with step-start before reasoning", () => {
746+
it("#then keeps the reasoning (step-start is metadata AI SDK ignores)", () => {
747+
const u = message("u", "user", [{ type: "text", text: "q" }]);
748+
const a1 = message("a1", "assistant", [
749+
{ type: "step-start" },
750+
{ type: "reasoning", text: "r1" },
751+
{ type: "text", text: "t1" },
752+
]);
753+
const messages = [u, a1];
754+
755+
const stripped = stripReasoningFromMergedAssistants(messages);
756+
757+
expect(stripped).toBe(0);
758+
expect(a1.parts).toHaveLength(3);
759+
});
760+
});
761+
762+
describe("#given a single assistant with many interleaved reasoning parts", () => {
763+
it("#then keeps only the first reasoning and strips the rest", () => {
764+
// Mirrors the worst-case observed in opus-4.7 output: one OpenCode
765+
// message with many reasoning parts interleaved with text/tool.
766+
const u = message("u", "user", [{ type: "text", text: "go" }]);
767+
const a1 = message("a1", "assistant", [
768+
{ type: "step-start" },
769+
{ type: "reasoning", text: "r1" },
770+
{ type: "text", text: "t1" },
771+
{ type: "reasoning", text: "r2" },
772+
{ type: "reasoning", text: "r3" },
773+
{ type: "reasoning", text: "r4" },
774+
{ type: "reasoning", text: "r5" },
775+
{ type: "tool", state: { status: "completed" } },
776+
{ type: "step-finish" },
777+
]);
778+
const messages = [u, a1];
779+
780+
const stripped = stripReasoningFromMergedAssistants(messages);
781+
782+
expect(stripped).toBe(4);
783+
expect(a1.parts.map((p) => (p as { type: string }).type)).toEqual([
784+
"step-start",
785+
"reasoning",
786+
"text",
787+
"tool",
788+
"step-finish",
789+
]);
790+
});
791+
});
792+
793+
describe("#given first assistant has text before reasoning, second has reasoning at pos 0", () => {
794+
it("#then strips reasoning from BOTH (can't repair the run)", () => {
795+
// Because reasoning in a1 is NOT at content position 0, we strip
796+
// it. Subsequent assistants in the same run lose reasoning too,
797+
// since only one reasoning per run is allowed.
798+
const u = message("u", "user", [{ type: "text", text: "q" }]);
799+
const a1 = message("a1", "assistant", [
800+
{ type: "text", text: "t1" },
801+
{ type: "reasoning", text: "r1" },
802+
]);
803+
const a2 = message("a2", "assistant", [
804+
{ type: "reasoning", text: "r2" },
805+
{ type: "text", text: "t2" },
806+
]);
807+
const messages = [u, a1, a2];
808+
809+
const stripped = stripReasoningFromMergedAssistants(messages);
810+
811+
expect(stripped).toBe(2);
812+
expect(a1.parts.map((p) => (p as { type: string }).type)).toEqual(["text"]);
813+
expect(a2.parts.map((p) => (p as { type: string }).type)).toEqual(["text"]);
814+
});
815+
});
816+
817+
describe("#given two consecutive assistants each with 'thinking' (wire-format) parts", () => {
818+
it("#then keeps thinking on the first and strips from the second", () => {
819+
// opus-4.7 can produce wire-format "thinking" parts (not just
820+
// OpenCode's internal "reasoning"). The merge-workaround must
821+
// treat them the same, otherwise the merged Anthropic block
822+
// ends up with thinking interleaved — the exact 400 error this
823+
// function exists to prevent.
824+
const u = message("u", "user", [{ type: "text", text: "go" }]);
825+
const a1 = message("a1", "assistant", [
826+
{ type: "thinking", thinking: "t1-think", signature: "sig1" },
827+
{ type: "text", text: "r1" },
828+
]);
829+
const a2 = message("a2", "assistant", [
830+
{ type: "thinking", thinking: "t2-think", signature: "sig2" },
831+
{ type: "text", text: "r2" },
832+
]);
833+
const messages = [u, a1, a2];
834+
835+
const stripped = stripReasoningFromMergedAssistants(messages);
836+
837+
expect(stripped).toBe(1);
838+
expect(a1.parts).toHaveLength(2);
839+
expect((a1.parts[0] as { type: string }).type).toBe("thinking");
840+
expect(a2.parts).toHaveLength(1);
841+
expect((a2.parts[0] as { type: string }).type).toBe("text");
842+
});
843+
});
844+
845+
describe("#given mixed reasoning/thinking/redacted_thinking types across a run", () => {
846+
it("#then treats all three as reasoning-like (keep first, strip rest)", () => {
847+
const u = message("u", "user", [{ type: "text", text: "go" }]);
848+
const a1 = message("a1", "assistant", [
849+
{ type: "reasoning", text: "r1" },
850+
{ type: "text", text: "answer1" },
851+
]);
852+
const a2 = message("a2", "assistant", [
853+
{ type: "thinking", thinking: "t2", signature: "sig2" },
854+
{ type: "text", text: "answer2" },
855+
]);
856+
const a3 = message("a3", "assistant", [
857+
{ type: "redacted_thinking", data: "opaque3" },
858+
{ type: "text", text: "answer3" },
859+
]);
860+
const messages = [u, a1, a2, a3];
861+
862+
const stripped = stripReasoningFromMergedAssistants(messages);
863+
864+
// Keep a1.reasoning (first-in-run, position 0), strip a2.thinking
865+
// and a3.redacted_thinking.
866+
expect(stripped).toBe(2);
867+
expect(a1.parts.map((p) => (p as { type: string }).type)).toEqual([
868+
"reasoning",
869+
"text",
870+
]);
871+
expect(a2.parts.map((p) => (p as { type: string }).type)).toEqual(["text"]);
872+
expect(a3.parts.map((p) => (p as { type: string }).type)).toEqual(["text"]);
873+
});
874+
});
875+
876+
describe("#given first assistant has text before a thinking-typed block", () => {
877+
it("#then strips the thinking block from first AND second assistant", () => {
878+
// If thinking is NOT at content position 0 in the first
879+
// assistant, no thinking can land at position 0 of the merged
880+
// block — so strip from every assistant in the run.
881+
const u = message("u", "user", [{ type: "text", text: "q" }]);
882+
const a1 = message("a1", "assistant", [
883+
{ type: "text", text: "prelude" },
884+
{ type: "thinking", thinking: "t1", signature: "sig1" },
885+
]);
886+
const a2 = message("a2", "assistant", [
887+
{ type: "thinking", thinking: "t2", signature: "sig2" },
888+
{ type: "text", text: "answer" },
889+
]);
890+
const messages = [u, a1, a2];
891+
892+
const stripped = stripReasoningFromMergedAssistants(messages);
893+
894+
expect(stripped).toBe(2);
895+
expect(a1.parts.map((p) => (p as { type: string }).type)).toEqual(["text"]);
896+
expect(a2.parts.map((p) => (p as { type: string }).type)).toEqual(["text"]);
897+
});
898+
});
727899
});
728900
});

0 commit comments

Comments
 (0)