|
| 1 | +# 030 — WP3: writer path (apply / disable / restore) |
| 2 | + |
| 3 | +Diff-level PRD. Depends on WP1 (`010`) and WP2 (`020`). This is the phase that |
| 4 | +earns the product's promise: every mutation is journaled, reversible, and |
| 5 | +refuses rather than guesses. It adds no route (WP4) and no UI (WP5/WP6). |
| 6 | + |
| 7 | +## Scope boundary |
| 8 | + |
| 9 | +IN |
| 10 | + |
| 11 | +- `src/integrations/writer.ts` — NEW (apply/disable/restore + preflight). |
| 12 | +- `src/integrations/merge.ts` — NEW (additive merge and removal per format). |
| 13 | +- `tests/integrations-writer.test.ts` — NEW. |
| 14 | + |
| 15 | +OUT |
| 16 | + |
| 17 | +- No routes, no GUI. No changes to the Grok or Claude Desktop writers — they |
| 18 | + keep their own semantics (004 §5.0); this module never touches their paths. |
| 19 | +- No vendor-CLI shelling (003 §5 Option B). File writing only in this phase; |
| 20 | + the CLI-delegation option stays an OPEN QUESTION below. |
| 21 | + |
| 22 | +## 1. Result contract |
| 23 | + |
| 24 | +```ts |
| 25 | +export type WriteOutcome = |
| 26 | + | { ok: true; changed: boolean; state: IntegrationState; opId?: string; message: string } |
| 27 | + | { ok: false; refused: RefusalReason; state: IntegrationState; message: string; snapshotPath?: string }; |
| 28 | + |
| 29 | +export type RefusalReason = |
| 30 | + | "not-installed" // detectDir missing |
| 31 | + | "conflict" // foreign edit or unowned key — never auto-delete |
| 32 | + | "unsafe" // unparseable / not a regular file |
| 33 | + | "non-loopback" // loopbackOnly client on a remote bind (kimi) |
| 34 | + | "drift-needs-confirm" // restore would replace post-snapshot edits |
| 35 | + | "snapshot-expired" // restore target was GC'd |
| 36 | + | "write-failed"; // the atomic write itself threw |
| 37 | +``` |
| 38 | + |
| 39 | +A refusal is a *result*, not an exception — the Grok precedent |
| 40 | +(`skippedReason` with `ok: true`) proved that a policy skip must be actionable |
| 41 | +in the UI rather than a 500. The difference here: a refusal sets `ok: false` |
| 42 | +because the user asked for a mutation that did not happen, and the GUI must |
| 43 | +show why. `snapshotPath` is included on failure paths so a user can finish by |
| 44 | +hand — 004 §6.2 rule 3, "a rollback feature that dead-ends silently is worse |
| 45 | +than none." |
| 46 | + |
| 47 | +## 2. `src/integrations/merge.ts` (NEW) |
| 48 | + |
| 49 | +Additive merge and surgical removal, per format, preserving unknown fields. |
| 50 | + |
| 51 | +```ts |
| 52 | +/** Parse a client config, tolerating an absent file. PARSE_FAILED on garbage. */ |
| 53 | +export function parseConfig(text: string | null, format: ConfigFormat): unknown | typeof PARSE_FAILED; |
| 54 | + |
| 55 | +/** Insert/replace ONLY our key at spec.ownership.path. Everything else is preserved. */ |
| 56 | +export function mergeOurBlock(doc: unknown, spec: IntegrationClientSpec, block: unknown): unknown; |
| 57 | + |
| 58 | +/** Remove ONLY our key. Returns { doc, removed } — removed:false means it was not there. */ |
| 59 | +export function removeOurBlock(doc: unknown, spec: IntegrationClientSpec): { doc: unknown; removed: boolean }; |
| 60 | +``` |
| 61 | + |
| 62 | +Format realities this must respect (from 002 and the WP1 serializers): |
| 63 | + |
| 64 | +- **JSON/YAML/JSON5**: parse → mutate the one key → re-serialize. Comments and |
| 65 | + key order are lost for YAML/JSON5. That is the same bar Kimi's own CLI sets |
| 66 | + for TOML (002 §Kimi: `smol-toml` rewrites the whole document, losing |
| 67 | + comments), so it is defensible — but it must be **stated in the API |
| 68 | + response** so the GUI can warn before the first apply. |
| 69 | +- **TOML (kimi)**: we do NOT round-trip. `Bun.TOML.parse` reads it, and the |
| 70 | + emitted document is rendered by `renderToml` (WP1). Same comment-loss |
| 71 | + caveat, same disclosure. |
| 72 | +- **Empty/missing file**: merge onto `{}` and create the parent directory |
| 73 | + (`mkdirSync(dirname(path), { recursive: true, mode: 0o700 })`) — because |
| 74 | + `atomicWriteFile` does not create parents (005 §3). |
| 75 | + |
| 76 | +**Activation scenarios:** merge onto a config carrying an unrelated provider → |
| 77 | +that provider survives byte-for-byte in the re-serialized output (assert by |
| 78 | +parsing both sides). Remove when our key is absent → `removed: false` and the |
| 79 | +file is not written at all (assert mtime unchanged). |
| 80 | + |
| 81 | +## 3. `applyIntegration` |
| 82 | + |
| 83 | +```ts |
| 84 | +export function applyIntegration(clientId: IntegrationClientId, ctx: { |
| 85 | + models: readonly ExportModel[]; config: OcxConfig; port: number; env?: NodeJS.ProcessEnv; |
| 86 | +}): WriteOutcome; |
| 87 | +``` |
| 88 | + |
| 89 | +Sequence: |
| 90 | + |
| 91 | +1. **Detect.** `detectDir` missing → refuse `not-installed` (no write, no |
| 92 | + journal row). Installing a client for the user is not our business. |
| 93 | +2. **Loopback gate.** `spec.loopbackOnly && !isLoopbackHostname(config.hostname)` |
| 94 | + → refuse `non-loopback`. This is the Grok reasoning applied to Kimi: the |
| 95 | + only way to make it work remotely is to serialize the user's real key, and |
| 96 | + AGENTS.md calls that a release blocker. |
| 97 | +3. **Classify** (WP2). `unsafe` → refuse `unsafe`. `conflict` → refuse |
| 98 | + `conflict` (the switch is locked in the UI; the API must agree). |
| 99 | + `current` → `{ ok: true, changed: false }` — apply is idempotent. |
| 100 | +4. **Build + merge.** `buildClientConfig` (WP1) → `mergeOurBlock` → serialize. |
| 101 | +5. **Snapshot first.** `captureSnapshot(clientId, opId, currentText)` before |
| 102 | + any write. A missing file records `snapshot: null`. |
| 103 | +6. **Compare-before-commit.** Re-read the file and verify its fingerprint |
| 104 | + still equals what step 3 classified. Mismatch → refuse `conflict` and |
| 105 | + delete the just-captured snapshot (nothing happened, so leave no debris). |
| 106 | + This is the lost-update guard 003 §3 caveat 1 demands; the residual race |
| 107 | + inside the re-read/rename window is accepted and documented, not claimed away. |
| 108 | +7. **Write** via `atomicWriteFile`. Throw → refuse `write-failed` with |
| 109 | + `snapshotPath` set. |
| 110 | +8. **Record + journal.** Write the `OwnershipRecord` (both fingerprints) and |
| 111 | + append the journal entry with `resultFingerprint`. |
| 112 | + |
| 113 | +## 4. `disableIntegration` |
| 114 | + |
| 115 | +```ts |
| 116 | +export function disableIntegration(clientId: IntegrationClientId, ctx: {...}): WriteOutcome; |
| 117 | +``` |
| 118 | + |
| 119 | +Same preflight, then `removeOurBlock`. Hard rules: |
| 120 | + |
| 121 | +- Allowed **only** from `current` or `stale` — i.e. only while the file |
| 122 | + fingerprint still matches our record (004 §6.2, 003 §3). `conflict` and |
| 123 | + `unsafe` refuse. |
| 124 | +- `absent` → `{ ok: true, changed: false }`; disabling nothing is a no-op, not |
| 125 | + an error. |
| 126 | +- Removal is surgical: the rest of the document is preserved. After removal |
| 127 | + the ownership record for that client is deleted (a record without a block is |
| 128 | + bookkeeping debris that would later read as `conflict`). |
| 129 | + |
| 130 | +`해제` IS this operation — there is no separate "remove block" action |
| 131 | +(004 §6, four-verb vocabulary). |
| 132 | + |
| 133 | +## 5. `restoreIntegration` — the preflight that makes rollback honest |
| 134 | + |
| 135 | +```ts |
| 136 | +export function restoreIntegration(opId: string, opts: { confirmDrift?: boolean }): WriteOutcome; |
| 137 | +``` |
| 138 | + |
| 139 | +1. **Resolve snapshot.** Missing/GC'd → refuse `snapshot-expired`. |
| 140 | +2. **Target sanity.** Not a regular writable file → refuse `unsafe` with the |
| 141 | + snapshot path named, so the user can copy it back by hand. |
| 142 | +3. **Snapshot the current file first.** Restore is itself journaled as an |
| 143 | + operation with its own snapshot, so a restore can be undone. Rollback that |
| 144 | + cannot be rolled back is a trap. |
| 145 | +4. **Drift check.** If the current file's fingerprint differs from the |
| 146 | + `resultFingerprint` of the operation being undone, someone edited it after |
| 147 | + us. Without `confirmDrift` → refuse `drift-needs-confirm`. With it → |
| 148 | + proceed, having already preserved those edits in step 3's snapshot. |
| 149 | +5. **Write** the snapshot text via `atomicWriteFile`, then journal |
| 150 | + (`kind: "restore"`) and recompute the ownership record from the restored |
| 151 | + content (the restored file may or may not contain our block — classify and |
| 152 | + store accordingly, or delete the record when it does not). |
| 153 | + |
| 154 | +**Activation scenarios (every branch):** |
| 155 | + |
| 156 | +| Branch | Trigger | Observable proof | |
| 157 | +|---|---|---| |
| 158 | +| `not-installed` | apply with no `detectDir` | `refused === "not-installed"`, no journal row | |
| 159 | +| `non-loopback` | apply kimi with `hostname: "0.0.0.0"` | `refused === "non-loopback"`, file unchanged | |
| 160 | +| `conflict` (apply) | apply, append a comment, apply again | refused; file still has the user's comment | |
| 161 | +| idempotent apply | apply twice unchanged | second returns `changed: false`, mtime unchanged | |
| 162 | +| compare-before-commit | stub the re-read to return different bytes | refused `conflict`; snapshot dir has no orphan | |
| 163 | +| `write-failed` | inject a throwing writer (desktop-3p test precedent) | `refused === "write-failed"`, `snapshotPath` set | |
| 164 | +| disable from `conflict` | apply, hand-edit, disable | refused; our block still present (no auto-delete) | |
| 165 | +| disable `absent` | disable on a clean config | `ok: true, changed: false` | |
| 166 | +| `snapshot-expired` | 11 ops then restore the oldest | `refused === "snapshot-expired"` | |
| 167 | +| `drift-needs-confirm` | apply, hand-edit, restore without confirm | refused; then with `confirmDrift` it succeeds AND the hand edit is recoverable from the newest snapshot | |
| 168 | +| restore onto a directory | point config path at a dir | `refused === "unsafe"`, message names the snapshot path | |
| 169 | + |
| 170 | +## 6. Tests — `tests/integrations-writer.test.ts` |
| 171 | + |
| 172 | +One `mkdtempSync` home per test with `rmSync` cleanup (the |
| 173 | +`tests/grok-config-inject.test.ts` shape). Beyond the activation table: |
| 174 | + |
| 175 | +- **No secret ever reaches disk**: apply every client with a config carrying a |
| 176 | + real-looking admission key; assert the written file contains neither the key |
| 177 | + nor `sk-`, and that kimi's file contains the loopback placeholder. |
| 178 | +- **Unrelated content survives**: seed each client's config with a foreign |
| 179 | + provider + a top-level unknown field; after apply and after disable, both |
| 180 | + are still present and parse identically. |
| 181 | +- **Round-trip parse**: every written file parses with its format's parser |
| 182 | + (`Bun.TOML.parse` for kimi — the same proof `grok-config-inject.test.ts` uses). |
| 183 | +- **Undo path end-to-end**: apply → restore(latest op) → the file equals its |
| 184 | + pre-apply bytes exactly. |
| 185 | + |
| 186 | +## 7. Accept criteria |
| 187 | + |
| 188 | +1. `bun run typecheck` clean. |
| 189 | +2. `bun test tests/integrations-writer.test.ts` green, plus WP1/WP2 suites |
| 190 | + still green. |
| 191 | +3. Every row of the §5 activation table has a test that triggers it. |
| 192 | +4. No test leaves a temp directory behind (teardown receipts). |
| 193 | +5. Grep proof: `src/integrations/writer.ts` writes only through |
| 194 | + `atomicWriteFile`; no `writeFileSync` on a client path. |
| 195 | +6. `bun run privacy:scan` clean. |
| 196 | + |
| 197 | +## OPEN QUESTIONS |
| 198 | + |
| 199 | +- **Vendor-CLI delegation** (003 §5 Option B/C) — `openclaw config set/unset`, |
| 200 | + `kimi provider add/remove`, `gjc setup provider` — is deliberately deferred. |
| 201 | + File writing is uniform and testable without a client binary on PATH; the |
| 202 | + CLI path buys cascade cleanup and comment preservation but adds a version |
| 203 | + dependency we cannot verify in CI. Revisit at WP7 with evidence. |
| 204 | +- **Comment loss disclosure**: the API response should carry a |
| 205 | + `formatCaveat: "comments-not-preserved"` flag for YAML/JSON5/TOML clients so |
| 206 | + the GUI can warn before the first apply. Exact field name is WP4's to fix. |
| 207 | +- **Concurrency across processes**: single-flight in WP4 covers one server |
| 208 | + process; a user editing the file in an editor at the same moment is caught |
| 209 | + by compare-before-commit, but two opencodex instances writing the same |
| 210 | + client config simultaneously is out of scope and should be documented. |
0 commit comments