mirror of https://github.com/software-mansion/argent.git synced 2026-09-14 19:27:14 +08:00

Files

T

Hubert Gancarczyk 2740a3ec38 chore(knip): clear the parked dead-code backlog and drop the ceiling to 0 (#770 )

> Built on top of #769, so this PR carries that commit too — the
workflows only run for PRs targeting `main`, which is why it is based
there rather than on the branch. #769 on its own is the minimal,
low-risk fix for the currently-red job; if it merges first this shrinks
to its own commit, and if this one merges first it closes #769 as well.

## What this does

#572 landed the gate with 215 findings parked under `--max-issues`. That
bounded the backlog but left it in place, and the first thing to touch
it (#663) pushed the total to 218 and turned the job red. This clears
the backlog and sets the ceiling to **0**, so the next unused export
fails the job instead of consuming headroom nobody knew was there.

| | before | after |
| --- | ---: | ---: |
| Unused exports | 47 | 0 |
| Unused exported types | 156 | 0 |
| Unused exported class members | 12 | 0 |
| `--max-issues` | 215 | **0** |

## Scope check first

Every workspace holding a finding is `private: true`.
`@swmansion/argent` — the one published package — had **zero**, so
nothing here moves a public API.

## Two shapes of fix

**Still referenced inside its own file → drop `export`** (146 types, 36
exports). The declaration stays put; it just leaves the module's public
surface. `tsc` keeps emitting these into the `.d.ts` as local
declarations that the exported signatures reference.

For the 146 **types** that is the whole story: they are erased, so the
emitted `.js` is byte-identical. For the 36 **values** it is not.
Dropping `export` from a `const`/`function`/`class` removes its
`exports.X` binding from the emitted `dist/*.js` — 36 declarations
across 26 files here. `packages/tool-server/dist/utils/device-info.js`
no longer carries `exports.REMOTE_PREFIX` or
`exports.VEGA_SERIAL_PREFIX`; `telemetry/dist/posthog.js` loses
`exports.POSTHOG_PROJECT_TOKEN`; in the ESM-emitting `argent-installer`,
`isTempRunnerPath` / `resolveLocalArgentDir` / `PACKAGE_ROOT` leave the
module namespace object entirely.

Nothing in the repo or in the `packages/argent-private` submodule at the
pinned revision reads any of them by path, so nothing breaks. Flagging
it because a `require()` of a built `dist/*.js` loses a dropped `export`
exactly the way it lost the deleted `__resetAndroidDevtoolsInstallCache`
this branch had to restore — so this half of the diff does deserve
runtime scrutiny, not a skim.

**Referenced nowhere → delete.** Orphaned test seams (`__test`,
`_testValidators`), stale compatibility re-exports, and a few real
orphans. The ones worth a reviewer's eye:

- **`localSimctl`** — the `SimctlBackend` strategy is only ever
constructed with `remoteSimctl`. What keeps the local iOS impl off it is
the handler signature: `buildIosLaunchHandler` /
`buildIosRestartHandler` read `services.nativeDevtools`, and
`ios-remote` is the only platform that declares that service, so the
local impl — which resolves native-devtools per device inside its own
handler — has nothing to hand them and shells out to `xcrun` itself.
- **`sendCharInsert`** — removing the `__sendCharInsert` test alias
exposed the underlying helper as unreferenced. It is superseded, not
unwired: the Chromium keyboard impl dispatches its own `type: "char"`
event inline (`keyboard/platforms/chromium.ts:58`), which is exactly
what this helper did. Chromium typing is unaffected.
- **Error metadata fields** — `ServiceNotFoundError.serviceId`,
`ToolNotFoundError.toolId` and their two siblings were never read. The
`e.serviceId` in `registry-error-events.test.ts` is a field on the event
object the handler pushes, not on the error class; the `err.toolId` in
`http.ts` is `NotImplementedOnPlatformError`, a different class that
keeps its fields. Flagging these in case they are wanted as deliberate
diagnostics.

Deleting `SourceMapsRegistry.toGeneratedPosition` / `findMatchingSource`
cascaded into their only helper (`buildSourceCandidates`) and then into
`projectRoot`, which had no remaining reader — so the constructor
parameter goes too. `StubSourceMapsRegistry` existed only to pass
`super("")`, so it goes with it.

## The three cross-workspace members

`ArtifactStore.register`, `ArtifactStore.list` and
`TypedEventEmitter.off` are all called from other workspaces — the
artifact route and screenshot tools in `tool-server`, and
`registry-listener` in `telemetry`. They are reported unused anyway, and
the reason is the **unbuilt tree** the gate runs against: every
workspace resolves `main`/`types` to a `dist/` that does not exist, so
`@argent/registry` resolves to nothing and the cross-workspace edge
never forms. Build the tree and all three findings disappear on their
own; a control member that really is dead is still reported in the built
run, so the pass is live either way. Nothing about this is specific to
`classMembers` — every cross-workspace reference is invisible the same
way.

Each is exempted at the declaration with a `@public` JSDoc tag rather
than by name in `knip.jsonc`, for one reason: **scope**. `ignoreMembers`
matches a name across the whole workspace, so it would also hide a
future dead `register`/`list`/`off` on any other class in
`packages/registry`; `@public` binds to the member it is written on.

What `@public` does **not** buy is a staleness check, and neither does
`ignoreMembers`. Measured on a clean unbuilt worktree: a bogus
`ignoreMembers` name and a redundant `@public` each leave the run at
exit 0 with no hint, while a bogus `ignoreDependencies` name exits 1
with `Remove from ignoreDependencies`. `treatConfigHintsAsErrors` is
live and simply has no producer for either exemption style — so when one
of the three stops being earned, nothing says so, and the JSDoc at each
member is the only record a re-audit has. Each one names its callers,
and `off` records how to re-derive them (rename the member and read the
`Property 'off' does not exist` errors; grepping `.off(` over-reports,
since 14 of the 38 call-site hits are Node `EventEmitter`s). One
correction to the commit message on that change: it says 35 hits where
the exact count is 38 — 14 Node `EventEmitter`, 24 `TypedEventEmitter`
(21 production, 3 in a test). The 14 and the 21 are right.

`knip.jsonc`, `CONTRIBUTING.md` step 5 and the workflow's
failure-explanation step all described a parked backlog and a ceiling to
stay under. All three are rewritten for a gate that must simply come
back empty, and all three now name the `@public` escape hatch — the
remedy for a symbol whose only caller is out of knip's reach.

## Verification

Run against a clean worktree with **no build output**, the way CI
counts:

- `npm run knip` — **exit 0, prints nothing**. Both passes empty, no
config hints.
- `npx tsc --build` — clean.
- `npm run lint` (`eslint . --max-warnings 0`) — clean.
- `npx prettier --check .` — clean.
- `npm test --workspaces` — tool-server 329 files / 3857 passed;
registry, telemetry, update-core, tools-client, mcp, cli green.
- `npm run typecheck:tests --workspaces` — 13 workspaces define the
script and all 13 are clean, but **the command itself exits 1**: npm
errors `Missing script: "typecheck:tests"` for the three that do not
define it rather than skipping them. Worth knowing before re-running
this bullet and reading the exit code as a failure.

End-to-end on a tool-server built from this branch, driven over its HTTP
API:

- **Chromium** (Electron smoke app) — `describe`, `screenshot`,
`gesture-tap` (counter advanced 0 → 1), `POST /api/clipboard/text` read
back out of the renderer, the WS `clipboardSync` command, and
`debugger-connect` / `debugger-status` / `debugger-evaluate`
(`sourceMapReady: true` with `StubSourceMapsRegistry` gone).
- **iOS** — `boot-device`, `launch-app`, `describe` via ax-service,
`screenshot`.
- **tvOS** — `boot-device`, `launch-app` on an Apple TV udid,
focus-driven `describe`. The registry snapshot then carries
`NativeDevtools:<tvOS udid>`, confirming injection **is** resolved on
tvOS through the local impl.
- **Android** — `boot-device` on a dedicated AVD, `describe` via
android-devtools, `gesture-tap`.
- **Metro source maps** — the registry driven against a real 12.5 MB
Metro `.map` over loopback, the way `Debugger.scriptParsed` drives it:
allowlist passes and `waitForPending()` resolves on both, while retained
heap after gc drops from 17.3 MB to 3.6 MB.


## Review follow-ups

Five commits on top, one per finding. The base is back on `main`, so the
workflows run again.

- **`docs(launch-app)`** — the services comment explained the empty
shape through `LaunchAppAndroidServices`, deleted here, and skipped
`LaunchAppVegaServices`, which is still there. Same edit in
`restart-app/types.ts`.
- **`docs(registry)`** — the `@public` tag on `ArtifactStore.register`
named 2 of 7 callers. Derived the full set the way the sibling tag on
`off` prescribes: renamed the member, read the 16 `TS2339` errors `tsc
--build` reports across flow-visual (5), screenshot (4),
native-profiler-stop (2), screenshot-diff (2), native-profiler-analyze,
react-profiler-analyze and screen-recording-stop. The re-derivation
recipe is recorded too — `.register(` greps as badly as `.off(`.
- **`docs(knip)`** — "the build, the tests and this gate all stay green
on a wrong delete" held for argent-private and not for the
cross-workspace case that justifies three of the four `@public` tags.
Deleting `ArtifactStore.register` gives 16 `TS2339` errors and `9 failed
| 2 passed` in `test/artifacts.test.ts`; only the gate stays green.
Split in the workflow comment, the failure annotation and
`CONTRIBUTING.md` step 5.
- **`perf(debugger)`** — `doRegister`'s `data:` branch still
base64-decoded and `JSON.parse`d a payload it dropped, inside a `catch`
that swallows. Both arms returned the same `void`, so the branch
collapses to an early return; `scriptUrl` / `scriptId` go with it, since
their last reader was the deleted `this.maps.push(...)` and `doRegister`
is private.
- **`test(debugger)`** — the allowlist check moved to a top-level early
return with nothing pinning it: delete the line and both SSRF files
still pass 17/17. Two tests now assert `doRegister` consults it — four
rejected URLs reach no `fetch`, and the loopback `*.map` Metro emits
still does.

Re-verified after: `npm run knip` exit 0 and empty on a `tsc --build
--clean` tree, `tsc --build`, `eslint . --max-warnings 0`, `prettier
--check .` all clean, and `npm test --workspaces` with tool-server at
329 files / 3854 passed. The one red test, `argent-installer`'s
`globalPath returns ~/.config/opencode/opencode.json`, fails identically
with these commits stashed — it reads the real `~/.config/opencode`,
which holds a `.jsonc` on this machine.

End-to-end on a tool-server run from this branch over its HTTP API, on
every platform:

- **Chromium** — `launch-app`, `describe`, `screenshot` (returned an
artifact handle, `GET /artifacts/<id>` served the 137542-byte PNG back),
`gesture-tap` (counter 0 → 1), `debugger-connect` / `debugger-status`
(`sourceMapReady: true`) / `debugger-evaluate` (read `taps: 1` out of
the page).
- **iOS** — `launch-app`, `describe` via native-devtools, `restart-app`,
`screenshot`.
- **tvOS** — `launch-app`, focus-driven `describe` (12 focusables in
TVSettings), `tv-remote`, `restart-app`.
- **Android** — `launch-app`, `describe` via android-devtools,
`restart-app`, `screenshot`.
- **Vega** — `boot-device` on the `tv` VVD, `describe` via the
automation toolkit.
- **Metro** — the real `debugger-connect` / `debugger-status` /
`debugger-evaluate` tools against a stand-in Metro (HTTP `/json/list` +
CDP WebSocket) emitting three `Debugger.scriptParsed` events: a loopback
`*.map` on a counting server, a 1.4 MB inline `data:` map, and
`http://169.254.169.254/latest.map`. `sourceMapReady: true`,
`loadedScripts: 3`, evaluate 42, the loopback map fetched exactly once
and the metadata URL never. A malformed inline payload gives
byte-identical output, which is the finding.

### Merge with `main`

Putting the base back on `main` made CI resolve the merge, and the Unit
Tests job went red on a conflict no side could see on its own: `#771`
fixed the same red gate this branch's first commit did, the other way
round — it made `component-names.test.ts` import `StrippedName`,
`ComponentAnnotation` and `ComponentNameResolution` instead of dropping
their `export`. Merged `main` in, reproduced it (`tsc --noEmit -p
tsconfig.test.json` → three `TS2459`s) and took main's resolution: the
three exports are back. The gate stays green because knip treats test
files as entry points, so a type a test imports is not an unused export.

All nine workflows are green on the head commit.

2026-08-25 17:27:12 +02:00

9.1 KiB

Raw Permalink Blame History

Contributing to Argent

Thank you for your interest in contributing to Argent! This guide covers everything you need to get started.

Requirements
Setting up the dev environment
Project structure
Building
Running the project
Running tests
Code style
Submitting a pull request

Requirements

macOS with Xcode installed (required for xcrun simctl and iOS simulator support)
Node.js 20.19+ (the lint toolchain's floor; the published package needs 20.12+)
The simulator-server and ax-service binaries in packages/native-devtools-ios/bin/ (arm64 macOS, installed separately via npx @swmansion/argent install)

Setting up the dev environment

Fork and clone the repository:

git clone https://github.com/software-mansion/argent.git
cd argent

Install dependencies (npm workspaces installs all packages at once):
```
npm install
```
Start the dev environment:
```
npm run dev
```
This builds the native devtools dylibs (if the private submodule is available, otherwise uses pre-built binaries), compiles the MCP TypeScript, patches ~/.claude.json to point at the local MCP, and starts the tool-server from source via ts-node. Press Ctrl+C to stop — the script automatically restores your global Argent configuration.

To use a different port:
```
PORT=4000 npm run dev
```
Note: packages/argent-private is a private git submodule that holds the ObjC source for the native devtools dylibs. If you don't have SSH access to it, npm run dev will use the pre-built dylibs committed to the repository — everything else works normally.

That's it, no separate install steps per package are needed, except packages/docs. It is excluded from the workspace glob and keeps its own lockfile, so npm run lint from the root needs npm install run inside packages/docs first.

Project structure

This is an npm workspaces monorepo. All packages live under packages/:

Package	Path	Purpose
`@argent/registry`	`packages/registry`	Core library: dependency-aware service lifecycle, blueprints, tools, URNs
`@argent/tool-server`	`packages/tool-server`	HTTP API over the registry (port 3001). Registers all blueprints and tools
`@swmansion/argent`	`packages/argent`	Published bin shell — minimal dispatcher + bundled subcommands + native runtime assets
`@argent/tools-client`	`packages/argent-tools-client`	HTTP client + tool-server launcher shared by `@argent/mcp` and `@argent/cli`
`@argent/mcp`	`packages/argent-mcp`	MCP stdio protocol adapter — proxies Claude/Cursor calls to the tool-server
`@argent/cli`	`packages/argent-cli`	Shell CLI commands: `argent run`, `argent tools`, `argent server`, `argent enable`, `argent disable`, `argent flags`
`@argent/installer`	`packages/argent-installer`	Workspace setup logic: `argent init`, `update`, `uninstall`
`@argent/skills`	`packages/skills`	Markdown skill files (prefixed `argent-*`) that instruct AI agents how to use Argent tools
`@argent/native-devtools-ios`	`packages/native-devtools-ios`	Pre-built dylibs for iOS simulator injection (view hierarchy, network inspection). ObjC source lives in `packages/argent-private` (private submodule)

The tsconfig.json at the root uses TypeScript project references; tsconfig.base.json holds shared compiler options (strict, ES2022, etc.).

Building

Local development (recommended)

npm run dev

Builds everything and starts the tool-server from source. See Setting up the dev environment for details.

Full build

Build all packages at once using TypeScript project references:

npm run build

To build a specific package:

npm run build -w @argent/registry
npm run build -w @argent/tool-server

To build and bundle the distributable MCP package:

npm run build -w @swmansion/argent
# or, to also produce a .tgz tarball:
npm run pack:mcp

Running the project

For day-to-day development, use npm run dev (see Setting up the dev environment).

Production-like start (builds bundles first, then starts from compiled output):

npm run start

This builds the registry, then starts the tools server on port 3001.

Verify the tools server is up:

curl http://localhost:3001/tools

Running tests

Tests are written with Vitest. Each package has its own test suite.

Run tests for a specific package:

npm test -w @argent/registry
npm test -w @argent/tool-server

Run tests in watch mode during development:

npm run test:watch -w @argent/registry
npm run test:watch -w @argent/tool-server

Code style

TypeScript strict mode is enabled across all packages ("strict": true in tsconfig.base.json). All code must compile without errors.
Target: ES2022 with CommonJS modules (except @swmansion/argent which uses ESM).
Prefer explicit types over any. Use Zod schemas for runtime validation where the codebase already does so.
Keep commits focused. Prefix commit messages with a type: feat:, fix:, chore:, docs:, refactor:, test:. This feeds the auto-generated changelog on release.

Submitting a pull request

Create a branch from main with a descriptive name (e.g. feat/add-screenshot-tool, fix/session-leak).
Make your changes. Keep the scope of a PR small and focused — it's easier to review.
Ensure the build passes:
```
npm run build
```
Ensure tests pass for the packages you touched.
Check for dead code, in a tree with no build output:
```
npx tsc --build --clean
npm run knip
```
Run it this way round, not straight after step 3. The gate is counted against an unbuilt tree, because that is what CI analyses; with packages/*/dist present knip finds fewer issues, so a built-tree run carries phantom headroom and can pass locally while the Dead Code job fails. This leaves the tree unbuilt, so run npm run build again before re-running any test suite. The gate runs at --max-issues 0, so the report must come back empty and there is no ceiling to raise: when knip names an export, type or class member you added, drop the export keyword if the symbol is still used inside its own file, or delete it if nothing uses it. A class member has no export keyword to drop, so it is delete or tag. When the only caller is somewhere knip cannot see — another workspace, because the unbuilt tree breaks cross-workspace edges, or the argent-private submodule, which CI does not check out — tag the declaration /** @public */ and name that caller in the comment. That last case is what Unused exported class members almost always is here: the three tagged members in packages/registry are each called from another workspace, and pass 2 reports all three the moment a tag comes off. Look for it before you delete. This gate stays green on a wrong delete either way, but the two cases differ after that: deleting a symbol another workspace calls turns npm run build and that workspace's tests red, so that mistake is caught, while deleting one only argent-private reaches breaks nothing here — it require()s the built dist/*.js, and CI never checks that submodule out.
Write a clear PR title — it becomes part of the release changelog. Use the same prefix convention as commit messages (feat:, fix:, etc.).
Open the PR against main and fill in the description with context on what changed and why.
A maintainer will review and may request changes. Address feedback with new commits (don't force-push after review starts).

Questions?

If you're unsure about something, open a GitHub Discussion or leave a comment on the relevant issue before spending time on a large change.

9.1 KiB Raw Permalink Blame History