test(cli): raise component coverage above 92 percent - #635
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:XXL
WalkthroughThe PR extracts CLI configuration and plugin prompts into dedicated modules, isolates logging and teardown helpers, refactors destination collection, updates Codecov exclusions, and expands integration and coverage tests. ChangesCLI interaction and runtime refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cli/src/commands/configure/editor/prompt.rs`:
- Around line 213-219: Extract a shared require_nonempty validator returning
Result<(), &'static str> with the unified "value must not be empty" message,
then replace the inline empty-value validation closures at the three affected
prompt locations with .validate_with(require_nonempty).
In `@crates/cli/src/commands/configure/wizard/prompt.rs`:
- Around line 179-188: Unify the duplicated TTY validation through a shared
helper in the CLI TTY module. In
crates/cli/src/commands/configure/wizard/prompt.rs lines 179-188, replace
ensure_tty’s inline stdin-only check with the shared helper using the
interactive menu stream requirement and preserve its message; in
crates/cli/src/commands/configure/editor/prompt.rs lines 58-60, retain
ensure_tty_with for testability but delegate it to the same helper with its
required streams and message. Ensure the existing plugins prompt guard also uses
this shared definition.
- Around line 179-188: Consolidate the duplicated ensure_tty implementations by
introducing one shared helper that accepts the required stream-readiness checks
and error message. Update the configure editor prompt, plugin prompt, and wizard
prompt callers to use it, preserving each caller’s required stdin/stdout/stderr
checks and user-facing message while removing their local ensure_tty
definitions.
- Around line 87-94: Update the configuration save flow around build_config,
confirm_summary, and save_config so agent_hint being None preserves existing
non-agent sections instead of overwriting them. Merge the current file’s
sections such as upstream, gateway, and logging into the document before preview
and saving, or require an explicit destructive confirmation that accurately
displays their removal.
In `@crates/cli/src/plugins/dynamic_editor/prompt.rs`:
- Around line 352-401: No change is required for this diff; preserve the
existing prompt_raw_config sequence, including redaction before display, secret
restoration before validation, and committing only after successful validation.
Treat re-prompting on errors as a separate follow-up rather than modifying this
implementation.
- Around line 14-35: The condition in edit_dynamic_plugin redundantly checks
state.schema.is_none() because fields is empty in that case; simplify the menu
selection condition to fields.is_empty(), preserving the existing root-menu and
fields-menu behavior.
In `@crates/cli/src/plugins/prompt.rs`:
- Around line 529-531: Update the Reset handling in the MenuResponse shortcut
match to re-assert the expected container type after calling
collection_shortcut_value, ensuring malformed defaults cannot reach the next
iteration’s as_array().expect(...) or as_object().expect(...). Preserve the
existing Clear behavior and use the shortcut-specific collection type when
coercing the reset value.
In `@crates/cli/tests/coverage/commands/configure_editor_tests.rs`:
- Line 339: Update the tests around clear_key and clear_sink_key to assert the
resulting configuration state after each operation, not merely successful return
values. Verify clear_key("missing", "value") removes the intended key and
clear_sink_key(0, "level") removes the sink-level key, adding assertions
alongside the existing path checks while preserving unrelated state assertions.
- Around line 369-378: Update
project_path_defaults_to_start_when_no_ancestor_config_exists to constrain
project_config_path’s ancestor search to the temporary root, preventing
configuration files outside the test fixture from affecting the result while
preserving the expected nested default path.
In `@crates/cli/tests/coverage/shared/plugins_lifecycle_tests.rs`:
- Around line 4417-4517: Strengthen
lifecycle_commands_cover_json_and_human_output_paths by capturing each list,
add, validate, and inspect result instead of only calling unwrap. For JSON
responses, parse the output and assert schema_version, ok, command, and a
representative field under data; for human responses, assert meaningful
command-specific text. Follow assertion patterns used elsewhere in this test
file and include lifecycle-event or scope-stack behavior where available rather
than relying only on successful completion.
In `@crates/cli/tests/coverage/shared/plugins_schema_tests.rs`:
- Around line 280-286: Update native_config_field to replace the bare unwrap
with an expectation that includes the requested key, so lookup failures identify
which schema field is missing while preserving the existing return behavior.
In `@crates/cli/tests/coverage/shared/plugins_tests.rs`:
- Around line 2152-2176: Rename the second `path` binding in
`dynamic_editor_raw_menu_and_nested_value_paths_are_deterministic` to
`field_path`, and update the related `set_value_at_path`, `value_at_path`, and
`remove_value_at_path` calls to use it; leave the filesystem `path` binding
unchanged.
- Around line 599-634: Expand
menu_keys_cover_selection_shortcuts_and_cancellation to assert every key in the
multi-key mappings handled by menu_response_for_key: add Del as Clear, and CtrlC
plus Char('q') as Cancel. Keep the existing assertions and expected response
indices unchanged.
- Around line 746-758: Strengthen component_shortcut_fallbacks_are_safe_noops by
capturing components[0] before calling reset_component_menu_item and
clear_component_menu_item, then asserting it remains unchanged afterward. Keep
the existing successful-result checks and verify both None and Back fallback
actions preserve the component state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c1d1d959-14d8-49f0-832e-33c294d579c3
📒 Files selected for processing (20)
codecov.ymlcrates/cli/src/commands/configure/editor.rscrates/cli/src/commands/configure/editor/prompt.rscrates/cli/src/commands/configure/wizard.rscrates/cli/src/commands/configure/wizard/prompt.rscrates/cli/src/commands/mod.rscrates/cli/src/plugins/dynamic_editor.rscrates/cli/src/plugins/dynamic_editor/prompt.rscrates/cli/src/plugins/mod.rscrates/cli/src/plugins/prompt.rscrates/cli/src/process/launcher.rscrates/cli/src/server/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/commands/configure_editor_tests.rscrates/cli/tests/coverage/shared/config_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/shared/plugins_lifecycle_tests.rscrates/cli/tests/coverage/shared/plugins_schema_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/tests/coverage/shared/server_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Check / Run
🧰 Additional context used
📓 Path-based instructions (12)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/cli/src/commands/mod.rscrates/cli/tests/coverage/shared/config_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/configure_editor_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/plugins_schema_tests.rscrates/cli/src/commands/configure/editor.rscrates/cli/src/commands/configure/editor/prompt.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/dynamic_editor/prompt.rscrates/cli/tests/coverage/shared/plugins_lifecycle_tests.rscrates/cli/src/commands/configure/wizard.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/commands/configure/wizard/prompt.rscrates/cli/src/plugins/mod.rscrates/cli/src/plugins/dynamic_editor.rscrates/cli/src/plugins/prompt.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/src/commands/mod.rscrates/cli/tests/coverage/shared/config_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/configure_editor_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/plugins_schema_tests.rscrates/cli/src/commands/configure/editor.rscrates/cli/src/commands/configure/editor/prompt.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/dynamic_editor/prompt.rscrates/cli/tests/coverage/shared/plugins_lifecycle_tests.rscrates/cli/src/commands/configure/wizard.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/commands/configure/wizard/prompt.rscrates/cli/src/plugins/mod.rscrates/cli/src/plugins/dynamic_editor.rscrates/cli/src/plugins/prompt.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/cli/src/commands/mod.rscrates/cli/tests/coverage/shared/config_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/configure_editor_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/plugins_schema_tests.rscrates/cli/src/commands/configure/editor.rscrates/cli/src/commands/configure/editor/prompt.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/dynamic_editor/prompt.rscrates/cli/tests/coverage/shared/plugins_lifecycle_tests.rscrates/cli/src/commands/configure/wizard.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/commands/configure/wizard/prompt.rscrates/cli/src/plugins/mod.rscrates/cli/src/plugins/dynamic_editor.rscrates/cli/src/plugins/prompt.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/cli/src/commands/mod.rscrates/cli/tests/coverage/shared/config_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/configure_editor_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/plugins_schema_tests.rscrates/cli/src/commands/configure/editor.rscrates/cli/src/commands/configure/editor/prompt.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/dynamic_editor/prompt.rscrates/cli/tests/coverage/shared/plugins_lifecycle_tests.rscrates/cli/src/commands/configure/wizard.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/commands/configure/wizard/prompt.rscrates/cli/src/plugins/mod.rscrates/cli/src/plugins/dynamic_editor.rscrates/cli/src/plugins/prompt.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/cli/src/commands/mod.rscrates/cli/tests/coverage/shared/config_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/configure_editor_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/plugins_schema_tests.rscrates/cli/src/commands/configure/editor.rscrates/cli/src/commands/configure/editor/prompt.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/dynamic_editor/prompt.rscrates/cli/tests/coverage/shared/plugins_lifecycle_tests.rscrates/cli/src/commands/configure/wizard.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/commands/configure/wizard/prompt.rscrates/cli/src/plugins/mod.rscrates/cli/src/plugins/dynamic_editor.rscrates/cli/src/plugins/prompt.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/cli/src/commands/mod.rscrates/cli/src/server/mod.rscrates/cli/src/process/launcher.rscrates/cli/src/commands/configure/editor.rscrates/cli/src/commands/configure/editor/prompt.rscrates/cli/src/plugins/dynamic_editor/prompt.rscrates/cli/src/commands/configure/wizard.rscrates/cli/src/commands/configure/wizard/prompt.rscrates/cli/src/plugins/mod.rscrates/cli/src/plugins/dynamic_editor.rscrates/cli/src/plugins/prompt.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/cli/src/commands/mod.rscodecov.ymlcrates/cli/tests/coverage/shared/config_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/configure_editor_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/plugins_schema_tests.rscrates/cli/src/commands/configure/editor.rscrates/cli/src/commands/configure/editor/prompt.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/dynamic_editor/prompt.rscrates/cli/tests/coverage/shared/plugins_lifecycle_tests.rscrates/cli/src/commands/configure/wizard.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/commands/configure/wizard/prompt.rscrates/cli/src/plugins/mod.rscrates/cli/src/plugins/dynamic_editor.rscrates/cli/src/plugins/prompt.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
**/*.{rs,py,go,js,ts}: When observability configuration or lifecycle is exposed, keep FFI and Python, Go, and Node.js binding-native config objects and subscriber/exporter methods aligned in logical knobs and semantics.
Require every OpenTelemetry endpoint to have a type and nonblank destination; resolveheader_envvalues at activation and reject missing, blank, or duplicate headers.
Concatenate layered ATOF sink, ATIF storage, and OpenTelemetry endpoint lists with higher-precedence entries first.
Preserve correct handling of mark events, start/end events, orphan cases, and span or trajectory fields derived from intended event data.
Run affected Rust tests andjust test-rustwhen event fields change; runjust test-python,just test-go, andjust test-nodewhen binding-native configuration or lifecycle changes.
Files:
crates/cli/src/commands/mod.rscrates/cli/tests/coverage/shared/config_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/configure_editor_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/plugins_schema_tests.rscrates/cli/src/commands/configure/editor.rscrates/cli/src/commands/configure/editor/prompt.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/dynamic_editor/prompt.rscrates/cli/tests/coverage/shared/plugins_lifecycle_tests.rscrates/cli/src/commands/configure/wizard.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/commands/configure/wizard/prompt.rscrates/cli/src/plugins/mod.rscrates/cli/src/plugins/dynamic_editor.rscrates/cli/src/plugins/prompt.rs
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
crates/cli/src/commands/mod.rscrates/cli/tests/coverage/shared/config_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/configure_editor_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/plugins_schema_tests.rscrates/cli/src/commands/configure/editor.rscrates/cli/src/commands/configure/editor/prompt.rscrates/cli/tests/cli_tests.rscrates/cli/src/plugins/dynamic_editor/prompt.rscrates/cli/tests/coverage/shared/plugins_lifecycle_tests.rscrates/cli/src/commands/configure/wizard.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/commands/configure/wizard/prompt.rscrates/cli/src/plugins/mod.rscrates/cli/src/plugins/dynamic_editor.rscrates/cli/src/plugins/prompt.rs
codecov.yml
📄 CodeRabbit inference engine (.agents/skills/maintain-ci/SKILL.md)
Keep Codecov component paths aligned with new crates, packages, and generated outputs; dynamic plugin SDK/protocol paths belong in the plugin component.
Files:
codecov.yml
{justfile,codecov.yml,codecov.yaml,.github/workflows/**/*.yml,.github/workflows/**/*.yaml}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
justfile, Codecov, and CI package/test workflows must include new plugin crates and packages.
Files:
codecov.yml
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/cli/tests/coverage/shared/config_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/configure_editor_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/plugins_schema_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/plugins_lifecycle_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rs
🔇 Additional comments (43)
crates/cli/src/commands/mod.rs (2)
56-97: LGTM!Also applies to: 99-107
56-107: 📐 Maintainability & Code QualityRun the mandated Rust validation.
The supplied context does not confirm execution of the required Rust commands. Run
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings, andjust test-rustbefore merge.
crates/cli/src/commands/mod.rs#L56-L107: validate the logging setup extraction.crates/cli/src/process/launcher.rs#L709-L779: validate the observability destination helper extraction.crates/cli/src/server/mod.rs#L415-L454: validate the server teardown logging helper extraction.As per coding guidelines, “Any Rust change must run
just test-rust” and must run the specified formatter and Clippy commands.Source: Coding guidelines
crates/cli/src/process/launcher.rs (1)
709-713: LGTM!Also applies to: 715-737, 739-759, 761-779
crates/cli/src/server/mod.rs (1)
415-418: LGTM!Also applies to: 431-454
crates/cli/tests/cli_tests.rs (2)
726-726: LGTM!Also applies to: 755-755, 775-813
1984-1984: LGTM!Also applies to: 2102-2102, 3528-3528, 3907-3907, 4003-4003
crates/cli/tests/coverage/shared/config_tests.rs (1)
4151-4188: LGTM!Also applies to: 4189-4238, 4240-4255
crates/cli/tests/coverage/shared/installer_tests.rs (1)
583-589: LGTM!Also applies to: 589-605, 607-640, 642-670
crates/cli/tests/coverage/shared/plugins_lifecycle_tests.rs (1)
1921-1921: LGTM!Also applies to: 1945-1946, 1989-2045, 4259-4346, 4348-4401, 4403-4415, 4519-4534
crates/cli/tests/coverage/shared/server_tests.rs (1)
676-705: LGTM!Also applies to: 2149-2190, 2192-2251, 2253-2268
crates/cli/src/commands/configure/editor.rs (1)
17-17: LGTM!Also applies to: 38-43
crates/cli/src/commands/configure/editor/prompt.rs (2)
19-56: LGTM!
280-309: LGTM!Also applies to: 311-342, 415-440
crates/cli/src/commands/configure/wizard.rs (2)
39-44: LGTM!
6-31: 🩺 Stability & AvailabilityNo issue found.
plugin_prompt_was_interruptedremains defined and resolves fromprompt.rs. The test module is#[cfg(test)], and the gated imports are used by the test module.> Likely an incorrect or invalid review comment.crates/cli/src/commands/configure/wizard/prompt.rs (4)
25-74: LGTM!
112-147: LGTM!Also applies to: 149-161
201-257: LGTM!Also applies to: 262-299
76-79: 🚀 Performance & ScalabilityKeep
runasync. Its caller awaits it, and the CLI uses a multi-threaded Tokio runtime. The synchronous wizard can block one worker thread during terminal input, but this matches the pre-refactor behavior and requires no change.crates/cli/tests/coverage/commands/configure_editor_tests.rs (2)
303-303: LGTM!Also applies to: 306-338
342-357: LGTM!crates/cli/src/plugins/dynamic_editor.rs (2)
25-25: LGTM!Also applies to: 153-156
363-363: 📐 Maintainability & Code QualityRetain
pub(super)for the tested helpers.
plugins_testsis an in-crate sibling ofdynamic_editor, so these calls require visibility throughcrate::plugins.pub(in crate::plugins::dynamic_editor)would exclude the test module. The widening is unnecessary forprompt.rsbut required by the tests.> Likely an incorrect or invalid review comment.crates/cli/src/plugins/dynamic_editor/prompt.rs (3)
37-105: LGTM!Also applies to: 107-162
164-277: LGTM!Also applies to: 317-350
289-289: 📐 Maintainability & Code QualityNo change is required. The workspace does not enable
clippy::uninlined_format_args, and this lint is not enabled by default.> Likely an incorrect or invalid review comment.crates/cli/src/plugins/mod.rs (3)
4-10: LGTM!Also applies to: 23-23, 106-108
235-246: LGTM!
32-35: 📐 Maintainability & Code QualityKeep these imports private; do not use
pub(crate) use. Descendant modules can use private parent imports.pub(crate) useis rejected because these functions arepub(super). Direct imports fromcrate::plugins::promptare optional.> Likely an incorrect or invalid review comment.crates/cli/src/plugins/prompt.rs (6)
14-55: LGTM!Also applies to: 57-122, 124-189
191-234: LGTM!Also applies to: 289-299
675-728: LGTM!Also applies to: 730-830
1254-1266: LGTM!
1194-1202: 🎯 Functional CorrectnessNo change needed:
parse_float_valueis still defined.crates/cli/src/plugins/mod.rs:742defines it, anduse super::*resolves the call.> Likely an incorrect or invalid review comment.
1220-1233: 🩺 Stability & AvailabilityDo not require an empty-enum guard for safety.
dialoguer::Select::interact()returns an error for an empty list, andeditor_errorconverts it toCliError::Config;values[idx]is not reached. An explicit field-specific error remains an optional clarity improvement.> Likely an incorrect or invalid review comment.crates/cli/tests/coverage/shared/plugins_schema_tests.rs (1)
275-278: LGTM!Also applies to: 319-334
crates/cli/tests/coverage/shared/plugins_tests.rs (6)
323-326: LGTM!Also applies to: 328-340, 342-408, 410-431
636-657: LGTM!Also applies to: 659-714, 716-744
790-807: LGTM!Also applies to: 1881-1907
2071-2103: LGTM!Also applies to: 2105-2150
2178-2193: LGTM!Also applies to: 2195-2261, 2324-2374, 2376-2399, 2401-2412
760-788: 📐 Maintainability & Code QualityNo change needed.
editable_componentsaddsSwitchyardunder the same feature gate as the enum variant, so the test covers every variant present in the active build. The shown CI job only publishesnemo-relay-switchyard; this does not make the test name inaccurate.> Likely an incorrect or invalid review comment.codecov.yml (1)
138-143: 🗄️ Data Integrity & IntegrationNo Codecov path update is required. The component paths are broad, all four prompt files exist, and no stale references remain.
> Likely an incorrect or invalid review comment.
| .validate_with(|value: &String| { | ||
| if value.trim().is_empty() { | ||
| Err("value must not be empty; use Clear to remove it") | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Unify the two empty-value validators.
Line 213 returns Err(&'static str). Line 295 returns Err(String). Line 353 also returns Err(String). The messages also differ: "value must not be empty; use Clear to remove it" versus "value must not be empty". Three near-identical closures exist in this file. Extract one helper.
♻️ Proposed helper
fn require_nonempty(value: &String) -> Result<(), &'static str> {
if value.trim().is_empty() {
Err("value must not be empty")
} else {
Ok(())
}
}Then use .validate_with(require_nonempty) at lines 213, 295, and 353.
Also applies to: 295-301
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/cli/src/commands/configure/editor/prompt.rs` around lines 213 - 219,
Extract a shared require_nonempty validator returning Result<(), &'static str>
with the unified "value must not be empty" message, then replace the inline
empty-value validation closures at the three affected prompt locations with
.validate_with(require_nonempty).
| let doc = build_config(&answers); | ||
| let preview_paths = preview_paths(answers.scope, &cwd, &home); | ||
|
|
||
| if !confirm_summary(&preview_paths, &doc)? { | ||
| return Err(CliError::Config("setup cancelled — no config saved".into())); | ||
| } | ||
|
|
||
| let written = save_config(&doc, answers.scope, &cwd, &home, agent_hint)?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== write_or_merge full body ==="
ast-grep run --pattern 'fn write_or_merge($$$) { $$$ }' --lang rust crates/cli/src/commands/configure/model.rs
echo "=== build_config full body ==="
ast-grep run --pattern 'fn build_config($$$) { $$$ }' --lang rust crates/cli/src/commands/configure/model.rs
echo "=== every save_config call site ==="
rg -nP --type=rust -C 4 '\bsave_config\s*\(' crates/cli
echo "=== tests that assert non-agent sections survive a wizard re-run ==="
rg -nP --type=rust -C 5 'upstream|gateway|logging' crates/cli/src/commands/configure/wizard/tests.rs 2>/dev/null || \
fd -t f 'tests.rs' crates/cli/src/commands/configure --exec rg -nP -C 5 'write_or_merge|save_config' {}Repository: NVIDIA/NeMo-Relay
Length of output: 188
🏁 Script executed:
set -u
echo "=== candidate files ==="
fd -t f . crates/cli/src/commands/configure | sort
echo "=== model outline ==="
ast-grep outline crates/cli/src/commands/configure/model.rs --lang rust 2>&1 || true
echo "=== wizard prompt outline ==="
ast-grep outline crates/cli/src/commands/configure/wizard/prompt.rs --lang rust 2>&1 || true
echo "=== relevant model symbols ==="
rg -n -C 8 'write_or_merge|build_config|save_config|merge_scope|ConfigDocument|toml' crates/cli/src/commands/configure/model.rs
echo "=== save_config call sites ==="
rg -n -C 6 '\bsave_config\s*\(' crates/cli/src/commands/configure
echo "=== configure tests ==="
rg -n -C 5 'write_or_merge|save_config|upstream|gateway|logging|agents' crates/cli --glob '*.rs' | head -400Repository: NVIDIA/NeMo-Relay
Length of output: 50373
🏁 Script executed:
set -u
echo "=== wizard run and agent_hint construction ==="
sed -n '1,125p' crates/cli/src/commands/configure/wizard/prompt.rs
echo "=== configure command dispatch ==="
rg -n -C 8 'prompt_user|wizard|agent_hint|run\(' crates/cli/src/commands/configure crates/cli/src/commands/configure.rs 2>/dev/null || true
echo "=== focused model implementation ==="
sed -n '100,230p' crates/cli/src/commands/configure/model.rs
echo "=== all configure-related test files and symbols ==="
fd -t f -e rs . crates/cli | rg 'configure|wizard|model' | sort
rg -n -C 4 'build_config|write_or_merge|merge_agents_entry|save_config|agent_hint|ConfigScope' crates/cli --glob '*.rs' --glob '*test*' | head -300Repository: NVIDIA/NeMo-Relay
Length of output: 50373
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import tomllib
model = Path("crates/cli/src/commands/configure/model.rs").read_text()
prompt = Path("crates/cli/src/commands/configure/wizard/prompt.rs").read_text()
configure = Path("crates/cli/src/commands/configure/mod.rs").read_text()
build = model[model.index("pub(crate) fn build_config"):model.index("pub(crate) fn build_agents_table")]
write = model[model.index("pub(crate) fn write_or_merge"):model.index("// Replaces the single")]
run = prompt[prompt.index("pub(super) async fn run"):prompt.index("/// After the base config")]
dispatch = configure[configure.index("pub(crate) async fn run"):]
assert 'doc["agents"]' in build
assert all(section not in build for section in ('"upstream"', '"gateway"', '"logging"'))
assert 'let Some(agent) = merge_scope else' in write
assert 'std::fs::write(path, doc.to_string())?;' in write
assert 'return Ok(());' in write
assert 'save_config(&doc, answers.scope, &cwd, &home, agent_hint)?' in run
assert 'wizard::run(agent, explicit_plugin_path).await?' in dispatch
existing = """[agents.claude]
command = "claude"
[upstream]
openai_base_url = "https://example.test"
[gateway]
bind = "127.0.0.1:8080"
[logging]
level = "debug"
"""
wizard_doc = """[agents]
[agents.codex]
command = "codex"
"""
# This models the exact None branch: write the wizard document instead of merging.
result = wizard_doc
before = tomllib.loads(existing)
after = tomllib.loads(result)
assert set(before) == {"agents", "upstream", "gateway", "logging"}
assert set(after) == {"agents"}
assert {"upstream", "gateway", "logging"} - set(after) == {"upstream", "gateway", "logging"}
print("None merge_scope overwrites an existing config with the agents-only wizard document.")
print("Dropped sections:", ", ".join(sorted(set(before) - set(after))))
print("Unhinted config path passes agent_hint=None to save_config.")
PYRepository: NVIDIA/NeMo-Relay
Length of output: 275
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import re
model = Path("crates/cli/src/commands/configure/model.rs").read_text()
prompt = Path("crates/cli/src/commands/configure/wizard/prompt.rs").read_text()
configure = Path("crates/cli/src/commands/configure/mod.rs").read_text()
build = model[model.index("pub(crate) fn build_config"):model.index("pub(crate) fn build_agents_table")]
write = model[model.index("pub(crate) fn write_or_merge"):model.index("// Replaces the single")]
run = prompt[prompt.index("pub(super) async fn run"):prompt.index("/// After the base config")]
dispatch = configure[configure.index("pub(crate) async fn run"):]
assert 'doc["agents"]' in build
assert all(section not in build for section in ('"upstream"', '"gateway"', '"logging"'))
assert 'let Some(agent) = merge_scope else' in write
assert 'std::fs::write(path, doc.to_string())?;' in write
assert 'return Ok(());' in write
assert 'save_config(&doc, answers.scope, &cwd, &home, agent_hint)?' in run
assert 'wizard::run(agent, explicit_plugin_path).await?' in dispatch
existing = """[agents.claude]
command = "claude"
[upstream]
openai_base_url = "https://example.test"
[gateway]
bind = "127.0.0.1:8080"
[logging]
level = "debug"
"""
wizard_doc = """[agents]
[agents.codex]
command = "codex"
"""
def top_level_tables(toml_text):
return {
match.group(1)
for match in re.finditer(r"(?m)^\[([A-Za-z0-9_-]+)(?:\.[^\]]+)?\]", toml_text)
}
before = top_level_tables(existing)
after = top_level_tables(wizard_doc)
assert before == {"agents", "upstream", "gateway", "logging"}
assert after == {"agents"}
assert {"upstream", "gateway", "logging"} - after == {"upstream", "gateway", "logging"}
print("None merge_scope overwrites an existing config with the agents-only wizard document.")
print("Dropped sections:", ", ".join(sorted(before - after)))
print("Unhinted config path passes agent_hint=None to save_config.")
PYRepository: NVIDIA/NeMo-Relay
Length of output: 261
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import re
model_lines = Path("crates/cli/src/commands/configure/model.rs").read_text().splitlines()
prompt = Path("crates/cli/src/commands/configure/wizard/prompt.rs").read_text()
configure = Path("crates/cli/src/commands/configure/mod.rs").read_text()
def region(lines, start, end):
i = next(i for i, line in enumerate(lines) if start in line)
j = next((j for j in range(i + 1, len(lines)) if end in lines[j]), len(lines))
return "\n".join(lines[i:j])
build = region(model_lines, "pub(crate) fn build_config", "pub(crate) fn build_agents_table")
write = region(model_lines, "pub(crate) fn write_or_merge", "// Replaces the single")
assert 'doc["agents"]' in build
assert all(section not in build for section in ('"upstream"', '"gateway"', '"logging"'))
assert 'let Some(agent) = merge_scope else' in write
assert 'std::fs::write(path, doc.to_string())?;' in write
assert 'return Ok(());' in write
assert 'save_config(&doc, answers.scope, &cwd, &home, agent_hint)?' in prompt
assert 'wizard::run(agent, explicit_plugin_path).await?' in configure
existing = """[agents.claude]
command = "claude"
[upstream]
openai_base_url = "https://example.test"
[gateway]
bind = "127.0.0.1:8080"
[logging]
level = "debug"
"""
wizard_doc = """[agents]
[agents.codex]
command = "codex"
"""
def top_level_tables(text):
return {
m.group(1)
for m in re.finditer(r"(?m)^\[([A-Za-z0-9_-]+)(?:\.[^\]]+)?\]", text)
}
before = top_level_tables(existing)
after = top_level_tables(wizard_doc)
assert before == {"agents", "upstream", "gateway", "logging"}
assert after == {"agents"}
print("None merge_scope overwrites an existing config with the agents-only wizard document.")
print("Dropped sections:", ", ".join(sorted(before - after)))
print("Unhinted config path passes agent_hint=None to save_config.")
PYRepository: NVIDIA/NeMo-Relay
Length of output: 345
Preserve existing sections when agent_hint is None. nemo-relay config passes None to write_or_merge, which overwrites the file with the agents-only document from build_config. A rerun therefore deletes existing [upstream], [gateway], [logging], and other non-agent sections without showing this in the confirmation. Merge non-agent sections or require an explicit destructive confirmation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/cli/src/commands/configure/wizard/prompt.rs` around lines 87 - 94,
Update the configuration save flow around build_config, confirm_summary, and
save_config so agent_hint being None preserves existing non-agent sections
instead of overwriting them. Merge the current file’s sections such as upstream,
gateway, and logging into the document before preview and saving, or require an
explicit destructive confirmation that accurately displays their removal.
| fn ensure_tty() -> Result<(), CliError> { | ||
| if !std::io::stdin().is_terminal() { | ||
| return Err(CliError::Config( | ||
| "interactive setup requires a TTY; pass `--config <path>` or set up \ | ||
| `.nemo-relay/config.toml` manually" | ||
| .into(), | ||
| )); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Three divergent ensure_tty guards now exist across the extracted prompt modules. The extraction created one TTY guard per prompt module. Each checks a different set of streams and emits a different message, with no shared definition, so the checks can drift independently.
Current state:
| Location | Streams checked | Message |
|---|---|---|
crates/cli/src/commands/configure/wizard/prompt.rs L180 |
stdin | "interactive setup requires a TTY; pass --config <path> ..." |
crates/cli/src/commands/configure/editor/prompt.rs L59 |
stdin | "interactive configuration editing requires a TTY" |
crates/cli/src/plugins/prompt.rs L290-292 |
stdin, stdout, stderr | "interactive plugin editing requires a TTY" |
All three modules render menus through dialoguer, which writes to the terminal. The two stdin-only guards accept a session where stdout or stderr is redirected. In that case the menu output disappears while the prompt still blocks on input.
Both stdin-only variants match their pre-refactor behavior, so neither is a regression. The duplication is new.
crates/cli/src/commands/configure/wizard/prompt.rs#L179-L188: replace the inline body with a call to a shared helper that takes the required streams and the message.crates/cli/src/commands/configure/editor/prompt.rs#L58-L60: keepensure_tty_withfor its testability, and route it through the same shared helper so the stream set is declared in one place.
♻️ Sketch of a shared guard
// crates/cli/src/tty.rs
use std::io::IsTerminal;
#[derive(Clone, Copy)]
pub(crate) struct TtyRequirement {
pub stdin: bool,
pub stdout: bool,
pub stderr: bool,
}
impl TtyRequirement {
pub(crate) const INTERACTIVE_MENU: Self = Self { stdin: true, stdout: true, stderr: true };
pub(crate) fn is_satisfied(self) -> bool {
(!self.stdin || std::io::stdin().is_terminal())
&& (!self.stdout || std::io::stdout().is_terminal())
&& (!self.stderr || std::io::stderr().is_terminal())
}
}
pub(crate) fn ensure_tty(requirement: TtyRequirement, message: &str) -> Result<(), CliError> {
if requirement.is_satisfied() {
return Ok(());
}
Err(CliError::Config(message.to_owned()))
}#!/bin/bash
set -euo pipefail
echo "=== every TTY guard in the CLI crate ==="
rg -nP --type=rust -B 2 -A 12 '\bfn ensure_tty(_with)?\s*\(' crates/cli/src
echo "=== every is_terminal() call site ==="
rg -nP --type=rust -C 3 '\bis_terminal\s*\(\)' crates/cli/src
echo "=== tests pinning the exact guard messages ==="
rg -nP --type=rust -C 3 'requires a TTY' crates/cli📍 Affects 2 files
crates/cli/src/commands/configure/wizard/prompt.rs#L179-L188(this comment)crates/cli/src/commands/configure/editor/prompt.rs#L58-L60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/cli/src/commands/configure/wizard/prompt.rs` around lines 179 - 188,
Unify the duplicated TTY validation through a shared helper in the CLI TTY
module. In crates/cli/src/commands/configure/wizard/prompt.rs lines 179-188,
replace ensure_tty’s inline stdin-only check with the shared helper using the
interactive menu stream requirement and preserve its message; in
crates/cli/src/commands/configure/editor/prompt.rs lines 58-60, retain
ensure_tty_with for testability but delegate it to the same helper with its
required streams and message. Ensure the existing plugins prompt guard also uses
this shared definition.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Third duplicated ensure_tty implementation.
This PR now has three separate ensure_tty functions:
crates/cli/src/commands/configure/editor/prompt.rsline 58 — delegates toensure_tty_with(stdin.is_terminal()).crates/cli/src/plugins/prompt.rsline 289 — checks stdin, stdout, and stderr.- This one, line 180 — checks stdin only.
Each carries a different message and a different set of stream checks. Consider one shared helper that takes the required streams and the error message, so the checks cannot drift further.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/cli/src/commands/configure/wizard/prompt.rs` around lines 179 - 188,
Consolidate the duplicated ensure_tty implementations by introducing one shared
helper that accepts the required stream-readiness checks and error message.
Update the configure editor prompt, plugin prompt, and wizard prompt callers to
use it, preserving each caller’s required stdin/stdout/stderr checks and
user-facing message while removing their local ensure_tty definitions.
| pub(super) fn edit_dynamic_plugin( | ||
| theme: &ColorfulTheme, | ||
| state: &mut DynamicPluginEditorState, | ||
| ) -> Result<(), CliError> { | ||
| if let Some(description) = &state.description { | ||
| println!(" {}", single_line_text(description)); | ||
| } | ||
| let fields = state | ||
| .schema | ||
| .as_ref() | ||
| .map(|schema| schema.fields().to_vec()) | ||
| .unwrap_or_default(); | ||
| if state.schema.is_none() || fields.is_empty() { | ||
| edit_dynamic_root_menu(theme, state, &fields) | ||
| } else { | ||
| let prompt = state | ||
| .editor_title | ||
| .clone() | ||
| .unwrap_or_else(|| state.label.clone()); | ||
| edit_dynamic_fields_menu(theme, state, &fields, &[], prompt) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Redundant condition at line 26.
fields is built at lines 21-25 from state.schema. When state.schema is None, unwrap_or_default() yields an empty vector. So fields.is_empty() is already true whenever state.schema.is_none() is true. The first operand never changes the result.
Keeping it is harmless and arguably documents intent. Reduce to fields.is_empty() if you prefer the minimal form.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/cli/src/plugins/dynamic_editor/prompt.rs` around lines 14 - 35, The
condition in edit_dynamic_plugin redundantly checks state.schema.is_none()
because fields is empty in that case; simplify the menu selection condition to
fields.is_empty(), preserving the existing root-menu and fields-menu behavior.
| #[test] | ||
| fn lifecycle_commands_cover_json_and_human_output_paths() { | ||
| let temp = tempfile::tempdir().unwrap(); | ||
| let _env = EnvScope::hermetic(&temp); | ||
| let _cwd = CurrentDirGuard::enter(temp.path()); | ||
| let server = GatewayOverrides::default(); | ||
|
|
||
| list( | ||
| PluginsListRequest { | ||
| all: false, | ||
| json: false, | ||
| }, | ||
| &server, | ||
| ) | ||
| .unwrap(); | ||
| list( | ||
| PluginsListRequest { | ||
| all: false, | ||
| json: true, | ||
| }, | ||
| &server, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let plugin_dir = temp.path().join("output-plugin"); | ||
| std::fs::create_dir_all(&plugin_dir).unwrap(); | ||
| let manifest_path = write_dynamic_manifest(&plugin_dir, "acme.output"); | ||
| add( | ||
| PluginsAddRequest { | ||
| scope: ConfigurationScope::Project, | ||
| path: plugin_dir, | ||
| }, | ||
| &server, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| validate( | ||
| PluginsValidateRequest { | ||
| target: manifest_path.display().to_string(), | ||
| json: false, | ||
| }, | ||
| &server, | ||
| ) | ||
| .unwrap(); | ||
| validate( | ||
| PluginsValidateRequest { | ||
| target: manifest_path.display().to_string(), | ||
| json: true, | ||
| }, | ||
| &server, | ||
| ) | ||
| .unwrap(); | ||
| validate( | ||
| PluginsValidateRequest { | ||
| target: "acme.output".into(), | ||
| json: false, | ||
| }, | ||
| &server, | ||
| ) | ||
| .unwrap(); | ||
| validate( | ||
| PluginsValidateRequest { | ||
| target: "acme.output".into(), | ||
| json: true, | ||
| }, | ||
| &server, | ||
| ) | ||
| .unwrap(); | ||
| list( | ||
| PluginsListRequest { | ||
| all: false, | ||
| json: false, | ||
| }, | ||
| &server, | ||
| ) | ||
| .unwrap(); | ||
| list( | ||
| PluginsListRequest { | ||
| all: false, | ||
| json: true, | ||
| }, | ||
| &server, | ||
| ) | ||
| .unwrap(); | ||
| inspect( | ||
| PluginsInspectRequest { | ||
| id: "acme.output".into(), | ||
| json: false, | ||
| }, | ||
| &server, | ||
| ) | ||
| .unwrap(); | ||
| inspect( | ||
| PluginsInspectRequest { | ||
| id: "acme.output".into(), | ||
| json: true, | ||
| }, | ||
| &server, | ||
| ) | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add content assertions to lifecycle_commands_cover_json_and_human_output_paths.
This test calls list, add, validate, and inspect in both human and JSON output modes, but every call only checks .unwrap(). No assertion verifies the JSON envelope fields (schema_version, ok, command, data) or the human-readable output content. A regression that breaks the JSON schema or omits a field for these commands would not fail this test.
Add assertions similar to other tests in this file, for example parse the JSON output and check parsed["ok"], parsed["command"], and a key field under parsed["data"] for each call.
As per path instructions, "Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/cli/tests/coverage/shared/plugins_lifecycle_tests.rs` around lines
4417 - 4517, Strengthen lifecycle_commands_cover_json_and_human_output_paths by
capturing each list, add, validate, and inspect result instead of only calling
unwrap. For JSON responses, parse the output and assert schema_version, ok,
command, and a representative field under data; for human responses, assert
meaningful command-specific text. Follow assertion patterns used elsewhere in
this test file and include lifecycle-event or scope-stack behavior where
available rather than relying only on successful completion.
Source: Path instructions
| fn native_config_field<'a>(schema: &'a PluginConfigSchema, key: &str) -> &'a DynamicConfigField { | ||
| schema | ||
| .fields() | ||
| .iter() | ||
| .find(|field| field.key == key) | ||
| .unwrap() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Name the missing key in the panic message.
This helper is now the single lookup point for roughly fifteen field assertions across assert_native_raw_and_scalar_fields and assert_native_choice_and_nested_fields. On failure, .unwrap() prints only called Option::unwrap() on a None value with a line number inside the helper, not the key that was missing.
💚 Proposed fix
fn native_config_field<'a>(schema: &'a PluginConfigSchema, key: &str) -> &'a DynamicConfigField {
schema
.fields()
.iter()
.find(|field| field.key == key)
- .unwrap()
+ .unwrap_or_else(|| {
+ panic!(
+ "schema has no field {key:?}; available: {:?}",
+ schema.fields().iter().map(|f| &f.key).collect::<Vec<_>>()
+ )
+ })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn native_config_field<'a>(schema: &'a PluginConfigSchema, key: &str) -> &'a DynamicConfigField { | |
| schema | |
| .fields() | |
| .iter() | |
| .find(|field| field.key == key) | |
| .unwrap() | |
| } | |
| fn native_config_field<'a>(schema: &'a PluginConfigSchema, key: &str) -> &'a DynamicConfigField { | |
| schema | |
| .fields() | |
| .iter() | |
| .find(|field| field.key == key) | |
| .unwrap_or_else(|| { | |
| panic!( | |
| "schema has no field {key:?}; available: {:?}", | |
| schema.fields().iter().map(|f| &f.key).collect::<Vec<_>>() | |
| ) | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/cli/tests/coverage/shared/plugins_schema_tests.rs` around lines 280 -
286, Update native_config_field to replace the bare unwrap with an expectation
that includes the requested key, so lookup failures identify which schema field
is missing while preserving the existing return behavior.
| #[test] | ||
| fn menu_keys_cover_selection_shortcuts_and_cancellation() { | ||
| assert_eq!( | ||
| menu_response_for_key(&Key::Enter, 2), | ||
| Some(MenuResponse::Selected(2)) | ||
| ); | ||
| assert_eq!( | ||
| menu_response_for_key(&Key::Char(' '), 3), | ||
| Some(MenuResponse::Selected(3)) | ||
| ); | ||
| assert_eq!( | ||
| menu_response_for_key(&Key::Char('p'), 1), | ||
| Some(MenuResponse::Shortcut(MenuShortcut::Preview, 1)) | ||
| ); | ||
| assert_eq!( | ||
| menu_response_for_key(&Key::Char('s'), 1), | ||
| Some(MenuResponse::Shortcut(MenuShortcut::Save, 1)) | ||
| ); | ||
| assert_eq!( | ||
| menu_response_for_key(&Key::Char('r'), 1), | ||
| Some(MenuResponse::Shortcut(MenuShortcut::Reset, 1)) | ||
| ); | ||
| assert_eq!( | ||
| menu_response_for_key(&Key::Backspace, 1), | ||
| Some(MenuResponse::Shortcut(MenuShortcut::Clear, 1)) | ||
| ); | ||
| assert_eq!( | ||
| menu_response_for_key(&Key::Char('?'), 1), | ||
| Some(MenuResponse::Shortcut(MenuShortcut::Help, 1)) | ||
| ); | ||
| assert_eq!( | ||
| menu_response_for_key(&Key::Escape, 1), | ||
| Some(MenuResponse::Cancel) | ||
| ); | ||
| assert_eq!(menu_response_for_key(&Key::Char('x'), 1), None); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The cancellation coverage is incomplete for the mapping under test.
menu_response_for_key in crates/cli/src/plugins/mod.rs lines 235-246 defines three multi-key arms. This test exercises only one key from two of them:
- Line 241 maps
Key::Backspace | Key::DeltoClear. The test coversBackspaceat line 622.Delis untested. - Line 243 maps
Key::Escape | Key::CtrlC | Key::Char('q')toCancel. The test coversEscapeat line 630.CtrlCandChar('q')are untested.
Key::CtrlC is the primary cancellation path for a terminal UI. If someone reorders that arm so CtrlC falls through to _ => None, prompt_menu at crates/cli/src/plugins/prompt.rs lines 229-232 would ignore Ctrl-C and loop, and this test would still pass.
Key::Char('q') is documented to the user in the help text at crates/cli/src/plugins/prompt.rs line 286 ("q or Esc go back/cancel"), so it is a promised behavior.
💚 Proposed additions
assert_eq!(
menu_response_for_key(&Key::Backspace, 1),
Some(MenuResponse::Shortcut(MenuShortcut::Clear, 1))
);
+ assert_eq!(
+ menu_response_for_key(&Key::Del, 1),
+ Some(MenuResponse::Shortcut(MenuShortcut::Clear, 1))
+ );
assert_eq!(
menu_response_for_key(&Key::Char('?'), 1),
Some(MenuResponse::Shortcut(MenuShortcut::Help, 1))
);
assert_eq!(
menu_response_for_key(&Key::Escape, 1),
Some(MenuResponse::Cancel)
);
+ assert_eq!(
+ menu_response_for_key(&Key::CtrlC, 1),
+ Some(MenuResponse::Cancel)
+ );
+ assert_eq!(
+ menu_response_for_key(&Key::Char('q'), 1),
+ Some(MenuResponse::Cancel)
+ );
assert_eq!(menu_response_for_key(&Key::Char('x'), 1), None);As per path instructions: "Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn menu_keys_cover_selection_shortcuts_and_cancellation() { | |
| assert_eq!( | |
| menu_response_for_key(&Key::Enter, 2), | |
| Some(MenuResponse::Selected(2)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Char(' '), 3), | |
| Some(MenuResponse::Selected(3)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Char('p'), 1), | |
| Some(MenuResponse::Shortcut(MenuShortcut::Preview, 1)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Char('s'), 1), | |
| Some(MenuResponse::Shortcut(MenuShortcut::Save, 1)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Char('r'), 1), | |
| Some(MenuResponse::Shortcut(MenuShortcut::Reset, 1)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Backspace, 1), | |
| Some(MenuResponse::Shortcut(MenuShortcut::Clear, 1)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Char('?'), 1), | |
| Some(MenuResponse::Shortcut(MenuShortcut::Help, 1)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Escape, 1), | |
| Some(MenuResponse::Cancel) | |
| ); | |
| assert_eq!(menu_response_for_key(&Key::Char('x'), 1), None); | |
| } | |
| #[test] | |
| fn menu_keys_cover_selection_shortcuts_and_cancellation() { | |
| assert_eq!( | |
| menu_response_for_key(&Key::Enter, 2), | |
| Some(MenuResponse::Selected(2)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Char(' '), 3), | |
| Some(MenuResponse::Selected(3)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Char('p'), 1), | |
| Some(MenuResponse::Shortcut(MenuShortcut::Preview, 1)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Char('s'), 1), | |
| Some(MenuResponse::Shortcut(MenuShortcut::Save, 1)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Char('r'), 1), | |
| Some(MenuResponse::Shortcut(MenuShortcut::Reset, 1)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Backspace, 1), | |
| Some(MenuResponse::Shortcut(MenuShortcut::Clear, 1)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Del, 1), | |
| Some(MenuResponse::Shortcut(MenuShortcut::Clear, 1)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Char('?'), 1), | |
| Some(MenuResponse::Shortcut(MenuShortcut::Help, 1)) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Escape, 1), | |
| Some(MenuResponse::Cancel) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::CtrlC, 1), | |
| Some(MenuResponse::Cancel) | |
| ); | |
| assert_eq!( | |
| menu_response_for_key(&Key::Char('q'), 1), | |
| Some(MenuResponse::Cancel) | |
| ); | |
| assert_eq!(menu_response_for_key(&Key::Char('x'), 1), None); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/cli/tests/coverage/shared/plugins_tests.rs` around lines 599 - 634,
Expand menu_keys_cover_selection_shortcuts_and_cancellation to assert every key
in the multi-key mappings handled by menu_response_for_key: add Del as Clear,
and CtrlC plus Char('q') as Cancel. Keep the existing assertions and expected
response indices unchanged.
Source: Path instructions
| #[test] | ||
| fn component_shortcut_fallbacks_are_safe_noops() { | ||
| let mut config = PluginConfig::default(); | ||
| ensure_observability_component(&mut config).unwrap(); | ||
| let mut components = editable_components(&config).unwrap(); | ||
|
|
||
| assert_eq!( | ||
| handle_reset_or_clear_shortcut(&mut components, None, MenuShortcut::Reset).unwrap(), | ||
| EditLoopControl::Continue | ||
| ); | ||
| reset_component_menu_item(&mut components[0], None).unwrap(); | ||
| clear_component_menu_item(&mut components[0], Some(ComponentMenuAction::Back)).unwrap(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This test never asserts the no-op it is named for.
The test is called component_shortcut_fallbacks_are_safe_noops. Lines 756 and 757 call reset_component_menu_item(&mut components[0], None) and clear_component_menu_item(&mut components[0], Some(ComponentMenuAction::Back)) and only unwrap the result.
Nothing checks that components[0] is unchanged. A regression that makes the None action reset the component, or makes Back clear a field, still returns Ok and still passes.
Capture the component state before the calls and compare after.
💚 Proposed fix
assert_eq!(
handle_reset_or_clear_shortcut(&mut components, None, MenuShortcut::Reset).unwrap(),
EditLoopControl::Continue
);
+ let before = config_with_editable_components(&config, &components).unwrap();
reset_component_menu_item(&mut components[0], None).unwrap();
clear_component_menu_item(&mut components[0], Some(ComponentMenuAction::Back)).unwrap();
+ let after = config_with_editable_components(&config, &components).unwrap();
+ assert_eq!(
+ serde_json::to_value(before).unwrap(),
+ serde_json::to_value(after).unwrap(),
+ "fallback shortcut actions must not mutate component state"
+ );
}As per path instructions: "Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn component_shortcut_fallbacks_are_safe_noops() { | |
| let mut config = PluginConfig::default(); | |
| ensure_observability_component(&mut config).unwrap(); | |
| let mut components = editable_components(&config).unwrap(); | |
| assert_eq!( | |
| handle_reset_or_clear_shortcut(&mut components, None, MenuShortcut::Reset).unwrap(), | |
| EditLoopControl::Continue | |
| ); | |
| reset_component_menu_item(&mut components[0], None).unwrap(); | |
| clear_component_menu_item(&mut components[0], Some(ComponentMenuAction::Back)).unwrap(); | |
| } | |
| #[test] | |
| fn component_shortcut_fallbacks_are_safe_noops() { | |
| let mut config = PluginConfig::default(); | |
| ensure_observability_component(&mut config).unwrap(); | |
| let mut components = editable_components(&config).unwrap(); | |
| assert_eq!( | |
| handle_reset_or_clear_shortcut(&mut components, None, MenuShortcut::Reset).unwrap(), | |
| EditLoopControl::Continue | |
| ); | |
| let before = config_with_editable_components(&config, &components).unwrap(); | |
| reset_component_menu_item(&mut components[0], None).unwrap(); | |
| clear_component_menu_item(&mut components[0], Some(ComponentMenuAction::Back)).unwrap(); | |
| let after = config_with_editable_components(&config, &components).unwrap(); | |
| assert_eq!( | |
| serde_json::to_value(before).unwrap(), | |
| serde_json::to_value(after).unwrap(), | |
| "fallback shortcut actions must not mutate component state" | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/cli/tests/coverage/shared/plugins_tests.rs` around lines 746 - 758,
Strengthen component_shortcut_fallbacks_are_safe_noops by capturing
components[0] before calling reset_component_menu_item and
clear_component_menu_item, then asserting it remains unchanged afterward. Keep
the existing successful-result checks and verify both None and Back fallback
actions preserve the component state.
Source: Path instructions
| #[test] | ||
| fn dynamic_editor_raw_menu_and_nested_value_paths_are_deterministic() { | ||
| let temp = tempfile::tempdir().unwrap(); | ||
| write_editor_dynamic_manifest(&temp.path().join("plugin"), "acme.raw-menu", None, None); | ||
| let path = temp.path().join("plugins.toml"); | ||
| std::fs::write( | ||
| &path, | ||
| "[[plugins.dynamic]]\nmanifest = \"./plugin/relay-plugin.toml\"\n", | ||
| ) | ||
| .unwrap(); | ||
| let document = PluginConfigDocument::read(&path).unwrap(); | ||
| let states = load_dynamic_plugin_states(&document).unwrap(); | ||
| let (items, actions) = dynamic_root_menu_items(&states[0], &[]); | ||
| assert_eq!(items.len(), 3); | ||
| assert!(matches!(actions[0], DynamicMenuAction::EditRawConfig)); | ||
|
|
||
| let mut config = None; | ||
| let path = vec!["outer".to_owned(), "inner".to_owned()]; | ||
| set_value_at_path(&mut config, &path, json!(7)); | ||
| assert_eq!(value_at_path(config.as_ref(), &path), Some(&json!(7))); | ||
| assert_eq!(value_at_path(config.as_ref(), &[]), None); | ||
| assert!(remove_value_at_path(config.as_mut().unwrap(), &path)); | ||
| set_value_at_path(&mut config, &[], json!(9)); | ||
| assert!(config.as_ref().unwrap().is_empty()); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Shadowed path binding with two different types.
Line 2156 binds path to the plugins.toml PathBuf. Line 2169 rebinds path to a Vec<String> field path. Both are live in the same function body.
The code is correct. The name reuse makes lines 2170-2174 harder to read, because path there means a JSON field path, not a filesystem path.
Rename the second binding to field_path.
The error-path coverage in this block is good: line 2172 checks the empty-path lookup, and line 2174 checks the empty-path write no-op.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/cli/tests/coverage/shared/plugins_tests.rs` around lines 2152 - 2176,
Rename the second `path` binding in
`dynamic_editor_raw_menu_and_nested_value_paths_are_deterministic` to
`field_path`, and update the related `set_value_at_path`, `value_at_path`, and
`remove_value_at_path` calls to use it; leave the filesystem `path` binding
unchanged.
1 participant
Overview
Raises the CLI component coverage target above 92 percent while excluding prompt-only logic that cannot be exercised reliably in automated coverage runs.
Details
Validation performed:
Where should the reviewer start?
Start with codecov.yml and crates/cli/src/commands/configure/editor.rs, then compare its prompt module and crates/cli/tests/coverage/commands/configure_editor_tests.rs.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Bug Fixes