From 6a81d7b39294922268f466844f06409671cbd392 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Mon, 3 Aug 2026 21:43:18 -0700 Subject: [PATCH 01/25] fix(ci): derive release base in never-supersede concurrency (not hardcoded master/main) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The concurrency group must never supersede release-relevant runs so the release PR posts a green check. The base branch is now DERIVED the same way resolve-release-base does — the CLDMV_RELEASE_BASE var, else the repo's default_branch — instead of the hardcoded refs/heads/master || refs/heads/main. next/hotfixes stay literal (the flow's fixed integration branches) and the release-PR detection (github.head_ref == next/hotfixes) was already base-agnostic. Mirrors the CLDMV/.github core-cicd ci.yml template. --- .github/workflows/ci.yml | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a7505bf..25e77f6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -131,14 +131,27 @@ on: required: false default: true -# Cancel superseded runs on feature branches; keep every master/main run as the -# permanent green record. Keyed on github.ref so push and pull_request events -# for the same branch share a group (the `if:` on the ci job already prevents -# non-fork PR sync from running, but the shared group guards against edge -# cases). +# Concurrency policy, by context: +# - FEATURE branches / feature PRs → cancel superseded runs (per-ref group + +# cancel-in-progress): a newer push makes the older run redundant. +# - RELEASE-relevant contexts → NEVER superseded. Pushes to the release base +# branch (derived: the CLDMV_RELEASE_BASE var → the repo's default branch), +# to next/hotfixes, and the next/hotfixes → base release PRs each get a +# UNIQUE group per run (run_id appended), so nothing cancels them. During +# the burst of pushes a release makes to next/hotfixes (the feature squash, +# the post-hotfix base→next sync merge, the bot's `chore: bump version`), +# every run completes and posts a GREEN check instead of the earlier one +# being cancelled into a red X on the release PR. A bare +# `cancel-in-progress: false` is NOT enough — GitHub still cancels the +# middle PENDING run when a newer one queues; a unique group avoids it. +# The base is NOT hardcoded to master/main — it derives the same way +# resolve-release-base does (CLDMV_RELEASE_BASE override → default_branch). +# next/hotfixes are the flow's fixed integration-branch names. github.head_ref +# is set only on pull_request (the release PR's head → next/hotfixes); +# github.ref carries the branch on push. concurrency: - group: ci-${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: ${{ github.ref != 'refs/heads/master' && github.ref != 'refs/heads/main' }} + group: ci-${{ github.workflow }}-${{ github.ref }}${{ (github.ref == format('refs/heads/{0}', vars.CLDMV_RELEASE_BASE != '' && vars.CLDMV_RELEASE_BASE || github.event.repository.default_branch) || github.ref == 'refs/heads/next' || github.ref == 'refs/heads/hotfixes' || github.head_ref == 'next' || github.head_ref == 'hotfixes') && format('-{0}', github.run_id) || '' }} + cancel-in-progress: true # Workflow-level: matches the broadest write surface the called # `workflow-ci.yml` reaches across its branches: From 7d3565be0b998b0d32a6504221a6c00bd6f5b28c Mon Sep 17 00:00:00 2001 From: "cldmv-bot[bot]" <230771808+cldmv-bot[bot]@users.noreply.github.com> Date: Tue, 4 Aug 2026 04:56:39 +0000 Subject: [PATCH 02/25] chore: bump version to 1.6.3 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 6753c0f..e990cfc 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@cldmv/holdmytask", - "version": "1.6.2", + "version": "1.6.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@cldmv/holdmytask", - "version": "1.6.2", + "version": "1.6.3", "license": "Apache-2.0", "devDependencies": { "@cldmv/vitest-runner": "^1.2.0", diff --git a/package.json b/package.json index 8a8e78c..d2ed457 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@cldmv/holdmytask", - "version": "1.6.2", + "version": "1.6.3", "description": "A tiny task queue that waits until your task is ready", "main": "./index.cjs", "module": "./index.mjs", From ac2ec877bb6942a33d8dfff1c872ccb0cd579d71 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Tue, 4 Aug 2026 05:48:46 -0700 Subject: [PATCH 03/25] test(ci): isolate flaky bypassDelay completion-delay test into its own file The "bypassed task still applies its own completion delay" test hangs to the 30s vitest timeout on the lts/* CI matrix job every time it runs there, while the explicit 20/22/24 jobs (same Node binary) pass. Doesn't reproduce locally, including under CPU throttling. Moving it out of HoldMyTask.test.vitest.mjs (72 tests, ~34 of which never call destroy() on their queue instance) into its own file isolates it from any state/timer accumulation earlier tests in that file might leave behind, to see if that's a factor. --- ...ypassDelayCompletionTiming.test.vitest.mjs | 68 +++++++++++++++++++ tests/HoldMyTask.test.vitest.mjs | 52 -------------- 2 files changed, 68 insertions(+), 52 deletions(-) create mode 100644 tests/BypassDelayCompletionTiming.test.vitest.mjs diff --git a/tests/BypassDelayCompletionTiming.test.vitest.mjs b/tests/BypassDelayCompletionTiming.test.vitest.mjs new file mode 100644 index 0000000..0d1aef2 --- /dev/null +++ b/tests/BypassDelayCompletionTiming.test.vitest.mjs @@ -0,0 +1,68 @@ +import { test, expect, describe } from "vitest"; +import { HoldMyTask } from "../src/hold-my-task.mjs"; + +// Isolated from HoldMyTask.test.vitest.mjs: this test hangs to the 30s vitest +// timeout on the `lts/*` CI matrix job specifically (never on the explicit 20/22/24 +// jobs running the identical Node binary), and never reproduces locally even under +// CPU throttling. Splitting it into its own file removes it from the tail end of a +// 72-test file where ~30 prior HoldMyTask instances are never destroy()'d, to check +// whether accumulated per-instance timer/state leakage from earlier tests in the same +// file is a factor. +describe.each([ + { smartScheduling: true, mode: "Smart Scheduling" }, + { smartScheduling: false, mode: "Traditional Polling" } +])("HoldMyTask with $mode", ({ smartScheduling }) => { + test("bypassed task still applies its own completion delay", async () => { + const q = new HoldMyTask({ + concurrency: 1, + delays: { 1: 200, 2: 400 }, + smartScheduling + }); + const results = []; + const timestamps = []; + + // Task 1: Priority 1 (200ms delay) - will complete first + q.enqueue( + () => { + timestamps.push(Date.now()); + return "task1"; + }, + (err, result) => results.push(result), + { priority: 1 } + ); + + // Task 2: Same priority but bypasses the delay from task1, priority 2 means higher priority but has delay + q.enqueue( + () => { + timestamps.push(Date.now()); + return "task2"; + }, + (err, result) => results.push(result), + { priority: 1, bypassDelay: true } + ); + + // Task 3: Should wait for whatever delay task2 creates (task2 has no specific delay config, so uses priority 1 = 200ms) + q.enqueue( + () => { + timestamps.push(Date.now()); + return "task3"; + }, + (err, result) => results.push(result), + { priority: 1 } + ); + + await new Promise((resolve) => q.on("drain", resolve)); + + expect(results).toEqual(["task1", "task2", "task3"]); // Normal priority order, task2 bypasses task1's delay + + // Task2 bypasses task1's delay (should start immediately after task1) + const task1ToTask2Gap = timestamps[1] - timestamps[0]; + expect(task1ToTask2Gap).toBeLessThan(100); + + // Task3 waits for task2's completion delay (200ms since task2 is priority 1) + const task2ToTask3Gap = timestamps[2] - timestamps[1]; + expect(task2ToTask3Gap).toBeGreaterThan(150); + + q.destroy(); + }); +}); diff --git a/tests/HoldMyTask.test.vitest.mjs b/tests/HoldMyTask.test.vitest.mjs index 82ea457..1ed7331 100644 --- a/tests/HoldMyTask.test.vitest.mjs +++ b/tests/HoldMyTask.test.vitest.mjs @@ -803,58 +803,6 @@ describe.each([ // Test passed if no timeout occurred (task2 started immediately) }); - test("bypassed task still applies its own completion delay", async () => { - const q = new HoldMyTask({ - concurrency: 1, - delays: { 1: 200, 2: 400 }, - smartScheduling - }); - const results = []; - const timestamps = []; - - // Task 1: Priority 1 (200ms delay) - will complete first - q.enqueue( - () => { - timestamps.push(Date.now()); - return "task1"; - }, - (err, result) => results.push(result), - { priority: 1 } - ); - - // Task 2: Same priority but bypasses the delay from task1, priority 2 means higher priority but has delay - q.enqueue( - () => { - timestamps.push(Date.now()); - return "task2"; - }, - (err, result) => results.push(result), - { priority: 1, bypassDelay: true } - ); - - // Task 3: Should wait for whatever delay task2 creates (task2 has no specific delay config, so uses priority 1 = 200ms) - q.enqueue( - () => { - timestamps.push(Date.now()); - return "task3"; - }, - (err, result) => results.push(result), - { priority: 1 } - ); - - await new Promise((resolve) => q.on("drain", resolve)); - - expect(results).toEqual(["task1", "task2", "task3"]); // Normal priority order, task2 bypasses task1's delay - - // Task2 bypasses task1's delay (should start immediately after task1) - const task1ToTask2Gap = timestamps[1] - timestamps[0]; - expect(task1ToTask2Gap).toBeLessThan(100); - - // Task3 waits for task2's completion delay (200ms since task2 is priority 1) - const task2ToTask3Gap = timestamps[2] - timestamps[1]; - expect(task2ToTask3Gap).toBeGreaterThan(150); - }); - test("handles maxQueue: -1 as unlimited queue", async () => { const q = new HoldMyTask({ smartScheduling, maxQueue: -1 }); From de48ac6154d0d85dcd22b8fdb13b306a58e7b603 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Tue, 4 Aug 2026 07:35:45 -0700 Subject: [PATCH 04/25] fix: prevent scheduler deadlock when a ready task's delay expires mid-reschedule _scheduleNextTick() only derived its next wake time from pendingHeap and nextAvailableTime. If it ran at the exact moment nextAvailableTime had just expired (a narrow race against schedulerTick's own timing) with pendingHeap empty, nextTime fell through to Infinity, arming a ~24.8-day setTimeout and stranding any task already sitting in readyHeap - a permanent hang, not a slow test. This is what caused the bypassDelay + concurrency:1 + postDelay tests to intermittently time out at exactly 30000ms on CI's lts/* job. Reproduced locally via a stress loop (1 stall per ~6000 iterations); the fix (accounting for a non-empty readyHeap when computing nextTime) ran 20000 iterations with zero stalls. --- src/hold-my-task.mjs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/hold-my-task.mjs b/src/hold-my-task.mjs index 9cdb198..f570d4b 100644 --- a/src/hold-my-task.mjs +++ b/src/hold-my-task.mjs @@ -1622,6 +1622,13 @@ export class HoldMyTask extends EventEmitter { nextTime = Math.min(nextTime, this.nextAvailableTime); } + // A task already sitting in the ready heap needs an imminent recheck even if + // pendingHeap is empty and nextAvailableTime has already expired - otherwise + // nextTime falls through to Infinity and the 24.8-day fallback below strands it. + if (this.readyHeap.size() > 0) { + nextTime = Math.min(nextTime, now); + } + // If next event is imminent or past, run immediately if (nextTime <= now + this.options.tick) { this.intervalId = setInterval(() => this.schedulerTick(), this.options.tick); From 471bd5003f3f21238639a4e76f3c64dd9c51f214 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Tue, 4 Aug 2026 10:03:11 -0700 Subject: [PATCH 05/25] fix(review): only force imminent recheck when nextTime would stay Infinity Addresses PR #8 review feedback: the prior fix forced nextTime = now whenever readyHeap was non-empty, which degrades an active post-completion delay into a 25ms setInterval poll loop instead of a single setTimeout. Narrow the guard to only fire when nextTime would otherwise fall through to Infinity - the actual stranded-ready-task case. Verified: 20000-iteration stress run stays at 0 stalls, and a normal delayed task now arms a single timeoutId (not intervalId) again. Also addresses three test-file review comments: a stale "priority 2" code comment, wrap the isolated test's assertions in try/finally with `once` instead of `on` so q.destroy() always runs even on assertion failure, and a grammar fix in the file header comment. --- src/hold-my-task.mjs | 13 ++++++---- ...ypassDelayCompletionTiming.test.vitest.mjs | 26 ++++++++++--------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/hold-my-task.mjs b/src/hold-my-task.mjs index f570d4b..a428686 100644 --- a/src/hold-my-task.mjs +++ b/src/hold-my-task.mjs @@ -1622,11 +1622,14 @@ export class HoldMyTask extends EventEmitter { nextTime = Math.min(nextTime, this.nextAvailableTime); } - // A task already sitting in the ready heap needs an imminent recheck even if - // pendingHeap is empty and nextAvailableTime has already expired - otherwise - // nextTime falls through to Infinity and the 24.8-day fallback below strands it. - if (this.readyHeap.size() > 0) { - nextTime = Math.min(nextTime, now); + // A task already sitting in the ready heap needs an imminent recheck if nothing + // else would schedule one - otherwise nextTime falls through to Infinity and the + // 24.8-day fallback below strands it. Only force this when nextTime would + // otherwise stay Infinity (the stranded-task case); an active future delay + // already set nextTime above and should keep its single efficient timeout + // rather than degrade into a tick-interval poll loop until it expires. + if (nextTime === Infinity && this.readyHeap.size() > 0) { + nextTime = now; } // If next event is imminent or past, run immediately diff --git a/tests/BypassDelayCompletionTiming.test.vitest.mjs b/tests/BypassDelayCompletionTiming.test.vitest.mjs index 0d1aef2..58474bb 100644 --- a/tests/BypassDelayCompletionTiming.test.vitest.mjs +++ b/tests/BypassDelayCompletionTiming.test.vitest.mjs @@ -1,7 +1,7 @@ import { test, expect, describe } from "vitest"; import { HoldMyTask } from "../src/hold-my-task.mjs"; -// Isolated from HoldMyTask.test.vitest.mjs: this test hangs to the 30s vitest +// Isolated from HoldMyTask.test.vitest.mjs: this test hangs until the 30s Vitest // timeout on the `lts/*` CI matrix job specifically (never on the explicit 20/22/24 // jobs running the identical Node binary), and never reproduces locally even under // CPU throttling. Splitting it into its own file removes it from the tail end of a @@ -31,7 +31,7 @@ describe.each([ { priority: 1 } ); - // Task 2: Same priority but bypasses the delay from task1, priority 2 means higher priority but has delay + // Task 2: Same priority but bypasses the delay from task1 q.enqueue( () => { timestamps.push(Date.now()); @@ -51,18 +51,20 @@ describe.each([ { priority: 1 } ); - await new Promise((resolve) => q.on("drain", resolve)); + await new Promise((resolve) => q.once("drain", resolve)); - expect(results).toEqual(["task1", "task2", "task3"]); // Normal priority order, task2 bypasses task1's delay + try { + expect(results).toEqual(["task1", "task2", "task3"]); // Normal priority order, task2 bypasses task1's delay - // Task2 bypasses task1's delay (should start immediately after task1) - const task1ToTask2Gap = timestamps[1] - timestamps[0]; - expect(task1ToTask2Gap).toBeLessThan(100); + // Task2 bypasses task1's delay (should start immediately after task1) + const task1ToTask2Gap = timestamps[1] - timestamps[0]; + expect(task1ToTask2Gap).toBeLessThan(100); - // Task3 waits for task2's completion delay (200ms since task2 is priority 1) - const task2ToTask3Gap = timestamps[2] - timestamps[1]; - expect(task2ToTask3Gap).toBeGreaterThan(150); - - q.destroy(); + // Task3 waits for task2's completion delay (200ms since task2 is priority 1) + const task2ToTask3Gap = timestamps[2] - timestamps[1]; + expect(task2ToTask3Gap).toBeGreaterThan(150); + } finally { + q.destroy(); + } }); }); From 96b6bf028c369991a5c4b1e467ad50d7b6a42765 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 5 Aug 2026 21:56:14 +0000 Subject: [PATCH 06/25] deps: bump prettier from 3.6.2 to 3.9.6 in the minor group Bumps the minor group with 1 update: [prettier](https://github.com/prettier/prettier). Updates `prettier` from 3.6.2 to 3.9.6 - [Release notes](https://github.com/prettier/prettier/releases) - [Changelog](https://github.com/prettier/prettier/blob/main/CHANGELOG.md) - [Commits](https://github.com/prettier/prettier/compare/3.6.2...3.9.6) --- updated-dependencies: - dependency-name: prettier dependency-version: 3.9.6 dependency-type: direct:development update-type: version-update:semver-minor dependency-group: minor ... Signed-off-by: dependabot[bot] --- package-lock.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index e990cfc..8eee536 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3792,9 +3792,9 @@ } }, "node_modules/prettier": { - "version": "3.6.2", - "resolved": "https://registry.npmjs.org/prettier/-/prettier-3.6.2.tgz", - "integrity": "sha512-I7AIg5boAr5R0FFtJ6rCfD+LFsWHp81dolrFD8S79U9tb8Az2nGrJncnMSnys+bpQJfRUzqs9hnA81OAA3hCuQ==", + "version": "3.9.6", + "resolved": "https://registry.npmjs.org/prettier/-/prettier-3.9.6.tgz", + "integrity": "sha512-OpN0zzVdiaiAhxpuuj5efpIS4sY9j7bY6uR5mnj5yPzGkdkjNKSJeUThPb60Jw29QuAZgA4o+/iB49kFiaBX6g==", "dev": true, "license": "MIT", "bin": { From 5737d19bb95261ef71f3cb583e30b8cb063738c7 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 5 Aug 2026 21:56:23 +0000 Subject: [PATCH 07/25] deps: bump @types/node from 20.19.24 to 26.1.2 Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 20.19.24 to 26.1.2. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node) --- updated-dependencies: - dependency-name: "@types/node" dependency-version: 26.1.2 dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- package-lock.json | 16 ++++++++-------- package.json | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/package-lock.json b/package-lock.json index e990cfc..3f8a2eb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,7 +16,7 @@ "@eslint/markdown": "^8.0.3", "@html-eslint/eslint-plugin": "^0.64.0", "@html-eslint/parser": "^0.64.0", - "@types/node": "^20.0.0", + "@types/node": "^26.1.2", "@vitest/coverage-v8": "^4.1.10", "@vitest/ui": "^4.1.10", "eslint": "^10.8.0", @@ -979,13 +979,13 @@ "license": "MIT" }, "node_modules/@types/node": { - "version": "20.19.24", - "resolved": "https://registry.npmjs.org/@types/node/-/node-20.19.24.tgz", - "integrity": "sha512-FE5u0ezmi6y9OZEzlJfg37mqqf6ZDSF2V/NLjUyGrR9uTZ7Sb9F7bLNZ03S4XVUNRWGA7Ck4c1kK+YnuWjl+DA==", + "version": "26.1.2", + "resolved": "https://registry.npmjs.org/@types/node/-/node-26.1.2.tgz", + "integrity": "sha512-Vu4a5UFA9rIIFJ7rB/Vaafh9lrCQszopTCx6KjFboXTGQbPNasehVR5TEiithSDGyd1DEiUByggTZsg8jukeIg==", "dev": true, "license": "MIT", "dependencies": { - "undici-types": "~6.21.0" + "undici-types": "~8.3.0" } }, "node_modules/@types/unist": { @@ -4057,9 +4057,9 @@ } }, "node_modules/undici-types": { - "version": "6.21.0", - "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-6.21.0.tgz", - "integrity": "sha512-iwDZqg0QAGrg9Rav5H4n0M64c3mkR59cJ6wQp+7C4nI0gsmExaedaYLNO44eT4AtBBwjbTiGPMlt2Md0T9H9JQ==", + "version": "8.3.0", + "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-8.3.0.tgz", + "integrity": "sha512-j375ScV60dom+YkPFIfTLcOiPxkN/buHz5GobjLhixFuANaNs3C9l4GmrWqejgXWJ7BbJcFYpTEUkS1Ge8bpZQ==", "dev": true, "license": "MIT" }, diff --git a/package.json b/package.json index d2ed457..47236e0 100644 --- a/package.json +++ b/package.json @@ -91,7 +91,7 @@ "@eslint/markdown": "^8.0.3", "@html-eslint/eslint-plugin": "^0.64.0", "@html-eslint/parser": "^0.64.0", - "@types/node": "^20.0.0", + "@types/node": "^26.1.2", "@vitest/coverage-v8": "^4.1.10", "@vitest/ui": "^4.1.10", "eslint": "^10.8.0", From cbc1ac770d20b0d78e985363babdc28ef98adaf4 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Fri, 7 Aug 2026 11:55:40 -0700 Subject: [PATCH 08/25] fix: make HoldMyTask and its constructor aliases real classes index.mjs exported HoldMyTask, Queue, TaskManager, TaskQueue, QueueManager, and TaskProcessor as async factory functions (createHoldMyTask() and friends) rather than the actual HoldMyTask class, so `new QueueManager()` etc. threw "QueueManager is not a constructor" - the entire CommonAliases.test.vitest.mjs suite (35/36 tests) was failing and had been worked around by skipping the whole file rather than fixed (#3). index.cjs already assumed the ESM HoldMyTask export was the real class (`module.exports = HoldMyTask`), and every README/example already used `new HoldMyTask(...)` - the async-factory pattern was the actual bug, not the tests or the CJS bridge. Switches index.mjs to a static top-level import of the real HoldMyTask class from @cldmv/holdmytask/main and re-exports it (and its aliases) directly. The createHoldMyTask/createQueue/createTaskManager/ createTaskProcessor async factory functions are kept as-is for backward compatibility, just simplified to use the now-eagerly-loaded class instead of a fresh dynamic import per call. Un-skips CommonAliases.test.vitest.mjs (all 36 tests now pass) and regenerates types/index.d.mts via `npm run build:types` to match the new export shapes. --- index.mjs | 31 ++++++++++------------- tests/CommonAliases.test.vitest.mjs | 23 +++++------------ types/examples/priority-stress-test.d.mts | 2 +- types/index.d.mts | 18 +++++++------ types/index.d.mts.map | 2 +- 5 files changed, 31 insertions(+), 45 deletions(-) diff --git a/index.mjs b/index.mjs index 8b26b48..f7b7a72 100644 --- a/index.mjs +++ b/index.mjs @@ -20,15 +20,14 @@ } })(); +import { HoldMyTask } from "@cldmv/holdmytask/main"; + /** * Creates a HoldMyTask instance for task queue management * @param {object} [options={}] - Configuration options * @returns {Promise} HoldMyTask instance */ -export default async function createHoldMyTask(options = {}) { - // Dynamic import after environment check - const mod = await import("@cldmv/holdmytask/main"); - const HoldMyTask = mod.HoldMyTask; +export async function createHoldMyTask(options = {}) { return new HoldMyTask(options); } @@ -38,8 +37,6 @@ export default async function createHoldMyTask(options = {}) { * @returns {Promise} HoldMyTask instance */ export async function createQueue(options = {}) { - const mod = await import("@cldmv/holdmytask/main"); - const HoldMyTask = mod.HoldMyTask; return new HoldMyTask(options); } @@ -49,8 +46,6 @@ export async function createQueue(options = {}) { * @returns {Promise} HoldMyTask instance */ export async function createTaskManager(options = {}) { - const mod = await import("@cldmv/holdmytask/main"); - const HoldMyTask = mod.HoldMyTask; return new HoldMyTask(options); } @@ -60,16 +55,16 @@ export async function createTaskManager(options = {}) { * @returns {Promise} HoldMyTask instance */ export async function createTaskProcessor(options = {}) { - const mod = await import("@cldmv/holdmytask/main"); - const HoldMyTask = mod.HoldMyTask; return new HoldMyTask(options); } -// Named export aliases -export { createHoldMyTask as HoldMyTask }; -export { createQueue as queue }; -export { createQueue as Queue }; -export { createTaskManager as TaskManager }; -export { createQueue as TaskQueue }; -export { createQueue as QueueManager }; -export { createTaskProcessor as TaskProcessor }; +// HoldMyTask and its constructor aliases are the real class (see issue #3) - `new +// HoldMyTask()`, `new QueueManager()`, etc. all construct the same underlying type. +export { HoldMyTask }; +export default HoldMyTask; +export { HoldMyTask as queue }; +export { HoldMyTask as Queue }; +export { HoldMyTask as TaskManager }; +export { HoldMyTask as TaskQueue }; +export { HoldMyTask as QueueManager }; +export { HoldMyTask as TaskProcessor }; diff --git a/tests/CommonAliases.test.vitest.mjs b/tests/CommonAliases.test.vitest.mjs index b853c13..f7f560f 100644 --- a/tests/CommonAliases.test.vitest.mjs +++ b/tests/CommonAliases.test.vitest.mjs @@ -8,21 +8,10 @@ * @Copyright: Copyright (c) 2013-2025 Catalyzed Motivation Inc. All rights reserved. */ -// Whole file skipped: every describe block below assumes HoldMyTask / -// Queue / TaskManager / TaskQueue / QueueManager / TaskProcessor (as -// imported from "../index.mjs") are constructors ("new HoldMyTask()" -// etc.), but index.mjs actually exports async factory functions -// (createHoldMyTask() and friends) under those names. This is a -// pre-existing mismatch between the test suite and the real runtime -// exports, unrelated to the v4 CI/vitest-runner onboarding that first -// wired these tests into CI — see -// https://github.com/CLDMV/holdmytask/issues/3. Un-skip once that's -// resolved (either the aliases become real constructors, or these -// tests are rewritten to call the factories instead of `new`-ing them). import { test, expect, describe } from "vitest"; import { HoldMyTask, Queue, TaskManager, TaskQueue, QueueManager, TaskProcessor } from "../index.mjs"; -describe.skip("Common Queue System Aliases", () => { +describe("Common Queue System Aliases", () => { test("should export HoldMyTask as the main class", () => { expect(HoldMyTask).toBeDefined(); expect(typeof HoldMyTask).toBe("function"); @@ -340,7 +329,7 @@ describe.skip("Common Queue System Aliases", () => { }); }); -describe.skip("Primary Method Names", () => { +describe("Primary Method Names", () => { test("has() should work as primary method", () => { const queue = new HoldMyTask(); const customId = "primary-has-test"; @@ -393,7 +382,7 @@ describe.skip("Primary Method Names", () => { }); }); -describe.skip("Method Alias Compatibility", () => { +describe("Method Alias Compatibility", () => { test("hasTask() should work as alias for has()", () => { const queue = new HoldMyTask(); const customId = "hasTask-alias-test"; @@ -456,7 +445,7 @@ describe.skip("Method Alias Compatibility", () => { }); }); -describe.skip("Enqueue Method Aliases", () => { +describe("Enqueue Method Aliases", () => { test("schedule() should work as alias for enqueue()", () => { const queue = new HoldMyTask(); let executed = false; @@ -538,7 +527,7 @@ describe.skip("Enqueue Method Aliases", () => { }); }); -describe.skip("Import Aliases", () => { +describe("Import Aliases", () => { test("queue alias should work", async () => { const { queue } = await import("../index.mjs"); const instance = new queue(); @@ -606,7 +595,7 @@ describe.skip("Import Aliases", () => { }); }); -describe.skip("Control Method Aliases", () => { +describe("Control Method Aliases", () => { test("shutdown() should work as alias for destroy()", async () => { const queue = new HoldMyTask(); diff --git a/types/examples/priority-stress-test.d.mts b/types/examples/priority-stress-test.d.mts index 16a1388..ae7f36d 100644 --- a/types/examples/priority-stress-test.d.mts +++ b/types/examples/priority-stress-test.d.mts @@ -60,7 +60,7 @@ declare class PriorityVolumeController { declare function runPriorityStressTests(): Promise<{ scenario: string; totalDuration: number; - accurateCommands: number; + accurateCommands: any; totalCommands: number; accuracyRate: number; finalVolume: number; diff --git a/types/index.d.mts b/types/index.d.mts index 911b5fe..ee05098 100644 --- a/types/index.d.mts +++ b/types/index.d.mts @@ -10,12 +10,13 @@ * ----- * @Copyright: Copyright (c) 2013-2025 Catalyzed Motivation Inc. All rights reserved. */ +import { HoldMyTask } from "@cldmv/holdmytask/main"; /** * Creates a HoldMyTask instance for task queue management * @param {object} [options={}] - Configuration options * @returns {Promise} HoldMyTask instance */ -export default function createHoldMyTask(options?: object): Promise; +export declare function createHoldMyTask(options?: object): Promise; /** * Create a task queue instance * @param {object} [options={}] - Configuration options @@ -34,11 +35,12 @@ export declare function createTaskManager(options?: object): Promise; * @returns {Promise} HoldMyTask instance */ export declare function createTaskProcessor(options?: object): Promise; -export { createHoldMyTask as HoldMyTask }; -export { createQueue as queue }; -export { createQueue as Queue }; -export { createTaskManager as TaskManager }; -export { createQueue as TaskQueue }; -export { createQueue as QueueManager }; -export { createTaskProcessor as TaskProcessor }; +export { HoldMyTask }; +export default HoldMyTask; +export { HoldMyTask as queue }; +export { HoldMyTask as Queue }; +export { HoldMyTask as TaskManager }; +export { HoldMyTask as TaskQueue }; +export { HoldMyTask as QueueManager }; +export { HoldMyTask as TaskProcessor }; //# sourceMappingURL=index.d.mts.map \ No newline at end of file diff --git a/types/index.d.mts.map b/types/index.d.mts.map index 72a5e49..17bb6bd 100644 --- a/types/index.d.mts.map +++ b/types/index.d.mts.map @@ -1 +1 @@ -{"version":3,"file":"index.d.mts","sourceRoot":"","sources":["../index.mjs"],"names":[],"mappings":"AAAA;;;;;;;;;;;GAWG;AAWH;;;;GAIG;AACH,wBAA8B,gBAAgB,CAAC,OAAO,AAHnD,CACA,EADQ,MAGgD,GAF9C,OAAO,CAAC,MAAM,CAAC,CAO3B;AAED;;;;GAIG;AACH,wBAAsB,WAAW,CAAC,OAAO,AAHtC,CACA,EADQ,MAGmC,GAFjC,OAAO,CAAC,MAAM,CAAC,CAM3B;AAED;;;;GAIG;AACH,wBAAsB,iBAAiB,CAAC,OAAO,AAH5C,CACA,EADQ,MAGyC,GAFvC,OAAO,CAAC,MAAM,CAAC,CAM3B;AAED;;;;GAIG;AACH,wBAAsB,mBAAmB,CAAC,OAAO,AAH9C,CACA,EADQ,MAG2C,GAFzC,OAAO,CAAC,MAAM,CAAC,CAM3B;AAGD,OAAO,EAAE,gBAAgB,IAAI,UAAU,EAAE,CAAC;AAC1C,OAAO,EAAE,WAAW,IAAI,KAAK,EAAE,CAAC;AAChC,OAAO,EAAE,WAAW,IAAI,KAAK,EAAE,CAAC;AAChC,OAAO,EAAE,iBAAiB,IAAI,WAAW,EAAE,CAAC;AAC5C,OAAO,EAAE,WAAW,IAAI,SAAS,EAAE,CAAC;AACpC,OAAO,EAAE,WAAW,IAAI,YAAY,EAAE,CAAC;AACvC,OAAO,EAAE,mBAAmB,IAAI,aAAa,EAAE,CAAC"} \ No newline at end of file +{"version":3,"file":"index.d.mts","sourceRoot":"","sources":["../index.mjs"],"names":[],"mappings":"AAAA;;;;;;;;;;;GAWG;AAWH,OAAO,EAAE,UAAU,EAAE,MAAM,wBAAwB,CAAC;AAEpD;;;;GAIG;AACH,wBAAsB,gBAAgB,CAAC,OAAO,AAH3C,CACA,EADQ,MAGwC,GAFtC,OAAO,CAAC,MAAM,CAAC,CAI3B;AAED;;;;GAIG;AACH,wBAAsB,WAAW,CAAC,OAAO,AAHtC,CACA,EADQ,MAGmC,GAFjC,OAAO,CAAC,MAAM,CAAC,CAI3B;AAED;;;;GAIG;AACH,wBAAsB,iBAAiB,CAAC,OAAO,AAH5C,CACA,EADQ,MAGyC,GAFvC,OAAO,CAAC,MAAM,CAAC,CAI3B;AAED;;;;GAIG;AACH,wBAAsB,mBAAmB,CAAC,OAAO,AAH9C,CACA,EADQ,MAG2C,GAFzC,OAAO,CAAC,MAAM,CAAC,CAI3B;AAID,OAAO,EAAE,UAAU,EAAE,CAAC;eACP,UAAU;AACzB,OAAO,EAAE,UAAU,IAAI,KAAK,EAAE,CAAC;AAC/B,OAAO,EAAE,UAAU,IAAI,KAAK,EAAE,CAAC;AAC/B,OAAO,EAAE,UAAU,IAAI,WAAW,EAAE,CAAC;AACrC,OAAO,EAAE,UAAU,IAAI,SAAS,EAAE,CAAC;AACnC,OAAO,EAAE,UAAU,IAAI,YAAY,EAAE,CAAC;AACtC,OAAO,EAAE,UAAU,IAAI,aAAa,EAAE,CAAC"} \ No newline at end of file From 964088d3b927b7306f0d429f6a5a110f5d87372f Mon Sep 17 00:00:00 2001 From: Shinrai Date: Fri, 7 Aug 2026 18:06:37 -0700 Subject: [PATCH 09/25] test: add regression test for the scheduler stranded-task deadlock Addresses PR #8 review feedback (suppressed comment on src/hold-my-task.mjs:1633): no regression test covered the specific failure mode the fix addresses, so a future refactor of _scheduleNextTick()/schedulerTick() could reintroduce the 24.8-day timeout fallback silently. Uses the constructor's injectable `now` option to deterministically force the exact race (a readyHeap task whose delay has just expired, with pendingHeap empty) instead of relying on real wall-clock timing luck. Verified the test fails against the pre-fix code (armed a ~2147483647ms setTimeout) and passes with the fix (armed delay < 1000ms). --- ...ypassDelayCompletionTiming.test.vitest.mjs | 66 ++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/tests/BypassDelayCompletionTiming.test.vitest.mjs b/tests/BypassDelayCompletionTiming.test.vitest.mjs index 58474bb..986d5fa 100644 --- a/tests/BypassDelayCompletionTiming.test.vitest.mjs +++ b/tests/BypassDelayCompletionTiming.test.vitest.mjs @@ -1,4 +1,4 @@ -import { test, expect, describe } from "vitest"; +import { test, expect, describe, vi } from "vitest"; import { HoldMyTask } from "../src/hold-my-task.mjs"; // Isolated from HoldMyTask.test.vitest.mjs: this test hangs until the 30s Vitest @@ -68,3 +68,67 @@ describe.each([ } }); }); + +// Regression test for the scheduler deadlock fixed alongside the test above: +// _scheduleNextTick() only derived its next wake time from pendingHeap and +// nextAvailableTime. Called at the exact moment nextAvailableTime had just +// expired (a narrow race against schedulerTick's own timing), with pendingHeap +// empty, nextTime fell through to Infinity - arming a ~24.8-day setTimeout and +// stranding any task already sitting in readyHeap forever. Traditional Polling +// only: Smart Scheduling uses a different scheduleSmartTimeout()/runScheduler() +// path unaffected by this bug. Uses the constructor's injectable `now` option to +// force the exact race deterministically instead of racing real wall-clock time. +test("a readyHeap task is not stranded when its delay has just expired (regression)", async () => { + let fakeNow = Date.now(); + const q = new HoldMyTask({ + concurrency: 1, + delays: { 1: 50 }, + smartScheduling: false, + now: () => fakeNow + }); + const results = []; + + q.enqueue( + () => "task1", + (err, r) => results.push(r), + { priority: 1 } + ); + q.enqueue( + () => "task2", + (err, r) => results.push(r), + { priority: 1 } + ); + + // Wait for task1 to actually run and complete (real time - the default poll + // tick is 25ms, so this needs to clear a few real ticks; the queue's own delay + // bookkeeping uses the injected now(), not real time). + await new Promise((resolve) => setTimeout(resolve, 120)); + + try { + // task2 should now be sitting in readyHeap, blocked by task1's post-completion delay. + expect(q.readyHeap.size()).toBe(1); + expect(q.pendingHeap.size()).toBe(0); + expect(q.nextAvailableTime).toBeGreaterThan(0); + + // Simulate wall-clock time crossing nextAvailableTime right before the + // scheduler gets a chance to recheck it - the exact race window that caused + // the deadlock. + fakeNow = q.nextAvailableTime + 1; + + const setTimeoutSpy = vi.spyOn(global, "setTimeout"); + q._scheduleNextTick(); + setTimeoutSpy.mockRestore(); + + if (setTimeoutSpy.mock.calls.length > 0) { + const armedDelay = setTimeoutSpy.mock.calls.at(-1)[1]; + // Before the fix this was ~2147483647 (the 24.8-day fallback), stranding + // task2 forever. After the fix it must be an imminent recheck. + expect(armedDelay).toBeLessThan(1000); + } else { + // Took the interval branch instead - also an imminent recheck, also fine. + expect(q.intervalId).toBeTruthy(); + } + } finally { + q.destroy(); + } +}); From 01d22d50f37d93c02d479c49922f6a6473fe1ddf Mon Sep 17 00:00:00 2001 From: Shinrai Date: Fri, 7 Aug 2026 18:11:59 -0700 Subject: [PATCH 10/25] fix(types): type createHoldMyTask/createQueue/etc. as returning HoldMyTask Addresses PR #11 review feedback: the factory functions' JSDoc @returns was Promise, losing the actual return type now that HoldMyTask is statically imported and in scope. Regenerated types/index.d.mts via `npm run build:types`. --- index.mjs | 8 ++++---- types/index.d.mts | 16 ++++++++-------- types/index.d.mts.map | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/index.mjs b/index.mjs index f7b7a72..223083c 100644 --- a/index.mjs +++ b/index.mjs @@ -25,7 +25,7 @@ import { HoldMyTask } from "@cldmv/holdmytask/main"; /** * Creates a HoldMyTask instance for task queue management * @param {object} [options={}] - Configuration options - * @returns {Promise} HoldMyTask instance + * @returns {Promise} HoldMyTask instance */ export async function createHoldMyTask(options = {}) { return new HoldMyTask(options); @@ -34,7 +34,7 @@ export async function createHoldMyTask(options = {}) { /** * Create a task queue instance * @param {object} [options={}] - Configuration options - * @returns {Promise} HoldMyTask instance + * @returns {Promise} HoldMyTask instance */ export async function createQueue(options = {}) { return new HoldMyTask(options); @@ -43,7 +43,7 @@ export async function createQueue(options = {}) { /** * Create a task manager instance * @param {object} [options={}] - Configuration options - * @returns {Promise} HoldMyTask instance + * @returns {Promise} HoldMyTask instance */ export async function createTaskManager(options = {}) { return new HoldMyTask(options); @@ -52,7 +52,7 @@ export async function createTaskManager(options = {}) { /** * Create a task processor instance * @param {object} [options={}] - Configuration options - * @returns {Promise} HoldMyTask instance + * @returns {Promise} HoldMyTask instance */ export async function createTaskProcessor(options = {}) { return new HoldMyTask(options); diff --git a/types/index.d.mts b/types/index.d.mts index ee05098..af165b3 100644 --- a/types/index.d.mts +++ b/types/index.d.mts @@ -14,27 +14,27 @@ import { HoldMyTask } from "@cldmv/holdmytask/main"; /** * Creates a HoldMyTask instance for task queue management * @param {object} [options={}] - Configuration options - * @returns {Promise} HoldMyTask instance + * @returns {Promise} HoldMyTask instance */ -export declare function createHoldMyTask(options?: object): Promise; +export declare function createHoldMyTask(options?: object): Promise; /** * Create a task queue instance * @param {object} [options={}] - Configuration options - * @returns {Promise} HoldMyTask instance + * @returns {Promise} HoldMyTask instance */ -export declare function createQueue(options?: object): Promise; +export declare function createQueue(options?: object): Promise; /** * Create a task manager instance * @param {object} [options={}] - Configuration options - * @returns {Promise} HoldMyTask instance + * @returns {Promise} HoldMyTask instance */ -export declare function createTaskManager(options?: object): Promise; +export declare function createTaskManager(options?: object): Promise; /** * Create a task processor instance * @param {object} [options={}] - Configuration options - * @returns {Promise} HoldMyTask instance + * @returns {Promise} HoldMyTask instance */ -export declare function createTaskProcessor(options?: object): Promise; +export declare function createTaskProcessor(options?: object): Promise; export { HoldMyTask }; export default HoldMyTask; export { HoldMyTask as queue }; diff --git a/types/index.d.mts.map b/types/index.d.mts.map index 17bb6bd..61ba120 100644 --- a/types/index.d.mts.map +++ b/types/index.d.mts.map @@ -1 +1 @@ -{"version":3,"file":"index.d.mts","sourceRoot":"","sources":["../index.mjs"],"names":[],"mappings":"AAAA;;;;;;;;;;;GAWG;AAWH,OAAO,EAAE,UAAU,EAAE,MAAM,wBAAwB,CAAC;AAEpD;;;;GAIG;AACH,wBAAsB,gBAAgB,CAAC,OAAO,AAH3C,CACA,EADQ,MAGwC,GAFtC,OAAO,CAAC,MAAM,CAAC,CAI3B;AAED;;;;GAIG;AACH,wBAAsB,WAAW,CAAC,OAAO,AAHtC,CACA,EADQ,MAGmC,GAFjC,OAAO,CAAC,MAAM,CAAC,CAI3B;AAED;;;;GAIG;AACH,wBAAsB,iBAAiB,CAAC,OAAO,AAH5C,CACA,EADQ,MAGyC,GAFvC,OAAO,CAAC,MAAM,CAAC,CAI3B;AAED;;;;GAIG;AACH,wBAAsB,mBAAmB,CAAC,OAAO,AAH9C,CACA,EADQ,MAG2C,GAFzC,OAAO,CAAC,MAAM,CAAC,CAI3B;AAID,OAAO,EAAE,UAAU,EAAE,CAAC;eACP,UAAU;AACzB,OAAO,EAAE,UAAU,IAAI,KAAK,EAAE,CAAC;AAC/B,OAAO,EAAE,UAAU,IAAI,KAAK,EAAE,CAAC;AAC/B,OAAO,EAAE,UAAU,IAAI,WAAW,EAAE,CAAC;AACrC,OAAO,EAAE,UAAU,IAAI,SAAS,EAAE,CAAC;AACnC,OAAO,EAAE,UAAU,IAAI,YAAY,EAAE,CAAC;AACtC,OAAO,EAAE,UAAU,IAAI,aAAa,EAAE,CAAC"} \ No newline at end of file +{"version":3,"file":"index.d.mts","sourceRoot":"","sources":["../index.mjs"],"names":[],"mappings":"AAAA;;;;;;;;;;;GAWG;AAWH,OAAO,EAAE,UAAU,EAAE,MAAM,wBAAwB,CAAC;AAEpD;;;;GAIG;AACH,wBAAsB,gBAAgB,CAAC,OAAO,AAH3C,CACA,EADQ,MAGwC,GAFtC,OAAO,CAAC,UAAU,CAAC,CAI/B;AAED;;;;GAIG;AACH,wBAAsB,WAAW,CAAC,OAAO,AAHtC,CACA,EADQ,MAGmC,GAFjC,OAAO,CAAC,UAAU,CAAC,CAI/B;AAED;;;;GAIG;AACH,wBAAsB,iBAAiB,CAAC,OAAO,AAH5C,CACA,EADQ,MAGyC,GAFvC,OAAO,CAAC,UAAU,CAAC,CAI/B;AAED;;;;GAIG;AACH,wBAAsB,mBAAmB,CAAC,OAAO,AAH9C,CACA,EADQ,MAG2C,GAFzC,OAAO,CAAC,UAAU,CAAC,CAI/B;AAID,OAAO,EAAE,UAAU,EAAE,CAAC;eACP,UAAU;AACzB,OAAO,EAAE,UAAU,IAAI,KAAK,EAAE,CAAC;AAC/B,OAAO,EAAE,UAAU,IAAI,KAAK,EAAE,CAAC;AAC/B,OAAO,EAAE,UAAU,IAAI,WAAW,EAAE,CAAC;AACrC,OAAO,EAAE,UAAU,IAAI,SAAS,EAAE,CAAC;AACnC,OAAO,EAAE,UAAU,IAAI,YAAY,EAAE,CAAC;AACtC,OAAO,EAAE,UAAU,IAAI,aAAa,EAAE,CAAC"} \ No newline at end of file From eb6d14f12cb0099b50d8cef08bb8e52d8ffaa174 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Fri, 7 Aug 2026 18:12:17 -0700 Subject: [PATCH 11/25] fix!: mark HoldMyTask default/alias export change as breaking Addresses PR #11 review feedback: the PR's auto-generated changelog claimed "No breaking changes", but cbc1ac7 changed the default export (and the HoldMyTask/Queue/TaskManager/TaskQueue/QueueManager/TaskProcessor named aliases) from an async factory function to the real HoldMyTask class. Code that previously called these as functions - e.g. `await HoldMyTask()`, or `await (await import("@cldmv/holdmytask")).default()` - now gets "Class constructor HoldMyTask cannot be invoked without 'new'" and must switch to `new HoldMyTask()` / `new QueueManager()` etc. instead. This matches the already-documented `new HoldMyTask(options)` usage throughout README and every example, and the existing CJS bridge (index.cjs), which already assumed the ESM default export was the class - only the previously-buggy async-factory calling convention on these specific export names is removed. BREAKING CHANGE: HoldMyTask, Queue, TaskManager, TaskQueue, QueueManager, TaskProcessor, and the package default export are now the real HoldMyTask class instead of an async factory function. Use `new HoldMyTask(options)` (or `new QueueManager(options)`, etc.) instead of calling them as functions. The createHoldMyTask/createQueue/createTaskManager/ createTaskProcessor named async factory functions are unchanged. From 2240dfd8d18b80dd05ec40d76259ca821e71a7fd Mon Sep 17 00:00:00 2001 From: Shinrai Date: Fri, 7 Aug 2026 21:42:10 -0700 Subject: [PATCH 12/25] 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 13/25] 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 14/25] 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 15/25] 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 16/25] 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 85afcf0c662dd0652fd2dc976f7db3ed0a2e40c7 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 10:04:12 -0700 Subject: [PATCH 17/25] test(review): restore setTimeout spy in finally, capture calls before restore Addresses the suppressed Copilot comment on PR #8 (BypassDelayCompletionTiming.test.vitest.mjs:130): the regression test called setTimeoutSpy.mockRestore() unconditionally right after _scheduleNextTick(), so a throw there would leak the global spy into later tests, and it read mock.calls after restoring. Now wrap the _scheduleNextTick() call in try/finally, capture the recorded calls into a local before restoring, and restore the spy in the finally so cleanup is guaranteed. --- tests/BypassDelayCompletionTiming.test.vitest.mjs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/BypassDelayCompletionTiming.test.vitest.mjs b/tests/BypassDelayCompletionTiming.test.vitest.mjs index 986d5fa..9805735 100644 --- a/tests/BypassDelayCompletionTiming.test.vitest.mjs +++ b/tests/BypassDelayCompletionTiming.test.vitest.mjs @@ -116,11 +116,18 @@ test("a readyHeap task is not stranded when its delay has just expired (regressi fakeNow = q.nextAvailableTime + 1; const setTimeoutSpy = vi.spyOn(global, "setTimeout"); - q._scheduleNextTick(); - setTimeoutSpy.mockRestore(); + let armedCalls; + try { + q._scheduleNextTick(); + } finally { + // Capture the recorded calls BEFORE restoring, and restore in finally so the + // global spy never leaks into later tests even if _scheduleNextTick throws. + armedCalls = setTimeoutSpy.mock.calls.slice(); + setTimeoutSpy.mockRestore(); + } - if (setTimeoutSpy.mock.calls.length > 0) { - const armedDelay = setTimeoutSpy.mock.calls.at(-1)[1]; + if (armedCalls.length > 0) { + const armedDelay = armedCalls.at(-1)[1]; // Before the fix this was ~2147483647 (the 24.8-day fallback), stranding // task2 forever. After the fix it must be an imminent recheck. expect(armedDelay).toBeLessThan(1000); From 1a2b6f32ef776452ab40f1f00b0cbdd4ca5d8eeb Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 10:05:28 -0700 Subject: [PATCH 18/25] docs(review): correct the devcheck ordering comment in index.mjs Addresses the suppressed Copilot comment on PR #11 (index.mjs:23): the comment claimed the devcheck "must happen before holdmytask imports", but the static `import` of the core is hoisted and evaluated before the devcheck IIFE runs, so that ordering isn't real. Reworded to state devcheck is a best-effort, fire-and-forget dev-time warning that does NOT run before the core loads, and why (running it strictly first needs top-level await, which breaks index.cjs's synchronous require). Regenerated the types sourcemap (source positions shifted; the .d.ts itself is unchanged). --- index.mjs | 6 +++++- types/index.d.mts.map | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/index.mjs b/index.mjs index 223083c..878d647 100644 --- a/index.mjs +++ b/index.mjs @@ -11,7 +11,11 @@ * @Copyright: Copyright (c) 2013-2025 Catalyzed Motivation Inc. All rights reserved. */ -// Development environment check (must happen before holdmytask imports) +// Development environment check. NOTE: the static `import` of the core below is +// hoisted and evaluated before this IIFE body runs, so devcheck does NOT run before +// the core loads - it's a best-effort, fire-and-forget dev-time warning. (Running it +// strictly first would require a dynamic import + top-level await, which breaks the +// index.cjs bridge's synchronous `require` of this module - see PR #11 discussion.) (async () => { try { await import("./devcheck.mjs"); diff --git a/types/index.d.mts.map b/types/index.d.mts.map index 61ba120..9a1ce9d 100644 --- a/types/index.d.mts.map +++ b/types/index.d.mts.map @@ -1 +1 @@ -{"version":3,"file":"index.d.mts","sourceRoot":"","sources":["../index.mjs"],"names":[],"mappings":"AAAA;;;;;;;;;;;GAWG;AAWH,OAAO,EAAE,UAAU,EAAE,MAAM,wBAAwB,CAAC;AAEpD;;;;GAIG;AACH,wBAAsB,gBAAgB,CAAC,OAAO,AAH3C,CACA,EADQ,MAGwC,GAFtC,OAAO,CAAC,UAAU,CAAC,CAI/B;AAED;;;;GAIG;AACH,wBAAsB,WAAW,CAAC,OAAO,AAHtC,CACA,EADQ,MAGmC,GAFjC,OAAO,CAAC,UAAU,CAAC,CAI/B;AAED;;;;GAIG;AACH,wBAAsB,iBAAiB,CAAC,OAAO,AAH5C,CACA,EADQ,MAGyC,GAFvC,OAAO,CAAC,UAAU,CAAC,CAI/B;AAED;;;;GAIG;AACH,wBAAsB,mBAAmB,CAAC,OAAO,AAH9C,CACA,EADQ,MAG2C,GAFzC,OAAO,CAAC,UAAU,CAAC,CAI/B;AAID,OAAO,EAAE,UAAU,EAAE,CAAC;eACP,UAAU;AACzB,OAAO,EAAE,UAAU,IAAI,KAAK,EAAE,CAAC;AAC/B,OAAO,EAAE,UAAU,IAAI,KAAK,EAAE,CAAC;AAC/B,OAAO,EAAE,UAAU,IAAI,WAAW,EAAE,CAAC;AACrC,OAAO,EAAE,UAAU,IAAI,SAAS,EAAE,CAAC;AACnC,OAAO,EAAE,UAAU,IAAI,YAAY,EAAE,CAAC;AACtC,OAAO,EAAE,UAAU,IAAI,aAAa,EAAE,CAAC"} \ No newline at end of file +{"version":3,"file":"index.d.mts","sourceRoot":"","sources":["../index.mjs"],"names":[],"mappings":"AAAA;;;;;;;;;;;GAWG;AAeH,OAAO,EAAE,UAAU,EAAE,MAAM,wBAAwB,CAAC;AAEpD;;;;GAIG;AACH,wBAAsB,gBAAgB,CAAC,OAAO,AAH3C,CACA,EADQ,MAGwC,GAFtC,OAAO,CAAC,UAAU,CAAC,CAI/B;AAED;;;;GAIG;AACH,wBAAsB,WAAW,CAAC,OAAO,AAHtC,CACA,EADQ,MAGmC,GAFjC,OAAO,CAAC,UAAU,CAAC,CAI/B;AAED;;;;GAIG;AACH,wBAAsB,iBAAiB,CAAC,OAAO,AAH5C,CACA,EADQ,MAGyC,GAFvC,OAAO,CAAC,UAAU,CAAC,CAI/B;AAED;;;;GAIG;AACH,wBAAsB,mBAAmB,CAAC,OAAO,AAH9C,CACA,EADQ,MAG2C,GAFzC,OAAO,CAAC,UAAU,CAAC,CAI/B;AAID,OAAO,EAAE,UAAU,EAAE,CAAC;eACP,UAAU;AACzB,OAAO,EAAE,UAAU,IAAI,KAAK,EAAE,CAAC;AAC/B,OAAO,EAAE,UAAU,IAAI,KAAK,EAAE,CAAC;AAC/B,OAAO,EAAE,UAAU,IAAI,WAAW,EAAE,CAAC;AACrC,OAAO,EAAE,UAAU,IAAI,SAAS,EAAE,CAAC;AACnC,OAAO,EAAE,UAAU,IAAI,YAAY,EAAE,CAAC;AACtC,OAAO,EAAE,UAAU,IAAI,aAAa,EAAE,CAAC"} \ No newline at end of file From 65cf83a4eba34218e9e8d65351302868b259e086 Mon Sep 17 00:00:00 2001 From: "cldmv-bot[bot]" <230771808+cldmv-bot[bot]@users.noreply.github.com> Date: Sun, 9 Aug 2026 00:00:27 +0000 Subject: [PATCH 19/25] chore: bump version to 2.0.0 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 8eee536..45b0a33 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@cldmv/holdmytask", - "version": "1.6.3", + "version": "2.0.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@cldmv/holdmytask", - "version": "1.6.3", + "version": "2.0.0", "license": "Apache-2.0", "devDependencies": { "@cldmv/vitest-runner": "^1.2.0", diff --git a/package.json b/package.json index d2ed457..1dadc19 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@cldmv/holdmytask", - "version": "1.6.3", + "version": "2.0.0", "description": "A tiny task queue that waits until your task is ready", "main": "./index.cjs", "module": "./index.mjs", From a959151f0cabf4e44b71a7491f32e461757e5dd4 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 17:07:49 -0700 Subject: [PATCH 20/25] 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 ff6c70f3f5503bf3fe59f75004c6050ba9fd30d5 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 17:08:59 -0700 Subject: [PATCH 21/25] test(review): fail fast + always clean up if drain never fires Addresses the suppressed Copilot comment on PR #8 (BypassDelayCompletionTiming.test.vitest.mjs:68): the `drain` await sat OUTSIDE the try/finally, so if drain is never emitted - the exact deadlock this file exists to catch - the test would hang to vitest's 30s global timeout and never run q.destroy(), leaking timers. Moved the wait inside the try and raced it against a 5s timeout that rejects with a clear message, so the failure surfaces fast and cleanup always runs. --- tests/BypassDelayCompletionTiming.test.vitest.mjs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/BypassDelayCompletionTiming.test.vitest.mjs b/tests/BypassDelayCompletionTiming.test.vitest.mjs index 9805735..b2dc9be 100644 --- a/tests/BypassDelayCompletionTiming.test.vitest.mjs +++ b/tests/BypassDelayCompletionTiming.test.vitest.mjs @@ -51,9 +51,19 @@ describe.each([ { priority: 1 } ); - await new Promise((resolve) => q.once("drain", resolve)); - try { + // Race the drain wait against a short timeout: if drain never fires (the exact + // deadlock this file exists to catch), fail fast instead of hanging to vitest's + // 30s global timeout - and keep the wait inside the try so the finally still + // runs q.destroy() and doesn't leak timers into later tests. + await new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error("drain not emitted within 5000ms")), 5000); + q.once("drain", () => { + clearTimeout(timer); + resolve(); + }); + }); + expect(results).toEqual(["task1", "task2", "task3"]); // Normal priority order, task2 bypasses task1's delay // Task2 bypasses task1's delay (should start immediately after task1) From cc37aa254ae1e9ff4e8ceb8ef508417c10a01f90 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 17:54:56 -0700 Subject: [PATCH 22/25] 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 a456202592ae191bbf4f6354c2e693be0c2e92aa Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 17:56:06 -0700 Subject: [PATCH 23/25] test(review): poll for the observable state instead of a fixed 120ms sleep Addresses the suppressed Copilot comment on PR #8 (BypassDelayCompletionTiming.test.vitest.mjs:115): the regression test used a hard-coded 120ms sleep to assume task1 had completed and task2 had reached readyHeap, which is nondeterministic on slow/contended CI. Replaced it with a bounded poll (2s deadline) on the observable condition - task1 in results and task2 parked in readyHeap - and kept it inside the try so q.destroy() still runs if it ever times out. --- ...ypassDelayCompletionTiming.test.vitest.mjs | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/tests/BypassDelayCompletionTiming.test.vitest.mjs b/tests/BypassDelayCompletionTiming.test.vitest.mjs index b2dc9be..9936455 100644 --- a/tests/BypassDelayCompletionTiming.test.vitest.mjs +++ b/tests/BypassDelayCompletionTiming.test.vitest.mjs @@ -109,13 +109,22 @@ test("a readyHeap task is not stranded when its delay has just expired (regressi { priority: 1 } ); - // Wait for task1 to actually run and complete (real time - the default poll - // tick is 25ms, so this needs to clear a few real ticks; the queue's own delay - // bookkeeping uses the injected now(), not real time). - await new Promise((resolve) => setTimeout(resolve, 120)); - try { - // task2 should now be sitting in readyHeap, blocked by task1's post-completion delay. + // Wait (bounded) until task1 has actually run + completed and task2 has moved + // into readyHeap, blocked by task1's post-completion delay - polling the + // observable state instead of a fixed sleep, which is nondeterministic on slow/ + // contended CI. The queue's delay bookkeeping uses the injected now(), not real + // time, so task2 stays parked in readyHeap. Kept inside the try so the finally + // still runs q.destroy() if this ever times out. + const deadline = Date.now() + 2000; + while (!(results.includes("task1") && q.readyHeap.size() === 1 && q.pendingHeap.size() === 0)) { + if (Date.now() > deadline) { + throw new Error("task1 did not complete / task2 did not reach readyHeap within 2000ms"); + } + await new Promise((resolve) => setTimeout(resolve, 5)); + } + + // task2 is now sitting in readyHeap, blocked by task1's post-completion delay. expect(q.readyHeap.size()).toBe(1); expect(q.pendingHeap.size()).toBe(0); expect(q.nextAvailableTime).toBeGreaterThan(0); From 4f96129594a977e0785ec8fb2de94cdc63b733f7 Mon Sep 17 00:00:00 2001 From: Shinrai Date: Sat, 8 Aug 2026 20:16:27 -0700 Subject: [PATCH 24/25] 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 25/25] 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):");