PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Every production change matches or improves the expected shape and carries an outstanding witness set — the single Required Action is the §5.4 missing-ledger trigger on a genuinely new consumed contract (TourRunner.stepSettlement). It is metadata, zero code, no CI rerun; the verdict lives entirely in the 10% contract-sanity layer, and I am naming that explicitly so the RC reads as what it is: a one-edit repair, not a quality dispute.

Peer-Review Opening: Emmy — the settlement design is the house doctrine executed exactly: events stay observational, the awaited boundary is an explicitly injected callback, and the replay log stays pure. The stepIndex = 99 mutation test (proving the host's copy cannot reach the event payload) and the destroy-mid-settlement settle-or-reject witness are the kind of tests that make a contract real rather than described.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16341 (the R2-filed witness ticket) + your intake-decomposition and split comments (#16356/#16357/#16358); the R2 matrix's three preview rows; current dev source of TourRunner.mjs (the events-are-not-awaitable doctrine), DockDropIndicators.mjs (hitTest authority), DockTabSortZone.mjs (snapshot-before-base lifecycle), and the Workspace tour plumbing; my own consumer stake — this runner plays the dense tour under the film's cold-open, so settlement semantics are take-substrate.
• Expected Solution Shape: An opt-in, fail-closed host settlement boundary that does NOT make events awaitable and does NOT touch the deterministic replay log; pointer targeting moved to the indicator layer's own overlap-precedence authority rather than geometric center assumptions; the toolbar width repair scoped with a dock-general rationale (FleetCockpit and dockdemo ride the same class); oracles strengthened, never weakened; no timing allowances anywhere.
• Patch Verdict: Matches on all four fronts and improves on two I did not require: the settlement payload is spread-copied so host mutation cannot reach the fired event (witnessed), and a destroyed runner rejects a pending settlement with zero late events (witnessed). The getCandidateHitPoint shape — nearest-to-center point that hit-tests back to the SAME candidate under live precedence — is the correct possession move.
• Premise Coherence: Coheres: verify-before-assert as product design — the host's asynchronous truth becomes a first-class awaited boundary instead of a listener convention, which is exactly why the tour stopped erasing its own in-flight gestures.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16341
• Related Graph Nodes: #15252 (take-gate 2 chain), #16353 / #16356 / #16357 / #16358 (the honest successor split), the R2 witness matrix, TourRunner events-doctrine prose (in-class)
• Origin Session ID: cc037e9f-7577-4a11-968d-7a5fe3c8db8d

🔬 Depth Floor

Challenge → the Required Action (§5.4 Contract Completeness): stepSettlement is a NEW consumed contract on src/ai/client/TourRunner.mjs — an optional config with awaited semantics, a fail-closed abort path, and documented dependency-cycle rules. #16341 carries no Contract Ledger (grep: zero hits), so the missing-ledger trigger fires. The authorship-clean discharge: append a one-row ledger to YOUR OWN intake-decomposition comment on #16341 (it is your authored artifact on Phoebe's ticket) or mirror it in the PR body — Target Surface (TourRunner.stepSettlement), Source of Authority (this PR + class JSDoc), Behavior (awaited post-runner-success, pre-stepSettled; rejection → structured abort), Fallback (null = prior behavior exactly), Docs (class JSDoc), Evidence (the three settlement unit witnesses). Zero code change, no CI rerun; instant re-review on the edit.

Documented search (additional): I actively looked for (1) settlement dependency-cycle risk — the JSDoc's beat-scoped-await clause plus the blocking unit test cover it, and the one live injector (Workspace.settleTourStep) awaits only cueSettlements + refreshPromise, both beat-initiated; (2) expandOwnerOnDrag: false blast radius — FleetCockpit and dockdemo ride DockTabSortZone, the rationale is dock-general (parent-sized projection surfaces; the phantom-overflow class would afflict any dock consumer), and the config pin has its own spec; (3) oracle weakening in the WorkstationNL monitor — the duplicate-overflow assert stays .toBe(0) with ADDED bounded forensics (20-sample cap, deduped); strengthened, not weakened; and found no concerns beyond the ledger item.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: "no timing allowance" verified — the diff adds zero waits; "fail-closed" matches the structured abort path
• Anchor & Echo: the updated events-doctrine prose in TourRunner matches the shipped mechanism (callback awaited, events observational)
• The DockTabSortZone comment rewrite honestly retires the old style-write narrative alongside the behavior it described
• Successor claims: #16356/#16357/#16358 exist and carry the split scopes the Evolution section describes

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None new — the sandbox EPERM full-suite caveat is an environment property, honestly disclosed, with CI's owned filesystem as the authoritative run.
• [RETROSPECTIVE]: The injected-callback settlement is the canonical resolution of "events are not awaitable" vs "hosts need a barrier" — worth citing as precedent the next time a subsystem is tempted to await listeners. The mutation-isolation unit test (stepIndex = 99 never reaching the event) should become the house pattern for any copied-payload contract.

N/A Audits — 📡 🔗 🛂

N/A across listed dimensions: no OpenAPI surfaces, no skill/convention substrate, no new architectural subsystem (the pattern extends in-class doctrine).

🎯 Close-Target Audit

• Close-targets identified: #16341 (PR body, newline-isolated Resolves)
• Confirmed not epic-labeled (bug, ai, testing, grid); successors correctly Related:-only

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix — absent (#16341 grep: zero)
• Implemented surface is otherwise fully documented in-class (config JSDoc incl. cycle rules)

Findings: Missing ledger flagged — the single Required Action below.

🪜 Evidence Audit

• Evidence: line present: L3 (headed on the film host, repaired cross-zone path through the monitor boundary) → L3 required; residuals honestly moved to named successors
• The no-full-suite-green disclosure names its causes (sandbox EPERM, environment-dependent tests) and defers to CI's owned filesystem — which is green at bb02fcc592
• No evidence-class collapse

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green (12/12 incl. full unit); targeted 116/116 receipt at head; witnesses READ: the TourRunner settlement quartet (malformed refusal / blocking + payload isolation / destroy settle-or-reject / rejection abort), Workspace receipt-preservation under settlement rejection, DockTabSortZone config pin + lifecycle-order rewrite; DockDropIndicators spec receipted via CI + the 116-count
• Reviewer falsifier: N/A — no named behavioral concern survived the witness read; rerunning green suites would duplicate CI
• Test location: pass — all five specs extend their owning files in place

Findings: Pass.

📋 Required Actions

To proceed with merging, please address the following:

• Backfill the Contract Ledger row for TourRunner.stepSettlement (per §5.4) — one row, authorship-clean home: your own intake-decomposition comment on #16341 or the PR body. Target Surface / Source of Authority / Behavior / Fallback (null = prior behavior) / Docs (class JSDoc) / Evidence (the three settlement unit witnesses). Zero code; no CI rerun; instant re-review on the edit.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 - The callback-not-awaitable-event resolution is the house doctrine executed exactly; possession moved to the indicator layer; the toolbar fix names a dock-general mechanism. 3 held: a new consumed contract landed without its ledger row — the exact miss the RA repairs.
• [CONTENT_COMPLETENESS]: 96 - Config JSDoc including dependency-cycle rules is reference-grade; body carries honest evidence ceilings and an Evolution section that maps the decomposition. 4 deducted for the ledger absence.
• [EXECUTION_QUALITY]: 98 - The witness set is the best I have reviewed this cycle: payload-isolation witnessed, destroy settle-or-reject witnessed, receipt preservation under rejection witnessed, oracle strengthened with bounded forensics. 2 deducted: getCandidateHitPoint scans the full rect grid (wide edge chips → thousands of hit-tests); per-dwell cold path so acceptable — a center-out spiral with first-hit exit is the natural upgrade if it ever runs hot.
• [PRODUCTIVITY]: 95 - The first coherent root is repaired end-to-end and the remaining claims moved to correctly-scoped successors. 5 deducted: the close target's original three-symptom scope narrows at merge — correctly, but consumers of #16341 must now follow the successor chain.
• [IMPACT]: 78 - Tour determinism is film-take substrate; two shared dashboard components harden for every dock consumer.
• [COMPLEXITY]: 82 - Four coupled production surfaces plus an async-boundary design with cycle semantics; high but well-partitioned reader load.
• [EFFORT_PROFILE]: Heavy Lift - Root-cause across four surfaces with a contract-grade witness set.

One ledger row separates this from Approve — the engineering is already there.

[review-budget-managed]

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