feat: add update-refs skill#70
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughAdds a new skill document and a Bash CLI script that scan YAML under data/services/insights/ccx-data-pipeline in an app-interface checkout and update Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Script as Update Script
participant Local as Local app-interface (fs)
participant Remote as Git Remote (gitlab)
rect rgba(200,220,255,0.5)
User->>Script: run ./scripts/update_refs.sh [--dry-run] [--repo] [--local-folder]
end
Script->>Local: ensure repo checkout (clone or reuse)
Script->>Local: scan YAMLs under data/services/insights/ccx-data-pipeline
loop per referenced repo URL
Script->>Remote: git ls-remote --symref <repo-url> (resolve default branch)
Remote-->>Script: default branch ref
Script->>Remote: git ls-remote <repo-url> <default-branch>
Remote-->>Script: latest commit SHA
Script->>Local: replace `ref:` lines (or report if dry-run)
end
Script->>User: summary of changes, per-file counts
alt not dry-run
Script->>Local: commit to timestamped branch and output next steps
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
1eca385 to
548d08a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
skills/update-refs/scripts/update_refs.sh (1)
90-111:⚠️ Potential issue | 🟠 Major
current_urlcan leak across YAML objects and rewrite the wrongref.
current_urlis sticky until anotherurl:appears. A later unrelatedref:key can be incorrectly paired with a stale URL and updated to the wrong SHA.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/update-refs/scripts/update_refs.sh` around lines 90 - 111, The bug is that current_url is sticky across YAML objects and can pair with later unrelated ref lines; fix by tracking the URL's indentation and scoping so refs only match when at the same indentation (or reset current_url on dedent/blank). Modify the url regex handling to capture and save the leading whitespace as url_indent (in addition to current_url), update the ref-match check to capture the ref's leading whitespace and only proceed when that indent equals url_indent (or when current_url is non-empty and repo_matches "$current_url" and indent matches), and also clear current_url/url_indent when a line with less indentation or an empty/top-level line is seen so later refs won't reuse a stale URL (refer to variables current_url, url_indent, prefix, old_ref and functions repo_matches and get_latest_sha).
🧹 Nitpick comments (1)
skills/update-refs/SKILL.md (1)
65-69: Use a full timestamp in branch example to avoid collisions.
date +%Y%m%dcan collide on repeated runs the same day. Prefer including time (for example%Y%m%d%H%M%S).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/update-refs/SKILL.md` around lines 65 - 69, The branch example uses a date format that can collide; update the BRANCH assignment and the example checkout in the SKILL.md snippet (the lines setting BRANCH="update-refs-$(date +%Y%m%d)" and git checkout -b $BRANCH) to use a full timestamp format such as %Y%m%d%H%M%S to ensure unique branch names (e.g., change the date command to date +%Y%m%d%H%M%S).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/update-refs/scripts/update_refs.sh`:
- Around line 29-43: The script update_refs.sh currently cd's into
PATH_TO_APP_INTERFACE and runs git fetch/checkout/pull without verifying the
target is the intended repo, risking mutations of an unrelated repository;
before executing git operations (the cd and the git fetch origin master / git
checkout master / git pull origin master sequence), validate
PATH_TO_APP_INTERFACE is a valid git repo and points to the expected remote
(e.g., check that "$PATH_TO_APP_INTERFACE/.git" exists and that git -C
"$PATH_TO_APP_INTERFACE" remote get-url origin matches the expected
git@gitlab.cee.redhat.com:service/app-interface.git), abort with an error if
validation fails, and only then perform the cd and git commands.
In `@skills/update-refs/SKILL.md`:
- Around line 18-19: The README prerequisites claim HTTPS Git access but the
script update_refs.sh performs an SSH clone using the git@... transport;
reconcile them by either updating the SKILL.md prerequisite to require
SSH/key-based access and mention the git@gitlab.cee.redhat.com clone style, or
change the clone invocation in update_refs.sh to use the HTTPS URL
(https://gitlab.cee.redhat.com/service/app-interface.git) so the documentation
and the script match; locate the SSH clone invocation in update_refs.sh (the git
clone git@gitlab.cee.redhat.com:... command) or the prerequisites list in
SKILL.md and make the corresponding edit.
- Around line 83-85: Update the wording in the SKILL.md section that currently
reads "Clones (or reuses) app-interface to `/tmp/app-interface`" so it no longer
contradicts the documented --local-folder behavior: explicitly state that when
--local-folder is provided the tool reuses the specified path, otherwise it
clones into /tmp (or uses `/tmp/app-interface` by default). Reference the phrase
"Clones (or reuses) app-interface to `/tmp/app-interface`" and the option name
"--local-folder" when making the change so the intent and behavior are clear and
consistent.
---
Duplicate comments:
In `@skills/update-refs/scripts/update_refs.sh`:
- Around line 90-111: The bug is that current_url is sticky across YAML objects
and can pair with later unrelated ref lines; fix by tracking the URL's
indentation and scoping so refs only match when at the same indentation (or
reset current_url on dedent/blank). Modify the url regex handling to capture and
save the leading whitespace as url_indent (in addition to current_url), update
the ref-match check to capture the ref's leading whitespace and only proceed
when that indent equals url_indent (or when current_url is non-empty and
repo_matches "$current_url" and indent matches), and also clear
current_url/url_indent when a line with less indentation or an empty/top-level
line is seen so later refs won't reuse a stale URL (refer to variables
current_url, url_indent, prefix, old_ref and functions repo_matches and
get_latest_sha).
---
Nitpick comments:
In `@skills/update-refs/SKILL.md`:
- Around line 65-69: The branch example uses a date format that can collide;
update the BRANCH assignment and the example checkout in the SKILL.md snippet
(the lines setting BRANCH="update-refs-$(date +%Y%m%d)" and git checkout -b
$BRANCH) to use a full timestamp format such as %Y%m%d%H%M%S to ensure unique
branch names (e.g., change the date command to date +%Y%m%d%H%M%S).
🪄 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 Plus
Run ID: 893c097d-a72d-486e-a21d-5942a94489e5
📒 Files selected for processing (2)
skills/update-refs/SKILL.mdskills/update-refs/scripts/update_refs.sh
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
skills/update-refs/scripts/update_refs.sh (1)
29-43:⚠️ Potential issue | 🔴 CriticalHarden repo targeting and handle
cdfailure before mutating anything.Line 38–42 can still operate on the wrong repository, and Line 38 is also the current ShellCheck blocker (SC2164). Validate
--local-folderis the intendedapp-interfacerepo, and fail immediately ifcdfails.Proposed hardening
if [[ -z "$PATH_TO_APP_INTERFACE" ]]; then PATH_TO_APP_INTERFACE="/tmp/app-interface" if [[ -d "$PATH_TO_APP_INTERFACE/.git" ]]; then echo "App-interface repo already exists at $PATH_TO_APP_INTERFACE, skipping clone." else git clone --depth 1 git@gitlab.cee.redhat.com:service/app-interface.git "$PATH_TO_APP_INTERFACE" fi fi -cd "$PATH_TO_APP_INTERFACE" +if ! git -C "$PATH_TO_APP_INTERFACE" rev-parse --is-inside-work-tree >/dev/null 2>&1; then + echo "ERROR: $PATH_TO_APP_INTERFACE is not a git repository" >&2 + exit 1 +fi +origin_url="$(git -C "$PATH_TO_APP_INTERFACE" remote get-url origin 2>/dev/null || true)" +if [[ "$origin_url" != *"service/app-interface.git" && "$origin_url" != *"service/app-interface" ]]; then + echo "ERROR: $PATH_TO_APP_INTERFACE is not app-interface (origin=$origin_url)" >&2 + exit 1 +fi +cd "$PATH_TO_APP_INTERFACE" || exit 1 git fetch origin master git checkout master -git pull origin master +git pull --ff-only origin master
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/update-refs/scripts/update_refs.sh`:
- Line 7: The script currently uses "set -uo pipefail" which doesn't abort on
failing commands; update the shell options in the update_refs.sh startup (the
set invocation) to enable errexit as well (e.g., include -e or --errexit) so the
script fails fast on any command error, ensuring git/network errors stop
execution and prevent partial/stale updates.
🪄 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 Plus
Run ID: 86852d3a-c64e-4761-afca-e8857f050578
📒 Files selected for processing (1)
skills/update-refs/scripts/update_refs.sh
Description
Adding a skill to update the repo refs in app-interface.
Type of change
Testing steps
tested in https://gitlab.cee.redhat.com/service/app-interface/-/merge_requests/183901 (before fixing the
git add .that was too wide)