feat(memlink): OS-agnostic vault->memory link primitive (CLI-025) - #557
Merged
Conversation
Add cli/internal/memlink: one Go primitive that links an agent's per-project memory directory to its source in the knowledge vault, converging the POSIX ensure-memory-symlink.sh and the Windows-only PowerShell junction hook into a single agent- and OS-agnostic noun. - resolveVaultMemory: shared source resolution via the three conventions (10_projects, in-vault CWD, work-SDK family/component slug match), faithfully reproducing the shell's break-on-first-match. - createLink branches symlink (POSIX) / directory junction (Windows). The junction needs no special privilege, closing the MEMORY-002 R4 gap the shell script deferred. - Idempotent and best-effort: a failed link is a silent no-op so it can never crash a session start. Consumed next by the Claude session-start adapter and by dotf doctor --fix (#551). Refs #494.
📝 WalkthroughWalkthroughAdds a new Changesmemlink: vault→memory link primitive
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Ensure
participant resolveVaultMemory
participant createLink
Caller->>Ensure: Ensure(cwd, target, project, vault)
Ensure->>Ensure: default project from cwd if empty
Ensure->>Ensure: check isLink(target) or dirNotEmpty(target)
alt already linked or non-empty dir
Ensure-->>Caller: "", nil
else
Ensure->>resolveVaultMemory: resolveVaultMemory(cwd, project, vault)
resolveVaultMemory-->>Ensure: source path or ""
alt source empty
Ensure-->>Caller: "", nil
else
Ensure->>Ensure: MkdirAll parent, remove empty placeholder
Ensure->>createLink: createLink(source, target)
createLink-->>Ensure: error or nil
alt link error
Ensure-->>Caller: "", nil
else
Ensure-->>Caller: "[auto-memory] Created symlink/junction …", nil
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cli/internal/memlink/memlink_test.go (1)
27-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a table-driven form for
TestResolveVaultMemory.These subtests share the same shape (set up dirs, call
resolveVaultMemory, assert the resolved path), which maps cleanly onto a[]struct{name, cwd, project string; setup func; want string}driven byt.Run.As per coding guidelines: "Use table-driven tests with t.Run in Go".
🤖 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 `@cli/internal/memlink/memlink_test.go` around lines 27 - 73, Refactor the TestResolveVaultMemory function from individual t.Run subtests to a table-driven test format. Define a table as a slice of structs with fields for the test name, cwd parameter, project parameter, a setup function to create the necessary vault directory structure, and the expected want result. Then replace the five separate t.Run blocks with a single loop that iterates through the table entries, calling resolveVaultMemory with the corresponding test parameters and asserting the result matches the want value. This maintains the same test logic while improving readability and making it easier to add new test cases.Source: Coding guidelines
cli/internal/memlink/memlink.go (1)
129-138: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout/context to the Windows
mklinkinvocation.
exec.Command(...).Run()blocks with no deadline. In a session-start hook a hungcmd /c mklinkwould stall startup indefinitely — arguably worse than a crash. Preferexec.CommandContextwith a bounded timeout (threading acontext.ContextfromEnsure).Sketch
- if runtime.GOOS == "windows" { - return exec.Command("cmd", "/c", "mklink", "/J", target, src).Run() - } + if runtime.GOOS == "windows" { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + return exec.CommandContext(ctx, "cmd", "/c", "mklink", "/J", target, src).Run() + }As per coding guidelines: "Propagate context.Context in all I/O operations in Go".
🤖 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 `@cli/internal/memlink/memlink.go` around lines 129 - 138, The createLink function uses exec.Command(...).Run() on Windows without a timeout or context, which can block indefinitely during startup. Modify the createLink function signature to accept a context.Context parameter, then replace the Windows branch's exec.Command call with exec.CommandContext, passing the context as the first argument to enable bounded timeout control. This ensures the mklink operation respects the deadline set by the caller (from Ensure) and prevents indefinite blocking.Source: Coding guidelines
🤖 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 `@cli/internal/memlink/memlink.go`:
- Around line 49-58: The error handling for os.MkdirAll is inconsistent with the
documented "never crash" contract and the best-effort approach used for
createLink. Either swallow the os.MkdirAll error with a similar comment pattern
as createLink (line 56-58) to maintain the best-effort, never-crash behavior, or
if returning the error is intentional, wrap it with context information before
returning using a wrapping pattern that provides diagnostic details about the
failure.
---
Nitpick comments:
In `@cli/internal/memlink/memlink_test.go`:
- Around line 27-73: Refactor the TestResolveVaultMemory function from
individual t.Run subtests to a table-driven test format. Define a table as a
slice of structs with fields for the test name, cwd parameter, project
parameter, a setup function to create the necessary vault directory structure,
and the expected want result. Then replace the five separate t.Run blocks with a
single loop that iterates through the table entries, calling resolveVaultMemory
with the corresponding test parameters and asserting the result matches the want
value. This maintains the same test logic while improving readability and making
it easier to add new test cases.
In `@cli/internal/memlink/memlink.go`:
- Around line 129-138: The createLink function uses exec.Command(...).Run() on
Windows without a timeout or context, which can block indefinitely during
startup. Modify the createLink function signature to accept a context.Context
parameter, then replace the Windows branch's exec.Command call with
exec.CommandContext, passing the context as the first argument to enable bounded
timeout control. This ensures the mklink operation respects the deadline set by
the caller (from Ensure) and prevents indefinite blocking.
🪄 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 Plus
Run ID: b63116af-fb30-4afa-a4ed-58f9175d6e62
📒 Files selected for processing (2)
cli/internal/memlink/memlink.gocli/internal/memlink/memlink_test.go
Comment on lines
+49
to
+58
| if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { | ||
| return "", err | ||
| } | ||
| // Remove an empty placeholder target dir so the link can be created. | ||
| if isDir(target) { | ||
| _ = os.Remove(target) | ||
| } | ||
| if err := createLink(src, target); err != nil { | ||
| return "", nil // best-effort: a failed link must never fail the session | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
MkdirAll error path breaks the documented "never crash" contract and is unwrapped.
The doc (Lines 24-30) states a session-start hook "must never crash" and that failures are silent no-ops, yet a failed os.MkdirAll propagates an error to the caller while a failed createLink is swallowed. This is inconsistent, and the returned error is also not wrapped with context.
If the intent is best-effort, swallow this too; otherwise wrap it.
Proposed change
- if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil {
- return "", err
- }
+ if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil {
+ return "", nil // best-effort: never fail the session
+ }As per coding guidelines: "Use if err != nil with context wrapping for error handling in Go".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { | |
| return "", err | |
| } | |
| // Remove an empty placeholder target dir so the link can be created. | |
| if isDir(target) { | |
| _ = os.Remove(target) | |
| } | |
| if err := createLink(src, target); err != nil { | |
| return "", nil // best-effort: a failed link must never fail the session | |
| } | |
| if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { | |
| return "", nil // best-effort: never fail the session | |
| } | |
| // Remove an empty placeholder target dir so the link can be created. | |
| if isDir(target) { | |
| _ = os.Remove(target) | |
| } | |
| if err := createLink(src, target); err != nil { | |
| return "", nil // best-effort: a failed link must never fail the session | |
| } |
🤖 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 `@cli/internal/memlink/memlink.go` around lines 49 - 58, The error handling for
os.MkdirAll is inconsistent with the documented "never crash" contract and the
best-effort approach used for createLink. Either swallow the os.MkdirAll error
with a similar comment pattern as createLink (line 56-58) to maintain the
best-effort, never-crash behavior, or if returning the error is intentional,
wrap it with context information before returning using a wrapping pattern that
provides diagnostic details about the failure.
Source: Coding guidelines
Touch the CLI-025 spec for the memlink primitive PR: add feature f6 (the OS-agnostic vault->memory link) and split the tasks PR2b section into PR2b-1 (memlink, this PR) and PR2b-2 (the Claude adapter). Satisfies the SDD Discipline Gate, which requires a >=50 LOC PR to touch its spec folder.
7 tasks
This was referenced Jun 24, 2026
mlorentedev
added a commit
that referenced
this pull request
Jun 24, 2026
…e spec (#571) The session-start convergence (PR2a → PR3 cutover, #554/#557/#566/#569/#570) is complete on main; the session hook cluster is one Go noun with zero shell twins. - docs/lessons.md: two lessons from the migration — (1) a Go-vs-shell byte-equivalence gate is POSIX-only (jq CRLF + MSYS /tmp) and retires at cutover; (2) CI golangci-lint enforces staticcheck QF* quickfixes a stale local version skips. - Move specs/CLI-025-dotf-mem-heal-and-session-start/ -> specs/archive/ and set its proposal status to archived. Refs #494.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
cli/internal/memlink— one Go primitive that links an agent's per-project memorydirectory to its source in the knowledge vault, converging two drifting OS twins into one
agnostic noun: the POSIX
ensure-memory-symlink.sh(symlink) and the Windows-only PowerShelljunction hook (the MEMORY-002 R4 gap the shell deferred). CLI-025 PR2b-1.
resolveVaultMemory— OS-agnostic source resolution via the three conventions(
10_projects/<project>/memory, in-vault<cwd>/memory, work-SDK family/component slug match),faithfully reproducing the shell's break-on-first-match.
createLink—os.Symlinkon POSIX, a directory junction on Windows (no privilege needed,confirmed end-to-end). Idempotent + best-effort: a failed link is a silent no-op so it can never
crash a session start.
cmd /c mklink /Jkeepsgo.modat stdlib + cobra.Consumed next by the Claude session-start adapter (PR2b-2) and
dotf doctor --fix(#551).SDD checklist
11-tasks.md)specs/CLI-025-dotf-mem-heal-and-session-start/is included in this PR (featuref6+ tasks PR2b-1 split)proposal.mdhas Why / What / Acceptance criteriatasks.mdis in TDD orderverification.mdis filled at spec close (multi-PR spec); per-PR evidence is the green CI belowTest plan
go test ./internal/memlink/green (9 subtests, incl. real junction creation on Windows and symlink on POSIX)go build ./...·go vet·golangci-lintcleanmklink /Jjunction creation confirmed without admin (Go + CItest (windows-latest)); symlink read-through on POSIX (test (ubuntu-latest)).shor.batsfiles changedRefs #494. Unblocks #551.