fix(agent): decouple context_window from max_tokens#556
fix(agent): decouple context_window from max_tokens#556repfigit wants to merge 2 commits intosipeed:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where the agent's context window was incorrectly set to the max tokens value (typically 8192), causing premature context trimming. The fix decouples context_window from max_tokens by adding a dedicated configurable field with a sensible 128K default, aligning with modern LLM capabilities.
Changes:
- Added
ContextWindowfield toAgentDefaultsconfig struct with environment variable support - Fixed
NewAgentInstanceto use dedicated context window value (defaulting to 128K) instead of max tokens - Added
.gitattributesto enforce LF line endings across the repository
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/config/config.go | Adds ContextWindow field to AgentDefaults struct with JSON and environment variable tags |
| pkg/agent/instance.go | Introduces context window initialization logic with 128K default and uses it instead of max tokens |
| .gitattributes | Enforces LF line endings for all text files to ensure consistency across platforms |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/config/config.go
Outdated
| ImageModel string `json:"image_model,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_IMAGE_MODEL"` | ||
| ImageModelFallbacks []string `json:"image_model_fallbacks,omitempty"` | ||
| MaxTokens int `json:"max_tokens" env:"PICOCLAW_AGENTS_DEFAULTS_MAX_TOKENS"` | ||
| ContextWindow int `json:"context_window,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_CONTEXT_WINDOW"` |
There was a problem hiding this comment.
The ContextWindow field uses omitempty in its JSON tag, which is inconsistent with similar integer fields like MaxTokens (line 177) and MaxToolIterations (line 180) that don't use omitempty. For consistency and to match the existing pattern for integer configuration fields with defaults in AgentDefaults, consider removing omitempty from the JSON tag.
| ContextWindow int `json:"context_window,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_CONTEXT_WINDOW"` | |
| ContextWindow int `json:"context_window" env:"PICOCLAW_AGENTS_DEFAULTS_CONTEXT_WINDOW"` |
| contextWindow := defaults.ContextWindow | ||
| if contextWindow == 0 { | ||
| contextWindow = 128000 | ||
| } |
There was a problem hiding this comment.
The new ContextWindow field lacks test coverage. Similar fields like MaxTokens and Temperature have dedicated tests in instance_test.go (e.g., TestNewAgentInstance_UsesDefaultsTemperatureAndMaxTokens). Consider adding tests to verify:
- ContextWindow uses the configured value when provided
- ContextWindow defaults to 128000 when not configured (zero value)
This is important because ContextWindow affects critical behavior like context trimming in loop.go (lines 727 and 892).
There was a problem hiding this comment.
Since picoclaw is intended to work on low-end machine, by default we should set a smaller value (better to keep 8192, the current value). Users with devices that have larger memory could set this value explicitly in the config file.
Please also update the README.md for this feature.
There was a problem hiding this comment.
Let the users to set this value may not be convient, I think the better way is when running the onboard guidance, ask the user their preference for deploying corresponding devices.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
03a6b3a to
8afe597
Compare
Add configurable context_window field to AgentDefaults with a 128K default. Previously ContextWindow was incorrectly set to MaxTokens (typically 8192), causing overly aggressive context trimming. Co-Authored-By: Claude Opus 4.6 <[email protected]>
8afe597 to
06ee821
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MaxTokens int `json:"max_tokens" env:"PICOCLAW_AGENTS_DEFAULTS_MAX_TOKENS"` | ||
| ContextWindow int `json:"context_window" env:"PICOCLAW_AGENTS_DEFAULTS_CONTEXT_WINDOW"` | ||
| Temperature *float64 `json:"temperature,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_TEMPERATURE"` | ||
| MaxToolIterations int `json:"max_tool_iterations" env:"PICOCLAW_AGENTS_DEFAULTS_MAX_TOOL_ITERATIONS"` |
There was a problem hiding this comment.
ContextWindow is added without omitempty, but DefaultConfig() (used by onboarding + SaveConfig) does not set this field yet. As a result, newly generated config files will likely include "context_window": 0, which conflicts with the stated 128K default and is confusing for users. Consider either setting ContextWindow to 128000 in DefaultConfig() (preferred if you want the default to be visible) or adding omitempty to this JSON tag so the field is omitted when unset.
There was a problem hiding this comment.
@repfigit I didn't see the DefaultConfig changes.
There was a problem hiding this comment.
Why do we need to change this file in a fix PR?
|
|
||
| contextWindow := defaults.ContextWindow | ||
| if contextWindow == 0 { | ||
| contextWindow = 128000 |
There was a problem hiding this comment.
Please at the very least state your rationale here. I share the same concern that @zenixls2 has raised.
nikolasdehor
left a comment
There was a problem hiding this comment.
Good bug fix. Setting ContextWindow = maxTokens (typically 8192) is clearly wrong - the context window is the total input capacity, not the output token limit. This would cause premature summarization on models with large context windows.
The fix is clean: add a configurable ContextWindow field with a 128K default. The tests cover both the configured and default cases.
One suggestion: 128K is a reasonable default for modern models (GPT-4+, Claude, Gemini), but some smaller/local models have 4K-32K context windows. Users running Ollama with smaller models might not notice they need to configure this. Consider logging a debug message when using the 128K default, or documenting this in the config example.
The .gitattributes addition for LF line endings is fine but unrelated - consider separating it into its own PR for clean history. Not a blocker.
LGTM.
|
repfigit seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
…and safe compression Separate context_window from max_tokens — they serve different purposes (input capacity vs output generation limit). The previous conflation caused premature summarization or missed compression triggers. Changes: - Add context_window field to AgentDefaults config (default: 4x max_tokens) - Extract boundary-safe truncation helpers (isSafeBoundary, findSafeBoundary) into context_budget.go — pure functions with no AgentLoop dependency - forceCompression: align split to safe boundary so tool-call sequences (assistant+ToolCalls → tool results) are never torn apart - summarizeSession: use findSafeBoundary instead of hardcoded keep-last-4 - estimateTokens: count ToolCalls arguments and ToolCallID metadata, not just Content — fixes systematic undercounting in tool-heavy sessions - Add proactive context budget check before LLM call in runAgentLoop, preventing 400 context-length errors instead of reacting to them - Add estimateToolDefsTokens for tool definition token cost Closes sipeed#556, closes sipeed#665 Ref sipeed#1439
Summary
context_windowfield toAgentDefaultswith a 128K defaultContextWindowwas incorrectly set toMaxTokens(typically 8192), causing overly aggressive context trimming.gitattributesto enforce LF line endingsTest plan
go build ./...go test ./pkg/config/... ./pkg/agent/...context_window— should default to 128Kcontext_window: 64000— should use 64K🤖 Generated with Claude Code