Repository navigation
ci: run the existing test suite on every pull request - #86
Conversation
The repository has 234 tests and no workflow that runs them. Only claude-review.yml and release-please.yml exist, so a change could break the entire suite and still merge green. This adds the missing gate. Scope is deliberately narrow: run the tests that already exist, on the current dependency set. No dependency file changes, no linter, no style requirements — those are separate follow-ups, so that a failure here is unambiguous about its cause. Details worth noting: - pytest is not in requirements.txt, which pins only the runtime harness, so it is installed inline. A follow-up moves both into a pyproject dev group with a lockfile and this becomes `uv sync --frozen`. - The Docker smoke build is gated on the tests rather than run beside them. The image installs Node, the gh CLI and a large SDK wheel, so it is much slower than the suite; this keeps fast feedback fast and skips a costly build when the tests are already red. - Actions are pinned by commit SHA, matching claude-review.yml in this repository. It is public and handles GitHub App and OAuth tokens. - cancel-in-progress applies to pull requests only. Runs on main are each the record for a distinct merged commit and should not be cancelled. - No Telegram notification: unlike mctl-api, this repository has no TELEGRAM_BOT_TOKEN or TELEGRAM_CHAT_ID secret, so the step would fail. Adding one needs those secrets first and is out of scope here.
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 1m 49s —— View job Review complete
Verdict: approved — No P1/P2 findings (1 P3). The workflow is well-scoped and internally consistent:
One P3 nit posted inline: neither job sets Note flagged in the PR body itself and worth restating: this workflow makes the tests run, but |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| jobs: | ||
| tests: | ||
| name: tests | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
P3: neither job sets timeout-minutes, so a hung pytest run or a stuck docker buildx build falls back to the GitHub default 6-hour job timeout. Not a correctness bug, but worth capping (e.g. timeout-minutes: 10 here, timeout-minutes: 20 on docker-build) so a hang doesn't silently tie up a runner slot / block the concurrency group for hours instead of failing fast.
Review finding (P3): neither job set timeout-minutes, so a wedged pytest or a stuck buildx build would fall back to GitHub's 6-hour default. That ties up a runner slot and, because the workflow uses a concurrency group, blocks subsequent runs on the same ref for hours instead of failing fast. 10 minutes for the tests (the suite runs in seconds) and 20 for the image build, which pulls apt packages, a global npm install and a large SDK wheel.
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 49s —— View job Reviewing PR #86 (synchronize event)
Verdict: approved — No P1/P2 findings (0 P3). This push adds No new issues found. The rest of the file is unchanged from the previously-approved version: read-only |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a13982c1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| contents: read | ||
|
|
||
| concurrency: | ||
| group: pr-validation-${{ github.ref }} |
There was a problem hiding this comment.
When three or more pushes to main arrive before validation drains, this shared github.ref group does not preserve every run: GitHub concurrency retains at most one running and one pending run per group, and a newly queued run replaces the existing pending run even when cancel-in-progress is false. Consequently, an intermediate merged commit can have its validation cancelled despite the stated requirement that each main commit retain its own run; include the SHA in the group for push events or omit concurrency for them.
Useful? React with 👍 / 👎.
| on: | ||
| pull_request: |
There was a problem hiding this comment.
For release PRs created or updated by .github/workflows/release-please.yml, this trigger never runs: that workflow invokes release-please-action without a custom token, so it uses GITHUB_TOKEN, and GitHub suppresses subsequent workflow runs caused by events generated with that token. Once tests is configured as required as instructed here, release-please PRs will therefore have no satisfiable tests check; create those PRs with the existing GitHub App token or another non-GITHUB_TOKEN credential.
Useful? React with 👍 / 👎.
Review finding (P2). The previous group keyed only on github.ref, with cancel-in-progress disabled for pushes, and a comment claiming each main commit keeps its own run. It does not: GitHub retains at most one running and one pending run per group, and a newly queued run evicts the pending one regardless of cancel-in-progress. Three merges landing close together would silently drop the middle commit's validation. Pushes now get one group per commit, so no main commit's run can be evicted. Pull requests keep the shared per-ref group, where cancelling a superseded run is the desired behaviour. Also documents, at the top of the file, why this workflow cannot be made a required check before release-please is dealt with: release-please opens its PR with the default GITHUB_TOKEN, and GitHub suppresses workflow triggers for events raised by that token, so release PRs can never satisfy a required `tests` check. They are merged today with `gh pr merge --admin`, which bypasses required checks, so releases keep working while the release commit goes unvalidated. Fixing that means giving release-please-action an App token, which belongs in release-please.yml rather than here.
|
Both Codex P2s were correct and are addressed. Thanks — the second one in particular would have bitten later. P2 "Give main pushes distinct concurrency groups" — fixed. P2 "Trigger validation for release-please PRs" — confirmed, documented, deliberately not fixed here. One correction to the impact, though: release PRs in this org are merged with I have not changed |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 1m 21s —— View job Reviewing PR #86 (synchronize event)
Verdict: approved — No P1/P2 findings (0 P3). This push (974767c) addresses the Codex P2 "Give main pushes distinct concurrency groups" finding: The commit also documents, in the file header, why No new issues found. All previously flagged items (timeout-minutes P3, concurrency-group P2) are now resolved. |
What
Adds
.github/workflows/pr-validation.yml: atestsjob that runs theexisting suite on every PR and on pushes to
main, plus a Docker smoke buildgated behind it.
Why
This repository has 234 tests and nothing runs them.
.github/workflows/contained only
claude-review.ymlandrelease-please.yml, so a change couldbreak the entire suite and still merge green. Of everything surfaced by the
recent architecture review, this was the highest-severity finding — higher
than the language question that prompted the review.
Verified locally before writing the workflow:
pytest -q→ 234 passed in4.71s. The suite is healthy; it was simply never wired to CI.
Scope
Deliberately narrow — run the tests that already exist, on the current
dependency set. No dependency file changes, no linter, no new style
requirements. Those are separate follow-ups, so that a red run here is
unambiguous about its cause rather than a mix of "the tests broke" and "the
linter has opinions".
Notes on specific choices
requirements.txt, which pinsonly the runtime harness. A follow-up moves both into a
pyproject.tomldev dependency group with a lockfile, at which point this step becomes
uv sync --frozen.needs: tests) rather than runbeside them. The image installs Node, the
ghCLI and a ~210 MB SDK wheel,so it is far slower than the suite; this keeps fast feedback fast and skips
a costly build when the tests are already red. It is a no-push smoke build —
image publishing stays in mctl-gitops.
claude-review.ymlin thisrepository. It is public and handles GitHub App and OAuth tokens.
cancel-in-progressapplies to pull requests only. Runs onmainareeach the record for a distinct merged commit and should not be cancelled.
mctl-api, this repository has noTELEGRAM_BOT_TOKENorTELEGRAM_CHAT_IDsecret (onlyCLAUDE_CODE_OAUTH_TOKEN,CLAUDE_CODE_OAUTH_TOKEN_2,GITOPS_TOKEN), so the step would fail on itsfirst use. Adding one requires provisioning those secrets and is out of
scope. Worth noting it would only ever fire on
mainanyway — on PRs it isnoise, since a developer may push several intermediate commits.
This PR alone does not close the risk
Adding the workflow makes the tests run; it does not make them required.
Until the
testsjob is a required status check onmain, a PR can still bemerged without it. That is a repository setting rather than a file, so it
needs a separate, explicit step — flagged here so it does not get lost.
Verification
pytest -qlocally → 234 passedtestsanddocker-build (needs: tests)testsset as a required status check, and merge blocked when it isfailing or skipped