Skip to content

Refuse ambient mutation with a lint rule #804

Description

@taras

Story

As someone changing how a command writes its output, I want a lint rule that
refuses ambient mutation, so a test cannot quietly replace a stream, a global or
a console method that every other test in the process is also using.

Current gap

Nothing stops a module from assigning to the environment it runs in. One test
helper did:

// packages/cli/tests/plan-cli.test.ts, before #803
process.stdout.write = ((chunk: string | Uint8Array) => {
  chunks.push(typeof chunk === "string" ? chunk : new TextDecoder().decode(chunk));
  return true;
}) as typeof process.stdout.write;

That is one assignment, and it cost three separate things:

  • It stood in for a stream without honouring the stream's contract.
    write(chunk[, encoding][, callback]) calls back when the stream has taken
    the bytes. This double never called back. So every case that drove xmd plan
    proved its behaviour against something no real stdout does.
  • It hid a production defect for as long as it existed. The command wrote
    with the fire-and-forget one-argument form, which is exactly the Make piped xmd syntax output complete #715 defect —
    a piped program past the pipe buffer loses its tail, silently, exit 0. No plan
    case could see that, because the double accepted everything instantly.
  • It turned the fix into a hang. When the command started waiting for the
    callback (Deliver every whole CLI result before exit #803), the double never delivered one, and the tiers failed with
    Promise resolution is still pending but the event loop has already resolved.
    The symptom named neither the stub nor the stream.

The seam to do this properly already existed and was already the documented
convention. PlanDependencies.progress is described as "the entrypoint's facts
about its own process.stderr … Nothing here detects a runtime or inspects a
terminal"
, with the harness capturing chunks and refuseProgress standing
where a broken pipe would. Stdout simply had no counterpart, so the test reached
around the design instead of through it. #803 adds deliver() beside
progress, and the mutation is gone — but nothing prevents the next one.

Contract

  • A rule local/no-ambient-mutation reports assignment to state the module does
    not own: members of process (notably process.stdout/stderr and their
    methods), globalThis, Deno, and console.
  • It reports the assignment, and names the alternative: take the capability as a
    dependency, or compose it as middleware.
  • Recognition is by binding rather than by spelling, the way
    local/no-sync-filesystem resolves Deno and node:fs — a local named
    process that the module declared itself is not the global.
  • Restoring in a finally or an ensure() does not make it acceptable: the
    window is still shared with everything else running in the process, which is
    why the verification battery cannot be run concurrently (Make the full verification battery safe to run concurrently #279).
  • The rule is "error" in .oxlintrc.json, with no test-file override: a test
    is the place this happens, so exempting tests would exempt the whole problem.

Acceptance

  • The rule reports the pre-Deliver every whole CLI result before exit #803 plan-cli.test.ts helper, and the message sends
    the reader to a dependency or middleware.
  • It reports assignment to process.env.X, globalThis.X, console.log and a
    method of Deno.stdout.
  • It does not report assignment to a local binding that merely shares a name,
    nor reading any of these.
  • deno task lint stays green on main with the rule enabled, or every site it
    finds is fixed in the same change.

Out of scope

  • Changing how any command writes today. Deliver every whole CLI result before exit #803 owns CLI output delivery.
  • Writing to a stream. The subject is assignment, not use: the service hosts
    that hand a child's bytes to process.stdout are calling a stream, not
    replacing one, and stay untouched.
  • Reaching for a global to read it. process.stdout.isTTY is a fact the
    entrypoint states; deciding whether that should also become a dependency is
    its own question.

Relationships

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions