Skip to content

feat(rules): prefer node:process over the process global, and sweep packages/cli/src - #348

Merged
thecodedrift merged 4 commits into
mainfrom
chore/node-process-rule
Sep 19, 2026
Merged

thecodedrift merged 4 commits into
mainfrom
chore/node-process-rule

Conversation

@thecodedrift

@thecodedrift thecodedrift commented Sep 18, 2026

Copy link
Copy Markdown
Member

What

Three commits, one PR, so the rule lands green:

  1. feat(rules): a Taskless ast-grep rule, prefer-node-process, under .taskless/rules/sg/prefer-node-process/, authored by following pnpm cli agent create-sg-rule (topic v5) against a fresh pnpm build of this tree.
  2. refactor(cli): the sweep. import process from "node:process"; added to every file under packages/cli/src the rule flagged. 27 files, not the 25 the issue estimated (measured by running the rule over the unswept tree: 91 findings across 27 files). One added line per file, two in the four files that had no Node built-in import yet (the import plus the blank line that separates the group). Nothing else changes in those files.
  3. feat(rules): two refinements from review. Shadowing is ancestor-scoped rather than file-scoped, and globalThis.process is always a finding.

What the rule catches

Two independent forms (a top-level any:), both severity: error, in packages/cli/src/**/*.ts:

1. A bare process identifier (kind: identifier, regex: ^process$) when the file has no import … from "node:process" (any import form: default, named, namespace; matched on the specifier string), unless an ancestor of the reference binds the name:

  • an enclosing function, function expression, generator, arrow, or method whose parameter list has a required or optional parameter named process (also the unparenthesised process => … arrow form);
  • an enclosing catch (process);
  • an enclosing statement_block, or the program itself, whose direct child is a const/let/var with a declarator named process, or an export of one.

Because the walk is over ancestors, a binding in a sibling scope does not count: function kill(process) {…} next to function cwd() { return process.cwd(); } reports the second, and { const process = …; } followed by process.cwd() outside the block reports the outside reach. Measured with ast-grep scan on each such fixture: exactly one finding, on the global reach only.

Property accesses (foo.process, { process: x }, settings.process) are property_identifier nodes, not identifier, so they never match. ESLint's n/prefer-global/process: ["error", "never"] is the reference behaviour. There is no fix: an import needs placement, so the finding names the file and the line gets added by hand.

2. globalThis.process, always. No import and no shadow excuses it: it is the same reach with the global spelled out, and it is never the module-graph dependency the rule asks for. The recipe's rule shape has one message per rule, so the note explains both forms rather than a per-match message.

What it does not catch (deliberately, and written into the rule's note)

  • Scope is structural, not semantic. ast-grep has no scope analysis, so the shadow check is "an ancestor node declares the name", which is what lexical scoping reduces to for these binding forms. Binding forms outside that list (a for (const process of …) head, a destructured parameter ({ process }), a class field) are not treated as shadows and a reach under them would be reported. None exist in packages/cli/src today.
  • import process from "process" (no node: prefix) is still flagged at the usage sites, because only the node:process specifier satisfies the rule. That is the convention the rest of our node: imports follow, so this is intended.
  • Scope is packages/cli/src/**/*.ts only. .github/scripts/*.cjs (CommonJS) are excluded and would not match the TypeScript parser anyway.

Tests

.tests/prefer-node-process-20260918-test.yml: 14 valid, 11 invalid. Beyond the four cases the issue asks for: destructuring (const { env } = process), a named import (import { env } from "node:process"), node:child_process (must not match), optional and single-arrow parameters, method parameters, catch (process), an exported const process, a reach from a nested arrow inside a shadowing function (valid), a shadowing parameter in one function with a global reach in another (invalid), a block-scoped const process with a reach outside the block (invalid), a catch (process) followed by a top-level reach (invalid), and globalThis.process bare, next to a node:process import, and under a shadowing parameter (all invalid).

Mutation standard, each run separately with the file restored between runs: moving any one of the six new invalid fixtures (sibling parameter, block const, catch-then-global, the three globalThis.process forms) into valid makes pnpm cli test report 1 test case(s) failed and exit 1. Regressing the rule itself to file-scoped shadowing (stopBy: end on the program declaration check) also fails the suite. Restored, it passes.

Why no changeset

A repo-local rule plus an import sweep changes nothing a consumer of @taskless/cli can observe: the built bundle externalises node:*, and pnpm build (whose assert-library-graphs plugin reads rollup's resolved chunk graph) still emits every entry, so no host-free entry picked up a Node import. skip-changeset label applied.

Verification (at the tip commit)

  • pnpm cli verify .taskless/rules/sg/prefer-node-process --jsonok: true
  • pnpm cli test .taskless/rules/sg/prefer-node-process --jsonok: true
  • pnpm cli check (with runtime rules) and pnpm cli check --anonymousNo issues found. (0 results)
  • pnpm typecheck → pass
  • pnpm lint (builds first, then pnpm cli check) → exit 0
  • pnpm --filter @taskless/cli test → 94 files, 1528 tests passed
  • git diff --stat origin/main → 29 files: 2 rule files + 27 swept sources, each with one added import line

Fixes #347

…:process

An ast-grep rule, prefer-node-process, reports a bare `process`
identifier in packages/cli/src when the file has no
`import … from "node:process"` and declares no binding named
`process` (const/let/var, parameter, or catch clause). A file that
shadows the name anywhere is treated as clean for the whole file, and
`globalThis.process` is a property access and out of scope. Reference
behaviour is ESLint n/prefer-global/process: ["error", "never"]. No
fix is attempted: an import needs placement.
Add `import process from "node:process";` to the 27 files under
packages/cli/src that reached for the global, placed in the Node
built-ins group per .conventions/STYLEGUIDE-CODE.md. Nothing else
changes in these files; this is what makes the prefer-node-process rule
land green.
@thecodedrift thecodedrift added the skip-changeset PR intentionally ships no release note (bypasses the changeset requirement) label Sep 18, 2026
…balThis.process

The shadow check now walks the reference's ancestors instead of the
whole file: an enclosing function, arrow, or method binding a
`process` parameter, an enclosing `catch (process)`, or an enclosing
block (or the file) whose direct child declares `process`, including
an exported one. A binding in a sibling scope no longer silences a
global reach elsewhere in the file.

`globalThis.process` is now an independent finding regardless of
imports or shadowing: it is the same reach with the global spelled out.
@thecodedrift

Copy link
Copy Markdown
Member Author

@claude /review

@claude

claude Bot commented Sep 18, 2026

Copy link
Copy Markdown

Claude finished @thecodedrift's task in 4m 27s —— View job


Review complete

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contained no reviewThreads and no reviews, only the original @claude /review trigger comment — so this is the first review pass on this PR, and there's nothing previously-raised to reconcile against.

  • Read prior review data (empty — first pass)
  • Fetch PR diff (gh pr diff 348)
  • Review the ast-grep rule (prefer-node-process.yml) and its 25-fixture test file
  • Review the 27-file node:process import sweep
  • Spot-check import placement/grouping across the swept files against .conventions/STYLEGUIDE-CODE.md
  • Post findings

Rule logic (prefer-node-process.yml): Traced both any: branches (globalThis.process, and the bare-identifier + ancestor-shadow check) by hand against every fixture in .tests/prefer-node-process-20260918-test.yml (14 valid / 11 invalid, counts match the PR description). The ancestor-scoped shadow walk (function/arrow/method params, catch (process), direct-child const/let/var in an enclosing block or program) checks out against every fixture I traced, including the sibling-scope, block-then-outside-reach, and catch-then-later-reach cases the PR calls out specifically. No deletions in this diff (pure additions), so there's no removed-behavior/regression surface to audit.

The 27-file sweep: Confirmed all 27 files add exactly one import process from "node:process"; line (two lines — plus a blank separator — in the four files that had no prior Node-builtin import group: index.ts, login-interactive.ts, color.ts, package-manager.ts). Checked import placement/grouping in every changed file against the "builtins → external → internal, blank-line separated" convention in .conventions/STYLEGUIDE-CODE.md — all consistent with each file's pre-existing style, nothing broken by the insertion. Grepped all touched files for any local variable/parameter literally named process that the new import could collide with or get shadowed by — none found. No function signatures or call sites change, so there's no cross-file blast radius to check.

One finding, posted inline (cleanup only, not a correctness bug):

  • .taskless/rules/sg/prefer-node-process/prefer-node-process.yml:32-33[New] The ignores: [".github/scripts/**"] entry is dead config: files already scopes the rule to packages/cli/src/**/*.ts, a path .github/scripts/** can never match, so the ignore can never fire either way. Rule behavior is unaffected; flagging only because it reads as intentional exclusion but does nothing.

Nothing else surfaced — the rule's documented limitations (destructured-parameter shadows, for...of head shadows, class fields, non-node:-prefixed process imports) are explicitly acknowledged in the rule's own note and PR body rather than gaps introduced by this diff.
(branch: chore/node-process-rule)

Comment thread .taskless/rules/sg/prefer-node-process/prefer-node-process.yml Outdated
@thecodedrift

Copy link
Copy Markdown
Member Author

Re: @claude[bot] — "Claude finished @thecodedrift's task — Review complete … One finding, posted inline."
#348 (comment)

Read and acknowledged. The one inline finding (unreachable ignores) is removed in 1bdf9f7; the thread carries the detail.

— AI Coding Agent

@thecodedrift
thecodedrift merged commit b1280ac into main Sep 19, 2026
5 checks passed
@thecodedrift
thecodedrift deleted the chore/node-process-rule branch September 19, 2026 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changeset PR intentionally ships no release note (bypasses the changeset requirement)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use node:process instead of the process global, enforced by a Taskless rule

1 participant