Feedback on memory usage guidance for LLMs — the current agent-profile block and MCP docstrings will likely produce store-happy but low-signal behavior. Some gaps worth closing:

1. No guidance on scope. The tool accepts global|project|session|agent but neither the docstrings nor the profile explain when to use which. Given the project default, an LLM told to persist "user preferences" will almost always land in project when most belong in global. This is the biggest likely failure mode.
2. No guidance on memory_type. Four types (user|feedback|project|reference) with no decision criteria; the default is project, so the type dimension will collapse regardless of content.
3. No "when NOT to store" list. Without explicit exclusions (code patterns derivable from the repo, git history, ephemeral task state, fix recipes), agents will accumulate redundant/decaying entries like "fixed bug in X" or "ran tests and they passed."
4. No key-naming convention. The auto-slug from the first 6 words will collide and hurt search. Agents need a nudge to pick stable, topical slugs.
5. recall triggers are too narrow. "Check before asking the user" misses the more useful trigger: before starting a task in an area, recall what we know about it. LLMs tend to skip recall entirely when a task feels self-contained.
6. No post-injection feedback loop. The <cao-memory> block shows what's loaded on first message, but after that the agent has no visibility into the store unless it calls recall, and nothing nudges it to.

Net effect: the "ALWAYS store" rule is forceful enough that agents will store things, but scope/type selection will be near-random and recall will be underused mid-task. Consider expanding the profile Memory section with (a) a scope/type decision rubric with examples, (b) an explicit exclusion list, and (c) explicit save/recall triggers.

@fanhongy thanks for the great work, it delivered majority of the features as discussed. Here are some feedbacks:

PR #179 — Phase 1 Scoped Review

TL;DR

• Status: Request changes. ~70% of Phase 1 spec shipped (13 ✅ / 5 ⚠️ / 2 ❌).
• Two merge blockers:
  1. C1 — memory_forget registered as a Phase 1 MCP tool; design explicitly labels it Phase 2. (Given the complexity of the PR, i suggest move this to next PR)
  2. C3 — SQLite metadata layer spec'd in four separate places in the doc; not shipped.
• Both blockers are doc-vs-code mismatches — resolution can be "fix the code" or "fix the doc," but today they disagree.

Scope: This review compares PR #179 against only the Phase 1 (MVP) section of memory-management.md (L530-550) and Phase 1 Success Criteria (L587-594). Out-of-scope items (Phase 2–4) are not graded.

Method: item-by-item walk of each Phase 1 deliverable against the shipped code.

Phase 1 deliverables

§Phase 1 — Memory creation

§Phase 1 — Hook-triggered self-save

§Phase 1 — Context delivery

§Phase 1 — Storage setup

§Phase 1 Success Criteria — verdict per item

Summary scorecard

Counting only the 20 main deliverables (treating #3a as a sub-note on #3):

Sub-note #3a (memory_forget exposed via MCP) is a doc-vs-code mismatch on top of shipped functionality and is tracked separately under C1 below.

Phase 1 comments — by severity

Must fix for Phase 1 merge

C1 — memory_forget is explicitly Phase 2; shipped as Phase 1 MCP tool.
The design doc is unambiguous at L169 (### memory_forget (Phase 2)) and L535 ("forget is an internal method for cleanup; MCP exposure via memory_forget is Phase 2"). The PR registers it anyway at mcp_server/server.py:765-794.-- Sugest remove this to a separate PR

Current state — shipped without doc update — means the design doc lies to anyone reading it.

C2 — REST CRUD endpoints missing.
§Phase 1 L537 calls for POST /memory, GET /memory, DELETE /memory, GET /terminals/{id}/memory. Only the last exists — and even that is shipped as GET /terminals/{terminal_id}/memory-context (api/main.py:437-458), with a different URL suffix than the spec.

Fix options (pick one):

1. Implement the three missing endpoints — they're thin wrappers over MemoryService calls.
2. Update §Phase 1 L537 to say "MCP + CLI only; REST CRUD deferred to Phase 4 alongside Web UI" and explain the rationale (web UI is Phase 4, no non-UI HTTP consumer exists yet).

Either works — the PR and doc just need to agree.

C3 — SQLite metadata layer absent.

One-line summary: The design describes a hybrid wiki + SQLite architecture in four separate places; the code ships file-only.

Design citations (all in memory-management.md): L533 (Phase 1 deliverable: MemoryMetadataModel table + migration), L543 (context delivery spec'd as "SQLite metadata query"), L593 (success criterion: "wiki files + SQLite metadata persisted"), L670-671 (§Rec 1 hybrid call-out), L743-749 (§Rec 5 full "Hybrid Storage" argument). The PR description acknowledges the divergence.

Why this still needs action:

• cleanup_service._find_expired_entries (L164-167) and memory_service._parse_index (L461-464) are two hand-rolled regex variants of the same format — drift risk §Rec 5 explicitly warns about.
• recall() and get_memory_context_for_terminal do O(N) regex-parse + file-read per call. Fine at Phase 1 scale; the design chose SQLite specifically to not worry about this.

Fix options (pick one):

1. Add a MemoryMetadataModel SQLAlchemy table that mirrors the index.md bullet line. Write to both on store; read from SQLite on recall and _find_expired_entries. Keep index.md as the human-readable index.
2. Update memory-management.md to re-scope Phase 1 to file-only — specifically: L533 (drop the MemoryMetadataModel line), L543 (replace with "index.md metadata scan"), L593 (drop "+ SQLite metadata"), and add a note to §Rec 5 explaining why Phase 1 ships without SQLite despite the hybrid pitch.

The doc and the code should agree. Right now they don't.

Should fix before merge

C4 — Per-scope injection cap (§Rec 6) not enforced.
get_memory_context_for_terminal(budget_chars=3000) is a single global budget (L615-628). §Rec 6 L756 specifies "Hard limit of 10 most recent memories per scope, max ~2KB total injection" — note per scope. A scope with 100 memories will monopolize the budget.

Fix: In get_memory_context_for_terminal, take at most N=10 memories per scope before applying the byte budget. Small change — one [:10] slice inside the scope loop. Keeps injection balanced across session/project/global.

C5 — Universal first-message injection vs per-provider table in §6a.

This is primarily a design-doc ambiguity, not a code failure. L543-544 and §6a L765-772 prescribe per-provider injection; L774 then licenses "Phase 1 simplification: Use instruction file/system prompt injection for all providers". The PR follows L774. Action is on the doc owner to pick one reading, then update Phase 1 Success Criterion S3 (L591) to match.

Shipped behavior: terminal_service.inject_memory_context (L52-74) prepends <cao-memory> to the first user message for all providers except Kiro. Kiro alone uses the agentSpawn hook.

If the doc settles on per-provider: push the injection logic behind a BaseProvider.format_memory_injection() method — matches @patricka3125 comment #4 and sets up Phase 2 cache-aware separation cleanly.

Nice to fix — low risk items worth noting

C6 — _memory_injected_terminals is module-level in-memory state.
terminal_service.py:49 — lost on server restart. Next user message after a restart mid-session will re-inject the memory block (harmless but surprising). Could be backed by a DB column or a per-terminal flag file. Low priority for Phase 1 — worth a comment at the definition site noting the known limitation.

C7 — Index regex drift risk (tied to C3).
memory_service._parse_index regex (L461-464) and cleanup_service._find_expired_entries regex (L164-167) are two separate hand-rolled patterns for the same format. Right now they agree — but there's no invariant enforcing that. Add a single test:

def test_parse_index_reads_what_update_index_writes():
    svc = MemoryService(base_dir=tmp_path)
    await svc.store(content="x", key="test-key", memory_type="project", scope="project", terminal_context={...})
    entries = svc._parse_index(svc.get_index_path("project", scope_id))
    assert entries[0]["key"] == "test-key"
    # Also parse with cleanup regex and assert it matches the same line

Or, if C3 is resolved by adding SQLite, this goes away entirely.

C8 — Hook flag file cleanup is time-based, not PID-based.
_cleanup_stale_hook_flags removes flag files older than 5 min (cleanup_service.py:218-242). If a Claude Code session runs a long operation inside a Stop hook (rare but possible during large memory_store calls), the flag could be deleted while still in use. PID-based cleanup (kill -0 $pid check) is more precise. Low priority — time-based is fine for Phase 1 if documented.

C9 — register_hooks_kiro always rewrites the agent config.
Every cao launch triggers register_hooks_kiro which self-heals the .json by removing all existing CAO entries and re-adding them (hooks/registration.py:174-186). Two concerns:

1. If a user hand-edits ~/.kiro/agents/{profile}.json to tweak a CAO hook, their edit is silently erased on next launch.
2. Writing to a user-managed config outside the CAO-owned filesystem is what @patricka3125's comment Feature: async delegate #3 flagged. The spec doesn't explicitly prohibit it, but Kiro's AgentSpawn hook is the only way CAO can inject memory into Kiro, so this is load-bearing. Worth either documenting the behavior in docs/memory.md or adding a "managed-by: cao" marker comment in the hook entries.

C10 — cao memory forget CLI + memory_forget MCP both delegate to MemoryService.forget().
If C1 is resolved by removing the MCP tool, the CLI command (cli/commands/memory.py delete/clear) still exposes the operation to the user — which is the intent of §User-control question (L641): "Should users be able to view, edit, and delete memories via CLI before Phase 4 web UI?". So the CLI path is correct Phase 1 behavior; only the MCP surface needs adjustment.

Testing — Phase 1 gaps

Phase 1 Success Criteria S1-S6 should each have at least one automated test. Coverage review:

Recommended test additions (low effort, high signal):

1. Writer/reader regex round-trip test (guards against C7 drift).
2. Concurrent _update_index test — two threads calling store() simultaneously should produce two index entries, not one corrupted one. fcntl.flock is the safety net being tested.
3. Restart durability test (S5).

Net — Phase 1

13 / 20 deliverables ✅, 5 / 20 ⚠️, 2 / 20 ❌ (plus sub-note #3a — doc/code mismatch). Call it ~70% spec-fidelity for Phase 1 — same number as the broader review arrived at, but this time with each item pinned to a specific design-doc line.

Merge-blocking: two items.

1. C1 (memory_forget in Phase 1 MCP) — pick one of the two fixes above. Currently the code ships a tool the doc says is Phase 2, with no rationale.
2. C3 (SQLite metadata layer absent) — pick one of the two fixes above. Currently the design doc describes a hybrid architecture and the code ships a file-only one. Either implement the hybrid or re-scope the doc.

Strongly recommended before merge:
3. C2 (REST CRUD) — implement or re-scope.
4. C4 (per-scope injection cap) — one-line fix, trivial.
5. Reply to @patricka3125's 7 inline comments — at least items 3, 4, 6, 7 have concrete code actions; 1, 2, 5 are design discussions the PR author should weigh in on.

Post-merge acceptable (Phase 2 rollover):

• C5 (per-provider injection), C6 (in-memory tracker), C7 (regex drift if SQLite adopted), C8 (PID-based flag cleanup), C9 (Kiro config ownership)

The Phase 1 implementation is solid work — security posture is good, retention is faithful, hook registration is idempotent and self-healing, and the wiki+index pattern matches §Rec 1 exactly. The gaps are specific and fixable. Fix C1 and C3, reconcile the doc, and Phase 1 is mergeable.

The work evolved through Phase 2 (cao-memory-feature), Phase 2.5 (feat/memory-implementation-phase-1), and a plugin-migration pass. Summary of what landed.

Storage + correctness

• SQLite metadata layer — MemoryMetadataModel in clients/database.py:72 with WAL mode and UniqueConstraint(key, scope, scope_id). Wiki files remain the source of truth; SQLite is the fast-lookup + concurrency mechanism.
• Per-scope injection cap — MEMORY_MAX_PER_SCOPE=10 and MEMORY_SCOPE_BUDGET_CHARS=1000 enforced in get_memory_context_for_terminal(). No single scope can monopolize the envelope.
• Index round-trip invariant — writer/reader regex drift is locked by a test (test_memory_service_index_roundtrip.py); ISO-8601 Z-suffix strptime format is part of the contract.
• Durability + concurrent writes — MemoryService reinstantiation recovers all prior memories via SQLite; concurrent writers to the same scope are serialized by per-connection SQLite locking + atomic os.replace(). fcntl.flock remains as defense-in-depth in the fallback path.

Architecture

• Opt-in flag — memory.enabled settings key. When false, writes raise, get_memory_context_for_terminal() returns "", and no filesystem or SQLite side-effects occur. Default behavior unchanged.
• Stable project identity — three-tier resolver (explicit config → git remote → cwd hash) plus an alias table. Memories survive directory rename and resolve identically across git worktrees. Dry-run migration planner (plan_project_dir_migration) classifies renames as none | rename | merge | conflict; live --apply is deferred to Phase 3.
• Hook registration on BaseProvider — terminal_service.py no longer contains any if provider == ProviderType.* hook conditionals. BaseProvider.register_hooks() is a no-op default; providers that need hooks override it.
• Plugin migration — memory hooks for Claude Code and Kiro CLI now ship as entry-point plugins under src/cli_agent_orchestrator/plugins/builtin/. The hooks/ directory has been retired entirely. Non-CAO Claude Code sessions are now hook-free.

Scope cleanup

• REST CRUD endpoints — formally deferred to Phase 3. Surface is now 1 shipped (GET /terminals/{id}/memory-context) + 3 deferred write endpoints. Rationale and Phase 3 prerequisites in aidlc-docs/phase2.5/phase3-backlog.md.
• Spec sync — design docs reconciled against shipped reality. Stale Phase-2 labels on memory_forget corrected to Phase 1 (already shipped as an MCP tool in this PR).

Documentation

• docs/memory.md gained dedicated subsections for Codex, Gemini/Kimi/Copilot (all instruction-only; no native hooks), and the plugin architecture.
• Agent-profile guidance is now split into Minimal (default template; matches built-in profiles) and Aggressive (test-harness template with concrete store/recall triggers plus session-start / session-end rituals). The Aggressive template is used by the -local variants under ~/.aws/cli-agent-orchestrator/agent-store/; built-ins will be upgraded once the memory feature exits dogfood.

Quality gates

• All 9 Phase 2.5 units passed a two-gate review (challenger + security-reviewer) using a source-mutation teeth-test methodology. Audit trail: aidlc-docs/phase2.5/audit.md.
• 1,504+ tests passing, zero critical findings across both gates.
• 14 CodeQL path-expression alerts previously flagged on hooks/registration.py — the file is now deleted, so the alerts should auto-resolve on this push.

Known deferrals (Phase 3)

Non-blocking items rolled forward: 9 INFO-severity findings from security review + the 3 REST write endpoints. Full list in aidlc-docs/phase2.5/phase3-backlog.md.

Happy to walk any specific change in more detail.

Converting this PR to Draft to split it into multiple for review purpose