diff --git a/docs-site/src/content/docs/guides/integrations.md b/docs-site/src/content/docs/guides/integrations.md index 05d371bc2..49720512f 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`, 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 21d3d2037..db4890d45 100644 --- a/docs-site/src/content/docs/tr/guides/integrations.md +++ b/docs-site/src/content/docs/tr/guides/integrations.md @@ -77,9 +77,23 @@ 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` 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 d9ed0614a..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 記錄為自己寫入的條目。如果你的檔案在我們寫入之後有變更，開關會鎖定，停用會拒絕執行，而不是猜測哪些編輯是你的。 +停用只移除 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 7f8e0ffc5..3a3bdf076 100644 --- a/src/integrations/config-io.ts +++ b/src/integrations/config-io.ts @@ -23,12 +23,110 @@ 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. + * + * Scanning also avoids recursing over attacker-shaped nesting depth. + */ +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; + 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 !== "-" && (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": { + const parsed = JSON.parse(text); + return jsonTextSafeToRewrite(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 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..8f7733e35 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, 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 @@ -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 5e854f86c..f899f79d2 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,90 @@ 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 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 +569,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 +589,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..618f344bd 100644 --- a/tests/integrations-writer.test.ts +++ b/tests/integrations-writer.test.ts @@ -151,6 +151,167 @@ 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" }); + // 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 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("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 + // 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); + 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");