Repository navigation
feat: trust levels, checksum updates, lock.json, custom taps - #105
Conversation
- Hoist cloneResult above try block so catch can clean up temp dir on any failure path, not just the happy path - Always include GitHub API in find search results (was behind --federated flag, now default)
Show trust badges for each skill during install: [Official] for known official sources (Anthropic, Vercel, Expo, Remotion, Supabase, Stripe), or [Trusted]/[Review]/[Caution] based on TrustScorer analysis.
Compute SHA-256 checksum of SKILL.md on install and store in metadata. During update, check GitHub pushedAt timestamp and compare checksums to skip unnecessary re-downloads when content hasn't changed.
Introduce ~/.skillkit/lock.json with LockEntry/LockFile schemas to track all installed skills in a single file. Install writes entries, remove deletes them. Enables future team sync and reproducible setups.
Introduce tap add/remove/list commands to manage custom skill sources stored in ~/.skillkit/taps.json. Taps are included in find/search results, allowing users to add private or community repositories.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds persistent tap management and three new CLI tap commands; preloads taps into search and always enables GitHub in federated search; introduces lockfile and checksum utilities and types; updates install/update/remove flows to compute/persist checksums, update the lockfile, and surface trust badges. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant TapsFile as "Taps (file)"
participant Federated as "FederatedSearch"
participant GitHub as "GitHubSkillRegistry"
User->>CLI: run `skillkit find`
CLI->>TapsFile: loadTaps()
TapsFile-->>CLI: taps[]
CLI->>Federated: build registry list (marketplace + GitHub + taps)
Federated->>GitHub: query GitHub
GitHub-->>Federated: results
Federated-->>CLI: merged results
CLI-->>User: display results
sequenceDiagram
participant User
participant CLI
participant Trust as "TrustScorer"
participant FS as "Filesystem"
participant Lock as "Lockfile (core)"
User->>CLI: run `skillkit install <skill>`
CLI->>Trust: readSkillContent() -> score
Trust-->>CLI: score
CLI->>CLI: formatTrustBadge(score) -> print
CLI->>FS: install files -> computeSkillChecksum(path)
FS-->>CLI: checksum
CLI->>Lock: addSkillToLock(name, {source, checksum, path, agents, installedAt})
Lock-->>CLI: saved
CLI-->>User: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/find.ts`:
- Around line 63-66: The no-results message in the find command still tells
users to rerun with `skillkit find -f <query>`, which is inaccurate because the
`federated` Option.Boolean (variable `federated`) is now hidden/always-on;
update the code that emits the "no results" guidance (the branch that prints
when no search hits are found in packages/cli/src/commands/find.ts) to remove
the `-f`/`--federated` suggestion and replace it with an accurate alternative
(e.g., suggest checking spelling, broadening the query, or using available
filters/options), ensuring the message code references the existing `federated`
behavior and any user-visible helper text is consistent with
`Option.Boolean("--federated,-f", ...)`.
In `@packages/cli/src/commands/install.ts`:
- Around line 140-150: The temp clone (cloneResult) declared before the try can
leak because only the happy path and the catch currently clean it; wrap the
entire post-clone flow that may return early (everything after the resolved =
await this.resolveSource(s) and any subsequent user prompts/returns) in a
try/finally (or add a finally block to the existing try) and in that finally
call the clone cleanup for cloneResult (the same cleanup used in the happy/error
paths), ensuring resolveSource/cloneResult is checked for non-null before
calling its cleanup so every return path (user cancels selections or declines
confirmation) removes the temp clone.
- Around line 453-468: The trust output loop (using TrustScorer.score over
skillsToInstall and formatTrustBadge) prints only "Trust: <badge>", making the
badge ambiguous when multiple skills are shown; update the console output to
include the skill identifier (e.g., skill.name or fallback to skill.path)
alongside the badge so each line clearly maps badge → skill (locate the loop
referencing TrustScorer, readSkillContent, formatTrustBadge and change the
console.log that prints `Trust: ${badge}` to include the skill name).
- Around line 443-452: The isOfficial check currently uses
this.source.startsWith(prefix) which fails for full GitHub URLs or alternate
source formats; update the logic that computes isOfficial (refer to
officialSources and isOfficial) to first normalize this.source into a canonical
owner/repo form (strip protocol/git+ssh prefixes, trailing .git, and host parts,
or parse known URL patterns) and then compare the normalized owner/repo against
officialSources (or use startsWith on the normalized value); implement
normalization as a helper (e.g., normalizeSource or parseOwnerRepo) and use it
wherever isOfficial is computed so URLs and alternate formats receive the
[Official] badge.
In `@packages/core/src/config.ts`:
- Around line 177-186: The current addSkillToLock and removeSkillFromLock use
the skill name as the map key and overwrite/delete whole entries, which loses
agent membership and collides across sources; change them to derive a stable
unique key (e.g., concatenate entry.source + entry.subpath or another immutable
identifier) instead of using name, and update addSkillToLock to merge the entry
by combining/unioning the agents list (or other agent metadata) into an existing
lock.skills[uniqueKey] rather than replacing it; update removeSkillFromLock to
remove the agent from the merged entry and only delete lock.skills[uniqueKey]
when no agents remain. Ensure loadLockFile/saveLockFile usage remains the same
and adjust callers if they expect name-based lookup.
- Around line 162-175: loadLockFile and saveLockFile currently do an unsafe
read-modify-write on LOCK_FILE that allows lost updates under concurrent
processes; change this to obtain an exclusive lock before reading and hold it
until an atomic replace is complete: implement a locking helper (e.g., using a
fcntl/flock or an advisory lock library) used by loadLockFile/saveLockFile so
operations perform read → mutate → write while holding the lock, and make
saveLockFile write to a temp file in the same directory then fs.rename (atomic
replace) before releasing the lock; update both loadLockFile and saveLockFile
(and the analogous functions at the other block) to use the lock helper and
ensure the lock is always released in finally/cleanup.
🪄 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: 7ab0c4e7-c44c-4c6f-9163-9c23ef4b02f8
📒 Files selected for processing (12)
apps/skillkit/src/cli.tspackages/cli/src/commands/find.tspackages/cli/src/commands/index.tspackages/cli/src/commands/install.tspackages/cli/src/commands/remove.tspackages/cli/src/commands/tap.tspackages/cli/src/commands/update.tspackages/cli/src/helpers.tspackages/cli/src/onboarding/index.tspackages/cli/src/onboarding/theme.tspackages/core/src/config.tspackages/core/src/types.ts
👮 Files not reviewed due to content moderation or server errors (8)
- packages/cli/src/onboarding/theme.ts
- packages/cli/src/onboarding/index.ts
- packages/cli/src/helpers.ts
- packages/cli/src/commands/index.ts
- packages/cli/src/commands/remove.ts
- apps/skillkit/src/cli.ts
- packages/cli/src/commands/update.ts
- packages/core/src/types.ts
| export function loadLockFile(): LockFile { | ||
| if (!existsSync(LOCK_FILE)) return { version: 1, skills: {} }; | ||
| try { | ||
| return LockFile.parse(JSON.parse(readFileSync(LOCK_FILE, 'utf-8'))); | ||
| } catch { | ||
| return { version: 1, skills: {} }; | ||
| } | ||
| } | ||
|
|
||
| export function saveLockFile(lock: LockFile): void { | ||
| const dir = dirname(LOCK_FILE); | ||
| if (!existsSync(dir)) mkdirSync(dir, { recursive: true }); | ||
| writeFileSync(LOCK_FILE, JSON.stringify(lock, null, 2), 'utf-8'); | ||
| } |
There was a problem hiding this comment.
Lockfile writes can lose updates across concurrent commands.
These helpers do a plain loadLockFile() → mutate → saveLockFile() on a shared ~/.skillkit/lock.json. Two install/update/remove processes running at the same time can read the same snapshot and the last writer will silently discard the other change. This needs locking plus atomic replace semantics.
Also applies to: 177-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/config.ts` around lines 162 - 175, loadLockFile and
saveLockFile currently do an unsafe read-modify-write on LOCK_FILE that allows
lost updates under concurrent processes; change this to obtain an exclusive lock
before reading and hold it until an atomic replace is complete: implement a
locking helper (e.g., using a fcntl/flock or an advisory lock library) used by
loadLockFile/saveLockFile so operations perform read → mutate → write while
holding the lock, and make saveLockFile write to a temp file in the same
directory then fs.rename (atomic replace) before releasing the lock; update both
loadLockFile and saveLockFile (and the analogous functions at the other block)
to use the lock helper and ensure the lock is always released in
finally/cleanup.
- Use finally for temp cleanup so user cancels don't leak cloned dirs - Move addSkillToLock outside inner agent loop, merge agents array - Use source:name as lock key to avoid cross-source collisions - Gate GitHub activity check on provider.type (skip for GitLab/Bitbucket) - Normalize source URL to owner/repo before official check - Include skill name in trust badge output - Atomic lock.json writes via tmp file + renameSync - Remove stale --federated suggestion from find no-results message
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- install: use actual installed path (handles .md files) for lock entry and results instead of reconstructing from skillName - update: call addSkillToLock after both local and remote updates so lock.json stays in sync with metadata - update: --force bypasses GitHub activity and checksum skip-checks - update: timestamp comparison falls back to installedAt when updatedAt is not set
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if (!this.force && parsed && 'owner' in parsed && 'repo' in parsed && (provider.type === 'github' || provider.type === 'skills-sh')) { | ||
| const activity = await fetchGitHubActivity(parsed.owner, parsed.repo); | ||
| const localDate = metadata.updatedAt ?? metadata.installedAt; | ||
| if (activity?.pushedAt && localDate && activity.pushedAt <= localDate) { |
There was a problem hiding this comment.
🟡 String comparison of ISO dates with different precision formats can produce incorrect ordering
In packages/cli/src/commands/update.ts:117, activity.pushedAt (from GitHub API, format "2024-01-15T10:30:00Z" without milliseconds) is compared to localDate (from new Date().toISOString(), format "2024-01-15T10:30:00.000Z" with milliseconds) using string <=. When both dates represent the same second, the comparison is incorrect because 'Z' (char code 90) > '.' (char code 46), making the GitHub date always appear greater than the local date at the same second. This means the optimization to skip unchanged repos can never trigger for same-second dates. While the error direction is safe (unnecessary clone, not missed update), it should use Date object comparison for correctness.
| if (activity?.pushedAt && localDate && activity.pushedAt <= localDate) { | |
| if (activity?.pushedAt && localDate && new Date(activity.pushedAt).getTime() <= new Date(localDate).getTime()) { |
Was this helpful? React with 👍 or 👎 to provide feedback.
| addSkillToLock(skill.name, { | ||
| source: metadata.source, | ||
| sourceType: metadata.sourceType, | ||
| installedAt: metadata.installedAt, | ||
| updatedAt: metadata.updatedAt, | ||
| checksum: metadata.checksum, | ||
| agents: [], | ||
| path: skill.path, | ||
| }); |
There was a problem hiding this comment.
🟡 Lock file entries from update command always have empty agents array, losing agent tracking data
Both addSkillToLock calls in packages/cli/src/commands/update.ts:92-100 and packages/cli/src/commands/update.ts:160-168 pass agents: []. When addSkillToLock merges with an existing entry (created by a prior install), the empty array is harmlessly merged with the existing agents. However, for skills installed before the lock file feature was introduced, no prior lock entry exists, so a new entry is created with agents: [], permanently losing information about which agents have the skill installed. The update command has access to skill.path and could scan agent install directories to infer the correct agents list, or at minimum should not create a new lock entry when the agents are unknown.
Prompt for agents
In packages/cli/src/commands/update.ts, both addSkillToLock calls (lines 92-100 for local path updates and lines 160-168 for remote updates) pass agents: []. This means when no prior lock entry exists for a skill (e.g. it was installed before the lock file feature), a new lock entry is created with an empty agents array, losing information about which agents have the skill installed.
The update command knows the skill.path but not which agents it's installed for. Possible approaches:
1. Scan all agent install directories (via getSearchDirs/getAllAdapters) to find which agents have this skill and populate the agents array.
2. Only call addSkillToLock when a prior lock entry exists (to update checksum/timestamp) and skip it otherwise, letting the install command be the sole creator of lock entries.
3. Accept the empty array but document it as intentional for migrated skills.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export function computeSkillChecksum(skillPath: string): string { | ||
| const mdPath = skillPath.endsWith('.md') ? skillPath : join(skillPath, 'SKILL.md'); | ||
| if (!existsSync(mdPath)) return ''; | ||
| return createHash('sha256').update(readFileSync(mdPath)).digest('hex').slice(0, 16); | ||
| } |
There was a problem hiding this comment.
🟡 computeSkillChecksum only hashes SKILL.md, causing update to silently skip changes to other files in directory-based skills
The new computeSkillChecksum function at packages/core/src/config.ts:154-157 only computes a hash of the SKILL.md file. However, directory-based skills can contain additional files (helper scripts, configs, templates, code) that are also copied during install/update via cpSync(sourcePath, skill.path, { recursive: true }). In packages/cli/src/commands/update.ts:144-150, the checksum is used to skip updates: if only non-SKILL.md files changed upstream, newChecksum === metadata.checksum will be true, and the update is silently skipped with "already up to date". The user has no indication that actual changes exist. The --force flag bypasses this, but users would need to know to use it.
Prompt for agents
The computeSkillChecksum function in packages/core/src/config.ts only hashes SKILL.md, but directory-based skills can have additional files (scripts, configs, etc.) that are also deployed during install/update. When the update command at packages/cli/src/commands/update.ts:144-150 uses this checksum to decide whether to skip an update, changes to non-SKILL.md files are missed.
Consider extending the function to hash all files in the skill directory when the skill is directory-based. For example, recursively read all files, sort them by path, and concatenate their contents (or hashes) to produce a stable checksum of the entire skill. For standalone .md files, the current behavior is correct.
Alternatively, document that the checksum only covers SKILL.md content and that --force should be used if other files may have changed.
Was this helpful? React with 👍 or 👎 to provide feedback.
- Fix "44 agents" → "45 agents" across 34 files (website, docs, package.json, tests, extension, API, TUI) - README: add tap, check, update, publish submit to commands section - Commands.tsx: add Sources category with tap commands, update manage group with remove --source/--all and check - commands.mdx: full docs for tap, check, update, remove, publish submit with all new flags and behaviors - configuration.mdx: document lock.json and taps.json - security.mdx: document trust badges system
| if (existing) { | ||
| const mergedAgents = [...new Set([...existing.agents, ...entry.agents])]; | ||
| lock.skills[key] = { ...entry, agents: mergedAgents }; |
There was a problem hiding this comment.
🟡 addSkillToLock merge overwrites installedAt instead of preserving the original install date
When a skill already exists in the lock file (e.g., the user installs the same skill to additional agents), the merge logic at packages/core/src/config.ts:185 does { ...entry, agents: mergedAgents }, which takes ALL fields from the new entry — including installedAt — and only overrides agents with the merged list. This means the original installation date is lost and replaced with the current timestamp.
The install command always passes installedAt: new Date().toISOString() (packages/cli/src/commands/install.ts:608), so on every subsequent install of the same skill, the original install date is silently discarded. This directly contradicts the documented contract in docs/fumadocs/content/docs/configuration.mdx:179 which states installedAt is the "ISO timestamp of initial installation".
Example scenario
- User runs
skillkit install anthropics/skills --agent claude-code→ lock entry created withinstalledAt: "2026-04-01T12:00:00Z" - User runs
skillkit install anthropics/skills --agent cursor→ merge triggers, agents correctly become["claude-code", "cursor"], butinstalledAtis overwritten to"2026-04-11T10:00:00Z"
| if (existing) { | |
| const mergedAgents = [...new Set([...existing.agents, ...entry.agents])]; | |
| lock.skills[key] = { ...entry, agents: mergedAgents }; | |
| if (existing) { | |
| const mergedAgents = [...new Set([...existing.agents, ...entry.agents])]; | |
| lock.skills[key] = { ...entry, installedAt: existing.installedAt, agents: mergedAgents }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Six infrastructure features that wire up existing but underutilized code.
1. Quarantine cleanup on failure
Temp directory is now cleaned up on any error path, not just the happy path.
2. Federated search by default
skillkit findnow always includes GitHub API results. The--federatedflag is deprecated (always on).3. Trust badges on install
Install confirmation shows trust level for each skill:
[Official]for known official sources (Anthropic, Vercel, Expo, Remotion, Supabase, Stripe)[Trusted]/[Review]/[Caution]for community sources via TrustScorer4. Checksum-based update detection
skillkit updatechecks GitHubpushed_atbefore cloning5. Centralized lock.json
Single
~/.skillkit/lock.jsontracks all installed skills across agents. Written on install, updated on update, cleaned on remove. Per-skill.skillkit.jsonkept for backward compat.6. Custom taps
skillkit tap add owner/repo --name "My Skills" skillkit tap list skillkit tap remove owner/repoUser-managed source list at
~/.skillkit/taps.json. Tap sources appear inskillkit findresults.Test plan
pnpm build— 13/13 taskspnpm test— all tests passskillkit install anthropics/skills --skill pdf— shows[Official]badgeskillkit update— skips unchanged skills with "Already up to date"skillkit find react— includes GitHub API results by defaultskillkit tap add owner/repo && skillkit tap listcat ~/.skillkit/lock.json— has entries after installSummary by CodeRabbit
New Features
tap add/tap remove/tap listCLI commands with persistent taps storage and registration in the CLI.Improvements