Skip to content

fix(plugin/codex): capture Stop assistant message#2020

Closed
ZaynJarvis wants to merge 1 commit into
volcengine:mainfrom
ZaynJarvis:fix/codex-plugin-last-assistant-capture
Closed

fix(plugin/codex): capture Stop assistant message#2020
ZaynJarvis wants to merge 1 commit into
volcengine:mainfrom
ZaynJarvis:fix/codex-plugin-last-assistant-capture

Conversation

@ZaynJarvis
Copy link
Copy Markdown
Collaborator

Summary

  • append Codex last_assistant_message on Stop when captureLastAssistantOnStop=true
  • store a hash in Codex plugin state so repeated Stop events do not duplicate the same assistant message
  • update Codex plugin README/DESIGN/VERIFICATION to match default user-turn + last-assistant capture semantics

Verification

  • npm run build in examples/codex-memory-plugin
  • node --check examples/codex-memory-plugin/scripts/auto-capture.mjs
  • node --check examples/codex-memory-plugin/scripts/session-state.mjs
  • git diff --check
  • Live OV smoke: isolated Codex plugin install; Stop hook appended user + last_assistant_message, repeat Stop no-op, PreCompact commit preserved state
  • Live OV MCP smoke: listed MCP tools and ran openviking_health, openviking_store, openviking_recall
  • Live auto-recall smoke: returned hookSpecificOutput.additionalContext containing the stored marker memory

@github-actions
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🏅 Score: 95
🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions
Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Normalize text consistently for hash comparison

Normalize transcript turn text identically to the last assistant message before
hashing to ensure consistent duplicate detection. Extract text normalization into a
shared helper to avoid logic duplication.

examples/codex-memory-plugin/scripts/auto-capture.mjs [84-285]

+function normalizeText(text) {
+  const trimmed = text.trim();
+  if (!trimmed) return "";
+  return trimmed.length > cfg.captureMaxLength ? trimmed.slice(0, cfg.captureMaxLength) : trimmed;
+}
+
 function normalizeLastAssistantMessage(value) {
-  const text = extractTextFromContent(value).trim();
-  if (!text) return "";
-  return text.length > cfg.captureMaxLength ? text.slice(0, cfg.captureMaxLength) : text;
+  const text = extractTextFromContent(value);
+  return normalizeText(text);
 }
 
 ...
 
 const alreadyCapturedInTranscript = newTurns.some(
-  (turn) => turn.role === "assistant" && hashText(turn.text) === hash,
+  (turn) => turn.role === "assistant" && hashText(normalizeText(turn.text)) === hash,
 ) && allNewTranscriptTurnsAppended;
Suggestion importance[1-10]: 8

__

Why: The suggestion fixes inconsistent text normalization between the last assistant message and transcript turns, which would cause incorrect duplicate detection. Extracting normalizeText ensures hashes are computed identically, improving correctness.

Medium
General
Use strict equality for turn count check

Use strict equality check instead of greater-or-equal to verify all new transcript
turns were appended, as added should never exceed newTurns.length. This avoids false
positives if added unexpectedly exceeds newTurns.length.

examples/codex-memory-plugin/scripts/auto-capture.mjs [282]

-const allNewTranscriptTurnsAppended = added >= newTurns.length;
+const allNewTranscriptTurnsAppended = added === newTurns.length;
Suggestion importance[1-10]: 5

__

Why: Using strict equality instead of >= is a defensive check that avoids false positives if added unexpectedly exceeds newTurns.length, improving code robustness.

Low

@ZaynJarvis
Copy link
Copy Markdown
Collaborator Author

Closing this PR because it was based on the wrong interpretation of task #103. The requested MCP validation should target OpenViking's native /mcp endpoint, not the example Codex memory plugin's small helper MCP server. I will re-test against the native endpoint and open a focused PR only if that path has an issue.

@ZaynJarvis ZaynJarvis closed this May 13, 2026
@github-project-automation github-project-automation Bot moved this from Backlog to Done in OpenViking project May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant