fix(HNT-1804): fix logic bugs across monorepo — timestamp rounding, off-by-one, null safety#371
fix(HNT-1804): fix logic bugs across monorepo — timestamp rounding, off-by-one, null safety#371
Conversation
… rounding in getUnixTimestamp getUnixTimestamp used .toFixed(0) which rounds to nearest integer, causing timestamps to be 1 second too high for dates with >=500ms. Unix timestamps should truncate, not round. This affected all Snowplow events for Sections and SectionItems.
…f Math.round for unix timestamp fallback
… rounding in UnixTimestamp resolver
…or JWT iat timestamp
…vent crash on slugless URLs
|
Claude finished @mmiermans's task in 1m 55s —— View job Code Review
Good TDD discipline throughout — failing tests written first, minimal targeted fixes, clear test names that describe the specific regression being prevented. Here are my observations: Timestamp Fixes (4 instances)All four One observation on the JWT test ( Off-by-One in
|
Plan Result (corpus-scheduler-lambda-cdk-production)
Change Result (Click me) # aws_lambda_function.corpus-scheduler-sqs-lambda_F2ECDF9F will be updated in-place
~ resource "aws_lambda_function" "corpus-scheduler-sqs-lambda_F2ECDF9F" {
id = "CorpusSchedulerLambda-Prod-SQS-Function"
~ qualified_arn = "arn:aws:lambda:us-east-1:996905175585:function:CorpusSchedulerLambda-Prod-SQS-Function:286" -> (known after apply)
~ qualified_invoke_arn = "arn:aws:apigateway:us-east-1:lambda:path/2015-03-31/functions/arn:aws:lambda:us-east-1:996905175585:function:CorpusSchedulerLambda-Prod-SQS-Function:286/invocations" -> (known after apply)
tags = {
"app_code" = "content"
"component_code" = "content-corpusschedulerlambda"
"env_code" = "prod"
"environment" = "Prod"
"service" = "CorpusSchedulerLambda"
}
~ version = "286" -> (known after apply)
# (20 unchanged attributes hidden)
~ environment {
~ variables = {
~ "GIT_SHA" = (sensitive value)
~ "SENTRY_DSN" = (sensitive value)
# (6 unchanged elements hidden)
}
}
# (4 unchanged blocks hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.
|
Plan Result (section-manager-lambda-cdk-production)
Change Result (Click me) # aws_lambda_function.section-manager-sqs-lambda_D7365DAE will be updated in-place
~ resource "aws_lambda_function" "section-manager-sqs-lambda_D7365DAE" {
id = "SectionManagerLambda-Prod-SQS-Function"
~ qualified_arn = "arn:aws:lambda:us-east-1:996905175585:function:SectionManagerLambda-Prod-SQS-Function:82" -> (known after apply)
~ qualified_invoke_arn = "arn:aws:apigateway:us-east-1:lambda:path/2015-03-31/functions/arn:aws:lambda:us-east-1:996905175585:function:SectionManagerLambda-Prod-SQS-Function:82/invocations" -> (known after apply)
tags = {
"app_code" = "content"
"component_code" = "content-sectionmanagerlambda"
"env_code" = "prod"
"environment" = "Prod"
"service" = "SectionManagerLambda"
}
~ version = "82" -> (known after apply)
# (20 unchanged attributes hidden)
~ environment {
~ variables = {
- "GIT_SHA" = "4091bdab8ff6fcb734392fdf2c606351e52246c9" -> null
# (5 unchanged elements hidden)
}
}
# (4 unchanged blocks hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.
|
Plan Result (prospect-translation-lambda-cdk-production)
Change Result (Click me) # aws_lambda_function.translation-lambda_translation-sqs-lambda_B9BDF6BA will be updated in-place
~ resource "aws_lambda_function" "translation-lambda_translation-sqs-lambda_B9BDF6BA" {
id = "ProspectAPI-Prod-Sqs-Translation-Function"
tags = {
"app_code" = "content"
"component_code" = "content-prospectapi"
"env_code" = "prod"
"environment" = "Prod"
"service" = "ProspectAPI-Sqs-Translation"
}
# (22 unchanged attributes hidden)
~ environment {
~ variables = {
~ "GIT_SHA" = (sensitive value)
~ "SENTRY_DSN" = (sensitive value)
# (4 unchanged elements hidden)
}
}
# (4 unchanged blocks hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.
|
Plan Result (prospect-api-cdk-production)
Change Result (Click me) # data.aws_iam_policy_document.application_ecs_service_ecs-iam_data-ecs-task-role-policy_090CC3AD will be read during apply
# (depends on a resource or a module with changes pending)
<= data "aws_iam_policy_document" "application_ecs_service_ecs-iam_data-ecs-task-role-policy_090CC3AD" {
+ id = (known after apply)
+ json = (known after apply)
+ minified_json = (known after apply)
+ version = "2012-10-17"
+ statement {
+ actions = [
+ "dynamodb:BatchGet*",
+ "dynamodb:DescribeTable",
+ "dynamodb:Get*",
+ "dynamodb:Query",
+ "dynamodb:Scan",
+ "dynamodb:UpdateItem",
]
+ effect = "Allow"
+ resources = [
+ "arn:aws:dynamodb:us-east-1:996905175585:table/PROAPI-Prod-Prospects",
+ "arn:aws:dynamodb:us-east-1:996905175585:table/PROAPI-Prod-Prospects/*",
]
}
+ statement {
+ actions = [
+ "s3:*",
]
+ effect = "Allow"
+ resources = [
+ "arn:aws:s3:::pocket-prospectapi-prod-images",
+ "arn:aws:s3:::pocket-prospectapi-prod-images/*",
]
}
+ statement {
+ actions = [
+ "logs:CreateLogGroup",
+ "logs:CreateLogStream",
+ "logs:DescribeLogGroups",
+ "logs:DescribeLogStreams",
+ "logs:PutLogEvents",
]
+ effect = "Allow"
+ resources = [
+ "*",
]
}
}
# aws_dynamodb_table.dynamodb_prospects_dynamodb_table_9854E41E will be updated in-place
~ resource "aws_dynamodb_table" "dynamodb_prospects_dynamodb_table_9854E41E" {
id = "PROAPI-Prod-Prospects"
name = "PROAPI-Prod-Prospects"
tags = {
"app_code" = "content"
"component_code" = "content-prospectapi"
"env_code" = "prod"
"environment" = "Prod"
"service" = "ProspectAPI"
}
# (9 unchanged attributes hidden)
- global_secondary_index {
- hash_key = "scheduledSurfaceGuid" -> null
- name = "scheduledSurfaceGuid-prospectType" -> null
- non_key_attributes = [] -> null
- projection_type = "ALL" -> null
- range_key = "prospectType" -> null
- read_capacity = 0 -> null
- write_capacity = 0 -> null
}
+ global_secondary_index {
+ hash_key = "scheduledSurfaceGuid"
+ name = "scheduledSurfaceGuid-prospectType"
+ non_key_attributes = []
+ projection_type = "ALL"
+ range_key = "prospectType"
+ read_capacity = 5
+ write_capacity = 5
}
# (5 unchanged blocks hidden)
}
# aws_iam_policy.application_ecs_service_ecs-iam_ecs-task-role-policy_6FC89FB6 will be updated in-place
~ resource "aws_iam_policy" "application_ecs_service_ecs-iam_ecs-task-role-policy_6FC89FB6" {
id = "arn:aws:iam::996905175585:policy/ProspectAPI-Prod-TaskRolePolicy"
name = "ProspectAPI-Prod-TaskRolePolicy"
~ policy = jsonencode(
{
- Statement = [
- {
- Action = [
- "dynamodb:UpdateItem",
- "dynamodb:Scan",
- "dynamodb:Query",
- "dynamodb:Get*",
- "dynamodb:DescribeTable",
- "dynamodb:BatchGet*",
]
- Effect = "Allow"
- Resource = [
- "arn:aws:dynamodb:us-east-1:996905175585:table/PROAPI-Prod-Prospects/*",
- "arn:aws:dynamodb:us-east-1:996905175585:table/PROAPI-Prod-Prospects",
]
},
- {
- Action = "s3:*"
- Effect = "Allow"
- Resource = [
- "arn:aws:s3:::pocket-prospectapi-prod-images/*",
- "arn:aws:s3:::pocket-prospectapi-prod-images",
]
},
- {
- Action = [
- "logs:PutLogEvents",
- "logs:DescribeLogStreams",
- "logs:DescribeLogGroups",
- "logs:CreateLogStream",
- "logs:CreateLogGroup",
]
- Effect = "Allow"
- Resource = "*"
},
]
- Version = "2012-10-17"
}
) -> (known after apply)
tags = {
"app_code" = "content"
"component_code" = "content-prospectapi"
"env_code" = "prod"
"environment" = "Prod"
"service" = "ProspectAPI"
}
# (5 unchanged attributes hidden)
}
Plan: 0 to add, 2 to change, 0 to destroy. |
| * * @param maxLength the maximum returned length allowed | ||
| * @returns sanitized string | ||
| */ | ||
| export const sanitizeText = (input: string, maxLength: number): string => { |
There was a problem hiding this comment.
This function is only used for the optional comment curators leave when removing/scheduling items (maxLength = 100). The longest comment ever recorded is 76 characters, so the off-by-one never truncated real data. It's a correct fix, but no real-world impact.
My bigger concern with this function is that the allowlist regex [^a-zA-Z0-9_ \-.!?] does not document why these characters are allowed. It silently strips commas, apostrophes, quotes, colons, and all non-ASCII. My guess is that this may have been done to protect against SQL-injection, but that would be an anti-pattern.
| export const getUrlId = (path: string): string | null => { | ||
| const match = path.match(slugRegex); | ||
| return match ? match[1] : null; | ||
| }; |
There was a problem hiding this comment.
This should be unused code, because we are no longer creating new syndicated or collection URLs.
| */ | ||
| export const generateJwt = (config: JwtConfig, privateKey: any): string => { | ||
| const now = Math.round(Date.now() / 1000); | ||
| const now = Math.floor(Date.now() / 1000); |
There was a problem hiding this comment.
This is an improvement, but without any real-world impact that I can think of.
|
I've thought about what we should review in this PR, and I think there are a few levels. The fixesCorrect, but no real-world impact. Other bugs foundThe loop also identified higher-severity bugs that were deferred because they need integration tests. Of those, the delete-before-validate (issue 1) looks like a real bug worth investigating, though probably low/medium rather than high severity. The cross-section scope (issue 2) is by design. The approachI wonder if code audits are a hard starting point for autonomous loops. The bugs that matter require context that isn't in the code: which paths are hot, what the data looks like, what are our OKRs, which features are deprecated. A Other applicationsOther applications might get traction faster:
I'm excited about this direction overall! |
jpetto
left a comment
jpetto
left a comment
There was a problem hiding this comment.
as mentioned in comments, the code changes here are fine, but don't have much (if any) real world impact.
as discussed, let's drop the tests around Math.floor, as they aren't providing value.
3 participants
Goal
Fix 6 real logic bugs found via systematic TDD audit across the monorepo.
Math.round/.toFixed(0)produces timestamps 1 second in the future when milliseconds >= 500. Changed toMath.floortomatch unix timestamp convention. Affected:
getUnixTimestampin shared utils,UnixTimestampresolver, JWTiatgeneration, and Snowplow prospect event fallback.sanitizeText(1 fix):substring(0, maxLength - 1)returned 99 characters whenmaxLengthwas 100. Fixed tosubstring(0, maxLength).getUrlId(1 fix):path.match(slugRegex)[1]crashes withTypeErroron getpocket.com collection URLs without a slug (e.g./collections/).Added null check.
I'd love feedback/perspectives on:
rejectApprovedItemdeleting before validating therejection reason, and
createSectionItemmissing asectionIdscope on the manually-removed check. Should those be addressed in a follow-up?Implementation Decisions
Deployment steps
References
JIRA ticket:
Issue:
Documentation: