fix(followups): F2 review R1 - a failed env load is not cached as loaded
ensure_env_loaded() set _env_loaded before _load_dotenv() ran. With F2,
load_env_or_report() turns the EnvFileError into a return value, so a second
main() in the same process skipped the unreadable file and ran unconfigured
(codex R1: registry.main twice -> rc 1 then rc 0 with a job registered; a
repaired file was never read). The flag is now set only after a successful
load: a failure is retried on the next call, a success still loads once.
envfile parses the whole file before applying, so a failed load sets nothing.
MESSAGING.md: the one-line ERROR is the Python entry points'; the wrapper
adds its own "ERROR: cannot load <path>" line (codex, minor).
Test: in one process, registry.main twice on an undecodable file is rc 1
both times, prints the ERROR line twice and registers nothing; the repaired
file is applied on the next call; a later change is not re-read. At 95eabc6
it fails with rc [1, 0], one job registered and the value unset.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HCGpJXhSZasJLU38ksDghe
This commit is contained in:
1 parent
95eabc693f
commit
75f364e4b4
3 files changed
+41
-2
No files matched your search
@@ -122,8 +122,10 @@ def ensure_env_loaded(workspace_dir: Optional[str] = None) -> None:
|
|||||||
global _env_loaded
|
global _env_loaded
|
||||||
if _env_loaded:
|
if _env_loaded:
|
||||||
return
|
return
|
||||||
_env_loaded = True
|
|
||||||
_load_dotenv(workspace_dir)
|
_load_dotenv(workspace_dir)
|
||||||
|
# Only a load that succeeded is cached: after an EnvFileError the next
|
||||||
|
# call reads the file again (load_env_or_report returns, it does not exit).
|
||||||
|
_env_loaded = True
|
||||||
|
|
||||||
|
|
||||||
def load_env_or_report() -> bool:
|
def load_env_or_report() -> bool:
|
||||||
|
|||||||
+1
-1
@@ -366,7 +366,7 @@ The subscriber acts as the central execution watchdog:
|
|||||||
* **`.mam.env` Resolution Hierarchy (`_load_dotenv`)**:
|
* **`.mam.env` Resolution Hierarchy (`_load_dotenv`)**:
|
||||||
Configuration files are resolved with strict precedence rules:
|
Configuration files are resolved with strict precedence rules:
|
||||||
1. **OS Environment Precedence**: Any variable already defined in `os.environ` is preserved and never overwritten by file-based configs.
|
1. **OS Environment Precedence**: Any variable already defined in `os.environ` is preserved and never overwritten by file-based configs.
|
||||||
2. **Explicit Override (`MAM_ENV_FILE`)**: If `MAM_ENV_FILE` is set, only that specific file is parsed. If the specified file does not exist, it is skipped silently and ambient search is refused (no fallback to unintended parent configs): the process runs on its environment and the defaults. If it exists but cannot be read or decoded, that is an error (`EnvFileError`): the wrapper and the Python entry points print one `ERROR: <path>: cannot read (<exception type>)` line and exit 1.
|
2. **Explicit Override (`MAM_ENV_FILE`)**: If `MAM_ENV_FILE` is set, only that specific file is parsed. If the specified file does not exist, it is skipped silently and ambient search is refused (no fallback to unintended parent configs): the process runs on its environment and the defaults. If it exists but cannot be read or decoded, that is an error (`EnvFileError`): the Python entry points print one `ERROR: <path>: cannot read (<exception type>)` line and exit 1; the wrapper prints that line followed by `ERROR: cannot load <path> (see the line above)` and exits 1.
|
||||||
3. **Workspace Root Auto-Discovery**: If `MAM_ENV_FILE` is not set, the resolver searches candidate paths in order: `MAM_REAL_ROOT`, `WORKSPACE_ROOT`, upward directory walk searching for `.agents` or `.git` boundary markers, and `os.getcwd()`.
|
3. **Workspace Root Auto-Discovery**: If `MAM_ENV_FILE` is not set, the resolver searches candidate paths in order: `MAM_REAL_ROOT`, `WORKSPACE_ROOT`, upward directory walk searching for `.agents` or `.git` boundary markers, and `os.getcwd()`.
|
||||||
4. **Public Broker Security Alert (B-17)**: If the final resolved host falls back to the public sandbox `broker.hivemq.com`, a prominent security warning is emitted.
|
4. **Public Broker Security Alert (B-17)**: If the final resolved host falls back to the public sandbox `broker.hivemq.com`, a prominent security warning is emitted.
|
||||||
* **Broker Config Resolution (`broker_config_from_job`)**:
|
* **Broker Config Resolution (`broker_config_from_job`)**:
|
||||||
|
|||||||
@@ -449,3 +449,40 @@ def test_a7_a_missing_explicit_file_loads_nothing_and_says_nothing(tmp_path):
|
|||||||
assert _python_values(env, ["PROBE_AMBIENT"]) == {"PROBE_AMBIENT": "<unset>"}
|
assert _python_values(env, ["PROBE_AMBIENT"]) == {"PROBE_AMBIENT": "<unset>"}
|
||||||
doc = next(l for l in (REPO_ROOT / "MESSAGING.md").read_text().splitlines() if "Explicit Override (`MAM_ENV_FILE`)" in l)
|
doc = next(l for l in (REPO_ROOT / "MESSAGING.md").read_text().splitlines() if "Explicit Override (`MAM_ENV_FILE`)" in l)
|
||||||
assert "does not exist, it is skipped silently" in doc and "an error is logged" not in doc, doc
|
assert "does not exist, it is skipped silently" in doc and "an error is logged" not in doc, doc
|
||||||
|
|
||||||
|
|
||||||
|
_RETRY_PROBE = r"""
|
||||||
|
import json, os, sys
|
||||||
|
from pathlib import Path
|
||||||
|
from lib_py.delegation import mqtt_common, registry
|
||||||
|
env_file, jobs = Path(sys.argv[1]), sys.argv[2]
|
||||||
|
argv = ["--registry-dir", jobs, "register", "--prompt", "retry probe"]
|
||||||
|
out = {"rc": [registry.main(argv), registry.main(argv)]}
|
||||||
|
out["registered"] = sorted(p.name for p in Path(jobs).glob("*.json")) if Path(jobs).exists() else []
|
||||||
|
env_file.write_text("PROBE_CONFIG=from-file\n")
|
||||||
|
out["recovered"] = [mqtt_common.load_env_or_report(), os.environ.get("PROBE_CONFIG")]
|
||||||
|
env_file.write_text("PROBE_CONFIG=changed\n")
|
||||||
|
out["cached"] = [mqtt_common.load_env_or_report(), os.environ.get("PROBE_CONFIG")]
|
||||||
|
print(json.dumps(out))
|
||||||
|
"""
|
||||||
|
|
||||||
|
|
||||||
|
def test_a1_a_failed_load_is_not_cached_as_loaded(tmp_path):
|
||||||
|
"""Review R1 (codex): load_env_or_report returns instead of exiting, so the
|
||||||
|
failure must not mark the env as loaded - in one process, a second main()
|
||||||
|
on the same unreadable file is rc 1 again and registers nothing, a repaired
|
||||||
|
file is read on the next call, and only a success is loaded once."""
|
||||||
|
f = tmp_path / "broken.env"
|
||||||
|
f.write_bytes(b"PROBE_CONFIG=from-file\nBAD=\xff\n")
|
||||||
|
jobs = tmp_path / "jobs"
|
||||||
|
env = _env(tmp_path, f)
|
||||||
|
env.pop("PROBE_CONFIG", None)
|
||||||
|
res = subprocess.run([sys.executable, "-c", _RETRY_PROBE, str(f), str(jobs)], capture_output=True, text=True,
|
||||||
|
env=env, cwd=str(tmp_path), timeout=60)
|
||||||
|
assert res.returncode == 0, res.stderr[-800:]
|
||||||
|
got = json.loads(res.stdout.strip().splitlines()[-1])
|
||||||
|
assert got["rc"] == [1, 1], (got, res.stderr[-800:])
|
||||||
|
assert got["registered"] == [], got
|
||||||
|
assert res.stderr.count(f"ERROR: {f}: cannot read (UnicodeDecodeError)") == 2, res.stderr[-800:]
|
||||||
|
assert got["recovered"] == [True, "from-file"], got
|
||||||
|
assert got["cached"] == [True, "from-file"], "a successful load stays once per process"
|
||||||
Reference in new issue
Block a user