Conversation
📝 WalkthroughWalkthroughAdds pre-delete validations in removeModel to prevent deleting OSS bucket roots or NAS root mounts by inspecting the first OSS mount's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/subCommands/model/model.ts`:
- Around line 112-120: The OSS root check incorrectly treats any non-empty
bucketPath as root because the condition uses
(processedOssMountPoints[0].bucketPath || processedOssMountPoints[0].bucketPath
=== '/'); update the guard around storage === 'oss' to explicitly detect
undefined/empty or the literal root by checking processedOssMountPoints[0]
exists and then (processedOssMountPoints[0].bucketPath === undefined ||
processedOssMountPoints[0].bucketPath === '' ||
processedOssMountPoints[0].bucketPath === '/'); throw the Error only in that
case so valid non-root bucketPath values (e.g., '/models/mymodel') are allowed
to proceed.
- Around line 110-111: NAS_ROOT_REGEX currently only matches China regions
because it hardcodes "cn-[a-z]+"; update the regex (symbol NAS_ROOT_REGEX in
model.ts) to accept any region token so non‑China domains (e.g., ap-southeast-1,
us-west-1) are matched. Replace the region part with a more general character
class such as [a-z0-9-]+ (for example change \.cn-[a-z]+\. to \.[a-z0-9-]+\.) so
the full pattern becomes something like
/^[\w-]+\.[a-z0-9-]+\.nas\.aliyuncs\.com:\/$/ and retain the rest of the pattern
and anchors.
5258e48 to
ee61e1e
Compare
src/subCommands/model/model.ts
Outdated
|
|
||
| const processedOssMountPoints = extractOssMountDir(ossMountPoints); | ||
|
|
||
| const NAS_ROOT_REGEX = /^[\w-]+\.((?:cn|ap|us|eu|me|sa)-[a-z]+-[0-9])\.nas\.aliyuncs\.com:\/$/; |
There was a problem hiding this comment.
你这个nas 挂载点的校验是不是太狠了,我理解你只需要关注 split by :, arr[1] 不会 / 即可
| (!processedOssMountPoints[0].bucketPath || processedOssMountPoints[0].bucketPath === '/') | ||
| ) { | ||
| throw new Error( | ||
| 'The current deleted directory is the OSS root directory. To delete the current model, please go to the OSS console to delete the model.', |
There was a problem hiding this comment.
之前那个 ignore remove 的环境变量似乎没有生效啊
3052f59 to
4afbc29
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/subCommands/model/model.ts`:
- Around line 120-123: The code currently accesses nasMountPoints[0].serverAddr
and splits it before checking storage, which can crash for empty or non-NAS
cases; move the parsing of serverAddr (the split and trim that produces nasPath)
inside the branch that only runs when storage === 'nas' and nasMountPoints[0]
exists, first validate nasMountPoints.length>0 and that
nasMountPoints[0].serverAddr is a non-empty string, then split into parts and
ensure parts.length>1 and parts[1] is defined before calling trim(); if
validation fails, handle it gracefully (throw a clear Error or treat as non-NAS)
so functions using nasPath (the nasPath variable) never operate on undefined.
src/subCommands/model/model.ts
Outdated
| const nasPath = nasMountPoints[0].serverAddr.split(':')[1]; | ||
|
|
||
| if (storage === 'nas' && nasMountPoints[0] && nasPath.trim() === '/') { | ||
| throw new Error( |
There was a problem hiding this comment.
Guard NAS parsing to avoid crashes for non‑NAS or empty mount points.
nasMountPoints[0].serverAddr is accessed unconditionally, so OSS deletions (or empty NAS configs) can throw before the NAS guard runs. Also split(':')[1] can be undefined (no colon/IPv6), causing .trim() to crash. Move parsing inside the NAS branch and validate the format.
🛠️ Suggested fix
- const nasPath = nasMountPoints[0].serverAddr.split(':')[1];
-
- if (storage === 'nas' && nasMountPoints[0] && nasPath.trim() === '/') {
+ if (storage === 'nas' && nasMountPoints[0]?.serverAddr) {
+ const idx = nasMountPoints[0].serverAddr.lastIndexOf(':');
+ const nasPath = idx >= 0 ? nasMountPoints[0].serverAddr.slice(idx + 1) : '';
+ if (nasPath.trim() === '/') {
throw new Error(
'The current deleted directory is the NAS root directory. To delete the current model, please go to the NAS console to delete the model.',
);
- }
+ }
+ }🤖 Prompt for AI Agents
In `@src/subCommands/model/model.ts` around lines 120 - 123, The code currently
accesses nasMountPoints[0].serverAddr and splits it before checking storage,
which can crash for empty or non-NAS cases; move the parsing of serverAddr (the
split and trim that produces nasPath) inside the branch that only runs when
storage === 'nas' and nasMountPoints[0] exists, first validate
nasMountPoints.length>0 and that nasMountPoints[0].serverAddr is a non-empty
string, then split into parts and ensure parts.length>1 and parts[1] is defined
before calling trim(); if validation fails, handle it gracefully (throw a clear
Error or treat as non-NAS) so functions using nasPath (the nasPath variable)
never operate on undefined.
4afbc29 to
c9a01e5
Compare
c9a01e5 to
943e89b
Compare
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.