chore(lint): fixes by mschfh · Pull Request #395 · brave/go-sync

mschfh · 2026-02-24T19:17:55Z

No description provided.

Copilot

Pull request overview

This PR applies linting fixes to address gosec security warnings and formatting issues throughout the codebase.

Changes:

Added gosec nolint directives with explanatory comments for intentional security suppressions (pprof endpoints, error handling in shutdown)
Changed maxSize parameter type from int64 to int32 in GetUpdatesForType method across all interfaces, implementations, mocks, and call sites to match AWS DynamoDB SDK requirements
Applied multi-line formatting to function calls and definitions with many parameters for improved readability

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
server/server.go	Added gosec nolint comments for pprof import, server shutdown, and pprof listener
datastore/sync_entity_test.go	Reformatted GetUpdatesForType test calls to multi-line format
datastore/sync_entity.go	Changed maxSize parameter from int64 to int32; reformatted long function calls
datastore/item_count_test.go	Reformatted ClientItemCounts struct initialization
datastore/item_count.go	Reformatted long expression and UpdateClientItemCount signature
datastore/instrumented_datastore.go	Updated GetUpdatesForType signature to use int32 for maxSize
datastore/datastoretest/mock_datastore.go	Updated mock method signatures to use int32; applied multi-line formatting
datastore/datastore.go	Updated interface signatures for GetUpdatesForType and UpdateClientItemCount
command/server_defined_unique_entity.go	Reformatted createServerDefinedUniqueEntity signature
command/command_test.go	Reformatted NewPBSyncAttrs signature and long function calls
command/command.go	Changed maxGUBatchSize to int32; added gosec nolint for safe conversion; reformatted signatures
cache/cache.go	Reformatted IncrementInterimCount signature
auth/auth.go	Added gosec nolint for false positive on regex pattern

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

command/command.go

server/server.go

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-31T09:38:10Z

datastore/datastore.go

+		clientToken int64,
+		fetchFolders bool,
+		clientID string,
+		maxSize int32,


Changing the Datastore interface signature from maxSize int64 to maxSize int32 is a breaking API change for any external implementers/callers of this module. If the goal is only to satisfy AWS SDK types, consider keeping int64 in the public interface and converting/validating internally (or introduce a new method) to avoid an unintended breaking change.

Suggested change

maxSize int32,

maxSize int64,

command/command.go

server/server.go

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-31T17:27:44Z

command/command.go

-	maxGUBatchSize              = 500
-	maxClientObjectQuota        = 50000
-	maxClientHistoryObjectQuota = 30000
+	maxGUBatchSize              int32 = 500


Changing maxGUBatchSize to int32 makes maxSize an int32, but later in this function maxSize is used as the slice capacity (guRsp.Entries = make(..., 0, maxSize)), which won’t compile because make() requires an int capacity. Either keep maxGUBatchSize/maxSize as int, or convert to int at the make() call (and any other places that require an int).

Suggested change

maxGUBatchSize int32 = 500

maxGUBatchSize int = 500

Copilot · 2026-03-31T17:27:44Z

datastore/datastore.go

+		clientToken int64,
+		fetchFolders bool,
+		clientID string,
+		maxSize int32,


GetUpdatesForType’s maxSize parameter was changed from int64 to int32, which is a breaking change to the exported Datastore interface (and the API surface of this module) even though the PR is labeled as a lint chore. If the goal is only to satisfy DynamoDB Limit’s int32 type, consider keeping the interface parameter as int/int64 and performing a checked conversion at the DynamoDB boundary (returning an error if out of range), to avoid forcing all downstream implementations/callers to change.

Suggested change

maxSize int32,

maxSize int64,

Copilot · 2026-03-31T17:27:44Z

server/server.go

+		//nolint:errcheck,gosec // Error during shutdown in signal handler is acceptable
 		srv.Shutdown(serverCtx)


srv.Shutdown(serverCtx) error is intentionally ignored here, but suppressing both errcheck and gosec removes any visibility into failed graceful shutdowns (e.g., context deadline exceeded). Consider capturing and logging the returned error instead of dropping it, even in the signal handler, so operational issues are diagnosable.

Suggested change

//nolint:errcheck,gosec // Error during shutdown in signal handler is acceptable

srv.Shutdown(serverCtx)

if err := srv.Shutdown(serverCtx); err != nil {

log.Err(err).Msg("HTTP server graceful shutdown failed")

}

Copilot · 2026-03-31T17:27:45Z

server/server.go

 		// host:6061/debug/pprof/
 		go func() {
+			//nolint:gosec // G114: pprof diagnostic server
 			if err := http.ListenAndServe(":6061", http.DefaultServeMux); err != nil {


The pprof listener is started with http.ListenAndServe(":6061", ...) (binds all interfaces) and has no server timeouts; adding a //nolint:gosec suppresses G114, but it doesn’t mitigate the underlying exposure/DoS risk if PPROF_ENABLED is enabled accidentally or in the wrong environment. Prefer binding to localhost (127.0.0.1:6061) and using an http.Server with reasonable timeouts (ReadHeaderTimeout/IdleTimeout/etc.), or otherwise restricting access at the network layer and documenting that requirement.

Suggested change

// host:6061/debug/pprof/

go func() {

//nolint:gosec // G114: pprof diagnostic server

if err := http.ListenAndServe(":6061", http.DefaultServeMux); err != nil {

// 127.0.0.1:6061/debug/pprof/

go func() {

pprofSrv := &http.Server{

Addr: "127.0.0.1:6061",

Handler: http.DefaultServeMux,

ReadHeaderTimeout: 5 * time.Second,

IdleTimeout: 60 * time.Second,

}

if err := pprofSrv.ListenAndServe(); err != nil {

DJAndries · 2026-03-31T18:24:35Z

command/command.go

-	maxGUBatchSize              = 500
-	maxClientObjectQuota        = 50000
-	maxClientHistoryObjectQuota = 30000
+	maxGUBatchSize              int32 = 500


I agree with Copilot's comment here. I suspect the "won't compile" part isn't correct because the checks pass, but it would be good to use int anyway for simplicity.

DJAndries · 2026-03-31T18:26:32Z

command/command.go

 		// FromProgressMarker, where the returned token stays the same as the one
 		// passed in FromProgressMarker.
-		if len(guRsp.Entries) >= maxSize {
+		if len(guRsp.Entries) >= int(maxSize) {


if maxGUBatchSize becomes int, then we don't need a cast

DJAndries · 2026-03-31T18:27:21Z

command/command.go


-		curMaxSize := int64(maxSize) - int64(len(guRsp.Entries))
-		hasChangesRemaining, entities, err := db.GetUpdatesForType(ctx, int(*fromProgressMarker.DataTypeId), token, fetchFolders, clientID, curMaxSize)
+		curMaxSize := int32(int(maxSize) - len(guRsp.Entries)) //nolint:gosec // maxSize is 500 and the >= check above ensures entries fits within it


It would be nice if we could simplify to maxSize - len(guRsp.Entries), making curMaxSize an int

DJAndries · 2026-03-31T18:29:20Z

datastore/datastore.go

+		clientToken int64,
+		fetchFolders bool,
+		clientID string,
+		maxSize int32,


maxSize could be int

DJAndries · 2026-03-31T18:32:07Z

datastore/sync_entity.go

 		ProjectionExpression:      aws.String(projPk),
 		TableName:                 aws.String(Table),
-		Limit:                     aws.Int32(int32(maxSize)),
+		Limit:                     aws.Int32(maxSize),


changing the maxSize param type to int, and keeping the int32 cast here will help remove the convoluted casting near the call site of this function.

mschfh requested a review from DJAndries February 24, 2026 19:17

mschfh self-assigned this Feb 24, 2026

mschfh requested a review from Copilot February 24, 2026 19:35

Copilot started reviewing on behalf of mschfh February 24, 2026 19:36 View session

Copilot AI reviewed Feb 24, 2026

View reviewed changes

mschfh marked this pull request as ready for review March 17, 2026 19:08

mihaiplesa requested a review from Copilot March 17, 2026 19:09

Copilot started reviewing on behalf of mihaiplesa March 17, 2026 19:09 View session

Copilot AI reviewed Mar 17, 2026

View reviewed changes

command/command.go Show resolved Hide resolved

server/server.go Show resolved Hide resolved

server/server.go Show resolved Hide resolved

mihaiplesa requested a review from Copilot March 31, 2026 09:33

Copilot started reviewing on behalf of mihaiplesa March 31, 2026 09:34 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

mschfh added 3 commits March 31, 2026 07:00

chore(lint): apply golines formatting

7fd1a09

chore(lint): fix gosec findings

60e878a

chore(lint): fix gosec findings

fc49664

mschfh force-pushed the mschfh-lint-fixes branch from 6053304 to fc49664 Compare March 31, 2026 12:00

mihaiplesa requested a review from Copilot March 31, 2026 17:23

Copilot started reviewing on behalf of mihaiplesa March 31, 2026 17:24 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

DJAndries reviewed Mar 31, 2026

View reviewed changes

		//nolint:errcheck,gosec // Error during shutdown in signal handler is acceptable
		srv.Shutdown(serverCtx)

-		// host:6061/debug/pprof/
-		go func() {
-			//nolint:gosec // G114: pprof diagnostic server
-			if err := http.ListenAndServe(":6061", http.DefaultServeMux); err != nil {
+		// 127.0.0.1:6061/debug/pprof/
+		go func() {
+			pprofSrv := &http.Server{
+				Addr:              "127.0.0.1:6061",
+				Handler:           http.DefaultServeMux,
+				ReadHeaderTimeout: 5 * time.Second,
+				IdleTimeout:       60 * time.Second,
+			}
+			if err := pprofSrv.ListenAndServe(); err != nil {

Conversation

mschfh commented Feb 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

DJAndries Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

DJAndries Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

DJAndries Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

DJAndries Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

DJAndries Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants