feat: Add controller-backed launch and REPL by bsutton · Pull Request #122 · andrzejchm/fdb

bsutton · 2026-05-01T09:41:54Z

Keeps the controller-backed launch work in one package so global activation stays simple while follow-up commands talk to the running Flutter session reliably.

This also tightens status, kill, wait, and app-death handling around stale controller or app process state.

andrzejchm

Thanks for the contribution! 👏🏻

I love the controller idea and making fdb launch less fragile. Left some comments on the output contract, code structure, and a few things that look like merge conflict casualties. 🙏🏻

andrzejchm · 2026-05-01T12:57:08Z

-      stdout.writeln('PID=$pid');
-      stdout.writeln('LOG_FILE=$logFilePath');
+    case LaunchSuccess(:final logFilePath):
+      stdout.writeln('launch: app is running');


This breaks the existing fdb launch output contract. Before this, success returned machine-readable tokens: APP_STARTED, VM_SERVICE_URI=..., PID=..., and LOG_FILE=.... Now it prints free-form text. The repo treats those tokens as a stable interface for agents and scripts. CODE-STYLE.md says smoke tests assert them byte-identically. Internal architecture can change, but the launch output should stay compatible.

I think we need two interfaces. Machine readable and when in interactive mode a human readable mode.

there is also the question as to whether 'machine readable' means anything when we are discussion ai usage - because everything is machine readable.

andrzejchm · 2026-05-01T12:57:08Z

+      return runMemCli(args);
+    case 'gc':
+      return runGcCli(args);
+    case 'skill':


grant-permission looks dropped by accident. No dispatch case for it here, and it is also missing from bin/fdb.dart. That makes fdb grant-permission an unknown command after this PR. Looks unrelated to the controller work and should be restored before merge.

andrzejchm · 2026-05-01T12:57:08Z

+            }
+          case 'app.stop':
+            _running = false;
+          case 'app.log':


This looks like it drops developer.log() capture. The old launch flow started a VM-service log collector specifically because flutter run does not forward developer.log() to stdout. Cleanup for the old collector files is still here but there is no replacement subscribing to the VM Logging stream. The controller only appends app.log and app.progress events from flutter run --machine. If that is right, fdb logs will miss developer.log() output again.

andrzejchm · 2026-05-01T12:57:08Z

+  final bool verbose;
+}
+
+_ControllerConfig? _parseArgs(List<String> args) {


The rest of fdb uses package:args for CLI parsing. This manual index-walking loop is fragile. If a flag like --device appears last with no value, args[++i] throws a RangeError with no useful message. ArgParser handles that and gives consistent error output for free.

updated to use args.

andrzejchm · 2026-05-01T12:57:08Z

+          'ok': result['result'] == true,
+          'result': result['result'],
+        };
+      case 'refresh_vm_uri':


refresh_vm_uri is a subset of status. Both return _vmUri ?? readVmUri(), this one just drops running and pid. The only caller in vm_service.dart only needs the URI. Consider removing refresh_vm_uri and having callers extract vmServiceUri from status instead.

andrzejchm · 2026-05-01T12:57:08Z

+    }
+  }
+
+  Future<void> _writeAppPidFromVm(String wsUri) async {


Manual WebSocket getVM round-trip that already exists in lib/core/vm_service.dart. Worth checking if a shared helper is feasible. Note that vm_service.dart calls sendControllerCommand, so extracting a helper would need to avoid a circular dependency.

I'm pulling all vm service methods into the vm_service library to hide the implementation details.

I've done a large refactor around this to hide maps by deserializing each map response into a response classe.

andrzejchm · 2026-05-01T12:57:08Z

+      if (appPid != null) {
+        File(appPidFile).writeAsStringSync(appPid.toString());
+      }
+    } catch (_) {}


catch (_) {} swallows all errors silently. Per CODE-STYLE.md that is only acceptable for genuinely non-critical failures. At minimum append the error to the log so there is a trace when app PID resolution fails.

removed the catch as the methods called do not thrown. Added an return check that logs if the kill fails.

andrzejchm · 2026-05-01T12:57:08Z

+    final ok = response['ok'] == true;
+    if (!ok) {
+      throw ControllerUnavailable(
+        response['error'] as String? ?? 'Controller command failed.',


ControllerUnavailable is thrown both when the socket cannot be reached and when the controller responds with ok: false. Those are different failure modes. Callers in reload.dart and restart.dart catch ControllerUnavailable and treat it as no session, so a live controller returning a command error gets misread as a dead session. Consider a separate exception type for command-level errors.

a new ControllerCommandFailed exception has been introduced.

andrzejchm · 2026-05-01T12:57:08Z

+      timeout: const Duration(seconds: reloadTimeoutSeconds),
+    );
+    return ReloadSuccess(stopwatch.elapsedMilliseconds);
+  } on ControllerUnavailable {


The ControllerUnavailable fallback reads PIDs to decide between NoSession, ProcessDead, and Failed. But if the controller was alive and returned a command error (see comment on controller_client.dart:71), these PID checks are looking at the wrong thing and may return the wrong result variant. Same issue applies to restart.dart.

these failure modes have now been differentiated.

andrzejchm · 2026-05-01T12:57:08Z

-      if (currentPid != null && !isProcessAlive(currentPid)) {
-        throw await buildAppDiedException(pid: currentPid);
-      }
+  while (true) {


The recoveredOnce flag bounds retries, but the loop can still call _refreshVmUriFromController twice on a single invocation. Once at line 23 when the URI file is missing, and again inside the timeout/connection-refused handler. A short comment explaining the intended retry limit would make this easier to reason about.

I think I've done what you requested.

bsutton · 2026-05-02T13:06:33Z

I'm looking to improve the code structure. We currently have the fdb front end making calls directly into the vm service as well as the controller calling the vm.

I'm looking to have all communication with the vm go via the controller as I think this will be cleaner and also make it simpler to implement different front ends (e.g. cli and mcp).

…tion details. Moved to the new controller command mode - all communications with the running app and vm service now go through the controller for consistency. Changed cli commands to use an enum rather than hard coded strings. Removed the switch statement from runFdbCommand as we can now use the cli command enum to dispatch directly. Added barrel files.

…variables.

…liveness # Conflicts: # CHANGELOG.md

bsutton · 2026-05-04T22:14:38Z

I've done a major refactor so the original review is somewhat outdated.

The controller now sits in its own package and 'owns' the vm service connection. Previously some of the fdb code reacted with the vm bypassing the controller.

I've done some fairly minor changes to the core fdb code to remove some string based switch statements - these now use a enum and the hard coded command strings have been removed.

andrzejchm · 2026-05-06T12:07:32Z

Thanks for the work here. There is probably a good direction in this PR, but I don't think it can land like this.

The problem is size and scope. This PR folds a lot of separate changes into one diff:

a new controller package/process
launch and session-state changes
command dispatch refactor
REPL / interactive mode
docs and test updates

That makes it very hard to review safely. I can't cleanly separate mechanical refactor from behavior changes, and I can't tell which failures are intended contract changes vs real regressions. We already have at least one real regression in stale/interrupted-session handling, plus drift between the runtime session files and the task/test expectations.

I also don't think the package split is earned yet. A separate controller process may be the right move. A separate fdb_controller package feels early. Right now it still looks like an internal part of fdb, not a reusable library with its own consumers. The package split adds a lot of files and wiring on top of an already large behavior change.

I don't want to block the idea. I do want this broken up.

A shape that would be much easier to review:

CLI dispatch refactor only
No behavior changes. Just command routing / enum / generated wiring.
Introduce controller support inside fdb
Add the controller process, transport, and request/response scaffolding as internal code first. No need to move every command yet.
Migrate one small vertical slice
For example status and one runtime command. That gives us a chance to settle controller/session failure semantics before this spreads across the whole tool.
Migrate launch and session ownership
This should be its own PR. It changes the runtime contract and smoke-test behavior.
Add REPL / interactive mode
This is user-facing and easier to review once the controller/session behavior is stable.
Migrate the remaining commands in small batches
Group them mechanically and keep the tests green as you go.

If splitting this up feels like too much overhead, I can help with that part. I'm happy to sketch the PR breakdown, or even take the current branch and split it into reviewable chunks so we can discuss the pieces one by one.

bsutton · 2026-05-06T12:27:09Z

Can I suggest another route, rather than trying to review the code line by line we focus on expanding the tests to ensure the code is behaving as expected. This also a better path for the long term health of the package. The package separation is intended to force a separation of concerns, previously the layers overlapped. The way we code has changed, we need to change the process of accepting code.

…

On Wed, 6 May 2026, 10:07 pm Andrzej Chmielewski, ***@***.***> wrote: *andrzejchm* left a comment (andrzejchm/fdb#122) <#122 (comment)> Thanks for the work here. There is probably a good direction in this PR, but I don't think it can land like this. The problem is size and scope. This PR folds a lot of separate changes into one diff: - a new controller package/process - launch and session-state changes - command dispatch refactor - REPL / interactive mode - docs and test updates That makes it very hard to review safely. I can't cleanly separate mechanical refactor from behavior changes, and I can't tell which failures are intended contract changes vs real regressions. We already have at least one real regression in stale/interrupted-session handling, plus drift between the runtime session files and the task/test expectations. I also don't think the package split is earned yet. A separate controller process may be the right move. A separate fdb_controller package feels early. Right now it still looks like an internal part of fdb, not a reusable library with its own consumers. The package split adds a lot of files and wiring on top of an already large behavior change. I don't want to block the idea. I do want this broken up. A shape that would be much easier to review: 1. CLI dispatch refactor only No behavior changes. Just command routing / enum / generated wiring. 2. Introduce controller support inside fdb Add the controller process, transport, and request/response scaffolding as internal code first. No need to move every command yet. 3. Migrate one small vertical slice For example status and one runtime command. That gives us a chance to settle controller/session failure semantics before this spreads across the whole tool. 4. Migrate launch and session ownership This should be its own PR. It changes the runtime contract and smoke-test behavior. 5. Add REPL / interactive mode This is user-facing and easier to review once the controller/session behavior is stable. 6. Migrate the remaining commands in small batches Group them mechanically and keep the tests green as you go. If splitting this up feels like too much overhead, I can help with that part. I'm happy to sketch the PR breakdown, or even take the current branch and split it into reviewable chunks so we can discuss the pieces one by one. — Reply to this email directly, view it on GitHub <#122 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG32OFJBEPCEYYAYBJMDIL4ZMTJXAVCNFSM6AAAAACYNIHN52VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DGOBXG42DENZRGQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

andrzejchm · 2026-05-06T14:07:26Z

I agree on improving the tests, and we should keep doing that.

My concern is simpler: this is too much change for one PR in a tool people rely on.

Even if the direction is right, this changes too many things at once: process model, package structure, session ownership, command dispatch, interactive mode, and command migration across the tool. That is too much surface area to assess safely in one step.

This is also a hard project to fully de-risk with tests alone. fdb needs real device and simulator coverage across platforms, and a lot of that testing is stateful and somewhat flaky by nature. So I don't think expanding the tests is enough reason to take a rewrite of this size in one go.

The reason I want this split is stability. Smaller PRs let us assess the impact step by step, keep the tool usable while we migrate it, and catch regressions earlier.

So I still think the right path is to break this into smaller, focused PRs and validate the change in stages.

bsutton · 2026-05-06T21:29:30Z

I don't actually think there is a path here that doesn't require a major refactoring. We have moved code into a separate process which requires process model and session ownership.
The command dispatch is minor in this and not hard to test.

The repl isn't a stability issue given its a new feature which can be ignored by users.

As to the risk, we can managed that by releasing the changes as a beta. Assuming that others are having the same problem that I was then there is going to be high motivation for those users to try the beta.

The last issue, is that while I had enough time to drive the changes this far, I'm not sure I have enough time to go through the incremental upgrade process you are describing.

andrzejchm · 2026-05-07T10:44:43Z

Thanks for the work here. There is a lot of good effort in this PR, and I appreciate the direction behind it.

I’m going to take it over from here, test it locally, make the changes I think are needed to get it into a shape I’m comfortable landing, and then reissue it for you to review.

I’ll make sure attribution stays with you. This is your feature and your contribution, and I’ll preserve that when I rework and split it.

bsutton · 2026-05-07T12:19:45Z

Sounds great, thanks.

…

On Thu, 7 May 2026, 8:45 pm Andrzej Chmielewski, ***@***.***> wrote: *andrzejchm* left a comment (andrzejchm/fdb#122) <#122 (comment)> Thanks for the work here. There is a lot of good effort in this PR, and I appreciate the direction behind it. I’m going to take it over from here, test it locally, make the changes I think are needed to get it into a shape I’m comfortable landing, and then reissue it for you to review. I’ll make sure attribution stays with you. This is your feature and your contribution, and I’ll preserve that when I rework and split it. — Reply to this email directly, view it on GitHub <#122 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG32OEC3DE7SDOBX3VV5C34ZRSLDAVCNFSM6AAAAACYNIHN52VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DGOJWGQYTEMJTHA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

Keep the controller process internal to fdb so global activation stays simple while preserving controller-backed runtime behavior. Co-authored-by: Brett Sutton <bsutton@noojee.com.au>

# Conflicts: # CHANGELOG.md

Add controller-backed launch and REPL

bc4afd4

andrzejchm requested changes May 1, 2026

View reviewed changes

Merge upstream main

56cbf9d

bsutton and others added 6 commits May 3, 2026 23:00

Refactor controller command handling

b8cd6e9

reverted the launch output so it shows the original machine readable …

6f04c9e

…variables.

Move controller into fdb_controller package

d21f1c2

Merge remote-tracking branch 'upstream/main' into fix/android-status-…

8c6f20f

…liveness # Conflicts: # CHANGELOG.md

Merge branch 'main' into fix/android-status-liveness

b422d15

bsutton requested a review from andrzejchm May 4, 2026 22:14

andrzejchm and others added 5 commits May 7, 2026 16:09

refactor: fold controller into main package

7f97195

Keep the controller process internal to fdb so global activation stays simple while preserving controller-backed runtime behavior. Co-authored-by: Brett Sutton <bsutton@noojee.com.au>

Merge remote-tracking branch 'origin/main' into worktree-takeover-pr-122

5fa4671

# Conflicts: # CHANGELOG.md

docs: align agent scenarios with current describe output

5ae9608

fix controller liveness edge cases

3689f2a

fix controller and launch edge cases

1450912

andrzejchm changed the title ~~Add controller-backed launch and REPL~~ feat: Add controller-backed launch and REPL May 7, 2026

Conversation

bsutton commented May 1, 2026 • edited by andrzejchm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrzejchm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsutton commented May 2, 2026

Uh oh!

bsutton commented May 4, 2026

Uh oh!

andrzejchm commented May 6, 2026

Uh oh!

bsutton commented May 6, 2026 via email

Uh oh!

andrzejchm commented May 6, 2026

Uh oh!

bsutton commented May 6, 2026

Uh oh!

andrzejchm commented May 7, 2026

Uh oh!

bsutton commented May 7, 2026 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bsutton commented May 1, 2026 •

edited by andrzejchm

Loading