fix: namespace dev condition to holdmytask-dev; correct + test devcheck by cldmv-bot[bot] · Pull Request #12 · CLDMV/holdmytask

cldmv-bot · 2026-08-08T04:42:35Z

🚀 What's Changed

💥 Breaking Changes

No breaking changes

✨ Features

No new features

🐛 Bug Fixes

fix(review): advance scanner past consumed --conditions value; drop invalid -C= form (fix: namespace dev condition to holdmytask-dev; correct + test devcheck #12) (4f96129)
fix(review): parse --conditions as whole literal values (no comma/pipe split) (fix: namespace dev condition to holdmytask-dev; correct + test devcheck #12) (a959151)
fix: remove vestigial ./devcheck export (points at unshipped file) (fix: namespace dev condition to holdmytask-dev; correct + test devcheck #12) (f3f39ef)
fix(review): exact --conditions match, drop NODE_ENV override, keep module in ssr conditions (fix: namespace dev condition to holdmytask-dev; correct + test devcheck #12) (2af4c55)
fix(devcheck): keep condition-only trigger + detect execArgv; drop uuid's NODE_ENV logic (fix: namespace dev condition to holdmytask-dev; correct + test devcheck #12) (d2c6896)
fix: namespace dev condition to holdmytask-dev; correct + test devcheck (fix: namespace dev condition to holdmytask-dev; correct + test devcheck #12) (2240dfd)

📦 Dependencies

No dependency updates

🔧 Other Changes

docs(review): soften devcheck message - don't assert it's "loading from dist/" (fix: namespace dev condition to holdmytask-dev; correct + test devcheck #12) (ee9235b)
docs(review): drop stale test.env reference in vitest config comment (fix: namespace dev condition to holdmytask-dev; correct + test devcheck #12) (cc37aa2)
refactor(devcheck): align to the @cldmv/uuid reference pattern (fix: namespace dev condition to holdmytask-dev; correct + test devcheck #12) (c6427b1)

👥 Contributors

@Shinrai

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.

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).

…id'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).

Copilot

Pull request overview

This PR updates the project’s “dev condition” mechanism to use a namespaced conditional export (holdmytask-dev) and aligns dev environment checks and Vitest configuration so local development and tests resolve the src/ entry intentionally (rather than accidentally via a generic development condition).

Changes:

Renames the ./main conditional export from development to holdmytask-dev.
Updates devcheck.mjs to detect the condition via NODE_OPTIONS and process.execArgv, and to skip when installed under node_modules.
Adds a dedicated Vitest test for devcheck behavior and updates Vitest config to include the new condition.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
`package.json`	Switches the `./main` conditional export to `holdmytask-dev`.
`devcheck.mjs`	Updates condition detection logic and adds an “installed package” guard.
`.configs/vitest.config.mjs`	Adds `holdmytask-dev` to resolver conditions and propagates it to Vitest workers.
`tests/DevCheck.test.vitest.mjs`	Adds coverage for devcheck behavior across env/execArgv/CI/installed scenarios.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…odule 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.

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.)

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

devcheck.mjs:62

collect() splits condition strings on | as well as ,. For Node's --conditions/NODE_OPTIONS the separator is comma; | is a valid character in a condition token (and is used in Vite's default development|production marker). Splitting on | can create false negatives, e.g. NODE_OPTIONS=--conditions=holdmytask-dev|production would silence devcheck even though Node would treat that as a single condition (so the holdmytask-dev export would NOT be selected and the developer would still be on dist/).

	const conditions = [];
	const collect = (value) => {
		if (value) for (const c of value.split(/[,|]/)) if (c.trim()) conditions.push(c.trim());
	};

tests/DevCheck.test.vitest.mjs:69

devcheck.mjs supports parsing --conditions <value> (space-separated) from argv/NODE_OPTIONS, but the test suite only covers the --conditions=<value> form. Adding a test for the space-separated form helps prevent regressions in the token-scanning logic.

	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("");
	});

…e 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.

Shinrai · 2026-08-09T00:09:32Z

Addressing the 2 suppressed comments from the latest review — both fixed in a959151:

devcheck.mjs:62 — splitting condition values on ,/|. Correct, and it was a real false-negative: I verified Node treats each --conditions occurrence as one literal condition and does not split on comma or pipe (--conditions=holdmytask-dev|production resolves to dist/, not src), so my split would have silenced devcheck while the developer was actually on dist/. Removed the splitting — each value is now collected whole and matched exactly. Fixed the test that wrongly asserted a comma-joined value silences it (now asserts it nags), and added pipe-joined-nag coverage.
tests/DevCheck.test.vitest.mjs:69 — only --conditions=<value> covered. Added a space-separated (--conditions holdmytask-dev) case and a repeated-flag case.

Also added the standard project header block to the test file (it was missing it vs the other test files).

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

.configs/vitest.config.mjs:17

Comment references test.env carrying the dev condition into workers, but this config no longer sets test.env. This is misleading for future maintenance; update the comment to only reference test.nodeOptions (or reintroduce test.env if intended).

	// 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.

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.

Shinrai · 2026-08-09T00:56:26Z

Addressing the suppressed comment from the latest review (.configs/vitest.config.mjs:17 — comment still references test.env, which the config no longer sets):

Fixed in cc37aa2. The comment now references only test.nodeOptions (which is what actually carries the condition into workers); the stale test.env mention is gone.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

devcheck.mjs:72

scan() collects the value after --conditions/-C, but it does not advance the loop index. This can incorrectly re-interpret the value token (e.g. --conditions --conditions=holdmytask-dev would be treated as enabling holdmytask-dev even though Node’s literal condition would be "--conditions=holdmytask-dev"). Increment i after consuming the next token and use continue to avoid double-processing.

	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]);
			} 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));
			}
		}
	};

devcheck.mjs:70

The updated condition parsing explicitly supports Node’s short flag -C / -C=..., but the new test suite doesn’t cover these cases. Adding coverage would help prevent regressions in the argument scanner across Node versions/runners that prefer -C.

			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));
			}

…nvalid -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=`).

Shinrai · 2026-08-09T03:18:25Z

Addressing the 2 suppressed comments from the latest review — both fixed in 4f96129:

devcheck.mjs:72 — scanner didn't advance past a consumed value. The space-form branch (--conditions x / -C x) pushed tokens[i+1] but didn't increment i, so a value that itself looks like a flag (e.g. --conditions --conditions=x) got double-processed. Now advances i past the consumed token.
devcheck.mjs:70 — no -C coverage. Added a -C short-flag test. In doing so I found Node rejects -C=value ("bad option"), so I also dropped the unreachable -C= branch — the valid forms are --conditions=x, --conditions x, and -C x.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

devcheck.mjs:87

The message claims holdmytask "is loading from dist/" when the condition is not set. That isn’t necessarily true when devcheck.mjs is run directly (as in the test fixtures), and can be misleading in an unbuilt checkout. Consider wording this as what will/would resolve by default instead of asserting the current runtime is definitely using dist/.

		console.error("📁 Source folder detected but the 'holdmytask-dev' condition is not set,");
		console.error("   so holdmytask is loading from dist/ instead of src/.");

…om 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/.

Shinrai · 2026-08-09T04:19:38Z

Addressing the suppressed comment from the latest review (devcheck.mjs:87 — the message asserts "loading from dist/", which isn't necessarily true standalone / in an unbuilt checkout):

Fixed in ee9235b. Reworded to "imports resolve to dist/ by default (or fail if it isn't built) instead of src/" — describes the default resolution rather than asserting the current runtime is definitely on dist/. Devcheck tests still green (15/15).

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Shinrai added 2 commits August 7, 2026 22:41

Shinrai requested a lite review from Copilot August 8, 2026 15:56

Copilot started reviewing on behalf of Shinrai August 8, 2026 15:56 View session

Copilot AI reviewed Aug 8, 2026

View reviewed changes

Comment thread .configs/vitest.config.mjs Outdated

Comment thread .configs/vitest.config.mjs Outdated

Comment thread package.json

Comment thread devcheck.mjs

Shinrai mentioned this pull request Aug 8, 2026

./devcheck export points to a file not in the published files allowlist #13

Closed

Shinrai requested a lite review from Copilot August 8, 2026 18:09

Copilot started reviewing on behalf of Shinrai August 8, 2026 18:10 View session

Copilot AI reviewed Aug 8, 2026

View reviewed changes

Shinrai requested a lite review from Copilot August 9, 2026 00:13

Copilot started reviewing on behalf of Shinrai August 9, 2026 00:14 View session

Copilot AI reviewed Aug 9, 2026

View reviewed changes

Shinrai requested a lite review from Copilot August 9, 2026 03:07

Copilot started reviewing on behalf of Shinrai August 9, 2026 03:07 View session

Copilot AI reviewed Aug 9, 2026

View reviewed changes

Shinrai mentioned this pull request Aug 9, 2026

fix(devcheck): condition-only trigger + execArgv detection + scoped-install guard CLDMV/uuid#10

Merged

Shinrai requested a lite review from Copilot August 9, 2026 04:10

Copilot started reviewing on behalf of Shinrai August 9, 2026 04:11 View session

Copilot AI reviewed Aug 9, 2026

View reviewed changes

Shinrai requested a lite review from Copilot August 9, 2026 04:38

Copilot started reviewing on behalf of Shinrai August 9, 2026 04:38 View session

Copilot AI reviewed Aug 9, 2026

View reviewed changes

Shinrai approved these changes Aug 9, 2026

View reviewed changes

Shinrai merged commit 6ed70af into next Aug 9, 2026
25 checks passed

cldmv-bot Bot deleted the fix/devcheck-normal-module branch August 9, 2026 04:54

cldmv-bot Bot mentioned this pull request Aug 9, 2026

release: v2.0.0 - mark HoldMyTask default/alias export change as breaking #7

Merged

Navigation Menu

Uh oh!

Uh oh!

fix: namespace dev condition to holdmytask-dev; correct + test devcheck - #12

fix: namespace dev condition to holdmytask-dev; correct + test devcheck#12
Shinrai merged 9 commits into
nextfrom
fix/devcheck-normal-module

cldmv-bot Bot commented Aug 8, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Shinrai commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Shinrai commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Shinrai commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Shinrai commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

cldmv-bot Bot commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 What's Changed

💥 Breaking Changes

✨ Features

🐛 Bug Fixes

📦 Dependencies

🔧 Other Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Shinrai commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Shinrai commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Shinrai commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Shinrai commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cldmv-bot Bot commented Aug 8, 2026 •

edited

Loading