Skip to content

Read env toggles, non-empty env vars and the home directory through one utils::env module #727

Description

[agent] Filed by the scheduled architecture audit routine (CLI and core). Register: discussion #560 register comment.

Kind: refactor. Source: review 7.3 "Env truthiness" and 7.6 #7; register C19.

Problem

Three env helper families are each written several times, in core and the CLI, on 045d7ec.

1. Truthiness: three vocabularies.

  • Core gates accept only the exact strings "1" and "true": is_debug_enabled / is_offline_env in env_compat.rs#L4-L23, and a separate copy for SOCKET_TELEMETRY_DISABLED (plus VITEST == "true") in telemetry.rs#L134-L139.
  • socket_cli_config::env_truthy (socket_cli_config.rs#L52-L67) accepts 1|true|yes|on|y|t, trimmed and case-insensitive. update_notifier imports it.
  • The CLI's clap parser parse_bool_flag (args.rs#L67-L86) accepts the same wide set plus the falsy spellings.
  • update_notifier::in_ci (update_notifier.rs#L105-L116) adds a fourth rule: anything except 0/false.

The narrow core match is kept correct only because apply_env_toggles (args.rs#L538-L575) rewrites every truthy flag back into the env as "1". Its own doc comment records the bug this caused: SOCKET_OFFLINE=yes let telemetry fire. I checked on 045d7ec that all ten command entry points call the mirror, so there is no user-visible drift today. The hazard is the next core gate or entry point that misses it, which is exactly how the list airgap bug arose (test list_run_mirrors_global_toggles_for_airgap).

2. "Empty means unset": one private helper and about 29 inline copies. env_non_empty is private to socket_cli_config.rs#L45-L50.`` Production code repeats env::var(..).ok().filter(|v| !v.is_empty()) in about 29 places across 16 files, among them `update/state.rs`, `update/channel.rs`, `update/release.rs`, `policy/mod.rs`, `utils/concurrent.rs`, `redirect/upstream/client.rs` (6), `update_notifier.rs` (3) and `env_compat.rs` (3). Some call sites nest their own `fn env_dir` / `fn path_var` closures (`state.rs#L57-L62`, `channel.rs#L50-L65`).

3. Home directory: at least six resolvers in this area, which already disagree.

  • utils::fs::home_dir (fs.rs#L377-L394): HOME, then USERPROFILE, then a literal "~"; empty counts as unset.
  • policy::home_dir (policy/mod.rs#L786-L792): only USERPROFILE on Windows and only HOME elsewhere, canonicalized. Under Git Bash, where HOME is set on Windows, the socket.yml repo-root walk therefore stops at a different home than the crawlers and telemetry redaction use.
  • update/channel.rs (HOME, then USERPROFILE), update/state.rs and socket_cli_config::config_json_paths (each with its own XDG/LOCALAPPDATA fallback), and redirect/npmrc.rs (npmrc.rs#L330-L340,`` which keeps Some("")).
  • The crawlers keep further deliberate variants: go and composer are strict, python prefers USERPROFILE on Windows. Those belong to the sibling auditor's area and are listed only for context.

Impact

Maintenance risk rather than a live bug: every new env knob picks a vocabulary at random, and the mirror papers over the gap. The env-mirroring also appears in C10/RunCtx, which wants apply_env_toggles gone, and that becomes possible only once core reads the same vocabulary the CLI parses. Size: small.

Proposed change

Create socket_patch_core::utils::env with:

  • non_empty(name) -> Option<String> and non_empty_os(name) -> Option<OsString>;
  • truthy(name) -> bool, using the parse_bool_flag vocabulary, which moves here; the CLI's clap value_parser delegates to env::parse_bool;
  • home_dir() -> Option<PathBuf> (HOME → USERPROFILE, empty counts as unset) and the "~"-fallback form for the crawlers' probe use.

Then:

  • is_debug_enabled, is_offline_env and is_telemetry_disabled call env::truthy, and their two private "1"|"true" matches are deleted;
  • socket_cli_config::env_truthy / env_non_empty are deleted in favor of the new module;
  • update/state.rs, update/channel.rs and policy::home_dir use env::non_empty / env::home_dir, and their local closures are deleted. Whether policy keeps a Windows USERPROFILE-first order becomes one documented parameter, not a fork.

Not in this issue: deleting apply_env_toggles (that's C10), and changing in_ci's semantics (keep its rule, but name it there).

Size and scope

utils/{env.rs (new), env_compat.rs, socket_cli_config.rs, fs.rs}, telemetry.rs, update/{state,channel}.rs, policy/mod.rs, CLI args.rs and update_notifier.rs. Roughly −80/+60 production lines. Crawler home resolvers are out of scope; a follow-up can move them once this lands.

Acceptance criteria

  • One truthiness vocabulary in production code: grep finds no "1" | "true" env match outside utils::env.
  • New unit tests: SOCKET_OFFLINE=yes / TRUE / on make is_offline_env() true without apply_env_toggles, and the same for SOCKET_DEBUG and SOCKET_TELEMETRY_DISABLED.
  • parse_bool_flag_* tests in args.rs stay green against the moved parser.
  • home_dir_treats_empty_home_as_unset, the socket_cli_config XDG/HOME tests and the update-state tests stay green.
  • list_run_mirrors_global_toggles_for_airgap and the telemetry airgap tests stay green.

Dependencies

Blocks the "delete apply_env_toggles" step of C10 (RunCtx). Independent of #678 (undocumented SOCKET_API_CONCURRENCY), which may use env::non_empty.

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

    agent:triagedarch-auditFiled by a scheduled architecture audit routine (see the architecture review discussion)priority:p3refactorStructural change: duplicated code or logic, missing abstraction, layering, dead code

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions