feat(connections): add Lark CLI direct connection - #299
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughAdds Lark CLI as a local direct Connections provider. The change manages pinned, checksum-verified binaries and exported Skills, supports browser-based authentication and updates, and exposes connection state through LinkRuntimeService. Electron Agent startup receives the active runtime, isolated configuration, binary path, and Skills. The Connections UI displays direct-mode metadata, runtime status, lifecycle progress, and direct connect, disconnect, and cancellation actions. Packaging and postinstall flows include the Lark CLI assets. Sequence Diagram(s)sequenceDiagram
participant ConnectionsUI
participant useLarkCliConnection
participant LinkRuntimeServiceImpl
participant LarkCliManager
participant Browser
ConnectionsUI->>useLarkCliConnection: connect
useLarkCliConnection->>LinkRuntimeServiceImpl: connectLarkCli()
LinkRuntimeServiceImpl->>LarkCliManager: connect()
LarkCliManager->>Browser: open official authorization URL
Browser-->>LarkCliManager: authorization completed
LarkCliManager-->>LinkRuntimeServiceImpl: LarkCliState
LinkRuntimeServiceImpl-->>useLarkCliConnection: larkCliChanged
useLarkCliConnection-->>ConnectionsUI: updated provider state
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/routes/Connections/connection-route-model.ts (1)
182-198: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
getProviderDescriptionloses the needs_attention message for lark-cli.
if (provider.description) return provider.description(Line 183) now runs before theswitchonprovider.status.larkCliProviderFromState(useLarkCliConnection.tsLine 38, 47) setsdescription: copy.descriptionunconditionally, so for lark-cli this short-circuit fires regardless of status. When the connection expires (status === "needs_attention"), the detail pane and catalog row keep showing the generic Lark marketing text instead oft("connections.providerNeedsAttentionDescription", { name }), losing the reauthorization hint that other providers still get.Preserve the needs_attention case before falling back to
provider.description:🐛 Proposed fix
export function getProviderDescription(provider: ConnectionProviderSummary, t: TranslateFn): string { - if (provider.description) return provider.description switch (provider.status) { case "needs_attention": return t("connections.providerNeedsAttentionDescription", { name: provider.displayName }) case "connected": + if (provider.description) return provider.description if (isDirectlyAvailableProvider(provider)) { return t("connections.noAuthReadyDescription") } if (provider.appCount > 1) { return t("connections.connectionCount", { count: provider.appCount }) } return provider.accountLabel ?? getProviderCategoryLabel(provider, t) case "available": + if (provider.description) return provider.description return getProviderCategoryLabel(provider, t) } }🤖 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/connection-route-model.ts` around lines 182 - 198, Update getProviderDescription so the needs_attention status is handled before the provider.description early return, ensuring it always uses connections.providerNeedsAttentionDescription with the provider display name. Preserve the existing description fallback and all other status-specific behavior for non-needs_attention providers.src/routes/Connections/index.tsx (1)
295-309: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSelection-cleanup effect can reset the Lark CLI selection before its state finishes loading.
This effect used to require a loaded summary before running; the diff removes that guard ("Provider selection cleanup now runs without requiring a loaded summary").
providers(Line 123-129) only includes the Lark CLI entry oncelarkCli.stateresolves from the asyncgetLarkCliState()call inuseLarkCliConnection.ts(Line 77-81). IfselectProvider("lark-cli")runs (for example through therequestedServiceeffect at Line 261-269, driven byauthIntent.serviceorselectedService) before that async state resolves,filteredProviderswill not yet contain"lark-cli". On the very next render this effect seesselectedProviderService === "lark-cli"with no match infilteredProvidersand immediately callssetSelectedProviderService(null), closing the detail pane before the Lark CLI provider ever gets a chance to appear.Skip the reset while the Lark CLI provider is still pending its initial load:
🐛 Proposed fix
React.useEffect(() => { if (!selectedProviderService) { return } - if (filteredProviders.some((provider) => provider.service === selectedProviderService)) { + if ( + filteredProviders.some((provider) => provider.service === selectedProviderService) || + (selectedProviderService === "lark-cli" && !larkCliProvider) + ) { return } clearDetailCloseTimer() setSelectedProviderService(null) setDetailPaneClosing(false) setNarrowPane("list") - }, [clearDetailCloseTimer, filteredProviders, selectedProviderService]) + }, [clearDetailCloseTimer, filteredProviders, larkCliProvider, selectedProviderService])Based on the summarized change ("Provider selection cleanup now runs without requiring a loaded summary"), this guard removal is what exposes the race against the asynchronously loaded Lark CLI provider.
🤖 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 295 - 309, Update the selection-cleanup effect around selectedProviderService and filteredProviders to avoid clearing the "lark-cli" selection while its initial asynchronous state is still loading. Preserve the existing reset behavior once loading completes and the provider remains absent, using the existing Lark CLI loading state symbol to identify the pending condition.
🧹 Nitpick comments (6)
electron/link-runtime/lark-cli.test.ts (1)
5-25: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for the credential redaction path.
redactCommandErrorinelectron/link-runtime/lark-cli.tsremoves URLs and token values from error text that reaches the renderer. That path carries the highest privacy risk in this module, and no test covers it. Export the function and assert thatdevice_code,app_secret,access_token, andrefresh_tokenvalues are replaced.🤖 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/lark-cli.test.ts` around lines 5 - 25, Add coverage for the credential-sanitization path by exporting redactCommandError from lark-cli.ts and adding a test in the existing Lark CLI test file. Construct error text containing device_code, app_secret, access_token, and refresh_token values, then assert redactCommandError replaces each value with the expected redacted representation while preserving non-sensitive text.electron/link-runtime/lark-cli.ts (2)
449-479: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueSkill export recursion has no depth limit.
exportSkillDirectorycalls itself for each entry that reportsis_dir. The recursion depends on the CLI output. If the CLI returns an entry whose path repeats an ancestor, the export never terminates. Add a depth limit or a visited-path set.🤖 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/lark-cli.ts` around lines 449 - 479, The exportSkillDirectory recursion can loop indefinitely when CLI directory entries repeat an ancestor path. Update exportSkillDirectory to track visited directory paths or enforce a recursion depth limit, and stop or reject traversal when the same directory is encountered again; preserve normal recursive export behavior for valid, non-repeating paths.
356-372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winManaged runtime versions accumulate without cleanup.
Every successful update writes a new directory under
runtime/versions/<version>and keeps the previous directories. The disk usage grows with each update. Remove the directories that are not referenced bycurrent.jsonafter the marker write succeeds.Also applies to: 401-425
🤖 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/lark-cli.ts` around lines 356 - 372, The installLatestIfAvailable method must clean up stale runtime version directories after successfully writing runtime/current.json. Enumerate runtime/versions, preserve only the version referenced by the newly written active bundle (and any versions required by the existing marker), and remove all unreferenced directories after atomicWriteText succeeds; keep update state and return behavior unchanged.scripts/lark-cli.ts (2)
175-195: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
exportLarkCliSkillsalways re-exports, with no cached-version short-circuit.
downloadLarkCliBinaryskips work whenisPinnedBinaryReadyfinds a matching.versionmarker.exportLarkCliSkillshas no equivalent check: every call re-runsskills listand askills readper file, even whenoutputRoot/.versionalready matchesLARK_CLI_VERSIONfrom a previous run.scripts/prepare-binaries.tscalls this on every build, and CI/dev cycles will re-run the full skill export unnecessarily.Add a short-circuit that reads
outputRoot/.versionand returns early when it already matchesLARK_CLI_VERSION, mirroringisPinnedBinaryReady.🤖 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/lark-cli.ts` around lines 175 - 195, Update exportLarkCliSkills to read outputRoot/.version before downloading or listing skills, and return outputRoot immediately when its trimmed contents match LARK_CLI_VERSION. Reuse the existing version-marker behavior from isPinnedBinaryReady while allowing missing or mismatched markers to continue the current export flow.
43-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPure helpers
resolveLarkCliTargetandchecksumForAssethave no unit tests.Both functions encode non-trivial branching logic: arch remapping (
x64→amd64), platform-specific archive/asset naming, and a hand-written regex parse ofchecksums.txtlines. Both are pure and easy to unit test.scripts/ripgrep.tshas a siblingripgrep.test.tscovering its own pure helper (extractFileFromZip), so this file diverges from an established local testing pattern.Add unit tests covering: each supported platform/arch combination, the unsupported-arch throw path, and
checksumForAssetparsing bothhash filenameandhash *filename(binary mode) checksum line formats.🤖 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/lark-cli.ts` around lines 43 - 86, Add unit tests following the local pattern used by scripts/ripgrep.test.ts for resolveLarkCliTarget and checksumForAsset. Cover supported macOS, Linux, and Windows architecture combinations including x64 remapping, unsupported platform/architecture throws, and checksum lines using both “hash filename” and “hash *filename” formats, while asserting the expected archive and asset metadata.src/hooks/useLarkCliConnection.ts (1)
125-132: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the hook's return value for referential stability.
useLarkCliConnectionreturns a new object literal on every render (Line 125-131).index.tsxputs this whole object (larkCli) into the dependency arrays ofconnectProviderandconfirmDisconnectTarget(useCallback), so both callbacks are recreated on every render regardless of whetherconnect,disconnect, orcancelactually changed. This defeats the memoization thoseuseCallbackcalls are meant to provide.Wrap the return value in
React.useMemokeyed oncancel,connect,disconnect,error, andstateso consumers only see a new reference when something meaningful changes.🤖 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/hooks/useLarkCliConnection.ts` around lines 125 - 132, Update useLarkCliConnection’s returned object to use React.useMemo, with cancel, connect, disconnect, error, and state as dependencies, so its reference remains stable when those values are unchanged.
🤖 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/agent/workspace.ts`:
- Around line 76-110: Update the skill synchronization flow around the source
discovery loop and skillDir cleanup so an unreadable bundled source does not
cause its previously synchronized skills to be removed when another source is
available. Preserve existing directories for unavailable sources, or stage and
merge only successfully read sources while still replacing successfully read
source contents; add a test covering readable bundledSkillsDir with unavailable
bundledLarkSkillsDir during agent restart.
In `@electron/link-runtime/lark-cli.ts`:
- Around line 145-150: Update cancelConnection() so it retains the activeChild
reference after sending SIGTERM, then schedules a short-delay SIGKILL fallback
if the child is still running. Clear activeChild only when the process exits or
after the fallback, and ensure cancellation still transitions the manager to the
idle phase without leaving stale cancellation state.
- Around line 116-143: Update the operation bookkeeping in connect() and
disconnect() to track each in-flight promise with its operation kind, allowing
connect() to reuse only an active connect operation and reject when a disconnect
is running, while disconnect() preserves the corresponding mismatch rejection
behavior. Adjust getState() to read the new operation structure when checking
for an active operation, and ensure each finally block clears the stored entry
only when it still references its own promise.
- Around line 427-435: Update fetchDownload to read response.body through its
stream reader instead of calling response.arrayBuffer(), accumulating chunks
while tracking total bytes and aborting immediately once the total exceeds
maxDownloadBytes. Preserve the existing HTTP-status and content-length checks,
and return a Buffer assembled only from chunks that remain within the limit.
In `@scripts/lark-cli.ts`:
- Around line 88-134: Update downloadLarkCliBinary and the version configuration
around LARK_CLI_VERSION to use a maintainer-committed expected SHA-256 value
pinned alongside the version, rather than trusting checksums.txt fetched from
the same release. Verify the downloaded archive against that source-pinned
checksum, preserving the existing mismatch error behavior and bump workflow when
LARK_CLI_VERSION changes; remove the fetched checksum dependency unless it
remains as an additional check.
In `@src/hooks/useLarkCliConnection.ts`:
- Around line 15-30: Update appFromState and larkCliProviderFromState so
connectedUpdatedAt reflects the timestamp when the connection transitions into
"connected", not each state push. Track that transition timestamp once using the
hook’s persistent state/ref, pass it through the app summary, and keep it
unchanged while the connection remains connected; reset or update it
appropriately for later reconnections.
- Around line 113-117: Update the cancel callback in useLarkCliConnection to
catch rejections from linkRuntimeService.invoke("cancelLarkCliConnection") and
route the caught error through setError, matching the existing mutate
error-handling pattern. Preserve the current cancellationRequestedRef and
setError(null) behavior while ensuring cancel() does not return an unhandled
rejected promise to its onClick callers.
In `@src/routes/Connections/index.tsx`:
- Around line 154-193: Update the lark CLI polling and cancellation flow around
larkCliBusy, selectedProviderPolling, and cancelSelectedProviderPolling so the
"disconnecting" phase is not treated as cancelable polling. Preserve
disconnect()’s in-flight auth logout state and ensure cancellation only applies
to phases handled by cancelConnection().
---
Outside diff comments:
In `@src/routes/Connections/connection-route-model.ts`:
- Around line 182-198: Update getProviderDescription so the needs_attention
status is handled before the provider.description early return, ensuring it
always uses connections.providerNeedsAttentionDescription with the provider
display name. Preserve the existing description fallback and all other
status-specific behavior for non-needs_attention providers.
In `@src/routes/Connections/index.tsx`:
- Around line 295-309: Update the selection-cleanup effect around
selectedProviderService and filteredProviders to avoid clearing the "lark-cli"
selection while its initial asynchronous state is still loading. Preserve the
existing reset behavior once loading completes and the provider remains absent,
using the existing Lark CLI loading state symbol to identify the pending
condition.
---
Nitpick comments:
In `@electron/link-runtime/lark-cli.test.ts`:
- Around line 5-25: Add coverage for the credential-sanitization path by
exporting redactCommandError from lark-cli.ts and adding a test in the existing
Lark CLI test file. Construct error text containing device_code, app_secret,
access_token, and refresh_token values, then assert redactCommandError replaces
each value with the expected redacted representation while preserving
non-sensitive text.
In `@electron/link-runtime/lark-cli.ts`:
- Around line 449-479: The exportSkillDirectory recursion can loop indefinitely
when CLI directory entries repeat an ancestor path. Update exportSkillDirectory
to track visited directory paths or enforce a recursion depth limit, and stop or
reject traversal when the same directory is encountered again; preserve normal
recursive export behavior for valid, non-repeating paths.
- Around line 356-372: The installLatestIfAvailable method must clean up stale
runtime version directories after successfully writing runtime/current.json.
Enumerate runtime/versions, preserve only the version referenced by the newly
written active bundle (and any versions required by the existing marker), and
remove all unreferenced directories after atomicWriteText succeeds; keep update
state and return behavior unchanged.
In `@scripts/lark-cli.ts`:
- Around line 175-195: Update exportLarkCliSkills to read outputRoot/.version
before downloading or listing skills, and return outputRoot immediately when its
trimmed contents match LARK_CLI_VERSION. Reuse the existing version-marker
behavior from isPinnedBinaryReady while allowing missing or mismatched markers
to continue the current export flow.
- Around line 43-86: Add unit tests following the local pattern used by
scripts/ripgrep.test.ts for resolveLarkCliTarget and checksumForAsset. Cover
supported macOS, Linux, and Windows architecture combinations including x64
remapping, unsupported platform/architecture throws, and checksum lines using
both “hash filename” and “hash *filename” formats, while asserting the expected
archive and asset metadata.
In `@src/hooks/useLarkCliConnection.ts`:
- Around line 125-132: Update useLarkCliConnection’s returned object to use
React.useMemo, with cancel, connect, disconnect, error, and state as
dependencies, so its reference remains stable when those values are unchanged.
🪄 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: 9d40fde1-05b6-4f05-8b4e-f20fc0975d53
⛔ Files ignored due to path filters (1)
src/assets/apps/lark.svgis excluded by!**/*.svg
📒 Files selected for processing (25)
.gitignoredocs/architecture.mdelectron-builder.tselectron/agent/binaries.tselectron/agent/manager.tselectron/agent/workspace.test.tselectron/agent/workspace.tselectron/connections/common.tselectron/link-runtime/common.tselectron/link-runtime/lark-cli.test.tselectron/link-runtime/lark-cli.tselectron/link-runtime/node.tselectron/main.tspackage.jsonscripts/download-lark-cli.tsscripts/lark-cli.tsscripts/prepare-binaries.tsscripts/ripgrep.tssrc/hooks/useLarkCliConnection.tssrc/i18n/app-messages.en.tssrc/i18n/app-messages.zh.tssrc/routes/Connections/ConnectionProviderDetailPane.tsxsrc/routes/Connections/connection-route-model.test.tssrc/routes/Connections/connection-route-model.tssrc/routes/Connections/index.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
electron/link-runtime/lark-cli.ts (1)
334-354: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer the bundled runtime when it is newer than the persisted managed version.
resolveActiveBundlealways prefers the persisted managed version when its files exist. After an app upgrade ships a newer bundled CLI, the older managed version still wins. The user stays on the older CLI until an update check succeeds and downloads a newer release.Compare the two versions with
isVersionNewerand select the bundled runtime when it is newer.🤖 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/lark-cli.ts` around lines 334 - 354, The resolveActiveBundle method should compare the persisted managed version with the bundled runtime version using isVersionNewer after both bundles are confirmed available. Prefer and return the bundled bundle when its version is newer; otherwise preserve the existing managed-first behavior and fallback to bundled when the managed bundle is unavailable.scripts/lark-cli.ts (1)
183-193: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueA failed
renameleaves no skills directory.Line 188 removes
outputRootbefore line 189 renamesstaginginto place. If therenamecall fails, the previous export is already gone and thefinallyblock removesstagingas well. The build then has no skills directory at all.Move the old directory aside first, and restore it if the rename fails.
♻️ Proposed change
try { for (const skillName of skillNames) await exportSkillDirectory(binary, skillName, "", staging) - await rm(outputRoot, { force: true, recursive: true }) - await rename(staging, outputRoot) + const previous = `${outputRoot}.previous` + await rm(previous, { force: true, recursive: true }) + await rename(outputRoot, previous).catch(() => undefined) + try { + await rename(staging, outputRoot) + } catch (error) { + await rename(previous, outputRoot).catch(() => undefined) + throw error + } + await rm(previous, { force: true, recursive: true }) } finally {🤖 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/lark-cli.ts` around lines 183 - 193, Update the export flow around exportSkillDirectory to preserve the existing outputRoot during replacement: move it to a temporary backup before renaming staging, restore that backup if the staging rename fails, and remove the backup only after a successful replacement. Keep staging cleanup in the finally block and ensure outputRoot remains available when replacement fails.
🤖 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/lark-cli.ts`:
- Around line 384-399: Update downloadBundle to authenticate the downloaded
archive against a trusted source independent of officialReleaseBase, rather than
relying only on checksums.txt from the same release. Add release-signature
verification or use an embedded/independently retrieved checksum allowlist
before extracting, writing, activating, or executing the bundle; preserve
rejection on any missing or mismatched verification data.
- Around line 94-102: Update the success-state assignment in the relevant
refresh flow to explicitly clear the stale error while preserving the existing
state fields and healthy connection values. Add the reset alongside
activeVersion, available, and phase in the object assigned to this.state.
---
Nitpick comments:
In `@electron/link-runtime/lark-cli.ts`:
- Around line 334-354: The resolveActiveBundle method should compare the
persisted managed version with the bundled runtime version using isVersionNewer
after both bundles are confirmed available. Prefer and return the bundled bundle
when its version is newer; otherwise preserve the existing managed-first
behavior and fallback to bundled when the managed bundle is unavailable.
In `@scripts/lark-cli.ts`:
- Around line 183-193: Update the export flow around exportSkillDirectory to
preserve the existing outputRoot during replacement: move it to a temporary
backup before renaming staging, restore that backup if the staging rename fails,
and remove the backup only after a successful replacement. Keep staging cleanup
in the finally block and ensure outputRoot remains available when replacement
fails.
🪄 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: 542c7350-a9f6-4850-bb66-5792cc8273ac
⛔ Files ignored due to path filters (1)
src/assets/apps/lark.svgis excluded by!**/*.svg
📒 Files selected for processing (26)
.gitignoredocs/architecture.mdelectron-builder.tselectron/agent/binaries.tselectron/agent/manager.tselectron/agent/workspace.test.tselectron/agent/workspace.tselectron/connections/common.tselectron/link-runtime/common.tselectron/link-runtime/lark-cli.test.tselectron/link-runtime/lark-cli.tselectron/link-runtime/node.tselectron/main.tspackage.jsonscripts/download-lark-cli.tsscripts/lark-cli.tsscripts/prepare-binaries.tsscripts/ripgrep.tssrc/hooks/useLarkCliConnection.tssrc/i18n/app-messages.en.tssrc/i18n/app-messages.zh.tssrc/routes/Connections/ConnectionAccountsList.tsxsrc/routes/Connections/ConnectionProviderDetailPane.tsxsrc/routes/Connections/connection-route-model.test.tssrc/routes/Connections/connection-route-model.tssrc/routes/Connections/index.tsx
🚧 Files skipped from review as they are similar to previous changes (23)
- electron-builder.ts
- src/routes/Connections/connection-route-model.ts
- electron/connections/common.ts
- scripts/ripgrep.ts
- .gitignore
- electron/main.ts
- docs/architecture.md
- src/i18n/app-messages.en.ts
- scripts/download-lark-cli.ts
- src/i18n/app-messages.zh.ts
- electron/link-runtime/common.ts
- src/routes/Connections/connection-route-model.test.ts
- src/routes/Connections/ConnectionProviderDetailPane.tsx
- package.json
- electron/agent/workspace.test.ts
- electron/link-runtime/lark-cli.test.ts
- electron/agent/workspace.ts
- scripts/prepare-binaries.ts
- electron/link-runtime/node.ts
- src/routes/Connections/ConnectionAccountsList.tsx
- src/hooks/useLarkCliConnection.ts
- electron/agent/binaries.ts
- electron/agent/manager.ts
alwaysmavs
changed the title
|
Addressed the AI review in 2ffbd00:
The checksum parser was removed when build-time verification switched to source-pinned checksums, so parser-specific tests are no longer applicable. Full test suite and production build pass. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/link-runtime/lark-cli.ts (1)
378-395: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDecouple stale-version cleanup from update activation and agent retirement.
- Catch and log
removeStaleVersionsfailures so a successful update does not trigger fallback to a deleted oldbundle.binaryPath.- Retain the old version until the Agent sidecar stops using its
larkCliBinPathandPATH. Refresh is deferred while a generation is busy, and POSIX cleanup can delete the old directory before that refresh completes.🤖 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/lark-cli.ts` around lines 378 - 395, Update installLatestIfAvailable so failures from removeStaleVersions are caught and logged without failing the otherwise successful update. Coordinate stale-version deletion with the Agent sidecar lifecycle: retain the previous bundle until its larkCliBinPath and PATH usage has stopped, including when refresh is deferred because the current generation is busy, then perform cleanup after refresh/retirement completes.
🧹 Nitpick comments (1)
electron/link-runtime/lark-cli.ts (1)
466-511: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsolidate the duplicated skill-directory traversal.
electron/link-runtime/lark-cli.tsandscripts/lark-cli.tsmaintain separate recursive exporters with different path-validation rules. Share the traversal and validation logic to prevent future fixes from diverging.🤖 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/lark-cli.ts` around lines 466 - 511, Consolidate the recursive skill export and path-validation logic currently duplicated between exportSkills/exportSkillDirectory and the corresponding scripts/lark-cli.ts implementation. Extract or reuse a shared traversal helper, including safeRelativePath checks, directory-cycle detection, listing, reading, and file writing, while preserving each caller’s existing CLI integration and output behavior.
🤖 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.
Outside diff comments:
In `@electron/link-runtime/lark-cli.ts`:
- Around line 378-395: Update installLatestIfAvailable so failures from
removeStaleVersions are caught and logged without failing the otherwise
successful update. Coordinate stale-version deletion with the Agent sidecar
lifecycle: retain the previous bundle until its larkCliBinPath and PATH usage
has stopped, including when refresh is deferred because the current generation
is busy, then perform cleanup after refresh/retirement completes.
---
Nitpick comments:
In `@electron/link-runtime/lark-cli.ts`:
- Around line 466-511: Consolidate the recursive skill export and
path-validation logic currently duplicated between
exportSkills/exportSkillDirectory and the corresponding scripts/lark-cli.ts
implementation. Extract or reuse a shared traversal helper, including
safeRelativePath checks, directory-cycle detection, listing, reading, and file
writing, while preserving each caller’s existing CLI integration and output
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 616dfee2-f9fb-4e03-bffd-56da0175626e
📒 Files selected for processing (9)
electron/link-runtime/lark-cli.test.tselectron/link-runtime/lark-cli.tsscripts/lark-cli.test.tsscripts/lark-cli.tssrc/hooks/useLarkCliConnection.test.tssrc/hooks/useLarkCliConnection.tssrc/routes/Connections/connection-route-model.test.tssrc/routes/Connections/connection-route-model.tssrc/routes/Connections/index.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- electron/link-runtime/lark-cli.test.ts
- src/routes/Connections/connection-route-model.test.ts
- src/routes/Connections/connection-route-model.ts
- src/routes/Connections/index.tsx
1 participant
Issue
Wanta's Connections catalog did not provide a local first-party path for the official Lark/Feishu CLI. Users had to install the CLI, keep its skills synchronized, and complete configuration and user authorization outside the product before the agent could use Lark capabilities. This also meant the application could not present connection state, guide browser authorization, or keep the CLI runtime current.
User impact
This adds a Lark CLI provider to the existing Connections experience and labels it as Direct mode. A user can connect from the same provider detail UI, complete the official browser-based app setup and OAuth flow, see the active account and CLI version, cancel an in-progress flow, and disconnect. Once authorized, the agent receives the active CLI on
PATH, the isolated CLI config directory, and the matching embeddedlark-*skills.Root cause
The existing catalog models only represented remote connector providers. The Electron runtime had no owner for a local CLI lifecycle, no IPC-safe redacted state for local authorization, no packaged Lark binary or skills, and no mechanism to inject a version-matched direct runtime into the OpenCode sidecar.
Fix
LarkCliManagerthat owns an isolated config directory, redacts sensitive command output, allowlists official authorization URLs, and drivesconfig init,auth login, verification, cancellation, and logout./v1/apps/by-id/direct:....lark-*skills into the private agent workspace independently of the selected OOMOL/OpenConnector Link backend.Credentials remain out of the renderer. Only redacted state crosses IPC; CLI configuration is isolated under app user data and tokens continue to be owned by the official CLI/system keychain.
Validation
pnpm lintpnpm ts-checkpnpm test— 284 test files, 2149 tests passedpnpm build:apppnpm prepare:binaries1.0.81, exported 27 embedded skills, and a healthy disconnected initial state.