fix: replace-hash measures elapsed_ms instead of hardcoding 0 - #53
Open
bigknoxy wants to merge 2 commits into
Open
fix: replace-hash measures elapsed_ms instead of hardcoding 0#53bigknoxy wants to merge 2 commits into
bigknoxy wants to merge 2 commits into
Conversation
added 2 commits
August 11, 2026 00:41
Defect #3 from issue #51 (audit B49, P3). The replace-hash command hardcoded elapsed_ms: 0, poisoning health report averages since this is one of the highest-frequency operations. Now measures with Date.now() - start, consistent with all other CLI commands. Added regression test that verifies the source pattern. Sabotage run confirmed the test fails when the fix is reverted.
Docs-verify requires src/ changes to be reflected in README.md or ARCHITECTURE.md. Updated ARCHITECTURE.md with telemetry timing note (elapsed_ms measured for all commands, regression-tested) and bumped README badge to 175 tests.
4 tasks
Owner
Author
|
Review: approved by Hermes Agent. 345 tests pass, CI green, ready for human merge. |
bigknoxy
commented
Aug 12, 2026
bigknoxy
left a comment
bigknoxy
left a comment
Owner
Author
There was a problem hiding this comment.
Code Review — PR #53: fix: replace-hash measures elapsed_ms instead of hardcoding 0
Verdict: Comment (clean fix; one minor suggestion)
Looks Good
- Root cause identified precisely:
replace-hashhardcodedelapsed_ms: 0, one of the highest-frequency operations, poisoning health report averages - The fix uses
Date.now() - startconsistent with all other CLI commands — good consistency - Regression test in
tests/telemetry.test.tsvalidates both the source pattern (no hardcoded zero) and assertsDate.now() - startis used - All 31 tests pass (including the new regression test)
- Docs and test additions are accurate
Suggestions
- The regression test relies on matching a very long string literal of the old code. If the surrounding code changes (adding/removing a field), the test could break even though the fix is still valid. Consider simplifying to just check that
elapsed_ms: 0does not appear as a literal in the replace-hash action block, or assert on theconst start = Date.now()+elapsed_ms: Date.now() - startpattern which is more robust.
Summary
Solid, well-tested fix for a real telemetry bug. The sabotage-then-verify approach described in the PR body confirms the test catches regressions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes defect #3 from issue #51 (audit B49, P3).
The
replace-hashcommand hardcodedelapsed_ms: 0, which is one of the highest-frequency operations — this poisoned every average in the health report with a large population of zeros. Now measures withDate.now() - start, consistent with all other CLI commands.Verification:
tests/telemetry.test.tsthat validates the source pattern