Repository navigation
docs(adr): correct stale facts in the ADR set - #3796
Conversation
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 33 files
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
The independent review by a different model provider is complete at Seven corrections remain:
I verified these Cubic claims independently and assigned one repair round. No production behavior change or new prose-only test is needed. All 25 source threads have PR-link replies and remain unresolved. The actual reply counts are 21 fixed and four base-correct; the PR metadata needs that correction. The exact-head receipt passed. CI and a review of the repaired head remain required. The PR is not ready. |
Item A. packages/command-registry/src/registry.ts declares the separately typed daemon facet; daemon-command-descriptor.ts owns DaemonCommandDescriptor. packages/daemon/src/core/daemon-command-registry.ts derives the table and exposes predicates. Its tests pin policy and routes. 4648051 (#906) introduced the dormant descriptor and parity tests. 47abc8c (#907) activated derivation and deleted the hand table. This corrects the brief attribution to #906 alone. Known invariant-3 gap: types.ts CommandDescriptorBase exposes daemon?: DaemonCommandTraits, and src/cli/command-explain.ts describeCommandDaemon reads and publishes route/traits. Preserve the invariant and report the gap; do not change behavior.
Items 1 and 2. packages/contracts/src/interaction-guarantees.ts declares 14 guarantees, including keyboardOcclusion, parentOwnedTouchPoint, settleObservation, resolutionDisclosure, outcomeObservation, and targetStability. Match the complete list. rg finds only the TypeScript snapshot-visibility-tap-point-policy.test.ts and RunnerTests+TapPointPolicyTests.swift reading tap-point-policy.json; the matrix parityTable is metadata. No fake runner reads it. Keep migration step 3 open.
Item 3. packages/daemon/src/resources/lease-registry-scope.ts DEFAULT_LEASE_TTL_MS = 60_000; normalizeLeaseTtlMs uses it when no positive TTL is supplied. The admitted-work section already states one minute.
Item 4. gh issue view 1299 reports CLOSED, closedAt 2026-07-21T08:54:50Z. packages/maestro/test/conformance/differential/scenarios.ts settle-after-tap has no knownDivergence and requires settleLatches >= 1 and settleTimeouts <= 0. invariants.test.ts exercises synthetic traces. This is repository evidence, not a new live-device result.
Item 5. a13a683 (#1567) added target-authored drag. packages/daemon/src/execution/interaction/internal/interaction-gesture.ts calls gesturePayloadToPositionals; packages/daemon/src/core/session-script-writer.ts calls dragGesturePayloadFromPositionals. PUBLIC_GESTURE_SYNTAX still defines arity for the two parsers.
Items A and 6. 47abc8c (#907) activated the derived daemon table after parity in 4648051 (#906). Mark the hand-table inventory as pre-migration context. 189b062 (#960) moved client-types.ts into client/. 494eb52 (#1877) removed read. gh issue view 1153 reports CLOSED, closedAt 2026-07-08T16:35:29Z, but src/client/client-types.ts alert and reactNative still return CommandRequestResult. Do not infer total narrowing from closure.
Item 7. packages/platform-apple/src/snapshot-route.ts opensGenerationCircuit excludes failure.kind === preparing and WINDOW_COORDINATE_SPACE_UNRESOLVED. snapshot-route.test.ts pins that bridge preparation sends only the current capture to XCTest. Keep glance and body consistent.
Item 8. ADR 0011 Layer 2 and ADR 0014 Mutation admission and read behavior / Command effects are daemon-owned and completely classified replace removed phase, step, and escape-hatch references. Change comments and one test name only. R56 evidence: c77bc40 (#2021) deleted the legacy src/core/dispatch.ts app-switcher arm and dispatch facet. The current descriptor retains daemon.route generic, but platformExecution is device-runtime; generic-runtime-execution.ts routes system buttons through system-button-runtime.ts, whose app-switcher row uses appSwitcherRuntimeUse. Retained generic routing is intentional, not evidence of a surviving legacy leaf or runtime bug. Clarify that distinction.
Item 9. c77bc40 (#2021) retired src/core/dispatch.ts and dispatchKnownCommand. packages/daemon/src/server/request-router.ts calls resolveGenericRuntimeExecution before dispatchGenericCommand. packages/daemon/src/execution/generic-runtime-execution.ts rejects its default with COMMAND_FAILED / generic-route-runtime-missing. Its __tests__/generic-route-runtime-completeness.test.ts pins unknown-command rejection. No dispatch-keyboard.test.ts exists. Keep the proposed fragment INVALID_ARGS contract conditional and identify the absent format-specific proof rather than claim this routing-gap test proves it.
Item 10. git log -S hard-reject identifies 66fe801 (#1177), which held the rejection rationale in Consequences, not Alternatives. The later structural-equivalence amendment superseded geometric mutation selection; 36957e8 rewrote the retained consequence. Remove only the dangling pointer in historical acceptance evidence. Do not restore an obsolete current selection rule.
Item 11. ADR 0019 section 5 proposed amendment already requires native-path disposition, retention by fence generation until both facts settle, and a failed-finish test for each kind before its cleanup changes. ADR 0024 sections 2.4 and 2.5 already retain archived generations; glance rule 6 already has the per-kind test gate. Match section 2.7 to these constraints. Both ADRs remain Proposed for this amendment.
Item 12. ADR 0019 section 2 Runtime use joins facts and narrows the bound runtime contains the exhaustive per-device-shape fact rule. Section 3 owns provider resolution. Change the citation only.
Items 15 and 16. Root src/__tests__/install-source.test.ts exercises shared provision-kit behavior and family adapters; cli-install-from-source.test.ts exercises the CLI boundary. Add the missing body owner matching the existing glance. efd2108 (#2808) added the no-fact-cell rule; 746e9b3 (#2806) enforces shape refusal at the web owner, pinned by packages/platform-web/src/runtime.test.ts. Mark the rule in force and correct its section pointer to section 2. Item 11: the section 5 proposed amendment is already correct and remains unchanged.
Item 17. The ADR trace counts 41 distinct files for press/Android and 30 shared plus 17 AX / 19 runner for snapshot/iOS (47/49). Both exceed the superseded 38/29. The facade PRs predate e624ef9, so they cannot explain the difference; the old count is not higher.
Item 18. 057dfb7 (2026-09-20) recorded the initial toolchain examination; e8da1e1 (2026-09-21, #2717) and 6cb160a (#2734) recorded a working stream. 590f171 (2026-10-14, #3587) records no motion subcommand in devicectl 651.13.5 / Xcode 27.1 build 27A9275, then called RC, and adds dispatched-unverified refusal. Read-only host check on 2026-10-16: xcodebuild -version reports Xcode 27.1, 27A9275; devicectl --version reports 651.13.5; device help lists no motion subcommand. Preserve dated successes without claiming every 27.1 build supports the stream. Sync the glance to existing no-sample behavior. No device state changed.
Item 19. ADR 0019 section 9 permits one second bind for --settle when the action use has no capture, after facts admission. The second bind is capture-only; captureSnapshot in the action use reuses the cached binding. Correct the fixture comment only.
Item 20. gh issue view 3294: CLOSED 2026-10-08T07:57:29Z. gh issue view 3281: CLOSED 2026-10-07T18:48:47Z. 8c5bce0 (#3297) moved daemon diagnostics to src/daemon-contracts; 75b414a (#3299) moved the assembly pair into src/command-runtime. Current owners are packages/daemon-contracts/src/daemon-diagnostics-scope.ts and src/command-runtime/{runtime-command-surface,runtime-factory}.ts. Add a 2026-10-16 correction to the dated observation without declaring every root move complete.
M1. Registry tests pin the derived table; types do not fully enforce invariants 3 and 4. Keep both invariants required and synchronize Rules at a glance. Evidence: packages/command-registry/src/types.ts:213 makes daemon optional. DaemonCommandDescriptor requires command and route but makes the other traits optional. packages/daemon/src/core/daemon-command-registry.ts:34-35 defaults missing descriptors to generic. src/cli/command-explain.ts:205-210 reads daemon traits in the public CLI projection. Registry policy pins are in packages/daemon/src/core/__tests__/daemon-command-registry.test.ts. Maintainer follow-up: determine how types should enforce invariant 4 without weakening the accepted declaration boundary.
L6. Describe a bridge preparation that is still running. Do not invent a preparation queue. Evidence: packages/platform-apple/src/snapshot-source/preparation.ts:119-123 reports bridge-preparation-pending through attempts.value. packages/platform-apple/src/snapshot-route.ts:314-315 excludes preparing failures from the generation circuit. The existing glance exception already agrees.
M2, L1, L2. Restore the contract-move prerequisite and its reason. State the full extent of broad client results. Retain the historical client path and restore prerequisite wording without the unrelated ADR 0010 pointer. Synchronize the client-contract rule in Rules at a glance. Evidence: src/client/client-types.ts has 21 Promise<CommandRequestResult> methods, including alert, reactNative, and interactions.swipe. packages/command-registry/src/command-result.ts:41-47 documents dynamic payload exclusions; its result map remains nonexhaustive. packages/daemon/src/execution/interaction/internal/interaction-gesture.ts:59-74 retains local result types and Record payloads. Git commit 189b062 (#960) moved src/client-types.ts into src/client/. gh issue view 1153 confirms CLOSED at 2026-07-08T16:35:29Z.
L7. Repoint the historical hard-reject rationale to Consequences instead of deleting the useful pointer or restoring an obsolete alternative. Evidence: git log -S hard-reject identifies 66fe801 (#1177), 36957e8, and 9362026. The current Consequences paragraph at line 1356 retains the reason blanket rejection is unsuitable: RN row/button/text wrapper chains. The structural-equivalence amendment remains authoritative.
L3. Add the two omitted consumers of the positional drag codec. Evidence: packages/replay-port/src/daemon-port/target-token.ts:13 calls dragGesturePayloadFromPositionals to select a source or destination target. packages/daemon/src/core/session-script-active-publication.ts:129 calls it through readRecordedDrag; assertPortableDragBindings at lines 102-123 validates portable endpoints and recorded target evidence before publication. No syntax or migration rule changes.
M3. Keep failure before platform execution as the proposed safety property. Record the session-dependent guard-request errors and the unresolved INVALID_ARGS requirement. A future codec cannot change an old reader. Keep Rules at a glance and both body descriptions aligned. Evidence path: packages/ad-script/src/internal/script.ts:270-283,483 accepts an unknown command as an action. packages/ad-replay/src/internal/verify-dispatch.ts:65,280-287 dispatches the unannotated action. packages/replay-port/src/daemon-port/session-replay-runtime-engine-adapter.ts:221-237 calls invokeReplayAction. session-replay-action-runtime.ts:171,199 preserves the command. packages/daemon/src/server/request-router.ts:407,658-662 returns unknown replay actions to the full request path because core/daemon-command-registry.ts:58-59 has no replayScopedAction trait for them. After admission, request-router.ts:346,393 reaches sessions/request-handler-chain.ts:127 and core/daemon-command-registry.ts:34-35. The unknown command routes to generic; the handler at request-handler-chain.ts:110-112 returns null. request-router.ts:596-598 returns noActiveSessionError with no session; packages/kernel/src/contracts.ts:263-264 sets SESSION_NOT_FOUND. With a session, request-router.ts:602-617 reaches execution/generic-runtime-execution.ts:116-123, which returns COMMAND_FAILED / generic-route-runtime-missing before dispatchGenericCommand at request-router.ts:619-620. The replay adapter wraps the failure through session-replay-runtime-engine-adapter.ts:295-303,384-398 and session-replay-runtime-failure.ts:106. session-replay-runtime-failure-response.ts:108 emits REPLAY_DIVERGENCE. packages/ad-replay/src/internal/step-loop.ts:178-185 stops the loop before later open or include actions. These are the current paths after admission, not universal error guarantees for every historical release. The existing generic-route completeness test proves only the resolver branch. Maintainer question: accept the existing old-reader errors, or keep INVALID_ARGS as an unresolved requirement?
L9. Attribute 41 and 47/49 to this trace, without claiming that they are current repository counts. Evidence: the Counting definition names commit 27a97ee and the retained trace tables measure press/Android and the two snapshot/iOS arms. Only the attribution changes; the counts and scope remain unchanged.
L4. Retain the dated corrections for completed moves and explicitly state that other physical moves remain outstanding as of 2026-10-16. Evidence: scripts/layering/root-module-zones.ts:9-14 states that logical classification is not physical collocation and that the table should shrink. Its core, command-runtime, and platform-runtime entries still name root src/*.ts modules. No layering rule or completed-move claim changes.
|
I reviewed 73d562b. The code edits change only comments and one test title. Three ADR claims from the open P2 threads below are still wrong and should be fixed before this is ready. Smaller notes: in 0008:104, Still open from the other review: the P2 threads on 0009 stale facet, platform-execution kinds and 0010 diagnostics scope still apply, and so do the four lower-priority threads. No thread is outdated. I spot-checked the 0008, 0009, 0010, 0014 and 0025 claims and the 21-method count against the code. I did not verify the remaining ADR hunks (0001, 0003, 0005, 0012, 0013, 0015, 0017, 0019, 0020, 0022, 0024, 0027, 0033) or the PR body's claims of an independent review and a local gate pass, and I ran no tests. Coverage failed, but its log stops at "building macOS helper" with no failing test visible. The diff changes only markdown, comments and one test title, so this looks unrelated, and Smoke Tests was still running. Before merge, fix the corrections above and get Coverage and Smoke Tests green on a re-run. |
platformExecution is a discriminated union of none, host, inventory, and device-runtime (packages/contracts/src/command-platform-execution.ts). The active design claimed it always names a runtime use; only the device-runtime variants do. Describe the shape and qualify the bound runtime use to those variants. Fixes #3796 review thread discussion_r4306650944.
The active design still said it composes with ADR 0008's descriptor capability facet. That facet is retired; the per-OS predicates live in request-bound runtime facts (packages/platform-apple/src/gesture-facts.ts and siblings). Match ADR 0008's platformExecution declaration. Dated history and the retirement notes keep their wording. Fixes #3796 review thread discussion_r4306650938.
Malformed RPC method params throw in packages/daemon/src/server/http/http-rpc-methods.ts before the router opens the request diagnostics scope (request-router.ts handleRequestOnLedger), so http-server.ts answers with normalizeError output that carries neither diagnosticId nor logPath; the validation tests pin that 400 / -32602 envelope. Scope the flush and field guarantee to errors inside a request diagnostics scope and state that pre-scope failures omit the fields. The remote logPathUnavailable rule is unchanged. Fixes #3796 review thread discussion_r4306650954.
… 0014 ref-frame.ts calls issuedRefFrame (ref-frame.ts activateRefFrame) and derives expiry through expiredRefFrame, both exported factories of ref-frame-slot.ts. The original sentence claimed no module outside the slot file can construct or derive a frame value. Confine the claim to the private class, its private fields, and direct construction; state the exported factory APIs and keep transitions and admission owned by ref-frame.ts. The slot-file comment carried the same overstatement and is corrected at the same scope. Fixes #3796 review thread discussion_r4306650966.
…e.ts resolveRefStalenessWarning (execution/session-snapshot.ts) warns on an active frame when a pinned ref's generation differs from the frame epoch; the existing test covers all three branches (expired any-ref, active plain/matching pin clean, active mismatched pin warns). The header comment claimed an active frame never warns. State the exact behavior. Fixes #3796 review thread discussion_r4306650958.
…fusal in ADR 0025 shouldFallbackToRunner gates the runner path in packages/platform-apple/src/core/screenshot.ts: a failure the policy rejects rethrows before any runner capture. The glance row implied the runner follows every simctl failure. State the policy condition. The app-window display facts are unchanged. #3761 merged into main as feca8cb (2026-10-18). When neither the sole lit panel nor CoreDevice's primary panel identifies a panel, resolveAppleCaptureTarget now refuses with COMMAND_FAILED and details.reason capture-panel-unidentified (display-inventory.ts); array order is never used. Update the Capture target body and add the glance row. The former deferred claim is removed. Fixes #3796 review thread discussion_r4306650971.
admitRuntimeOperations (packages/daemon/src/core/runtime-admission.ts) performs the side-effect-free facts inspection of the owner's appSwitcher fact and only then hands back the binding gateway; execution (packages/daemon/src/execution/system-button-runtime.ts) uses the bound operation. The comment implied admission itself uses the bound operation. State the two stages separately. Fixes #3796 review thread discussion_r4306650973.
…ntract The gate asserted the pre-#3761 help literal, so the merged pair (#3788 test, #3761 help owner) fails only in combination on main. Replace the stale rule with the current full contract line, including the capture-panel-unidentified refusal. The help text itself is unchanged; the assertion now matches the rendered first screen. CI evidence: Coverage job 117649571201 at 73d562b fails this assertion on unchanged-base integration; it is not caused by this PR's edits and not a flake.
73d562b to
278baaa
Compare
|
Round 2 complete at head 278baaa (rebased onto main feca8cb; range-diff proves the prior 47 commits unchanged). Corrected conditions: ADR 0008 platformExecution execution shapes (0008/0009 pair), ADR 0009 active design vs retired facet, ADR 0010 diagnostics-scope qualifier, ADR 0014 exported factories vs private slot class, ref-frame active-frame pinned-generation warning, ADR 0025 runner fallback policy + merged #3761 capture-panel-unidentified refusal, registry admission-vs-binding comment. Each fix reply names its commit and code proof. Threads: all 7 new threads on this PR replied and resolved. The 25 threads on #3729/#3731 stay open: 21 fixed, 4 base-correct, already replied with links. Metadata: PR body counts corrected (was 18/7; actual 21 fixed + 4 base-correct). Validation: |
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
The independent review of The diagnostics scope does not guarantee both fields. A bad-token request enters the scope, then returns I assigned a text correction for those conditions. The other repairs match the code. All 47 rebased commits match the prior range exactly. The current foldable assertion preserves the first-screen check and matches merged #3761 help. The exact-head receipt passed with an unchanged checkout. The separate prose and link checks passed. CI is still running. The PR is not ready yet. |
withDiagnosticsScope (packages/host-kit/src/internal/diagnostics.ts) runs no finalizer. Only finalizeThrownRequestError (request-router.ts) flushes, and it is reached by thrown errors alone: the token refusal in runRequestWithinScope is a returned normalizeError output without context, and flushDiagnosticsToSessionFile returns no record for zero events or a write failure, so an in-scope thrown error can still omit logPath. State that a thrown request error attempts one forced flush, diagnosticId requires an active scope, logPath appears only when the flush produces a file, and returned or pre-scope failures may omit both fields. The remote logPathUnavailable rule and the no-secrets rule are unchanged. Follow-up qualification on discussion_r4306650954.
… ADR 0025 buildInventory names the primary panel and emits apple_display_capture_ambiguous only when a primary panel exists (display-inventory.ts: the capture display is the sole lit panel or the primary panel); with neither, resolveAppleCaptureTarget refuses with capture-panel-unidentified. The two glance rows overlapped on zero or multiple lit panels. Qualify the fallback row condition. Fixes #3796 review thread discussion_r4306742872.
|
Follow-up to the review in #3796 (comment), at head c190504. Remaining qualification applied (0d83062): scope alone did not prove flush or fields. Coverage at 73d562b (job 117649571201) was not unrelated to the branch pair: the foldable first-screen test pinned the pre-#3761 help literal, so #3788's gate and #3761's help owner fail only in combination on merged main — not a flake. Fixed on this branch in 278baaa; the assertion now matches the rendered first screen and the focused test passes. Local gate at c190504: |
The previous sentence generalized over every thrown request error, but a malformed-RPC throw never reaches the router. Name the owner: the router's thrown-error finalizer attempts the flush. Replace the generic returned/pre-scope sentence with the two concrete cases that omit both fields, so the text no longer claims every returned or pre-scope failure must omit them; normalization preserves any existing metadata. Completes the qualification on discussion_r4306650954.
|
Qualification completed at 9edf321 (ADR 0010 only, no behavior): the flush owner is now the router's thrown-error finalizer (a malformed-RPC throw never reaches the router), and the omit-fields sentence names the two concrete cases instead of all returned/pre-scope failures. Gate |
|
The independent review by a different model provider is complete at I compared each changed sentence with the base in the initial review. All 47 rebased commits match that reviewed range exactly. I then compared every repair sentence and the existing help assertion with the base and current code. The diagnostic finalizer and panel fallback conditions now match their owners. The exact-commit receipt passed with an unchanged checkout. All 5,052 related tests passed. The prose and link checks passed. All eight PR threads have evidence replies and are resolved. The 25 source threads on #3729/#3731 retain the PR links and remain open. For ADR 0022 §2.5, the deletion test applies to pass-through and translation hops. It serves CLI, Node, and MCP users because both kinds of hop can add cost. Evidence proves only CI at this head is still running. The PR is not merge-ready yet. |
|
iOS Smoke failed at The raw job log shows This does not match the approved daemon-startup retry signature. I have not rerun the job. I assigned an isolated diagnosis of the fixture, capture route, artifacts, and base attribution. The code review and local receipt remain valid at their recorded head. The failed device check remains a blocker. |
|
The failure matches #3337, signature 3. I checked the raw log and artifacts. The wait exhausted 20 seconds with 31 readable captures. The later failure snapshot already contains The issue records the same signature on #3336 and a successful rerun at its unchanged head. This supports a timing-flake classification. It does not prove the host cause or a reproduction on current main. I reran the failed job once in run |
|
Ready at iOS attempt 1 failed with the documented #3337 signature 3. The single rerun passed at the unchanged head: attempt 2. Coverage and Integration Tests also pass. The native-cache matrix check is skipped; it is not a test pass. The exact-commit local gate, prose checks, and link checks passed. The independent review by a different model provider is complete. The 25 source threads retain the PR-link replies and remain open. The maintainer can merge this PR. I will continue monitoring it. |
|
Reviewed 9edf321. The ADR corrections match the code, and the findings from the earlier review at 73d562b are fixed: the 0008 platformExecution kinds, the 0009 retired facet, the 0010 diagnostics scope, the 0014 factory wording, the pinned generation in ref-frame.ts, the appSwitcher admission in registry.ts and the 0025 text. The code edits change only comments, one test title and one help rule string that now matches what main renders. All checks pass; the first iOS attempt failed with the known flake and the rerun at this head passed. Nothing else blocks this PR. |
thymikee
added
the
ready-for-human
|
1 participant
Summary
Correct stale ADR facts after #3729/#3731 merged. The change covers 35 files: docs, comments, one test title, and one existing help assertion. Runtime behavior is unchanged. The diff is 238 added and 128 removed lines against main
feca8cb2fc.All 25 source review threads have PR-link replies. Of these, 21 facts are corrected and four already match the code. Those threads remain open. All eight review threads on this PR have evidence replies and are resolved.
ADR 0022 §2.5 applies the deletion test to pass-through and translation hops. This covers CLI, Node, and MCP users. Only
commands/runtime-types.tsis proven removable. The correction permits no unsupported removal.ADR 0025 records merged #3761: a capture requires a sole lit panel or a primary panel. Otherwise, it refuses with
capture-panel-unidentified. Runner fallback requires its policy to permit the error.Validation
Head
9edf321f25; basefeca8cb2fc. The independent review by a different model provider compared each changed sentence with the base and code. No findings remain. All 47 rebased commits match the previously reviewed range.pnpm check:affected --run --head 9edf321f25passed:exactCommit=true,checkoutChanged=false; 5,052 related tests passed. Prose and link checks passed. Runtime device tests do not apply.Coverage at
73d562bfailed on an old foldable help assertion after #3761/#3788 merged. The assertion now matches the rendered help. No CI rerun masked the failure.All required CI checks pass. iOS attempt 1 matched #3337 signature 3; the single unchanged-head rerun passed. The branch is CLEAN and mergeable.