diff --git a/src/main/agent-hooks/installer-utils-remote.ts b/src/main/agent-hooks/installer-utils-remote.ts index b4518220e90..7c321be5515 100644 --- a/src/main/agent-hooks/installer-utils-remote.ts +++ b/src/main/agent-hooks/installer-utils-remote.ts @@ -48,11 +48,13 @@ export async function readHooksJsonRemote( export async function writeHooksJsonRemote( sftp: SFTPWrapper, remotePath: string, - config: HooksConfig + config: HooksConfig, + // Why: mirrors the local writer — a JSONC config supplies text edited in place. + options?: { serialized?: string } ): Promise { const dir = dirnamePosix(remotePath) await mkdirpRemote(sftp, dir) - const serialized = `${JSON.stringify(config, null, 2)}\n` + const serialized = options?.serialized ?? `${JSON.stringify(config, null, 2)}\n` // Why: skip the write when on-disk content is identical so repeated // install() calls do not bump the file's mtime / inode unnecessarily. try { diff --git a/src/main/agent-hooks/installer-utils.ts b/src/main/agent-hooks/installer-utils.ts index f6e4c131826..8861d41f0cc 100644 --- a/src/main/agent-hooks/installer-utils.ts +++ b/src/main/agent-hooks/installer-utils.ts @@ -316,7 +316,9 @@ function writeScriptWithAclRetry(scriptPath: string, content: string): void { export function writeHooksJson( configPath: string, config: HooksConfig, - options?: { preserveMode?: boolean } + // Why: `serialized` lets a JSONC config (Devin) supply text edited in place, so the + // atomic write + rolling backup below stay shared instead of being reimplemented. + options?: { preserveMode?: boolean; serialized?: string } ): void { const writePath = resolveHooksJsonWritePath(configPath) const dir = dirname(writePath) @@ -325,7 +327,7 @@ export function writeHooksJson( // Why: temp+rename leaves the original untouched on a crash/disk-full mid-write. // Why randomUUID: avoids tmp-path collisions when two install() calls fire in the same millisecond. const tmpPath = join(dir, `.${Date.now()}-${randomUUID()}.tmp`) - const serialized = `${JSON.stringify(config, null, 2)}\n` + const serialized = options?.serialized ?? `${JSON.stringify(config, null, 2)}\n` const existingMode = options?.preserveMode === true && existsSync(writePath) ? statSync(writePath).mode : undefined diff --git a/src/main/agent-hooks/remote-hook-service-installers.test.ts b/src/main/agent-hooks/remote-hook-service-installers.test.ts index 9ba01bc81c6..1627821d5b1 100644 --- a/src/main/agent-hooks/remote-hook-service-installers.test.ts +++ b/src/main/agent-hooks/remote-hook-service-installers.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, vi } from 'vitest' +import { parse as parseJsonc } from 'jsonc-parser' import type { SFTPWrapper } from 'ssh2' vi.mock('electron', () => ({ @@ -415,7 +416,11 @@ describe('remote hook service installers', () => { expect(grokConfig.hooks.PostToolUse?.[0]?.matcher).toBe('.*') expect(grokConfig.hooks.StopFailure?.[0]?.matcher).toBeUndefined() - const devinConfig = JSON.parse(devin.fs.files.get('/home/dev/.config/devin/config.json')!) as { + const devinText = devin.fs.files.get('/home/dev/.config/devin/config.json')! + // Why: Devin config.json is JSONC — parse it as such, and assert the user's comment + // survived. Asserting with JSON.parse would only pass if the install had stripped it. + expect(devinText).toContain('// Existing Devin config comment') + const devinConfig = parseJsonc(devinText) as { permissions: { mode: string } hooks: Record } diff --git a/src/main/devin/hook-config-json.ts b/src/main/devin/hook-config-json.ts index 6a316ebc6aa..8c73c4b916a 100644 --- a/src/main/devin/hook-config-json.ts +++ b/src/main/devin/hook-config-json.ts @@ -1,5 +1,5 @@ import { existsSync, readFileSync } from 'node:fs' -import { parse as parseJsonc, type ParseError } from 'jsonc-parser' +import { applyEdits, modify, parse as parseJsonc, type ParseError } from 'jsonc-parser' import { isPlainObject, type HooksConfig } from '../agent-hooks/installer-utils' /** Devin documents config.json as JSONC; stock JSON.parse rejects comments. */ @@ -16,6 +16,60 @@ export function readDevinHooksConfig(configPath: string): HooksConfig | null { } } +/** Original file text alongside its parsed form, so a write can edit the text in place. */ +export function readDevinHooksSource( + configPath: string +): { text: string | null; config: HooksConfig } | null { + if (!existsSync(configPath)) { + return { text: null, config: {} } + } + + let text: string + try { + text = readFileSync(configPath, 'utf-8') + } catch { + return null + } + const config = parseDevinHooksConfigText(text, 'Devin config.json') + return config === null ? null : { text, config } +} + +/** + * Serialize by editing the original JSONC text one hook event at a time, so the user's + * comments, key order, and formatting survive. A parse -> JSON.stringify round trip would + * silently drop all of them. + */ +export function serializeDevinHooksConfig( + originalText: string | null, + nextConfig: HooksConfig +): string { + if (originalText === null) { + return `${JSON.stringify(nextConfig, null, 2)}\n` + } + + const previous = parseJsonc(originalText) as HooksConfig | undefined + const previousHooks = isPlainObject(previous?.hooks) ? (previous.hooks ?? {}) : {} + const nextHooks = nextConfig.hooks ?? {} + + let text = originalText + // Why: touch only the events that actually changed, so comments attached to a user's + // own untouched hook entries stay put. + for (const eventName of new Set([...Object.keys(previousHooks), ...Object.keys(nextHooks)])) { + const nextValue = nextHooks[eventName] + if (JSON.stringify(previousHooks[eventName]) === JSON.stringify(nextValue)) { + continue + } + text = applyEdits( + text, + // Why: `undefined` removes the key, which is how remove() drops an emptied event. + modify(text, ['hooks', eventName], nextValue, { + formattingOptions: { insertSpaces: true, tabSize: 2 } + }) + ) + } + return text +} + export function parseDevinHooksConfigText( text: string, diagnosticName: string diff --git a/src/main/devin/hook-service.test.ts b/src/main/devin/hook-service.test.ts index 98a83993809..e606d54a19f 100644 --- a/src/main/devin/hook-service.test.ts +++ b/src/main/devin/hook-service.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { parse as parseJsonc } from 'jsonc-parser' import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' @@ -104,7 +105,45 @@ describe('DevinHookService', () => { const status = new DevinHookService().install() expect(status.state).toBe('installed') - expect(JSON.parse(readFileSync(configPath, 'utf8')).hooks.UserPromptSubmit).toBeDefined() + // Why: parse as JSONC, not JSON — asserting with JSON.parse would only pass once the + // comment had been stripped, which is the regression this test exists to catch. + expect(parseJsonc(readFileSync(configPath, 'utf8')).hooks.UserPromptSubmit).toBeDefined() + }) + + it('keeps user comments and unrelated formatting when installing and removing', () => { + const configPath = getDevinConfigPath() + mkdirSync(dirname(configPath), { recursive: true }) + writeFileSync( + configPath, + `{ + // keep me: chosen deliberately + "permissions": { "mode": "normal" }, + /* block comment */ + "hooks": { + // the user's own hook + "SessionEnd": [{ "hooks": [{ "type": "command", "command": "mine.sh" }] }] + } +} +` + ) + + const service = new DevinHookService() + service.install() + + const installed = readFileSync(configPath, 'utf8') + expect(installed).toContain('// keep me: chosen deliberately') + expect(installed).toContain('/* block comment */') + expect(installed).toContain("// the user's own hook") + expect(installed).toContain('mine.sh') + expect(parseJsonc(installed).permissions.mode).toBe('normal') + expect(parseJsonc(installed).hooks.UserPromptSubmit).toBeDefined() + + service.remove() + + const removed = readFileSync(configPath, 'utf8') + expect(removed).toContain('// keep me: chosen deliberately') + expect(removed).toContain('mine.sh') + expect(parseJsonc(removed).hooks.UserPromptSubmit).toBeUndefined() }) it('surfaces read_config_from overlap in status detail', () => { diff --git a/src/main/devin/hook-service.ts b/src/main/devin/hook-service.ts index 6cff0d2b824..6c485b0daf5 100644 --- a/src/main/devin/hook-service.ts +++ b/src/main/devin/hook-service.ts @@ -32,7 +32,9 @@ import { mergeHookInstallDetail, parseDevinHooksConfigText, readConfigFromOrcaOverlapDetail, - readDevinHooksConfig + readDevinHooksConfig, + readDevinHooksSource, + serializeDevinHooksConfig } from './hook-config-json' function getManagedScript(target: 'local' | 'posix' = 'local'): string { @@ -140,8 +142,8 @@ export class DevinHookService { install(): AgentHookInstallStatus { const configPath = getDevinConfigPath() const scriptPath = getDevinManagedScriptPath() - const config = readDevinHooksConfig(configPath) - if (!config) { + const source = readDevinHooksSource(configPath) + if (!source) { return { agent: 'devin', state: 'error', @@ -152,9 +154,15 @@ export class DevinHookService { } const command = getDevinManagedCommand(scriptPath) - const nextConfig = applyDevinManagedHooks(config, command, getDevinManagedScriptFileName()) + const nextConfig = applyDevinManagedHooks( + source.config, + command, + getDevinManagedScriptFileName() + ) writeManagedScript(scriptPath, getManagedScript()) - writeHooksJson(configPath, nextConfig) + writeHooksJson(configPath, nextConfig, { + serialized: serializeDevinHooksConfig(source.text, nextConfig) + }) return this.getStatus() } @@ -187,7 +195,9 @@ export class DevinHookService { // Why: write script before settings so a mid-install failure never leaves settings.json referencing a missing script. // Why: SSH remotes use POSIX `.sh` hooks even when Orca runs on Windows; never derive remote script syntax from local OS. await writeManagedScriptRemote(sftp, remoteScriptPath, getManagedScript('posix')) - await writeHooksJsonRemote(sftp, remoteConfigPath, nextConfig) + await writeHooksJsonRemote(sftp, remoteConfigPath, nextConfig, { + serialized: serializeDevinHooksConfig(body, nextConfig) + }) return { agent: 'devin', @@ -209,8 +219,8 @@ export class DevinHookService { remove(): AgentHookInstallStatus { const configPath = getDevinConfigPath() - const config = readDevinHooksConfig(configPath) - if (!config) { + const source = readDevinHooksSource(configPath) + if (!source) { return { agent: 'devin', state: 'error', @@ -220,11 +230,13 @@ export class DevinHookService { } } const { config: nextConfig, changed } = removeDevinManagedHooks( - config, + source.config, getDevinManagedScriptFileName() ) if (changed) { - writeHooksJson(configPath, nextConfig) + writeHooksJson(configPath, nextConfig, { + serialized: serializeDevinHooksConfig(source.text, nextConfig) + }) } return this.getStatus() }