From 30c1709cfbc39cb7f46ec5b56cecd82f5fb737c8 Mon Sep 17 00:00:00 2001 From: Robin Bially <7304732+RobinBially@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:08:04 +0200 Subject: [PATCH 1/5] fix(integrations): classify a json sibling edit as stale instead of conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A user edit anywhere in a client's config file flipped the integration into a permanent conflict, even when every opencodex-owned fragment was untouched — adding an MCP server to opencode.json was enough, and the only way out was deleting the owned block by hand. The whole-file fingerprint exists so a rewrite never destroys comments or formatting we did not write. For comment-capable formats (yaml, json5, toml) that stays a hard conflict. Strict JSON cannot carry comments — a commented file fails parsing long before classification — so with the owned block verified intact, re-applying can only normalize formatting. Classify that case as stale: the toggle offers a refresh, and apply re-owns the file while merging into the document as it stands, keeping the user's entries. The owned-fragment check now runs before the file-level check so the exemption can never mask an edit inside our block. Fixes #1631 Co-Authored-By: Claude Fable 5 --- src/integrations/state.ts | 26 +++++++++++++-- tests/integrations-state.test.ts | 55 ++++++++++++++++++++++++++++++- tests/integrations-writer.test.ts | 39 ++++++++++++++++++++++ 3 files changed, 116 insertions(+), 4 deletions(-) diff --git a/src/integrations/state.ts b/src/integrations/state.ts index b6b970de1..3e8fb101d 100644 --- a/src/integrations/state.ts +++ b/src/integrations/state.ts @@ -180,12 +180,32 @@ 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, and refusing forever over + * that 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. + */ + 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/tests/integrations-state.test.ts b/tests/integrations-state.test.ts index 5e854f86c..eff433676 100644 --- a/tests/integrations-state.test.ts +++ b/tests/integrations-state.test.ts @@ -471,7 +471,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 +491,55 @@ describe("ownership is scoped to recorded fragments", () => { contribution: piContribution, }); + expect(result).toEqual({ state: "stale" }); + }); + + test("comment-capable formats still conflict on an unrelated source edit", () => { + // hermes writes YAML: re-serializing the whole document would drop any + // comments the user keeps next to our block, so file-level drift stays a + // hard conflict there. + const hermesContribution = { ...ownedContribution, clientId: "hermes" as const }; + const hermesRecord: OwnershipRecord = { + ...record, + clientId: "hermes", + configPath: "/tmp/hermes-config.yaml", + blockFingerprint: fingerprint(canonicalContribution(hermesContribution)), + }; + const result = classifyIntegration({ + fileText: textWithExtra, + fileIsRegular: true, + parsed: documentWithExtra, + record: hermesRecord, + contribution: hermesContribution, + }); + + 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..577a554c9 100644 --- a/tests/integrations-writer.test.ts +++ b/tests/integrations-writer.test.ts @@ -151,6 +151,45 @@ 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 spec = INTEGRATION_CLIENTS.pi; + mkdirSync(spec.detectDir(TEST_ENV, home), { recursive: true }); + const configPath = spec.configPath(TEST_ENV, home); + mkdirSync(dirname(configPath), { recursive: true }); + + 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" }); + expect((after.providers as Record).opencodex).toBeDefined(); + + // 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("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"); From 43ef83fab72eeffa3cb762335ec49c90ae1243f6 Mon Sep 17 00:00:00 2001 From: Robin Bially <7304732+RobinBially@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:25:28 +0200 Subject: [PATCH 2/5] fix(integrations): guard the rewrite against non-round-tripping numbers, after multi-round review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates four adversarial review rounds on the initial commit into the final, focused shape of the fix: - parseConfig scans the raw json text — same posture as the TOML inf/nan guard — and refuses number literals whose value a rewrite would actually change: overflow to Infinity (rewritten as null; the merge layer's JSON clone does it before any serializer could refuse), plain-digit integer runs a BigInt comparison proves were rounded past 2^53 (the one spelling consumers like python's json read with exact integer semantics), and -0, which re-serializes as 0. Exponent spellings and exactly-representable big integers (1e21, 2^54) stay usable end to end — refusing them would only manufacture new dead ends; that decision is pinned in comments and tests. Without this guard, the newly allowed rewrite route would bake silent value changes into files the old conflict refusal used to protect. - disableIntegration's precondition comment now names the real invariant (the block fingerprint, not the file fingerprint), with sibling-survival and refusal mirror tests for disable. - The classifier's module comment revises devlog 021 §3's unconditional whole-file rule for json clients; the preflight refusal message names the non-round-trip value class instead of claiming a valid file 'could not be parsed'; docs (en, zh-tw) describe the behavior including the exception. - Regression tests: readIntegrationState-level sibling drift, openclaw/kimi comment-capable conflicts, scanner lexer edges (bare literal, escaped quotes, -0 spellings), 1e999 refusal and 2^54 symmetry for apply AND disable, re-apply block shape and re-ownership. Deeper hardening surfaced by the same review (nesting depth ceiling, serializer value walk for builder documents) targets pre-existing exposure and follows separately on hardening/json-rewrite-depth. Co-Authored-By: Claude Fable 5 --- .../src/content/docs/guides/integrations.md | 9 +- .../content/docs/zh-tw/guides/integrations.md | 2 +- src/integrations/config-io.ts | 55 ++++++++- src/integrations/state.ts | 24 ++-- src/integrations/writer.ts | 14 ++- tests/integrations-serialize.test.ts | 4 + tests/integrations-state.test.ts | 106 ++++++++++++++---- tests/integrations-writer.test.ts | 104 ++++++++++++++++- 8 files changed, 283 insertions(+), 35 deletions(-) diff --git a/docs-site/src/content/docs/guides/integrations.md b/docs-site/src/content/docs/guides/integrations.md index 05d371bc2..4c1782307 100644 --- a/docs-site/src/content/docs/guides/integrations.md +++ b/docs-site/src/content/docs/guides/integrations.md @@ -65,7 +65,14 @@ 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 +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 a value JSON cannot rewrite exactly — a non-finite +number like `1e999`, an integer literal past 2^53, or `-0` — which locks the +switch instead, so the value is never silently changed. For formats that can carry comments (YAML, JSON5, TOML), or when 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/zh-tw/guides/integrations.md b/docs-site/src/content/docs/zh-tw/guides/integrations.md index d9ed0614a..d9dff2996 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` 這類非有限數字、超過 2^53 的整數字面值,或 `-0`——此時開關會鎖定,確保這些值永遠不會被悄悄改動。對於可以包含註解的格式(YAML、JSON5、TOML),或當我們自己的條目被編輯過時,開關會鎖定,停用會拒絕執行,而不是猜測哪些編輯是你的。 ## 誠實的預期 diff --git a/src/integrations/config-io.ts b/src/integrations/config-io.ts index 7f8e0ffc5..dd024323a 100644 --- a/src/integrations/config-io.ts +++ b/src/integrations/config-io.ts @@ -23,12 +23,65 @@ import type { IntegrationClientId } from "./registry"; */ export const PARSE_FAILED = Symbol("parse-failed"); +/** + * Number literals JSON.parse has already damaged: `1e999` overflows to + * Infinity (a later rewrite would bake in `null` — the merge layer's JSON + * clone does it even before the serializer could refuse), an integer literal + * beyond 2^53 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`. By the time the document is parsed the + * original literal is gone, which is why this scans the RAW text — same + * reasoning as the TOML inf/nan guard below — and only literals whose value + * actually changed: `1e21` or 2^54 round-trip exactly and stay usable. + * Scanning also avoids recursing over attacker-shaped nesting depth. + */ +function jsonNumberLiteralsRoundTrip(text: string): boolean { + let inString = false; + let escaped = false; + 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; + continue; + } + if (ch === "\"") { inString = true; 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 && literal.startsWith("-")) 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": { + const parsed = JSON.parse(text); + return jsonNumberLiteralsRoundTrip(text) ? parsed : PARSE_FAILED; + } case "json5": return Bun.JSON5.parse(text); case "yaml": return Bun.YAML.parse(text); case "toml": { diff --git a/src/integrations/state.ts b/src/integrations/state.ts index 3e8fb101d..55fd6901b 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; @@ -196,10 +202,14 @@ export function classifyIntegration(input: { * 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, and refusing forever over - * that 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. + * possible loss is formatting normalization: numbers that would not + * round-trip (non-finite, integers past 2^53) are PARSE_FAILED in + * parseConfig and classify 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" }; diff --git a/src/integrations/writer.ts b/src/integrations/writer.ts index 9afbc4df1..6031df868 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 a value opencodex cannot rewrite without changing it (a non-finite number, an integer past 2^53, or -0)`) } 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..3a285bdcf 100644 --- a/tests/integrations-serialize.test.ts +++ b/tests/integrations-serialize.test.ts @@ -155,6 +155,10 @@ describe("serializeDocument", () => { expect(() => serializeDocument([1, 2], "toml")).toThrow(/TOML root must be a table/); }); + + + + 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 eff433676..b3a075ca2 100644 --- a/tests/integrations-state.test.ts +++ b/tests/integrations-state.test.ts @@ -1,5 +1,5 @@ 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"; @@ -128,6 +128,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 +398,49 @@ 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); }); + + 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)); @@ -494,27 +551,34 @@ describe("ownership is scoped to recorded fragments", () => { expect(result).toEqual({ state: "stale" }); }); - test("comment-capable formats still conflict on an unrelated source edit", () => { - // hermes writes YAML: re-serializing the whole document would drop any - // comments the user keeps next to our block, so file-level drift stays a - // hard conflict there. - const hermesContribution = { ...ownedContribution, clientId: "hermes" as const }; - const hermesRecord: OwnershipRecord = { - ...record, - clientId: "hermes", - configPath: "/tmp/hermes-config.yaml", - blockFingerprint: fingerprint(canonicalContribution(hermesContribution)), - }; - const result = classifyIntegration({ - fileText: textWithExtra, - fileIsRegular: true, - parsed: documentWithExtra, - record: hermesRecord, - contribution: hermesContribution, + // 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" }); }); - - 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 diff --git a/tests/integrations-writer.test.ts b/tests/integrations-writer.test.ts index 577a554c9..c2e865fff 100644 --- a/tests/integrations-writer.test.ts +++ b/tests/integrations-writer.test.ts @@ -171,7 +171,10 @@ describe("apply", () => { 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).toBeDefined(); + // 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" })); @@ -179,6 +182,105 @@ describe("apply", () => { if (third.ok) expect(third.changed).toBe(false); }); + test("json disable after a sibling edit keeps the sibling (#1631)", () => { + 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 }); + + 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 spec = INTEGRATION_CLIENTS.pi; + mkdirSync(spec.detectDir(TEST_ENV, home), { recursive: true }); + const configPath = spec.configPath(TEST_ENV, home); + mkdirSync(dirname(configPath), { recursive: true }); + + 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("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 spec = INTEGRATION_CLIENTS.pi; + mkdirSync(spec.detectDir(TEST_ENV, home), { recursive: true }); + const configPath = spec.configPath(TEST_ENV, home); + mkdirSync(dirname(configPath), { recursive: true }); + + 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 spec = INTEGRATION_CLIENTS.pi; + mkdirSync(spec.detectDir(TEST_ENV, home), { recursive: true }); + const configPath = spec.configPath(TEST_ENV, home); + mkdirSync(dirname(configPath), { recursive: true }); + + 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 spec = INTEGRATION_CLIENTS.pi; + mkdirSync(spec.detectDir(TEST_ENV, home), { recursive: true }); + const configPath = spec.configPath(TEST_ENV, home); + mkdirSync(dirname(configPath), { recursive: true }); + + 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); From f93e50ff222786a1b5cdef7742df09f0735cfbc8 Mon Sep 17 00:00:00 2001 From: Robin Bially <7304732+RobinBially@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:26:08 +0200 Subject: [PATCH 3/5] perf(integrations): depth-cap json configs and harden the serializer walk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up hardening split out of #1632 (review findings on pre-existing exposure the sibling-edit fix did not enlarge — every first apply onto a foreign file always ran through the same rewrite layers): - parseConfig's raw-text scan now also counts container nesting (shared MAX_JSON_NESTING constant with the serializer): 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, then blew up serialization with a raw RangeError after a multi-GB allocation spike. Measured post-fix: PARSE_FAILED in 3ms, no spike. - serializeDocument('json') gains a value-safety walk for builder/preview documents (non-finite → null and -0 → 0 are the only values serialization itself damages; anything stricter re-created the recoverable-but-refused asymmetry #1632 closes). Iterative frames keep memory proportional to nesting depth instead of ~18x the document size a node stack cost, and the walk enforces the same ceiling as the scanner, with clamped paths in refusal messages. - Boundary pins: exactly 1000 levels parse AND serialize (one document through both layers), 1001 refuses on both, brackets inside strings do not count, clamp shape (head…tail) asserted. Based on #1632; review only the last commit until that lands. Co-Authored-By: Claude Fable 5 --- src/integrations/config-io.ts | 23 ++++++-- src/integrations/serialize.ts | 81 +++++++++++++++++++++++++++- tests/integrations-serialize.test.ts | 52 +++++++++++++++++- tests/integrations-state.test.ts | 16 +++++- 4 files changed, 165 insertions(+), 7 deletions(-) diff --git a/src/integrations/config-io.ts b/src/integrations/config-io.ts index dd024323a..d422a265b 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"; @@ -33,11 +34,21 @@ export const PARSE_FAILED = Symbol("parse-failed"); * original literal is gone, which is why this scans the RAW text — same * reasoning as the TOML inf/nan guard below — and only literals whose value * actually changed: `1e21` or 2^54 round-trip exactly and stay usable. - * Scanning also avoids recursing over attacker-shaped nesting 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 jsonNumberLiteralsRoundTrip(text: string): boolean { +function jsonTextSafeToRewrite(text: string): boolean { let inString = false; let escaped = false; + let depth = 0; for (let i = 0; i < text.length; i += 1) { const ch = text[i]!; if (inString) { @@ -47,6 +58,12 @@ function jsonNumberLiteralsRoundTrip(text: string): boolean { continue; } if (ch === "\"") { inString = true; continue; } + if (ch === "{" || ch === "[") { + depth += 1; + if (depth > MAX_JSON_NESTING) return false; + continue; + } + if (ch === "}" || ch === "]") { depth -= 1; continue; } if (ch !== "-" && (ch < "0" || ch > "9")) continue; let end = i + 1; while (end < text.length && /[0-9+\-.eE]/.test(text[end]!)) end += 1; @@ -80,7 +97,7 @@ export function parseConfig(text: string | null, format: ConfigFormat): unknown switch (format) { case "json": { const parsed = JSON.parse(text); - return jsonNumberLiteralsRoundTrip(text) ? parsed : PARSE_FAILED; + return jsonTextSafeToRewrite(text) ? parsed : PARSE_FAILED; } case "json5": return Bun.JSON5.parse(text); case "yaml": return Bun.YAML.parse(text); 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/tests/integrations-serialize.test.ts b/tests/integrations-serialize.test.ts index 3a285bdcf..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,9 +156,56 @@ 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 b3a075ca2..6163c6f11 100644 --- a/tests/integrations-state.test.ts +++ b/tests/integrations-state.test.ts @@ -4,6 +4,7 @@ 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, @@ -415,7 +416,20 @@ describe("classifier unit behavior", () => { // 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); }); + expect(parseConfig("{\"a\": \"x\\\\\", \"b\": 9007199254740993}", "json")).toBe(PARSE_FAILED); + // 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 From ad411319a5b7076802f0838514223b11d83313d4 Mon Sep 17 00:00:00 2001 From: Robin Bially <7304732+RobinBially@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:47:02 +0200 Subject: [PATCH 4/5] docs(i18n): carry the json sibling-edit behavior into the Turkish integrations guide The Turkish translation landed on dev after this branch changed the English and zh-tw paragraph, so tr/ still described the old fail-closed rule for strict-JSON clients. Mirrors the canonical wording 1-to-1. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/content/docs/tr/guides/integrations.md | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/docs-site/src/content/docs/tr/guides/integrations.md b/docs-site/src/content/docs/tr/guides/integrations.md index 21d3d2037..163dcdb3d 100644 --- a/docs-site/src/content/docs/tr/guides/integrations.md +++ b/docs-site/src/content/docs/tr/guides/integrations.md @@ -77,9 +77,18 @@ 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ğı bir değerdir +— `1e999` gibi sonlu olmayan bir sayı, 2^53'ü aşan bir tam sayı sabiti veya `-0` +— bu durumda anahtar kilitlenir, böylece değer hiçbir zaman sessizce +değiştirilmez. Yorum taşıyabilen biçimlerde (YAML, JSON5, 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? From 7f2e2dfa927758a3041f3d02d1430128c30147d0 Mon Sep 17 00:00:00 2001 From: Robin Bially <7304732+RobinBially@users.noreply.github.com> Date: Fri, 14 Aug 2026 01:08:11 +0200 Subject: [PATCH 5/5] fix(integrations): refuse underflow and duplicate members before a json rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps CodeRabbit found in the guard this PR introduces, both invisible in the parsed document and both reachable only because classify now hands a user-edited strict-JSON file to apply instead of conflicting forever: - 1e-9999 underflows to +0, so the rewrite wrote 0. The significand now decides, keeping genuine zero spellings (0, 0.0, 0e10) and subnormals. - {"a":1,"a":2} parses to a single member, so the rewrite DELETED the earlier one while reporting success. The scanner now tracks decoded member names per open object. Also corrects four texts that named 'an integer past 2^53' as a refusal cause when 2^54 is accepted (our own test applies with it), and the docs claim that YAML always locks — OMP patches only its own range and is exempt in classifyIntegration. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/content/docs/guides/integrations.md | 16 ++-- .../content/docs/tr/guides/integrations.md | 17 +++-- .../content/docs/zh-tw/guides/integrations.md | 2 +- src/integrations/config-io.ts | 73 +++++++++++++++---- src/integrations/state.ts | 9 ++- src/integrations/writer.ts | 2 +- tests/integrations-state.test.ts | 43 ++++++++++- tests/integrations-writer.test.ts | 20 +++++ 8 files changed, 150 insertions(+), 32 deletions(-) diff --git a/docs-site/src/content/docs/guides/integrations.md b/docs-site/src/content/docs/guides/integrations.md index 4c1782307..49720512f 100644 --- a/docs-site/src/content/docs/guides/integrations.md +++ b/docs-site/src/content/docs/guides/integrations.md @@ -69,11 +69,17 @@ after we wrote it, what happens depends on whether our own entries are still int 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 a value JSON cannot rewrite exactly — a non-finite -number like `1e999`, an integer literal past 2^53, or `-0` — which locks the -switch instead, so the value is never silently changed. For formats that can carry comments (YAML, JSON5, TOML), or when our own -entries were edited, the switch locks and disable refuses rather than guessing which -edits were yours. +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`, or the same key written twice in one object +— 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 163dcdb3d..db4890d45 100644 --- a/docs-site/src/content/docs/tr/guides/integrations.md +++ b/docs-site/src/content/docs/tr/guides/integrations.md @@ -83,12 +83,17 @@ Katı JSON yapılandırmalarında (OpenCode, Pi), bloğumuzun **yanında** yapı 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ğı bir değerdir -— `1e999` gibi sonlu olmayan bir sayı, 2^53'ü aşan bir tam sayı sabiti veya `-0` -— bu durumda anahtar kilitlenir, böylece değer hiçbir zaman sessizce -değiştirilmez. Yorum taşıyabilen biçimlerde (YAML, JSON5, 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. +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` veya aynı +nesnede iki kez yazılmış bir anahtar — 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 d9dff2996..faf9f451b 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 記錄為自己寫入的條目。如果你的檔案在我們寫入之後有變更,後續行為取決於我們自己的條目是否完好,以及檔案的格式。對於嚴格 JSON 設定檔(OpenCode、Pi),在我們的區塊**旁邊**進行的編輯——例如新增 MCP 伺服器或你自己的 provider——會顯示為**需要更新**:重新整理會在保留你的條目的前提下合併寫入,但格式可能會被正規化。例外情況是 JSON 無法精確重寫的值——例如 `1e999` 這類非有限數字、超過 2^53 的整數字面值,或 `-0`——此時開關會鎖定,確保這些值永遠不會被悄悄改動。對於可以包含註解的格式(YAML、JSON5、TOML),或當我們自己的條目被編輯過時,開關會鎖定,停用會拒絕執行,而不是猜測哪些編輯是你的。 +停用只移除 opencodex 記錄為自己寫入的條目。如果你的檔案在我們寫入之後有變更,後續行為取決於我們自己的條目是否完好,以及檔案的格式。對於嚴格 JSON 設定檔(OpenCode、Pi),在我們的區塊**旁邊**進行的編輯——例如新增 MCP 伺服器或你自己的 provider——會顯示為**需要更新**:重新整理會在保留你的條目的前提下合併寫入,但格式可能會被正規化。例外情況是 JSON 無法精確重寫的內容——例如 `1e999` 這類非有限數字、重寫會被四捨五入的數字(極大的整數,或小到會塌縮成零的數字)、`-0`,或同一個物件裡重複出現的鍵——此時開關會鎖定,確保沒有任何值被悄悄改動或刪除。**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 dd024323a..3a3bdf076 100644 --- a/src/integrations/config-io.ts +++ b/src/integrations/config-io.ts @@ -24,29 +24,67 @@ import type { IntegrationClientId } from "./registry"; export const PARSE_FAILED = Symbol("parse-failed"); /** - * Number literals JSON.parse has already damaged: `1e999` overflows to - * Infinity (a later rewrite would bake in `null` — the merge layer's JSON - * clone does it even before the serializer could refuse), an integer literal - * beyond 2^53 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`. By the time the document is parsed the - * original literal is gone, which is why this scans the RAW text — same - * reasoning as the TOML inf/nan guard below — and only literals whose value - * actually changed: `1e21` or 2^54 round-trip exactly and stay usable. + * 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. + * * Scanning also avoids recursing over attacker-shaped nesting depth. */ -function jsonNumberLiteralsRoundTrip(text: string): boolean { +function jsonTextSafeToRewrite(text: string): boolean { + /** One frame per open container; a Set for objects, null for arrays. */ + 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; + 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); + 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 === "\"") { inString = true; continue; } if (ch !== "-" && (ch < "0" || ch > "9")) continue; let end = i + 1; while (end < text.length && /[0-9+\-.eE]/.test(text[end]!)) end += 1; @@ -54,7 +92,14 @@ function jsonNumberLiteralsRoundTrip(text: string): boolean { i = end - 1; const value = Number(literal); if (!Number.isFinite(value)) return false; - if (value === 0 && literal.startsWith("-")) 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 @@ -80,7 +125,7 @@ export function parseConfig(text: string | null, format: ConfigFormat): unknown switch (format) { case "json": { const parsed = JSON.parse(text); - return jsonNumberLiteralsRoundTrip(text) ? parsed : PARSE_FAILED; + return jsonTextSafeToRewrite(text) ? parsed : PARSE_FAILED; } case "json5": return Bun.JSON5.parse(text); case "yaml": return Bun.YAML.parse(text); diff --git a/src/integrations/state.ts b/src/integrations/state.ts index 55fd6901b..443d093f1 100644 --- a/src/integrations/state.ts +++ b/src/integrations/state.ts @@ -202,10 +202,11 @@ export function classifyIntegration(input: { * 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: numbers that would not - * round-trip (non-finite, integers past 2^53) are PARSE_FAILED in - * parseConfig and classify as unsafe long before this branch, exactly - * like comments. Refusing forever over formatting + * 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 diff --git a/src/integrations/writer.ts b/src/integrations/writer.ts index 6031df868..8f7733e35 100644 --- a/src/integrations/writer.ts +++ b/src/integrations/writer.ts @@ -214,7 +214,7 @@ function preflight(input: IntegrationWriteInput) { const parsed = parseConfig(before, exportSpec.format); if (parsed === PARSE_FAILED) { return { failed: refuse(clientId, "unsafe", "unsafe", - `${configPath} could not be parsed, or holds a value opencodex cannot rewrite without changing it (a non-finite number, an integer past 2^53, or -0)`) } as const; + `${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, or a duplicate member)`) } as const; } const contribution = exportSpec.buildContribution(exportContextOf(input)); // A record proves ownership of the file it was written FOR. Matching only by diff --git a/tests/integrations-state.test.ts b/tests/integrations-state.test.ts index b3a075ca2..f899f79d2 100644 --- a/tests/integrations-state.test.ts +++ b/tests/integrations-state.test.ts @@ -415,7 +415,48 @@ describe("classifier unit behavior", () => { // 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); }); + 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 keeps json numbers that round-trip exactly", () => { // 1e21 and 2^54 are exactly representable doubles; only the literal's diff --git a/tests/integrations-writer.test.ts b/tests/integrations-writer.test.ts index c2e865fff..618f344bd 100644 --- a/tests/integrations-writer.test.ts +++ b/tests/integrations-writer.test.ts @@ -221,6 +221,26 @@ describe("apply", () => { expect(readFileSync(configPath, "utf8")).toContain("1e999"); }); + test("json apply refuses a duplicate sibling member instead of deleting it", () => { + 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 }); + + 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