refactor(gateway): explicit boolean comparison in webhook timestamp check#700
Open
marcusrbrown wants to merge 1 commit into
Open
refactor(gateway): explicit boolean comparison in webhook timestamp check#700marcusrbrown wants to merge 1 commit into
marcusrbrown wants to merge 1 commit into
Conversation
Align checkTimestamp with the project's strict-boolean convention (`Number.isFinite(parsedMs) === false` rather than negation). No behavior change.
fro-bot
approved these changes
May 30, 2026
Owner
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
This is a one-line, behavior-preserving refactor: !Number.isFinite(parsedMs) → Number.isFinite(parsedMs) === false in checkTimestamp (packages/gateway/src/http/hmac.ts:76).
I verified:
- Behavioral equivalence.
Number.isFinitealways returns a realboolean, andparsedMsis typednumber(fromDate.parse).=== falseand!are identical here with no coercion subtlety. TheNaN/ unparseable branch returns the same{ok: false, reason: 'timestamp_expired'}. - Convention claim holds. The strict-boolean style is pervasive in this package (90+
=== true/=== falsematches), including the structurally identicalNumber.isFinite(httpPort) === falseatconfig.ts:327. The change genuinely aligns with house style rather than introducing a one-off. - Test coverage exists.
hmac.test.tsalready exercises the touched branch —unparseable timestampdescribe block (garbage string and empty string both asserttimestamp_expired), plus the in-window / boundary / expired cases. No new behavior to test. - Commit scope.
refactorscope correctly avoids triggering a release, matching the PR description's intent.
Blocking issues
None
Non-blocking concerns
None — the explicit form is marginally more verbose but is the established package convention, so it's the right call here.
Missing tests
None — no behavior changed; the affected branch is already covered.
Risk assessment (LOW): No behavior change to a pure, well-tested function. No public API surface change (signature and return shape identical). Blast radius is a single conditional inside one function; regression likelihood is effectively nil and there is no new security exposure on the auth path.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 26676988757 |
| Cache | hit |
| Session | ses_1886847a7ffepR5mGe63aA68fE |
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
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.
Aligns
checkTimestampin the gateway HMAC module with the project's strict-boolean convention —Number.isFinite(parsedMs) === falseinstead of!Number.isFinite(parsedMs). No behavior change; the two forms are equivalent.refactorscope so it doesn't trigger a release on its own.