mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-08-05 02:45:28 +00:00
docs: record push-permission blocker and clean up stale handoff notes
git push to origin failed with 403 (yakamoz221 lacks write access to odysseus-dev/odysseus.git) — recorded in handoff/progress/todos as the new exact-next-step. Also resolved two stale sections left over from before Milestone 3: the "Open decision needed" (feature-flag toggle, answered by the user) and the "agent_loop.py fix is uncommitted" risk note (it's been committed since Session 4).
This commit is contained in:
parent
6b3caf6bf7
commit
158ad4d0c6
3 changed files with 36 additions and 10 deletions
|
|
@ -13,24 +13,34 @@ For the full checklist see `docs/todos.md`. For the session-by-session history s
|
|||
|
||||
## Exact next task
|
||||
|
||||
There is no open milestone left on the original plan. Two loose ends, neither blocking:
|
||||
There is no open milestone left on the original plan. The branch is fully committed but **not pushed** — the last push attempt (`git push -u origin feature/memory-graph-view`) failed:
|
||||
|
||||
1. **Known limitation, not fixed**: 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 — 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, the new legend header — is CSS-variable-driven and updates live). Left as-is; fix if it ever actually bothers a user.
|
||||
```
|
||||
remote: Permission to odysseus-dev/odysseus.git denied to yakamoz221.
|
||||
fatal: unable to access '...': The requested URL returned error: 403
|
||||
```
|
||||
|
||||
The `origin` remote (`https://github.com/odysseus-dev/odysseus.git`) doesn't grant this GitHub account write access. Before this branch can be shared, the user needs to either: grant `yakamoz221` push access to that repo, point `origin` at a fork the account does own, or push some other way (e.g. a different configured remote/credential). This is a permissions/credentials matter outside what a coding session can resolve on its own.
|
||||
|
||||
Two loose ends beyond that, neither blocking:
|
||||
|
||||
1. **Known limitation, not fixed**: 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 — 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, the legend header — is CSS-variable-driven and updates live). Left as-is by deliberate choice at the end of Milestone 3, not an oversight — revisit only if a user actually notices/minds.
|
||||
2. If compact/mobile icon-rail parity is wanted for the Memory Graph nav item (see Implementation notes below), that's a small standalone follow-up, not scoped into any milestone so far.
|
||||
|
||||
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` Sessions 4 and 5 — worth reusing as-is rather than re-deriving.
|
||||
|
||||
## Open decision needed from the user
|
||||
## Resolved decisions (kept for context, not open anymore)
|
||||
|
||||
**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.**
|
||||
- **"Feature flag enabled (beta)"**: asked the user directly during Session 5 whether this meant a real togglable on/off switch or the shipped privilege-gate (`can_manage_memory`, same as the "Brain" button) plus a static "beta" label was enough. User confirmed the latter is sufficient — no separate settings switch was built, and none is needed.
|
||||
- **The `src/agent_loop.py` one-line fix** (a genuinely pre-existing bug — `NameError: name 'Any' is not defined` from a missing `typing` import, confirmed via `git stash` on a clean `dev` checkout, blocking the whole app from starting): kept and committed separately (`fix(agent_loop): add missing typing.Any import`), not folded into any Memory Graph commit. Low-risk enough (adds a missing stdlib import; nothing downstream of that line worked before this fix either) that it didn't need further debate.
|
||||
|
||||
## 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).
|
||||
1. **The branch is unpushed** — see "Exact next task" above. Anyone continuing this work locally has everything; anyone expecting to see it on GitHub does not yet.
|
||||
2. **Do not seed demo/test memory data into the real ChromaDB without cleaning it up.** Sessions 4 and 5 both did this correctly (seed via the real `POST /api/memory/add`, delete via the real `DELETE /api/memory/{id}`, then directly query the shared Chroma collection's `/get` endpoint by id to *confirm* zero vectors remain rather than assuming). Any future session doing this must follow the same symmetric pattern.
|
||||
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.
|
||||
4. **Pre-existing test-collection errors exist in this sandbox and are environment-driven**, not caused by this feature: an `mcp` package version mismatch (`AttributeError: 'Server' object has no attribute 'list_tools'`) affecting 4 test files, plus one unrelated `UnicodeDecodeError` in a document-diff test — confirmed via `pytest --collect-only` at the end of Session 5 to be the exact same 5 errors, nothing new. (Session 2 additionally diffed a full *execution* run against a clean `dev` baseline and found 199 identical pre-existing failures/errors; that full-run comparison wasn't re-done in Sessions 4/5, only the lighter collection-only check — worth a full re-diff if a future session wants that stronger guarantee.)
|
||||
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 point 2 above matters — any manual testing against a real/shared Chroma instance needs symmetric cleanup.
|
||||
|
||||
## Implementation notes worth knowing before touching this code
|
||||
|
||||
|
|
@ -39,6 +49,6 @@ For a repeatable verification loop in a future session (isolated data dir, seedi
|
|||
- **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`/`ensureMammoth` → `ensureCytoscape` 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.
|
||||
- **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. This was considered during Milestone 3 and deliberately left unbuilt (see "Exact next task" above) rather than missed.
|
||||
- **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.
|
||||
|
|
|
|||
|
|
@ -29,6 +29,17 @@ Session-by-session record. Newest entry on top. See `docs/todos.md` for the live
|
|||
| `tests/memoryGraph/graphHarness.mjs`, `tests/memoryGraph/pureLogic.test.mjs`, `tests/test_memory_graph_pure_logic_js.py` | new — pure-logic test suite |
|
||||
| `docs/handoff.md`, `docs/progress.md`, `docs/todos.md` | updated to reflect M3 done/verified/committed |
|
||||
|
||||
### Push attempt
|
||||
|
||||
After M3 was committed, `git push -u origin feature/memory-graph-view` was attempted and failed:
|
||||
|
||||
```
|
||||
remote: Permission to odysseus-dev/odysseus.git denied to yakamoz221.
|
||||
fatal: unable to access '...': The requested URL returned error: 403
|
||||
```
|
||||
|
||||
The `origin` remote is `https://github.com/odysseus-dev/odysseus.git`; the authenticated GitHub account (`yakamoz221`) doesn't have write access to it. Not something a coding session can fix on its own — needs the user to sort out repo permissions, point at a fork, or push via different credentials. The branch is fully committed locally (11 commits total on top of `dev`) and otherwise ready; see `docs/handoff.md` for the exact next step.
|
||||
|
||||
---
|
||||
|
||||
## Session 4 — Milestone 2 verification, two bugs found and fixed, M2 committed
|
||||
|
|
|
|||
|
|
@ -24,7 +24,7 @@ Live checklist. Update as work proceeds. See `docs/handoff.md` for the exact nex
|
|||
- [x] Found and fixed a real Milestone-1 backend bug while seeding real data: `memory_graph_routes.py`'s graph/links endpoints used `require_user()` (returns `""`) instead of the rest of the memory routes' `get_current_user()`-based `_owner()` (returns `None`), which meant the graph was always empty and links always 404'd in single-user/no-auth mode. Fixed and committed separately from the M2 frontend commit.
|
||||
- [x] Seeded memories cleaned up: all 7 deleted via the real `DELETE /api/memory/{id}` endpoint, confirmed zero orphaned vectors left in the shared Chroma `odysseus_memories_fastembed` collection, demo server process killed (confirmed real PID via `Get-NetTCPConnection`, not the bash job id).
|
||||
- [x] Commit M2 work — 4 commits: pre-existing `agent_loop.py` fix (standalone), the M1 owner-scoping fix (standalone), the M2 frontend (module + vendored lib + wiring + CSS), this docs update.
|
||||
- [ ] **NOT DONE**: explicit on/off feature-flag setting. Right now the nav item is always visible to anyone with `can_manage_memory` (same gating as the existing Brain button) and the UI carries a static "(beta)" label — there is no separate toggle a user/admin can flip to hide the feature independent of that privilege. The user's instruction said "Feature flag enabled (beta)"; this was interpreted as "ship visible, labeled beta" rather than "build a togglable flag primitive." **Needs confirmation** — see Open Questions in `docs/memory-graph-design.md`.
|
||||
- [x] Explicit on/off feature-flag setting — resolved in Milestone 3 (user confirmed the privilege-gate + static "beta" label is sufficient; no separate toggle needed).
|
||||
- [x] Automated frontend tests for `memoryGraph.js` pure logic — see Milestone 3.
|
||||
|
||||
## Milestone 3 — Polish (DONE, committed)
|
||||
|
|
@ -38,6 +38,11 @@ Live checklist. Update as work proceeds. See `docs/handoff.md` for the exact nex
|
|||
- [x] Final regression pass: `node --check` clean on all touched JS; the 27 existing Milestone 1 backend tests + the new JS suite all pass; `pytest --collect-only` across the full suite shows the same 5 pre-existing collection errors as before this session (mcp package version mismatch + one unrelated `UnicodeDecodeError`), zero new failures.
|
||||
- [x] `src/agent_loop.py` fix: kept, committed separately (see Session 4) — decided low-risk enough to ship without further debate.
|
||||
|
||||
## Distribution
|
||||
|
||||
- [x] All work committed locally — 11 commits ahead of `dev` on `feature/memory-graph-view`.
|
||||
- [ ] **BLOCKED**: `git push -u origin feature/memory-graph-view` fails with 403 (`Permission to odysseus-dev/odysseus.git denied to yakamoz221`). Needs the user to grant push access, point `origin` at a fork, or push via different credentials — see `docs/handoff.md`.
|
||||
|
||||
## Known pre-existing issues (not introduced by this feature, out of scope to fix here)
|
||||
|
||||
- `mcp_servers/rag_server.py` (and a few tests importing it) hit `AttributeError: 'Server' object has no attribute 'list_tools'` — an `mcp` package version mismatch in this sandbox's venv vs whatever version the repo's real environment pins. Not touched, not in scope.
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue