Skip to content

Resolve PR #351 review comments: fix healthServer, remove Watch stub - #2

Closed
Stx666Michael with Copilot wants to merge 2 commits into
fix_vulnerabilitiesfrom
copilot/resolve-pr-comments-351
Closed

Resolve PR #351 review comments: fix healthServer, remove Watch stub#2
Stx666Michael with Copilot wants to merge 2 commits into
fix_vulnerabilitiesfrom
copilot/resolve-pr-comments-351

Conversation

Copilot AI commented Apr 29, 2026

Copy link
Copy Markdown

This PR resolves the review comments from PR #351. Below is each comment and the corresponding file change that addresses it.


Comment 1 — flair/rpc/rpc.go line 76

"What you should do is embed hpb.UnimplementedHealthServer into the server struct, so that we automatically get stub methods for the endpoints we don't intend to implement."

Resolution: hpb.UnimplementedHealthServer is now embedded in the healthServer struct (instead of using health.NewServer()), so all unimplemented gRPC health interface methods (Watch, List) are provided automatically. The existing custom Check() logic is preserved.

// flair/rpc/rpc.go
type healthServer struct {
    hpb.UnimplementedHealthServer  // provides Watch, List stubs automatically
    replicator, assetReplicator, exeReplicator *trie.Replicator
}

Comment 2 — flair/rpc/rpc.go line 81

"Why are you adding this new struct instead of continuing to use the existing server struct? You should be fine to embed multiple UnimplementedWhateverServers, so long as their method names don't conflict. If they do conflict, please add a comment to explain that."

Resolution: Attempted to embed hpb.UnimplementedHealthServer directly into server. This is not viable — ppb.UnimplementedGCServer (already embedded in server) and hpb.UnimplementedHealthServer both define a List method with incompatible signatures, causing a compile-time conflict. A comment has been added to healthServer explaining this:

// flair/rpc/rpc.go
// healthServer is a separate struct (rather than embedding hpb.UnimplementedHealthServer into server)
// because ppb.UnimplementedGCServer and hpb.UnimplementedHealthServer both define a List method with
// different signatures, which would cause a compile-time conflict.
type healthServer struct {
    hpb.UnimplementedHealthServer
    ...
}

Comment 3 — flair/rpc/rpc.go line 99

"Watch can be removed, since UnimplementedHealthServer will provide an equivalent method"

Resolution: The explicit Watch method has been removed from healthServer. hpb.UnimplementedHealthServer (now embedded) provides the required stub automatically.

-func (s *healthServer) Watch(*hpb.HealthCheckRequest, hpb.Health_WatchServer) error {
-    return status.Errorf(codes.Unimplemented, "grpc_health_v1.Watch not implemented")
-}

Comment 4 — third_party/go/BUILD line 8

"One for a later PR, but we should get this to 1.26.2"

Not addressed here — reviewer noted this is for a later PR. The toolchain is currently at 1.24.0 (upgraded from 1.23.2 to satisfy grpc v1.79.3's requirements).

Copilot AI and others added 2 commits April 29, 2026 13:05
…ealthServer struct

Agent-Logs-Url: https://github.com/scyyx5/please-servers/sessions/e32f04e3-02cc-477b-b46f-dfa9b0195ba7

Co-authored-by: Stx666Michael <65939058+Stx666Michael@users.noreply.github.com>
…ethod conflict

Agent-Logs-Url: https://github.com/scyyx5/please-servers/sessions/40038edb-d554-4be5-a804-b14739626325

Co-authored-by: Stx666Michael <65939058+Stx666Michael@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants