test(common): gate the release job's completions pre-merge - #1048
Closed
norrietaylor wants to merge 1 commit into
Closed
test(common): gate the release job's completions pre-merge#1048norrietaylor wants to merge 1 commit into
norrietaylor wants to merge 1 commit into
Conversation
release.yml's "Generate completions" step runs the freshly built mip / min / minimald binaries, but only on a dispatched release — so a breaking CLI change clears every PR gate and only surfaces an hour into a release, taking the GCS upload, the GitHub Release and stage-installer down with it. That is how the `completions <shell>` → `completions print <shell>` split (#1009) shipped broken for twenty days (#1034). Move the generation into scripts/gen-completions.sh, one definition of what the release ships, and call it from crates/common/tests/release_completions.rs against target/debug. The release path and the pre-merge gate are then the same code rather than one replaying the other, so they cannot drift, and the always-running Linux lanes execute it through the workspace suite — the reviewed-code extension point CI schedules over, since .github/workflows/ is frozen. release_job_calls_the_generator holds the other half: the workflow has to keep calling the helper instead of inlining the commands again. The helper derives each destination filename from the command name rather than spelling it out per row, because a shell only autoloads completions from a file named for the command they complete. The #737 binary rename left the release job writing `min`'s completions to files named `minimal` — inert, and invisible to any exit-code check — until #1034. Deriving them makes that mismatch unrepresentable; the test still asserts each generated file registers the command it is named for, which an exit-code-only gate would not have caught. This commit does not carry the release.yml edit that points the step at the helper: .github/workflows/ is CODEOWNER-gated and frozen to agents, so it is applied by hand (the diff is in the PR description). Until it lands, release_job_calls_the_generator fails by design. Verified: reverting the helper's `min` verb to the pre-#1009 flat form fails the test with `unrecognized subcommand 'bash'`; pointing `min`'s row at a different binary fails the registration check; the release job's exact call form (MIP_BIN/MIN_BIN/MINIMALD_BIN at the platform-suffixed artifacts) reproduces today's output tree. minimald is left out on macOS, where it does not build, and covered by the Linux lanes. Refs: #1035 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughRelease completion generation is centralized in a shell script covering bash, zsh, and fish. Integration tests build binaries, validate generated registrations, and verify release workflow wiring. A ChangesRelease completion validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
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 `@crates/common/tests/release_completions.rs`:
- Around line 107-108: Update the test output path setup around the
release-completions test to place the materialized output under the repository
tree instead of using std::env::temp_dir(). Preserve the process-specific
directory naming and existing cleanup via fs::remove_dir_all.
- Around line 153-163: The release workflow assertions in the relevant test must
inspect only the `Generate completions` step’s `run` block, verifying that it
invokes `scripts/gen-completions.sh` and does not inline binary invocations or
redirect to `artifacts/completions/`. Update the checks around these `assert!`
calls to parse or isolate that step before matching, so unrelated workflow
references cannot satisfy them.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6653803-8510-46e0-a5b0-e3b3e5e5730b
📒 Files selected for processing (3)
crates/common/tests/release_completions.rsjustfilescripts/gen-completions.sh
Comment on lines
+107
to
+108
| let out = std::env::temp_dir().join(format!("release-completions-{}", std::process::id())); | ||
| let _ = fs::remove_dir_all(&out); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Materialize test output inside the repository tree on macOS.
temp_dir() is normally outside the checkout, contrary to the VM synchronization requirement.
Proposed fix
- let out = std::env::temp_dir().join(format!("release-completions-{}", std::process::id()));
+ let out = root
+ .join("target")
+ .join(format!("release-completions-{}", std::process::id()));As per coding guidelines, “On macOS, keep --output paths for materialization under the repository tree because the VM-backed CLI only synchronizes the project directory.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let out = std::env::temp_dir().join(format!("release-completions-{}", std::process::id())); | |
| let _ = fs::remove_dir_all(&out); | |
| let out = root | |
| .join("target") | |
| .join(format!("release-completions-{}", std::process::id())); | |
| let _ = fs::remove_dir_all(&out); |
🤖 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 `@crates/common/tests/release_completions.rs` around lines 107 - 108, Update
the test output path setup around the release-completions test to place the
materialized output under the repository tree instead of using
std::env::temp_dir(). Preserve the process-specific directory naming and
existing cleanup via fs::remove_dir_all.
Source: Coding guidelines
Comment on lines
+153
to
+163
| assert!( | ||
| body.contains("scripts/gen-completions.sh"), | ||
| "release.yml does not call scripts/gen-completions.sh, so its completion \ | ||
| invocations are unexercised until a release is dispatched — the gap #1035 \ | ||
| is about. Point the \"Generate completions\" step at the helper.", | ||
| ); | ||
| assert!( | ||
| !body.contains("> artifacts/completions/"), | ||
| "release.yml inlines completion invocations again; they belong in \ | ||
| scripts/gen-completions.sh, which this test exercises pre-merge.", | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 12 \
'Generate completions|gen-completions\.sh|MIP_BIN|MIN_BIN|MINIMALD_BIN|artifacts/completions' \
.github/workflows/release.ymlRepository: gominimal/minimal
Length of output: 7483
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n## crates/common/tests/release_completions.rs\n'
sed -n '120,180p' crates/common/tests/release_completions.rs
printf '\n## .github/workflows/release.yml occurrences\n'
rg -n -C 4 'gen-completions\.sh|Generate completions|MIP_BIN|MIN_BIN|MINIMALD_BIN|artifacts/completions' .github/workflows/release.ymlRepository: gominimal/minimal
Length of output: 5921
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '1,220p' crates/common/tests/release_completions.rsRepository: gominimal/minimal
Length of output: 6897
Scope the workflow check to the Generate completions step. A stray scripts/gen-completions.sh reference or artifacts/completions/ redirect elsewhere in release.yml can satisfy these string checks without proving the step still runs the helper; match the step’s run block and its binary invocations instead.
🤖 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 `@crates/common/tests/release_completions.rs` around lines 153 - 163, The
release workflow assertions in the relevant test must inspect only the `Generate
completions` step’s `run` block, verifying that it invokes
`scripts/gen-completions.sh` and does not inline binary invocations or redirect
to `artifacts/completions/`. Update the checks around these `assert!` calls to
parse or isolate that step before matching, so unrelated workflow references
cannot satisfy them.
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
1 participant
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.
Closes #1035.
The gap
release.yml's Generate completions step shells out to the freshly builtmip/min/minimaldbinaries. Those invocations only ever run on aworkflow_dispatchrelease, so a breaking CLI change clears every PR gate, merges, and sits until someone cuts a release. #1009 (completions <shell>→completions print <shell>) did exactly that: twenty days later it exited 2, an hour of build time in, taking the GCS upload, the GitHub Release, and the wholestage-installerjob with it. #1034 fixed the call site; the structure that let it happen stayed.The fix
Move the generation into
scripts/gen-completions.sh— one definition of what the release ships — and have both the release job and a workspace test call it. The pre-merge gate is then the same code as the release path, not a replay of it, so the two cannot drift.crates/common/tests/release_completions.rsruns the helper againsttarget/debugand asserts every generated file registers the command it is autoloaded for. It is convention-discovered, so the always-running Linux lanes execute it through the core-tests suite with no CI edit — the extension point in docs/ci-strategy.md §10, same asscripts/lint-shell.sh(#899).just test-completionsruns it directly.Filenames are derived from the command name inside the helper rather than spelled out per row. A shell only autoloads completions from a file named for the command they complete, and the #737 binary rename left the release job writing
min's completions to files namedminimal— inert, exit 0, invisible until #1034. Deriving makes that mismatch unrepresentable; the test still checks the registration line, because an exit-code-only gate would not have caught it..github/workflows/is frozen to agents, so the commit stops at the repo side.release_job_calls_the_generatorfails by design until the step calls the helper — that failure is the gap #1035 describes, still open. Apply on this branch:Or:
gh pr checkout <this PR> && git apply <patch> && git commit -am 'ci(release): generate completions through scripts/gen-completions.sh'.The step keeps the artifact names because only the workflow knows them (platform suffixes, and that
minships as theminimal-*artifact); everything that can break — verbs, filenames, the shell list — moves into reviewed code. Output tree is byte-for-byte the layout the release produces today, socompletions.tar.gzandstage-release.share untouched.Verification
min/mipchange only surfaces on a release run #1035. Reverting the helper'sminrow to the pre-feat(min)!: split completions into print and install verbs #1009 flat verb fails the test witherror: unrecognized subcommand 'bash'.min's row at a different binary fails withbash/min does not register the command `min` it is autoloaded for.MIP_BIN=… MIN_BIN=… scripts/gen-completions.sh artifacts/completionsagainst platform-suffixed copies reproduces today's six-file tree (bash/min,zsh/_min,fish/min.fish, …); a missing binary exits 1 rather than writing a partial tree.cargo fmt --all --check,cargo clippy -p common --all-targets -- -D warnings,shellcheck,just lint-shell(25/25).minimaldis skipped on macOS (Linux-only crate) and covered by the Linux lanes here.Audit
Per the issue's ask: the completions step is the only place the
releaseandstage-installerjobs invoke a shipped binary. The rest isgcloud,gh,tar, or ascripts/helper that already has a harness (install.sh→install_test.sh,verify-nightly-provenance.sh→verify-nightly-provenance_test.sh).Complements #687's PR9b (post-release artifact smoke) — this is the pre-merge half.
🤖 Generated with Claude Code
Note
Gate the release job's completion generation with pre-merge integration tests
mip,min, andminimald, with strict error checking and support forBIN_DIR/per-binary path overrides.gen-completions.shagainst debug binaries and asserts each output file registers its command; one that asserts the release workflow callsgen-completions.shrather than redirecting completions inline.just test-completionsrecipe to run the new test file in isolation.cargo buildat test time, which can significantly increase CI time on cold caches.Macroscope summarized 6497f58.
Summary by CodeRabbit
New Features
Bug Fixes
Tests