diff --git a/src/ask/ask.ts b/src/ask/ask.ts index 655fe69f..939d99fd 100644 --- a/src/ask/ask.ts +++ b/src/ask/ask.ts @@ -17,7 +17,7 @@ import { existsSync, readdirSync, readFileSync } from "node:fs"; import { join, resolve } from "node:path"; import { matter } from "../util/front-matter.js"; import { contextDirFor } from "../context/node-file.js"; -import { withSavings, savingsFor, savingsTurnNudge, type Savings } from "../context/savings.js"; +import { withSavings, savingsFor, savingsTurnNudge, groupDigits, type Savings } from "../context/savings.js"; import { loadGraphCached, loadAskIndexCached } from "../graph/load.js"; import { assertPrefixIndexed, @@ -1553,9 +1553,9 @@ function askSavingsLine(r: AskResult, body: string): string { const saved = base - pack; const pct = Math.round((saved / base) * 100); return ( - `${tag()} tokens saved ≈ ${saved.toLocaleString()} (${pct}%) — this pack ≈ ` + - `${pack.toLocaleString()} tok vs reading the ${r.saved.files} source file(s) whole ≈ ` + - `${base.toLocaleString()} tok. Estimate (baseline = those files read in full).` + + `${tag()} tokens saved ≈ ${groupDigits(saved)} (${pct}%) — this pack ≈ ` + + `${groupDigits(pack)} tok vs reading the ${r.saved.files} source file(s) whole ≈ ` + + `${groupDigits(base)} tok. Estimate (baseline = those files read in full).` + savingsTurnNudge(saved) ); } diff --git a/src/claude/format.ts b/src/claude/format.ts index 211262d6..55254f6a 100644 --- a/src/claude/format.ts +++ b/src/claude/format.ts @@ -6,6 +6,7 @@ import type { GraphV1, EdgeV1 } from '../graph/types.js'; // question in both places, and one set of calibrated numbers beats two. import { HIGH_FLOOR, STRONG_FLOOR } from '../ask/fuse.js'; import { dollarsSaved, formatDollars } from '../context/price.js'; +import { groupDigits } from '../context/savings.js'; import { brand, cmd, tag } from '../brand.js'; import { formatNoteHits, learningCredit, learningSavingsLine, PR_SPEEDUP, sectionLead, type Note, type NoteHit } from '../notes/notes.js'; @@ -43,7 +44,7 @@ export function renderStatusline( // stands alone rather than carrying a rate nobody measured. const usd = dollarsSaved(saved, session?.inputCostMicros, session?.inputTokensBilled); const money = usd === null ? '' : ` · ~${formatDollars(usd)}`; - top.push(C.indigo(`~${saved.toLocaleString()} tok saved${money}`)); + top.push(C.indigo(`~${groupDigits(saved)} tok saved${money}`)); } const bottom: string[] = []; @@ -167,9 +168,9 @@ export function formatRetrieval(ask: AskJson, cap = 5): string | null { const base = tokensOf(ask.saved!.baselineChars); const pct = Math.round((saved / base) * 100); return ( - `${body}\n${tag()} tokens saved ≈ ${saved.toLocaleString()} (${pct}%); this pack ≈ ` + - `${tokensOf(body.length).toLocaleString()} tok vs reading the ${ask.saved!.files} file(s) whole ≈ ` + - `${base.toLocaleString()} tok (estimate).` + `${body}\n${tag()} tokens saved ≈ ${groupDigits(saved)} (${pct}%); this pack ≈ ` + + `${groupDigits(tokensOf(body.length))} tok vs reading the ${ask.saved!.files} file(s) whole ≈ ` + + `${groupDigits(base)} tok (estimate).` ); } diff --git a/src/claude/session-metrics.ts b/src/claude/session-metrics.ts index 3e970024..272f0861 100644 --- a/src/claude/session-metrics.ts +++ b/src/claude/session-metrics.ts @@ -31,7 +31,7 @@ */ import { statSync } from 'node:fs'; import { join } from 'node:path'; -import { sumSavingsFooters } from '../context/savings.js'; +import { groupDigits, sumSavingsFooters } from '../context/savings.js'; import { dollarsSaved, formatDollars } from '../context/price.js'; import { readSession, writeSession, sessionDir, listSessionIds, type SessionState } from './state.js'; import type { AgentHost } from '../telemetry/contract.js'; @@ -202,7 +202,7 @@ export function formatSessionStats(s: SessionSummary | null): string { ` graft reads: ${graft}`, ` source reads: ${source} (Read / Grep / Glob)`, ` mix: ${mix}`, - ` tokens saved: ~${saved.toLocaleString()}`, + ` tokens saved: ~${groupDigits(saved)}`, ]; // Omitted, not zeroed, when no turn has been billed yet: a host whose hooks // name no transcript can't know what a token costs here, and a made-up rate diff --git a/src/context/savings.ts b/src/context/savings.ts index 75c3daf9..d6ef86a4 100644 --- a/src/context/savings.ts +++ b/src/context/savings.ts @@ -107,6 +107,25 @@ export function savingsTurnNudge(savedTokens: number): string { ); } +/** + * Digits grouped for display, pinned to en-US. + * + * Not a style choice: the `[graft] tokens saved ≈ N` line below is read back by + * {@link sumSavingsFooters}, whose regex knows `,` and nothing else. Rendered + * with the machine's locale instead, a de-DE or tr-TR box wrote `1.990` and + * graft parsed its own footer as `1` — every savings total downstream, and the + * dollar value beside it, off by a factor of a thousand (#338). fr-FR and ru-RU + * group with a narrow no-break space and lost it the same way. + * + * Every number graft renders goes through here, not only the parsed one: a + * statusline that grouped differently from the footer right above it would be + * its own small bug. `src/cli-epilogue.ts` pins the same way, for the same + * reason. + */ +export function groupDigits(n: number): string { + return n.toLocaleString('en-US'); +} + /** The one-line savings estimate for a command's text output, so the agent gets * the number for free — no extra tool call. `body` is the exact rendered output * the agent reads (the pack). Returns "" when there's nothing honest to claim: @@ -120,9 +139,9 @@ export function savingsLine(body: string, saved: Savings | undefined): string { const delta = base - pack; const pct = Math.round((delta / base) * 100); return ( - `${tag()} tokens saved ≈ ${delta.toLocaleString()} (${pct}%) — this output ≈ ` + - `${pack.toLocaleString()} tok vs reading the ${saved.files} file(s) it covers whole ≈ ` + - `${base.toLocaleString()} tok (estimate).` + + `${tag()} tokens saved ≈ ${groupDigits(delta)} (${pct}%) — this output ≈ ` + + `${groupDigits(pack)} tok vs reading the ${saved.files} file(s) it covers whole ≈ ` + + `${groupDigits(base)} tok (estimate).` + savingsTurnNudge(delta) ); } diff --git a/test/savings-locale.test.ts b/test/savings-locale.test.ts new file mode 100644 index 00000000..0e944061 --- /dev/null +++ b/test/savings-locale.test.ts @@ -0,0 +1,67 @@ +/** + * #338: graft rendered its savings footer with the machine's locale and read it + * back with a regex that knows only `,`, so a `de-DE`/`tr-TR` box wrote `1.990` + * and parsed it as `1`. + * + * A process cannot change its own ambient locale — Node takes it from the + * environment on posix and from the OS on Windows — so these tests move the + * *default* instead: `toLocaleString()` with no argument answers the way a + * `.`-grouping locale would, while calls that name a locale are left alone. + * That is exactly the difference the fix turns on, and it behaves identically on + * an en-US CI runner, which is why CI never caught the original. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { savingsLine, sumSavingsFooters } from '../src/context/savings.js'; +import { formatSessionStats } from '../src/claude/session-metrics.js'; +import { formatAsk, type AskResult } from '../src/ask/ask.js'; + +/** Run `fn` on a machine whose default locale groups thousands with `locale`'s + * separator. Restores the real method even if `fn` throws. */ +function withDefaultLocale(locale: string, fn: () => T): T { + const real = Number.prototype.toLocaleString; + Number.prototype.toLocaleString = function ( + this: number, + locales?: string | string[], + options?: Intl.NumberFormatOptions, + ): string { + return real.call(this, locales ?? locale, options); + }; + try { + return fn(); + } finally { + Number.prototype.toLocaleString = real; + } +} + +test('#338: the savings footer round-trips on a machine that is not en-US', () => { + // Sanity: the stand-in really does change what an unqualified call renders, + // or the rest of this file would pass for the wrong reason. + assert.equal(withDefaultLocale('de-DE', () => (1990).toLocaleString()), '1.990'); + + const line = withDefaultLocale('de-DE', () => savingsLine('x'.repeat(40), { files: 2, baselineChars: 8000 })); + assert.match(line, /tokens saved ≈ 1,990/, 'grouped for the reader, not for the machine'); + assert.equal(sumSavingsFooters(line), 1990, 'and read back whole, not as its first group'); +}); + +test('#338: what graft displays groups the same way as what it parses', () => { + const out = withDefaultLocale('de-DE', () => + formatSessionStats({ id: 'abc', perAgentQuery: {}, graftReads: 8, sourceReads: 2, savedTokens: 12345 }), + ); + assert.match(out, /tokens saved:\s+~12,345/); +}); + +test('#338: `graft ask` writes a footer its own reader can parse, on a machine that is not en-US', () => { + // The third writer of the parsed line, and the one no other test renders: ask.ts + // has its own wording of the footer, so a revert there would bring the bug back + // for every `graft ask` while the other two writers stayed green. + const result: AskResult = { + query: 'probe', + mode: 'lexical', + hits: [{ kind: 'symbol', title: 'probe', pointer: 'src/probe.ts:L1-L2', snippet: '', score: 1 }], + saved: { files: 3, baselineChars: 8000 }, // ≈ 2000 tok against a pack of a few dozen + }; + const out = withDefaultLocale('de-DE', () => formatAsk(result)); + assert.match(out, /tokens saved ≈ \d{1,3},\d{3} /, 'grouped for the reader, not for the machine'); + assert.ok(sumSavingsFooters(out) >= 1000, `read back whole, not as its first group (got ${sumSavingsFooters(out)})`); +}); diff --git a/test/savings.test.ts b/test/savings.test.ts index 4a9596bf..4c862e57 100644 --- a/test/savings.test.ts +++ b/test/savings.test.ts @@ -4,7 +4,7 @@ */ import { test } from 'node:test'; import assert from 'node:assert/strict'; -import { savingsFor, savingsLine, withSavings, toTokens, setInputRate } from '../src/context/savings.js'; +import { savingsFor, savingsLine, withSavings, toTokens, setInputRate, groupDigits } from '../src/context/savings.js'; import { hasSavingsTally } from '../src/claude/tally.js'; import type { GraphV1, NodeV1 } from '../src/graph/types.js'; @@ -49,7 +49,10 @@ test('savingsLine: reports saved tokens and percent when the output is smaller', assert.match(footer, /tokens saved ≈ [\d,]+ \(\d+%\)/); assert.match(footer, /2 file\(s\)/); const base = toTokens(8000); - assert.ok(footer.includes((base - toTokens(body.length)).toLocaleString())); + // Through the same pinned formatter the footer uses: rendered with the test + // machine's locale instead, this asserted `1.990` against a footer that says + // `1,990` — the mirror image of #338, and just as invisible on an en-US runner. + assert.ok(footer.includes(groupDigits(base - toTokens(body.length)))); // The nudge rides along so the agent reports the turn total without SKILL.md. assert.match(footer, /end of your reply/i); assert.match(footer, /graft saved ~N tokens this turn/);