Skip to content

Fix/general run#917

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

Fix/general run#917
suluyana wants to merge 58 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 and others added 28 commits April 3, 2026 11:37
Add SubAgentStreamWriter (ms_agent/utils/stream_writer.py) that appends
each new message to a JSONL file as soon as it arrives, so the parent
agent or an external observer can tail -f to watch a sub-agent run
step-by-step instead of waiting for it to finish.

Key details:
- JSONL format: header -> message* -> footer, one JSON object per line
- Deduplication via last_written_count: each chunk carries the full
  accumulated history; only newly added messages are written
- Thread-safe (threading.Lock) and flush-on-every-line for tail -f support
- Works for both inline-async and subprocess execution paths
- event_queue is now created when either _chunk_cb or the writer is active
- Opt-in via config: agent_stream_file: true (or
  tools.agent_tools.enable_stream_file: true)
- File path: {output_dir}/subagents/{call_id}.stream.jsonl
- A descriptive note is appended to the tool result so the parent LLM
  understands the file is an incremental execution trace, not tool output

Also includes AgentTool refactor: replace ThreadPoolExecutor with native
asyncio subprocess spawning, add sync_timeout_s + escape-to-background
support, TaskControlTool improvements, and related smoke tests.

Entire-Checkpoint: 37377e309a88
Add WorkspacePolicyKernel (allow-roots from output_dir), ArtifactManager for large shell outputs, TaskManager with shell background support and asyncio process kill, WorkspaceSearchTool (grep_files/glob_files). Wire TaskManager into LLMAgent (prepare_tools, cleanup, task notifications in step) and extend LocalCodeExecutionTool with policy checks, artifact spill, run_in_background shell, sh -lc wrapping. ToolManager registers WorkspaceSearchTool by default and injects __call_id for shell_executor. Add tests for workspace policy. Document implementation map in shell-grep-glob-workspace-policy.md.

Made-with: Cursor
…ools

Replace removed file_system tools (list_files, delete_file_or_dir) with workspace_search (glob/grep) and/or code_executor (shell, file_operation). Update deep_research prompts and callbacks for read_file offset/limit and edit_file. fin_research: aggregator adds python_env shell/file_operation; collector exposes shell and file_operation in sandbox; file_system keeps read/write/edit. code_genesis: prompts use glob_files/shell for listing; orchestrator_callback uses os.makedirs instead of removed create_directory(). singularity registers workspace_search.

Made-with: Cursor
Remove WorkspaceSearchTool and register grep and glob on the file_system
server alongside read/write/edit. Add read/edit/write include aliases and
optional grep_head_limit, glob_max_files, and grep_timeout_s on file_system.

Update project YAML and prompts to drop workspace_search blocks; document
the mapping in shell-grep-glob-workspace-policy.md. Add tests for include
aliases and grep/glob filtering.

Made-with: Cursor
Add Tavily HTTP client, search/extract schema, WebSearchTool integration,
optional large-result spill, researcher/searcher Tavily YAML presets, and
run_benchmark env hooks for RESEARCHER_CONFIG / BENCH paths.

Made-with: Cursor
…ack)

WebSearchTool imports fetch_single_text_with_meta; add tiered fetch helpers
and optional Playwright fallback module used by jina_reader.

Made-with: Cursor
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 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 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 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 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')

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.

1 participant