feat(cli): add self-update command and cargo-binstall support#9
feat(cli): add self-update command and cargo-binstall support#9lucifer1004 merged 1 commit intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 37 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (51)
📝 WalkthroughWalkthroughBumps govctl to v0.8.3 and adds a built-in Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI / User
participant Router as Command Router
participant SelfUpdate as self_update Module
participant GitHub as GitHub Releases API
participant FS as Filesystem
CLI->>Router: govctl self-update [--check]
Router->>SelfUpdate: self_update(check_only)
SelfUpdate->>SelfUpdate: Read CARGO_PKG_VERSION
alt check_only = true
SelfUpdate->>GitHub: Query latest release
GitHub-->>SelfUpdate: Latest version tag
SelfUpdate->>SelfUpdate: Normalize tag, compare semver
alt update available
SelfUpdate-->>CLI: Emit Diagnostic (update available)
else up-to-date
SelfUpdate-->>CLI: Print "no update needed"
end
else check_only = false
SelfUpdate->>GitHub: Query latest release & assets
GitHub-->>SelfUpdate: Release metadata + asset
SelfUpdate->>SelfUpdate: Select platform asset
SelfUpdate->>GitHub: Download asset (with progress if TTY)
GitHub-->>SelfUpdate: Binary data
SelfUpdate->>SelfUpdate: Verify integrity
SelfUpdate->>FS: Replace running executable
FS-->>SelfUpdate: Replace result
SelfUpdate->>SelfUpdate: Print old/new versions
SelfUpdate-->>CLI: Success (exit 0)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/command_router.rs (1)
873-875: Add planner tests for the newSelfUpdateroute.Lines 873-875 and Line 512 add new routing branches, but this file’s tests do not yet assert
Commands::SelfUpdate { check }mapping and lock disposition behavior. A small unit test here would prevent silent regressions.Also applies to: 512-512
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command_router.rs` around lines 873 - 875, Add unit tests that exercise the new Commands::SelfUpdate { check } branch and assert it maps to global(Op::Builtin(BuiltinOp::SelfUpdate { check })) and has the correct lock disposition; specifically, add tests that call the planner/routing helper used elsewhere in this module (the same helper used by other command tests) with a SelfUpdate command variant and assert the returned Route/Op equals global(Op::Builtin(BuiltinOp::SelfUpdate { check: true })) and global(Op::Builtin(BuiltinOp::SelfUpdate { check: false })) respectively, and verify the route's lock behavior matches the intended disposition (no lock or whatever the module's convention is) so future changes will fail the test if mapping or lock semantics regress.gov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.toml (1)
172-175: Keep global-command entries structurally uniform.Optional: add explicit
Syntax:/Behavior:lines here (even if brief with a cross-reference) to match the established pattern used by commands 1-10 and keep parsing/readability consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.toml` around lines 172 - 175, The "govctl self-update" global-command entry is missing the structural `Syntax:` and `Behavior:` lines used by entries 1–10; update the "11. `govctl self-update`" stanza to include a brief `Syntax:` (e.g., "govctl self-update [--channel <channel>]") and a short `Behavior:` that either summarizes the action ("Updates the govctl binary to the latest release") or cross-references `[[RFC-0002:C-SELF-UPDATE]]`, so the entry matches the established format and remains machine/reader-friendly.
🤖 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-0041-self-update-and-cargo-binstall-binary-distribution.toml`:
- Around line 41-47: Remove execution-specific bullets from the ADR-0041
"Implementation Notes" section: delete or move the GitHub endpoint line, the
platform mapping/target-triple detail, the `--check` flag behavior, the
`cargo-binstall` URL template (`govctl-v{version}-{target}.tar.gz`), and the
amendment note referencing [[RFC-0002:C-GLOBAL-COMMANDS]] out of the ADR
document; leave ADR-0041 focused on decision, rationale, and consequences only,
and relocate the removed items into the RFC or implementation docs (e.g., the
CLI command RFC and code-level README) so command semantics and asset naming are
documented where execution details belong.
In `@src/cmd/self_update.rs`:
- Around line 37-48: The current check treats any string-inequality as
"newer"—parse both current and latest_version as semver::Version (using
semver::Version::parse) and compare them semantically (if latest_version <=
current then treat as up-to-date and return success; only when latest_version >
current produce the Diagnostic via Diagnostic::new). Update the code paths
around the equality check in self_update.rs (the block that uses
ui::success/ui::info and returns Ok(vec![]) or Ok(vec![Diagnostic::new(...)]))
to use the semantic comparison and handle parse errors (return an appropriate
error/diagnostic or fallback) rather than simple string inequality.
In `@src/command_router.rs`:
- Around line 215-216: The planner currently treats
Op::Builtin(BuiltinOp::SelfUpdate { .. }) as lock-free; change the mapping so
that non-check SelfUpdate requests require LockDisposition::GovRootExclusive
(i.e., treat Op::Builtin(BuiltinOp::SelfUpdate { check: false }) like other
mutating ops), update the branch in the lock disposition resolver that groups
SelfUpdate with TagList to return LockDisposition::GovRootExclusive for mutating
SelfUpdate, and add a unit test assertion in
test_lock_disposition_requires_lock_for_mutating_commands (e.g.,
assert_eq!(global(Op::Builtin(BuiltinOp::SelfUpdate { check: false
})).lock_disposition(), LockDisposition::GovRootExclusive)) to cover the new
behavior while keeping SelfUpdate { check: true } lock-free.
---
Nitpick comments:
In `@gov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.toml`:
- Around line 172-175: The "govctl self-update" global-command entry is missing
the structural `Syntax:` and `Behavior:` lines used by entries 1–10; update the
"11. `govctl self-update`" stanza to include a brief `Syntax:` (e.g., "govctl
self-update [--channel <channel>]") and a short `Behavior:` that either
summarizes the action ("Updates the govctl binary to the latest release") or
cross-references `[[RFC-0002:C-SELF-UPDATE]]`, so the entry matches the
established format and remains machine/reader-friendly.
In `@src/command_router.rs`:
- Around line 873-875: Add unit tests that exercise the new Commands::SelfUpdate
{ check } branch and assert it maps to global(Op::Builtin(BuiltinOp::SelfUpdate
{ check })) and has the correct lock disposition; specifically, add tests that
call the planner/routing helper used elsewhere in this module (the same helper
used by other command tests) with a SelfUpdate command variant and assert the
returned Route/Op equals global(Op::Builtin(BuiltinOp::SelfUpdate { check: true
})) and global(Op::Builtin(BuiltinOp::SelfUpdate { check: false }))
respectively, and verify the route's lock behavior matches the intended
disposition (no lock or whatever the module's convention is) so future changes
will fail the test if mapping or lock semantics regress.
🪄 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: 25c7b688-939a-42d2-9db4-aaee2ab38000
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.claude-plugin/marketplace.json.claude/.claude-plugin/plugin.json.claude/skills/commit/SKILL.mdCHANGELOG.mdCargo.tomldocs/rfc/RFC-0002.mdgov/adr/ADR-0041-self-update-and-cargo-binstall-binary-distribution.tomlgov/releases.tomlgov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.tomlgov/rfc/RFC-0002/clauses/C-SELF-UPDATE.tomlgov/rfc/RFC-0002/rfc.tomlgov/work/2026-04-13-implement-self-update-command-and-cargo-binstall-support.tomlsrc/cli.rssrc/cmd/mod.rssrc/cmd/self_update.rssrc/command_router.rs
| | Op::Builtin(BuiltinOp::SelfUpdate { .. }) | ||
| | Op::Builtin(BuiltinOp::TagList { .. }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Confirm self-update is currently lock-free in planner:"
rg -n -C3 'lock_disposition|SelfUpdate' src/command_router.rs
echo
echo "2) Confirm non-check self-update mutates executable:"
rg -n -C4 'perform_update|Update::configure|\\.update\\(\\)' src/cmd/self_update.rs
echo
echo "3) Check for any dedicated update/process lock mechanism in source:"
rg -n -C2 'lockfile|flock|fs2|FileExt|advisory|named mutex|mutex' srcRepository: govctl-org/govctl
Length of output: 5153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find the full perform_update implementation to see what Update::update does:"
rg -n -A 15 'fn perform_update' src/cmd/self_update.rs
echo
echo "2) Check if there are any SelfUpdate-specific tests:"
rg -n 'SelfUpdate' src/command_router.rs | grep -i test
echo
echo "3) Verify the lock mechanism is actually being used elsewhere for mutations:"
rg -n 'GovRootExclusive' src/command_router.rs | head -20
echo
echo "4) Check if there's any protective locking in the self_update module itself:"
rg -n -C 3 'lock|Lock|Mutex|RwLock' src/cmd/self_update.rsRepository: govctl-org/govctl
Length of output: 794
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Show all GovRootExclusive assignments to confirm pattern for mutations:"
rg -n 'GovRootExclusive' src/command_router.rs
echo
echo "2) Check if SelfUpdate appears in any tests section:"
sed -n '1100,1300p' src/command_router.rs | rg -n 'SelfUpdate|lock_disposition|test'
echo
echo "3) Search entire test module for any SelfUpdate references:"
rg -n 'fn test' src/command_router.rs | wc -l
rg -n 'SelfUpdate' src/command_router.rs | tail -10Repository: govctl-org/govctl
Length of output: 1971
🌐 Web query:
self_update rust crate internal locking file mutex
💡 Result:
The self_update Rust crate (latest v0.44.0 as of 2026-04-05) enables in-place self-updates for standalone executables by downloading from backends like GitHub releases, extracting archives, and replacing the running binary using the re-exported self_replace crate. It does not use std::sync::Mutex or explicit file locking mechanisms internally for concurrency control. self_update's update process: - Fetches release info (e.g., via GitHub API). - Downloads the asset to a temp file using tempfile. - Extracts (tar/zip/gz) to a temp directory. - Calls self_replace::self_replace(&new_exe_path) to atomically replace the current executable. self_replace handles platform-specific replacement without Mutex: - Unix: Uses fs::rename for atomic move (no lock needed as running process doesn't hold write lock). - Windows: Copies self to temp with DELETE_ON_CLOSE flag, renames self aside, copies new binary, spawns temp copy to wait and delete old via Windows API (CreateFile with FILE_FLAG_DELETE_ON_CLOSE, no Rust Mutex). No evidence of internal Mutex usage in self_update or self_replace source snippets. Concurrency safety relies on atomic filesystem ops and platform self-replacement tricks. For multi-threaded apps, users must externally synchronize update calls.
Citations:
- 1: https://crates.io/crates/self_update
- 2: https://docs.rs/self_update/0.9.0/self_update/index.html
- 3: https://docs.rs/self_update/0.15.0/self_update/index.html
- 4: https://github.com/jaemk/self_update
- 5: https://lib.rs/crates/self_update
- 6: https://docs.rs/self-replace/latest/self_replace/
- 7: https://github.com/mitsuhiko/self-replace
- 8: https://github.com/mitsuhiko/self-replace/blob/main/src/windows.rs
- 9: https://docs.rs/self_update/latest/self_update/
- 10: https://docs.rs/self_update/0.42.0/self_update/
Mutating self-update path requires exclusive lock to prevent concurrent update races.
Line 215 treats all SelfUpdate invocations as lock-free, but the non---check branch invokes self_update::backends::github::Update::configure().build()?.update()?, which performs an atomic executable replacement. The self_update crate provides no internal Mutex or file-locking mechanism—it relies entirely on OS atomic operations and explicitly requires callers to externally synchronize concurrent update attempts. Without an application-level exclusive lock, concurrent govctl self-update invocations can race and corrupt the binary.
Additionally, the test module includes comprehensive lock disposition assertions for mutations (Init, Edit, WorkItem) but lacks coverage for SelfUpdate.
💡 Proposed minimal planner fix
match &self.op {
Op::Builtin(BuiltinOp::Check { .. })
| Op::Builtin(BuiltinOp::Status)
| Op::Builtin(BuiltinOp::Verify { .. })
| Op::Builtin(BuiltinOp::Describe { .. })
| Op::Builtin(BuiltinOp::Completions { .. })
- | Op::Builtin(BuiltinOp::SelfUpdate { .. })
+ | Op::Builtin(BuiltinOp::SelfUpdate { check: true })
| Op::Builtin(BuiltinOp::TagList { .. })
| Op::Get
| Op::List { .. }
| Op::Show { .. } => LockDisposition::None,
+ Op::Builtin(BuiltinOp::SelfUpdate { check: false }) => {
+ LockDisposition::GovRootExclusive
+ }Follow with test assertions (e.g., assert_eq!(global(Op::Builtin(BuiltinOp::SelfUpdate { check: false })).lock_disposition(), LockDisposition::GovRootExclusive);) added to the test_lock_disposition_requires_lock_for_mutating_commands test.
📝 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.
| | Op::Builtin(BuiltinOp::SelfUpdate { .. }) | |
| | Op::Builtin(BuiltinOp::TagList { .. }) | |
| | Op::Builtin(BuiltinOp::Check { .. }) | |
| | Op::Builtin(BuiltinOp::Status) | |
| | Op::Builtin(BuiltinOp::Verify { .. }) | |
| | Op::Builtin(BuiltinOp::Describe { .. }) | |
| | Op::Builtin(BuiltinOp::Completions { .. }) | |
| | Op::Builtin(BuiltinOp::SelfUpdate { check: true }) | |
| | Op::Builtin(BuiltinOp::TagList { .. }) | |
| | Op::Get | |
| | Op::List { .. } | |
| | Op::Show { .. } => LockDisposition::None, | |
| Op::Builtin(BuiltinOp::SelfUpdate { check: false }) => { | |
| LockDisposition::GovRootExclusive | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/command_router.rs` around lines 215 - 216, The planner currently treats
Op::Builtin(BuiltinOp::SelfUpdate { .. }) as lock-free; change the mapping so
that non-check SelfUpdate requests require LockDisposition::GovRootExclusive
(i.e., treat Op::Builtin(BuiltinOp::SelfUpdate { check: false }) like other
mutating ops), update the branch in the lock disposition resolver that groups
SelfUpdate with TagList to return LockDisposition::GovRootExclusive for mutating
SelfUpdate, and add a unit test assertion in
test_lock_disposition_requires_lock_for_mutating_commands (e.g.,
assert_eq!(global(Op::Builtin(BuiltinOp::SelfUpdate { check: false
})).lock_disposition(), LockDisposition::GovRootExclusive)) to cover the new
behavior while keeping SelfUpdate { check: true } lock-free.
lucifer1004
force-pushed
the
feat/self-update
branch
2 times, most recently
from
9e0d43e to
c108579
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/terminal_md.rs`:
- Around line 13-29: The regexes html_comment, html_anchor, html_del, and
md_link are being compiled on every call and silently return the raw markdown on
compile errors; move these known-valid literal patterns to static, precompiled
Regex instances (e.g., using std::sync::LazyLock or once_cell::sync::Lazy) so
compilation happens once at startup, and replace the Err(_) branches with
expect() so the program fails fast with a clear message if a pattern is wrong;
update the code that currently defines html_comment, html_anchor, html_del, and
md_link to reference the newly created static Regex values instead of calling
Regex::new() per-invocation.
In `@src/validate.rs`:
- Around line 913-917: The code currently swallows failures from
crate::cmd::tag::tag_re() by using Err(_) => continue, which disables both
E1101TagInvalidFormat and E1105TagUnknown checks for that tag; instead, when
tag_re() returns Err, record a validation error for that tag (the same error
surfaced by tag_new — e.g., E1101TagInvalidFormat or the same message used
there) and do not skip remaining validation flow in validate_project; replace
the Err(_) => continue arm in the tag_re lookup with logic that
constructs/pushes the appropriate ValidationError for the tag (including the
underlying error text) so the validator fails visibly rather than silently
continuing.
🪄 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: d2239650-d3f4-4166-91d1-cf0c7a60a549
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
.claude-plugin/marketplace.json.claude/.claude-plugin/plugin.json.claude/skills/commit/SKILL.mdCHANGELOG.mdCargo.tomlbuild.rsdocs/rfc/RFC-0002.mdgov/adr/ADR-0041-self-update-and-cargo-binstall-binary-distribution.tomlgov/releases.tomlgov/rfc/RFC-0002/clauses/C-GLOBAL-COMMANDS.tomlgov/rfc/RFC-0002/clauses/C-SELF-UPDATE.tomlgov/rfc/RFC-0002/rfc.tomlgov/work/2026-04-13-implement-self-update-command-and-cargo-binstall-support.tomlsrc/cli.rssrc/cmd/edit/engine.rssrc/cmd/edit/mod.rssrc/cmd/edit/path.rssrc/cmd/edit/rules.rssrc/cmd/edit/runtime.rssrc/cmd/lifecycle.rssrc/cmd/mod.rssrc/cmd/self_update.rssrc/cmd/tag.rssrc/command_router.rssrc/render.rssrc/signature.rssrc/terminal_md.rssrc/tui/mod.rssrc/validate.rssrc/write.rstests/common/mod.rstests/test_agent_dir.rstests/test_changelog.rstests/test_delete.rstests/test_describe.rstests/test_display_paths.rstests/test_edit.rstests/test_errors.rstests/test_guard.rstests/test_happy_path.rstests/test_help.rstests/test_init.rstests/test_lifecycle.rstests/test_lock.rstests/test_migrate.rstests/test_move.rstests/test_rfc_lifecycle.rstests/test_scan.rstests/test_source_scan.rstests/test_tags.rstests/test_verify.rs
✅ Files skipped from review due to trivial changes (7)
- .claude/.claude-plugin/plugin.json
- .claude-plugin/marketplace.json
- build.rs
- src/cmd/mod.rs
- gov/releases.toml
- gov/work/2026-04-13-implement-self-update-command-and-cargo-binstall-support.toml
- gov/adr/ADR-0041-self-update-and-cargo-binstall-binary-distribution.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- .claude/skills/commit/SKILL.md
- Cargo.toml
- src/cmd/self_update.rs
- gov/rfc/RFC-0002/clauses/C-SELF-UPDATE.toml
| // Compile regexes inline; these patterns are known-valid literals. | ||
| let html_comment = match Regex::new(r"<!--[\s\S]*?-->") { | ||
| Ok(r) => r, | ||
| Err(_) => return md.to_string(), | ||
| }; | ||
| let html_anchor = match Regex::new(r#"\s*<a\s+id="[^"]*"\s*></a>"#) { | ||
| Ok(r) => r, | ||
| Err(_) => return md.to_string(), | ||
| }; | ||
| let html_del = match Regex::new(r"</?del>") { | ||
| Ok(r) => r, | ||
| Err(_) => return md.to_string(), | ||
| }; | ||
| let md_link = match Regex::new(r"\[([^\]]+)\]\([^)]+\.md(?:#[^)]*)?\)") { | ||
| Ok(r) => r, | ||
| Err(_) => return md.to_string(), | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "terminal_md.rs" | head -20Repository: govctl-org/govctl
Length of output: 81
🏁 Script executed:
git ls-files src/ | grep -E "\.(rs|ts|tsx|js|jsx)$" | head -20Repository: govctl-org/govctl
Length of output: 436
🏁 Script executed:
cat -n src/terminal_md.rsRepository: govctl-org/govctl
Length of output: 6938
🏁 Script executed:
cat Cargo.toml | head -50Repository: govctl-org/govctl
Length of output: 1249
🏁 Script executed:
grep -E "^\[package\]|^edition|^rust-version" Cargo.tomlRepository: govctl-org/govctl
Length of output: 109
🏁 Script executed:
grep -A 20 "dependencies" Cargo.toml | grep -E "regex|lazy"Repository: govctl-org/govctl
Length of output: 72
🏁 Script executed:
grep -E "use std::sync" src/terminal_md.rsRepository: govctl-org/govctl
Length of output: 43
🏁 Script executed:
grep -E "lazy_static|once_cell" Cargo.tomlRepository: govctl-org/govctl
Length of output: 43
🌐 Web query:
LazyLock std::sync Rust stabilized version
💡 Result:
std::sync::LazyLock was stabilized in Rust 1.80.0, released on July 25, 2024.
Citations:
- 1: Stabilize
LazyCellandLazyLockrust-lang/rust#121377 - 2: https://www.infoq.com/news/2024/08/rust-1-80-lazy-globals/
- 3: https://doc.rust-lang.org/beta/std/sync/struct.LazyLock.html
- 4: https://blog.rust-lang.org/2024/07/25/Rust-1.80.0/
🏁 Script executed:
rg "strip_for_terminal" src/ --type rsRepository: govctl-org/govctl
Length of output: 87
🏁 Script executed:
rg "strip_for_terminal" --type rsRepository: govctl-org/govctl
Length of output: 87
🏁 Script executed:
rg "strip_for_terminal" src/Repository: govctl-org/govctl
Length of output: 746
🏁 Script executed:
rg "Regex::new" src/terminal_md.rs -A 2 -B 2Repository: govctl-org/govctl
Length of output: 709
Avoid per-call regex compilation and silent fallback on compile failure.
At Lines 14/18/22/26, compiling regexes on each invocation introduces avoidable overhead in a rendering helper called frequently; at Lines 16/20/24/28, returning raw markdown on Err(_) silently bypasses normalization. For literal, known-valid patterns, precompile once with LazyLock and fail fast with expect().
Proposed fix
+use std::sync::LazyLock;
use regex::Regex;
+static HTML_COMMENT_RE: LazyLock<Regex> =
+ LazyLock::new(|| Regex::new(r"<!--[\s\S]*?-->").expect("valid HTML comment regex"));
+static HTML_ANCHOR_RE: LazyLock<Regex> =
+ LazyLock::new(|| Regex::new(r#"\s*<a\s+id="[^"]*"\s*></a>"#).expect("valid HTML anchor regex"));
+static HTML_DEL_RE: LazyLock<Regex> =
+ LazyLock::new(|| Regex::new(r"</?del>").expect("valid HTML del regex"));
+static MD_LINK_RE: LazyLock<Regex> =
+ LazyLock::new(|| Regex::new(r"\[([^\]]+)\]\([^)]+\.md(?:#[^)]*)?\)").expect("valid markdown link regex"));
+
pub fn strip_for_terminal(md: &str) -> String {
- // Compile regexes inline; these patterns are known-valid literals.
- let html_comment = match Regex::new(r"<!--[\s\S]*?-->") {
- Ok(r) => r,
- Err(_) => return md.to_string(),
- };
- let html_anchor = match Regex::new(r#"\s*<a\s+id="[^"]*"\s*></a>"#) {
- Ok(r) => r,
- Err(_) => return md.to_string(),
- };
- let html_del = match Regex::new(r"</?del>") {
- Ok(r) => r,
- Err(_) => return md.to_string(),
- };
- let md_link = match Regex::new(r"\[([^\]]+)\]\([^)]+\.md(?:#[^)]*)?\)") {
- Ok(r) => r,
- Err(_) => return md.to_string(),
- };
-
- let s = html_comment.replace_all(md, "");
- let s = html_anchor.replace_all(&s, "");
- let s = html_del.replace_all(&s, "");
- let s = md_link.replace_all(&s, "$1");
+ let s = HTML_COMMENT_RE.replace_all(md, "");
+ let s = HTML_ANCHOR_RE.replace_all(&s, "");
+ let s = HTML_DEL_RE.replace_all(&s, "");
+ let s = MD_LINK_RE.replace_all(&s, "$1");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/terminal_md.rs` around lines 13 - 29, The regexes html_comment,
html_anchor, html_del, and md_link are being compiled on every call and silently
return the raw markdown on compile errors; move these known-valid literal
patterns to static, precompiled Regex instances (e.g., using std::sync::LazyLock
or once_cell::sync::Lazy) so compilation happens once at startup, and replace
the Err(_) branches with expect() so the program fails fast with a clear message
if a pattern is wrong; update the code that currently defines html_comment,
html_anchor, html_del, and md_link to reference the newly created static Regex
values instead of calling Regex::new() per-invocation.
Add [lints.clippy] with deny rules for unwrap_used, expect_used, and panic. Refactor all production code and tests to use Result + ? instead. - Production: replace LazyLock<Regex>.expect() with inline Regex::new()?, .parent().unwrap() with .ok_or_else()?, and all .expect() on Options with .ok_or_else()? - Tests: convert all test functions to return Result<(), Box<dyn Error>>, replace unwrap/expect with ?, update common test helpers to return Result - Build script: exempt via crate-level allow (panicking on failure is correct)
lucifer1004
force-pushed
the
feat/self-update
branch
from
c108579 to
5b84011
Compare
1 participant
Summary
Add a
govctl self-updatecommand for GitHub Releases-based binary updates, and addcargo-binstallmetadata so users can install prebuiltgovctlbinaries without building from source.What Changed
govctl self-update [--check]self_updatecrate to check GitHub Releases and replace the current binarypackage.metadata.binstallsocargo binstall govctlcan fetch release artifacts directly0.8.3Behavior
govctl self-update --checkreports whether a newer release is availablegovctl self-updatedownloads and installs the latest release binary for the current platformcargo binstall govctlnow resolves the published release archives using repo metadataGovernance
C-SELF-UPDATEand amend global command coverageself_update+cargo-binstallas the release and distribution strategyVerification
cargo test -qcargo run --quiet -- checkcargo clippy --all-targets --all-features -- -D warningsNotes
Summary by CodeRabbit
New Features
govctl self-updatecommand to update the binary from GitHub Releases.--checkflag to report available updates without installing.Release