ci: gate openjd-cli on the OpenJD conformance suite - #234
Conversation
Signed-off-by: David Leong <leongdl@amazon.com>
| type: string | ||
|
|
||
| jobs: | ||
| conformance: |
There was a problem hiding this comment.
No permissions: block, so this job inherits the repository/org default GITHUB_TOKEN scopes rather than least privilege. Every other workflow added to this repo declares them explicitly (permissions: {} in claude_pr_review_collect.yml, contents: read in release_publish.yml, security-events: write in codeql.yml).
This job only needs to read code, and it goes on to execute a test runner from another repository (see the openjd-specifications checkout below), so leaving the token broader than necessary is worth avoiding:
permissions:
contents: readDeclaring it at the top level (or on the conformance job) keeps it consistent with the rest of .github/workflows/.
| uses: actions/checkout@v7 | ||
| with: | ||
| repository: OpenJobDescription/openjd-specifications | ||
| path: openjd-specifications |
There was a problem hiding this comment.
This checks out openjd-specifications at whatever its default branch happens to point at, and the next steps execute code from it (uv run run_openjd_cli_tests.py, plus whatever dependencies uv resolves for that script). Two consequences:
- Reproducibility — a push to
openjd-specificationsmainline can turn this PR check red without anything changing in this repo, and a previously-green commit will not necessarily re-run green. - Trust — the job runs arbitrary code from a floating ref of another repo. That is a sibling repo in the same org so the risk is modest, but combined with the inherited
GITHUB_TOKENscopes (see the earlier comment) it is worth constraining.
Consider pinning to a tag or commit SHA and bumping it deliberately, e.g.
- name: Checkout openjd-specifications
uses: actions/checkout@v7
with:
repository: OpenJobDescription/openjd-specifications
ref: <tag-or-sha>
path: openjd-specifications
persist-credentials: falsepersist-credentials: false is also worth adding here since nothing in the job pushes to that checkout.
| type: string | ||
| tag: | ||
| required: false | ||
| type: string |
There was a problem hiding this comment.
The workflow_call trigger and its tag / branch inputs are never exercised — no workflow in the repo calls ./.github/workflows/conformance.yml. Compare code_quality.yml, which declares the same input surface and is invoked from release_publish.yml:33 as the UnitTests job.
So the ref: ${{ inputs.tag }} plumbing below is currently dead: on pull_request the expression evaluates to the empty string and actions/checkout falls back to the event ref (correct behaviour, just not what the input is for). Either wire conformance into the release pipeline alongside UnitTests, or drop the workflow_call block and the ref: until it is needed — otherwise it reads as configured-and-working when it has never run.
Separately, branch is declared but unused even in the intended design (code_quality.yml has the same vestigial input, so this may just be copied).
| - name: Checkout | ||
| uses: actions/checkout@v7 | ||
| with: | ||
| ref: ${{ inputs.tag }} |
There was a problem hiding this comment.
actions/checkout defaults to persist-credentials: true, which leaves the GITHUB_TOKEN in .git/config of this checkout for the life of the job. The last step then executes code from a different repository (uv run run_openjd_cli_tests.py) in the same workspace, and that suite in turn launches job fixtures that run shell/python commands. Any of that code can read $GITHUB_WORKSPACE/.git/config and use the token.
Nothing in this job pushes, so the credential is not needed:
- name: Checkout
uses: actions/checkout@v7
with:
ref: ${{ inputs.tag }}
persist-credentials: falseThis matters more here than in a typical build job precisely because the job’s purpose is to run externally-authored fixtures — and it compounds with the inherited token scopes noted above.
| jobs: | ||
| conformance: | ||
| name: Conformance (${{ matrix.os }}) | ||
| runs-on: ${{ matrix.os }} |
There was a problem hiding this comment.
Operational note: this job has no timeout-minutes and the workflow has no concurrency group. It runs the full conformance suite across three OSes, and the suite launches job fixtures that spawn subprocesses — a fixture that hangs will hold a runner for the default 6-hour limit, ×3.
concurrency:
group: conformance-${{ github.ref }}
cancel-in-progress: true
jobs:
conformance:
timeout-minutes: 30 # tune to observed runtimeThe concurrency block also stops rapid pushes to a PR from stacking up three-OS runs; claude_pr_review.yml already uses this pattern in the repo.
Signed-off-by: David Leong <leongdl@amazon.com>
| COMMAND = [sys.executable, "-c", "print(r'OUTPUT:{{Param.Printed}}')"] | ||
| EXPECTED = ["OUTPUT:EXPECTED_VALUE"] | ||
|
|
||
| completed = subprocess.run(COMMAND, capture_output=True, text=True) |
There was a problem hiding this comment.
capture_output=True means the grandchild never writes to the action’s inherited stdout/stderr at all — the pipes are drained into completed.stdout/completed.stderr and the action then re-emits them through its own streams on lines 33-34.
So the runner only ever sees output produced directly by the process it spawned. If the CLI/session dropped grandchild output entirely, both new tests would still pass, because the bytes reach the log via the action’s own sys.stdout.write.
That undercuts the stated purpose. The template description says "losing grandchild output [...] silently weakens that whole suite", and test_self_asserting_task.py:17 is named test_grandchild_output_captured_and_assertion_passes with the comment "Missing means output from a process we did not spawn ourselves was dropped" — but nothing here exercises that path.
To actually pin grandchild output propagation the child has to inherit the streams rather than have them captured, e.g. print a second marker without capturing:
subprocess.run([sys.executable, "-c", "print(\"INHERITED:{{Param.Printed}}\")"])and assert INHERITED:EXPECTED_VALUE shows up. As written, the fixture is a valid test of action exit-status propagation (which the second test does cover), just not of grandchild output capture.
| with: | ||
| python-version: '3.12' | ||
| # The suite's job fixtures run `command: python`, which does not exist on a | ||
| # bare Ubuntu runner -- only `python3` does. A virtualenv provides both names |
There was a problem hiding this comment.
The comment claims "A virtualenv provides both names" (python and python3), but that is not true on Windows: CPython does not create a python3.exe in Scripts/ — only python.exe and pythonw.exe. A symlinked python3 exists only in POSIX venvs (bin/python, bin/python3, bin/pythonX.Y).
Not a bug for the stated need — the fixtures run command: python, which the Windows venv does provide — but if any fixture ever uses python3 the Windows leg will fail while Linux/macOS pass, and this comment would send the reader looking in the wrong place. Worth narrowing the claim to what is actually relied on (the venv provides python plus the openjd entry point) rather than asserting it covers all three names on every OS.
| something it should have rejected, or vice versa. A job test failing means `openjd run` produced the | ||
| wrong output, or exited non-zero when the fixture expected a clean run — many of the single-task job | ||
| fixtures assert their own output from inside the task and signal a mismatch through the task's exit | ||
| status, so a non-zero exit with `OPENJD_CONFORMANCE_ASSERT_FAILED` in the log is an output mismatch, |
There was a problem hiding this comment.
OPENJD_CONFORMANCE_ASSERT_FAILED does not appear anywhere in this repo, and the fixture added in this same PR emits a different string — ASSERT_FAILED: expected output line not found (test/openjd/cli/templates/self_asserting_task.yaml:39), which is what the new tests grep for.
If the external suite really does emit OPENJD_CONFORMANCE_ASSERT_FAILED, this is fine but worth double-checking against the current openjd-specifications fixtures, since a reader following this doc will grep the log for a string that may not be there. If it was extrapolated from the local fixture, the doc and the fixture should use the same marker so the instruction is actually actionable.
| # Printed by the grandchild and echoed by the action. Missing means output from a | ||
| # process we did not spawn ourselves was dropped. | ||
| assert "OUTPUT:EXPECTED_VALUE" in outerr.out, format_capsys_outerr(outerr) | ||
| assert "ASSERT_FAILED" not in outerr.out, format_capsys_outerr(outerr) |
There was a problem hiding this comment.
This negative assertion can never fire. The fixture writes the marker to stderr (sys.stderr.write("ASSERT_FAILED: ..."), self_asserting_task.yaml:39), but this checks outerr.out only. Note the sibling test on line 40 correctly checks outerr.out + outerr.err.
It is also largely redundant with expected_exit_code=0: the fixture only emits the marker on the same branch where it exits 1, so a run that reaches this line already cannot have failed the assertion. Either check both streams for symmetry with the other test, or drop the line.
What changed
Two things, both about the conformance suite:
Conformanceworkflow running theopenjd-specifications
suite against the installed
openjdcommand on Linux, macOS and Windows, plus aDEVELOPMENT.md section on running it locally.
specifications#174
depends on.
Current result: 1160 passed, 0 failed on
2023-09/*.Why the CI job
openjd-rs has had a conformance job for a while. This repo — the reference
implementation the suite's runner was written against — has had none, so a specification
regression here is caught only by whoever happens to run the suite by hand.
Why the tests
specifications#174 makes 187 conformance fixtures assert their own output: the action
runs the case's command as a child, echoes its output, and exits non-zero on mismatch.
Those verdicts only reach a runner if we do two things, neither of which had a test:
test_grandchild_output_captured_and_assertion_passestest_assertion_failure_fails_the_runRegressing either would not fail this repo's suite. It would quietly weaken 187
conformance cases into "the task ran" — which is the false-pass shape that PR exists to
remove. The fixture takes a job parameter so the second test drives a real output
mismatch rather than asserting on a contrived failure.
No behavioural change; both pass against
mainlineas-is.One wrinkle in the workflow
The job fixtures use
command: python, and a bare Ubuntu runner provides onlypython3. The workflow installs into a virtualenv outside the checkout and puts its bindirectory on
PATH, which suppliespython,python3and theopenjdentry point inone step. Verified by simulating the workflow's steps locally, not just by reading the
YAML.
Testing
2023-09/*against this CLIpythonandopenjdboth resolve from the venv; suite passeshatch run fmthatch run linthatch run test