fix: refresh GitHub App tokens before push/reply in review-agent - #82494
fix: refresh GitHub App tokens before push/reply in review-agent#82494bryan-cox wants to merge 1 commit into
Conversation
|
@bryan-cox: GitHub didn't allow me to request PR reviews from the following users: openshift/hypershift-team-psa. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
openshift-ci
Bot
added
the
needs-rebase
WalkthroughThe review agent now retrieves paginated issue comments, stores and refreshes GitHub App tokens through temporary files and a generated script, and adds prompt instructions for tracking posted replies within a session. ChangesReview agent updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewAgent
participant GitHubRESTAPI
participant Claude
ReviewAgent->>GitHubRESTAPI: Fetch paginated issue comments
GitHubRESTAPI-->>ReviewAgent: Return all comment pages
ReviewAgent->>Claude: Provide comments and duplicate-prevention instructions
Claude-->>ReviewAgent: Produce review replies
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
@bryan-cox, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh (2)
623-627: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueToken files are world-readable for a brief window before
chmod 600.
echo ... > /tmp/.github-token-fork/upstreamwrites with default umask permissions, andchmod 600only runs afterward, leaving a short window where the token content is more widely readable. Given the retrieved learning that these step-registry scripts run in ephemeral, single-tenant containers with no other local users, the symlink/TOCTOU angle flagged by static analysis doesn't really apply here (andmktempisn't viable since the credential helper and refresh script need this exact predictable path). Still, tightening the write itself (e.g.(umask 077; echo "$TOKEN" > file)orinstall -m 600 /dev/null file && echo ... > file) is a trivial defense-in-depth improvement for token material.🔒 Optional tightening
-echo "$GITHUB_TOKEN_FORK" > /tmp/.github-token-fork -echo "$GITHUB_TOKEN_UPSTREAM" > /tmp/.github-token-upstream -chmod 600 /tmp/.github-token-fork /tmp/.github-token-upstream +(umask 077; echo "$GITHUB_TOKEN_FORK" > /tmp/.github-token-fork) +(umask 077; echo "$GITHUB_TOKEN_UPSTREAM" > /tmp/.github-token-upstream)Based on learnings, "assume they run in ephemeral containers created at execution time with no pre-existing filesystem/symlinks from the image... the 'pre-existing-path symlink' concern should not be raised for predictable /tmp paths in these scripts."
🤖 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 623 - 627, Update the token writes near the refresh-file setup to create both /tmp token files with restrictive permissions from the outset, using a scoped umask or equivalent approach before writing their contents. Preserve the existing predictable paths required by the credential helper and refresh script; the subsequent chmod may remain as defense in depth.Source: Learnings
636-686: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGenerated refresh script isn't permission-restricted, and duplicates JWT-signing logic.
Two independent points:
/tmp/refresh-github-tokens.shonly getschmod +x(line 685); unlike the token files it doesn't get a600/700restriction, so its content — including$PRIVATE_KEY_FILE's path and both installation IDs — is left at default umask readability.- The JWT-minting logic inside this heredoc (lines 646-661) is a near-verbatim duplicate of
generate_github_token()(lines 583-603). Any future change to the signing algorithm or claims would need to be kept in sync in two places.🔒 Restrict permissions on the generated script
chmod +x /tmp/refresh-github-tokens.sh +chmod 700 /tmp/refresh-github-tokens.sh🤖 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 636 - 686, Restrict the generated /tmp/refresh-github-tokens.sh permissions to owner-only access by applying 700 (or stricter) instead of only making it executable. Remove the duplicated generate_token JWT-signing implementation from the heredoc and have the refresh script reuse the existing generate_github_token() logic, preserving the current fork and upstream token refresh behavior.
🤖 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.
Nitpick comments:
In
`@ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh`:
- Around line 623-627: Update the token writes near the refresh-file setup to
create both /tmp token files with restrictive permissions from the outset, using
a scoped umask or equivalent approach before writing their contents. Preserve
the existing predictable paths required by the credential helper and refresh
script; the subsequent chmod may remain as defense in depth.
- Around line 636-686: Restrict the generated /tmp/refresh-github-tokens.sh
permissions to owner-only access by applying 700 (or stricter) instead of only
making it executable. Remove the duplicated generate_token JWT-signing
implementation from the heredoc and have the refresh script reuse the existing
generate_github_token() logic, preserving the current fork and upstream token
refresh behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4c1abc2e-774d-466c-9a04-834a97232037
📒 Files selected for processing (1)
ci-operator/step-registry/hypershift/review-agent/process/hypershift-review-agent-process-commands.sh
Add a pre-step to address-review-pr that calls /tmp/refresh-github-tokens.sh (written by the CI process step) before posting replies or pushing. This ensures tokens are valid after long sessions that outlast the 1-hour GitHub App token lifetime. Companion to openshift/release#82494. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
GitHub App installation tokens expire after 1 hour. When Claude sessions run longer than that (the PR openshift#9102 session ran 1h17m), push and reply operations fail with 401 Bad credentials. Store tokens in files instead of baking them into env vars and git config. Write a refresh script to /tmp that Claude can call before push/reply to regenerate both fork and upstream tokens (~2-3s). Switch from `export GITHUB_TOKEN` to `gh auth login --with-token` so the stored token can be updated mid-session. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
37f8fd9 to
6c0d2b0
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox 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 |
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@bryan-cox: 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. |
| $_was_tracing && set -x || true | ||
|
|
||
| # Write a refresh script that Claude can call before push/reply operations | ||
| cat > /tmp/refresh-github-tokens.sh << REFRESH_EOF |
There was a problem hiding this comment.
2 participants
Summary
/tmp/.github-token-{fork,upstream}) instead of baking them into env vars and git configexport GITHUB_TOKENtogh auth login --with-tokenso stored auth can be updated mid-session/tmp/refresh-github-tokens.shthat Claude can call to regenerate both tokens (~2-3s)Problem
GitHub App installation tokens expire after 1 hour. When Claude sessions run longer (e.g. PR #9102 session ran 1h17m), push and reply operations fail with
401 Bad credentials. The agent processes the review comments correctly but can't post replies or push code changes.Test plan
/tmp/refresh-github-tokens.shis created with correct credentialsgh auth statusshows valid auth after initial setup/tmp/refresh-github-tokens.shafter setup/cc @openshift/hypershift-team-psa
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements