Skip to content

cli: parse a help flag after the verb in the pm and node groups - #739

Open
colinhacks wants to merge 3 commits into
mainfrom
fix-653-subverb-help
Open

cli: parse a help flag after the verb in the pm and node groups#739
colinhacks wants to merge 3 commits into
mainfrom
fix-653-subverb-help

Conversation

@colinhacks

Copy link
Copy Markdown
Contributor

Every nub pm and nub node sub-verb ignored --help/-h and ran the verb. nub pm shim --help installed six shims and added a PATH block to every shell startup file; nub pm unshim --help removed them. The two nub node shim verbs behaved the same way.

The top-level scan stops matching nub's own flags once a subcommand is seen, so nub run build --watch reaches the script. Both groups bypass clap and guarded help on argv[0] only, so past it the flag was dropped.

Both now recognize a help flag at any position. Neither forwards argv to a child, and their argument consumers take a package-manager name or a version, so no help flag was valid there.

Closes #653

`nub pm shim --help` installed the six package-manager shims and appended a
PATH block to every shell startup file for the current shell, and `nub pm
unshim --help` removed them again. `nub node shim --help` and `nub node
unshim --help` behaved the same way. None of the fifteen `nub pm` / `nub
node` verbs printed help for `--help` or `-h`.

The top-level argument scan stops matching nub's own flags once a subcommand
is seen, so `nub run build --watch` reaches the script rather than nub (the
three-position rule). `pm` and `node` are non-forwarding groups that bypass
clap, and their help guard only inspected argv[0]; past it the sub-verbs read
no arguments at all, so the flag was dropped and the verb ran.

Both groups now recognize `--help` and `-h` at any position. Neither forwards
argv to a child process, and their only argument consumers take a package
manager name or a version, so no help flag was ever a valid value there.

The regression test asserts the predicate rather than driving `run_pm`: a test
that ran the real verb would install shims into the test runner's own HOME the
moment this regressed.

Closes #653
Copilot AI lite review requested due to automatic review settings August 15, 2026 18:11

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@vercel

vercel Bot commented Aug 15, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Ready Ready Preview Aug 15, 2026 7:13pm

Request Review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No correctness issues found — one follow-up worth a decision.

Reviewed changes

  • group_help_requested helper — new predicate in crates/nub-cli/src/cli.rs matching help at argv[0] or --help/-h at any position, with a doc comment recording why the blanket scan is safe.
  • run_node / run_pm guards — both group help guards now call the helper instead of matching on argv[0] only, so nub pm shim --help and nub node shim --help print help instead of installing shims and editing shell startup files.
  • Unit testgroup_help_is_recognized_after_the_verb covers the post-verb forms, the pre-existing argv[0] forms, and real verb invocations that must still run.

I reproduced the bug against published @nubjs/nub@0.7.5 to confirm the diagnosis in the PR body: nub node ls --help prints the version listing and nub pm cache --help prints the cache listing, so argv really does reach both groups with the flag intact and the argv[0]-only guard was the whole cause. I also enumerated every match arm in both groups — use takes a package-manager name, install/uninstall/pin take versions, cache takes clear, and the rest take nothing — so the blanket scan cannot swallow a legitimate value, and neither group forwards argv to a child. Clap-dispatched commands are unaffected: nub init --help already prints usage and scaffolds nothing.

ℹ️ nub agent is the third non-forwarding group and keeps the old guard

crates/nub-cli/src/agent/mod.rs:55 carries the identical argv[0]-only match, and the module doc describes it as "a non-forwarding group handled by a manual sub-verb match (like nub node / nub pm)". On 0.7.5 nub agent skill --help prints the skill and ignores the flag, while nub agent docs --help and -h fail with unexpected argument '--help'. Nothing destructive happens, so this is a follow-up rather than a blocker — but the helper's doc comment now enumerates two of the three groups it describes, which is the kind of gap that goes stale quietly.

Technical details
# Extend the group help guard to `nub agent`

## Affected sites
- `crates/nub-cli/src/agent/mod.rs:55``matches!(verb, None | Some("help") | Some("--help") | Some("-h"))`, the same argv[0]-only shape the PR replaces in `run_pm` / `run_node`.
- `crates/nub-cli/src/agent/mod.rs:~78` (`run_docs`) — the `other => bail!` arm is what turns a trailing `--help` into `unexpected argument '--help'`.
- `crates/nub-cli/src/cli.rs:8513-8523` — the new doc comment names `nub pm` and `nub node` as "a non-forwarding command group"; `agent` is a third one.

## Required outcome
- `nub agent docs --help` and `nub agent skill --help` print help rather than erroring or ignoring the flag, or the decision to leave `agent` alone is recorded so the next reader does not re-derive it.

## Suggested approach (optional)
- `group_help_requested` is a private `fn` in `cli.rs`; reaching it from `agent/mod.rs` needs `pub(crate)` (and arguably a home shared by all three groups).
- `agent`'s argument consumer takes a `/docs/...` slug, so the same "no help flag is valid there" reasoning holds.

## Open questions for the human
- Is keeping this PR scoped to the destructive groups deliberate, with `agent` tracked separately?

ℹ️ Nitpicks

  • crates/nub-cli/src/cli.rs:8888 — the line comment above the run_node guard still reads "nub node --help/-h/help: short usage listing the verbs", which describes the argv[0] forms only now that the guard fires at any position.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

`nub agent` is the third non-forwarding group that bypasses clap for a manual
sub-verb match, and it carried the same argv[0]-only guard: `nub agent docs
--help` failed with `unexpected argument '--help'`, and `nub agent skill
--help` printed the skill markdown with the flag ignored. Neither is
destructive, but both are the same defect.

`group_help_requested` becomes `pub(crate)` and `agent::run` calls it, so all
three groups share one definition. The `agent` argument consumer takes a
`/docs/...` slug, so no help flag was a valid value there either.

The agent test asserts only the `docs` form. The `skill` form returned 0
before this change too, so an exit-code assertion on it could never fail; the
flag-anywhere semantics it relies on are asserted on the predicate instead.

Also corrects the comment above the `run_node` guard, which still described
the argv[0] forms only.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No correctness issues in the new commit — one doc-comment wording nit inline.

Reviewed changes

Since the prior review (6dc7cb9), one commit extends the fix to the third non-forwarding group and tidies the surrounding comments.

  • nub agent now shares the guardagent::run calls crate::cli::group_help_requested instead of matching argv[0], so nub agent docs --help prints usage rather than failing with unexpected argument '--help', and nub agent skill --help stops dumping the whole skill markdown.
  • Helper opened up and re-documentedgroup_help_requested became pub(crate) and its doc comment now names all three groups and restates the "safe as a blanket scan" rationale to cover agent's /docs/... slug consumer.
  • run_node guard comment refreshed — it now describes the any-position semantics instead of the argv[0] forms only (the prior review's nitpick).
  • Wiring test addedagent::tests::a_help_flag_after_the_verb_is_not_an_unexpected_argument drives run(&["docs", flag]) and asserts Ok(0); it fails without the fix because run_docs's other => bail! arm makes .unwrap() panic. The cli.rs predicate test gained docs --help / skill -h positives and a docs --page /docs/runtime/jsx negative.

I confirmed agent is in SUBCOMMANDS and is routed by dispatch_subcommand ahead of clap, so the top-level three-position scan hands the flag through untouched — the same path that made the pm/node bug reachable. I also checked the one manual group left out: run_global does its own argv[0]-only match, but it forwards to the engine config verb through parse_config_args, a clap parser that already handles --help at any position, so it has neither the bug nor the non-forwarding property the blanket scan depends on. Excluding it is correct.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread crates/nub-cli/src/cli.rs Outdated
The comment claimed the sub-verbs never read past argv[0]. They do —
`install <version>`, `use <pm>` and `docs --page <slug>` all take a value.
It was each group's help GUARD that inspected argv[0] alone, and the flag
then met whatever the verb does with its arguments, which is why the same
defect surfaced three ways: ignored and the verb ran, consumed as a bad
value, or rejected as an unexpected argument.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

Since the prior review (981d77f), one commit changes the group_help_requested doc comment and nothing else — no code, no test, no behavior.

  • Helper doc comment corrected — the middle paragraph now attributes the argv[0]-only inspection to each group's help guard rather than to the sub-verbs, and replaces the blanket "silently dropped" claim with the three symptoms the defect actually produced.

I checked each factual claim the new text makes rather than taking it at face value. no published Node version matches "--help" is the exact format string at crates/nub-core/src/version_management/manage.rs:89, so a value-taking verb really did consume the flag as a bad version. unexpected argument '--help' matches crates/nub-cli/src/agent/mod.rs:104, the other => bail! arm the new agent test asserts against. And the load-bearing "no group forwards argv to a child process" premise still holds — I re-enumerated run_pm's arms (which, use, update/up, cache, pin, shim, unshim, switch, _) and none spawns with the group's argv.

This resolves the inline thread from the prior review; I've replied and marked it resolved.

Pullfrog  | View workflow run | Using Claude Opus𝕏

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.

nub pm shim --help runs the shim installer instead of printing help

2 participants