Repository navigation
Conversation
06b89da to
f21198a
Compare
Size Report
Startup median (7 runs, lower is better):
|
|
I found nothing that needs a fix in f21198a, but this review is not a full check. I did not run tsc, depgraph or check:di-seams, so the zero-edge depgraph claim rests on my read of the PR head, not on the graph. The injected value is the same createCommandSurfaceAgentDevice function the old imports supplied, called with the same config, so I did not ask for a live device run. I did not read all 143 test bag edits one by one. I filtered the diff for non-fixture lines and found only a reformat. The 14 checks are green and none fail. I know of no conflicts. The base is the stacked branch refactor/command-runtime-contracts (#3394), which is still open, and this review assumes #3394 lands as reviewed. This PR can merge after #3394 merges and it is rebased onto main. Not blocking: the port in daemon-command-surface.ts re-declares option shapes that src/commands already owns, such as DaemonWaitTarget, DaemonSelectorSnapshotOptions and the gesture and press/fill/longPress options. Each shape now has two declarations, and only the assignability check in daemon.ts ties them together. The rule would be one declaration per command option or result shape, in contracts, with src/commands importing it. Also, the members of DaemonCommandSurfaceCapture, DaemonCommandSurfaceSelectors and DaemonCommandSurfaceInteractions use method shorthand, which TypeScript checks bivariantly (see line 300). A required field added to the implementation's options would still compile, which weakens the header claim at lines 1-4. Function-typed properties would make strictFunctionTypes check them. Take or leave both. Is the 357-line contracts mirror the part to cut? The threading itself matches how RequestRouterDeps already carries androidObservation and platformResourceCleanup. I found no single per-request bag that reaches all four construction sites, so I accept it at 433 net production lines. If the command option and result types the daemon calls (wait, find/get/is, press/fill/longPress/hover, gesture, screenshot, snapshot) moved into @agent-device/contracts, the port could be about 30 lines of interface over shared types. That would be a follow-up type move like #3394, not part of this PR. |
f21198a to
d297702
Compare
|
Rebased onto the rebased Eager closure fix. The static Function-typed members (taken). Type mirror (deferred). Agreeing: one declaration per command option/result shape, in contracts, with No new production Stays draft until #3394 merges. |
|
Follow-up on the eager-closure fix (b617d12): the startup-await variant still evaluated the whole surface on every daemon start. |
|
The code in b617d12 looks good to me, and the earlier open question from f21198a is now resolved. The only evidence still outstanding is the production route. Smoke Tests are queued and they are the only check that reaches it. They spawn the built daemon, so the first surface command runs the composition-root lambda in src/daemon.ts and its dynamic import from dist. No unit test calls that lambda, because every test bag uses the static import in command-surface-fixture.ts. I did not run tsc, the eager-closure budget test or depgraph, so the 550-to-470 closure count and the zero-edge claim rest on your report and on my read of the import sites. CodeQL Analyze is also pending, and this change has no data-flow overlap with it. I know of no conflicts. Before merge, Smoke Tests must go green on b617d12, and #3394 must merge so this PR can be rebased onto main. |
thymikee
added
the
ready-for-human
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 175 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="src/daemon.ts">
<violation number="1" location="src/daemon.ts:5">
P3: This comment restates the dynamic-import behavior shown immediately below, contrary to `AGENTS.md`'s no-control-flow-narration rule. Replace it with the non-obvious reason for keeping this import lazy.</violation>
</file>
<file name="src/daemon/interaction/internal/interaction-touch-runtime.ts">
<violation number="1" location="src/daemon/interaction/internal/interaction-touch-runtime.ts:75">
P3: `actionStartedAt` is now stamped *after* the awaited port call on line 75, so the first-use command-surface module load (the `await import` in `src/daemon.ts`) is excluded from the touch action's recorded span — while `dispatchGestureInteraction` stamps its start at function entry and includes the same load. First-request durations for touch vs gesture interactions are therefore not comparable. Move `const actionStartedAt = Date.now();` to before the runtime construction (matching the gesture path) if the span is meant to cover the whole request-side interaction work.</violation>
</file>
<file name="packages/contracts/src/daemon-command-surface.ts">
<violation number="1" location="packages/contracts/src/daemon-command-surface.ts:180">
P2: `DaemonWaitResult` collapses the wait command's discriminated union, so `kind === 'text'` still leaves `text` optional and permits impossible result shapes. Preserve the per-kind variants so consumers and injected fakes can rely on the discriminator.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| export type DaemonWaitResult = Readonly<{ | ||
| kind: 'sleep' | 'text' | 'selector' | 'absent' | 'stable'; | ||
| waitedMs: number; | ||
| text?: string; | ||
| selector?: string; | ||
| captures?: number; | ||
| nodeCount?: number; | ||
| hint?: string; | ||
| node?: SnapshotNode; | ||
| preActionNodes?: SnapshotNode[]; | ||
| }>; |
There was a problem hiding this comment.
P2: DaemonWaitResult collapses the wait command's discriminated union, so kind === 'text' still leaves text optional and permits impossible result shapes. Preserve the per-kind variants so consumers and injected fakes can rely on the discriminator.
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/contracts/src/daemon-command-surface.ts, line 180:
<comment>`DaemonWaitResult` collapses the wait command's discriminated union, so `kind === 'text'` still leaves `text` optional and permits impossible result shapes. Preserve the per-kind variants so consumers and injected fakes can rely on the discriminator.</comment>
<file context>
@@ -0,0 +1,370 @@
+ DaemonSelectorSnapshotOptions &
+ Readonly<{ target: DaemonWaitTarget }>;
+
+export type DaemonWaitResult = Readonly<{
+ kind: 'sleep' | 'text' | 'selector' | 'absent' | 'stable';
+ waitedMs: number;
</file context>
| export type DaemonWaitResult = Readonly<{ | |
| kind: 'sleep' | 'text' | 'selector' | 'absent' | 'stable'; | |
| waitedMs: number; | |
| text?: string; | |
| selector?: string; | |
| captures?: number; | |
| nodeCount?: number; | |
| hint?: string; | |
| node?: SnapshotNode; | |
| preActionNodes?: SnapshotNode[]; | |
| }>; | |
| export type DaemonWaitResult = | |
| | Readonly<{ kind: 'sleep'; waitedMs: number }> | |
| | Readonly<{ kind: 'text'; waitedMs: number; text: string }> | |
| | Readonly<{ | |
| kind: 'selector'; | |
| waitedMs: number; | |
| selector: string; | |
| node?: SnapshotNode; | |
| preActionNodes?: SnapshotNode[]; | |
| }> | |
| | Readonly<{ kind: 'absent'; waitedMs: number }> | |
| | Readonly<{ | |
| kind: 'stable'; | |
| waitedMs: number; | |
| captures: number; | |
| nodeCount: number; | |
| hint?: string; | |
| }>; |
|
|
||
| void startDaemonRuntime().catch((error) => { | ||
| void startDaemonRuntime({ | ||
| // The composition root's surface stays behind `await import`, loaded on first request use. |
There was a problem hiding this comment.
P3: This comment restates the dynamic-import behavior shown immediately below, contrary to AGENTS.md's no-control-flow-narration rule. Replace it with the non-obvious reason for keeping this import lazy.
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 src/daemon.ts, line 5:
<comment>This comment restates the dynamic-import behavior shown immediately below, contrary to `AGENTS.md`'s no-control-flow-narration rule. Replace it with the non-obvious reason for keeping this import lazy.</comment>
<file context>
@@ -1,7 +1,13 @@
-void startDaemonRuntime().catch((error) => {
+void startDaemonRuntime({
+ // The composition root's surface stays behind `await import`, loaded on first request use.
+ createCommandSurface: async (config) =>
+ (await import('./command-runtime/runtime-command-surface.ts')).createCommandSurfaceAgentDevice(
</file context>
| // The composition root's surface stays behind `await import`, loaded on first request use. | |
| // Keep command-runtime out of the daemon's eager import graph. |
| const session = params.sessionStore.requireCurrent(params.sessionRef); | ||
| const consumedCapture: { state?: SnapshotState } = {}; | ||
| const runtime = createInteractionRuntimeForRoute({ | ||
| const runtime = await createInteractionRuntimeForRoute({ |
There was a problem hiding this comment.
P3: actionStartedAt is now stamped after the awaited port call on line 75, so the first-use command-surface module load (the await import in src/daemon.ts) is excluded from the touch action's recorded span — while dispatchGestureInteraction stamps its start at function entry and includes the same load. First-request durations for touch vs gesture interactions are therefore not comparable. Move const actionStartedAt = Date.now(); to before the runtime construction (matching the gesture path) if the span is meant to cover the whole request-side interaction work.
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 src/daemon/interaction/internal/interaction-touch-runtime.ts, line 75:
<comment>`actionStartedAt` is now stamped *after* the awaited port call on line 75, so the first-use command-surface module load (the `await import` in `src/daemon.ts`) is excluded from the touch action's recorded span — while `dispatchGestureInteraction` stamps its start at function entry and includes the same load. First-request durations for touch vs gesture interactions are therefore not comparable. Move `const actionStartedAt = Date.now();` to before the runtime construction (matching the gesture path) if the span is meant to cover the whole request-side interaction work.</comment>
<file context>
@@ -72,7 +72,7 @@ export async function dispatchRuntimeInteraction<
const session = params.sessionStore.requireCurrent(params.sessionRef);
const consumedCapture: { state?: SnapshotState } = {};
- const runtime = createInteractionRuntimeForRoute({
+ const runtime = await createInteractionRuntimeForRoute({
...params,
touchExecutor: options.touchExecutor,
</file context>
…ed port The daemon no longer imports createCommandSurfaceAgentDevice; the composition root (src/daemon.ts) supplies it through the required createCommandSurface field threaded from RequestRouterDeps down to every execution runtime. The port shape lives in @agent-device/contracts/daemon-command-surface so naming it costs daemon files no type edge into the command families.
…contravariantly
- src/daemon.ts injects loadCommandSurface behind await import so the command
surface stays out of the daemon entry's eager closure (470 modules, at the
merge-base; the static import reached 550). startDaemonRuntime awaits the
loader once and threads the sync factory unchanged.
- DaemonCommandSurface{Capture,Selectors,Interactions} members become
function-typed properties so strictFunctionTypes checks them
contravariantly. The check found the port's DaemonGetOptions.target was
InteractionTarget while the implementation takes ElementTarget; the port
narrows to ElementTarget, which matches the one call site (ref | selector).
|
Follow-up on b617d12, same commit as my earlier review. Two things now block this PR. First, the branch conflicts with main after today's merges. Please rebase. Second, the open Cubic thread on Not blocking: the two P3 Cubic notes (the narrating comment in I removed |
thymikee
removed
the
ready-for-human
…at startup CreateDaemonCommandSurface becomes an async factory: the composition root's lambda keeps the surface behind await import, and the four construction sites await the port, so a daemon that never serves a surface command never evaluates it. startDaemonRuntime's startup await and the LoadDaemonCommandSurface seam go away; the test fixture wraps the real factory in an async lambda and the test bags keep the createCommandSurface field name.
b617d12 to
7c4f6b7
Compare
|
1 participant
Summary
Stacked on #3394. The daemon stops importing
createCommandSurfaceAgentDevice: the composition root (src/daemon.ts) supplies it as the requiredcreateCommandSurfaceport, typed byCreateDaemonCommandSurfacein@agent-device/contracts/daemon-command-surface. The port is an async factory kept behindawait importin the executable, so the command surface loads on first request use — the daemon entry's eager closure stays at the merge-base's 470 modules. Threading path:RequestRouterDeps-> request-handler-chain params -> session/snapshot/find/interaction handlers -> generic dispatch -> execution runtimes (interaction, screenshot, selector, snapshot, sparse-fallback, generic-settle, session-open), whose four construction sites await the port. Behavior unchanged; the field stays required, so 146 test param-bag literals gained one line via a shared fixture (src/daemon/__tests__/command-surface-fixture.ts). No assertions changed except three exact-forward-shape tests that now include the port.Diff budget: the production change is ~490 lines (29 files); the rest is the mechanical AST-codemod insertion of the required
createCommandSurfacefixture line into 146 test param bags, and splitting it out would leave an intermediate commit that does not type-check.Validation
Commit
7c4f6b716(stack rebased onto currentmainvia #3394, then replayed).pnpm build,pnpm check:quick,pnpm check:layering,pnpm check:di-seams, test-file size ratchet, andpnpm check:affected --runpass on this head. Depgraph query: zero edges fromdaemon/**production nodes tocommand-runtime/**orcommands/**nodes (jqon.tmp/depgraph/graph.json); the only edge intocommand-runtime/runtime-command-surface.tsoriginates from thedaemon.tsexecutable (the dynamic-import seam).