Skip to content

Comments

fix: resolve default branch instead of hardcoding main/master#406

Open
kjanat wants to merge 3 commits intovercel-labs:mainfrom
kjanat:fix/update-detect-default-branch
Open

fix: resolve default branch instead of hardcoding main/master#406
kjanat wants to merge 3 commits intovercel-labs:mainfrom
kjanat:fix/update-detect-default-branch

Conversation

@kjanat
Copy link

@kjanat kjanat commented Feb 21, 2026

Summary

skills update failed against a real repo (kjanat/paperless-mcp) because the reinstall path was rebuilt with a hardcoded /tree/main/..., while the repo’s default branch is master.

The only user-facing message was:

✗ Failed to update ...

…which gave no actionable reason.

This PR fixes that specific failure and makes branch handling more robust across update/hash paths so repos using non-standard default branches (master, develop, trunk, etc.) work correctly.

Closes #373.

Reproduction / observed behavior

  • skills check reported an update for paperless-ngx
  • skills update consistently failed with:
    • ✗ Failed to update paperless-ngx
  • Manually reproducing the generated install target surfaced the real error:
    • fatal: Remote branch main not found in upstream origin
  • Verified repo default branch:
    • git ls-remote --symref https://github.com/kjanat/paperless-mcp.git HEAD
    • ref: refs/heads/master HEAD

Root causes

  1. src/cli.ts (runUpdate) rebuilt install URLs using a hardcoded /tree/main/...
  2. src/skill-lock.ts (fetchSkillFolderHash) only tried main/master, which is brittle for repos with other default branches
  3. Update failures did not clearly surface the underlying clone/branch error

Changes

src/git.ts

  • Add resolveDefaultBranch() using git ls-remote --symref <repo> HEAD (via existing simple-git)
  • Add parseSymrefOutput() as a pure parser for easier testing
  • Fix .env() usage in cloneRepo (constructor env was not being applied as intended)

src/skill-lock.ts

  • fetchSkillFolderHash() now resolves the actual default branch first
  • If branch resolution fails, fall back to main/master and emit a warning

src/cli.ts

  • runUpdate() now uses GitHub shorthand (owner/repo/path) instead of constructing /tree/main/..., so clone/install follows the repo’s default branch naturally
  • Add block scoping braces to the remove switch case

src/remove.ts

  • Add optional _detectInstalledAgents DI hook in RemoveOptions for deterministic tests without vi.mock()

Tests

  • tests/resolve-default-branch.test.ts
    • Add coverage for standard, non-standard, malformed, and empty symref output
    • Add integration-level behavior tests
  • tests/remove-canonical.test.ts
    • Refactor from vi.mock() to DI for better compatibility with plain Node execution

Tooling

  • package.json
    • Add pnpm.onlyBuiltDependencies for esbuild

Testing

  • pnpm run exec:test — 17/17 passing
  • pnpm test — 346/346 passing

- Use git ls-remote --symref via simple-git to detect default branch
- Fix update URL to use GitHub shorthand instead of /tree/main/
- Fix simple-git .env() API usage in cloneRepo
- Fall back to main/master with stderr warning if resolution fails
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.

[Bug]: skills update hardcodes tree/main, breaks repos using master

1 participant