Skip to content

feat(cli): add shell completion scripts (bash/zsh/fish) - #1234

Merged
akramcodez merged 8 commits into
Nano-Collective:mainfrom
puri-adityakumar:feat/1003-shell-completions
Sep 15, 2026
Merged

akramcodez merged 8 commits into
Nano-Collective:mainfrom
puri-adityakumar:feat/1003-shell-completions

Conversation

@puri-adityakumar

@puri-adityakumar puri-adityakumar commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Description

Closes #1003

The CLI surface was only discoverable through --help. nanocoder completion <bash|zsh|fish> prints a tab-completion script for the requested shell, so subcommands, flags, and known flag values complete at the prompt.

What changed:

  • source/cli-completions/spec.ts — single spec of every subcommand, flag, short flag, and closed value set; the source all three shell scripts render from, with a drift test so a new flag is one edit and scripts can't silently go stale
  • source/cli-completions/render.ts — bash/zsh/fish renderers; every word list, description, and value set is interpolated from the spec, nothing hardcoded
  • source/cli-completions/cli.ts — dispatch in the {exitCode, output, stream} shape daemon/cli.ts uses, wired as a fast path in cli.tsx (dynamic import, like daemon/init), so it exits before any Ink/provider code loads
  • Shell argument required: a missing or unknown shell exits 1 with usage
  • Help text lists the new command; docs at docs/features/shell-completions.md; changeset added (minor)

Not changed on purpose: in-app slash commands are not completed — they live inside the TUI, which already completes them, and never reach the shell. And no parser framework was introduced to auto-generate the scripts: the CLI parses args by hand, so spec + drift test give the same never-drifts guarantee without the migration.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)

Note: 4 git-tool tests fail on a clean main checkout in this environment too — the machine's global git template installs a commit-msg hook into the temp repos those tests create. Unrelated to this PR.

  • Tests cover both success and error scenarios

New tests (cli-completions/cli.spec.ts): missing/unknown shell exit 1 with usage, --help exits 0; each shell renders a registering script; drift checks that every spec flag, subcommand, child token, and enum value appears in all three scripts.

Manual Testing

Generated scripts syntax-checked in real shells (bash -n, zsh -n, fish -n). Functional smoke: bash completes subcommands and --mode values, fish offers subcommands with descriptions, zsh registers in both install modes (eval'd or autoloaded). Compiled dist/cli.js verified end-to-end with exit codes. Biome, tsc and knip all clean.

Checklist

  • If this was for an open issue, I was assigned to it (claimed in [Feature] Shell Completions (bash/zsh/fish) #1003)
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md)

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

nc-review: comments — 2 important, 5 nits

@puri-adityakumar — a few things worth a look, none blocking.

Adds nanocoder completion <bash|zsh|fish>, dispatched from the startup fast path in cli.tsx via a dynamic import, rendering three scripts from a single shared spec. Code is well-structured, tests cover the spec-to-script drift path, and the change resolves issue #1003. A few small zsh quoting oddities and a stray bash short-form collision are worth addressing before merge, but nothing blocking.

🟠 important · correctness · source/cli-completions/spec.ts:150

help and version both use short -h and -v respectively, but the bash script writes both --help and -h (and likewise --version / -v) into the same flags word list. If the user types nanocoder --h<Tab> bash will offer -h alongside --help, which is fine. The actual issue is the other direction — bash's default completion for -h will conflict with complete -F _nanocoder nanocoder only if _nanocoder doesn't fire, which it does because of -F. So this is not a bug, but the drift test in cli.spec.ts (every spec flag is offered by every shell script) verifies each flag appears once — and help/version short forms both correctly appear. No action required, but flagging because the test does not assert uniqueness, so a future duplicate entry (e.g. accidentally registering two -hs) would silently pass. Consider tightening the assertion to script.match(/-{1,2}h\b/g)?.length === 2 or similar for short flags.

🟠 important · completeness · docs/features/shell-completions.md

Issue #1003 explicitly mentions copilot login and daemon subcommands being discoverable, which this PR covers. However, the --help text shown in cli.tsx (top-level help) was updated to mention completion but init and daemon are listed without their short forms or value enums — that help text is hand-rolled and already drifts from the spec today. The new spec.ts comment acknowledges this:

* The hand-rolled parser in `cli.tsx` and the static `--help` text both
* describe the same flags; this module adds a third consumer ...

The author is aware. Worth filing: when --output-format is added to the spec, the help text in cli.tsx is not regenerated, so the closed enum appears in completion but not in --help. Either remove the enumeration from completion (no — completion users want it) or add a follow-up that renders --help from the same spec.

⚪ nit · correctness · source/cli-completions/render.ts:195

The zsh autoload-vs-eval guard uses unbraced $funcstack[1]:

if [ "$funcstack[1]" = "_nanocoder" ]

In zsh this expands to $funcstack followed by the literal [1], which is not what the surrounding #compdef-autoload path expects. Use ${funcstack[1]} for consistency with the rest of the script (the subcommand array uses ${name}-style braces elsewhere). Behaviorally this still passes [ ... ]'s argument through as a string and the author reports it works in both install modes, but it is the kind of subtle zsh trap that will bite a future reader.

⚪ nit · correctness · source/cli-completions/render.ts:90

The bash value-completion case arms reference $prev unquoted:

case "$prev" in
	--mode)
		COMPREPLY=( $(compgen -W "normal auto-accept yolo plan" -- "$cur") )

If a flag ever takes a value containing a glob character (or whitespace), the unquoted $prev in case will be word-split/glob-expanded by bash before matching. None of the current values contain globs so this is harmless today, but quoting inside the case pattern (case "$prev" in) is already done correctly — the issue is that future values added in spec.ts could be surprising. Consider documenting the constraint, or validating values in spec.ts.

⚪ nit · tests · source/cli-completions/cli.spec.ts:60

The drift tests assert that every spec flag and subcommand name appears as a substring in each script. Substrings are easy to false-positive: --mode is a substring of --mode-foo, run is a substring of runner, etc. None of the current spec entries collide, but the test would silently pass if a future contributor added a flag like mode-strict and a renderer bug emitted it twice. Tighten to word-boundary matches (new RegExp(\b${flag.name}\b)) or, better, to asserting the exact rendered word list once the spec stabilises.

⚪ nit · design · source/cli.tsx:128

The completion fast path checks args[0] === 'completion' after the init and run fast paths above it but before the --help check. That ordering is correct (nanocoder completion --help would hit --help first), but it means nanocoder init completion would be ambiguous — init is matched first as a positional argument and completion becomes its first arg, which init's parser will reject. Worth a one-line comment noting the precedence, or moving the completion branch above the init branch so completion always wins as a subcommand.

⚪ nit · warranted

No duplicate detected in the open-PR corpus (the closest are PRs #1196, #1200, #1201 which touch unrelated tooling, sessions and usage). This PR uniquely addresses #1003. Justified.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:comments nc-review left non-blocking findings label Sep 8, 2026
@puri-adityakumar
puri-adityakumar force-pushed the feat/1003-shell-completions branch from 751c828 to ad97763 Compare September 8, 2026 18:36
@puri-adityakumar
puri-adityakumar force-pushed the feat/1003-shell-completions branch from ad97763 to 6d02a6a Compare September 8, 2026 18:43
Comment thread source/cli-completions/render.ts Fixed
@akramcodez

Copy link
Copy Markdown
Member

@puri-adityakumar could you please address the issue suggested by the github-advanced-security?

@puri-adityakumar
puri-adityakumar marked this pull request as draft September 9, 2026 13:05
@puri-adityakumar

Copy link
Copy Markdown
Contributor Author

@puri-adityakumar could you please address the issue suggested by the github-advanced-security?

Yes sure, converting this PR in to draft.

@puri-adityakumar
puri-adityakumar marked this pull request as ready for review September 9, 2026 18:44
@puri-adityakumar

Copy link
Copy Markdown
Contributor Author

@akramcodez can you please run a review, I made the changes which were mentioned.

@akramcodez

Copy link
Copy Markdown
Member

/re-review

@github-actions

Copy link
Copy Markdown
Contributor

nc-review: comments — 1 important, 3 nits

@puri-adityakumar — a few things worth a look, none blocking.

PR adds nanocoder completion <bash|zsh|fish>, dispatched from the cli.tsx fast path via a dynamic import, rendering three self-contained scripts from a single shared spec. Code is well-structured, the boundary-aware drift tests in cli.spec.ts would catch a --mode vs --mode-foo collision, and the change resolves issue #1003. The previous review's substantive concerns (zsh funcstack braces, substring-matchers, init/completion ordering) have all landed. One pre-existing concern — --help text in cli.tsx and the new spec are hand-rolled in parallel — remains and is acknowledged in the spec's own comment but not yet fixed.

Addressed since the last review

  • ✅ zsh funcstack bracket now braced (${funcstack[1]})
  • ✅ drift tests now use boundary-aware matchers (longFlagPattern/shortFlagPattern) instead of plain includes()
  • ✅ subcommand/child token test uses \b${token}\b word boundaries
  • ✅ completion fast-path placement in cli.tsx is correct (between init and --help); ordering concern resolved
  • ✅ bash $prev is already quoted inside the case pattern; the prior nit's underlying concern (word-split/glob expansion) does not apply

🟠 important · completeness · source/cli.tsx:168

The cli.tsx --help text and source/cli-completions/spec.ts are now two hand-rolled sources of truth for the CLI surface. The spec's own header comment acknowledges this: "the hand-rolled parser in cli.tsx and the static --help text both describe the same flags; this module adds a third consumer ... The other two call sites still need a matching edit." As of this PR the two already drift: --help lists --mouse and --no-mouse (fullscreen-only TUI flags) but the spec omits them, and --help mentions --mode normal|auto-accept|yolo|plan without an enum constraint while the spec hard-codes the closed set. Either (a) generate --help from the spec, or (b) drop the spec entries for these flags and explicitly call out the gap. Leaving it as-is means the next flag added to the CLI silently skips one of the two surfaces.

⚪ nit · correctness · source/cli-completions/spec.ts:75

run is listed as a CompletionSubcommand next to init, daemon, config, codex, copilot, completion — but in cli.tsx only daemon/config/init/completion are handled as separate subcommands on the fast path; run is a positional token that falls through to the interactive app. Bash completion will happily suggest run as a top-level token, which is what users type, so this is technically harmless. But the spec comment ("init, run, daemon, …") elides the distinction, and a future contributor who adds a fast-path branch for run (e.g. to bypass Ink in CI) will need to remember this entry exists. A one-line note in the spec, or grouping the entry as positional: true, would prevent that surprise.

⚪ nit · correctness · source/cli-completions/render.ts:98

The bash renderer interpolates spec fields into a double-quoted bash variable:

const flags = [...longFlagNames(), ...shorts].join(' ');
// ...
local flags="${flags}"

Flag names that contain shell metacharacters (;, `, $, \, ", ', glob chars, whitespace) would either break the script or change its meaning once sourced. Today every entry in COMPLETION_FLAGS is a plain identifier so this is theoretical, but the spec exposes name and short as free-form string and does not validate them. Either validate at module load (/^[a-z][a-z0-9-]*$/) or document the constraint in CompletionFlag so a future contributor who adds e.g. --amend=force does not silently produce a script that re-runs nothing.

⚪ nit · scope

The PR adds a docs page (docs/features/shell-completions.md) and a changelog entry but does not add the new completion <shell> line to the top-level command listing in any obvious aggregator other than cli.tsx's static --help text. With the addition of completion, the CLI now has four fast-path subcommands (daemon, config, init, completion) but the README still only documents init and the in-app init slash command. Not a blocker — the docs page is self-contained — but readers looking for "how do I install completions" will not find the new feature from the repo root.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

akramcodez and others added 4 commits September 15, 2026 10:47
The --help text listed copilot login but omitted codex login, even
though cli.tsx handles 'codex login' as a fast-path subcommand and
cli-completions/spec.ts lists it alongside copilot. The three
descriptions of the CLI surface are now aligned.
The cli-integration tests spawn `node dist/cli.js` to exercise the
real CLI entry point, but `test:ava` did not ensure the build was
fresh. The new completion tests added by this PR depend on dist/cli.js
having the completion fast path, so the tests fail whenever the
build artefact is missing or stale.
cli.tsx is a CLI entry point whose branches can only be exercised by
spawning the compiled binary, which c8 does not track. The new
`completion` fast path added by this PR contributed uncovered lines
that dragged the project-wide coverage drop below the baseline.

Adding cli.tsx to the c8 exclude list mirrors the treatment already
applied to source/app/App.tsx and source/web/page.ts, both of which
are likewise entry-point surfaces covered by spawnSync integration
tests rather than in-process AVA tests. The matching `include`
pattern restricts the report to source files so the exclusion takes
effect reliably.
@akramcodez
akramcodez merged commit e7d2393 into Nano-Collective:main Sep 15, 2026
16 checks passed
@akramcodez

Copy link
Copy Markdown
Member

Thanks for the PR @puri-adityakumar

@puri-adityakumar
puri-adityakumar deleted the feat/1003-shell-completions branch September 16, 2026 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:comments nc-review left non-blocking findings area:docs Documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Shell Completions (bash/zsh/fish)

3 participants