feat: add cache lineage metrics - #11
Conversation
zhumengzhu
left a comment
There was a problem hiding this comment.
Review — PR #11: feat: add cache lineage metrics
Thorough review of the lineage/metrics feature. Verified the changes locally on pr-11: bun test → 432 pass / 0 fail (matches the PR description). I also traced the agent field source against the OpenCode runtime to confirm the compaction filter actually works (details in the inline note on stats.ts).
Overall the feature is solid and well-tested. Findings below, roughly in priority order.
Blocking
None. The core mechanics are sound: lineage separation on providerID:modelID, compaction exclusion via agent: "compaction", per-message pricing, and the missing-rate cost fallback all check out against the runtime.
Should fix (before or shortly after merge)
1. recomputedCost / recomputeSessionCost is now dead code (src/use-cache-hit-metrics.ts:131)
The memo is computed and returned (line 284) but never consumed — the UI switched to sessionPricing(). recomputeSessionCost is referenced only by tests. Either delete the memo + the recomputeSessionCost pipeline, or keep recomputeSessionCost and drop the wrapper. As-is it's a production-dead price-accumulation path kept alive by tests.
2. metricMessageStatus is plumbed but never consumed (src/sidebar-host.tsx:351 → widget.tsx:57 → use-cache-hit-metrics.ts:269)
The "incomplete-history" signal (capped/unavailable) is threaded through three layers and never rendered or acted on. The PR summary advertises "an incomplete-history fallback" — but the user is never told the shown totals are truncated. Either render a small indicator (e.g. a * / muted "history truncated" note near the Hit row when status !== "complete") or drop the plumbing.
Nits (non-blocking)
3. Doc drift: skippedForHit semantics (docs/en/timeline.md:149, docs/zh-CN/timeline.md:157)
Docs still say skippedForHit = msg.summary === true, but records.ts now sets skippedForHit = !isInteractiveAssistantMessage(msg) — which includes compaction. Code is the intended behavior (compaction rows are diagnostic-only); the doc rule line needs the same update you gave the type comment.
4. Duplicated lineage-key logic (src/lineage-stats.ts:12 vs src/stats.ts:150)
lineageKey() re-implements messageLineageKey() — same ${providerID}:${modelID} / "unknown" shape, two sources of truth (UNKNOWN_LINEAGE_KEY const vs the "unknown" literal in stats.ts). Worth consolidating into one helper in stats.ts and importing it.
5. Convoluted state ternary (src/stats.ts:194)
!last ? "warming" : switched || !previous ? (switched ? "switch" : "warming") : "steady" — works, but the !previous branch is unreachable once last exists, and "warming" is overloaded for both the zero-call and first-call cases. A small helper or early-return would make the intent readable.
6. Minor doc formatting — the type block in both timeline docs gained stray 3-space indent on the new fields (docs/en/timeline.md:57 etc.).
Note on agent (already verified — no action needed)
For the record: msg.agent === "compaction" filtering is valid. The OpenCode runtime sets agent: "compaction" + summary: true on compaction assistant messages, and agent: <subagent> on subtask messages. The v1 SDK generated types just don't declare agent on AssistantMessage — it's present at runtime. No change required; if you want, a one-line comment on the plugin's AssistantMessage type noting the runtime field would help future readers.
Thanks for the thorough documentation updates (EN + ZH in lockstep) and the test coverage — 10 test files, all green.
|
Hey @zhumengzhu i implemented your suggestions. Thank you so much for your feedback. |
|
Thanks for the PR — the lineage work looks good and all tests pass. One thing before I merge: GitHub reports the branch can't be rebased due to conflicts (in Could you rebase onto the latest |
7e8922d to
aae87af
Compare
done! |
Post-#11 polish: - Drop redundant approx prefix on cost rows (formatCost already marks ~) - History-truncated hint only when data may actually be truncated: mirror full (100) or direct history capped (10k); empty main session shows '-' instead of '0.0% warming' - Main metrics prefer the DB-level session.get aggregate (complete, not capped by the 100-message mirror), keeping the active lineage as the model identifier; blendedMain falls back the same way - Remove dead recomputeSessionCost and related tests - Sync README/design docs (user-facing history hint, DB-aggregate main, mirror-cap semantics) - Bump version to 0.7.3
Summary
What:
Why:
Model and agent switches can attach mixed-session totals to the last model and can create misleading hit-rate trends. This change keeps each provider/model lineage separate and preserves OpenCode cost values when a message has no matching rate.
Related Work
No matching open issue was found for lineage metrics, model switches, compaction filtering, or the 100-message history limit.
Testing
bun test: 432 pass, 0 fail, 791 assertions.index.tsx.Deployment Notes
setCacheKeyconfiguration is unchanged.Review Focus
Checklist