PR Review Summary

Status: Request Changes

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The split-out pure chunker is the right substrate shape for #13513, but the boundedness contract has one cheap functional gap: emitted chunk text can exceed the declared safe limit because separator characters are not included in the estimate.

Peer-Review Opening: The scope split is good: a pure, deterministic helper with no graph write or model invocation is the right first slice before the #12073 map/reduce integration. One boundedness bug needs fixing before merge.

Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13513 close-target body, #12073 parent contract ledger, #12065 epic context, GitHub changed-file list, current SemanticGraphExtractor.executeTriVectorExtraction(), and prior Memory Core recall about large-session chunking / guardrail skips.
• Expected Solution Shape: A correct leaf should add a pure helper that chunks turn-aligned session content deterministically, preserves small-session single-pass behavior, carries source traceability, and does not couple to graph writes or provider invocation. It should not hardcode integration policy, but every non-oversized emitted chunk must be bounded according to the same text the integration will send onward.
• Patch Verdict: Mostly matches the expected shape, but the emitted-text boundedness is false for boundary-tight inputs because the implementation sums per-turn estimates and then inserts \n separators later.

Context & Graph Linking

• Target Epic / Issue ID: Resolves #13513
• Related Graph Nodes: #12073, #12065, SemanticGraphExtractor, sessionChunker

Depth Floor

Challenge OR documented search (per guide §7.1):

Challenge: The helper reports chunk estimates from summed turn estimates, but emits chunk text by joining turns with \n. That separator text is part of the real chunk payload and can push a chunk over safeProcessingLimitTokens even when estimatedTokens reports it under the limit.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the intended leaf split, except the "bounded" claim needs the fix below.
• Anchor & Echo summaries: clear and source-local; no broad architecture inflation.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: #13513/#12073 establish the pattern and residual split.

Findings: Request Changes on emitted-text boundedness.

Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: GitHub CI broad unit / integration-unified jobs were still pending during review; local focused spec passed.
• [RETROSPECTIVE]: Pure chunking primitive is the right way to de-risk #12073 before separating extraction from graph commit.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI/MCP tool description surface and no skill/convention surface changed.

Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

• Close-targets identified: #13513.
• #13513 confirmed not epic-labeled.

Findings: Pass.

Contract Completeness Audit

• Originating ticket / parent contains a Contract Ledger matrix: #12073 has the parent Contract Ledger.
• Implemented PR diff matches the ledger except for the boundedness edge called out in Required Actions.

Findings: Contract drift flagged: non-oversized chunks are not always bounded against the emitted chunk text.

Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence is L1, which is the right class for this pure helper leaf.
• Evidence-class collapse check: review language does not promote this to live REM integration proof.

Findings: Pass for evidence class; the required action needs an additional boundary-tight unit fixture.

Test-Execution & Location Audit

• Branch checked out locally in /Users/Shared/codex/neomjs/neo/tmp/review-13514.
• Exact head verified: ee9be2ca22874a1156369e16b05da971e98b3b10.
• Canonical Location: new test file is under test/playwright/unit/ai/services/graph/.
• Ran the specific test file.

Findings: Existing focused tests pass, but they miss the newline-separator boundary case.

Local command:

UNIT_TEST_MODE=true npx playwright test -c test/playwright/playwright.config.unit.mjs test/playwright/unit/ai/services/graph/sessionChunker.spec.mjs

Result: 11 passed.

Falsifier:

node --input-type=module -e "import {chunkSession, estimateTokens} from './ai/services/graph/sessionChunker.mjs'; const r = chunkSession(['aaaa','aaaa'], {sessionId:'s', safeProcessingLimitTokens:2}); console.log(JSON.stringify({chunked:r.chunked, reported:r.chunks[0].estimatedTokens, actual:estimateTokens(r.chunks[0].text), textLength:r.chunks[0].text.length}, null, 2));"

Output:

{
  "chunked": false,
  "reported": 2,
  "actual": 3,
  "textLength": 9
}

Required Actions

To proceed with merging, please address the following:

• Make chunk boundedness use the emitted chunk text, not only summed per-turn estimates. A safe shape is to estimate turnTexts.join('\n') for the small-session fast path and estimate each candidate emitted chunk text when deciding whether appending the next turn would exceed safeProcessingLimitTokens. Add a boundary-tight unit test proving every non-oversized emitted chunk satisfies estimate(chunk.text) <= limit, including the current ['aaaa', 'aaaa'] / limit 2 falsifier.

Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - 12 points deducted for the boundedness drift; the pure-helper placement and no-I/O shape otherwise align with the REM substrate split.
• [CONTENT_COMPLETENESS]: 90 - 10 points deducted because the docs say bounded chunks, but the separator edge is not represented in the contract/test wording.
• [EXECUTION_QUALITY]: 72 - 28 points deducted for one functional edge-case defect in the core guarantee despite focused tests passing.
• [PRODUCTIVITY]: 84 - 16 points deducted because most #13513 ACs land, but the boundedness AC needs correction before the leaf can close.
• [IMPACT]: 72 - Meaningful REM reliability foundation, but this PR is a primitive leaf rather than the full #12073 integration.
• [COMPLEXITY]: 36 - Low-to-moderate: one pure helper and one focused spec, with complexity mainly from exact token-boundary semantics.
• [EFFORT_PROFILE]: Quick Win - Small, high-leverage primitive; the requested fix is local and testable.

Once the emitted-text estimate and boundary fixture land, I expect this to be approve-ready.