Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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=<n>` 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: <flag>` 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.

Expand Down
56 changes: 46 additions & 10 deletions lib/src/commands/helpers/bootstrap_command_runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Comment thread
anilcancakir marked this conversation as resolved.
Outdated

/// Runs a plugin's declared `bootstrap_command` as a fresh dispatcher
/// subprocess after `plugin:install` registers the plugin.
///
Expand Down Expand Up @@ -84,28 +111,37 @@ 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<BootstrapRunOutcome> 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<BootstrapRunResult> run({
required String bootstrapCommand,
required String projectRoot,
}) async {
// 1. Build the dispatcher invocation: bin/fsa fast-CLI when present,
// else `dart run <consumer>: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
Expand Down
19 changes: 17 additions & 2 deletions lib/src/commands/plugin_install_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
45 changes: 42 additions & 3 deletions test/commands/helpers/bootstrap_command_runner_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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));
Expand Down
50 changes: 46 additions & 4 deletions test/commands/plugin_install_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -87,22 +87,23 @@ 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<BootstrapRunOutcome> run({
Future<BootstrapRunResult> run({
required String bootstrapCommand,
required String projectRoot,
}) async {
callCount++;
this.bootstrapCommand = bootstrapCommand;
this.projectRoot = projectRoot;
if (throwOnRun != null) throw throwOnRun!;
return returnOutcome;
return returnResult;
}
}

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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');
});
});
}