Skip to content

Conversation

@mohammedahmed18
Copy link
Contributor

@mohammedahmed18 mohammedahmed18 commented Oct 28, 2025

PR Type

Bug fix


Description

  • Guard LSP logging when LSP disabled

  • Prevent message serialization if LSP off


Diagram Walkthrough

flowchart LR
  Logger["LSP Logger"] -- "LSP disabled" --> SkipLog["Return early (no log)"]
  Logger -- "LSP enabled" --> LSPPath["Proceed with LSP logging"]
  Serializer["LSP Message.serialize"] -- "LSP disabled" --> EmptyStr["Return empty string"]
  Serializer -- "LSP enabled" --> SerializeFlow["Fetch context and serialize"]
Loading

File Walkthrough

Relevant files
Bug fix
lsp_logger.py
Early return in logger when LSP disabled                                 

codeflash/lsp/lsp_logger.py

  • Add early return when lsp_enabled is False.
  • Ensures LSP-targeted logs are skipped if disabled.
+4/-0     
lsp_message.py
Guard message serialization by LSP enabled state                 

codeflash/lsp/lsp_message.py

  • Import is_LSP_enabled helper.
  • Return empty string from serialize if LSP disabled.
+3/-1     

@github-actions
Copy link

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

Behavior Change

Early-return when LSP is disabled skips all logging, including non-LSP fallbacks. Verify that messages intended to go to standard logging are not inadvertently suppressed when LSP is off.

if not lsp_enabled:
    # it's for LSP and LSP is disabled
    return
Silent Drop

Returning an empty string when LSP is disabled may be indistinguishable from serializing to an intentional empty payload. Consider clearer signaling or caller-side handling to avoid ambiguity.

if not is_LSP_enabled():
    return ""

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid dropping non-LSP logs

Guarding only on not lsp_enabled can drop non-LSP messages when LSP is disabled.
Ensure this early return only triggers for messages intended exclusively for LSP
(e.g., tags.lsp_only). This prevents losing standard logs when LSP is off.

codeflash/lsp/lsp_logger.py [113-115]

-if not lsp_enabled:
-    # it's for LSP and LSP is disabled
+if not lsp_enabled and getattr(tags, "lsp_only", False):
+    # it's for LSP only and LSP is disabled
     return
Suggestion importance[1-10]: 7

__

Why: The added early return at lines 113-115 unconditionally drops logs when lsp_enabled is false, potentially suppressing non-LSP messages; guarding with tags.lsp_only is a reasonable, context-aware fix. Impact is moderate to high for correctness, and the improved code aligns with the existing snippet.

Medium
General
Use None instead of empty string

Returning an empty string makes call sites treat it as a valid serialized message
and may emit empty payloads. Return None to signal "no serialization" and let
callers optionally skip sending or handle it explicitly.

codeflash/lsp/lsp_message.py [36-41]

-def serialize(self) -> str:
+def serialize(self) -> Optional[str]:
     if not is_LSP_enabled():
-        return ""
+        return None
     from codeflash.lsp.beta import server
Suggestion importance[1-10]: 6

__

Why: Suggesting Optional[str] with None avoids ambiguous empty payloads and can improve call-site handling, but it changes the method's contract and requires broader updates not shown here. The existing snippet matches the new hunk and the improved code accurately reflects the proposed change.

Low

@KRRT7 KRRT7 enabled auto-merge October 30, 2025 06:13
@KRRT7 KRRT7 merged commit b804aca into main Oct 30, 2025
21 of 22 checks passed
@KRRT7 KRRT7 deleted the lsp/fix-lsp-tag branch October 30, 2025 07:42
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