Skip to content

fix: support short-format URIs in VikingURI and VikingFS access control#402

Merged
MaojiaSheng merged 2 commits intovolcengine:mainfrom
r266-tech:fix/short-uri-normalization
Mar 4, 2026
Merged

fix: support short-format URIs in VikingURI and VikingFS access control#402
MaojiaSheng merged 2 commits intovolcengine:mainfrom
r266-tech:fix/short-uri-normalization

Conversation

@r266-tech
Copy link
Contributor

Description

Fix VikingURI and VikingFS._is_accessible() to accept short-format URI paths (e.g., /resources, user/memories) in addition to full-format URIs (viking://resources).

Related Issue

Fixes #259

Root Cause

Two issues prevented short-format URIs from working:

  1. VikingURI.__init__ required URIs to start with viking://, raising ValueError for short paths like /resources
  2. VikingFS._is_accessible() returned False (access denied) for any URI not starting with viking://, which triggered PermissionError before _uri_to_path could normalize the path

Changes Made

File Change
openviking_cli/utils/uri.py VikingURI.__init__ now auto-normalizes via the existing normalize() static method
openviking/storage/viking_fs.py _is_accessible() normalizes non-viking:// URIs instead of rejecting them
tests/unit/test_uri_short_format.py 19 tests covering short-format normalization, scope validation, join/parent operations

Design Decisions

  • Leverages existing normalize() method: The VikingURI.normalize() static method already handled the conversion correctly — the fix simply wires it into the constructor
  • Backward compatible: Full-format URIs pass through normalize() unchanged (idempotent)
  • Invalid scopes still rejected: VikingURI("/invalid_scope/foo") still raises ValueError — only the format requirement is relaxed

Testing

# Before: ValueError / PermissionError
VikingURI("/resources")           # ValueError
client.ls("/resources")           # PermissionError: Access denied

# After: works correctly
VikingURI("/resources").scope     # "resources"
VikingURI("/resources").uri       # "viking://resources"

All 19 new tests pass. No existing tests affected.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Checklist

  • My code follows the project coding style
  • I have performed a self-review of my code
  • New and existing unit tests pass locally with my changes
  • My changes generate no new warnings

VikingURI now auto-normalizes short-format URIs ('/resources', 'user/memories')
to full format ('viking://resources', 'viking://user/memories') via the existing
normalize() static method. This eliminates ValueError when constructing VikingURI
with short paths.

VikingFS._is_accessible() now normalizes URIs before checking, preventing
false PermissionError('Access denied') for short-format paths.

Changes:
- openviking_cli/utils/uri.py: VikingURI.__init__ calls self.normalize(uri)
- openviking/storage/viking_fs.py: _is_accessible normalizes non-viking:// URIs
- tests/unit/test_uri_short_format.py: 19 tests covering normalization

Fixes #259
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@MaojiaSheng MaojiaSheng merged commit f694540 into volcengine:main Mar 4, 2026
5 checks passed
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Bug: URI parsing fails for short format paths (e.g., '/resources' instead of 'viking://resources')

3 participants