#17887: refactor: consolidate atomic JSON writes into shared writeJsonAtomic
stale
size: S
Cluster:
Config File Security Enhancements
## 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
#17900: refactor(security): extract shared normalizeAllowFromList into audi...
by iyoda · 2026-02-16
77.7%
#17463: fix: write config files with explicit 0o600 mode instead of post-wr...
by miclaldogan · 2026-02-15
76.9%
#17770: refactor(cli): reuse shared option builders
by iyoda · 2026-02-16
76.2%
#16542: fix(sessions): use atomic temp+rename write on Windows
by aldoeliacim · 2026-02-14
75.5%
#22648: fix(plugin-sdk): re-throw non-ENOENT errors in readJsonFileWithFall...
by adhitShet · 2026-02-21
75.1%
#23669: refactor(logging): migrate node-host and tailscale console calls to...
by kevinWangSheng · 2026-02-22
73.4%
#4664: fix: per-session metadata files to eliminate lock contention
by tsukhani · 2026-01-30
72.3%
#23503: fix: preserve pairing state on device token mismatch + migrate lega...
by dorukardahan · 2026-02-22
71.9%
#22383: Chore: apply oxfmt baseline for CI check
by bmendonca3 · 2026-02-21
71.5%
#17897: refactor: extract firstDefined utility from telegram/line/slack
by iyoda · 2026-02-16
71.4%