Conversation
CatarinaGamboa
left a comment
CatarinaGamboa
left a comment
There was a problem hiding this comment.
Two things in the new workflow, both about the checks not quite guarding what they're meant to.
Reviewed with Claude Code (reviewer + adversarial agents per PR, findings checked against the code before posting).
| jobs: | ||
| checks: | ||
| name: Checks | ||
| if: github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository |
There was a problem hiding this comment.
A skipped Checks job counts as passing. For PRs from branches in this repo, the pull_request run still creates a Checks job, which this if skips. GitHub reports skipped jobs as successful, and a skipped job "will not prevent a pull request from merging, even if it is a required check" (docs). This PR's head commit shows it: one Checks from the push run (passed) and one from the PR run (skipped).
Once #126 makes Checks required on main, a PR could merge while its push run is failing or still running. The simplest fix is to drop the if and accept one duplicate run per PR push. Otherwise, make sure the required check can only be satisfied by a job that actually ran.
| - name: Setup Java | ||
| uses: actions/setup-java@v4 | ||
| with: | ||
| java-version: 21 |
There was a problem hiding this comment.
CI builds the server with JDK 21, but publish.yml builds the shipped JAR with JDK 20. server/pom.xml sets <source>20</source> / <target>20</target> but no <release>, so under JDK 21 javac compiles against the JDK 21 class library and accepts 21-only APIs such as List.getFirst(). A change like that passes here and only fails when a release tag is pushed, which is the case #125 is meant to catch.
Suggest using the same JDK in both workflows, and setting <maven.compiler.release>20</maven.compiler.release> (or <release>20</release> in the compiler plugin) so javac rejects newer APIs regardless of the JDK.
2 participants
Closes #125. Depends on #137.
Check lint, TypeScript, the server build, and packaged runtime files on branch pushes and fork PRs. Allow publishing to reuse these checks.
Validated locally on Node 22 and Java 21; GitHub Actions passed.
Generated by Codex.