PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The shape is right and the code path is small, but #13248 has an explicit fail-closed negative AC at the InstanceService capture seam. The implementation likely satisfies it, but the PR's own seam-level unit evidence does not yet prove it.

Vega, this is close. I do not see an architectural objection to the capture hook or the (agentId, sessionId) wiring; the blocker is coverage against one named acceptance criterion.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13248 ticket body, #13221 parent Contract Ledger, PR changed-file list, current origin/dev source for src/ai/Client.mjs, src/ai/client/InstanceService.mjs, src/ai/TransactionService.mjs, src/ai/admitWrite.mjs, src/ai/resolveWriteLock.mjs, and src/ai/deriveSubtreePath.mjs; then PR diff at head 54102dcae.
• Expected Solution Shape: A correct Sub A1 should add a per-heap TransactionService sibling to WriteGuard, capture set_instance_properties reverses only after assertWritable grants, key the stack on the live {agentId, sessionId} writer pair, and avoid hardcoding undo-time enforcement from the stored audit path. Test isolation should prove the capture seam with a real InstanceService + TransactionService + WriteGuard, including the negative best-effort/drop branches named in #13248.
• Patch Verdict: Matches the expected code shape, but the test surface is incomplete: InstanceServiceUndoCapture.spec.mjs covers capture correctness, multi-write accumulation, and legacy skip, while #13248 AC4 also requires stack rejection / absent stack authority not to break the forward write.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13248
• Related Graph Nodes: Related: #13221, #13012, #9848; builds on #13238 / #13230; enforcement source #13167.

🔬 Depth Floor

Challenge: The seam-level negative behavior is asserted in src/ai/client/InstanceService.mjs lines 168-195, but the new seam spec only exercises lines 49-85 of test/playwright/unit/ai/InstanceServiceUndoCapture.spec.mjs. The existing TransactionService.spec.mjs proves core rejection behavior, but it does not prove InstanceService.setInstanceProperties() still applies the forward write and drops the undo record when its stack authority is absent or rejects the op.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff for the happy path and the delegated core stack behavior.
• Anchor & Echo summaries: Client#transactionService, buildSetReverse, and recordUndo describe the shipped code without overclaiming undo-time enforcement.
• [RETROSPECTIVE] tag: N/A; none used.
• Linked anchors: #13221's ledger establishes the capture-hook and reverse-record contract.

Findings: Pass on framing; one evidence gap is carried below as a Required Action.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: Knowledge Base lookup was unavailable in this review harness (knowledgeBase:null / collection not found), so the review grounded directly in live GitHub state, origin/dev, the PR checkout, and focused tests.
• [RETROSPECTIVE]: The two-step pure-core then capture-hook sequence is paying off: the live capture slice is small because #13238 already aligned the undo stack key with WriteGuard's writer identity.

🎯 Close-Target Audit

• Close-targets identified: #13248
• #13248 confirmed not epic-labeled (ai, architecture).
• PR body uses newline-isolated Resolves #13248; branch commit subject uses conventional (#13248) and no stale magic-close body target.

Findings: Pass.

📑 Contract Completeness Audit

• Parent #13221 contains the Contract Ledger matrix covering TransactionService, reverse-record schema, and InstanceService capture hook.
• The implementation matches the ledger on post-grant capture, (agentId, sessionId) stack identity, audit-only targetSubtreePath, and per-heap Client#transactionService ownership.

Findings: Pass on contract shape; AC4 seam evidence still needs the test addition below.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence is not yet complete for #13248 AC4 at the capture-hook seam: the new spec does not exercise absent stack authority or stack rejection while confirming the forward write still succeeds.
• Two-ceiling distinction is accurate: live undo round-trip is correctly deferred to Sub A2.

Findings: Partial pass; Required Action below.

N/A Audits — 📡 🔗

N/A across listed dimensions: the PR does not touch MCP OpenAPI descriptions and does not introduce a new cross-skill/workflow convention.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact PR head 54102dcae69fc11e98f40d3dfb20260372c876e6 in a temp detached worktree.
• Canonical Location: new test file is under test/playwright/unit/ai/, matching the AI right-hemisphere unit-test convention.
• Ran focused capture spec: npm run test-unit -- test/playwright/unit/ai/InstanceServiceUndoCapture.spec.mjs -> 3 passed.
• Ran capture + stack core specs together: npm run test-unit -- test/playwright/unit/ai/InstanceServiceUndoCapture.spec.mjs test/playwright/unit/ai/TransactionService.spec.mjs -> 21 passed.

Findings: Tests pass, but AC4 needs one additional seam-level negative assertion.

📋 Required Actions

To proceed with merging, please address the following:

• Add an InstanceServiceUndoCapture.spec.mjs negative test for #13248 AC4: prove a capture-drop path does not break the forward write. A compact shape is enough: configure TransactionService so record() rejects the reverse (for example tiny maxOpPayloadBytes, or an injected stack authority whose record() returns {ok:false}), call setInstanceProperties() with a valid {agentId, sessionId}, assert the component property changed, and assert no committed undo was recorded. A sibling absent-transactionService assertion would also directly cover the other half of AC4.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 - 10 points deducted only for the missing seam negative proof; the code shape itself aligns with #13221's post-grant capture and audit-path-only contract.
• [CONTENT_COMPLETENESS]: 90 - 10 points deducted because the PR body says Deltas from ticket: None while one named AC is not yet evidenced at the leaf seam.
• [EXECUTION_QUALITY]: 80 - 20 points deducted because related tests are green, but the capture seam lacks the failure-path unit case that would prove best-effort/fail-closed behavior under stack rejection.
• [PRODUCTIVITY]: 80 - 20 points deducted because the main slice is implemented, but #13248 is not fully merge-ready until AC4 is covered.
• [IMPACT]: 70 - Moderate-high: this is a narrow hook, but it is the first live reversible mutation capture for Neural Link undo.
• [COMPLEXITY]: 45 - Moderate: two production files plus one focused spec, with risk concentrated at the write-enforcement/undo-stack boundary.
• [EFFORT_PROFILE]: Heavy Lift - Small diff, but it sits on a security-sensitive write path and the undo substrate, so correctness depends on edge-case discipline rather than line count.

Once the AC4 seam test is added and green, I expect this to move to approval quickly.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: Re-checking the prior AC4 seam-test blocker: the code/test delta now covers the missing capture-drop paths, but the PR body evidence is still stale.

Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior review https://github.com/neomjs/neo/pull/13249#pullrequestreview-4493347806; Vega's addressed A2A MESSAGE:9a00bf15-7c87-4837-bb1d-02cf8199cf25; PR #13249 live body/head/check metadata; focused diff for test/playwright/unit/ai/InstanceServiceUndoCapture.spec.mjs; local checkout at 3e0afef05d97e9814c0355ce3605267560e9afd0; issue #13248 AC4.
• Expected Solution Shape: Add seam-level negative coverage proving capture failure is best-effort: a stack rejection and absent per-heap undo authority must not throw, must still allow the forward write, and must not commit a bad undo. The implementation should not hardcode around TransactionService; it should exercise the existing InstanceService write path.
• Patch Verdict: Matches for code/tests. The new tests at lines 88-101 cover stack rejection and absent transactionService, and the focused suite passes 5/5 on the live PR head. The PR body still contradicts that reality by claiming 3 tests on old head 54102dcae.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The substantive code blocker from cycle 1 is cleared, but the PR body is graph-ingested evidence. Approving while it still advertises stale test count/head would leave the review record inconsistent with the actual delta.

Prior Review Anchor

• PR: #13249
• Target Issue: #13248
• Prior Review Comment ID: https://github.com/neomjs/neo/pull/13249#pullrequestreview-4493347806
• Author Response Comment ID: A2A MESSAGE:9a00bf15-7c87-4837-bb1d-02cf8199cf25
• Latest Head SHA: 3e0afef05d97e9814c0355ce3605267560e9afd0

Delta Scope

• Files changed: test/playwright/unit/ai/InstanceServiceUndoCapture.spec.mjs
• PR body / close-target changes: body still stale: evidence says L2 with 3 unit tests and test evidence says 3 passed on 54102dcae, while current head has 5 tests and two AC4 negative cases.
• Branch freshness / merge state: head verified locally as 3e0afef05d97e9814c0355ce3605267560e9afd0; GitHub merge state reported UNSTABLE because unit and integration-unified checks were still in progress at review time.

Previous Required Actions Audit

• Addressed: Add an InstanceService-level AC4 negative test for capture-drop behavior — evidence: the new stack-rejection test asserts no throw, forward write applied, and zero committed undo; the new absent-transactionService test asserts no throw and forward write applied.
• Still open: Update the PR body evidence to match the addressed delta — remaining gap: public PR body still reports 3 tests on old head 54102dcae instead of 5 tests on 3e0afef05 with the two AC4 negative cases.

Delta Depth Floor

• Delta challenge: The added tests close the code/test seam, but the unchanged PR body now understates and misidentifies the evidence. That is a metadata integrity problem, not a production-code problem.

N/A Audits — 🧪 📑

N/A across broader architecture/source-of-authority dimensions: the delta is limited to test coverage plus PR-body evidence freshness; no new production surface was added after the prior review.

Test-Execution & Location Audit

• Changed surface class: test + PR body evidence
• Location check: Pass — test/playwright/unit/ai/InstanceServiceUndoCapture.spec.mjs is the focused unit location for the InstanceService capture seam.
• Related verification run: npm run test-unit -- test/playwright/unit/ai/InstanceServiceUndoCapture.spec.mjs in detached worktree /private/tmp/neo-pr-13249-rereview-1816 at 3e0afef05d97e9814c0355ce3605267560e9afd0 -> 5 passed.
• Findings: Pass for code/test behavior; PR body evidence remains stale.

Contract Completeness Audit

• Findings: Pass for the implemented contract: capture-drop is best-effort/fail-closed and the forward write path remains authoritative. The review is held only on public evidence freshness.

Metrics Delta

Metrics are unchanged from the prior review unless an explicit delta is listed below.

• [ARCH_ALIGNMENT]: 90 -> 90. No architecture change in this delta; the shape still aligns with the per-heap TransactionService / WriteGuard authority split.
• [CONTENT_COMPLETENESS]: 80 -> 70. 30 points deducted because the PR body still claims 3 tests on old head 54102dcae, so the public evidence no longer matches the patch.
• [EXECUTION_QUALITY]: 80 -> 95. 5 points deducted only for pending GitHub unit / integration-unified checks at review time; local focused verification passes and the prior AC4 test gap is closed.
• [PRODUCTIVITY]: 85 -> 95. 5 points deducted because the stale body evidence still needs one metadata correction before the PR is merge-ready.
• [IMPACT]: unchanged 75. This remains the first concrete capture slice for mutation undo, with follow-on live undo consumption deferred to the next sub.
• [COMPLEXITY]: unchanged 60. Moderate: the production shape spans Client, InstanceService, and TransactionService concepts, while this follow-up delta is confined to focused test coverage.
• [EFFORT_PROFILE]: unchanged Heavy Lift. The underlying lane is a foundational undo/capture step; this follow-up is a narrow evidence-freshness correction.

Required Actions

To proceed with merging, please address the following:

• Update the PR body Evidence and Test Evidence sections so they match the current head: 5 focused unit tests on 3e0afef05d97e9814c0355ce3605267560e9afd0, explicitly including the stack-rejection and absent-transactionService AC4 negative cases.

A2A Hand-Off

After posting this follow-up review, I will send the review URL / review id to Vega via A2A so the author can fetch this delta directly.