[withdrawn]#644
Conversation
Two independent bugs made `npx @larksuite/cli install` unreliable on slow emulators (iSH, QEMU, Wine) and underpowered CI runners: 1. **Hardcoded 120s timeouts**. `npm install -g` and `npx skills add` child processes take >120s on slow runtimes (measured 140s on iSH ARM64 for `skills add https://open.feishu.cn -y -g` which fetches 30+ skill manifests). The wizard always SIGTERM'd them mid-flight and reported "Failed to install skills", even though the underlying install was succeeding. New `LARK_CLI_INSTALL_TIMEOUT_MS` env var overrides, defaulting to 600000 (10 min) — safe for both fast and slow environments. 2. **`skillsAlreadyInstalled` regex never matches**. `skills ls -g` prints ANSI colour codes before each name (`\x1b[36mlark-approval`). The existing `/^lark-/m.test(out)` consequently returned false every time, so subsequent re-runs always triggered a fresh `skills add` (with the 120s timeout that then failed). Added a `stripAnsi` helper and apply it before matching; now re-runs correctly short-circuit with `step2Skip` ("Already installed. Skipped"). Verified on iSH ARM64 under macOS: - Scenario A (fresh install, skills not present): `skills add` runs for ~140s, succeeds, `step2Done` reached. Step 3 (QR code + app config) reached in ~5 min total. - Scenario B (re-run with skills already in ~/.agents/skills/): `skillsAlreadyInstalled` matches → `step2Skip` fires immediately → Step 3 reached in ~30s. Both scenarios exit 0 end-to-end. No regressions on fast Docker Alpine ARM64 (times are unchanged; the higher timeout ceiling only matters when the child is genuinely slow).
|
|
github-actions
Bot
added
the
size/M
📝 WalkthroughWalkthroughThe script now computes a configurable install timeout from an environment variable with a 10-minute default and applies it to child processes running Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/install-wizard.js (2)
97-101: ANSI regex is narrower than the comment suggests — fine for the current use case.The CSI pattern
\x1b\[[0-9;]*[a-zA-Z]won't match sequences that include intermediate bytes or private-mode markers (e.g.\x1b[?25l,\x1b[1;2Hwith non-letter terminators are covered, but\x1b[?…,\x1b[>…,\x1b[=…and 8-bit CSI\x9b…are not). For the specific goal — stripping the SGR color codes emitted byskills lsbefore matching/^lark-/m— this is sufficient, and the header comment already scopes it to "good enough for tool output." No change required; just flagging in casestripAnsiis ever reused for richer output.Consider the widely-used pattern from the
ansi-regexpackage if broader coverage is ever needed:♻️ Broader ANSI pattern (optional)
function stripAnsi(s) { - // Covers CSI (colour, cursor, etc.) and OSC (title). Good enough for - // tool output — we never need to preserve formatting in regex checks. - return String(s).replace(/\x1b\[[0-9;]*[a-zA-Z]/g, "").replace(/\x1b\][^\x07]*\x07/g, ""); + // Covers CSI (including private-mode `?`, `>`, `=`) and OSC (BEL or ST terminated). + const pattern = /[\u001B\u009B][[\]()#;?]*(?:(?:(?:(?:;[-a-zA-Z\d\/#&.:=?%@~_]+)*|[a-zA-Z\d]+(?:;[-a-zA-Z\d\/#&.:=?%@~_]*)*)?\u0007)|(?:(?:\d{1,4}(?:;\d{0,4})*)?[\dA-PR-TZcf-nq-uy=><~]))/g; + return String(s).replace(pattern, ""); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-wizard.js` around lines 97 - 101, The current stripAnsi function uses a narrow CSI/OSC regex that’s fine for stripping SGR color codes but doesn’t cover private-mode markers, 8-bit CSI, or other sequences; either update the comment in stripAnsi to explicitly document this limitation (mentioning it only targets SGR/tool output) or replace the implementation with a broader, well-tested pattern by importing the ansi-regex package and using it inside stripAnsi to ensure full ANSI coverage; reference the stripAnsi function and its existing regex when making the change.
277-290: Consider also stripping ANSI fromstderr/ capturing both streams.
runSilentAsync'sexecFilecallback resolves only withstdout. If a future version ofskillsroutes the listing to stderr (or mixes warnings in),skillsAlreadyInstalledwill silently returnfalseand trigger a redundant re-install — the very bug this PR fixes. Not a blocker since the currentskillsCLI writes to stdout, but worth a thought.Also note: applying the full
INSTALL_TIMEOUT_MS(10 min default) toskills ls -gis generous —lsshould return in seconds. A shorter timeout here would fail faster on a hung environment without affecting the slow-emulator fix foradd.🔧 Optional: tighter timeout for the list check
try { const out = await runSilentAsync("npx", ["-y", "skills", "ls", "-g"], { - timeout: INSTALL_TIMEOUT_MS, + timeout: Math.min(INSTALL_TIMEOUT_MS, 60000), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-wizard.js` around lines 277 - 290, skillsAlreadyInstalled currently only checks stdout from runSilentAsync and uses INSTALL_TIMEOUT_MS; update it to also capture and inspect stderr (or use a combined output) so stripAnsi is applied to both streams before testing with /^lark-/m, and reduce the timeout by using a small constant (e.g. SKILLS_LIST_TIMEOUT_MS ~ 30_000) instead of INSTALL_TIMEOUT_MS for the skills ls call; adjust either runSilentAsync to return both stdout and stderr or change skillsAlreadyInstalled to read stderr and combine with stdout, then stripAnsi(combined) and run the regex against that.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/install-wizard.js`:
- Around line 97-101: The current stripAnsi function uses a narrow CSI/OSC regex
that’s fine for stripping SGR color codes but doesn’t cover private-mode
markers, 8-bit CSI, or other sequences; either update the comment in stripAnsi
to explicitly document this limitation (mentioning it only targets SGR/tool
output) or replace the implementation with a broader, well-tested pattern by
importing the ansi-regex package and using it inside stripAnsi to ensure full
ANSI coverage; reference the stripAnsi function and its existing regex when
making the change.
- Around line 277-290: skillsAlreadyInstalled currently only checks stdout from
runSilentAsync and uses INSTALL_TIMEOUT_MS; update it to also capture and
inspect stderr (or use a combined output) so stripAnsi is applied to both
streams before testing with /^lark-/m, and reduce the timeout by using a small
constant (e.g. SKILLS_LIST_TIMEOUT_MS ~ 30_000) instead of INSTALL_TIMEOUT_MS
for the skills ls call; adjust either runSilentAsync to return both stdout and
stderr or change skillsAlreadyInstalled to read stderr and combine with stdout,
then stripAnsi(combined) and run the regex against that.
|
Closing; not ready to pursue this upstream right now. Will reopen later if needed. |
wsvn53
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #644 +/- ##
==========================================
+ Coverage 60.65% 62.49% +1.84%
==========================================
Files 416 416
Lines 43969 36299 -7670
==========================================
- Hits 26669 22686 -3983
+ Misses 15231 11544 -3687
Partials 2069 2069 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@20c92c71dc523d3dee5c05a59bf6e4afb8093a1e🧩 Skill updatenpx skills add wsvn53/cli#fix/install-wizard-slow-emulator -y -g |
2 participants
Withdrawn by author.