feat: Support embedding and installing shell completion scripts #68
Conversation
📝 WalkthroughWalkthroughReplaces the experimentalCompletionCommand flag with enableCompletionCommand, adds embeddedCompletions support, exposes completion APIs, and implements embed/install/generate completion subcommands plus examples and expanded README documentation; updates tests to exercise embedding and installation flows. No behavioral changes outside the completion subsystem. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer CLI
participant Runner as BetterCommandRunner
participant Completion as CompletionCommand
participant Embed as CompletionEmbedCommand
participant Install as CompletionInstallCommand
participant FS as FileSystem
Note over Dev,Runner: CLI constructed with enableCompletionCommand=true and embeddedCompletions
Dev->>Runner: new BetterCommandRunner(enableCompletionCommand: true, embeddedCompletions: [...])
Runner->>Completion: register CompletionCommand(embeddedCompletions)
Completion->>Completion: register Embed command
alt embeddedCompletions provided
Completion->>Install: register Install command (with embedded scripts map)
end
%% Embed flow
Dev->>Completion: run "completion embed --target=completely --script-file=..."
Completion->>Embed: dispatch embed
Embed->>FS: read script (file/stdin)
Embed->>FS: write Dart file or stdout (generated constant)
%% Install flow
Dev->>Completion: run "completion install --target=carapace --exec-name=mycmd"
Completion->>Install: dispatch install
Install->>FS: select embedded script by target
Install->>FS: write script to target-specific path
Install-->>Dev: return/print installed path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
christerswahn
force-pushed
the
completion
branch
3 times, most recently
from
8753e1c to
90c47b2
Compare
New subcommand 'completion embed' to embed a completion script in the command source, for later install by end users.
New subcommand 'completion install' to install an embedded completion script.
christerswahn
force-pushed
the
completion
branch
from
90c47b2 to
422b409
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (15)
packages/cli_tools/lib/src/better_command_runner/completion/completion_generate_command.dart (2)
56-71: Ensure file sink is always closed on errorIf generation throws after openWrite(), the sink isn’t closed. Wrap writes in try/finally and close only for file sinks; close() also flushes, so explicit flush isn’t needed.
- final IOSink out = file?.openWrite() ?? stdout; - - switch (target) { + final IOSink out = file?.openWrite() ?? stdout; + try { + switch (target) { case CompletionTarget.completely: - CompletelyYamlGenerator().generate(out, usage); - break; + CompletelyYamlGenerator().generate(out, usage); + break; case CompletionTarget.carapace: - CarapaceYamlGenerator().generate(out, usage); - break; + CarapaceYamlGenerator().generate(out, usage); + break; - } - - if (file != null) { - await out.flush(); - await out.close(); - } + } + } finally { + if (file != null) { + await out.close(); + } + }
72-77: Avoid shadowing ‘out’ with a different typeUsing out for both IOSink (outer scope) and String (inner scope) hurts readability.
- final out = file == null ? ' to stdout' : ': ${file.path}'; - stderr.writeln('Generated specification$out'); + final destination = file == null ? ' to stdout' : ': ${file.path}'; + stderr.writeln('Generated specification$destination');packages/cli_tools/lib/src/better_command_runner/completion/completion_target.dart (1)
1-7: Enum + record typedef are cleanSimple, clear surface. Consider brief doc comments for IDE help, but otherwise good.
packages/cli_tools/example/completion_script_carapace.dart (2)
4-4: Optionally import the typedef and annotate the constantAnnotating with CompletionScript helps catch shape mismatches at compile time.
-import 'package:cli_tools/better_command_runner.dart' show CompletionTarget; +import 'package:cli_tools/better_command_runner.dart' + show CompletionTarget, CompletionScript;
61-65: Annotate embedded script constant for clarityType the record as CompletionScript to align with the public API.
-const completionScriptCarapace = ( +const CompletionScript completionScriptCarapace = ( target: CompletionTarget.carapace, script: _completionScript, );packages/cli_tools/example/completion_script_completely.dart (1)
136-139: Optional: annotate the record with the typedef for stronger checking.If convenient, import CompletionScript and annotate the constant.
-import 'package:cli_tools/better_command_runner.dart' show CompletionTarget; +import 'package:cli_tools/better_command_runner.dart' show CompletionTarget; +// If you decide to annotate explicitly: +// import 'package:cli_tools/src/better_command_runner/completion/completion_target.dart' show CompletionScript; -const completionScriptCompletely = ( +// const CompletionScript completionScriptCompletely = ( target: CompletionTarget.completely, script: _completionScript, );packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart (1)
37-41: Guard against empty embeddedCompletions to avoid a dead ‘install’ command.Only add the install subcommand when the iterable is non-empty.
- if (embeddedCompletions != null) { - addSubcommand(CompletionInstallCommand( - embeddedCompletions: embeddedCompletions, - )); - } + if (embeddedCompletions?.isNotEmpty == true) { + addSubcommand(CompletionInstallCommand( + embeddedCompletions: embeddedCompletions!, + )); + }packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart (1)
152-154: Couple the new params and document them.
- Add an assert to catch cases where embeddedCompletions is provided but the feature is disabled.
- Add constructor doc bullets for enableCompletionCommand and embeddedCompletions.
BetterCommandRunner( @@ - final bool enableCompletionCommand = false, - final Iterable<CompletionScript>? embeddedCompletions, + final bool enableCompletionCommand = false, + final Iterable<CompletionScript>? embeddedCompletions, @@ - ) { + ) { + assert( + !(!enableCompletionCommand && embeddedCompletions != null), + 'embeddedCompletions provided but enableCompletionCommand is false', + ); @@ - if (enableCompletionCommand) { + if (enableCompletionCommand) { addCommand( CompletionCommand<T>(embeddedCompletions: embeddedCompletions), ); }Add to the constructor docs (above):
- [enableCompletionCommand] enables the “completion” command group.
- [embeddedCompletions] supplies pre-embedded scripts; when non-empty, the “install” subcommand is exposed.
Also applies to: 179-183
packages/cli_tools/test/better_command_runner/completion_test.dart (2)
311-422: Install path tests look good; add an unknown-target failure test.Add a test asserting a helpful error when passing an unsupported target to install.
200-308: Add negative test for install completion absence and update library expectation if removed
- Add a test verifying that the
installsubcommand is not listed whenembeddedCompletionsis null or empty.- If you remove the standalone
library;directive in the generator, update the stringContainsInOrder assertions to drop that line.packages/cli_tools/README_completion.md (3)
57-61: Clarify “UserConfigDir” and platform pathsRather than a pseudo variable, reference the actual defaults used by the install command (XDG_CONFIG_HOME on Linux, ~/Library/Application Support on macOS, %APPDATA% on Windows), and point readers to the new completion install subcommand so the tool writes to the correct location automatically.
218-224: Unify placeholder command nameExamples mix “my-command” and “example”. Pick one and use it consistently (e.g., my-command). Also fix the carapace example to match:
source <(carapace my-command).-source <(carapace example) # for bash; for others see https://carapace.sh/setup.html +source <(carapace my-command) # for bash; for others see https://carapace.sh/setup.html
170-195: Avoid author-specific absolute path in help textThe default output directory shows a machine-specific path:
(defaults to "Directory: '/Users/christer/.../lib/src'"). Replace with a generic description, e.g.(defaults to "lib/src/ under the package root").packages/cli_tools/lib/src/better_command_runner/completion/completion_embed_command.dart (2)
119-127: Triple-quote collision risk in embedded scriptEmbedding as
r'''...'''will break if the script contains'''. Low probability, but protect by splitting the literal or using base64. Example minimal change: split on the sentinel.-const String _completionScript = r''' -$scriptContent -'''; +const String _completionScript = + r'''$scriptContent'''; // If needed, replace occurrences of ''' in content with ''' + r"'" "' + '''Alternatively, store base64 and decode at runtime (drops “const”), or use triple double-quotes.
57-64: Default output dir may not exist
lib/srcwon’t exist for some packages/examples. Either create it or relax validation to create when missing.- return Directory(p.join(dir.path, 'lib', 'src')); + final out = Directory(p.join(dir.path, 'lib', 'src')); + if (!out.existsSync()) out.createSync(recursive: true); + return out;Also consider changing
outDiroption’s PathExistMode to create when absent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/cli_tools/README_completion.md(3 hunks)packages/cli_tools/example/command_completion_example.dart(1 hunks)packages/cli_tools/example/completion_script_carapace.dart(1 hunks)packages/cli_tools/example/completion_script_completely.dart(1 hunks)packages/cli_tools/lib/better_command_runner.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart(3 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart(3 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_embed_command.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_generate_command.dart(3 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_target.dart(1 hunks)packages/cli_tools/test/better_command_runner/completion_test.dart(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T07:55:17.269Z
Learnt from: christerswahn
PR: serverpod/cli_tools#57
File: packages/config/test/better_command_runner/default_flags_test.dart:1-1
Timestamp: 2025-08-07T07:55:17.269Z
Learning: In the `config` package, `better_command_runner.dart` is intentionally kept as a separate import (`package:config/better_command_runner.dart`) rather than being re-exported through the main `packages/config/lib/config.dart` barrel file. This separation is by design according to the package maintainer christerswahn.
Applied to files:
packages/cli_tools/lib/src/better_command_runner/completion/completion.dartpackages/cli_tools/lib/src/better_command_runner/completion/completion_command.dartpackages/cli_tools/lib/src/better_command_runner/better_command_runner.dartpackages/cli_tools/lib/better_command_runner.dartpackages/cli_tools/lib/src/better_command_runner/completion/completion_generate_command.dart
🔇 Additional comments (10)
packages/cli_tools/lib/src/better_command_runner/completion/completion.dart (1)
1-1: Barrel export looks goodRe-exporting completion_target.dart here keeps the public surface tidy. No issues.
packages/cli_tools/lib/src/better_command_runner/completion/completion_generate_command.dart (2)
46-49: Use of StateError is appropriateThrowing StateError when runner is missing fits the situation better than a generic Exception. LGTM.
78-79: No action needed:return null as Tis safe
CompletionGenerateCommandis only ever instantiated without a concrete type (soTdefaults todynamic), making the null cast safe and consistent with the BetterCommand pattern of signaling “no meaningful return.”Likely an incorrect or invalid review comment.
packages/cli_tools/lib/better_command_runner.dart (1)
3-3: Publicly exporting completion module is sensibleMakes CompletionTarget (and related typedefs) available via the primary entrypoint. Consistent with existing export pattern in this file. LGTM.
packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart (1)
18-21: Nice: clearer help for targets.Adding allowedHelp entries for ‘completely’ and ‘carapace’ improves UX.
packages/cli_tools/test/better_command_runner/completion_test.dart (1)
32-44: Good toggle test coverage for enabling the completion feature.Verifies presence/absence of the command cleanly.
packages/cli_tools/example/completion_script_completely.dart (1)
2-2: Invalidlibrary;directive.
library;without a name isn’t valid Dart and will fail analysis/compile. Remove it or give it a name (e.g.,library completion_script_completely;). Update the generator and tests accordingly.-library; +// (no library directive needed)⛔ Skipped due to learnings
Learnt from: christerswahn PR: serverpod/cli_tools#47 File: lib/src/config/options.dart:552-567 Timestamp: 2025-06-12T14:55:38.006Z Learning: For this project, avoid review comments that simply repeat compile-time errors the Dart analyzer already reports; focus on issues not caught by the analyzer.packages/cli_tools/README_completion.md (1)
13-15: Enable flag docs look goodClear, actionable, and matches the new API surface (enableCompletionCommand).
packages/cli_tools/lib/src/better_command_runner/completion/completion_embed_command.dart (1)
82-95: Make this command void-returning and drop null-castAlign with install command to avoid
null as T.-class CompletionEmbedCommand<T> - extends BetterCommand<CompletionEmbedOption, T> { - CompletionEmbedCommand() : super(options: CompletionEmbedOption.values); +class CompletionEmbedCommand + extends BetterCommand<CompletionEmbedOption, void> { + CompletionEmbedCommand() : super(options: CompletionEmbedOption.values); @@ - Future<T> runWithConfig( + Future<void> runWithConfig( @@ - return null as T; + return;Also applies to: 150-151
⛔ Skipped due to learnings
Learnt from: christerswahn PR: serverpod/cli_tools#67 File: packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart:81-81 Timestamp: 2025-09-22T13:21:24.881Z Learning: In the BetterCommand framework, using `return null as T` is a consistent pattern for commands that don't return meaningful values, rather than using void return types. The generic T parameter should be maintained even when the command doesn't produce a concrete return value.packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart (1)
28-31: Retain thereturn null as Tpattern; don’t convert tovoid. The BetterCommand framework intentionally usesFuture<T>withreturn null as Tfor commands that produce no meaningful value—switching tovoidwould break consistency with other commands (e.g. the embed and generate commands use the same pattern).Likely an incorrect or invalid review comment.
packages/cli_tools/lib/src/better_command_runner/completion/completion_embed_command.dart
Outdated
Show resolved
Hide resolved
packages/cli_tools/lib/src/better_command_runner/completion/completion_embed_command.dart
Show resolved
Hide resolved
packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart
Show resolved
Hide resolved
packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart
Show resolved
Hide resolved
packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart
Show resolved
Hide resolved
packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart
Show resolved
Hide resolved
packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart
Show resolved
Hide resolved
nielsenko
left a comment
nielsenko
left a comment
There was a problem hiding this comment.
I noticed that both embed and install are hidden subcommands. I can understand why embed is, but shouldn't install be visible, and should generate perhaps be hidden? Or even better have a separate cli for embed and generate.
As a user I think you should just run my_command completion install -t <shell or carapace> in the ideal case.
That said, lets not make perfect the enemy of good. This is a great addition.
packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart
Show resolved
Hide resolved
For the next version, will refactor the sub-commands into "business logic" and "commands", to allow for simpler command interface as well as enabling the developer to change the subcommand interface. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart (1)
88-93: Optional: avoid accidental overwrite or add--forceCurrently overwrites without prompt. Consider a
--forceflag or skip when file exists, emitting a hint to use--force.packages/cli_tools/lib/src/better_command_runner/completion/completion_embed_command.dart (4)
104-111: Use async file read for consistency and non-blocking I/OSmall polish: switch to
await readAsString()instead of sync I/O inside an async method.- } else { - final scriptFile = File(scriptFileName); - scriptContent = scriptFile.readAsStringSync(); - } + } else { + final scriptFile = File(scriptFileName); + scriptContent = await scriptFile.readAsString(); + }
56-62: Default output dir may not exist; consider falling back whenlib/srcis missingIf a package root is found but
lib/srcdoesn’t exist,DirOption(PathExistMode.mustExist)will fail. Either create it or fall back toDirectory.current.Directory _defaultWriteDir() { final dir = _findPackageRoot(); if (dir == null) { return Directory.current; } - return Directory(p.join(dir.path, 'lib', 'src')); + final candidate = Directory(p.join(dir.path, 'lib', 'src')); + return candidate.existsSync() ? candidate : Directory.current; }
141-147: Avoid shadowingout(IOSink) with a local string variableMinor readability nit; rename the message variable.
- final out = - embeddedFile == null ? ' to stdout' : ': ${embeddedFile.path}'; - stderr.writeln('Wrote embedded script$out'); + final destMsg = + embeddedFile == null ? ' to stdout' : ': ${embeddedFile.path}'; + stderr.writeln('Wrote embedded script$destMsg');
122-126: Optional: add an explicit record type for the exported constantTyping the constant improves IDE hints without importing internal typedefs.
-const completionScript${target.name.capitalize()} = ( +const ({CompletionTarget target, String script}) completionScript${target.name.capitalize()} = ( target: $target, script: _completionScript, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/cli_tools/example/command_completion_example.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_embed_command.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli_tools/example/command_completion_example.dart
🔇 Additional comments (4)
packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart (4)
59-63: Good fix: error message now goes to stderr and preserves non‑zero exitThis resolves the earlier stdout pollution concern.
88-90: Good fix: ensure target directory exists before writingCreating the parent directory avoids openWrite() failures on default locations.
108-118: Good fix: resolve%APPDATA%via environment with sane fallbacksWindows branch now returns a real path and handles missing
APPDATA.
71-86: Confirm Windows behavior forCompletionTarget.completelyDefault path for
completelyuses_getHomeDir()which throws ifHOMEis unset. If users attemptinstall --target completelyon Windows (Git Bash/MSYS), this will crash. Either document “completely is non‑Windows only,” guard it, or add a Windows fallback.If you want to guard quickly:
@@ - final writeDirPath = writeDir?.path ?? + final writeDirPath = writeDir?.path ?? switch (target) { - CompletionTarget.completely => p.join( + CompletionTarget.completely => Platform.isWindows + ? throw ExitException.error() + : p.join( _getHomeDir(), '.local', 'share', 'bash-completion', 'completions', ),
| final outputContent = """ | ||
| /// This file is auto-generated. | ||
| library; | ||
| import 'package:cli_tools/better_command_runner.dart' show CompletionTarget; | ||
| const String _completionScript = r''' | ||
| $scriptContent | ||
| '''; | ||
| /// Embedded script for command line completion for `${target.name}`. | ||
| const completionScript${target.name.capitalize()} = ( | ||
| target: $target, | ||
| script: _completionScript, | ||
| ); | ||
| """; |
There was a problem hiding this comment.
Make embedding robust when the script contains triple quotes
If the completion script includes ''', the generated Dart won’t compile. Prefer choosing a delimiter that doesn’t occur, with a fallback.
One pragmatic approach: pick between r''' and r""" based on content and build the output with a StringBuffer to avoid escaping inside a """...""" template:
- final outputContent = """
-/// This file is auto-generated.
-...
-const String _completionScript = r'''
-$scriptContent
-''';
-...
-""";
+ final usesSingle = scriptContent.contains("'''");
+ final usesDouble = scriptContent.contains('"""');
+ final delim = usesSingle && !usesDouble ? '"""' : "'''";
+
+ final b = StringBuffer()
+ ..writeln('/// This file is auto-generated.')
+ ..writeln()
+ ..writeln('library;')
+ ..writeln()
+ ..writeln("import 'package:cli_tools/better_command_runner.dart' show CompletionTarget;")
+ ..writeln()
+ ..writeln('const String _completionScript = r$delim')
+ ..writeln(scriptContent)
+ ..writeln('$delim;')
+ ..writeln()
+ ..writeln('/// Embedded script for command line completion for `${target.name}`.')
+ ..writeln('const completionScript${target.name.capitalize()} = (')
+ ..writeln(' target: $target,')
+ ..writeln(' script: _completionScript,')
+ ..writeln(');');
+ final outputContent = b.toString();If both ''' and """ appear (rare), consider a future fallback (e.g., base64) and drop const for _completionScript. Happy to draft that if you want.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final outputContent = """ | |
| /// This file is auto-generated. | |
| library; | |
| import 'package:cli_tools/better_command_runner.dart' show CompletionTarget; | |
| const String _completionScript = r''' | |
| $scriptContent | |
| '''; | |
| /// Embedded script for command line completion for `${target.name}`. | |
| const completionScript${target.name.capitalize()} = ( | |
| target: $target, | |
| script: _completionScript, | |
| ); | |
| """; | |
| final usesSingle = scriptContent.contains("'''"); | |
| final usesDouble = scriptContent.contains('"""'); | |
| final delim = usesSingle && !usesDouble ? '"""' : "'''"; | |
| final b = StringBuffer() | |
| ..writeln('/// This file is auto-generated.') | |
| ..writeln() | |
| ..writeln('library;') | |
| ..writeln() | |
| ..writeln("import 'package:cli_tools/better_command_runner.dart' show CompletionTarget;") | |
| ..writeln() | |
| ..writeln('const String _completionScript = r$delim') | |
| ..writeln(scriptContent) | |
| ..writeln('$delim;') | |
| ..writeln() | |
| ..writeln('/// Embedded script for command line completion for `${target.name}`.') | |
| ..writeln('const completionScript${target.name.capitalize()} = (') | |
| ..writeln(' target: $target,') | |
| ..writeln(' script: _completionScript,') | |
| ..writeln(');'); | |
| final outputContent = b.toString(); |
🤖 Prompt for AI Agents
In
packages/cli_tools/lib/src/better_command_runner/completion/completion_embed_command.dart
around lines 112-127, the generated Dart uses a fixed raw triple-single-quote
delimiter which will break if the embedded script contains '''. Change
generation to pick a safe delimiter: check the scriptContent for
triple-single-quotes and triple-double-quotes and use r''' if script does not
contain ''' otherwise use r""" if script does not contain """. If the script
contains both triple-single and triple-double sequences, fall back to encoding
the script (e.g., base64) and emit a non-const _completionScript that decodes at
runtime (or otherwise use a decoded StringBuffer build), so the file always
compiles. Ensure the generated constant name and surrounding code reflect the
chosen representation (raw string when possible, non-const decoded string when
using fallback).
| final executableName = execName ?? betterRunner.executableName; | ||
|
|
||
| final writeFileName = switch (target) { | ||
| CompletionTarget.completely => '$executableName.bash', | ||
| CompletionTarget.carapace => '$executableName.yaml', | ||
| }; |
There was a problem hiding this comment.
Validate --exec-name to prevent path traversal and invalid filenames
A user‑supplied exec name can contain separators or .., enabling writes outside the intended dir when joined. Fail fast instead of silently sanitizing to match your stated preference.
Apply this minimal validation:
@@
- final executableName = execName ?? betterRunner.executableName;
+ final executableName = execName ?? betterRunner.executableName;
+ _assertSafeFileName(executableName);
@@
}
+ static void _assertSafeFileName(final String name) {
+ if (name.isEmpty) {
+ throw const FormatException('Executable name cannot be empty');
+ }
+ // Disallow path separators and traversal segments.
+ if (name.contains('/') || name.contains('\\') || name.contains('..')) {
+ throw FormatException(
+ 'Invalid --exec-name "$name": must not contain path separators or ".."',
+ );
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final executableName = execName ?? betterRunner.executableName; | |
| final writeFileName = switch (target) { | |
| CompletionTarget.completely => '$executableName.bash', | |
| CompletionTarget.carapace => '$executableName.yaml', | |
| }; | |
| // Compute the executable name, then validate it before using. | |
| final executableName = execName ?? betterRunner.executableName; | |
| _assertSafeFileName(executableName); | |
| final writeFileName = switch (target) { | |
| CompletionTarget.completely => '$executableName.bash', | |
| CompletionTarget.carapace => '$executableName.yaml', | |
| }; | |
| // ... | |
| // Add this static helper within the same class to enforce filename safety. | |
| static void _assertSafeFileName(final String name) { | |
| if (name.isEmpty) { | |
| throw const FormatException('Executable name cannot be empty'); | |
| } | |
| // Disallow path separators and traversal segments. | |
| if (name.contains('/') || name.contains('\\') || name.contains('..')) { | |
| throw FormatException( | |
| 'Invalid --exec-name "$name": must not contain path separators or ".."', | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart
around lines 65 to 70, the user-supplied execName (execName) is used directly to
form filenames which can include path separators or traversal segments; add
validation to reject any execName containing path separators (such as '/' or '\'
depending on platform) or the substring '..' and also reject empty or
whitespace-only names. If validation fails, throw/return a clear error (e.g.,
usage exception) indicating an invalid exec name so the command fails fast
rather than producing a potentially dangerous path; only after validation,
compute executableName and writeFileName as before.
To make it easy for command developers to distribute the shell completion scripts to their end users, this adds two sub-commands:
completion embed: Embeds a completion script in a dart source file for inclusion in the command itself.completion install: Installs an embedded completion script in the user's environment.Example
This PR also includes a complete example in the
examplefolder of using the completion feature with embedding and installing.Summary by CodeRabbit