fix: resolve agents by pid or name#2666
Conversation
Greptile code reviewThis repo uses Greptile for automated review. Before merge, aim for Confidence Score: 5/5 with zero unresolved review threads — see CONTRIBUTING.md. Run a review — add a PR comment with: Give it ~5-10 minutes (sometimes longer) for results, then fix feedback and re-trigger until you reach Confidence Score: 5/5. Optional: automate with the greploop skill. |
Greptile SummaryThis PR adds name-based target resolution for
Confidence Score: 3/5The production code changes are straightforward and localized; the test suite has a reliability gap that could mask regressions on developer machines. The Both test cases named Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["User input: /agents kill|trace <arg>"] --> B["_resolve_agent_arg(arg, registry)"]
B --> C{"int(arg) succeeds?"}
C -- "Yes, pid > 0" --> D["return pid, registry.get(pid)"]
C -- "Yes, pid <= 0" --> E["name scan: registry.list()"]
C -- "No (ValueError)" --> E
E --> F{"matches count"}
F -- "== 1" --> G["return record.pid, record"]
F -- "> 1" --> H["raise ValueError: ambiguous name"]
F -- "== 0 AND pid is not None" --> I["return pid, None\n(negative/zero PID passthrough)"]
F -- "== 0 AND pid is None" --> J["raise ValueError: unknown name"]
D --> K["downstream: terminate / attach"]
G --> K
I --> K
K --> L{"pid == os.getpid()?"}
L -- "Yes" --> M["Error: refusing to kill self"]
L -- "No" --> N["Execute operation"]
Reviews (1): Last reviewed commit: "fix: resolve agents by pid or name" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def test_unknown_name_rejected(self) -> None: | ||
| sess_obj = ReplSession() | ||
| console, buf = _capture() | ||
| assert dispatch_slash("/agents trace abc", sess_obj, console) is True | ||
| out = buf.getvalue().lower() | ||
| assert "invalid pid" in out | ||
| assert "invalid pid or unknown agent name" in out | ||
| assert sess_obj.history[-1]["ok"] is False |
There was a problem hiding this comment.
test_unknown_name_rejected
Both TestAgentsTrace.test_unknown_name_rejected (line 509) and TestAgentsKill.test_unknown_name_rejected (line 699) call AgentRegistry() through the un-patched default constructor, which reads from ~/.opensre/agents.jsonl. If the developer's real registry contains an agent named "abc" or "missing-agent", _resolve_agent_arg will resolve it to a live PID instead of raising ValueError, the error message check ("invalid pid or unknown agent name") will fail, and the test turns into a false positive. Every other new test that exercises the name-resolution path calls _isolate_registry(monkeypatch, tmp_path / "agents.jsonl") before registering records — these two error-path tests should do the same.
| if pid is not None and pid > 0: | ||
| return pid, registry.get(pid) | ||
|
|
||
| matches = [record for record in registry.list() if record.name == text] | ||
| if len(matches) == 1: | ||
| record = matches[0] | ||
| return record.pid, record | ||
| if len(matches) > 1: | ||
| pids = ", ".join(str(record.pid) for record in sorted(matches, key=lambda r: r.pid)) | ||
| raise ValueError( | ||
| f"ambiguous agent name {text!r}: {len(matches)} registered agents (pids: {pids})" | ||
| ) | ||
|
|
||
| if pid is not None: | ||
| return pid, None | ||
| raise ValueError(f"invalid pid or unknown agent name: {text}") |
There was a problem hiding this comment.
When text is a positive integer string (e.g. "1234"), the function returns on line 147 without ever checking the name index, so an agent registered as name="1234" can never be targeted by name — the argument is always treated as a raw PID. Conversely, an agent named "0" or "-5" can be targeted by name because those fall through to the registry.list() scan. This is noted in the docstring, but the asymmetry may surprise users who name their agents with numeric strings and receive no error — they just silently hit whichever process owns that PID.
|
Closing for now. Please wait to get assigned before opening a pr, and avoid opening multiple prs at the same time as a first-time contributor. |
2 participants
Summary
/agentstarget resolution for PID or registered agent name/agents killand/agents traceto accept<pid|name>Test
uv run ruff check --fix app/cli/interactive_shell/command_registry/agents.py tests/cli/interactive_shell/test_agents_commands.pyuv run ruff format app/cli/interactive_shell/command_registry/agents.py tests/cli/interactive_shell/test_agents_commands.pyuv run pytest tests/cli/interactive_shell/test_agents_commands.py -qCloses #1749