Skip to content

Fix/general run#917

Open
suluyana wants to merge 67 commits into
modelscope:mainfrom
suluyana:fix/general_run
Open

Fix/general run#917
suluyana wants to merge 67 commits into
modelscope:mainfrom
suluyana:fix/general_run

Conversation

@suluyana

Copy link
Copy Markdown
Collaborator

No description provided.

suluyan and others added 30 commits February 6, 2026 15:03
…date reporter delivery flow, and improve the quality check module (54.51)
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Add snapshot.py: isolated git repo under output_dir/.ms_agent_snapshots,
  stores message_count per commit for history truncation on rollback
- Auto-snapshot on every user turn via on_task_begin (enable_snapshots=True by default)
- Add list_snapshots()/rollback() to Agent base and LLMAgent:
  rollback restores files, truncates saved history, clears _read_cache
- Refactor filesystem_tool.py: remove replace_file_contents, rewrite
  edit_file with old_string→new_string exact replace, quote-normalization
  fallback, smart delete, trailing-whitespace strip, staleness check,
  multi-type read (images/binary), read dedup cache
- Add smoke tests (20 cases, all offline)
- AgentTool now handles dynamic mode (split_to_sub_task) internally,
  replacing the standalone SplitTask class. Backward compat preserved:
  configs with tools.split_task auto-register the built-in dynamic spec.
- Fix execution_mode missing from split_to_sub_task schema (was silently
  ignored before; now exposed as enum field with sequential/parallel).
- Increase max_subtask_output_chars default from 2048 to 8192.
- Add disallowed_tools to _AgentToolSpec to prevent recursive tool calls
  in sub-agents.
- Add sub-agent transcript persistence: in-process runs write messages
  to output_dir/subagents/<agent_tag>.jsonl for debugging.
- Add TaskManager (ms_agent/utils/task_manager.py): agent-level registry
  for background tasks with notification queue. LLMAgent initializes it
  in run_loop, wires it into AgentTool instances, and drains notifications
  at the top of each while-loop iteration. Supports future BashTool
  background mode via the same interface.
- diversity.py: replace SplitTask dependency with inline _run_tasks_sequential
  helper using LLMAgent directly.
When a spec has run_in_background=true, call_tool fires off the
subprocess and returns immediately with {status: async_launched,
task_id, tool_name}. A background asyncio watcher task polls the
result queue and calls task_manager.complete/fail when the process
exits. LLMAgent drains the TaskManager notification queue at the
top of each run_loop iteration, injecting <task-notification> XML
into the conversation so the model sees the result on the next turn.

run_in_background is opt-in per agent_tools definition:
  agent_tools:
    definitions:
      - tool_name: my_agent
        config_path: my_agent.yaml
        run_in_background: true
Exposes two tools to the model when tools.task_control is configured:
- list_tasks: show all background tasks with status and duration
- cancel_task: kill a running task by task_id

TaskControlTool receives the TaskManager reference via set_task_manager(),
which LLMAgent already calls for all extra_tools in run_loop. Enable with:

  tools:
    task_control: {}
suluyan added 13 commits April 21, 2026 17:28
Replace bench-specific filesystem_tool with feat/git version; accept
behavior differences vs prior tavily bench worktree.

Made-with: Cursor
AgentTool overhaul: stream files, TaskControlTool, TaskManager wiring,
SplitTask removal from default tool path. Excludes decision_chain_transparency.

Made-with: Cursor
Align edit/write with disk-backed validation (Claude Code style): remove
_check_staleness and post-write cache pops that caused redundant read_file
round-trips and noisy errors.

test: use tool_manager.extra_tools in rollback read_cache smoke (matches
LLMAgent.rollback).

Made-with: Cursor
…nfigs

- Subagent snapshot defaults and snapshot repo hook bypass
- FileSystemTool read_file path alias; grep newline guard
- Evidence write_note optional title; report commit_outline coercion and report_generator load_index
- Reporter todo_list; Tavily-only searcher yaml; exp_nosnap configs
- Searcher JSON parse resilience in callback

Made-with: Cursor
Resolve conflicts in llm_agent (omit bench-only knowledge_search init),
llm/utils (union UI-stripped keys), and tool_manager (keep LocalSearchTool
and TaskControlTool).

Made-with: Cursor
This reverts commit 67668e9.
Tool system overhaul:
- Increase default timeout from 30s to 300s, add per-call timeout via
  tool arguments (max 900s), and propagate timeout to subprocess wait
- Add try/except in tool_manager.connect() so a single tool failure
  no longer crashes the entire agent process
- Truncate excessively long tool outputs (>20000 chars) to prevent
  context window explosion
- Return structured JSON error responses with recovery hints on timeout
  and exception, replacing opaque string messages
- Conditional MCPClient import for graceful degradation when mcp
  package is not installed

Agent rollback:
- rollback() now returns (success, truncated_messages) tuple so the
  run loop can refresh in-memory history after disk restore
- Add _apply_pending_rollback() in step/run loop for seamless
  in-flight rollback without restarting the agent

Filesystem tool:
- Switch grep from read_text() to streaming line iteration for memory
  efficiency on large files
- read_file returns raw content without line-number prefixes so
  snippets can be reused directly in edit_file

Local code executor:
- Add min/max constraints to timeout schema, rename __call_id to
  call_id to avoid Python dunder name mangling, add
  _prepare_shell_command() helper for composite shell input

Python 3.9 compatibility:
- Add from __future__ import annotations to 14 files in the import
  chain to support PEP 604 union type syntax (str | None)

Other:
- Register RepetitionGuardCallback to detect and break agent loops
- Increase LLM retry delay from 1.0s to 3.0s
- Increase DEFAULT_RETRY_COUNT from 3 to 5
- AgentTool always saves transcript regardless of run_in_process
- Python 3.10 f-string nested quote fix in filesystem_tool
The default agent.yaml had an empty tools: section, meaning no tools
were available out of the box. Add the two core built-in tools:

- file_system: write_file, read_file, edit_file, grep, glob
- code_executor: python_env with shell_executor, python_executor,
  notebook_executor

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several significant enhancements to the ms-agent framework, including a new RepetitionGuardCallback to detect stuck loops, an isolated git-based snapshot and rollback utility for agent output directories, a TaskManager for managing background tasks, and a LocalSearchTool for on-demand codebase search via sirchmunk. Additionally, Tavily Search and Extract APIs have been integrated, and FileSystemTool has been upgraded with grep, glob, and safer edit_file capabilities. The code review identified several critical issues: a potential AttributeError in the repetition guard when retrieving configuration, a possible crash in the snapshot utility if git is missing, potential option-parsing errors in ripgrep when search patterns start with a hyphen, a missing path validation in edit_file, and incomplete dictionary handling in _save_transcript.

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.

Comment thread ms_agent/callbacks/repetition_guard.py Outdated
Comment on lines +75 to +80
guard_cfg = getattr(config, "repetition_guard", None)
if guard_cfg is None:
guard_cfg = {}
self.threshold: int = int(getattr(guard_cfg, "threshold", _DEFAULT_THRESHOLD))
self.lookback: int = int(getattr(guard_cfg, "lookback_rounds", _DEFAULT_LOOKBACK))
self.max_warnings: int = int(getattr(guard_cfg, "max_warnings", _DEFAULT_MAX_WARNINGS))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Using getattr() on a plain dictionary (which guard_cfg becomes if it is None) will raise an AttributeError because dictionaries do not support attribute-based lookup for keys. Since both dict and DictConfig support the .get() method, you should use .get() instead of getattr() to safely retrieve the configuration values.

Suggested change
guard_cfg = getattr(config, "repetition_guard", None)
if guard_cfg is None:
guard_cfg = {}
self.threshold: int = int(getattr(guard_cfg, "threshold", _DEFAULT_THRESHOLD))
self.lookback: int = int(getattr(guard_cfg, "lookback_rounds", _DEFAULT_LOOKBACK))
self.max_warnings: int = int(getattr(guard_cfg, "max_warnings", _DEFAULT_MAX_WARNINGS))
guard_cfg = getattr(config, "repetition_guard", None)
if guard_cfg is None:
guard_cfg = {}
self.threshold: int = int(guard_cfg.get("threshold", _DEFAULT_THRESHOLD))
self.lookback: int = int(guard_cfg.get("lookback_rounds", _DEFAULT_LOOKBACK))
self.max_warnings: int = int(guard_cfg.get("max_warnings", _DEFAULT_MAX_WARNINGS))

Comment on lines +232 to +234
except subprocess.CalledProcessError as e:
logger.warning(f'[snapshot] restore failed: {e.stderr.strip()}')
return False, 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If git is not installed on the system, _git will raise a FileNotFoundError (which inherits from OSError). Since restore_snapshot only catches subprocess.CalledProcessError, a missing git executable will cause a crash. You should catch Exception or OSError to make it robust against missing dependencies.

Suggested change
except subprocess.CalledProcessError as e:
logger.warning(f'[snapshot] restore failed: {e.stderr.strip()}')
return False, 0
except (subprocess.CalledProcessError, OSError) as e:
logger.warning(f'[snapshot] restore failed: {e}')
return False, 0

Comment thread ms_agent/tools/filesystem_tool.py Outdated
Comment on lines +509 to +514
if output_mode == 'files_with_matches':
args.extend(['-l', pattern, str(file_path)])
elif output_mode == 'count':
args.extend(['-c', pattern, str(file_path)])
else:
args.extend(['-n', pattern, str(file_path)])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If pattern starts with a hyphen (e.g., -foo), rg (ripgrep) will interpret it as a command-line option rather than the search pattern, which can cause errors or unexpected behavior. To prevent this, use the -e option to explicitly specify the pattern.

Suggested change
if output_mode == 'files_with_matches':
args.extend(['-l', pattern, str(file_path)])
elif output_mode == 'count':
args.extend(['-c', pattern, str(file_path)])
else:
args.extend(['-n', pattern, str(file_path)])
if output_mode == 'files_with_matches':
args.extend(['-l', '-e', pattern, str(file_path)])
elif output_mode == 'count':
args.extend(['-c', '-e', pattern, str(file_path)])
else:
args.extend(['-n', '-e', pattern, str(file_path)])

Comment thread ms_agent/tools/filesystem_tool.py Outdated
Comment on lines +546 to +551
if output_mode == 'files_with_matches':
args.extend(['-l', pattern, str(root)])
elif output_mode == 'count':
args.extend(['--count-matches', pattern, str(root)])
else:
args.extend(['-n', pattern, str(root)])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to _grep_rg_file, if pattern starts with a hyphen, rg will treat it as a command-line option. Use the -e option to safely pass the pattern to rg.

Suggested change
if output_mode == 'files_with_matches':
args.extend(['-l', pattern, str(root)])
elif output_mode == 'count':
args.extend(['--count-matches', pattern, str(root)])
else:
args.extend(['-n', pattern, str(root)])
if output_mode == 'files_with_matches':
args.extend(['-l', '-e', pattern, str(root)])
elif output_mode == 'count':
args.extend(['--count-matches', '-e', pattern, str(root)])
else:
args.extend(['-n', '-e', pattern, str(root)])

Comment on lines +1005 to +1008
if old_string is None:
return 'Error: `old_string` is required.'
if new_string is None:
return 'Error: `new_string` is required.'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If path is not provided (it defaults to None), calling self.get_real_path(path) on line 1010 will raise a TypeError (which is caught by the except block but results in a confusing error message). It is better to validate that path is not None or empty at the beginning of the method.

Suggested change
if old_string is None:
return 'Error: `old_string` is required.'
if new_string is None:
return 'Error: `new_string` is required.'
if not path:
return 'Error: `path` is required.'
if old_string is None:
return 'Error: `old_string` is required.'
if new_string is None:
return 'Error: `new_string` is required.'

Comment thread ms_agent/tools/agent_tool.py Outdated
Comment on lines +1201 to +1205
for msg in messages:
if hasattr(msg, 'to_dict'):
f.write(
json.dumps(msg.to_dict(), ensure_ascii=False)
+ '\n')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The _save_transcript method only writes messages that have a to_dict attribute. If any message in the list is already a plain dictionary, it will be silently ignored. You should handle both Message objects (with to_dict) and plain dictionaries, similar to how SubAgentStreamWriter does.

Suggested change
for msg in messages:
if hasattr(msg, 'to_dict'):
f.write(
json.dumps(msg.to_dict(), ensure_ascii=False)
+ '\n')
for msg in messages:
if hasattr(msg, 'to_dict'):
f.write(
json.dumps(msg.to_dict(), ensure_ascii=False)
+ '\n')
elif isinstance(msg, dict):
f.write(
json.dumps(msg, ensure_ascii=False)
+ '\n')

suluyan added 3 commits July 3, 2026 16:21
Resolved 24 conflicts across tool system, agent core, and projects.
Upstream brought: hooks system, plugins, permission, unified memory,
ACP/A2A protocols, WorkspaceContext, personalization, cron, session mgmt.

Our changes preserved on top:
- tool_manager: connect() try/except fault tolerance
- tool_manager: structured JSON error responses with recovery hints
- tool_manager: output truncation (20000 chars max)
- llm_agent: rollback() returning (success, truncated_messages) tuple
- llm_agent: _apply_pending_rollback in run loop
- local_code_executor: timeout schema, call_id rename, _prepare_shell_command
- filesystem_tool: streaming grep, raw content read_file
- 14 files: from __future__ import annotations (Python 3.9 compat)
… 70.8%)

Fixed 10 framework-level obstacles that were unnecessarily blocking the agent:

1. cd+write compound: track cwd through cd commands instead of rejecting all cd+write combos
2. Shell variable expansion: only flag $VAR/${VAR}/$(cmd), not bare $ or $2 (awk)
3. Python 3.9 compat: add `from __future__ import annotations` to 30 files
4. sed {} expressions: relax from deny to ask
5. read_file paths: check both `paths` (list) and `path` (string) in safety check
6. Command max length: increase from 8192 to 32768 in benchmark YAML
7. Shell comment parsing: skip #-prefixed lines before shlex.split
8. Zsh = expansion: only flag =( not bare =
9. Glob in file content: skip heuristic for code-like content (parentheses/long)
10. RepetitionGuard: threshold 3→5, max_warnings 2→3

Result: 63/89 (70.8%) vs previous 54/89 (60.7%), +9 tasks recovered.
CommandValidator = Callable[[list[str]], str | None] crashes on Python 3.9
because `from __future__ import annotations` does not affect runtime type
aliases (only function/variable annotations). This caused qemu-startup
and qemu-alpine-ssh to fail at import time in Python 3.9 Docker containers.

These two tasks have never passed in any benchmark run due to this bug.
suluyan added 2 commits July 8, 2026 15:28
One-command workflow for TB 2.1 benchmarks:
  sync   — rsync code to remote, clean macOS artifacts, verify compile
  run    — launch full or selective benchmark via nohup
  status — check trial count and pass/fail rate
  results — list all pass/fail tasks
  compare — diff two runs showing recovered/regressed tasks
Improve repetition guard config parsing, snapshot restore when git is
missing, ripgrep hyphen patterns, edit_file path validation, and sub-agent
transcript serialization for dict messages.

Co-authored-by: Cursor <cursoragent@cursor.com>
@@ -1,3 +1,4 @@
from __future__ import annotations

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个 PR 里有比较多的脚本在头部新增了 from __future__ import annotations,主要解决的问题是?如果暂时没有类型注解延迟求值需求的话是不是可以先不加这些。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

主要解决兼容py3.9的问题

Comment thread ms_agent/self_improve/classifier.py Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个模块目前的状态是完整的吗?看起来互相调用的一批文件还没有提交。

# Find cls which base class is `ToolBase`
if issubclass(cls, ToolBase) and cls.__module__ == _plugin:
self.register_tool(cls(self.config))
# Used temporarily during async initialization; the actual client is managed in self.servers

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里做顺序调整是主要是为了做什么处理吗

DEFAULT_OUTPUT_WRAPPER = ['<result>', '</result>']

DEFAULT_RETRY_COUNT = 3
DEFAULT_RETRY_COUNT = 5

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增加重试次数主要是为了处理什么 case?

@suluyana suluyana Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

解决 API 调用因瞬时错误(rate limit、网络超时、502/503)导致任务失败的问题。与这个同时添加的还有retry的delay从1s改到3s

suluyan added 2 commits July 9, 2026 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants