CNTRLPLANE-3757: review-agent: trigger periodic via gangway instead of running inline#81366
Conversation
The review-agent presubmit has a design flaw: when the agent pushes commits to the PR branch, the presubmit job aborts itself. This change splits the architecture: - Add a periodic-review-agent periodic job (cron: @Yearly, triggered only via gangway) that runs the full hypershift-review-agent workflow - Add a lightweight hypershift-review-agent-trigger step that: 1. Triggers the periodic job via gangway API with the PR number 2. Posts a comment on the PR with a link to the triggered job - Update all presubmit configs (openshift + openshift-priv) to use the trigger workflow instead of running the agent inline The gangway token is read from the hypershift-team-claude-prow secret. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The REVIEW_AGENT_TARGET_PR env var should not have a hardcoded default. The periodic job receives the PR number from gangway via MULTISTAGE_PARAM_OVERRIDE_REVIEW_AGENT_TARGET_PR, and the process script already fails fast if no PR number is provided. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Declare MULTISTAGE_PARAM_OVERRIDE_REVIEW_AGENT_TARGET_PR in the ref YAML and apply it in the script, matching the jira-agent pattern for gangway API overrides. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThis PR adds a new Gangway-backed HyperShift review-agent trigger workflow, updates the review-agent process step to accept a multistage PR override, and repoints ChangesHyperShift review-agent trigger flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ci-operator/step-registry/hypershift/review-agent/trigger/hypershift-review-agent-trigger-commands.sh (1)
96-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
JOB_STATUSis computed but never used.Shellcheck flags this (SC2034). Consider wiring it into the final summary output for better observability instead of discarding it.
♻️ Proposed fix
echo "=== Trigger Complete ===" echo "Job ID: ${JOB_ID}" echo "Job URL: ${JOB_URL:-pending}" +echo "Job Status: ${JOB_STATUS:-unknown}"Also applies to: 136-138
🤖 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/trigger/hypershift-review-agent-trigger-commands.sh` around lines 96 - 97, The script computes JOB_STATUS in the trigger flow but never uses it, which triggers the unused-variable warning; update the review-agent trigger logic so JOB_STATUS is included in the final summary/output instead of being discarded. Make the change in the STATUS_BODY parsing and the related summary emission paths in hypershift-review-agent-trigger-commands.sh, including the duplicate status handling later in the script, so the collected job status is surfaced consistently.Source: Linters/SAST tools
🤖 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/config/openshift/hypershift/openshift-hypershift-main.yaml`:
- Around line 574-578: Regenerate the periodic job definitions for the
hypershift main config so the missing periodic-review-agent job is added to the
generated periodics file. Update the generated job output associated with the
hypershift-review-agent workflow and ensure the PERIODIC_JOB_NAME target
periodic-ci-openshift-hypershift-main-periodic-review-agent is present in
ci-operator/jobs/openshift/hypershift/openshift-hypershift-main-periodics.yaml.
In
`@ci-operator/step-registry/hypershift/review-agent/trigger/hypershift-review-agent-trigger-commands.sh`:
- Around line 57-75: The trigger loop in
hypershift-review-agent-trigger-commands.sh is turning tracing back on too
early, so parsing RESPONSE with echo/sed/jq can leak the API response body into
logs. Update the retry block around the curl call in the trigger logic to keep
set -x disabled while capturing and parsing RESPONSE, matching the safer pattern
already used in the later polling loop, and only re-enable tracing after the
sensitive response handling is complete.
- Around line 41-45: All curl invocations in
hypershift-review-agent-trigger-commands.sh need explicit timeout handling so
the step fails fast instead of hanging indefinitely. Update the token exchange,
Gangway trigger/poll, and PR comment request paths to pass both connect and
overall timeouts, and keep the existing retry logic in the surrounding shell
flow. Use the existing curl call sites in the script as the targets, especially
the token exchange block and the Gangway/PR comment helper invocations, and
apply the same timeout pattern consistently everywhere curl is used.
---
Nitpick comments:
In
`@ci-operator/step-registry/hypershift/review-agent/trigger/hypershift-review-agent-trigger-commands.sh`:
- Around line 96-97: The script computes JOB_STATUS in the trigger flow but
never uses it, which triggers the unused-variable warning; update the
review-agent trigger logic so JOB_STATUS is included in the final summary/output
instead of being discarded. Make the change in the STATUS_BODY parsing and the
related summary emission paths in hypershift-review-agent-trigger-commands.sh,
including the duplicate status handling later in the script, so the collected
job status is surfaced consistently.
🪄 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: 6993c18c-c735-4f44-963a-003ef8a842a2
📒 Files selected for processing (16)
ci-operator/config/openshift-priv/hypershift/openshift-priv-hypershift-main.yamlci-operator/config/openshift-priv/hypershift/openshift-priv-hypershift-release-4.22.yamlci-operator/config/openshift-priv/hypershift/openshift-priv-hypershift-release-4.23.yamlci-operator/config/openshift-priv/hypershift/openshift-priv-hypershift-release-5.0.yamlci-operator/config/openshift-priv/hypershift/openshift-priv-hypershift-release-5.1.yamlci-operator/config/openshift/hypershift/openshift-hypershift-main.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-4.22.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-4.23.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-5.0.yamlci-operator/config/openshift/hypershift/openshift-hypershift-release-5.1.yamlci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.shci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-ref.yamlci-operator/step-registry/hypershift/review-agent/trigger/OWNERSci-operator/step-registry/hypershift/review-agent/trigger/hypershift-review-agent-trigger-commands.shci-operator/step-registry/hypershift/review-agent/trigger/hypershift-review-agent-trigger-ref.yamlci-operator/step-registry/hypershift/review-agent/trigger/hypershift-review-agent-trigger-workflow.yaml
| curl -s -X POST \ | ||
| -H "Authorization: Bearer ${JWT}" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| "https://api.github.com/app/installations/${INSTALL_ID}/access_tokens" \ | ||
| | jq -r '.token' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add timeouts to all curl calls.
None of the curl invocations (token exchange, Gangway trigger/poll, PR comment) set --max-time/--connect-timeout. If Gangway or GitHub's API hangs, the retry loops (and the whole trigger step) can block indefinitely instead of failing fast and retrying.
🔧 Proposed fix (apply pattern to each curl call)
- RESPONSE=$(curl -s -X POST -d "${POST_DATA}" \
+ RESPONSE=$(curl -s --max-time 30 -X POST -d "${POST_DATA}" \
-H "Authorization: Bearer $(cat "${TOKEN_FILE}")" \
"${GANGWAY_API}/v1/executions/${PERIODIC_JOB_NAME}" \
-w "\n%{http_code}")Also applies to: 59-62, 88-91, 122-126
🤖 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/trigger/hypershift-review-agent-trigger-commands.sh`
around lines 41 - 45, All curl invocations in
hypershift-review-agent-trigger-commands.sh need explicit timeout handling so
the step fails fast instead of hanging indefinitely. Update the token exchange,
Gangway trigger/poll, and PR comment request paths to pass both connect and
overall timeouts, and keep the existing retry logic in the surrounding shell
flow. Use the existing curl call sites in the script as the targets, especially
the token exchange block and the Gangway/PR comment helper invocations, and
apply the same timeout pattern consistently everywhere curl is used.
| for ((i=1; i<=MAX_RETRIES; i++)); do | ||
| set +x | ||
| RESPONSE=$(curl -s -X POST -d "${POST_DATA}" \ | ||
| -H "Authorization: Bearer $(cat "${TOKEN_FILE}")" \ | ||
| "${GANGWAY_API}/v1/executions/${PERIODIC_JOB_NAME}" \ | ||
| -w "\n%{http_code}") | ||
| set -x | ||
| HTTP_STATUS=$(echo "$RESPONSE" | tail -1) | ||
| JSON_BODY=$(echo "$RESPONSE" | sed '$d') | ||
|
|
||
| if [ "$HTTP_STATUS" -eq 200 ]; then | ||
| JOB_ID=$(echo "$JSON_BODY" | jq -r '.id') | ||
| echo "Job triggered successfully. Job ID: ${JOB_ID}" | ||
| break | ||
| else | ||
| echo "[$i/$MAX_RETRIES] Gangway API returned HTTP $HTTP_STATUS. Retrying in ${RETRY_INTERVAL}s..." | ||
| sleep "$RETRY_INTERVAL" | ||
| fi | ||
| done |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Re-enabling set -x turns tracing on that was never active, tracing response bodies.
The script starts with set -euo pipefail (no -x), so tracing is off by default. Inside the trigger loop, set -x at line 63 re-enables tracing immediately, before the response is parsed (lines 64-65), causing the traced echo "$RESPONSE" | ... commands to print the full API response body to logs. The second polling loop (lines 85-105) correctly keeps tracing off for the entire block — apply the same pattern here instead of toggling -x on mid-loop.
As per coding guidelines, "Be cautious with set -x (debug tracing) - it logs all executed commands with their arguments. Variable expansions in traced commands will expose their values in logs" and "Keep the tracing-disabled scope minimal (only around sensitive operations)".
🔒 Proposed fix
for ((i=1; i<=MAX_RETRIES; i++)); do
set +x
RESPONSE=$(curl -s -X POST -d "${POST_DATA}" \
-H "Authorization: Bearer $(cat "${TOKEN_FILE}")" \
"${GANGWAY_API}/v1/executions/${PERIODIC_JOB_NAME}" \
-w "\n%{http_code}")
- set -x
HTTP_STATUS=$(echo "$RESPONSE" | tail -1)
JSON_BODY=$(echo "$RESPONSE" | sed '$d')
+ set -xAlso applies to: 105-105, 127-127
🤖 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/trigger/hypershift-review-agent-trigger-commands.sh`
around lines 57 - 75, The trigger loop in
hypershift-review-agent-trigger-commands.sh is turning tracing back on too
early, so parsing RESPONSE with echo/sed/jq can leak the API response body into
logs. Update the retry block around the curl call in the trigger logic to keep
set -x disabled while capturing and parsing RESPONSE, matching the safer pattern
already used in the later polling loop, and only re-enable tracing after the
sensitive response handling is complete.
Source: Coding guidelines
- Suppress SC2034 for JOB_STATUS (used for debugging) - Add generated metadata files for trigger ref and workflow Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
76728a0 to
6ce8e55
Compare
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@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. |
|
@enxebre: This pull request references CNTRLPLANE-3757 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/pj-rehearse ack |
|
@enxebre: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre, jparrill, sdminonne 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 |
…f running inline (openshift#81366) * review-agent: trigger periodic via gangway instead of running inline The review-agent presubmit has a design flaw: when the agent pushes commits to the PR branch, the presubmit job aborts itself. This change splits the architecture: - Add a periodic-review-agent periodic job (cron: @Yearly, triggered only via gangway) that runs the full hypershift-review-agent workflow - Add a lightweight hypershift-review-agent-trigger step that: 1. Triggers the periodic job via gangway API with the PR number 2. Posts a comment on the PR with a link to the triggered job - Update all presubmit configs (openshift + openshift-priv) to use the trigger workflow instead of running the agent inline The gangway token is read from the hypershift-team-claude-prow secret. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review-agent: remove hardcoded PR default from process step The REVIEW_AGENT_TARGET_PR env var should not have a hardcoded default. The periodic job receives the PR number from gangway via MULTISTAGE_PARAM_OVERRIDE_REVIEW_AGENT_TARGET_PR, and the process script already fails fast if no PR number is provided. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review-agent: follow jira-agent pattern for gangway param override Declare MULTISTAGE_PARAM_OVERRIDE_REVIEW_AGENT_TARGET_PR in the ref YAML and apply it in the script, matching the jira-agent pattern for gangway API overrides. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review-agent: fix shellcheck warning and add registry metadata - Suppress SC2034 for JOB_STATUS (used for debugging) - Add generated metadata files for trigger ref and workflow Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…f running inline (openshift#81366) * review-agent: trigger periodic via gangway instead of running inline The review-agent presubmit has a design flaw: when the agent pushes commits to the PR branch, the presubmit job aborts itself. This change splits the architecture: - Add a periodic-review-agent periodic job (cron: @Yearly, triggered only via gangway) that runs the full hypershift-review-agent workflow - Add a lightweight hypershift-review-agent-trigger step that: 1. Triggers the periodic job via gangway API with the PR number 2. Posts a comment on the PR with a link to the triggered job - Update all presubmit configs (openshift + openshift-priv) to use the trigger workflow instead of running the agent inline The gangway token is read from the hypershift-team-claude-prow secret. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review-agent: remove hardcoded PR default from process step The REVIEW_AGENT_TARGET_PR env var should not have a hardcoded default. The periodic job receives the PR number from gangway via MULTISTAGE_PARAM_OVERRIDE_REVIEW_AGENT_TARGET_PR, and the process script already fails fast if no PR number is provided. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review-agent: follow jira-agent pattern for gangway param override Declare MULTISTAGE_PARAM_OVERRIDE_REVIEW_AGENT_TARGET_PR in the ref YAML and apply it in the script, matching the jira-agent pattern for gangway API overrides. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review-agent: fix shellcheck warning and add registry metadata - Suppress SC2034 for JOB_STATUS (used for debugging) - Add generated metadata files for trigger ref and workflow Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…f running inline (openshift#81366) * review-agent: trigger periodic via gangway instead of running inline The review-agent presubmit has a design flaw: when the agent pushes commits to the PR branch, the presubmit job aborts itself. This change splits the architecture: - Add a periodic-review-agent periodic job (cron: @Yearly, triggered only via gangway) that runs the full hypershift-review-agent workflow - Add a lightweight hypershift-review-agent-trigger step that: 1. Triggers the periodic job via gangway API with the PR number 2. Posts a comment on the PR with a link to the triggered job - Update all presubmit configs (openshift + openshift-priv) to use the trigger workflow instead of running the agent inline The gangway token is read from the hypershift-team-claude-prow secret. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review-agent: remove hardcoded PR default from process step The REVIEW_AGENT_TARGET_PR env var should not have a hardcoded default. The periodic job receives the PR number from gangway via MULTISTAGE_PARAM_OVERRIDE_REVIEW_AGENT_TARGET_PR, and the process script already fails fast if no PR number is provided. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review-agent: follow jira-agent pattern for gangway param override Declare MULTISTAGE_PARAM_OVERRIDE_REVIEW_AGENT_TARGET_PR in the ref YAML and apply it in the script, matching the jira-agent pattern for gangway API overrides. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review-agent: fix shellcheck warning and add registry metadata - Suppress SC2034 for JOB_STATUS (used for debugging) - Add generated metadata files for trigger ref and workflow Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
periodic-review-agentperiodic job (cron:@yearly, triggered only via gangway) that runs the fullhypershift-review-agentworkflow.hypershift-review-agent-triggerstep/workflow that triggers the periodic via gangway API and comments on the PR.MULTISTAGE_PARAM_OVERRIDE_REVIEW_AGENT_TARGET_PRgangway parameter passing.Test plan
make updategenerates correct periodic and presubmit job definitions/test address-review-commentson a test PR and confirm it triggers the periodic via gangway and posts a comment🤖 Generated with Claude Code
Summary by CodeRabbit
This PR updates the OpenShift CI infrastructure for HyperShift review-agents so presubmits no longer execute the full
hypershift-review-agentinline. Instead, presubmit jobs now invoke a new lightweighthypershift-review-agent-triggerworkflow that uses Gangway to start a separate scheduled job and then posts a PR comment linking to the resulting job run.Practically:
openshiftandopenshift-privpresubmit configs are updated so theaddress-review-commentsstep/workflow switches fromhypershift-review-agenttohypershift-review-agent-triggeracross main and multiple release branches, while preserving the existing “skip if only changed” behavior.ci-operator/config/openshift/hypershift/openshift-hypershift-main.yaml, a new scheduledperiodic-review-agentjob is added with an@yearlycron; the trigger workflow is intended to run the fullhypershift-review-agentworkflow via Gangway when kicked off.To support Gangway-driven targeting, the review-agent process logic was updated to follow the
jira-agentmultistage override pattern:MULTISTAGE_PARAM_OVERRIDE_REVIEW_AGENT_TARGET_PRand changed PR selection to derive the target PR number from that value (or fall back toPULL_NUMBER), removing the previous directREVIEW_AGENT_TARGET_PRoverride.The PR also introduces the trigger step/workflow plumbing:
hypershift-review-agent-triggerstep + workflow that calls the Gangway periodic execution endpoint (with retries/polling) and extractsJOB_ID/job_url.OWNERS/ metadata JSON) for the new trigger step and workflow.