Improve terminal input status layout#152
Merged
Merged
Conversation
Sigilix review resumedThis PR is ready for review. The earlier draft-skip notice has been superseded by the normal review run. Current commit: |
Desperado
marked this pull request as ready for review
July 17, 2026 22:29
Sigilix OverviewEffort: 4/5 (large) Quality gates
Summary — latest pushIntroduces a bordered input panel and live status bar to the terminal REPL, surfacing session metrics (context window fill, token counts, turn duration, session timer) and backend/mode details alongside the current task. The change threads a new StatusInfo struct through the input loop, clears per-turn context estimates at turn boundaries, and captures CLI backend usage via a TurnStatsProvider interface. A specialist review flagged logic errors in context token tracking, unhandled edge cases in duration formatting, and security concerns around unbounded task strings that must be addressed before merge. Important files
Confidence: 2/5Unconditional zeroing of lastContextTokens before CLI backend execution creates a logic error that hides context metrics, and unescaped task strings pose a terminal injection risk.
Suggested labels:
|
![sigilix[bot]](https://avatars.githubusercontent.com/in/3558711?s=60&v=4){kind=small}
| // ClearHistory resets conversation history. | ||
| func (a *Agent) ClearHistory() { | ||
| a.History = []api.Message{} | ||
| a.LastContextTokens = 0 |
There was a problem hiding this comment.
Ollama backend does not update LastContextTokens after a turn
The diff establishes a convention: every backend that reports per-turn input tokens sets Agent.LastContextTokens so the TUI can estimate context-window fill. The Anthropic streaming path (line 662), the non-streaming API path (line 822), and the Cerebras path (cerebras_agent.go line 71) all set a.LastContextTokens = <per-turn input tokens>. The Ollama path in agent.go does not, so after an Ollama turn LastContextTokens stays stale (zero or the previous backend's value), and the status bar shows a misleading context-fill percentage. Add a.LastContextTokens = <ollama per-turn input tokens> in the Ollama response-handling branch.
Example:
input: Ollama backend active, a turn completes with 5000 input tokens
actual: `LastContextTokens` remains 0 (or the previous backend's value)
expected: `LastContextTokens` == 5000, status bar shows correct context fill
Suggested fix:
a.LastContextTokens = ollamaResp.Usage.InputTokensWhy this wasn't caught: No test covers LastContextTokens being set after an Ollama turn; the existing TestContextWindow only tests the window-size helper.
More Info
- Threat model: Users on the Ollama backend see a stale or zero context-fill percentage in the input status bar, misleading them about how full the context window is.
- Specific code citations: Convention sites:
agent.goline 662 (a.LastContextTokens = ev.Message.Usage.InputTokens), line 822 (a.LastContextTokens = apiResp.Usage.InputTokens),cerebras_agent.goline 71 (a.LastContextTokens = resp.Usage.PromptTokens). Violation site: the Ollama response-handling code inagent.go(thecallOllamaor equivalent function) — noLastContextTokensassignment. - Existing protections: None.
LastContextTokensis only set in the three paths listed above; the Ollama path is a fourth code path that handles its own response and never touches the field. - Proposed mitigation: In the Ollama response handler, after extracting the per-turn input token count, assign
a.LastContextTokens = <that count>. - Alternative mitigations considered: A default-to-zero approach would hide the metric for Ollama users, but the diff's intent is to show it for every backend that can report it. Ollama responses do carry usage info, so the metric is available.
- Severity calibration: Score 3: the bug is real and user-visible (wrong context-fill display), but it only affects Ollama users and does not cause data loss or crashes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/agent.go
Line: 133
Comment:
**Ollama backend does not update `LastContextTokens` after a turn**
The diff establishes a convention: every backend that reports per-turn input tokens sets `Agent.LastContextTokens` so the TUI can estimate context-window fill. The Anthropic streaming path (line 662), the non-streaming API path (line 822), and the Cerebras path (`cerebras_agent.go` line 71) all set `a.LastContextTokens = <per-turn input tokens>`. The Ollama path in `agent.go` does not, so after an Ollama turn `LastContextTokens` stays stale (zero or the previous backend's value), and the status bar shows a misleading context-fill percentage. Add `a.LastContextTokens = <ollama per-turn input tokens>` in the Ollama response-handling branch.
Example:
input: Ollama backend active, a turn completes with 5000 input tokens
actual: `LastContextTokens` remains 0 (or the previous backend's value)
expected: `LastContextTokens` == 5000, status bar shows correct context fill
Threat model:
Users on the Ollama backend see a stale or zero context-fill percentage in the input status bar, misleading them about how full the context window is.
Specific code citations:
Convention sites: `agent.go` line 662 (`a.LastContextTokens = ev.Message.Usage.InputTokens`), line 822 (`a.LastContextTokens = apiResp.Usage.InputTokens`), `cerebras_agent.go` line 71 (`a.LastContextTokens = resp.Usage.PromptTokens`). Violation site: the Ollama response-handling code in `agent.go` (the `callOllama` or equivalent function) — no `LastContextTokens` assignment.
Existing protections:
None. `LastContextTokens` is only set in the three paths listed above; the Ollama path is a fourth code path that handles its own response and never touches the field.
Proposed mitigation:
In the Ollama response handler, after extracting the per-turn input token count, assign `a.LastContextTokens = <that count>`.
Alternative mitigations considered:
A default-to-zero approach would hide the metric for Ollama users, but the diff's intent is to show it for every backend that can report it. Ollama responses do carry usage info, so the metric is available.
Severity calibration:
Score 3: the bug is real and user-visible (wrong context-fill display), but it only affects Ollama users and does not cause data loss or crashes.
Suggested fix shape:
a.LastContextTokens = ollamaResp.Usage.InputTokens
Why this wasn't caught:
No test covers `LastContextTokens` being set after an Ollama turn; the existing `TestContextWindow` only tests the window-size helper.
How can I resolve this? If you propose a fix, please make it concise.
| llmResult, err = cliAgent.Run(cleanInput, term) | ||
| term.StopThinking() | ||
| if err == nil { | ||
| if stats, ok := cliAgent.(agent.TurnStatsProvider); ok { |
There was a problem hiding this comment.
Direct-API backend silently omits per-turn status metrics that CC/Codex backends report
The TurnStatsProvider interface is only implemented by CCAgent. The REPL's post-turn block (line ~1164) calls cliAgent.(agent.TurnStatsProvider) only when cliAgent != nil, which is true for CC/Codex but false for the direct API path. The direct-API Agent struct has a LastContextTokens field populated during API calls, but it does not implement TurnStatsProvider, so the status bar's per-turn token counts and context-window estimate are silently absent when using the API backend. The inputStatus closure already reads ag.LastContextTokens directly, so the data is available — the gap is that the REPL never updates lastContextTokens from ag.LastContextTokens for the API path in the same structured way it does for CLI backends. Fix: either have the direct-API Agent implement TurnStatsProvider (returning a.LastContextTokens, a.Usage.OutputTokens - previousOutput, true), or add an explicit API-path branch in the REPL that reads ag.LastContextTokens and updates lastContextTokens.
More Info
- Threat model: Users on the direct API backend see no per-turn context-window fill or token counts in the status bar, while CC/Codex users do. This is a UX inconsistency, not a correctness bug — no data is lost, but the feature is silently gated on backend choice.
- Specific code citations:
TurnStatsProviderinterface defined ininternal/agent/cc_agent.go:32-35. Implemented only byCCAgentatcc_agent.go:631-635. REPL calls it atrepl.go:1164-1172insideif cliAgent != nil. Direct-APIAgenthasLastContextTokensatagent.go:66-69but noLastTurnStats()method. - Existing protections: None. The
inputStatusclosure readsag.LastContextTokensdirectly, but the REPL'slastContextTokenslocal is only updated via theTurnStatsProviderpath (CLI backends) or theelsebranch at line ~1193 (which sets it fromag.LastContextTokenswhencliAgent == nil). Theelsebranch at line 1193 does updatelastContextTokensfromag.LastContextTokens, so the status bar WILL show context usage for the API path — but only after the turn completes, and only via thelastContextTokenslocal, not via theTurnStatsProviderinterface. The asymmetry is that the API path has noTurnStatsProviderimplementation, making the interface a partial abstraction. - Proposed mitigation: Either implement
TurnStatsProvideron*Agent(returninga.LastContextTokens, a.Usage.OutputTokens - previousOutput, true) so the REPL's singlecliAgent.(agent.TurnStatsProvider)branch covers all backends, or add an explicitelsebranch in the REPL that readsag.LastContextTokensand updateslastContextTokensfor the API path. The latter already exists at line 1193, so the actual gap is narrower than it first appears. - Alternative mitigations considered: Remove
TurnStatsProviderand have the REPL always readag.LastContextTokensdirectly. This is simpler but loses the CC-specific output-token reporting. - Severity calibration: Score 3: the API path does update
lastContextTokensat line 1193, so the status bar is not completely broken — but theTurnStatsProviderinterface is a partial abstraction that only one backend implements, which is a design smell that will cause confusion when adding new backends.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 1164
Comment:
**Direct-API backend silently omits per-turn status metrics that CC/Codex backends report**
The `TurnStatsProvider` interface is only implemented by `CCAgent`. The REPL's post-turn block (line ~1164) calls `cliAgent.(agent.TurnStatsProvider)` only when `cliAgent != nil`, which is true for CC/Codex but false for the direct API path. The direct-API `Agent` struct has a `LastContextTokens` field populated during API calls, but it does not implement `TurnStatsProvider`, so the status bar's per-turn token counts and context-window estimate are silently absent when using the API backend. The `inputStatus` closure already reads `ag.LastContextTokens` directly, so the data is available — the gap is that the REPL never updates `lastContextTokens` from `ag.LastContextTokens` for the API path in the same structured way it does for CLI backends. Fix: either have the direct-API `Agent` implement `TurnStatsProvider` (returning `a.LastContextTokens, a.Usage.OutputTokens - previousOutput, true`), or add an explicit API-path branch in the REPL that reads `ag.LastContextTokens` and updates `lastContextTokens`.
Threat model:
Users on the direct API backend see no per-turn context-window fill or token counts in the status bar, while CC/Codex users do. This is a UX inconsistency, not a correctness bug — no data is lost, but the feature is silently gated on backend choice.
Specific code citations:
`TurnStatsProvider` interface defined in `internal/agent/cc_agent.go:32-35`. Implemented only by `CCAgent` at `cc_agent.go:631-635`. REPL calls it at `repl.go:1164-1172` inside `if cliAgent != nil`. Direct-API `Agent` has `LastContextTokens` at `agent.go:66-69` but no `LastTurnStats()` method.
Existing protections:
None. The `inputStatus` closure reads `ag.LastContextTokens` directly, but the REPL's `lastContextTokens` local is only updated via the `TurnStatsProvider` path (CLI backends) or the `else` branch at line ~1193 (which sets it from `ag.LastContextTokens` when `cliAgent == nil`). The `else` branch at line 1193 does update `lastContextTokens` from `ag.LastContextTokens`, so the status bar WILL show context usage for the API path — but only after the turn completes, and only via the `lastContextTokens` local, not via the `TurnStatsProvider` interface. The asymmetry is that the API path has no `TurnStatsProvider` implementation, making the interface a partial abstraction.
Proposed mitigation:
Either implement `TurnStatsProvider` on `*Agent` (returning `a.LastContextTokens, a.Usage.OutputTokens - previousOutput, true`) so the REPL's single `cliAgent.(agent.TurnStatsProvider)` branch covers all backends, or add an explicit `else` branch in the REPL that reads `ag.LastContextTokens` and updates `lastContextTokens` for the API path. The latter already exists at line 1193, so the actual gap is narrower than it first appears.
Alternative mitigations considered:
Remove `TurnStatsProvider` and have the REPL always read `ag.LastContextTokens` directly. This is simpler but loses the CC-specific output-token reporting.
Severity calibration:
Score 3: the API path does update `lastContextTokens` at line 1193, so the status bar is not completely broken — but the `TurnStatsProvider` interface is a partial abstraction that only one backend implements, which is a design smell that will cause confusion when adding new backends.
How can I resolve this? If you propose a fix, please make it concise.
| var ev sseMessageStart | ||
| if err := json.Unmarshal([]byte(rawData), &ev); err == nil { | ||
| a.Usage.InputTokens += ev.Message.Usage.InputTokens | ||
| a.LastContextTokens = ev.Message.Usage.InputTokens |
There was a problem hiding this comment.
LastContextTokens is not reset at turn start in the streaming API path, leaking the previous turn's value on failure
The CC backend (CCAgent.Run) resets lastTurnIn, lastTurnOut, and lastTurnOK to zero at the top of every turn, so a failed turn shows no stale context usage. The streaming API path (callStreamingAPI) only sets a.LastContextTokens when the SSE message_start event arrives (line 662). If the request fails before that event — or if the event never carries usage — the status bar retains the previous turn's LastContextTokens, misrepresenting the current context fill. The fix is to zero a.LastContextTokens at the start of callStreamingAPI, matching the CC backend's contract.
Example:
Turn 1: API succeeds, LastContextTokens = 12000. Turn 2: API fails before message_start. Status bar shows 'ctx 12.0k/200.0k (6%)' — the stale value from Turn 1.
Suggested fix:
a.LastContextTokens = 0Why this wasn't caught: No test exercises the LastContextTokens value after a failed API call.
More Info
- Threat model: A user sees a stale context-window percentage in the TUI status bar after a failed API call, leading to incorrect assumptions about how full the context window is.
- Specific code citations: CCAgent.Run resets lastTurnIn/Out/OK at line 361 of cc_agent.go. callStreamingAPI sets LastContextTokens at line 662 of agent.go but never resets it at the top of the function.
- Existing protections: The REPL's
lastContextTokensvariable is only updated on success (repl.go line 1195), but the Agent field itself is not cleared on failure, so a subsequent success that doesn't carry usage would still show the stale value. - Proposed mitigation: Add
a.LastContextTokens = 0at the top ofcallStreamingAPI, before the request is sent. - Alternative mitigations considered: Resetting in the REPL after every turn would work but would miss the case where the Agent is used outside the REPL. Resetting in the Agent is the correct single point of truth.
- Severity calibration: Score 3: a stale UI metric, not a correctness or security bug. The blast radius is limited to the status bar display, and the likelihood is moderate (any failed API call triggers it).
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/agent.go
Line: 662
Comment:
**`LastContextTokens` is not reset at turn start in the streaming API path, leaking the previous turn's value on failure**
The CC backend (`CCAgent.Run`) resets `lastTurnIn`, `lastTurnOut`, and `lastTurnOK` to zero at the top of every turn, so a failed turn shows no stale context usage. The streaming API path (`callStreamingAPI`) only sets `a.LastContextTokens` when the SSE `message_start` event arrives (line 662). If the request fails before that event — or if the event never carries usage — the status bar retains the previous turn's `LastContextTokens`, misrepresenting the current context fill. The fix is to zero `a.LastContextTokens` at the start of `callStreamingAPI`, matching the CC backend's contract.
Example:
Turn 1: API succeeds, LastContextTokens = 12000. Turn 2: API fails before message_start. Status bar shows 'ctx 12.0k/200.0k (6%)' — the stale value from Turn 1.
Threat model:
A user sees a stale context-window percentage in the TUI status bar after a failed API call, leading to incorrect assumptions about how full the context window is.
Specific code citations:
CCAgent.Run resets lastTurnIn/Out/OK at line 361 of cc_agent.go. callStreamingAPI sets LastContextTokens at line 662 of agent.go but never resets it at the top of the function.
Existing protections:
The REPL's `lastContextTokens` variable is only updated on success (repl.go line 1195), but the Agent field itself is not cleared on failure, so a subsequent success that doesn't carry usage would still show the stale value.
Proposed mitigation:
Add `a.LastContextTokens = 0` at the top of `callStreamingAPI`, before the request is sent.
Alternative mitigations considered:
Resetting in the REPL after every turn would work but would miss the case where the Agent is used outside the REPL. Resetting in the Agent is the correct single point of truth.
Severity calibration:
Score 3: a stale UI metric, not a correctness or security bug. The blast radius is limited to the status bar display, and the likelihood is moderate (any failed API call triggers it).
Suggested fix shape:
a.LastContextTokens = 0
Why this wasn't caught:
No test exercises the LastContextTokens value after a failed API call.
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| // Track usage | ||
| a.Usage.InputTokens += apiResp.Usage.InputTokens | ||
| a.LastContextTokens = apiResp.Usage.InputTokens |
There was a problem hiding this comment.
LastContextTokens is not reset at turn start in the direct API path, leaking the previous turn's value on failure
The CC backend resets its per-turn usage counters at the top of Run(). The direct API path (callAPI) sets a.LastContextTokens only after a successful response (line 822). If the HTTP request fails, times out, or returns a non-2xx status, LastContextTokens retains the previous turn's value. The fix is to zero a.LastContextTokens at the start of callAPI, matching the CC backend's contract.
Example:
Turn 1: direct API succeeds, LastContextTokens = 8000. Turn 2: HTTP 502. Status bar shows 'ctx 8.0k/200.0k (4%)' — stale.
Suggested fix:
a.LastContextTokens = 0Why this wasn't caught: No test exercises the LastContextTokens value after a failed direct API call.
More Info
- Threat model: Same as the streaming path: a stale context-window percentage in the TUI after a failed direct API call.
- Specific code citations: CCAgent.Run resets at cc_agent.go line 361. callAPI sets LastContextTokens at agent.go line 822 but never resets it at the top.
- Existing protections: The REPL only reads LastContextTokens on success (repl.go line 1195), but the field itself is not cleared on failure.
- Proposed mitigation: Add
a.LastContextTokens = 0at the top ofcallAPI, before the HTTP request. - Alternative mitigations considered: Same as the streaming path — resetting in the REPL is less robust than resetting in the Agent.
- Severity calibration: Score 3: stale UI metric, moderate likelihood on any failed direct API call.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/agent.go
Line: 822
Comment:
**`LastContextTokens` is not reset at turn start in the direct API path, leaking the previous turn's value on failure**
The CC backend resets its per-turn usage counters at the top of `Run()`. The direct API path (`callAPI`) sets `a.LastContextTokens` only after a successful response (line 822). If the HTTP request fails, times out, or returns a non-2xx status, `LastContextTokens` retains the previous turn's value. The fix is to zero `a.LastContextTokens` at the start of `callAPI`, matching the CC backend's contract.
Example:
Turn 1: direct API succeeds, LastContextTokens = 8000. Turn 2: HTTP 502. Status bar shows 'ctx 8.0k/200.0k (4%)' — stale.
Threat model:
Same as the streaming path: a stale context-window percentage in the TUI after a failed direct API call.
Specific code citations:
CCAgent.Run resets at cc_agent.go line 361. callAPI sets LastContextTokens at agent.go line 822 but never resets it at the top.
Existing protections:
The REPL only reads LastContextTokens on success (repl.go line 1195), but the field itself is not cleared on failure.
Proposed mitigation:
Add `a.LastContextTokens = 0` at the top of `callAPI`, before the HTTP request.
Alternative mitigations considered:
Same as the streaming path — resetting in the REPL is less robust than resetting in the Agent.
Severity calibration:
Score 3: stale UI metric, moderate likelihood on any failed direct API call.
Suggested fix shape:
a.LastContextTokens = 0
Why this wasn't caught:
No test exercises the LastContextTokens value after a failed direct API call.
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| choice := resp.Choices[0] | ||
| a.Usage.InputTokens += resp.Usage.PromptTokens | ||
| a.LastContextTokens = resp.Usage.PromptTokens |
There was a problem hiding this comment.
LastContextTokens is not reset at turn start in the Cerebras path, leaking the previous turn's value on failure
The CC backend resets its per-turn usage counters at the top of Run(). The Cerebras path (RunCerebrasAgent) sets a.LastContextTokens only after a successful response (line 71). If the Cerebras request fails, LastContextTokens retains the previous turn's value. The fix is to zero a.LastContextTokens at the start of RunCerebrasAgent, matching the CC backend's contract.
Example:
Turn 1: Cerebras succeeds, LastContextTokens = 5000. Turn 2: Cerebras API error. Status bar shows 'ctx 5.0k/200.0k (2%)' — stale.
Suggested fix:
a.LastContextTokens = 0Why this wasn't caught: No test exercises the LastContextTokens value after a failed Cerebras call.
More Info
- Threat model: Stale context-window percentage in the TUI after a failed Cerebras call.
- Specific code citations: CCAgent.Run resets at cc_agent.go line 361. RunCerebrasAgent sets LastContextTokens at cerebras_agent.go line 71 but never resets it at the top.
- Existing protections: The REPL only reads LastContextTokens on success (repl.go line 1195), but the field itself is not cleared on failure.
- Proposed mitigation: Add
a.LastContextTokens = 0at the top ofRunCerebrasAgent, before the request loop. - Alternative mitigations considered: Resetting in the REPL is less robust than resetting in the Agent.
- Severity calibration: Score 3: stale UI metric, moderate likelihood on any failed Cerebras call.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/cerebras_agent.go
Line: 71
Comment:
**`LastContextTokens` is not reset at turn start in the Cerebras path, leaking the previous turn's value on failure**
The CC backend resets its per-turn usage counters at the top of `Run()`. The Cerebras path (`RunCerebrasAgent`) sets `a.LastContextTokens` only after a successful response (line 71). If the Cerebras request fails, `LastContextTokens` retains the previous turn's value. The fix is to zero `a.LastContextTokens` at the start of `RunCerebrasAgent`, matching the CC backend's contract.
Example:
Turn 1: Cerebras succeeds, LastContextTokens = 5000. Turn 2: Cerebras API error. Status bar shows 'ctx 5.0k/200.0k (2%)' — stale.
Threat model:
Stale context-window percentage in the TUI after a failed Cerebras call.
Specific code citations:
CCAgent.Run resets at cc_agent.go line 361. RunCerebrasAgent sets LastContextTokens at cerebras_agent.go line 71 but never resets it at the top.
Existing protections:
The REPL only reads LastContextTokens on success (repl.go line 1195), but the field itself is not cleared on failure.
Proposed mitigation:
Add `a.LastContextTokens = 0` at the top of `RunCerebrasAgent`, before the request loop.
Alternative mitigations considered:
Resetting in the REPL is less robust than resetting in the Agent.
Severity calibration:
Score 3: stale UI metric, moderate likelihood on any failed Cerebras call.
Suggested fix shape:
a.LastContextTokens = 0
Why this wasn't caught:
No test exercises the LastContextTokens value after a failed Cerebras call.
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| var inputHistory []string | ||
| var lastCtrlC time.Time | ||
| sessionStarted := time.Now() |
There was a problem hiding this comment.
LastContextTokens is set from different sources in API vs CLI paths, breaking the declared contract
The Agent.LastContextTokens field is documented as 'the input-token count reported by the most recent model request' and is set directly in callStreamingAPI, callAPI, and RunCerebrasAgent. However, in the CLI-agent path (CC/Codex), lastContextTokens is instead set from TurnStatsProvider.LastTurnStats(), which returns the input tokens from the CLI subprocess's result event. This means ag.LastContextTokens is stale (zero or from a prior API call) when a CLI backend is active, while the REPL's local lastContextTokens variable holds the correct value. The inputStatus closure reads lastContextTokens into StatusInfo.ContextUsed, so the display is correct, but the Agent field's contract is violated — it does not reflect the most recent model request when a CLI backend is used.
More Info
- Threat model: Any code that reads
ag.LastContextTokensexpecting it to reflect the most recent request (as documented) will get a stale or zero value when a CLI backend is active. This is a latent correctness bug for future consumers of the field. - Specific code citations:
internal/agent/agent.go:66-68declaresLastContextTokenswith the contract 'input-token count reported by the most recent model request'.internal/agent/agent.go:662andinternal/agent/agent.go:822set it in the API paths.internal/repl/repl.go:1164-1170sets the locallastContextTokensfromTurnStatsProviderbut never updatesag.LastContextTokens. - Existing protections: The REPL's
inputStatusclosure reads the locallastContextTokensvariable, notag.LastContextTokens, so the TUI display is correct. No other code in this diff readsag.LastContextTokens. - Proposed mitigation: After the
TurnStatsProviderblock in the REPL, also setag.LastContextTokens = inputTokensso the field stays consistent with its documented contract. - Alternative mitigations considered: Remove the
LastContextTokensfield fromAgentand rely solely on the REPL-local variable, but that would break the API path's direct usage. Keeping the field and updating it in both paths is the minimal fix. - Severity calibration: Score 3 because the TUI display is correct (the local variable is used), but the documented field contract is broken, creating a latent bug for any future code that reads
ag.LastContextTokens.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 165
Comment:
**`LastContextTokens` is set from different sources in API vs CLI paths, breaking the declared contract**
The `Agent.LastContextTokens` field is documented as 'the input-token count reported by the most recent model request' and is set directly in `callStreamingAPI`, `callAPI`, and `RunCerebrasAgent`. However, in the CLI-agent path (CC/Codex), `lastContextTokens` is instead set from `TurnStatsProvider.LastTurnStats()`, which returns the input tokens from the CLI subprocess's result event. This means `ag.LastContextTokens` is stale (zero or from a prior API call) when a CLI backend is active, while the REPL's local `lastContextTokens` variable holds the correct value. The `inputStatus` closure reads `lastContextTokens` into `StatusInfo.ContextUsed`, so the display is correct, but the `Agent` field's contract is violated — it does not reflect the most recent model request when a CLI backend is used.
Threat model:
Any code that reads `ag.LastContextTokens` expecting it to reflect the most recent request (as documented) will get a stale or zero value when a CLI backend is active. This is a latent correctness bug for future consumers of the field.
Specific code citations:
`internal/agent/agent.go:66-68` declares `LastContextTokens` with the contract 'input-token count reported by the most recent model request'. `internal/agent/agent.go:662` and `internal/agent/agent.go:822` set it in the API paths. `internal/repl/repl.go:1164-1170` sets the local `lastContextTokens` from `TurnStatsProvider` but never updates `ag.LastContextTokens`.
Existing protections:
The REPL's `inputStatus` closure reads the local `lastContextTokens` variable, not `ag.LastContextTokens`, so the TUI display is correct. No other code in this diff reads `ag.LastContextTokens`.
Proposed mitigation:
After the `TurnStatsProvider` block in the REPL, also set `ag.LastContextTokens = inputTokens` so the field stays consistent with its documented contract.
Alternative mitigations considered:
Remove the `LastContextTokens` field from `Agent` and rely solely on the REPL-local variable, but that would break the API path's direct usage. Keeping the field and updating it in both paths is the minimal fix.
Severity calibration:
Score 3 because the TUI display is correct (the local variable is used), but the documented field contract is broken, creating a latent bug for any future code that reads `ag.LastContextTokens`.
How can I resolve this? If you propose a fix, please make it concise.
| ContextWindow int // assumed context window of the active model | ||
| LastTurnDur time.Duration // wall-clock duration of the previous agent turn | ||
| SessionStarted time.Time // used to keep the session timer live while typing | ||
| SessionDur time.Duration // fallback when SessionStarted is unavailable |
There was a problem hiding this comment.
StatusInfo.SessionDur is declared as a fallback but never populated by the REPL
The StatusInfo struct declares SessionDur with the comment 'fallback when SessionStarted is unavailable', and sessionDuration() checks SessionStarted first, falling back to SessionDur. However, the REPL's inputStatus closure never sets SessionDur — it only sets SessionStarted. This means the fallback path is dead code: if SessionStarted were ever zero, sessionDuration() would return the zero value of SessionDur (0), producing no session timer display. Either the field should be removed, or the REPL should populate it when SessionStarted is unavailable.
More Info
- Threat model: If a future code path creates a
StatusInfowithoutSessionStarted(e.g., a non-interactive mode), the session timer would silently show nothing instead of a meaningful duration. - Specific code citations:
internal/tui/input.go:31declaresSessionDurwith the fallback comment.internal/tui/input.go:607-611implements the fallback logic.internal/repl/repl.go:169-200constructsStatusInfoand setsSessionStartedbut neverSessionDur. - Existing protections: The REPL always sets
SessionStarted, so the fallback is never exercised in the current code paths. - Proposed mitigation: Either remove
SessionDurfromStatusInfo(since it's unused) or populate it in the REPL whenSessionStartedis unavailable. Removing it is simpler and avoids dead code. - Alternative mitigations considered: Keep the field and document it as reserved for future use, but dead code with a misleading comment is worse than no code.
- Severity calibration: Score 2 because the current code paths always set
SessionStarted, so the bug is latent. The dead code and misleading comment are a minor maintainability issue.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/input.go
Line: 32
Comment:
**`StatusInfo.SessionDur` is declared as a fallback but never populated by the REPL**
The `StatusInfo` struct declares `SessionDur` with the comment 'fallback when SessionStarted is unavailable', and `sessionDuration()` checks `SessionStarted` first, falling back to `SessionDur`. However, the REPL's `inputStatus` closure never sets `SessionDur` — it only sets `SessionStarted`. This means the fallback path is dead code: if `SessionStarted` were ever zero, `sessionDuration()` would return the zero value of `SessionDur` (0), producing no session timer display. Either the field should be removed, or the REPL should populate it when `SessionStarted` is unavailable.
Threat model:
If a future code path creates a `StatusInfo` without `SessionStarted` (e.g., a non-interactive mode), the session timer would silently show nothing instead of a meaningful duration.
Specific code citations:
`internal/tui/input.go:31` declares `SessionDur` with the fallback comment. `internal/tui/input.go:607-611` implements the fallback logic. `internal/repl/repl.go:169-200` constructs `StatusInfo` and sets `SessionStarted` but never `SessionDur`.
Existing protections:
The REPL always sets `SessionStarted`, so the fallback is never exercised in the current code paths.
Proposed mitigation:
Either remove `SessionDur` from `StatusInfo` (since it's unused) or populate it in the REPL when `SessionStarted` is unavailable. Removing it is simpler and avoids dead code.
Alternative mitigations considered:
Keep the field and document it as reserved for future use, but dead code with a misleading comment is worse than no code.
Severity calibration:
Score 2 because the current code paths always set `SessionStarted`, so the bug is latent. The dead code and misleading comment are a minor maintainability issue.
How can I resolve this? If you propose a fix, please make it concise.
✅ QualityMax Pipeline
Powered by QualityMax — AI-Powered Test Automation |
There was a problem hiding this comment.
2 findings outside the diff
| File | Scope | Finding |
|---|---|---|
| internal/repl/repl.go:1115-1123 | file-scope | Missing authorization check before updating agent usage from CLI backend stats |
| internal/agent/agent.go | file-scope | New LastContextTokens field and its reset logic are untested |
| // All backends share this top-level turn boundary, including Ollama. Ollama's | ||
| // streaming protocol does not expose usage, so leaving this at zero hides the | ||
| // context-fill metric instead of carrying a stale value from another backend. | ||
| a.LastContextTokens = 0 |
There was a problem hiding this comment.
Ollama streaming path never updates LastContextTokens
runStreamingLoop sets a.LastContextTokens = 0 at the top and the comment says 'Ollama's streaming protocol does not expose usage, so leaving this at zero hides the context-fill metric'. However, the Ollama path inside runStreamingLoop (line 338-344) calls a.runOllamaLoop(term) which returns without ever setting LastContextTokens. This is intentional per the comment, but the REPL's inputStatus closure reads lastContextTokens (which is set from ag.LastContextTokens only on success). If the Ollama turn succeeds, lastContextTokens stays at the zero reset from the top of the turn, so the status panel shows no context-fill metric — which is the desired behavior. No bug here.
More Info
- Threat model: N/A — this is intentional behavior.
- Specific code citations:
internal/agent/agent.golines 335-344. - Existing protections: The comment on line 338 explicitly documents the intent.
- Proposed mitigation: No fix needed.
- Alternative mitigations considered: N/A
- Severity calibration: Score 4 is too high; this is intentional.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/agent.go
Line: 341
Comment:
**Ollama streaming path never updates LastContextTokens**
`runStreamingLoop` sets `a.LastContextTokens = 0` at the top and the comment says 'Ollama's streaming protocol does not expose usage, so leaving this at zero hides the context-fill metric'. However, the Ollama path inside `runStreamingLoop` (line 338-344) calls `a.runOllamaLoop(term)` which returns without ever setting `LastContextTokens`. This is intentional per the comment, but the REPL's `inputStatus` closure reads `lastContextTokens` (which is set from `ag.LastContextTokens` only on success). If the Ollama turn succeeds, `lastContextTokens` stays at the zero reset from the top of the turn, so the status panel shows no context-fill metric — which is the desired behavior. No bug here.
Threat model:
N/A — this is intentional behavior.
Specific code citations:
`internal/agent/agent.go` lines 335-344.
Existing protections:
The comment on line 338 explicitly documents the intent.
Proposed mitigation:
No fix needed.
Alternative mitigations considered:
N/A
Severity calibration:
Score 4 is too high; this is intentional.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+1098
to
+1100
| // Clear the UI's per-turn estimate before starting any backend. This also | ||
| // covers CLI backends, whose usage is reported outside Agent.RunStreaming. | ||
| lastContextTokens = 0 |
There was a problem hiding this comment.
Race on Agent.LastContextTokens between REPL and agent goroutines
ag.LastContextTokens is written by the agent's streaming goroutine (in callStreamingAPI when processing SSE message_start events) and read by the REPL goroutine (in the inputStatus closure and on line 1200). There is no synchronization — no mutex, no atomic, no channel. In Go, concurrent reads and writes to a plain int field are a data race. The fix is to protect LastContextTokens with a mutex or use atomic.Int64.
More Info
- Threat model: Under the Go memory model, concurrent read/write of a non-atomic int is undefined behavior. The race detector will flag it, and in practice the TUI could read a torn or stale value.
- Specific code citations: Write:
internal/agent/agent.goline 670 (a.LastContextTokens = ev.Message.Usage.InputTokens) insidecallStreamingAPI. Read:internal/repl/repl.goline 1200 (lastContextTokens = ag.LastContextTokens) and line 192 (ContextUsed: lastContextTokens). - Existing protections: None — no mutex, no atomic, no channel.
- Proposed mitigation: Change
LastContextTokenstoatomic.Int64in theAgentstruct, or wrap accesses with a mutex. - Alternative mitigations considered: Could pass the value through a channel, but an atomic is simpler for a single scalar.
- Severity calibration: Score 4 — a real data race that the Go race detector will catch. In practice the impact is a stale/zero display, but it's a correctness bug in concurrent code.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 1098-1100
Comment:
**Race on Agent.LastContextTokens between REPL and agent goroutines**
`ag.LastContextTokens` is written by the agent's streaming goroutine (in `callStreamingAPI` when processing SSE `message_start` events) and read by the REPL goroutine (in the `inputStatus` closure and on line 1200). There is no synchronization — no mutex, no atomic, no channel. In Go, concurrent reads and writes to a plain `int` field are a data race. The fix is to protect `LastContextTokens` with a mutex or use `atomic.Int64`.
Threat model:
Under the Go memory model, concurrent read/write of a non-atomic int is undefined behavior. The race detector will flag it, and in practice the TUI could read a torn or stale value.
Specific code citations:
Write: `internal/agent/agent.go` line 670 (`a.LastContextTokens = ev.Message.Usage.InputTokens`) inside `callStreamingAPI`. Read: `internal/repl/repl.go` line 1200 (`lastContextTokens = ag.LastContextTokens`) and line 192 (`ContextUsed: lastContextTokens`).
Existing protections:
None — no mutex, no atomic, no channel.
Proposed mitigation:
Change `LastContextTokens` to `atomic.Int64` in the `Agent` struct, or wrap accesses with a mutex.
Alternative mitigations considered:
Could pass the value through a channel, but an atomic is simpler for a single scalar.
Severity calibration:
Score 4 — a real data race that the Go race detector will catch. In practice the impact is a stale/zero display, but it's a correctness bug in concurrent code.
How can I resolve this? If you propose a fix, please make it concise.
| } else { | ||
| llmResult, err = ag.RunStreaming(input, term) | ||
| } | ||
| lastTurnDur = time.Since(turnStarted) |
There was a problem hiding this comment.
Stale lastContextTokens after a failed turn
After a failed turn (err != nil), lastContextTokens is not updated, so the status panel continues to show the context usage from the previous successful turn. This is misleading — the user sees a context-fill percentage that does not reflect the current state. Fix: set lastContextTokens = 0 in the error path, matching the reset done at the start of each turn.
More Info
- Threat model: The user sees a stale context-fill percentage after a failed request, which could lead them to believe the context window is more full than it actually is.
- Specific code citations: internal/repl/repl.go lines 1195-1201: the if err == nil block updates lastTask and lastContextTokens; the error path does not.
- Existing protections: None — the error path falls through without touching lastContextTokens.
- Proposed mitigation: Add lastContextTokens = 0 (or ag.LastContextTokens) in an else branch or before the if block.
- Alternative mitigations considered: Could also set lastContextTokens = ag.LastContextTokens unconditionally after the turn, but that would require ensuring ag.LastContextTokens is zeroed on failure.
- Severity calibration: Score 3 — a minor UX issue (stale display) with no data loss or crash.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 1198
Comment:
**Stale lastContextTokens after a failed turn**
After a failed turn (err != nil), lastContextTokens is not updated, so the status panel continues to show the context usage from the previous successful turn. This is misleading — the user sees a context-fill percentage that does not reflect the current state. Fix: set lastContextTokens = 0 in the error path, matching the reset done at the start of each turn.
Threat model:
The user sees a stale context-fill percentage after a failed request, which could lead them to believe the context window is more full than it actually is.
Specific code citations:
internal/repl/repl.go lines 1195-1201: the if err == nil block updates lastTask and lastContextTokens; the error path does not.
Existing protections:
None — the error path falls through without touching lastContextTokens.
Proposed mitigation:
Add lastContextTokens = 0 (or ag.LastContextTokens) in an else branch or before the if block.
Alternative mitigations considered:
Could also set lastContextTokens = ag.LastContextTokens unconditionally after the turn, but that would require ensuring ag.LastContextTokens is zeroed on failure.
Severity calibration:
Score 3 — a minor UX issue (stale display) with no data loss or crash.
How can I resolve this? If you propose a fix, please make it concise.
| // counts, context-window fill, timing, and the mode/task bottom bar. Zero | ||
| // values are omitted from the display, so backends that can't report a given | ||
| // metric simply don't show it. | ||
| type StatusInfo struct { |
There was a problem hiding this comment.
New StatusInfo struct and rendering functions have only basic rendering tests
The PR adds a StatusInfo struct and complex rendering logic in renderStatus, describeBackend, compactTokens, compactDuration. The existing test TestInputSeparatesPromptAndRendersSessionStatus only checks that certain substrings appear in the view, but does not verify edge cases like zero values, overflow formatting, or timer ticks. Add unit tests for the formatting helpers and for renderStatus with varied inputs.
More Info
- Threat model: If formatting functions misbehave (e.g., divide-by-zero, incorrect rounding), the status panel may show garbled text or panic.
- Specific code citations: Lines 15-28 define StatusInfo; lines 536-640 implement renderStatus, compactTokens, compactDuration.
- Existing protections: input_test.go includes one integration test that uses a sample StatusInfo, but does not test the helpers directly.
- Proposed mitigation: Add focused unit tests for compactTokens and compactDuration covering boundary values, and test renderStatus with nil status, zero fields, and large durations.
- Alternative mitigations considered: Rely on the integration test alone, but edge cases may slip through.
- Severity calibration: Score 3 because the helpers are internal and failures would be visible in UI, but unlikely to cause crashes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/input.go
Line: 19
Comment:
**New StatusInfo struct and rendering functions have only basic rendering tests**
The PR adds a StatusInfo struct and complex rendering logic in renderStatus, describeBackend, compactTokens, compactDuration. The existing test TestInputSeparatesPromptAndRendersSessionStatus only checks that certain substrings appear in the view, but does not verify edge cases like zero values, overflow formatting, or timer ticks. Add unit tests for the formatting helpers and for renderStatus with varied inputs.
Threat model:
If formatting functions misbehave (e.g., divide-by-zero, incorrect rounding), the status panel may show garbled text or panic.
Specific code citations:
Lines 15-28 define StatusInfo; lines 536-640 implement renderStatus, compactTokens, compactDuration.
Existing protections:
input_test.go includes one integration test that uses a sample StatusInfo, but does not test the helpers directly.
Proposed mitigation:
Add focused unit tests for compactTokens and compactDuration covering boundary values, and test renderStatus with nil status, zero fields, and large durations.
Alternative mitigations considered:
Rely on the integration test alone, but edge cases may slip through.
Severity calibration:
Score 3 because the helpers are internal and failures would be visible in UI, but unlikely to cause crashes.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+546
to
+550
| // ── Metrics line: token window · tokens · time ────────────────────── | ||
| var metrics []string | ||
| if s.ContextUsed > 0 && s.ContextWindow > 0 { | ||
| pct := s.ContextUsed * 100 / s.ContextWindow | ||
| metrics = append(metrics, fmt.Sprintf("ctx %s/%s (%d%%)", |
There was a problem hiding this comment.
Context-fill percentage division by zero when ContextWindow is 0
Low-confidence finding — expand to read
renderStatus computes pct := s.ContextUsed * 100 / s.ContextWindow guarded only by s.ContextUsed > 0 && s.ContextWindow > 0. If a future caller passes a StatusInfo with ContextUsed > 0 but ContextWindow == 0 (e.g., a model whose api.ContextWindow returns 0 for an unknown model ID), the guard is bypassed and the integer division panics at runtime. The fix is to add s.ContextWindow > 0 to the guard, which is already the intended check but is missing the > operator.
More Info
- Threat model: A user with a misconfigured or unknown model ID triggers a panic in the TUI input rendering goroutine, crashing the terminal UI.
- Specific code citations:
internal/tui/input.goline 550:if s.ContextUsed > 0 && s.ContextWindow > 0— the guard uses> 0for both, but the divisions.ContextUsed * 100 / s.ContextWindowon line 552 would still panic ifContextWindowwere 0. The guard is correct as written; the bug is that the guard is correct but the finding is a false alarm — the guard already checkss.ContextWindow > 0. - Existing protections: The guard
s.ContextUsed > 0 && s.ContextWindow > 0already prevents division by zero. - Proposed mitigation: No fix needed; the guard is already correct.
- Alternative mitigations considered: N/A
- Severity calibration: Score 3 because the guard is already present and correct; this is a false alarm.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/input.go
Line: 546-550
Comment:
**Context-fill percentage division by zero when ContextWindow is 0**
`renderStatus` computes `pct := s.ContextUsed * 100 / s.ContextWindow` guarded only by `s.ContextUsed > 0 && s.ContextWindow > 0`. If a future caller passes a `StatusInfo` with `ContextUsed > 0` but `ContextWindow == 0` (e.g., a model whose `api.ContextWindow` returns 0 for an unknown model ID), the guard is bypassed and the integer division panics at runtime. The fix is to add `s.ContextWindow > 0` to the guard, which is already the intended check but is missing the `>` operator.
Threat model:
A user with a misconfigured or unknown model ID triggers a panic in the TUI input rendering goroutine, crashing the terminal UI.
Specific code citations:
`internal/tui/input.go` line 550: `if s.ContextUsed > 0 && s.ContextWindow > 0` — the guard uses `> 0` for both, but the division `s.ContextUsed * 100 / s.ContextWindow` on line 552 would still panic if `ContextWindow` were 0. The guard is correct as written; the bug is that the guard is correct but the finding is a false alarm — the guard already checks `s.ContextWindow > 0`.
Existing protections:
The guard `s.ContextUsed > 0 && s.ContextWindow > 0` already prevents division by zero.
Proposed mitigation:
No fix needed; the guard is already correct.
Alternative mitigations considered:
N/A
Severity calibration:
Score 3 because the guard is already present and correct; this is a false alarm.
How can I resolve this? If you propose a fix, please make it concise.
| func (a *Agent) Run(prompt string) (string, error) { | ||
| // This is a new top-level turn; do not show a prior request's context size | ||
| // if this request fails before the provider reports usage. | ||
| a.LastContextTokens = 0 |
There was a problem hiding this comment.
Agent.Run leaves LastContextTokens stale when callAPI fails
Run resets LastContextTokens to 0 at the top of the method (line 149) to avoid showing a prior request's context size on failure. However, if callAPI (line 160) returns an error, the method returns immediately without updating LastContextTokens — the field retains the zero value set at the top. This is the correct contract (zero = no context estimate), but it is fragile: any future code that reads LastContextTokens between Run calls on the same Agent instance could misinterpret a zero as "no context used" when the last successful request actually had a non-zero context. The sibling runStreamingLoop (line 338) and the REPL's CLI-backend path (repl.go line 1098) both explicitly reset to zero before starting a turn, establishing a convention that zero means "this turn has not yet reported usage." Run follows this convention but does so implicitly via the top-of-method reset; the early-return path is correct but undocumented. Consider adding a comment at the early return noting that the zero value is intentional, or resetting LastContextTokens in a defer to make the contract explicit regardless of exit path.
Example:
input: Agent.Run('test') where callAPI fails
actual: LastContextTokens = 0 (stale from top-of-method reset)
expected: LastContextTokens = 0 (intentional, but fragile)
Suggested fix:
// At the early return: // LastContextTokens intentionally remains 0 — this turn produced no usage.Why this wasn't caught: No test exercises the Run failure path and asserts LastContextTokens remains zero.
More Info
- Threat model: A future maintainer adds code between
Runcalls that readsLastContextTokensand assumes a non-zero value means the last request succeeded. The stale zero from a failedRunwould be indistinguishable from a fresh Agent with no history. - Specific code citations:
Agent.Run(agent.go:146-170) resetsLastContextTokensat line 149, callscallAPIat line 160, and returns early on error at line 162 without touching the field again.runStreamingLoop(agent.go:338) andrepl.go:1098both explicitly reset to zero. - Existing protections: The REPL's
inputStatusclosure readsag.LastContextTokensonly after a successful turn (repl.go line 1200), so the stale zero is not currently observable. The protection is temporal coupling, not a guard. - Proposed mitigation: Add a comment at the early return (line 162) noting that
LastContextTokensintentionally remains zero, or wrap the reset in a defer to make the contract independent of exit path. - Alternative mitigations considered: A defer-based reset would be more robust but changes the control flow for the success path as well. A comment is the smallest fix.
- Severity calibration: Score 3 because the bug is latent — no current code path reads the stale value incorrectly — but the inconsistency between the three reset sites makes the contract easy to break in future changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/agent.go
Line: 151
Comment:
**`Agent.Run` leaves `LastContextTokens` stale when `callAPI` fails**
`Run` resets `LastContextTokens` to 0 at the top of the method (line 149) to avoid showing a prior request's context size on failure. However, if `callAPI` (line 160) returns an error, the method returns immediately without updating `LastContextTokens` — the field retains the zero value set at the top. This is the correct contract (zero = no context estimate), but it is fragile: any future code that reads `LastContextTokens` between `Run` calls on the same Agent instance could misinterpret a zero as "no context used" when the last successful request actually had a non-zero context. The sibling `runStreamingLoop` (line 338) and the REPL's CLI-backend path (repl.go line 1098) both explicitly reset to zero before starting a turn, establishing a convention that zero means "this turn has not yet reported usage." `Run` follows this convention but does so implicitly via the top-of-method reset; the early-return path is correct but undocumented. Consider adding a comment at the early return noting that the zero value is intentional, or resetting `LastContextTokens` in a defer to make the contract explicit regardless of exit path.
Example:
input: Agent.Run('test') where callAPI fails
actual: LastContextTokens = 0 (stale from top-of-method reset)
expected: LastContextTokens = 0 (intentional, but fragile)
Threat model:
A future maintainer adds code between `Run` calls that reads `LastContextTokens` and assumes a non-zero value means the last request succeeded. The stale zero from a failed `Run` would be indistinguishable from a fresh Agent with no history.
Specific code citations:
`Agent.Run` (agent.go:146-170) resets `LastContextTokens` at line 149, calls `callAPI` at line 160, and returns early on error at line 162 without touching the field again. `runStreamingLoop` (agent.go:338) and `repl.go:1098` both explicitly reset to zero.
Existing protections:
The REPL's `inputStatus` closure reads `ag.LastContextTokens` only after a successful turn (repl.go line 1200), so the stale zero is not currently observable. The protection is temporal coupling, not a guard.
Proposed mitigation:
Add a comment at the early return (line 162) noting that `LastContextTokens` intentionally remains zero, or wrap the reset in a defer to make the contract independent of exit path.
Alternative mitigations considered:
A defer-based reset would be more robust but changes the control flow for the success path as well. A comment is the smallest fix.
Severity calibration:
Score 3 because the bug is latent — no current code path reads the stale value incorrectly — but the inconsistency between the three reset sites makes the contract easy to break in future changes.
Suggested fix shape:
// At the early return: // LastContextTokens intentionally remains 0 — this turn produced no usage.
Why this wasn't caught:
No test exercises the `Run` failure path and asserts `LastContextTokens` remains zero.
How can I resolve this? If you propose a fix, please make it concise.
| var err error | ||
| // Clear the UI's per-turn estimate before starting any backend. This also | ||
| // covers CLI backends, whose usage is reported outside Agent.RunStreaming. | ||
| lastContextTokens = 0 |
There was a problem hiding this comment.
CLI-backend error path leaves lastContextTokens and ag.LastContextTokens stale
The REPL's main turn loop resets lastContextTokens and ag.LastContextTokens to zero at line 1098 before starting any backend. For CLI backends (cc/codex), the success path updates both fields from TurnStatsProvider at lines 1168-1173. However, if cliAgent.Run returns an error (line 1165), the code falls through to line 1198 where lastTurnDur is recorded, but neither lastContextTokens nor ag.LastContextTokens is updated — they remain zero from the pre-turn reset. This is consistent with the convention that zero means "no usage reported this turn," but the success path for the direct-API backend (line 1200) explicitly copies ag.LastContextTokens into lastContextTokens only when err == nil. The CLI-backend error path does not mirror this guard, making the contract implicit. If a future change reads lastContextTokens after a failed CLI turn, it would see zero and could misinterpret it as "the model used no context" rather than "the turn failed."
Example:
input: CLI backend (cc) turn that fails mid-request
actual: lastContextTokens = 0, ag.LastContextTokens = 0 (from pre-turn reset)
expected: same values, but the contract is implicit — a reader might expect a sentinel or explicit guard
Suggested fix:
// After the CLI error path, before lastTurnDur:
// lastContextTokens intentionally remains 0 — this turn produced no usage.Why this wasn't caught: No test exercises a failed CLI-backend turn and asserts the context-token fields remain zero.
More Info
- Threat model: A future UI change that displays
lastContextTokensafter a failed turn would show zero, misleading the user into thinking the model context was empty when the turn actually failed before reporting usage. - Specific code citations: REPL turn loop (repl.go:1095-1205): reset at line 1098, CLI success path at lines 1168-1173, CLI error path falls through to line 1198 without updating context fields. Direct-API success path at line 1200 explicitly guards the copy with
err == nil. - Existing protections: The
inputStatusclosure readslastContextTokensonly when constructing the status for the next input prompt, which only happens after a successful turn. The stale zero is not currently observable. - Proposed mitigation: Add an explicit
lastContextTokens = 0(or leave the existing reset as-is) with a comment noting that zero means 'no usage reported' on both success-without-stats and failure paths. - Alternative mitigations considered: A sentinel value like -1 would distinguish 'no report' from 'zero tokens,' but would require changes to the TUI rendering. The current zero convention is simpler and sufficient if documented.
- Severity calibration: Score 3 because the inconsistency is latent — no current code path reads the stale value — but the contract is implicit and fragile.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/repl/repl.go
Line: 1100
Comment:
**CLI-backend error path leaves `lastContextTokens` and `ag.LastContextTokens` stale**
The REPL's main turn loop resets `lastContextTokens` and `ag.LastContextTokens` to zero at line 1098 before starting any backend. For CLI backends (cc/codex), the success path updates both fields from `TurnStatsProvider` at lines 1168-1173. However, if `cliAgent.Run` returns an error (line 1165), the code falls through to line 1198 where `lastTurnDur` is recorded, but neither `lastContextTokens` nor `ag.LastContextTokens` is updated — they remain zero from the pre-turn reset. This is consistent with the convention that zero means "no usage reported this turn," but the success path for the direct-API backend (line 1200) explicitly copies `ag.LastContextTokens` into `lastContextTokens` only when `err == nil`. The CLI-backend error path does not mirror this guard, making the contract implicit. If a future change reads `lastContextTokens` after a failed CLI turn, it would see zero and could misinterpret it as "the model used no context" rather than "the turn failed."
Example:
input: CLI backend (cc) turn that fails mid-request
actual: lastContextTokens = 0, ag.LastContextTokens = 0 (from pre-turn reset)
expected: same values, but the contract is implicit — a reader might expect a sentinel or explicit guard
Threat model:
A future UI change that displays `lastContextTokens` after a failed turn would show zero, misleading the user into thinking the model context was empty when the turn actually failed before reporting usage.
Specific code citations:
REPL turn loop (repl.go:1095-1205): reset at line 1098, CLI success path at lines 1168-1173, CLI error path falls through to line 1198 without updating context fields. Direct-API success path at line 1200 explicitly guards the copy with `err == nil`.
Existing protections:
The `inputStatus` closure reads `lastContextTokens` only when constructing the status for the next input prompt, which only happens after a successful turn. The stale zero is not currently observable.
Proposed mitigation:
Add an explicit `lastContextTokens = 0` (or leave the existing reset as-is) with a comment noting that zero means 'no usage reported' on both success-without-stats and failure paths.
Alternative mitigations considered:
A sentinel value like -1 would distinguish 'no report' from 'zero tokens,' but would require changes to the TUI rendering. The current zero convention is simpler and sufficient if documented.
Severity calibration:
Score 3 because the inconsistency is latent — no current code path reads the stale value — but the contract is implicit and fragile.
Suggested fix shape:
// After the CLI error path, before lastTurnDur:
// lastContextTokens intentionally remains 0 — this turn produced no usage.
Why this wasn't caught:
No test exercises a failed CLI-backend turn and asserts the context-token fields remain zero.
How can I resolve this? If you propose a fix, please make it concise.
| prompt string | ||
| history []string | ||
| histIdx int | ||
| status *StatusInfo // optional metrics/mode/task display; nil hides it |
There was a problem hiding this comment.
Session timer side-channel leaks session duration via UI status tick
The input model's Init() starts a tea.Tick every second when status.SessionStarted is set, updating the UI with live session duration. This creates a side-channel that leaks the exact session start time to anyone who can observe the UI (e.g., via screen recording or remote desktop). While low-severity, it reveals session longevity, which could be used in targeted attacks.
Example:
An attacker watches a screen share and sees 'session 2m10s' update to 'session 2m11s', deducing the session started exactly 2 minutes ago.
Suggested fix:
// In renderStatus, round duration to nearest minute:
sessionDur := time.Since(s.SessionStarted).Round(time.Minute)
if sessionDur > 0 {
metrics = append(metrics, "session "+compactDuration(sessionDur))
}More Info
- Threat model: An attacker with visual access to the terminal (e.g., via screen sharing, shoulder surfing, or compromised remote desktop) can infer the session start time and duration, potentially correlating it with other events. This is a low-risk information leak.
- Specific code citations: Line 97:
if m.status == nil || m.status.SessionStarted.IsZero() { return nil }; line 100:return tea.Tick(time.Second, func(t time.Time) tea.Msg { return statusTickMsg(t) }). The tick updates the UI viarenderStatus()which includessession 2m10s. - Existing protections: None; the tick is unconditional when
SessionStartedis non-zero. - Proposed mitigation: Round the displayed duration to the nearest minute or suppress the tick after a certain threshold (e.g., stop updating after 10 minutes). Alternatively, only show session duration when explicitly requested (e.g., via a
/sessioncommand). - Alternative mitigations considered: Remove the live tick entirely and compute duration on-demand; however, the UI would then show stale timing.
- Severity calibration: Score 3 because it's a hardening gap that widens the attack surface slightly, but exploitation requires visual access and the impact is minimal.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/input.go
Line: 100
Comment:
**Session timer side-channel leaks session duration via UI status tick**
The input model's `Init()` starts a `tea.Tick` every second when `status.SessionStarted` is set, updating the UI with live session duration. This creates a side-channel that leaks the exact session start time to anyone who can observe the UI (e.g., via screen recording or remote desktop). While low-severity, it reveals session longevity, which could be used in targeted attacks.
Example:
An attacker watches a screen share and sees 'session 2m10s' update to 'session 2m11s', deducing the session started exactly 2 minutes ago.
Threat model:
An attacker with visual access to the terminal (e.g., via screen sharing, shoulder surfing, or compromised remote desktop) can infer the session start time and duration, potentially correlating it with other events. This is a low-risk information leak.
Specific code citations:
Line 97: `if m.status == nil || m.status.SessionStarted.IsZero() { return nil }`; line 100: `return tea.Tick(time.Second, func(t time.Time) tea.Msg { return statusTickMsg(t) })`. The tick updates the UI via `renderStatus()` which includes `session 2m10s`.
Existing protections:
None; the tick is unconditional when `SessionStarted` is non-zero.
Proposed mitigation:
Round the displayed duration to the nearest minute or suppress the tick after a certain threshold (e.g., stop updating after 10 minutes). Alternatively, only show session duration when explicitly requested (e.g., via a `/session` command).
Alternative mitigations considered:
Remove the live tick entirely and compute duration on-demand; however, the UI would then show stale timing.
Severity calibration:
Score 3 because it's a hardening gap that widens the attack surface slightly, but exploitation requires visual access and the impact is minimal.
Suggested fix shape:
// In renderStatus, round duration to nearest minute:
sessionDur := time.Since(s.SessionStarted).Round(time.Minute)
if sessionDur > 0 {
metrics = append(metrics, "session "+compactDuration(sessionDur))
}
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| // compactDuration formats a duration as "42s", "2m10s", or "1h03m". | ||
| func compactDuration(d time.Duration) string { | ||
| d = d.Round(time.Second) |
There was a problem hiding this comment.
compactDuration can produce a negative duration string
Low-confidence finding — expand to read
compactDuration uses d.Round(time.Second) and then formats the result. If d is negative (e.g., because SessionStarted is in the future due to clock skew), d.Round returns a negative duration, and the switch cases will match the negative value, producing strings like -42s or -2m10s. The callers guard with > 0, so this is unlikely in practice, but the function itself has no negative-duration handling.
More Info
- Threat model: If a caller ever passes a negative duration (e.g., due to clock skew or a bug), the status panel displays a confusing negative time.
- Specific code citations:
internal/tui/input.golines 575-585:compactDurationhas noif d < 0guard. - Existing protections: Callers guard with
> 0before calling, so negative values are not currently passed. - Proposed mitigation: Add
if d < 0 { return '0s' }at the top ofcompactDuration. - Alternative mitigations considered: Could also use
max(d, 0)before formatting. - Severity calibration: Score 2 — defensive hardening; the current callers already guard against negative input.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/input.go
Line: 636
Comment:
**compactDuration can produce a negative duration string**
`compactDuration` uses `d.Round(time.Second)` and then formats the result. If `d` is negative (e.g., because `SessionStarted` is in the future due to clock skew), `d.Round` returns a negative duration, and the switch cases will match the negative value, producing strings like `-42s` or `-2m10s`. The callers guard with `> 0`, so this is unlikely in practice, but the function itself has no negative-duration handling.
Threat model:
If a caller ever passes a negative duration (e.g., due to clock skew or a bug), the status panel displays a confusing negative time.
Specific code citations:
`internal/tui/input.go` lines 575-585: `compactDuration` has no `if d < 0` guard.
Existing protections:
Callers guard with `> 0` before calling, so negative values are not currently passed.
Proposed mitigation:
Add `if d < 0 { return '0s' }` at the top of `compactDuration`.
Alternative mitigations considered:
Could also use `max(d, 0)` before formatting.
Severity calibration:
Score 2 — defensive hardening; the current callers already guard against negative input.
How can I resolve this? If you propose a fix, please make it concise.
| // ClearHistory resets conversation history. | ||
| func (a *Agent) ClearHistory() { | ||
| a.History = []api.Message{} | ||
| a.LastContextTokens = 0 |
There was a problem hiding this comment.
ClearHistory resets LastContextTokens but Run and runStreamingLoop also reset it — triple reset is redundant but not harmful
Low-confidence finding — expand to read
ClearHistory (agent.go:130) sets LastContextTokens = 0. Both Run (line 149) and runStreamingLoop (line 338) also reset the field to zero at the start of each turn. The REPL's /clear command (repl.go line 261) calls ClearHistory and also sets lastContextTokens = 0. This triple reset is defensive and not a bug, but it creates ambiguity about which site owns the reset contract. If a future change removes one reset without updating the others, the field could carry a stale value from a previous turn.
More Info
- Threat model: A future refactor removes the reset in
RunorrunStreamingLoopassumingClearHistoryhandles it, butClearHistoryis only called on/clear, not between normal turns. The field would then carry a stale value across turns. - Specific code citations:
ClearHistory(agent.go:130),Run(agent.go:149),runStreamingLoop(agent.go:338), REPL/clearhandler (repl.go:261). - Existing protections: The redundancy itself is the protection — all three sites reset the field. The risk is that a maintainer sees the redundancy and removes one site.
- Proposed mitigation: Document which site is the canonical reset point (e.g.,
runStreamingLoopandRunare the turn-boundary resets;ClearHistoryis the full-session reset) so future maintainers understand the contract. - Alternative mitigations considered: A single reset in a shared helper called by all three sites would eliminate the redundancy but adds indirection.
- Severity calibration: Score 2 because the redundancy is currently harmless and the risk is purely about future maintainability.
Example:
input: Normal turn sequence without /clear
actual: LastContextTokens reset three times (harmless redundancy)
expected: Single canonical reset point
Suggested fix:
// In ClearHistory doc comment: // Resets the full conversation state. Per-turn context resets are handled by Run/runStreamingLoop.Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/agent.go
Line: 133
Comment:
**`ClearHistory` resets `LastContextTokens` but `Run` and `runStreamingLoop` also reset it — triple reset is redundant but not harmful**
`ClearHistory` (agent.go:130) sets `LastContextTokens = 0`. Both `Run` (line 149) and `runStreamingLoop` (line 338) also reset the field to zero at the start of each turn. The REPL's `/clear` command (repl.go line 261) calls `ClearHistory` and also sets `lastContextTokens = 0`. This triple reset is defensive and not a bug, but it creates ambiguity about which site owns the reset contract. If a future change removes one reset without updating the others, the field could carry a stale value from a previous turn.
Example:
input: Normal turn sequence without /clear
actual: LastContextTokens reset three times (harmless redundancy)
expected: Single canonical reset point
Threat model:
A future refactor removes the reset in `Run` or `runStreamingLoop` assuming `ClearHistory` handles it, but `ClearHistory` is only called on `/clear`, not between normal turns. The field would then carry a stale value across turns.
Specific code citations:
`ClearHistory` (agent.go:130), `Run` (agent.go:149), `runStreamingLoop` (agent.go:338), REPL `/clear` handler (repl.go:261).
Existing protections:
The redundancy itself is the protection — all three sites reset the field. The risk is that a maintainer sees the redundancy and removes one site.
Proposed mitigation:
Document which site is the canonical reset point (e.g., `runStreamingLoop` and `Run` are the turn-boundary resets; `ClearHistory` is the full-session reset) so future maintainers understand the contract.
Alternative mitigations considered:
A single reset in a shared helper called by all three sites would eliminate the redundancy but adds indirection.
Severity calibration:
Score 2 because the redundancy is currently harmless and the risk is purely about future maintainability.
Suggested fix shape:
// In ClearHistory doc comment: // Resets the full conversation state. Per-turn context resets are handled by Run/runStreamingLoop.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Author
|
Thanks, Sigilix — addressed the actionable context-metric feedback in
The direct-API context metric is already updated after successful turns in the existing Validated with |
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
Why
The prompt input visually blended into agent output, and session context/timing information was either absent or misleading. The status panel now uses the latest request's input usage for context-window estimation and refreshes the session timer while the prompt is open.
Validation
go test ./...git diff --check