feat(compare): add offline result comparison - #183
Conversation
|
Thanks for the proposal and implementation. I understand that this command provides domain-aware comparison, such as primary-case handling, case transition classification, and CI exit codes. However, if the scope is only comparing two local I think the feature would be more compelling if it formed part of a complete baseline workflow—for example:
Could you clarify the concrete user workflow this is intended to unlock, and what advantage the built-in command provides over a documented |
JHWang-1997
left a comment
There was a problem hiding this comment.
The compare command is well structured, but the token gate mixes primary-case metrics with full benchmark-run totals. This can make CI report the opposite of the tested skill's actual token trend. Please see the inline example and suggested fix.
| primary := input.PrimaryCaseResults() | ||
| metrics := RunMetrics{ | ||
| CaseCount: len(primary), | ||
| TotalTokens: input.TotalTokens, |
There was a problem hiding this comment.
Use primary-case tokens for the token growth gate
This field uses the report-level TotalTokens, while InputTokens and OutputTokens below are calculated from PrimaryCaseResults(). In benchmark mode, report-level TotalTokens includes both with_skill and without_skill executions, so changes in the baseline can mask a token regression in the tested skill.
For example:
| Run | with_skill tokens |
without_skill tokens |
report TotalTokens |
|---|---|---|---|
| Old | 100 | 100 | 200 |
| New | 120 | 50 | 170 |
The tested skill's primary token usage increased by 20%, but --max-token-increase-percent 10 sees 200 -> 170 and incorrectly passes because the full benchmark total decreased by 15%. The text output is also internally inconsistent: primary input/output tokens increase while total tokens decreases.
Since this command otherwise applies primary-case semantics to run metrics and transitions, please derive the comparable total from the primary results as well, for example after accumulation:
metrics.TotalTokens = metrics.InputTokens + metrics.OutputTokensIf full-run benchmark cost is also useful, it should be exposed under a separate, clearly named metric and should not drive the primary token regression gate.
Summary
skill-up compare <old-result.json> <new-result.json>for text and JSON result diffs.Test Plan
GOCACHE=/private/tmp/skill-up-go-build-cache GOTOOLCHAIN=go1.26.4 make verifyGOCACHE=/private/tmp/skill-up-go-build-cache GOTOOLCHAIN=go1.26.4 make testCloses #175