Skip to content

fix: point missing-config errors to the correct docs URL (#1363)#1372

Closed
chethanuk wants to merge 2 commits intovolcengine:mainfrom
chethanuk:fix/1363-config-docs-url
Closed

fix: point missing-config errors to the correct docs URL (#1363)#1372
chethanuk wants to merge 2 commits intovolcengine:mainfrom
chethanuk:fix/1363-config-docs-url

Conversation

@chethanuk
Copy link
Copy Markdown
Contributor

Summary

  • update missing-config runtime errors to point to the current OpenViking docs URL
  • tighten regression tests for the server and shared config loader paths
  • address review feedback by removing flaky filesystem assumptions and duplicate exception calls

Validation

  • python3 -m pytest tests/test_server_config_loader.py tests/test_config_loader.py
  • python3 -m ruff check tests/test_server_config_loader.py tests/test_config_loader.py

Closes #1363

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 11, 2026

PR Reviewer Guide 🔍

(Review updated until commit cf39c54)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

1363 - PR Code Verified

Compliant requirements:

  • Added regression tests to verify the correct docs URL is used in missing-config errors

Requires further human verification:

  • Verify the actual error message files (not shown in diff) are updated to use the correct URL
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🏅 Score: 75
🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Add regression tests for config docs URL

Relevant files:

  • tests/test_config_loader.py
  • tests/test_server_config_loader.py

Sub-PR theme: Remove unused attributes from Volcengine embedder

Relevant files:

  • openviking/models/embedder/volcengine_embedders.py

⚡ Recommended focus areas for review

Potential Regression

Removed instance attributes self._ark_kwargs and self._async_client without checking if they are used elsewhere in the class. If the class has async embedding methods that rely on these attributes (e.g., an embed_documents_async method that initializes an async client), this change will cause AttributeErrors at runtime.

    self.api_key = api_key
    self.api_base = api_base

    if not self.api_key:
        raise ValueError("api_key is required")

    ark_kwargs = {"api_key": self.api_key}
    if self.api_base:
        ark_kwargs["base_url"] = self.api_base
    self.client = volcenginesdkarkruntime.Ark(**ark_kwargs)

def _update_telemetry_token_usage(self, response) -> None:
    usage = getattr(response, "usage", None)
    if not usage:
        return

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 11, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Restore async client attributes

The _ark_kwargs and _async_client attributes were removed, which likely breaks async
embedding functionality (if present). These attributes are typically used to lazily
initialize an async Ark client for async methods. Restore them to avoid
AttributeError in async code paths.

openviking/models/embedder/volcengine_embedders.py [336-339]

 ark_kwargs = {"api_key": self.api_key}
 if self.api_base:
     ark_kwargs["base_url"] = self.api_base
 self.client = volcenginesdkarkruntime.Ark(**ark_kwargs)
+self._ark_kwargs = ark_kwargs
+self._async_client = None
Suggestion importance[1-10]: 8

__

Why: Removing _ark_kwargs and _async_client could break async embedding methods that rely on these attributes, so restoring them prevents potential AttributeErrors in async code paths, making this a high-impact correctness fix.

Medium

@qin-ctx
Copy link
Copy Markdown
Collaborator

qin-ctx commented Apr 13, 2026

Why was openviking/models/embedder/volcengine_embedders.py modified?

@qin-ctx qin-ctx closed this Apr 13, 2026
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Apr 13, 2026
@qin-ctx qin-ctx reopened this Apr 13, 2026
@github-actions
Copy link
Copy Markdown

Persistent review updated to latest commit cf39c54

@github-actions
Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Restore async client attributes

The _ark_kwargs and _async_client attributes were removed, which likely breaks async
embedding functionality (if present). These attributes are typically used to lazily
initialize an async Ark client for async methods. Restore them to avoid
AttributeError in async code paths.

openviking/models/embedder/volcengine_embedders.py [336-339]

 ark_kwargs = {"api_key": self.api_key}
 if self.api_base:
     ark_kwargs["base_url"] = self.api_base
 self.client = volcenginesdkarkruntime.Ark(**ark_kwargs)
+self._ark_kwargs = ark_kwargs
+self._async_client = None
Suggestion importance[1-10]: 8

__

Why: Removing _ark_kwargs and _async_client could break async embedding methods that rely on these attributes, so restoring them prevents potential AttributeErrors in async code paths, making this a high-impact correctness fix.

Medium

@qin-ctx qin-ctx self-assigned this Apr 13, 2026
@qin-ctx
Copy link
Copy Markdown
Collaborator

qin-ctx commented Apr 14, 2026

fixed by #1370

@qin-ctx qin-ctx closed this Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Bug]: openviking-server help message points to non-existent domain openviking.dev

2 participants