Skip to content

refactor(cli): share diagnostics hook inspection - #171

Merged
errfld merged 1 commit into
mainfrom
gh-164/shared-diagnostics-inspection
Jun 25, 2026
Merged

refactor(cli): share diagnostics hook inspection#171
errfld merged 1 commit into
mainfrom
gh-164/shared-diagnostics-inspection

Conversation

@errfld

@errfld errfld commented Jun 25, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add a shared CLI diagnostics hook-inspection module for managed/unmanaged/missing/stale hook checks.
  • Update doctor and status to use the shared inspection and expected-wrapper drift helpers.
  • Keep existing text/JSON output behavior stable while adding focused unit coverage for the extracted helpers.

Refs #164

Validation

  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo test --workspace --all-targets --all-features
  • Manual smoke in a temporary Git repository: git smee init, git smee install, git smee doctor, git smee doctor --json, git smee status, git smee status --json; parsed JSON statuses were ok.

Summary by CodeRabbit

  • New Features

    • Improved status and doctor output to better identify managed, missing, invalid, unreadable, and stale hook scripts.
    • Added detection for obsolete managed hooks that no longer match configured lifecycle phases.
  • Bug Fixes

    • More accurately distinguishes managed hooks from hooks that only contain similar content.
    • Path display in reports is now normalized for clearer, repository-relative output.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e369bdb7-dc9b-41c4-933f-6a116e60360e

📥 Commits

Reviewing files that changed from the base of the PR and between 61c3a81 and d26acb7.

📒 Files selected for processing (4)
  • crates/git-smee-cli/src/diagnostics.rs
  • crates/git-smee-cli/src/doctor.rs
  • crates/git-smee-cli/src/main.rs
  • crates/git-smee-cli/src/status.rs

📝 Walkthrough

Walkthrough

Adds a shared diagnostics module for hook inspection and repository-relative path display, then refactors doctor and status to use it for hook state, stale reasons, and obsolete-hook reporting.

Changes

Shared hook diagnostics refactor

Layer / File(s) Summary
Diagnostics core
crates/git-smee-cli/src/main.rs, crates/git-smee-cli/src/diagnostics.rs
Adds the shared hook-inspection types and helpers, including expected-script comparison, repository-relative path display, obsolete managed hook scanning, and tests.
Doctor hook inspection
crates/git-smee-cli/src/doctor.rs
Replaces inline wrapper checks with inspect_hook, HookInspectionState, and stale-reason generation from ExpectedHookScript.
Status hook report
crates/git-smee-cli/src/status.rs
Rebuilds hook-state reporting and obsolete-hook detection on top of the shared diagnostics helpers and removes the local path formatter.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Possibly related PRs

  • errfld/git-smee#25: Introduces the hook-script content and executable/config path checks that this PR now centralizes in ExpectedHookScript.
  • errfld/git-smee#167: Refactors status hook/diagnostic state handling, which this PR further routes through shared diagnostics helpers.
  • errfld/git-smee#168: Refactors doctor hook inspection, which this PR replaces with shared inspection state and stale-reason logic.

Poem

A bunny hopped through hooks and paths,
and sniffed for staleness in its maths.
“Managed!” thumped the fluffy guide,
“Now shared diagnostics walk beside.”
🐇✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: a CLI refactor that shares hook inspection diagnostics across commands.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch gh-164/shared-diagnostics-inspection

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@errfld
errfld merged commit f77363a into main Jun 25, 2026
28 checks passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d26acb7937

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +122 to +123
let inspection = inspect_hook(repository_root, hooks_dir, phase);
matches!(inspection.state(), HookInspectionState::Managed { .. }).then_some(inspection)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep obsolete detection header-only

For unconfigured hooks, status only needs to know whether the git-smee managed marker is present, but this now reuses inspect_hook, which only returns Managed after fs::read_to_string succeeds. A stale managed hook with the marker in its header but non-UTF-8 bytes later in the file will be treated as Unreadable and filtered out here, so git smee status reports no obsolete hook even though the installer/pruning logic would still treat it as managed based on the header alone. Preserve the previous header-only check for obsolete hooks instead of requiring the whole file to decode as UTF-8.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants