Skip to content

fix: report an unregistered extension and a still-building start without a stack - #67

Merged
anilcancakir merged 2 commits into
masterfrom
fix/connected-extension-and-start-timeout
Oct 11, 2026
Merged

anilcancakir merged 2 commits into
masterfrom
fix/connected-extension-and-start-timeout

Conversation

@anilcancakir

Copy link
Copy Markdown
Member

Summary

Two defects from a field test of app-local connected commands (Watchools player:*, which call ext.watchools.player from bin/dispatcher.dart):

  1. A connected command run while the app was still booting printed Connected command player:state failed: ext.watchools.player: (-32601) Method not found plus a vm_service stack (exit 3), then worked a few seconds later. A consumer cannot catch this itself without importing package:vm_service, which trips depend_on_referenced_packages.
  2. start --device macos on a fresh worktree: the first native build (CocoaPods, SwiftPM) outlasted the 90 s scrape, the timeout's StateError escaped as an unexpected error whose visible tail was <asynchronous suspension>, and the session was left booting: true with no URI while the app kept building.

Changes:

  • VmServiceClient.callServiceExtension maps JSON-RPC -32601 to a new public ExtensionNotRegisteredException, on the first call and on both retries (collected promise, refreshed isolate after a hot restart). ArtisanApplication prints it as one line and exits 75.
  • ArtisanCommand.exitTryAgain = 75 (EX_TEMPFAIL): "not ready yet, retry".
  • start, plain branch: the scrape throws TimeoutException. With flutter run alive, start keeps the booting session, prints flutter run (pid <n>) is still building after <n>s with the status / retry / --timeout hints and exits 75. With it gone, it stops the FIFO holder, removes the session (the log stays), names the log and exits 1.
  • A connected command against a booting session with no URI yet exits 75 while the recorded pid is alive (was 1), so a caller can retry on one code from start to the first command that connects.
  • --cdp-port branch: behaviour unchanged (fail + reap, exit 1); the message drops the TimeoutException after <duration> prefix.
  • Docs: doc/commands/start.md, skills/.../state-and-recovery.md, mcp-tools.md, SKILL.md, CHANGELOG under [Unreleased].

Left as is: a permanent -32601 (a typo in the method, or a plugin newer than the app's copy) also exits 75 with "retry"; telling it apart would need an extensionRPCs lookup per failure. A web runtime that reports a missing extension with a code other than -32601 (older DWDS) keeps the old output.

Verification

dart test: 1432 passed; dart analyze lib/ test/ bin/ and dart format clean; line coverage 87.78%; dart pub publish --dry-run 0 warnings. Each new test failed first. The extension tests run against a fake JSON-RPC WebSocket server; a real Dart VM with no extension registered printed the one line and exited 75. The cold-build path is covered through the start seams; it has not been run against a real multi-minute macOS build.

…out a stack

A connected command run before the app registered its extension printed the
raw `(-32601) Method not found` RPC error with a vm_service stack. The client
now maps -32601 (retries included) to ExtensionNotRegisteredException, and the
dispatcher prints one line and exits 75 (ArtisanCommand.exitTryAgain).

`start` on a cold native build let the scrape timeout escape as an unexpected
error. The plain branch now checks the flutter run pid: alive, it keeps the
booting session, prints a still-building line and exits 75; gone, it reaps the
FIFO holder, removes the session, names the log and exits 1. A connected
command against a booting session whose app is alive also exits 75.
@kodizm

kodizm Bot commented Oct 10, 2026

Copy link
Copy Markdown

Note

Kodizm (AI-generated). May contain mistakes; verify before acting.

The change looks correct to me, and I have no findings; the one gap is that the repo's own CI workflow has not reported on this commit.

Why I think it is correct:

  • -32601 is mapped inside _invokeExtension (lib/src/vm/vm_service_client.dart:121), so the first call and both retries all raise ExtensionNotRegisteredException. The exception is not a vm.RPCError, so it does not wrongly trigger the -32603 retry.
  • Callers keep working. The MCP plugin dispatch (lib/src/mcp/mcp_server.dart:296) catches everything and prints $e, which now gives the one-line message. dusk_evaluate goes through evaluate, not this path, so it is unaffected.
  • When flutter run has died (lib/src/commands/start_command.dart:574), StateFile.delete() removes only the session's state.json and the legacy pointer. flutter-dev.log stays, as the message promises.
  • Docs, skill references and CHANGELOG [Unreleased] are updated, as Golden Rules 1, 2 and 5 in CLAUDE.md require.
  • The new type is a final class, matching the conventions in CLAUDE.md.

Tests

New tests cover every new path:

  • The -32601 mapping on the first call and on the retries, against a fake WebSocket server.
  • The dispatcher printing one line and exiting 75, for both an unregistered extension and a live booting session.
  • start timing out with flutter run alive (exit 75) and gone (exit 1, session reaped), through the startIsAlive seam.
  • The --cdp-port message no longer carrying the TimeoutException after prefix.

CI

  • Auto-merge low-risk Dependabot PRs: skipped. This is the only check reported for 3b50620, so the overall: success it produces is not a real pass.
  • CI / Lint & Test (format, analyze, tests, 80% coverage floor, publish dry-run): no result reported for this commit, although .github/workflows/ci.yml triggers on pull requests into master. Treat this review as static until that run shows up and passes.

…sion-and-start-timeout

# Conflicts:
#	CHANGELOG.md
@codecov

codecov Bot commented Oct 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
lib/src/vm/extension_not_registered_exception.dart 50.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@kodizm

kodizm Bot commented Oct 10, 2026

Copy link
Copy Markdown

Note

Kodizm (AI-generated). May contain mistakes; verify before acting.

The only new commit merges master into this branch, and the merge looks correct. I have no new findings.

The new commit is 100cdd0, a merge of origin/master. It brings in the 0.0.19 release (#66), the bump to xml 7 (#64) and cupertino_icons 2.0.0 for the example app (#65). Those changes were reviewed in their own pull requests, so I did not review them again here. The one hand-resolved conflict was in CHANGELOG.md, and it is resolved correctly: this PR's Added and Fixed entries stay under [Unreleased], above ## [0.0.19] - 2026-10-09. The merge leaves this PR's own code unchanged, and that code does not touch the XML helpers that #64 changed.

Tests

The tests for this PR are the same as in my last review. The merge only adjusts the existing XML and installer tests that came from #64.

CI

  • Lint & Test: success. This answers the gap I raised last time, because this check had not reported then.
  • codecov/patch: success (94.11% of the diff covered, target 80%).
  • Auto-merge low-risk Dependabot PRs: skipped. This is expected, since this is not a Dependabot PR.

@anilcancakir
anilcancakir merged commit 6802001 into master Oct 11, 2026
4 checks passed
@anilcancakir
anilcancakir deleted the fix/connected-extension-and-start-timeout branch October 11, 2026 11:44
@anilcancakir anilcancakir mentioned this pull request Oct 11, 2026
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.

1 participant