Skip to content

ci(security-review): fix origin/HEAD missing so /security-review's git diff works#1327

Open
tejaskash wants to merge 1 commit into
mainfrom
fix-origin-head
Open

ci(security-review): fix origin/HEAD missing so /security-review's git diff works#1327
tejaskash wants to merge 1 commit into
mainfrom
fix-origin-head

Conversation

@tejaskash
Copy link
Copy Markdown
Contributor

Why

Run #26186045056 on PR #1321 failed with Reached maximum number of turns (30) because the very first thing the bundled /security-review skill does is git diff origin/HEAD..., and that resolves to:

fatal: ambiguous argument 'origin/HEAD...': unknown revision or path not in the working tree

actions/checkout clones the remote and sets origin/<head-ref> but does not set the remote's symbolic HEAD ref. So origin/HEAD is undefined, the skill's git command crashes, Claude tries 30 turns of recovery variants, then the action exits 1.

This affects every PR run after #1310 merged (we switched from inlining the prompt + custom git context to letting the bundled skill drive its own git commands).

What changes

One step inserted right after Checkout PR head:

- name: Set origin/HEAD for /security-review skill
  env:
    BASE_REF: ${{ steps.pr.outputs.base_ref }}
  run: |
    set -euo pipefail
    git remote set-head origin "$BASE_REF"
    git symbolic-ref refs/remotes/origin/HEAD

git remote set-head origin <base-ref> makes origin/HEAD resolve to the PR's base branch. The follow-up git symbolic-ref line is a sanity print that fails the step early if the head still isn't set.

Test plan

  • Merge.
  • Open a small PR (or wait for the next community PR + safe-to-review); confirm the security review run no longer hits error_max_turns, that the skill's git diff origin/HEAD... resolves correctly, and that inline findings (or a "no findings" summary) post as expected.

…works

The /security-review slash command runs `git diff origin/HEAD...` as its
first action to enumerate the PR's changes. actions/checkout doesn't set
up the remote's symbolic HEAD ref, so that command fails with
"ambiguous argument 'origin/HEAD...': unknown revision". Claude then
loops trying variants until --max-turns 30 trips and the action exits 1.

Set origin/HEAD to the PR's base ref right after checkout so the skill's
git invocations resolve correctly. Run #26186045056 on PR #1321 was the
trigger - 30 turns spent on shell-error recovery, no findings posted.
@tejaskash tejaskash requested a review from a team May 20, 2026 20:00
@github-actions github-actions Bot added the size/xs PR size: XS label May 20, 2026
@github-actions github-actions Bot added the agentcore-harness-reviewing AgentCore Harness review in progress label May 20, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label May 20, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Package Tarball

aws-agentcore-0.14.1.tgz

How to install

gh release download pr-1327-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.14.1.tgz

Copy link
Copy Markdown

@agentcore-cli-automation agentcore-cli-automation left a comment

Choose a reason for hiding this comment

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

LGTM. The diagnosis matches the failure (actions/checkout doesn't materialize refs/remotes/origin/HEAD, and git diff origin/HEAD... is the first thing the bundled /security-review skill runs), and the fix is the right shape:

  • fetch-depth: 0 already pulls every refs/heads/* into refs/remotes/origin/* (see getRefSpecForAllHistory in actions/checkout v6), so origin/$BASE_REF is guaranteed to exist locally before git remote set-head runs.
  • Using the PR's actual base.ref (rather than --auto, which would query the remote default branch) is correctly more conservative — a PR targeting a release branch will diff against that release branch, which is what a security review actually wants.
  • set -euo pipefail plus the trailing git symbolic-ref refs/remotes/origin/HEAD gives a clean fail-fast if the symbolic ref somehow didn't get set, instead of letting the failure surface 30 turns deep inside Claude.

No product code touched, so telemetry/mocking guidance doesn't apply. Ship it.

@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label May 20, 2026
@agentcore-devx-automation
Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label May 20, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 43.73% 9316 / 21299
🔵 Statements 42.98% 9884 / 22996
🔵 Functions 40.33% 1608 / 3987
🔵 Branches 40.45% 6058 / 14976
Generated in workflow #3164 for commit e480956 by the Vitest Coverage Report Action

Copy link
Copy Markdown
Contributor

@Hweinstock Hweinstock left a comment

Choose a reason for hiding this comment

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

nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/xs PR size: XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants