Skip to content

fix: make blob project-lock hashes comparable during updates - #1867

Open
AndreaCovelli wants to merge 1 commit into
vercel-labs:mainfrom
AndreaCovelli:agent/skip-unchanged-project-skills
Open

AndreaCovelli wants to merge 1 commit into
vercel-labs:mainfrom
AndreaCovelli:agent/skip-unchanged-project-skills

Conversation

@AndreaCovelli

@AndreaCovelli AndreaCovelli commented Aug 4, 2026 •

Copy link
Copy Markdown
Contributor

skills update -p can still reinstall unchanged skills installed through the GitHub blob fast path. Nested snapshots can record a server hash that differs from the cloned folder hash; root snapshots install only SKILL.md, while updates hash the entire repository.

This change computes blob snapshot hashes locally from downloaded relative paths and contents, records computedHashScope: 'skill-file' for root snapshots, and makes the existing update hash check honor that scope. Both runAdd and installFromSource record it through the shared project lock writer introduced in #2401.

Rebased onto current main (48dc9e8). Commit 4843f2f already implements project update skipping, so this PR is now narrowed to the remaining blob hash compatibility fixes. It addresses the hash mismatch reproduced in @tenequm's field-test report.

Locks without computedHashScope continue to use folder hashes. Existing blob entries acquire the corrected hash and scope when reinstalled. Relocation and failure handling continue through the existing update flow.

Validation:

  • Full suite: 68 files, 936 tests passed.
  • Focused suite: 7 files, 133 tests passed.
  • Integration coverage exercises real blob hashing, installation, lock writing, and update checks for root and nested skills through both add and installFromSource; unchanged content skips reinstalling, and changed content triggers an update.
  • Type check, formatting check, and build passed on Node 24.21.0.

tenequm added a commit to tenequm/dotfiles that referenced this pull request Aug 12, 2026
Upstream skills update never compares computedHash, so it reinstalls every
project skill on every run. Alias runs the local build carrying
vercel-labs/skills#1867 (fork: tenequm/vercel-skills).
@tenequm

tenequm commented Aug 12, 2026

Copy link
Copy Markdown

Ran this patch locally across 9 projects / 30 lock entries. It does what it says: a 10-skill project went from 10 reinstalls per run to 0, and a 3-skill project from 3 to 0. Type-check and the update suites pass.

One defect that affects this PR, though, and the current tests can't catch it.

src/add.ts:1911 writes two different kinds of hash into the same computedHash field:

const computedHash =
  blobResult && 'snapshotHash' in skill
    ? (skill as BlobSkill).snapshotHash
    : await computeSkillFolderHash(skill.path);

When add resolves through the skills.sh blob path, the lock stores snapshotHash, which is either the server-provided download.hash or computeSnapshotHash(files) (src/blob.ts:644). This PR always compares against computeSkillFolderHash over a cloned folder, so for any blob-sourced skill the two are structurally incomparable and the skill reinstalls on every run.

Reproduced with vercel-labs/agent-browser installed project-scoped. Its lock computedHash stayed at a674b7d8... across repeated skills update -p runs, each one reporting "Updated". Recomputing computeSkillFolderHash over a fresh clone of skills/agent-browser gives 548a1f92... folder-relative and 69f58240... repo-relative. Neither matches, so the mismatch is the hash basis, not upstream content.

Note that computeSnapshotHash (src/blob.ts:487) uses the same sorted-path + sha256 construction as computeSkillFolderHash, so this is a file-set / path-prefix difference plus the server-hash case, not a different digest.

The regression tests here mock computeSkillFolderHash directly, so they exercise only the clone-sourced path and pass regardless.

This isn't a blocker: blob-sourced skills just keep today's main behavior, and everything else gets correctly skipped. But it's worth either recording which basis produced the hash, or having add always write a clone-comparable one, otherwise the fix silently doesn't apply to a subset of installs.

@AndreaCovelli

Copy link
Copy Markdown
Contributor Author

Thanks @tenequm — excellent catch, and especially helpful reproduction details.

I pushed 2dab585 to address both parts of the hash-basis mismatch:

  • blob snapshot hashes are now computed locally from the downloaded file paths and contents instead of trusting the potentially incomparable server hash
  • root-level blob installs, which intentionally install only SKILL.md, now record that narrower hash scope so updates compare the same file set

I also reproduced your exact vercel-labs/agent-browser case in a disposable project. The project lock now records 548a1f92…, matching the cloned skill-folder hash, and two consecutive update -p runs performed zero reinstalls. Regression coverage now exercises the snapshot basis, root-only scope, and update comparison.

Thanks again for testing this so thoroughly.

@AndreaCovelli

Copy link
Copy Markdown
Contributor Author

@quuu When you have a chance, could you take a look at this follow-up to #1865? It refreshes #1371 against current main, has now been field-tested across 9 projects / 30 lock entries, and all CI checks are green.

The field test caught a blob-install hash-basis gap; that is addressed in 2dab585 and was verified end to end with vercel-labs/agent-browser producing stable no-op updates. Happy to adjust the approach or scope.

@AndreaCovelli
AndreaCovelli force-pushed the agent/skip-unchanged-project-skills branch from 2dab585 to 7f82fca Compare September 23, 2026 08:29
@AndreaCovelli

AndreaCovelli commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor Author

@quuu I rebased this onto current main (v1.7.0) and resolved the conflicts with the relocated-skill update flow from #2124.

I also checked the other open update PRs. #1371 is the exact older duplicate, but it is currently conflicting and does not include the blob hash-basis fix, relocation handling, or regression coverage here. #565 is an older broader project-lock design and is also conflicting; #544 covers local filesystem sources; #958 explicitly defers project-scope hash checks. Current main still unconditionally reinstalls every resolved project skill, so this change is still needed.

The rebased branch keeps moved-skill migration and fail-closed ambiguity handling, skips unchanged exact-path skills, and conservatively reinstalls only the affected skill when hashing alone fails.

Validation: focused suite 126/126, type-check, formatting, and build pass. After making the rebased assertions platform-aware and repairing the stale private-repo test mock from current main, the full suite passes 881/881.

@AndreaCovelli
AndreaCovelli force-pushed the agent/skip-unchanged-project-skills branch from dc6e691 to b7a0143 Compare October 7, 2026 20:39
@AndreaCovelli AndreaCovelli changed the title fix(update): skip unchanged project skill reinstalls fix: make blob project-lock hashes comparable during updates Oct 7, 2026
@AndreaCovelli

Copy link
Copy Markdown
Contributor Author

@quuu I rebased this onto current main and narrowed its scope after 4843f2f landed the original project-skill hash comparison.

The remaining fix covers blob installs: nested snapshots now compute their hash locally, and root-level snapshots record the SKILL.md-only hash scope so updates compare the same file set. Both add and installFromSource record that scope through the shared project lock writer from #2401.

The PR is now one commit (b7a0143). New integration tests exercise real blob hashing, installation, lock writing, and update checks through both install paths, verifying that unchanged content skips reinstalling and changed content triggers an update. The full suite passes 936/936, and all CI checks are green across Ubuntu and Windows on Node 22.20.0, 24, and 26, including type-check, formatting, and build.

Could you take another look when you have a chance?

This branch has not been deployed

No deployments
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