diff --git a/.changes/pr-pipeline/feat-code-review-on-pr-review-cli-8ce885be.md b/.changes/pr-pipeline/feat-code-review-on-pr-review-cli-8ce885be.md new file mode 100644 index 0000000000..4cbf00bc3f --- /dev/null +++ b/.changes/pr-pipeline/feat-code-review-on-pr-review-cli-8ce885be.md @@ -0,0 +1,8 @@ +--- +bump: minor +--- + +### Changed + +- The code review lane (`pr-review-hosted.yml`) runs on ci-workflows' [`pr-review-cli.yml`](https://github.com/melodic-software/ci-workflows/blob/83b739f625b501ad40c65dee7168d836883195e1/.github/workflows/pr-review-cli.yml) (v0.63.1), lane `code`: the reviews role's API key first, and its subscription token only after the key hands off or when the repository variable `REVIEWS_KEY_EXHAUSTED` is `true`. The check stays `pr-review / claude-review-status`; the review now posts as `github-actions[bot]`, each comment ending in its `claude-lane: claude-review` footer. A newer push no longer cancels a running review: it waits for it. +- Both review lanes now run on `pr-review-cli.yml`, funded by the `reviews` role's `REVIEWS_CLAUDE_API_KEY` and `REVIEWS_CLAUDE_CODE_OAUTH_TOKEN`; the code lane no longer reads the bare `CLAUDE_CODE_OAUTH_TOKEN`. diff --git a/.changes/source-control/feat-code-review-on-pr-review-cli-61c0e47d.md b/.changes/source-control/feat-code-review-on-pr-review-cli-61c0e47d.md new file mode 100644 index 0000000000..5b686f359f --- /dev/null +++ b/.changes/source-control/feat-code-review-on-pr-review-cli-61c0e47d.md @@ -0,0 +1,7 @@ +--- +bump: minor +--- + +### Changed + +- `review-venue.sh` finds the code-review lane on ci-workflows' [`pr-review-cli.yml`](https://github.com/melodic-software/ci-workflows/blob/83b739f625b501ad40c65dee7168d836883195e1/.github/workflows/pr-review-cli.yml) from a caller whose path does not contain `security`; a caller whose path does still counts only for security-review. diff --git a/.github/lane-funding.json b/.github/lane-funding.json index eb1859320d..8771060395 100644 --- a/.github/lane-funding.json +++ b/.github/lane-funding.json @@ -10,7 +10,7 @@ "keyed": true, "category": "review", "lanes": [ - { "id": "pr-review", "workflow": "pr-review-hosted.yml", "launcher": "claude-code-action", "currentSecrets": ["CLAUDE_CODE_OAUTH_TOKEN"] }, + { "id": "pr-review", "workflow": "pr-review-hosted.yml", "launcher": "run-claude-cli", "currentSecrets": ["REVIEWS_CLAUDE_API_KEY", "REVIEWS_CLAUDE_CODE_OAUTH_TOKEN"] }, { "id": "pr-review-security", "workflow": "pr-review-security-hosted.yml", "launcher": "run-claude-cli", "currentSecrets": ["REVIEWS_CLAUDE_API_KEY", "REVIEWS_CLAUDE_CODE_OAUTH_TOKEN"] } ] }, diff --git a/.github/scripts/pr-unblock/address-feedback.test.mjs b/.github/scripts/pr-unblock/address-feedback.test.mjs index fe76cddadb..acac42d8b6 100644 --- a/.github/scripts/pr-unblock/address-feedback.test.mjs +++ b/.github/scripts/pr-unblock/address-feedback.test.mjs @@ -424,9 +424,9 @@ test("a github-actions[bot] thread without the note marker, or in another accoun // body ending `\n\nclaude-lane: ` (ci-workflows // .github/actions/publish-review/publish.cjs commentBody at v0.63.0). const LANE_FINDING = "severity: IMPORTANT\nThe input reaches the shell unquoted.\n\nclaude-lane: claude-security-review"; -const laneFinding = (body, reviewer = ACTIONS_BOT) => +const laneFinding = (body, reviewer = ACTIONS_BOT, lane = "claude-security-review") => fakeApi({ - issueComments: [{ id: 1, user: { id: ACTIONS_BOT }, body: marker("claude-security-review", HEAD) }], + issueComments: [{ id: 1, user: { id: ACTIONS_BOT }, body: marker(lane, HEAD) }], reviewComments: [comment(100, ACTIONS_BOT, { body })], reviews: [{ id: 10, user: { id: reviewer }, commit_id: HEAD }], }); @@ -439,6 +439,13 @@ test("a github-actions[bot] finding ending in the lane footer is kept as the rev assert.equal((await run(laneFinding(`${LANE_FINDING}\n\n`))).decision, "proceed"); }); +test("the code lane's github-actions[bot] finding is kept as the reviewer's", async () => { + const body = "The loop skips the last item.\n\nclaude-lane: claude-review"; + const result = await run(laneFinding(body, ACTIONS_BOT, "claude-review")); + assert.equal(result.decision, "proceed"); + assert.deepEqual(result.threads[0].comments, [{ id: 100, author: "reviewer", body }]); +}); + test("a github-actions[bot] comment whose last line is not a review lane's footer is no finding", async () => { for (const body of [ "The input reaches the shell unquoted.", diff --git a/.github/scripts/pr-unblock/feedback-dispatch.mjs b/.github/scripts/pr-unblock/feedback-dispatch.mjs index 4709dff18e..c17d07dd7a 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.mjs @@ -73,12 +73,13 @@ export const DEFAULTS = Object.freeze({ ".github/workflows/pr-review-security-hosted.yml", ]), // The lanes each review workflow names in its reviewed-head marker - // (melodic-software/ci-workflows pr-review.yml and pr-review-cli.yml). + // (melodic-software/ci-workflows pr-review-cli.yml). markerLanes: Object.freeze(["claude-review", "claude-security-review"]), holdLabels: Object.freeze(["do-not-merge"]), - // claude[bot], the code lane's reviewer (.github/standards/trusted-actors/trusted-actors.json). - // pr-review-cli posts as github-actions[bot]; threadAuthors accepts those - // comments only when the lane footer is their last line. + // claude[bot], which posted reviews before both lanes moved to pr-review-cli + // (.github/standards/trusted-actors/trusted-actors.json). pr-review-cli posts + // as github-actions[bot]; threadAuthors accepts those comments only when the + // lane footer is their last line. reviewerId: 209825114, // github-actions[bot], which writes the reviewed-head markers. markerAuthorId: 41898282, diff --git a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs index 89b8f90023..5fe91b500b 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs @@ -241,6 +241,11 @@ test("a github-actions[bot] thread ending in the lane footer dispatches like the assert.equal((await decide(api)).result.number, 42); }); +test("the code lane's github-actions[bot] thread dispatches like the reviewer bot's", async () => { + const api = fakeApi({ threads: [laneThread("The loop skips the last item.\n\nclaude-lane: claude-review")] }); + assert.equal((await decide(api)).result.number, 42); +}); + test("a github-actions[bot] thread without a review lane's footer last dispatches nothing", async () => { for (const body of ["Unquoted input.", "claude-lane: claude-security-review\nUnquoted input."]) { const api = fakeApi({ threads: [laneThread(body)] }); diff --git a/.github/workflows/pr-review-hosted.yml b/.github/workflows/pr-review-hosted.yml index 97c7152029..bf0b44ee75 100644 --- a/.github/workflows/pr-review-hosted.yml +++ b/.github/workflows/pr-review-hosted.yml @@ -6,31 +6,22 @@ name: pr-review-hosted # this copy can add the resolve-model and kill-switch jobs below. It differs # from the component in those jobs and these comments. # -# Thin caller for the org's reusable code-review lane. The reusable workflow -# owns the action pin, model, and the secret-handling model; per its header, -# this caller owns the triggers, concurrency, and the GITHUB_TOKEN permission -# grant (a called workflow can only downgrade the caller's grant, never -# elevate it). +# Thin caller for the org's pr-review-cli reusable, lane `code`. The reusable +# owns the launcher, the credential order (the reviews role's API key, then +# its subscription token after a handoff) and the secret-handling model; per +# its header, this caller owns the triggers, concurrency, and the GITHUB_TOKEN +# permission grant (a called workflow can only downgrade the caller's grant, +# never elevate it). # -# The reusable skips fork PRs (they receive no secrets and a read-only token) -# and every `[bot]` actor whose sender id is not in `allowed-bot-sender-ids` -# (below), so neither is reviewed by design. On a public repository that means -# the lane reviews only pull requests from branches of this repository: fork -# and Dependabot pull requests get no secrets. +# The reusable skips fork PRs (they receive no secrets and a read-only token), +# Dependabot, and every `[bot]` actor whose sender id is not in +# `allowed-bot-sender-ids` (below), so none is reviewed by design. On a public +# repository that means the lane reviews only pull requests from branches of +# this repository: fork and Dependabot pull requests get no secrets. # -# RUNNER PLACEMENT: hosted shape. This file runs the lane on the GitHub-hosted -# `ubuntu-24.04` runner, which the standards `runner-policy` component admits -# on a public repository. It is the hosted sibling of -# components/claude-lanes/claude-review.yml, which names the private fleet -# review label, and differs from it only in these comments, `runner`, the -# resolve-model and kill-switch jobs and the workflow `name:`, which equals each file's stem. The job id is the same, so -# the check context is the same on every target. A target manages one of the -# two, never both. -# -# INTERIM: this per-visibility pair is removed when the runner choice moves -# inside the ci-workflows reusable and runner-policy gains a cross-repo -# routing kind, so one caller serves both visibilities -# (distribution/sync-manifest.yml, the claude lane caller note). +# RUNNER PLACEMENT: every job of the reusable runs on ubuntu-24.04; it takes +# no runner input. The job id `pr-review` keeps the check +# `pr-review / claude-review-status`. # # This lane is advisory (github-iac ADR 0011: no organization ruleset may make # an agentic review a merge prerequisite). @@ -52,21 +43,14 @@ on: permissions: contents: read -# Per-PR supersede group: a newer event for the same PR, a push included, -# cancels the older run, so per-push reviews do not stack. A skipped actor's -# same-PR event can cancel an in-flight review through this group; that is -# accepted: with `synchronize`, the cancelled review was of a head the new -# event has already superseded. The security caller cancels superseded runs -# the same way. -# -# The group is the `${{ github.workflow }}`-prefixed canonical form the -# standards `concurrency-policy` component requires, so a target enforcing -# that policy passes without an exemption. A file whose header says -# downstream edits are overwritten cannot ask its consumers to normalize it -# themselves. +# Per-PR group: a newer event for the same PR waits for the running review +# instead of cancelling it, because the guardrails merge guard reads a +# cancelled run as red. The security caller does the same. The group is the +# `${{ github.workflow }}`-prefixed canonical form the standards +# `concurrency-policy` component requires. concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} - cancel-in-progress: true + cancel-in-progress: false jobs: # The review's model, turns and effort: the review-code route of the models @@ -120,7 +104,7 @@ jobs: SETTINGS_JSON: ${{ steps.models.outputs.settings-json }} run: | if [ -n "$SETTINGS_JSON" ]; then - echo "::error::the review-code profile sets subagent-model, which pr-review.yml does not support" + echo "::error::the review-code profile sets subagent-model, which pr-review-cli.yml does not support" exit 1 fi @@ -175,15 +159,16 @@ jobs: # A draft is work in progress and does not spend a review; the # ready_for_review trigger above is what delivers one when it is ready. if: ${{ !cancelled() && github.event.pull_request.draft == false && needs.kill-switch.outputs.proceed == 'true' }} + # The model jobs narrow this to contents: read and pull-requests: read; + # only the reusable's status job (the claude-review-status check) writes. permissions: - contents: read # checkout + read the diff - pull-requests: write # post review, tracking comment and the lane's + contents: read + pull-requests: write # the review, its inline comments and the lane's # reviewed-head marker comment - id-token: write # OIDC, mints the Claude GitHub App token (required - # even with an OAuth/API-key credential) - uses: melodic-software/ci-workflows/.github/workflows/pr-review.yml@2fe38c031867900824c164e33a140a335ed9ecf0 # v0.62.0 + uses: melodic-software/ci-workflows/.github/workflows/pr-review-cli.yml@83b739f625b501ad40c65dee7168d836883195e1 # v0.63.1 with: - runner: ubuntu-24.04 + lane: code + publish: comments # Empty unless resolve-model succeeded, so a failed job's partial output # never reaches the review. model-args: ${{ needs.resolve-model.result == 'success' && needs.resolve-model.outputs.claude-args || '' }} @@ -199,8 +184,11 @@ jobs: # bot ids in allowed-bot-sender-ids above also mark issues those bots # wrote as trusted closing-issue context. allowed-author-ids: '153232337' - # Pass only the one named secret (least privilege) rather than - # `secrets: inherit`, which would forward every parent secret to the - # called workflow. + # Empty, as pr-review.yml's default was: the code lane reviews every + # change, documentation included. + docs-only-paths: '' + # The reviews role's key runs first; its subscription token only after the + # key hands off. Named secrets only, never `secrets: inherit`. secrets: - CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + CLAUDE_API_KEY: ${{ secrets.REVIEWS_CLAUDE_API_KEY }} + CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.REVIEWS_CLAUDE_CODE_OAUTH_TOKEN }} diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 01319c77a4..b7a3d1e880 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -2011,7 +2011,7 @@ the lanes. | `trust-root` ruleset, merge queues, code-owner review settings | github-iac `OrgRulesets.cs`; ADR 0015 Decision 5; [ADR 0007](https://github.com/melodic-software/github-iac/blob/main/docs/adr/0007-no-required-approving-reviews.md). Its rollback is not yet tracked: [github-iac#677](https://github.com/melodic-software/github-iac/issues/677) | | Test repository | [`melodic-software/pr-pipeline-sandbox`](https://github.com/melodic-software/pr-pipeline-sandbox), whose README states its purpose | | Trusted actors, runner policy, Actions naming vocabulary | `melodic-software/standards` components [`trusted-actors`](https://github.com/melodic-software/standards/tree/main/components/trusted-actors), [`runner-policy`](https://github.com/melodic-software/standards/tree/main/components/runner-policy) and [`github-actions-conventions`](https://github.com/melodic-software/standards/tree/main/components/github-actions-conventions), synced into `.github/standards/` | -| Review and intake reusable workflows | [`melodic-software/ci-workflows`](https://github.com/melodic-software/ci-workflows) `.github/workflows/pr-review.yml`, `pr-review-cli.yml` (the security lane) and `intake-triage.yml`, called by this repository's synced `*-hosted.yml` callers and `intake-triage.yml` | +| Review and intake reusable workflows | [`melodic-software/ci-workflows`](https://github.com/melodic-software/ci-workflows) `.github/workflows/pr-review-cli.yml` (both review lanes) and `intake-triage.yml`, called by this repository's `*-hosted.yml` review callers and `intake-triage.yml` | In this repository, the lane runners and their contract are in [Lanes](#lanes), the config is `docs/conventions/pr-pipeline.yaml`, and the decisions are ADRs 0049 to 0051, 0053 to 0055 and 0058 in `docs/adr/`. diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 0edb1144fe..04426980b9 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -798,7 +798,8 @@ the base's `.github/scripts/pr-unblock/address-feedback.mjs`, `apply-patch.mjs` 1. Before any model turn, the select job reads the PR head, resolves the slot as the read file does (a draft or `do-not-merge` skips it), and runs `address-feedback.mjs select` with its `GITHUB_TOKEN`. It keeps each unresolved thread whose first comment's review - the reviewer bot (`claude[bot]` in the base's trusted-actors list, by id) submitted on a head + the reviewer bot (`claude[bot]` in the base's trusted-actors list, by id, or `github-actions[bot]` + for a comment whose last non-blank line is a review lane's `claude-lane:` footer) submitted on a head that a `claude-lane-reviewed-head` marker from `github-actions[bot]` names, or `github-actions[bot]` submitted there for a verifier note (a first comment by it whose first line is ``, author `verifier`), when every comment is the reviewer diff --git a/plugins/source-control/reference/providers/github/pull-request/review-venue.md b/plugins/source-control/reference/providers/github/pull-request/review-venue.md index 79a3bbd9bd..c8ed9de2e6 100644 --- a/plugins/source-control/reference/providers/github/pull-request/review-venue.md +++ b/plugins/source-control/reference/providers/github/pull-request/review-venue.md @@ -11,10 +11,11 @@ workflows, fixed in the script: | Lane | Reusable workflow the caller's runs reference | |---|---| -| `code-review` | `melodic-software/ci-workflows/.github/workflows/pr-review.yml@` | -| `security-review` | `melodic-software/ci-workflows/.github/workflows/pr-review-security.yml@`, or `pr-review-cli.yml@` from a caller whose path contains `security` (the run listing does not show its `lane` input) | +| `code-review` | `melodic-software/ci-workflows/.github/workflows/pr-review.yml@`, or `pr-review-cli.yml@` from a caller whose path does not contain `security` | +| `security-review` | `melodic-software/ci-workflows/.github/workflows/pr-review-security.yml@`, or `pr-review-cli.yml@` from a caller whose path contains `security` | -The caller's own file name does not matter (`pr-review-hosted.yml` on a public repository, +The run listing does not show `pr-review-cli.yml`'s `lane` input, so the caller's path decides its +lane. Otherwise the caller's own file name does not matter (`pr-review-hosted.yml` on a public repository, `pr-review.yml` on a private one). The script reads the `referenced_workflows` field of the repository's newest pull request runs, up to three pages of 100: diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py index 73065e806b..c545689f2d 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py @@ -25,8 +25,7 @@ since, raising when neither is proven. The record format is the lane producer's (ci-workflows -`.github/workflows/pr-review.yml`, "Report the review status" and "Record -the reviewed head"). +`.github/workflows/pr-review-cli.yml`, and `pr-review.yml` for older runs). """ from __future__ import annotations diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_review_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_review_evidence.py index 1698ed293f..b8e264b05d 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_review_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_review_evidence.py @@ -507,17 +507,14 @@ def test_no_completion_record(self) -> None: def test_a_subscription_leg_record_binds(self) -> None: # ci-workflows pr-review-cli.yml ends the record ` account=fallback` # when the subscription leg reviewed (publish-review publish.cjs). - runs = [ - whole("claude-review", S), - run( - "claude-security-review", - S, - "findings=0 inline=0 scope=whole account=fallback", - ), - ] - result = self.bind(runs)["claude-security-review"] - self.assertTrue(result.bound) - self.assertEqual(result.open_findings, 0) + # Both lanes run on it. + for lane, other in (LANES, LANES[::-1]): + with self.subTest(lane=lane): + tail = "findings=0 inline=0 scope=whole account=fallback" + runs = [whole(other, S), run(lane, S, tail)] + result = self.bind(runs)[lane] + self.assertTrue(result.bound) + self.assertEqual(result.open_findings, 0) def test_another_account_suffix_is_malformed(self) -> None: for tail in ( @@ -1833,5 +1830,42 @@ def test_claude_review_findings_all_block(self) -> None: self.assertIsNone(result.blocking_findings) +class CodeLaneOnCliTests(Base): + """The code lane on ci-workflows pr-review-cli.yml posts as + github-actions[bot], each comment footed `claude-lane: claude-review`.""" + + ACTIONS = {"__typename": "Bot", "login": "github-actions"} + FOOTED = "finding\n\nclaude-lane: claude-review" + + def test_its_thread_stands_for_the_recorded_finding(self) -> None: + runs = [whole("claude-review", S, n=1, k=1), whole("claude-security-review", S)] + open_thread = thread(self.FOOTED, author=self.ACTIONS) + result = self.bind(runs, threads=[open_thread]) + self.assertEqual(result["claude-review"].open_findings, 1) + self.assertEqual(result["claude-security-review"].open_findings, 0) + # Resolved by a person, it closes the finding. + closed = thread( + self.FOOTED, author=self.ACTIONS, resolved=True, resolved_by=MAINTAINER + ) + result = self.bind(runs, threads=[closed])["claude-review"] + self.assertEqual(result.open_findings, 0) + + def test_a_thread_without_its_footer_last_does_not(self) -> None: + # Not the lane's thread, so the recorded inline finding has none and + # stays open however the thread was resolved. + runs = [whole("claude-review", S, n=1, k=1), whole("claude-security-review", S)] + for body in ( + "finding", + "claude-lane: claude-review\nfinding", + "finding\n\nclaude-lane: claude-security-review", + ): + with self.subTest(body=body): + closed = thread( + body, author=self.ACTIONS, resolved=True, resolved_by=MAINTAINER + ) + result = self.bind(runs, threads=[closed])["claude-review"] + self.assertEqual(result.open_findings, 1) + + if __name__ == "__main__": unittest.main() diff --git a/plugins/source-control/skills/pull-request/scripts/review-venue.sh b/plugins/source-control/skills/pull-request/scripts/review-venue.sh index 2b7553a584..e6ea239017 100755 --- a/plugins/source-control/skills/pull-request/scripts/review-venue.sh +++ b/plugins/source-control/skills/pull-request/scripts/review-venue.sh @@ -3,9 +3,9 @@ # pull requests and is healthy, so a local review runs only where CI does not. # # A lane is a workflow whose pull_request runs call the org's reusable review -# workflow: ci-workflows pr-review.yml (code-review), or pr-review-security.yml -# or, from a caller whose path names security, pr-review-cli.yml -# (security-review). It is healthy when its workflow is active and its newest +# workflow: ci-workflows pr-review.yml (code-review) or pr-review-security.yml +# (security-review), or pr-review-cli.yml: security-review from a caller whose +# path names security, code-review from any other. It is healthy when its workflow is active and its newest # pull_request run that reached a verdict (completed, and neither skipped nor # cancelled) succeeded and was created within --max-age-days. A failed run whose # every failed job says failure-class=skipped-validation reached no verdict: the @@ -70,9 +70,10 @@ REUSABLE=( '^melodic-software/ci-workflows/\.github/workflows/pr-review-security\.yml@' ) # pr-review-cli.yml serves either lane through an input the run listing does -# not show, so it counts for a lane only from a caller whose path names it. +# not show, so its caller's path decides the lane: security-review when it +# names security, code-review otherwise. CLI='^melodic-software/ci-workflows/\.github/workflows/pr-review-cli\.yml@' -CLI_CALLER=('' 'security') +CLI_SECURITY=(false true) WORKFLOW_ID=("" "") WORKFLOW_PATH=("" "") PAGES=3 # Skipped-validation reds passed over per lane before it reads stale. @@ -103,9 +104,9 @@ for ((page = 1; page <= PAGES; page++)); do die 2 "cannot list the workflow runs of $REPO" for i in "${!LANES[@]}"; do [[ -n "${WORKFLOW_ID[i]}" ]] && continue - hit=$(jq -r --arg re "${REUSABLE[i]}" --arg cli "$CLI" --arg caller "${CLI_CALLER[i]}" ' + hit=$(jq -r --arg re "${REUSABLE[i]}" --arg cli "$CLI" --argjson sec "${CLI_SECURITY[i]}" ' [.workflow_runs[]? | select(any(.referenced_workflows[]?; .path | test($re)) - or ($caller != "" and (.path // "" | test($caller)) + or ((.path // "" | test("security")) == $sec and any(.referenced_workflows[]?; .path | test($cli))))][0] | select(. != null) | "\(.workflow_id)\t\(.path)"' <<<"$runs") || die 2 "cannot parse the workflow runs of $REPO" diff --git a/plugins/source-control/skills/pull-request/scripts/review-venue.test.sh b/plugins/source-control/skills/pull-request/scripts/review-venue.test.sh index a36f9e5cff..ddaefbb633 100755 --- a/plugins/source-control/skills/pull-request/scripts/review-venue.test.sh +++ b/plugins/source-control/skills/pull-request/scripts/review-venue.test.sh @@ -322,7 +322,8 @@ run >/dev/null 2>&1 assert_exit "an unparsable run listing fails closed" 2 "$?" # pr-review-cli.yml serves either lane; it counts for security-review only from -# a caller whose path names security, so a code-lane caller of it is not taken. +# a caller whose path names security, so the security lane never takes a +# code-lane caller of it. CLI='melodic-software/ci-workflows/.github/workflows/pr-review-cli.yml@83b739f625b501ad40c65dee7168d836883195e1' reset page 1 "$(other_run)" "$(lane_run 11 .github/workflows/pr-review-hosted.yml "$CODE")" \ @@ -337,6 +338,22 @@ assert_contains "a security caller of pr-review-cli covers the security lane" "$ assert_contains "the security line names the pr-review-cli caller" "$out" \ ".github/workflows/pr-review-security-hosted.yml" +# Both lanes on pr-review-cli: code-review takes the caller whose path does not +# name security, even when the security caller's run is listed first. +reset +page 1 "$(other_run)" "$(lane_run 22 .github/workflows/pr-review-security-hosted.yml "$CLI")" \ + "$(lane_run 11 .github/workflows/pr-review-hosted.yml "$CLI")" +wf 11 completed:success:2 +wf 22 completed:failure:2 +out=$(run) +assert_exit "two pr-review-cli callers exit 0" 0 "$?" +assert_contains "a code caller of pr-review-cli covers the code lane" "$out" \ + "code-review ci healthy" +assert_contains "the code line names its own caller" "$out" \ + "(.github/workflows/pr-review-hosted.yml)" +assert_contains "the security lane keeps its own caller's verdict" "$out" \ + "security-review local failing" + # pr-review-cli writes the class after the lane id: still a skipped-validation red. reset page 1 "${BOTH_PAGE[@]}"