Skip to content

Follow-up: deferred Suggestions from #12119 (/context category accounting) #12235

Description

@yiliang114

Follow-up: deferred Suggestions from PR #12119 (/context category accounting)

PR #12119 (fix(cli): make /context categories add up to the provider total, issue #12033) has been through five-plus review rounds. Per AGENTS.md ("Once a PR has been through roughly 5 review rounds, land only Critical fixes … defer remaining Suggestions to a follow-up issue or PR"), the round-2 Critical findings were fixed in the branch and the sixteen Suggestions below are deferred here, one entry per review thread. Each is recorded in the PR thread so nothing is silently dropped.

Correctness-adjacent

  • R1-5 — packages/cli/src/ui/commands/contextCommand.ts:688: the estimated branch's tier and freeSpace now read rawContent, but messagesTokens stays 0 there and all four renderers gate the Messages row on hasTokenCount, so the quantity that drives the displayed tier is never displayed.
  • R1-6 — contextCommand.ts:493: the recovery loop seeds rowedNames from all of listSkills() while the rows are finally filtered by enabledSkillNames, so a listing entry whose skill is disabled sits inside the Skills figure but has no detail row.
  • R2-4 — contextCommand.ts:184: isSkillListingText is a bare substring test over text whose author may be the model, the user, or a remote MCP server, adopted at two call sites with asymmetric guards.
  • R2-5 — contextCommand.ts:497: the rows this loop pushes omit loaded, so skills mixes undefined with false; all three detail comparators open with a.loaded !== b.loaded, which is true for undefined vs false, so two not-loaded rows compare as 1 in both directions.
  • R2-7 — packages/cli/src/ui/opentui/item-projection.test.ts:227: the new fixture is arithmetically impossible — its category rows sum to 7100 against totalTokens 5000, and freeSpace 94000 does not follow either. It violates the invariant this PR documents in ui/types.ts.
  • R2-8 — packages/cli/src/ui/commands/contextCommand.test.ts:331: the makeChatConfig registry double declares a state production does not produce (a Skill tool in getAllTools() while getFunctionDeclarations() returns []), which is what drives displayBuiltinTools into its clamp rather than into the Skill-definition subtraction.
  • R2-9 — item-projection.test.ts:205: the comment labels the fixture as the pre-first-turn estimated view, but the producer's pre-first-turn path emits totalTokens 0 with a nonzero startupContext; Startup context is the one new row that is not total-gated. No fixture pins that real shape.

Test coverage

  • R1-7 — contextCommand.ts:450: the new !activeChat branch is entered by existing tests but its measurement never runs, so deleting the whole block leaves the suite green.
  • R1-13 — packages/web-shell/client/components/messages/ContextUsageMessage.tsx:482: the web shell gained the same three rows and no test covers them; the nearest existing assertion cannot catch their loss.
  • R2-6 — contextCommand.ts:465: the case-normalised listing lookup has no test, so either .toLowerCase() can be dropped unnoticed.
  • R2-10 … R2-14 — contextCommand.test.ts:272, :652, :289, :463, :552: one pattern filed at five locations — every guard and branch arm this PR adds to the skill-body and nested-media billing path survives deletion with the suite green. Respectively: isBilledSkillBody's typeof output !== 'string' guard; measureTailSkillListings's if (measured.byName.size === 0) continue; fail-closed guard; the isLoaded && gate in front of loaded-body billing; the nested loop's || inner.fileData and nested-text arms; and the matcher's prefix arm (body carrying a suffix).

Presentation

  • R1-9 — packages/cli/src/ui/components/views/ContextUsage.tsx:396: the new Cached prefix row's German and Japanese translations are wider than CategoryRow's fixed 24-column label box, so the legend row wraps to a second orphan line and the block renders misaligned in those two locales.

Not deferred (fixed in the branch)

R2-1 (top-level media arm), R2-2 + R1-2 (scale divisor), R2-3 (clamp deficit), R1-3 entrance 1 (path.dirname) are fixed. R1-3 entrances 2–5 (exec-nested restore, scheduler truncation with the persisted sentinel, duplicate body after /restore, and body + '\n' + suffix skipping the whole response) are a class-closing design change — derive the billed skill bodies from history and consume one occurrence per billed copy — and are deferred here as well.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions