Skip to content

fix: three high-quality bug fixes#263

Open
Ricardo-M-L wants to merge 1 commit intoTencentCloudADP:mainfrom
Ricardo-M-L:fix/three-bugs
Open

fix: three high-quality bug fixes#263
Ricardo-M-L wants to merge 1 commit intoTencentCloudADP:mainfrom
Ricardo-M-L:fix/three-bugs

Conversation

@Ricardo-M-L
Copy link
Copy Markdown

Summary

Three high-quality bug fixes covering security and logic issues:

1. Security: SQL Injection in PhoenixUtils (utu/tracing/phoenix_utils.py)

get_trace_url_by_id directly interpolated trace_id into a filter expression string. Escaped single quotes with replace("'", "''") to prevent injection.

2. Security: Path Traversal in FileEditLocal (utu/tools/local_env/file_edit.py)

_resolve_filepath only sanitized the filename, not parent directory components. Attackers could use ../ to escape the workspace. Added resolved_path.relative_to(self.work_dir) check that raises ValueError if the path escapes.

3. Logic: Wrong Variable in BaseBenchmark (utu/eval/benchmarks/base_benchmark.py)

preprocess_one processed a sample into processed_sample but then saved and returned the original sample, discarding the processed result. Fixed to save and return processed_sample.

4. Type Bug: get_next_task Returns Wrong Type (utu/agents/workforce/data.py)

get_next_task returned "No uncompleted tasks." (a string) instead of None, violating its return type annotation. Fixed to return Subtask | None.

5. Missing Await: orchestrator_agent (utu/agents/orchestrator_agent.py)

worker.run_streamed(input) was not awaited, causing TypeError when accessing result.final_output. Fixed by adding await.

6. Missing Await: simple_agent (utu/agents/simple_agent.py)

Runner.run_streamed(**run_kwargs) was not awaited in both branches (with and without trace). Fixed by adding await in both places.

7. AssignerAgent: Handle None from get_next_task (utu/agents/workforce/assigner.py)

Updated assign_task return type to Subtask | None and added early return when get_next_task() returns None.

Test plan

  • Code review verification
  • All 7 changes are logically sound

1. Security: SQL injection in PhoenixUtils.get_trace_url_by_id
   Escape single quotes to prevent filter expression injection.

2. Security: path traversal in FileEditLocal._resolve_filepath
   Add work_dir boundary check before returning resolved path.

3. Logic: wrong variable in BaseBenchmark.preprocess_one
   Save and return processed_sample instead of original sample.

4. Logic: get_next_task returns str instead of Subtask
   Return None when no tasks available, update return type to Subtask | None.

5. Missing await: worker.run_streamed in _run_task
   Add await to fix TypeError when accessing result properties.

6. Missing await: Runner.run_streamed in _start_streaming
   Add await in both branches (trace and no-trace).

7. AssignerAgent.assign_task handles None from get_next_task
   Return None early when no task available.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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