Fix running hydraJobs in CI#494
Merged
Merged
Conversation
The `nix flake show --json` output format changed, so this jq query was returning an empty list.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdjusts GitHub Actions VM test gating and discovery, adds a SHA256 fixed-output fast-path to fetchTree to ensure pinned store paths, and removes/fixes several NixOS test definitions and exports. ChangesWorkflow, fetch, and tests edits
Sequence DiagramsequenceDiagram
participant Actions as GitHub Actions
participant Flake as "nix flake show --json"
participant JQ as jq
participant NixBuild as "nix build"
Actions->>Flake: run `nix flake show --json`
Flake->>JQ: JSON at .inventory.hydraJobs.output.children.tests.children
JQ->>Actions: `$tests` (filtered with has("derivation"))
Actions->>NixBuild: `nix build $tests --timeout 3600`
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 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/build.yml:
- Line 125: vm_tests_all is currently gated only by inputs.run_vm_tests which
causes it to run alongside vm_tests_smoke; change the job condition so
vm_tests_all only runs for the full-suite case (e.g. inputs.run_vm_tests ==
'all') or on merge_group events, making it mutually exclusive with
vm_tests_smoke (which handles the non-merge_group/smoke path). Update the
vm_tests_all job's if expression to explicitly check the input value or
github.event_name rather than a plain inputs.run_vm_tests flag so both jobs
cannot be scheduled in the same run.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47f893dd-4822-4e4d-b7fa-966ae680cab5
📒 Files selected for processing (1)
.github/workflows/build.yml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build.yml (1)
133-138:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the configured flake input when discovering VM tests.
At Line 133,
nix flake show --jsonignoresinputs.flake, but Line 138 builds targets under${{ inputs.flake }}#.... If callers pass a non-root flake, discovery and build can diverge and produce empty/incorrect target sets.Suggested fix
- tests=$(nix flake show --json \ + tests=$(nix flake show --json ${{ inputs.flake }} \ | jq -r ' .inventory.hydraJobs.output.children.tests.children | with_entries(select(.value | has("derivation"))) | keys[] | "${{ inputs.flake }}`#hydraJobs.tests`." + .')🤖 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/build.yml around lines 133 - 138, The nix flake discovery step uses `nix flake show --json` which inspects the root flake instead of the configured `inputs.flake`, causing a mismatch with the later build targets that use `${{ inputs.flake }}`; change the command that computes `tests` to call `nix flake show` against the configured flake (i.e., `nix flake show "${{ inputs.flake }}" --json`) so the JSON inspection and the resulting keys align with the targets you later build, keeping the same jq pipeline that produces the `${{ inputs.flake }}`#hydraJobs.tests`.` keys.
🤖 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.
Outside diff comments:
In @.github/workflows/build.yml:
- Around line 133-138: The nix flake discovery step uses `nix flake show --json`
which inspects the root flake instead of the configured `inputs.flake`, causing
a mismatch with the later build targets that use `${{ inputs.flake }}`; change
the command that computes `tests` to call `nix flake show` against the
configured flake (i.e., `nix flake show "${{ inputs.flake }}" --json`) so the
JSON inspection and the resulting keys align with the targets you later build,
keeping the same jq pipeline that produces the `${{ inputs.flake
}}`#hydraJobs.tests`.` keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7edfad1-ea5c-4191-811e-3d51b1ffcb7d
📒 Files selected for processing (2)
.github/workflows/build.ymltests/nixos/user-sandboxing/default.nix
💤 Files with no reviewable changes (1)
- tests/nixos/user-sandboxing/default.nix
This reverts commit 3ec0993. This actually broke fetchersSubstitute test, since we want to be able to substitute builtins.fetchurl too, which is only handled by the tarball fetcher. See: https://hydra.nixos.org/build/331423127
Determinate Nix remove `nix upgrade-nix`.
edolstra
force-pushed
the
run-hydra-tests
branch
from
ccb6e84 to
823d910
Compare
edolstra
force-pushed
the
run-hydra-tests
branch
from
823d910 to
fa8e406
Compare
cole-h
approved these changes
Jun 10, 2026
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/build.yml:
- Line 138: The workflow embeds `${{ inputs.flake }}` into a jq expression and
later expands `$tests` unquoted, allowing shell metacharacters in inputs to be
interpreted; change the pipeline to have jq emit a JSON array (rather than
raw/unquoted strings) and then read that JSON safely into a bash array (e.g.,
via mapfile/readarray + jq -r '.[]' or using jq -c to produce an array and jq -r
'.[]' into mapfile) and always expand the results as `"${tests[@]}"`; in short,
stop inserting `${{ inputs.flake }}` raw into shell text, emit/consume JSON-safe
values from jq, populate `tests` with mapfile/readarray, and use quoted
expansion (`"${tests[@]}"`) wherever `$tests` is used.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 465dd3d1-087f-49bf-9897-638c2681fe9f
📒 Files selected for processing (6)
.github/workflows/build.ymlsrc/libexpr/primops/fetchTree.cctests/nixos/default.nixtests/nixos/s3-binary-cache-store.nixtests/nixos/upgrade-nix.nixtests/nixos/user-sandboxing/default.nix
💤 Files with no reviewable changes (4)
- tests/nixos/upgrade-nix.nix
- tests/nixos/default.nix
- tests/nixos/user-sandboxing/default.nix
- tests/nixos/s3-binary-cache-store.nix
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libexpr/primops/fetchTree.cc
| .inventory.hydraJobs.output.children.tests.children | ||
| | with_entries(select(.value | has("derivation"))) | ||
| | keys[] | ||
| | "${{ inputs.flake }}#hydraJobs.tests." + .') |
There was a problem hiding this comment.
Harden against code injection via template expansion and unquoted variable usage.
Line 138 embeds ${{ inputs.flake }} into the jq output, and line 146 uses $tests unquoted. If inputs.flake contains shell metacharacters (e.g., ; or |), they will be interpreted by the shell at line 146, enabling command injection.
While the threat is mitigated because this is a workflow_call (only trusted repo workflows can supply inputs), defense-in-depth is warranted.
🔒 Proposed fix using a bash array
Replace lines 133–146 with:
- - run: |
- tests=$(nix flake show --json \
- | jq -r '
- .inventory.hydraJobs.output.children.tests.children
- | with_entries(select(.value | has("derivation")))
- | keys[]
- | "${{ inputs.flake }}`#hydraJobs.tests`." + .')
-
- if [ -z "$tests" ]; then
- echo "error: no tests found in hydraJobs.tests"
- exit 1
- fi
-
- cmd() {
- nix build -L --keep-going --timeout 600 $tests
- }
+ - run: |
+ mapfile -t tests < <(
+ nix flake show --json \
+ | jq -r '
+ .inventory.hydraJobs.output.children.tests.children
+ | with_entries(select(.value | has("derivation")))
+ | keys[]
+ | "${{ inputs.flake }}`#hydraJobs.tests`." + .'
+ )
+
+ if [ ${`#tests`[@]} -eq 0 ]; then
+ echo "error: no tests found in hydraJobs.tests"
+ exit 1
+ fi
+
+ cmd() {
+ nix build -L --keep-going --timeout 600 "${tests[@]}"
+ }This uses mapfile to safely populate an array and "${tests[@]}" to expand elements without shell interpretation of metacharacters.
Also applies to: 146-146
🧰 Tools
🪛 zizmor (1.25.2)
[error] 138-138: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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/build.yml at line 138, The workflow embeds `${{
inputs.flake }}` into a jq expression and later expands `$tests` unquoted,
allowing shell metacharacters in inputs to be interpreted; change the pipeline
to have jq emit a JSON array (rather than raw/unquoted strings) and then read
that JSON safely into a bash array (e.g., via mapfile/readarray + jq -r '.[]' or
using jq -c to produce an array and jq -r '.[]' into mapfile) and always expand
the results as `"${tests[@]}"`; in short, stop inserting `${{ inputs.flake }}`
raw into shell text, emit/consume JSON-safe values from jq, populate `tests`
with mapfile/readarray, and use quoted expansion (`"${tests[@]}"`) wherever
`$tests` is used.
Source: Linters/SAST tools
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
3 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.
Motivation
The
nix flake show --jsonoutput format changed, so the jq query was returning an empty list. This was hiding a couple of broken VM tests.Context
Summary by CodeRabbit