Skip to content

Conversation

@mohammedahmed18
Copy link
Contributor

@mohammedahmed18 mohammedahmed18 commented Oct 9, 2025

PR Type

Enhancement, Bug fix


Description

  • Return specific config validation errors to clients

  • Improve pyproject.toml validator with detailed checks

  • Adjust LSP responses to include failure reasons

  • Minor variable rename to avoid unused warning


Diagram Walkthrough

flowchart LR
  A["cmd_init.is_valid_pyproject_toml returns (config, reason)"] -- "called by" --> B["lsp.beta.init_project"]
  A -- "validates module_root / tests_root exist" --> C["Detailed error messages"]
  B -- "on invalid" --> D["LSP error response includes reason"]
Loading

File Walkthrough

Relevant files
Enhancement
cmd_init.py
Enrich pyproject.toml validation with reasons                       

codeflash/cli_cmds/cmd_init.py

  • Refactor validator to return (config, reason)
  • Add explicit checks and messages for missing/invalid fields
  • Update callers to handle tuple return
  • Rename unused variable from parse_config_file result
+21/-11 
beta.py
Include validation reason in LSP error response                   

codeflash/lsp/beta.py

  • Consume new validator tuple (config, reason)
  • Return LSP error including specific invalid reason
+2/-2     

@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

API Change

The signature of is_valid_pyproject_toml now returns (config, reason) instead of just config. Verify all call sites are updated accordingly to avoid tuple handling mistakes or ignored error messages.

def is_valid_pyproject_toml(pyproject_toml_path: Path) -> tuple[dict[str, Any] | None, str]:  # noqa: PLR0911
    if not pyproject_toml_path.exists():
        return None, f"Configuration file not found: {pyproject_toml_path}"

    try:
        config, _ = parse_config_file(pyproject_toml_path)
    except Exception as e:
        return None, f"Failed to parse configuration: {e}"

    module_root = config.get("module_root")
    if not module_root:
        return None, "Missing required field: 'module_root'"

    if not Path(module_root).is_dir():
        return None, f"Invalid 'module_root': directory does not exist at {module_root}"

    tests_root = config.get("tests_root")
    if not tests_root:
        return None, "Missing required field: 'tests_root'"

    if not Path(tests_root).is_dir():
        return None, f"Invalid 'tests_root': directory does not exist at {tests_root}"

    return config, ""
UX Message

Error response returns message formatted as "reason: {reason}". Consider returning the reason directly (and/or a structured field) to avoid redundant prefixing and improve client display.

config, reason = 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": f"reason: {reason}", "pyprojectPath": pyproject_toml_path}

@github-actions
Copy link

github-actions bot commented Oct 9, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Resolve relative paths correctly

Resolve non-deterministic path checks by resolving module_root and tests_root
relative to pyproject_toml_path.parent when they are not absolute. This avoids false
negatives when the LSP runs from a different CWD than where the config lives.

codeflash/cli_cmds/cmd_init.py [158-181]

 def is_valid_pyproject_toml(pyproject_toml_path: Path) -> tuple[dict[str, Any] | None, str]:  # noqa: PLR0911
     if not pyproject_toml_path.exists():
         return None, f"Configuration file not found: {pyproject_toml_path}"
 
     try:
         config, _ = parse_config_file(pyproject_toml_path)
     except Exception as e:
         return None, f"Failed to parse configuration: {e}"
 
     module_root = config.get("module_root")
     if not module_root:
         return None, "Missing required field: 'module_root'"
-
-    if not Path(module_root).is_dir():
-        return None, f"Invalid 'module_root': directory does not exist at {module_root}"
+    module_root_path = Path(module_root)
+    if not module_root_path.is_absolute():
+        module_root_path = (pyproject_toml_path.parent / module_root_path).resolve()
+    if not module_root_path.is_dir():
+        return None, f"Invalid 'module_root': directory does not exist at {module_root_path}"
 
     tests_root = config.get("tests_root")
     if not tests_root:
         return None, "Missing required field: 'tests_root'"
-
-    if not Path(tests_root).is_dir():
-        return None, f"Invalid 'tests_root': directory does not exist at {tests_root}"
+    tests_root_path = Path(tests_root)
+    if not tests_root_path.is_absolute():
+        tests_root_path = (pyproject_toml_path.parent / tests_root_path).resolve()
+    if not tests_root_path.is_dir():
+        return None, f"Invalid 'tests_root': directory does not exist at {tests_root_path}"
 
     return config, ""
Suggestion importance[1-10]: 8

__

Why: Correctly resolving module_root and tests_root relative to the pyproject.toml prevents false negatives when CWD differs; it's accurate and improves robustness without altering behavior otherwise.

Medium
General
Log and return explicit reason

Surface the specific reason to the user in the log message for easier diagnosis and
keep the error response concise. This prevents hiding the actionable context in API
payloads only.

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

 config, reason = 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": f"reason: {reason}", "pyprojectPath": pyproject_toml_path}
+    server.show_message_log(f"pyproject.toml is not valid: {reason}", "Error")
+    return {"status": "error", "message": reason, "pyprojectPath": pyproject_toml_path}
Suggestion importance[1-10]: 7

__

Why: Enhances error visibility by logging the specific reason and simplifying the response; aligns with the new is_valid_pyproject_toml signature and is contextually correct.

Medium
Show reason when prompting reconfigure

When the config is invalid, include the validation reason in the decision path to
inform users why reconfiguration is prompted. This avoids silent failures and aids
debugging during CLI runs.

codeflash/cli_cmds/cmd_init.py [193-195]

-config, _message = is_valid_pyproject_toml(pyproject_toml_path)
+config, message = is_valid_pyproject_toml(pyproject_toml_path)
 if config is None:
+    console = Console()  # assuming rich Console is available in this module's context
+    console.print(f"[red]Invalid pyproject.toml:[/red] {message}")
     return True, None
Suggestion importance[1-10]: 5

__

Why: Informing users why reconfiguration is needed is helpful and consistent with the new validator returning a message, but adding console printing here is a minor UX improvement and assumes Console availability.

Low

if "tests_root" not in config or config["tests_root"] is None or not Path(config["tests_root"]).is_dir():
return None
if not Path(tests_root).is_dir():
return None, f"Invalid 'tests_root': directory does not exist at {tests_root}"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We can do test framework check later in this.

@Saga4 Saga4 merged commit a307d81 into main Oct 9, 2025
19 of 21 checks passed
@mohammedahmed18 mohammedahmed18 deleted the lsp/return-why-codeflash-config-not-valid branch October 10, 2025 08:52
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