fix(lifecycle): preserve supersession history and atomicity#32
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (30)
✅ Files skipped from review due to trivial changes (7)
🚧 Files skipped from review as they are similar to previous changes (21)
📝 WalkthroughWalkthroughThis PR updates clause supersession rules and validation, adds related diagnostics and tests, and introduces atomic file writes with rollback semantics used by RFC bump, finalize, and supersede lifecycle commands. ChangesClause Supersession Chain Support
Atomic File Writes and Lifecycle Transactions
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Poem
🚥 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
📢 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cmd/lifecycle/clause.rs (1)
47-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve
--bybefore the lookup.validate_fieldaccepts same-RFC shorthand likeC-NEW, butrequire_replacement_clause_toml_path(config, by)still sees the raw value and can reject it before normalization. Normalizebyagainstclause_idfirst, or run the existence check on the normalized target.🤖 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/cmd/lifecycle/clause.rs` around lines 47 - 60, The replacement lookup in the clause lifecycle flow is using the raw `by` value before shorthand normalization, so fix `clause` handling to resolve `--by` against `clause_id` before calling `require_replacement_clause_toml_path`. Use the same normalization logic as `validate_field` for `superseded_by`, then perform the existence/path check on the normalized target so shorthand like `C-NEW` is accepted consistently.
🧹 Nitpick comments (4)
gov/adr/ADR-0050-preserve-direct-clause-supersession-chains.toml (1)
44-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the ADR out of implementation-spec territory.
This subsection prescribes validation and consumer behavior, which belongs in the RFC/work-item trail rather than the ADR itself. As per coding guidelines,
gov/adr/*.toml: ADRs must explain design choices and consequences; do not turn them into mini-RFCs or implementation plans.Suggested trim
-### Implementation Notes - -Validation of persisted history follows references across RFCs and checks graph integrity. Consumers may traverse direct edges when they need the latest reachable replacement.🤖 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 `@gov/adr/ADR-0050-preserve-direct-clause-supersession-chains.toml` around lines 44 - 46, The ADR’s “Implementation Notes” section is too prescriptive and drifts into RFC/work-item territory. Trim the `ADR-0050-preserve-direct-clause-supersession-chains.toml` content so it only states the design choice and its consequences, and remove validation/consumer-behavior guidance from this subsection. Keep the language in the ADR focused on the supersession-chain rationale, not on how `direct edges` should be traversed or validated.Source: Coding guidelines
tests/error_tests/rfc_clause_cases/check.rs (1)
228-233: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert
E0212in the new cycle checks.These assertions only pin the message text, so a regression that emits the wrong diagnostic code would still pass even though this change introduces
E0212as the cycle-specific contract.Proposed assertion update
- assert!( - output.contains("Clause supersession cycle detected"), - "output: {output}" - ); + assert!( + output.contains("error[E0212]: Clause supersession cycle detected"), + "output: {output}" + ); @@ - output.contains( - "Clause supersession cycle detected: RFC-0001:C-ONE -> RFC-0002:C-TWO -> RFC-0001:C-ONE" - ), + output.contains( + "error[E0212]: Clause supersession cycle detected: RFC-0001:C-ONE -> RFC-0002:C-TWO -> RFC-0001:C-ONE" + ), "output: {output}" );Also applies to: 322-329
🤖 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/error_tests/rfc_clause_cases/check.rs` around lines 228 - 233, The new cycle-check tests only verify the message text, so they can miss a regression where the diagnostic code is wrong. Update the assertions in the relevant `check.rs` cycle cases to also assert the emitted code is `E0212`, alongside the existing `"Clause supersession cycle detected"` text, using the same `run_commands` output checks.tests/lifecycle_tests/clause.rs (1)
386-390: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPin
E0212in the lifecycle self-replacement test.This is the direct regression for the new
validate_fieldpath, so matching only the message will miss an accidental diagnostic-code remap.Proposed assertion update
- assert!( - output.contains("Cannot supersede a clause with itself: RFC-0001:C-ONE"), - "output: {output}" - ); + assert!( + output.contains("error[E0212]: Cannot supersede a clause with itself: RFC-0001:C-ONE"), + "output: {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/lifecycle_tests/clause.rs` around lines 386 - 390, Update the lifecycle self-replacement test in clause.rs to assert the diagnostic code E0212 in addition to the existing message. The current check around the self-supersede output only matches the text, so adjust the test case that exercises the validate_field path to verify the emitted diagnostic code symbol alongside “Cannot supersede a clause with itself: RFC-0001:C-ONE”, while keeping the existing exit status assertion.tests/lifecycle_tests/rfc_cases/bump.rs (1)
297-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the other success-only UI lines too.
This only rejects
Bumped RFC-0001. IfAdded change:orSet RFC-0001:C-PENDING.since = ...leaks before rollback, the test still passes. Add negative assertions for those strings so the output contract is covered end-to-end.🤖 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/lifecycle_tests/rfc_cases/bump.rs` around lines 297 - 299, The rollback test in the bump RFC case only asserts that the success message is absent, so it can miss other leaked success-only UI lines. Update the assertions around the existing output checks to also verify that the output does not contain the other success strings, including the ones emitted by the bump flow such as the change-added message and the RFC-0001 pending-since update. Keep the checks near the existing output.contains assertions so the output contract is covered end-to-end.
🤖 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 `@src/cmd/lifecycle/rfc_clause_versions.rs`:
- Around line 22-38: The clause scanning logic is swallowing read/load failures
by using filter_map(Result::ok) and read_clause(...).ok(), which lets invalid or
unreadable pending clauses be skipped silently. Update the pending-clause
collection flow in the rfc_clause_versions helper (the code using read_dir,
read_clause, and paths.extend) to return the first directory/entry/read error
instead of filtering it out. Keep the .toml filtering, but propagate failures
from directory iteration and clause loading so bump/finalize cannot advance with
missing or unchanged pending clauses.
---
Outside diff comments:
In `@src/cmd/lifecycle/clause.rs`:
- Around line 47-60: The replacement lookup in the clause lifecycle flow is
using the raw `by` value before shorthand normalization, so fix `clause`
handling to resolve `--by` against `clause_id` before calling
`require_replacement_clause_toml_path`. Use the same normalization logic as
`validate_field` for `superseded_by`, then perform the existence/path check on
the normalized target so shorthand like `C-NEW` is accepted consistently.
---
Nitpick comments:
In `@gov/adr/ADR-0050-preserve-direct-clause-supersession-chains.toml`:
- Around line 44-46: The ADR’s “Implementation Notes” section is too
prescriptive and drifts into RFC/work-item territory. Trim the
`ADR-0050-preserve-direct-clause-supersession-chains.toml` content so it only
states the design choice and its consequences, and remove
validation/consumer-behavior guidance from this subsection. Keep the language in
the ADR focused on the supersession-chain rationale, not on how `direct edges`
should be traversed or validated.
In `@tests/error_tests/rfc_clause_cases/check.rs`:
- Around line 228-233: The new cycle-check tests only verify the message text,
so they can miss a regression where the diagnostic code is wrong. Update the
assertions in the relevant `check.rs` cycle cases to also assert the emitted
code is `E0212`, alongside the existing `"Clause supersession cycle detected"`
text, using the same `run_commands` output checks.
In `@tests/lifecycle_tests/clause.rs`:
- Around line 386-390: Update the lifecycle self-replacement test in clause.rs
to assert the diagnostic code E0212 in addition to the existing message. The
current check around the self-supersede output only matches the text, so adjust
the test case that exercises the validate_field path to verify the emitted
diagnostic code symbol alongside “Cannot supersede a clause with itself:
RFC-0001:C-ONE”, while keeping the existing exit status assertion.
In `@tests/lifecycle_tests/rfc_cases/bump.rs`:
- Around line 297-299: The rollback test in the bump RFC case only asserts that
the success message is absent, so it can miss other leaked success-only UI
lines. Update the assertions around the existing output checks to also verify
that the output does not contain the other success strings, including the ones
emitted by the bump flow such as the change-added message and the RFC-0001
pending-since update. Keep the checks near the existing output.contains
assertions so the output contract is covered end-to-end.
🪄 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: cc8073dc-7a63-4428-aadc-f7fe3ef5ac10
⛔ Files ignored due to path filters (1)
tests/snapshots/test_errors__broken_superseded_check.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
CHANGELOG.mddocs/rfc/RFC-0001.mddocs/rfc/RFC-0002.mdgov/adr/ADR-0050-preserve-direct-clause-supersession-chains.tomlgov/rfc/RFC-0001/clauses/C-CLAUSE-STATUS.tomlgov/rfc/RFC-0001/rfc.tomlgov/rfc/RFC-0002/clauses/C-LIFECYCLE-VERBS.tomlgov/rfc/RFC-0002/rfc.tomlgov/work/2026-06-28-harden-clause-supersession-validation.tomlgov/work/2026-06-28-make-artifact-writes-failure-atomic.tomlgov/work/2026-06-28-support-clause-supersession-chains.tomlsrc/cmd/lifecycle/clause.rssrc/cmd/lifecycle/rfc.rssrc/cmd/lifecycle/rfc_clause_versions.rssrc/cmd/lifecycle/rfc_supersede.rssrc/diagnostic/code/metadata.rssrc/diagnostic/code/mod.rssrc/validate/fields/mod.rssrc/validate/rfc.rssrc/write/mod.rstests/error_tests/rfc_clause_cases/check.rstests/lifecycle_tests/clause.rstests/lifecycle_tests/rfc_cases/bump.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/lifecycle_tests/clause.rs (1)
78-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso run
checkafter persisting the shorthand edge.This test confirms the CLI stores
C-NEW, but it never exercises the follow-on validation path.src/cmd/lifecycle/clause.rs:41-75persists the raw--byvalue, andsrc/validate/fields/mod.rs:96-136only normalizes same-RFC shorthand later during validation. Adding a trailingcheckhere would catch regressions where shorthand edges are written successfully but fail subsequent scans.Suggested test tightening
&[ "clause", "supersede", "RFC-0001:C-OLD", "--by", "C-NEW", "--force", ], &["clause", "get", "RFC-0001:C-OLD", "superseded_by"], + &["check"], ], )?; @@ assert!( output.contains("$ govctl clause get RFC-0001:C-OLD superseded_by\nC-NEW\nexit: 0"), "output: {output}" ); + assert!(output.ends_with("exit: 0\n\n"), "output: {output}"); Ok(()) }🤖 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/lifecycle_tests/clause.rs` around lines 78 - 127, This test only verifies that clause supersede stores the shorthand replacement, but it does not cover the later validation path. Update the lifecycle test in test_supersede_clause_accepts_same_rfc_shorthand_replacement to run a trailing check after the supersede/get sequence so it exercises the normalization flow in clause supersede handling and the validation logic that resolves same-RFC shorthand in validate::fields.
🤖 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 `@tests/lifecycle_tests/rfc_cases/bump.rs`:
- Around line 314-327: The lifecycle test in bump.rs is setting C-PENDING.toml
to read-only, but atomic_write_file writes through a temp file in the parent
directory, so this does not reliably trigger a failure. Update the failing setup
in the RFC bump test to block writes at the clauses directory level (or
otherwise prevent temp-file creation/rename) so the rollback path is exercised
deterministically, and keep the existing run_commands assertion flow unchanged.
---
Nitpick comments:
In `@tests/lifecycle_tests/clause.rs`:
- Around line 78-127: This test only verifies that clause supersede stores the
shorthand replacement, but it does not cover the later validation path. Update
the lifecycle test in
test_supersede_clause_accepts_same_rfc_shorthand_replacement to run a trailing
check after the supersede/get sequence so it exercises the normalization flow in
clause supersede handling and the validation logic that resolves same-RFC
shorthand in validate::fields.
🪄 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: 59a38ae0-461f-4beb-a20e-29bb55f8de2c
⛔ Files ignored due to path filters (1)
tests/snapshots/test_errors__broken_superseded_check.snapis excluded by!**/*.snap
📒 Files selected for processing (26)
CHANGELOG.mddocs/rfc/RFC-0001.mddocs/rfc/RFC-0002.mdgov/adr/ADR-0050-preserve-direct-clause-supersession-chains.tomlgov/rfc/RFC-0001/clauses/C-CLAUSE-STATUS.tomlgov/rfc/RFC-0001/rfc.tomlgov/rfc/RFC-0002/clauses/C-LIFECYCLE-VERBS.tomlgov/rfc/RFC-0002/rfc.tomlgov/work/2026-06-28-harden-clause-lifecycle-review-paths.tomlgov/work/2026-06-28-harden-clause-supersession-validation.tomlgov/work/2026-06-28-make-artifact-writes-failure-atomic.tomlgov/work/2026-06-28-support-clause-supersession-chains.tomlsrc/cmd/lifecycle/clause.rssrc/cmd/lifecycle/rfc.rssrc/cmd/lifecycle/rfc_clause_versions.rssrc/cmd/lifecycle/rfc_supersede.rssrc/diagnostic/code/metadata.rssrc/diagnostic/code/mod.rssrc/validate/fields/mod.rssrc/validate/mod.rssrc/validate/rfc.rssrc/write/mod.rstests/error_tests/rfc_clause_cases/check.rstests/lifecycle_tests/clause.rstests/lifecycle_tests/rfc_cases/bump.rstests/lifecycle_tests/rfc_cases/finalize.rs
✅ Files skipped from review due to trivial changes (5)
- gov/work/2026-06-28-make-artifact-writes-failure-atomic.toml
- gov/work/2026-06-28-harden-clause-supersession-validation.toml
- CHANGELOG.md
- gov/rfc/RFC-0001/rfc.toml
- gov/adr/ADR-0050-preserve-direct-clause-supersession-chains.toml
🚧 Files skipped from review as they are similar to previous changes (16)
- src/diagnostic/code/metadata.rs
- gov/work/2026-06-28-support-clause-supersession-chains.toml
- gov/rfc/RFC-0002/clauses/C-LIFECYCLE-VERBS.toml
- src/cmd/lifecycle/clause.rs
- gov/rfc/RFC-0002/rfc.toml
- src/cmd/lifecycle/rfc_supersede.rs
- gov/rfc/RFC-0001/clauses/C-CLAUSE-STATUS.toml
- docs/rfc/RFC-0001.md
- tests/error_tests/rfc_clause_cases/check.rs
- src/validate/rfc.rs
- src/diagnostic/code/mod.rs
- docs/rfc/RFC-0002.md
- src/cmd/lifecycle/rfc.rs
- src/validate/fields/mod.rs
- src/cmd/lifecycle/rfc_clause_versions.rs
- src/write/mod.rs
Keep clause supersession as direct historical edges with iterative cycle validation. Restore prior artifact contents when multi-file lifecycle operations return errors. Clarify the RFC-0002 failure-atomicity boundary and add regression coverage.
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.
Keep clause supersession as direct historical edges with iterative cycle validation. Restore prior artifact contents when multi-file lifecycle operations return errors. Clarify the RFC-0002 failure-atomicity boundary and add regression coverage.
Summary by CodeRabbit
clause supersede/validation: rejects missing, self-referential, and cyclic targets; prevents stack overflows on large acyclic graphs; improves malformed-input safety by stopping before mutations.