env-vars: the environment read outside the declared edge

CHANGELOG.md

@@ -43,6 +43,21 @@
   listing what beamte does have, because a typo that silently disables a rule
   is the same quiet failure as a test that passes by being empty.
 
+- `env-vars`, an opt-in rule that reports code reading the process environment
+  where nothing declares it -- `std::env::var`, `os.environ`, `process.env`,
+  `ENV[...]`, `System.getenv`, `getenv` -- in the nine languages beamte's
+  `env-read` covers. An ambient read is configuration no signature admits to,
+  and no small test of that code can stay hermetic (*Test Sizes*, 2010-12-13).
+  `env-files` names the files that *are* the configuration edge, where reads
+  are licensed -- `theme-files` for the environment; everywhere else a read is
+  an error. Enable with `env-vars = true` or `--env-vars`. Opt-in for
+  `test-quality`'s reason: the first scan of a language downloads its grammar.
+  Shell is deliberately not covered, `$VAR` being the language's own variable
+  model, and Rust's compile-time `env!` is not a finding -- the build declares
+  those variables, which is the announced channel the rule steers reads toward.
+- `test-rules` now rejects a file-scoped beamte rule by name, pointing at
+  `env-vars` instead: listing `env-read` there would run it over test files
+  alone while looking like it ran everywhere.
 
 - [Update Straitjacket](https://straitjacket.dev/guides/updating), a guide for
   the thing every installed tool eventually needs and this one never documented:

Cargo.toml

@@ -34,7 +34,7 @@ wasmer = { version = "7", default-features = false, features = ["sys", "cranelif
 # `pure` selects the Rust implementation instead, and naming blake3 here is what
 # unifies that feature across the whole graph.
 blake3 = { version = "1", default-features = false, features = ["pure"] }
-beamte = "0.1"
+beamte = "0.2"
 clap = { version = "4", features = ["derive"] }
 ignore = "0.4"
 inventory = "0.3"
@@ -64,3 +64,13 @@ let_underscore_must_use = "deny"
 [profile.release]
 lto = true
 strip = true
+
+# beamte 0.2 (`env-read`, `Rule::scope`) is merged but not yet on crates.io.
+# Until it is, the requirement above resolves through this patch to beamte's
+# default branch, which carries it. Deliberately not a branch pin: the feature
+# branch was deleted when it merged, and a patch naming a branch that no longer
+# exists fails dependency resolution before a single check runs. Drop this
+# table the moment 0.2 is published -- the requirement is already written for
+# the registry, so deleting these lines is the whole change.
+[patch.crates-io]
+beamte = { git = "https://github.com/PowderworksCode/beamte" }

README.md

@@ -27,8 +27,11 @@ src/theme/button.css:12:14  [color]  #ff6600
 straitjacket: 1 error(s), 0 warning(s) across 84 file(s); 0 suppressed
 ```
 
-Eleven rules ship; nine run at the first invocation. The other two you opt
-into: `no-comments`, and `test-quality`, which reads your tests the way the
+Twelve rules ship; nine run at the first invocation. The other three you opt
+into: `no-comments`; `env-vars`, which flags code reading the process
+environment where nothing declares it (`std::env::var`, `os.environ`,
+`process.env`) outside the files you designate as the configuration edge; and
+`test-quality`, which reads your tests the way the
 language writes them — `#[test]`, `@Test`, `it(...)`, `TEST(...)`,
 `test "..."` — and flags the ones that weaken what they prove, such as a loop
 or a conditional in a test body. It parses with a

site/content/getting-started.md

@@ -37,7 +37,7 @@ straitjacket
 ```
 
 With no arguments, Straitjacket scans the current directory, honoring your
-`.gitignore`. Nine of the eleven rules are on by default — it runs near its
+`.gitignore`. Nine of the twelve rules are on by default — it runs near its
 max and you ratchet down later. The other two are modes you opt into:
 `no-comments`, and `test-quality`, which parses your tests with a downloaded
 grammar.

site/content/index.md

@@ -18,7 +18,7 @@ Run `straitjacket` at the root of any project. It honors your `.gitignore`,
 prints one line per finding as `path:line:col [rule] matched`, and exits
 non-zero on any error — so CI fails the moment slop lands.
 
-Nine of the eleven rules are on at the first run, so the strictest
+Nine of the twelve rules are on at the first run, so the strictest
 Straitjacket gets by default takes no configuration to reach. What you
 disagree with, you turn off — `--skip` for a run, `straitjacket.toml` for
 good, `straitjacket-allow` on the one line you meant. The other two you opt

site/content/reference/config-file.md

@@ -55,6 +55,8 @@ Every key mirrors a [CLI flag](/reference/cli) one-for-one, in
 | `test-rules` | list of [test rule](/reference/rules#test-quality) ids; unset runs all | — |
 | `test-quality` | boolean | `--test-quality` ([test quality](/reference/rules#test-quality)) |
 | `no-comments` | boolean | `--no-comments` ([no-comments mode](/reference/rules#no-comments-mode)) |
+| `env-vars` | boolean | `--env-vars` ([environment variables](/reference/rules#environment-variables)) |
+| `env-files` | list of files licensed to read the process environment — the declared [configuration edge](/reference/rules#environment-variables) | — |
 | `include-json` | boolean | `--include-json` |
 | `no-ignore` | boolean | `--no-ignore` |
 | `no-fail` | boolean | `--no-fail` |

site/content/reference/rules.md

@@ -34,6 +34,7 @@
 | `unused-marker` | on | a suppression marker that did not suppress anything — the finding it was written for is gone, so the marker is stale. Turn it off with `--no-fail-on-unused-markers`. |
 | `no-comments` | **opt-in** | every comment, in every language it knows (`//`, `/* */`, `#`, `--`, `<!-- -->`). See [no-comments mode](#no-comments-mode) below. |
 | `test-quality` | **opt-in** | tests that weaken what they prove — currently a loop or a conditional in a test body. Parses the file with a treebank grammar, so it reads a test the way the language writes one: `#[test]`, `@Test`, `it(...)`, `TEST(...)`, `test "..."`. See [test quality](#test-quality) below. |
+| `env-vars` | **opt-in** | code that reads the process environment where nothing declares it — `std::env::var`, `os.environ`, `process.env`, `ENV[...]`, `System.getenv`, `getenv`. Files listed in `env-files` are the declared configuration edge and are allowed to read it. See [environment variables](#environment-variables) below. |
 
 ### `deep-nesting` and embedded DSLs
 
@@ -106,7 +107,7 @@
 
 ```toml
 only = ["test-quality"]
-test-rules = ["test-logic"]   # optional: unset runs every rule beamte has
+test-rules = ["test-logic"]   # optional: unset runs every test rule beamte has
 ```
 
 **It is opt-in because it reaches the network.** The grammar for a language is
@@ -211,3 +212,52 @@
 | respect `.gitignore` | on | `--no-ignore` |
 | fail on unused markers | on | `--no-fail-on-unused-markers` |
 | fail on findings | on | `--no-fail` |
+
+## environment variables
+
+An environment variable read in the middle of ordinary code is configuration
+no signature admits to: the function behaves differently on two machines and
+nothing in its declaration says why. `env-vars` reports every such read —
+`std::env::var` in Rust, `os.environ` and `os.getenv` in Python, `process.env`
+in TypeScript and JavaScript, `ENV[...]` in Ruby, `System.getenv` and
+`System.getProperty` in Java, `getenv` in C and C++, `std.process.getEnvVarOwned`
+in Zig. The finding is [beamte](https://github.com/PowderworksCode/beamte)'s
+`env-read`, restating [Test
+Sizes](https://testing.googleblog.com/2010/12/test-sizes.html): a small test
+may not touch system properties, and a component that reads the environment
+mid-body forces that violation on every small test that executes it.
+
+```sh
+straitjacket --env-vars
+```
+
+or in [`straitjacket.toml`](/reference/config-file):
+
+```toml
+env-vars = true
+env-files = ["src/config.rs"]   # the declared configuration edge
+```
+
+`env-files` names the files that *are* the configuration edge — the one module
+that reads the environment and hands values on as arguments. Reads there are
+licensed; reads anywhere else are errors. It is `theme-files` for the
+environment: designate the edge instead of papering readers over with markers.
+The exception with a story — a genuinely per-invocation override — takes a
+[suppression marker](/reference/suppression-markers), which must carry one.
+
+**It is opt-in because it reaches the network**, exactly as `test-quality` is:
+the grammar for a language is downloaded the first time a file in that language
+carries an environment-shaped token, verified and cached content-addressed
+after that. A file whose grammar cannot be fetched is reported as **not read**
+rather than passing quietly.
+
+Files are prefiltered by cheap substrings (`env::var`, `environ`,
+`process.env`), so a file that cannot contain a read is never parsed and the
+rule stays affordable over a whole repository.
+
+Nine languages: Python, Ruby, Rust, Java, TypeScript, JavaScript, C, C++ and
+Zig. Shell is deliberately not among them — `$VAR` is the language's own
+variable model, and flagging every expansion would be flagging the language.
+Compile-time reads are not findings either: Rust's `env!` resolves when the
+build runs, against variables the build declares, which is the announced
+channel this rule steers reads toward.

site/content/rules.json

@@ -17,6 +17,11 @@
       "summary": "emoji glyph in source; use a text label or named icon",
       "default_enabled": true
     },
+    {
+      "id": "env-vars",
+      "summary": "code reads the process environment outside the declared edge",
+      "default_enabled": false
+    },
     {
       "id": "file-size",
       "summary": "file exceeds the configured line budget",

src/config.rs

@@ -30,6 +30,13 @@ pub struct FileConfig {
     /// Which of beamte's test-quality rules to run. Unset means all of them,
     /// including any beamte adds later.
     pub test_rules: Option<Vec<String>>,
+    /// Turn on `env-vars`, which reads every parseable file for environment
+    /// reads outside the declared edge. Opt-in for `test-quality`'s reason:
+    /// the first run downloads a grammar.
+    pub env_vars: Option<bool>,
+    /// The files licensed to read the process environment: the declared
+    /// configuration edge. `theme-files` for the environment.
+    pub env_files: Option<Vec<String>>,
     /// Sections that configured rules Straitjacket no longer has. They are
     /// accepted by the parser only so that [`reject_removed_sections`] can
     /// name the rule that went away.
@@ -60,6 +67,8 @@ pub struct Settings {
     pub no_fail: bool,
     pub fail_on_unused_markers: bool,
     pub test_rules: Vec<String>,
+    pub env_vars: bool,
+    pub env_files: Vec<PathBuf>,
 }
 
 impl Default for Settings {
@@ -82,6 +91,8 @@ impl Default for Settings {
             no_fail: false,
             fail_on_unused_markers: true,
             test_rules: Vec::new(),
+            env_vars: false,
+            env_files: Vec::new(),
         }
     }
 }
@@ -108,6 +119,12 @@ impl Settings {
         if let Some(test_rules) = file.test_rules {
             self.test_rules = test_rules;
         }
+        if let Some(value) = file.env_vars {
+            self.env_vars = value;
+        }
+        if let Some(paths) = file.env_files {
+            self.env_files = paths.into_iter().map(PathBuf::from).collect();
+        }
         if let Some(value) = file.max_lines {
             self.max_lines = value;
         }

src/main.rs

@@ -55,6 +55,9 @@ struct Cli {
     #[arg(long, help = "Enable the opt-in `test-quality` rule")]
     test_quality: bool,
 
+    #[arg(long, help = "Enable the opt-in `env-vars` rule")]
+    env_vars: bool,
+
     #[arg(long, help = "Scan JSON files, which are skipped by default")]
     include_json: bool,
 
@@ -333,6 +336,7 @@ fn resolve(cli: &Cli) -> anyhow::Result<Settings> {
         settings.max_nesting = value;
     }
     settings.no_comments |= cli.no_comments;
+    settings.env_vars |= cli.env_vars;
     settings.test_quality |= cli.test_quality;
     settings.include_json |= cli.include_json;
     settings.no_ignore |= cli.no_ignore;

src/pack.rs

@@ -9,8 +9,10 @@
 //! `tb_*` ABI, statically linked, importing only WASI. It is not linked C, so
 //! it cannot break the musl cross-build the release depends on.
 
+use std::cell::RefCell;
 use std::collections::HashMap;
 use std::path::Path;
+use std::rc::Rc;
 use std::sync::Mutex;
 
 use anyhow::{Context, Result, bail};
@@ -399,6 +401,45 @@ impl Loaded {
     }
 }
 
+thread_local! {
+    /// Loaded packs, and the reasons for the ones that would not load. Shared
+    /// by every rule that parses, so two rules meeting the same language in
+    /// one scan JIT its grammar once between them.
+    ///
+    /// A `FileRule` must be `Send + Sync` and a wasmer `Store` is neither, so
+    /// the packs cannot live in a rule. They live beside the rules instead,
+    /// which costs nothing today -- the walk in `src/walk.rs` is a single
+    /// sequential iterator -- and stays correct rather than unsound if that
+    /// ever changes. A parallel walk would pay one JIT per thread per grammar.
+    ///
+    /// The failure is cached with the same weight as the success: a machine
+    /// with no network pays one failed fetch, not one per file.
+    static CACHED: RefCell<HashMap<&'static str, std::result::Result<Rc<Pack>, String>>> =
+        RefCell::new(HashMap::new());
+}
+
+/// The pack for a grammar, fetched once and then reused.
+///
+/// Fetched per language, and only once a rule has already decided a file is
+/// worth parsing, so a Python repository never downloads the Java grammar.
+pub fn cached(grammar: &'static str) -> std::result::Result<Rc<Pack>, String> {
+    CACHED.with_borrow_mut(|packs| {
+        packs
+            .entry(grammar)
+            .or_insert_with(|| {
+                acquire(grammar)
+                    .map(Rc::new)
+                    .map_err(|error| format!("{error:#}"))
+            })
+            .clone()
+    })
+}
+
+fn acquire(grammar: &'static str) -> Result<Pack> {
+    let bytes = treebank::fetch::fetch_bytes(grammar)?;
+    Pack::from_bytes(&bytes, &format!("the treebank {grammar} pack"))
+}
+
 fn intern(table: &mut Vec<String>, ids: &mut HashMap<String, u32>, value: String) -> u32 {
     if let Some(id) = ids.get(&value) {
         return *id;

src/rules/beamte_findings.rs

@@ -0,0 +1,83 @@
+//! How a beamte finding reads once straitjacket owns it.
+//!
+//! Two rules host beamte -- `test-quality` over test files, `env-vars` over
+//! every file -- and a finding must read the same way whichever door it came
+//! through. One place formats them, so the two cannot drift.
+
+use crate::finding::{EvidenceStep, Finding, Location, Severity};
+use crate::rule::{Candidate, RuleKey};
+
+/// A beamte finding as a straitjacket candidate.
+///
+/// The beamte rule is named in the message rather than in the key, because
+/// straitjacket registers one rule for a family of them and `test-logic`
+/// would be a key nothing in its manifest declares. Beamte's DESIGN.md §6.3
+/// puts citing the post on the host: it turns an argument with a linter into
+/// a much shorter argument with Titus Winters.
+pub fn candidate(
+    key: RuleKey,
+    severity: Severity,
+    path: &str,
+    text: &str,
+    finding: beamte::Finding,
+) -> Candidate {
+    let line = finding.span.line;
+    let column = finding.span.column;
+    Candidate::line(Finding {
+        rule: key,
+        severity,
+        location: Location::point(path, line, column),
+        matched: matched_text(text, line),
+        message: format!("{}: {}", finding.rule, finding.message),
+        help: help_of(&finding),
+        related: Vec::new(),
+        evidence: finding
+            .evidence
+            .into_iter()
+            .map(|step| EvidenceStep {
+                location: Location::point(path, step.span.line, step.span.column),
+                message: step.message,
+            })
+            .collect(),
+    })
+}
+
+/// A file that could not be checked, said out loud.
+///
+/// Beamte's DESIGN.md §7.3: a file that was not read is reported as unread,
+/// never as clean. Returning nothing would mean a failed pack fetch reads
+/// exactly like a file with nothing wrong in it.
+pub fn not_read(key: RuleKey, path: &str, message: String, help: Option<String>) -> Candidate {
+    Candidate::file(Finding {
+        rule: key,
+        severity: Severity::Warning,
+        location: Location::point(path, 1, 1),
+        matched: String::new(),
+        message,
+        help,
+        related: Vec::new(),
+        evidence: Vec::new(),
+    })
+}
+
+fn help_of(finding: &beamte::Finding) -> Option<String> {
+    let citation = beamte::rule(finding.rule.as_str()).map(|rule| rule.citation);
+    match (&finding.help, citation) {
+        (Some(help), Some(citation)) => {
+            Some(format!("{help} — {} ({})", citation.title, citation.url))
+        }
+        (Some(help), None) => Some(help.clone()),
+        (None, Some(citation)) => Some(format!("{} ({})", citation.title, citation.url)),
+        (None, None) => None,
+    }
+}
+
+/// The line a finding sits on, trimmed, for the `matched` field every other
+/// rule fills in from its own regex.
+fn matched_text(text: &str, line: usize) -> String {
+    text.lines()
+        .nth(line.saturating_sub(1))
+        .unwrap_or_default()
+        .trim()
+        .to_string()
+}