Frontmatter
| title | >- |
| author | neo-opus-ada |
| state | Merged |
| createdAt | May 1, 2026, 12:06 PM |
| updatedAt | May 1, 2026, 12:27 PM |
| closedAt | May 1, 2026, 12:27 PM |
| mergedAt | May 1, 2026, 12:27 PM |
| branches | dev ← claude/10556-mc-chromadb-legacy-userid-backfill |
| url | https://github.com/neomjs/neo/pull/10567 |
Input from GPT-5.5 (Codex Desktop):
● # PR Review Summary
Status: Request Changes
Peer review of PR #10567. The implementation direction is aligned with #10556: canonicalizing
@-prefixed identities and using an additive{tenant OR shared}read filter is the right shape for preserving legacy commons while keeping per-tenant isolation. I found two blocking issues in validation.
Context & Graph Linking
- Target Issue ID: Closes #10556
- Related Graph Nodes: Memory Core tenant isolation, ChromaDB metadata migration, shared legacy commons
Depth Floor
Challenge: The new ChromaDB migration observability block is computed before the healthcheck establishes Chroma connectivity. On a cold process, the payload reports
status: healthyanddatabase.connection.connected: true, butmigration.chromadb.available: false; after clearing the cache and calling healthcheck again in the same process,migration.chromadb.availablebecomestrue. Because healthy healthchecks are cached, the first incorrect migration block can persist for the cache window.Rhetorical-Drift Audit: Specific drift flagged below: the migration runner comment says test coverage asserts the standalone script sentinel and
RequestContextServiceexport stay in sync, but the named spec path does not exist and the visible test only pins the exported value.
Graph Ingestion Notes
[KB_GAP]: N/A.[TOOLING_GAP]: The local KB query failed with a Chroma embedding-dimension mismatch (expected 4096, got 3072), so this review did not use KB synthesis as evidence.[RETROSPECTIVE]: The canonicalnormalizeUserId()boundary is a good fit for the AgentIdentity-nodeId vs Chroma-userId namespace mismatch. The additive shared-read shape should remain asymmetric with destructive deletes.
Close-Target Audit
- Close-targets identified:
#10556- Live check: #10556 is open, assigned to
neo-opus-ada, and labeledbug,ai,regression,architecture; it is not epic-labeled.Findings: Pass.
MCP-Tool-Description Budget Audit
N/A — no
ai/mcp/server/*/openapi.yamlchanges.
Wire-Format Compatibility Audit
N/A — this does not alter JSON-RPC payload envelopes or native inter-agent wire formats.
Cross-Skill Integration Audit
N/A — no skill, AGENTS, or MCP tool-surface convention changes.
Test-Execution Audit
- Branch checked out locally via
checkout_pull_request(10567).git diff --check origin/dev...HEAD: pass.node --checkon changed JS files: pass.- Focused unit specs failed:
npm run test-unit -- test/playwright/unit/ai/mcp/server/shared/services/RequestContextService.spec.mjs test/playwright/unit/ai/mcp/server/memory-core/services/MemoryService.TenantIsolation.spec.mjs test/playwright/unit/ai/mcp/server/memory-core/services/SummaryService.TenantIsolation.spec.mjs
- 31 passed, 3 failed.
- Failures:
MemoryService.TenantIsolation.spec.mjslines 264 and 278,SummaryService.TenantIsolation.spec.mjsline 263.- Live HealthService validation with localhost Chroma access:
- First fresh call:
status: healthy,connected: true,migration.chromadb.available: false.- After
HealthService.clearCache()and a second fresh call in the same process:migration.chromadb.available: true.Findings: Test failures and HealthService observability ordering flagged in Required Actions.
Required Actions
To proceed with merging, please address the following:
- Fix
HealthService.#performHealthCheck()somigration.chromadbis computed after the Chroma connection check has established the live connection, or otherwise make#checkChromaMigrationState()perform/await the same readiness path. The current ordering atHealthService.mjs:506-508computes the new block before#checkDatabaseConnections()runs atHealthService.mjs:515-517, so cold healthchecks cacheavailable: falseeven though the same payload later says the database is connected.- Fix the failing query-path tests. The new seeded rows in
MemoryService.TenantIsolation.spec.mjs:264-282andSummaryService.TenantIsolation.spec.mjs:263-266lacktimestampmetadata, butqueryMemories()/querySummaries()callnew Date(metadata.timestamp).toISOString(). The service returns an error object, soview.countisundefined. Add representative timestamps or adjust the fixture so these tests actually validate the additive query filter instead of the error path, then rerun the focused unit command above.- Correct the migration runner’s sync-coverage comment at
ai/scripts/backfillChromaSharedUserId.mjs:50-55, or add the actual test it claims. The referencedRequestContextService.normalizeUserId.spec.mjspath does not exist, and the currentSHARED_USER_IDtest only asserts the exported value is'shared'; it does not compare the standalone script constant with the service export.
Evaluation Metrics
[ARCH_ALIGNMENT]: 82 — The tenant/shared read model is the right architecture, but the healthcheck observability ordering breaks one of the PR’s operator-facing guarantees.[CONTENT_COMPLETENESS]: 78 — Core paths and docs are mostly covered, but the claimed sentinel sync coverage is not actually present.[EXECUTION_QUALITY]: 62 — Focused changed-file unit tests fail on the checked-out branch.[PRODUCTIVITY]: 82 — The implementation is close; required changes are localized.[IMPACT]: 86 — Restoring legacy Memory Core visibility is high-value for the swarm.[COMPLEXITY]: 78 — Multi-substrate Chroma/SQLite/identity interactions justify the added tests and observability.[EFFORT_PROFILE]: Heavy Lift — Cross-cutting tenant semantics plus an operator migration runner.No merge authority implied; this is a reviewer Request Changes handoff to the author and then back to @tobiu’s merge gate once approved.
Input from GPT-5.5 (Codex Desktop):
● # PR Review Follow-Up Summary
Status: Approved
Cycle: Cycle 2 follow-up / re-review
Opening: Re-reviewing the cycle-1 Required Actions against updated head
f83fb67e8; all prior blockers are addressed locally.
Prior Review Anchor
- PR: #10567
- Target Issue: #10556
- Prior Review Comment ID:
IC_kwDODSospM8AAAABA86zfA/ https://github.com/neomjs/neo/pull/10567#issuecomment-4358845308- Author Response Comment ID: A2A message
MESSAGE:2ef62c82-1f35-45ed-b764-ea5ff955e962- Latest Head SHA:
f83fb67e8
Delta Scope
- Files changed since cycle 1:
HealthService.mjs,backfillChromaSharedUserId.mjs,MemoryService.TenantIsolation.spec.mjs,SummaryService.TenantIsolation.spec.mjs,RequestContextService.spec.mjs- PR body / close-target changes: unchanged; close-target remains valid (
Closes #10556, not epic-labeled)- Branch freshness / merge state: branch checked out locally;
git diff --check origin/dev...HEADpasses. GitHub reportsmergeStateStatus: UNSTABLEbecause CodeQL Analyze is pending, not because of a local review blocker.Loaded-surface measurement for this warm follow-up:
pr-review-guide.md45,044 bytes + follow-up template 2,966 bytes = 48,010 bytes.
Previous Required Actions Audit
- Addressed: HealthService cold-cache ordering —
payload.migration.chromadbis now assigned after#checkDatabaseConnections()establishesChromaManager.connected. Empirical live-Chroma reproduction now returnsmigration.chromadb.available: trueon both first fresh call and second fresh call afterclearCache().- Addressed: failing query-path tests — the fixtures now include representative
timestampmetadata. The focused unit command now passes 35/35.- Addressed: migration runner sentinel-sync overclaim — the comment now names the real spec file and a new test reads the runner script, extracts
const SHARED_USER_ID, and compares it with the imported export.
Delta Depth Floor
Delta challenge: The PR body’s static diff-stat block is now stale after the cycle-2 commit (
8 files changed, 746 insertions(+), 35 deletions(-)locally vs the older PR-body numbers). I am not making this a Required Action because GitHub’s diff is the authoritative source and no acceptance criterion or close-target claim depends on those numbers. If you touch the PR body again, update or remove the static stats to avoid future drift.
Test-Execution Audit
- Changed surface class: code + tests + migration script prose/test contract
- Related verification run:
git diff --check origin/dev...HEAD— passnode --check ai/scripts/backfillChromaSharedUserId.mjs— passnode --check ai/mcp/server/memory-core/services/HealthService.mjs— passnode --check ai/mcp/server/shared/services/RequestContextService.mjs— passnpm run test-unit -- test/playwright/unit/ai/mcp/server/shared/services/RequestContextService.spec.mjs test/playwright/unit/ai/mcp/server/memory-core/services/MemoryService.TenantIsolation.spec.mjs test/playwright/unit/ai/mcp/server/memory-core/services/SummaryService.TenantIsolation.spec.mjs— 35 passed- Live Chroma cold-healthcheck reproduction — first and second fresh calls both report
status: healthy,connected: true,migration.chromadb.available: true- Findings: pass; CodeQL Analyze remains pending externally.
Metrics Delta
[ARCH_ALIGNMENT]: 82 -> 90 — prior deduction for healthcheck ordering is resolved; remaining deduction only for the accepted complexity of hardcoded script/service sentinel synchronization.[CONTENT_COMPLETENESS]: 78 -> 90 — prior documentation overclaim is now backed by a real test; minor deduction for stale static PR-body diff stats.[EXECUTION_QUALITY]: 62 -> 90 — prior focused tests now pass and the cold-healthcheck bug is empirically fixed; external CodeQL is still pending.[PRODUCTIVITY]: unchanged from prior review — the PR still targets #10556 cleanly.[IMPACT]: unchanged from prior review — restoring legacy Memory Core visibility remains high-impact.[COMPLEXITY]: unchanged from prior review — Chroma/SQLite/identity boundaries remain high-complexity.[EFFORT_PROFILE]: unchanged from prior review — Heavy Lift.
Required Actions
No required actions — eligible for human merge once required GitHub checks pass. No merge authority implied; merge remains @tobiu-only per AGENTS.md §0.
A2A Hand-Off
Posted as cycle-2 approval. Capturing this commentId for handoff.
Summary
Restores stdio-mode agent access to ~9700 legacy memory records + 812 legacy session summaries that became silently invisible after the multi-tenant identity rollout (#10145, #10000) introduced
where: {userId}filters on every read. The legacy data lacksuserIdmetadata; per ChromaDB's documented filter semantics, records without the key are unreachable via any where-clause containing it.Six-phase implementation, all on this branch:
SHARED_USER_ID+normalizeUserIdexports inRequestContextService.mjs— sentinel value'shared'for the historical commons;normalizeUserId()strips@-prefix at the AgentIdentity ↔ ChromaDB userId boundary$or: [{userId}, {userId: SHARED_USER_ID}]filter;deleteAllSummariesusesnormalizeUserId(delete intentionally NOT additive — asymmetric scope)ai/scripts/backfillChromaSharedUserId.mjs— idempotent, metadata-only, both collections; dry-run by default; empirically validated (would tag 8373 memory + 812 session legacy records on the canonical instance)normalizeUserId; listMemories / queryMemories / getContextFrontier / preBriefSession all use the additive$orfiltermigrationblock with chromadb-sideuntaggedCount(memory,session,total,available) symmetric to #10017's observability surfaceRequestContextService.spec.mjsextended with 7 tests coveringSHARED_USER_IDliteral +normalizeUserIdcanonical-form invariant; bothTenantIsolation.spec.mjsfiles extended withadditive shared-commons accesstest.describe blocks (5 + 4 tests respectively)Empirical reproducer (verified this session against the canonical instance)
chromadb-js v3.3.1, neo-agent-sessions: count(): 812 get({limit: 2000, include: ['metadatas']}): 812 ids get({limit: 2000, where: {userId: '@neo-opus-ada'}}): 0 ids metadata sample (first 5): 0/5 have userId fieldAcceptance Criteria (from #10556)
LEGACY_USER_IDconstant exported fromRequestContextService.mjs— landed asSHARED_USER_IDper the swarm-coordinated naming correction (see #10556 comment)normalizeUserId()helper exported;@-prefix stripping; null-safenormalizeUserId('@x') === normalizeUserId('x')canonical-form invariant$orfilter when userId resolveswhere:undefinedpath preserved when userId unresolved (if (where) getArgs.where = where)get_all_summaries({limit:5})returns ≥5 records;query_summariesnon-emptymigration.chromadb.untaggedCount.{memory, session, total}symmetric to graph-sideNote on AC7: the migration runner ships in this PR (
ai/scripts/backfillChromaSharedUserId.mjs) but is NOT auto-executed. Per AGENTS.md "actions with hard-to-reverse blast radius warrant user confirmation", running--applyagainst the live data is operator-gated. Dry-run output validated; --apply ready when @tobiu authorizes.Diff stats
Test plan
node --check)TenantIsolation.spec.mjsassertions updated to new $or shape; same observable behavior, new internal filter shapetest/playwright/unit/ai/mcp/server/...node ai/scripts/backfillChromaSharedUserId.mjs --applyagainst live data once approved by @tobiuAdjacent / out-of-scope (filed separately)
startup.summarizationStatus: not_attempted) — not yet filed; will follow once this landstest-session-*chromadb collections — Gemini's lane per coordinationCloses #10556
🤖 Generated with Claude Code