Repository navigation
An adopt entry in [decide] may carry the repository's why, and uphold check prints it - #455
Merged
Merged
Conversation
… check prints it (#454) An entry in `[decide] adopt` is a record id or a table `{ id, why }`, and both forms mix in one list. `uphold check` prints the table entry as `id: why`, as it already prints an order's reason. The why is the consumer's fact: the catalog says what a record holds, the consumer says why it rules by it. It stays optional, where an order's is required. At load, exit 2 refuses a table with a key other than `id` and `why`, a blank `why`, and a `why` equal to the record's `summary` or `claim`. The JSON report keeps each entry in the form it was written, so a string entry is still a string there. The schema, REFERENCE.md and the design-ruling skill describe the form; the skill reads the why as context, never as a citation. Closes #454 Claude-Session: https://claude.ai/code/session_01BHpLckRt4BNGCdFG9vLyEP
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 2 minutes. View limit 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 |
uphold-reviewWalkthroughThis push moves the check that refuses an adoption I re-judged the open separation-of-concerns thread on src/config.rs and it is resolved. All checks against the catalog in Changes
❌ uphold rules: uphold's rules refuse this · this repository's policy at 72b4d98Rule output✅ uphold reviewThe review document asked about 33 of uphold's records; this push violates none of them. This reviewed the push e4a2056..5958736: a new thread lies in a line the push added, and each open thread was judged again at this head. Use Full review on the uphold review check to review the whole change again. Reviewed with uphold 1.38.0 by uphold-review 0.4.1 · 5958736 · claude-opus-5-5 · 33 s · $0.11 list |
![uphold-review[bot]](https://avatars.githubusercontent.com/u/8541644?s=60&v=4){kind=small}
Comment on lines
+846
to
+857
| // An id the catalog lacks is `uphold check`'s to report, at exit 1. | ||
| if let Some(record) = crate::catalog::get(id).map_err(|error| error.to_string())? { | ||
| for (field, text) in [("summary", &record.summary), ("claim", &record.claim)] { | ||
| if why.trim() == text.trim() { | ||
| return Err(format!( | ||
| "`[decide] adopt` entry {id:?} has a `why` that is the record's \ | ||
| `{field}`. The catalog already says what the record holds; `why` \ | ||
| says why this repository adopts it" | ||
| )); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Separation of Concerns · separation-of-concerns · level review
uphold's claim and questions
Organize a system so distinct concerns can be understood, changed, tested, and governed independently where their real dependencies permit it.
- What distinct reasons would cause this component to change?
- Which concern owns each invariant?
- Does the proposed boundary reduce or merely relocate coupling?
Decide states how the work is divided (config.rs lines 728-730): the loader refuses the table's shape, and questions about the catalog this binary carries are answered by uphold check at exit 1 (decided in src/check.rs). The new check at lines 846-857 asks a catalog question in the loader: it fetches the record and compares the why against its summary and claim. The record's question is 'Which concern owns each invariant?' This change gives catalog-dependent [decide] checks two owners. Unknown and deprecated ids stay in decided; the copied-text check moves into Decide::validate. As a result, a change to a record's wording in the catalog now decides whether every command that reads the policy fails at load with exit 2, not just whether uphold check reports a refusal. The doc comment at 728-730 was not updated and now describes a split the code no longer follows.
🤖 Prompt for AI agents
In src/config.rs, `Decide::validate` (lines 846-857) looks each adopted record up with `crate::catalog::get` and refuses a `why` equal to the record's `summary` or `claim`. That contradicts the `Decide` doc comment at lines 728-730, which says catalog questions belong to `uphold check` at exit 1. Move the summary/claim comparison out of config.rs and into `decided` in src/check.rs, where the adopted record is already fetched. Push a failure such as `adopt: {id:?} has a `why` that is the record's `summary`...` onto `failures`, so it is reported with the other `[decide]` refusals at exit 1. Keep the blank-`why` and unknown-key refusals at load, because those are shape checks. Then update docs/REFERENCE.md (the exit-2 paragraph near the end of the `[decide]` section) and the `Reasoned::why` doc comment and schema description to match. Finally, change the summary and claim cases in `an_adopt_entry_of_the_wrong_shape_is_two_and_named` in tests/check_cli.rs to expect exit 1 and the refusal text from `uphold check`, or move them into a separate test.
There was a problem hiding this comment.
No longer violated at 5958736. Resolve this thread when you agree.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #455 +/- ##
=======================================
Coverage 94.97% 94.98%
=======================================
Files 56 56
Lines 28500 28550 +50
=======================================
+ Hits 27068 27118 +50
Misses 1432 1432 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… record Whether a `why` repeats a record's `summary` or `claim` is a question about the catalog this binary carries. `Decide`'s own comment gives those to `uphold check` at exit 1, as it does an unknown or deprecated id, so a catalog rewording no longer decides whether every command can load the policy. The loader keeps the shape refusals: a blank `why`, an unknown key, and an entry that is neither an id nor a table, now tested too. Claude-Session: https://claude.ai/code/session_01BHpLckRt4BNGCdFG9vLyEP
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #454.
An entry in
[decide] adoptmay be a table{ id, why }beside plain ids, anduphold checkprints it asid: why.why, and an entry that is neither an id nor a table.uphold check, exit 1: awhyequal to the record'ssummaryorclaim. The issue placed this at exit 2. It moved because it is a catalog question, andDecide's comment gives those touphold check, as with an unknown id. A catalog rewording therefore cannot stop every command from loading the policy./decide/adopt.[decide]section, and the design-ruling skill's step 1, which reads the why as context and never as a citation.The table form is deserialized by hand rather than with
#[serde(untagged)], because untagged reports only "did not match any variant" and the issue asks that the bad entry be named.Done-when checks:
tests/check_cli.rscovers both forms printed and in JSON, one test covers the three load refusals, and one covers the summary and claim copies at exit 1. The schema snapshot test and the existing decide tests pass unchanged.https://claude.ai/code/session_01BHpLckRt4BNGCdFG9vLyEP