fix: layer publish support code is http url#143
Closed
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:
📝 WalkthroughWalkthroughExtracted URL-download logic into a shared Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI/Command"
participant Layer as "Layer.publish()"
participant Deploy as "Deploy/function flow"
participant Utils as "utils._downloadFromUrl"
participant FS as "Filesystem (temp dirs)"
CLI->>Layer: publish(codeUri)
alt codeUri is http(s)
Layer->>Utils: _downloadFromUrl(codeUri)
Utils->>FS: create temp dir & download file
Utils-->>Layer: local downloadedTempFile
Layer->>Layer: safe_publish_layer(..., downloadedTempFile)
Layer->>FS: remove temp zip & downloadedTempFile
else non-URL
Layer->>Layer: proceed with local path
end
CLI->>Deploy: deploy/function(codeUri)
alt codeUri is http(s)
Deploy->>Utils: _downloadFromUrl(codeUri)
Utils->>FS: create temp dir & download file
Utils-->>Deploy: local downloadedTempFile
Deploy->>FS: remove downloadedTempFile on skip/after upload
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 (3)
src/subCommands/layer/index.ts (1)
34-100:⚠️ Potential issue | 🟠 MajorUse
try/finallyinsafe_publish_layerfor guaranteed temp cleanup.
downloadedTempDirand generated zip cleanup run only on success/skip branches. If an exception is thrown during checksum, OSS upload, or create-layer call, temp artifacts are left behind.Suggested fix
static async safe_publish_layer( @@ - let zipPath = toZipDir; - let generateZipFilePath = ''; - if (!toZipDir.endsWith('.zip')) { + let zipPath = toZipDir; + let generateZipFilePath = ''; + try { + if (!toZipDir.endsWith('.zip')) { const zipConfig = { @@ - generateZipFilePath = (await zip(zipConfig)).outputFile; - zipPath = generateZipFilePath; - } - const latestLayer = await fcSdk.getLayerLatestVersion(layerName); - if (latestLayer) { + generateZipFilePath = (await zip(zipConfig)).outputFile; + zipPath = generateZipFilePath; + } + const latestLayer = await fcSdk.getLayerLatestVersion(layerName); + if (latestLayer) { @@ - return latestLayer; + return latestLayer; + } } - } - logger.debug(`Zip file: ${zipPath}`); - getFileSize(zipPath); - const ossConfig = await fcSdk.uploadCodeToTmpOss(zipPath); - logger.debug('ossConfig: ', ossConfig); + logger.debug(`Zip file: ${zipPath}`); + getFileSize(zipPath); + const ossConfig = await fcSdk.uploadCodeToTmpOss(zipPath); + logger.debug('ossConfig: ', ossConfig); @@ - console.log(JSON.stringify(result)); - return result; + console.log(JSON.stringify(result)); + return result; + } finally { + if (generateZipFilePath) { + try { + fs.rmSync(generateZipFilePath); + } catch (ex) { + logger.debug(`Unable to remove zip file: ${zipPath}`); + } + } + if (downloadedTempDir) { + try { + logger.debug(`Removing temp download dir: ${downloadedTempDir}`); + fs.rmSync(downloadedTempDir, { recursive: true, force: true }); + } catch (ex) { + logger.debug(`Unable to remove temp download dir: ${downloadedTempDir}`); + } + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/layer/index.ts` around lines 34 - 100, The temp artifacts (downloadedTempDir and generateZipFilePath) are only deleted on the normal/skip paths, leaving leftovers on exceptions; update the function (safe_publish_layer / the block using zipPath, generateZipFilePath, downloadedTempDir, calculateCRC64, uploadCodeToTmpOss, createLayerVersion) to wrap the main work in a try block and move all cleanup logic into a finally block that always attempts to fs.rmSync(generateZipFilePath) and fs.rmSync(downloadedTempDir, { recursive: true, force: true }), logging any removal errors with logger.debug; keep existing conditional checks so you only remove files that were actually created and preserve existing debug messages in the finally.src/utils/index.ts (1)
301-304:⚠️ Potential issue | 🟠 MajorPrefix validation in
checkFcDiris over-broad and rejects valid paths.Using
startsWith(prefix)blocks unrelated paths like/various/...when checking/var. Match exact prefix directory boundaries instead.Suggested fix
for (const prefix of forbiddenPrefixes) { - if (normalizedPath.startsWith(prefix)) { + if (normalizedPath === prefix || normalizedPath.startsWith(`${prefix}/`)) { throw new Error(`Invalid ${paramName}, ${prefix} and its subdirectories are not allowed`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/index.ts` around lines 301 - 304, The check in checkFcDir currently uses normalizedPath.startsWith(prefix) which falsely rejects paths that only share a leading segment (e.g., "/various" vs "/var"); update the logic to only reject when the normalizedPath equals the forbidden prefix or lies inside it by checking boundary-aware matches: i.e., treat a forbidden prefix as matched only if normalizedPath === prefix or normalizedPath.startsWith(prefix + path.sep) (use the appropriate path.sep/posix separator for your environment) for each entry in forbiddenPrefixes; update any references to normalizedPath and forbiddenPrefixes in checkFcDir accordingly.src/subCommands/deploy/impl/function.ts (1)
286-360:⚠️ Potential issue | 🟠 MajorMake temp file cleanup exception-safe with
finally.
downloadedTempDircleanup currently runs only on normal branches. If an exception occurs after download (for example at Line 341), the temp dir can leak.Suggested fix
private async _uploadCode(): Promise<boolean> { @@ - let generateZipFilePath = ''; - if (needZip) { + let generateZipFilePath = ''; + try { + if (needZip) { setNodeModulesBinPermissions(zipPath); @@ - zipPath = generateZipFilePath; - } + zipPath = generateZipFilePath; + } @@ - const ossConfig = await this.fcSdk.uploadCodeToTmpOss(zipPath); - logger.debug('ossConfig: ', ossConfig); - _.set(this.local, 'code', ossConfig); - - if (generateZipFilePath) { - try { - fs.rmSync(generateZipFilePath); - } catch (ex) { - logger.debug(`Unable to remove zip file: ${zipPath}`); - } - } - - if (downloadedTempDir) { - try { - logger.debug(`Removing temp download dir: ${downloadedTempDir}`); - fs.rmSync(downloadedTempDir, { recursive: true, force: true }); - } catch (ex) { - logger.debug(`Unable to remove temp download dir: ${downloadedTempDir}`); - } - } - - return true; + const ossConfig = await this.fcSdk.uploadCodeToTmpOss(zipPath); + logger.debug('ossConfig: ', ossConfig); + _.set(this.local, 'code', ossConfig); + return true; + } finally { + if (generateZipFilePath) { + try { + fs.rmSync(generateZipFilePath); + } catch (ex) { + logger.debug(`Unable to remove zip file: ${zipPath}`); + } + } + if (downloadedTempDir) { + try { + logger.debug(`Removing temp download dir: ${downloadedTempDir}`); + fs.rmSync(downloadedTempDir, { recursive: true, force: true }); + } catch (ex) { + logger.debug(`Unable to remove temp download dir: ${downloadedTempDir}`); + } + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` around lines 286 - 360, The temp download and generated zip cleanup can leak if an exception occurs; move the deletion of downloadedTempDir and generateZipFilePath into a finally block so they always run after processing (wrap the code between obtaining zipPath and the return/oss upload in try { ... } finally { try { fs.rmSync(generateZipFilePath) } catch{}; try { fs.rmSync(downloadedTempDir, { recursive: true, force: true }) } catch{} }). Update the block that calls calculateCRC64, this.fcSdk.uploadCodeToTmpOss, and _.set(this.local, 'code', ossConfig) to be inside the try, and ensure you only attempt to remove generateZipFilePath when it was created and not remove zipPath if it pointed to a local codeUri; reference variables downloadedTempDir, generateZipFilePath, zipPath, calculateCRC64, and uploadCodeToTmpOss when making the changes.
🤖 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 322-323: The current fixed temp directory (const tempDir =
path.join(os.tmpdir(), 'fc_code_download')) in _downloadFromUrl risks
collisions; replace it with a unique per-invocation temp directory (e.g., create
via fs.mkdtemp or fs.promises.mkdtemp using os.tmpdir() + 'fc_code_download-')
and use that new directory as the working downloadPath so each call gets its own
folder; ensure cleanup only removes the specific per-call directory (not a
shared parent) and update any logic that assumes a single shared tempDir.
---
Outside diff comments:
In `@src/subCommands/deploy/impl/function.ts`:
- Around line 286-360: The temp download and generated zip cleanup can leak if
an exception occurs; move the deletion of downloadedTempDir and
generateZipFilePath into a finally block so they always run after processing
(wrap the code between obtaining zipPath and the return/oss upload in try { ...
} finally { try { fs.rmSync(generateZipFilePath) } catch{}; try {
fs.rmSync(downloadedTempDir, { recursive: true, force: true }) } catch{} }).
Update the block that calls calculateCRC64, this.fcSdk.uploadCodeToTmpOss, and
_.set(this.local, 'code', ossConfig) to be inside the try, and ensure you only
attempt to remove generateZipFilePath when it was created and not remove zipPath
if it pointed to a local codeUri; reference variables downloadedTempDir,
generateZipFilePath, zipPath, calculateCRC64, and uploadCodeToTmpOss when making
the changes.
In `@src/subCommands/layer/index.ts`:
- Around line 34-100: The temp artifacts (downloadedTempDir and
generateZipFilePath) are only deleted on the normal/skip paths, leaving
leftovers on exceptions; update the function (safe_publish_layer / the block
using zipPath, generateZipFilePath, downloadedTempDir, calculateCRC64,
uploadCodeToTmpOss, createLayerVersion) to wrap the main work in a try block and
move all cleanup logic into a finally block that always attempts to
fs.rmSync(generateZipFilePath) and fs.rmSync(downloadedTempDir, { recursive:
true, force: true }), logging any removal errors with logger.debug; keep
existing conditional checks so you only remove files that were actually created
and preserve existing debug messages in the finally.
In `@src/utils/index.ts`:
- Around line 301-304: The check in checkFcDir currently uses
normalizedPath.startsWith(prefix) which falsely rejects paths that only share a
leading segment (e.g., "/various" vs "/var"); update the logic to only reject
when the normalizedPath equals the forbidden prefix or lies inside it by
checking boundary-aware matches: i.e., treat a forbidden prefix as matched only
if normalizedPath === prefix or normalizedPath.startsWith(prefix + path.sep)
(use the appropriate path.sep/posix separator for your environment) for each
entry in forbiddenPrefixes; update any references to normalizedPath and
forbiddenPrefixes in checkFcDir accordingly.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72143392-0abe-4488-9c7f-04c710e13482
📒 Files selected for processing (5)
__tests__/ut/commands/invoke_test.ts__tests__/ut/utils/utils_test.tssrc/subCommands/deploy/impl/function.tssrc/subCommands/layer/index.tssrc/utils/index.ts
rsonghuster
reviewed
Mar 4, 2026
| logger.debug( | ||
| yellow(`skip uploadCode because code is no changed, codeChecksum=${crc64Value}`), | ||
| ); | ||
| if (downloadedTempDir) { |
Contributor
There was a problem hiding this comment.
为什么是删除那个文件夹， 不是那个 zip 文件
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/subCommands/layer/index.ts (1)
52-97:⚠️ Potential issue | 🟠 MajorCleanup is not guaranteed on failures and some early returns.
downloadedTempDir/generated zip cleanup only runs in selected branches. If upload/create throws, temp artifacts are left behind. Also, checksum-skip return does not cleangenerateZipFilePath.Suggested refactor (single cleanup point)
static async safe_publish_layer(...) { let zipPath = toZipDir; let generateZipFilePath = ''; - if (!toZipDir.endsWith('.zip')) { + try { + if (!toZipDir.endsWith('.zip')) { ... - } - const latestLayer = await fcSdk.getLayerLatestVersion(layerName); - if (latestLayer) { + } + const latestLayer = await fcSdk.getLayerLatestVersion(layerName); + if (latestLayer) { ... - if (latestLayer.codeChecksum === crc64Value) { + if (latestLayer.codeChecksum === crc64Value) { ... - if (downloadedTempDir) { - ... - } - return latestLayer; + return latestLayer; + } } - } - ... - const result = await fcSdk.createLayerVersion(...); - ... - if (generateZipFilePath) { ... } - if (downloadedTempDir) { ... } - console.log(JSON.stringify(result)); - return result; + ... + const result = await fcSdk.createLayerVersion(...); + console.log(JSON.stringify(result)); + return result; + } finally { + if (generateZipFilePath) { + try { fs.rmSync(generateZipFilePath); } catch (ex) { logger.debug(`Unable to remove zip file: ${zipPath}`); } + } + if (downloadedTempDir) { + try { fs.rmSync(downloadedTempDir, { recursive: true, force: true }); } catch (ex) { logger.debug(`Unable to remove temp download dir: ${downloadedTempDir}`); } + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/layer/index.ts` around lines 52 - 97, The code leaves temp artifacts when errors or early-returns occur (e.g., the checksum-skip branch and failures in fcSdk.uploadCodeToTmpOss / fcSdk.createLayerVersion); refactor the flow in src/subCommands/layer/index.ts so all temporary cleanup is done in one place: wrap the main work (checksum check, getFileSize, fcSdk.uploadCodeToTmpOss, fcSdk.createLayerVersion) in a try block and perform removal of generateZipFilePath and downloadedTempDir in a single finally block (or extract a cleanup function and call it from finally), ensuring the checksum early-return does not bypass cleanup by returning after the finally (e.g., set latestLayer/skip flag and return after finally), and keep the existing guarded fs.rmSync(... { recursive:true, force:true }) calls with try/catch and logger.debug for any removal errors.
🤖 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`:
- Line 319: The current logger.info call logs the full download URL
(logger.info(`Downloading code from URL: ${url}`)), which can leak sensitive
query parameters; update the logging to redact query strings by parsing the url
variable with the URL class and logging only the origin + pathname (e.g., new
URL(url).origin + new URL(url).pathname) or otherwise strip/redact url.search
before passing to logger.info so only non-sensitive parts are emitted.
---
Outside diff comments:
In `@src/subCommands/layer/index.ts`:
- Around line 52-97: The code leaves temp artifacts when errors or early-returns
occur (e.g., the checksum-skip branch and failures in fcSdk.uploadCodeToTmpOss /
fcSdk.createLayerVersion); refactor the flow in src/subCommands/layer/index.ts
so all temporary cleanup is done in one place: wrap the main work (checksum
check, getFileSize, fcSdk.uploadCodeToTmpOss, fcSdk.createLayerVersion) in a try
block and perform removal of generateZipFilePath and downloadedTempDir in a
single finally block (or extract a cleanup function and call it from finally),
ensuring the checksum early-return does not bypass cleanup by returning after
the finally (e.g., set latestLayer/skip flag and return after finally), and keep
the existing guarded fs.rmSync(... { recursive:true, force:true }) calls with
try/catch and logger.debug for any removal errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f23cd7fb-32fc-47ab-bdae-69d0d1d0d303
📒 Files selected for processing (5)
__tests__/ut/commands/invoke_test.ts__tests__/ut/utils/utils_test.tssrc/subCommands/deploy/impl/function.tssrc/subCommands/layer/index.tssrc/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/ut/utils/utils_test.ts
- tests/ut/commands/invoke_test.ts
- src/subCommands/deploy/impl/function.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utils/index.ts (1)
320-321:⚠️ Potential issue | 🟠 MajorUse a per-invocation temp directory to avoid collisions.
A fixed
fc_code_downloadfolder can race across parallel runs and cross-cleanup each other.Suggested fix
- const tempDir = path.join(os.tmpdir(), 'fc_code_download'); + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'fc_code_download-'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/index.ts` around lines 320 - 321, The temp directory is currently a fixed path (tempDir = path.join(os.tmpdir(), 'fc_code_download')) which can collide across concurrent runs; change it to create a per-invocation temp dir (e.g. use fs.mkdtemp or fs.mkdtempSync with a prefix like path.join(os.tmpdir(), 'fc_code_download-')) and assign that result to tempDir/downloadPath so each invocation gets a unique folder; update any code that references tempDir or downloadPath accordingly and keep existing cleanup logic to remove the created temp dir when finished.
🤖 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 326-333: The download filename sent to downloads() is missing the
extension (parsedPathName uses path.parse(...).name) causing downloadPath to
mismatch the actual file; fix by using the full basename (use
path.parse(...).base or path.basename(urlPath)) when constructing the name
passed to downloads() and ensure downloadPath is built using that same full
filename variable (update parsedPathName/filename usage so downloads(url, {
filename: fullFilename }) and downloadPath = path.join(tempDir, fullFilename)
are consistent).
---
Duplicate comments:
In `@src/utils/index.ts`:
- Around line 320-321: The temp directory is currently a fixed path (tempDir =
path.join(os.tmpdir(), 'fc_code_download')) which can collide across concurrent
runs; change it to create a per-invocation temp dir (e.g. use fs.mkdtemp or
fs.mkdtempSync with a prefix like path.join(os.tmpdir(), 'fc_code_download-'))
and assign that result to tempDir/downloadPath so each invocation gets a unique
folder; update any code that references tempDir or downloadPath accordingly and
keep existing cleanup logic to remove the created temp dir when finished.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a7d0287-b6d2-4a4f-a504-86adb154e7a8
📒 Files selected for processing (5)
__tests__/ut/commands/invoke_test.ts__tests__/ut/utils/utils_test.tssrc/subCommands/deploy/impl/function.tssrc/subCommands/layer/index.tssrc/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/ut/utils/utils_test.ts
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/subCommands/deploy/impl/function.ts (1)
323-359:⚠️ Potential issue | 🟠 MajorMove temp-artifact cleanup into a
finally.This still leaks files in two cases: the early
return falsepath never removesgenerateZipFilePath, and any exception after download/zip creation skips both cleanup blocks entirely. Repeated failed or no-op deploys will keep accumulating temp artifacts.Suggested fix
- const crc64Value = await calculateCRC64(zipPath); - logger.debug(`code zip crc64=${crc64Value}; codeChecksum=${this.codeChecksum}`); - if (this.codeChecksum) { - if (this.codeChecksum === crc64Value) { - logger.debug( - yellow(`skip uploadCode because code is no changed, codeChecksum=${crc64Value}`), - ); - if (downloadedTempFile) { - try { - logger.debug(`Removing temp download dir: ${downloadedTempFile}`); - fs.rmSync(downloadedTempFile, { recursive: true, force: true }); - } catch (ex) { - logger.debug(`Unable to remove temp download dir: ${downloadedTempFile}`); - } - } - return false; - } else { - logger.debug(`\x1b[33mcodeChecksum from ${this.codeChecksum} to ${crc64Value}\x1b[0m`); - } - } - const ossConfig = await this.fcSdk.uploadCodeToTmpOss(zipPath); - logger.debug('ossConfig: ', ossConfig); - _.set(this.local, 'code', ossConfig); - - if (generateZipFilePath) { - try { - fs.rmSync(generateZipFilePath); - } catch (ex) { - logger.debug(`Unable to remove zip file: ${zipPath}`); - } - } - - if (downloadedTempFile) { - try { - logger.debug(`Removing temp download dir: ${downloadedTempFile}`); - fs.rmSync(downloadedTempFile, { recursive: true, force: true }); - } catch (ex) { - logger.debug(`Unable to remove temp download dir: ${downloadedTempFile}`); - } - } - - return true; + try { + const crc64Value = await calculateCRC64(zipPath); + logger.debug(`code zip crc64=${crc64Value}; codeChecksum=${this.codeChecksum}`); + if (this.codeChecksum) { + if (this.codeChecksum === crc64Value) { + logger.debug( + yellow(`skip uploadCode because code is no changed, codeChecksum=${crc64Value}`), + ); + return false; + } + logger.debug(`\x1b[33mcodeChecksum from ${this.codeChecksum} to ${crc64Value}\x1b[0m`); + } + + const ossConfig = await this.fcSdk.uploadCodeToTmpOss(zipPath); + logger.debug('ossConfig: ', ossConfig); + _.set(this.local, 'code', ossConfig); + return true; + } finally { + for (const tempPath of [generateZipFilePath, downloadedTempFile]) { + if (!tempPath) continue; + try { + fs.rmSync(tempPath, { recursive: true, force: true }); + } catch (ex) { + logger.debug(`Unable to remove temp artifact: ${tempPath}`); + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` around lines 323 - 359, The temp-artifact cleanup must be moved into a finally block so generateZipFilePath and downloadedTempFile are always removed (including the early return when this.codeChecksum === crc64Value and on any exception). Refactor the section around the crc64 comparison and the call to this.fcSdk.uploadCodeToTmpOss(zipPath): perform checksum check but defer all removals of generateZipFilePath and downloadedTempFile to a single finally block that runs before returning (so the early return returns after cleanup), and eliminate the duplicate cleanup blocks; keep the _.set(this.local, 'code', ossConfig) assignment inside the try so upload errors propagate but cleanup still runs.
🤖 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/subCommands/model/model.ts`:
- Around line 120-122: The NAS branch dereferences nasMountPoints[0] without
consistently guarding its existence: ensure any access to nasMountPoints[0] is
protected by a check that nasMountPoints exists and has at least one entry
(e.g., nasMountPoints && nasMountPoints.length > 0 or nasMountPoints?.[0])
before using nasPath, filepath or other properties; update the conditional that
currently reads storage === 'nas' && nasMountPoints[0] ... to include the
nasMountPoints guard, and when using filepath later, either use optional
chaining (nasMountPoints?.[0]?.filepath) or assign const mount =
nasMountPoints[0] after the guard and reference mount.filepath to avoid crashes
when mount points are missing.
---
Outside diff comments:
In `@src/subCommands/deploy/impl/function.ts`:
- Around line 323-359: The temp-artifact cleanup must be moved into a finally
block so generateZipFilePath and downloadedTempFile are always removed
(including the early return when this.codeChecksum === crc64Value and on any
exception). Refactor the section around the crc64 comparison and the call to
this.fcSdk.uploadCodeToTmpOss(zipPath): perform checksum check but defer all
removals of generateZipFilePath and downloadedTempFile to a single finally block
that runs before returning (so the early return returns after cleanup), and
eliminate the duplicate cleanup blocks; keep the _.set(this.local, 'code',
ossConfig) assignment inside the try so upload errors propagate but cleanup
still runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e908f691-2483-4fd4-abf3-165f75ba7058
📒 Files selected for processing (7)
__tests__/ut/commands/invoke_test.ts__tests__/ut/utils/utils_test.tspackage.jsonsrc/subCommands/deploy/impl/function.tssrc/subCommands/layer/index.tssrc/subCommands/model/model.tssrc/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/subCommands/layer/index.ts
- tests/ut/commands/invoke_test.ts
- src/utils/index.ts
- tests/ut/utils/utils_test.ts
mozhou52
force-pushed
the
support-http
branch
7 times, most recently
from
3832ccb to
f4d27a4
Compare
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)
src/utils/index.ts (1)
267-309:⚠️ Potential issue | 🟠 MajorGuard and normalize
inputPathbefore validating it.
_deployAuto()passesparams.mountDirfromparseAutoConfig()straight into this helper, soinputPath.trim()can throw a rawTypeErrorwhen the value deserializes to a boolean/number. Even for strings, validating the unnormalized path lets/mnt/../var/logthrough and wrongly rejects sibling names like/various/app.Suggested fix
export function checkFcDir(inputPath: string, paramName = 'path'): string { - const normalizedPath = inputPath.trim(); + if (typeof inputPath !== 'string') { + throw new Error(`${paramName} must be a string`); + } + const normalizedPath = path.posix.normalize(inputPath.trim()); @@ for (const prefix of forbiddenPrefixes) { - if (normalizedPath.startsWith(prefix)) { + if (normalizedPath === prefix || normalizedPath.startsWith(`${prefix}/`)) { throw new Error(`Invalid ${paramName}, ${prefix} and its subdirectories are not allowed`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/index.ts` around lines 267 - 309, Guard and normalize the input before validating in checkFcDir: validate that inputPath is not null/undefined and coerce it to a string (e.g., String(inputPath)) then trim; use a POSIX path normalization (path.posix.normalize) to resolve "."/".." and collapse duplicate slashes while ensuring the normalized result still begins with "/" (prepend "/" if normalization removed it for inputs like ""). After normalization apply length checks and replace the current forbiddenPrefixes test so it only rejects exact prefix matches or true subdirectories (use equality or startsWith(prefix + "/") rather than startsWith(prefix) to avoid rejecting "/various"); update references to forbiddenPrefixes and normalizedPath in the function accordingly.
♻️ Duplicate comments (2)
src/utils/index.ts (2)
325-333:⚠️ Potential issue | 🔴 CriticalReturn the file that
downloads()actually writes.Line 326 strips the extension before passing the name to
downloads()at Line 332, whiledownloadPathstill points to the full basename. Forcode.zip, this helper returns.../code.zipbut writes.../code, so the HTTP deploy/layer flow will look up a file that does not exist.Suggested fix
- const parsedPathName = path.parse(urlPath).name; const filename = path.basename(urlPath) || `downloaded_code_${Date.now()}`; downloadPath = path.join(tempDir, filename); await downloads(url, { dest: tempDir, - filename: parsedPathName, + filename, extract: false, });Find the official README/docs for `@serverless-devs/downloads` 0.0.7 and confirm whether the `filename` option sets the exact saved filename on disk, including its extension.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/index.ts` around lines 325 - 333, The helper currently strips the extension when calling downloads() by using parsedPathName but sets downloadPath using the full basename, causing a mismatch; update the call-site in this function so the filename passed to downloads() matches the actual saved filename (use filename (basename with extension) instead of parsedPathName) or, if downloads() always strips/changes extensions per its docs, compute downloadPath from the name downloads() will write (e.g., derive from parsedPathName) after confirming behavior from `@serverless-devs/downloads` 0.0.7 README; adjust the downloads(...) invocation and the downloadPath assignment so both reference the same final filename (identify symbols: urlPath, parsedPathName, filename, downloadPath, and downloads()).
320-320:⚠️ Potential issue | 🟠 MajorDon't reuse one global temp directory for every download.
fc_code_downloadis still shared across all invocations, and the catch path deletes that whole directory. Parallel deploy/layer publishes can clobber each other here.Also applies to: 340-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/index.ts` at line 320, The temp directory is currently a single global path stored in tempDir (path.join(os.tmpdir(), 'fc_code_download')) which causes concurrent runs to collide and the catch-path cleanup to delete others' data; change creation to a unique per-invocation directory (e.g., use fs.mkdtemp or append a UUID/timestamp/pid to the base name) so each call gets its own folder, ensure any cleanup logic in the catch block and functions that reference tempDir only remove that specific per-invocation directory, and update all references (the tempDir variable and the cleanup block around lines 340-344) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/utils/index.ts`:
- Around line 267-309: Guard and normalize the input before validating in
checkFcDir: validate that inputPath is not null/undefined and coerce it to a
string (e.g., String(inputPath)) then trim; use a POSIX path normalization
(path.posix.normalize) to resolve "."/".." and collapse duplicate slashes while
ensuring the normalized result still begins with "/" (prepend "/" if
normalization removed it for inputs like ""). After normalization apply length
checks and replace the current forbiddenPrefixes test so it only rejects exact
prefix matches or true subdirectories (use equality or startsWith(prefix + "/")
rather than startsWith(prefix) to avoid rejecting "/various"); update references
to forbiddenPrefixes and normalizedPath in the function accordingly.
---
Duplicate comments:
In `@src/utils/index.ts`:
- Around line 325-333: The helper currently strips the extension when calling
downloads() by using parsedPathName but sets downloadPath using the full
basename, causing a mismatch; update the call-site in this function so the
filename passed to downloads() matches the actual saved filename (use filename
(basename with extension) instead of parsedPathName) or, if downloads() always
strips/changes extensions per its docs, compute downloadPath from the name
downloads() will write (e.g., derive from parsedPathName) after confirming
behavior from `@serverless-devs/downloads` 0.0.7 README; adjust the downloads(...)
invocation and the downloadPath assignment so both reference the same final
filename (identify symbols: urlPath, parsedPathName, filename, downloadPath, and
downloads()).
- Line 320: The temp directory is currently a single global path stored in
tempDir (path.join(os.tmpdir(), 'fc_code_download')) which causes concurrent
runs to collide and the catch-path cleanup to delete others' data; change
creation to a unique per-invocation directory (e.g., use fs.mkdtemp or append a
UUID/timestamp/pid to the base name) so each call gets its own folder, ensure
any cleanup logic in the catch block and functions that reference tempDir only
remove that specific per-invocation directory, and update all references (the
tempDir variable and the cleanup block around lines 340-344) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c698e1b7-a0f2-40ac-945b-6bb66303ed20
📒 Files selected for processing (7)
__tests__/ut/commands/invoke_test.ts__tests__/ut/utils/utils_test.tspackage.jsonsrc/subCommands/deploy/impl/function.tssrc/subCommands/layer/index.tssrc/subCommands/model/model.tssrc/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/subCommands/model/model.ts
- tests/ut/commands/invoke_test.ts
- tests/ut/utils/utils_test.ts
mozhou52
force-pushed
the
support-http
branch
2 times, most recently
from
c3425b2 to
a91c9e0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Tests
Bug Fixes
Refactor
Chores