fix(vscode): stop forcing color when the project config disables it - #33
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3795cf6586
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eda80fa062
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The master hard-coded FORCE_COLOR=1 into the rstest worker env (as the upstream extension does), so a project whose config sets process.env.NO_COLOR = '1' at load time hit Node's "'NO_COLOR' env is ignored" warning in every pool process. Mirror the CLI's getForceColorEnv semantics instead (adaptation #9): the master injects FORCE_COLOR=1 into the composed spawn env only when the user set neither color standard (marking the injection), and the worker retracts the marked injection right after config load — the CLI's own decision point — when the config turned color off. A user-set FORCE_COLOR beside a config-set NO_COLOR still warns, exactly as the bare CLI does. Also apply the review cleanups from the same pass: versionOk now retires a root's not-installed latch too (dropping the paired installed() calls), versionMismatch gains the same re-raise dedupe as notInstalled, the missing-package verdict rides on RslintResolutionError instead of a partial code table, missingDependencyCauseOf moves to shared/ for the lint/fmt config loaders (#30), stackCommandTitle loses its single-value verb parameter and now also feeds the status-bar hover, the unused NormalizedConfigResult.reason discriminant is dropped, the lint core-selection failure wording is spelled once, and the worker spawn reuses one resolved rstest path and one IPC send callback.
…, color marker Three review findings, each verified before fixing: - The missing-cwd spawn refusal threw a plain Error, so callers re-logged the already-warned stale-project state as an error with a stack. The guard now throws ReportedRstestResolutionError (which gained an optional message), and the four catch sites above RstestApi share one logUnlessReported helper next to the class instead of re-deciding. - The re-raise dedupe in versionMismatch/notInstalled short-circuited before the package-state restatement, so a crash latched between two identical verdicts survived a retry that aborted before spawning. Both observations now fold into one #observePackageState that restates first and skips only the repaint. - retractForceColorIfDisabled left RSTACK_FORCE_COLOR_INJECTED in the env on the no-NO_COLOR path; the marker is now removed once the decision is complete, so pool processes and user test code never observe it.
eda80fa to
942ac51
Compare
…ding RPCs The delete-after-check handler logged the stale-project warning but closed the worker with birpc's default error, so pending RPCs rejected with a bare '[birpc] rpc is closed' and the callers' catches re-logged the failure. birpc's $close(customError) rejects pending calls with the given error; the cwd-gone branch now passes a ReportedRstestResolutionError carrying the same message, so logUnlessReported stays quiet — the same contract the pre-spawn guard already follows. The branch's race window (cwd deleted between the guard and spawn, with no 'spawn' event timing guarantee) has no deterministic unit test; the classification is covered by types and the existing spawn-failure suite.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f91ac30cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A config that assigns both FORCE_COLOR and NO_COLOR at load time owns the FORCE_COLOR value; the bare CLI, deciding after config load, leaves both intact. The retraction now removes FORCE_COLOR only while it still holds the injected '1', so pools keep a config-set force-color preference.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b9c988cb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Why
The rstest master hard-coded
FORCE_COLOR: '1'into the worker's spawn env (as the upstream extension does). A project whose config setsprocess.env.NO_COLOR = '1'at load time then runs every pool process with both variables set, and Node printsWarning: The 'NO_COLOR' env is ignored due to the 'FORCE_COLOR' env being set.once per process, interleaved with the test output.The bare CLI does not have this problem: its
getForceColorEnvdecides the color env lazily at pool-spawn time — after the config has loaded — and injects nothing when either standard is already set.What
stacks/test/shared/colorEnv.ts): the master injectsFORCE_COLOR=1into the composed spawn env only when the user set neitherFORCE_COLORnorNO_COLOR(marking the injection withRSTACK_FORCE_COLOR_INJECTED), and the worker retracts the marked injection right after config load — the CLI's own decision point — when the config turned color off. A user-setFORCE_COLORbeside a config-setNO_COLORstill warns, exactly as the bare CLI does.versionOknow also retires a root's not-installed latch (the pairedinstalled()calls go away),versionMismatchgains the same re-raise dedupe asnotInstalled, the missing-package verdict rides onRslintResolutionError.missingPackageinstead of a partial code table,missingDependencyCauseOfmoves toshared/for the lint/fmt config loaders (Classify config-import missing dependencies for lint and fmt #30),stackCommandTitleloses its single-value verb parameter and now also feeds the status-bar hover, the unusedNormalizedConfigResult.reasondiscriminant is dropped, the lint core-selection failure wording is spelled once, and the worker spawn reuses one resolved rstest path and one IPC send callback.Verified
pnpm lint && pnpm test:unitgreen (254 tests, includes newcolorEnvandmissingDependencysuites)VSCODE_CLI=1 pnpm test:e2e rstest lintgreen locally@rstest/core0.11.9 with a config settingNO_COLOR: two warnings (main + pool process) with the hard-coded injection, zero with the conditional inject + retract, tests still pass