Repository navigation
Conversation
Size Report
Startup median (7 runs, lower is better):
|
33d14c8 to
cf03d03
Compare
cf03d03 to
7a799d9
Compare
thymikee
changed the title
7a799d9 to
08ce68f
Compare
|
I reviewed 08ce68f. The code has two problems to fix before merge. CI is green at this head: the 12 non-passing jobs in the first run were cancelled, and the same jobs pass in the re-runs. Smoke Tests and the Mutants matrix were skipped, so they give no device signal. There are no conflicts. The Maestro help still tells agents to install only Native Not blocking, take or leave: This review is from reading the code. I did not run tests or the packed smoke, and I did not reproduce the earlier-commit claims (tree identity with 7a799d9, 1,714 focused tests, seven packed checks, 31 repaired hosted cases). I did not trace the composite-resource-admission fixture, the CI gate action changes or the install-workspace-replay-plugins script in depth. Whether ADR 0034's proposed native amendment is accepted is a maintainer decision outside this review. Before merge, the Maestro help must state the |
08ce68f to
67c0e73
Compare
|
Thanks for the update. I re-read the changes at 67c0e73 against the earlier review (#4537 (comment)). Two findings from that review remain, and the other earlier findings are now addressed. CI is green. The 12 jobs that first showed as not passing were cancelled runs, and all 12 passed in their re-runs. Smoke Tests and the Mutants matrix were skipped, so CI gives no device signal for the plugin execution route. I reviewed by reading the code. I did not run the new daemon-composition-replay-selection tests or the packed smoke. I reasoned about the four custom replace/remove cases from the earlier code path and did not run them against the old code. No conflicts. The Maestro help still tells agents to install only No run yet shows a device-facing route working through the plugin-provided engine (native-command.ts#L91). The PR body still lists live Android and iOS verification as pending. Please install Before merge, the Maestro help must name |
67c0e73 to
5fe9441
Compare
|
Thanks for the update. At 651a68b, two findings from the earlier review (#4537 (comment)) are still open: the The Native replay and test suites now route through the plugin execution (native-command.ts#L91, test-command.ts:199). The PR body claims no device run, and Smoke Tests were skipped. Nothing yet shows that a device replay works through the plugin engine, or that the plugin-absent refusal happens before a session exists. Please attach these runs: (1) Not blocking, take or leave: I read the code and did not run any tests, including command-tools-parity, cli-startup-import-closure, the installed-package-metro consumer, or the packed smokes. I did not re-trace the client-side Maestro collection route or the "core clients can send .ad sources" sentence, since they moved verbatim and the earlier review covered them. I compared the help text before and after by reading it, not by a run. |
|
Fixed the Maestro prerequisite finding in e567c5b. The existing help test failed on the missing native install command before the fix. Committed focused results: 1,019 passed across eight files, including all eager-closure roots and MCP text policy. Format, lint/typecheck, build and layering passed. Seven actual packed checks passed. The new Maestro-only packed embedded request uses the real factory and current client: {"code":"INVALID_ARGS","details":{"reason":"plugin-not-installed","package":"@agent-device/replay","installCommand":"agent-device plugins add @agent-device/replay"}}Sessions are empty before and after. This is a device-free packed embedded admission check, not an installed-store live device run. Client source collection may create Maestro's backend before daemon admission; I removed an incorrect test assumption that no backend could be created. No execution or session success is inferred from collection. For the outstanding live requests, here are actual retained outputs at 67c0e73, not a new execution at e567: {"backend":"ad","source":"store","package":"@agent-device/replay","version":"0.0.0","state":"active"}
{"nativeResume":{"success":true,"replayed":14,"healed":0,"sessionActive":false}}
{"nativeSuite":{"passed":0,"failed":1,"attempts":1}}
{"maestroReplay":{"success":true,"replayed":5,"healed":0,"sessionActive":true}}The native package was explicitly installed from its real tarball (SHA256 Android native replay and a passing two-test retry suite remain unproven. Recording's HTTP restriction exists in current code as well as the old bound client. I have not bypassed T3 with an unbound device CLI, retried stopped device paths, changed assertions or called these partial results full clearance. Evidence: |
![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 69 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Apex Sentinel · ✅ ApproveSince the last review: 0 new · 1 still open · 0 no longer raised 1 finding(s): 1 minor. 🟡 Minor (1)
Not in the reviewers' prompt (1 file)
Reviewed with this repository's own Sentinel configuration ( 💬 Talking to SentinelIn a finding's thread, reply with:
On this conversation, comment Earlier reviewsReview of
|
| Where | Finding |
|---|---|
packages/replay-port/src/daemon-port/replay-plugin-admission.ts:20 |
Missing replay plugin still hides the "requires explicit --maestro routing" hint for .yaml input |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
2 findings below minor hidden (user_settings.min_severity).
Reviewed with this repository's own Sentinel configuration (d38e35e61193).
Review of e567c5ba · 14.11.2026 08:22 UTC
Apex Sentinel · ✅ Approve
1 finding(s): 1 minor.
🟡 Minor (1)
| Where | Finding |
|---|---|
packages/replay-port/src/daemon-port/replay-plugin-admission.ts:20 |
Missing replay plugin hides the "requires explicit --maestro routing" hint for .yaml input |
Not in the reviewers' prompt (1 file)
pnpm-lock.yaml: generated or a lockfile, so its lines were left out
1 finding below minor hidden (user_settings.min_severity).
Reviewed with this repository's own Sentinel configuration (d38e35e61193).
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 1 new finding on e567c5b. The verdict and the full list are in the summary comment.
| } else if (request.command === 'replay') { | ||
| const entry = request.flags?.replayScriptSource?.entry ?? request.positionals?.[0] ?? ''; | ||
| if (resolveReplayFormat(entry, request.flags?.replayBackend) === 'maestro') | ||
| await getReplayBackend('maestro'); | ||
| else await getReplayExecution(); | ||
| } |
There was a problem hiding this comment.
🟡 Minor · Missing replay plugin still hides the explicit --maestro routing hint for .yaml test inputs
test suite.yaml (no --maestro) against a daemon without @agent-device/replay now returns plugin-not-installed instead of the Maestro YAML requires explicit --maestro routing refusal. getReplayExecution() is awaited before the format check, and resolveReplayFormat(entry, flags.replayBackend) returns 'ad' for every path when replayBackend is absent, so the Maestro routing message is unreachable. No test in request-router-replay-plugin.test.ts pins the test case.
Suggested change
In the test branch, check isMaestroYamlPath over replayScriptSources entries and raise maestroBackendRequiredMessage('test', entry) before awaiting getReplayExecution(), mirroring the replay branch; add the test <flow.yaml> case to request-router-replay-plugin.test.ts.
if (request.command === 'test') {
const entry = request.flags?.replayScriptSources?.find((source) =>
isMaestroYamlPath(source.entry),
)?.entry;
if (entry !== undefined && backend !== 'maestro')
throw new AppError('INVALID_ARGS', maestroBackendRequiredMessage('test', entry));
await getReplayExecution();
if (
request.flags?.replayScriptSources?.some(
(source) => resolveReplayFormat(source.entry, request.flags?.replayBackend) === 'maestro',
)
)
await getReplayBackend('maestro');
} else if (request.command === 'replay') {
Details
persona: senior-developer, maintainability
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if it is wrong — I will re-check the cited code.
There was a problem hiding this comment.
Fixed in 3c32e14. Admission now returns the existing Maestro routing INVALID_ARGS error for bare YAML before any plugin lookup or device selection. It uses the canonical path predicate and message; native .ad admission and explicit Maestro plugin requirements remain intact. The YAML/YML router regressions first failed with native-plugin installation advice and now pass, with no discovery or session creation. Combined focused checks pass (58 cases); new-head broad validation and independent review remain pending.
There was a problem hiding this comment.
This is valid at published 3c32e14e0046147ac31a63ab41a978edf0091970. I reopened the thread when the trigger changed from replay flow.yaml to raw test suite.yaml. Local 651a25a22a071f87e10122cebefca73d3c9b8fd5 checks serialized suite entries with the existing YAML-path and canonical routing-message owners before getReplayExecution().
The isolated unchanged-test experiment is complete: published admission bytes produced two intended failures for YAML/YML test requests; fixed bytes passed both. The full router file passed 11 cases, with no device discovery or session creation. Native .ad admission, explicitly routed Maestro suites’ native scheduler requirement, and Maestro-only replay remain covered. Typecheck, lint, full format and layering passed. These are router-fixture results, not live-device proof.
The identical admission blob is in review-only c9fc5f653aaa730d6793351b63b40e1765c1f821, whose independent source recheck completed with no findings. This repeated notification adds no new trigger. The source is not pushed; independent review, publication and new-head gates remain pending. Leaving the thread open until the reviewed correction is published and verified. No duplicate implementation or test rerun is needed.
|
Both new device-job failures are now traced to missing plugin installation, not assumed infrastructure faults:
The C3 Android workflow fix is locally committed: select an isolated AGENT_DEVICE_HOME and invoke its existing built-workspace installer after build and before daemon startup. When restacked here, that call uses the existing combined replay-plugin installer. I also prepared Apple workflow setup after successful shared build, exporting the same isolated plugin home to subsequent iOS/macOS phases. Linux already routes execution through run-gate, whose declared execution-gate setup installs the real plugins; no Linux change is proposed. No scenario/assertion/timeout/selector fallback or product auto-install is added. Separately, the exact e567 local affected and full gates both passed once: 42/25 and 41/25, exactCommit=true, checkoutChanged=false, clean fingerprints. I independently verified both raw receipts and 113 original/copy SHA-256 pairs in the immutable report. These do not clear either hosted device failure. Corrections will be published with the coordinated stack refresh after the integration candidate is validated and the active source review is preserved. The current failed jobs will not be blindly rerun. This PR stays ready for review. |
|
I reviewed e567c5b against the earlier findings (#4537 (comment)). The smoke jobs still fail because of this stack, and the native device evidence is still missing. The smoke jobs start a daemon with an empty plugin store, but replay and test now need the installed plugins ( The second finding from the earlier review still stands. Native replay and test suites now run through the @agent-device/replay plugin (https://github.com/callstack/agent-device/blob/e567c5b/packages/replay-port/src/daemon-port/native-command.ts#L91). The only live runs I could see are iOS-only partial results at 67c0e73: the full replay failed at Catalog selection, the suite passed 0 and failed 1, and only a Not blocking: the help path at cli-help.ts:887 passes the same ad installation twice to Open threads still apply on #4537 (comment) (private package, docs install) and #4537 (comment) (ADR 0016 scope). Eight lower-priority threads also still apply. One thread does not apply, so please resolve it: #4537 (comment) (the empty backend is consistently treated as native). CI looks related to this change, since the failing jobs run replay and test on a booted device. No conflicts. Before merge, the shared plugin install needs to land with the smoke jobs green, and the live Android and iOS runs need to be attached. |
e567c5b to
153a127
Compare
There was a problem hiding this comment.
1 issue found across 20 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=".github/workflows/apple.yml">
<violation number="1" location=".github/workflows/apple.yml:88">
P3: The macOS matrix lane rebuilds and reinstalls both replay plugins three times: once here, then again for `macos-coverage` and `replay-macos` through `run-gate`. Restrict this installation to the iOS matrix, where the smoke action invokes the CLI directly.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| gate: build | ||
| - if: steps.build.outcome == 'success' | ||
| run: node --experimental-strip-types scripts/install-workspace-maestro.ts | ||
| run: node --experimental-strip-types scripts/install-workspace-replay-plugins.ts |
There was a problem hiding this comment.
P3: The macOS matrix lane rebuilds and reinstalls both replay plugins three times: once here, then again for macos-coverage and replay-macos through run-gate. Restrict this installation to the iOS matrix, where the smoke action invokes the CLI directly.
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 .github/workflows/apple.yml, line 88:
<comment>The macOS matrix lane rebuilds and reinstalls both replay plugins three times: once here, then again for `macos-coverage` and `replay-macos` through `run-gate`. Restrict this installation to the iOS matrix, where the smoke action invokes the CLI directly.</comment>
<file context>
@@ -85,7 +85,7 @@ jobs:
gate: build
- if: steps.build.outcome == 'success'
- run: node --experimental-strip-types scripts/install-workspace-maestro.ts
+ run: node --experimental-strip-types scripts/install-workspace-replay-plugins.ts
- name: iOS smoke tests
id: ios
</file context>
There was a problem hiding this comment.
Valid: the macOS composite invokes run-gate for both macos-coverage and replay-macos, and those gates each own plugin setup. I prepared fe4fbbf30f14bc5f86e5cfe944efe8dda8527994 to restrict the workflow-level installer to the iOS matrix after a successful build. iOS still runs the CLI directly; its job-scoped plugin home is preserved. Full-repository formatting and parsed workflow/owner checks pass. The fix is handed to the active local preparation/restack worker; it is not published yet, so this thread remains open until the updated source is verified. No native/CI clearance is implied.
153a127 to
3c32e14
Compare
There was a problem hiding this comment.
1 issue found across 2 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="packages/replay-port/src/daemon-port/replay-plugin-admission.ts">
<violation number="1" location="packages/replay-port/src/daemon-port/replay-plugin-admission.ts:27">
P2: This guard fixes unrouted YAML only for `replay`; a raw `test` request with a YAML source and no `replayBackend` still calls `getReplayExecution()` and admits the native plugin before the nested replay can reject it. Validate every `replayScriptSources` YAML entry with `maestroBackendRequiredMessage('test', entry)` before test plugin admission.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| await getReplayBackend('maestro'); | ||
| } else if (request.command === 'replay') { | ||
| const entry = request.flags?.replayScriptSource?.entry ?? request.positionals?.[0] ?? ''; | ||
| if (isMaestroYamlPath(entry) && backend !== 'maestro') |
There was a problem hiding this comment.
P2: This guard fixes unrouted YAML only for replay; a raw test request with a YAML source and no replayBackend still calls getReplayExecution() and admits the native plugin before the nested replay can reject it. Validate every replayScriptSources YAML entry with maestroBackendRequiredMessage('test', entry) before test plugin admission.
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 packages/replay-port/src/daemon-port/replay-plugin-admission.ts, line 27:
<comment>This guard fixes unrouted YAML only for `replay`; a raw `test` request with a YAML source and no `replayBackend` still calls `getReplayExecution()` and admits the native plugin before the nested replay can reject it. Validate every `replayScriptSources` YAML entry with `maestroBackendRequiredMessage('test', entry)` before test plugin admission.</comment>
<file context>
@@ -19,6 +24,8 @@ export async function requireReplayPlugins(request: DaemonWireRequest): Promise<
await getReplayBackend('maestro');
} else if (request.command === 'replay') {
const entry = request.flags?.replayScriptSource?.entry ?? request.positionals?.[0] ?? '';
+ if (isMaestroYamlPath(entry) && backend !== 'maestro')
+ throw new AppError('INVALID_ARGS', maestroBackendRequiredMessage('replay', entry));
if (resolveReplayFormat(entry, request.flags?.replayBackend) === 'maestro')
</file context>
There was a problem hiding this comment.
Valid at published 3c32e14e0046147ac31a63ab41a978edf0091970. This is the same raw-suite admission gap already identified in Sentinel thread #discussion_r4572549692. It is fixed locally in 651a25a22a071f87e10122cebefca73d3c9b8fd5: any YAML/YML entry in replayScriptSources requires explicit Maestro routing before getReplayExecution(). The shared canonical test … --maestro refusal is preserved; native .ad and explicitly routed Maestro suite prerequisites remain unchanged.
The isolated unchanged-test experiment produced two intended failures with the published admission code, then two passes with the fix; the complete router file passed all 11 cases. Device discovery was not called and no session was created. This is router-fixture evidence, not live execution. The identical admission blob is already included in review-only c9fc5f653aaa730d6793351b63b40e1765c1f821, whose independent source recheck completed with no findings. The fix is not pushed yet. I’m leaving this thread open until the reviewed correction is published and verified; no second implementation or duplicate validation is needed.
|
Thanks for the update. At 3c32e14 the code looks right, but I still can't see live proof for the new route. I read the code and did not run any test, gate, installer or packed smoke. Since the earlier review (#4537 (comment)), the docs now say both plugin packages are private and unpublished, the ADR scope and the Maestro requirement are fixed, the empty backend is refused, and the nightly and prepack install steps are fixed. Two points remain: live validation and the CI installer step. Native replay and every test suite now run only through the installed The previous review asked for one owned installer step, but the delta added inline installer calls at android.yml#L96 and apple.yml#L88. This copy-per-site approach already misses a site. The Not blocking, and you can take or leave these: the unrouted-YAML refusal now exists in three places with the same predicate and message (admission.ts#L27, native-command.ts:81-82, source-discovery.ts:87-88), and Could both collapses be done together? (a) One ad-script function returns the route or throws the typed routing error (unknown or empty backend, YAML without All 13 non-passing checks at 3c32e14 are cancelled, not failed, so there is no failure to attribute. The delta changes the route that Smoke Tests exercise, so those jobs need to rerun to completion before the smoke lanes count as fixed. I did not check whether the cancellations came from a newer push or a manual cancel. I also did not trace every gate id to confirm the run-gate install list covers all gates that start a daemon; I only grepped workflow and script call sites. There are no conflicts. Before merge, please attach the live Android and iOS plugin-route evidence at this head with Smoke Tests (Android) and (ios) green, and fold the per-workflow installer copies into one owned step that also covers On the open inline threads: the thread on redundant macOS installs still holds (#4537 (comment)), and it folds into the single installer step above. No other blocking thread still applies. These threads do not apply and can be resolved: admission order for suites, because every suite needs |
Summary
Advance #3871: make native replay/test optional through the managed registry while retaining core lifecycle ownership. Installation guidance, atomic smoke-fixture setup and packed public types use existing owners.
Bare YAML now reports the required --maestro routing before plugin lookup or device selection, instead of demanding the unrelated native plugin. 74 files; 1,840 gross lines against #4535.
Example:
agent-device plugins add @agent-device/replay; Maestro suites additionally need @agent-device/maestro. Both 0.22 packages remain private and unpublished.Validation
The updated Sentinel suite-YAML trigger is valid. Local
651a25a22a071f87e10122cebefca73d3c9b8fd5fixes its admission order; isolated red/green and all 11 router cases passed. Both fixes together passed 27 focused cases at review-onlyc9fc5f653a, with fresh setup/build/typecheck/lint/format. Independent source recheck passed atc9; publication remains pending. Previous source approval retains its original scope.Published
3c32e14e0046147ac31a63ab41a978edf0091970, base894007501dd05245beb766dc36c4415e0f5c065a. All fifteen prior top patches remain unchanged through range-diff; an independent tree reconstruction matches exactly. Pinned pnpm 11.17 frozen install/build, full format, typecheck, lint and layering passed; 58 focused cases passed. New YAML/YML routing regressions first failed with native-plugin advice; native missing-plugin and explicit Maestro controls pass.Independent Apex source/prose recheck found no regressions and sustained the ownership design. C3 scope remains over the limit. New-head exact affected/full gates and hosted CI remain pending. Previous exact and packed-consumer receipts remain historical evidence.
Merge blockers: C3 prerequisites/integration; representative full native replay/suite success; recording HTTP contract; platform/remote/live and performance qualifications; maintainer native-contract acceptance and npm placeholder/trusted publisher setup. Ready for review; no merge clearance.