From 2240dfd8d18b80dd05ec40d76259ca821e71a7fd Mon Sep 17 00:00:00 2001 From: Shinrai Date: Fri, 7 Aug 2026 21:42:10 -0700 Subject: [PATCH 1/9] fix: namespace dev condition to holdmytask-dev; correct + test devcheck MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The devcheck/src-dist machinery was inherited from @cldmv/slothlet but diverged in two ways that mattered for a normal module: 1. The /main export used the GENERIC `development` condition to route to src/, but the published package ships dist/ only (no src/). Any consumer running with `--conditions=development` (a common dev setting) therefore resolved @cldmv/holdmytask/main to ./src/hold-my-task.mjs, which isn't in the tarball -> ERR_MODULE_NOT_FOUND. This shipped in v1.6.2. Slothlet avoids it by namespacing its condition (`slothlet-dev`); do the same here with `holdmytask-dev` so a consumer's generic conditions can never route this package to a source tree it doesn't ship. 2. devcheck.mjs had its `!existsSync(dist)` guard commented out (so it would nag even after a build) and its installed-package guard checked only the immediate parent dir for "node_modules" — which never matches a SCOPED package (`node_modules/@cldmv/holdmytask`, parent is `@cldmv`). Restored the dist guard and fixed the check to detect a node_modules segment anywhere above the file (covers scoped + unscoped installs). Also: - Add `prepare: npm run build` so a fresh checkout's install produces dist/, keeping the package usable from a clone without setting the dev condition (build is a dependency-free ~0.25s file copy). - Route the condition through CI (ci.yml test_environment -> holdmytask-dev) and vitest (resolve/ssr conditions include holdmytask-dev) so tests still exercise src/. Verified: with dist absent, the package entry resolves to src only via holdmytask-dev; a generic `development` condition no longer does. - Add tests/DevCheck.test.vitest.mjs (7 cases) validating the guard across unbuilt-checkout, condition-set, dist-built, generic-condition, CI, scoped-install, and no-src scenarios. --- .configs/vitest.config.mjs | 15 ++++++ .github/workflows/ci.yml | 10 ++-- devcheck.mjs | 48 +++++++++++------ package.json | 3 +- tests/DevCheck.test.vitest.mjs | 95 ++++++++++++++++++++++++++++++++++ 5 files changed, 149 insertions(+), 22 deletions(-) create mode 100644 tests/DevCheck.test.vitest.mjs diff --git a/.configs/vitest.config.mjs b/.configs/vitest.config.mjs index fd25d2e..422f7b4 100644 --- a/.configs/vitest.config.mjs +++ b/.configs/vitest.config.mjs @@ -8,6 +8,21 @@ const root = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); export default defineConfig({ root, + // The package-scoped dev condition that routes `@cldmv/holdmytask/main` to `src/` + // (see the `./main` export in package.json). Tests exercise and cover the SOURCE + // tree, so the resolver must add `holdmytask-dev`. This *replaces* vite's default + // conditions, so the usual ones are kept alongside it. CI also sets + // `NODE_OPTIONS=--conditions=holdmytask-dev` (via ci.yml `test_environment`); this + // makes a bare local `npm test` resolve to src the same way without needing it. + resolve: { + conditions: ["holdmytask-dev", "module", "browser", "development|production"] + }, + ssr: { + // Vitest often routes node-environment resolution through the SSR pipeline. + resolve: { + conditions: ["holdmytask-dev", "node", "development|production"] + } + }, test: { // Fleet-wide vitest test-file convention: `*.test.vitest.mjs`. include: ["tests/**/*.test.vitest.mjs"], diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 25e77f6..c97454c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -77,7 +77,7 @@ on: description: "Environment for tests (affects NODE_ENV and NODE_OPTIONS --conditions flag)" type: string required: false - default: "development" + default: "holdmytask-dev" # ── Coverage badge ─────────────────────────────────────────────── enable_coverage_badge: description: "Run the coverage + badge-push job after CI passes" @@ -212,11 +212,11 @@ jobs: # workflow_dispatch can still opt out (set false). lts_only_matrix: ${{ github.event.inputs.lts_only_matrix != 'false' }} package_manager: ${{ github.event.inputs.package_manager || 'npm' }} - test_command: "npm test" # Use defaults: NODE_ENV=development, NODE_OPTIONS=--conditions=development - # test_command: "NODE_OPTIONS='--conditions=slothlet-dev' npm test" # Override NODE_OPTIONS only + test_command: "npm test" # Uses NODE_ENV=holdmytask-dev, NODE_OPTIONS=--conditions=holdmytask-dev (see test_environment) + # test_command: "NODE_OPTIONS='--conditions=holdmytask-dev' npm test" # Override NODE_OPTIONS only # test_command: "NODE_ENV=test npm test" # Override NODE_ENV only - # test_command: "NODE_ENV=test NODE_OPTIONS='--conditions=slothlet-dev' npm test" # Override both - test_environment: ${{ github.event.inputs.test_environment || 'development' }} # Alternative to setting in test_command + # test_command: "NODE_ENV=test NODE_OPTIONS='--conditions=holdmytask-dev' npm test" # Override both + test_environment: ${{ github.event.inputs.test_environment || 'holdmytask-dev' }} # Alternative to setting in test_command build_command: "npm run build" skip_performance_tests: false skip_matrix_tests: false diff --git a/devcheck.mjs b/devcheck.mjs index 9303010..9f9577e 100644 --- a/devcheck.mjs +++ b/devcheck.mjs @@ -8,7 +8,7 @@ * @Last modified by: Nate Hyson (Shinrai@users.noreply.github.com) * @Last modified time: 2025-11-21 14:51:16 -08:00 (1763765476) * ----- - * @Copyright: Copyright (c) 2013-2025 Catalyzed Motivation Inc. All rights reserved. + * @Copyright: Copyright (c) 2013-2026 Catalyzed Motivation Inc. All rights reserved. */ import { existsSync } from "node:fs"; @@ -18,7 +18,7 @@ import path from "node:path"; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); const srcPath = path.join(__dirname, "src"); -// const distPath = path.join(__dirname, "dist"); +const distPath = path.join(__dirname, "dist"); // Detect if we're running in a CI environment const isCI = !!( @@ -32,29 +32,45 @@ const isCI = !!( process.env.TF_BUILD // Azure DevOps ); -if (existsSync(srcPath) && !isCI) { - // if (existsSync(srcPath) && !existsSync(distPath)) { - const nodeEnv = process.env.NODE_ENV?.toLowerCase(); - const hasNodeOptions = process.env.NODE_OPTIONS?.includes("--conditions=development"); +// Only meaningful in a source checkout that hasn't been built yet: `src/` present +// but `dist/` absent. Once `dist/` exists (via `npm run build`, which the `prepare` +// script runs on install) the package entry resolves cleanly regardless, so stay +// silent. Also skip when installed as a dependency (parent dir is `node_modules`) - +// the published package ships `dist/` only, so this branch never applies there, but +// guard anyway to match the fleet convention. +// Detect a `node_modules` segment anywhere above this file - covers both scoped +// (`node_modules/@cldmv/holdmytask`) and unscoped (`node_modules/pkg`) installs. +// A simple "parent dir === node_modules" check misses scoped packages, where the +// immediate parent is the scope directory (`@cldmv`). +const isInstalledPackage = __dirname.split(path.sep).includes("node_modules"); - if (!nodeEnv || (!["dev", "development"].includes(nodeEnv) && !hasNodeOptions)) { +if (existsSync(srcPath) && !existsSync(distPath) && !isCI && !isInstalledPackage) { + // The package-scoped condition that routes `@cldmv/holdmytask/main` to `src/` + // (see the `./main` export in package.json). Namespaced (not the generic + // `development`) so a consuming app's own `--conditions=development` never + // accidentally flips this package to a source tree it doesn't ship. + const hasDevCondition = (process.env.NODE_OPTIONS || "").includes("--conditions=holdmytask-dev"); + + if (!hasDevCondition) { console.error("❌ Development environment not properly configured!"); - console.error("📁 Source folder detected but NODE_ENV/NODE_OPTIONS not set for development."); + console.error("📁 Source folder detected but the build output (dist/) is missing and"); + console.error(" NODE_OPTIONS is not set to load from src/ for development."); + console.error(""); + console.error("🔧 To fix this, either build the package once:"); + console.error(" npm run build"); console.error(""); - console.error("🔧 To fix this, run one of these commands:"); + console.error(" or develop directly against src/ by setting the condition:"); console.error(" Windows (cmd):"); - console.error(" set NODE_ENV=development"); - console.error(" set NODE_OPTIONS=--conditions=development"); + console.error(" set NODE_OPTIONS=--conditions=holdmytask-dev"); console.error(""); console.error(" Windows (PowerShell):"); - console.error(" $env:NODE_ENV='development'"); - console.error(" $env:NODE_OPTIONS='--conditions=development'"); + console.error(" $env:NODE_OPTIONS='--conditions=holdmytask-dev'"); console.error(""); console.error(" Unix/Linux/macOS:"); - console.error(" export NODE_ENV=development"); - console.error(" export NODE_OPTIONS=--conditions=development"); + console.error(" export NODE_OPTIONS=--conditions=holdmytask-dev"); console.error(""); - console.error("💡 This ensures this module loads from src/ instead of dist/ for development."); + console.error("💡 The 'holdmytask-dev' condition loads src/ instead of dist/, and is"); + console.error(" namespaced so it can't collide with a consumer's own dev conditions."); console.error("🚀 CI environments automatically skip this check."); process.exit(1); } diff --git a/package.json b/package.json index d2ed457..d12c453 100644 --- a/package.json +++ b/package.json @@ -16,7 +16,7 @@ "import": "./devcheck.mjs" }, "./main": { - "development": { + "holdmytask-dev": { "types": "./types/src/hold-my-task.d.mts", "import": "./src/hold-my-task.mjs" }, @@ -42,6 +42,7 @@ "build": "node build.mjs", "build:types": "tsc --project .configs/tsconfig.dts.jsonc", "build:ci": "npm run build:types && npm run test:types && npm run build", + "prepare": "npm run build", "precommit": "npm run build:types && npm run test:types && npm run lint && npm run test" }, "keywords": [ diff --git a/tests/DevCheck.test.vitest.mjs b/tests/DevCheck.test.vitest.mjs new file mode 100644 index 0000000..cd7a8ef --- /dev/null +++ b/tests/DevCheck.test.vitest.mjs @@ -0,0 +1,95 @@ +import { test, expect, describe, beforeAll, afterAll } from "vitest"; +import { spawnSync } from "node:child_process"; +import { mkdtempSync, mkdirSync, copyFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +// devcheck.mjs resolves `src/`/`dist/` relative to its own file location and reads +// process.env, so each case runs a COPY of it in a purpose-built fixture directory +// with a from-scratch env (only PATH), preventing the real CI environment this suite +// runs in from leaking `CI`/`GITHUB_ACTIONS` into the subprocess and skewing results. +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const devcheckSrc = path.join(repoRoot, "devcheck.mjs"); + +let tmpRoot; + +beforeAll(() => { + tmpRoot = mkdtempSync(path.join(tmpdir(), "holdmytask-devcheck-")); +}); + +afterAll(() => { + if (tmpRoot) rmSync(tmpRoot, { recursive: true, force: true }); +}); + +/** + * Materialize a fixture: a directory containing a copy of devcheck.mjs, plus + * optional src/ and dist/ subdirs, optionally nested under a node_modules/ + * path to simulate an installed package. Returns the path to the devcheck copy. + */ +let counter = 0; +function makeFixture({ src = true, dist = false, installed = false } = {}) { + const base = path.join(tmpRoot, `f${counter++}`); + const pkgDir = installed ? path.join(base, "node_modules", "@cldmv", "holdmytask") : path.join(base, "holdmytask"); + mkdirSync(pkgDir, { recursive: true }); + if (src) mkdirSync(path.join(pkgDir, "src"), { recursive: true }); + if (dist) mkdirSync(path.join(pkgDir, "dist"), { recursive: true }); + copyFileSync(devcheckSrc, path.join(pkgDir, "devcheck.mjs")); + return path.join(pkgDir, "devcheck.mjs"); +} + +function runDevcheck(fixtureOpts, env = {}) { + const devcheck = makeFixture(fixtureOpts); + // From-scratch env: only PATH (so node runs); nothing else unless explicitly set. + const result = spawnSync(process.execPath, [devcheck], { + env: { PATH: process.env.PATH, ...env }, + encoding: "utf8" + }); + return { status: result.status, stderr: result.stderr || "" }; +} + +describe("devcheck", () => { + test("advises and exits non-zero in a source checkout with no dist and no dev condition", () => { + const { status, stderr } = runDevcheck({ src: true, dist: false }); + expect(status).toBe(1); + expect(stderr).toContain("Development environment not properly configured"); + expect(stderr).toContain("--conditions=holdmytask-dev"); + }); + + test("stays silent when the holdmytask-dev condition is set", () => { + const { status, stderr } = runDevcheck({ src: true, dist: false }, { NODE_OPTIONS: "--conditions=holdmytask-dev" }); + expect(status).toBe(0); + expect(stderr).toBe(""); + }); + + test("stays silent once dist/ has been built (even without the condition)", () => { + const { status, stderr } = runDevcheck({ src: true, dist: true }); + expect(status).toBe(0); + expect(stderr).toBe(""); + }); + + test("does NOT fire when a generic development condition is set (namespacing)", () => { + // The old generic `development` condition must no longer satisfy the check - + // otherwise a consumer's dev settings would mask a genuinely unbuilt checkout. + const { status } = runDevcheck({ src: true, dist: false }, { NODE_OPTIONS: "--conditions=development" }); + expect(status).toBe(1); + }); + + test("skips in CI", () => { + const { status, stderr } = runDevcheck({ src: true, dist: false }, { CI: "true" }); + expect(status).toBe(0); + expect(stderr).toBe(""); + }); + + test("skips when installed as a dependency (parent dir is node_modules)", () => { + const { status, stderr } = runDevcheck({ src: true, dist: false, installed: true }); + expect(status).toBe(0); + expect(stderr).toBe(""); + }); + + test("does nothing when there is no src/ (published dist-only layout)", () => { + const { status, stderr } = runDevcheck({ src: false, dist: true }); + expect(status).toBe(0); + expect(stderr).toBe(""); + }); +}); From c6427b16c111a7b5e41f24defdb724e0e58c4b28 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Fri, 7 Aug 2026 22:41:34 -0700 Subject: [PATCH 2/9] refactor(devcheck): align to the @cldmv/uuid reference pattern Follow-up to the initial commit, correcting it against the established normal-module devcheck fix in @cldmv/uuid (the repo that first solved the generic-condition collision by namespacing to `uuid-dev`). - devcheck.mjs: the warning is INTENTIONAL whenever src/ is present and the dev condition isn't set - a built checkout has both src/ and dist/, and the developer should be running from src/ via the condition, so flagging that they're silently on dist/ is the point. Removed the `!existsSync(dist)` guard added in the previous commit (which wrongly silenced it after a build). Also removed the node_modules/installed-package guard: the published package ships neither src/ nor devcheck.mjs, so index.mjs's `import("./devcheck.mjs")` just fails and is ignored - it never runs for consumers, so there's nothing to guard. Guard logic now mirrors uuid. - Dropped the `prepare: npm run build` script - not part of the reference pattern (uuid has none); the nag model expects you to build or set the condition, not auto-build on install. - vitest config: carry the condition into forked workers via `test.nodeOptions` + `test.env.NODE_ENV` (mirrors uuid) rather than relying on resolve/ssr conditions alone. - Reverted the ci.yml `test_environment` change - uuid leaves it at the reusable-workflow default and lets the vitest config carry the condition. - Updated DevCheck tests to the corrected behavior (notably: STILL nags when dist/ is present but the condition is unset). --- .configs/vitest.config.mjs | 11 ++++++-- .github/workflows/ci.yml | 10 +++---- devcheck.mjs | 48 +++++++++++++------------------- package.json | 1 - tests/DevCheck.test.vitest.mjs | 51 +++++++++++++++------------------- 5 files changed, 55 insertions(+), 66 deletions(-) diff --git a/.configs/vitest.config.mjs b/.configs/vitest.config.mjs index 422f7b4..e58abbb 100644 --- a/.configs/vitest.config.mjs +++ b/.configs/vitest.config.mjs @@ -11,9 +11,10 @@ export default defineConfig({ // The package-scoped dev condition that routes `@cldmv/holdmytask/main` to `src/` // (see the `./main` export in package.json). Tests exercise and cover the SOURCE // tree, so the resolver must add `holdmytask-dev`. This *replaces* vite's default - // conditions, so the usual ones are kept alongside it. CI also sets - // `NODE_OPTIONS=--conditions=holdmytask-dev` (via ci.yml `test_environment`); this - // makes a bare local `npm test` resolve to src the same way without needing it. + // conditions, so the usual ones are kept alongside it. `test.nodeOptions`/`test.env` + // below carry the same condition into forked test workers (for native imports of + // the package entry, e.g. CommonAliases importing index.mjs -> /main), so a bare + // local `npm test` resolves to src the same way CI does. Mirrors @cldmv/uuid. resolve: { conditions: ["holdmytask-dev", "module", "browser", "development|production"] }, @@ -30,6 +31,10 @@ export default defineConfig({ environment: "node", globals: true, testTimeout: 30000, + nodeOptions: ["--conditions=holdmytask-dev"], + env: { + NODE_ENV: "holdmytask-dev" + }, // "dot" keeps CI logs to one character per test file instead of a full // "RUN vX.Y.Z" + per-file pass/fail block for every file — vitest's // non-interactive fallback (no TTY to redraw) otherwise reprints that diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c97454c..25e77f6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -77,7 +77,7 @@ on: description: "Environment for tests (affects NODE_ENV and NODE_OPTIONS --conditions flag)" type: string required: false - default: "holdmytask-dev" + default: "development" # ── Coverage badge ─────────────────────────────────────────────── enable_coverage_badge: description: "Run the coverage + badge-push job after CI passes" @@ -212,11 +212,11 @@ jobs: # workflow_dispatch can still opt out (set false). lts_only_matrix: ${{ github.event.inputs.lts_only_matrix != 'false' }} package_manager: ${{ github.event.inputs.package_manager || 'npm' }} - test_command: "npm test" # Uses NODE_ENV=holdmytask-dev, NODE_OPTIONS=--conditions=holdmytask-dev (see test_environment) - # test_command: "NODE_OPTIONS='--conditions=holdmytask-dev' npm test" # Override NODE_OPTIONS only + test_command: "npm test" # Use defaults: NODE_ENV=development, NODE_OPTIONS=--conditions=development + # test_command: "NODE_OPTIONS='--conditions=slothlet-dev' npm test" # Override NODE_OPTIONS only # test_command: "NODE_ENV=test npm test" # Override NODE_ENV only - # test_command: "NODE_ENV=test NODE_OPTIONS='--conditions=holdmytask-dev' npm test" # Override both - test_environment: ${{ github.event.inputs.test_environment || 'holdmytask-dev' }} # Alternative to setting in test_command + # test_command: "NODE_ENV=test NODE_OPTIONS='--conditions=slothlet-dev' npm test" # Override both + test_environment: ${{ github.event.inputs.test_environment || 'development' }} # Alternative to setting in test_command build_command: "npm run build" skip_performance_tests: false skip_matrix_tests: false diff --git a/devcheck.mjs b/devcheck.mjs index 9f9577e..cf82729 100644 --- a/devcheck.mjs +++ b/devcheck.mjs @@ -18,7 +18,6 @@ import path from "node:path"; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); const srcPath = path.join(__dirname, "src"); -const distPath = path.join(__dirname, "dist"); // Detect if we're running in a CI environment const isCI = !!( @@ -32,45 +31,38 @@ const isCI = !!( process.env.TF_BUILD // Azure DevOps ); -// Only meaningful in a source checkout that hasn't been built yet: `src/` present -// but `dist/` absent. Once `dist/` exists (via `npm run build`, which the `prepare` -// script runs on install) the package entry resolves cleanly regardless, so stay -// silent. Also skip when installed as a dependency (parent dir is `node_modules`) - -// the published package ships `dist/` only, so this branch never applies there, but -// guard anyway to match the fleet convention. -// Detect a `node_modules` segment anywhere above this file - covers both scoped -// (`node_modules/@cldmv/holdmytask`) and unscoped (`node_modules/pkg`) installs. -// A simple "parent dir === node_modules" check misses scoped packages, where the -// immediate parent is the scope directory (`@cldmv`). -const isInstalledPackage = __dirname.split(path.sep).includes("node_modules"); +// Only runs in a source checkout. `src/` is present here but is NOT shipped in the +// published package, and neither is this file - in distribution index.mjs's +// `import("./devcheck.mjs")` simply fails and is ignored, so this never fires for +// consumers. When `src/` IS present the developer should be loading from it via the +// `holdmytask-dev` condition; if that isn't set they're silently running the built +// `dist/` copy instead, so warn (even after a build - that is the point). +if (existsSync(srcPath) && !isCI) { + const nodeEnv = process.env.NODE_ENV?.toLowerCase(); + // Namespaced (not the generic `development`) so a consuming app's own + // `--conditions=development` can't accidentally flip this package to a source + // tree it doesn't ship. See the `./main` export in package.json. + const hasHoldMyTaskDev = process.env.NODE_OPTIONS?.includes("--conditions=holdmytask-dev"); -if (existsSync(srcPath) && !existsSync(distPath) && !isCI && !isInstalledPackage) { - // The package-scoped condition that routes `@cldmv/holdmytask/main` to `src/` - // (see the `./main` export in package.json). Namespaced (not the generic - // `development`) so a consuming app's own `--conditions=development` never - // accidentally flips this package to a source tree it doesn't ship. - const hasDevCondition = (process.env.NODE_OPTIONS || "").includes("--conditions=holdmytask-dev"); - - if (!hasDevCondition) { + if (!nodeEnv || (!["", "development"].includes(nodeEnv) && !hasHoldMyTaskDev)) { console.error("❌ Development environment not properly configured!"); - console.error("📁 Source folder detected but the build output (dist/) is missing and"); - console.error(" NODE_OPTIONS is not set to load from src/ for development."); - console.error(""); - console.error("🔧 To fix this, either build the package once:"); - console.error(" npm run build"); + console.error("📁 Source folder detected but NODE_ENV/NODE_OPTIONS not set for holdmytask development."); console.error(""); - console.error(" or develop directly against src/ by setting the condition:"); + console.error("🔧 To fix this, run one of these commands:"); console.error(" Windows (cmd):"); + console.error(" set NODE_ENV=development"); console.error(" set NODE_OPTIONS=--conditions=holdmytask-dev"); console.error(""); console.error(" Windows (PowerShell):"); + console.error(" $env:NODE_ENV='development'"); console.error(" $env:NODE_OPTIONS='--conditions=holdmytask-dev'"); console.error(""); console.error(" Unix/Linux/macOS:"); + console.error(" export NODE_ENV=development"); console.error(" export NODE_OPTIONS=--conditions=holdmytask-dev"); console.error(""); - console.error("💡 The 'holdmytask-dev' condition loads src/ instead of dist/, and is"); - console.error(" namespaced so it can't collide with a consumer's own dev conditions."); + console.error("💡 This ensures holdmytask loads from src/ instead of dist/ for development."); + console.error("🔧 Using 'holdmytask-dev' prevents conflicts with consumer development settings."); console.error("🚀 CI environments automatically skip this check."); process.exit(1); } diff --git a/package.json b/package.json index d12c453..55b9c35 100644 --- a/package.json +++ b/package.json @@ -42,7 +42,6 @@ "build": "node build.mjs", "build:types": "tsc --project .configs/tsconfig.dts.jsonc", "build:ci": "npm run build:types && npm run test:types && npm run build", - "prepare": "npm run build", "precommit": "npm run build:types && npm run test:types && npm run lint && npm run test" }, "keywords": [ diff --git a/tests/DevCheck.test.vitest.mjs b/tests/DevCheck.test.vitest.mjs index cd7a8ef..3b43793 100644 --- a/tests/DevCheck.test.vitest.mjs +++ b/tests/DevCheck.test.vitest.mjs @@ -5,14 +5,15 @@ import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; -// devcheck.mjs resolves `src/`/`dist/` relative to its own file location and reads +// devcheck.mjs resolves `src/` relative to its own file location and reads // process.env, so each case runs a COPY of it in a purpose-built fixture directory // with a from-scratch env (only PATH), preventing the real CI environment this suite -// runs in from leaking `CI`/`GITHUB_ACTIONS` into the subprocess and skewing results. +// runs in from leaking `CI`/`GITHUB_ACTIONS`/`NODE_OPTIONS` into the subprocess. const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const devcheckSrc = path.join(repoRoot, "devcheck.mjs"); let tmpRoot; +let counter = 0; beforeAll(() => { tmpRoot = mkdtempSync(path.join(tmpdir(), "holdmytask-devcheck-")); @@ -22,15 +23,9 @@ afterAll(() => { if (tmpRoot) rmSync(tmpRoot, { recursive: true, force: true }); }); -/** - * Materialize a fixture: a directory containing a copy of devcheck.mjs, plus - * optional src/ and dist/ subdirs, optionally nested under a node_modules/ - * path to simulate an installed package. Returns the path to the devcheck copy. - */ -let counter = 0; -function makeFixture({ src = true, dist = false, installed = false } = {}) { - const base = path.join(tmpRoot, `f${counter++}`); - const pkgDir = installed ? path.join(base, "node_modules", "@cldmv", "holdmytask") : path.join(base, "holdmytask"); +// Materialize a fixture dir with a copy of devcheck.mjs plus optional src/ and dist/. +function makeFixture({ src = true, dist = false } = {}) { + const pkgDir = path.join(tmpRoot, `f${counter++}`); mkdirSync(pkgDir, { recursive: true }); if (src) mkdirSync(path.join(pkgDir, "src"), { recursive: true }); if (dist) mkdirSync(path.join(pkgDir, "dist"), { recursive: true }); @@ -40,7 +35,6 @@ function makeFixture({ src = true, dist = false, installed = false } = {}) { function runDevcheck(fixtureOpts, env = {}) { const devcheck = makeFixture(fixtureOpts); - // From-scratch env: only PATH (so node runs); nothing else unless explicitly set. const result = spawnSync(process.execPath, [devcheck], { env: { PATH: process.env.PATH, ...env }, encoding: "utf8" @@ -49,46 +43,45 @@ function runDevcheck(fixtureOpts, env = {}) { } describe("devcheck", () => { - test("advises and exits non-zero in a source checkout with no dist and no dev condition", () => { - const { status, stderr } = runDevcheck({ src: true, dist: false }); + test("nags in a source checkout when neither NODE_ENV nor the dev condition is set", () => { + const { status, stderr } = runDevcheck({ src: true }, { NODE_ENV: "production" }); expect(status).toBe(1); expect(stderr).toContain("Development environment not properly configured"); expect(stderr).toContain("--conditions=holdmytask-dev"); }); - test("stays silent when the holdmytask-dev condition is set", () => { - const { status, stderr } = runDevcheck({ src: true, dist: false }, { NODE_OPTIONS: "--conditions=holdmytask-dev" }); + test("stays silent with the holdmytask-dev condition set (even when NODE_ENV isn't development)", () => { + const { status, stderr } = runDevcheck({ src: true }, { NODE_ENV: "production", NODE_OPTIONS: "--conditions=holdmytask-dev" }); expect(status).toBe(0); expect(stderr).toBe(""); }); - test("stays silent once dist/ has been built (even without the condition)", () => { - const { status, stderr } = runDevcheck({ src: true, dist: true }); + test("stays silent with NODE_ENV=development", () => { + const { status, stderr } = runDevcheck({ src: true }, { NODE_ENV: "development" }); expect(status).toBe(0); expect(stderr).toBe(""); }); - test("does NOT fire when a generic development condition is set (namespacing)", () => { - // The old generic `development` condition must no longer satisfy the check - - // otherwise a consumer's dev settings would mask a genuinely unbuilt checkout. - const { status } = runDevcheck({ src: true, dist: false }, { NODE_OPTIONS: "--conditions=development" }); + test("STILL nags when dist/ has been built but the dev condition is not set", () => { + // The presence of a build must NOT silence the check: with src/ present the + // developer should be running from src/ via the condition, not the stale dist/. + const { status } = runDevcheck({ src: true, dist: true }, { NODE_ENV: "production" }); expect(status).toBe(1); }); - test("skips in CI", () => { - const { status, stderr } = runDevcheck({ src: true, dist: false }, { CI: "true" }); - expect(status).toBe(0); - expect(stderr).toBe(""); + test("does NOT accept a generic development condition (namespacing)", () => { + const { status } = runDevcheck({ src: true }, { NODE_ENV: "production", NODE_OPTIONS: "--conditions=development" }); + expect(status).toBe(1); }); - test("skips when installed as a dependency (parent dir is node_modules)", () => { - const { status, stderr } = runDevcheck({ src: true, dist: false, installed: true }); + test("skips in CI", () => { + const { status, stderr } = runDevcheck({ src: true }, { NODE_ENV: "production", CI: "true" }); expect(status).toBe(0); expect(stderr).toBe(""); }); test("does nothing when there is no src/ (published dist-only layout)", () => { - const { status, stderr } = runDevcheck({ src: false, dist: true }); + const { status, stderr } = runDevcheck({ src: false, dist: true }, { NODE_ENV: "production" }); expect(status).toBe(0); expect(stderr).toBe(""); }); From d2c689684446297ac34766e47af8851f3b2b1005 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Fri, 7 Aug 2026 22:58:03 -0700 Subject: [PATCH 3/9] fix(devcheck): keep condition-only trigger + detect execArgv; drop uuid's NODE_ENV logic The previous commit over-corrected by mirroring @cldmv/uuid's devcheck verbatim, which regressed the trigger to uuid's NODE_ENV-coupled form. That form is wrong for what devcheck detects, because ONLY the `--conditions=holdmytask-dev` condition selects src/ (NODE_ENV does not): - NODE_ENV=development with no condition -> uuid stays silent, but the package is actually resolving to dist/ (false negative - the exact case to catch). - condition set but NODE_ENV unset -> uuid nags even though you're correctly on src/ (false positive). Restore the condition-only trigger, and additionally detect the condition in process.execArgv, not just NODE_OPTIONS: node accepts `--conditions=` on the CLI (landing in execArgv) and that's how vitest passes it to workers - a probe showed NODE_OPTIONS is undefined in a worker while execArgv carries the flag, so the NODE_OPTIONS-only check (both mine originally and uuid's) would have spuriously fired inside the CommonAliases entry-import test and only avoided it by racing devcheck's fire-and-forget import. Checking both makes it correct and deterministic. Kept from the reference direction: the nag stays on after a build (no `!existsSync(dist)` guard). Kept my own additions uuid lacks: the scoped-aware node_modules install guard (protects git/tarball-install consumers) and the DevCheck test suite (now 9 cases, incl. execArgv form, NODE_ENV-doesn't-silence, still-nags-after-build, and scoped-install skip). --- devcheck.mjs | 53 +++++++++++++++++----------- tests/DevCheck.test.vitest.mjs | 63 ++++++++++++++++++++++------------ 2 files changed, 74 insertions(+), 42 deletions(-) diff --git a/devcheck.mjs b/devcheck.mjs index cf82729..4ceacf4 100644 --- a/devcheck.mjs +++ b/devcheck.mjs @@ -31,38 +31,51 @@ const isCI = !!( process.env.TF_BUILD // Azure DevOps ); -// Only runs in a source checkout. `src/` is present here but is NOT shipped in the -// published package, and neither is this file - in distribution index.mjs's -// `import("./devcheck.mjs")` simply fails and is ignored, so this never fires for -// consumers. When `src/` IS present the developer should be loading from it via the -// `holdmytask-dev` condition; if that isn't set they're silently running the built -// `dist/` copy instead, so warn (even after a build - that is the point). -if (existsSync(srcPath) && !isCI) { - const nodeEnv = process.env.NODE_ENV?.toLowerCase(); - // Namespaced (not the generic `development`) so a consuming app's own - // `--conditions=development` can't accidentally flip this package to a source - // tree it doesn't ship. See the `./main` export in package.json. - const hasHoldMyTaskDev = process.env.NODE_OPTIONS?.includes("--conditions=holdmytask-dev"); +// Skip when installed as a dependency (a `node_modules` segment anywhere above this +// file - covers scoped `node_modules/@cldmv/holdmytask` and unscoped installs). The +// npm-published package ships neither `src/` nor this file, so this branch is already +// moot there; but a git/tarball install DOES include them, and without this guard +// devcheck would `process.exit(1)` inside a consumer's app. A "parent dir === +// node_modules" check would miss scoped packages (parent is the scope dir). +const isInstalledPackage = __dirname.split(path.sep).includes("node_modules"); - if (!nodeEnv || (!["", "development"].includes(nodeEnv) && !hasHoldMyTaskDev)) { +// Only meaningful in a source checkout. When `src/` is present the developer should be +// loading from it via the `holdmytask-dev` condition; if that condition isn't set they +// are silently running the built `dist/` copy instead, so warn - even after a build, +// since a built checkout has BOTH src/ and dist/ and the condition is the only thing +// that selects src/. +if (existsSync(srcPath) && !isCI && !isInstalledPackage) { + // The condition selects src/ (see the `./main` export in package.json). It can be + // supplied via NODE_OPTIONS (`NODE_OPTIONS=--conditions=holdmytask-dev`) OR directly + // on the node CLI (`node --conditions=holdmytask-dev`), which lands in execArgv - + // this is how vitest passes it to workers - so check both. Namespaced (not the + // generic `development`) so a consuming app's own `--conditions=development` can't + // flip this package to a source tree it doesn't ship. NODE_ENV is deliberately NOT + // consulted: it does not affect which tree resolves, so keying off it would both + // miss the real problem (dev env set, condition absent -> silently on dist/) and + // false-alarm (condition set, dev env absent -> actually fine). + const flags = (process.env.NODE_OPTIONS || "") + " " + process.execArgv.join(" "); + const hasHoldMyTaskDev = flags.includes("holdmytask-dev"); + + if (!hasHoldMyTaskDev) { console.error("❌ Development environment not properly configured!"); - console.error("📁 Source folder detected but NODE_ENV/NODE_OPTIONS not set for holdmytask development."); + console.error("📁 Source folder detected but the 'holdmytask-dev' condition is not set,"); + console.error(" so holdmytask is loading from dist/ instead of src/."); console.error(""); - console.error("🔧 To fix this, run one of these commands:"); + console.error("🔧 To load from src/ for development, set the condition:"); console.error(" Windows (cmd):"); - console.error(" set NODE_ENV=development"); console.error(" set NODE_OPTIONS=--conditions=holdmytask-dev"); console.error(""); console.error(" Windows (PowerShell):"); - console.error(" $env:NODE_ENV='development'"); console.error(" $env:NODE_OPTIONS='--conditions=holdmytask-dev'"); console.error(""); console.error(" Unix/Linux/macOS:"); - console.error(" export NODE_ENV=development"); console.error(" export NODE_OPTIONS=--conditions=holdmytask-dev"); console.error(""); - console.error("💡 This ensures holdmytask loads from src/ instead of dist/ for development."); - console.error("🔧 Using 'holdmytask-dev' prevents conflicts with consumer development settings."); + console.error(" ...or pass it directly: node --conditions=holdmytask-dev "); + console.error(""); + console.error("💡 'holdmytask-dev' is namespaced so it can't conflict with a consumer's"); + console.error(" own development conditions."); console.error("🚀 CI environments automatically skip this check."); process.exit(1); } diff --git a/tests/DevCheck.test.vitest.mjs b/tests/DevCheck.test.vitest.mjs index 3b43793..edeaf3c 100644 --- a/tests/DevCheck.test.vitest.mjs +++ b/tests/DevCheck.test.vitest.mjs @@ -5,10 +5,11 @@ import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; -// devcheck.mjs resolves `src/` relative to its own file location and reads -// process.env, so each case runs a COPY of it in a purpose-built fixture directory -// with a from-scratch env (only PATH), preventing the real CI environment this suite -// runs in from leaking `CI`/`GITHUB_ACTIONS`/`NODE_OPTIONS` into the subprocess. +// devcheck.mjs resolves `src/` relative to its own file location and reads process.env +// / process.execArgv, so each case runs a COPY of it in a purpose-built fixture +// directory with a from-scratch env (only PATH), preventing the real CI environment +// this suite runs in from leaking `CI`/`GITHUB_ACTIONS`/`NODE_OPTIONS` into the +// subprocess. const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const devcheckSrc = path.join(repoRoot, "devcheck.mjs"); @@ -23,9 +24,11 @@ afterAll(() => { if (tmpRoot) rmSync(tmpRoot, { recursive: true, force: true }); }); -// Materialize a fixture dir with a copy of devcheck.mjs plus optional src/ and dist/. -function makeFixture({ src = true, dist = false } = {}) { - const pkgDir = path.join(tmpRoot, `f${counter++}`); +// Materialize a fixture dir with a copy of devcheck.mjs plus optional src/ and dist/, +// optionally nested under node_modules// to simulate an installed package. +function makeFixture({ src = true, dist = false, installed = false } = {}) { + const base = path.join(tmpRoot, `f${counter++}`); + const pkgDir = installed ? path.join(base, "node_modules", "@cldmv", "holdmytask") : path.join(base, "holdmytask"); mkdirSync(pkgDir, { recursive: true }); if (src) mkdirSync(path.join(pkgDir, "src"), { recursive: true }); if (dist) mkdirSync(path.join(pkgDir, "dist"), { recursive: true }); @@ -33,9 +36,11 @@ function makeFixture({ src = true, dist = false } = {}) { return path.join(pkgDir, "devcheck.mjs"); } -function runDevcheck(fixtureOpts, env = {}) { +// nodeArgs are passed on the node CLI (i.e. become process.execArgv); env is a +// from-scratch environment (only PATH plus whatever is given). +function runDevcheck(fixtureOpts, { env = {}, nodeArgs = [] } = {}) { const devcheck = makeFixture(fixtureOpts); - const result = spawnSync(process.execPath, [devcheck], { + const result = spawnSync(process.execPath, [...nodeArgs, devcheck], { env: { PATH: process.env.PATH, ...env }, encoding: "utf8" }); @@ -43,45 +48,59 @@ function runDevcheck(fixtureOpts, env = {}) { } describe("devcheck", () => { - test("nags in a source checkout when neither NODE_ENV nor the dev condition is set", () => { - const { status, stderr } = runDevcheck({ src: true }, { NODE_ENV: "production" }); + test("nags in a source checkout when the holdmytask-dev condition is not set", () => { + const { status, stderr } = runDevcheck({ src: true }); expect(status).toBe(1); expect(stderr).toContain("Development environment not properly configured"); expect(stderr).toContain("--conditions=holdmytask-dev"); }); - test("stays silent with the holdmytask-dev condition set (even when NODE_ENV isn't development)", () => { - const { status, stderr } = runDevcheck({ src: true }, { NODE_ENV: "production", NODE_OPTIONS: "--conditions=holdmytask-dev" }); + test("stays silent when the condition is set via NODE_OPTIONS", () => { + const { status, stderr } = runDevcheck({ src: true }, { env: { NODE_OPTIONS: "--conditions=holdmytask-dev" } }); expect(status).toBe(0); expect(stderr).toBe(""); }); - test("stays silent with NODE_ENV=development", () => { - const { status, stderr } = runDevcheck({ src: true }, { NODE_ENV: "development" }); + test("stays silent when the condition is passed on the node CLI (execArgv)", () => { + // vitest passes --conditions to workers this way, so devcheck must detect it here too. + const { status, stderr } = runDevcheck({ src: true }, { nodeArgs: ["--conditions=holdmytask-dev"] }); expect(status).toBe(0); expect(stderr).toBe(""); }); - test("STILL nags when dist/ has been built but the dev condition is not set", () => { - // The presence of a build must NOT silence the check: with src/ present the - // developer should be running from src/ via the condition, not the stale dist/. - const { status } = runDevcheck({ src: true, dist: true }, { NODE_ENV: "production" }); + test("NODE_ENV=development alone does NOT silence it (only the condition selects src/)", () => { + // Keying off NODE_ENV would be a false negative: dev env set but no condition means + // the package is still resolving to dist/, which is exactly what should be flagged. + const { status } = runDevcheck({ src: true }, { env: { NODE_ENV: "development" } }); + expect(status).toBe(1); + }); + + test("STILL nags when dist/ has been built but the condition is not set", () => { + // A build must NOT silence the check: with src/ present the developer should be on + // src/ via the condition, not the stale dist/. + const { status } = runDevcheck({ src: true, dist: true }); expect(status).toBe(1); }); test("does NOT accept a generic development condition (namespacing)", () => { - const { status } = runDevcheck({ src: true }, { NODE_ENV: "production", NODE_OPTIONS: "--conditions=development" }); + const { status } = runDevcheck({ src: true }, { nodeArgs: ["--conditions=development"] }); expect(status).toBe(1); }); test("skips in CI", () => { - const { status, stderr } = runDevcheck({ src: true }, { NODE_ENV: "production", CI: "true" }); + const { status, stderr } = runDevcheck({ src: true }, { env: { CI: "true" } }); + expect(status).toBe(0); + expect(stderr).toBe(""); + }); + + test("skips when installed as a scoped dependency (node_modules/@cldmv/holdmytask)", () => { + const { status, stderr } = runDevcheck({ src: true, installed: true }); expect(status).toBe(0); expect(stderr).toBe(""); }); test("does nothing when there is no src/ (published dist-only layout)", () => { - const { status, stderr } = runDevcheck({ src: false, dist: true }, { NODE_ENV: "production" }); + const { status, stderr } = runDevcheck({ src: false, dist: true }); expect(status).toBe(0); expect(stderr).toBe(""); }); From 2af4c558d38efa1d4317511b8099e0cd8554677f Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 09:52:32 -0700 Subject: [PATCH 4/9] fix(review): exact --conditions match, drop NODE_ENV override, keep module in ssr conditions Addresses PR #12 Copilot review: - devcheck.mjs: parse the actual `--conditions` values (from execArgv and NODE_OPTIONS, handling `=`/space/`-C`/comma forms) and match `holdmytask-dev` EXACTLY, instead of a substring `.includes()` that would false-positive on e.g. `--conditions=not-holdmytask-dev`. (Same fix as CLDMV/uuid#10.) Added regression tests: rejects a substring-containing condition; accepts holdmytask-dev among comma-separated conditions. - .configs/vitest.config.mjs: removed the `test.env.NODE_ENV=holdmytask-dev` override - it doesn't select the conditional export (that's `--conditions`, carried via nodeOptions) and forcing a non-standard NODE_ENV can confuse deps keying off test/development/production. Added `module` to ssr.resolve.conditions so a dependency's `module`-keyed export resolves the same under Vitest's SSR pipeline as in the non-SSR resolver. --- .configs/vitest.config.mjs | 14 +++++++++----- devcheck.mjs | 28 +++++++++++++++++++++------- tests/DevCheck.test.vitest.mjs | 11 +++++++++++ 3 files changed, 41 insertions(+), 12 deletions(-) diff --git a/.configs/vitest.config.mjs b/.configs/vitest.config.mjs index e58abbb..a434a8e 100644 --- a/.configs/vitest.config.mjs +++ b/.configs/vitest.config.mjs @@ -19,9 +19,11 @@ export default defineConfig({ conditions: ["holdmytask-dev", "module", "browser", "development|production"] }, ssr: { - // Vitest often routes node-environment resolution through the SSR pipeline. + // Vitest often routes node-environment resolution through the SSR pipeline. Keep + // `module` here alongside the non-SSR resolver so a dependency's `module`-keyed + // export resolves the same under Vitest's SSR pipeline as in a normal build. resolve: { - conditions: ["holdmytask-dev", "node", "development|production"] + conditions: ["holdmytask-dev", "module", "node", "development|production"] } }, test: { @@ -31,10 +33,12 @@ export default defineConfig({ environment: "node", globals: true, testTimeout: 30000, + // Carry the dev condition into forked workers (native imports of the package + // entry, e.g. CommonAliases importing index.mjs -> /main). NODE_ENV is left + // alone: it does not select the conditional export (that's `--conditions`), and + // forcing a non-standard `NODE_ENV=holdmytask-dev` could confuse deps that key + // off the usual test/development/production values. nodeOptions: ["--conditions=holdmytask-dev"], - env: { - NODE_ENV: "holdmytask-dev" - }, // "dot" keeps CI logs to one character per test file instead of a full // "RUN vX.Y.Z" + per-file pass/fail block for every file — vitest's // non-interactive fallback (no TTY to redraw) otherwise reprints that diff --git a/devcheck.mjs b/devcheck.mjs index 4ceacf4..b89087c 100644 --- a/devcheck.mjs +++ b/devcheck.mjs @@ -47,15 +47,29 @@ const isInstalledPackage = __dirname.split(path.sep).includes("node_modules"); if (existsSync(srcPath) && !isCI && !isInstalledPackage) { // The condition selects src/ (see the `./main` export in package.json). It can be // supplied via NODE_OPTIONS (`NODE_OPTIONS=--conditions=holdmytask-dev`) OR directly - // on the node CLI (`node --conditions=holdmytask-dev`), which lands in execArgv - - // this is how vitest passes it to workers - so check both. Namespaced (not the - // generic `development`) so a consuming app's own `--conditions=development` can't - // flip this package to a source tree it doesn't ship. NODE_ENV is deliberately NOT - // consulted: it does not affect which tree resolves, so keying off it would both + // on the node CLI (`node --conditions=holdmytask-dev` / `-C holdmytask-dev`), which + // lands in execArgv - this is how vitest passes it to workers - so scan both. + // Parse the actual `--conditions` values and match EXACTLY (not a substring), so + // e.g. `--conditions=not-holdmytask-dev` does not spuriously count. Namespaced (not + // the generic `development`) so a consuming app's own `--conditions=development` + // can't flip this package to a source tree it doesn't ship. NODE_ENV is deliberately + // NOT consulted: it does not affect which tree resolves, so keying off it would both // miss the real problem (dev env set, condition absent -> silently on dist/) and // false-alarm (condition set, dev env absent -> actually fine). - const flags = (process.env.NODE_OPTIONS || "") + " " + process.execArgv.join(" "); - const hasHoldMyTaskDev = flags.includes("holdmytask-dev"); + const conditions = []; + const collect = (value) => { + if (value) for (const c of value.split(/[,|]/)) if (c.trim()) conditions.push(c.trim()); + }; + const scan = (tokens) => { + for (let i = 0; i < tokens.length; i++) { + if (tokens[i] === "--conditions" || tokens[i] === "-C") collect(tokens[i + 1]); + else if (tokens[i].startsWith("--conditions=")) collect(tokens[i].slice("--conditions=".length)); + else if (tokens[i].startsWith("-C=")) collect(tokens[i].slice("-C=".length)); + } + }; + scan(process.execArgv); + scan((process.env.NODE_OPTIONS || "").split(/\s+/).filter(Boolean)); + const hasHoldMyTaskDev = conditions.includes("holdmytask-dev"); if (!hasHoldMyTaskDev) { console.error("❌ Development environment not properly configured!"); diff --git a/tests/DevCheck.test.vitest.mjs b/tests/DevCheck.test.vitest.mjs index edeaf3c..0d795fd 100644 --- a/tests/DevCheck.test.vitest.mjs +++ b/tests/DevCheck.test.vitest.mjs @@ -87,6 +87,17 @@ describe("devcheck", () => { expect(status).toBe(1); }); + test("does NOT match a condition that merely contains 'holdmytask-dev' as a substring", () => { + // Exact-value match, not substring: --conditions=not-holdmytask-dev must NOT silence it. + const { status } = runDevcheck({ src: true }, { nodeArgs: ["--conditions=not-holdmytask-dev"] }); + expect(status).toBe(1); + }); + + test("accepts holdmytask-dev among comma-separated conditions", () => { + const { status } = runDevcheck({ src: true }, { nodeArgs: ["--conditions=foo,holdmytask-dev,bar"] }); + expect(status).toBe(0); + }); + test("skips in CI", () => { const { status, stderr } = runDevcheck({ src: true }, { env: { CI: "true" } }); expect(status).toBe(0); From f3f39efefd4f0a108a13170b3fcdb477423a9f2d Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 10:03:10 -0700 Subject: [PATCH 5/9] fix: remove vestigial ./devcheck export (points at unshipped file) The `./devcheck` -> `./devcheck.mjs` export pointed at a file not in the published `files` allowlist (verified via npm pack: devcheck.mjs isn't in the tarball), so `import "@cldmv/holdmytask/devcheck"` 404s for consumers. devcheck is an internal dev-time guard that index.mjs loads via a relative import, not the package export - nothing imports the subpath. Removing the dead export makes package.json honest. (Copilot review on #12.) --- package.json | 4 ---- 1 file changed, 4 deletions(-) diff --git a/package.json b/package.json index 55b9c35..1e233f3 100644 --- a/package.json +++ b/package.json @@ -11,10 +11,6 @@ "import": "./index.mjs", "require": "./index.cjs" }, - "./devcheck": { - "types": "./types/devcheck.d.mts", - "import": "./devcheck.mjs" - }, "./main": { "holdmytask-dev": { "types": "./types/src/hold-my-task.d.mts", From a959151f0cabf4e44b71a7491f32e461757e5dd4 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 17:07:49 -0700 Subject: [PATCH 6/9] fix(review): parse --conditions as whole literal values (no comma/pipe split) Addresses the second Copilot re-review on PR #12 (2 suppressed comments): - devcheck.mjs: stop splitting condition values on `,`/`|`. Node treats each `--conditions` occurrence as ONE literal condition and does not split on comma or pipe (verified: `--conditions=holdmytask-dev,x` and `--conditions=holdmytask-dev|production` do NOT enable holdmytask-dev). The old split caused a false negative - `holdmytask-dev|production` would silence devcheck while Node actually resolved to dist/. Now collect each value whole and match exactly. Fixed the test that wrongly asserted comma-joined silences (now asserts it nags), and added pipe-joined-nag plus space-separated (`--conditions holdmytask-dev`) and repeated-flag silent cases. - tests/DevCheck.test.vitest.mjs: added the standard project header block so it isn't an outlier vs the other test files. --- devcheck.mjs | 31 +++++++++++++++------------ tests/DevCheck.test.vitest.mjs | 39 +++++++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 17 deletions(-) diff --git a/devcheck.mjs b/devcheck.mjs index b89087c..6c68b22 100644 --- a/devcheck.mjs +++ b/devcheck.mjs @@ -48,23 +48,26 @@ if (existsSync(srcPath) && !isCI && !isInstalledPackage) { // The condition selects src/ (see the `./main` export in package.json). It can be // supplied via NODE_OPTIONS (`NODE_OPTIONS=--conditions=holdmytask-dev`) OR directly // on the node CLI (`node --conditions=holdmytask-dev` / `-C holdmytask-dev`), which - // lands in execArgv - this is how vitest passes it to workers - so scan both. - // Parse the actual `--conditions` values and match EXACTLY (not a substring), so - // e.g. `--conditions=not-holdmytask-dev` does not spuriously count. Namespaced (not - // the generic `development`) so a consuming app's own `--conditions=development` - // can't flip this package to a source tree it doesn't ship. NODE_ENV is deliberately - // NOT consulted: it does not affect which tree resolves, so keying off it would both - // miss the real problem (dev env set, condition absent -> silently on dist/) and - // false-alarm (condition set, dev env absent -> actually fine). + // lands in execArgv - this is how vitest passes it to workers - so scan both. Each + // `--conditions` occurrence is ONE literal condition value: Node does not split it on + // `,` or `|` (verified - `--conditions=holdmytask-dev,x` and + // `--conditions=holdmytask-dev|x` do NOT enable `holdmytask-dev`), and multiple + // conditions are passed as repeated flags. So collect each value whole and match + // EXACTLY - no substring, no splitting - so `not-holdmytask-dev`, `holdmytask-dev,x`, + // and `holdmytask-dev|production` all correctly fail to count. Namespaced (not the + // generic `development`) so a consuming app's own `--conditions=development` can't + // flip this package to a source tree it doesn't ship. NODE_ENV is deliberately NOT + // consulted: it does not affect which tree resolves. const conditions = []; - const collect = (value) => { - if (value) for (const c of value.split(/[,|]/)) if (c.trim()) conditions.push(c.trim()); - }; const scan = (tokens) => { for (let i = 0; i < tokens.length; i++) { - if (tokens[i] === "--conditions" || tokens[i] === "-C") collect(tokens[i + 1]); - else if (tokens[i].startsWith("--conditions=")) collect(tokens[i].slice("--conditions=".length)); - else if (tokens[i].startsWith("-C=")) collect(tokens[i].slice("-C=".length)); + if (tokens[i] === "--conditions" || tokens[i] === "-C") { + if (tokens[i + 1] !== undefined) conditions.push(tokens[i + 1]); + } else if (tokens[i].startsWith("--conditions=")) { + conditions.push(tokens[i].slice("--conditions=".length)); + } else if (tokens[i].startsWith("-C=")) { + conditions.push(tokens[i].slice("-C=".length)); + } } }; scan(process.execArgv); diff --git a/tests/DevCheck.test.vitest.mjs b/tests/DevCheck.test.vitest.mjs index 0d795fd..d6af8ac 100644 --- a/tests/DevCheck.test.vitest.mjs +++ b/tests/DevCheck.test.vitest.mjs @@ -1,3 +1,16 @@ +/** + * @Project: @cldmv/holdmytask + * @Filename: /tests/DevCheck.test.vitest.mjs + * @Date: 2026-08-08T00:00:00-08:00 (1786233600) + * @Author: Nate Hyson + * @Email: + * ----- + * @Last modified by: Nate Hyson (Shinrai@users.noreply.github.com) + * @Last modified time: 2026-08-08T00:00:00-08:00 (1786233600) + * ----- + * @Copyright: Copyright (c) 2013-2026 Catalyzed Motivation Inc. All rights reserved. + */ + import { test, expect, describe, beforeAll, afterAll } from "vitest"; import { spawnSync } from "node:child_process"; import { mkdtempSync, mkdirSync, copyFileSync, rmSync } from "node:fs"; @@ -61,13 +74,24 @@ describe("devcheck", () => { expect(stderr).toBe(""); }); - test("stays silent when the condition is passed on the node CLI (execArgv)", () => { + test("stays silent when the condition is passed on the node CLI (execArgv, = form)", () => { // vitest passes --conditions to workers this way, so devcheck must detect it here too. const { status, stderr } = runDevcheck({ src: true }, { nodeArgs: ["--conditions=holdmytask-dev"] }); expect(status).toBe(0); expect(stderr).toBe(""); }); + test("stays silent when the condition is passed space-separated (--conditions holdmytask-dev)", () => { + const { status, stderr } = runDevcheck({ src: true }, { nodeArgs: ["--conditions", "holdmytask-dev"] }); + expect(status).toBe(0); + expect(stderr).toBe(""); + }); + + test("stays silent when holdmytask-dev is one of several repeated --conditions flags", () => { + const { status } = runDevcheck({ src: true }, { nodeArgs: ["--conditions=foo", "--conditions=holdmytask-dev"] }); + expect(status).toBe(0); + }); + test("NODE_ENV=development alone does NOT silence it (only the condition selects src/)", () => { // Keying off NODE_ENV would be a false negative: dev env set but no condition means // the package is still resolving to dist/, which is exactly what should be flagged. @@ -93,9 +117,18 @@ describe("devcheck", () => { expect(status).toBe(1); }); - test("accepts holdmytask-dev among comma-separated conditions", () => { + test("does NOT treat a comma-joined value as separate conditions", () => { + // Node does not split --conditions on `,`: `foo,holdmytask-dev,bar` is one literal + // condition, so holdmytask-dev is NOT enabled and devcheck must still nag. const { status } = runDevcheck({ src: true }, { nodeArgs: ["--conditions=foo,holdmytask-dev,bar"] }); - expect(status).toBe(0); + expect(status).toBe(1); + }); + + test("does NOT treat a pipe-joined value as separate conditions", () => { + // Likewise Node does not split on `|` (it's a valid condition character, e.g. + // Vite's `development|production`): `holdmytask-dev|production` does not enable it. + const { status } = runDevcheck({ src: true }, { nodeArgs: ["--conditions=holdmytask-dev|production"] }); + expect(status).toBe(1); }); test("skips in CI", () => { From cc37aa254ae1e9ff4e8ceb8ef508417c10a01f90 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 17:54:56 -0700 Subject: [PATCH 7/9] docs(review): drop stale test.env reference in vitest config comment Addresses the suppressed Copilot comment on PR #12 (.configs/vitest.config.mjs:17): the comment still mentioned `test.env` carrying the dev condition into workers, but that override was removed earlier in this PR. Comment now references only `test.nodeOptions`, which is what actually carries it. --- .configs/vitest.config.mjs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.configs/vitest.config.mjs b/.configs/vitest.config.mjs index a434a8e..b0bedf6 100644 --- a/.configs/vitest.config.mjs +++ b/.configs/vitest.config.mjs @@ -11,10 +11,10 @@ export default defineConfig({ // The package-scoped dev condition that routes `@cldmv/holdmytask/main` to `src/` // (see the `./main` export in package.json). Tests exercise and cover the SOURCE // tree, so the resolver must add `holdmytask-dev`. This *replaces* vite's default - // conditions, so the usual ones are kept alongside it. `test.nodeOptions`/`test.env` - // below carry the same condition into forked test workers (for native imports of - // the package entry, e.g. CommonAliases importing index.mjs -> /main), so a bare - // local `npm test` resolves to src the same way CI does. Mirrors @cldmv/uuid. + // conditions, so the usual ones are kept alongside it. `test.nodeOptions` below + // carries the same condition into forked test workers (for native imports of the + // package entry, e.g. CommonAliases importing index.mjs -> /main), so a bare local + // `npm test` resolves to src the same way CI does. Mirrors @cldmv/uuid. resolve: { conditions: ["holdmytask-dev", "module", "browser", "development|production"] }, From 4f96129594a977e0785ec8fb2de94cdc63b733f7 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 20:16:27 -0700 Subject: [PATCH 8/9] fix(review): advance scanner past consumed --conditions value; drop invalid -C= form Addresses the 2 suppressed comments on PR #12's latest review: - devcheck.mjs: the space-form branch (`--conditions x` / `-C x`) consumed tokens[i+1] as the value but did not advance the loop index, so a value that itself looks like a flag (e.g. `--conditions --conditions=x`) was double-processed. Now increment i past the consumed value token. - Dropped the `-C=` branch: Node rejects `-C=value` outright ("bad option"), so it can never appear in execArgv/NODE_OPTIONS - it was dead code. Valid forms are `--conditions=x`, `--conditions x`, and `-C x`. - Added -C short-flag test coverage (space form; verified Node rejects `-C=`). --- devcheck.mjs | 13 ++++++++++--- tests/DevCheck.test.vitest.mjs | 8 ++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/devcheck.mjs b/devcheck.mjs index 6c68b22..80a5df0 100644 --- a/devcheck.mjs +++ b/devcheck.mjs @@ -62,12 +62,19 @@ if (existsSync(srcPath) && !isCI && !isInstalledPackage) { const scan = (tokens) => { for (let i = 0; i < tokens.length; i++) { if (tokens[i] === "--conditions" || tokens[i] === "-C") { - if (tokens[i + 1] !== undefined) conditions.push(tokens[i + 1]); + // Space form (`--conditions x` / `-C x`): consume the following token as this + // flag's value and SKIP it, so a value that itself looks like a flag (e.g. the + // literal `--conditions=x`) isn't re-interpreted on the next iteration. + if (tokens[i + 1] !== undefined) { + conditions.push(tokens[i + 1]); + i++; + } } else if (tokens[i].startsWith("--conditions=")) { conditions.push(tokens[i].slice("--conditions=".length)); - } else if (tokens[i].startsWith("-C=")) { - conditions.push(tokens[i].slice("-C=".length)); } + // Note: `-C=x` is intentionally not handled - Node rejects it ("bad option"), + // so it can never appear in execArgv/NODE_OPTIONS. Valid forms are + // `--conditions=x`, `--conditions x`, and `-C x`. } }; scan(process.execArgv); diff --git a/tests/DevCheck.test.vitest.mjs b/tests/DevCheck.test.vitest.mjs index d6af8ac..e086a0d 100644 --- a/tests/DevCheck.test.vitest.mjs +++ b/tests/DevCheck.test.vitest.mjs @@ -92,6 +92,14 @@ describe("devcheck", () => { expect(status).toBe(0); }); + test("stays silent via the -C short flag (Node's alias for --conditions)", () => { + // Node accepts `-C ` (space form) but rejects `-C=`, so only the + // space form is a real input to detect. + const { status, stderr } = runDevcheck({ src: true }, { nodeArgs: ["-C", "holdmytask-dev"] }); + expect(status).toBe(0); + expect(stderr).toBe(""); + }); + test("NODE_ENV=development alone does NOT silence it (only the condition selects src/)", () => { // Keying off NODE_ENV would be a false negative: dev env set but no condition means // the package is still resolving to dist/, which is exactly what should be flagged. From ee9235b9cb850c1f181f114095384772d5940992 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 21:19:15 -0700 Subject: [PATCH 9/9] docs(review): soften devcheck message - don't assert it's "loading from dist/" Addresses the suppressed Copilot comment on PR #12 (devcheck.mjs:87): the message asserted "holdmytask is loading from dist/", which isn't necessarily true - when devcheck runs standalone (test fixtures) or in an unbuilt checkout, dist/ may not exist at all. Reworded to describe default resolution behavior ("imports resolve to dist/ by default, or fail if it isn't built") rather than asserting the current runtime is definitely on dist/. --- devcheck.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/devcheck.mjs b/devcheck.mjs index 80a5df0..aef5cf2 100644 --- a/devcheck.mjs +++ b/devcheck.mjs @@ -84,7 +84,7 @@ if (existsSync(srcPath) && !isCI && !isInstalledPackage) { if (!hasHoldMyTaskDev) { console.error("❌ Development environment not properly configured!"); console.error("📁 Source folder detected but the 'holdmytask-dev' condition is not set,"); - console.error(" so holdmytask is loading from dist/ instead of src/."); + console.error(" so imports resolve to dist/ by default (or fail if it isn't built) instead of src/."); console.error(""); console.error("🔧 To load from src/ for development, set the condition:"); console.error(" Windows (cmd):");