skills/software-mansion__argent
1
0
Fork 0
mirror of https://github.com/software-mansion/argent.git synced 2026-09-14 19:27:14 +08:00
Code Issues Packages Projects Releases Wiki Activity
Files
jpiasecki/flow-repeat
software-mansion__argent/CONTRIBUTING.md
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

189 lines
9.1 KiB
Markdown
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.
## Table of contents
- [Requirements](#requirements)
- [Setting up the dev environment](#setting-up-the-dev-environment)
- [Project structure](#project-structure)
- [Building](#building)
- [Running the project](#running-the-project)
- [Running tests](#running-tests)
- [Code style](#code-style)
- [Submitting a pull request](#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
1. **Fork and clone** the repository:
```bash
git clone https://github.com/software-mansion/argent.git
cd argent
```
2. **Install dependencies** (npm workspaces installs all packages at once):
```bash
npm install
```
3. **Start the dev environment:**
```bash
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:
```bash
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)
```bash
npm run dev
```
Builds everything and starts the tool-server from source. See [Setting up the dev environment](#setting-up-the-dev-environment) for details.
### Full build
Build all packages at once using TypeScript project references:
```bash
npm run build
```
To build a specific package:
```bash
npm run build -w @argent/registry
npm run build -w @argent/tool-server
```
To build and bundle the distributable MCP package:
```bash
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](#setting-up-the-dev-environment)).
**Production-like start** (builds bundles first, then starts from compiled output):
```bash
npm run start
```
This builds the registry, then starts the tools server on port 3001.
Verify the tools server is up:
```bash
curl http://localhost:3001/tools
```
---
## Running tests
Tests are written with [Vitest](https://vitest.dev/). Each package has its own test suite.
Run tests for a specific package:
```bash
npm test -w @argent/registry
npm test -w @argent/tool-server
```
Run tests in watch mode during development:
```bash
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
1. **Create a branch** from `main` with a descriptive name (e.g. `feat/add-screenshot-tool`, `fix/session-leak`).
2. **Make your changes.** Keep the scope of a PR small and focused — it's easier to review.
3. **Ensure the build passes:**
```bash
npm run build
```
4. **Ensure tests pass** for the packages you touched.
5. **Check for dead code**, in a tree with no build output:
```bash
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.
6. **Write a clear PR title** — it becomes part of the release changelog. Use the same prefix convention as commit messages (`feat:`, `fix:`, etc.).
7. **Open the PR** against `main` and fill in the description with context on what changed and why.
8. 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.
Reference in New Issue View Git Blame Copy Permalink