Skip to content

feat: add account namespace policy phase 1#1449

Closed
yeyitech wants to merge 2 commits intovolcengine:mainfrom
yeyitech:feat/account-namespace-phase1
Closed

feat: add account namespace policy phase 1#1449
yeyitech wants to merge 2 commits intovolcengine:mainfrom
yeyitech:feat/account-namespace-phase1

Conversation

@yeyitech
Copy link
Copy Markdown
Contributor

Summary

  • implement #1351 Phase 1 account namespace policy persistence under /local/{account_id}/_system/setting.json
  • add a namespace resolver to canonicalize user/agent/session URIs and move sessions to account-level viking://session/{session_id}
  • enforce shared-session visibility within an account and restrict session deletion to ADMIN / ROOT

What Changed

  • add NamespacePolicy + NamespaceResolver and cache policy loading inside VikingFS
  • add canonical user/agent root helpers on UserIdentifier
  • persist namespace policy during account initialization and accept the policy fields in the admin create-account API
  • switch session create/list/delete/storage paths to account-level session roots
  • add regression coverage for namespace canonicalization, account policy persistence, shared-session visibility, and delete permissions

Phase 1 Scope

Included in this PR:

  • account-level namespace policy persistence
  • canonical URI/root resolution for user/agent/session
  • account-level shared session root
  • delete-session permission tightening

Explicitly left for follow-up:

  • retrieval/vector ownership redesign
  • message role_id identity model
  • participant metadata / ACL
  • legacy agent-data migration
  • online policy mutation

Validation

  • ruff check openviking/core/namespace.py openviking/storage/viking_fs.py openviking/core/directories.py openviking/service/core.py openviking/server/routers/admin.py openviking/service/session_service.py openviking/session/session.py openviking/message/part.py openviking_cli/session/user_id.py tests/unit/test_namespace_resolver.py tests/server/test_admin_api.py tests/server/test_auth.py tests/session/test_session_lifecycle.py tests/test_session_task_tracking.py tests/unit/test_summarizer_context_type.py
  • python -m compileall openviking/core/namespace.py openviking/core/directories.py openviking/storage/viking_fs.py openviking/service/core.py openviking/server/routers/admin.py openviking/service/session_service.py openviking/session/session.py openviking/message/part.py openviking_cli/session/user_id.py tests/unit/test_namespace_resolver.py tests/server/test_admin_api.py tests/server/test_auth.py tests/session/test_session_lifecycle.py tests/test_session_task_tracking.py tests/unit/test_summarizer_context_type.py
  • manual end-to-end validation script covering:
    • admin account creation with namespace policy persistence
    • shared session visibility across users in one account
    • USER delete denied / ADMIN delete allowed

Notes

pytest collection is currently blocked by the repo's existing pytest-asyncio package-collection error (AttributeError: 'Package' object has no attribute 'obj'), so I used compile/lint plus a direct integration harness for this PR.

@github-actions
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1351 - Partially compliant

Compliant requirements:

  • account-level namespace policy persistence
  • canonical URI/root resolution for user/agent/session
  • account-level shared session root
  • delete-session permission tightening

Non-compliant requirements:

  • (no non-compliant requirements in Phase 1 scope)

Requires further human verification:

  • (no items needing further human verification)
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🏅 Score: 85
🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Add namespace policy and resolver

Relevant files:

  • openviking/core/namespace.py
  • tests/unit/test_namespace_resolver.py

Sub-PR theme: Move sessions to account-level root

Relevant files:

  • openviking/service/session_service.py
  • openviking/session/session.py
  • openviking/message/part.py
  • tests/session/test_session_lifecycle.py
  • tests/test_session_task_tracking.py
  • tests/server/test_auth.py

Sub-PR theme: Accept namespace policy in admin create-account API

Relevant files:

  • openviking/server/routers/admin.py
  • tests/server/test_admin_api.py

⚡ Recommended focus areas for review

Missing License Header

New Python file missing required AGPL-3.0 license header at the top.

"""Account-scoped namespace policy and canonical URI resolution helpers."""
Overly Broad Exception Handling

Broad except Exception: clauses without logging swallow errors and hide real issues.

def _ensure_parent_dirs(viking_fs: Any, path: str) -> None:
    parts = [part for part in path.lstrip("/").split("/") if part]
    for index in range(1, len(parts)):
        parent = "/" + "/".join(parts[:index])
        try:
            viking_fs.agfs.mkdir(parent)
        except Exception:
            pass


def _coerce_bytes(viking_fs: Any, result: Any) -> bytes:
    if hasattr(viking_fs, "_handle_agfs_read"):
        return viking_fs._handle_agfs_read(result)
    if isinstance(result, bytes):
        return result
    if result is None:
        return b""
    if hasattr(result, "content") and result.content is not None:
        return result.content
    return str(result).encode("utf-8")


async def load_namespace_policy(viking_fs: Any, account_id: str) -> NamespacePolicy:
    """Read the persisted account policy, falling back to defaults."""
    path = namespace_policy_path(account_id)
    try:
        try:
            raw = viking_fs.agfs.read(path, 0, -1)
        except TypeError:
            raw = viking_fs.agfs.read(path)
        payload = _coerce_bytes(viking_fs, raw)
        payload = await viking_fs.decrypt_bytes(account_id, payload)
        data = json.loads(payload.decode("utf-8"))
        return NamespacePolicy.from_dict(data)
    except Exception:
        return NamespacePolicy()
Blocking Call in Async Context

_get_namespace_resolver uses run_async to load policy; this may block the event loop when called from async functions like initialize_user_directories.

def _get_namespace_resolver(self, ctx: Optional[RequestContext]) -> NamespaceResolver:
    real_ctx = self._ctx_or_default(ctx)
    cache = getattr(self, "_namespace_policy_cache", None)
    if cache is None:
        cache = {}
        self._namespace_policy_cache = cache

    policy = cache.get(real_ctx.account_id)
    if policy is None:
        policy = run_async(load_namespace_policy(self, real_ctx.account_id))
        cache[real_ctx.account_id] = policy
    return NamespaceResolver(policy)
Unsynced Cache Access

_namespace_policy_cache is accessed without synchronization, which could lead to race conditions in concurrent requests.

    self._namespace_policy_cache: Dict[str, Any] = {}

@staticmethod
def _default_ctx() -> RequestContext:
    return RequestContext(user=UserIdentifier.the_default_user(), role=Role.ROOT)

def _ctx_or_default(self, ctx: Optional[RequestContext]) -> RequestContext:
    if ctx is not None:
        return ctx
    bound = self._bound_ctx.get()
    return bound or self._default_ctx()

async def _encrypt_content(self, content: bytes, ctx: Optional[RequestContext] = None) -> bytes:
    """Encrypt content if encryption is enabled."""
    if not self._encryptor:
        return content
    real_ctx = self._ctx_or_default(ctx)
    return await self._encryptor.encrypt(real_ctx.account_id, content)

async def _decrypt_content(self, content: bytes, ctx: Optional[RequestContext] = None) -> bytes:
    """Decrypt content if encryption is enabled."""
    if not self._encryptor:
        return content
    real_ctx = self._ctx_or_default(ctx)
    return await self._encryptor.decrypt(real_ctx.account_id, content)

async def encrypt_bytes(self, account_id: str, data: bytes) -> bytes:
    """
    Encrypt bytes using the encryptor for the specified account.

    Args:
        account_id: Account ID to use for encryption
        data: Bytes to encrypt

    Returns:
        Encrypted bytes, or original bytes if encryption is disabled
    """
    if not self._encryptor:
        return data
    return await self._encryptor.encrypt(account_id, data)

async def decrypt_bytes(self, account_id: str, data: bytes) -> bytes:
    """
    Decrypt bytes using the encryptor for the specified account.

    Args:
        account_id: Account ID to use for decryption
        data: Bytes to decrypt

    Returns:
        Decrypted bytes, or original bytes if encryption is disabled
    """
    if not self._encryptor:
        return data
    return await self._encryptor.decrypt(account_id, data)

@contextmanager
def bind_request_context(self, ctx: RequestContext):
    """Temporarily bind ctx for legacy internal call paths without explicit ctx param."""
    token = self._bound_ctx.set(ctx)
    try:
        yield
    finally:
        self._bound_ctx.reset(token)

@staticmethod
def _normalize_uri(uri: str) -> str:
    """Normalize short-format URIs to the canonical viking:// form."""
    if uri.startswith("viking://"):
        return uri
    return VikingURI.normalize(uri)

def _get_namespace_resolver(self, ctx: Optional[RequestContext]) -> NamespaceResolver:
    real_ctx = self._ctx_or_default(ctx)
    cache = getattr(self, "_namespace_policy_cache", None)
    if cache is None:
        cache = {}
        self._namespace_policy_cache = cache

    policy = cache.get(real_ctx.account_id)
    if policy is None:
        policy = run_async(load_namespace_policy(self, real_ctx.account_id))
        cache[real_ctx.account_id] = policy
    return NamespaceResolver(policy)

@github-actions
Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Make namespace resolver methods async

The _get_namespace_resolver method uses run_async() to call the async
load_namespace_policy function, which blocks the event loop when invoked from async
contexts (like initialize_user_directories). Convert _get_namespace_resolver and
get_namespace_resolver to async methods, and update callers to await them.

openviking/storage/viking_fs.py [257-272]

-def _get_namespace_resolver(self, ctx: Optional[RequestContext]) -> NamespaceResolver:
+async def _get_namespace_resolver(self, ctx: Optional[RequestContext]) -> NamespaceResolver:
     real_ctx = self._ctx_or_default(ctx)
     cache = getattr(self, "_namespace_policy_cache", None)
     if cache is None:
         cache = {}
         self._namespace_policy_cache = cache
 
     policy = cache.get(real_ctx.account_id)
     if policy is None:
-        policy = run_async(load_namespace_policy(self, real_ctx.account_id))
+        policy = await load_namespace_policy(self, real_ctx.account_id)
         cache[real_ctx.account_id] = policy
     return NamespaceResolver(policy)
 
-def get_namespace_resolver(self, ctx: Optional[RequestContext]) -> NamespaceResolver:
+async def get_namespace_resolver(self, ctx: Optional[RequestContext]) -> NamespaceResolver:
     """Expose the per-account namespace resolver for callers that need canonical roots."""
-    return self._get_namespace_resolver(ctx)
+    return await self._get_namespace_resolver(ctx)
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential event loop blocking issue with run_async(), but the provided improved_code does not include necessary updates to synchronous callers (like _canonicalize_uri and _is_accessible), which would break existing functionality.

Low
Make policy loading async

Convert _load_namespace_policy to an async method and update its callers to await
it, since get_namespace_resolver is now async. This eliminates blocking calls in
async functions like initialize_user_directories and initialize_agent_directories.

openviking/core/directories.py [364-368]

 @staticmethod
-def _load_namespace_policy(ctx: RequestContext) -> NamespacePolicy:
+async def _load_namespace_policy(ctx: RequestContext) -> NamespacePolicy:
     from openviking.storage.viking_fs import get_viking_fs
 
-    return get_viking_fs().get_namespace_resolver(ctx).policy
+    resolver = await get_viking_fs().get_namespace_resolver(ctx)
+    return resolver.policy
Suggestion importance[1-10]: 5

__

Why: The suggestion depends on making get_namespace_resolver async (from suggestion 1), which is not addressed independently. Additionally, it does not account for synchronous callers of get_namespace_resolver elsewhere, limiting its standalone validity.

Low

@yeyitech
Copy link
Copy Markdown
Contributor Author

Updated the PR to address the bot suggestions: added the missing license header, tightened namespace-policy fallback logging, and reworked namespace policy loading/cache warmup to avoid blocking async paths.

@yeyitech yeyitech force-pushed the feat/account-namespace-phase1 branch from 02fec02 to fb6577b Compare April 15, 2026 02:36
@yeyitech yeyitech force-pushed the feat/account-namespace-phase1 branch from fb6577b to 55d21e1 Compare April 15, 2026 02:42
@qin-ctx
Copy link
Copy Markdown
Collaborator

qin-ctx commented Apr 15, 2026

Thanks for the contribution here. This issue already has an implementation in progress in #1356, so I do not recommend continuing with another parallel implementation to avoid duplicated work and review overhead. If you would like to help on this topic, reviewing or testing #1356 would be more useful.

@qin-ctx qin-ctx closed this Apr 15, 2026
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Apr 15, 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.

2 participants