feat(cli): enforce ADR writing order with structural gates#10
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (15)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR implements ADR writing order governance gates that enforce completeness validation at two lifecycle points: when setting an ADR's decision field (requires at least 2 alternatives with both accepted and rejected statuses), and when accepting an ADR. Both gates support a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tests/test_adr_gates.rs (1)
66-109: Add the symmetric “no accepted alternative” failure case.You already test “no rejected”; adding a case with no accepted alternative would fully lock the invariant matrix for both
adr set ... decisionandadr accept.Also applies to: 178-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_adr_gates.rs` around lines 66 - 109, Add a symmetric test that verifies setting a decision fails when there are alternatives but none are accepted: copy the existing test_set_decision_blocked_without_rejected and create test_set_decision_blocked_without_accepted which creates two alternatives, marks both as "rejected" via the same tick commands (use -s rejected for both indices), runs adr set ADR-0001 decision ..., and asserts the output contains "alternatives incomplete" and snapshot; also add the analogous symmetric test for the acceptance gate (the test near the other block around the accept-related tests) to cover the "no accepted alternative" failing case for adr accept/accept-related commands so both gate invariants are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 14: Update the changelog bullet to precisely state that the --force flag
is attached to the adr accept command rather than both gates; e.g., change "Both
gates bypassable with --force" to wording like "adr accept supports --force to
bypass gates" (mention the specific command symbol adr accept and the flag
--force) so the entry matches the PR behavior.
In `@gov/adr/ADR-0042-enforce-adr-writing-order-with-structural-gates.toml`:
- Around line 41-45: Update ADR-0042 wording so the gate contract matches the
CLI: remove the blanket statement that acceptance requires non-empty `context`,
`decision`, and `consequences` and instead state that acceptance is
alternatives-based (minimum alternative count and evaluated-alternatives rules
are the deciding criteria per the `adr-writer` skill). Also clarify that the
`--force` flag is exposed on the `adr accept` command to bypass the write-time
gate (rather than implicitly bypassing both gates), and avoid restating
implementation details; reference the `adr-writer` ordering and the
minimum-deliberation threshold as guidance.
In `@gov/work/2026-04-14-implement-adr-writing-order-gates.toml`:
- Line 18: The journal entry (content.journal) says "all tests pass" but the
test-status field "Unit tests for both gates" remains "pending", so update the
test-status entry to reflect success (e.g., change "Unit tests for both gates"
from pending to passed/complete) or, alternatively, change content.journal to
match the pending state; locate the fields named content.journal and the
test-status entry "Unit tests for both gates" in the TOML (including the related
entries at the other occurrence on lines 36-38) and make them consistent.
- Line 13: The description currently claims "Both gates support --force", which
is incorrect and conflicts with the PR that only exposes --force on the adr
accept path; update the description text to state that only the Lifecycle gate
(used by the "adr accept" command) supports a --force override for historical
backfills, and explicitly note the Write-time gate blocks setting decision until
alternatives are complete and is not bypassable; make the same correction in the
other occurrences of this wording (the description and the repeat on lines
referencing the Write-time gate and Lifecycle gate).
In `@src/cmd/edit/mod.rs`:
- Around line 437-440: Update the comment above the invariant gate to cite the
specific RFC clause that authorizes this lifecycle/edit check in addition to
ADR-0042: edit the line starting with "// Implements [[ADR-0042]]: block setting
`decision` without complete alternatives" to include the RFC identifier and
clause number (e.g., "RFC-XXXX §Y.Z") and a short phrase linking it to the
normative invariant; keep the code using ArtifactType::Adr, fp.as_simple() ==
Some("decision"), and crate::cmd::lifecycle::validate_adr_completeness(config,
id)? unchanged.
- Around line 438-440: Add a regression test that mirrors
test_set_decision_blocked_without_alternatives but uses the legacy dotted path
"content.decision": call the command flow through plan_mutation_request() (so
path normalization occurs) and attempt `adr set <ID> content.decision` on an ADR
with incomplete alternatives, then assert that
crate::cmd::lifecycle::validate_adr_completeness blocks the operation (i.e., the
command returns the same failure as the non-dotted variant). Ensure the new test
invokes the same setup/fixtures as
test_set_decision_blocked_without_alternatives and checks for the same
error/exit condition to confirm the gate at ArtifactType::Adr + fp.as_simple()
== Some("decision") is enforced for the dotted legacy path.
In `@src/cmd/lifecycle.rs`:
- Around line 297-300: Update the doc comment for the function
validate_adr_completeness (and the related ADR comment at the other nearby
comment block referenced in the review) to append the exact normative RFC clause
identifier(s) that define the enforced invariants (requirement of at least 2
alternatives, and at least 1 accepted and 1 rejected). Do not paraphrase—insert
the official RFC clause IDs/section numbers (e.g., "RFC-XXXX, Section Y.Z" or
the precise clause tokens) into the comment so the code directly cites the
governing normative text; apply the same insertion to the second commented
invariant noted in the review.
---
Nitpick comments:
In `@tests/test_adr_gates.rs`:
- Around line 66-109: Add a symmetric test that verifies setting a decision
fails when there are alternatives but none are accepted: copy the existing
test_set_decision_blocked_without_rejected and create
test_set_decision_blocked_without_accepted which creates two alternatives, marks
both as "rejected" via the same tick commands (use -s rejected for both
indices), runs adr set ADR-0001 decision ..., and asserts the output contains
"alternatives incomplete" and snapshot; also add the analogous symmetric test
for the acceptance gate (the test near the other block around the accept-related
tests) to cover the "no accepted alternative" failing case for adr
accept/accept-related commands so both gate invariants are covered.
🪄 Autofix (Beta)
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
Run ID: a621dbfe-e137-4eaa-8059-f26346c45bc3
⛔ Files ignored due to path filters (12)
tests/snapshots/test_adr_gates__accept_blocked_with_only_one_alternative.snapis excluded by!**/*.snaptests/snapshots/test_adr_gates__accept_blocked_without_alternatives.snapis excluded by!**/*.snaptests/snapshots/test_adr_gates__accept_force_bypasses_gates.snapis excluded by!**/*.snaptests/snapshots/test_adr_gates__accept_succeeds_with_complete_adr.snapis excluded by!**/*.snaptests/snapshots/test_adr_gates__set_decision_blocked_with_only_one_alternative.snapis excluded by!**/*.snaptests/snapshots/test_adr_gates__set_decision_blocked_without_alternatives.snapis excluded by!**/*.snaptests/snapshots/test_adr_gates__set_decision_blocked_without_rejected.snapis excluded by!**/*.snaptests/snapshots/test_adr_gates__set_decision_succeeds_with_complete_alternatives.snapis excluded by!**/*.snaptests/snapshots/test_edit__adr_set_decision.snapis excluded by!**/*.snaptests/snapshots/test_edit__path_backward_compat.snapis excluded by!**/*.snaptests/snapshots/test_lifecycle__accept_already_accepted_fails.snapis excluded by!**/*.snaptests/snapshots/test_lifecycle__accept_proposed_adr.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
CHANGELOG.mdgov/adr/ADR-0042-enforce-adr-writing-order-with-structural-gates.tomlgov/work/2026-04-14-implement-adr-writing-order-gates.tomlsrc/cli.rssrc/cmd/edit/mod.rssrc/cmd/lifecycle.rssrc/command_router.rssrc/resource_plan.rstests/test_adr_gates.rstests/test_edit.rstests/test_lifecycle.rs
Comment on lines
+437
to
+440
| // Implements [[ADR-0042]]: block setting `decision` without complete alternatives | ||
| if artifact == ArtifactType::Adr && fp.as_simple() == Some("decision") { | ||
| crate::cmd::lifecycle::validate_adr_completeness(config, id)?; | ||
| } |
There was a problem hiding this comment.
Add RFC clause citation for this invariant gate.
This enforces a lifecycle/edit invariant in Rust code, but the comment cites only ADR-0042. Please add the corresponding RFC clause reference used as normative authority for this check.
As per coding guidelines: src/**/*.rs: Cite RFC clauses when implementing invariants in code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cmd/edit/mod.rs` around lines 437 - 440, Update the comment above the
invariant gate to cite the specific RFC clause that authorizes this
lifecycle/edit check in addition to ADR-0042: edit the line starting with "//
Implements [[ADR-0042]]: block setting `decision` without complete alternatives"
to include the RFC identifier and clause number (e.g., "RFC-XXXX §Y.Z") and a
short phrase linking it to the normative invariant; keep the code using
ArtifactType::Adr, fp.as_simple() == Some("decision"), and
crate::cmd::lifecycle::validate_adr_completeness(config, id)? unchanged.
Comment on lines
+297
to
+300
| /// Validate ADR alternatives completeness per [[ADR-0042]]. | ||
| /// | ||
| /// Requires at least 2 alternatives, with at least 1 accepted and 1 rejected. | ||
| pub fn validate_adr_completeness(config: &Config, adr_id: &str) -> anyhow::Result<()> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add RFC clause references for these newly enforced invariants.
These invariant comments currently cite [[ADR-0042]] only. Please also cite the exact normative RFC clause ID(s) that define this behavior so the implementation references the governing RFC directly in code.
As per coding guidelines src/**/*.rs: "Cite RFC clauses when implementing invariants in code".
Also applies to: 349-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cmd/lifecycle.rs` around lines 297 - 300, Update the doc comment for the
function validate_adr_completeness (and the related ADR comment at the other
nearby comment block referenced in the review) to append the exact normative RFC
clause identifier(s) that define the enforced invariants (requirement of at
least 2 alternatives, and at least 1 accepted and 1 rejected). Do not
paraphrase—insert the official RFC clause IDs/section numbers (e.g., "RFC-XXXX,
Section Y.Z" or the precise clause tokens) into the comment so the code directly
cites the governing normative text; apply the same insertion to the second
commented invariant noted in the review.
Add write-time gate blocking `adr set decision` without complete alternatives (>= 2, with 1 accepted and 1 rejected), and lifecycle gate blocking `adr accept` for incomplete ADRs. Both gates use shared `validate_adr_completeness()` helper. Accept gate supports `--force` for historical backfills. ADR-0042: accepted. WI-2026-04-14-001: done.
lucifer1004
force-pushed
the
feat/adr-gate
branch
from
5a566bf to
e9092fe
Compare
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
1 participant
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.
Summary
Add structural ADR completeness gates so
decisionandacceptfollow the repo's alternatives-first ADR writing model.What Changed
govctl adr set/edit ... decisionwhen alternatives are incompletegovctl adr acceptwhen the ADR does not yet have a complete alternatives setacceptedalternativerejectedalternativegovctl adr accept --forcefor historical backfills where alternatives cannot be reconstructedBehavior
decisionnow fails early if the ADR has no alternatives, only one alternative, or no rejected alternativegovctl adr acceptnow enforces the same completeness gate before moving toacceptedgovctl adr accept --forcebypasses the structural gate for backfill scenariosGovernance
enforce ADR writing order with structural gates)Verification
cargo test -qcargo run --quiet -- checkcargo clippy --all-targets --all-features -- -D warningsNotes
--forceexists only to support historical migration/backfill casesSummary by CodeRabbit
Release Notes
New Features
--forceflag to theadr acceptcommand to bypass validation requirements.Enhancements
--forceflag.