Let the loader answer which rules run, and close the second reader - #5
Conversation
`rules --effective --json` emitted `git_hooks` and nothing else, so a rule that fires at no git hook was indistinguishable from any other rule that fires at no git hook -- and there are two unrelated kinds. `files.*` is the scan's; `command.before` is a checker standing in front of a command, which runs when the shim is on PATH ahead of the real one. A reader with only the hooks has to guess between them. `uphold_check.py` guessed the scan. Its `elif scan` branch credited every hookless rule to `uphold scan`, so a claim on a shim-only rule reconciled green, exit 0, in a repository that pins `uphold-scan` and nothing else -- over a rule the scan never touches. This repository declares two such rules of its own, `no-published-host-identity` and `no-published-markers`. The drift test between the two readers did not catch it and could not have: it compared `git_hooks`, and both readers agreed on the empty list. They were wrong in the same direction, which is the failure mode a comparison of two implementations has and a comparison against the answer does not. So the loader says it. `Rule::seams` returns `scan` / `guard` / `shim` from the same conditions the three seams use to select rules, the JSON carries it, and the human form prints it where it used to print "no git hook" of both kinds. `_rule_stages` reads `command.before` to match, and returns a `Where` carrying both fields, so the drift test compares the seam as well. A shim-only rule is reported as a seam this script cannot establish -- no runner configuration here says whether the shim is on PATH -- rather than as one the scan supplies. That is could-not-look, and `inventory_local` remains where a repository asserts a seam this script cannot observe.
`--review` builds `claimed` from every entry in the declaration and `active` from the entries whose rule a seam actually supplies. Two lines apart, one filtered and one not, and the unfiltered one decided what a human is asked about. `review.route` drops an `automatable = "yes"` record when a rule claims it -- "a rule enforces it; a reviewer repeating it is noise" -- which is right when a rule does enforce it. A claim naming a rule no seam here supplies enforces nothing, so it is not that case: the record left the review document, and the rule was absent from the "already active here" list the same document prints, because that list IS filtered. Enforced by nothing, reviewed by nobody, and the page showed no trace of either. The reconcile refuses such a claim outright. This mode has to survive one, because it runs over a declaration somebody is still writing -- so it filters rather than refuses, and the record goes back to the reviewer it was taken from.
|
Warning Review limit reached
Next review available in: 49 minutes 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?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Rust binary now performs policy reconciliation and coverage reporting. The Python script delegates engine-backed results for review and OSCAL generation. Hooks, workflows, documentation, and tests use the new ChangesBinary-backed policy checking
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Checker
participant Config
participant Catalog
participant Python
CLI->>Checker: run check or check --coverage
Checker->>Config: discover installed seams
Checker->>Catalog: validate principle claims
Checker-->>CLI: return reconciliation or coverage results
Python->>Checker: request supplier evidence
Checker-->>Python: return binary-backed suppliers
Python-->>Python: generate review or OSCAL output
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 (92.04%) 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 #5 +/- ##
==========================================
+ Coverage 87.19% 87.63% +0.44%
==========================================
Files 22 24 +2
Lines 6605 7094 +489
==========================================
+ Hits 5759 6217 +458
- Misses 846 877 +31 ☔ 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 146-151: Update the `seams` documentation to state that every
valid effective rule contains exactly one seam: `scan`, `guard`, or `shim`.
Clarify that multiple seams are not currently supported, `git.hooks` is valid
only for `builtin` rules, `command.before` only for `exec` rules, and
file-reading non-builtins support neither.
In `@uphold_check.py`:
- Around line 909-934: Update the early-return path before the declared-rule
loop so it parses an existing policy and runs coverage analysis even when
neither scan nor guard is configured. Ensure shim-only rules reach the “shim”
branch in the declared-rules loop and report the “stands in front of a command”
diagnostic instead of the generic runner note, while preserving behavior when no
policy exists.
- Around line 615-620: Update the hook validation in the rule-processing logic
around git.hooks to raise CouldNotLook as soon as any entry is not a string,
instead of filtering invalid values out. Preserve the existing set construction
for valid string hooks, and add a regression test covering an integer hook
entry.
🪄 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: ca45b7b5-fc95-4505-a7be-0b2505ff2608
📒 Files selected for processing (7)
docs/REFERENCE.mdsrc/config.rssrc/main.rstests/scan_cli.rstests/test_review.pytests/test_uphold_check.pyuphold_check.py
| `seams` is `scan`, `guard`, `shim`, or more than one, and it is the half | ||
| `git_hooks` cannot express. An empty hook list is true of a content rule and of | ||
| a checker standing in front of a command alike, so a reader with only the hooks | ||
| has to guess between two unrelated places — and the reconciler guessed `scan`, | ||
| which credited a shim-only rule to a seam that never touches it. An empty | ||
| `seams` means nothing runs the rule at all, which the loader refuses. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe the valid seams cardinality correctly.
A loaded rule cannot currently have more than one seam. Validation permits git.hooks only on builtin rules and command.before only on exec rules. File-reading non-builtins cannot use either. State that each valid effective rule contains exactly one of scan, guard, or shim.
🤖 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 146 - 151, Update the `seams` documentation
to state that every valid effective rule contains exactly one seam: `scan`,
`guard`, or `shim`. Clarify that multiple seams are not currently supported,
`git.hooks` is valid only for `builtin` rules, `command.before` only for `exec`
rules, and file-reading non-builtins support neither.
|
Holding this rather than merging. The seam derivation it adds to The engine half here survives that port and is needed by it: |
`uphold_check.py` re-implemented `config::load` to reconcile a declaration: bundled sets, `inherit.paths`, `inherit.disabled_rules`, and a repository's own rule shadowing an inherited id. Five interacting fields, read twice, by two programs free to disagree -- and `effective_rules_command` has said since it was written that this is what would end it: "every second reader of them is a reader free to disagree with the engine about which rules run. The reconciler in `uphold_check.py` is that second reader today." They disagreed about the seam a hookless rule runs at. `files.*` is the scan's and `command.before` is a checker standing in front of a command; both come back with no git hooks, and the reconciler read the second as the first. A claim on a shim-only rule reconciled green, exit 0, in a repository that pins `uphold-scan` and nothing else -- over a rule the scan never touches. `Rule::seams` answers it now, from the same conditions the three seams use to select rules, and it is on `rules --effective --json` for anything else that has to ask. The reconcile and the coverage report are `uphold check` and `uphold check --coverage`. What stayed in Python is what never reads the policy -- `--explain`, `--list`, `--review`, `--oscal`, `--init` -- because a mode that cannot read the policy cannot disagree with the loader about which rules run. `--oscal` gates on the reconcile, so it asks the binary and treats an unreachable one as could-not-look; `--review` asks too but survives a refusal, because it runs over a declaration somebody is still writing. `uphold-check` becomes `language: rust` like every other id in the manifest. It was the only `language: script` one, and pre-commit and prek key an environment on (repo, language, version) -- so it now shares the environment the other seven ids already build, and costs no second compile. Three things the port fixes rather than carries: The runner configs are PARSED. The script line-scanned them because it could take no dependency, which is how `configs:` -- the key README.md tells every lefthook consumer to write under `remotes:` -- was read as a command name. `package.repository` is read at compile time through `CARGO_PKG_REPOSITORY` rather than off disk, so the slug cannot drift from the crate and works outside a checkout of this repository. The coverage denominator counts what a seam SUPPLIES. `records: N of M` was computed from the claims, so a declaration whose only claim named a rule nothing runs reported one record as claimed two lines under the line saying that rule is supplied by nothing. Verified against the fleet: `uphold check` and the reader it replaces return the same exit code and the same claim count in 68 of 71 repositories. The three that differ have a policy `config::load` refuses outright -- `command.before` on a `builtin` -- which the Python never validated and which `uphold scan` already fails on today. 35 behaviour tests move from tests/test_uphold_check.py to tests/check_cli.rs rather than going away, and two caught real regressions in the port: a lefthook remote given as a filesystem path, which is what scripts/consumer_check.sh writes, and the rule that a remote is only ours when ONE entry names this repository and takes its config.
`uphold check` and `uphold check --coverage` where the reconcile used to be, and a paragraph in the README on where the split falls: a mode that decides whether a check passed reads the policy, and the loader that resolves the policy is the binary. What is left in the script renders prose for a person and cannot disagree with the engine about anything.
HackingGate
changed the title
CI found the constraint `content_policy_rules` documented before this port started: the modes that ask the engine run in a pre-commit environment, and the binary is not on PATH there. `--review` is a `language: system` hook in this repository's own config, so it ran on a machine holding cargo, a checkout, and every ingredient except the one command nobody had run -- and reported could-not-look on a tree that was fine. Three attempts now, in order: a built binary under `target/`, then PATH, then `cargo run --manifest-path`. The last is not a convenience. Neither caller leaves this repository -- `--review` is in no consumer's manifest and `--oscal` is run by hand -- so the fallback costs a consumer nothing and is the difference between a hook that works in a fresh checkout and one that needs a build first. A binary none of the three can produce is still could-not-look, and still exit 2. It is not a smaller answer or an older one.
Three call sites still invoked `uphold_check.py` for a mode it no longer has: the `uphold-check-here` pre-commit hook, the `uphold-check` lefthook command, and the trigger list on both, which still named `.cmd-shims/checks.enabled`. They run `cargo run --quiet -- check` now, beside the `content-policy` and `guards` commands that already did. The Python tests that reach the engine skip where neither a built binary nor cargo can answer, which is the precedent `test_the_two_readers_of_the_policy_agree` already set: the catalog job runs on an image with no Rust toolchain by design, and a test needing a `cargo build` to be meaningful must not report a red suite to somebody who has not run one. What is skipped there is asserted in tests/check_cli.rs, which runs where a toolchain exists -- so the behaviour is covered, and it is covered in the language that can reach it. Verified by running the suite with `target/` moved aside and a PATH holding python3 and git and nothing else: 22 skip, the rest pass. With a toolchain, all 73 run.
The port scoped hook ids by the `repo:` url they were pinned under, on the reasoning that another repository's `uphold-scan` establishes nothing here. The reasoning is fine and the predicate is wrong, which the consumer harness caught: `scripts/consumer_check.sh` clones this repository into a temporary directory and pins it by PATH, so the last segment of its `repo:` is `hooks`. Every guard the consumer pinned was read as belonging to somebody else, and a true claim was refused in the one job that drives a real consumer end to end. The id is specific enough on its own -- `uphold-guard-push` is this binary's name for this binary's stage -- and the manifest is where the list comes from, so an id cannot be published there and forgotten here. This is what the test named `the_seam_is_found_by_a_published_id_not_by_one_repositorys_name` has been saying since before the port; I ported the name and inverted the behaviour. It now asserts what it says, over every published guard id and against a `repo:` url naming no owner at all. `names_this_repository` stays for the lefthook `remotes:` block, where the url is the only thing there is to match on and both halves of one entry have to agree.
The job proved the reconcile by running `uphold_check.py` from a directory that is not this repository, which is the right shape and the wrong entry point: the reconcile is `uphold check`. It runs on a toolchain-free image, so it builds one, and builds rather than pinning a release because what is under test is this commit's reconcile against this commit's manifest. The consumer also gets a `policy/principles.toml`. It had none, and did not need one while the reader tolerated a repository with no policy; the loader does not, and a repository with nothing to resolve has nothing to reconcile against. Both steps verified locally against the built binary: the starter declaration reconciles at exit 0 in a tree with no runner configuration, and a directory with no declaration at all is still exit 2.
`hooks/lefthook.yml` -- the config a lefthook consumer inherits -- still ran `uphold_check.py` through `.lefthook/pre-commit/uphold-check`, so a clean commit in the lefthook consumer was refused by a usage message. That wrapper existed for one reason, written at the top of it: "uphold_check.py is not in the binary, so it has to be reached IN THIS REPOSITORY -- and lefthook's `scripts` is the one mechanism that resolves against the remote clone rather than against the consumer's own tree." The reconcile is `uphold check` now and PATH reaches it, so the job is a plain `run:` beside `uphold scan` and `uphold guard`, and the wrapper is deleted. It stays a `jobs:` entry rather than a command, for the reason already recorded: `glob` is a job key, and a check with no firing condition would load the catalog in front of every one-line fix while `.pre-commit-hooks.yaml` publishes the opposite as the design. The shell-lint globs in lefthook.yml go back to `scripts/*.sh`. The brace pattern was there to catch an extensionless wrapper that no longer exists.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
203-217: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTwo lines still describe the script as the reconciler.
Line 206 tells the reader to run
./uphold_check.pyfor "this repo's own declaration". That invocation now prints usage and exits 2. Line 215 callsuphold_check.pythe "reconciler; the hook other repos install";.pre-commit-hooks.yamlnow publishesuphold checkunderlanguage: rust.📝 Proposed fix
python3 -m unittest discover -s tests -./uphold_check.py # this repo's own declaration +cargo run --quiet -- check # this repo's own declaration cargo run --quiet -- guard --stage manual # every pin still names a refprinciples/*.toml canonical records QUICK_REFERENCE.md generated human index REVIEW.md, AGENTS.md generated review tier: the judgment no rule decides name-index.json generated lookup index: every name -> a record id -uphold_check.py reconciler; the hook other repos install +uphold_check.py the catalog: explain, list, init, review, OSCAL export scripts/ analysis, catalog loading, validation, generation🤖 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 `@README.md` around lines 203 - 217, Update the README command and generated-file descriptions to match the current CLI: replace the obsolete ./uphold_check.py invocation with the supported catalog command, and revise the uphold_check.py entry to describe it as the catalog supporting explain, list, init, review, and OSCAL export rather than as a reconciler or installed hook.
🧹 Nitpick comments (6)
src/catalog.rs (1)
145-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider rejecting unrecognized
statusandautomatablevalues.
deprecated()andrefuses_automation()compare exact strings. A record that writesautomatable = "No","none", orstatus = "Deprecated"is then treated as claimable and not deprecated. The records are compiled in, so this is not exploitable today. A typed enum would turn a future typo into a parse failure instead of a silently wrong reconcile.♻️ Proposed typed representation
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize)] +#[serde(rename_all = "lowercase")] +pub(crate) enum Automatable { + Yes, + Partial, + No, +} + #[derive(Debug, Clone, Default, Deserialize)] pub(crate) struct Enforcement { #[serde(default)] - pub automatable: Option<String>, + pub automatable: Option<Automatable>, }🤖 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/catalog.rs` around lines 145 - 158, Replace the string-based status and automatable handling used by Record::deprecated, Record::refuses_automation, and Record::automatable with typed representations that reject unrecognized values during record parsing, while preserving the existing semantics for valid "deprecated" and "no" values..github/workflows/test.yml (1)
326-326: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop
-rfrom the file copy.
policy/principles.tomlis a single file.cp policy/principles.toml /tmp/consumer/policy/principles.tomlis enough, and it fails loudly if the source ever becomes a directory.🤖 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/test.yml at line 326, Update the copy command in the workflow to remove the recursive -r option when copying policy/principles.toml to the consumer policy destination, preserving a direct single-file copy that fails if the source is a directory.tests/check_cli.rs (1)
91-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for the rejected argument form.
src/main.rsLines 235-237 return exit 2 withusage: uphold check [--coverage]for any other argument list. No test here covers it, and Codecov reports 38 uncovered changed lines. One assertion oncheck(&root, &["--bogus"])closes that gap cheaply.🤖 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/check_cli.rs` around lines 91 - 116, Add a test alongside the existing exit-code contract tests that invokes check(&root, &["--bogus"]) and asserts exit code 2, covering the rejected argument path in the check command while preserving the existing setup and assertion style.src/main.rs (1)
46-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument
--upstreaminUSAGE.
--upstreamis a supported option anduphold_check.py::upstream_urldepends on it.USAGElists--versionand--helpbut not--upstream, so the printed help hides an interface another program calls.📝 Proposed addition
uphold shim <command> [args...] check what a command would publish, then run it + uphold --upstream the repository url this binary was built from🤖 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/main.rs` around lines 46 - 53, Update the USAGE help text to document the supported --upstream option, including its expected value or argument, alongside the existing global options such as --version and --help. Keep the wording consistent with the other entries so uphold_check.py::upstream_url is represented in the printed interface..pre-commit-hooks.yaml (1)
40-40: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
inherit.pathsfiles do not fire this hook.The trigger list names
policy/principles.tomlbut not files reached throughinherit.paths.tests/check_cli.rs::a_rule_inherited_through_inherit_paths_is_suppliedshows an inherited file can define a claimed rule. An edit that removes that rule turns a true claim false, and this hook does not run. The same gap exists in.pre-commit-config.yamlandhooks/lefthook.yml. Consider widening the pattern to^policy/.*\.toml$.🤖 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 @.pre-commit-hooks.yaml at line 40, Widen the file pattern for the hook in `.pre-commit-hooks.yaml` from individual policy files to all TOML files under `policy/`, so changes to files referenced through `inherit.paths` trigger it. Preserve the existing configuration-file matches, including `.pre-commit-config.yaml` and `lefthook.yml`.tests/test_review.py (1)
30-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
needs_the_engineprobe, and neither copy checks the exit code. Both test modules define byte-identical decorators.uphold_check.engineraisesCouldNotLookonly when no attempt starts; it returns the firstCompletedProcessotherwise without readingreturncode. Anupholdon PATH that answers non-zero therefore leaves the decorated tests enabled, and they fail where the intent is to skip. Extract one helper and add the exit-code check in that single place.
tests/test_review.py#L30-L43: keep the implementation, addif answered.returncode != 0: return unittest.skip(...)(test), and move it into a shared test helper module.tests/test_uphold_check.py#L26-L39: delete this copy and import the shared helper 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 `@tests/test_review.py` around lines 30 - 43, Extract the duplicated needs_the_engine decorator from tests/test_review.py:30-43 into a shared test helper, retaining the CouldNotLook skip and also skipping when the returned answered.returncode is non-zero. Update tests/test_uphold_check.py:26-39 to delete its local copy and import the shared helper; both sites should use the single corrected implementation.
🤖 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/test.yml:
- Around line 329-335: Update the workflow step named “A declaration that cannot
be read is not a pass” so it exercises an actual unreadable or malformed
declaration rather than an empty directory; create the required declaration
fixture in /tmp/unreadable and apply permissions or content that triggers the
intended read failure, while preserving the expected nonzero assertion for
uphold check. If the test is meant to cover missing policy instead, rename the
step accordingly.
In `@src/check.rs`:
- Around line 539-563: Update the validation branches in the declaration
enforcement loop to classify leftover tier, missing principle/rule, and
blank-field cases as refused claims rather than fatal inspection errors: record
each claim in failures and continue processing, unless the documented contract
explicitly defines malformed declarations as exit 2. If retaining Fatal::at,
revise the related doc comment to state that malformed declarations produce that
exit classification.
- Around line 289-312: Update the run-processing loop to collect and handle
every matching subcommand seam instead of stopping at the first match from
words.windows(2).find_map(...). Preserve the existing validation for the
preceding word and apply the scan and guard handling logic to each matching
pair, so commands such as “uphold scan && uphold guard” register both seams.
- Around line 610-625: Update the success path in run() to print every entry in
installed.unreadable alongside the existing installed.how notes, using the same
“could not look” reporting style as the coverage path while preserving the clean
exit behavior.
- Around line 274-277: Update lefthook_seams() to discover all supported
Lefthook configuration names, including lefthook.yml, lefthook.yaml, their
dot-prefixed forms, and -local variants, instead of returning early when
lefthook.yml is absent. Preserve seam detection and installed() behavior for
every valid configuration name.
- Around line 283-288: Update the stage-processing loop in the relevant check
flow to recognize Lefthook stage/group keys independently of the manifest-only
guards map, including uphold-manual. Parse each guard command’s --stage
argument, add the referenced manual guard to Installed.stages, and collect its
command names into Installed.local only when the stage/group key is valid; do
not use guards as the stage-key filter.
In `@tests/check_cli.rs`:
- Around line 672-691: Update
the_starter_declaration_is_valid_and_enforces_nothing so a missing python3
interpreter skips the test instead of panicking on Command::output().unwrap();
handle the Command construction/execution error using the test suite’s
established skip mechanism, while preserving the existing assertions when
python3 is available.
In `@tests/test_review.py`:
- Around line 30-43: Update needs_the_engine to inspect the CompletedProcess
returned by uphold_check.engine(ROOT, "--version") and skip the test when its
returncode is non-zero, while preserving the existing CouldNotLook skip behavior
and allowing execution only when the probe succeeds.
In `@tests/test_uphold_check.py`:
- Around line 137-145: Remove the class-level needs_the_engine decorator from
NoProseInRuntime and apply it only to the two test methods that invoke --review.
Leave test_the_catalog_modes_carry_no_record_prose_into_a_report undecorated so
the --list catalog assertion runs without a Rust toolchain.
In `@uphold_check.py`:
- Around line 240-278: Update engine_suppliers so strict=False preserves
evidence for claims that reconcile even when other claims fail; either make the
underlying engine output successful claim evidence on violation exits or have
engine_suppliers reconcile claims individually. Ensure run_review still receives
every valid rule in suppliers while strict=True continues refusing unreconciled
enforcement results.
---
Outside diff comments:
In `@README.md`:
- Around line 203-217: Update the README command and generated-file descriptions
to match the current CLI: replace the obsolete ./uphold_check.py invocation with
the supported catalog command, and revise the uphold_check.py entry to describe
it as the catalog supporting explain, list, init, review, and OSCAL export
rather than as a reconciler or installed hook.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Line 326: Update the copy command in the workflow to remove the recursive -r
option when copying policy/principles.toml to the consumer policy destination,
preserving a direct single-file copy that fails if the source is a directory.
In @.pre-commit-hooks.yaml:
- Line 40: Widen the file pattern for the hook in `.pre-commit-hooks.yaml` from
individual policy files to all TOML files under `policy/`, so changes to files
referenced through `inherit.paths` trigger it. Preserve the existing
configuration-file matches, including `.pre-commit-config.yaml` and
`lefthook.yml`.
In `@src/catalog.rs`:
- Around line 145-158: Replace the string-based status and automatable handling
used by Record::deprecated, Record::refuses_automation, and Record::automatable
with typed representations that reject unrecognized values during record
parsing, while preserving the existing semantics for valid "deprecated" and "no"
values.
In `@src/main.rs`:
- Around line 46-53: Update the USAGE help text to document the supported
--upstream option, including its expected value or argument, alongside the
existing global options such as --version and --help. Keep the wording
consistent with the other entries so uphold_check.py::upstream_url is
represented in the printed interface.
In `@tests/check_cli.rs`:
- Around line 91-116: Add a test alongside the existing exit-code contract tests
that invokes check(&root, &["--bogus"]) and asserts exit code 2, covering the
rejected argument path in the check command while preserving the existing setup
and assertion style.
In `@tests/test_review.py`:
- Around line 30-43: Extract the duplicated needs_the_engine decorator from
tests/test_review.py:30-43 into a shared test helper, retaining the CouldNotLook
skip and also skipping when the returned answered.returncode is non-zero. Update
tests/test_uphold_check.py:26-39 to delete its local copy and import the shared
helper; both sites should use the single corrected implementation.
🪄 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: c8c877df-84ee-48c0-8b38-0985dbfff164
📒 Files selected for processing (15)
.github/workflows/test.yml.lefthook/pre-commit/uphold-check.pre-commit-config.yaml.pre-commit-hooks.yamlREADME.mddocs/REFERENCE.mdhooks/lefthook.ymllefthook.ymlsrc/catalog.rssrc/check.rssrc/main.rstests/check_cli.rstests/test_review.pytests/test_uphold_check.pyuphold_check.py
💤 Files with no reviewable changes (1)
- .lefthook/pre-commit/uphold-check
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/REFERENCE.md
| - name: A declaration that cannot be read is not a pass | ||
| run: | | ||
| set -euo pipefail | ||
| mkdir -p /tmp/unreadable | ||
| cd /tmp/unreadable | ||
| set +e | ||
| "$GITHUB_WORKSPACE/uphold_check.py" | ||
| "$GITHUB_WORKSPACE/target/release/uphold" check |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The step name does not match what the step exercises.
/tmp/unreadable is empty. uphold check therefore fails at policy discovery, not at reading a declaration. The exit code is 2 in both cases, so the assertion passes for the wrong reason, and a regression in the unreadable-declaration path would not be caught here. Write an actual malformed declaration into the directory, or rename the step to describe missing policy.
🧪 Proposed fix
- - name: A declaration that cannot be read is not a pass
+ - name: A declaration that cannot be parsed is not a pass
run: |
set -euo pipefail
- mkdir -p /tmp/unreadable
+ mkdir -p /tmp/unreadable/policy
+ cp policy/principles.toml /tmp/unreadable/policy/principles.toml
+ printf '[[enforce]] this is not toml\n' > /tmp/unreadable/policy/upheld.toml
cd /tmp/unreadable📝 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.
| - name: A declaration that cannot be read is not a pass | |
| run: | | |
| set -euo pipefail | |
| mkdir -p /tmp/unreadable | |
| cd /tmp/unreadable | |
| set +e | |
| "$GITHUB_WORKSPACE/uphold_check.py" | |
| "$GITHUB_WORKSPACE/target/release/uphold" check | |
| - name: A declaration that cannot be parsed is not a pass | |
| run: | | |
| set -euo pipefail | |
| mkdir -p /tmp/unreadable/policy | |
| cp policy/principles.toml /tmp/unreadable/policy/principles.toml | |
| printf '[[enforce]] this is not toml\n' > /tmp/unreadable/policy/upheld.toml | |
| cd /tmp/unreadable | |
| set +e | |
| "$GITHUB_WORKSPACE/target/release/uphold" check |
🤖 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/test.yml around lines 329 - 335, Update the workflow step
named “A declaration that cannot be read is not a pass” so it exercises an
actual unreadable or malformed declaration rather than an empty directory;
create the required declaration fixture in /tmp/unreadable and apply permissions
or content that triggers the intended read failure, while preserving the
expected nonzero assertion for uphold check. If the test is meant to cover
missing policy instead, rename the step accordingly.
| let path = root.join("lefthook.yml"); | ||
| if !path.is_file() { | ||
| return Ok(found); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
lefthook supported configuration file names lefthook.yaml lefthook-local
💡 Result:
Lefthook supports a variety of configuration file names and formats, allowing for both project-wide configurations and local developer-specific overrides [1][2]. Main Configuration Files Lefthook searches for a main configuration file using the base names lefthook or.lefthook, supporting YAML, TOML, JSON, and JSONC formats [1][2]. Supported filenames include [2]: - YAML: lefthook.yml, lefthook.yaml,.lefthook.yml,.lefthook.yaml,.config/lefthook.yml,.config/lefthook.yaml - TOML: lefthook.toml,.lefthook.toml,.config/lefthook.toml - JSON: lefthook.json,.lefthook.json,.config/lefthook.json - JSONC: lefthook.jsonc,.lefthook.jsonc,.config/lefthook.jsonc Only one main configuration file should be used per project [1][2]. Files without a leading dot are also looked up in the.config subdirectory [1][3]. Local Configuration Files Lefthook supports an additional local configuration file to merge with or override the main configuration [1][2]. This file uses the base name lefthook-local or.lefthook-local, followed by the appropriate extension (e.g.,.yml,.yaml,.json,.toml, or.jsonc) [1][2]. If you use a leading dot for your main configuration file (e.g.,.lefthook.yml), you must also use a leading dot for the local configuration file (e.g.,.lefthook-local.yml) [1][4]. The local configuration can be used independently without a main configuration file, which is useful for setting up developer-specific workflows that are not shared with the rest of the team [1][2]. It is common practice to add the local configuration file to your global.gitignore [1][5].
Citations:
- 1: https://github.com/evilmartians/lefthook/blob/HEAD/docs/configuration.md
- 2: https://lefthook.dev/configuration/
- 3: https://github.com/evilmartians/lefthook/blob/master/docs/configuration.md
- 4: https://github.com/evilmartians/lefthook/blob/v1.7.15/docs/configuration.md
- 5: https://lefthook.dev/examples/lefthook-local/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files and relevant symbols ---'
git ls-files | rg '(^|/)(check\.rs|Cargo\.toml|.*test.*|README.*)$' | head -80
rg -n -C 8 'lefthook\.yml|installed\(|let path = root\.join|found' src tests 2>/dev/null || true
printf '%s\n' '--- source outline ---'
ast-grep outline src/check.rs 2>/dev/null || trueRepository: HackingGate/uphold
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- src/check.rs structure and target ---'
ast-grep outline src/check.rs 2>/dev/null | head -120
sed -n '1,360p' src/check.rs
printf '%s\n' '--- check call sites and configuration loading ---'
rg -n -C 6 'check::|installed|lefthook|config::load|load\(' src tests -g '*.rs' -g '*.toml' -g '*.yml' -g '*.yaml' | head -300Repository: HackingGate/uphold
Length of output: 34288
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- remaining check flow ---'
sed -n '350,700p' src/check.rs
printf '%s\n' '--- focused check tests and fixtures ---'
sed -n '1,240p' tests/check_cli.rs
sed -n '400,540p' tests/check_cli.rs
rg -n -C 4 'lefthook(-local)?\.(yml|yaml)|\.lefthook(-local)?\.(yml|yaml)|config file|configuration file' README.md docs src tests .github 2>/dev/null | head -240Repository: HackingGate/uphold
Length of output: 38327
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
source = Path("src/check.rs").read_text()
pins = Path("src/pins.rs").read_text()
start = source.index("fn lefthook_seams")
end = source.index("\n/// The command names a lefthook config defines", start)
body = source[start:end]
joined_candidates = re.findall(r'root\.join\("([^"]+)"\)', body)
has_early_absent_return = bool(re.search(
r'if !path\.is_file\(\) \{\s*return Ok\(found\);\s*\}',
body,
re.S,
))
supported_main_names = re.findall(
r'^\s*"((?:\.?lefthook)\.(?:yml|yaml))",\s*$',
pins,
re.M,
)
print("lefthook_seams root.join candidates:", joined_candidates)
print("missing candidate returns empty Installed:", has_early_absent_return)
print("pins.rs main Lefthook names:", supported_main_names)
assert joined_candidates == ["lefthook.yml"]
assert has_early_absent_return
assert set(supported_main_names) >= {
"lefthook.yml", "lefthook.yaml", ".lefthook.yml", ".lefthook.yaml"
}
PYRepository: HackingGate/uphold
Length of output: 388
Read supported Lefthook configuration names. lefthook_seams() checks only lefthook.yml and returns no seams when it is absent. It must also handle lefthook.yaml, dot-prefixed names, and -local variants. Otherwise, valid configurations under those names make installed() reject their claims with exit 1.
🤖 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/check.rs` around lines 274 - 277, Update lefthook_seams() to discover all
supported Lefthook configuration names, including lefthook.yml, lefthook.yaml,
their dot-prefixed forms, and -local variants, instead of returning early when
lefthook.yml is absent. Preserve seam detection and installed() behavior for
every valid configuration name.
| for (stage, body) in &config.stages { | ||
| // Only a name git knows is a stage; `remotes`, `colors` and the rest of | ||
| // lefthook's top-level keys are not. | ||
| if !guards.contains_key(stage.as_str()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare the stages published by the manifest against the stages used in runner configs.
set -uo pipefail
# The stages this manifest publishes for guard hooks.
rg -n -C6 'guard' .pre-commit-hooks.yaml || true
# Stage keys written in the repository's own lefthook configurations.
fd -i 'lefthook' -e yml -e yaml --exec rg -n '^[a-z][a-z-]*:' {} \;Repository: HackingGate/uphold
Length of output: 5455
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/check.rs structure ---'
ast-grep outline src/check.rs
printf '%s\n' '--- relevant symbols and call sites ---'
rg -n -C8 'published|lefthook_commands|guards|config\.stages|uphold guard|local' src/check.rs src
printf '%s\n' '--- target implementation ---'
sed -n '230,390p' src/check.rs
printf '%s\n' '--- manifest and runner configuration files ---'
git ls-files | rg '(^|/)(lefthook|\.pre-commit-hooks)(\.ya?ml)?$|lefthook'Repository: HackingGate/uphold
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- check.rs target implementation ---'
sed -n '245,385p' src/check.rs
printf '%s\n' '--- relevant configuration files ---'
git ls-files | rg '(^|/)(lefthook\.ya?ml|\.pre-commit-hooks\.yaml|README\.md|.*consumer.*)$'
printf '%s\n' '--- lefthook stage declarations ---'
while IFS= read -r file; do
printf '%s\n' "--- $file"
rg -n -C2 '^(pre-commit|commit-msg|pre-merge-commit|pre-push|manual|uphold-manual|[A-Za-z0-9_-]+):' "$file" || true
done < <(git ls-files | rg '(^|/)lefthook\.ya?ml$')
printf '%s\n' '--- stage terminology and hook-name references ---'
rg -n -C4 'git stage|git hook|hook name|pre-commit|commit-msg|pre-merge-commit|pre-push|manual|uphold-manual' README.md docs src .github 2>/dev/null | head -n 300Repository: HackingGate/uphold
Length of output: 5912
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository lefthook configurations ---'
for file in lefthook.yml hooks/lefthook.yml; do
if test -f "$file"; then
printf '%s\n' "--- $file"
cat -n "$file"
fi
done
printf '%s\n' '--- manifest guard stages ---'
python3 - <<'PY'
import re
from pathlib import Path
text = Path(".pre-commit-hooks.yaml").read_text()
current_entry = None
current_stages = []
for line in text.splitlines():
m = re.match(r"\s*entry:\s*(.*)$", line)
if m:
current_entry = m.group(1)
current_stages = []
continue
m = re.match(r"\s*stages:\s*\[(.*)\]\s*$", line)
if m and current_entry and re.search(r"\bguard\b", current_entry):
stages = [x.strip().strip("'\"") for x in m.group(1).split(",")]
print(f"{current_entry}: {stages}")
PY
printf '%s\n' '--- stage keys in both lefthook configurations ---'
python3 - <<'PY'
import re
from pathlib import Path
for name in ("lefthook.yml", "hooks/lefthook.yml"):
path = Path(name)
if not path.is_file():
continue
keys = []
for line in path.read_text().splitlines():
m = re.match(r"^([A-Za-z0-9_-]+):\s*$", line)
if m and m.group(1) not in {"remotes", "colors", "skip_output", "extends", "assert_lefthook_version"}:
keys.append(m.group(1))
print(f"{name}: {keys}")
PYRepository: HackingGate/uphold
Length of output: 10915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- policy stages and guard consumers ---'
rg -n -C6 'guard::stages|hooks\s*=|pre-commit|commit-msg|pre-merge-commit|pre-push|manual' policy src tests scripts README.md 2>/dev/null | head -n 400
printf '%s\n' '--- tests for lefthook detection ---'
rg -n -C8 'lefthook|published|commands|manual|uphold-manual|guards' tests src/check.rs | head -n 500
printf '%s\n' '--- behavioral model of the two guarded loops ---'
python3 - <<'PY'
from pathlib import Path
import re
manifest = Path(".pre-commit-hooks.yaml").read_text()
guards = {}
entry = None
for line in manifest.splitlines():
m = re.match(r"\s*entry:\s*(.*)$", line)
if m:
entry = m.group(1)
continue
m = re.match(r"\s*stages:\s*\[(.*)\]\s*$", line)
if m and entry and re.search(r"\bguard\b", entry):
for stage in (x.strip().strip("'\"") for x in m.group(1).split(",")):
guards.setdefault(stage, entry)
for filename in ("lefthook.yml", "hooks/lefthook.yml"):
text = Path(filename).read_text()
stages = [
m.group(1)
for m in re.finditer(r"^([A-Za-z0-9_-]+):\s*$", text, re.MULTILINE)
if m.group(1) not in {"remotes", "colors", "skip_output", "extends", "assert_lefthook_version"}
]
skipped = [stage for stage in stages if stage not in guards]
print(f"{filename}")
print(f" published guard stages: {sorted(guards)}")
print(f" stage keys: {stages}")
print(f" skipped by both loops: {skipped}")
PYRepository: HackingGate/uphold
Length of output: 50376
Separate Lefthook stage keys from guard stages. guards contains only manifest stages, but lefthook.yml uses uphold-manual to run uphold guard --stage manual. Both loops skip that key. The manual guard is not added to Installed.stages, and its command names are omitted from Installed.local. Parse the guard's --stage argument and collect commands from valid Lefthook stage/group keys without using guards as the stage-key filter.
🤖 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/check.rs` around lines 283 - 288, Update the stage-processing loop in the
relevant check flow to recognize Lefthook stage/group keys independently of the
manifest-only guards map, including uphold-manual. Parse each guard command’s
--stage argument, add the referenced manual guard to Installed.stages, and
collect its command names into Installed.local only when the stage/group key is
valid; do not use guards as the stage-key filter.
| for run in runs_in(body) { | ||
| let words: Vec<&str> = run.split_whitespace().collect(); | ||
| // The subcommand, and the word before it. Matched on the | ||
| // SUBCOMMAND and not the executable: this repository runs its own | ||
| // binary out of the tree with `cargo run -- scan`, a consumer runs | ||
| // `uphold scan` from PATH, and a third by absolute path. All three | ||
| // are the same seam, and a pattern anchored on the program name | ||
| // recognised only the middle one. | ||
| let Some((before, subcommand)) = words.windows(2).find_map(|pair| match pair { | ||
| [before, word @ ("scan" | "guard")] => Some((*before, *word)), | ||
| _ => None, | ||
| }) else { | ||
| continue; | ||
| }; | ||
| if !(before.ends_with("uphold") || before == "--") { | ||
| continue; | ||
| } | ||
| direct = true; | ||
| if subcommand == "scan" && !words.contains(&"--text") { | ||
| found.scan = true; | ||
| } else if subcommand == "guard" { | ||
| found.stages.insert(stage.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
find_map reads only the first seam in a run: string.
words.windows(2).find_map(...) stops at the first scan or guard word. A single command that drives both seams registers only the first one. Example: run: uphold scan && uphold guard. The guard stage is then absent from found.stages, and a claim on a guard rule fails with exit 1 while the seam is installed.
Collect every match instead of the first.
🐛 Proposed fix
let words: Vec<&str> = run.split_whitespace().collect();
- let Some((before, subcommand)) = words.windows(2).find_map(|pair| match pair {
- [before, word @ ("scan" | "guard")] => Some((*before, *word)),
- _ => None,
- }) else {
- continue;
- };
- if !(before.ends_with("uphold") || before == "--") {
- continue;
- }
- direct = true;
- if subcommand == "scan" && !words.contains(&"--text") {
- found.scan = true;
- } else if subcommand == "guard" {
- found.stages.insert(stage.clone());
- }
+ for pair in words.windows(2) {
+ let [before, subcommand @ ("scan" | "guard")] = pair else {
+ continue;
+ };
+ if !(before.ends_with("uphold") || *before == "--") {
+ continue;
+ }
+ direct = true;
+ if *subcommand == "scan" && !words.contains(&"--text") {
+ found.scan = true;
+ } else if *subcommand == "guard" {
+ found.stages.insert(stage.clone());
+ }
+ }📝 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.
| for run in runs_in(body) { | |
| let words: Vec<&str> = run.split_whitespace().collect(); | |
| // The subcommand, and the word before it. Matched on the | |
| // SUBCOMMAND and not the executable: this repository runs its own | |
| // binary out of the tree with `cargo run -- scan`, a consumer runs | |
| // `uphold scan` from PATH, and a third by absolute path. All three | |
| // are the same seam, and a pattern anchored on the program name | |
| // recognised only the middle one. | |
| let Some((before, subcommand)) = words.windows(2).find_map(|pair| match pair { | |
| [before, word @ ("scan" | "guard")] => Some((*before, *word)), | |
| _ => None, | |
| }) else { | |
| continue; | |
| }; | |
| if !(before.ends_with("uphold") || before == "--") { | |
| continue; | |
| } | |
| direct = true; | |
| if subcommand == "scan" && !words.contains(&"--text") { | |
| found.scan = true; | |
| } else if subcommand == "guard" { | |
| found.stages.insert(stage.clone()); | |
| } | |
| } | |
| for run in runs_in(body) { | |
| let words: Vec<&str> = run.split_whitespace().collect(); | |
| // The subcommand, and the word before it. Matched on the | |
| // SUBCOMMAND and not the executable: this repository runs its own | |
| // binary out of the tree with `cargo run -- scan`, a consumer runs | |
| // `uphold scan` from PATH, and a third by absolute path. All three | |
| // are the same seam, and a pattern anchored on the program name | |
| // recognised only the middle one. | |
| for pair in words.windows(2) { | |
| let [before, subcommand @ ("scan" | "guard")] = pair else { | |
| continue; | |
| }; | |
| if !(before.ends_with("uphold") || *before == "--") { | |
| continue; | |
| } | |
| direct = true; | |
| if *subcommand == "scan" && !words.contains(&"--text") { | |
| found.scan = true; | |
| } else if *subcommand == "guard" { | |
| found.stages.insert(stage.clone()); | |
| } | |
| } | |
| } |
🤖 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/check.rs` around lines 289 - 312, Update the run-processing loop to
collect and handle every matching subcommand seam instead of stopping at the
first match from words.windows(2).find_map(...). Preserve the existing
validation for the preceding word and apply the scan and guard handling logic to
each matching pair, so commands such as “uphold scan && uphold guard” register
both seams.
| for (index, claim) in declaration.enforce.iter().enumerate() { | ||
| let at = format!("enforce[{index}]"); | ||
| if claim.tier.is_some() { | ||
| return Err(Fatal::at( | ||
| &path, | ||
| format!( | ||
| "{at} carries a `tier`. The field is gone: a rule id resolves across \ | ||
| every seam at once, so a claim naming one no longer has to say which. \ | ||
| Drop the line." | ||
| ), | ||
| )); | ||
| } | ||
| let (Some(principle), Some(rule)) = (claim.principle.as_deref(), claim.rule.as_deref()) | ||
| else { | ||
| return Err(Fatal::at( | ||
| &path, | ||
| format!("{at}: `principle` and `rule` are both required"), | ||
| )); | ||
| }; | ||
| if principle.trim().is_empty() || rule.trim().is_empty() { | ||
| return Err(Fatal::at( | ||
| &path, | ||
| format!("{at}: `principle` and `rule` must not be blank"), | ||
| )); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A leftover tier returns exit 2, which reads as could-not-look.
The doc comment at Lines 95-98 states that a leftover tier fails as a false claim. The code returns Fatal::at, which maps to exit 2. engine_suppliers in uphold_check.py (Lines 240-278) turns exit 2 into CouldNotLook. An author error in the declaration is then reported as an inability to inspect the repository, not as a refused claim.
The same applies to the missing-field and blank-field branches. If exit 2 is intended for a malformed declaration, state that in the doc comment at Lines 95-98. If a leftover tier is intended to be a refused claim, push it onto failures and continue.
🤖 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/check.rs` around lines 539 - 563, Update the validation branches in the
declaration enforcement loop to classify leftover tier, missing principle/rule,
and blank-field cases as refused claims rather than fatal inspection errors:
record each claim in failures and continue processing, unless the documented
contract explicitly defines malformed declarations as exit 2. If retaining
Fatal::at, revise the related doc comment to state that malformed declarations
produce that exit classification.
| if !failures.is_empty() { | ||
| eprintln!("enforcement claims refused ({DECLARATION}):"); | ||
| for failure in &failures { | ||
| eprintln!("- {failure}"); | ||
| } | ||
| return Ok(Exit::Violations); | ||
| } | ||
|
|
||
| println!("reconciled {} enforcement claims:", evidence.len()); | ||
| for line in &evidence { | ||
| println!(" {line}"); | ||
| } | ||
| for note in &installed.how { | ||
| println!(" note {note}"); | ||
| } | ||
| Ok(Exit::Clean) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The success path hides installed.unreadable.
run() prints installed.how but never installed.unreadable. If every claim resolves and one runner configuration could not be read, the command exits 0 and reports nothing about the configuration it could not inspect. The coverage path reports the same holes as could not look at Lines 694-699.
Print the unreadable notes here as well.
🐛 Proposed fix
for note in &installed.how {
println!(" note {note}");
}
+ for note in &installed.unreadable {
+ println!(" could not look {note}");
+ }
Ok(Exit::Clean)📝 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.
| if !failures.is_empty() { | |
| eprintln!("enforcement claims refused ({DECLARATION}):"); | |
| for failure in &failures { | |
| eprintln!("- {failure}"); | |
| } | |
| return Ok(Exit::Violations); | |
| } | |
| println!("reconciled {} enforcement claims:", evidence.len()); | |
| for line in &evidence { | |
| println!(" {line}"); | |
| } | |
| for note in &installed.how { | |
| println!(" note {note}"); | |
| } | |
| Ok(Exit::Clean) | |
| if !failures.is_empty() { | |
| eprintln!("enforcement claims refused ({DECLARATION}):"); | |
| for failure in &failures { | |
| eprintln!("- {failure}"); | |
| } | |
| return Ok(Exit::Violations); | |
| } | |
| println!("reconciled {} enforcement claims:", evidence.len()); | |
| for line in &evidence { | |
| println!(" {line}"); | |
| } | |
| for note in &installed.how { | |
| println!(" note {note}"); | |
| } | |
| for note in &installed.unreadable { | |
| println!(" could not look {note}"); | |
| } | |
| Ok(Exit::Clean) |
🤖 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/check.rs` around lines 610 - 625, Update the success path in run() to
print every entry in installed.unreadable alongside the existing installed.how
notes, using the same “could not look” reporting style as the coverage path
while preserving the clean exit behavior.
| #[test] | ||
| fn the_starter_declaration_is_valid_and_enforces_nothing() { | ||
| let root = workspace(); | ||
| write(&root, "policy/principles.toml", GUARD_POLICY); | ||
| write(&root, ".pre-commit-config.yaml", PRE_COMMIT); | ||
| let starter = Command::new("python3") | ||
| .args([ | ||
| Path::new(env!("CARGO_MANIFEST_DIR")) | ||
| .join("uphold_check.py") | ||
| .to_str() | ||
| .unwrap(), | ||
| "--init", | ||
| ]) | ||
| .output() | ||
| .unwrap(); | ||
| assert_eq!(starter.status.code().unwrap(), 0); | ||
| write(&root, "policy/upheld.toml", &stdout(&starter)); | ||
| let output = check(&root, &[]); | ||
| assert_eq!(code(&output), 0, "{}{}", stdout(&output), stderr(&output)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This test fails, rather than skips, where python3 is absent.
Command::new("python3") returns Err when no interpreter is on PATH, and .unwrap() turns that into a panic. The Python suite added needs_the_engine precisely so a missing toolchain reports a skip and not a red suite; this test does the reverse for the Python toolchain.
Either embed the starter declaration as a constant in this file, or skip when the interpreter is missing.
🧪 Proposed fix
- let starter = Command::new("python3")
+ let Ok(starter) = Command::new("python3")
.args([
Path::new(env!("CARGO_MANIFEST_DIR"))
.join("uphold_check.py")
.to_str()
.unwrap(),
"--init",
])
.output()
- .unwrap();
+ else {
+ // No interpreter here. tests/test_uphold_check.py asserts --init where one exists.
+ return;
+ };📝 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.
| #[test] | |
| fn the_starter_declaration_is_valid_and_enforces_nothing() { | |
| let root = workspace(); | |
| write(&root, "policy/principles.toml", GUARD_POLICY); | |
| write(&root, ".pre-commit-config.yaml", PRE_COMMIT); | |
| let starter = Command::new("python3") | |
| .args([ | |
| Path::new(env!("CARGO_MANIFEST_DIR")) | |
| .join("uphold_check.py") | |
| .to_str() | |
| .unwrap(), | |
| "--init", | |
| ]) | |
| .output() | |
| .unwrap(); | |
| assert_eq!(starter.status.code().unwrap(), 0); | |
| write(&root, "policy/upheld.toml", &stdout(&starter)); | |
| let output = check(&root, &[]); | |
| assert_eq!(code(&output), 0, "{}{}", stdout(&output), stderr(&output)); | |
| } | |
| #[test] | |
| fn the_starter_declaration_is_valid_and_enforces_nothing() { | |
| let root = workspace(); | |
| write(&root, "policy/principles.toml", GUARD_POLICY); | |
| write(&root, ".pre-commit-config.yaml", PRE_COMMIT); | |
| let Ok(starter) = Command::new("python3") | |
| .args([ | |
| Path::new(env!("CARGO_MANIFEST_DIR")) | |
| .join("uphold_check.py") | |
| .to_str() | |
| .unwrap(), | |
| "--init", | |
| ]) | |
| .output() | |
| else { | |
| // No interpreter here. tests/test_uphold_check.py asserts --init where one exists. | |
| return; | |
| }; | |
| assert_eq!(starter.status.code().unwrap(), 0); | |
| write(&root, "policy/upheld.toml", &stdout(&starter)); | |
| let output = check(&root, &[]); | |
| assert_eq!(code(&output), 0, "{}{}", stdout(&output), stderr(&output)); | |
| } |
🤖 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/check_cli.rs` around lines 672 - 691, Update
the_starter_declaration_is_valid_and_enforces_nothing so a missing python3
interpreter skips the test instead of panicking on Command::output().unwrap();
handle the Command construction/execution error using the test suite’s
established skip mechanism, while preserving the existing assertions when
python3 is available.
| def needs_the_engine(test): | ||
| """Skip where neither a built binary nor cargo can answer. | ||
|
|
||
| The same reason `test_the_two_readers_of_the_policy_agree` gives: a test | ||
| that needs a `cargo build` to be meaningful must not report a red suite to | ||
| somebody who has not run one, and the catalog job runs on an image with no | ||
| Rust toolchain by design. What is skipped here is asserted in | ||
| tests/check_cli.rs, which runs where a toolchain exists. | ||
| """ | ||
| try: | ||
| uphold_check.engine(ROOT, "--version") | ||
| except uphold_check.CouldNotLook as error: | ||
| return unittest.skip(str(error))(test) | ||
| return test |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The probe ignores the exit code, so it does not prove the engine can answer.
uphold_check.engine raises CouldNotLook only when every attempt fails to start. It returns the first CompletedProcess otherwise, without inspecting returncode. An uphold on PATH that is a different or older program returns non-zero and the decorator still marks the test as runnable. The test then fails where the intent is to skip.
Check the exit code as well.
🧪 Proposed fix
try:
- uphold_check.engine(ROOT, "--version")
+ answered = uphold_check.engine(ROOT, "--version")
except uphold_check.CouldNotLook as error:
return unittest.skip(str(error))(test)
+ if answered.returncode != 0:
+ return unittest.skip(f"the engine could not answer: {answered.stderr.strip()}")(
+ test
+ )
return test📝 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.
| def needs_the_engine(test): | |
| """Skip where neither a built binary nor cargo can answer. | |
| The same reason `test_the_two_readers_of_the_policy_agree` gives: a test | |
| that needs a `cargo build` to be meaningful must not report a red suite to | |
| somebody who has not run one, and the catalog job runs on an image with no | |
| Rust toolchain by design. What is skipped here is asserted in | |
| tests/check_cli.rs, which runs where a toolchain exists. | |
| """ | |
| try: | |
| uphold_check.engine(ROOT, "--version") | |
| except uphold_check.CouldNotLook as error: | |
| return unittest.skip(str(error))(test) | |
| return test | |
| def needs_the_engine(test): | |
| """Skip where neither a built binary nor cargo can answer. | |
| The same reason `test_the_two_readers_of_the_policy_agree` gives: a test | |
| that needs a `cargo build` to be meaningful must not report a red suite to | |
| somebody who has not run one, and the catalog job runs on an image with no | |
| Rust toolchain by design. What is skipped here is asserted in | |
| tests/check_cli.rs, which runs where a toolchain exists. | |
| """ | |
| try: | |
| answered = uphold_check.engine(ROOT, "--version") | |
| except uphold_check.CouldNotLook as error: | |
| return unittest.skip(str(error))(test) | |
| if answered.returncode != 0: | |
| return unittest.skip(f"the engine could not answer: {answered.stderr.strip()}")( | |
| test | |
| ) | |
| return test |
🤖 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/test_review.py` around lines 30 - 43, Update needs_the_engine to
inspect the CompletedProcess returned by uphold_check.engine(ROOT, "--version")
and skip the test when its returncode is non-zero, while preserving the existing
CouldNotLook skip behavior and allowing execution only when the probe succeeds.
| @needs_the_engine | ||
| class NoProseInRuntime(unittest.TestCase): | ||
| """`enforcement-needs-a-trigger`: the tool must not carry principle text.""" | ||
|
|
||
| def test_output_contains_no_record_prose(self): | ||
| result = run(ROOT) | ||
| def test_the_catalog_modes_carry_no_record_prose_into_a_report(self): | ||
| # `--list` is the mode a runtime is most likely to pipe somewhere. The | ||
| # reconcile's half of this invariant moved with the reconcile and is | ||
| # asserted in tests/check_cli.rs. | ||
| result = run(ROOT, "--list") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The class-level skip removes a test that needs no engine.
test_the_catalog_modes_carry_no_record_prose_into_a_report runs --list. That mode reads the catalog only and never calls the binary. @needs_the_engine on the class skips it on an image with no Rust toolchain, which the docstring says is the image the catalog job uses. The one assertion that must hold there is then never made.
Move the decorator onto the two methods that run --review.
🤖 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/test_uphold_check.py` around lines 137 - 145, Remove the class-level
needs_the_engine decorator from NoProseInRuntime and apply it only to the two
test methods that invoke --review. Leave
test_the_catalog_modes_carry_no_record_prose_into_a_report undecorated so the
--list catalog assertion runs without a Rust toolchain.
| def engine_suppliers(root: Path, *, strict: bool = True) -> dict[str, list[str]]: | ||
| """Which seams supply each rule, as the reconcile in the binary sees it. | ||
|
|
||
| claimable = { | ||
| record_id: record | ||
| for record_id, record in records.items() | ||
| if record.get("status") != "deprecated" | ||
| and record.get("enforcement", {}).get("automatable") != "no" | ||
| } | ||
| # Intersected with what is SUPPLIED, not merely with what was claimed. The | ||
| # orphans printed immediately above are claims naming a rule no seam here | ||
| # runs, and counting them here put them back into the numerator of the one | ||
| # number a reader takes away -- a record counted as claimed by a rule that | ||
| # this very report has just said does not exist. | ||
| enforced = {principle for principle, rule in claims if rule in supplied} & set( | ||
| claimable | ||
| ) | ||
| unclaimable = len(records) - len(claimable) | ||
| lines.append("") | ||
| lines.append( | ||
| f"records: {len(enforced)} of {len(claimable)} claimable records are " | ||
| f"claimed by a rule here" | ||
| ) | ||
| if unclaimable: | ||
| lines.append( | ||
| f" {unclaimable} record(s) are deprecated or declare " | ||
| f'enforcement.automatable = "no" and can never be claimed' | ||
| `strict` is the difference between the two callers. `--oscal` publishes an | ||
| assertion to an outside reader, so a declaration that does not reconcile has | ||
| nothing honest to export and the refusal travels. `--review` runs over a | ||
| declaration somebody is still writing: a claim naming a rule nothing | ||
| supplies is exactly the state it exists to help with, and refusing there | ||
| would take the review document away at the moment it is most wanted. The | ||
| evidence lines for the claims that DID hold are on stdout either way. | ||
| """ | ||
| answered = engine(root, "check") | ||
| if answered.returncode == 2: | ||
| raise CouldNotLook(answered.stderr.strip() or "uphold check could not look") | ||
| if answered.returncode != 0 and strict: | ||
| raise Refused( | ||
| "the enforcement claims do not reconcile, so there is nothing " | ||
| f"honest to export:\n{answered.stderr.strip()}" | ||
| ) | ||
| lines.append( | ||
| " an unclaimed record is not a gap to close by writing a claim: a claim " | ||
| "without a rule behind it is the failure `enforcement-needs-a-trigger` names" | ||
| ) | ||
| return lines, status | ||
| suppliers: dict[str, list[str]] = {} | ||
| for line in answered.stdout.splitlines(): | ||
| if " <- " not in line or "enforced by" not in line: | ||
| continue | ||
| _, rest = line.split(" <- ", 1) | ||
| rule, by = rest.split(" enforced by ", 1) | ||
| # Folded back to the SEAM, because an OSCAL component is a thing that | ||
| # implements a control and the seam is that thing. `uphold check` names | ||
| # the evidence -- which stage, which scan -- and a component per stage | ||
| # would split one implementation across five. | ||
| seams = [] | ||
| for part in by.split(","): | ||
| part = part.strip() | ||
| if not part: | ||
| continue | ||
| seams.append("local" if part.startswith("a hook") else "uphold") | ||
| for seam in seams: | ||
| if seam not in suppliers.setdefault(rule.strip(), []): | ||
| suppliers[rule.strip()].append(seam) | ||
| return suppliers |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
--review loses every supplied rule when one claim is false.
The docstring states that evidence lines reach stdout either way. src/check.rs::run does not do that. When failures is non-empty, it writes the refusals to stderr and returns Exit::Violations before the reconciled ... block that prints the {principle} <- {rule} enforced by ... lines. So on exit 1 stdout carries no evidence.
With strict=False, this function then returns an empty suppliers map. run_review computes claimed and active from that map, so a declaration with one false claim drops every valid claim from the review document as well. That is the exact failure the comment at Lines 590-601 says the filter exists to prevent.
Make the engine emit evidence for the claims that held even when others fail, or have --review reconcile per claim instead of relying on the exit-0 stdout block.
🤖 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 `@uphold_check.py` around lines 240 - 278, Update engine_suppliers so
strict=False preserves evidence for claims that reconcile even when other claims
fail; either make the underlying engine output successful claim evidence on
violation exits or have engine_suppliers reconcile claims individually. Ensure
run_review still receives every valid rule in suppliers while strict=True
continues refusing unreconciled enforcement results.
… path The port asked "does this url name us", and most git urls cannot answer it. lefthook takes any of them, and `scripts/consumer_check.sh` clones this repository to a neutral `$WORK/hooks` on purpose -- so the url its consumer writes carries neither the owner nor the repository name, every guard went unrecognised, and a true claim was refused. Answering exit 1 there says the claim is FALSE about a repository whose only fault is cloning from a path. The question is the other way round, as it was before the port: a remote is rejected only when it spells a forge `owner/name` and that pair is not ours. Anything without a host is a path, and a path is unidentifiable rather than foreign. Both spellings git accepts for a host are read. The load-bearing half is untouched: the remote and `hooks/lefthook.yml` must appear in the SAME entry, so a fork pinning its own config is still not credited with running every guard here.
2 participants
Supersedes the earlier version of this branch, which patched the seam derivation
inside
uphold_check.py. Those lines are gone instead.The cause
uphold_check.pyre-implementedconfig::loadto reconcile a declaration:bundled sets,
inherit.paths,inherit.disabled_rules, and a repository's ownrule shadowing an inherited id.
effective_rules_commandhas said since it waswritten what that costs:
They disagreed about the seam a hookless rule runs at.
files.*is the scan's,command.beforeis a checker standing in front of a command, and both come backwith no git hooks. The reconciler read the second as the first, so a claim on
a shim-only rule reconciled green, exit 0, in a repository that pins
uphold-scanand nothing else — over a rule the scan never touches. Thisrepository declares two such rules.
test_the_two_readers_of_the_policy_agreecould not catch it: it comparedgit_hooks, and both readers said[]. Two implementations agreeing is not thesame as either being right.
What changed
Rule::seamsreturnsscan/guard/shimfrom the same conditions thethree seams use to select rules, and it is on
rules --effective --jsonforanything else that has to ask.
The reconcile and the coverage report are
uphold checkanduphold check --coverage.uphold-checkbecomeslanguage: rustlike everyother id in the manifest — it was the only
language: scriptone, andpre-commit and prek key an environment on
(repo, language, version), so it nowshares the environment the other seven ids already build and costs no second
compile.
What stayed in Python is what never reads the policy:
--explain,--list,--review,--oscal,--init. A mode that cannot read the policy cannotdisagree with the loader.
--oscalgates on the reconcile so it asks the binaryand treats an unreachable one as could-not-look;
--reviewasks too butsurvives a refusal, because it runs over a declaration somebody is still
writing.
uphold_check.py: 1,589 → 680 lines.Three things the port fixes rather than carries
could take no dependency, which is how
configs:— the key README.md tellsevery lefthook consumer to write under
remotes:— was read as a command name.package.repositoryis read throughCARGO_PKG_REPOSITORYat compiletime, so the slug cannot drift from the crate and works outside a checkout of
this repository.
records: N of Mwascomputed from the claims, so a declaration whose only claim named a rule
nothing runs reported one record as claimed two lines under the line saying
that rule is supplied by nothing.
Verification
Both reconcilers were run against every repository in the fleet: 68 of 71
return the same exit code and the same claim count.
The three that differ have a policy
config::loadrefuses outright —command.beforeon abuiltin— which the Python never validated. Worthknowing separately:
uphold scanalready fails in those three today, andthey are unaffected only because the fleet pins
v1.0.0, which predates thatrefusal. Cutting the next tag exits 2 on every hook there.
cargo test— 294 tests green, including a newtests/check_cli.rs.tests/test_uphold_check.pyrather thangoing away, and two caught real regressions in the port: a lefthook remote
given as a filesystem path, which is what
scripts/consumer_check.shwrites,and the rule that a remote is only ours when ONE entry both names this
repository and takes its config.
python3 -m pytest tests/— 73 passed, 224 subtests.cargo clippy --all-targetsclean under the crate's own profile (which is whatturned two
panic!s into exit-2 paths);cargo fmt --checkclean; therepository's own hooks pass on every commit.
Summary by CodeRabbit
New Features
uphold checkcommand to validate policy claims, detect unsupported or unfulfilled rules, and report execution coverage.uphold check --coveragefor coverage summaries and seam diagnostics.--upstreamrepository URL option.Documentation
Chores