Replace bare git push with Claude-driven push phase in review-agent#80412
Replace bare git push with Claude-driven push phase in review-agent#80412enxebre wants to merge 6 commits into
Conversation
The push step now uses Claude to handle push failures (e.g., pre-push hooks, branch constraints) with up to 3 retries and minimal fixes, matching the agentic pattern used in other phases of the review-agent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughStandardizes Claude usage to pipe stream-json through ChangesReview agent process updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
/rehearse address-review-comments |
|
/rehearse periodic-ci-openshift-hypershift-main-periodic-review-agent |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh`:
- Around line 1305-1333: The script currently persists raw Claude transcripts
(PUSH_RESULT and /tmp/claude-pr-${PR_NUMBER}-push-output.json) while the fork
token is stored in git's printable credential.helper, risking credential
leakage; remove/stop writing raw transcripts to ARTIFACT_DIR and CI logs and
instead save only a redacted summary via extract_claude_summary, restrict the
temporary file's permissions, and move the fork token out of printable git
config into a non-printable helper (e.g., an askpass/helper script with
restrictive perms) so Claude cannot read credentials. Concretely: stop copying
/tmp/claude-pr-*-push-output.json to "${ARTIFACT_DIR}", ensure
extract_claude_summary produces and copies the sanitized
"${SHARED_DIR}/claude-pr-*-push-summary.json" only, do not echo or tail raw
$PUSH_RESULT to logs (replace those echo/tail lines with logging the redacted
summary), set restrictive mode (chmod 600) on any temp files, and change git
config usage of credential.helper to use an askpass/helper script instead of
embedding the fork token in the printable credential.helper.
- Around line 1323-1335: After PUSH_EXIT == 0, verify the remote branch actually
advanced by comparing the remote ref to the local HEAD (use git ls-remote origin
"${BRANCH_NAME}" and compare the returned commit hash to git rev-parse HEAD or
perform a git fetch origin "${BRANCH_NAME}" and compare
refs/remotes/origin/"${BRANCH_NAME}" to HEAD); if the hashes differ, treat the
push as failed: print a clear diagnostic (include PUSH_RESULT tail output and
the remote vs local hashes) and set PR_HAD_ERROR=true, otherwise proceed to
print the existing success messages for PR_NUMBER.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 18338b51-3a63-4141-9bab-0e8582aeca45
📒 Files selected for processing (1)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh
| PUSH_RESULT=$(claude -p "$PUSH_USER_PROMPT" \ | ||
| --system-prompt "$PUSH_SYSTEM_PROMPT" \ | ||
| --allowedTools "Bash Read Write Edit Grep Glob" \ | ||
| --max-turns 60 \ | ||
| --effort max \ | ||
| --model "$CLAUDE_MODEL" \ | ||
| --verbose \ | ||
| --output-format stream-json \ | ||
| < /dev/null \ | ||
| 2>&1 | tee "/tmp/claude-pr-${PR_NUMBER}-push-output.json") | ||
| PUSH_EXIT=$? | ||
| set -e | ||
|
|
||
| if [ -f "/tmp/claude-pr-${PR_NUMBER}-push-output.json" ]; then | ||
| cp "/tmp/claude-pr-${PR_NUMBER}-push-output.json" "${ARTIFACT_DIR}/claude-pr-${PR_NUMBER}-push-output.json" | ||
| extract_claude_summary "/tmp/claude-pr-${PR_NUMBER}-push-output.json" "${SHARED_DIR}/claude-pr-${PR_NUMBER}-push-summary.json" | ||
| fi | ||
|
|
||
| if [ $PUSH_EXIT -eq 0 ]; then | ||
| echo "Push completed for PR #$PR_NUMBER" | ||
| echo "Push phase succeeded for PR #$PR_NUMBER" | ||
| echo "" | ||
| echo "--- Claude push output for PR #$PR_NUMBER ---" | ||
| echo "$PUSH_RESULT" | tail -50 | ||
| echo "--- End Claude push output ---" | ||
| echo "" | ||
| else | ||
| echo "Push failed for PR #$PR_NUMBER" | ||
| echo "Push phase failed for PR #$PR_NUMBER" | ||
| echo "Error output (last 20 lines):" | ||
| echo "$PUSH_RESULT" | tail -20 |
There was a problem hiding this comment.
Don’t persist raw Claude push transcripts while the fork token is stored in printable git config.
At Line 817 the fork token is embedded directly in credential.helper. This new phase then gives Claude Bash access to diagnose push failures and copies/prints the raw transcript at Lines 1318-1333. If Claude ever inspects git config while debugging, that helper value will be written into artifacts/logs as a live push credential. The prompt ban on credential inspection is not an enforcement boundary; move auth out of printable git config (for example, an askpass/helper script with restricted permissions) and only persist a redacted summary here.
As per coding guidelines, step-registry command scripts must never print sensitive information to logs and should exclude sensitive credentials from CI logs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh`
around lines 1305 - 1333, The script currently persists raw Claude transcripts
(PUSH_RESULT and /tmp/claude-pr-${PR_NUMBER}-push-output.json) while the fork
token is stored in git's printable credential.helper, risking credential
leakage; remove/stop writing raw transcripts to ARTIFACT_DIR and CI logs and
instead save only a redacted summary via extract_claude_summary, restrict the
temporary file's permissions, and move the fork token out of printable git
config into a non-printable helper (e.g., an askpass/helper script with
restrictive perms) so Claude cannot read credentials. Concretely: stop copying
/tmp/claude-pr-*-push-output.json to "${ARTIFACT_DIR}", ensure
extract_claude_summary produces and copies the sanitized
"${SHARED_DIR}/claude-pr-*-push-summary.json" only, do not echo or tail raw
$PUSH_RESULT to logs (replace those echo/tail lines with logging the redacted
summary), set restrictive mode (chmod 600) on any temp files, and change git
config usage of credential.helper to use an askpass/helper script instead of
embedding the fork token in the printable credential.helper.
Source: Coding guidelines
| if [ $PUSH_EXIT -eq 0 ]; then | ||
| echo "Push completed for PR #$PR_NUMBER" | ||
| echo "Push phase succeeded for PR #$PR_NUMBER" | ||
| echo "" | ||
| echo "--- Claude push output for PR #$PR_NUMBER ---" | ||
| echo "$PUSH_RESULT" | tail -50 | ||
| echo "--- End Claude push output ---" | ||
| echo "" | ||
| else | ||
| echo "Push failed for PR #$PR_NUMBER" | ||
| echo "Push phase failed for PR #$PR_NUMBER" | ||
| echo "Error output (last 20 lines):" | ||
| echo "$PUSH_RESULT" | tail -20 | ||
| PR_HAD_ERROR=true | ||
| fi |
There was a problem hiding this comment.
Verify the remote branch before marking the push phase successful.
Right now Line 1323 treats Claude exit code 0 as proof that the push happened. That is weaker than the old direct git push contract: Claude can exit cleanly after diagnosing a failure or making local fixes without ever advancing origin/${BRANCH_NAME}, and this script will still write a success state for the PR. Add an explicit remote check here (git ls-remote origin "${BRANCH_NAME}" vs git rev-parse HEAD, or fetch and compare) before reporting success.
🛠 Suggested guard
- if [ $PUSH_EXIT -eq 0 ]; then
+ if [ $PUSH_EXIT -eq 0 ]; then
+ REMOTE_SHA=$(git ls-remote --heads origin "$BRANCH_NAME" | awk '{print $1}')
+ HEAD_SHA=$(git rev-parse HEAD)
+ if [ -z "$REMOTE_SHA" ] || [ "$REMOTE_SHA" != "$HEAD_SHA" ]; then
+ echo "Push verification failed for PR #$PR_NUMBER: remote branch was not updated"
+ PUSH_EXIT=1
+ fi
+ fi
+
+ if [ $PUSH_EXIT -eq 0 ]; then
echo "Push phase succeeded for PR #$PR_NUMBER"Based on the step reference and PR objective, this phase is responsible for actually pushing changes back to the fork.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh`
around lines 1323 - 1335, After PUSH_EXIT == 0, verify the remote branch
actually advanced by comparing the remote ref to the local HEAD (use git
ls-remote origin "${BRANCH_NAME}" and compare the returned commit hash to git
rev-parse HEAD or perform a git fetch origin "${BRANCH_NAME}" and compare
refs/remotes/origin/"${BRANCH_NAME}" to HEAD); if the hashes differ, treat the
push as failed: print a clear diagnostic (include PUSH_RESULT tail output and
the remote vs local hashes) and set PR_HAD_ERROR=true, otherwise proceed to
print the existing success messages for PR_NUMBER.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, enxebre The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/rehearse periodic-ci-openshift-hypershift-main-periodic-review-agent |
|
/pj-rehearse periodic-ci-openshift-hypershift-main-periodic-review-agent |
|
@enxebre: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
- Use ${PIPESTATUS[0]} instead of $? to capture claude's exit code
rather than tee's (which is always 0). Fixes all 4 phases: rebase,
review, CI fix, and push.
- Pre-compute PULL_BASE_SHA/PULL_PULL_SHA from local git history before
push so gitlint resolves commit ranges on the first attempt. CI sets
these env vars to commits from the rehearsal repo (openshift/release),
not hypershift, causing gitlint to fail with "Invalid revision range".
- Read phase output from tee'd files instead of $RESULT variables that
are no longer captured in command substitution.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh`:
- Around line 1286-1298: The current code computes PUSH_MERGE_BASE against a
hard-coded origin/main and only documents the gitlint override inside
PUSH_SYSTEM_PROMPT; change it to fetch the actual base ref resolved earlier
(BASE_BRANCH), compute the merge base from that fetched ref and HEAD (set
PUSH_MERGE_BASE=$(git merge-base HEAD <fetched-base-ref> || echo "HEAD~5")), set
PUSH_HEAD_SHA from git rev-parse HEAD as now (PUSH_HEAD_SHA), and export the
environment variables PULL_BASE_SHA=${PUSH_MERGE_BASE} and
PULL_PULL_SHA=${PUSH_HEAD_SHA} before Claude/agent runs so retries that run
plain git push inherit the overrides; keep PUSH_SYSTEM_PROMPT for instruction
text but do not rely on it as the sole mechanism.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f0620944-9a1a-4c6b-99ba-8a742cca2c5a
📒 Files selected for processing (1)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh
The process step was getting killed while dumping large stream-json output to the build log, before it could write processed-prs.txt. This left the report step with no data to generate a report from. Move STATE_FILE and actions JSON writes to immediately after phase completion, before the output dump section. Also reduce tail lines from 50 to 10 since each stream-json line can be 10KB+. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address CodeRabbit review: use the PR's actual base branch instead of hardcoded origin/main for the merge-base computation, and export PULL_BASE_SHA/PULL_PULL_SHA so all git push invocations (including retries) inherit them without relying on prompt instructions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/pj-rehearse periodic-ci-openshift-hypershift-main-periodic-review-agent |
|
@enxebre: your |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh (1)
1286-1308:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap
git pushin a guarded helper instead of encoding the contract in prompt text.
PULL_BASE_SHA/PULL_PULL_SHAare exported once before Claude starts, but this prompt explicitly allows commit rewrites and retries. After an amend/fixup, later pushes still inherit the old range, so hook checks can validate the wrong commits. The "Maximum 3 push attempts" limit is also only advisory right now—nothing in the shell stops a fourth push.🛠 Suggested approach
+ cat > /tmp/git-push-guard.sh <<EOF +#!/bin/bash +set -euo pipefail +attempt_file="/tmp/pr-${PR_NUMBER}-push-attempts" +attempt=$(( $(cat "$attempt_file" 2>/dev/null || echo 0) + 1 )) +if [ "$attempt" -gt 3 ]; then + echo "push attempt limit exceeded" >&2 + exit 1 +fi +echo "$attempt" > "$attempt_file" +export PULL_BASE_SHA="$(git merge-base HEAD "upstream/${BASE_BRANCH}" 2>/dev/null || echo HEAD~5)" +export PULL_PULL_SHA="$(git rev-parse HEAD)" +exec git push --force-with-lease origin "${BRANCH_NAME}" +EOF + chmod +x /tmp/git-push-guard.sh @@ -- Push command: git push --force-with-lease origin ${BRANCH_NAME} - PULL_BASE_SHA and PULL_PULL_SHA are already exported in the environment. +- Push command: /tmp/git-push-guard.shAlso applies to: 1313-1325
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh` around lines 1286 - 1308, Replace the inline push logic in the prompt by adding a guarded helper function (e.g., push_with_retries or guarded_push) that encapsulates the git push contract (uses git push --force-with-lease origin ${BRANCH_NAME}), enforces the MAX_ATTEMPTS (3) loop, re-exports/updates PULL_BASE_SHA and PULL_PULL_SHA before each attempt (so amended commits get correct range), inspects push failure output to detect hook/branch errors and returns actionable failure codes, attempts minimal automated fixes only when a specific hook error is detected, retries up to MAX_ATTEMPTS, and returns non-zero if all attempts fail; update PUSH_SYSTEM_PROMPT to reference this helper rather than encoding retry/max rules. Ensure the helper name (push_with_retries or guarded_push), the push command string, and the variables PULL_BASE_SHA and PULL_PULL_SHA are used so callers can find and invoke the new function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh`:
- Around line 1286-1308: Replace the inline push logic in the prompt by adding a
guarded helper function (e.g., push_with_retries or guarded_push) that
encapsulates the git push contract (uses git push --force-with-lease origin
${BRANCH_NAME}), enforces the MAX_ATTEMPTS (3) loop, re-exports/updates
PULL_BASE_SHA and PULL_PULL_SHA before each attempt (so amended commits get
correct range), inspects push failure output to detect hook/branch errors and
returns actionable failure codes, attempts minimal automated fixes only when a
specific hook error is detected, retries up to MAX_ATTEMPTS, and returns
non-zero if all attempts fail; update PUSH_SYSTEM_PROMPT to reference this
helper rather than encoding retry/max rules. Ensure the helper name
(push_with_retries or guarded_push), the push command string, and the variables
PULL_BASE_SHA and PULL_PULL_SHA are used so callers can find and invoke the new
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 032e2fdc-108d-46f5-96b8-4a8bbdec2907
📒 Files selected for processing (1)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/pj-rehearse periodic-ci-openshift-hypershift-main-periodic-review-agent |
|
@enxebre: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
The unguarded git rebase --abort on line 772 was killing the entire script under set -e when the CI entrypoint wrapper returned non-zero. This prevented the script from continuing to process remaining PRs after a rebase conflict. The same command on line 778 already had || true. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse periodic-ci-openshift-hypershift-main-periodic-review-agent |
|
@enxebre: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-hypershift-main-periodic-review-agent |
|
@enxebre: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@enxebre: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
The push step now uses Claude to handle push failures (e.g., pre-push hooks, branch constraints) with up to 3 retries and minimal fixes, matching the agentic pattern used in other phases of the review-agent.
Summary by CodeRabbit
This PR updates the HyperShift review-agent ci-operator step (ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh) to replace direct git pushes with a Claude-driven push phase and to harden Claude integration across the review-agent phases.
What changed in practical terms
Why this matters for the repo/CI
Files affected
Other notable details