chore: codebase cleanup pass (dedup, types, dead code, cycles)#420
Merged
chore: codebase cleanup pass (dedup, types, dead code, cycles)#420
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3115ac6416
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const logPathOnFailure = flushDiagnosticsToSessionFile({ force: true }) ?? undefined; | ||
| const normalizedError = normalizeError( | ||
| new AppError(response.error.code as any, response.error.message, { | ||
| new AppError(toAppErrorCode(response.error.code), response.error.message, { |
There was a problem hiding this comment.
finalizeDaemonResponse now coerces handler error codes via toAppErrorCode(), which drops codes not in the AppErrorCode union. find still emits AMBIGUOUS_MATCH (src/daemon/handlers/find.ts), so ambiguous-find failures are now rewritten to COMMAND_FAILED before they reach clients. This removes a previously available signal that callers can use to handle ambiguity differently (for example, prompting to narrow selectors or retrying with --first/--last), so unknown daemon codes should be preserved or AMBIGUOUS_MATCH should be added to the accepted code set.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Good catch — fixed in 29a34a8. The narrower fix (just adding AMBIGUOUS_MATCH to the union) would have left the class-of-bug in place, so I widened the wire boundary instead:
AppErrorCodeis nowKnownAppErrorCode | (string & {})— keeps IDE autocomplete of known codes while accepting any wire stringtoAppErrorCodenow preserves any non-empty code verbatim; fallback only kicks in for undefined/empty- Added
AMBIGUOUS_MATCHtoKnownAppErrorCode(it's documented at skills/agent-device/references/exploration.md:340, belongs on the autocomplete list) - Added test coverage: unknown codes are preserved, fallback is only used for missing/empty codes
No more silent downgrades for any future handler-emitted code.
|
- Delete interaction-get.ts, interaction-is.ts, interaction-selector.ts (superseded by selector-runtime dispatchers) - Remove handleWaitCommand and its private helpers from snapshot-wait.ts (replaced by dispatchWaitViaRuntime) - Remove unused distanceFromSafeViewportBand/isRectWithinSafeViewportBand from rect-visibility - Trim Linux platform barrel to only the re-export still in use (snapshotLinux)
- Move sleep() to utils/timeouts.ts; remove 6 duplicate implementations - Use existing isApplePlatform() helper for ios||macos checks (5 sites) - Export trimRuntimeValue from runtime-hints; drop duplicate trimRuntimeString - Merge normalizeTextSurfaceType/normalizeType into single text-surface helper - Remove pointless isScrollableContainerType wrapper
- SnapshotDiffLine/Summary: single definition in utils/snapshot-diff, re-exported from capture-snapshot - FindLocator: single definition in utils/finders, re-exported from client-types - JsonRpc envelope in http-server now uses JsonRpcRequestEnvelope/JsonRpcId from contracts - Inline 'primary'|'secondary'|'middle' literals replaced with ClickButton (internal sites only)
- Add toAppErrorCode() validator and DEVICE_IN_USE to AppErrorCode union - Replace 'as any' casts on wire error codes with the validator (daemon-error, daemon-client, request-router, http-server) - Type Metro worker payload as MetroTunnelResponseMessage - Type xctestrun plist parsing with explicit partial schema - Narrow 'details.stderr' via typeof checks on Android error paths - Type runner-session parseRunnerResponse with RunnerResponsePayload Bug fix: handler emitted 'DEVICE_IN_USE' but router cast silently downgraded to 'COMMAND_FAILED' because the code was missing from AppErrorCode. Clients can now react to same-device contention.
- Replace 16 instances of 3-line catch { // ignore } with 1-line catch {}
- Remove one self-describing 'Re-export public API' comment
Specific 'ignore shutdown races' / 'ignore malformed pid files' style comments
that name concrete failure modes are kept.
…to leaves Resolves all 44 cycles reported by madge. Pattern throughout: extract shared type into a leaf module; both producer and consumer import from the leaf; original module re-exports for API stability. New leaf type modules: - src/runtime-contract.ts (AgentDeviceRuntime, CommandContext, ...) - src/metro-types.ts (MetroRuntimeHints, MetroBridgeResult, ...) - src/commands/runtime-types.ts (CommandResult, RuntimeCommand, ...) - src/commands/diagnostics-types.ts - src/cli/commands/router-types.ts (ClientCommandParams, ...) - src/core/interactor-types.ts (Interactor, BackMode, ...) - src/platforms/ios/runner-session-types.ts (RunnerSession) - src/utils/screenshot-diff-region-types.ts (MutableDiffRegion) - src/daemon/handlers/record-trace-types.ts madge --circular now reports 0 cycles (was 44). No runtime behavior changes.
The initial weak-types pass validated wire error codes against a closed
union, silently downgrading any unknown code to COMMAND_FAILED. This
dropped signals like AMBIGUOUS_MATCH that handlers emit and clients are
documented to handle (skills/agent-device/references/exploration.md).
- Widen AppErrorCode to 'KnownAppErrorCode | (string & {})' so autocomplete
of known codes is preserved while any wire code flows through
- toAppErrorCode now preserves any non-empty code; fallback only when
undefined or empty
- Add AMBIGUOUS_MATCH to KnownAppErrorCode (documented public code)
- Add test coverage for preservation and fallback behavior
- Add DEVICE_IN_USE to the batch error taxonomy in exploration.md (now
observable by clients after earlier fix, needs agent-facing guidance)
- Delete one-line src/platforms/linux/index.ts barrel; both consumers
(core/dispatch, daemon/handlers/snapshot-capture) now import from
platforms/linux/snapshot directly
- Replace inline { tenantId; runId; leaseId } shapes with MetroBridgeScope
alias at client-types, metro, and cli/commands/connection-runtime
- Consolidate remaining inline setTimeout wrappers onto utils/timeouts.ts#sleep
(12 files, ~18 sites). Left test files and the runtime-clock aware helper
in commands/selector-read-utils alone. Also removes the local sleepMs
helper from daemon-client.ts.
- daemon-client RPC error path: stringify any non-null data.code instead
of only forwarding strings. Preserves numeric codes from hypothetical
future proxies/servers; for first-party daemon today this is a no-op
since handlers already emit strings.
- errors.ts: expand AppErrorCode comment to call out the exhaustiveness
tradeoff of the '(string & {})' widening for SDK consumers.
thymikee
force-pushed
the
chore/code-quality-cleanup
branch
from
bba4d9f to
47c70c4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Mechanical code-quality pass delegated to six focused subagents, each reviewed and merged as a distinct commit. Every intermediate state and the final branch pass
typecheck,lint,test(155 files / 1384 tests),test:smoke(8/8), andbuild.madge --circulardrops from 44 cycles to 0.Net lines: -566 across
src/(the circular-deps pass reshuffles ~500 lines into leaf modules; all other passes are net-negative).Included passes (read as separate merge commits for easier review)
interaction-get.ts/interaction-is.ts/interaction-selector.ts(superseded byselector-runtimedispatchers); striphandleWaitCommand+ private helpers fromsnapshot-wait.ts(replaced bydispatchWaitViaRuntime); remove unused rect-visibility helpers; trim unused Linux platform barrel re-exports. -568 lines, 6 files.sleep()inutils/timeouts.ts(6 copies removed); use existingisApplePlatform()forios || macoschecks; merge duplicatenormalizeType/trimRuntimeString. -42 lines, 15 files.SnapshotDiffLine/SnapshotDiffSummary,FindLocator,JsonRpcRequestEnvelope/JsonRpcId,ClickButton(internal sites). No public SDK shape changes — duplicated types now re-export from the canonical home. 7 files.as anycasts on wire error codes replaced with a runtime-validatedtoAppErrorCode()helper. While converting, surfaced a real bug:session-open.tsemitsDEVICE_IN_USEbut it was missing from theAppErrorCodeunion, so the daemon→client round-trip silently downgraded it toCOMMAND_FAILED. Clients can now react to same-device contention. Also: Metro worker payload, xctestrun plist, and runner-session parser now have proper types. 10 files.catch { // ignore }blocks tocatch {}. Specific comments that name a concrete failure mode (e.g. "ignore shutdown races") are kept. 9 files.madgeby extracting shared types into leaf modules and pointing both producer and consumer at the leaf; original modules re-export for API stability. New leaves:runtime-contract.ts,metro-types.ts,commands/runtime-types.ts,commands/diagnostics-types.ts,cli/commands/router-types.ts,core/interactor-types.ts,platforms/ios/runner-session-types.ts,utils/screenshot-diff-region-types.ts,daemon/handlers/record-trace-types.ts. No runtime behavior changes.Passes attempted that produced zero changes
Test plan
pnpm typecheck— cleanpnpm lint— 0 warnings / 0 errorspnpm test— 155 files / 1384 tests passpnpm test:smoke— 8/8 passpnpm build— succeeds (rslib)pnpm dlx madge --circular --extensions ts src/— 0 cycles (was 44)