feat: add memory management UI

[lsm]

What

Surfaces a space's agent memories in the UI. The agentMemory.* RPC API existed but nothing in packages/web consumed it; memories were invisible to the user.

Changes

• Shared types: promote AgentMemoryEntry / AgentMemorySearchResult to @hyperneo/shared so the web client and daemon share one source of truth. Daemon repository imports + re-exports them (existing daemon imports unchanged).
• memory-store (packages/web/src/lib/memory-store.ts): one-shot client over agentMemory.list / search / write / delete, with load-generation race guarding and space attach/detach lifecycle.
• SpaceMemories panel: dedicated /space/:id/memories route + sidebar entry. Header with count, debounced hybrid search (delegates to the daemon's BM25 + vector backend via agentMemory.list?query=), memory cards (key, content preview, tags, updated), create/edit/delete.
• SpaceMemoryEditor: modal form. Key is locked when editing (the daemon upserts on (spaceId, key), so renaming would create a duplicate). Client validation mirrors the daemon's normalize* rules.
• Router/signals/island wiring mirrors the existing Goals view.

Scope note

Space-scoped only. Per-agent (mine / space / all) filtering lands with the per-agent namespacing task, as called out in #765.

Verification

• bun run typecheck, bun run lint, bun run knip — clean.
• Full web suite: 6436 passed.
• Daemon memory tests (repository, RPC handlers, MCP tools, consolidation) — pass.
• New SpaceMemories.test.tsx (13 cases): list, search/empty-states, create/edit/delete round-trips, validation, lifecycle.

Deferred

The task acceptance mentions a Playwright visual verify; per the standing instruction that E2E tests are on hold, this is left for the QA pass rather than added here.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 34a78490c9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: REQUEST_CHANGES — one P2 integration finding to resolve or consciously accept; the rest is polish. Verified independently: typecheck/lint/knip clean, SpaceMemories.test.tsx 13/13, daemon memory suite 58/58.

What's good

Acceptance is met. Route + sidebar entry wired (mirrors Goals); list/create/edit/delete round-trip against the existing agentMemory.* handlers; search delegates to the daemon's hybrid BM25+vector backend (list?query= → AgentMemoryRepository.searchWithOptions RRF fusion — verified); client-side validation mirrors the daemon normalize* rules (key ≤200, content ≤10000, tag ≤50); AgentMemoryEntry/AgentMemorySearchResult correctly promoted to @hyperneo/shared. The Playwright visual verify is deferred per the E2E-on-hold instruction — fine for the QA pass.

P2 — Management search mutates agent access telemetry

memory-store.search() → agentMemory.list?query= → searchWithOptions, which bumps access_count and last_accessed_at on every returned row (agent-memory-repository.ts:421-432). Browsing the list (no query) is read-only, but every search is not. Those two fields drive agent behavior:

• coreMemoryScore = access_count / (1 + ageDays) (age from last_accessed_at) — a search double-boosts a memory's core-memory ranking.
• pruneStaleMemories keeps rows whose last_accessed_at is recent — so a search resets the staleness clock and rescues otherwise-prunable memories for ~90 days.

A human inspecting memories shouldn't side-effect what agents recall or whether consolidation prunes. Recommend a read-only path: thread a recordAccess?: boolean (default true) through list/searchWithOptions and have the agentMemory.list handler pass recordAccess: false for management reads. If you consciously decide the bump is acceptable, say so and I'll approve.

P3 — nits (optional, won't block on their own)

• Header count mislabels during search: Memories · {memories.length} stored shows the filtered result count while searching, not total stored. Show "N results" when a query is active.
• navigationSpaceId prop unused: declared in SpaceMemoriesProps and passed by SpaceIsland, but never destructured/used in SpaceMemories. Drop it or use it.
• onSaved is speculative: SpaceMemoryEditor defines and fires onSaved?.(entry), but no caller passes it. Wire it (e.g., select the saved memory) or drop it.
• Create silently overwrites: the daemon upserts on (spaceId, key), so "Create Memory" with an existing key updates content with no warning. Consider a duplicate-key guard.
• First-load empty flash: loaded=false && isLoading=false on first paint briefly shows "No memories stored yet." before the spinner. Guard on !loaded (and set loaded=true in the error branch so errors still surface).

Out of scope (pre-existing, not introduced here): the agentMemory.* RPC handlers take spaceId from the payload with no per-space authorization — any authenticated socket can read/write any space's memories. Worth a separate task, not this PR.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fabafef915

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: APPROVE

Re-reviewed fresh across the three fix commits (fabafef → 461a9c0). All round-1 findings resolved correctly and independently verified, plus substantive hardening beyond what I raised.

Round-1 findings — resolved & verified:

• P2 read-only management search ✅ — agentMemory.list handler now passes recordAccess: false; searchWithOptions guards if (options?.recordAccess !== false && rows.length > 0), so agent-facing search() still records access while management reads don't. Verified at both layers — handler test asserts the flag is forwarded; repo test (agent-memory-repository.test.ts:178) asserts no access_count/last_accessed_at bump with the flag and a bump without it.
• P3 count label ✅ — "N results" while searching, "N stored" otherwise.
• P3 unused navigationSpaceId ✅ — dropped from props and the SpaceIsland call site.
• P3 speculative onSaved ✅ — removed.
• P3 silent create-overwrite ✅ — duplicate-key now blocked (local existingKeys + debounced authoritative exists() via read-only agentMemory.read).
• P3 first-load flash ✅ — guard is now !loaded && !error, which also covers initial-load failure (no indefinite spinner).

Additional hardening (verified): pagination (loadMore/hasMore/dedup), tag-count limit mirroring the daemon, optimistic write/delete that survives a failed reconciling reload, save-in-flight dismissal guard (backdrop/Escape/close), and ContextPanel preservation of the Memories view across space switches. New memory-store.test.ts (7 cases) and daemon additions cover these substantively, not trivially.

Verification (run independently, not on trust): typecheck/lint/knip clean; web memory tests 25/25; daemon handler+repo 55/55. Hybrid-search delegation and recordAccess threading confirmed in source.

Non-blocking polish notes (optional, not findings):

• existingKeys={memories.map(...)} is a fresh array per parent render, so the editor's debounced exists effect re-subscribes each render — the existingKeys.includes(key) guard keeps it correct, but memoize if it ever surfaces in profiling.
• An optimistic edit during an active search re-sorts the list by updated_at until the reconciling reload re-ranks by relevance — a transient flicker only.
• After a mutation, reload() resets pagination to page 0, collapsing any manually-loaded extra pages — a reasonable consistency trade-off.

Playwright visual verify remains deferred to the QA pass per the E2E-on-hold standing instruction.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 461a9c0b13

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

@codex review — requesting a fresh review on the current head (461a9c0). The prior eyes/reviews predate the round-3 fix commit. Reviewer has approved; looking for your +1 to satisfy the gate.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 461a9c0b13

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Revising recommendation: APPROVE → REQUEST_CHANGES. Retracting my prior approval.

I approved prematurely — I verified router.ts but missed that App.tsx runs a second, independent URL-sync effect that every sibling view satisfies and memories does not. Codex's review on this head (which the gate waited for) flagged it P1; I've independently confirmed it plus two residual store races.

P1 — Memories view bounces back to Overview (App.tsx URL-sync has no memories branch)

App.tsx:126 is a Preact signal effect that re-derives the URL from the view-mode signals. Its expectedPath chain (App.tsx:138–172) and navigation chain (App.tsx:176–201) branch on agents/sessions/goals/forge/tasks/configure — not memories. When the sidebar sets viewMode='memories':

1. The effect re-runs; expectedPath falls through to createSpacePath(spaceId) = /space/:id (no /memories segment).
2. currentPath (/space/:id/memories) !== expectedPath (/space/:id) → falls through to navigateToSpace(spaceId, true).
3. navigateToSpace unconditionally sets currentSpaceViewModeSignal.value = 'overview' (router.ts:454/468) and rewrites the URL to /space/:id.

Net: clicking Memories flashes and returns to Overview; deep-link/refresh to /space/:id/memories drifts to overview too. I confirmed the expectedPath drop empirically (/space/s1/memories → /space/s1) while goals keeps /space/s1/goals. The PR wired router.ts (applyPathToSignals, navigateToSpaceMemories, create/get path helpers) but not this parallel effect — App.tsx isn't in the diff.
Fix: mirror the goals branch — import createSpaceMemoriesPath + navigateToSpaceMemories; add : spaceId && spaceViewMode === 'memories' ? createSpaceMemoriesPath(spaceId) to the expectedPath chain and } else if (spaceId && spaceViewMode === 'memories') { navigateToSpaceMemories(spaceId, true); } to the navigation chain. Please add an App-level routing test too — the component tests mock the store, which is why this slipped through.

P2 — loadMore spinner sticks if a reload interrupts it (memory-store.ts:149–153)

reload() and loadMore() share loadGeneration. If a search (debounced reload) or post-mutation refresh fires while a loadMore fetch is in flight, loadGeneration advances and loadMore's finally skips clearing isLoadingMore (it gates on generation === this.loadGeneration). The "Load more" button then stays in its loading state and loadMore() early-returns forever. Reachable: click Load more, then type in search inside the fetch window.
Fix: clear isLoadingMore unconditionally in the finally (the generation guard should gate the result, not the spinner), or have reload() reset isLoadingMore.

P2 — cross-space write/delete can contaminate the new space's list (memory-store.ts:183)

write() captures spaceId up front but calls this.upsertEntry(entry) against the live signal after the await. If the user switches spaces mid-write, the old space's entry lands in the new space's list; the reconciling reload() clears it only if it succeeds, and refreshBestEffort swallows that failure. Same shape for deleteMemory/removeEntry.
Fix: after the await, if (this.spaceId !== spaceId) return entry; — skip the optimistic update + refresh when the store has moved on.

Already addressed / non-blocking (Codex raised these too — verified fixed or acceptable)

• Tag-count >50 → blocked at SpaceMemoryEditor.tsx:106. ✅
• Refresh-failure losing writes → optimistic refreshBestEffort. ✅
• Initial-load error spinner → !loaded && !error. ✅
• Clear pending search on space switch → in the effect. ✅
• ContextPanel memories case → present (ContextPanel.tsx:213). ✅
• Non-atomic dup-check (TOCTOU inside the 250 ms debounce): residual, narrow, safe-direction — acceptable; optionally re-run exists() synchronously at save as a final guard.

Not re-handing off to QA. Once the P1 (and ideally the two P2s) are fixed, I'll re-review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cefb9242c1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: APPROVE

Re-reviewed fresh across cefb924 → 8da14e7. The P1 and both P2s from round 3 are fixed correctly and verified, plus the residual dup-key TOCTOU and additional pagination races.

P1 — Memories→Overview bounce: FIXED & tested. App.tsx's URL-sync now derives expectedPath via the extracted pure helper deriveAppExpectedPath (lib/app-routing.ts, includes the memories branch), and the navigation dispatch also has the memories branch (App.tsx:163–164). New app-routing.test.ts explicitly asserts the memories regression AND enumerates every view mode → distinct path, so a future missing branch fails the test — the App-level routing coverage that was absent.

P2 — loadMore spinner: FIXED. Separate loadMoreGen ownership token: a reload (bumps loadGeneration only) still lets loadMore clear its spinner; a detach (bumps both) prevents a stale cross-space loadMore from clearing the new space's spinner.

P2 — cross-space write/delete: FIXED. write/create/deleteMemory guard if (this.spaceId !== spaceId) return after the await, before the optimistic update.

Atomic create RPC (beyond). New agentMemory.create → repo.create (INSERT ... ON CONFLICT DO NOTHING RETURNING *, throws on conflict) closes the dup-key TOCTOU atomically; editor uses create for New and write for Edit. Tested in both directions at repo + handler. Also: delete-modal dismissal guarded during the RPC; reload() resets hasMore up front.

Verification (independent): typecheck/lint/knip clean; web 37/37 (app-routing 5 + memory-store races + SpaceMemories); daemon 57/57 (incl. create success/conflict). All fixes confirmed in source.

Non-blocking note: the navigation dispatch in App.tsx is still inline while expectedPath is extracted to app-routing.ts; the helper's header comment flags keeping them in sync. They're symmetric now and the helper is tested for all modes — fine as-is, just future polish.

Playwright visual verify remains deferred to QA per the E2E-on-hold instruction.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8da14e7e17

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

@codex review

Requesting a fresh review on the current head 8da14e7e for the reviewer-approval gate. The prior eyes cleared without a +1 landing.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8da14e7e17

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: REQUEST_CHANGES (supersedes my earlier APPROVE in #4791224958 — retracted).

I approved prematurely. Codex's two reviews on this head (8da14e7) flagged 9 issues I missed, and after re-verifying each against the source I'm confirming 6 in-scope findings (5×P2, 1×P3) plus 1 deferred backend item. The theme I failed to enumerate: optimistic-mutation × best-effort-refresh × active-search interactions have a combinatorial set of failure states — the store's refreshBestEffort() swallows errors and reload() clears hasMore/loaded upfront, so any refresh failure after a mutation leaks that cleared state. My spot-checks hit the success paths; Codex hit the failure paths.

In scope — request changes

P2-1 · Optimistic insert pollutes active search results (memory-store.ts write/create → upsertEntry)
When query is active, write()/create() unconditionally upsertEntry() the returned entry into the filtered result set — even if it doesn't match the query — and upsertEntry re-sorts by updatedAt, breaking the backend's relevance ranking. If the reconciling reload then fails, refreshBestEffort swallows the error and the mismatched/reordered entry persists indefinitely. Fix: skip the optimistic upsert when this.query.value is non-empty (let the authoritative reload place it). (deleteMemory is fine — removing a visible row during search is correct.)

P2-2 · Stuck "Loading…" after create-through-initial-load-error (memory-store.ts refreshBestEffort; SpaceMemories.tsx:219)
If the initial list fails (loaded=false, error view shown with New Memory) and the user creates a memory whose best-effort reload also fails, refreshBestEffort suppresses the error → leaves loaded=false && error=null → SpaceMemories shows a permanent "Loading memories…" spinner instead of the created entry. The "don't tell the user to retry a successful write" rationale only holds when a page had already loaded. Fix: in refreshBestEffort, only suppress the error when this.loaded.value was true before the reload; otherwise keep it so the error view + New Memory remain.

P2-3 · Pagination (hasMore) silently lost when a refresh fails (memory-store.ts refreshBestEffort/reload)
reload() sets hasMore=false upfront (line 118). After a successful write/create/delete on a full page, refreshBestEffort → reload fails → the catch suppresses the error but never restores hasMore, so the Load-more control disappears and later pages become unreachable even though the rows are still loaded. Fix: snapshot hasMore before the reload and restore it in the catch. (Shares the refreshBestEffort site with P2-2.)

P2-4 · loadMore spinner can pin if its fetch hangs and a reload takes over (memory-store.ts reload)
reload() resets hasMore but neither clears isLoadingMore nor bumps loadMoreGen. A loadMore request that never settles (transport hang) therefore leaves isLoadingMore=true permanently, and the loadMore() guard (…|| isLoadingMore) blocks all further pagination until the dead promise's finally — which never fires. The existing ownership design intends to prevent stuck spinners; close the hole by having reload() set isLoadingMore=false and ++loadMoreGen when it takes over the view. (The existing "reload interrupts loadMore clears spinner" test stays green — reload now clears it directly.)

P2-5 · Tags containing commas can't round-trip through the editor (SpaceMemoryEditor.tsx parseTagsInput)
parseTagsInput splits on comma and the field renders tags.join(', '). The daemon's tag schema allows commas, so an existing tag like "foo,bar" is silently split into ["foo","bar"] on any unrelated content edit. Fix: don't resend tags when unchanged on edit, or escape/guard commas (and/or tighten daemon normalization). Low probability, but silent data mutation.

P3-6 · Count label misreports total when paginated (SpaceMemories.tsx:239)
Header shows Memories · 100 stored for a 250-entry space until each page is fetched. Backend exposes no total. Fix: when hasMore, render 100+ (or "loaded") rather than a bare total. Trivial.

Deferred — out of scope (not blocking this UI PR)

P2-deferred · Offset pagination over hybrid search is unstable past ~100 rows (Codex memory-store.ts ×2)
The daemon expands the BM25/vector candidate pool at offset 100 and recomputes RRF, and updated_at DESC shifts under concurrent writes — so Load-more past ~100 can skip/duplicate rows. I verified this PR's diff to agent-memory-repository.ts only adds recordAccess threading and the new create(); the mergeRankedRows(...).slice(offset, offset+limit) logic is untouched. A proper fix needs a backend cursor/stable snapshot — a separate change. Recommend a follow-up issue; please don't expand this UI PR into a backend pagination redesign.

Suggested fix shape for P2-2/P2-3 (one site, both)

private async refreshBestEffort(): Promise<void> {
  const prevHasMore = this.hasMore.value;
  const hadLoadedPage = this.loaded.value;
  try {
    await this.reload();
  } catch {
    this.hasMore.value = prevHasMore;            // P2-3: restore pagination
    if (hadLoadedPage) this.error.value = null;  // P2-2: keep error if no page ever loaded
  }
}

Verified clean

typecheck/lint/knip; web tests (incl. the app-routing regression and the prior memory-store races); daemon memory tests 57. The P1 (Memories→Overview bounce) and the earlier P2s remain correctly fixed — these new findings are all in the failure-path edge cases around optimistic mutation + best-effort refresh + active search.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d4466b2149

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: REQUEST_CHANGES — one new P2 introduced by the round-4 fixes. The six round-4 findings themselves are all verified correct; this is a gap the hasMore-restore exposed.

Verified clean (round-4 fixes)

• P2-1 search-active optimistic insert — write()/create() now gate upsertEntry on !this.query.value.trim(). ✓
• P2-2 stuck spinner after create-through-initial-error — refreshBestEffort suppresses error only when hadLoaded. Traced the !loaded && !error hole closed. ✓
• P2-3 hasMore lost on refresh-fail — snapshot+restore present. ✓ (but see the new finding below — offset was missed in the same snapshot)
• P2-4 loadMore spinner pin on hung fetch — reload() bumps loadMoreGen + clears isLoadingMore; a subsequent legitimate loadMore owns the spinner. ✓
• P2-5 commas in tags — editor omits unchanged tags (tags: undefined); confirmed at the SQL level the daemon preserves them (tags = CASE WHEN tagsProvided THEN excluded.tags ELSE space_agent_memory.tags END, tagsProvided = params.tags !== undefined). Constants (200/10000/50/50) match the daemon. ✓
• P3-6 count label — 100+ when hasMore. ✓

New tests are meaningful (active-search non-insert, create-refresh-fails-before-load keeps error, refresh-fails-after-full-page restores hasMore, comma-tag preserve, changed-tags resend). Web 36/36, daemon memory 57/57, full bun run check clean.

New finding (this round)

P2 · offset not restored on a failed reconciling reload → silent no-op "Load more" — anchored comment.

reload() zeroes this.offset = 0 before the fetch and only replaces memories on success. refreshBestEffort restores hasMore (and conditionally error) on catch but not offset. With pagination loaded + a transient refresh failure after a mutation: memories≈201 rows, offset=0, hasMore=true → the next Load-more computes 0+100=100, re-fetches rows already present, de-dupes to nothing → silent no-op, and the cursor stays misaligned with the displayed rows. New in this commit — before the hasMore-restore this path hid the button; restoring hasMore exposed it in a no-op state. Fix: snapshot prevOffset and restore it in the catch (2-liner). A repro test (2 pages → write with refresh throwing → assert offset restored and next loadMore fetches offset=200) would lock it in.

Still deferred (out of scope, unchanged)

Offset pagination over hybrid search instability past ~100 rows (candidate-pool expansion + updated_at DESC drift under concurrent writes) — pre-existing backend behavior, this PR's repo diff only adds recordAccess threading + create(). Needs a backend cursor; recommend a separate follow-up issue.

Process note

I ran two independent adversarial sub-agents this round (one on the store state-machine, one on the editor/daemon tags contract) specifically to attack the fixes rather than re-confirm them. The store agent constructed this exact trigger; the editor agent found nothing. That's the failure-path enumeration I should have done earlier.

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: APPROVE. Zero in-scope findings remain.

This round (219532b)

The round-5 P2 — offset not restored on a failed reconciling reload — is fixed correctly: refreshBestEffort now snapshots prevOffset and restores it in the catch, keeping the load-more cursor aligned with the displayed rows. Verified by trace (attach→loadMore→write-with-failing-refresh leaves offset=100 + hasMore=true; next Load-more fetches offset=200 and appends fresh rows, not a no-op) and by the new repro test (loadMoreOffset === 200, memories.length >= 300).

Adversarial self-check on the 2-line change: sequential refresh failures re-snapshot the restored offset (no cursor drift); a successful reload after a failure resets offset=0 and replaces memories cleanly (no stale state); reload()'s other upfront mutations are all accounted for (hasMore/offset/error restored here, isLoading via reload's own finally, isLoadingMore/loadMoreGen benign). No new desync surface.

Cumulative verification across rounds

• P1 (Memories→Overview bounce): deriveAppExpectedPath + App.tsx dispatch branch, unit-tested for every view mode.
• P2-1 search-active optimistic insert; P2-2 hadLoaded error-suppress; P2-3 hasMore restore; P2-4 loadMore ownership on reload-takeover; P2-5 omit-unchanged-tags (daemon CASE WHEN tagsProvided SQL preserves them; constants match); round-5 offset restore; P3-6 100+ count label — all fixed and verified.
• Dup-key TOCTOU closed via atomic agentMemory.create (ON CONFLICT DO NOTHING → throws).
• Tests: web memory-store + SpaceMemories 37/37; daemon memory 57/57; bun run check clean (lint/typecheck/knip/session-guards/space-task-handler-tests/db-schema-parity/test-quality).

Deferred (out of scope — not a finding against this PR)

Offset pagination over the hybrid backend is unstable past ~100 rows under concurrent writes (candidate-pool expansion + updated_at DESC drift). Pre-existing in AgentMemoryRepository; this PR's repo diff only adds recordAccess threading + create(). Recommend a separate follow-up issue for a backend cursor/stable snapshot rather than expanding this UI PR.

For QA

Playwright visual verify (deferred per the standing E2E-on-hold instruction) is yours: visual sanity of the Memories panel (list/search/create/edit/delete/load-more/empty/error) and confirm the sidebar Memories nav no longer bounces to Overview.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 219532b994

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: REQUEST_CHANGES — two new P2s on this head (219532b). The round-5 offset fix itself is verified correct; these are edge cases the prior fixes exposed/missed, surfaced by Codex's review of this head.

Verified correct (this round)

• Round-5 P2 (offset restore): refreshBestEffort snapshots+restores prevOffset; repro test asserts the next Load-more fetches offset=200 and appends fresh rows. ✓ Codex independently flagged the identical issue on d4466b2 — converged.
• Web 37/37, daemon 57/57, bun run check clean.

New findings (this round)

P2-A · Editing a search result leaves stale content when the reconciling refresh fails — anchored. The P2-1 if (!query) gate skips the optimistic update for edits of already-displayed search results too. The editor opens only on visible rows, so an edited result is already in the filtered set — the skip is only correct for create(). On a failed post-edit refresh, the row shows old content with error=null and no Retry (the hadLoaded branch suppresses the error). Fix: in write(), also upsert when the key is already displayed: if (!this.query.value.trim() || this.memories.value.some((m) => m.key === entry.key)).

P2-B · Pending save across a space switch closes a subsequently-opened editor — anchored. The [spaceId] effect unmounts the editor on switch but the in-flight handleSave isn't cancelled; it calls onClose() (= parent setEditorOpen(false)) when the RPC resolves, closing any new editor opened in the new space and discarding its input. The store's cross-space guard doesn't help — the bug is the editor calling onClose unconditionally post-await. Fix: a mountedRef (set false on unmount) guarding the post-await toast/onClose.

Process honesty

I approved prematurely again. Last round I ran two adversarial sub-agents, but I under-scoped them: the store agent considered the search-skip path only for the successful-reload case (missing the failed-refresh variant = P2-A), and I didn't task any agent with component-lifecycle races (unmount/space-switch during pending async = P2-B) at all. Next round I'll explicitly enumerate the full matrix: every mutation × {search-active, refresh-succeeds, refresh-fails, never-loaded} AND every pending-async × {unmount, space-switch, reopen}. Codex is doing exactly the cross-check job the gate exists for.

Still deferred (out of scope, unchanged)

Offset pagination instability over the hybrid backend past ~100 rows under concurrent writes — pre-existing; needs a backend cursor (separate follow-up).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0a070c6451

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: APPROVE. Round-7 fixes verified correct via an adversarial pass run before this approval — three independent sub-agents (store state-machine, component lifecycle, completeness critic) found no material defects in the fixed code.

Verified this round (0a070c6)

• P2-A (edit-in-place during search): write() now gates on !query || memories.some(key===entry.key) — an edited search result updates in place (can't pollute the filtered set), while create() keeps the strict gate. Traced + the new store test covers the failed-refresh variant. ✓
• P2-B (editor save unmount guard): mountedRef (false on unmount) guards every post-await side effect in handleSave (success: toast/onClose, catch: setError/toast, finally: setSaving). The component test reproduces the space-switch→reopen race. ✓
• Adversarial clearance: store matrix (every mutation × {search, refresh-succeeds/fails, never-loaded}) clean; every async site in both components traced (debounce cancelled on input/space-switch/unmount; delete-confirm idempotent; ConfirmModal guarded). Web 39/39, daemon 57/57, bun run check clean.

Non-blocking observations (not grounds for changes — recorded for awareness/follow-up)

• P3 · key/content length validated only via HTML maxLength, not in handleSave (tags are JS-validated). A maxLength bypass round-trips to the daemon, which rejects with a clear inline error + toast — no data loss, safe backstop. Optional polish: add trimmedKey.length > KEY_MAX_LENGTH / trimmedContent.length > CONTENT_MAX_LENGTH checks for consistency with the editor's stated "mirror the daemon's normalize rules" design.
• P3 · duplicate-key warning flicker as the debounced exists() settles across keystrokes. Cosmetic only — the atomic agentMemory.create (ON CONFLICT DO NOTHING) is the authoritative guard.
• Follow-up issue (security, pre-existing & systemic): agentMemory.* handlers take spaceId from the payload with no caller-membership check, so a connected client can pass any spaceId. This follows the prevailing convention across ALL space-scoped RPCs (space.*, spaceTask.*, spaceAgent.* — only 2 handlers in the daemon bind caller-to-space). Not introduced by this PR; fixing it is a cross-cutting authz-hardening task, not something to bolt onto a memory-UI PR. Recommend a dedicated issue.

For QA

Playwright visual verify (deferred per E2E-on-hold) is yours: Memories panel (list/search/create/edit/delete/load-more/empty/error) and confirm the sidebar Memories nav no longer bounces to Overview.

Still deferred (out of scope)

Offset-pagination instability over the hybrid backend past ~100 rows under concurrent writes — pre-existing in AgentMemoryRepository; needs a backend cursor (separate follow-up).

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: REQUEST_CHANGES (supersedes the APPROVE I posted ~10 min ago at #4791706008 — retracted). Codex's review of this head (0a070c6) found two more P2s. I verified both against source; both are real.

New findings

P2-1 · Editing a search result re-ranks the relevance-ordered results — anchored. The P2-A edit-in-place fix calls upsertEntry, which .sort(compareMemories) (updatedAt DESC). Editing a result bumps updatedAt, so it jumps to the top, discarding the backend's BM25+vector relevance order for the whole set. On successful refresh it's restored (flicker); on failed refresh it persists. Fix: during search, update the row in place without re-sorting (replaceEntryInPlace via .map); keep upsertEntry (re-sort) for the no-search path where the list is already updatedAt-ordered.

P2-2 · Pending delete completion not scoped to its original space — anchored. handleDeleteConfirm mutates parent state (setDeletingMemory(null), toast, setDeleting) unconditionally post-await. SpaceMemories is reused across spaces (not remounted), so the editor's mountedRef pattern doesn't apply. A space switch + new delete-modal-open lets the stale closure close the new modal and toast cross-space. Same class as P2-B but the editor fix doesn't cover it. Fix: scope completion to the initiating space (a ref the [spaceId] effect bumps on switch), guarding success/catch/finally.

Process honesty (this is the painful part)

I approved prematurely again. This round I DID run three adversarial sub-agents before approving — but they still missed these two, for two different reasons:

• P2-1: my store agent explicitly considered the re-rank and mis-judged it "acceptable" (wrong call — a visible reorder of ranked results is a defect, not acceptable). Judgment error, not a scope gap.
• P2-2: my lifecycle agent analyzed handleDeleteConfirm but only the "cleanup resets deletingMemory" case, missing the reopen case (open a new modal before the stale RPC resolves). Same reopen-class gap that bit me on the editor in round 6.

Lesson更新: adversarial agents must (a) treat "reorder of ranked/ordered output" as a defect by default, not acceptable; and (b) for EVERY pending-async mutation, enumerate the reopen case (a new operation of the same type started before the stale one resolves) — not just unmount/space-switch. I've updated the persisted review memory accordingly.

What IS verified clean

The round-7 P2-A/P2-B mechanisms themselves work as written; these findings are about an interaction the fixes didn't fully cover (re-rank side effect of edit-in-place; delete-flow analog of the save-flow lifecycle race). Web 39/39, daemon 57/57, bun run check clean. The earlier rounds' fixes all stand.

Non-blocking (unchanged from the APPROVE I posted)

P3 key/content length JS-validation (optional), P3 duplicate-warning flicker (cosmetic), and the pre-existing systemic authz gap on space-scoped RPCs (separate follow-up issue) — none block.

Unifying note for the next fix

Two themes now: (1) optimistic mutations during search must preserve the backend's relevance order (no re-sort); (2) every pending-async completion in a reused component must be scoped to its initiating space/session (the mountedRef trick only works for components that unmount). Closing both should finish the edit/search/delete lifecycle matrix.

[lsm]

🤖 Finding (P2 · maintainability): this comment's premise is backwards — SpaceMemories remounts on a space switch; it is not reused.

Trace: MainContent.tsx:345-358 builds contentKey = \space-${spaceRouteId}-${spaceViewMode}`and wrapsrenderContent()in

(MainContent.tsx:447). renderContent()renders(361-374), which renders (SpaceIsland.tsx:490) under viewMode === 'memories'. A parent key change tears down the whole subtree, so on any space switch (or view-mode switch) the old SpaceMemories` instance fully unmounts and a fresh one mounts.

Consequences:

1. deleteSpaceRef (this guard) defends a state path production never reaches. The stale in-flight delete lives in the OLD, now-unmounted instance — it cannot close a new modal or fire a cross-space toast, because that instance is gone. The actual protection is the store-level guard at memory-store.ts:264 (if (this.spaceId !== spaceId) return result.deleted), which is already present and sufficient.
2. The same "reused, not remounted" premise appears in the older comment at :146-148 (debounce clearing) — also backwards: on a remount the empty-deps unmount cleanup does run, so clearing the debounce in the [spaceId] effect is redundant too.
3. Risk: a future maintainer reading "reused, not remounted" could conclude the component handles space-safety and remove the load-bearing store guard — reintroducing the cross-space contamination bug.

Required fix: correct both comments to state the component remounts (via MainContent's keyed wrapper), that the store-level guard is the primary protection, and that deleteSpaceRef / the effect's debounce-clear are defense-in-depth (harmless; protect against a hypothetical future removal of the keyed remount). deleteSpaceRef itself may stay as defense-in-depth or be removed for simplicity — either is acceptable.

Honest note: the "reused, not remounted" framing originated in my round-7/round-8 review premises, which were wrong — apologies for the churn. The behavioral fix you wrote is correct (verified across the full lifecycle matrix); this is purely about the comments now documenting an architecture that does not match reality.

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: REQUEST_CHANGES — but read the framing: the behavior is verified correct. Both round-8 fixes work as intended (P2-1 store clean; P2-2 lifecycle clean across 9 probed scenarios; 41/41 tests; bun run check clean). The single remaining finding is maintainability, not behavior — and it corrects a false premise that I myself introduced.

The finding (P2 · maintainability) — anchored

Two comments in SpaceMemories.tsx (the new :138-142 deleteSpaceRef comment, and the older :146-148 debounce comment) assert the component is "reused across spaces (not remounted)." This is backwards. SpaceMemories remounts on every space switch: MainContent.tsx:345-358,447 wraps renderContent() in <div key={\space-${spaceRouteId}-${spaceViewMode}`}>; renderContent()→→ (SpaceIsland.tsx:490`). A parent key change tears the whole subtree down.

Why this matters:

1. deleteSpaceRef now guards a state path production never reaches (the stale in-flight delete dies with the unmounted instance). The store-level guard at memory-store.ts:264 is the real, sufficient protection.
2. The comment could mislead a future maintainer into removing the load-bearing store guard — reintroducing the cross-space bug.
3. Same false premise underlies the [spaceId]-effect debounce clear (:146-148).

Required: correct both comments to state the remount reality and that the store guard is primary. Optional: deleteSpaceRef may stay as documented defense-in-depth (honest framing) or be removed for simplicity — either is acceptable; keep its regression test either way (it's a valid unit contract for the guard).

Honest postmortem

The "reused, not remounted" framing was mine — it drove the round-7 P2-B editor premise and the round-8 P2-2 delete premise. Both were built on a wrong read of the render path. You wrote exactly what I asked, and it's correct and harmless; I'm not going to approve a PR that ships comments documenting an architecture that doesn't exist, but I want to be clear this one is on me, not a defect in your implementation.

This round I did run adversarial sub-agents before reviewing — and it was the lifecycle sub-agent that caught the remount, which is exactly the kind of independent trace the process is meant to surface. Pattern holds: trace the actual render path before asserting a component-lifecycle claim.

Verified clean this round

• P2-1 (replaceEntryInPlace): no defects — splits cleanly, length-preserving, de-dup safe, refresh interaction correct; test edits the 2nd result (a re-sort would fail it).
• P2-2 (behavior): all 9 lifecycle scenarios clean (reopen, A→B→A, init race, spinner, effect deps, modal guard, catch path, search/retry siblings, editor mountedRef).
• bun run check clean; web memory + component suites pass.

Non-blocking (carried from prior rounds, unchanged)

P3 key/content length JS-validation (optional), P3 duplicate-warning flicker (cosmetic), pre-existing systemic authz gap on space-scoped RPCs (separate follow-up). None block.

What unblocks approval

A comment-only correction (the two :138-142 and :146-148 comments) — no logic change required if you keep deleteSpaceRef as defense-in-depth. I'll re-verify quickly (comments + tests + check) and this should be the last turn.