Add direct repository package specs for GitHub, GitLab, Bitbucket, and SourceHut - #38
Conversation
WalkthroughThe PR adds direct GitHub, GitLab, Bitbucket, and SourceHut package specifications. It parses and resolves repository refs, installs pinned snapshots, records manual lockfile entries, updates registry types, simplifies workspace paths, and revises command documentation. ChangesDirect repository package support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant addPackageReference
participant resolveDirectRepositoryRef
participant GitRepository
participant Lockfile
User->>addPackageReference: provide repository package specification
addPackageReference->>resolveDirectRepositoryRef: resolve repository and ref
resolveDirectRepositoryRef->>GitRepository: list HEADs, branches, and tags
GitRepository-->>resolveDirectRepositoryRef: remote repository refs
resolveDirectRepositoryRef-->>addPackageReference: normalized source and pinned ref
addPackageReference->>Lockfile: record manual repository entry
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
99de96d to
e3c47f4
Compare
3ccdec1 to
34ce2a8
Compare
34ce2a8 to
06af555
Compare
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The feature works and the npm path is intact, but repository specs that Packref cannot serve fail with errors that name the wrong subsystem, and the new add path has no test behind it.
Reviewed changes — full read of the 28-file diff for commit 06af555, checked against docs/plans/11-direct-repository-sources.md, with the spec parser exercised directly and the provider archive endpoints verified live.
- Tagged spec union —
ParsedPackageSpecbecomesRegistryPackageSpec | RepositoryPackageSpec, andresolvePackageReference/RegistryAdapter.resolveare narrowed to the registry variant so repository specs can never reach a registry adapter. - Repository spec detection —
checkIsRepositorySpecandparseRepositoryIdentityinsrc/lib/core/packages.tsrecognize provider shorthands, bareowner/repo, standard URLs, and SCP-style SSH URLs, falling through to the existing registry parsing when no supported provider is derived. - Ref pinning — new
resolveDirectRepositoryRefpins the default branch and branch refs to a 12-hex SHA, slash-free tags to the tag name, slash-containing tags to the full SHA, and SHA-shaped input verbatim. ls-remotewidened —git ls-remote --tagsbecomesgit ls-remoteso HEAD and branches are visible, with a newparseGitRemoteRefsOutput; the pre-existingparseGitRemoteTagsOutputfilters onrefs/tags/, so the registry-derived tag path is unaffected.- Reinstall path — direct repository entries use
entry.versionas the exact fetch ref instead of re-resolving through tag matching. - Store identity —
getPackageIdentitySegmentsnow splits any name containing/, producingpackages/<provider>/<owner>/<repo>/<version>. - Unrelated refactor — the path helpers in
workspace/paths.tsandsrc/lib/shared/path.tsare removed and inlined, andsrc/commands/remove.tsis rewritten offOption.
Two things I checked that came back clean, so nobody needs to redo them. The plan left "verify giget/codeload accepts abbreviated SHAs; fall back to the full SHA if not" (lines 54-56) as an open item — a 12-hex SHA resolves on all four provider archive endpoints, confirmed against live URLs with ffffffffffff negative controls returning 404, so no full-SHA fallback is needed. And I could not produce any npm spec that the new pre-registry branch misroutes: react@, @effect/cli@0.29.0, npm: react, npm:@effect/cli@0.29.0, lodash.get@4.4.2 all still parse as registry specs, and jsr:/pypi: still raise UnsupportedRegistryError. bun test passes 289/289 and bunx tsc --noEmit is clean.
⚠️ The direct-repository add path has no test behind it
resolveDirectRepositoryRef is unit-tested in tags.test.ts and the reinstall branch is tested in install.test.ts, but addDirectRepositoryReferenceToProject — the function that joins them and writes the lockfile entry — is never executed by a test. Nothing pins the entry shape (registry: "github", name: "owner/repo", tracking: "manual") or the packages/github/<owner>/<repo>/<version> store layout, which are the parts of the identity model most likely to drift.
Technical details
# Cover the direct repository add path
## Affected sites
- `src/lib/references/add.ts:177-204` — `addDirectRepositoryReferenceToProject` has no test exercising it
- `src/lib/references/__tests__/add.test.ts` — `addPackageReference` tests only pass `_tag: "registry"` specs
- `docs/plans/11-direct-repository-sources.md:112-117` — plan step 7 asks for an add-side test in `src/lib/references/__tests__/`
## Required outcome
- A test drives `addPackageReference` with a `_tag: "repository"` spec and asserts the resulting lockfile entry's `registry`, `name`, `version`, `tracking`, and `source`, plus the materialized reference path.
- The assertions pin exact values rather than existence, so a regression in the identity model fails the test.
## Suggested approach (optional)
- `add.test.ts` already builds a layer with a fake `RepositoryDownloader`; it needs a `RemoteTagReader` stub too. `tags.test.ts` has `runWithRemoteTagCommand`, which stubs `ls-remote` stdout — the same canned ref output would work here.
## Open questions for the human
- The plan also asks for a network-backed integration test against a real repository. Is that wanted in CI, or is a fully stubbed test the intended scope?ℹ️ The path-helper removal and remove.ts rewrite are unrelated to this feature
Roughly a third of the diff is refactoring that direct repository specs do not require: four helpers deleted from workspace/paths.ts and inlined at ~12 call sites, src/lib/shared/path.ts deleted with checkIsPathWithin relocated, and src/commands/remove.ts converted from Option/Array.match to undefined/ternaries. The lockfile path path.join(projectPath, PACKREF_DIRECTORY_NAME, LOCKFILE_NAME) is now spelled out in five places instead of one. This is a deliberate-looking direction rather than an accident, so the question is scope, not correctness.
Technical details
# Unrelated refactoring bundled with the feature
## Affected sites
- `src/lib/workspace/paths.ts:8-18` — `getDirectoryPath`, `getProjectLockfilePath`, `getGlobalDirectoryPath`, `getGlobalConfigPath` removed
- `src/lib/workspace/lockfile.ts:100,114,135,158` and `src/lib/references/clean.ts:32,38` — lockfile path now rebuilt inline at each site
- `src/lib/shared/path.ts` — deleted; `checkIsPathWithin` moved to `workspace/paths.ts`
- `src/commands/remove.ts:45-77,117` — `Option`-based selection rewritten to `undefined` and ternaries
## Required outcome
- Either split this refactor into its own commit or PR so the feature diff stays reviewable, or confirm it is an intentional codebase-wide direction so reviewers stop flagging it.
## Open questions for the human
- Is moving away from `Option` for internal absent-value plumbing a general convention now, or local to `remove.ts`?
- Was collapsing the named path helpers deliberate? Re-deriving the lockfile path at five call sites makes a future change to the on-disk layout a multi-site edit.ℹ️ Nitpicks
src/lib/core/packages.ts:44-49,98-103—SUPPORTED_REPOSITORY_PROVIDERSandREPOSITORY_PROVIDER_HOSTSexactly duplicateKNOWN_PROVIDERSandPROVIDER_HOSTSatsrc/lib/sources/repository/normalize.ts:17,23-28. Adding a fifth provider now means editing two tables that must agree, or the parser and the normalizer disagree about which hosts are supported.src/lib/sources/repository/normalize.ts:171-173— a slash-containing tag pins the full 40-hex SHA while HEAD and branch refs pin 12 hex (:166,:181), soPackageEntry.versionand the store directory name vary in width for no functional reason.src/lib/core/packages.ts:145,161—RegExp.execreturnsnull, notundefined, soscpMatch !== undefinedat:161is always true once reached and the`${owner}/${repository}`fallback at:164is unreachable. Harmless today because the SCP branch happens to produce the same string, but it will not stay that way.src/lib/core/packages.ts:171—value.slice(0, value.indexOf(":"))becomesslice(0, -1)when there is no colon, sogithubsandgithub@satisfycheckIsRepositoryProviderand enter the repository branch. They only escape because the derived name ends in/and hits the fallthrough guard.src/lib/core/packages.ts:174— the trailing(?:@[^@]+)?$requires at least one character after@, soowner/repo@misses the gate entirely and is handed to npm as a package namedowner/repo, whilegithub:owner/repo@correctly parses as a repository spec at the default branch. Worth accepting an empty trailing ref for consistency withreact@.
Claude Opus ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
src/lib/sources/repository/tags.ts (1)
160-160: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
git ls-remotewithout filters returns every ref.The command now returns all refs, including
refs/pull/*andrefs/merge-requests/*. Large repositories publish tens of thousands of such refs, so each lookup transfers and parses much more data than before. HEAD is required, so--heads --tagsalone is not sufficient. Considergit ls-remote --symref <url> HEAD "refs/heads/*" "refs/tags/*"to keep HEAD while excluding unrelated ref namespaces.🤖 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 `@src/lib/sources/repository/tags.ts` at line 160, Update the git invocation in the repository tag lookup to pass --symref, HEAD, and explicit refs/heads/* and refs/tags/* patterns to ls-remote, preserving HEAD resolution while excluding pull-request, merge-request, and other unrelated namespaces.src/lib/core/packages.ts (1)
170-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
checkIsRepositorySpecaccepts registry-prefixed specs.The SCP pattern on line 176 matches
npm:react@19.0.0andnpm:@effect/cli@0.29.0, because[^:]+:.+matches any prefixed spec. Those inputs enter the repository branch and only return to registry parsing becauseparseRepositoryIdentityproduces an unsupported host or a name that ends with/. The correct result depends on a fallback, not on the check. Exclude known registry prefixes from the SCP branch to make the intent explicit.♻️ Proposed refinement
const checkIsRepositorySpec = (value: string) => { const prefix = value.slice(0, value.indexOf(":")) + + if (checkIsRegistry(prefix)) { + return false + } + return ( checkIsRepositoryProvider(prefix) ||🤖 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 `@src/lib/core/packages.ts` around lines 170 - 178, Update checkIsRepositorySpec so its SCP-style pattern excludes known registry prefixes such as npm before evaluating the generic host:path match. Preserve repository-provider, repository shorthand, and URL detection while preventing registry-prefixed specs like npm:react@19.0.0 from entering the repository branch.src/lib/core/__tests__/packages.test.ts (1)
346-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the unresolved parsing edges.
The table covers GitHub and GitLab happy paths only. Add cases for
github.com/owner/repo, an unsupported host such ashttps://git.example.com/owner/repo, and a SourceHut spec such assourcehut:~owner/repo. Those inputs exercise the host default on line 146 ofsrc/lib/core/packages.tsand the fall-through on lines 258-274, so the tests would pin the intended behavior.🤖 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 `@src/lib/core/__tests__/packages.test.ts` around lines 346 - 397, Extend the “parses direct repository spec” table in the parsePackageSpec tests with cases for github.com/owner/repo, an unsupported HTTPS host such as https://git.example.com/owner/repo, and sourcehut:~owner/repo. Set each expected result to the intended behavior of the host-default and unsupported/sourcehut parsing branches, preserving the existing test structure.src/lib/references/add.ts (1)
177-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared materialization tail.
Lines 181-202 repeat lines 152-173 of
materializePackageReferenceToProject: create the project reference, build the entry, upsert it, and build theAddPackageResult. Only the store-entry acquisition differs. Extract the tail into one helper that takes the identity, the store entry, the manifest range, and the tracking mode.🤖 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 `@src/lib/references/add.ts` around lines 177 - 204, Extract the duplicated materialization tail from materializePackageReferenceToProject and addDirectRepositoryReferenceToProject into a shared helper. Have the helper accept the package identity, store entry, manifest range, and tracking mode, then createProjectReference, build and upsert the PackageEntry, and return the AddPackageResult; replace both inline tails while preserving their existing inputs and results.src/lib/sources/repository/normalize.ts (1)
162-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the
Matchpredicates.Each handler repeats
?? ""because the matched value keeps the optional types. Narrow once before the match, or destructure into locals, so the fallbacks disappear. The current form hides which branch can really produce an empty ref.🤖 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 `@src/lib/sources/repository/normalize.ts` around lines 162 - 191, Simplify the Match flow around the resolved value by narrowing or destructuring optional fields before matching, so each matched handler receives non-optional values where its predicate guarantees presence. Remove the redundant ?? "" fallbacks from the ref and version construction while preserving the existing branch selection and TagNotFoundError behavior.src/lib/references/__tests__/install.test.ts (1)
215-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case with a sha-shaped pinned version.
The entry uses
1.0.0, which is a tag-shaped version. Direct repository entries created from HEAD or from a branch store a 12-character sha instead. Add a second case with a sha-shaped version so the test covers the ref thatresolveDirectRepositoryRefactually writes to the lockfile.🤖 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 `@src/lib/references/__tests__/install.test.ts` around lines 215 - 234, The reinstall test currently covers only a tag-shaped pinned ref; add a second test case alongside “reinstalls a direct repository entry with its exact pinned ref” using a 12-character SHA-shaped version, initialize and install the entry with the existing helpers, and assert repositoryRefs contains that exact SHA to cover resolveDirectRepositoryRef’s lockfile behavior.
🤖 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 `@src/lib/references/install.ts`:
- Around line 60-68: Update the direct-repository branch in the install
resolution flow to obtain an exact commit ref rather than using entry.version,
and preserve the source.fetchSource === undefined validation performed by
resolveRepositoryRef. Reuse the repository normalization/resolution logic
associated with resolveDirectRepositoryRef and resolveRepositoryRef so
unsupported hosts still produce UnsupportedRepositoryHostError instead of
SnapshotFetchError.
In `@src/lib/references/remove.ts`:
- Line 73: Use the project-local .packref/packages root consistently in both
workflows: update projectDirectoryPath in src/lib/references/remove.ts at lines
73-73 and the corresponding path construction in src/lib/references/sync.ts at
lines 84-84 to include the packages directory, so getStorePackagePath resolves
the actual source tree for removal and synchronization.
In `@src/lib/sources/repository/__tests__/tags.test.ts`:
- Around line 292-316: Update resolveDirectRepositoryRef to normalize resolved
commit versions consistently, using the intended truncated SHA format for HEAD,
branch, nested-tag, and abbreviated-commit resolutions. Adjust the affected
expectedVersion values in the pins test table so every case reflects the same
stored version shape while preserving each expected repository.ref.
In `@src/lib/sources/repository/normalize.ts`:
- Around line 162-191: Update resolveDirectRepositoryRef in
src/lib/sources/repository/normalize.ts (lines 162-191) so every resolved
repository case stores the full resolved commit SHA in version, while retaining
tag names only as separate display metadata. In src/lib/references/install.ts
(lines 60-68), use the lockfile’s full pinned SHA as the download ref and
preserve the existing fetchSource === undefined bypass check. Update the
expected versions for the HEAD, branch, and tag cases in
src/lib/sources/repository/__tests__/tags.test.ts (lines 292-316) to the full
SHA.
In `@src/lib/workspace/paths.ts`:
- Around line 8-16: Update materializeStoreEntry and the checkIsPathWithin
validation flow to reject symlinked source paths before invoking
Reflinker.reflink. Resolve the candidate’s real path or inspect each path
component, then validate the resolved path remains within storePath; preserve
the existing lexical boundary checks for non-symlink paths.
---
Nitpick comments:
In `@src/lib/core/__tests__/packages.test.ts`:
- Around line 346-397: Extend the “parses direct repository spec” table in the
parsePackageSpec tests with cases for github.com/owner/repo, an unsupported
HTTPS host such as https://git.example.com/owner/repo, and
sourcehut:~owner/repo. Set each expected result to the intended behavior of the
host-default and unsupported/sourcehut parsing branches, preserving the existing
test structure.
In `@src/lib/core/packages.ts`:
- Around line 170-178: Update checkIsRepositorySpec so its SCP-style pattern
excludes known registry prefixes such as npm before evaluating the generic
host:path match. Preserve repository-provider, repository shorthand, and URL
detection while preventing registry-prefixed specs like npm:react@19.0.0 from
entering the repository branch.
In `@src/lib/references/__tests__/install.test.ts`:
- Around line 215-234: The reinstall test currently covers only a tag-shaped
pinned ref; add a second test case alongside “reinstalls a direct repository
entry with its exact pinned ref” using a 12-character SHA-shaped version,
initialize and install the entry with the existing helpers, and assert
repositoryRefs contains that exact SHA to cover resolveDirectRepositoryRef’s
lockfile behavior.
In `@src/lib/references/add.ts`:
- Around line 177-204: Extract the duplicated materialization tail from
materializePackageReferenceToProject and addDirectRepositoryReferenceToProject
into a shared helper. Have the helper accept the package identity, store entry,
manifest range, and tracking mode, then createProjectReference, build and upsert
the PackageEntry, and return the AddPackageResult; replace both inline tails
while preserving their existing inputs and results.
In `@src/lib/sources/repository/normalize.ts`:
- Around line 162-191: Simplify the Match flow around the resolved value by
narrowing or destructuring optional fields before matching, so each matched
handler receives non-optional values where its predicate guarantees presence.
Remove the redundant ?? "" fallbacks from the ref and version construction while
preserving the existing branch selection and TagNotFoundError behavior.
In `@src/lib/sources/repository/tags.ts`:
- Line 160: Update the git invocation in the repository tag lookup to pass
--symref, HEAD, and explicit refs/heads/* and refs/tags/* patterns to ls-remote,
preserving HEAD resolution while excluding pull-request, merge-request, and
other unrelated namespaces.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 608eb434-798c-4f9a-84cd-6ba9378d1bce
📒 Files selected for processing (28)
.changeset/bright-repos-arrive.mdREADME.mdsrc/commands/add.tssrc/commands/remove.tssrc/lib/core/__tests__/packages.test.tssrc/lib/core/packages.tssrc/lib/references/__tests__/install.test.tssrc/lib/references/__tests__/remove.test.tssrc/lib/references/add.tssrc/lib/references/clean.tssrc/lib/references/install.tssrc/lib/references/remove.tssrc/lib/references/sync.tssrc/lib/registries/index.tssrc/lib/registries/npm/__tests__/resolver.test.tssrc/lib/registries/registry.tssrc/lib/shared/path.tssrc/lib/sources/repository/__tests__/tags.test.tssrc/lib/sources/repository/normalize.tssrc/lib/sources/repository/tags.tssrc/lib/sources/tarball/fetch.tssrc/lib/store/paths.tssrc/lib/workspace/config.tssrc/lib/workspace/home.tssrc/lib/workspace/integration.tssrc/lib/workspace/lockfile.tssrc/lib/workspace/paths.tssrc/lib/workspace/project.ts
💤 Files with no reviewable changes (1)
- src/lib/shared/path.ts
|
@pullfrog review |
06af555 to
6d20d8e
Compare
There was a problem hiding this comment.
Important
Direct repository entries introduce a two-segment package name (owner/repo), but the global store traversal still assumes only @scope/name can be nested. packref prune will read those entries back as bogus identities and delete them while projects still reference them.
Reviewed changes — full read of the 28-file diff for PR #38, which adds direct repository package specs alongside registry specs.
- Discriminated
ParsedPackageSpec—RegistryPackageSpec(_tag: "registry") andRepositoryPackageSpec(_tag: "repository"), with registry resolution,remove, and the npm resolver narrowed to the registry variant. - Repository spec parsing —
checkIsRepositorySpec/splitRepositoryRef/parseRepositoryIdentityinsrc/lib/core/packages.tshandle GitHub shorthand, provider shorthand, standard Git URLs, and SCP-style SSH URLs, falling back to the registry path when no supported provider is derived. - Ref resolution and pinning —
resolveDirectRepositoryRefresolves the requested tag, branch, commit, or default branch via a newlistRefs, andgit ls-remotenow fetches all refs instead of only tags. - Reinstall path —
fetchLockedStoreEntryskips tag matching for direct repository registries and reuses the pinnedentry.versionas the fetch ref. - Path helper consolidation —
getDirectoryPath,getProjectLockfilePath,getGlobalDirectoryPath, andgetGlobalConfigPathare inlined at their call sites;checkIsPathWithinmoves from#lib/shared/path.tsinto#lib/workspace/paths.ts. - Docs — README, agent guidance, and the
addcommand/argument descriptions document the new spec forms.
I traced the classification order for the registry specs that already had coverage (react@19.0.0, @effect/cli@0.29.0, npm:react, npm: react, npm:@effect/cli@0.29.0, jsr:effect, pypi:requests): each either fails checkIsRepositorySpec or derives an undefined provider and falls through to the registry branch, so no existing spec is misclassified. The suite passes locally (289 tests).
⚠️ Two subdirectories of the same repository at the same ref cannot coexist
resolveDirectRepositoryRef builds the identity from name/registry/version only — repository.directory is not part of it. So packref add github:owner/repo/packages/a@v1 followed by packref add github:owner/repo/packages/b@v1 produces the same identity twice: upsertPackageEntry overwrites the first lockfile entry, and materializeStoreEntry reports the existing store entry as reused and hands back the first entry's source, so the second reference is silently reflinked to packages/a. The user gets the wrong directory with no error.
Technical details
# Directory is not part of the package identity for direct repository specs
## Affected sites
- `src/lib/sources/repository/normalize.ts:193-196` — identity is `{ name, registry, version }`; the resolved `directory` lives only on `repository.source`
- `src/lib/store/index.ts:196-215` — `materializeStoreEntry` keys on identity alone and returns the previously stored `source` when the entry exists, so a second add with a different directory reuses the first snapshot's source
- `src/lib/references/add.ts:180-193` — `createProjectReference` and the lockfile entry both consume `storeEntry.source`, propagating the wrong directory
- `src/lib/workspace/lockfile.ts` — `upsertPackageEntry` matches on `packageIdentityEquivalence`, so the second entry replaces the first
## Required outcome
- Two direct repository references that differ only by `directory` must either coexist correctly or be rejected with a clear error. Silently resolving the second to the first directory's contents is the outcome to eliminate.
## Open questions for the human
- Is monorepo-subdirectory support intended to be first-class here? If so, `directory` likely needs to participate in the identity (and therefore in the store path and lockfile key); if not, an explicit unsupported-combination error is the cheaper answer for this PR.ℹ️ Nitpicks
- The
Option→undefinedrewrite insrc/commands/remove.tsand the inlining of the four#lib/workspace/paths.tshelpers are unrelated to direct repository specs; bundling them makes the feature diff harder to review and to revert independently.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Important
The new commit fixes the ls-remote ref explosion and drops the unrelated refactors, but pinning tags to a SHA breaks packref remove owner/repo@v1.0. Two findings from the previous review are also still open.
Reviewed changes — delta between 06af555 (previous pullfrog review) and 6d20d8e, an amended force-push.
ls-remotescoped to refspecs — nowgit ls-remote <url> HEAD 'refs/heads/*' 'refs/tags/*', which restores the bound the--tagsremoval had lifted.- Unrelated refactors reverted —
src/commands/remove.ts,src/lib/workspace/paths.ts,src/lib/workspace/home.ts, and thesrc/lib/shared/path.tsdeletion are back to theirmainstate, leaving the diff focused on the feature. - Unsupported hosts now fail loudly —
parseRepositoryIdentityreturns the derivedhost, and an unambiguous repository locator (URL scheme or SCP form) on an unsupported host raisesUnsupportedRepositoryHostErrorinstead of falling through to a confusing "unsupported registry" error. The error message lists the supported providers. - Bare-host shorthand —
github.com/owner/repo,gitlab.com/owner/repo,bitbucket.org/owner/repo, andgit.sr.ht/~owner/reponow parse, via aHOST_REPOSITORY_PROVIDERSlookup on the first path segment. ThescpMatch === nullcomparisons were also corrected (the old!== undefinedchecks were always true). - Tag refs pin the commit — the tag match arm no longer special-cases slash-containing tags; every tag now resolves to a 12-character SHA.
- New coverage —
src/lib/references/__tests__/add.test.tsasserts the full lockfile entry, store path, and materialized content forgithub:owner/repo;packages.test.tsadds the bare-host and unsupported-host cases.
Suite passes locally (297 tests, up from 289). I re-traced the registry-vs-repository classification against the new checkIsRepositorySpec and isUnambiguousRepositoryLocator guards — npm:react, npm: react, npm:@effect/cli@0.29.0, react@, jsr:effect, and pypi:requests all still reach the registry parser.
⚠️ Still open from the previous review
Neither of these is touched by the amended commit, so they carry forward rather than being restated here:
- Global store traversal does not recognize two-segment repository names —
listStoreEntriesstill special-cases only@-prefixed segments, sopackref prunereadsowner/repo/<version>back as{ name: "owner", version: "repo" }and deletes it as unreferenced. The newadd.test.tsconfirms thepackages/github/owner/repo/<version>layout it mis-parses. - Two subdirectories of the same repository at the same ref collide —
directoryis still absent from the identity, so the secondaddsilently reuses the first snapshot's source.
The abbreviated-SHA round-trip question also still stands, and now applies to tag-resolved entries too: ref is the full 40-character SHA at add time while version — replayed as the fetch ref by install — is the 12-character prefix.
ℹ️ Nitpicks
- Bare SourceHut shorthand
~user/repo(nosourcehut:prefix, no host) falls into the default-GitHub branch, andnormalizeFromShorthandUrlstrips the tilde, so it silently resolves tohttps://github.com/user/repo— a valid but wrong repository rather than an error. The newly supportedgit.sr.ht/~user/repoform covers the same intent, so treating a leading~as a SourceHut signal (or rejecting it) would close the gap. - The PR description still states "Tags and lightweight refs resolve to the ref name itself", which the new tag arm no longer does.
Claude Opus ｜ 𝕏
6d20d8e to
1eab264
Compare
There was a problem hiding this comment.
Important
Both prior findings are genuinely fixed and the new tests pin exact values. Two things still need attention before merge: the CI format job fails on this commit, and the host-prefixed-shorthand fix covers only the four supported hosts — any other host-shaped first segment is still silently absorbed as the repository owner.
Reviewed changes — the delta 06af555..6d20d8e (13 files), read against the full PR diff, with parsePackageSpec executed directly on ~35 inputs and the repo's own check / format --check / typecheck / test commands run locally.
- Fixed host-prefixed shorthand — a new
HOST_REPOSITORY_PROVIDERSmap lets a schemeless locator whose first segment isgithub.com/gitlab.com/bitbucket.org/git.sr.htbe split into host + path, sogithub.com/owner/reponow resolves toowner/repoinstead ofgithub.com/owner. Verified for all four hosts, plusgit.sr.ht/~owner/repoand case-insensitiveGitHub.com/.... - Added an unsupported-host gate —
scheme://anduser@host:pathlocators on unrecognized hosts now fail withUnsupportedRepositoryHostError, whose message lists the four supported providers, instead of falling through toUnsupportedRegistryError. - Changed tag pinning to always store a SHA —
resolveDirectRepositoryRefno longer pins slash-free tags to the tag name; every resolved tag, branch, and HEAD now pins the 12-hex commit abbreviation, withtags.test.tsupdated to exactref/versionpairs. - Narrowed
ls-remote— the command now passesHEAD refs/heads/* refs/tags/*sorefs/pull/*andrefs/merge-requests/*are no longer transferred. Peeledrefs/tags/v1^{}lines still arrive, so annotated tags still resolve to the commit. - Covered the direct-repository add path — the new
add.test.tscase drivesaddPackageReferencewithgithub:owner/repoand asserts the exact lockfile entry,tracking: "manual", and thepackages/github/owner/repo/<sha12>reference path. - Deduplicated the provider tables —
normalize.tsnow importsSUPPORTED_REPOSITORY_PROVIDERSandREPOSITORY_PROVIDER_HOSTSfromcore/packages.tsrather than redeclaring them. - Reverted part of the unrelated refactor —
src/lib/shared/path.tsis restored,remove.tsis back onOption/Array.matchwith a namedReferenceSelection, andPackrefHomeValueis re-extracted. The four deletedworkspace/paths.tshelpers remain inlined. - Cleared prior nits —
scpMatchis now null-checked so the${owner}/${repository}fallback is reachable,checkIsRepositorySpecno longer computes aslice(0, -1)prefix when there is no colon, andowner/repo@now parses as a repository spec.
Two things I re-verified so nobody redoes them: no npm spec regressed (react@, @effect/cli@0.29.0, npm: react, npm:@effect/cli@0.29.0, lodash.get@4.4.2 all still parse as registry specs; jsr: / pypi: still raise UnsupportedRegistryError), and bun test --parallel is 297/297 with tsc --noEmit and bun run check clean.
ℹ️ Nitpicks
src/lib/sources/repository/normalize.ts:164-168— pinning tags to a SHA means the requested tag is no longer recoverable from the entry:packref add owner/repo@v1.0.0writesversion: "333333333333"and nothing recordsv1.0.0, sopackref listshows an opaque SHA and the PR description's "Tags and lightweight refs resolve to the ref name itself" is now stale. Also, an explicitly SHA-shaped specifier is still stored verbatim at whatever width the user typed (:179), so@<40 hex>and the 12-hex form of the same commit occupy two different store directories.
Claude Opus ｜ 𝕏
1eab264 to
870b637
Compare
There was a problem hiding this comment.
Important
Every prior finding is genuinely fixed and the new rejection gates are well tested, but three things landed in this commit that should be addressed before merge: the CI format job still fails (new file), bare SourceHut shorthand with a directory builds the wrong repository URL, and the new requestedRef field is stored in the shared store metadata, so it leaks across refs.
Reviewed changes — the delta 6d20d8e..870b637 (15 files), read against the full 30-file PR diff, with parsePackageSpec + normalizeRepositorySource executed directly on the new locator forms and the repo's own test / typecheck / format commands run locally.
- Rewrote
parseRepositoryIdentity— now anEffect.fndispatching to five pure builders (identityFromShorthand,identityFromStandardUrl,identityFromScpUrl,identityFromKnownHostPath,identityFromBarePath) over sharedsplitLocatorPath/formatRepositoryNamehelpers, replacing the single interleaved function. - Added three rejection gates — unsupported host-shaped shorthand (
git.mycompany.com/owner/repo,bitbucket.org.evil.com/owner/repo) now raisesUnsupportedRepositoryHostError; a locator with no repository name (https://github.com/owner,github:owner,sourcehut:~owner) raisesInvalidPackageIdentity; host-qualified GitLab locators with a third path segment are rejected as nested subgroups. All three have exact-value tests. - Added bare SourceHut shorthand —
~owner/repo(no prefix, no host) now routes to sourcehut instead of silently resolving tohttps://github.com/owner/repo. - Changed pinning to the full 40-hex SHA —
PINNED_SHA_LENGTHis gone, so HEAD, tag, and branch resolutions all store the complete SHA;tags.test.ts,add.test.ts, andinstall.test.tswere updated to the wider values. - Recorded the requested ref —
RepositorySourcegains an optionalrequestedRef, andfindPackageEntriesmatchesspec.specifieragainstentry.versionorentry.source.requestedRef, sopackref remove owner/repo@v1.0.0finds a SHA-pinned entry. - Fixed store traversal for two-segment names —
listStoreEntriestreats a segment as nested when it starts with@or the registry is a repository provider, sopruneno longer misreadsowner/repo/<version>. Covered by a newstore.test.tscase. - Rejected a second directory for the same identity —
addDirectRepositoryReferenceToProjectraisesStoreSourceMismatchErrorwhen the reused store entry'sdirectorydiffers, and both add tails were extracted intoaddMaterializedReferenceToProject. - Tightened the install path — a direct repository entry whose source has no
fetchSourcenow raisesUnsupportedRepositoryHostErrorinstead ofSnapshotFetchError.
I re-ran the classification check so nobody redoes it: react, react@, react@19.0.0, @effect/cli@0.29.0, npm:react, npm: react, npm:@effect/cli@0.29.0, lodash.get@4.4.2 all still parse as registry specs, and jsr: / pypi: still raise UnsupportedRegistryError. bun test is 311/311 (up from 297) and bunx tsc --noEmit is clean.
ℹ️ Nitpicks
README.md:83-91lists four spec forms, but this commit added two more that only appear in the PR description: host-prefixed shorthand (github.com/owner/repo,git.sr.ht/~owner/repo) and bare SourceHut shorthand (~owner/repository). Worth adding so the newly supported inputs are discoverable.- An explicitly SHA-shaped specifier is still stored verbatim at whatever width the user typed (
normalize.ts:171-174), while every resolved path now stores 40 hex, so@abcdef1and the full SHA of that same commit occupy two different store directories.
Claude Opus ｜ 𝕏
870b637 to
d08356b
Compare
|
@pullfrog review |
|
Run failed. View the logs →
|
|
@pullfrog review |
There was a problem hiding this comment.
Important
Two subdirectories of the same repository at the same commit share one PackageIdentity, so adding the second one fails with a StoreSourceMismatchError that poisons the entry machine-wide — and the error text points the user at a remediation that reproduces it.
Reviewed changes
This PR teaches packref to accept repository locators directly (github:owner/repo@ref, owner/repo, full clone URLs, SCP-like URLs) alongside registry specs, resolving refs through git ls-remote and snapshotting via giget.
- Spec parsing became a discriminated union —
parsePackageSpecnow returnsRegistryPackageSpec | RepositoryPackageSpec, with repository locators detected ahead of the registry path via provider shorthands,owner/reposhape, and URL shape. - Four providers are recognized —
SUPPORTED_REPOSITORY_PROVIDERScoversbitbucket,github,gitlab, andsourcehut, mapped to hosts and kept deliberately separate fromSUPPORTED_REGISTRIES. - Ref resolution widened beyond tags —
git ls-remotenow requestsHEAD refs/heads/* refs/tags/*and the newparseGitRemoteRefsOutputreturns{head, heads, tags}, so bare locators pinHEADand requested refs match tags, branches, or a literal 40-hex SHA. requestedRefis recorded lockfile-only —fetchRepositorySnapshotstrips it from store metadata, sopackref remove owner/repo@v1.0.0still resolves against a SHA-pinned entry while the store stays content-addressed.- Store traversal understands provider registries —
listStoreEntriestreats provider-named registries like scoped names soowner/reporound-trips throughpackref list. src/lib/workspace/paths.tsshrank to constants — its four path helpers were deleted and inlined at their call sites.
⚠️ Two subdirectories of the same repository at one commit cannot coexist
PackageIdentity is {name, registry, version} and has no directory component, so github:owner/repo/packages/a and github:owner/repo/packages/b at the same commit resolve to the identical identity and therefore the identical store path. The store entry is written once (materializeStoreEntry is first-writer-wins), so its metadata permanently records whichever directory was added first; the new guard in src/lib/references/add.ts then rejects the second subdirectory with StoreSourceMismatchError. Because the global store is shared, the first add poisons that repo+commit for every project on the machine.
Technical details
# Subdirectory is part of the source but not part of the identity
## Affected sites
- `src/lib/core/packages.ts` — `getPackageIdentitySegments` keys store paths on `{name, registry, version}` only; `parseRepositoryIdentity` folds the locator's subdirectory into `source.directory`, not into `name`.
- `src/lib/sources/repository/fetch.ts` — the stored source strips `requestedRef` but **keeps** `directory`, so `directory` is durable store metadata.
- `src/lib/store/index.ts` — `materializeStoreEntry` returns `{reused: true, source: storedEntry.source}` when the path exists and never rewrites metadata.
- `src/lib/references/add.ts` — the new guard fails when `storeEntry.source.directory !== resolved.repository.source.directory`.
- `src/lib/workspace/project.ts` — `createProjectReference` resolves `directory` against `storePath` at reflink time, confirming the store holds the whole repository and `directory` is only a per-reference subpath selector.
- `docs/plans/11-direct-repository-sources.md` (lines 64-66) — states subdirectory locators are supposed to work.
## Required outcome
Two references to different subdirectories of the same repository at the same commit must both succeed. Since the snapshot on disk is the full repository and `directory` is applied only when reflinking into `.packref/`, the mismatch is not a real conflict.
## Suggested approach
Either drop `directory` from the store metadata entirely (it is a reference-time concern, and `packageSnapshotSourceEquivalence` in `src/lib/references/install.ts` would need the same treatment), or include the subdirectory in the identity so the two references get distinct store paths.
## Open questions for the human
`src/lib/core/errors.ts` (~line 262) tells the user the entry "does not match the source recorded in packref-lock.json" and to run `packref clean --global` and retry. On the add path nothing is in the lockfile yet, and after a clean the same first-writer-wins sequence reproduces the failure. Should this message be rewritten regardless of how the identity question is resolved?
⚠️ No test exercises the giget fetch contract this PR now depends on
Every new test stubs git ls-remote output and the RepositoryDownloader, so the suite proves the parsing and routing logic but never that giget can actually fetch the refs this PR chooses to pin. The plan's own step 7 (a real-network integration test) and its note to verify that giget/codeload accepts full commit SHAs are both still outstanding.
Technical details
# The provider archive contract is unverified
## Affected sites
- `src/lib/sources/repository/normalize.ts` — `COMMIT_SHA_PATTERN` accepts any 40-hex string as a pinnable ref for all four providers.
- `src/lib/sources/repository/fetch.ts` — passes `${fetchSource}#${ref}` straight to giget's `downloadTemplate`.
- `docs/plans/11-direct-repository-sources.md` — step 7 requires a real-network integration test; the plan also flags verifying SHA acceptance.
## Required outcome
At least one test (or a documented manual verification) that a full commit SHA resolves to a real archive for each supported provider, so a SHA pin does not fail only in the field.
## Open questions for the human
giget builds sourcehut archives as `https://git.sr.ht/~${repo}/archive/${ref}.tar.gz`. GitHub and Bitbucket document arbitrary-commit archives and GitLab very likely supports them; sourcehut's behaviour for a non-tip SHA is the one I could not confirm. Is sourcehut SHA pinning something you have exercised by hand, or should it be restricted to tags and branches until it is?
ℹ️ The plan doc no longer describes the implemented pinning rules
docs/plans/11-direct-repository-sources.md ships in this PR still marked "Not started" (line 11), and its pinning section describes behaviour the code does not implement: lines 55-56 say bare locators pin the abbreviated 12-hex SHA (the code pins the full 40-hex SHA) and lines 57-58 say tags are pinned verbatim (the code resolves the tag to its SHA).
Technical details
# Plan and implementation disagree on what gets pinned
## Affected sites
- `docs/plans/11-direct-repository-sources.md` line 11 — status still "Not started".
- `docs/plans/11-direct-repository-sources.md` lines 55-58 — abbreviated-SHA and verbatim-tag pinning.
- `src/lib/sources/repository/normalize.ts` — `resolveDirectRepositoryRef` sets `version` to the full resolved SHA in every branch.
## Required outcome
The plan either reflects the shipped pinning rules and its real status, or records explicitly that the rules changed and why (full SHAs are unambiguous and immutable, which reads like the better choice).
ℹ️ Nitpicks
src/lib/references/__tests__/add.test.ts(~249-251) andsrc/lib/sources/repository/__tests__/tags.test.ts(~330-334): the new.rejectsassertions are neitherawaited nor returned, so a broken expectation would surface as an unhandled rejection rather than a failing test. This matches existing house style at ~8 other sites, so it is only worth fixing if you want to start unwinding the pattern.src/lib/core/__tests__/packages.test.ts(~338-348):it("builds direct repository identity segments")sits insidedescribe("parsePackageSpec")but tests identity segmentation, and its fixture version"abc123def456"is a 12-hex string that the implementation would never produce now that refs pin to full SHAs.src/lib/workspace/paths.ts: deleting the four path helpers is unrelated to direct repository sources and spreadspath.join(projectPath, PACKREF_DIRECTORY_NAME)across 6+ files. Not wrong, just extra diff to review alongside a feature.src/lib/sources/repository/normalize.ts: theMatch.valuechain inresolveDirectRepositoryRefneeds six?? ""fallbacks becauseMatch.whenpredicates do not narrow the matched value. A plain if/else ladder over the same four cases would narrow naturally and drop all six.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Important
Adding two subdirectories of the same repository at the same commit dead-ends with a StoreSourceMismatchError whose prescribed remedy cannot resolve it. That is the primary monorepo use case for the [/directory] form this PR documents.
Reviewed changes — full initial review of the direct repository package spec feature across all 30 files.
ParsedPackageSpecbecomes a discriminated union —RegistryPackageSpec(_tag: "registry") andRepositoryPackageSpec(_tag: "repository"); registry resolution, lockfile lookup, and add/remove all branch on the tag.- Repository locator parsing —
checkIsRepositorySpecplus five identity constructors insrc/lib/core/packages.tscover provider shorthand, host-prefixed shorthand, bareowner/repo, SourceHut~owner, standard/git+URLs, and SCP-style SSH, each with optional subdirectory and@ref. - Ref resolution and pinning —
resolveDirectRepositoryRefreadsHEAD,refs/heads/*, andrefs/tags/*via a newRemoteTagReader.listRefs, resolving tags (including peeled annotated tags), branches, and full 40-hex SHAs to a commit SHA that becomes the pinnedversion. - Requested ref tracking —
RepositorySourcegains an optionalrequestedRef, written only into the project lockfile (fetchRepositorySnapshotstrips it from store metadata), sopackref remove owner/repo@v1.0.0still resolves a SHA-pinned entry. - Install path — direct repository entries skip tag re-resolution and fetch
entry.versionverbatim;packageSnapshotSourceEquivalencecompares repository sources field-by-field so the lockfile-onlyrequestedRefdoes not trip the store mismatch check. - Store layout —
getPackageIdentitySegmentsandlistStoreEntriesboth handle two-segmentowner/reponames, keeping repository entries visible tolistandprune. - Unrelated refactor —
src/lib/workspace/paths.tsloses its four path helpers, inlined aspath.join(...)across ten call sites.
I ran bunx tsc --noEmit (clean) and bun test (314 pass, 0 fail) on the head commit. I also verified independently that all four providers' archive endpoints giget builds accept a full 40-hex commit SHA, so the change from tag-refs to SHA-refs on reinstall is safe, and that every spec form the README advertises routes to the repository branch correctly.
ℹ️ Nitpicks
- The
src/lib/workspace/paths.tshelper removal and its ten inlinedpath.join(...)call sites are unrelated to direct repository sources. Bundling them here makes the feature harder to revert on its own.
Claude Opus ｜ 𝕏
d08356b to
3739330
Compare
|
@pullfrog review |
|
Your Claude subscription has hit its usage limit. It resets at 5:40pm (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
|
Your Claude subscription has hit its usage limit. It resets at 5:40pm (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at d08356b:
- Separated snapshot and project directory metadata — Removed the selected repository directory from shared store metadata while retaining it in each project lockfile, so different projects can reuse one full-repository source snapshot with different directory selections.
- Added a clear same-project conflict — Added
RepositoryDirectoryConflictErrorbefore snapshot fetching when one project requests a second directory for the same package identity, with a remedy that resolves the conflict. - Preserved reinstall equivalence — Updated direct-repository snapshot fetching and source comparison to ignore project-only
directoryandrequestedReffields while still validating repository host and URL. - Closed parser and guidance gaps — Kept npm aliases on the registry parser path, removed the unused
hostQualifiedfield, and documented the full 40-character commit SHA requirement in generated agent guidance. - Restored shared path helpers — Reintroduced the project and global path helpers and replaced the unrelated inlined path construction.
- Added regression coverage — Added exact tests for same-project conflicts, cross-project directory reuse, npm aliases, and per-project requested refs.
Verification passed: bun run format --check, bun run check, bunx tsc --noEmit, and 100 affected tests.
GPT Sol ｜ 𝕏
…lpers Plan 11 shipped in #38 but its status still said "Not started"; mark it Complete to match plan 10's convention and point its two code references at the helper names introduced by this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pers (#42) * Consolidate package reference orchestration and shared formatting helpers Remove the addPackageCandidateReference wrapper so the two-phase resolve/materialize API is the single path for candidate adds, delegate the manifest-dependency branch of addPackageReference to it so the manifest range computation has one home, and rename the near-identical add.ts helpers to distinct names (recordStoreEntryReference, fetchAndRecordResolvedPackage, addRepositoryReference). Also extract formatPackageIdentity into core/packages.ts to replace three copied formatIdentity definitions plus two inlined templates, and encode the prune store-safety condition once as canPruneStore, shared by discoverPrunePlan and applyPrunePlan. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Update plan 11 to reflect its completed implementation and renamed helpers Plan 11 shipped in #38 but its status still said "Not started"; mark it Complete to match plan 10's convention and point its two code references at the helper names introduced by this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Packref can now fetch package source directly from repository hosts without going through a registry. Passing a repository spec to
packref addresolves the ref, pins the commit, and records the entry as a manual reference in the lockfile.Supported spec formats:
owner/repository[/directory][@ref]~owner/repository[/directory][@ref]github:owner/repository[/directory][@ref],gitlab:…,bitbucket:…, orsourcehut:…github.com/owner/repository[/directory][@ref],gitlab.com/owner/repository[/directory][@ref],bitbucket.org/owner/repository[/directory][@ref], orgit.sr.ht/~owner/repository[/directory][@ref]https://github.com/owner/repository.git[@ref]orgit+https://…git@github.com:owner/repository.git[@ref]The optional ref can be a tag, branch, or full 40-character commit SHA. Packref stores the full resolved commit SHA so add and reinstall use the same immutable ref. Explicit commit SHAs are normalized to lowercase. The requested tag or branch stays in the project lockfile so removal accepts the original package spec without leaking request metadata into the shared store.
ParsedPackageSpecis a discriminated union ofRegistryPackageSpec(_tag: "registry") andRepositoryPackageSpec(_tag: "repository"). Registry resolution and removal branch on this distinction. Repository ref discovery requests onlyHEAD,refs/heads/*, andrefs/tags/*, which excludes pull-request and merge-request refs. On reinstall, direct repository entries use their pinned version as the exact fetch ref instead of re-resolving through tag matching.The global store supports two-segment repository names such as
owner/repo. Repository URL, host-prefixed, and provider shorthand forms handle monorepo subdirectories consistently. Direct repository snapshots exclude project-specific directory metadata, so different projects can select different directories while they reuse one full-repository snapshot. Within one project, selecting a second directory for the same repository and commit fails before fetching with a dedicated conflict error that tells the user to remove the existing reference first.Summary by CodeRabbit
New Features
Documentation
Bug Fixes