Skip to content

Update Adamantite and satisfy anti-slop checks - #45

Open
adelrodriguez wants to merge 1 commit into
mainfrom
maintenance/upgrade-adamantite
Open

Update Adamantite and satisfy anti-slop checks#45
adelrodriguez wants to merge 1 commit into
mainfrom
maintenance/upgrade-adamantite

Conversation

@adelrodriguez

@adelrodriguez adelrodriguez commented Aug 18, 2026

Copy link
Copy Markdown
Owner

Adamantite 0.37.0 enables stricter anti-slop checks that the existing code did not satisfy. This prevented bun run check from 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-lockfile
  • bun run format --check
  • bun run check
  • bun run analyze
  • bun run typecheck
  • bun test, 316 passed and 0 failed
  • bun run build
  • adamantite doctor, no issues found

Changes made with GPT-5.6 Sol through T3 Code.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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 + ignorePatternsoxlint.config.ts extends the new adamantite/lint/antislop preset 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 an if-guarded write, in core/packages.ts, references/add.ts, references/remove.ts, sources/repository/normalize.ts, sources/repository/fetch.ts, and three test helpers.
  • typeof checks → effect/Matchregistries/npm/resolver.ts normalizes the npm repository field and terminal/prompter.ts resolves its success message via Match instead of typeof x === "function".
  • Widened dictionaries → satisfiesREPOSITORY_PROVIDER_HOSTS and packageManagerLockfileStrategies drop their Record annotations, and the latter gains an explicit getPackageManagerLockfileStrategy lookup.
  • Untyped rejection handlers → try/catch and Effect.flipadd.test.ts and tags.test.ts; the tarball test's status param narrows to () => number.
  • CI runtime discoveryadamantite.yml reads Node from .node-version and drops its bun-version pin.

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 the RepositorySource type import left a stray blank line splitting the effect/* imports from the #lib/* ones. Harmless, but the formatter won't collapse it for you.
  • src/lib/manifests/javascript.ts:304Match.orElse(() => void 0) reads a little obliquely; () => undefined says the same thing.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread src/lib/sources/repository/normalize.ts
@adelrodriguez
adelrodriguez force-pushed the maintenance/upgrade-adamantite branch from 58e2f08 to 138bce8 Compare August 23, 2026 05:17

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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.ts exports Mutable<Value>, and all eight builder aliases now derive from it, with the two error payloads going through Mutable<ConstructorParameters<typeof SomeError>[0]>.
  • Restored the canonical check at the definition site — all three parsePackageSpec return statements gained satisfies RegistryPackageSpec / satisfies RepositoryPackageSpec.
  • Corrected the Zed lsp.oxfmt settings — dropped the oxlint-namespaced keys (bare configPath, typeAware, unusedDisableDirectives) and the obsolete fmt.experimental, leaving fmt.configPath and run.
  • Documented the two as A casts in src/commands/__tests__/init.test.ts with // 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.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread src/lib/core/types.ts
@adelrodriguez
adelrodriguez force-pushed the maintenance/upgrade-adamantite branch from 138bce8 to 064bbb7 Compare August 23, 2026 05:27

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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-version file — pins Bun at 1.3.14, matching the @types/bun devDependency.
  • Restored the Bun pin on adamantite.yml — the workflow regained a with: block reading bun-version-file: ".bun-version", so it no longer resolves to whatever Bun released most recently.
  • Moved the other three workflows onto the same filesbuild.yml, test.yml, and release.yml swapped node-version: "24" for node-version-file: ".node-version" and bun-version: 1.3.14 for bun-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.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread .bun-version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant