Three answers the seams gave about repositories they were not looking at - #7
Merged
Merged
Conversation
`refs/audit/pull/*` is a destination this subcommand writes and nothing else touches, and the fetch that fills it did not prune. So a ref left by an earlier run outlived the pull request it named -- and outlived the remote it came from, once origin was repointed at a different repository. Every commit and every blob under one was then walked as something this forge serves, and reported as would-be-republished. The branch half was already pruned, for the reason stated two functions up: a report that cries wolf about the unpublishable is one nobody finishes reading. This clone was carrying forty such refs, and the run over them buried the one finding that is real under sixty that are not -- 769 surfaces read, against 284 once the refspec prunes its own destination.
The link is on PATH for the whole machine; a `[[shim]]` is a line in one repository's policy. So the undeclared case is the ordinary one, not the exception -- every directory outside a participating repository reaches it, and so does every participating repository that declares a shim for some other command. Sixty-six of the seventy-one policies in the fleet declare none at all. While that answer was an error, installing the link the way the documentation describes made `git` exit 2 nearly everywhere it was typed, including outside a repository entirely. What gets installed after that is nothing, which loses the seam in the repositories that did declare one -- and that is what had happened: the links still pointed at the predecessor engine, because pointing them here broke the command. An absent declaration is a place the rule does not run, the same way an absent `[git]` table is. It is a reading rather than a failure to read, so it is not exit 2 by the rule that governs those; a policy that exists and cannot be READ still is, because the declaration nobody could read might have been the one. Asked for by name rather than run as the command, an undeclared shim stays an error: nothing is standing in front of anything and the caller asked.
`visibility` is the whole scope test for the three name guards, and it still said `private` a day after the flip. A rule that believes its repository is private refuses nothing, silently, and reports a clean tree while doing it -- so a declaration left behind by a visibility change is not a stale comment, it is the guard switched off. That is how the one finding the publication audit now reports was written: a commit message naming a private organisation, on a branch pushed after the flip, past a commit-msg guard that had been told it had nothing to protect. Each variant carries its own scope, so all three say it. The comment says which line to edit when a repository is flipped, and records what the flip already published: the squashed commit on the default branch is clean, the retained head ref of the pull request that merged it is not, and no rewrite reaches that one.
📝 WalkthroughWalkthroughThe change updates shim dispatch for undeclared commands and adds documentation and CLI coverage. It also prunes stale pull references before audit publication and changes repository-name policy visibility to public. ChangesShim dispatch behavior
Audit publication references
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Invocation
participant shim_command
participant shim_run
participant UnderlyingCommand
Invocation->>shim_command: invoke command or `uphold shim <command>`
shim_command->>shim_run: pass invocation mode
alt command-name invocation and no declared shim
shim_run->>UnderlyingCommand: execute transparently
else explicit by-name invocation
shim_run-->>Invocation: return undeclared-shim error
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (53.12%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7 +/- ##
==========================================
+ Coverage 87.74% 88.38% +0.63%
==========================================
Files 24 24
Lines 7140 7162 +22
==========================================
+ Hits 6265 6330 +65
+ Misses 875 832 -43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@docs/REFERENCE.md`:
- Around line 481-489: Update the no-policy explanation in the paragraph around
“where nothing declares the command” to distinguish the two cases: discovery
finding no policy reads nothing, while a policy declaring only an unrelated shim
is read and confirms the command is undeclared. Preserve the existing exit-code
behavior and the separate unreadable-policy explanation.
In `@src/shim.rs`:
- Around line 820-830: Update exec_through to handle current_exe() failure
explicitly instead of converting it to None: return a Fatal error containing the
underlying error before calling real_command, while preserving the existing
command execution flow when the executable path is resolved successfully.
In `@tests/audit_publication_cli.rs`:
- Around line 296-301: Strengthen the test around audit(&root) by asserting that
refs/audit/pull/9 was pruned and no longer exists. Also verify the report
includes the successful empty-fetch message and excludes the fetch-failure
message, while retaining the existing stale-marker assertions.
🪄 Autofix
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 Plus
Run ID: 15db3495-2ddf-46fd-b85b-810416a6ec98
📒 Files selected for processing (7)
docs/REFERENCE.mdpolicy/principles.tomlsrc/audit.rssrc/main.rssrc/shim.rstests/audit_publication_cli.rstests/shim_cli.rs
Comment on lines
+481
to
+489
| That link is on PATH for the whole machine, while a `[[shim]]` is a line in one | ||
| repository's policy — so **where nothing declares the command, the command | ||
| simply runs**: no policy in this directory, or a policy that declares a shim for | ||
| some other command. Neither is a could-not-look, so neither is exit `2`; the | ||
| policy was read and it said this command is not one it stands in front of. A | ||
| policy that exists and cannot be *read* still exits `2`, because the | ||
| declaration that could not be read might have been the one. Asked for by name — | ||
| `uphold shim faux …` — an undeclared command is still an error, since nothing is | ||
| standing in front of anything and the caller asked. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the no-policy explanation.
The paragraph includes the no-policy case, but Lines 484-485 state that “the policy was read.” No policy is read when discovery finds no policy. State that only the unrelated-shim case reads a policy.
🤖 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 `@docs/REFERENCE.md` around lines 481 - 489, Update the no-policy explanation
in the paragraph around “where nothing declares the command” to distinguish the
two cases: discovery finding no policy reads nothing, while a policy declaring
only an unrelated shim is read and confirms the command is undeclared. Preserve
the existing exit-code behavior and the separate unreadable-policy explanation.
Comment on lines
+820
to
+830
| pub(crate) fn exec_through(name: &str, argv: &[OsString]) -> Result<Exit> { | ||
| let own = std::env::current_exe().ok(); | ||
| let Some(real) = real_command(name, own.as_deref()) else { | ||
| return Err(Fatal::new(format!( | ||
| "nothing here stands in front of {name}, and there is no {name} on PATH to run" | ||
| ))); | ||
| }; | ||
| let mut command = Command::new(&real); | ||
| command.args(argv); | ||
| hand_off(&mut command, name, None) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail when the shim cannot identify itself.
Line 821 discards a current_exe() failure. If that failure occurs, real_command cannot exclude the PATH link that launched the shim. It can select that link and exec this process again indefinitely. Return a Fatal error when current_exe() fails.
Proposed fix
- let own = std::env::current_exe().ok();
- let Some(real) = real_command(name, own.as_deref()) else {
+ let own = std::env::current_exe()
+ .map_err(|error| Fatal::new(format!("{name}: cannot identify shim executable: {error}")))?;
+ let Some(real) = real_command(name, Some(&own)) else {As per coding guidelines, “When continuing cannot satisfy the contract safely, detect the condition at the earliest reliable boundary and return an explicit failure with evidence.”
📝 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
| pub(crate) fn exec_through(name: &str, argv: &[OsString]) -> Result<Exit> { | |
| let own = std::env::current_exe().ok(); | |
| let Some(real) = real_command(name, own.as_deref()) else { | |
| return Err(Fatal::new(format!( | |
| "nothing here stands in front of {name}, and there is no {name} on PATH to run" | |
| ))); | |
| }; | |
| let mut command = Command::new(&real); | |
| command.args(argv); | |
| hand_off(&mut command, name, None) | |
| } | |
| pub(crate) fn exec_through(name: &str, argv: &[OsString]) -> Result<Exit> { | |
| let own = std::env::current_exe() | |
| .map_err(|error| Fatal::new(format!("{name}: cannot identify shim executable: {error}")))?; | |
| let Some(real) = real_command(name, Some(&own)) else { | |
| return Err(Fatal::new(format!( | |
| "nothing here stands in front of {name}, and there is no {name} on PATH to run" | |
| ))); | |
| }; | |
| let mut command = Command::new(&real); | |
| command.args(argv); | |
| hand_off(&mut command, name, None) | |
| } |
🤖 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 `@src/shim.rs` around lines 820 - 830, Update exec_through to handle
current_exe() failure explicitly instead of converting it to None: return a
Fatal error containing the underlying error before calling real_command, while
preserving the existing command execution flow when the executable path is
resolved successfully.
Source: Coding guidelines
Comment on lines
+296
to
+301
| let output = audit(&root); | ||
| let report = text(&output); | ||
| assert!( | ||
| !report.contains("STALE.md") && !report.contains("some other forge"), | ||
| "a pruned pull ref was still read as a surface this forge serves:\n{report}" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the stale destination ref was pruned.
This assertion also passes when retained_pull_refs cannot fetch origin. That path returns before it reads refs/audit/pull/9, so neither stale marker appears in the report.
After audit(&root), verify that refs/audit/pull/9 no longer exists. Also assert that the report contains the successful empty-fetch message and does not contain the fetch-failure message.
Proposed test strengthening
let output = audit(&root);
let report = text(&output);
+ let stale_ref = Command::new("git")
+ .args(["show-ref", "--verify", "--quiet", "refs/audit/pull/9"])
+ .current_dir(&root)
+ .status()
+ .unwrap();
+ assert!(!stale_ref.success(), "the stale audit pull ref was not pruned");
+ assert!(
+ report.contains("refs/pull/*/head fetched no commits")
+ && !report.contains("refs/pull/*/head could not be fetched"),
+ "the pull-ref fetch did not complete successfully:\n{report}"
+ );
assert!(
!report.contains("STALE.md") && !report.contains("some other forge"),As per coding guidelines, a constraint becomes machine enforcement only when it has observable evidence.
📝 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 output = audit(&root); | |
| let report = text(&output); | |
| assert!( | |
| !report.contains("STALE.md") && !report.contains("some other forge"), | |
| "a pruned pull ref was still read as a surface this forge serves:\n{report}" | |
| ); | |
| let output = audit(&root); | |
| let report = text(&output); | |
| let stale_ref = Command::new("git") | |
| .args(["show-ref", "--verify", "--quiet", "refs/audit/pull/9"]) | |
| .current_dir(&root) | |
| .status() | |
| .unwrap(); | |
| assert!(!stale_ref.success(), "the stale audit pull ref was not pruned"); | |
| assert!( | |
| report.contains("refs/pull/*/head fetched no commits") | |
| && !report.contains("refs/pull/*/head could not be fetched"), | |
| "the pull-ref fetch did not complete successfully:\n{report}" | |
| ); | |
| assert!( | |
| !report.contains("STALE.md") && !report.contains("some other forge"), | |
| "a pruned pull ref was still read as a surface this forge serves:\n{report}" | |
| ); |
🤖 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 `@tests/audit_publication_cli.rs` around lines 296 - 301, Strengthen the test
around audit(&root) by asserting that refs/audit/pull/9 was pruned and no longer
exists. Also verify the report includes the successful empty-fetch message and
excludes the fetch-failure message, while retaining the existing stale-marker
assertions.
Source: Coding guidelines
Merged
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.
Three findings from asking whether the migration to this repository is finished. It is not, and each of these is a place where a seam reported something other than what it had read.
The shim could not be installed. A link named for a command is on PATH for the whole machine, while a
[[shim]]is a line in one repository's policy — and an undeclared command was an error. Sixty-six of the seventy-one policies in the fleet declare no shim at all, so installing the link as the documentation describes madegitexit 2 nearly everywhere it was typed, including outside a repository entirely. The consequence was visible on the machine that tried: the links still pointed at the predecessor engine. An absent declaration is now a place the rule does not run, the same way an absent[git]table is. A policy that exists and cannot be read is still exit 2, anduphold shim <name>asked for an undeclared command is still an error.audit --for-publicationwalked refs the forge no longer serves.refs/audit/pull/*is written by that subcommand and by nothing else, and the fetch filling it did not prune — so refs from an earlier run against a different remote were walked as surfaces this forge publishes. Forty of them here: 769 surfaces read, against 284 once the refspec prunes its own destination, and one real finding that sixty false ones had buried.This repository told its own guards it was private. A day after it was made public.
visibilityis the whole scope test, so all three name guards stood down, and that is how the finding above got written — a commit message naming a private organisation, pushed past acommit-msgguard that had been told it had nothing to protect. The squashed commit onmainis clean; the retained head ref of the pull request that merged it is not, and no rewrite reaches that one.Tests fail without each fix and pass with it.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation