From 66786ebc91f226b2b14d3599c23669340b3c4c08 Mon Sep 17 00:00:00 2001 From: cvince Date: Sat, 6 Jun 2026 02:19:36 -0700 Subject: [PATCH] fix(sync): stop rewriting keep.lock on every sync via canonical self-heal compare MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bare `capy` sync rewrote keep.lock on every run — even with an empty .env. The self-heal step compares the local keep.lock against the server's keep_file and overwrites on any difference. But the local copy is read off disk (so it carries the injected `_comment` and is already sorted) while the server's copy has neither, so the bare JSON.stringify comparison ALWAYS reported a difference and rewrote the file. - Extract the canonical lockfile serialization into `serializeKeep()` (the single source of truth for on-disk form) and have the self-heal comparison normalize both sides through it. - Give `writeKeepFile()` a no-op guard so it skips the write when the on-disk file is already byte-identical to the canonical output. - Add regression tests covering the canonical comparison and the no-op guard. --- src/commands/capyCommand.ts | 10 ++- src/files/fileManager.ts | 92 ++++++++++++++++++--------- tests/files/fileManager.test.ts | 108 +++++++++++++++++++++++++++++++- 3 files changed, 177 insertions(+), 33 deletions(-) diff --git a/src/commands/capyCommand.ts b/src/commands/capyCommand.ts index 310c1c1..3ce9ace 100644 --- a/src/commands/capyCommand.ts +++ b/src/commands/capyCommand.ts @@ -1,6 +1,6 @@ import ora from '../ui/spinner'; import { ProjectManager } from '../core/projectManager'; -import { FileManager } from '../files/fileManager'; +import { FileManager, serializeKeep } from '../files/fileManager'; import { AuthService } from '../auth/authService'; import { ServiceClient } from '../service/serviceClient'; import { SyncEngine } from '../sync/syncEngine'; @@ -1173,8 +1173,12 @@ export class CapyCommand { // so that "Pinned vs Local vs Remote" is a true three-way comparison. if (decryptData.keep_file) { const serverKeep = JSON.parse(decryptData.keep_file) as KeepFile; - const localSerialized = currentKeep ? JSON.stringify(currentKeep) : ''; - const serverSerialized = JSON.stringify(serverKeep); + // Compare in the canonical on-disk form. A keep read from disk carries + // `_comment` and is already sorted; the server's copy has neither, so a + // bare JSON.stringify comparison always reports a difference and would + // rewrite keep.lock on every sync. serializeKeep normalizes both sides. + const localSerialized = currentKeep ? serializeKeep(currentKeep) : ''; + const serverSerialized = serializeKeep(serverKeep); if (localSerialized !== serverSerialized) { this.debug('self-heal: local keep.lock differs from server, overwriting'); this.fileManager.writeKeepFile(serverKeep); diff --git a/src/files/fileManager.ts b/src/files/fileManager.ts index 77e2c48..a275b6d 100644 --- a/src/files/fileManager.ts +++ b/src/files/fileManager.ts @@ -6,6 +6,52 @@ import { Encryptor } from '../crypto/encryptor'; import { deriveResourceId } from '../crypto/resourceId'; import { debug } from '../ui/debug'; +const KEEP_COMMENT = 'Capy lockfile — pins env var identities and value hashes per branch. Safe to commit; no secrets here. Plaintext lives in .env (gitignored). Regenerated by the Capy CLI on sync/push. See https://capy.sc/keep-docs'; + +/** + * Canonical, byte-for-byte stable serialization of a keep.lock. + * + * This is the SINGLE source of truth for what a keep.lock looks like on disk: + * fixed top-level key order, an injected `_comment`, alphabetically sorted + * variables, and a stable field order per entry. `writeKeepFile` writes exactly + * this, and any code that needs to decide "did the keep change?" MUST compare + * `serializeKeep(a) === serializeKeep(b)` rather than bare `JSON.stringify`. + * + * Why this matters: a keep read off disk carries `_comment` and is already + * sorted, while a keep handed back by the server has neither. Comparing them + * with `JSON.stringify` always reports a difference, which previously made sync + * self-heal rewrite keep.lock on every run even when nothing had changed. + */ +export function serializeKeep(keep: KeepFile): string { + const sorted: Record = { + _comment: KEEP_COMMENT, + version: keep.version, + org_id: keep.org_id, + project_id: keep.project_id, + project_name: keep.project_name, + variables: {} as Record, + }; + // v3 format: variables are arrays of { resource_id, branch?, value_hash, ...extras }. + // Build a stable key order, then attach any other fields the entry carries + // (e.g. `connector` metadata set by `capy connect`). Without this, every + // sync would strip extras off the entry. + for (const key of Object.keys(keep.variables).sort()) { + const entries = keep.variables[key]; + sorted.variables[key] = entries.map(e => { + const obj: Record = { resource_id: e.resource_id }; + if (e.branch) obj.branch = e.branch; + obj.value_hash = e.value_hash; + for (const [k, v] of Object.entries(e)) { + if (k === 'resource_id' || k === 'branch' || k === 'value_hash') continue; + if (v === undefined) continue; + obj[k] = v; + } + return obj; + }); + } + return JSON.stringify(sorted, null, 2) + '\n'; +} + export class FileManager { private projectRoot: string; @@ -160,38 +206,26 @@ export class FileManager { writeKeepFile(keep: KeepFile): void { const keepPath = join(this.projectRoot, 'keep.lock'); + const content = serializeKeep(keep); + + // No-op guard: if the on-disk lockfile is already byte-identical to the + // canonical output, don't touch it. This keeps `capy` from churning + // keep.lock (and producing a spurious git diff) on every sync when nothing + // actually changed. + if (existsSync(keepPath)) { + try { + if (readFileSync(keepPath, 'utf-8') === content) { + return; + } + } catch { + // Unreadable existing file — fall through and rewrite it. + } + } + const backup = this.createBackup(keepPath); try { - // Deterministic output: fixed key order, sorted variables - const sorted: Record = { - _comment: 'Capy lockfile — pins env var identities and value hashes per branch. Safe to commit; no secrets here. Plaintext lives in .env (gitignored). Regenerated by the Capy CLI on sync/push. See https://capy.sc/keep-docs', - version: keep.version, - org_id: keep.org_id, - project_id: keep.project_id, - project_name: keep.project_name, - variables: {} as Record, - }; - // v3 format: variables are arrays of { resource_id, branch?, value_hash, ...extras }. - // Build a stable key order, then attach any other fields the entry carries - // (e.g. `connector` metadata set by `capy connect`). Without this, every - // sync would strip extras off the entry. - for (const key of Object.keys(keep.variables).sort()) { - const entries = keep.variables[key]; - sorted.variables[key] = entries.map(e => { - const obj: Record = { resource_id: e.resource_id }; - if (e.branch) obj.branch = e.branch; - obj.value_hash = e.value_hash; - for (const [k, v] of Object.entries(e)) { - if (k === 'resource_id' || k === 'branch' || k === 'value_hash') continue; - if (v === undefined) continue; - obj[k] = v; - } - return obj; - }); - } - const content = JSON.stringify(sorted, null, 2); - writeFileSync(keepPath, content + '\n', 'utf-8'); + writeFileSync(keepPath, content, 'utf-8'); if (backup) { this.removeBackup(backup); diff --git a/tests/files/fileManager.test.ts b/tests/files/fileManager.test.ts index 10f9a55..5a51361 100644 --- a/tests/files/fileManager.test.ts +++ b/tests/files/fileManager.test.ts @@ -20,7 +20,7 @@ mock.module('fs', () => ({ afterAll(() => { mock.restore(); }); import { join, dirname } from 'path'; -import { FileManager } from '../../src/files/fileManager'; +import { FileManager, serializeKeep } from '../../src/files/fileManager'; import { KeepFile, CapyError, ERROR_CODES } from '../../src/types/index'; describe('FileManager', () => { @@ -222,6 +222,112 @@ describe('FileManager', () => { }); }); + describe('serializeKeep canonical comparison (sync self-heal)', () => { + // Regression for the bug where a bare `capy` sync rewrote keep.lock on + // EVERY run — even with an empty .env. During sync, self-heal compares the + // local keep.lock against the server's keep_file. The local copy is read + // off disk (so it carries `_comment` and is already sorted); the server's + // copy has neither. A bare JSON.stringify comparison therefore always saw a + // difference and rewrote the file. serializeKeep normalizes both sides so + // the comparison reflects real changes only. + + test('on-disk keep (with _comment) and server keep (without _comment) serialize identically', () => { + // Shape the server hands back: no _comment, variables in arbitrary order. + const serverKeep = { + version: '3.0', + org_id: 'org_1', + project_id: 'proj_1', + project_name: 'test', + variables: { + B_VAR: [{ resource_id: 'rb', value_hash: 'hb' }], + A_VAR: [{ resource_id: 'ra', value_hash: 'ha' }], + }, + } as KeepFile; + + // Shape on disk: writeKeepFile injects _comment + sorts; readKeepFile + // returns it verbatim (raw `as KeepFile`), so the parsed object still + // carries _comment. + mockExistsSync.mockReturnValue(false); + fileManager.writeKeepFile(serverKeep); + const onDiskContent = mockWriteFileSync.mock.calls[0][1] as string; + const onDiskKeep = JSON.parse(onDiskContent) as KeepFile; + expect((onDiskKeep as any)._comment).toBeDefined(); + + // The fix: self-heal must treat these as equal → no rewrite. + expect(serializeKeep(onDiskKeep)).toBe(serializeKeep(serverKeep)); + + // Documents the root cause: the OLD bare-stringify comparison reported a + // difference for the very same data, triggering the spurious rewrite. + expect(JSON.stringify(onDiskKeep)).not.toBe(JSON.stringify(serverKeep)); + }); + + test('is independent of variable insertion order and extra entry fields', () => { + const a = { + version: '3.0', org_id: 'o', project_id: 'p', project_name: 'n', + variables: { + Z: [{ resource_id: 'rz', value_hash: 'hz', connector: { provider: 'aws' } }], + A: [{ resource_id: 'ra', value_hash: 'ha' }], + }, + } as unknown as KeepFile; + const b = { + version: '3.0', org_id: 'o', project_id: 'p', project_name: 'n', + variables: { + A: [{ resource_id: 'ra', value_hash: 'ha' }], + Z: [{ resource_id: 'rz', connector: { provider: 'aws' }, value_hash: 'hz' }], + }, + } as unknown as KeepFile; + + expect(serializeKeep(a)).toBe(serializeKeep(b)); + }); + + test('reports a real change when a value_hash differs', () => { + const before = { + version: '3.0', org_id: 'o', project_id: 'p', project_name: 'n', + variables: { API_KEY: [{ resource_id: 'r', value_hash: 'old' }] }, + } as KeepFile; + const after = { + version: '3.0', org_id: 'o', project_id: 'p', project_name: 'n', + variables: { API_KEY: [{ resource_id: 'r', value_hash: 'new' }] }, + } as KeepFile; + + expect(serializeKeep(before)).not.toBe(serializeKeep(after)); + }); + }); + + describe('writeKeepFile no-op guard', () => { + const keep: KeepFile = { + version: '3.0', + org_id: 'org_1', + project_id: 'proj_1', + project_name: 'test', + variables: {}, + }; + + test('does not rewrite keep.lock when on-disk content is already canonical', () => { + const keepPath = join(testRoot, 'keep.lock'); + mockExistsSync.mockImplementation((p: string) => p === keepPath); + mockReadFileSync.mockReturnValue(serializeKeep(keep)); + + fileManager.writeKeepFile(keep); + + expect(mockWriteFileSync).not.toHaveBeenCalled(); + }); + + test('rewrites keep.lock when on-disk content differs', () => { + const keepPath = join(testRoot, 'keep.lock'); + mockExistsSync.mockImplementation((p: string) => p === keepPath); + mockReadFileSync.mockReturnValue('stale content'); + + fileManager.writeKeepFile(keep); + + expect(mockWriteFileSync).toHaveBeenCalledWith( + keepPath, + serializeKeep(keep), + 'utf-8', + ); + }); + }); + describe('updateGitignore', () => { test('should add new entries to .gitignore', () => { const entries = ['.env'];