-
Notifications
You must be signed in to change notification settings - Fork 0
Implementation Plan: Split Token Summary Table Into 4 API Token Fields #615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
58b0dae
af8cf48
30bd313
bab9f68
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,9 +12,10 @@ | |
|
|
||
| _TOKEN_COLUMNS = ( | ||
| TerminalColumn("STEP", max_width=40, align="<"), | ||
| TerminalColumn("INPUT", max_width=10, align=">"), | ||
| TerminalColumn("UNCACHED", max_width=10, align=">"), | ||
| TerminalColumn("OUTPUT", max_width=10, align=">"), | ||
| TerminalColumn("CACHED", max_width=10, align=">"), | ||
| TerminalColumn("CACHE_RD", max_width=10, align=">"), | ||
| TerminalColumn("CACHE_WR", max_width=10, align=">"), | ||
| TerminalColumn("COUNT", max_width=7, align=">"), | ||
| TerminalColumn("TIME", max_width=8, align=">"), | ||
| ) | ||
|
|
@@ -64,28 +65,29 @@ def format_token_table(steps: list[dict], total: dict) -> str: | |
| lines = [ | ||
| "## Token Usage Summary", | ||
| "", | ||
| "| Step | input | output | cached | count | time |", | ||
| "|------|-------|--------|--------|-------|------|", | ||
| "| Step | uncached | output | cache_read | cache_write | count | time |", | ||
| "|------|----------|--------|------------|-------------|-------|------|", | ||
| ] | ||
| for step in steps: | ||
| name = step.get("step_name", "?") | ||
| inp = h(step.get("input_tokens", 0)) | ||
| out = h(step.get("output_tokens", 0)) | ||
| cached = h( | ||
| step.get("cache_read_input_tokens", 0) + step.get("cache_creation_input_tokens", 0) | ||
| ) | ||
| cache_rd = h(step.get("cache_read_input_tokens", 0)) | ||
| cache_wr = h(step.get("cache_creation_input_tokens", 0)) | ||
| count = step.get("invocation_count", 1) | ||
| wc = step.get("wall_clock_seconds", step.get("elapsed_seconds", 0.0)) | ||
| lines.append(f"| {name} | {inp} | {out} | {cached} | {count} | {fmt_dur(wc)} |") | ||
| lines.append( | ||
| f"| {name} | {inp} | {out} | {cache_rd} | {cache_wr} | {count} | {fmt_dur(wc)} |" | ||
| ) | ||
|
|
||
| total_in = h(total.get("input_tokens", 0)) | ||
| total_out = h(total.get("output_tokens", 0)) | ||
| total_cached = h( | ||
| total.get("cache_read_input_tokens", 0) + total.get("cache_creation_input_tokens", 0) | ||
| ) | ||
| total_cached_rd = h(total.get("cache_read_input_tokens", 0)) | ||
Trecek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| total_cached_wr = h(total.get("cache_creation_input_tokens", 0)) | ||
| total_time = total.get("total_elapsed_seconds", 0.0) | ||
| lines.append( | ||
| f"| **Total** | {total_in} | {total_out} | {total_cached} | | {fmt_dur(total_time)} |" | ||
| f"| **Total** | {total_in} | {total_out} | {total_cached_rd}" | ||
| f" | {total_cached_wr} | | {fmt_dur(total_time)} |" | ||
| ) | ||
| return "\n".join(lines) | ||
|
|
||
|
|
@@ -116,17 +118,15 @@ def format_token_table_terminal(steps: list[dict], total: dict) -> str: | |
| h = TelemetryFormatter._humanize | ||
| fmt_dur = TelemetryFormatter._fmt_duration | ||
|
|
||
| rows: list[tuple[str, str, str, str, str, str]] = [] | ||
| rows: list[tuple[str, str, str, str, str, str, str]] = [] | ||
| for step in steps: | ||
| rows.append( | ||
| ( | ||
| step.get("step_name", "?"), | ||
| h(step.get("input_tokens", 0)), | ||
| h(step.get("output_tokens", 0)), | ||
| h( | ||
| step.get("cache_read_input_tokens", 0) | ||
| + step.get("cache_creation_input_tokens", 0) | ||
| ), | ||
| h(step.get("cache_read_input_tokens", 0)), | ||
| h(step.get("cache_creation_input_tokens", 0)), | ||
| str(step.get("invocation_count", 1)), | ||
| fmt_dur(step.get("wall_clock_seconds", step.get("elapsed_seconds", 0.0))), | ||
| ) | ||
|
|
@@ -136,10 +136,8 @@ def format_token_table_terminal(steps: list[dict], total: dict) -> str: | |
| "Total", | ||
| h(total.get("input_tokens", 0)), | ||
| h(total.get("output_tokens", 0)), | ||
| h( | ||
| total.get("cache_read_input_tokens", 0) | ||
| + total.get("cache_creation_input_tokens", 0) | ||
| ), | ||
| h(total.get("cache_read_input_tokens", 0)), | ||
| h(total.get("cache_creation_input_tokens", 0)), | ||
| "", | ||
| fmt_dur(total.get("total_elapsed_seconds", 0.0)), | ||
| ) | ||
|
|
@@ -177,19 +175,18 @@ def format_compact_kv( | |
| count = step.get("invocation_count", 1) | ||
| inp = h(step.get("input_tokens", 0)) | ||
| out = h(step.get("output_tokens", 0)) | ||
| cached = h( | ||
| step.get("cache_read_input_tokens", 0) + step.get("cache_creation_input_tokens", 0) | ||
| ) | ||
| cache_rd = h(step.get("cache_read_input_tokens", 0)) | ||
| cache_wr = h(step.get("cache_creation_input_tokens", 0)) | ||
| wc = step.get("wall_clock_seconds", step.get("elapsed_seconds", 0.0)) | ||
| lines.append(f"{name} x{count} [in:{inp} out:{out} cached:{cached} t:{wc:.1f}s]") | ||
| lines.append( | ||
| f"{name} x{count} [uc:{inp} out:{out} cr:{cache_rd} cw:{cache_wr} t:{wc:.1f}s]" | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [warning] cohesion: Asymmetric abbreviation: per-step compact KV uses short labels
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Investigated — this is intentional. The per-step compact format [uc:X out:X cr:X cw:X t:Xs] is space-constrained inline; totals are standalone KV pairs where full names (total_uncached, total_cache_read, total_cache_write) improve readability. This same pattern appears consistently in both telemetry_fmt.py::format_compact_kv and pretty_output.py::_fmt_get_token_summary, and is explicitly encoded as expected behavior in tests/pipeline/test_telemetry_formatter.py:239-246 (asserts both 'uc:' and 'total_cache_read:'). |
||
| ) | ||
| if total: | ||
| lines.append("") | ||
| lines.append(f"total_in: {h(total.get('input_tokens', 0))}") | ||
| lines.append(f"total_uncached: {h(total.get('input_tokens', 0))}") | ||
| lines.append(f"total_out: {h(total.get('output_tokens', 0))}") | ||
| cache_tokens = total.get("cache_read_input_tokens", 0) + total.get( | ||
| "cache_creation_input_tokens", 0 | ||
| ) | ||
| lines.append(f"total_cached: {h(cache_tokens)}") | ||
| lines.append(f"total_cache_read: {h(total.get('cache_read_input_tokens', 0))}") | ||
| lines.append(f"total_cache_write: {h(total.get('cache_creation_input_tokens', 0))}") | ||
| if mcp_responses: | ||
| mcp_total = mcp_responses.get("total", {}) | ||
| if mcp_total: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -376,13 +376,14 @@ def test_format_get_token_summary_compact(): | |
| out, _ = _run_hook(event=event) | ||
| text = json.loads(out)["hookSpecificOutput"]["updatedMCPToolOutput"] | ||
| assert "token_summary" in text | ||
| # Assert specific compact line format: name xN [in:Xk out:Xk cached:XM t:Xs] | ||
| assert "investigate x1 [in:45.2k out:12.8k cached:1.2M t:0.0s]" in text | ||
| assert "make_plan x2 [in:30.0k out:8.0k cached:500.0k t:0.0s]" in text | ||
| assert "implement x1 [in:60.0k out:15.0k cached:2.0M t:0.0s]" in text | ||
| assert "total_in:" in text | ||
| # Assert specific compact line format: name xN [uc:Xk out:Xk cr:XM cw:XM t:Xs] | ||
| assert "investigate x1 [uc:45.2k out:12.8k cr:1.2M cw:0 t:0.0s]" in text | ||
| assert "make_plan x2 [uc:30.0k out:8.0k cr:0 cw:500.0k t:0.0s]" in text | ||
| assert "implement x1 [uc:60.0k out:15.0k cr:2.0M cw:0 t:0.0s]" in text | ||
| assert "total_uncached:" in text | ||
| assert "total_out:" in text | ||
| assert "total_cached:" in text | ||
| assert "total_cache_read:" in text | ||
| assert "total_cache_write:" in text | ||
|
|
||
|
|
||
| # PHK-16 | ||
|
|
@@ -1270,6 +1271,33 @@ def test_hook_token_summary_output_equivalent_to_canonical(): | |
| ) | ||
|
|
||
|
|
||
| def test_fmt_run_skill_interactive_shows_four_token_fields(): | ||
Trecek marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| """_fmt_run_skill interactive mode shows all 4 token fields.""" | ||
| data = { | ||
| "success": True, | ||
| "subtype": "COMPLETED", | ||
| "exit_code": 0, | ||
| "needs_retry": False, | ||
| "result": "done", | ||
| "token_usage": { | ||
| "input_tokens": 5000, | ||
| "output_tokens": 3000, | ||
| "cache_read_input_tokens": 200000, | ||
| "cache_creation_input_tokens": 8000, | ||
| }, | ||
| } | ||
| rendered = _format_response( | ||
| "mcp__plugin_autoskillit_autoskillit__run_skill", | ||
| json.dumps({"result": json.dumps(data)}), | ||
| pipeline=False, | ||
| ) | ||
| assert rendered is not None | ||
| assert "tokens_uncached:" in rendered | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [info] tests: Assertions use substring matching ( |
||
| assert "tokens_out:" in rendered | ||
| assert "tokens_cache_read:" in rendered | ||
| assert "tokens_cache_write:" in rendered | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Timing summary dedicated formatter | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[info] cohesion:
_fmt_run_skilluses full labelstokens_uncached,tokens_cache_read,tokens_cache_writewhile_fmt_get_token_summaryuses abbreviateduc:,cr:,cw:for the same token categories in the same file. Creates inconsistent user experience.