feat: refresh setup wizard to chat-like dialoguer flow - #113
Merged
Conversation
…preservation in save_config
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthroughegopulse setup が dialoguer ベースの順次プロンプトへ置き換わり、入力検証、既存設定の再編集、保存、レビュー表示、完了メッセージ、関連ドキュメントが更新された。 ChangesSetup prompt refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 9
🧹 Nitpick comments (9)
src/setup/provider.rs (2)
256-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
normalize_provider_idに doc comment を付けたい既知 preset だけ小文字化して未知値はそのまま返す、ってルールがここだけで完結してる。保存/検証側がこの前提に乗る関数なので、意図をコメントで残しておきたい。
As per coding guidelines,**/*.rs: All public items must include doc comments;# Errors,# Panics, and# Safetysections are required where applicable.🤖 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 `@src/setup/provider.rs` around lines 256 - 264, Add a doc comment to normalize_provider_id explaining that it trims input, returns an empty string for blank values, lowercases only known provider presets via find_provider_preset, and preserves unknown values as-is. Keep the comment close to the function so the save/validation behavior is documented, and ensure it satisfies the Rust doc-comment requirement for this public-facing item.Source: Coding guidelines
268-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winテストを AAA でそろえたい
今は Arrange / Act / Assert が一段に詰まってて、ケース追加のときに読み筋を追いづらい。空行かコメントで 3 段に分けておくと保守しやすい。
As per coding guidelines,**/*.rs: All tests must follow the AAA (Arrange-Act-Assert) pattern.🤖 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 `@src/setup/provider.rs` around lines 268 - 284, The tests in mod tests need to be rewritten to follow the AAA pattern instead of having setup, call, and assertion on one line. Update find_provider_preset_matches_known_id, find_provider_preset_returns_none_for_unknown, and normalize_provider_id_lowercases_known_preset so each test clearly separates Arrange, Act, and Assert using whitespace or comments. Keep the same coverage, but make the flow explicit and consistent with the project’s test style.Source: Coding guidelines
src/setup/channels.rs (2)
33-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
build_channel_configsに doc comment がほしい
web_enabledで omit する契約と secret ref 化の前提がこの関数の責務として大きいので、公開スコープの入口には説明を置いておきたい。
As per coding guidelines,**/*.rs: All public items must include doc comments;# Errors,# Panics, and# Safetysections are required where applicable.🤖 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 `@src/setup/channels.rs` around lines 33 - 39, Add a doc comment for build_channel_configs because it is a public-facing entry point and must follow the Rust doc-comment guideline for public items. Describe the function’s responsibility, especially the web_enabled omission behavior and the secret-ref-related assumptions, and include any applicable # Errors, # Panics, or # Safety sections if relevant. Keep the comment attached to build_channel_configs so it remains discoverable even if the signature changes.Source: Coding guidelines
170-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winテストを AAA でそろえたい
有効/無効の分岐テストが増えていく場所なので、Arrange / Act / Assert を分けておくと差分がかなり読みやすくなる。
As per coding guidelines,**/*.rs: All tests must follow the AAA (Arrange-Act-Assert) pattern.🤖 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 `@src/setup/channels.rs` around lines 170 - 233, The tests in build_channel_configs should be rewritten to follow AAA so each case clearly separates setup, the call to build_channel_configs, and the assertions. In the mod tests block, update the four test functions to first Arrange inputs and expected state, then Act by invoking build_channel_configs, and finally Assert on the returned channel map and token serialization. Keep the existing coverage and use the current identifiers like build_channel_configs, build_channel_configs_stores_channel_secrets_as_env_refs, and build_channel_configs_includes_web_when_enabled to locate the test cases.Source: Coding guidelines
src/setup/inputs.rs (1)
89-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winテストを AAA でそろえたい
検証ケースが増えやすいモジュールだから、なおさら Arrange / Act / Assert を明示しておいたほうが追いやすい。今のうちに形を揃えておきたい。
As per coding guidelines,**/*.rs: All tests must follow the AAA (Arrange-Act-Assert) pattern.🤖 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 `@src/setup/inputs.rs` around lines 89 - 149, The tests in the SetupInputs validation module are written without an explicit Arrange-Act-Assert structure, which violates the test style guideline. Update each test in the validate_inputs test module to clearly separate setup of inputs, the call to validate_inputs, and the assertion on the result. Keep the existing test names and use the existing helpers like valid_inputs and validate_inputs so the AAA flow is obvious and consistent across all cases.Source: Coding guidelines
src/setup/prompts.rs (1)
305-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winテストを AAA でそろえたい
このファイルは入出力 abstraction の土台なので、テストも Arrange / Act / Assert を明示して読みやすくしておきたい。
As per coding guidelines,**/*.rs: All tests must follow the AAA (Arrange-Act-Assert) pattern.🤖 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 `@src/setup/prompts.rs` around lines 305 - 319, The tests in the `tests` module for `format_api_key_for_review` should be rewritten to follow the AAA pattern. Update each test (`format_api_key_for_review_masks_long_values` and `format_api_key_for_review_shows_empty_for_blank`) so the input setup is clearly separated as Arrange, the function call as Act, and the assertion as Assert, keeping the test names and the `format_api_key_for_review` helper as the main reference points.Source: Coding guidelines
src/setup/slugify.rs (1)
41-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winテストを AAA でそろえたい
このままでも動くけど、ケースが増えると Arrange / Act / Assert の境目が埋もれやすい。ここも 3 段に分けておくと揃って読みやすい。
As per coding guidelines,**/*.rs: All tests must follow the AAA (Arrange-Act-Assert) pattern.🤖 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 `@src/setup/slugify.rs` around lines 41 - 76, The tests in the slugify_agent_id test module are written as direct asserts and should be refactored to follow the AAA pattern. Update each test case in the tests module under slugify_agent_id to clearly separate Arrange, Act, and Assert sections, keeping the existing coverage but making the setup, function call, and expectation explicit and consistent across all cases.Source: Coding guidelines
src/setup/wizard.rs (1)
515-850: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winテストを AAA でそろえたい
この規模の統合テスト群は Arrange / Act / Assert を分けるだけで流れがかなり追いやすくなる。今のうちにパターンを固定しておきたい。
As per coding guidelines,**/*.rs: All tests must follow the AAA (Arrange-Act-Assert) pattern.🤖 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 `@src/setup/wizard.rs` around lines 515 - 850, The tests in the `tests` module are not consistently structured as AAA, so refactor each test to clearly separate setup, execution, and verification. For the affected test cases like `prefill_defaults_uses_existing_config_values`, `wizard_review_startover_returns_to_q1`, and `wizard_parse_error_accept_continues`, keep all fixture and mock setup in Arrange, call `run_with_source_and_sink` or the target helper in Act, and move all `assert_*` checks and output verification into Assert. Use the existing helpers such as `setup_happy_path`, `assert_config_saved`, and `build_done_message` to keep the flow explicit.Source: Coding guidelines
src/setup/summary.rs (1)
372-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ExistingConfigのフィールド公開範囲と docs を絞って
pub(crate)型の中で barepubフィールドになっていて、フィールド単位の doc comment もないよ。crate 内利用ならpub(crate)に揃えて、フィールドにも説明を付けるのが安全。🐰 修正案
pub(crate) struct ExistingConfig { - pub fields: HashMap<String, String>, - pub root: Option<yaml_serde::Value>, + /// ウィザードの事前入力に使う、プロンプト項目名ベースの平坦な既存値。 + pub(crate) fields: HashMap<String, String>, + /// 既存 YAML のルートノード。`state_root` などの保持に使う。 + pub(crate) root: Option<yaml_serde::Value>, }As per coding guidelines,
All public items must include doc commentsandMinimize public API scope; start with private, use pub(crate) or pub(super) where appropriate.🤖 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 `@src/setup/summary.rs` around lines 372 - 379, `ExistingConfig` exposes its fields too broadly and the fields lack docs. In `summary.rs`, update the `fields` and `root` members of `ExistingConfig` to use crate-scoped visibility instead of bare public visibility, and add field-level doc comments that describe their roles. Keep the change localized to `ExistingConfig` so the type remains crate-facing while minimizing the public API surface.Source: Coding guidelines
🤖 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 `@docs/config.md`:
- Around line 673-675: The markdown link in the Q2 provider preset reference
points to a non-matching heading anchor, so update the link target to the actual
section heading used in this document or remove the anchor entirely. Check the
table entry in the config documentation that references “Provider” and “§5” and
make the link text/anchor consistent with the real heading name for the preset
section.
In `@docs/plan/plan-setup-refresh.md`:
- Line 425: The count reference for PROVIDER_PRESETS is inconsistent here:
update the wording to match the rest of the documentation and test plan, using
26 instead of 25. Keep the surrounding rationale intact in plan-setup-refresh.md
so the preset count stays aligned with the other references to PROVIDER_PRESETS.
In `@docs/setup-redesign.md`:
- Line 425: `PROVIDER_PRESETS` の件数表記が資料内で不一致になっているので、`PROVIDER_PRESETS`
を参照している説明文の件数を実装とテストの前提に合わせて統一してください。`src/setup/provider.rs` の
`PROVIDER_PRESETS` に基づく正しい件数へ修正し、他の章や関連ドキュメントでも同じ表記になるように揃えてください。
- Line 72: Fix the broken Markdown anchor in the setup redesign note: the TUI
channel item currently points to a non-existent §9 anchor, so update the
reference in the documented TUI channel section to use the existing §8 anchor
instead. Locate the link text in the setup-redesign document near the
`src/channels/tui.rs` mention and replace the fragment target so the reference
resolves correctly.
In `@src/setup/prompts.rs`:
- Around line 14-29: format_api_key_for_review currently reveals short API keys
because it always keeps a 3-char head and 4-char tail, so 4–7 character values
can be reconstructed in review output. Update this function to detect short
inputs and return a fixed masked placeholder instead of preserving characters,
while keeping the existing behavior for longer keys; use
format_api_key_for_review as the main place to apply the masking rule.
In `@src/setup/summary.rs`:
- Around line 74-92: The setup flow in summary handling only creates the default
state root directory, but it can leave the selected existing or custom state
root missing. In the logic around extract_existing_state_root,
default_state_root, and default_workspace_dir, ensure the actual state_root
being saved is created when present, and fall back to creating the default_root
only when no existing/custom root is available. Keep the directory creation near
the state-root resolution path so later writes after save do not fail.
- Around line 417-424: The provider lookup in summary setup should use the
normalized provider ID first, not only the original default_provider value. In
src/setup/summary.rs, update the providers map access near provider_id so it
resolves providers by normalize_provider_id(default_provider) while still
falling back to the original key for backward compatibility. Keep the existing
PROVIDER field assignment using provider_id, and make sure the provider_map
lookup used for BASE_URL and MODEL prefill works with YAML shapes like
default_provider: OpenAI and providers.openai.
In `@src/setup/wizard.rs`:
- Around line 176-209: The prefill flow in extract_prefill and the surrounding
setup wizard logic is not preserving existing provider, base_url, and model
values when rerunning setup. Extend PrefillValues to include provider, restore
the default index for the provider/model selectors from ExistingConfig, and
ensure the base_url step is always editable after provider selection instead of
reusing a preset fixed value. Update the wizard path that uses root_agent_label,
root_web_enabled, and the provider/model selection flow so Enter-only reruns
keep the existing configuration intact, including Azure/Bedrock placeholder
URLs.
- Around line 449-513: `run_with_source_and_sink` and `run` currently return
`Result<_, String>`, which makes setup failures impossible to distinguish
cleanly; replace the ad hoc String errors with a dedicated structured error enum
such as `SetupWizardError` using `thiserror`, and update all fallible paths in
`load_existing`, `collect_inputs`, `save_and_finish`, and the abort branch to
return that type. Keep the error variants explicit for user abort, parse
failure, prompt failure, and save failure, and ensure the enum’s Display text
stays lower-case to match the project convention.
---
Nitpick comments:
In `@src/setup/channels.rs`:
- Around line 33-39: Add a doc comment for build_channel_configs because it is a
public-facing entry point and must follow the Rust doc-comment guideline for
public items. Describe the function’s responsibility, especially the web_enabled
omission behavior and the secret-ref-related assumptions, and include any
applicable # Errors, # Panics, or # Safety sections if relevant. Keep the
comment attached to build_channel_configs so it remains discoverable even if the
signature changes.
- Around line 170-233: The tests in build_channel_configs should be rewritten to
follow AAA so each case clearly separates setup, the call to
build_channel_configs, and the assertions. In the mod tests block, update the
four test functions to first Arrange inputs and expected state, then Act by
invoking build_channel_configs, and finally Assert on the returned channel map
and token serialization. Keep the existing coverage and use the current
identifiers like build_channel_configs,
build_channel_configs_stores_channel_secrets_as_env_refs, and
build_channel_configs_includes_web_when_enabled to locate the test cases.
In `@src/setup/inputs.rs`:
- Around line 89-149: The tests in the SetupInputs validation module are written
without an explicit Arrange-Act-Assert structure, which violates the test style
guideline. Update each test in the validate_inputs test module to clearly
separate setup of inputs, the call to validate_inputs, and the assertion on the
result. Keep the existing test names and use the existing helpers like
valid_inputs and validate_inputs so the AAA flow is obvious and consistent
across all cases.
In `@src/setup/prompts.rs`:
- Around line 305-319: The tests in the `tests` module for
`format_api_key_for_review` should be rewritten to follow the AAA pattern.
Update each test (`format_api_key_for_review_masks_long_values` and
`format_api_key_for_review_shows_empty_for_blank`) so the input setup is clearly
separated as Arrange, the function call as Act, and the assertion as Assert,
keeping the test names and the `format_api_key_for_review` helper as the main
reference points.
In `@src/setup/provider.rs`:
- Around line 256-264: Add a doc comment to normalize_provider_id explaining
that it trims input, returns an empty string for blank values, lowercases only
known provider presets via find_provider_preset, and preserves unknown values
as-is. Keep the comment close to the function so the save/validation behavior is
documented, and ensure it satisfies the Rust doc-comment requirement for this
public-facing item.
- Around line 268-284: The tests in mod tests need to be rewritten to follow the
AAA pattern instead of having setup, call, and assertion on one line. Update
find_provider_preset_matches_known_id,
find_provider_preset_returns_none_for_unknown, and
normalize_provider_id_lowercases_known_preset so each test clearly separates
Arrange, Act, and Assert using whitespace or comments. Keep the same coverage,
but make the flow explicit and consistent with the project’s test style.
In `@src/setup/slugify.rs`:
- Around line 41-76: The tests in the slugify_agent_id test module are written
as direct asserts and should be refactored to follow the AAA pattern. Update
each test case in the tests module under slugify_agent_id to clearly separate
Arrange, Act, and Assert sections, keeping the existing coverage but making the
setup, function call, and expectation explicit and consistent across all cases.
In `@src/setup/summary.rs`:
- Around line 372-379: `ExistingConfig` exposes its fields too broadly and the
fields lack docs. In `summary.rs`, update the `fields` and `root` members of
`ExistingConfig` to use crate-scoped visibility instead of bare public
visibility, and add field-level doc comments that describe their roles. Keep the
change localized to `ExistingConfig` so the type remains crate-facing while
minimizing the public API surface.
In `@src/setup/wizard.rs`:
- Around line 515-850: The tests in the `tests` module are not consistently
structured as AAA, so refactor each test to clearly separate setup, execution,
and verification. For the affected test cases like
`prefill_defaults_uses_existing_config_values`,
`wizard_review_startover_returns_to_q1`, and
`wizard_parse_error_accept_continues`, keep all fixture and mock setup in
Arrange, call `run_with_source_and_sink` or the target helper in Act, and move
all `assert_*` checks and output verification into Assert. Use the existing
helpers such as `setup_happy_path`, `assert_config_saved`, and
`build_done_message` to keep the flow explicit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c598f66-e07e-4314-85dc-51de4c4589ce
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomldocs/commands.mddocs/config.mddocs/plan/plan-setup-refresh.mddocs/setup-redesign.mdsrc/setup/channels.rssrc/setup/inputs.rssrc/setup/mod.rssrc/setup/prompts.rssrc/setup/provider.rssrc/setup/slugify.rssrc/setup/summary.rssrc/setup/wizard.rs
| ### 今回選ぶ項目 | ||
|
|
||
| - 対象: `T27`, `T28`, `T29`, `T30`, `T31`, `T32`, `T33`, `T34` | ||
| - 選ぶ理由: インタラクションロジックのコア。Step 8 の dialoguer 統合だけで回帰を防げない指摘 (codex レビュー指摘2) を受けて、分岐判断も純粋関数化してテスト可能にする |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
PROVIDER_PRESETS の件数表記を揃えて。
ここだけ 25 になってるけど、同じ資料群では 26 前提で揃ってる。数がズレると UI 文言とテスト計画がぶれる。
🤖 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 `@docs/plan/plan-setup-refresh.md` at line 425, The count reference for
PROVIDER_PRESETS is inconsistent here: update the wording to match the rest of
the documentation and test plan, using 26 instead of 25. Keep the surrounding
rationale intact in plan-setup-refresh.md so the preset count stays aligned with
the other references to PROVIDER_PRESETS.
- Validate SetupInputs via validate_inputs right after collect_inputs so invalid combinations (empty base_url, discord without token, etc.) are surfaced before the Review step instead of failing at save time. - Guard prompt_model against empty preset model lists by falling back to free-text input when the preset unexpectedly exposes no models. - Surface 'Setup aborted. No configuration was saved.' on the output sink before returning the Abort error so the user sees explicit feedback.
- prompts: fully mask short (<=7 chars) API keys in Review to prevent reconstruction from the visible head/tail slices. - summary: create the resolved state_root directory and its workspace instead of always creating the default root, so custom state_root paths actually exist when the runtime writes state. - summary: look up the provider map with the normalized provider id so non-canonical ids (e.g. uppercase) resolve consistently. - docs: fix broken anchors in setup-redesign.md (§9 -> §8) and drop the mismatched config.md §5 anchor. - docs: correct PROVIDER_PRESETS count from 25 to 26. - tests: add regression coverage for short-value masking.
Owner
Author
レビュー対応完了対応したレビューコメント: 7件 (commit
対応しなかったコメント: 2件 (別 Issue 対応)
検証
|
The setup module and run_setup_wizard are publicly visible, so their doc comments cannot link to the private wizard module / wizard::run via intra-doc links. Convert them to plain code spans so 'cargo doc' passes under RUSTDOCFLAGS='-D warnings'.
Coderabbit review flagged that PrefillValues had no provider field, so existing provider/base_url/model edits were lost between runs. Re-enable prefill by: - Adding a 'default: usize' parameter to PromptSource::select so callers can position the cursor at the previously chosen item. - Using it in DialoguerPromptSource (dialoguer::Select::default) and recording it in MockPromptSource (select_defaults) for assertion. - Restoring PrefillValues.provider_id and computing the preset index with provider_default_index (custom falls back to the Custom slot). - Computing the model select default from prefill.model when the preset exposes models. - Extending T35 to assert the provider select default matches the existing provider preset (ollama).
Owner
Author
|
@CodeRabbit 再レビューお願いします |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/setup/mod.rs (1)
21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
run_setup_wizardはpub(crate)に絞って# Errorsを足して。提供された文脈だとこの関数は
src/main.rsからしか呼ばれてないから、公開範囲はpub(crate)で足りるはず。あわせてResultを返す公開アイテムなので# Errorsセクションも入れておきたい。🐇 こんな差分でいける
/// Runs the interactive setup wizard and writes the resulting configuration file. /// /// Thin wrapper around `wizard::run` for backwards-compatible entrypoint. -pub async fn run_setup_wizard(config_path: Option<PathBuf>) -> Result<(), EgoPulseError> { +/// +/// # Errors +/// Returns an error when the setup flow fails to prompt, resolve paths, or save config. +pub(crate) async fn run_setup_wizard(config_path: Option<PathBuf>) -> Result<(), EgoPulseError> { wizard::run(config_path).map_err(EgoPulseError::from) }As per coding guidelines, "All public items must include doc comments;
# Errors,# Panics, and# Safetysections are required where applicable" と "Minimize public API scope; start withprivate, usepub(crate)orpub(super)where appropriate, and only usepubfor binary entrypoints or externally required APIs" に合わせてね。🤖 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 `@src/setup/mod.rs` around lines 21 - 25, Restrict `run_setup_wizard` in `src/setup/mod.rs` from `pub` to `pub(crate)` since it is only used internally (e.g. from `src/main.rs`), and update its doc comment to satisfy the public-item docs rule by adding a `# Errors` section that describes the `Result<(), EgoPulseError>` failure cases. Keep the existing `wizard::run` wrapper behavior and `EgoPulseError::from` mapping unchanged.Source: Coding guidelines
🤖 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 `@src/setup/mod.rs`:
- Around line 24-25: `run_setup_wizard` is converting every `wizard::run` error
into `EgoPulseError`, which incorrectly treats the setup wizard’s abort path as
a failure. Update `run_setup_wizard` to special-case `SetupWizardError::Aborted`
from `wizard::run` and return `Ok(())` for that case, while continuing to map
real errors through `EgoPulseError::from`. Keep the fix localized around the
`run_setup_wizard` function and the `wizard::run` error handling flow.
---
Nitpick comments:
In `@src/setup/mod.rs`:
- Around line 21-25: Restrict `run_setup_wizard` in `src/setup/mod.rs` from
`pub` to `pub(crate)` since it is only used internally (e.g. from
`src/main.rs`), and update its doc comment to satisfy the public-item docs rule
by adding a `# Errors` section that describes the `Result<(), EgoPulseError>`
failure cases. Keep the existing `wizard::run` wrapper behavior and
`EgoPulseError::from` mapping unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54c5a1df-af24-4d48-94a2-3227c3f1e0aa
📒 Files selected for processing (6)
src/error.rssrc/main.rssrc/setup/error.rssrc/setup/mod.rssrc/setup/prompts.rssrc/setup/wizard.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/setup/prompts.rs
- src/setup/wizard.rs
Mapping every wizard::run error into EgoPulseError surfaced the user's explicit 'Abort' choice as a process failure (exit 1). Special-case SetupWizardError::Aborted to Ok(()) so cancellation exits cleanly while genuine setup failures still propagate via EgoPulseError::from.
Owner
Author
レビュー対応 (追加コミット
|
| 指摘 | ステータス | 対応 |
|---|---|---|
Inline: run_setup_wizard が Abort をエラー扱いしている |
✅ 修正済み | SetupWizardError::Aborted を Ok(()) で抜けるよう special-case。ユーザーキャンセル時の exit(1) を解消。# Errors セクションも追加 (AGENTS.md 規約)。 |
Nitpick: run_setup_wizard を pub → pub(crate) |
ℹ️ スキップ | lib.rs が pub mod setup; で setup モジュールを公開 API の一部として公開しており、run_setup_wizard がその唯一の公開エントリポイント。pub(crate) にするとモジュール内の 76 個の pub(crate) アイテムが一斉に dead_code 扱いになり -D warnings で CI が fail する。現状維持が正しい。 |
検証
cargo fmt --check: 通過cargo clippy --all-targets --all-features -- -D warnings: 警告ゼロcargo test --lib setup::: 46 テスト全合格RUSTDOCFLAGS="-D warnings" cargo doc --no-deps: 通過
Previously save_config rebuilt agents/providers/channels from scratch with only the user's Q1-Q7 inputs, so re-running 'egopulse setup' with existing custom agents/providers silently dropped them. Existing users also lost their log_level (hard-coded to 'info'). Merge-save semantics: - agents: clone existing map and insert the new agent (same id overwrites, different id adds). default_agent follows the new input. - providers: clone existing map and insert the chosen provider. - channels: clone existing map. Web is removed when disabled, overwritten when enabled. Discord/Telegram are left untouched when disabled, and updated when enabled (default bot merge for Discord). - log_level: read from existing config instead of hard-coded 'info'. Drop the 'custom agents preserved in backup' completion warning since those agents are no longer dropped. Add regression tests for preserved non-default agents/providers and log_level.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/setup/summary.rs`:
- Around line 204-214: The agents map update in the summary setup overwrites an
existing entry and resets fields like provider, model, discord_bot,
telegram_bot, and profiles to defaults. Update the existing AgentConfig in the
agents collection so that only the label from inputs.agent_label is changed
while preserving the rest of the current configuration; use the existing
agent_id lookup in the summary setup flow to merge instead of replacing the full
struct.
- Around line 188-202: The save logic in summary.rs is only handling the enabled
paths for discord and telegram, so disabled selections can leave stale
`channels.discord` / `channels.telegram` data behind. Update the summary merge
logic around the Discord and Telegram branches to explicitly clear or overwrite
those entries when the channel is disabled, and make sure the Discord path that
uses `or_default()` also sets the Discord channel’s enabled state in the stored
summary. Use the existing `discord_enabled`, `telegram_enabled`, `new_channels`,
and `ChannelName::new(...)` flow to keep the saved config aligned with the
user’s selection.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae1ad65f-d307-4ed1-993f-588e66238bf8
📒 Files selected for processing (1)
src/setup/summary.rs
…setup
Two issues in the previous merge-save implementation:
1. agents.insert(.., AgentConfig { label, ..Default::default() }) reset
existing agent fields (provider, model, discord_bot, telegram_bot,
profiles) when the user re-set up an existing agent. Use entry().or_default()
and update only the label so the rest of the AgentConfig is preserved.
2. Disabled discord/telegram selections left stale channel entries in the
saved config, so the bot kept running despite the user opting out.
Remove channels.discord / channels.telegram when disabled. For enabled
discord, insert the build_channel_configs entry first (carries the
enabled flag) before merging discord_bots, so the channel state reflects
the user's selection.
Add regression tests covering preserved agent provider/model and removal
of disabled discord/telegram channels.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
egopulse setupを ratatui フル TUI からdialoguerベースのチャットライク順次プロンプトへ全面刷新しました。docs/setup-redesign.mddocs/plan/plan-setup-refresh.md変更ポイント
Agent-First フロー
Q1 Agent Label → Q2 Provider → Q3 Model / base_url → Q4 API Key → Q5 Web → Q6 Discord → Q7 Telegram の順次プロンプト。Agent Label を slugify して agent id を自動生成し、
default_agent/agents.<id>.labelを自動設定。Web チャネル強制有効化を廃止
ユーザー選択で Web を無効化可能 (Discord / Telegram と一貫した omit パターン、
enabled: Some(false)残しではない)。Review の3択
Start over/Abort/Save anywayをdialoguer::Selectで提供 (ReviewDecisionenum で型安全に処理)。Additional Options ステップ新設
設定対象外項目 (System / Web UI / Channels / Subsystems の4カテゴリ) を Done に表示し、YAML 直接編集を案内。
既存資産の流用と拡張
PROVIDER_PRESETS/find_provider_preset/normalize_provider_id等は流用generate_auth_token/backup_config/extract_existing_state_rootは流用build_channel_configsにweb_enabledパラメータを追加save_configをSetupInputs受け取りに変更既存設定の保持
WEB_AUTH_TOKENを再利用 (ローテーションしない)state_rootを保持 (上書きしない)旧 TUI 実装を完全削除
src/setup/mod.rs: 1115 → 21 行 (thin wrapper のみ残置)SetupApp/draw_*/handle_*/init_terminal等 ~1094 行を削除ratatui/crossterm依存はsrc/channels/tui.rsが残るため維持テスト可能性の向上
PromptSource/OutputSinktrait で入出力を抽象化slugify_agent_id,validate_inputs,build_review_summary,review_decision_from_index,should_confirm_empty_api_key等)テスト
cargo fmt --check: 通過cargo clippy --all-targets --all-features -- -D warnings: 警告ゼロcargo test: 1383 テスト全合格E2E 手動確認 (
egopulse setupを実際に起動して dialoguer 入力を試すこと) は本 PR スコープ外。Step 8 の trait 抽象 + モック駆動テストで実質的なフロー検証を機械的に担保済み。既知の制限 (次 Issue 対応)
PromptSource::selecttrait に default パラメータを追加することで対応予定。コミット構成
docs(setup): add setup wizard refresh plan and design memofeat(setup): add slugify_agent_id for agent label normalization(Step 1, T1-T6)feat(setup): add SetupInputs type and validate_inputs for chat-based wizard(Step 2, T7-T11)feat(setup): allow web channel disablement in build_channel_configs(Step 3, T12-T14)feat(setup): support agent label, web disablement and existing value preservation in save_config(Step 4, T15-T21)refactor(setup): extract parse_existing_config as pure function(Step 5, T22-T23)feat(setup): add format_api_key_for_review for Review step(Step 6, T25-T26)feat(setup): add wizard message builders, review decision and branch predicates(Step 7, T27-T34)feat(setup): integrate dialoguer prompts with trait abstraction and wizard flow tests(Step 8, T35-T41)docs(setup): refresh setup wizard docs(Step 10)refactor(setup): remove legacy ratatui TUI implementation(Step 9)test(setup): add mask_secret regression test for short values(T24)Summary by CodeRabbit