From 0651946aef68f48eabc812b0c7a76926c84077e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?An=C4=B1lcan=20=C3=87ak=C4=B1r?= Date: Thu, 25 Jun 2026 15:40:55 +0300 Subject: [PATCH 1/2] fix: surface a failing bootstrap_command instead of implying success plugin:install discarded the bootstrap subprocess ProcessResult and only checked the notResolvable case, so a chained bootstrap_command that exited non-zero (stale fast-CLI bundle, unknown command, scaffold error) was reported to the operator as if it had completed. A from-scratch install QA hit this: the chained starter:install failed silently and the manifest post_install message still claimed the scaffold succeeded. BootstrapCommandRunner.run now returns a BootstrapRunResult carrying the subprocess exit code and captured stderr. plugin:install warns with the exit code + stderr and prints the manual bootstrap hint when the chained command exits non-zero; the success path stays quiet. Tests: runner exit-code/stderr propagation + succeeded() on zero exit; a non-zero bootstrap exit warns + hints without failing the install. --- CHANGELOG.md | 1 + .../helpers/bootstrap_command_runner.dart | 56 +++++++++++++++---- lib/src/commands/plugin_install_command.dart | 19 ++++++- .../bootstrap_command_runner_test.dart | 45 ++++++++++++++- .../commands/plugin_install_command_test.dart | 50 +++++++++++++++-- 5 files changed, 152 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fead316..cabd55f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ This project follows [Semantic Versioning 2.0.0](https://semver.org/spec/v2.0.0. ### Fixed +- `plugin:install` now surfaces a failing `bootstrap_command` instead of implying success. `BootstrapCommandRunner.run` returns a `BootstrapRunResult` carrying the subprocess exit code and captured stderr (it previously discarded the `ProcessResult`), and `plugin:install` warns with the exit code + stderr and prints the manual bootstrap hint when the chained command exits non-zero. A bootstrap that fails (stale fast-CLI bundle, unknown command, scaffold error) is no longer reported to the operator as if it had completed. - `start --timeout=` now rejects zero and negative values immediately with an actionable error ("--timeout must be a positive integer"), instead of silently passing a non-positive deadline to the VM Service scrape loop and producing a confusing "Timed out after 0s" failure. - Passing an unknown option to any command now fails loudly instead of silently printing help and exiting as if help were requested (issue #12). The dispatcher writes `Unknown option: ` to stderr (both long `--foo` and short `-x` forms), prints the command help, and exits non-zero. Other parse failures keep their original messages: a missing option value (`Missing argument for "..."`), a disallowed value, and a value given to a flag each surface their specific diagnostic unchanged. `--help` / `-h` and every valid invocation are unaffected. Because this is the shared dispatch path for every command, the fix benefits every plugin CLI built on the substrate. diff --git a/lib/src/commands/helpers/bootstrap_command_runner.dart b/lib/src/commands/helpers/bootstrap_command_runner.dart index 9cd70f7..a2ebf3c 100644 --- a/lib/src/commands/helpers/bootstrap_command_runner.dart +++ b/lib/src/commands/helpers/bootstrap_command_runner.dart @@ -38,6 +38,33 @@ enum BootstrapRunOutcome { notResolvable, } +/// Result of a [BootstrapCommandRunner.run] attempt. +/// +/// Carries the [outcome] plus, when a subprocess actually ran, its +/// [exitCode] and captured [stderr]. The caller uses [succeeded] to decide +/// whether the chained command completed; a non-zero exit (a stale bundle, an +/// unknown command, a scaffold error) must NOT be reported to the operator as +/// success, which is exactly what discarding the [ProcessResult] used to do. +class BootstrapRunResult { + const BootstrapRunResult({ + required this.outcome, + this.exitCode, + this.stderr, + }); + + final BootstrapRunOutcome outcome; + + /// Subprocess exit code, or `null` when [outcome] is `notResolvable` + /// (nothing was spawned). + final int? exitCode; + + /// Captured subprocess stderr, or `null` when nothing was spawned. + final String? stderr; + + /// True only when a subprocess ran AND exited zero. + bool get succeeded => outcome == BootstrapRunOutcome.invoked && exitCode == 0; +} + /// Runs a plugin's declared `bootstrap_command` as a fresh dispatcher /// subprocess after `plugin:install` registers the plugin. /// @@ -84,10 +111,10 @@ class BootstrapCommandRunner { /// @param bootstrapCommand The plugin command to chain (e.g. /// `starter:install`). /// @param projectRoot Absolute path to the consumer project root. - /// @return [BootstrapRunOutcome.invoked] when a dispatcher was resolved and - /// the subprocess ran; [BootstrapRunOutcome.notResolvable] when no - /// dispatcher could be resolved. - Future run({ + /// @return a [BootstrapRunResult]: `invoked` (with the subprocess exit code + /// and captured stderr) when a dispatcher was resolved and ran; + /// `notResolvable` when no dispatcher could be resolved. + Future run({ required String bootstrapCommand, required String projectRoot, }) async { @@ -95,17 +122,26 @@ class BootstrapCommandRunner { // else `dart run :artisan`. Return early when neither is // resolvable so the caller can fall back to the hint. final invocation = _resolveDispatcher(projectRoot, bootstrapCommand); - if (invocation == null) return BootstrapRunOutcome.notResolvable; + if (invocation == null) { + return const BootstrapRunResult( + outcome: BootstrapRunOutcome.notResolvable, + ); + } - // 2. Run the chained command as a subprocess scoped to the consumer root. - // The chained command's own exit code does not change this outcome: - // "invoked" reports that the auto-run fired, not that it succeeded. - await _runner( + // 2. Run the chained command as a subprocess scoped to the consumer root, + // and surface its exit code + stderr so the caller can tell success + // from a silent failure (a stale bundle, an unknown command, or a + // scaffold error must not be reported to the operator as success). + final result = await _runner( invocation.executable, invocation.arguments, workingDirectory: projectRoot, ); - return BootstrapRunOutcome.invoked; + return BootstrapRunResult( + outcome: BootstrapRunOutcome.invoked, + exitCode: result.exitCode, + stderr: result.stderr?.toString(), + ); } /// Resolves the dispatcher executable + argument list, or `null` when no diff --git a/lib/src/commands/plugin_install_command.dart b/lib/src/commands/plugin_install_command.dart index 3ddf7b8..ce8a7d5 100644 --- a/lib/src/commands/plugin_install_command.dart +++ b/lib/src/commands/plugin_install_command.dart @@ -307,11 +307,26 @@ class PluginInstallCommand extends ArtisanInstallCommand { // issues) must not crash `plugin:install`: catch it, warn, and fall back // to the manual hint so the operator can still bootstrap by hand. try { - final outcome = await buildBootstrapRunner().run( + final result = await buildBootstrapRunner().run( bootstrapCommand: bootstrap, projectRoot: getProjectRoot(), ); - if (outcome == BootstrapRunOutcome.notResolvable) { + + // No dispatcher resolved: nothing ran, fall back to the manual hint. + if (result.outcome == BootstrapRunOutcome.notResolvable) { + _emitBootstrapHint(ctx, bootstrap: bootstrap, skipped: false); + return; + } + + // A subprocess ran but exited non-zero: surface the failure instead of + // letting the manifest post_install message imply the chain succeeded. + if (!result.succeeded) { + final detail = (result.stderr ?? '').trim(); + ctx.output.warning( + 'The bootstrap command "$bootstrap" exited with code ' + '${result.exitCode} and may not have completed.' + '${detail.isEmpty ? '' : '\n$detail'}', + ); _emitBootstrapHint(ctx, bootstrap: bootstrap, skipped: false); } } catch (e) { diff --git a/test/commands/helpers/bootstrap_command_runner_test.dart b/test/commands/helpers/bootstrap_command_runner_test.dart index 278f081..aa46faa 100644 --- a/test/commands/helpers/bootstrap_command_runner_test.dart +++ b/test/commands/helpers/bootstrap_command_runner_test.dart @@ -44,7 +44,7 @@ void main() { projectRoot: root.path, ); - expect(result, BootstrapRunOutcome.invoked); + expect(result.outcome, BootstrapRunOutcome.invoked); expect(runner.executable, './bin/fsa'); expect( runner.arguments, @@ -71,7 +71,7 @@ void main() { projectRoot: root.path, ); - expect(result, BootstrapRunOutcome.invoked); + expect(result.outcome, BootstrapRunOutcome.invoked); expect(runner.executable, 'dart'); expect( runner.arguments, @@ -99,11 +99,50 @@ void main() { projectRoot: root.path, ); - expect(result, BootstrapRunOutcome.notResolvable); + expect(result.outcome, BootstrapRunOutcome.notResolvable); expect(runner.executable, isNull, reason: 'no dispatcher means the runner is never invoked'); }); + test('surfaces the subprocess exit code and stderr on failure', () async { + final root = Directory.systemTemp.createTempSync('bootstrap_fail_'); + addTearDown(() => root.deleteSync(recursive: true)); + Directory(p.join(root.path, 'bin')).createSync(recursive: true); + File(p.join(root.path, 'bin', 'fsa')).writeAsStringSync('#!/bin/sh\n'); + + final runner = _RecordingRunner() + ..returnResult = + ProcessResult(0, 64, '', 'Unknown command: starter:install'); + final result = + await BootstrapCommandRunner(processRunner: runner.call).run( + bootstrapCommand: 'starter:install', + projectRoot: root.path, + ); + + expect(result.outcome, BootstrapRunOutcome.invoked); + expect(result.succeeded, isFalse); + expect(result.exitCode, 64); + expect(result.stderr, contains('Unknown command')); + }); + + test('reports succeeded when the subprocess exits zero', () async { + final root = Directory.systemTemp.createTempSync('bootstrap_ok_'); + addTearDown(() => root.deleteSync(recursive: true)); + Directory(p.join(root.path, 'bin')).createSync(recursive: true); + File(p.join(root.path, 'bin', 'fsa')).writeAsStringSync('#!/bin/sh\n'); + + final runner = _RecordingRunner() + ..returnResult = ProcessResult(0, 0, '', ''); + final result = + await BootstrapCommandRunner(processRunner: runner.call).run( + bootstrapCommand: 'starter:install', + projectRoot: root.path, + ); + + expect(result.succeeded, isTrue); + expect(result.exitCode, 0); + }); + test('always forwards --non-interactive to the chained command', () async { final root = Directory.systemTemp.createTempSync('bootstrap_noninter_'); addTearDown(() => root.deleteSync(recursive: true)); diff --git a/test/commands/plugin_install_command_test.dart b/test/commands/plugin_install_command_test.dart index 8e86465..f020f10 100644 --- a/test/commands/plugin_install_command_test.dart +++ b/test/commands/plugin_install_command_test.dart @@ -87,14 +87,15 @@ class _RecordingBootstrapRunner implements BootstrapCommandRunner { String? projectRoot; int callCount = 0; - BootstrapRunOutcome returnOutcome = BootstrapRunOutcome.invoked; + BootstrapRunResult returnResult = const BootstrapRunResult( + outcome: BootstrapRunOutcome.invoked, exitCode: 0); /// When set, [run] throws this instead of returning, simulating a runtime /// failure (e.g. `dart` missing on PATH) so the best-effort path is covered. Object? throwOnRun; @override - Future run({ + Future run({ required String bootstrapCommand, required String projectRoot, }) async { @@ -102,7 +103,7 @@ class _RecordingBootstrapRunner implements BootstrapCommandRunner { this.bootstrapCommand = bootstrapCommand; this.projectRoot = projectRoot; if (throwOnRun != null) throw throwOnRun!; - return returnOutcome; + return returnResult; } } @@ -1084,7 +1085,9 @@ void main() { writeManifestWithBootstrap(manifestPath, 'magic_starter'); final runner = _RecordingBootstrapRunner() - ..returnOutcome = BootstrapRunOutcome.notResolvable; + ..returnResult = const BootstrapRunResult( + outcome: BootstrapRunOutcome.notResolvable, + ); final cmd = _TestablePluginInstallCommand( fakeProjectRoot: root.path, fakeInstallYamlPath: manifestPath, @@ -1138,5 +1141,44 @@ void main() { expect(out, contains('Bootstrap with: artisan starter:install'), reason: 'falls back to the manual hint after a runner failure'); }); + + test('a non-zero bootstrap exit warns + hints instead of implying success', + () async { + final root = Directory.systemTemp.createTempSync('plinst_boot_nonzero_'); + addTearDown(() => root.deleteSync(recursive: true)); + _seedConsumerProject(root, pluginName: 'magic_starter'); + seedLibApp(root); + + final manifestPath = p.join(root.path, 'install.yaml'); + writeManifestWithBootstrap(manifestPath, 'magic_starter'); + + final runner = _RecordingBootstrapRunner() + ..returnResult = const BootstrapRunResult( + outcome: BootstrapRunOutcome.invoked, + exitCode: 64, + stderr: 'Unknown command: starter:install', + ); + final cmd = _TestablePluginInstallCommand( + fakeProjectRoot: root.path, + fakeInstallYamlPath: manifestPath, + fakeBootstrapRunner: runner, + ); + final ctx = _ctxWith( + opts(), + positional: const ['magic_starter'], + signature: cmd.parsedSignature, + ); + + final exit = await cmd.handle(ctx); + expect(exit, 0, + reason: 'a failed auto-run is best-effort, install still succeeds'); + final out = (ctx.output as BufferedOutput).content; + expect(out, contains('exited with code 64'), + reason: 'a non-zero bootstrap exit must be surfaced, not hidden'); + expect(out, contains('Unknown command: starter:install'), + reason: 'the captured stderr explains the failure'); + expect(out, contains('Bootstrap with: artisan starter:install'), + reason: 'falls back to the manual hint so the operator can recover'); + }); }); } From d8eb0cabf07e3e4a5fe7239b750a395338296ee0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?An=C4=B1lcan=20=C3=87ak=C4=B1r?= Date: Thu, 25 Jun 2026 15:45:38 +0300 Subject: [PATCH 2/2] refactor: make BootstrapRunResult final + assert invoked carries exit code Addresses review feedback: new public types follow the final class convention, and an assert invariant prevents constructing an invoked result without an exit code (which could surface as 'exited with code null'). --- lib/src/commands/helpers/bootstrap_command_runner.dart | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/src/commands/helpers/bootstrap_command_runner.dart b/lib/src/commands/helpers/bootstrap_command_runner.dart index a2ebf3c..fe0d048 100644 --- a/lib/src/commands/helpers/bootstrap_command_runner.dart +++ b/lib/src/commands/helpers/bootstrap_command_runner.dart @@ -45,12 +45,18 @@ enum BootstrapRunOutcome { /// whether the chained command completed; a non-zero exit (a stale bundle, an /// unknown command, a scaffold error) must NOT be reported to the operator as /// success, which is exactly what discarding the [ProcessResult] used to do. -class BootstrapRunResult { +final class BootstrapRunResult { + /// The [invoked] outcome MUST carry an [exitCode]: a subprocess that ran + /// always has one, and the invariant stops an incomplete result from + /// producing operator messages like "exited with code null". const BootstrapRunResult({ required this.outcome, this.exitCode, this.stderr, - }); + }) : assert( + outcome != BootstrapRunOutcome.invoked || exitCode != null, + 'an invoked BootstrapRunResult must carry a subprocess exit code', + ); final BootstrapRunOutcome outcome;