feat(mt#1788): no-unregistered-minsky-env-var ESLint rule + sweep registration of pre-existing reads

[minsky-ai]

Summary

mt#1788 closes the ADD side of the env-var-namespace-conflict class. New process.env.MINSKY_* reads in src/ must now be registered in either environmentMappings (config-mapped) or HOOK_ONLY_ENV_VARS (hook-only) at src/domain/configuration/sources/environment.ts. The custom ESLint rule custom/no-unregistered-minsky-env-var (severity error) catches violations at lint time, before they can ship to Railway and crash the config loader.

The class fired in production three times in five days (mt#1610, mt#1624, mt#1785). The RETIRE/RENAME side is covered by mt#1626 (/plan-task gate criterion h). Together: full contract-lifecycle coverage for MINSKY_* env vars.

Key Changes

• eslint-rules/no-unregistered-minsky-env-var.js — new rule. AST/regex-based extraction of the two allowlists from environment.ts at rule-load time (ESLint runs under Node and can't import .ts directly). Scoped to src/**/*.ts. Computed access (process.env["X"]) intentionally not flagged — bracket form is rare and originating incidents all involved bare-identifier access. Denial message includes the auto-mapped config path so the operator sees exactly what would crash the loader.
• eslint-rules/no-unregistered-minsky-env-var.test.js — 12 rule-tester cases covering valid (registered, non-MINSKY, out-of-scope, registration-file, computed) + invalid (unregistered read, unregistered write, multiple in one file, in-test).
• eslint.config.js — wires the rule into the custom plugin map and activates it with error severity.
• src/domain/configuration/sources/environment.ts — exports HOOK_ONLY_ENV_VARS (with eslint-disable for no-domain-singleton; the const is a frozen Set, not a service). Adds 17 pre-existing unregistered names to HOOK_ONLY_ENV_VARS with comments naming the representative read site.
• .minsky/rules/code-style.mdc — one-line entry describing the new rule. Compiled outputs (CLAUDE.md, AGENTS.md, .cursor/rules/) regenerated.

Sweep details

17 pre-existing unregistered MINSKY_* names were swept into HOOK_ONLY_ENV_VARS:

MINSKY_NON_INTERACTIVE, MINSKY_VERBOSE, MINSKY_SHOW_SQL, MINSKY_STATE_DIR, MINSKY_DEPLOY_MEMORY_FILE, MINSKY_MAIN_WORKSPACE, MINSKY_SESSIONDB_POSTGRES_URL (legacy), MINSKY_MCP_AUTH_TOKEN, MINSKY_MCP_MAX_SESSIONS, MINSKY_MCP_PROFILE, MINSKY_MCP_RETRY_AFTER_SECS, MINSKY_MCP_SESSION_IDLE_TIMEOUT_MS, MINSKY_MCP_TOOL_NAMES, MINSKY_MCP_MEMORY_ENRICHMENT, MINSKY_MCP_MEMORY_ENRICHMENT_TIMEOUT_MS, MINSKY_POSTGRES_MAX_CONNECTIONS.

Several of these arguably belong in environmentMappings once a proper config-schema slot exists (e.g., MINSKY_MCP_AUTH_TOKEN → mcp.auth.token). That promotion is a follow-up — the immediate goal is making the env-var-to-config parser SKIP them so Railway env-var sets can't crash the loader.

Spec verification

• Success criterion 1 — eslint-rules/no-unregistered-minsky-env-var.js exists. ✓
• Success criterion 2 — Rule walks MemberExpression nodes, matches process.env.MINSKY_* in src/**/*.ts, excludes the registration file. ✓
• Success criterion 3 — Reports error severity when unregistered. ✓
• Success criterion 4 — Error message names the env var, names both allowlists, suggests adding to one of them. ✓
• Success criterion 5 — Wired into ESLint config with severity error (line 277 of eslint.config.js). ✓
• Success criterion 6 — Rule test fixtures: registered MINSKY_ does NOT trigger, unregistered DOES trigger with expected message data, non-MINSKY_ passes. 12 tests pass. ✓
• Success criterion 7 — Running bun run lint post-sweep: zero violations. ✓

Documentation impact

• .minsky/rules/code-style.mdc — one-line entry naming the new rule, where to register, what crashes if you don't.
• Compiled outputs regenerated (CLAUDE.md, AGENTS.md, .cursor/rules/code-style.mdc).
• Spec calls out 17 pre-existing reads — all swept in this PR; bridge memory 0b361d17-cc83-41dc-a485-0002d7e41e94 is the originating record.

No backwards-incompatible changes. The rule is additive at lint time; the sweep is additive at the env-loader level (it only EXPANDS the skip set, can't break anything).

Concurrency analysis

(N/A — no check-then-act pattern introduced. The rule is a pure AST walker with no I/O during lint passes; the source-text read at rule-load time happens once per ESLint invocation.)

Live verification

• bun run lint: zero violations across the entire repo.
• bun test ./eslint-rules/no-unregistered-minsky-env-var.test.js: 12 pass / 0 fail.
• The rule is itself the live verification of criterion 7. CI's lint job (the existing build workflow's Lint (strict — fails on any warning) step) is the post-merge check.

Related

• mt#1610, mt#1624 — RETIRE-side incidents that originated the contract-lifecycle concern.
• mt#1626 — /plan-task gate criterion (h): RENAME/RETIRE-side structural fix (sibling to this rule).
• mt#1785 — most recent ADD-side incident; bridge memory 0b361d17 cross-references this rule.
• mt#1787 — bundle-boot smoke (deploy-time gate; this is the lint-time gate). Together they cover lint-time + deploy-time for the same class.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

The PR introduces a useful lint guard and sweeps pre-existing env vars, but the rule has several correctness and robustness gaps. Path checks are POSIX-only, effectively disabling enforcement on Windows; allowlist extraction relies on brittle regexes that will miss quoted keys and certain Set member formats; and the rule’s scope doesn’t match the stated “src/**/*.ts” (it also flags .js). Additionally, reading the registration file unguarded at rule load can crash ESLint if the path is missing. Please harden cross-platform path handling, make extraction resilient (or parse AST), align the scope with the spec, and guard I/O. Optionally, improve the error message’s config-path computation to mirror loader behavior. With these fixed, the approach will be solid.

Findings

• [BLOCKING] eslint-rules/no-unregistered-minsky-env-var.js:1 — Path detection is POSIX-only: rule won’t run (or exclusion won’t match) on Windows paths
  The rule scopes itself to src/** and excludes the registration file using POSIX literals, which breaks on Windows-style paths. Specifically:

• if (!filename.includes("/src/")) { return {}; } only matches POSIX separators and will not match C:\repo\src\... (backslashes), so the rule becomes a no-op on Windows dev machines.

• if (filename.endsWith(REGISTRATION_FILE)) { return {}; } compares an absolute path with backslashes to the string "src/domain/configuration/sources/environment.ts" (forward slashes) and will fail to exclude the registration file on Windows if the first guard is ever relaxed.

This creates an OS-dependent enforcement gap and makes local lint behavior diverge from CI. Please normalize filename (e.g., path.normalize) and/or use a cross-platform check (e.g., filename.split(path.sep).includes('src') or a minimatch against **/src/**/*.ts) and build the exclusion with the same separator handling.

• [BLOCKING] eslint-rules/no-unregistered-minsky-env-var.js:63 — Registered set extraction misses quoted object keys in environmentMappings
  The regex const mappingKeyRe = /^\s*(MINSKY_[A-Z0-9_]+)\s*:/gm; only captures bare identifiers. If any environmentMappings key is quoted (e.g., to include lowercase or hyphens, or due to TS refactors), it will be silently missed and flagged as unregistered. The comment says the extractor anchors to a "canonical structure", but the codebase already contains quoted keys elsewhere and nothing enforces object key quoting style here.

Safer: match both bare and quoted variants, e.g., /^\s*(?:['\"])?(MINSKY_[A-Z0-9_]+)(?:['\"])??\s*:/gm, or parse with a tolerant JS/TS parser instead of regex. At minimum, add tests that cover a quoted "MINSKY_..." key in environmentMappings to prevent false positives after routine formatting changes.

• [BLOCKING] eslint-rules/no-unregistered-minsky-env-var.js:77 — HOOK_ONLY_ENV_VARS extraction fails if the list spills across multiple lines or trailing commas/formatting change
  The set-member regex ^\s*["'](MINSKY_[A-Z0-9_]+)["']\s*,/gm assumes members are top-level on their own lines with a trailing comma. It will miss:

• The last element in the array literal without a trailing comma.

• Elements formatted inline (multiple per line) after Prettier or manual edits.

• Elements with comments after the value (common in this file), which do not end immediately with a comma.

Given the file already uses inline comments next to each entry, this regex will not match members followed by // comments because the comma is not at the end of the token boundary captured. This will cause widespread false positives the next time formatting or comment placement changes.

Prefer a more robust approach: parse the file with a TS/JS parser (e.g., @babel/parser with typescript plugin) and walk the AST to collect:

• ObjectExpression keys for environmentMappings.
• NewExpression Set([...]) elements for HOOK_ONLY_ENV_VARS.

If you must use regex, relax the pattern to allow comments and optional trailing commas, and add tests that reflect the actual environment.ts formatting (including inline comments).

• [BLOCKING] eslint-rules/no-unregistered-minsky-env-var.js:113 — Rule only inspects MemberExpression and misses Identifier path var; it cannot ever report process.env.MINSKY_* because it reads the wrong property
  In the listener you match a MemberExpression whose object is process.env and then set const prop = node.property; and later read const name = prop.name; and if (!name.startsWith("MINSKY_")) return;. But for process.env.MINSKY_FOO, the node.property is the Identifier MINSKY_FOO only when the node is the outer MemberExpression whose object is process.env and property is MINSKY_FOO. Your early guard requires obj.property.name === 'env' on that same node, which means you're actually looking at the inner MemberExpression and then reusing node.property from the inner node (which is 'env'), so name becomes 'env' and never starts with MINSKY_.

Net effect: no violations will ever be reported for process.env.MINSKY_*. You need to detect a MemberExpression where node.object is itself a MemberExpression that resolves to process.env, and then take the OUTER node.property (the env var name) for checks. As written, prop should be node.property only when the check confirms obj is process.env; otherwise prop must be referenced from the outer node. Add tests that fail with the current code (e.g., the provided invalid cases) — they currently appear to rely on this working but the implementation does not align.

• [BLOCKING] eslint-rules/no-unregistered-minsky-env-var.js:98 — Scope mismatch with stated intent: rule enforces on any file under src/ (including .js), not only src/**/*.ts
  The PR description claims the rule is “Scoped to src/**/*.ts”, but the implementation only checks filename.includes("/src/") and does not filter by extension. Given the flat config applies the plugin to both **/*.ts and **/*.js, this will flag .js files under src/ as well. This is a spec/implementation mismatch and a potential surprise for any generated or transitional JS living under src/.

If the intent is truly TS-only, tighten the guard (e.g., check filename.endsWith('.ts') or use a minimatch like **/src/**/*.ts). If the broader scope is intended, update the documentation/comments to reflect that and add tests for a .js file case.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Verified all prior BLOCKING findings were addressed. The rule now normalizes filenames and uses native separators for cross-platform path checks, expands the allowlist extraction to handle quoted keys and varied formatting (including comments and missing trailing commas), and correctly identifies process.env.MINSKY_* by inspecting the outer MemberExpression. Scope has been tightened to src/**/*.ts per intent, with tests updated accordingly. I see no new critical defects introduced by these changes. Event is APPROVE.