memory: ship rem CLI; sync installs memory tools - #142
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideAdds a new Sequence diagram for guarded vault synchronizationsequenceDiagram
participant Rem
participant Sync as knowledge-sync
participant Vault as VaultWorktree
participant Remote as GitRemote
Rem->>Sync: cmdSync()
Sync->>Vault: git status --short
Sync->>Vault: git rev-parse --abbrev-ref HEAD
alt wrong branch
Sync-->>Rem: refuse sync
else expected branch
Sync->>Remote: git fetch remote branch
Sync->>Vault: git merge --no-edit remote/branch
alt merge conflict
Sync->>Vault: git branch conflict-branch
Sync->>Vault: git merge --abort
Sync-->>Rem: report conflict
else merge succeeds
Sync->>Remote: git push remote branch
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, and 6 other issues
Security issues:
- Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="memory/tools/rem/go.mod" line_range="3" />
<code_context>
+module rem
+
+go 1.26.1
</code_context>
<issue_to_address>
**issue (bug_risk):** `dotagents sync` fails to build `rem` on machines running the repository's supported Go 1.24 toolchain because this module requires Go 1.26.1. The new installer propagates that build error, so sync no longer completes for ordinary hosts with the available toolchain.
**Triggers:** When Go 1.24.x is installed and automatic toolchain download is unavailable or disabled.
**Suggested fix:** Use a Go version compatible with the repository's declared toolchain, or lower the module directive to the minimum version required by the code.
```suggestion
go 1.24.2
```
</issue_to_address>
### Comment 2
<location path="memory/tools/knowledge-sync/main.go" line_range="66-69" />
<code_context>
}
}
+ if out, err := git(repo, "rev-parse", "--abbrev-ref", "HEAD"); err != nil {
+ fatal("current branch", err, out)
+ } else if current := strings.TrimSpace(out); current != branch {
+ fatal("branch guard", fmt.Errorf("checkout is on %q, not %q", current, branch),
+ "knowledge-sync: refusing to sync: worktree checked out on "+current+", expected "+branch+"\n")
+ }
</code_context>
<issue_to_address>
**issue (broader_impact):** The branch guard runs after the existing dirty-tree handling has staged and committed all changes. A dirty worktree on the wrong branch is therefore committed to that wrong branch before the command refuses to fetch, merge, or push, directly violating the guard's purpose.
**Triggers:** When the vault worktree is dirty and checked out on a branch other than `KNOWLEDGE_BRANCH`.
**Suggested fix:** Validate the current branch before the status/add/commit block.
</issue_to_address>
### Comment 3
<location path="memory/tools/knowledge-sync/main.go" line_range="78-79" />
<code_context>
- if abortOut, abortErr := git(repo, "merge", "--abort"); abortErr != nil {
- fatal("git merge abort", abortErr, abortOut)
- }
+ git(repo, "branch", conflict)
+ git(repo, "merge", "--abort")
fatal("git merge", err, "created conflict branch "+conflict+"\n")
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Errors from creating the conflict branch and aborting the merge are discarded, so a failed `git branch` or `git merge --abort` still reports only the original merge failure and can leave the repository in a conflicted merge state without the promised recovery branch.
**Triggers:** When conflict recovery itself fails, such as when branch creation is rejected or the repository cannot abort the merge.
**Suggested fix:** Restore explicit error checks for both commands and call `fatal` with the recovery step that failed.
```suggestion
if out, err := git(repo, "branch", conflict); err != nil {
fatal("git branch", err, out)
}
if out, err := git(repo, "merge", "--abort"); err != nil {
fatal("git merge --abort", err, out)
}
```
</issue_to_address>
### Comment 4
<location path="cmd/dotagents/memory_tools.go" line_range="72-87" />
<code_context>
+ return false, err
+ }
+ for _, entry := range entries {
+ if entry.IsDir() || (!strings.HasSuffix(entry.Name(), ".go") && entry.Name() != "go.mod") {
+ continue
+ }
+ info, err := entry.Info()
</code_context>
<issue_to_address>
**issue (bug_risk):** The freshness scan ignores `.go` files in subdirectories, so changing an imported local package does not advance `newest`; an existing binary is then treated as fresh and the installer skips rebuilding it.
**Triggers:** When a memory tool has Go source in a nested package directory and that source changes.
**Suggested fix:** Walk the module recursively, or use Go's package/build metadata to determine whether any source dependency is newer than the installed binary.
```suggestion
err := filepath.Walk(srcDir, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() || (!strings.HasSuffix(info.Name(), ".go") && info.Name() != "go.mod") {
return nil
}
if info.ModTime().After(newest) {
newest = info.ModTime()
}
return nil
})
if err != nil {
return false, err
}
```
</issue_to_address>
### Comment 5
<location path="memory/tools/rem/main.go" line_range="62-83" />
<code_context>
+ return fmt.Errorf("usage: rem add [-src harness] \"fact\"")
+ }
+
+ dir := filepath.Join(knowledgeDir(), "ai")
+ existing, err := loadCandidates(dir)
+ if err != nil {
+ return err
+ }
+ fp := normalize(fact)
+ for _, c := range existing {
+ if normalize(c.Text) == fp {
+ fmt.Printf("rem add: already captured on %s: %s\n", c.Day, c.Text)
+ return nil
+ }
+ }
+
+ if err := os.MkdirAll(dir, 0o755); err != nil {
+ return err
+ }
+ path := filepath.Join(dir, today()+".md")
+ fresh := false
+ if _, err := os.Stat(path); os.IsNotExist(err) {
+ fresh = true
+ }
+ f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644)
+ if err != nil {
+ return err
</code_context>
<issue_to_address>
**issue (bug_risk):** `cmdAdd` loads existing candidates and later opens today's file for append without any lock or atomic check, so concurrent `rem add` processes can both observe a missing fact and append it, defeating the documented cross-file deduplication.
**Triggers:** When two capture hooks invoke `rem add` concurrently for the same fact.
**Suggested fix:** Serialize candidate read-and-append with a lock in the vault or use an atomic update mechanism.
</issue_to_address>
### Comment 6
<location path="memory/tools/rem/main.go" line_range="167-166" />
<code_context>
+ }
+ bin = filepath.Join(home, ".local", "bin", "knowledge-sync")
+ }
+ if _, err := os.Stat(bin); err != nil {
+ return fmt.Errorf("knowledge-sync not found at %s: %w", bin, err)
+ }
+ cmd := exec.Command(bin)
+ cmd.Stdin, cmd.Stdout, cmd.Stderr = os.Stdin, os.Stdout, os.Stderr
</code_context>
<issue_to_address>
**issue (bug_risk):** `rem sync` ignores `GOBIN` and falls back only to `~/.local/bin/knowledge-sync`, even though `dotagents sync` installs knowledge-sync into the configured `GOBIN`; with a custom GOBIN, the wrapper cannot find the binary it just provisioned.
**Triggers:** When `GOBIN` is set to a directory other than `~/.local/bin` and `REM_SYNC_BIN` is unset.
**Suggested fix:** Resolve the default sync binary from `GOBIN` before falling back to `~/.local/bin`, or have the installer/configuration provide the path consistently.
</issue_to_address>
### Comment 7
<location path="memory/tools/rem/main.go" line_range="170" />
<code_context>
cmd := exec.Command(bin)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>Sourcery assessment
Needs a human reviewer. 7 findings to address first, and the new rem dream --apply can rewrite and commit sessions/knowledge.md, dropping sections based on normalized duplicate bodies; a misclassification would alter persisted knowledge beyond the immediate command. The change creates a timestamped backup and a local commit, so the affected file is bounded and recoverable by restoring the backup or reverting the commit, while the rest of the CLI has ordinary runtime-bug consequences.
Blocking findings: memory/tools/rem/go.mod:3, memory/tools/knowledge-sync/main.go:69, memory/tools/knowledge-sync/main.go:79, cmd/dotagents/memory_tools.go:87, memory/tools/rem/main.go:83, and 2 more
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -0,0 +1,3 @@ | |||
| module rem | |||
|
|
|||
| go 1.26.1 | |||
There was a problem hiding this comment.
issue (bug_risk): dotagents sync fails to build rem on machines running the repository's supported Go 1.24 toolchain because this module requires Go 1.26.1. The new installer propagates that build error, so sync no longer completes for ordinary hosts with the available toolchain.
Triggers: When Go 1.24.x is installed and automatic toolchain download is unavailable or disabled.
Suggested fix: Use a Go version compatible with the repository's declared toolchain, or lower the module directive to the minimum version required by the code.
| go 1.26.1 | |
| go 1.24.2 |
| fatal("current branch", err, out) | ||
| } else if current := strings.TrimSpace(out); current != branch { | ||
| fatal("branch guard", fmt.Errorf("checkout is on %q, not %q", current, branch), | ||
| "knowledge-sync: refusing to sync: worktree checked out on "+current+", expected "+branch+"\n") |
There was a problem hiding this comment.
issue (broader_impact): The branch guard runs after the existing dirty-tree handling has staged and committed all changes. A dirty worktree on the wrong branch is therefore committed to that wrong branch before the command refuses to fetch, merge, or push, directly violating the guard's purpose.
Triggers: When the vault worktree is dirty and checked out on a branch other than KNOWLEDGE_BRANCH.
Suggested fix: Validate the current branch before the status/add/commit block.
| git(repo, "branch", conflict) | ||
| git(repo, "merge", "--abort") |
There was a problem hiding this comment.
issue (bug_risk): Errors from creating the conflict branch and aborting the merge are discarded, so a failed git branch or git merge --abort still reports only the original merge failure and can leave the repository in a conflicted merge state without the promised recovery branch.
Triggers: When conflict recovery itself fails, such as when branch creation is rejected or the repository cannot abort the merge.
Suggested fix: Restore explicit error checks for both commands and call fatal with the recovery step that failed.
| git(repo, "branch", conflict) | |
| git(repo, "merge", "--abort") | |
| if out, err := git(repo, "branch", conflict); err != nil { | |
| fatal("git branch", err, out) | |
| } | |
| if out, err := git(repo, "merge", "--abort"); err != nil { | |
| fatal("git merge --abort", err, out) | |
| } |
| entries, err := os.ReadDir(srcDir) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| for _, entry := range entries { | ||
| if entry.IsDir() || (!strings.HasSuffix(entry.Name(), ".go") && entry.Name() != "go.mod") { | ||
| continue | ||
| } | ||
| info, err := entry.Info() | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if info.ModTime().After(newest) { | ||
| newest = info.ModTime() | ||
| } | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): The freshness scan ignores .go files in subdirectories, so changing an imported local package does not advance newest; an existing binary is then treated as fresh and the installer skips rebuilding it.
Triggers: When a memory tool has Go source in a nested package directory and that source changes.
Suggested fix: Walk the module recursively, or use Go's package/build metadata to determine whether any source dependency is newer than the installed binary.
| entries, err := os.ReadDir(srcDir) | |
| if err != nil { | |
| return false, err | |
| } | |
| for _, entry := range entries { | |
| if entry.IsDir() || (!strings.HasSuffix(entry.Name(), ".go") && entry.Name() != "go.mod") { | |
| continue | |
| } | |
| info, err := entry.Info() | |
| if err != nil { | |
| return false, err | |
| } | |
| if info.ModTime().After(newest) { | |
| newest = info.ModTime() | |
| } | |
| } | |
| err := filepath.Walk(srcDir, func(path string, info os.FileInfo, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| if info.IsDir() || (!strings.HasSuffix(info.Name(), ".go") && info.Name() != "go.mod") { | |
| return nil | |
| } | |
| if info.ModTime().After(newest) { | |
| newest = info.ModTime() | |
| } | |
| return nil | |
| }) | |
| if err != nil { | |
| return false, err | |
| } |
| dir := filepath.Join(knowledgeDir(), "ai") | ||
| existing, err := loadCandidates(dir) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| fp := normalize(fact) | ||
| for _, c := range existing { | ||
| if normalize(c.Text) == fp { | ||
| fmt.Printf("rem add: already captured on %s: %s\n", c.Day, c.Text) | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| if err := os.MkdirAll(dir, 0o755); err != nil { | ||
| return err | ||
| } | ||
| path := filepath.Join(dir, today()+".md") | ||
| fresh := false | ||
| if _, err := os.Stat(path); os.IsNotExist(err) { | ||
| fresh = true | ||
| } | ||
| f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) |
There was a problem hiding this comment.
issue (bug_risk): cmdAdd loads existing candidates and later opens today's file for append without any lock or atomic check, so concurrent rem add processes can both observe a missing fact and append it, defeating the documented cross-file deduplication.
Triggers: When two capture hooks invoke rem add concurrently for the same fact.
Suggested fix: Serialize candidate read-and-append with a lock in the vault or use an atomic update mechanism.
| return err | ||
| } | ||
| bin = filepath.Join(home, ".local", "bin", "knowledge-sync") | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): rem sync ignores GOBIN and falls back only to ~/.local/bin/knowledge-sync, even though dotagents sync installs knowledge-sync into the configured GOBIN; with a custom GOBIN, the wrapper cannot find the binary it just provisioned.
Triggers: When GOBIN is set to a directory other than ~/.local/bin and REM_SYNC_BIN is unset.
Suggested fix: Resolve the default sync binary from GOBIN before falling back to ~/.local/bin, or have the installer/configuration provide the path consistently.
| if _, err := os.Stat(bin); err != nil { | ||
| return fmt.Errorf("knowledge-sync not found at %s: %w", bin, err) | ||
| } | ||
| cmd := exec.Command(bin) |
There was a problem hiding this comment.
security (go.lang.security.audit.dangerous-exec-command): Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
Source: opengrep
30ff335 to
e4ad7bf
Compare
| if _, err := os.Stat(bin); err != nil { | ||
| return fmt.Errorf("knowledge-sync not found at %s: %w", bin, err) | ||
| } | ||
| cmd := exec.Command(bin) |
There was a problem hiding this comment.
security (go.lang.security.audit.dangerous-exec-command): Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
Source: opengrep
1 participant
Summary
memory/tools/rem— capture (rem add), consolidation (rem dream [--apply]), search and vault sync wrapper; the default memory workflow CLI described in the knowledge-vault planknowledge-syncwith a branch guard: refuses to run when the vault worktree is not on the target branch (prevents wrong-branch commits + duplicate resurrection seen in production)dotagents syncnow builds every Go tool undermemory/tools/and installs it to $GOBIN /~/.local/bin, rebuilding only on source changes and skipping gracefully without a Go toolchainTest plan
TestInstallMemoryTools*unit tests (build, freshness skip, rebuild-on-touch, missing-dir noop)go test ./cmd/dotagents/greenSummary by Sourcery
Ship the
remmemory workflow and provision bundled memory tools throughdotagents syncwhile guarding vault synchronization against wrong-branch operations.New Features:
remCLI for capturing, searching, reviewing, consolidating, and syncing knowledge-vault memory.dotagents sync, with incremental rebuilds and graceful handling on hosts without Go.Bug Fixes:
knowledge-syncfrom operating when the vault worktree is checked out on the wrong branch, avoiding unintended commits and duplicate resurrection.Enhancements:
rem dream --applysafely consolidate duplicate knowledge sections with worktree and branch safeguards, backups, and a commit.Documentation:
Tests: