t1160.1: Create build_cli_cmd() abstraction in supervisor/dispatch.sh#2053
t1160.1: Create build_cli_cmd() abstraction in supervisor/dispatch.sh#2053marcusquinn merged 1 commit intomainfrom
Conversation
…ranches (t1160.1) Introduces build_cli_cmd() — a single semantic-to-CLI-specific command builder that replaces 6 duplicated if/else opencode-vs-claude branches across: - build_dispatch_cmd() (NUL-delimited output) - build_verify_dispatch_cmd() (NUL-delimited output) - do_prompt_repeat() (array output) - cmd_reprompt() (array output) - check_cli_health() (version command) - check_model_health() (probe command) Supports 3 actions (run, version, probe) and 2 output modes (nul, array). Pure refactor — no behavior change.
WalkthroughA shell script refactor that introduces a centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Sat Feb 21 03:23:30 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
Summary of ChangesHello @marcusquinn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the maintainability and extensibility of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a new function build_cli_cmd() to centralize the logic for constructing CLI commands for different AI clients (opencode and claude). This refactoring significantly reduces code duplication and improves maintainability, as noted in the PR description. The changes correctly integrate the new abstraction into existing call sites, including do_prompt_repeat(), check_cli_health(), check_model_health(), build_dispatch_cmd(), and build_verify_dispatch_cmd(). While printf '%q ' is used for quoting, the use of eval for array construction, though common, is noted as an area where direct bash array manipulation is generally preferred for robustness and safety, aligning with best practices for dynamic command construction. The NUL-delimited output for process substitution is also well-implemented. The PR adheres to the shell scripting quality standards by using local var="$1" patterns and explicit return statements. Overall, this is a well-executed refactoring that enhances the code's structure and reduces complexity.
| eval "cmd_parts=($(build_cli_cmd \ | ||
| --cli "$ai_cli" \ | ||
| --action run \ | ||
| --output array \ | ||
| --model "$tmodel" \ | ||
| --title "$session_title" \ | ||
| --prompt "$reinforced_prompt"))" |
There was a problem hiding this comment.
While printf '%q ' is used to quote arguments, eval for dynamic command construction, even with careful quoting, can still introduce risks. It's generally more robust and safer to use bash arrays directly for dynamic command construction to avoid eval entirely and simplify argument handling, aligning with best practices for shell scripting.
References
- Use bash arrays for dynamic command construction to avoid duplicating argument expansion patterns (e.g., safe expansion for empty arrays) across conditional branches like sudo vs non-sudo paths.
| fi | ||
| # t1160.1: Build version command via build_cli_cmd abstraction | ||
| local -a version_cmd=() | ||
| eval "version_cmd=($(build_cli_cmd --cli "$ai_cli" --action version --output array))" |
There was a problem hiding this comment.
Similar to other instances, while printf '%q ' helps quote arguments, eval for dynamic command construction can still introduce risks. It's generally more robust and safer to use bash arrays directly for dynamic command construction to avoid eval entirely and simplify argument handling, aligning with best practices for shell scripting.
References
- Use bash arrays for dynamic command construction to avoid duplicating argument expansion patterns (e.g., safe expansion for empty arrays) across conditional branches like sudo vs non-sudo paths.
| fi | ||
| # t1160.1: Build probe command via build_cli_cmd abstraction | ||
| local -a probe_cmd=() | ||
| eval "probe_cmd=($(build_cli_cmd --cli "$ai_cli" --action probe --output array --model "$model"))" |
There was a problem hiding this comment.
As noted elsewhere, while printf '%q ' helps quote arguments, eval for dynamic command construction can still introduce risks. It's generally more robust and safer to use bash arrays directly for dynamic command construction to avoid eval entirely and simplify argument handling, aligning with best practices for shell scripting.
References
- Use bash arrays for dynamic command construction to avoid duplicating argument expansion patterns (e.g., safe expansion for empty arrays) across conditional branches like sudo vs non-sudo paths.
| eval "cmd_parts=($(build_cli_cmd \ | ||
| --cli "$ai_cli" \ | ||
| --action run \ | ||
| --output array \ | ||
| --model "$resolved_model" \ | ||
| --title "$retry_title" \ | ||
| --prompt "$reprompt_msg"))" |
There was a problem hiding this comment.
While printf '%q ' is used to quote arguments, eval for dynamic command construction, even with careful quoting, can still introduce risks. It's generally more robust and safer to use bash arrays directly for dynamic command construction to avoid eval entirely and simplify argument handling, aligning with best practices for shell scripting.
References
- Use bash arrays for dynamic command construction to avoid duplicating argument expansion patterns (e.g., safe expansion for empty arrays) across conditional branches like sudo vs non-sudo paths.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.agents/scripts/supervisor/dispatch.sh (2)
1028-1035: No guard against emptycmd_partsifbuild_cli_cmdfails.The
eval "cmd_parts=($(build_cli_cmd ...))"pattern is correct for deserializingprintf %qoutput, and in practicebuild_cli_cmdwon't fail here since--cliand--actionare always provided. However, ifbuild_cli_cmdever returns non-zero (e.g., future action added, typo in a flag),cmd_partssilently becomes an empty array, leading to an emptyexecline in the dispatch script.This same pattern appears at Lines 1575, 1708, and 3299.
🛡️ Optional: guard against empty cmd_parts
eval "cmd_parts=($(build_cli_cmd \ --cli "$ai_cli" \ --action run \ --output array \ --model "$tmodel" \ --title "$session_title" \ --prompt "$reinforced_prompt"))" + if [[ ${`#cmd_parts`[@]} -eq 0 ]]; then + log_error "do_prompt_repeat: build_cli_cmd produced empty command" + return 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/supervisor/dispatch.sh around lines 1028 - 1035, The eval assignment that deserializes build_cli_cmd into local -a cmd_parts=() can yield an empty array when build_cli_cmd fails; update the code around the eval "cmd_parts=($(build_cli_cmd ...))" to capture build_cli_cmd's exit status (or check whether cmd_parts is empty) immediately after the eval, and if non-zero/empty log an error and exit non‑zero (or skip the exec) to avoid executing an empty command; apply the same guard to the other occurrences of this pattern (the eval usages that populate cmd_parts) so you don't call exec with an empty array.
1894-1896: Minor:_emit_tokenleaks into global scope after firstbuild_cli_cmdcall.Bash nested functions become globally visible. After the first invocation of
build_cli_cmd,_emit_tokenpersists as a global function. It works correctly here because it's only called withinbuild_cli_cmdwherelocal -a _tokens=()is in scope, but if another function later defines or calls_emit_token, there could be unexpected behavior.Not a practical issue today (no other callers exist), but worth noting for future-proofing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/supervisor/dispatch.sh around lines 1894 - 1896, The nested helper function _emit_token defined inside build_cli_cmd leaks into the global function namespace; change build_cli_cmd to clean it up by unsetting the function after use (call unset -f _emit_token at the end of build_cli_cmd) or use a uniquely namespaced name (e.g., __build_cli_cmd_emit_token) for the helper and then unset it; update the code around the local -a _tokens=() and _emit_token() declaration so the helper is removed (unset -f _emit_token or unset -f __build_cli_cmd_emit_token) before build_cli_cmd returns to avoid global leakage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/supervisor/dispatch.sh:
- Around line 1726-1730: The fallback wait invocation currently uses "wait
\"$probe_pid\" 2>/dev/null || true", which masks the real exit code because the
"|| true" sets "$?" to 0; change the logic to run wait without immediately
chaining "|| true", capture its exit status into probe_exit (e.g., run wait
"$probe_pid" 2>/dev/null; probe_exit=$?; if wait itself failed and you still
need to ignore that specific failure then handle that case explicitly), and only
then apply any fallback/default value (like setting probe_exit=124 when
appropriate); update references to probe_exit and probe_pid so the probe result
check uses the captured exit code rather than the result of the "|| true" guard.
---
Nitpick comments:
In @.agents/scripts/supervisor/dispatch.sh:
- Around line 1028-1035: The eval assignment that deserializes build_cli_cmd
into local -a cmd_parts=() can yield an empty array when build_cli_cmd fails;
update the code around the eval "cmd_parts=($(build_cli_cmd ...))" to capture
build_cli_cmd's exit status (or check whether cmd_parts is empty) immediately
after the eval, and if non-zero/empty log an error and exit non‑zero (or skip
the exec) to avoid executing an empty command; apply the same guard to the other
occurrences of this pattern (the eval usages that populate cmd_parts) so you
don't call exec with an empty array.
- Around line 1894-1896: The nested helper function _emit_token defined inside
build_cli_cmd leaks into the global function namespace; change build_cli_cmd to
clean it up by unsetting the function after use (call unset -f _emit_token at
the end of build_cli_cmd) or use a uniquely namespaced name (e.g.,
__build_cli_cmd_emit_token) for the helper and then unset it; update the code
around the local -a _tokens=() and _emit_token() declaration so the helper is
removed (unset -f _emit_token or unset -f __build_cli_cmd_emit_token) before
build_cli_cmd returns to avoid global leakage.
| probe_exit=124 | ||
| else | ||
| local probe_pid probe_tmpfile | ||
| probe_tmpfile=$(mktemp) | ||
| push_cleanup "rm -f '${probe_tmpfile}'" | ||
| ("${probe_cmd[@]}" >"$probe_tmpfile" 2>&1) & | ||
| probe_pid=$! | ||
| local waited=0 | ||
| while kill -0 "$probe_pid" 2>/dev/null && [[ "$waited" -lt 15 ]]; do | ||
| sleep 1 | ||
| waited=$((waited + 1)) | ||
| done | ||
| if kill -0 "$probe_pid" 2>/dev/null; then | ||
| kill "$probe_pid" 2>/dev/null || true | ||
| wait "$probe_pid" 2>/dev/null || true | ||
| probe_exit=124 | ||
| else | ||
| wait "$probe_pid" 2>/dev/null || true | ||
| probe_exit=$? | ||
| fi | ||
| probe_result=$(cat "$probe_tmpfile" 2>/dev/null || true) | ||
| rm -f "$probe_tmpfile" | ||
| wait "$probe_pid" 2>/dev/null || true | ||
| probe_exit=$? | ||
| fi |
There was a problem hiding this comment.
Bug: || true masks probe exit code in the no-timeout fallback path.
On Line 1728, wait "$probe_pid" 2>/dev/null || true causes $? to always be 0 (from true), so probe_exit on Line 1729 is always 0 regardless of the actual probe outcome. A failed probe that emits non-empty error output (e.g., a CLI error message) would bypass the [[ -z "$probe_result" && "$probe_exit" -ne 0 ]] check at Line 1754 and be cached as healthy.
🐛 Fix: capture exit code before the || guard
else
- wait "$probe_pid" 2>/dev/null || true
- probe_exit=$?
+ wait "$probe_pid" 2>/dev/null
+ probe_exit=$?
fiIf wait itself can fail for reasons other than the child's exit code (unlikely here since the PID is valid), an alternative is:
- wait "$probe_pid" 2>/dev/null || true
- probe_exit=$?
+ probe_exit=0
+ wait "$probe_pid" 2>/dev/null || probe_exit=$?📝 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.
| probe_exit=124 | |
| else | |
| local probe_pid probe_tmpfile | |
| probe_tmpfile=$(mktemp) | |
| push_cleanup "rm -f '${probe_tmpfile}'" | |
| ("${probe_cmd[@]}" >"$probe_tmpfile" 2>&1) & | |
| probe_pid=$! | |
| local waited=0 | |
| while kill -0 "$probe_pid" 2>/dev/null && [[ "$waited" -lt 15 ]]; do | |
| sleep 1 | |
| waited=$((waited + 1)) | |
| done | |
| if kill -0 "$probe_pid" 2>/dev/null; then | |
| kill "$probe_pid" 2>/dev/null || true | |
| wait "$probe_pid" 2>/dev/null || true | |
| probe_exit=124 | |
| else | |
| wait "$probe_pid" 2>/dev/null || true | |
| probe_exit=$? | |
| fi | |
| probe_result=$(cat "$probe_tmpfile" 2>/dev/null || true) | |
| rm -f "$probe_tmpfile" | |
| wait "$probe_pid" 2>/dev/null || true | |
| probe_exit=$? | |
| fi | |
| probe_exit=124 | |
| else | |
| wait "$probe_pid" 2>/dev/null | |
| probe_exit=$? | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/scripts/supervisor/dispatch.sh around lines 1726 - 1730, The
fallback wait invocation currently uses "wait \"$probe_pid\" 2>/dev/null ||
true", which masks the real exit code because the "|| true" sets "$?" to 0;
change the logic to run wait without immediately chaining "|| true", capture its
exit status into probe_exit (e.g., run wait "$probe_pid" 2>/dev/null;
probe_exit=$?; if wait itself failed and you still need to ignore that specific
failure then handle that case explicitly), and only then apply any
fallback/default value (like setting probe_exit=124 when appropriate); update
references to probe_exit and probe_pid so the probe result check uses the
captured exit code rather than the result of the "|| true" guard.
marcusquinn
dismissed
coderabbitai[bot]’s stale review
Auto-dismissed: bot review does not block autonomous pipeline
1 participant
Summary
Introduces
build_cli_cmd()— a single semantic-to-CLI-specific command builder that replaces 6 duplicated if/else opencode-vs-claude branches (12+ total branches) acrossdispatch.sh.What changed
build_cli_cmd()accepts named flags (--cli,--action,--model,--title,--prompt,--output) and produces the correct CLI-specific command tokensrun(dispatch worker),version(health check),probe(model health)nul(NUL-delimited for process substitution) andarray(printf %q for eval)build_dispatch_cmd(),build_verify_dispatch_cmd(),do_prompt_repeat(),cmd_reprompt(),check_cli_health(),check_model_health()Why
The opencode-vs-claude branching was duplicated 6 times with subtle differences (model prefix stripping for claude, title support for opencode only, different output format flags). This made it error-prone to add new CLI support or modify command structure. Now there's a single source of truth.
Verification
bash -n dispatch.sh— syntax validshellcheck -x -S warning dispatch.sh— zero violationsanthropic/claude-opus-4-6→claude-opus-4-6)Pure refactor — no behavior change.
Ref #1747
Summary by CodeRabbit
Refactor
Documentation