Skip to content

fix: address round-4 review feedback on semantic memory - #882

Open
Dhirenderchoudhary wants to merge 1 commit into
Nano-Collective:feat/semantic-memoryfrom
Dhirenderchoudhary:fix/619-semantic-memory-review-fixes-v4
Open

fix: address round-4 review feedback on semantic memory#882
Dhirenderchoudhary wants to merge 1 commit into
Nano-Collective:feat/semantic-memoryfrom
Dhirenderchoudhary:fix/619-semantic-memory-review-fixes-v4

Conversation

@Dhirenderchoudhary

@Dhirenderchoudhary Dhirenderchoudhary commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Description

Addresses comments of the semantic memory feature (#619): #619 (comment)

Revised merge bar, all four items:

  • /memory accept <n> re-indexing bug fixed — accepted indices are now tracked in a Set instead of splicing the proposal array, so accepting one proposal no longer shifts the numbers under proposals still on screen.
  • Docs settings-path contradiction fixed — both docs now correctly say /settings → Advanced → Semantic Memory.
  • Token budget wired to preferences — resolveProjectContextPreferences() is the single source of truth, wired into all three call sites (TUI, ACP agent, plain shell), plus two new cyclable rows in the Advanced settings panel.
  • /memory list short IDs — memory.ts rewritten as a themed memory.tsx Ink component (matching tasks.tsx/mcp.tsx/agents.tsx), with 8-char short IDs and unambiguous-prefix resolution replacing raw 121-char UUID lines.

Also hardened the assistant-reversal detector per the reviewer's own follow-up: added a real contradiction check between assistant turns (isContradiction), widened the technical-evidence guard so a bare / no longer disables detection, and made the walk-back skip tool-call-only turns instead of bailing on any intervening assistant message. Plus: fenced project-context content can no longer be escaped by backticks in memory content, /memory propose output is now bounded (last 40 messages scanned, 20 proposals max), and the semanticMemoryEnabled default no longer lives in three separate places.

Closes #619

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

Ran tsc --noEmit (clean), biome check on every touched file (clean), and targeted ava runs covering every touched spec file — 142+ tests passing, 0 failing.

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Checklist

  • If this was for an open issue, I was assigned to it
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md) — not applicable, no new logging paths in this change

@Dhirenderchoudhary
Dhirenderchoudhary marked this pull request as ready for review August 15, 2026 18:19
@Dhirenderchoudhary
Dhirenderchoudhary force-pushed the fix/619-semantic-memory-review-fixes-v4 branch from 3ed5fb7 to eb7355a Compare August 15, 2026 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant