fix(core): address round-2 review on declarative agents PR #4842

LaZzyMan · LaZzyMan · commit 98036e05398a · 2026-06-09T13:56:25.000+08:00
Two findings: 1. `serializeSubagent` wrote both `permissionMode` and the bridge-derived `approvalMode` into the same frontmatter block. On the next load the parser takes `approvalMode` (explicit wins over bridge), so `permissionMode` silently became dead frontmatter — a user editing it later in the file would be ignored. Skip the `permissionMode` emit when `approvalMode` is also being written; `permissionMode` still round-trips when it's the user's only intent. Two new tests pin both branches. 2. `subagents/index.ts` had a NOTE comment referencing `EFFORT_VALUES` and `parseEffort` as example schema helpers that intentionally aren't re-exported. Those symbols were removed during the v1 scope shrink (637b8b7) and don't exist in the current module. Updated the comment to reference `claudePermissionModeToApprovalMode`, `parseMaxTurns`, and `isPermissionMode`, which are the actual current contents. Sibling-drift note: the same dual-emit pattern exists for `maxTurns` vs `runConfig.max_turns` — round-X revert (70a876d) explicitly deferred the `runConfig.max_turns` prune. Not addressed here per that earlier decision. Refs #4842
diff --git a/packages/core/src/subagents/index.ts b/packages/core/src/subagents/index.ts
@@ -37,11 +37,12 @@ export {
 // Validation system
 export { SubagentValidator } from './validation.js';
 
-// NOTE: declarative-agent schema helpers (EFFORT_VALUES, parseEffort,
-// claudePermissionModeToApprovalMode, …) live in `agent-frontmatter-schema.ts`
-// and are intentionally NOT re-exported here — they are internal to the
-// `SubagentManager` / `claude-converter` parse paths and locking their names
-// in the package's public API would constrain follow-up PRs (e.g. when
+// NOTE: declarative-agent schema helpers (e.g.
+// claudePermissionModeToApprovalMode, parseMaxTurns, isPermissionMode)
+// live in `agent-frontmatter-schema.ts` and are intentionally NOT
+// re-exported here — they are internal to the `SubagentManager` /
+// `claude-converter` parse paths and locking their names in the
+// package's public API would constrain follow-up PRs (e.g. when
 // `js-yaml` lands and the schema shape changes). Re-introduce specific
 // exports here when a cross-package caller actually needs them.
 
diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts
@@ -839,6 +839,29 @@ You are an agent.
       expect(serialized).toContain('maxTurns: 25');
     });
 
+    it('should NOT emit permissionMode when approvalMode is also being emitted (avoid round-trip drift)', () => {
+      // Regression for PR #4842 round-2 review: if both fields land on the
+      // serialised frontmatter, the next parse takes approvalMode (explicit
+      // wins over bridge) and silently ignores any user edits to
+      // permissionMode in the file.
+      const serialized = manager.serializeSubagent({
+        ...validConfig,
+        permissionMode: 'bypassPermissions',
+        approvalMode: 'yolo',
+      });
+      expect(serialized).toContain('approvalMode: yolo');
+      expect(serialized).not.toContain('permissionMode:');
+    });
+
+    it('should still emit permissionMode when approvalMode is unset (faithful round-trip of the user intent)', () => {
+      const serialized = manager.serializeSubagent({
+        ...validConfig,
+        permissionMode: 'plan',
+      });
+      expect(serialized).toContain('permissionMode: plan');
+      expect(serialized).not.toContain('approvalMode:');
+    });
+
     it('should not include new fields when undefined', () => {
       const serialized = manager.serializeSubagent(validConfig);
       expect(serialized).not.toContain('permissionMode:');
diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts
@@ -620,7 +620,11 @@ export class SubagentManager {
     }
 
     // CC 2.1.168 declarative-agent fields (round-trip parity).
-    if (config.permissionMode) {
+    // Skip permissionMode when approvalMode is already being written: on the
+    // next load the parser takes approvalMode (explicit wins over bridge),
+    // making permissionMode dead frontmatter that silently ignores any
+    // later user edits.
+    if (config.permissionMode && frontmatter['approvalMode'] === undefined) {
       frontmatter['permissionMode'] = config.permissionMode;
     }
 

Original file line number	Diff line number	Diff line change
`@@ -620,7 +620,11 @@ export class SubagentManager {`
`620`	`620`	`}`
`621`	`621`
`622`	`622`	`// CC 2.1.168 declarative-agent fields (round-trip parity).`
`623`		`- if (config.permissionMode) {`
	`623`	`+ // Skip permissionMode when approvalMode is already being written: on the`
	`624`	`+ // next load the parser takes approvalMode (explicit wins over bridge),`
	`625`	`+ // making permissionMode dead frontmatter that silently ignores any`
	`626`	`+ // later user edits.`
	`627`	`+ if (config.permissionMode && frontmatter['approvalMode'] === undefined) {`
`624`	`628`	`frontmatter['permissionMode'] = config.permissionMode;`
`625`	`629`	`}`
`626`	`630`