diff --git a/.devcontainer/ai_backends.py b/.devcontainer/ai_backends.py index 29572f8e..b3e1a4db 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 00000000..09dc6bb4 --- /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 ae69092d..9c1e6a9f 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 c1f17cbd..c9998336 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 3257d1a1..dc8e6b85 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 34eeffd8..456cd742 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 78380c62..e537d496 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 cf98d265..ba37c898 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()