Skip to content

feat(completion): complete context names for positional args#424

Merged
JoshMock merged 2 commits into
elastic:mainfrom
levontumanyan:feat/positional-completion-context-set
Jun 18, 2026
Merged

feat(completion): complete context names for positional args#424
JoshMock merged 2 commits into
elastic:mainfrom
levontumanyan:feat/positional-completion-context-set

Conversation

@levontumanyan

@levontumanyan levontumanyan commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Extends shell tab completion to offer context names as candidates when completing positional arguments on leaf commands — specifically config current-context set, config context edit, and config context remove.

Before this change, pressing Tab after elastic config current-context set yielded no candidates. After it, the context names from ~/.elasticrc.yml are offered.

Approach

  • Adds an optional getPositional(commandPath: string) method to the DynamicCompleterRegistry interface. The command path is the space-joined ancestry excluding the root program name (e.g. "config current-context set").
  • Adds a getCommandPath() helper to enumerate.ts that walks cmd.parent to build the path.
  • In enumerate(), when the walk lands on a leaf command (no children) and a positional completer is registered for its path, that completer's candidates are returned instead of the empty children list.
  • Registers the three above commands in registry.ts, all reusing the existing completeContextNames function — no new I/O code.

The change is additive: registries that don't implement getPositional continue to work unchanged.

Test plan

  • enumerate -- positional completer registry suite (4 new tests): invocation, prefix-filtering, error tolerance, no call when command has children
  • defaultRegistry -- positional completers suite (4 new tests): registered paths, unregistered paths
  • npm test — 1471/1471 pass
  • Manual: elastic config current-context set <TAB> lists context names from config

🤖 Generated with Claude Code

Extend the DynamicCompleterRegistry interface with an optional
getPositional(commandPath) method. enumerate() now checks for a
positional completer when it lands on a childless leaf command, using
the space-joined command path (e.g. "config current-context set") as
the lookup key.

Registers completers for:
- config current-context set
- config context edit
- config context remove

All three reuse the existing completeContextNames function, so typing
`elastic config current-context set <TAB>` now offers the context names
from ~/.elasticrc.yml.

@JoshMock JoshMock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! Thanks for the contrib. 🙏

@JoshMock JoshMock enabled auto-merge (squash) June 17, 2026 15:19
@levontumanyan

levontumanyan commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Hi @JoshMock,

All GitHub Actions checks are green. The Buildkite job is waiting on a maintainer to approve the fork pipeline run — would you be able to trigger it or merge directly if you're happy with the changes?

@plinde plinde left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 Claude Code

Two non-blocking improvements from a local review. Nice, tightly-scoped change.


const cands = collectChildren(current)
const children = collectChildren(current)
if (children.length === 0 && registry != null) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The branch offers positional candidates whenever the leaf has no children, regardless of how many positionals are already typed. For a single-positional command like set, config current-context set prod <TAB> walks to the set leaf (prod isn't a child, so the walk breaks on it) and context names are offered again for a second positional the command doesn't accept. Harmless in practice — but worth a one-line note documenting it as a conscious won't-fix, since matching positional depth would add real complexity:

Suggested change
if (children.length === 0 && registry != null) {
// Leaf command (no subcommands): offer a registered positional completer.
// Note: this fires regardless of how many positional args are already typed,
// so a single-positional command re-offers candidates after the first.
if (children.length === 0 && registry != null) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤖 Claude Code

Both addressed in 935fffe — added the inline comment and updated the algorithm docstring in step 5.

const cands = collectChildren(current)
const children = collectChildren(current)
if (children.length === 0 && registry != null) {
const positionalCompleter = registry.getPositional?.(getCommandPath(current))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doc nit: the function-level JSDoc above (the numbered algorithm, step 5) still describes the default branch as returning only "subcommand names and aliases of the deepest matched command/group." With this change, a leaf command that has a registered positional completer returns those candidates instead. The interface comment was updated, but the algorithm narrative — the canonical description of enumerate's priority order — wasn't. Consider adding the positional-completer case to step 5 so the docstring stays authoritative.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤖 Claude Code

Both addressed in 935fffe — added the inline comment and updated the algorithm docstring in step 5.

- Add inline comment noting that the positional completer fires
  regardless of how many positional args are already typed
- Update enumerate() JSDoc algorithm step 5 to describe the
  positional-completer fallback path
auto-merge was automatically disabled June 17, 2026 17:38

Head branch was pushed to by a user without write access

@levontumanyan levontumanyan requested a review from plinde June 18, 2026 07:52
@JoshMock

Copy link
Copy Markdown
Member

buildkite test this

@JoshMock

Copy link
Copy Markdown
Member

Buildkite is for integration tests against ES, KB and control plane APIs, which isn't affected by shell completion changes. Will ignore that and merge.

@JoshMock JoshMock merged commit 3883800 into elastic:main Jun 18, 2026
26 checks passed
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.

3 participants