Conversation
Added support for an install matrix in the E2E tests for `arkor init` and `create-arkor`, allowing tests to run with different package managers (npm, pnpm, yarn, yarn-berry, bun). Introduced the `ARKOR_E2E_PM` environment variable to control which package manager is used during the tests, enhancing test coverage and reliability across various environments. Updated CI workflow to accommodate this new testing strategy.
Updated the CI workflow to intentionally skip pnpm caching during the setup-node step. This change addresses issues with pnpm not being on the PATH at that point, preventing cache lookup failures. Each job will now fetch dependencies fresh, which, while adding some time, avoids potential cross-OS pitfalls and ensures a smoother setup process.
Enhanced the CI workflow by adding a step to upgrade corepack to the latest version before enabling it. This addresses issues with stale npm-registry signing keys in older Node versions, ensuring successful `pnpm install` operations. The change improves reliability across different environments by preventing signature verification failures.
Two CI failures from the previous install-matrix run: 1. `yarn --version` (and `bun --version`) in the "Confirm pm version" step were rejected by corepack with "This project is configured to use pnpm" because the workspace's package.json pins pnpm. Step out into $RUNNER_TEMP before the version check so corepack falls back to the matrix pm we just `--activate`d. 2. `npm install -g corepack@latest` failed on windows-latest with `EEXIST: ... C:\npm\prefix\yarn.cmd` — the runner image pre-seeds yarn classic's shim under the global npm prefix and corepack tries to write its own yarn shim there. `--force` clobbers the existing shim; harmless on POSIX runners which don't ship the colliding bin.
…ide deps
yarn-berry defaults to Plug'n'Play, which doesn't materialise a
`node_modules/` tree. The arkor runtime (`arkor build` → esbuild →
`node ./.arkor/build/index.mjs`) doesn't load PnP, so a vanilla `arkor
init --use-yarn` against yarn 4 leaves `arkor dev` / `arkor train`
unable to resolve their dependencies.
When the user picks yarn, scaffold now:
- emits `.yarnrc.yml` with `nodeLinker: node-modules` (yarn 1.x
ignores it — it reads `.yarnrc`, not `.yarnrc.yml` — so the file
is harmless on the classic line); existing `.yarnrc.yml` is kept
so users who deliberately want PnP aren't overridden
- appends `.yarn/cache` and `.yarn/install-state.gz` to `.gitignore`
so the initial commit doesn't capture the (~tens of MB) zip cache
yarn berry leaves under `.yarn/` regardless of the linker
`patchGitignore` is generalised to a missing-line set so the yarn
extras coexist with the existing `node_modules/` / `dist/` / `.arkor/`
required entries. `init.ts` and `create-arkor/bin.ts` now thread
`packageManager` into `scaffold(...)`.
Two related tweaks to the install-matrix e2e cases: 1. Drop the `if (label === "yarn-berry") expect(.yarn) else expect (node_modules)` branch. Now that scaffold emits `.yarnrc.yml` with `nodeLinker: node-modules`, yarn-berry produces a real `node_modules/` and the regular invariant holds for every pm. 2. Print the captured arkor stdout/stderr on assertion failure. arkor init / create-arkor swallow `<pm> install` failures into a one-line warning so the user can retry manually — which means a broken pm setup leaves the test with `result.code === 0` but no node_modules, and the only diagnostic ends up buried in result .stderr that vitest doesn't surface. Eager `console.error` keeps the install-matrix CI logs actionable instead of bare `expected false to be true`.
Two more install-matrix failures from the previous run:
1. bun and yarn (both classic and berry) failed with:
error: Workspace dependency "@arkor/cli-internal" not found
error: @arkor/cli-internal@workspace:* failed to resolve
bun and yarn try to resolve a `file:` dep's devDependencies even
though npm/pnpm skip them, so the workspace-protocol entry that
pnpm uses to link cli-internal during the in-repo build trips the
scaffolded fixture's install. cli-internal isn't a real runtime
dep — tsdown bundles it into dist via `deps.alwaysBundle` — and
on a published arkor `pnpm publish` would have stripped /
substituted it. Stage a copy of `packages/arkor` into
`$RUNNER_TEMP/arkor-scaffold-source` with the workspace devDep
removed and aim `ARKOR_INTERNAL_SCAFFOLD_ARKOR_SPEC` there for
the install-matrix tests.
2. Windows × Node 22.13.x kept failing the corepack signing-key
check even after `npm install -g --force corepack@latest`. On
POSIX the upgrade replaces the bundled corepack inside the
tool-cache, but on Windows `npm install -g` lands in
`C:\npm\prefix\` which sits *behind* setup-node's tool-cache in
PATH — the bundled (stale-keys) corepack keeps winning
resolution. The corepack-keys bug isn't pm-version-specific, so
add a single `exclude` for `windows-latest × >=22.13.0
<22.14.0` until either Node 22.13 ships a patch with fresh keys
or we add a tool-cache copy step. Windows pnpm/npm coverage is
still exercised at 22.17 / 22.21 / 24.12.
The build job is unaffected — it points `file:` at
`packages/arkor` directly, both as a canary that the in-workspace
package.json itself is sane and because npm/pnpm (the only pms it
exercises) already skip a file: dep's devDependencies.
yarn classic and yarn-berry both fail in CI with separate root causes that the matrix env can pin around: - yarn-berry (yarn 4): detects \`CI=1\` (set by spawn-cli) and turns on \`enableImmutableInstalls\` by default, refusing to write the lockfile that freshly scaffolded projects don't have yet — exits with \`YN0028: The lockfile would have been created by this install, which is explicitly forbidden\`. Set \`YARN_ENABLE_IMMUTABLE_INSTALLS=false\` so yarn-berry can author the initial lockfile during install. - yarn classic (yarn 1.22.x): has a known race extracting platform-specific optionalDependencies — esbuild's \`@esbuild/<arch>\` packages exit with \`ENOENT: ... open '...integrity/node_modules/@esbuild/linux-x64/.yarn-tarball.tgz'\` because the destination dir is created racily. Setting \`YARN_NETWORK_CONCURRENCY=1\` serialises the extraction and sidesteps the race. yarn classic is in maintenance mode so this isn't going to be fixed upstream. Harmless for yarn-berry, which reads \`networkConcurrency\` from \`.yarnrc.yml\`. Both env vars are scoped to the install-matrix's "Run e2e suite" step so they only apply to the matrix pm under test, not to the regular build matrix.
yarn classic 1.22.x has a known race extracting tarballs into \`~/.cache/yarn/v6/\`: when two yarn processes write to the same cache concurrently, the inner mkdir-then-write sequence collides and the second loser dies with \`ENOENT: ... open '...integrity/node_modules/<pkg>/.yarn-tarball.tgz'\`. The install matrix hits this because vitest runs test files in parallel workers — \`arkor init --use-yarn\` and \`create-arkor --use-yarn\` fire up two simultaneous yarn 1 processes pointed at the shared cache. \`YARN_NETWORK_CONCURRENCY=1\` only serialises within a single yarn process; it doesn't help when two processes race the cache. \`yarn install --mutex network\` would, but that's an arg the SDK doesn't pass and shouldn't (we don't want pm-aware install args). Allocating a per-spawn cache via \`mkdtempSync\` and exposing it through \`YARN_CACHE_FOLDER\` removes the contention entirely. yarn-berry / npm / pnpm / bun all ignore the env var, so this is a yarn-1-only knob in practice. Cleanup on close keeps long CI runs from leaking \`arkor-e2e-yarn-cache-*\` dirs into tmpdir.
The remaining yarn-classic install-matrix failures all share the
same root cause — not a CI bug:
error posthog-node@5.30+: The engine "node" is incompatible
with this module. Expected version "^20.20.0 || >=22.22.0".
Got "22.13.1" (or 22.17.1 / 22.6.0)
arkor pulls in `posthog-node@^5.30.6`, whose engines field is
`^20.20.0 || >=22.22.0`. yarn classic is strict-engines by default
and refuses to extract on any Node in the gap; npm / pnpm / bun
warn-and-continue, and yarn-berry's engine check is opt-in. The
proper fix is on the arkor side (either tighten `engines.node`
above 22.22 or pin posthog-node to a Node-22.x-friendly version)
and is out of scope for this CI sweep.
Drop the yarn × pre-22.22-Node combinations rather than mask the
real incompatibility with `--ignore-engines` / `YARN_IGNORE_ENGINES`:
- exclude `yarn × Node 22.13.x` and `yarn × Node 22.17.x` from the
main matrix (3 OS × 2 Nodes = 6 jobs)
- drop the `yarn × Node 22.6.x` ubuntu sanity entry from `include`
`yarn × Node 22.21.x` happens to pass today (yarn picked an older
posthog-node copy from cache) but is on the same theoretical
failure cliff — leaving it in keeps a canary for when behaviour
changes. yarn × Node 24.12.x is the only entry in the canonical
posthog-node engines window and continues to cover the real-user
yarn-classic + arkor flow.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the CLI E2E coverage to exercise real dependency installation across multiple package managers and CI environments, while updating scaffolding so Yarn-based projects get config needed for Arkor’s runtime model. It fits into the repo’s testing/build pipeline by extending the GitHub Actions matrix and plumbing the selected package manager through scaffolding and E2E helpers.
Changes:
- Added a new CI install matrix covering multiple package managers, OSes, and selected Node.js versions, with per-runner gating via
ARKOR_E2E_PM. - Updated
arkor initandcreate-arkorflows to pass the chosen package manager into scaffolding so Yarn projects can emit.yarnrc.ymland Yarn-specific.gitignoreentries. - Expanded E2E and unit tests plus developer docs to support the new matrix and improve install-failure diagnostics.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| turbo.json | Whitelists ARKOR_E2E_PM for Turbo test task env propagation. |
| packages/create-arkor/src/bin.ts | Passes selected package manager into scaffolding before install. |
| packages/cli-internal/src/scaffold.ts | Adds pm-aware scaffold behavior for Yarn config and .gitignore updates. |
| packages/cli-internal/src/scaffold.test.ts | Adds unit coverage for Yarn-specific scaffold output. |
| packages/arkor/src/cli/commands/init.ts | Passes selected package manager into in-place scaffolding. |
| e2e/cli/src/spawn-cli.ts | Isolates Yarn cache per spawned CLI run and cleans it up. |
| e2e/cli/src/create-arkor.test.ts | Reworks install E2E cases into pm-gated matrix-style tests with diagnostics. |
| e2e/cli/src/arkor-init.test.ts | Reworks install E2E cases into pm-gated matrix-style tests with diagnostics. |
| AGENTS.md | Documents running a specific install-matrix case locally. |
| .github/workflows/ci.yaml | Adds the cross-package-manager install matrix job and pm setup logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+135
to
+142
| // Don't clobber an existing `.yarnrc.yml`. The user might already have | ||
| // PnP intentionally configured, or be pinning a different `nodeLinker` | ||
| // (`pnp-loose`, `pnpm`, …) — leaving theirs alone is safer than racing | ||
| // a default in. | ||
| const path = join(cwd, YARNRC_YML_PATH); | ||
| if (existsSync(path)) return "kept"; | ||
| await writeFile(path, YARNRC_YML_CONTENT); | ||
| return "created"; |
Conflict resolution:
- packages/cli-internal/src/scaffold.ts: keep both — main bumped
DEFAULT_ARKOR_SPEC to ^0.0.1-alpha.6, eng-603 added
YARNRC_YML_PATH for the yarn-berry support.
- packages/cli-internal/src/scaffold.test.ts: keep both — main
added a "separating newline" test for patchGitignore, eng-603
added the yarn-only .yarnrc.yml + .gitignore yarn-cache tests.
Adjusted post-merge to keep tests green:
- scaffold.test.ts "inserts a separating newline" expectation
updated to `node_modules/\ndist/\n.arkor/\n`. eng-603 generalised
patchGitignore to ensure ALL required entries are present
(previously it only patched in `.arkor/`); the separator-newline
invariant the new test covers still holds, just with two more
appended lines for the missing entries.
- cli/commands/init.test.ts "happy path" now asserts
`packageManager: "pnpm"` is forwarded to scaffold(). eng-603
threads pm into ScaffoldOptions so yarn picks up `.yarnrc.yml`.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+135
to
+142
| // Don't clobber an existing `.yarnrc.yml`. The user might already have | ||
| // PnP intentionally configured, or be pinning a different `nodeLinker` | ||
| // (`pnp-loose`, `pnpm`, …) — leaving theirs alone is safer than racing | ||
| // a default in. | ||
| const path = join(cwd, YARNRC_YML_PATH); | ||
| if (existsSync(path)) return "kept"; | ||
| await writeFile(path, YARNRC_YML_CONTENT); | ||
| return "created"; |
Comment on lines
+233
to
+237
| if (options.packageManager === "yarn") { | ||
| files.push({ | ||
| path: YARNRC_YML_PATH, | ||
| action: await patchYarnConfig(cwd), | ||
| }); |
Comment on lines
+243
to
+248
| # masking them with `--ignore-engines`. yarn × 22.21 happens | ||
| # to pass today (yarn picked an older posthog-node copy in | ||
| # cache) but is on the same theoretical failure cliff — | ||
| # leaving it in keeps a canary for when behaviour changes. | ||
| - { id: yarn, node: '>=22.13.0 <22.14.0' } | ||
| - { id: yarn, node: '>=22.17.0 <22.18.0' } |
Four review comments, three on `scaffold.ts` for the yarn-berry
compatibility flow and one on `ci.yaml`'s install-matrix exclusions.
scaffold.ts (3 comments):
- Existing `.yarnrc.yml` was previously kept verbatim. The reviewer
flagged two real failure modes that left scaffolded projects
broken under arkor:
1. Existing `.yarnrc.yml` carries unrelated yarn settings but
no `nodeLinker:` key — yarn 4 silently defaults to PnP.
2. Existing `.yarnrc.yml` explicitly pins `nodeLinker: pnp`
from a prior repo state — arkor still can't load through
PnP.
`patchYarnConfig` now patches in-place: if `nodeLinker:` is
absent, append `nodeLinker: node-modules`; if present with any
other value, rewrite to `node-modules`; if already correct,
report `ok`. Other settings are preserved verbatim.
- The yarn config + yarn-cache `.gitignore` lines were only
emitted when `packageManager === "yarn"`. The reviewer noted
that both CLIs have a real `packageManager === undefined` path
(the manual install hint, "install dependencies (npm i / pnpm
install / yarn / bun install)") — a yarn-berry user reading
that hint and running `yarn install` would land on PnP and
break the runtime. Defensively emit the yarn config + cache
lines when pm is yarn OR undefined; only skip when the user
*explicitly* picked another pm. yarn 1 ignores `.yarnrc.yml`
(reads `.yarnrc`) and other pms ignore the file entirely, so
this is harmless for non-yarn flows.
ci.yaml (1 comment):
- The `yarn × Node 22.21` install-matrix lane was kept on the
theory that it's "useful as a canary" even though posthog-node
requires `>=22.22.0`. The reviewer pointed out that hosted
runners have ephemeral caches — what passed today (because
yarn happened to reuse an older cached posthog-node) will
flake spuriously next push. Add the lane to the exclude list
alongside 22.13 / 22.17.
scaffold.test.ts: replace the now-obsolete "keeps an existing
.yarnrc.yml untouched" case with three cases covering the new
patch behaviour (append-when-missing / rewrite-when-different /
ok-when-already-correct), plus a case for the
`packageManager === undefined` flow. The two existing tests that
asserted exact post-scaffold contents (writes-all-starter-files,
inserts-separating-newline) now expect the additional yarn
artifacts that fire on the undefined-pm flow.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+32
to
+33
| * `undefined` when the caller didn't / couldn't determine a pm: no | ||
| * pm-specific files are written in that case. |
Conflict resolution:
- packages/cli-internal/src/scaffold.test.ts: keep both — main
dropped the deprecated `minimal` template (commits 7debabb /
b382ae9 of eng-525), eng-603 added the .yarnrc.yml /
yarn-cache test cases for the manual-install-hint flow that
Copilot's PR #99 review asked for.
Adjusted post-merge:
- All `template: "minimal"` literals in the eng-603-added yarn
tests are switched to `template: "triage"` since `"minimal"`
is no longer a valid TemplateId on main.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+153
to
+156
| // Policy: always insist on `nodeLinker: node-modules` when arkor is | ||
| // scaffolding into a yarn project. If the file already has a | ||
| // matching key (any value), patch it to `node-modules`; otherwise | ||
| // append the line. Other settings are preserved verbatim — we only |
Comment on lines
+32
to
+33
| * `undefined` when the caller didn't / couldn't determine a pm: no | ||
| * pm-specific files are written in that case. |
| pnpm --filter create-arkor dev # tsdown --watch on the scaffolder | ||
| pnpm --filter @arkor/e2e-cli test # E2E (slow; spawns real CLIs) | ||
| SKIP_E2E_INSTALL=1 pnpm --filter @arkor/e2e-cli test # skip `<pm> install` inside fixtures | ||
| ARKOR_E2E_PM=bun pnpm --filter @arkor/e2e-cli test # run the install-matrix case for one pm only (CI sets this per-runner; valid labels: npm / pnpm / yarn / yarn-berry / bun) |
Three follow-up review comments after the previous round: 1. **scaffold.ts:156 — "rewrites pnp → node-modules" is too aggressive.** Both `arkor init` and `create-arkor .` support scaffolding into existing directories, so silently overwriting an explicit `nodeLinker: pnp` would flip the install mode for the entire repo and break unrelated packages in a yarn-berry workspace. Walk that back: when `nodeLinker:` is missing, still APPEND `nodeLinker: node-modules` (yarn 4 would otherwise silently default to PnP and break the arkor runtime); when an explicit value is already set, leave the file `kept` and let the user reconcile arkor's runtime expectations with their own yarn config. Test renamed from "rewrites pnp" to "keeps an existing .yarnrc.yml that explicitly pins a non-node-modules linker". 2. **scaffold.ts:33 — `ScaffoldOptions.packageManager` docstring is inaccurate.** The previous round changed the behaviour so `undefined` also writes `.yarnrc.yml` and yarn-cache `.gitignore` entries (defending the manual-install-hint flow), but the docblock still claimed `undefined` skipped pm-specific files. `ScaffoldOptions` is exported from `@arkor/cli-internal`, so the wrong contract leaks to external callers — refresh the comment to describe the "undefined is treated as could-be-yarn" policy and call out which pms opt the project out (`"npm" | "pnpm" | "bun"`). 3. **AGENTS.md:38 — `ARKOR_E2E_PM=yarn-berry ...` local example doesn't actually work.** `runCli()` forces `CI=1` for every spawned CLI, so yarn 4's `enableImmutableInstalls` default kicks in and refuses the freshly scaffolded project's first lockfile (`YN0028`). `YARN_ENABLE_IMMUTABLE_INSTALLS=false` lived in ci.yaml until now, so local runs needed it threaded through manually. Move the env into spawn-cli.ts so it's set per spawn — local + CI both work without extra plumbing. Also moved `YARN_NETWORK_CONCURRENCY=1` (yarn 1 optionalDeps race guard) for consistency. The ci.yaml block is replaced with a pointer comment.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+191
to
+205
| const installCases = [ | ||
| { label: "npm", flag: "npm", localDefault: true }, | ||
| { label: "pnpm", flag: "pnpm", localDefault: true }, | ||
| { label: "yarn", flag: "yarn", localDefault: false }, | ||
| // yarn-berry shares the SDK's `--use-yarn` flag; the CI matrix swaps | ||
| // the `yarn` binary in PATH between 1.x (classic) and 4.x (berry). | ||
| { label: "yarn-berry", flag: "yarn", localDefault: false }, | ||
| { label: "bun", flag: "bun", localDefault: false }, | ||
| ] as const; | ||
|
|
||
| for (const { label, flag, localDefault } of installCases) { | ||
| const skip = | ||
| SKIP_INSTALL || | ||
| (E2E_PM !== undefined && E2E_PM !== label) || | ||
| (E2E_PM === undefined && !localDefault); |
Comment on lines
+163
to
+181
| // the file untouched and surface `kept`; the user gets to | ||
| // reconcile arkor's runtime expectations with their existing | ||
| // yarn configuration themselves. | ||
| const current = await readFile(path, "utf8"); | ||
| const lines = current.split(/\r?\n/); | ||
| // Match `nodeLinker:` at line start (yarnrc.yml is YAML, top-level | ||
| // keys live at column 0). | ||
| const nodeLinkerRe = /^nodeLinker\s*:/; | ||
| let existingValueIsCorrect = false; | ||
| let hasExplicitNodeLinker = false; | ||
| for (const line of lines) { | ||
| if (nodeLinkerRe.test(line)) { | ||
| hasExplicitNodeLinker = true; | ||
| existingValueIsCorrect = line === "nodeLinker: node-modules"; | ||
| break; | ||
| } | ||
| } | ||
| if (hasExplicitNodeLinker) { | ||
| return existingValueIsCorrect ? "ok" : "kept"; |
Comment on lines
+340
to
+352
| const installCases = [ | ||
| { label: "npm", flag: "npm", localDefault: true }, | ||
| { label: "pnpm", flag: "pnpm", localDefault: true }, | ||
| { label: "yarn", flag: "yarn", localDefault: false }, | ||
| { label: "yarn-berry", flag: "yarn", localDefault: false }, | ||
| { label: "bun", flag: "bun", localDefault: false }, | ||
| ] as const; | ||
|
|
||
| for (const { label, flag, localDefault } of installCases) { | ||
| const skip = | ||
| SKIP_INSTALL || | ||
| (E2E_PM !== undefined && E2E_PM !== label) || | ||
| (E2E_PM === undefined && !localDefault); |
…ve git command output order
Comment on lines
+359
to
+382
| // matrix, don't expected-fail it". | ||
| // | ||
| // Gate it outside the supported matrix. arkor init + bun on | ||
| // Windows is a documented non-supported combination until the | ||
| // CLI-binary-axis divergence is fixed; CI does NOT advertise | ||
| // green for it. `it.skip` keeps it visible in the vitest | ||
| // summary as a skipped case (each CI run prints a skip line), | ||
| // while removing the implicit "we test this" claim that | ||
| // `it.fails` / silent skip both made. create-arkor's bun lane | ||
| // on Windows continues to assert the lockfile invariant | ||
| // through `create-arkor.test.ts`, which IS in the supported | ||
| // matrix. Adding bun-on-Windows back to the arkor-init lane | ||
| // requires (a) fixing the spawn-shape divergence and (b) | ||
| // dropping this gate. | ||
| function isBunOnWindows(flag: string): boolean { | ||
| return flag === "bun" && process.platform === "win32"; | ||
| } | ||
| for (const { label, flag } of INSTALL_CASES) { | ||
| const testFn = | ||
| shouldSkipInstallCase(label) || isBunOnWindows(flag) ? it.skip : it; | ||
| testFn( | ||
| isBunOnWindows(flag) | ||
| ? `runs real ${label} install + git commit (NOT IN SUPPORTED MATRIX on Windows × bun — see the block comment above)` | ||
| : `runs real ${label} install + git commit (gated by SKIP_E2E_INSTALL)`, |
| - The target directory already contains files and no `--use-*` flag is set (we don't drop workspace-level config into someone else's project on a guess). | ||
| - An ancestor directory already has a `pnpm-workspace.yaml` (the parent monorepo workspace governs; creating a nested one here would shadow it and break `workspace:*` resolution above). | ||
|
|
||
| If `pnpm-workspace.yaml` already exists at the target AND pnpm is plausible (the same `--use-pnpm` / UA-detected pnpm / unresolved-detection conditions that gate fresh creation above; `--use-npm` / `--use-yarn` / `--use-bun` runs leave any existing file untouched), the scaffold patches it: it appends `esbuild: false` (or `esbuild: true` if `--allow-builds` is set) to the existing top-level `allowBuilds:` block (or creates one) without touching unrelated keys, and bows out as a no-op when esbuild is already pinned (allow or deny) or when the file uses the global scalar form `allowBuilds: false` / `allowBuilds: true`. |
| - ターゲットディレクトリーに既存ファイルがあり、`--use-*` 指定がない（推測でワークスペース設定を他人のプロジェクトに置かない）。 | ||
| - 祖先ディレクトリーに既に `pnpm-workspace.yaml` がある（親モノレポのワークスペースが支配的。ここでネストして作ると親を遮蔽し、上位の `workspace:*` 解決が壊れます）。 | ||
|
|
||
| ターゲットに `pnpm-workspace.yaml` が既にある場合、**pnpm があり得る** とき（新規作成と同じく `--use-pnpm` / UA 検出が pnpm / 未解決のいずれか。`--use-npm` / `--use-yarn` / `--use-bun` の場合は既存ファイルも触りません）のみ patch します。トップレベルの `allowBuilds:` ブロックに `esbuild: false`（`--allow-builds` が指定されている場合は `esbuild: true`）を追記（無ければブロックごと作成）し、無関係なキーは触りません。esbuild が既に明示的に値（true / false）でピン留めされていたり、ファイルが `allowBuilds: false` / `allowBuilds: true` のスカラー形（global pin）を使っている場合は何もせず no-op で終わります。 |
|
|
||
| `esbuild: false` is an explicit deny — pnpm sees a decision and silently skips the script instead of erroring. esbuild itself still works because pnpm already installs `@esbuild/<platform>` as an `optionalDependency`. yarn / npm / bun all ignore the workspace yaml. | ||
|
|
||
| If you genuinely need the postinstall to run (rare; typically a broken installer or unusual platform), pass `--allow-builds`. The flag controls only the value the scaffold *writes*: a fresh-scaffold run emits `esbuild: true` instead of `esbuild: false`, and a patch-into-existing-file run that adds a new `esbuild` entry uses `true`. An *existing* explicit pin is always preserved — if the file already has `allowBuilds.esbuild: false` (or `: true`), the scaffold leaves it untouched even when `--allow-builds` is passed. To change a prior explicit deny, edit `pnpm-workspace.yaml` by hand. |
| ); | ||
|
|
||
| onPosix( | ||
| "does NOT forward YARN_ENABLE_IMMUTABLE_INSTALLS for non-yarn package managers", |
| !installArtifactsLanded | ||
| ) { | ||
| ui.log.info(`Retry manually: ${pm} install`); | ||
| } |
Comment on lines
+513
to
+521
| if ( | ||
| installThrewError !== undefined && | ||
| !installAttemptCompleted && | ||
| !installArtifactsLanded | ||
| ) { | ||
| clack.log.info( | ||
| inPlace | ||
| ? `Retry manually: ${pm} install` | ||
| : `Retry manually: cd ${shellQuoteIfNeeded(cdTarget)} && ${pm} install`, |
Comment on lines
+421
to
+428
| for (const key of Object.keys(env)) { | ||
| if (key.toUpperCase() === "YARN_ENABLE_IMMUTABLE_INSTALLS") { | ||
| if (hasLockfile && isYarnTruthy(env[key])) { | ||
| // User explicitly opted into immutable installs in | ||
| // a context where we want to honour that decision. | ||
| continue; | ||
| } | ||
| delete env[key]; |
Comment on lines
+196
to
+206
| # Cross-pm install matrix: exercises `arkor init --use-<pm>` and | ||
| # `create-arkor --use-<pm>` against every package manager the SDK accepts, | ||
| # crossed with OS and a curated set of Node versions known to shift | ||
| # spawn / pipe / strip-types behaviour (e.g. libuv 1.51 in Node 22.17 | ||
| # changed macOS pipe-flush timing — the kind of thing that ate hours of | ||
| # debugging in eng-606). The build job above already runs `pnpm test` | ||
| # end-to-end, but its install assertions only cover the pms that a | ||
| # typical contributor has on PATH (npm + pnpm). The yarn / yarn-berry / | ||
| # bun lanes pull their runtimes in CI-only because they aren't a | ||
| # development prerequisite and we don't want `pnpm test` on a fresh | ||
| # checkout to require them. |
…eate-arkor commands, document known issue with bun on Windows
Comment on lines
+316
to
+320
| for (const [key, value] of Object.entries(saved)) { | ||
| if (value !== undefined) process.env[key] = value; | ||
| } | ||
| delete process.env.YARN_ENABLE_IMMUTABLE_INSTALLS; | ||
| } |
| for (const [key, value] of Object.entries(saved)) { | ||
| if (value !== undefined) process.env[key] = value; | ||
| } | ||
| delete process.env.YARN_ENABLE_IMMUTABLE_INSTALLS; |
Comment on lines
+363
to
+382
| // CLI-binary-axis divergence is fixed; CI does NOT advertise | ||
| // green for it. `it.skip` keeps it visible in the vitest | ||
| // summary as a skipped case (each CI run prints a skip line), | ||
| // while removing the implicit "we test this" claim that | ||
| // `it.fails` / silent skip both made. create-arkor's bun lane | ||
| // on Windows continues to assert the lockfile invariant | ||
| // through `create-arkor.test.ts`, which IS in the supported | ||
| // matrix. Adding bun-on-Windows back to the arkor-init lane | ||
| // requires (a) fixing the spawn-shape divergence and (b) | ||
| // dropping this gate. | ||
| function isBunOnWindows(flag: string): boolean { | ||
| return flag === "bun" && process.platform === "win32"; | ||
| } | ||
| for (const { label, flag } of INSTALL_CASES) { | ||
| const testFn = | ||
| shouldSkipInstallCase(label) || isBunOnWindows(flag) ? it.skip : it; | ||
| testFn( | ||
| isBunOnWindows(flag) | ||
| ? `runs real ${label} install + git commit (NOT IN SUPPORTED MATRIX on Windows × bun — see the block comment above)` | ||
| : `runs real ${label} install + git commit (gated by SKIP_E2E_INSTALL)`, |
Comment on lines
+218
to
+224
| # flag stays exposed without an OS caveat in the SDK docs because the | ||
| # divergence is treated as a bug to fix, not a permanent constraint; | ||
| # the docs/cli/init.mdx page and `--use-bun` flag description carry an | ||
| # explicit Windows note pointing here so users hitting it have signal | ||
| # that this is known. Re-enable the case (drop the `isBunOnWindows` | ||
| # arm in arkor-init.test.ts AND this comment block) when the spawn- | ||
| # shape divergence is fixed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbd2d29f8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // root, in the file's EOL. The trailing-newline normalization | ||
| // at the top of the function guarantees `contents` already | ||
| // ends with one, so we don't need to insert a separator. | ||
| return `${contents}${root}allowBuilds:${eol}${root} esbuild: ${value}${eol}`; |
There was a problem hiding this comment.
When an existing pnpm-workspace.yaml has packages: but no allowBuilds: and ends the YAML document with ..., this append path writes allowBuilds after the end marker. That turns the file into a multi-document stream, which the surrounding comments already note pnpm rejects, so arkor init --use-pnpm / create-arkor --use-pnpm can leave a previously valid workspace file unparseable. Insert the new block before a trailing document-end marker instead of blindly appending at EOF.
Useful? React with 👍 / 👎.
Comment on lines
+316
to
+319
| for (const [key, value] of Object.entries(saved)) { | ||
| if (value !== undefined) process.env[key] = value; | ||
| } | ||
| delete process.env.YARN_ENABLE_IMMUTABLE_INSTALLS; |
Comment on lines
+398
to
+401
| for (const [key, value] of Object.entries(saved)) { | ||
| if (value !== undefined) process.env[key] = value; | ||
| } | ||
| delete process.env.YARN_ENABLE_IMMUTABLE_INSTALLS; |
Comment on lines
+377
to
+379
| const testFn = | ||
| shouldSkipInstallCase(label) || isBunOnWindows(flag) ? it.skip : it; | ||
| testFn( |
Comment on lines
+218
to
+222
| # flag stays exposed without an OS caveat in the SDK docs because the | ||
| # divergence is treated as a bug to fix, not a permanent constraint; | ||
| # the docs/cli/init.mdx page and `--use-bun` flag description carry an | ||
| # explicit Windows note pointing here so users hitting it have signal | ||
| # that this is known. Re-enable the case (drop the `isBunOnWindows` |
Comment on lines
+578
to
+588
| beforeEach(() => { | ||
| dir = mkdtempSync(join(tmpdir(), "node-modules-snapshot-")); | ||
| }); | ||
| afterEach(() => { | ||
| rmSync(dir, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| it("snapshotNodeModules records no-existence when cwd has no node_modules", () => { | ||
| const snap = snapshotNodeModules(dir); | ||
| expect(snap.cwd).toEqual({ exists: false, mtimeMs: 0 }); | ||
| expect(snap.enclosing.size).toBe(0); |
Comment on lines
+660
to
+668
| it("nodeModulesChangedSince returns true when an ancestor node_modules newly appears (no prior enclosing)", () => { | ||
| const sub = join(dir, "packages", "foo"); | ||
| mkdirSync(sub, { recursive: true }); | ||
| const before: NodeModulesSnapshot = snapshotNodeModules(sub); | ||
| expect(before.cwd.exists).toBe(false); | ||
| expect(before.enclosing.size).toBe(0); | ||
| // Hoisted install creates the ancestor for the first time. | ||
| mkdirSync(join(dir, "node_modules")); | ||
| expect(nodeModulesChangedSince(sub, before)).toBe(true); |
Comment on lines
+460
to
+471
| expect(snapshotLockfile(dir, pm).exists).toBe(false); | ||
| writeFileSync(join(dir, file), ""); | ||
| const snap = snapshotLockfile(dir, pm); | ||
| expect(snap.exists).toBe(true); | ||
| expect(snap.mtimeMs).toBeGreaterThan(0); | ||
| }, | ||
| ); | ||
|
|
||
| it("snapshotLockfile does not match a different pm's lockfile (e.g. yarn.lock when pm=npm)", () => { | ||
| writeFileSync(join(dir, "yarn.lock"), ""); | ||
| expect(snapshotLockfile(dir, "npm").exists).toBe(false); | ||
| expect(snapshotLockfile(dir, "yarn").exists).toBe(true); |
Comment on lines
+469
to
+471
| writeFileSync(join(dir, "yarn.lock"), ""); | ||
| expect(snapshotLockfile(dir, "npm").exists).toBe(false); | ||
| expect(snapshotLockfile(dir, "yarn").exists).toBe(true); |
Comment on lines
+509
to
+512
| const before: LockfileSnapshot = snapshotLockfile(dir, "pnpm"); | ||
| expect(before.exists).toBe(false); | ||
| writeFileSync(join(dir, "pnpm-lock.yaml"), "lockfileVersion: 9\n"); | ||
| expect(lockfileChangedSince(dir, "pnpm", before)).toBe(true); |
Comment on lines
+155
to
+158
| if (value.startsWith("-")) { | ||
| const prefix = process.platform === "win32" ? ".\\" : "./"; | ||
| value = `${prefix}${value}`; | ||
| } |
…s-platform compatibility
| | `--use-pnpm` | Force pnpm as the package manager. | | ||
| | `--use-yarn` | Force yarn as the package manager. | | ||
| | `--use-bun` | Force bun as the package manager. | | ||
| | `--use-bun` | Force bun as the package manager. Known limitation on Windows: `arkor init --use-bun` populates `node_modules` but does not produce `bun.lock`, so the initial commit will not include a lockfile. `bun install` run directly, and `create-arkor --use-bun`, are not affected. | |
…better readability
Comment on lines
+277
to
+310
| // Per-spawn yarn cache. Vitest runs test files in parallel workers; | ||
| // when two workers each call `arkor init --use-yarn`, both yarn 1 | ||
| // processes hammer the shared `~/.cache/yarn/v6/` and race during | ||
| // tarball extraction — the inner mkdir-then-write sequence collides | ||
| // and the second loser dies with `ENOENT: ... open | ||
| // '...integrity/node_modules/<pkg>/.yarn-tarball.tgz'`. yarn 1's | ||
| // `--mutex network` would also work, but it's a flag (we'd need | ||
| // pm-aware install args in the SDK); a per-spawn `YARN_CACHE_FOLDER` | ||
| // sidesteps the issue at the env layer for free, and yarn-berry / | ||
| // npm / pnpm / bun all ignore the variable. | ||
| // Per-spawn bun install cache. Same parallel-worker race | ||
| // story as yarn above, observed concretely on Windows + CI: | ||
| // bun's tarball extraction does an | ||
| // NtSetInformationFile rename of a `.<hash>-<n>.<pkg>` | ||
| // staging dir into `<pkg>@<ver>@@@<n>`. When two vitest | ||
| // workers race the same staging dir under | ||
| // `~/.bun/install/cache/`, the rename fails with | ||
| // `ENOTEMPTY: Directory not empty (NtSetInformationFile)` → | ||
| // `error: InstallFailed extracting tarball from <pkg>` | ||
| // (CI run 25349847532, install · bun · windows-latest). | ||
| // Per-spawn `BUN_INSTALL_CACHE_DIR` isolates each worker | ||
| // (yarn / npm / pnpm all ignore the variable). | ||
| // | ||
| // Round 39 (Copilot, PR #99): the two `mkdtempSync` calls | ||
| // run sequentially. If the SECOND throws (tmpdir EACCES / | ||
| // ENOSPC / permissions race), the first dir would leak — | ||
| // the close/error listeners that fire `cleanup()` haven't | ||
| // attached yet because `spawn(...)` runs even later. Wrap | ||
| // both in a try/catch and unwind the first if the second | ||
| // mkdtemp throws. | ||
| const yarnCacheDir = mkdtempSync(join(tmpdir(), "arkor-e2e-yarn-cache-")); | ||
| let bunCacheDir: string; | ||
| try { | ||
| bunCacheDir = mkdtempSync(join(tmpdir(), "arkor-e2e-bun-cache-")); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f129d2b996
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1254
to
+1256
| function hasTopLevelPackagesKey(contents: string): boolean { | ||
| const root = detectYamlRootIndent(contents); | ||
| return new RegExp(`^${root}packages:`, "m").test(contents); |
There was a problem hiding this comment.
packages key before patching YAML
Handle UTF-8 BOM at the start of existing pnpm-workspace.yaml files before checking for packages:. With a BOM-prefixed file (common from some Windows editors), this regex misses an existing top-level packages key, so patchPnpmWorkspace() prepends a second packages: entry and shifts the BOM into the middle of the document. That can leave ambiguous/broken YAML semantics for pnpm and causes avoidable config corruption on re-scaffold.
Useful? React with 👍 / 👎.
| const indent = /^(\s*)/.exec(line)?.[1].length ?? 0; | ||
| if (rootIndent === undefined) rootIndent = indent; | ||
| if (indent !== rootIndent) continue; // nested — skip | ||
| const m = /^\s*nodeLinker\s*:\s*(.*)$/.exec(line); |
There was a problem hiding this comment.
nodeLinker key
Treat UTF-8 BOM-prefixed .yarnrc.yml content as valid when parsing nodeLinker. A file that starts with BOM and nodeLinker: node-modules is currently parsed as if no nodeLinker were present, which pushes patchYarnConfig() into the Berry-caveat/block-install path and can incorrectly skip install/git init in otherwise-correct Yarn setups.
Useful? React with 👍 / 👎.
Comment on lines
+1179
to
+1181
| `^${root}allowBuilds:[ \\t]*\\{([^}]*)\\}${TRAILING_COMMENT_AND_EOL}`, | ||
| "m", | ||
| ), |
There was a problem hiding this comment.
allowBuilds before appending another
Normalize or strip a leading UTF-8 BOM before reading top-level allowBuilds in pnpm-workspace.yaml. When the file starts with BOM and already contains allowBuilds (scalar or map), the current matcher can miss it and appendEsbuildToAllowBuilds() appends a second top-level allowBuilds: block, producing duplicate keys and potentially changing pnpm’s effective install policy unexpectedly.
Useful? React with 👍 / 👎.
… related explanations
Comment on lines
+88
to
+138
| * Quoting style is platform-aware (round-39 Codex P2 / Copilot): | ||
| * | ||
| * - POSIX (Linux / macOS): single quotes. Embedded `'` is | ||
| * escaped via the standard `'\''` close-literal-open | ||
| * sequence — the bytes `'`, `\`, `'`, `'` parse as | ||
| * end-quote, literal `'`, start-quote. | ||
| * - Windows (cmd.exe / PowerShell): double quotes with a | ||
| * MIXED escape strategy — there is no single quoting form | ||
| * that's clean for both shells, so we pick the safer | ||
| * escape per metacharacter: | ||
| * * `` ` `` (backtick) and `$`: PS-style backtick prefix | ||
| * (`` ` `` → `` `` ``, `$` → `` `$ ``). PowerShell | ||
| * interpolates `$VAR` / `$()` inside double quotes, so | ||
| * a path containing those would let a copy-pasted | ||
| * `cd "..." && <pm> install` evaluate a subexpression | ||
| * when pasted into a PS prompt (round-39 Copilot | ||
| * flagged this as the copy-paste injection vector). In | ||
| * cmd these chars are literal so the backtick-prefix | ||
| * is just a cosmetic mismatch. | ||
| * * `\` and `"`: backslash-style (`\` → `\\`, `"` → `\"`). | ||
| * These match the `_setargv` / msvcrt argv parsing | ||
| * convention used when the quoted path is forwarded to | ||
| * a child program from cmd. PS doesn't honour `\` as an | ||
| * escape inside double quotes (the literal text is | ||
| * what `Set-Location` sees), but `cd "C:\\foo\\bar"` | ||
| * still resolves to `C:\foo\bar` because NTFS | ||
| * normalizes adjacent separators. PS-style would be | ||
| * `` `" `` for embedded `"`; we keep `\"` because cmd | ||
| * users with literal `"` in a path are no rarer than | ||
| * PS users with the same, and the cmd msvcrt | ||
| * convention is what gets the path correctly through | ||
| * to programs spawned via argv. | ||
| * * `%`: NOT escaped — there is no transparent escape for | ||
| * `%VAR%` inside double quotes in interactive `cmd.exe` | ||
| * (`^%` only suppresses expansion in batch files, not | ||
| * at the prompt; `%%` becomes literal `%%` outside | ||
| * batch). PowerShell treats `%` as literal in double | ||
| * quotes, so PS users see correct hints. cmd users | ||
| * with a path like `My%Project%App` would see | ||
| * `%Project%` substituted with the env var of that | ||
| * name (or left as-is if undefined on Windows 10+). | ||
| * Same level of edge case as the other documented | ||
| * mismatches; round-39 Copilot flagged it explicitly. | ||
| * `cmd.exe` users with paths containing `` ` ``, `$`, or | ||
| * `%` see a slightly mangled but not-injection-vector hint; | ||
| * PS users with paths containing `"` see a broken-but-not- | ||
| * injection hint. Paths with these metachars are | ||
| * vanishingly rare in practice. The single-line cd-recovery | ||
| * print is a pragmatic compromise — emitting separate cmd | ||
| * / PS lines would more than double the closing summary | ||
| * length for a hazard most users will never hit. |
Comment on lines
+242
to
+269
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: [ubuntu-latest, windows-latest, macos-latest] | ||
| # Curated Node versions — the supported Node lines: the | ||
| # active LTS lines (22, 24) plus Current (26, not yet LTS | ||
| # — promotes in Oct 2026). main's engines bump | ||
| # (`packages/arkor/package.json` → `>=22.22.0`) made the | ||
| # prior pre-22.22 entries (22.13/17/21) point at | ||
| # unsupported configurations, so they've been dropped | ||
| # along with the yarn × pre-22.22 excludes that masked | ||
| # the engines mismatch. | ||
| # | ||
| # Each Node line gets two entries — exact engines floor | ||
| # (so the lowest officially supported version is actually | ||
| # exercised, not just whatever 22.22+ patch setup-node | ||
| # happens to resolve) plus the live `<minor>` range. The | ||
| # build matrix above uses the same pattern. | ||
| node: | ||
| - '22.22.0' # exact engines floor (DoS-patched 22 LTS) | ||
| - '>=22.22.0 <23' # latest 22.x | ||
| - '24.0.0' # exact 24.x floor (initial release; type-stripping default-on / Stability: RC) | ||
| - '24.12.0' # exact boundary where type stripping became Stable | ||
| - '>=24.12.0 <25' # latest 24.x | ||
| - '26.0.0' # exact 26.x floor (initial release) | ||
| - '>=26.0.0 <27' # latest 26.x | ||
| id: [pnpm-9, pnpm-10, pnpm-11, npm, yarn, yarn-berry, bun] | ||
| steps: |
…uildCdAndRun` function
| // inside double quotes (would shell-execute `arkor init`). The | ||
| // auto-commit path (Trainer.gitInitialCommit) keeps the | ||
| // backticked message — that goes via spawn argv, not a shell. | ||
| const gitCmd = '`git init && git add -A && git commit -m "Initial commit from arkor init"`'; |
| └── pnpm-workspace.yaml # OPTIONAL: pnpm 11 allowBuilds (see below) | ||
| ``` | ||
|
|
||
| `src/arkor/`, `arkor.config.ts`, `README.md`, `.gitignore`, and `package.json` are always written. The two yaml files are conditional: |
…ell execution issues
Comment on lines
+115
to
+119
| * `` `" `` for embedded `"`; we keep `\"` because cmd | ||
| * users with literal `"` in a path are no rarer than | ||
| * PS users with the same, and the cmd msvcrt | ||
| * convention is what gets the path correctly through | ||
| * to programs spawned via argv. |
Comment on lines
671
to
675
| clack.outro( | ||
| [ | ||
| `Next steps:`, | ||
| ...(inPlace ? [] : [` cd ${cdTarget}`]), | ||
| outroIntro, | ||
| ...(inPlace ? [] : [` cd ${shellQuoteIfNeeded(cdTarget)}`]), | ||
| ...(installLine ? [installLine] : []), |
Comment on lines
+444
to
+448
| // substitution (the shell tries to run the chain and capture | ||
| // its output, then exec the empty result), PowerShell treats | ||
| // \` as an escape character. Emit the command bare so even a | ||
| // verbatim copy lands cleanly. | ||
| const gitCmd = 'git init && git add -A && git commit -m "Initial commit from arkor init"'; |
Comment on lines
+483
to
+487
| const installCmd = pm ? `\`${pm} install\`` : MANUAL_INSTALL_HINT; | ||
| const steps: string[] = []; | ||
| if (!treeIsReady) steps.push(installCmd); | ||
| if (gitInitSkipped) steps.push(gitCmd); | ||
| steps.push(`\`${devCmd}\``); |
Comment on lines
+245
to
+262
| strategy: | ||
| fail-fast: false | ||
| # Full Cartesian product: 3 OS × 7 Node × 7 pm = 147 jobs. | ||
| # This is intentional. `arkor init` and `create-arkor` are | ||
| # bootstrap-time entry points; every (OS, Node, pm) cell | ||
| # exercises a different combination of `child_process.spawn` | ||
| # shell semantics (`.cmd` shims on Windows, corepack | ||
| # provisioning, yarn-berry's PnP-vs-`node-modules` linker, | ||
| # pnpm 9 / 10 / 11 lockfile and `allowBuilds` schema | ||
| # divergence, bun's lockfile + spawn-shape quirks). The | ||
| # observed divergences this matrix has caught (bun on | ||
| # Windows missing `bun.lock` under `arkor init`'s spawn | ||
| # shape; pnpm 9's `packages:` requirement; yarn 4 PnP | ||
| # default on `CI=1`) were not reproducible from a smaller | ||
| # axis-folded matrix; a single missing combination would | ||
| # have masked the bug at review time. The runtime / queue- | ||
| # time cost is accepted for that reason. | ||
| matrix: |
Comment on lines
+247
to
+286
| # Full Cartesian product: 3 OS × 7 Node × 7 pm = 147 jobs. | ||
| # This is intentional. `arkor init` and `create-arkor` are | ||
| # bootstrap-time entry points; every (OS, Node, pm) cell | ||
| # exercises a different combination of `child_process.spawn` | ||
| # shell semantics (`.cmd` shims on Windows, corepack | ||
| # provisioning, yarn-berry's PnP-vs-`node-modules` linker, | ||
| # pnpm 9 / 10 / 11 lockfile and `allowBuilds` schema | ||
| # divergence, bun's lockfile + spawn-shape quirks). The | ||
| # observed divergences this matrix has caught (bun on | ||
| # Windows missing `bun.lock` under `arkor init`'s spawn | ||
| # shape; pnpm 9's `packages:` requirement; yarn 4 PnP | ||
| # default on `CI=1`) were not reproducible from a smaller | ||
| # axis-folded matrix; a single missing combination would | ||
| # have masked the bug at review time. The runtime / queue- | ||
| # time cost is accepted for that reason. | ||
| matrix: | ||
| os: [ubuntu-latest, windows-latest, macos-latest] | ||
| # Curated Node versions: the supported Node lines, i.e. | ||
| # the active LTS lines (22, 24) plus Current (26, not | ||
| # yet LTS; promotes in Oct 2026). main's engines bump | ||
| # (`packages/arkor/package.json` → `>=22.22.0`) made the | ||
| # prior pre-22.22 entries (22.13/17/21) point at | ||
| # unsupported configurations, so they've been dropped | ||
| # along with the yarn × pre-22.22 excludes that masked | ||
| # the engines mismatch. | ||
| # | ||
| # Each Node line gets two entries — exact engines floor | ||
| # (so the lowest officially supported version is actually | ||
| # exercised, not just whatever 22.22+ patch setup-node | ||
| # happens to resolve) plus the live `<minor>` range. The | ||
| # build matrix above uses the same pattern. | ||
| node: | ||
| - '22.22.0' # exact engines floor (DoS-patched 22 LTS) | ||
| - '>=22.22.0 <23' # latest 22.x | ||
| - '24.0.0' # exact 24.x floor (initial release; type-stripping default-on / Stability: RC) | ||
| - '24.12.0' # exact boundary where type stripping became Stable | ||
| - '>=24.12.0 <25' # latest 24.x | ||
| - '26.0.0' # exact 26.x floor (initial release) | ||
| - '>=26.0.0 <27' # latest 26.x | ||
| id: [pnpm-9, pnpm-10, pnpm-11, npm, yarn, yarn-berry, bun] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a comprehensive cross-package-manager install matrix to the CI workflow, ensuring that the SDK's scaffolding and installation flows are robust across all supported package managers (npm, pnpm 9/10/11, yarn classic, yarn berry, and bun), major Node.js versions, and operating systems. The E2E test suites for both
arkor initandcreate-arkorare updated to run real install and git commit flows for each package manager, with gating logic to ensure local development remains fast and CI covers the full matrix. Diagnostic output is improved for easier debugging of install failures in CI.Key changes:
CI Workflow Improvements
install-matrixjob in.github/workflows/ci.yamlthat runs E2E install and scaffold tests across a matrix of package managers, Node.js versions, and operating systems, with careful gating and workarounds for known issues (e.g., corepack signing keys, strict engine checks, workspace protocol quirks).E2E Test Suite Enhancements
e2e/cli/src/arkor-init.test.tsande2e/cli/src/create-arkor.test.tsto dynamically select which package manager's install flow to test based on theARKOR_E2E_PMenvironment variable, set by the CI matrix or manually for local testing. Each supported package manager is exercised, but only one per CI runner, and local runs default to npm/pnpm for speed. [1] [2].each([{ pm: "npm" }, { pm: "pnpm" }])patterns with explicit loops over all supported package managers, with gating logic for both CI and local development. [1] [2]Diagnostics and Developer Experience
node_modulesis missing, the full stdout/stderr is printed to aid debugging in CI logs, rather than just failing the assertion. [1] [2]AGENTS.mdon how to run E2E install-matrix cases locally or for a specific package manager.Test Suite Cleanup