Browse Source

fix(hooks): prevent phase-reminder from leaking into UI/chat history (#448)

The phase-reminder hook was mutating lastUserMessage.parts[textPartIndex].text
to append the reminder, which caused the <internal_reminder> block to be
persisted in the session DB and rendered in the OpenCode UI.

This change appends the reminder as a separate MessagePart instead of
mutating the user-authored text. The reminder still reaches the API, but it
no longer leaks into the UI or chat history.

- Replace in-place text mutation with parts.push({ type: 'text', text: PHASE_REMINDER })
- Update deduplication to scan all parts instead of only the first text part
- Add comprehensive unit tests for the hook including regression test for #448

Fixes #448
qwtoe 3 months ago
parent
commit
3bd3f5a6cd
2 changed files with 83 additions and 11 deletions
  1. 73 8
      src/hooks/phase-reminder/index.test.ts
  2. 10 3
      src/hooks/phase-reminder/index.ts

+ 73 - 8
src/hooks/phase-reminder/index.test.ts

@@ -3,7 +3,7 @@ import { SLIM_INTERNAL_INITIATOR_MARKER } from '../../utils';
 import { createPhaseReminderHook, PHASE_REMINDER } from './index';
 
 describe('createPhaseReminderHook', () => {
-  test('appends reminder for orchestrator sessions', async () => {
+  test('appends reminder as a separate part for orchestrator sessions', async () => {
     const hook = createPhaseReminderHook();
     const output = {
       messages: [
@@ -16,9 +16,10 @@ describe('createPhaseReminderHook', () => {
 
     await hook['experimental.chat.messages.transform']({}, output);
 
-    expect(output.messages[0].parts[0].text).toBe(
-      `hello\n\n---\n\n${PHASE_REMINDER}`,
-    );
+    // Reminder is appended as a new part, not merged into the original text
+    expect(output.messages[0].parts.length).toBe(2);
+    expect(output.messages[0].parts[0].text).toBe('hello');
+    expect(output.messages[0].parts[1].text).toBe(PHASE_REMINDER);
   });
 
   test('skips non-orchestrator sessions', async () => {
@@ -34,6 +35,7 @@ describe('createPhaseReminderHook', () => {
 
     await hook['experimental.chat.messages.transform']({}, output);
 
+    expect(output.messages[0].parts.length).toBe(1);
     expect(output.messages[0].parts[0].text).toBe('hello');
   });
 
@@ -52,23 +54,86 @@ describe('createPhaseReminderHook', () => {
     await hook['experimental.chat.messages.transform']({}, output);
 
     expect(output.messages[0].parts[0].text).toBe(text);
-    expect(output.messages[0].parts[0].text).not.toContain(PHASE_REMINDER);
+    expect(output.messages[0].parts.length).toBe(1);
   });
 
   test('does not append duplicate reminder', async () => {
     const hook = createPhaseReminderHook();
-    const text = `hello\n\n---\n\n${PHASE_REMINDER}`;
     const output = {
       messages: [
         {
           info: { role: 'user', agent: 'orchestrator' },
-          parts: [{ type: 'text', text }],
+          parts: [
+            { type: 'text', text: 'hello' },
+            { type: 'text', text: PHASE_REMINDER },
+          ],
         },
       ],
     };
 
     await hook['experimental.chat.messages.transform']({}, output);
 
-    expect(output.messages[0].parts[0].text).toBe(text);
+    expect(output.messages[0].parts.length).toBe(2);
+    expect(output.messages[0].parts[0].text).toBe('hello');
+  });
+
+  test('does not modify original user message text (bug #448)', async () => {
+    const hook = createPhaseReminderHook();
+    const originalText = 'Hello world';
+    const output = {
+      messages: [
+        {
+          info: { role: 'user', agent: 'orchestrator' },
+          parts: [{ type: 'text', text: originalText }],
+        },
+      ],
+    };
+
+    await hook['experimental.chat.messages.transform']({}, output);
+
+    // The original text part must remain unchanged so it doesn't leak into UI/history
+    expect(output.messages[0].parts[0].text).toBe(originalText);
+    expect(output.messages[0].parts[1].text).toBe(PHASE_REMINDER);
+  });
+
+  test('handles messages without text parts', async () => {
+    const hook = createPhaseReminderHook();
+    const output = {
+      messages: [
+        {
+          info: { role: 'user', agent: 'orchestrator' },
+          parts: [{ type: 'image', url: 'http://example.com/img.png' }],
+        },
+      ],
+    };
+
+    await hook['experimental.chat.messages.transform']({}, output);
+
+    expect(output.messages[0].parts.length).toBe(1);
+  });
+
+  test('handles empty messages array', async () => {
+    const hook = createPhaseReminderHook();
+    const output = { messages: [] };
+
+    await hook['experimental.chat.messages.transform']({}, output);
+
+    expect(output.messages).toEqual([]);
+  });
+
+  test('handles no user messages', async () => {
+    const hook = createPhaseReminderHook();
+    const output = {
+      messages: [
+        {
+          info: { role: 'assistant' },
+          parts: [{ type: 'text', text: 'Hi' }],
+        },
+      ],
+    };
+
+    await hook['experimental.chat.messages.transform']({}, output);
+
+    expect(output.messages[0].parts[0].text).toBe('Hi');
   });
 });

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

@@ -74,12 +74,19 @@ export function createPhaseReminderHook() {
       if (originalText.includes(SLIM_INTERNAL_INITIATOR_MARKER)) {
         return;
       }
-      if (originalText.includes(PHASE_REMINDER)) {
+      // Prevent duplicate injection: check if any existing part already contains
+      // the phase reminder (either merged into text or as a standalone part).
+      if (lastUserMessage.parts.some((p) => p.text?.includes(PHASE_REMINDER))) {
         return;
       }
 
-      lastUserMessage.parts[textPartIndex].text =
-        `${originalText}\n\n---\n\n${PHASE_REMINDER}`;
+      // Append reminder as a new, separate message part instead of mutating
+      // the user-authored text. This prevents the reminder from leaking into
+      // the UI display and chat history (issue #448).
+      lastUserMessage.parts.push({
+        type: 'text',
+        text: PHASE_REMINDER,
+      });
     },
   };
 }