Skip to content

Commit c63db22

Browse files
branchseerclaude
andauthored
fix(plan): move FORCE_COLOR fallback after pattern filtering (#379)
## Motivation The change respects the principle that cached output should always be colored by default (for consistency), while allowing users who need different behavior (e.g., `FORCE_COLOR=0` for tools that misbehave with ANSI codes) to opt in explicitly via their task configuration. ## Summary Changes how `FORCE_COLOR` is handled during environment variable resolution. Instead of pre-injecting `FORCE_COLOR=1` before pattern filtering (which forced it through regardless of user intent), the fallback is now applied *after* pattern filtering. This allows users to opt in to passing through their own `FORCE_COLOR` value via task config while still defaulting to colored output when not explicitly configured. ## Key Changes - **Move fallback timing**: `FORCE_COLOR=1` is now inserted after pattern filtering using `entry().or_insert_with()`, so it only applies when the user hasn't opted in to passing the variable through - **Remove from DEFAULT_UNTRACKED_ENV**: `FORCE_COLOR` is no longer in the default untracked env list, giving users explicit control via `env` or `untrackedEnv` config - **Update test suite**: Renamed and rewrote tests to reflect the new behavior: - `test_force_color_defaults_to_one_when_user_does_not_opt_in` — validates fallback when user doesn't list `FORCE_COLOR` - `test_force_color_defaults_to_one_when_absent_from_parent` — validates fallback when parent env has no `FORCE_COLOR` - `test_force_color_passthrough_when_user_opts_in_via_untracked` — validates parent value passes through when listed in `untrackedEnv` - `test_force_color_passthrough_when_user_opts_in_via_fingerprinted` — validates parent value passes through and is fingerprinted when listed in `env` - `test_force_color_fallback_fingerprinted_when_opted_in_but_parent_absent` — validates fallback is recorded in fingerprint when user opts in but parent has no value - **Update documentation**: Comments in both `envs.rs` and `config/mod.rs` clarify the new opt-in model and that users can override `FORCE_COLOR` by listing it in task config https://claude.ai/code/session_01JZDXjffYu9W5qykGdmkUXD --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 51e35ea commit c63db22

2 files changed

Lines changed: 64 additions & 34 deletions

File tree

crates/vite_task_graph/src/config/mod.rs

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -422,15 +422,14 @@ pub const DEFAULT_UNTRACKED_ENV: &[&str] = &[
422422
"LIBPATH",
423423
// Terminal/display
424424
//
425-
// The only color-related var allowed through by default is `FORCE_COLOR`,
426-
// which the planner pre-injects with value `1` before env resolution so
427-
// cached output is always colored. The reporter strips colors at the
428-
// writer level when the user's terminal cannot render them. Other
429-
// color-related vars (`NO_COLOR`, `COLORTERM`, `TERM`, `TERM_PROGRAM`)
430-
// are intentionally NOT included — users may opt in to passing them
431-
// through via a task's `env`/`untrackedEnv` config.
425+
// No color-related vars are included by default. The planner ensures
426+
// `FORCE_COLOR=1` is set on the child after env resolution (as a fallback
427+
// when neither the parent env nor task config provides one), so cached
428+
// output is always colored. The reporter strips colors at the writer
429+
// level when the user's terminal cannot render them. Users wanting to
430+
// pass through `NO_COLOR`, `COLORTERM`, `TERM`, `TERM_PROGRAM`, or
431+
// override `FORCE_COLOR` can opt in via a task's `env`/`untrackedEnv`.
432432
"DISPLAY",
433-
"FORCE_COLOR",
434433
// Temporary directories
435434
"TMP",
436435
"TEMP",

crates/vite_task_plan/src/envs.rs

Lines changed: 57 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -76,16 +76,15 @@ impl EnvFingerprints {
7676
/// Before the call, `all_envs` is expected to contain all available envs.
7777
/// After the call, it will be modified to contain only envs to be passed to the execution (fingerprinted + untracked).
7878
///
79-
/// `FORCE_COLOR` is pre-inserted with value `"1"` so cached output is
80-
/// always colored. Because `FORCE_COLOR` is part of `DEFAULT_UNTRACKED_ENV`,
81-
/// the pattern filter below keeps it; its value (`"1"`) is left untracked
82-
/// (not part of the cache fingerprint).
79+
/// After pattern filtering, `FORCE_COLOR=1` is inserted as a fallback if
80+
/// nothing else set it, so cached output is always colored by default.
81+
/// Tasks that need a different value (e.g. `FORCE_COLOR=0` to suppress
82+
/// ANSI for a misbehaving tool) can opt in to passthrough by listing
83+
/// `FORCE_COLOR` in `env` or `untrackedEnv`.
8384
pub fn resolve(
8485
all_envs: &mut FxHashMap<Arc<OsStr>, Arc<OsStr>>,
8586
env_config: &EnvConfig,
8687
) -> Result<Self, ResolveEnvError> {
87-
all_envs.insert(OsStr::new("FORCE_COLOR").into(), Arc::<OsStr>::from(OsStr::new("1")));
88-
8988
// Collect all envs matching fingerprinted or untracked envs in env_config
9089
*all_envs = resolve_envs_with_patterns(
9190
all_envs.iter(),
@@ -97,6 +96,14 @@ impl EnvFingerprints {
9796
.collect::<Vec<&str>>(),
9897
)?;
9998

99+
// Ensure cached output is colored by default. Skipped if the user
100+
// opted into passing `FORCE_COLOR` through (via `env` / `untrackedEnv`)
101+
// and the parent supplied a value — in that case the user's choice
102+
// wins, even `FORCE_COLOR=0`.
103+
all_envs
104+
.entry(Arc::<OsStr>::from(OsStr::new("FORCE_COLOR")))
105+
.or_insert_with(|| Arc::<OsStr>::from(OsStr::new("1")));
106+
100107
// Resolve fingerprinted envs
101108
let mut fingerprinted_envs = BTreeMap::<Str, Arc<str>>::new();
102109
if !env_config.fingerprinted_envs.is_empty() {
@@ -223,14 +230,13 @@ mod tests {
223230
}
224231

225232
#[test]
226-
fn test_force_color_always_set_to_one() {
227-
// `FORCE_COLOR=1` is pre-injected before pattern filtering so cached
228-
// output is always colored. Because the merged untracked-env list
229-
// (config resolution adds DEFAULT_UNTRACKED_ENV, which includes
230-
// `FORCE_COLOR`) keeps it, the child sees `FORCE_COLOR=1` regardless
231-
// of the parent's value.
233+
fn test_force_color_defaults_to_one_when_user_does_not_opt_in() {
234+
// The user did not list `FORCE_COLOR` in `env` or `untrackedEnv`, so
235+
// the parent's value is filtered out by the pattern step. The
236+
// post-resolution fallback then inserts `FORCE_COLOR=1` so cached
237+
// output is colored.
232238
let mut all_envs = create_test_envs(vec![("PATH", "/usr/bin"), ("FORCE_COLOR", "2")]);
233-
let env_config = create_env_config(&[], &["PATH", "FORCE_COLOR"]);
239+
let env_config = create_env_config(&[], &["PATH"]);
234240

235241
let _result = EnvFingerprints::resolve(&mut all_envs, &env_config).unwrap();
236242

@@ -241,32 +247,57 @@ mod tests {
241247
}
242248

243249
#[test]
244-
fn test_force_color_dropped_when_pattern_does_not_allow_it() {
245-
// The resolver itself only pre-injects; it does not force-keep
246-
// `FORCE_COLOR` through the filter. Real callers always provide
247-
// patterns that include `FORCE_COLOR` (via `DEFAULT_UNTRACKED_ENV`),
248-
// but this test pins the contract: if `FORCE_COLOR` is absent from
249-
// the merged pattern list, the filter drops it.
250+
fn test_force_color_defaults_to_one_when_absent_from_parent() {
251+
// Parent env has no `FORCE_COLOR` at all. The fallback still inserts
252+
// `FORCE_COLOR=1` so the child emits colored output.
250253
let mut all_envs = create_test_envs(vec![("PATH", "/usr/bin")]);
251254
let env_config = create_env_config(&[], &["PATH"]);
252255

253256
let _result = EnvFingerprints::resolve(&mut all_envs, &env_config).unwrap();
254257

255-
assert!(!all_envs.contains_key(OsStr::new("FORCE_COLOR")));
258+
assert_eq!(all_envs.get(OsStr::new("FORCE_COLOR")).unwrap().to_str().unwrap(), "1");
259+
}
260+
261+
#[test]
262+
fn test_force_color_passthrough_when_user_opts_in_via_untracked() {
263+
// If the user lists `FORCE_COLOR` in `untrackedEnv`, the parent's
264+
// value passes through verbatim and the fallback is skipped.
265+
let mut all_envs = create_test_envs(vec![("FORCE_COLOR", "0")]);
266+
let env_config = create_env_config(&[], &["FORCE_COLOR"]);
267+
268+
let result = EnvFingerprints::resolve(&mut all_envs, &env_config).unwrap();
269+
270+
assert_eq!(all_envs.get(OsStr::new("FORCE_COLOR")).unwrap().to_str().unwrap(), "0");
271+
assert!(!result.fingerprinted_envs.contains_key("FORCE_COLOR"));
256272
}
257273

258274
#[test]
259-
fn test_force_color_value_one_overrides_user_fingerprinted_value() {
260-
// A user can list `FORCE_COLOR` as a fingerprinted env, but the
261-
// pre-injection still wins — fingerprint records `"1"`, not the
262-
// parent's value. (`FORCE_COLOR` is the colour-pipeline contract;
263-
// users wanting a different colour level should configure the tool
264-
// they're running, not the runner.)
275+
fn test_force_color_passthrough_when_user_opts_in_via_fingerprinted() {
276+
// If the user lists `FORCE_COLOR` in `env` (fingerprinted), the
277+
// parent's value passes through and is recorded in the cache key.
265278
let mut all_envs = create_test_envs(vec![("FORCE_COLOR", "3")]);
266279
let env_config = create_env_config(&["FORCE_COLOR"], &[]);
267280

268281
let result = EnvFingerprints::resolve(&mut all_envs, &env_config).unwrap();
269282

283+
assert_eq!(all_envs.get(OsStr::new("FORCE_COLOR")).unwrap().to_str().unwrap(), "3");
284+
assert_eq!(
285+
result.fingerprinted_envs.get("FORCE_COLOR").map(std::convert::AsRef::as_ref),
286+
Some("3")
287+
);
288+
}
289+
290+
#[test]
291+
fn test_force_color_fallback_fingerprinted_when_opted_in_but_parent_absent() {
292+
// User opts in to `FORCE_COLOR` as fingerprinted, but parent has no
293+
// value. The fallback supplies `1`, and because the fingerprint scan
294+
// runs after the fallback, `1` is recorded in the cache key — keeping
295+
// the fingerprint consistent with what the child actually sees.
296+
let mut all_envs = create_test_envs(vec![]);
297+
let env_config = create_env_config(&["FORCE_COLOR"], &[]);
298+
299+
let result = EnvFingerprints::resolve(&mut all_envs, &env_config).unwrap();
300+
270301
assert_eq!(all_envs.get(OsStr::new("FORCE_COLOR")).unwrap().to_str().unwrap(), "1");
271302
assert_eq!(
272303
result.fingerprinted_envs.get("FORCE_COLOR").map(std::convert::AsRef::as_ref),

0 commit comments

Comments
 (0)