feat: ignore Codex and Codex-app internal system prompts#2810
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffcf27481a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (trimmed.startsWith('## Memory Writing Agent: Phase 2 (Consolidation)') || trimmed.startsWith('## Memory Writing Agent')) { | ||
| return true; |
There was a problem hiding this comment.
Narrow the Memory Writing Agent prefix check
For any user-authored prompt that begins with a markdown heading like ## Memory Writing Agent (for example asking the assistant to draft or review a prompt/template with that heading), this returns true and the session-init callers drop the prompt before creating/saving the session. Because the check is based only on this short prefix rather than the full internal prompt shape or metadata identifying an internal Codex event, legitimate prompts in all supported hook paths can be skipped and excluded from memory tracking.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds
Confidence Score: 3/5Safe to merge once the stale reason field in the HTTP skip response is corrected. The HTTP route returns reason: 'internal_protocol' unconditionally even when the skip is triggered by isInternalSystemPrompt. Any caller or monitoring rule reading that field will get wrong data on every system-prompt skip. src/services/worker/http/routes/SessionRoutes.ts needs the skip response reason field updated to reflect the actual cause when isInternalSystemPrompt fires. Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/services/worker/http/routes/SessionRoutes.ts:321
**Stale `reason` field when skipping internal system prompts**
When `isInternalSystemPrompt(rawPrompt)` is the match (and `isInternalProtocolPayload` is not), the response still emits `reason: 'internal_protocol'`. Any downstream client, monitoring pipeline, or log query keyed on `reason` to distinguish skip types will silently receive the wrong label for this new skip path. The `reason` should reflect the actual cause — for example `'internal_system_prompt'` — or the two checks should be reported with separate values.
```suggestion
res.json({ skipped: true, reason: isInternalProtocolPayload(rawPrompt) ? 'internal_protocol' : 'internal_system_prompt' });
```
### Issue 2 of 2
src/utils/tag-stripping.ts:80-83
The first condition `trimmed.startsWith('## Memory Writing Agent: Phase 2 (Consolidation)')` is dead code — any string that starts with the longer Phase 2 prefix also starts with `'## Memory Writing Agent'`, so the second condition always fires first via short-circuit evaluation. The specific Phase 2 check can be removed without changing behaviour.
```suggestion
// 2. Memory Writing Agent consolidation prompt
if (trimmed.startsWith('## Memory Writing Agent')) {
return true;
}
```
Reviews (1): Last reviewed commit: "feat: ignore Codex and Codex-app interna..." | Re-trigger Greptile |
| logger.debug('HTTP', 'session-init: skipping internal protocol payload before session creation', { contentSessionId }); | ||
| if (rawPrompt && (isInternalProtocolPayload(rawPrompt) || isInternalSystemPrompt(rawPrompt))) { | ||
| logger.debug('HTTP', 'session-init: skipping internal protocol or system payload before session creation', { contentSessionId }); | ||
| res.json({ skipped: true, reason: 'internal_protocol' }); |
There was a problem hiding this comment.
Stale
reason field when skipping internal system prompts
When isInternalSystemPrompt(rawPrompt) is the match (and isInternalProtocolPayload is not), the response still emits reason: 'internal_protocol'. Any downstream client, monitoring pipeline, or log query keyed on reason to distinguish skip types will silently receive the wrong label for this new skip path. The reason should reflect the actual cause — for example 'internal_system_prompt' — or the two checks should be reported with separate values.
| res.json({ skipped: true, reason: 'internal_protocol' }); | |
| res.json({ skipped: true, reason: isInternalProtocolPayload(rawPrompt) ? 'internal_protocol' : 'internal_system_prompt' }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/services/worker/http/routes/SessionRoutes.ts
Line: 321
Comment:
**Stale `reason` field when skipping internal system prompts**
When `isInternalSystemPrompt(rawPrompt)` is the match (and `isInternalProtocolPayload` is not), the response still emits `reason: 'internal_protocol'`. Any downstream client, monitoring pipeline, or log query keyed on `reason` to distinguish skip types will silently receive the wrong label for this new skip path. The `reason` should reflect the actual cause — for example `'internal_system_prompt'` — or the two checks should be reported with separate values.
```suggestion
res.json({ skipped: true, reason: isInternalProtocolPayload(rawPrompt) ? 'internal_protocol' : 'internal_system_prompt' });
```
How can I resolve this? If you propose a fix, please make it concise.| // 2. Memory Writing Agent consolidation prompt | ||
| if (trimmed.startsWith('## Memory Writing Agent: Phase 2 (Consolidation)') || trimmed.startsWith('## Memory Writing Agent')) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The first condition
trimmed.startsWith('## Memory Writing Agent: Phase 2 (Consolidation)') is dead code — any string that starts with the longer Phase 2 prefix also starts with '## Memory Writing Agent', so the second condition always fires first via short-circuit evaluation. The specific Phase 2 check can be removed without changing behaviour.
| // 2. Memory Writing Agent consolidation prompt | |
| if (trimmed.startsWith('## Memory Writing Agent: Phase 2 (Consolidation)') || trimmed.startsWith('## Memory Writing Agent')) { | |
| return true; | |
| } | |
| // 2. Memory Writing Agent consolidation prompt | |
| if (trimmed.startsWith('## Memory Writing Agent')) { | |
| return true; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/utils/tag-stripping.ts
Line: 80-83
Comment:
The first condition `trimmed.startsWith('## Memory Writing Agent: Phase 2 (Consolidation)')` is dead code — any string that starts with the longer Phase 2 prefix also starts with `'## Memory Writing Agent'`, so the second condition always fires first via short-circuit evaluation. The specific Phase 2 check can be removed without changing behaviour.
```suggestion
// 2. Memory Writing Agent consolidation prompt
if (trimmed.startsWith('## Memory Writing Agent')) {
return true;
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This PR ignores Codex and Codex-app internal system-initiated prompts (e.g., session title generation, memory consolidation agent, and suggestions overview) to prevent triggering unnecessary session initialization and hook events.