Skip to content

refactor(test-kit): move the dispatch-disclosure harness out of contracts - #3690

Merged
thymikee merged 7 commits into
mainfrom
refactor/dispatch-disclosure-to-test-kit
Oct 16, 2026
Merged

thymikee merged 7 commits into
mainfrom
refactor/dispatch-disclosure-to-test-kit

Conversation

@thymikee

@thymikee thymikee commented Oct 15, 2026 •

Copy link
Copy Markdown
Member

What

The dispatch-disclosure harness is a cross-package test fixture: eleven driver tests across daemon, platform-android, platform-apple, platform-web, provider-webdriver and root each read the shared row table and assert they drive exactly the rows they own. It was published from @agent-device/contracts/dispatch-disclosure-fixtures; it now lives in @agent-device/test-kit as three named exports.

  • Commit 1: pure git mv of dispatch-disclosure.fixtures.ts and dispatch-disclosure.test.ts from packages/contracts/src to packages/test-kit/src (contracts cannot import test-kit, so its table test moves with the harness). Content is unchanged; both files use ../../../ for the repo root, which is the same depth.
  • Commit 2: the file's first test (discloseDispatch / discloseUnclassifiedDispatch behaviour) tests kernel/errors, so it becomes packages/kernel/src/errors-dispatch-disclosure.test.ts, verbatim; the two table tests stay in test-kit.
  • Commit 3: the eleven drivers import the three names they use (assertDispatchDisclosureDriversMatchRows, DISPATCH_DISCLOSURE_TABLE_PATH, dispatchDisclosureRowsOwnedBy) from @agent-device/test-kit; platform-android, platform-apple, platform-web and provider-webdriver declare test-kit in devDependencies and tsconfig references (daemon already did); the contracts subpath and its export-snapshot line are removed.
  • Commit 4: merges two duplicate test-kit import statements that lint flagged.
  • Commit 5 (chore(gates)): R13 refused any @agent-device/test-kit import from a platform package, tests included. It now allows it in test files (*.test.ts, __tests__/, *.fixtures.*) only; production source is still refused. Unit-tested both ways in platform-package-policy.test.ts.

Fixture decision

Fixture Importers by package Outcome Reason
contracts/dispatch-disclosure-fixtures daemon 2 tests, platform-android 2, platform-apple 2, platform-web 1, provider-webdriver 2, root 2 case 3: test-kit shared by driver tests of six packages; needs only kernel types and the repo-level table

Parity

vitest run <files> --reporter=json, origin/main against this branch:

files tests passed
before 12 (11 drivers + contracts dispatch-disclosure.test.ts, 3 tests) 196 / 196
after 13 (11 drivers + kernel errors-dispatch-disclosure 1 + test-kit dispatch-disclosure 2) 196 / 196

unit-core includes packages/*/src/**/*.test.ts, so the moved test-kit and kernel tests are selected; check:affected --run ran them. The 11 drivers were not moved (specifier only). Several use vi.mock; none of those mocks was edited, and all pass.

Surface and counts

exports: contracts 133 -> 132 (./dispatch-disclosure-fixtures removed); test-kit stays one "." entry with +3 named exports; no new subpaths, barrels or export *. New devDependency + tsconfig reference on test-kit in platform-android, platform-apple, platform-web, provider-webdriver (test-kit reaches only capture-kit, contracts, host-kit, kernel, so no cycle; tsc -b passes).

Colocation (my scan): contracts 1 -> 0 not beside a named module.

Gates

check:quick, check:layering, check:fallow, check:gate-manifest, pnpm format, check:affected --run: all pass on head.

🤖 Generated with Claude Code

View guided diff Turn on auto-fix

@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 28 files

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

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

Comment thread scripts/layering/platform-package-policy.ts Outdated
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.24 MB 5.24 MB 0 B
Package (unpacked) 5.24 MB 5.24 MB 0 B
Package (download) 1.58 MB 1.58 MB -12 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 29.1 ms 28.3 ms -0.9 ms
CLI --help 81.9 ms 82.4 ms +0.5 ms

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-16 06:03 UTC

@thymikee

Copy link
Copy Markdown
Member Author

The move itself looks right at 1e9a0ab: test-kit owns the harness, kernel owns the error test, and contracts shrinks.

One layering rule needs a small change before merge. isTestKitTestImport decides "is test support" from path substrings. The same file already imports isProductionSourceFile, which owns that classification and also covers test/ trees, *.spec.*, fixtures.ts and *-fixtures.ts. A platform-package test-support file outside the listed substrings, such as a *.spec.ts, would get a false R13 refusal when it imports test-kit. No current file is hit, so this fails closed and does not break anything today. Could R13 state the rule once as specifier === '@agent-device/test-kit' && !isProductionSourceFile(file)? That matches the invariant R11 and R83 already use. I could not find a smaller shape: the harness cannot sit in kernel without mixing six packages' test paths into a production package, and keeping it in contracts is what R83 forbids.

Not blocking: after that change, platform-package-policy.test.ts could also cover a __tests__/ file and a .fixtures. file, since the new test only exercises probe.test.ts. You can take or leave it.

The open inline thread on this predicate still applies: #3690 (comment). It is the same issue as above. Its cited example, root src/recording/fixtures.ts, is outside platform packages and never reaches this branch, so the problem is latent only.

All 21 checks are still queued and none has failed, so I have no job output. I did not run check:layering, the unit tests or tsc -b, so the 196/196 parity and gate-pass claims are yours. No device validation is needed for a test-only move. There are no conflicts.

Before merge, replace isTestKitTestImport with !isProductionSourceFile(file), then get the layering, typecheck and unit jobs green on the new head.

thymikee and others added 7 commits October 15, 2026 20:47
…e test from contracts

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… kernel errors module

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r files

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Test support is not a platform substrate dependency; production source stays refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…source classification

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@thymikee
thymikee force-pushed the refactor/dispatch-disclosure-to-test-kit branch from 1e9a0ab to efec583 Compare October 15, 2026 19:17
@thymikee

Copy link
Copy Markdown
Member Author

Addressed the review and the open thread: R13's test-kit exception now uses the shared isProductionSourceFile predicate instead of path substrings, with test cases for src/recording/fixtures.ts, .test, .spec and a refused production file (efec583).

@thymikee

Copy link
Copy Markdown
Member Author

This PR is ready at efec583. The earlier thread on scripts/layering/platform-package-policy.ts:178 is fixed: isTestKitTestImport now calls isProductionSourceFile, and platform-package-policy.test.ts covers the fixtures.ts, .test, .spec and refused-production cases. CI is green (23 checks, none failing), and the layering gate and platform-package-policy.test.ts exercise the changed R13 path directly. There are no conflicts. I read the diff against the merge base and did not rerun layering or vitest locally. The unchanged parts of the move (fixtures content, kernel test split) were covered by the previous review and I did not recheck them.

Not blocking, and you can take or leave these: R13 reads every tracked .ts file, so a non-src/ tooling file in a platform package, such as packages/platform-apple/scripts/verify-installed-snapshot-bridge.ts, can import test-kit without a violation. The classifier docstring says this is on purpose. Also, commit 5 in the PR body still says the exception covers *.test.ts, __tests__/ and *.fixtures.*, while the code now delegates to isProductionSourceFile.

The earlier inline thread on the R13 classifier no longer applies, because the head fixes it, so you can resolve it: #3690

No further work is needed before merge, beyond maintainer approval.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 15, 2026
@thymikee
thymikee merged commit 86e7d52 into main Oct 16, 2026
23 checks passed
@thymikee
thymikee deleted the refactor/dispatch-disclosure-to-test-kit branch October 16, 2026 06:02
thymikee added a commit that referenced this pull request Oct 16, 2026
…ctions-1

* origin/main:
  feat(react-devtools): inspect React components and profile renders through the Metro inspector (#3613)
  refactor(daemon-contracts): drop the registration and claims fixture exports (#3689)
  refactor(test-kit): drop the daemon-contracts dependency (#3686)
  refactor(test-kit): drop command-registry by making the replay-script generator a root fixture (#3687)
  fix(ios): handle delayed launch confirmations without stopping XCTest (#3682)
  refactor(test-kit): move the dispatch-disclosure harness out of contracts (#3690)
  refactor(test-kit): host the target classification capture fixture (#3691)
  ci: fold six CI jobs into four with parallel and background steps; cancel superseded and closed-PR runs (#3658)
  build(typescript): move root project references into tsconfig.typecheck.json (#3688)
  chore(daemon): declare @agent-device/proxy as a devDependency, since only a test imports it (#3685)
  refactor(settings): declare settings once and admit them per setting (#3662)

# Conflicts:
#	packages/platform-android/src/alert.ts
#	website/docs/docs/limrun.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant