Repository navigation
[CXH-3052] Run the pr-review tests in CI, and move the pr-review detail out of the README - #155
FeliLucero1 wants to merge 6 commits into
Conversation
…EADME 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) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Superseded — see the current review report for commit
|
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) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
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) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
FeliLucero1
changed the title
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) <noreply@anthropic.com>
main gained the CA-incident work while this branch was open. The only conflict is the Makefile, where this branch replaced the hand-written test-go package list with ./... and main added a ca-trust-guard line to that same list. Kept both. ./... covers every package including ca-trust-guard, and main's second line stays because it asserts something ./... does not: that ca-trust-guard passes with GOTOOLCHAIN=local and GOPROXY=off, with no toolchain fetch and no module proxy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
|
Heads up @gontzess — this PR just went red, and it is not this PR's change. Turning Both failures are the same line shape: json.loads(subprocess.check_output(["yq", ".", str(ROOT / ".github/workflows/release.yaml")], text=True))
# json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
Confirmed it is pre-existing rather than mine: I have not touched it. It is two days old, it is yours, and it is part of the incident work, so the call on how to fix it is yours — In the meantime this PR's own check stays red, which I would rather leave visible than hide: a green |
Superseded — see the current review report for commit
|
sergiocorral-conductorone
left a comment
sergiocorral-conductorone
left a comment
There was a problem hiding this comment.
Automated review. drafted 4 → confirmed 4, low-confidence 0, dropped 0, added 0 (panel: opus-5.5 / gpt-6-astra / gemini-4-argon, all high). The Makefile and docs moves look good. The new self-test.yaml fails its own check and duplicates ci.yaml. Linked ticket CXH-3052 body could not be read, so its Fix-list coverage was not checked.
|
Correction to my comment above, and apologies for the noise @gontzess — The apt What actually failed was my I also had the history wrong. I said nothing ran Removing |
@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) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
FeliLucero1
changed the title
General PR Review: [CXH-3052] Run this repo's own tests, and move the pr-review detail out of the READMEBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe PR changes three things. Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsReviewed commit: |
|
On the one open suggestion in the latest report — the description was already rewritten, in the same sitting as
So the finding was accurate when the run read it and is not now. Noting it rather than leaving a stale "Suggestions: 1" for whoever picks this up. This is the third time in this series that a description edit landed just after the run that read it. Not the re-post problem #156 fixes — that one is about threads — and probably not worth fixing: it is inherent to editing a description after pushing. Worth knowing when reading a report, though. |
Two loose ends from #147 and #149. The workflow this PR originally added is gone — see below.
The pr-review suite was in no test target
.github/actions/pr-review/scripts/test_*.pywas in no Makefile target and no workflow, so the scripts carrying the most logic in this repo — and ~1200 lines newer than the rest after #149 — had 175 automated tests and no automated run.Not hypothetical: while working on #147 two defects shipped into
action.ymlthat these tests would have caught at once — abash -eo pipefailcase wherevar=$(failing-cmd)aborted the step instead of falling through to its fallback, and a doc pointer broken by moving the file it pointed at. Both were found by a reviewer afterwards.This adds one line to
make test-scripts.ci.yaml'sshell-testsjob runs that target, so the suite is now covered on every PR and push tomainthat touches.github/actions/**,cmd/**,scripts/**or theMakefile.What changed during review. This PR originally added a
self-test.yamlworkflow, on the premise that nothing ranmake testhere. That was true when it was written and stopped being true the same day:ci.yaml("Repository CI") landed ine9b91a2as part of the CA-incident batch, after this PR was opened. @sergiocorral-conductorone caught the duplication. The workflow is dropped; the Makefile line is the part that was actually missing.That also retired a bug of mine.
self-test.yamlasserted that someyqwas installed and got the runner image's mikefarah build, while the script tests need kislyuk's Python/jq one — whichci.yamlinstalls explicitly. I reported that failure as a pre-existing break intest-tool-integrity.py; it was not one, and that is corrected in a comment above.The README
@gontzess asked for this in #147 and was right: PR Review was 147 of the README's 475 lines. The other workflows already follow the split —
docs/release-workflow.md,docs/verify-workflow.md,docs/connector-docs-verify.mdcarry the internals while the README keeps a paragraph, a pointer, and how to call the thing. PR Review was the only one with everything inline.The moved text is byte-identical after demoting its headings one level — nothing reworded, nothing dropped — so that part of the diff reads as a move. I verified that programmatically, not by eye.
The Development section now also names which
yqis required and why, since getting that wrong is what cost this branch a round.Not included
make workflow-validate.ci.yamlalready runs shellcheck; extending it is a separate call.license-shapes.mddiscarding anok— different repo, ConductorOne/claude-marketplace#211.🤖 Generated with Claude Code