feat: add computeTTL and sanitizeValue utilities#253
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 PR Reviewer
The two new utility functions are well-documented and follow existing conventions. There are minor correctness and robustness issues worth addressing before merging.
📝 3 suggestion(s) - Please review inline comments below.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
There was a problem hiding this comment.
🤖 PR Reviewer
The two new utility functions (computeTTL and sanitizeValue) are clean and well-documented, but all three previously raised issues remain unaddressed: multi-byte UTF-8 boundary splitting in sanitizeValue, negative TTL for past dates in computeTTL, and missing type guard in sanitizeValue.
🔄 3 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
|
/review |
chsrimanaswi
left a comment
chsrimanaswi
left a comment
There was a problem hiding this comment.
🤖 PR Reviewer
The two previously raised issues with real impact (negative TTL from past dates, and multi-byte UTF-8 splitting) are still present in the diff unchanged. The sanitizeValue type-check concern has a reasonable developer explanation and is not re-raised. The negative TTL issue can silently produce incorrect behaviour at runtime, and the UTF-8 slicing issue can produce corrupted strings for any non-ASCII input.
🔄 2 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
⚠️ Inline comments could not be attached (lines not in diff). See summary above.
chsrimanaswi
dismissed
github-actions[bot]’s stale review
Superseded by new review
chsrimanaswi
left a comment
chsrimanaswi
left a comment
There was a problem hiding this comment.
🤖 PR Reviewer
Two previously raised issues remain unaddressed: the multi-byte UTF-8 boundary split in sanitizeValue, and the negative TTL for past dates in computeTTL. The sanitizeValue non-string guard was reasonably dismissed given upstream AJV validation. The other two issues still have real impact at runtime.
🔄 2 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
⚠️ Inline comments could not be attached (lines not in diff). See summary above.
chsrimanaswi
left a comment
chsrimanaswi
left a comment
There was a problem hiding this comment.
🤖 PR Reviewer
The new utility functions are clean and well-documented. The sanitizeValue concern about non-string inputs was acknowledged as handled upstream via AJV validation, which is a reasonable justification. One minor issue exists in computeTTL where a negative TTL can be returned for already-expired dates, which could cause unexpected behavior in storage backends.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
⚠️ Inline comments could not be attached (lines not in diff). See summary above.
chsrimanaswi
left a comment
chsrimanaswi
left a comment
There was a problem hiding this comment.
🤖 PR Reviewer
Two small utility functions added with good documentation. The sanitizeValue function has a correctness issue where slicing UTF-8 bytes and calling .toString() can produce garbled output or an exception if the slice cuts in the middle of a multi-byte character. The computeTTL function looks correct and handles edge cases well.
📝 1 suggestion(s) - Please review inline comments below.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
⚠️ Inline comments could not be attached (lines not in diff). See summary above.
|
/review |
chsrimanaswi
left a comment
chsrimanaswi
left a comment
There was a problem hiding this comment.
🤖 PR Reviewer
Two small utility functions added with clear documentation. The logic is straightforward, but there is a subtle bug in sanitizeValue where slicing UTF-8 bytes and converting back to a string can produce an invalid (truncated mid-character) string for multi-byte characters. computeTTL correctly handles Date and number inputs, though negative TTLs from past dates are silently returned without validation.
📝 2 suggestion(s) - Please review inline comments below.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
chsrimanaswi
left a comment
chsrimanaswi
left a comment
There was a problem hiding this comment.
🤖 PR Reviewer
Two small utility functions are added with clear documentation. The logic is straightforward, but there are a couple of correctness issues worth addressing before approval.
📝 2 suggestion(s) - Please review inline comments below.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
There was a problem hiding this comment.
🤖 PR Reviewer
The two new utility functions are well-documented and straightforward. Minor issues exist around edge-case handling and potential data corruption when truncating multi-byte UTF-8 characters. The workflow change pins to a feature branch instead of a stable ref, which is a concern for production use.
📝 4 suggestion(s) - Please review inline comments below.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
|
/review |
There was a problem hiding this comment.
🤖 PR Reviewer
The diff adds two utility functions (computeTTL and sanitizeValue) and updates a workflow reference. The workflow branch-pinning issue and the UTF-8 truncation bug in sanitizeValue remain unaddressed from the previous review. The negative TTL issue in computeTTL and the missing type guard for sanitizeValue also persist.
🔄 3 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
|
/review |
There was a problem hiding this comment.
🤖 PR Reviewer
Two of the three previously raised issues remain unaddressed: the reusable workflow is still pinned to a feature branch, and both computeTTL (negative TTL for past dates) and sanitizeValue (missing type guard, unsafe UTF-8 byte-boundary slicing) retain the same problems. No new issues were introduced.
🔄 3 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
There was a problem hiding this comment.
🤖 PR Reviewer
The diff introduces computeTTL and sanitizeValue utility functions. Both previously-raised issues in lib/utils.js remain unaddressed: negative TTL for past dates and unsafe UTF-8 boundary slicing. The workflow branch pin is also still present.
🔄 3 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
|
/review |
There was a problem hiding this comment.
🤖 PR Reviewer
The diff introduces two new utility functions (computeTTL and sanitizeValue) and updates a workflow reference. Both previously raised issues in lib/utils.js remain unaddressed: computeTTL can return a negative TTL for past dates, and sanitizeValue still lacks a type guard and can corrupt multi-byte UTF-8 characters at the slice boundary. The workflow branch pin issue also persists.
🔄 3 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
There was a problem hiding this comment.
🤖 PR Reviewer
The diff introduces two new utility functions. The previously raised issues in lib/utils.js (negative TTL and UTF-8 boundary slicing) remain unaddressed. The workflow branch pin is still on the feature branch, acknowledged as temporary.
🔄 2 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| */ | ||
| function computeTTL (expiry) { | ||
| if (expiry === undefined || expiry === null) return undefined | ||
| if (expiry instanceof Date) { |
There was a problem hiding this comment.
[Re-raised] When a Date in the past is provided, this returns a negative TTL, which will likely cause silent bugs or unexpected behavior downstream. Consider returning 0 (or throwing) for expired dates.
| if (expiry instanceof Date) { | |
| if (expiry instanceof Date) { | |
| const ttl = Math.floor((expiry.getTime() - Date.now()) / 1000) | |
| return ttl > 0 ? ttl : 0 | |
| } |
| */ | ||
| function sanitizeValue (value, maxBytes) { | ||
| const encoded = Buffer.from(value) | ||
| if (encoded.length <= maxBytes) return value |
There was a problem hiding this comment.
[Re-raised] Slicing a UTF-8 Buffer at an arbitrary byte boundary can split a multi-byte character, producing an invalid UTF-8 sequence and corrupting the last character. Also, the missing type guard means non-string values will cause Buffer.from to throw or behave unexpectedly.
| */ | |
| function sanitizeValue (value, maxBytes) { | |
| const encoded = Buffer.from(value) | |
| if (encoded.length <= maxBytes) return value | |
| function sanitizeValue (value, maxBytes) { | |
| if (typeof value !== 'string') throw new TypeError('sanitizeValue: value must be a string') | |
| const encoded = Buffer.from(value) | |
| if (encoded.length <= maxBytes) return value | |
| let truncated = encoded.slice(0, maxBytes).toString('utf8') | |
| while (Buffer.byteLength(truncated) > maxBytes) truncated = truncated.slice(0, -1) | |
| return truncated | |
| } |
There was a problem hiding this comment.
🤖 PR Reviewer
The two previously flagged issues in lib/utils.js remain unaddressed: computeTTL still returns negative TTLs for past dates, and sanitizeValue still lacks a type guard and can corrupt multi-byte UTF-8 characters by slicing at arbitrary byte boundaries. The workflow pin to a feature branch is a minor concern but may be intentional for testing.
📝 3 suggestion(s) (1 new, 2 re-raised)
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| if (expiry === undefined || expiry === null) return undefined | ||
| if (expiry instanceof Date) { | ||
| return Math.floor((expiry.getTime() - Date.now()) / 1000) |
There was a problem hiding this comment.
[Re-raised] When a Date in the past is provided, this returns a negative TTL, which will likely cause silent bugs or unexpected behavior downstream. Consider returning 0 (or throwing) for expired dates.
| if (expiry === undefined || expiry === null) return undefined | |
| if (expiry instanceof Date) { | |
| return Math.floor((expiry.getTime() - Date.now()) / 1000) | |
| if (expiry instanceof Date) { | |
| const ttl = Math.floor((expiry.getTime() - Date.now()) / 1000) | |
| return ttl > 0 ? ttl : 0 | |
| } |
| const encoded = Buffer.from(value) | ||
| if (encoded.length <= maxBytes) return value | ||
| return encoded.slice(0, maxBytes).toString() | ||
| } | ||
|
|
There was a problem hiding this comment.
[Re-raised] Slicing a UTF-8 Buffer at an arbitrary byte boundary can split a multi-byte character, producing an invalid UTF-8 sequence and corrupting the last character. Also, the missing type guard means non-string values will cause Buffer.from to throw or behave unexpectedly.
| const encoded = Buffer.from(value) | |
| if (encoded.length <= maxBytes) return value | |
| return encoded.slice(0, maxBytes).toString() | |
| } | |
| function sanitizeValue (value, maxBytes) { | |
| if (typeof value !== 'string') throw new TypeError('sanitizeValue: value must be a string') | |
| const encoded = Buffer.from(value) | |
| if (encoded.length <= maxBytes) return value | |
| let truncated = encoded.slice(0, maxBytes).toString('utf8') | |
| while (Buffer.byteLength(truncated) > maxBytes) truncated = truncated.slice(0, -1) | |
| return truncated | |
| } |
| needs: check | ||
| if: needs.check.outputs.allowed == 'true' | ||
| uses: adobe/aio-reusable-workflows/.github/workflows/pr-review.yml@main | ||
| uses: adobe/aio-reusable-workflows/.github/workflows/pr-review.yml@fix/pr-review-pedantic-and-human-replies |
There was a problem hiding this comment.
Pinning the reusable workflow to a feature branch (fix/pr-review-pedantic-and-human-replies) rather than a stable ref like main or a tagged version means any force-push or deletion of that branch could break CI. Ensure this is reverted to main or a versioned tag once the fix is merged.
| uses: adobe/aio-reusable-workflows/.github/workflows/pr-review.yml@fix/pr-review-pedantic-and-human-replies | |
| uses: adobe/aio-reusable-workflows/.github/workflows/pr-review.yml@main |
1 participant
Test PR to validate automated PR reviewer.