Skip to content

Conversation

@HeshamHM28
Copy link
Contributor

@HeshamHM28 HeshamHM28 commented Jul 2, 2025

PR Type

Enhancement


Description

  • Add --staging-review CLI option

  • Introduce create_staging API endpoint

  • Refactor PR creation into process_review

  • Route to staging or GitHub PR based on args


Changes diagram

flowchart LR
  CLI["CLI --staging-review"] --> Process["process_review"]
  Process -- "staging_review" --> Staging["create_staging"]
  Process -- "generate PR" --> PR["check_create_pr"]
  Process -- "no PR" --> Mark["mark_optimization_success"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
cfapi.py
Add create_staging API function                                                   

codeflash/api/cfapi.py

  • Imported get_current_branch, git_root_dir, FileDiffContent, PrComment
  • Added create_staging function for staging PRs
  • Payload builds diffContents and prCommentFields JSON
  • +55/-2   
    function_optimizer.py
    Refactor PR logic and add staging path                                     

    codeflash/optimization/function_optimizer.py

  • Imported create_staging in CF API imports
  • Refactored PR logic into new process_review method
  • process_review routes to create_staging or check_create_pr
  • +91/-56 
    Configuration changes
    cli.py
    Add staging-review CLI flag                                                           

    codeflash/cli_cmds/cli.py

    • Added --staging-review CLI argument
    +1/-0     

    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

    github-actions bot commented Jul 2, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Signature Mismatch

    The create_staging function signature expects original_code and new_code as strings, but it is called with dictionaries of file paths to content. Ensure the parameter types align or adjust the signature.

    def create_staging(
        original_code: str,
        new_code: str,
        explanation: Explanation,
        existing_tests_source: str,
        generated_original_test_source: str,
        function_trace_id: str,
        coverage_message: str,
    ) -> Response:
    Writing Logic Bug

    The condition for writing code uses ((not no_pr) or not staging_review) which is always true when no_pr is false, causing unintended writes during staging. Consider using and not staging_review instead.

    if ((not self.args.no_pr) or not self.args.staging_review) and (
        self.args.all or env_utils.get_pr_number() or (self.args.file and not self.args.function)
    ):
        self.write_code_and_helpers(
            self.function_to_optimize_source_code, original_helper_code, self.function_to_optimize.file_path
        )
    Hardcoded Base Branch

    create_staging uses the current branch for baseBranch, but staging PRs should target a specific branch (e.g., 'staging') or allow configuration. Verify branch selection logic.

    "baseBranch": get_current_branch(),

    @github-actions
    Copy link

    github-actions bot commented Jul 2, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix write-back boolean logic

    The write-back condition is too permissive because or not self.args.staging_review
    makes it true even when --no-pr is set. Change to require that either a PR or
    staging was created.

    codeflash/optimization/function_optimizer.py [1086-1087]

    -if ((not self.args.no_pr) or not self.args.staging_review) and (
    +if (not self.args.no_pr or self.args.staging_review) and (
         self.args.all or env_utils.get_pr_number() or (self.args.file and not self.args.function)
     ):
    Suggestion importance[1-10]: 7

    __

    Why: Adjusting the condition to (not self.args.no_pr or self.args.staging_review) ensures code is written back during staging reviews, correcting the original logic that unintentionally suppressed it when --staging-review was set.

    Medium
    General
    Correct parameter types

    The type annotations for original_code and new_code should reflect that they are
    mappings of file paths to code strings, not simple strings. Update the signature and
    docstring to accurately describe parameters.

    codeflash/api/cfapi.py [180-188]

     def create_staging(
    -    original_code: str,
    -    new_code: str,
    +    original_code: dict[Path, str],
    +    new_code: dict[Path, str],
         explanation: Explanation,
         existing_tests_source: str,
         generated_original_test_source: str,
         function_trace_id: str,
         coverage_message: str,
     ) -> Response:
    Suggestion importance[1-10]: 6

    __

    Why: The parameters original_code and new_code are actually mappings of file paths to code strings, so updating their annotations to dict[Path, str] aligns the signature with their usage and improves type correctness.

    Low

    @HeshamHM28
    Copy link
    Contributor Author

    Saga4
    Saga4 previously approved these changes Jul 30, 2025
    Copy link
    Contributor

    @Saga4 Saga4 left a comment

    Choose a reason for hiding this comment

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

    We should think of adding staging-review related E2E tests as a task to linear to track.
    LGTM only small question around generated_tests.

    @HeshamHM28 HeshamHM28 merged commit 2e46088 into main Jul 31, 2025
    18 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.

    2 participants