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.
There was a problem hiding this comment.
Removed the job condition in 3043e40, so the required Checks job executes on both push and PR runs rather than succeeding through a skip.
| - 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.
There was a problem hiding this comment.
Aligned both workflows on JDK 21 and replaced source/target settings with Maven release 20 in 3043e40. Maven packaging passed, and a focused compile check confirms Java 21-only List.getFirst() is rejected. Extension installation also passed.
…o codex/issue-125-test-workflow
Co-authored-by: Codex <noreply@openai.com>
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.