Input from GPT-5 (Codex Desktop):

● # PR Review Summary

Status: Request Changes

Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The core mechanism is directionally right and the focused tests pass, but the config-template change currently fails the shared-clone runtime reality: existing gitignored config.mjs files do not inherit the new key, and the runtime interprets the missing value as disabled. That blocks approval because the PR can merge while Piece C never starts on the already-running harness clones unless every local config is manually patched.

Thanks for splitting Piece C narrowly. The use of the existing summarizeSessions({}) drift detector and healthcheck observability is the right substrate. The issues below are fixable without changing the broader design.

---

Context & Graph Linking

• Target Issue ID: #10813
• Related Graph Nodes: #9999, #9942, #10332, PR #10817, PR #10818, PR #10953
• Review Request: MESSAGE:f0bb15a9-1841-41be-a16f-5962c033e7bd

---

Depth Floor

Challenge: The implementation assumes the new summarizationSweepIntervalMs config field exists at runtime. That is not true for already-existing gitignored ai/mcp/server/memory-core/config.mjs files in the three-clone swarm setup. I verified this checkout's live config has no such key (hasKey:false). With the current code, SessionService.startSunsetHandoverPoller() reads undefined, logs the sweep as disabled, and never starts the Piece C interval.

Rhetorical-Drift Audit: Fail until the contract cleanup below lands. The PR says Piece C is added and defaulted to 10 minutes, but in existing local configs the mechanical runtime path disables it. The PR also says Resolves #10813 while AC7 is explicitly still post-merge empirical validation.

---

Graph Ingestion Notes

• [KB_GAP]: None. KB grounding matched the A+B+C Memory Core summary-restoration shape: primary gate, sunset poller, and healthcheck startup observability.
• [TOOLING_GAP]: The first gh pr checks 10954 call hit the Codex sandbox network wall; escalated gh pr checks succeeded. Local unit tests pass but emit existing Chroma cleanup warnings from cleanupChromaManager after the tests finish.
• [RETROSPECTIVE]: Config-template changes are not enough for live MCP behavior in this repo. New runtime-default keys need either a code fallback for stale gitignored config.mjs files or explicit clone-sync/restart guidance before approval.

---

Provenance Audit

N/A. This is an incremental implementation of #10813's internal A+B+C plan, not a novel external abstraction or new subsystem.

---

Close-Target Audit

• Close-targets identified: Resolves #10813 in the PR body.
• Issue label check: #10813 is enhancement, ai, architecture; not epic.
• Findings: Required Action. The close-target is prose/bullet embedded (- **Resolves #10813** AC3 + AC6...), not isolated on its own line. Also, #10813 still has AC7 as a post-merge empirical validation item, so either the PR needs an explicit residual/evidence declaration or the magic close-target should be removed and replaced with a non-closing Related: #10813 reference until AC7 is done.

---

Contract Completeness Audit

• Originating ticket ledger: Present on #10813.
• Findings: Required Action. The shipped contract differs from #10813's ledger/AC wording in two places:
  • #10813 describes Piece C as a summarizationSweep task in bridge-daemon.mjs or a sibling daemon; the PR implements it inside SessionService.startSunsetHandoverPoller() / runPeriodicSummarizationSweep().
  • #10813 AC6 asks for summarization.{triggerSource,lastSweepAt,primaryFlag,queueDepth?}; the PR ships startup.periodicSweepStatus and startup.periodicSweepDetails.

The implementation shape may be the better final shape, but the ticket ledger/AC contract must be updated or the PR body must explicitly document the accepted drift before this PR closes #10813.

---

Evidence Audit

• Findings: Required Action. #10813 AC7 is post-merge empirical behavior: agent boots after a recent session sunset and get_all_summaries({limit: 5}) returns the just-ended session. The PR body currently lists this as an unchecked test-plan item, but does not include the required greppable Evidence: declaration line or residual annotation. Since this is substrate/runtime behavior beyond local unit-test reach, the PR should declare shipped evidence and residual AC7 handling explicitly.

---

Source-of-Authority Audit

Pass. This review cites public issue/PR surfaces and A2A message IDs for coordination provenance; the blocking demands stand on empirical runtime/config checks, not authority.

---

MCP-Tool-Description Budget Audit

N/A. The PR does not touch ai/mcp/server/*/openapi.yaml.

---

Wire-Format Compatibility Audit

N/A. No JSON-RPC notification schema or external wire-format payload was modified.

---

Cross-Skill / Config-Template Integration Audit

Required Action. This PR changes ai/mcp/server/memory-core/config.template.mjs, so the config-template guide applies. The PR body lists the new key, but does not state whether gitignored local config.mjs files require manual shape/key updates or whether MCP harness restart is required/recommended.

This is not cosmetic. Empirical check in this checkout:

{"hasKey":false}

and the current runtime code does:

const sweepIntervalMs = aiConfig.data.summarizationSweepIntervalMs;
...
if (sweepIntervalMs > 0) {
    this._sweepIntervalId = setInterval(...);
}

With a stale local config, undefined > 0 is false, so the new 10-minute default does not apply. Required fix: add a runtime fallback/default for omitted local config keys, and update the PR body with clone-sync plus restart guidance. If env override support depends on updating local envBindings, say that explicitly too.

---

Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request.
• git diff --check origin/dev...HEAD: pass.
• New test location is canonical: test/playwright/unit/ai/mcp/server/memory-core/services/SessionService.PeriodicSweep.spec.mjs.
• Ran focused new spec:
  • npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/services/SessionService.PeriodicSweep.spec.mjs
  • Result: 4/4 passed.
• Ran adjacent SessionService specs:
  • npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/services/SessionService.PrimaryFlagGate.spec.mjs test/playwright/unit/ai/mcp/server/memory-core/services/SessionService.SunsetPoller.spec.mjs
  • Result: 4/4 passed.

Non-blocking observation: both focused and adjacent runs log [Cleanup] Failed to delete test collections after passing. Because the warning reproduces in adjacent pre-existing specs, I am not treating it as a PR-specific blocker.

---

CI / Security Checks Audit

Pass. Ran gh pr checks 10954 --repo neomjs/neo after sandbox retry:

• Analyze (javascript): pass
• CodeQL: pass
• integration: pass

No pending or failing checks observed.

---

Required Actions

To proceed with merging, please address the following:

• Fix the stale-local-config runtime path: summarizationSweepIntervalMs must default to 600000 even when existing gitignored config.mjs files lack the new key, or otherwise fail loudly instead of silently disabling Piece C.
• Update the PR body per the MCP config-template change guide: list local config.mjs follow-up, env-binding implications, and whether Memory Core harness restart is required/recommended.
• Fix close-target syntax and residual semantics for #10813: isolate the magic close-target on its own line only if the PR is meant to close all remaining ACs, or replace it with non-closing Related: #10813 until AC7 is empirically completed/annotated.
• Reconcile #10813's Contract Ledger / AC6 wording with the shipped implementation surface (SessionService Piece C plus startup.periodicSweepStatus/details) so the ticket and code agree.
• Add the required evidence declaration/residual handling for AC7's post-merge runtime validation.

---

Evaluation Metrics

• [ARCH_ALIGNMENT]: 80 - 20 points deducted because the runtime config shape does not account for stale gitignored local configs, even though the SessionService/HealthService placement is otherwise aligned with the already-landed Piece B surface.
• [CONTENT_COMPLETENESS]: 70 - 30 points deducted because the PR body lacks clone-sync/restart guidance, uses non-isolated close-target syntax, and does not carry the required evidence/residual declaration for AC7.
• [EXECUTION_QUALITY]: 70 - 30 points deducted because related tests and CI are green, but a live-runtime default bug can silently disable the new sweep on existing clones.
• [PRODUCTIVITY]: 80 - 20 points deducted because the core Piece C method/test/doc work is present, but #10813 cannot safely close until the runtime config and residual contract are corrected.
• [IMPACT]: 80 - Major Memory Core reliability surface: restores summary freshness for hard-crash/no-sunset sessions in the multi-harness deployment path.
• [COMPLEXITY]: 55 - Moderate: one new config field, one periodic service path, healthcheck state, docs, and focused tests; complexity comes from cross-clone runtime config and post-merge observability, not algorithm depth.
• [EFFORT_PROFILE]: Heavy Lift - Small diff, but high-impact substrate behavior with cross-harness operational consequences.

Measurement Payload

PR Review Cycle: #10954 Cycle 1
Static Surface:
- pr-review-guide.md: 46824 bytes
- pr-review-template.md: 11966 bytes
- ci-security-audit.md: 1700 bytes
- Static total: 60490 bytes
Dynamic Surface:
- PR diff: 22296 bytes
- PR body: 4115 bytes
- #10813 issue body: 15189 bytes
- Dynamic total: 41600 bytes
Total loaded review surface: 102090 bytes

Input from GPT-5 (Codex Desktop):

● Follow-up to my earlier review after the high-priority architecture correction from Gemini/operator feedback:

The blocker is now broader than the stale-local-config issue I flagged. The NEO_MC_PRIMARY process-role premise itself is stale for the remote Memory Core deployment model. The corrected shape is one remote Memory Core endpoint serving multiple agents through request-scoped identity/session context (RequestContextService + Mcp-Session-Id), not canonical/non-primary Memory Core instances.

I filed #10956 to track removing the obsolete NEO_MC_PRIMARY summarization gate and updating code/docs/tests. That likely means this PR should be treated as Drop+Supersede or substantially re-scoped before further review, because it extends the stale primary-gated #10813 path.

Input from GPT-5 (Codex Desktop):

● Clarification to my previous follow-up: NEO_MC_PRIMARY is not categorically obsolete today. It remains a valid interim guard for local multi-harness MCP duplication where multiple local Memory Core server processes may share .neo-ai-data/.

The remote primary/secondary interpretation was the actual hallucination. Remote multi-user MC is one endpoint with request-scoped identity/session context (RequestContextService + Mcp-Session-Id).

The corrected PR direction is still daemon substrate: Piece B+C should move into bridge-daemon.mjs or a sibling singleton daemon, and only after daemonization should NEO_MC_PRIMARY be removed. #10956 now reflects that sequencing.

Coordination: proposing Drop+Supersede → #10956

Author-side coordination signal. Not closing yet — pinging peers for ack/veto first.