Skip to content

Fix cache hit percentage calculation - #11

Closed
dylanneve1 wants to merge 1 commit into
mainfrom
fix-cache-hit-percentage
Closed

Fix cache hit percentage calculation#11
dylanneve1 wants to merge 1 commit into
mainfrom
fix-cache-hit-percentage

Conversation

@dylanneve1

@dylanneve1 dylanneve1 commented Apr 5, 2026

Copy link
Copy Markdown
Owner

Summary

  • Cache hit % was calculated as cache_read / (input + cache_read + cache_write), which incorrectly included cache writes in the denominator β€” diluting the metric
  • Now calculates as cache_read / (input + cache_read), which accurately reflects how much of the readable prompt was served from cache
  • Fixed in both the agent response logging (claude-sdk/index.ts) and the /status command display (commands.ts)

Test plan

  • Run /status and verify cache hit % looks reasonable
  • Check agent logs show corrected cache percentages

πŸ€– Generated with Claude Code

Cache hit % was dividing cache_read by (input + cache_read + cache_write),
which diluted the metric. Now divides by (input + cache_read) to accurately
reflect how much of the readable prompt was served from cache.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates cache hit percentage reporting to avoid diluting the metric by excluding cache-write tokens from the denominator.

Changes:

  • Recomputes cache hit % as cache_read / (input + cache_read) in Telegram status output.
  • Recomputes cache hit % as cache_read / (input + cache_read) in backend agent logging.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/frontend/telegram/commands.ts Adjusts cache hit % calculation used in the /status display.
src/backend/claude-sdk/index.ts Adjusts cache hit % calculation used in agent response logs.

πŸ’‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/frontend/telegram/commands.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


πŸ’‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

dylanneve1 added a commit that referenced this pull request Apr 10, 2026
- Validate mcpServer.args elements are strings and reject empty command (#1)
- Shell-quote interpolated paths in dream bash commands (#2, #11)
- Replace CLI-based diary write with mempalace_diary_write MCP tool (#3)
- Make validation error message platform-agnostic (#4)
- Update mempalacePython comment for platform-dependent default (#5)
- Wrap mp.init() in Promise.race with 30s timeout (#6)
- Make init conditional on successful validation, pass actual config (#7)
- Move import mempalace check into validateConfig (#8)
- Replace execFileSync with async execFile in init() (#9)
- Document that registerPlugin does NOT call init (#10)
- Update dream prompt header from "4-stage" to "5-stage" (#12)
- Update getPluginMcpServers JSDoc to document mcpServer path (#13, #17)
- Add .min(1) to palacePath/pythonPath zod schemas (#14)
- Distinguish ENOENT/EACCES/EPERM from import failures in validation (#15)
- Fix test name from "logs warning" to match actual behavior (#16)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dylanneve1 added a commit that referenced this pull request Apr 10, 2026
* feat: integrate mempalace as built-in plugin for long-term memory

Adds mempalace (Python MCP server) as a first-class memory system.
When enabled, the agent gets semantic search, knowledge graph, and
verbatim memory storage via ChromaDB β€” all local, zero API calls.

Key changes:
- Extend plugin system with `mcpServer` field for non-Node MCP servers
- Add `registerPlugin()` for built-in plugin registration
- Create mempalace plugin (factory pattern, validates python venv)
- Wire mempalace into dream mode (Stage 5: mine logs into palace)
- Add `mempalace` config schema (enabled, palacePath, pythonPath)
- Add default paths for palace dir and python venv binary

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor(dream): mine daily notes instead of raw logs, add diary writing

Dream Stage 5 now mines memory/daily/ (curated observations) instead of
raw logs/ directory, eliminating junk chunks (tool JSON, df output, etc).
Added personal diary writing instruction β€” agent reflects on feelings,
state of mind, learnings, and loose threads after each dream run.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address PR #27 review comments

- plugin.ts: validate mcpServer.args entries are strings and reject empty command
- dream.ts: quote interpolated paths in shell commands, replace mcp_server CLI diary with direct file write
- mempalace/index.ts: platform-agnostic error message for missing python binary
- paths.ts: update comment to reflect platform-dependent venv path
- bootstrap.ts: wrap mempalace init in 30s timeout to match loadSinglePlugin behavior

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: restore mempalace CLI diary writer, keep path quoting

Copilot suggested removing the mcp_server CLI invocation for diary
writing but that's the intended mempalace interface. Restored it
with quoted paths.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* test: add gating/validation tests for mempalace integration

- plugin.ts: test rejection of empty mcpServer.command and non-string args elements
- dream.ts: test mempalace section gating β€” verify mining/diary instructions
  only appear when mempalace is configured, skip message when not
- 1306 tests passing (4 new)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* feat: upgrade mempalace system prompt with comprehensive tool docs

Adapted from mempalace SKILL.md (v3.1.0). Key improvements:
- Session protocol (verify before responding, invalidate stale facts)
- Full tool documentation including kg_timeline, traverse, find_tunnels,
  diary_read/write, delete_drawer, graph_stats, check_duplicate
- Semantic search tips (meaning-based, not keyword)
- Knowledge graph temporal validity guidance
- Tests updated to verify all tool names appear in prompt

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor: extract mempalace prompt to prompts/mempalace.md

Move system prompt instructions out of TypeScript into a .md file,
matching the pattern used by dream.md and other prompts. Plugin loads
and interpolates {{palacePath}} at runtime with graceful fallback.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: replace pixi.intel.com registry URLs in lockfile with npmjs.org

Lockfile had resolved URLs pointing to pixi.intel.com (private/corporate
registry) for @Anthropic-AI packages, causing CI to fail with ENOTFOUND.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: resolve all lint warnings and formatting issues

Remove unused imports, variables, and catch bindings across 14 files.
Add yield statements to generator function mocks. Fix prettier formatting.

0 lint warnings, 0 format issues, 1307 tests passing.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address all 17 Copilot review comments on PR #27

- Validate mcpServer.args elements are strings and reject empty command (#1)
- Shell-quote interpolated paths in dream bash commands (#2, #11)
- Replace CLI-based diary write with mempalace_diary_write MCP tool (#3)
- Make validation error message platform-agnostic (#4)
- Update mempalacePython comment for platform-dependent default (#5)
- Wrap mp.init() in Promise.race with 30s timeout (#6)
- Make init conditional on successful validation, pass actual config (#7)
- Move import mempalace check into validateConfig (#8)
- Replace execFileSync with async execFile in init() (#9)
- Document that registerPlugin does NOT call init (#10)
- Update dream prompt header from "4-stage" to "5-stage" (#12)
- Update getPluginMcpServers JSDoc to document mcpServer path (#13, #17)
- Add .min(1) to palacePath/pythonPath zod schemas (#14)
- Distinguish ENOENT/EACCES/EPERM from import failures in validation (#15)
- Fix test name from "logs warning" to match actual behavior (#16)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: add 'mempalace' to LogComponent type

TypeScript type check was failing because 'mempalace' wasn't in the
LogComponent union type used by log/logError/logWarn functions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: allow MCP tools in dream prompt when required by Stage 5

Update tool access statement to permit MCP tools for mempalace
mining stage instead of blanket-blocking all MCP tools.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: duplicate guard in registerPlugin, pass MCP servers to dream

- registerPlugin now checks for duplicates before setting env vars
  or logging success, preventing misleading logs and env clobbering
- Dream agent now receives mempalace MCP servers when configured,
  so Stage 5 diary/mining tools actually work

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* feat: add selective MCP server loading via 'only' filter

getPluginMcpServers now accepts an optional plugin name filter:
- omitted = all plugins (backwards compatible for chat sessions)
- [] = none
- ["mempalace"] = only mempalace

Dream mode uses ["mempalace"] to load only what it needs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: load mempalace prompt from dirs.prompts, fix dream systemPrompt

- Mempalace prompt now loads from ~/.talon/prompts/mempalace.md
  (user-customisable, seeded on first run) instead of relative to
  source file. Consistent with heartbeat/dream prompt loading.
- Dream systemPrompt now permits MemPalace MCP tools when configured,
  preventing conflict with the markdown prompt's Stage 5 instructions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: move duplicate check before validation, gate dream on plugin registration

- registerPlugin checks for duplicates before running validateConfig,
  avoiding expensive re-validation on accidental double registration
- Dream mempalace integration now gated on getPlugin("mempalace")
  instead of just config.mempalace.enabled, so failed validation
  or registration doesn't cause dream-time MCP tool failures

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(mempalace): correct CLI arg order for status check

The --palace flag is a global option that must come before the
subcommand. Wrong order caused the init health check to always
fail with exit 2, logging a misleading "not yet initialized" warning.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address Copilot review round 8 β€” validation, error handling, unused param

- Validate `import mempalace.mcp_server` (actual spawned module) instead of
  just `import mempalace` in validateConfig
- Add timeout/killed error branching in validateConfig catch block
  (ETIMEDOUT, signal, killed) with specific messages instead of generic
  "not installed"
- Include stderr details in import failure messages for debugging
- Remove unused `config` from ProcessAndReplyParams and all processAndReply
  call sites (flushQueue, retry, callback handler)
- Replace `mempalace status` CLI smoke test in init() with a simple import
  check β€” fixes false "Palace not yet initialized" warning when palace IS
  initialized but CLI subcommand doesn't exist

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: remove config from message queue chain, fix pythonPath comment

- Remove config from queue entry type, enqueueMessage signature, and
  all 3 call sites β€” completes the cleanup started in round 8
- Eliminates unnecessary TalonConfig reference (including botToken) from
  queue state
- Update mempalace plugin header comment to document platform-dependent
  pythonPath default (bin/python on Unix, Scripts/python.exe on Windows)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

@claudiusthebot claudiusthebot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review from heartbeat #44 (2026-05-03)

The fix here is correct and still needed β€” both locations still have the old denominator on main. But the branch is stale and needs updating.

What changed since April 5: The SDK refactor in v1.9.0 (PR merged after this was filed) split src/backend/claude-sdk/index.ts into 8 focused modules. The cache hit % calculation that was in the old index.ts is now in src/backend/claude-sdk/handler.ts (~line 217):

// Current main (handler.ts) β€” still wrong:
const totalPrompt =
  state.sdkInputTokens + state.sdkCacheRead + state.sdkCacheWrite;
const cacheHitPct =
  totalPrompt > 0 ? Math.round((state.sdkCacheRead / totalPrompt) * 100) : 0;

The commands.ts fix is still directly applicable β€” that file hasn't changed structurally and the bug is at the same location.

To land this:

  1. Rebase fix-cache-hit-percentage onto current main
  2. Drop the claude-sdk/index.ts hunk (file no longer exists β€” it's now a barrel re-export only)
  3. Apply the same fix to src/backend/claude-sdk/handler.ts instead
  4. Keep the commands.ts fix as-is

The fix logic is identical in both places: rename totalPrompt β†’ cacheTotal, drop + cacheWrite from the denominator. Six lines total changed.

Confirmed cacheWrite should not be in the denominator: it represents tokens being written to cache on this call β€” they're not "readable input served from cache." Including them deflates the metric any time the cache is being warmed for the first time.

APPROVE β€” once rebased.

@claudiusthebot

Copy link
Copy Markdown
Collaborator

Filed the rebased fix as #107 on a fresh branch. Same fix, applied at the post-SDK-refactor file locations (handler.ts instead of the old claude-sdk/index.ts; commands.ts slightly different lines now). 1635/1635 tests, tsc clean, lint 0 errors.

Suggest closing this one in favour of #107 β€” left this PR alone rather than force-resetting your branch.

@dylanneve1 dylanneve1 closed this May 7, 2026
dylanneve1 pushed a commit that referenced this pull request May 7, 2026
Cache hit % was calculated as cache_read / (input + cache_read +
cache_write), which dilutes the metric every time the cache is being
warmed for the first time β€” cache_write tokens are tokens being
*written to* cache on this call, they're not "readable input served
from cache."

Now: cache_read / (input + cache_read).

Same fix as PR #11, applied at the post-SDK-refactor file locations:
- src/backend/claude-sdk/handler.ts (was claude-sdk/index.ts on #11)
- src/frontend/telegram/commands.ts (slightly different lines now,
  reads displayCacheRead/Write from getSessionSnapshot enrichment)

Six-line change. Supersedes #11.

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
dylanneve1 pushed a commit that referenced this pull request May 9, 2026
Closes two high-severity Dependabot alerts:
- #7: path traversal via percent-encoded dot segments (fast-uri ≀3.1.1)
- #11: host confusion via percent-encoded authority components (fast-uri ≀3.1.1)

Fix: add `"fast-uri": "^3.1.2"` to package.json overrides (same pattern as
#120 ip-address and other prior lockfile security fixes). Surgical lockfile
patch β€” only the version + integrity hash for node_modules/fast-uri changed.

Validates with bare `npm ci` (no --include=optional or other flags).

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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.

3 participants