PR Review Summary

Status: Request Changes

Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The PR is the right shape for #13018: a new realm-pure parser, a thin value_ component, focused unit coverage, and a real-pipeline coherence observe-run. I am requesting changes because the component reset path bypasses the parser reset contract, so one close-target AC is not actually satisfied at the consumed component surface.

Peer-Review Opening: I took this as a self-selected review because the PR was green/open with no requested reviewer. The implementation is strong overall; the required action is narrow but contract-level.

Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13018 live issue body/labels/Contract Ledger, PR #13034 live metadata/CI/files, legacy Markdown.mjs premise via KB, new parser/component/tests/example source, and local unit/e2e execution.
• Expected Solution Shape: The parser can memoize settled streaming blocks, but any non-append/reset path exposed through the component must clear the parser ledger so subsequent renders get fresh block ids. This is the ticket's explicit guard against cross-batch two-nodes-one-id defects.
• Patch Verdict: The parser itself matches that reset contract. The component does not, because value = null writes [] directly instead of routing the empty value through the parser/reset path.

Context & Graph Linking

• Target Epic / Issue ID: Resolves #13018
• Related Graph Nodes: #13012, #8920, #13016, DeltaCapture, DeltaCoherenceRegistry, Neo.component.markdown.Component

Depth Floor

Challenge: The reset AC must be proven at the component surface, not only at the parser surface. The parser-level test shows fresh ids after reset, but the component clear path bypasses parser.update(''), so re-rendering the same source after value = null reuses every old id.

Probe result against PR head d75fea17b562f2bcc715d116ec8ea21ebfdb61c9:

{
  "clearedLength": 0,
  "overlap": [
    "markdown-reset-probe__md-0",
    "markdown-reset-probe__md-0-c0",
    "markdown-reset-probe__md-1",
    "markdown-reset-probe__md-1-c0"
  ]
}

The parser contrast is clean: calling parser.update('') before re-rendering the same markdown produces ids __md-2/__md-3 and zero overlap. The gap is in the component binding, not the parser ledger.

Rhetorical-Drift Audit: Findings: drift flagged. The PR body/ticket say non-append reset gets fresh ids and coherence-registry-clean behavior. The component-level reset path at src/component/markdown/Component.mjs:67-71 does not substantiate that claim yet.

Graph Ingestion Notes

• [KB_GAP]: KB currently knows the legacy HTML-string Markdown.mjs surface, not this new streaming VDOM primitive yet. The PR's new docs/tests are the right substrate to close that gap post-merge.
• [TOOLING_GAP]: The e2e command needs unsandboxed execution here because the sandbox blocks the dev server bind and uv_uptime; under escalation the e2e passed.
• [RETROSPECTIVE]: For streaming-id primitives, parser-only reset coverage is insufficient. The consumed component must also prove clear/reset/replay behavior, because that is the user-facing transcript lifecycle.

Close-Target Audit

• Close-targets identified: #13018
• #13018 confirmed not epic-labeled. Live labels: enhancement, ai, architecture, core.

Findings: Pass.

Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix.
• Implemented PR diff matches the Contract Ledger exactly.

Findings: Contract drift flagged. #13018 AC: "Non-append value change falls back to full reset correctly (fresh block sequence; no id reuse across the reset — coherence-registry-clean)." afterSetValue() currently uses value ? me.parser.update(value) : [], so a clear empties vdom.cn but leaves MarkdownParser.#source and #blocks untouched. Reassigning the same source after clear returns the old memoized vdom objects/ids.

Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence covers the close-target required evidence.
• Two-ceiling distinction is satisfied: L2 unit + L3 whitebox e2e for live coherence.
• Evidence-class collapse check passes; the review does not promote unit-only reset coverage to live component reset coverage.

Findings: Evidence gap on reset/replay. Existing tests cover parser non-append reset and component clear-to-empty, but not component clear-then-replay-same-source with fresh ids.

Cross-Skill Integration Audit

• New framework primitive is documented through JSDoc and a runnable example surface.
• No AGENTS.md, MCP tool, or workflow-skill trigger is introduced in this slice.
• Follow-up transcript integration and vdom highlighter work are explicitly out of scope.

Findings: All checks pass after the component reset fixture/fix lands.

Test-Execution & Location Audit

• Branch checked out locally at PR head d75fea17b562f2bcc715d116ec8ea21ebfdb61c9.
• Canonical Location: unit tests are under test/playwright/unit/component/markdown/; the whitebox e2e is under test/playwright/e2e/.
• Ran the targeted unit suite:

npm run test-unit -- test/playwright/unit/component/markdown/

Result: 28 passed.

• Ran the committed whitebox e2e:

npx playwright test test/playwright/e2e/MarkdownStreamingCoherenceNL.spec.mjs -c test/playwright/playwright.config.e2e.mjs

Sandboxed result: environment failure before test execution (listen EPERM 0.0.0.0:8080, uv_uptime EPERM). Escalated rerun result: 1 passed in 10.6s; observe-run logged 43 batches, 50 live ids, zero findings.

Findings: Tests pass, but one component reset/replay fixture is missing.

Required Actions

To proceed with merging, please address the following:

• Make the component clear/reset path reset the parser ledger instead of only assigning vdom.cn = [], and add a component-level fixture for clear-then-replay-same-source proving the second render uses fresh block/child ids with zero overlap against the pre-clear ids. Routing null/empty through parser.update(value ?? '') or explicitly calling parser.reset() before clearing are both plausible shapes; the important contract is fresh ids after reset at Neo.component.markdown.Component, not only in MarkdownParser.

Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - Correct primitive split and migration boundary; reset surface needs contract tightening.
• [CONTENT_COMPLETENESS]: 87 - Broad grammar/security/delta coverage; missing component reset replay fixture.
• [EXECUTION_QUALITY]: 84 - Unit/e2e evidence is strong, with one lifecycle gap at the consumed component boundary.
• [PRODUCTIVITY]: 90 - Excellent slice discipline and follow-up scoping.
• [IMPACT]: 92 - This is a high-value transcript primitive once the reset AC is closed.
• [COMPLEXITY]: 64 - Parser/delta/coherence behavior is non-trivial; the required fix itself is small.
• [EFFORT_PROFILE]: Heavy Lift - The PR carries a new parser, component, demo, unit suite, and whitebox e2e.

This should be a small re-review: the architecture and proof stack are already in good shape, but the reset contract has to hold at the component API surface before the PR closes #13018.