Skip to content

fix(codex): keep foreign memory out of chats - #8989

Open
SunkenInTime wants to merge 1 commit into
pingdotgg:mainfrom
SunkenInTime:t3code/fix-computer-history-leak
Open

SunkenInTime wants to merge 1 commit into
pingdotgg:mainfrom
SunkenInTime:t3code/fix-computer-history-leak

Conversation

@SunkenInTime

@SunkenInTime SunkenInTime commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Codex background memory work can begin emitting item notifications on an existing app-server subscription without sending a new thread/started notification. The stateful memory filter never learns that provider thread ID, so its assistant text can be projected into the active T3 chat.

This treats the active Codex provider thread ID as the chat ownership boundary. Known notifications addressed to another thread are suppressed even when their lifecycle start was missed. Registered multi-agent children still use their dedicated routing, and serverRequest/resolved still passes through for approval correlation cleanup.

The regression replay matches the observed ordering: agent message delta and item completion for a memory thread with no preceding thread/started.

Verification:

  • vp test run apps/server/src/provider/Layers/CodexSessionRuntime.test.ts apps/server/src/provider/Layers/CodexCollabWire.test.ts apps/server/src/provider/Layers/CodexCollabRuntime.integration.test.ts
  • vp lint --type-aware --type-check --report-unused-disable-directives apps/server/src/provider/Layers/CodexSessionRuntime.ts apps/server/src/provider/Layers/CodexSessionRuntime.test.ts apps/server/src/provider/Layers/CodexCollabRuntime.integration.test.ts
  • vp fmt --check apps/server/src/provider/Layers/CodexSessionRuntime.ts apps/server/src/provider/Layers/CodexSessionRuntime.test.ts apps/server/src/provider/Layers/CodexCollabRuntime.integration.test.ts
  • git diff --check upstream/main...HEAD

Generated with GPT-5.6 Sol in T3 Code via the Codex harness.


Note

Medium Risk
Changes Codex notification routing for all non-root threads; incorrect suppression could hide legitimate cross-thread traffic, though collab interception and serverRequest/resolved exceptions limit blast radius.

Overview
Fixes background Codex memory work showing up in the active T3 chat when item deltas arrive on an existing app-server subscription without a preceding thread/started, so the stateful memory filter never learns that thread id.

The runtime now treats the active provider thread id as the chat boundary: exported shouldSuppressForeignConversationNotification drops any known notification whose threadId differs from the root, including item/agentMessage/delta and item/completed, independent of lifecycle registration. Registered collab children still go through interception first; serverRequest/resolved still passes through for approval correlation.

Suppression logic is split so v1 receiver-map children keep lifecycle-only routing via shouldSuppressChildConversationNotification, while the new helper handles the broader foreign-thread case (including the memory leak ordering).

Tests cover the helper directly and extend the collab integration replay to assert no events leak for the memory thread.

Reviewed by Cursor Bugbot for commit 509d07f. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Suppress foreign memory-thread notifications in makeCodexSessionRuntime

  • Adds shouldSuppressForeignConversationNotification to classify non-root-thread notifications as suppressible, except serverRequest/resolved.
  • The notification loop now suppresses all notifications addressed to non-root provider threads (item deltas, completions) instead of only selected lifecycle events, preventing background memory output from appearing in the parent session.
  • Adds unit tests for the new predicate and an integration assertion that no events from the background memory thread leak without a prior thread/started.
  • Risk: makeCodexSessionRuntime now drops all non-root-thread item/* notifications; any consumer relying on foreign thread deltas appearing in the session stream will no longer receive them.

Macroscope summarized 509d07f.

Summary by CodeRabbit

  • Bug Fixes
    • Prevented background memory-thread messages from appearing in the parent conversation stream before the thread is initialized.
    • Improved filtering of notifications from unrelated conversation threads while preserving valid root-thread updates and resolved server requests.
    • Added coverage for memory output and cross-thread notification handling to ensure these events remain isolated.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 1, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 509d07f

Macroscope's review found this PR approvable — This is a focused Codex bug fix that prevents foreign-thread memory output from being emitted into the active chat while preserving root events, child routing, and approval correlation. The runtime change is narrowly scoped and accompanied by unit and integration regression coverage.

You can add or adjust custom eligibility rules. Learn more.

@gfdelarue

Copy link
Copy Markdown

Confirmed another occurrence on 2026-09-05 with T3 desktop 0.0.38 on NixOS, source checkout 77b655c47, and Codex CLI 0.153.3. The running package already includes the earlier #5468 filter.

Observed reproduction:

  1. Ask an ordinary Git question in a Codex chat with automatic memories enabled.
  2. After the answer completes, background memory consolidation starts and spawns three workers.
  3. Their memory-extraction commentary, command activity, and final replies appear in the original chat, interleaved with subsequent user messages.

Read-only inspection confirmed persistence, not just a rendering problem. Sanitized identities:

Codex conversation A: Git question and answer; no worker reports
Codex logs: memory consolidation M spawned workers B, C, D
T3 projection_thread_messages: worker replies stored under A's T3 thread
T3 projection_thread_activities: same T3 thread, but data.threadId = B/C/D
Worker command cwd: ~/.codex/memories

The reports only entered conversation A's Codex transcript when I pasted the mixed UI transcript back to the assistant. This is the same user-visible failure as #4683 despite the earlier fix.

I checked this PR's actual shouldSuppressForeignConversationNotification at 509d07f: all 12 combinations of the three observed worker IDs and four message/command event methods were suppressed. Root messages and serverRequest/resolved still passed. This was a filter-level check, not an installed end-to-end test of the PR; the incident does not establish whether each worker's thread/started was absent or missed.

The follow-up filter remains absent from upstream main at 4ca71463a and nightly 0.0.39-nightly.20260905.1289. This PR addresses the observed routing gap. No private transcripts or raw logs attached.

@SunkenInTime
SunkenInTime force-pushed the t3code/fix-computer-history-leak branch from 509d07f to b6b06b0 Compare September 12, 2026 04:45
@cursor

cursor Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 5adf7a77-aa65-4637-8664-385febbd21f7

📥 Commits

Reviewing files that changed from the base of the PR and between e816064 and b6b06b0.

📒 Files selected for processing (3)
  • apps/server/src/provider/Layers/CodexCollabRuntime.integration.test.ts
  • apps/server/src/provider/Layers/CodexSessionRuntime.test.ts
  • apps/server/src/provider/Layers/CodexSessionRuntime.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The runtime now suppresses notifications from foreign threads, except serverRequest/resolved, before parent-stream fan-out. Unit and integration tests cover memory-thread deltas and completion events without preceding thread/started notifications.

Changes

Notification suppression

Layer / File(s) Summary
Foreign notification suppression contract
apps/server/src/provider/Layers/CodexSessionRuntime.ts, apps/server/src/provider/Layers/CodexSessionRuntime.test.ts
Adds shouldSuppressForeignConversationNotification and tests suppression for foreign memory events while preserving root-thread and serverRequest/resolved events.
Runtime filtering and fan-out validation
apps/server/src/provider/Layers/CodexSessionRuntime.ts, apps/server/src/provider/Layers/CodexCollabRuntime.integration.test.ts
Updates handleRawNotification and verifies that memory-thread delta and completion events do not reach the parent event stream.

Priority: ⬆️ High

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to b6b06

Foreign memory-thread notifications are filtered from the parent stream while registered child routing and approval cleanup remain intact. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preventing foreign Codex memory notifications from appearing in chats.
Description check ✅ Passed The description explains what changed, why it changed, the routing behavior, regression coverage, and verification steps. UI changes are not applicable. The template checklist is not reproduced, but t…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants