feat(commands): defineCommand — typed declarative commands via an ICommand adapter - #6101
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds a typed declarative command API. It validates and brands definitions, converts them to legacy ChangesDeclarative commands
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CommandsService
participant CommandRegistry
participant CommandDefinitionAdapter
participant OptionsService
participant CommandContext
CommandsService->>CommandRegistry: dispatch registered command
CommandRegistry->>CommandDefinitionAdapter: create command from definition
CommandDefinitionAdapter->>OptionsService: resolve option values
CommandDefinitionAdapter->>CommandContext: construct execution context
CommandDefinitionAdapter->>CommandContext: run canExecute and run
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
5 tasks
edusperoni
force-pushed
the
feat/define-command
branch
4 times, most recently
from
July 30, 2026 02:51
cafa737 to
a1ba0ef
Compare
edusperoni
force-pushed
the
feat/define-command
branch
from
August 4, 2026 20:41
a1ba0ef to
07c979c
Compare
…adapter Commands can now be declared as plain objects: a name, an option schema built from booleanOption/stringOption/numberOption/arrayOption, and a run function whose context carries the positional args plus the declared options, typed by inference from the schema. lib/common/define-command holds the types and the pure factories only, so it stays side-effect-free and can be re-exported from nativescript/contracts. The runtime bridge lives in lib/common/services/command-definition-adapter, which compiles a definition into the ICommand the legacy registry expects and runs it inside an injection context. canExecute is emitted only when the definition supplies one or opts into arguments: "any"; CommandsService skips all parameter validation as soon as canExecute exists, so omitting it is what lets the framework reject stray positional arguments for arguments: "none". Fully additive — existing ICommand classes are untouched.
…nitions A definition with no declared options must be executable in a container that has no options service registered - manifest-loaded extension commands run in exactly that situation.
The parent-dispatcher leak onto the module-level injector is fixed in the base branch, so the round-trip test no longer needs the global facade.
Yok extends Injector on the base branch; the di bridge is gone.
…rgument policy Reworks the declarative command API after the design review: - the new public types drop the `I` prefix, and `defineCommand` returns a `DefinedCommand` branded with the marker `isCommandDefinition` narrows to. `registerCommandDefinition` requires that brand, so nothing reaches the registry without having been validated. - an option is `T` only when its spec declares a `default`; without one it is `T | undefined`, which is what the command line actually produces. Asserted by test/type-fixtures, compiled under strict mode because this build has strictNullChecks off. - `defineCommand` validates the definition and throws naming the command and the accepted form, instead of failing deep and unattributed later. - `arguments` is enforced before the definition's `canExecute` runs, so the two compose: a command that leaves `arguments` at "none" rejects stray positional arguments whether or not it refines further. - `canExecute` runs in an injection context, like `run`. - registration goes through the `CommandRegistry` facet the target injector provides rather than the injector itself. - a schema entry shadowing a CLI-wide option warns naming the collision.
Unknown options warn and only fail under NS_STRICT_OPTIONS=error; `description` reaches the parser but nothing renders it; `canExecute` gets a context of the same shape as run's, not the same one. Replaces the "canExecute owns validation" rule with how the two fields compose, renames the flagship example's option off the CLI-wide `verbose`, and documents option value types, array aliases, the parent-name collision and `satisfies` for shared schemas.
`ctx.fail(message)` is the failure verb on the command context, in both `run` and `canExecute`. It maps to the errors service's `failWithHelp`, so a command failure carries the usage suggestion, and returns `never` so it can end a branch without a return. The message is validated like the define-time errors are, naming the command. Throwing keeps working unchanged — fail() is sugar over it, not a replacement. Commands get no `skip()`: warn-and-continue has no meaning inside run(). The CLI-wide option collision warning now covers aliases on both sides, so an `alias: "p"` that shadows `--path`'s shorthand is reported the same way a `verbose` option shadowing `--verbose` is, naming both sides.
edusperoni
force-pushed
the
feat/define-command
branch
from
August 5, 2026 19:22
07c979c to
d712a4e
Compare
edusperoni
marked this pull request as ready for review
August 5, 2026 20:22
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/common/services/command-definition-adapter.ts (1)
74-97: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
cliSpellingsinheritsObject.prototypekeys, so some option names report a false collision.
cliSpellingsis a plain object literal. An option or alias namedconstructor,toString, orvalueOfresolves through the prototype chain, so line 84 or line 91 is truthy without any real collision. The warning then interpolates a function into the message. The same hardening applies to theoptionsobject built on line 163.Use a prototype-less object for both.
♻️ Proposed change
- const cliSpellings: IDictionary<string> = {}; + const cliSpellings: IDictionary<string> = Object.create(null);const buildContext = (args: string[]): CommandContext<TSchema> => { - const options: any = {}; + const options: any = Object.create(null);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/common/services/command-definition-adapter.ts` around lines 74 - 97, Use prototype-less objects for both the cliSpellings map in the collision checks and the options object built later in the command-definition adapter. Preserve their existing key assignments and lookups while preventing inherited Object.prototype names from being treated as real options or aliases.lib/common/define-command.ts (1)
332-340: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidation accepts a definition whose
runlives on a prototype; the returned copy drops it.
isPlainObjectaccepts a class instance, and line 277 findsrunthrough the prototype chain. The spread on line 337 copies own enumerable properties only, so the returnedDefinedCommandhas norun. The failure then surfaces later, when the command executes, instead of at define time.Reject a definition whose
runis not an own property, or copy the resolved handlers explicitly.♻️ Proposed check
if (typeof definition.run !== "function") { invalid(definition, "'run' must be a function"); } + + if (!Object.prototype.hasOwnProperty.call(definition, "run")) { + invalid( + definition, + "'run' must be declared on the definition object itself, not inherited from a prototype", + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/common/define-command.ts` around lines 332 - 340, Update defineCommand and its validation flow so definitions with a prototype-inherited run handler are rejected before the spread copy is returned; require run to be an own property while preserving valid own-handler definitions and existing validation behavior.defining-commands.md (1)
60-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced block.
markdownlint reports MD040 here. The block holds an error message, so mark it as
text.📝 Proposed change
-``` +```text Invalid command definition for 'widget|add': unknown field(s) 'handler'; a🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@defining-commands.md` around lines 60 - 66, Update the fenced code block in defining-commands.md to specify the text language, changing the opening fence to ```text while preserving the error message content.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/common/services/command-definition-adapter.ts`:
- Around line 173-181: Update createCommandFromDefinition to pass
definition.description through in the returned command object when it is
defined, alongside disableAnalytics and enableHooks, so the adapter preserves
the validated command metadata.
---
Nitpick comments:
In `@defining-commands.md`:
- Around line 60-66: Update the fenced code block in defining-commands.md to
specify the text language, changing the opening fence to ```text while
preserving the error message content.
In `@lib/common/define-command.ts`:
- Around line 332-340: Update defineCommand and its validation flow so
definitions with a prototype-inherited run handler are rejected before the
spread copy is returned; require run to be an own property while preserving
valid own-handler definitions and existing validation behavior.
In `@lib/common/services/command-definition-adapter.ts`:
- Around line 74-97: Use prototype-less objects for both the cliSpellings map in
the collision checks and the options object built later in the
command-definition adapter. Preserve their existing key assignments and lookups
while preventing inherited Object.prototype names from being treated as real
options or aliases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61c4e27b-40eb-4bf0-a237-8136d760e17f
📒 Files selected for processing (8)
defining-commands.mdlib/common/define-command.tslib/common/services/command-definition-adapter.tslib/contracts/index.tstest/define-command.tstest/type-fixtures/define-command-types.tstest/type-fixtures/tsconfig.jsontsconfig.json
Comment on lines
+173
to
+181
| return { | ||
| allowedParameters: [], | ||
| dashedOptions, | ||
| ...(definition.disableAnalytics === undefined | ||
| ? {} | ||
| : { disableAnalytics: definition.disableAnalytics }), | ||
| ...(definition.enableHooks === undefined | ||
| ? {} | ||
| : { enableHooks: definition.enableHooks }), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Determine whether a per-command description can be carried on the command object.
set -uo pipefail
# ICommand / ICommandOptions surface
fd -t f 'commands.d.ts' lib | xargs -r rg -n -C4 'interface ICommand\b|interface ICommandOptions'
# How help resolves per-command text
rg -n -C4 --type=ts 'commandHelp|getCommandHelp|helpCommand|description' lib/common/services/help-service.ts 2>/dev/null
# Any existing consumer of a description on a command object
rg -nP --type=ts -C3 '\bcommand\.description\b|\bdescription\b.*ICommand'Repository: NativeScript/nativescript-cli
Length of output: 412
🏁 Script executed:
#!/bin/bash
set -uo pipefail
printf 'Repo files matching command-definition-adapter.ts / define-command.ts:\n'
fd -t f 'command-definition-adapter.ts|define-command.ts|defining-commands.md' .
printf '\nCommandDefinitionAdapter adapter relevant section:\n'
f="$(fd -t f 'command-definition-adapter.ts' . | head -n1)"
[ -n "$f" ] && sed -n '130,210p' "$f" | nl -ba -v130
printf '\ndefine-command validation around description:\n'
f="$(fd -t f 'define-command.ts' . | head -n1)"
[ -n "$f" ] && sed -n '280,330p' "$f" | nl -ba -v280
printf '\nhelp service description consumers:\�'
fd -t f 'help-service.ts' lib | xargs -r sed -n '1,260p' | nl -ba | rg -n -C5 'description|commandHelp|getCommandHelp|helpCommand|ICommand'
printf '\nFind descriptions in declarations and commands.d.ts:\n'
fd -t f 'commands.d.ts' lib | xargs -r rg -n -C3 'interface ICommandOptions\b|interface ICommand\b|description'Repository: NativeScript/nativescript-cli
Length of output: 926
🏁 Script executed:
#!/bin/bash
set -uo pipefail
f_adapter="$(fd -t f 'command-definition-adapter.ts' . | head -n1)"
f_define="$(fd -t f 'define-command.ts' . | head -n1)"
f_help="$(fd -t f 'help-service.ts' lib | head -n1)"
f_decl="$(fd -t f 'commands.d.ts' lib | head -n1)"
printf 'adapter %s validation and transform:\n' "$f_adapter"
sed -n '130,210p' "$f_adapter"
printf '\ndefine validation %s:\n' "$f_define"
sed -n '260,335p' "$f_define"
printf '\nhelp service relevant description/help consumers %s:\n' "$f_help"
sed -n '1,240p' "$f_help" | grep -n -C 5 'description\|commandHelp\|getCommandHelp\|helpCommand\|ICommand' || true
printf '\ndeclarations %s:\n' "$f_decl"
sed -n '1,80p' "$f_decl"
printf '\nAny TypeScript/d declaration consumers for command.description:\n'
rg -n -C 3 --iglob '*.{ts,d.ts}' '\\bcommand\.description\b|ICommandOptions' .Repository: NativeScript/nativescript-cli
Length of output: 8973
🏁 Script executed:
#!/bin/bash
set -uo pipefail
printf 'DefineCommand and CommandDefinition type definitions:\n'
rg -n -C8 -P 'type DefinedCommand|interface (CommandDefinition|ICommandOptions)\b|description' lib/common/define-command.ts lib/common/declarations.d.ts
printf '\nHelpService full relevant methods around reading command files:\n'
f="$(fd -t f 'help-service.ts' lib | head -n1)"
sed -n '80,220p' "$f"
printf '\nCommandData interface and construction references:\n'
rg -n -C4 -P 'interface ICommandData\b|type ICommandData\b|showCommandLineHelp|openHelpForCommandInBrowser|ICommandData' lib test | head -n 240
printf '\ncommand-definition-adapter usages and validateDefinition references:\n'
rg -n -C3 'registerCommandDefinition|defineCommand|validateDefinition|definition\.description|DEFINITION_FIELDS' lib testRepository: NativeScript/nativescript-cli
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -uo pipefail
rg -n --iglob '*.{md,d.ts,ts}' -C3 '\binterface ICommandData\b|\btype ICommandData\b|description' test/define-command.ts test || true
printf '\nCheck help markup references in generated command descriptions:\n'
rg -n --iglob '*.{md,d.ts,ts}' -C2 'Generated from command\\.|commandDescription|module\.commandDescription|#{module\.command' .
printf '\nCheck exact help service methods that read command docs:\n'
f="$(fd -t f 'help-service.ts' lib | head -n1)"
sed -n '220,340p' "$f"
printf '\nRead command-definition-adapter mapping section using Python line numbers:\n'
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('command-definition-adapter.ts'):
print(f'\n{p}')
text = p.read_text()
lines = text.splitlines()
for i in range(max(0, len(lines)-1), -1, -1):
if 'function createCommandFromDefinition' in lines[i]:
start = max(0, i-10); end = min(len(lines), i+90)
break
else:
continue
for n, line in enumerate(lines[start:end], start=start+1):
print(f'{n:4d}\t{line}')
PYRepository: NativeScript/nativescript-cli
Length of output: 19670
Pass definition.description through the command adapter.
CommandDefinition validates and documents description, but createCommandFromDefinition only copies disableAnalytics and enableHooks, so the registered command object drops it. Map description: definition.description when present, or document the field as non-passed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/common/services/command-definition-adapter.ts` around lines 173 - 181,
Update createCommandFromDefinition to pass definition.description through in the
returned command object when it is defined, alongside disableAnalytics and
enableHooks, so the adapter preserves the validated command metadata.
5 tasks
Contributor
|
@copilot resolve the merge conflicts in this pull request |
# Conflicts: # lib/contracts/index.ts Co-authored-by: NathanWalker <457187+NathanWalker@users.noreply.github.com>
Contributor
Resolved by merging |
NathanWalker
approved these changes
Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Checklist
What is the current behavior?
A command is a class implementing
ICommand, registered under a stringly-typed key, reading flags off the untyped global$optionsobject. The validation semantics carry a trap: declaringcanExecutesilently disablesallowedParametersvalidation, and an emptyallowedParametersmeans "reject all positional arguments" — none of which the types express.What is the new behavior?
defineCommand— a declarative, typed command definition that plugs into the existing registry through an adapter (createCommandFromDefinition/registerCommandDefinition, routed through theCommandRegistryfacet with a realuseFactoryprovider). Fully additive: routing, help, hooks, and analytics behavior are untouched, and legacyICommandclasses remain fully supported.defaultisT; without one it isT | undefined— pinned by a strict-mode compile fixture (test/type-fixtures/), since the repo's own build hasstrictNullChecksoff and any in-suite assertion would be vacuous. A no-options command'sctx.optionsrejects typos.canExecutetrap is gone: the adapter always enforces the declaredargumentspolicy first, then calls a usercanExecuteas pure refinement — the fields compose instead of interacting.run, invalid aliases all throw atdefineCommand()with messages naming the command. Options colliding with CLI-wide option names or aliases get a define-time warning naming both sides.ctx.fail(message)fails the command throughfailWithHelp; plainthrowremains equivalent. BothrunandcanExecuteexecute in an injection context, soinject()works inside commands the same as everywhere else.dashedOptions, riding the CLI's revived option validation (unknown flags warn by default today and hard-fail underNS_STRICT_OPTIONS=error).lib/common/define-command.tsis side-effect-free and exported fromnativescript/contracts; definitions carry a plain-assigned, spread-safeSymbol.formarker;isCommandDefinitionis a type predicate and the return type is branded —registerCommandDefinitionrequires it at compile time and verifies it at runtime.Iprefix):CommandDefinition,CommandContext,CommandOptionSpec. Legacy publishedI*types untouched.test/define-command.ts, including end-to-end dispatch through the parent name (tryExecuteCommand("widget", ["add"])and a*defaultcase) and throughCommandsService.tryExecuteCommand.defining-commands.md.The registry gaps this work surfaced were fixed on
mainrather than in this PR:registerCommandnow populates hierarchical routing state, a registered command is no longer silently clobbered when a subcommand later shadows it (which also revealed and fixed the CLI's own deadwidgetflat registration), and options declared with an array of aliases resolve correctly under the revived validator.Full suite: 115 files, 1759 passed / 9 skipped (main baseline 1713 + 46 branch tests); yok oracle, public-API test, and compat fixtures untouched.
Summary by CodeRabbit
New Features
Documentation
Tests