Skip to content

Conversation

@KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented Oct 24, 2025

User description

check for the formatter as early as we can so that we don't run into these problems later on


PR Type

Bug fix, Enhancement


Description

  • Robust formatter presence check with early exit

  • Improved formatter error messages and panels

  • Centralize check at CLI entry, remove duplicate

  • Safer command parsing and which lookup


Diagram Walkthrough

flowchart LR
  main["CLI main()"] -- "check formatter once" --> env["env_utils.check_formatter_installed"]
  env -- "shlex + which + temp run" --> fmt["formatter invocation"]
  env -- "fail -> log and return False" --> exit["early return from CLI"]
  formatter["apply_formatter_cmds"] -- "FileNotFoundError panel improved" --> user["clear error message"]
Loading

File Walkthrough

Relevant files
Bug fix
env_utils.py
Harden formatter check with preflight execution                   

codeflash/code_utils/env_utils.py

  • Add robust formatter existence and run check
  • Use shlex, shutil.which, temp file validation
  • Log clear errors; avoid raising; return booleans
  • Handle disabled/empty commands safely
+36/-14 
Enhancement
formatter.py
Clearer formatter not-found panel output                                 

codeflash/code_utils/formatter.py

  • Improve FileNotFoundError panel message
  • Build command string safely for display
  • Set panel border style to yellow
+2/-5     
main.py
Centralize early formatter check in CLI main                         

codeflash/main.py

  • Import env_utils in CLI entry
  • Early exit if formatter check fails
  • Run check before telemetry and checkpoints
+3/-0     
optimizer.py
Deduplicate formatter check in optimizer                                 

codeflash/optimization/optimizer.py

  • Remove redundant formatter check in optimizer
  • Rely on centralized CLI-level verification
+0/-2     

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Error Handling

The new formatter check logs on failure but ignores the provided 'exit_on_failure' flag and always returns False instead of optionally exiting, which changes behavior from the previous implementation. Verify this does not regress UX or control flow for callers expecting exit behavior.

def check_formatter_installed(formatter_cmds: list[str], exit_on_failure: bool = True) -> bool:  # noqa
    if not formatter_cmds or formatter_cmds[0] == "disabled":
        return True

    first_cmd = formatter_cmds[0]
    cmd_tokens = shlex.split(first_cmd) if isinstance(first_cmd, str) else [first_cmd]

    if not cmd_tokens:
        return True

    exe_name = cmd_tokens[0]
    command_str = " ".join(formatter_cmds).replace(" $file", "")

    if shutil.which(exe_name) is None:
        logger.error(
            f"Could not find formatter: {command_str}\n"
            f"Please install it or update 'formatter-cmds' in your codeflash configuration"
        )
        return False

    tmp_code = """print("hello world")"""
    try:
        with tempfile.TemporaryDirectory() as tmpdir:
            tmp_file = Path(tmpdir) / "test_codeflash_formatter.py"
            tmp_file.write_text(tmp_code, encoding="utf-8")
            format_code(formatter_cmds, tmp_file, print_status=False, exit_on_failure=False)
            return True
    except FileNotFoundError:
        logger.error(
            f"Could not find formatter: {command_str}\n"
            f"Please install it or update 'formatter-cmds' in your codeflash configuration"
        )
        return False
    except Exception as e:
        logger.error(f"Formatter failed to run: {command_str}\nError: {e}")
        return False
Message Consistency

The FileNotFoundError panel styling changed from bold red text to a yellow border with plain text. Confirm this maintains intended visibility and consistency with other console panels.

    if result.returncode == 0:
        if print_status:
            console.rule(f"Formatted Successfully with: {command.replace('$file', path.name)}")
        changed = True
    else:
        logger.error(f"Failed to format code with {' '.join(formatter_cmd_list)}")
except FileNotFoundError as e:
    from rich.panel import Panel

    command_str = " ".join(str(part) for part in formatter_cmd_list)
    panel = Panel(f"⚠️  Formatter command not found: {command_str}", expand=False, border_style="yellow")
    console.print(panel)
    if exit_on_failure:
        raise e from None
Early Exit Scope

Formatter check moved to CLI entry; ensure all code paths that rely on formatting (including subcommands) pass through this check so removal of checks elsewhere (e.g., optimizer) does not miss cases.

    args.func()
elif args.verify_setup:
    args = process_pyproject_config(args)
    init_sentry(not args.disable_telemetry, exclude_errors=True)
    posthog_cf.initialize_posthog(not args.disable_telemetry)
    ask_run_end_to_end_test(args)
else:
    args = process_pyproject_config(args)
    if not env_utils.check_formatter_installed(args.formatter_cmds):
        return
    args.previous_checkpoint_functions = ask_should_use_checkpoint_get_functions(args)
    init_sentry(not args.disable_telemetry, exclude_errors=True)
    posthog_cf.initialize_posthog(not args.disable_telemetry)

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Robustly build command string

Joining formatter_cmds directly can break when items contain spaces or shell
metacharacters, and the naive replace may miss variations like "$file" without a
leading space. Build command_str robustly by tokenizing each command, uniformly
stripping all "$file" placeholders, and shell-quoting parts for accurate error
messages.

codeflash/code_utils/env_utils.py [29]

-command_str = " ".join(formatter_cmds).replace(" $file", "")
+def _normalize_cmd_str(cmds: list[str]) -> str:
+    parts: list[str] = []
+    for cmd in cmds:
+        # Split each command safely and remove any $file placeholders
+        tokens = [t for t in shlex.split(cmd) if t != "$file"]
+        # Re-quote tokens to reflect actual shell invocation
+        parts.extend(shlex.quote(t) for t in tokens)
+    return " ".join(parts)
 
+command_str = _normalize_cmd_str(formatter_cmds)
+
Suggestion importance[1-10]: 6

__

Why: Using naive join and a single replace for "$file" can misrepresent commands in logs; proposing shlex-based normalization improves robustness and error messages without altering behavior significantly.

Low
Avoid false success on failure

If formatting fails for reasons other than missing executable, return False instead
of True so callers can early-exit correctly. Move the return True after confirming
format_code returned success (e.g., no exception) and that the temp file exists
post-run. This avoids false positives on broken formatter configurations.

codeflash/code_utils/env_utils.py [40-53]

 try:
     with tempfile.TemporaryDirectory() as tmpdir:
         tmp_file = Path(tmpdir) / "test_codeflash_formatter.py"
         tmp_file.write_text(tmp_code, encoding="utf-8")
         format_code(formatter_cmds, tmp_file, print_status=False, exit_on_failure=False)
-        return True
+    # If we reached here without exceptions, consider it a success
+    return True
 except FileNotFoundError:
     logger.error(
         f"Could not find formatter: {command_str}\n"
         f"Please install it or update 'formatter-cmds' in your codeflash configuration"
     )
     return False
 except Exception as e:
     logger.error(f"Formatter failed to run: {command_str}\nError: {e}")
     return False
Suggestion importance[1-10]: 3

__

Why: The current code already returns True only when no exception occurs; moving the return outside the with block is a minor style change with negligible impact on correctness.

Low
General
Reinstate defensive formatter check

Removing the formatter check here can let optimization proceed without a working
formatter, causing downstream failures. Keep the centralized early-exit in main, but
also retain a defensive check in optimizer.run to cover programmatic entry points
and tests that bypass main.

codeflash/optimization/optimizer.py [261-265]

 if not env_utils.ensure_codeflash_api_key():
     return
-# Removed formatter check here
+if not env_utils.check_formatter_installed(self.args.formatter_cmds):
+    return
 if self.args.no_draft and is_pr_draft():
     logger.warning("PR is in draft mode, skipping optimization")
     return
Suggestion importance[1-10]: 5

__

Why: Keeping a secondary check in optimizer.run can help for non-CLI entry points, but the PR intentionally centralized this in main, so the benefit is modest and may duplicate logic.

Low

@codeflash-ai
Copy link
Contributor

codeflash-ai bot commented Oct 24, 2025

⚡️ Codeflash found optimizations for this PR

📄 1,676% (16.76x) speedup for check_formatter_installed in codeflash/code_utils/env_utils.py

⏱️ Runtime : 1.75 seconds 98.4 milliseconds (best of 43 runs)

A dependent PR with the suggested changes has been created. Please review:

If you approve, it will be merged into this PR (branch fix/formatter-reporting-and-early-exit).

tmp_file.write_text(tmp_code, encoding="utf-8")
format_code(formatter_cmds, tmp_file, print_status=False, exit_on_failure=False)
return True
except FileNotFoundError:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there might be exceptions other than FileNotFoundError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, and they're caught right below, this just makes the output better looking for when we know it's not FileNotFoundError

@KRRT7 KRRT7 merged commit 44e46b4 into main Oct 25, 2025
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants