Skip to content

refactor: remove test-only seams, dead option fields, and duplicate tests - #3308

Open
thymikee wants to merge 3 commits into
mainfrom
simplify/test-only-seams
Open

thymikee wants to merge 3 commits into
mainfrom
simplify/test-only-seams

Conversation

@thymikee

@thymikee thymikee commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Summary

Production code reachable only from tests, option fields no caller sets, and tests that restated a sibling:

  • DaemonError is now an alias of NormalizedError (field-for-field identical; wire ledger ack in the final gates commit).
  • host-kit: one override-path resolver behind the executable and file variants, the canceled-command wrapper inlined, and the abort-aware sleep exported from retry instead of a second module.
  • provision-kit drops the downloadTimeoutMs option no caller supplies; Android observation reads blocking dialogs through app-parsers instead of a verbatim copy.
  • Deleted shutdownSimulator, providerDeviceAdmission(), CoordinateGesturePayload, getCloseShutdown, prefixItems, and selectMaestroSnapshotMatches: each had zero production references.
  • isLikelyStaleSnapshotDrop is imported from capture-kit instead of duplicated; CheckId derives from ALL_CHECKS instead of a second 59-member list.
  • Removed tests whose owner remains: a byte-identical plan-validator case, idle-reap and surface-signature cases implied by a tolerance test, a perf positional case subsumed by the case-insensitive one, interactor-catalog subsets of the seven-operation test, hint regexes duplicated by exact pins, a stub-client clipboard case covered through the real CLI, catalog and lockfile tests re-spelling their siblings, an output-economy expectation computed by the helper under test, and a zip fixture nothing imports. The never-run integration-progress-model.test.ts is now wired into check:affected:test.

39 files, net −473 lines.

Validation

Tested at 12b9b97: pnpm check:affected --run passed (format, lint, typecheck, daemon-wire-compat, related unit tests, check-affected and mutation node tests, output-economy project). No device run applies.

🤖 Generated with Claude Code

View guided diff Turn on auto-fix

thymikee and others added 3 commits October 7, 2026 22:58
…ests

- DaemonError is NormalizedError; one wire shape instead of two copies
- host-kit: one override-path resolver, inline the canceled-command
  wrapper, and export the abort-aware sleep instead of a second module
- provision-kit: drop the downloadTimeoutMs option no caller supplies
- android observation reads blocking dialogs through app-parsers
- delete shutdownSimulator, providerDeviceAdmission(), the unused
  CoordinateGesturePayload, getCloseShutdown, prefixItems, and
  selectMaestroSnapshotMatches, each reachable only from tests
- import isLikelyStaleSnapshotDrop instead of a verbatim copy
- drop five tests that restated a sibling or their own input, and wire
  the never-run integration-progress model test into check:affected:test

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…CHECKS

Each removed test had a stronger owner in the same or a neighbouring file:
a byte-identical plan-validator case, idle-reap and surface-signature
cases implied by a tolerance test, a perf positional case subsumed by the
case-insensitive one, interactor-catalog subsets of the seven-operation
test, hint regexes duplicated by exact pins, a stub-client clipboard case
covered through the real CLI, and a zip fixture nothing imports.

The check-id union and the ALL_CHECKS array listed the same 59 ids twice;
the type is now derived from the array.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
DaemonError is now declared as an alias of NormalizedError; the members
were already field-for-field identical, so no wire byte changes and the
ack records the moved declaration text.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1

QR code for preview link

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

Built to branch gh-pages at 2026-10-07 21:08 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.13 MB 5.13 MB -1.4 kB
Package (unpacked) 5.13 MB 5.13 MB -1.4 kB
Package (download) 1.54 MB 1.54 MB -366 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.3 ms 26.3 ms -1.0 ms
CLI --help 82.2 ms 81.0 ms -1.2 ms

@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.

No issues found across 39 files

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

@thymikee

thymikee commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

This PR is ready at 12b9b97. The removed seams, dead option fields and duplicate tests look right to me, and I found nothing that needs a change.

Not blocking: wiring scripts/integration-progress-model.test.ts into check:affected:test in package.json (https://github.com/callstack/agent-device/blob/12b9b97/package.json#L151) is a second change in a deletion PR, and that test may not be selected when only scripts/integration-progress-model.ts changes, so you can register it under the gate that owns integration-progress or explain why affected-selector owns it, or leave it as is.

The affected decision-kernel check failed only because two of eight mutant shards (kernel-errors and selectors-2) were cancelled, so it reported "Incomplete shard set: 6 report(s), expected 8". No test failed. I could not see why they were cancelled. selectors-2 does not touch this diff. For kernel-errors, the changes are a type-only alias and three removed tests whose hint assertions are still pinned per code, so I expect the same result. I did not run any tests myself.

There are no conflicts. Please rerun the two cancelled shards so the check gets all eight reports before merge.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 7, 2026

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

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