Repository navigation
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/adr/0025-foldable-apple-panels.md">
<violation number="1" location="docs/adr/0025-foldable-apple-panels.md:22">
P2: This glance rule incorrectly applies the CoreDevice lit-panel resolver to runner captures. The runner fallback after `simctl` failure follows the target app window and its display, so scope this row to CoreDevice-backed captures and document the runner path separately.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| | The hinge reaches `half-open` but keeps moving | Refuse it as `fold-pose-unsettled` with the observed and previous angles: an angle inside the open interval is an observed category, not a pose the hinge holds | | ||
| | An external display is attached | It is not a panel: it never makes the device multi-screen and never produces a pose | | ||
| | CoreDevice cannot answer | Return an unresolved inventory and keep the single-panel capture path; a missing host feature is not a capture failure | | ||
| | Any iOS simulator capture | First resolve the CoreDevice display table. Capture the panel that the system lights. | |
There was a problem hiding this comment.
P2: This glance rule incorrectly applies the CoreDevice lit-panel resolver to runner captures. The runner fallback after simctl failure follows the target app window and its display, so scope this row to CoreDevice-backed captures and document the runner path separately.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/adr/0025-foldable-apple-panels.md, line 22:
<comment>This glance rule incorrectly applies the CoreDevice lit-panel resolver to runner captures. The runner fallback after `simctl` failure follows the target app window and its display, so scope this row to CoreDevice-backed captures and document the runner path separately.</comment>
<file context>
@@ -2,566 +2,617 @@
-| The hinge reaches `half-open` but keeps moving | Refuse it as `fold-pose-unsettled` with the observed and previous angles: an angle inside the open interval is an observed category, not a pose the hinge holds |
-| An external display is attached | It is not a panel: it never makes the device multi-screen and never produces a pose |
-| CoreDevice cannot answer | Return an unresolved inventory and keep the single-panel capture path; a missing host feature is not a capture failure |
+| Any iOS simulator capture | First resolve the CoreDevice display table. Capture the panel that the system lights. |
+| More than one integrated panel | Pass `--display=<panel name>` on every `simctl io screenshot`. Never use the implicit default. |
+| Zero or multiple lit panels | Name the `primary` panel. Emit `apple_display_capture_ambiguous`. Report pose as `unknown`. |
</file context>
| | Any iOS simulator capture | First resolve the CoreDevice display table. Capture the panel that the system lights. | | |
| | CoreDevice-backed iOS capture | First resolve the CoreDevice display table. Capture the panel that the system lights. | |
There was a problem hiding this comment.
Not changed here. This row has the same meaning as the base row on main ("Any iOS simulator capture | Resolve the CoreDevice display table first…"), and this PR keeps base meaning. Scoping it to CoreDevice-backed captures corrects a fact that is stale at base, so it belongs to docs/adr-stale-facts (umbrella #3744). That branch does not fix it yet. I left this thread open.
|
Code looks fine at 76d7191: this PR changes only docs/adr markdown, so there is no code risk. No conflicts. The three CodeQL Analyze jobs are still queued, not failed, and they do not scan these files, so I read them as unrelated. I did not diff all 12 ADRs against main to check that every decision, measurement and status survived. I also did not check whether any deleted history still carries an active rule, and I did not check the 0022 and 0025 claims against the code or the R76 gate. Not blocking, take or leave: 0033:64 still says Open review threads still apply. These two hold: 0033:26 "R76 inventory" claim and 0025:27 fully-open pose. The thread on 0033:64 managed-allocation holds too. One lower-priority thread holds: 0025:224-226 keeps superseded history under a current section (thread). Three P2 threads and one P3 thread are still unchecked: 0022:156 querySelector qualifier, 0025:22 simulator capture rule, and 0022:49 platform-runtime gate. None of the threads can be resolved yet. Before merge, please fix the 0033 managed-allocation row and the 0025 pose-report row, then answer or resolve the remaining threads. Please check the unchecked ones against the code first. |
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/adr/0025-foldable-apple-panels.md">
<violation number="1" location="docs/adr/0025-foldable-apple-panels.md:139">
P2: This sentence omits two typed fail-closed paths: a resolved window can lack a display, or its display image can be unrenderable. Document all three refusal conditions so this ADR does not mislead consumers about runner capture failures.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
| Thus, a session without an app, or with a stopped app, can still resolve a window's display. | ||
| The result always names a display occupied by a window. | ||
| It never defaults to `XCUIScreen.main`, which is dark on a Duo when the app uses the inner panel. | ||
| A capture fails closed with a typed reason only when neither window resolves. |
There was a problem hiding this comment.
P2: This sentence omits two typed fail-closed paths: a resolved window can lack a display, or its display image can be unrenderable. Document all three refusal conditions so this ADR does not mislead consumers about runner capture failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/adr/0025-foldable-apple-panels.md, line 139:
<comment>This sentence omits two typed fail-closed paths: a resolved window can lack a display, or its display image can be unrenderable. Document all three refusal conditions so this ADR does not mislead consumers about runner capture failures.</comment>
<file context>
@@ -135,7 +136,7 @@ SpringBoard owns the home-screen window.
The result always names a display occupied by a window.
It never defaults to `XCUIScreen.main`, which is dark on a Duo when the app uses the inner panel.
-Only failure to resolve either window causes a typed, fail-closed capture refusal.
+A capture fails closed with a typed reason only when neither window resolves.
The query uses both `exists` and frame evidence to refuse an app without a window.
Such an app returns `(0,0 0x0)` from `frame` without an exception.
</file context>
| A capture fails closed with a typed reason only when neither window resolves. | |
| A capture fails closed with a typed reason when neither window resolves, the resolved window has no display, or its image cannot be encoded upright. |
There was a problem hiding this comment.
Not changed here. This sentence is identical to the base sentence on main, which round 1 restored. The code does have three typed failures (RunnerAppScreenCapture.swift:21-23: unresolvedWindow, unresolvedScreen, unrenderableImage), so the base claim is stale. That correction belongs to docs/adr-stale-facts (umbrella #3744), which does not fix it yet. I left this thread open.
|
Two meaning-drift problems in the ADR rewrite remain at 4577385, so it is not ready to merge yet. CI is green (8 checks, none failing; the diff touches docs only), and there are no conflicts. I did not read the finding list from 76d7191, so I cannot confirm those items are fixed. I also did not diff all 12 ADRs against main for dropped conditions. First, ADR 0022 line 32 now says route depth may collapse only pass-through hops. ADR 0023 (line 13 and line 59) applies the deletion test to pass-through and translation hops. The rewrite drops the translation case from a decision rule, so a reader could wrongly bar the removal of a removable translation hop. Please restate the rule as "pass-through or translation hops that the deletion test proves removable". Then check the other rewritten rules in 0022 against main for dropped scope. Second, ADR 0033 line 65 still says "move (batch 1) → packages/managed-allocation", but line 10 of the same ADR says that package was removed on 2026-10-10. Line 159 presents R76's inventory as a current boundary, while ADR 0022 says the inventory is empty and was deleted. The table now contradicts itself and 0022, and it shows a deleted package as a live move target. Please mark the row as withdrawn or rewrite it against the current state. Please also reword line 159 to refer to R76's plain ban. Not blocking: in ADR 0025 line 27, the pose rule says panel power alone can report fully-open. This may be a silent change from the original, so please compare it with main and the pose contract section, and restore the original narrower claim if it differs. I did not check the runner or inventory code for 0025. You can take or leave this. The open threads from other reviewers still apply to these files. Please address them with the fixes above:
One lower-priority thread also still applies: #3731 (comment) Before merge, fix the drift in 0022 line 32 and 0033 lines 65 and 159. Then resolve or dismiss the remaining 0025 and 0023 threads. |
The base decision applies the deletion test to pass-through and translation hops and endorses only the proven removable hop. The glance named only pass-through hops. Restore the selector "seam" term from the base rationale. #3731 (comment)
The base text says the row and Batch 1 record a move that landed in #3287 and that the package removal then deleted. The rewrite dropped that the move landed. #3731 (comment)
The Device Hub deletion also removed the base sentence that fold sets the pose and that only the read-back is evidence. Point to the HID amendment that replaced the Device Hub press. #3731 (comment)
Name pass-through and translation hops in one rule, as the base deletion test does, and drop the commit hash that the base does not attach to the verdict. #3731 (comment)
ADR 0022 records that #3430 emptied the R76 inventory and reduced R76 to a plain ban on the src/platform-runtime-*.ts family. The ban still keys on these paths, so the retain-for-this-batch rationale holds; only the noun was stale. #3731 (comment)
…oved The row is history: the move landed in #3287, and the package removal on 2026-10-10 deleted the file. State this in the row, as the Status note does, so the row does not read as a live move target. #3731 (comment)
|
Follow-up at Maintainer items
CI: green at Threads: 5 resolved (4 fixed; 1 already fixed on Validation: |
|
Addendum at A second, sentence-by-sentence drift pass over all 12 ADRs against
Where the earlier push already changed the same line, I kept that text. For the maintainer: the 0022 glance now says "pass-through or translation hops", but §2.5 (line 121) still says "collapses only the proven pass-through hops", which is the base wording. The base had this inconsistency. I left §2.5 unchanged because meaning is frozen. Please say if §2.5 should change too. Validation: |
There was a problem hiding this comment.
4 issues found across 11 files (changes from recent commits).
You’re at about 96% 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.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/adr/0033-collocation-decision-table.md">
<violation number="1" location="docs/adr/0033-collocation-decision-table.md:46">
P3: This row claims platform packages import `snapshot-freshness`, but no `packages/platform-*` source does so today. Replace the consumer list with the actual root command and daemon consumers, or update the imports before documenting that edge.</violation>
<violation number="2" location="docs/adr/0033-collocation-decision-table.md:157">
P2: The cycle rationale names `selectors`, but reversing the existing capture-kit dependency creates a direct daemon-contracts/capture-kit project-reference cycle. State that direct cycle so the collocation constraint remains verifiable against the manifests.</violation>
</file>
<file name="docs/adr/0027-descriptor-root-vs-eager-closure-budget.md">
<violation number="1" location="docs/adr/0027-descriptor-root-vs-eager-closure-budget.md:102">
P2: This sentence overstates the deleted-source guarantee: the gate detects only a two-statement verbatim move, and rewritten transplants can pass as novel. Qualify it as a detected verbatim transplant and state that the textual probe can miss rewritten code.
(Based on your team's feedback about the deleted-source match threshold.)</violation>
</file>
<file name="docs/adr/0025-foldable-apple-panels.md">
<violation number="1" location="docs/adr/0025-foldable-apple-panels.md:405">
P3: `recording-overlay.swift` bypasses export when there are no events, so it does not always use the geometry-preserving preset. Qualify this as applying to every composited export.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| | `src/runtime-command-surface.ts` | moved → `src/command-runtime/runtime-command-surface.ts` ([#3299](https://github.com/callstack/agent-device/pull/3299)) | Physical move landed with `runtime-factory.ts` as one assembly group: the command-surface binding sits beside the factory it composes, under the folder that derives its `command-runtime` zone, so `topFolder` replaces the per-file `ROOT_MODULE_ZONES` row. | | ||
| | `src/runtime-factory.ts` | moved → `src/command-runtime/runtime-factory.ts` ([#3299](https://github.com/callstack/agent-device/pull/3299)) | Physical move landed: the runtime assembly now lives under the folder that derives its `command-runtime` zone, with the same row deletion through folder-derived ownership. | | ||
| | `src/daemon-diagnostics-scope.ts` | moved → `packages/daemon-contracts/src/daemon-diagnostics-scope.ts` ([#3297](https://github.com/callstack/agent-device/pull/3297) into `src/daemon-contracts/`, then the package extraction) | The complete zone resides in `packages/daemon-contracts`. Its directory derives zone ownership without per-file `ROOT_MODULE_ZONES` rows. An internal rename therefore needs no ownership-table edit. | | ||
| | `packages/daemon-contracts/src/device-claim-recovery.ts` | keep (shared-below-both) | The owner-scoped claim reconciler recovers durable resources from a dead owner's state directory. Daemon `open`/startup and daemonless `device release --stale` both run it. It therefore belongs below both. Runtime recovery may reside in `daemon-contracts` only when both sides run it and root composition supplies only the gateway, per ADR 0019 section 1. The package depends on `capture-kit`. Capture-kit cannot depend back on it, because that dependency forms a project-reference cycle through `selectors`. | |
There was a problem hiding this comment.
P2: The cycle rationale names selectors, but reversing the existing capture-kit dependency creates a direct daemon-contracts/capture-kit project-reference cycle. State that direct cycle so the collocation constraint remains verifiable against the manifests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/adr/0033-collocation-decision-table.md, line 157:
<comment>The cycle rationale names `selectors`, but reversing the existing capture-kit dependency creates a direct daemon-contracts/capture-kit project-reference cycle. State that direct cycle so the collocation constraint remains verifiable against the manifests.</comment>
<file context>
@@ -153,15 +154,15 @@ They are not independent proof that the placement is optimal.
| --- | --- | --- |
| `src/daemon-diagnostics-scope.ts` | moved → `packages/daemon-contracts/src/daemon-diagnostics-scope.ts` ([#3297](https://github.com/callstack/agent-device/pull/3297) into `src/daemon-contracts/`, then the package extraction) | The complete zone resides in `packages/daemon-contracts`. Its directory derives zone ownership without per-file `ROOT_MODULE_ZONES` rows. An internal rename therefore needs no ownership-table edit. |
-| `packages/daemon-contracts/src/device-claim-recovery.ts` | keep (shared-below-both) | The owner-scoped reconciler recovers durable resources from a dead owner's state directory. Daemon `open`/startup and daemonless `device release --stale` both run it. It therefore belongs below both. Runtime recovery may reside in `daemon-contracts` only when both sides run it and root composition supplies only the gateway, per ADR 0019 section 1. The package depends on `capture-kit`. A reverse dependency forms a project-reference cycle through `selectors`. |
+| `packages/daemon-contracts/src/device-claim-recovery.ts` | keep (shared-below-both) | The owner-scoped claim reconciler recovers durable resources from a dead owner's state directory. Daemon `open`/startup and daemonless `device release --stale` both run it. It therefore belongs below both. Runtime recovery may reside in `daemon-contracts` only when both sides run it and root composition supplies only the gateway, per ADR 0019 section 1. The package depends on `capture-kit`. Capture-kit cannot depend back on it, because that dependency forms a project-reference cycle through `selectors`. |
| `src/daemon-policy-file.ts` | moved → `packages/daemon-contracts/src/daemon-policy-file.ts` | This shared daemon/client contract follows #2559 and #3288's `daemon-contracts` declaration. Rank-5 daemon-client dynamically imports it, so the spine requires a home below both. Package extraction supplies that physical home with manifest-owned exports. |
| `src/daemon.ts` | keep | This is the `internal/daemon` bundle entry. Umbrella §4 keeps composition roots at the root. |
</file context>
| | `packages/daemon-contracts/src/device-claim-recovery.ts` | keep (shared-below-both) | The owner-scoped claim reconciler recovers durable resources from a dead owner's state directory. Daemon `open`/startup and daemonless `device release --stale` both run it. It therefore belongs below both. Runtime recovery may reside in `daemon-contracts` only when both sides run it and root composition supplies only the gateway, per ADR 0019 section 1. The package depends on `capture-kit`. Capture-kit cannot depend back on it, because that dependency forms a project-reference cycle through `selectors`. | | |
| | `packages/daemon-contracts/src/device-claim-recovery.ts` | keep (shared-below-both) | The owner-scoped claim reconciler recovers durable resources from a dead owner's state directory. Daemon `open`/startup and daemonless `device release --stale` both run it. It therefore belongs below both. Runtime recovery may reside in `daemon-contracts` only when both sides run it and root composition supplies only the gateway, per ADR 0019 section 1. The package depends on `capture-kit`. Capture-kit cannot depend back on it, because that would create a direct project-reference cycle between `daemon-contracts` and `capture-kit`. | |
There was a problem hiding this comment.
Not changed here. The base row on main says the same thing: "a project-reference cycle through selectors". This PR keeps base meaning. If the manifests show a direct cycle, docs/adr-stale-facts (#3744) handles that correction.
| It must also carry no code from a source deleted since the merge-base. | ||
| The check matches an added module when it shares two or more identical normalized top-level statements with one deleted source; that is a move signal. |
There was a problem hiding this comment.
P2: This sentence overstates the deleted-source guarantee: the gate detects only a two-statement verbatim move, and rewritten transplants can pass as novel. Qualify it as a detected verbatim transplant and state that the textual probe can miss rewritten code.
(Based on your team's feedback about the deleted-source match threshold.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/adr/0027-descriptor-root-vs-eager-closure-budget.md, line 102:
<comment>This sentence overstates the deleted-source guarantee: the gate detects only a two-statement verbatim move, and rewritten transplants can pass as novel. Qualify it as a detected verbatim transplant and state that the textual probe can miss rewritten code.
(Based on your team's feedback about the deleted-source match threshold.) </comment>
<file context>
@@ -99,8 +99,8 @@ Three facts determine eligibility from the closure graphs and merge-base tree, w
Each module newly evaluated by the head is absent from the merge-base tree at its rename-canonicalized path.
- It must also contain no detected transplant from a source deleted since the merge-base.
- Two or more identical normalized top-level statements shared with one deleted source constitute a move signal.
+ It must also carry no code from a source deleted since the merge-base.
+ The check matches an added module when it shares two or more identical normalized top-level statements with one deleted source; that is a move signal.
One shared statement counts as coincidence.
</file context>
| It must also carry no code from a source deleted since the merge-base. | |
| The check matches an added module when it shares two or more identical normalized top-level statements with one deleted source; that is a move signal. | |
| It must also have no detected verbatim transplant from a source deleted since the merge-base. | |
| The check matches an added module when it shares two or more identical normalized top-level statements with one deleted source; rewritten transplants can evade this textual probe. |
There was a problem hiding this comment.
Not changed here. The base text on main makes the same claim ("carries no code from a source deleted since the merge-base"), and the next sentence already names the two-statement match. A change to the guarantee's wording is a factual correction for docs/adr-stale-facts (#3744).
| | `src/capture-admission/screen-recording-admission-ledger.ts` | keep (proposed move rejected) | The same direction is rejected. Capture-kit's screen-recording session resource and stop-recovery consume it. | | ||
| | `src/durable-json.ts` | keep (proposed move rejected) | Managed-allocation already imports this capture-kit module. Reversal forms a package cycle unless that import disappears first. A design that first removes that import stays open. Contracts beside `json.ts` was also rejected: under R18, contracts holds wire vocabulary, not durable-descriptor validation mechanics. Three managed-allocation `record-*` consumers explain the community coupling and can remain. | | ||
| | `src/snapshot/snapshot-evidence.ts` | keep | This is snapshot-capture mechanics. The daemon-dominated community represents consumers. A daemon-server home would force every platform and provider to depend on the daemon zone for capture evidence. | | ||
| | `src/snapshot/snapshot-freshness/index.ts` | keep | The reason for the row above applies. This is platform-neutral freshness policy. All its consumers, capture-kit itself, platforms, and daemon, import it downward today. | |
There was a problem hiding this comment.
P3: This row claims platform packages import snapshot-freshness, but no packages/platform-* source does so today. Replace the consumer list with the actual root command and daemon consumers, or update the imports before documenting that edge.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/adr/0033-collocation-decision-table.md, line 46:
<comment>This row claims platform packages import `snapshot-freshness`, but no `packages/platform-*` source does so today. Replace the consumer list with the actual root command and daemon consumers, or update the imports before documenting that edge.</comment>
<file context>
@@ -40,9 +41,9 @@ Re-evaluate those files against their remaining edges.
+| `src/durable-json.ts` | keep (proposed move rejected) | Managed-allocation already imports this capture-kit module. Reversal forms a package cycle unless that import disappears first. A design that first removes that import stays open. Contracts beside `json.ts` was also rejected: under R18, contracts holds wire vocabulary, not durable-descriptor validation mechanics. Three managed-allocation `record-*` consumers explain the community coupling and can remain. |
| `src/snapshot/snapshot-evidence.ts` | keep | This is snapshot-capture mechanics. The daemon-dominated community represents consumers. A daemon-server home would force every platform and provider to depend on the daemon zone for capture evidence. |
-| `src/snapshot/snapshot-freshness/index.ts` | keep | This is platform-neutral freshness policy. Capture-kit, platforms, and daemon currently import it downward. |
+| `src/snapshot/snapshot-freshness/index.ts` | keep | The reason for the row above applies. This is platform-neutral freshness policy. All its consumers, capture-kit itself, platforms, and daemon, import it downward today. |
| `src/snapshot/touch-reference-frame.ts` | keep | Interaction and capture share these snapshot-geometry mechanics. Only capture-kit is below both. |
</file context>
| | `src/snapshot/snapshot-freshness/index.ts` | keep | The reason for the row above applies. This is platform-neutral freshness policy. All its consumers, capture-kit itself, platforms, and daemon, import it downward today. | | |
| | `src/snapshot/snapshot-freshness/index.ts` | keep | The reason for the row above applies. This is platform-neutral freshness policy. Its cross-package consumers are the root capture command and daemon; capture-kit owns the implementation. | |
There was a problem hiding this comment.
Not changed here. The base row on main gives the same consumer list ("capture-kit itself, platforms, daemon"). If no packages/platform-* source imports it today, the fact is stale at base, and docs/adr-stale-facts (#3744) handles it.
| Hardware encoding makes full-resolution export inexpensive: approximately 1–2s for 90s at `1206x2622`. | ||
| Thus, `high` was not slower. | ||
|
|
||
| `recording-overlay.swift` now always exports through the geometry-preserving preset at both quality tiers. |
There was a problem hiding this comment.
P3: recording-overlay.swift bypasses export when there are no events, so it does not always use the geometry-preserving preset. Qualify this as applying to every composited export.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/adr/0025-foldable-apple-panels.md, line 405:
<comment>`recording-overlay.swift` bypasses export when there are no events, so it does not always use the geometry-preserving preset. Qualify this as applying to every composited export.</comment>
<file context>
@@ -399,7 +402,7 @@ Only `AVAssetExportPresetHighestQuality` preserves arbitrary geometry.
Thus, `high` was not slower.
-`recording-overlay.swift` now uses the geometry-preserving preset at both quality tiers.
+`recording-overlay.swift` now always exports through the geometry-preserving preset at both quality tiers.
`--quality` no longer reduces capture resolution.
Before adoption, it verifies that the composited track's resolved size matches the capture.
</file context>
| `recording-overlay.swift` now always exports through the geometry-preserving preset at both quality tiers. | |
| `recording-overlay.swift` uses the geometry-preserving preset for every composited export at both quality tiers. |
There was a problem hiding this comment.
Not changed here. The base text on main says "now always exports through the geometry-preserving preset". The rewrite had dropped "always", and the drift pass restored it. If the no-events bypass makes "always" wrong, docs/adr-stale-facts (#3744) handles that correction.
… poses Base: "Poses came from the hinge read-back, not from the pose that was requested; the amendment above is why that is the only defensible source". #3731 (comment)
|
Update at
Validation: |
|
At a2b6b0c the docs change looks good, and I found no new problems in the update since my earlier review. The earlier findings are fixed. CI is green, with 8 checks and none failing, and I know of no conflicts. The diff touches only docs/adr, so no code route is affected. I sampled the update in full and spot-checked the added facts against the base. I did not diff all of the roughly 5,000 lines, I did not run the layering or fallow gates, and I did not check the open facts against the code. The open inline threads still stand. The glance rule thread is still open: #3731 (comment). Four lower-priority threads also apply: 0025:139, 0033:157, 0027:103 and 0033:46. They are stale base facts, so they can wait for #3744. The 0025:405 thread also applies, where the update strengthened the wording to "always exports". Please fix that one in #3744 too. This thread no longer applies, so please resolve it: #3731 (comment) (0025 now says panel power cannot tell half-open from open). Until the threads that still apply are fixed here or moved to #3744 with a note on each, I am leaving the ready-for-human label off. |
Restore "seam" wherever the base used it; "boundary" stays only where the base used it. Restore that contracts holds wire vocabulary under R18, that exports", path-keyed rules as examples of a class, the rejected proposal as a hypothetical, and "the next pass re-opens it". Keep the Follow-ups deletion: #3281 and #3294 are closed and every listed move landed, so the suggested "moves continue under #3294" would be wrong.
Restore the shape rule in section 2 as a plain statement in force, including the timing "at the point the shape is requested"; #2808, #2806, and the web runtime tests already rely on it. The Status line keeps its "proposed" wording, so the conflict stays visible. Restore the operative half of the deleted section 6 substrate paragraph: no duplication or package-to-root back-import of shared Apple-runner or Android-helper mechanics, the injected-capability path, and one execution owner. Repoint the dangling substrate references in sections 6, 10, and the ADR 0009 relationship. Restore the Status outcome of the adoption checkpoint, the kernel helper scope, the only static value-import sites, the owning provider, owner cell, freezes, the time-bound await using ban, the trust choke point, "seam" as the code term, the conditional scope of the preferred/required rule, the loading shape, the "yet" qualifiers of the End state, and the declared host-port adapter.
- ADR 0025: emit apple_display_primary_geometry_conflict only when the primary panel is also the largest. - ADR 0025: keep the pre-panel normalization only when the runner reports no panel metadata. - ADR 0032: the revisit trigger is an engine that can supply the shared import model, not one that already supplies it. Found in review round 2.
The base decision applies the deletion test to pass-through and translation hops and endorses only the proven removable hop. The glance named only pass-through hops. Restore the selector "seam" term from the base rationale. #3731 (comment)
The base text says the row and Batch 1 record a move that landed in #3287 and that the package removal then deleted. The rewrite dropped that the move landed. #3731 (comment)
The Device Hub deletion also removed the base sentence that fold sets the pose and that only the read-back is evidence. Point to the HID amendment that replaced the Device Hub press. #3731 (comment)
Name pass-through and translation hops in one rule, as the base deletion test does, and drop the commit hash that the base does not attach to the verdict. #3731 (comment)
ADR 0022 records that #3430 emptied the R76 inventory and reduced R76 to a plain ban on the src/platform-runtime-*.ts family. The ban still keys on these paths, so the retain-for-this-batch rationale holds; only the noun was stale. #3731 (comment)
…oved The row is history: the move landed in #3287, and the package removal on 2026-10-10 deleted the file. State this in the row, as the Status note does, so the row does not read as a live move target. #3731 (comment)
Restore base scope dropped in the rewrite: "design, not a move" for the limrun rows, the shared reason and "all" for snapshot-freshness, the fingerprint and policy-file links, the real-seam conditions, "every edge" and "at all" for client imports, "the platform package's", the capture-kit back-edge, R11's "from a non-test file", and the daemon entry's re-export scope.
Restore "provider" and "concrete" in the bindDevice glance, the conditional parity evidence that explains why, "touches" for the SessionState unit, "not pass-through layers" for the declared boundaries, and "grab bag". Restore the operative ban that temporary fixtures never authorize a production bridge, duplicate route, or package back-import; the section 7 deletion removed it.
ADR 0021: restore the two "so" consequences. ADR 0023: name `isEligible` as the fallback condition, and describe the adb-host binding instead of a permission. ADR 0025: limit "never promoted to a verified pose" to the `screen` evidence. ADR 0026: name the ledger as the shape to extend, not an order. ADR 0027: restore "carry no code", "new code", and "all". ADR 0029: restore the "HTTP edge" term. ADR 0032: restore "only", "one more", the "and" between edge kinds, and the "because" link.
Found by the second meaning-drift pass against base 59fdf00.
Found by the second meaning-drift pass against base 59fdf00.
Found by the second meaning-drift pass against base 59fdf00.
Found by the second meaning-drift pass against base 59fdf00.
Found by the second meaning-drift pass against base 59fdf00.
Found by the second meaning-drift pass against base 59fdf00.
Found by the second meaning-drift pass against base 59fdf00.
Found by the second meaning-drift pass against base 59fdf00.
Found by the second meaning-drift pass against base 59fdf00. Also repair the review instruction that pointed at the deleted substrate paragraph.
Base: "device claims still guard against two sessions ... driving the same device". "prevent" claimed a stronger guarantee.
… poses Base: "Poses came from the hinge read-back, not from the pose that was requested; the amendment above is why that is the only defensible source". #3731 (comment)
a2b6b0c to
17d2102
Compare
|
Rebased onto Conflicts. Only Comments. No new items on any surface. The 7 open threads stay as they are: stale base facts for the fact-correction PR, per the maintainer's note. Verification. |
|
I reviewed 17d2102. The earlier findings from a2b6b0c are fixed, and CI is green: 8 checks pass, and this PR changes only docs, so no job exercises code it touches. There are no conflicts. One item still needs a fix: the snapshot-freshness row at docs/adr/0033-collocation-decision-table.md:46 says platforms import it downward, but grep finds no platform importer. List the real consumers (daemon and src/commands), or state only the downward-import rule. Fix it here or defer it explicitly to #3744. The open threads on 0027:98-106 (overstated gate guarantee, #3731 (comment)) and 0033:46 (#3731 (comment)) still apply, and the second is the item above. One lower-priority thread on 0025:405 (#3731 (comment)) also still applies. Three threads look like stale facts from the base, not changes in this PR: 0025 glance rule (#3731 (comment)), 0025:139 refusal paths (#3731 (comment)) and 0033:157 selectors cycle (#3731 (comment)). I did not check them against the Swift or tsconfig sources, so please resolve them or defer them to #3744. I spot-checked only the passages the threads cite. I did not diff all 12 rewritten ADRs against the base for meaning. Before merge, fix 0033:46 here and resolve or defer those threads. |
|
At All six unresolved fact threads are explicitly deferred to the stale-facts PR under #3744:
This includes the line-46 item in the latest review. The maintainer assigned these corrections to |
|
Thanks. The items left from my review of 17d2102 (the snapshot-freshness consumers at 0033:46, the 0027 gate guarantee, and the three base-era fact threads) are now explicitly deferred to the stale-facts follow-up under #3744, which the maintainer assigned. That settles them for this PR. This is a docs-only change, all checks pass, and there are no conflicts, so it is ready for human review. |
thymikee
added
the
ready-for-human
This branch has not been deployed
1 participant
Summary
Rewrite 12 runtime/architecture ADRs in ASD-STE100 with frozen meaning. Delete only completed history. Review round 1 fixups restore drifted meaning per ADR, plus operative content: the ADR 0019 §8 size rule, the §6 shared-mechanics rule, the ADR 0025 physical-device gap, and the ADR 0023 comparability sentence.
Ambiguities
Discrepancies found, not changed
Older problems at base
src/__tests__/owns shared install-source tests".snapshot-handler.fixtures.ts:101says "at most one bind per request"; §9 allows the--settlesecond bind.display-inventory.ts; the runner has three typed capture failures, not one. Left fordocs/adr-stale-facts(Restructure ADRs into short decision records #3744).Validation
pnpm check:affected --run --head 0d4137ca5b: passed,exactCommit=true, 0 checks (ADR-only).4577385f87. Round 3 (maintainer items plus a 12-ADR drift pass): 0 high, about 35 low; fixed. Three threads stay open fordocs/adr-stale-facts.