Skip to content

chore: 编译子命令失败报错, golang版本最低要求1.22+#444

Merged
qin-ctx merged 3 commits intomainfrom
fix/agfs
Mar 5, 2026
Merged

chore: 编译子命令失败报错, golang版本最低要求1.22+#444
qin-ctx merged 3 commits intomainfrom
fix/agfs

Conversation

@chuanbao666
Copy link
Collaborator

编译子命令失败报错, golang版本最低要求1.22+

Type of Change

  • New feature (feat)
  • Bug fix (fix)
  • Documentation (docs)
  • Refactoring (refactor)
  • Other

Testing

描述如何测试这些更改:

  • Unit tests pass
  • Manual testing completed

Related Issues

Checklist

  • Code follows project style guidelines
  • Tests added for new functionality
  • Documentation updated (if needed)
  • All tests pass

@chuanbao666 chuanbao666 marked this pull request as ready for review March 5, 2026 12:45
@chuanbao666 chuanbao666 marked this pull request as draft March 5, 2026 12:46
@qin-ctx qin-ctx marked this pull request as ready for review March 5, 2026 12:52
@qin-ctx qin-ctx merged commit 8c740fd into main Mar 5, 2026
41 of 43 checks passed
@qin-ctx qin-ctx deleted the fix/agfs branch March 5, 2026 12:52
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Mar 5, 2026
@github-actions
Copy link

github-actions bot commented Mar 5, 2026

PR Reviewer Guide 🔍

(Review updated until commit 8055dfb)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Encapsulation Concern

The private property _agfs is accessed from outside the class (in app.py and tests). Consider making this a public property (e.g., agfs_client) to follow encapsulation best practices.

@property
def _agfs(self) -> Any:
    """Internal access to AGFS client for APIKeyManager."""
    return self._agfs_client
Type Safety Suggestion

The _agfs property returns Any, but APIKeyManager expects an AGFSClient instance. Consider tightening the type annotation to AGFSClient (if guaranteed non-None) or handling the optional case explicitly.

def _agfs(self) -> Any:
    """Internal access to AGFS client for APIKeyManager."""
    return self._agfs_client

@github-actions
Copy link

github-actions bot commented Mar 5, 2026

PR Code Suggestions ✨

Latest suggestions up to 8055dfb
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Preserve AGFS URL config for all modes

For non "http-client" modes, ensure config.agfs.url is explicitly preserved/set in
case any downstream code still relies on it. This maintains backward compatibility
and robustness.

openviking/service/core.py [113-118]

 mode = getattr(config.agfs, "mode", "http-client")
 if mode == "http-client":
     self._agfs_manager = AGFSManager(config=config.agfs)
     self._agfs_manager.start()
     agfs_url = self._agfs_manager.url
     config.agfs.url = agfs_url
+else:
+    # Ensure config.agfs.url is available for downstream use
+    pass  # Assume config.agfs.url is already set by user
Suggestion importance[1-10]: 1

__

Why: The suggestion adds a comment and empty else block, which has negligible functional impact. The config.agfs.url is likely already available for non "http-client" modes as it's used to create the AGFS client, making this change unnecessary for robustness.

Low

Previous suggestions

Suggestions up to commit 8055dfb
CategorySuggestion                                                                                                                                    Impact
General
Add initialization check for AGFS client property

Add a check to ensure _agfs_client is initialized before returning it, or document
that this property is only available after storage initialization. This prevents
accidental use before the service is fully initialized.

openviking/service/core.py [146-149]

 @property
 def _agfs(self) -> Any:
-    """Internal access to AGFS client for APIKeyManager."""
+    """Internal access to AGFS client for APIKeyManager.
+
+    Only available after _init_storage() has been called.
+    """
+    if self._agfs_client is None:
+        raise RuntimeError("AGFS client not initialized - call initialize() first")
     return self._agfs_client
Suggestion importance[1-10]: 5

__

Why: Adding an initialization check and updating the docstring for the _agfs property helps prevent accidental use before storage is initialized, improving robustness and clarity.

Low

@github-actions
Copy link

github-actions bot commented Mar 5, 2026

Persistent review updated to latest commit 8055dfb

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.

3 participants