Address AI feedback

Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io>

Assisted by [OpenAI](https://platform.openai.com/)

Assisted by [GitHub Copilot](https://github.com/features/copilot)

Assisted by [Claude](https://claude.ai)

Author: lukaszgryglicki

cla-backend-go/company/mocks/mock_repo.go

@@ -55,7 +55,7 @@ type IRepository interface { //nolint
 	RejectCompanyAccessRequest(ctx context.Context, companyInviteID string) error
 	UpdateCompanyAccessList(ctx context.Context, companyID string, companyACL []string) error
 	UpdateCompanySanctionStatus(ctx context.Context, companyID string, sanctioned bool, origin string) error
-	ClearCompanySanctionStatusIfSSS(ctx context.Context, companyID string) error
+	ClearCompanySanctionStatusIfSSS(ctx context.Context, companyID string) (bool, error)
 	IsCCLAEnabledForCompany(ctx context.Context, companyID string) (bool, error)
 }
 
@@ -1279,6 +1279,9 @@ func (repo repository) UpdateCompanyAccessList(ctx context.Context, companyID st
 	return nil
 }
 
+// sanctionOriginSSS is the sanction_origin value written by the Sanctions Screening Service.
+const sanctionOriginSSS = "sss"
+
 // UpdateCompanySanctionStatus sets is_sanctioned and, when origin is non-empty, sanction_origin.
 // Pass origin="sss" when flagging via SSS; pass origin="" for manual admin updates.
 func (repo repository) UpdateCompanySanctionStatus(ctx context.Context, companyID string, sanctioned bool, origin string) error {
@@ -1327,7 +1330,7 @@ func (repo repository) UpdateCompanySanctionStatus(ctx context.Context, companyI
 	// with absent or non-"sss" origin). Only set the SSS flag when the company is
 	// currently unblocked or already SSS-blocked. A ConditionalCheckFailedException
 	// therefore means a manual/admin block is already in place and must be preserved.
-	sssSettingBlock := sanctioned && origin == "sss"
+	sssSettingBlock := sanctioned && origin == sanctionOriginSSS
 	if sssSettingBlock {
 		values[":false"] = &dynamodb.AttributeValue{BOOL: aws.Bool(false)}
 		input.ConditionExpression = aws.String("attribute_not_exists(#S) OR #S = :false OR #O = :o")
@@ -1347,8 +1350,10 @@ func (repo repository) UpdateCompanySanctionStatus(ctx context.Context, companyI
 }
 
 // ClearCompanySanctionStatusIfSSS clears is_sanctioned only when sanction_origin="sss".
-// A ConditionalCheckFailedException (manual/absent origin) is silently ignored.
-func (repo repository) ClearCompanySanctionStatusIfSSS(ctx context.Context, companyID string) error {
+// It returns (cleared, err): cleared is true only when the conditional matched and the
+// record was actually cleared; a ConditionalCheckFailedException (manual/absent origin)
+// returns (false, nil) so callers can leave any manual/admin block in place.
+func (repo repository) ClearCompanySanctionStatusIfSSS(ctx context.Context, companyID string) (bool, error) {
 	f := logrus.Fields{
 		"functionName":   "company.repository.ClearCompanySanctionStatusIfSSS",
 		utils.XREQUESTID: ctx.Value(utils.XREQUESTID),
@@ -1372,19 +1377,19 @@ func (repo repository) ClearCompanySanctionStatusIfSSS(ctx context.Context, comp
 		ExpressionAttributeValues: map[string]*dynamodb.AttributeValue{
 			":false": {BOOL: aws.Bool(false)},
 			":m":     {S: aws.String(now)},
-			":sss":   {S: aws.String("sss")},
+			":sss":   {S: aws.String(sanctionOriginSSS)},
 		},
 	}
 
 	if _, err := repo.dynamoDBClient.UpdateItem(input); err != nil {
 		if aerr, ok := err.(awserr.Error); ok && aerr.Code() == dynamodb.ErrCodeConditionalCheckFailedException {
 			log.WithFields(f).Debugf("sanction_origin != sss for company %s; not auto-clearing (manual/admin block)", companyID)
-			return nil
+			return false, nil
 		}
 		log.WithFields(f).Warnf("error clearing company sanction status: %v", err)
-		return err
+		return false, err
 	}
-	return nil
+	return true, nil
 }
 
 func (repo repository) CreateCompany(ctx context.Context, in *models.Company) (*models.Company, error) {

cla-backend-go/company/repository.go

@@ -64,6 +64,7 @@ const (
 	DocusignCompleted      = "Completed"
 	complianceCacheTTL     = 5 * time.Minute
 	maxComplianceCacheSize = 1000
+	sanctionOriginSSS      = "sss"
 )
 
 // errors
@@ -2997,14 +2998,18 @@ func (s *service) checkCompanyCompliance(ctx context.Context, company *v1Models.
 
 	// Short-circuit for manually/admin-set blocks (sanction_origin != "sss" or no origin).
 	// SSS-origin blocks fall through so a now-clean result can clear them.
-	if company.IsSanctioned && company.SanctionOrigin != "sss" {
+	if company.IsSanctioned && company.SanctionOrigin != sanctionOriginSSS {
 		log.WithFields(f).Warnf("company has non-SSS sanction block (origin=%q), blocking without SSS call", company.SanctionOrigin)
 		return true, nil
 	}
 
 	cacheKey := s.complianceCacheKey(company)
 	if cached, ok := s.getComplianceCache(cacheKey); ok {
 		log.WithFields(f).Debugf("using cached compliance result for organization/company: %s", cacheKey)
+		// Mirror the cached decision onto the loaded model so downstream gates in this
+		// request stay consistent. Manual/admin blocks already short-circuited above, so a
+		// cached-clean result here cannot be masking such a block.
+		s.applyComplianceToModel(company, cached.sanctioned)
 		return cached.sanctioned, nil
 	}
 
@@ -3091,40 +3096,55 @@ func (s *service) checkCompanyCompliance(ctx context.Context, company *v1Models.
 		return false, fmt.Errorf("checkCompanyCompliance: unexpected SSS status %q for company %s (required mode blocks on ambiguous results)", result.Status, company.CompanyID)
 	}
 
-	// Persist result: set origin="sss" on flagged; conditionally clear on clean (only if sss-origin).
+	// Persist result and reflect it on the in-memory model so downstream gates in this
+	// same request (e.g. ProcessEmployeeSignature) see the just-updated state instead of
+	// the stale value loaded before this check ran.
 	if sanctioned {
 		log.WithFields(f).Warnf("SSS returned flagged status for company %s, persisting sanction with origin=sss", company.CompanyID)
-		if persistErr := s.companyRepo.UpdateCompanySanctionStatus(ctx, company.CompanyID, true, "sss"); persistErr != nil {
+		if persistErr := s.companyRepo.UpdateCompanySanctionStatus(ctx, company.CompanyID, true, sanctionOriginSSS); persistErr != nil {
 			log.WithFields(f).WithError(persistErr).Warnf("failed to persist sanction status for company %s", company.CompanyID)
 			return false, fmt.Errorf("failed to persist sanction status for company %s: %w", company.CompanyID, persistErr)
 		}
+		// Flagged always blocks downstream, so reflecting it is safe even if a concurrent
+		// manual/admin block now owns the persisted record (that also blocks).
+		s.applyComplianceToModel(company, true)
 	} else {
 		// Clear only when previously set by SSS; manual blocks are left untouched.
-		// ClearCompanySanctionStatusIfSSS treats a conditional-check failure (manual
-		// block / nothing to clear) as a no-op, so a non-nil error here is a real
-		// persistence failure. In required mode we fail closed rather than allow with a
-		// stale persisted sanction still in place.
+		// ClearCompanySanctionStatusIfSSS reports whether it actually cleared; a non-nil
+		// error is a real persistence failure (not a benign conditional-check no-op), so in
+		// required mode we fail closed rather than allow with a stale persisted sanction.
 		log.WithFields(f).Debugf("SSS returned clean status for company %s; attempting conditional clear", company.CompanyID)
-		if clearErr := s.companyRepo.ClearCompanySanctionStatusIfSSS(ctx, company.CompanyID); clearErr != nil {
+		cleared, clearErr := s.companyRepo.ClearCompanySanctionStatusIfSSS(ctx, company.CompanyID)
+		if clearErr != nil {
 			log.WithFields(f).WithError(clearErr).Warnf("failed to conditionally clear sanction status for company %s", company.CompanyID)
 			if s.sssRequired {
 				return false, fmt.Errorf("checkCompanyCompliance: SSS returned clean but clearing the persisted sanction failed for company %s: %w", company.CompanyID, clearErr)
 			}
 		}
+		// Only mirror the clear on the in-memory model when it actually applied. If the
+		// conditional did not match — e.g. a concurrent manual/admin block now owns the
+		// record — leave the loaded state so downstream gates still honor that block.
+		if cleared {
+			s.applyComplianceToModel(company, false)
+		}
 	}
 
-	// Reflect the live SSS decision on the in-memory model so any downstream gate in
-	// this same request (e.g. ProcessEmployeeSignature) sees the just-updated state
-	// rather than the now-stale value that was loaded before this check ran.
+	s.setComplianceCache(cacheKey, sanctioned)
+	return sanctioned, nil
+}
+
+// applyComplianceToModel updates the in-memory company model to reflect a compliance
+// decision so downstream gates in the same request stay consistent with this check.
+func (s *service) applyComplianceToModel(company *v1Models.Company, sanctioned bool) {
+	if company == nil {
+		return
+	}
 	company.IsSanctioned = sanctioned
 	if sanctioned {
-		company.SanctionOrigin = "sss"
+		company.SanctionOrigin = sanctionOriginSSS
 	} else {
 		company.SanctionOrigin = ""
 	}
-
-	s.setComplianceCache(cacheKey, sanctioned)
-	return sanctioned, nil
 }
 
 func (s *service) complianceCacheKey(company *v1Models.Company) string {