Skip to content

fix(memory): add GlobalDir routing + tab-aware panel to fix empty memory view (#3756)#4073

Open
ashishexee wants to merge 1 commit into
esengine:main-v2from
ashishexee:fix/3756-memory-panel-empty
Open

fix(memory): add GlobalDir routing + tab-aware panel to fix empty memory view (#3756)#4073
ashishexee wants to merge 1 commit into
esengine:main-v2from
ashishexee:fix/3756-memory-panel-empty

Conversation

@ashishexee

Copy link
Copy Markdown
Contributor

Fix: Memory panel always empty on Global tab (#3756)

Problem

The desktop memory panel always shows empty when the Global tab (or any tab whose project-specific memory directory has no auto-memories) is active. The root cause is two-fold:

  1. Store only has Dir (project-specific path)App.Memory() reads from the active controller's Store.Dir, which for the Global tab points to an empty projects/<global-workspace-slug>/memory. There is no concept of a shared memory directory for cross-project memories (user preferences, feedback).

  2. Panel has no tab awareness — The memory panel always reads from the active tab's controller. When viewing settings from the Global tab, it gets the Global tab's empty store.

Fix (two layers)

Layer 1 — Backend Store (internal/memory):

  • Add GlobalDir field to Store, populated by StoreFor() as <userDir>/memory/global
  • DirFor(t Type) routes TypeUser/TypeFeedbackGlobalDir (shared), others → Dir (project-specific); falls back to Dir when GlobalDir is empty
  • List(), Index() merge both dirs with deduplication (global first)
  • Delete() removes from all dirs (handles migration where same memory name exists in both)
  • Save() routes via DirFor() and updates only the target dir's MEMORY.md
  • Index() reconstructs a single clean index from managed lines across both dirs (no duplicate headers)

Layer 2 — Desktop App + Frontend:

  • Add MemoryForTab/RememberForTab/ForgetForTab/SaveDocForTab using ctrlByTabID(tabID) with fallback=false (no silent fallback to wrong tab)
  • Empty tabID falls back to active tab for backward compatibility
  • Add StoreGlobalDir to MemoryView JSON payload
  • MemorySettingsPage gets a tab selector dropdown (shown when >1 tab) so user can browse any project's memories
  • All mutation calls use *ForTab when a tab is selected
  • "Memory unavailable" screen still shows the tab selector so user can recover

Backward Compatibility

  • Store{Dir: x} with empty GlobalDirDirFor() falls back to Dir, all methods work with single directory
  • Old Memory()/Remember()/Forget()/SaveDoc() unchanged (delegate with fallback=true)

Migration Safety

  • Existing memories in Dir are unaffected — List() merges both dirs
  • Same memory name in both dirs (pre-routing copy + new routing): List()/Index() deduplicate (GlobalDir first), Delete() cleans both
  • Index() produces exactly one # Memory header regardless of how many dirs contribute entries

Tests (9 new, 65 total pass)

  • TestStoreGlobalAndProject — routing + merge + delete + header dedup
  • TestStoreForInitializesGlobalDirStoreFor sets GlobalDir
  • TestDirForRoutesCorrectly — type→dir routing
  • TestDirForFallsBackWhenNoGlobalDir — empty GlobalDirDir
  • TestStoreDeleteRemovesFromAllDirs — migration: both copies deleted
  • TestStoreIndexDeduplicatesAcrossDirs — no duplicate index lines/headers
  • TestStoreSaveVerifiesIndexDirMEMORY.md written to correct dir per type
  • TestStoreDeleteFlushesIndexPerDir — both dirs' indexes flushed, Index() returns ""
  • TestStorePathWithGlobalDir — GlobalDir-first lookup + Dir fallback

Hi maintainers — this is my first time tackling a larger issue in this codebase. I've tried to be thorough with the implementation, test coverage, and edge cases (migration scenarios, stale tab handling, nil controller guards). That said, I'm sure there are things I could improve, and I'd genuinely appreciate any feedback on the approach, the code style, or anything I may have missed. Happy to iterate and learn. Thank you for your time reviewing this.

@github-actions github-actions Bot added v2 Go rewrite (1.x) — main-v2 branch, active development desktop Wails desktop app (desktop/**) labels Jun 11, 2026
Comment thread internal/memory/store.go Fixed
@ashishexee ashishexee force-pushed the fix/3756-memory-panel-empty branch 2 times, most recently from 6cb0163 to 32a4d45 Compare June 11, 2026 23:21
@ashishexee

Copy link
Copy Markdown
Contributor Author

Hi @esengine / @SivanCola — regarding the CodeQL "Uncontrolled data used in path expression" alert:

This is a false positive

The name parameter flows through slug() before reaching any path construction:

var slugRe = regexp.MustCompile(`[^a-z0-9]+`)

func slug(s string) string {
    return strings.Trim(slugRe.ReplaceAllString(strings.ToLower(strings.TrimSpace(s)), "-"), "-")
}

After slug(), the output can only contain [a-z0-9-] — no /, no \, no .., no null bytes. Path traversal is structurally impossible. CodeQL does not recognize slug() as a sanitizer because it treats custom helpers as opaque.

What I have tried

I wrapped the path construction in a safeJoin() helper using filepath.Rel containment checks (matching the repo's own confine.within pattern in internal/tool/builtin/confine.go), but CodeQL does not follow custom sanitizer wrappers — the taint still flows through.

Two options going forward

  1. Add // codeql[go/path-injection] suppression on the flagged lines with a comment referencing slug()'s guarantee — standard approach for verified false positives
  2. Restructure to avoid the taint flow entirely — use os.ReadDir to discover the file by matching e.Name() instead of constructing the path from user input; eliminates the false positive structurally but adds minor I/O overhead

I would lean toward option 1 as it is cleaner and slug() is trivially verifiable. Happy to go either way — let me know your preference and I will update the PR.

@ashishexee ashishexee force-pushed the fix/3756-memory-panel-empty branch from 1c21b29 to f12e509 Compare June 12, 2026 19:21
…ory view (esengine#3756)

The desktop memory panel always shows empty when the Global tab (or any
tab whose project-specific memory dir has no auto-memories) is active.

Root cause: Store only has Dir (project-specific path), so the panel
reads from the active controller's empty project dir. No shared memory
directory exists for cross-project memories (user preferences, feedback).
Also, the panel has no tab awareness — it always reads from the active
tab's controller.

Fix (two layers):

Layer 1 — Backend Store:
- Add GlobalDir field, populated by StoreFor() as <userDir>/memory/global
- DirFor(t Type) routes TypeUser/TypeFeedback → GlobalDir, others → Dir
- List(), Index() merge both dirs with dedup (global first)
- Delete() removes from all dirs (handles migration duplicates)
- Save() routes via DirFor(), updates only target dir's MEMORY.md
- Index() reconstructs single clean index from managed lines (no dup headers)

Layer 2 — Desktop App + Frontend:
- Add MemoryForTab/RememberForTab/ForgetForTab/SaveDocForTab with
  fallback=false (no silent fallback to wrong tab on nil ctrl)
- Empty tabID falls back to active tab for backward compat
- Add StoreGlobalDir to MemoryView JSON payload
- MemorySettingsPage gets tab selector dropdown (shown when >1 tab)
- All mutations use *ForTab when a tab is selected
- Unavailable screen still shows tab selector for recovery

Backward compat: Store{Dir:} with empty GlobalDir works — DirFor falls
back to Dir. Old Memory/Remember/Forget/SaveDoc unchanged.

Tests: 9 new (65 total pass), covering routing, merge, delete-migration,
index dedup, per-dir index verification, Path() GlobalDir-first lookup.
@ashishexee ashishexee force-pushed the fix/3756-memory-panel-empty branch from f12e509 to c5511bb Compare June 12, 2026 19:34
@github-actions github-actions Bot added the config Configuration & setup (internal/config) label Jun 12, 2026
@ashishexee

Copy link
Copy Markdown
Contributor Author

CI fix note for reviewers:

The test (windows-latest) failure was caused by global memory being loaded from the runner's user config directory because TestBuildWithoutMemoryLeavesPromptUnchanged did not isolate os.UserConfigDir(). The failure path matches the CI output exactly — the global memory file name (synthesis-cache-policy.md) appears in the log, and the duplicated # Memory header is visible in the failure output.

Two fixes applied:

  1. Test isolation — redirect HOME/XDG_CONFIG_HOME/AppData to a temp directory so os.UserConfigDir() doesn't pick up the runner's real reasonix\memory\global. This matches the pattern already used in other boot tests (e.g., boot_test.go:1496-1497).

  2. Removed duplicate # Memory header from Index()Block() already provides # Memory + ## Saved memories as the section structure, so Index() now returns just the index lines without its own header. The on-disk MEMORY.md files still get headers from flushIndexIn() — only the in-memory merge output is stripped.

This makes the test deterministic across platforms and removes a redundant header that would have appeared in the system prompt whenever both GlobalDir and Dir contributed entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

config Configuration & setup (internal/config) desktop Wails desktop app (desktop/**) v2 Go rewrite (1.x) — main-v2 branch, active development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants