[Fix] Fix review issues#4
Merged
Merged
Conversation
…th old SDK) - login() now calls POST /api/v1/login and saves server cookies, git token, and user info to ~/.modelscope/credentials/ (pickle format), matching the old modelscope SDK behavior exactly. - get_cookies() falls back to loading saved cookies from disk when no token is available in memory or env. - load_token() resolution: ~/.modelscope/token → m_session_id from saved cookies, ensuring existing old-SDK logins are recognized. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove ~/.modelscope/token dependency; token now lives only in credentials/cookies (pickled RequestsCookieJar with expiry) - save_token() creates m_session_id cookie with 30-day expiry - load_token() reads from credentials/cookies with is_expired() check - clear_token() deletes credentials/cookies file - Add get_session_id() for stable UUID tracking (credentials/session) - Add load_git_token() for reading credentials/git_token - OpenAPI _auth_headers falls back to load_token() when token is None - Legacy client token syncs from config on property access - Fix --exist-ok to handle Chinese error messages (已被注册) - Fix create_tag payload (TagName/Ref instead of Tag/Revision) - Fix delete_files to use per-file DELETE endpoint - Add delete_file method to LegacyClient - Mark server-restricted deletion tests as xfail Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add _translate_message() mapping for known Chinese API responses (e.g. "该名称已被注册使用" → "Repository name already exists"). Raw server JSON is preserved in debug Response: line for troubleshooting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The skill API requires a category field. Also print ASCII banner to stderr so it doesn't interleave with stdout command output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add Cookie header (m_session_id) to LFS blob PUT requests, fixing cross-domain auth failure on LFS endpoints (e.g. pre-lfs.modelscope.cn) - Add application-level response body validation (Code field check), matching old SDK's raise_on_error pattern - Add User-Agent header to LegacyClient session, carrying stable session_id from ~/.modelscope/credentials/session - Extract shared build_user_agent() utility, replacing duplicate code in DownloadManager and fixing random-UUID-per-call bug Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rt_type
- Skill repos don't support per-file /repo?FilePath=... download (404).
Use /archive/zip/{revision} endpoint matching old SDK's download_skill().
- Collection download creates per-skill subdirectories to avoid overwriting.
- MCP deploy defaults transport_type to "sse" (server rejects empty value).
- Add --transport-type and --expiration-minutes CLI options for ms mcp deploy.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for skill repository downloads via zip archives, refactors credential persistence to use a dedicated credentials directory with cookie-based session authentication, and adds several CLI enhancements including MCP deployment options and Chinese error message translation. The review feedback highlights several critical security and robustness issues: sensitive session cookies and git tokens are saved without restrictive file permissions (0o600), empty successful upload responses can trigger JSONDecodeError, temporary download files may leak on network failures, type mismatches exist in error code checks, and fatal errors in the file deletion loop should be propagated immediately.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- Security: Set 0600 permissions on credentials/cookies and git_token files to prevent other local users from reading sensitive tokens. - Robustness: Handle empty/non-JSON response bodies in upload_blob() (presigned URLs may return empty body on success). - Robustness: Wrap try/finally around entire archive download to prevent temp file leaks on network failures. - Error handling: Re-raise AuthenticationError and NetworkError immediately in delete_files loop (no point retrying other files). - Type safety: Convert response Code to int before checking against _ALREADY_EXISTS_CODES set (server may return Code as string). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.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.
🚀 New Features & Enhancements
🛡️ Security & Robustness Fixes
0o600file permissions.JSONDecodeErroron empty successful upload responses.