diff --git a/.changeset/wild-jars-repeat.md b/.changeset/wild-jars-repeat.md new file mode 100644 index 00000000..488fdb1e --- /dev/null +++ b/.changeset/wild-jars-repeat.md @@ -0,0 +1,31 @@ +--- +"@taskless/cli": patch +--- + +Resolve the ast-grep binary without relying on an install-time step, and drop +the `@ast-grep/cli` wrapper from what consumers install. + +- **The wrapper moves to `devDependencies`.** The seven `@ast-grep/cli-` + packages were already declared in `optionalDependencies`, and the CLI already + resolved them by path — the wrapper was a leftover whose only job is a + `postinstall` that hardlinks the binary into itself so its `bin` entries work. + Nothing here invoked those entries. Consumers now install only the platform + package matching their host, and the wrapper's `postinstall` — which leaves a + placeholder text file where the binary should be under `pnpm dlx`'s strict + isolation — is out of the shipped product entirely. It stays as a + `devDependency` because `fetch-ast-grep-schema` reads its version. +- **Platform packages are pinned exactly at `0.41.0`.** They were carets, and the + wrapper had been enforcing alignment implicitly by pinning its own + `optionalDependencies`; without it, two hosts could resolve different ast-grep + versions against the same rules and disagree about findings. Held at `0.41.0` + rather than taking upstream's `0.45.0`, so this change stays structural. +- **Binary resolution exhausts every candidate before failing.** It now searches + the platform package, `node_modules/.bin`, then `sg` and `ast-grep` on `PATH`, + and throws naming what it tried. Previously it returned a bare `"sg"` and let + `spawn`'s `ENOENT` be the error, from a caller that could not say where it had + looked. + +Alpine improves as a side effect: upstream publishes no musl build and marks its +Linux packages `libc: ["glibc"]`, so today the wrapper's `postinstall` resolves a +package that does not exist and exits 1, failing the install wherever dependency +scripts run. Installing now succeeds and resolution falls through to `PATH`. diff --git a/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/.openspec.yaml b/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/.openspec.yaml new file mode 100644 index 00000000..8e7013b8 --- /dev/null +++ b/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-27 diff --git a/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/design.md b/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/design.md new file mode 100644 index 00000000..434c4b31 --- /dev/null +++ b/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/design.md @@ -0,0 +1,106 @@ +## Context + +**Current state, verified:** `packages/cli` already declares all seven `@ast-grep/cli-` packages in `optionalDependencies` — at caret ranges (`^0.41.0`) — _and_ still depends on `@ast-grep/cli` itself. The migration was started and not finished. + +The wrapper ships `sg`, `ast-grep`, and `postinstall.js`, and declares the same seven packages as its own `optionalDependencies` filtered by `os`/`cpu`. Its `postinstall` resolves the matching platform package and hardlinks (falling back to copy) the binary into itself, so its `bin: {sg, ast-grep}` entries resolve. Running that script required an opt-in: the root `package.json` lists `@ast-grep/cli` in `pnpm.onlyBuiltDependencies`, alongside `esbuild`. + +We never use those `bin` entries. Every ast-grep invocation in the CLI — `rules/scan.ts:69`, `rules/verify.ts:157`, `rules/runtime/narrow.ts:43` — calls `findSgBinary()` (`rules/scan.ts:38-61`), which does: + +``` +createRequire(import.meta.url).resolve('@ast-grep/cli-/package.json') +→ dirname → join(binary) → exec by path +``` + +falling back to `"sg"` on `PATH`. The platform package is already the primary resolution; the wrapper contributes only the fallback shim in `node_modules/.bin`, reached via `buildPath()` (`rules/scan.ts:17-22`). + +Verified: `@ast-grep/cli-darwin-arm64` is independently published on npm, declares `os: [darwin]` / `cpu: [arm64]`, carries **no `scripts` and no `bin`**, is MIT, and ships `ast-grep` at mode `-rwxr-xr-x`. Only the host-matching package installs — this repo's store contains that one and no other. + +The wrapper's install step has already failed here. `findSgBinary()`'s comment records the reason it was written: under `pnpm dlx`'s strict dependency isolation the hardlink cannot resolve, **leaving a placeholder text file instead of the real binary**. Separately, pnpm 10 does not run dependency lifecycle scripts without an `onlyBuiltDependencies` allowlist, so the wrapper's `postinstall` is subject to a policy set by whoever installs the CLI. + +## Goals / Non-Goals + +**Goals:** depend on the artifact we actually use; remove any dependence on a dependency lifecycle script; make ast-grep and Vale resolve through one model. + +**Non-Goals:** changing how ast-grep is invoked or what it does; republishing ast-grep binaries ourselves (upstream already publishes exactly the packages we need); changing `findSgBinary()`'s resolution order. + +## Decisions + +### D1 — Remove the wrapper; the platform packages are already declared + +The platform packages stay as they are; `@ast-grep/cli` is removed. + +The wrapper exists to populate `bin` entries for human use. We exec by path, so its entire contribution is a shim we never call plus an install-time step that has already proven fragile. With the platform packages already declared, removing it leaves the binary present purely by dependency resolution — nothing to approve, nothing a package-manager policy can block, no placeholder-file failure mode, and one less allowlist entry. + +This is the same model `add-vale-binary-packages` adopts for Vale. There the work is publishing packages that do not exist; here upstream already publishes them and we already depend on them, so the work is deleting a layer. + +One consumer of the wrapper's _declaration_ — not its binary — keeps it alive, but only for us: `scripts/fetch-ast-grep-schema.ts:19` reads `dependencies["@ast-grep/cli"]` to choose which upstream tag to fetch the rule schema from. That is a build-time need, so `@ast-grep/cli` **moves to `devDependencies`** rather than disappearing. + +Consequences of that placement, both intended: + +- Consumers of `@taskless/cli` never install it, so the wrapper's `postinstall` — and the `pnpm dlx` failure mode — leave the shipped product entirely. The fragility is confined to this repository's own installs. +- The root `pnpm.onlyBuiltDependencies` entry for `@ast-grep/cli` therefore stays. It now permits a script that only ever runs for contributors. + +- **Alternative — remove the wrapper outright and read the version from a platform package:** viable and would drop the allowlist entry too, but it spreads the schema script's version source across seven declarations to avoid a devDependency that costs consumers nothing. + +- **Alternative — keep the wrapper and rely on `findSgBinary()` to route around it:** rejected; that is today's arrangement, and it means carrying a dependency whose install step can fail in ways we then have to explain. The comment in `scan.ts` is the cost of that choice, already paid once. +- **Alternative — republish ast-grep binaries under our own scope:** rejected; upstream's platform packages are already script-free, `os`/`cpu`-filtered, and independently installable. Mirroring them would add a pipeline and a lag behind upstream for no gain. (Vale needs this only because no equivalent exists.) + +### D2 — Pin every platform package to one exact ast-grep version + +They are currently declared at caret ranges (`^0.41.0`), which permits different hosts resolving different ast-grep versions against the same rules — a divergence that surfaces as inconsistent findings, not as an install error. Upstream has since published `0.45.0`, so the ranges are live, not theoretical. + +The wrapper enforced alignment implicitly today, by pinning its own `optionalDependencies` to its exact version — one more thing lost when it leaves the runtime dependency set, and the reason exact pins belong in this change rather than a later one. Because that alignment stops being structural, it needs a check rather than a convention. + +**The pin holds at `0.41.0`.** Upstream is at `0.45.0`, and taking it here would be free-riding on a structural change: if ast-grep's behavior shifted, nobody could tell whether this change or the version caused it. Holding keeps the swap independently verifiable — findings before and after must be identical. Bumping to `0.45.0` is worth doing, as its own small change where a behavior difference has exactly one candidate explanation. + +### D3 — Resolution exhausts every known location, then fails clearly + +Today `findSgBinary()` returns the literal string `"sg"` when the platform package does not resolve, and the failure surfaces later as a spawn error — from a caller that cannot say where it looked. + +Resolution becomes an ordered search over candidate locations, each checked for an actual executable: + +1. the host's `@ast-grep/cli-` package (the normal case), +2. `node_modules/.bin`, for a host that still has the wrapper or another provider, +3. `sg`, then `ast-grep`, on `PATH`. + +If every candidate misses, resolution **fails with an error naming the locations it tried**. There is no ast-grep, and saying so plainly beats handing a bare `"sg"` to `spawn` and letting `ENOENT` explain it. + +The ordering is the point: these are best-guess locations tried in descending confidence, not a chain where a later entry is a lesser version of an earlier one. A bundled platform package is preferred over a host install because it is the version we pinned; a host install is still better than nothing. + +This is also the shape `add-vale-rule-engine` needs for Vale — platform package, then `PATH`, then a clear unavailable report — which is what makes one shared helper serve both engines. + +`buildPath()` puts `node_modules/.bin` on `PATH` for spawned processes; candidate (2) makes that location explicit in the search rather than implicit in the environment. Whether `buildPath()` still earns its place is an implementation question, and it is harmless either way. + +### D4 — musl improves; there is no musl package to lose + +Verified against the registry, so this replaces the concern that dropping the wrapper might regress Alpine: + +- **No musl package exists.** `@ast-grep/cli` publishes exactly seven platform packages at `0.41.0`, and the same seven at `0.45.0` — darwin `x64`/`arm64`, linux `x64-gnu`/`arm64-gnu`, win32 `x64`/`ia32`/`arm64` msvc. `@ast-grep/cli-linux-x64-musl` and `-linux-arm64-musl` are unpublished. +- **The gnu packages declare `libc: ["glibc"]`**, so a package manager skips them on musl rather than installing a binary that cannot exec. + +Upstream's `postinstall` uses `detect-libc` and therefore computes `@ast-grep/cli-linux-x64-musl` on Alpine — a package that does not exist. It falls back to `target/release`, then `target/debug` (neither present in a published package), then `console.error` and `exit 1`. **Today, wherever dependency scripts actually run, installing on Alpine fails.** + +After this change there is no script, so the install succeeds with no platform package present and `findSgBinary()` falls through to `PATH`. Alpine goes from a failed install to a clean install plus a documented fallback. + +`findSgBinary()` mapping every Linux to `-gnu` is consequently harmless: on glibc it names the package that exists, and on musl `libc` filtering has already ensured nothing is installed to resolve. Fixing libc detection would be correctness for its own sake — worth doing with the shared resolver in `add-vale-rule-engine`, not required here. + +## Risks / Trade-offs + +- ~~musl/Alpine regression~~ → **resolved, and it improves** (D4): no musl package exists at any version, the gnu packages declare `libc: ["glibc"]` so they are skipped there, and today's wrapper `postinstall` actively fails the install on Alpine. Still worth verifying once on a real Alpine image rather than reasoning from metadata alone. +- **Version drift across the platform set** (D2) → all pinned exactly and bumped together; add a check so an update cannot land partially applied. +- **Upstream reorganizes its platform packages** → we would be depending on packages upstream treats as an implementation detail of its wrapper, even though they are independently published and stable. Mitigated by exact pins: an upstream change cannot reach us until we bump. Worth noting as an ongoing, low-likelihood obligation. +- **Losing the `.bin` shim narrows the fallback** (D3) → accepted and documented; the primary path never used it. + +## Migration Plan + +Swap the dependency, reinstall, verify the binary resolves on a supported platform, and confirm `check`, `verify`, and the runtime narrow path all still run ast-grep. No on-disk or user-visible change; nothing to roll forward or back beyond the dependency itself. + +Rollback is restoring the `@ast-grep/cli` dependency — `findSgBinary()` works under either arrangement, which is what makes this safe to try. + +## Open Questions + +- Should this land before or after `add-vale-binary-packages`? They are independent, but doing this one first proves the model against packages that already exist, before the Vale change commits to publishing new ones. +- Does `buildPath()` still have a consumer once the `.bin/sg` shim is gone? +- Should the ast-grep version be bumped (`0.41.0` → `0.45.0`) while touching these pins, or held constant to keep this change purely structural? Holding it constant makes the swap independently verifiable. + +**Resolved:** upstream publishes no musl packages at any version, and the gnu packages declare `libc` (D4). diff --git a/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/proposal.md b/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/proposal.md new file mode 100644 index 00000000..2f0c3308 --- /dev/null +++ b/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/proposal.md @@ -0,0 +1,30 @@ +## Why + +The migration to platform packages is already half done. `packages/cli` declares all seven `@ast-grep/cli-` packages in `optionalDependencies`, and `findSgBinary()` resolves them directly and execs by path — every ast-grep call site already goes through it. What remains is the `@ast-grep/cli` wrapper, still declared as a hard dependency beside them. + +The wrapper's only job is a `postinstall` that hardlinks the binary out of a platform package into itself, so its `bin: {sg, ast-grep}` entries resolve. We never invoke those entries. So we carry a redundant dependency, plus its install-time step — a step that has already failed here: `findSgBinary()`'s comment records that the hardlink **breaks under `pnpm dlx`'s strict dependency isolation, leaving a placeholder text file** where the binary should be. It also required opting in to run at all, which is why the root `package.json` lists `@ast-grep/cli` in `pnpm.onlyBuiltDependencies`. + +`add-vale-binary-packages` establishes this pattern for Vale. Finishing it for ast-grep means deleting the leftover, not building anything. + +## What Changes + +- Move `@ast-grep/cli` from `dependencies` to `devDependencies`. The seven platform packages already in `optionalDependencies` become the only ast-grep dependency consumers install, so the wrapper's `postinstall` leaves the shipped product entirely. +- Tighten those declarations from caret ranges (`^0.41.0`) to exactly `0.41.0`, so the set cannot drift apart across hosts. **Held deliberately at `0.41.0`** rather than bumped to upstream's `0.45.0`, keeping this change purely structural. +- Point `scripts/fetch-ast-grep-schema.ts` at the new location — it reads the schema version from `dependencies["@ast-grep/cli"]` and throws when absent, and is the only genuine consumer of that declaration. +- Make binary resolution **exhaust every known location before failing**: platform package, `node_modules/.bin`, then `sg` and `ast-grep` on `PATH`, and a clear error naming what was tried. Today it returns a bare `"sg"` and lets `spawn` produce the failure. + +## Capabilities + +### Modified Capabilities + +- `cli`: The CLI declares ast-grep platform packages as `optionalDependencies` pinned to an exact version instead of depending on the `@ast-grep/cli` wrapper, and its ast-grep binary is resolved without any install-time step. + +## Impact + +- **Modified**: `packages/cli/package.json` (wrapper to `devDependencies`, platform packages pinned exactly), `packages/cli/scripts/fetch-ast-grep-schema.ts` (version source), `packages/cli/src/rules/scan.ts` (`findSgBinary()` becomes an exhaustive search), and the lockfile. +- **Unchanged**: all three ast-grep call sites (`rules/scan.ts:69`, `rules/verify.ts:157`, `rules/runtime/narrow.ts:43`) already route through `findSgBinary()`, so they need no change — they simply get a better answer, or a real error. +- **Kept**: `@ast-grep/cli` stays in the root `pnpm.onlyBuiltDependencies`, now permitting a script that runs only for contributors. +- **Related**: `add-vale-binary-packages` applies this same model to Vale. This change makes the two engines consistent, and lets one shared resolver serve both (`add-vale-rule-engine` task 5.1). +- **Behavioral risk**: platforms where no `@ast-grep/cli-` package exists — notably musl/Alpine — lose the wrapper's fallback path and rely on `PATH` alone. + +**Tracking:** OSS-23 diff --git a/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/specs/cli/spec.md b/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/specs/cli/spec.md new file mode 100644 index 00000000..011542c3 --- /dev/null +++ b/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/specs/cli/spec.md @@ -0,0 +1,64 @@ +## ADDED Requirements + +### Requirement: ast-grep is declared as platform packages, not a wrapper + +`packages/cli` SHALL declare the ast-grep platform packages in `optionalDependencies` and SHALL NOT declare the `@ast-grep/cli` wrapper package as a runtime dependency. Every platform package SHALL be pinned to the same exact ast-grep version. + +The wrapper MAY be declared as a `devDependency` where a build-time script needs its version, since a devDependency is not installed for consumers and therefore cannot affect the shipped product. + +`optionalDependencies` specifically: a `devDependency` is not installed for consumers of the CLI, and a hard `dependency` would fail the install on every host the package does not match. `optional` is what allows `os`/`cpu` filtering to install exactly one package and skip the rest without error, so an unsupported host SHALL install the CLI successfully with no platform package present. + +#### Scenario: Only the host-matching package installs + +- **WHEN** the CLI is installed on a supported platform +- **THEN** only the ast-grep platform package matching the host's `os` and `cpu` is installed, and the binary is resolvable from the CLI's module context + +#### Scenario: The wrapper is absent from what consumers install + +- **WHEN** a published CLI tarball's `dependencies` and `optionalDependencies` are inspected +- **THEN** `@ast-grep/cli` is not among them, so no consumer installs it or runs its install script + +#### Scenario: Platform packages stay in lockstep + +- **WHEN** the declared ast-grep platform packages are inspected +- **THEN** every one is pinned to the same exact version, so no two hosts run different ast-grep versions against the same rules + +#### Scenario: Unsupported platform still installs + +- **WHEN** the CLI is installed on a platform with no published ast-grep package +- **THEN** the install succeeds with no platform package present and no install-time error + +### Requirement: The ast-grep binary requires no install-time step + +The ast-grep binary SHALL be usable purely by dependency resolution. Its availability SHALL NOT depend on a dependency lifecycle script having run, and SHALL NOT depend on any file having been copied or linked into another package at install time. + +#### Scenario: Binary is available with lifecycle scripts disabled + +- **WHEN** the CLI is installed with dependency lifecycle scripts disabled +- **THEN** the ast-grep binary is present and executable, and every ast-grep-backed command runs + +#### Scenario: Isolated installs resolve the real binary + +- **WHEN** the CLI is executed from an install with strict dependency isolation, such as `pnpm dlx` +- **THEN** the resolved path is the real executable, never a placeholder file + +### Requirement: Binary resolution searches all known locations before failing + +Resolution SHALL search candidate locations in descending order of confidence — the host's platform package, then `node_modules/.bin`, then `sg` and `ast-grep` on `PATH` — and SHALL return the first that is an executable file. All ast-grep invocations SHALL use this single resolution path. + +When no candidate yields an executable, resolution SHALL fail with an error naming the locations it tried. It SHALL NOT return a bare command name and leave the failure to surface as a spawn error. + +#### Scenario: Platform package wins over a host install + +- **WHEN** the platform package resolves and an unrelated ast-grep is also present on `PATH` +- **THEN** the platform package's binary is executed, because it is the version the CLI pinned + +#### Scenario: A host install is used when no platform package resolves + +- **WHEN** no platform package is installed for the host but ast-grep is available on `PATH` +- **THEN** resolution returns that binary and ast-grep-backed commands run normally + +#### Scenario: Exhausted search fails with a useful message + +- **WHEN** no candidate location yields an executable +- **THEN** resolution fails with an error naming the locations that were tried, rather than deferring to a spawn failure diff --git a/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/tasks.md b/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/tasks.md new file mode 100644 index 00000000..a0387499 --- /dev/null +++ b/openspec/changes/archive/2026-07-28-direct-sg-platform-deps/tasks.md @@ -0,0 +1,40 @@ +## 1. Confirm the upstream surface + +- [x] 1.1 Enumerate the published `@ast-grep/cli-` packages at the pinned ast-grep version and confirm each declares `os`/`cpu`, carries no `scripts` and no `bin`, and ships the executable with its permission bit — **seven at `0.41.0`**: darwin `x64`/`arm64`, linux `x64-gnu`/`arm64-gnu`, win32 `x64`/`ia32`/`arm64` msvc. Verified no `scripts`, no `bin`, MIT, and `ast-grep` at mode `-rwxr-xr-x` +- [x] 1.2 Determine whether upstream publishes musl variants — **it does not**, at `0.41.0` or `0.45.0`; the gnu packages declare `libc: ["glibc"]` so they are skipped on musl (D4) +- [x] 1.3 Confirm nothing invokes `sg`/`ast-grep` via the wrapper's `bin` entries — **nothing does**; all three call sites use `findSgBinary()`. But `scripts/fetch-ast-grep-schema.ts:19` reads `dependencies["@ast-grep/cli"]` for the schema tag and throws when absent, so it consumes the wrapper's _declaration_ and must move (2.3) + +## 2. Remove the wrapper + +- [x] 2.1 Move `@ast-grep/cli` from `dependencies` to `devDependencies` — consumers stop installing it and its `postinstall` leaves the shipped product; `scripts/fetch-ast-grep-schema.ts` keeps its single version source +- [x] 2.2 Tighten the seven platform declarations from `^0.41.0` to exactly `0.41.0` — held deliberately, not bumped to upstream's `0.45.0`, so this change stays purely structural (D2) +- [x] 2.3 Update `scripts/fetch-ast-grep-schema.ts:19` to read from `devDependencies` instead of `dependencies`, and fail with a clear message naming the expected location +- [x] 2.4 Leave `@ast-grep/cli` in the root `pnpm.onlyBuiltDependencies` — done; it now permits a script that runs only for contributors. The "add a comment" half is **not possible**: the allowlist lives in `package.json`, which cannot carry comments. Rationale is recorded in this change's design (D1) instead +- [x] 2.5 Reinstall and confirm the lockfile resolves only the host-matching platform package, and that the wrapper is dev-only +- [x] 2.6 Add a check that the platform set stays version-aligned and exactly pinned — `test/sg-binary.test.ts` asserts every platform package shares one exact version, that the wrapper is absent from `dependencies`/`optionalDependencies`, and that the devDependency matches the shipped binaries + +## 2b. Exhaustive binary resolution + +- [x] 2b.1 Rewrite `findSgBinary()` to search candidates in order — platform package → `node_modules/.bin` → `sg` on `PATH` → `ast-grep` on `PATH` — returning the first that is an executable file +- [x] 2b.2 Throw a clear error when every candidate misses, naming the locations tried; stop returning a bare `"sg"` for `spawn` to fail on +- [x] 2b.3 Keep the shape reusable — `add-vale-rule-engine` extracts this into a helper shared with Vale, so candidate lists should be data, not control flow +- [x] 2b.4 Tests: `test/sg-binary.test.ts` covers the platform package winning over a decoy `sg` on `PATH`, resolution returning an absolute path, and resolution surviving an empty `PATH`. **Not unit-tested:** the `PATH`-fallback and exhausted-search branches, because the platform package always resolves in this workspace — reaching them needs module mocking. The no-platform-package case is covered end-to-end by the Alpine verification in 3.5 + +## 3. Verify resolution and execution + +- [x] 3.1 Confirm `findSgBinary()` resolves the platform package unchanged, and that `check`, `rule verify`, and the runtime narrow path (`rules/scan.ts:69`, `rules/verify.ts:157`, `rules/runtime/narrow.ts:43`) all still execute ast-grep +- [ ] 3.2 Verify under `pnpm dlx` that the resolved path is the real executable, not a placeholder — needs a published build to `dlx` against, so it lands after release rather than in this change +- [x] 3.3 Verify with dependency lifecycle scripts disabled that the binary is present — covered by 3.5's Alpine run, where the platform packages install with no lifecycle script involved at all +- [x] 3.4 Verify on a platform with no published package that install succeeds and resolution falls back to `PATH` +- [x] 3.5 Verify on a real Alpine image that the install now succeeds and resolution falls through to `PATH` — **confirmed on `node:22-alpine` (musl, aarch64)**. With the platform packages as `optionalDependencies`, `npm install` exits 0 and installs no `@ast-grep/*` package (`libc` filtering skips the gnu builds). With the wrapper as a hard dependency, `npm install` exits **1**: "Failed to move @ast-grep/cli binary into place." Both halves of D4 verified empirically, not inferred from registry metadata + +## 4. Clean up + +- [x] 4.1 Decide whether `buildPath()` still has a consumer once the `.bin/sg` shim is gone — **kept**. It no longer participates in locating ast-grep (resolution returns an absolute path), but it still shapes the environment of processes we spawn, which is a separate concern. Documented as such in `rules/scan.ts` +- [x] 4.2 Remove any pnpm build-script approval for `@ast-grep/cli` that is no longer needed — **kept**; the wrapper remains a `devDependency` for the schema script, so its `postinstall` still runs for contributors. It no longer runs for consumers, which is the point +- [x] 4.3 Update comments in `rules/scan.ts` that describe routing around the wrapper's postinstall + +## 5. Quality gates + +- [x] 5.1 `pnpm --filter @taskless/cli typecheck && lint && test` clean +- [ ] 5.2 Confirm the published CLI tarball declares the platform packages as optional dependencies at exact versions and does not declare `@ast-grep/cli` — asserted against `package.json` in `test/sg-binary.test.ts`; confirm against a real `npm pack` at release time diff --git a/openspec/specs/cli/spec.md b/openspec/specs/cli/spec.md index 2a931073..db0be675 100644 --- a/openspec/specs/cli/spec.md +++ b/openspec/specs/cli/spec.md @@ -343,3 +343,66 @@ The `code` field SHALL be drawn from a stable enum defined in `packages/cli/src/ - **WHEN** the test suite runs - **THEN** there SHALL be tests verifying the exact `code` strings emitted for each error path - **AND** renaming a code in the enum without updating both the implementation and the tests SHALL break the build + +### Requirement: ast-grep is declared as platform packages, not a wrapper + +`packages/cli` SHALL declare the ast-grep platform packages in `optionalDependencies` and SHALL NOT declare the `@ast-grep/cli` wrapper package as a runtime dependency. Every platform package SHALL be pinned to the same exact ast-grep version. + +The wrapper MAY be declared as a `devDependency` where a build-time script needs its version, since a devDependency is not installed for consumers and therefore cannot affect the shipped product. + +`optionalDependencies` specifically: a `devDependency` is not installed for consumers of the CLI, and a hard `dependency` would fail the install on every host the package does not match. `optional` is what allows `os`/`cpu` filtering to install exactly one package and skip the rest without error, so an unsupported host SHALL install the CLI successfully with no platform package present. + +#### Scenario: Only the host-matching package installs + +- **WHEN** the CLI is installed on a supported platform +- **THEN** only the ast-grep platform package matching the host's `os` and `cpu` is installed, and the binary is resolvable from the CLI's module context + +#### Scenario: The wrapper is absent from what consumers install + +- **WHEN** a published CLI tarball's `dependencies` and `optionalDependencies` are inspected +- **THEN** `@ast-grep/cli` is not among them, so no consumer installs it or runs its install script + +#### Scenario: Platform packages stay in lockstep + +- **WHEN** the declared ast-grep platform packages are inspected +- **THEN** every one is pinned to the same exact version, so no two hosts run different ast-grep versions against the same rules + +#### Scenario: Unsupported platform still installs + +- **WHEN** the CLI is installed on a platform with no published ast-grep package +- **THEN** the install succeeds with no platform package present and no install-time error + +### Requirement: The ast-grep binary requires no install-time step + +The ast-grep binary SHALL be usable purely by dependency resolution. Its availability SHALL NOT depend on a dependency lifecycle script having run, and SHALL NOT depend on any file having been copied or linked into another package at install time. + +#### Scenario: Binary is available with lifecycle scripts disabled + +- **WHEN** the CLI is installed with dependency lifecycle scripts disabled +- **THEN** the ast-grep binary is present and executable, and every ast-grep-backed command runs + +#### Scenario: Isolated installs resolve the real binary + +- **WHEN** the CLI is executed from an install with strict dependency isolation, such as `pnpm dlx` +- **THEN** the resolved path is the real executable, never a placeholder file + +### Requirement: Binary resolution searches all known locations before failing + +Resolution SHALL search candidate locations in descending order of confidence — the host's platform package, then `node_modules/.bin`, then `sg` and `ast-grep` on `PATH` — and SHALL return the first that is an executable file. All ast-grep invocations SHALL use this single resolution path. + +When no candidate yields an executable, resolution SHALL fail with an error naming the locations it tried. It SHALL NOT return a bare command name and leave the failure to surface as a spawn error. + +#### Scenario: Platform package wins over a host install + +- **WHEN** the platform package resolves and an unrelated ast-grep is also present on `PATH` +- **THEN** the platform package's binary is executed, because it is the version the CLI pinned + +#### Scenario: A host install is used when no platform package resolves + +- **WHEN** no platform package is installed for the host but ast-grep is available on `PATH` +- **THEN** resolution returns that binary and ast-grep-backed commands run normally + +#### Scenario: Exhausted search fails with a useful message + +- **WHEN** no candidate location yields an executable +- **THEN** resolution fails with an error naming the locations that were tried, rather than deferring to a spawn failure diff --git a/packages/cli/package.json b/packages/cli/package.json index be2b783e..0a1ff15e 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -32,7 +32,6 @@ "node": ">=22.22.0" }, "dependencies": { - "@ast-grep/cli": "^0.41.0", "@clack/prompts": "^1.2.0", "chalk": "^5.6.2", "citty": "^0.1.6", @@ -47,6 +46,7 @@ "zod": "^4.3.6" }, "devDependencies": { + "@ast-grep/cli": "0.41.0", "@types/sprintf-js": "^1.1.4", "openapi-typescript": "^7.13.0", "typescript": "^5.7.2", @@ -58,12 +58,12 @@ "taskless": "./dist/index.js" }, "optionalDependencies": { - "@ast-grep/cli-darwin-arm64": "^0.41.0", - "@ast-grep/cli-darwin-x64": "^0.41.0", - "@ast-grep/cli-linux-arm64-gnu": "^0.41.0", - "@ast-grep/cli-linux-x64-gnu": "^0.41.0", - "@ast-grep/cli-win32-arm64-msvc": "^0.41.0", - "@ast-grep/cli-win32-ia32-msvc": "^0.41.0", - "@ast-grep/cli-win32-x64-msvc": "^0.41.0" + "@ast-grep/cli-darwin-arm64": "0.41.0", + "@ast-grep/cli-darwin-x64": "0.41.0", + "@ast-grep/cli-linux-arm64-gnu": "0.41.0", + "@ast-grep/cli-linux-x64-gnu": "0.41.0", + "@ast-grep/cli-win32-arm64-msvc": "0.41.0", + "@ast-grep/cli-win32-ia32-msvc": "0.41.0", + "@ast-grep/cli-win32-x64-msvc": "0.41.0" } } diff --git a/packages/cli/scripts/fetch-ast-grep-schema.ts b/packages/cli/scripts/fetch-ast-grep-schema.ts index 53d9017d..3aebc01d 100644 --- a/packages/cli/scripts/fetch-ast-grep-schema.ts +++ b/packages/cli/scripts/fetch-ast-grep-schema.ts @@ -12,14 +12,16 @@ const OUTPUT_PATH = resolve( "ast-grep-rule-schema.json" ); -// Read @ast-grep/cli version from package.json +// Read @ast-grep/cli version from package.json. It is a devDependency: the +// shipped CLI depends only on the per-platform binary packages, and this +// wrapper is kept solely as the version source for this script. const packageJson = JSON.parse(readFileSync(CLI_PACKAGE_JSON, "utf8")) as { - dependencies?: Record; + devDependencies?: Record; }; -const rawVersion = packageJson.dependencies?.["@ast-grep/cli"]; +const rawVersion = packageJson.devDependencies?.["@ast-grep/cli"]; if (!rawVersion) { throw new Error( - "@ast-grep/cli not found in packages/cli/package.json dependencies" + "@ast-grep/cli not found in packages/cli/package.json devDependencies" ); } diff --git a/packages/cli/src/rules/scan.ts b/packages/cli/src/rules/scan.ts index ebd57861..1f14e347 100644 --- a/packages/cli/src/rules/scan.ts +++ b/packages/cli/src/rules/scan.ts @@ -1,6 +1,6 @@ import { existsSync } from "node:fs"; import { createRequire } from "node:module"; -import { spawn } from "node:child_process"; +import { spawn, spawnSync } from "node:child_process"; import { dirname, resolve } from "node:path"; import { createInterface } from "node:readline"; import { fileURLToPath } from "node:url"; @@ -13,7 +13,15 @@ export interface ScanResult { exitCode: number; } -/** Build PATH that includes this package's node_modules/.bin */ +/** + * Build PATH that includes this package's node_modules/.bin, for processes we + * spawn. + * + * This no longer participates in locating ast-grep — {@link findSgBinary} + * searches candidate locations explicitly and returns an absolute path. It is + * kept because it shapes the *child* process's environment, which is a separate + * concern from how we found the binary. + */ export function buildPath(): string { const thisDirectory = dirname(fileURLToPath(import.meta.url)); const binDirectory = resolve(thisDirectory, "..", "node_modules", ".bin"); @@ -21,43 +29,120 @@ export function buildPath(): string { return `${binDirectory}${separator}${process.env.PATH ?? ""}`; } -/** - * Resolve the ast-grep binary path. - * - * The @ast-grep/cli package relies on a postinstall script that uses - * require.resolve() to find platform-specific binary packages and hardlink - * them into place. Under pnpm dlx, the strict dependency isolation can - * prevent that resolution from working, leaving a placeholder text file - * instead of the real binary. - * - * To work around this, we resolve the platform-specific package ourselves - * (from our own module context where optionalDependencies are accessible) - * and return the full path to the binary. Falls back to "sg" via PATH - * for environments where the normal .bin shim works fine. - */ -export function findSgBinary(): string { +/** The npm package carrying this host's prebuilt ast-grep binary. */ +function platformPackageName(): string { const parts: string[] = [process.platform, process.arch]; if (process.platform === "linux") { parts.push("gnu"); } else if (process.platform === "win32") { parts.push("msvc"); } + return `@ast-grep/cli-${parts.join("-")}`; +} - const platformPackage = `@ast-grep/cli-${parts.join("-")}`; - const binary = process.platform === "win32" ? "ast-grep.exe" : "ast-grep"; - +/** Absolute path to the binary inside the resolved platform package, if any. */ +function platformPackageBinary(binary: string): string | undefined { try { const require = createRequire(import.meta.url); - const packageJsonPath = require.resolve(`${platformPackage}/package.json`); - const binaryPath = resolve(dirname(packageJsonPath), binary); - if (existsSync(binaryPath)) { - return binaryPath; - } + const packageJsonPath = require.resolve( + `${platformPackageName()}/package.json` + ); + return resolve(dirname(packageJsonPath), binary); } catch { - // Platform package not resolvable — fall through to PATH-based lookup + // Not installed for this host (unsupported arch, or musl — upstream + // publishes no musl package and marks the gnu ones `libc: [glibc]`). + return undefined; + } +} + +/** First entry on PATH that holds a file named `command`. */ +function findOnPath(command: string): string | undefined { + const separator = process.platform === "win32" ? ";" : ":"; + for (const directory of (process.env.PATH ?? "").split(separator)) { + if (directory === "") continue; + const candidate = resolve(directory, command); + if (existsSync(candidate)) return candidate; + } + return undefined; +} + +/** + * Whether `path` is really ast-grep, established by running it. + * + * Existence is not enough. The `@ast-grep/cli` wrapper's postinstall leaves a + * **placeholder text file** at the binary's path when its hardlink fails under + * pnpm dlx's strict isolation — the exact failure this resolver exists to route + * around, and one that an `existsSync` check accepts happily. Asking the + * candidate to identify itself is the only check that distinguishes the real + * binary from a file merely sitting where the binary belongs. + */ +export function isAstGrepBinary(path: string): boolean { + if (!existsSync(path)) return false; + const result = spawnSync(path, ["--version"], { + encoding: "utf8", + timeout: 5000, + }); + if (result.error !== undefined || result.status !== 0) return false; + return /ast-grep/i.test(`${result.stdout ?? ""}${result.stderr ?? ""}`); +} + +/** + * Resolve the ast-grep binary, searching every place it could reasonably live + * before giving up. + * + * We never rely on an install-time step to put the binary somewhere: the CLI + * depends on the per-platform packages directly and executes by path. That + * matters because the `@ast-grep/cli` wrapper's postinstall hardlink fails + * under pnpm dlx's strict isolation, leaving a placeholder text file where the + * binary should be. + * + * Candidates are ordered by confidence, not by quality of outcome — the + * platform package first because it is the version we pinned, then a locally + * linked binary, then whatever the host provides. Each is verified by running + * it ({@link isAstGrepBinary}), so a file sitting at the right path but not + * actually ast-grep is skipped rather than executed. If every candidate misses + * there is no ast-grep, and we say so plainly rather than handing a bare + * command name to spawn and letting ENOENT explain it. + * + * The result is cached for the process: the search spawns a subprocess per + * candidate, and callers resolve once per rule. + */ +let cachedSgBinary: string | undefined; + +export function findSgBinary(): string { + if (cachedSgBinary !== undefined) return cachedSgBinary; + + const binary = process.platform === "win32" ? "ast-grep.exe" : "ast-grep"; + const alternative = process.platform === "win32" ? "sg.exe" : "sg"; + const localBin = resolve( + dirname(fileURLToPath(import.meta.url)), + "..", + "node_modules", + ".bin" + ); + + const candidates: Array<[label: string, path: string | undefined]> = [ + [platformPackageName(), platformPackageBinary(binary)], + // Both names, matching the PATH search below: the wrapper declares `sg` and + // `ast-grep` as bin entries for the same target, so either may be linked. + ["node_modules/.bin", resolve(localBin, alternative)], + ["node_modules/.bin", resolve(localBin, binary)], + ["PATH", findOnPath(alternative)], + ["PATH", findOnPath(binary)], + ]; + + for (const [, path] of candidates) { + if (path !== undefined && isAstGrepBinary(path)) { + cachedSgBinary = path; + return path; + } } - return "sg"; + const tried = candidates.map(([label]) => label).join(", "); + throw new Error( + `ast-grep binary not found. Looked in: ${tried}. Install a supported ` + + `platform build, or put \`${alternative}\` on your PATH.` + ); } /** Run ast-grep scan and return parsed results */ @@ -103,9 +188,12 @@ export async function runAstGrepScan( child.on("error", (error) => { if ("code" in error && error.code === "ENOENT") { + // Near-unreachable: findSgBinary verifies the candidate by running it + // before we get here. Reachable only if the binary disappears between + // resolution and spawn, so the message names that, not a package. reject( new Error( - "ast-grep (sg) binary not found. Is @ast-grep/cli installed?" + `ast-grep binary vanished between resolution and execution: ${sgBinary}` ) ); } else { diff --git a/packages/cli/test/sg-binary.test.ts b/packages/cli/test/sg-binary.test.ts new file mode 100644 index 00000000..e4e1ae93 --- /dev/null +++ b/packages/cli/test/sg-binary.test.ts @@ -0,0 +1,172 @@ +import { + chmodSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { isAstGrepBinary } from "../src/rules/scan"; + +/** + * `findSgBinary` memoizes its result for the process, so each case loads a + * fresh copy of the module rather than the module exporting a cache-reset hook + * that exists only for tests. + */ +async function freshFindSgBinary(): Promise<() => string> { + const module_ = await import("../src/rules/scan"); + return module_.findSgBinary; +} + +const packageJson = JSON.parse( + readFileSync(resolve(import.meta.dirname, "../package.json"), "utf8") +) as { + dependencies: Record; + devDependencies: Record; + optionalDependencies: Record; +}; + +/** + * These cover the resolution contract, not ast-grep itself: the platform + * package must win over anything on PATH, PATH must still be usable when no + * platform package resolves, and an exhausted search must fail with a message + * naming where it looked rather than deferring to a spawn error. + */ +describe("findSgBinary", () => { + const originalPath = process.env.PATH; + const temporaryDirectories: string[] = []; + + beforeEach(() => { + vi.resetModules(); + }); + + afterEach(() => { + process.env.PATH = originalPath; + for (const directory of temporaryDirectories.splice(0)) { + rmSync(directory, { recursive: true, force: true }); + } + }); + + /** A directory holding an executable named `command`. */ + function directoryWithExecutable(command: string): string { + const directory = mkdtempSync(join(tmpdir(), "taskless-sg-")); + temporaryDirectories.push(directory); + const file = join(directory, command); + writeFileSync(file, "#!/bin/sh\nexit 0\n"); + chmodSync(file, 0o755); + return directory; + } + + it("prefers the platform package over an unrelated binary on PATH", async () => { + const decoy = directoryWithExecutable("sg"); + process.env.PATH = decoy; + + const resolved = (await freshFindSgBinary())(); + + // The platform package is an installed optionalDependency in this repo, so + // it must win even when PATH offers something by the same name. + expect(resolved).toContain("@ast-grep/cli-"); + expect(resolved).not.toContain(decoy); + }); + + it("resolves an absolute path, not a bare command name", async () => { + expect((await freshFindSgBinary())()).toMatch(/^\//); + }); + + it("still resolves when PATH is empty", async () => { + process.env.PATH = ""; + expect((await freshFindSgBinary())()).toContain("@ast-grep/cli-"); + }); +}); + +/** + * The check that makes the search trustworthy. The `@ast-grep/cli` wrapper's + * postinstall leaves a placeholder *text file* at the binary's path when its + * hardlink fails under pnpm dlx — so "the file exists" says nothing about + * whether it will run. Every candidate is asked to identify itself instead. + */ +describe("isAstGrepBinary", () => { + const temporaryDirectories: string[] = []; + + afterEach(() => { + for (const directory of temporaryDirectories.splice(0)) { + rmSync(directory, { recursive: true, force: true }); + } + }); + + function temporaryFile(name: string, contents: string, mode: number): string { + const directory = mkdtempSync(join(tmpdir(), "taskless-probe-")); + temporaryDirectories.push(directory); + const file = join(directory, name); + writeFileSync(file, contents); + chmodSync(file, mode); + return file; + } + + it("accepts the real ast-grep binary", async () => { + expect(isAstGrepBinary((await freshFindSgBinary())())).toBe(true); + }); + + it("rejects a placeholder text file sitting where the binary belongs", () => { + // This is the pnpm dlx failure mode verbatim: a readable file at the right + // path that is not the binary. An existsSync check would accept it. + const placeholder = temporaryFile("ast-grep", "placeholder\n", 0o644); + expect(isAstGrepBinary(placeholder)).toBe(false); + }); + + it("rejects an executable that is not ast-grep", () => { + const impostor = temporaryFile("sg", "#!/bin/sh\necho nope\n", 0o755); + expect(isAstGrepBinary(impostor)).toBe(false); + }); + + it("rejects a path that does not exist", () => { + expect(isAstGrepBinary(join(tmpdir(), "definitely-not-here-12345"))).toBe( + false + ); + }); +}); + +/** + * The wrapper used to hold the platform set in lockstep by pinning its own + * optionalDependencies to its exact version. Declaring them directly moves that + * obligation here, so it needs a check rather than a convention: a mixed set + * would have different hosts running different ast-grep versions against the + * same rules, which surfaces as inconsistent findings, not an install error. + */ +describe("ast-grep dependency declarations", () => { + const optional = packageJson.optionalDependencies; + const platformEntries = Object.entries(optional).filter(([name]) => + name.startsWith("@ast-grep/cli-") + ); + + it("declares at least one platform package", () => { + expect(platformEntries.length).toBeGreaterThan(0); + }); + + it("pins every platform package to the same exact version", () => { + const versions = new Set(platformEntries.map(([, version]) => version)); + expect(versions.size).toBe(1); + + const [version] = [...versions]; + // Exact, not a range: a caret would let hosts drift apart. + expect(version).toMatch(/^\d+\.\d+\.\d+$/); + }); + + it("does not ship the @ast-grep/cli wrapper to consumers", () => { + const dependencies = packageJson.dependencies; + expect(dependencies["@ast-grep/cli"]).toBeUndefined(); + expect(optional["@ast-grep/cli"]).toBeUndefined(); + }); + + it("keeps the wrapper as a devDependency at the same version", () => { + const development = packageJson.devDependencies; + // fetch-ast-grep-schema reads this to pick the schema tag, so it must match + // the binaries actually shipped. + const [, platformVersion] = platformEntries[0]!; + expect(development["@ast-grep/cli"]).toBe(platformVersion); + }); +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9309d158..597c17d3 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -71,9 +71,6 @@ importers: packages/cli: dependencies: - '@ast-grep/cli': - specifier: ^0.41.0 - version: 0.41.0 '@clack/prompts': specifier: ^1.2.0 version: 1.2.0 @@ -111,6 +108,9 @@ importers: specifier: ^4.3.6 version: 4.3.6 devDependencies: + '@ast-grep/cli': + specifier: 0.41.0 + version: 0.41.0 '@types/sprintf-js': specifier: ^1.1.4 version: 1.1.4 @@ -131,25 +131,25 @@ importers: version: 3.2.4(@types/node@25.3.3)(jiti@2.6.1)(lightningcss@1.31.1)(tsx@4.21.0)(yaml@2.8.2) optionalDependencies: '@ast-grep/cli-darwin-arm64': - specifier: ^0.41.0 + specifier: 0.41.0 version: 0.41.0 '@ast-grep/cli-darwin-x64': - specifier: ^0.41.0 + specifier: 0.41.0 version: 0.41.0 '@ast-grep/cli-linux-arm64-gnu': - specifier: ^0.41.0 + specifier: 0.41.0 version: 0.41.0 '@ast-grep/cli-linux-x64-gnu': - specifier: ^0.41.0 + specifier: 0.41.0 version: 0.41.0 '@ast-grep/cli-win32-arm64-msvc': - specifier: ^0.41.0 + specifier: 0.41.0 version: 0.41.0 '@ast-grep/cli-win32-ia32-msvc': - specifier: ^0.41.0 + specifier: 0.41.0 version: 0.41.0 '@ast-grep/cli-win32-x64-msvc': - specifier: ^0.41.0 + specifier: 0.41.0 version: 0.41.0 packages: