Repository navigation
feat(agent-cli): report tool-call progress events for CLI agents and the swe-atlas loop - #148
jamespsterling wants to merge 9 commits into
Conversation
|
I'll fix CI failures and address comments from users with write access that start with 'Devin'.
Original prompt from James
|
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Perry's Review
Verdict: 🔧 Changes requested
Review notes
Re-review after the two fix commits (f140274, f9a73b5) that addressed my earlier truncation and index findings. Both are genuinely fixed: truncateCommand is now exported from the harbor agent loop and applied in reportShellCommands, and toolCallIndex: index + 1 makes the CLI path 1-based like the live harbor loop. Devin's zero-index bug is closed.
The change is additive telemetry and does not touch sandbox execution, network policy, prompts, or scoring, so "no benchmark impact" holds.
What checks out. extractShellCommands is defensively written (non-record args, non-string and empty commands all filtered) with three tests covering the Claude tool_use and pi tool_execution_start shapes plus malformed input. serviceOption(ProgressReporter) is the correct lookup for runAgentCli, which is reachable from contexts with no reporter layer installed. The swe-atlas onStep wiring matches deep-swe's pattern exactly, and fullCommand carrying the untruncated command is the right call — the truncation is for display, and the consumer classifies a URL that a 200-char cut would mangle.
Why changes are requested
-
prime-agent almost certainly still reports nothing. The shell-tool allowlist accepts only
bashandBash, yet the repo's own captured prime-agent event stream — thePRIME_AGENT_STREAMfixture in the terminal-bench solver tests — emitstoolName: "ipython"for its only execution tool. Under that shapeextractShellCommandsreturns[]for every prime run, so one of the five agents the description claims to fix keeps the exact blind spot this PR exists to close. Either prime exposes abashtool the fixture does not capture, or the allowlist needs the name prime actually uses. -
No test covers the new runner wiring.
extractShellCommandsis well tested in isolation, but nothing asserts thatreportShellCommandsruns afterparseRunand emits onetool-callper command with the truncation applied — which is precisely the defect the first review found and the fix commits repaired. The repo's contributor guide asks for regression coverage on behavior changes.
Open question, not blocking: fullCommand now forwards agent shell commands verbatim into telemetry, and agent commands routinely carry secrets (pip install --index-url https://user:token@…, curl -H "Authorization: Bearer sk-…"). Flagging because the checklist asserts no credentials are included.
Prior threads: both of my earlier findings are resolved and their fixes are verified in the diff; Devin's index bug is resolved. CI is green (validate, CodeQL, Analyze — all SUCCESS; Devin Review reports nothing outstanding).
Risk: 🟡 Medium
Risk assessment:
| Dimension | Severity | Risk | Reasoning |
|---|---|---|---|
| Implementation risk | 🟨🟨 | Medium | The extraction and wiring are correct for claude/pi/omp; prime-agent's tool name is unverified and likely not matched. |
| Premise risk | 🟨🟨 | Medium | The claim that all five CLI agents now report is contradicted by the repo's own prime-agent fixture. |
| Estimated impact | 🟨🟨 | Medium | A monitor built on bench-agent-network-command silently misses every prime-agent run — the blind spot the PR targets. |
| Risk Factor | Severity | Risk | Reasoning |
|---|---|---|---|
| Reversibility | 🟩 | Low | Additive telemetry; a revert restores prior behavior completely. |
| Detectability | 🟥🟥🟥 | High | A missing tool name produces no error — the monitor just reports fewer commands. |
| Blast radius | 🟩 | Low | Observability only; no scoring, execution, or network-policy change. |
| Data integrity | None | No persisted state touched. | |
| Financial exposure | None | No billing or accounting surface. | |
| Security and privacy exposure | 🟨🟨 | Medium | Untruncated agent commands now reach telemetry, and those strings can embed credentials. |
| Propagation | 🟨🟨 | Medium | Downstream Datadog metric, span, and log line absorb the payload shape. |
| Availability | None | No serving path affected. | |
| Recovery cost | 🟩 | Low | Add the tool name, redeploy. |
| Time to correct | 🟨🟨 | Medium | A silently incomplete signal can go unnoticed until someone audits coverage. |
What these risk factors mean
Each factor rates the potential consequence if anything about the change is wrong, from None to Critical between these two ends:
- Reversibility — None: the previous state can be fully restored; Critical: what was lost or altered cannot be restored by any later action.
- Detectability — None: any problem is caught immediately by tests, alerts, or plain observation; Critical: a problem produces no signal and is found only after harm accumulates.
- Blast radius — None: a single contained component; Critical: every user, organization, or dependent system is affected.
- Data integrity — None: no persisted state is touched; Critical: persisted data is corrupted, lost, or made untrustworthy.
- Financial exposure — None: no effect on billing or accounting outcomes; Critical: the business or its customers lose money through incorrect charges, missed revenue, or unrecoverable spend.
- Security and privacy exposure — None: nothing sensitive is reachable; Critical: credentials, personal data, or tenant isolation are compromised.
- Propagation — None: nothing else reads or builds on the affected output; Critical: downstream systems absorb an error and carry it forward.
- Availability — None: the change cannot affect whether anything serves; Critical: a relied-on path stops serving.
- Recovery cost — None: a rollback completes the recovery; Critical: correction requires manual reconciliation or customer remediation.
- Time to correct — None: corrected as soon as noticed; Critical: stays in effect through long investigation or deployment cycles.
…d with display and full text Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Perry's Review
Verdict: 💬 Comments / questions (approved in substance — verdict downgraded because the maintainer app lacks pull_requests:write on OpenRouterTeam)
Review notes
Third pass on this PR. The delta since my last review is one commit (caccd3a), plus description text addressing both open threads. I verified the new commit and re-checked both rebuttals against the source rather than taking them at face value — both hold.
The two open findings are closed.
-
prime-agent tool name — accepted, no change needed. The author's reasoning is correct and I confirmed it in the code: prime's only execution tool in the repo's captured stream is
ipython, and that tool carries Python cell source in its args, not acommand. AddingipythontoSHELL_TOOL_NAMESwould feed Python source to a shell-command classifier, which finds nothing useful in it and would poison the metric with false the classifier can't act on. Excluding it is the right call; the PR description now lists only the agents with a real shell tool (pi, ori, claude, omp) and names prime as an explicit follow-up needing a Python-aware classifier on the consumer side. The earlier "every CLI agent" overclaim is gone, which is what the finding actually asked for. -
Test coverage — fixed in
caccd3a, and it covers the right thing. The new terminal-bench test drives the claude harness end to end throughrunOriSolverwith a fake sandbox, a stream whose onlytool_useis aBashblock over 200 characters, and a recordingProgressReporterinstalled in place of the noop layer. It asserts exactly onetool-callevent carryingcommandtruncated at 200 +…,fullCommandequal to the input,toolCallIndex: 1, and the correctsampleId/epochfrom run context. That is the regression the first round found andf140274/f9a73b5fixed, now pinned.
fullCommand — the security question resolves in the code's favour. I re-checked the claim that the full command never leaves the process: fullCommand appears only as an interface field, the three construction sites, and test expectations — no log call, no checkpoint field (CheckpointDataSchema has no such key), no heartbeat. The 200-char command is what any display path would use. The field is deliberately the untruncated string so the openrouter-web classifier can recover a host from a URL that truncation would cut in half, and that consumer emits only a closed set of values. Keeping it required is fine given the vendored-subtree coupling and the fullCommand ?? command fallback already in the interim worker code. Worth a follow-up only if the harness ever gains a path that persists events wholesale.
What checks out. extractShellCommands stays defensive (non-record args, non-string and empty commands filtered) with malformed-input coverage. serviceOption(ProgressReporter) keeps runAgentCli context-free, so standalone CLI runs are unaffected. The swe-atlas onStep wiring mirrors deep-swe exactly, and the post-run CLI events correctly reuse the command index for step/toolCallIndex since the stream carries no turn boundaries there — now documented in the description.
CI is green (validate, CodeQL, Analyze — all SUCCESS). Both of my earlier threads are resolved and their fixes verified in the diff; Devin's zero-index bug is resolved. The two unchecked checklist boxes (dataset provenance, docs) don't apply to an observability-only change with no dataset or user-facing surface.
Risk: 🟢 Low
Risk assessment:
| Dimension | Severity | Risk | Reasoning |
|---|---|---|---|
| Implementation risk | 🟩 | Low | Extraction and wiring are correct for every harness with a shell tool, and the new end-to-end test pins the truncation and index behavior. |
| Premise risk | 🟩 | Low | The description now matches the code: prime is documented as a follow-up rather than claimed as fixed. |
| Estimated impact | 🟩 | Low | Worst case is an incomplete telemetry signal for one agent that already lacks it today — no behavior regresses. |
| Risk Factor | Severity | Risk | Reasoning |
|---|---|---|---|
| Reversibility | 🟩 | Low | Additive telemetry; a revert fully restores prior behavior. |
| Detectability | 🟨🟨 | Medium | A future drop in emitted events produces no error, though the new test now covers the wiring. |
| Blast radius | 🟩 | Low | Observability only; no scoring, execution, or network-policy change. |
| Data integrity | None | No persisted state touched. | |
| Financial exposure | None | No billing or accounting surface. | |
| Security and privacy exposure | 🟩 | Low | fullCommand stays in process — never logged, checkpointed, or heartbeated — and the consumer emits only a closed set of values. |
| Propagation | 🟩 | Low | The one consumer ships in lockstep with a fullCommand ?? command fallback. |
| Availability | None | No serving path affected. | |
| Recovery cost | 🟩 | Low | A revert or a follow-up classifier both land cheaply. |
| Time to correct | 🟩 | Low | Any gap is a telemetry follow-up, not a live defect. |
What these risk factors mean
Each factor rates the potential consequence if anything about the change is wrong, from None to Critical between these two ends:
- Reversibility — None: the previous state can be fully restored; Critical: what was lost or altered cannot be restored by any later action.
- Detectability — None: any problem is caught immediately by tests, alerts, or plain observation; Critical: a problem produces no signal and is found only after harm accumulates.
- Blast radius — None: a single contained component; Critical: every user, organization, or dependent system is affected.
- Data integrity — None: no persisted state is touched; Critical: persisted data is corrupted, lost, or made untrustworthy.
- Financial exposure — None: no effect on billing or accounting outcomes; Critical: the business or its customers lose money through incorrect charges, missed revenue, or unrecoverable spend.
- Security and privacy exposure — None: nothing sensitive is reachable; Critical: credentials, personal data, or tenant isolation are compromised.
- Propagation — None: nothing else reads or builds on the affected output; Critical: downstream systems absorb an error and carry it forward.
- Availability — None: the change cannot affect whether anything serves; Critical: a relied-on path stops serving.
- Recovery cost — None: a rollback completes the recovery; Critical: correction requires manual reconciliation or customer remediation.
- Time to correct — None: corrected as soon as noticed; Critical: stays in effect through long investigation or deployment cycles.
…ommands Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…t-tool-call-progress # Conflicts: # src/benchmarks/agent-cli/harness.test.ts
TL;DR
Every benchmark path whose agent runs shell commands now reports each command as a
tool-callprogress event, so the bench worker can observe agent network activity in Datadog. Sandbox execution and network access are unchanged.What changed?
src/benchmarks/agent-cli/harness.ts: newextractShellCommands(responseItems)reads Claude Codetool_useblocks namedBashand pi-familytool_execution_startevents namedbashfrom the parsed agent stream.src/benchmarks/agent-cli/runner.ts: afterparseRun,runAgentClireports onetool-callevent per shell command through theProgressReporterwhen the solver runs under one (serviceOption, so the function signature and tests are unchanged). CLI agents run to completion before the harness reads their stream, so these events arrive after the run, in stream order.src/benchmarks/swe-atlas/solver.ts: the liverunAgentLooppath now passesonStepto the reporter, matchingdeep-swe.src/harness/progress.ts:AgentToolCallEventcarriesfullCommand(the whole command) next to the 200-character displaycommand. The field stays in process: the harness never logs, heartbeats, or checkpoints it, and the only consumer is the vendoredpackages/bench-harnesssubtree in openrouter-web, which compiles this source and emits only fixed values (tool, host class, known host orother), never the command text.src/benchmarks/agent-cli/harness.test.ts: tests forextractShellCommands.src/benchmarks/terminal-bench/ori-solver.test.ts: a solver run with a 260-characterBashtool call asserts onetool-callevent with the truncatedcommandand the fullfullCommand.Why?
Only
deep-swe,harbor,search, andwandrreported tool calls.swe-atlas,terminal-bench, and the CLI agents with a shell tool (pi, ori, claude, omp) did not, so a Datadog monitor on agent network commands would miss most runs. The consumer isemitNetworkCommandTelemetryin openrouter-web, which classifies each command into fixed values and emits a metric, a span, and a log line.How to test
bun test src/benchmarks/agent-cli src/benchmarks/swe-atlas. Expect all tests to pass, including the threeextractShellCommandscases.bench-agent-network-commandlog lines for anycurl,wget,git clone, orpip installthe agent ran.Benchmark impact
None. The change only reports commands that already ran; it does not change prompts, sandbox settings, network access, or scoring.
Reviewer focus
extractShellCommandsonly readsBashandbashtool names. Other tools (read,edit,ipython) are not shell commands and are skipped.prime-agentexecutes throughipython(Python cells), so it reports nothing here; covering it needs a Python-aware classifier on the consumer side and is a follow-up.stepandtoolCallIndexbecause the CLI stream does not expose turn boundaries at that point.Checklist
Link to Devin session: https://openrouter.devinenterprise.com/sessions/36bad2ac0e464803a3828b416c3a7274
Open in Devin Desktop: https://openrouter.devinenterprise.com/desktop/session/36bad2ac0e464803a3828b416c3a7274?variant=devin
Requested by: @jamespsterling