Feat/context budget controls#10
Merged
Merged
Conversation
TestMemorySink_Put previously only checked len(stored) == 1, never verifying the callback received the correct value. Now asserts the stored value matches and the returned URI ends with the stored key. Also apply filepath.Base to agentName in tempFileSink.Put for consistency with the existing key sanitization, preventing a malicious agentName from escaping ~/.abbyfile/.
Extend CompiledDefaults with MaxOutputLines, MaxOutputBytes, OnOverflow, and EagerInstructions so `config get`/`config get context_budget.*` displays effective context_budget values and their source (compiled vs override), matching the existing model/tool_timeout rows. compiledDefaults() now populates these from the agent's tools.ContextBudget. Add an integration test that builds a real agent binary with a custom tool emitting 1000 lines and a context_budget capped at 20 lines, then runs it via `run-tool` and asserts the shaped output is elided and far shorter than the raw output — locking in end-to-end shaping behavior wired in earlier tasks.
Add docs/guides/context-budget.md covering output shaping (head-tail, spill, passthrough), the context_budget frontmatter block, shipped defaults, config-set/get overrides, the --subagent flag, and eager_instructions. Update concepts.md and README.md to note that --subagent gives Abbyfile agents real context isolation.
…mited Final whole-branch review found two correctness bugs at the codegen/runtime boundary: buildBudgetData was forcing an empty per-tool on_overflow to head-tail, defeating effectiveFor's inherit-from-base semantics; and MaxOutputLines/MaxOutputBytes were plain ints, so frontmatter couldn't distinguish "0 (unlimited)" from "omitted (use shipped default)".
PR Summary by QodoAdd context budget controls: tool output shaping + sub-agent isolation
AI Description
Diagram
High-Level Assessment
Files changed (32)
|
Code Review by Qodo
1. Memory spill always fails
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+121
to
+123
| key := fmt.Sprintf("spill/%s", toolName) | ||
| if uri, err := sink.Put(key, raw); err == nil { | ||
| res.SpillURI = uri |
There was a problem hiding this comment.
1. Memory spill always fails 🐞 Bug ≡ Correctness
When OverflowSpill is selected, ContextBudget.Shape generates a spill key containing '/', but the memory-backed sink is wired to memory.Manager.Set which rejects keys with path separators. This makes spill-to-memory fail every time (degrading to head-tail truncation) whenever memory is enabled.
Agent Prompt
### Issue description
`tools.ContextBudget.Shape()` uses spill keys like `spill/<toolName>`. When memory is enabled, the spill sink is `tools.NewMemorySink(..., mgr.Set)`, and the underlying `memory.FileStore` rejects keys containing `/` or `\`. This causes all memory-backed spills to fail and the shaper to degrade to truncation.
### Issue Context
- Memory storage (`memory.FileStore`) enforces flat keys (no path separators).
- The spill feature is expected to store full output losslessly when `on_overflow: spill`.
### Fix Focus Areas
- pkg/tools/spill.go[21-27]
- pkg/tools/shaper.go[118-133]
- pkg/memory/store.go[69-80]
- pkg/tools/spill_test.go[10-41]
### Implementation notes
- Prefer fixing this in `memorySink.Put()` by sanitizing `key` into a memory-safe key (e.g., replace `/` and `\` with `_`), then append the hash.
- Optionally also change `ContextBudget.Shape()` to generate a memory-safe base key (e.g. `spill_<toolName>`) so *all* sinks receive safe keys.
- Add/adjust a test asserting the stored memory key contains no path separators.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+151
to
+173
| if cfg.ContextBudget != nil { | ||
| cb := cfg.ContextBudget | ||
| if cb.MaxOutputLines != nil { | ||
| a.budget.MaxOutputLines = *cb.MaxOutputLines | ||
| } | ||
| if cb.MaxOutputBytes != nil { | ||
| a.budget.MaxOutputBytes = *cb.MaxOutputBytes | ||
| } | ||
| if cb.OnOverflow != nil { | ||
| a.budget.OnOverflow = tools.OverflowStrategy(*cb.OnOverflow) | ||
| } | ||
| if cb.HeadLines != nil { | ||
| a.budget.HeadLines = *cb.HeadLines | ||
| } | ||
| if cb.TailLines != nil { | ||
| a.budget.TailLines = *cb.TailLines | ||
| } | ||
| if cb.SummaryLines != nil { | ||
| a.budget.SummaryLines = *cb.SummaryLines | ||
| } | ||
| if cb.EagerInstructions != nil { | ||
| a.budget.EagerInstructions = *cb.EagerInstructions | ||
| } |
There was a problem hiding this comment.
2. Negative budget disables shaping 🐞 Bug ☼ Reliability
Context budget overrides from config.yaml are applied without validating non-negative values, and the shaper only enforces caps when MaxOutputLines/MaxOutputBytes are > 0. A negative override therefore disables shaping entirely (or yields misleading shaping behavior) without any warning.
Agent Prompt
### Issue description
`pkg/config/loader.WriteFieldTo()` accepts negative integers for context_budget numeric fields, and `Agent.applyConfigOverrides()` blindly copies them into the runtime budget. Since `ContextBudget.Shape()` only triggers caps when values are `> 0`, negative values effectively turn off shaping without any log warning.
### Issue Context
Frontmatter context_budget values are validated, but config.yaml overrides (manual edits or `config set`) currently are not.
### Fix Focus Areas
- pkg/config/loader.go[96-146]
- pkg/agent/agent.go[151-173]
- pkg/tools/shaper.go[95-102]
### Implementation notes
- In `WriteFieldTo`, reject negative values for `max_output_lines`, `max_output_bytes`, `head_lines`, `tail_lines`, and `summary_lines` with a clear error.
- In `applyConfigOverrides`, defensively validate and ignore invalid negative values (log a warning and keep the previous/default value), so manual config edits can’t silently disable shaping.
- Consider validating `on_overflow` here too (ignore invalid strategy and warn), to match frontmatter validation behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+130
to
+163
| case "context_budget.max_output_lines": | ||
| val := defaults.MaxOutputLines | ||
| source := "compiled" | ||
| if cfg.ContextBudget != nil && cfg.ContextBudget.MaxOutputLines != nil { | ||
| val = *cfg.ContextBudget.MaxOutputLines | ||
| source = "override" | ||
| } | ||
| fmt.Fprintf(cmd.OutOrStdout(), "%d (%s)\n", val, source) | ||
| case "context_budget.max_output_bytes": | ||
| val := defaults.MaxOutputBytes | ||
| source := "compiled" | ||
| if cfg.ContextBudget != nil && cfg.ContextBudget.MaxOutputBytes != nil { | ||
| val = *cfg.ContextBudget.MaxOutputBytes | ||
| source = "override" | ||
| } | ||
| fmt.Fprintf(cmd.OutOrStdout(), "%d (%s)\n", val, source) | ||
| case "context_budget.on_overflow": | ||
| val := defaults.OnOverflow | ||
| source := "compiled" | ||
| if cfg.ContextBudget != nil && cfg.ContextBudget.OnOverflow != nil { | ||
| val = *cfg.ContextBudget.OnOverflow | ||
| source = "override" | ||
| } | ||
| fmt.Fprintf(cmd.OutOrStdout(), "%s (%s)\n", val, source) | ||
| case "context_budget.eager_instructions": | ||
| val := defaults.EagerInstructions | ||
| source := "compiled" | ||
| if cfg.ContextBudget != nil && cfg.ContextBudget.EagerInstructions != nil { | ||
| val = *cfg.ContextBudget.EagerInstructions | ||
| source = "override" | ||
| } | ||
| fmt.Fprintf(cmd.OutOrStdout(), "%t (%s)\n", val, source) | ||
| default: | ||
| return fmt.Errorf("unknown field: %s (supported: model, tool_timeout)", field) | ||
| return fmt.Errorf("unknown field: %s (supported: model, tool_timeout, context_budget.max_output_lines, context_budget.max_output_bytes, context_budget.on_overflow, context_budget.eager_instructions)", field) |
There was a problem hiding this comment.
3. Config cli hides budget fields 🐞 Bug ◔ Observability
The CLI config get command does not support or print several context_budget override fields (head_lines, tail_lines, summary_lines) even though they exist in the config schema and can be written. This makes it difficult to verify/debug runtime override behavior for those fields.
Agent Prompt
### Issue description
`internal/cli/config.go` only supports reading/printing a subset of `context_budget.*` fields, but `pkg/config` and `pkg/config/loader` support additional fields (`head_lines`, `tail_lines`, `summary_lines`). Users can set these fields but cannot inspect effective values via `config get`.
### Issue Context
This is a CLI observability/debuggability gap: it doesn’t change runtime behavior, but it makes misconfiguration hard to detect.
### Fix Focus Areas
- internal/cli/config.go[108-231]
- internal/cli/config_test.go[39-142]
- internal/cli/config.go[11-20]
- pkg/agent/agent.go[93-103]
### Implementation notes
- Extend `cli.CompiledDefaults` to include `HeadLines`, `TailLines`, and `SummaryLines` (and optionally document them).
- Populate them in `Agent.compiledDefaults()`.
- Add `printField` + `printAllFields` cases for:
- `context_budget.head_lines`
- `context_budget.tail_lines`
- `context_budget.summary_lines`
- Update the supported-field error string and add/extend tests to cover these fields.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
2 participants
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.
No description provided.