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.
Follow-up: deferred Suggestions from PR #12119 (
/contextcategory accounting)PR #12119 (
fix(cli): make /context categories add up to the provider total, issue #12033) has been through five-plus review rounds. PerAGENTS.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
packages/cli/src/ui/commands/contextCommand.ts:688: the estimated branch's tier andfreeSpacenow readrawContent, butmessagesTokensstays0there and all four renderers gate the Messages row onhasTokenCount, so the quantity that drives the displayed tier is never displayed.contextCommand.ts:493: the recovery loop seedsrowedNamesfrom all oflistSkills()while the rows are finally filtered byenabledSkillNames, so a listing entry whose skill is disabled sits inside the Skills figure but has no detail row.contextCommand.ts:184:isSkillListingTextis 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.contextCommand.ts:497: the rows this loop pushes omitloaded, soskillsmixesundefinedwithfalse; all three detail comparators open witha.loaded !== b.loaded, which istrueforundefinedvsfalse, so two not-loaded rows compare as1in both directions.packages/cli/src/ui/opentui/item-projection.test.ts:227: the new fixture is arithmetically impossible — its category rows sum to7100againsttotalTokens5000, andfreeSpace94000does not follow either. It violates the invariant this PR documents inui/types.ts.packages/cli/src/ui/commands/contextCommand.test.ts:331: themakeChatConfigregistry double declares a state production does not produce (a Skill tool ingetAllTools()whilegetFunctionDeclarations()returns[]), which is what drivesdisplayBuiltinToolsinto its clamp rather than into the Skill-definition subtraction.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 emitstotalTokens0with a nonzerostartupContext;Startup contextis the one new row that is not total-gated. No fixture pins that real shape.Test coverage
contextCommand.ts:450: the new!activeChatbranch is entered by existing tests but its measurement never runs, so deleting the whole block leaves the suite green.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.contextCommand.ts:465: the case-normalised listing lookup has no test, so either.toLowerCase()can be dropped unnoticed.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'stypeof output !== 'string'guard;measureTailSkillListings'sif (measured.byName.size === 0) continue;fail-closed guard; theisLoaded &&gate in front of loaded-body billing; the nested loop's|| inner.fileDataand nested-text arms; and the matcher's prefix arm (body carrying a suffix).Presentation
packages/cli/src/ui/components/views/ContextUsage.tsx:396: the newCached prefixrow's German and Japanese translations are wider thanCategoryRow'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-3entrance 1 (path.dirname) are fixed.R1-3entrances 2–5 (exec-nested restore, scheduler truncation with the persisted sentinel, duplicate body after/restore, andbody + '\n' + suffixskipping 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.