Prepare registry and manifest seams for multi-ecosystem support - #37
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe change reorganizes Effect services into terminal, workspace, manifest, and source modules. It adds typed file errors, bounded retries, concurrent I/O, Option-based results, repository tag services, and multi-manifest dependency reading. ChangesPackref service and adapter foundation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ProjectDependencyReader
participant PackageManagerResolver
participant RemoteTagReader
participant Store
CLI->>ProjectDependencyReader: read detected manifests
ProjectDependencyReader->>PackageManagerResolver: resolve dependency versions
PackageManagerResolver-->>ProjectDependencyReader: Option-based dependency results
CLI->>RemoteTagReader: resolve repository tags
RemoteTagReader-->>CLI: matching tag or network error
CLI->>Store: materialize or update package references
Store-->>CLI: completed operation or typed file error
🚥 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. |
ac0cccd to
99de96d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
src/lib/references/install.ts (1)
72-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd parentheses around the conditional operand of
yield*.
yield* reusedStoreEntry ? readStoreEntry(entry)... : fetchLockedStoreEntry(entry)parses asyield* (reusedStoreEntry ? ... : ...), which is the intended behavior. The precedence is not obvious to a reader and invites an incorrect edit later.♻️ Proposed readability fix
- const storeEntry = yield* reusedStoreEntry - ? readStoreEntry(entry).pipe( - Effect.flatMap((storedEntry) => ensureMatchingSource(entry, storedEntry)) - ) - : fetchLockedStoreEntry(entry) + const storeEntry = yield* (reusedStoreEntry + ? readStoreEntry(entry).pipe( + Effect.flatMap((storedEntry) => ensureMatchingSource(entry, storedEntry)) + ) + : fetchLockedStoreEntry(entry))🤖 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/install.ts` around lines 72 - 77, In the store-entry assignment around reusedStoreEntry, wrap the conditional expression passed to yield* in explicit parentheses while preserving the existing readStoreEntry/ensureMatchingSource and fetchLockedStoreEntry branches.src/lib/sources/tarball/__tests__/fetch.test.ts (1)
153-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a rejection assertion instead of the try/catch sentinel.
If
runresolves, line 155 throws the sentinelErrorinside the sametry, so thecatchblock receives it. The test then fails ontoBeInstanceOf(TarballFetchError)with a message that points at the sentinel rather than at the missing rejection. The same pattern appears at lines 232-245.♻️ Proposed change
- try { - await run(fetchTarballSnapshot(identity, tarballUrl), home, () => Effect.succeed(archive)) - throw new Error("Expected tarball extraction to fail.") - } catch (error) { - expect(error).toBeInstanceOf(TarballFetchError) - - if (error instanceof TarballFetchError) { - expect(error.cause).toBe("Package archive must contain exactly one top-level directory") - } - } + const promise = run( + fetchTarballSnapshot(identity, tarballUrl), + home, + () => Effect.succeed(archive) + ) + + await expect(promise).rejects.toBeInstanceOf(TarballFetchError) + await expect(promise).rejects.toMatchObject({ + cause: "Package archive must contain exactly one top-level directory", + })🤖 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/tarball/__tests__/fetch.test.ts` around lines 153 - 162, Replace the try/catch sentinel pattern around run in the tarball extraction failure tests with a rejection assertion that verifies run rejects with TarballFetchError and the expected cause. Apply the same change to the corresponding test block around lines 232–245, avoiding any catch that could intercept a locally thrown sentinel.src/lib/sources/repository/__tests__/tags.test.ts (1)
103-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
expect(...).rejectsover the try/catch guard.Both tests place
throw new Error("Expected remote tag listing to fail.")inside thetryblock. Thecatchblock then receives that guard error and thetoBeInstanceOfassertion fails, so the test still reports a failure. The behavior is correct, but the intent is clearer with the rejection matcher, and the assertion count stays deterministic.♻️ Proposed refactor for the bounded-retry test
- try { - await runWithRemoteTagCommand(listRemoteTags(), () => { - commandCount += 1 - return Effect.succeed({ - exitCode: 128, - stderr: "fatal: repository not found", - stdout: "", - }) - }) - throw new Error("Expected remote tag listing to fail.") - } catch (error) { - expect(error).toBeInstanceOf(NetworkError) - } + await expect( + runWithRemoteTagCommand(listRemoteTags(), () => { + commandCount += 1 + return Effect.succeed({ + exitCode: 128, + stderr: "fatal: repository not found", + stdout: "", + }) + }) + ).rejects.toBeInstanceOf(NetworkError)Also applies to: 139-156
🤖 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/__tests__/tags.test.ts` around lines 103 - 115, Refactor the rejection assertions in both affected tests to use Jest’s expect(...).rejects matcher instead of a try/catch with a manual guard error. Assert that runWithRemoteTagCommand(listRemoteTags(), ...) rejects with NetworkError while preserving the existing commandCount and mocked Effect.succeed behavior.src/lib/registries/npm/__tests__/resolver.test.ts (1)
4-9: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the
latestbranch ofresolveVersion.The two new tests cover the exact-version and range branches. The
latestbranch insrc/lib/registries/npm/resolver.tslines 11-15 is new behavior: it filters out adist-tags.latestvalue that has no matching entry inmetadata.versions. That filter has no test.💚 Proposed additional tests
it("returns None for a missing version", () => { expect(Option.isNone(resolveVersion(baseMetadata, "20.0.0"))).toBe(true) }) + + it("resolves the latest dist-tag", () => { + expect(Option.getOrThrow(resolveVersion(baseMetadata, "latest"))).toBe("19.0.0") + }) + + it("returns None when the latest dist-tag has no published version", () => { + const metadata = { + ...baseMetadata, + "dist-tags": { latest: "99.0.0" }, + } satisfies NpmPackageMetadata + + expect(Option.isNone(resolveVersion(metadata, "latest"))).toBe(true) + })Also applies to: 68-76
🤖 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/registries/npm/__tests__/resolver.test.ts` around lines 4 - 9, Add a test for the latest branch of resolveVersion, covering metadata where dist-tags.latest does not match any key in metadata.versions and verifying the unmatched latest value is filtered out. Keep the existing exact-version and range tests unchanged.src/lib/sources/repository/tags.ts (1)
113-135: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the
git ls-remotesubprocess.The default layer spawns
git ls-remote --tags <url>with no time bound. If the remote host accepts the connection and then stalls, the effect waits indefinitely, and the retry policy at lines 92-95 never engages. The npm client insrc/lib/registries/npm/client.tsbounds its remote calls through the HTTP client, so this path is the only unbounded remote call in the cohort.♻️ Proposed change
return makeRemoteTagReader((source) => Effect.scoped( Effect.gen(function* () { // ... }) - ) + ).pipe(Effect.timeoutFail({ + duration: REMOTE_TAG_TIMEOUT, + onTimeout: () => new NetworkError({ cause: "git ls-remote timed out", url: source.url }), + })) )Note that
Effect.timeoutFailmust produce an error thelistsignature already declares, soNetworkErroris the correct choice. It also becomes retryable through the existingwhilepredicate.🤖 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` around lines 113 - 135, Apply a bounded timeout to the remote `git ls-remote` effect in the `makeRemoteTagReader` implementation, including subprocess spawning and output collection. Use `Effect.timeoutFail` with the existing timeout configuration and produce a `NetworkError` so the `list` signature remains valid and its existing retry predicate handles timeout failures.src/lib/manifests/index.ts (1)
10-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider generic parameters on
layerWithAdaptersinstead of the derived default types.
DefaultManifestErrorandDefaultManifestRequirementsare derived frommanifestAdapters, which currently holds one adapter. The parameter type therefore pins every injected adapter to the JavaScript adapter's error and requirement types. When a second ecosystem adapter is registered, this constraint blocks adapters that declare narrower requirements.Generic parameters keep the seam open for the multi-ecosystem goal stated in the PR objectives.
♻️ Proposed refactor
- static readonly layerWithAdapters = ( - adapters: ReadonlyArray<ManifestAdapter<DefaultManifestError, DefaultManifestRequirements>> - ) => + static readonly layerWithAdapters = < + E extends DefaultManifestError, + R extends DefaultManifestRequirements, + >( + adapters: ReadonlyArray<ManifestAdapter<E, R>> + ) =>🤖 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/manifests/index.ts` around lines 10 - 27, Update ProjectDependencyReader.layerWithAdapters to introduce generic error and requirements parameters derived from the supplied adapter collection instead of using DefaultManifestError and DefaultManifestRequirements. Ensure each injected ManifestAdapter is accepted according to its own generic types, while preserving the existing service and layer behavior.src/lib/sources/repository/normalize.ts (1)
101-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the
Optionhandling in the two regular-expression capture sites.Both sites build an
Optionof the named-group object, then immediately unwrap it twice withOption.getOrUndefinedand test forundefined. TheOptionadds no safety here, and each unwrap re-reads the same value.Option.matchor a direct nullish check onexecreads more clearly and keeps a single code path.♻️ Proposed refactor for `normalizeFromScpLikeUrl`
const normalizeFromScpLikeUrl = (candidate: RepositorySourceCandidate, rawUrl: string) => { - const groups = Option.fromNullishOr( - /^(?:[^@]+@)?(?<host>[^:]+):(?<repositoryPath>.+)$/u.exec(rawUrl) - ).pipe(Option.flatMap((match) => Option.fromNullishOr(match.groups))) - const host = Option.getOrUndefined(groups)?.host - const repositoryPath = Option.getOrUndefined(groups)?.repositoryPath - - if (host === undefined || repositoryPath === undefined) { - return Effect.fail(invalidRepositoryUrl(candidate, "unsupported repository URL format")) - } - - return makeNormalizedSource(candidate, host.toLowerCase(), repositoryPath) + const groups = /^(?:[^@]+@)?(?<host>[^:]+):(?<repositoryPath>.+)$/u.exec(rawUrl)?.groups + + if (groups?.host === undefined || groups.repositoryPath === undefined) { + return Effect.fail(invalidRepositoryUrl(candidate, "unsupported repository URL format")) + } + + return makeNormalizedSource(candidate, groups.host.toLowerCase(), groups.repositoryPath) }Apply the same shape to the shorthand capture at lines 119-123.
🤖 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 101 - 127, In normalizeFromScpLikeUrl and normalizeRepositorySource, simplify the regex capture handling by avoiding the intermediate Option pipelines and repeated Option.getOrUndefined calls. Use a single direct nullish check or Option.match on each exec result, while preserving the existing invalid-format failure and shorthand-provider branching 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/__tests__/add.test.ts`:
- Around line 575-614: Gate the network-dependent suite named
“addPackageReference with production adapters” behind the project’s explicit
integration-test flag, so both live-service tests are skipped during the default
bun test run and execute only when that flag is enabled. Use the existing test
gating convention or flag symbol used elsewhere in the repository, without
changing the assertions or test behavior.
In `@src/lib/references/add.ts`:
- Around line 81-85: Replace the catch-all domain-error mappings for raw
filesystem PlatformError values with filesystem/workspace-specific errors. In
src/lib/references/add.ts lines 81-85, use that error for realPath and
ensureDirectory; in src/lib/references/prune.ts lines 52-65, use it for
project-directory exists/stat while retaining toLockfileError only for
readProjectLockfile at line 71; in src/lib/store/index.ts line 65, reserve
StoreCorruptedError for metadata read/decode failures; in
src/lib/references/remove.ts lines 66-71 and src/lib/references/sync.ts lines
90-100, add or reuse a removal/operation-specific ReflinkError instead of using
the target as the source.
In `@src/lib/references/install.ts`:
- Around line 95-110: Update the failure handling after the Effect.forEach call
in the install flow to collect all attempts with type "failure" instead of
retaining only attempts.find(...). Keep the first failure as the returned error,
and log or attach every subsequent failure so callers can discover all broken
entries in one run.
In `@src/lib/references/remove.ts`:
- Around line 118-126: Update the removal flow around Effect.forEach and
removePackageEntries so filesystem removals are attempted without aborting or
interrupting remaining references. Collect each removal’s success or failure,
pass only successfully removed entries to removePackageEntries, then propagate
the collected removal failures after the lockfile update while preserving the
existing concurrency and error conversion.
In `@src/lib/registries/__tests__/index.test.ts`:
- Around line 21-24: Update the test’s error handling to assert that the caught
error is an UnsupportedRegistryError before checking its message, ensuring
unexpected error types fail the test rather than skipping assertions. Preserve
the existing expected message assertion for the valid error type.
In `@src/lib/registries/npm/client.ts`:
- Around line 27-32: Add an Effect.timeout to each npm HTTP client attempt in
the HttpClient construction before applying HttpClient.retryTransient, using the
project’s intended request-timeout duration. Keep the existing exponential retry
schedule and retry count unchanged, and do not use HttpClient.withTimeout.
In `@src/lib/sources/tarball/fetch.ts`:
- Around line 91-93: Update the chmod handling in the tarball extraction flow to
parse the archive mode, reject non-integer results such as NaN, and mask the
value to permission bits before passing it to fs.chmod. Preserve the existing
behavior of applying chmod only when entry.attrs.mode is defined, while
preventing setuid, setgid, and sticky bits from being applied.
In `@src/lib/store/index.ts`:
- Around line 99-148: Limit aggregate directory-read concurrency in the registry
traversal instead of applying independent limits at each nested Effect.forEach
level. Introduce one shared semaphore sized by STORE_TRAVERSAL_CONCURRENCY, have
every listDirectoryOrEmpty call in this traversal acquire it (or use
listDirectory through the semaphore), and set the nested Effect.forEach
operations to unbounded so the semaphore is the sole concurrency bound.
---
Nitpick comments:
In `@src/lib/manifests/index.ts`:
- Around line 10-27: Update ProjectDependencyReader.layerWithAdapters to
introduce generic error and requirements parameters derived from the supplied
adapter collection instead of using DefaultManifestError and
DefaultManifestRequirements. Ensure each injected ManifestAdapter is accepted
according to its own generic types, while preserving the existing service and
layer behavior.
In `@src/lib/references/install.ts`:
- Around line 72-77: In the store-entry assignment around reusedStoreEntry, wrap
the conditional expression passed to yield* in explicit parentheses while
preserving the existing readStoreEntry/ensureMatchingSource and
fetchLockedStoreEntry branches.
In `@src/lib/registries/npm/__tests__/resolver.test.ts`:
- Around line 4-9: Add a test for the latest branch of resolveVersion, covering
metadata where dist-tags.latest does not match any key in metadata.versions and
verifying the unmatched latest value is filtered out. Keep the existing
exact-version and range tests unchanged.
In `@src/lib/sources/repository/__tests__/tags.test.ts`:
- Around line 103-115: Refactor the rejection assertions in both affected tests
to use Jest’s expect(...).rejects matcher instead of a try/catch with a manual
guard error. Assert that runWithRemoteTagCommand(listRemoteTags(), ...) rejects
with NetworkError while preserving the existing commandCount and mocked
Effect.succeed behavior.
In `@src/lib/sources/repository/normalize.ts`:
- Around line 101-127: In normalizeFromScpLikeUrl and normalizeRepositorySource,
simplify the regex capture handling by avoiding the intermediate Option
pipelines and repeated Option.getOrUndefined calls. Use a single direct nullish
check or Option.match on each exec result, while preserving the existing
invalid-format failure and shorthand-provider branching behavior.
In `@src/lib/sources/repository/tags.ts`:
- Around line 113-135: Apply a bounded timeout to the remote `git ls-remote`
effect in the `makeRemoteTagReader` implementation, including subprocess
spawning and output collection. Use `Effect.timeoutFail` with the existing
timeout configuration and produce a `NetworkError` so the `list` signature
remains valid and its existing retry predicate handles timeout failures.
In `@src/lib/sources/tarball/__tests__/fetch.test.ts`:
- Around line 153-162: Replace the try/catch sentinel pattern around run in the
tarball extraction failure tests with a rejection assertion that verifies run
rejects with TarballFetchError and the expected cause. Apply the same change to
the corresponding test block around lines 232–245, avoiding any catch that could
intercept a locally thrown sentinel.
🪄 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: 0813309d-5470-48d7-986c-ab69b3291249
📒 Files selected for processing (62)
.changeset/calm-stores-retry.md.changeset/tough-hounds-juggle.md.packref/packref-lock.jsondocs/architecture.mddocs/plans/10-multi-registry-groundwork.mddocs/plans/11-direct-repository-sources.mdoxlint.config.tssrc/commands/add.tssrc/commands/clean.tssrc/commands/init.tssrc/commands/install.tssrc/commands/list.tssrc/commands/prune.tssrc/commands/remove.tssrc/commands/sync.tssrc/index.tssrc/lib/core/errors.tssrc/lib/core/packages.tssrc/lib/manifests/__tests__/index.test.tssrc/lib/manifests/__tests__/javascript.test.tssrc/lib/manifests/index.tssrc/lib/manifests/javascript.tssrc/lib/manifests/manifest.tssrc/lib/references/__tests__/add.integration.test.tssrc/lib/references/__tests__/add.test.tssrc/lib/references/__tests__/install.test.tssrc/lib/references/__tests__/prune.test.tssrc/lib/references/__tests__/sync.test.tssrc/lib/references/add.tssrc/lib/references/install.tssrc/lib/references/prune.tssrc/lib/references/remove.tssrc/lib/references/sync.tssrc/lib/registries/__tests__/index.test.tssrc/lib/registries/npm/__tests__/client.test.tssrc/lib/registries/npm/__tests__/resolver.test.tssrc/lib/registries/npm/client.tssrc/lib/registries/npm/resolver.tssrc/lib/services/__tests__/command-runner.test.tssrc/lib/services/command-runner.tssrc/lib/services/packref-home.tssrc/lib/sources/repository/__tests__/fetch.test.tssrc/lib/sources/repository/__tests__/tags.test.tssrc/lib/sources/repository/fetch.tssrc/lib/sources/repository/normalize.tssrc/lib/sources/repository/tags.tssrc/lib/sources/tarball/__tests__/fetch.test.tssrc/lib/sources/tarball/fetch.tssrc/lib/store/__tests__/store.test.tssrc/lib/store/index.tssrc/lib/store/paths.tssrc/lib/workspace/__tests__/project.test.tssrc/lib/workspace/config.tssrc/lib/workspace/home.tssrc/lib/workspace/integration.tssrc/lib/workspace/lockfile.tssrc/lib/workspace/project.tssrc/lib/workspace/reflinker.tssrc/terminal/__tests__/prompter.test.tssrc/terminal/prompter.tssrc/terminal/title.macro.tssrc/terminal/title.ts
💤 Files with no reviewable changes (4)
- src/lib/services/tests/command-runner.test.ts
- src/lib/services/command-runner.ts
- src/lib/services/packref-home.ts
- src/lib/references/tests/add.integration.test.ts
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The manifest seam this PR is named for does not actually open. layerWithAdapters is typed against the JavaScript adapter's concrete error union, so a second ecosystem adapter is rejected at compile time. Details inline on src/lib/manifests/index.ts.
Reviewed changes
- Read the full diff end-to-end (62 files, 1 commit).
- Verified on disk at
99de96d:bunx tsc --noEmitclean,bun test268 pass / 0 fail. - Confirmed no stale references remain to
#lib/services/*,#lib/store/store.ts, or#lib/shared/title*after the relocations, and that the newcore/errors.ts→core/registry.tsimport is not a cycle. - Empirically probed the new adapter seam with a scratch second-ecosystem adapter to check the extensibility claim.
🧩 The manifest seam is typed against the one adapter it has
This is the headline concern, so restating it outside the inline thread: the PR's stated goal is preparing the manifest system "for projects that use multiple package ecosystems," but DefaultManifestError and DefaultManifestRequirements in src/lib/manifests/index.ts are inferred from manifestAdapters, which contains only javascript. Every adapter passed to layerWithAdapters must therefore fail with exactly ManifestParseError | ManifestResolutionError.
Worth noting the registry side of the same PR gets this right — src/lib/registries/index.ts uses satisfies Record<Registry, RegistryAdapter<unknown, unknown>>, which keeps the adapter types open. The manifest side dropped that satisfies clause. The two seams the PR title pairs together are not equally open.
🏷️ Filesystem errors are relabelled as domain errors whose remedies don't apply
The reliability pass narrows several error channels by mapping PlatformError into domain errors. That's a reasonable goal, but three of the mappings produce user-facing guidance that is wrong for the failure that actually occurred. Each error class has a get message() in src/lib/core/errors.ts that prescribes a specific remedy, so the mislabel is not cosmetic — it is the text the user reads and acts on.
Flagged inline at references/add.ts, store/index.ts, and references/remove.ts. The store/index.ts one is the most user-hostile: a clean that fails on a permissions error tells the user to run clean.
🧪 A concurrency regression test was deleted without replacement
src/lib/services/__tests__/command-runner.test.ts is removed along with CommandRunner. That test drove 256 KiB through the spawner to prove stdout and stderr are drained concurrently — without that, a child process filling one pipe while the reader blocks on the other deadlocks.
The equivalent logic now lives inside RemoteTagReader.layer (Effect.all([...], { concurrency: 3 }) over handle.stdout, handle.stderr, handle.exitCode). But every test in src/lib/sources/repository/__tests__/tags.test.ts injects layerWithCommand with a fake, bypassing that code entirely. The only reference to the real RemoteTagReader.layer is src/lib/references/__tests__/add.test.ts:155, where it sits in a layer stack that never reaches git.
So the deadlock guard is now untested. The logic moving house is fine; the test not moving with it is the gap. Porting the old large-output test onto RemoteTagReader.layer would restore it.
🏠 PackrefHome as a Context.Reference trades a compile error for a silent write to the real $HOME
Moving PackrefHome off the required service set genuinely does simplify test signatures — that part works. The cost is that defaultValue: () => ({ path: homedir() }) makes omission legal.
Previously, a test (or a new code path) that forgot to provide PackrefHome failed typechecking. Now it compiles and resolves to the developer's actual home directory, so a store test missing its PackrefHome.at(...) would create and later fs.remove(..., { recursive: true }) real paths under ~/.packref. All twelve current call sites do provide it, so nothing is broken today; the concern is that the guardrail against the thirteenth is gone, and the failure mode is destructive rather than loud.
If the ergonomics win is worth keeping, a default that points at an obviously-invalid sentinel path outside test/production wiring would preserve most of it while making an unprovided PackrefHome fail fast instead of silently.
ℹ️ Nitpicks
ReflinkError with source === target. In src/lib/references/remove.ts, toReflinkError sets both source and target to referencePath, so an fs.exists failure renders as "Failed to materialize project reference from X to X" — during a removal, which materializes nothing. Same category as the mappings above, just lower impact since the path shown is at least the right one.
The tarball section of the PR description doesn't match the diff. The body says top-level stripping "now handles crate-style (name-version/) and sdist-style (Name-Version/) archive layouts … and rejects archives that contain more than one top-level directory." getArchivePrefix in src/lib/sources/tarball/fetch.ts is unchanged context in this diff — it already derived the prefix from the single observed root and already rejected roots.size !== 1. What the PR adds is test coverage for those layouts, which is a genuine improvement, just not the behavior change described. Worth correcting before this text becomes the changelog entry.
404 coverage was dropped from the tarball tests. In src/lib/sources/tarball/__tests__/fetch.test.ts the existing rejection test was retargeted from 404 to 503 so it could assert requestCount === 3 against the new retryTransient. That's a good test to have, but it replaced rather than joined the 404 case — there's now no assertion that a permanent 404 fails without burning retries, which is the more common failure and the one where retrying is pure latency. Adding a second case pinning requestCount === 1 for 404 would cover both halves of the retry policy.
Schedule.upTo({ times: 2 }) checks out. Verified against node_modules/effect/src/Schedule.ts — valid v4 API, and 2 retries means 3 total attempts, which is what the tarball tests assert. No action needed; noting it so it isn't re-litigated.
Concurrent tar entry writes are safe. TAR_ENTRY_WRITE_CONCURRENCY = 8 looked like an ordering hazard (a file entry landing before its parent directory entry), but each file write does its own fs.makeDirectory(dirname, { recursive: true }) first, so there's no dependency on entry order. Noting it as checked-and-fine.
Claude Opus ｜ 𝕏
fcdeedd to
3dc2c7a
Compare
There was a problem hiding this comment.
Important
All four of my prior inline threads are genuinely fixed. One fix over-corrected: git ls-remote now never retries any failure that surfaces as a non-zero exit code, which includes the transient ones the changeset promises to retry. The test that was supposed to guard the fix cannot fail.
Reviewed changes
manifests/index.ts— the adapter seam is back:layerWithAdapters<E, R>is generic over foreign adapters,makeLayeris private, andindex.test.tsexercises a real foreign adapter (CargoManifestError/CargoManifestEnvironment) rather than asserting on the default one. Resolved.references/add.ts—realPathandensureDirectorymap to the newProjectFilesystemErrorwith anoperation: "access" | "prepare" | "resolve"discriminant. Resolved.sources/repository/tags.ts— the exit-code check moved outsideEffect.retry, sofatal: repository not foundis no longer retried three times. Resolved, but see the inline note: the pendulum swung too far.store/index.ts—cleanStoreraisesGlobalStoreFilesystemError(operation: "clean") instead of mislabelling I/O failures as corruption;StoreCorruptedErroris now reserved for metadata decode. Resolved.- Restored coverage I'd flagged as dropped: the
git ls-remotestdout/stderr deadlock guard is back astags.test.ts"drains large stdout and stderr from the live command concurrently" (real fakegitonPATH, 12k lines to both streams), and the 404 tarball case is back intarball/__tests__/fetch.test.ts. - Integration tests moved to
src/__tests__/integration.test.tsbehindPACKREF_INTEGRATIONwith a dedicatedtest:integrationscript, so the defaultbun testno longer hits the network. - Aggregate error handling in
install.tsandremove.tsnow collects every failure viaEffect.match+Array.partition/Resultinstead of aborting on the first one;prune.tsandsync.tsfan out at concurrency 8;store/index.tsbounds its traversal with aSemaphoreat 16;npm/client.tsgained a 30s per-attempt timeout plus one transient retry;tarball/fetch.tsvalidates archive modes against/^[0-7]+$/uand masks with& 0o777.
Verification
bunx tsc --noEmit is clean. bun test is 274 pass / 1 fail. The one failure — store > checks, lists, and removes scoped and unscoped entries — reproduces identically on origin/main in a clean worktree, so it is not a regression from this PR. Details inline on store/index.ts.
Not re-raised
PackrefHome's Context.Reference default still silently falls back to the real $HOME when no layer is provided. I raised this last round, it's unchanged, and it's a design call rather than a defect — noting it only so it isn't mistaken for something I stopped caring about.
Claude Opus ｜ 𝕏
3dc2c7a to
dfa04b7
Compare
There was a problem hiding this comment.
ℹ️ All three prior findings are genuinely fixed and the suite is fully green. One suggestion inline, one nit below.
Reviewed changes
Delta since my last review (3dc2c7a → dfa04b7), three files.
- Restored bounded retries for transient
git ls-remotefailures. TheexitCode !== 0check moved back inside theEffect.retrypipeline, and thewhilepredicate now classifies onstderrviaTRANSIENT_GIT_FAILURE_PATTERN— permanent failures (fatal: repository not found) still fail on the first attempt, DNS/connection failures get the bounded schedule. Non-string causes (spawnerPlatformError,Effect.timeout) remain retryable, which is right. - Made the retry tests able to fail.
"does not retry a permanent command failure"and"reports an actionable error without retry when the git executable is missing"nowawaitthe rejection before assertingcommandCount, and the counter increments throughEffect.syncinside the command rather than in the factory closure. A new"retries a transient command failure with a bounded policy"pinscommandCount === 3onCould not resolve host. - Sorted
listStoreEntriesoutput.registryEntries.flat().toSorted(Order.mapInput(packageIdentityOrder, …))makes the ordering a contract instead of an accident offs.readDirectory, retiring the previously-red store test.
Verification
bunx tsc --noEmit clean. bun test → 276 pass / 0 fail (was 274 / 1). I re-ran the mutation that defeated the old test: relaxing the while predicate to Predicate.isTagged(error, "NetworkError") alone now produces 13 pass / 1 fail, so the guard is real. tags.ts was restored afterwards; the tree is clean.
ℹ️ Nitpicks
src/lib/sources/repository/__tests__/tags.test.ts:299-300—"skips tag discovery for unsupported repository hosts"still has the unawaited shape the two fixed tests had:expect(resolution).rejects.toBeInstanceOf(...)is not awaited, soexpect(commandWasRun).toBe(false)on the next line races the effect. It passes today becauseresolveRepositoryRefrejects before reaching the command, but the assertion isn't pinning that.awaiting the rejection first would make it match its siblings.
Claude Opus ｜ 𝕏
dfa04b7 to
e3c47f4
Compare
There was a problem hiding this comment.
✅ No new issues found. The one open finding from my last review is fixed, and the fix is guarded by a test that can actually fail.
Reviewed changes
Delta since my last review (dfa04b7 → e3c47f4), two files, nine lines.
- Pinned the
git ls-remotechild locale.src/lib/sources/repository/tags.ts:122-125now spawns with{ env: { LC_ALL: "C" }, extendEnv: true }, soTRANSIENT_GIT_FAILURE_PATTERNclassifies untranslatedstderrregardless of the invoking user's locale.extendEnv: truemerges overglobalThis.process.envwithenvwinning, soPATHand the rest of the environment survive — which the live-command test depends on to find its fakegit.LC_ALL=Calso suppresses GNU gettext'sLANGUAGEpriority list, so there's no residual leak through that variable. - Guarded it in the live-command test. The fake
gitinsrc/lib/sources/repository/__tests__/tags.test.ts:177-180exits1with a diagnostic whenLC_ALLis notC. That message doesn't match the transient pattern, so it fails on the first attempt rather than burning retries, and the test'sexpect(tags).toEqual(["v19.0.0"])never sees a value.
Verification
bunx tsc --noEmit clean. bun test → 276 pass / 0 fail, unchanged from the prior head. I mutation-tested the new guard: reverting tags.ts to the bare ChildProcess.make("git", ["ls-remote", "--tags", source.url]) produces 13 pass / 1 fail in tags.test.ts, so the assertion is load-bearing rather than incidental. tags.ts was restored afterwards and the tree is clean.
Claude Opus ｜ 𝕏
1 participant
This PR restructures the module boundaries to prepare the registry and manifest systems for projects that use multiple package ecosystems.
Registry and manifest seams
ProjectDependencyReaderreplaces the previousgetManifestAdapterandreadProjectDependenciesfree functions with a service that iterates all registered manifest adapters and merges their results. Adapters that do not detect a matching manifest are skipped; the service returnsOption.noneonly when no adapter detects any manifest at all, distinguishing that case from a detected but empty manifest.layerWithAdaptersallows tests and future callers to supply an explicit adapter list without touching the default registration. Thegroupfield onManifestDependencyis widened from a fixed union tostringso adapters outside the JavaScript ecosystem can declare their own dependency group names.RemoteTagReaderserviceThe
CommandRunnerservice is removed. Its only consumer wasgit ls-remotein the repository tag resolution path. That logic is now encapsulated inRemoteTagReader, which exposes alistmethod and ships its ownlayer(backed byChildProcessSpawner) andlayerWithCommandfor test injection. Tests that previously constructed aCommandRunnerlayer now useRemoteTagReader.layerWithCommand.PackrefHomeconverted toContext.ReferencePackrefHomemoves fromsrc/lib/services/tosrc/lib/workspace/home.tsand is implemented as aContext.Referencewith a default value ofhomedir(). This removes it from the required service set for store and workspace operations, eliminating the need to thread it through test effect type signatures.Module relocations
Reflinkermoves fromsrc/lib/services/tosrc/lib/workspace/.Prompterand the title utilities move fromsrc/lib/services/andsrc/lib/shared/tosrc/terminal/.store.tsis renamed tosrc/lib/store/index.ts.addPackageReferenceis consolidated intoadd.test.tsunder awith production adaptersdescribe block.Tarball extraction
Tarball extraction tests now cover crate-style (
name-version/) and sdist-style (Name-Version/) archive layouts, in addition to the existingpackage/convention, and verify that archives with more than one top-level directory are rejected.Summary by CodeRabbit