Skip to content

Commit 3d40d57

Browse files
ework-agentranxianglei
authored andcommitted
test: address dual-review findings for #371
- add nudge breakdown line test asserting rendered reasoning category - fix 2 pre-existing vacuous acp_status tests (partial mocks dropped by filterMessages → passed with zero visible messages) - tighten protected-reasoning assertion to exact value - system prompt breakdown example percentages now sum to 100% 1086/1086 tests passing
1 parent de7c051 commit 3d40d57

5 files changed

Lines changed: 59 additions & 7 deletions

File tree

‎devlog/2026-09-08_reasoning-in-context-estimate/WORKLOG.md‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,16 +26,32 @@
2626

2727
### Tests
2828
- `tests/inject-utils-pure.test.ts`: +4 — reasoning counted in `reasoningTokens`/`total`; total formula includes reasoning; mixed message (msgTotal vs messageTokens + largestRanges footprint); no-reasoning regression guard.
29-
- `tests/protection-aware-stats.test.ts`: +1 — reasoning on a protected message counted in `protectedTokens`.
29+
- `tests/protection-aware-stats.test.ts`: +1 — reasoning on a protected message counted in `protectedTokens` (exact `reasoningTokens === 200`).
3030
- `tests/acp-status.test.ts`: +3 — overview reasoning category (100 text/33% + 200 reasoning/67% of 300); reasoning-only message visible (overview 100% + drilldown line); drilldown per-message footprint includes reasoning.
31+
- `tests/inject.test.ts`: +1 — rendered nudge breakdown line shows `2.0K reasoning (Q%)` (added per test review).
32+
- `tests/acp-status.test.ts` (pre-existing fixes): 2 vacuous tests (partial mocks dropped by `filterMessages`) given complete mocks + real listing-line assertions (per both reviews).
33+
34+
## Dual-agent review (AGENTS.md §5.3 + §5.6)
35+
36+
Both reviewers: **APPROVE**. Findings addressed in this branch:
37+
38+
- **Test reviewer finding (minor)**: rendered nudge breakdown line was untested → added `tests/inject.test.ts` "E2E: nudge breakdown line shows reasoning category with token count (#371)" (asserts `2.0K reasoning (\d+%)` in the injected nudge).
39+
- **Both reviewers (minor)**: pre-existing vacuous tests `tests/acp-status.test.ts:286-305` / `:307-328` used partial mocks (missing `info.sessionID`/`info.time.created`) that `filterMessages` drops → tests passed with ZERO visible messages. Fixed: complete mocks + assertions on the actual `m00001 (...) text|bash` listing lines.
40+
- **Test reviewer (nit)**: tightened `comp.reasoningTokens >= 200` → `assert.equal(comp.reasoningTokens, 200)`.
41+
- **Code reviewer (nit)**: system prompt example percentages now sum to 100% (were 121% pre-existing, 131% after first edit).
42+
43+
Not addressed (documented, non-blocking):
44+
- Composition-vs-range divergence (code reviewer minor): `buildCompressibleRanges` range tokens still exclude reasoning (intentional — ranges = compressible amounts; the pipeline's min-size check `countMessageCharacters` also excludes reasoning, so adding it there risks phantom "Range too small" rejections per #37). Consequence: "Effective compressible: ~X" (nudge) and overview totals now include reasoning while per-range lines don't. Documented in PR description; candidate follow-up issue (source-tagged).
45+
- Reasoning-only messages render with `text` label in the drilldown (`toolName || "text"`); `classifyMessageType` would say `reasoning` — label-semantics change, out of scope.
46+
- Per-message `dcp-message-id` token annotation (`countMessageCharacters`, token-utils.ts) still excludes reasoning — pre-existing, out of scope, candidate follow-up.
3147

3248
## Verification
3349

3450
- `npm run typecheck` — clean.
35-
- `npm run test` — **1085/1085 pass** (was 1077 on master; +8 new).
51+
- `npm run test` — **1086/1086 pass** (was 1077 on master; +9 new).
3652
- `npm run build` — clean.
3753
- `npm run format:check` — repo-wide pre-existing Prettier drift (423 files fail on clean master, incl. all 7 touched files); CI does not run format checks; no reformat to keep the diff minimal.
38-
- Test-input fidelity note: `filterMessages` (`lib/messages/shape.ts:14-24`) drops messages lacking `info.sessionID`/`info.time.created` — the new acp_status tests use complete mocks. (Pre-existing tests at `tests/acp-status.test.ts:286/:307` use partial mocks and only assert section headers, so they pass with zero visible messages — flagged for the review, not fixed here.)
54+
- Test-input fidelity note: `filterMessages` (`lib/messages/shape.ts:14-24`) drops messages lacking `info.sessionID`/`info.time.created` — all acp_status tests (new + 2 pre-existing fixed per review) use complete mocks.
3955

4056
## Open items / known interactions
4157

‎lib/prompts/system.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ CONTEXT BREAKDOWN
7373
7474
When context usage passes a threshold, the system appends a breakdown showing where your context tokens are spent:
7575
76-
Breakdown: 5.2K system (21%) | 12.3K tool (40%) | 3.1K summaries (10%) | 8.5K code (28%) | 6.5K text (22%) | 2.4K reasoning (10%)
76+
Breakdown: 4.2K system (21%) | 8.0K tool (40%) | 2.0K summaries (10%) | 2.6K code (13%) | 2.2K text (11%) | 1.0K reasoning (5%)
7777
7878
- "system" = system prompt tokens (AGENTS.md, tool definitions — not compressible)
7979
- "tool" = tool call outputs (largest category — compress first when consumed)

‎tests/acp-status.test.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,10 @@ test("acp_status: scope=uncompressed defaults to ranges view", async () => {
285285

286286
test("acp_status: scope=uncompressed view=messages shows per-message listing", async () => {
287287
const mockMsgs = [
288-
{ info: { id: "raw-1", role: "assistant" }, parts: [{ type: "text", text: "hello world" }] },
288+
{
289+
info: { id: "raw-1", role: "assistant", sessionID: SID, time: { created: 1 } },
290+
parts: [{ type: "text", text: "hello world" }],
291+
},
289292
]
290293
const mockClient = makeMockClient(mockMsgs)
291294
const state = makeState([], new Map())
@@ -302,12 +305,13 @@ test("acp_status: scope=uncompressed view=messages shows per-message listing", a
302305

303306
assert.match(result, /UNCOMPRESSED/)
304307
assert.match(result, /Sorted by/)
308+
assert.match(result, /m00001 \(\d+\) text/, "per-message listing must include the visible message")
305309
})
306310

307311
test("acp_status: scope=uncompressed view=messages with tool filter shows filter in header", async () => {
308312
const mockMsgs = [
309313
{
310-
info: { id: "raw-1", role: "assistant" },
314+
info: { id: "raw-1", role: "assistant", sessionID: SID, time: { created: 1 } },
311315
parts: [{ type: "tool", tool: "bash", state: { input: { command: "ls" } } }],
312316
},
313317
]
@@ -325,6 +329,7 @@ test("acp_status: scope=uncompressed view=messages with tool filter shows filter
325329
const result = await statusTool.execute({ scope: "uncompressed", view: "messages", tool: "bash" } as any, { sessionID: SID } as any)
326330

327331
assert.match(result, /UNCOMPRESSED — bash:/)
332+
assert.match(result, /m00001 \(\d+\) bash/, "filtered listing must include the bash message")
328333
})
329334

330335
test("acp_status: invalid scope falls back to overview", async () => {

‎tests/inject.test.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -646,6 +646,37 @@ test("E2E: nudge recommendation content includes composition breakdown and compr
646646
)
647647
})
648648

649+
test("E2E: nudge breakdown line shows reasoning category with token count (#371)", () => {
650+
const state = createSessionState()
651+
state.modelContextLimit = 1_000_000
652+
state.nudges.lastPerMessageNudgeTokens = 200_000
653+
const config = buildConfig()
654+
config.compress.maxContextLimit = 800_000
655+
config.compress.minContextLimit = 200_000
656+
657+
const messages: WithParts[] = [
658+
userMsg("u1", "hello"),
659+
{
660+
info: {
661+
id: "a1", role: "assistant", sessionID: SID, agent: "a", time: { created: 2 },
662+
tokens: { input: 200_000, output: 55_000 },
663+
} as WithParts["info"],
664+
parts: [
665+
{ id: "a1-r", messageID: "a1", sessionID: SID, type: "reasoning" as const, text: "z".repeat(8_000) },
666+
textPart("a1", "done"),
667+
],
668+
},
669+
]
670+
injectCompressNudges(state, config, logger, messages, {} as any)
671+
672+
assert.equal(state.nudges.shouldInjectThisTurn, true, "should nudge (55K growth >= 50K threshold)")
673+
674+
const injected = suffixText(messages)
675+
assert.ok(injected.includes("Breakdown:"), "nudge must include composition breakdown")
676+
// 8_000 chars of reasoning / 4 = 2_000 tokens
677+
assert.match(injected, /2\.0K reasoning \(\d+%\)/, "breakdown must show reasoning category with its token count")
678+
})
679+
649680
test("growth floor: nudge suppressed when growth below floor (issue #27 anti-thrashing)", () => {
650681
// 1M model: growthFloor = max(5000, 0.45×50000) = 22500
651682
// Growth of 5K < 22500 → no nudge output at all

‎tests/protection-aware-stats.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ test("estimateContextComposition: reasoning on protected message counted in prot
220220
setupRefs(state, messages)
221221

222222
const comp = estimateContextComposition(messages, state, ["skill"])
223-
assert.ok(comp.reasoningTokens >= 200, "reasoning tokens counted")
223+
assert.equal(comp.reasoningTokens, 200, "reasoning tokens counted exactly (800 chars / 4)")
224224
assert.ok(
225225
comp.protectedTokens >= comp.reasoningTokens,
226226
"protected tokens include the protected message's reasoning",

0 commit comments

Comments
 (0)