fix(devin): preserve JSONC comments when writing hook config (#14199)

Devin documents config.json as JSONC. Installing hooks parsed it with
jsonc-parser and then reserialized with JSON.stringify, silently dropping
the user's comments, key order, and formatting on every install.

Edit the original text with modify/applyEdits one hook event at a time so
untouched entries keep their attached comments, and let both writers accept
pre-serialized text so the shared atomic write and rolling backup are reused.

The two existing tests asserted with JSON.parse, which could only pass once
the comment had been stripped; both now parse as JSONC and assert the
comment survives.

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<void> {
   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 {

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
 

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<string, { matcher?: string; hooks?: { command: string }[] }[]>
     }

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

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', () => {

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()
   }