Fix #3010: Warn/prompt before persisting empty environment config

[Mustafa11300]

Summary

Closes #3010.

persist_env() in configure_cmd.py previously wrote env_dict to config.json without checking if it was empty. This silently wiped the user's existing configuration.

Changes

Core fix – configure_cmd.py

Added a guard in persist_env() that detects an empty env_dict and:

1. Displays a yellow warning explaining that the configuration to be saved is empty and will overwrite existing settings.
2. In interactive mode (TTY): prompts the user for confirmation before writing. If the user declines, the operation is aborted and the original configuration is preserved.
3. In non-interactive mode (CI, piped stdin): automatically aborts the write with a red error message, preventing accidental configuration loss.
4. Returns a boolean (True on successful write, False on abort) so callers can react if needed.

The existing TODO comment (# TODO: Should we persist empty env_dict or notify user differently?) has been resolved.

Tests – test_configure_cmd.py

14 unit tests covering:

• Non-empty dict persisted without any prompt
• Empty dict + interactive TTY + user confirms → writes empty config
• Empty dict + interactive TTY + user declines → config unchanged
• Empty dict + non-interactive → auto-abort, no confirmation called
• None treated as falsy/empty → triggers the guard
• Named profile path is respected
• Warning and abort messages are emitted
• Return values are correct
• Config file created fresh when missing
• Non-empty dict never calls click.confirm
• get_config_path helper for default and named profiles

Examples – empty_config_guard

Three runnable scenarios with a README:

#	Script	What it shows
1	01_interactive_empty_config_confirm.py	Interactive TTY warning + confirmation prompt
2	02_non_interactive_empty_config_abort.py	Automatic abort when stdin is not a TTY
3	03_nonempty_config_no_prompt.py	Non-empty config written directly, no extra prompt

How to test

python -m pytest test/unit/test_configure_cmd.py -v

[greptile-apps]

Greptile Summary

This PR adds a safety guard to persist_env() in configure_cmd.py that prevents silently overwriting config.json with an empty dictionary. In interactive (TTY) environments the user is warned and prompted for confirmation; in non-interactive environments (CI, piped stdin) the write is automatically aborted. A boolean return value (True/False) is added and 14 unit tests plus three runnable examples are included.

Key issues found:

• import_from misleading success output — The "Configuration successfully read from: …" echo is printed before persist_env is called. If the imported file is empty and the write is subsequently aborted, the user sees a success message followed by an abort, making it appear the import succeeded when it did not.
• Callers discard the new False return value — All six existing call sites (aws, azure, gcp, kubernetes, sandbox, import_from) ignore the return value of persist_env. If the guard aborts the write (e.g., when a user declines all prompts and the resulting dict comprehension {k: v for k, v in env.items() if v} produces {}), the CLI exits with code 0 with no indication that nothing was saved.
• Test 11 relies on implicit MagicMock truthiness — test_empty_confirmed_file_is_empty_json omits an explicit mock_stdin.isatty.return_value = True, making the test pass by accident.

Confidence Score: 2/5

• Not safe to merge — the guard logic itself is sound but two caller-side bugs mean an aborted write can go undetected by both the user and the CLI exit code.
• The core guard in persist_env works correctly for the common case, but the PR changes the function's semantics (it now returns False and can abort) without updating any of the six existing call sites. The import_from command actively emits a misleading success message before the potentially-aborting call. Both issues are reproducible in normal usage (e.g., importing an empty config file, or declining all options in the aws/azure/gcp/kubernetes wizards), making them logic bugs rather than theoretical edge cases.
• Pay close attention to metaflow/cmd/configure_cmd.py — specifically the import_from function (lines 213–218) and all six persist_env call sites that do not handle the new False return value.

Important Files Changed

Filename	Overview
metaflow/cmd/configure_cmd.py	Core fix adding an empty-config guard to persist_env. Contains two logic issues: (1) the import_from command prints a success message before persist_env runs, misleading users when an abort follows; (2) all six call sites across the file discard the new False return value, so an aborted write is invisible to callers and the CLI exits with code 0.
test/unit/test_configure_cmd.py	14 new unit tests covering the guard scenarios. Test 11 (test_empty_confirmed_file_is_empty_json) does not explicitly set mock_stdin.isatty.return_value = True, relying on implicit MagicMock truthiness. The brittle tearDown using os.rmdir (already flagged in a previous thread) remains unaddressed.
examples/empty_config_guard/01_interactive_empty_config_confirm.py	Runnable example demonstrating the interactive TTY path. No issues found.
examples/empty_config_guard/02_non_interactive_empty_config_abort.py	Runnable example demonstrating the non-interactive auto-abort path. No issues found.
examples/empty_config_guard/03_nonempty_config_no_prompt.py	Runnable example demonstrating the happy-path write without prompts. No issues found.
examples/empty_config_guard/README.md	Documentation for the three example scenarios. Clear and accurate. No issues found.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["persist_env(env_dict, profile)"] --> B{env_dict is empty\nor None?}
    B -- No --> G["Write JSON to config file"]
    G --> H["echo success message"]
    H --> I["return True"]

    B -- Yes --> C["echo yellow warning"]
    C --> D{"sys.stdin.isatty()?"}
    D -- No\nnon-interactive --> E["echo red abort message"]
    E --> F["return False"]
    D -- Yes\ninteractive --> J{"click.confirm\ndefault=False"}
    J -- User confirms --> G
    J -- User declines --> K["echo 'Operation aborted'"]
    K --> F

    style F fill:#f55,color:#fff
    style I fill:#5a5,color:#fff
    style C fill:#fa0,color:#000
    style E fill:#f55,color:#fff

Loading

Comments Outside Diff (1)

1. metaflow/cmd/configure_cmd.py, line 213-218 (link)

   import_from displays success message before persist_env can abort

   The "Configuration successfully read from" message at lines 213-214 is printed before persist_env is called. With the new guard, if the imported file contains an empty dict {}, persist_env will either auto-abort (non-interactive) or prompt the user (interactive). In both abort cases the user has already seen the success output, making it appear the import completed successfully when it actually was not written.

   A concrete repro: run metaflow configure import empty_config.json in a CI environment where stdin is not a TTY. The user will see:

   Configuration successfully read from: "empty_config.json"
   Warning: The configuration to be saved is empty. ...
   Aborting: refusing to write an empty configuration in a non-interactive environment.
   

   The first line directly contradicts the outcome. Consider moving the success echo to after a confirmed write, or checking the return value and printing an appropriate message:

   confirm_overwrite_config(profile)
   if persist_env(env_dict, profile):
       echo("Configuration successfully read and written from: ", nl=False)
       echo('"%s"' % input_path, fg="cyan")

_{Last reviewed commit: 755e174}

[greptile-apps]

None passes guard and writes null to config file

if not env_dict: is truthy for both {} and None. Test 5 passes None and only catches the non-interactive branch (auto-abort). In an interactive TTY where the user confirms, the code falls through to json.dump(None, ...), which serialises as JSON null. When get_env() reads that file it returns None rather than {}, and any subsequent call to existing_env.get(...) in the configure helpers raises AttributeError.

Consider normalising None to {} before the guard:

env_dict = env_dict or {}
if not env_dict:
    ...  # existing warning / abort / confirm logic

[greptile-apps]

sys.stdin.isatty() may not reflect Click's own stdin stream

The rest of configure_cmd.py routes all user interaction through Click's click.confirm / click.prompt (which respect Click's internal stdin stream). Checking sys.stdin.isatty() directly bypasses Click's stream handling: if a caller overrides Click's input (e.g. via CliRunner(mix_stderr=False) in integration tests, or a plugin that wraps sys.stdin), the TTY check may return the wrong result.

A more consistent approach would be to use Click's own context or stream helpers:

if not click.get_text_stream("stdin").isatty():

Or, if Click's vendored copy doesn't expose that helper, use click.utils._default_text_stdin().isatty() — or simply rely on click.confirm's built-in behaviour with a timeout/default rather than pre-checking TTY manually.

[greptile-apps]

Brittle tearDown with os.rmdir

os.rmdir only removes an empty directory. Test 6 (test_named_profile_path) creates an extra file (config_production.json) in self.tmpdir and cleans it up inline — but if that test raises an exception before reaching os.remove(profile_path), the file will remain, and os.rmdir(self.tmpdir) will raise OSError: [Errno 66] Directory not empty. This masks the original test failure with a confusing teardown error.

Consider using shutil.rmtree:

import shutil

def tearDown(self):
    shutil.rmtree(self.tmpdir, ignore_errors=True)

[greptile-apps]

Callers don't handle the new False return value

persist_env now returns False when the write is aborted, but every call site in this file discards that return value. This means an aborted write is silently swallowed — the configure command exits with code 0 (success) even though no config was written.

This is a realistic scenario: a user runs metaflow configure aws, declines both the S3 and Metadata Service prompts, and the dict comprehension {k: v for k, v in env.items() if v} produces {}. persist_env aborts, returns False, and the command exits as if everything succeeded.

Affected call sites:

• azure command — line 837: persist_env({k: v for k, v in env.items() if v}, profile)
• gcp command — line 878: same pattern
• aws command — line 936: same pattern
• kubernetes command — line 1000: same pattern
• sandbox command — line 261: persist_env(env_dict, profile)
• import_from — line 218: persist_env(env_dict, profile)

Each of these should check the return value and either abort the command or inform the user that nothing was saved.

[greptile-apps]

isatty() not explicitly set — relies on implicit MagicMock truthiness

mock_stdin.isatty.return_value is never set in this test, so it defaults to a MagicMock() instance. That instance is truthy, which happens to make not sys.stdin.isatty() evaluate to False (i.e., "interactive"), allowing the test to reach the click.confirm path and pass.

This is fragile: the test passes by accident rather than by intent. A future change (e.g., switching to if sys.stdin.isatty() instead of if not sys.stdin.isatty()) or a different Mock configuration could silently break the behaviour being tested without a clear failure message.

Suggested change

	mock_stdin.isatty.return_value = True
	mock_click.confirm.return_value = True
	mock_stdin.isatty.return_value = True
	mock_click.confirm.return_value = True