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

Files

T

Ignacy Łątka c876fe8dbc ci: add knip anti-dead-code gate (#572 )

## What

Adds a CI gate for dead code, using [Knip](https://knip.dev).

We didn't have one. ESLint's `no-unused-vars` only sees unused locals
inside a module, so whole files nobody imports, exports nobody uses, and
dependencies declared but never required all slipped through. Knip
covers that gap in one tool (roughly `ts-prune` + `depcheck` +
`unimported` combined).

The new `.github/workflows/knip.yml` workflow ("Dead Code", job `Knip`)
runs `npm run knip` on push and PR to `main`. `CONTRIBUTING.md`'s
pull-request checklist gains it as a step, with the reason it has to run
against a tree with no build output. Knip reads source directly, mapping
each package's `dist` entry back to `src`, so the job needs no workspace
build. It installs with `--ignore-scripts`: an analysis that only reads
`package.json` and source gains nothing from the seven packages here
that carry a native install script, six of which reach this runner
(`fsevents` is darwin-only). Five try a prebuilt binary before
compiling, and `dtrace-provider` always runs node-gyp but ends in `||
node suppress-error.js` — and off mac/solaris/freebsd its `binding.gyp`
picks a stub target with no sources, so on ubuntu-latest node-gyp builds
nothing for it and `node-pty` is the only package that reaches a
compiler at all — an `optionalDependency`, whose failure npm ignores.
The exposure is a bump that leaves a package with no prebuild for the
runner, which is exactly what `dependabot.yml` pins `tree-sitter`
against (0.25.0 ships no Node 24 prebuild and its compile breaks `npm
ci`).

While adding it I checked whether any other installed checker was
missing a gate. None were: ESLint (`lint.yml`), Prettier (`format.yml`),
`tsc` build + `typecheck:tests` + `typecheck:scripts` + vitest
(`unit-tests.yml`), the workspace-version / focused-test / merge-marker
checks (`repo-hygiene.yml`) and lockfile sync (`lockfile.yml`) are all
already enforced. Dead code was the only gap.

## How it's enforced

`npm run knip` is two invocations:

```
knip --exclude exports,types && knip --include exports,types,classMembers,nsExports,nsTypes --max-issues 206
```

Every issue type knip reports is an error — that is its default
severity, so `knip.jsonc` sets no `rules` block at all. The pre-existing
backlog is bounded by the `--max-issues` ceiling on the second pass
instead of being hidden behind a non-failing severity: it cannot grow
unnoticed, and the number comes down as follow-up passes pay it off.
When a change legitimately adds an export nothing imports yet, the
number moves up in the same commit — that bump is what review should
see, rather than the contributor deleting someone else's backlog to make
room. Two PRs making that identical one-line edit merge without
conflicting, so `main` lands one short of the truth and its
push-triggered run is what catches it. Splitting the passes also keeps a
real failure out from under the backlog — the enforced classes report on
their own, and the second pass only runs once the first is clean.

The second pass names `classMembers`, `nsExports` and `nsTypes`
explicitly because they sit in knip's own `defaultExcludedIssueTypes`:
`--exclude exports,types` never covered them and `--include
exports,types` did not ask for them, so a new unused class member cost
nothing and moving an export behind opaque namespace use took it out of
the ceiling's scope entirely. `classMembers` has 12 findings on the
unbuilt tree the ceiling counts (9 on a built one); the other two have
none today. None of the three is gated at zero — all five parked classes
share the one ceiling, so a new `nsExports` finding passes if something
else in the backlog is retired in the same PR. Naming `classMembers`
also turns knip's `isSkipLibs` off (`create-options.js:122`), which
makes it search declaration files for external references and
**suppresses** 44 export/type findings the same run reports without it —
57/181 becomes 44/150. That, not the 12 findings it adds, is why the
ceiling is lower than the two counts alone would suggest;
`--include-libs` with no `classMembers` reproduces 44/150 exactly.

The ceiling counts what CI counts, against a tree with no build output.
Whether a workspace's `main`/`types` path resolves to an existing file
changes the result, and not by way of anything in that file — a
zero-byte `packages/registry/dist/index.js` moves the count just as a
real build does, while a zero-byte `dist/zzz.js` does nothing. A built
tree reports fewer issues — 43/147/9 = 199 built, against 44/150/12 =
206 unbuilt — so `npm run knip` passing after a local `tsc --build` is
not evidence the job will pass, which is how the Knip job first went red
on 199. The ceiling is 206 and it is exact: 206 passes, 205 fails.
Remove every `dist/` before trusting a number from it.

The ceiling is one total across those classes, so it bounds the backlog
rather than each class: paying off one parked export licences one new
one elsewhere in the same PR. Knip has no per-class ceiling.

`treatConfigHintsAsErrors` is on, so an exemption that stops suppressing
anything fails the job rather than printing a hint below the backlog.

## Config

`knip.jsonc` documents the false positives so the gate is both green and
meaningful.

- **`@swmansion/argent`** is a thin dispatcher whose real payload is an
esbuild bundle of the other workspaces
(`packages/argent/scripts/bundle-tools.cjs`). Knip can't follow imports
across that boundary, so packages consumed only by the bundled-in code
read as unused; 11 are exempted by name here, though not all for that
reason — the http3-quiche transport is pulled in by
`@fails-components/webtransport` itself at runtime, and no first-party
file imports it. They're exempted **by name**, not behind a catch-all:
`ignoreDependencies` also suppresses `unlisted`, and a pattern that
matches everything would take this workspace's own `src/` and `scripts/`
out of that check — where an import with no matching declaration is a
genuine bug rather than a bundler artifact. (It reaches `unresolved`
only nominally: knip matches the list against a package name parsed out
of the specifier, and an unresolvable relative path yields none, so even
a catch-all leaves those reported. `ignoreUnresolved` is the option that
does silence them.)
- **`tool-server`** reaches a few packages in ways static analysis can't
see. The exemptions cover two distinct classes, kept apart in the config
so each stated reason matches the finding it silences:
- *declared but loaded too dynamically to detect* (would be reported as
**unused dependencies**): `tree-sitter` / `tree-sitter-typescript` via
dynamic `_require`, and the http3-quiche transport as a runtime
companion of `@fails-components/webtransport`;
- *imported but deliberately absent from that `package.json`* (would be
reported as **unlisted dependencies**): `sharp` (kept out so the ~30 MB
native binary isn't a cost every consumer pays) and `electron` (an
`optionalDependency` of the published package at one call site, the
Electron main-process API at the other).
- Root `scripts/`: knip resolves those from `package.json` scripts
**and** from `run:` steps in `.github/workflows` via its github-actions
plugin — multi-line `run: |` blocks included. What the plugin doesn't
parse is a PowerShell body, so `scripts/ci/windows-chromium-e2e.mjs` —
invoked at `windows-e2e.yml:176`, in the `windows-latest` job where
PowerShell is the default shell — is the one entry declared by hand.

### Known limits

- `ignoreDependencies` exempts a package **name across its whole
workspace**, in both directions; knip has nothing narrower. While
`sharp` is listed for `tool-server`, an undeclared `import "sharp"`
anywhere in that workspace passes too. The list is held to packages that
genuinely cannot be declared, for exactly that reason.
- Knip analyzes JavaScript and TypeScript only (`DEFAULT_EXTENSIONS`),
so **only 11 of the 30 files under `scripts/` are in the gate at all**.
The other 19 — 15 `.sh` files, `e2e-full/lib/report.py`,
`ci/e2e-chromium-page.html`, and e2e-full's `README.md` and
`results/.gitignore` — are invisible to it however they are reached —
including the whole `scripts/e2e-full/` harness (13 files), invoked from
nothing outside its own directory, its only mention elsewhere being a
`.prettierignore` entry.
- Reachability's widest hole is the workspace entry. Whatever
`src/index.ts` re-exports is reached, so a single `export { x } from
"./dead-module";` takes that module out of `files` altogether — verified
against a module reported as an unused file one run earlier. The same
line applied to parked exports retires them from the count without
deleting anything: re-exporting `equalsCI`, `includesCI` and `Settled`
from `tool-server`'s entry takes 206 to 203 and frees three ceiling
slots. The ceiling bounds what knip can see, not what is dead.
- Test files are entry points, so a production module kept alive only by
its own test still passes `files`. Being reached by a test says nothing
about being reachable in production, which is what the check
approximates, and two modules sit in exactly that gap: `tools/paste` is
imported by two test files and registered by nothing, so it is
unreachable over MCP and HTTP, and `utils/ios-profiler/lifecycle.ts` is
a re-export barrel whose header keeps it "for source compatibility with
existing imports", of which its own test is now the only one — its four
production consumers all import `profiler-shared/lifecycle` directly.
That single test import is exactly what keeps `files` from reporting it:
repoint it and knip returns `Unused files (1)`. `knip --production` is
the lens that reports the class; the repo isn't clean under it — 13
unused files (those two, seven root scripts,
`packages/argent/scripts/{bundle-tools,observe,sync-readme}.cjs` and
`packages/registry/tests/helpers.ts`) with the export/type backlog at
227/206 — measured with `classMembers` off, so comparable to the 57/181
above rather than to the ceiling's 44/150 — so a production-mode pass is
a follow-up of its own.
- The same entry treatment hides the test trees themselves. Root
`package.json`'s `test:scripts` runs `node --test`, which is what makes
knip's node plugin contribute its test entries, and that plugin
registers four entry patterns in **every** workspace: `**/test/**`, plus
`*{.,-,_}test.<ext>`, `test-*.<ext>` and `test.<ext>` matched anywhere.
So none of the **380** JS/TS files under `packages/*/test/` can be
reported unused, and under `packages/*/tests/` only a name none of those
three match can — of today's 15 that is `registry/tests/helpers.ts`
alone, which is why it is the lone `tests/` entry in the `--production`
list above. A `tests/test-utils.ts` or `tests/test.ts` would be just as
invisible. A dead `zz.test.ts` planted in `packages/registry/tests/`
passes; a dead `zz-helper.ts` in the same directory fails.
- `knip@5.88.1` is the last 5.x release ever published — 6.0.0 landed
the next day and `latest` is now 6.x. The pin is deliberate: knip 6
removed the `classMembers` issue type outright, and the shipped script
hard-errors on it (`ERROR: Invalid issue type: classMembers`, exit 2).
Upgrading is therefore not a version bump — it means re-deriving the
ceiling, rewriting the paragraph that explains it, and resolving the
pass-1 findings knip 6 adds (six unlisted binaries: `tail`, `plutil`,
`xctrace`, `netstat`, `pkill`, `pgrep`; two unresolved imports:
`/usr/bin/osascript`, `/usr/libexec/PlistBuddy`). Dependabot's
`npm-minor-patch` group cannot move it, since no 5.x exists above
5.88.1; the `npm-major` group will keep offering the 6.x jump.
- Knip folds `optionalDependencies` into the declared set, so importing
`electron` or `node-pty` is not `unlisted` — they pass because it read
them. What it never reports is an optionalDependency nothing uses, so
only that direction is outside the gate, along with `argent-cli`'s
runtime `req("node-pty")`, resolved through a
`createRequire(import.meta.url)` in `lens-pty.ts`, which knip can't see
either.

## Dead code this found

- `@types/semver` and `@argent/telemetry` from `packages/argent` — both
were being suppressed by exemptions whose stated bundler-boundary reason
could not apply. esbuild strips types and never resolves `@types/*`, and
`packages/argent` names `semver` nowhere; `@argent/telemetry` is aliased
to an absolute source path by `bundle-tools.cjs`, so the declaration
bought nothing. Removing them is proved safe by building: four of the
five bundles still emit and telemetry is inlined in each with no runtime
`require` of it. The fifth, preview-window's main, sits behind a
downloaded native asset absent from a script-free install, so it is
covered by mechanism rather than observation — `bundle-tools.cjs` passes
the same `alias` map to every build, and esbuild's `alias` bypasses node
resolution entirely: deleting `node_modules/@argent/telemetry` outright
leaves all four emitted bundles byte-identical.
Removed alongside, but **not** gate-reported: the `benchmark` npm script
in `packages/argent`, whose `scripts/benchmark.cjs` does not exist. A
dangling script target is a blind spot of this gate — restoring the
script leaves knip at exit 0.

- Four files with no importers: the `render-hook` injected script,
react-profiler's `source-map` + `snippet-extractor`, and
`scripts/setup-in-workspace.cjs` (referenced nowhere, and its own usage
string names a `scripts/setup-project.cjs` that doesn't exist).
- `superagent` in `tool-server`'s `test/http-streaming.test.ts` —
imported for a type but declared in no manifest in the repo, resolving
only through npm hoisting of supertest's own dependency. Reverting that
import to `superagent` reports `Unlisted dependencies (1) … :3:32` and
exits 1; `@types/supertest/index.d.ts:10` re-exports the identical type
(`type Response = superagent.Response`).
- `@clack/prompts` and `@moq/lite` from `tool-server`'s `package.json` —
declared, imported nowhere. (`@moq/net` is the one actually used;
`@moq/lite` isn't a peer of it.)

Also deleted here, but **not** something this gate reported:
`src/tools/simulator/simulator-server.ts`. It sat in the `files` blind
spot above — `interaction-messages.test.ts` imported it, which is enough
to count as reached. `setup-registry.ts` never registers it, so it was
unreachable over MCP and HTTP, and that catalog roll-call was its only
reference; the import and the catalog count (77 → 76) went with it.

## Lockfile

Adding knip moves two transitive packages: `@napi-rs/wasm-runtime` 1.1.5
→ 1.1.6 and `@tybys/wasm-util` 0.10.2 → 0.10.3. Knip pulls in
`@oxc-resolver/binding-wasm32-wasi`, which floors
`@napi-rs/wasm-runtime` at `^1.1.6`, lifting the hoisted entry that had
resolved to 1.1.5 for `@rolldown/binding-wasm32-wasi`'s `^1.1.4`, which
then cascades to `@tybys/wasm-util`. No runtime impact — both are
support packages for the wasm32-wasi fallback bindings, which don't get
installed where a native binding exists; `npm ls` reports both
extraneous on darwin-arm64. Those two are the only resolved versions
that change in the diff; the rest is knip's own tree (49 packages added)
and `@moq/lite` removed.

## Verification

- `npm run knip` exits 0.
- **Coverage of the enforced set.** With no `rules` block, knip's
included set is 15 issue types: ten enforced at zero tolerance by the
first pass, plus the five the second pass names under the ceiling.
Eleven mutations cover all ten, each flipping the gate to exit 1 and
back to 0 on revert:

| class | mutation |
| --- | --- |
| `files` | a throwaway `.mjs` in `scripts/`; a throwaway `.ts` in
`packages/argent/src` |
| `dependencies` | a bogus declared dep in
`packages/tool-server/package.json` |
| `devDependencies` | a bogus declared devDep in the same manifest |
| `optionalPeerDependencies` | `bytes` moved to an optional
`peerDependency` there |
| `unlisted` | an undeclared import in `packages/argent/src/cli.ts` |
| `unresolved` | an unresolvable relative specifier in the same file |
| `binaries` | a root `package.json` script invoking a nonexistent
binary |
| `duplicates` | `export default sleep` alongside the existing named
export in `utils/timing.ts` |
| `enumMembers` | an unused member on an enum a second module imports
and uses |
| `catalog` | a `catalog` block in the root `package.json` naming a
package nothing uses |

One nuance worth recording: `duplicates` is narrower than it looks. It
reports a symbol exported both by name and as `default`
(`sleep|default`); a second *named* alias of the same symbol is not a
duplicate, and a freshly added symbol nothing references is not reported
at all — so several naive probes read as a miss.

`enumMembers` needed a shape the repo does not currently contain, not an
unreachable one. The only exported enum here, `ServiceState`, is
re-exported through `@argent/registry`'s entry, where knip counts
exports as used, so an unused member added to it is **not** reported —
but an enum in a non-entry module that another module imports and uses
does report one (`Blue ZzColor …/zz-enum.ts:3:3`, exit 1; deleting the
unused member returns exit 0). That is the ninth of the ten covered.

`catalog` is reachable here despite the repo using npm workspaces:
knip's catalog loader falls back to `manifest.catalog` when there is no
pnpm or yarn workspace file, so a `catalog` block in the root
`package.json` reports `Unused catalog entries (1)` and exits 1.
- The two new mechanisms discriminate as well: one extra unused export
takes the backlog to 207 and fails the ceiling, and a stale
`ignoreDependencies` entry produces `Configuration hints (1) … Remove
from ignoreDependencies` and exit 1.
- `npm ci --ignore-scripts` in a clean worktree, followed by `npx knip`,
is green — the job doesn't need any of the postinstalls it now skips.
- `tsc --build`, `typecheck:tests` (all workspaces),
`typecheck:scripts`, `test:scripts`, `eslint . --max-warnings 0`,
`prettier --check .` and the full workspace vitest suite (4711 passed, 3
skipped across 13 workspaces; 3070 of those are `@argent/tool-server`
alone) are green; `package-lock.json` is untouched by the review pass.

2026-08-10 17:09:16 +02:00

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

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 --max-issues ceiling 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. If your change legitimately adds an export nothing imports yet, raise the --max-issues number in the same commit rather than deleting someone else's backlog — the bump is the part reviewers should see.
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.

8.0 KiB Raw Permalink Blame History