Skip to content

Conversation

@mohammedahmed18
Copy link
Contributor

@mohammedahmed18 mohammedahmed18 commented Oct 9, 2025

PR Type

Bug fix, Enhancement


Description

  • Revert LSP optimization initialization flow

  • Simplify patch creation API usage

  • Improve error handling on patch failure

  • Return task identifier in response


Diagram Walkthrough

flowchart LR
  InitOld["Server-held init result"] -- reuse --> OptimRun["Run optimization"]
  OptimRun -- best found --> PatchCreate["Create patch from worktree"]
  PatchCreate -- success --> RespondOK["LSP success response"]
  PatchCreate -- failure --> RespondErr["LSP error response"]
Loading

File Walkthrough

Relevant files
Enhancement
perform_optimization.py
Revert to server-provided init and simplify patch flow     

codeflash/lsp/features/perform_optimization.py

  • Reuse server.current_optimization_init_result and existing optimizer.
  • Remove module prep and optimizer creation logic.
  • Replace metadata-based patch creation with simplified API.
  • Add failure check, return task_id, adjust response fields.
+17/-56 

@github-actions
Copy link

github-actions bot commented Oct 9, 2025

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

Possible None access

The code assumes server.current_optimization_init_result and server.optimizer.current_function_optimizer are present and valid without checks; if either is None or not initialized, this can raise exceptions during optimization start.

def sync_perform_optimization(server: CodeflashLanguageServer, params) -> dict[str, str]:  # noqa: ANN001
    server.show_message_log(f"Starting optimization for function: {params.functionName}", "Info")
    should_run_experiment, code_context, original_helper_code = server.current_optimization_init_result
    function_optimizer = server.optimizer.current_function_optimizer
    current_function = function_optimizer.function_to_optimize
Patch creation check

create_diff_patch_from_worktree return value is only checked for truthiness; clarify and handle different failure modes (e.g., exceptions vs. empty path) to avoid silent failures and ensure consistent error messaging.

patch_path = create_diff_patch_from_worktree(
    server.optimizer.current_worktree, relative_file_paths, function_to_optimize_qualified_name
)

if not patch_path:
    return {
        "functionName": params.functionName,
        "status": "error",
        "message": "Failed to create a patch for optimization",
    }
Missing patch metadata

Response no longer includes patch id or original file path metadata, which may be relied upon by clients; confirm API contract and update consumers or include minimal identifiers.

server.show_message_log(f"Optimization completed for {params.functionName} with {speedup:.2f}x speedup", "Info")

return {
    "functionName": params.functionName,
    "status": "success",
    "message": "Optimization completed successfully",
    "extra": f"Speedup: {speedup:.2f}x faster",
    "patch_file": str(patch_path),
    "task_id": params.task_id,
    "explanation": best_optimization.explanation_v2,
}

@github-actions
Copy link

github-actions bot commented Oct 9, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add initialization guards

Guard against missing initialization state to prevent attribute errors. If the
current optimizer or init result is absent, return a clear error instead of
dereferencing None.

codeflash/lsp/features/perform_optimization.py [12-14]

+if not getattr(server, "current_optimization_init_result", None):
+    return {"functionName": params.functionName, "status": "error", "message": "Optimization not initialized"}
 should_run_experiment, code_context, original_helper_code = server.current_optimization_init_result
-function_optimizer = server.optimizer.current_function_optimizer
+
+function_optimizer = getattr(server.optimizer, "current_function_optimizer", None)
+if not function_optimizer:
+    return {"functionName": params.functionName, "status": "error", "message": "No function optimizer found"}
 current_function = function_optimizer.function_to_optimize
Suggestion importance[1-10]: 7

__

Why: Guarding server.current_optimization_init_result and server.optimizer.current_function_optimizer prevents attribute errors; it's contextually accurate given the new code directly dereferences them. Moderate impact as it's defensive error handling.

Medium
General
Pre-validate patch prerequisites

Validate relative_file_paths before creating the patch to avoid generating empty or
invalid diffs. Also ensure server.optimizer.current_worktree is present to prevent
failures.

codeflash/lsp/features/perform_optimization.py [92-101]

+if not relative_file_paths:
+    return {
+        "functionName": params.functionName,
+        "status": "error",
+        "message": "No files changed during optimization",
+    }
+
+if not getattr(server.optimizer, "current_worktree", None):
+    return {
+        "functionName": params.functionName,
+        "status": "error",
+        "message": "Worktree not available for patch creation",
+    }
+
 patch_path = create_diff_patch_from_worktree(
     server.optimizer.current_worktree, relative_file_paths, function_to_optimize_qualified_name
 )
 
 if not patch_path:
     return {
         "functionName": params.functionName,
         "status": "error",
         "message": "Failed to create a patch for optimization",
     }
Suggestion importance[1-10]: 6

__

Why: Validating relative_file_paths and presence of current_worktree reduces failure modes before calling create_diff_patch_from_worktree. It's reasonable and consistent with nearby code, offering robustness but not fixing a critical bug.

Low
Handle missing optional fields

Protect against division by zero or invalid baseline runtimes when computing
speedup. If the baseline is zero or non-positive, return an explicit error to avoid
ZeroDivisionError and misleading results.

codeflash/lsp/features/perform_optimization.py [103-113]

 server.show_message_log(f"Optimization completed for {params.functionName} with {speedup:.2f}x speedup", "Info")
 
 return {
     "functionName": params.functionName,
     "status": "success",
     "message": "Optimization completed successfully",
     "extra": f"Speedup: {speedup:.2f}x faster",
     "patch_file": str(patch_path),
-    "task_id": params.task_id,
+    "task_id": getattr(params, "task_id", None),
     "explanation": best_optimization.explanation_v2,
 }
Suggestion importance[1-10]: 3

__

Why: The change only makes task_id access defensive with getattr, which is a minor improvement; it does not address the earlier-mentioned speedup validation and offers limited impact on correctness.

Low

@mohammedahmed18 mohammedahmed18 merged commit 7b203a3 into main Oct 9, 2025
20 of 22 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