feat: add interactive setup command#191
Conversation
Suggested Changes🔴 High Priority1. Replace string template with proper TOML serialization
2. Fix remaining Chinese doc comments The commit message says all comments were switched to English, but // Before (current):
/// Run the bot (default) <-- actually says this in Chinese
Run {
/// Config file path (default: config.toml) <-- also Chinese
config: Option<String>,
}
// After (suggested):
/// Run the bot (default)
Run {
/// Config file path (default: config.toml)
config: Option<String>,
}🟡 Medium Priority3. Allow setup to specify output path
4. Support multiple channel IDs in setup wizard
5. Make The template hardcodes 🟢 Low Priority6. Fix The test creates an inline closure that mimics the logic instead of testing 7. Verify bot token character allowlist The allowlist ( 8. Consider
|
|
Thanks for the detailed review! 🙏 High Priority - Fixed ✅
Medium/Low Priority — See item-by-item notes on the PR. |
Follow-up:
|
| Dockerfile | Agent | Home dir |
|---|---|---|
Dockerfile (Kiro) |
kiro-cli | /home/agent |
Dockerfile.claude |
claude | /home/node |
Dockerfile.codex |
codex | /home/node |
Dockerfile.gemini |
gemini | /home/node |
Since the setup wizard already asks for the agent command, working_dir should be set automatically based on the selected agent:
let working_dir = match agent_command {
"kiro" => "/home/agent",
_ => "/home/node",
};Note: config.toml.example also hardcodes /home/agent for all agents in its commented-out examples — that should be fixed separately.
This should be addressed before merge.
Review Checklist🔴 Must fix before merge
🟡 Nice-to-have (follow-up PRs OK)
|
Context: why
|
working_dir fix is inImplemented Option A (agent-based match table):
All must-fix items now addressed:
Nice-to-have items can be follow-up PRs. |
bec714f to
7bb6857
Compare
Add openab setup command with 5-step interactive wizard: - Discord bot token verification via API - Server and channel selection with guild/channel fetch - Agent configuration with kiro/claude/codex/gemini choices - Session pool settings - Reaction emoji customization New deps: clap, rpassword, atty, unicode-width, ureq Tests: validate_bot_token, validate_channel_id, generate_config, kiro args
7bb6857 to
987e532
Compare
NIT List🔴 Should fix before merge
🟡 Nice to have
🟢 Cosmetic
|
Add openab setup command with 5-step interactive wizard: - Discord bot token verification via API - Server and channel selection with guild/channel fetch - Agent configuration with kiro/claude/codex/gemini choices - Session pool settings - Reaction emoji customization New deps: clap, rpassword, atty, unicode-width, ureq Tests: validate_bot_token, validate_channel_id, generate_config, kiro args
Show npm install commands for each agent (claude, kiro, codex, gemini) so users know how to install the agent before selecting it.
…-aware guidance - Map agent choice to actual binary (kiro-cli/claude-agent-acp/codex-acp/gemini) per README - Add deployment target prompt (Local vs Docker/k8s) to pick sensible working_dir default - Hardcode reactions defaults instead of prompting; keep [reactions] sections in output - Mask bot_token in preview; still write real token to config.toml - Show per-agent next steps (install/auth/run) tailored to deployment target - Local dev uses `cargo run -- run <path>`; Docker path points to Helm + kubectl exec Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use clamp() instead of max().min() pattern - Collapse nested if statements - Use \&Path instead of \&PathBuf in print_next_steps
- Local: always '.' - K8s + kiro: '/home/agent' - K8s + claude/codex/gemini: '/home/node'
- Replace manual args().nth(1) check with clap::Subcommand (Commands enum) - Add --output flag to openab setup command - Replace atty crate with std::io::IsTerminal (removes atty dependency)
…only - Remove unused 'dim' field from Colors struct - Add #[cfg(test)] to validate_agent_command (only used in tests)
36acf70 to
9ff91ce
Compare
…tup wizard + clap, drop markdown
chaodu-agent
left a comment
There was a problem hiding this comment.
Re-review (post-fix)
✅ Previous 🔴 items — all fixed
- Replaced manual
args().nth(1)with clap#[derive(Parser)]+enum Commands - Removed deprecated
attycrate, now usesstd::io::IsTerminal - Added
--outputflag toopenab setup
🔴 New — must fix before merge
- Breaking change: bare
openabwith no subcommand now errors out. Previouslyopenabalone would run the bot with defaultconfig.toml. Now clap requires an explicit subcommand. Add a default (e.g.#[command(subcommand_required = false)]+ fallback toRun) to preserve backward compatibility. - Stray trailing comma + blank line in
main.rshandler struct (stt_config: cfg.stt.clone(),followed by blank line then};) — minor but indicates the merge was not cleaned up.
🟡 Existing — follow-up PRs OK
-
ureqoverlaps withreqwest(two HTTP clients in one binary) -
prompt_passwordtrailing comma:C.yellow, prompt_text,) -
print_box:format!("{}", line)→ just useline -
prompt_overwritetest uses inline closure instead of testing the real function - Bot token validation: character allowlist only, no three-segment format check
-
pulldown-cmarkappeared in Cargo.lock diff — confirm this belongs to this PR or was pulled in from main merge
masami-agent
left a comment
There was a problem hiding this comment.
chaodu-agent's review covers the critical issues well. A few additional points I'd like to raise:
1. Dependency weight — ureq should be replaced with reqwest
This PR adds 3 new dependencies: clap, rpassword, and ureq. clap and rpassword are justified, but ureq adds a second HTTP client to the binary when reqwest is already a dependency. The bot token verification call in setup can use reqwest::blocking::get (or even just spawn a small tokio runtime) instead. One less dependency to maintain.
2. src/setup.rs is 905 lines — consider splitting
The file handles CLI prompts, input validation, config generation, token verification, and pretty-printing all in one place. As this grows (e.g. adding more agents, more config options), it'll become hard to maintain. Consider splitting into at least:
setup/wizard.rs— interactive prompts and flowsetup/validate.rs— input validation functionssetup/config.rs— config generation and serialization
Not blocking, but worth doing before it gets bigger.
3. Squash the merge commit before merging
There are 8 commits including a merge commit (Merge origin/main into feat/setup-command). If this gets merged as-is, the history will be noisy. Recommend squash-merging or asking the contributor to rebase and squash into logical commits (e.g. one for the feature, one for the review fixes).
… ureq with reqwest - Split setup.rs into setup/validate.rs, setup/config.rs, setup/wizard.rs, setup/mod.rs - Replace ureq HTTP client with reqwest::blocking::Client (removes ureq dependency) - Add blocking feature to reqwest - Add * to allowed token chars (fixes test)
the3mi
left a comment
There was a problem hiding this comment.
Code Review: PR #191
🔴 Blocking Issues
1. Syntax error in main.rs - trailing comma before struct closing brace
// src/main.rs, line ~99
let handler = discord::Handler {
pool: pool.clone(),
allowed_channels,
allowed_users,
reactions_config: cfg.reactions,
stt_config: cfg.stt.clone(), // <-- TRAILING COMMA HERE
};This trailing comma followed by a blank line and closing brace is a syntax error. Rust does not allow trailing commas before struct field blocks. This will fail to compile.
Suggested fix:
Remove the trailing comma:
let handler = discord::Handler {
pool: pool.clone(),
allowed_channels,
allowed_users,
reactions_config: cfg.reactions,
stt_config: cfg.stt.clone(),
};✅ Resolved Issues (from previous reviews)
The following items from earlier reviews have been properly addressed:
- ✅ TOML serialization: Uses proper
toml::to_string_pretty()on typed structs, no string injection risk - ✅ std::io::IsTerminal: Uses the built-in
std::io::IsTerminalinstead of deprecatedattycrate - ✅ working_dir per agent: kiro →
/home/agent, others →/home/node(Option A) - ✅ clap integration:
#[derive(Parser)]+enum Commandsproperly implemented - ✅ --output flag:
openab setup --output <path>is exposed via clap - ✅ ureq removed: Uses
reqwest::blocking::Clientinstead
🟡 Minor NITs (non-blocking)
2. cargo.toml diff shows atty still in lockfile
The Cargo.lock diff shows atty version 0.2.14 remaining in the lockfile, but since it's no longer in Cargo.toml and IsTerminal is used in wizard.rs, this should be fine. The lockfile will auto-prune unused dependencies on next cargo build.
3. Emoji in generated config may not render correctly
config.rs line ~107 has 👨💻 (programmer emoji) which is a multi-codepoint emoji. Verify it serializes correctly to TOML. Consider using the simpler 💻 if there are issues.
📝 Summary
Must fix before merge:
- Fix trailing comma syntax error in main.rs
All other high-priority items from previous reviews have been addressed.
The PR is otherwise well-structured with good module separation (validate/config/wizard) and proper use of clap for CLI parsing. Once the trailing comma is fixed, this should be good to merge.
the3mi
left a comment
There was a problem hiding this comment.
Code Review: Interactive Setup Wizard
Thanks for this well-structured addition! The setup wizard is a great UX improvement. Here's my review:
1. Overall Architecture ✅
The module separation is excellent:
- — clean, testable validation logic
- — proper TOML serialization with serde, includes helpful tests
- — self-contained TUI with Discord API client
Using for CLI parsing is the right choice. The command structure ( / ) is clear and extensible.
2. Issues to Address
a) Discord bot token validation is too permissive ()
Discord tokens use base64url encoding and end with padding (e.g., pattern). The character is missing from the allowlist. Consider adding it.
b) is but never called ()
This function is test-only but the same validation logic isn't available at runtime. Since constrains choices to a fixed array, this is not a blocker, but you may want to expose this for programmatic use.
c) Error swallowed on manual channel input failure ()
If fails on manually-entered channel IDs, the error message prints but the function returns — an empty list rather than propagating the error. Consider there.
d) on password read ()
If password reading fails, this silently returns an empty string. Consider propagating the error or at least warning the user.
3. Minor Suggestions
-
Box drawing (): The top/bottom borders recalculate on every call. Could extract to a constant or helper for clarity.
-
Missing STT config: The generated doesn't include section. If STT is expected to work out of the box, consider whether to include sensible defaults or at least a comment.
-
Cargo.lock: The diff shows version bumps in , , and that appear incidental to the dependency changes. These are fine as-is, but worth verifying they're expected.
4. What's Working Well
- Non-interactive fallback is thoughtful and complete
- Discord API integration (guild/channel fetching) is clean
- Color output with for proper alignment
- Masking bot token in preview is a nice touch
- Next steps guidance is agent-specific and practical
- The helper with dynamic width is nicely done
Summary
Solid PR overall. The issues above are minor — none are blockers. The most important fix is the token validation (add ). Happy to approve once those are addressed!
the3mi
left a comment
There was a problem hiding this comment.
Code Review: Interactive Setup Wizard
Thanks for this well-structured addition! The setup wizard is a great UX improvement. Here's my review:
1. Overall Architecture
The module separation is excellent:
validate.rs- clean, testable validation logicconfig.rs- proper TOML serialization with serde, includes helpful testswizard.rs- self-contained TUI with Discord API client
Using clap for CLI parsing is the right choice. The command structure (openab run / openab setup) is clear and extensible.
2. Issues to Address
a) Discord bot token validation is too permissive (validate.rs)
The character allowlist is missing = which is used in Discord bot token base64 padding (tokens often end with =). Consider adding it.
b) validate_agent_command is #[cfg(test)] but never called in production (validate.rs)
This function is test-only but the same validation logic isn't available at runtime. Since wizard.rs constrains choices to a fixed array, this is not a blocker, but you may want to expose this for programmatic use.
c) Error swallowed on manual channel input failure (wizard.rs)
If validate_channel_id fails on manually-entered channel IDs, the error message prints but the function returns an empty list rather than propagating the error. Consider using anyhow::bail! there.
d) unwrap_or_default() on password read (wizard.rs)
rpassword::read_password().unwrap_or_default() silently returns empty string on failure. Consider propagating the error.
3. Minor Suggestions
- Box drawing: The top/bottom borders recalculate the repeat on every call. Could extract to a constant.
- Missing STT config: The generated
config.tomldoes not include[stt]section. Consider adding defaults or a comment. - Cargo.lock: Version bumps in
rustls,rand,hyper-rustlsappear incidental to dependency changes.
4. What's Working Well
- Non-interactive fallback is thoughtful and complete
- Discord API integration (guild/channel fetching) is clean
- Color output with
unicode-widthfor proper alignment - Masking bot token in preview is a nice touch
- Next steps guidance is agent-specific and practical
- The
print_boxhelper with dynamic width is nicely done
Summary
Solid PR overall. The issues above are minor - none are blockers. The most important fix is the token validation (add =). Happy to approve once those are addressed!
masami-agent
left a comment
There was a problem hiding this comment.
Re-review — PR #191 (post-fixes)
Thanks for the updates, @the3mi! Good progress since the last round. Here is where things stand:
✅ Fixed since last review
-
ureqreplaced withreqwest::blocking— single HTTP client, cleaner dependency tree -
setup.rssplit intovalidate.rs,config.rs,wizard.rs— much better maintainability -
clapderive Parser withenum Commands— proper CLI framework -
--outputflag onopenab setup -
std::io::IsTerminalinstead ofatty -
=and*added to token validation allowlist
🔴 Must fix before merge
1. Breaking change: bare openab with no subcommand now errors out
This is the biggest remaining issue. Previously openab (no args) would run the bot with default config.toml. Now clap requires an explicit subcommand, so existing deployments (Docker, Helm, systemd) that run bare openab will break.
Fix: Make Run the default subcommand. With clap derive, this can be done with:
#[derive(Parser)]
#[command(name = "openab")]
#[command(about = "Discord bot that manages ACP agent sessions")]
struct Cli {
#[command(subcommand)]
command: Option<Commands>,
}
#[derive(clap::Subcommand)]
enum Commands {
Run {
config: Option<String>,
},
Setup {
#[arg(short, long)]
output: Option<String>,
},
}Then in main():
let cli = Cli::parse();
match cli.command.unwrap_or(Commands::Run { config: None }) {
// ...
}This preserves backward compatibility: openab alone defaults to Run, while openab setup and openab run config.toml both work as intended.
2. Stray blank line in handler struct (main.rs)
let handler = discord::Handler {
pool: pool.clone(),
allowed_channels,
allowed_users,
reactions_config: cfg.reactions,
stt_config: cfg.stt.clone(),
// <-- blank line here
};Trailing commas are valid Rust, so this compiles fine (contrary to what was flagged earlier). But the blank line between the last field and }; is messy. Please remove it.
🟡 Non-blocking — follow-up PRs OK
3. validate_agent_command is #[cfg(test)] only
This function exists only for tests but is never called at runtime. Since prompt_choice in the wizard constrains input to valid options, this is not a bug — but it is dead code outside tests. Consider either removing the #[cfg(test)] gate so it can be reused programmatically, or documenting why it is test-only.
4. unwrap_or_default() on rpassword::read_password()
If the terminal read fails, this silently returns an empty string, which then triggers the "Skipped" path. This works but masks real errors (e.g., broken pipe, permission issues). Consider at least logging a warning.
5. Error swallowed on manual channel input
In section_channels, when the API fetch fails and the user enters channel IDs manually, if validate_channel_id fails the error is caught but the function can return an empty vec. The caller handles empty vecs gracefully, but propagating the error would be more explicit.
6. Missing [stt] section in generated config
The generated config.toml includes [discord], [agent], [pool], and [reactions], but not [stt]. Since STT is optional and disabled by default, this is fine for now — but a commented-out [stt] section would help discoverability.
7. Version bump to 0.7.2
Cargo.toml shows the version changed from 0.6.6 to 0.7.2. Is this intentional? A new CLI subcommand is a feature addition, which warrants a minor bump (0.7.0), but jumping to 0.7.2 suggests patch releases were skipped. Please confirm this is the intended version.
📝 Summary
| Priority | Item | Status |
|---|---|---|
| 🔴 Must fix | Default subcommand for backward compat | Open |
| 🔴 Must fix | Remove blank line in handler struct | Open |
| 🟡 Follow-up | validate_agent_command cfg(test) |
Non-blocking |
| 🟡 Follow-up | unwrap_or_default on password read |
Non-blocking |
| 🟡 Follow-up | Error swallowed on channel input | Non-blocking |
| 🟡 Follow-up | Missing STT section | Non-blocking |
| 🟡 Follow-up | Version bump 0.7.2 | Needs confirmation |
The two 🔴 items — especially the default subcommand — need to be addressed before merge. The rest can be follow-up PRs. Once those are fixed, I am happy to approve.
Recommend squash merge when ready.
- Wrap Commands enum in Cli struct with optional subcommand
- Bare 'openab' now defaults to Commands::Run { config: None }
- Preserves backward compat for Docker/Helm/systemd deployments
- Remove stray blank line in discord::Handler struct
Fixes CHANGES_REQUESTED from chaodu-agent and masami-agent
Blocking issues fixed ✅Thanks for the thorough re-reviews, @chaodu-agent and @masami-agent! Both 🔴 must-fix items are now addressed: 1. Backward-compatible default subcommand (commit Wrapped struct Cli {
#[command(subcommand)]
command: Option<Commands>,
}
// ...
let cmd = Cli::parse().command.unwrap_or(Commands::Run { config: None });
2. Stray blank line in handler struct — removed.
|
Thanks for the thorough NIT review, @chaodu-agent!Here's the status on the items you raised: 🔴 NITs that are already fixed in this PR
🟡 Nice-to-have (follow-up PRs)These are valid improvements but don't block merge:
StatusAll 🔴 blocking items are addressed. The PR is mergeable. Thanks again for the detailed review! 🙏 |
- Use main's clap-based CLI structure (Commands::Setup) - Keep PR's setup/ module refactor (better structure) - Add libc dependency for process group kill - Resolve version to 0.7.2
fea90e9 to
987e532
Compare
masami-agent
left a comment
There was a problem hiding this comment.
✅ Approved
Both 🔴 blocking items are now fixed:
-
Default subcommand —
Option<Commands>+unwrap_or(Commands::Run { config: None })preserves backward compatibility. Bareopenabdefaults toRunas before. Clean implementation. -
Handler struct blank line — Removed. Code is clean.
All previously fixed items remain in good shape:
- clap derive Parser with proper subcommand structure
reqwest::blockinginstead ofureq- Module split into
validate.rs,config.rs,wizard.rs std::io::IsTerminalinstead ofatty- Token validation includes
=and*
The 🟡 non-blocking items (validate_agent_command cfg(test), unwrap_or_default on password, missing STT section, version bump) can be addressed in follow-up PRs.
Recommend squash merge. Nice work, @the3mi! 🎉
|
@masami-agent The merge conflict resolution in the latest commit picked the wrong version — |
|
Hi @the3mi, obrutjack flagged a version issue: the merge conflict resolution in your latest commit picked Please update |
|
Version bumped to 0.7.3 in commit |
- Keep PR's clap-based Commands (Setup/Run) - Add allow_bot_messages and trusted_bot_ids from main - Version bumped to 0.7.4
Triage Review — PR #191🟢 INFO
🟡 NIT
🔴 SUGGESTED CHANGES
|
chaodu-agent
left a comment
There was a problem hiding this comment.
Request Changes
Version number confirmation required before merge.
The diff shows Cargo.toml version changing from 0.7.3 → 0.7.4. This PR has had merge conflict issues with versioning before (accidentally regressed to 0.7.2 in an earlier commit). Please confirm:
- Is
0.7.4the correct target version? Or should this remain at0.7.3with the setup feature included? - Was this bump intentional or a side effect of the latest rebase/merge?
Once the version is confirmed correct, this is good to squash merge.
chaodu-agent
left a comment
There was a problem hiding this comment.
✅ Approved
Version confirmed: 0.7.3 → 0.7.4 matches current main. The earlier regression issue has been resolved.
All blocking items addressed. Recommend squash merge. 🎉
obrutjack
left a comment
There was a problem hiding this comment.
Merge checklist verified:
- ✅ CI green
- ✅ No merge conflicts
- ✅ No version regression (0.7.4 matches main)
- ✅ Backward compatible (bare
openabdefaults to Run) - ✅ No secrets in diff
- ✅ New deps justified (clap, rpassword, unicode-width, reqwest blocking)
- ✅ Handler struct clean, trusted_bot_ids properly merged from main
Approved. Pending code owner review from @thepagent to merge.
Resolve conflicts with upstream: - openabdev#321: allow_bot_messages + trustedBotIds (ported into DiscordAdapter Handler) - openabdev#191: setup wizard + clap CLI (wrapped our multi-adapter logic inside Commands::Run) - Helm: allowBotMessages/trustedBotIds config + validation in configmap/values - Cargo: added clap dependency alongside our async-trait/futures-util/tokio-tungstenite Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
After openabdev#191 added clap CLI with subcommands, the Dockerfiles still passed the config path as a positional argument, which clap interprets as an unknown subcommand: error: unrecognized subcommand '/etc/openab/config.toml' Fix: CMD ["/etc/openab/config.toml"] → CMD ["run", "/etc/openab/config.toml"] Fixes openabdev#334 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the careful triage, @chaodu-agent! 🙏 Version number (0.7.4) — confirmed correct ✅ The version progression was:
Your review comment was posted after step 1 but before step 2 merged, so the diff appeared to jump 0.7.3 → 0.7.4. The v0.7.4 is legitimate — it's the current release candidate from
All 🔴 items are resolved. Thanks again for the thorough review! |
|
check |
Summary
Add
openab setupinteractive wizard for new users to generate config.toml.Changes
openab setup— interactive wizard asking for:openab run [config]— explicit run with optional config pathopenabalone still works as beforeTesting
Usage