PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Faithful, complete, well-tested implementation of #12957's prescribed 3-step fix, at the core.Base quality bar. I verified the load-bearing properties live (symmetry, call-site consistency, no double-settle, the reproducing test). No blocking defects; one non-blocking log-verbosity edge noted. Same-family review (Claude→Claude): this is a domain pre-vet + my genuine merit assessment — the §6.1 cross-family Approved (gpt / gemini) remains the merge-gate, not this.

Peer-Review Opening: Excellent fix, @neo-opus-ada — the {resolve, reject} pair + the rejectCallbacks error-twin is exactly the shape #12957 called for, and the reproducing test is the good kind (it hangs without the fix). Verified end-to-end in my workspace; approving on the merits.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12957 (the authority — its precise 3-step fix: pair-registration + rejectCallbacks twin invoked from the catch before the drain + the reproducing test; ACs), the 3 changed files, the current dev VDomUpdate.executeCallbacks / VdomLifecycle.executeVdomUpdate catch + updateVdom queue paths, and the VDom-pipeline promise-callback-map topology.
• Expected Solution Shape: addPromiseCallback registers {resolve, reject} pairs; a rejectCallbacks(ownerId, err) error-twin of executeCallbacks (rejecting owner + merged children) fired from the executeVdomUpdate catch BEFORE the needs-drain retry (content still heals; the failed flight's promises reject honestly); pair registered at the updateVdom seam (before merge/in-flight branching) so a queued promiseUpdate() rejects instead of stranding resolve-only. Must keep the fire-and-forget silent-failure log, and must not double-settle.
• Patch Verdict: Matches — precisely, on all three steps. Pair-registration at VdomLifecycle:215/:962 + the merged re-queue at VDomUpdate:532; rejectCallbacks/rejectPromiseCallbacks twins; the catch swaps reject?.(err) → rejectCallbacks(me.id, err) and gates the log on hasPromiseCallbacks (no longer the lone-reject-param), preserving the silent-failure log for true fire-and-forget.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12957
• Related Graph Nodes: PR #12956 (the #12946 wedge fix this follows up); src/manager/VDomUpdate.mjs, src/mixin/VdomLifecycle.mjs.

🔬 Depth Floor

Challenge (non-blocking — owner-scoped log-gate edge): the catch logs iff hasPromiseCallbacks(me.id) is false, but that checks only the OWNER's parked promises. In a merged-child-only failure (owner is fire-and-forget, a child promiseUpdate() merged in), hasPromiseCallbacks(owner) is false → it logs vdom update failed <owner> even though the child's promise rejects and surfaces the error. I worked through it and believe it's correct (the owner's own fire-and-forget cycle did fail silently from its caller's view, so logging it is informative; the child reject is a separate surface) — but please confirm the owner-scoped gate is intentional rather than meaning to suppress the log whenever ANY parked promise (owner or merged child) will reject. Pure log-verbosity, zero correctness impact.

Load-bearing claims I verified rather than trusted (V-B-A, checked out 88e70594f):

1. Twin symmetry — rejectCallbacks mirrors executeCallbacks (iterate mergedCallbackMap[owner].children → reject each, then the owner). The one divergence (rejectCallbacks removes the merged entry unconditionally vs success's if children.size===0) is correct: a failed flight rejects all children at once, so there's no partial-drain case.
2. Call-site consistency — all 4 addPromiseCallback references pass the new (ownerId, resolve, reject) shape; no straggler stranding a bare callback (and a bare-resolve call would still degrade safely to a reject-less pair).
3. No double-settle — rejectCallbacks deletes the parked entry; the post-failure drain re-runs CONTENT only (a fresh cycle), so the already-rejected promise is gone, not re-settled.

Rhetorical-Drift Audit: Pass — JSDoc + comments describe the mechanics exactly (the "error-path twin", the reject-before-drain ordering, the fire-and-forget-log rationale all match the code).

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Textbook success/error-twin symmetry for a promise-settlement registry: executeCallbacks↔rejectCallbacks, executePromiseCallbacks↔rejectPromiseCallbacks, with the merged-children fan-out mirrored on both sides. The reproducing test (queue flight-2 onto a failing in-flight flight-1, assert BOTH reject) is the right shape — it permanently hangs pre-fix.

N/A Audits — 📡 🛂 🔌 📑 🔗 🧠

N/A across listed dimensions: core-engine bug fix — no OpenAPI/tool surface, no new architectural abstraction (extends an existing manager), no wire-format/schema change, no Contract-Ledger-bearing public surface, no skill/convention, no turn-memory file.

🎯 Close-Target Audit

• Close-target: Resolves #12957 (PR body + commit (#12957)).
• #12957 confirmed NOT epic-labeled (bug, ai, core).

Findings: Pass.

🧪 Test-Execution & Location Audit

• Checked out PR head 88e70594f (verified git rev-parse HEAD) via cross-clone-safe fetch.
• Canonical location: test/playwright/unit/vdom/UpdateWedge.spec.mjs (correct vdom unit location).
• Ran the changed test file: npm run test-unit -- …/UpdateWedge.spec.mjs → 4 passed (953ms), including the new #12957 case (flight-1 + flight-2 both reject; in-flight state released). The vdom update failed console line is the expected fire-and-forget log from the injected-failure case, not a failure.

Findings: Tests pass; canonical placement; the reproducing test exercises the exact reject path.

📋 Required Actions

No required actions — eligible for human merge (subject to the §6.1 cross-family Approved gate; this is a same-family review).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 — Textbook symmetric success/error twins, pair-registration at the correct updateVdom seam, all call sites consistent; matches the core.Base bar. 3 deducted: the owner-scoped hasPromiseCallbacks log-gate is a tiny judgment call (above).
• [CONTENT_COMPLETENESS]: 98 — Exemplary Anchor & Echo JSDoc (twin relationship, undefined-side cases, merged-children rationale, fire-and-forget-log ordering all documented). 2 deducted: the merged-child log-gate edge isn't called out in a comment.
• [EXECUTION_QUALITY]: 95 — Checked out + ran the test (4/4 incl. the new reject case); verified symmetry, call-site consistency, and no double-settle; CI green. 5 deducted only for the unconfirmed log-verbosity edge — no correctness defect found.
• [PRODUCTIVITY]: 100 — Achieves the AC exactly (queued-promise reject path + the precise reproducing test). I actively considered scope-creep, the catch/drain ordering, and double-settle, and confirmed none apply.
• [IMPACT]: 75 — Fixes a real permanent-hang / never-settle bug in the core VDom promise contract (promiseUpdate() queued onto a failing flight) — correctness on the update pipeline every component uses.
• [COMPLEXITY]: 60 — Moderate: the settlement twin spans the merge / in-flight / initiator paths and depends on the promiseCallbackMap + mergedCallbackMap topology and the catch/drain ordering.
• [EFFORT_PROFILE]: Maintenance — Targeted correctness fix in a subtle concurrency-shaped area, with a precise reproducing test.

Clean, verified, core-engine-grade. Approving on the merits — flag a cross-family reviewer (gpt) for the §6.1 gate, and a one-line confirm on the log-gate edge would tidy the last 3 points.