Feat/bug fix#920
Conversation
… management; support for personalization module
…at/refractor_llm
There was a problem hiding this comment.
Code Review
This pull request introduces a data-driven provider layer that replaces hard-coded LLM service mappings with a declarative spec registry and transport system. It also adds a lightweight Terminal UI (TUI) application and consolidates framework-internal directories under a single .ms_agent/ namespace to keep the working directory clean. The review feedback highlights several critical issues: the permission handler selection needs to support the default restricted mode; the OpenAI-compatible transport must guard continuation attempts to avoid 400 errors on strict APIs; the workspace zip download requires symlink resolution to prevent path traversal; the argument parser should ignore the standard -- separator; and log directory creation needs exception handling to prevent startup crashes.
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.
| if chunk.choices[0].finish_reason in [ | ||
| 'length', 'null' | ||
| ] and (max_runs is None or max_runs != 0): |
There was a problem hiding this comment.
If continue_gen_mode is None (meaning the provider does not support continuation, such as standard OpenAI, Gemini, etc.), the code still attempts to trigger continuation when finish_reason is length or null. This results in calling _call_llm_for_continue_gen, which appends partial: True or prefix: True to the message and sends consecutive assistant messages. This will cause a 400 Bad Request error on strict OpenAI-compatible APIs (e.g., official OpenAI) because they do not permit extra fields like partial or consecutive assistant messages. Continuation should only be attempted if self.continue_gen_mode is not None.
| if chunk.choices[0].finish_reason in [ | |
| 'length', 'null' | |
| ] and (max_runs is None or max_runs != 0): | |
| if self.continue_gen_mode and chunk.choices[0].finish_reason in [ | |
| 'length', 'null' | |
| ] and (max_runs is None or max_runs != 0): |
| if completion.choices[0].finish_reason in [ | ||
| 'length', 'null' | ||
| ] and (max_runs is None or max_runs != 0): |
There was a problem hiding this comment.
If continue_gen_mode is None (meaning the provider does not support continuation, such as standard OpenAI, Gemini, etc.), the code still attempts to trigger continuation when finish_reason is length or null. This results in calling _call_llm_for_continue_gen, which appends partial: True or prefix: True to the message and sends consecutive assistant messages. This will cause a 400 Bad Request error on strict OpenAI-compatible APIs (e.g., official OpenAI) because they do not permit extra fields like partial or consecutive assistant messages. Continuation should only be attempted if self.continue_gen_mode is not None.
| if completion.choices[0].finish_reason in [ | |
| 'length', 'null' | |
| ] and (max_runs is None or max_runs != 0): | |
| if self.continue_gen_mode and completion.choices[0].finish_reason in [ | |
| 'length', 'null' | |
| ] and (max_runs is None or max_runs != 0): |
| for p in sorted(target.rglob('*')): | ||
| if p.is_file(): | ||
| zf.write(p, p.relative_to(target)) |
There was a problem hiding this comment.
In zip_download, the code packages files in the workspace directory into a zip file. However, if there are symlinks inside the workspace pointing to files outside the workspace (e.g., /etc/passwd), p.is_file() will follow the symlink and package the external file into the zip. This represents an arbitrary file read / information disclosure vulnerability. We should resolve the path and ensure it is still within the workspace root before packaging.
| for p in sorted(target.rglob('*')): | |
| if p.is_file(): | |
| zf.write(p, p.relative_to(target)) | |
| for p in sorted(target.rglob('*')): | |
| if p.is_file(): | |
| try: | |
| p.resolve().relative_to(self._root) | |
| except ValueError: | |
| continue | |
| zf.write(p, p.relative_to(target)) |
| if (key.startswith('--') and idx + 1 < len(unknown) | ||
| and not unknown[idx + 1].startswith('--')): |
There was a problem hiding this comment.
In parse_args, the code checks if (key.startswith('--') and idx + 1 < len(unknown) and not unknown[idx + 1].startswith('--')). If key is exactly '--' (which is a standard argument separator in many CLI tools), key[2:] will be '', resulting in _dict_config[''] = unknown[idx + 1]. We should ensure len(key) > 2 to avoid parsing the standard '--' separator as an empty-key override.
| if (key.startswith('--') and idx + 1 < len(unknown) | |
| and not unknown[idx + 1].startswith('--')): | |
| if (key.startswith('--') and len(key) > 2 and idx + 1 < len(unknown) | |
| and not unknown[idx + 1].startswith('--')): |
| if env.strip().lower() in ('1', 'true', 'yes', 'on'): | ||
| from ms_agent.project.paths import global_logs_dir | ||
| d = global_logs_dir() | ||
| d.mkdir(parents=True, exist_ok=True) | ||
| return str(d / 'ms_agent.log') |
There was a problem hiding this comment.
In _resolve_log_file, d.mkdir(parents=True, exist_ok=True) is called without any exception handling. If there are permission issues or other filesystem errors when creating the ~/.ms_agent/logs/ directory, this will raise an OSError and crash the logger initialization, which in turn crashes the entire application at startup. We should wrap the directory creation in a try...except OSError block and fall back gracefully (e.g., returning None for console-only logging).
| if env.strip().lower() in ('1', 'true', 'yes', 'on'): | |
| from ms_agent.project.paths import global_logs_dir | |
| d = global_logs_dir() | |
| d.mkdir(parents=True, exist_ok=True) | |
| return str(d / 'ms_agent.log') | |
| if env.strip().lower() in ('1', 'true', 'yes', 'on'): | |
| from ms_agent.project.paths import global_logs_dir | |
| d = global_logs_dir() | |
| try: | |
| d.mkdir(parents=True, exist_ok=True) | |
| return str(d / 'ms_agent.log') | |
| except OSError: | |
| return None |
Change Summary
Related issue number
Checklist
pre-commit installandpre-commit run --all-filesbefore git commit, and passed lint check.