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 32 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="scripts/__tests__/eager-closure-budgets.ts">
<violation number="1" location="scripts/__tests__/eager-closure-budgets.ts:140">
P3: This allowance row is authored in the same PR that uses it, and the gate only checks that an `owner` string exists. Please record the owner's sign-off before merge, for example in the PR approval, since the diff itself does not show it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| issue: '#3377', | ||
| reason: | ||
| 'The Maestro replay backend plugin IS the engine behind `replay --maestro`; the registry reaches it only through a function-scoped import(), and no startup hub evaluates it (cli-startup-import-closure pins that).', | ||
| owner: 'thymikee', |
There was a problem hiding this comment.
P3: This allowance row is authored in the same PR that uses it, and the gate only checks that an owner string exists. Please record the owner's sign-off before merge, for example in the PR approval, since the diff itself does not show it.
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 scripts/__tests__/eager-closure-budgets.ts, line 140:
<comment>This allowance row is authored in the same PR that uses it, and the gate only checks that an `owner` string exists. Please record the owner's sign-off before merge, for example in the PR approval, since the diff itself does not show it.</comment>
<file context>
@@ -132,7 +132,14 @@ export const NEW_ENTRY_CEILINGS: Readonly<Record<EntryCategory, number>> = Objec
+ issue: '#3377',
+ reason:
+ 'The Maestro replay backend plugin IS the engine behind `replay --maestro`; the registry reaches it only through a function-scoped import(), and no startup hub evaluates it (cli-startup-import-closure pins that).',
+ owner: 'thymikee',
+ },
+});
</file context>
There was a problem hiding this comment.
No code change: maintainer approval of this PR is the sign-off for the APPROVED_OVER_CEILING row. Leaving this thread open so thymikee sees it.
|
The design holds at 5352eef, and one small test fix is needed before merge: the first inline thread below, where a test name promises suggestNodes but the test never calls it. CI is still pending: all 17 lanes were queued or running and none had failed. The lanes this diff touches are Typecheck & Package, Repo Guards and Integration/Smoke, and they decide whether it can merge. I read the diff but did not run the test suites or the closure-budget and layering gates, and I did not measure the eager-closure size of replay-maestro-backend.ts. I also did not check the CLI startup closure numerically after cli-help moved to replay-port/replay-backend-declarations, though reading the imports it should shrink. I checked the moved tests by reading the diff, not with Not blocking: the host builds Three inline threads on this PR still apply. The first is the test name that promises suggestNodes. The daemon test at lines 342-475 covers the route end to end, so either assert the suggestion source or drop it from the name. The second is the lost first-event ordering check, where Before merge, the CI lanes above need to go green and thymikee needs to approve the APPROVED_OVER_CEILING row. After that, retarget the PR to main once #3622 merges. No conflicts. |
Size Report
Startup median (7 runs, lower is better):
|
|
5352eef to
be07f5b
Compare
|
Both non-blocking notes taken in be07f5b: the host's |
be07f5b to
a12c6ab
Compare
|
The Maestro replay backend now has the create(host) plugin shape. The code at a12c6ab is clean: I found no new problems since the earlier review (#3623 (comment)), and the findings from 5352eef are fixed. I read the code. I did not run the unit suites, the eager-closure budget gate, the layering gate or the startup-closure test, and I did not measure the eager-closure size of replay-maestro-backend.ts, so the over-ceiling margin is unverified. I would like those results before this counts as proven. The Smoke Tests lane failed in the iOS runner XCTest RunnerTests.testNonReadCommandStillLaunchesANotRunningSessionApp, because the fixture /tmp/agent-device-inject-tap-recorded-failure-for-testing was missing. This looks unrelated: the diff from e59514f to a12c6ab has no Swift, runner or Apple files, and the PR only changes Maestro replay packaging. I judged this only from that lack of overlap. I did not compare it with runs of the same test on main. Please rerun the lane. One question on simplicity. Nothing reads ReplayBackendDeclaration.apiVersion, and only provider plugins validate theirs (src/plugins/manifest.ts:63). The registry never reads ReplayBackendPlugin.declaration, because help uses the separate REPLAY_BACKEND_DECLARATIONS table. Should apiVersion and plugin.declaration wait for the installed-plugin loader that would check them? Or should the registry be the one reader: derive the help table from the plugins' declarations and assert that declaration.id and backend.id match the registry key? Either way, the id and the declaration would no longer be registered in parallel with nothing keeping them in step. Of the open review threads, the cubic-dev-ai P3 on scripts/tests/eager-closure-budgets.ts:136-141 still holds (#3623 (comment)). The owner row is self-authored, so it needs no code change, only the owner's approval. The reason text is now true, because isLazyEngineModule covers the plugin subpath. The other two threads are fixed at this head and can be resolved: the failure-suggestion test now compares against collectMaestroFailureSuggestions and asserts a non-empty result (#3623 (comment)), and the observer test uses toHaveBeenNthCalledWith(1, ...) (#3623 (comment)). No conflicts are known. To merge, the Smoke Tests lane needs a green rerun, the owner must approve the APPROVED_OVER_CEILING row, and the PR must be retargeted to main after #3622 merges. |
This branch has not been deployed
1 participant
Summary
Second half of the #3377 replay-backend prep. Stacks on #3622 (base is its branch; retarget to
mainonce it merges). Behavior, wire shapes, help text and error wording are unchanged; the backend still resolves once per process (ADR 0015).packages/maestro/src/replay-backend/and ships asReplayBackendPlugin = { declaration, create(host) }(@agent-device/maestro/replay-backend), with an engine-free declaration (.../replay-backend-declaration).ReplayBackendHost(built inreplay-backend-host.ts) is the only way the adapter reaches process state or daemon policy: trace append, builtin vars, script-source read, target-device resolution, request signal, and the divergence-failure builder from refactor(replay-port): own the replay backend contract in contracts #3622.create(host)once.replay-backend-declarations.tsholds the declaration table, so the registry's eager closure does not grow.help maestroreads its topic from the declaration, socli-helpno longer imports the engine.getReplayBackend('maestro'); two warnings checks became unit tests../daemon-runtime-portexport.@agent-device/maestroin production (scripts/fuzzstill does).32 files touched. Sign-off needed: the
APPROVED_OVER_CEILINGrow forreplay-maestro-backend.tsnames thymikee as owner, so it needs his approval.Refs #3377.
Validation
Tested SHA: a12c6ab.
pnpm check:affected --base <layer-1 branch> --run: all runnable checks passed.pnpm check:quickand focused vitest (replay-port, maestro, daemon sessions, CLI help/MCP, startup closure, closure budgets) passed.-M90%stat shows more because several moved files are 80–91% similar after the host rewrite.maestro-differentialare CI-authoritative.🤖 Generated with Claude Code