diff --git a/docs-site/src/content/docs/guides/integrations.md b/docs-site/src/content/docs/guides/integrations.md index 05d371bc2..afeba687a 100644 --- a/docs-site/src/content/docs/guides/integrations.md +++ b/docs-site/src/content/docs/guides/integrations.md @@ -65,8 +65,21 @@ always recoverable: and their history rows read **Backup expired**. Disable removes only the entries opencodex recorded as its own. If your file changed -after we wrote it, the switch locks and disable refuses rather than guessing which -edits were yours. +after we wrote it, what happens depends on whether our own entries are still intact +and on the file's format. For strict-JSON configs (OpenCode, Pi), an edit **next to** +our block — adding an MCP server, a provider of your own — shows as **Update needed**: +refreshing merges around your entries and keeps them, though formatting may be +normalized. The exception is something JSON cannot rewrite exactly — a non-finite +number like `1e999`, a number a rewrite would round (a very large integer, or one +so small it collapses to zero), `-0`, the same key written twice in one object, or nesting deeper +than 1000 levels — which locks the switch instead, so nothing is silently changed or dropped. +**OMP** is unaffected by sibling edits too, for a different reason: its writer +patches only its own `providers.opencodex` range byte-wise, so the rest of the +file is never rewritten. For the remaining formats that can carry comments +(Hermes, OpenClaw, Kimi Code, Gajae Code — YAML, JSON5 and TOML written as whole +documents), or +whenever our own entries were edited, the switch locks and disable refuses rather +than guessing which edits were yours. ## What to expect, honestly diff --git a/docs-site/src/content/docs/tr/guides/integrations.md b/docs-site/src/content/docs/tr/guides/integrations.md index 21d3d2037..57d6d6b40 100644 --- a/docs-site/src/content/docs/tr/guides/integrations.md +++ b/docs-site/src/content/docs/tr/guides/integrations.md @@ -77,9 +77,24 @@ sahip olduğunuz durum her zaman kurtarılabilir: expired)** yazar. Devre dışı bırakma, yalnızca opencodex'in kendisine ait olarak kaydettiği -girdileri kaldırır. Dosyanız biz yazdıktan sonra değiştiyse, anahtar kilitlenir -ve hangi düzenlemelerin size ait olduğunu tahmin etmek yerine devre dışı -bırakmayı reddeder. +girdileri kaldırır. Dosyanız biz yazdıktan sonra değiştiyse, ne olacağı kendi +girdilerimizin hâlâ bozulmamış olup olmadığına ve dosyanın biçimine bağlıdır. +Katı JSON yapılandırmalarında (OpenCode, Pi), bloğumuzun **yanında** yapılan bir +düzenleme — bir MCP sunucusu eklemek, kendinize ait bir sağlayıcı tanımlamak — +**Güncelleme gerekli (Update needed)** olarak görünür: yenileme, girdilerinizin +etrafında birleştirir ve onları korur; yalnızca biçimlendirme +normalleştirilebilir. İstisna, JSON'un birebir yeniden yazamayacağı şeylerdir — +`1e999` gibi sonlu olmayan bir sayı, yeniden yazımın yuvarlayacağı bir sayı (çok +büyük bir tam sayı ya da sıfıra çökecek kadar küçük bir sayı), `-0`, aynı +nesnede iki kez yazılmış bir anahtar veya 1000 seviyeden derin iç içe geçme — +bu durumda anahtar kilitlenir, böylece +hiçbir şey sessizce değiştirilmez veya düşürülmez. **OMP** de yanındaki +düzenlemelerden etkilenmez, ama başka bir nedenle: writer'ı yalnızca kendi +`providers.opencodex` aralığını bayt bayt yamalar, dosyanın geri kalanı hiçbir +zaman yeniden yazılmaz. Yorum taşıyabilen diğer biçimlerde (Hermes, OpenClaw, +Kimi Code, Gajae Code — bütün belge olarak yazılan YAML, JSON5 ve TOML) veya +kendi girdilerimiz düzenlenmişse, anahtar kilitlenir ve hangi düzenlemelerin +size ait olduğunu tahmin etmek yerine devre dışı bırakmayı reddeder. ## Dürüstçe ne beklenmeli? diff --git a/docs-site/src/content/docs/zh-tw/guides/integrations.md b/docs-site/src/content/docs/zh-tw/guides/integrations.md index d9ed0614a..19057a0e6 100644 --- a/docs-site/src/content/docs/zh-tw/guides/integrations.md +++ b/docs-site/src/content/docs/zh-tw/guides/integrations.md @@ -37,7 +37,7 @@ opencodex 從自己的環境讀取這些變數。如果你的 gateway 以 profil - **Restore this point…** 會出現在較舊的操作上，或當檔案在那次操作之後有變更時。跨過這樣的變更做回復會再詢問一次，才覆蓋你的較新編輯——並且也會備份它們，所以那次的回復本身也可以復原。 - 每個客戶端保留十份備份。超過之後，最舊的快照檔案會被移除，其歷史列顯示為 **Backup expired**。 -停用只移除 opencodex 記錄為自己寫入的條目。如果你的檔案在我們寫入之後有變更，開關會鎖定，停用會拒絕執行，而不是猜測哪些編輯是你的。 +停用只移除 opencodex 記錄為自己寫入的條目。如果你的檔案在我們寫入之後有變更，後續行為取決於我們自己的條目是否完好，以及檔案的格式。對於嚴格 JSON 設定檔（OpenCode、Pi），在我們的區塊**旁邊**進行的編輯——例如新增 MCP 伺服器或你自己的 provider——會顯示為**需要更新**：重新整理會在保留你的條目的前提下合併寫入，但格式可能會被正規化。例外情況是 JSON 無法精確重寫的內容——例如 `1e999` 這類非有限數字、重寫會被四捨五入的數字（極大的整數，或小到會塌縮成零的數字）、`-0`、同一個物件裡重複出現的鍵，或巢狀層數超過 1000 層——此時開關會鎖定，確保沒有任何值被悄悄改動或刪除。**OMP** 同樣不受旁邊編輯影響，但原因不同：它的 writer 只逐位元組修補自己的 `providers.opencodex` 範圍，檔案其餘部分從不會被重寫。至於其餘可以包含註解的格式（Hermes、OpenClaw、Kimi Code、Gajae Code——以整份文件寫出的 YAML、JSON5 與 TOML），或當我們自己的條目被編輯過時，開關會鎖定，停用會拒絕執行，而不是猜測哪些編輯是你的。 ## 誠實的預期 diff --git a/src/integrations/config-io.ts b/src/integrations/config-io.ts index 7f8e0ffc5..4f2a83482 100644 --- a/src/integrations/config-io.ts +++ b/src/integrations/config-io.ts @@ -9,6 +9,7 @@ */ import { mkdirSync, readFileSync, rmSync, statSync } from "node:fs"; import type { ConfigFormat } from "../clients/config-export"; +import { MAX_JSON_NESTING } from "./serialize"; import { atomicWriteFile } from "../config"; import type { JournalEntry } from "./journal"; import type { OwnershipRecord } from "./ownership"; @@ -23,12 +24,129 @@ import type { IntegrationClientId } from "./registry"; */ export const PARSE_FAILED = Symbol("parse-failed"); +/** + * What `JSON.parse` has already discarded by the time we hold the parsed value, + * and a rewrite would therefore silently change. Both classes are invisible in + * the parsed object, which is why this scans the RAW text — same reasoning as + * the TOML inf/nan guard below. + * + * Numbers: `1e999` overflows to Infinity (a rewrite bakes in `null` — the merge + * layer's JSON clone does it even before the serializer could refuse), `1e-9999` + * underflows to `+0`, an integer literal may have been rounded (a rewrite then + * hands consumers that read JSON integers exactly — python, jq, BigInt revivers + * — a different value), and `-0` re-serializes as `0`. Only literals whose value + * actually changed are refused: `1e21`, `1e-320` or 2^54 round-trip exactly and + * stay usable. + * + * Duplicate members: `{"a":1,"a":2}` parses to a single `a`, so rewriting the + * document DELETES the earlier member. That is content loss, not the formatting + * normalization we promise, and this classifier is what makes the rewrite of a + * user-edited file reachable at all — so it fails closed here. + * + * Depth: the same pass counts container nesting against MAX_JSON_NESTING + * (shared with the serializer, see serialize.ts). JSON.parse handles hundreds + * of thousands of levels iteratively, but the downstream merge and + * JSON.stringify recurse — a 100KB file nested 50k deep sailed through parse + * and guard, then blew up serialization with a raw RangeError after a + * multi-GB allocation spike. A hand-written scan rather than a JSON.parse + * source-access reviver on purpose: the reviver walk recurses internally, so + * its depth limit would be an unpredictable stack size instead of this + * deterministic ceiling. + */ +function jsonTextSafeToRewrite(text: string): boolean { + /** + * One frame per open container; a Set of member names for objects, null for + * arrays. Its length is the current nesting depth. + */ + const containers: Array | null> = []; + /** The most recent string literal — the member name if a `:` follows. */ + let lastString: string | null = null; + let inString = false; + let escaped = false; + let stringStart = 0; + for (let i = 0; i < text.length; i += 1) { + const ch = text[i]!; + if (inString) { + if (escaped) escaped = false; + else if (ch === "\\") escaped = true; + else if (ch === "\"") { + inString = false; + lastString = text.slice(stringStart, i + 1); + } + continue; + } + if (ch === "\"") { inString = true; stringStart = i; continue; } + if (ch === "{" || ch === "[") { + containers.push(ch === "{" ? new Set() : null); + if (containers.length > MAX_JSON_NESTING) return false; + lastString = null; + continue; + } + if (ch === "}" || ch === "]") { containers.pop(); lastString = null; continue; } + if (ch === ":") { + const members = containers[containers.length - 1]; + if (members && lastString !== null) { + /* + * Decoded, not raw: `"a"` and `"a"` are spellings of ONE member, + * and JSON.parse keeps only the last of them. + */ + let name: string; + try { name = JSON.parse(lastString) as string; } catch { return false; } + if (members.has(name)) return false; + members.add(name); + } + lastString = null; + continue; + } + if (ch !== "-" && (ch < "0" || ch > "9")) continue; + let end = i + 1; + while (end < text.length && /[0-9+\-.eE]/.test(text[end]!)) end += 1; + const literal = text.slice(i, end); + i = end - 1; + const value = Number(literal); + if (!Number.isFinite(value)) return false; + if (value === 0) { + /* + * `-0` (re-serializes as `0`) and underflow: `1e-9999` is a nonzero + * value the parse already flattened to `+0`. The significand alone + * decides, so genuine zero spellings (`0`, `0.0`, `0e10`) stay usable. + */ + if (literal.startsWith("-") || /[1-9]/.test(literal.split(/[eE]/)[0]!)) return false; + } + /* + * Deliberately plain digit runs only. They are the one spelling real + * consumers read with exact integer semantics (python's json yields an + * arbitrary-precision int, jq preserves big integer literals), so baking + * in the rounded double changes what those consumers extract. Decimal or + * exponent spellings of the same value (`9007199254740993e0`, `…3.0`) are + * float semantics for every consumer — they round identically before and + * after a rewrite, and shortest-round-trip stringify preserves what any + * reader can observe, so refusing them would only manufacture dead ends + * (`1e308` is not exactly representable either, yet rewrites losslessly + * for every possible reader). + */ + const digits = literal[0] === "-" ? literal.slice(1) : literal; + if (/^[0-9]{16,}$/.test(digits) && BigInt(literal) !== BigInt(value)) return false; + } + return true; +} + /** Parse a client config, tolerating absence. PARSE_FAILED on garbage. */ export function parseConfig(text: string | null, format: ConfigFormat): unknown | typeof PARSE_FAILED { if (text === null || text.trim().length === 0) return {}; try { switch (format) { - case "json": return JSON.parse(text); + case "json": { + /* + * Scanned BEFORE parsing: the guard is text-only, and refusing first + * means a hostile document is never materialized — the depth ceiling + * would otherwise cap the rewrite only after JSON.parse had already + * built the 50k-deep object graph. The outcome is unchanged: invalid + * JSON still returns PARSE_FAILED, from the catch below. + */ + if (!jsonTextSafeToRewrite(text)) return PARSE_FAILED; + return JSON.parse(text); + } case "json5": return Bun.JSON5.parse(text); case "yaml": return Bun.YAML.parse(text); case "toml": { diff --git a/src/integrations/serialize.ts b/src/integrations/serialize.ts index 7ef402916..d64950f95 100644 --- a/src/integrations/serialize.ts +++ b/src/integrations/serialize.ts @@ -221,10 +221,89 @@ export function renderToml(document: Record, prefix = ""): stri return `${[scalars.join("\n"), tables.join("\n\n")].filter(Boolean).join("\n\n")}\n`; } +/** + * Ceiling on container nesting for json documents, shared by the parse-time + * scanner (config-io.ts) and the serializer walk below. One constant on + * purpose: the walk must accept every document the scanner admits, or a file + * the classifier reported as recoverable would refuse at rewrite time. Real + * configs nest a handful of levels. + */ +export const MAX_JSON_NESTING = 1000; + +/** Error messages carry the path to the offending value; keep them readable. */ +function clampPath(path: string): string { + return path.length > 200 ? `${path.slice(0, 100)}…${path.slice(-100)}` : path; +} + +/** + * JSON.stringify writes a non-finite number as `null` and -0 as `0`; any + * other finite double round-trips value-exactly (literal-level rounding is + * the parse-time scanner's concern), so those two are exactly what this walk + * refuses — refusing more turned a state the classifier had promised as + * recoverable into a permanent refusal. Documents read from disk are already + * guarded at parse time, and the writer's merge layer JSON-clones documents — + * normalizing these values — before serializing, so on the apply/disable path + * this walk is unreachable for them: it guards the direct serializers + * (preview/export builders), same posture as the YAML and TOML renderers + * above, and enforces the nesting ceiling for every json caller before the + * recursive JSON.stringify can turn depth into a RangeError. + * + * Iterative frames instead of recursion or a node stack: depth AND size of + * the document are inputs under the writer of the config file. Recursion made + * a deep file a RangeError-500; materializing every node with its path made a + * wide file allocate a large multiple of its size. Frames keep memory + * proportional to nesting depth, and path strings exist only for the + * containers on the current path plus the failing value itself. + */ +function assertJsonNumbersRoundTrip(document: unknown, rootPath: string): void { + const refuse = (value: number, path: string): never => { + throw new UnserializableValueError(Object.is(value, -0) + ? `JSON cannot rewrite -0 at ${clampPath(path)} without changing it to 0` + : `JSON cannot rewrite the number at ${clampPath(path)} without changing it to null`); + }; + if (typeof document === "number" && (!Number.isFinite(document) || Object.is(document, -0))) { + refuse(document, rootPath); + } + type Frame = { container: unknown; keys: string[] | null; index: number; prefix: string }; + const frames: Frame[] = []; + const pushContainer = (value: unknown, prefix: string) => { + if (Array.isArray(value)) frames.push({ container: value, keys: null, index: 0, prefix }); + else if (isPlainRecord(value)) frames.push({ container: value, keys: Object.keys(value), index: 0, prefix }); + }; + pushContainer(document, rootPath); + while (frames.length > 0) { + const frame = frames[frames.length - 1]!; + const length = frame.keys ? frame.keys.length : (frame.container as unknown[]).length; + if (frame.index >= length) { frames.pop(); continue; } + const i = frame.index; + frame.index += 1; + const child = frame.keys + ? (frame.container as Record)[frame.keys[i]!] + : (frame.container as unknown[])[i]; + const childPath = () => frame.keys + ? (frame.prefix === "$" ? frame.keys[i]! : `${frame.prefix}.${frame.keys[i]!}`) + : `${frame.prefix}[${i}]`; + if (typeof child === "number") { + if (!Number.isFinite(child) || Object.is(child, -0)) refuse(child, childPath()); + continue; + } + if (typeof child === "object" && child !== null) { + if (frames.length >= MAX_JSON_NESTING) { + throw new UnserializableValueError( + `the document nests deeper than ${MAX_JSON_NESTING} levels at ${clampPath(childPath())}, which JSON serialization cannot rewrite safely`); + } + pushContainer(child, childPath()); + } + } +} + /** Every serializer returns text ending in exactly one newline. */ export function serializeDocument(document: unknown, format: ConfigFormat): string { switch (format) { - case "json": return `${JSON.stringify(document, null, 2)}\n`; + case "json": { + assertJsonNumbersRoundTrip(document, "$"); + return `${JSON.stringify(document, null, 2)}\n`; + } case "json5": return `${Bun.JSON5.stringify(document, null, 2)}\n`; case "yaml": return renderYaml(document); case "toml": { diff --git a/src/integrations/state.ts b/src/integrations/state.ts index b6b970de1..443d093f1 100644 --- a/src/integrations/state.ts +++ b/src/integrations/state.ts @@ -134,9 +134,15 @@ function recordedFragmentFingerprint( /** * The two-axis rule: the recorded bytes or fragments prove nobody changed * what we may rewrite, and the contribution hash proves our catalog has not - * moved on. OMP is the sole fragment-scoped client because its writer patches - * only `providers.opencodex`; every whole-document serializer retains the - * whole-file fingerprint guard. + * moved on. Three classes of client (revising the unconditional whole-file + * rule of devlog 260802_client_toggle_api/021 §3 for json — #1631): + * OMP is fragment-scoped because its writer patches only + * `providers.opencodex`, so the whole-file check is skipped entirely; + * strict-json clients keep the whole-file check but downgrade a drift with + * intact owned fragments to `stale`, because a rewrite there can lose only + * formatting (comments cannot parse, non-round-tripping numbers are refused + * by the serializer); every comment-capable whole-document serializer (yaml, + * json5, toml) retains the whole-file fingerprint guard as a hard conflict. */ export function classifyIntegration(input: { fileText: string | null; @@ -180,12 +186,37 @@ export function classifyIntegration(input: { return { state: "conflict", reason: "unowned-key" }; } const clientId = input.clientId ?? input.record.clientId; - if (clientId !== "omp" && fingerprint(input.fileText ?? "") !== input.record.fileFingerprint) { - return { state: "conflict", reason: "foreign-edit" }; - } + /* + * Checked BEFORE file-level drift: an edit INSIDE an owned fragment is a + * conflict no matter what the rest of the file looks like, so the sibling- + * edit exemption below can never mask it. + */ if (recordedFragmentFingerprint(input.parsed, input.record) !== input.record.blockFingerprint) { return { state: "conflict", reason: "foreign-edit" }; } + if (clientId !== "omp" && fingerprint(input.fileText ?? "") !== input.record.fileFingerprint) { + /* + * The file changed since we wrote it, but every fragment we own is still + * byte-for-byte what we put there — a sibling edit, not tampering. Apply + * rewrites the WHOLE document, so for comment-capable formats (yaml, + * json5, toml) it would drop comments the user wrote next to us: fail + * closed there. Strict JSON cannot carry comments — a commented file + * never reaches this branch because parsing already failed — so the only + * possible loss is formatting normalization: everything a rewrite would + * actually change (numbers that would not round-trip, duplicate members + * a rewrite would delete) is PARSE_FAILED in parseConfig and classifies + * as unsafe long before this branch, exactly like comments. Refusing + * forever over formatting + * dead-ends the integration on the user's first own config edit (#1631). + * Report drift instead; a re-apply merges into the parsed document as it + * stands and re-owns the file. This also lets disable proceed on a + * drifted file — removal still touches only the recorded fragment paths. + */ + if (EXPORT_CLIENTS[clientId].format !== "json") { + return { state: "conflict", reason: "foreign-edit" }; + } + return { state: "stale" }; + } return input.record.blockFingerprint === fingerprint(canonicalContribution(input.contribution)) ? { state: "current" } : { state: "stale" }; diff --git a/src/integrations/writer.ts b/src/integrations/writer.ts index 9afbc4df1..4c6abd6e9 100644 --- a/src/integrations/writer.ts +++ b/src/integrations/writer.ts @@ -213,7 +213,8 @@ function preflight(input: IntegrationWriteInput) { const before = target.before; const parsed = parseConfig(before, exportSpec.format); if (parsed === PARSE_FAILED) { - return { failed: refuse(clientId, "unsafe", "unsafe", `${configPath} could not be parsed`) } as const; + return { failed: refuse(clientId, "unsafe", "unsafe", + `${configPath} could not be parsed, or holds something opencodex cannot rewrite without changing it (a non-finite number, a large integer or a tiny one a rewrite would round, -0, a duplicate member, or nesting deeper than 1000 levels)`) } as const; } const contribution = exportSpec.buildContribution(exportContextOf(input)); // A record proves ownership of the file it was written FOR. Matching only by @@ -368,8 +369,15 @@ export function disableIntegration(input: IntegrationWriteInput): WriteOutcome { : `${configPath} cannot be changed safely`); } - // current | stale only: the file fingerprint still matches our record, so the - // recorded paths are exactly what we put there. + /* + * current | stale only. What makes the removal safe is the BLOCK + * fingerprint, not the file fingerprint: the classifier verified the values + * at the recorded paths are byte-for-byte what we wrote, so removing them + * cannot take a user edit with them. The file itself may have drifted — a + * json client classifies a sibling edit as stale (#1631) — which is why the + * removal runs against the document as parsed NOW, and the re-serialize is + * value-safe because non-round-tripping numbers were refused at parse time. + */ const { doc, removed } = removeFragments( parsed, record!.fragmentPaths, diff --git a/tests/integrations-serialize.test.ts b/tests/integrations-serialize.test.ts index ae5d07191..10f4ceef8 100644 --- a/tests/integrations-serialize.test.ts +++ b/tests/integrations-serialize.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test"; import { FORMAT_MEDIA_TYPE, + UnserializableValueError, quoteTomlKey, renderToml, renderYaml, @@ -155,6 +156,57 @@ describe("serializeDocument", () => { expect(() => serializeDocument([1, 2], "toml")).toThrow(/TOML root must be a table/); }); + test("json refuses numbers that would not survive the rewrite", () => { + expect(() => serializeDocument({ q: Infinity }, "json")).toThrow(UnserializableValueError); + expect(() => serializeDocument({ q: Number.NaN }, "json")).toThrow(UnserializableValueError); + expect(() => serializeDocument({ q: -0 }, "json")).toThrow(UnserializableValueError); + // The failure names where the value sits — including through arrays and + // for a bare root scalar. + expect(() => serializeDocument({ a: { b: [1, -Infinity] } }, "json")).toThrow(/a\.b\[1\]/); + expect(() => serializeDocument({ a: [{ b: -0 }] }, "json")).toThrow(/a\[0\]\.b/); + expect(() => serializeDocument(-0, "json")).toThrow(/at \$/); + // Empty containers pass the walk untouched. + expect(serializeDocument({}, "json")).toBe("{}\n"); + expect(serializeDocument([], "json")).toBe("[]\n"); + }); + + test("json keeps every finite non-negative-zero double, however large", () => { + // A finite double round-trips value-exactly; refusing 2^54 here while the + // parse-time scanner admits it turned a classifier-promised 'stale' into + // a permanent 'unsafe' refusal. + expect(JSON.parse(serializeDocument({ q: 2 ** 54 }, "json"))).toEqual({ q: 2 ** 54 }); + expect(JSON.parse(serializeDocument({ q: 1e21 }, "json"))).toEqual({ q: 1e21 }); + }); + + test("hostile nesting depth is a structured refusal, not a RangeError", () => { + // The walk itself is iterative, and past MAX_SERIALIZED_JSON_NESTING it + // refuses — JSON.stringify recurses, so letting a 50k-deep document + // through would trade the refusal for a multi-GB spike and a raw + // RangeError. The refusal message stays readable (clamped path). + let deep: unknown = -Infinity; + for (let i = 0; i < 100_000; i += 1) deep = [deep]; + try { + serializeDocument({ q: deep }, "json"); + throw new Error("expected a refusal"); + } catch (error) { + expect(error).toBeInstanceOf(UnserializableValueError); + expect((error as Error).message).toContain("nests deeper"); + expect((error as Error).message.length).toBeLessThan(500); + // The clamp keeps head and tail of the path, joined by an ellipsis. + expect((error as Error).message).toMatch(/q\[0\].*….*\[0\]/); + } + }); + + test("nesting within the ceiling still serializes, up to the exact boundary", () => { + // The ceiling admits exactly MAX_JSON_NESTING container levels — the same + // count the parse-time scanner admits. Pinning both edges catches an + // off-by-one drift in either layer. + let atCeiling: unknown = 1; + for (let i = 0; i < 1000; i += 1) atCeiling = [atCeiling]; + expect(() => serializeDocument(atCeiling, "json")).not.toThrow(); + expect(() => serializeDocument([atCeiling], "json")).toThrow(/nests deeper/); + }); + test("media types are declared for every format", () => { expect(Object.keys(FORMAT_MEDIA_TYPE).sort()).toEqual(["json", "json5", "toml", "yaml"]); }); diff --git a/tests/integrations-state.test.ts b/tests/integrations-state.test.ts index 5e854f86c..2fbb7a3b0 100644 --- a/tests/integrations-state.test.ts +++ b/tests/integrations-state.test.ts @@ -1,9 +1,10 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { EXPORT_CLIENTS, type ExportModel } from "../src/clients/config-export"; import { PARSE_FAILED, fileIO, loadTarget, parseConfig } from "../src/integrations/config-io"; +import { serializeDocument } from "../src/integrations/serialize"; import { canonicalContribution, fingerprint, @@ -128,6 +129,20 @@ describe("the five states, each triggered directly", () => { expect(readIntegrationState(input()).state).toBe("stale"); }); + test("stale: a sibling edit next to an intact json block is drift, not conflict", () => { + // The full readIntegrationState path — real file I/O, configPath and + // clientId guards engaged — not just the synthetic classify calls below. + seedRecord(seedOurConfig()); + const path = join(home, ".pi", "agent", "models.json"); + const edited = JSON.parse(readFileSync(path, "utf8")) as { + providers: Record; + }; + edited.providers.mine = { baseUrl: "http://user.invalid/v1" }; + writeFileSync(path, `${JSON.stringify(edited, null, 2)}\n`); + + expect(readIntegrationState(input()).state).toBe("stale"); + }); + test("conflict: an owned fragment changed after we wrote it", () => { const text = seedOurConfig(); seedRecord(text); @@ -384,6 +399,105 @@ describe("classifier unit behavior", () => { expect(parseConfig("{{{", "json")).toBe(PARSE_FAILED); }); + test("parseConfig refuses json number literals a rewrite would change", () => { + // Overflow to Infinity — a rewrite would bake in null. + expect(parseConfig("{\"a\": 1e999}", "json")).toBe(PARSE_FAILED); + expect(parseConfig("[-1e999]", "json")).toBe(PARSE_FAILED); + // Rounded at parse: consumers reading JSON integers exactly (python, jq) + // would see a different value after the rewrite. + expect(parseConfig("{\"a\": 9007199254740993}", "json")).toBe(PARSE_FAILED); + expect(parseConfig("{\"a\": [{\"b\": [9007199254740993]}]}", "json")).toBe(PARSE_FAILED); + // Negative zero re-serializes as 0 — in every literal spelling. + expect(parseConfig("{\"a\": -0}", "json")).toBe(PARSE_FAILED); + expect(parseConfig("{\"a\": -0.0}", "json")).toBe(PARSE_FAILED); + expect(parseConfig("{\"a\": -0e5}", "json")).toBe(PARSE_FAILED); + // Scanner lexing edges: a bare top-level literal (token touches both text + // boundaries), a literal right after a comma, and a backslash-terminated + // string followed by a real literal (escape-flag handling). + expect(parseConfig("9007199254740993", "json")).toBe(PARSE_FAILED); + expect(parseConfig("[1, 9007199254740993]", "json")).toBe(PARSE_FAILED); + expect(parseConfig("{\"a\": \"x\\\\\", \"b\": 9007199254740993}", "json")).toBe(PARSE_FAILED); + // Underflow: a nonzero value the parse already flattened to +0, so a + // rewrite would write 0. The sign is irrelevant here. + expect(parseConfig("{\"a\": 1e-9999}", "json")).toBe(PARSE_FAILED); + expect(parseConfig("{\"a\": -1e-9999}", "json")).toBe(PARSE_FAILED); + expect(parseConfig("{\"a\": 0.00001e-9999}", "json")).toBe(PARSE_FAILED); + }); + + test("parseConfig refuses duplicate json members a rewrite would delete", () => { + // JSON.parse keeps only the last member, so serializing the parsed + // document drops the earlier one — content loss, not normalization. + expect(parseConfig("{\"a\": 1, \"a\": 2}", "json")).toBe(PARSE_FAILED); + expect(parseConfig("{\"providers\": {\"mine\": 1}, \"providers\": {\"ocx\": 2}}", "json")) + .toBe(PARSE_FAILED); + // Two spellings of ONE member name: the comparison is on decoded names. + expect(parseConfig("{\"a\": 1, \"\\u0061\": 2}", "json")).toBe(PARSE_FAILED); + // Nested, and after a closed container (the frame must pop, not leak). + expect(parseConfig("{\"x\": {\"a\": 1, \"a\": 2}}", "json")).toBe(PARSE_FAILED); + expect(parseConfig("{\"a\": {\"b\": 1}, \"a\": 2}", "json")).toBe(PARSE_FAILED); + expect(parseConfig("{\"a\": [1], \"a\": 2}", "json")).toBe(PARSE_FAILED); + }); + + test("parseConfig keeps repeated names that are separate json members", () => { + // Same name in sibling objects, in array elements, and as string data — + // none of these lose anything in a rewrite. + expect(parseConfig("{\"a\": {\"b\": 1}, \"c\": {\"b\": 2}}", "json")) + .toEqual({ a: { b: 1 }, c: { b: 2 } }); + expect(parseConfig("[{\"a\": 1}, {\"a\": 2}]", "json")).toEqual([{ a: 1 }, { a: 2 }]); + expect(parseConfig("{\"a\": \"x:y\", \"b\": \"a\"}", "json")) + .toEqual({ a: "x:y", b: "a" }); + // A colon and a brace inside a string must not be read as structure. + expect(parseConfig("{\"a\": \"{\\\"a\\\": 1, \\\"a\\\": 2}\"}", "json")) + .toEqual({ a: "{\"a\": 1, \"a\": 2}" }); + }); + + test("parseConfig keeps json numbers that underflow to a genuine zero", () => { + // Exact-zero spellings: the value never changes, only the spelling may. + expect(parseConfig("{\"a\": 0e10}", "json")).toEqual({ a: 0 }); + expect(parseConfig("{\"a\": 0.0}", "json")).toEqual({ a: 0 }); + // A subnormal is a representable nonzero double — it survives a rewrite. + expect(parseConfig("{\"a\": 1e-320}", "json")).toEqual({ a: 1e-320 }); + }); + + test("parseConfig refuses json nested deeper than the rewrite can carry", () => { + // Nesting past the ceiling: parse would succeed, but the downstream + // rewrite machinery recurses — refuse at the trust boundary. + expect(parseConfig(`${"[".repeat(1001)}1${"]".repeat(1001)}`, "json")).toBe(PARSE_FAILED); + // The exact boundary: what the scanner admits, the serializer must also + // rewrite — one document through both layers, or a config the classifier + // reported recoverable would refuse at rewrite time. + const atCeiling = parseConfig(`${"[".repeat(1000)}1${"]".repeat(1000)}`, "json"); + expect(atCeiling).not.toBe(PARSE_FAILED); + expect(() => serializeDocument(atCeiling, "json")).not.toThrow(); + // Brackets inside strings do not count toward depth. + expect(parseConfig(`{"a": "${"[".repeat(2000)}"}`, "json")) + .toEqual({ a: "[".repeat(2000) }); + }); + + test("parseConfig keeps json numbers that round-trip exactly", () => { + // 1e21 and 2^54 are exactly representable doubles; only the literal's + // spelling may normalize, never the value any JSON consumer reads. + expect(parseConfig("{\"a\": 1e21}", "json")).toEqual({ a: 1e21 }); + expect(parseConfig("{\"a\": 18014398509481984}", "json")).toEqual({ a: 2 ** 54 }); + // A huge number inside a string is data, not a number literal — even + // behind an escaped quote. + expect(parseConfig("{\"a\": \"1e999\"}", "json")).toEqual({ a: "1e999" }); + expect(parseConfig("{\"a\": \"id 9007199254740993 ok\"}", "json")) + .toEqual({ a: "id 9007199254740993 ok" }); + expect(parseConfig("{\"a\": \"he said \\\" 9007199254740993\"}", "json")) + .toEqual({ a: "he said \" 9007199254740993" }); + // Decimal/exponent spellings are float semantics for every consumer — + // they round identically before and after a rewrite, so they stay usable + // (only plain digit runs carry exact-integer semantics, e.g. python's + // json module reads them as arbitrary-precision int). + expect(parseConfig("{\"a\": 9007199254740993e0}", "json")) + .toEqual({ a: 9007199254740992 }); + expect(parseConfig("{\"a\": 9007199254740993.0}", "json")) + .toEqual({ a: 9007199254740992 }); + // The guard is json-only: json5 keeps today's behavior. + expect(parseConfig("{\"a\": 9007199254740993}", "json5")).toEqual({ a: 9007199254740992 }); + }); + test("fragment order does not change the contribution fingerprint", () => { const reversed = { ...contribution, fragments: [...contribution.fragments].reverse() }; expect(canonicalContribution(reversed)).toBe(canonicalContribution(contribution)); @@ -471,7 +585,11 @@ describe("ownership is scoped to recorded fragments", () => { expect(result).toEqual({ state: "conflict", reason: "foreign-edit" }); }); - test("whole-document serializers still conflict on an unrelated source edit", () => { + test("json clients report a sibling edit as stale, not conflict", () => { + // Strict JSON cannot carry comments — a commented file fails parsing long + // before this branch — so rewriting the document cannot destroy anything + // but formatting. Refusing forever here dead-ended the integration on the + // user's first own config edit (#1631). const piContribution = { ...ownedContribution, clientId: "pi" as const }; const piRecord: OwnershipRecord = { ...record, @@ -487,6 +605,62 @@ describe("ownership is scoped to recorded fragments", () => { contribution: piContribution, }); + expect(result).toEqual({ state: "stale" }); + }); + + // Re-serializing a whole document in these formats would drop any comments + // the user keeps next to our block, so file-level drift stays a hard + // conflict for every one of them — a regression that narrowed the condition + // (say, to yaml only) must fail here, not in a user's config. + for (const { clientId, configPath } of [ + { clientId: "hermes" as const, configPath: "/tmp/hermes-config.yaml" }, + { clientId: "openclaw" as const, configPath: "/tmp/openclaw.json5" }, + { clientId: "kimi" as const, configPath: "/tmp/kimi-config.toml" }, + ]) { + test(`${clientId} (comment-capable) still conflicts on an unrelated source edit`, () => { + const contribution = { ...ownedContribution, clientId }; + const clientRecord: OwnershipRecord = { + ...record, + clientId, + configPath, + blockFingerprint: fingerprint(canonicalContribution(contribution)), + }; + const result = classifyIntegration({ + fileText: textWithExtra, + fileIsRegular: true, + parsed: documentWithExtra, + record: clientRecord, + contribution, + }); + + expect(result).toEqual({ state: "conflict", reason: "foreign-edit" }); + }); + } + + test("a json sibling edit combined with an edit inside our block is still a conflict", () => { + // The sibling-edit exemption must never mask tampering with an owned + // fragment: the block check runs first. + const piContribution = { ...ownedContribution, clientId: "pi" as const }; + const piRecord: OwnershipRecord = { + ...record, + clientId: "pi", + configPath: "/tmp/pi-models.json", + blockFingerprint: fingerprint(canonicalContribution(piContribution)), + }; + const editedDocument = { + providers: { + opencodex: { ...ownedValue, baseUrl: "http://user-edited.invalid/v1" }, + freebuff: extraValue, + }, + }; + const result = classifyIntegration({ + fileText: `${JSON.stringify(editedDocument, null, 2)}\n`, + fileIsRegular: true, + parsed: editedDocument, + record: piRecord, + contribution: piContribution, + }); + expect(result).toEqual({ state: "conflict", reason: "foreign-edit" }); }); }); diff --git a/tests/integrations-writer.test.ts b/tests/integrations-writer.test.ts index 7477e11fd..642c9eece 100644 --- a/tests/integrations-writer.test.ts +++ b/tests/integrations-writer.test.ts @@ -72,6 +72,14 @@ function installHermes(): string { return configPath; } +function installPi(): string { + const spec = INTEGRATION_CLIENTS.pi; + mkdirSync(spec.detectDir(TEST_ENV, home), { recursive: true }); + const configPath = spec.configPath(TEST_ENV, home); + mkdirSync(dirname(configPath), { recursive: true }); + return configPath; +} + function installOmp(): string { const spec = INTEGRATION_CLIENTS.omp; mkdirSync(spec.detectDir(TEST_ENV, home), { recursive: true }); @@ -151,6 +159,146 @@ describe("apply", () => { expect(readFileSync(configPath, "utf8")).toContain("api_mode: user_edited"); }); + test("json clients re-apply after a sibling edit and keep the user's entry (#1631)", () => { + const configPath = installPi(); + + expect(applyIntegration(input({ clientId: "pi" })).ok).toBe(true); + + // The user adds an unrelated sibling — the routine edit that used to + // dead-end the integration in `conflict` with no recovery path. + const doc = JSON.parse(readFileSync(configPath, "utf8")) as Record; + (doc.providers as Record).mine = { baseUrl: "http://user.invalid/v1" }; + writeFileSync(configPath, `${JSON.stringify(doc, null, 4)}\n`); + + const second = applyIntegration(input({ clientId: "pi" })); + expect(second.ok).toBe(true); + if (second.ok) expect(second.changed).toBe(true); + + const after = JSON.parse(readFileSync(configPath, "utf8")) as Record; + expect((after.providers as Record).mine).toEqual({ baseUrl: "http://user.invalid/v1" }); + // The block a fresh apply would write, not merely "something is there". + expect((after.providers as Record).opencodex).toMatchObject({ + baseUrl: "http://127.0.0.1:10100/v1", + }); + + // The re-apply re-owned the file: a third apply is a no-op again. + const third = applyIntegration(input({ clientId: "pi" })); + expect(third.ok).toBe(true); + if (third.ok) expect(third.changed).toBe(false); + }); + + test("json disable after a sibling edit keeps the sibling (#1631)", () => { + const configPath = installPi(); + + expect(applyIntegration(input({ clientId: "pi" })).ok).toBe(true); + const doc = JSON.parse(readFileSync(configPath, "utf8")) as Record; + (doc.providers as Record).mine = { baseUrl: "http://user.invalid/v1" }; + writeFileSync(configPath, `${JSON.stringify(doc, null, 2)}\n`); + + const result = disableIntegration(input({ clientId: "pi" })); + expect(result.ok).toBe(true); + + const after = JSON.parse(readFileSync(configPath, "utf8")) as Record; + expect((after.providers as Record).mine).toEqual({ baseUrl: "http://user.invalid/v1" }); + expect((after.providers as Record).opencodex).toBeUndefined(); + }); + + test("json apply refuses when a sibling number cannot round-trip", () => { + const configPath = installPi(); + + expect(applyIntegration(input({ clientId: "pi" })).ok).toBe(true); + // 1e999 is valid strict JSON but parses to Infinity; a rewrite would bake + // in `null`. The refusal must fire instead of reporting success. + const drifted = readFileSync(configPath, "utf8") + .replace(/^\{/, "{\n \"quota\": 1e999,"); + writeFileSync(configPath, drifted); + + const result = applyIntegration(input({ clientId: "pi" })); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("unsafe"); + // The file is untouched, the user's literal survives. + expect(readFileSync(configPath, "utf8")).toContain("1e999"); + }); + + test("json apply refuses a duplicate sibling member instead of deleting it", () => { + const configPath = installPi(); + + expect(applyIntegration(input({ clientId: "pi" })).ok).toBe(true); + // Valid strict JSON, but JSON.parse keeps only the last "notes" — a + // rewrite would silently delete the first one while reporting success. + const drifted = readFileSync(configPath, "utf8") + .replace(/^\{/, "{\n \"notes\": \"keep me\",\n \"notes\": \"second\","); + writeFileSync(configPath, drifted); + + const result = applyIntegration(input({ clientId: "pi" })); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("unsafe"); + // Byte-for-byte untouched: both members survive on disk. + expect(readFileSync(configPath, "utf8")).toBe(drifted); + }); + + test("a sibling with an exactly-representable big number stays usable (#1631)", () => { + // 2^54 round-trips value- and literal-exactly. classify promises 'stale' + // (recoverable) for this file; apply must honor that promise instead of + // refusing at serialize time — the asymmetry that re-created the dead-end. + const configPath = installPi(); + + expect(applyIntegration(input({ clientId: "pi" })).ok).toBe(true); + const drifted = readFileSync(configPath, "utf8") + .replace(/^\{/, "{\n \"quota\": 18014398509481984,"); + writeFileSync(configPath, drifted); + + const second = applyIntegration(input({ clientId: "pi" })); + expect(second.ok).toBe(true); + + expect(readFileSync(configPath, "utf8")).toContain("18014398509481984"); + const after = JSON.parse(readFileSync(configPath, "utf8")) as Record; + expect(after.quota).toBe(2 ** 54); + }); + + test("disable also honors a 2^54 sibling: proceeds and keeps the literal", () => { + const configPath = installPi(); + + expect(applyIntegration(input({ clientId: "pi" })).ok).toBe(true); + const drifted = readFileSync(configPath, "utf8") + .replace(/^\{/, "{\n \"quota\": 18014398509481984,"); + writeFileSync(configPath, drifted); + + const result = disableIntegration(input({ clientId: "pi" })); + expect(result.ok).toBe(true); + + const text = readFileSync(configPath, "utf8"); + expect(text).toContain("18014398509481984"); + const after = JSON.parse(text) as Record; + expect(after.quota).toBe(2 ** 54); + expect((after.providers as Record | undefined)?.opencodex).toBeUndefined(); + }); + + test("json disable also refuses when a sibling number cannot round-trip", () => { + const configPath = installPi(); + + expect(applyIntegration(input({ clientId: "pi" })).ok).toBe(true); + const drifted = readFileSync(configPath, "utf8") + .replace(/^\{/, "{\n \"quota\": 1e999,"); + writeFileSync(configPath, drifted); + + const result = disableIntegration(input({ clientId: "pi" })); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("unsafe"); + expect(readFileSync(configPath, "utf8")).toContain("1e999"); + }); + + test("yaml clients still refuse a sibling edit rather than risk user comments", () => { + const configPath = installHermes(); + expect(applyIntegration(input()).ok).toBe(true); + writeFileSync(configPath, `${readFileSync(configPath, "utf8")}unknown_top: added-later\n`); + + const result = applyIntegration(input()); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toBe("conflict"); + expect(readFileSync(configPath, "utf8")).toContain("unknown_top: added-later"); + }); + test("refuses an unparseable config rather than overwriting it", () => { const configPath = installHermes(); writeFileSync(configPath, "{{{ not yaml\n");