t1264.3: Integrate repo-sync into aidevops CLI, setup.sh, and onboarding#2016
t1264.3: Integrate repo-sync into aidevops CLI, setup.sh, and onboarding#2016marcusquinn merged 1 commit intomainfrom
Conversation
…per.sh (t1264.3) - Add check_repo_sync() to show repo-sync scheduler and dir count in status - Add configure_dirs() for interactive parent directory setup - Add configure-dirs command to main() case statement - Update show_help() to document new command - Integrates with repo-sync-helper.sh dirs add for config management
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 enhances 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
|
WalkthroughThe pull request refactors Changes
Sequence DiagramsequenceDiagram
participant User
participant Script as onboarding-helper.sh
participant GitConfig as Git Configuration
participant RepoSync as repo-sync
User->>Script: Run "configure-dirs" (or "dirs")
Script->>User: Prompt for Git parent directory path(s)
User->>Script: Provide directory input
Script->>Script: Validate & format input
Script->>GitConfig: Write git_parent_dirs config
GitConfig-->>Script: Config stored
Script->>User: Display confirmation
Note over Script,RepoSync: Later invocations...
User->>Script: Run "status"
Script->>Script: Execute check_repo_sync
Script->>RepoSync: Query repo-sync state
RepoSync->>GitConfig: Read git_parent_dirs config
GitConfig-->>RepoSync: Config retrieved
RepoSync-->>Script: Status information
Script->>User: Display overall status (including Repo Sync)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
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: Fri Feb 20 17:53:51 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![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 integrates repo-sync functionality into the onboarding-helper.sh script, adding a new check_repo_sync function to display its status and a configure_dirs function for interactive setup of parent directories. The show_status and show_help functions have been updated to reflect these changes. The changes are well-implemented and follow the existing code style. I've provided some feedback regarding error handling and consistency in output messages, incorporating repository-specific rules for shell scripting practices.
| # Check configured parent dirs | ||
| local dir_count=0 | ||
| if [[ -f "$config_file" ]] && command -v jq &>/dev/null; then | ||
| dir_count=$(jq -r '.git_parent_dirs[]? // empty' "$config_file" 2>/dev/null | wc -l | tr -d ' ') |
There was a problem hiding this comment.
The jq command should not suppress stderr with 2>/dev/null to ensure diagnostic information is visible, as per repository guidelines. If jq fails on an optional lookup, dir_count should correctly be 0. The || true pattern is used to prevent script exit while allowing stderr to be visible and ensuring wc -l correctly yields 0 on empty input.
| dir_count=$(jq -r '.git_parent_dirs[]? // empty' "$config_file" 2>/dev/null | wc -l | tr -d ' ') | |
| if [[ -f "$config_file" ]] && command -v jq >/dev/null; then | |
| dir_count=$(jq -r '.git_parent_dirs[]? // empty' "$config_file" || true | wc -l) | |
| fi |
References
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.
- When reporting errors for failed file operations in shell scripts, such as 'jq' writes, include the file path in the error message. Avoid suppressing stderr with '2>/dev/null' to ensure that diagnostic information about malformed files or write failures is visible.
| if bash "$repo_sync_script" dirs add "$new_dir"; then | ||
| added=$((added + 1)) | ||
| fi | ||
| else |
There was a problem hiding this comment.
The fallback mechanism for adding directories should handle jq failures or write errors. The error message should include the file path for better debugging, as per repository guidelines for failed file operations. The current suggestion correctly uses continue to skip to the next iteration.
| else | |
| if jq --arg d "$normalized" '.git_parent_dirs += [$d]' "$config_file" >"$temp_file"; then | |
| mv "$temp_file" "$config_file" | |
| echo "Added: $normalized" | |
| added=$((added + 1)) | |
| else | |
| rm -f "$temp_file" | |
| echo "Error: Failed to add '$new_dir' to '$config_file'. Check permissions or file format." >&2 | |
| continue # Skip to the next iteration if adding failed | |
| fi |
References
- When reporting errors for failed file operations in shell scripts, such as 'jq' writes, include the file path in the error message. Avoid suppressing stderr with '2>/dev/null' to ensure that diagnostic information about malformed files or write failures is visible.
| echo " $0 guide openai" | ||
| echo " $0 configure-dirs" | ||
| echo "" | ||
| echo "Use /onboarding in Claude Code for the full interactive experience." |
There was a problem hiding this comment.
The message "Use /onboarding in Claude Code for the full interactive experience." is updated to "Claude Code". This should be consistent with the rest of the codebase. If the intention is to refer to a specific tool, it should be explicitly named.
| echo "Use /onboarding in Claude Code for the full interactive experience." | |
| echo -e "Use /onboarding in OpenCode for the full interactive experience." |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.agents/scripts/onboarding-helper.sh (3)
448-449:repo_sync_scriptpath duplicated across two functions.
"$HOME/.aidevops/agents/scripts/repo-sync-helper.sh"is defined identically at Line 448 (check_repo_sync) and Line 481 (configure_dirs). A single file-scopereadonlykeeps this DRY and avoids drift.♻️ Proposed refactor
Add near the other
readonlyconstants at the top of the file:+readonly REPO_SYNC_SCRIPT="$HOME/.aidevops/agents/scripts/repo-sync-helper.sh"Then in both functions:
- local repo_sync_script="$HOME/.aidevops/agents/scripts/repo-sync-helper.sh" + local repo_sync_script="$REPO_SYNC_SCRIPT"Also applies to: 481-482
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/onboarding-helper.sh around lines 448 - 449, The repo_sync_script path is duplicated inside check_repo_sync and configure_dirs; define a single file-scope readonly variable (e.g., readonly REPO_SYNC_SCRIPT) near the other readonly constants at the top of the script and replace the local definitions in the functions (check_repo_sync and configure_dirs) with references to that constant to keep the value DRY and avoid divergence.
325-325: Redundant2>&1after&>/dev/null.
&>/dev/nullalready redirects both stdout and stderr to/dev/null; the trailing2>&1is a no-op. Appears on Lines 325, 425, and 944.♻️ Proposed cleanup (repeat for Lines 425, 944)
- if is_installed "npx" && npx --no-install playwright --version &>/dev/null 2>&1; then + if is_installed "npx" && npx --no-install playwright --version &>/dev/null; thenAlso applies to: 425-425, 944-944
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/onboarding-helper.sh at line 325, The shell redirection "&>/dev/null 2>&1" is redundant in the is_installed checks; remove the trailing "2>&1" so the command uses only "&>/dev/null". Update each occurrence that checks npx/playwright version (the lines using is_installed "npx" and the command `npx --no-install playwright --version`) — there are three instances noted — to eliminate the extraneous "2>&1", leaving "&>/dev/null" as the sole stdout+stderr redirect.
49-50:${value,,}avoids the fork-per-call in hot-path helpers.
echo "$value" | trspawns two subprocesses on every call tois_configured. Since this function is invoked ~20+ times perstatusrun, the bash built-in lowercase expansion is a free win.♻️ Proposed refactor
- local lower_value - lower_value=$(echo "$value" | tr '[:upper:]' '[:lower:]') + local lower_value="${value,,}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/onboarding-helper.sh around lines 49 - 50, Replace the external echo+tr pipeline in is_configured with bash parameter expansion to avoid forking: instead of running lower_value=$(echo "$value" | tr '[:upper:]' '[:lower:]'), assign lower_value="${value,,}" (ensure the script is run with bash so ${value,,} is supported and keep the variable name lower_value and the is_configured function intact).
🤖 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/onboarding-helper.sh:
- Around line 550-559: The fallback jq append unconditionally (jq
'.git_parent_dirs += [$d]') and can write duplicate entries for the same path
(variables: config_file, temp_file, normalized, new_dir), so change the jq
invocation to first deduplicate or check for existence before appending: either
use an expression like '.git_parent_dirs = (.git_parent_dirs + [$d] | unique)'
or conditional append 'if (.git_parent_dirs | index($d)) then . else
.git_parent_dirs += [$d] end', write to temp_file and mv on success as before,
and adjust the echo messages to reflect whether the path was added or already
present; keep the existing rm -f "$temp_file" on failure.
- Around line 530-534: The read in the while loop that prompts for "Parent
directory" (the read -r -p that stores into new_dir) can return non-zero on EOF
(Ctrl+D) and triggers a set -e exit; fix it by explicitly guarding the read's
exit status: perform the read inside a conditional or immediately test its
return and treat non-zero (EOF) as a graceful break (i.e., if the read fails,
break the loop), leaving the existing empty-input check for new_dir intact.
---
Nitpick comments:
In @.agents/scripts/onboarding-helper.sh:
- Around line 448-449: The repo_sync_script path is duplicated inside
check_repo_sync and configure_dirs; define a single file-scope readonly variable
(e.g., readonly REPO_SYNC_SCRIPT) near the other readonly constants at the top
of the script and replace the local definitions in the functions
(check_repo_sync and configure_dirs) with references to that constant to keep
the value DRY and avoid divergence.
- Line 325: The shell redirection "&>/dev/null 2>&1" is redundant in the
is_installed checks; remove the trailing "2>&1" so the command uses only
"&>/dev/null". Update each occurrence that checks npx/playwright version (the
lines using is_installed "npx" and the command `npx --no-install playwright
--version`) — there are three instances noted — to eliminate the extraneous
"2>&1", leaving "&>/dev/null" as the sole stdout+stderr redirect.
- Around line 49-50: Replace the external echo+tr pipeline in is_configured with
bash parameter expansion to avoid forking: instead of running lower_value=$(echo
"$value" | tr '[:upper:]' '[:lower:]'), assign lower_value="${value,,}" (ensure
the script is run with bash so ${value,,} is supported and keep the variable
name lower_value and the is_configured function intact).
| while true; do | ||
| read -r -p "Parent directory (e.g. ~/Git): " new_dir | ||
| if [[ -z "$new_dir" ]]; then | ||
| break | ||
| fi |
There was a problem hiding this comment.
read without EOF guard causes abrupt set -e exit on Ctrl+D.
read's exit status is non-zero when EOF is encountered. Because set -euo pipefail is active and this read sits in the loop body (not in an if condition), pressing Ctrl+D instead of typing an empty line causes the whole script to exit with a non-zero code — confusing for users who expect graceful cancellation.
🛡️ Proposed fix
- read -r -p "Parent directory (e.g. ~/Git): " new_dir
- if [[ -z "$new_dir" ]]; then
- break
- fi
+ read -r -p "Parent directory (e.g. ~/Git): " new_dir || break
+ if [[ -z "$new_dir" ]]; then
+ break
+ fiAs per coding guidelines for .agents/scripts/*.sh: "Reliability and robustness".
📝 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.
| while true; do | |
| read -r -p "Parent directory (e.g. ~/Git): " new_dir | |
| if [[ -z "$new_dir" ]]; then | |
| break | |
| fi | |
| while true; do | |
| read -r -p "Parent directory (e.g. ~/Git): " new_dir || break | |
| if [[ -z "$new_dir" ]]; then | |
| break | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/scripts/onboarding-helper.sh around lines 530 - 534, The read in the
while loop that prompts for "Parent directory" (the read -r -p that stores into
new_dir) can return non-zero on EOF (Ctrl+D) and triggers a set -e exit; fix it
by explicitly guarding the read's exit status: perform the read inside a
conditional or immediately test its return and treat non-zero (EOF) as a
graceful break (i.e., if the read fails, break the loop), leaving the existing
empty-input check for new_dir intact.
| fi | ||
| local temp_file="${config_file}.tmp" | ||
| if jq --arg d "$normalized" '.git_parent_dirs += [$d]' "$config_file" >"$temp_file"; then | ||
| mv "$temp_file" "$config_file" | ||
| echo "Added: $normalized" | ||
| added=$((added + 1)) | ||
| else | ||
| rm -f "$temp_file" | ||
| echo "Failed to add: $new_dir" | ||
| fi |
There was a problem hiding this comment.
Fallback path silently adds duplicate directories.
The primary path (repo_sync_script dirs add) presumably deduplicates, but the fallback jq '.git_parent_dirs += [$d]' appends unconditionally. Entering the same path twice in one session writes it twice to repos.json, which could cause double-sync noise.
🛡️ Proposed fix — deduplicate before appending
- if jq --arg d "$normalized" '.git_parent_dirs += [$d]' "$config_file" >"$temp_file"; then
+ if jq --arg d "$normalized" \
+ 'if (.git_parent_dirs | index($d)) then . else .git_parent_dirs += [$d] end' \
+ "$config_file" >"$temp_file"; then📝 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.
| fi | |
| local temp_file="${config_file}.tmp" | |
| if jq --arg d "$normalized" '.git_parent_dirs += [$d]' "$config_file" >"$temp_file"; then | |
| mv "$temp_file" "$config_file" | |
| echo "Added: $normalized" | |
| added=$((added + 1)) | |
| else | |
| rm -f "$temp_file" | |
| echo "Failed to add: $new_dir" | |
| fi | |
| fi | |
| local temp_file="${config_file}.tmp" | |
| if jq --arg d "$normalized" \ | |
| 'if (.git_parent_dirs | index($d)) then . else .git_parent_dirs += [$d] end' \ | |
| "$config_file" >"$temp_file"; then | |
| mv "$temp_file" "$config_file" | |
| echo "Added: $normalized" | |
| added=$((added + 1)) | |
| else | |
| rm -f "$temp_file" | |
| echo "Failed to add: $new_dir" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/scripts/onboarding-helper.sh around lines 550 - 559, The fallback jq
append unconditionally (jq '.git_parent_dirs += [$d]') and can write duplicate
entries for the same path (variables: config_file, temp_file, normalized,
new_dir), so change the jq invocation to first deduplicate or check for
existence before appending: either use an expression like '.git_parent_dirs =
(.git_parent_dirs + [$d] | unique)' or conditional append 'if (.git_parent_dirs
| index($d)) then . else .git_parent_dirs += [$d] end', write to temp_file and
mv on success as before, and adjust the echo messages to reflect whether the
path was added or already present; keep the existing rm -f "$temp_file" on
failure.
marcusquinn
dismissed
coderabbitai[bot]’s stale review
Auto-dismissed: bot review does not block autonomous pipeline
1 participant
Add repo-sync parent directories onboarding question to onboarding-helper.sh.
Changes:
check_repo_sync(): shows repo-sync scheduler status and configured dir count instatusoutputconfigure_dirs(): interactive prompt to add git parent directories, delegates torepo-sync-helper.sh dirs addconfigure-dirscommand added tomain()case statementshow_help()updated to document new commandcheck_repo_syncadded toshow_status()outputNote:
aidevops.shrepo-sync case andsetup.shenable prompt were already present in main (implemented in prior subtasks).Ref #1987
Summary by CodeRabbit
New Features
Style