Skip to content

Fix passThroughOptions validation for commands nested more than one level deep - #2607

Closed
dualfroz wants to merge 29 commits into
tj:masterfrom
dualfroz:dualfroz/fix-nested-passthrough-options
Closed

Fix passThroughOptions validation for commands nested more than one level deep#2607
dualfroz wants to merge 29 commits into
tj:masterfrom
dualfroz:dualfroz/fix-nested-passthrough-options

Conversation

@dualfroz

@dualfroz dualfroz commented Sep 5, 2026

Copy link
Copy Markdown

Problem

Command.passThroughOptions() is documented (and enforced by _checkForBrokenPassThrough())
to require that enablePositionalOptions() has been turned on for the "parent command(s)"
(plural, per the existing JSDoc and error message), because any ancestor command that has
not enabled positional options will keep scanning the entire remaining argv for its own
registered flags, regardless of position. If any ancestor beyond the immediate parent still
does this, an option meant to be passed through untouched to a grandchild command can be
silently intercepted by a same-named option registered higher up the tree.

The runtime check, however, only ever validated the immediate parent:

_checkForBrokenPassThrough() {
  if (
    this.parent &&
    this._passThroughOptions &&
    !this.parent._enablePositionalOptions
  ) {
    throw new Error(`passThroughOptions cannot be used for '${this._name}' ...`);
  }
}

For a command nested two or more levels below the program (a documented and tested pattern,
see tests/command.nested.test.js), this means .passThroughOptions() can be turned on
without error even though a grandparent (or higher ancestor) still lacks positional options,
and the guarantee is silently broken at parse time.

Reproduction on unpatched develop (lib/command.js, _checkForBrokenPassThrough, line 876):

import { Command } from './index.js';

const program = new Command();
program.option('--dry-run');           // root option, not positional

const mid = program.command('mid').enablePositionalOptions();
const leaf = mid.command('leaf');
leaf.argument('<utility>').argument('[args...]');
leaf.passThroughOptions();             // no error, even though root lacks positional options

leaf.action((utility, args) => console.log(utility, args));
program.parse(['mid', 'leaf', 'git', 'push', '--dry-run'], { from: 'user' });
console.log(program.opts());

Output on unpatched code:

leaf utility: git args: [ 'push' ]
root opts: { dryRun: true }

--dry-run is swallowed by the root command's own option instead of being passed through to
leaf, and no error was raised at .passThroughOptions() time to warn the author that their
setup is incomplete.

Root cause

lib/command.js, _checkForBrokenPassThrough() (around line 876), only inspects
this.parent, not the full ancestor chain up to the program root.

Fix

Walk the whole ancestor chain (same style already used elsewhere in the file, e.g.
Help.commandUsage() / Help.visibleGlobalOptions()), and throw the existing error message
if any ancestor is missing enablePositionalOptions():

_checkForBrokenPassThrough() {
  if (!this._passThroughOptions) return;
  for (
    let ancestorCmd = this.parent;
    ancestorCmd;
    ancestorCmd = ancestorCmd.parent
  ) {
    if (!ancestorCmd._enablePositionalOptions) {
      throw new Error(
        `passThroughOptions cannot be used for '${this._name}' without turning on enablePositionalOptions for parent command(s)`,
      );
    }
  }
}

The error message is unchanged; only the validation scope is widened from "immediate parent"
to "every ancestor", matching what the message and the existing JSDoc on passThroughOptions()
already promise. Behaviour for the previously supported one-level-deep case (program -> sub) is
unchanged, since walking the chain for a single ancestor is equivalent to the old check.

dependabot Bot and others added 29 commits June 13, 2026 16:30
Bumps [typescript-eslint](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/typescript-eslint) from 8.59.4 to 8.60.0.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/typescript-eslint/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v8.60.0/packages/typescript-eslint)

---
updated-dependencies:
- dependency-name: typescript-eslint
  dependency-version: 8.60.0
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [eslint](https://github.com/eslint/eslint) from 10.4.0 to 10.4.1.
- [Release notes](https://github.com/eslint/eslint/releases)
- [Commits](eslint/eslint@v10.4.0...v10.4.1)

---
updated-dependencies:
- dependency-name: eslint
  dependency-version: 10.4.1
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 4.36.0 to 4.36.2.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@7211b7c...8aad20d)

---
updated-dependencies:
- dependency-name: github/codeql-action
  dependency-version: 4.36.2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/checkout](https://github.com/actions/checkout) from 6.0.2 to 6.0.3.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@de0fac2...df4cb1c)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-version: 6.0.3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
The @example for commandsGroup() had an unterminated string literal
(missing closing quote), and the @example for helpOption() was missing
the comma between the flags and description arguments. Both snippets
throw a SyntaxError if copied and run as-is.

Co-authored-by: Patrick Wehbe <patrick.wehbe.applications@gmail.com>
Bumps [actions/checkout](https://github.com/actions/checkout) from 6.0.3 to 7.0.0.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@df4cb1c...9c091bb)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-version: 7.0.0
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
negativeNumberArg matched only a lowercase e exponent, so a negative
number in scientific notation with an uppercase E (e.g. -1E3) was not
recognised and was rejected as an unknown option, even though JavaScript
parses -1E3 and -1e3 identically. Add the i flag to the pattern.
Bumps [actions/setup-node](https://github.com/actions/setup-node) from 6.4.0 to 7.0.0.
- [Release notes](https://github.com/actions/setup-node/releases)
- [Commits](actions/setup-node@48b55a0...8207627)

---
updated-dependencies:
- dependency-name: actions/setup-node
  dependency-version: 7.0.0
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github/codeql-action/init](https://github.com/github/codeql-action) from 4.37.0 to 4.37.1.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@99df26d...7188fc3)

---
updated-dependencies:
- dependency-name: github/codeql-action/init
  dependency-version: 4.37.1
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Bump github/codeql-action/analyze from 4.37.0 to 4.37.1

Bumps [github/codeql-action/analyze](https://github.com/github/codeql-action) from 4.37.0 to 4.37.1.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@99df26d...7188fc3)

---
updated-dependencies:
- dependency-name: github/codeql-action/analyze
  dependency-version: 4.37.1
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Fix CodeQL action version mismatch: update init to v4.37.1

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Bumps [eslint](https://github.com/eslint/eslint) from 10.7.0 to 10.8.0.
- [Release notes](https://github.com/eslint/eslint/releases)
- [Commits](eslint/eslint@v10.7.0...v10.8.0)

---
updated-dependencies:
- dependency-name: eslint
  dependency-version: 10.8.0
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [globals](https://github.com/sindresorhus/globals) from 17.7.0 to 17.8.0.
- [Release notes](https://github.com/sindresorhus/globals/releases)
- [Commits](sindresorhus/globals@v17.7.0...v17.8.0)

---
updated-dependencies:
- dependency-name: globals
  dependency-version: 17.8.0
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [prettier](https://github.com/prettier/prettier) from 3.9.4 to 3.9.6.
- [Release notes](https://github.com/prettier/prettier/releases)
- [Changelog](https://github.com/prettier/prettier/blob/main/CHANGELOG.md)
- [Commits](prettier/prettier@3.9.4...3.9.6)

---
updated-dependencies:
- dependency-name: prettier
  dependency-version: 3.9.6
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
The Chinese README documented the inspector as node -inspect, which Node rejects as a bad option. The English README already uses node --inspect.

Co-authored-by: MoonsvnLyn <287222957+FirmaSpring@users.noreply.github.com>
nodejs.org/en/docs/guides/debugging-getting-started/ now 404s. Use the current learn/getting-started/debugging page in both READMEs.

Co-authored-by: MoonsvnLyn <287222957+FirmaSpring@users.noreply.github.com>
_checkForBrokenPassThrough() only checked the immediate parent for
enablePositionalOptions(), so a command nested more than one level
deep (program -> mid -> leaf) could enable passThroughOptions()
without error even though a grandparent still lacked positional
options. That ancestor then kept matching its own same-named options
anywhere in argv, silently swallowing values that should have been
passed through untouched to the leaf command.

Walk the whole ancestor chain instead of just this.parent, matching
what the existing error message and JSDoc already promise ("parent
command(s)").

@nrps9909 nrps9909 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 99c705336a209e1ab83a45a38ae5d460d589729a. The direct .command() construction in the new tests is fixed, and all 1,422 tests plus types/lint/format pass locally (Node 26.8.1, macOS, with inherited color controls cleared).

The same swallowed-option problem is still reachable when an existing command tree is attached with addCommand:

import { Command } from './index.js';

const program = new Command().option('--dry-run');
const mid = new Command('mid').enablePositionalOptions();
mid.command('leaf')
  .argument('<utility>').argument('[args...]')
  .passThroughOptions()
  .action((utility, args) => console.log({ utility, args }));

program.addCommand(mid); // no validation error on this head
program.parse(['mid', 'leaf', 'git', 'push', '--dry-run'], { from: 'user' });
console.log(program.opts());

This prints { utility: 'git', args: ['push'] } and { dryRun: true }. The leaf is valid while detached, but after attaching the tree the new ancestor is never checked: addCommand calls the check only on mid, whose _passThroughOptions is false. Could the fix cover validation of pass-through descendants when attaching a subtree, with a regression for this construction order? The fully positional root should remain accepted and preserve --dry-run in the leaf arguments.

Please also retarget this PR from master to develop, as requested in CONTRIBUTING.md. The actual fix commit has two changed files on top of develop 97411c6; the current master target includes unrelated development changes in the PR diff.

Review and local validation performed with Codex.

@shadowspawn

Copy link
Copy Markdown
Collaborator

This appears to be a low value and low quality AI generated Pull Request.

@shadowspawn shadowspawn closed this Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants