From 9f2229da29742092a9b3134e06f26e59bef858da Mon Sep 17 00:00:00 2001 From: Eli Pinkerton Date: Sun, 18 Oct 2026 02:39:32 +0000 Subject: [PATCH] Load startup credentials directly in typed Python Python startup and service checks still spawned Bash to parse environment files, while backend launchers duplicated the parser. Share one typed loader across those callers. Keep the sourced shell loader for parent-shell exports and retain process signalling behavior. Preserve literal values, BOM/CRLF, export prefixes, quotes, duplicate-key ordering, and credential-only file precedence. Shell-equivalence and direct cases pass, including loading with Bash absent and unreadable file evidence that cannot recycle a service. Strict pyright, nine selected fast-check suites, and the scoped 11-step gate pass. The packaged payload and Unity source are unchanged from main, validated earlier in this goal session. Nine interleaved local environment-loading samples measured median 7.509 ms through Bash and 0.201 ms through Python. This is a focused loading-step measurement, not an end-to-end startup or CI claim. Refs #171. --- .devcontainer/ai_backends.py | 40 +---------- .devcontainer/env_local.py | 40 +++++++++++ .devcontainer/post_start.py | 56 ++++------------ .../recycle_stale_opencode_service.py | 66 +++++-------------- .llm/context.md | 3 +- scripts/harness_scope.py | 1 + scripts/tests/test_ai_backends.py | 5 +- scripts/tests/test_env_local_precedence.py | 62 +++++++++++++---- 8 files changed, 130 insertions(+), 143 deletions(-) create mode 100644 .devcontainer/env_local.py diff --git a/.devcontainer/ai_backends.py b/.devcontainer/ai_backends.py index 29572f8..b3e1a4d 100755 --- a/.devcontainer/ai_backends.py +++ b/.devcontainer/ai_backends.py @@ -25,6 +25,8 @@ from pathlib import Path from typing import Callable, Dict, List, NoReturn, Optional, Sequence, Tuple +from env_local import load_environment + PROFILE_NAME = "devcontainer-zai" OPENROUTER_PROFILE_NAME = "devcontainer-openrouter" ZAI_ANTHROPIC_URL = "https://api.z.ai/api/anthropic" @@ -32,20 +34,6 @@ LAUNCHER_NAMES = ("codex-zai", "claude-zai", "codex-openrouter", "claude-openrouter") -# Credential names whose .env.local value may override an already-exported -# stale copy when DATAVIZ_ENV_LOCAL_PRECEDENCE=file. Deliberately narrow: -# PATH and every other variable keep "non-empty preexisting wins". -MCP_CREDENTIAL_KEYS = frozenset( - { - "GITHUB_PERSONAL_ACCESS_TOKEN", - "GITHUB_MCP_PAT", - "GITHUB_TOKEN", - "ZAI_API_KEY", - "Z_AI_API_KEY", - "UNITY_MCP_TOKEN", - } -) - # Environment variables each Claude launch action unsets before re-exporting. # Membership matches the bash original's unset list; order is not observable. CLAUDE_ZAI_UNSET_KEYS = ( @@ -126,29 +114,7 @@ def load_env_local(env_file: Optional[str] = None) -> None: else: return - precedence_inverted = os.environ.get("DATAVIZ_ENV_LOCAL_PRECEDENCE") == "file" - with open(resolved, "r", encoding="utf-8", newline="") as handle: - content = handle.read() - if content.startswith("\ufeff"): - content = content[1:] - for raw_line in content.split("\n"): - line = raw_line.rstrip("\r").strip() - if not line or line.startswith("#") or "=" not in line: - continue - key, _, value = line.partition("=") - if key.startswith("export "): - key = key[len("export "):] - key = key.strip() - value = value.strip() - if len(value) >= 1 and value[0] == value[-1] and value[0] in ('"', "'"): - value = value[1:-1] - if not key or any(not (character.isalnum() or character == "_") for character in key): - continue - if os.environ.get(key): - overwrite = precedence_inverted and key in MCP_CREDENTIAL_KEYS - if not overwrite: - continue - os.environ[key] = value + os.environ.update(load_environment(Path(resolved), os.environ)) def resolve_action(invoked_as: str, first_argument: Optional[str]) -> str: diff --git a/.devcontainer/env_local.py b/.devcontainer/env_local.py new file mode 100644 index 0000000..09dc6bb --- /dev/null +++ b/.devcontainer/env_local.py @@ -0,0 +1,40 @@ +#!/usr/bin/env python3 +"""Load literal environment-file assignments for Python startup tools (issue #171).""" +from __future__ import annotations + +import re +from pathlib import Path +from typing import Dict, Mapping, Optional + +MCP_CREDENTIAL_KEYS = frozenset({ + 'GITHUB_PERSONAL_ACCESS_TOKEN', 'GITHUB_MCP_PAT', 'GITHUB_TOKEN', + 'ZAI_API_KEY', 'Z_AI_API_KEY', 'UNITY_MCP_TOKEN', +}) + + +def load_environment(path: Path, environment: Mapping[str, str], prefer_file: Optional[bool] = None) -> Dict[str, str]: + """Return a copy with file assignments merged using the sourced loader's rules. + + Values remain literal. Managed credentials may use file precedence; all + other non-empty environment values win. Process entries in order so repeated + keys follow the same precedence rule as repeated shell exports. + """ + result = dict(environment) + if not path.is_file(): + return result + content = path.read_bytes().decode('utf-8', errors='surrogateescape') + for raw in content.split('\n'): + line = raw.removeprefix('\ufeff').strip() + if not line or line.startswith('#') or '=' not in line: + continue + key, value = (part.strip() for part in line.split('=', 1)) + if key.startswith('export '): + key = key[7:].strip() + if not re.fullmatch(r'[A-Za-z_][A-Za-z0-9_]*', key): + continue + if len(value) >= 2 and value[0] == value[-1] and value[0] in "\"'": + value = value[1:-1] + file_wins = result.get('DATAVIZ_ENV_LOCAL_PRECEDENCE') == 'file' if prefer_file is None else prefer_file + if not result.get(key) or (file_wins and key in MCP_CREDENTIAL_KEYS): + result[key] = value + return result diff --git a/.devcontainer/post_start.py b/.devcontainer/post_start.py index ae69092..9c1e6a9 100644 --- a/.devcontainer/post_start.py +++ b/.devcontainer/post_start.py @@ -20,6 +20,8 @@ sys.dont_write_bytecode = True +from env_local import load_environment + from _lifecycle_common import ( ENV_BLOCK_BEGIN, ENV_BLOCK_END, @@ -104,50 +106,20 @@ def repair_cache_ownership(home: Path) -> None: repair_ownership(npm_cache) -def child_env_with_env_local(repo_root: Path) -> Optional[Dict[str, str]]: - """Load .env.local with file precedence in a child bash and capture its environment. - - The bash original sourced ``env-local.sh`` into its own shell so every later - child saw the loaded credentials; a Python process cannot source bash, so - the same loader runs in a child that prints its final environment with - ``env -0``. devcontainer remoteEnv forwards ``${localEnv:VAR}`` once at - container start, and OpenCode's shared ``serve --service`` inherits the - environment of whichever client started it, so a credential rotated in - .env.local is otherwise invisible until the container is rebuilt. Scoped to - credential names by the loader; PATH and all other vars keep "non-empty - preexisting wins". +def child_env_with_env_local(repo_root: Path) -> Dict[str, str]: + """Load credentials for child tools without spawning a shell. - Returns None when the loader is missing; exits with the loader's code when - it fails, matching the bash original's ``set -e`` behavior. + File precedence applies only to managed credentials; PATH and unrelated + non-empty variables retain their inherited values. """ - if not (repo_root / ".devcontainer" / "env-local.sh").is_file(): - return None - script = ( - '. "$1/.devcontainer/env-local.sh" 1>&2 || exit 10\n' - "DATAVIZ_ENV_LOCAL_PRECEDENCE=file\n" - 'load_env_local "$1/.env.local" 1>&2 || exit 11\n' - "env -0" - ) - completed = subprocess.run( - ["bash", "-c", script, "env-local-loader", str(repo_root)], - capture_output=True, - check=False, - ) - if completed.returncode != 0: - sys.stderr.buffer.write(completed.stderr or b"") - raise SystemExit(completed.returncode) - entries: Dict[str, str] = {} - for record in (completed.stdout or b"").split(b"\0"): - if not record or b"=" not in record: - continue - name, value = record.split(b"=", 1) - entries[name.decode("utf-8", errors="surrogateescape")] = value.decode("utf-8", errors="surrogateescape") - if not entries: - # An environment dump with no entries cannot be an honest snapshot; - # abort instead of silently continuing with stale credentials. - print("env-local loader produced an empty environment; aborting.", file=sys.stderr) - raise SystemExit(1) - return entries + environment = dict(os.environ) + if 'DATAVIZ_ENV_LOCAL_PRECEDENCE' in environment: + environment['DATAVIZ_ENV_LOCAL_PRECEDENCE'] = 'file' + try: + return load_environment(repo_root / '.env.local', environment, prefer_file=True) + except OSError: + print('Cannot read .env.local; aborting startup.', file=sys.stderr) + raise SystemExit(11) def repair_credential_aliases(workspace: Path, env: Optional[Dict[str, str]]) -> None: diff --git a/.devcontainer/recycle_stale_opencode_service.py b/.devcontainer/recycle_stale_opencode_service.py index c1f17cb..c999833 100644 --- a/.devcontainer/recycle_stale_opencode_service.py +++ b/.devcontainer/recycle_stale_opencode_service.py @@ -13,21 +13,11 @@ it lets the next client respawn it from the corrected environment, which is the only fix that does not require rebuilding the container. -Bash stays in the loop for the operations whose semantics must not fork: - -* Reading the env file sources ``env-local.sh`` so the loader's rules (BOM, - CRLF, ``export `` prefix, symmetric quotes) keep exactly one implementation. - A hand-rolled parser that disagrees with the loader would compare the service - against a string the loader never produces and recycle a healthy service on - every start - the exact bug that moved parsing behind the loader once before. - The child unsets every candidate key first, so a value inherited from the - caller can never read as present in the file, and it sets - ``DATAVIZ_ENV_LOCAL_PRECEDENCE=file`` so the file's value wins for the - managed credential names. -* Probing, signalling, and reading the environment of the service process run - through bash's ``kill`` and ``/proc``, so pid namespaces stay consistent on - every host: MSYS pid tables differ from Windows pids, and ``os.kill(pid, 0)`` - on Windows terminates the process instead of probing it. +Environment-file parsing uses the same typed loader as startup and backend +launchers. Shell-equivalence tests pin BOM, CRLF, quote, and precedence rules. +Only values actually defined in the file may judge the service's credentials. +Bash still probes and signals processes so the PID namespace stays consistent +on MSYS, where Windows process IDs differ and os.kill(pid, 0) is unsafe. Safety properties, each covered by a contract test: @@ -35,7 +25,7 @@ * A record whose pid is gone is removed (clients must not attach to it). * No .env.local means nothing to compare against, so nothing to do. * Credential VALUES are never printed - only variable NAMES. -* Missing evidence fails safe: a missing loader, an unreadable environment, or +* Missing evidence fails safe: an unreadable environment or an empty file value leaves a key unjudged, and an unjudged key never kills. The recycler is a Linux devcontainer component: it reads the service's @@ -61,6 +51,8 @@ from pathlib import Path from typing import Dict, List, Optional, Sequence, Tuple +from env_local import load_environment + # Credential names worth comparing. The github pair and the z.ai pair are the # ones that circulate under two names; the rest are single-name secrets whose # rotation has the same staleness problem. @@ -72,29 +64,9 @@ "Z_AI_API_KEY", ) -SCRIPT_DIR = Path(__file__).resolve().parent - # The bash child templates. Each one is a fixed string, so nothing the record # or the environment contains can reach a shell parser: pids and paths travel # as argv, never as interpolated script text. -ENV_VALUES_CHILD = """ -loader="$1" -env_file="$2" -shift 2 -for key in "$@"; do - unset "$key" -done -. "$loader" -DATAVIZ_ENV_LOCAL_PRECEDENCE=file -load_env_local "$env_file" -for key in "$@"; do - value="${!key-}" - if [ -n "$value" ]; then - printf '%s=%s\\0' "$key" "$value" - fi -done -""" - SERVICE_ENVIRON_CHILD = 'cat "/proc/$1/environ" 2>/dev/null' PID_ALIVE_CHILD = 'kill -0 "$1" 2>/dev/null' @@ -105,9 +77,8 @@ def _bash_child_output(script: str, arguments: Sequence[str]) -> bytes: """Run one bash child and return its stdout bytes, or empty on any failure. - A failed child (missing bash, a loader that cannot source, a dead pid's - unreadable /proc entry) returns empty output; every caller treats empty as - "no evidence", which is the fail-safe direction. + A failed child (missing bash or an unreadable /proc entry) returns empty + output. Callers treat it as no evidence and leave the service alone. """ try: completed = subprocess.run( @@ -154,19 +125,12 @@ def _parse_env_entries(raw: bytes) -> Dict[str, str]: def read_env_file_values(env_file: Path) -> Dict[str, str]: - """Return the tracked credential keys the env file defines, with their values. - - The values come from the sanctioned loader, so the recycle decision compares - the service against exactly the strings a real session would load. - """ - loader = SCRIPT_DIR / "env-local.sh" - if not loader.is_file(): - # Standalone fallback the bash original had (a copied script without - # its sibling loader) is deliberately gone: without the loader there - # are no file values, and no file values means nothing is killed. + """Return non-empty tracked values defined by the file, without inherited keys.""" + try: + values = load_environment(env_file, {}, prefer_file=True) + except OSError: return {} - raw = _bash_child_output(ENV_VALUES_CHILD, [str(loader), str(env_file), *CREDENTIAL_KEYS]) - return _parse_env_entries(raw) + return {key: value for key, value in values.items() if key in CREDENTIAL_KEYS and value} def service_environment(pid: int) -> Dict[str, str]: diff --git a/.llm/context.md b/.llm/context.md index 3257d1a..dc8e6b8 100644 --- a/.llm/context.md +++ b/.llm/context.md @@ -226,7 +226,8 @@ runs everything, including the Prettier Markdown check, on ubuntu and windows. C - The devcontainer (`.devcontainer/`) installs the four coding CLIs as the non-root user and refreshes them on create/start; **never** suggest `sudo npm` — rerun `uv run .devcontainer/install_npm_tools.py` or rebuild instead. - Gateway launchers (`codex-zai`, `claude-zai`, `codex-openrouter`, `claude-openrouter`) come from `.devcontainer/ai_backends.py`. They self-load the gitignored `.env.local` for - keys not already exported (preexisting environment variables win), so they work in non-interactive shells too; contract tests live in `scripts/tests/`. + keys not already exported (preexisting environment variables win), so they work in non-interactive shells too. Python startup, backend, and service tools share + `.devcontainer/env_local.py`; the sourced Bash loader remains for parent-shell exports, with parity pinned by `scripts/tests/test_env_local_precedence.py`. - `npm run mcp:sync` regenerates machine-local MCP configs for Claude Code, Codex, OpenCode, Nanocoder, VS Code, and Cursor from `.llm/mcp/configure.mjs`. Those outputs are gitignored and must never be committed. - Secrets live in gitignored `.env.local` (template: `.env.local.example`); config files reference env vars instead of embedding credentials. diff --git a/scripts/harness_scope.py b/scripts/harness_scope.py index 34eeffd..456cd74 100644 --- a/scripts/harness_scope.py +++ b/scripts/harness_scope.py @@ -164,6 +164,7 @@ ".devcontainer/install_npm_tools.py": ("test_install_npm_tools.py",), ".devcontainer/install-npm-tools.sh": ("test_install_npm_tools.py",), "scripts/tests/test_install_npm_tools.py.meta": ("test_install_npm_tools.py",), + ".devcontainer/env_local.py": ("test_ai_backends.py", "test_env_local_precedence.py", "test_devcontainer_lifecycle.py", "test_opencode_service_recycle.py"), ".devcontainer/ai_backends.py": ("test_ai_backends.py", "test_env_local_precedence.py"), ".devcontainer/env-local.sh": ("test_ai_backends.py", "test_env_local_precedence.py"), ".devcontainer/backend_state.py": ("test_backend_state.py", "test_ai_backends.py", "test_env_local_precedence.py"), diff --git a/scripts/tests/test_ai_backends.py b/scripts/tests/test_ai_backends.py index 78380c6..e537d49 100644 --- a/scripts/tests/test_ai_backends.py +++ b/scripts/tests/test_ai_backends.py @@ -18,6 +18,7 @@ from __future__ import annotations import os +import shutil import subprocess import sys from pathlib import Path @@ -50,6 +51,7 @@ STUB_SCRIPT = """#!/usr/bin/env python3 import os +import shutil import sys lines = [f"cmd: {os.path.basename(sys.argv[0])}"] @@ -132,7 +134,7 @@ def link_launchers(self) -> None: self._symlinked = True def copy_launcher_pair(self, broken_state_writer: bool) -> None: - """Stage the launcher + state writer as a copied self-contained pair. + """Stage the launcher, environment helper, and optional failing state writer. A copied launcher computes its workspace from its own location, so a deliberately broken ``backend_state.py`` beside it exercises the @@ -142,6 +144,7 @@ def copy_launcher_pair(self, broken_state_writer: bool) -> None: target = devcontainer / "ai_backends.py" target.write_bytes(LAUNCHER.read_text(encoding="utf-8").encode("utf-8")) target.chmod(0o755) + shutil.copy(LAUNCHER.parent / "env_local.py", devcontainer / "env_local.py") if broken_state_writer: writer = devcontainer / "backend_state.py" writer.write_bytes("import sys\nsys.exit(1)\n".encode("utf-8")) diff --git a/scripts/tests/test_env_local_precedence.py b/scripts/tests/test_env_local_precedence.py index cf98d26..ba37c89 100644 --- a/scripts/tests/test_env_local_precedence.py +++ b/scripts/tests/test_env_local_precedence.py @@ -1,10 +1,9 @@ #!/usr/bin/env python3 """env-local precedence contract tests (typed Python port of ``test-env-local-precedence.sh``, issue #171). -Every case from the bash original is kept with the same assertions; only -the driver language changed. The subject stays the sourced bash loader -(``env-local.sh``), so each case still resolves one variable through a -fresh bash that sources it, exactly like the bash suite's ``resolve``. +The suite compares the typed Python loader with the sourced Bash loader +(``env-local.sh``). Existing precedence cases remain, and direct Python +cases run even when Bash is unavailable. Background: a repaired GitHub PAT in .env.local had no effect because the stale value was already exported into the container's long-lived process @@ -13,8 +12,7 @@ inverts that rule for MCP credential names only. These tests use synthetic placeholder values and never read the real -.env.local. The cases skip on hosts without bash (the subject is a bash -loader), printing a SKIP line like the previous wrapper did. +.env.local. Shell-equivalence cases skip when Bash is unavailable. """ from __future__ import annotations @@ -24,7 +22,8 @@ import subprocess import sys from pathlib import Path -from typing import Dict, Optional +from typing import Dict, List, Optional, Tuple +from unittest.mock import patch sys.dont_write_bytecode = True @@ -33,6 +32,8 @@ TESTS_DIR = Path(__file__).resolve().parent REPO_ROOT = TESTS_DIR.parent.parent LOADER = REPO_ROOT / ".devcontainer" / "env-local.sh" +sys.path.insert(0, str(LOADER.parent)) +from env_local import load_environment def bash_path(path: Path) -> str: @@ -83,7 +84,11 @@ def resolve(env_file: Path, variable: str, overrides: Optional[Dict[str, str]] = errors="replace", check=False, ) - return completed.stdout.strip() + assert_true(completed.returncode == 0, f"the sourced loader must succeed: {completed.stderr}") + actual = completed.stdout + direct = load_environment(env_file, env).get(variable, '') + assert_eq('Python and sourced-shell loaders agree', actual, direct) + return actual def case_fills_empty_preexisting_variable() -> None: @@ -246,6 +251,40 @@ def case_missing_file_is_not_an_error() -> None: ) +def case_python_loader_preserves_literal_formats_and_precedence() -> None: + from post_start import child_env_with_env_local + from recycle_stale_opencode_service import read_env_file_values + fixture = new_temp_root(prefix='env-python-') / '.env.local' + cases: List[Tuple[str, Dict[str, str], str, str]] = [ + ('\ufeff export TOKEN = " spaced = value "\r\n', {}, 'TOKEN', ' spaced = value '), + ("TOKEN='$(never-run) `never-run` $HOME # literal'\n", {}, 'TOKEN', '$(never-run) `never-run` $HOME # literal'), + ('TOKEN=first\nTOKEN=second\n', {}, 'TOKEN', 'first'), + ('TOKEN=\nTOKEN=second', {}, 'TOKEN', 'second'), + ('ZAI_API_KEY=first\nZAI_API_KEY=second', {'DATAVIZ_ENV_LOCAL_PRECEDENCE': 'file'}, 'ZAI_API_KEY', 'second'), + ('DATAVIZ_ENV_LOCAL_PRECEDENCE=file\nZAI_API_KEY=repaired', {'ZAI_API_KEY': 'stale'}, 'ZAI_API_KEY', 'repaired'), + ('# ignored\ninvalid-key=bad\nTOKEN=kept\n', {}, 'TOKEN', 'kept'), + ] + try: + for content, environment, key, expected in cases: + fixture.write_bytes(content.encode('utf-8')) + before = dict(environment) + result = load_environment(fixture, environment) + assert_eq('literal Python loader result', expected, result.get(key, '')) + assert_true(environment == before, 'loading must not mutate the caller environment') + if skip_guard(): + assert_eq('format agrees with sourced shell', expected, resolve(fixture, key, environment)) + fixture.write_text('GITHUB_MCP_PAT=synthetic-file-value\n', encoding='utf-8') + with patch.dict(os.environ, {'GITHUB_MCP_PAT': 'synthetic-stale-value'}, clear=True): + assert_eq('startup loads without a shell', 'synthetic-file-value', child_env_with_env_local(fixture.parent)['GITHUB_MCP_PAT']) + assert_true(read_env_file_values(fixture) == {'GITHUB_MCP_PAT': 'synthetic-file-value'}, 'recycler reads only file values') + with patch.object(Path, 'read_bytes', side_effect=PermissionError('synthetic read failure')): + assert_true(read_env_file_values(fixture) == {}, 'unreadable credentials must not judge a service') + fixture.unlink() + assert_true(load_environment(fixture, {'TOKEN': 'keep'}) == {'TOKEN': 'keep'}, 'a missing file leaves inherited values intact') + finally: + remove_temp_root(fixture.parent) + + CASES = ( case_fills_empty_preexisting_variable, case_nonempty_preexisting_variable_wins_by_default, @@ -260,16 +299,17 @@ def case_missing_file_is_not_an_error() -> None: def skip_guard() -> bool: - """Report whether the suite can run: the subject is a sourced bash loader.""" + """Report whether shell-equivalence cases can run.""" return shutil.which("bash") is not None def main() -> int: assert_true(LOADER.is_file(), f"loader not found at {LOADER}") print("== env-local precedence contract tests ==") + run_case(case_python_loader_preserves_literal_formats_and_precedence.__name__, case_python_loader_preserves_literal_formats_and_precedence) if not skip_guard(): - print(" SKIP: bash not found on PATH; env-local precedence tests require a POSIX shell.") - return 0 + print(" SKIP: bash not found on PATH; skipping shell-equivalence cases.") + return 1 if failure_count() else 0 for case in CASES: run_case(case.__name__, case) failures = failure_count()