Open
Conversation
📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
__tests__/ut/commands/session_test.ts (1)
408-437: Redundant re-assignment ofmockInputs.argsandsession.Lines 417-428 re-declare the exact same args already set in the
beforeEach(lines 394-405) and re-instantiatesession. Harmless, but can be deleted for clarity.♻️ Suggested cleanup
mockFcSdk.updateFunctionSession = jest.fn().mockResolvedValue(mockResult); - mockInputs.args = [ - 'update', - '--session-id', - 'session-123', - '--qualifier', - 'LATEST', - '--session-ttl-in-seconds', - '7200', - '--session-idle-timeout-in-seconds', - '3600', - ]; - session = new Session(mockInputs); const result = await session.update();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/ut/commands/session_test.ts` around lines 408 - 437, Remove the redundant re-assignment of mockInputs.args and the duplicate instantiation of session inside the 'should update session successfully' test: the args are already set in beforeEach and session was already created, so delete the block that resets mockInputs.args and re-creates session (references: mockInputs.args, session = new Session(...), and the test for session.update()) to keep the test clearer and avoid duplicate setup.
🤖 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__/ut/commands/session_test.ts`:
- Around line 264-294: The test uses a misleading OSS mount fixture: change the
OSSMountPoint in the mockInputs.args for Session (used by Session.create()) so
bucketPath is a real bucket-internal path (e.g. "/" or "/my-dir") instead of
"cn-hangzhou", and add the endpoint field (e.g. "oss-cn-hangzhou.aliyuncs.com");
then update the expected assertion passed to mockFcSdk.createFunctionSession to
include the corrected ossMountConfig with the new bucketPath and endpoint fields
so the test reflects real OSS semantics.
- Around line 296-336: The test is using an invalid PolarFsMountConfig field
`polarDbClusterId` and omitting required `remoteDir`; update the test payload
and expectation in the 'should create session with polarFsConfig when provided'
spec so the mock input JSON for --polar-fs-config uses mountPoints objects with
instanceId, mountDir, and remoteDir (e.g. remoteDir: "/") instead of
polarDbClusterId, and adjust the expected argument passed to
mockFcSdk.createFunctionSession (the call asserted in the test) to match this
shape; locate references in this test to Session and
mockFcSdk.createFunctionSession to change both the input string and the expected
polarFsConfig object.
In `@src/resources/fc/impl/utils.ts`:
- Around line 127-130: The getCustomEndpoint function currently hardcodes
protocol: 'https' and endpoint: `https://${CUSTOM_ENDPOINT}` which ignores the
computed protocol variable from isDebugMode; update the return to use the
resolved protocol variable (e.g., protocol: protocol, host: CUSTOM_ENDPOINT,
endpoint: `${protocol}://${CUSTOM_ENDPOINT}`) so protocol-less custom endpoints
use http in debug mode, and add a unit test mocking isDebugMode to true that
asserts getCustomEndpoint('test.com') returns protocol 'http', host 'test.com',
and endpoint 'http://test.com'.
---
Nitpick comments:
In `@__tests__/ut/commands/session_test.ts`:
- Around line 408-437: Remove the redundant re-assignment of mockInputs.args and
the duplicate instantiation of session inside the 'should update session
successfully' test: the args are already set in beforeEach and session was
already created, so delete the block that resets mockInputs.args and re-creates
session (references: mockInputs.args, session = new Session(...), and the test
for session.update()) to keep the test clearer and avoid duplicate setup.
🪄 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: bc33ef9e-9208-4502-a15b-209fe136d725
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
__tests__/e2e/session/run__tests__/e2e/session/s.yaml__tests__/ut/commands/session_test.tssrc/commands-help/session.tssrc/resources/fc/impl/utils.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 (2)
- src/subCommands/session/index.ts
- src/commands-help/session.ts
Comment on lines
+264
to
+294
| mockInputs.args = [ | ||
| 'create', | ||
| '--qualifier', | ||
| 'LATEST', | ||
| '--session-ttl-in-seconds', | ||
| '3600', | ||
| '--session-idle-timeout-in-seconds', | ||
| '1800', | ||
| '--oss-mount-config', | ||
| '{"mountPoints":[{"bucketName":"test-bucket","bucketPath":"cn-hangzhou","mountDir":"/mnt/oss","readOnly":false}]}', | ||
| ]; | ||
| session = new Session(mockInputs); | ||
|
|
||
| const result = await session.create(); | ||
| expect(result).toEqual(mockResult); | ||
| expect(mockFcSdk.createFunctionSession).toHaveBeenCalledWith('test-function', { | ||
| qualifier: 'LATEST', | ||
| sessionTTLInSeconds: 3600, | ||
| sessionIdleTimeoutInSeconds: 1800, | ||
| ossMountConfig: { | ||
| mountPoints: [ | ||
| { | ||
| bucketName: 'test-bucket', | ||
| bucketPath: 'cn-hangzhou', | ||
| mountDir: '/mnt/oss', | ||
| readOnly: false, | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Misleading bucketPath value in ossMountConfig test fixture.
bucketPath is a path inside the OSS bucket (e.g. "/" or "/my-dir"), not a region. Passing "cn-hangzhou" as bucketPath is confusing test data that could be copied by users reading tests as a reference. Also missing endpoint, which is the real OSS-region-bearing field in OSSMountPoint.
♻️ Suggested fix
- '--oss-mount-config',
- '{"mountPoints":[{"bucketName":"test-bucket","bucketPath":"cn-hangzhou","mountDir":"/mnt/oss","readOnly":false}]}',
+ '--oss-mount-config',
+ '{"mountPoints":[{"bucketName":"test-bucket","bucketPath":"/","endpoint":"http://oss-cn-hangzhou.aliyuncs.com","mountDir":"/mnt/oss","readOnly":false}]}',And update the expected assertion object accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/ut/commands/session_test.ts` around lines 264 - 294, The test uses
a misleading OSS mount fixture: change the OSSMountPoint in the mockInputs.args
for Session (used by Session.create()) so bucketPath is a real bucket-internal
path (e.g. "/" or "/my-dir") instead of "cn-hangzhou", and add the endpoint
field (e.g. "oss-cn-hangzhou.aliyuncs.com"); then update the expected assertion
passed to mockFcSdk.createFunctionSession to include the corrected
ossMountConfig with the new bucketPath and endpoint fields so the test reflects
real OSS semantics.
Comment on lines
+296
to
+336
| it('should create session with polarFsConfig when provided', async () => { | ||
| const mockResult = { | ||
| sessionId: 'session-123', | ||
| qualifier: 'LATEST', | ||
| sessionTTLInSeconds: 3600, | ||
| sessionIdleTimeoutInSeconds: 1800, | ||
| }; | ||
|
|
||
| mockFcSdk.createFunctionSession = jest.fn().mockResolvedValue(mockResult); | ||
| mockInputs.args = [ | ||
| 'create', | ||
| '--qualifier', | ||
| 'LATEST', | ||
| '--session-ttl-in-seconds', | ||
| '3600', | ||
| '--session-idle-timeout-in-seconds', | ||
| '1800', | ||
| '--polar-fs-config', | ||
| '{"userId":1000,"groupId":1000,"mountPoints":[{"polarDbClusterId":"pc-test","instanceId":"pfs-test","mountDir":"/mnt/polar"}]}', | ||
| ]; | ||
| session = new Session(mockInputs); | ||
|
|
||
| const result = await session.create(); | ||
| expect(result).toEqual(mockResult); | ||
| expect(mockFcSdk.createFunctionSession).toHaveBeenCalledWith('test-function', { | ||
| qualifier: 'LATEST', | ||
| sessionTTLInSeconds: 3600, | ||
| sessionIdleTimeoutInSeconds: 1800, | ||
| polarFsConfig: { | ||
| userId: 1000, | ||
| groupId: 1000, | ||
| mountPoints: [ | ||
| { | ||
| polarDbClusterId: 'pc-test', | ||
| instanceId: 'pfs-test', | ||
| mountDir: '/mnt/polar', | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
polarDbClusterId is not a valid PolarFsMountConfig field; remoteDir is missing.
The SDK PolarFsMountConfig accepts instanceId, mountDir, remoteDir (see the mapping in createFunctionSession at src/resources/fc/index.ts lines 1009–1015). polarDbClusterId is neither in the SDK nor consumed by the code — including it in this test payload is misleading and inconsistent with the earlier doc fix in commit 8daee0f. Also remoteDir is required in typical PolarFS usage and should be represented here.
♻️ Suggested fix
- '--polar-fs-config',
- '{"userId":1000,"groupId":1000,"mountPoints":[{"polarDbClusterId":"pc-test","instanceId":"pfs-test","mountDir":"/mnt/polar"}]}',
+ '--polar-fs-config',
+ '{"userId":1000,"groupId":1000,"mountPoints":[{"instanceId":"pfs-test","mountDir":"/mnt/polar","remoteDir":"/"}]}',And update the expected assertion object to drop polarDbClusterId and include remoteDir: "/".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/ut/commands/session_test.ts` around lines 296 - 336, The test is
using an invalid PolarFsMountConfig field `polarDbClusterId` and omitting
required `remoteDir`; update the test payload and expectation in the 'should
create session with polarFsConfig when provided' spec so the mock input JSON for
--polar-fs-config uses mountPoints objects with instanceId, mountDir, and
remoteDir (e.g. remoteDir: "/") instead of polarDbClusterId, and adjust the
expected argument passed to mockFcSdk.createFunctionSession (the call asserted
in the test) to match this shape; locate references in this test to Session and
mockFcSdk.createFunctionSession to change both the input string and the expected
polarFsConfig object.
Comment on lines
127
to
+130
| return { | ||
| protocol: 'https', | ||
| host: endpoint, | ||
| endpoint: `https://${endpoint}`, | ||
| host: CUSTOM_ENDPOINT, | ||
| endpoint: `https://${CUSTOM_ENDPOINT}`, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate existing getCustomEndpoint tests and confirm whether debug + host-only endpoint is covered.
# Expected: A test case where isDebugMode() is mocked true and getCustomEndpoint('test.com')
# or FC_CLIENT_CUSTOM_ENDPOINT='test.com' returns protocol/endpoints using http.
rg -n -C4 "getCustomEndpoint|isDebugMode|FC_CLIENT_CUSTOM_ENDPOINT" --glob '*test*' --glob '!node_modules/**'Repository: devsapp/fc3
Length of output: 12576
🏁 Script executed:
sed -n '90,135p' src/resources/fc/impl/utils.ts | cat -nRepository: devsapp/fc3
Length of output: 1323
Use the resolved debug protocol for host-only endpoints.
Lines 38-42 hardcode protocol: 'https' and endpoint: 'https://...', ignoring the protocol variable computed at lines 11-14. This means when isDebugMode() returns true and a protocol-less custom endpoint is provided (e.g., localhost:8080), the function returns HTTPS instead of HTTP, breaking debug-mode connections to HTTP-only local endpoints.
The proposed fix is correct:
🐛 Fix
return {
- protocol: 'https',
+ protocol,
host: CUSTOM_ENDPOINT,
- endpoint: `https://${CUSTOM_ENDPOINT}`,
+ endpoint: `${protocol}://${CUSTOM_ENDPOINT}`,
};Additionally, add a test case for debug mode with a protocol-less custom endpoint to prevent regression. For example:
it('should use http protocol in debug mode with protocol-less custom endpoint', () => {
(isDebugMode as jest.Mock).mockReturnValue(true);
const result = getCustomEndpoint('test.com');
expect(result).toEqual({
protocol: 'http',
host: 'test.com',
endpoint: 'http://test.com',
});
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/resources/fc/impl/utils.ts` around lines 127 - 130, The getCustomEndpoint
function currently hardcodes protocol: 'https' and endpoint:
`https://${CUSTOM_ENDPOINT}` which ignores the computed protocol variable from
isDebugMode; update the return to use the resolved protocol variable (e.g.,
protocol: protocol, host: CUSTOM_ENDPOINT, endpoint:
`${protocol}://${CUSTOM_ENDPOINT}`) so protocol-less custom endpoints use http
in debug mode, and add a unit test mocking isDebugMode to true that asserts
getCustomEndpoint('test.com') returns protocol 'http', host 'test.com', and
endpoint 'http://test.com'.
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