feat: remove chat.LLMClient wrapper and use pgedge-go-llm-lib directly (PR 5 of 5)#176
Draft
dpage wants to merge 2 commits into
Draft
feat: remove chat.LLMClient wrapper and use pgedge-go-llm-lib directly (PR 5 of 5)#176dpage wants to merge 2 commits into
dpage wants to merge 2 commits into
Conversation
Delete the chat.LLMClient interface, libClient wrapper, and the translation helpers (toLibMessages, toLibTools, fromLibContent) that were introduced as a temporary shim during the LLM-library migration. The chat package now consumes llm.Client, llm.Message, llm.ContentBlock, llm.ChatRequest, llm.ChatResponse, llm.Tool, and llm.TokenUsage directly throughout. The client.go Client struct holds the LLM behind a small package-local chatLLM interface (Chat + ListModels) which the real library client satisfies structurally — this keeps test mocks straightforward without re-introducing a wrapper layer around the rest of the library surface (Embed, Rerank, ChatStream, etc.). The Anthropic prompt-cache markers (system + tools) are now applied directly in processQuery via anthropic.WithSystemCaching and anthropic.WithToolCaching. The chat system prompt constants move to a new prompts.go file with a buildSystemPrompt(readOnly) helper. Conversations saved by earlier versions are migrated transparently on load: messages with a plain-string content field are wrapped as a single text block, and tool-result messages saved with role "user" are promoted to role "tool" to match the library shape. The on-disk format written going forward is the typed content-block form. A Conversation.UnmarshalJSON method and two new unit tests cover the legacy-format paths. llm_loglevel.go and llm_tracing.go remain — the CLI's debug mode still wires the tracingRoundTripper into llm.Options.HTTPClient. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe PR removes the internal ChangesLLM type migration and client refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Actions performedReview triggered.
|
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 8 medium |
🟢 Metrics 70 complexity · -5 duplication
Metric Results Complexity 70 Duplication -5
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/chat/client_test.go (1)
520-539:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten token assertions so they validate tool payload accounting.
These checks can pass from message overhead alone (
+10), so they don’t provetool_result/tool_usecontent is being counted.Suggested test fix
- if tokens < 10 { + if tokens <= 10 { t.Errorf("Expected at least 10 tokens, got %d", tokens) } @@ - if estimateTotalTokens(withToolUse) <= 0 { + if estimateTotalTokens(withToolUse) <= 10 { t.Errorf("Expected non-zero tokens for tool_use input") }🤖 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 `@internal/chat/client_test.go` around lines 520 - 539, The current test uses a fixed threshold that can be satisfied by message overhead alone; instead compute a baseline token count (using estimateTotalTokens on the original messages variable or a minimal message) and then assert that estimateTotalTokens(withToolUse) is strictly greater than that baseline to prove the tool input is counted; use the same approach for any tool_result cases and reference the estimateTotalTokens function and the withToolUse variable (and tokens baseline) so the test validates an increase (optionally at least the JSON input length) rather than a fixed minimum.internal/chat/commands.go (1)
1070-1106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefresh
currentDBWritablewhen loading a saved connection.This path can switch the active database, but it leaves
c.currentDBWritableat the previous session's value. The next request then uses stale safety state: Line 956 ininternal/chat/client.gobuilds the wrong system prompt, and Line 1019 can skip write confirmation on a writable database.Suggested fix
if conv.Connection != "" { if _, current, err := c.mcp.ListDatabases(ctx); err == nil { if current != conv.Connection { if err := c.mcp.SelectDatabase(ctx, conv.Connection); err != nil { c.ui.PrintError(fmt.Sprintf("Warning: Failed to restore database connection: %v", err)) } else { // Refresh capabilities after database change if err := c.refreshCapabilities(ctx); err != nil { c.ui.PrintError(fmt.Sprintf("Warning: Failed to refresh capabilities: %v", err)) } } } } + c.currentDBWritable = false + if databases, current, err := c.mcp.ListDatabases(ctx); err == nil { + for _, db := range databases { + if db.Name == current && db.AllowWrites { + c.currentDBWritable = true + break + } + } + } }🤖 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 `@internal/chat/commands.go` around lines 1070 - 1106, When restoring a saved connection in the conversation load path, the code calls c.mcp.SelectDatabase and c.refreshCapabilities but does not update c.currentDBWritable, leaving stale writable state; after a successful SelectDatabase (and preferably after refreshCapabilities) set c.currentDBWritable to the correct writable state by querying the MCP or using the refreshed capability result so that the client’s writable flag (c.currentDBWritable) reflects the newly selected database and prevents incorrect prompt construction in client.go (c.currentDBWritable used at lines building system prompt and write-confirm logic). Ensure the update occurs in the same branch where SelectDatabase succeeds and handle any errors consistently with existing ui.PrintError behavior.
🤖 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 `@docs/changelog.md`:
- Around line 82-89: Update the Unreleased notes so the two bullets consistently
state that the internal chat.LLMClient wrapper (internal/chat/llm.go and
internal/chat/llm_translate.go) was removed and the code now uses llm.Client
directly, while clarifying that the external llm.Client interface (not the
deleted chat wrapper) remains unchanged; replace the ambiguous “the LLMClient
interface is unchanged” sentence with wording that explicitly distinguishes the
removed chat.LLMClient wrapper from the unchanged llm.Client API and keep the
reference to llm.Options.HTTPClient tracing.
In `@internal/chat/client.go`:
- Around line 1007-1012: The code currently swallows json.Unmarshal errors and
proceeds to call the tool with an empty input; change the flow so when
json.Unmarshal(toolUse.Input, &input) returns an error you do NOT proceed to
invoke the tool. Specifically, in the block around json.Unmarshal and
c.ui.PrintToolExecution: check the error from json.Unmarshal, surface it to the
UI (e.g., via c.ui.PrintToolExecution or an error-specific UI method) and
return/abort the tool execution instead of calling the MCP tool; additionally,
for tools like query_database validate that the parsed input contains the
required "query" field before invoking the tool and abort with a clear UI error
if missing or if JSON was malformed.
---
Outside diff comments:
In `@internal/chat/client_test.go`:
- Around line 520-539: The current test uses a fixed threshold that can be
satisfied by message overhead alone; instead compute a baseline token count
(using estimateTotalTokens on the original messages variable or a minimal
message) and then assert that estimateTotalTokens(withToolUse) is strictly
greater than that baseline to prove the tool input is counted; use the same
approach for any tool_result cases and reference the estimateTotalTokens
function and the withToolUse variable (and tokens baseline) so the test
validates an increase (optionally at least the JSON input length) rather than a
fixed minimum.
In `@internal/chat/commands.go`:
- Around line 1070-1106: When restoring a saved connection in the conversation
load path, the code calls c.mcp.SelectDatabase and c.refreshCapabilities but
does not update c.currentDBWritable, leaving stale writable state; after a
successful SelectDatabase (and preferably after refreshCapabilities) set
c.currentDBWritable to the correct writable state by querying the MCP or using
the refreshed capability result so that the client’s writable flag
(c.currentDBWritable) reflects the newly selected database and prevents
incorrect prompt construction in client.go (c.currentDBWritable used at lines
building system prompt and write-confirm logic). Ensure the update occurs in the
same branch where SelectDatabase succeeds and handle any errors consistently
with existing ui.PrintError behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4015f4f3-74be-4d48-a366-86b5f077453d
📒 Files selected for processing (12)
docs/changelog.mdinternal/chat/client.gointernal/chat/client_integration_test.gointernal/chat/client_test.gointernal/chat/commands.gointernal/chat/conversations.gointernal/chat/conversations_test.gointernal/chat/llm.gointernal/chat/llm_test.gointernal/chat/llm_translate.gointernal/chat/llm_translate_test.gointernal/chat/prompts.go
💤 Files with no reviewable changes (4)
- internal/chat/llm_translate_test.go
- internal/chat/llm_test.go
- internal/chat/llm.go
- internal/chat/llm_translate.go
- commands.go: refresh c.currentDBWritable after loading a saved conversation that switches database. Previously the writable flag was left at the previous session's value, causing the next request to build the wrong system prompt and potentially skip write confirmation on a writable database. - client.go: fail the tool call when json.Unmarshal of toolUse.Input errors out, rather than silently invoking the tool with an empty input map. Returns the parse error to the model as a tool_result with is_error=true so the loop continues sensibly. - client_test.go: tighten TestEstimateTotalTokensWithToolContent to compare against a same-shape empty-content baseline, so the test proves tool_use/tool_result payloads are being counted rather than passing on structural overhead alone. - docs/changelog.md: clarify that the removed wrapper is the internal chat.LLMClient interface, distinct from the library's llm.Client API which is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Final clean-up PR for the LLM library migration. Removes the temporary
chat.LLMClientwrapper introduced in PR 1 (#172) and rewires the CLI chat client to usepgedge-go-llm-lib'sllm.Clientdirectly.internal/chat/llm.go(theLLMClientinterface,libClientwrapper,NewAnthropicClient/NewOpenAIClient/NewOllamaClientconstructors, and helper functions) — deleted.internal/chat/llm_translate.go(thetoLibMessages/toLibTools/fromLibContenttranslation helpers from PR 1) — deleted.internal/chat/llm_test.goandllm_translate_test.go— deleted.internal/chat/prompts.gohosts the system prompt constants and abuildSystemPrompt(readOnly bool)helper.internal/chat/client.goandcommands.gonow usellm.Message,llm.ContentBlock,llm.ChatRequest, etc. directly. Anthropic prompt caching (system + tools) is applied inline viaanthropic.WithSystemCaching/WithToolCaching.internal/chat/conversations.goreads/writes[]llm.Message. AConversation.UnmarshalJSONshim migrates pre-PR-5 saved conversations (stringcontentbecomes a[{type: "text", text: "..."}]block; legacy user-role tool_result messages are promoted to RoleTool).Net: -706 lines across the chat package.
This is PR 5 of 5. Based on PR 4 (#175); will need rebase onto main as PR 1-4 land.
Behaviour preserved
buildSystemPrompt(true)).llm_tracing.go's round-tripper.chat.NewClient,chat.LoadConfig,chat.ConfigOverrides,chat.ClientVersion) is unchanged.Test plan
make testpasses (all packages green, including the chat package's rewritten tests + two new migration tests).make lintreports 0 issues.go run ./cmd/pgedge-pg-mcp-cli -helpruns cleanly.🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
New Features