refactor: consolidate architecture ownership and client results#1210
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Review found one small cleanup before readiness: two source-of-truth breadcrumbs are stale after the ownership moves. |
|
Addressed in |
|
Follow-up re-review at
All exact-head CI is green, but the two P2s keep this follow-up from readiness. Assigning a focused fixer. |
|
Follow-up re-review items are fixed in |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Re-review at Because this is a broad architecture refactor, the configured high-risk route still calls for an independently authorized Claude pass. I am withholding |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
devin-ai-integration
Bot
force-pushed
the
devin/1783749457-architecture-consolidation
branch
from
4924684 to
e731982
Compare
Independent Claude/Opus cross-vendor high-risk passReviewed exact head FindingsLOW — stale ADR breadcrumbs to a deleted module (actionable)
All four symbols now live in INFO — one back-edge cleared by zone reclassification, not dependency inversion (non-blocking) Verified clean (no findings)
ReadinessNo blocking issues. Runtime semantics, layering, selector ownership, client narrowing, MCP, and platform wire are all sound. Recommend fixing the LOW stale-breadcrumb finding (docs-only) and confirming the pending Smoke check before marking ready-for-human. Not approving/merging or labeling, per scope. |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Independent Claude/Opus cross-vendor pass — verdict at
|
devin-ai-integration
Bot
added
the
ready-for-human
|
thymikee
removed
the
ready-for-human
…lution guard + step-5 resume wiring Rebuilt onto current main (#1210 selector relocation, #1211 resume + --update retirement, #1213, #1215) as one coherent commit — the prior 4-commit history collided structurally with #1211's own session-replay-runtime split. Step 4 (decision 3, enforcement): every annotated resolved-target replay/test action is verified against a fresh pre-action snapshot BEFORE dispatch (verifyReplayActionTarget → classifyReplayTarget, decision 3's six paths). Only a verified outcome sends the action; selector-miss / identity-mismatch / identity-unverifiable each return a complete target-binding REPLAY_DIVERGENCE built from its own capture, with targetBinding {classification==kind, matchCount per the presence rule, recorded/observed, mismatches, candidates} through #1211's shared failure-response + bounding/sanitizer pipeline. Post-resolution guard: a verified outcome mints internal.replayTargetGuard (the verified member's identity); dispatch's own resolution (occlusion/ visibility guards verification does not replicate) must land on the SAME element or assertExpectedResolvedTarget refuses pre-action, which the loop converts to an identity-mismatch divergence. Guarded dispatch skips the direct-iOS and native-ref fast paths so a tree node exists to check. Step-5 resume wiring (this rebase): the target-binding divergence resume stub is replaced with #1211's buildReplayDivergenceResume — a pre-action divergence resumes AT the failed step (from = step index, real SHA-256 planDigest, allowed unless preflight rejects the skipped range). buildTargetBindingDivergenceResponse is the single resume site; planActions/planDigest are threaded from the loop. --update healing is retired by #1211, so the in-branch "verify healed action" work is dropped (its module and test removed as moot). Tests: classification (13), guard (4), token (5), runtime end-to-end (9, incl. old-script pass-through, guard threading + mismatch conversion, and a real computed resume assertion), plus target-binding wire tests in divergence.test.ts (classification==kind, matchCount presence, overflow-fallback targetBinding, computed-resume-not-stub, text report). Also repoints daemon-http-disconnect.test.ts already handled upstream by #1215.
… enforcement (#1209) * feat(replay): ADR 0012 step 4 target-binding verification + post-resolution guard + step-5 resume wiring Rebuilt onto current main (#1210 selector relocation, #1211 resume + --update retirement, #1213, #1215) as one coherent commit — the prior 4-commit history collided structurally with #1211's own session-replay-runtime split. Step 4 (decision 3, enforcement): every annotated resolved-target replay/test action is verified against a fresh pre-action snapshot BEFORE dispatch (verifyReplayActionTarget → classifyReplayTarget, decision 3's six paths). Only a verified outcome sends the action; selector-miss / identity-mismatch / identity-unverifiable each return a complete target-binding REPLAY_DIVERGENCE built from its own capture, with targetBinding {classification==kind, matchCount per the presence rule, recorded/observed, mismatches, candidates} through #1211's shared failure-response + bounding/sanitizer pipeline. Post-resolution guard: a verified outcome mints internal.replayTargetGuard (the verified member's identity); dispatch's own resolution (occlusion/ visibility guards verification does not replicate) must land on the SAME element or assertExpectedResolvedTarget refuses pre-action, which the loop converts to an identity-mismatch divergence. Guarded dispatch skips the direct-iOS and native-ref fast paths so a tree node exists to check. Step-5 resume wiring (this rebase): the target-binding divergence resume stub is replaced with #1211's buildReplayDivergenceResume — a pre-action divergence resumes AT the failed step (from = step index, real SHA-256 planDigest, allowed unless preflight rejects the skipped range). buildTargetBindingDivergenceResponse is the single resume site; planActions/planDigest are threaded from the loop. --update healing is retired by #1211, so the in-branch "verify healed action" work is dropped (its module and test removed as moot). Tests: classification (13), guard (4), token (5), runtime end-to-end (9, incl. old-script pass-through, guard threading + mismatch conversion, and a real computed resume assertion), plus target-binding wire tests in divergence.test.ts (classification==kind, matchCount presence, overflow-fallback targetBinding, computed-resume-not-stub, text report). Also repoints daemon-http-disconnect.test.ts already handled upstream by #1215. * test(replay): assert real computed resume on identity-mismatch and guard-mismatch paths Non-blocking review follow-up: only the selector-miss runtime test asserted the computed resume {allowed, from, planDigest}. All three target-binding paths funnel through buildTargetBindingDivergenceResponse (one shared resume site), so this is coverage not a bug — but pin resume on the identity-mismatch and guard-mismatch tests too so no path can regress its resume wiring silently. * fix(replay): carry structural denotation into the post-resolution guard (P1) The guard carried only LOCAL identity {id, role, label}. ADR path 6 isolates ONE member among several SAME-local-identity nodes via sibling / region-scoped viewportOrder — a discriminator lost at the guard. So if verification denoted duplicate A but dispatch's occlusion/visibility filter selected duplicate B (identical id/role/label), the guard passed and SENT the action on the wrong element — the exact verified-but-taps-different-node mis-binding step 4 exists to prevent. Fix: replayTargetGuard now carries a structural denotation of the verified member — its pre-order document index (decision 3's canonical total order, `node.index` after the shared reindex pipeline) plus its same-parent sibling ordinal. `assertExpectedResolvedTarget` compares BOTH local identity AND structural denotation against dispatch's pre-promotion capture, refusing pre-action when EITHER differs. Two distinct duplicates always differ in document order, so a different-member resolution can no longer pass. The promotion exemption is preserved (comparison is on the verified LEAF, and duplicates are distinct leaves). New helpers live in target-identity-node.ts (replay zone) so writer/classifier/guard share one sibling-ordinal implementation; computeSiblingOrdinal now delegates to it. The guard-mismatch divergence surfaces a `position:` mismatch line (from the structural denotations) so `mismatches` is never empty when local identity is identical. END-TO-END regression (session-replay-target-guard.test.ts): two "Save" buttons with IDENTICAL id/role/label, A covered+deeper (verification's winner), B visible (dispatch's winner) — driven through the real interaction runtime (`device.interactions.press`), the guard refuses pre-action with ZERO backend taps AND zero native tapTargets, the refusal driven purely by the structural denotation (both observedStructural/expectedStructural asserted). Plus a same-structural passing case, a runtime-level `position:`-mismatch divergence test, and updated guard-threading assertions for the {identity, structural} shape. Wire contract unchanged: replayTargetGuard is DaemonRequestInternal (never on the wire); decision 4's targetBinding {recorded, observed} stay LOCAL identity per decision 3's Identity tier — the structural denotation is an internal guard concern, surfaced only as a `mismatches` string. No ADR change needed.
1 participant
Summary
Consolidates #1182, #1183, #1184, and #1187 by moving shared contracts below their consumers and eliminating the full 36-identity target-spine back-edge baseline without introducing compatibility barrels:
doctor,diff,replay,test,record,trace) to canonical exact contracts while preserving runtime payloads and documenting the 0.20 minor-version policy; dynamic interaction families remain deliberately broad.ios/macoswire values, command behavior, descriptor semantics, fast paths, and centralized interaction guarantees—including the selector-resolution disclosure added onmain.sdk-selectors.jswith a clean-build regression that rejects auto-numbered fallback chunks.Link to Devin session: https://app.devin.ai/sessions/b1c3093bf2b248ca82a1a5df89152d02
Requested by: @thymikee