feat: CLI server commands with named instances and --server flag

[vinifig]

Summary

Adds a named server registry + TCP IPC layer so every MCP action can also be invoked as a plain CLI command targeting a named, running server instance.

✨ New capabilities

• flutter_skill connect --id=myapp --port=50000 — attach to a running Flutter app and give it a persistent name
• flutter_skill server list — table of all running named servers
• flutter_skill server stop --id=myapp — stop a named server
• flutter_skill server status --id=myapp — show port, PID, project path, VM URI
• flutter_skill servers — shorthand for server list
• flutter_skill ping --server=myapp — health check a named server
• flutter_skill tap "Login" --server=myapp — run any action via a named server
• flutter_skill screenshot --server=app-a,app-b — parallel execution across multiple servers

🏗️ Architecture

File	Role
lib/src/server_registry.dart	Reads/writes ~/.flutter_skill/servers/<id>.json; PID-alive filtering, collision guard, path-traversal validation
lib/src/skill_server.dart	JSON-RPC 2.0 server over TCP (+ Unix socket fast-path on macOS/Linux)
lib/src/skill_client.dart	Resolves named server → port, sends JSON-RPC requests
lib/src/cli/connect.dart	flutter_skill connect command
lib/src/cli/server_cmd.dart	flutter_skill server list/stop/status
lib/src/cli/ping_cmd.dart	flutter_skill ping command
lib/src/cli/output_format.dart	isCiEnvironment() helper; --output=json|human flag; callServersParallel()

📝 Modified files (additive only — no breaking changes)

• lib/src/cli/launch.dart — new --id=<name> and --detach flags; auto-registers a SkillServer when the VM URI is discovered; monitors flutter run exit to stop the server
• lib/src/cli/inspect.dart — new --server=<id>[,...] flag for parallel forwarding; --output flag; consistent JSON schema with direct-VM path
• lib/src/cli/act.dart — same --server pattern for all act subcommands; --output=json respected on both direct-VM and server-forwarding paths
• bin/flutter_skill.dart — routes connect, ping, servers, server list/stop/status to new handlers while keeping server alone as the MCP server

🤖 CI output

When CI, GITHUB_ACTIONS, CIRCLECI, TRAVIS, or BUILDKITE env vars are set, output defaults to JSON. Always overridable with --output=json or --output=human.

Single-server JSON output matches the same schema as the direct-VM path. Multi-server output wraps results in an array with per-server metadata.

Test plan

• flutter_skill connect --id=myapp --port=50000 registers entry in ~/.flutter_skill/servers/myapp.json
• flutter_skill server list shows the registered server
• flutter_skill tap "Submit" --server=myapp forwards tap to the named server
• flutter_skill screenshot --server=a,b runs in parallel and writes screenshot_a.png / screenshot_b.png
• flutter_skill server stop --id=myapp removes the registry entry and stops the server process
• flutter_skill ping --server=myapp exits 0 when reachable, 1 when not
• All existing commands (launch, inspect, act without --server) behave identically to before
• dart analyze lib/src/... reports no issues on new files
• Unit tests pass: dart test test/server_registry_test.dart test/skill_server_client_test.dart

[vinifig]

Code Review — Initial

Summary

The PR delivers the right architecture — TCP registry, SkillServer, SkillClient, connect command, --server flag, parallel execution. The bones are solid. However, there are two critical bugs that make server stop non-functional (accumulated zombie processes), and a severe mismatch between the stated goal ("160+ tools accessible from CLI") and reality (only 8 methods implemented).

Changed Files

File	Change
bin/flutter_skill.dart	Added connect, servers cases; routed server list/stop/status to server_cmd
lib/src/cli/act.dart	Added --server= forwarding with parallel execution via SkillClient
lib/src/cli/connect.dart	New command: attach to running Flutter app and start a named SkillServer
lib/src/cli/inspect.dart	Added --server= forwarding with parallel execution
lib/src/cli/launch.dart	Added --id= and --detach flags
lib/src/cli/output_format.dart	New: CI detection, OutputFormat enum, resolveOutputFormat
lib/src/cli/server_cmd.dart	New: server list/stop/status subcommands
lib/src/server_registry.dart	New: ServerEntry model + ServerRegistry static class
lib/src/skill_client.dart	New: TCP/Unix-socket JSON-RPC 2.0 client
lib/src/skill_server.dart	New: TCP + Unix-socket JSON-RPC 2.0 server wrapping AppDriver

---

🔴 CRITICAL — 3 issues

[CRITICAL] Correctness — skill_server.dart:_dispatch + server_cmd.dart:834

server_cmd.dart:_cmdStop calls client.call('shutdown', {}). _dispatch has no shutdown case — the default throws, _cmdStop catches silently, removes the registry entry, but the Dart process keeps parking on the 365-day Future.delayed. Every server stop call leaves a zombie Dart process.

[CRITICAL] Correctness — connect.dart:378

await Future<void>.delayed(const Duration(days: 365)) will genuinely time out after 365 days and exit silently. Use a Completer<void> instead: set it in the signal handler and await completer.future.

[CRITICAL] Correctness — server_registry.dart:_isPidAlive (Windows)

result.stdout.toString().contains(pid.toString()) false-positives: PID 123 matches inside 1234. Split on whitespace and compare the PID field exactly.

🟠 MAJOR — 7 issues

[MAJOR] Architecture — skill_server.dart:_dispatch — only 8 of 160+ methods

Tools like hot_restart, scroll_to, wait_for_element, assert_visible are silently unreachable from CLI. Expand _dispatch or document the phase-1 gap explicitly.

[MAJOR] Architecture — _parseServerIds duplicated in act.dart and inspect.dart

Byte-for-byte identical private functions. Move to output_format.dart.

[MAJOR] Architecture — _ActResult / _ServerResult near-duplicated

Same shape in two files. Extract to shared ServerCallResult.

[MAJOR] Correctness — launch.dart:_spawnDetachedServer with Platform.resolvedExecutable

Under dart run, resolvedExecutable is the Dart VM binary. The spawned dart connect ... command will fail silently.

[MAJOR] Correctness — act.dart:_buildRpcCall scroll direction hardcoded

Both scroll and scroll_to map to swipe with direction: 'up' hardcoded. flutter_skill act scroll down --server=myapp silently scrolls up.

[MAJOR] Security — server_registry.dart:register — no ID validation

id is used directly as a filename. An id like ../../../etc/cron.d/evil causes path traversal. Validate [a-zA-Z0-9_\-]+ before writing.

[MAJOR] Correctness — server_cmd.dart:_cmdStop does not terminate the server process

stop() closes sockets but does not call exit(). The Dart isolate keeps running.

🟡 MINOR — 5 issues

[MINOR] Correctness — connect.dart — https:// not handled

HTTP-to-WS normalization only covers http://. Add https:// → wss://.

[MINOR] Correctness — connect.dart — silent auto-discovery

When neither --port nor --uri is provided, falls back to port scanning (10-30s) with no documentation. Add --auto-discover or document the fallback.

[MINOR] Design — skill_server.dart:_vmServiceUri uses as dynamic

Fragile duck typing. Use driver is FlutterSkillClient ? (driver as FlutterSkillClient).vmServiceUri : ''.

[MINOR] Correctness — skill_server.dart:stop() concurrent iteration

_connections.clear() and _connections.remove(socket) from onDone can fire in the same event loop turn.

[MINOR] Naming — output_format.dart:stripOutputFlag

Only strips --output=*, not all meta-flags. Rename to stripOutputFormatFlag.

⚪ NIT — 3 issues

[NIT] Redundancy — server_cmd.dart:_padRight

One-line wrapper around String.padRight. Call .padRight directly.

[NIT] Correctness — skill_client.dart:_connect Unix socket check

File.exists() on a bound but unresponsive socket passes the check then fails to connect. The catch (_) handles it, but add a comment explaining why the double-check is safe.

[NIT] Unused — findFreePort() in skill_server.dart

Defined but never called. Remove.

---

Verdict

CHANGES REQUESTED — Two critical bugs (zombie processes, 365-day park hack) plus the major tool surface gap and Platform.resolvedExecutable detach failure must be resolved before merge.

[vinifig]

Code Review — commit 5f6293f

Summary

Architecture is sound and first-pass fixes were applied correctly. However, two CRITICAL runtime failures remain (go_back unhandled in _dispatch, scroll semantic mismatch between paths), plus several MAJOR correctness and security gaps.

---

🔴 CRITICAL — 2 issues

[CRITICAL] Correctness — act.dart:_buildRpcCall / skill_server.dart:_dispatch

go_back is mapped in _buildRpcCall but _dispatch has no case 'go_back' — falls through to throw Exception('Method not found: go_back'). Any flutter_skill act --server=foo go_back always fails at runtime.

[CRITICAL] Correctness — act.dart (direct path vs _buildRpcCall)

scroll has completely different semantics per path. Direct VM: param1 = widget key. _buildRpcCall: param1 = direction string. flutter_skill act scroll myButton scrolls to a widget; flutter_skill act --server=foo scroll myButton tries to swipe in direction "myButton".

🟠 MAJOR — 6 issues

[MAJOR] Correctness — act.dart:_buildRpcCall / skill_server.dart:_dispatch

assert_visible, assert_gone, get_text, find_element, wait_for_element fall through to default with empty params. Assertions always pass (null key matches nothing checked). Masks real test failures.

[MAJOR] Correctness — skill_server.dart:_dispatch (hot_restart)

hot_restart silently falls back to hot_reload. Caller gets success: true but app state is not reset. Use FlutterSkillClient.hotRestart() with a warning field on fallback.

[MAJOR] Correctness — skill_server.dart:_dispatch (screenshot null)

driver.takeScreenshot() returns String?. When null, returns {'image': null} with success: true. Throw an exception when image == null.

[MAJOR] Architecture — server_registry.dart:listAll

listAll() silently deletes stale files as a side effect of a read operation. Extract cleanup into a separate prune() method.

[MAJOR] Architecture — launch.dart:_attachServer

Fire-and-forget void _attachServer(...) async: when flutter run exits, SkillServer is left running with a dead driver. Monitor process.exitCode and call server.stop() when it completes.

[MAJOR] Security — skill_client.dart:_connect

ID validation only runs in ServerRegistry.register(), not in read-path lookups. --server=../../../tmp/evil constructs an arbitrary socket path. Add validation in SkillClient.byId(), ServerRegistry.get(), and ServerRegistry.unixSocketPath().

🟡 MINOR — 4 issues

[MINOR] State Management — skill_client.dart:_nextId

Instance field always 1 since every call site creates a fresh SkillClient. Make it a local variable.

[MINOR] Correctness — connect.dart (double-stop on concurrent signals)

SIGINT and SIGTERM can fire concurrently, calling server.stop() and driver.disconnect() twice. unregister() throws FileSystemException on the second call. Add a bool _stopping guard.

[MINOR] Naming — output_format.dart

stripOutputFlag deprecated via doc comment but missing @Deprecated('...') annotation. Dart tooling won't warn consumers.

[MINOR] Naming — skill_server.dart:_handleLine

final id = request['id'] shadows this.id (server name). Rename to requestId.

⚪ NIT — 2 issues

[NIT] Code Duplication — act.dart and inspect.dart

Parallel-server fan-out pattern duplicated verbatim. Extract a shared callServersParallel() helper.

[NIT] Testing

~900 lines of new logic with zero test coverage.

---

Verdict

CHANGES REQUESTED — Two CRITICAL bugs (go_back unhandled, scroll semantic mismatch) and the MAJOR client-side path traversal gap must be resolved before merge.

[vinifig]

Code Review — commit 4c647da

Summary

Core architecture clean and well-motivated. Two CRITICAL bugs remain: scroll_to calls the wrong driver method and hotRestart is byte-for-byte identical to hotReload. The HIGH _buildRpcCall gap means primary assertion primitives are broken in the --server= path.

---

🔴 CRITICAL — 2 issues

[CRITICAL] Correctness — skill_server.dart:scroll_to

Dispatches to driver.swipe() instead of driver.scrollTo(). AppDriver has a dedicated scrollTo({key, text}) that sends the correct VM extension. Using swipe() performs a raw gesture — it does not find the widget and scroll it into view.

[CRITICAL] Correctness — flutter_driver.dart:hotRestart

hotRestart() is byte-for-byte identical to hotReload() — both call _service!.reloadSources(). A restart resets app state; a reload does not. SkillServer calls this and returns success: true with no warning. Implement a real restart or return a 501-style error.

🟠 HIGH — 4 issues

[HIGH] Correctness — act.dart:_buildRpcCall

assert_visible, assert_gone, wait_for_element, get_text, find_element, hot_reload, hot_restart fall through to default: return {'method': action, 'params': {}}. assert_visible "Login" via --server= silently sends null key — assertion always passes, masking real test failures.

[HIGH] Correctness — act.dart:_actViaServers screenshot

With --server=app-a,app-b and a single output path, last server's bytes overwrite earlier ones silently. Data loss. Derive per-server paths (e.g. path_app-a.png).

[HIGH] Correctness — skill_server.dart:shutdown

Future.microtask(() async { await stop(); exit(0); }) bypasses doShutdown() in connect.dart, leaving the registry entry stale. Complete a Completer instead so doShutdown() handles cleanup before exit.

[HIGH] Testing

Zero automated tests for ServerRegistry, SkillServer, SkillClient, runConnect, runServerCmd, output_format.dart.

🟡 MEDIUM — 5 issues

[MEDIUM] Correctness — skill_server.dart (socket write errors)

_sendResult/_sendError call socket.writeln() without error handling. A disconnected client causes an unhandled SocketException caught by _handleLine's generic catch, which then attempts _sendError on the same broken socket.

[MEDIUM] Correctness — skill_client.dart (StreamSubscription never cancelled)

After completer.complete(), the subscription keeps the socket draining. Save and call subscription.cancel() after completing.

[MEDIUM] Architecture — server_registry.dart (ID collision)

register() silently overwrites an existing live entry. Check if the ID exists and PID is alive; reject with a clear error.

[MEDIUM] Correctness — docs/cli-server-commands.md

CI example uses flutter_skill ping --server=ci-test but ping is not yet a top-level CLI command — hits default and exits 1. Expose ping as a CLI command or replace the example.

[MEDIUM] Security — skill_server.dart (unauthenticated shutdown)

Any local process can call shutdown on the unauthenticated TCP port. Consider a random nonce in the registry file required for shutdown. At minimum, document the limitation.

🔵 LOW — 4 issues

[LOW] Correctness — connect.dart (URI normalisation)

http://host/abc → ws://host/abc/ws (wrong path). Strip existing path before appending /ws.

[LOW] Naming — output_format.dart

stripOutputFlag deprecated alias is unused in the entire codebase. Remove it entirely.

[LOW] Architecture — launch.dart:_spawnDetachedServer

Platform.script.toFilePath() returns empty or snapshot path when running as compiled binary. Use Platform.executable.

[LOW] Code Duplication — skill_server.dart (assert cases)

assert_visible, assert_gone, wait_for_element repeat the same element-search logic ~60 lines. Extract _findElementInList() helper.

---

Verdict

CHANGES REQUESTED — Two CRITICAL bugs (scroll_to wrong method, hotRestart == hotReload) silently produce wrong behaviour, and the HIGH _buildRpcCall gap means assertions are broken in the --server= path — the primary new feature of this PR.

[vinifig]

Code Review — commit 5c14c9f

Summary

Architecture sound, most prior feedback addressed. Five issues remain before merge: inconsistent JSON schemas between single/multi-server paths, direction silently dropped in scroll_to, --output=json ignored in the direct-VM act path, missing tests, and two lower-severity state/validation issues.

---

🟠 HIGH — 3 issues

[HIGH] API Contract — inspect.dart + act.dart

Inconsistent JSON schema between single-server and multi-server paths. inspect --output=json emits {"elements":[...]} but inspect --server=myapp --output=json emits [{"server":"myapp","success":true,"data":{...},"duration_ms":5}]. Any script that conditionally passes --server must branch on response shape. When exactly one server is given, unwrap the envelope to match the direct-VM schema.

[HIGH] Correctness — act.dart:_buildRpcCall / skill_server.dart:_dispatch

_buildRpcCall for scroll_to sends direction but the server's scroll_to dispatch only passes key/text to FlutterSkillClient.scrollTo() — direction silently ignored. Remove direction from _buildRpcCall's scroll_to output or thread it through scrollTo().

[HIGH] Correctness — act.dart:runAct (direct-VM path)

format is resolved but every case in the direct-VM switch unconditionally prints human text. A CI environment running flutter_skill act tap "Login" (with CI=true) silently gets human output instead of JSON. Wrap each print with if (format == OutputFormat.json) { ... } else { ... }.

🟡 MEDIUM — 3 issues

[MEDIUM] Correctness — connect.dart

ID validation happens inside ServerRegistry.register(), after driver.connect() already succeeded. An invalid --id leaves the driver connected and never cleaned up. Validate id at the top of runConnect before any I/O.

[MEDIUM] State Management — skill_server.dart:stop()

Not re-entrant — concurrent calls (e.g. two rapid shutdown RPCs) double-unregister and double-close. Add bool _stopped = false; guard.

[MEDIUM] Testing

Zero tests for SkillServer, SkillClient, ServerRegistry, connect.dart, server_cmd.dart, ping_cmd.dart. Add unit tests for ServerRegistry I/O and an integration smoke test (start SkillServer against mock driver, ping via SkillClient, shutdown).

🔵 LOW — 2 issues

[LOW] Correctness — skill_server.dart:stop()

conn.close().catchError((_) => conn) returns Socket not void. Fix: conn.close().catchError((_) {}).

[LOW] Architecture — launch.dart:_extractUri

Only matches ws:// prefixes. Newer Flutter versions advertise http:// URIs — silently dropped, no server registered. Extend regex to match http://127.0.0.1:PORT too.

---

Verdict

CHANGES REQUESTED — Inconsistent JSON schema and ignored --output=json in the direct-VM path are real breakages for CI/scripting consumers. Must be resolved before merge.

[vinifig]

Code Review — commit 91f240a (Round 5 / Final)

Summary

Well-structured feature with solid security mitigations (path traversal, ID validation, collision guard, Windows PID word-boundary matching) and the onShutdownRequested stream. All previous CRITICAL/HIGH blockers are fully resolved. Remaining issues are two medium correctness bugs worth fixing before the next release, and a collection of low-severity polish items. None are merge-blocking.

---

🟡 MEDIUM — 3 issues

[MEDIUM] Correctness — skill_client.dart

StreamSubscription not cancelled when the 30-second timeout fires. onTimeout throws TimeoutException but the subscription keeps the socket stream alive; a late-arriving response can attempt to complete an already-completed Completer, throwing unhandled errors. Call subscription.cancel() inside onTimeout before throwing.

[MEDIUM] Correctness — skill_server.dart (scroll_to fallback)

The bare catch (_) around FlutterSkillClient.scrollTo swallows ALL exceptions — including real connection errors — silently degrading to a generic swipe with no error surfaced to the caller. Use driver is! FlutterSkillClient before casting instead of a catch-all.

[MEDIUM] Correctness — skill_server.dart (wait_for_element)

The polling loop blocks dispatch of other concurrent JSON-RPC requests for up to the full timeout window. Add a comment documenting this blocking behaviour so future maintainers are aware.

🔵 LOW — 7 issues

[LOW] Correctness — act.dart (screenshot confirmation)

In human output mode, no confirmation printed after writing the file. Direct-VM path prints 'Screenshot saved to $path ($bytes bytes)'. Match parity.

[LOW] Error Handling — server_cmd.dart:_cmdStop

server stop --id=nonexistent exits 0 and prints success even when no entry existed. Exit 1 when the registry had no entry to stop.

[LOW] API Contract — skill_server.dart (go_back)

Returns {'success': true} silently when the driver doesn't support go_back. Add a 'warning' key like hot_restart does.

[LOW] Code Duplication — act.dart vs skill_server.dart

'scroll' is mapped to 'scroll_to' only in _buildRpcCall. Calling 'scroll' directly on the server hits default and throws "Method not found". Add case 'scroll': as an alias in _dispatch, or document that 'scroll' is a CLI-only alias.

[LOW] Naming — flutter_driver.dart

Several print('DEBUG: ...') statements remain in connect() and _reconnect(). Remove or gate behind a verbose flag.

[LOW] Testing

prune() has no test. wait_for_element timeout expiry and screenshot null-return error path are untested. Worth adding before next release.

[LOW] State Management — skill_server.dart

onShutdownRequested.listen() in connect.dart is set up after start() — the Future.microtask delay in the shutdown dispatch makes this work, but it's a fragile timing assumption. Add an inline comment explaining the microtask dependency.

---

Verdict

✅ APPROVED WITH COMMENTS

Architecture is correct, security mitigations are solid, and all previous CRITICAL/HIGH blockers are fully resolved. The two medium items (subscription not cancelled on timeout, scroll_to catch-all) should be addressed before the next release but are not merge-blocking. All remaining findings are low-severity polish.