fix: import skills cli edge cases - #108
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 53 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCreates missing output parent directories before writing; refactors CLI help printing; adds GitHub archive ZIP candidate generation, deduplication, HTTP(S) validation, sequential download attempts (skip 404s, abort on other errors), and guaranteed temp-dir cleanup; expands exports and tests for these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "import-skills-url CLI"
participant Parser as "parseGithubRepoFromUrl"
participant Builder as "buildGithubArchiveZipCandidates"
participant Validator as "HTTP(s) Validator"
participant Downloader as "HTTP Downloader"
participant Importer as "Skill Importer"
User->>CLI: invoke with GitHub repo URL
CLI->>Parser: parse URL -> {owner,repo,ref}
Parser-->>CLI: repo descriptor
CLI->>Builder: build candidate ZIP URLs
Builder-->>CLI: candidates[]
CLI->>Validator: filter HTTP(s) & dedupe
Validator-->>CLI: validCandidates[]
loop for each candidate
CLI->>Downloader: download(candidate)
alt 404
Downloader-->>CLI: 404 -> continue
else non-404 error
Downloader-->>CLI: error -> abort flow
else success
Downloader-->>Importer: provide archive
Importer-->>CLI: import complete
end
end
CLI->>CLI: cleanup temp directory (finally)
CLI-->>User: exit with result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cli/import-skills-url.js (1)
36-68: Optional: extract shared GitHub URL parsing.
buildGithubArchiveZipCandidatesduplicates the parsing/validation block (protocol, hostname, owner/repo,.gitstripping) already present inresolveGithubArchiveZipUrl. Consider factoring out a small helper likeparseGithubRepoFromUrl(inputUrl)returning{ owner, repo, ref? }(ornull) and have both functions consume it. Defers any future fix (e.g., handling refs containing slashes such asfeature/x) to a single place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/import-skills-url.js` around lines 36 - 68, The parsing/validation logic in buildGithubArchiveZipCandidates duplicates code in resolveGithubArchiveZipUrl; extract a single helper (e.g., parseGithubRepoFromUrl) that accepts inputUrl and returns {owner, repo, ref?} or null, move protocol/hostname checks, .git stripping and pathname splitting into it, then update buildGithubArchiveZipCandidates and resolveGithubArchiveZipUrl to call parseGithubRepoFromUrl and build their candidate URLs from the returned owner/repo/ref; ensure ref (when present) is preserved (including possible slashes) and URL-encoded where used.tests/unit/import-skills-url.test.mjs (1)
26-34: Tests cover the main branches; consider one small addition.Coverage of the bare repo,
/tree/<ref>, and non-github paths is sufficient. As a nice-to-have, consider asserting parity behaviors that the helper inherits fromresolveGithubArchiveZipUrl(trailing slash,.gitsuffix, malformed input →[]) so a future refactor that consolidates the two parsers can't silently regress one.assert.deepEqual(buildGithubArchiveZipCandidates('https://github.com/foo/bar.git'), [ 'https://github.com/foo/bar/archive/refs/heads/main.zip', 'https://github.com/foo/bar/archive/refs/heads/master.zip' ]); assert.deepEqual(buildGithubArchiveZipCandidates('not a url'), []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/import-skills-url.test.mjs` around lines 26 - 34, Add assertions to tests for buildGithubArchiveZipCandidates to cover parity with resolveGithubArchiveZipUrl: verify inputs with a trailing .git suffix (e.g., 'https://github.com/foo/bar.git') return the same two candidate URLs, confirm a trailing slash variations are handled similarly, and assert malformed input (e.g., 'not a url') returns an empty array; update tests in tests/unit/import-skills-url.test.mjs near the existing buildGithubArchiveZipCandidates cases so future refactors keep behavior consistent with resolveGithubArchiveZipUrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/import-skills-url.js`:
- Around line 209-221: The parsing loop is capturing short flags like "-h" as
the URL because the URL-capture branch only excludes tokens starting with "--";
update the loop in the section that handles argv/token so that flag-style tokens
are detected before assigning options.url — either move the help/version checks
(token === '--help' || token === '-h' and similar) above the options.url
assignment or change the exclusion to reject any token that starts with '-'
(e.g., token.startsWith('-')) before setting options.url; ensure options.url is
only set when token is a non-flag value and keep subsequent isValidHttpUrl
checks unchanged.
---
Nitpick comments:
In `@cli/import-skills-url.js`:
- Around line 36-68: The parsing/validation logic in
buildGithubArchiveZipCandidates duplicates code in resolveGithubArchiveZipUrl;
extract a single helper (e.g., parseGithubRepoFromUrl) that accepts inputUrl and
returns {owner, repo, ref?} or null, move protocol/hostname checks, .git
stripping and pathname splitting into it, then update
buildGithubArchiveZipCandidates and resolveGithubArchiveZipUrl to call
parseGithubRepoFromUrl and build their candidate URLs from the returned
owner/repo/ref; ensure ref (when present) is preserved (including possible
slashes) and URL-encoded where used.
In `@tests/unit/import-skills-url.test.mjs`:
- Around line 26-34: Add assertions to tests for buildGithubArchiveZipCandidates
to cover parity with resolveGithubArchiveZipUrl: verify inputs with a trailing
.git suffix (e.g., 'https://github.com/foo/bar.git') return the same two
candidate URLs, confirm a trailing slash variations are handled similarly, and
assert malformed input (e.g., 'not a url') returns an empty array; update tests
in tests/unit/import-skills-url.test.mjs near the existing
buildGithubArchiveZipCandidates cases so future refactors keep behavior
consistent with resolveGithubArchiveZipUrl.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b41d152-2a81-4b31-b356-b1db2d29cb2a
📒 Files selected for processing (3)
cli.jscli/import-skills-url.jstests/unit/import-skills-url.test.mjs
📜 Review details
🔇 Additional comments (2)
cli.js (1)
6516-6517: Good guard before writing doctor output.Line 6516 correctly ensures the parent directory exists before file write, preventing
ENOENTon nested output paths.cli/import-skills-url.js (1)
265-317: Candidate iteration and cleanup look correct.Good handling: 404-only continues to next candidate, non-404 aborts immediately,
lastErroris rethrown if the loop exhausts without success, andfinallyguaranteestempDirremoval regardless of the path taken (including the help/early-error paths above, which exit beforetempDiris created). This matches the PR objective of cleanup-on-failure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cli/import-skills-url.js (1)
259-262: RedundantstartsWith('--')guard.After the parser tightening on Line 213 (
!token.startsWith('-')),options.urlcan never be a---prefixed token, sooptions.url.trim().startsWith('--')is unreachable. The!options.urlcheck alone is sufficient; consider dropping the second clause to avoid implying the parser still allows it.♻️ Suggested simplification
- if (!options.url || options.url.trim().startsWith('--')) { + if (!options.url) { printImportSkillsUsage(); throw new Error('错误: 缺少 URL（例如: https://github.com/<owner>/<repo>/archive/refs/heads/main.zip）'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/import-skills-url.js` around lines 259 - 262, The guard checking options.url.trim().startsWith('--') is redundant because the parser already prevents tokens starting with '-' (see the token check around Line 213), so simplify the validation in the import flow by removing the startsWith('--') clause and only validate with if (!options.url) then call printImportSkillsUsage() and throw the existing error; update the condition that references options.url and ensure printImportSkillsUsage() and the Error message remain unchanged.tests/unit/cli-help.test.mjs (1)
23-23: Strict empty-stderr assertion may be flaky.
assert.equal(result.stderr, '')will fail if Node emits any warning to stderr (e.g., ExperimentalWarning, deprecation notices) during CLI startup, even when the help path is correct. Consider asserting stderr does not contain error-shaped output instead, e.g.assert.doesNotMatch(result.stderr, /error/i), or run with--no-warnings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/cli-help.test.mjs` at line 23, The test's strict assertion assert.equal(result.stderr, '') is flaky because Node warnings can appear on stderr; update the test (in tests/unit/cli-help.test.mjs) to avoid exact-empty check by asserting stderr does not contain error-like output instead (e.g., replace assert.equal(result.stderr, '') with assert.doesNotMatch(result.stderr, /error/i) or a similar regex check against /error|exception/i), or alternatively launch the CLI with NODE_OPTIONS='--no-warnings' (or include '--no-warnings' in the spawn args) so result.stderr is not polluted by runtime warnings; target the assertion using the existing result.stderr variable to implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/import-skills-url.js`:
- Around line 41-64: The bug is that resolveGithubArchiveZipUrl and
buildGithubArchiveZipCandidates call encodeURIComponent on the whole ref (e.g.,
"release/v1.0"), which turns "/" into "%2F" and breaks GitHub archive URLs; fix
by splitting the ref into path segments, encodeURIComponent each segment, then
rejoin with "/" before interpolating into the `${base}/heads/...` or
`${base}/tags/...` URLs (use the ref value produced by parseGithubRepoFromUrl
but encode per-segment); also update tests that assert tree/<ref> handling to
expect per-segment encoding for multi-part refs.
---
Nitpick comments:
In `@cli/import-skills-url.js`:
- Around line 259-262: The guard checking options.url.trim().startsWith('--') is
redundant because the parser already prevents tokens starting with '-' (see the
token check around Line 213), so simplify the validation in the import flow by
removing the startsWith('--') clause and only validate with if (!options.url)
then call printImportSkillsUsage() and throw the existing error; update the
condition that references options.url and ensure printImportSkillsUsage() and
the Error message remain unchanged.
In `@tests/unit/cli-help.test.mjs`:
- Line 23: The test's strict assertion assert.equal(result.stderr, '') is flaky
because Node warnings can appear on stderr; update the test (in
tests/unit/cli-help.test.mjs) to avoid exact-empty check by asserting stderr
does not contain error-like output instead (e.g., replace
assert.equal(result.stderr, '') with assert.doesNotMatch(result.stderr,
/error/i) or a similar regex check against /error|exception/i), or alternatively
launch the CLI with NODE_OPTIONS='--no-warnings' (or include '--no-warnings' in
the spawn args) so result.stderr is not polluted by runtime warnings; target the
assertion using the existing result.stderr variable to implement the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e43056b-9752-481f-a80e-153d5f544ac1
📒 Files selected for processing (5)
cli.jscli/import-skills-url.jstests/unit/cli-help.test.mjstests/unit/import-skills-url.test.mjstests/unit/run.mjs
✅ Files skipped from review due to trivial changes (1)
- tests/unit/run.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/import-skills-url.test.mjs
📜 Review details
🔇 Additional comments (6)
cli/import-skills-url.js (2)
205-217: Help/-hhandling — past finding addressed.The help check now runs before URL capture, and the URL branch rejects any
--prefixed token, socodexmate import-skills -hno longer gets swallowed as the URL. Matches the suggestion from the prior review.
273-311: Cleanup + sequential candidate flow look solid.
finallyguaranteestempDirremoval on every path, the 404-only continuation matches the documented intent, andfinalUrlis captured per-iteration so the eventualimportSkillsFromZipFileuses the candidate that actually downloaded. No partial-zip leakage between attempts becausedownloadUrlToFilerejects before opening the write stream on non-2xx responses.cli.js (3)
6516-6517: Nice fix fordoctor --outputpath handling.Creating the parent directory before writing makes nested output paths reliable and prevents avoidable write failures.
13166-13195: Help output extraction is clean and maintainable.Moving main help text into
printMainHelp()improves reuse and keeps command help behavior centralized.
13212-13214: Help dispatch behavior is now consistent.Routing both empty args and help flags through one path is straightforward and gives predictable CLI UX with successful exit.
tests/unit/cli-help.test.mjs (1)
17-17: Thetestglobal is available in this file—no import is needed. The project uses a custom test runner (tests/unit/run.mjs) that sets upglobalThis.testbefore dynamically importing all test files, including this one. This is a valid pattern and works correctly as-is.> Likely an incorrect or invalid review comment.
2 participants
Summary
Tests
Summary by CodeRabbit
New Features
Bug Fixes
Tests