feat(skills): report hash drift in skill info - #795
Conversation
Mirror plugin info: compare on-disk SKILL.md to the skills.lock hash so edited installs are visible. Also drop the nonexistent skills lock command from EXTENDING.md.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds ChangesSkill hash drift reporting
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SkillCLI
participant SkillInfo
participant SkillFile
User->>SkillCLI: run skill info
SkillCLI->>SkillInfo: request skill metadata
SkillInfo->>SkillFile: read SKILL.md and recorded hash
SkillInfo-->>SkillCLI: return HashDrift
SkillCLI-->>User: print or serialize drift status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 (1)
internal/cli/distribution_test.go (1)
86-101: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd JSON regression assertions.
This test only exercises human-readable output, while the PR also promises
hashDriftin JSON. Check bothfalsebefore editing andtrueafter editing; otherwise theomitemptyregression can pass unnoticed.As per coding guidelines, add regression tests for behavior changes and keep them beside the source.
🤖 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 `@internal/cli/distribution_test.go` around lines 86 - 101, Extend the distribution regression test around runWithDeps and the skill info command to request JSON output and assert hashDrift is false before editing the skill, then true after editing it. Parse the JSON response and verify the field is present with the correct boolean value, while preserving the existing human-readable assertions.Source: Coding guidelines
🤖 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 `@docs/EXTENDING.md`:
- Line 111: Update the management-command examples in the documentation to use
the actual singular `zero skill add` and `zero skill remove` names, while
preserving the existing descriptions of their directory behavior.
In `@internal/skills/install.go`:
- Around line 504-518: Update skillHashDrift and its callers in
Info/InfoFromRoots so a locked skill whose os.ReadFile(skill.Path) fails is
surfaced as an error or marked as HashDrift=true, rather than returning a clean
false state; preserve the existing behavior for empty lock hashes and
successfully read files.
- Line 82: Update the HashDrift field’s JSON serialization so a computed
unchanged result is emitted as hashDrift: false instead of being omitted; remove
omitempty from its json tag, or use an equivalent pointer/custom-marshaler
representation if the no-lock “not computed” state must remain distinguishable.
---
Nitpick comments:
In `@internal/cli/distribution_test.go`:
- Around line 86-101: Extend the distribution regression test around runWithDeps
and the skill info command to request JSON output and assert hashDrift is false
before editing the skill, then true after editing it. Parse the JSON response
and verify the field is present with the correct boolean value, while preserving
the existing human-readable assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 734c8dff-6e45-46b1-bcbe-cc08571f7df2
📒 Files selected for processing (5)
docs/EXTENDING.mdinternal/cli/distribution.gointernal/cli/distribution_test.gointernal/skills/install.gointernal/skills/install_test.go
Merge main (Info moved to export_test). Treat unreadable locked SKILL.md as drift, always emit hashDrift in JSON, use singular skill CLI names in docs, and cover JSON output in CLI tests.
|
Addressed CodeRabbit review and the main conflict in 1ca2352:
|
|
cc: @kevincodex1 |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. Both of the earlier findings are genuinely fixed at this head, and I checked the part nobody had: whether the hash comparison itself can report false drift.
The unreadable-SKILL.md fail-open is properly closed (it now returns true with the intent stated in the comment, and the test pins both branches), and the omitempty tag is gone. On the hash side I installed skills whose SKILL.md was all-CRLF, had no trailing newline, and was mixed CRLF/LF: all three installed byte-identical and reported no drift, so the line-ending false-positive risk I was worried about does not exist. Neither side normalises, which is the right choice here. Name-collision and agents-only paths correctly report no hash rather than false drift.
One correction worth recording so it does not get re-raised: the claim that omitempty made a clean result serialise as absent was never user-visible. The only production JSON path goes through redaction.RedactValue, whose struct walk reads just the field name and never consults omitempty. Removing the tag is still the right defensive change, but the reported impact did not exist, and the new JSON assertion cannot catch a regression that re-adds it.
One minor for a follow-up, not blocking and not introduced here: InfoFromRoots swallows a ReadLock error behind if lock, err := ReadLock(...); err == nil, so a corrupt or truncated skills.lock leaves Hash empty and skillHashDrift short-circuits to false. A tampered SKILL.md plus a truncated lockfile then prints no hash, no drift, and exits 0. That is the same fail-open class this PR just fixed for an unreadable SKILL.md, and someone who can tamper with the skills directory can also truncate the lock, which is exactly the threat drift detection is for. It predates the PR, but this change is what makes it load-bearing.
LGTM.
3 participants
Summary
SKILL.mdno longer matchesskills.lockhashDriftinzero skill infotext + JSON (same idea as plugins info)skills lockcommandCloses #792
Test plan
go test ./internal/skills/ ./internal/cli/ -run 'Info|Skill|HashDrift|RunSkill' -count=1go vet ./internal/skills/ ./internal/cli/Summary by CodeRabbit
New Features
skill info(text and JSON), indicating whether an installedSKILL.mdmatches its recorded lock hash.Bug Fixes
Documentation