Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CI step that publishes and removes a serverless layer, bumps component Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/index.ts (1)
331-344:⚠️ Potential issue | 🔴 CriticalReturned path may not exist after forced
.zipsuffix.On Line [344], the function mutates the returned path string but does not ensure the downloaded file is written with that exact name. This can return a non-existent file path.
Proposed fix
- const parsedPathName = path.parse(urlPath).name; - const filename = path.basename(urlPath) || `downloaded_code_${Date.now()}`; + const parsed = path.parse(urlPath); + const baseName = parsed.name || `downloaded_code_${Date.now()}`; + const filename = parsed.ext ? parsed.base : `${baseName}.zip`; downloadPath = path.join(tempDir, filename); await downloads(url, { dest: tempDir, - filename: parsedPathName, + filename, extract: false, }); logger.debug(`Downloaded file to: ${downloadPath}`); - // 返回下载文件路径，由主流程决定是否需要压缩 - return downloadPath.endsWith('.zip') ? downloadPath : `${downloadPath}.zip`; + // 返回真实下载文件路径 + return downloadPath;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/index.ts` around lines 331 - 344, The code returns downloadPath with a forced .zip suffix which may not match the actual downloaded file name; update the download flow so the saved file and returned path always match: compute the desired saved filename (use parsedPathName or filename plus desired extension), pass that exact filename/path into downloads (or save into downloadPath directly) and, if needed, after downloads finishes rename/move the saved file to downloadPath (or create a zip at downloadPath) before returning; adjust references to parsedPathName, filename, downloadPath and the downloads call so the returned path points to an existing file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/e2e/ci-mac-linux.sh`:
- Around line 160-162: Replace the hardcoded layer name and add non-interactive
removal: generate a unique layer name (e.g., using timestamp or CI JOB_ID)
instead of the literal `--layer-name test` when calling `s layer publish` and
use that same generated name for `s layer remove`; also add the auto-confirm
flag `-y` to the `s layer remove` invocation so the CI won't wait for
interactive confirmation. Ensure the generated name is stored in a variable and
referenced in both `s layer publish` and `s layer remove -y`.
---
Outside diff comments:
In `@src/utils/index.ts`:
- Around line 331-344: The code returns downloadPath with a forced .zip suffix
which may not match the actual downloaded file name; update the download flow so
the saved file and returned path always match: compute the desired saved
filename (use parsedPathName or filename plus desired extension), pass that
exact filename/path into downloads (or save into downloadPath directly) and, if
needed, after downloads finishes rename/move the saved file to downloadPath (or
create a zip at downloadPath) before returning; adjust references to
parsedPathName, filename, downloadPath and the downloads call so the returned
path points to an existing file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3c496a4-78b5-412e-a6d0-9ab97a7cf257
📒 Files selected for processing (3)
__tests__/e2e/ci-mac-linux.shpublish.yamlsrc/utils/index.ts
__tests__/e2e/ci-mac-linux.sh
Outdated
| echo "test layer code support http" | ||
| s layer publish --layer-name test --code https://images.devsapp.cn/bin/supergateway --compatible-runtime custom.debian12 | ||
| s layer remove --layer-name test |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify remove commands that may be interactive
rg -nP --type=sh '\bs\s+layer\s+remove\b' __tests__/e2e/ci-mac-linux.sh
# Verify hard-coded layer names in CI
rg -nP --type=sh '--layer-name\s+test\b' __tests__/e2e/ci-mac-linux.shRepository: devsapp/fc3
Length of output: 532
🏁 Script executed:
# Get exact content at lines 160-162
sed -n '160,162p' __tests__/e2e/ci-mac-linux.sh
# Check if RANDSTR is defined in the script
grep -n "RANDSTR" __tests__/e2e/ci-mac-linux.sh
# Look for any layer remove commands with flags for context
grep -n "layer remove" __tests__/e2e/ci-mac-linux.shRepository: devsapp/fc3
Length of output: 1249
🏁 Script executed:
# Search for all layer remove commands across the codebase
rg "s layer remove" -A 1 -B 1
# Check if -y flag is used with layer remove elsewhere
rg "layer remove.*-y" --type=shRepository: devsapp/fc3
Length of output: 875
CI layer commands use hardcoded name and lack auto-confirmation flag.
Line 161 uses static --layer-name test, risking collisions with concurrent runs or leftovers. Line 162 omits -y, which can cause CI to hang waiting for interactive confirmation. Other test suites (command-api/run, run-windows.ps1) use dynamic layer names and include -y with layer remove.
Proposed fix
echo "test layer code support http"
-s layer publish --layer-name test --code https://images.devsapp.cn/bin/supergateway --compatible-runtime custom.debian12
-s layer remove --layer-name test
+layer_name="test-${RANDSTR}-${RANDOM}"
+s layer publish --layer-name "${layer_name}" --code https://images.devsapp.cn/bin/supergateway --compatible-runtime custom.debian12
+s layer remove --layer-name "${layer_name}" -y🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/e2e/ci-mac-linux.sh` around lines 160 - 162, Replace the hardcoded
layer name and add non-interactive removal: generate a unique layer name (e.g.,
using timestamp or CI JOB_ID) instead of the literal `--layer-name test` when
calling `s layer publish` and use that same generated name for `s layer remove`;
also add the auto-confirm flag `-y` to the `s layer remove` invocation so the CI
won't wait for interactive confirmation. Ensure the generated name is stored in
a variable and referenced in both `s layer publish` and `s layer remove -y`.
mozhou52
force-pushed
the
layer-publish
branch
3 times, most recently
from
0e926fe to
6a1c2b8
Compare
__tests__/e2e/ci-mac-linux.sh
Outdated
| s remove -y -t s2.yaml | ||
|
|
||
| echo "test layer code support http" | ||
| s layer publish --layer-name test --code https://test-gwh.oss-cn-hangzhou.aliyuncs.com/test.zip --compatible-runtime custom.debian12 |
There was a problem hiding this comment.
你这个放的位置太随意了， 使用 https://images.devsapp.cn/zip/devs_fc3_layer_test_url_test.zip
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/index.ts`:
- Around line 376-390: The file descriptor opened with const fd =
fs.openSync(filePath, 'r') can leak if fs.readSync throws; modify the function
that performs the open/read (the block using fd, fs.readSync, and fs.closeSync)
to ensure the descriptor is always closed by moving fs.closeSync into a finally
block (or use a try/finally around the read) — declare fd in a scope visible to
finally (e.g., let fd) and in finally check fd is defined before calling
fs.closeSync(fd); keep the existing catch logger.debug(...) behavior.
- Around line 341-349: The bug is a filename mismatch between parsedPathName and
filename: downloads() is called with parsedPathName (no extension) while
downloadPath uses filename (with extension), causing the file on disk to differ
from the path validated by isZipFile; fix by using path.basename(urlPath)
consistently for the filename passed into downloads() and when building
downloadPath so parsedPathName is not used for the actual download (update the
call to downloads() to use filename), and ensure the variables filename,
downloadPath, downloads(), and isZipFile() all reference the same basename with
extension.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62eff660-02ec-4687-b712-24031897fc46
📒 Files selected for processing (3)
__tests__/e2e/ci-mac-linux.shpublish.yamlsrc/utils/index.ts
✅ Files skipped from review due to trivial changes (1)
- publish.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/ci-mac-linux.sh
src/utils/index.ts
Outdated
| const downloadFilePath = downloadPath.endsWith('.zip') ? downloadPath : `${downloadPath}.zip`; | ||
| logger.debug(`Downloaded file to: ${downloadFilePath}`); | ||
|
|
||
| // 返回下载文件路径，由主流程决定是否需要压缩 | ||
| return downloadPath; | ||
| const isDownloadedFileZip = await isZipFile(downloadFilePath); | ||
| if (!isDownloadedFileZip) { | ||
| throw new Error(`Downloaded file is not a valid zip file: ${downloadFilePath}`); | ||
| } | ||
|
|
||
| return downloadFilePath; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In @serverless-devs/downloads, when filename is provided, is it used exactly as the local output filename, or can extension be auto-derived from URL/headers?
💡 Result:
When the filename option is provided to @serverless-devs/downloads, it is used exactly as the local output filename. The official npm package documentation lists filename as an optional parameter ("文件保存的名称 | string | 否 | demo.zip"), indicating it directly specifies the saved file name without mention of auto-deriving extensions from the URL or HTTP headers (e.g., Content-Disposition). The default is "demo.zip" when not provided, further suggesting exact usage as specified. No source code or usage examples indicate automatic extension derivation; it appears to be a direct filename override.
Citations:
- 1: https://registry.npmjs.org/%40serverless-devs%2Fdownloads
- 2: https://www.npmjs.com/package/@serverless-devs/downloads?activeTab=versions
🏁 Script executed:
cat -n src/utils/index.ts | sed -n '325,355p'Repository: devsapp/fc3
Length of output: 1329
Use consistent filename throughout: download and validation receive same path.
The code has a path mismatch bug. Line 331 extracts parsedPathName = path.parse(urlPath).name (no extension), while line 332 extracts filename = path.basename(urlPath) (with extension). Line 333 sets downloadPath = path.join(tempDir, filename), but line 337 passes filename: parsedPathName to downloads(). Since @serverless-devs/downloads uses the filename parameter exactly as specified, the file is written to disk without the extension, but the code then attempts to validate the extension-included path—causing a file-not-found or wrong-file-read error.
Use path.basename(urlPath) consistently for both the actual download filename and the validation path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/index.ts` around lines 341 - 349, The bug is a filename mismatch
between parsedPathName and filename: downloads() is called with parsedPathName
(no extension) while downloadPath uses filename (with extension), causing the
file on disk to differ from the path validated by isZipFile; fix by using
path.basename(urlPath) consistently for the filename passed into downloads() and
when building downloadPath so parsedPathName is not used for the actual download
(update the call to downloads() to use filename), and ensure the variables
filename, downloadPath, downloads(), and isZipFile() all reference the same
basename with extension.
2 participants
Summary by CodeRabbit
Bug Fixes
Chores
Tests