odysseus/docs/handoff.md
yakamoz221 ee0db74064 docs: record Milestone 2 verification session and its fixes
Updates handoff/progress/todos to reflect that M2 is committed and
visually verified, and documents the three bugs found (and fixed)
along the way: the memory_graph_routes.py owner-scoping mismatch,
the detail panel's stuck `hidden` class, and search leaving stale
selection-highlight opacity over its own matches.
2026-07-31 00:49:59 +03:00

11 KiB

Memory Graph View — Handoff

Read this first if picking up this work in a new session. It answers: what's done, what's the very next thing to do, what could bite you, and why certain calls were made.

For the full checklist see docs/todos.md. For the session-by-session history see docs/progress.md. For the accepted design see docs/memory-graph-design.md. For the repo research behind it see docs/memory-graph-analysis.md.

Where things stand right now

  • Branch: feature/memory-graph-view.
  • Milestone 1 (backend API): done, tested, committed. One additional bug found and fixed during M2 verification (see below) — memory_graph_routes.py's owner resolution was inconsistent with the rest of the memory routes, which made the graph always empty and links always 404 in single-user/no-auth mode. Committed separately (fix(memory-graph): align owner resolution with rest of memory routes).
  • Milestone 2 (frontend): done, committed, visually verified end-to-end against a real seeded dataset (render, node click + neighborhood highlight, detail panel, inline edit, search, category filters, similarity slider, link-mode create, theme switch — see docs/progress.md Session 4 for the full pass). Two frontend bugs found and fixed in the same commit: the detail panel's hidden class was never removed on node selection (panel was permanently invisible), and _applySearch() left stale mg-dimmed/mg-highlighted classes from a prior node selection that masked search-match nodes at opacity:0.08.
  • Milestone 3 (polish): not started. This is the next work.

Exact next task

Resume the Milestone 3 checklist in docs/todos.md (legend refinement, isolate-component affordance, keyboard shortcuts, MODULE_SUMMARY.md update, a real automated-test pass for memoryGraph.js). Two things need explicit user sign-off before or during M3:

  1. The "feature flag enabled (beta)" open question (see below) — confirm whether a real togglable flag is wanted, or whether the current privilege-gate + static "beta" label is sufficient as shipped.
  2. Whether to also fix the known limitation that graph node colors are only recomputed on next render, not live-reactive to a theme flip while the modal is already open (confirmed still true during verification — switching theme via the Theme modal while Memory Graph was open did not immediately recolor node fills, though all non-canvas chrome — detail panel, chips, buttons — is CSS-variable-driven and did update live).

For a repeatable verification loop in a future session (isolated data dir, seeding via the real API, symmetric Chroma cleanup, PID-verified process kill), the exact steps used are preserved in docs/progress.md Session 4 — worth reusing as-is rather than re-deriving.

Open decision needed from the user

The "feature flag enabled (beta)" requirement was interpreted as "ship it visible, label it beta" — not as "build an actual on/off toggle." Right now the only thing gating visibility of the "Memory Graph" nav item is the existing can_manage_memory privilege (the same gate the "Brain" button already uses), plus a static "beta" label next to the title in the modal header. There is no separate settings switch a user or admin could flip to hide the feature independent of that privilege, and no server-side flag either. If a real togglable flag was intended (e.g. a Settings-tab checkbox like the design doc's original Phase 2 proposal in docs/memory-graph-design.md §8), that hasn't been built. Confirm which was actually wanted before Milestone 3.

Risks / things to watch

  1. The src/agent_loop.py one-line fix is uncommitted and out of this feature's actual scope. It's a genuinely pre-existing bug (confirmed via git stash to reproduce identically on a clean dev checkout — NameError: name 'Any' is not defined because dict[str, Any] was used in a function signature without importing Any), and without it the entire app fails to import, which blocks any local verification at all. It's about as safe a fix as they come (adding a missing stdlib import can't break anything that currently works, since nothing downstream of that line currently works). But it wasn't asked for, so don't fold it silently into a Memory Graph commit — either commit it separately with a clear message, or ask the user whether they even want it touched at all (they may already have a fix for it elsewhere, or may want to report it upstream themselves).
  2. Do not seed demo/test memory data into the real ChromaDB without cleaning it up. See step 4 above — this bit us mid-session (server was mid-startup, about to seed, when told to stop; it was killed before anything was written, so no cleanup is currently owed, but the next session doing this must not skip the deletion step).
  3. The dev venv (.venv-test/) is a throwaway, git-ignored (via .git/info/exclude, not .gitignore) local artifact, created because this sandbox had no Python environment with the project's dependencies installed. It is NOT part of the repo and should not be referenced by anything committed. A future session may need to recreate it (python -m venv .venv-test && .venv-test/Scripts/python.exe -m pip install -r requirements.txt) if it's not still present.
  4. Pre-existing test failures are numerous (199) in this sandbox and are all environment-driven (missing Node/rg, an mcp package version mismatch, Windows path-confinement assumptions) — confirmed byte-for-byte identical between this branch and a clean dev baseline. Don't waste time trying to fix them as part of this feature; they were already broken before this work started, and fixing them is a separate, unrelated effort.
  5. Chroma collections are global, not owner-scoped. This was already true of the existing memory system before this feature (docs/memory-graph-analysis.md §10) and the graph feature doesn't change it, but it's exactly why step 4 above matters — any manual testing against a real/shared Chroma instance needs symmetric cleanup.

Implementation notes worth knowing before touching this code

  • Route-ordering is load-bearing. routes/memory/memory_graph_routes.py must be include_router()'d in app.py before routes/memory/memory_routes.py's router. The latter's GET/PUT/DELETE /api/memory/{memory_id} is a single-segment wildcard; Starlette matches routes in registration order across the whole app (not by specificity), so GET /api/memory/graph would otherwise resolve to the wildcard with memory_id="graph" and 404. This is regression-tested in tests/test_memory_graph_route_ordering.py using a real TestClient — the only place in this feature's tests that a full ASGI app was actually needed, because the repo's usual "look up the route by exact path string, call the endpoint function directly" test convention can't catch this class of bug (it never exercises real Starlette path matching).
  • The runtime memory store is a JSON file, not the SQL memories table. src/memory.py::MemoryManager persists to data/memory.json; there's a memories SQLAlchemy table in core/database.py too, but it's unused by any live code path (see docs/memory-graph-analysis.md §8's "Important discrepancy"). The Memory Graph feature correctly targets the JSON store + the odysseus_memories Chroma collection, matching what every other memory route already does. Don't be tempted to "fix" this by wiring the graph to the SQL table — that's explicitly out of scope (see the analysis doc).
  • Manual links are stored as a new optional links: [] field directly on memory JSON entries. No migration was needed because it's a JSON file, not a SQL schema — old entries without the field are handled via entry.get("links", [])/mem.get("links") or [] throughout, so nothing breaks for pre-existing memories.
  • The frontend has zero bundler. Cytoscape.js is vendored as a single UMD file (static/lib/cytoscape.min.js, fetched via npm pack cytoscape@3 in a scratch dir — not a hand-typed CDN URL) and lazy-loaded via a dynamically-created <script> tag the first time the Graph modal opens, mirroring the exact pattern documentLibrary.js already uses for xlsx.full.min.js/mammoth.browser.min.js (ensureXLSX/ensureMammothensureCytoscape in memoryGraph.js).
  • The modal itself is built entirely in JS, not in index.html. memoryGraph.js's _getModal() follows calendar.js's _getModal() template exactly: lazily document.createElement'd on first open, appended to document.body once, memoized in a module-level variable, registered with modalManager.js's Modals.register(...) (gets minimize/dock/restore/z-order for free — same mechanism gallery/notes/cookbook/calendar all use). This was a deliberate choice over adding static modal markup to index.html (which is how the older "Brain"/"Calendar" full-page structure looks from the HTML side, but their actual open/close JS logic turned out to live in app.js/calendar.js, not be purely declarative — worth rereading docs/memory-graph-analysis.md if this surprises you). Following the newer self-registering pattern meant zero edits to app.js's older hardcoded Escape-key modal-id arrays (modalItemMap, _modalSidebarMap, dynamicModals) — the new modal manages its own Escape handling directly, exactly like calendar.js does.
  • Category colors are resolved from existing theme CSS custom properties (--fg, --hl-keyword, --warn, --color-accent, --color-brand-blue, --accent-warm, --green) via getComputedStyle(document.documentElement) at render time — not hardcoded hex values — so dark/light theme support came largely for free. Known limitation: colors are only (re)computed when the graph is (re)rendered (on open, or after a mutation reload), not on a live theme-toggle while the modal is already open. Flipping the theme mid-session and expecting instant recolor without reopening the modal is a Milestone 3 nice-to-have, not yet built.
  • Filtering is entirely client-side; only the initial load hits the backend. The graph is fetched once per open (or after a mutation) at a generous floor (min_similarity=0.5, max_edges_per_node=8) and cached in memory; the category chips, the similarity slider, and the search box all operate on that cached copy via Cytoscape's own style('display', ...)/class toggling. This was a deliberate deviation from a literal reading of the design doc's per-request query params (docs/memory-graph-design.md §3) — it makes filter interactions instant instead of a round-trip per click, at the cost of the UI slider not being able to go below 0.5 (the server floor). Worth flagging as a design refinement, not an oversight.
  • Nav item scope was deliberately kept to the sidebar Tools list only. There's also a separate, fixed-size compact "icon rail" (mobile/collapsed-sidebar duplicate) in index.html with a hardcoded _railToolMap in app.js — extending that to add a Memory Graph icon was skipped as out of scope for this milestone (higher risk of visually cramming a fixed-width icon strip, not explicitly requested). If compact/mobile nav parity is wanted, that's a small, well-scoped follow-up.