Agents/sss integration review feedback#5083
Closed
psrsingh wants to merge 3 commits into
Closed
Conversation
Signed-off-by: psrsingh <psr.singh336@gmail.com>
Signed-off-by: psrsingh <psr.singh336@gmail.com>
WalkthroughThe PR integrates an external Sanctions Screening Service (SSS) client into corporate and employee signature processing. Configuration is loaded from local and SSM sources with Auth0 credentials and timeout settings. A new company repository method persists sanction flags to DynamoDB. The sign service is extended with SSS client initialization, compliance caching with TTL, and domain-resolution logic for screening. Both signature flows are updated to call compliance checks before proceeding. ChangesSanctions Screening Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Signed-off-by: psrsingh <psr.singh336@gmail.com>
psrsingh
force-pushed
the
agents/sss-integration-review-feedback
branch
from
6347fe6 to
2ebd9e1
Compare
Member
|
Why a new PR? Instead of continuying on #5078 ? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cla-backend-go/v2/sign/service.go (1)
3057-3057: SSS flag comparison is valid; minor redundancy in blocked computation
sss.StatusFlaggedexists (exported asStatusFlagged = "flagged"incla-backend-go/sss/types.go), so the line 3057 comparison is sound.blocked := company.IsSanctioned || sanctionedis redundant if the function already returns whencompany.IsSanctionedis true; it’s defensive but can be simplified for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cla-backend-go/v2/sign/service.go` at line 3057, The comparison using sss.StatusFlagged is correct but the subsequent blocked := company.IsSanctioned || sanctioned is redundant if the code already returns when company.IsSanctioned is true; simplify by either removing the early return and keeping blocked as company.IsSanctioned || result.Status == sss.StatusFlagged (referencing result.Status and sss.StatusFlagged) or, if you keep the early return that exits when company.IsSanctioned is true, change blocked to just sanctioned (the local variable sanctioned computed from result.Status) and remove the redundant company.IsSanctioned check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cla-backend-go/company/repository.go`:
- Around line 1282-1338: UpdateCompanySanctionStatus currently calls
repo.dynamoDBClient.UpdateItem(input) without ctx; change it to the
context-aware API repo.dynamoDBClient.UpdateItemWithContext(ctx, input) so the
passed ctx (from the function parameter and f log fields) propagates to DynamoDB
calls and respects cancellations/deadlines; update the call site that assigns _,
err = repo.dynamoDBClient.UpdateItem(...) to use UpdateItemWithContext(ctx, ...)
and keep the same error handling and return path.
---
Nitpick comments:
In `@cla-backend-go/v2/sign/service.go`:
- Line 3057: The comparison using sss.StatusFlagged is correct but the
subsequent blocked := company.IsSanctioned || sanctioned is redundant if the
code already returns when company.IsSanctioned is true; simplify by either
removing the early return and keeping blocked as company.IsSanctioned ||
result.Status == sss.StatusFlagged (referencing result.Status and
sss.StatusFlagged) or, if you keep the early return that exits when
company.IsSanctioned is true, change blocked to just sanctioned (the local
variable sanctioned computed from result.Status) and remove the redundant
company.IsSanctioned check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd6c02e9-ecd2-44d4-9f2e-8e94f22bc935
📒 Files selected for processing (9)
cla-backend-go/cmd/s3_upload/main.gocla-backend-go/cmd/server.gocla-backend-go/company/repository.gocla-backend-go/config/config.gocla-backend-go/config/local.gocla-backend-go/config/ssm.gocla-backend-go/v2/sign/helpers.gocla-backend-go/v2/sign/service.gocla-backend-go/v2/sign/service_sss_test.go
Comment on lines
+1282
to
+1338
| func (repo repository) UpdateCompanySanctionStatus(ctx context.Context, companyID string, sanctioned bool) error { | ||
| f := logrus.Fields{ | ||
| "functionName": "company.repository.UpdateCompanySanctionStatus", | ||
| utils.XREQUESTID: ctx.Value(utils.XREQUESTID), | ||
| "companyID": companyID, | ||
| "sanctioned": sanctioned, | ||
| } | ||
|
|
||
| // Fetch current company to check if value has changed | ||
| currentCompany, err := repo.GetCompany(ctx, companyID) | ||
| if err != nil { | ||
| log.WithFields(f).Warnf("unable to fetch current company record to check sanction status, error: %v", err) | ||
| return err | ||
| } | ||
| if currentCompany == nil { | ||
| return fmt.Errorf("company not found: %s", companyID) | ||
| } | ||
|
|
||
| // Avoid unnecessary writes - only update if value has changed | ||
| if currentCompany.IsSanctioned == sanctioned { | ||
| log.WithFields(f).Debugf("sanction status unchanged (current=%v, new=%v), skipping update", currentCompany.IsSanctioned, sanctioned) | ||
| return nil | ||
| } | ||
|
|
||
| log.WithFields(f).Debugf("updating sanction status from %v to %v", currentCompany.IsSanctioned, sanctioned) | ||
|
|
||
| _, now := utils.CurrentTime() | ||
|
|
||
| input := &dynamodb.UpdateItemInput{ | ||
| ExpressionAttributeNames: map[string]*string{ | ||
| "#S": aws.String("is_sanctioned"), | ||
| "#M": aws.String("date_modified"), | ||
| }, | ||
| ExpressionAttributeValues: map[string]*dynamodb.AttributeValue{ | ||
| ":s": { | ||
| BOOL: aws.Bool(sanctioned), | ||
| }, | ||
| ":m": { | ||
| S: aws.String(now), | ||
| }, | ||
| }, | ||
| TableName: aws.String(repo.companyTableName), | ||
| Key: map[string]*dynamodb.AttributeValue{ | ||
| "company_id": { | ||
| S: aws.String(companyID), | ||
| }, | ||
| }, | ||
| UpdateExpression: aws.String("SET #S = :s, #M = :m"), | ||
| } | ||
|
|
||
| _, err = repo.dynamoDBClient.UpdateItem(input) | ||
| if err != nil { | ||
| log.WithFields(f).Warnf("error updating company sanction status, error: %v", err) | ||
| return err | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does aws-sdk-go v1.36.27 dynamodb client provide an UpdateItemWithContext method?
💡 Result:
No. In aws-sdk-go v1, the DynamoDB client’s context-aware method for UpdateItem is named UpdateItemWithContext (not UpdateItemWithContextWithContext / etc.), and aws-sdk-go v1 does not provide any additional method beyond that naming in the dynamodbiface interface. The interface includes UpdateItemWithContext(aws.Context, *dynamodb.UpdateItemInput,...request.Option), alongside UpdateItem(*dynamodb.UpdateItemInput). [1] So, the answer to “Does aws-sdk-go v1.36.27 dynamodb client provide an UpdateItemWithContext method?” is: Yes—UpdateItemWithContext exists for the DynamoDB client in aws-sdk-go v1 (as specified by the official dynamodbiface for the v1 SDK). [1]
Citations:
Propagate request ctx to the DynamoDB UpdateItem call.
UpdateCompanySanctionStatus accepts ctx but the DynamoDB write uses UpdateItem(input) (no context), so cancellations/deadlines won’t reach DynamoDB. Use the context-aware method supported by aws-sdk-go v1 (UpdateItemWithContext).
♻️ Use context-aware UpdateItem
- _, err = repo.dynamoDBClient.UpdateItem(input)
+ _, err = repo.dynamoDBClient.UpdateItemWithContext(ctx, input)
if err != nil {
log.WithFields(f).Warnf("error updating company sanction status, error: %v", err)
return err
}The read-before-write skip optimization and the BOOL/string attribute mapping look correct and consistent with the table model.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (repo repository) UpdateCompanySanctionStatus(ctx context.Context, companyID string, sanctioned bool) error { | |
| f := logrus.Fields{ | |
| "functionName": "company.repository.UpdateCompanySanctionStatus", | |
| utils.XREQUESTID: ctx.Value(utils.XREQUESTID), | |
| "companyID": companyID, | |
| "sanctioned": sanctioned, | |
| } | |
| // Fetch current company to check if value has changed | |
| currentCompany, err := repo.GetCompany(ctx, companyID) | |
| if err != nil { | |
| log.WithFields(f).Warnf("unable to fetch current company record to check sanction status, error: %v", err) | |
| return err | |
| } | |
| if currentCompany == nil { | |
| return fmt.Errorf("company not found: %s", companyID) | |
| } | |
| // Avoid unnecessary writes - only update if value has changed | |
| if currentCompany.IsSanctioned == sanctioned { | |
| log.WithFields(f).Debugf("sanction status unchanged (current=%v, new=%v), skipping update", currentCompany.IsSanctioned, sanctioned) | |
| return nil | |
| } | |
| log.WithFields(f).Debugf("updating sanction status from %v to %v", currentCompany.IsSanctioned, sanctioned) | |
| _, now := utils.CurrentTime() | |
| input := &dynamodb.UpdateItemInput{ | |
| ExpressionAttributeNames: map[string]*string{ | |
| "#S": aws.String("is_sanctioned"), | |
| "#M": aws.String("date_modified"), | |
| }, | |
| ExpressionAttributeValues: map[string]*dynamodb.AttributeValue{ | |
| ":s": { | |
| BOOL: aws.Bool(sanctioned), | |
| }, | |
| ":m": { | |
| S: aws.String(now), | |
| }, | |
| }, | |
| TableName: aws.String(repo.companyTableName), | |
| Key: map[string]*dynamodb.AttributeValue{ | |
| "company_id": { | |
| S: aws.String(companyID), | |
| }, | |
| }, | |
| UpdateExpression: aws.String("SET #S = :s, #M = :m"), | |
| } | |
| _, err = repo.dynamoDBClient.UpdateItem(input) | |
| if err != nil { | |
| log.WithFields(f).Warnf("error updating company sanction status, error: %v", err) | |
| return err | |
| } | |
| return nil | |
| func (repo repository) UpdateCompanySanctionStatus(ctx context.Context, companyID string, sanctioned bool) error { | |
| f := logrus.Fields{ | |
| "functionName": "company.repository.UpdateCompanySanctionStatus", | |
| utils.XREQUESTID: ctx.Value(utils.XREQUESTID), | |
| "companyID": companyID, | |
| "sanctioned": sanctioned, | |
| } | |
| // Fetch current company to check if value has changed | |
| currentCompany, err := repo.GetCompany(ctx, companyID) | |
| if err != nil { | |
| log.WithFields(f).Warnf("unable to fetch current company record to check sanction status, error: %v", err) | |
| return err | |
| } | |
| if currentCompany == nil { | |
| return fmt.Errorf("company not found: %s", companyID) | |
| } | |
| // Avoid unnecessary writes - only update if value has changed | |
| if currentCompany.IsSanctioned == sanctioned { | |
| log.WithFields(f).Debugf("sanction status unchanged (current=%v, new=%v), skipping update", currentCompany.IsSanctioned, sanctioned) | |
| return nil | |
| } | |
| log.WithFields(f).Debugf("updating sanction status from %v to %v", currentCompany.IsSanctioned, sanctioned) | |
| _, now := utils.CurrentTime() | |
| input := &dynamodb.UpdateItemInput{ | |
| ExpressionAttributeNames: map[string]*string{ | |
| "`#S`": aws.String("is_sanctioned"), | |
| "`#M`": aws.String("date_modified"), | |
| }, | |
| ExpressionAttributeValues: map[string]*dynamodb.AttributeValue{ | |
| ":s": { | |
| BOOL: aws.Bool(sanctioned), | |
| }, | |
| ":m": { | |
| S: aws.String(now), | |
| }, | |
| }, | |
| TableName: aws.String(repo.companyTableName), | |
| Key: map[string]*dynamodb.AttributeValue{ | |
| "company_id": { | |
| S: aws.String(companyID), | |
| }, | |
| }, | |
| UpdateExpression: aws.String("SET `#S` = :s, `#M` = :m"), | |
| } | |
| _, err = repo.dynamoDBClient.UpdateItemWithContext(ctx, input) | |
| if err != nil { | |
| log.WithFields(f).Warnf("error updating company sanction status, error: %v", err) | |
| return err | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cla-backend-go/company/repository.go` around lines 1282 - 1338,
UpdateCompanySanctionStatus currently calls
repo.dynamoDBClient.UpdateItem(input) without ctx; change it to the
context-aware API repo.dynamoDBClient.UpdateItemWithContext(ctx, input) so the
passed ctx (from the function parameter and f log fields) propagates to DynamoDB
calls and respects cancellations/deadlines; update the call site that assigns _,
err = repo.dynamoDBClient.UpdateItem(...) to use UpdateItemWithContext(ctx, ...)
and keep the same error handling and return path.
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
2 participants
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.
Summary
Add SSS (Sanctions Screening Service) enforcement to signing flows and address review feedback around sanction handling, rollout safety, and performance.
Changes
SSS compliance checks
Added SSS compliance checks to signing flows in
cla-backend-go.Preserve existing manual sanction behavior:
is_sanctionedremains an independent/manual block.is_sanctioned=true.Added explicit handling for SSS timeout, auth, retryable, and not-found errors.
Configurable rollout
cla-sss-required-<stage>configuration support.Required=true, SSS availability errors fail closed.Required=false, SSS availability errors are logged and signing proceeds.Domain resolution
Resolve domains using:
DomainsLinkas fallbackNormalize parsed domains and strip
www.prefixes.Removed unreachable code from
resolveDomain().Persistence
UpdateCompanySanctionStatus.Performance
Notes
cla-backend-legacydoes not currently have an SSS client integration. The live SSS checks added in this change are incla-backend-go.The legacy ECLA endpoints continue to enforce the persisted
is_sanctionedflag. Adding live SSS checks directly to the legacy ECLA flow would require separate work incla-backend-legacy.Validation
gofmtcompletedgit diff --checkpassed