feat(wecom): add CLI integration - #300
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThis change adds WeCom CLI support across distribution, runtime, Agent integration, and Connections UI. The build downloads pinned platform binaries and Skills with validation and integrity checks. The Electron runtime manages QR authorization, isolated credentials, connection state, cancellation, and redacted output. The Agent receives WeCom paths and direct Skills. The UI exposes WeCom connection actions, status phases, polling reopening, localized labels, and provider details. Sequence Diagram(s)sequenceDiagram
participant User
participant ConnectionsUI
participant LinkRuntimeService
participant WecomCliManager
participant WeComCLI
User->>ConnectionsUI: Start WeCom connection
ConnectionsUI->>LinkRuntimeService: connectWecomCli()
LinkRuntimeService->>WecomCliManager: connect()
WecomCliManager->>WeComCLI: Run QR authorization
WeComCLI-->>WecomCliManager: Authorization URL and status
WecomCliManager-->>LinkRuntimeService: Updated WecomCliState
LinkRuntimeService-->>ConnectionsUI: wecomCliChanged
ConnectionsUI-->>User: Display connection status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
scripts/wecom-cli.ts (1)
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument or enforce the version/commit pairing.
WECOM_CLI_VERSIONandWECOM_CLI_GIT_HEADare two independent pins. Nothing ties them together. If a future update bumps one without the other, the packaged CLI binary and the exported Skills can silently diverge (for example, a Skill that references a CLI subcommand the pinned binary does not yet support). Add a comment stating that both constants must be updated together, or add a lightweight consistency check (for example, verify the binary's reported capabilities against the exported Skills at build time).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/wecom-cli.ts` around lines 15 - 16, Document or enforce the pairing between WECOM_CLI_VERSION and WECOM_CLI_GIT_HEAD so they cannot be updated independently. Prefer a clear comment requiring both constants to change together, or add a lightweight build-time consistency check validating the pinned binary against the exported Skills.scripts/wecom-cli.test.ts (1)
1-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the path-traversal guard and tar parsing.
The tests cover
resolveWecomCliTargetandwecomCliBinaryName, both pure and low-risk.safeSkillPathandtarEntriesin scripts/wecom-cli.ts are not covered. These functions protect against path traversal and parse a custom tar format used to select what gets written into the app's Skills resources. Export these functions and add unit tests for at least: a skill path containing../, an absolute path, and a truncated/oversized tar entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/wecom-cli.test.ts` around lines 1 - 29, Export the safeSkillPath and tarEntries functions from scripts/wecom-cli.ts, then extend scripts/wecom-cli.test.ts with unit coverage for rejecting skill paths containing ../ and absolute paths, plus rejecting truncated or oversized entries in the custom tar parser. Keep the existing resolveWecomCliTarget and wecomCliBinaryName tests unchanged.src/routes/Connections/index.tsx (1)
184-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a single direct-provider lookup instead of repeated service ternaries.
Direct-provider dispatch by service is repeated at Lines 184-189, 198-205, 233-243, 251-260, 372-373, 391-394, and 541-545. Lark is matched by the literal
"lark-cli"while WeCom uses the exportedwecomCliService. A third direct provider requires an edit in every one of those places.Extract one map from service to
{ cli, busy, phaseKey }, and export alarkCliServiceconstant for symmetry. Behavior stays the same.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/Connections/index.tsx` around lines 184 - 263, Consolidate direct-provider dispatch in the Connections component by exporting a larkCliService constant and defining one service-keyed map containing each provider’s cli, busy state, and phase key. Update the repeated ternaries around selectedProviderDetail, busy/polling/progress handling, and direct-provider actions to reuse this lookup, preserving current Lark and WeCom behavior while making future providers require only one map entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/link-runtime/wecom-cli.ts`:
- Around line 170-175: Check for cancellation in connectNow after readAuthState
determines the account is not connected and before setting phase to
waiting_for_scan or invoking runAuthorizationCommand. Reuse the existing
cancellation assertion mechanism so a cancellation recorded during preparation
prevents the QR authorization process and external page from starting.
- Around line 277-290: Update the consume callback to call
findOfficialWecomAuthorizationUrl only with the portion of output ending at the
last newline, so URLs are matched only after complete lines are received.
Preserve the existing openedUrl latch and authorization-opening behavior once a
complete-line match is found.
In `@src/i18n/app-messages.zh.ts`:
- Around line 1100-1101: Update the localized value for
connections.wecomCli.description to distinguish capabilities by enterprise size:
enterprises over 10 people support documents and smart sheets, while enterprises
with 10 or fewer people support messaging, documents, schedules, meetings, and
to-dos. Remove the wording that states the official CLI is restricted to
enterprises with no more than 10 people.
---
Nitpick comments:
In `@scripts/wecom-cli.test.ts`:
- Around line 1-29: Export the safeSkillPath and tarEntries functions from
scripts/wecom-cli.ts, then extend scripts/wecom-cli.test.ts with unit coverage
for rejecting skill paths containing ../ and absolute paths, plus rejecting
truncated or oversized entries in the custom tar parser. Keep the existing
resolveWecomCliTarget and wecomCliBinaryName tests unchanged.
In `@scripts/wecom-cli.ts`:
- Around line 15-16: Document or enforce the pairing between WECOM_CLI_VERSION
and WECOM_CLI_GIT_HEAD so they cannot be updated independently. Prefer a clear
comment requiring both constants to change together, or add a lightweight
build-time consistency check validating the pinned binary against the exported
Skills.
In `@src/routes/Connections/index.tsx`:
- Around line 184-263: Consolidate direct-provider dispatch in the Connections
component by exporting a larkCliService constant and defining one service-keyed
map containing each provider’s cli, busy state, and phase key. Update the
repeated ternaries around selectedProviderDetail, busy/polling/progress
handling, and direct-provider actions to reuse this lookup, preserving current
Lark and WeCom behavior while making future providers require only one map
entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8f93272-5ec7-4268-ac5e-c1006ededd55
⛔ Files ignored due to path filters (1)
src/assets/apps/wecom.svgis excluded by!**/*.svg
📒 Files selected for processing (30)
.gitignoreNOTICETHIRD_PARTY_NOTICES.mddocs/architecture.mdelectron-builder.tselectron/agent/binaries.tselectron/agent/manager.test.tselectron/agent/manager.tselectron/agent/sidecar.tselectron/agent/workspace.test.tselectron/agent/workspace.tselectron/connections/common.tselectron/link-runtime/common.tselectron/link-runtime/node.tselectron/link-runtime/wecom-cli.test.tselectron/link-runtime/wecom-cli.tselectron/main.tspackage.jsonscripts/download-wecom-cli.tsscripts/prepare-binaries.tsscripts/wecom-cli.test.tsscripts/wecom-cli.tssrc/hooks/useLarkCliConnection.tssrc/hooks/useWecomCliConnection.test.tssrc/hooks/useWecomCliConnection.tssrc/i18n/app-messages.en.tssrc/i18n/app-messages.zh.tssrc/routes/Connections/ConnectionAccountsList.tsxsrc/routes/Connections/ConnectionProviderDetailPane.tsxsrc/routes/Connections/index.tsx
|
Addressed all actionable review feedback in
Validation completed:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
electron/link-runtime/wecom-cli.test.ts (1)
98-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWait for the authorization-status process before cancelling.
The fixed 25 ms delay does not prove that the mock reached the
auth --auth-statusbranch. If setup takes longer,cancelConnection()can run before the status child starts. The test can then pass without covering cancellation of the in-flight status check. Have the mock create a readiness marker beforesleep 0.1, and wait for that marker with a bounded timeout before callingcancelConnection().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/link-runtime/wecom-cli.test.ts` around lines 98 - 132, Update the “honors cancellation while checking existing authorization” test so its mock auth-status branch creates a readiness marker before sleeping, then wait for that marker with a bounded timeout before invoking manager.cancelConnection(). Preserve the cancellation assertions and cleanup while ensuring the test cancels only after the authorization-status process is in flight.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/wecom-cli.ts`:
- Around line 146-150: Update the TAR size parsing used by the WeCom CLI archive
reader to validate the entire size field before converting it, rejecting
non-octal trailing characters instead of allowing Number.parseInt to partially
parse them. Ensure malformed fields fail through the existing invalid or
truncated archive error path, and add a test covering a size value with an
invalid trailing character such as “00000000001x”.
---
Nitpick comments:
In `@electron/link-runtime/wecom-cli.test.ts`:
- Around line 98-132: Update the “honors cancellation while checking existing
authorization” test so its mock auth-status branch creates a readiness marker
before sleeping, then wait for that marker with a bounded timeout before
invoking manager.cancelConnection(). Preserve the cancellation assertions and
cleanup while ensuring the test cancels only after the authorization-status
process is in flight.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d896eccf-bf29-43a2-a6e8-0f067de925e3
📒 Files selected for processing (8)
electron/link-runtime/wecom-cli.test.tselectron/link-runtime/wecom-cli.tsscripts/wecom-cli.test.tsscripts/wecom-cli.tssrc/hooks/useLarkCliConnection.tssrc/i18n/app-messages.en.tssrc/i18n/app-messages.zh.tssrc/routes/Connections/index.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/i18n/app-messages.zh.ts
- src/i18n/app-messages.en.ts
- src/routes/Connections/index.tsx
- electron/link-runtime/wecom-cli.ts
| const dataEnd = dataStart + size | ||
| const nextOffset = dataStart + Math.ceil(size / 512) * 512 | ||
| if (!Number.isSafeInteger(size) || size < 0 || dataEnd > tar.length || nextOffset > tar.length) { | ||
| throw new Error(`Invalid or truncated WeCom CLI source archive entry: ${entryPath}`) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the TAR string normalization and numeric parsing path.
rg -n -C 4 'function tarString|const size = Number\.parseInt|Invalid or truncated' scripts/wecom-cli.ts
# Confirm that the test suite covers malformed numeric fields, not only truncation.
rg -n -C 3 'truncated|oversized|parseInt|invalid.*size|size.*invalid' scripts/wecom-cli.test.tsRepository: oomol-lab/wanta
Length of output: 1702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '120,175p' scripts/wecom-cli.ts
sed -n '1,80p' scripts/wecom-cli.test.ts
python3 - <<'PY'
import math
fields = ["00000000001x", "00000000001", "0000000000x1", "", "bad"]
for field in fields:
text = field.strip() or "0"
digits = []
for ch in text:
if ch in "01234567":
digits.append(ch)
else:
break
value = int("".join(digits), 8) if digits else None
print(repr(field), "parseInt-like:", value, "full-octal:", bool(text) and all(c in "01234567" for c in text))
PYRepository: oomol-lab/wanta
Length of output: 5022
Reject malformed TAR size fields.
Number.parseInt accepts an octal prefix and ignores invalid trailing bytes. A field such as 00000000001x parses as 1. Validate the complete size field and add a test for an invalid trailing character.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/wecom-cli.ts` around lines 146 - 150, Update the TAR size parsing
used by the WeCom CLI archive reader to validate the entire size field before
converting it, rejecting non-octal trailing characters instead of allowing
Number.parseInt to partially parse them. Ensure malformed fields fail through
the existing invalid or truncated archive error path, and add a test covering a
size value with an invalid trailing character such as “00000000001x”.
1 participant
Summary
Wanta already supported a local direct connection through the Lark CLI, but users who work in WeCom had no equivalent way to authorize the official WeCom CLI and make its tools available to the local agent. This change adds a dedicated WeCom CLI connection while keeping its QR-code workflow and bot identity model separate from Lark's authorization experience.
User impact
Users can now open Connections, select WeCom CLI, scan the enterprise WeChat QR code, and use the capabilities granted to the resulting WeCom bot. The connection page exposes progress, cancellation, QR-code reopening, runtime version, and disconnect controls. It uses the official WeCom brand icon rather than the consumer WeChat icon.
Root cause and implementation
The application previously had no bundled WeCom runtime, isolated credential directory, IPC lifecycle, renderer provider model, or agent-side Skill installation. The new
WecomCliManagerruns the pinned official@wecom/clibinary with a Wanta-owned config and temporary-data root, allowlists the official QR-code URL, redacts credential-bearing output, and exposes only safe connection metadata to the renderer.The build now downloads and verifies the platform-specific CLI package, exports the matching official Skills from the pinned source commit, and packages both resources. The agent workspace installs Lark and WeCom direct-mode Skills independently, injects the managed binary and isolated paths, and removes inherited WeCom logging overrides that could redirect credential-adjacent diagnostics.
The Connections UI routes each direct provider through its own hook and behavior. Shared presentation components gained only the small extension points required for provider-specific labels, reconnect behavior, QR reopening, and direct-provider actions.
Validation
pnpm lintpnpm build:apppnpm test— 289 test files and 2,164 tests passed during implementationpnpm prepare:binarieswecom-cli 0.1.9The unrelated untracked
docs/research/directory is intentionally excluded from this PR.