Update Adamantite and satisfy anti-slop checks - #45
Conversation
There was a problem hiding this comment.
ℹ️ No behavior changes found — the refactor is faithful. A few rough edges around the type-safety of the new idioms.
Reviewed changes — the adamantite 0.34.4 → 0.36.0 bump and the mechanical refactor that satisfies its new antislop ruleset, across all 17 files.
- Enable
antislop+ignorePatterns—oxlint.config.tsextends the newadamantite/lint/antisloppreset and adopts the package's shared ignore list. - Conditional object spreads → conditional assignment — the dominant change:
...(x === undefined ? {} : { key: x })becomes a base object plus anif-guarded write, incore/packages.ts,references/add.ts,references/remove.ts,sources/repository/normalize.ts,sources/repository/fetch.ts, and three test helpers. typeofchecks →effect/Match—registries/npm/resolver.tsnormalizes the npmrepositoryfield andterminal/prompter.tsresolves its success message viaMatchinstead oftypeof x === "function".- Widened dictionaries →
satisfies—REPOSITORY_PROVIDER_HOSTSandpackageManagerLockfileStrategiesdrop theirRecordannotations, and the latter gains an explicitgetPackageManagerLockfileStrategylookup. - Untyped rejection handlers →
try/catchandEffect.flip—add.test.tsandtags.test.ts; the tarball test'sstatusparam narrows to() => number. - CI runtime discovery —
adamantite.ymlreads Node from.node-versionand drops itsbun-versionpin.
I re-ran validation on the PR tree: bun run typecheck (tsc) is clean and bun test is 316 pass / 0 fail. Note the PR body lists bun run check, which is oxlint-only and never type-checks — typecheck is the one that matters here, and it passes.
I also audited the spread rewrite site by site for the failure mode it invites, namely a key flipping from absent to present-with-undefined. It does not happen anywhere; every guard is the identical !== undefined test. The two most suspicious sites are both provably safe: resolveDirectRepositoryRef's { ...source } can never arrive already carrying a requestedRef (the repository field has no such property), and fetch.ts's includeDirectory !== false && directory !== undefined guard is byte-identical to the one it replaced. Insertion order does shift in add.ts and normalize.ts, but every persistence path goes through Schema.encodeEffect and errors are field-accessed, so it is unobservable.
Worth calling out as a real improvement: swapping await listing.catch((e: unknown) => e) for Effect.flip in tags.test.ts makes those assertions strictly stronger. The old form would have quietly fed a success value into expect(error).toBeInstanceOf(NetworkError) if the effect ever stopped failing; Effect.flip fails the test instead.
ℹ️ Builder types are hand-duplicated instead of derived, so they can drift from the types they mirror
Seven new local interfaces mirror an existing canonical type by hand: RegistryPackageSpecBuilder, RepositoryBuilder, RepositoryPackageSpecBuilder (core/packages.ts), ProjectRepositorySource, RepositoryDirectoryConflict (references/add.ts), PackageReferenceIdentity (references/remove.ts), and TestRepositorySource (install.test.ts). Interestingly this PR already contains the better pattern — add.test.ts derives VersionMetadata from NpmPackageMetadata with a -readonly mapped type, so it cannot drift. The hand-written ones can.
The practical consequence is modest and I want to be precise about it: adding a required field to a canonical type still errors, just at the call site rather than at the definition. Removing or renaming one goes unenforced. Since a mutable-mapped-type alias is roughly the same number of characters, it seems worth converging on the one you already wrote.
Technical details
# Derive builder types from their canonical types
## Affected sites
- `src/lib/core/packages.ts:72-90` — `RegistryPackageSpecBuilder` / `RepositoryBuilder` /
`RepositoryPackageSpecBuilder` duplicate `RegistryPackageSpec` / `RepositoryPackageSpec["repository"]` /
`RepositoryPackageSpec` (declared just above at lines 44-68).
- `src/lib/references/add.ts:69-83` — `RepositoryDirectoryConflict` duplicates the
`RepositoryDirectoryConflictError` payload; `ProjectRepositorySource` duplicates `RepositorySource`
(Schema-derived, `src/lib/core/source.ts:16`).
- `src/lib/references/remove.ts:29-33` — `PackageReferenceIdentity` duplicates the
`PackageNotReferencedError` payload.
- `src/lib/references/__tests__/install.test.ts:24-30` — `TestRepositorySource` duplicates `RepositorySource`.
## Required outcome
- Each builder interface stays automatically in sync with the type it mirrors, so a field added to,
removed from, or retyped on the canonical type is reflected without a manual edit.
- `parsePackageSpec` remains checked against the exported `ParsedPackageSpec` at its own definition
rather than only at its four call sites.
## Suggested approach (optional)
Reuse the mapped-type idiom this PR already introduces at `src/lib/references/__tests__/add.test.ts:44-48`:
```ts
type Mutable<T> = { -readonly [K in keyof T]: T[K] }
type RepositoryPackageSpecBuilder = Mutable<RepositoryPackageSpec>
type RepositoryBuilder = Mutable<RepositoryPackageSpec["repository"]>
type ProjectRepositorySource = Mutable<RepositorySource>
```
For `parsePackageSpec`, a `satisfies` on each return statement restores the link to the canonical type
without reintroducing a conditional spread (`satisfies` is not what the new lint rules object to — this PR
adds two of them elsewhere):
```ts
return packageSpec satisfies RegistryPackageSpec
```
## Open questions for the human
- Are the `*Builder` types meant to be a lasting seam, or purely a workaround for the anti-slop rules?
If the latter, a single shared `Mutable<T>` helper is probably the whole fix.ℹ️ Nitpicks
src/lib/references/add.ts:7— removing theRepositorySourcetype import left a stray blank line splitting theeffect/*imports from the#lib/*ones. Harmless, but the formatter won't collapse it for you.src/lib/manifests/javascript.ts:304—Match.orElse(() => void 0)reads a little obliquely;() => undefinedsays the same thing.
Claude Opus | 𝕏
58e2f08 to
138bce8
Compare
There was a problem hiding this comment.
ℹ️ The prior review's main finding is properly addressed, and I confirmed it enforces rather than just reads better. No new correctness issues — two minor points.
Reviewed changes — the delta since the prior pullfrog review at 58e2f08. Note the branch was force-pushed to a single amended commit and main also advanced, so the version bumps now read 0.35.0 → 0.37.0 and 1.78.0 → 1.79.0 rather than the 0.34.4 → 0.36.0 the first review described. That is merge-base movement, not a change of intent.
- Derived the builder types instead of hand-writing them — a new
src/lib/core/types.tsexportsMutable<Value>, and all eight builder aliases now derive from it, with the two error payloads going throughMutable<ConstructorParameters<typeof SomeError>[0]>. - Restored the canonical check at the definition site — all three
parsePackageSpecreturn statements gainedsatisfies RegistryPackageSpec/satisfies RepositoryPackageSpec. - Corrected the Zed
lsp.oxfmtsettings — dropped the oxlint-namespaced keys (bareconfigPath,typeAware,unusedDisableDirectives) and the obsoletefmt.experimental, leavingfmt.configPathandrun. - Documented the two
as Acasts insrc/commands/__tests__/init.test.tswith// SAFETY:comments.
I re-ran validation on this tree: bun run check is 0 warnings / 0 errors across 424 rules, bun run typecheck (tsc) is clean, bun run analyze is clean, and bun test is 316 pass / 0 fail.
More usefully, I probed that the new derivations actually enforce rather than merely deduplicate. On this branch a bogus key in the object literal (TS2353), a wrong-typed value on assignment (TS2322), and a typo'd key on assignment (TS2551) are each caught at the builder sites. Worth recording why that was not a foregone conclusion: Effect v3's Data.TaggedError omitted _tag | keyof Error from its constructor argument, which would have silently dropped the name field from both ConstructorParameters<...>[0] derivations. Under effect@4.0.0-rc.109 it resolves to the full payload, so PackageReferenceIdentity and RepositoryDirectoryConflict are faithful.
I also checked the .zed change against primary sources rather than assuming scope creep — the official oxc-project/oxc-zed examples show exactly {"fmt.configPath": null, "run": "onSave"} key-for-key, and fmt.experimental appears in no current oxc documentation. It is a real correction of a copy-paste from the sibling oxlint block, not an unrelated edit.
ℹ️ adamantite.yml is now the only workflow running an unpinned Bun
Dropping bun-version: 1.3.14 does not make the job inherit a pin — oven-sh/setup-bun with no input resolves to the latest Bun release. build.yml, test.yml, and release.yml all still pin 1.3.14, so from this commit onward the four workflows type-check and lint the same tree under two different Bun builds, and the split widens on its own every time Bun ships. The failure mode is a lint or install break appearing only in the adamantite job, on a commit that did not touch anything related.
The node-version half of the change is fine — .node-version contains 24, which is what the other three hardcode — but it leaves the same asymmetry: a future .node-version bump would move one workflow and not the other three.
Technical details
# Align the Bun and Node toolchain across all four workflows
## Affected sites
- `.github/workflows/adamantite.yml:43` — `oven-sh/setup-bun@v2` with no `with:` block, so no `bun-version`.
- `.github/workflows/adamantite.yml:40` — reads `node-version-file: ".node-version"`.
- `.github/workflows/build.yml:37-39`, `.github/workflows/test.yml:37-39`, `.github/workflows/release.yml:40-42` —
all still pin `bun-version: 1.3.14` and hardcode `node-version: "24"`.
## Required outcome
- Every workflow that installs Bun resolves the same Bun version for a given commit, so a CI failure is
attributable to the commit rather than to which job happened to run first after a Bun release.
- Node and Bun versions are declared in one place, so a future bump cannot move a subset of the workflows.
## Suggested approach (optional)
Either restore `bun-version: 1.3.14` in `adamantite.yml`, or — better, since it also fixes the Node asymmetry —
add a `.bun-version` file at the repo root and switch all four workflows to the file-based form:
```yaml
- name: Setup Node.js
uses: actions/setup-node@v7
with:
node-version-file: ".node-version"
- name: Setup Bun
uses: oven-sh/setup-bun@v2
with:
bun-version-file: ".bun-version"
```
Note `@types/bun` is pinned to `1.3.14` in `devDependencies`, so `1.3.14` is the version the type definitions
already assume.
## Open questions for the human
- Was un-pinning Bun here deliberate — i.e. is `adamantite.yml` meant to be a canary that catches Bun
regressions early? If so it is worth a comment in the workflow saying so, because it currently reads as an
oversight and the next person will "fix" it back.Claude Opus | 𝕏
138bce8 to
064bbb7
Compare
There was a problem hiding this comment.
ℹ️ The Bun/Node toolchain asymmetry from the last review is fully resolved. One minor point on where the pin now lives.
Reviewed changes — the delta since the prior pullfrog review at 138bce8, which is entirely CI toolchain configuration. No file under src changed, so the refactor findings from the previous two runs are unaffected.
- Added a
.bun-versionfile — pins Bun at1.3.14, matching the@types/bundevDependency. - Restored the Bun pin on
adamantite.yml— the workflow regained awith:block readingbun-version-file: ".bun-version", so it no longer resolves to whatever Bun released most recently. - Moved the other three workflows onto the same files —
build.yml,test.yml, andrelease.ymlswappednode-version: "24"fornode-version-file: ".node-version"andbun-version: 1.3.14forbun-version-file: ".bun-version".
This is the right shape of fix and it closes the concern cleanly: all four workflows now resolve both runtimes from the same two files, so a future bump cannot move a subset of them. I verified bun-version-file is a genuine oven-sh/setup-bun@v2 input rather than a silently-ignored key — it is declared in the action's action.yml and bun-version-file: ".bun-version" is the README's own example, so this does not quietly fall back to installing latest. Both version files are well-formed (24 and 1.3.14, each newline-terminated).
Worth noting package.json's engines.node is ">=22" while CI now runs 24 from .node-version. That is a floor-versus-pin distinction rather than drift, so I am not flagging it.
Claude Opus | 𝕏

Adamantite 0.37.0 enables stricter anti-slop checks that the existing code did not satisfy. This prevented
bun run checkfrom completing after the tooling update.This change updates the managed lint configuration and CI runtime discovery. It replaces conditional object spreads, runtime representation checks, widened dictionaries, and untyped rejection handlers with explicit typed constructions. Shared mutable builder types now derive from their canonical types. Package source reference behavior does not change.
Validation
bun install --frozen-lockfilebun run format --checkbun run checkbun run analyzebun run typecheckbun test, 316 passed and 0 failedbun run buildadamantite doctor, no issues foundChanges made with GPT-5.6 Sol through T3 Code.