feat: prepare govctl 0.17.0 - #41
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (63)
📝 WalkthroughWalkthroughVersion 0.17.0 introduces schema version 5, layered source-scan ignore semantics, deterministic reference diagnostics, transactional migration rollback, CRLF-independent validation, Zig-based release packaging, pre-1.0 aliases, authenticated self-update requests, and related documentation, governance, and test updates. ChangesSource scanning and schema migration
Release distribution and self-update
Validation and release metadata
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cmd/migrate/mod.rs (1)
302-347: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve hand-edited
gov/config.tomlformatting when bumping the schema.
plan_config_version_bumpparses the whole config intotoml::Tableand writes it withtoml::to_string_pretty, so any user comments are discarded. The dependency is justtoml = "1"with nopreserve_orderfeature, so key ordering can also be normalized. Use a format-preserving writer such astoml_edit::DocumentMutfor this migration mutation instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cmd/migrate/mod.rs` around lines 302 - 347, Update plan_config_version_bump to parse and serialize the configuration with toml_edit::DocumentMut instead of toml::Table and toml::to_string_pretty, preserving comments, formatting, and key order. Apply the existing schema.version update and source_scan.exclude removal through the toml_edit document API, while retaining the current diagnostics and FileOp::Write behavior.
🧹 Nitpick comments (2)
src/scan.rs (2)
303-346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDiagnostic path is hardcoded instead of derived from config.
Everywhere else in this function's callers the location comes from
config.display_path(&config.gov_root.join("config.toml")). Here the literal"gov/config.toml"will be wrong whenever the gov root is not./gov. Passing the display path intobuild_include_matcherkeeps diagnostics accurate.♻️ Proposed refactor
-fn build_include_matcher(root: &Path, patterns: &[String]) -> Result<Gitignore, Diagnostic> { +fn build_include_matcher( + root: &Path, + patterns: &[String], + config_path: &str, +) -> Result<Gitignore, Diagnostic> { let mut builder = GitignoreBuilder::new(root); for pattern in patterns { if pattern.is_empty() { return Err(Diagnostic::new( DiagnosticCode::E0501ConfigInvalid, "Invalid source_scan.include pattern: entries cannot be empty", - "gov/config.toml".to_string(), + config_path.to_string(), ));Apply the same substitution to the remaining three diagnostics in this function and thread the already-computed config display path through from
scan_source_refs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/scan.rs` around lines 303 - 346, Update build_include_matcher to accept the config display path as an argument and use it for all four Diagnostic instances instead of the hardcoded "gov/config.toml". In scan_source_refs, compute the path with config.display_path(&config.gov_root.join("config.toml")) and pass it through to build_include_matcher.
356-369: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winProbing ignore-file readability by reading full contents on every reached directory.
read_to_stringis used purely to detect unreadable paths, so every directory in the tree pays two extra opens plus a full file read whose result is discarded — and the.gitignore/.govignorebytes are then read a second time by the walker.File::opengives the sameNotFound/IsADirectory/permission signal without the read.♻️ Proposed refactor
for name in [".gitignore", ".govignore"] { let path = directory.join(name); - match fs::read_to_string(&path) { + match fs::File::open(&path) { Ok(_) => {} Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} Err(error) => result.diagnostics.push(Diagnostic::io_error(Note that
File::openon a directory succeeds on Linux, so keep ametadata().is_dir()check if thetest_scan_reports_reached_unreadable_ignore_pathcase must keep failing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/scan.rs` around lines 356 - 369, Update validate_reached_ignore_files to probe each ignore path with File::open instead of fs::read_to_string, preserving the existing NotFound handling and diagnostic reporting for other errors. After opening, check metadata().is_dir() so directory paths still produce the expected unreadable-path diagnostic, while avoiding reading and discarding file contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 183-276: In both smoke jobs, `smoke-cross` and the sibling job
spanning `.github/workflows/release.yml` lines 278-334, set job permissions to
`contents: read`, configure the `actions/checkout` step with
`persist-credentials: false`, and keep `GITHUB_TOKEN` defined only on the
self-update probe step rather than the whole job.
In `@gov/releases.toml`:
- Around line 5-6: Remove duplicated TOML array-table headers in
gov/releases.toml at lines 5-6, retaining one [[releases]] header before the
0.17.0 fields. In gov/rfc/RFC-0002/rfc.toml at lines 44-79, retain exactly one
[[sections]] header and one [[changelog]] header for each record.
---
Outside diff comments:
In `@src/cmd/migrate/mod.rs`:
- Around line 302-347: Update plan_config_version_bump to parse and serialize
the configuration with toml_edit::DocumentMut instead of toml::Table and
toml::to_string_pretty, preserving comments, formatting, and key order. Apply
the existing schema.version update and source_scan.exclude removal through the
toml_edit document API, while retaining the current diagnostics and
FileOp::Write behavior.
---
Nitpick comments:
In `@src/scan.rs`:
- Around line 303-346: Update build_include_matcher to accept the config display
path as an argument and use it for all four Diagnostic instances instead of the
hardcoded "gov/config.toml". In scan_source_refs, compute the path with
config.display_path(&config.gov_root.join("config.toml")) and pass it through to
build_include_matcher.
- Around line 356-369: Update validate_reached_ignore_files to probe each ignore
path with File::open instead of fs::read_to_string, preserving the existing
NotFound handling and diagnostic reporting for other errors. After opening,
check metadata().is_dir() so directory paths still produce the expected
unreadable-path diagnostic, while avoiding reading and discarding file contents.
🪄 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 Plus
Run ID: 093fbf0d-dcf0-4a66-84a1-0eb07fd61934
⛔ Files ignored due to path filters (6)
Cargo.lockis excluded by!**/*.locktests/snapshots/test_help__rfc_bump_help.snapis excluded by!**/*.snaptests/snapshots/test_scan__scan_deprecated_rfc_reference.snapis excluded by!**/*.snaptests/snapshots/test_scan__scan_mixed_valid_invalid_references.snapis excluded by!**/*.snaptests/snapshots/test_scan__scan_unknown_clause_reference.snapis excluded by!**/*.snaptests/snapshots/test_scan__scan_unknown_rfc_reference.snapis excluded by!**/*.snap
📒 Files selected for processing (61)
.claude-plugin/marketplace.json.claude/.claude-plugin/plugin.json.claude/skills/gov/SKILL.md.github/workflows/release.ymlCHANGELOG.mdCargo.tomlREADME.mddocs/guide/conformance-cases.mddocs/guide/rfcs.mddocs/guide/validation.mddocs/rfc/RFC-0002.mddocs/rfc/RFC-0009.mdgov/adr/ADR-0009-configurable-source-code-reference-scanning.tomlgov/adr/ADR-0041-self-update-and-cargo-binstall-binary-distribution.tomlgov/adr/ADR-0059-use-project-root-selection-with-layered-ignore-rules.tomlgov/adr/ADR-0060-build-releases-with-zig-and-preserve-target-aliases.tomlgov/config.tomlgov/releases.tomlgov/rfc/RFC-0002/clauses/C-COMPATIBILITY-BOUNDARY.tomlgov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.tomlgov/rfc/RFC-0002/clauses/C-PRE-1-RELEASE-TARGET-COMPATIBILITY.tomlgov/rfc/RFC-0002/rfc.tomlgov/rfc/RFC-0009/clauses/C-IGNORE-MIGRATION.tomlgov/rfc/RFC-0009/clauses/C-IGNORE-RULES.tomlgov/rfc/RFC-0009/clauses/C-REFERENCE-REPORTING.tomlgov/rfc/RFC-0009/clauses/C-SOURCE-SELECTION.tomlgov/rfc/RFC-0009/clauses/C-SUMMARY.tomlgov/rfc/RFC-0009/clauses/C-TRAVERSAL.tomlgov/rfc/RFC-0009/rfc.tomlgov/schema/SCHEMA.mdgov/work/2026-07-29-prune-excluded-source-scan-directories-during-traversal.tomlgov/work/2026-07-30-adopt-zig-release-builds-with-pre-1-0-aliases.tomlgov/work/2026-07-30-authenticate-self-update-api-requests.tomlgov/work/2026-07-30-make-source-reference-diagnostics-precise-and-deterministic.tomlgov/work/2026-07-30-make-validation-line-ending-independent.tomlsrc/cli/resources/rfc.rssrc/cmd/check.rssrc/cmd/edit/delete_referrers.rssrc/cmd/migrate/mod.rssrc/cmd/migrate/ops.rssrc/cmd/migrate/ops_tests.rssrc/cmd/self_update.rssrc/cmd/self_update_tests.rssrc/config/mod.rssrc/config/runtime.rssrc/config/template.rssrc/main.rssrc/reference_pattern.rssrc/render/links.rssrc/scan.rssrc/schema.rssrc/signature/canonical_json.rssrc/signature/tests.rssrc/validate/bracket_refs.rssrc/verification/runner/process_group.rssrc/write/artifact.rssrc/write/artifact_io.rstests/edit_tests/clause.rstests/test_conformance.rstests/test_migrate.rstests/test_scan.rs
| [[releases]] | ||
| version = "0.17.0" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove duplicated TOML array-table headers across the release metadata.
Both files create unintended empty records before the populated entries.
gov/releases.toml#L5-L6: retain only one[[releases]]header before the0.17.0fields.gov/rfc/RFC-0002/rfc.toml#L44-L79: retain one[[sections]]header and one[[changelog]]header per record.
📍 Affects 2 files
gov/releases.toml#L5-L6(this comment)gov/rfc/RFC-0002/rfc.toml#L44-L79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gov/releases.toml` around lines 5 - 6, Remove duplicated TOML array-table
headers in gov/releases.toml at lines 5-6, retaining one [[releases]] header
before the 0.17.0 fields. In gov/rfc/RFC-0002/rfc.toml at lines 44-79, retain
exactly one [[sections]] header and one [[changelog]] header for each record.
8b923fb to
c6e3ac6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_scan.rs (2)
613-627: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSymlink test passes vacuously.
The project contains no
.rsfile other than the symlinked one, so0 source files scannedwould also hold if the include matcher or the walker were broken entirely. Add a real in-tree file so the count distinguishes "didn't follow the link" from "scanned nothing at all".💚 Proposed strengthening
let external = tempfile::tempdir()?; fs::write(external.path().join("linked.rs"), "fn linked() {}\n")?; symlink(external.path(), temp_dir.path().join("linked"))?; + write_main_rs(temp_dir.path(), "fn main() {}\n")?; let output = run_commands(temp_dir.path(), &[&["check"]])?; - assert!(output.contains(" 0 source files scanned"), "{output}"); + assert!(output.contains(" 1 source files scanned"), "{output}"); Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_scan.rs` around lines 613 - 627, Strengthen test_scan_does_not_follow_symbolic_links by creating a real in-tree .rs file under the initialized project before running the check. Update the assertion to expect one source file scanned, preserving the symlink setup so the test distinguishes scanning the real file from following the external link.
29-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo adjacent helpers navigate the same TOML two different ways.
set_source_scan_includeuses panicking index assignment (config["source_scan"]["include"] = ...), which aborts with an unhelpful panic if[source_scan]is ever absent, whileset_source_scan_patternright below does the same navigation defensively. Current callers always go throughinit_source_scan_project, so this works today, but it's an easy trap for the next test. Consider a sharedset_source_scan_field(dir, key, value)helper.♻️ Sketch of a shared helper
-fn set_source_scan_include( - dir: &Path, - patterns: &[&str], -) -> Result<(), Box<dyn std::error::Error>> { - let config_path = dir.join("gov/config.toml"); - let mut config: toml::Value = toml::from_str(&fs::read_to_string(&config_path)?)?; - config["source_scan"]["include"] = toml::Value::Array( - patterns - .iter() - .map(|pattern| toml::Value::String((*pattern).to_string())) - .collect(), - ); - fs::write(config_path, toml::to_string_pretty(&config)?)?; - Ok(()) -} - -fn set_source_scan_pattern(dir: &Path, pattern: &str) -> Result<(), Box<dyn std::error::Error>> { +fn set_source_scan_field( + dir: &Path, + key: &str, + value: toml::Value, +) -> Result<(), Box<dyn std::error::Error>> { let config_path = dir.join("gov/config.toml"); let mut config: toml::Value = toml::from_str(&fs::read_to_string(&config_path)?)?; let config_table = config .as_table_mut() .ok_or("config root must be a TOML table")?; let source_scan = config_table .entry("source_scan") .or_insert_with(|| toml::Value::Table(toml::Table::new())) .as_table_mut() .ok_or("source_scan must be a TOML table")?; - source_scan.insert( - "pattern".to_string(), - toml::Value::String(pattern.to_string()), - ); + source_scan.insert(key.to_string(), value); fs::write(config_path, toml::to_string_pretty(&config)?)?; Ok(()) } + +fn set_source_scan_include( + dir: &Path, + patterns: &[&str], +) -> Result<(), Box<dyn std::error::Error>> { + let array = patterns + .iter() + .map(|pattern| toml::Value::String((*pattern).to_string())) + .collect(); + set_source_scan_field(dir, "include", toml::Value::Array(array)) +} + +fn set_source_scan_pattern(dir: &Path, pattern: &str) -> Result<(), Box<dyn std::error::Error>> { + set_source_scan_field(dir, "pattern", toml::Value::String(pattern.to_string())) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_scan.rs` around lines 29 - 62, Refactor set_source_scan_include and set_source_scan_pattern to share a defensive set_source_scan_field helper that validates or creates the root source_scan table before inserting the requested field. Preserve each helper’s existing value conversion and file-writing behavior, and remove the panicking indexed assignment from set_source_scan_include.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gov/rfc/RFC-0009/clauses/C-IGNORE-RULES.toml`:
- Line 14: The RFC clause’s stated ignore-rule precedence is not validated by
the implementation and tests. Add scan tests covering cross-file exclusion
conflicts in both directions—root .govignore versus deeper .gitignore and root
.gitignore versus deeper .govignore—then ensure src/scan.rs preserves the
clause’s file-type-before-depth precedence; alternatively revise the clause to
match the library’s depth-before-file-type behavior.
In `@src/scan.rs`:
- Around line 117-129: Update the WalkBuilder configuration in the
source-scanning setup to add an entry filter that prunes any .git directory
before traversal descends into it. Preserve the existing git_ignore(true)
behavior and all other walker options, using the builder’s entry-filter
mechanism rather than a post-scan exclusion.
In `@src/validate/bracket_refs.rs`:
- Around line 291-301: The invalid-match diagnostic in the target_capture error
branch lacks occurrence-specific location data, allowing distinct matches to be
removed by extend_with_pattern_dedup. Update the Diagnostic::new location
argument in the reference-pattern validation flow to include the match byte
offset within text, or preferably the line/column from SourceLocator, while
preserving the existing error code and message.
---
Nitpick comments:
In `@tests/test_scan.rs`:
- Around line 613-627: Strengthen test_scan_does_not_follow_symbolic_links by
creating a real in-tree .rs file under the initialized project before running
the check. Update the assertion to expect one source file scanned, preserving
the symlink setup so the test distinguishes scanning the real file from
following the external link.
- Around line 29-62: Refactor set_source_scan_include and
set_source_scan_pattern to share a defensive set_source_scan_field helper that
validates or creates the root source_scan table before inserting the requested
field. Preserve each helper’s existing value conversion and file-writing
behavior, and remove the panicking indexed assignment from
set_source_scan_include.
🪄 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 Plus
Run ID: 2bbd1564-4d02-4476-ab74-b1c0daf261a6
⛔ Files ignored due to path filters (6)
Cargo.lockis excluded by!**/*.locktests/snapshots/test_help__rfc_bump_help.snapis excluded by!**/*.snaptests/snapshots/test_scan__scan_deprecated_rfc_reference.snapis excluded by!**/*.snaptests/snapshots/test_scan__scan_mixed_valid_invalid_references.snapis excluded by!**/*.snaptests/snapshots/test_scan__scan_unknown_clause_reference.snapis excluded by!**/*.snaptests/snapshots/test_scan__scan_unknown_rfc_reference.snapis excluded by!**/*.snap
📒 Files selected for processing (62)
.claude-plugin/marketplace.json.claude/.claude-plugin/plugin.json.claude/skills/gov/SKILL.md.github/workflows/release.ymlCHANGELOG.mdCargo.tomlREADME.mddocs/guide/conformance-cases.mddocs/guide/rfcs.mddocs/guide/validation.mddocs/rfc/RFC-0002.mddocs/rfc/RFC-0009.mdgov/adr/ADR-0009-configurable-source-code-reference-scanning.tomlgov/adr/ADR-0041-self-update-and-cargo-binstall-binary-distribution.tomlgov/adr/ADR-0059-use-project-root-selection-with-layered-ignore-rules.tomlgov/adr/ADR-0060-build-releases-with-zig-and-preserve-target-aliases.tomlgov/config.tomlgov/releases.tomlgov/rfc/RFC-0002/clauses/C-COMPATIBILITY-BOUNDARY.tomlgov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.tomlgov/rfc/RFC-0002/clauses/C-PRE-1-RELEASE-TARGET-COMPATIBILITY.tomlgov/rfc/RFC-0002/rfc.tomlgov/rfc/RFC-0009/clauses/C-IGNORE-MIGRATION.tomlgov/rfc/RFC-0009/clauses/C-IGNORE-RULES.tomlgov/rfc/RFC-0009/clauses/C-REFERENCE-REPORTING.tomlgov/rfc/RFC-0009/clauses/C-SOURCE-SELECTION.tomlgov/rfc/RFC-0009/clauses/C-SUMMARY.tomlgov/rfc/RFC-0009/clauses/C-TRAVERSAL.tomlgov/rfc/RFC-0009/rfc.tomlgov/schema/SCHEMA.mdgov/work/2026-07-29-prune-excluded-source-scan-directories-during-traversal.tomlgov/work/2026-07-30-adopt-zig-release-builds-with-pre-1-0-aliases.tomlgov/work/2026-07-30-authenticate-self-update-api-requests.tomlgov/work/2026-07-30-close-0-17-release-review-findings.tomlgov/work/2026-07-30-make-source-reference-diagnostics-precise-and-deterministic.tomlgov/work/2026-07-30-make-validation-line-ending-independent.tomlsrc/cli/resources/rfc.rssrc/cmd/check.rssrc/cmd/edit/delete_referrers.rssrc/cmd/migrate/mod.rssrc/cmd/migrate/ops.rssrc/cmd/migrate/ops_tests.rssrc/cmd/self_update.rssrc/cmd/self_update_tests.rssrc/config/mod.rssrc/config/runtime.rssrc/config/template.rssrc/main.rssrc/reference_pattern.rssrc/render/links.rssrc/scan.rssrc/schema.rssrc/signature/canonical_json.rssrc/signature/tests.rssrc/validate/bracket_refs.rssrc/verification/runner/process_group.rssrc/write/artifact.rssrc/write/artifact_io.rstests/edit_tests/clause.rstests/test_conformance.rstests/test_migrate.rstests/test_scan.rs
🚧 Files skipped from review as they are similar to previous changes (17)
- .claude-plugin/marketplace.json
- docs/guide/rfcs.md
- docs/guide/conformance-cases.md
- src/config/template.rs
- gov/adr/ADR-0009-configurable-source-code-reference-scanning.toml
- docs/guide/validation.md
- src/verification/runner/process_group.rs
- gov/adr/ADR-0041-self-update-and-cargo-binstall-binary-distribution.toml
- gov/rfc/RFC-0009/clauses/C-SUMMARY.toml
- gov/adr/ADR-0060-build-releases-with-zig-and-preserve-target-aliases.toml
- README.md
- .claude/.claude-plugin/plugin.json
- gov/releases.toml
- src/cli/resources/rfc.rs
- CHANGELOG.md
- gov/schema/SCHEMA.md
- docs/rfc/RFC-0002.md
c6e3ac6 to
6c50978
Compare
Summary
.gitignoreand.govignoretraversal, schema v5 migration, and precise deterministic reference diagnosticsUpgrade notes
source_scan.exclude; rungovctl migrateto convert schema v4 exclusions into root.govignorerulesValidation
just pre-commitcargo package --locked --allow-dirtySummary by CodeRabbit
.gitignoreand.govignore.GITHUB_TOKENis available.