Set up GitHub Actions CI#269
Open
YukiYang31 wants to merge 30 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds Makefile-driven generation for CI YAML and M4-based templating: a new .github/workflows/Makefile that renders .github/workflows/ci.yml from .github/workflows/ci.yml.m4 and .github/workflows/defs.m4 (post-processed by ${PLUME_SCRIPTS}/squeeze-blank-lines). Introduces .github/workflows/defs.m4 with canary/version macros and job-generator macros (cftests_job, daikon_job, plume_lib_job). Adds .github/workflows/ci.yml.m4 and the generated .github/workflows/ci.yml describing containerized build jobs (build_jdk, build_jdk21u), a canary_jobs gate, and expanded downstream test jobs across JDK 17/21/25/26. Mirrors and parameterizes these canary job definitions for Azure Pipelines via .azure/jobs.m4 and updates related Makefiles and container-image/version macros. Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml.m4:
- Line 1: Add a CI sanity step in the .github/workflows/ci.yml.m4 workflow that
runs the generator and fails the job if the generated ci.yml is out of date:
invoke make -C .github/workflows to regenerate artifacts and then run git diff
--exit-code .github/workflows/ci.yml; place this step in the appropriate
validation job (e.g., the existing lint/validate job) so PRs fail when committed
.github/workflows/ci.yml does not match the output of the .m4 source.
- Around line 79-81: The git global configs (user.email, user.name,
core.longpaths, core.protectNTFS) are duplicated between the
clone-related-jdk21u step and the git merge step; remove the redundancy by
moving those git config --global lines into a single earlier step (e.g., leave
them only in clone-related-jdk21u) or extract them into a small reusable
composite/action or shell snippet and call it from both places; update the
.github/workflows/ci.yml.m4 to reference the consolidated step instead of
repeating the four git config --global settings.
- Around line 86-105: The workflow currently uses bare `set` in the "git merge
plan" and "git merge" steps which dumps the entire shell environment to public
CI logs; remove those bare `set` calls and instead print only the specific debug
variables you actually need (e.g., echo or printf the named vars like
CI_BRANCH_NAME, CI_ORGANIZATION, GITHUB_REF, or use `declare -p VAR_NAME` for
each) or enable controlled shell tracing with `set -x`/`set +x` around the exact
commands to trace; update the occurrences of `set` in those steps to one of
these targeted approaches so the full environment is no longer exposed.
- Around line 120-130: The workflow currently calls cftests_job(inference,
cftests-inference, 21) and 25 but omits any cftests_job(inference,
cftests-inference, 17); either add a brief inline comment next to the existing
cftests_job entries explaining why the inference test is intentionally excluded
for JDK 17 (e.g., "test-cftests-inference.sh requires JDK 21+") or, if the
omission was accidental, add the missing cftests_job(inference,
cftests-inference, 17) line so JDK 17 runs inference tests—place the comment
adjacent to the cftests_job lines referencing inference to make the reasoning
obvious.
In @.github/workflows/defs.m4:
- Around line 4-7: The macros canary_os and canary_test are defined but unused,
causing confusion; either remove these unused definitions or thread them into
the job templates so they actually control runner selection. Update the job
templates referenced (cftests_job, daikon_job, plume_lib_job) to use the macros
(e.g., replace hardcoded runs-on: ubuntu-latest with runs-on: canary_os-latest
or expand canary_test appropriately) or delete the define([canary_os],
[ubuntu])dnl and define([canary_test], [canary_os[]canary_version])dnl lines
from the m4 file to avoid stale metadata. Ensure any change keeps
canary_version/latest_version behavior consistent with the other macros.
- Line 15: The workflow uses mutable image tags "mdernst/cf-ubuntu-jdk$3:latest"
(and the two other identical occurrences) which should be pinned; update each
occurrence of the literal string "mdernst/cf-ubuntu-jdk$3:latest" to use an
immutable digest (e.g. "@sha256:...") or a stable version/date tag that matches
the image variants referenced by the Azure pipeline, ensuring all three
occurrences are replaced so CI runs are deterministic and not dependent on
:latest.
- Around line 47-63: The macro plume_lib_job currently ignores any arguments and
hardcodes the container image to mdernst/cf-ubuntu-jdk17:latest; update it to
either accept a JDK/version parameter (e.g., use $1 inside the plume_lib_job
definition to build the container image tag, mirroring cftests_job) so calls
like plume_lib_job(canary_version) actually control the JDK, or remove the
argument from the call site where plume_lib_job is invoked so it's clear no
parameter is used; alternatively, if JDK17 is intentional, add a brief comment
inside the plume_lib_job macro stating that plume-lib requires JDK17 to prevent
future confusion.
In @.github/workflows/Makefile:
- Around line 1-16: Add Makefile phony and delete-on-error declarations: declare
.PHONY for the targets all, default, and clean (so files named those won't
short-circuit recipes) and add a .DELETE_ON_ERROR: directive to ensure partial
outputs (e.g., created by the m4 -> $@ redirection in the pattern rule that
generates ci.yml) are removed if the recipe fails; place these declarations near
the top of the Makefile before the rules for all/default/% and clean.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fdcd7f16-e435-493a-90c2-c6c72d1842f8
📒 Files selected for processing (4)
.github/workflows/Makefile.github/workflows/ci.yml.github/workflows/ci.yml.m4.github/workflows/defs.m4
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 1: The generated ci.yml header indicates the file is produced from
templates (ci.yml.m4 and defs.m4) but there's no enforcement that the committed
ci.yml stays in sync; add a CI job (e.g., "ci-template-check") that regenerates
ci.yml from ci.yml.m4 and defs.m4 (using the same m4 invocation used to produce
the artifact) and fails the workflow if the regenerated output differs from the
committed ci.yml, to surface template-drift as a failing check; reference the
filenames ci.yml, ci.yml.m4, defs.m4 and the new job name when implementing this
enforcement.
- Around line 5-9: Add a top-level GitHub Actions concurrency block in the
template file ci.yml.m4 (not the generated ci.yml) to cancel superseded runs for
the same ref—e.g., add a concurrency stanza with group using the ref (like
github.ref or github.workflow + github.ref) and cancel-in-progress: true so
newer pushes cancel in-flight runs; update the template near the existing
top-level "on:" declaration so generated ci.yml includes it. Also consider
changing the "on: push" trigger in ci.yml.m4 to scope branches (e.g., branches:
[master]) to avoid duplicate runs for PR branches that already trigger via
"pull_request."
- Around line 19-20: Replace the mutable third-party action tag
actions/checkout@v4 with its pinned full-length commit SHA wherever it appears
in the CI templates and macros (the ci.yml.m4 template and defs.m4 macro
definitions referenced by the PR); update each occurrence of the v4 tag to the
specific commit SHA string for actions/checkout and ensure the same SHA is used
consistently across both ci.yml.m4 and defs.m4.
In @.github/workflows/defs.m4:
- Around line 28-45: The daikon_job macro currently hardcodes the container
image "mdernst/cf-ubuntu-jdk17:latest" while only accepting the part parameter
`test_daikon_part$1`; either document that JDK 17 is required or add a second
parameter for the JDK image/version similar to how `cftests_job` uses `$3`.
Update the `daikon_job` definition (symbol: daikon_job and the step invoking
test-daikon-part$1.sh) to accept and use a new `$2` for the container image (or
replace the literal with `$2`), and add a one-line m4 comment explaining the
chosen approach so future maintainers understand why JDK17 is pinned or how to
override it.
In @.github/workflows/Makefile:
- Around line 6-16: The recipe leaves a stale intermediate file ($@.tmp) if any
step between its creation and mv -f fails; update the Makefile recipe that uses
cat -s, sed and mv -f to ensure $@.tmp is always removed on error — either run
the whole sequence in a single shell with a trap (e.g., trap 'rm -f $@.tmp'
EXIT) or rewrite the steps as safe pipelines so the intermediate file is never
left behind (or remove $@.tmp at the end of the recipe); ensure the change
references the existing .DELETE_ON_ERROR behavior and the $@.tmp temporary name.
- Line 14: The sed expression in the Makefile target uses the GNU-only escape
`\s` so blank-line trimming fails on macOS/BSD; update the sed script (the rule
containing the line starting with "sed -e :a -e '/^\s*$$/{$$d;N;ba' -e '}' $@ >
$@.tmp") to use the POSIX character class [[:space:]] in place of `\s` (i.e.,
change `/^\s*$$/` to `/^[[:space:]]*$$/`) so the command is portable across sed
implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7db8c773-94a0-44bc-b0ca-910b754f7560
📒 Files selected for processing (3)
.github/workflows/Makefile.github/workflows/ci.yml.github/workflows/defs.m4
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/ci.yml.m4 (2)
136-146: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDocument why JDK 17 deliberately omits the
inferencetest.
cftests_job(inference, cftests-inference, …)is invoked for JDK 21 and 25 but skipped for JDK 17. If this is intentional (e.g.,test-cftests-inference.shrequires JDK 21+), add a one-line m4 comment next to thecftests_jobblock so future maintainers don't reintroduce or further drift the matrix. If unintentional, add the JDK 17 invocation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml.m4 around lines 136 - 146, The JDK 17 matrix currently omits the inference test: update the cftests_job entries to either add the missing invocation cftests_job(inference, cftests-inference, 17) if omission was accidental, or add a one-line m4 comment next to the existing cftests_job block (near the lines invoking cftests_job(..., 17)) explaining that inference (test-cftests-inference.sh) requires JDK 21+ and is intentionally skipped for JDK 17; reference the cftests_job symbol and cftests-inference test name so future maintainers understand the rationale.
105-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBare
setstill dumps the full shell environment to public CI logs.Lines 106 and 119 invoke
setwith no arguments, which prints every shell and environment variable for a public-repo log. GitHub auto-masks its injected secrets, but anything else —CI_*values fromci-info, runtime tokens, custom env, paths — is exposed. Replace with targetedecho/declare -pof only the variables you actually need to debug, or use scopedset -x/set +xaround specific commands.🔒 Suggested change
- name: git merge plan run: | cd ../jdk21u && git status eval $(/tmp/$USER/plume-scripts/ci-info typetools) - set + echo "CI_ORGANIZATION=${CI_ORGANIZATION} CI_BRANCH_NAME=${CI_BRANCH_NAME}" echo "About to run: git pull --no-edit https://github.com/${CI_ORGANIZATION}/jdk ${CI_BRANCH_NAME}" - name: git merge run: | ... cd ../jdk21u && git status eval $(/tmp/$USER/plume-scripts/ci-info typetools) - set + echo "CI_ORGANIZATION=${CI_ORGANIZATION} CI_BRANCH_NAME=${CI_BRANCH_NAME}" echo "About to run: git pull --no-edit https://github.com/${CI_ORGANIZATION}/jdk ${CI_BRANCH_NAME}"Also applies to: 118-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml.m4 around lines 105 - 107, The workflow currently uses a bare "set" (invocations adjacent to the eval $(/tmp/$USER/plume-scripts/ci-info typetools) and before the git pull echo) which dumps the entire shell environment into public CI logs; remove the bare "set" calls and replace them with either scoped debugging (wrap only the specific command with set -x / set +x) or explicit, minimal diagnostics such as echo or declare -p for only the needed variables (e.g., echo "CI_BRANCH_NAME=$CI_BRANCH_NAME" or declare -p CI_BRANCH_NAME CI_ORGANIZATION) so you do not expose unrelated env vars or secrets while preserving necessary debug output..github/workflows/defs.m4 (1)
15-15: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPin container images to immutable digests instead of
:latest.All three job macros pull
mdernst/cf-ubuntu-jdkNN:latest.:latestis mutable, so two CI runs of the same commit can produce different toolchain results, and any registry compromise silently propagates to every job. Pin to an immutable@sha256:…digest, or at minimum a stable date/version tag aligned with the Azure pipeline references.Also applies to: 34-34, 52-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/defs.m4 at line 15, The workflow uses mutable image tags "container: mdernst/cf-ubuntu-jdk$3:latest" (and the other occurrences of the same pattern) — replace those with immutable image references by pinning to a digest (e.g., mdernst/cf-ubuntu-jdk$3@sha256:...) or at minimum a stable version/date tag consistent with your Azure pipeline; update all three occurrences (the instances matching "mdernst/cf-ubuntu-jdk$3:latest" in this file) so CI runs are hermetic and reproducible.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml.m4:
- Around line 18-20: The custom shell in the workflow defaults removes Bash
fail-fast flags; update the defaults.run.shell value used in this file (the
"defaults" -> "run" -> "shell" entry) to include the missing flags so it reads
bash --noprofile --norc -eo pipefail {0} to restore -e and pipefail semantics
for all run steps (this will ensure steps like the "show environment" multi-line
blocks, "git merge plan" commands, and pipelines such as the one using git diff
| head -1000 fail fast); after changing the shell, re-run or inspect the "show
environment" and "git merge plan" steps to confirm they no longer silently
ignore upstream errors.
---
Duplicate comments:
In @.github/workflows/ci.yml.m4:
- Around line 136-146: The JDK 17 matrix currently omits the inference test:
update the cftests_job entries to either add the missing invocation
cftests_job(inference, cftests-inference, 17) if omission was accidental, or add
a one-line m4 comment next to the existing cftests_job block (near the lines
invoking cftests_job(..., 17)) explaining that inference
(test-cftests-inference.sh) requires JDK 21+ and is intentionally skipped for
JDK 17; reference the cftests_job symbol and cftests-inference test name so
future maintainers understand the rationale.
- Around line 105-107: The workflow currently uses a bare "set" (invocations
adjacent to the eval $(/tmp/$USER/plume-scripts/ci-info typetools) and before
the git pull echo) which dumps the entire shell environment into public CI logs;
remove the bare "set" calls and replace them with either scoped debugging (wrap
only the specific command with set -x / set +x) or explicit, minimal diagnostics
such as echo or declare -p for only the needed variables (e.g., echo
"CI_BRANCH_NAME=$CI_BRANCH_NAME" or declare -p CI_BRANCH_NAME CI_ORGANIZATION)
so you do not expose unrelated env vars or secrets while preserving necessary
debug output.
In @.github/workflows/defs.m4:
- Line 15: The workflow uses mutable image tags "container:
mdernst/cf-ubuntu-jdk$3:latest" (and the other occurrences of the same pattern)
— replace those with immutable image references by pinning to a digest (e.g.,
mdernst/cf-ubuntu-jdk$3@sha256:...) or at minimum a stable version/date tag
consistent with your Azure pipeline; update all three occurrences (the instances
matching "mdernst/cf-ubuntu-jdk$3:latest" in this file) so CI runs are hermetic
and reproducible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80833766-3a6c-48dd-b181-e09825d2768f
📒 Files selected for processing (4)
.github/workflows/Makefile.github/workflows/ci.yml.github/workflows/ci.yml.m4.github/workflows/defs.m4
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/ci.yml.m4 (1)
35-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin the job containers to immutable digests.
These build jobs still pull mutable
:latestimages. Retagging those images changes CI behavior without any repo diff and undermines the SHA pinning you already added for actions. Use@sha256:digests here, and apply the same change in the macro-generated jobs too.#!/bin/bash # Verify workflow containers are still tag-based instead of digest-pinned. rg -n '^\s*container:\s+\S+:(latest|[A-Za-z0-9._-]+)$' .github/workflows -C1Expected result: the matches should be replaced with
image@sha256:...references.Also applies to: 56-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml.m4 at line 35, The workflow uses a mutable image tag "mdernst/cf-ubuntu-jdk21-plus:latest"; replace that tag with an immutable digest form (e.g., "mdernst/cf-ubuntu-jdk21-plus@sha256:<digest>") and do the same replacement for the equivalent macro-generated job entries so CI pulls a fixed image; update the container: value in the workflow and any macros that generate jobs to use `@sha256`: digests instead of :latest or other mutable tags.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml.m4:
- Around line 124-125: The build_jdk21u step currently runs "cd ../jdk21u &&
make jdk" with no timeout; update that GitHub Actions step (the step named "make
jdk" in the build_jdk21u job) to include a timeout-minutes value (e.g.,
timeout-minutes: 90) so the runner will cancel the step if it exceeds the limit;
locate the step by its run command "cd ../jdk21u && make jdk" or its step name
and add the timeout-minutes field at the same indentation level as run/name.
- Around line 67-80: The workflow currently clones/pulls the helper repos in the
"git-scripts" and "plume-scripts" steps from mutable HEADs; change those steps
to fetch and checkout immutable refs: either clone with a branch/tag via git
clone --branch <TAG_OR_SHA> --depth=1 or clone then git fetch origin && git
checkout <COMMIT_SHA>, and avoid using plain git pull; expose the chosen
tag/commit as a workflow variable or input so the steps reference a fixed
immutable ref (e.g., GIT_SCRIPTS_REF / PLUME_SCRIPTS_REF) before running any
scripts.
---
Duplicate comments:
In @.github/workflows/ci.yml.m4:
- Line 35: The workflow uses a mutable image tag
"mdernst/cf-ubuntu-jdk21-plus:latest"; replace that tag with an immutable digest
form (e.g., "mdernst/cf-ubuntu-jdk21-plus@sha256:<digest>") and do the same
replacement for the equivalent macro-generated job entries so CI pulls a fixed
image; update the container: value in the workflow and any macros that generate
jobs to use `@sha256`: digests instead of :latest or other mutable tags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 214fdaf0-9dcc-477b-afd1-e93cb231a5ea
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/ci.yml.m4
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/ci.yml.m4 (1)
67-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin helper repos to immutable refs before executing scripts.
These steps still pull/clone
git-scriptsandplume-scriptsfrom moving HEAD, so external upstream changes can silently alter CI behavior.Suggested hardening
+env: + GIT_SCRIPTS_REF: <pinned-commit-or-tag> + PLUME_SCRIPTS_REF: <pinned-commit-or-tag> ... - name: git-scripts run: | set -ex if test -d /tmp/$USER/git-scripts ; \ - then git -C /tmp/$USER/git-scripts pull -q > /dev/null 2>&1 ; \ - else mkdir -p /tmp/$USER && git -C /tmp/$USER clone --depth=1 -q https://github.com/plume-lib/git-scripts.git ; \ + then git -C /tmp/$USER/git-scripts fetch -q origin "${GIT_SCRIPTS_REF}" && git -C /tmp/$USER/git-scripts checkout -q "${GIT_SCRIPTS_REF}" ; \ + else mkdir -p /tmp/$USER && git -C /tmp/$USER clone -q https://github.com/plume-lib/git-scripts.git && git -C /tmp/$USER/git-scripts checkout -q "${GIT_SCRIPTS_REF}" ; \ fi - name: plume-scripts run: | set -ex if test -d /tmp/$USER/plume-scripts ; \ - then git -C /tmp/$USER/plume-scripts pull -q > /dev/null 2>&1 ; \ - else mkdir -p /tmp/$USER && git -C /tmp/$USER clone --depth=1 -q https://github.com/plume-lib/plume-scripts.git ; \ + then git -C /tmp/$USER/plume-scripts fetch -q origin "${PLUME_SCRIPTS_REF}" && git -C /tmp/$USER/plume-scripts checkout -q "${PLUME_SCRIPTS_REF}" ; \ + else mkdir -p /tmp/$USER && git -C /tmp/$USER clone -q https://github.com/plume-lib/plume-scripts.git && git -C /tmp/$USER/plume-scripts checkout -q "${PLUME_SCRIPTS_REF}" ; \ fiAlso applies to: 74-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml.m4 around lines 67 - 73, The workflow step named "git-scripts" (which clones/pulls https://github.com/plume-lib/git-scripts into /tmp/$USER/git-scripts) currently uses moving HEAD; change it to checkout a pinned immutable ref (e.g., a SHA or tag) by introducing and respecting a variable like GIT_SCRIPTS_REF and using git fetch && git checkout $GIT_SCRIPTS_REF (or clone --branch <ref> or similar) so the action always runs an exact ref; apply the same change to the corresponding "plume-scripts" step (the clone/pull block around lines 74-80) and make the step fail if the ref variable is unset or the checkout fails to ensure CI determinism.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml.m4:
- Line 45: Remove the unconditional dump of the full process environment ("env |
sort") from the CI job and replace it with either nothing or explicit echo
statements that print only the specific variables needed for debugging (e.g.,
echo "VAR_NAME=${VAR_NAME}"). Locate occurrences of "env | sort" and change them
to targeted prints of required variables or remove them entirely to avoid
leaking secrets.
- Line 121: The git pull invocation in the ci workflow uses unquoted dynamic
variables and can suffer shell word-splitting; update the git pull command
string so the remote URL and branch argument are each quoted (i.e., quote the
"https://github.com/${CI_ORGANIZATION}/jdk" segment and the "${CI_BRANCH_NAME}"
segment in the git pull invocation) to ensure proper handling of spaces/special
characters and avoid parsing edge cases.
---
Duplicate comments:
In @.github/workflows/ci.yml.m4:
- Around line 67-73: The workflow step named "git-scripts" (which clones/pulls
https://github.com/plume-lib/git-scripts into /tmp/$USER/git-scripts) currently
uses moving HEAD; change it to checkout a pinned immutable ref (e.g., a SHA or
tag) by introducing and respecting a variable like GIT_SCRIPTS_REF and using git
fetch && git checkout $GIT_SCRIPTS_REF (or clone --branch <ref> or similar) so
the action always runs an exact ref; apply the same change to the corresponding
"plume-scripts" step (the clone/pull block around lines 74-80) and make the step
fail if the ref variable is unset or the checkout fails to ensure CI
determinism.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e946297-6963-443f-9180-6cf8c066f081
📒 Files selected for processing (1)
.github/workflows/ci.yml.m4
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.azure/azure-pipelines.yml.m4:
- Line 129: The pipeline generator rule in the .azure/Makefile currently lists
azure-pipelines.yml.m4 and defs.m4 as prerequisites but omits .azure/jobs.m4
even though .azure/azure-pipelines.yml.m4 includes jobs.m4; update the generator
rule that builds azure-pipelines.yml to also depend on .azure/jobs.m4 (add
".azure/jobs.m4" to the prerequisite list for the target that references
azure-pipelines.yml.m4/defs.m4) so edits to jobs.m4 will trigger regeneration
and keep the committed YAML in sync.
In @.github/workflows/Makefile:
- Around line 11-15: The Make rule "%: %.m4 defs.m4" rebuilds targets from only
the per-file m4 and defs.m4, so add the shared jobs m4 as a prerequisite so
changes to ../../.azure/jobs.m4 trigger regeneration; update the rule that
defines the pattern target (the "%: %.m4 defs.m4" rule) to also depend on
../../.azure/jobs.m4 (or the appropriate jobs.m4 variable) so ci.yml gets
rebuilt when the shared jobs file is edited.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e007727-b507-4608-95fd-9831c88ecbd1
📒 Files selected for processing (6)
.azure/Makefile.azure/azure-pipelines.yml.m4.azure/jobs.m4.github/workflows/Makefile.github/workflows/ci.yml.github/workflows/ci.yml.m4
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml.m4:
- Around line 115-122: The git merge diagnostics use pipelines like "git show |
head -100" and "git diff | head -1000" which can fail on a closed pipe and abort
the remaining diagnostic commands; update those two pipelines to use a robust
line-limiting tool (e.g., sed -n to print only the first N lines) and append a
fallback to always return success (|| true) so that failures in the pager/pipe
do not stop the subsequent diagnostics (leave the surrounding git pull || (git
--version && git show ... && git status && git diff ... && echo ... && false)
structure intact).
In @.github/workflows/Makefile:
- Around line 1-4: The Makefile currently uses $(shell git clone ...) assigned
to the dummy variable (PLUME_SCRIPTS ?= ...) which executes during parsing;
instead remove the $(shell ...) invocation and add a dedicated recipe (e.g., a
target named init-plume-scripts or prerequisites for targets that need
PLUME_SCRIPTS) that checks for PLUME_SCRIPTS existence and runs git clone only
when the target is invoked, and ensure the recipe propagates git's exit status
(using normal shell commands in the recipe) so clone failures fail fast; update
references that relied on the dummy variable to depend on the new target (or
make it a prerequisite for targets like build/test) so no network operation
happens during Makefile parse.
In @.gitignore:
- Line 26: The .gitignore entry ".plume-scripts" should use a trailing slash to
explicitly match the directory (consistent with other entries like "/build/" and
"/dist/"); update the entry for ".plume-scripts" to ".plume-scripts/" so it only
ignores the directory and matches the intent reflected by the Makefile changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ff5932c-32d1-49b6-bc6d-ccd04de8f35c
📒 Files selected for processing (10)
.azure/Makefile.azure/azure-pipelines.yml.azure/azure-pipelines.yml.m4.azure/defs.m4.azure/jobs.m4.github/workflows/Makefile.github/workflows/ci.yml.github/workflows/ci.yml.m4.github/workflows/defs.m4.gitignore
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
.github/workflows/ci.yml.m4 (1)
75-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHelper repos
git-scripts/plume-scriptsare still tracked from mutable HEAD.This was raised previously and not addressed in the current diff:
git pull -qandclone --depth=1(no ref) both follow the upstream default branch, so changes to those repos alter CI behavior here without any commit ontypetools/jdk. Pin to a tag or commit SHA (e.g., via aGIT_SCRIPTS_REF/PLUME_SCRIPTS_REFvariable indefs.m4) andgit checkoutthat ref after clone/fetch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml.m4 around lines 75 - 88, The workflow currently clones and pulls helper repos from mutable HEAD; create and use pinned refs (e.g., add GIT_SCRIPTS_REF and PLUME_SCRIPTS_REF variables in defs.m4) and modify the "clone git-scripts" and "clone plume-scripts" steps to (1) clone with --depth=1 if desired, (2) after clone or fetch run git fetch --no-tags origin <ref> && git checkout --detach <ref> (or git checkout <ref> if it's a tag), and replace any git -C ... pull usage with a fetch+checkout to ensure the CI checks out the pinned tag/SHA from the corresponding GIT_SCRIPTS_REF / PLUME_SCRIPTS_REF variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml.m4:
- Around line 134-142: The canary_jobs job currently is intentionally a no-op
and relies on needs: to gate downstream jobs; add a brief inline comment inside
the canary_jobs step explaining that canary_jobs depends on build_jdk and
build_jdk21u via needs and must not be changed to use if: always() because that
would bypass the intended failure gating for downstream jobs; reference the job
name canary_jobs and the needs entries build_jdk and build_jdk21u in the comment
so future contributors understand why the no-op run: true must remain and why
if: always() must not be added.
- Line 122: The line using eval $(/tmp/$USER/plume-scripts/ci-info typetools)
dangerously executes arbitrary stdout; replace it so ci-info returns only
specific variables (e.g. call ci-info to output CI_ORGANIZATION and
CI_BRANCH_NAME explicitly or have ci-info write a temp file that you source)
instead of eval-ing its entire output, and after obtaining CI_ORGANIZATION and
CI_BRANCH_NAME validate them against strict allowlist regexes before use; locate
the eval invocation and update the logic around the ci-info invocation and the
CI_ORGANIZATION / CI_BRANCH_NAME consumption to follow this safer pattern.
- Line 35: The workflow currently pins container:
mdernst/cf-ubuntu-jdk21-plus:latest which is mutable; change this to an
immutable identifier (prefer digest, fallback to a specific versioned tag) and
centralize that value in defs.m4 so all jobs reuse it; update the references
used by build_jdk, build_jdk21u and the cftests/daikon/plume-lib matrix to
reference the new defs.m4 variable (e.g., CF_UBUNTU_JDK_IMAGE) rather than
:latest so CI runs use a reproducible image.
- Around line 99-116: The trailing `echo $?` after `set -ex` is dead/incorrect
because the shell will already exit on a non-zero `git diff --exit-code`; update
the `clone-related-jdk21u` step by either removing the `echo $?` line entirely,
or replace it with a proper explicit check: run `git diff --exit-code` and if it
fails capture its exit code, print a diagnostic message including that code, and
then exit with the same code (i.e., perform a conditional check that logs the
failure and returns the non-zero status instead of relying on `echo $?`).
- Around line 117-124: The git merge run block contains a redundant second "cd
../jdk21u" (in the run script that executes the git pull) which is unnecessary
because the shell is already in ../jdk21u from the earlier cd; remove the second
"cd ../jdk21u" so the sequence is: set -ex; cd ../jdk21u; git status; eval ...;
echo "About to run: git pull ..."; git pull --no-edit ... || (git --version &&
... && false). Update the git merge run block to keep only a single cd ../jdk21u
(the first one) to avoid masking future path bugs.
---
Duplicate comments:
In @.github/workflows/ci.yml.m4:
- Around line 75-88: The workflow currently clones and pulls helper repos from
mutable HEAD; create and use pinned refs (e.g., add GIT_SCRIPTS_REF and
PLUME_SCRIPTS_REF variables in defs.m4) and modify the "clone git-scripts" and
"clone plume-scripts" steps to (1) clone with --depth=1 if desired, (2) after
clone or fetch run git fetch --no-tags origin <ref> && git checkout --detach
<ref> (or git checkout <ref> if it's a tag), and replace any git -C ... pull
usage with a fetch+checkout to ensure the CI checks out the pinned tag/SHA from
the corresponding GIT_SCRIPTS_REF / PLUME_SCRIPTS_REF variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16690a34-70e3-42ff-a70c-5ac8f1ac9681
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/ci.yml.m4
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 463-478: The test_plume_lib job lacks a timeout and can hang;
update the source macro that renders this job (the macro that emits the job with
id test_plume_lib which runs test-plume-lib.sh in the checker-framework
directory) to include a timeout-minutes setting (e.g., timeout-minutes: 60) so
the generated YAML contains timeout-minutes for the test_plume_lib job instead
of editing the generated .github/workflows/ci.yml directly.
In @.github/workflows/ci.yml.m4:
- Around line 117-123: The fallback diagnostics in the git pull failure branch
are being aborted by the workflow-wide -o pipefail; modify the fallback block
that follows the git pull (the subshell starting with git --version && git show
| head -100 ...) to temporarily disable pipefail, run the piped commands with
"|| true" so head truncation won't cause failure, and then restore pipefail;
specifically, wrap the diagnostics in a subshell or inline sequence that runs
"set +o pipefail" before the piped git show | head and git diff | head pipelines
and "set -o pipefail" (or leave as-is) afterwards while appending "|| true" to
the piped commands so all diagnostics (git --version, git show, git status, git
diff, echo ...) always run even if head closes the pipe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d339718-14b9-41f5-bddd-1cf526c0c949
📒 Files selected for processing (4)
.azure/azure-pipelines.yml.azure/azure-pipelines.yml.m4.github/workflows/ci.yml.github/workflows/ci.yml.m4
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml.m4:
- Around line 28-29: The current "Check generated ci.yml" step only regenerates
and diffs .github/workflows/ci.yml, but the pipeline templates also pull shared
job definitions from ../../.azure/jobs.m4 which can affect
.azure/azure-pipelines.yml; update the step (the job named "Check generated
ci.yml") to regenerate both artifacts and validate them by running the make
target(s) that build .github/workflows/ci.yml and .azure/azure-pipelines.yml and
then run git diff --exit-code on both .github/workflows/ci.yml and
.azure/azure-pipelines.yml so changes in .azure/jobs.m4 are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21b55c53-bc07-44af-b415-2bc50fbcdd33
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/ci.yml.m4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge with typetools/jdk21u#7