Skip to content
This repository was archived by the owner on Jun 8, 2026. It is now read-only.

Add code review to closedloop-electron - #158

Closed
shafty023 wants to merge 1 commit into
mainfrom
add-claude-github-actions-1778080131164
Closed

Add code review to closedloop-electron#158
shafty023 wants to merge 1 commit into
mainfrom
add-claude-github-actions-1778080131164

Conversation

@shafty023

Copy link
Copy Markdown
Contributor

No description provided.

@shafty023
shafty023 requested a review from a team May 6, 2026 15:14
@chatgpt-codex-connector

Copy link
Copy Markdown

💡 Codex Review

pull_request_target:
types: [opened]

P1 Badge Avoid checking out PR code under pull_request_target

When a Dependabot PR is opened, this new pull_request_target trigger runs with repository secrets and the write-capable GitHub App token, but the job later checks out github.event.pull_request.head.sha and runs Claude with broad file/Bash/GitHub permissions over that PR content. In the Dependabot path, a malicious dependency update or repository instruction change can therefore influence a privileged workflow that has access to secrets and PR/issue write permissions; use pull_request without secrets, or keep pull_request_target on trusted base code and fetch only the diff metadata needed for review.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

on:
pull_request:
types: [opened]
pull_request_target:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensure this does not cause a double review

@mikeangstadt mikeangstadt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — Fork Security & Manual Triggers

2 Blocking, 1 High — see inline comments.

Key Issues

  1. BLOCKING: Fork PRs fail hard at token generation (secrets are empty) — need graceful skip
  2. BLOCKING: workflow_dispatch has no PR number input — manual reviews can't target a PR
  3. HIGH: claude.yml has no actor protection — any GitHub user can @claude to burn API credits

Note: pull_request event runs the fork's copy of the workflow, so if conditions there aren't a security boundary. The real protection is GitHub's secrets withholding for fork PRs. Findings focus on graceful failure and base-branch-enforced controls only.

name: Claude Code Review

on:
pull_request:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING — Fork PRs fail hard instead of skipping gracefully.

pull_request fires for fork PRs. Secrets are (correctly) withheld by GitHub, so there's no security leak. But the job still runs and fails at the create-github-app-token step because secrets.CLOSEDLOOP_APP_ID_STAGE is empty. This creates a red X check on every fork PR.

If branch protection requires this check to pass, fork PRs become unmergeable. Even without branch protection, it's confusing UX.

Fix — graceful skip for forks:

# In the job if condition, add a fork guard to the pull_request branch:
(github.event_name == 'pull_request' && github.actor != 'dependabot[bot]' && github.event.pull_request.head.repo.full_name == github.repository)

Fork PRs would then skip this job entirely (green skip, not red failure). Fork reviews would only happen via workflow_dispatch by a collaborator.

types: [opened]
pull_request_target:
types: [opened]
workflow_dispatch:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING — Manual triggers can't target a specific PR.

workflow_dispatch passes the if condition, but there are no inputs:. When manually triggered:

  • github.event.pull_request is empty → checkout falls back to github.ref (default branch)
  • The Claude prompt becomes /code-review:start --github with no PR number
  • Summary posting skips (PR_NUMBER is empty)

Fix:

workflow_dispatch:
  inputs:
    pr_number:
      description: 'PR number to review'
      required: true
      type: number

Then update the checkout ref and prompt to use github.event.inputs.pr_number when event_name == 'workflow_dispatch'. Resolve the PR's head SHA via gh pr view in a setup step.

types: [submitted]

jobs:
claude:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HIGH — No actor protection. Any GitHub user can burn API credits via @claude.

issue_comment runs from the base branch with secrets — fork authors can't modify this workflow. But any GitHub user can comment @claude on any issue/PR and trigger Claude Code using the org's CLAUDE_CODE_OAUTH_TOKEN.

Fix — restrict to collaborators:

if: |
  (
    github.event_name == 'issue_comment' &&
    contains(github.event.comment.body, '@claude') &&
    contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)
  ) || ...

author_association is on the event payload — no API call needed. This ensures only repo collaborators can invoke @claude.

@mikeangstadt

Copy link
Copy Markdown
Contributor

@peterulsteen let's copy this pattern once we get this landed to claude-plugins and the new repo.

@shafty023 shafty023 closed this May 18, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants