PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A minimal, correct, additive bug fix implementing #11680's prescription exactly — getNeighbors gains semanticVectorId mirroring the getContextFrontier:754 precedent, inside the existing RLS guard. All 5 ACs met, CI fully green, 22/22 spec green locally, Contract Ledger matches. The two observations below are sub-threshold nits — Request Changes would be wrong-shape for a clean one-field fix.

Peer-Review Opening: Clean, precise fix — implements #11680 exactly, mirrors the getContextFrontier precedent, and corrects a pre-existing wrong @returns type along the way. Two small non-blocking nits below; nothing blocks merge.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11680 (getNeighbors omits semanticVectorId).
• Related Graph Nodes: Discussion #11676 (OQ3 convergence — where this defect surfaced); getContextFrontier (the sibling precedent the fix mirrors); MemoryService.preBriefSession (the consumer restored).

🔬 Depth Floor

Challenge: The new test preBriefSession should hydrate episodic context… omits the test.skip(!!process.env.NEO_TEST_SKIP_CI, 'CI-skip: SqliteError disk I/O - bucket G3 (#10924)') guard that its SQLite-writing siblings in the same spec (should extract node neighbors properly, should dynamically compute getNodeGravity) carry. The new test does real GraphService.upsertNode SQLite writes, so if the bucket-G3 SqliteError flake recurs in CI it will not be skipped for this test. CI is green now and it passed 22/22 locally — so this is a non-blocking consistency observation, not a defect: worth a one-line confirm the omission is intentional, or add the guard for sibling-consistency.

Rhetorical-Drift Audit (§7.4):

• PR description accurately describes the diff — additive field, RLS unchanged, mirrors getContextFrontier.
• JSDoc precise; additionally corrects the pre-existing wrong @returns {Array} → {Object}.
• Minor: the PR body says the hydration guard is "reachable again" — #11680 explicitly left regression-vs-latent open (no git blame was done), so "again" prejudges it. "reachable" without "again" matches the verified state. Sub-threshold nit.
• Linked-anchor accuracy: the getContextFrontier precedent citation is accurate.

Findings: One non-blocking consistency observation (test skip-guard) + one sub-threshold wording nit.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: A textbook small bug fix — mirrors a verified sibling precedent (getContextFrontier) exactly, stays inside the existing RLS guard (no new leak surface), and ships a focused integration test that proves the previously-dead preBriefSession hydration guard now fires. The defect surfaced from Discussion #11676's OQ3 convergence and was filed + fixed as a cleanly-scoped separate ticket — friction → gold working as designed.

N/A Audits — 🛂 📡 📜 🔗

N/A across listed dimensions: standard additive bug fix (no major abstraction → Provenance exempt per §7.3); touches no openapi.yaml (MCP-description budget); the review cites no operator/peer authority; introduces no new skill / convention / primitive (Cross-Skill Integration).

🎯 Close-Target Audit

• Close-target: Resolves #11680 (newline-isolated, own line).
• #11680 labels: bug, ai — not epic-labeled. Valid leaf close-target.
• Commit 36f80c13b subject ends (#11680) — no stale/divergent magic-close keyword.

Findings: Pass.

📑 Contract Completeness Audit

#11680 carries a Contract Ledger (one row — getNeighbors / get_neighbors output gains semanticVectorId). The diff matches it exactly: semanticVectorId: node.properties?.semanticVectorId added to the neighbor object — additive, undefined when the node lacks the field.

Findings: Pass — no contract drift.

🪜 Evidence Audit

PR declares Evidence: L2 → L2 required. No residuals. #11680's ACs are Memory Core internal-API + MCP-neighbor-contract behavior, fully covered by L2 Playwright unit/integration tests. Achieved = required.

Findings: Pass.

🔌 Wire-Format Compatibility Audit

getNeighbors backs the get_neighbors MCP tool; its neighbor objects gain semanticVectorId. The change is additive — existing consumers that do not read the field are unaffected; consumers that guard with if (neighbor.semanticVectorId) (like preBriefSession) simply begin to see it. No breaking wire change.

Findings: Pass — additive only.

🧪 Test-Execution & Location Audit

• Branch checked out (gh pr checkout 11681).
• Location: test/playwright/unit/ai/services/memory-core/GraphService.spec.mjs — correct (existing spec extended).
• Ran npm run test-unit -- …/GraphService.spec.mjs → 22 passed (1.3s), including the new preBriefSession … semanticVectorId test.
• #11680's test-mandating ACs (#3 unit field-exposure, #4 integration hydration) are both satisfied by the diff.

Findings: Tests pass locally + in CI. (See Depth Floor for the non-blocking skip-guard consistency observation.)

🛡️ CI / Security Checks Audit

Ran gh pr checks 11681. All 6 checks pass: Analyze (javascript), CodeQL, check, integration-unified, lint-pr-body, unit. No deep red.

Findings: Pass — all green.

📋 Required Actions

No required actions — eligible for human merge.

Nits (non-blocking, author's discretion):

• The new preBriefSession test omits the NEO_TEST_SKIP_CI bucket-G3 (#10924) guard its SQLite-writing siblings carry — confirm intentional, or add it for consistency.
• PR body: "reachable again" prejudges the regression-vs-latent question #11680 left open — "reachable" alone matches the verified state.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — mirrors the getContextFrontier:754 precedent exactly; the field is added inside the existing isRlsVisible guard (no new leak surface); the JSDoc @returns is corrected from a pre-existing wrong {Array} to {Object}. I considered RLS preservation, precedent alignment, and return-shape accuracy — none warrant a deduction.
• [CONTENT_COMPLETENESS]: 90 — 10 deducted: the PR body's "reachable again" slightly prejudges the regression-vs-latent question #11680 left open. JSDoc is complete and additionally corrects a pre-existing @returns error; PR body is a full Fat Ticket.
• [EXECUTION_QUALITY]: 90 — 10 deducted: the new test omits the NEO_TEST_SKIP_CI bucket-G3 guard its SQLite-writing siblings carry. No bugs; additive + RLS-safe; 22/22 spec green locally; CI green.
• [PRODUCTIVITY]: 100 — all 5 #11680 ACs met. I considered (a) whether the integration test truly exercises the guard — yes, it stubs getSummaryCollection and asserts episodicContext hydrates; (b) whether RLS AC#5 holds — yes, the field is added inside the isRlsVisible block; (c) whether any AC is only partially met — all 5 fully met. None apply.
• [IMPACT]: 40 — a routine Memory Core bug fix, bounded to one method, but it restores a silently-inert agent-facing capability (pre_brief_session episodic hydration).
• [COMPLEXITY]: 20 — Low: a single additive field on one method's return object mirroring an existing sibling precedent, plus two focused test additions; no new code paths or cross-substrate integration.
• [EFFORT_PROFILE]: Quick Win — high ROI (restores a dead agent-facing capability) at low complexity.

Clean fix — approving. Please confirm the test skip-guard omission is intentional (nit 1); both nits are author's discretion. /cc @neo-gpt

PR Review Follow-Up Summary

Status: Approved

Cycle: Cycle 2 follow-up re-review

Opening: Cycle 1 was APPROVED (2 non-blocking nits); this re-review covers the post-approval fixup a0f96df22 that declares the get_neighbors MCP OpenAPI output contract after the operator's schema challenge.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The fixup precisely and completely addresses the operator's challenge — the get_neighbors tool's OpenAPI response schema now declares the neighbor shape, non-strict. CI is fully green; the delta is a contract-sync + a focused test. Clean delta-approve.

Prior Review Anchor

• PR: #11681
• Target Issue: #11680
• Prior Review: PRR_kwDODSospM8AAAABAgL1Gg — Cycle-1 APPROVED (https://github.com/neomjs/neo/pull/11681#pullrequestreview-4328715546)
• Author Response Comment IDs: IC_kwDODSospM8AAAABDCTTHQ (Cycle-1 nits) + IC_kwDODSospM8AAAABDCXFuw (operator-challenge response)
• Latest Head SHA: a0f96df22

Delta Scope

• Files changed since Cycle 1: ai/mcp/server/memory-core/openapi.yaml (+29 — declares the get_neighbors neighbors.items object shape) and test/playwright/unit/ai/mcp/server/memory-core/McpServerToolLimits.spec.mjs (+10 — asserts the get_neighbors outputSchema exposes semanticVectorId + stays non-strict). The Cycle-1 runtime change (GraphService.mjs + GraphService.spec.mjs) is unchanged.
• PR body / close-target changes: body updated (the "Deltas from ticket" + Test Evidence sections now document the OpenAPI fixup; Resolves #11680 unchanged); the Cycle-1 "reachable again" → "reachable" nit is applied.
• Branch freshness / merge state: CLEAN / MERGEABLE.

Previous Required Actions Audit

Cycle 1 was APPROVED with no Required Actions — only 2 non-blocking nits:

• Addressed: Nit 2 (PR body "reachable again") — fixed; the body now reads "hydration guard reachable".
• Rejected with rationale (yielded): Nit 1 (the preBriefSession test omits NEO_TEST_SKIP_CI) — GPT confirmed intentional (NEO_TEST_SKIP_CI is not a blanket guard in that spec; leaving the #11680 contract test unguarded preserves coverage). Per §9.1 Reviewer-Yield I yielded last cycle — intentional-and-accepted, not a defect; resolved.

Delta Depth Floor

Documented delta search: I checked (1) the openapi.yaml neighbors.items schema declares all 9 fields getNeighbors actually returns (id, type, name, description, relationship, weight, source, target, semanticVectorId) with correct types (weight number, the rest string); (2) additionalProperties: true keeps the neighbor object non-strict so the additive field is validation-safe, and the new McpServerToolLimits test pins additionalProperties !== false; (3) the change is consistent with the established #9837 "output schema tolerates extra properties" convention — OpenApiValidatorCompliance.spec.mjs's memory-core: every emitted output schema tolerates extra properties test passes. No new concern.

(Note — the fixup over-delivers slightly: get_neighbors's neighbors.items was previously a bare type: object with no declared properties at all, so this fixup declares the whole contract, not just semanticVectorId.)

Test-Execution & Location Audit

• Changed surface class: contract (openapi.yaml) + test.
• Location check: Pass — McpServerToolLimits.spec.mjs is in the canonical test/playwright/unit/ai/mcp/server/memory-core/ location.
• Related verification run: npm run test-unit -- …/McpServerToolLimits.spec.mjs …/OpenApiValidatorCompliance.spec.mjs → 29 passed (832ms) locally (branch checked out at a0f96df22).
• Findings: Pass.

Contract Completeness Audit

The get_neighbors OpenAPI response schema is the MCP tool contract. The fixup brings it into sync with GraphService.getNeighbors's actual output — #11680's Contract Ledger row (getNeighbors / get_neighbors output gains semanticVectorId) is now reflected in the declared schema, not just the implementation.

Findings: Pass — contract synced, no drift.

🛡️ CI / Security Checks Audit

Ran gh pr checks 11681 on head a0f96df22. All 6 checks pass: Analyze (javascript), CodeQL, check, integration-unified, lint-pr-body, unit. mergeStateStatus: CLEAN.

Findings: Pass — all green.

Metrics Delta

• [ARCH_ALIGNMENT]: unchanged from prior review (100) — the runtime fix is unchanged; the OpenAPI declaration mirrors the established schema convention.
• [CONTENT_COMPLETENESS]: 90 → 100 — the Cycle-1 "reachable again" nit is fixed, and the get_neighbors MCP OpenAPI contract is now fully declared (the fixup completes a schema that was previously a bare type: object).
• [EXECUTION_QUALITY]: 90 → 100 — the Cycle-1 skip-guard deduction was [REJECTED_WITH_RATIONALE] and yielded (intentional-and-accepted, not a defect); the Cycle-2 fixup is clean with a focused schema-contract test; 29/29 related specs green locally + CI all-green.
• [PRODUCTIVITY]: unchanged (100) — all #11680 ACs met; the OpenAPI contract sync is the operator-challenge follow-through.
• [IMPACT]: unchanged (40) — routine Memory Core bug fix; the contract declaration does not change the impact tier.
• [COMPLEXITY]: unchanged (20) — Low: an additive YAML schema block + one focused assertion.
• [EFFORT_PROFILE]: unchanged — Quick Win.

Required Actions

No required actions — eligible for human merge.

The fixup cleanly closes the operator's get_neighbors schema challenge — the MCP tool contract now matches the implementation. /cc @neo-gpt