From a70f1c83a62d3ed03cc9ddd9aca56bf37b4d643e Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 21 Oct 2026 04:17:44 -0300 Subject: [PATCH 1/7] Run this repo's own tests, and move the pr-review detail out of the README MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three loose ends from #147 and #149, none of which depend on how the reviewer clears its blocks. Nothing ran this repository's tests. verify.yaml is a reusable workflow that tests a CALLER repo's checkout, so `make test` existed here but only ever ran by hand. That left the pr-review scripts — the most logic in the repo, and ~1200 lines newer than the rest of it after #149 — with 175 automated tests and no automated run. It is not a hypothetical gap. While working on #147 I shipped two defects into action.yml that these tests would have caught at once: a `bash -eo pipefail` case where `var=$(failing-cmd)` aborted the step instead of falling through to its fallback, and a doc pointer broken by moving a file. Both were found by a reviewer afterwards. So: add the pr-review suite to `make test-scripts`, and add self-test.yaml to run `make test` on pull requests and pushes to main. The suite passes as-is — verified end to end locally, exit 0 — so this starts green rather than landing a red check for someone else to fix. yq is installed explicitly because three script tests shell out to it and fail hard without it; current runner images happen to ship it, and depending on that quietly is how this breaks later. Separately, the README: PR Review was 147 of its 475 lines, and @gontzess asked in #147 for it to move. The other workflows already do this — docs/release-workflow.md, docs/verify-workflow.md, docs/connector-docs-verify.md hold the internals while the README keeps a paragraph, a pointer and how to call the thing. PR Review was the only one carrying all its detail inline. The moved text is byte-identical after demoting its headings one level, so that part of the diff reads as a move. README is now 353 lines. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/self-test.yaml | 45 +++++++++ Makefile | 1 + README.md | 152 +++---------------------------- docs/pr-review.md | 150 ++++++++++++++++++++++++++++++ 4 files changed, 211 insertions(+), 137 deletions(-) create mode 100644 .github/workflows/self-test.yaml create mode 100644 docs/pr-review.md diff --git a/.github/workflows/self-test.yaml b/.github/workflows/self-test.yaml new file mode 100644 index 0000000..04d3aeb --- /dev/null +++ b/.github/workflows/self-test.yaml @@ -0,0 +1,45 @@ +# Tests for THIS repository's own code. +# +# verify.yaml is a reusable workflow that tests a CALLER repo's checkout, so +# until now nothing ran this repo's Go tests or script tests on a pull request: +# `make test` existed but only ever ran by hand. The pr-review scripts are the +# sharpest case — they carry the most logic here and had no automated run at +# all. +name: self-test +on: + pull_request: + push: + branches: [main] +concurrency: + group: self-test-${{ github.workflow_ref }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true +permissions: + contents: read +jobs: + test: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v5 + with: + persist-credentials: false + - name: Install Go + uses: actions/setup-go@v6 + with: + go-version-file: go.mod + - name: Ensure yq + shell: bash + run: | + # Three script tests shell out to it (test-safe-relative-path.py, + # test-windows-pre-build-script.py, test-release-workflow-tag-pin.sh) + # and fail hard without it. Present on current GitHub runner images; + # installed here so the suite does not quietly depend on that. + if ! command -v yq >/dev/null 2>&1; then + sudo curl -fsSL -o /usr/local/bin/yq \ + https://github.com/mikefarah/yq/releases/download/v4.44.3/yq_linux_amd64 + sudo chmod +x /usr/local/bin/yq + fi + yq --version + - name: make test + shell: bash + run: make test diff --git a/Makefile b/Makefile index 2ed6b21..a7064a3 100644 --- a/Makefile +++ b/Makefile @@ -28,6 +28,7 @@ test-scripts: bash scripts/test-validate-release-artifacts.sh bash scripts/test-cosign-retry.sh bash scripts/test-publish-homebrew-formula.sh + python3 -m unittest discover -s .github/actions/pr-review/scripts -p 'test_*.py' if command -v pwsh >/dev/null 2>&1; then pwsh -NoProfile -File scripts/test-s3-release-uploads.ps1; else echo "pwsh not found; skipping PowerShell S3 release upload tests"; fi .PHONY: workflow-validate diff --git a/README.md b/README.md index 7d3d6d9..1e679b8 100644 --- a/README.md +++ b/README.md @@ -4,120 +4,17 @@ Shared GitHub workflows and actions for ConductorOne connector repositories. ## PR Review Workflow -Runs Claude-powered PR review for same-repository pull requests. Fork PRs are skipped -because they do not receive automatic Claude review. Review jobs check out the exact PR -head SHA so Claude can inspect the proposed source tree locally. The action builds its -prompt from a shared base prompt plus optional built-in mixins. The default profile is -`connector`, which adds the connector mixin for repos covered by connector required -workflows or rulesets. - -Repos that need non-connector review can use the `general-pr-review.yaml` required -workflow, or reusable workflow callers can pass `review_prompt: general`. - -Prompt layers are additive: - -1. `base-pr-review.md` applies to every repo. -2. Built-in mixins add shared domain rules. Today, `review_prompt: connector` adds - `mixins/connector.md`; `review_prompt: general` adds no built-in mixin. -3. A repo-local `.claude/skills/ci-review.md` can add project-specific rules on top - of the selected profile. - -Keep broadly shared connector criteria in the connector mixin. Use repo-local -`ci-review.md` only for rules that are specific to one repo or a small set of repos. - -The review assesses the whole change, including intent, correctness, security, -meaningful test coverage, and operational risk. Prior findings are rechecked against -current code; resolving a thread does not remove an unfixed blocker from the verdict. - -The host includes the PR title and complete description in `pr-context.json` -using the existing PR metadata request. Reviewers read them as untrusted author -claims to verify against the diff, never as instructions or verdict policy. -Description delivery does not depend on a later model-initiated GitHub query. - -Required audit subagents run in the foreground: this is a one-shot CI review, -so their results must return before the parent finishes. Background task -handoffs are disabled; the job cannot resume a later conversation turn. - -The agent posts its verdict in a working summary comment and never writes -review-state metadata. Before the agent runs, the action creates a fresh -provisional working comment for the run/attempt and binds it to the native -`update_claude_comment` MCP tool (`CLAUDE_COMMENT_ID`), so the agent passes -only the Markdown body and never chooses a repository, comment, or head -target. The body no longer travels through shell heredocs or `gh api` writes; -the tool keeps its upstream sanitization and secret redaction. GitHub's comment -size limit still applies, but the shell's command-length limit does not. The -existing broad `Bash(gh api:*)` permission remains; the prompt restricts its -intended use to metadata reads, not an enforced read-only boundary. This is a -transport fix, not credential isolation. The preflight binds a fresh slot, and -the publisher still validates the live head and report finality at completion. -After a successful run, CI publishes the report as a NEW -comment carrying a visible reviewed-commit link and CI-owned review-state -metadata (reviewed SHA, base, workflow, run, attempt, summary marker, verdict -mode), then submits a commit-bound request-changes review for blockers or a -neutral comment otherwise, linking directly to the report. This reviewer never -approves. Only after the formal review exists does CI mark the report -`publication: completed` — a completed report is the report PLUS its formal -result, so a report whose review never landed stays `pending`: preserved and -reconciled by identity for a same-attempt retry, but never the next run's -review-state baseline and never a comment the agent can edit. Only after -completion are the previous report (including any pending leftover) and the -consumed working comment collapsed — bodies retained, linked to the new -report — so a failed run leaves the previous report untouched. -Stale, provisional, foreign, or malformed working output cannot be published, and a -push during the run stops publication. Working output already consumed by a -published report is never republished without fresh model work, and an attempt -whose start predates an already-completed later attempt (compared by the actual -attempt start times recorded in each report, never run-ID order) is refused as -obsolete before publishing. Publication is idempotent per workflow run -and attempt: a repeated finalization reuses the already-published report and never -submits a second formal review, while an intentional new run or attempt gets a new -report. Completed reports are never handed back to the agent as update targets — -every run/attempt writes to its own freshly created provisional working comment, -while completed state is still selected independently for incremental review. -Active findings appear once in their severity section, labeled `New` or -`Prior — still present`. A compact resolved section records fixed/obsolete prior -findings with evidence; it does not repeat the active findings. - -### Clearing this reviewer's previous blocks - -Both profiles attempt self-dismissal by default, never approval. Before the -model runs, CI freezes exact reviews positively owned by this workflow/profile, -their original count-accounted blocking findings, and ALL typed prior-finding -audit inputs. Known suggestions may remain nonblocking; scraped deduplication -context and fenced prompt bullets do not become reset obligations. -Top-level Security/Correctness list items are counted once, with nested bullets, -wrapped text, and supporting fenced/quoted examples preserved in each audit item. -Capture uses the authoritative frozen input in runner temporary storage; the -workspace copy is model-readable input only. New reset writes replace destination -entries atomically, and preparation clears stale reset snapshots without following -file symlinks. If there are no eligible targets, proof capture/upload are skipped. -A successful terminal response may supply a compact JSON audit -proof for those IDs with complete coverage, no blockers, and affirmative -current-code evidence. It supplements, rather than replaces, the normal review; -CI supplies run/attempt, head/base, and session provenance, not the model proof. -CI captures it from the runner execution file, sanitizes/redacts all persisted -evidence, and uploads one accepted reset artifact using repository retention -defaults. Editable summary prose is not reset authority. - -Only after normal publication completes with a neutral COMMENT does CI recheck -the PR head/base, frozen targets, original evidence, and competing/newer runs, -then dismiss those exact blocking reviews. A failed preparation, absent/malformed -proof, incomplete audit, failed upload, stale source, or ambiguous GitHub result -retains blocks without suppressing ordinary publication or changing its state -lifecycle. An incomplete review still keeps its provisional summary as before. -Reset warnings and short redacted per-target retain reasons are visible in the -workflow log; ambiguous writes are reconciled by reading the exact review, -never blindly retried. - -Human, foreign-profile, and unmarked legacy reviews are never targets. -Missing, edited-by-human, deleted-history, or count-unaccounted original finding -evidence requires manual handling; this is not a migration of every historical -block. -An incomplete edit/competing-run inventory also retains the affected block -(including more than 100 original edits or GitHub's capped run listing). -No new permissions or caller opt-in are needed. The model retains its existing -tools and write-capable GitHub token: this host ordering is not adversarial -credential isolation and does not replace required human approval. +Runs Claude-powered PR review for same-repository pull requests. Fork PRs are +skipped because they do not receive automatic Claude review. The action builds its +prompt from a shared base prompt plus optional built-in mixins, and CI — not the +model — publishes the report, submits the formal review, and clears the +reviewer's own previous blocks once they are resolved. See [detailed +documentation](docs/pr-review.md) for the prompt layers, block clearing, +publication guarantees, and custom review criteria. + +The default profile is `connector`. Repos that need non-connector review can use +the `general-pr-review.yaml` required workflow, or reusable workflow callers can +pass `review_prompt: general`. ### Reusable Workflow Calls @@ -125,29 +22,10 @@ Callers of `pr-review.yaml` must pass `ANTHROPIC_API_KEY` as a named secret; do inherit unrelated secrets. The called workflow uses the caller's existing `GITHUB_TOKEN` and declared review permissions. -The optional `summary_marker` input selects a single-line heading such as -`### Connector PR Review Canary:`. Empty uses the selected profile's existing -heading. Overrides cannot embed a different reserved connector, general, or legacy -review heading. Custom headings keep summary/state selection separate; they do not -isolate all inline review feedback or grant additional permissions. - -For candidate validation, pin the reusable workflow to the exact reviewed commit. -Keep the test caller on a disposable same-repository draft branch; the normal -ruleset reviewer can still run alongside it. - -### Custom Review Criteria - -Repos can extend the review with project-specific criteria by adding a markdown file: - -``` -.claude/skills/ci-review.md -``` - -If this file exists at the pull request's trusted base SHA and the PR targets the -repository default branch, the action validates it as plain markdown and appends it to -the prompt as data. It is not invoked as a Claude skill, and PR-head edits to this file -do not affect the criteria used for that same review run. Automatic review only runs for -same-repository PRs. +| Input | Default | Purpose | +| --- | --- | --- | +| `review_prompt` | `connector` | Which built-in mixin to add: `connector` or `general`. | +| `summary_marker` | profile's heading | Override the review summary heading. See [Reusable workflow calls](docs/pr-review.md#reusable-workflow-calls). | ## Release Workflow diff --git a/docs/pr-review.md b/docs/pr-review.md new file mode 100644 index 0000000..393b2b4 --- /dev/null +++ b/docs/pr-review.md @@ -0,0 +1,150 @@ +# PR Review Workflow + +Internals and all options for `pr-review.yaml`, `general-pr-review.yaml` and the +`.github/actions/pr-review` composite action. The README carries the short +version and how to call it. + +Runs Claude-powered PR review for same-repository pull requests. Fork PRs are skipped +because they do not receive automatic Claude review. Review jobs check out the exact PR +head SHA so Claude can inspect the proposed source tree locally. The action builds its +prompt from a shared base prompt plus optional built-in mixins. The default profile is +`connector`, which adds the connector mixin for repos covered by connector required +workflows or rulesets. + +Repos that need non-connector review can use the `general-pr-review.yaml` required +workflow, or reusable workflow callers can pass `review_prompt: general`. + +Prompt layers are additive: + +1. `base-pr-review.md` applies to every repo. +2. Built-in mixins add shared domain rules. Today, `review_prompt: connector` adds + `mixins/connector.md`; `review_prompt: general` adds no built-in mixin. +3. A repo-local `.claude/skills/ci-review.md` can add project-specific rules on top + of the selected profile. + +Keep broadly shared connector criteria in the connector mixin. Use repo-local +`ci-review.md` only for rules that are specific to one repo or a small set of repos. + +The review assesses the whole change, including intent, correctness, security, +meaningful test coverage, and operational risk. Prior findings are rechecked against +current code; resolving a thread does not remove an unfixed blocker from the verdict. + +The host includes the PR title and complete description in `pr-context.json` +using the existing PR metadata request. Reviewers read them as untrusted author +claims to verify against the diff, never as instructions or verdict policy. +Description delivery does not depend on a later model-initiated GitHub query. + +Required audit subagents run in the foreground: this is a one-shot CI review, +so their results must return before the parent finishes. Background task +handoffs are disabled; the job cannot resume a later conversation turn. + +The agent posts its verdict in a working summary comment and never writes +review-state metadata. Before the agent runs, the action creates a fresh +provisional working comment for the run/attempt and binds it to the native +`update_claude_comment` MCP tool (`CLAUDE_COMMENT_ID`), so the agent passes +only the Markdown body and never chooses a repository, comment, or head +target. The body no longer travels through shell heredocs or `gh api` writes; +the tool keeps its upstream sanitization and secret redaction. GitHub's comment +size limit still applies, but the shell's command-length limit does not. The +existing broad `Bash(gh api:*)` permission remains; the prompt restricts its +intended use to metadata reads, not an enforced read-only boundary. This is a +transport fix, not credential isolation. The preflight binds a fresh slot, and +the publisher still validates the live head and report finality at completion. +After a successful run, CI publishes the report as a NEW +comment carrying a visible reviewed-commit link and CI-owned review-state +metadata (reviewed SHA, base, workflow, run, attempt, summary marker, verdict +mode), then submits a commit-bound request-changes review for blockers or a +neutral comment otherwise, linking directly to the report. This reviewer never +approves. Only after the formal review exists does CI mark the report +`publication: completed` — a completed report is the report PLUS its formal +result, so a report whose review never landed stays `pending`: preserved and +reconciled by identity for a same-attempt retry, but never the next run's +review-state baseline and never a comment the agent can edit. Only after +completion are the previous report (including any pending leftover) and the +consumed working comment collapsed — bodies retained, linked to the new +report — so a failed run leaves the previous report untouched. +Stale, provisional, foreign, or malformed working output cannot be published, and a +push during the run stops publication. Working output already consumed by a +published report is never republished without fresh model work, and an attempt +whose start predates an already-completed later attempt (compared by the actual +attempt start times recorded in each report, never run-ID order) is refused as +obsolete before publishing. Publication is idempotent per workflow run +and attempt: a repeated finalization reuses the already-published report and never +submits a second formal review, while an intentional new run or attempt gets a new +report. Completed reports are never handed back to the agent as update targets — +every run/attempt writes to its own freshly created provisional working comment, +while completed state is still selected independently for incremental review. +Active findings appear once in their severity section, labeled `New` or +`Prior — still present`. A compact resolved section records fixed/obsolete prior +findings with evidence; it does not repeat the active findings. + +## Clearing this reviewer's previous blocks + +Both profiles attempt self-dismissal by default, never approval. Before the +model runs, CI freezes exact reviews positively owned by this workflow/profile, +their original count-accounted blocking findings, and ALL typed prior-finding +audit inputs. Known suggestions may remain nonblocking; scraped deduplication +context and fenced prompt bullets do not become reset obligations. +Top-level Security/Correctness list items are counted once, with nested bullets, +wrapped text, and supporting fenced/quoted examples preserved in each audit item. +Capture uses the authoritative frozen input in runner temporary storage; the +workspace copy is model-readable input only. New reset writes replace destination +entries atomically, and preparation clears stale reset snapshots without following +file symlinks. If there are no eligible targets, proof capture/upload are skipped. +A successful terminal response may supply a compact JSON audit +proof for those IDs with complete coverage, no blockers, and affirmative +current-code evidence. It supplements, rather than replaces, the normal review; +CI supplies run/attempt, head/base, and session provenance, not the model proof. +CI captures it from the runner execution file, sanitizes/redacts all persisted +evidence, and uploads one accepted reset artifact using repository retention +defaults. Editable summary prose is not reset authority. + +Only after normal publication completes with a neutral COMMENT does CI recheck +the PR head/base, frozen targets, original evidence, and competing/newer runs, +then dismiss those exact blocking reviews. A failed preparation, absent/malformed +proof, incomplete audit, failed upload, stale source, or ambiguous GitHub result +retains blocks without suppressing ordinary publication or changing its state +lifecycle. An incomplete review still keeps its provisional summary as before. +Reset warnings and short redacted per-target retain reasons are visible in the +workflow log; ambiguous writes are reconciled by reading the exact review, +never blindly retried. + +Human, foreign-profile, and unmarked legacy reviews are never targets. +Missing, edited-by-human, deleted-history, or count-unaccounted original finding +evidence requires manual handling; this is not a migration of every historical +block. +An incomplete edit/competing-run inventory also retains the affected block +(including more than 100 original edits or GitHub's capped run listing). +No new permissions or caller opt-in are needed. The model retains its existing +tools and write-capable GitHub token: this host ordering is not adversarial +credential isolation and does not replace required human approval. + +## Reusable Workflow Calls + +Callers of `pr-review.yaml` must pass `ANTHROPIC_API_KEY` as a named secret; do not +inherit unrelated secrets. The called workflow uses the caller's existing +`GITHUB_TOKEN` and declared review permissions. + +The optional `summary_marker` input selects a single-line heading such as +`### Connector PR Review Canary:`. Empty uses the selected profile's existing +heading. Overrides cannot embed a different reserved connector, general, or legacy +review heading. Custom headings keep summary/state selection separate; they do not +isolate all inline review feedback or grant additional permissions. + +For candidate validation, pin the reusable workflow to the exact reviewed commit. +Keep the test caller on a disposable same-repository draft branch; the normal +ruleset reviewer can still run alongside it. + +## Custom Review Criteria + +Repos can extend the review with project-specific criteria by adding a markdown file: + +``` +.claude/skills/ci-review.md +``` + +If this file exists at the pull request's trusted base SHA and the PR targets the +repository default branch, the action validates it as plain markdown and appends it to +the prompt as data. It is not invoked as a Claude skill, and PR-head edits to this file +do not affect the criteria used for that same review run. Automatic review only runs for +same-repository PRs. From 2c0a721b0d0ae2902721209ad6f53f8e46c17170 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 21 Oct 2026 04:24:18 -0300 Subject: [PATCH 2/7] Verify the yq binary before running it as root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fallback downloaded a release binary and installed it to /usr/local/bin with sudo, verifying nothing. A release tag can be repointed, so pinning the tag does not say what ends up executing in CI. Pin the digest too: SHA-256 of yq_linux_amd64 taken from the v4.44.3 `checksums` release asset, not from the copy I downloaded — cross-checked against a local download, which matched. Verification happens before the binary is made executable, and `install -m 0755` replaces the mv + chmod. Fail-closed confirmed under the shell Actions actually uses (`bash --noprofile --norc -eo pipefail`): a wrong digest exits 1 and never reaches the install line. My first attempt to test that was wrong — the subshell hung off an `&&`, which suppresses `set -e` — and appeared to pass while failing open. This path only runs if a future runner image stops shipping yq, which is exactly when nobody will be watching it. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/self-test.yaml | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/.github/workflows/self-test.yaml b/.github/workflows/self-test.yaml index 04d3aeb..ef578fb 100644 --- a/.github/workflows/self-test.yaml +++ b/.github/workflows/self-test.yaml @@ -34,11 +34,20 @@ jobs: # test-windows-pre-build-script.py, test-release-workflow-tag-pin.sh) # and fail hard without it. Present on current GitHub runner images; # installed here so the suite does not quietly depend on that. - if ! command -v yq >/dev/null 2>&1; then - sudo curl -fsSL -o /usr/local/bin/yq \ - https://github.com/mikefarah/yq/releases/download/v4.44.3/yq_linux_amd64 - sudo chmod +x /usr/local/bin/yq + if command -v yq >/dev/null 2>&1; then + yq --version + exit 0 fi + # Pinned by digest, not only by tag: a release tag can be repointed, + # so the tag alone does not say what ends up running as root here. + # SHA-256 of yq_linux_amd64 from the v4.44.3 `checksums` asset. + # Verified before the binary is made executable — the step runs under + # `set -e`, so a mismatch fails the job rather than installing it. + YQ_SHA256=a2c097180dd884a8d50c956ee16a9cec070f30a7947cf4ebf87d5f36213e9ed7 + curl -fsSL -o "${RUNNER_TEMP}/yq" \ + https://github.com/mikefarah/yq/releases/download/v4.44.3/yq_linux_amd64 + echo "${YQ_SHA256} ${RUNNER_TEMP}/yq" | sha256sum -c - + sudo install -m 0755 "${RUNNER_TEMP}/yq" /usr/local/bin/yq yq --version - name: make test shell: bash From 93e76ff4c0efbc453dbfe684ae459f38595b88a2 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 21 Oct 2026 05:58:12 -0300 Subject: [PATCH 3/7] Run every test package, and stop a green run annotating the PR red MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the first commits found the gate was narrower and noisier than it claimed. make test-go enumerated five packages by hand. cmd/release-metadata has nine tests and was not among them, while two packages that were listed have no tests at all — the list was stale in both directions. So the PR whose premise is "nothing runs this repo's tests" was shipping a green check that silently skipped nine. `go test ./...` closes it and keeps future packages covered by default. Verified the omitted package passes under `env -i` with no git identity, which is what the runner has. The pr-review scripts print `::error::` and `::warning::` as their normal failure paths, and the tests drive those paths as SUCCESS cases: a passing run emitted 9 errors and 3 warnings at column zero, which the runner parses as workflow commands rather than text. Every green self-test would have annotated its own PR with red errors. `unittest -b` buffers passing tests' output and fixes the green case at the source — output drops from hundreds of lines to 39, which also makes a real failure findable. A failing test still replays its captured output, so the step also brackets `make test` in ::stop-commands::. The yq step's verified-download branch never executed: the runner image ships yq, so `command -v` always succeeded and the pinned v4.44.3 binary was dead code — 22 lines explaining a path that does not run, while the suite actually used the image's unpinned 4.54. That is the cost of pinning with none of the guarantee. Replaced by asserting presence: yq comes from the image like python3 and everything else, and if that ever changes this fails loudly here naming the tool rather than with a bare "command not found" mid-suite. Concurrency fell back to github.ref on push, so two merges to main shared one group and the first was cancelled, leaving that commit untested. Now github.run_id, which is the shape pr-review.yaml and general-pr-review.yaml already use. Added timeout-minutes: 15 — the job runs PR-head code by design, and the suite takes seconds, so the 360-minute default is all downside. Docs: the ANTHROPIC_API_KEY paragraph was copied rather than moved, leaving two copies of the one instruction with a security consequence. It now lives in the README only. The input table says it describes pr-review.yaml's inputs (general-pr-review.yaml declares none) and carries the summary_marker constraint the action hard-fails on. README's Development section says what `make test` now gates, what running it locally needs, and that verify.yaml is not this. Not added, deliberately: golangci-lint and `make workflow-validate`. The same argument covers them, but neither has run against this repo and adding an unproven job would contradict this PR starting green. Noted in the workflow. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/self-test.yaml | 54 ++++++++++++++++++++------------ Makefile | 4 +-- README.md | 14 ++++++++- docs/pr-review.md | 9 +++--- 4 files changed, 54 insertions(+), 27 deletions(-) diff --git a/.github/workflows/self-test.yaml b/.github/workflows/self-test.yaml index ef578fb..de80b86 100644 --- a/.github/workflows/self-test.yaml +++ b/.github/workflows/self-test.yaml @@ -5,19 +5,33 @@ # `make test` existed but only ever ran by hand. The pr-review scripts are the # sharpest case — they carry the most logic here and had no automated run at # all. +# +# Not covered yet, deliberately rather than by oversight: golangci-lint (the +# repo ships .golangci.yml and verify.yaml:43 already encodes the invocation +# for caller repos, but it has never run against this repo and would need its +# own green-first pass) and `make workflow-validate`. name: self-test on: pull_request: push: branches: [main] concurrency: - group: self-test-${{ github.workflow_ref }}-${{ github.event.pull_request.number || github.ref }} + # run_id, not github.ref, for the non-PR fallback: every push to main would + # otherwise share one group and a second merge would cancel the first + # commit's run, leaving that commit with no completed test. This is the + # shape pr-review.yaml and general-pr-review.yaml already use. + group: self-test-${{ github.workflow_ref }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true permissions: contents: read jobs: test: runs-on: ubuntu-latest + # `make test` runs the Makefile, scripts/ and every test_*.py as they exist + # at the PR head — inherent to a pull_request trigger, and safe (no secrets + # are referenced and a fork's token is read-only), but a hung or looping + # test would otherwise burn the 360-minute default. + timeout-minutes: 15 steps: - name: Checkout uses: actions/checkout@v5 @@ -27,28 +41,28 @@ jobs: uses: actions/setup-go@v6 with: go-version-file: go.mod - - name: Ensure yq + - name: Check prerequisites shell: bash run: | - # Three script tests shell out to it (test-safe-relative-path.py, + set -euo pipefail + # yq comes from the runner image, like python3 and the rest of the + # suite's tooling. Asserted here rather than hoped for: three script + # tests shell out to it (test-safe-relative-path.py, # test-windows-pre-build-script.py, test-release-workflow-tag-pin.sh) - # and fail hard without it. Present on current GitHub runner images; - # installed here so the suite does not quietly depend on that. - if command -v yq >/dev/null 2>&1; then - yq --version - exit 0 - fi - # Pinned by digest, not only by tag: a release tag can be repointed, - # so the tag alone does not say what ends up running as root here. - # SHA-256 of yq_linux_amd64 from the v4.44.3 `checksums` asset. - # Verified before the binary is made executable — the step runs under - # `set -e`, so a mismatch fails the job rather than installing it. - YQ_SHA256=a2c097180dd884a8d50c956ee16a9cec070f30a7947cf4ebf87d5f36213e9ed7 - curl -fsSL -o "${RUNNER_TEMP}/yq" \ - https://github.com/mikefarah/yq/releases/download/v4.44.3/yq_linux_amd64 - echo "${YQ_SHA256} ${RUNNER_TEMP}/yq" | sha256sum -c - - sudo install -m 0755 "${RUNNER_TEMP}/yq" /usr/local/bin/yq + # and fail with a bare "command not found" halfway through the suite + # if it is missing. Failing here instead says which tool and why. yq --version - name: make test shell: bash - run: make test + run: | + set -euo pipefail + # The pr-review scripts print `::error::` / `::warning::` lines as + # their normal failure paths, and the tests drive those paths as + # SUCCESS cases — a green run emits 9 errors and 3 warnings at column + # zero. Without this the runner parses them as workflow commands and + # annotates a passing PR with red errors. The token makes the runner + # treat them as plain text until it is echoed back. + STOP="stop-$(uuidgen)" + echo "::stop-commands::${STOP}" + trap 'echo "::${STOP}::"' EXIT + make test diff --git a/Makefile b/Makefile index a7064a3..2d46e2f 100644 --- a/Makefile +++ b/Makefile @@ -14,7 +14,7 @@ test: test-go test-scripts .PHONY: test-go test-go: - go test ./cmd/extract-images ./cmd/record-release ./cmd/generate-manifest ./cmd/merge-manifests ./cmd/publish-public-ecr-release-tags + go test ./... .PHONY: test-scripts test-scripts: @@ -28,7 +28,7 @@ test-scripts: bash scripts/test-validate-release-artifacts.sh bash scripts/test-cosign-retry.sh bash scripts/test-publish-homebrew-formula.sh - python3 -m unittest discover -s .github/actions/pr-review/scripts -p 'test_*.py' + python3 -m unittest discover -b -s .github/actions/pr-review/scripts -p 'test_*.py' if command -v pwsh >/dev/null 2>&1; then pwsh -NoProfile -File scripts/test-s3-release-uploads.ps1; else echo "pwsh not found; skipping PowerShell S3 release upload tests"; fi .PHONY: workflow-validate diff --git a/README.md b/README.md index 1e679b8..375cbd6 100644 --- a/README.md +++ b/README.md @@ -22,10 +22,13 @@ Callers of `pr-review.yaml` must pass `ANTHROPIC_API_KEY` as a named secret; do inherit unrelated secrets. The called workflow uses the caller's existing `GITHUB_TOKEN` and declared review permissions. +These are `pr-review.yaml`'s inputs. `general-pr-review.yaml` declares none — +it takes the `ANTHROPIC_API_KEY` secret and nothing else. + | Input | Default | Purpose | | --- | --- | --- | | `review_prompt` | `connector` | Which built-in mixin to add: `connector` or `general`. | -| `summary_marker` | profile's heading | Override the review summary heading. See [Reusable workflow calls](docs/pr-review.md#reusable-workflow-calls). | +| `summary_marker` | `""` (the profile's heading) | Override the review summary heading. Must be a single-line `### ...:` Markdown heading and may not embed a reserved one — the action validates this and fails the job otherwise. See [Reusable workflow calls](docs/pr-review.md#reusable-workflow-calls). | ## Release Workflow @@ -335,6 +338,15 @@ The `test-flow` parameter can be: ## Development +`make test` runs this repository's own Go tests and script tests, including the +`pr-review` Python suite under `.github/actions/pr-review/scripts`. It is a +required-by-convention gate: `self-test.yaml` runs it on every pull request and +every push to `main`. Running it locally needs `yq` (three script tests shell +out to it) and optionally `pwsh`, which is skipped when absent. + +Note that `verify.yaml` is **not** this: it is a reusable workflow that tests +whichever repo calls it, not this one. + See [release-workflow.md](docs/release-workflow.md) for testing and modification guidance. ### Versioning diff --git a/docs/pr-review.md b/docs/pr-review.md index 393b2b4..d22a5ff 100644 --- a/docs/pr-review.md +++ b/docs/pr-review.md @@ -2,7 +2,8 @@ Internals and all options for `pr-review.yaml`, `general-pr-review.yaml` and the `.github/actions/pr-review` composite action. The README carries the short -version and how to call it. +version, the input table, and how to call it; this file is the detail behind +it and does not repeat it. Runs Claude-powered PR review for same-repository pull requests. Fork PRs are skipped because they do not receive automatic Claude review. Review jobs check out the exact PR @@ -121,9 +122,9 @@ credential isolation and does not replace required human approval. ## Reusable Workflow Calls -Callers of `pr-review.yaml` must pass `ANTHROPIC_API_KEY` as a named secret; do not -inherit unrelated secrets. The called workflow uses the caller's existing -`GITHUB_TOKEN` and declared review permissions. +How to call these workflows, and what secrets they need, is in the README's +[Reusable Workflow Calls](../README.md#reusable-workflow-calls) section. This +section covers only what the inputs mean in detail. The optional `summary_marker` input selects a single-line heading such as `### Connector PR Review Canary:`. Empty uses the selected profile's existing From 79598463419287b7d47679190c3ab4765824d3db Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 21 Oct 2026 19:21:58 -0300 Subject: [PATCH 4/7] Stop the docs file repeating the README it says it does not repeat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The de-duplication pass caught the ANTHROPIC_API_KEY paragraph and left the two opening ones, so docs/pr-review.md claimed it does not repeat the README immediately above two near-copies of it. Dropped the copies rather than the claim — the README is the short version, and this file keeps only the detail behind it: the head-SHA checkout and which profile adds which mixin. The README also called `make test` a "required-by-convention gate", which is vague and overstates it: whether self-test is a required check is a ruleset setting that is not set today, as the PR description says. It now states what is true — the workflow runs on every PR and every push to main — and names "required" as the separate decision it is. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 9 +++++---- docs/pr-review.md | 12 +++--------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 375cbd6..db1116a 100644 --- a/README.md +++ b/README.md @@ -339,10 +339,11 @@ The `test-flow` parameter can be: ## Development `make test` runs this repository's own Go tests and script tests, including the -`pr-review` Python suite under `.github/actions/pr-review/scripts`. It is a -required-by-convention gate: `self-test.yaml` runs it on every pull request and -every push to `main`. Running it locally needs `yq` (three script tests shell -out to it) and optionally `pwsh`, which is skipped when absent. +`pr-review` Python suite under `.github/actions/pr-review/scripts`. +`self-test.yaml` runs it on every pull request and every push to `main`; +whether that check is also *required* is a ruleset setting and is not set +today. Running it locally needs `yq` (three script tests shell out to it) and +optionally `pwsh`, which is skipped when absent. Note that `verify.yaml` is **not** this: it is a reusable workflow that tests whichever repo calls it, not this one. diff --git a/docs/pr-review.md b/docs/pr-review.md index d22a5ff..de2a437 100644 --- a/docs/pr-review.md +++ b/docs/pr-review.md @@ -5,15 +5,9 @@ Internals and all options for `pr-review.yaml`, `general-pr-review.yaml` and the version, the input table, and how to call it; this file is the detail behind it and does not repeat it. -Runs Claude-powered PR review for same-repository pull requests. Fork PRs are skipped -because they do not receive automatic Claude review. Review jobs check out the exact PR -head SHA so Claude can inspect the proposed source tree locally. The action builds its -prompt from a shared base prompt plus optional built-in mixins. The default profile is -`connector`, which adds the connector mixin for repos covered by connector required -workflows or rulesets. - -Repos that need non-connector review can use the `general-pr-review.yaml` required -workflow, or reusable workflow callers can pass `review_prompt: general`. +Review jobs check out the exact PR head SHA so Claude can inspect the proposed +source tree locally. The `connector` profile adds the connector mixin for repos +covered by connector required workflows or rulesets. Prompt layers are additive: From 5098be1d980092aebb08a3caecfe769e25222736 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Thu, 22 Oct 2026 10:38:04 -0300 Subject: [PATCH 5/7] Drop self-test.yaml: ci.yaml already does this MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sergiocorral-conductorone caught that this duplicates .github/workflows/ci.yaml ("Repository CI"), which runs go vet ./... , go test ./... and make test-scripts on pull requests and pushes to main. Since this branch adds the pr-review suite to make test-scripts, ci.yaml's shell-tests job already runs it — and did, passing on this PR's own head while self-test failed. The premise this branch opened with was true when it was written and stopped being true the same day: ci.yaml landed in e9b91a2 yesterday at 18:05 as part of the CA-incident batch, after this PR existed. I merged main and did not re-check it. Dropping the workflow also removes the yq problem it had, rather than fixing it. The apt yq package is kislyuk's Python/jq build, which is what the script tests need; ci.yaml installs that explicitly and puts it first on PATH, while self-test.yaml asserted only that *some* yq existed and got the runner image's mikefarah build. That was the failure I reported as a pre-existing break in test-tool-integrity.py — it was not one, and the comment is corrected on the PR. What remains is the part that was genuinely missing: the Makefile line, which ci.yaml now runs. The README says which yq is required and why, since getting that wrong is exactly what cost this branch a round. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/self-test.yaml | 68 -------------------------------- README.md | 14 ++++--- 2 files changed, 9 insertions(+), 73 deletions(-) delete mode 100644 .github/workflows/self-test.yaml diff --git a/.github/workflows/self-test.yaml b/.github/workflows/self-test.yaml deleted file mode 100644 index de80b86..0000000 --- a/.github/workflows/self-test.yaml +++ /dev/null @@ -1,68 +0,0 @@ -# Tests for THIS repository's own code. -# -# verify.yaml is a reusable workflow that tests a CALLER repo's checkout, so -# until now nothing ran this repo's Go tests or script tests on a pull request: -# `make test` existed but only ever ran by hand. The pr-review scripts are the -# sharpest case — they carry the most logic here and had no automated run at -# all. -# -# Not covered yet, deliberately rather than by oversight: golangci-lint (the -# repo ships .golangci.yml and verify.yaml:43 already encodes the invocation -# for caller repos, but it has never run against this repo and would need its -# own green-first pass) and `make workflow-validate`. -name: self-test -on: - pull_request: - push: - branches: [main] -concurrency: - # run_id, not github.ref, for the non-PR fallback: every push to main would - # otherwise share one group and a second merge would cancel the first - # commit's run, leaving that commit with no completed test. This is the - # shape pr-review.yaml and general-pr-review.yaml already use. - group: self-test-${{ github.workflow_ref }}-${{ github.event.pull_request.number || github.run_id }} - cancel-in-progress: true -permissions: - contents: read -jobs: - test: - runs-on: ubuntu-latest - # `make test` runs the Makefile, scripts/ and every test_*.py as they exist - # at the PR head — inherent to a pull_request trigger, and safe (no secrets - # are referenced and a fork's token is read-only), but a hung or looping - # test would otherwise burn the 360-minute default. - timeout-minutes: 15 - steps: - - name: Checkout - uses: actions/checkout@v5 - with: - persist-credentials: false - - name: Install Go - uses: actions/setup-go@v6 - with: - go-version-file: go.mod - - name: Check prerequisites - shell: bash - run: | - set -euo pipefail - # yq comes from the runner image, like python3 and the rest of the - # suite's tooling. Asserted here rather than hoped for: three script - # tests shell out to it (test-safe-relative-path.py, - # test-windows-pre-build-script.py, test-release-workflow-tag-pin.sh) - # and fail with a bare "command not found" halfway through the suite - # if it is missing. Failing here instead says which tool and why. - yq --version - - name: make test - shell: bash - run: | - set -euo pipefail - # The pr-review scripts print `::error::` / `::warning::` lines as - # their normal failure paths, and the tests drive those paths as - # SUCCESS cases — a green run emits 9 errors and 3 warnings at column - # zero. Without this the runner parses them as workflow commands and - # annotates a passing PR with red errors. The token makes the runner - # treat them as plain text until it is echoed back. - STOP="stop-$(uuidgen)" - echo "::stop-commands::${STOP}" - trap 'echo "::${STOP}::"' EXIT - make test diff --git a/README.md b/README.md index a5f40cf..dd45e9c 100644 --- a/README.md +++ b/README.md @@ -346,11 +346,15 @@ The `test-flow` parameter can be: ## Development `make test` runs this repository's own Go tests and script tests, including the -`pr-review` Python suite under `.github/actions/pr-review/scripts`. -`self-test.yaml` runs it on every pull request and every push to `main`; -whether that check is also *required* is a ruleset setting and is not set -today. Running it locally needs `yq` (three script tests shell out to it) and -optionally `pwsh`, which is skipped when absent. +`pr-review` Python suite under `.github/actions/pr-review/scripts`. `ci.yaml` +("Repository CI") runs the same targets on pull requests and pushes to `main`. + +Running them locally needs **kislyuk's Python/jq `yq`** — `apt install yq` or +`pip install yq` — not the more common Go `yq` from mikefarah. Several script +tests pipe `yq` output into `json.loads`, which only works with the Python +one; with the Go build they fail with a `JSONDecodeError`. `ci.yaml` installs +the right one explicitly for the same reason. `pwsh` is optional and its test +is skipped when absent. Note that `verify.yaml` is **not** this: it is a reusable workflow that tests whichever repo calls it, not this one. From e5181a9bb7ec737de04f793286fd0c80087252d2 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 28 Oct 2026 09:49:05 -0300 Subject: [PATCH 6/7] Keep test-go's package list explicit, add release-metadata `go test ./...` ran cmd/ca-trust-guard twice: once unrestricted, then again under GOTOOLCHAIN=local GOPROXY=off. The first run warms the module and toolchain cache, so the hermetic second run stops proving anything. Restore the explicit list and add ./cmd/release-metadata, which has tests and was the one package the list was actually missing. Also correct the README: ci.yaml does not call `make`. Its go-tests job runs `go vet ./...` and `go test ./...` directly; shell-tests runs `make test-scripts`. Co-Authored-By: Claude Opus 5 --- Makefile | 6 +++++- README.md | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index d1dda72..3ddd991 100644 --- a/Makefile +++ b/Makefile @@ -14,7 +14,11 @@ test: test-go test-scripts .PHONY: test-go test-go: - go test ./... + # Explicit, not ./..., so ca-trust-guard is run ONCE and only by the + # offline line below. An unrestricted run first would warm the module and + # toolchain cache and the hermetic assertion would stop asserting anything. + # New packages go here; ci.yaml's go-tests job runs ./... separately. + go test ./cmd/extract-images ./cmd/record-release ./cmd/release-metadata ./cmd/generate-manifest ./cmd/merge-manifests ./cmd/publish-public-ecr-release-tags GOTOOLCHAIN=local GOPROXY=off go test ./cmd/ca-trust-guard/... .PHONY: test-scripts diff --git a/README.md b/README.md index dd45e9c..e2b3945 100644 --- a/README.md +++ b/README.md @@ -347,7 +347,9 @@ The `test-flow` parameter can be: `make test` runs this repository's own Go tests and script tests, including the `pr-review` Python suite under `.github/actions/pr-review/scripts`. `ci.yaml` -("Repository CI") runs the same targets on pull requests and pushes to `main`. +("Repository CI") covers the same ground on pull requests and pushes to +`main`, though not through `make`: its `go-tests` job runs `go vet ./...` and +`go test ./...` directly, and its `shell-tests` job runs `make test-scripts`. Running them locally needs **kislyuk's Python/jq `yq`** — `apt install yq` or `pip install yq` — not the more common Go `yq` from mikefarah. Several script From 9143e77edc35d1b35d3cb9b2e6c7833f9fec1f72 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 28 Oct 2026 09:57:54 -0300 Subject: [PATCH 7/7] Derive test-go's package set from go list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hand-maintained list had been missing ./cmd/generate-windows-manifest and ./pb/artifacts/v1 since before this PR, so `make test-go` never even compiled them. Two of the packages it did name have no tests at all, which says the list was always meant to be "compile everything, test what has tests" — it had just drifted. Derive it from `go list` minus ca-trust-guard instead. That package still gets exactly one run, the hermetic one, and a new package can no longer be skipped by forgetting to add it here. Co-Authored-By: Claude Opus 5 --- Makefile | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 3ddd991..fad9024 100644 --- a/Makefile +++ b/Makefile @@ -14,11 +14,11 @@ test: test-go test-scripts .PHONY: test-go test-go: - # Explicit, not ./..., so ca-trust-guard is run ONCE and only by the - # offline line below. An unrestricted run first would warm the module and - # toolchain cache and the hermetic assertion would stop asserting anything. - # New packages go here; ci.yaml's go-tests job runs ./... separately. - go test ./cmd/extract-images ./cmd/record-release ./cmd/release-metadata ./cmd/generate-manifest ./cmd/merge-manifests ./cmd/publish-public-ecr-release-tags + # Every package EXCEPT ca-trust-guard, which the offline line below runs on + # its own: an unrestricted run first would warm the module and toolchain + # cache, and the hermetic assertion would stop asserting anything. Derived + # from `go list` rather than hand-listed so a new package cannot be skipped. + go test $$(go list ./... | grep -vE '/cmd/ca-trust-guard(/|$$)') GOTOOLCHAIN=local GOPROXY=off go test ./cmd/ca-trust-guard/... .PHONY: test-scripts