feat(advisor): render /advisor calls as tool rows with token accounting#209
feat(advisor): render /advisor calls as tool rows with token accounting#209WesleyMFrederick wants to merge 1 commit into
Conversation
- Render /advisor tool calls as standard tool rows (matt1398#175) - Count advisor output tokens; label empty input; surface in context panel - Add eval-report display path and tool-row improvements - Add tool-name string utilities (stringUtils) used by linking and summaries - Extend jsonl/messages types for advisor_tool_result and server_tool_use - Guard block.content.text; harden test literal Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the advisor server tool, handling server_tool_use and advisor_tool_result content blocks across the backend parsing, semantic step extraction, and frontend rendering layers. It ensures that advisor tool calls (which have empty inputs) do not synthesize call tokens, carries over the advisorModel as the source model, and capitalizes tool names in the UI. It also adds helper scripts to package.json for managing the standalone server process, along with comprehensive unit tests. There are no review comments to address, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
📝 WalkthroughWalkthroughThis PR adds end-to-end support for an "advisor" server tool flow: new JSONL content types encode advisor calls and results, messages carry ChangesAdvisor Tool Flow Support
Standalone Mode Process Management
Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/services/analysis/SemanticStepExtractor.ts (1)
93-109: 💤 Low valueStructural inconsistency: advisor tool_call omits
tokensfield.Regular
tool_useblocks (lines 73-90) set atokensfield with{ input: callTokens, output: 0 }, but the advisor tool_call step omits this field entirely. While test line 44 confirms this is intentional, the structural asymmetry could cause issues if downstream code expects alltool_callsteps to have consistent shape.Consider either:
- Explicitly setting
tokens: undefinedortokens: { input: 0, output: 0 }for structural consistency- Adding a comment explaining why advisor diverges from the standard tool pattern (e.g., "advisor calls bypass normal token estimation")
🤖 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/main/services/analysis/SemanticStepExtractor.ts` around lines 93 - 109, The advisor branch in SemanticStepExtractor (the if checking block.type === 'server_tool_use' && block.name === 'advisor') creates a tool_call step but omits the tokens field, causing structural inconsistency with the other tool_use handling; update that step object to include a tokens property (e.g., tokens: { input: 0, output: 0 } or tokens: undefined explicitly) so all tool_call steps share the same shape, or alternatively add a brief inline comment in the advisor branch explaining why tokens are intentionally omitted (e.g., "advisor calls bypass normal token estimation")—modify the step creation inside the advisor conditional to implement this change.test/mocks/advisorBlocks.fixture.ts (1)
1-68: ⚡ Quick winOutdated comment and potentially unnecessary type cast.
Line 17 states "Cast needed until M2 adds advisorModel to ParsedMessage," but according to the
ParsedMessagetype definition (context snippet from messages.ts:66-106),advisorModelis already an optional field. The comment appears outdated.Additionally, the
as unknown as ParsedMessagedouble-cast on lines 39 and 67 suggests a type mismatch that might be resolvable. If the cast is genuinely needed due to content block type definitions, consider updating the comment to explain the actual reason.🤖 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 `@test/mocks/advisorBlocks.fixture.ts` around lines 1 - 68, The comment claiming "Cast needed until M2 adds advisorModel to ParsedMessage" is outdated and the double-cast "as unknown as ParsedMessage" is likely unnecessary; update or remove that comment and fix typing for advisorCallMessage and advisorResultMessage by either (a) typing the objects directly as ParsedMessage (remove the double-cast) if ParsedMessage already has advisorModel?: string, or (b) narrow the content block types so the messages satisfy ParsedMessage without casting (or replace the double-cast with a single explicit cast only if truly unavoidable). Refer to the exported symbols advisorCallMessage, advisorResultMessage, and the ParsedMessage/advisorModel field when making the change.package.json (1)
49-49: ⚡ Quick winConsider graceful shutdown instead of forced termination.
Using
kill -9(SIGKILL) immediately terminates the process without allowing cleanup. This prevents graceful shutdown of HTTP connections, file handles, or other resources.Best practice is to try
kill -15(SIGTERM) first, wait briefly, then escalate tokill -9if the process doesn't exit.🛠️ Suggested graceful kill script
-"standalone:kill": "PIDS=$(lsof -ti :3456); [ -n \"$PIDS\" ] && kill -9 $PIDS || true", +"standalone:kill": "PIDS=$(lsof -ti :3456); if [ -n \"$PIDS\" ]; then kill -15 $PIDS; sleep 2; kill -9 $PIDS 2>/dev/null || true; fi",Note: This still requires addressing the platform and port discovery issues.
🤖 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 `@package.json` at line 49, The NPM script "standalone:kill" currently uses a forced SIGKILL (kill -9) which prevents cleanup; change it to first send SIGTERM (kill -15) to PIDS=$(lsof -ti :3456), wait a short interval (e.g., a sleep loop or timeout) to allow graceful shutdown, then if processes still exist escalate to SIGKILL; update the script that defines PIDS and the kill invocation in package.json accordingly so it attempts graceful termination before force-killing.src/renderer/utils/toolLinkingEngine.ts (1)
84-86: 💤 Low valueConsider clarifying the comment rationale.
The comment mentions advisor input is empty and "the fallback estimates ~0", but if token estimation ran on
"advisor" + "{}", it would estimate ~10 tokens, not ~0. The actual intent appears to be that advisor call tokens should not be estimated/charged at all (separate from the fact that input happens to be empty).Consider rephrasing to: "Advisor call tokens are not estimated since advisor invocations are internal; only result tokens are counted."
🤖 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/renderer/utils/toolLinkingEngine.ts` around lines 84 - 86, The inline comment for callTokens is misleading about why advisor tokens aren't estimated; update the comment near the callTokens assignment (referencing callTokens, toolName, estimateTokens and the 'advisor' branch) to state that advisor call tokens are intentionally not estimated/charged because advisor invocations are internal and only result tokens are counted (e.g., "Advisor call tokens are not estimated since advisor invocations are internal; only result tokens are counted.").
🤖 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 `@package.json`:
- Line 49: The standalone:kill npm script is Unix-only and hardcodes port 3456;
update it to be cross-platform and to detect the actual running server port
(respecting PORT env and the server's fallback range 3456–3466 used in
HttpServer.ts). Change the script to (a) prefer reading a PID or port file
created by the server process if available (look for any PID/port-writing logic
in the standalone server startup), (b) otherwise read the PORT env var and try
that port first then iterate the 3456–3466 range to find a listening process,
and (c) use platform-appropriate commands: lsof (Unix) or
netstat/PowerShell/Get-Process (Windows) to map port→PID and then
kill/Stop-Process the PID. Ensure the npm script (standalone:kill) exposes a
PORT override via environment variable and falls back to the range so Windows
users and servers started on non-3456 ports are handled.
In `@src/main/types/jsonl.ts`:
- Around line 76-87: The AdvisorToolResultContent type currently requires a
content property but downstream files (displayItemBuilder.ts and
SemanticStepExtractor.ts) use optional chaining on block.content?.text, so
update the AdvisorToolResultContent interface (symbol: AdvisorToolResultContent)
to make content optional (content?) so the type matches runtime usage; ensure
the nested shape remains { type: 'advisor_result'; text: string } unless you
also want to allow text to be absent — if downstream expects text possibly
empty, consider making text optional as well.
---
Nitpick comments:
In `@package.json`:
- Line 49: The NPM script "standalone:kill" currently uses a forced SIGKILL
(kill -9) which prevents cleanup; change it to first send SIGTERM (kill -15) to
PIDS=$(lsof -ti :3456), wait a short interval (e.g., a sleep loop or timeout) to
allow graceful shutdown, then if processes still exist escalate to SIGKILL;
update the script that defines PIDS and the kill invocation in package.json
accordingly so it attempts graceful termination before force-killing.
In `@src/main/services/analysis/SemanticStepExtractor.ts`:
- Around line 93-109: The advisor branch in SemanticStepExtractor (the if
checking block.type === 'server_tool_use' && block.name === 'advisor') creates a
tool_call step but omits the tokens field, causing structural inconsistency with
the other tool_use handling; update that step object to include a tokens
property (e.g., tokens: { input: 0, output: 0 } or tokens: undefined explicitly)
so all tool_call steps share the same shape, or alternatively add a brief inline
comment in the advisor branch explaining why tokens are intentionally omitted
(e.g., "advisor calls bypass normal token estimation")—modify the step creation
inside the advisor conditional to implement this change.
In `@src/renderer/utils/toolLinkingEngine.ts`:
- Around line 84-86: The inline comment for callTokens is misleading about why
advisor tokens aren't estimated; update the comment near the callTokens
assignment (referencing callTokens, toolName, estimateTokens and the 'advisor'
branch) to state that advisor call tokens are intentionally not
estimated/charged because advisor invocations are internal and only result
tokens are counted (e.g., "Advisor call tokens are not estimated since advisor
invocations are internal; only result tokens are counted.").
In `@test/mocks/advisorBlocks.fixture.ts`:
- Around line 1-68: The comment claiming "Cast needed until M2 adds advisorModel
to ParsedMessage" is outdated and the double-cast "as unknown as ParsedMessage"
is likely unnecessary; update or remove that comment and fix typing for
advisorCallMessage and advisorResultMessage by either (a) typing the objects
directly as ParsedMessage (remove the double-cast) if ParsedMessage already has
advisorModel?: string, or (b) narrow the content block types so the messages
satisfy ParsedMessage without casting (or replace the double-cast with a single
explicit cast only if truly unavoidable). Refer to the exported symbols
advisorCallMessage, advisorResultMessage, and the ParsedMessage/advisorModel
field when making the change.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0651aa59-14af-4be1-9517-1b4cb604fef3
📒 Files selected for processing (20)
package.jsonsrc/main/services/analysis/SemanticStepExtractor.tssrc/main/types/jsonl.tssrc/main/types/messages.tssrc/main/utils/jsonl.tssrc/renderer/components/chat/SessionContextPanel/components/RankedInjectionList.tsxsrc/renderer/components/chat/SessionContextPanel/items/ToolBreakdownItem.tsxsrc/renderer/components/chat/items/LinkedToolItem.tsxsrc/renderer/components/chat/items/linkedTool/renderHelpers.tsxsrc/renderer/utils/displayItemBuilder.tssrc/renderer/utils/stringUtils.tssrc/renderer/utils/toolLinkingEngine.tssrc/renderer/utils/toolRendering/toolSummaryHelpers.tstest/main/services/analysis/SemanticStepExtractor.test.tstest/mocks/advisorBlocks.fixture.tstest/renderer/utils/displayItemBuilder.test.tstest/renderer/utils/renderHelpers.test.tstest/renderer/utils/stringUtils.test.tstest/renderer/utils/toolLinkingEngine.test.tstest/renderer/utils/toolRendering/toolTokens.test.ts
| "standalone:build": "electron-vite build && vite build --config vite.standalone.config.ts", | ||
| "standalone:start": "node dist-standalone/index.cjs" | ||
| "standalone:start": "node dist-standalone/index.cjs", | ||
| "standalone:kill": "PIDS=$(lsof -ti :3456); [ -n \"$PIDS\" ] && kill -9 $PIDS || true", |
There was a problem hiding this comment.
Platform incompatibility and port discovery issue.
The standalone:kill script has two major issues:
- Unix-only: Uses
lsof, which is not available on Windows. Windows users cannot use this script. - Hardcoded port: Targets port
3456, but the standalone server can bind to ports3456through3466if the preferred port is busy (perHttpServer.ts:124-133). Additionally, if a user setsPORT=3457in the environment, the server will start on3457but this script will target3456.
Consider:
- Detecting the actual port from a PID file or server output
- Providing a Windows-compatible alternative using
netstator PowerShell - Making the port configurable via environment variable
🔍 Script to verify port binding behavior
#!/bin/bash
# Description: Verify that HttpServer can bind to ports beyond 3456
rg -n -A5 -B5 'for.*attempt.*tryPort.*preferredPort' src/main/services/infrastructure/HttpServer.ts🤖 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 `@package.json` at line 49, The standalone:kill npm script is Unix-only and
hardcodes port 3456; update it to be cross-platform and to detect the actual
running server port (respecting PORT env and the server's fallback range
3456–3466 used in HttpServer.ts). Change the script to (a) prefer reading a PID
or port file created by the server process if available (look for any
PID/port-writing logic in the standalone server startup), (b) otherwise read the
PORT env var and try that port first then iterate the 3456–3466 range to find a
listening process, and (c) use platform-appropriate commands: lsof (Unix) or
netstat/PowerShell/Get-Process (Windows) to map port→PID and then
kill/Stop-Process the PID. Ensure the npm script (standalone:kill) exposes a
PORT override via environment variable and falls back to the range so Windows
users and servers started on non-3456 ports are handled.
| export interface ServerToolUseContent extends BaseContent { | ||
| type: 'server_tool_use'; | ||
| id: string; | ||
| name: string; | ||
| input?: Record<string, unknown>; | ||
| } | ||
|
|
||
| export interface AdvisorToolResultContent extends BaseContent { | ||
| type: 'advisor_tool_result'; | ||
| tool_use_id: string; | ||
| content: { type: 'advisor_result'; text: string }; | ||
| } |
There was a problem hiding this comment.
Consider making AdvisorToolResultContent.content optional.
Downstream code in displayItemBuilder.ts and SemanticStepExtractor.ts uses optional chaining (block.content?.text ?? ''), suggesting content may be undefined at runtime. The type currently requires it, creating a gap between the type contract and defensive runtime guards.
🛡️ Suggested type refinement
export interface AdvisorToolResultContent extends BaseContent {
type: 'advisor_tool_result';
tool_use_id: string;
- content: { type: 'advisor_result'; text: string };
+ content?: { type: 'advisor_result'; text: string };
}🤖 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/main/types/jsonl.ts` around lines 76 - 87, The AdvisorToolResultContent
type currently requires a content property but downstream files
(displayItemBuilder.ts and SemanticStepExtractor.ts) use optional chaining on
block.content?.text, so update the AdvisorToolResultContent interface (symbol:
AdvisorToolResultContent) to make content optional (content?) so the type
matches runtime usage; ensure the nested shape remains { type: 'advisor_result';
text: string } unless you also want to allow text to be absent — if downstream
expects text possibly empty, consider making text optional as well.
1 participant
Summary
Renders
/advisortool calls as normal tool rows in the timeline, adds token accounting for advisor output, and an eval-report display.What changed
/advisorcalls now render as standard tool rows instead of being dropped ([BUG] Missing/advisorsupport #175).stringUtils.tsfor tool-name formatting, used by linking + summaries.jsonl.ts/messages.tsextended foradvisor_tool_result/server_tool_use.block.content.text; harden a test literal.Tests
SemanticStepExtractor.test.ts,displayItemBuilder.test.ts,renderHelpers.test.ts,stringUtils.test.ts,toolLinkingEngine.test.ts,toolTokens.test.tstest/mocks/advisorBlocks.fixture.tsScope
20 files changed (feature code + tests). Single squashed commit.
Summary by CodeRabbit
New Features
standalone:kill,standalone:restart) for managing standalone mode processes.UI Improvements
Improvements