fix(devcheck): condition-only trigger + execArgv detection + scoped-install guard - #10
Merged
Merged
Conversation
…nstall guard Backports the corrected devcheck from @cldmv/holdmytask (CLDMV/holdmytask#12); addresses the bugs filed in #9. The `uuid-dev` namespacing was already correct; the check logic around it was not. - Trigger is now condition-only. Only `--conditions=uuid-dev` selects src/ (via the ./main export); NODE_ENV does not affect resolution, so the old `!nodeEnv || (!["","development"].includes(nodeEnv) && !hasUUIDDev)` was wrong both ways: NODE_ENV=development without the condition stayed silent while actually on dist/ (false negative), and the condition set without NODE_ENV nagged though correctly on src/ (false positive). - Detect the condition in process.execArgv as well as NODE_OPTIONS. `node --conditions=uuid-dev file.mjs` puts it in execArgv, and that's how vitest passes it to workers (worker NODE_OPTIONS is undefined; execArgv carries the flag) - so the NODE_OPTIONS-only check missed the CLI/worker form. - Skip when installed as a dependency, detecting a `node_modules` segment anywhere above the file so it works for the scoped `node_modules/@cldmv/uuid` layout (a "parent === node_modules" check would miss it). Guards against a git/tarball install exiting inside a consumer's app. - Fix the copy-paste `@Project: @cldmv/slothlet` header to `@cldmv/uuid`. - Add tests/DevCheck.test.vitest.mjs (9 cases): nags without the condition; silent with it via NODE_OPTIONS and via execArgv; NODE_ENV=development alone does not silence; still nags after a dist/ build; generic development condition rejected; skips in CI; skips when installed as a scoped dep; does nothing with no src/. The nag remains intentional whenever src/ is present and the condition is unset (even after a build): a source checkout should run from src/ via the condition, so flagging that you're silently on dist/ is the point.
cldmv-bot
Bot
added
! fix → next
Contributor
|
Resolves #9 — closed when this ships to the default branch. |
There was a problem hiding this comment.
Pull request overview
This PR refines the devcheck.mjs “source-checkout guard” so it only triggers when the uuid-dev condition is missing (including when passed via process.execArgv), and adds a Vitest suite to validate behavior across common execution contexts (CI, NODE_OPTIONS, execArgv, installed under node_modules/).
Changes:
- Update
devcheck.mjsto: (1) ignoreNODE_ENV, (2) detectuuid-devvia bothNODE_OPTIONSandprocess.execArgv, and (3) skip when running from an installed package path containingnode_modules. - Add
tests/DevCheck.test.vitest.mjsto rundevcheck.mjsin isolated fixture directories and assert expected exit codes/output.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
devcheck.mjs |
Switches devcheck gating to condition-based detection (NODE_OPTIONS + execArgv) and skips execution when run from installed dependency paths. |
tests/DevCheck.test.vitest.mjs |
Adds an isolated subprocess-based test suite covering the new devcheck triggering/skipping rules. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Shinrai
added a commit
to CLDMV/holdmytask
that referenced
this pull request
Aug 8, 2026
…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.
Addresses PR #10 Copilot review: `flags.includes("uuid-dev")` would false-positive on any flag value containing that substring (e.g. `--conditions=not-uuid-dev`), spuriously silencing devcheck. Parse the actual `--conditions` values from execArgv and NODE_OPTIONS (handling `=`/space/`-C`/ comma forms) and match `uuid-dev` exactly. Added regression tests: rejects a substring-containing condition; accepts uuid-dev among comma-separated conditions.
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/uuid/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. Same cleanup as CLDMV/holdmytask.
cldmv-bot
Bot
added
the
type: dependencies
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
devcheck.mjs:2
- The file header’s
@Projectvalue was changed to@cldmv/uuid, but the rest of the repository headers (and thetools/fix-headers.mjsconfig) still use@cldmv/slothlet. This makes devcheck.mjs inconsistent with the established header convention and likely to get rewritten by the header-fixing tooling. Either migrate headers/tooling repo-wide in a dedicated change, or keep this file consistent for now.
* @Project: @cldmv/uuid
devcheck.mjs:62
--conditionsvalues are comma-separated by Node. Splitting on|here can create false positives (e.g.--conditions=foo|uuid-devwould be treated as enablinguuid-devby devcheck, but Node would treatfoo|uuid-devas a single condition value). This can incorrectly silence the devcheck warning.
const collect = (value) => {
if (value) for (const c of value.split(/[,|]/)) if (c.trim()) conditions.push(c.trim());
};
tests/DevCheck.test.vitest.mjs:3
- All other test files under
tests/include the standard project/file header block, but this new test file does not. If header checks (or thenpm run fix:headerstooling) are part of the repo workflow, this file will be an outlier and may fail validation or get rewritten later.
import { test, expect, describe, beforeAll, afterAll } from "vitest";
import { spawnSync } from "node:child_process";
import { mkdtempSync, mkdirSync, copyFileSync, rmSync } from "node:fs";
…r; add test header Addresses the second Copilot re-review on PR #10 (3 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=uuid-dev,x` and `--conditions=uuid-dev|production` do NOT enable uuid-dev). The old split caused a false negative - e.g. `uuid-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), added pipe-joined nag, space-separated (`--conditions uuid-dev`) and repeated-flag silent cases. - devcheck.mjs: reverted the `@Project` header to `@cldmv/slothlet` to match the rest of the repo and tools/fix-headers.mjs (`projectName: @cldmv/slothlet`); a single-file change to `@cldmv/uuid` is an outlier that fix:headers would revert. The repo-wide @project correction (config + restamp of all files) is a separate, dedicated change. - tests/DevCheck.test.vitest.mjs: added the standard project header block so it isn't an outlier vs the other test files.
Contributor
|
Addressing the 3 suppressed comments from the latest review — fixed in
|
…wide The header @project across the repo was a copy-paste leftover from the slothlet template, and tools/fix-headers.mjs was hardcoded to projectName "@cldmv/slothlet" so `npm run fix:headers` kept re-stamping it. Set projectName to "@cldmv/uuid" and ran fix:headers, correcting @project on all files (and re-stamping @Last-modified). This also makes devcheck.mjs's header consistent rather than an outlier (the earlier single-file change was reverted for exactly that reason). Folded into this PR per maintainer request.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.configs/tsconfig.dts.jsonc:16
- This file now contains two consecutive header blocks (the newly added /** ... / header and the pre-existing / ... */ header). Keeping both is redundant and can confuse tooling that expects a single canonical header at the top of each file.
*/
/*
* @Project: @cldmv/uuid
* @Filename: /.configs/tsconfig.dts.jsonc
The repo-wide fix:headers run added a second header block to .configs/tsconfig.dts.jsonc: the file's pre-existing header used a legacy single-star `/* ... */` block, which @cldmv/fix-headers does not recognize as its managed header (it manages the JSDoc `/** ... */` form), so it added a new `/** */` block on top instead of updating in place. Removed the legacy `/* */` block, leaving a single recognized `/** */` header. Verified fix:headers is now idempotent on the file (dry-run reports 0 updates). Addresses the suppressed Copilot comment on PR #10.
Contributor
|
Addressing the suppressed comment from the latest review ( Fixed in |
Shinrai
approved these changes
Aug 9, 2026
This was referenced Aug 9, 2026
Contributor
|
Heads up — pushed Copilot's latest review here didn't flag these — they were only surfaced on #12 — but since it's the same code I fixed it here too rather than leave the two repos divergent. Note this new commit likely dismisses the prior approval; a re-approve may be needed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚀 What's Changed
💥 Breaking Changes
No breaking changes
✨ Features
No new features
🐛 Bug Fixes
📦 Dependencies
No dependency updates
🔧 Other Changes
👥 Contributors