Skip to content

feat(cli): add dynamic shell completion for flag values (#339)#512

Merged
lockwobr merged 2 commits into
mainfrom
feat/flag-value-completion
Apr 8, 2026
Merged

feat(cli): add dynamic shell completion for flag values (#339)#512
lockwobr merged 2 commits into
mainfrom
feat/flag-value-completion

Conversation

@lockwobr
Copy link
Copy Markdown
Contributor

@lockwobr lockwobr commented Apr 8, 2026

Summary

Closes #339

  • Add CompletableFlag wrapper that co-locates completion values with flag definitions, ensuring completions stay in sync with accepted values
  • Extend completeWithAllFlags to emit flag values when the user TABs after an enum flag
  • Wire completions for --intent, --service, --accelerator/--gpu, --os, --platform, --format, --deployer,
    --min-trust-level
  • Add GetTrustLevels() getter to verifier package (was the only enum type missing one)

Test plan

  • go test -race ./pkg/cli/... — 30 test cases pass including 6 new flag value completion tests
  • go test -race ./pkg/bundler/verifier/...TestGetTrustLevels passes
  • Manual verification: aicr recipe --intent "" --generate-shell-completion outputs inference, training
  • make qualify passes

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Refactoring (no functional changes)
  • Build/CI/tooling

Component(s) Affected

  • CLI (cmd/aicr, pkg/cli)
  • API server (cmd/aicrd, pkg/api, pkg/server)
  • Recipe engine / data (pkg/recipe)
  • Bundlers (pkg/bundler, pkg/component/*)
  • Collectors / snapshotter (pkg/collector, pkg/snapshotter)
  • Validator (pkg/validator)
  • Core libraries (pkg/errors, pkg/k8s)
  • Docs/examples (docs/, examples/)
  • Other: ____________

Implementation Notes

Testing

# Commands run (prefer `make qualify` for non-trivial changes)
make qualify

Risk Assessment

  • Low — Isolated change, well-tested, easy to revert
  • Medium — Touches multiple components or has broader impact
  • High — Breaking change, affects critical paths, or complex rollout

Rollout notes:

Checklist

  • Tests pass locally (make test with -race)
  • Linter passes (make lint)
  • I did not skip/disable tests to make CI green
  • I added/updated tests for new functionality
  • I updated docs if user-facing behavior changed
  • Changes follow existing patterns in the codebase
  • Commits are cryptographically signed (git commit -S) — GPG signing info

@lockwobr lockwobr self-assigned this Apr 8, 2026
@lockwobr lockwobr requested a review from a team as a code owner April 8, 2026 21:17
@lockwobr lockwobr force-pushed the feat/flag-value-completion branch from cd3b8db to 07a66e1 Compare April 8, 2026 21:22
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 8, 2026

Coverage Report ✅

Metric Value
Coverage 74.4%
Threshold 70%
Status Pass
Coverage Badge
![Coverage](https://img.shields.io/badge/coverage-74.4%25-green)

Merging this branch will increase overall coverage

Impacted Packages Coverage Δ 🤖
github.com/NVIDIA/aicr/pkg/bundler/verifier 63.79% (+0.80%) 👍
github.com/NVIDIA/aicr/pkg/cli 35.99% (+2.30%) 👍

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/NVIDIA/aicr/pkg/bundler/verifier/trust.go 77.55% (+2.55%) 49 (+5) 38 (+5) 11 👍
github.com/NVIDIA/aicr/pkg/cli/bundle.go 0.72% (ø) 139 1 138
github.com/NVIDIA/aicr/pkg/cli/bundle_verify.go 36.21% (+1.12%) 58 (+1) 21 (+1) 37 👍
github.com/NVIDIA/aicr/pkg/cli/completion_values.go 100.00% (+100.00%) 13 (+13) 13 (+13) 0 🌟
github.com/NVIDIA/aicr/pkg/cli/recipe.go 76.54% (ø) 81 62 19
github.com/NVIDIA/aicr/pkg/cli/root.go 70.41% (+1.36%) 98 (+14) 69 (+11) 29 (+3) 👍
github.com/NVIDIA/aicr/pkg/cli/snapshot.go 5.00% (ø) 40 2 38

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Add CompletableFlag wrapper that co-locates completion values with flag definitions, preventing drift between accepted and suggested values.
Flags wired: --intent, --service, --accelerator/--gpu, --os, --platform,  --format, --deployer, --min-trust-level.
@lockwobr lockwobr force-pushed the feat/flag-value-completion branch from 07a66e1 to c4b6188 Compare April 8, 2026 21:40
@yuanchen8911
Copy link
Copy Markdown
Contributor

yuanchen8911 commented Apr 8, 2026

Cross-Review Summary for PR #512

Reviewers: Claude Code, Codex, CodeRabbit + Integration Analysis
Rounds: 1
Consensus reached: Yes

Confirmed Issues

# Severity Finding Confirmed By
1 Low query.go:39 type assertion fragilef.(*cli.StringFlag) targets outputFlag (currently unwrapped, safe). If outputFlag is ever wrapped with withCompletions, this assertion silently fails and --output leaks into the query command. Recommend using f.Names() check instead. Codex + CodeRabbit + Integration

Positive Observations

  • Clean CompletableFlag interface design — extends cli.Flag via embedding, preserves all existing interface methods
  • Lazy func() []string completions always return current values from source of truth
  • formatFlag var → func eliminates shared mutable state in parallel tests
  • shellCompletionFlag constant replaces 4 string literals
  • 135 lines of new tests covering all completion paths, alias resolution, and cross-command inheritance
  • Both bash and zsh/fish shell paradigms explicitly handled and documented

Cross-review by Claude Code + Codex + CodeRabbit

Copy link
Copy Markdown
Contributor

@yuanchen8911 yuanchen8911 left a comment

Choose a reason for hiding this comment

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

/lgtm

@lockwobr lockwobr enabled auto-merge (squash) April 8, 2026 23:35
@lockwobr lockwobr merged commit 9b09c94 into main Apr 8, 2026
19 checks passed
@lockwobr lockwobr deleted the feat/flag-value-completion branch April 8, 2026 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Add dynamic shell completion for flag values (enums)

2 participants