feat(memory): cross-session learned memory (file-based, git-backed, BYOM-aware)#112
Conversation
…YOM-aware)
Adds a three-layer agent memory system so jcode learns durable facts from past
sessions and feeds them into future ones:
- L0 AGENTS.md (unchanged) — static, human-authored instructions.
- L1 online notes — the `memory_note` tool writes one durable fact to a
per-project inbox instantly (path-locked + secret redaction).
- L2 offline distillation — phase 1 extracts per-ended-session facts with the
small model; phase 2 consolidates via a restricted subagent, git-diff driven
with a zero-token no-op fast path and an ADD/UPDATE/DELETE/NOOP protocol.
The read path injects a size-capped memory summary into the system prompt
(TUI/ACP/web + plan mode); usage accounting feeds consolidation ranking. No
SQLite — state.json + flock, and the memory root is a git repo used for change
detection, forgetting, and rollback. BYOM cost discipline: a daily token budget
spanning both phases, a cooldown, small-model default, and a kill switch.
Storage mirrors Claude Code/Codex: global ~/.jcode/memory/, scoped per project.
Config: config.Memory{...} with zero-config defaults. CLI `jcode memory
{path,status,sync,clear}`; TUI `/memory`.
e2e: agent-eval is extended with multi-step runs, HOME fixtures/config, and
home_* oracles, plus a `memory` tier (9 cases, 9/9 pass on glm-5.1). Unit tests
cover redaction, the path guard, concurrency, UTF-8 truncation, and the git
no-op fast path.
Docs: site/docs/overview/learned-memory.md plus config/commands cross-links.
Design + research live in internal-doc/agent-memory-{design,e2e-plan}.md and
memory-research-2026-07.md.
Reviewed adversarially across 5 dimensions; fixes include git churn breaking
the no-op fast path, phase-2 budget/cooldown gaps, a broken usage-feedback
loop, a WriteNote concurrency race, redaction gaps, and UTF-8-safe truncation.
Generated with Jack AI bot
📝 WalkthroughWalkthroughThis PR adds cross-session Project Memory: persistent local storage, redaction and path safety, memory injection into prompts, an offline distillation pipeline, runtime wiring, CLI/TUI commands, documentation, and agent-eval coverage for multi-step memory flows. ChangesProject Memory feature
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ACP/Interactive/Web session
participant memory_note tool
participant mempipeline.Run
participant memory store
participant consolidation agent
ACP/Interactive/Web session->>mempipeline.Run: MaybeStartBackground(cfg, pwd)
ACP/Interactive/Web session->>memory_note tool: write durable fact
memory_note tool->>memory store: WriteNote
mempipeline.Run->>memory store: LoadState / TryLockPipeline
mempipeline.Run->>consolidation agent: phase 1 extraction / phase 2 consolidation
consolidation agent-->>memory store: summaries, state, baseline commit
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
cnjack
left a comment
cnjack
left a comment
There was a problem hiding this comment.
Staff-engineer review
Reviewed the core new package (internal/memory, internal/memory/pipeline), the tool/command surface (memory_note, jcode memory {path,status,sync,clear}, TUI /memory), and all wiring points (agent.go, interactive.go, acp.go, web.go, config.go). The security posture (path guard, redaction, git-dir/lock-file blocking for the restricted consolidation subagent) is solid and clearly already adversarially reviewed, per the PR description. Found one real, unaddressed concurrency/data-integrity gap and a related weaker instance of the same class.
Finding 1 — TUI /memory clear has no pipeline lock at all
Impact: interactive.go calls mempipeline.MaybeStartBackground(cfg, pwd) on essentially every interactive session start (gated only by cooldown/budget, not user action). That goroutine may be mid-flight — reading/writing notes/, session_summaries/, state.json, or running git add/git commit inside the scope's .git — when the user types /memory clear. This is a real risk in normal usage, not a hypothetical, since the pipeline auto-starts on nearly every session.
Evidence: internal/tui/input_views.go, handleMemoryInput, case "clear":
case "clear":
if err := os.RemoveAll(root); err != nil {No call to memory.TryLockPipeline anywhere in this path. Contrast with the CLI equivalent (internal/command/memory.go, newMemoryClearCmd), which does take the lock first — the codebase already recognizes this hazard elsewhere, just not here. Racing a git commit mid-write can corrupt the baseline repo; racing phase 1/2 writes can silently recreate files under a directory the user just tried to wipe, defeating "clear."
Suggested fix: Acquire memory.TryLockPipeline(root) before RemoveAll in the TUI clear case and refuse with a clear message if the pipeline is running, mirroring the CLI path — but see Finding 2 for why the lock must be held through the delete, not released beforehand.
Finding 2 — CLI jcode memory clear releases the pipeline lock before deleting
Impact: The comment directly above the code says "Don't delete out from under a running pipeline: take its lock first," but the lock is released immediately after acquisition, before RemoveAll runs — reopening the exact TOCTOU window the lock exists to close. A pipeline invocation from another process (or another interactive session in the same project) can grab the now-free lock and start writing in the gap before the delete executes.
Evidence: internal/command/memory.go, newMemoryClearCmd:
release, ok, lerr := memory.TryLockPipeline(root)
if lerr == nil && !ok {
return fmt.Errorf("memory pipeline is running for this project; try again shortly")
}
if release != nil {
release() // <-- lock dropped here
}
fmt.Printf("clearing project memory: %s\n", root)
return os.RemoveAll(root) // <-- unprotected window starts hereSuggested fix: Defer the release until after RemoveAll completes:
release, ok, lerr := memory.TryLockPipeline(root)
if lerr == nil && !ok {
return fmt.Errorf(...)
}
if release != nil {
defer release()
}
return os.RemoveAll(root)Other areas checked (no issues found at high confidence)
- Path-traversal guarding (
withinRoot): symlink resolution on existing prefixes, encoded-traversal rejection (%2e/%2f/%5c). - Restricted consolidation subagent's tool/argument allowlist — verified
pathGuardMiddleware'spathKeys(file_path,path,dir,directory,root) actually covers every path-bearing argument used by the four tools (read,grep,write,edit) it's granted;.git/and lock-file writes are separately blocked. - Secret redaction ordering in
WriteNote— redaction runs before filename-slug derivation, so secrets can't leak into filenames. O_EXCLunique-filename loop inWriteNotecorrectly handles same-second concurrent writers.- Daily token-budget/cooldown gates and the git-diff-driven no-op fast path.
Overall Risk: Medium
The memory pipeline runs automatically on nearly every session, so the two clear-command races aren't edge cases — worth fixing before merge.
Generated by Claude Code
- phase1: os.MkdirAll shadowed err so the summary WriteFile error was checked against the wrong variable and silently dropped (ineffassign) — use a dedicated werr. - firstJSONObject: the `break` after an invalid balanced object broke the switch, not the scan loop, so an invalid-then-valid object sequence never found the valid one (staticcheck SA4011) — use a labeled `break scan`, plus a regression test. - note: check handle.Close() (errcheck) — close explicitly after the write to surface flush errors, defer covers error paths. - git: drop ensureBaseline's unused bool return (unparam); update callers. Generated with Jack AI bot
The .githooks/pre-commit existed but was never active (core.hooksPath unset), which is why the golangci-lint failure only surfaced in CI. Split into: - pre-commit: fast gofmt/goimports gate on staged Go files only (instant, keeps commits snappy). Portable to macOS's stock bash 3.2 (no mapfile). - pre-push: mirrors CI's Go job — go build, go vet, golangci-lint with the same --new-from-rev=origin/main gating (so pre-existing lint debt doesn't block), and go test. Runs once per push. Enable with `make setup-hooks`. Bypass a hook with --no-verify; skip only tests via SKIP_TESTS=1 git push. Generated with Jack AI bot
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
agent-eval/suite/orchestrate.py (1)
253-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
harness_rcsemantics differ between step kinds.For "cli" steps,
harness_rcis set to the invoked jcode binary's own exit code (Line 267), but for "prompt" steps it's the ACP test-harness wrapper's exit code (Line 287) — not jcode's. Downstream tooling readingrecord["harness_rc"]can't distinguish "harness crashed" from "my cli subcommand returned non-zero" without also inspectingstep_records[].kind. Consider a distinct field (e.g.cli_rc) for cli-step failures to avoid conflating the two.Also applies to: 285-297
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent-eval/suite/orchestrate.py` around lines 253 - 268, The step failure status is overloaded in orchestrate.py: the run loop in the cli-step branch stores the jcode subcommand exit code into harness_rc, while prompt steps use harness_rc for the wrapper’s own exit code. Update the step-record/result handling in the cli branch (around subprocess.run, step_records.append, and the rc != 0 failure path) to record the subcommand exit code in a separate field such as cli_rc, and keep harness_rc reserved for harness-level failures so downstream consumers can reliably tell them apart.internal/memory/inject.go (3)
17-33: 📐 Maintainability & Code Quality | 🔵 TrivialConsider adding unit tests for
BuildInjection.This function has non-trivial budget/truncation logic (per-source truncation + a global hard cap, empty-state short-circuit) but no test file is included in this cohort. Given it directly controls what gets injected into every model call, targeted tests (empty state, hard-cap truncation, index-only case) would help guard against regressions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/memory/inject.go` around lines 17 - 33, Add unit tests for BuildInjection to cover its budget and empty-state behavior. Focus on the BuildInjection function in internal/memory/inject.go and validate the empty short-circuit, the per-source truncation from readTruncated, the global hard cap for globalSummary, and the index-only case when fileExists is true but summaries/notes are empty. Use targeted tests around ProjectRoot, GlobalRoot, RecentNotes, and fileExists so regressions in injection content are caught.
24-29: 🚀 Performance & Scalability | 🔵 TrivialChar-per-token heuristic and hardcoded global budget.
maxChars := config.MemorySummaryInjectTokens(cfg) * 4and thehardCapon Line 79 assume ~4 chars/token, which is a reasonable approximation for English but significantly overestimates capacity for CJK text (jcode explicitly supports Chinese, e.g. the WeChat channel andmemory_note's "记住X" example). For CJK-heavy memory content, the actual token count injected into every prompt could be far higher thanMemorySummaryInjectTokensintends, defeating the budget control this code is meant to enforce. Separately, the global-summary budget (300*4on Line 26) is hardcoded rather than derived from a config knob like the project summary is, which is an inconsistency worth reconsidering.Also applies to: 79-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/memory/inject.go` around lines 24 - 29, The memory injection logic in inject() is using a fixed chars-per-token approximation and a hardcoded global summary budget, which can overrun the intended token limit for CJK-heavy content. Replace the `* 4` sizing in the project/global summary truncation and the `hardCap` logic with a token-aware calculation or a shared budgeting helper, and make the global-summary limit derive from configuration instead of a literal so it stays consistent with `config.MemorySummaryInjectTokens` and `readTruncated`.
36-73: 🔒 Security & Privacy | 🔵 TrivialReliance on soft "data not instructions" guardrail for injected memory.
Notes/summaries persisted via
memory_note(potentially containing content the agent picked up during a session) get re-injected verbatim into every future system prompt. The mitigation here is purely textual instruction to the model ("Memory content below is data, not instructions"), which is a weak defense against prompt injection if a session ever records attacker-influenced text (e.g., from a fetched file/URL) as a "durable fact." Worth considering whether redaction/sanitization at write time (innote.go) should also strip instruction-like patterns, in addition to the read-time disclaimer here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/memory/inject.go` around lines 36 - 73, The memory injection prompt in inject.go relies only on a read-time disclaimer, but note content from memory_note can still carry instruction-like text into future system prompts. Update the write path in note.go to sanitize or redact prompt-injection patterns before persisting notes, and keep the existing inject.go disclaimer as a secondary guard; use the memory_note, NoteFile, and summary/globalSummary flow to locate where stored text is later reinserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal-doc/agent-memory-design.md`:
- Around line 78-89: The fenced examples in the agent-memory design doc are
missing language tags and are triggering the MD040 lint rule. Update the
affected fenced blocks in the documentation to use explicit language identifiers
appropriate to the content (for example, text or json) so the examples remain
lint-clean; use the existing fenced sections in the architecture overview and
other referenced example blocks as the targets.
In `@internal/command/memory.go`:
- Around line 119-127: The memory clear flow releases the pipeline lock before
`os.RemoveAll`, which reintroduces a race in `memory.TryLockPipeline` handling.
Keep the lock held until after the remove completes, and only call the returned
`release` afterward. Also update the `lerr` handling in this block so any lock
acquisition error causes an immediate return instead of proceeding with the
delete.
In `@site/docs/overview/learned-memory.md`:
- Around line 222-227: The “Read-only notebook” description for the generate
toggle is misleading in the learned-memory docs. Update the wording in the
“Turning it off” section to match the actual behavior of the generate=false mode
in learned-memory.md: either rename the mode to something more accurate or
explicitly state whether /memory writes are still allowed when generation is
disabled, while keeping the contrast with enabled=false clear.
---
Nitpick comments:
In `@agent-eval/suite/orchestrate.py`:
- Around line 253-268: The step failure status is overloaded in orchestrate.py:
the run loop in the cli-step branch stores the jcode subcommand exit code into
harness_rc, while prompt steps use harness_rc for the wrapper’s own exit code.
Update the step-record/result handling in the cli branch (around subprocess.run,
step_records.append, and the rc != 0 failure path) to record the subcommand exit
code in a separate field such as cli_rc, and keep harness_rc reserved for
harness-level failures so downstream consumers can reliably tell them apart.
In `@internal/memory/inject.go`:
- Around line 17-33: Add unit tests for BuildInjection to cover its budget and
empty-state behavior. Focus on the BuildInjection function in
internal/memory/inject.go and validate the empty short-circuit, the per-source
truncation from readTruncated, the global hard cap for globalSummary, and the
index-only case when fileExists is true but summaries/notes are empty. Use
targeted tests around ProjectRoot, GlobalRoot, RecentNotes, and fileExists so
regressions in injection content are caught.
- Around line 24-29: The memory injection logic in inject() is using a fixed
chars-per-token approximation and a hardcoded global summary budget, which can
overrun the intended token limit for CJK-heavy content. Replace the `* 4` sizing
in the project/global summary truncation and the `hardCap` logic with a
token-aware calculation or a shared budgeting helper, and make the
global-summary limit derive from configuration instead of a literal so it stays
consistent with `config.MemorySummaryInjectTokens` and `readTruncated`.
- Around line 36-73: The memory injection prompt in inject.go relies only on a
read-time disclaimer, but note content from memory_note can still carry
instruction-like text into future system prompts. Update the write path in
note.go to sanitize or redact prompt-injection patterns before persisting notes,
and keep the existing inject.go disclaimer as a secondary guard; use the
memory_note, NoteFile, and summary/globalSummary flow to locate where stored
text is later reinserted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 473c872c-18d2-4c64-b99a-25bd5dfa6584
📒 Files selected for processing (42)
agent-eval/suite/orchestrate.pyagent-eval/suite/testcases.jsonagent-eval/suite/verify.pycmd/jcode/main.gointernal-doc/agent-memory-design.mdinternal-doc/agent-memory-e2e-plan.mdinternal-doc/memory-research-2026-07.mdinternal/agent/agent.gointernal/command/acp.gointernal/command/interactive.gointernal/command/memory.gointernal/command/memory_sync.gointernal/command/web.gointernal/config/config.gointernal/memory/filelock_unix.gointernal/memory/filelock_windows.gointernal/memory/guard.gointernal/memory/inject.gointernal/memory/memory.gointernal/memory/memory_test.gointernal/memory/note.gointernal/memory/pipeline/git.gointernal/memory/pipeline/phase1.gointernal/memory/pipeline/phase2.gointernal/memory/pipeline/pipeline.gointernal/memory/pipeline/pipeline_test.gointernal/memory/pipeline/prompts.gointernal/memory/redact.gointernal/memory/state.gointernal/memory/usage.gointernal/prompts/prompts.gointernal/tools/memory_note.gointernal/tui/input_views.gointernal/tui/update.gosite/docs/commands.mdsite/docs/configuration.mdsite/docs/overview/buddy.mdsite/docs/overview/channels.mdsite/docs/overview/context-memory.mdsite/docs/overview/ide-integration.mdsite/docs/overview/learned-memory.mdsite/docs/overview/mcp.md
| release, ok, lerr := memory.TryLockPipeline(root) | ||
| if lerr == nil && !ok { | ||
| return fmt.Errorf("memory pipeline is running for this project; try again shortly") | ||
| } | ||
| if release != nil { | ||
| release() | ||
| } | ||
| fmt.Printf("clearing project memory: %s\n", root) | ||
| return os.RemoveAll(root) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e go filelock internal/memory --exec cat -n {}
rg -nP -C3 'func TryLockPipeline|func.*[Ll]ock' internal/memoryRepository: cnjack/jcode
Length of output: 7252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== internal/command/memory.go =="
cat -n internal/command/memory.go | sed -n '1,220p'
echo
echo "== internal/memory/state.go =="
cat -n internal/memory/state.go | sed -n '1,180p'
echo
echo "== any clear/removeall related tests/usages =="
rg -n -C 3 'RemoveAll|TryLockPipeline|clear project memory|--all|--global|state.lock' internal/command internal/memoryRepository: cnjack/jcode
Length of output: 19093
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import os, tempfile, fcntl, shutil, time, subprocess, textwrap, pathlib, sys
base = tempfile.mkdtemp(prefix="locktest-")
root = os.path.join(base, "scope")
os.makedirs(root, exist_ok=True)
lock = os.path.join(root, ".state.lock")
f = open(lock, "w+")
fcntl.flock(f, fcntl.LOCK_EX | fcntl.LOCK_NB)
print("locked:", os.path.exists(lock), os.path.isdir(root))
try:
shutil.rmtree(root)
print("rmtree succeeded while locked")
except Exception as e:
print("rmtree failed:", type(e).__name__, e)
print("after:", os.path.exists(lock), os.path.exists(root))
fcntl.flock(f, fcntl.LOCK_UN)
f.close()
shutil.rmtree(base, ignore_errors=True)
PYRepository: cnjack/jcode
Length of output: 217
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n internal/command/memory.go | sed -n '70,160p'Repository: cnjack/jcode
Length of output: 3661
🌐 Web query:
Go os.OpenFile Windows share delete open file RemoveAll locked file directory behavior
💡 Result:
In Go on Windows, the behavior of file operations like os.Remove and os.RemoveAll is constrained by Windows-specific file locking and sharing semantics, which differ significantly from Unix-like systems [1][2][3]. File Deletion and Locking On Windows, a file that is currently open by a process generally cannot be deleted by another process (or even the same process) unless it was opened with appropriate sharing flags [4][5][2]. Standard os.Open or os.OpenFile calls do not automatically include the FILE_SHARE_DELETE flag [1][4]. Consequently, if a file is held open without this share mode, attempts to call os.Remove or os.RemoveAll on that file will typically fail with a sharing violation or access denied error [4][5][3]. Modern Windows Behavior Recent versions of Windows (Windows 10 RS1 and later) support FILE_DISPOSITION_POSIX_SEMANTICS, which allows for Unix-like deletion behavior (deleting a file even if it is currently open) [6]. Go's os.Remove and os.RemoveAll have been updated to utilize these POSIX semantics when supported by the underlying Windows version and file system [6]. However, this support is not universal across all Windows versions and file systems, leading to historical differences in behavior [7][6]. os.RemoveAll and Read-Only Files os.RemoveAll is designed to recursively delete directory trees. In recent Go versions (notably Go 1.25+), there have been significant updates and some regressions regarding how os.RemoveAll handles read-only files and access permissions on Windows [8][9][10]. Historically, os.Remove would explicitly clear the read-only attribute before attempting deletion if the initial attempt failed [11]. Recent improvements have aimed to make this process more robust by handling read-only attributes more effectively, though these changes have occasionally introduced regressions related to access control lists (ACLs) and permission checks when requesting additional access rights like SYNCHRONIZE [9][7]. Workarounds If you encounter issues with file deletion on Windows due to locks: 1. Ensure all file handles are closed: This is the most reliable way to ensure deletion succeeds [1][4]. 2. Use FILE_SHARE_DELETE: If you must open a file that you intend to delete later while it remains open, you can use syscall.OpenFile (or similar low-level syscalls) to pass the FILE_SHARE_DELETE flag, provided the API allows it [4][2]. 3. Handle errors: Applications should be prepared to handle "Access is denied" or "sharing violation" errors, as these are expected Windows behaviors when resources are busy [12][8][9]. For the most up-to-date behavior, consult the Go repository issues, specifically those tracking Windows-specific regressions in the os package, as this is an area of active development and platform-specific adjustment [9][10][6].
Citations:
- 1: proposal: os: Create/Open/OpenFile() set FILE_SHARE_DELETE on windows golang/go#32088
- 2: https://stackoverflow.com/questions/23897174/is-it-possible-to-open-a-file-allowing-another-processes-to-delete-this-file
- 3: os: Remove/RemoveAll should remove read-only files on Windows golang/go#9606
- 4: proposal: syscall: define Windows O_ALLOW_DELETE for use in os.OpenFile golang/go#34681
- 5: https://groups.google.com/g/golang-nuts/c/aRvSo3iKvJY
- 6: proposal: os: support deleting files with POSIX semantics for Windows 2019 and older golang/go#78072
- 7: golang/go@b31dc77
- 8: os: on windows RemoveAll removing directories containing read-only files errors with
unlinkat ... Access is deniedgolang/go#75922 - 9: os: Go 1.25.x regression on RemoveAll for windows golang/go#77402
- 10: os: on windows RemoveAll removing directories containing read-only files errors with
unlinkat ... Access is denied, ReOpenFile error handling followup [1.25 backport] golang/go#76360 - 11: https://github.com/golang/go/blob/99d4927f/src/os/file_windows.go
- 12: os: file write blocks subsequent Open() with "sharing violation" in OneDrive golang/go#35219
🌐 Web query:
Unix unlink file while flock held safe open file descriptor lock persists
💡 Result:
Yes, the flock lock persists even if the file is unlinked while the lock is held [1][2]. In Unix-like systems, when you unlink a file, you are merely removing the directory entry (the name) associated with that file's inode [3][2]. The file itself—including its data and the associated "open file description"—remains in existence as long as at least one process holds an open file descriptor referring to it [3][2]. Because flock locks are associated with the "open file description" rather than the file path or name, the lock remains active and effective for any process that currently holds an open file descriptor to that specific inode [4][5][6]. Key implications: 1. Persistence: The lock does not disappear when unlink is called [2]. Other processes that also have the file open (or that open it before it is fully deleted) will continue to respect the lock because they are still interacting with the same underlying open file description or inode [1][2]. 2. Cleanup: The file's inode and data are only fully removed from the filesystem when all file descriptors referring to that file are closed [3][2]. 3. Race Conditions: If you are using a file as a lockfile, be aware that while unlink removes the name, it does not release the lock [1][2]. If your goal is to manage a mutex, a common strategy to avoid race conditions is to verify the file's identity (e.g., using fstat to compare inode numbers) after acquiring the lock to ensure you are locking the intended file and not a new file that was created with the same name after the original was unlinked [1].
Citations:
- 1: https://stackoverflow.com/questions/17708885/flock-removing-locked-file-without-race-condition
- 2: https://stackoverflow.com/questions/34313774/c-c-flock-as-mutex-on-linux-not-robust-to-file-delete
- 3: https://linuxman7.org/linux/man-pages/man2/unlink.2.html
- 4: https://linuxman7.org/linux/man-pages/man2/flock.2.html
- 5: https://unix.stackexchange.com/questions/594027/handling-of-stale-file-locks-in-linux-and-robust-usage-of-flock
- 6: https://man.archlinux.org/man/flock.2.txt
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 'GlobalRoot\(|TryLockPipeline\(|ProjectRoot\(' internalRepository: cnjack/jcode
Length of output: 10008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n go.mod | sed -n '1,80p'Repository: cnjack/jcode
Length of output: 4237
Hold the pipeline lock through RemoveAll. Releasing it first reopens a race where another pipeline can reacquire .pipeline.lock and write into the scope before the clear finishes. Return on lerr too; proceeding on a lock error deletes without the intended guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/command/memory.go` around lines 119 - 127, The memory clear flow
releases the pipeline lock before `os.RemoveAll`, which reintroduces a race in
`memory.TryLockPipeline` handling. Keep the lock held until after the remove
completes, and only call the returned `release` afterward. Also update the
`lerr` handling in this block so any lock acquisition error causes an immediate
return instead of proceeding with the delete.
Source: Coding guidelines
Review (CodeRabbit) follow-ups: - clear race (Major): `memory clear` acquired the pipeline lock, released it, then RemoveAll — a TOCTOU gap where a pipeline could start mid-clear, plus a Windows sharing-violation on the still-open lock file. Add memory.ClearScope: refuse if the pipeline holds the lock, otherwise hold it across the delete and retry after release (Windows). Wire both the CLI and TUI clear through it; add TestClearScope (busy / success / missing-scope). - docs (Minor): "read-only notebook" for generate=false was misleading (online notes still write). Reword to "manual notebook" and spell out that reading, injection, and memory_note stay on — only distillation is disabled. - docs (Minor, MD040): fenced blocks in the design doc now carry language tags. Also: translate the internal-doc memory docs (design, research, e2e-plan) from Chinese to English so the whole PR is community-reviewable — structure, tables, code blocks, [[wikilinks]], paths, and config keys preserved verbatim. Generated with Jack AI bot
|
Addressed the review in
Also translated the internal design docs ( |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.githooks/pre-commit (1)
10-19: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing
pipefailmasks upstreamgit difffailures.Without
pipefail, the exit status of thegit diff --cached | while ...pipeline is only that of the trailingwhileloop. Ifgit diff --cacheditself fails (corrupted index, git error), the loop simply reads no lines,filesends up empty, and the hook exits 0 on line 20 — silently skipping the formatting gate instead of failing loudly. Bash 3.2 (the portability target mentioned in the header comment) does supportpipefail, so it can be enabled here without breaking the stated compatibility goal.🛠️ Proposed fix
-set -eu +set -eu -o pipefail🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.githooks/pre-commit around lines 10 - 19, The pre-commit hook pipeline in the files collection block can hide failures from git diff because only the while loop exit status is checked. Update the hook near the files assignment to enable pipefail alongside set -eu so git diff --cached errors propagate and the hook fails instead of silently skipping formatting checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.githooks/pre-commit:
- Around line 10-19: The pre-commit hook pipeline in the files collection block
can hide failures from git diff because only the while loop exit status is
checked. Update the hook near the files assignment to enable pipefail alongside
set -eu so git diff --cached errors propagate and the hook fails instead of
silently skipping formatting checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2be4b737-d78c-46b3-8126-171cb4309fcb
📒 Files selected for processing (16)
.githooks/pre-commit.githooks/pre-pushMakefileinternal-doc/agent-memory-design.mdinternal-doc/agent-memory-e2e-plan.mdinternal-doc/memory-research-2026-07.mdinternal/command/memory.gointernal/memory/memory_test.gointernal/memory/note.gointernal/memory/pipeline/git.gointernal/memory/pipeline/phase1.gointernal/memory/pipeline/phase2.gointernal/memory/pipeline/pipeline_test.gointernal/memory/state.gointernal/tui/input_views.gosite/docs/overview/learned-memory.md
✅ Files skipped from review due to trivial changes (1)
- site/docs/overview/learned-memory.md
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/memory/pipeline/git.go
- internal/command/memory.go
- internal/memory/state.go
- internal/memory/note.go
- internal/tui/input_views.go
- internal/memory/memory_test.go
- internal/memory/pipeline/phase2.go
- internal/memory/pipeline/pipeline_test.go
1 participant
What & why
jcode had only static memory (AGENTS.md) and within-session memory (compaction) — nothing that learns across sessions. This adds Project Memory: jcode distills durable facts (preferences, conventions, pitfalls, workflows) from past sessions and feeds them back into future ones. Modeled on Claude Code's auto-memory and Codex's memory pipeline, adapted for jcode's pure-Go, no-SQLite, BYOM (bring-your-own-model) reality.
Design (three layers)
~/.jcodememory_notetool writes one durable fact to an inbox instantly; path-locked + secret-redacted~/.jcode/memory/projects/<slug>/notes/MEMORY.md+memory_summary.mdstate.json+flock; the memory root is a git repo for change detection, forgetting, and rollback.-por remote sessions.~/.jcode/memory/, scoped per project) — matches Claude Code/Codex; keeps personal learned data out of the repo. Shareable/committable project knowledge stays in AGENTS.md.Surface
config.Memory{...}, zero-config defaults (all documented).jcode memory {path,status,sync,clear}. TUI:/memory,/memory sync,/memory clear.internal/memory(+pipelinesubpkg); toolmemory_note; usage middleware; injection inGetSystemPrompt/GetPlanSystemPrompt; wiring in interactive/acp/web.Testing
internal/memory/..., all green): redaction (incl. JSON-quoted/URL/github_pat_), path-guard traversal +.git/block, concurrent note writes, state flock, UTF-8-safe truncation, git no-op fast path, phase-1 selection/budget, expire+rank.home_*oracles; added amemorytier — 9/9 cases pass (explicit remember, cross-session recall, summary injection, redaction, prompt-injection resistance, write discipline, kill switch, phase-1 extract, phase-2 consolidate incl. no-op-on-rerun).Adversarial review
Reviewed across 5 dimensions (correctness/concurrency/security/cost/integration). All confirmed findings fixed, including:
.gitignorein the scope root; phase-2 had no budget gate and failure didn't set cooldown → retry storm.TruncateRunes); greedy JSON extraction;.git/writable to the consolidation agent.Reviewer notes
internal-doc/{dynamic-workflow-prd,model-research,sdk-hooks-design,usage-stats}.md) and a stray rootMEMORY.md(not created here).internal-doc/agent-memory-design.md,agent-memory-e2e-plan.md,memory-research-2026-07.md.Generated with Jack AI bot
Summary by CodeRabbit
/memorycommands (status, path, clear, and sync) and persistent memory notes.