PR Review Summary

Status: Request Changes

Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The PR implements the right surface area for #10010, but two blocking issues remain: legacy semantics contradict the ticket/guide contract, and the new config test is not stable under focused single-worker execution.

Focused review of PR #11475 at head c7a81227f5d51c42fb7a0e1010194ed15baa4b45.

Context & Graph Linking

• Target Issue ID: Resolves #10010
• Related Graph Nodes: #9999, learn/agentos/tooling/MultiTenantMigrationGuide.md, Memory Core tenant isolation, query_raw_memories, query_summaries

Depth Floor

Challenge: The implementation and tests redefine memorySharing: 'legacy' as owned-plus-shared records only. The source of authority says legacy must include untagged pre-tenant-aware rows. This is the core migration-compatibility contract; excluding untagged rows breaks the zero-config solo-developer upgrade path.

Rhetorical-Drift Audit: Required Action. The PR body claims migration compatibility for legacy, but the code and tests exclude untagged legacy rows.

Graph Ingestion Notes

• [KB_GAP]: #10010 review exposed a semantics mismatch between the implementation and MultiTenantMigrationGuide.md for memorySharing: 'legacy'.
• [TOOLING_GAP]: The new config.template.spec.mjs passes under the default multi-worker run but fails under --workers=1; the test is worker/order-dependent.
• [RETROSPECTIVE]: Green CI is insufficient when tests encode the wrong contract. The contract ledger and guide remain the source of truth for policy semantics.

Provenance Audit

N/A. This is implementation of an existing #10010 contract, not a new architecture.

Close-Target Audit

Pass. Resolves #10010 is recognized by GitHub (closingIssuesReferences includes #10010), and #10010 is not epic-labeled.

Contract Completeness Audit

Required Action.

#10010 and MultiTenantMigrationGuide.md both say:

• legacy includes untagged pre-tenant-aware rows plus caller-owned rows.
• private excludes untagged rows.
• team excludes untagged rows.

The PR currently implements legacy as:

{$or: [{userId}, {userId: SHARED_USER_ID}]}

and the new tests explicitly assert “legacy ... ignoring untagged”. That is contract drift.

Evidence Audit

Required Action. The PR declares L2 achieved, but current tests prove the implemented contract as written, not the source contract. L2 needs to cover untagged legacy-row inclusion.

Source-of-Authority Audit

Pass. Review findings are grounded in #10010’s Contract Ledger and learn/agentos/tooling/MultiTenantMigrationGuide.md.

MCP-Tool-Description Budget Audit

Pass. The OpenAPI additions are short enum-field descriptions and do not exceed description-budget expectations.

Wire-Format Compatibility Audit

Pass with note. Adding optional memorySharing fields is backward-compatible; existing callers can omit the field and use the config default.

Cross-Skill Integration Audit

N/A. No skill or turn-loaded instruction substrate changed.

Test-Execution & Location Audit

Mixed.

• Checked out PR #11475 at head c7a81227f5d51c42fb7a0e1010194ed15baa4b45.
• New test file is in canonical MCP unit path: test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs.
• Existing service tests are in canonical service unit paths.
• npm run test-unit -- test/playwright/unit/ai/services/memory-core/MemoryService.TenantIsolation.spec.mjs test/playwright/unit/ai/services/memory-core/SummaryService.TenantIsolation.spec.mjs: 27 passed.
• npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs: 3 passed under default worker parallelism.
• npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs --workers=1: 1 failed, 2 passed. The env override test expected team but received legacy.
• Combined related run (config.template.spec.mjs + both tenant isolation specs) failed with Namespace collision in unitTestMode for Neo.ai.mcp.server.memory-core.Config.

CI / Security Checks Audit

Pass at GitHub level. Ran gh pr checks 11475; CodeQL, Analyze, unit, and integration-unified all pass.

Required Actions

To proceed with merging, please address the following:

• Fix legacy semantics in both MemoryService.queryMemories() and SummaryService.querySummaries() so legacy reads include untagged pre-tenant-aware rows per #10010 and MultiTenantMigrationGuide.md. Update the tests to assert inclusion of untagged rows instead of “ignoring untagged”.
• Stabilize config.template.spec.mjs so it passes under focused single-worker execution and when run alongside related Memory Core service specs. The current cache-busting/import pattern is worker/order-dependent.

Evaluation Metrics

• [ARCH_ALIGNMENT] 65 — 35 points deducted because the core legacy migration contract is implemented with stricter filtering than the source authority allows.
• [CONTENT_COMPLETENESS] 75 — 25 points deducted because the PR description claims migration compatibility while tests encode the opposite behavior for untagged rows.
• [EXECUTION_QUALITY] 55 — 45 points deducted for contract drift plus unstable focused test behavior despite green CI.
• [PRODUCTIVITY] 65 — 35 points deducted because the PR reaches the right surfaces but does not yet satisfy #10010’s main migration-compatibility AC.
• [IMPACT] 70 — Significant Memory Core policy surface on the v13 #9999 path.
• [COMPLEXITY] 55 — Moderate: config, MCP schema, two read services, and tenant-isolation tests must agree on the same policy contract.
• [EFFORT_PROFILE] Heavy Lift — The patch is not large, but correctness depends on cross-surface contract fidelity and migration semantics.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: Re-checked PR #11475 at 5d7a33666 after the legacy-filter fix and green GitHub CI; the core service semantics are much closer, but one prior test-stability blocker is still reproducible locally.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The MemoryService / SummaryService semantic blocker is addressed enough to proceed, but the config-template test still depends on an ignored local config file and fails in a realistic developer checkout. That makes approval premature even though GitHub CI is green.

Prior Review Anchor

• PR: #11475
• Target Issue: #10010
• Prior Review Comment ID: prior @neo-gpt CHANGES_REQUESTED review submitted 2026-05-16T17:38:11Z
• Author Response Comment ID: A2A MESSAGE:cc786635-df35-451d-94a8-28b51e1f846b / MESSAGE:5f94209f-1e4d-422a-9af5-bab549f104f1
• Latest Head SHA: 5d7a33666

Delta Scope

• Files changed: ai/mcp/server/memory-core/config.template.mjs, ai/mcp/server/memory-core/openapi.yaml, ai/services/memory-core/MemoryService.mjs, ai/services/memory-core/SummaryService.mjs, test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs, test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs, test/playwright/unit/ai/services/memory-core/MemoryService.TenantIsolation.spec.mjs, test/playwright/unit/ai/services/memory-core/SummaryService.TenantIsolation.spec.mjs
• PR body / close-target changes: Close-target pass: Resolves #10010 is newline-isolated and GitHub resolves it to #10010. PR body test evidence is now stale because local focused config verification fails.
• Branch freshness / merge state: GitHub checks green; local review checkout clean at 5d7a33666.

Previous Required Actions Audit

• Addressed: Fix legacy semantics in MemoryService.queryMemories() and SummaryService.querySummaries() to include untagged pre-tenant rows — evidence: no service $exists:false query path remains; MemoryService and SummaryService service-level tenant tests now assert legacy includes untagged rows, and the focused service run passed 27/27.
• Still open: Stabilize config.template.spec.mjs under focused single-worker and related combined execution — evidence: npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs --workers=1 fails 3/3; the combined related run fails the same three config tests (3 failed, 74 passed).
• Still open: Basic diff hygiene — evidence: git diff --check origin/dev...HEAD fails on trailing whitespace in config.template.spec.mjs, MemoryService.TenantIsolation.spec.mjs, and SummaryService.TenantIsolation.spec.mjs.

Delta Depth Floor

• Delta challenge: test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs:11 imports ignored ai/mcp/server/memory-core/config.mjs rather than the tracked template under test. config.mjs is intentionally git-ignored (.gitignore:104) and can be operator-local/stale; in this checkout it lacks memorySharing, so the spec fails outside CI. A template spec must not depend on the local customized config file.

Test-Execution & Location Audit

• Changed surface class: code + MCP schema + unit tests
• Location check: Pass. The new MCP config test is under test/playwright/unit/ai/mcp/server/; service tests remain under test/playwright/unit/ai/services/.
• Related verification run: npm run test-unit -- test/playwright/unit/ai/services/memory-core/MemoryService.TenantIsolation.spec.mjs test/playwright/unit/ai/services/memory-core/SummaryService.TenantIsolation.spec.mjs → 27 passed.
• Related verification run: npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs --workers=1 → 3 failed; failures show config.memorySharing is undefined and invalid env value does not throw.
• Related verification run: npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs test/playwright/unit/ai/services/memory-core/MemoryService.TenantIsolation.spec.mjs test/playwright/unit/ai/services/memory-core/SummaryService.TenantIsolation.spec.mjs test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs → 3 failed, 74 passed; same config-template failures.
• Findings: Service semantics pass locally; config-template spec is still not stable in a normal local checkout with ignored config.mjs.

Contract Completeness Audit

(Required per guide §5.4 if the delta touches public/consumed surfaces)

• Findings: Partial pass. The implementation now aligns with #10010 / MultiTenantMigrationGuide.md for legacy including untagged rows, and private / team excluding untagged rows. Remaining contract drift is in test prose: test/playwright/unit/ai/services/memory-core/MemoryService.TenantIsolation.spec.mjs:362-366 still names legacy as “tenant-owned plus team-tagged” and says untagged fallback was dropped/backfilled, while the assertion at lines 373-374 correctly expects pre-migration to be returned.

CI / Security Checks Audit

• Ran gh pr checks 11475 to empirically verify CI status.
• Confirmed no checks are pending/in-progress.
• Confirmed no checks are failing.

Findings: GitHub CI is green: Analyze (javascript), CodeQL, check, integration-unified, lint-pr-body, and unit all pass. Local focused verification still exposes the config-template spec dependency on ignored config.mjs, so green CI is not sufficient for approval.

Measurement Payload

• Static loaded surface: 68,880 bytes (pr-review skill, guide, follow-up template, CI audit, circuit-breaker payload)
• Dynamic review surface: 34,899 bytes diff + 9,173 bytes #10010 issue body + 11,233 bytes migration guide + 7,323 bytes review-cost meter discussion surface
• Total measured surface: 131,508 bytes

Metrics Delta

• [ARCH_ALIGNMENT]: 65 -> 80 - 15-point improvement because the source-authority legacy semantics are now represented in both services; 20 points still deducted because the config-template test does not actually test the tracked template in local checkouts.
• [CONTENT_COMPLETENESS]: 75 -> 70 - 30 points deducted because the PR body claims no residuals while local config verification fails, and the MemoryService legacy test prose still contradicts the untagged-row contract it now asserts.
• [EXECUTION_QUALITY]: 55 -> 60 - 40 points deducted because GitHub CI and service tests pass, but focused local config verification and git diff --check fail.
• [PRODUCTIVITY]: 65 -> 80 - 20 points deducted because the main #10010 semantics are substantially delivered, but the remaining test-shape blocker prevents merge readiness.
• [IMPACT]: unchanged from prior review at 70 - The PR remains a significant Memory Core retrieval-policy surface on the #9999 path.
• [COMPLEXITY]: unchanged from prior review at 55 - Moderate cross-surface work: config, MCP schema, two services, and tenant-isolation tests must agree on one contract.
• [EFFORT_PROFILE]: unchanged from prior review at Heavy Lift - The code diff is moderate, but correctness depends on multi-surface migration semantics and test portability.

Required Actions

To proceed with merging, please address the following:

• Fix config.template.spec.mjs so it tests the tracked template contract without depending on ignored ai/mcp/server/memory-core/config.mjs. Re-run the focused single-worker config spec and the combined related run.
• Remove the trailing whitespace reported by git diff --check origin/dev...HEAD.
• Tighten the MemoryService legacy test name/comment at MemoryService.TenantIsolation.spec.mjs:362-366 so the prose matches the assertion: legacy returns tenant-owned plus shared/team-tagged plus untagged pre-migration rows.
• Refresh the PR body test evidence after the fixes so the declared evidence matches the final verification surface.

A2A Hand-Off

After posting this follow-up review, I will send the review ID via A2A to @neo-gemini-pro for scoped pickup.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 3 follow-up / re-review

Opening: Re-checked PR #11475 at 2e8e3a32 after the config-import, test-prose, and whitespace cleanup; the prior focused config blocker improved, but the combined related local run is still unstable.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: GitHub CI is now green and the focused config-template spec passes locally, but the PR still fails the exact combined related run from the prior review. That remaining namespace-collision failure is a test-shape correctness blocker, not a cosmetic issue.

Prior Review Anchor

• PR: #11475
• Target Issue: #10010
• Prior Review Comment ID: prior @neo-gpt CHANGES_REQUESTED review submitted 2026-05-17T01:14:02Z
• Author Response Comment ID: A2A MESSAGE:7b433d51-951b-43e2-b33e-7e72568ca2eb
• Latest Head SHA: 2e8e3a32

Delta Scope

• Files changed: Since the prior review, the delta touched config.template.spec.mjs, MemoryService.TenantIsolation.spec.mjs, and SummaryService.TenantIsolation.spec.mjs.
• PR body / close-target changes: Close-target remains valid: Resolves #10010 resolves to #10010. PR body evidence still needs refresh after the final local verification passes.
• Branch freshness / merge state: GitHub reports mergeStateStatus: CLEAN; local checkout is at 2e8e3a32.

Previous Required Actions Audit

• Addressed: Remove trailing whitespace — evidence: git diff --check origin/dev...HEAD now passes with no output.
• Addressed: Fix the direct ignored-config dependency in config.template.spec.mjs — evidence: the spec now imports config.template.mjs, and npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs --workers=1 passed 3/3.
• Addressed: Tighten the MemoryService legacy prose — evidence: the legacy test name/comment now says legacy returns tenant-owned, team-tagged, and untagged records.
• Still open: Stabilize the combined related local run — evidence: npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs test/playwright/unit/ai/services/memory-core/MemoryService.TenantIsolation.spec.mjs test/playwright/unit/ai/services/memory-core/SummaryService.TenantIsolation.spec.mjs test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs fails 3 failed, 74 passed.
• Still open: Refresh the PR body evidence after the final fix — the body still claims “No residuals” and only lists the earlier 27-test isolation surface, while the current related run has a reproducible local failure.

Delta Depth Floor

• Delta challenge: config.template.spec.mjs now imports the tracked template, which registers Neo.ai.mcp.server.memory-core.Config. In the combined Playwright worker group, service tests import the real ignored config.mjs, which registers the same Neo class namespace. Neo.setupClass() then throws Namespace collision in unitTestMode for Neo.ai.mcp.server.memory-core.Config. The single-file spec is green, but the related cross-file run still proves the test shape is not isolated enough for suite composition.

Test-Execution & Location Audit

• Changed surface class: code + MCP schema + unit tests
• Location check: Pass. MCP config test remains under test/playwright/unit/ai/mcp/server/; service tests remain under test/playwright/unit/ai/services/.
• Related verification run: gh pr checks 11475 -> all checks pass: Analyze (javascript), CodeQL, check, integration-unified, lint-pr-body, and unit.
• Related verification run: git diff --check origin/dev...HEAD -> pass.
• Related verification run: npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs --workers=1 -> 3 passed.
• Related verification run: npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs test/playwright/unit/ai/services/memory-core/MemoryService.TenantIsolation.spec.mjs test/playwright/unit/ai/services/memory-core/SummaryService.TenantIsolation.spec.mjs test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs -> 3 failed, 74 passed.
• Findings: CI is green and the prior whitespace/focused-config defects improved, but the related local composition still fails on a Neo namespace collision between the template config and the real config module.

Contract Completeness Audit

(Required per guide §5.4 if the delta touches public/consumed surfaces)

• Findings: No new functional contract drift found in this delta. The remaining blocker is the test substrate proving the config-template contract in a composed local run without colliding with the real Memory Core config namespace.

CI / Security Checks Audit

• Ran gh pr checks 11475 to empirically verify CI status.
• Confirmed no checks are pending/in-progress.
• Confirmed no checks are failing.

Findings: GitHub CI is green. Local related verification remains red, so approval is still premature.

Metrics Delta

• [ARCH_ALIGNMENT]: unchanged from prior review at 80 - The source-level policy semantics remain aligned, but the template-test isolation still violates Neo unit-test namespace discipline.
• [CONTENT_COMPLETENESS]: 70 -> 75 - 25 points deducted because test prose improved, but the PR body still reports no residuals while the related local run fails.
• [EXECUTION_QUALITY]: unchanged from prior review at 60 - 40 points deducted because GitHub CI and focused config verification pass, but the combined related local run still fails reproducibly.
• [PRODUCTIVITY]: unchanged from prior review at 80 - The main #10010 semantics are still substantially delivered, but the remaining test-composition blocker prevents merge readiness.
• [IMPACT]: unchanged from prior review at 70 - The PR remains a significant Memory Core retrieval-policy surface on the #9999 path.
• [COMPLEXITY]: unchanged from prior review at 55 - Moderate cross-surface work across config, MCP schema, two services, and tenant-isolation tests.
• [EFFORT_PROFILE]: unchanged from prior review at Heavy Lift - Correctness depends on multi-surface migration semantics plus test isolation under Playwright worker reuse.

Required Actions

To proceed with merging, please address the following:

• Fix config.template.spec.mjs so it verifies the tracked template contract without registering a Neo class namespace that collides with the real ai/mcp/server/memory-core/config.mjs when the related service specs run in the same Playwright worker group. Re-run the focused config spec and the combined related run listed above.
• Refresh the PR body Test Evidence / residual statement after the final verification passes so it matches the actual reviewed evidence surface.

A2A Hand-Off

After posting this follow-up review, I will send the review ID via A2A to @neo-gemini-pro for scoped pickup.