You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR removes the sanitizeFileNameForUpload utility from extensions/feishu/src/media.ts and passes file_name directly to the Feishu SDK, fixing the user-visible issue where recipients saw percent-encoded filenames (e.g. %E6%B5%8B%E8%AF%95.pdf) instead of the original Chinese/special-character names. The companion test file is updated to remove the deleted function's test suite and flip the filename assertions from "encodes" to "preserves".
Root cause addressed: The previous encoding was surfaced verbatim to message recipients rather than being decoded by Feishu's server, making file names unreadable.
Tradeoff to be aware of: The sanitizeFileNameForUpload JSDoc stated that raw non-ASCII characters could cause silent upload failures in the SDK's multipart form-data serialization (referencing larksuite/node-sdk#121). This workaround has been removed without confirming whether the underlying SDK issue is resolved — see inline comment on media.ts.
Test coverage: All assertions are against a mocked SDK client, so they confirm the value passed to the SDK but cannot catch real-world multipart serialization failures. A live-tenant smoke test with a Chinese filename is recommended before relying on this in production.
Confidence Score: 4/5
Safe to merge with low risk, but a live Feishu tenant test with non-ASCII filenames is recommended before wide rollout.
The change is small (two files, one deleted function, updated assertions) and clearly scoped. The new behavior directly fixes a confirmed user-visible bug. The only open risk is whether the SDK-level multipart issue that originally motivated the encoding workaround still exists; this was not verified against a real Feishu API and the tests are fully mocked, so a silent-failure regression cannot be ruled out from the test suite alone.
Pay close attention to extensions/feishu/src/media.ts — specifically the uploadFileFeishu call to client.im.file.create with a raw non-ASCII file_name. Verify the Feishu SDK version in use correctly handles UTF-8 filenames in multipart form-data before merging.
Comments Outside Diff (1)
extensions/feishu/src/media.ts, line 249-256 (link)
SDK multipart form-data compatibility risk
The removed sanitizeFileNameForUpload function's JSDoc explicitly noted that raw non-ASCII characters in file_name"cause the upload to silently fail when passed raw through the SDK's form-data serialization" (referencing larksuite/node-sdk#121). That workaround was added to address a known SDK-level limitation.
This change trades one problem (recipients seeing encoded strings like %E6%B5%8B%E8%AF%95.pdf) for a potential regression (silent upload failures for non-ASCII filenames). Both problems affect non-ASCII filenames, but the silent failure mode is harder to detect.
Since the tests mock the SDK client entirely, they don't exercise the real multipart serialization path and cannot catch this class of failure. The PR description also acknowledges that live Feishu API testing was not done.
Before merging, it would be worth confirming either:
A real-tenant smoke test with a Chinese-character filename succeeds end-to-end.
If the silent-failure risk is confirmed to be gone (e.g. SDK was updated), a brief comment in the code noting why encoding is no longer needed would help future readers understand why the workaround was removed.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/feishu/src/media.ts
Line: 249-256
Comment:
**SDK multipart form-data compatibility risk**
The removed `sanitizeFileNameForUpload` function's JSDoc explicitly noted that raw non-ASCII characters in `file_name`*"cause the upload to silently fail when passed raw through the SDK's form-data serialization"* (referencing [larksuite/node-sdk#121](https://github.com/larksuite/node-sdk/issues/121)). That workaround was added to address a known SDK-level limitation.
This change trades one problem (recipients seeing encoded strings like `%E6%B5%8B%E8%AF%95.pdf`) for a potential regression (silent upload failures for non-ASCII filenames). Both problems affect non-ASCII filenames, but the silent failure mode is harder to detect.
Since the tests mock the SDK client entirely, they don't exercise the real multipart serialization path and cannot catch this class of failure. The PR description also acknowledges that live Feishu API testing was not done.
Before merging, it would be worth confirming either:
1. The referenced SDK issue (#121) has since been resolved upstream (the SDK now handles UTF-8 `file_name` correctly in form-data), or
2. A real-tenant smoke test with a Chinese-character filename succeeds end-to-end.
If the silent-failure risk is confirmed to be gone (e.g. SDK was updated), a brief comment in the code noting *why* encoding is no longer needed would help future readers understand why the workaround was removed.
How can I resolve this? If you propose a fix, please make it concise.
Addressed the Greptile risk note with a minimal clarification in code:
Added an inline comment in extensions/feishu/src/media.ts at uploadFileFeishu() explaining why file_name is intentionally passed as raw UTF-8 (to preserve recipient-visible filenames).
Not changed intentionally: I did not reintroduce percent-encoding or add fallback retry logic, because that would bring back the user-facing %XX filename regression this PR fixes.
Rationale: the upstream SDK issue referenced in the old workaround (larksuite/node-sdk#121) is now closed as fixed, and this repo is on @larksuiteoapi/node-sdk@1.59.0.
Validation run:
pnpm test extensions/feishu/src/media.test.ts (pass, 18/18)
I still agree with the recommendation that a real-tenant smoke test is useful before broad rollout; that is operational validation rather than a unit-test gap in this PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
file_namevalues before upload.%E7%A4%BA%E4%BE%8B.pdfinstead of human-readable names.file_namevalues.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
测试文档.pdf).im.file.createdata.file_name.Expected
Actual
data.file_nameequals original non-ASCII filename.Evidence
Human Verification (required)
pnpm test extensions/feishu/src/media.test.ts1 passed,18 passedtests.Review Conversations
Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
extensions/feishu/src/media.ts,extensions/feishu/src/media.test.ts.Risks and Mitigations