fix(cli): load .env when a command runs, not at import (#1064)

[frankbria]

Closes #1064.

The fix

load_env_files() moved from module scope into the existing Typer root
callback. The issue's own reproduction:

$ env -u ANTHROPIC_API_KEY .venv/bin/python -c "
  import os; from codeframe.cli.app import app
  print('after cli app import:', bool(os.environ.get('ANTHROPIC_API_KEY')))"
after cli app import: False

A real command still loads it (False->True), and the #904 precedence rules are
unchanged — they just apply a moment later. validators.py, the other caller,
already calls it inside functions (AC4).

One behaviour change worth naming: --help and --version are eager options
that short-circuit before the callback body, so they no longer load the env. That
is why the Ignoring 1 security-sensitive key(s) banner has disappeared from
cf --help. Neither needs credentials, so I consider this an improvement rather
than a regression — flagging it because it is user-visible.

The second bug, found by not trusting the new skip count

The fix made six tests start skipping. Rather than accept that as "correct now",
I checked why — and found this in tests/lifecycle/conftest.py:

if "lifecycle" in str(item.fspath):

A substring match on the filename. It catches:

• tests/auth/test_api_key_lifecycle_919.py
• tests/core/test_proof_requirement_lifecycle_923.py
• tests/test_lifecycle_gates_948.py

None of which make an LLM call. So 51 tests have been silently skipped in
CI for having "lifecycle" in their name.

It went unnoticed because of #1064: the import-time env load meant the key was
always set on a dev machine, so the guard only ever fired in CI — where nobody
reads the skip list. Fixing the env bug is what made it visible.

Now keyed on the lifecycle marker those tests already carry
(pytestmark = [pytest.mark.lifecycle, pytest.mark.slow]), which is also what
pytest.ini's -m "not lifecycle" uses. Verified both directions:

	without ANTHROPIC_API_KEY	with it
the 51 mis-skipped tests	51 passed	pass
tests/lifecycle/ (real LLM)	2 skipped, correct reason	2 collected

Evidence

Full suite: 6407 passed, 49 skipped — byte-identical skip set to the
pre-change baseline, and +6 passing. So the env fix costs no coverage and the
guard fix restores some.

Acceptance criteria

• load_env_files() runs on command execution, not module import
• Import with the key unset leaves it unset — asserted in a child process
• cf still picks up .env; [P0.10] Stop a repository .env from overriding the operator's environment #904 precedence tests unchanged
• validators.py checked for the same pattern (already correct; pinned by a test)

Six tests, run in child processes as the AC requires — this module has already
imported the CLI transitively, so an in-process assertion would prove nothing.
Confirmed non-tautological: restoring the import-time call fails four of six.

Note

This is the third substring-standing-in-for-a-classifier bug found this session,
after #1113 (typer.Exit handling) and #1116 ("started" in event_type). Might
be worth a broader sweep for the pattern; I have not filed one, since three
instances is suggestive rather than conclusive.

[github-actions]

Claude finished @frankbria's task in 7m 0s —— View job

---

Precision bug review — complete

• Gather PR diff and existing comments (no duplicate findings)
• Read changed files + callers/callees to confirm any defects
• Post inline findings (concrete failure scenarios only)
• Post summary

Method note: verified each hunk against the surrounding code — load_env_files() (env_provenance.py:241-316), all callers (app.py, validators.py lines 30/61/112/144, config.py:816-830), the Typer callback/subapp wiring (app.py:287-303, app.add_typer at 6475-6506), the session-wide collection hook (tests/conftest.py:405), and the tests/lifecycle/ marker (pytestmark = [pytest.mark.lifecycle, pytest.mark.slow]).

severity	file:line	finding
major	tests/cli/test_env_not_loaded_at_import_1064.py:84-95	test_a_command_loads_the_env is non-hermetic — it depends on an ambient ~/.env/<repo>/.env and fails on CI / fresh clones, breaking the uv run pytest tests/ gate. Suggestion posted inline.

What I checked and cleared (no defect):

• app.py env-load move — every os.environ/os.getenv read in app.py (lines 427, 949, 3007, 4434) is inside a command body that runs after the root callback; no typer.Option/Argument default reads env at def-time, so moving load_env_files() from import to callback loses no .env value. cf serve (root command, app.py:966) and every subapp command run the parent _root callback first, so env loads. The ui/ server never used this loader, so server behavior is unchanged. --help/--version short-circuit is documented and harmless.
• tests/lifecycle/conftest.py marker switch — pytest_collection_modifyitems is session-wide (confirmed: tests/conftest.py:409 uses the same all-items loop), and tests/lifecycle/test_cli_lifecycle.py carries the module-level lifecycle marker (2/2 tests), so the guard correctly skips exactly the real-LLM tests when no key is set. The fix tightens (not loosens) the skip set.

1 finding (1 major), posted as an inline review comment.

[claude]

Review

The core fix is sound. load_env_files() moving from module scope into the Typer root callback (codeframe/cli/app.py:287-303) correctly stops from codeframe.cli.app import app from mutating os.environ as a side effect of import. I traced the Click/Typer dispatch chain: a @app.callback() on a Typer() root becomes the group's invoke, which Click always runs before any nested subcommand body -- confirmed structurally by config_app being a nested sub-typer (app.add_typer(config_app, name="config")) and the eager --version/--help options short-circuiting via ctx.exit() before the callback body runs, which is exactly why the "Ignoring N security-sensitive key(s)" banner disappears from cf --help. I also checked that nothing else in codeframe/ imports codeframe.cli.app (only the two __main__.py entry points do), so there's no other code path that was silently relying on the old import-time load.

The tests/lifecycle/conftest.py fix is a genuine, well-diagnosed second bug: "lifecycle" in str(item.fspath) is a substring match on the filename, and I confirmed tests/auth/test_api_key_lifecycle_919.py, tests/core/test_proof_requirement_lifecycle_923.py, and tests/test_lifecycle_gates_948.py all carry pytestmark = pytest.mark.v2 but not pytest.mark.lifecycle -- so they were being silently skipped by this guard whenever ANTHROPIC_API_KEY was unset in CI. Switching to item.get_closest_marker("lifecycle") matches the same marker pytest.ini's -m "not lifecycle" already keys on. Good catch, and the PR description's explanation of why it went unnoticed (import-time env load kept it masked locally) checks out.

One concern: test_a_command_loads_the_env looks environment-dependent

tests/cli/test_env_not_loaded_at_import_1064.py::TestRunningACommandStillLoadsIt::test_a_command_loads_the_env runs cf config telemetry status in a child process with cwd=REPO_ROOT and ANTHROPIC_API_KEY stripped from the env, then asserts stdout contains "False->True" -- i.e. it asserts that running a real command actually populates ANTHROPIC_API_KEY from a .env file.

For that to happen, load_env_files() needs to find either <REPO_ROOT>/.env or ~/.env containing ANTHROPIC_API_KEY. I checked:

• .gitignore excludes .env* (only .env*.example is re-included), so a fresh checkout in CI has no .env at the repo root.
• .github/workflows/test.yml's "Backend Unit Tests" job never sets ANTHROPIC_API_KEY (no occurrence in the workflow at all) and never writes a .env file before the uv run pytest step.
• The GitHub-hosted runner's $HOME is freshly provisioned per job, so ~/.env won't exist either.

So on a clean CI runner, load_env_files() should find no .env at all, ANTHROPIC_API_KEY stays unset, and the assertion "False->True" in result.stdout should fail. This test would only pass on a machine that happens to already have a .env (or ~/.env) defining ANTHROPIC_API_KEY -- plausible on a contributor's dev box that also runs scripts/lifecycle locally, which is presumably how this passed for the author, but that's exactly the kind of ambient state the rest of this test file (and tests/core/test_repo_env_override_904.py, which is the established pattern here) is careful to avoid by writing its own temp .env under tmp_path.

Worth having this one create its own temp .env (e.g. tmp_path / ".env" with ANTHROPIC_API_KEY=test-value) and pass cwd= pointed at it, rather than depending on whatever happens to be in the ambient environment. As of this review, the PR's own "Backend Unit Tests" CI check was still running -- worth confirming it actually passes there before merging, since a local-only "6407 passed" run wouldn't have caught this.

Everything else -- the AST-based structural guard against a regression back to module scope, the validators.py pin, and the operator-precedence test that sets the env var directly rather than via a file -- is solid and doesn't have this issue.

🤖 Generated with Claude Code

[github-actions]

Claude finished @frankbria's task in 5m 39s —— View job

---

Precision bug review — complete

Synchronize review (new commit 0c501d0, the hermeticity fix). The prior non-hermetic cwd=.env finding (major) is resolved and not re-flagged. I checked the new code for fresh concrete defects only.

• Gather PR diff + existing inline/issue comments (avoid duplicates)
• Read changed files + callers/callees against real source
• Post inline finding (concrete failure scenario only)
• Post summary

severity	file:line	finding
minor	tests/cli/test_env_not_loaded_at_import_1064.py:92	_child_in isolates cwd but not HOME; a contributor's real ~/.env defining ANTHROPIC_API_KEY wins (override=False) over the tmp .env, failing test_a_command_loads_the_env locally. CI is unaffected.

1 finding (1 minor), posted as an inline review comment.

Method note — what I verified and cleared (no defect):

• app.py env-load move — load_env_files() now runs in the _root callback (app.py:303). Confirmed ANTHROPIC_API_KEY is not in _REPO_FORBIDDEN_EXACT/suffixes/prefixes (env_provenance.py:175-228), so the loader does populate it from <cwd>/.env — the new test's premise is valid. --version's eager callback short-circuits before the body, so it (correctly) no longer loads env.
• tests/lifecycle/conftest.py marker switch — item.get_closest_marker("lifecycle") matches the same marker pytest.ini's -m "not lifecycle" keys on; tests/lifecycle/ contains only test_cli_lifecycle.py (carries the module-level marker), and the three mis-matched files (test_api_key_lifecycle_919, test_proof_requirement_lifecycle_923, test_lifecycle_gates_948) now correctly run. Tightens, not loosens.
• Import-inertness tests (TestImportingTheCliIsInert) — robust: importing no longer loads any .env, so ~/.env/cwd contents are irrelevant to them; the AST test pins no module-level call regresses.
• test_the_operator_environment_still_wins — robust against the home-.env issue below, because it sets the key itself and override=False protects it.

The one residual: the load test still measures the contributor's ~/.env (consulted first, override=False) unless HOME is also isolated — same bug class as the prior finding, home leg instead of cwd leg.

[claude]

Review

I re-verified the fix on the current diff (2284a158 + the follow-up 0c501d07). Both prior reviews on this PR already covered the core mechanics well, so I'll avoid re-litigating those and just confirm/extend.

Confirmed sound:

• load_env_files() moving into _root (codeframe/cli/app.py:287-303) is correct: I checked every os.environ/os.getenv read in app.py (lines 427, 949, 3007, 4434) and none are evaluated as a typer.Option/Argument default at decoration time — they're all inside command bodies, which run after the callback. So no .env value is lost by the timing shift. I also grepped every sibling CLI module pulled in via add_typer (auth_commands, pr_commands, env_commands, engines_commands, hooks_commands, stats_commands, import_commands) for their own import-time env loading — none exists, so nothing else was silently relying on the old side effect.
• The tests/lifecycle/conftest.py substring→marker fix is correct and well-targeted: only tests/lifecycle/test_cli_lifecycle.py carries pytest.mark.lifecycle, and I confirmed tests/auth/test_api_key_lifecycle_919.py, tests/core/test_proof_requirement_lifecycle_923.py, and tests/test_lifecycle_gates_948.py do not — so the old "lifecycle" in str(item.fspath) guard really was mis-skipping them, and item.get_closest_marker("lifecycle") is the right replacement (matches what pytest.ini's -m "not lifecycle" already keys on).
• The previously-flagged non-hermetic test (test_a_command_loads_the_env asserting against the repo's own untracked .env) was already fixed in the 0c501d07 follow-up commit — it now builds its own .env under tmp_path and runs the child there via PYTHONPATH, so it no longer depends on ambient repo state. Good catch-and-fix cycle.

Nothing new to add of substance. One very minor observation, not blocking: the no_args_is_help=True on the root Typer() means bare cf (no subcommand) also skips _root entirely, same as --help/--version — consistent with the documented eager-option behavior change, just noting it's a third case with the same shape (harmless, since bare cf only shows help).

Nice diagnosis chain overall — finding the marker bug by refusing to accept "these six tests started skipping" as expected fallout from the primary fix is a good instinct, and the PR body's evidence table (before/after skip counts) makes it easy to verify.

[github-actions]

Claude finished @frankbria's task in 3m 6s —— View job

---

Precision bug review — complete

Synchronize review (new commit 09aa7e4, the HOME-isolation fix). The prior minor finding (_child_in isolating cwd but not HOME, letting a contributor's real ~/.env win under override=False) is resolved and not re-flagged. I checked the new code for fresh concrete defects only — none found.

• Gather PR diff + existing inline/issue comments (avoid duplicates)
• Read changed files + callers/callees against real source
• Verify the latest fix; hunt for fresh defects
• Post summary

✅ GLM review: no defects found.

Method note — what I verified and cleared (no defect):

• 09aa7e4 HOME isolation (test_env_not_loaded_at_import_1064.py:97-100) — env["HOME"] = empty_home (empty dir, no .env) makes Path.home()/.env not exist, so in load_env_files() home_env.exists() is False (env_provenance.py:284), the home leg is skipped, and only the test-created cwd/.env loads. Traced the full call for both TestRunningACommandStillLoadsIt tests: ANTHROPIC_API_KEY is not in _REPO_FORBIDDEN_EXACT/suffixes/prefixes (env_provenance.py:175-228), so blocked={}, load_dotenv(cwd_env, override=False) sets the key (load test) or is no-op'd by the operator value (precedence test). Both assertions hold hermetically. USERPROFILE is set for Windows parity and inert on Linux. Trailing os.pathsep when PYTHONPATH is empty is harmless (tmp_path has no codeframe/).
• Command path the test drives — cf config telemetry status is a real non-erroring command: config_app is wired via app.add_typer(config_app, name="config") (app.py:6506), telemetry(action="status") → _print_status() returns with returncode == 0 (config_commands.py:33-35). Click runs the root _root callback before descending into the nested subapp, so load_env_files() fires and before=None/after=from-dot-env is sound.
• tests/lifecycle/conftest.py marker switch — confirmed session-wide scope (the old "lifecycle" in str(item.fspath) did skip tests/auth/, tests/core/, and tests/-root files, which is only possible if the hook sees all collected items). The lifecycle marker is registered in both pytest.ini and pytest_configure; only tests/lifecycle/test_cli_lifecycle.py carries it (module pytestmark). The three formerly mis-matched files (test_api_key_lifecycle_919, test_proof_requirement_lifecycle_923, test_lifecycle_gates_948) reference "lifecycle" only as string/path literals and a @pytest.mark.parametrize id — never as a marker — so item.get_closest_marker("lifecycle") returns None for them. Tightens, not loosens.
• Import-inertness tests (TestImportingTheCliIsInert) — robust without HOME isolation because import no longer calls the loader at all; the AST test (TestTheModuleHasNoImportTimeCall) pins no regression to module scope by inspecting tree.body only.

No inline findings to post — the diff is clean on this synchronize.

[claude]

Review

I re-verified the final state of this PR (commits 2284a158 → 0c501d07 → the HOME-isolation follow-up), building on the three prior review rounds already on this thread. Nothing new to flag — the previously-raised issues are resolved and I didn't find additional defects.

Confirmed sound, independently:

• load_env_files() now runs inside _root (codeframe/cli/app.py:287-303), which is Click's group callback — it always executes before any nested subcommand body runs (verified via the sub-typer wiring, e.g. auth_app/pr_app/env_app added with app.add_typer(...)), so every real cf ... invocation still loads .env per the [P0.10] Stop a repository .env from overriding the operator's environment #904 precedence rules. --version is is_eager=True and exits before the callback body, so it correctly no longer loads env — and since the root Typer() has no_args_is_help=True, bare cf also skips it, same shape as --help/--version. All harmless, all documented in the PR body.
• tests/lifecycle/conftest.py's switch from "lifecycle" in str(item.fspath) to item.get_closest_marker("lifecycle") matches exactly what pytest.ini's -m "not lifecycle" already keys on (confirmed in pytest.ini:38-39,54), and the marker is registered in the same conftest (pytest.ini-style markers addition at line 46). This is a real, well-isolated second bug — a substring match on the filename would also catch any future test with "lifecycle" in its name regardless of whether it makes a real LLM call.
• The previously-flagged hermeticity issues in test_a_command_loads_the_env (first: asserting against the repo's own untracked .env; then: not isolating HOME) are both fixed in the current diff — it builds its own tmp_path / ".env", runs the child there via PYTHONPATH, and points HOME/USERPROFILE at an empty directory so an ambient ~/.env on a contributor's machine can't leak in.
• The AST-based structural test (TestTheModuleHasNoImportTimeCall) is a nice touch — it pins the fix against silent regression back to module scope, which is exactly the kind of thing that's easy to reintroduce accidentally during a future refactor of app.py.

Minor, non-blocking observation: the module-scope comment left where load_env_files() used to be called (app.py:41-44) is good context, but note it now sits between the import and the app = typer.Typer(...) construction — worth a glance next time that section is touched to make sure it still reads naturally if more code is inserted there. Not asking for a change.

Solid diagnosis-and-fix cycle overall: refusing to accept "six tests started skipping" as expected fallout from the primary fix, and instead tracing it to a second, independent bug, is exactly the right instinct. The evidence table in the PR description (before/after skip counts, both with and without the API key) makes the fix easy to verify without re-running everything.

🤖 Generated with Claude Code