Skip to content

Conversation

@misrasaurabh1
Copy link
Contributor

@misrasaurabh1 misrasaurabh1 commented May 29, 2025

PR Type

Bug fix


Description

  • Preserve formatter sequence order

  • Remove set-based deduplication of commands

  • Iterate original formatter_cmds list directly

  • Allow duplicate formatter invocations


Changes walkthrough 📝

Relevant files
Bug fix
formatter.py
Preserve formatter commands order                                               

codeflash/code_utils/formatter.py

  • Removed use of set() around formatter_cmds
  • Changed loop to iterate original list
  • Preserved command order and duplicates
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Performance Concern

    Allowing duplicate formatter commands can lead to unnecessary repeated invocations of the same formatter, impacting performance.

    for command in formatter_cmds:

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Security
    Enforce formatter whitelist

    Filter formatter_cmds against a whitelist of allowed formatter names to prevent
    execution of arbitrary commands. Reject any command whose base name is not in the
    allowed_formatters set. This enforces the intended security TODO.

    codeflash/code_utils/formatter.py [25]

    +allowed_formatters = {"black", "autopep8"}
     for command in formatter_cmds:
    +    cmd_name = shlex.split(command, posix=os.name != "nt")[0].lower()
    +    if cmd_name not in allowed_formatters:
    +        raise ValueError(f"Disallowed formatter: {cmd_name}")
    Suggestion importance[1-10]: 9

    __

    Why: Implementing a whitelist (as noted in the TODO) prevents arbitrary command execution and addresses a critical security concern.

    High
    Possible issue
    Raise on formatter failure

    Enable check=True so that subprocess.CalledProcessError is raised on non-zero exit
    codes. This ensures formatter failures are not silently ignored. Alternatively
    handle returncode explicitly to provide clear error messages.

    codeflash/code_utils/formatter.py [29]

    -result = subprocess.run(formatter_cmd_list, capture_output=True, check=False)
    +result = subprocess.run(formatter_cmd_list, capture_output=True, check=True)
    Suggestion importance[1-10]: 8

    __

    Why: Enabling check=True surfaces non-zero formatter exit codes as CalledProcessError instead of silently ignoring failures, improving reliability.

    Medium
    General
    Deduplicate commands preserving order

    Deduplicate commands while preserving their original order to avoid running the same
    formatter multiple times. Using dict.fromkeys achieves this without losing order.
    This prevents unnecessary duplicate formatting runs.

    codeflash/code_utils/formatter.py [25]

    -for command in formatter_cmds:
    +for command in dict.fromkeys(formatter_cmds):
    Suggestion importance[1-10]: 6

    __

    Why: Using dict.fromkeys avoids redundant formatter runs while preserving the original order, improving efficiency.

    Low

    @aseembits93 aseembits93 merged commit 7878da6 into main May 29, 2025
    17 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.

    4 participants