Browse Source

Keep post-file reminders out of system prompts

DanMaly 2 weeks ago
parent
commit
de8302a67a

+ 1 - 10
src/hooks/phase-reminder/index.ts

@@ -8,7 +8,6 @@
 import { PHASE_REMINDER } from '../../config/constants';
 import { isInternalInitiatorPart } from '../../utils';
 import { isRecord } from '../../utils/guards';
-import type { SessionLifecycle } from '../session-lifecycle';
 import { isUserMessageWithParts } from '../types';
 
 export { PHASE_REMINDER };
@@ -20,7 +19,7 @@ export const PHASE_REMINDER_METADATA_KEY = 'oh-my-opencode-slim.phaseReminder';
  * This hook runs right before sending to API, so it doesn't affect UI display.
  * Only injects for the orchestrator agent.
  */
-export function createPhaseReminderHook(coordinator?: SessionLifecycle) {
+export function createPhaseReminderHook() {
   return {
     'experimental.chat.messages.transform': async (
       _input: Record<string, never>,
@@ -54,14 +53,6 @@ export function createPhaseReminderHook(coordinator?: SessionLifecycle) {
         return;
       }
 
-      // If post-file-tool-nudge is pending for this session, it handles
-      // injection via system prompt — skip message-level injection.
-      const sessionId = (lastUserMessage as { info?: { sessionID?: string } })
-        ?.info?.sessionID;
-      if (sessionId && coordinator?.hasPendingSession(sessionId)) {
-        return;
-      }
-
       const textPartIndex = lastUserMessage.parts.findIndex(
         (p) => p.type === 'text' && p.text !== undefined,
       );

+ 10 - 16
src/hooks/post-file-tool-nudge/codemap.md

@@ -1,25 +1,19 @@
 # src/hooks/post-file-tool-nudge/
 
 ## Responsibility
-Implements a post-tool execution hook that automatically appends delegation reminders to file operation outputs, preventing the "inspect/edit files → implement myself" anti-pattern where agents attempt to implement functionality themselves instead of delegating to specialized tools.
+Implements a post-tool execution hook that queues delegation reminders after file operations and injects them as synthetic message parts for the next eligible orchestrator turn.
 
 ## Design
 
 ### Hook Structure
-- **Factory Pattern**: `createPostFileToolNudgeHook()` returns a hook object with a `tool.execute.after` handler
+- **Factory Pattern**: `createPostFileToolNudgeHook()` returns `tool.execute.after` and `experimental.chat.messages.transform` handlers
 - **Conditional Injection**: Uses `shouldInject` option to filter sessions where the reminder should be applied
 - **Set-based Tool Filtering**: Maintains a Set of file tool names for O(1) lookup
 
 ### Core Logic
-```typescript
-const FILE_TOOLS = new Set(['Read', 'read', 'Write', 'write']);
-
-function appendReminder(output: ToolExecuteAfterOutput): void {
-  if (typeof output.output !== 'string') return;
-  if (output.output.includes(PHASE_REMINDER)) return;
-  output.output = `${output.output}\n\n${PHASE_REMINDER}`;
-}
-```
+- Read/Write records one pending marker per session.
+- The message transform finds the latest matching orchestrator user message with a non-internal text part before consuming that marker.
+- It appends `PHASE_REMINDER` as a synthetic metadata-tagged text part, preserving user-authored text and allowing phase-reminder metadata deduplication.
 
 ### Integration Points
 - **Config Dependency**: Imports `PHASE_REMINDER` constant from `../../config/constants`
@@ -34,10 +28,10 @@ function appendReminder(output: ToolExecuteAfterOutput): void {
    - Verify sessionID exists
    - Apply shouldInject filter if provided
 3. **Reminder Injection**:
-   - Extract output string
-   - Check if PHASE_REMINDER already present (idempotent)
-   - Append PHASE_REMINDER to output
-4. **Result**: Agent receives output with delegation reminder prepended
+   - Find a matching eligible orchestrator user message
+   - Consume the session marker only after validation
+   - Append one synthetic, metadata-tagged reminder part
+4. **Result**: The API receives the reminder without mutating tool output or user-authored text
 
 ## Integration
 
@@ -65,4 +59,4 @@ This hook addresses the common failure mode where agents:
 - Attempt to implement changes themselves instead of delegating to specialized tools
 - Violate the delegation principle of the OpenCode architecture
 
-The reminder reinforces the expected workflow: inspect → delegate → implement via specialized agents.
+The reminder reinforces the expected workflow: inspect → delegate → implement via specialized agents.

+ 161 - 120
src/hooks/post-file-tool-nudge/index.test.ts

@@ -1,200 +1,241 @@
 import { describe, expect, test } from 'bun:test';
 
 import { PHASE_REMINDER } from '../../config/constants';
+import { createInternalAgentTextPart } from '../../utils';
+import {
+  createPhaseReminderHook,
+  PHASE_REMINDER_METADATA_KEY,
+} from '../phase-reminder';
 import { SessionLifecycle } from '../session-lifecycle';
 import { createPostFileToolNudgeHook } from './index';
 
+const orchestratorMessage = (sessionID = 's1') => ({
+  info: { role: 'user', agent: 'orchestrator', sessionID },
+  parts: [{ type: 'text', text: 'hello' }],
+});
+
+const reminderParts = (message: ReturnType<typeof orchestratorMessage>) =>
+  message.parts.filter((part) => part.text === PHASE_REMINDER);
+
 describe('post-file-tool-nudge hook', () => {
-  test('records pending session on Read tool', async () => {
+  test('injects a synthetic reminder without a system transform or text mutation', async () => {
     const coordinator = new SessionLifecycle(() => {});
     const hook = createPostFileToolNudgeHook({ coordinator });
-    const output = { system: [] };
+    const message = orchestratorMessage();
 
+    expect(hook['experimental.chat.system.transform']).toBeUndefined();
     await hook['tool.execute.after']({ tool: 'Read', sessionID: 's1' }, {});
-    await hook['experimental.chat.system.transform'](
-      { sessionID: 's1' },
-      output,
+    await hook['experimental.chat.messages.transform'](
+      {},
+      { messages: [message] },
     );
 
-    expect(output.system).toContain(PHASE_REMINDER);
+    expect(message.parts[0].text).toBe('hello');
+    expect(reminderParts(message)).toHaveLength(1);
+    expect(message.parts[1]).toMatchObject({
+      synthetic: true,
+      metadata: { [PHASE_REMINDER_METADATA_KEY]: true },
+    });
   });
 
-  test('records pending session on Write tool', async () => {
+  test('composes with phase reminder without duplication and permits a fresh reminder', async () => {
     const coordinator = new SessionLifecycle(() => {});
-    const hook = createPostFileToolNudgeHook({ coordinator });
-    const output = { system: [] };
+    const nudge = createPostFileToolNudgeHook({ coordinator });
+    const phaseReminder = createPhaseReminderHook();
+    const afterFileMessage = orchestratorMessage();
 
-    await hook['tool.execute.after']({ tool: 'Write', sessionID: 's1' }, {});
-    await hook['experimental.chat.system.transform'](
-      { sessionID: 's1' },
-      output,
+    await nudge['tool.execute.after']({ tool: 'Write', sessionID: 's1' }, {});
+    await nudge['experimental.chat.messages.transform'](
+      {},
+      { messages: [afterFileMessage] },
     );
-
-    expect(output.system).toContain(PHASE_REMINDER);
-  });
-
-  test('does not mutate tool output', async () => {
-    const coordinator = new SessionLifecycle(() => {});
-    const hook = createPostFileToolNudgeHook({ coordinator });
-    const toolOutput = { output: 'real content' };
-
-    await hook['tool.execute.after'](
-      { tool: 'Read', sessionID: 's1' },
-      toolOutput,
+    await phaseReminder['experimental.chat.messages.transform'](
+      {},
+      { messages: [afterFileMessage] },
     );
+    expect(reminderParts(afterFileMessage)).toHaveLength(1);
 
-    expect(toolOutput.output).toBe('real content');
+    const freshMessage = orchestratorMessage();
+    await phaseReminder['experimental.chat.messages.transform'](
+      {},
+      { messages: [freshMessage] },
+    );
+    expect(reminderParts(freshMessage)).toHaveLength(1);
   });
 
-  test('deduplicates multiple Read/Write calls in same session', async () => {
+  test('collapses multiple Read and Write calls into one reminder', async () => {
     const coordinator = new SessionLifecycle(() => {});
     const hook = createPostFileToolNudgeHook({ coordinator });
+    const message = orchestratorMessage();
 
     await hook['tool.execute.after']({ tool: 'read', sessionID: 's1' }, {});
     await hook['tool.execute.after']({ tool: 'write', sessionID: 's1' }, {});
     await hook['tool.execute.after']({ tool: 'Read', sessionID: 's1' }, {});
-
-    const output = { system: [] };
-    await hook['experimental.chat.system.transform'](
-      { sessionID: 's1' },
-      output,
+    await hook['experimental.chat.messages.transform'](
+      {},
+      { messages: [message] },
     );
 
-    expect(output.system.filter((s) => s === PHASE_REMINDER)).toHaveLength(1);
+    expect(reminderParts(message)).toHaveLength(1);
   });
 
-  test('consumes pending marker after injection', async () => {
+  test.each([
+    ['wrong session', { messages: [orchestratorMessage('s2')] }],
+    ['empty messages', { messages: [] }],
+    [
+      'non-orchestrator turn',
+      {
+        messages: [
+          {
+            info: { role: 'user', agent: 'explorer', sessionID: 's1' },
+            parts: [{ type: 'text', text: 'hello' }],
+          },
+        ],
+      },
+    ],
+    [
+      'turn without a session',
+      {
+        messages: [
+          {
+            info: { role: 'user', agent: 'orchestrator' },
+            parts: [{ type: 'text', text: 'hello' }],
+          },
+        ],
+      },
+    ],
+    [
+      'attachment-only turn',
+      {
+        messages: [
+          {
+            info: { role: 'user', agent: 'orchestrator', sessionID: 's1' },
+            parts: [{ type: 'image', url: 'https://example.com/image.png' }],
+          },
+        ],
+      },
+    ],
+    [
+      'internal turn',
+      {
+        messages: [
+          {
+            info: { role: 'user', agent: 'orchestrator', sessionID: 's1' },
+            parts: [createInternalAgentTextPart('internal notification')],
+          },
+        ],
+      },
+    ],
+  ])('does not consume pending for %s', async (_name, output) => {
     const coordinator = new SessionLifecycle(() => {});
     const hook = createPostFileToolNudgeHook({ coordinator });
+    const eligibleMessage = orchestratorMessage();
 
     await hook['tool.execute.after']({ tool: 'Read', sessionID: 's1' }, {});
-    await hook['experimental.chat.system.transform'](
-      { sessionID: 's1' },
-      { system: [] },
-    );
-
-    // Second transform should not inject
-    const output = { system: [] };
-    await hook['experimental.chat.system.transform'](
-      { sessionID: 's1' },
-      output,
+    await hook['experimental.chat.messages.transform']({}, output);
+    await hook['experimental.chat.messages.transform'](
+      {},
+      { messages: [eligibleMessage] },
     );
 
-    expect(output.system).toHaveLength(0);
+    expect(reminderParts(eligibleMessage)).toHaveLength(1);
   });
 
-  test('ignores non-file tools', async () => {
+  test('trusted phase reminder metadata consumes pending without duplication', async () => {
     const coordinator = new SessionLifecycle(() => {});
     const hook = createPostFileToolNudgeHook({ coordinator });
-    const output = { system: [] };
+    const message = orchestratorMessage();
+    message.parts.push({
+      type: 'text',
+      synthetic: true,
+      text: PHASE_REMINDER,
+      metadata: { [PHASE_REMINDER_METADATA_KEY]: true },
+    });
 
-    await hook['tool.execute.after']({ tool: 'bash', sessionID: 's1' }, {});
-    await hook['experimental.chat.system.transform'](
-      { sessionID: 's1' },
-      output,
+    await hook['tool.execute.after']({ tool: 'Read', sessionID: 's1' }, {});
+    await hook['experimental.chat.messages.transform'](
+      {},
+      { messages: [message] },
     );
+    expect(reminderParts(message)).toHaveLength(1);
 
-    expect(output.system).toHaveLength(0);
+    const freshMessage = orchestratorMessage();
+    await hook['experimental.chat.messages.transform'](
+      {},
+      { messages: [freshMessage] },
+    );
+    expect(reminderParts(freshMessage)).toHaveLength(0);
   });
 
-  test('skips injection when shouldInject returns false', async () => {
+  test('passes the derived session ID to shouldInject', async () => {
     const coordinator = new SessionLifecycle(() => {});
+    const seenSessionIDs: string[] = [];
     const hook = createPostFileToolNudgeHook({
-      shouldInject: () => false,
       coordinator,
+      shouldInject: (sessionID) => {
+        seenSessionIDs.push(sessionID);
+        return false;
+      },
     });
-    const output = { system: [] };
+    const message = orchestratorMessage();
 
     await hook['tool.execute.after']({ tool: 'Read', sessionID: 's1' }, {});
-    await hook['experimental.chat.system.transform'](
-      { sessionID: 's1' },
-      output,
+    await hook['experimental.chat.messages.transform'](
+      {},
+      { messages: [message] },
     );
-
-    expect(output.system).toHaveLength(0);
+    expect(reminderParts(message)).toHaveLength(0);
+    expect(seenSessionIDs).toEqual(['s1']);
   });
 
-  test('ignores Read/Write without sessionID', async () => {
+  test('cleans pending state after session deletion', async () => {
     const coordinator = new SessionLifecycle(() => {});
     const hook = createPostFileToolNudgeHook({ coordinator });
-    const output = { system: [] };
+    const message = orchestratorMessage();
 
-    await hook['tool.execute.after']({ tool: 'read' }, {});
-    await hook['experimental.chat.system.transform'](
-      { sessionID: 's1' },
-      output,
+    await hook['tool.execute.after']({ tool: 'Read', sessionID: 's1' }, {});
+    coordinator.dispatchSessionDeleted('s1');
+    await hook['experimental.chat.messages.transform'](
+      {},
+      { messages: [message] },
     );
 
-    expect(output.system).toHaveLength(0);
+    expect(reminderParts(message)).toHaveLength(0);
   });
 
-  test('cleans up pending marker on session.deleted via coordinator', async () => {
+  test('ignores non-file tools and file calls without a session', async () => {
     const coordinator = new SessionLifecycle(() => {});
     const hook = createPostFileToolNudgeHook({ coordinator });
+    const message = orchestratorMessage();
 
-    await hook['tool.execute.after']({ tool: 'Read', sessionID: 's1' }, {});
-    coordinator.dispatchSessionDeleted('s1');
-
-    const output = { system: [] };
-    await hook['experimental.chat.system.transform'](
-      { sessionID: 's1' },
-      output,
+    await hook['tool.execute.after']({ tool: 'bash', sessionID: 's1' }, {});
+    await hook['tool.execute.after']({ tool: 'Read' }, {});
+    await hook['experimental.chat.messages.transform'](
+      {},
+      { messages: [message] },
     );
 
-    expect(output.system).toHaveLength(0);
+    expect(reminderParts(message)).toHaveLength(0);
   });
 
-  test('cleans up pending marker via coordinator with info.id shape', async () => {
+  test('keeps pending sessions isolated', async () => {
     const coordinator = new SessionLifecycle(() => {});
     const hook = createPostFileToolNudgeHook({ coordinator });
+    const s1Message = orchestratorMessage('s1');
+    const s2Message = orchestratorMessage('s2');
 
     await hook['tool.execute.after']({ tool: 'Read', sessionID: 's1' }, {});
-    coordinator.dispatchSessionDeleted('s1');
-
-    const output = { system: [] };
-    await hook['experimental.chat.system.transform'](
-      { sessionID: 's1' },
-      output,
+    await hook['tool.execute.after']({ tool: 'Write', sessionID: 's2' }, {});
+    await hook['experimental.chat.messages.transform'](
+      {},
+      { messages: [s2Message] },
     );
-
-    expect(output.system).toHaveLength(0);
-  });
-
-  test('composed: phase-reminder skips when post-file-tool-nudge handles system', async () => {
-    const { createPhaseReminderHook } = await import('../phase-reminder/index');
-    const coordinator = new SessionLifecycle(() => {});
-    const nudgeHook = createPostFileToolNudgeHook({ coordinator });
-    const phaseHook = createPhaseReminderHook(coordinator);
-
-    // Simulate Read tool call
-    await nudgeHook['tool.execute.after'](
-      { tool: 'Read', sessionID: 's1' },
+    await hook['experimental.chat.messages.transform'](
       {},
+      { messages: [s1Message] },
     );
 
-    // System transform injects into system array
-    const systemOutput = { system: [] as string[] };
-    await nudgeHook['experimental.chat.system.transform'](
-      { sessionID: 's1' },
-      systemOutput,
-    );
-    expect(systemOutput.system).toContain(PHASE_REMINDER);
-
-    // Messages transform should NOT inject (pending already handled by system)
-    const messagesOutput = {
-      messages: [
-        {
-          info: { role: 'user', sessionID: 's1' },
-          parts: [{ type: 'text', text: 'hello' }],
-        },
-      ],
-    };
-    await phaseHook['experimental.chat.messages.transform']({}, messagesOutput);
-
-    const userParts = messagesOutput.messages[0].parts;
-    const reminderParts = userParts.filter(
-      (p: { text?: string }) => p.text === PHASE_REMINDER,
-    );
-    expect(reminderParts).toHaveLength(0);
+    expect(reminderParts(s2Message)).toHaveLength(1);
+    expect(reminderParts(s1Message)).toHaveLength(1);
   });
 });

+ 65 - 7
src/hooks/post-file-tool-nudge/index.ts

@@ -3,11 +3,15 @@
  * Catches the "inspect/edit files → implement myself" anti-pattern.
  *
  * The reminder is ephemeral: recorded on tool execution, injected via
- * system.transform, and consumed once. File tool output stays clean.
+ * messages.transform, and consumed once. File tool output stays clean.
  */
 
 import { PHASE_REMINDER } from '../../config/constants';
+import { isInternalInitiatorPart } from '../../utils';
+import { isRecord } from '../../utils/guards';
+import { PHASE_REMINDER_METADATA_KEY } from '../phase-reminder';
 import type { SessionLifecycle } from '../session-lifecycle';
+import { isUserMessageWithParts, type MessageWithParts } from '../types';
 
 const FILE_TOOLS = new Set(['Read', 'read', 'Write', 'write']);
 
@@ -33,17 +37,71 @@ export function createPostFileToolNudgeHook(
       if (!FILE_TOOLS.has(input.tool) || !input.sessionID) return;
       coordinator?.markPending(input.sessionID);
     },
-    'experimental.chat.system.transform': async (
-      input: { sessionID?: string },
-      output: { system: string[] },
+    'experimental.chat.messages.transform': async (
+      _input: Record<string, never>,
+      output: { messages?: unknown },
     ): Promise<void> => {
-      if (!input.sessionID || !coordinator?.consumePending(input.sessionID)) {
+      if (!coordinator) {
         return;
       }
-      if (options.shouldInject && !options.shouldInject(input.sessionID)) {
+
+      const messages = Array.isArray(output.messages) ? output.messages : [];
+      let lastUserMessage: unknown;
+      for (let index = messages.length - 1; index >= 0; index--) {
+        if (isUserMessageWithParts(messages[index])) {
+          lastUserMessage = messages[index];
+          break;
+        }
+      }
+
+      const eligible = getEligibleMessage(lastUserMessage);
+      if (!eligible) {
+        return;
+      }
+      const { message, sessionID } = eligible;
+
+      const hasReminder = message.parts.some(
+        (part) =>
+          part.synthetic === true &&
+          isRecord(part.metadata) &&
+          part.metadata[PHASE_REMINDER_METADATA_KEY] === true,
+      );
+      if (!coordinator.consumePending(sessionID)) {
+        return;
+      }
+      if (
+        hasReminder ||
+        (options.shouldInject && !options.shouldInject(sessionID))
+      ) {
         return;
       }
-      output.system.push(PHASE_REMINDER);
+      message.parts.push({
+        type: 'text',
+        synthetic: true,
+        text: PHASE_REMINDER,
+        metadata: { [PHASE_REMINDER_METADATA_KEY]: true },
+      });
     },
   };
 }
+
+function getEligibleMessage(
+  message: unknown,
+): { message: MessageWithParts; sessionID: string } | undefined {
+  if (
+    !isUserMessageWithParts(message) ||
+    !message.info.sessionID ||
+    (message.info.agent && message.info.agent !== 'orchestrator')
+  ) {
+    return undefined;
+  }
+
+  const textPart = message.parts.find(
+    (part) => part.type === 'text' && part.text !== undefined,
+  );
+  if (!textPart || isInternalInitiatorPart(textPart)) {
+    return undefined;
+  }
+
+  return { message, sessionID: message.info.sessionID };
+}

+ 2 - 15
src/hooks/session-lifecycle.test.ts

@@ -31,23 +31,10 @@ describe('SessionLifecycle', () => {
     expect(lc.consumePending('s1')).toBe(false);
   });
 
-  test('hasPendingSession after consume', () => {
+  test('clearSession removes pending state', () => {
     const lc = new SessionLifecycle(noop);
     lc.markPending('s1');
-    lc.consumePending('s1');
-    expect(lc.hasPendingSession('s1')).toBe(true);
-  });
-
-  test('hasPendingSession false for unknown session', () => {
-    const lc = new SessionLifecycle(noop);
-    expect(lc.hasPendingSession('s1')).toBe(false);
-  });
-
-  test('clearSession removes all state', () => {
-    const lc = new SessionLifecycle(noop);
-    lc.markPending('s1');
-    lc.consumePending('s1');
     lc.clearSession('s1');
-    expect(lc.hasPendingSession('s1')).toBe(false);
+    expect(lc.consumePending('s1')).toBe(false);
   });
 });

+ 0 - 10
src/hooks/session-lifecycle.ts

@@ -1,7 +1,6 @@
 export class SessionLifecycle {
   #cleanupCallbacks: Array<(sessionId: string) => void> = [];
   #pendingSessionIds = new Set<string>();
-  #everPendingSessionIds = new Set<string>();
   #log: (msg: string, meta?: Record<string, unknown>) => void;
 
   constructor(log: (msg: string, meta?: Record<string, unknown>) => void) {
@@ -27,7 +26,6 @@ export class SessionLifecycle {
 
   markPending(sessionId: string): void {
     this.#pendingSessionIds.add(sessionId);
-    this.#everPendingSessionIds.add(sessionId);
   }
 
   /** Atomic — only one caller gets true per markPending call. */
@@ -37,15 +35,7 @@ export class SessionLifecycle {
     return had;
   }
 
-  hasPendingSession(sessionId: string): boolean {
-    return (
-      this.#everPendingSessionIds.has(sessionId) &&
-      !this.#pendingSessionIds.has(sessionId)
-    );
-  }
-
   clearSession(sessionId: string): void {
     this.#pendingSessionIds.delete(sessionId);
-    this.#everPendingSessionIds.delete(sessionId);
   }
 }

+ 5 - 7
src/index.ts

@@ -373,7 +373,7 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
       };
     };
 
-    phaseReminder = createPhaseReminderHook(sessionLifecycle);
+    phaseReminder = createPhaseReminderHook();
 
     filterAvailableSkills = createFilterAvailableSkillsHook(ctx, config);
 
@@ -1145,12 +1145,6 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
         }
       }
 
-      // Inject ephemeral post-file-tool-nudge reminder
-      await postFileToolNudge['experimental.chat.system.transform'](
-        input as never,
-        output as never,
-      );
-
       // Collapse to single system message for provider compatibility.
       // Some providers (e.g. Qwen via VLLM/DashScope) reject multiple
       // system messages. Sub-hooks above may push additional entries; join
@@ -1195,6 +1189,10 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
         log,
       });
 
+      await postFileToolNudge['experimental.chat.messages.transform'](
+        input as never,
+        typedOutput as never,
+      );
       await phaseReminder['experimental.chat.messages.transform'](
         input as never,
         typedOutput as never,