Init SGLang Ascend Agent structure#417
Init SGLang Ascend Agent structure#417shengzhaotian wants to merge 2 commits intosgl-project:mainfrom
Conversation
![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 the SGLang Ascend Agents architecture, featuring a four-layer agent model and a persistent memory system. It includes the 'planning-with-files' skill, which uses Markdown-based external memory for task management, along with supporting scripts for session initialization and catch-up. Feedback focuses on enhancing script robustness through explicit UTF-8 encoding, more precise file path matching, and the use of regular expressions for parsing task status in shell scripts.
|
|
||
| # First, try to parse the entire file as JSON (for *.json session files). | ||
| try: | ||
| with open(session_file, 'r') as f: |
There was a problem hiding this comment.
When opening files for reading, it is best practice to explicitly specify the encoding (e.g., utf-8) to ensure consistent behavior across different operating systems and environments where the default encoding might differ.
| with open(session_file, 'r') as f: | |
| with open(session_file, 'r', encoding='utf-8') as f: |
|
|
||
| # Fallback: treat file as JSONL (one JSON object per line), the original behavior. | ||
| messages = [] | ||
| with open(session_file, 'r') as f: |
There was a problem hiding this comment.
Explicitly specifying the encoding as utf-8 here will prevent potential UnicodeDecodeError issues on systems with different default locales.
| with open(session_file, 'r') as f: | |
| with open(session_file, 'r', encoding='utf-8') as f: |
| if tool_name in ('Write', 'Edit'): | ||
| file_path = tool_input.get('file_path', '') | ||
| for pf in PLANNING_FILES: | ||
| if file_path.endswith(pf): |
There was a problem hiding this comment.
Using endswith(pf) can lead to false positives if other files in the project have names ending with the same string (e.g., old_task_plan.md). It is safer to check the exact filename using os.path.basename.
| if file_path.endswith(pf): | |
| if os.path.basename(file_path) == pf: |
| COMPLETE=$(grep -cF "**Status:** complete" "$PLAN_FILE" || true) | ||
| IN_PROGRESS=$(grep -cF "**Status:** in_progress" "$PLAN_FILE" || true) | ||
| PENDING=$(grep -cF "**Status:** pending" "$PLAN_FILE" || true) |
There was a problem hiding this comment.
Using grep -F (fixed strings) for status checks is fragile as it depends on exact whitespace. If the agent or a user adds an extra space after the colon, the check will fail. Using a regular expression with grep -E would be more robust.
| COMPLETE=$(grep -cF "**Status:** complete" "$PLAN_FILE" || true) | |
| IN_PROGRESS=$(grep -cF "**Status:** in_progress" "$PLAN_FILE" || true) | |
| PENDING=$(grep -cF "**Status:** pending" "$PLAN_FILE" || true) | |
| COMPLETE=$(grep -cE "\\*\\*Status:\\*\\*[[:space:]]+complete" "$PLAN_FILE" || true) | |
| IN_PROGRESS=$(grep -cE "\\*\\*Status:\\*\\*[[:space:]]+in_progress" "$PLAN_FILE" || true) | |
| PENDING=$(grep -cE "\\*\\*Status:\\*\\*[[:space:]]+pending" "$PLAN_FILE" || true) |
1 participant
No description provided.