feat(agents): observe remaining provider activity - #154
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughLocal agent adapters now accept observers and report session IDs, token usage, and normalized tool activity for OpenCode, ACP, and Pi events. Tests cover completed, running, and failed activity states. ChangesLocal agent activity observation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderAdapter
participant ObservationHelper
participant Observer
ProviderAdapter->>ObservationHelper: provider result or event
ObservationHelper->>Observer: normalized session, usage, or tool activity
Observer-->>ProviderAdapter: callback completes
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR adds session, activity, and token-usage observation for the OpenCode, ACP, and Pi local-agent adapters.
Confidence Score: 4/5The PR should not merge until resumed OpenCode sessions stop attributing historical tool calls to the current workflow-agent call. OpenCode reads the complete reused provider session and emits every historical tool part through an append-only observer, causing duplicated and incorrectly attributed activity on resumed runs. Files Needing Attention: src/local-agent-adapters.ts, src/local-agent-adapters.test.ts
|
| Filename | Overview |
|---|---|
| src/local-agent-adapters.ts | Adds observer integration and provider-specific event normalization, but OpenCode replays historical tool activity when a provider session is resumed. |
| src/local-agent-adapters.test.ts | Adds basic observation-shape coverage, but does not cover resumed OpenCode sessions containing historical tool calls. |
Sequence Diagram
sequenceDiagram
participant Runtime
participant Adapter
participant Provider
participant Observer
participant Store
Runtime->>Adapter: run(input, observer)
Adapter->>Provider: create/resume session and prompt
Provider-->>Adapter: updates, messages, and usage
Adapter->>Observer: onSession(sessionId)
Adapter->>Observer: onActivity(activity)
Adapter->>Observer: onUsage(snapshot)
Observer->>Store: append activity / update usage
Reviews (1): Last reviewed commit: "feat(agents): observe remaining provider..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
20b2ad2 to
7f310ed
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/local-agent-adapters.test.ts (1)
23-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
onUsagecallback payload.The test checks only
observeOpenCodeResultreturn data. It does not checkobservedUsage. A regression that stops callingobserver.onUsagecan pass this test.Proposed test addition
assert.equal(openCodeUsage?.totalTokens, 1_250); +assert.deepEqual(observedUsage.shift(), { + inputTokens: 1_000, + cachedInputTokens: 400, + cacheCreationInputTokens: 50, + outputTokens: 250, + totalTokens: 1_250, + state: "final", +}); assert.deepEqual(observedActivity.shift(), {As per coding guidelines, verify the actual user-consumption path and state when only a narrower proxy was verified.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/local-agent-adapters.test.ts` around lines 23 - 44, Extend the test around observeOpenCodeResult to assert the observedUsage callback payload, not just the returned totalTokens value. Verify observedUsage contains the expected LocalAgentUsageSnapshot derived from the input token data, ensuring observer.onUsage is actually invoked while preserving the existing activity assertion.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/local-agent-adapters.ts`:
- Around line 374-377: Update the ACP adapter result near lines 374-377 in
src/local-agent-adapters.ts to retain the final tokenUsage snapshot and include
it as usage in the returned LocalAgentRunResult, while preserving observer
notifications. In the Pi adapter flow at lines 510-539 of the same file, track
the latest usage snapshot and return it as usage alongside the generated text.
Trace and update affected persistence and tool-surface contracts so adapter
callers receive and retain provider-reported usage consistently.
---
Nitpick comments:
In `@src/local-agent-adapters.test.ts`:
- Around line 23-44: Extend the test around observeOpenCodeResult to assert the
observedUsage callback payload, not just the returned totalTokens value. Verify
observedUsage contains the expected LocalAgentUsageSnapshot derived from the
input token data, ensuring observer.onUsage is actually invoked while preserving
the existing activity assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d39b1684-d1a3-4200-b9e6-811573f023f3
📒 Files selected for processing (2)
src/local-agent-adapters.test.tssrc/local-agent-adapters.ts
| const response = await prompt; | ||
| const usage = tokenUsage(asRecord(response.usage), "final"); | ||
| if (usage) observer?.onUsage?.(usage); | ||
| return textParts.join("").trim(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Retain provider-reported usage in every adapter result.
ACP and Pi send usage only through an optional observer. OpenCode also stores it in LocalAgentRunResult. This difference loses reported usage for direct adapter callers and result-based persistence.
src/local-agent-adapters.ts#L374-L377: Save the final ACP snapshot and return it asusage.src/local-agent-adapters.ts#L510-L539: Track the latest Pi snapshot and return it asusage.
As per coding guidelines, trace all affected persistence and tool-surface contracts for cross-cutting changes.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
📍 Affects 1 file
src/local-agent-adapters.ts#L374-L377(this comment)src/local-agent-adapters.ts#L510-L539
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/local-agent-adapters.ts` around lines 374 - 377, Update the ACP adapter
result near lines 374-377 in src/local-agent-adapters.ts to retain the final
tokenUsage snapshot and include it as usage in the returned LocalAgentRunResult,
while preserving observer notifications. In the Pi adapter flow at lines 510-539
of the same file, track the latest usage snapshot and return it as usage
alongside the generated text. Trace and update affected persistence and
tool-surface contracts so adapter callers receive and retain provider-reported
usage consistently.
Source: Coding guidelines
1 participant
This final layer extends workflow visibility to OpenCode, Pi, Cursor, and Copilot. It normalizes OpenCode current-turn message parts, Pi RPC events, and ACP tool updates while preserving provider differences: optional usage is stored only when reported, resumed OpenCode history is not reattributed, and unavailable token data stays unavailable rather than being estimated.\n\nVerified on the complete stack with npm test, npm run build, and a compiled CLI render against the large phased-running SQLite fixture.\n\nDepends on #153.
Summary by CodeRabbit