Repository navigation
Conversation
ADR 0012 requires equal-strength candidates in the recorded scroll region to precede candidates in other regions. Compare regions with the shared selector evidence owner after basis deduplication and before document order. Regression: divergence suggestions across regions fail on base 4c1c786 (1 failed, 2 passed) and pass with the fix (3 passed). No recorded evidence retains document order. No live device run is required for diagnostic suggestion ranking.
ADR 0010 requires reachable SDK failures to use AppError with a stable code and machine-readable reason. Malformed daemon lease envelopes now throw COMMAND_FAILED with invalid-lease-response and structured response details. Regression: the public SDK allocate and heartbeat malformed-response matrix fails on base 4c1c786 (8 failed, 4 passed) and passes with the fix (12 passed). Assertions also verify normalizeError preserves the code, reason, and response details.
ADR 0033 permits barrels only at package boundaries. Remove the session lifecycle and observability re-export barrels and point production callers, test imports, and mocks at the existing implementation owners. Pin the same command exports at their owning modules. Keep private helper and reverse-handler boundaries enforced. Retire both barrel paths in R10. Neither deleted path has a Fallow baseline entry. Regression: direct owner and restored-barrel checks fail on base 4c1c786 (2 failed) and pass with this fix (2 passed). Focused architecture coverage: 48 passed. Retained command-routing tests: 4 passed. No live device run is required because behavior is unchanged.
ADR 0022 uses a membership ratchet with no approval input. Remove the nonexistent ADR-only growth approval path from both R75 diagnostics. Route reads and operations through an owning facade or semantic query; an intended boundary change must change its invariant and gate together. Regression: the existing shape/authority diagnostic test fails against base 4c1c786 (1 failed) and passes with this fix (1 passed). The complete session-authority overlay suite passes all 10 tests. No live device run is required for tooling diagnostics.
|
![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 26 files
You’re at about 91% 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
Size Report
Startup median (7 runs, lower is better):
|
ADR 0012: only verified record-time evidence can select a scroll partition. Preserve the base order for unverifiable evidence and retain verified none-partition ranking. Proof on e984698 with updated tests: unverifiable evidence fails (1 failed, 4 passed); fixed suite: 5 passed. Removing region ranking separately fails the verified-none positive control.
ADR 0010: use INVALID_LEASE_RESPONSE in the lease reason family for non-object payloads and invalid required leaseId, tenantId, runId, and backend fields. Retain response details through normalization. Regression proof on e984698: 40 failed, 4 passed; with the fix: 44 passed. Both allocate and heartbeat exercise non-object leases and invalid required fields.
ADR 0010: put INVALID_LEASE_RESPONSE beside the SDK lease API and remove the duplicate proxy mention. Document both non-object responses and invalid required lease fields. Validation: pnpm check:doc-prose and pnpm check:doc-links passed, including website build. No prose assertion tests added.
ADR 0022: distinguish named store operations for SessionStore authority from semantic queries for SessionState shape reads. Pinned diagnostic proof on e984698: 1 failed, 9 passed; fixed suite: 10 passed.
ADR 0033: session retired-path lists include both former handler paths and deleted barrels. Rename those lists to RETIRED_PATHS and update consumers without changing enforcement or moving owners out of internal/. Focused architecture/modularity/overlay tests: 58 passed. Restoring both tracked barrels in a disposable checkout: 2 failed, 7 passed; guards retained after the rename. Internal owner placement remains a follow-up.
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
You’re at about 91% 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 PR is ready at 20066f1, and I found no problems in the code. Not blocking: the user doc at website/docs/docs/replay-e2e.md:327 says equally strong candidates in the recorded scroll region rank first, but the code applies this only to On the open review threads, the ADR 0012 scroll-region wording thread (#3762 (comment)) still applies, as do three lower-priority threads. You can resolve #3762 (comment), because the backend failure contract carries no target-v1 evidence, so the explicit undefined at replay-backend-failure.ts:61 is correct. The Coverage check did not pass, and the log excerpt I have shows no failing test or threshold line, so I cannot say whether it comes from this PR. Please find the failure in the full job log. A changed line without coverage, such as replay-backend-failure.ts:61, is one possible cause. Before merge, the Coverage check must pass and ADR 0012 and replay-e2e.md must describe the verified-only region tie-break. |
thymikee
added
the
ready-for-human
Fix the eager-closure failure in Coverage: the ranking facade must not eagerly load the ad-script and selector graphs. Await ranking at both production response seams and cover Maestro ranking without target-v1 evidence. Share scroll-region membership across ranking, classification, recording, and repair hints. Review: #3762 (comment) Review: #3762 (comment)
The maintainer requires both ADR 0012 passages and the user documentation to match the verified-only target-v1 contract before merge. Review: #3762 (comment)
Restore the actionable ADR and gate pointers in the SessionState shape-edge diagnostic. Review: #3762 (comment)
|
Follow-up pushed at
The full Coverage job log identifies an eager-closure failure: 45 modules versus 1 at the merge base, adding 729 top-level statements. The changed-line threshold stage was skipped. The same guard reproduced locally at Validation: setup/build and repository format passed; focused suite 1,013 passed; authority suite 10 passed; related suite 3,054 passed. Exact-head affected gate passed all 36 runnable checks: Threads: 4 fixed, 1 already correct, 0 rejected; all 5 replied to and resolved. The internal-facade move remains a follow-up. All 18 new-head CI checks are queued/running, including Coverage and Integration Tests; their final results remain required before merge. |
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
You’re at about 95% 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
Compute verified region membership at the divergence caller that owns target evidence. Keep the ranking entry dependency-free without deferred imports.
Keep isInScrollRegion as the shared consumer predicate and update the repair-hint comment to name it.
Apply the scroll-region tie-break only for a verified target-v1 annotation. Review: #3762 (comment)
|
Follow-up pushed at
Proof: replay/backend suites 16 passed; budget suite 940 passed; related suite 3,054 passed. Isolated mutation at
|
|
The changes at 8a49ba0 look good, and I found no problems. The earlier review at 20066f1 (#3762 (comment)) was clean, and the new commits fix the doc, ADR and overlay wording gaps raised since then. I did not run the tests. I judged the "unverifiable" regression case by reading the expected orders against the ranking code. All 20 reported checks are green at 8a49ba0, and none is failing. There are no conflicts. On the other threads, these no longer apply, so please resolve them: session-replay-divergence doc comment, ADR 0012 target-v1 wording, client-api error.details.response, overlay authority invariant and shared isInScrollRegion helper are fixed at this head. The Maestro region tie-break is benign, because ADR 0012 now scopes the tie-break to verified evidence and the new Maestro test pins the order. The gate message wording is benign, because it only concerns the text of a gate message for a retired barrel path and this delta does not change it. Nothing else blocks it, so the PR is ready for human review. |
This branch has not been deployed
1 participant
Summary
COMMAND_FAILED,INVALID_LEASE_RESPONSE, and the preserved daemon response. Document the error contract.Part of #3744. 31 files; scope includes regression tests, diagnostics, and matching documentation. ADR 0012’s two factual corrections satisfy the maintainer’s before-merge requirement. No device behavior changes.
Validation
8a49ba0a0857a25fb735226fba81698f40885f9c(follow-up basee4c4813491).296e27e205: same suggestion suite changes from 5 passed to 2 failed/3 passed; temporary worktree removed..tmp/validation/run-dDItTG/receipt.json:exactCommit=true,checkoutChanged=false.