Make init/install safe by default - #23
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThreads a new Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as "CLI Handler"
participant Installer as "FileSystemHookInstaller"
participant FS as "File System"
User->>CLI: git smee install [--force]
CLI->>Installer: from_default_with_force(force)
CLI->>Installer: install_hook("pre-commit")
Installer->>FS: stat/read hook file
alt file exists
FS-->>Installer: file contents
Installer->>Installer: is_managed_file(path) -> bool
alt is_managed OR force == true
Installer->>FS: write managed header + new content (overwrite)
FS-->>Installer: write OK
Installer-->>CLI: success
else unmanaged AND force == false
Installer-->>CLI: Err(RefusingToOverwriteUnmanagedHookFile)
end
else not exists
Installer->>FS: write managed header + content (create)
FS-->>Installer: write OK
Installer-->>CLI: success
end
CLI-->>User: Hooks installed or error
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/git-smee-core/src/installer.rs`:
- Around line 200-238: The current is_managed_file scans the whole file for
MANAGED_FILE_MARKER which can false-positive on unrelated occurrences; change
detection to only inspect the file header: read only the start of the file (or
the first two lines), handle an optional shebang (#!) on line 1 and then check
that the next line equals or starts with MANAGED_FILE_MARKER (use
MANAGED_FILE_MARKER.as_str()), and update ensure_can_write_hook and
ensure_can_write_config behavior by relying on this new header-only
is_managed_file implementation so unmanaged files with the marker elsewhere are
not treated as managed.
🧹 Nitpick comments (1)
crates/git-smee-core/src/installer.rs (1)
8-14: Consolidatewith_managed_headerto avoid duplicate helpers.The provided context shows a
with_managed_headeralready incrates/git-smee-core/src/config.rs(lines 11-13). Consider re-exporting a single helper to prevent drift.
## What changed - added a dedicated `SmeeConfig::validate()` pass and invoke it during config load - reject empty hook lists and empty/whitespace commands with explicit `ValidationError` variants that include hook name and entry index - reject unknown fields in `HookDefinition` via `#[serde(deny_unknown_fields)]` - expanded config integration coverage for whitespace commands and unknown hook entry fields - added a CLI integration test verifying `install` reports config validation failures - removed a stray assertion from `installer_integration.rs` that prevented workspace tests from compiling ## Validation - `cargo test --workspace` Closes git-smee-4q5. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Configuration validation now enforces non-empty hook entries and commands, reporting detailed errors that identify the specific hook and entry with issues. * Unknown configuration fields are now rejected during parsing. * **Tests** * Added integration tests covering validation error reporting for invalid configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Owner
Author
|
Follow-up on the nitpick about duplicate |
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)
crates/git-smee-core/src/installer.rs (1)
270-278:⚠️ Potential issue | 🟡 MinorWrong error variant used for config file write failure.
install_config_filemaps write errors toError::FailedToWriteHook, which produces a misleading error message referencing "hook" for a config file failure.🐛 Proposed fix: add a dedicated error variant or rename appropriately
Option 1: Add a new error variant:
#[error("Failed to write config file '{path}': {source}")] FailedToWriteConfigFile { path: String, #[source] source: std::io::Error, },Then update
install_config_file:- fs::write(&config_path, config_content).map_err(|source| Error::FailedToWriteHook { + fs::write(&config_path, config_content).map_err(|source| Error::FailedToWriteConfigFile { path: config_path.to_string_lossy().to_string(), source, })?;Option 2: Rename the existing variant to something generic like
FailedToWriteFile.
🤖 Fix all issues with AI agents
In `@crates/git-smee-core/src/installer.rs`:
- Around line 243-247: The loop that scans the header bytes (the for loop
iterating over header.split(|byte| *byte == b'\n').take(8)) incorrectly uses
break when encountering an empty normalized_line, which can miss markers after a
shebang plus blank line; update the loop in installer.rs (the block handling
header/normalized_line) to skip empty lines instead of breaking (replace the
break with continue) or remove the empty-line check entirely so the subsequent
lines (like "# THIS FILE IS MANAGED BY git-smee") are still examined within the
take(8) window.
- Around line 229-256: Move the per-iteration format allocations out of
is_managed_file: compute the two marker byte sequences once before the loop
(e.g., create marker_hash = format!("# {MANAGED_FILE_MARKER}").into_bytes() and
marker_rem = format!("REM {MANAGED_FILE_MARKER}").into_bytes() or equivalent)
and then compare normalized_line to marker_hash.as_slice() and
marker_rem.as_slice() inside the loop; this keeps the detection logic the same
but avoids reallocating the format strings on every iteration.
Owner
Author
|
Addressed the outside-diff note in commit |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/git-smee-core/src/installer.rs`:
- Around line 2-15: The helper with_managed_header currently always prepends the
marker with a '#' which can end up above a script shebang; change it to accept a
comment prefix parameter (e.g., fn with_managed_header(content: &str,
comment_prefix: &str) -> String) so callers can choose the correct prefix for
their target file (or add an overload/variant for script-safe behavior), and
update all call sites to pass the appropriate prefix; also update the doc
comment on with_managed_header to state its intended use and mention that
callers should use a script-safe prefix (or not use the helper) for hook
scripts.
Owner
Author
|
Addressed the remaining review request in commit
I also replied on the inline thread with details. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/git-smee-core/src/installer.rs`:
- Around line 2-31: The current with_managed_header_with_prefix inserts the
marker before the shebang when content starts with "#!" but contains no newline;
update the logic in with_managed_header_with_prefix to detect starts_with("#!")
separately: if content starts_with("#!") then try to find the first '\n' and, if
found, split at shebang_end as now; otherwise (no newline) treat the entire
content as the shebang line and return a string composed of the shebang
(content), then a newline, then the marker_line, then two newlines (so the
shebang remains first and executable). Keep all other behavior unchanged for
non-shebang input and reference the function name
with_managed_header_with_prefix and the constant MANAGED_FILE_MARKER.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This was referenced Jun 14, 2026
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.
What changed
git smee installnow only overwrites managed hook files by default--forcetogit smee installandgit smee initfor explicit overwrite behaviorgit smee initnow writes a managed header marker into generated config filesTests
Summary by CodeRabbit
New Features
Behavior
Validation
Tests