Conversation
|
Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (83)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Run failed. View the logs →
|
45c6e06 to
c165006
Compare
|
Run failed. View the logs →
|
|
Run failed. View the logs →
|
|
Run failed. View the logs →
|
|
Run failed. View the logs →
|
|
@pullfrog review this pull request |
Allow Memory Store attach on session and deployment create paths, persist mount snapshots, and expose the console Resources UI for attach + display. Sandbox mount and filestore writeback remain follow-up PRs. Co-authored-by: Cursor <cursoragent@cursor.com>
1785998 to
d2f009b
Compare
|
Run failed. View the logs →
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2f009b39c
ℹ️ 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".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Create Session/Deployment used the list helper's first page of 5, so stores beyond that could not be selected. Follow next_page at the API limit.
Route Filestore /memory/{slug} through a memory backend so attached stores
write back to Memory tables and object storage.
Co-authored-by: Cursor <cursoragent@cursor.com>
d2f009b to
a95e95d
Compare
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed changes — reviewed the isolated #331 commit (a95e95de), i.e. git diff 0c7346eb..a95e95de; the remaining files in the branch diff belong to the stacked #329/#330 PRs and are out of scope here.
- Converted
memory_storesession resources from opaquesourcesinto parsedmemoryMounts, failing closed (invalidMemoryResources) on any unparseable snapshot so an attached store is never silently unmounted. - Appended one rclone mount per attached store (
/memory/{slug}→mount_path, 1s cache, token by access), keeping the/mnt/memoryparent as local disk rather than a filestore mount. - Added
startManagedAgentSessionFilesystem(mkdir → rclone ready → writeMEMORY.md), fail-closed on mkdir/write errors, and gated to the fresh-sandbox create path so pause/resume does not re-run it. - Injected
CLAUDE_CODE_REMOTE_MEMORY_DIR/CLAUDE_COWORK_MEMORY_PATH_OVERRIDEinto the sandbox env only when stores are mounted, while keepingappendSystemPromptstatic (no memory-policy leak); reserved both names in the vault credential layer. - Hid the platform
/MEMORY.mdfrom the console store list/tree and blocked Add-memory for it. - Added the
e2e-memory-sandboxworkflow/just target plus extensive unit tests and a real-E2B cross-session lifetime suite (M5-*).
✅ No new issues found.
ℹ️ Confirm memory consumption against the pinned claude build
This is a verification ask, not a code defect — the sandbox-side plumbing (mounts, MEMORY.md, env injection) is correct and well-tested. But no test in this commit exercises an actual agent turn persisting memory through /mnt/memory: the E2E writes sandbox files via shell (writeSandboxFile) and asserts MEMORY.md is present and appendSystemPrompt stays clean — it never proves Claude consumes the memory dir.
Both injected env vars are real but undocumented, feature-gated Claude Code knobs (verified names). Claude Code's loadMemoryPrompt ingests a root MEMORY.md verbatim and non-destructively, so the <!-- oma-stores --> store list is at least injected into the model context. However, this PR deliberately inverts the documented Anthropic memory contract (read-only auto-mount + system-prompt store description → local-disk root + env-var memdir) to keep the prompt cache stable, and whether the sandbox's pinned claude build honors these vars under current storage-path semantics (dir as root vs. base) is unverified — if the build implements newer semantics, the env injection could be a silent no-op.
Technical details
# Confirm the env-var memory wiring works on the deployed claude image
## Affected sites
- internal/environments/memory_runtime.go:131-139 — memorySessionEnvironment injects the two vars = /mnt/memory
- internal/environments/runner.go:695-711 — startManagedAgentSessionFilesystem writes /mnt/memory/MEMORY.md
- internal/environments/memory_runtime.go:59-74 — MEMORY.md renders <!-- oma-stores --> + per-store bullets
- tests/environments_memory_sandbox_e2e_test.go — writes via shell, no agent-turn persistence assertion
## Required outcome
- The author confirms the E2B image's pinned claude build honors CLAUDE_CODE_REMOTE_MEMORY_DIR /
CLAUDE_COWORK_MEMORY_PATH_OVERRIDE as a memdir root containing /mnt/memory/MEMORY.md, OR
- add an E2E step that drives one model turn (e.g. "remember X" / read a store) and asserts the
memory write lands in the rw store as a session_actor version — proving the model consumes /mnt/memory.
## Open questions for the human
- Which claude version/feature gates run in the E2B template, and which storage-path semantics does it use?
- Is the native (frontmatter topic-file) auto-memory contract intentionally not used here?DeepSeek Flash ｜ 𝕏
…ory TTL Overwrite move deleted the destination before rename committed; identical createFile left unreferenced objects; ttlSeconds on /memory was accepted and silently ignored. Co-authored-by: Cursor <cursoragent@cursor.com>
Provision rclone multimounts, MEMORY.md, and reserved env vars for attached Memory Stores, and add real E2B lifetime e2e coverage. Co-authored-by: Cursor <cursoragent@cursor.com>
a95e95d to
09190c3
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the only new commit since the prior Pullfrog review at a95e95de is a8bd1aea; the sandbox commit (09190c3a) is a byte-identical rebase of the already-reviewed a95e95de. Reviewed a8bd1aea fix(filestore):
- Made overwrite
moveFileatomic by extractingapplyMemoryUpdate/applyMemoryDeleteand adding a single-transactionMoveMemorythat soft-deletes the destination and renames the source together, fixing the prior two-transaction delete-then-rename that could lose the destination on a rename failure. - Discarded the freshly-uploaded object on a no-op (identical) rewrite via
!result.VersionCreated, so identicalcreateFileno longer leaks unreferenced S3 objects. - Rejected nonzero
ttlSecondson the memorycreateFilepath (Memory has no expiry) instead of silently ignoring it; the internal rclone writeback path sends no TTL, so there's no regression. - Added real-Postgres integration tests and a unit test covering the atomicity/no-op/overwrite-move behavior.
I traced the concurrency and indexing implications and they hold: every memory mutation path (create/update/delete/move) takes the store-row FindForUpdate first, so per-store writers serialize with consistent lock ordering, and the partial unique index on active (memory_store_uuid, path) is what MoveMemory correctly relies on.
DeepSeek Flash ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the only new commit since the prior Pullfrog review at 09190c3a is 1d5f660e fix(filestore): reject non-UTF-8 memory contents before upload:
- Added a
utf8.Valid(content)guard at the top ofupsertMemoryContent, the single shared mutation helper that every memory write path (createFile / copyFile / moveFile and the rclone / FUSE writeback) funnels through, rejecting non-UTF-8 bytes withinvalid_argumentbefore any DB write or object-store upload. - Updated the
filestore.mddesign doc with the new validity rule and its rationale (RESTcontentis already UTF-8; the Filestore/FUSE writeback path is the only injector of invalid bytes). - Added a real-Postgres test writing
{0xff,0xfe,0xfd}that asserts a 400invalid_argument, no memory row created, and no objects uploaded.
I traced the placement and coverage: the check sits before any side effect, is fail-closed, and centralizes the guard so no write path can bypass it. The test can genuinely fail (it asserts an exact error code and the absence of both DB and object-store artifacts), so it is meaningful coverage. No issues found.
DeepSeek Flash ｜ 𝕏
2 participants
Summary
MEMORY.md, and reserved env vars for attached Memory Stores.MEMORY.mdfrom Memory Store listings and block adding it via Add memory.Stack
feat/memory-attach-consolefeat/memory-filestoreReview order: 1 → 2 → 3.
Still targets
main; GitHub Files includes #329 and #330 until those merge. Review only the sandbox commit (feat(environments): mount Memory Stores into session sandboxes), orgit diff feat/memory-filestore...feat/memory-sandbox-e2e.Test plan
MEMORY.md/MEMORY.mdjust test-e2e-memory-sandbox/ workflow when e2b key presentinternal/environmentsandtests/environments_memory_*pass