From cc1daafadd30b960810150c895a01b4798e28cee Mon Sep 17 00:00:00 2001 From: Gabriel Wu <13583761+lucifer1004@users.noreply.github.com> Date: Sun, 28 Jun 2026 15:29:12 +0800 Subject: [PATCH] fix(lifecycle): preserve supersession history and atomicity 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. --- .claude-plugin/marketplace.json | 2 +- .claude/.claude-plugin/plugin.json | 2 +- CHANGELOG.md | 20 + Cargo.lock | 2 +- Cargo.toml | 2 +- docs/rfc/RFC-0001.md | 62 +- docs/rfc/RFC-0002.md | 25 +- ...rve-direct-clause-supersession-chains.toml | 89 +++ gov/releases.toml | 10 + gov/rfc/RFC-0001/clauses/C-CLAUSE-STATUS.toml | 25 +- gov/rfc/RFC-0001/rfc.toml | 29 +- .../RFC-0002/clauses/C-LIFECYCLE-VERBS.toml | 13 +- gov/rfc/RFC-0002/rfc.toml | 12 +- ...-harden-clause-lifecycle-review-paths.toml | 46 ++ ...harden-clause-supersession-validation.toml | 41 ++ ...8-make-artifact-writes-failure-atomic.toml | 38 ++ ...28-support-clause-supersession-chains.toml | 55 ++ src/cmd/lifecycle/clause.rs | 6 +- src/cmd/lifecycle/rfc.rs | 62 +- src/cmd/lifecycle/rfc_clause_versions.rs | 85 ++- src/cmd/lifecycle/rfc_supersede.rs | 28 +- src/diagnostic/code/metadata.rs | 2 + src/diagnostic/code/mod.rs | 2 + src/validate/fields/mod.rs | 42 +- src/validate/mod.rs | 1 + src/validate/rfc.rs | 127 +++- src/write/mod.rs | 449 +++++++++++++- tests/error_tests/rfc_clause_cases/check.rs | 245 +++++++- tests/lifecycle_tests/clause.rs | 561 ++++++++++++++++++ tests/lifecycle_tests/rfc_cases/bump.rs | 95 +++ tests/lifecycle_tests/rfc_cases/finalize.rs | 25 + .../test_errors__broken_superseded_check.snap | 2 +- 32 files changed, 2084 insertions(+), 121 deletions(-) create mode 100644 gov/adr/ADR-0050-preserve-direct-clause-supersession-chains.toml create mode 100644 gov/work/2026-06-28-harden-clause-lifecycle-review-paths.toml create mode 100644 gov/work/2026-06-28-harden-clause-supersession-validation.toml create mode 100644 gov/work/2026-06-28-make-artifact-writes-failure-atomic.toml create mode 100644 gov/work/2026-06-28-support-clause-supersession-chains.toml diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 5e99452f..af779fe0 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -10,7 +10,7 @@ { "name": "govctl", "description": "Governed workflow skills, reviewer agents, and enforcement hooks for govctl", - "version": "0.10.0", + "version": "0.10.1", "source": "./.claude", "author": { "name": "govctl-org" diff --git a/.claude/.claude-plugin/plugin.json b/.claude/.claude-plugin/plugin.json index c6ba6eb1..ef4d67ee 100644 --- a/.claude/.claude-plugin/plugin.json +++ b/.claude/.claude-plugin/plugin.json @@ -1,5 +1,5 @@ { "name": "govctl", - "version": "0.10.0", + "version": "0.10.1", "description": "Governed workflow skills, reviewer agents, and enforcement hooks for govctl" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c140dbb..9152b159 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,26 @@ Release entries are curated summaries for readers. Work item traceability remain ## [Unreleased] +## [0.10.1] - 2026-06-29 + +0.10.1 is a lifecycle correctness and write-safety patch. Clause supersession +now preserves direct history across replacement chains, validates replacement +targets consistently, and avoids partial updates when lifecycle commands fail. + +### Fixed + +- Clause supersession now preserves direct replacement history, so chains such + as `A -> B -> C` remain valid after B is superseded or deprecated. +- `govctl clause supersede` now accepts active same-RFC shorthand and qualified + cross-RFC replacements while rejecting missing, self-referential, and cyclic + targets. +- Supersession validation now handles very large acyclic graphs without + exhausting the call stack. +- RFC bump and finalize operations now stop before making changes when a + pending clause cannot be read or parsed. +- Lifecycle writes are now failure-atomic: failed multi-file operations restore + earlier changes, and diagnostics report any incomplete rollback. + ## [0.10.0] - 2026-06-17 0.10.0 tightens loop execution semantics and governance validation. Loop rounds diff --git a/Cargo.lock b/Cargo.lock index 3f064750..c0869b13 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1142,7 +1142,7 @@ dependencies = [ [[package]] name = "govctl" -version = "0.10.0" +version = "0.10.1" dependencies = [ "ansi-to-tui", "chrono", diff --git a/Cargo.toml b/Cargo.toml index 2bdbef4b..33e1daf8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "govctl" -version = "0.10.0" +version = "0.10.1" edition = "2024" rust-version = "1.96" description = "Project governance CLI for RFC, ADR, and Work Item management" diff --git a/docs/rfc/RFC-0001.md b/docs/rfc/RFC-0001.md index 34d1ad48..fd42a912 100644 --- a/docs/rfc/RFC-0001.md +++ b/docs/rfc/RFC-0001.md @@ -1,9 +1,9 @@ - + # RFC-0001: Lifecycle State Machines -> **Version:** 0.3.0 | **Status:** normative | **Phase:** stable +> **Version:** 0.4.2 | **Status:** normative | **Phase:** stable --- @@ -148,9 +148,28 @@ Invalid transitions (MUST be rejected): - deprecated → active (no resurrection) - superseded → any (superseded is terminal) -When a clause is superseded: -- The `superseded_by` field MUST be set to the ID of the replacing clause -- The replacing clause SHOULD have a `since` field indicating the version it was introduced +A clause supersession transition has the following preconditions: +- The replacing clause MUST exist. +- The replacing clause MUST be active when the transition is performed. +- The replacing clause MUST be different from the clause being superseded. +- A replacing clause in another RFC MUST be identified by its qualified `RFC-NNNN:C-NAME` ID. + +The transition MUST be rejected if any required precondition is not satisfied. + +After a successful clause supersession transition: +- The source clause status MUST be `superseded`. +- The source clause `superseded_by` field MUST identify the direct replacing clause selected for that transition. +- The replacing clause SHOULD have a `since` field indicating the version it was introduced. + +Every populated `superseded_by` field defines a directed edge from its source clause to its target clause. The clause supersession graph consists of these edges across every RFC in the governed project. + +Repository validation MUST reject an edge whose target clause does not exist. Repository validation MUST NOT reject an existing edge solely because its target clause was later deprecated or superseded. + +If clause A identifies clause B as its direct replacement and B is later replaced by clause C, B MUST identify C as its direct replacement. Clause A MUST continue to identify B. The clause supersession graph MUST NOT contain a cycle. Repository validation MUST reject a graph that contains a cycle. + +**Rationale:** + +Transition-time checks ensure that a newly selected replacement is in effect. Persisted edges record historical direct replacements, so later clause evolution does not invalidate or erase prior provenance. Qualified IDs allow the same model to represent unambiguous replacements across RFC boundaries. > **Tags:** `lifecycle` @@ -194,6 +213,39 @@ Future gates MAY be added via RFC amendment, but MUST NOT break existing valid w ## Changelog +### v0.4.2 (2026-06-28) + +Split direct-edge preservation requirements + +#### Changed + +- Express B-to-C insertion and A-to-B preservation as separate testable obligations + +### v0.4.1 (2026-06-28) + +Clarify clause supersession validation boundaries + +#### Added + +- Define project-wide direct-edge graph semantics and explicit rejection behavior for missing targets and cycles + +#### Changed + +- Separate transition-time replacement checks from persisted graph validation +- Document the distinct-target rule and preserve historical edges after target status changes + +### v0.4.0 (2026-06-28) + +Define direct clause supersession chains + +#### Added + +- Require active transition targets, qualified cross-RFC references, and acyclic supersession graphs + +#### Changed + +- Treat superseded_by as a persistent direct replacement edge + ### v0.3.0 (2026-03-17) Add executable verification gates to work completion diff --git a/docs/rfc/RFC-0002.md b/docs/rfc/RFC-0002.md index 5305d81b..6c45c486 100644 --- a/docs/rfc/RFC-0002.md +++ b/docs/rfc/RFC-0002.md @@ -1,9 +1,9 @@ - + # RFC-0002: CLI Resource Model and Command Architecture -> **Version:** 0.10.3 | **Status:** normative | **Phase:** test +> **Version:** 0.10.4 | **Status:** normative | **Phase:** test --- @@ -322,9 +322,14 @@ Resource-specific lifecycle verbs implement state transitions defined in [RFC-00 1. All lifecycle verbs MUST validate transitions per [RFC-0001](../rfc/RFC-0001.md) 2. All lifecycle verbs MUST update timestamp fields where applicable -3. All lifecycle verbs MUST be atomic (no partial state changes) -4. All lifecycle verbs MUST support global `--dry-run` flag -5. Invalid transitions MUST error with clear explanation of valid transitions +3. Except for the rollback-failure path in requirement 4, a lifecycle invocation that handles an error and completes through govctl's normal error path with a non-zero exit status MUST restore every governed artifact changed by that invocation byte-for-byte before returning +4. If an I/O or storage error prevents complete restoration, the invocation MUST return a non-zero exit status +5. The rollback-failure diagnostic MUST contain the term `rollback` +6. The rollback-failure diagnostic MUST state that governed-artifact restoration may be incomplete +7. The restoration baseline is the artifact content observed after the invocation acquires its exclusive write scope under [RFC-0004:C-SCOPE](../rfc/RFC-0004.md#rfc-0004c-scope) and before its first governed-artifact mutation +8. Unexpected process termination before normal error completion, host failure, and the rollback-failure path in requirement 4 are outside the lifecycle atomicity guarantee +9. All lifecycle verbs MUST support global `--dry-run` flag +10. Invalid transitions MUST error with clear explanation of valid transitions **Rationale:** @@ -334,7 +339,7 @@ Lifecycle verbs are resource-specific because: - Work items have gate conditions (acceptance criteria) - Each resource has different valid transitions -Scoping these verbs to resources makes their applicability explicit and prevents confusion. +Scoping these verbs to resources makes their applicability explicit and prevents confusion. Lifecycle atomicity defines the byte content observed when an invocation completes through its normal error path; it does not require a crash-recovery subsystem for the flat-file artifact store. > **Tags:** `cli`, `lifecycle` @@ -736,6 +741,14 @@ Search is discovery across the governance corpus, not a resource-specific CRUD o ## Changelog +### v0.10.4 (2026-06-28) + +Clarify lifecycle failure atomicity boundary + +#### Changed + +- define byte-for-byte restoration for normally returned lifecycle errors and explicit rollback-failure boundaries + ### v0.10.3 (2026-06-15) Reject empty RFC version bumps diff --git a/gov/adr/ADR-0050-preserve-direct-clause-supersession-chains.toml b/gov/adr/ADR-0050-preserve-direct-clause-supersession-chains.toml new file mode 100644 index 00000000..d7cf4756 --- /dev/null +++ b/gov/adr/ADR-0050-preserve-direct-clause-supersession-chains.toml @@ -0,0 +1,89 @@ +#:schema ../schema/adr.schema.json + +[govctl] +id = "ADR-0050" +title = "Preserve direct clause supersession chains" +status = "accepted" +date = "2026-06-28" +refs = [ + "RFC-0001:C-CLAUSE-STATUS", + "RFC-0002:C-LIFECYCLE-VERBS", +] +tags = [ + "lifecycle", + "validation", +] + +[content] +context = """ +[[RFC-0001:C-CLAUSE-STATUS]] records the clause that replaced a superseded clause. Existing validation interpreted that reference as a permanently active destination, so a valid `A -> B` replacement became invalid after `B` was replaced by `C`. + +### Problem Statement + +The stored relation needs an unambiguous meaning that supports repeated clause evolution without rewriting or invalidating prior history. + +### Constraints + +- Supersession history must remain auditable. +- A lifecycle transition should update only the clause being transitioned. +- Malformed references and cycles must remain detectable. +- Qualified clause IDs already provide an unambiguous representation for cross-RFC references. + +### Options Considered + +The discussion considered preserving direct historical edges, flattening all predecessors to the latest replacement, and retaining the active-target invariant that prevents chains.""" +decision = """ +We will preserve each `superseded_by` value as the direct replacement selected when its source clause was superseded. Later replacements extend the chain instead of rewriting earlier clauses because: + +1. **Auditability:** Direct edges retain the exact sequence of clause evolution. +2. **Write locality:** Each transition changes only its source clause instead of rewriting every predecessor. +3. **Composability:** The same relation represents repeated and cross-RFC replacement without a second storage model. + +This separates two concerns: transition-time eligibility determines whether a new replacement edge may be created, while repository validation determines whether persisted history is structurally sound. Cross-RFC replacements use qualified clause IDs so the edge remains unambiguous.""" +consequences = """ +### Positive + +- Historical provenance remains intact across any number of replacements. +- Superseding a clause requires only a local write to that clause. +- The same model supports both same-RFC and qualified cross-RFC replacements. + +### Negative + +- Resolving the latest replacement can require following multiple direct edges. +- Malformed direct-edge histories can contain cycles or unresolved references. + +### Neutral + +- A chain may end at a clause that is no longer active; this describes history rather than asserting that an active replacement currently exists.""" + +[[content.alternatives]] +text = "Preserve each direct replacement as a historical edge and resolve later replacements by traversing the chain" +status = "accepted" +pros = [ + "Retains the exact sequence of clause evolution", + "Each transition changes only its source clause", +] +cons = [ + "Consumers that need the latest replacement must traverse the chain", + "Validation must detect self-references and cycles", +] + +[[content.alternatives]] +text = "Flatten predecessors to the newest replacement whenever another clause is superseded" +status = "rejected" +pros = ["A stored reference always points directly to the latest replacement"] +cons = [ + "Destroys the direct replacement history", + "Requires multi-file rewrites for one lifecycle transition", +] +rejection_reason = "The simpler lookup does not justify loss of provenance, wider writes, and additional merge conflicts" + +[[content.alternatives]] +text = "Require every stored replacement target to remain active and disallow supersession chains" +status = "rejected" +pros = ["Keeps validation and lookup limited to one edge"] +cons = [ + "A later replacement invalidates previously valid history", + "Prevents normal repeated evolution of a clause contract", +] +rejection_reason = "It creates the issue being addressed and conflates transition-time eligibility with historical validity" diff --git a/gov/releases.toml b/gov/releases.toml index f3af181e..1307d9ef 100644 --- a/gov/releases.toml +++ b/gov/releases.toml @@ -2,6 +2,16 @@ [govctl] +[[releases]] +version = "0.10.1" +date = "2026-06-29" +refs = [ + "WI-2026-06-28-001", + "WI-2026-06-28-002", + "WI-2026-06-28-003", + "WI-2026-06-28-004", +] + [[releases]] version = "0.10.0" date = "2026-06-17" diff --git a/gov/rfc/RFC-0001/clauses/C-CLAUSE-STATUS.toml b/gov/rfc/RFC-0001/clauses/C-CLAUSE-STATUS.toml index f20f7b89..2f925b42 100644 --- a/gov/rfc/RFC-0001/clauses/C-CLAUSE-STATUS.toml +++ b/gov/rfc/RFC-0001/clauses/C-CLAUSE-STATUS.toml @@ -25,6 +25,25 @@ Invalid transitions (MUST be rejected): - deprecated → active (no resurrection) - superseded → any (superseded is terminal) -When a clause is superseded: -- The `superseded_by` field MUST be set to the ID of the replacing clause -- The replacing clause SHOULD have a `since` field indicating the version it was introduced""" +A clause supersession transition has the following preconditions: +- The replacing clause MUST exist. +- The replacing clause MUST be active when the transition is performed. +- The replacing clause MUST be different from the clause being superseded. +- A replacing clause in another RFC MUST be identified by its qualified `RFC-NNNN:C-NAME` ID. + +The transition MUST be rejected if any required precondition is not satisfied. + +After a successful clause supersession transition: +- The source clause status MUST be `superseded`. +- The source clause `superseded_by` field MUST identify the direct replacing clause selected for that transition. +- The replacing clause SHOULD have a `since` field indicating the version it was introduced. + +Every populated `superseded_by` field defines a directed edge from its source clause to its target clause. The clause supersession graph consists of these edges across every RFC in the governed project. + +Repository validation MUST reject an edge whose target clause does not exist. Repository validation MUST NOT reject an existing edge solely because its target clause was later deprecated or superseded. + +If clause A identifies clause B as its direct replacement and B is later replaced by clause C, B MUST identify C as its direct replacement. Clause A MUST continue to identify B. The clause supersession graph MUST NOT contain a cycle. Repository validation MUST reject a graph that contains a cycle. + +**Rationale:** + +Transition-time checks ensure that a newly selected replacement is in effect. Persisted edges record historical direct replacements, so later clause evolution does not invalidate or erase prior provenance. Qualified IDs allow the same model to represent unambiguous replacements across RFC boundaries.""" diff --git a/gov/rfc/RFC-0001/rfc.toml b/gov/rfc/RFC-0001/rfc.toml index 043b7421..ed28c271 100644 --- a/gov/rfc/RFC-0001/rfc.toml +++ b/gov/rfc/RFC-0001/rfc.toml @@ -3,17 +3,17 @@ [govctl] id = "RFC-0001" title = "Lifecycle State Machines" -version = "0.3.0" +version = "0.4.2" status = "normative" phase = "stable" owners = ["@govctl-org"] created = "2026-01-17" -updated = "2026-03-17" +updated = "2026-06-28" tags = [ "core", "lifecycle", ] -signature = "a914b5def2a5a95e84ae3fd0d26f81b166e1fc218b204a9aabf3a948ae662bbc" +signature = "9bf9e3a853547590af3b22db2ad5c8ae4296dae6b79fe652a8a2cea9ba5085a1" [[sections]] title = "Summary" @@ -30,6 +30,29 @@ clauses = [ "clauses/C-GATE-CONDITIONS.toml", ] +[[changelog]] +version = "0.4.2" +date = "2026-06-28" +notes = "Split direct-edge preservation requirements" +changed = ["Express B-to-C insertion and A-to-B preservation as separate testable obligations"] + +[[changelog]] +version = "0.4.1" +date = "2026-06-28" +notes = "Clarify clause supersession validation boundaries" +added = ["Define project-wide direct-edge graph semantics and explicit rejection behavior for missing targets and cycles"] +changed = [ + "Separate transition-time replacement checks from persisted graph validation", + "Document the distinct-target rule and preserve historical edges after target status changes", +] + +[[changelog]] +version = "0.4.0" +date = "2026-06-28" +notes = "Define direct clause supersession chains" +added = ["Require active transition targets, qualified cross-RFC references, and acyclic supersession graphs"] +changed = ["Treat superseded_by as a persistent direct replacement edge"] + [[changelog]] version = "0.3.0" date = "2026-03-17" diff --git a/gov/rfc/RFC-0002/clauses/C-LIFECYCLE-VERBS.toml b/gov/rfc/RFC-0002/clauses/C-LIFECYCLE-VERBS.toml index b79a0dd3..476d70a3 100644 --- a/gov/rfc/RFC-0002/clauses/C-LIFECYCLE-VERBS.toml +++ b/gov/rfc/RFC-0002/clauses/C-LIFECYCLE-VERBS.toml @@ -70,9 +70,14 @@ Resource-specific lifecycle verbs implement state transitions defined in [[RFC-0 1. All lifecycle verbs MUST validate transitions per [[RFC-0001]] 2. All lifecycle verbs MUST update timestamp fields where applicable -3. All lifecycle verbs MUST be atomic (no partial state changes) -4. All lifecycle verbs MUST support global `--dry-run` flag -5. Invalid transitions MUST error with clear explanation of valid transitions +3. Except for the rollback-failure path in requirement 4, a lifecycle invocation that handles an error and completes through govctl's normal error path with a non-zero exit status MUST restore every governed artifact changed by that invocation byte-for-byte before returning +4. If an I/O or storage error prevents complete restoration, the invocation MUST return a non-zero exit status +5. The rollback-failure diagnostic MUST contain the term `rollback` +6. The rollback-failure diagnostic MUST state that governed-artifact restoration may be incomplete +7. The restoration baseline is the artifact content observed after the invocation acquires its exclusive write scope under [[RFC-0004:C-SCOPE]] and before its first governed-artifact mutation +8. Unexpected process termination before normal error completion, host failure, and the rollback-failure path in requirement 4 are outside the lifecycle atomicity guarantee +9. All lifecycle verbs MUST support global `--dry-run` flag +10. Invalid transitions MUST error with clear explanation of valid transitions **Rationale:** @@ -82,4 +87,4 @@ Lifecycle verbs are resource-specific because: - Work items have gate conditions (acceptance criteria) - Each resource has different valid transitions -Scoping these verbs to resources makes their applicability explicit and prevents confusion.""" +Scoping these verbs to resources makes their applicability explicit and prevents confusion. Lifecycle atomicity defines the byte content observed when an invocation completes through its normal error path; it does not require a crash-recovery subsystem for the flat-file artifact store.""" diff --git a/gov/rfc/RFC-0002/rfc.toml b/gov/rfc/RFC-0002/rfc.toml index 53847f8e..22ebffc7 100644 --- a/gov/rfc/RFC-0002/rfc.toml +++ b/gov/rfc/RFC-0002/rfc.toml @@ -3,12 +3,12 @@ [govctl] id = "RFC-0002" title = "CLI Resource Model and Command Architecture" -version = "0.10.3" +version = "0.10.4" status = "normative" phase = "test" owners = ["@govctl-org"] created = "2026-01-19" -updated = "2026-06-15" +updated = "2026-06-28" tags = [ "cli", "editing", @@ -16,7 +16,7 @@ tags = [ "validation", "release", ] -signature = "10d3e6d4fb3468159ccb4d1bb48e79bac70dee20a76c213fd8e5f1647cd502f6" +signature = "6a1b4f4f096cd9cd0d9b260e77ad08daf0a0a04874eab8953b4b78a1e8e5a1b7" [[sections]] title = "Summary" @@ -36,6 +36,12 @@ clauses = [ "clauses/C-SEARCH-COMMAND.toml", ] +[[changelog]] +version = "0.10.4" +date = "2026-06-28" +notes = "Clarify lifecycle failure atomicity boundary" +changed = ["define byte-for-byte restoration for normally returned lifecycle errors and explicit rollback-failure boundaries"] + [[changelog]] version = "0.10.3" date = "2026-06-15" diff --git a/gov/work/2026-06-28-harden-clause-lifecycle-review-paths.toml b/gov/work/2026-06-28-harden-clause-lifecycle-review-paths.toml new file mode 100644 index 00000000..f4da60b3 --- /dev/null +++ b/gov/work/2026-06-28-harden-clause-lifecycle-review-paths.toml @@ -0,0 +1,46 @@ +#:schema ../schema/work.schema.json + +[govctl] +id = "WI-2026-06-28-004" +title = "Harden clause lifecycle review paths" +status = "done" +created = "2026-06-28" +started = "2026-06-28" +completed = "2026-06-28" +refs = [ + "RFC-0001:C-CLAUSE-STATUS", + "RFC-0002:C-LIFECYCLE-VERBS", + "ADR-0050", +] +tags = [ + "lifecycle", + "validation", +] + +[content] +description = "Propagate `clause` scan failures and align `same-RFC` replacement lookup with existing `superseded_by` validation while tightening review coverage and ADR boundaries." + +[[content.acceptance_criteria]] +text = "malformed clause TOML blocks RFC bump and finalize before mutation" +status = "done" +category = "fixed" + +[[content.acceptance_criteria]] +text = "same-RFC shorthand replacement resolves and persists the direct edge" +status = "done" +category = "fixed" + +[[content.acceptance_criteria]] +text = "cycle diagnostics and failed-bump output assertions cover stable codes and success-only messages" +status = "done" +category = "chore" + +[[content.acceptance_criteria]] +text = "ADR-0050 remains focused on rationale and consequences" +status = "done" +category = "chore" + +[[content.acceptance_criteria]] +text = "govctl check passes" +status = "done" +category = "chore" diff --git a/gov/work/2026-06-28-harden-clause-supersession-validation.toml b/gov/work/2026-06-28-harden-clause-supersession-validation.toml new file mode 100644 index 00000000..505cc28b --- /dev/null +++ b/gov/work/2026-06-28-harden-clause-supersession-validation.toml @@ -0,0 +1,41 @@ +#:schema ../schema/work.schema.json + +[govctl] +id = "WI-2026-06-28-003" +title = "Harden clause supersession validation" +status = "done" +created = "2026-06-28" +started = "2026-06-28" +completed = "2026-06-28" +refs = [ + "RFC-0001:C-CLAUSE-STATUS", + "ADR-0050", + "RFC-0002:C-LIFECYCLE-VERBS", +] +tags = [ + "lifecycle", + "validation", +] + +[content] +description = "Harden `clause supersession graph` validation against `stack exhaustion` and strengthen lifecycle coverage for governed transitions." + +[[content.acceptance_criteria]] +text = "validation completes for an acyclic supersession graph with 20,000 direct edges without stack overflow" +status = "done" +category = "fixed" + +[[content.acceptance_criteria]] +text = "same-RFC and qualified cross-RFC supersession tests persist the direct replacement edge" +status = "done" +category = "chore" + +[[content.acceptance_criteria]] +text = "deprecated sources succeed while invalid targets and repeated transitions are rejected without replacing a persisted edge" +status = "done" +category = "chore" + +[[content.acceptance_criteria]] +text = "govctl check passes" +status = "done" +category = "chore" diff --git a/gov/work/2026-06-28-make-artifact-writes-failure-atomic.toml b/gov/work/2026-06-28-make-artifact-writes-failure-atomic.toml new file mode 100644 index 00000000..7d25e4ed --- /dev/null +++ b/gov/work/2026-06-28-make-artifact-writes-failure-atomic.toml @@ -0,0 +1,38 @@ +#:schema ../schema/work.schema.json + +[govctl] +id = "WI-2026-06-28-002" +title = "Make artifact writes failure-atomic" +status = "done" +created = "2026-06-28" +started = "2026-06-28" +completed = "2026-06-28" +refs = [ + "RFC-0002:C-LIFECYCLE-VERBS", + "RFC-0004:C-CONCURRENT-WRITE", + "RFC-0004:C-SCOPE", +] +tags = ["safety"] + +[content] +description = "Implement byte-for-byte restoration for normally returned lifecycle failures and explicit `rollback`-failure diagnostics." + +[[content.acceptance_criteria]] +text = "lifecycle artifact write failures preserve prior file contents" +status = "done" +category = "fixed" + +[[content.acceptance_criteria]] +text = "govctl check passes" +status = "done" +category = "chore" + +[[content.acceptance_criteria]] +text = "multi-artifact RFC lifecycle failures restore every file changed earlier in the operation" +status = "done" +category = "fixed" + +[[content.acceptance_criteria]] +text = "rollback failure diagnostics identify incomplete restoration" +status = "done" +category = "fixed" diff --git a/gov/work/2026-06-28-support-clause-supersession-chains.toml b/gov/work/2026-06-28-support-clause-supersession-chains.toml new file mode 100644 index 00000000..6fc008fd --- /dev/null +++ b/gov/work/2026-06-28-support-clause-supersession-chains.toml @@ -0,0 +1,55 @@ +#:schema ../schema/work.schema.json + +[govctl] +id = "WI-2026-06-28-001" +title = "Support clause supersession chains" +status = "done" +created = "2026-06-28" +started = "2026-06-28" +completed = "2026-06-28" +refs = [ + "ADR-0050", + "RFC-0001:C-CLAUSE-STATUS", +] +tags = [ + "lifecycle", + "validation", +] + +[content] +description = "Correct clause supersession validation so historical replacement chains remain valid while malformed or cyclic links are rejected, aligning lifecycle checks with [[RFC-0001:C-CLAUSE-STATUS]]." + +[[content.acceptance_criteria]] +text = "clause supersession chains remain valid after an intermediate replacement is superseded" +status = "done" +category = "fixed" + +[[content.acceptance_criteria]] +text = "clause supersession validation rejects missing, self-referential, and cyclic targets" +status = "done" +category = "fixed" + +[[content.acceptance_criteria]] +text = "focused lifecycle and validation tests pass" +status = "done" +category = "chore" + +[[content.acceptance_criteria]] +text = "govctl check passes" +status = "done" +category = "chore" + +[[content.acceptance_criteria]] +text = "clause supersession accepts a qualified active replacement from another RFC" +status = "done" +category = "fixed" + +[[content.acceptance_criteria]] +text = "direct supersession history preserves A to B when B is later replaced by C" +status = "done" +category = "fixed" + +[[content.acceptance_criteria]] +text = "persisted supersession history remains valid after its target is deprecated" +status = "done" +category = "fixed" diff --git a/src/cmd/lifecycle/clause.rs b/src/cmd/lifecycle/clause.rs index c8cb8c3a..3a7003ae 100644 --- a/src/cmd/lifecycle/clause.rs +++ b/src/cmd/lifecycle/clause.rs @@ -4,6 +4,7 @@ use crate::config::Config; use crate::diagnostic::{Diagnostic, DiagnosticCode, DiagnosticResult, Diagnostics}; use crate::model::ClauseStatus; use crate::ui; +use crate::validate::{ArtifactKind, normalize_clause_supersession_target, validate_field}; use crate::write::{WriteOp, read_clause, write_clause}; pub(super) fn deprecate_clause( @@ -43,7 +44,8 @@ pub(super) fn supersede_clause( by: &str, op: WriteOp, ) -> DiagnosticResult { - require_replacement_clause_toml_path(config, by)?; + let replacement_id = normalize_clause_supersession_target(clause_id, by)?; + require_replacement_clause_toml_path(config, &replacement_id)?; let clause_path = require_clause_toml_path(config, clause_id)?; let mut clause = read_clause(config, &clause_path)?; @@ -56,6 +58,8 @@ pub(super) fn supersede_clause( )); } + validate_field(config, clause_id, ArtifactKind::Clause, "superseded_by", by)?; + clause.status = ClauseStatus::Superseded; clause.superseded_by = Some(by.to_string()); write_clause( diff --git a/src/cmd/lifecycle/rfc.rs b/src/cmd/lifecycle/rfc.rs index df4bcde5..d58a3a9f 100644 --- a/src/cmd/lifecycle/rfc.rs +++ b/src/cmd/lifecycle/rfc.rs @@ -1,5 +1,5 @@ use super::paths::require_rfc_toml_path; -use super::rfc_clause_versions::fill_pending_clause_versions; +use super::rfc_clause_versions::{fill_pending_clause_versions, rfc_update_paths}; use crate::FinalizeStatus; use crate::cmd::edit; use crate::config::Config; @@ -7,7 +7,9 @@ use crate::diagnostic::{Diagnostic, DiagnosticCode, DiagnosticResult, Diagnostic use crate::model::{RfcPhase, RfcSpec, RfcStatus}; use crate::ui; use crate::validate::{is_valid_phase_transition, is_valid_status_transition}; -use crate::write::{BumpLevel, WriteOp, add_changelog_change, bump_rfc_version, read_rfc}; +use crate::write::{ + BumpLevel, WriteOp, add_changelog_change, bump_rfc_version, read_rfc, with_file_transaction, +}; use std::path::Path; /// Bump RFC version @@ -27,21 +29,30 @@ pub fn bump( ensure_rfc_has_content_amendment(config, &rfc_path, rfc_id)?; let new_version = bump_rfc_version(&mut rfc, lvl, sum)?; - if !op.is_preview() { - ui::version_bumped(rfc_id, &new_version); - } for change in changes { add_changelog_change(&mut rfc, change)?; - if !op.is_preview() { - ui::sub_info(format!("Added change: {change}")); - } } - write_lifecycle_rfc(config, &rfc_path, &rfc, op)?; + let paths = rfc_update_paths(config, &rfc_path)?; + let path_refs: Vec<_> = paths.iter().map(std::path::PathBuf::as_path).collect(); + let updated_clause_ids = with_file_transaction(&path_refs, op, || { + write_lifecycle_rfc(config, &rfc_path, &rfc, op)?; + let updated_clause_ids = + fill_pending_clause_versions(config, &rfc_path, &new_version, op)?; + refresh_rfc_signature_best_effort(config, &rfc_path, &mut rfc, op)?; + Ok(updated_clause_ids) + })?; - fill_pending_clause_versions(config, &rfc_path, &new_version, op)?; - refresh_rfc_signature_best_effort(config, &rfc_path, &mut rfc, op)?; + if !op.is_preview() { + ui::version_bumped(rfc_id, &new_version); + for change in changes { + ui::sub_info(format!("Added change: {change}")); + } + for clause_id in updated_clause_ids { + ui::sub_info(format!("Set {clause_id}.since = {new_version}")); + } + } return Ok(vec![]); } @@ -57,9 +68,6 @@ pub fn bump( should_refresh_signature_after_changelog_only(config, &rfc_path)?; for change in changes { add_changelog_change(&mut rfc, change)?; - if !op.is_preview() { - ui::changelog_change_added(rfc_id, &rfc.version, change); - } } refresh_signature_after_write } @@ -79,9 +87,17 @@ pub fn bump( } }; - write_lifecycle_rfc(config, &rfc_path, &rfc, op)?; - if refresh_signature_after_write { - refresh_rfc_signature_best_effort(config, &rfc_path, &mut rfc, op)?; + with_file_transaction(&[rfc_path.as_path()], op, || { + write_lifecycle_rfc(config, &rfc_path, &rfc, op)?; + if refresh_signature_after_write { + refresh_rfc_signature_best_effort(config, &rfc_path, &mut rfc, op)?; + } + Ok(()) + })?; + if !op.is_preview() { + for change in changes { + ui::changelog_change_added(rfc_id, &rfc.version, change); + } } Ok(vec![]) } @@ -114,11 +130,17 @@ pub fn finalize( )); } - edit::set_field_direct(config, rfc_id, "status", target_status.as_ref(), op)?; - - fill_pending_clause_versions(config, &rfc_path, &rfc.version, op)?; + let paths = rfc_update_paths(config, &rfc_path)?; + let path_refs: Vec<_> = paths.iter().map(std::path::PathBuf::as_path).collect(); + let updated_clause_ids = with_file_transaction(&path_refs, op, || { + edit::set_field_direct(config, rfc_id, "status", target_status.as_ref(), op)?; + fill_pending_clause_versions(config, &rfc_path, &rfc.version, op) + })?; if !op.is_preview() { + for clause_id in updated_clause_ids { + ui::sub_info(format!("Set {clause_id}.since = {}", rfc.version)); + } ui::finalized(rfc_id, target_status.as_ref()); } Ok(vec![]) diff --git a/src/cmd/lifecycle/rfc_clause_versions.rs b/src/cmd/lifecycle/rfc_clause_versions.rs index 67b802f1..4bbea20e 100644 --- a/src/cmd/lifecycle/rfc_clause_versions.rs +++ b/src/cmd/lifecycle/rfc_clause_versions.rs @@ -1,19 +1,20 @@ use crate::config::Config; use crate::diagnostic::{Diagnostic, DiagnosticCode, DiagnosticResult}; -use crate::ui; use crate::write::{WriteOp, read_clause, write_clause}; -use std::path::Path; +use std::path::{Path, PathBuf}; -/// Update pending clauses (since: null) with the given version. -/// -/// Clauses are created with `since: None` and filled in when the RFC -/// is bumped or finalized. -pub(super) fn fill_pending_clause_versions( - config: &Config, - rfc_path: &Path, - version: &str, - op: WriteOp, -) -> DiagnosticResult<()> { +pub(super) fn rfc_update_paths(config: &Config, rfc_path: &Path) -> DiagnosticResult> { + let mut paths = vec![rfc_path.to_path_buf()]; + for path in clause_toml_paths(rfc_path)? { + if read_clause(config, &path)?.since.is_none() { + paths.push(path); + } + } + paths.sort(); + Ok(paths) +} + +fn clause_toml_paths(rfc_path: &Path) -> DiagnosticResult> { let clauses_dir = rfc_path .parent() .ok_or_else(|| { @@ -25,34 +26,64 @@ pub(super) fn fill_pending_clause_versions( })? .join("clauses"); if !clauses_dir.exists() { - return Ok(()); + return Ok(Vec::new()); } - let mut pending_clauses: Vec<_> = std::fs::read_dir(&clauses_dir) - .map_err(|err| { + let entries = std::fs::read_dir(&clauses_dir).map_err(|err| { + Diagnostic::io_error( + "read clauses directory", + err, + clauses_dir.display().to_string(), + ) + })?; + let mut paths = Vec::new(); + for entry in entries { + let entry = entry.map_err(|err| { Diagnostic::io_error( - "read clauses directory", + "read clauses directory entry", err, clauses_dir.display().to_string(), ) - })? - .filter_map(Result::ok) - .map(|e| e.path()) - .filter(|p| p.extension().is_some_and(|e| e == "toml")) - .filter_map(|p| read_clause(config, &p).ok().map(|c| (p, c))) - .filter(|(_, c)| c.since.is_none()) - .collect(); + })?; + let path = entry.path(); + if path + .extension() + .is_some_and(|extension| extension == "toml") + { + paths.push(path); + } + } + paths.sort(); + Ok(paths) +} + +/// Update pending clauses (since: null) with the given version. +/// +/// Clauses are created with `since: None` and filled in when the RFC +/// is bumped or finalized. +pub(super) fn fill_pending_clause_versions( + config: &Config, + rfc_path: &Path, + version: &str, + op: WriteOp, +) -> DiagnosticResult> { + let mut pending_clauses = Vec::new(); + for path in clause_toml_paths(rfc_path)? { + let clause = read_clause(config, &path)?; + if clause.since.is_none() { + pending_clauses.push((path, clause)); + } + } // Sort by clause_id for deterministic output order. pending_clauses.sort_by_key(|(_, c)| c.clause_id.clone()); + let mut updated_clause_ids = Vec::with_capacity(pending_clauses.len()); for (path, mut clause) in pending_clauses { clause.since = Some(version.to_string()); write_clause(&path, &clause, op, Some(&config.display_path(&path)))?; - if !op.is_preview() { - ui::sub_info(format!("Set {}.since = {}", clause.clause_id, version)); - } + updated_clause_ids.push(clause.clause_id); } - Ok(()) + Ok(updated_clause_ids) } diff --git a/src/cmd/lifecycle/rfc_supersede.rs b/src/cmd/lifecycle/rfc_supersede.rs index c12fda61..2c8004c4 100644 --- a/src/cmd/lifecycle/rfc_supersede.rs +++ b/src/cmd/lifecycle/rfc_supersede.rs @@ -4,7 +4,7 @@ use crate::diagnostic::{Diagnostic, DiagnosticCode, DiagnosticResult, Diagnostic use crate::model::RfcStatus; use crate::ui; use crate::validate::is_valid_status_transition; -use crate::write::{WriteOp, read_rfc, today, write_rfc}; +use crate::write::{WriteOp, read_rfc, today, with_file_transaction, write_rfc}; pub(super) fn supersede_rfc( config: &Config, @@ -33,17 +33,23 @@ pub(super) fn supersede_rfc( replacement.supersedes = Some(rfc_id.to_string()); replacement.updated = Some(today); - write_rfc( - &rfc_path, - &source, + with_file_transaction( + &[rfc_path.as_path(), replacement_path.as_path()], op, - Some(&config.display_path(&rfc_path)), - )?; - write_rfc( - &replacement_path, - &replacement, - op, - Some(&config.display_path(&replacement_path)), + || { + write_rfc( + &rfc_path, + &source, + op, + Some(&config.display_path(&rfc_path)), + )?; + write_rfc( + &replacement_path, + &replacement, + op, + Some(&config.display_path(&replacement_path)), + ) + }, )?; if !op.is_preview() { diff --git a/src/diagnostic/code/metadata.rs b/src/diagnostic/code/metadata.rs index d1a5305c..61d956a3 100644 --- a/src/diagnostic/code/metadata.rs +++ b/src/diagnostic/code/metadata.rs @@ -44,6 +44,8 @@ pub(super) fn code(code: &DiagnosticCode) -> &'static str { DiagnosticCode::E0209ClauseAlreadySuperseded => "E0209", DiagnosticCode::E0210ClauseInvalidIdFormat => "E0210", DiagnosticCode::E0211ClauseStillReferenced => "E0211", + DiagnosticCode::E0212ClauseSupersessionCycle => "E0212", + DiagnosticCode::E0213ClauseSupersededByMissing => "E0213", // E03xx - ADR DiagnosticCode::E0301AdrSchemaInvalid => "E0301", DiagnosticCode::E0302AdrNotFound => "E0302", diff --git a/src/diagnostic/code/mod.rs b/src/diagnostic/code/mod.rs index 64e172bf..540de33a 100644 --- a/src/diagnostic/code/mod.rs +++ b/src/diagnostic/code/mod.rs @@ -40,6 +40,8 @@ pub enum DiagnosticCode { E0209ClauseAlreadySuperseded, E0210ClauseInvalidIdFormat, E0211ClauseStillReferenced, + E0212ClauseSupersessionCycle, + E0213ClauseSupersededByMissing, // ADR errors (E03xx) E0301AdrSchemaInvalid, diff --git a/src/validate/fields/mod.rs b/src/validate/fields/mod.rs index 900dedce..9cb0176f 100644 --- a/src/validate/fields/mod.rs +++ b/src/validate/fields/mod.rs @@ -34,7 +34,7 @@ enum FieldValidation { None, /// Must be valid semver (e.g., "1.2.3"). Semver, - /// Must be a valid clause reference within same RFC, target must be active. + /// Must be a valid clause reference to a distinct, active target. ClauseSupersededBy, /// Must be a valid artifact reference (RFC-xxx, ADR-xxx, etc.). ArtifactRef, @@ -93,23 +93,22 @@ fn validate_semver(value: &str) -> DiagnosticResult<()> { Ok(()) } -fn validate_clause_superseded_by(ctx: &ValidationContext, target: &str) -> DiagnosticResult<()> { - if target.is_empty() { - return Ok(()); - } - - let (source_rfc, source_clause) = ctx.artifact_id.split_once(':').ok_or_else(|| { +pub(crate) fn normalize_clause_supersession_target( + source_id: &str, + target: &str, +) -> DiagnosticResult { + let (source_rfc, source_clause) = source_id.split_once(':').ok_or_else(|| { Diagnostic::new( DiagnosticCode::E0210ClauseInvalidIdFormat, - format!("Invalid clause ID format: {}", ctx.artifact_id), - ctx.artifact_id, + format!("Invalid clause ID format: {source_id}"), + source_id, ) })?; if source_rfc.is_empty() || source_clause.is_empty() { return Err(Diagnostic::new( DiagnosticCode::E0210ClauseInvalidIdFormat, - format!("Invalid clause ID format: {}", ctx.artifact_id), - ctx.artifact_id, + format!("Invalid clause ID format: {source_id}"), + source_id, )); } @@ -118,7 +117,6 @@ fn validate_clause_superseded_by(ctx: &ValidationContext, target: &str) -> Diagn } else { format!("{source_rfc}:{target}") }; - let (target_rfc, target_clause) = full_target.split_once(':').ok_or_else(|| { Diagnostic::new( DiagnosticCode::E0210ClauseInvalidIdFormat, @@ -134,13 +132,21 @@ fn validate_clause_superseded_by(ctx: &ValidationContext, target: &str) -> Diagn )); } - if target_rfc != source_rfc { + Ok(full_target) +} + +fn validate_clause_superseded_by(ctx: &ValidationContext, target: &str) -> DiagnosticResult<()> { + if target.is_empty() { + return Ok(()); + } + + let full_target = normalize_clause_supersession_target(ctx.artifact_id, target)?; + + if full_target == ctx.artifact_id { return Err(Diagnostic::new( - DiagnosticCode::E0206ClauseSupersededByUnknown, - format!( - "superseded_by must reference a clause in the same RFC (got {target_rfc}, expected {source_rfc})" - ), - target, + DiagnosticCode::E0212ClauseSupersessionCycle, + format!("Cannot supersede a clause with itself: {}", ctx.artifact_id), + ctx.artifact_id, )); } diff --git a/src/validate/mod.rs b/src/validate/mod.rs index a07d1a0c..f7337461 100644 --- a/src/validate/mod.rs +++ b/src/validate/mod.rs @@ -29,6 +29,7 @@ use tags::validate_artifact_tags; use work_items::{validate_work_item_descriptions, validate_work_item_legacy_inline_history}; pub use artifact_refs::validate_artifact_ref_edit; +pub(crate) use fields::normalize_clause_supersession_target; pub use fields::{ArtifactKind, validate_field}; pub use lifecycle::{ is_valid_adr_transition, is_valid_phase_transition, is_valid_status_transition, diff --git a/src/validate/rfc.rs b/src/validate/rfc.rs index 5a06916d..3f8d8d26 100644 --- a/src/validate/rfc.rs +++ b/src/validate/rfc.rs @@ -2,7 +2,13 @@ use super::ValidationResult; use crate::config::Config; use crate::diagnostic::{Diagnostic, DiagnosticCode}; use crate::model::{ClauseStatus, ProjectIndex, RfcIndex, RfcPhase, RfcStatus}; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ClauseSupersessionVisit { + Visiting, + Visited, +} pub(super) fn validate_rfc(rfc: &RfcIndex, config: &Config, result: &mut ValidationResult) { let rfc_path_display = config.display_path(&rfc.path).display().to_string(); @@ -102,15 +108,29 @@ pub(super) fn validate_clause_references( config: &Config, result: &mut ValidationResult, ) { - let active_clauses: HashSet = index + let known_clauses: HashSet = index .iter_clauses() - .filter(|(_, c)| c.spec.status == ClauseStatus::Active) .map(|(rfc, c)| format!("{}:{}", rfc.rfc.rfc_id, c.spec.clause_id)) .collect(); + let mut graph = HashMap::new(); + let mut path_by_id = HashMap::new(); for (rfc, clause) in index.iter_clauses() { + if clause.spec.status == ClauseStatus::Superseded && clause.spec.superseded_by.is_none() { + result.diagnostics.push(Diagnostic::new( + DiagnosticCode::E0213ClauseSupersededByMissing, + format!( + "Superseded clause '{}' has no superseded_by target", + clause.spec.clause_id + ), + config.display_path(&clause.path).display().to_string(), + )); + } + if let Some(ref superseded_by) = clause.spec.superseded_by { + let source = format!("{}:{}", rfc.rfc.rfc_id, clause.spec.clause_id); let clause_path_display = config.display_path(&clause.path).display().to_string(); + path_by_id.insert(source.clone(), clause_path_display.clone()); if clause.spec.status != ClauseStatus::Superseded { result.diagnostics.push(Diagnostic::new( DiagnosticCode::E0206ClauseSupersededByUnknown, @@ -128,16 +148,111 @@ pub(super) fn validate_clause_references( format!("{}:{}", rfc.rfc.rfc_id, superseded_by) }; - if !active_clauses.contains(&full_ref) { + if !known_clauses.contains(&full_ref) { result.diagnostics.push(Diagnostic::new( - DiagnosticCode::E0207ClauseSupersededByNotActive, + DiagnosticCode::E0206ClauseSupersededByUnknown, format!( - "Clause '{}' superseded by '{}' which is not active", + "Clause '{}' superseded by unknown clause '{}'", clause.spec.clause_id, superseded_by ), clause_path_display, )); + continue; } + + graph.insert(source, full_ref); + } + } + + let mut state = HashMap::new(); + let mut stack = Vec::new(); + let mut clause_ids: Vec<_> = graph.keys().cloned().collect(); + clause_ids.sort(); + + for clause_id in clause_ids { + if state.contains_key(&clause_id) { + continue; } + + if let Some(cycle) = + detect_clause_supersession_cycle(&clause_id, &graph, &mut state, &mut stack) + { + let cycle_start = cycle.first().unwrap_or(&clause_id); + let path = path_by_id + .get(cycle_start) + .cloned() + .unwrap_or_else(|| config.display_path(&config.rfc_dir()).display().to_string()); + result.diagnostics.push(Diagnostic::new( + DiagnosticCode::E0212ClauseSupersessionCycle, + format!("Clause supersession cycle detected: {}", cycle.join(" -> ")), + path, + )); + break; + } + } +} + +// Persisted `superseded_by` values are direct historical edges per +// [[RFC-0001:C-CLAUSE-STATUS]]. +fn detect_clause_supersession_cycle( + clause_id: &str, + graph: &HashMap, + state: &mut HashMap, + stack: &mut Vec, +) -> Option> { + let mut current = clause_id.to_string(); + loop { + match state.get(¤t) { + Some(ClauseSupersessionVisit::Visiting) => { + let start = stack.iter().position(|id| id == ¤t).unwrap_or(0); + let mut cycle = stack[start..].to_vec(); + cycle.push(current); + return Some(cycle); + } + Some(ClauseSupersessionVisit::Visited) => break, + None => {} + } + + state.insert(current.clone(), ClauseSupersessionVisit::Visiting); + stack.push(current.clone()); + + let Some(target) = graph + .get(¤t) + .filter(|target| graph.contains_key(*target)) + else { + break; + }; + current.clone_from(target); + } + + for visited in stack.drain(..) { + state.insert(visited, ClauseSupersessionVisit::Visited); + } + None +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn long_acyclic_supersession_chain_does_not_overflow_the_stack() { + const EDGE_COUNT: usize = 20_000; + let graph: HashMap<_, _> = (0..EDGE_COUNT) + .map(|index| { + ( + format!("RFC-0001:C-{index}"), + format!("RFC-0001:C-{}", index + 1), + ) + }) + .collect(); + let mut state = HashMap::new(); + let mut stack = Vec::new(); + + assert_eq!( + detect_clause_supersession_cycle("RFC-0001:C-0", &graph, &mut state, &mut stack,), + None + ); + assert_eq!(state.len(), graph.len()); } } diff --git a/src/write/mod.rs b/src/write/mod.rs index aaacfcb1..53dfb782 100644 --- a/src/write/mod.rs +++ b/src/write/mod.rs @@ -3,9 +3,12 @@ //! Implements [[ADR-0006]] global dry-run support for content-modifying commands. //! Implements [[ADR-0012]] prefix-based changelog category parsing. -use crate::diagnostic::{Diagnostic, DiagnosticResult}; +use crate::diagnostic::{Diagnostic, DiagnosticCode, DiagnosticResult}; use crate::ui; -use std::path::Path; +use std::collections::HashSet; +use std::fs::OpenOptions; +use std::io::Write; +use std::path::{Path, PathBuf}; mod artifact; mod artifact_io; @@ -61,9 +64,7 @@ pub fn write_file( let output_path = display_path.unwrap_or(path); match op { WriteOp::Execute => { - std::fs::write(path, content).map_err(|err| { - Diagnostic::io_error("write file", err, output_path.display().to_string()) - })?; + atomic_write_file(path, content, output_path)?; } WriteOp::Preview => { ui::dry_run_file_preview(output_path, content); @@ -72,6 +73,231 @@ pub fn write_file( Ok(()) } +// Write and sync in the target directory before replacing the destination so +// returned lifecycle errors can restore prior content per +// [[RFC-0002:C-LIFECYCLE-VERBS]]. +fn atomic_write_file(path: &Path, content: &str, output_path: &Path) -> DiagnosticResult<()> { + let (target_path, existing_permissions) = inspect_write_target(path, output_path)?; + let parent = target_path + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + .unwrap_or_else(|| Path::new(".")); + + let mut builder = tempfile::Builder::new(); + builder.prefix(".govctl-write-").suffix(".tmp"); + if let Some(ref permissions) = existing_permissions { + builder.permissions(permissions.clone()); + } else { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + builder.permissions(std::fs::Permissions::from_mode(0o666)); + } + } + + let mut temporary = builder.tempfile_in(parent).map_err(|err| { + Diagnostic::io_error( + "create temporary file", + err, + output_path.display().to_string(), + ) + })?; + temporary.write_all(content.as_bytes()).map_err(|err| { + Diagnostic::io_error( + "write temporary file", + err, + output_path.display().to_string(), + ) + })?; + if let Some(permissions) = existing_permissions { + temporary + .as_file() + .set_permissions(permissions) + .map_err(|err| { + Diagnostic::io_error( + "set temporary file permissions", + err, + output_path.display().to_string(), + ) + })?; + } + temporary.as_file().sync_all().map_err(|err| { + Diagnostic::io_error( + "sync temporary file", + err, + output_path.display().to_string(), + ) + })?; + temporary.persist(&target_path).map_err(|err| { + Diagnostic::io_error("replace file", err.error, output_path.display().to_string()) + })?; + Ok(()) +} + +fn inspect_write_target( + path: &Path, + output_path: &Path, +) -> DiagnosticResult<(PathBuf, Option)> { + let target_path = resolve_write_target(path, output_path)?; + match std::fs::metadata(&target_path) { + Ok(metadata) => { + OpenOptions::new() + .write(true) + .open(&target_path) + .map_err(|err| { + Diagnostic::io_error( + "open file for writing", + err, + output_path.display().to_string(), + ) + })?; + Ok((target_path, Some(metadata.permissions()))) + } + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok((target_path, None)), + Err(err) => Err(Diagnostic::io_error( + "read file metadata", + err, + output_path.display().to_string(), + )), + } +} + +fn resolve_write_target(path: &Path, output_path: &Path) -> DiagnosticResult { + const MAX_SYMLINK_DEPTH: usize = 40; + + let mut current = path.to_path_buf(); + let mut visited = HashSet::new(); + let mut traversed = 0; + loop { + match std::fs::symlink_metadata(¤t) { + Ok(metadata) if metadata.file_type().is_symlink() => { + if traversed == MAX_SYMLINK_DEPTH || !visited.insert(current.clone()) { + break; + } + traversed += 1; + let link = std::fs::read_link(¤t).map_err(|err| { + Diagnostic::io_error( + "resolve symbolic link", + err, + output_path.display().to_string(), + ) + })?; + current = if link.is_absolute() { + link + } else { + current + .parent() + .unwrap_or_else(|| Path::new(".")) + .join(link) + }; + } + Ok(_) => return Ok(current), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(current), + Err(err) => { + return Err(Diagnostic::io_error( + "read file metadata", + err, + output_path.display().to_string(), + )); + } + } + } + + Err(Diagnostic::io_error( + "resolve symbolic link", + std::io::Error::other("too many symbolic links"), + output_path.display().to_string(), + )) +} + +struct FileSnapshot { + path: PathBuf, + content: Option>, +} + +/// Run a group of artifact writes with preflight checks and rollback on error. +pub fn with_file_transaction( + paths: &[&Path], + op: WriteOp, + operation: impl FnOnce() -> DiagnosticResult, +) -> DiagnosticResult { + if op.is_preview() { + return operation(); + } + + let mut targets = HashSet::new(); + let mut snapshots = Vec::new(); + for path in paths { + let (target, _) = inspect_write_target(path, path)?; + if !targets.insert(target.clone()) { + continue; + } + let content = match std::fs::read(&target) { + Ok(content) => Some(content), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => None, + Err(err) => { + return Err(Diagnostic::io_error( + "read file before transaction", + err, + path.display().to_string(), + )); + } + }; + snapshots.push(FileSnapshot { + path: target, + content, + }); + } + + match operation() { + Ok(value) => Ok(value), + Err(operation_error) => match rollback_files(snapshots) { + Ok(()) => Err(operation_error), + Err(rollback_error) => Err(Diagnostic::new( + DiagnosticCode::E0903UnexpectedError, + format!( + "{}; transaction rollback failed; governed-artifact restoration may be incomplete: {}", + operation_error.message, rollback_error.message + ), + operation_error.file, + )), + }, + } +} + +fn rollback_files(snapshots: Vec) -> DiagnosticResult<()> { + for snapshot in snapshots.into_iter().rev() { + match snapshot.content { + Some(content) => { + let current = std::fs::read(&snapshot.path).ok(); + if current.as_deref() == Some(content.as_slice()) { + continue; + } + let content = std::str::from_utf8(&content).map_err(|err| { + Diagnostic::new( + DiagnosticCode::E0903UnexpectedError, + format!("Failed to restore non-UTF-8 artifact: {err}"), + snapshot.path.display().to_string(), + ) + })?; + atomic_write_file(&snapshot.path, content, &snapshot.path)?; + } + None => match std::fs::remove_file(&snapshot.path) { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => { + return Err(Diagnostic::io_error( + "remove file during transaction rollback", + err, + snapshot.path.display().to_string(), + )); + } + }, + } + } + Ok(()) +} + /// Create a directory, respecting WriteOp mode. /// /// In Preview mode, shows what directory would be created. @@ -113,3 +339,216 @@ pub fn delete_file(path: &Path, op: WriteOp, display_path: Option<&Path>) -> Dia } Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + #[cfg(unix)] + use std::os::unix::fs::PermissionsExt; + + #[cfg(unix)] + #[test] + fn failed_write_preserves_existing_content() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let path = dir.path().join("artifact.toml"); + std::fs::write(&path, "original")?; + + let original_dir_permissions = std::fs::metadata(dir.path())?.permissions(); + let mut unwritable_dir_permissions = original_dir_permissions.clone(); + unwritable_dir_permissions.set_mode(original_dir_permissions.mode() & !0o222); + std::fs::set_permissions(dir.path(), unwritable_dir_permissions)?; + + let result = write_file(&path, "replacement", WriteOp::Execute, None); + std::fs::set_permissions(dir.path(), original_dir_permissions)?; + + assert!(result.is_err()); + assert_eq!(std::fs::read_to_string(path)?, "original"); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn successful_write_preserves_existing_permissions() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let path = dir.path().join("artifact.toml"); + std::fs::write(&path, "original")?; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o640))?; + + write_file(&path, "replacement", WriteOp::Execute, None)?; + + assert_eq!(std::fs::read_to_string(&path)?, "replacement"); + assert_eq!(std::fs::metadata(path)?.permissions().mode() & 0o777, 0o640); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn write_rejects_read_only_existing_file() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let path = dir.path().join("artifact.toml"); + std::fs::write(&path, "original")?; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o444))?; + + let result = write_file(&path, "replacement", WriteOp::Execute, None); + + assert!(result.is_err()); + assert_eq!(std::fs::read_to_string(path)?, "original"); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn successful_write_preserves_symlink() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let target = dir.path().join("target.toml"); + let link = dir.path().join("artifact.toml"); + std::fs::write(&target, "original")?; + std::os::unix::fs::symlink(&target, &link)?; + + write_file(&link, "replacement", WriteOp::Execute, None)?; + + assert!(std::fs::symlink_metadata(&link)?.file_type().is_symlink()); + assert_eq!(std::fs::read_to_string(target)?, "replacement"); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn successful_write_follows_dangling_symlink() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let target = dir.path().join("target.toml"); + let link = dir.path().join("artifact.toml"); + std::os::unix::fs::symlink("target.toml", &link)?; + + write_file(&link, "replacement", WriteOp::Execute, None)?; + + assert!(std::fs::symlink_metadata(&link)?.file_type().is_symlink()); + assert_eq!(std::fs::read_to_string(target)?, "replacement"); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn successful_write_follows_forty_symlinks() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let target = dir.path().join("target.toml"); + std::fs::write(&target, "original")?; + for index in (0..40).rev() { + let destination = if index == 39 { + PathBuf::from("target.toml") + } else { + PathBuf::from(format!("link-{}.toml", index + 1)) + }; + std::os::unix::fs::symlink(destination, dir.path().join(format!("link-{index}.toml")))?; + } + + write_file( + &dir.path().join("link-0.toml"), + "replacement", + WriteOp::Execute, + None, + )?; + + assert_eq!(std::fs::read_to_string(target)?, "replacement"); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn write_rejects_forty_one_symlinks() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let target = dir.path().join("target.toml"); + std::fs::write(&target, "original")?; + for index in (0..41).rev() { + let destination = if index == 40 { + PathBuf::from("target.toml") + } else { + PathBuf::from(format!("link-{}.toml", index + 1)) + }; + std::os::unix::fs::symlink(destination, dir.path().join(format!("link-{index}.toml")))?; + } + + let result = write_file( + &dir.path().join("link-0.toml"), + "replacement", + WriteOp::Execute, + None, + ); + + assert!(result.is_err()); + assert_eq!(std::fs::read_to_string(target)?, "original"); + Ok(()) + } + + #[test] + fn file_transaction_restores_prior_writes_after_failure() + -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let first = dir.path().join("first.toml"); + let second = dir.path().join("second.toml"); + std::fs::write(&first, "first-original")?; + std::fs::write(&second, "second-original")?; + + let result = with_file_transaction( + &[first.as_path(), second.as_path()], + WriteOp::Execute, + || { + write_file(&first, "first-replacement", WriteOp::Execute, None)?; + Err::<(), _>(Diagnostic::new( + crate::diagnostic::DiagnosticCode::E0903UnexpectedError, + "injected write failure", + second.display().to_string(), + )) + }, + ); + + assert!(result.is_err()); + assert_eq!(std::fs::read_to_string(first)?, "first-original"); + assert_eq!(std::fs::read_to_string(second)?, "second-original"); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn file_transaction_reports_rollback_failure() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let path = dir.path().join("artifact.toml"); + std::fs::write(&path, "original")?; + + let result = with_file_transaction(&[path.as_path()], WriteOp::Execute, || { + write_file(&path, "replacement", WriteOp::Execute, None)?; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o444)).map_err( + |err| Diagnostic::io_error("make file read-only", err, path.display().to_string()), + )?; + Err::<(), _>(Diagnostic::new( + crate::diagnostic::DiagnosticCode::E0903UnexpectedError, + "injected write failure", + path.display().to_string(), + )) + }); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644))?; + + let Err(error) = result else { + return Err("rollback unexpectedly succeeded for a read-only file".into()); + }; + assert_eq!( + error.code, + crate::diagnostic::DiagnosticCode::E0903UnexpectedError + ); + assert!(error.message.contains("transaction rollback failed")); + assert!(error.message.contains("restoration may be incomplete")); + Ok(()) + } + + #[test] + fn successful_write_replaces_existing_content() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let path = dir.path().join("artifact.toml"); + std::fs::write(&path, "original")?; + + write_file(&path, "replacement", WriteOp::Execute, None)?; + + assert_eq!(std::fs::read_to_string(path)?, "replacement"); + Ok(()) + } +} diff --git a/tests/error_tests/rfc_clause_cases/check.rs b/tests/error_tests/rfc_clause_cases/check.rs index 2cf38fa3..295bbf1b 100644 --- a/tests/error_tests/rfc_clause_cases/check.rs +++ b/tests/error_tests/rfc_clause_cases/check.rs @@ -1,11 +1,19 @@ use super::*; -fn rfc_dir(project_dir: &std::path::Path) -> std::path::PathBuf { - project_dir.join("gov/rfc/RFC-0001") +fn rfc_dir_for(project_dir: &std::path::Path, rfc_id: &str) -> std::path::PathBuf { + project_dir.join("gov/rfc").join(rfc_id) } fn write_rfc_toml(project_dir: &std::path::Path, content: &str) -> common::TestResult { - let rfc_dir = rfc_dir(project_dir); + write_rfc_toml_for(project_dir, "RFC-0001", content) +} + +fn write_rfc_toml_for( + project_dir: &std::path::Path, + rfc_id: &str, + content: &str, +) -> common::TestResult { + let rfc_dir = rfc_dir_for(project_dir, rfc_id); fs::create_dir_all(rfc_dir.join("clauses"))?; fs::write(rfc_dir.join("rfc.toml"), content)?; Ok(()) @@ -16,7 +24,16 @@ fn write_clause_toml( file_name: &str, content: &str, ) -> common::TestResult { - let rfc_dir = rfc_dir(project_dir); + write_clause_toml_for(project_dir, "RFC-0001", file_name, content) +} + +fn write_clause_toml_for( + project_dir: &std::path::Path, + rfc_id: &str, + file_name: &str, + content: &str, +) -> common::TestResult { + let rfc_dir = rfc_dir_for(project_dir, rfc_id); fs::create_dir_all(rfc_dir.join("clauses"))?; fs::write(rfc_dir.join("clauses").join(file_name), content)?; Ok(()) @@ -95,6 +112,226 @@ text = "This is the new clause." Ok(()) } +#[test] +fn test_superseded_clause_without_replacement_check() -> common::TestResult { + let temp_dir = init_project()?; + + write_rfc_toml( + temp_dir.path(), + r#"[govctl] +schema = 1 +id = "RFC-0001" +title = "Missing Replacement Test" +version = "1.0.0" +status = "normative" +phase = "stable" +owners = ["test@example.com"] +created = "2026-01-01" + +[[sections]] +title = "Clauses" +clauses = ["clauses/C-OLD.toml"] + +[[changelog]] +version = "1.0.0" +date = "2026-01-01" +added = ["Initial release"] +"#, + )?; + + write_clause_toml( + temp_dir.path(), + "C-OLD.toml", + r#"[govctl] +schema = 1 +id = "C-OLD" +title = "Old Clause" +kind = "normative" +status = "superseded" +since = "1.0.0" + +[content] +text = "This clause is superseded." +"#, + )?; + + let output = run_commands(temp_dir.path(), &[&["check"]])?; + assert!( + output.contains("error[E0213]: Superseded clause 'C-OLD' has no superseded_by target"), + "output: {output}" + ); + assert!(output.ends_with("exit: 1\n\n"), "output: {output}"); + Ok(()) +} + +#[test] +fn test_clause_supersession_cycle_check() -> common::TestResult { + let temp_dir = init_project()?; + + write_rfc_toml( + temp_dir.path(), + r#"[govctl] +schema = 1 +id = "RFC-0001" +title = "Supersession Cycle Test" +version = "1.0.0" +status = "normative" +phase = "stable" +owners = ["test@example.com"] +created = "2026-01-01" + +[[sections]] +title = "Clauses" +clauses = ["clauses/C-ONE.toml", "clauses/C-TWO.toml"] + +[[changelog]] +version = "1.0.0" +date = "2026-01-01" +added = ["Initial release"] +"#, + )?; + + write_clause_toml( + temp_dir.path(), + "C-ONE.toml", + r#"[govctl] +schema = 1 +id = "C-ONE" +title = "Clause One" +kind = "normative" +status = "superseded" +superseded_by = "C-TWO" +since = "1.0.0" + +[content] +text = "Clause one." +"#, + )?; + + write_clause_toml( + temp_dir.path(), + "C-TWO.toml", + r#"[govctl] +schema = 1 +id = "C-TWO" +title = "Clause Two" +kind = "normative" +status = "superseded" +superseded_by = "C-ONE" +since = "1.0.0" + +[content] +text = "Clause two." +"#, + )?; + + let output = run_commands(temp_dir.path(), &[&["check"]])?; + assert!(output.contains("error[E0212]"), "output: {output}"); + assert!( + output.contains("Clause supersession cycle detected"), + "output: {output}" + ); + assert!(output.ends_with("exit: 1\n\n"), "output: {output}"); + Ok(()) +} + +#[test] +fn test_cross_rfc_clause_supersession_cycle_check() -> common::TestResult { + let temp_dir = init_project()?; + + write_rfc_toml_for( + temp_dir.path(), + "RFC-0001", + r#"[govctl] +schema = 1 +id = "RFC-0001" +title = "First RFC" +version = "1.0.0" +status = "normative" +phase = "stable" +owners = ["test@example.com"] +created = "2026-01-01" + +[[sections]] +title = "Clauses" +clauses = ["clauses/C-ONE.toml"] + +[[changelog]] +version = "1.0.0" +date = "2026-01-01" +added = ["Initial release"] +"#, + )?; + write_rfc_toml_for( + temp_dir.path(), + "RFC-0002", + r#"[govctl] +schema = 1 +id = "RFC-0002" +title = "Second RFC" +version = "1.0.0" +status = "normative" +phase = "stable" +owners = ["test@example.com"] +created = "2026-01-01" + +[[sections]] +title = "Clauses" +clauses = ["clauses/C-TWO.toml"] + +[[changelog]] +version = "1.0.0" +date = "2026-01-01" +added = ["Initial release"] +"#, + )?; + write_clause_toml_for( + temp_dir.path(), + "RFC-0001", + "C-ONE.toml", + r#"[govctl] +schema = 1 +id = "C-ONE" +title = "Clause One" +kind = "normative" +status = "superseded" +superseded_by = "RFC-0002:C-TWO" +since = "1.0.0" + +[content] +text = "Clause one." +"#, + )?; + write_clause_toml_for( + temp_dir.path(), + "RFC-0002", + "C-TWO.toml", + r#"[govctl] +schema = 1 +id = "C-TWO" +title = "Clause Two" +kind = "normative" +status = "superseded" +superseded_by = "RFC-0001:C-ONE" +since = "1.0.0" + +[content] +text = "Clause two." +"#, + )?; + + let output = run_commands(temp_dir.path(), &[&["check"]])?; + assert!(output.contains("error[E0212]"), "output: {output}"); + assert!( + output.contains( + "Clause supersession cycle detected: RFC-0001:C-ONE -> RFC-0002:C-TWO -> RFC-0001:C-ONE" + ), + "output: {output}" + ); + assert!(output.ends_with("exit: 1\n\n"), "output: {output}"); + Ok(()) +} + /// Test: RFC has invalid status/phase combination (draft + stable) #[test] fn test_invalid_transition_check() -> common::TestResult { diff --git a/tests/lifecycle_tests/clause.rs b/tests/lifecycle_tests/clause.rs index 44ccd113..bd265a56 100644 --- a/tests/lifecycle_tests/clause.rs +++ b/tests/lifecycle_tests/clause.rs @@ -75,6 +75,567 @@ fn test_supersede_clause() -> common::TestResult { Ok(()) } +#[test] +fn test_supersede_clause_accepts_same_rfc_shorthand_replacement() -> common::TestResult { + let temp_dir = init_project()?; + + let output = run_commands( + temp_dir.path(), + &[ + &["rfc", "new", "Test RFC"], + &[ + "clause", + "new", + "RFC-0001:C-OLD", + "Old Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "new", + "RFC-0001:C-NEW", + "New Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "supersede", + "RFC-0001:C-OLD", + "--by", + "C-NEW", + "--force", + ], + &["clause", "get", "RFC-0001:C-OLD", "superseded_by"], + &["check"], + ], + )?; + + assert!( + output.contains("Superseded clause: RFC-0001:C-OLD\n Replaced by: C-NEW\nexit: 0"), + "output: {output}" + ); + 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"), + "check should accept the shorthand replacement: {output}" + ); + Ok(()) +} + +#[test] +fn test_supersede_clause_chain_remains_valid() -> common::TestResult { + let temp_dir = init_project()?; + + let output = run_commands( + temp_dir.path(), + &[ + &["rfc", "new", "Test RFC"], + &[ + "clause", + "new", + "RFC-0001:C-ONE", + "Clause One", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "new", + "RFC-0001:C-TWO", + "Clause Two", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "new", + "RFC-0001:C-THREE", + "Clause Three", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "supersede", + "RFC-0001:C-ONE", + "--by", + "RFC-0001:C-TWO", + "--force", + ], + &[ + "clause", + "supersede", + "RFC-0001:C-TWO", + "--by", + "RFC-0001:C-THREE", + "--force", + ], + &["clause", "get", "RFC-0001:C-ONE", "superseded_by"], + &["clause", "get", "RFC-0001:C-TWO", "superseded_by"], + &["check"], + ], + )?; + + assert!( + output + .contains("$ govctl clause get RFC-0001:C-ONE superseded_by\nRFC-0001:C-TWO\nexit: 0"), + "output: {output}" + ); + assert!( + output.contains( + "$ govctl clause get RFC-0001:C-TWO superseded_by\nRFC-0001:C-THREE\nexit: 0" + ), + "output: {output}" + ); + assert!(output.ends_with("exit: 0\n\n"), "output: {output}"); + Ok(()) +} + +#[test] +fn test_supersede_clause_history_remains_valid_after_target_is_deprecated() -> common::TestResult { + let temp_dir = init_project()?; + + let output = run_commands( + temp_dir.path(), + &[ + &["rfc", "new", "Test RFC"], + &[ + "clause", + "new", + "RFC-0001:C-OLD", + "Old Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "new", + "RFC-0001:C-REPLACEMENT", + "Replacement Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "supersede", + "RFC-0001:C-OLD", + "--by", + "RFC-0001:C-REPLACEMENT", + "--force", + ], + &["clause", "deprecate", "RFC-0001:C-REPLACEMENT", "--force"], + &["clause", "get", "RFC-0001:C-OLD", "superseded_by"], + &["check"], + ], + )?; + + assert!( + output.contains( + "$ govctl clause get RFC-0001:C-OLD superseded_by\nRFC-0001:C-REPLACEMENT\nexit: 0" + ), + "output: {output}" + ); + assert!(output.ends_with("exit: 0\n\n"), "output: {output}"); + Ok(()) +} + +#[test] +fn test_supersede_deprecated_clause() -> common::TestResult { + let temp_dir = init_project()?; + + let output = run_commands( + temp_dir.path(), + &[ + &["rfc", "new", "Test RFC"], + &[ + "clause", + "new", + "RFC-0001:C-OLD", + "Old Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "new", + "RFC-0001:C-REPLACEMENT", + "Replacement Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &["clause", "deprecate", "RFC-0001:C-OLD", "--force"], + &[ + "clause", + "supersede", + "RFC-0001:C-OLD", + "--by", + "RFC-0001:C-REPLACEMENT", + "--force", + ], + &["clause", "get", "RFC-0001:C-OLD", "superseded_by"], + &["check"], + ], + )?; + + assert!( + output.contains( + "$ govctl clause get RFC-0001:C-OLD superseded_by\nRFC-0001:C-REPLACEMENT\nexit: 0" + ), + "output: {output}" + ); + assert!(output.ends_with("exit: 0\n\n"), "output: {output}"); + Ok(()) +} + +#[test] +fn test_supersede_clause_accepts_qualified_cross_rfc_replacement() -> common::TestResult { + let temp_dir = init_project()?; + + let output = run_commands( + temp_dir.path(), + &[ + &["rfc", "new", "Source RFC"], + &[ + "clause", + "new", + "RFC-0001:C-SOURCE", + "Source Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &["rfc", "new", "Target RFC"], + &[ + "clause", + "new", + "RFC-0002:C-TARGET", + "Target Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "supersede", + "RFC-0001:C-SOURCE", + "--by", + "RFC-0002:C-TARGET", + "--force", + ], + &["clause", "get", "RFC-0001:C-SOURCE", "superseded_by"], + &["check"], + ], + )?; + + assert!( + output.contains( + "$ govctl clause get RFC-0001:C-SOURCE superseded_by\nRFC-0002:C-TARGET\nexit: 0" + ), + "output: {output}" + ); + assert!(output.ends_with("exit: 0\n\n"), "output: {output}"); + Ok(()) +} + +#[test] +fn test_supersede_clause_rejects_unqualified_cross_rfc_replacement() -> common::TestResult { + let temp_dir = init_project()?; + + let output = run_commands( + temp_dir.path(), + &[ + &["rfc", "new", "Source RFC"], + &[ + "clause", + "new", + "RFC-0001:C-SOURCE", + "Source Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &["rfc", "new", "Target RFC"], + &[ + "clause", + "new", + "RFC-0002:C-TARGET", + "Target Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "supersede", + "RFC-0001:C-SOURCE", + "--by", + "C-TARGET", + "--force", + ], + ], + )?; + + assert!( + output.contains("Replacement clause not found: RFC-0001:C-TARGET"), + "output: {output}" + ); + assert!(output.ends_with("exit: 1\n\n"), "output: {output}"); + Ok(()) +} + +#[test] +fn test_supersede_clause_rejects_self_replacement() -> common::TestResult { + let temp_dir = init_project()?; + + let output = run_commands( + temp_dir.path(), + &[ + &["rfc", "new", "Test RFC"], + &[ + "clause", + "new", + "RFC-0001:C-ONE", + "Clause One", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "supersede", + "RFC-0001:C-ONE", + "--by", + "RFC-0001:C-ONE", + "--force", + ], + ], + )?; + + assert!( + output.contains("Cannot supersede a clause with itself: RFC-0001:C-ONE"), + "output: {output}" + ); + assert!(output.contains("error[E0212]"), "output: {output}"); + assert!(output.ends_with("exit: 1\n\n"), "output: {output}"); + Ok(()) +} + +#[test] +fn test_supersede_clause_rejects_deprecated_replacement() -> common::TestResult { + let temp_dir = init_project()?; + + let output = run_commands( + temp_dir.path(), + &[ + &["rfc", "new", "Test RFC"], + &[ + "clause", + "new", + "RFC-0001:C-OLD", + "Old Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "new", + "RFC-0001:C-REPLACEMENT", + "Replacement Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &["clause", "deprecate", "RFC-0001:C-REPLACEMENT", "--force"], + &[ + "clause", + "supersede", + "RFC-0001:C-OLD", + "--by", + "RFC-0001:C-REPLACEMENT", + "--force", + ], + ], + )?; + + assert!( + output.contains("Cannot supersede by a deprecated clause: RFC-0001:C-REPLACEMENT"), + "output: {output}" + ); + assert!(output.ends_with("exit: 1\n\n"), "output: {output}"); + Ok(()) +} + +#[test] +fn test_supersede_clause_rejects_superseded_replacement() -> common::TestResult { + let temp_dir = init_project()?; + + let output = run_commands( + temp_dir.path(), + &[ + &["rfc", "new", "Test RFC"], + &[ + "clause", + "new", + "RFC-0001:C-OLD", + "Old Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "new", + "RFC-0001:C-REPLACEMENT", + "Replacement Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "new", + "RFC-0001:C-LATEST", + "Latest Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "supersede", + "RFC-0001:C-REPLACEMENT", + "--by", + "RFC-0001:C-LATEST", + "--force", + ], + &[ + "clause", + "supersede", + "RFC-0001:C-OLD", + "--by", + "RFC-0001:C-REPLACEMENT", + "--force", + ], + ], + )?; + + assert!( + output.contains("Cannot supersede by a superseded clause: RFC-0001:C-REPLACEMENT"), + "output: {output}" + ); + assert!(output.ends_with("exit: 1\n\n"), "output: {output}"); + Ok(()) +} + +#[test] +fn test_supersede_clause_rejects_repeated_transition() -> common::TestResult { + let temp_dir = init_project()?; + + let output = run_commands( + temp_dir.path(), + &[ + &["rfc", "new", "Test RFC"], + &[ + "clause", + "new", + "RFC-0001:C-OLD", + "Old Clause", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "new", + "RFC-0001:C-FIRST", + "First Replacement", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "new", + "RFC-0001:C-SECOND", + "Second Replacement", + "-s", + "Specification", + "-k", + "normative", + ], + &[ + "clause", + "supersede", + "RFC-0001:C-OLD", + "--by", + "RFC-0001:C-FIRST", + "--force", + ], + &[ + "clause", + "supersede", + "RFC-0001:C-OLD", + "--by", + "RFC-0001:C-SECOND", + "--force", + ], + &["clause", "get", "RFC-0001:C-OLD", "superseded_by"], + ], + )?; + + assert!( + output.contains("Clause is already superseded"), + "output: {output}" + ); + assert!( + output.contains("error[E0209]: Clause is already superseded (RFC-0001:C-OLD)\nexit: 1"), + "output: {output}" + ); + assert!( + output.contains( + "$ govctl clause get RFC-0001:C-OLD superseded_by\nRFC-0001:C-FIRST\nexit: 0" + ), + "output: {output}" + ); + Ok(()) +} + #[test] fn test_supersede_clause_rejects_missing_replacement() -> common::TestResult { let temp_dir = init_project()?; diff --git a/tests/lifecycle_tests/rfc_cases/bump.rs b/tests/lifecycle_tests/rfc_cases/bump.rs index 231f390c..8955072c 100644 --- a/tests/lifecycle_tests/rfc_cases/bump.rs +++ b/tests/lifecycle_tests/rfc_cases/bump.rs @@ -249,3 +249,98 @@ fn test_bump_nonexistent_rfc() -> common::TestResult { assert_lifecycle_snapshot!(normalize_output(&output, temp_dir.path(), &date)?); Ok(()) } + +#[test] +fn test_bump_rejects_malformed_unlisted_clause_without_mutation() -> common::TestResult { + let temp_dir = init_project()?; + run_commands(temp_dir.path(), &[&["rfc", "new", "Test RFC"]])?; + + let rfc_path = temp_dir.path().join("gov/rfc/RFC-0001/rfc.toml"); + let original_rfc = fs::read_to_string(&rfc_path)?; + fs::write( + temp_dir + .path() + .join("gov/rfc/RFC-0001/clauses/C-BROKEN.toml"), + "not valid TOML [", + )?; + + let output = run_commands( + temp_dir.path(), + &[&[ + "rfc", + "bump", + "RFC-0001", + "--patch", + "--summary", + "Rejected bump", + ]], + )?; + + assert!(output.contains("error[E0201]"), "output: {output}"); + assert!(!output.contains("Bumped RFC-0001"), "output: {output}"); + assert_eq!(fs::read_to_string(rfc_path)?, original_rfc); + Ok(()) +} + +#[cfg(unix)] +#[test] +fn test_failed_bump_preserves_files_without_success_output() -> common::TestResult { + use std::os::unix::fs::PermissionsExt; + + let temp_dir = init_project()?; + run_commands( + temp_dir.path(), + &[ + &["rfc", "new", "Test RFC"], + &[ + "clause", + "new", + "RFC-0001:C-PENDING", + "Pending Clause", + "-s", + "Specification", + "-k", + "normative", + ], + ], + )?; + + let rfc_path = temp_dir.path().join("gov/rfc/RFC-0001/rfc.toml"); + let clause_path = temp_dir + .path() + .join("gov/rfc/RFC-0001/clauses/C-PENDING.toml"); + let original_rfc = fs::read_to_string(&rfc_path)?; + let original_clause = fs::read_to_string(&clause_path)?; + let clauses_dir = clause_path + .parent() + .ok_or("pending clause path has no parent directory")?; + let original_dir_permissions = fs::metadata(clauses_dir)?.permissions(); + let mut unwritable_dir_permissions = original_dir_permissions.clone(); + unwritable_dir_permissions.set_mode(original_dir_permissions.mode() & !0o222); + fs::set_permissions(clauses_dir, unwritable_dir_permissions)?; + + let output = run_commands( + temp_dir.path(), + &[&[ + "rfc", + "bump", + "RFC-0001", + "--patch", + "--summary", + "Rejected bump", + ]], + ); + fs::set_permissions(clauses_dir, original_dir_permissions)?; + let output = output?; + + assert!(output.contains("error[E0901]"), "output: {output}"); + assert!(!output.contains("Bumped RFC-0001"), "output: {output}"); + assert!(!output.contains("Added change:"), "output: {output}"); + assert!( + !output.contains("Set C-PENDING.since ="), + "output: {output}" + ); + assert_eq!(fs::read_to_string(rfc_path)?, original_rfc); + assert_eq!(fs::read_to_string(clause_path)?, original_clause); + Ok(()) +} diff --git a/tests/lifecycle_tests/rfc_cases/finalize.rs b/tests/lifecycle_tests/rfc_cases/finalize.rs index 0edcba55..9266bc0d 100644 --- a/tests/lifecycle_tests/rfc_cases/finalize.rs +++ b/tests/lifecycle_tests/rfc_cases/finalize.rs @@ -82,6 +82,31 @@ fn test_finalize_nonexistent_rfc() -> common::TestResult { Ok(()) } +#[test] +fn test_finalize_rejects_malformed_unlisted_clause_without_mutation() -> common::TestResult { + let temp_dir = init_project()?; + run_commands(temp_dir.path(), &[&["rfc", "new", "Test RFC"]])?; + + let rfc_path = temp_dir.path().join("gov/rfc/RFC-0001/rfc.toml"); + let original_rfc = fs::read_to_string(&rfc_path)?; + fs::write( + temp_dir + .path() + .join("gov/rfc/RFC-0001/clauses/C-BROKEN.toml"), + "not valid TOML [", + )?; + + let output = run_commands( + temp_dir.path(), + &[&["rfc", "finalize", "RFC-0001", "normative"]], + )?; + + assert!(output.contains("error[E0201]"), "output: {output}"); + assert!(!output.contains("Finalized RFC-0001"), "output: {output}"); + assert_eq!(fs::read_to_string(rfc_path)?, original_rfc); + Ok(()) +} + #[test] fn test_finalize_legacy_json_rfc_requires_migrate() -> common::TestResult { let (temp_dir, date) = init_project_with_date()?; diff --git a/tests/snapshots/test_errors__broken_superseded_check.snap b/tests/snapshots/test_errors__broken_superseded_check.snap index 409bf1d2..5e31dd27 100644 --- a/tests/snapshots/test_errors__broken_superseded_check.snap +++ b/tests/snapshots/test_errors__broken_superseded_check.snap @@ -10,5 +10,5 @@ Checked: 0 work items 0 verification guards -error[E0207]: Clause 'C-OLD' superseded by 'C-NONEXISTENT' which is not active (gov/rfc/RFC-0001/clauses/C-OLD.toml) +error[E0206]: Clause 'C-OLD' superseded by unknown clause 'C-NONEXISTENT' (gov/rfc/RFC-0001/clauses/C-OLD.toml) exit: 1