update: add agent to do upstream sync#41
Conversation
📝 WalkthroughWalkthroughIntroduces a new agent instruction document providing a scripted workflow for syncing code from an upstream repository ( Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Security FindingsCWE-78 (OS Command Injection): Variables like CWE-74 (Improper Neutralization of Special Elements): The PR body includes a commit URL constructed from user-controlled values. Specify escaping/quoting requirements for the GitHub CLI invocation or validate URL structure before insertion. Missing branch name sanitization: The sync branch is constructed as Incomplete merge conflict guidance: Document instructs "manual resolution followed by add/commit" but doesn't explicitly warn against partial/dangerous merge states (e.g., staged unresolved conflicts, uncommitted changes). Specify mandatory 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/agents/sync-upstream.md:
- Around line 53-56: The duplicate-branch check currently only warns and
continues; change the behavior where the BRANCH existence check using git
show-ref (refs/heads/${BRANCH} or refs/remotes/origin/${BRANCH}) causes the
script to exit or prompt and then exit when a duplicate is detected; update the
block around the git show-ref condition so that after echo "Warning: branch
${BRANCH} already exists" the script either exits nonzero (e.g., exit 1) or asks
the user and exits on a negative response, ensuring no further branch creation
or PR attempts occur.
- Around line 103-105: The git checkout back to the original branch is currently
inside the PR-only block so it doesn't always run; move the git checkout
"${ORIGINAL_BRANCH}" out of the gh pr create conditional path so it always runs
after the push flow, and update the gh pr create failure branch to print a
message that the branch was pushed to origin/${BRANCH} and include the manual PR
URL
https://github.com/opendatahub-io/workload-variant-autoscaler/compare/main...${FORK_OWNER}:${BRANCH}
so users can create the PR manually before or after you run git checkout
"${ORIGINAL_BRANCH}".
- Around line 46-49: Enable strict shell mode and validate commits explicitly:
add "set -euo pipefail" at the top of the script to fail fast on empty vars;
replace the git rev-parse calls for FULL_SHA and SHORT_SHA (TARGET_COMMIT,
FULL_SHA, SHORT_SHA) with git rev-parse --verify --quiet and check their exit
status so you don't proceed with empty values; also replace the merge-base check
with git merge-base --is-ancestor "${FULL_SHA}" upstream/main || { echo "Error:
commit not on upstream/main"; exit 1; } to ensure the script exits immediately
on invalid refs.
- Around line 31-35: The origin-owner validation is too permissive and currently
can be bypassed when authentication fails; update the script so that after
computing ORIGIN_URL and extracting ORIGIN_OWNER (from ORIGIN_URL), you must
obtain the authenticated GitHub user (via `gh auth status`/`gh api user` or
equivalent) and explicitly compare ORIGIN_OWNER to that authenticated username,
and if authentication is unavailable or the owner does not match, exit non-zero
with an explicit error; remove any `|| true` fallback that silences `gh`
failures so authentication failures cause the validation to fail rather than
pass.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7dd3bdb1-3c31-4a0c-857e-435eecf903aa
📒 Files selected for processing (1)
.claude/agents/sync-upstream.md
| ORIGIN_URL=$(git remote get-url origin) | ||
| if echo "${ORIGIN_URL}" | grep -qE '(llm-d/llm-d-workload-variant-autoscaler|opendatahub-io/workload-variant-autoscaler)'; then | ||
| echo "Error: origin remote points to upstream or opendatahub, not your fork" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the sync-upstream.md file
fd sync-upstream.mdRepository: opendatahub-io/workload-variant-autoscaler
Length of output: 68
🏁 Script executed:
# Search more broadly for the file
find . -name "sync-upstream.md" -type f 2>/dev/nullRepository: opendatahub-io/workload-variant-autoscaler
Length of output: 119
🏁 Script executed:
# Read the sync-upstream.md file
cat -n ./.claude/agents/sync-upstream.mdRepository: opendatahub-io/workload-variant-autoscaler
Length of output: 5906
🏁 Script executed:
# Check if there are other protections or if origin validation is the only gate
grep -n "origin" ./.claude/agents/sync-upstream.md | head -20Repository: opendatahub-io/workload-variant-autoscaler
Length of output: 984
🏁 Script executed:
# Verify the sed pattern works for various GitHub URL formats
python3 << 'EOF'
import re
import subprocess
test_urls = [
"https://github.com/myuser/workload-variant-autoscaler.git",
"git@github.com:myuser/workload-variant-autoscaler.git",
"https://github.com/attacker/workload-variant-autoscaler",
"git@github.com:attacker/workload-variant-autoscaler",
]
# Test the sed pattern from the script
pattern = r'.*[:/]([^/]+)/[^/]+(.git)?$'
for url in test_urls:
match = re.search(pattern, url)
if match:
owner = match.group(1)
print(f"{url} -> owner: {owner}")
else:
print(f"{url} -> NO MATCH")
EOFRepository: opendatahub-io/workload-variant-autoscaler
Length of output: 377
🏁 Script executed:
# Check if gh CLI authentication requirement is mentioned anywhere in the script
grep -i "gh\|auth\|github\|login\|credential" ./.claude/agents/sync-upstream.mdRepository: opendatahub-io/workload-variant-autoscaler
Length of output: 1000
🏁 Script executed:
# Verify that gh pr create later in script also requires authentication
sed -n '94,95p' ./.claude/agents/sync-upstream.mdRepository: opendatahub-io/workload-variant-autoscaler
Length of output: 154
Fix origin ownership validation (CWE-20: Improper Input Validation).
Negative-only validation allows arbitrary third-party origins. An attacker with git config control could set origin to attacker/workload-variant-autoscaler, causing the sync branch to push to an unintended repo.
However, the suggested patch has a critical flaw: the || true fallback bypasses the ownership check if gh auth fails, silently allowing mismatched origins. Since the script already requires gh pr create later (making authentication a hard dependency), the validation should fail explicitly, not silently pass on auth failures.
Recommended fix: Compare ORIGIN_OWNER against authenticated user without fallback, or reject entirely if authentication is unavailable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/sync-upstream.md around lines 31 - 35, The origin-owner
validation is too permissive and currently can be bypassed when authentication
fails; update the script so that after computing ORIGIN_URL and extracting
ORIGIN_OWNER (from ORIGIN_URL), you must obtain the authenticated GitHub user
(via `gh auth status`/`gh api user` or equivalent) and explicitly compare
ORIGIN_OWNER to that authenticated username, and if authentication is
unavailable or the owner does not match, exit non-zero with an explicit error;
remove any `|| true` fallback that silences `gh` failures so authentication
failures cause the validation to fail rather than pass.
| TARGET_COMMIT="${USER_SHA:-upstream/main}" | ||
| FULL_SHA=$(git rev-parse "${TARGET_COMMIT}") | ||
| SHORT_SHA=$(git rev-parse --short "${TARGET_COMMIT}") | ||
| git merge-base --is-ancestor "${FULL_SHA}" upstream/main || { echo "Error: commit not on upstream/main"; exit 1; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .claude/agents/sync-upstream.md | head -80Repository: opendatahub-io/workload-variant-autoscaler
Length of output: 4058
🏁 Script executed:
# Check if set -euo pipefail or other strict mode is present in the file
rg "set -[euo]" .claude/agents/sync-upstream.md -A 2 -B 2Repository: opendatahub-io/workload-variant-autoscaler
Length of output: 68
🏁 Script executed:
# Get the full context around lines 46-49
sed -n '30,60p' .claude/agents/sync-upstream.mdRepository: opendatahub-io/workload-variant-autoscaler
Length of output: 1545
Add strict shell mode and explicit commit validation to prevent misleading error states.
Lines 47–48 silently fail without set -e, leaving empty variables that trigger unrelated errors downstream. Use --verify --quiet for explicit commit validation and fail immediately on invalid refs.
Suggested patch
+# Fail fast on command errors and unset vars
+set -euo pipefail
+
# 3. Resolve target commit
TARGET_COMMIT="${USER_SHA:-upstream/main}"
-FULL_SHA=$(git rev-parse "${TARGET_COMMIT}")
-SHORT_SHA=$(git rev-parse --short "${TARGET_COMMIT}")
+FULL_SHA=$(git rev-parse --verify --quiet "${TARGET_COMMIT}^{commit}") || {
+ echo "Error: invalid commit '${TARGET_COMMIT}'"
+ exit 1
+}
+SHORT_SHA=$(git rev-parse --short "${FULL_SHA}")
git merge-base --is-ancestor "${FULL_SHA}" upstream/main || { echo "Error: commit not on upstream/main"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/sync-upstream.md around lines 46 - 49, Enable strict shell
mode and validate commits explicitly: add "set -euo pipefail" at the top of the
script to fail fast on empty vars; replace the git rev-parse calls for FULL_SHA
and SHORT_SHA (TARGET_COMMIT, FULL_SHA, SHORT_SHA) with git rev-parse --verify
--quiet and check their exit status so you don't proceed with empty values; also
replace the merge-base check with git merge-base --is-ancestor "${FULL_SHA}"
upstream/main || { echo "Error: commit not on upstream/main"; exit 1; } to
ensure the script exits immediately on invalid refs.
| if git show-ref --verify --quiet "refs/heads/${BRANCH}" || git show-ref --verify --quiet "refs/remotes/origin/${BRANCH}"; then | ||
| echo "Warning: branch ${BRANCH} already exists" | ||
| # Ask user whether to force-update or skip | ||
| fi |
There was a problem hiding this comment.
Duplicate detection does not enforce stop behavior.
Line 54 warns but continues. That conflicts with the documented workflow (“inform the user and stop”) and can create duplicate sync branches/PR attempts.
Suggested patch
if git show-ref --verify --quiet "refs/heads/${BRANCH}" || git show-ref --verify --quiet "refs/remotes/origin/${BRANCH}"; then
- echo "Warning: branch ${BRANCH} already exists"
- # Ask user whether to force-update or skip
+ echo "Error: branch ${BRANCH} already exists"
+ exit 1
fi📝 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.
| if git show-ref --verify --quiet "refs/heads/${BRANCH}" || git show-ref --verify --quiet "refs/remotes/origin/${BRANCH}"; then | |
| echo "Warning: branch ${BRANCH} already exists" | |
| # Ask user whether to force-update or skip | |
| fi | |
| if git show-ref --verify --quiet "refs/heads/${BRANCH}" || git show-ref --verify --quiet "refs/remotes/origin/${BRANCH}"; then | |
| echo "Error: branch ${BRANCH} already exists" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/sync-upstream.md around lines 53 - 56, The duplicate-branch
check currently only warns and continues; change the behavior where the BRANCH
existence check using git show-ref (refs/heads/${BRANCH} or
refs/remotes/origin/${BRANCH}) causes the script to exit or prompt and then exit
when a duplicate is detected; update the block around the git show-ref condition
so that after echo "Warning: branch ${BRANCH} already exists" the script either
exits nonzero (e.g., exit 1) or asks the user and exits on a negative response,
ensuring no further branch creation or PR attempts occur.
zdtsw
force-pushed
the
agent_sync_upstream
branch
from
61e4b66 to
4ae6a00
Compare
Signed-off-by: Wen Zhou <wenzhou@redhat.com>
zdtsw
force-pushed
the
agent_sync_upstream
branch
2 times, most recently
from
27a7cca to
0d802ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
.claude/agents/sync-upstream.md (3)
55-60:⚠️ Potential issue | 🟠 MajorEnforce positive origin ownership validation (CWE-20).
Line 57 uses blacklist matching only; any third-party repo URL passes. Bind
originowner to the authenticated GitHub user and fail closed if auth is unavailable or owner mismatch.Proposed fix
ORIGIN_URL=$(git -C "${REPO_PATH}" remote get-url origin) if echo "${ORIGIN_URL}" | grep -qE '(llm-d/llm-d-workload-variant-autoscaler|opendatahub-io/workload-variant-autoscaler)'; then echo "Error: origin remote points to upstream or opendatahub, not your fork" exit 1 fi +AUTH_USER=$(gh api user -q .login) || { echo "Error: GitHub auth required"; exit 1; } +ORIGIN_OWNER=$(echo "${ORIGIN_URL}" | sed -E 's#.*[:/]([^/]+)/[^/]+(\.git)?$#\1#') +if [ "${ORIGIN_OWNER}" != "${AUTH_USER}" ]; then + echo "Error: origin owner (${ORIGIN_OWNER}) does not match authenticated user (${AUTH_USER})" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/sync-upstream.md around lines 55 - 60, The origin validation currently uses a blacklist; change it to verify the origin owner matches the authenticated GitHub user by querying the GitHub API for the authenticated username and comparing it to the owner parsed from ORIGIN_URL (variables/functions involved: ORIGIN_URL, REPO_PATH, origin remote). If authentication to GitHub fails or the owners do not match, exit non‑zero (fail closed). Update the script to obtain a token from the environment, call the GitHub user endpoint to get the authenticated login, parse the owner from ORIGIN_URL, and enforce owner equality before proceeding.
50-74:⚠️ Potential issue | 🟠 MajorFail fast and validate commit refs explicitly before using them.
Lines 72-73 use
rev-parsewithout explicit--verify --quiet; invalid refs can produce misleading downstream failures. Add strict mode and commit-object verification.Proposed fix
```bash +# Fail fast on command errors and unset variables +set -euo pipefail + # 0. Detect repository path and save current branch REPO_PATH=$(git rev-parse --show-toplevel) @@ TARGET_COMMIT="${USER_SHA:-upstream/main}" -FULL_SHA=$(git -C "${REPO_PATH}" rev-parse "${TARGET_COMMIT}") -SHORT_SHA=$(git -C "${REPO_PATH}" rev-parse --short "${TARGET_COMMIT}") +FULL_SHA=$(git -C "${REPO_PATH}" rev-parse --verify --quiet "${TARGET_COMMIT}^{commit}") || { + echo "Error: invalid commit '${TARGET_COMMIT}'" + exit 1 +} +SHORT_SHA=$(git -C "${REPO_PATH}" rev-parse --short "${FULL_SHA}") git -C "${REPO_PATH}" merge-base --is-ancestor "${FULL_SHA}" upstream/main || {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/sync-upstream.md around lines 50 - 74, Enable strict bash mode and validate commit refs before using them: add set -euo pipefail at the top, and replace the current rev-parse calls that compute FULL_SHA and SHORT_SHA for TARGET_COMMIT with a verified rev-parse (use --verify --quiet and append ^{commit}) that fails fast and prints a clear "Error: invalid commit '<TARGET_COMMIT>'" before exiting; then derive SHORT_SHA from the validated FULL_SHA and keep the existing merge-base --is-ancestor check as-is.
87-91:⚠️ Potential issue | 🟠 MajorStop on duplicate sync branch instead of continuing.
Line 89 warns but continues, which contradicts the documented behavior in Line 36 (“inform the user and stop”) and risks duplicate branch/PR attempts.
Proposed fix
if git -C "${REPO_PATH}" show-ref --verify --quiet "refs/heads/${BRANCH}" || \ git -C "${REPO_PATH}" show-ref --verify --quiet "refs/remotes/origin/${BRANCH}"; then - echo "Warning: branch ${BRANCH} already exists" - # Ask user whether to force-update or skip + echo "Error: branch ${BRANCH} already exists" + exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/sync-upstream.md around lines 87 - 91, The conditional that detects an existing branch (the git -C "${REPO_PATH}" show-ref --verify --quiet "refs/heads/${BRANCH}" || git -C "${REPO_PATH}" show-ref --verify --quiet "refs/remotes/origin/${BRANCH}") currently only echoes a warning and continues; change it to stop the script instead: after echo "Warning: branch ${BRANCH} already exists" call a non-zero exit (e.g., exit 1) or otherwise return/fail so execution does not proceed with branch/PR creation for the same BRANCH and REPO_PATH.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/agents/sync-upstream.md:
- Around line 189-198: The fenced code block in .claude/agents/sync-upstream.md
(the block starting with ``` that contains the Sync completed message and bullet
list) lacks a language tag; update the opening fence from ``` to ```text so
markdownlint MD040 is satisfied and the block is explicitly marked as plain
text. Ensure only the opening fence is changed and the closing ``` remains
unchanged.
- Around line 24-35: The fenced code block beginning with "Target: upstream/main
HEAD (<short_sha>)" is missing a language identifier; update the opening
triple-backticks for that block to include a language token (e.g., change ``` to
```text or ```bash) so markdownlint rule MD040 is satisfied and the block is
properly annotated.
- Around line 163-174: The workflow uses the command symbol `gh pr create` which
is not permitted by the local allowlist in `.claude/settings.local.json` (only
`gh pr view` and `gh pr list` are allowed); to fix this, replace or gate the `gh
pr create` invocation in the sync script so it either uses an allowed
alternative (e.g., instruct users to open the PR manually using the
repository/branch variables `ODH_BRANCH`, `FORK_OWNER`, `BRANCH`, `SHORT_SHA`,
`FULL_SHA`) or update `.claude/settings.local.json` to include `gh pr create` in
its allowlist, ensuring the script and permission file are consistent.
- Around line 168-173: The heredoc is quoted with <<'EOF' which prevents
expansion of ${ODH_BRANCH} and ${FULL_SHA}; update the --body heredoc (the cat
<<'EOF' ... EOF block used inside the --body "$(cat ... )" command) to use an
unquoted delimiter (<<EOF) so those variables are expanded at runtime, keeping
the surrounding command substitution and EOF terminator intact.
---
Duplicate comments:
In @.claude/agents/sync-upstream.md:
- Around line 55-60: The origin validation currently uses a blacklist; change it
to verify the origin owner matches the authenticated GitHub user by querying the
GitHub API for the authenticated username and comparing it to the owner parsed
from ORIGIN_URL (variables/functions involved: ORIGIN_URL, REPO_PATH, origin
remote). If authentication to GitHub fails or the owners do not match, exit
non‑zero (fail closed). Update the script to obtain a token from the
environment, call the GitHub user endpoint to get the authenticated login, parse
the owner from ORIGIN_URL, and enforce owner equality before proceeding.
- Around line 50-74: Enable strict bash mode and validate commit refs before
using them: add set -euo pipefail at the top, and replace the current rev-parse
calls that compute FULL_SHA and SHORT_SHA for TARGET_COMMIT with a verified
rev-parse (use --verify --quiet and append ^{commit}) that fails fast and prints
a clear "Error: invalid commit '<TARGET_COMMIT>'" before exiting; then derive
SHORT_SHA from the validated FULL_SHA and keep the existing merge-base
--is-ancestor check as-is.
- Around line 87-91: The conditional that detects an existing branch (the git -C
"${REPO_PATH}" show-ref --verify --quiet "refs/heads/${BRANCH}" || git -C
"${REPO_PATH}" show-ref --verify --quiet "refs/remotes/origin/${BRANCH}")
currently only echoes a warning and continues; change it to stop the script
instead: after echo "Warning: branch ${BRANCH} already exists" call a non-zero
exit (e.g., exit 1) or otherwise return/fail so execution does not proceed with
branch/PR creation for the same BRANCH and REPO_PATH.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6a66a13f-cce2-44b4-80e6-c040e9e4a0f4
📒 Files selected for processing (2)
.claude/agents/sync-upstream.md.claude/settings.local.json
✅ Files skipped from review due to trivial changes (1)
- .claude/settings.local.json
| gh pr create \ | ||
| --repo opendatahub-io/workload-variant-autoscaler \ | ||
| --base "${ODH_BRANCH}" \ | ||
| --head "${FORK_OWNER}:${BRANCH}" \ | ||
| --title "[sync] upstream llm-d main branch ${SHORT_SHA} [$(date -u +%Y-%m-%d)]" \ | ||
| --body "$(cat <<'EOF' | ||
| Syncs llm-d/llm-d-workload-variant-autoscaler main branch into ODH ${ODH_BRANCH} branch. | ||
|
|
||
| Upstream commit: https://github.com/llm-d/llm-d-workload-variant-autoscaler/commit/${FULL_SHA} | ||
| EOF | ||
| )" | ||
| ``` |
There was a problem hiding this comment.
gh pr create is documented but not permitted by local allowlist.
Lines 163-174 require gh pr create, but .claude/settings.local.json only allows gh pr view and gh pr list. This flow will fail under the configured permissions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/sync-upstream.md around lines 163 - 174, The workflow uses
the command symbol `gh pr create` which is not permitted by the local allowlist
in `.claude/settings.local.json` (only `gh pr view` and `gh pr list` are
allowed); to fix this, replace or gate the `gh pr create` invocation in the sync
script so it either uses an allowed alternative (e.g., instruct users to open
the PR manually using the repository/branch variables `ODH_BRANCH`,
`FORK_OWNER`, `BRANCH`, `SHORT_SHA`, `FULL_SHA`) or update
`.claude/settings.local.json` to include `gh pr create` in its allowlist,
ensuring the script and permission file are consistent.
| --body "$(cat <<'EOF' | ||
| Syncs llm-d/llm-d-workload-variant-autoscaler main branch into ODH ${ODH_BRANCH} branch. | ||
|
|
||
| Upstream commit: https://github.com/llm-d/llm-d-workload-variant-autoscaler/commit/${FULL_SHA} | ||
| EOF | ||
| )" |
There was a problem hiding this comment.
Single-quoted heredoc prevents ${ODH_BRANCH} and ${FULL_SHA} expansion.
Line 168 uses <<'EOF', so template variables stay literal in the PR body instead of rendering runtime values.
Proposed fix
- --body "$(cat <<'EOF'
+ --body "$(cat <<EOF
Syncs llm-d/llm-d-workload-variant-autoscaler main branch into ODH ${ODH_BRANCH} branch.
Upstream commit: https://github.com/llm-d/llm-d-workload-variant-autoscaler/commit/${FULL_SHA}
EOF
)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/sync-upstream.md around lines 168 - 173, The heredoc is
quoted with <<'EOF' which prevents expansion of ${ODH_BRANCH} and ${FULL_SHA};
update the --body heredoc (the cat <<'EOF' ... EOF block used inside the --body
"$(cat ... )" command) to use an unquoted delimiter (<<EOF) so those variables
are expanded at runtime, keeping the surrounding command substitution and EOF
terminator intact.
| ``` | ||
| Sync completed successfully | ||
|
|
||
| - Branch: sync/upstream-<short_sha> pushed to origin | ||
| - PR: opendatahub-io/workload-variant-autoscaler#<pr_number> | ||
| - URL: <pr_url> | ||
| - Syncs: <N> upstream commits from llm-d/llm-d-workload-variant-autoscaler main (<short_sha>) into opendatahub-io ${ODH_BRANCH} | ||
| - Conflict resolved: <file> (<resolution details>) ← only if conflicts were resolved | ||
| - Returned to: <original_branch> branch | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block (MD040).
Line 189 starts a fenced block without a language tag. Use ```text to satisfy markdownlint.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 189-189: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/sync-upstream.md around lines 189 - 198, The fenced code
block in .claude/agents/sync-upstream.md (the block starting with ``` that
contains the Sync completed message and bullet list) lacks a language tag;
update the opening fence from ``` to ```text so markdownlint MD040 is satisfied
and the block is explicitly marked as plain text. Ensure only the opening fence
is changed and the closing ``` remains unchanged.
- use ODH main as target bracnh Signed-off-by: Wen Zhou <wenzhou@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (6)
.claude/agents/sync-upstream.md (6)
189-198:⚠️ Potential issue | 🟡 MinorAdd a language tag to the completion-summary fence (MD040).
This fenced block is also missing a language identifier; set it to
text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/sync-upstream.md around lines 189 - 198, The fenced block labeled starting with "Sync completed successfully" is missing a language identifier which triggers MD040; update the triple-backtick fence in .claude/agents/sync-upstream.md around that completion-summary block to use a language tag ```text so the fence becomes ```text and then close with ``` to satisfy the linter.
70-74:⚠️ Potential issue | 🟠 MajorValidate target commit explicitly and fail fast.
rev-parsewithout--verify --quietcan cascade into misleading downstream failures. Validate${TARGET_COMMIT}as a commit object before computing short SHA.Patch
TARGET_COMMIT="${USER_SHA:-upstream/main}" -FULL_SHA=$(git -C "${REPO_PATH}" rev-parse "${TARGET_COMMIT}") -SHORT_SHA=$(git -C "${REPO_PATH}" rev-parse --short "${TARGET_COMMIT}") +FULL_SHA=$(git -C "${REPO_PATH}" rev-parse --verify --quiet "${TARGET_COMMIT}^{commit}") || { + echo "Error: invalid commit '${TARGET_COMMIT}'" + exit 1 +} +SHORT_SHA=$(git -C "${REPO_PATH}" rev-parse --short "${FULL_SHA}") git -C "${REPO_PATH}" merge-base --is-ancestor "${FULL_SHA}" upstream/main || { echo "Error: commit not on upstream/main"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/sync-upstream.md around lines 70 - 74, The TARGET_COMMIT is not being validated before using git rev-parse which can produce misleading downstream failures; add an explicit validation step that runs git -C "${REPO_PATH}" rev-parse --verify --quiet "${TARGET_COMMIT}^{commit}" (or similar) and if it fails, log a clear error and exit non‑zero before computing FULL_SHA or SHORT_SHA; then compute FULL_SHA and SHORT_SHA with rev-parse as before and proceed to the merge-base check (references: TARGET_COMMIT, FULL_SHA, SHORT_SHA, and the git rev-parse/merge-base calls).
168-173:⚠️ Potential issue | 🟡 MinorUnquote heredoc delimiter so PR body variables expand.
<<'EOF'keeps${ODH_BRANCH}and${FULL_SHA}literal in PR body. Use unquoted delimiter.Patch
- --body "$(cat <<'EOF' + --body "$(cat <<EOF Syncs llm-d/llm-d-workload-variant-autoscaler main branch into ODH ${ODH_BRANCH} branch. Upstream commit: https://github.com/llm-d/llm-d-workload-variant-autoscaler/commit/${FULL_SHA} EOF )"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/sync-upstream.md around lines 168 - 173, The heredoc in the --body construction uses a quoted delimiter (<<'EOF') which prevents expansion of ${ODH_BRANCH} and ${FULL_SHA}; edit the snippet that builds the PR body (the --body "$(cat <<'EOF' ... EOF )" block) to use an unquoted delimiter (<<EOF) so the variables ODH_BRANCH and FULL_SHA are expanded into the PR body at runtime.
55-60:⚠️ Potential issue | 🟠 MajorHarden
originownership validation (CWE-20).Negative-only matching allows arbitrary third-party forks as
origin; this can push sync branches to unintended repos. Explicitly resolve authenticated GitHub user and requireORIGIN_OWNER == AUTH_USER; fail closed if auth lookup fails.Patch
ORIGIN_URL=$(git -C "${REPO_PATH}" remote get-url origin) if echo "${ORIGIN_URL}" | grep -qE '(llm-d/llm-d-workload-variant-autoscaler|opendatahub-io/workload-variant-autoscaler)'; then echo "Error: origin remote points to upstream or opendatahub, not your fork" exit 1 fi +ORIGIN_OWNER=$(echo "${ORIGIN_URL}" | sed -E 's#.*[:/]([^/]+)/[^/]+(\.git)?$#\1#') +AUTH_USER=$(gh api user -q .login 2>/dev/null) || { + echo "Error: unable to determine authenticated GitHub user" + exit 1 +} +[ "${ORIGIN_OWNER}" = "${AUTH_USER}" ] || { + echo "Error: origin owner '${ORIGIN_OWNER}' does not match authenticated user '${AUTH_USER}'" + exit 1 +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/sync-upstream.md around lines 55 - 60, The origin ownership check is too permissive; update the script to resolve the authenticated GitHub user and verify the origin owner matches that user: fetch ORIGIN_URL from origin (already via ORIGIN_URL variable), parse ORIGIN_OWNER from that URL, obtain AUTH_USER via an authenticated git/GitHub API call or `git config --get` token-based lookup, and then require ORIGIN_OWNER == AUTH_USER failing closed (exit non-zero) if the auth lookup fails or owners differ; update the check that currently examines ORIGIN_URL to instead compare ORIGIN_OWNER and AUTH_USER and emit a clear error message referencing ORIGIN_URL, ORIGIN_OWNER, and AUTH_USER when rejecting.
24-35:⚠️ Potential issue | 🟡 MinorAdd a language tag to the fenced block to satisfy MD040.
The code fence is untyped; markdownlint will keep failing. Change opening fence to
```text(or```bashif executable).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/sync-upstream.md around lines 24 - 35, Update the untyped fenced code block that starts with "Target: upstream/main HEAD" so the opening fence includes a language tag (e.g., change the leading "```" to "```text" or "```bash"); this satisfies MD040 and prevents markdownlint failures—locate the triple-backtick block containing the lines "Target: upstream/main HEAD", "Into: opendatahub-io/<repo-name> ${ODH_BRANCH} branch", and the commit/file summary, and add the chosen tag to the opening fence.
87-91:⚠️ Potential issue | 🟠 MajorStop execution when duplicate sync branch exists.
Current behavior warns and continues, which can produce duplicate branch/PR attempts despite the documented “stop” behavior.
Patch
if git -C "${REPO_PATH}" show-ref --verify --quiet "refs/heads/${BRANCH}" || \ git -C "${REPO_PATH}" show-ref --verify --quiet "refs/remotes/origin/${BRANCH}"; then - echo "Warning: branch ${BRANCH} already exists" - # Ask user whether to force-update or skip + echo "Error: branch ${BRANCH} already exists" + exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/sync-upstream.md around lines 87 - 91, The current check that detects an existing branch (using git -C "${REPO_PATH}" show-ref --verify --quiet "refs/heads/${BRANCH}" and refs/remotes/origin/${BRANCH}) only echoes a warning and continues; change it to stop execution immediately by emitting an error and exiting non-zero (e.g., call echo to stderr or use printf >&2 and then exit 1) so the script does not proceed to create duplicate branches/PRs; update the block around the git show-ref checks (the REPO_PATH and BRANCH variables) to perform the error message and exit instead of continuing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.claude/agents/sync-upstream.md:
- Around line 189-198: The fenced block labeled starting with "Sync completed
successfully" is missing a language identifier which triggers MD040; update the
triple-backtick fence in .claude/agents/sync-upstream.md around that
completion-summary block to use a language tag ```text so the fence becomes
```text and then close with ``` to satisfy the linter.
- Around line 70-74: The TARGET_COMMIT is not being validated before using git
rev-parse which can produce misleading downstream failures; add an explicit
validation step that runs git -C "${REPO_PATH}" rev-parse --verify --quiet
"${TARGET_COMMIT}^{commit}" (or similar) and if it fails, log a clear error and
exit non‑zero before computing FULL_SHA or SHORT_SHA; then compute FULL_SHA and
SHORT_SHA with rev-parse as before and proceed to the merge-base check
(references: TARGET_COMMIT, FULL_SHA, SHORT_SHA, and the git
rev-parse/merge-base calls).
- Around line 168-173: The heredoc in the --body construction uses a quoted
delimiter (<<'EOF') which prevents expansion of ${ODH_BRANCH} and ${FULL_SHA};
edit the snippet that builds the PR body (the --body "$(cat <<'EOF' ... EOF )"
block) to use an unquoted delimiter (<<EOF) so the variables ODH_BRANCH and
FULL_SHA are expanded into the PR body at runtime.
- Around line 55-60: The origin ownership check is too permissive; update the
script to resolve the authenticated GitHub user and verify the origin owner
matches that user: fetch ORIGIN_URL from origin (already via ORIGIN_URL
variable), parse ORIGIN_OWNER from that URL, obtain AUTH_USER via an
authenticated git/GitHub API call or `git config --get` token-based lookup, and
then require ORIGIN_OWNER == AUTH_USER failing closed (exit non-zero) if the
auth lookup fails or owners differ; update the check that currently examines
ORIGIN_URL to instead compare ORIGIN_OWNER and AUTH_USER and emit a clear error
message referencing ORIGIN_URL, ORIGIN_OWNER, and AUTH_USER when rejecting.
- Around line 24-35: Update the untyped fenced code block that starts with
"Target: upstream/main HEAD" so the opening fence includes a language tag (e.g.,
change the leading "```" to "```text" or "```bash"); this satisfies MD040 and
prevents markdownlint failures—locate the triple-backtick block containing the
lines "Target: upstream/main HEAD", "Into: opendatahub-io/<repo-name>
${ODH_BRANCH} branch", and the commit/file summary, and add the chosen tag to
the opening fence.
- Around line 87-91: The current check that detects an existing branch (using
git -C "${REPO_PATH}" show-ref --verify --quiet "refs/heads/${BRANCH}" and
refs/remotes/origin/${BRANCH}) only echoes a warning and continues; change it to
stop execution immediately by emitting an error and exiting non-zero (e.g., call
echo to stderr or use printf >&2 and then exit 1) so the script does not proceed
to create duplicate branches/PRs; update the block around the git show-ref
checks (the REPO_PATH and BRANCH variables) to perform the error message and
exit instead of continuing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 931b5c76-4db3-48d4-bd83-4a1cd12ffa5c
📒 Files selected for processing (1)
.claude/agents/sync-upstream.md
1 participant
Description
How Has This Been Tested?
outcome
#64
ref https://redhat.atlassian.net/browse/INFERENG-5594
Merge criteria:
Summary by CodeRabbit