mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
fix/flow-dir-export-throw
17 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Jakub Piasecki
|
d4a712c401 |
refactor(gesture): rename the momentum-free flag to momentum (#732)
`settle: true` collided with the runner's other `settle` — waiting for the tree to quiesce — which is the very thing a swipe step does after every dispatch, whichever way the flag is set. One sentence of the flow skill used both senses at once. This names the physics instead: `momentum: false` is the momentum-free gesture, and the natural fling is the default the prose already described. ### Renamed | surface | before | after | | --- | --- | --- | | `swipe` flow directive | `settle: true` | `momentum: false` | | `gesture-swipe` param | `settle` | `momentum` | | `gesture-drag` param | `settle` | `momentum` | The directive passes the flag straight through to the tools, so all three move together. `momentum: true` is the default and normalizes to absent, mirroring what `settle: false` did — parse/serialize stay exact inverses. ### Also - Recording summary label `(settle)` → `(momentum-free)` - Internal constants → `MOMENTUM_FREE_EASE_EXPONENT`, `MOMENTUM_FREE_MIN_STEPS` - `flow-run` tool description and `argent-create-flow` skill updated ### Testing `tsc -b`, eslint (`--max-warnings 0`) and prettier clean; `test/flows` + `test/tools` pass 671/671, including a new case covering the legacy-spelling rejection. |
||
|
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. |
||
|
Hubert Gancarczyk
|
50c9580e30 |
fix(tools): make a schema failure name the parameter that was sent (#718)
> Stacked on #717. ## The problem An invalid tool call returned Zod's raw issue array. Someone who wrote `flow_name` instead of `name` got back, depending on the tool: ``` Invalid params for tool "flow-start-recording": [{"expected":"string","code":"invalid_type","path":["name"]}] Invalid params for tool "flow-execute": [{"code":"custom","path":["flow_path"],"message":"Pass exactly one flow source: name or flow_path."}] ``` The first names the parameter the tool **wanted** and never the one the caller **sent**. The second — flow-execute's, whose `name` is optional so `flow_path` can stand in — names neither: it is anchored on the one source field the caller had no reason to send. Either way the mistake is invisible, and working it out costs a whole turn. ## What changes **1. `describeParamIssues` — one sentence per bad parameter.** Missing fields read as `` `name` is required (string) and was not provided`` rather than as a type error about `undefined`. Unknown keys are named and path-qualified, so a key nested in `selector` is not reported as a bare name that contradicts the top-level list printed beside it. The message closes with the caller's own keys — `You sent: \`flow_name\`, \`project_root\`.` — which is what makes the mistake self-evident. Key names only, never values: this string reaches logs, telemetry and the agent transcript, and a params object can carry a secret. Wired into both entry points: the registry's `invokeTool` and the HTTP boundary. **2. `flow-execute` accepts `flow_name` as an alias for `name`.** The tool is called `flow-execute`, so "the flow's name" spells itself that way; this is a mistake worth absorbing rather than rejecting. The name is resolved in code rather than by the schema — a Zod `required` failure would once again name only the field it wanted — and the error text names the spellings that are silently discarded (`flowName`, `flow`, `flow_file`), because by then Zod has already stripped them and the caller has no other way to learn their key was dropped. **3. A flow DIRECTIVE passed as `command` points at the tool that records it.** `command: "echo"` used to come back as a bare "Tool not found". It now names `flow-add-echo` — and says to call it **directly**, since routing it through the recorder would write the echo *and* a `tool: flow-add-echo` step that fails on every replay. The hint distinguishes directives the recorder **rewrites** from those it stores **raw** for the polish pass, because promising a rewrite that does not happen sends the author looking for a directive that is not in the file. `wait` and `long-press` get their own answers: neither has a recording tool at all. Only a genuine not-found is rewritten — a tool that ran and failed still reports its own error. **4. A recorder tool as `command` is refused.** `flow-add-echo`, `flow-add-step`, `flow-start-recording`, `flow-finish-recording` each mutate the recording itself, and nesting one reported success either way. Each is refused for its own reason: `flow-add-echo` would write the echo **and** a replay-breaking `tool:` step, `flow-start-recording` would **erase** this flow at replay, `flow-finish-recording` would end the take it is a step in, and `flow-add-step` cannot record itself. Now nothing is executed and nothing is written. --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com> |
||
|
filip131311
|
4975ff490c |
fix(debugger): cut debugger tool fail rate — structured not-connected results, classified CDP failures, narrow recovery (#610)
## Why
PostHog (30d) shows the debugger tool family is argent's least reliable
surface:
| tool | fail rate | dominant causes |
|---|---|---|
| `debugger-status` | **37%** | METRO_NO_TARGETS /
SERVICE_INITIALIZATION_FAILED / zod |
| `debugger-connect` | **24%** | SERVICE_INITIALIZATION_FAILED (~7k, >1k
users), METRO_NO_TARGETS |
| `debugger-log-registry` | 41.7% raw / 4.2% excl. storms | one user
retry-looped it **12,742×** on NO_TARGETS |
Three design issues drive this: (1) "app/Metro not ready yet" — an
expected precondition — is reported as a tool *error*, which both
inflates fail rates and invites agent retry storms; (2) every CDP
init/lifecycle fault lands in telemetry as the catch-all
`REGISTRY_SERVICE_INITIALIZATION_FAILED` with `error_kind=unknown`; (3)
~600 users/30d hit zod validation failures with zero visibility into
which parameter was wrong.
## What
**1. Classified failures** (`d71efa43`) — six former plain-`Error` sites
in `cdp-client.ts` / `target-selection.ts` now carry dedicated
`DEBUGGER_CDP_*` / `DEBUGGER_TARGET_DEVICE_MISMATCH` signals (messages
byte-identical). The registry's two causeless "terminating window"
`ServiceInitializationError`s get their own
`REGISTRY_SERVICE_TERMINATING` code.
**2. Narrow recovery** (`ff9c1955`) — `recoverable()` on both debugger
blueprints, scoped to windows where retry is provably safe: Metro
recovers only `DEBUGGER_CDP_NOT_CONNECTED` (request never left the host;
a detected socket death already self-heals for the *next* call via the
terminated cascade — recovering `CONNECTION_CLOSED` there would be dead
code, see the blueprint comment). Chromium also recovers
`CONNECTION_CLOSED` for the tab-switch reconnect window.
`REQUEST_TIMEOUT` stays unrecoverable (double-execution risk). A
reachability test proves the retry actually fires for a RUNNING node.
**3. Structured not-connected results** (`48b91ffd`) — `debugger-status`
and `debugger-log-registry` become factories that return `{ status:
"not_connected", reason, detail, guidance }` for classified
preconditions (`metro_not_running | no_app_connected | device_mismatch |
cdp_unreachable | stale_connection | reconnecting`) and still throw on
unexpected faults. `detail` preserves the original error text for
string-matching agents. `debugger-status` gains a socket-state gate
(Metro: dispose stale node; Chromium: report transient reconnect, no
dispose — the client belongs to the ChromiumCdp dependency).
log-registry keeps **no** gate on purpose: captured logs are readable
over a dead socket, and disposal would destroy the post-crash log file.
Flows/run-sequence map a not_connected result to a **failed step**
(mirrors `isUnmetUiWaitResult`) so recorded connectivity gates don't
silently pass. `debugger-evaluate` re-codes agent-expression throws as
`DEBUGGER_EVALUATE_EXPRESSION_THREW`.
**4. Telemetry + guidance + docs** (`7602a70d`) —
`tool:fail.invalid_params` (schema-declared names only; unknown keys →
literal `"unrecognized_keys"`; capped emit-side at 16), new
`debugger:tool_outcome` event (coded enum; joins
`tool:invoke`/`complete` via `tool_invocation_id` —
`ai_client`/`duration` live on the joined row), anti-retry-loop guidance
appended to the Metro discovery errors (first sentences verbatim —
skills match on them), six skills updated, Telemetry.md bumped to v1.03
with the new coded diagnostics disclosed.
## Dashboard migration note
`debugger-status` / `debugger-log-registry` precondition failures stop
emitting `tool:fail` — track them via `debugger:tool_outcome` (`outcome
!= 'connected'`). Per-AI-client breakdowns of those outcomes need a join
on `tool_invocation_id` against `tool:complete`.
## Testing
- Full suites green: tool-server 3,142, telemetry 290, registry 87. New
coverage: recoverable truth tables (incl. non-Error inputs), status
not-connected matrix
(Metro/Chromium/terminating-window/stale/dispose-side-effect/fall-through-rethrow),
log-registry rows, zod `invalid_params` derivation + emission-level
guard, flow gate, evaluate rewrap, sanitize-level enum checks.
- Live-tested against a booted iOS simulator + RN app and an Electron
target (details in PR comments).
Plan hardened through 3 adversarial review rounds (46 confirmed findings
addressed) before implementation.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
||
|
Stanisław Chmiela
|
a7775e4ba6 |
feat(tool-server): add tool-server event log (#367)
We want to display a nicely parsed and rendered log of events that happened over a span of an Argent session. Current log file is not designed to be machine-readable, so adding a JSONL-formatted log of events (gated by `tool-server-event-log`). I eventually want this 😋 https://x.com/j3nnings/status/2069072177584849192 When enabled, the tool server writes Bunyan-compatible records to `ARGENT_EVENT_LOG` or `~/.argent/tool-server-events.jsonl`. The log captures tool-server lifecycle events and registry lifecycle events, including tool invocations and others. To make messages nicer, added `interaction` object to tool definition. This allows tools to define a nicer message in log (print "Screenshot capture completed." instead of "Tool `screenshot` completed.") Adding these to the existing tools is a follow up PR matter I think. |
||
|
filip131311
|
a2ef316cf1 |
fix(registry): auto-recover a wedged simulator-server on connection-refused (#463)
## Problem A user reported that argent "worked once, then every call timed out" — an hour of `this is taking longer than expected` / `request timed out`. Their logs showed the tell: dozens of ``` Screenshot failed: cannot connect to simulator-server (connection refused at http://127.0.0.1:<port>) … ECONNREFUSED ``` against a single stale port, starting right after `Simulator state changed to 4 (not booted)`. **Root cause — liveness ≠ readiness.** When a simulator is un-booted (or the native server crashes), the `simulator-server` child process can stay **alive** while its API socket stops **listening**. The registry only tears a service down on the child's `exit` event, so the dead-but-cached handle is reused on every subsequent call → `ECONNREFUSED` forever — *even after the simulator is booted again*. The only escape was a manual `stop-simulator-server`, which no ordinary user discovers. ## Fix A generic, opt-in recovery hook on the service registry: **`ServiceBlueprint.recoverable(error)`**. After a tool fails, the registry asks each service that tool resolved whether the error means its instance is dead; any that say yes are disposed (killing the wedged process) and the tool is **retried once** against a freshly re-created instance. Bounded to a single retry so a genuinely broken service can't spin. The registry stays generic — it learns nothing about simulators. The **simulator-server blueprint** implements `recoverable` for **connection-refused only**: - `ECONNREFUSED` proves the request never reached the server, so respawning + retrying can't double-apply a gesture. - **Timeouts and resets are deliberately excluded** — the request there may have taken effect, and a hung-but-listening server is a different failure that respawning wouldn't fix. ## Verification Reproduced live on a real simulator (un-boot → re-boot mid-session), same harness for both arms: | | Result | |---|---| | **Fix enabled** | second screenshot succeeds automatically in ~2.8s (fresh sim-server respawned) — `RECOVERED` | | **Fix disabled** (1-line toggle) | `connection refused at :<port>` persists through the reboot — `WEDGED`, the reported symptom | Automated: - **Registry** (`registry-service-recovery.test.ts`) — retry-once success, no-retry on non-recoverable errors, single-retry cap (no infinite respawn), and targeted disposal (only the service that reports the error is torn down). - **Blueprint** (`simulator-server-blueprint.test.ts`) — recovers on ECONNREFUSED; does **not** on reset / timeout / unrelated errors. - Full suites green: registry 87, tool-server 1846. Lint + prettier clean. ## Files - `packages/registry/src/types.ts` — add optional `recoverable?(error)` to `ServiceBlueprint`. - `packages/registry/src/registry.ts` — `invokeTool` disposes recoverable services and retries once. - `packages/tool-server/src/blueprints/simulator-server.ts` — implement `recoverable` (connection-refused only). - Tests for both layers. |
||
|
Ignacy Łątka
|
fa5af59931 |
refactor: replace hand-rolled parsers with standard packages (#373)
## What & why
Swept the repo for hand-rolled regex/string parsers that reimplement a
solved problem, and replaced the genuinely fragile ones with
battle-tested packages (or deps already in the tree). Each swap is
paired with a regression test for an input the old code got wrong.
| Site | Was | Now |
|------|-----|-----|
| `adb.ts` `resolveAvdPath`, `linux-preflight.ts` `readMb` | per-key
anchored regex over AVD `.ini` / `config.ini` | **`ini`** |
| `workspace-reader.ts` `extractEnvKeys` | split-on-first-`=` line loop
| **`dotenv`** (`parse`) |
| `uninstall.ts` `readBundledSkillName` | `^name:(.+)$` capture +
outer-quote strip | **`yaml`** (already a dep) |
| `mcp-configs.ts` `escapeRegExp` | hand-rolled char-class escape |
**`escape-string-regexp`** |
| `export.ts` xctrace version, `session-context.ts` RN minor |
`match(/(\d+)\./)` / `split(".")[1]` | **`semver`** `coerce` (already a
dep) |
| `source-resolver.ts` `parseDebugStack` | `at fn (file:l:c)` regex
(required both line+col) | **`stacktrace-parser`** |
### Bugs these fix (covered by new tests)
- **ini**: a quoted `path = "/Users/My Name/.../X.avd"` kept its quotes,
failed the `startsWith("/")` guard → AVD unresolved. Inline comments /
trailing junk after a sizing value made the anchored regex return `null`
→ sizing warning silently lost.
- **dotenv**: `export FOO=bar` produced the key `"export FOO"` (dropped
by the identifier filter); a multi-line quoted value leaked its
continuation lines (`FAKE=...`) as phantom keys.
- **yaml**: a `name:` value with a trailing `# comment` was taken
verbatim (comment included); odd quoting only had one outer pair
stripped.
- **stacktrace-parser**: a `:line`-only frame (no column, as Hermes/JSC
can emit) became an unusable `unknown` frame.
## Deliberately left alone (not "flaky", or no safe swap)
To avoid churn and regressions, I did **not** touch:
- **`uiautomator-parser.ts`** hand-rolled XML parser — a deliberate,
documented choice with a dedicated hardening suite, including a
*leniency* test for malformed dumps (stray `</node>`). A spec XML parser
would reject inputs this is built to recover from.
- **`ios-profiler/pipeline/xml-parser.ts`** (xctrace XML) — the most
tangled parser, but it has no real `.trace` fixtures and only thin
entity-decode tests. A rewrite can't be proven equivalent without
device-generated traces, so swapping it blind risks silently breaking
iOS CPU/hang/leak reports. Best done separately with a captured fixture.
- **`http.ts` `extractHostname`** — DNS-rebinding guard; `URL.hostname`
keeps the IPv6 brackets the loopback check compares without, so the
"fix" isn't simpler and the path is security-sensitive.
- **`adb.ts` `shellQuote`** — the POSIX single-quote escape is the
standard correct form.
- **`parseAdbDevices`, `android-screen.ts` `wm size`, `detect-app.ts`
dumpsys** — tool-specific CLI-output scraping with no clean package
equivalent (and documented edge-case handling).
- **`workspace-reader.ts` `extractMetroPort`** — a best-effort heuristic
over JS config; a correct replacement needs an AST parse, out of scope
here.
- **`config-parse.ts` Yarn/npm age-gate parsing** — a precise,
unit-tested domain parser; `ms` would introduce a bare-number ambiguity.
## Verification
- `tsc --build` clean across the workspace graph.
- Tests: tool-server **1205**, installer **264**, update-core **31** —
all pass (incl. the new regression tests).
- `prettier --check` clean on all changed files.
- Production esbuild bundle (`bundle-tools.cjs`) emits a valid
`tool-server.cjs`/`mcp-server.mjs` with
`ini`/`dotenv`/`stacktrace-parser` inlined (`node --check` OK). *(The
script's final step errored only on the auth-gated
`trace-processor.wasm` download, unrelated to bundling.)*
New runtime deps: `ini`, `dotenv`, `stacktrace-parser` (tool-server),
`escape-string-regexp` (installer) — all small, pure-JS. Lockfile change
is purely additive.
|
||
|
Hubert Gancarczyk
|
8483fe23f9 |
feat: add telemetry (#274)
## Summary Adds anonymous, opt-out telemetry for Argent installer, update/uninstall flows, CLI telemetry controls, tool-server lifecycle, and tool invocation health signals. ## What Changed - Added a new private `@argent/telemetry` package - Introduced typed telemetry events with runtime property allowlisting/sanitization. - Added lazy anonymous identity creation in `~/.argent/telemetry-id`. - Added consent handling via: `argent telemetry enable`, `argent telemetry disable`, `argent telemetry status` - Added local debug logging with `ARGENT_TELEMETRY_DEBUG=1`. - Tracks installer funnel events for init, global install/update decisions, editor selection, allowlist decisions, skill installation, rules/agents copy, update, and uninstall. - Tracks tool-server start/stop and tool invocation/complete/fail events. - Adds per-tool invocation IDs so request metadata can be correlated without relying on ordering. - Records only coarse platform metadata (`ios` / `android`) for device tools - Ensures short-lived commands drain telemetry with bounded shutdown before exit. - Wires telemetry package into bundling and injects full Argent CLI version metadata at build time. - Added central `FAILURE_CODES` and `FailureSignal` helpers in `@argent/registry`. - Attached structured, sanitized failure metadata across registry errors, tool-server tools/services, HTTP validation/dependency gates, installer flows, and `argent run`. - Added telemetry allowlisting for safe failure fields only: static error codes/stages, coarse area/kind/command, bounded exit codes, allowlisted signals/spawn codes, and coarse network failure classes. - Split CLI-local `argent run` failures into `cli:run_fail` so they are not double-counted as tool failures. - Added installer telemetry for failed package actions, update/uninstall failures, rules/agents copy results, and skill refresh results. - Made telemetry consent writes atomic and kept telemetry status side-effect free via `peekAnonId()`. - Adds a coarse, anonymous signal to tool telemetry: which AI coding tool is driving the MCP server (Claude Code, Codex, Cursor, Gemini, VS Code, Windsurf, Zed, opencode, Copilot, or other). ## Tests Added / Updated - Telemetry unit coverage for consent precedence, identity creation, base props, sanitization, PostHog config, debug logging, shutdown, opt-out, and forget behavior. - Registry/tool-server tests for invocation IDs, platform metadata boundaries, startup failure flushing, and tool lifecycle telemetry. - Installer tests for final telemetry drain, update action classification, and uninstall identity cleanup ordering. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Ignacy Łątka
|
f3d108a5a1 |
feat: Argent Lens (#271)
Adds a UI for proposing and choosing between multiple agent-generated variants on a running app. **MCP tools** (`packages/tool-server/src/tools/variants/`) - `propose_variant` — non-blocking; agent stages a variant for an on-screen element (text/label/identifier/role matcher). - `await_user_selection` — one blocking call; parks until the human submits, then returns the picks + free-form annotations + a global comment. **Preview UI** (`packages/ui`) - Floating cards anchored to live element frames via the existing `describe` tree, with connector lines. - Comment-mode inspector: hover spotlight (blur+dim everything else), right-click any element to pin a comment. - Sticky padded hitbox + trajectory-aware dismissal so focus doesn't drop on cursor jitter. - Footbar [Complete] [Clear], confirm modal for destructive actions, green toast on submit. - Single source of feel constants in `theme.css`. **Electron host** (`packages/preview-window`) - New workspace package. Frameless transparent BrowserWindow at fixed size; the squeeze open/close is a CSS `transform: scaleY` on `<html>`, GPU-composited — no OS-level resize. - Tool-server spawns it on `await_user_selection` park, reuses it across re-awaits, and asks it to animate-close on submit (1s grace so the toast is visible). - Stdio-line IPC (no extra ports). Ephemeral throughout — the renderer hits the tool-server's already-ephemeral `/preview/*` HTTP. --------- Co-authored-by: Filip131311 <contact@filipkaminski.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Ignacy Łątka
|
acfe80ab6b |
chore: add ESLint with typescript-eslint (#349)
## What Adds type-aware linting to the workspace. Until now the only static gate on JS/TS was Prettier (formatting) and `tsc` — there was no linter, so the whole class of bugs `tsc` doesn't catch (floating promises, misused promises, throwing non-Errors, unsafe stringification, dead code) went unguarded across ~450 TS files. - **`eslint.config.mjs`** — flat config. `recommendedTypeChecked` runs on the TS sources via an explicit `project` list (each package keeps tests in a separate `tsconfig.test.json` that `projectService` can't auto-discover, so it's listed explicitly). Type-checked rules are disabled for the loosely-typed plain-JS build/dev scripts. The `argent-private` submodule and build artifacts are ignored (mirrors `.prettierignore`). - **`lint` / `lint:fix`** npm scripts. - **`.github/workflows/lint.yml`** — builds the workspace (type-aware rules need the referenced projects' declarations) then runs `eslint .`. Node 24 (ESLint 10 requires `^20.19 || ^22.13 || >=24`). ## Rule calibration This is an adoption PR, so it's deliberately calibrated rather than maximal: - **Kept on (bug-catchers):** `no-floating-promises`, `no-misused-promises` (with `checksVoidReturn: false` to drop the noisy callback variant), `only-throw-error`, `no-base-to-string`, `prefer-promise-reject-errors`, `no-unused-vars` (`_`-prefix escape hatch), plus the ESLint recommended set. - **Parked off as documented debt:** the high-volume style/`any`-family rules — `no-unsafe-*`, `require-await`, `no-unnecessary-type-assertion`, `restrict-template-expressions`, `unbound-method`, `no-explicit-any`, etc. These flag broad classes of pre-existing code and are out of scope here; each is commented as a ratchet target for follow-up passes. All findings from the kept-on rules are fixed in this PR (dead code/imports, error-cause chaining, **3 genuine floating promises**, non-Error throws, unsafe stringification). Stale unused `eslint-disable` directives left over in the tree were removed by `--fix`. ## Verification Locally, all green: `eslint .` (0 errors), `tsc --build`, `typecheck:tests`/`typecheck:scripts`, the full `vitest` suite (no behavior change from the fixes), and `prettier --check`. ## Not included (intentionally) Other static-analysis gates discussed (CodeQL, dependency-audit, ShellCheck, actionlint) are out of scope for this PR. |
||
|
Ignacy Łątka
|
74c2065ca0 |
fix: multiple minor bugs (#194)
## Human Summary
This PR aims to fix bugs that are benign for now, could become serious
in the future:
- multiple CSRF via CORS misconfiguration on `tool-server`
- multiple SSRF that were possible due to the CSRF
- path traversal via flow creator
- multiple shell injections
- lack of auth on `MCP/CLI <-> tool-server` path (bug details discussed
in person). After the other fixes this is somewhat redundant, but
doesn't hurt and protects against theoretical access attempts from
within other processes running on the user's device.
## AI Summary
<details>
Bundles six independent fixes that together close a CSRF cluster on the
local tool-server. The vulnerabilities chain because the HTTP API has no
auth and serves wildcard CORS, so any web origin (Firefox
unconditionally; Chrome via PNA-disabled or loopback-served) can drive
the tool-server from a cross-origin tab. Each individual fix lives on
its own branch (linked below) and stands alone — this PR exists so the
cluster can be reviewed and shipped as one piece.
### Fixes
1. **`fix(http): require auth token + drop wildcard CORS`** —
per-process 32-byte hex Bearer token, generated by the launcher,
persisted in `~/.argent/tool-server.json` with mode `0600`, required on
every route except `/preview`. Constant-time compare. Wildcard CORS
dropped. Threaded through MCP server + CLI + tools-client.
Backwards-compat: when `ARGENT_AUTH_TOKEN` is unset (e.g. `npm run
dev`), the server logs a one-shot warning and skips the gate.
2. **`fix(http): reject non-loopback Host headers`** — middleware
(before the auth gate) returns 403 for any request whose Host is not
`127.0.0.1`, `localhost`, or `::1`. Closes DNS-rebinding bypass.
3. **`fix(ios-profiler-start): prevent shell injection via device_id`**
— `detectRunningApp(udid)` no longer interpolates `params.device_id`
into `execSync` template strings. Both `xcrun simctl spawn ${udid} …`
and the piped `xcrun simctl listapps ${udid} | plutil …` are rewritten
to `execFileSync` array form, with the pipe to `plutil` happening in JS
(input option) rather than the shell.
4. **`fix(flows): validate name to close path-traversal in
getFlowPath`** — `getFlowPath()` requires `^[A-Za-z0-9_\\-]+$` and
verifies `path.relative(getFlowsDir(), filePath)` does not start with
`..`. Centralizing the check in `getFlowPath` covers every caller
(start-recording, add-step, insert-echo, finish-recording, run,
read-prerequisite) without per-tool churn.
5. **`fix(source-resolver): reject paths outside projectRoot +
non-source extensions`** — `readSourceFragment()` previously accepted
any `file` field on the SourceLocation it was given (the field traces
back to a React fiber's `_debugSource.fileName`, which is
attacker-controllable). Now: containment check via
`path.relative(projectRoot, abs)` plus an extension allowlist
(`.js/.jsx/.ts/.tsx/.mjs/.cjs/.json`).
6. **`fix(source-maps): reject non-loopback URLs (close SSRF)`** —
`doRegister()` validates `sourceMapURL` before fetching: only
`http://`/`https://` URLs whose hostname is loopback. Closes a
blind-SSRF probe of the host network (cloud metadata at
`169.254.169.254`, intranet pages, etc.).
### Individual branches
- `fix/auth-token-drop-cors`
- `fix/host-header-validation`
- `fix/profiler-command-injection`
- `fix/flow-name-validation`
- `fix/source-resolver-traversal`
- `fix/source-map-ssrf`
If you want to land them as separate PRs, the integration branch can be
dropped after each lands.
## Test plan
Unit tests (already in CI):
- [x] `npm run build` passes (workspace tsc clean)
- [x] `npm run test -w @argent/tool-server` — **510 passed (was 473 on
`main`)**, +37 new tests across:
- `test/http-auth.test.ts` — 10 tests (auth, CORS removal, dev-mode
escape hatch)
- `test/http-host-validation.test.ts` — 7 tests (loopback allow,
DNS-rebinding reject)
- `test/flows/flow-utils.test.ts` — +5 tests (path-traversal name
rejection)
- `test/metro/source-resolver.test.ts` — +5 tests (containment +
extension allowlist)
- `test/metro/source-maps-ssrf.test.ts` — 10 tests (loopback-only allow)
Live end-to-end (run locally against this branch's bundled artifacts):
- [x] Launcher spawns tool-server; `~/.argent/tool-server.json` contains
a 64-char hex token and is mode `0600`
- [x] `GET /tools` without `Authorization` → **401**
- [x] `GET /tools` with wrong token → **401**
- [x] `GET /tools` with correct token → **200** (61 tools)
- [x] Request with `Host: attacker.example` → **403**
- [x] No `Access-Control-Allow-Origin` header on responses
- [x] Cross-origin command-injection probe via `device_id=\"x;touch
/tmp/marker;#\"` does NOT create the marker file (Vuln 4 closed)
- [x] `flow-start-recording` with `name=\"../../../tmp/escape\"`
rejected with `Invalid flow name`
- [x] `flow-start-recording` with a valid name still works
- [x] `tools-client.fetchTools()` and
`tools-client.callTool('list-simulators')` succeed via the auth-token
threading (the path the MCP server uses internally)
- [x] MCP-stdio entry point (`startMcpServer`) end-to-end: `initialize`
→ `tools/list` (61 tools) → `tools/call('list-simulators')` returns
content. Confirms the launcher → token → fetch → tool-server flow is
intact for the production `argent mcp` path.
## Notes for reviewers
- `/preview` is intentionally exempt from the auth gate (it was not
gated before; tightening it deserves a separate PR).
- The `ARGENT_AUTH_TOKEN`-unset escape hatch keeps `npm run dev`
workable; production `argent mcp` always passes a token via env, so the
warning never fires there.
- The auth bundle changes the public return shape of `ensureToolsServer`
from `Promise<string>` to `Promise<{ url, token }>`. The two in-tree
callers (`argent-mcp/mcp-server.ts`,
`argent-tools-client/tools-client.ts`) were updated; external consumers
using `ARGENT_TOOLS_URL` should also set `ARGENT_AUTH_TOKEN` to match
the server.
</details>
|
||
|
Ignacy Łątka
|
2180dd8e6a |
ci: typecheck test files in CI (#202)
We've had lots of type errors in our tests, scripts and other non-core
code despite all CI being green. This PR fixes that.
This PR also removes some redundant features like `@ts-check` inline
directives which have been replaced by proper ts config files.
<details>
## AI Summary
Audit of every CI workflow against every package's `package.json` and
`tsconfig` surfaced several gaps where CI did partial coverage. This PR
closes them and fixes every pre-existing type error the new gates
surfaced.
## Gaps closed
| # | Gap | Fix |
|---|---|---|
| 1 | `tsc --build` only covers `src/**`; test files (`test/**`,
`tests/**`) never typecheck. Vitest transforms tests with esbuild —
strips types instead of checking them. | New `tsconfig.test.json` per
test-bearing package (`composite: false`, `noEmit`, `rootDir: "."`,
includes `src/**` + `test/**` (or `tests/**`) + `vitest.config.ts`). New
`typecheck:tests` npm script per package. New CI step `npm run
typecheck:tests --workspaces --if-present`. |
| 2 | Build/publish scripts (`scripts/*.{cjs,mjs}`,
`packages/argent/scripts/*.cjs`, `packages/skills/scripts/install.js`) —
JS, never typechecked. `sync-readme.cjs` runs in `prepack`;
`postinstall.cjs` runs on every install; `bundle-tools.cjs` is
build-critical. A typo only surfaces at runtime. | Root
`tsconfig.scripts.json` with `allowJs` + `checkJs` + `noImplicitReturns`
covers all script paths. Root `typecheck:scripts` npm script. New CI
step. Per-file `// @ts-check` comments removed — tsconfig drives
inclusion. |
| 3 | `argent-cli` had no `typecheck:tests` script. When tests get added
there they'd silently bypass the gate. | Added `tsconfig.test.json` +
`typecheck:tests` script. `--if-present` would have skipped it anyway,
so this is future-proofing. |
| 4 | Local `prettier --check .` walked into the `argent-private`
submodule and reported violations there. CI didn't see this because
`actions/checkout@v4` doesn't init submodules — so local `prettier
--check` did not match CI behavior. | New `.prettierignore` excludes the
submodule plus `dist/`, `node_modules/`, lock files, `tsbuildinfo`.
No-op in CI; aligns local with remote. |
## Test type errors fixed
These were already present. The `npm test` passed because esbuild
stripped the offending types:
- **argent-installer**: widen `scope` literal type for
dead-code-elimination guard; non-null `addAllowlist!`/`removeAllowlist!`
(optional methods on adapter).
- **argent-mcp**: add `.js` extensions for NodeNext module resolution.
- **registry**: broaden `StaticBlueprintResult` blueprint api from `{
id; deps? }` to `Record<string, unknown>` so existing tests can pass
arbitrary api shapes.
- **tool-server**:
- non-null `zodSchema!` on tool defs that always carry one
(`launchAppTool`, `restartAppTool`, etc.)
- cast sentinel `"ignored"` strings to `DeviceInfo` in factory-rejection
tests
- match the 4-arg `dispatchByPlatform<IosServices, AndroidServices,
Params, Result>` signature (tests still passed the old 3-arg shape)
- `assertFlowRunResult` type guard for the `FlowRunResult |
FlowPrerequisiteNotice` union before `.steps` access
- swap broken `typeof import("supertest").default` for static `import
supertest` (supertest is `export = supertest`)
- fill in the 4 fields missing from a `NativeProfilerSessionApi` mock
(`xctraceProcess`, `recordingTimedOut`, `recordingExitedUnexpectedly`,
`lastExitInfo`)
- cast a vitest `Mock` to a plain function at one direct call site where
the `Procedure | Constructable` union loses callability
- fix `update-checker.test.ts` reading `../../package.json` (off-by-one
— went past tool-server; was relying on a Vite resolver quirk)
- replace one cross-package `../../../registry/src/index` import with
`@argent/registry` so registry's private `services` field isn't compared
between src and dist declarations
All fixes are type-level. Full `npm test --workspaces --if-present`
still reports **991 passed across 87 test files**.
</details>
|
||
|
filip131311
|
85e7aa9612 |
fix(registry): emit JSON Schema type for ZodDefault fields (#199)
## Summary
`zodTypeToJsonSchema` in `packages/registry/src/zod-to-json-schema.ts`
did not handle `z.ZodDefault`. Any tool input field declared as
`z.X().default(...)` fell through to `{}`, dropping the type.
Downstream, the MCP harness sent values for those fields as strings, and
the tool-server's strict Zod validators rejected them with `Expected
number, received string` / `Expected boolean, received string`.
Also fixes a related issue in the same file: `isOptional` returned
`false` for `ZodDefault`, so defaulted fields were pushed into
`required` even though the caller doesn't have to provide them.
Tools that misbehave in practice today because of this and start working
after this fix:
- `stop-metro` — `port`
- `react-profiler-start` — `port`, `force`, `sample_interval_us`
- `debugger-component-tree` — `onScreenOnly`, `includeSkipped`
(Reproduced against the published `argent 0.6.1` MCP server while
profiling an Expo app — the `argent run <tool> --args '<json>'` CLI path
is unaffected because it doesn't go through the published JSON Schema.)
## Changes
- `packages/registry/src/zod-to-json-schema.ts`: handle `ZodDefault`
(recurse into `_def.innerType`, attach `default: defaultValue()`); treat
`ZodDefault` as optional.
- `packages/registry/tests/zod-to-json-schema.test.ts`: new file. Covers
`z.number()`, `z.boolean()`, `z.string().optional()`,
`z.number().default(...)`, `z.boolean().default(...)`,
`z.coerce.number().int().positive().default(...)`, and a mixed-object
happy path.
## Test plan
- [x] `npx vitest run` in `packages/registry` — 64 passed, 0 failed (4
files; the new test file contributes 7 cases).
- [ ] Reviewer sanity check: load the rebuilt MCP server and confirm a
tool with a `.default(...)` numeric/boolean field now publishes the
correct \`type\` in its input schema.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Paweł Fornagiel
|
42cc5e3c09 |
Enhance error handling in registry and tool server blueprints (#115)
* feat: enhance error handling in registry and tool server blueprints * test: add tests for error handling in registry and tool server blueprints * refactor: streamline exception detail formatting in CDP client * format: run format |
||
|
Filip131311
|
8c2a313c76 |
chore: format codebase with Prettier
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Paweł Fornagiel
|
badf203d57 |
feat: add timeout system and process killing capabilities (#14)
* feat: add termination * refactor: refactor how idletimer is used within index and http server * feat: enhance confirmation message for stopping Metro bundler process |
||
|
pFornagiel
|
f4f882bad1 | feat: registry integration |