fix(feishu): preserve document titles in resource names and rename on… - #3031
fix(feishu): preserve document titles in resource names and rename on…#3031dfwgj wants to merge 1 commit into
Conversation
qin-ctx
left a comment
qin-ctx
left a comment
There was a problem hiding this comment.
Requesting changes for the Feishu naming fix. The core direction is right, but the current implementation still misses the default Feishu import path and changes the existing explicit to contract. I also left a few non-blocking Ponytail notes where the diff can be shortened without losing behavior.
| parse_kwargs["resource_name"] = repo_name.split("/")[-1] | ||
| else: | ||
| # Prefer original_filename from meta for HTTP downloads | ||
| original_filename = local_resource.meta.get("original_filename") |
There was a problem hiding this comment.
[Bug] (blocking)
UnifiedResourceProcessor.process() still only reads local_resource.meta["original_filename"] when the caller did not pass source_name. For Feishu resources, the accessor stores the real document title in local_resource.meta["feishu_title"], not in original_filename.
Why this is a problem:
The PR is meant to fix Feishu title-based naming, but the default API/Web Studio path does not pass source_name; it relies on the accessor metadata. Since this branch ignores feishu_title, the main import path can still miss the actual title.
Concrete example:
- The user sends
POST /api/v1/resourceswithpath="https://example.feishu.cn/docx/abc"and nosource_name. - The Feishu document title is
API Docs/Overview. FeishuAccessor.access()returns a temp file likeov_feishu_abcd.mdwithmeta["feishu_title"] = "API Docs/Overview".
Execution process:
source_name = kwargs.get("source_name")isNone.original_filename = local_resource.meta.get("original_filename")is alsoNonefor Feishu.- The code falls through to
_smart_stem(local_resource.path), so the resource name is derived from the temp filename instead ofAPI_Docs_Overview.
Impact:
The default reproduction from #3025 can still create a resource named after the temp file rather than the Feishu title, so the PR does not reliably fix the reported bug.
There was a problem hiding this comment.
Fixed in 68a04216. UnifiedResourceProcessor.process() now has an explicit SourceType.FEISHU branch that reads the title from meta["feishu_title"] (falling back to source_name when the caller does pass one), so the default API / Web Studio path no longer falls through to the temp-file stem. The derived segment is marked resource_name_is_safe=True so the parser doesn't re-sanitize it with a different rule set.
# media_processor.py — UnifiedResourceProcessor.process()
source_name = kwargs.get("source_name")
if local_resource.source_type == SourceType.FEISHU:
# Feishu titles are semantic display names, not filesystem paths:
# derive the segment from the title (source_name when provided,
# otherwise meta["feishu_title"] recorded by FeishuAccessor) so the
# default import path does not fall back to the temp-file stem.
feishu_title = source_name or local_resource.meta.get("feishu_title")
if feishu_title:
parse_kwargs["resource_name"] = feishu_title_to_resource_segment(feishu_title)
parse_kwargs["resource_name_is_safe"] = True
parse_kwargs.setdefault("source_name", feishu_title.strip())
else:
parse_kwargs.setdefault("resource_name", _smart_stem(local_resource.path))
elif source_name:
...Verified against the #3025 repro: POST /api/v1/resources with path=<feishu docx> and no source_name, title API Docs/Overview → resource now lands at API_Docs_Overview instead of the temp name.
| lock_manager = get_lock_manager() | ||
| try: | ||
| temp_doc_name = VikingURI(temp_uri).uri.rstrip("/").rsplit("/", 1)[-1] | ||
| if to and temp_doc_name: |
There was a problem hiding this comment.
[Bug] (blocking)
if to and temp_doc_name: runs the Feishu title-rename path for every explicit to import, not only for watch resync of an existing Feishu resource.
Why this is a problem:
The existing to contract is an exact target URI. A user-provided to="viking://resources/team/handbook" should not be replaced by a URI derived from the Feishu title unless this is specifically a resync of the watched old root.
Concrete example:
- The user imports a Feishu doc titled
API Docs/Overview. - They explicitly pass
to="viking://resources/team/handbook". - That target URI does not exist yet.
Execution process:
finalize_from_temp()first returnsroot_uri="viking://resources/team/handbook", preserving the explicit target.- This new branch calls
_resolve_feishu_resync_root_uri()becausetois set. _resolve_feishu_resync_root_uri()derivesviking://resources/team/API_Docs_Overview.- Because the original
handbookroot does not exist, line 326/327 updatesroot_urito the title-derived URI.
Impact:
A normal explicit-target import lands at a different URI than the caller requested. That is a behavioral regression for existing API/CLI callers that rely on to as an exact destination.
There was a problem hiding this comment.
Fixed in 68a04216 by removing the resync auto-rename path entirely. The is_resync branch and _resolve_feishu_resync_root_uri() are gone, so an explicit to is once again an exact destination — finalize_from_temp() returns the requested root_uri and nothing rewrites it from the Feishu title.
The finalize block is now back to the pre-existing shape:
try:
if candidate_uri:
if resource_lock.active:
root_uri = candidate_uri
else:
root_uri, resource_lock = await self.reserve_unique_candidate(...)
...
else:
target_preexisting = await viking_fs.exists(root_uri, ctx=ctx)
...The rationale: without a stored title baseline the resync path could not tell a real title change from a user-chosen name, and (as you noted) it silently moved explicit targets. A correct "rename on real title change" belongs on top of #3029 with a manifest title baseline that respects user-specified names; I've noted that in the commit message as follow-up.
| _FEISHU_HOSTS = ("feishu.cn", "larksuite.com", "larkoffice.com") | ||
|
|
||
|
|
||
| @dataclass(frozen=True) |
There was a problem hiding this comment.
[Design] (non-blocking)
FeishuResourceNames currently wraps one real value: folder_segment and markdown_stem are always identical, and markdown_filename is only exercised by tests.
Concrete example:
Every production caller ultimately reads .folder_segment; no caller needs a separate markdown stem or filename object today.
Impact:
This adds a dataclass, property, wrapper helper, and tests for future flexibility that the PR does not need. A single feishu_title_to_resource_segment(title) string keeps the fix smaller and makes the call sites harder to misuse.
There was a problem hiding this comment.
Done in 68a04216. Collapsed the dataclass, its property, and the wrapper into a single feishu_title_to_resource_segment(title) -> str. The folder segment and the primary .md stem share this one string, and the module is now just two functions:
def feishu_title_to_resource_segment(title: str) -> str:
from openviking_cli.utils.uri import VikingURI
text = (title or "").strip()
if not text:
return "unnamed"
text = text.replace("\\", "_").replace("/", "_")
return VikingURI.sanitize_segment(text)The bespoke long-title hash-suffix logic was also dropped — length capping now defers to VikingURI.sanitize_segment (same behavior as the rest of the codebase), keeping the helper minimal.
| return path.name | ||
|
|
||
|
|
||
| def _resource_name_from_original_filename(original_filename: str, source_type: str) -> str: |
There was a problem hiding this comment.
[Design] (non-blocking)
_resource_name_from_original_filename() has one production caller and only branches on SourceType.FEISHU.
Concrete example:
The only call site is the original_filename branch below, where the same behavior can be expressed inline as feishu_title_to_resource_segment(original_filename) for Feishu and _smart_stem(original_filename) otherwise.
Impact:
This private wrapper adds another named surface and separate tests without real reuse. Inlining it would remove code while keeping the same behavior.
There was a problem hiding this comment.
Done in 68a04216. The private wrapper is removed. Its only caller was the Feishu branch, and that behavior is now expressed inline: feishu_title_to_resource_segment(...) for Feishu, _smart_stem(...) otherwise. No extra named surface, no separate tests for a one-line indirection.
| if planned_uri.rstrip("/") == current_root_uri.rstrip("/"): | ||
| return current_root_uri, None | ||
|
|
||
| if candidate_uri: |
There was a problem hiding this comment.
[Design] (non-blocking)
The _1 through _100 loop introduces a new conflict-resolution policy inside Feishu resync.
Concrete example:
If a watched document changes title to API Docs/Overview and viking://resources/team/API_Docs_Overview already exists, this code silently searches for API_Docs_Overview_1, API_Docs_Overview_2, and so on.
Impact:
The issue only asks to rename the watched root when the title changes; it does not require creating a new suffixed resource on conflict. This also duplicates existing unique-name behavior elsewhere. The smaller behavior is to keep the current root and warn when the title-derived target already exists.
There was a problem hiding this comment.
Resolved in 68a04216. The _1…_100 loop lived inside _resolve_feishu_resync_root_uri(), which was removed together with the whole resync path (see the reply on the explicit-to comment). No new conflict-resolution policy is introduced anymore, and we no longer duplicate the existing unique-name logic.
| assert name == "report" | ||
|
|
||
|
|
||
| def test_wiki_title_overrides_page_title_when_present(): |
There was a problem hiding this comment.
[Suggestion] (non-blocking)
These wiki title tests only validate local variable assignment in the test body; they do not call FeishuAccessor, _fetch_document(), or any production function.
Concrete example:
The test sets doc_title = page_title, then runs if wiki_title: doc_title = wiki_title, then asserts the assigned local value.
Impact:
This increases the test suite size without protecting the Feishu import path from regressions. It can be deleted, or replaced with a production-path test that proves wiki title metadata flows into resource naming.
ae25b3b to
b0b321f
Compare
Normalize Feishu/Lark document titles into OpenViking-safe path segments so a title with `/` or other path-sensitive characters is not truncated to its last path component (fixes volcengine#3025). The resource folder and its primary Markdown file share one segment. Details: - Add feishu_title_to_resource_segment(): replace `/` and `\` with `_` *before* VikingURI.sanitize_segment, so "API Docs/Overview" becomes "API_Docs_Overview" instead of losing the "API Docs" prefix. The segment otherwise follows the same safe-path style and length cap as the rest of the codebase. - Default import path derives the resource name from meta["feishu_title"] (set by FeishuAccessor) when no source_name is supplied, instead of falling back to the temp-file stem, so the volcengine#3025 case no longer lands under the temp upload name on the main import path. - MarkdownParser accepts resource_name_is_safe so an already-sanitized Feishu segment is not re-sanitized with the parser's different rule set. The resource name is fixed at import time; a Feishu title change does not move the local resource. An earlier revision of this PR auto-renamed the resource to the latest Feishu title on every re-sync, but with no stored title baseline it could not distinguish a real title change from a user-chosen name and would overwrite an explicit name (e.g. adding a watch renamed "测试文件" to the raw Feishu title). That behavior is removed here; a correct "rename on real title change" should be built on top of volcengine#3029 with a manifest title baseline that respects user-specified names.
b0b321f to
68a0421
Compare
|
@qin-ctx Thanks for the thorough review — all seven points are addressed in Blocking
Non-blocking (Ponytail)
The PR is now scoped to exactly one behavior: Feishu documents are named by their real title, and a title containing |
2 participants
Description
Fixes Feishu/Lark resource naming when document titles contain
/or other path-sensitive characters. The import path now treats Feishu titles as display names, normalizes them into OpenViking-safe path segments, and keeps the generated resource folder and primary Markdown file stem consistent. Watch resync also updates the watched resource URI when the Feishu title changes.Related Issue
Fixes #3025
Type of Change
Changes Made
Path.nametruncation and follows OpenViking-safe path segment naming..mdstem are derived consistently.Testing
Test coverage added/updated:
tests/parse/test_feishu_naming.pytests/parse/test_feishu_resource_name.pytests/parse/test_feishu_resync_rename.pytests/parse/test_feishu_title_fixes.pyManual validation:
/.Checklist
Screenshots (if applicable)
N/A
Additional Notes
This PR intentionally keeps the final resource segment in OpenViking's safe naming style instead of preserving Feishu title characters verbatim. For example,
API Docs/OverviewbecomesAPI_Docs_Overview, avoiding filesystem/path ambiguity while preserving the full title prefix.