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); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+146
to
+147
| for (const warning of warnings) { | ||
| ui.log.warn(warning); |
Comment on lines
+221
to
+230
| const { files, warnings } = await scaffold({ cwd, name, template, packageManager: pm }); | ||
| spin.stop("Done"); | ||
|
|
||
| clack.note( | ||
| files.map((f) => `${f.action.padEnd(8)} ${f.path}`).join("\n"), | ||
| "Files", | ||
| ); | ||
|
|
||
| const pm = options.packageManager; | ||
| // Surface non-fatal scaffolder advisories (see arkor init for the | ||
| // mirror of this loop and the rationale). | ||
| for (const warning of warnings) { |
Two follow-up review items from PR #99 round 6 — both about test coverage on the new `warnings: string[]` channel from `scaffold()`: 1. **packages/arkor/src/cli/commands/init.ts:147** — the warning loop in `runInit` was the only place the conflicting- `nodeLinker:` notice (and any future scaffold advisory) reached the user. A regression that dropped it would silently eat the guidance. Added two cases to `init.test.ts`: - returns warnings via the scaffold mock and asserts every string surfaces verbatim, in order, through the mocked `clack.log.warn` (which `ui.log.warn` resolves to via `../prompts.ts`) - asserts `clack.log.warn` is *not* called when scaffold returns `warnings: []`, locking down the quiet-by-default contract The existing `beforeEach` was also tightened to call `vi.clearAllMocks()` — `restoreAllMocks` only undoes spies, not `vi.mock()`-created fakes, so `clack.log.warn` call lists leaked between tests once anything started asserting on them. 2. **packages/create-arkor/src/bin.ts:230** — symmetric gap on the create-arkor side, plus the lack of any focused unit test for the wider `run()` function. Added `bin.test.ts` covering the same warning-surface contract and the `packageManager` forwarding to `scaffold()` (which the install-matrix relies on to write `.yarnrc.yml` for yarn). To make `run()` testable without spawning the bin, exported it from bin.ts and gated `program.parseAsync(process.argv)` behind an `import.meta.url === pathToFileURL(argv[1]).href` entrypoint check — under vitest the module loads but commander stays out of the way.
![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: ff515e430e
ℹ️ 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".
| // argv[1] file URL — `pathToFileURL` handles Windows backslashes | ||
| // correctly where a hand-rolled `file://${argv[1]}` would not. | ||
| const argv1 = process.argv[1]; | ||
| if (argv1 && import.meta.url === pathToFileURL(argv1).href) { |
There was a problem hiding this comment.
The new entrypoint guard can skip CLI execution for normal package-manager invocations because it compares import.meta.url to pathToFileURL(process.argv[1]).href verbatim. When create-arkor is run via node_modules/.bin/create-arkor (common for npx, npm create, and script shims), process.argv[1] is typically a symlink while import.meta.url resolves to the real file, so the comparison is false and program.parseAsync(...) never runs. In that path the command exits without performing any scaffolding, which is a user-facing regression that the current tests miss because they execute the real file path directly.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 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
+364
to
+365
| const argv1 = process.argv[1]; | ||
| if (argv1 && import.meta.url === pathToFileURL(argv1).href) { |
PR #99 round 7 (Codex P1 + Copilot): the entrypoint guard added in round 6 to make `bin.ts` safe to import under vitest broke every package-manager invocation. `npm create arkor` / `pnpm create arkor` / `npx create-arkor` all run the bin via a `node_modules/.bin/<name>` shim, so `process.argv[1]` is the symlink while `import.meta.url` is the real `dist/bin.mjs` (Node's `--preserve-symlinks-main` defaults to false, resolving the main module through symlinks). A verbatim equality check returned `false`, `program.parseAsync` never ran, and the CLI silently exited doing nothing. Replace the inline check with `shouldRunAsCli(argv1, moduleUrl)`, which `realpath`s both sides before comparing — symlink and target collapse to the same canonical path. The helper falls back to returning false on `fileURLToPath` failure so non-file module URLs (vitest's transform, `data:`, …) never trigger the CLI path. `shouldRunAsCli` is exported so the new bin.test.ts cases can drive it with a real symlink fixture (`symlinkSync` in a temp dir), locking down the four scenarios: - direct invocation (argv == module file) → true - symlink → real (npm/pnpm/npx shim) → true - unrelated argv (vitest worker) → false - missing argv → false The existing tests passed because they execute the real file path directly (no shim involved); only an integration test against an actual symlink could surface the regression — Codex's review called this out explicitly.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 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
+380
to
+382
| if (shouldEmitYarnConfig) { | ||
| const yarn = await patchYarnConfig(cwd); | ||
| files.push({ path: YARNRC_YML_PATH, action: yarn.action }); |
…g-project flow PR #99 round-8 review: the `pm === undefined && isExistingProject` branch deliberately skips `patchYarnConfig()` (per round-5: don't flip the install mode of an unknown surrounding workspace), but that also dropped the `nodeLinker:` conflict warning on the floor. A user reading the manual install hint and running `yarn install` against an existing `nodeLinker: pnp` setup would still hit the same `arkor dev` runtime failure as the explicit-yarn path — just without any heads-up. Add an inspect-only `inspectYarnConfigConflict()` helper that reads the existing `.yarnrc.yml` (without mutating it) and returns the conflicting value when present. Centralise the advisory copy in `buildYarnLinkerConflictWarning()` so the patch and inspect paths emit identical wording. Wire the inspect branch into the `else if` arm so the warning surfaces regardless of which path ran. Tests cover the conflict, no-config, and already-correct cases for the new branch.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+130
to
+142
| // Yarn-specific entries follow the same emission policy as | ||
| // `.yarnrc.yml` in `scaffold()` (Copilot review on PR #99 round 5 | ||
| // pushed back on the inconsistency where gitignore added yarn | ||
| // lines while yarnrc didn't): | ||
| // - explicit `pm === "yarn"` always adds them | ||
| // - undefined pm + fresh dir adds them defensively (yarn-berry | ||
| // user reading the manual install hint may run `yarn install`) | ||
| // - undefined pm + pre-existing project DOES NOT touch them — | ||
| // a merge into an npm/pnpm/bun repo shouldn't sprinkle yarn | ||
| // residue into their `.gitignore` | ||
| const shouldEmitYarnEntries = | ||
| packageManager === "yarn" || | ||
| (packageManager === undefined && !isExistingProject); |
Comment on lines
+425
to
+439
| } else if ( | ||
| options.packageManager === undefined && | ||
| isExistingProject | ||
| ) { | ||
| // Inspect-only counterpart to the patch path above. The | ||
| // emission rules elected NOT to write `.yarnrc.yml` here (an | ||
| // unknown-pm merge into a pre-existing project mustn't flip | ||
| // the install mode of a surrounding yarn-berry workspace), but | ||
| // the manual install hint still tells the user "yarn / bun | ||
| // install" — so if they're on yarn-berry with `nodeLinker: | ||
| // pnp` already pinned, `arkor dev` will fail just the same as | ||
| // in the patch path. Read the existing config and surface the | ||
| // identical advisory without touching the file. (PR #99 | ||
| // round-8 review.) | ||
| const conflict = await inspectYarnConfigConflict(cwd); |
…g-project flow PR #99 round-9 (Copilot, scaffold.ts:142 + :439): the round-5 policy strips both the defensive `.yarnrc.yml` write AND the yarn-cache `.gitignore` lines when undefined-pm merges into an existing project (so we don't flip the install mode of an unknown surrounding workspace). But the manual install hint still says `npm i / pnpm install / yarn / bun install`, so a yarn-berry user following it would silently land on PnP (no nodeLinker) and pollute the repo with `.yarn/cache` (no gitignore entries) — and this branch currently emits no advisory in either case. Generalise the inspect helper from a "conflict only" boolean into `inspectYarnConfig()` that classifies as `ok` / `conflict` / `needs-setup`. The new `needs-setup` outcome (no `.yarnrc.yml`, or one without a `nodeLinker:` key) now triggers a single `buildYarnBerryCaveatAdvisory()` warning that bundles both fixups (yarnrc nodeLinker + gitignore yarn-cache lines), clearly scoped to yarn 4+ so non-yarn users immediately know they can ignore. Tests: replaced the now-incorrect "stays silent when no yarnrc" assertion with two new cases covering both `needs-setup` sub-states (no file / file with no nodeLinker key); the existing `conflict` and already-correct `node-modules` cases stay green.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 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
+471
to
+489
| // the install mode of a surrounding yarn-berry workspace), but | ||
| // the manual install hint still tells the user "yarn / bun | ||
| // install" — so if they're on yarn-berry with `nodeLinker: | ||
| // pnp` already pinned, `arkor dev` will fail just the same as | ||
| // in the patch path. Read the existing config and surface the | ||
| // appropriate advisory without touching the file: | ||
| // - explicit conflict (e.g. `nodeLinker: pnp`) — round 8. | ||
| // - no usable yarn config — yarn-berry would default to PnP | ||
| // and the `.gitignore` we wrote here doesn't cover the | ||
| // `.yarn/` artifacts it'd generate either. Surface the | ||
| // yarn-berry caveat covering both the yarnrc and gitignore | ||
| // fixups the user would need (round 9, addressing both the | ||
| // scaffold.ts:142 `.gitignore` and scaffold.ts:439 `.yarnrc.yml` | ||
| // review comments). | ||
| const status = await inspectYarnConfig(cwd); | ||
| if (status.kind === "conflict") { | ||
| warnings.push(buildYarnLinkerConflictWarning(status.value)); | ||
| } else if (status.kind === "needs-setup") { | ||
| warnings.push(buildYarnBerryCaveatAdvisory()); |
| # Without --force npm bails with `EEXIST: ... C:\npm\prefix\ | ||
| # yarn.cmd`. POSIX runners don't have the colliding shim so | ||
| # --force is just a no-op there. | ||
| npm install -g --force corepack@latest |
Comment on lines
+344
to
+346
| # `--force` is required on Windows to clobber the runner image's | ||
| # pre-seeded `C:\npm\prefix\yarn.cmd` (see bootstrap step). | ||
| npm install -g --force corepack@latest |
scaffold.ts:489 — the round-9 yarn-berry caveat fired on every undefined-pm + existing-project scaffold (e.g. CLI invoked via `node`/`tsx`, which doesn't set `npm_config_user_agent`), spamming pure npm/pnpm/bun projects with irrelevant noise. Gate the `needs-setup` advisory on `package.json#packageManager` declaring yarn 2+ via the corepack-style field. Conflict warning stays unconditional — an existing `.yarnrc.yml` pinned to a non-`node-modules` value is unambiguous evidence the project uses yarn-berry, regardless of whether the corepack field is set. Tests: existing yarn-berry-caveat fixtures now declare `packageManager: "yarn@4.6.0"`/`"yarn@2.4.3"`. New cases cover the silence path (no field, pnpm, npm, bun, yarn 1.x) and the conflict-without-declaration path. ci.yaml:302/346 — `corepack@latest` made the matrix non-reproducible (an upstream release could change resolution behaviour or raise the supported-Node floor and break jobs without any repository change). Pin to `corepack@0.34.7` (published 2026-04-17 with the post-rotation signing keys; engines `^20.10.0 || ^22.11.0 || >=24.0.0` cover every Node minor in this matrix), mirroring the same pinning policy the workflow already applies to pnpm/yarn/bun.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| warnings.push(buildYarnLinkerConflictWarning(status.value)); | ||
| } else if ( | ||
| status.kind === "needs-setup" && | ||
| (await existingProjectDeclaresYarnBerry(cwd)) |
Comment on lines
+193
to
+199
| let rootIndent: number | undefined; | ||
| for (const line of yarnrc.split(/\r?\n/)) { | ||
| if (!line.trim() || /^\s*#/.test(line)) continue; | ||
| 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); |
| - '>=22.17.0 <22.18.0' # libuv 1.51: macOS pipe-flush, spawn behaviour | ||
| - '>=22.21.0 <23' # 22 LTS latest | ||
| - '>=24.12.0 <25' # 24 LTS, type stripping marked Stable | ||
| id: [pnpm-9, pnpm-10, pnpm-11, npm, yarn, yarn-berry, bun] |
scaffold.ts (yarn-berry caveat gate): the round-10 `existingProjectDeclaresYarnBerry` filter was over-broad — it silenced the caveat even when an existing `.yarnrc.yml` lacked `nodeLinker:`. But yarn 1 reads `.yarnrc` (no `.yml`), so the file's mere existence is unambiguous yarn-berry evidence regardless of the corepack declaration. Split the prior `needs-setup` outcome into `no-config` and `berry-without-linker`: the latter now warns unconditionally (matching the `conflict` treatment), while `no-config` keeps the round-10 corepack gate. scaffold.ts (`readNodeLinkerValue`): YAML structural markers (`---`, `...`, `%YAML 1.2`, `%TAG …`) aren't mapping entries, but the prior anchor-on-first-content-line strategy treated them as such and pinned `rootIndent` at the wrong column. A valid `---\n nodeLinker: node-modules\n` got misclassified as needs-setup and would (in the patch path) cause a duplicate `nodeLinker` append. Skip these markers when picking the root indent. Tests: replaced the round-10 yarn-berry-caveat-with-yarnrc test to assert the caveat fires WITHOUT a packageManager declaration (round-11 expectation). Added 4 `it.each` cases covering the YAML marker forms — leading `---`, leading `---` with indented body, leading `%YAML 1.2` directive, trailing `...` end marker.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+400
to
+403
| // No `nodeLinker:` line at all — append. Keep the existing | ||
| // trailing-newline shape (mirrors patchGitignore). | ||
| const separator = current.endsWith("\n") ? "" : "\n"; | ||
| await writeFile(path, `${current}${separator}${YARNRC_YML_CONTENT}`); |
Comment on lines
+400
to
+403
| // No `nodeLinker:` line at all — append. Keep the existing | ||
| // trailing-newline shape (mirrors patchGitignore). | ||
| const separator = current.endsWith("\n") ? "" : "\n"; | ||
| await writeFile(path, `${current}${separator}${YARNRC_YML_CONTENT}`); |
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