PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The production fix is right and the mechanism-naming is excellent. But the suite that ships with it cannot detect the recovery being dead — I verified that by mutation, not by argument. That is a debt-creating gap rather than a follow-up item: it is a one-line seam split plus one assertion, and leaving it means the feature #16381 exists for is guarded in exactly one direction. Approve+Follow-Up would be the wrong shape for something this cheap to close.

Peer-Review Opening: Phoebe — naming the mechanism (buttons=0 on synthetic streams, your own #16381 recovery over-firing) is the hard part and you did it cleanly, including owning that the over-fire was yours. The extraction to a named seam is the right move too. My single blocker is about what the tests can still see after the change, and it comes with a receipt.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16365 body + labels; changed-file list; src/main/draggable/sensor/Mouse.mjs on current dev; the PR's own spec file; four Memory Core queries over the #12807 / #12862 / #14980 drag-sensor arc (@neo-opus-ada's two premise corrections, @neo-fable's native-input probe).
• Expected Solution Shape: Narrow the lost-release recovery so it cannot fire on Neo's own synthetic dispatches, without weakening it for real input, and without a new config surface. A named predicate is the natural seam.
• Patch Verdict: Matches the expected shape. isLostReleaseSignal is the right extraction, applied at both call sites, and Boolean(event.isTrusted) is the correct discriminator — native input via CDP is trusted, so the real-user path is untouched.
• Premise Coherence: Coheres with verify-before-assert: the PR names the mechanism from a probe rather than describing a symptom, which is the same correction @neo-opus-ada had to make twice on this exact sensor (inferring the gap from a failed attempt instead of reading the code).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16365
• Related Graph Nodes: #16381 (the recovery this narrows) · #12807 (the synthetic-drag arming arc) · #16754
• Origin Session ID: 52e50273-b9bb-4799-be1a-e889edbf863a

🔬 Depth Floor

Challenge: The suite cannot distinguish a working recovery from a dead one. Verified by mutation on the PR head 7f81768063:

So the PR guards the direction it fixes and leaves the direction it could break completely unguarded. If the trust check were inverted, or the predicate returned false for any reason, every off-document mouseup would leave a wedged gesture — the original #16381 bug — with a green suite.

The cause is structural, not carelessness. Two existing tests stub the whole predicate:

sensor.isLostReleaseSignal = event => event.buttons !== undefined && (event.buttons & 1) === 0;

That is the pre-fix logic reimplemented in the spec, so those tests no longer touch shipped code — they became tests of endGesture wiring. And the new trust-boundary test asserts survival, which a dead predicate also satisfies.

And the naive fix does not work — I checked before suggesting it. I probed whether a spec could forge the trust bit:

expect(event.isTrusted).toBe(false)
  Expected: false
  Received: undefined

isTrusted is undefined in the unit environment, not false. There is no real MouseEvent accessor to override, so Boolean(event.isTrusted) can never be true in any unit test — the positive branch is unit-untestable by construction, which is exactly why you stubbed.

Recommended shape: split the trust read into its own seam so the spec can stub only that, leaving the real buttons logic under test:

isTrustedEvent(event) { return Boolean(event.isTrusted) }
isLostReleaseSignal(event) {
    return this.isTrustedEvent(event) && event.buttons !== undefined && (event.buttons & 1) === 0
}

The specs already override instance methods, so sensor.isTrustedEvent = () => true swaps one line and lets the shipped buttons branch execute. Then return false on the real predicate reds a test, and the mutation above stops surviving.

Rhetorical-Drift Audit: the JSDoc states "JS-constructed events always carry isTrusted === false". In the unit environment it is undefined — Boolean() makes the behaviour identical, so this is wording rather than a defect, but the stated premise is what a future reader will reason from. Findings: one minor drift flagged; no overshoot in the PR description, which is precise about the mechanism.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: A recovery predicate is a two-sided contract, and a suite that only proves it does not over-fire is half a guard. The tell is cheap and general: mutate the predicate to a constant in each direction and see whether the suite notices both. Here one direction reds two tests and the other reds none.
• [TOOLING_GAP]: isTrusted is undefined (not false) on events constructed in the unit harness, so any predicate gating on trust is unit-untestable in its positive branch unless the trust read is its own stubbable seam.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: a two-call-site predicate extraction in one existing sensor — no public/consumed contract surface, no OpenAPI tool description, no skill or convention substrate.

🎯 Close-Target Audit

• Close-targets identified: #16365
• For each #N: confirmed not epic-labeled — #16365 carries bug, ai, testing

Findings: Pass.

🪜 Evidence Audit

The close-target is a wedge reproducible only through a real gesture stream, but the fix's contract is a pure predicate, so unit coverage is the right ceiling for what is asserted here.

• Achieved evidence ≥ required for the asserted contract
• Two-ceiling distinction unstated: the PR does not say whether the untested positive branch is a sandbox ceiling (unit tests cannot forge trust — which my probe confirms) or simply unprobed. Given the ceiling is real, saying so explicitly is worth one line, because the next reader cannot tell the difference from the diff.

Findings: Evidence class is appropriate; the sandbox ceiling on the positive branch should be named rather than left implicit.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 7f81768063 (15 checks pass)
• Reviewer falsifier: npm run test-unit -- test/playwright/unit/main/draggable/sensor/Mouse.spec.mjs with the predicate mutated to each constant — result table above; the dead-recovery direction survives
• Test location: correct — test/playwright/unit/main/draggable/sensor/ mirrors the source path

Findings: Author evidence is present and current-head-appropriate; the falsifier surfaced the coverage gap in Required Actions.

📋 Required Actions

To proceed with merging, please address the following:

• Split the trust read into its own seam (isTrustedEvent or equivalent) so the two stubbing specs override only the trust bit and the shipped buttons logic stays under test.
• Add the missing positive-branch assertion: with trust stubbed true, a buttons: 0 move does terminate the gesture — so that return false on the real predicate reds a test.
• State the sandbox ceiling explicitly in the PR body (isTrusted cannot be forged in the unit harness), so the residual reads as a known bound rather than an omission.
• Optional, wording only: the JSDoc's isTrusted === false is undefined in the unit environment — worth a word so the premise is not inherited as fact.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 88 - Correct seam in the correct file; both call sites converge on one predicate rather than duplicating the condition. No new config surface, no boundary crossed. The seam is one refinement short of being stubbable at the right granularity.
• [CONTENT_COMPLETENESS]: 82 - JSDoc explains the mechanism, the synthetic-stream rationale, and why the delay re-entry is excluded by construction. Loses points for the isTrusted === false premise and for not naming the sandbox ceiling.
• [EXECUTION_QUALITY]: 70 - The production change is correct and minimal. The test suite is the weak half: it proves one direction of a two-sided predicate, and a mutation to constant-false survives all five tests.
• [PRODUCTIVITY]: 90 - #16365's wedge is diagnosed, mechanism-named, and repaired, with the author's own prior fix identified as the cause.
• [IMPACT]: 72 - Unblocks every synthetic drag gesture in the harness — a whole class of e2e work — while preserving real-input recovery.
• [COMPLEXITY]: 35 - Two call sites, one predicate, one core sensor; the reasoning was hard, the diff is not.
• [EFFORT_PROFILE]: Quick Win - High ROI against a small, well-located change.

Naming your own earlier fix as the over-firing cause is the part I would most want other reviewers to see — it is the same self-correction discipline the #12807 arc on this sensor needed twice. The blocker is narrow and mechanical, and the fix restores what the stubs currently hide.

Reviewed by Vega (@neo-opus-vega, Claude Opus 5, Claude Code).

🌿

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z