LCORE-731: Updated docstrings in utils module #912
Conversation
WalkthroughExpanded and clarified docstrings across many utilities; introduced a guarded runtime warning in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/utils/common.py (3)
18-36: Docstring contradiction aboutloggerbeing “intentionally undocumented”.
Theregister_mcp_servers_asyncdocstring documentsloggerunder “Parameters”, but “Notes” claims it’s intentionally undocumented (Lines 25-35). Please drop/adjust that note to avoid confusing readers. As per coding guidelines / learnings, keep Google-style sections consistent.@@ - Notes: - - The `logger` parameter is used for debug/info logging and is - intentionally undocumented as a common service. + Notes: + - The `logger` parameter is used for debug/info logging. - Exceptions from the LlamaStack client (network/errors during initialization or registration) are not caught here and will propagate to the caller.
103-116:run_once_async“Returns” section is incorrect for a decorator.
run_once_asyncreturns a callable (thewrapper), not “Any: The result produced by the wrapped coroutine” (Lines 113-115). That “Any” return description belongs onwrapper(which you already documented below).def run_once_async(func: Callable) -> Callable: @@ - Returns: - Any: The result produced by the wrapped coroutine, or the exception it - raised propagated to callers. + Returns: + Callable: A wrapped async function that will execute at most once and + return the cached awaited result (or re-raise the cached exception) + on subsequent calls.
60-80: Rewrite docstring to use Google style (Args:/Returns:) and clarifyprovider_resource_idcomparison semantics.Two issues in the docstring (lines 60–80):
- The statement "whose
nameis not present in the client'sprovider_resource_idlist" is misleading—provider_resource_idis a provider-specific identifier. Clarify that the code checks whether each MCP server's name has already been registered by comparing against theprovider_resource_idvalues from existing toolgroups, and explain why this field serves as the duplicate-check key.- Docstring uses
Parameters:with typed bullet format instead of Google style (Args:/Returns:), violating coding guidelines forsrc/**/*.pyfiles.src/utils/endpoints.py (1)
392-405:get_temp_agentdocstring return contract is wrong (and mismatched ordering).
The signature returnstuple[AsyncAgent, str, str], and the implementation returns(agent, session_id, conversation_id)(Line 423), but the docstring saystuple[AsyncAgent, str]: agent and session_id(Lines 403-405). Please fix the docstring to match the real return shape and ordering (or fix the return ordering, but that’s beyond a docstring-only PR).- Returns: - tuple[AsyncAgent, str]: A tuple containing the agent and session_id. + Returns: + tuple[AsyncAgent, str, str | None]: A tuple of (agent, session_id, conversation_id).
🧹 Nitpick comments (9)
src/utils/common.py (1)
121-132: Prefer Google docstring section headers (Args/Returns) in touched docstrings.
Thewrapperdocstring uses “Returns:” but the module otherwise mixes “Parameters:”/“Args:”. Since this PR is explicitly about docstrings, consider standardizing the touched sections to Google style for consistency with repo guidelines.src/utils/endpoints.py (8)
207-218: Docstring is clear, but use GoogleArgs:instead ofParameters:(repo convention).
Forget_topic_summary_system_prompt(Lines 208-218), consider switching toArgs:/Returns:formatting to match guidelines.
256-273: Docstring looks good; consider GoogleArgs:header for consistency.
store_conversation_into_cache’s expanded docstring is helpful, but it usesParameters:(Lines 263-272). Repo guidelines call for Google-style docstrings.
442-461: Good detail; consider tightening wording + GoogleArgs:.
_process_http_sourcedocstring is materially improved. Two small follow-ups (Lines 448-461): useArgs:instead ofParameters:; and consider explicitly stating thatsrcis deduped by raw string match (not normalized URL).
475-504: Docstring is accurate; align to Google style and avoid “truthy/falsy” ambiguity.
In_process_document_id(Lines 482-504), “If provided (truthy)” can be misread; consider “If metadata_map is not None” (even if behavior remains truthiness-based) and useArgs:.
567-582: Docstring improvement is solid; consider typingrag_chunksaslist[Any](doc + signature) for clarity.
The docstring for_process_rag_chunks_for_documentsis clearer now (Lines 572-582). Since you’re documenting “chunk must provide asourceattribute”, you might also want to typerag_chunks: list[Any](or a Protocol) eventually; currentrag_chunks: listis very loose.
622-637: Partial docstring update: consider fully converting to Google-style sections while you’re here.
create_referenced_documentsonly has a small touched line marker (Line 629), but the whole docstring still mixes styles. If you’re updating docstrings in this PR, it may be worth fully standardizing this one toArgs/Returnstoo.
658-672: Nice docstring clarification; align indentation underReturns:.
Increate_referenced_documents_with_metadata(Lines 668-671), the continuation lines underReturns:look under-indented relative to typical Google formatting; consider indenting wrapped lines under the first line’s text block.
685-698: Docstring looks correct; recommend GoogleArgs:header for consistency.
create_referenced_documents_from_chunksdocstring is clear; recommend switchingParameters:→Args:(Lines 691-697).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/utils/common.py(3 hunks)src/utils/endpoints.py(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/utils/endpoints.pysrc/utils/common.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/*.py : All modules must start with descriptive docstrings explaining their purpose
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/*.py : All functions must have docstrings with brief descriptions following Google Python docstring conventions
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/utils/endpoints.py (1)
236-238: UpdateRaisessection to name the exception type.The
Raisessection should specifyHTTPException(the actual exception type raised) rather than "HTTP 403" (the status code). For example: "Raises: HTTPException: If the request includes model or provider and the caller lacks Action.MODEL_OVERRIDE permission (403)."
🧹 Nitpick comments (1)
src/utils/mcp_headers.py (1)
20-20: Inconsistent docstring terminology within the file.The change from
Args:toParameters:creates inconsistency withhandle_mcp_headers_with_toolgroups(line 70), which still usesArgs:. Additionally, Google Python docstring conventions (referenced in the coding guidelines) standardize onArgs:, notParameters:.Consider using
Args:consistently across all functions in this module to align with Google conventions and maintain internal consistency.As per coding guidelines, functions should follow Google Python docstring conventions.
Also applies to: 35-35
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/utils/endpoints.py(10 hunks)src/utils/llama_stack_version.py(1 hunks)src/utils/mcp_headers.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/utils/endpoints.pysrc/utils/mcp_headers.pysrc/utils/llama_stack_version.py
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/app/endpoints/**/*.py : Use FastAPI `HTTPException` with appropriate status codes for API endpoint error handling
Applied to files:
src/utils/endpoints.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (5)
src/utils/llama_stack_version.py (1)
34-37: Documentation enhancement improves clarity.The added
Raisessection clearly documents the exception conditions, making the function's error behavior explicit. This is a valuable improvement for API consumers.src/utils/endpoints.py (4)
208-218: Documentation enhancement clarifies function behavior.The expanded docstring provides clear parameter and return value descriptions, improving API documentation quality.
256-278: Documentation and defensive coding improvement.The expanded docstring clearly documents the function's behavior, including the new edge case (Lines 276-278) where a configured cache is uninitialized. The warning and early return prevent potential errors while maintaining non-fatal behavior.
398-402: Documentation enhancements improve code clarity.The expanded docstrings across these helper functions provide comprehensive parameter descriptions, return value specifications, and behavioral notes. These improvements significantly enhance code maintainability and developer experience.
Also applies to: 445-461, 482-504, 571-581
629-633: Consistent documentation improvements for document creation functions.The enhanced docstrings provide clear descriptions of parameters and return types across this family of related functions, improving API documentation consistency.
Also applies to: 663-671, 691-697
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/utils/mcp_headers.py (2)
15-25: Docstring should follow Google format (Args:), and fix wording/casing.This PR flips to
Parameters:, but your guidelines call for Google-style docstrings (Args:). Also, there are a few grammar/casing issues that reduce clarity.async def mcp_headers_dependency(request: Request) -> dict[str, dict[str, str]]: - """Get the MCP headers dependency to passed to mcp servers. + """Get MCP headers to pass to MCP servers. - mcp headers is a json dictionary or mcp url paths and their respective headers + MCP headers is a JSON dictionary of MCP URL paths to their respective headers. - Parameters: - request (Request): The FastAPI request object. + Args: + request (Request): The FastAPI request object. Returns: - The mcp headers dictionary, or empty dictionary if not found or on json decoding error + The MCP headers dictionary, or an empty dictionary if missing/invalid. """ return extract_mcp_headers(request)
29-40: UseArgs:(Google style) and type the param in the docstring.def extract_mcp_headers(request: Request) -> dict[str, dict[str, str]]: """Extract mcp headers from MCP-HEADERS header. @@ - Parameters: - request: The FastAPI request object + Args: + request (Request): The FastAPI request object. Returns: - The mcp headers dictionary, or empty dictionary if not found or on json decoding error + The MCP headers dictionary, or an empty dictionary if missing/invalid. """src/utils/endpoints.py (2)
248-286: Cache guard: avoid possibleAttributeErrorifconversation_cache_configurationis unset.You added a good “configured-but-uninitialized cache” warning path, but the new
config.conversation_cache_configuration.typedereference will crash ifconversation_cache_configurationcan beNone(or missing). If that’s impossible by construction, ignore—otherwise guard it.def store_conversation_into_cache( @@ ) -> None: @@ - if config.conversation_cache_configuration.type is not None: + cache_cfg = getattr(config, "conversation_cache_configuration", None) + if cache_cfg is None or cache_cfg.type is None: + return cache = config.conversation_cache if cache is None: logger.warning("Conversation cache configured but not initialized") return @@ - if topic_summary and len(topic_summary) > 0: + if topic_summary: cache.set_topic_summary( user_id, conversation_id, topic_summary, _skip_userid_check )
387-424: Fixget_temp_agentreturn type annotation and docstring inconsistencies.The function signature declares
-> tuple[AsyncAgent, str, str], but:
- The actual return is
(agent, session_id, conversation_id)whereconversation_id = None—violating thestrtype annotation- The docstring states
tuple[AsyncAgent, str](incomplete, missing the third element)The return order also differs from
get_agentwhich returns(agent, conversation_id, session_id).Recommended fix: Either:
- Option A (minimal): Update type to
tuple[AsyncAgent, str, str | None]and fix docstring to document all three return values in actual order(agent, session_id, conversation_id), or- Option B (align with
get_agent): Change return order to(agent, conversation_id, session_id)and update the caller insrc/app/endpoints/query.py:194accordingly (currently unpacks asagent, session_id, _ = await get_temp_agent(...)).
🧹 Nitpick comments (6)
src/utils/llama_stack_version.py (1)
26-38: LGTM! Docstring enhancement improves API documentation.The addition of the Raises section accurately documents that
InvalidLlamaStackVersionExceptioncan be raised when the version is outside the supported range or cannot be parsed. This aligns well with the actual behavior in thecompare_versionsfunction.Optional: Consider adding a Parameters section for consistency.
The
compare_versionsfunction below includes a Parameters section in its docstring. For consistency and completeness, you could add a similar section to this function's docstring:Verify the connected Llama Stack's version is within the supported range. This coroutine fetches the Llama Stack version from the provided client and validates it against the configured minimal and maximal supported versions. Raises InvalidLlamaStackVersionException if the detected version is outside the supported range. + + Args: + client: An AsyncLlamaStackClient instance used to query the + Llama Stack version. Raises: InvalidLlamaStackVersionException: If the detected version is outside the supported range or cannot be parsed.src/utils/responses.py (1)
14-16: Docstring uses "Parameters:" instead of Google-style "Args:"The coding guidelines specify Google Python docstring conventions, which use
Args:rather thanParameters:. However, if the team has decided to standardize onParameters:across the codebase, this is acceptable as long as it's applied consistently.If aligning with strict Google style is preferred:
- Parameters: + Args: output_item: A Responses API output item (typically from response.output array).src/utils/quota.py (1)
75-77: Inconsistent docstring format within the same file.
consume_tokensandcheck_tokens_availablewere updated to useParameters:, butget_available_quotasstill usesArgs:. For consistency within this file, consider updating this function as well.- Args: + Parameters: quota_limiters: List of quota limiter instances to query. user_id: Identifier of the user to get quotas for.src/utils/tool_formatter.py (1)
126-131: Redundant type annotation in parameter description.The type is already specified in the function signature
tools: list[dict[str, Any]], so repeating it in the docstring description creates redundancy and maintenance burden.Parameters: - tools: (list[dict[str, Any]]): List of raw tool dictionaries + tools: List of raw tool dictionariessrc/utils/endpoints.py (2)
207-218: Docstring style: preferArgs:overParameters:(Google format).This docstring (and others below) uses
Parameters:; your repo guidelines call for Google convention (Args:).- Parameters: + Args: config (AppConfig): Application configuration from which to read customization/profile settings.
442-461: Docstring style nit: useArgs:consistently; otherwise content looks good.These updated docstrings are clearer, but they use
Parameters:instead ofArgs:per your guidelines.Also applies to: 475-504, 563-582
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/utils/common.py(3 hunks)src/utils/endpoints.py(10 hunks)src/utils/llama_stack_version.py(1 hunks)src/utils/mcp_headers.py(2 hunks)src/utils/quota.py(2 hunks)src/utils/responses.py(1 hunks)src/utils/shields.py(2 hunks)src/utils/token_counter.py(3 hunks)src/utils/tool_formatter.py(2 hunks)src/utils/transcripts.py(2 hunks)src/utils/types.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/utils/types.pysrc/utils/common.pysrc/utils/quota.pysrc/utils/endpoints.pysrc/utils/tool_formatter.pysrc/utils/responses.pysrc/utils/llama_stack_version.pysrc/utils/token_counter.pysrc/utils/shields.pysrc/utils/mcp_headers.pysrc/utils/transcripts.py
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/app/endpoints/**/*.py : Use FastAPI `HTTPException` with appropriate status codes for API endpoint error handling
Applied to files:
src/utils/endpoints.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (15)
src/utils/shields.py (2)
13-28: LGTM!The docstring updates for
get_available_shieldsare consistent and complete. The function properly usesasync deffor the external API call as per coding guidelines.
31-54: LGTM!The docstring updates for
detect_shield_violationsare clear and the function implementation is correct.src/utils/tool_formatter.py (1)
22-29: LGTM!The expanded return documentation clearly describes the structure of the returned dictionary, which is helpful for consumers of this API.
src/utils/types.py (3)
20-40: LGTM!The docstring for
content_to_stris clear and properly documents all the input types the function handles.
144-165: LGTM!The expanded docstring for
append_tool_calls_from_llamais comprehensive and clearly documents the behavior, including the status derivation logic and RAG chunk extraction.
184-192: Clarify the hardcodedround=1value.The comment
# clarify meaning of this attributesuggests this needs attention. Consider either:
- Adding documentation explaining what
roundrepresents- Deriving the value from the actual execution step if available
Is
roundintended to represent the iteration/step number within an agentic loop? If so, should it be extracted fromtecrather than hardcoded?src/utils/token_counter.py (3)
33-47: LGTM!The
__str__docstring clearly describes the returned summary format.
50-62: LGTM!The docstring update for
extract_token_usage_from_turnproperly documents parameters and return type.
103-118: LGTM!The docstring update for
extract_and_update_token_metricsis clear and consistent with the other functions in this file.src/utils/common.py (2)
55-79: LGTM!The expanded docstring for
_register_mcp_toolgroups_asyncclearly documents all parameters with their types and purpose.
103-137: LGTM with a note on behavior.The docstring accurately describes the run-once semantics. Note that subsequent calls with different arguments will still return the result from the first invocation (as documented), which is the intended behavior for initialization patterns.
src/utils/transcripts.py (3)
27-53: LGTM!The expanded docstring for
construct_transcripts_paththoroughly explains the path construction logic and the purpose of each parameter.
56-86: LGTM!The docstring correctly documents the parameters including the newly documented
model_idandprovider_id, and appropriately includes theRaisessection for potential file I/O errors.
92-110: Verify the distinction between model/provider metadata fields.The stored metadata includes both
provider/model(from function parameters) andquery_provider/query_model(from query_request). This appears intentional to capture both the actual model used and the originally requested model, but consider adding a comment clarifying this distinction for future maintainers.#!/bin/bash # Check how store_transcript is called to understand the provider/model distinction rg -n -A 5 'store_transcript\(' --type pysrc/utils/endpoints.py (1)
231-239: RBAC override docstring now namesHTTPException— good.Minor wording tweak if you want it closer to Google style: “Raises: HTTPException: If … (status code 403).”
1 participant
Description
common.pyandendpoints.pyquota.pyandresponses.pyllama_stack_version.pyandmcp_headers.pyshield.py,token_counter.py,andtool_formatter.pytranscripts.pyandtypes.pyType of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.