Wire up memory extensions for ACP sessions#329
Conversation
|
Hi @benbrandt, could you take a look when you have a chance? This PR wires the Codex memories extension and startup pipeline into the ACP session path and includes focused coverage for the wiring. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fc1d46dda
ℹ️ 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".
| } | ||
|
|
||
| fn acp_session_source() -> SessionSource { | ||
| SessionSource::Mcp |
There was a problem hiding this comment.
Include ACP sessions in memory extraction sources
For ACP sessions created after this change, their rollouts are now persisted as SessionSource::Mcp, but the pinned codex-memories-write phase-1 startup path claims jobs via codex_rollout::INTERACTIVE_SESSION_SOURCES, which in rust-v0.137.0 contains only CLI, VSCode, and the atlas/chatgpt custom sources. As a result, when users work exclusively through ACP the startup task is triggered but never selects those ACP rollouts for memory extraction, so the feature cannot learn from ACP sessions until Mcp is added to the eligible source list (or ACP uses an eligible source).
Useful? React with 👍 / 👎.
c85271b to
f6d4675
Compare
|
Update after local verification and follow-up cleanup: I validated the ACP path with a real ACP JSON-RPC flow, not only unit tests. The smoke test started The resulting thread was written to I removed the temporary Verification run after removing the vendored patch:
|
f6d4675 to
3f383d8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f383d811d
ℹ️ 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".
3f383d8 to
f030e56
Compare
|
Hi thanks for taking the time to dig in! |
Summary
SessionSource::Mcpand keep them visible in session listingAddresses the ACP-side wiring in #327.
ACP scenario covered
codex-acpover stdio JSON-RPC.initialize,session/new, and thensession/promptfor a workspace.source=mcp, the memories startup task is invoked after prompt submission, and the session remains listable through the ACP session list path.Scope note
This PR no longer vendors
codex-rollout. The current upstreamrust-v0.137.0codex_rollout::INTERACTIVE_SESSION_SOURCESlist does not includeSessionSource::Mcp, so startup extraction allowlist support for MCP-sourced rollouts should be handled in the upstream Codex dependency rather than patched in this repository.Local verification
target/debug/codex-acp, sentinitialize -> session/new -> session/prompt -> session/close, and observedstopReason=end_turn.~/.codex/state_5.sqlitewithsource=mcp,memory_mode=enabled, and the codex-acp workspace cwd.session_meta.source=mcp. Its rawmemory_modewas absent, which is expected; rollout reconciliation normalizes absent memory mode toenabledin the state DB.cargo tree -i codex-rolloutresolvescodex-rolloutfrom the upstream git dependency, not a vendored path.Tests
cargo fmt --checkgit diff --checkcargo test --lib codex_agent::tests::cargo test