Skip to content

feat(agents): add external agent runner - #14

Merged
liupeirong merged 5 commits into
mainfrom
feat/f09-external-agent-runner
Jun 5, 2026
Merged

feat(agents): add external agent runner#14
liupeirong merged 5 commits into
mainfrom
feat/f09-external-agent-runner

Conversation

@liupeirong

Copy link
Copy Markdown
Owner

No description provided.

liupeirong and others added 2 commits June 5, 2026 07:22
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Generated by Agentic PR Review for issue #14 · opus46 3.9M

Comment thread src/vimai/cli.py
Comment on lines +45 to +51
result = subprocess.run(
[str(wrapper), "--prompt-file", str(prompt_path)],
capture_output=True,
check=False,
encoding="utf-8",
text=True,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reliabilitysubprocess.run is called without a timeout parameter. A misbehaving or hanging external agent will block vimai indefinitely with no way for the user to recover (Vim will appear frozen).

Consider adding a generous default timeout (e.g. 120s) or making it configurable:

result = subprocess.run(
    [str(wrapper), "--prompt-file", str(prompt_path)],
    capture_output=True,
    check=False,
    encoding="utf-8",
    text=True,
    timeout=120,
)

Catch subprocess.TimeoutExpired alongside the existing OSError handler and raise a clear ExternalAgentError.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

liupeirong and others added 2 commits June 5, 2026 14:06
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown

✅ Looks Good

No new issues found. The external agent runner is well-implemented and production-ready.

Key strengths

  • Security: Agent name regex prevents path traversal; no shell=True; prompt passed via temp file
  • Reliability: 120s subprocess timeout; temp file cleanup in finally
  • Correctness: Clear fallback flow (prompt-only → external → error); user-facing error messages
  • Testing: Comprehensive coverage across wrapper discovery, timeout, cleanup, error paths, and CLI routing

Previously raised items

Thread Status
UX regression on missing env var (cli.py) Resolved by reviewer
Subprocess timeout (external.py) ✅ Fixed — 120s timeout implemented

No actionable findings to add.

Generated by Agentic PR Review for issue #14 · opus46 4.9M ·

@liupeirong
liupeirong merged commit 9d2ac68 into main Jun 5, 2026
7 checks passed
@liupeirong
liupeirong deleted the feat/f09-external-agent-runner branch June 5, 2026 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant