Honour llm.max_tokens on the AI analysis paths - #413
Closed
dpage wants to merge 1 commit into
Closed
Conversation
The estate summary, the Server Info database analysis, and the alerter's tier 3 classification each hardcoded a 512-token output budget, so the configured llm.max_tokens had no bearing on any of them. A reasoning model spends part of its budget on an internal thinking block before it emits any answer text, and with a local model served over an OpenAI-compatible endpoint that small cap was consumed entirely by reasoning; the response then arrived with no text block at all, and because the callers treated the resulting empty string as a successful generation the failure surfaced as a blank panel with no error. Both server paths now take their budget from the shared llmproxy config via a new AnalysisMaxTokens helper, which reports the operator-configured value and falls back to 4096 when the setting is absent or non-positive. The alerter previously had no such setting at all, so it gains an llm.max_tokens option with the same default and fallback rule. A response that yields no usable text is now reported rather than rendered: the overview path returns an error naming the configured budget, the Server Info path logs and skips the cache so the next request retries, and the alerter's Classify returns an error, which its caller already handles by failing safe to an alert. Closes #399
WalkthroughThe change adds configurable LLM output-token budgets with a 4096-token fallback. Alerter reasoning, estate summaries, and server-info analysis now use configured budgets. Empty or whitespace-only responses return errors and are not cached. ChangesLLM token budget configuration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ServerInfoHandler
participant AnalysisConfig
participant LLMProvider
participant AnalysisCache
ServerInfoHandler->>AnalysisConfig: resolve AnalysisMaxTokens()
ServerInfoHandler->>LLMProvider: send request with configured MaxTokens
LLMProvider-->>ServerInfoHandler: return analysis response
alt usable text exists
ServerInfoHandler->>AnalysisCache: cache analysis
else no usable text
ServerInfoHandler-->>ServerInfoHandler: log failure and return unavailable
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 35 |
| Duplication | 0 |
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 64-76: Update the changelog entry to state that llm.max_tokens
falls back to 4096 when absent, zero, or negative. Clarify the path-specific
empty-response behavior: Server Info database analysis reports an error, while
the alerter’s tier 3 classification reports a failed classification.
In `@docs/getting-started/configuration/server.md`:
- Around line 439-445: Update the max_tokens documentation to state that
non-positive values fall back to 4096 only for the AI estate summary and AI
database analysis requests. Do not claim this fallback applies to Ask Ellie
chat, which omits MaxTokens when llm.max_tokens is non-positive.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24078633-806c-4125-8986-789b892d0690
📒 Files selected for processing (16)
alerter/src/internal/config/config.goalerter/src/internal/config/config_test.goalerter/src/internal/llm/llm.goalerter/src/internal/llm/reasoning.goalerter/src/internal/llm/reasoning_test.godocs/changelog.mddocs/getting-started/configuration/alerter.mddocs/getting-started/configuration/server.mdexamples/ai-dba-alerter.yamlexamples/ai-dba-server.yamlserver/src/internal/api/server_info_handlers.goserver/src/internal/api/server_info_handlers_test.goserver/src/internal/llmproxy/proxy.goserver/src/internal/llmproxy/proxy_test.goserver/src/internal/overview/generator.goserver/src/internal/overview/generator_test.go
Comment on lines
+64
to
+76
| - Honour the configured `llm.max_tokens` on the AI estate summary, | ||
| the Server Info database analysis, and the alerter's tier 3 | ||
| classification, each of which previously hardcoded a 512-token | ||
| output budget. A reasoning model spends part of its budget on an | ||
| internal thinking block, so the small cap was consumed before the | ||
| model emitted any answer and the estate summary rendered as an | ||
| empty panel. Each path now reads the operator-configured budget | ||
| and falls back to `4096` when the setting is absent, and a | ||
| response that carries no text content is reported as an error | ||
| rather than being cached and rendered as an empty result. The | ||
| alerter gains a matching `llm.max_tokens` setting, which it | ||
| previously lacked. (#399) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
State the full fallback and empty-response behavior.
llm.max_tokens also falls back to 4096 for zero and negative values, not only when the setting is absent. Empty-response handling is path-specific: server analysis reports an error, while tier 3 classification reports a failed classification. Update this entry to match the configuration guides.
🤖 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 `@docs/changelog.md` around lines 64 - 76, Update the changelog entry to state
that llm.max_tokens falls back to 4096 when absent, zero, or negative. Clarify
the path-specific empty-response behavior: Server Info database analysis reports
an error, while the alerter’s tier 3 classification reports a failed
classification.
Comment on lines
+439
to
+445
| The `max_tokens` option sets the output-token budget | ||
| for every LLM request the server makes. The budget | ||
| governs Ask Ellie chat, the AI estate summary on the | ||
| overview page, and the AI database analysis in the | ||
| Server Info dialog. The default value is `4096`; | ||
| values less than or equal to zero fall back to that | ||
| default. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'AnalysisMaxTokens|BuildClientOptions|MaxTokens' \
server/src --glob '*.go'Repository: pgEdge/ai-dba-workbench
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- chat and streaming call sites ---'
rg -n -C 5 \
'providerOptions\(|\.Chat\(|\.ChatStream\(|ChatStream|ChatRequest' \
server/src/internal/llmproxy server/src/internal/api server/src/internal/overview \
--glob '*.go' | head -n 260
printf '%s\n' '--- max-token assignment paths ---'
rg -n -C 4 \
'MaxTokens|AnalysisMaxTokens' \
server/src/internal/llmproxy/proxy.go \
server/src/internal/api \
server/src/internal/overview \
--glob '*.go' | head -n 320Repository: pgEdge/ai-dba-workbench
Length of output: 40426
Limit the fallback statement to analysis requests. Ask Ellie chat uses providerOptions, which omits MaxTokens when llm.max_tokens is non-positive and does not apply the 4096 fallback.
🤖 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 `@docs/getting-started/configuration/server.md` around lines 439 - 445, Update
the max_tokens documentation to state that non-positive values fall back to 4096
only for the AI estate summary and AI database analysis requests. Do not claim
this fallback applies to Ask Ellie chat, which omits MaxTokens when
llm.max_tokens is non-positive.
Member
Author
|
Closing as a duplicate. PR #411 fixes the same three hardcoded output-token caps and goes further, also updating |
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
The estate summary, the Server Info database analysis, and the alerter's
tier 3 classification each hardcoded a 512-token output budget, so the
configured
llm.max_tokenshad no bearing on any of them. A reasoningmodel spends part of its budget on an internal thinking block before it
emits any answer text, so with a local model served over an
OpenAI-compatible endpoint the cap was consumed entirely by reasoning;
the response then carried no text block, and because the callers treated
the resulting empty string as a successful generation the failure
surfaced as a blank panel with no error at all.
The changes are as follows:
Add
(*llmproxy.Config).AnalysisMaxTokens, which reports theoperator-configured
llm.max_tokensand falls back to the newDefaultAnalysisMaxTokens(4096) when the setting is absent ornon-positive, and use it for both the estate summary and the Server
Info analysis, on the chat request and on the client options alike.
Give the alerter an
llm.max_tokenssetting, which it previouslylacked entirely, with the same 4096 default and fallback rule, and
plumb it through
newLibReasoninginto the classification request.Report a response that yields no usable text rather than rendering it:
the overview path returns an error naming the configured budget, the
Server Info path logs and skips the cache so the next request retries
instead of serving a blank result, and the alerter's
Classifyreturns an error, which its caller already handles by failing safe to
an alert.
Document the setting's effect on the analysis paths in the server and
alerter configuration guides, update both commented example
configurations, and add a changelog entry.
Test plan
New unit tests cover
AnalysisMaxTokensandReasoningMaxTokensacross nil, unset, zero, negative, and configured values.
New table-driven tests drive the overview and Server Info paths
against an OpenAI-compatible stub that decodes the request body, and
assert that the budget on the wire is the configured value, or the
4096 default when the setting is unset.
New tests assert that a response carrying no text (empty content, and
whitespace only) produces an error naming the configured budget, and
that the Server Info path leaves the cache untouched in that case.
Every changed function reports 100% line coverage:
AnalysisMaxTokens,generateSummaryFromPrompt, bothcreateLLMClientmethods,getAIAnalysis,ReasoningMaxTokens,Classify, andnewLibReasoning.make lintpasses with zero issues for bothserverandalerter,and
gofmtreports no changes.make test-allpasses bar one pre-existing failure inserver/internal/tools(TestStoreMemoryGeneratesEmbeddingIntegrationand
TestRecallMemoriesGeneratesQueryEmbeddingIntegration), which isa stale
vector(3)fixture against the 4000-dimension padding andreproduces identically on a clean checkout of
main; it is unrelatedto this change and skips in CI, where Postgres lacks pgvector.
Closes #399
Summary by CodeRabbit
New Features
Bug Fixes
Documentation