Skip to content

refactor: P3 polish — Interactor type safety, error response builder, ESLint#316

Open
thymikee wants to merge 6 commits intomainfrom
refactor/p3-polish
Open

refactor: P3 polish — Interactor type safety, error response builder, ESLint#316
thymikee wants to merge 6 commits intomainfrom
refactor/p3-polish

Conversation

@thymikee
Copy link
Copy Markdown
Contributor

@thymikee thymikee commented Mar 28, 2026 •
edited
Loading

Summary

This PR is a P3 polish/refactor pass against main, with three concrete outcomes.

  • Adds repo-native linting and formatting with OXC (oxlint + oxfmt) instead of introducing a heavier ESLint + Prettier stack.
  • Tightens TypeScript enforcement for unused code (noUnusedLocals, noUnusedParameters) and enables isolatedModules so type-checking better matches the actual build pipeline.
  • Improves the iOS runner architecture by extracting a shared runner-contract.ts seam for RunnerCommand and runner connect/error helpers.

Additional cleanup included in the same scope:

  • Upgrades the repo to TypeScript 6 and aligns @rslib/core so the compiler and declaration build path stay internally consistent.
  • Removes the accidental package-lock.json addition so the repo stays on a single package manager (pnpm).
  • Fixes the concrete cleanup issues surfaced by the stricter tooling rather than suppressing them.

Why this is better than the base branch:

  • main had no repo-level lint/format enforcement here; this adds one, but does it with the smaller/faster OXC toolchain rather than adding parallel ESLint + Prettier maintenance.
  • Unused code is now enforced by both lint and TypeScript, and the branch runs cleanly under the repo’s real build/test workflow.
  • The iOS runner path has a cleaner dependency direction than the current refactor had: runner-transport.ts no longer imports back from runner-client.ts; both now depend on a small leaf contract module.
  • The refactor improves boundaries and toolchain safety without changing command behavior.

Validation

  • pnpm format
  • pnpm lint
  • pnpm typecheck
  • pnpm build
  • pnpm test:unit
  • pnpm test:smoke

@thymikee @claude
refactor: polish improvements from codebase review (P3)
89920e2 
- Export Interactor type and add `satisfies Interactor` to both return
  objects in getInteractor() for compile-time completeness checking
- Create errorResponse/successResponse builder in handlers/response.ts
  and adopt it in 5 representative handler files (find, record-trace,
  interaction-press, session-deploy, snapshot-wait)
- Add ESLint with typescript-eslint, fix unused imports and trivial
  lint issues, configure rules to warn for stylistic patterns

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@chatgpt-codex-connector
Copy link
Copy Markdown

chatgpt-codex-connector bot commented Mar 28, 2026

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

thymikee and others added 3 commits March 28, 2026 08:18
@thymikee @claude
refactor: complete P3 items — consolidate runner files, document macO…
8bd4d3a 
…S exports, standardize dispatch

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@thymikee @claude
fix: address P3 review feedback — remove invalid ESLint rules, dead c…
4de9baf 
…ode, unused export

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@thymikee
chore: replace eslint and prettier with oxc
c98ff14
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 29, 2026
View reviewed changes
src/client-metro.ts

function shellQuote(value: string): string {
return `'${value.replace(/'/g, `'\"'\"'`)}'`;
return `'${value.replace(/'/g, `'"'"'`)}'`;

Check warning

Code scanning / CodeQL

Unsafe shell command constructed from library input Medium

This string concatenation which depends on
library input
Loading
is later used in a
shell command
Loading
.

Copilot Autofix

AI about 6 hours ago

General approach: Avoid manually building a shell command string with ad-hoc quoting. Instead, use a standard library (shell-quote) to escape arbitrary strings before embedding them in a shell command. This keeps the /bin/sh -c pattern (needed for nohup ... & echo $!) but ensures all untrusted inputs (metro.command, its args, and logPath) are correctly escaped for the shell.

Concrete fix in this file:

  1. Add an import for shell-quote in src/client-metro.ts.
  2. Replace the custom shellQuote helper with a thin wrapper around shell-quote’s quote function, so it still returns a string suitable for inclusion in a shell command.
  3. Leave the existing use sites in startMetroProcess as-is (shellQuote(metro.command), metro.installArgs.map(shellQuote), and shellQuote(logPath)), now backed by the library implementation.

This preserves all existing behavior (still using /bin/sh -c with nohup ... & echo $!, same function signatures, same return types) while eliminating the home-grown quoting logic that CodeQL flags. Changes are limited to src/client-metro.ts: one new import at the top, and a new implementation of shellQuote near lines 243–245.

Suggested changeset 2
src/client-metro.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/client-metro.ts b/src/client-metro.ts
--- a/src/client-metro.ts
+++ b/src/client-metro.ts
@@ -3,6 +3,7 @@
 import { AppError } from './utils/errors.ts';
 import { runCmdSync } from './utils/exec.ts';
 import { resolveUserPath } from './utils/path-resolution.ts';
+import shellQuoteLib from 'shell-quote';
 
 export type MetroPrepareKind = 'auto' | 'react-native' | 'expo';
 type ResolvedMetroKind = Exclude<MetroPrepareKind, 'auto'>;
@@ -241,7 +242,8 @@
 }
 
 function shellQuote(value: string): string {
-  return `'${value.replace(/'/g, `'"'"'`)}'`;
+  // Use a well-tested library to safely escape values for the shell.
+  return shellQuoteLib.quote([value]);
 }
 
 function installDependenciesIfNeeded(
EOF
@@ -3,6 +3,7 @@
import { AppError } from './utils/errors.ts';
import { runCmdSync } from './utils/exec.ts';
import { resolveUserPath } from './utils/path-resolution.ts';
import shellQuoteLib from 'shell-quote';

export type MetroPrepareKind = 'auto' | 'react-native' | 'expo';
type ResolvedMetroKind = Exclude<MetroPrepareKind, 'auto'>;
@@ -241,7 +242,8 @@
}

function shellQuote(value: string): string {
return `'${value.replace(/'/g, `'"'"'`)}'`;
// Use a well-tested library to safely escape values for the shell.
return shellQuoteLib.quote([value]);
}

function installDependenciesIfNeeded(
package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -68,7 +68,8 @@
     "android"
   ],
   "dependencies": {
-    "pngjs": "^7.0.0"
+    "pngjs": "^7.0.0",
+    "shell-quote": "^1.8.3"
   },
   "devDependencies": {
     "@microsoft/api-extractor": "^7.52.10",
EOF
@@ -68,7 +68,8 @@
"android"
],
"dependencies": {
"pngjs": "^7.0.0"
"pngjs": "^7.0.0",
"shell-quote": "^1.8.3"
},
"devDependencies": {
"@microsoft/api-extractor": "^7.52.10",
This fix introduces these dependencies
Package Version Security advisories
shell-quote (npm) 1.8.3 None
Copilot is powered by AI and may make mistakes. Always verify output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@thymikee