Conversation
chenhg5
left a comment
There was a problem hiding this comment.
CI Gate 阻塞 — lint 失败
CI 在 lint check 上 fail,未进入完整 diff review。按项目 CI gate 流程,先打回开发修 lint,绿了再做完整评审。
失败 check
- lint: FAILURE (2m10s) — https://github.com/chenhg5/cc-connect/actions/runs/34935683578/job/104272942060
- unit-test / smoke-test / regression-test / performance-test: 全部 SKIPPED(因为 lint fail 中断 pipeline,这些没跑)
4 个 staticcheck issue,都在新增的测试文件
platform/wps-xiezuo/wpsxiezuo_test.go:692:3: QF1002: could use tagged switch on r.URL.Path (staticcheck)
platform/wps-xiezuo/wpsxiezuo_test.go:754:3: QF1002: could use tagged switch on r.URL.Path (staticcheck)
platform/wps-xiezuo/wpsxiezuo_test.go:877:2: S1021: should merge variable declaration with assignment on next line (staticcheck)
platform/wps-xiezuo/wpsxiezuo_test.go:939:3: QF1002: could use tagged switch on r.URL.Path (staticcheck)
修复建议
- QF1002 (3 处):把
switch { case r.URL.Path == "/x": ... }改为switch r.URL.Path { case "/x": ... }(tagged switch)— 测试 fixture 里都是 path 字符串比较,改成 tagged form 可读性更高,也是 staticcheck 推荐写法 - S1021 (1 处):
var srv *httptest.Server+ 下一行srv = httptest.NewServer(...)合并为srv := httptest.NewServer(...)
影响与下一步
- 不阻塞 merge flow,但阻塞 review flow:CI 必须 5/5 green 后再做代码评审
- 改动小,作者可以快速 rebase / amend 后重推,无需新 PR
- 等 CI 5/5 green 后,我会进入完整 diff review(注意 PR 1101 行 / 4 文件,需要更多 token)
QA 决策报告: 待 CI 绿了再创建 doc-XXXXX-PR-1850-QA-...(避免在 lint fail 时记录"已批准",污染后续 re-review)
— qa-claudecode
chenhg5
left a comment
There was a problem hiding this comment.
Conclusion: Approve
This is a thoughtful, well-tested feature addition for the wps-xiezuo platform. The diff demonstrates strong security awareness (size limits at multiple layers, path traversal sanitization, MIME detection with sensible fallback, explicit auth-signing test helper) and the documentation/config updates correctly guide users through the new WPS Open Platform permissions.
✅ What looks good
- Defense in depth on file size: Three layers — declared size check,
Content-Lengthpre-check,io.LimitReader+ post-read validation with+1byte probe.TestDownloadMessageResource_RejectsOversizeBodydirectly verifies the third layer with a 4-byte cap against a 5-byte body. - Path traversal protection:
sanitizeAttachmentNameusesfilepath.Base, andTestHandleChatMessage_LocalFileAttachmentproves it works (folder/report.txt→report.txt). - Authentication test coverage: The new
assertWPSResourceAuthhelper verifies bothAuthorization: Bearer <token>andX-Kso-Authorization: KSO-1 <appid>:<hmac-sha256>on every signed API call. This catches signing regressions across all four download paths. - Clean refactor:
signWSHeader→signKSO1Header(method, requestURI, contentType, body)is the right extraction. It already acceptsbody []bytefor SHA256 hashing, so the code is ready for future POST/PUT endpoints without further refactor. Alsohttp.DefaultClient→p.client()for connection reuse + timeout config is a correct hygiene fix across 4 call sites. - Graceful degradation: Cloud document with no token / no
link_idfalls back to forwarding just thelink_urltext (no crash, no content).TestHandleChatMessage_CloudDocumentForwardsLinkcovers this exact path. - Explicit auth marker for cloud docs: The
[WPS云文档正文(已由应用授权读取,请优先基于以下正文回答,不要通过网页链接再次访问)]marker is the right pattern — it tells the agent that the content is application-authorized and discourages re-fetching via the login-protected web link (which would either fail or hit a different access path).
🔵 Optional nit (P3, non-blocking)
- SSRF hardening:
validateResourceDownloadURLrequireshttp(s)scheme + non-empty host + nouserinfo, but does not block private IP ranges (127.0.0.1,169.254.169.254,10.0.0.0/8). Since the URL comes from the WPS API (which the caller has already authenticated to withapp_id+app_secret+ bearer token), the practical attack surface is limited — but adding an IP/host denylist would be a worthwhile follow-up hardening for the entireplatform/wps-xiezuo/package. Not a blocker for this PR. - Hardcoded 50 MiB cap:
defaultMaxAttachmentBytes = 50 * 1024 * 1024is hardcoded. WPS allows up to 2 GiB. If users later want to handle larger files (datasets, archives), consider exposingmax_attachment_bytesinconfig.example.tomlfor this platform. Not needed for this PR.
❓ Question
- The new doc string
[WPS云文档正文(已由应用授权读取,请优先基于以下正文回答,不要通过网页链接再次访问)]is hardcoded in two places:enrichCloudDocument(creates it) andappendCloudDocumentContent(checks for it). Is there a core-level helper for "marker-prefixed forwarded content" that I missed, or is this the right layer for it? Just asking because if the same pattern needs to apply to future platform adapters (e.g. Lark, DingTalk), centralizing it would reduce duplication.
Testing / Risk
- CI: 5/5 SUCCESS (run 34972435417) — lint / unit-test / smoke-test / regression-test / performance-test all green
- Merge state: CLEAN
- Local verification:
gofmt -lclean on changed files. Sandbox lacks vendored deps so I couldn't rungo test -racelocally, but the CI covers it. - Test quality: All 9 new tests use
httptest.NewServerend-to-end, so they exercise real HTTP plumbing, JSON parsing, signing, and MIME detection together — not just isolated units. TheassertWPSResourceAuthhelper is the kind of cross-cutting assertion that catches signing regressions when someone later changes the KSO-1 scheme. - Remaining risk: minimal. Behavior change is scoped to the wps-xiezuo platform and only activates for the new message types (
image,file,rich_text). Existing text-only path is unchanged. Single-workspace / other-platform users see zero impact.
Next step
Maintainer merge. No blockers, no required follow-ups from this PR.
(1)
(2) |
chenhg5
left a comment
There was a problem hiding this comment.
QA Re-review — PR #1850 (head 26f7fce)
Two new commits since my last review (d64dabb + 35b7ac1 + 26f7fce). CI still 5/5 green, mergeStateStatus CLEAN.
Conclusion: still APPROVE. No new blockers introduced; in fact one of my prior P3 nits is now fixed.
What changed
max_attachment_bytesconfig — replaces the hardcoded 50 MiB default with a user-tunable setting (default 2 GiB, hard cap 5 GiB). StrictparseWPSAttachmentLimitvalidates>0,<= 5 GiB, and accepts int/int8-64/uint8-64/float32-64/string input. This resolves the P3 nit I raised in the original review ("config layer didn't expose the limit") — thanks.- Two-way file/image upload — implements
core.ImageSender+core.FileSender. NewSendFile/SendImagemethods use a 2-step upload-then-message-create flow against WPS's/v7/chats/resources/upload→upload_entryURL →/v7/messages/create. ExtractedcreateWPSMessageconsolidates the KSO-1 signing path that was previously duplicated. messageContent.Text/Image/Fileare now pointers — JSON omitempty works correctly for image/file-only messages.checkWPSAPIResponseunifies response-code parsing — handles WPS API quirks wherecodeis sometimes an int, sometimes a string (usesjson.RawMessage+ dual-decoder).wpsCloudDocumentMarkerextracted to a const — fixes a brittle inline string that appeared in two places.core.RedactTokenused in error messages — prevents bearer token leak to logs on non-200 responses.
✅ What looks good
- 5 new tests cover the new code:
TestNew_CustomAttachmentLimit,TestNew_RejectsAttachmentLimitAboveWPSBound,TestSendFile_UploadsResourceAndCreatesMessage,TestSendFile_RejectsConfiguredSizeLimit,TestSendImage_UploadsResourceAndCreatesImageMessage.TestPlatformImplementsInterfacesnow asserts bothcore.ImageSenderandcore.FileSender. validateResourceUploadURLmirrors the download SSRF guard — same scheme/host/userinfo constraints, parameterized by operation name.getTokenis mutex-guarded with expires_in caching — the double-call inSendFile(upload + create) is effectively free on the second hit.normalizeWPSImageMIMEwhitelists onlyimage/jpg|png|gif|webp— explicit reject of unknown image types rather than silent fallback to a generic image type.- The
messageContentpointer refactor is the right call — without it, sending an image-only message would emit"text": {}and confuse the WPS server. - Documentation in
config.example.tomlanddocs/wps-xiezuo.mdis honest about the limit semantics: "This is an adapter-side bound, not a claim that the chat-resource API guarantees 5 GiB."
🟠 Should improve (not blocking)
- In-memory upload for large files:
core.FileAttachment.Data []byteandmultipart.NewWriteroverbytes.Bufferboth hold the entire payload in RAM. With the new 5 GiB cap, a singleSendFilecall can reserve up to 5 GiB of heap. Realistic triggers are limited (agentsend_file/send_imagetool calls,cc-connect sendCLI), and the failure mode (OOM kill) is observable rather than silent. But this is a real concern. Suggested follow-up: streaming upload withio.Reader+ chunked PUT, or document the RAM cost loudly in user-facing docs. - 40× default-jump from 50 MiB to 2 GiB: previously anyone hitting the cap got a clear "exceeds limit" error. Now users will silently get larger uploads accepted up to 2 GiB; if WPS API itself rejects, the failure surfaces correctly but the user has no early warning. The config docs already flag this — good.
🔵 Optional / future
- The
SSRF doesn't block private IPsP3 nit from the original review still applies (127.0.0.1, 169.254.169.254). Author didn't address it. Threat model remains limited (attacker would need valid app_secret + bearer to inject a malicious URL), so still not blocking. - A test that exercises
parseWPSAttachmentLimitwith non-int types (float64 fractional, string with whitespace, zero, negative, exactly 5 GiB) would lock in the bounds. The unit tests cover happy path + over-limit only.
Testing / Risk
- CI: 5/5 PASS.
mergeStateStatus: CLEAN. - I matched the 5 new test names against the new code paths; the
SendFiletest verifies checksum (sha256) round-trip and KSO-1 signing, theSendImagetest verifies image data round-trip and storage key propagation.
Next step
- Approve for merge. Author addressed my prior P3 nit. New risks are bounded (in-memory upload for large files) but not blocking. Re-review addendum doc:
doc-20260916-ssx61w.
— QA claude (cc-connect/qa-claudecode)
Summary
Extend WPS Xiezuo inbound message parsing to support images, local files, and cloud documents.
This change downloads images and local files from WPS chat messages and forwards them to the configured agent as attachments. It also resolves cloud-document links and includes the document content when the application has the required permissions, with a link-only fallback when the content cannot be accessed.
Type of change
Testing
Automated tests added in this PR
TestHandleChatMessage_ImageAttachmentinplatform/wps-xiezuo/wpsxiezuo_test.go— verifies that chat images are downloaded and forwarded as image attachments.TestHandleChatMessage_LocalFileAttachmentinplatform/wps-xiezuo/wpsxiezuo_test.go— verifies that local files are downloaded and forwarded as file attachments.TestHandleChatMessage_CloudDocumentForwardsLinkinplatform/wps-xiezuo/wpsxiezuo_test.go— verifies cloud-document links are preserved when document content is unavailable.TestHandleChatMessage_RichTextEmbeddedCloudDocumentinplatform/wps-xiezuo/wpsxiezuo_test.go— verifies cloud documents embedded in rich text are parsed.TestHandleChatMessage_RichTextEmbeddedCloudDocumentReadsContentinplatform/wps-xiezuo/wpsxiezuo_test.go— verifies authorized cloud-document content is fetched and included in the message.TestHandleChatMessage_RichTextImageAttachmentinplatform/wps-xiezuo/wpsxiezuo_test.go— verifies images embedded in rich-text messages are downloaded and forwarded.TestDownloadMessageResource_RejectsOversizeBodyinplatform/wps-xiezuo/wpsxiezuo_test.go— verifies oversized attachments are rejected.Package-level test: