fix(spec): catch nixpkgs-reference pin drift, and repair the drift #55 introduced - #56
Merged
Merged
Conversation
REFERENCE.md declared nixpkgs_rev = aec71e3 while flake.nix pinned 88ae382. The two diverged in #55, which merged the Renovate digest bump for nixpkgs-reference without the accompanying REFERENCE.md update that this file's own upgrade policy requires ("Edit the [reference] block in this file _and_ the nixpkgs-reference.url rev in flake.nix in the same commit"). Nothing caught it. `cargo xtask spec versions` only compares the pinned bash/coreutils *versions* against the devshell, and on_disk_reference_doc_parses only compares REFERENCE.md against a hardcoded constant — so the doc and the test agreed with each other while both disagreed with the flake. The following commit closes that hole. No fixtures are affected: aec71e3, 88ae382, and ad903c1 all resolve to bash 5.3p15 and coreutils 9.11, so the oracle never actually changed. `cargo xtask compat` reports regression: 0 against the corpus, confirming the recorded sidecars still match the pinned reference bash. pinned_on is set to the date 88ae382 was adopted on main (#55), not the date the rev was authored upstream.
`cargo xtask spec versions` compared the pinned bash/coreutils versions against the devshell but never checked the rev, and on_disk_reference_doc_parses compared REFERENCE.md against a hardcoded constant rather than against the flake. The doc and the test therefore agreed with each other while both disagreed with flake.nix, which is how the pin silently drifted in #55. Two different nixpkgs revs can ship identical bash and coreutils — aec71e3, 88ae382 and ad903c1 all give bash 5.3p15 / coreutils 9.11 — so a version-only comparison reports "ok" on a stale pin by construction. The rev is the only field that distinguishes them. Adds `parse_flake_input_rev`, which reads `<input>.url` out of flake.nix and requires a full 40-character object name. A branch ref or a short digest yields `NotPinned` rather than being accepted, since an unpinned reference input is precisely the failure the pin exists to prevent. The rev is now compared in `run_versions` (checked first, as the root cause when versions coincide) and in a new test that reads both files from disk. Verified the guard actually fires: re-pointing REFERENCE.md at the old rev fails the test and exits 1 from `spec versions` with a message naming both revs and the policy. Because the new test lives in `cargo test --all`, it is already gated by the existing `xtask check` matrix and the pre-commit hook — this class of drift can no longer reach main. `run_versions` crossed clippy::too_many_lines (103/100) with the added check, so the flake read and the comparison are extracted into `read_flake_rev` and `verify_pin` rather than suppressing the lint. `verify_pin` being a free function also makes the comparison directly unit-testable, including the #55 case where the rev is stale but both versions still agree.
Adds a fredshell spec versions ====================== Pinned in tests/spec/REFERENCE.md: bash : 5.3p15 coreutils : 9.11 rev : 88ae3822eb8aec31f12a4a1895cb064413511177 pinned on : 2026-09-18 Resolved from `nixpkgs-reference` (nix devshell): bash : 5.3p15 coreutils : 9.11 rev : 88ae3822eb8aec31f12a4a1895cb064413511177 pin matches flake.nix and devshell: ok Floating nixos-unstable (advisory): bash : 5.3p15 coreutils : 9.11 no drift step to the Spec compat report job, so a REFERENCE.md / flake.nix / devshell disagreement fails CI rather than being reported as "ok". The rev comparison is already covered by a unit test in cargo test --all, but the version half of the check reads the FREDSHELL_REFERENCE_* vars that only the nix devshell exports, so it cannot run in the bare-runner check matrix. The compat job already runs inside nix develop, which makes it the only place this can live. Ordered before the compat report deliberately: if the resolved oracle is not the one the fixtures were recorded against, the pass rate is measuring the wrong bash and is worse than no number at all.
nixpkgs-reference is the pinned oracle for the bash-compat spec corpus. Its upgrade policy in tests/spec/REFERENCE.md requires the [reference] block and the flake.nix rev to move in the same commit, re-recording any affected fixtures. Renovate can do neither, so its PRs for this input are unmergeable as written — #45 sat open for two and a half weeks proving the point, and #55 merged an earlier incarnation of it without the REFERENCE.md half, which is how the pin drifted in the first place. Note this input was never auto-merged: the existing rule matches major/minor/patch/pin, and these arrive as digest updates, so they fell through to the top-level automerge: false. The problem is noise and the invitation to merge without the policy work, not silent automerging. Drift versus nixos-unstable is still surfaced advisorily by cargo xtask spec versions, so disabling the bot loses no signal. Bump the pin by hand, deliberately.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: 9 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour. 📝 WalkthroughWalkthroughThe specification tooling now parses the pinned Nix revision from ChangesToolchain Pin Validation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant CI compat job
participant run_versions
participant flake.nix
participant verify_pin
CI compat job->>run_versions: run cargo xtask spec versions
run_versions->>flake.nix: read nixpkgs-reference revision
flake.nix-->>run_versions: return pinned revision
run_versions->>verify_pin: compare revision and tool versions
verify_pin-->>CI compat job: return validation result
Merge Risk: ⚪ Minimal · up to The reference revision, validation tooling, and CI enforcement align without an identified merge-blocking issue. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🟡 Changes recommended
Address the updater-workflow exclusion and tighten .url attribute matching.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR prevents drift between the documented spec-oracle revision and the flake.nix pin, adds CI verification, and disables unsupported Renovate updates.
Changes:
- Added flake revision parsing and consistency checks.
- Corrected
REFERENCE.md. - Added CI validation and Renovate exclusion.
File summaries
| File | Summary |
|---|---|
xtask/src/spec/mod.rs |
Adds revision parsing and pin verification. Moderate issue remains in attribute matching. |
tests/spec/REFERENCE.md |
Records the corrected oracle revision and date. |
renovate.json |
Disables Renovate updates for the oracle input; a critical updater-workflow gap remains. |
.github/workflows/ci.yml |
Runs pin verification before compatibility reporting. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+31
to
+34
| "description": "The nixpkgs-reference input is the pinned oracle for the bash-compat spec corpus (PLAN_05 4.5). Its upgrade policy in tests/spec/REFERENCE.md requires the [reference] block and the flake.nix rev to change in the same commit, re-recording any affected fixtures. Renovate cannot do either, so a bot PR here is unmergeable as-is and only adds noise; #45 sat open for weeks for exactly this reason. Version drift versus nixos-unstable is already surfaced advisorily by `cargo xtask spec versions`, so nothing is lost by disabling it. Bump this pin by hand.", | ||
| "matchManagers": ["nix"], | ||
| "matchDepNames": ["nixpkgs-reference"], | ||
| "enabled": false |
Comment on lines
+162
to
+164
| if !trimmed.starts_with(&needle) { | ||
| continue; | ||
| } |
1 task
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
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.
Follow-up to #55. Closes the hole that let the spec-corpus oracle pin drift
silently, and repairs the drift #55 introduced.
Supersedes #45.
The bug
tests/spec/REFERENCE.mddeclarednixpkgs_rev = "aec71e3…"whileflake.nixpinned
88ae382…. They diverged in #55, which merged the Renovate digest bumpfor
nixpkgs-referencewithout theREFERENCE.mdupdate that the file's ownupgrade policy demands:
That was my miss in #55 — the roundup treated #45 as a routine digest bump when
the input it touches is explicitly policy-gated.
Why nothing caught it
Two checks existed and both missed it by construction:
cargo xtask spec versionscompared the pinned bash/coreutils versionsagainst the devshell, never the rev.
on_disk_reference_doc_parsescomparedREFERENCE.mdagainst a hardcodedconstant, never against
flake.nix.So the doc and the test agreed with each other while both disagreed with the
flake, and
spec versionsprintedpin matches devshell: ok.The version-only check cannot detect this even in principle:
aec71e3,88ae382andad903c1all resolve to bash 5.3p15 / coreutils 9.11. Differentrevs, identical toolchain. The rev is the only field that distinguishes them.
Changes
1.
fix(spec):record the revflake.nixactually pins —REFERENCE.mdaec71e3→88ae382,pinned_on→ the date it was adopted on main.2.
fix(spec):verify the rev, not just the versions — addsparse_flake_input_rev, which reads<input>.urlfromflake.nixand requiresa full 40-char object name; a branch ref or short digest yields
NotPinned,since an unpinned oracle is the failure the pin exists to prevent. The rev is now
compared in
run_versionsand in a test that reads both files from disk.run_versionscrossedclippy::too_many_lines(103/100), so the flake read andthe comparison were extracted into
read_flake_revandverify_pinrather thansuppressed.
verify_pinbeing a free function also made the comparison directlyunit-testable.
3.
ci:runcargo xtask spec versionsin the compat job — the rev half iscovered by
cargo test --all, but the version half needs theFREDSHELL_REFERENCE_*vars only the devshell exports, so it can't run in thebare-runner matrix. Ordered before the compat report: if the oracle isn't the one
the fixtures were recorded against, the pass rate measures the wrong bash.
4.
chore(renovate):disable the bot fornixpkgs-reference— its PRs areunmergeable as written, since the policy needs
REFERENCE.md+ fixture workRenovate can't do. #45 sat open 2.5 weeks proving it.
Fixtures
Unaffected, and verified rather than assumed. All three revs give bash 5.3p15 /
coreutils 9.11, and
cargo xtask compatreportsregression: 0against the55-case corpus, so the recorded sidecars still match the pinned reference bash.
Worth noting:
REFERENCE.mdpolicy item 2 tells you to runcargo xtask spec record --all, but no--allflag exists —recordtakes asingle
<CASE>. Left alone here; flagging it as a real doc/impl mismatch.Verification
The guard was confirmed to actually fire, not just to pass. Re-pointing
REFERENCE.mdat the old rev:and
on_disk_reference_doc_rev_matches_the_flakefails with both revs named.cargo xtask check(stable)cargo xtask check(nightly 1.100.0)cargo xtask spec versionspre-commit run --all-filescargo test -p xtask9 new tests: 5 for
parse_flake_input_rev(dotted form, sibling-inputconfusion, unpinned branch ref, short digest, missing input), 3 for
verify_pin(consistent, stale-rev-with-agreeing-versions, version mismatch), 1 on-disk
consistency test.
Summary by CodeRabbit
Bug Fixes
Chores