Repository navigation
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a streaming Chat RPC and protobuf types, integrates llama.cpp as a submodule with a Go cgo wrapper for model loading/caching and token streaming, implements InferenceService.Predict, rewires ChatService to call the inference gRPC (in-process via Unix socket), and updates CLI/client stubs for streaming. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant FE as Frontend
participant CS as ChatService (gRPC)
participant IS as InferenceService (gRPC, Unix socket)
participant LL as llamacpp Wrapper
FE->>CS: Chat(ChatRequest)
note right of CS #efe8ff: Construct Inference ChatRequest
CS->>IS: Chat(ChatRequest) [server-stream]
IS->>LL: Predict(model, text) → chan<string>
loop stream tokens
LL-->>IS: token
IS-->>CS: ChatResponse{text: token}
CS-->>FE: ChatResponse{text: token}
end
IS-->>CS: stream EOF
CS-->>FE: stream EOF
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @xask00, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the application's AI capabilities by integrating local LLM support via llamacpp. It involves a substantial rearchitecture of the chat system, moving the core processing to a dedicated inference service, and optimizing internal communication for improved performance. The changes lay the groundwork for more advanced AI features, including potential RAG capabilities.
Highlights
- Local Model Integration: Integrated the
llamacpplibrary as a Git submodule and provided a Go wrapper (llamacpp.go) to enable running large language models (LLMs) locally within the application. This allows for offline inference capabilities. - Chat Functionality Rearchitecture: The core chat logic has been shifted from the
chatserviceto theinferenceservice. Thechatservicenow acts as a gRPC client, forwarding chat requests to theinferenceserviceand streaming back responses. - Enhanced Inference Service API: Introduced new protobuf definitions and gRPC methods within the
inferenceserviceto support chat requests, streaming responses, and include structures for project context, RAG (Retrieval Augmented Generation) document references, and detailed response summaries. - Optimized Inter-Service Communication: Implemented Unix domain sockets for efficient, in-process gRPC communication between the
chatserviceandinferenceservicein the monolithic backend, reducing overhead for local model inference.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds support for local models using llama.cpp, which is a significant and valuable feature. The implementation includes adding llama.cpp as a submodule, creating Go bindings, and integrating it into the inferenceservice. The chatservice is then updated to leverage this new capability. While the overall approach is sound, I've identified several issues that need attention. These include a critical issue with a hardcoded absolute path, high-severity problems related to error handling and protobuf message duplication, and several medium-severity concerns such as debug logging and typos. Please see the detailed comments for suggestions on how to address these points.
| // if err != nil { | ||
| // return err | ||
| // } |
There was a problem hiding this comment.
The authentication error handling is commented out. This is a critical security issue as it would allow unauthenticated requests to proceed. This check must be enabled to secure the endpoint.
| // if err != nil { | |
| // return err | |
| // } | |
| if err != nil { | |
| return err | |
| } |
| "unsafe" | ||
| ) | ||
|
|
||
| const modelDir = "/home/xask/projects/sortedstartup/experiments/llamacpp/llama.cpp/" |
There was a problem hiding this comment.
The modelDir constant is hardcoded to an absolute path on a local machine. This is a critical issue that will prevent the application from running in any other environment. This path should be made configurable, for instance, through an environment variable or a configuration file.
| if err != nil { | ||
| // If streaming fails, break the loop | ||
| break | ||
| } |
There was a problem hiding this comment.
If sending a message to the stream fails, the loop breaks and the function returns nil. This incorrectly signals a successful end of the stream to the client. The error should be propagated so the client is aware of the failure.
if err != nil {
// If streaming fails, log the error and return it.
slog.Error("failed to send token to stream", "error", err)
return err
}| nPrompt := int(-C.llama_tokenize(model.vocab, cPrompt, C.int32_t(len(text)), nil, 0, C.bool(true), C.bool(true))) | ||
| if nPrompt <= 0 { | ||
| output <- fmt.Sprintf("Error: failed to tokenize prompt") | ||
| return |
There was a problem hiding this comment.
Errors within this goroutine are sent as formatted strings over the data channel. This is problematic because it mixes data and error conditions, making it difficult for the consumer to handle errors reliably. A better pattern would be to use a separate error channel to signal failures, or to have the Predict function return a struct that can contain either a token or an error.
| inferenceConn, err := grpc.NewClient("unix:///tmp/inference-service.sock", | ||
| grpc.WithContextDialer(unixDialer), | ||
| grpc.WithTransportCredentials(insecure.NewCredentials())) |
There was a problem hiding this comment.
The Unix socket path is hardcoded in the grpc.NewClient call. This path is already defined in the socketPath variable on line 173. Using the variable instead of a hardcoded string will prevent potential inconsistencies and make the code easier to maintain.
| inferenceConn, err := grpc.NewClient("unix:///tmp/inference-service.sock", | |
| grpc.WithContextDialer(unixDialer), | |
| grpc.WithTransportCredentials(insecure.NewCredentials())) | |
| inferenceConn, err := grpc.NewClient("unix://"+socketPath, | |
| grpc.WithContextDialer(unixDialer), | |
| grpc.WithTransportCredentials(insecure.NewCredentials())) |
| message ChatRequest { | ||
| string text = 1; | ||
| string chatId = 2; | ||
| string model = 3; | ||
| ProjectContext project_context = 4; | ||
| } | ||
|
|
||
| message ProjectContext { | ||
| string project_id = 1; | ||
| bool rag_enabled = 2; | ||
| } | ||
|
|
||
| message ChatResponse { | ||
| oneof response { | ||
| string text = 1; | ||
| // was introduced to send assistant message id, because it is generated server side | ||
| // sending one of the multiple oneof's help us do all of this in one streaming API call | ||
| ResponseSummary summary = 2; | ||
| // message id of user message(question to llm) | ||
| string request_message_id = 3; | ||
| RAGDocumentReferenceSummaryList document_reference = 4; | ||
| ChatInfo chat_metadata = 5; | ||
| } | ||
| } | ||
|
|
||
| // TODO: reasonable limits from server side, limit to 10 (constant in server side) docs | ||
| message RAGDocumentReferenceSummaryList { | ||
| repeated Summary summary = 1; | ||
| message Summary { | ||
| string doc_id = 1; | ||
| string file_name = 2; | ||
| int32 chunkCount = 3; | ||
| } | ||
| } | ||
|
|
||
| message RAGDocumentReference { | ||
| string doc_id = 1; | ||
| string file_name = 2; | ||
| repeated Chunk Chunks = 3; | ||
| message Chunk { | ||
| string chunk_text = 3; | ||
| int32 start_byte = 4; | ||
| int32 end_byte = 5; | ||
| float simillarity = 6; | ||
| } | ||
| } | ||
|
|
||
| message ResponseSummary { | ||
| string message_id = 1; | ||
| string model = 2; | ||
| int32 input_tokens = 3; | ||
| int32 output_tokens = 4; | ||
| int32 cached_tokens = 5; | ||
| float cost = 6; | ||
| } | ||
|
|
||
| //total chat cost/token | ||
| message ChatInfo { | ||
| string chatId = 1; | ||
| string name = 2; | ||
| float cost = 3; | ||
| int32 input_token_count = 4; | ||
| int32 output_token_count = 5; | ||
| int32 cached_token_count = 6; | ||
| } |
There was a problem hiding this comment.
There is significant message definition duplication between this file and chatservice.proto. This will lead to duplicated generated code and increased maintenance overhead. It would be better to move these common message definitions to a shared .proto file and import it where needed.
| fmt.Println("userID", userID) | ||
|
|
||
| // Populate inference request with actual chat data | ||
| infereceChatRequest := &inferenceProto.ChatRequest{ | ||
| Text: req.GetText(), | ||
| ChatId: req.GetChatId(), | ||
| Model: req.GetModel(), | ||
| } | ||
|
|
||
| fmt.Printf("calling inference client with text: %s model: %s\n", req.GetText(), req.GetModel()) | ||
| x, err := s.inferenceClient.Chat(stream.Context(), infereceChatRequest) | ||
| if err != nil { | ||
| slog.Error("error ", err) | ||
| return err | ||
| } | ||
|
|
||
| for { | ||
| response, err := x.Recv() | ||
| if err != nil { | ||
| // Handle EOF - this means the stream has ended normally | ||
| if err == io.EOF { | ||
| return nil // Return nil to stop the stream gracefully | ||
| } | ||
| // Handle other errors | ||
| return err | ||
| } | ||
|
|
||
| // Create and send chat response | ||
| chatResponse := &pb.ChatResponse{ | ||
| Response: &pb.ChatResponse_Text{ | ||
| Text: response.GetText(), | ||
| }, | ||
| } | ||
|
|
||
| if err := stream.Send(chatResponse); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| // return s.service.Chat(stream.Context(), userID, req, func(response *pb.ChatResponse) error { | ||
| // return stream.Send(response) | ||
| // }) |
There was a problem hiding this comment.
This function contains some leftover debugging code and commented-out logic that should be cleaned up before merging.
- The
fmt.Printlnandfmt.Printfcalls on lines 90 and 99 should be removed or replaced with structured logging (e.g.,slog.Debug). - The old service call on lines 129-131 is commented out and should be removed.
| string chunk_text = 3; | ||
| int32 start_byte = 4; | ||
| int32 end_byte = 5; | ||
| float simillarity = 6; |
There was a problem hiding this comment.
There is a typo in the field name simillarity. It should be similarity.
| float simillarity = 6; | |
| float similarity = 6; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (10)
.gitmodules (1)
3-3: Supply-chain sanity: mirror/pin policy and license check
- Consider mirroring ggml-org/llama.cpp to your org and referencing that URL to reduce upstream deletion/force-push risk.
- Confirm license compatibility and document the update cadence (who bumps the submodule and how it’s tested).
Would you like me to scan workflows for a submodule update job and check the upstream license/readme? I can propose a lightweight policy doc and a GH Action to open PRs on upstream updates.
backend/inferenceservice/llamacpp/llamacpp.go (3)
4-8: Build portability: CGO flags and backend loading.Hardcoded -fopenmp/-lstdc++ and static ggml libs will fail on macOS/Windows and non-OpenMP builds; also calling ggml_backend_load_all() per-load is unnecessary.
- Add platform-specific build tags or use dynamic backends conditionally.
- Move ggml_backend_load_all() into an init() run-once.
I can draft OS-conditional cgo blocks if desired.Also applies to: 35-45
208-216: Consider not echoing prompt back in the stream.Most chat UIs don’t expect the prompt echoed token-by-token, and it complicates client rendering.
If you want configurable echoing, add a flag in ChatRequest (e.g., echo_prompt).
243-249: Token buffer may truncate multi-byte pieces.llama_token_to_piece can exceed 128 bytes for some scripts; handle resize when n == len(buf) or negative.
I can provide a small helper to grow the buffer as needed.
proto/inferenceservice.proto (1)
72-79: Use double for monetary “cost”.float32 may lose precision over session aggregates.
Change cost fields to double; adjust TS/Go types on regen.
Also applies to: 81-89
backend/inferenceservice/api/api.go (1)
5-13: Avoid printing prompts/user data with fmt; use slog at debug level.Minimize PII in logs; use structured logging.
Example:
-import "fmt" +import "fmt" +// consider: "log/slog" - fmt.Printf("model name %s, text %s\n", modelName, text) + // slog.Debug("inference request", "model", modelName, "chars", len(text))Also applies to: 171-172
backend/inferenceservice/service/llama.go (1)
17-26: Follow-up: stop constructing ctx/sampler in LoadModel.Once per-request ctx/sampler is in place, simplify Model to cache only llama_model/vocab; free memory and avoid accidental sharing.
I can prepare a safe refactor PR-sized diff if you want it in this one.
backend/mono/main.go (2)
197-200: Respect context cancellation in the Unix dialer.Use Dialer.DialContext so client cancellation/deadlines propagate to the socket dial.
-unixDialer := func(ctx context.Context, address string) (net.Conn, error) { - return net.Dial("unix", socketPath) -} +dialer := net.Dialer{} +unixDialer := func(ctx context.Context, address string) (net.Conn, error) { + return dialer.DialContext(ctx, "unix", socketPath) +}
35-38: Spelling: rename infereceProto → inferenceProto for consistency.Pure readability/naming nit; reduces confusion across files.
- infereceProto "sortedstartup/inferenceservice/proto" + inferenceProto "sortedstartup/inferenceservice/proto" @@ - infereceProto.RegisterInferenceServiceServer(grpcServer, inferenceServiceApi) + // inferenceProto.RegisterInferenceServiceServer(grpcServer, inferenceServiceApi) // (removed per earlier comment) @@ - infereceProto.RegisterInferenceServiceServer(clientGrpcServer, inferenceServiceApi) + inferenceProto.RegisterInferenceServiceServer(clientGrpcServer, inferenceServiceApi) @@ - inferenceClient := infereceProto.NewInferenceServiceClient(inferenceConn) + inferenceClient := inferenceProto.NewInferenceServiceClient(inferenceConn)Also applies to: 170-170, 188-188, 209-209
backend/chatservice/api/api.go (1)
90-91: Remove debug print of userID.Avoid stdout prints in RPC handlers; use structured logs if needed, but user identifiers are sensitive.
- fmt.Println("userID", userID) + // slog.Debug("chat user", "userID", userID) // avoid logging PII in production
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
backend/inferenceservice/proto/inferenceservice.pb.gois excluded by!**/*.pb.gobackend/inferenceservice/proto/inferenceservice_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
.gitmodules(1 hunks)backend/chatservice/api/api.go(5 hunks)backend/inferenceservice/api/api.go(2 hunks)backend/inferenceservice/llamacpp/llama.cpp(1 hunks)backend/inferenceservice/llamacpp/llamacpp.go(1 hunks)backend/inferenceservice/service/llama.go(1 hunks)backend/mono/main.go(3 hunks)frontend/proto/inferenceservice.ts(2 hunks)proto/inferenceservice.proto(4 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
backend/**
📄 CodeRabbit inference engine (.cursor/rules/project-description.mdc)
Backend code resides under backend/
Files:
backend/inferenceservice/llamacpp/llama.cppbackend/inferenceservice/service/llama.gobackend/inferenceservice/llamacpp/llamacpp.gobackend/mono/main.gobackend/inferenceservice/api/api.gobackend/chatservice/api/api.go
backend/!(mono)/**
📄 CodeRabbit inference engine (.cursor/rules/project-description.mdc)
Each microservice lives in its own directory under backend/ (excluding the monolith in backend/mono)
Files:
backend/inferenceservice/llamacpp/llama.cppbackend/inferenceservice/service/llama.gobackend/inferenceservice/llamacpp/llamacpp.gobackend/inferenceservice/api/api.gobackend/chatservice/api/api.go
backend/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/project-description.mdc)
Backend services are written in Go 1.24 using gRPC
Files:
backend/inferenceservice/service/llama.gobackend/inferenceservice/llamacpp/llamacpp.gobackend/mono/main.gobackend/inferenceservice/api/api.gobackend/chatservice/api/api.go
backend/mono/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/project-description.mdc)
The monolith is implemented under backend/mono and must be runnable via
go run ./monofrom backend/
Files:
backend/mono/main.go
proto/**/*.proto
📄 CodeRabbit inference engine (.cursor/rules/project-description.mdc)
proto/**/*.proto: All protobuf definitions are stored under proto/
gRPC service contracts are defined via protobuf files
Files:
proto/inferenceservice.proto
proto/*.proto
📄 CodeRabbit inference engine (.cursor/rules/project-description.mdc)
Each microservice has a corresponding proto file in proto/
Files:
proto/inferenceservice.proto
frontend/**
📄 CodeRabbit inference engine (.cursor/rules/project-description.mdc)
Frontend code resides under frontend/
Files:
frontend/proto/inferenceservice.ts
frontend/proto/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/project-description.mdc)
Generated JS/TS protobuf/grpc code is placed under frontend/proto/
Files:
frontend/proto/inferenceservice.ts
frontend/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-description.mdc)
Frontend is implemented with React and Tailwind CSS
Files:
frontend/proto/inferenceservice.ts
🧠 Learnings (4)
📚 Learning: 2025-08-27T17:27:06.967Z
Learnt from: CR
PR: sortedstartup/sortedchat#0
File: .cursor/rules/project-description.mdc:0-0
Timestamp: 2025-08-27T17:27:06.967Z
Learning: Applies to backend/**/*.go : Backend services are written in Go 1.24 using gRPC
Applied to files:
backend/mono/main.gobackend/chatservice/api/api.go
📚 Learning: 2025-08-27T17:27:06.967Z
Learnt from: CR
PR: sortedstartup/sortedchat#0
File: .cursor/rules/project-description.mdc:0-0
Timestamp: 2025-08-27T17:27:06.967Z
Learning: Applies to backend/*/proto/**/*.go : Generated Go code from protobufs for each service is placed under backend/<service>/proto
Applied to files:
backend/chatservice/api/api.goproto/inferenceservice.proto
📚 Learning: 2025-08-27T17:27:06.967Z
Learnt from: CR
PR: sortedstartup/sortedchat#0
File: .cursor/rules/project-description.mdc:0-0
Timestamp: 2025-08-27T17:27:06.967Z
Learning: Applies to proto/**/*.proto : gRPC service contracts are defined via protobuf files
Applied to files:
backend/chatservice/api/api.goproto/inferenceservice.proto
📚 Learning: 2025-08-27T17:27:06.967Z
Learnt from: CR
PR: sortedstartup/sortedchat#0
File: .cursor/rules/project-description.mdc:0-0
Timestamp: 2025-08-27T17:27:06.967Z
Learning: Applies to proto/*.proto : Each microservice has a corresponding proto file in proto/
Applied to files:
backend/chatservice/api/api.goproto/inferenceservice.proto
🧬 Code graph analysis (5)
backend/inferenceservice/service/llama.go (2)
backend/inferenceservice/service/service.go (1)
InferenceService(19-23)backend/inferenceservice/llamacpp/llamacpp.go (2)
Predict(167-257)LoadModelByName(113-142)
backend/mono/main.go (3)
backend/inferenceservice/api/api.go (1)
NewInferenceServiceAPI(23-30)backend/inferenceservice/proto/inferenceservice_grpc.pb.go (2)
RegisterInferenceServiceServer(160-169)NewInferenceServiceClient(44-46)backend/chatservice/api/api.go (1)
NewChatService(66-83)
backend/inferenceservice/api/api.go (3)
backend/inferenceservice/proto/inferenceservice.pb.go (11)
ChatRequest(83-91)ChatRequest(104-104)ChatRequest(119-121)ChatResponse(203-215)ChatResponse(228-228)ChatResponse(243-245)Model(678-693)Model(706-706)Model(721-723)ChatResponse_Text(303-305)ChatResponse_Text(326-326)backend/common/auth/jwt.go (1)
GetUserIDFromContext_WithError(140-146)backend/inferenceservice/llamacpp/llamacpp.go (2)
Model(27-32)Predict(167-257)
backend/chatservice/api/api.go (3)
backend/inferenceservice/proto/inferenceservice_grpc.pb.go (1)
InferenceServiceClient(32-38)backend/inferenceservice/proto/inferenceservice.pb.go (11)
ChatRequest(83-91)ChatRequest(104-104)ChatRequest(119-121)Model(678-693)Model(706-706)Model(721-723)ChatResponse(203-215)ChatResponse(228-228)ChatResponse(243-245)ChatResponse_Text(303-305)ChatResponse_Text(326-326)backend/chatservice/proto/chatservice.pb.go (8)
ChatRequest(986-994)ChatRequest(1007-1007)ChatRequest(1022-1024)ChatResponse(1054-1066)ChatResponse(1079-1079)ChatResponse(1094-1096)ChatResponse_Text(1154-1156)ChatResponse_Text(1176-1176)
frontend/proto/inferenceservice.ts (2)
backend/inferenceservice/proto/inferenceservice.pb.go (45)
ChatRequest(83-91)ChatRequest(104-104)ChatRequest(119-121)ProjectContext(151-157)ProjectContext(170-170)ProjectContext(185-187)ChatResponse(203-215)ChatResponse(228-228)ChatResponse(243-245)ResponseSummary(441-451)ResponseSummary(464-464)ResponseSummary(479-481)RAGDocumentReferenceSummaryList(337-342)RAGDocumentReferenceSummaryList(355-355)RAGDocumentReferenceSummaryList(370-372)ChatInfo(526-536)ChatInfo(549-549)ChatInfo(564-566)RAGDocumentReference(381-388)RAGDocumentReference(401-401)RAGDocumentReference(416-418)DownloadModelRequest(802-807)DownloadModelRequest(820-820)DownloadModelRequest(835-837)DownloadModelResponse(846-851)DownloadModelResponse(864-864)DownloadModelResponse(879-881)GetLLMModelsRequest(890-894)GetLLMModelsRequest(907-907)GetLLMModelsRequest(922-924)GetLLMModelsResponse(926-931)GetLLMModelsResponse(944-944)GetLLMModelsResponse(959-961)CancelDownloadRequest(970-975)CancelDownloadRequest(988-988)CancelDownloadRequest(1003-1005)CancelDownloadResponse(1014-1019)CancelDownloadResponse(1032-1032)CancelDownloadResponse(1047-1049)DeleteModelRequest(1058-1063)DeleteModelRequest(1076-1076)DeleteModelRequest(1091-1093)DeleteModelResponse(1102-1107)DeleteModelResponse(1120-1120)DeleteModelResponse(1135-1137)backend/inferenceservice/proto/inferenceservice_grpc.pb.go (1)
InferenceServiceClient(32-38)
🪛 Buf (1.55.1)
proto/inferenceservice.proto
3-3: Files with package "sortedchat.inferenceservice" must be within a directory "sortedchat/inferenceservice" relative to root but were in directory "proto".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Go backend (darwin, arm64, macos-latest)
- GitHub Check: Build Go backend (linux, amd64, ubuntu-latest)
- GitHub Check: Build Go backend (darwin, amd64, macos-latest)
- GitHub Check: Build Go backend (windows, amd64, ubuntu-latest)
- GitHub Check: Build Go backend (linux, arm64, ubuntu-latest)
🔇 Additional comments (5)
frontend/proto/inferenceservice.ts (2)
2207-2210: Client Chat streaming wiring looks correct.Method path matches proto (/sortedchat.inferenceservice.InferenceService/Chat). No issues spotted.
751-800: Fix proto: renamesimillarity→similarityand correctChunkscasing, then regenerate protos
- frontend/proto/inferenceservice.ts (lines 751–800) contains the propagated typo
simillarityand incorrectChunkscasing — fix these in the .proto.- Regenerate artifacts locally/CI (sandbox run failed:
protoc: command not found):# from repo root protoc --go_out=backend/inferenceservice/proto --go-grpc_out=backend/inferenceservice/proto proto/**/*.proto protoc --ts_out frontend/proto proto/**/*.proto
- Verify frontend/proto/inferenceservice.ts no longer contains
simillarityand that generated identifiers use correct casing.backend/mono/main.go (1)
201-207: Keep grpc.NewClient (it's valid) — avoid hardcoded socket pathFile: backend/mono/main.go (lines 201–207)
- grpc.NewClient was added in gRPC-Go v1.63 and your module uses google.golang.org/grpc v1.72.1, so NewClient is valid; Dial/DialContext are deprecated. (pkg.go.dev)
- Replace the hardcoded "unix:///tmp/inference-service.sock" with a target built from socketPath (e.g. "unix://"+socketPath) and either (a) have your dialer use the passed address parameter or (b) use the passthrough resolver — this avoids inconsistent/address-mismatch bugs.
Likely an incorrect or invalid review comment.
backend/chatservice/api/api.go (2)
66-83: Constructor wiring looks good.NewChatService signature and client injection align with the proto/client changes.
106-127: Incorrect — inference layer only emits ChatResponse_Text.Search of backend/inferenceservice shows only ChatResponse_Text at backend/inferenceservice/api/api.go:181; no RequestMessageId/Summary/DocumentReference/ChatMetadata variants are emitted, so the current forwarding of Text is correct.
Likely an incorrect or invalid review comment.
| [submodule "backend/inferenceservice/llamacpp/llama.cpp"] | ||
| path = backend/inferenceservice/llamacpp/llama.cpp | ||
| url = https://github.com/ggml-org/llama.cpp.git |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Submodule added — add guardrails for CI and local build noise
- Ensure CI initializes submodules; otherwise builds will fail when llama.cpp isn’t present.
- llama.cpp builds often touch files inside the submodule; consider ignoring local dirt to reduce dev noise.
Apply this minimal .gitmodules tweak:
[submodule "backend/inferenceservice/llamacpp/llama.cpp"]
path = backend/inferenceservice/llamacpp/llama.cpp
url = https://github.com/ggml-org/llama.cpp.git
+ ignore = dirtyRun to verify CI and repo state:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Submodule status =="
git submodule status || true
echo "== Check GitHub Actions checkout uses submodules =="
rg -nP -C2 'uses:\s*actions/checkout@' .github/workflows \
| rg -nP -C1 'submodules:\s*(true|recursive|["'\'']recursive["'\''])' || {
echo "HINT: Add 'submodules: recursive' to actions/checkout." >&2
}
echo "== Detect dirty submodule worktrees =="
git submodule foreach 'git diff --quiet || { echo "DIRTY: $name"; exit 1; }' || trueLength of output: 254
Action required — ensure CI initializes llama.cpp submodule; add .gitmodules guardrail
- CI: .github/workflows does not appear to checkout submodules (script printed a hint). Add
submodules: recursivetoactions/checkoutor rungit submodule update --init --recursivebefore the build to avoid CI failures. - Local dev noise: add an ignore rule for the new submodule in .gitmodules.
Apply this minimal .gitmodules tweak:
[submodule "backend/inferenceservice/llamacpp/llama.cpp"]
path = backend/inferenceservice/llamacpp/llama.cpp
url = https://github.com/ggml-org/llama.cpp.git
+ ignore = dirtyVerification: git submodule status shows -2b3efea... backend/inferenceservice/llamacpp/llama.cpp (submodule present but not initialized); workflow check returned a hint to add submodules: recursive.
📝 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.
| [submodule "backend/inferenceservice/llamacpp/llama.cpp"] | |
| path = backend/inferenceservice/llamacpp/llama.cpp | |
| url = https://github.com/ggml-org/llama.cpp.git | |
| [submodule "backend/inferenceservice/llamacpp/llama.cpp"] | |
| path = backend/inferenceservice/llamacpp/llama.cpp | |
| url = https://github.com/ggml-org/llama.cpp.git | |
| ignore = dirty |
🤖 Prompt for AI Agents
In .gitmodules around lines 1-3, the repo adds the llama.cpp submodule but CI
won't initialize it and local devs can get noisy diffs; update CI to initialize
submodules (either add submodules: recursive to the actions/checkout step in the
relevant .github/workflows workflow or run git submodule update --init
--recursive before the build) and add an ignore rule to the .gitmodules entry
for this submodule (set ignore = all under the submodule section) so local
changes are ignored.
| infereceChatRequest := &inferenceProto.ChatRequest{ | ||
| Text: req.GetText(), | ||
| ChatId: req.GetChatId(), | ||
| Model: req.GetModel(), | ||
| } | ||
|
|
||
| fmt.Printf("calling inference client with text: %s model: %s\n", req.GetText(), req.GetModel()) | ||
| x, err := s.inferenceClient.Chat(stream.Context(), infereceChatRequest) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Populate missing fields and stop logging raw prompts.
- Forward ProjectContext to the inference request.
- Don’t log full user prompts; log model and lengths at most.
- infereceChatRequest := &inferenceProto.ChatRequest{
- Text: req.GetText(),
- ChatId: req.GetChatId(),
- Model: req.GetModel(),
- }
-
- fmt.Printf("calling inference client with text: %s model: %s\n", req.GetText(), req.GetModel())
+ infChatReq := &inferenceProto.ChatRequest{
+ Text: req.GetText(),
+ ChatId: req.GetChatId(),
+ Model: req.GetModel(),
+ ProjectContext: req.GetProjectContext(),
+ }
+ // slog.Debug("calling inference client", "model", req.GetModel(), "text_len", len(req.GetText()))Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/chatservice/api/api.go around lines 93 to 100, the inference request
is missing ProjectContext and the code logs the raw user prompt; update the
constructed inferenceProto.ChatRequest to set the ProjectContext field from the
incoming req (e.g., req.GetProjectContext()) and remove printing of the full
prompt—replace the fmt.Printf with a safe log that records only the model name
and the prompt length (and optionally chatId), not the prompt content itself.
| x, err := s.inferenceClient.Chat(stream.Context(), infereceChatRequest) | ||
| if err != nil { | ||
| slog.Error("error ", err) | ||
| return err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Propagate caller identity/metadata to the inference call and fix slog usage.
- Forward userID via gRPC metadata so the inference layer can enforce per-user access/policy.
- slog.Error should include key/value pairs.
- x, err := s.inferenceClient.Chat(stream.Context(), infereceChatRequest)
+ md := metadata.Pairs("x-user-id", userID)
+ ctx := metadata.NewOutgoingContext(stream.Context(), md)
+ x, err := s.inferenceClient.Chat(ctx, infChatReq)
if err != nil {
- slog.Error("error ", err)
+ slog.Error("inference client Chat failed", "error", err)
return err
}Add import:
import (
"context"
"fmt"
"io"
"log"
"log/slog"
"net/http"
+ "google.golang.org/grpc/metadata"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/chatservice/api/api.go around lines 100 to 104, the inference call
neither forwards caller identity via gRPC metadata nor uses slog correctly;
extract the userID from the incoming stream context metadata (or from the
authenticated context), create an outgoing context with metadata including a
"user-id" key (using google.golang.org/grpc/metadata and
metadata.AppendToOutgoingContext or metadata.NewOutgoingContext) and call
s.inferenceClient.Chat with that new context; on error, replace
slog.Error("error ", err) with slog.Error("inference Chat failed", "error", err,
"userID", userID) so the log uses key/value pairs and includes the user
identity; add the metadata import.
| userID, err := auth.GetUserIDFromContext_WithError(stream.Context()) | ||
| // if err != nil { | ||
| // return err | ||
| // } | ||
| fmt.Println("userID", userID) | ||
| text := req.Text |
There was a problem hiding this comment.
Handle auth error; don’t ignore it.
You’re discarding the error and continuing, which can allow unauthenticated access.
Apply:
- userID, err := auth.GetUserIDFromContext_WithError(stream.Context())
- // if err != nil {
- // return err
- // }
- fmt.Println("userID", userID)
+ userID, err := auth.GetUserIDFromContext_WithError(stream.Context())
+ if err != nil {
+ return err
+ }
+ _ = userID // reserved for audit/logging📝 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.
| userID, err := auth.GetUserIDFromContext_WithError(stream.Context()) | |
| // if err != nil { | |
| // return err | |
| // } | |
| fmt.Println("userID", userID) | |
| text := req.Text | |
| userID, err := auth.GetUserIDFromContext_WithError(stream.Context()) | |
| if err != nil { | |
| return err | |
| } | |
| _ = userID // reserved for audit/logging | |
| text := req.Text |
🤖 Prompt for AI Agents
In backend/inferenceservice/api/api.go around lines 162-167, the auth error
returned by auth.GetUserIDFromContext_WithError is being ignored and userID is
printed; instead check if err != nil and return an authentication error (e.g.,
return status.Errorf(codes.Unauthenticated, "unauthenticated: %v", err) or the
appropriate gRPC error for your service) to prevent unauthenticated access,
remove the debug fmt.Println of userID, and only proceed when userID is valid.
| fmt.Printf("model name %s, text %s\n", modelName, text) | ||
| tokenChan, err := s.service.Predict(stream.Context(), modelName, text) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Stream tokens to the client | ||
| for token := range tokenChan { | ||
| // Send each token as it's generated | ||
| err := stream.Send(&pb.ChatResponse{ | ||
| Response: &pb.ChatResponse_Text{ | ||
| Text: token, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| // If streaming fails, break the loop | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return streaming errors and respect cancellation.
Breaking the loop on Send error and returning nil hides real errors; also consider ctx cancellation to stop early.
Apply:
- // Stream tokens to the client
- for token := range tokenChan {
- // Send each token as it's generated
- err := stream.Send(&pb.ChatResponse{
- Response: &pb.ChatResponse_Text{
- Text: token,
- },
- })
- if err != nil {
- // If streaming fails, break the loop
- break
- }
- }
-
- return nil
+ ctx := stream.Context()
+ for {
+ select {
+ case <-ctx.Done():
+ return ctx.Err()
+ case token, ok := <-tokenChan:
+ if !ok {
+ return nil
+ }
+ if err := stream.Send(&pb.ChatResponse{
+ Response: &pb.ChatResponse_Text{Text: token},
+ }); err != nil {
+ return err
+ }
+ }
+ }📝 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.
| fmt.Printf("model name %s, text %s\n", modelName, text) | |
| tokenChan, err := s.service.Predict(stream.Context(), modelName, text) | |
| if err != nil { | |
| return err | |
| } | |
| // Stream tokens to the client | |
| for token := range tokenChan { | |
| // Send each token as it's generated | |
| err := stream.Send(&pb.ChatResponse{ | |
| Response: &pb.ChatResponse_Text{ | |
| Text: token, | |
| }, | |
| }) | |
| if err != nil { | |
| // If streaming fails, break the loop | |
| break | |
| } | |
| } | |
| return nil | |
| fmt.Printf("model name %s, text %s\n", modelName, text) | |
| tokenChan, err := s.service.Predict(stream.Context(), modelName, text) | |
| if err != nil { | |
| return err | |
| } | |
| ctx := stream.Context() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| case token, ok := <-tokenChan: | |
| if !ok { | |
| return nil | |
| } | |
| if err := stream.Send(&pb.ChatResponse{ | |
| Response: &pb.ChatResponse_Text{Text: token}, | |
| }); err != nil { | |
| return err | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In backend/inferenceservice/api/api.go around lines 171 to 191, the current loop
breaks on stream.Send error and returns nil, which hides real send errors and
does not respect request cancellation; change the loop to select between
receiving tokens from tokenChan and stream.Context().Done(), return ctx.Err() if
canceled, and when stream.Send returns an error return that error immediately
(do not just break); also ensure you handle the closed tokenChan by exiting the
loop and returning nil only when the channel is exhausted without errors.
| func (s *InferenceService) Predict(ctx context.Context, modelName string, prompt string) (<-chan string, error) { | ||
| // Load model by name (uses caching for faster subsequent loads) | ||
|
|
||
| fmt.Println("predict %s, %s", modelName, prompt) | ||
| model, err := l.LoadModelByName(modelName) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper formatting/logging; current Println ignores format specifiers.
Also, pass ctx through to the llamacpp predictor for cancellation.
Apply:
-func (s *InferenceService) Predict(ctx context.Context, modelName string, prompt string) (<-chan string, error) {
+func (s *InferenceService) Predict(ctx context.Context, modelName string, prompt string) (<-chan string, error) {
// Load model by name (uses caching for faster subsequent loads)
- fmt.Println("predict %s, %s", modelName, prompt)
+ // fmt.Printf("predict model=%s chars=%d\n", modelName, len(prompt))
model, err := l.LoadModelByName(modelName)
if err != nil {
return nil, err
}And update generation to use context:
- tokens, err := l.Predict(model, prompt)
+ tokens, err := l.Predict(ctx, model, prompt)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
backend/inferenceservice/service/llama.go lines 9-16: the current call uses
fmt.Println with format specifiers (ignored) and does not propagate the context
to the model/predictor; replace the fmt.Println with a proper formatted
log/print (e.g., fmt.Printf or logger.Printf) that includes modelName and a
safe/trimmed prompt, and ensure ctx is passed into the llamacpp predictor and
used for generation so operations can be cancelled — update calls to
create/obtain the predictor and to start generation to accept and forward ctx.
| // Create InferenceService first | ||
| inferenceServiceApi := inferenceApi.NewInferenceServiceAPI(inferenceDaoFactory) | ||
| inferenceServiceApi.Init(inferenceConfig) | ||
| infereceProto.RegisterInferenceServiceServer(grpcServer, inferenceServiceApi) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not expose InferenceService on the public gRPC port.
You register the same InferenceService on the external TCP server and the internal UDS server. If the service isn’t intended for public access, drop the public registration to reduce attack surface and avoid duplicate serving of the same instance.
- infereceProto.RegisterInferenceServiceServer(grpcServer, inferenceServiceApi)
+ // Do not expose InferenceService on the public gRPC server; it is served over UDS below.📝 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.
| // Create InferenceService first | |
| inferenceServiceApi := inferenceApi.NewInferenceServiceAPI(inferenceDaoFactory) | |
| inferenceServiceApi.Init(inferenceConfig) | |
| infereceProto.RegisterInferenceServiceServer(grpcServer, inferenceServiceApi) | |
| // Create InferenceService first | |
| inferenceServiceApi := inferenceApi.NewInferenceServiceAPI(inferenceDaoFactory) | |
| inferenceServiceApi.Init(inferenceConfig) | |
| // Do not expose InferenceService on the public gRPC server; it is served over UDS below. |
🤖 Prompt for AI Agents
In backend/mono/main.go around lines 167 to 171, you register InferenceService
on both the external TCP gRPC server and the internal UDS server; remove the
public registration by deleting or guarding the line that calls
infereceProto.RegisterInferenceServiceServer(grpcServer, inferenceServiceApi)
for the external (public) server so the service is only registered on the
internal UDS gRPC server, and ensure no other code path registers the same
instance on the public port.
| unixListener, err := net.Listen("unix", socketPath) | ||
| if err != nil { | ||
| log.Fatalf("Failed to listen on Unix socket: %v", err) | ||
| } | ||
|
|
||
| // Create a separate gRPC server for the client connection | ||
| clientGrpcServer := grpc.NewServer() | ||
| infereceProto.RegisterInferenceServiceServer(clientGrpcServer, inferenceServiceApi) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tighten UDS security and add cleanup.
- Set restrictive permissions (0600) on the socket.
- Ensure graceful shutdown and file cleanup to prevent stale sockets.
unixListener, err := net.Listen("unix", socketPath)
if err != nil {
log.Fatalf("Failed to listen on Unix socket: %v", err)
}
+
+ // Restrict socket to the current user
+ if err := os.Chmod(socketPath, 0o600); err != nil {
+ log.Fatalf("Failed to set socket permissions: %v", err)
+ }
+
+ // Cleanup on shutdown
+ defer os.Remove(socketPath)
+ defer unixListener.Close()
// Create a separate gRPC server for the client connection
clientGrpcServer := grpc.NewServer()
infereceProto.RegisterInferenceServiceServer(clientGrpcServer, inferenceServiceApi)
go func() {
if err := clientGrpcServer.Serve(unixListener); err != nil {
log.Fatalf("Failed to serve client gRPC server: %v", err)
}
}()
+ defer clientGrpcServer.GracefulStop()Also applies to: 190-195
| syntax = "proto3"; | ||
|
|
||
| package sortedchat; | ||
| package sortedchat.inferenceservice; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Buf package-directory violation.
With package sortedchat.inferenceservice, Buf expects this file under proto/sortedchat/inferenceservice/. Currently it’s in proto/.
- Move to proto/sortedchat/inferenceservice/inferenceservice.proto (preferred), or
- Change package back to sortedchat and adjust service namepaths.
Please run buf lint locally to confirm.
🏁 Script executed:
#!/bin/bash
buf lintLength of output: 5235
Fix Buf package-directory violation for proto/inferenceservice.proto
buf lint confirms the package/path mismatch and versioning requirement. Fix by one of:
- Preferred: move proto/inferenceservice.proto → proto/sortedchat/inferenceservice/v1/inferenceservice.proto and change the file header to:
package sortedchat.inferenceservice.v1; - Alternative: change the package to a single, versioned package (e.g., package sortedchat.v1;) and move the file under proto/sortedchat/v1/, updating any service/message namepaths accordingly.
Re-run buf lint to verify.
🧰 Tools
🪛 Buf (1.55.1)
3-3: Files with package "sortedchat.inferenceservice" must be within a directory "sortedchat/inferenceservice" relative to root but were in directory "proto".
(PACKAGE_DIRECTORY_MATCH)
🤖 Prompt for AI Agents
In proto/inferenceservice.proto around line 3, Buf reports a package/path
mismatch and missing versioning; move the file to
proto/sortedchat/inferenceservice/v1/inferenceservice.proto and update the file
header package line to package sortedchat.inferenceservice.v1; then update any
dependent imports and fully-qualified message/service references to the new
path/package name, run buf lint and fix any remaining path or import issues
until it passes.
| message ChatRequest { | ||
| string text = 1; | ||
| string chatId = 2; | ||
| string model = 3; | ||
| ProjectContext project_context = 4; | ||
| } | ||
|
|
||
| message ProjectContext { | ||
| string project_id = 1; | ||
| bool rag_enabled = 2; | ||
| } |
There was a problem hiding this comment.
Fix field naming and typos before release (breaking API change).
Use snake_case for fields and correct the misspelling “simillarity”.
Apply these renames (then regenerate Go/TS stubs):
- ChatRequest.chatId -> chat_id
- RAGDocumentReferenceSummaryList.Summary.chunkCount -> chunk_count
- RAGDocumentReference.Chunks -> chunks
- RAGDocumentReference.Chunk.simillarity -> similarity
- ChatInfo.chatId -> chat_id
I can push a full proto diff and regenerate bindings on request.
Also applies to: 50-58, 60-70, 81-89
🤖 Prompt for AI Agents
In proto/inferenceservice.proto around lines 25 to 35 (and also apply to ranges
50-58, 60-70, 81-89), several field names use camelCase or contain a typo;
update the proto field names to snake_case and fix the misspelling, then
regenerate language bindings: rename ChatRequest.chatId -> chat_id; rename
ChatInfo.chatId -> chat_id; rename
RAGDocumentReferenceSummaryList.Summary.chunkCount -> chunk_count; rename
RAGDocumentReference.Chunks -> chunks; rename
RAGDocumentReference.Chunk.simillarity -> similarity; after updating the .proto
file, run the Go and TS proto generators to regenerate stubs and update any code
references to the new field names.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
backend/mono/main.go (4)
207-215: Graceful shutdown for the UDS gRPC server.
Add GracefulStop to avoid leaving the server half-open on exit.Apply this diff:
// Create a separate gRPC server for the client connection clientGrpcServer := grpc.NewServer() infereceProto.RegisterInferenceServiceServer(clientGrpcServer, inferenceServiceApi) + defer clientGrpcServer.GracefulStop()
188-192: Do not register InferenceService on the public gRPC server.
This exposes an internal surface externally and duplicates serving of the same instance. Keep it UDS-only.Apply this diff:
- infereceProto.RegisterInferenceServiceServer(grpcServer, inferenceServiceApi) + // Do not expose InferenceService on the public gRPC server; it is served over UDS below.
196-206: UDS cleanup and permissions are missing.
- Remove the existing file safely (avoid RemoveAll).
- Restrict the socket file to 0600.
- Ensure cleanup of the socket file and listener.
Apply this diff:
- // Remove existing socket file if it exists - if err := os.RemoveAll(socketPath); err != nil { - log.Fatalf("Failed to remove existing socket: %v", err) - } + // Remove existing socket file if it exists + if err := os.Remove(socketPath); err != nil && !os.IsNotExist(err) { + log.Fatalf("Failed to remove existing socket: %v", err) + } @@ unixListener, err := net.Listen("unix", socketPath) if err != nil { log.Fatalf("Failed to listen on Unix socket: %v", err) } + // Restrict socket to the current user + if err := os.Chmod(socketPath, 0o600); err != nil { + log.Fatalf("Failed to set socket permissions: %v", err) + } + // Cleanup on shutdown + defer os.Remove(socketPath) + defer unixListener.Close()
222-229: grpc.NewClient does not exist; use DialContext, block with timeout, and avoid hardcoded path.
This won’t compile as-is and risks an indefinite hang without a deadline. Also, reuse socketPath instead of the literal.Apply this diff:
- inferenceConn, err := grpc.NewClient("unix:///tmp/inference-service.sock", - grpc.WithContextDialer(unixDialer), - grpc.WithTransportCredentials(insecure.NewCredentials())) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + inferenceConn, err := grpc.DialContext( + ctx, + "unix://"+socketPath, + grpc.WithContextDialer(unixDialer), + grpc.WithTransportCredentials(insecure.NewCredentials()), + grpc.WithBlock(), + )
🧹 Nitpick comments (2)
backend/mono/main.go (2)
217-221: Respect context cancellation in the Unix dialer.
Current dialer ignores ctx; use net.Dialer.DialContext.Apply this diff:
- unixDialer := func(ctx context.Context, address string) (net.Conn, error) { - return net.Dial("unix", socketPath) - } + d := net.Dialer{} + unixDialer := func(ctx context.Context, _ string) (net.Conn, error) { + return d.DialContext(ctx, "unix", socketPath) + }
193-229: Optional: consider in-memory bufconn for in-process calls.
Using grpc/test/bufconn (or an equivalent) avoids filesystem sockets, simplifies cleanup, and improves portability (Windows).I can provide a bufconn wiring snippet if you want to pursue this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/mono/main.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/**
📄 CodeRabbit inference engine (.cursor/rules/project-description.mdc)
Backend code resides under backend/
Files:
backend/mono/main.go
backend/mono/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/project-description.mdc)
The monolith is implemented under backend/mono and must be runnable via
go run ./monofrom backend/
Files:
backend/mono/main.go
backend/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/project-description.mdc)
Backend services are written in Go 1.24 using gRPC
Files:
backend/mono/main.go
🧠 Learnings (1)
📚 Learning: 2025-08-27T17:27:06.967Z
Learnt from: CR
PR: sortedstartup/sortedchat#0
File: .cursor/rules/project-description.mdc:0-0
Timestamp: 2025-08-27T17:27:06.967Z
Learning: Applies to backend/**/*.go : Backend services are written in Go 1.24 using gRPC
Applied to files:
backend/mono/main.go
🧬 Code graph analysis (1)
backend/mono/main.go (3)
backend/inferenceservice/api/api.go (1)
NewInferenceServiceAPI(23-30)backend/inferenceservice/proto/inferenceservice_grpc.pb.go (2)
RegisterInferenceServiceServer(160-169)NewInferenceServiceClient(44-46)backend/chatservice/api/api.go (1)
NewChatService(66-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Go backend (linux, amd64, ubuntu-latest)
- GitHub Check: Build Go backend (windows, amd64, ubuntu-latest)
- GitHub Check: Build Go backend (darwin, amd64, macos-latest)
- GitHub Check: Build Go backend (linux, arm64, ubuntu-latest)
- GitHub Check: Build Go backend (darwin, arm64, macos-latest)
🔇 Additional comments (2)
backend/mono/main.go (2)
5-5: Good addition; will use this for deadline-aware dialing.
No issues with importing context.
27-27: OK to use insecure creds for UDS-only traffic.
Acceptable here since the socket is local-only; see separate comment about tightening UDS permissions.
| inferenceClient := infereceProto.NewInferenceServiceClient(inferenceConn) | ||
|
|
||
| chatServiceApi := api.NewChatService(mux, queue, settingsManager, daoFactory, inferenceClient) |
There was a problem hiding this comment.
🧩 Analysis chain
Wiring ChatService with the inference client looks correct.
Constructor signature matches the referenced API and passes the UDS client.
Run to verify no lingering hardcoded paths or public registrations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP 'RegisterInferenceServiceServer\s*\(\s*grpcServer' -C2
rg -nP 'inference-service\.sock'Length of output: 563
Wiring OK — hardcoded UDS path & public registration found; fix required
Wiring ChatService with the inference client is correct, but verification found an explicit server registration and a hardcoded Unix socket path; make the socket path configurable (env/flag/config) and avoid/register safely (or lock UDS permissions) instead of using a fixed /tmp path.
Locations: backend/mono/main.go:191 (infereceProto.RegisterInferenceServiceServer(...)), 194 (socketPath := "/tmp/inference-service.sock"), 222 (grpc.NewClient("unix:///tmp/inference-service.sock", ...)).
🤖 Prompt for AI Agents
In backend/mono/main.go around lines 191–232, the code uses a hardcoded Unix
socket path and performs an explicit public server registration; make the socket
path configurable and harden UDS usage: introduce/consume a config/flag/env
value (e.g., INFERENCE_SOCKET_PATH) instead of "/tmp/inference-service.sock" and
use that variable at the registration, socket creation, and grpc.NewClient call
sites; before binding, ensure any existing socket file is removed and then set
restrictive permissions (chmod 0600 or create in a restricted runtime dir) or
use an abstract namespace on Linux to avoid world-readable sockets; also avoid
exposing the service publicly by keeping registration and listener scoped to the
local runtime (or restrict access via file permissions) rather than leaving a
globally writable /tmp socket.
Summary by CodeRabbit
New Features
Refactor
Chores