Refuse two shim tables naming one command, which silently dropped one of them - #79
Merged
Merged
Conversation
… of them
`shim::run` resolves the declared shims into a map keyed by command name, so a
second `[[shim]]` naming a command already declared replaced the first and said
nothing. The surviving table is whichever the map happened to keep, and the lost
one is a set of verbs the repository believes are guarded.
Reproduced before this was written. A policy declaring `gh` twice -- once for
`pr:create` with `--body`, once for `issue:close` with `--comment`:
uphold scan policy checks passed
uphold shim gh issue close 1 --comment X refused by the checker
uphold shim gh pr create --body X exec'd the real gh, unexamined
Text the author had written a table for reached a forge with nothing in front of
it, and the run that was supposed to notice reported a clean policy.
WHY REFUSED RATHER THAN MERGED.
Merging is a guess about which vocabulary wins where two tables disagree, and
they disagree exactly where it matters: `-c` is a BOOLEAN on `gh pr review`
("Comment on a pull request") and takes a VALUE on `gh issue close` ("Leave a
closing comment"). A merged `text_flags` misreads one of those whichever way it
is built -- either `-c` swallows the next argument on a review, or a closing
comment is published unread.
That collision is the reason somebody would write a second table in the first
place, so the refusal has to make it visible to the person writing it rather
than resolve it silently. What the refusal cannot do is fix it: `text_flags` is
per-command and gh's flag meanings are per-verb, and closing that gap is a
schema question rather than a load check.
No policy in the surveyed workspace declares one command twice, so nothing that
loads today stops loading.
558 tests pass, clippy and fmt clean, every lefthook pre-commit command run
directly.
|
Warning Review limit reached
Next review available in: 36 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #79 +/- ##
==========================================
+ Coverage 90.42% 90.45% +0.02%
==========================================
Files 35 35
Lines 11225 11259 +34
==========================================
+ Hits 10150 10184 +34
Misses 1075 1075 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
HackingGate
added a commit
that referenced
this pull request
Aug 21, 2026
…ommand does (#81) A `[[shim]]` names one `text_flags` for a whole command, and a command's flags do not mean one thing. On `gh`, `-c` is a BOOLEAN on `pr review` -- "Comment on a pull request" -- and TAKES A VALUE on `issue close` -- "Leave a closing comment". Name it once for the table and one of the two is read wrong: named `gh pr review -c -b "body"` reads `-c` as consuming `-b`, and the body being published goes unread not named `gh issue close --comment "text"` publishes with nothing in front of it Both are false negatives in the seam that exists to prevent one, and #79 closed the workaround -- a second `[[shim]]` for the same command -- because merging two vocabularies is the same guess in a different place. So a table may now carry entries: [[shim]] command = "gh" match = ["pr:review", "issue:close"] text_flags = ["-t", "--title", "-b", "--body"] [[shim.verbs]] match = ["issue:close"] text_flags = ["-c", "--comment"] The entry's lists REPLACE the table's for the verbs it names rather than adding to them, which is the rule `allowed_scripts` already follows: what is declared beside the narrower thing is the whole truth for it. A union would mean a vocabulary nobody wrote -- here `issue close --body`, which the real command does not accept, and reading a flag a command will not take is the shim claiming to have checked a subject that was never published. `target_flags` is deliberately not overridable. `-R`/`--repo` means the same thing on every verb, and a per-verb answer to "which repository is this going to" would be a way to publish somewhere the table did not expect. WHY THE VERB IS KNOWN IN TIME. `reading` locates the subcommand by trying both arities for every option it does not know and matching under either -- "matching under either reading errs towards checking" -- so identification never needed the vocabulary it is about to select. Only collection does, and `for_verb` runs between them. A table with no entries borrows itself, which is every shim written before this. Load refuses an entry naming a verb the table's own `match` does not cover: the shim never stands in front of that invocation, so the flags classify nothing. That is usually a verb somebody meant to add to `match` and added here instead. WHAT THIS DOES NOT DO, AND THE ORDER THAT FORCED IT. uphold's own policy does not adopt this yet, and the attempt is why. Writing `[[shim.verbs]]` into policy/principles.toml made every `git` command in this tree fail closed: the installed binary IS the shim, it predates the field, and a policy it cannot parse is a policy that refuses everything. Its own .pre-commit-config.yaml pins the same version, so CI would have failed identically. The capability ships first and policies adopt after a binary that understands it is pinned -- the same ordering a new bundled set needs, for the same reason. 564 tests pass, clippy and fmt clean, every lefthook pre-commit command run directly.
HackingGate
added a commit
that referenced
this pull request
Aug 21, 2026
A `[[shim]]` named one `text_flags` for a whole command, and a command's flags do not all mean one thing. On `gh`, `-c` is a boolean on `pr review` and takes a value on `issue close`; naming it once for the table reads one of the two wrong, and either way the mistake is a false negative in the seam that exists to prevent one. `[[shim.verbs]]` lets the verbs whose grammar differs say so. The entry's lists replace the table's for those verbs rather than adding to them -- the rule `allowed_scripts` already follows, because a union would mean a vocabulary nobody wrote. `target_flags` stays table-wide: a per-verb answer to which repository something is going to would be a way to publish somewhere the table did not expect. Also in this release, from #76 and #78: a baseline entry may be signed `path | owner | reason` and required to be, and a baseline line that does not parse is refused rather than skipped -- the skip removed a ratchet silently, and the staleness check could not see it because a dropped entry was never listed. #79 refuses two shim tables naming one command, which used to drop one of them. TAKING IT. Nothing existing changes shape: a policy written for 1.7.0 resolves to exactly the rules it resolved to before, and no default changed. But a policy that USES `[[shim.verbs]]` cannot be read by an older binary, and the installed binary is the shim -- so a tree whose policy adopts the field before its shims are reinstalled has every git command failing closed. Bump the pin, reinstall, then adopt. Documented pins move to v1.8.0 in README.md and hooks/lefthook.yml.
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.
shim::runresolves the declared shims into a map keyed by command name, so a second[[shim]]naming a command already declared replaced the first and said nothing.Reproduced
A policy declaring
ghtwice -- once forpr:createwith--body, once forissue:closewith--comment:uphold scanpolicy checks passeduphold shim gh issue close 1 --comment Xuphold shim gh pr create --body XText the author had written a table for reached a forge with nothing in front of it, and the run that was supposed to notice reported a clean policy. The surviving table is whichever the map happened to keep.
Why refused rather than merged
Merging is a guess about which vocabulary wins where two tables disagree, and they disagree exactly where it matters:
-congh pr reviewis a boolean -- "Comment on a pull request"-congh issue closetakes a value -- "Leave a closing comment"A merged
text_flagsmisreads one of those whichever way it is built: either-cswallows the next argument on a review, or a closing comment is published unread.That collision is the reason somebody writes a second table in the first place, so the refusal has to make it visible to the person writing it rather than resolve it silently.
What this does not fix
text_flagsis per-command andgh's flag meanings are per-verb. Closing that is a schema question, not a load check, and it is filed separately -- along with the verbs that publish text and are matched by no table today.Blast radius
No policy in the surveyed workspace declares one command twice. Nothing that loads today stops loading.
Verification
cargo test-- 558 pass, 0 fail (556 + 2)cargo clippy --all-targets,cargo fmt --check-- cleanNew tests: the duplicate is refused and the message says what the silence cost; one table may name every verb for its command; and two tables for two different commands still load.