← Back to PRs

#17887: refactor: consolidate atomic JSON writes into shared writeJsonAtomic

by iyoda open 2026-02-16 08:32 View on GitHub →
stale size: S
## Summary - Problem: Atomic write pattern (write-to-tmp + rename) duplicated across pairing-store, delivery-queue, and infra/json-files with minor variations. - Why it matters: Each copy handles edge cases (chmod, directory creation) slightly differently; consolidating reduces maintenance surface and ensures consistent behavior. - What changed: Enhanced `writeJsonAtomic` with a `dirMode` option; replaced local `writeJsonFile` in `pairing-store.ts` and inline atomic writes in `delivery-queue.ts` with the shared helper. - What did NOT change (scope boundary): `config/io.ts` and `sessions/store.ts` have specialized atomic write logic (backup rotation, audit, Windows fallback, ENOENT recovery) and are intentionally left untouched. ## Change Type (select all) - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Docs - [ ] Security hardening - [ ] Chore/infra ## Scope (select all touched areas) - [ ] Gateway / orchestration - [ ] Skills / tool execution - [ ] Auth / tokens - [x] Memory / storage - [ ] Integrations - [ ] API / contracts - [ ] UI / DX - [ ] CI/CD / infra ## Linked Issue/PR - Related #17770 #17793 #17865 #17878 (cross-channel dedup refactor series) ## User-visible / Behavior Changes None ## Security Impact (required) - New permissions/capabilities? `No` - Secrets/tokens handling changed? `No` - New/changed network calls? `No` - Command/tool execution surface changed? `No` - Data access scope changed? `No` ## Repro + Verification ### Environment - OS: macOS (darwin) - Runtime/container: Node 22+ / Bun - Model/provider: N/A (refactor) - Integration/channel (if any): N/A ### Steps 1. `corepack pnpm tsgo` — type check passes 2. `corepack pnpm build` — build passes 3. `corepack pnpm test -- src/infra/json-files.test.ts src/pairing/pairing-store.test.ts src/infra/outbound/outbound.test.ts` — 76 tests pass ### Expected - All tests pass, no behavior changes ### Actual - 3 test files, 76 tests, all pass ## Evidence - [x] Failing test/log before + passing after - [x] Trace/log snippets ``` ✓ src/infra/json-files.test.ts (8 tests) 11ms ✓ src/pairing/pairing-store.test.ts (7 tests) 31ms ✓ src/infra/outbound/outbound.test.ts (61 tests) 29ms Test Files 3 passed (3) Tests 76 passed (76) ``` ## Human Verification (required) - Verified scenarios: Type check, build, unit tests for all 3 changed modules - Edge cases checked: dirMode option propagation, overwrite behavior, no leftover tmp files (all covered by new json-files tests) - What you did **not** verify: Windows-specific behavior (not applicable to this refactor scope) ## Compatibility / Migration - Backward compatible? `Yes` - Config/env changes? `No` - Migration needed? `No` ## Failure Recovery (if this breaks) - How to disable/revert this change quickly: Revert this PR (3 commits) - Files/config to restore: `src/infra/json-files.ts`, `src/pairing/pairing-store.ts`, `src/infra/outbound/delivery-queue.ts` - Known bad symptoms reviewers should watch for: Pairing store write failures, delivery queue persistence failures ## Risks and Mitigations - Risk: `dirMode` option could change directory permissions on existing directories - Mitigation: `fs.mkdir({ recursive: true })` with `mode` only applies to newly created directories; existing directories are unaffected - Risk: Dropping trailing newline from pairing-store JSON output - Mitigation: JSON parsers are newline-agnostic; no consumer relies on trailing newline <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR consolidates duplicated atomic JSON write patterns across `pairing-store.ts`, `delivery-queue.ts`, and the shared `json-files.ts` into a single `writeJsonAtomic` helper. The enhanced helper gains a `dirMode` option so that credential directories can retain their restrictive `0o700` permissions while other callers use the default. - Enhanced `writeJsonAtomic` in `src/infra/json-files.ts` with a `dirMode` option that is only applied when explicitly provided - Replaced inline atomic write logic in `delivery-queue.ts` (`enqueueDelivery`, `failDelivery`) with the shared helper, matching the previous `0o600` file mode default - Replaced the local `writeJsonFile` in `pairing-store.ts` (previously delegating to `plugin-sdk/json-store.ts`) with `writeJsonAtomic`, passing `dirMode: 0o700` to preserve credential directory permissions - Inlined `readJsonFile` implementation in `pairing-store.ts` using `safeParseJson` directly, removing the dependency on `plugin-sdk/json-store.ts` - Added comprehensive test coverage for `writeJsonAtomic` and `readJsonFile` in a new test file with POSIX-specific mode assertions correctly skipped on Windows <h3>Confidence Score: 5/5</h3> - This PR is safe to merge — it is a clean refactor that preserves existing behavior with no functional changes. - The changes are purely mechanical: replacing duplicated atomic-write patterns with calls to a shared utility. File permissions (0o600 for files, 0o700 for credential directories) are preserved exactly. The inlined `readJsonFile` is functionally identical to the previous `readJsonFileWithFallback`. New tests cover all key scenarios. No new code paths, no behavioral changes, no security surface changes. - No files require special attention. <sub>Last reviewed commit: 23d8a76</sub> <!-- greptile_other_comments_section --> <!-- /greptile_comment -->

Most Similar PRs