Skip to content

Conversation

@HeshamHM28
Copy link
Contributor

@HeshamHM28 HeshamHM28 commented Oct 23, 2025

PR Type

Bug fix, Enhancement


Description

  • Fix API key precheck in user lookup

  • Separate key validation from optimizer init

  • Initialize optimizer after successful key save

  • Improve exception messages with details


Diagram Walkthrough

flowchart LR
  A["provide_api_key()"] -- "validate format" --> B["_check_api_key_validity()"]
  B -- "success" --> C["save_api_key_to_rc()"]
  C -- "success" --> D["_initialize_optimizer()"]
  A -- "error w/ details" --> E["return error"]
  F["check_api_key()"] -- "validate and init" --> G["_initialize_optimizer_if_api_key_is_valid()"]
  G -- "now uses _check + _init" --> D
  H["get_user_id()"] -- "skip ensure if api_key provided" --> I["make_cfapi_request()"]
Loading

File Walkthrough

Relevant files
Bug fix
cfapi.py
Fix api key precheck in get_user_id                                           

codeflash/api/cfapi.py

  • Respect provided api_key before ensuring stored key
  • Prevents unnecessary prompts/reads when explicit key passed
+1/-1     
Enhancement
beta.py
Refactor key validation and optimizer init flow                   

codeflash/lsp/beta.py

  • Split validation _check_api_key_validity from initialization
  • Add _initialize_optimizer helper
  • Update flows in check_api_key and provide_api_key
  • Improve exception messages and comments
+24/-12 

@github-actions github-actions bot changed the title [Fix] API key saving logic [Fix] API key saving logic Oct 23, 2025
@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

Missing Space in Error Message

The new exception messages concatenate the exception string without a preceding space, resulting in awkward messages like "keyErrorMessage". Consider adding a space or using ': ' before the exception text.

except Exception as e:
    return {"status": "error", "message": f"something went wrong while validating the api key{e!s}"}
Comment Clarity

The added comment has a duplicated hash which may be accidental and could confuse linters or readers. Clean it up for consistency.

# # clear cache to ensure the new api key is used
Optimizer Init Ordering

Optimizer is initialized before confirming API key persistence succeeds. If saving fails after initialization, state may become inconsistent. Consider initializing only after a successful save.

key_check_result = _check_api_key_validity(api_key)
if key_check_result.get("status") != "success":
    return key_check_result
user_id = key_check_result["user_id"]
result = save_api_key_to_rc(api_key)
# initialize optimizer with the new api key
_initialize_optimizer(server)
if not is_successful(result):

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Treat empty keys as missing

This condition allows an empty string API key (e.g., "") to bypass the fallback and
be sent downstream, likely causing invalid auth. Treat empty or whitespace-only keys
as missing to trigger ensure_codeflash_api_key().

codeflash/api/cfapi.py [94-95]

-if not api_key and not ensure_codeflash_api_key():
+if not (api_key and api_key.strip()) and not ensure_codeflash_api_key():
     return None
Suggestion importance[1-10]: 7

__

Why: Valid point: an empty string would pass if not api_key but current code already treats falsy strings as missing; however .strip() also catches whitespace-only keys, improving robustness with low risk.

Medium
General
Ensure initialization uses persisted key

You initialize the optimizer before persisting the key and without ensuring caches
reflect the newly saved key, risking stale state. Save the key first, clear caches
once more, then initialize to guarantee consistency.

codeflash/lsp/beta.py [317-327]

-# # clear cache to ensure the new api key is used
+# clear cache to ensure the new api key is used
 get_codeflash_api_key.cache_clear()
 get_user_id.cache_clear()
 
 key_check_result = _check_api_key_validity(api_key)
 if key_check_result.get("status") != "success":
     return key_check_result
+
 user_id = key_check_result["user_id"]
 result = save_api_key_to_rc(api_key)
-# initialize optimizer with the new api key
+if not is_successful(result):
+    return {"status": "error", "message": result.failure()}
+
+# clear caches post-save to avoid stale reads, then initialize
+get_codeflash_api_key.cache_clear()
+get_user_id.cache_clear()
 _initialize_optimizer(server)
Suggestion importance[1-10]: 6

__

Why: Reordering to save the key and re-clear caches before initializing can avoid stale state; impact is moderate and aligns with consistency, though current flow likely works since validity is checked prior to save.

Low
Fix error message formatting

The error message concatenates text without spacing, producing unreadable messages.
Add a separator to ensure clarity and avoid leaking raw exception types accidentally
in UX.

codeflash/lsp/beta.py [306-307]

 except Exception as e:
-    return {"status": "error", "message": f"something went wrong while validating the api key{e!s}"}
+    return {"status": "error", "message": f"something went wrong while validating the api key: {e!s}"}
Suggestion importance[1-10]: 5

__

Why: The existing code lacks a space before the exception text; adding ": " improves readability, but it's a minor UX fix and slightly increases risk of leaking details.

Low

@HeshamHM28 HeshamHM28 enabled auto-merge October 23, 2025 14:28
@HeshamHM28 HeshamHM28 merged commit 226d10e into main Oct 23, 2025
22 of 23 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