feat(tags): controlled-vocabulary tags for governance artifacts#8
Merged
Conversation
…(controlled-vocabulary tags) ADR-0039: SQLite FTS5 as read-only search index — on hold pending tags evaluation. ADR-0040: Controlled-vocabulary tags for governance artifacts — accepted.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds controlled-vocabulary tags: project-level Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (govctl)"
participant CLI as "CLI Parser"
participant Router as "CommandRouter"
participant TagMod as "cmd::tag"
participant Config as "gov/config.toml"
User->>CLI: govctl tag new <tag>
CLI->>Router: parsed TagNew
Router->>TagMod: tag_new(config, tag)
TagMod->>Config: read/update `[tags].allowed`
Config-->>TagMod: updated TOML (write)
TagMod-->>CLI: print "Added tag: <tag>"
CLI-->>User: exit: 0
sequenceDiagram
participant User as "User (govctl)"
participant CLI as "CLI Parser"
participant Router as "CommandRouter"
participant ListMod as "cmd::list"
participant Index as "ProjectIndex (artifact files)"
participant Config as "gov/config.toml"
User->>CLI: govctl adr list --tag a,b
CLI->>Router: parsed List(tags=[a,b])
Router->>ListMod: list(tags=[a,b], filter...)
ListMod->>Index: read artifacts (ADRs, RFCs, Clauses, Work, Guards)
Index-->>ListMod: artifacts w/ metadata (including tags)
Note right of ListMod: filter: artifact.tags contains all [a,b]
ListMod-->>CLI: formatted list output
CLI-->>User: display results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
gov/rfc/RFC-0002/clauses/C-CRUD-VERBS.toml (1)
40-53: Minor inconsistency in reference notation.Lines 40 and 53 reference
RFC-0002:C-OUTPUT-FORMATwithout the[[...]]notation, but other references in this clause (e.g.,[[ADR-0017]]on line 69) still use the double-bracket notation for inline refs.Consider using consistent notation throughout the clause text for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gov/rfc/RFC-0002/clauses/C-CRUD-VERBS.toml` around lines 40 - 53, The clause uses bare references to RFC-0002:C-OUTPUT-FORMAT on the lines that currently read "MUST respect RFC-0002:C-OUTPUT-FORMAT flags"; update those inline references to the consistent double-bracket notation (e.g., [[RFC-0002:C-OUTPUT-FORMAT]]) so they match other references like [[ADR-0017]] and maintain consistent inline-ref formatting throughout the C-CRUD-VERBS text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 12-13: Update the changelog lines to accurately reflect the
changes: change "all four artifact schemas (rfc, adr, work, guard)" to list all
five schemas including "clause" (rfc, clause, adr, work, guard) and replace the
command names "govctl tag add/remove/list" with the actual commands "govctl tag
new", "govctl tag delete", and "govctl tag list" so the entry matches the PR
changes.
In `@docs/rfc/RFC-0002.md`:
- Around line 142-144: The file contains a duplicated metadata marker "*Since:
v0.1.0*" repeated consecutively; remove the redundant "*Since: v0.1.0*"
occurrence so only one "*Since: v0.1.0*" marker remains (search for the
duplicate string "*Since: v0.1.0*" in RFC-0002.md and delete the extra
instance).
- Around line 258-259: Remove the duplicated "*Since: v0.1.0*" marker by keeping
a single occurrence and deleting the extra one; locate the duplicate marker
string "*Since: v0.1.0*" in RFC-0002.md (around the section where "Since" is
documented) and remove the redundant line so only one "*Since: v0.1.0*" remains.
In `@gov/schema/clause.schema.json`:
- Around line 41-45: The "tags" property in clause.schema.json currently allows
arbitrary strings; update its items schema to enforce the controlled-vocabulary
tag format used elsewhere by adding a "pattern" (e.g.,
"^[a-z0-9]+(?:-[a-z0-9]+)*$") and optionally "minLength":1 so each tag is
lowercase alphanumeric with optional hyphen-separated segments; modify the
"tags" definition (the "tags" property in the clause schema) to replace items: {
"type": "string" } with items: { "type":"string",
"pattern":"^[a-z0-9]+(?:-[a-z0-9]+)*$", "minLength":1 } while keeping "default":
[].
In `@src/cmd/new.rs`:
- Line 364: The default empty tags assignment (the `tags: vec![]` field in
src/cmd/new.rs) implements a cross-artifact invariant and must cite the
governing RFC clause; update the code by adding a short inline RFC citation
comment next to the `tags: vec![]` expression (e.g. `// RFC-<number> §<section>:
tags-default invariant`) or introduce a small shared constant like `const
RFC_CLAUSE_TAGS: &str = "RFC-<number> §<section>";` and use that constant in an
inline comment or helper used by the `tags` default; apply the same citation
pattern to the other occurrences of `tags: vec![]` in this module so each
default is explicitly traceable to the RFC clause.
In `@src/cmd/tag.rs`:
- Around line 208-234: The OutputFormat enum lacks Yaml and Toml variants
required by RFC-0002; add Yaml and Toml variants to the OutputFormat definition
(used by the tag_list command) and update the match in tag_list (the shown match
on OutputFormat) to handle OutputFormat::Yaml and OutputFormat::Toml: for Yaml,
serialize entries with serde_yaml::to_string(&entries).unwrap_or_else(|_|
"---\n".to_string()) and print it; for Toml, serialize with
toml::to_string(&entries).unwrap_or_else(|_| String::new()) and print it. Also
ensure the crate imports (serde_yaml and toml) are available and any required
error handling mirrors the existing Json branch behavior.
In `@src/validate.rs`:
- Around line 902-956: validate_artifact_tags is missing validation for guards;
add a loop that mirrors the other artifact loops to call the existing check_tags
closure for each guard's tags (use GuardMeta.tags), using the guard's meta().id
as artifact_id and config.display_path(&guard.path).display().to_string() as
path_display. Locate the guards collection on ProjectIndex (e.g., index.guards
or the appropriate field name) and ensure you call
check_tags(&guard.meta().tags, &guard.meta().id, &path) so guard tags are
validated against TAG_RE and config.tags.allowed.
In `@tests/test_tags.rs`:
- Around line 196-218: The top comment for the
test_artifact_add_unregistered_tag is misleading — it states that adding an
unregistered tag succeeds and validation is later, but the test asserts
immediate rejection (E1105), which is the intended behavior; update the comment
to state that adding an unregistered tag should be rejected at add time (per
RFC-0002) and mention that the test asserts the immediate error (E1105 / "not in
config.toml") so readers aren’t confused about where validation occurs.
---
Nitpick comments:
In `@gov/rfc/RFC-0002/clauses/C-CRUD-VERBS.toml`:
- Around line 40-53: The clause uses bare references to RFC-0002:C-OUTPUT-FORMAT
on the lines that currently read "MUST respect RFC-0002:C-OUTPUT-FORMAT flags";
update those inline references to the consistent double-bracket notation (e.g.,
[[RFC-0002:C-OUTPUT-FORMAT]]) so they match other references like [[ADR-0017]]
and maintain consistent inline-ref formatting throughout the C-CRUD-VERBS text.
🪄 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: 1638db04-3207-4656-8f73-2adacd248c63
📒 Files selected for processing (31)
CHANGELOG.mddocs/rfc/RFC-0002.mdgov/adr/ADR-0039-use-sqlite-fts5-as-read-only-search-index-for-governance-artifacts.tomlgov/adr/ADR-0040-controlled-vocabulary-tags-for-governance-artifacts.tomlgov/config.tomlgov/rfc/RFC-0002/clauses/C-CRUD-VERBS.tomlgov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.tomlgov/rfc/RFC-0002/clauses/C-RESOURCES.tomlgov/rfc/RFC-0002/rfc.tomlgov/schema/adr.schema.jsongov/schema/clause.schema.jsongov/schema/edit-ops.jsongov/schema/guard.schema.jsongov/schema/rfc.schema.jsongov/schema/work.schema.jsongov/work/2026-04-09-implement-controlled-vocabulary-tags-for-governance-artifacts.tomlsrc/cli.rssrc/cmd/edit/mod.rssrc/cmd/guard.rssrc/cmd/list.rssrc/cmd/mod.rssrc/cmd/new.rssrc/cmd/tag.rssrc/command_router.rssrc/config.rssrc/diagnostic.rssrc/model.rssrc/render.rssrc/resource_plan.rssrc/validate.rstests/test_tags.rs
| updated: None, | ||
| supersedes: None, | ||
| refs: vec![], | ||
| tags: vec![], |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit RFC clause citations for the tags-default invariant.
These new tags: vec![] defaults implement a cross-artifact invariant, but the changed lines do not cite the governing RFC clause. Please add short inline references (or a shared helper with one citation) to keep implementation-to-spec traceability explicit.
Example (inline annotation pattern)
- tags: vec![],
+ tags: vec![], // Default empty controlled-vocabulary tags per [[RFC-0002:C-RESOURCE-MODEL]]Also applies to: 452-452, 538-538, 627-627
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cmd/new.rs` at line 364, The default empty tags assignment (the `tags:
vec![]` field in src/cmd/new.rs) implements a cross-artifact invariant and must
cite the governing RFC clause; update the code by adding a short inline RFC
citation comment next to the `tags: vec![]` expression (e.g. `// RFC-<number>
§<section>: tags-default invariant`) or introduce a small shared constant like
`const RFC_CLAUSE_TAGS: &str = "RFC-<number> §<section>";` and use that constant
in an inline comment or helper used by the `tags` default; apply the same
citation pattern to the other occurrences of `tags: vec![]` in this module so
each default is explicitly traceable to the RFC clause.
Comment on lines
+208
to
+234
| match output { | ||
| OutputFormat::Json => { | ||
| println!( | ||
| "{}", | ||
| serde_json::to_string_pretty(&entries).unwrap_or_else(|_| "[]".to_string()) | ||
| ); | ||
| } | ||
| OutputFormat::Plain => { | ||
| for e in &entries { | ||
| println!("{}\t{}", e.tag, e.usage); | ||
| } | ||
| } | ||
| OutputFormat::Table => { | ||
| let mut table_out = Table::new(); | ||
| table_out | ||
| .load_preset(UTF8_FULL) | ||
| .set_content_arrangement(ContentArrangement::Dynamic) | ||
| .set_header(vec![ | ||
| Cell::new("Tag").add_attribute(Attribute::Bold), | ||
| Cell::new("Usage").add_attribute(Attribute::Bold), | ||
| ]); | ||
| for e in &entries { | ||
| table_out.add_row(vec![Cell::new(&e.tag), Cell::new(e.usage.to_string())]); | ||
| } | ||
| println!("{table_out}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check OutputFormat enum variants to verify all are handled
ast-grep --pattern 'enum OutputFormat {
$$$
}'Repository: govctl-org/govctl
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for OutputFormat enum definition
rg "enum OutputFormat" -A 10 -B 2Repository: govctl-org/govctl
Length of output: 530
🏁 Script executed:
#!/bin/bash
# Also check if RFC-0002 exists and what it specifies about output formats
fd -type f "RFC-0002*" gov/rfcRepository: govctl-org/govctl
Length of output: 231
🏁 Script executed:
#!/bin/bash
# Search for the complete match statement in tag.rs to see all branches
rg "match output \{" -A 30 src/cmd/tag.rsRepository: govctl-org/govctl
Length of output: 1027
🏁 Script executed:
#!/bin/bash
# Search for RFC-0002 in gov/rfc directory
find gov/rfc -name "*002*" -type fRepository: govctl-org/govctl
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Also check if there are any other output-related RFCs
ls -la gov/rfc/ 2>/dev/null || echo "gov/rfc not found, trying other paths"
fd "RFC.*0002" gov/Repository: govctl-org/govctl
Length of output: 472
🏁 Script executed:
#!/bin/bash
# Read RFC-0002 document to check output format specifications
find gov/rfc/RFC-0002 -name "*.toml" -o -name "*.md" | head -5Repository: govctl-org/govctl
Length of output: 292
🏁 Script executed:
#!/bin/bash
# Read the RFC-0002 file
cat gov/rfc/RFC-0002/rfc.tomlRepository: govctl-org/govctl
Length of output: 2891
🏁 Script executed:
#!/bin/bash
# Read the C-OUTPUT-FORMAT clause to see what output formats are specified
cat gov/rfc/RFC-0002/clauses/C-OUTPUT-FORMAT.tomlRepository: govctl-org/govctl
Length of output: 2539
OutputFormat enum missing Yaml and Toml variants required by RFC-0002:C-OUTPUT-FORMAT.
RFC-0002:C-OUTPUT-FORMAT is normative and specifies that all list commands must support table, json, and yaml output formats. The OutputFormat enum in src/cli.rs only defines Table, Json, and Plain variants, omitting Yaml and Toml entirely. This means tag_list cannot comply with the RFC requirement. The match statement here is exhaustive for the current enum, but the enum itself is incomplete. Add Yaml and Toml variants to OutputFormat and handle them in this match block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cmd/tag.rs` around lines 208 - 234, The OutputFormat enum lacks Yaml and
Toml variants required by RFC-0002; add Yaml and Toml variants to the
OutputFormat definition (used by the tag_list command) and update the match in
tag_list (the shown match on OutputFormat) to handle OutputFormat::Yaml and
OutputFormat::Toml: for Yaml, serialize entries with
serde_yaml::to_string(&entries).unwrap_or_else(|_| "---\n".to_string()) and
print it; for Toml, serialize with toml::to_string(&entries).unwrap_or_else(|_|
String::new()) and print it. Also ensure the crate imports (serde_yaml and toml)
are available and any required error handling mirrors the existing Json branch
behavior.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
tests/test_tags.rs (1)
253-258: Fail fast if[govctl]table is missing before mutation.The conditional mutation can silently skip injecting
tagswhen fixture shape changes. Prefer an explicitexpect(...)to keep this test deterministic and diagnostics crisp.♻️ Suggested change
- if let Some(govctl) = doc.get_mut("govctl").and_then(|v| v.as_table_mut()) { - govctl.insert( - "tags".into(), - toml::Value::Array(vec![toml::Value::String("unknown-tag".into())]), - ); - } + let govctl = doc + .get_mut("govctl") + .and_then(|v| v.as_table_mut()) + .expect("expected top-level [govctl] table in ADR document"); + govctl.insert( + "tags".into(), + toml::Value::Array(vec![toml::Value::String("unknown-tag".into())]), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_tags.rs` around lines 253 - 258, The test currently mutates the TOML doc only if doc.get_mut("govctl").and_then(|v| v.as_table_mut()) returns Some, which can silently skip inserting "tags" when the fixture shape changes; replace that conditional with an explicit assertion (e.g., expect or unwrap) on doc.get_mut("govctl").and_then(|v| v.as_table_mut()) before calling govctl.insert so the test fails fast and deterministically if the [govctl] table is missing—locate the mutation around govctl and change the conditional to an explicit expect/unwrap on the table reference.src/command_router.rs (1)
212-213: Add focused planner tests for new tag operations.Please add assertions for
TagListlock disposition andCommands::Tag→BuiltinOp::{TagNew, TagDelete, TagList}mapping to prevent regressions in routing semantics.Also applies to: 871-883
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command_router.rs` around lines 212 - 213, Add focused planner unit tests that assert the lock disposition for TagList and verify that Commands::Tag variants map to the correct BuiltinOp variants (TagNew, TagDelete, TagList) in the command routing logic; specifically, create tests (e.g., test_tag_lock_disposition and test_commands_tag_to_builtin_mapping) that build the planner input for tag operations, assert the produced Op is Op::Builtin(BuiltinOp::TagList { .. }) (with the expected lock mode/flags) for list semantics, and assert Commands::Tag -> BuiltinOp::TagNew/TagDelete/TagList mapping for create/delete/list flows so regressions in routing semantics are caught.src/cmd/list.rs (1)
173-176: Extract a shared helper for tag matching to avoid semantic drift.The same ALL-tags predicate is duplicated in five list paths. A single helper will keep behavior consistent and reduce future divergence.
♻️ Suggested refactor
+fn has_all_tags(item_tags: &[String], query_tags: &[String]) -> bool { + query_tags.iter().all(|t| item_tags.contains(t)) +} ... - if !tags.is_empty() { - rfcs.retain(|r| tags.iter().all(|t| r.rfc.tags.contains(t))); - } + if !tags.is_empty() { + rfcs.retain(|r| has_all_tags(&r.rfc.tags, tags)); + }Apply similarly in clauses/ADRs/guards/work-items.
Also applies to: 250-253, 316-319, 383-386, 438-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/list.rs` around lines 173 - 176, Extract the duplicated ALL-tags predicate into a single helper (e.g. has_all_tags) and replace the inline closures like rfcs.retain(|r| tags.iter().all(|t| r.rfc.tags.contains(t))); with calls to that helper; implement helper to take the artifact's tag collection and the requested tags (for example fn has_all_tags(artifact_tags: &Vec<String>, tags: &Vec<String>) -> bool { tags.iter().all(|t| artifact_tags.contains(t)) }) and apply the same replacement in the other list branches (clauses, ADRs, guards, work-items) so all tag-matching uses the same function and behavior remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cmd/list.rs`:
- Around line 173-176: The ALL-tags invariant implemented by the retain/filter
expression (rfcs.retain(|r| tags.iter().all(|t| r.rfc.tags.contains(t))); and
the equivalent closures at the other listed locations) needs an inline RFC
clause citation; update each such predicate to include a short comment
immediately above or inline (e.g. "// per RFC <clause>") naming the governing
RFC clause(s) that mandate "artifact must have ALL specified tags" so the
invariant is traceable to the spec—add the citation next to the rfcs.retain(...)
closure(s) that call tags.iter().all(...) and ensure the comment text references
the exact clause identifier used by the project.
In `@src/command_router.rs`:
- Around line 171-172: The doc comment on the struct field tags: Vec<String>
currently states the invariant “artifact must have ALL specified tags” but lacks
RFC traceability; update that comment (the documentation line immediately above
the tags field) to append the specific normative clause identifiers from the
applicable RFC(s) (e.g. “(see RFC <ID> §<clause>” or “RFC-<ID>-<clause>”) so the
invariant is traceable, and follow the repository’s citation format for
src/**/*.rs comment style when adding the clause references.
In `@src/validate.rs`:
- Around line 941-945: The loop currently computes path from rfc.path, causing
tag errors to be reported on the RFC file; change it to use the clause's own
path so diagnostics target the clause TOML. Specifically, in the loop over
index.iter_clauses() replace use of rfc.path with the clause's path (use
clause.path) when creating path and passing it to check_tags(&clause.spec.tags,
&clause_id, &path), so the diagnostic is reported against the clause file that
owns the tags.
- Around line 923-932: The current guard `!allowed.is_empty()` skips emitting
E1105 when the allowed set is empty, effectively treating an empty allowlist as
"allow anything"; change the check so every used tag is validated against
`allowed` regardless of its size. Replace the conditional that currently reads
`if !allowed.is_empty() && !allowed.contains(tag)` with a straight membership
test `if !allowed.contains(tag)` so
`result.diagnostics.push(Diagnostic::new(DiagnosticCode::E1105TagUnknown, ...))`
runs for any tag not present in `allowed` (use the existing `artifact_id`,
`tag`, and `path_display` values). Ensure `allowed` is still a well-defined
collection before this check.
In `@tests/test_tags.rs`:
- Around line 213-218: The test only checks error text in the "adr add with
unregistered tag" block but doesn't assert the command actually failed; update
the test in tests/test_tags.rs to also assert the second command's exit is
non‑zero by checking the Command status (e.g., ensure !status.success() or
status.code().map_or(true, |c| c != 0)) for the add invocation that produced
output, so the test verifies behavioral failure as well as message content;
reference the existing output variable and the status/result value returned by
the Command invocation used for the "add" step.
---
Nitpick comments:
In `@src/cmd/list.rs`:
- Around line 173-176: Extract the duplicated ALL-tags predicate into a single
helper (e.g. has_all_tags) and replace the inline closures like rfcs.retain(|r|
tags.iter().all(|t| r.rfc.tags.contains(t))); with calls to that helper;
implement helper to take the artifact's tag collection and the requested tags
(for example fn has_all_tags(artifact_tags: &Vec<String>, tags: &Vec<String>) ->
bool { tags.iter().all(|t| artifact_tags.contains(t)) }) and apply the same
replacement in the other list branches (clauses, ADRs, guards, work-items) so
all tag-matching uses the same function and behavior remains consistent.
In `@src/command_router.rs`:
- Around line 212-213: Add focused planner unit tests that assert the lock
disposition for TagList and verify that Commands::Tag variants map to the
correct BuiltinOp variants (TagNew, TagDelete, TagList) in the command routing
logic; specifically, create tests (e.g., test_tag_lock_disposition and
test_commands_tag_to_builtin_mapping) that build the planner input for tag
operations, assert the produced Op is Op::Builtin(BuiltinOp::TagList { .. })
(with the expected lock mode/flags) for list semantics, and assert Commands::Tag
-> BuiltinOp::TagNew/TagDelete/TagList mapping for create/delete/list flows so
regressions in routing semantics are caught.
In `@tests/test_tags.rs`:
- Around line 253-258: The test currently mutates the TOML doc only if
doc.get_mut("govctl").and_then(|v| v.as_table_mut()) returns Some, which can
silently skip inserting "tags" when the fixture shape changes; replace that
conditional with an explicit assertion (e.g., expect or unwrap) on
doc.get_mut("govctl").and_then(|v| v.as_table_mut()) before calling
govctl.insert so the test fails fast and deterministically if the [govctl] table
is missing—locate the mutation around govctl and change the conditional to an
explicit expect/unwrap on the table reference.
🪄 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: 661b785e-dc40-47be-b89f-0c6b7ec4400c
📒 Files selected for processing (29)
CHANGELOG.mddocs/rfc/RFC-0002.mdgov/config.tomlgov/rfc/RFC-0002/clauses/C-CRUD-VERBS.tomlgov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.tomlgov/rfc/RFC-0002/clauses/C-RESOURCES.tomlgov/rfc/RFC-0002/rfc.tomlgov/schema/adr.schema.jsongov/schema/clause.schema.jsongov/schema/edit-ops.jsongov/schema/guard.schema.jsongov/schema/rfc.schema.jsongov/schema/work.schema.jsongov/work/2026-04-09-implement-controlled-vocabulary-tags-for-governance-artifacts.tomlsrc/cli.rssrc/cmd/edit/mod.rssrc/cmd/guard.rssrc/cmd/list.rssrc/cmd/mod.rssrc/cmd/new.rssrc/cmd/tag.rssrc/command_router.rssrc/config.rssrc/diagnostic.rssrc/model.rssrc/render.rssrc/resource_plan.rssrc/validate.rstests/test_tags.rs
✅ Files skipped from review due to trivial changes (13)
- src/cmd/guard.rs
- src/cmd/mod.rs
- gov/schema/guard.schema.json
- CHANGELOG.md
- src/cmd/new.rs
- src/render.rs
- src/cmd/edit/mod.rs
- gov/schema/adr.schema.json
- gov/schema/work.schema.json
- gov/config.toml
- gov/work/2026-04-09-implement-controlled-vocabulary-tags-for-governance-artifacts.toml
- src/cmd/tag.rs
- gov/schema/edit-ops.json
🚧 Files skipped from review as they are similar to previous changes (5)
- gov/schema/clause.schema.json
- gov/rfc/RFC-0002/clauses/C-CRUD-VERBS.toml
- src/config.rs
- gov/rfc/RFC-0002/clauses/C-RESOURCES.toml
- gov/rfc/RFC-0002/rfc.toml
Comment on lines
+173
to
+176
| // Filter by tags (artifact must have ALL specified tags) | ||
| if !tags.is_empty() { | ||
| rfcs.retain(|r| tags.iter().all(|t| r.rfc.tags.contains(t))); | ||
| } |
There was a problem hiding this comment.
Add RFC clause citations next to the ALL-tags invariant.
These blocks implement a normative invariant (“must match all tags”) but don’t cite the governing RFC clause(s). Please add clause references inline where the predicate is enforced.
As per coding guidelines src/**/*.rs: Cite RFC clauses when implementing invariants in code.
Also applies to: 250-253, 316-319, 383-386, 438-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cmd/list.rs` around lines 173 - 176, The ALL-tags invariant implemented
by the retain/filter expression (rfcs.retain(|r| tags.iter().all(|t|
r.rfc.tags.contains(t))); and the equivalent closures at the other listed
locations) needs an inline RFC clause citation; update each such predicate to
include a short comment immediately above or inline (e.g. "// per RFC <clause>")
naming the governing RFC clause(s) that mandate "artifact must have ALL
specified tags" so the invariant is traceable to the spec—add the citation next
to the rfcs.retain(...) closure(s) that call tags.iter().all(...) and ensure the
comment text references the exact clause identifier used by the project.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
docs/rfc/RFC-0002.md (1)
142-145:⚠️ Potential issue | 🟡 MinorRemove the duplicated
*Since: v0.1.0*markers.The generated output still repeats the same marker in three sections. Please fix the source/render step so each section emits it only once.
Also applies to: 258-259, 588-590
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/rfc/RFC-0002.md` around lines 142 - 145, The document contains duplicated "*Since: v0.1.0*" markers; update the rendering/source step that emits section metadata so it emits the "Since" tag only once per section—locate the template or function responsible for metadata emission (search for the generator function or template that outputs the literal "*Since: v0.1.0*" or any helper named like renderSectionMetadata/emitSinceTag/emit_since) and change it to either deduplicate by checking if a "Since" entry already exists before appending or consolidate the metadata emission into a single code path (or add a post-render pass to collapse duplicate "*Since: ..." lines) so each section outputs a single "*Since: vX*" marker.tests/test_tags.rs (1)
213-220:⚠️ Potential issue | 🟡 MinorAssert non-zero for the add step, not specifically
exit: 1.This test is stricter than the rest of the file and will fail for the wrong reason if the command exits with
2instead of1. Parse the second command’s exit line and assert non-zero, as the other failure-path tests already do.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_tags.rs` around lines 213 - 220, The test currently asserts output.contains("exit: 1") which is too strict; change it to parse the command exit line from the second command's output (using the existing pattern used elsewhere in this file) and assert the parsed exit code is non-zero instead of equal to 1; update the assertion that references the "add" step to use the parsed exit code (variable output) and keep the existing E1105 / "not in config.toml" message check intact so only the exit-code check becomes a non-zero assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/rfc/RFC-0002.md`:
- Around line 616-618: The v0.7.0 changelog entry omits that tag support was
added for clauses; update the changelog text in the v0.7.0 section to list
"clauses" alongside RFC/ADR/work/guard (e.g., "tags field on
RFC/ADR/work/guard/clauses") and, if present, also update the corresponding
mention of the registry CLI flags (--tag filter and govctl tag new/delete/list)
to reflect that clauses are covered; modify the single line describing v0.7.0 so
it explicitly includes "clauses".
In `@src/cmd/edit/mod.rs`:
- Around line 1121-1123: Remove the special-case that lets any tag through when
config.tags.allowed is empty: replace the conditional that currently checks
"!allowed.is_empty() && !allowed.iter().any(|t| t == value)" with a
membership-only check so that if allowed.iter().any(|t| t == value) is false you
return the Diagnostic; i.e., always validate "value" against config.tags.allowed
(reference variables/symbols: allowed, config.tags.allowed, value, and the
Diagnostic::new return) so unregistered tags are rejected even when the allowed
list is empty.
In `@src/cmd/tag.rs`:
- Around line 82-124: Change count_tag_usage to return Result<usize, E> and stop
swallowing loader errors: update the signature of count_tag_usage(config:
&Config) -> Result<usize, _>, replace the if let Ok(...) { ... } branches with
direct calls using the ? operator (e.g. let rfcs = load_rfcs(config)?; let adrs
= load_adrs(config)?; let items = load_work_items(config)?; let guards =
load_guards_with_warnings(config)?;) and keep the existing counting logic but
operating on those results; then propagate the Result to callers (the code paths
that call count_tag_usage such as the tag delete and tag list handlers) so they
handle/unwrap errors appropriately or return an error upward. Ensure you use the
crate’s existing error type (or anyhow) for the Result error parameter and
adjust caller error handling to match.
- Around line 69-80: The function set_allowed_tags currently panics when the
"tags" key exists but is not a table (via .expect("tags is a table")); change
set_allowed_tags to return a Result (e.g., Result<(), Error> or
anyhow::Result<()>) and detect whether the existing entry is a table using
as_table_mut(); if it's Some(table) proceed, if it's None return a descriptive
error (e.g., "config: 'tags' exists but is not a table") instead of panicking,
and update callers to propagate/handle the error from set_allowed_tags; keep
references to set_allowed_tags and the tags_table lookup so you modify the same
logic path.
In `@tests/test_tags.rs`:
- Around line 293-310: The test currently only asserts that run_commands output
shows "check" exited 0, which can hide a failing "adr add ... tags" step; after
invoking run_commands (the sequence that includes &["adr", "add", "ADR-0001",
"tags", "caching"]), explicitly verify the add succeeded by asserting the
run_commands output contains the add-step success message or by invoking the CLI
to read back the ADR tags (e.g., call the same run_commands helper with a "adr
show" or "adr tags ADR-0001" action) and assert the returned output includes
"caching" before proceeding to the final "check" assertion; modify the test
around run_commands/ADR-0001 to include that intermediate assertion so the test
fails if tag addition regresses.
---
Duplicate comments:
In `@docs/rfc/RFC-0002.md`:
- Around line 142-145: The document contains duplicated "*Since: v0.1.0*"
markers; update the rendering/source step that emits section metadata so it
emits the "Since" tag only once per section—locate the template or function
responsible for metadata emission (search for the generator function or template
that outputs the literal "*Since: v0.1.0*" or any helper named like
renderSectionMetadata/emitSinceTag/emit_since) and change it to either
deduplicate by checking if a "Since" entry already exists before appending or
consolidate the metadata emission into a single code path (or add a post-render
pass to collapse duplicate "*Since: ..." lines) so each section outputs a single
"*Since: vX*" marker.
In `@tests/test_tags.rs`:
- Around line 213-220: The test currently asserts output.contains("exit: 1")
which is too strict; change it to parse the command exit line from the second
command's output (using the existing pattern used elsewhere in this file) and
assert the parsed exit code is non-zero instead of equal to 1; update the
assertion that references the "add" step to use the parsed exit code (variable
output) and keep the existing E1105 / "not in config.toml" message check intact
so only the exit-code check becomes a non-zero assertion.
🪄 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: 849b3e11-488d-44f4-bc49-387e05e6db4f
📒 Files selected for processing (29)
CHANGELOG.mddocs/rfc/RFC-0002.mdgov/config.tomlgov/rfc/RFC-0002/clauses/C-CRUD-VERBS.tomlgov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.tomlgov/rfc/RFC-0002/clauses/C-RESOURCES.tomlgov/rfc/RFC-0002/rfc.tomlgov/schema/adr.schema.jsongov/schema/clause.schema.jsongov/schema/edit-ops.jsongov/schema/guard.schema.jsongov/schema/rfc.schema.jsongov/schema/work.schema.jsongov/work/2026-04-09-implement-controlled-vocabulary-tags-for-governance-artifacts.tomlsrc/cli.rssrc/cmd/edit/mod.rssrc/cmd/guard.rssrc/cmd/list.rssrc/cmd/mod.rssrc/cmd/new.rssrc/cmd/tag.rssrc/command_router.rssrc/config.rssrc/diagnostic.rssrc/model.rssrc/render.rssrc/resource_plan.rssrc/validate.rstests/test_tags.rs
✅ Files skipped from review due to trivial changes (10)
- src/cmd/mod.rs
- src/cmd/guard.rs
- gov/schema/adr.schema.json
- gov/schema/rfc.schema.json
- gov/config.toml
- gov/schema/work.schema.json
- gov/work/2026-04-09-implement-controlled-vocabulary-tags-for-governance-artifacts.toml
- gov/schema/guard.schema.json
- src/cmd/new.rs
- gov/schema/edit-ops.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/resource_plan.rs
- CHANGELOG.md
- gov/schema/clause.schema.json
- gov/rfc/RFC-0002/clauses/C-RESOURCES.toml
- gov/rfc/RFC-0002/rfc.toml
lucifer1004
force-pushed
the
feat/tag
branch
2 times, most recently
from
50fb0ef to
e933523
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gov/rfc/RFC-0002/clauses/C-CRUD-VERBS.toml (1)
31-42:⚠️ Potential issue | 🟠 MajorResolve the contradiction on
--tagin universallistverbs.The clause currently introduces
--tagbut also says universal operations MUST NOT have resource-specific flags. Since--tagapplies only to a subset of resources, this needs an explicit exception or a rule rewrite to avoid conflicting normative requirements.Suggested wording adjustment
-- MUST NOT have resource-specific flags for universal operations +- MUST NOT have resource-specific flags for universal operations, except flags explicitly defined as conditional on resource capability (e.g., `--tag` for resources that define `tags`)As per coding guidelines:
gov/rfc/**/*.toml: "RFCs are normative and long-lived; they must stay valid even if implementation language or internal representation changes".Also applies to: 116-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gov/rfc/RFC-0002/clauses/C-CRUD-VERBS.toml` around lines 31 - 42, The clause currently conflicts with the universal "MUST NOT have resource-specific flags" rule by introducing a resource-scoped --tag option for the universal list verb; fix by either (A) adding an explicit exception in RFC-0002:C-CRUD-VERBS that allows --tag for the universal "list" verb when applied to tag-bearing resources (rfc, clause, adr, work, guard) and explicitly excluding release, or (B) remove --tag from the universal "list" clause and instead place the --tag requirement into the per-resource list subsections for rfc, clause, adr, work, and guard (and note releases do not carry tags); apply the same change to the related text around lines 116-119 and ensure the normative language (MUST/MUST NOT) is updated accordingly.
♻️ Duplicate comments (5)
docs/rfc/RFC-0002.md (1)
142-145:⚠️ Potential issue | 🟡 MinorRemove duplicate
*Since: v0.1.0*markers.Both sections still contain duplicated consecutive
*Since: v0.1.0*lines.Also applies to: 256-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/rfc/RFC-0002.md` around lines 142 - 145, Remove the duplicated consecutive "*Since: v0.1.0*" markers in RFC-0002.md by keeping only one "*Since: v0.1.0*" line per section; specifically edit the repeated occurrences shown at the first instance around the highlighted diff and the second instance noted (lines referenced in the review as 256-259), deleting the extra duplicate lines so each section has a single "*Since: v0.1.0*" marker.src/cmd/new.rs (1)
364-364:⚠️ Potential issue | 🟡 MinorAdd RFC clause citations next to the default
tagsinvariant.These
tags: vec![]defaults encode a normative invariant, but they still don’t include an inline RFC clause reference for traceability.Suggested patch
- tags: vec![], + tags: vec![], // [[RFC-0002:C-RESOURCES]] default empty controlled-vocabulary tags ... - tags: vec![], + tags: vec![], // [[RFC-0002:C-RESOURCES]] default empty controlled-vocabulary tags ... - tags: vec![], + tags: vec![], // [[RFC-0002:C-RESOURCES]] default empty controlled-vocabulary tags ... - tags: vec![], + tags: vec![], // [[RFC-0002:C-RESOURCES]] default empty controlled-vocabulary tagsAs per coding guidelines:
src/**/*.rs: "Cite RFC clauses when implementing invariants in code".Also applies to: 452-452, 538-538, 627-627
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/new.rs` at line 364, The default empty-tags invariant is missing an RFC clause citation: add an inline comment citing the RFC clause(s) that justify the invariant immediately beside the `tags: vec![]` field in the struct literal (and mirror the same comment at the other occurrences you noted), e.g., `tags: vec![], // RFC §X.Y: ...` so reviewers can trace the normative behavior; update the three other locations (the other `tags: vec![]` occurrences you flagged) to include identical RFC clause comments.src/validate.rs (1)
923-932:⚠️ Potential issue | 🟠 MajorDo not treat an empty allowlist as “allow all tags.”
The
!allowed.is_empty()guard lets unregistered tags pass when[tags].allowedis empty, weakening controlled-vocabulary enforcement.Suggested patch
- // Validate against allowed set (only if allowed is non-empty) - if !allowed.is_empty() && !allowed.contains(tag) { + // Validate against allowed set + if !allowed.contains(tag) { result.diagnostics.push(Diagnostic::new( DiagnosticCode::E1105TagUnknown, format!( "Artifact '{artifact_id}' uses unknown tag '{tag}' (not in config.toml [tags] allowed)" ), path_display, )); }As per coding guidelines:
src/**/*.{rs,ts,tsx,js,jsx}: "Code that conflicts with a normative RFC is a bug; no silent deviation allowed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/validate.rs` around lines 923 - 932, The current guard `if !allowed.is_empty() && !allowed.contains(tag)` treats an empty `allowed` set as "allow all"; remove the `!allowed.is_empty()` check so every tag is validated against `allowed` and unknown tags always push the diagnostic. Update the block around `allowed.contains(tag)` (the code that calls `result.diagnostics.push(Diagnostic::new(... DiagnosticCode::E1105TagUnknown ...)))` to run unconditionally when `!allowed.contains(tag)`, ensuring an empty `[tags].allowed` acts as deny-all rather than permit-all.src/cmd/list.rs (1)
173-176:⚠️ Potential issue | 🟡 MinorAdd the exact RFC clause ID next to each ALL-tags predicate.
These closures implement a normative invariant, but the new comments still stop at prose. Please cite the governing clause inline at each
tags.iter().all(...)site so the behavior is traceable from code to spec.As per coding guidelines
src/**/*.rs: Cite RFC clauses when implementing invariants in code.Also applies to: 250-253, 316-319, 383-386, 438-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/list.rs` around lines 173 - 176, The tags filter uses an "ALL-tags" predicate via tags.iter().all(|t| r.rfc.tags.contains(t)) inside rfcs.retain; add an inline code comment immediately adjacent to that closure citing the exact RFC clause ID that mandates this behavior (e.g., "RFC x.y.z") so the invariant is traceable; apply the same change to the other occurrences referenced (lines with rfcs.retain and tags.iter().all / r.rfc.tags.contains at the other sites) so each implementation of the ALL-tags predicate has the precise RFC clause ID documented next to the closure.src/cmd/tag.rs (1)
194-203:⚠️ Potential issue | 🟠 MajorPropagate usage-count failures instead of reporting zero.
unwrap_or(0)reintroduces the silent-underreport bug fortag list: if any loader fails, referenced tags suddenly look unused. Buildentriesas aResultand bubble the error instead.Possible fix
- let entries: Vec<TagEntry> = allowed - .iter() - .map(|tag| { - let usage = count_tag_usage(config, tag).unwrap_or(0); - TagEntry { - tag: tag.clone(), - usage, - } - }) - .collect(); + let entries: Vec<TagEntry> = allowed + .iter() + .map(|tag| { + Ok(TagEntry { + tag: tag.clone(), + usage: count_tag_usage(config, tag)?, + }) + }) + .collect::<Result<_>>()?;As per coding guidelines
src/**/*.{rs,ts,tsx,js,jsx}: Code that conflicts with a normative RFC is a bug; no silent deviation allowed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/tag.rs` around lines 194 - 203, The current mapping uses unwrap_or(0) when calling count_tag_usage, which hides loader errors and reports zero usage; change the construction of entries to propagate errors by building a Result<Vec<TagEntry>, E> instead of Vec<TagEntry>. Replace the map that creates TagEntry with one that returns Result<TagEntry, _> by forwarding the Err from count_tag_usage (remove unwrap_or(0)), then collect::<Result<Vec<_>, _>>() and propagate the error with ? from the surrounding function (update the enclosing function to return Result if needed); target the code around count_tag_usage and the entries Vec<TagEntry> construction to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cmd/tag.rs`:
- Around line 123-145: The tag mutation functions (e.g., tag_new and the
tag_delete counterpart) currently call write_config_table unconditionally and
ignore the global dry-run; add a WriteOp parameter (or an enum indicating DryRun
vs Execute) to the signatures of tag_new and tag_delete, propagate it from the
caller, and branch before persisting: after set_allowed_tags(&mut table,
allowed)? only call write_config_table(config, &table)? when WriteOp indicates
execute; for DryRun return success without writing and still print a summary
message. Ensure you update all call sites to pass the WriteOp and keep other
helpers (validate_tag_format, read_config_table, get_allowed_tags,
set_allowed_tags) unchanged.
- Around line 53-67: The helper get_allowed_tags currently silently returns an
empty Vec when "tags" or "tags.allowed" exist but have the wrong TOML shape;
change get_allowed_tags to validate shapes and fail instead of coercing: when
table.get("tags") is Some but not a table, or when tags.get("allowed") is Some
but not an array of strings, return an Err (or propagate a Result) with a clear
error message describing the offending key and expected type; update callers of
get_allowed_tags to handle the Result. Ensure the validation logic in
get_allowed_tags inspects toml::Value variants (e.g. .as_table(), .as_array(),
.as_str()) and only returns Ok(Vec<String>) for a correct table->array<string>
shape.
In `@tests/test_tags.rs`:
- Around line 178-181: The assertions use the full run_commands transcript
(variable output) and can falsely pass due to earlier commands; update the
assertions that check for "caching" and "ADR-0001" to only inspect the response
from the specific command under test (the `adr get` / `adr list` invocation).
Locate the test in tests/test_tags.rs where run_commands is called and `output`
is produced, slice or extract only the portion produced by the `adr get` (or
`adr list`) command—e.g., by capturing per-command output from `run_commands` or
splitting `output` at the command invocation string—and then assert that that
sliced result contains "caching" or "ADR-0001" respectively. Ensure you update
both occurrences (around the current 178-181 and 338-341 checks) and keep
assertions scoped to the relevant command output variable rather than the whole
transcript.
---
Outside diff comments:
In `@gov/rfc/RFC-0002/clauses/C-CRUD-VERBS.toml`:
- Around line 31-42: The clause currently conflicts with the universal "MUST NOT
have resource-specific flags" rule by introducing a resource-scoped --tag option
for the universal list verb; fix by either (A) adding an explicit exception in
RFC-0002:C-CRUD-VERBS that allows --tag for the universal "list" verb when
applied to tag-bearing resources (rfc, clause, adr, work, guard) and explicitly
excluding release, or (B) remove --tag from the universal "list" clause and
instead place the --tag requirement into the per-resource list subsections for
rfc, clause, adr, work, and guard (and note releases do not carry tags); apply
the same change to the related text around lines 116-119 and ensure the
normative language (MUST/MUST NOT) is updated accordingly.
---
Duplicate comments:
In `@docs/rfc/RFC-0002.md`:
- Around line 142-145: Remove the duplicated consecutive "*Since: v0.1.0*"
markers in RFC-0002.md by keeping only one "*Since: v0.1.0*" line per section;
specifically edit the repeated occurrences shown at the first instance around
the highlighted diff and the second instance noted (lines referenced in the
review as 256-259), deleting the extra duplicate lines so each section has a
single "*Since: v0.1.0*" marker.
In `@src/cmd/list.rs`:
- Around line 173-176: The tags filter uses an "ALL-tags" predicate via
tags.iter().all(|t| r.rfc.tags.contains(t)) inside rfcs.retain; add an inline
code comment immediately adjacent to that closure citing the exact RFC clause ID
that mandates this behavior (e.g., "RFC x.y.z") so the invariant is traceable;
apply the same change to the other occurrences referenced (lines with
rfcs.retain and tags.iter().all / r.rfc.tags.contains at the other sites) so
each implementation of the ALL-tags predicate has the precise RFC clause ID
documented next to the closure.
In `@src/cmd/new.rs`:
- Line 364: The default empty-tags invariant is missing an RFC clause citation:
add an inline comment citing the RFC clause(s) that justify the invariant
immediately beside the `tags: vec![]` field in the struct literal (and mirror
the same comment at the other occurrences you noted), e.g., `tags: vec![], //
RFC §X.Y: ...` so reviewers can trace the normative behavior; update the three
other locations (the other `tags: vec![]` occurrences you flagged) to include
identical RFC clause comments.
In `@src/cmd/tag.rs`:
- Around line 194-203: The current mapping uses unwrap_or(0) when calling
count_tag_usage, which hides loader errors and reports zero usage; change the
construction of entries to propagate errors by building a Result<Vec<TagEntry>,
E> instead of Vec<TagEntry>. Replace the map that creates TagEntry with one that
returns Result<TagEntry, _> by forwarding the Err from count_tag_usage (remove
unwrap_or(0)), then collect::<Result<Vec<_>, _>>() and propagate the error with
? from the surrounding function (update the enclosing function to return Result
if needed); target the code around count_tag_usage and the entries Vec<TagEntry>
construction to implement this change.
In `@src/validate.rs`:
- Around line 923-932: The current guard `if !allowed.is_empty() &&
!allowed.contains(tag)` treats an empty `allowed` set as "allow all"; remove the
`!allowed.is_empty()` check so every tag is validated against `allowed` and
unknown tags always push the diagnostic. Update the block around
`allowed.contains(tag)` (the code that calls
`result.diagnostics.push(Diagnostic::new(... DiagnosticCode::E1105TagUnknown
...)))` to run unconditionally when `!allowed.contains(tag)`, ensuring an empty
`[tags].allowed` acts as deny-all rather than permit-all.
🪄 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: 5e636038-03b7-461d-bd07-55b6056219ef
📒 Files selected for processing (29)
CHANGELOG.mddocs/rfc/RFC-0002.mdgov/config.tomlgov/rfc/RFC-0002/clauses/C-CRUD-VERBS.tomlgov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.tomlgov/rfc/RFC-0002/clauses/C-RESOURCES.tomlgov/rfc/RFC-0002/rfc.tomlgov/schema/adr.schema.jsongov/schema/clause.schema.jsongov/schema/edit-ops.jsongov/schema/guard.schema.jsongov/schema/rfc.schema.jsongov/schema/work.schema.jsongov/work/2026-04-09-implement-controlled-vocabulary-tags-for-governance-artifacts.tomlsrc/cli.rssrc/cmd/edit/mod.rssrc/cmd/guard.rssrc/cmd/list.rssrc/cmd/mod.rssrc/cmd/new.rssrc/cmd/tag.rssrc/command_router.rssrc/config.rssrc/diagnostic.rssrc/model.rssrc/render.rssrc/resource_plan.rssrc/validate.rstests/test_tags.rs
✅ Files skipped from review due to trivial changes (12)
- src/cmd/guard.rs
- src/render.rs
- gov/schema/rfc.schema.json
- gov/schema/clause.schema.json
- gov/schema/work.schema.json
- src/cmd/mod.rs
- gov/schema/adr.schema.json
- gov/config.toml
- src/cmd/edit/mod.rs
- gov/schema/guard.schema.json
- CHANGELOG.md
- gov/work/2026-04-09-implement-controlled-vocabulary-tags-for-governance-artifacts.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- gov/rfc/RFC-0002/clauses/C-RESOURCES.toml
- gov/rfc/RFC-0002/rfc.toml
- src/model.rs
- gov/schema/edit-ops.json
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/validate.rs (1)
923-925:⚠️ Potential issue | 🟠 MajorUnknown-tag validation is incorrectly bypassed when allowlist is empty.
Current logic treats an empty
[tags].allowedas permissive, so unregistered tags can passgovctl check, which weakens controlled-vocabulary enforcement.Suggested fix
- // Validate against allowed set (only if allowed is non-empty) - if !allowed.is_empty() && !allowed.contains(tag) { + // Validate against allowed set + if !allowed.contains(tag) { result.diagnostics.push(Diagnostic::new( DiagnosticCode::E1105TagUnknown, format!( "Artifact '{artifact_id}' uses unknown tag '{tag}' (not in config.toml [tags] allowed)" ), path_display, )); }As per coding guidelines "Code that conflicts with a normative RFC is a bug; no silent deviation allowed".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/validate.rs` around lines 923 - 925, The current check incorrectly skips unknown-tag validation when the `allowed` set is empty; update the condition so that an empty `allowed` does not act as permissive. Specifically, change the branch that currently guards the validation (the `if !allowed.is_empty() && !allowed.contains(tag)` check) to treat emptiness as a failure (e.g., `if allowed.is_empty() || !allowed.contains(tag)`) so you still push the diagnostic via `result.diagnostics.push(Diagnostic::new(...))` when `tag` is not in `allowed`; adjust any accompanying diagnostic message or metadata near the `Diagnostic::new` call to reflect the empty-allowlist case.tests/test_tags.rs (2)
214-217:⚠️ Potential issue | 🟡 MinorAssert non-zero for the add step instead of matching only
exit: 1.This can fail on valid non-zero exits (e.g.,
2) and is not scoped to the second command’s exit line.💚 Suggested fix
- assert!( - output.contains("exit: 1"), - "adr add with unregistered tag should exit non-zero: {output}" - ); + let add_exit = output + .split('\n') + .filter(|l| l.starts_with("exit:")) + .nth(1) + .unwrap_or("exit: 0"); + assert!( + add_exit.contains("exit: 1") || add_exit.contains("exit: 2"), + "adr add with unregistered tag should exit non-zero: {output}" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_tags.rs` around lines 214 - 217, The assertion currently checks for the literal "exit: 1" via output.contains("exit: 1") which fails for other non-zero exits and isn't scoped to the add step; replace that check with logic that extracts the exit code for the add step (e.g., parse the second command's "exit: N" line from output or use a regex like r"exit:\s*([0-9]+)"), convert the captured value to an integer and assert it != 0 so any non-zero exit passes, updating the assertion that references output.contains("exit: 1") accordingly.
178-181:⚠️ Potential issue | 🟡 MinorScope positive assertions to the command-under-test output section.
Both assertions can pass from earlier transcript lines (
adr add ... caching,ADR-0001in command args), even ifadr get/adr listoutput regresses.💚 Suggested fix
- assert!( - output.contains("caching"), - "expected tag in adr get output: {output}" - ); + let get_section = output + .split("$ govctl adr get ADR-0001 tags") + .last() + .unwrap_or(""); + assert!( + get_section.contains("caching"), + "expected tag in adr get output: {output}" + ); ... - assert!( - output.contains("ADR-0001"), - "tagged ADR should appear: {output}" - ); + assert!( + list_section.contains("ADR-0001"), + "tagged ADR should appear in filtered list: {output}" + );Also applies to: 338-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_tags.rs` around lines 178 - 181, The positive assertions are checking the entire test transcript in the output variable, so they can pass due to earlier commands; change the assertions to scope to the specific command-under-test output by extracting the substring for the executed command (e.g., find the index of the command invocation string "adr get" or "adr list" in output, then take the slice from that index to the next command invocation or end of output) and assert the expected tag (e.g., "caching" or "ADR-0001") appears only in that sliced section; update the assertions around the output variable (the contains checks at the shown location and the similar ones at 338-341) to operate on that command-specific slice instead of the whole output.
🧹 Nitpick comments (2)
docs/rfc/RFC-0002.md (1)
575-575: Optional wording polish for repeated sentence starts.This paragraph repeats the same sentence opening pattern several times; a small rephrase would improve readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/rfc/RFC-0002.md` at line 575, The paragraph starting with "`govctl tag` qualifies because it manages project-level configuration that applies across all resource types (criterion 1)" repeats the same sentence opening; edit RFC-0002.md to vary sentence structure and wording for this and the other repeated lines (look for other sentences that begin with "`govctl tag` qualifies..." or the same clause), e.g., rephrase some to passive voice, combine clauses, or start with the criterion or effect instead of the command name so the repeated pattern is avoided while preserving the original meaning.tests/test_tags.rs (1)
23-25: Prefer mutating existing[tags]table over replacing it wholesale.Current helper recreates
tagsand may drop future sibling keys under[tags], making tests less representative as config evolves.♻️ Suggested refactor
- let mut tags_table = toml::Table::new(); - tags_table.insert("allowed".into(), toml::Value::Array(arr)); - doc.insert("tags".into(), toml::Value::Table(tags_table)); + let tags_table = doc + .entry("tags") + .or_insert_with(|| toml::Value::Table(toml::Table::new())) + .as_table_mut() + .expect("[tags] must be a table"); + tags_table.insert("allowed".into(), toml::Value::Array(arr));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_tags.rs` around lines 23 - 25, The test currently recreates the entire "tags" table (using a new tags_table and doc.insert("tags", ...)), which can drop existing sibling keys; instead fetch or create a mutable table and insert/extend the "allowed" array into it: in the test (tests/test_tags.rs) look up doc.get_mut("tags") and if it's a toml::Value::Table use as_table_mut() to insert or update the "allowed" key with arr, otherwise create a new table and insert it via doc.insert("tags", toml::Value::Table(...)); this preserves any existing keys under "tags" while still setting "allowed".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/validate.rs`:
- Around line 923-925: The current check incorrectly skips unknown-tag
validation when the `allowed` set is empty; update the condition so that an
empty `allowed` does not act as permissive. Specifically, change the branch that
currently guards the validation (the `if !allowed.is_empty() &&
!allowed.contains(tag)` check) to treat emptiness as a failure (e.g., `if
allowed.is_empty() || !allowed.contains(tag)`) so you still push the diagnostic
via `result.diagnostics.push(Diagnostic::new(...))` when `tag` is not in
`allowed`; adjust any accompanying diagnostic message or metadata near the
`Diagnostic::new` call to reflect the empty-allowlist case.
In `@tests/test_tags.rs`:
- Around line 214-217: The assertion currently checks for the literal "exit: 1"
via output.contains("exit: 1") which fails for other non-zero exits and isn't
scoped to the add step; replace that check with logic that extracts the exit
code for the add step (e.g., parse the second command's "exit: N" line from
output or use a regex like r"exit:\s*([0-9]+)"), convert the captured value to
an integer and assert it != 0 so any non-zero exit passes, updating the
assertion that references output.contains("exit: 1") accordingly.
- Around line 178-181: The positive assertions are checking the entire test
transcript in the output variable, so they can pass due to earlier commands;
change the assertions to scope to the specific command-under-test output by
extracting the substring for the executed command (e.g., find the index of the
command invocation string "adr get" or "adr list" in output, then take the slice
from that index to the next command invocation or end of output) and assert the
expected tag (e.g., "caching" or "ADR-0001") appears only in that sliced
section; update the assertions around the output variable (the contains checks
at the shown location and the similar ones at 338-341) to operate on that
command-specific slice instead of the whole output.
---
Nitpick comments:
In `@docs/rfc/RFC-0002.md`:
- Line 575: The paragraph starting with "`govctl tag` qualifies because it
manages project-level configuration that applies across all resource types
(criterion 1)" repeats the same sentence opening; edit RFC-0002.md to vary
sentence structure and wording for this and the other repeated lines (look for
other sentences that begin with "`govctl tag` qualifies..." or the same clause),
e.g., rephrase some to passive voice, combine clauses, or start with the
criterion or effect instead of the command name so the repeated pattern is
avoided while preserving the original meaning.
In `@tests/test_tags.rs`:
- Around line 23-25: The test currently recreates the entire "tags" table (using
a new tags_table and doc.insert("tags", ...)), which can drop existing sibling
keys; instead fetch or create a mutable table and insert/extend the "allowed"
array into it: in the test (tests/test_tags.rs) look up doc.get_mut("tags") and
if it's a toml::Value::Table use as_table_mut() to insert or update the
"allowed" key with arr, otherwise create a new table and insert it via
doc.insert("tags", toml::Value::Table(...)); this preserves any existing keys
under "tags" while still setting "allowed".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69f7a42f-9834-4c03-8b73-8584e828ac55
📒 Files selected for processing (29)
CHANGELOG.mddocs/rfc/RFC-0002.mdgov/config.tomlgov/rfc/RFC-0002/clauses/C-CRUD-VERBS.tomlgov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.tomlgov/rfc/RFC-0002/clauses/C-RESOURCES.tomlgov/rfc/RFC-0002/rfc.tomlgov/schema/adr.schema.jsongov/schema/clause.schema.jsongov/schema/edit-ops.jsongov/schema/guard.schema.jsongov/schema/rfc.schema.jsongov/schema/work.schema.jsongov/work/2026-04-09-implement-controlled-vocabulary-tags-for-governance-artifacts.tomlsrc/cli.rssrc/cmd/edit/mod.rssrc/cmd/guard.rssrc/cmd/list.rssrc/cmd/mod.rssrc/cmd/new.rssrc/cmd/tag.rssrc/command_router.rssrc/config.rssrc/diagnostic.rssrc/model.rssrc/render.rssrc/resource_plan.rssrc/validate.rstests/test_tags.rs
✅ Files skipped from review due to trivial changes (13)
- src/cmd/guard.rs
- src/render.rs
- src/cmd/mod.rs
- gov/schema/work.schema.json
- gov/schema/clause.schema.json
- gov/schema/rfc.schema.json
- gov/schema/adr.schema.json
- gov/config.toml
- src/cmd/edit/mod.rs
- gov/work/2026-04-09-implement-controlled-vocabulary-tags-for-governance-artifacts.toml
- src/cmd/tag.rs
- gov/schema/edit-ops.json
- src/cmd/list.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/cmd/new.rs
- gov/schema/guard.schema.json
- gov/rfc/RFC-0002/clauses/C-RESOURCES.toml
- gov/rfc/RFC-0002/rfc.toml
- src/command_router.rs
- gov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.toml
- gov/rfc/RFC-0002/clauses/C-CRUD-VERBS.toml
lucifer1004
force-pushed
the
feat/tag
branch
7 times, most recently
from
f9c83e0 to
7fe79b1
Compare
Contributor
Author
|
@coderabbitai Review again |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/cmd/tag.rs (1)
232-240:⚠️ Potential issue | 🟠 MajorPropagate usage-scan failures in
tag_list.
count_tag_usagenow returns aResult, but this call still collapses any loader/parse failure back to0. That underreports usage and can makegovctl tag listlook healthy while the project is actually unreadable. Please return the error instead of fabricating counts.Possible fix
- let entries: Vec<TagEntry> = allowed - .iter() - .map(|tag| { - let usage = count_tag_usage(config, tag).unwrap_or(0); - TagEntry { - tag: tag.clone(), - usage, - } - }) - .collect(); + let entries: Vec<TagEntry> = allowed + .iter() + .map(|tag| { + Ok(TagEntry { + tag: tag.clone(), + usage: count_tag_usage(config, tag)?, + }) + }) + .collect::<Result<Vec<_>>>()?;As per coding guidelines "Code that conflicts with a normative RFC is a bug; no silent deviation allowed."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/tag.rs` around lines 232 - 240, The code in tag_list is hiding failures by calling count_tag_usage(...).unwrap_or(0); instead propagate the Result from count_tag_usage so errors bubble up. Change the iterator map to produce Result<TagEntry, E> (calling let usage = count_tag_usage(config, tag)?), then collect the iterator with collect::<Result<Vec<TagEntry>, _>>() and return that Result from tag_list (adjust its return type if needed). Update references to TagEntry and count_tag_usage accordingly so failures are not converted to zero counts.
🧹 Nitpick comments (1)
tests/test_tags.rs (1)
98-240: Add one non-ADR end-to-end tag case.This suite validates the feature almost entirely through
adr, even though the PR wires tags throughrfc,clause,work, andguardtoo. A resource-specific edit-ops or list-path regression can still slip through unnoticed; one happy-path add/get/list assertion outside ADR would materially de-risk the change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_tags.rs` around lines 98 - 240, Add a new test (e.g., fn test_rfc_add_tag()) that mirrors test_artifact_add_tag but uses the RFC CLI paths: call init_project() and today(), create the tag via run_commands temp_dir with &["tag","new","caching"], create an RFC with &["rfc","new","Test RFC"], add the tag to the RFC with &["rfc","add","RFC-0001","tags","caching"], then assert via &["rfc","get","RFC-0001","tags"] and/or &["rfc","list","--tag","caching"] using run_commands and normalize_output; place the test near other tag tests so it validates non-ADR end-to-end behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@gov/adr/ADR-0039-use-sqlite-fts5-as-read-only-search-index-for-governance-artifacts.toml`:
- Around line 51-60: Update the ADR text and the govctl init behavior to account
for SQLite WAL sidecar files: when mentioning gov/.search.db (and the govctl
init step that adds it to .gitignore) also add gov/.search.db-wal and
gov/.search.db-shm (or state explicitly to ignore the full glob gov/.search.db*)
and/or document choosing a non-WAL journal mode; update the lines that reference
gov/.search.db (and the "govctl init" instruction) to include these sidecar
filenames or the alternate journal-mode option so repo noise is fully
suppressed.
In `@gov/adr/ADR-0040-controlled-vocabulary-tags-for-governance-artifacts.toml`:
- Around line 42-54: The ADR text and schema description are inconsistent with
the shipped behavior: clauses were made taggable but ADR-0040 still states only
the four artifact types get a tags field and that "clauses inherit
discoverability"; update ADR-0040 (the prose around the [govctl] `tags` field,
the "four artifact types" consequence, and the clause schema/model/edit-ops/RFC
text) to explicitly include clauses as taggable artifacts (or alternatively
revert the code change), and also update the nearby lines referenced (including
the text around lines 66-68) so the long-lived ADR accurately documents that
clauses receive a `tags` array and how clauses are managed via the existing
`add`/`remove` verbs and `govctl check` validation.
In `@gov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.toml`:
- Around line 168-170: The tag-mutation command text is too generic; restrict
<resource> to the five taggable resource types defined in C-RESOURCES (RFCs,
clauses, ADRs, work items, and guards). Update the two example verbs (govctl
<resource> add <ID> tags <tag> and govctl <resource> remove <ID> tags <tag>) to
explicitly enumerate or constrain <resource> to those types (e.g., govctl
rfc|clause|adr|work-item|guard add <ID> tags <tag>) and add a short note
referencing that only those resources carry tags.
In
`@gov/work/2026-04-09-implement-controlled-vocabulary-tags-for-governance-artifacts.toml`:
- Line 16: Update the description value (the description string) to avoid
claiming tags apply to “all artifact types”; change it to either “all taggable
artifact types” or explicitly list the five supported kinds (e.g., artifacts A,
B, C, D, E) so the scope matches the RFC changes that exclude releases and does
not overstate implemented scope.
In `@src/cmd/guard.rs`:
- Line 55: The default empty-invariant for the tags field (the initializer
"tags: vec![]") must include an RFC clause citation; update the code by adding a
short inline comment next to the tags initializer that cites the RFC clause
enforcing this invariant (e.g., "// RFC <number>: <brief clause title>") so
reviewers can trace the rationale for the default on the tags field; ensure the
comment sits immediately adjacent to the tags: vec![] initializer to satisfy the
"Cite RFC clauses when implementing invariants in code" guideline.
In `@src/command_router.rs`:
- Around line 261-268: The implicit branch that returns OwnedEditAction::Set
when args.stdin is true (action_count == 0) skips the selector-flag validation,
allowing invalid combos like --stdin with --at/--exact/--regex/--all; update the
logic in the action inference path so it performs the same selector validation
as the explicit --set branch (or call the same validation helper) before
returning OwnedEditAction::Set (reference action_count, args.stdin, and
OwnedEditAction::Set) and reject the combination if any of the selector flags
(--at, --exact, --regex, --all) are present.
---
Duplicate comments:
In `@src/cmd/tag.rs`:
- Around line 232-240: The code in tag_list is hiding failures by calling
count_tag_usage(...).unwrap_or(0); instead propagate the Result from
count_tag_usage so errors bubble up. Change the iterator map to produce
Result<TagEntry, E> (calling let usage = count_tag_usage(config, tag)?), then
collect the iterator with collect::<Result<Vec<TagEntry>, _>>() and return that
Result from tag_list (adjust its return type if needed). Update references to
TagEntry and count_tag_usage accordingly so failures are not converted to zero
counts.
---
Nitpick comments:
In `@tests/test_tags.rs`:
- Around line 98-240: Add a new test (e.g., fn test_rfc_add_tag()) that mirrors
test_artifact_add_tag but uses the RFC CLI paths: call init_project() and
today(), create the tag via run_commands temp_dir with &["tag","new","caching"],
create an RFC with &["rfc","new","Test RFC"], add the tag to the RFC with
&["rfc","add","RFC-0001","tags","caching"], then assert via
&["rfc","get","RFC-0001","tags"] and/or &["rfc","list","--tag","caching"] using
run_commands and normalize_output; place the test near other tag tests so it
validates non-ADR end-to-end behavior.
🪄 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: 33854d82-2ed4-40cb-9f53-94d46239fafd
⛔ Files ignored due to path filters (11)
tests/snapshots/test_tags__artifact_add_tag.snapis excluded by!**/*.snaptests/snapshots/test_tags__artifact_add_unregistered_tag.snapis excluded by!**/*.snaptests/snapshots/test_tags__check_accepts_registered_tag.snapis excluded by!**/*.snaptests/snapshots/test_tags__check_rejects_unknown_tag.snapis excluded by!**/*.snaptests/snapshots/test_tags__list_filter_by_tag.snapis excluded by!**/*.snaptests/snapshots/test_tags__list_filter_multiple_tags.snapis excluded by!**/*.snaptests/snapshots/test_tags__tag_delete.snapis excluded by!**/*.snaptests/snapshots/test_tags__tag_delete_referenced.snapis excluded by!**/*.snaptests/snapshots/test_tags__tag_new.snapis excluded by!**/*.snaptests/snapshots/test_tags__tag_new_duplicate.snapis excluded by!**/*.snaptests/snapshots/test_tags__tag_new_invalid_format.snapis excluded by!**/*.snap
📒 Files selected for processing (32)
CHANGELOG.mddocs/rfc/RFC-0002.mdgov/adr/ADR-0039-use-sqlite-fts5-as-read-only-search-index-for-governance-artifacts.tomlgov/adr/ADR-0040-controlled-vocabulary-tags-for-governance-artifacts.tomlgov/config.tomlgov/rfc/RFC-0002/clauses/C-CRUD-VERBS.tomlgov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.tomlgov/rfc/RFC-0002/clauses/C-RESOURCES.tomlgov/rfc/RFC-0002/rfc.tomlgov/schema/adr.schema.jsongov/schema/clause.schema.jsongov/schema/edit-ops.jsongov/schema/guard.schema.jsongov/schema/rfc.schema.jsongov/schema/work.schema.jsongov/work/2026-04-09-implement-controlled-vocabulary-tags-for-governance-artifacts.tomlgov/work/2026-04-10-fix-stdin-without-set-infers-set-in-edit-commands.tomlsrc/cli.rssrc/cmd/edit/mod.rssrc/cmd/guard.rssrc/cmd/list.rssrc/cmd/mod.rssrc/cmd/new.rssrc/cmd/tag.rssrc/command_router.rssrc/config.rssrc/diagnostic.rssrc/model.rssrc/render.rssrc/resource_plan.rssrc/validate.rstests/test_tags.rs
| id: id.clone(), | ||
| title: title.to_string(), | ||
| refs: vec![], | ||
| tags: vec![], |
There was a problem hiding this comment.
Add an RFC clause citation on the default tags invariant.
Line 55 introduces an invariant default (tags starts empty) but lacks a clause citation in code.
Suggested patch
- tags: vec![],
+ tags: vec![], // Default controlled-vocabulary tags per [[RFC-0002:C-RESOURCES]]As per coding guidelines: src/**/*.rs: "Cite RFC clauses when implementing invariants in code".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tags: vec![], | |
| tags: vec![], // Default controlled-vocabulary tags per [[RFC-0002:C-RESOURCES]] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cmd/guard.rs` at line 55, The default empty-invariant for the tags field
(the initializer "tags: vec![]") must include an RFC clause citation; update the
code by adding a short inline comment next to the tags initializer that cites
the RFC clause enforcing this invariant (e.g., "// RFC <number>: <brief clause
title>") so reviewers can trace the rationale for the default on the tags field;
ensure the comment sits immediately adjacent to the tags: vec![] initializer to
satisfy the "Cite RFC clauses when implementing invariants in code" guideline.
…acts Per ADR-0040 and RFC-0002 v0.7.0: - Add optional `tags` array to RFC, ADR, work item, and guard schemas - Add `govctl tag new/delete/list` for registry management in config - Add `--tag` filter on `rfc/adr/work/guard list` with AND semantics - Add `govctl check` validation: rejects invalid format and unregistered tags - Add immediate rejection at `add` time for unregistered tags (E1105) - Add edit-ops SSOT entries for `add/remove/get` on tags field - Add 11 integration tests covering registry, tagging, validation, filtering - Amend RFC-0002 v0.7.0: C-RESOURCES, C-CRUD-VERBS, C-GLOBAL-COMMANDS
…action When `--stdin` is present with no `--set`/`--add`/`--remove`/`--tick`, the edit command now infers `--set` instead of erroring with "exactly one edit action is required". Fixes the documented but broken pattern: govctl clause edit RFC-0001:C-SUMMARY text --stdin Closes WI-2026-04-10-001.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
gov/config.tomlunder[tags] allowedand enforced bygovctl checkWhat changed
Governance:
--taglist filter), C-GLOBAL-COMMANDS (govctl tag new/delete/list)Implementation (31 files, +1409/-41):
govctl tag new <tag>/delete <tag>/list— registry management with usage countsgovctl <resource> add <ID> tags <tag>/remove— artifact tagging via existing verb modelgovctl <resource> list --tag <tag>[,<tag>...]— AND-semantics filter on all five taggable typesgovctl check— validates tag format (^[a-z][a-z0-9-]*$) and rejects unregistered tagsaddtime for unregistered tags (E1105), not just deferred tocheckadd/remove/geton tags fieldTests:
tests/test_tags.rs) covering registry CRUD, artifact tagging, validation, and list filteringDesign decisions
new/delete/listverbs — follows existing resource verb convention rather thanadd/remove(which means something different at the resource level)C-CACHE-BACKENDis more precise than tagging the parent RFCLazyLock<Regex>— single tag format regex definition used across validation, add, and checkTest plan
cargo test— all 21 suites pass (0 failures)cargo run --quiet -- check— all checks passtag new→tag list(shows usage counts) →tag deleteadr add ADR-0001 tags "caching"with unregistered tag → rejected with E1105govctl tag listoutput formatting in TTY and non-TTY modesSummary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes