Skip to content

Conversation

@mohammedahmed18
Copy link
Contributor

@mohammedahmed18 mohammedahmed18 commented Oct 8, 2025

PR Type

Enhancement, Bug fix


Description

  • Return selected config file path

  • Detect top-level pyproject.toml presence

  • Validate presence of codeflash config section

  • Improve error payloads and logging


Diagram Walkthrough

flowchart LR
  A["init_project(params)"] -- "has config_file?" --> B["prepare_optimizer_arguments(path)"]
  A -- "no config_file" --> C["_find_pyproject_toml(root)"]
  C -- "found with [tool.codeflash]" --> D["prepare_optimizer_arguments(path)"]
  C -- "found without section" --> E["error: no codeflash config + pyprojectPath"]
  C -- "not found" --> F["error: no pyproject.toml"]
  D -- "validate" --> G["is_valid_pyproject_toml(path)"]
  G -- "invalid" --> H["error: not valid + pyprojectPath"]
  G -- "valid" --> I["return success metadata"]
Loading

File Walkthrough

Relevant files
Enhancement
beta.py
Robust pyproject discovery and validation in LSP init       

codeflash/lsp/beta.py

  • Find pyproject returns path and has-config flag
  • Prefer provided config_file (params or server.args)
  • Error if pyproject lacks [tool.codeflash], include pyprojectPath
  • Shorten invalid-config error message and include pyprojectPath
+24/-19 

@github-actions
Copy link

github-actions bot commented Oct 8, 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 Issue

In _find_pyproject_toml, top-level pyproject is only set when found at depth==0; deeper valid configs return immediately, but if none contain [tool.codeflash], the function returns the top-level path even if that file might not exist (None) or may not be the intended config when multiple pyprojects exist. Reviewer should confirm intended precedence and behavior when multiple pyproject.toml files are present and none include the codeflash section.

def _find_pyproject_toml(workspace_path: str) -> tuple[Path | None, bool]:
    workspace_path_obj = Path(workspace_path)
    max_depth = 2
    base_depth = len(workspace_path_obj.parts)
    top_level_pyproject = None

    for root, dirs, files in os.walk(workspace_path_obj):
        depth = len(Path(root).parts) - base_depth
        if depth > max_depth:
            # stop going deeper into this branch
            dirs.clear()
            continue

        if "pyproject.toml" in files:
            file_path = Path(root) / "pyproject.toml"
            if depth == 0:
                top_level_pyproject = file_path
            with file_path.open("r", encoding="utf-8", errors="ignore") as f:
                for line in f:
                    if line.strip() == "[tool.codeflash]":
                        return file_path.resolve(), True
    return top_level_pyproject, False
Type Consistency

init_project may return dicts where 'pyprojectPath' is a Path object; ensure the LSP transport expects a string path and consider converting to str to avoid JSON serialization or client-side type issues.

pyproject_toml_path, has_codeflash_config = _find_pyproject_toml(params.root_path_abs)
if pyproject_toml_path and has_codeflash_config:
    server.show_message_log(f"Found pyproject.toml at: {pyproject_toml_path}", "Info")
    server.prepare_optimizer_arguments(pyproject_toml_path)
elif pyproject_toml_path and not has_codeflash_config:
    return {
        "status": "error",
        "message": "pyproject.toml found in workspace, but no codeflash config.",
        "pyprojectPath": pyproject_toml_path,
    }
else:
    return {"status": "error", "message": "No pyproject.toml found in workspace."}
Error Messaging

The "not valid" error message is terse and changed from the previous explicit phrasing; this may break clients relying on specific text. Ensure clients/extensions parsing these messages are updated or keep compatibility wording.

config = is_valid_pyproject_toml(pyproject_toml_path)
if config is None:
    server.show_message_log("pyproject.toml is not valid", "Error")
    return {"status": "error", "message": "not valid", "pyprojectPath": pyproject_toml_path}

@github-actions
Copy link

github-actions bot commented Oct 8, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Ensure JSON-serializable response

Convert pyproject_toml_path to string when placing into the response to ensure it is
JSON-serializable over LSP. Returning a Path object can cause serialization errors.

codeflash/lsp/beta.py [175-179]

 elif pyproject_toml_path and not has_codeflash_config:
     return {
         "status": "error",
         "message": "pyproject.toml found in workspace, but no codeflash config.",
-        "pyprojectPath": pyproject_toml_path,
+        "pyprojectPath": str(pyproject_toml_path),
     }
Suggestion importance[1-10]: 8

__

Why: Returning Path objects in LSP responses can break JSON serialization; converting to str is a correct, high-impact fix for interoperability and stability.

Medium
Normalize returned path resolution

Ensure returned paths are resolved for consistency and to avoid mixed
relative/absolute paths. Also, always return the resolved path for
top_level_pyproject to prevent downstream consumers from handling two path formats.

codeflash/lsp/beta.py [134-155]

 def _find_pyproject_toml(workspace_path: str) -> tuple[Path | None, bool]:
     workspace_path_obj = Path(workspace_path)
     max_depth = 2
     base_depth = len(workspace_path_obj.parts)
-    top_level_pyproject = None
+    top_level_pyproject: Path | None = None
 
     for root, dirs, files in os.walk(workspace_path_obj):
         depth = len(Path(root).parts) - base_depth
         if depth > max_depth:
-            # stop going deeper into this branch
             dirs.clear()
             continue
 
         if "pyproject.toml" in files:
-            file_path = Path(root) / "pyproject.toml"
+            file_path = (Path(root) / "pyproject.toml").resolve()
             if depth == 0:
                 top_level_pyproject = file_path
             with file_path.open("r", encoding="utf-8", errors="ignore") as f:
                 for line in f:
                     if line.strip() == "[tool.codeflash]":
-                        return file_path.resolve(), True
-    return top_level_pyproject, False
+                        return file_path, True
+    return (top_level_pyproject.resolve() if top_level_pyproject else None), False
Suggestion importance[1-10]: 7

__

Why: Ensuring all returned Path values are resolved avoids mixed relative/absolute paths and downstream inconsistencies; the change aligns with existing use of .resolve() and is accurate to the diff context.

Medium
General
Preserve stable error message

Keep the user-facing error message consistent to avoid breaking clients that depend
on exact strings. Include a stringified path for serialization and better UX.

codeflash/lsp/beta.py [196-199]

 if config is None:
     server.show_message_log("pyproject.toml is not valid", "Error")
-    return {"status": "error", "message": "not valid", "pyprojectPath": pyproject_toml_path}
+    return {
+        "status": "error",
+        "message": "pyproject.toml is not valid",
+        "pyprojectPath": str(pyproject_toml_path) if pyproject_toml_path else None,
+    }
Suggestion importance[1-10]: 8

__

Why: The PR regressed a previously stable error string; restoring it and stringifying the path maintains client compatibility and prevents serialization issues.

Medium

@mohammedahmed18 mohammedahmed18 merged commit 952bb6d into main Oct 8, 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.

2 participants