diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index b9443d9..46465cd 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -10,16 +10,21 @@ concurrency: jobs: review: - # Skip forks, skip non-claude bots (e.g. dependabot, renovate), - # but allow claude-created PRs through for self-review + # Skip forks and third-party bots. Allow human-authored PRs plus PRs created + # by our triage workflow on same-repository claude/issue-* branches. if: | github.event.pull_request.head.repo.fork == false && ( !endsWith(github.event.pull_request.user.login, '[bot]') || - contains(github.event.pull_request.user.login, 'claude') + contains(github.event.pull_request.user.login, 'claude') || + ( + github.event.pull_request.user.login == 'github-actions[bot]' && + startsWith(github.event.pull_request.head.ref, 'claude/issue-') + ) ) runs-on: ubuntu-latest permissions: + actions: read contents: read pull-requests: write id-token: write @@ -27,12 +32,29 @@ jobs: - name: Checkout repository uses: actions/checkout@v4 with: - fetch-depth: 1 + fetch-depth: 0 + + - name: Detect review workflow changes + id: review-workflow + shell: bash + run: | + if git diff --quiet \ + '${{ github.event.pull_request.base.sha }}' \ + '${{ github.event.pull_request.head.sha }}' \ + -- .github/workflows/claude-review.yml; then + echo "changed=false" >> "$GITHUB_OUTPUT" + else + echo "changed=true" >> "$GITHUB_OUTPUT" + fi - name: Claude Code Review uses: anthropics/claude-code-action@v1.0.93 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + # Claude's app token gives ordinary PRs a distinct reviewer identity. + # A workflow-token fallback is required when this workflow itself changes, + # because the Claude app validates it against the default-branch version. + github_token: ${{ steps.review-workflow.outputs.changed == 'true' && github.token || '' }} allowed_bots: '*' track_progress: true classify_inline_comments: 'true' @@ -44,6 +66,8 @@ jobs: prompt: | REPO: ${{ github.repository }} PR NUMBER: ${{ github.event.pull_request.number }} + PR AUTHOR: ${{ github.event.pull_request.user.login }} + REVIEW ACTOR: ${{ steps.review-workflow.outputs.changed == 'true' && 'github-actions[bot]' || 'claude[bot]' }} **Before doing anything else, read and clean up stale Claude feedback:** 1. Fetch all previous Claude comments on this PR and read them to retain context: @@ -68,10 +92,17 @@ jobs: Leave inline comments on specific lines and a top-level summary. Skip nitpicks; prioritize substantive feedback. - **When you are done, submit a formal GitHub review using one of:** + **When you are done, submit a GitHub review using one of:** - `gh pr review ${{ github.event.pull_request.number }} --approve --body "..."` — if no substantive issues found - `gh pr review ${{ github.event.pull_request.number }} --request-changes --body "..."` — if there are blocking problems - Do not leave the PR without a formal APPROVE or REQUEST_CHANGES review state. + Exception when PR AUTHOR and REVIEW ACTOR are both `github-actions[bot]`: GitHub + prevents that workflow identity from approving or requesting changes on its own PR. + In that case, use + `gh pr review ${{ github.event.pull_request.number }} --comment --body "..."` + and begin the body with either `PASS —` or `BLOCKING —`. The successful/failed + review workflow check is the merge gate for that self-review case. + + Do not leave a human-authored PR without a formal APPROVE or REQUEST_CHANGES state. claude_args: | --model sonnet --allowedTools "mcp__github__pull_request_review_write,mcp__github__add_comment_to_pending_review,mcp__github__add_issue_comment,mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr review:*),Bash(gh api repos/*/issues/*/comments:*),Bash(gh api repos/*/issues/comments/*:*),Bash(gh api repos/*/pulls/*/reviews:*),Bash(gh api repos/*/pulls/*/comments:*)" diff --git a/src/services/checkmarx/client.test.ts b/src/services/checkmarx/client.test.ts new file mode 100644 index 0000000..26aa50b --- /dev/null +++ b/src/services/checkmarx/client.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it, vi } from 'vitest'; +import type { HttpClient } from '../../lib/http.js'; +import { CheckmarxClient } from './client.js'; + +function makeClient(response: unknown): { + client: CheckmarxClient; + checkmarx: ReturnType; +} { + const checkmarx = vi.fn().mockResolvedValue(response); + const http = { checkmarx } as unknown as HttpClient; + return { client: new CheckmarxClient(http), checkmarx }; +} + +describe('CheckmarxClient', () => { + it('returns an empty project list when Checkmarx responds with null', async () => { + const { client, checkmarx } = makeClient({ + projects: null, + totalCount: 0, + filteredTotalCount: 0 + }); + + await expect(client.listProjects()).resolves.toEqual([]); + expect(checkmarx).toHaveBeenCalledWith('/api/projects', { params: { limit: 100 } }); + }); + + it('returns an empty scan list when Checkmarx responds with null', async () => { + const { client, checkmarx } = makeClient({ + scans: null, + totalCount: 0, + filteredTotalCount: 0 + }); + + await expect(client.listScans({ projectId: 'project-id', last: 25 })).resolves.toEqual([]); + expect(checkmarx).toHaveBeenCalledWith('/api/scans', { + params: { limit: 25, 'project-id': 'project-id' } + }); + }); +}); diff --git a/src/services/checkmarx/client.ts b/src/services/checkmarx/client.ts index d77aa90..651139f 100644 --- a/src/services/checkmarx/client.ts +++ b/src/services/checkmarx/client.ts @@ -12,10 +12,11 @@ export class CheckmarxClient { async listProjects(): Promise { const res = await this.http.checkmarx('/api/projects', { params: { limit: 100 } }); - if (res.filteredTotalCount > res.projects.length) { - process.stderr.write(`warning: ${res.filteredTotalCount} projects found; only showing first ${res.projects.length}\n`); + const projects = res.projects ?? []; + if (res.filteredTotalCount > projects.length) { + process.stderr.write(`warning: ${res.filteredTotalCount} projects found; only showing first ${projects.length}\n`); } - return res.projects; + return projects; } async getProject(id: string): Promise { @@ -26,10 +27,11 @@ export class CheckmarxClient { const params: Record = { limit: opts.last ?? 100 }; if (opts.projectId) params['project-id'] = opts.projectId; const res = await this.http.checkmarx('/api/scans', { params }); - if (res.filteredTotalCount > res.scans.length) { - process.stderr.write(`warning: ${res.filteredTotalCount} scans found; only showing first ${res.scans.length}\n`); + const scans = res.scans ?? []; + if (res.filteredTotalCount > scans.length) { + process.stderr.write(`warning: ${res.filteredTotalCount} scans found; only showing first ${scans.length}\n`); } - return res.scans; + return scans; } async getScan(id: string): Promise { diff --git a/src/types/checkmarx.ts b/src/types/checkmarx.ts index 2cd1d2e..6896d83 100644 --- a/src/types/checkmarx.ts +++ b/src/types/checkmarx.ts @@ -37,13 +37,13 @@ export interface CxOneResultsSummary { } export interface CxOneProjectsResponse { - projects: CxOneProject[]; + projects: CxOneProject[] | null; totalCount: number; filteredTotalCount: number; } export interface CxOneScansResponse { - scans: CxOneScan[]; + scans: CxOneScan[] | null; totalCount: number; filteredTotalCount: number; }