Skip to content

Commit 2e813dd

Browse files
ualtinokualtinok
committed
fix: apply council audit fixes
- fix(compressor): register background compressor in activeRuns so historian cannot race against in-flight replaceAllCompartmentState writes - fix(recomp): clear compression depth on full recomp promotion so rebuilt compartments start at depth 0, matching partial recomp behavior - fix(config): redact secret values in Zod validation warnings — post-substitution {env:VAR} values no longer leak through config error messages - fix(compressor): stop skipping the boundary element when a singleton same-depth run ends — band selection now correctly anchors the next run at j, not j+1 - docs(temporal): document why temporal marker injection intentionally runs on every transform pass including defer passes
1 parent 99aa41e commit 2e813dd

10 files changed

Lines changed: 468 additions & 17 deletions

‎packages/plugin/src/config/index.test.ts‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
import { describe, expect, it } from "bun:test";
2+
import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
3+
import { tmpdir } from "node:os";
4+
import { join } from "node:path";
5+
6+
import { loadPluginConfig } from "./index";
7+
8+
/**
9+
* Writes a magic-context.jsonc file inside a fresh temp XDG_CONFIG_HOME tree
10+
* and runs loadPluginConfig against it. Returns warnings + parsed config.
11+
*
12+
* Scope directory is NOT set — we pass a unique directory that does not
13+
* contain a project config so only the user config is loaded.
14+
*/
15+
function loadWithUserConfig(configText: string, extraEnv: Record<string, string> = {}) {
16+
const xdg = mkdtempSync(join(tmpdir(), "mc-config-test-"));
17+
const configDir = join(xdg, "opencode");
18+
// biome-ignore lint/correctness/noNodejsModules: test helper
19+
const fs = require("node:fs") as typeof import("node:fs");
20+
fs.mkdirSync(configDir, { recursive: true });
21+
writeFileSync(join(configDir, "magic-context.jsonc"), configText, "utf-8");
22+
23+
const origXdg = process.env.XDG_CONFIG_HOME;
24+
const savedEnv: Record<string, string | undefined> = {};
25+
for (const [k, v] of Object.entries(extraEnv)) {
26+
savedEnv[k] = process.env[k];
27+
process.env[k] = v;
28+
}
29+
process.env.XDG_CONFIG_HOME = xdg;
30+
31+
// Use a directory that definitely has no project config so only the
32+
// user config feeds the loader. We use a sibling temp directory.
33+
const projectDir = mkdtempSync(join(tmpdir(), "mc-config-proj-"));
34+
try {
35+
return loadPluginConfig(projectDir);
36+
} finally {
37+
if (origXdg === undefined) {
38+
delete process.env.XDG_CONFIG_HOME;
39+
} else {
40+
process.env.XDG_CONFIG_HOME = origXdg;
41+
}
42+
for (const [k, v] of Object.entries(savedEnv)) {
43+
if (v === undefined) delete process.env[k];
44+
else process.env[k] = v;
45+
}
46+
rmSync(xdg, { recursive: true, force: true });
47+
rmSync(projectDir, { recursive: true, force: true });
48+
}
49+
}
50+
51+
describe("loadPluginConfig — secret redaction", () => {
52+
it("does NOT leak resolved env values through Zod validation warnings", () => {
53+
const secret = "sk-live-CARDINAL-SIN-IF-THIS-APPEARS-IN-LOGS";
54+
const config = JSON.stringify({
55+
// `historian_timeout_ms` has a minimum of 60_000. Feeding the
56+
// substituted secret string here causes Zod to reject the field
57+
// and route through the warning path we care about.
58+
historian_timeout_ms: "{env:MC_TEST_SECRET}",
59+
});
60+
61+
const result = loadWithUserConfig(config, { MC_TEST_SECRET: secret });
62+
const warnings = result.configWarnings ?? [];
63+
64+
// The plugin should still load (enabled: true kept by recovery path).
65+
expect(result.enabled).toBe(true);
66+
67+
// No warning or config field may contain the resolved secret.
68+
const allText = JSON.stringify({ config: result, warnings });
69+
expect(allText).not.toContain(secret);
70+
expect(allText).not.toContain("CARDINAL-SIN");
71+
72+
// But the warnings should still describe what failed. We expect a
73+
// warning mentioning historian_timeout_ms and the safe type summary.
74+
const relevantWarning = warnings.find((w) => w.includes("historian_timeout_ms"));
75+
expect(relevantWarning).toBeDefined();
76+
expect(relevantWarning).toContain("invalid value");
77+
// Must show type + length, not the value itself.
78+
expect(relevantWarning).toMatch(/string, \d+ chars?/);
79+
});
80+
81+
it("redacts long string values of any source (not just env-substituted)", () => {
82+
// Verifies the redaction applies to plain invalid values too — we
83+
// don't want to special-case env vs non-env because we can't tell
84+
// them apart at the Zod layer.
85+
const config = JSON.stringify({
86+
historian_timeout_ms: "super-secret-plain-literal-that-should-not-leak",
87+
});
88+
89+
const result = loadWithUserConfig(config);
90+
const warnings = result.configWarnings ?? [];
91+
const combined = warnings.join("\n");
92+
93+
expect(combined).not.toContain("super-secret-plain-literal-that-should-not-leak");
94+
expect(combined).toMatch(/string, \d+ chars?/);
95+
});
96+
97+
it("redacts nested object values to structural shape only", () => {
98+
const config = JSON.stringify({
99+
historian_timeout_ms: { nested: "secret-xyz", apiKey: "also-secret" },
100+
});
101+
102+
const result = loadWithUserConfig(config);
103+
const warnings = result.configWarnings ?? [];
104+
const combined = warnings.join("\n");
105+
106+
expect(combined).not.toContain("secret-xyz");
107+
expect(combined).not.toContain("also-secret");
108+
expect(combined).toContain("object with keys");
109+
expect(combined).toContain("nested");
110+
expect(combined).toContain("apiKey");
111+
});
112+
113+
it("still shows numeric and boolean invalid values (not secrets by nature)", () => {
114+
// Numbers/booleans in config fields are never secrets — they're
115+
// plain validation mistakes — so we surface them fully to help
116+
// the user diagnose.
117+
const config = JSON.stringify({
118+
execute_threshold_percentage: 5, // below min (20)
119+
});
120+
121+
const result = loadWithUserConfig(config);
122+
const warnings = result.configWarnings ?? [];
123+
const combined = warnings.join("\n");
124+
125+
expect(combined).toContain("execute_threshold_percentage");
126+
// `number 5` is the human-friendly safe render.
127+
expect(combined).toMatch(/number 5/);
128+
});
129+
});

‎packages/plugin/src/config/index.ts‎

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,31 @@ function mergeConfigs(
9999
return config;
100100
}
101101

102+
/**
103+
* Render a config value for a warning message in a way that never leaks resolved
104+
* secrets from `{env:API_KEY}` / `{file:...}` substitution.
105+
*
106+
* Strings, numbers, booleans, and nulls are shown as type-plus-length so the
107+
* user can still diagnose the problem ("string, 48 chars", "number 200001") but
108+
* never see the resolved content. Objects and arrays are shown as their
109+
* structural shape only. `undefined` / missing values are reported as
110+
* `<missing>`.
111+
*/
112+
function redactConfigValue(value: unknown): string {
113+
if (value === undefined) return "<missing>";
114+
if (value === null) return "null";
115+
if (typeof value === "string")
116+
return `string, ${value.length} char${value.length === 1 ? "" : "s"}`;
117+
if (typeof value === "number") return `number ${value}`;
118+
if (typeof value === "boolean") return `boolean ${value}`;
119+
if (Array.isArray(value)) return `array, ${value.length} item${value.length === 1 ? "" : "s"}`;
120+
if (typeof value === "object") {
121+
const keys = Object.keys(value as Record<string, unknown>);
122+
return `object with keys [${keys.join(", ")}]`;
123+
}
124+
return typeof value;
125+
}
126+
102127
function parsePluginConfig(
103128
rawConfig: Record<string, unknown>,
104129
): MagicContextPluginConfig & { configWarnings?: string[] } {
@@ -145,11 +170,14 @@ function parsePluginConfig(
145170
`"${key}": invalid agent configuration, ignoring. Check your magic-context.jsonc.`,
146171
);
147172
} else {
148-
// Use Zod default for this field
173+
// Use Zod default for this field.
174+
// Intentional: redactConfigValue reports type+length, never the
175+
// resolved value itself, because `{env:...}` / `{file:...}`
176+
// substitution may have already expanded secrets into rawConfig.
149177
delete patched[key];
150178
const defaultVal = (defaults as unknown as Record<string, unknown>)[key];
151179
warnings.push(
152-
`"${key}": invalid value ${JSON.stringify(rawConfig[key])}, using default ${JSON.stringify(defaultVal)}.`,
180+
`"${key}": invalid value (${redactConfigValue(rawConfig[key])}), using default ${JSON.stringify(defaultVal)}.`,
153181
);
154182
}
155183
}

‎packages/plugin/src/hooks/magic-context/compartment-runner-compressor.test.ts‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
/// <reference types="bun-types" />
2+
3+
import { describe, expect, it } from "bun:test";
4+
5+
import { findOldestContiguousSameDepthBand } from "./compartment-runner-compressor";
6+
7+
// Minimal shape for the test — the selector only reads averageDepth and index.
8+
// We use `as unknown as Parameters<typeof findOldestContiguousSameDepthBand>[0][number]`
9+
// to satisfy the ScoredCompartment interface without pulling in the full type.
10+
function scored(...depths: number[]) {
11+
return depths.map(
12+
(d, i) =>
13+
({
14+
compartment: { sequence: i } as unknown,
15+
index: i,
16+
tokenEstimate: 100,
17+
averageDepth: d,
18+
score: 1,
19+
}) as Parameters<typeof findOldestContiguousSameDepthBand>[0][number],
20+
);
21+
}
22+
23+
describe("findOldestContiguousSameDepthBand — band-boundary regression", () => {
24+
it("returns the oldest same-depth run ≥ 2 when it starts at the anchor", () => {
25+
const band = findOldestContiguousSameDepthBand(scored(0, 0, 1, 1), {
26+
maxPickable: 2,
27+
maxMergeDepth: 5,
28+
graceCompartments: 0,
29+
floorHeadroom: 10,
30+
});
31+
expect(band.map((b) => b.index)).toEqual([0, 1]);
32+
});
33+
34+
it("does NOT skip element j when the singleton-run at i ends because of a depth mismatch", () => {
35+
// This was the bug: with [0, 1, 1, 1], maxPickable=2, the old code did
36+
// `i = max(j+1, i+1)` when runLen<2 at i=0. That advanced i to 2
37+
// instead of 1, losing the chance to pick [scored[1], scored[2]] as
38+
// the first depth-1 band. The fix advances to `j` (which is 1 here)
39+
// so scored[1] can anchor the next run.
40+
const band = findOldestContiguousSameDepthBand(scored(0, 1, 1, 1), {
41+
maxPickable: 2,
42+
maxMergeDepth: 5,
43+
graceCompartments: 0,
44+
floorHeadroom: 10,
45+
});
46+
// Must pick the band starting at index 1, not skip to index 2.
47+
expect(band.map((b) => b.index)).toEqual([1, 2]);
48+
});
49+
50+
it("skips past an always-max-depth element without stalling", () => {
51+
// [d=0, d=max, d=1, d=1] — the middle element is skipped because it's
52+
// at max depth. After that, scored[2] and scored[3] form a band.
53+
// Progress guarantee: the outer loop uses `continue` on max-depth and
54+
// advances i by 1 via `i++`, not via `i = max(j, i+1)`. So this test
55+
// verifies the skip branch stays separate and functional.
56+
const band = findOldestContiguousSameDepthBand(scored(0, 5, 1, 1), {
57+
maxPickable: 2,
58+
maxMergeDepth: 5,
59+
graceCompartments: 0,
60+
floorHeadroom: 10,
61+
});
62+
expect(band.map((b) => b.index)).toEqual([2, 3]);
63+
});
64+
65+
it("honors the grace period and never considers the newest compartments", () => {
66+
// [0, 0, 1, 1], graceCompartments=2 → scope shrinks to [0, 0]; depth-0
67+
// band there is valid.
68+
const band = findOldestContiguousSameDepthBand(scored(0, 0, 1, 1), {
69+
maxPickable: 2,
70+
maxMergeDepth: 5,
71+
graceCompartments: 2,
72+
floorHeadroom: 10,
73+
});
74+
expect(band.map((b) => b.index)).toEqual([0, 1]);
75+
76+
// Pull grace too tight → no band possible.
77+
const empty = findOldestContiguousSameDepthBand(scored(0, 0, 1, 1), {
78+
maxPickable: 2,
79+
maxMergeDepth: 5,
80+
graceCompartments: 3,
81+
floorHeadroom: 10,
82+
});
83+
expect(empty).toEqual([]);
84+
});
85+
86+
it("caps the band at maxPickable regardless of longer runs", () => {
87+
const band = findOldestContiguousSameDepthBand(scored(0, 0, 0, 0), {
88+
maxPickable: 3,
89+
maxMergeDepth: 5,
90+
graceCompartments: 0,
91+
floorHeadroom: 10,
92+
});
93+
expect(band.map((b) => b.index)).toEqual([0, 1, 2]);
94+
});
95+
96+
it("caps the band at floorHeadroom when it is tighter than maxPickable", () => {
97+
// floorHeadroom < maxPickable — floor wins.
98+
const band = findOldestContiguousSameDepthBand(scored(0, 0, 0, 0), {
99+
maxPickable: 4,
100+
maxMergeDepth: 5,
101+
graceCompartments: 0,
102+
floorHeadroom: 2,
103+
});
104+
expect(band.map((b) => b.index)).toEqual([0, 1]);
105+
});
106+
107+
it("returns [] when no same-depth run ≥ 2 exists in scope", () => {
108+
// Every compartment has a distinct rounded depth.
109+
const band = findOldestContiguousSameDepthBand(scored(0, 1, 2, 3), {
110+
maxPickable: 2,
111+
maxMergeDepth: 5,
112+
graceCompartments: 0,
113+
floorHeadroom: 10,
114+
});
115+
expect(band).toEqual([]);
116+
});
117+
118+
it("returns [] when hardMaxPick is below 2", () => {
119+
expect(
120+
findOldestContiguousSameDepthBand(scored(0, 0, 0), {
121+
maxPickable: 1,
122+
maxMergeDepth: 5,
123+
graceCompartments: 0,
124+
floorHeadroom: 10,
125+
}),
126+
).toEqual([]);
127+
128+
expect(
129+
findOldestContiguousSameDepthBand(scored(0, 0, 0), {
130+
maxPickable: 5,
131+
maxMergeDepth: 5,
132+
graceCompartments: 0,
133+
floorHeadroom: 1,
134+
}),
135+
).toEqual([]);
136+
});
137+
});

‎packages/plugin/src/hooks/magic-context/compartment-runner-compressor.ts‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -298,7 +298,7 @@ interface SelectionConstraints {
298298
* merge reduces count by (input - output), so limiting picks to floorHeadroom
299299
* guarantees we can't fall below floor even in the worst case (output = 1).
300300
*/
301-
function findOldestContiguousSameDepthBand(
301+
export function findOldestContiguousSameDepthBand(
302302
scored: ScoredCompartment[],
303303
constraints: SelectionConstraints,
304304
): ScoredCompartment[] {
@@ -332,8 +332,17 @@ function findOldestContiguousSameDepthBand(
332332
if (runLen >= 2) {
333333
return scored.slice(i, j);
334334
}
335-
// No viable run starting at i. Skip past the broken element to find the next candidate.
336-
i = Math.max(j + 1, i + 1);
335+
// No viable run starting at i. Resume scanning AT j, not past it — j is
336+
// the boundary where the inner loop broke, which means scored[j] has a
337+
// different depth from scored[i] but may itself anchor a new run with
338+
// scored[j+1], scored[j+2], etc. Jumping to j+1 would skip scored[j]
339+
// and miss the band [scored[j], scored[j+1], ...].
340+
//
341+
// Progress is still guaranteed: `i+1` ensures we never stall on the
342+
// same index when the inner loop didn't advance (e.g. c was at max
343+
// depth and got `continue`-d above, or hardMaxPick=2 stops at j=i+1
344+
// and we need to move to j — which is i+1 — anyway).
345+
i = Math.max(j, i + 1);
337346
}
338347

339348
return [];

‎packages/plugin/src/hooks/magic-context/compartment-runner-recomp.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
replaceAllCompartmentState,
66
saveRecompStagingPass,
77
} from "../../features/magic-context/compartment-storage";
8+
import { clearCompressionDepth } from "../../features/magic-context/compression-depth-storage";
89
import { promoteSessionFactsToMemory } from "../../features/magic-context/memory";
910
import { resolveProjectIdentity } from "../../features/magic-context/memory/project-identity";
1011
import { getMemoriesByProject } from "../../features/magic-context/memory/storage-memory";
@@ -103,6 +104,13 @@ export async function executeContextRecompInternal(deps: CompartmentRunnerDeps):
103104
const promoted = promoteRecompStaging(db, sessionId);
104105
if (!promoted) return null;
105106

107+
// Full recomp rebuilds every compartment from message 1 onward, so
108+
// all pre-existing compression-depth rows are stale — the compressor
109+
// would otherwise skip or wrongly tier the fresh compartments. Wipe
110+
// per-session depth state so the rebuilt compartments start at depth
111+
// 0, matching what partial recomp does for its rebuilt range.
112+
clearCompressionDepth(db, sessionId);
113+
106114
// Invalidate injection cache after recomp promotion
107115
clearInjectionCache(sessionId);
108116

@@ -284,6 +292,9 @@ export async function executeContextRecompInternal(deps: CompartmentRunnerDeps):
284292
replaceAllCompartmentState(db, sessionId, candidateCompartments, candidateFacts);
285293
clearRecompStaging(db, sessionId);
286294
}
295+
// Full recomp rebuilds every compartment, so all pre-existing depth
296+
// rows are stale. Matches partial recomp's behavior for rebuilt ranges.
297+
clearCompressionDepth(db, sessionId);
287298
// Invalidate injection cache after final recomp promotion
288299
clearInjectionCache(sessionId);
289300

0 commit comments

Comments
 (0)