Skip to content

Commit 84da8e3

Browse files
anandgupta42claude
andcommitted
test: add v0.8.8 adversarial suite; fix ineffective MCP config parse-guard
Adds packages/opencode/test/skill/release-v0.8.8-adversarial.test.ts covering the shipping code + Step-5 review fixes under hostile input: - #937 QuestionTool non-interactive: blank/garbage/injection-shaped ALTIMATE_AUTO_ANSWER, reserved first/last with empty options, oversized questions — never throws, never invents an answer. - #893 addMcpToConfig parse-guard: corrupt config refused + left unchanged; nonexistent created; valid JSONC tolerated. - #940 startup upgrade check: throwing/rejecting/non-Error deps all swallowed (serve can't be taken down); jittered delay stays in [base, base*6). The adversarial suite caught that the parse-guard added in the prior commit was INEFFECTIVE: jsonc-parser's parseTree() is error-tolerant and returns a partial tree for truncated JSON, so `!parseTree(text)` never fired. Switched the guard to parse() with a ParseError sink (allowTrailingComma) which correctly detects malformed/truncated JSONC while still tolerating comments and trailing commas. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 3443829 commit 84da8e3

3 files changed

Lines changed: 272 additions & 17 deletions

File tree

packages/opencode/src/mcp/config.ts

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import path from "path"
2-
import { modify, applyEdits, parseTree, findNodeAtLocation, getNodeValue } from "jsonc-parser"
2+
import { modify, applyEdits, parse, parseTree, findNodeAtLocation, getNodeValue, type ParseError } from "jsonc-parser"
33
import { Filesystem } from "../util/filesystem"
44
import type { Config } from "../config/config"
55

@@ -39,11 +39,16 @@ export async function addMcpToConfig(name: string, mcpConfig: Config.Mcp, config
3939
}
4040

4141
// Guard: refuse to overwrite a config whose JSON/JSONC we cannot parse.
42-
// jsonc-parser's modify() is error-tolerant and would best-effort clobber a
43-
// recoverable file; the read helpers (removeMcpFromConfig/listMcpInConfig)
44-
// already bail on a parse failure, so mirror that on the write path.
45-
if (text.trim() && !parseTree(text)) {
46-
throw new Error(`Refusing to write MCP config: ${configPath} is not valid JSON/JSONC`)
42+
// jsonc-parser's modify() (and parseTree()) are error-tolerant and would
43+
// best-effort clobber a recoverable file, so use parse() with an error sink —
44+
// comments and trailing commas are allowed (it is JSONC), but a genuinely
45+
// malformed/truncated file produces errors and we bail instead of overwriting.
46+
if (text.trim()) {
47+
const parseErrors: ParseError[] = []
48+
parse(text, parseErrors, { allowTrailingComma: true })
49+
if (parseErrors.length > 0) {
50+
throw new Error(`Refusing to write MCP config: ${configPath} is not valid JSON/JSONC`)
51+
}
4752
}
4853

4954
const edits = modify(text, ["mcp", name], mcpConfig, {

packages/opencode/test/release-validation/mcp-datamate-893.test.ts

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -81,24 +81,24 @@ describe("PR893: addMcpToConfig writes updatedAt and round-trips through strict
8181

8282
// ── Gap 2: malformed JSONC does not silently lose recoverable config ─────────
8383
describe("PR893: addMcpToConfig on a malformed JSONC file — clobbering contract", () => {
84-
test("broken (partial-tree) JSON: existing key text is preserved and new entry added", async () => {
84+
test("broken (truncated) JSON: addMcpToConfig refuses and leaves the file unchanged", async () => {
8585
await using tmp = await tmpdir()
8686
const configPath = path.join(tmp.path, "altimate-code.json")
87-
// Truncated/broken JSON. jsonc-parser still produces a partial tree for this
88-
// (parseTree returns a truthy node), so addMcpToConfig does NOT bail.
87+
// Truncated/broken JSON. parseTree() is error-tolerant (returns a partial
88+
// node), but the v0.8.8 parse-guard uses parse() with an error sink, so a
89+
// genuinely malformed file is REFUSED rather than best-effort clobbered.
8990
const brokenText = `{ "mcp": { "a": `
9091
await writeFile(configPath, brokenText)
9192

92-
await addMcpToConfig("b", { type: "remote", url: "http://y" } as any, configPath)
93+
await expect(addMcpToConfig("b", { type: "remote", url: "http://y" } as any, configPath)).rejects.toThrow(
94+
/not valid JSON/i,
95+
)
9396

97+
// CONTRACT (v0.8.8): no data loss — the original file is left byte-for-byte
98+
// unchanged, and the new entry was NOT half-written into an unparseable file.
9499
const after = await readFile(configPath, "utf-8")
95-
// CONTRACT (current, conscious): the original entry "a" is NOT silently
96-
// dropped — its key text survives in the file — and the new entry "b" is
97-
// appended. The file is left unparseable because the source was unparseable;
98-
// addMcpToConfig is a text-edit, not a sanitizer.
99-
expect(after).toContain('"a"')
100-
expect(after).toContain('"b"')
101-
expect(after).toContain("http://y")
100+
expect(after).toBe(brokenText)
101+
expect(after).not.toContain("http://y")
102102
})
103103

104104
test("asymmetry: list/remove bail when parseTree returns undefined (severe garbage)", async () => {
Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,250 @@
1+
/**
2+
* Adversarial tests for v0.8.8 — the 8 PRs since v0.8.7 and the pre-release
3+
* review fixes that ship in the same tag.
4+
*
5+
* Focus: hostile / malformed inputs against the FINAL shipping code, including
6+
* the Step-5 review fixes:
7+
* - #937 QuestionTool non-interactive: blank/garbage ALTIMATE_AUTO_ANSWER,
8+
* reserved first/last keywords, empty option lists, injection-shaped labels,
9+
* and oversized question text must never throw and must never invent an answer.
10+
* - #893 addMcpToConfig parse-guard: a corrupt config file must be REFUSED
11+
* (thrown), never best-effort clobbered, and a valid/JSONC file must still write.
12+
* - #940 startup upgrade check: hostile/throwing/hanging deps must never reject
13+
* (serve cannot be taken down), and the jittered delay stays in its window.
14+
*
15+
* Coverage for the other shipping changes lives elsewhere and is not duplicated:
16+
* - #941 transcript endpoint coercion + sessionID — test/release-validation/session-transcript-941*.test.ts
17+
* - #933 dbt error bubbling / stripAnsi — packages/dbt-tools/test/dbt-cli*.test.ts
18+
* - #929 trace-dir logging — test/skill/release-v0.8.6-adversarial.test.ts (TraceConsumer)
19+
* - #844 chunk timeout — test/release-validation/chunk-timeout-844.test.ts
20+
*
21+
* Determinism: no timers waited on, no network, no shared state between tests
22+
* (env + globals saved/restored per test). No mock.module().
23+
*/
24+
25+
import { describe, expect, test, beforeEach, afterEach } from "bun:test"
26+
import fs from "fs/promises"
27+
import path from "path"
28+
import os from "os"
29+
import { QuestionTool } from "../../src/tool/question"
30+
import { SessionID, MessageID } from "../../src/session/schema"
31+
import { addMcpToConfig } from "../../src/mcp/config"
32+
import { parse as parseJsonc } from "jsonc-parser"
33+
import type { Config } from "../../src/config/config"
34+
import {
35+
runStartupUpgradeCheck,
36+
scheduleStartupUpgradeCheck,
37+
STARTUP_UPGRADE_DELAY_MS,
38+
type StartupUpgradeDeps,
39+
} from "../../src/cli/cmd/serve-upgrade-check"
40+
41+
const ctx = {
42+
sessionID: SessionID.make("ses_adv-0_8_8"),
43+
messageID: MessageID.make("adv-message"),
44+
callID: "adv-call",
45+
agent: "adv-agent",
46+
abort: AbortSignal.any([]),
47+
messages: [],
48+
metadata: () => {},
49+
ask: async () => {},
50+
} as any
51+
52+
// ---------------------------------------------------------------------------
53+
// #937 — QuestionTool non-interactive auto-resolution (hostile env)
54+
// ---------------------------------------------------------------------------
55+
describe("v0.8.8 #937: question tool non-interactive hostile inputs", () => {
56+
const ENV_KEYS = ["ALTIMATE_NON_INTERACTIVE", "ALTIMATE_FORCE_INTERACTIVE", "ALTIMATE_AUTO_ANSWER"]
57+
let saved: Record<string, string | undefined>
58+
59+
beforeEach(() => {
60+
saved = Object.fromEntries(ENV_KEYS.map((k) => [k, process.env[k]]))
61+
for (const k of ENV_KEYS) delete process.env[k]
62+
// Force the non-interactive branch so execute() never blocks on Question.ask().
63+
process.env["ALTIMATE_NON_INTERACTIVE"] = "1"
64+
})
65+
afterEach(() => {
66+
for (const k of ENV_KEYS) {
67+
if (saved[k] === undefined) delete process.env[k]
68+
else process.env[k] = saved[k]
69+
}
70+
})
71+
72+
const q = (question: string, options: { label: string; description: string }[]) => ({
73+
question,
74+
header: "h",
75+
options,
76+
})
77+
78+
test("no AUTO_ANSWER → every question Unanswered, no invented answer, never throws", async () => {
79+
const tool = await QuestionTool.init()
80+
const result = await tool.execute(
81+
{ questions: [q("Pick one?", [{ label: "Snowflake", description: "a" }, { label: "BigQuery", description: "b" }])] },
82+
ctx,
83+
)
84+
expect(result.output).toContain("non-interactive")
85+
expect(result.metadata.answers).toEqual([[]])
86+
})
87+
88+
test("garbage/injection-shaped AUTO_ANSWER that matches no label → Unanswered (no crash, no leak)", async () => {
89+
process.env["ALTIMATE_AUTO_ANSWER"] = "'; DROP TABLE options; --"
90+
const tool = await QuestionTool.init()
91+
const result = await tool.execute(
92+
{ questions: [q("Pick?", [{ label: "yes", description: "a" }, { label: "no", description: "b" }])] },
93+
ctx,
94+
)
95+
expect(result.metadata.answers).toEqual([[]])
96+
// The hostile env value must not be echoed back as if it were an answer.
97+
expect(result.output).not.toContain("DROP TABLE")
98+
})
99+
100+
test("AUTO_ANSWER=first/last with an EMPTY options list → Unanswered, no out-of-bounds crash", async () => {
101+
for (const mode of ["first", "last"]) {
102+
process.env["ALTIMATE_AUTO_ANSWER"] = mode
103+
const tool = await QuestionTool.init()
104+
const result = await tool.execute({ questions: [q("Empty?", [])] }, ctx)
105+
expect(result.metadata.answers).toEqual([[]])
106+
}
107+
})
108+
109+
test("AUTO_ANSWER label match is case-insensitive and selects exactly that option", async () => {
110+
process.env["ALTIMATE_AUTO_ANSWER"] = "snowflake"
111+
const tool = await QuestionTool.init()
112+
const result = await tool.execute(
113+
{ questions: [q("WH?", [{ label: "Snowflake", description: "a" }, { label: "BigQuery", description: "b" }])] },
114+
ctx,
115+
)
116+
expect(result.metadata.answers).toEqual([["Snowflake"]])
117+
})
118+
119+
test("oversized question text + many questions is formatted without throwing", async () => {
120+
const huge = "x".repeat(50_000)
121+
const tool = await QuestionTool.init()
122+
const questions = Array.from({ length: 20 }, (_, i) =>
123+
q(`${huge}-${i}?`, [{ label: `opt${i}`, description: "d" }]),
124+
)
125+
const result = await tool.execute({ questions }, ctx)
126+
expect(result.metadata.answers.length).toBe(20)
127+
expect(result.title).toContain("20 question")
128+
})
129+
})
130+
131+
// ---------------------------------------------------------------------------
132+
// #893 — addMcpToConfig must refuse to clobber an unparseable config
133+
// ---------------------------------------------------------------------------
134+
describe("v0.8.8 #893: addMcpToConfig parse-guard", () => {
135+
let dir: string
136+
const remote: Config.Mcp = { type: "remote", url: "https://example.test/mcp", enabled: true } as Config.Mcp
137+
138+
beforeEach(async () => {
139+
dir = path.join(os.tmpdir(), `mcpcfg-adv-${Date.now()}-${Math.random().toString(36).slice(2)}`)
140+
await fs.mkdir(dir, { recursive: true })
141+
})
142+
afterEach(async () => {
143+
await fs.rm(dir, { recursive: true, force: true }).catch(() => {})
144+
})
145+
146+
test("corrupt JSON file is REFUSED (throws) and left byte-for-byte unchanged", async () => {
147+
const cfg = path.join(dir, "altimate-code.json")
148+
const corrupt = '{ "mcp": { "a": '
149+
await fs.writeFile(cfg, corrupt)
150+
await expect(addMcpToConfig("b", remote, cfg)).rejects.toThrow(/not valid JSON/i)
151+
expect(await fs.readFile(cfg, "utf-8")).toBe(corrupt)
152+
})
153+
154+
test("nonexistent config is created and contains the new server", async () => {
155+
const cfg = path.join(dir, "altimate-code.json")
156+
await addMcpToConfig("svc", remote, cfg)
157+
const parsed = JSON.parse(await fs.readFile(cfg, "utf-8"))
158+
expect(parsed.mcp.svc.url).toBe("https://example.test/mcp")
159+
})
160+
161+
test("valid JSONC with comments/trailing commas is tolerated (modify still writes)", async () => {
162+
const cfg = path.join(dir, "altimate-code.json")
163+
const original = '{\n // existing\n "mcp": { "old": { "type": "remote", "url": "https://x.test" } },\n}'
164+
await fs.writeFile(cfg, original)
165+
await addMcpToConfig("new", remote, cfg)
166+
const text = await fs.readFile(cfg, "utf-8")
167+
// The file is still JSONC (comment preserved) — parse with the JSONC parser,
168+
// not JSON.parse, and confirm both the new and existing servers are present.
169+
const parsed = parseJsonc(text) as { mcp: Record<string, { url: string }> }
170+
expect(parsed.mcp.new.url).toBe("https://example.test/mcp")
171+
expect(parsed.mcp.old.url).toBe("https://x.test")
172+
expect(text).toContain("// existing")
173+
})
174+
})
175+
176+
// ---------------------------------------------------------------------------
177+
// #940 — startup upgrade check is fail-safe (cannot take serve down)
178+
// ---------------------------------------------------------------------------
179+
describe("v0.8.8 #940: startup upgrade check fail-safety + jitter", () => {
180+
test("a synchronously-throwing run() resolves (never rejects)", async () => {
181+
const deps: StartupUpgradeDeps = {
182+
provide: (_dir, fn) => fn(),
183+
run: () => {
184+
throw new Error("sync boom")
185+
},
186+
}
187+
await expect(runStartupUpgradeCheck(deps)).resolves.toBeUndefined()
188+
})
189+
190+
test("a rejecting run() resolves (never rejects)", async () => {
191+
const deps: StartupUpgradeDeps = {
192+
provide: (_dir, fn) => fn(),
193+
run: () => Promise.reject(new Error("async boom")),
194+
}
195+
await expect(runStartupUpgradeCheck(deps)).resolves.toBeUndefined()
196+
})
197+
198+
test("a provide() that rejects (bootstrap failure) resolves and never runs the upgrade", async () => {
199+
let ran = false
200+
const deps: StartupUpgradeDeps = {
201+
provide: () => Promise.reject(new Error("instance boom")),
202+
run: async () => {
203+
ran = true
204+
},
205+
}
206+
await expect(runStartupUpgradeCheck(deps)).resolves.toBeUndefined()
207+
expect(ran).toBe(false)
208+
})
209+
210+
test("a non-Error thrown value (string) is still swallowed", async () => {
211+
const deps: StartupUpgradeDeps = {
212+
provide: (_dir, fn) => fn(),
213+
run: () => {
214+
// eslint-disable-next-line no-throw-literal
215+
throw "stringly-typed failure"
216+
},
217+
}
218+
await expect(runStartupUpgradeCheck(deps)).resolves.toBeUndefined()
219+
})
220+
221+
test("scheduleStartupUpgradeCheck jitters within [base, base*6), unrefs, returns void", () => {
222+
const original = globalThis.setTimeout
223+
const calls: Array<{ delay: number | undefined }> = []
224+
let unrefCount = 0
225+
try {
226+
// Sample repeatedly so the random jitter window is actually exercised.
227+
;(globalThis as any).setTimeout = (_cb: () => void, delay?: number) => {
228+
calls.push({ delay })
229+
return {
230+
unref() {
231+
unrefCount++
232+
return this
233+
},
234+
}
235+
}
236+
for (let i = 0; i < 50; i++) {
237+
const ret = scheduleStartupUpgradeCheck()
238+
expect(ret).toBeUndefined()
239+
}
240+
} finally {
241+
;(globalThis as any).setTimeout = original
242+
}
243+
expect(calls.length).toBe(50)
244+
expect(unrefCount).toBe(50)
245+
for (const c of calls) {
246+
expect(c.delay).toBeGreaterThanOrEqual(STARTUP_UPGRADE_DELAY_MS)
247+
expect(c.delay).toBeLessThan(STARTUP_UPGRADE_DELAY_MS * 6)
248+
}
249+
})
250+
})

0 commit comments

Comments
 (0)