Open
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 47 minutes and 50 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: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds OSS and Polar FS mount support and new session controls ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as "User CLI"
participant Subcmd as "Session subcommand"
participant Resource as "resources/fc.createFunctionSession"
participant SDK as "FC SDK"
participant FC as "Function Compute Service"
CLI->>Subcmd: invoke with flags (session-id, --disable-session-id-reuse, --oss-mount-config, --polar-fs-config)
Subcmd->>Subcmd: parse flags, JSON.parse storage configs (fallback to string)
Subcmd->>Resource: call createFunctionSession(config with nas/oss/polar, sessionId, disableSessionIdReuse)
Resource->>SDK: build CreateSessionInput and call SDK.createSession(...)
SDK->>FC: HTTP request to create session
FC-->>SDK: response (sessionId, details)
SDK-->>Resource: return SDK response
Resource-->>Subcmd: return created session info
Subcmd-->>CLI: print JSON result (includes .sessionId)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
🧹 Nitpick comments (1)
src/subCommands/session/index.ts (1)
82-95: Silent failure on invalid JSON could confuse users.When
JSON.parsefails, the raw string is stored and silently ignored downstream (the mount config condition infc/index.tswill evaluate to false). This replicates the existingnasConfigpattern (lines 75-80), but users won't know their config was malformed.Consider logging a warning when JSON parsing fails:
♻️ Suggested improvement
if (opts['oss-mount-config']) { try { this.ossMountConfig = JSON.parse(opts['oss-mount-config']); } catch (error) { + logger.warn(`Failed to parse --oss-mount-config as JSON, will be ignored: ${error.message}`); this.ossMountConfig = opts['oss-mount-config']; } } if (opts['polar-fs-config']) { try { this.polarFsConfig = JSON.parse(opts['polar-fs-config']); } catch (error) { + logger.warn(`Failed to parse --polar-fs-config as JSON, will be ignored: ${error.message}`); this.polarFsConfig = opts['polar-fs-config']; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/session/index.ts` around lines 82 - 95, The OSS and Polar FS JSON parsing silently fall back to the raw string on parse error; update the catch blocks for opts['oss-mount-config'] and opts['polar-fs-config'] to log a warning when JSON.parse throws (mirroring the nasConfig handling) and then retain the raw string in this.ossMountConfig / this.polarFsConfig; reference the existing parsing code for nasConfig to reuse the same warning message style and the same logger (e.g., this.logger.warn or the project's logger) so users are informed their mount config was malformed.
🤖 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/commands-help/session.ts`:
- Around line 59-62: The help text for the '--region <region>' option in
session.ts uses the wrong documentation URL (2018.html); update the string
literal in the array containing '--region <region>' so it matches the other
occurrences by replacing '2018.html' with '2512917.html' (locate the array entry
near the '--region <region>' help line in session.ts to make the change).
- Around line 121-123: The help text for the session TTL option contains a wrong
upper bound "2512917"; update the option description string for '--st,
--session-ttl-in-seconds <seconds>' (the array entry containing '[Optional]
Session TTL in seconds, between 0 and 2512917') to use the correct max value
21600 to match the create command help and the validation in
src/subCommands/session/index.ts (the validation enforcing max 21600).
In `@src/resources/fc/index.ts`:
- Around line 991-1006: The documentation uses an invalid SDK field name
`bucketRegion`; update the example in src/commands-help/session.ts so it matches
the OSSMountPoint fields used in the code (see OSSMountPoint and OSSMountConfig
usage around config.ossMountConfig and mountPoints) by replacing `bucketRegion`
with the proper fields `bucketPath` and `endpoint` (so the example includes
"bucketPath": "/" and a full "endpoint" URL) so the documented JSON matches the
SDK shape the OSSMountPoint constructor expects.
- Around line 1008-1023: Update the help example JSON in the session help text
so its field names match the SDK PolarFS model: replace "polarDbClusterId" with
"instanceId" and add the required "remoteDir" property (e.g. mountPoints: [{
"instanceId": "pfs-...", "mountDir": "/mnt/polar", "remoteDir": "/" }]) so the
example aligns with PolarFsConfig/PolarFsMountConfig expectations and prevents
users from copying invalid config.
---
Nitpick comments:
In `@src/subCommands/session/index.ts`:
- Around line 82-95: The OSS and Polar FS JSON parsing silently fall back to the
raw string on parse error; update the catch blocks for opts['oss-mount-config']
and opts['polar-fs-config'] to log a warning when JSON.parse throws (mirroring
the nasConfig handling) and then retain the raw string in this.ossMountConfig /
this.polarFsConfig; reference the existing parsing code for nasConfig to reuse
the same warning message style and the same logger (e.g., this.logger.warn or
the project's logger) so users are informed their mount config was malformed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35cf4177-7685-4425-980f-414d082bb3f7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/commands-help/session.tssrc/resources/fc/index.tssrc/subCommands/session/index.ts
Comment on lines
59
to
62
| [ | ||
| '--region <region>', | ||
| '[C-Required] Specify fc region, you can see all supported regions in https://help.aliyun.com/document_detail/2512917.html', | ||
| '[C-Required] Specify fc region, you can see all supported regions in https://help.aliyun.com/document_detail/2018.html', | ||
| ], |
There was a problem hiding this comment.
Inconsistent region help URL.
Line 61 uses document 2018.html while all other occurrences (lines 17, 38, 95, 115) use 2512917.html. This appears to be an unintended change.
🐛 Proposed fix
[
'--region <region>',
- '[C-Required] Specify fc region, you can see all supported regions in https://help.aliyun.com/document_detail/2018.html',
+ '[C-Required] Specify fc region, you can see all supported regions in https://help.aliyun.com/document_detail/2512917.html',
],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands-help/session.ts` around lines 59 - 62, The help text for the
'--region <region>' option in session.ts uses the wrong documentation URL
(2018.html); update the string literal in the array containing '--region
<region>' so it matches the other occurrences by replacing '2018.html' with
'2512917.html' (locate the array entry near the '--region <region>' help line in
session.ts to make the change).
liuzewen99
force-pushed
the
fix-session
branch
4 times, most recently
from
a975feb to
8daee0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/commands-help/session.ts (1)
119-119:⚠️ Potential issue | 🟡 MinorTTL upper bound
2512917is incorrect — should be21600.The value
2512917matches the Aliyun document ID pattern (seen in help URLs). Thecreatecommand help (line 68) correctly showsbetween 0 and 21600, and the validation insrc/subCommands/session/index.ts(lines 190-195) enforces21600as the max.🐛 Proposed fix
[ '--st, --session-ttl-in-seconds <seconds>', - '[Optional] Session TTL in seconds, between 0 and 2512917', + '[Optional] Session TTL in seconds, between 0 and 21600', ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands-help/session.ts` at line 119, The help text for the Session TTL currently states an incorrect upper bound "2512917"; update the string entry "'[Optional] Session TTL in seconds, between 0 and 2512917'" to use the correct max "21600" so it reads "'[Optional] Session TTL in seconds, between 0 and 21600'"; ensure this matches the existing validation in the create subcommand (the create command's help and the validation in the session create handler) so that the 'Session TTL' help and the validation logic are consistent.
🤖 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/commands-help/session.ts`:
- Line 53: The example OSS mount JSON in the session command uses an incorrect
region string for bucketPath; update the oss-mount-config example to use a valid
bucket path (e.g., "/" or "/data") instead of "cn-hangzhou". Specifically,
modify the example JSON's "bucketPath" field in the session help/example (the
oss-mount-config snippet) to a semantic bucket path like "/" or "/data" so it
matches the SDK expectation for bucketPath.
---
Duplicate comments:
In `@src/commands-help/session.ts`:
- Line 119: The help text for the Session TTL currently states an incorrect
upper bound "2512917"; update the string entry "'[Optional] Session TTL in
seconds, between 0 and 2512917'" to use the correct max "21600" so it reads
"'[Optional] Session TTL in seconds, between 0 and 21600'"; ensure this matches
the existing validation in the create subcommand (the create command's help and
the validation in the session create handler) so that the 'Session TTL' help and
the validation logic are consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8728c9c0-48a3-4027-b24a-69a031e5d938
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
__tests__/e2e/session/run__tests__/e2e/session/s.yaml__tests__/ut/commands/session_test.tssrc/commands-help/session.tssrc/resources/fc/index.tssrc/subCommands/session/index.ts
✅ Files skipped from review due to trivial changes (2)
- tests/e2e/session/s.yaml
- tests/ut/commands/session_test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/subCommands/session/index.ts
| $ s cli fc3 session create --region cn-hangzhou --function-name my-function --qualifier LATEST --session-ttl-in-seconds 600 -a default | ||
| $ s cli fc3 session create --region cn-hangzhou --function-name my-function --qualifier LATEST --nas-config '{"userId": 1000, "groupId": 1000, "mountPoints": [{"serverAddr": "example.nas.aliyuncs.com:/", "mountDir": "/mnt/nas", "enableTLS": true}]}' -a default`, | ||
| $ s cli fc3 session create --region cn-hangzhou --function-name my-function --qualifier LATEST --nas-config '{"userId": 1000, "groupId": 1000, "mountPoints": [{"serverAddr": "example.nas.aliyuncs.com:/", "mountDir": "/mnt/nas", "enableTLS": true}]}' -a default | ||
| $ s cli fc3 session create --region cn-hangzhou --function-name my-function --qualifier LATEST --oss-mount-config '{"mountPoints": [{"bucketName": "my-bucket", "bucketPath": "cn-hangzhou", "mountDir": "/mnt/oss", "readOnly": false}]}' -a default |
There was a problem hiding this comment.
The bucketPath example value is semantically incorrect.
The example shows "bucketPath": "cn-hangzhou" which appears to be a region name, not a bucket path. According to the SDK documentation, bucketPath should be the path within the OSS bucket (e.g., "/", "/data").
This likely stems from confusion with the previously incorrect bucketRegion field that was removed.
🐛 Proposed fix
- $ s cli fc3 session create --region cn-hangzhou --function-name my-function --qualifier LATEST --oss-mount-config '{"mountPoints": [{"bucketName": "my-bucket", "bucketPath": "cn-hangzhou", "mountDir": "/mnt/oss", "readOnly": false}]}' -a default
+ $ s cli fc3 session create --region cn-hangzhou --function-name my-function --qualifier LATEST --oss-mount-config '{"mountPoints": [{"bucketName": "my-bucket", "bucketPath": "/", "endpoint": "http://oss-cn-hangzhou.aliyuncs.com", "mountDir": "/mnt/oss", "readOnly": false}]}' -a default🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands-help/session.ts` at line 53, The example OSS mount JSON in the
session command uses an incorrect region string for bucketPath; update the
oss-mount-config example to use a valid bucket path (e.g., "/" or "/data")
instead of "cn-hangzhou". Specifically, modify the example JSON's "bucketPath"
field in the session help/example (the oss-mount-config snippet) to a semantic
bucket path like "/" or "/data" so it matches the SDK expectation for
bucketPath.
67c9cec to
af690ea
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/commands-help/session.ts (2)
119-119:⚠️ Potential issue | 🟡 MinorUpdate TTL max in help to match actual validation.
Line 119 says
between 0 and 2512917, butsrc/subCommands/session/index.ts(Line 190-195) enforces0..21600. This mismatch causes incorrect CLI guidance.🐛 Proposed fix
[ '--st, --session-ttl-in-seconds <seconds>', - '[Optional] Session TTL in seconds, between 0 and 2512917', + '[Optional] Session TTL in seconds, between 0 and 21600', ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands-help/session.ts` at line 119, The help text for the session TTL is incorrect; update the string "'[Optional] Session TTL in seconds, between 0 and 2512917'" in commands-help/session.ts to match the validator's range (0..21600) used in the session subcommand (see validation in src/subCommands/session/index.ts around the TTL checks), so the CLI help accurately reflects the enforced maximum of 21600 seconds.
53-53:⚠️ Potential issue | 🟡 MinorFix OSS example:
bucketPathshould be a path, not a region.Line 53 uses
"bucketPath": "cn-hangzhou", which is semantically incorrect for a bucket path and can mislead users.🐛 Proposed fix
- $ s cli fc3 session create --region cn-hangzhou --function-name my-function --qualifier LATEST --oss-mount-config '{"mountPoints": [{"bucketName": "my-bucket", "bucketPath": "cn-hangzhou", "mountDir": "/mnt/oss", "readOnly": false}]}' -a default + $ s cli fc3 session create --region cn-hangzhou --function-name my-function --qualifier LATEST --oss-mount-config '{"mountPoints": [{"bucketName": "my-bucket", "bucketPath": "/", "mountDir": "/mnt/oss", "readOnly": false}]}' -a default🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands-help/session.ts` at line 53, The OSS example in the CLI usage string in session.ts incorrectly sets "bucketPath" to a region ("cn-hangzhou"); update that example value to a valid bucket path (e.g. "/" or "/path/to/dir") in the OSS mount config JSON in the usage/example string so the "bucketPath" semantically represents a path rather than a region; locate the CLI example string around the OSS mount config (the literal containing "oss-mount-config" and "mountPoints") and replace the "bucketPath": "cn-hangzhou" token with a proper path value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/commands-help/session.ts`:
- Line 119: The help text for the session TTL is incorrect; update the string
"'[Optional] Session TTL in seconds, between 0 and 2512917'" in
commands-help/session.ts to match the validator's range (0..21600) used in the
session subcommand (see validation in src/subCommands/session/index.ts around
the TTL checks), so the CLI help accurately reflects the enforced maximum of
21600 seconds.
- Line 53: The OSS example in the CLI usage string in session.ts incorrectly
sets "bucketPath" to a region ("cn-hangzhou"); update that example value to a
valid bucket path (e.g. "/" or "/path/to/dir") in the OSS mount config JSON in
the usage/example string so the "bucketPath" semantically represents a path
rather than a region; locate the CLI example string around the OSS mount config
(the literal containing "oss-mount-config" and "mountPoints") and replace the
"bucketPath": "cn-hangzhou" token with a proper path value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 815b90ce-e1c5-401d-a731-31570bbf9542
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
__tests__/e2e/session/run__tests__/e2e/session/s.yaml__tests__/ut/commands/session_test.tssrc/commands-help/session.tssrc/resources/fc/index.tssrc/subCommands/session/index.ts
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/session/s.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/ut/commands/session_test.ts
- src/resources/fc/index.ts
- tests/e2e/session/run
- src/subCommands/session/index.ts
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
1 participant
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
New Features
Documentation
Tests