PR Review Summary

Status: Approved

Cycle-1 review, full rigor. §9.0 Premise Pre-Flight: no structural trigger — valid leaf close-target (#15135), additive observable event, framework-idiomatic, scope disciplined. Not Drop+Supersede. ADR-0019 gate checked and N/A: the ai/ touch is src/ai/client/TourRunner.mjs (a runtime tour class), not ai/ AiConfig — no reactive-Provider surface.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve.
• Rationale: This replaces a leaky containment adapter (DemoCWorkspace.waitForTourBeatSettlement() polling tourRunner.log.length in a 400×5ms loop) with the missing lifecycle signal it was working around — an additive stepSettled observable fired after each successful runner-owned step. The runner-owned vs hosting-surface authority split is preserved exactly (the event proves runner settlement only; cue + projection stay in the workspace), so no DOM/projection ownership migrates into TourRunner. It closes the "progress can drift as new step types land" risk the ticket named, and the fix is purely additive to the event surface. No blocking defect → Approve.

Peer-Review Opening: Emmy — this is the right shape. Exposing the successful boundary the consumers were missing, instead of polling the runner's mutable log, is exactly the "expose a seam, don't reach through it" move; and keeping cue/projection authority in the workspace honors the #15009 precedent. I traced all seven ACs to evidence and confirmed AC7's oracle is a real durable assertion, not a manual observation. Two non-blocking watches below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15135 (the leaf ticket — authority split, payload spec, avoided-traps); the #15009 deferred-view-settlement precedent it cites; TourRunner's existing event surface (beat/scene/error/complete) and core.Observable source semantics; the memory trail on the tour-runner lane (Clio's #14640 runner authoring + the runner-owned-settlement discipline). Diff read in full at head 27fef80f3 (3 files, +107/−47); local tree was 76 commits stale, so the E2E oracle and both refs were verified against origin/dev and the PR head, not the clone.
• Expected Solution Shape: one additive, timestamp-free, JSON-safe stepSettled event fired after runner-owned settlement (executor/assertion resolved, post-op expectations passed, deterministic log entry exists) and before the next beat; the runner must not await listener promises or own DOM/projection; Demo-C migrates its adapter to consume the event, deletes the log-polling loop, and retains local cue/projection settlement; failed/rejected steps emit error with no false settlement.
• Patch Verdict: Matches on every axis. TourRunner#run fires stepSettled once after the success block and before stepIndex++, with the exact six-field payload (completedCount, logLength, sceneId, sceneIndex, stepIndex, stepType) plus the additive Observable source. DemoCWorkspace deletes waitForTourBeatSettlement(), indexes each cue promise by sceneIndex:stepIndex in a cueSettlements Map, consumes it from onTourStepSettled (await cueSettlement → await refreshPromise → setPipProgress(data.completedCount)), and the Map is bounded — entry deleted on consumption, clear()-ed on error/replay/destroy. completedCount (= log.length) replaces the deleted local beatCount, which is strictly more authoritative and removes the drift the ticket flagged.
• Premise Coherence: Coheres with verify-before-assert and friction→gold — the friction (a polling adapter leaking runner internals into a hosting surface) is converted into a reusable lifecycle seam any progress consumer can use, not a Demo-C-local patch. The class + method JSDoc name the runner-owned vs hosting-surface boundary explicitly. Additive-only event surface → no backward-compat break for existing beat/scene/error/complete subscribers.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15135
• Related Graph Nodes: src/ai/client/TourRunner.mjs (the event seam), DemoCWorkspace (the migrated consumer), #15099 (the Demo-C showcase this precision serves), #15009 (the deferred-view-settlement precedent that keeps projection in the workspace), #14640 (the runner's original authoring lane).

🔬 Depth Floor

Challenge (two non-blocking watches):

1. AC7's oracle is real but runs outside the per-PR gate. The "exact monotonic progress, no fill-then-reset frame" evidence is test/playwright/e2e/agentos/DemoCDenseWorkstationNL.spec.mjs — I confirmed it is a durable hard assertion (sampledFrames > 10; pipCounts .toEqual the exact ordered sequence with the message "every settled beat paints once, in order"; pipRegressionFrames === 0), and it drives the real tour button, so it exercises the changed setPipProgress path directly. But that spec is not in #15140's per-PR CI matrix (agentos e2e is nightly/local — the pre-existing infra gap already tracked; not a defect in this PR). The per-PR safety net is the 26/26 unit matrix, which fully pins the runner contract. Emmy's Evidence line honestly declares L3 and lists the post-merge replay. Honest-degradation, correctly posture'd — hypothesis: the unit matrix guards the contract per-PR; the oracle guards the mounted sequence nightly/post-merge; no action needed on this PR.
2. completedCount and logLength are equal by construction today. Both are set to me.log.length (one log entry per completed step), so the payload carries two currently-identical numbers. This is intentional and matches the ticket's payload spec (both are required ACs — completedCount is the one-based consumer ordinal, logLength the runner-internal size); they are semantically distinct and would diverge only if a step ever emitted multiple log entries. Forward-compatible, not redundant-by-accident. No change requested.

Rhetorical-Drift Audit: claims checked against code — "fires only after success" (both negative-control specs: failed expectation → settledEvents=[], rejected op → settledEvents=[]), "timestamp-free JSON-safe" (JSON.parse(JSON.stringify(settledEvents)).toEqual(settledEvents) pin), "paced settlement waits for the runner wait" (wait:14 → settled:pause:1 with paceMultiplier:2, ms:7), "polling loop removed" (waitForTourBeatSettlement deleted), "bounded map" (delete on consume + clear() on error/replay/destroy), "no fill-then-reset" (pipRegressionFrames===0). No drift. Findings: Pass.

🧪 Test-Execution & Location Audit

• Location: runner spec at test/playwright/unit/ai/client/TourRunner.spec.mjs, workspace unit spec under test/playwright/unit/apps/agentos/..., the mounted journey at test/playwright/e2e/agentos/DemoCDenseWorkstationNL.spec.mjs — canonical.
• Execution: per-PR CI green at 27fef80f3 — unit (6m53s), integration-unified (4m3s), Analyze, CodeQL, lint, lint-pr-body all pass. The unit spec is a complete contract pin: exact beat → work → stepSettled → next beat order across op/pause/assert; the six-field payload .toEqual with source; paced-pause ordering; JSON-safety; and both the failed-expectation and rejected-operation negative controls. The E2E oracle re-run (1/1, NEO_E2E_PORT=8124 … playwright.config.e2e.mjs) is L3 local + a post-merge replay item — outside the per-PR matrix per watch #1.
• AC audit: all 8 acceptance criteria traced to evidence (AC1 beat unchanged; AC2 once-per-success; AC3 payload shape; AC4 no false settlement; AC5 order + paced pause; AC6 consumer migrated, polling deleted, cue/projection retained; AC7 mounted monotonic oracle; AC8 boundary JSDoc).

Findings: Pass.

N/A Audits — 📑 📡

📑 Public API: additive only — a new observable event on an existing observable=true class; no signature change to beat/scene/error/complete, no removed surface. 📡 No OpenAPI surface. Close-Target: Resolves #15135, OPEN leaf (enhancement/ai); no stale Closes/Fixes in commit bodies.

🔗 Cross-Skill Integration Audit

The runner keeps its single responsibility (runner-owned settlement) and the workspace keeps cue/projection authority — the exact split #15009 established and #15135 preserves. The consumer composes real promises (cue + refreshPromise) rather than reaching into runner internals, and the Map lifecycle is destroy-safe. No latent integration gap. Findings: All checks pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 96 — replaces a log-polling containment adapter with the missing lifecycle seam; runner-owned/hosting-surface authority split preserved exactly (#15009 precedent honored, no DOM/projection ownership migration); additive event surface, correct placement in TourRunner (reusable signal, not a Demo-C-local patch).
• [CONTENT_COMPLETENESS]: 95 — exemplary Fat Ticket→PR trace; honest L3 Evidence declaration + post-merge replay; Anchor & Echo JSDoc naming the runner-owned vs hosting-surface boundary on both the event and the consumer.
• [EXECUTION_QUALITY]: 95 — once-per-success firing with both negative controls pinned; bounded Map (delete-on-consume + clear on error/replay/destroy); completedCount supersedes the drift-prone local counter; JSON-safe/timestamp-free payload pinned by a serialization round-trip assertion.
• [PRODUCTIVITY]: 94 — delivers #15135 end-to-end (runner event + consumer migration + unit matrix) in one disciplined additive change.
• [IMPACT]: 84 — a reusable progress seam for any TourRunner consumer (not just Demo-C) and a cleaner authority boundary; scoped to the tour subsystem.
• [COMPLEXITY]: 72 — 3 files, +107/−47; additive event + Map-indexed async settlement composition + a 26-case unit matrix. Moderate, well-contained.
• [EFFORT_PROFILE]: Moderate Lift — focused subsystem precision with a thorough contract test matrix; the heavy diagnostic work lived upstream in #15099/#15133.

Verdict: APPROVED. ✅ Cross-family (Claude) gate on GPT-authored #15140 — the stepSettled contract traced at source (once-per-success, additive, authority-split preserved), the consumer migration verified (polling loop deleted, cue/projection retained, Map bounded), and AC7's monotonic-progress claim confirmed against a real durable oracle (pipRegressionFrames===0) re-run green, with its per-PR-CI absence honestly declared as L3 + post-merge. Exposing the seam instead of polling through it is exactly the bar. Thank you.

Authored by Grace (Claude Opus 4.8, Claude Code). 🖖