Skip to content

test: route test/ and scripts/ off the daemon's testing exports - #3733

Open
thymikee wants to merge 25 commits into
mainfrom
refactor/daemon-testing-exports-integration
Open

thymikee wants to merge 25 commits into
mainfrom
refactor/daemon-testing-exports-integration

Conversation

@thymikee

@thymikee thymikee commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Rebased onto origin/main after refactor(remote): extract src/remote into @agent-device/remote #3711 extracted src/remote into @agent-device/remote and refactor(plugins): extract the provider plugins into a private package #3712 moved provider plugins into a private package. The remote diagnostics conflict now targets @agent-device/remote/remote-request-diagnostics; the artifact-directory client test moved beside packages/remote/src/daemon-artifacts.ts.
  • Rebuilt the provider-scenario harness on startDaemonRuntime, the root composition, the real daemon HTTP server, and a real client. Provider scenarios use the public runtime/client and no daemon testing imports.
  • Moved the HTTP server provider-scenario test beside packages/daemon/src/server/http; moved the Node concurrency-torture lane and its helpers into packages/daemon/test/nightly.
  • Added package-local tests for the touch response payload and action recording. Updated command-coverage path evidence. Output-economy and help-conformance now start the real root composition through startDaemonRuntime and use real HTTP RPC responses to exercise the production response views and error producers.
  • The proxy parity fixture forwards the diagnostics GET used by its missing-ref case and asserts the remote log is materialized. Provider-admission cleanup now closes the daemon, provider runtime, and regression server even if lease release fails.
  • Removed 15 daemon testing exports from the rebased origin/main manifest (70 test-only exports → 55); the retained ./testing/context line remains for the final-pin owner.

Placement

  • (b) Composed behavior: provider scenarios start the public daemon runtime and drive it through a real client or HTTP RPC. They do not inspect daemon state.
  • (a) Daemon logic: the HTTP server scenario is beside the server module; the touch response and action recorder invariants are tested beside their daemon modules. The concurrency suite is package-local.
  • (c) Root behavior: command-coverage declarations cite test paths only. Output-economy and help-conformance stay in root and drive the public daemon route through createProviderScenarioHarness (startDaemonRuntime, real HTTP server, real RPC client). Their digest samples come from live route responses; their error samples come from public RPC responses passed through the root CLI error renderer. Neither root gate imports packages/daemon/test, a testing export, or daemon internals. No R11 exception was added.
  • remote-test-artifacts-directory now tests @agent-device/remote's daemon-artifacts module in packages/remote/src/__tests__, beside the module it exercises. The proxy diagnostics, tenant, parity, and materialization scenarios stay in test/integration/provider-scenarios because they cover composed proxy, daemon, replay-port, and remote-client behavior; the package-local remote tests already cover its individual transfer modules. Provider scenarios remain integration tests around the real daemon and private plugin packages, rather than copies of their package-local runtime tests.
  • The scripts/layering files were checked as text: model.ts has a comment, while package-boundaries-facade.test.ts and test-kit-isolation.test.ts use fixture strings. None imports daemon test support or calls vi.mock with it.

Test counts

Counts are test cases at the main path and at the head destination. Helper files have no test cases.

Main path Main Head path Head
test/integration/provider-scenarios/harness.ts 0 same 0
test/integration/provider-scenarios/stale-provider-runtime-admission.fixtures.ts 0 same 0
test/integration/provider-scenarios/cloud-webdriver-lease-http.test.ts 1 same 1
test/integration/provider-scenarios/cloud-webdriver-runtime.test.ts 7 same 7
test/integration/provider-scenarios/daemon-http-server.test.ts 10 packages/daemon/src/server/http/http-server-provider-scenarios.test.ts 10
test/integration/provider-scenarios/ios-fold.test.ts 2 same 2
test/integration/provider-scenarios/ios-wait-selector.test.ts 4 same 4
test/integration/provider-scenarios/limrun-ios-snapshot-owner.test.ts 1 same 1
test/integration/provider-scenarios/remote-proxy-artifact-tenant.test.ts 2 same 2
test/integration/provider-scenarios/remote-proxy-parity.test.ts 5 same 6
test/integration/provider-scenarios/remote-proxy-request-diagnostics.test.ts 1 same 1
test/integration/provider-scenarios/remote-test-artifacts-directory.test.ts 3 packages/remote/src/__tests__/daemon-artifacts-directory.test.ts 3
test/integration/provider-scenarios/remote-test-artifacts-materialization.test.ts 2 same 2
test/integration/provider-scenarios/scroll-until.test.ts 3 same 3
test/integration/provider-scenarios/cloud-webdriver-ios-text-entry.test.ts 5 same 5
test/integration/provider-scenarios/web-desktop.test.ts 1 same 1
test/integration/provider-scenarios/stale-provider-runtime-admission.test.ts 3 same 3
test/integration/provider-scenarios/provider-device-runtime.test.ts 4 same 5
test/integration/nightly/concurrency-torture/bindings.ts 0 packages/daemon/test/nightly/concurrency-torture/bindings.ts 0
test/integration/nightly/concurrency-torture/harness.ts 0 packages/daemon/test/nightly/concurrency-torture/harness.ts 0
test/integration/nightly/concurrency-torture/invariants.ts 0 packages/daemon/test/nightly/concurrency-torture/invariants.ts 0
test/integration/nightly/concurrency-torture/real-scope-serialization.ts 0 packages/daemon/test/nightly/concurrency-torture/real-scope-serialization.ts 0
test/integration/command-coverage/declarations.ts 0 same 0
test/integration/interaction-contract/native-ref.contract.test.ts 9 same 9
test/output-economy/render-fixtures.ts 0 same 0
scripts/__tests__/help-conformance-sample-producers.ts 0 same 0
test/integration/provider-scenarios/help-conformance-error-routes.ts 0 same 0
New 0 test/integration/provider-scenarios/help-conformance-error-routes.test.ts 1
test/output-economy/output-economy.baseline.test.ts 13 same 15
scripts/layering/model.ts 0 same, string evidence only 0
scripts/layering/package-boundaries-facade.test.ts 5 same, string fixtures only 5
scripts/layering/test-kit-isolation.test.ts 12 same, string fixture only 12

Additional moves and split tests:

Main path Main Head path Head
test/integration/nightly/concurrency-torture.test.ts 5 packages/daemon/test/nightly/concurrency-torture.test.ts 5
test/integration/nightly/concurrency-torture/claim-registry.ts 0 packages/daemon/test/nightly/concurrency-torture/claim-registry.ts 0
test/integration/nightly/concurrency-torture/deterministic-scheduler.ts 0 packages/daemon/test/nightly/concurrency-torture/deterministic-scheduler.ts 0
test/integration/nightly/concurrency-torture/envelope.ts 0 packages/daemon/test/nightly/concurrency-torture/envelope.ts 0
New 0 packages/test-kit/src/lane-envelope.ts 0
test/integration/nightly/concurrency-torture/prng.ts 0 packages/daemon/test/nightly/concurrency-torture/prng.ts 0
New 0 packages/daemon/test/nightly/concurrency-torture/lane-envelope.ts 0
Existing 29 packages/daemon/src/core/__tests__/session-action-recorder.test.ts 31
New 0 packages/daemon/src/execution/interaction/internal/interaction-touch-response-contract.test.ts 1
packages/daemon/src/execution/interaction/internal/interaction-touch-routing.test.ts 7 same 8

The native-ref contract remains at 9 cases; its daemon-only response-builder assertion moved to the new package test, the response builder keeps capture evidence out of serialized touch payloads. The recorder test adds ordinary action history preserves authored commands, positionals, and replay flags and --no-record suppresses a mutating coordinate press from action history. The provider device test adds the public session-to-device-and-lease binding case. Root script-publication scenarios retain their public error, saved-file, and replay assertions; daemon recording and publication state assertions are covered beside the owning daemon modules, including parameterized fills keep literals out of recording state with deterministic placeholders, publishes without close, returns the path/count, and leaves a terminal live session, #1533: a re-open leaves an aborted authoring lifecycle aborted, and not recording, and #1533: an ordinary armed authoring session still publishes on bare close. The scroll pass-limit assertion is covered by packages/daemon/src/execution/scroll-until.test.ts, test the default budget is the shared constant. The stale-provider route now asserts a typed owner-capability-missing refusal before the counted provider bind or provider execution; packages/daemon/src/sessions/handlers/__tests__/session-reinstall.test.ts remains the daemon-level fail-closed assertion. No test case was dropped from a moved test.

Selection

  • Provider scenarios remain selected by Vitest project provider-integration (vitest.config.ts, include test/integration/provider-scenarios/**/*.test.ts).
  • Package-local daemon src/**/*.test.ts tests are selected by unit-core via packages/*/src/**/*.test.ts.
  • packages/remote/src/__tests__/daemon-artifacts-directory.test.ts is selected by unit-core through that same package-source include and by CI's unit-ci coverage gate (test:coverage:ci). Its three tests pass after the relocation.
  • Concurrency torture is a Node node:test lane, not a Vitest project. The test:concurrency-torture script selects packages/daemon/test/nightly/concurrency-torture.test.ts; both the PR CI lane and nightly workflow call the existing concurrency-torture gate. The package test/ path is outside unit-core.
  • test/output-economy/**/*.test.ts is selected by the output-economy Vitest project. The help sample output and recovery coverage tests are selected by unit-core, including the explicitly listed scripts/__tests__/help-conformance-sample-outputs.test.ts. The route helpers are imported by these selected tests and are not standalone tests.
  • pnpm exec vitest related --project output-economy --run --passWithNoTests packages/daemon/src/server/response-views.ts selects test/output-economy/output-economy.baseline.test.ts; the route driver is a static import chain from render-fixtures.ts, so response-view changes reach the output-economy gate without a child process.
  • pnpm check:gate-manifest: passed, 57 checks wired across 25 lanes. pnpm test:concurrency-torture: passed, 4 tests.

Moves

Pure move commits are separate from edits: 20972a21f (concurrency tree), f402a5f00 (HTTP test), and 10c48bf63 (remote artifact-directory test). The move commits contain no line edits; follow-up edits are separate. git diff -M --summary origin/main...HEAD reports the concurrency test 100%, concurrency helpers 77–98%, HTTP test 98%, and remote artifact-directory test 69% similarity.

Mocks and spies

  • limrun-ios-snapshot-owner.test.ts: @limrun/api mock is observed by createInstance.mock.calls.length === 1.
  • limrun-ios-snapshot-owner.test.ts: @limrun/api/ios-client mock is observed by elementTree.mock.calls.length === 1.
  • stale-provider-runtime-admission.test.ts: @limrun/api mock is observed by one allocation before release and no additional allocation before admission.
  • stale-provider-runtime-admission.test.ts: @limrun/api/instance-client mock is observed by the release-time disconnect.
  • stale-provider-runtime-admission.test.ts: the counted provider-module bind spy remains unchanged after the public route's typed admission refusal.
  • cloud-webdriver-runtime.test.ts: the Date.now spy advances past lease expiry; the server records WebDriver DELETE.
  • remote-proxy-parity.test.ts expiry case: the Date.now spy advances past expiry; the proxy returns LEASE_NOT_FOUND and a fresh lease has no prior comparison state.
  • remote-proxy-parity.test.ts heartbeat case: the Date.now spy proves renewed expiry moves forward and the session baseline remains usable.
  • ios-fold.test.ts: the Date.now spy advances by accumulated tool timeout; the final permitted hinge read succeeds and stays within the client envelope.

The dead ios-app-session-hint and Apple runner operations mocks were removed.

Mutation checks

Each new or moved destination test was mutated and then restored:

  • HTTP body limit changed from 1 MiB to 2 MiB: the moved HTTP server test failed.
  • Recorded touch capture evidence replaced with an empty object: the touch response contract test failed.
  • Action history changed from append to prepend: the recorder ordering assertion failed.
  • Fresh-session device locking disabled: the concurrency-torture lane failed all four cases on overlap.
  • downloadRemoteArtifact forced to treat directories as files: the moved remote artifact-directory test failed on extraction and rejected symlink coverage; the implementation was restored.
  • The Apple output-guard fake's runner authority changed to scoped-provider: the iOS guard hit its record timeout; the fake was restored to local-lease.
  • In an isolated worktree at eb5528854, removing artifacts from the production screenshot digest made test/output-economy/output-economy.baseline.test.ts fail its reviewed byte/shape baseline (1 failed, 25 passed).
  • In the same isolated worktree, changing the production DEVICE_IN_USE recovery hint made DEVICE_IN_USE_SAMPLE fail in scripts/__tests__/help-conformance-sample-outputs.test.ts (1 failed, 13 passed).
  • Changing the production live-claim hint made DEVICE_CLAIM_IN_USE_SAMPLE fail in that same test (1 failed, 13 passed).
  • Changing the production stale-ref recovery hint made STALE_REF_SAMPLE fail there (1 failed, 13 passed).
  • Changing the production ambiguous-match message made AMBIGUOUS_MATCH_SAMPLE fail there (1 failed, 13 passed).
  • Removing appBundleId from the production open response made all five cloud iOS text-entry scenarios fail at appBundleId === 'com.example.demo'.
  • Adding fabricated x/y to a recorded ref click made the interaction-route test fail; suppressing type action recording made its recorded-command assertion fail.
  • Dropping the lease from a published provider session made the provider device-runtime test fail because close no longer released the allocated lease.
  • Binding the unsupported install route before returning the typed refusal made the public admission test fail its zero-bind assertion (observed 1 bind).
  • Removing the production artifact-download authorization header made the public materialization test fail against its token-protected endpoint.
  • Omitting params.tenantId made the proxy integration test fail for agent_device.command; removing the upstream token header made its RPC-handler observation fail.
  • Removing encodeURIComponent from the diagnostics session path made the proxy test fail its exact PLAIN_SESSION URL assertion (cwd: versus cwd%3A).
  • Disabling diagnostics GET forwarding in the parity fixture made the direct/proxied scenario fail because the missing-ref response had no localized logPath (1 failed, 4 passed).
  • At eee378dd5, a disposable worktree was installed and built. Removing session encoding made the diagnostics request test fail with HTTP 404 (1 failed); preventing snapshot-generation increments made the settle-generation test fail on equal pre-action and settle generations (1 failed, 14 passed).
  • In the same disposable worktree, making daemon close unconditional replaced the expected startup error with a TypeError; disabling either temp-directory removal left an entry in the temp root; and skipping the claims-directory env restore failed the previous-value assertion. Each mutation was reverted, the restored tests passed, and the worktree was clean before removal.

Exports

packages/daemon exports: 71 on rebased origin/main (. plus 70 testing exports) → 56 on this branch (. plus 55 testing exports). This PR removes:

./testing/daemon-command-registry
./testing/device/device-claim-conflict
./testing/handlers/session-app-deployment
./testing/interaction/internal/interaction-touch-response
./testing/interactor-resolution
./testing/ios-app-session-hint
./testing/request-binding
./testing/request-execution-scope
./testing/request-router
./testing/response-views
./testing/scroll-until
./testing/selector-match-errors
./testing/session-observability/__tests__/*
./testing/session-recovery-hints
./testing/web-session-names

Remaining exports and their owners:

  • daemon-composition owner mcp:57b5c3dd (44): ./testing/__tests__/android-observation-fixture, ./testing/__tests__/application-lifecycle-runtime-fixture, ./testing/__tests__/capture-disclosure-target-activation-fact, ./testing/__tests__/interaction-get-runtime-fixture, ./testing/__tests__/legacy-snapshot-capture-fixture, ./testing/__tests__/request-router-dispatch-mocks, ./testing/__tests__/request-router-open-harness, ./testing/__tests__/runtime-binding-conformance, ./testing/__tests__/screenshot-runtime-fixture, ./testing/__tests__/selector-capture-fixture, ./testing/__tests__/snapshot-runtime-fixture, ./testing/__tests__/support/command-surface-fake, ./testing/__tests__/support/screen-recording-live-handle, ./testing/__tests__/support/unavailable-device-runtime-gateway, ./testing/__tests__/test-device-runtime-gateway, ./testing/android-system-dialog, ./testing/app-events, ./testing/artifact-tracking, ./testing/capture-disclosure, ./testing/deferred-interaction-outcome, ./testing/device/device-ready, ./testing/generic-leaf-execution, ./testing/gesture-runtime, ./testing/handlers/snapshot, ./testing/interaction/index, ./testing/interaction/internal/__tests__/*, ./testing/provider-device-admission, ./testing/ref-frame, ./testing/request-dispatch-disclosure, ./testing/request-finalization, ./testing/request-generic-dispatch, ./testing/request-handler-chain, ./testing/request-platform-provider-context, ./testing/screenshot-runtime, ./testing/selector-capture-binding, ./testing/selector-runtime, ./testing/selector-runtime-backend, ./testing/session-capture-binding, ./testing/session-snapshot, ./testing/session-state, ./testing/snapshot-diff-runtime, ./testing/snapshot-quality-latch, ./testing/snapshot-runtime, ./testing/wait-runtime.
  • Shared seams still used by daemon-composition owner mcp:57b5c3dd (8): ./testing/__tests__/support/session-factories, ./testing/__tests__/support/store-factory, ./testing/lease-registry, ./testing/request-dispatch-ledger, ./testing/request-runtime-binding, ./testing/server/http-server, ./testing/session-store, ./testing/snapshot-interactor-capture.
  • Other root-src sweep owner (2): ./testing/provider-lease-expiry, ./testing/session-action-recorder. These remain outside this PR's importer ownership.
  • String-only guard evidence (1): ./testing/context is named only by scripts/layering/test-kit-isolation.test.ts; it is not a live importer. Its released export remains for the final-pin owner, thread mcp:57b5c3dd, to resolve.

Validation: rebased onto latest origin/main (4c1c786) without file conflicts; tested head f2592cb3f248c3cf9acdf26629ed76fff4908d00. CI=true pnpm install --frozen-lockfile, pnpm build, repository-wide pnpm format (through affected), pnpm check:quick, pnpm check:layering, pnpm check:fallow, and pnpm check:gate-manifest passed. pnpm check:affected --run --head f2592cb3f248c3cf9acdf26629ed76fff4908d00 selected 63 checks: 36 passed, 25 GitHub-authoritative or parked checks were skipped, 2 were deduplicated, and the exact-head receipt passed at .tmp/validation/run-kVszWM/receipt.json. Provider-integration passed 64 files / 191 tests; concurrency-torture passed 4 tests. Earlier output-economy and help-conformance test counts and mutation evidence remain recorded above. GitHub CI, including CodeQL, must pass on this pushed head.

Latest review follow-up: origin/main is 4c1c786; git rebase origin/main reports the branch is up to date, and the branch contains #3728's seven export deletions plus this sweep's deletions while retaining ./testing/context. Commit 63df0ae1ccbb71b8a988b6b5fa92466f21f18b38 strengthens the materialization evidence by asserting the shared recorder sees a real daemon test RPC followed by an authenticated artifact GET. The materialization, tenant, and diagnostics scenarios all use the shared proxy-to-real-daemon fixture in harness.ts. Full provider-integration passed 64 files / 191 tests; pnpm format, check:quick, check:layering, check:fallow, and check:gate-manifest passed. Disabling artifact forwarding failed materialization; disabling the daemon tenant-owner check failed the tenant scenario with Missing expected rejection. Both mutations were reverted. No test cases were added or removed by this follow-up.

The new exact-head check:affected --run --head 63df0ae1ccbb71b8a988b6b5fa92466f21f18b38 did not complete: the sandbox first denied its external reclaim lock, and the escalated retry waited on the host gate lock owned by another live validation process (PID 75149, started 22:27) before being aborted. The earlier exact-head run at f2592cb3f248c3cf9acdf26629ed76fff4908d00 passed; the current head's complete provider-integration suite and the gates listed above passed.

Latest review follow-up at 85857f5009706a432c83ef5a11df037f6aabac0d:

  • 4ece298a0 moves the recording proxy and its forwarding helpers from harness.ts to harness-proxy.ts; the separate commit precedes all behavior changes. Shared command/result conversion lives in harness-rpc.ts.
  • f7a2f9081 replaces the copied Android audio contract with ANDROID_AUDIO_CONTRACT_EVIDENCE in test/integration/command-coverage/evidence.ts, pointing to the literal title in session-audio.test.ts; it deletes session-audio.coverage.ts and the unused wildcard export. The disposable evidence-check run passed with the original title, failed after changing it to Android audio helper starts on emulator (audio evidence is not named), then passed after restoration.
  • f7a2f9081 makes daemon and proxy callCommand return the HTTP status from the response. The initial shared error assertion derived statuses from error codes; commit 35dd42016 below removes that derivation. The help route pins its session-use conflict, stale-ref, ambiguity, and cross-daemon claim responses to 500; the missing-session diagnostics request is 404, and the provider failure is 500. The existing daemon human-control HTTP test asserts the real 423 response.
  • The first help-conformance run exposed an incorrect 423 expectation for the session-use conflict. 85857f500 corrects it to the server's 500; no error assertion was loosened.
  • pnpm test:integration:provider passes 64 files / 192 tests. Help-conformance passes 5 files / 99 tests. pnpm format passes across 4,677 files.
  • pnpm check:affected --run --head 85857f5009706a432c83ef5a11df037f6aabac0d passes with exactCommit: true, checkoutChanged: false: 36 passed, 25 GitHub-authoritative or parked checks skipped, 2 deduplicated, 0 failures. Receipt: .tmp/validation/run-4AdMJH/receipt.json.
  • The --flag=value thread remains resolved; it was scoped file-wide and no one-off matcher was added.

Latest review follow-up at 35dd42016bdfd0354de8a77decbce716f0ad65ae:

  • assertRpcError now requires an explicit 400 | 401 | 404 | 423 | 500 status argument and compares it with the actual HTTP response. All 49 call sites pass their scenario's literal; there is no code-to-status mapping in the helper. The provider test trees contain no REPAIR_SESSION_EXPIRED scenario.
  • Mutation proof in a disposable worktree at this commit: the focused batch-flags.test.ts scenario passed (1/1), failed after changing its expected status from 400 to 500 (400 !== 500), then passed after restoration (1/1). The disposable worktree was removed.
  • pnpm test:integration:provider passes 64 files / 192 tests. pnpm format passes across 4,677 files. pnpm check:affected --run --head 35dd42016bdfd0354de8a77decbce716f0ad65ae passes with exactCommit: true, checkoutChanged: false: 36 passed, 25 GitHub-authoritative or parked checks skipped, 2 deduplicated, 0 failures. Receipt: .tmp/validation/run-6mtmnQ/receipt.json.

CodeQL stack-trace follow-up: commit 125a5c2986d9e0cfacd5da5d6f5c97311b673319 records only an upstream error code on the test proxy entry, never the caught Error object; its response remains the fixed upstream request failed body. The fixed-body assertion passes in the focused parity suite. The first isolated mutation setup hit registry DNS ENOTFOUND; a subsequent isolated install succeeded for the proxy-cleanup mutation. No separate stack-trace mutant was run. CodeQL must be confirmed on this pushed head.

Review follow-up: commit 070be35470 moves daemon and proxy setup under the protected try and makes each cleanup conditional. A startup-failure regression uses a real daemon and injected proxy startup error, then asserts close ran. The provider parity suite passed 7/7; a disposable baseline passed (1 passed, 6 skipped) and the mutation skipping daemon close failed (1 failed, 6 skipped, closeCalls was 0 instead of 1). pnpm format and pnpm check:quick passed. The existing CodeQL fix from 125a5c2986 is included in the pushed branch.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.32 MB 5.32 MB +4 B
Package (unpacked) 5.32 MB 5.32 MB +4 B
Package (download) 1.60 MB 1.60 MB -5 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.6 ms 27.8 ms +0.2 ms
CLI --help 76.6 ms 79.1 ms +2.5 ms

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://callstack.github.io/agent-device/pr-preview/pr-3733/

Built to branch gh-pages at 2026-10-17 03:52 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 56 files

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread test/output-economy/render-fixtures.ts Outdated
Comment thread packages/daemon/test/nightly/concurrency-torture/envelope.ts Outdated
Comment thread test/integration/provider-scenarios/harness.ts
Comment thread scripts/__tests__/help-conformance-sample-producers.ts Outdated
Comment thread test/integration/provider-scenarios/remote-proxy-artifact-tenant.test.ts Outdated
Comment thread test/integration/provider-scenarios/remote-proxy-artifact-tenant.test.ts Outdated
Comment thread test/integration/provider-scenarios/remote-proxy-request-diagnostics.test.ts Outdated
Comment thread packages/remote/src/__tests__/daemon-artifacts-directory.test.ts Outdated
Comment thread test/integration/provider-scenarios/remote-proxy-parity.test.ts Fixed
@thymikee

Copy link
Copy Markdown
Member Author

Two tests in this PR no longer reach the real daemon, so they cannot catch daemon regressions. Reviewed commit 1908337. CodeQL fails on this head, and the Smoke Tests jobs are still queued. Once the two findings below are fixed and the checks are green, nothing else stops the merge. No conflicts.

The test in remote-test-artifacts-materialization.test.ts (https://github.com/callstack/agent-device/blob/1908337/test/integration/provider-scenarios/remote-test-artifacts-materialization.test.ts#L75) says it covers the composed daemon and client path. It now packs the directory with createDirectoryArtifactArchive and serves it from a hand-written server. The real packaging in packages/daemon/src/server/downloadable-artifact-http.ts no longer feeds downloadRemoteArtifact. If the daemon changes the archive root name or layout, every test still passes while real test --remote materialization breaks. The rule: every scenario that names the daemon must serve its bytes from the real daemon HTTP server. Start the daemon with createProviderScenarioHarness and produce the artifact with a real command. Or move the composed case into packages/daemon/src/server/http beside daemon-runtime-artifacts.test.ts. Then delete the archive packer copy.

The fake upstream in remote-proxy-artifact-tenant.test.ts (https://github.com/callstack/agent-device/blob/1908337/test/integration/provider-scenarios/remote-proxy-artifact-tenant.test.ts#L28) enforces the tenant rule itself, then the test asserts that the fake refuses the other tenant. remote-proxy-request-diagnostics.test.ts:50 likewise answers any session path with a canned record. These doubles fail the way the test author wrote them, not the way the daemon fails. A regression in the daemon's artifact tenant check or diagnostics path resolution would still pass. createRecordingUpstreamServer in remote-proxy-parity.test.ts:284 already forwards to the real harness daemon and records traffic. The rule: one shared fixture owns "proxy in front of the real daemon", and every proxy scenario asserts on its recorded requests. Please lift it into harness.ts and route the tenant, diagnostics and materialization scenarios through it. That also covers the upstream-auth and params.tenantId threads.

Not blocking: at remote-proxy-parity.test.ts:314, fetch(new URL(request.url ?? '/', baseUrl)) lets an absolute request URL replace baseUrl, which is what CodeQL flags. You can build the target from baseUrl plus the parsed pathname and search only. Fix it in the harness copy once the helper moves. You can take or leave this.

Could one rule replace most of the copies here: a root test that names a daemon producer either runs it through the public harness or moves into the daemon package? Then the proxy and artifact scenarios would use the harness daemon behind the shared forwarder. The output-economy digests and help-conformance error samples would become golden fixtures that tests in packages/daemon pin against RESPONSE_VIEWS and the error builders, and the root gates would keep reading the same fixtures. That is the contracts/fixtures pattern from AGENTS.md. The torture lane would import one lane-envelope. Most of the 175-line response-view-fixtures copy and the duplicated archive server would go away. What would have to change first is a shared home for golden daemon-output fixtures that both daemon tests and root gates can import, and moving lane-envelope out of scripts/lib.

Open review threads still apply:

This was a read-only review. I did not rerun the mutation checks, provider-integration or concurrency-torture. I did not inspect the individual move commits, so similarity comes from the PR body. I also did not confirm whether a daemon-package test already pins web-provider ref actions without coordinates, or the /artifacts tenant refusal end to end.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 33 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread test/integration/provider-scenarios/stale-provider-runtime-admission.test.ts Outdated
Comment thread test/output-economy/render-fixtures.ts Outdated
Comment thread test/integration/provider-scenarios/remote-proxy-parity.test.ts Outdated
@thymikee

Copy link
Copy Markdown
Member Author

Thanks for the update. The new commit 6c6437e fixes most of the earlier points, but two findings from 1908337 are still open.

Fixed since the last review: the response-views and help-conformance fixtures now call the real daemon code, the platform-runtime flag is gone, the stale-provider install now goes through the harness daemon, and the extra assertions on the proxy upstream are in. Required evidence is in place.

The materialization scenario at test/integration/provider-scenarios/remote-test-artifacts-materialization.test.ts:76 is named "composed daemon and client path", but it still does not use the daemon. The new expectedToken check lives in the hand-written server in artifact-download-fixtures.ts, and the archive still comes from the test's own packer, not from packages/daemon/src/server/downloadable-artifact-http.ts. If the daemon changes the archive root name, the layout or its artifact auth, this test still passes while real test --remote materialization breaks. Please make every scenario that names the daemon serve its bytes from the real daemon HTTP server. Start createProviderScenarioHarness and produce the artifact with a real command, or move the case next to daemon-runtime-artifacts.test.ts in packages/daemon/src/server/http. Then delete the packer copy.

The tenant scenario at test/integration/provider-scenarios/remote-proxy-artifact-tenant.test.ts:27 has the same gap. The upstream is a fake that returns 401 when the tenant header differs (lines 27-36), and the test asserts that the fake refuses. remote-proxy-request-diagnostics.test.ts:50 still answers with a canned record. On main this scenario used the real createDaemonHttpServer with trackDownloadableArtifact. A regression in the daemon's tenant check or in its diagnostics path resolution now passes, so "tenant-scoped" proves only that the proxy forwards the header. Please move createRecordingUpstreamServer (remote-proxy-parity.test.ts:285), which forwards to the real harness daemon, into harness.ts. Then route the tenant, diagnostics and materialization scenarios through it and assert on the recorded forwarded requests, so one shared fixture owns "proxy in front of the real daemon".

Not blocking, and you can take or leave it: scripts/lib/lane-envelope.ts is now a pass-through re-export with four importers (scripts/fuzz/envelope.ts, scripts/fuzz/harness.test.ts, scripts/mutation/envelope.test.ts, scripts/mutation/run.ts), so you could import @agent-device/test-kit there and delete the shim, and the related-selection edge raised on render-fixtures.ts also applies to the second subprocess driver at scripts/tests/help-conformance-sample-producers.ts:109, so fixing both together would be cleaner.

Would lifting the recording forwarder into harness.ts and running every proxy and artifact scenario against the harness daemon be simpler overall? Both archive and download-server copies (packages/remote/src/tests/support and test/integration/provider-scenarios/artifact-download-fixtures.ts) could then go. For output-economy and help-conformance, could tests in packages/daemon emit or pin golden fixtures from RESPONSE_VIEWS and the error builders, so the root gates read static fixtures with no subprocess and no hidden import edge? That needs a shared home for golden daemon-output fixtures that both daemon tests and root gates can import, plus a harness-owned proxy-in-front-of-daemon fixture.

Open threads from other reviewers: these still apply, #3733 (comment) (finally-block cleanup order), #3733 (comment) (subprocess driver import edge), #3733 (comment) (proxy diagnostics parity), #3733 (comment) (token checked only by fake), and one lower-priority thread on the duplicated packer and server (#3733 (comment)). These are fixed at 6c6437e and can be resolved: #3733 (comment) (public open response check added), #3733 (comment) (click and hover ref checks added), #3733 (comment) (digests now come from RESPONSE_VIEWS), #3733 (comment) (lane-envelope moved to test-kit), #3733 (comment) (boolean flag removed), #3733 (comment) (real error builders used), #3733 (comment) (install goes through harness daemon), #3733 (comment) (device id and lease checks added), #3733 (comment) (tenantId asserted), #3733 (comment) (Bearer token asserted), #3733 (comment) (exact URL asserted), #3733 (comment) (redundant rmSync removed), #3733 (comment) (rpc URL built from baseUrl).

CI is still running and no job has failed. Smoke and Coverage run the test tree this PR rewrites, so their results matter once they finish. The Mutants kernel-errors and selectors shards run on packages this PR does not touch. There are no conflicts.

I did not run the two subprocess drivers, so their package resolution under node --experimental-strip-types from packages/daemon/test is unverified. I also did not check whether a daemon-package test already covers the /artifacts tenant refusal end to end. If one does, the tenant half narrows to the scenario's name and claim. Before merge, the materialization, tenant and diagnostics scenarios must run through the real harness daemon via a shared forwarder in harness.ts.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 15 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread test/integration/provider-scenarios/remote-proxy-request-diagnostics.test.ts Outdated
Comment thread test/output-economy/output-economy.baseline.test.ts
Comment thread test/integration/provider-scenarios/help-conformance-error-routes.ts Outdated
@thymikee
thymikee force-pushed the refactor/daemon-testing-exports-integration branch from c197ce8 to eee378d Compare October 16, 2026 19:38
Comment thread test/integration/provider-scenarios/harness.ts Fixed

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 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

Comment thread test/integration/provider-scenarios/remote-proxy-parity.test.ts Outdated
@thymikee
thymikee force-pushed the refactor/daemon-testing-exports-integration branch from ed4d96d to f2592cb Compare October 16, 2026 20:18
@thymikee

Copy link
Copy Markdown
Member Author

Thanks for the update. The earlier review of 6c6437e is mostly addressed at 070be35, and the 39 checks show no failures. One problem remains.

The new DEVICE_IN_USE sample in scripts/help-conformance-sample-outputs.mjs now shows agent-device open https://example.test --platform web --session default, and its hint says to run the same open with --wait (https://github.com/callstack/agent-device/blob/070be35/scripts/help-conformance-sample-outputs.mjs#L88). The case that quotes it, sample-output-device-in-use-reuses-session (https://github.com/callstack/agent-device/blob/070be35/scripts/help-conformance-cases.mjs#L558-L577), still requires a press ... --session checkout command and --settle, and it forbids any agent-device open. So the correct recovery, the same open with --session checkout, now fails the case, and the case rewards a press command the sample never showed. That skews the benchmark signal for this route. Every help-conformance case must match the command and hint of the sample it quotes. Please update this case to require open ... --session checkout (or --wait), drop the --settle and no-reopen checks, and keep the no-close check. Then check every case that quotes a sample this PR changed, including STALE_REF_SAMPLE. The STALE_REF case task still says "The Continue control" (https://github.com/callstack/agent-device/blob/070be35/scripts/help-conformance-cases.mjs#L607), while the new sample's @e2 is "Place order". Could you also confirm that a response-views.test.ts case still asserts that the selector digest keeps a recovery warning, now that the output-economy floor for it is gone?

All five Cubic threads are fixed at 070be35, so please resolve them: the cleanup in stale-provider-runtime-admission.test.ts (#3733 (comment)), the direct import in render-fixtures.ts (#3733 (comment)), the diagnostics route in harness.ts (#3733 (comment)), the artifact test using the real daemon (#3733 (comment)), and the removed duplicate artifact-download-fixtures.ts (#3733 (comment)).

I read the case matchers against the changed sample text. I did not run the output-economy, help-conformance or provider-scenario suites, and I rely on CI for those. The case needs to match the new open-route sample before this can merge.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

You’re at about 94% 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

Comment thread scripts/help-conformance-cases.mjs
@thymikee

Copy link
Copy Markdown
Member Author

Addressed at 1de8fe00a:

  • sample-output-device-in-use-reuses-session now requires agent-device open https://example.test --platform web with --session checkout or a numeric --wait. The press/--settle requirement and the no-open check are gone, and the no-close check stays.
  • I checked every case that quotes a sample this PR changed. The STALE_REF task now names "Place order", matching @e2.
  • response-views.test.ts already asserts that the selector text digest keeps warning: 'recovered from a blocking dialog', so nothing needed adding. That test passes.
  • At this head, help-conformance passes (99 tests) and output-economy passes (27 tests). check:affected --run --head 1de8fe00a passes with exactCommit: true (36 passed, 25 skipped, 2 deduplicated, 0 failures).
  • The five cubic threads you listed were already resolved.

@thymikee

Copy link
Copy Markdown
Member Author

Checks are green at 1de8fe0 (39 checks, none failing), and no conflicts. The earlier finding from 070be35 is fixed. The update only changes a benchmark case table and a test name. I did not run the help-conformance, output-economy or provider-scenario suites, and I did not run a live benchmark, so I have not measured how the new matcher affects real plan scoring. I rely on CI and your report for that. I did not re-audit the full move for new production seams beyond confirming this update adds none. The code looks ready apart from the simplicity question below.

Not blocking, and you can take or leave it: in declarations.ts, the audio evidence is now a hand-copied literal instead of an import of ANDROID_AUDIO_CONTRACT_EVIDENCE. Nothing checks that its testName exists, so renaming the daemon test would leave this entry stale with no error. The established pattern is a defineAndroidContractEvidence entry with a repository path in evidence.ts, as ANDROID_VIEWPORT_RUNTIME_CONTRACT_EVIDENCE does. If you move it there, give session-audio.test.ts a literal title and delete session-audio.coverage.ts. The export ./testing/session-observability/__tests__/* in packages/daemon/package.json now has no importers, so it can go too, and the PR body counts can follow.

A simplicity question: the harness now drives a real HTTP daemon. Should callCommand return the server's real HTTP status instead of rewriting every -32000 error to 200 and leaving -32602 at 400? The rewrite rebuilds the old in-process behavior on top of the transport, so a test cannot see 423 for human_control_active or a 404. Should the recording proxy (createProviderScenarioProxy and the upstream helpers) also move into its own harness-proxy.ts, so harness.ts (752 lines) stays focused on daemon composition?

The open inline thread on --flag=value matching in scripts/help-conformance-cases.mjs does not apply to this PR. Every flag matcher in that file uses the space-separated form, so supporting --flag=value should be one change across the whole file. You can resolve it here: #3733 (comment)

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 15 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

Comment thread test/integration/provider-scenarios/assertions.ts Outdated
@thymikee

Copy link
Copy Markdown
Member Author

Addressed through 35dd42016:

  • Proxy split: the recording proxy is in harness-proxy.ts, moved in its own earlier commit 4ece298a0.
  • Audio evidence (f7a2f9081): it is now a defineAndroidContractEvidence entry checked by path and title, the daemon test has a literal title, and session-audio.coverage.ts is deleted. Mutation: renaming the test title fails the evidence guard (audio evidence is not named), and it passes again after restoring.
  • Unused export: ./testing/session-observability/__tests__/* is deleted. This branch now takes the daemon from 71 to 56 exports.
  • Real HTTP statuses: both harness callCommand functions return the server's status, and the -32000→200 rewrite is gone.
    • assertRpcError no longer derives the status from the error code (that was a copy of the daemon's statusCodeForNormalizedError, and it had already drifted for REPAIR_SESSION_EXPIRED, per cubic). Every one of the 49 call sites passes its literal status.
    • Statuses now asserted:
      • 404 for the missing-session diagnostics route;
      • 400 for INVALID_ARGS;
      • 423 for human_control_active (also pinned by human-control-http.test.ts);
      • 500 for the session-use DEVICE_IN_USE conflict, stale ref, ambiguity, cross-daemon claim and normalized provider failures. Of these, the session-use DEVICE_IN_USE conflict is a corrected status (85857f500): its details carry no human_control_active.
    • Mutation: changing a literal from 400 to 500 fails batch-flags.test.ts.
  • Gates: provider integration 64 files / 192 tests, help-conformance 5 files / 99 tests. Exact-head check:affected --run passed (36 passed, 25 skipped, 2 deduplicated, 0 failed).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants