Skip to content

fix: pin rendered numbers to one locale so graft can read its own savings footer (#338) - #345

Open
Ercaner1988 wants to merge 3 commits into
trailhq:mainfrom
Ercaner1988:locale-safe-savings
Open

fix: pin rendered numbers to one locale so graft can read its own savings footer (#338)#345
Ercaner1988 wants to merge 3 commits into
trailhq:mainfrom
Ercaner1988:locale-safe-savings

Conversation

@Ercaner1988

@Ercaner1988 Ercaner1988 commented Sep 10, 2026

Copy link
Copy Markdown

Fixes #338.

savingsLine() wrote the footer with the machine's locale; sumSavingsFooters() read it back with ([\d,]+). On a machine that groups thousands with anything but a comma, graft parsed its own footer as the first group — 1.990 came back as 1, and every savings total downstream went with it.

The change

One formatter, pinned, next to the reader that has to cope with its output:

export function groupDigits(n: number): string {
  return n.toLocaleString('en-US');
}

Five call sites now go through it. Three of them write the [graft] tokens saved ≈ N line that sumSavingsFooters parses — context/savings.ts, ask/ask.ts, claude/format.ts, each with its own wording of the same footer — and the other two are the displays that sit beside it, the statusline's ~N tok saved and graft stats.

I pinned the two display sites as well, and that is the one judgement call in here worth flagging. They are never parsed, so locale-aware would be defensible — but graft's output is English throughout, a statusline reading 1.990 directly above a footer reading 1,990 would be its own small bug, and src/cli-epilogue.ts:50 already pins the same way (nodes.toLocaleString("en-US")), which is where I took the precedent from. I left that line alone rather than reach across from an init banner into the savings module; say the word if you would rather have it routed through the same helper, or would rather keep the two displays locale-aware and pin only the parsed line.

The reader is untouched. Widening it instead was the option I would not take: 1.990 is a valid en-US decimal, so a reader that strips separators is guessing, and it would have to know about U+202F and U+00A0 too.

Tests

test/savings-locale.test.ts. A process cannot change its own ambient locale — Node reads it from the environment on posix and from the OS on Windows — so the test moves the default instead: toLocaleString() with no argument answers as a .-grouping locale would, while calls that name a locale explicitly are left alone. That is exactly the difference the fix turns on, so the test fails without it and passes with it on an en-US runner too, which is the property CI was missing. It asserts the stand-in actually changes an unqualified call first, so the rest cannot pass for the wrong reason.

The third test renders the footer through formatAsk(). ask/ask.ts has its own wording of the parsed line, and no test rendered it at all — your blast-radius bot flagged it ("no test reaches Ask Token Savings"), and it was right: reverting just that file to toLocaleString() brought #338 back for every graft ask with the rest of the suite still green. Now it fails.

All three fail on upstream/main. The first two:

✖ #338: the savings footer round-trips on a machine that is not en-US
  actual: '[graft] tokens saved ≈ 1.990 (100%) — this output ≈ 10 tok …'
  expected: /tokens saved ≈ 1,990/
✖ #338: what graft displays groups the same way as what it parses
  actual: '… tokens saved:  ~12.345'

Verified

The eight tests that fail on upstream/main on this machine — the ones I mentioned in #337 — are the eight this fixes. npm test on a tr-TR box:

tests pass fail
upstream/main 1220 1207 8
this branch 1223 1218 0

(5 skipped either way.)

Seven of the eight were assertions that assumed ,, and they now hold because the output does. The eighth, tool-savings counts a REAL savings line, was the round trip itself failing 1 !== 1990 — it is the test that should have caught this and could not, because the footer it feeds is rendered in the runner's own locale. It passes here now, which is the end-to-end proof: a real savingsLine() through the real hook, on a machine that groups with ., recording 1990.

One test needed the same treatment as the source. test/savings.test.ts:52 built its expected value with (…).toLocaleString(), so once the footer was pinned it asserted 1.990 against a footer saying 1,990 — the mirror image of the bug. It now goes through groupDigits too.

Re-measured against de8456e (0.18.0) after rebasing onto it: same 8, same numbers.

No CHANGELOG entry

I dropped the one I had. 0.18.0 is already released, so there is no unreleased section to add to, and de8456e shows the release commit is what writes them. Happy to add a bullet wherever you want it.

Environment: Node v24.16.0, Windows 11 x64, OS locale tr-TR.

🤖 Generated with Claude Code

@trailhq-graft

trailhq-graft Bot commented Sep 10, 2026

Copy link
Copy Markdown

🌱 graft blast radius

3 areas changed → 5 areas can be affected. 10 dependent symbols, depth 2.
Tests: no test reaches Ask Savings Display; 2 areas updated their tests.
Tag: @shhdwi — 7 of 8 areas · @anirudhkumar-nanonets — 8 of 8 areas · @afeddersen — Token Savings Calculation

flowchart TB
  A0(("Workspace Federation<br/>4 symbols"))
  A1(("MCP Tool Invocation<br/>2 symbols"))
  A2(("Claude UI Integration<br/>2 symbols"))
  A3(("CLI Entry Point<br/>1 symbol"))
  A4(("Grep Search Formatting<br/>1 symbol"))
  classDef reached fill:#D9EDF3,stroke:#3AA7C9,stroke-width:1.5px,color:#0E313C;
  class A0,A1,A2,A3,A4 reached;
Loading
Can be affected Symbols Nearest hop Reached from
Workspace Federation 4 src/graph/map.ts:L325-L350 formatRepoMap — calls, depth 2 Ask Savings Display, Token Savings Calculation
MCP Tool Invocation 2 src/mcp/tools.ts:L247-L329 callSingleTool — calls, depth 2 Ask Savings Display, Token Savings Calculation
Claude UI Integration 2 src/claude/statusline.ts:L24-L35 main — calls, depth 1 Session Metrics Formatting, Token Savings Calculation
CLI Entry Point 1 src/cli.ts:L1-L1407 cli.ts — calls, depth 1 Ask Savings Display, Session Metrics Formatting +1
Grep Search Formatting 1 src/search/grep-cli.ts:L54-L63 formatGrepResult — calls, depth 2 Token Savings Calculation
Who knows this code — 3 people across 8 areas
Area Who knows it
Token Savings Calculation · changed @anirudhkumar-nanonets — 4 commits, last 10d ago · @afeddersen — 1 commit, last 12d ago
Ask Savings Display · changed @shhdwi — 16 commits, last 1mo ago · @anirudhkumar-nanonets — 11 commits, last 2d ago
Session Metrics Formatting · changed @shhdwi — 10 commits, last 2mo ago · @anirudhkumar-nanonets — 6 commits, last 10d ago
Workspace Federation · affected @shhdwi — 14 commits, last 30d ago · @anirudhkumar-nanonets — 8 commits, last 19d ago
MCP Tool Invocation · affected @shhdwi — 14 commits, last 30d ago · @anirudhkumar-nanonets — 7 commits, last 10d ago
Claude UI Integration · affected @shhdwi — 13 commits, last 30d ago · @anirudhkumar-nanonets — 9 commits, last 10d ago
CLI Entry Point · affected @anirudhkumar-nanonets — 39 commits, last 2d ago · @shhdwi — 23 commits, last 30d ago
Grep Search Formatting · affected @anirudhkumar-nanonets — 4 commits, last 1mo ago · @shhdwi — 3 commits, last 2mo ago

Ownership is git history over each area's own files, weighted towards recent work (120-day half-life). Merge commits and bots are dropped, and you are dropped from your own PR. A name with no @ has no GitHub handle in its commit email — tag them by hand, or add a .mailmap entry. A suggestion from history, not a CODEOWNERS rule.

All 10 dependent symbols, grouped by area

Workspace Federation — 4 symbols in 4 files

  • src/graph/map.ts:L325-L350 — formatRepoMap (calls, depth 2)
  • src/graph/traverse-cli.ts:L166-L242 — runCallersCommand (calls, depth 2)
    194: // `all` (aka full/max) = the whole transitive closure: walk until no new
  • src/graph/workspace-cli.ts:L88-L97 — runWorkspaceAsk (calls, depth 2)
  • src/graph/workspace.ts:L659-L695 — federateCallers (calls, depth 2)

MCP Tool Invocation — 2 symbols in 1 file

  • src/mcp/tools.ts:L247-L329 — callSingleTool (calls, depth 2)
  • src/mcp/tools.ts:L153-L199 — callWorkspaceTool (calls, depth 2)

Claude UI Integration — 2 symbols in 2 files

  • src/claude/statusline.ts:L24-L35 — main (calls, depth 1)
    34: process.stdout.write(renderStatusline(stats, session, { ctxPct }).join('\n'));
  • src/claude/hooks.ts:L394-L462 — main (calls, depth 2)
    421: if (event === 'tool-savings') { handleToolUse(input, dir); return; }

CLI Entry Point — 1 symbol in 1 file

  • src/cli.ts:L1-L1407 — cli.ts (calls, depth 1)
    48: import { latestSession, formatSessionStats, sessionInputRate } from "./claude/session-metrics.js";

Grep Search Formatting — 1 symbol in 1 file

  • src/search/grep-cli.ts:L54-L63 — formatGrepResult (calls, depth 2)
Test signal per changed area — 2 ✓ · 1 ✗

Reached = a node under a test path has a resolved edge into the changed symbol. It undercounts anything called indirectly — through a CLI, a spawned process or a dynamic import — so read a low ratio as “look here”, never as a coverage gate.

  • Token Savings Calculation — 2 of 2 reached · 2 test files changed here: test/savings-locale.test.ts, test/savings.test.ts
  • Ask Savings Display — 0 of 1 reached · no test file reaches it
    • not reached: askSavingsLine
  • Session Metrics Formatting — 3 of 3 reached · 1 test file changed here: test/savings-locale.test.ts
8 test suites also reference this code

8 symbols, kept out of the diagram and the table so they cannot crowd out the areas a reviewer has to look at.

  • test/ask-fusion.test.ts
  • test/ask.test.ts
  • test/claude-format.test.ts
  • test/claude-hooks.test.ts
  • test/claude-statusline.test.ts
  • test/regression-single-repo.test.ts
  • test/session-metrics.test.ts
  • test/workspace.test.ts

graft blast · origin/main...HEAD · depth 2 · 7 changed files

Open the interactive graph → — click an area to see its dependent symbols at file:line.

github-actions Bot added a commit that referenced this pull request Sep 10, 2026
…s own footer

savingsLine() wrote the `[graft] tokens saved ≈ N` footer with the machine's
locale; sumSavingsFooters() read it back with `([\d,]+)`. The two are fifteen
lines apart, under a comment saying they are kept together so they cannot
drift — and they had not drifted from each other, only from Intl.

On a machine that groups thousands with anything but a comma, graft parsed its
own footer as the first group: de-DE and tr-TR write `1.990` and get 1, fr-FR
and ru-RU write `1 990` (narrow no-break space) and get 1. Everything
downstream of savedTokens carried that number, not the real one — the
statusline's tokens saved and its dollar value, `graft stats`, and the
`saved_tokens_bucket` on session_summary telemetry, all short by ~1000x for
every user outside a comma locale.

One pinned formatter, used by all five sites that render one of these numbers:
the three that write the parsed footer (context/savings.ts, ask/ask.ts,
claude/format.ts) and the two displays beside it. The displays are never
parsed, but a statusline reading `1.990` above a footer reading `1,990` would
be its own small bug, and cli-epilogue.ts already pins the same way.

The reader is untouched: widening it would mean guessing, since `1.990` is a
valid en-US decimal.

The test moves the *default* locale rather than the ambient one — which a
process cannot set for itself — so it fails without this fix on an en-US
runner too. That is the property CI was missing: its runners always had the
comma, so the round trip could never break there.

Fixes trailhq#338

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…parsed line

groupDigits() went into three places that write the `[graft] tokens saved ≈ N`
line sumSavingsFooters() parses, but the tests only rendered two of them. The
third, askSavingsLine() in ask/ask.ts, has its own wording of the footer and
no test reached it at all — so reverting it to toLocaleString() would have
brought trailhq#338 back for every `graft ask` while the rest of the suite stayed
green, which is the kind of drift between writers trailhq#338 was.

Renders it through formatAsk() under the same moved default locale as the
other two tests. Fails with ask.ts reverted; passes as it is.

Refs trailhq#338

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions Bot added a commit that referenced this pull request Sep 11, 2026
Add a regression test that walks the TypeScript AST for every file under `src/` and fails if any native `tree-sitter-*` grammar is imported statically. This protects the lazy-loading behavior that prevents missing native prebuilds from crashing the whole CLI, and includes fixtures to prove the scanner catches banned imports without flagging `tree-sitter` itself or lazy `require()` usage.

Co-Authored-By: Copilot <198982749+Copilot@users.noreply.github.com>
Co-Authored-By: sonnet5 <24634891+sonnet5@users.noreply.github.com>
Co-Authored-By: opus5 <103333277+opus5@users.noreply.github.com>
github-actions Bot added a commit that referenced this pull request Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Savings are under-counted ~1000x outside en-US: the footer is written with toLocaleString() and read back with a comma-only regex

1 participant