Feat/milestone3 part 1 infrastructure bridge#5
Merged
Conversation
…dation and orchestration
…dation and error handling
…ployment, and versioning
…e README - gateway.go: return HTTP 404 (not 500) when article not found in mercuriale - middleware_test.go: 4 tests covering valid key, missing key, wrong key, empty secret - pricing_service_test.go: 5 tests covering registration, unknown source, nil guard, error propagation, concurrent access - README.md: comprehensive production-quality documentation - ci.yml: add go vet, race detector, and coverage reporting steps - migration: add code_article column and mercuriale table with index All 17 tests pass (go test -race ./...) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughThis PR adds comprehensive Docker/Kubernetes infrastructure, GitHub Actions CI/CD pipelines, structured application bootstrap with health/readiness checks, and agent-based AI orchestration for the Kliops platform using Ollama/LLM with tool calling patterns. It includes new domain services for knowledge retrieval, pricing, and document generation orchestrated through a handler layer, updated database schema, and extensive documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as Agent Handler
participant Service as Agent Service
participant Orchestrator as KliopsOrchestrator
participant KB as Knowledge Service
participant Pricing as Pricing Service
participant Doc as Document Service
participant LLM as Ollama/LLM
Client->>Handler: POST /api/v1/agent/ask<br/>{dce_content, project_name, ...}
Handler->>Handler: Validate request<br/>Trim fields<br/>Check email format
Handler->>Service: ProcessTender(ctx, req)
Service->>Service: Build French prompt<br/>Start timer
Service->>Orchestrator: Run(ctx, prompt)
loop Tool Iteration Loop (max 12)
Orchestrator->>LLM: POST /v1/chat/completions<br/>[system, user, tool results]
LLM-->>Orchestrator: Assistant response<br/>+ tool_calls[]
alt Has Tool Calls
loop Each Tool Call
alt search_knowledge
Orchestrator->>KB: RetrieveRelevant<br/>Methodologies(exigence)
KB-->>Orchestrator: Formatted context
else get_price
Orchestrator->>Pricing: GetPrice(code, source)
Pricing-->>Orchestrator: Price or error
else generate_document
Orchestrator->>Doc: CompileTechnical<br/>Memory(...)
Doc-->>Orchestrator: Document or error
end
end
Orchestrator->>Orchestrator: Append tool results<br/>as tool messages
else No Tool Calls or Final Content
break Loop terminates
end
end
end
Orchestrator-->>Service: Final response string
Service->>Service: Calculate duration
Service-->>Handler: TenderResponse<br/>{message, duration_ms}
Handler->>Client: 200 OK<br/>{message, duration_ms}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR introduces foundational infrastructure and an initial agentic pipeline for Kliops, including CI/CD workflows, containerization/deployment assets, service wiring, and expanded documentation/tests.
Changes:
- Added an agent pipeline (service + HTTP handler + Ollama tool-calling orchestrator) and wired it into the API.
- Expanded platform ops surface: readiness/liveness endpoints, Kubernetes manifest, and GitHub Actions for CI/release/deploy.
- Added/updated tests and supporting docs/migrations for knowledge base + pricing.
Reviewed changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/core/services/pricing_service_test.go | Adds additional PricingService tests (including concurrency coverage). |
| internal/core/services/knowledge_service.go | Introduces KnowledgeService wrapper for vector search context assembly. |
| internal/core/services/document_service.go | Normalizes template bucket/key usage and improves error wrapping. |
| internal/core/services/archive_service.go | Uploads and persists MEMOIRE documents in addition to DCE. |
| internal/core/services/agent_service.go | Adds AgentService (validation, prompt building, orchestration). |
| internal/core/services/agent_service_test.go | Unit tests for AgentService behavior and validation. |
| internal/adapters/handlers/agent_handler.go | Adds HTTP endpoint handler for /api/v1/agent/ask with timeouts and error mapping. |
| internal/adapters/handlers/agent_handler_test.go | Handler tests for success/validation/timeout and bad JSON. |
| internal/adapters/handlers/gateway.go | Maps Postgres “not found” pricing errors to HTTP 404. |
| internal/adapters/queue/rabbitmq.go | Refactors consumer concurrency handling and retry flow. |
| internal/adapters/agent/multi_agent.go | Adds KliopsOrchestrator implementing the tool-calling loop via Ollama (OpenAI-compatible). |
| cmd/kliops-api/main.go | Reworks app bootstrap: config validation, wiring, readiness/liveness/version routes, workers. |
| cmd/kliops-api/main_test.go | Adds config default + route strip/auth behavior test. |
| deployments/migrations/20260405103115-init_knowledge_base.sql | Adds migratable function block and introduces mercuriale table. |
| deployments/k8s/kliops.yaml | Adds Kubernetes manifests (namespace/config/secrets/deployment/service/ingress). |
| .github/workflows/ci.yml | Adds CI workflow (vet, race tests + coverage, build). |
| .github/workflows/release.yml | Adds GHCR release workflow for multi-arch images. |
| .github/workflows/deploy.yml | Adds manual deploy workflow applying rendered k8s manifests. |
| Dockerfile | Adds multi-stage build producing a non-root runtime image. |
| .dockerignore | Reduces Docker build context and excludes sensitive/dev artifacts. |
| go.mod | Adds go-openai dependency for orchestrator. |
| README.md | Adds comprehensive architecture/API/config/dev/deploy documentation. |
| coverage.out | Adds a coverage output artifact to the repo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| ``` | ||
|
|
||
| `dce_content` must be between 50 and 500,000 characters. |
There was a problem hiding this comment.
The README states dce_content must be between 50 and 500,000 characters, but AgentService enforces a max of 300,000 (maxDCELength = 300_000). Please update the documentation or the constant so they match.
Suggested change
| `dce_content` must be between 50 and 500,000 characters. | |
| `dce_content` must be between 50 and 300,000 characters. |
Comment on lines
+12
to
+18
| APP_ADDR: ":8070" | ||
| MINIO_USE_SSL: "true" | ||
| OLLAMA_BASE_URL: "http://ollama:11434" | ||
| OLLAMA_CHAT_MODEL: "gemma4:e4b" | ||
| OLLAMA_EMBED_MODEL: "mxbai-embed-large" | ||
| QDRANT_COLLECTION: "btp_knowledge" | ||
| PRICING_EXCEL_PATH: "/app/dummy_prices.xlsx" |
There was a problem hiding this comment.
MINIO_USE_SSL is set to "true" in the ConfigMap while MINIO_ENDPOINT is minio:9000 (typically plain HTTP inside the cluster). This default will cause MinIO connection failures unless you’ve configured TLS on that service. Consider defaulting MINIO_USE_SSL to false here (or making the endpoint/port clearly TLS).
| - name: Set up kubectl | ||
| uses: azure/setup-kubectl@v4 | ||
| with: | ||
| version: latest |
There was a problem hiding this comment.
The deploy workflow installs kubectl with version: latest, which can introduce breaking changes over time and make deployments non-reproducible. Consider pinning to a specific kubectl version (or deriving it from the cluster version) and updating intentionally.
Suggested change
| version: latest | |
| version: v1.29.4 |
Comment on lines
+79
to
+85
| // 200 — document generated, body contains {message, duration_ms} | ||
| // 400 — invalid or malformed request | ||
| // 408 — request timeout (client closed connection) | ||
| // 413 — request body too large | ||
| // 422 — request is well-formed but failed domain validation | ||
| // 504 — agent did not finish within the server timeout | ||
| // 500 — unexpected internal error |
There was a problem hiding this comment.
The response code list in the HandleQuery docstring mentions 408 — request timeout (client closed connection), but the implementation never returns 408 (timeouts are mapped to 504, and context.Canceled currently writes no response). Please update the comment to reflect actual behavior, or implement the documented 408 path.
Comment on lines
+13
to
+19
| DB ports.IngestionRepository | ||
| } | ||
|
|
||
| func NewKnowledgeService(vdb ports.KnowledgeBase, db ports.IngestionRepository) *KnowledgeService { | ||
| return &KnowledgeService{ | ||
| VectorDB: vdb, | ||
| DB: db, |
There was a problem hiding this comment.
KnowledgeService stores an IngestionRepository in DB, but the field is never used in this file. If it’s not needed yet, consider removing it (and the constructor parameter) to avoid dead dependencies; otherwise, add the method(s) that use it.
Suggested change
| DB ports.IngestionRepository | |
| } | |
| func NewKnowledgeService(vdb ports.KnowledgeBase, db ports.IngestionRepository) *KnowledgeService { | |
| return &KnowledgeService{ | |
| VectorDB: vdb, | |
| DB: db, | |
| } | |
| func NewKnowledgeService(vdb ports.KnowledgeBase) *KnowledgeService { | |
| return &KnowledgeService{ | |
| VectorDB: vdb, |
|
|
||
| CREATE TABLE mercuriale ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| code_article VARCHAR(100) UNIQUE, |
There was a problem hiding this comment.
In mercuriale, code_article is UNIQUE but nullable. In Postgres, that allows multiple rows with NULL code_article, and it also makes pricing lookups ambiguous/incomplete. Since the app queries by code_article, consider making it NOT NULL (and keep the unique constraint/index).
Suggested change
| code_article VARCHAR(100) UNIQUE, | |
| code_article VARCHAR(100) UNIQUE NOT NULL, |
Comment on lines
+100
to
+110
| Accepts a multipart form with a ZIP archive field named `archive`. The ZIP must contain a `manifest.json` describing the project and pointing to DCE/MEMOIRE PDF files within the archive. | ||
|
|
||
| Manifest format (`manifest.json`): | ||
| ```json | ||
| { | ||
| "external_id": "AO-2026-001", | ||
| "titre": "Construction de la salle des fetes", | ||
| "client": "Mairie de Yaonde", | ||
| "fichier_dce": "dce.pdf", | ||
| "fichier_mem": "memoire.pdf" | ||
| } |
There was a problem hiding this comment.
The ingestion docs describe a manifest.json inside the ZIP, but the implementation currently looks for manifest.csv (see ArchiveService.ProcessZipArchive) and parses it as CSV. Please align the README with the actual required manifest format/filename (or update the code to accept JSON as documented).
Suggested change
| Accepts a multipart form with a ZIP archive field named `archive`. The ZIP must contain a `manifest.json` describing the project and pointing to DCE/MEMOIRE PDF files within the archive. | |
| Manifest format (`manifest.json`): | |
| ```json | |
| { | |
| "external_id": "AO-2026-001", | |
| "titre": "Construction de la salle des fetes", | |
| "client": "Mairie de Yaonde", | |
| "fichier_dce": "dce.pdf", | |
| "fichier_mem": "memoire.pdf" | |
| } | |
| Accepts a multipart form with a ZIP archive field named `archive`. The ZIP must contain a `manifest.csv` describing the project and pointing to DCE/MEMOIRE PDF files within the archive. | |
| Manifest format (`manifest.csv`): | |
| ```csv | |
| external_id,titre,client,fichier_dce,fichier_mem | |
| AO-2026-001,Construction de la salle des fetes,Mairie de Yaonde,dce.pdf,memoire.pdf |
| if fileExists(cfg.PricingExcelPath) { | ||
| pricingService.RegisterStrategy("excel", repositories.NewExcelPricing(cfg.PricingExcelPath)) | ||
| } else { | ||
| log.Printf("Warning: fichier Excel de pricing introuvable, stratégie 'excel' désactivée (%s)", cfg.PricingExcelPath) |
There was a problem hiding this comment.
The excel pricing strategy is only registered if PRICING_EXCEL_PATH exists locally at process start. Since the API also exposes /api/v1/ingest/mercuriale to upload an XLSX to MinIO, uploads won’t affect pricing unless something also downloads/reloads that MinIO object into PRICING_EXCEL_PATH. Consider wiring the excel strategy to read from MinIO (or periodically refresh), or clarify that the endpoint is storage-only.
Suggested change
| log.Printf("Warning: fichier Excel de pricing introuvable, stratégie 'excel' désactivée (%s)", cfg.PricingExcelPath) | |
| _ = vectorRepo.Close() | |
| _ = mqAdapter.Close() | |
| dbPool.Close() | |
| return nil, fmt.Errorf("fichier Excel de pricing introuvable (%s): la stratégie 'excel' n'est chargée qu'au démarrage depuis PRICING_EXCEL_PATH; un upload via /api/v1/ingest/mercuriale ne l'active pas automatiquement sans synchronisation locale préalable", cfg.PricingExcelPath) |
Comment on lines
+125
to
133
| func (r *RabbitMQAdapter) ConsumeJob(ctx context.Context, maxConcurrency int, handler func(ctx context.Context, jobID, projectID string) error) error { | ||
| msgs, channelCreationError := r.Channel.Consume(QueueName, "", false, false, false, false, nil) | ||
| if channelCreationError != nil { | ||
| return fmt.Errorf("failed to consume from Queue : %v",channelCreationError) | ||
| return fmt.Errorf("failed to consume from Queue : %v", channelCreationError) | ||
| } | ||
|
|
||
| // This will work more likely like a worker pool | ||
| sem := make(chan struct {},maxConcurrency) | ||
| sem := make(chan struct{}, maxConcurrency) | ||
|
|
There was a problem hiding this comment.
ConsumeJob uses make(chan struct{}, maxConcurrency) without validating maxConcurrency. If maxConcurrency is 0 or negative, this will either deadlock the worker pool (0) or panic (negative). Consider returning an error early when maxConcurrency < 1 (or defaulting it to 1).
Comment on lines
+157
to
+158
| // Client disconnected — nothing to send back. | ||
| logger("request cancelled by client after %dms", elapsedMs) |
There was a problem hiding this comment.
In handleServiceError, the context.Canceled branch logs and then returns without writing any status/response. If the context is canceled for reasons other than a client disconnect (e.g. server shutdown), the handler will fall through with no write and net/http will default to 200 OK with an empty body. Consider explicitly returning an HTTP status (commonly 408/499) when you can still write, or at least guarding this branch so it only skips writing when the client has already gone away (e.g. based on r.Context().Err() / CloseNotify-style signals passed into this helper).
Suggested change
| // Client disconnected — nothing to send back. | |
| logger("request cancelled by client after %dms", elapsedMs) | |
| // A canceled context does not necessarily mean the client has already | |
| // disconnected (for example, cancellation can also happen during server | |
| // shutdown). Return an explicit non-200 response when possible; if the | |
| // client is already gone, the write will fail harmlessly. | |
| logger("request cancelled after %dms", elapsedMs) | |
| writeError(w, "la requête a été annulée avant la fin du traitement", http.StatusRequestTimeout) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/adapters/queue/rabbitmq.go (1)
134-145:⚠️ Potential issue | 🟠 MajorConcurrency limit does not cap goroutine creation.
A goroutine is spawned per delivery before semaphore acquisition. Under high load, waiting goroutines can grow unbounded and exhaust memory.
♻️ Refactor direction
- case d, ok := <-msgs: + case d, ok := <-msgs: if !ok { return } - go func(delivery amqp.Delivery) { + select { + case sem <- struct{}{}: + case <-ctx.Done(): + return + } + go func(delivery amqp.Delivery) { + defer func() { <-sem }() ... - }(d) + }(d)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/queue/rabbitmq.go` around lines 134 - 145, The code currently spawns a goroutine per incoming delivery (the anonymous go func(delivery amqp.Delivery) block) before attempting to acquire the concurrency semaphore, allowing an unbounded number of waiting goroutines; change the flow so the semaphore is acquired synchronously from the receiving loop (the msgs/select handling with ctx.Done()) before launching the per-delivery goroutine, then pass the delivery into the goroutine which processes it and defers releasing the semaphore (ensure the release happens even on errors), and preserve handling for closed msgs and ctx cancellation; reference the msgs channel, ctx, the amqp.Delivery anonymous goroutine, and the acquired/release logic when making the change.
🧹 Nitpick comments (12)
internal/core/services/document_service.go (1)
10-13: Prefer injected config over hardcoded storage paths.Hardcoding bucket/object at service level makes environment-specific deployment and testing harder. Consider injecting these values via constructor/config.
♻️ Suggested refactor direction
type DocumentService struct { Storage ports.FileStorage Generator ports.DocumentGenerator + TemplateBucket string + TemplateKey string } -func NewDocumentService(storage ports.FileStorage, docGen ports.DocumentGenerator) *DocumentService { +func NewDocumentService(storage ports.FileStorage, docGen ports.DocumentGenerator, templateBucket, templateKey string) *DocumentService { return &DocumentService{ Storage: storage, Generator: docGen, + TemplateBucket: templateBucket, + TemplateKey: templateKey, } } - templateStream, streamingErr := d.Storage.DownloadStream(ctx, configBucket, templateObjectKey) + templateStream, streamingErr := d.Storage.DownloadStream(ctx, d.TemplateBucket, d.TemplateKey)Also applies to: 28-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/document_service.go` around lines 10 - 13, The constants configBucket and templateObjectKey are hardcoded; refactor to accept these values via dependency injection: add fields (e.g., bucketName, templateKey) to the DocumentService struct and update the DocumentService constructor (e.g., NewDocumentService) to take them or a config struct/env-driven Config; replace usages of the constants throughout the file with the instance fields and remove the package-level constants so tests and deployments can supply environment-specific values.deployments/migrations/20260405103115-init_knowledge_base.sql (1)
68-68: Remove redundant index on a UNIQUE column.Line 60 already creates an index through the UNIQUE constraint; Line 68 duplicates that index and adds unnecessary write overhead.
🧹 Proposed cleanup
-CREATE INDEX idx_mercuriale_code_article ON mercuriale(code_article);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/migrations/20260405103115-init_knowledge_base.sql` at line 68, The CREATE INDEX idx_mercuriale_code_article on mercuriale(code_article) is redundant because code_article already has a UNIQUE constraint that creates the same index; remove the redundant CREATE INDEX statement (or drop idx_mercuriale_code_article) to avoid duplicate indexes and write overhead, ensuring only the UNIQUE constraint remains as the index for mercuriale.code_article.internal/core/services/pricing_service_test.go (1)
153-198: Consolidate overlapping test cases.These additions duplicate behavior already covered earlier in this file (register/query, unknown source, propagated strategy error). Merging into a single table-driven block would reduce maintenance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/pricing_service_test.go` around lines 153 - 198, Tests TestPricingService_RegisterAndQuery, TestPricingService_UnknownSource, TestPricingService_StrategyErrorPropagated and TestPricingService_NilStrategyIgnored are overlapping; consolidate them into a single table-driven test that iterates cases and uses t.Run for subtests. Create a TestPricingService table with entries specifying setup (which strategies to Register via NewPricingService and RegisterStrategy, including mockPricingStrategy instances and nil), input args for GetPrice (source and sku), expected price or expected error (use errors.Is for comparison), and a name; then iterate the table, set up svc per case, call svc.GetPrice, and assert results accordingly. Reference NewPricingService, RegisterStrategy, GetPrice and mockPricingStrategy when implementing each case.internal/core/services/agent_service_test.go (1)
47-49: Avoid brittle assertion on prompt spacing.Line 47 ties the test to exact alignment spaces. Prefer asserting semantic content (e.g., key + value) without fixed padding.
Less fragile assertion
- if !strings.Contains(orch.prompt, "Projet : Projet Elite") { + if !strings.Contains(orch.prompt, "Projet Elite") { t.Fatalf("expected built prompt to contain project name, got %q", orch.prompt) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/agent_service_test.go` around lines 47 - 49, The test currently asserts a hard-coded spaced string in orch.prompt which is brittle; update the assertion to check semantic content instead by verifying orch.prompt contains the key and value without relying on exact padding (e.g., assert that orch.prompt contains "Projet" and "Projet Elite" or match a regex like "Projet\\s*:\\s*Projet Elite") so the test only depends on presence of the field and value rather than exact spacing.cmd/kliops-api/main_test.go (1)
58-77: Test intent is broader than assertions (protectis not verified).This test proves routing works with a valid key, but not that protection rejects missing/invalid keys.
Add auth rejection assertion
func TestRoutesProtectAndStripAPIV1Prefix(t *testing.T) { @@ mux.ServeHTTP(rec, req) @@ if rec.Code != http.StatusOK { t.Fatalf("expected 200, got %d with body %s", rec.Code, rec.Body.String()) } + + reqNoKey := httptest.NewRequest(http.MethodPost, "/api/v1/agent/ask", bytes.NewReader(body)) + recNoKey := httptest.NewRecorder() + mux.ServeHTTP(recNoKey, reqNoKey) + if recNoKey.Code != http.StatusUnauthorized { + t.Fatalf("expected 401 without API key, got %d with body %s", recNoKey.Code, recNoKey.Body.String()) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/kliops-api/main_test.go` around lines 58 - 77, TestRoutesProtectAndStripAPIV1Prefix only verifies success with a valid API key; add assertions that the auth middleware rejects requests without a key and with an invalid key. In the same test (or new subtests) reuse mux produced by app.routes and create httptest requests to "/api/v1/agent/ask" with (a) no "X-API-KEY" header and (b) an incorrect "X-API-KEY" value, call mux.ServeHTTP(rec, req) and assert rec.Code == http.StatusUnauthorized (401) and optionally check the response body contains an auth error; keep the existing valid-key assertion intact. Ensure you reference the same mux, handlers.NewGatewayHandler, handlers.NewIngestionHandler, handlers.NewAgentHandler setup so the protection middleware is exercised..github/workflows/deploy.yml (1)
25-29: Pinkubectlversion to a specific release instead oflatest.Line 28 uses
version: latest, which creates non-deterministic behavior in deployments. This can introduce breaking changes from upstream releases that weren't tested in your CI/CD pipeline. Replace with a pinned version (e.g.,version: v1.28.0) matching your cluster's Kubernetes version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 25 - 29, The workflow step named "Set up kubectl" currently uses azure/setup-kubectl@v4 with version: latest which is non-deterministic; update that step to pin a specific kubectl release (for example replace version: latest with version: v1.28.0 or the exact Kubernetes minor/patch that matches your cluster) so the azure/setup-kubectl@v4 action always installs a known, tested kubectl binary.deployments/k8s/kliops.yaml (3)
63-103: AddsecurityContextto harden the container.Static analysis flags that the container lacks security hardening. Adding a security context prevents privilege escalation and reduces attack surface.
🛡️ Proposed fix to add securityContext
spec: + securityContext: + runAsNonRoot: true + runAsUser: 65534 + fsGroup: 65534 containers: - name: kliops-api image: IMAGE_REPOSITORY_PLACEHOLDER:IMAGE_TAG_PLACEHOLDER imagePullPolicy: IfNotPresent + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL ports: - name: http containerPort: 8070If
readOnlyRootFilesystem: truecauses issues (e.g., temp file writes), add anemptyDirvolume for/tmp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/k8s/kliops.yaml` around lines 63 - 103, Add a Pod securityContext for the kliops-api container to harden it: in the container spec for name "kliops-api" set securityContext fields like runAsNonRoot: true, runAsUser (e.g., 1000), allowPrivilegeEscalation: false, capabilities.drop: ["ALL"], readOnlyRootFilesystem: true and set a seccompProfile (runtime/default); if readOnlyRootFilesystem prevents necessary temp writes, add an emptyDir volume and mount it at /tmp via volumeMounts and volumes entries to preserve functionality.
130-131: Usespec.ingressClassNameinstead of the deprecated annotation.The
kubernetes.io/ingress.classannotation is deprecated since Kubernetes 1.18. Use theingressClassNamefield instead.♻️ Proposed fix
metadata: name: kliops-api namespace: NAMESPACE_PLACEHOLDER - annotations: - kubernetes.io/ingress.class: nginx spec: + ingressClassName: nginx rules:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/k8s/kliops.yaml` around lines 130 - 131, Replace the deprecated metadata annotation "kubernetes.io/ingress.class: nginx" with the Ingress spec field by removing that annotation and adding spec.ingressClassName: nginx under the Ingress manifest's spec; locate the Ingress resource that currently sets the annotation and move the class value into the spec.ingressClassName field (ensuring it sits at the same top-level under spec) so the manifest uses the modern ingressClassName property instead of the deprecated annotation.
26-34: Placeholder secrets should be clearly documented or externalized.The
stringDatacontains placeholder values like"change-me"and default credentials. While acceptable for a template, consider adding a comment or using external secret management (e.g., Sealed Secrets, External Secrets Operator) to prevent accidental deployment with insecure defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/k8s/kliops.yaml` around lines 26 - 34, The Kubernetes Secret manifest contains insecure placeholder values in stringData (e.g., MINIO_ROOT_PASSWORD, API_KEY_SECRET, DATABASE_URL with embedded creds); update the manifest to remove plaintext defaults by either replacing these entries with references to externally managed secrets (e.g., ExternalSecrets/SealedSecrets/SecretRef) or clearly document each placeholder with an inline comment explaining it must be replaced before deploy and how to provision it (include the key names MINIO_ROOT_PASSWORD, API_KEY_SECRET, DATABASE_URL, RABBITMQ_URL, QDRANT_ADDR); ensure no real credentials remain in the template and add a short comment at the top pointing to the chosen secret-management workflow.internal/core/services/agent_service.go (2)
147-156: Email validation is minimal — consider edge cases.The
isValidEmailfunction only checks for@and.positions. It will accept malformed addresses likea@b.(if the string ends right after the dot check) or addresses with invalid characters. For a production system handling business email addresses, consider usingnet/mail.ParseAddressor a more robust check.💡 Alternative using net/mail
import "net/mail" func isValidEmail(email string) bool { _, err := mail.ParseAddress(email) return err == nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/agent_service.go` around lines 147 - 156, The current isValidEmail function performs only basic positional checks and can accept malformed addresses; replace its implementation to use net/mail.ParseAddress for robust validation by updating isValidEmail to call mail.ParseAddress and return err == nil, and add the "net/mail" import; ensure the function name is unchanged so callers remain the same.
115-145: Validation mutates the input request.The
validateTenderRequestfunction modifies the inputreqby trimming fields (lines 120-123). This is a side effect that may surprise callers. Consider either:
- Documenting this behavior clearly, or
- Returning a new sanitized struct instead of mutating the input.
Current approach works but couples validation with sanitization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/agent_service.go` around lines 115 - 145, The validator currently mutates the input in validateTenderRequest by trimming fields (DCEContent, ProjectName, TargetEmail, ClientName); change it to return a sanitized copy and an error instead of mutating the original: update validateTenderRequest signature to return (*TenderRequest, error) or create a new helper sanitizeTender(req *TenderRequest) *TenderRequest that produces a trimmed copy, run validations against that copy (keep all current length/email checks) and return the sanitized struct with nil error or the validation error, and update callers to use the returned sanitized TenderRequest; ensure all references to validation use the new non-mutating behavior to avoid side effects.internal/adapters/agent/multi_agent.go (1)
65-85: Missing nil validation for injected services.The constructor
NewKliopsOrchestratorWithModeldoesn't validate thatknowledge,pricing, anddocumentservices are non-nil. This could lead to nil pointer dereferences inexecuteToolif a caller forgets to provide a service.🛡️ Proposed fix to add nil checks
func NewKliopsOrchestratorWithModel( ollamaBaseURL string, model string, knowledge *services.KnowledgeService, pricing *services.PricingService, document *services.DocumentService, ) *KliopsOrchestrator { + if knowledge == nil { + panic("orchestrator: KnowledgeService must not be nil") + } + if pricing == nil { + panic("orchestrator: PricingService must not be nil") + } + if document == nil { + panic("orchestrator: DocumentService must not be nil") + } if model == "" { model = llmModel }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/agent/multi_agent.go` around lines 65 - 85, NewKliopsOrchestratorWithModel must validate that the injected services (knowledge, pricing, document) are non-nil to avoid nil dereferences in methods like executeTool; add explicit nil checks inside NewKliopsOrchestratorWithModel and return a clear failure (e.g., panic or log.Fatalf) with a descriptive message naming the missing service(s) if any are nil, then only construct and return &KliopsOrchestrator when all required services are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.dockerignore:
- Around line 3-4: The .dockerignore currently excludes only the exact filenames
".env" and "credentials.json"; broaden these entries to glob patterns so
variants can't slip into the build context—replace or augment the exact names
with patterns like ".env*", "credentials*.json" and optionally
"**/credentials/*.json" (or similar globs) so files such as ".env.local",
"mycredentials.json" or credential files in subdirectories are ignored; update
the .dockerignore entries that reference ".env" and "credentials.json"
accordingly.
In @.github/workflows/deploy.yml:
- Around line 37-43: The workflow step "Render deployment image" currently
injects raw user inputs (${ { github.event.inputs.image_tag }} and ${ {
github.event.inputs.namespace }}) directly into sed replacements
(IMAGE_TAG_PLACEHOLDER, NAMESPACE_PLACEHOLDER), which is unsafe; add a
pre-render validation/sanitization step that (1) validates namespace against a
whitelist/regex (e.g. allowed k8s namespace pattern) and fails early if invalid,
and (2) sanitizes/escapes the image tag and namespace for use in sed (or better,
avoid sed and use a safe templating approach like yq/jq to write the fields).
Ensure the new step runs before "Render deployment image", references the same
placeholders IMAGE_REPOSITORY_PLACEHOLDER / IMAGE_TAG_PLACEHOLDER /
NAMESPACE_PLACEHOLDER, and emits only safe, escaped env vars used by the sed
command (or updates the manifest via yq) so special characters cannot break the
replacement.
In `@cmd/kliops-api/main.go`:
- Around line 224-230: The server's WriteTimeout (app.server.WriteTimeout) is
30s which will cut off the long-running agentHandler (referenced in
internal/adapters/handlers/agent_handler.go) that expects ~10 minutes; fix by
either increasing or disabling the global WriteTimeout (set WriteTimeout:
10*time.Minute or 0) in the http.Server initialization where app.server is
created (the recoveryMiddleware(app.routes(gatewayHandler, ingestionHandler,
agentHandler)) block), or better yet create a separate http.Server instance that
mounts only the agentHandler with a longer WriteTimeout while keeping the main
server unchanged.
In `@deployments/migrations/20260405103115-init_knowledge_base.sql`:
- Line 60: The column code_article is declared UNIQUE but allows multiple NULLs
in PostgreSQL; update the table definition in the migration so code_article is
non-nullable by adding NOT NULL (e.g., change "code_article VARCHAR(100)
UNIQUE," to "code_article VARCHAR(100) NOT NULL UNIQUE," or add an explicit
ALTER TABLE ... ALTER COLUMN code_article SET NOT NULL) to enforce the lookup
identifier constraint.
In `@Dockerfile`:
- Around line 12-13: The Dockerfile hardcodes GOARCH=amd64 in the RUN build
line, breaking multi-arch releases; add a build ARG for the target architecture
(e.g., ARG TARGETARCH) and replace the hardcoded GOARCH=amd64 with
GOARCH=${TARGETARCH} (or set a default ARG TARGETARCH=amd64) so the go build
invocation (the RUN line with CGO_ENABLED=0 GOOS=linux GOARCH=... go build
-trimpath -ldflags="-s -w -X main.version=${VERSION}" -o /out/kliops-api
./cmd/kliops-api) respects the platform passed by buildx. Ensure ARG TARGETARCH
appears before the RUN that executes go build.
In `@internal/adapters/agent/multi_agent.go`:
- Line 9: CI fails because the imported package openai
("github.com/sashabaranov/go-openai") lacks a go.sum entry; run `go mod tidy` in
the module root to update go.sum (and optionally run `go build`/`go test`) so
the checksum for the dependency is recorded, then commit the updated go.sum;
locate the import in internal/adapters/agent/multi_agent.go (openai
"github.com/sashabaranov/go-openai") to confirm the dependency being added.
In `@internal/adapters/handlers/gateway.go`:
- Line 14: The repository is missing the Go checksum file required for module
verification (go.sum) referenced by the import "github.com/jackc/pgx/v5" in
gateway.go; run `go mod tidy` in the module root to regenerate go.sum (ensuring
checksums for github.com/jackc/pgx/v5 v5.9.1 are recorded), then add and commit
the generated go.sum to the repo so builds succeed.
In `@internal/adapters/queue/rabbitmq.go`:
- Around line 132-133: The semaphore creation and reacquire are unsafe: ensure
maxConcurrency is validated and that semaphore acquire observes ctx
cancellation. Before creating sem (variable sem in rabbitmq.go) coerce
maxConcurrency to a sane positive value (e.g., if maxConcurrency <= 0 set
maxConcurrency = 1) and then create sem := make(chan struct{}, maxConcurrency);
when acquiring the token (the blocking send sem <- struct{}{} currently used
around the worker/retry path) replace the unconditional send with a select that
either sends into sem or returns/propagates ctx.Err() on <-ctx.Done(), so
shutdowns and retries cannot hang; keep the corresponding release (<-sem)
unchanged.
In `@internal/core/services/archive_service.go`:
- Around line 152-157: The memoire upload is using the same object key pattern
as DCE and can overwrite files with identical filenames; modify the upload key
to include the document type to prevent collisions—either pass the doc type into
UploadFileFromZip (callsite: UploadFileFromZip(ctx, zipReader,
manifest.FichierMEM, projID, "MEMOIRE")) and ensure UploadFileFromZip builds
keys like "<projID>/MEMOIRE/<filename>" (or otherwise prefix/suffix the
filename) instead of "<projID>/<filename>", or update UploadFileFromZip to
derive a type-specific subpath when the "MEMOIRE" tag is provided so MEMOIRE and
DCE objects cannot collide.
In `@internal/core/services/knowledge_service.go`:
- Line 25: The call to s.VectorDB.SearchSimilar can panic if s.VectorDB is nil;
add a nil-guard before invoking SearchSimilar (in the method that contains the
line calling s.VectorDB.SearchSimilar) that checks if s.VectorDB == nil and
returns a clear error (or a safe default) instead of calling SearchSimilar, so
the function (and any variables like results, err) are only used after
confirming s.VectorDB is non-nil.
In `@README.md`:
- Line 11: Multiple fenced code blocks use bare backticks (``` or indented "
```") without language identifiers, which fails markdownlint MD040; update each
bare fence to include an appropriate language tag (e.g., change ``` to ```text
for plain output blocks and to ```sh for shell snippets) so every
triple-backtick block has a language identifier; search for occurrences of ```
and " ```" in README.md and replace them with the correct tagged variants
(e.g., ```text, ```sh) matching the block content.
---
Outside diff comments:
In `@internal/adapters/queue/rabbitmq.go`:
- Around line 134-145: The code currently spawns a goroutine per incoming
delivery (the anonymous go func(delivery amqp.Delivery) block) before attempting
to acquire the concurrency semaphore, allowing an unbounded number of waiting
goroutines; change the flow so the semaphore is acquired synchronously from the
receiving loop (the msgs/select handling with ctx.Done()) before launching the
per-delivery goroutine, then pass the delivery into the goroutine which
processes it and defers releasing the semaphore (ensure the release happens even
on errors), and preserve handling for closed msgs and ctx cancellation;
reference the msgs channel, ctx, the amqp.Delivery anonymous goroutine, and the
acquired/release logic when making the change.
---
Nitpick comments:
In @.github/workflows/deploy.yml:
- Around line 25-29: The workflow step named "Set up kubectl" currently uses
azure/setup-kubectl@v4 with version: latest which is non-deterministic; update
that step to pin a specific kubectl release (for example replace version: latest
with version: v1.28.0 or the exact Kubernetes minor/patch that matches your
cluster) so the azure/setup-kubectl@v4 action always installs a known, tested
kubectl binary.
In `@cmd/kliops-api/main_test.go`:
- Around line 58-77: TestRoutesProtectAndStripAPIV1Prefix only verifies success
with a valid API key; add assertions that the auth middleware rejects requests
without a key and with an invalid key. In the same test (or new subtests) reuse
mux produced by app.routes and create httptest requests to "/api/v1/agent/ask"
with (a) no "X-API-KEY" header and (b) an incorrect "X-API-KEY" value, call
mux.ServeHTTP(rec, req) and assert rec.Code == http.StatusUnauthorized (401) and
optionally check the response body contains an auth error; keep the existing
valid-key assertion intact. Ensure you reference the same mux,
handlers.NewGatewayHandler, handlers.NewIngestionHandler,
handlers.NewAgentHandler setup so the protection middleware is exercised.
In `@deployments/k8s/kliops.yaml`:
- Around line 63-103: Add a Pod securityContext for the kliops-api container to
harden it: in the container spec for name "kliops-api" set securityContext
fields like runAsNonRoot: true, runAsUser (e.g., 1000),
allowPrivilegeEscalation: false, capabilities.drop: ["ALL"],
readOnlyRootFilesystem: true and set a seccompProfile (runtime/default); if
readOnlyRootFilesystem prevents necessary temp writes, add an emptyDir volume
and mount it at /tmp via volumeMounts and volumes entries to preserve
functionality.
- Around line 130-131: Replace the deprecated metadata annotation
"kubernetes.io/ingress.class: nginx" with the Ingress spec field by removing
that annotation and adding spec.ingressClassName: nginx under the Ingress
manifest's spec; locate the Ingress resource that currently sets the annotation
and move the class value into the spec.ingressClassName field (ensuring it sits
at the same top-level under spec) so the manifest uses the modern
ingressClassName property instead of the deprecated annotation.
- Around line 26-34: The Kubernetes Secret manifest contains insecure
placeholder values in stringData (e.g., MINIO_ROOT_PASSWORD, API_KEY_SECRET,
DATABASE_URL with embedded creds); update the manifest to remove plaintext
defaults by either replacing these entries with references to externally managed
secrets (e.g., ExternalSecrets/SealedSecrets/SecretRef) or clearly document each
placeholder with an inline comment explaining it must be replaced before deploy
and how to provision it (include the key names MINIO_ROOT_PASSWORD,
API_KEY_SECRET, DATABASE_URL, RABBITMQ_URL, QDRANT_ADDR); ensure no real
credentials remain in the template and add a short comment at the top pointing
to the chosen secret-management workflow.
In `@deployments/migrations/20260405103115-init_knowledge_base.sql`:
- Line 68: The CREATE INDEX idx_mercuriale_code_article on
mercuriale(code_article) is redundant because code_article already has a UNIQUE
constraint that creates the same index; remove the redundant CREATE INDEX
statement (or drop idx_mercuriale_code_article) to avoid duplicate indexes and
write overhead, ensuring only the UNIQUE constraint remains as the index for
mercuriale.code_article.
In `@internal/adapters/agent/multi_agent.go`:
- Around line 65-85: NewKliopsOrchestratorWithModel must validate that the
injected services (knowledge, pricing, document) are non-nil to avoid nil
dereferences in methods like executeTool; add explicit nil checks inside
NewKliopsOrchestratorWithModel and return a clear failure (e.g., panic or
log.Fatalf) with a descriptive message naming the missing service(s) if any are
nil, then only construct and return &KliopsOrchestrator when all required
services are present.
In `@internal/core/services/agent_service_test.go`:
- Around line 47-49: The test currently asserts a hard-coded spaced string in
orch.prompt which is brittle; update the assertion to check semantic content
instead by verifying orch.prompt contains the key and value without relying on
exact padding (e.g., assert that orch.prompt contains "Projet" and "Projet
Elite" or match a regex like "Projet\\s*:\\s*Projet Elite") so the test only
depends on presence of the field and value rather than exact spacing.
In `@internal/core/services/agent_service.go`:
- Around line 147-156: The current isValidEmail function performs only basic
positional checks and can accept malformed addresses; replace its implementation
to use net/mail.ParseAddress for robust validation by updating isValidEmail to
call mail.ParseAddress and return err == nil, and add the "net/mail" import;
ensure the function name is unchanged so callers remain the same.
- Around line 115-145: The validator currently mutates the input in
validateTenderRequest by trimming fields (DCEContent, ProjectName, TargetEmail,
ClientName); change it to return a sanitized copy and an error instead of
mutating the original: update validateTenderRequest signature to return
(*TenderRequest, error) or create a new helper sanitizeTender(req
*TenderRequest) *TenderRequest that produces a trimmed copy, run validations
against that copy (keep all current length/email checks) and return the
sanitized struct with nil error or the validation error, and update callers to
use the returned sanitized TenderRequest; ensure all references to validation
use the new non-mutating behavior to avoid side effects.
In `@internal/core/services/document_service.go`:
- Around line 10-13: The constants configBucket and templateObjectKey are
hardcoded; refactor to accept these values via dependency injection: add fields
(e.g., bucketName, templateKey) to the DocumentService struct and update the
DocumentService constructor (e.g., NewDocumentService) to take them or a config
struct/env-driven Config; replace usages of the constants throughout the file
with the instance fields and remove the package-level constants so tests and
deployments can supply environment-specific values.
In `@internal/core/services/pricing_service_test.go`:
- Around line 153-198: Tests TestPricingService_RegisterAndQuery,
TestPricingService_UnknownSource, TestPricingService_StrategyErrorPropagated and
TestPricingService_NilStrategyIgnored are overlapping; consolidate them into a
single table-driven test that iterates cases and uses t.Run for subtests. Create
a TestPricingService table with entries specifying setup (which strategies to
Register via NewPricingService and RegisterStrategy, including
mockPricingStrategy instances and nil), input args for GetPrice (source and
sku), expected price or expected error (use errors.Is for comparison), and a
name; then iterate the table, set up svc per case, call svc.GetPrice, and assert
results accordingly. Reference NewPricingService, RegisterStrategy, GetPrice and
mockPricingStrategy when implementing each case.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 845ecbca-0ff0-488f-8917-174692aed0b4
⛔ Files ignored due to path filters (2)
Architecture_Kliops.pngis excluded by!**/*.pngcoverage.outis excluded by!**/*.out
📒 Files selected for processing (23)
.dockerignore.github/workflows/ci.yml.github/workflows/deploy.yml.github/workflows/release.ymlDockerfileREADME.mdcmd/kliops-api/main.gocmd/kliops-api/main_test.godeployments/k8s/kliops.yamldeployments/migrations/20260405103115-init_knowledge_base.sqlgo.modinternal/adapters/agent/multi_agent.gointernal/adapters/handlers/agent_handler.gointernal/adapters/handlers/agent_handler_test.gointernal/adapters/handlers/gateway.gointernal/adapters/handlers/middleware_test.gointernal/adapters/queue/rabbitmq.gointernal/core/services/agent_service.gointernal/core/services/agent_service_test.gointernal/core/services/archive_service.gointernal/core/services/document_service.gointernal/core/services/knowledge_service.gointernal/core/services/pricing_service_test.go
Comment on lines
+3
to
+4
| .env | ||
| credentials.json |
Contributor
There was a problem hiding this comment.
Broaden secret-file ignore patterns.
Lines 3-4 only exclude exact filenames. Variants (for example .env.local or differently named credential JSON files) can still leak into Docker build context.
🔧 Proposed hardening
.git
.gitignore
-.env
-credentials.json
+.env
+.env.*
+*.env
+credentials.json
+**/*credentials*.json
+**/*.pem
+**/*.key
bin/
.github/
deployments/docker-compose.yml
**/*_test.go📝 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
| .env | |
| credentials.json | |
| .git | |
| .gitignore | |
| .env | |
| .env.* | |
| *.env | |
| credentials.json | |
| **/*credentials*.json | |
| **/*.pem | |
| **/*.key | |
| bin/ | |
| .github/ | |
| deployments/docker-compose.yml | |
| **/*_test.go |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.dockerignore around lines 3 - 4, The .dockerignore currently excludes only
the exact filenames ".env" and "credentials.json"; broaden these entries to glob
patterns so variants can't slip into the build context—replace or augment the
exact names with patterns like ".env*", "credentials*.json" and optionally
"**/credentials/*.json" (or similar globs) so files such as ".env.local",
"mycredentials.json" or credential files in subdirectories are ignored; update
the .dockerignore entries that reference ".env" and "credentials.json"
accordingly.
Comment on lines
+37
to
+43
| - name: Render deployment image | ||
| run: | | ||
| sed \ | ||
| -e "s|IMAGE_REPOSITORY_PLACEHOLDER|ghcr.io/${{ github.repository_owner }}/kliops|g" \ | ||
| -e "s|IMAGE_TAG_PLACEHOLDER|${{ github.event.inputs.image_tag }}|g" \ | ||
| -e "s|NAMESPACE_PLACEHOLDER|${{ github.event.inputs.namespace }}|g" \ | ||
| deployments/k8s/kliops.yaml > /tmp/kliops-rendered.yaml |
Contributor
There was a problem hiding this comment.
Validate and escape workflow_dispatch inputs before templating.
Line 41–42 interpolate raw user inputs into sed. Special characters can corrupt replacements, and invalid namespaces can render non-applicable manifests.
Suggested hardening
+ - name: Validate deploy inputs
+ run: |
+ [[ "${{ github.event.inputs.namespace }}" =~ ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ ]] || {
+ echo "Invalid namespace"; exit 1;
+ }
+ [[ "${{ github.event.inputs.image_tag }}" =~ ^[A-Za-z0-9_][A-Za-z0-9._-]{0,127}$ ]] || {
+ echo "Invalid image tag"; exit 1;
+ }
+
- name: Render deployment image
run: |
+ esc_tag=$(printf '%s' "${{ github.event.inputs.image_tag }}" | sed 's/[&|]/\\&/g')
+ esc_ns=$(printf '%s' "${{ github.event.inputs.namespace }}" | sed 's/[&|]/\\&/g')
sed \
-e "s|IMAGE_REPOSITORY_PLACEHOLDER|ghcr.io/${{ github.repository_owner }}/kliops|g" \
- -e "s|IMAGE_TAG_PLACEHOLDER|${{ github.event.inputs.image_tag }}|g" \
- -e "s|NAMESPACE_PLACEHOLDER|${{ github.event.inputs.namespace }}|g" \
+ -e "s|IMAGE_TAG_PLACEHOLDER|${esc_tag}|g" \
+ -e "s|NAMESPACE_PLACEHOLDER|${esc_ns}|g" \
deployments/k8s/kliops.yaml > /tmp/kliops-rendered.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/deploy.yml around lines 37 - 43, The workflow step "Render
deployment image" currently injects raw user inputs (${ {
github.event.inputs.image_tag }} and ${ { github.event.inputs.namespace }})
directly into sed replacements (IMAGE_TAG_PLACEHOLDER, NAMESPACE_PLACEHOLDER),
which is unsafe; add a pre-render validation/sanitization step that (1)
validates namespace against a whitelist/regex (e.g. allowed k8s namespace
pattern) and fails early if invalid, and (2) sanitizes/escapes the image tag and
namespace for use in sed (or better, avoid sed and use a safe templating
approach like yq/jq to write the fields). Ensure the new step runs before
"Render deployment image", references the same placeholders
IMAGE_REPOSITORY_PLACEHOLDER / IMAGE_TAG_PLACEHOLDER / NAMESPACE_PLACEHOLDER,
and emits only safe, escaped env vars used by the sed command (or updates the
manifest via yq) so special characters cannot break the replacement.
Comment on lines
+224
to
+230
| app.server = &http.Server{ | ||
| Addr: cfg.Addr, | ||
| Handler: recoveryMiddleware(app.routes(gatewayHandler, ingestionHandler, agentHandler)), | ||
| ReadTimeout: 15 * time.Second, | ||
| WriteTimeout: 30 * time.Second, | ||
| IdleTimeout: 60 * time.Second, | ||
| } |
Contributor
There was a problem hiding this comment.
WriteTimeout of 30s conflicts with agent handler's 10-minute timeout.
The HTTP server's WriteTimeout: 30 * time.Second will terminate the connection before the agent handler can complete its work (which has a 10-minute timeout defined in internal/adapters/handlers/agent_handler.go:19). This will cause the agent endpoint to fail with a broken pipe or reset connection.
For long-running endpoints, you have a few options:
- Remove/increase
WriteTimeoutglobally (but this affects all endpoints) - Use a separate HTTP server for long-running endpoints
- Implement streaming responses or use Server-Sent Events
- Set
WriteTimeout: 0to disable (not recommended for production)
🔧 Quick fix to increase WriteTimeout
app.server = &http.Server{
Addr: cfg.Addr,
Handler: recoveryMiddleware(app.routes(gatewayHandler, ingestionHandler, agentHandler)),
ReadTimeout: 15 * time.Second,
- WriteTimeout: 30 * time.Second,
+ WriteTimeout: 12 * time.Minute, // Must exceed agentTimeout (10min) + buffer
IdleTimeout: 60 * time.Second,
}Note: This affects all endpoints. A more robust solution would be to use per-request timeout handling or a dedicated server for long-running operations.
📝 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
| app.server = &http.Server{ | |
| Addr: cfg.Addr, | |
| Handler: recoveryMiddleware(app.routes(gatewayHandler, ingestionHandler, agentHandler)), | |
| ReadTimeout: 15 * time.Second, | |
| WriteTimeout: 30 * time.Second, | |
| IdleTimeout: 60 * time.Second, | |
| } | |
| app.server = &http.Server{ | |
| Addr: cfg.Addr, | |
| Handler: recoveryMiddleware(app.routes(gatewayHandler, ingestionHandler, agentHandler)), | |
| ReadTimeout: 15 * time.Second, | |
| WriteTimeout: 12 * time.Minute, // Must exceed agentTimeout (10min) + buffer | |
| IdleTimeout: 60 * time.Second, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/kliops-api/main.go` around lines 224 - 230, The server's WriteTimeout
(app.server.WriteTimeout) is 30s which will cut off the long-running
agentHandler (referenced in internal/adapters/handlers/agent_handler.go) that
expects ~10 minutes; fix by either increasing or disabling the global
WriteTimeout (set WriteTimeout: 10*time.Minute or 0) in the http.Server
initialization where app.server is created (the
recoveryMiddleware(app.routes(gatewayHandler, ingestionHandler, agentHandler))
block), or better yet create a separate http.Server instance that mounts only
the agentHandler with a longer WriteTimeout while keeping the main server
unchanged.
|
|
||
| CREATE TABLE mercuriale ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| code_article VARCHAR(100) UNIQUE, |
Contributor
There was a problem hiding this comment.
Make code_article non-nullable.
UNIQUE alone still permits multiple NULL rows in PostgreSQL. For a lookup identifier, this weakens data integrity.
🛠️ Proposed fix
- code_article VARCHAR(100) UNIQUE,
+ code_article VARCHAR(100) UNIQUE NOT NULL,📝 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
| code_article VARCHAR(100) UNIQUE, | |
| code_article VARCHAR(100) UNIQUE NOT NULL, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deployments/migrations/20260405103115-init_knowledge_base.sql` at line 60,
The column code_article is declared UNIQUE but allows multiple NULLs in
PostgreSQL; update the table definition in the migration so code_article is
non-nullable by adding NOT NULL (e.g., change "code_article VARCHAR(100)
UNIQUE," to "code_article VARCHAR(100) NOT NULL UNIQUE," or add an explicit
ALTER TABLE ... ALTER COLUMN code_article SET NOT NULL) to enforce the lookup
identifier constraint.
Comment on lines
+12
to
+13
| RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 \ | ||
| go build -trimpath -ldflags="-s -w -X main.version=${VERSION}" -o /out/kliops-api ./cmd/kliops-api |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify mismatch between release multi-arch settings and Dockerfile compile target.
rg -n 'platforms:\s*linux/amd64,linux/arm64|GOARCH=amd64|TARGETARCH|TARGETOS' Dockerfile .github/workflows/release.yml
# Expected: release advertises arm64, Dockerfile currently hardcodes amd64.Repository: MiltonJ23/Kliops
Length of output: 195
Multi-arch release is broken by hardcoded GOARCH=amd64.
The Dockerfile forces an amd64 binary (line 12) while .github/workflows/release.yml declares platforms: linux/amd64,linux/arm64 (line 49), causing arm64 builds to fail at runtime.
Fix build to respect target platform
ARG VERSION=dev
-RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 \
+ARG TARGETOS
+ARG TARGETARCH
+RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH:-amd64} \
go build -trimpath -ldflags="-s -w -X main.version=${VERSION}" -o /out/kliops-api ./cmd/kliops-api📝 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
| RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 \ | |
| go build -trimpath -ldflags="-s -w -X main.version=${VERSION}" -o /out/kliops-api ./cmd/kliops-api | |
| ARG TARGETOS | |
| ARG TARGETARCH | |
| RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH:-amd64} \ | |
| go build -trimpath -ldflags="-s -w -X main.version=${VERSION}" -o /out/kliops-api ./cmd/kliops-api |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 12 - 13, The Dockerfile hardcodes GOARCH=amd64 in
the RUN build line, breaking multi-arch releases; add a build ARG for the target
architecture (e.g., ARG TARGETARCH) and replace the hardcoded GOARCH=amd64 with
GOARCH=${TARGETARCH} (or set a default ARG TARGETARCH=amd64) so the go build
invocation (the RUN line with CGO_ENABLED=0 GOOS=linux GOARCH=... go build
-trimpath -ldflags="-s -w -X main.version=${VERSION}" -o /out/kliops-api
./cmd/kliops-api) respects the platform passed by buildx. Ensure ARG TARGETARCH
appears before the RUN that executes go build.
|
|
||
| "github.com/MiltonJ23/Kliops/internal/core/ports" | ||
| "github.com/MiltonJ23/Kliops/internal/core/services" | ||
| "github.com/jackc/pgx/v5" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking pgx requirement in go.mod..."
rg -n 'github.com/jackc/pgx/v5' go.mod
echo "Checking pgx checksums in go.sum..."
rg -n 'github.com/jackc/pgx/v5' go.sum || true
echo
echo "Expected: at least one matching go.sum entry for github.com/jackc/pgx/v5."Repository: MiltonJ23/Kliops
Length of output: 306
Generate and commit go.sum file for Go module dependencies.
The go.sum file is missing from the repository, even though go.mod correctly declares github.com/jackc/pgx/v5 v5.9.1. Without go.sum, the build will fail. Run go mod tidy to regenerate the checksums file and commit it.
🧰 Tools
🪛 GitHub Check: test-and-build
[failure] 14-14:
missing go.sum entry for module providing package github.com/jackc/pgx/v5 (imported by github.com/MiltonJ23/Kliops/internal/adapters/handlers); to add:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/adapters/handlers/gateway.go` at line 14, The repository is missing
the Go checksum file required for module verification (go.sum) referenced by the
import "github.com/jackc/pgx/v5" in gateway.go; run `go mod tidy` in the module
root to regenerate go.sum (ensuring checksums for github.com/jackc/pgx/v5 v5.9.1
are recorded), then add and commit the generated go.sum to the repo so builds
succeed.
Comment on lines
+132
to
133
| sem := make(chan struct{}, maxConcurrency) | ||
|
|
Contributor
There was a problem hiding this comment.
Semaphore usage can deadlock/hang under valid runtime conditions.
Two issues here:
- If
maxConcurrency <= 0, Line 132 creates a non-functional semaphore and workers can block forever. - Line 186 re-acquires with a blocking send that does not observe
ctx.Done(), which can hang shutdown/retry paths.
✅ Safer semaphore handling
func (r *RabbitMQAdapter) ConsumeJob(ctx context.Context, maxConcurrency int, handler func(ctx context.Context, jobID, projectID string) error) error {
+ if maxConcurrency <= 0 {
+ return fmt.Errorf("maxConcurrency must be > 0")
+ }
msgs, channelCreationError := r.Channel.Consume(QueueName, "", false, false, false, false, nil)
@@
- case <-time.After(time.Duration(msg.Retry*5) * time.Second):
- sem <- struct{}{}
- acquired = true
+ case <-time.After(time.Duration(msg.Retry*5) * time.Second):
+ select {
+ case sem <- struct{}{}:
+ acquired = true
+ case <-ctx.Done():
+ delivery.Nack(false, true)
+ return
+ }Also applies to: 185-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/adapters/queue/rabbitmq.go` around lines 132 - 133, The semaphore
creation and reacquire are unsafe: ensure maxConcurrency is validated and that
semaphore acquire observes ctx cancellation. Before creating sem (variable sem
in rabbitmq.go) coerce maxConcurrency to a sane positive value (e.g., if
maxConcurrency <= 0 set maxConcurrency = 1) and then create sem := make(chan
struct{}, maxConcurrency); when acquiring the token (the blocking send sem <-
struct{}{} currently used around the worker/retry path) replace the
unconditional send with a select that either sends into sem or
returns/propagates ctx.Err() on <-ctx.Done(), so shutdowns and retries cannot
hang; keep the corresponding release (<-sem) unchanged.
Comment on lines
+152
to
+157
| memPaths, uploadingMemoireErr := s.UploadFileFromZip(ctx, zipReader, manifest.FichierMEM, projID, "MEMOIRE") | ||
| if uploadingMemoireErr != nil { | ||
| return fmt.Errorf("an error occurred while trying to upload the memoire file to the bucket") | ||
| } | ||
| uploadedPaths = append(uploadedPaths, memPaths...) | ||
| for _, path := range memPaths { |
Contributor
There was a problem hiding this comment.
Prevent DCE/MEMOIRE storage key collisions.
At Line 152, MEMOIRE uploads use the same key pattern as DCE (<projectId>/<filename>). If both docs share a filename, one can overwrite the other and both types may point to the same object.
🔧 Proposed fix
diff --git a/internal/core/services/archive_service.go b/internal/core/services/archive_service.go
@@
func (s *ArchiveService) UploadFileFromZip(ctx context.Context, z *zip.ReadCloser, filename, projectId, docType string) ([]string, error) {
@@
- path, uploadingFileError := s.Storage.Upload(ctx, "dce-archive", fmt.Sprintf("%s/%s", projectId, filename), rc, int64(f.UncompressedSize64), contentType)
+ objectKey := fmt.Sprintf("%s/%s/%s", projectId, strings.ToLower(docType), filepath.Base(filename))
+ path, uploadingFileError := s.Storage.Upload(ctx, "dce-archive", objectKey, rc, int64(f.UncompressedSize64), contentType)
if uploadingFileError != nil {
return nil, fmt.Errorf("an error occurred while uploading file %s , into the bucket:%v", filename, uploadingFileError)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/archive_service.go` around lines 152 - 157, The
memoire upload is using the same object key pattern as DCE and can overwrite
files with identical filenames; modify the upload key to include the document
type to prevent collisions—either pass the doc type into UploadFileFromZip
(callsite: UploadFileFromZip(ctx, zipReader, manifest.FichierMEM, projID,
"MEMOIRE")) and ensure UploadFileFromZip builds keys like
"<projID>/MEMOIRE/<filename>" (or otherwise prefix/suffix the filename) instead
of "<projID>/<filename>", or update UploadFileFromZip to derive a type-specific
subpath when the "MEMOIRE" tag is provided so MEMOIRE and DCE objects cannot
collide.
|
|
||
| // RetrieveRelevantMethodologies fetch pertinent historique reponses from Qdrant | ||
| func (s *KnowledgeService) RetrieveRelevantMethodologies(ctx context.Context, exigence string) (string, error) { | ||
| results, err := s.VectorDB.SearchSimilar(ctx, exigence, 3) |
Contributor
There was a problem hiding this comment.
Guard against nil VectorDB before use.
Line 25 will panic if the service is misconfigured with a nil vector DB dependency.
🛡️ Proposed guard
func (s *KnowledgeService) RetrieveRelevantMethodologies(ctx context.Context, exigence string) (string, error) {
+ if s == nil || s.VectorDB == nil {
+ return "", fmt.Errorf("knowledge service is not properly initialized")
+ }
results, err := s.VectorDB.SearchSimilar(ctx, exigence, 3)📝 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
| results, err := s.VectorDB.SearchSimilar(ctx, exigence, 3) | |
| func (s *KnowledgeService) RetrieveRelevantMethodologies(ctx context.Context, exigence string) (string, error) { | |
| if s == nil || s.VectorDB == nil { | |
| return "", fmt.Errorf("knowledge service is not properly initialized") | |
| } | |
| results, err := s.VectorDB.SearchSimilar(ctx, exigence, 3) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/knowledge_service.go` at line 25, The call to
s.VectorDB.SearchSimilar can panic if s.VectorDB is nil; add a nil-guard before
invoking SearchSimilar (in the method that contains the line calling
s.VectorDB.SearchSimilar) that checks if s.VectorDB == nil and returns a clear
error (or a safe default) instead of calling SearchSimilar, so the function (and
any variables like results, err) are only used after confirming s.VectorDB is
non-nil.
|
|
||
| ## Architecture | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (markdownlint MD040).
These fences currently omit language tags, which can fail docs linting.
Minimal markdownlint-compliant update
-```
+```text
@@
-```
+```text
@@
-```
+```text
@@
- ```
+ ```sh
@@
-```
+```textAlso applies to: 40-40, 55-55, 233-233, 368-368
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 11, Multiple fenced code blocks use bare backticks (``` or
indented " ```") without language identifiers, which fails markdownlint MD040;
update each bare fence to include an appropriate language tag (e.g., change ```
to ```text for plain output blocks and to ```sh for shell snippets) so every
triple-backtick block has a language identifier; search for occurrences of ```
and " ```" in README.md and replace them with the correct tagged variants
(e.g., ```text, ```sh) matching the block content.
Owner
Author
|
@copilot resolve the merge conflicts in this pull request |
Resolve conflicts in: - .github/workflows/ci.yml: keep HEAD CI workflow (vet, test, build) - Dockerfile: keep HEAD version, add tzdata to builder stage - README.md: keep HEAD comprehensive documentation - internal/core/services/document_service.go: keep HEAD clean version with named constants Co-authored-by: MiltonJ23 <151161809+MiltonJ23@users.noreply.github.com>
Contributor
Merge conflicts have been resolved in commit
The branch now also includes the new files from |
Owner
Author
|
@copilot apply changes based on the comments in this thread Go and look at the main branch, do not broke things there, find the correct changes to make here to bring what is missing onto the main branch, ensure yourself it passes the tests before going with it Make sure all the remarks of coderabbit are taken into account, make sure to not break the main branch content, and only add things there notably concerning the readme file, add things that are missing there, but since its pretty good looking, ensure the look always stays great |
Copilot AI
added a commit
that referenced
this pull request
Apr 15, 2026
…aphore, archive key collision Agent-Logs-Url: https://github.com/MiltonJ23/Kliops/sessions/f43177bc-d75f-4cc8-a6e0-4aba41dff780 Co-authored-by: MiltonJ23 <151161809+MiltonJ23@users.noreply.github.com>
MiltonJ23
added a commit
that referenced
this pull request
Apr 15, 2026
Apply PR #5 review feedback: dockerignore hardening, semaphore fix, storage key collision, README lint, Kubernetes manifest
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
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.
This pull request sets up the foundational infrastructure for CI/CD, containerization, and documentation of the Kliops platform. It introduces Docker support, comprehensive GitHub Actions workflows for CI, release, and deployment, and a detailed
README.mddescribing the architecture, API, configuration, and development process. Additionally, it adds an initial test for the main application entrypoint.The most important changes are:
CI/CD Workflows:
.github/workflows/ci.ymlfor running vet, tests (with race and coverage), and building the API binary on push and pull requests..github/workflows/release.ymlto build and push multi-arch Docker images to GHCR on tag pushes and manual triggers..github/workflows/deploy.ymlfor manual Kubernetes deployments with image tag and namespace inputs, rendering manifests and applying them viakubectl.Containerization:
Dockerfileto build and package the Go API as a statically linked binary, using a non-root user and including runtime dependencies..dockerignoreto exclude development files, test binaries, and sensitive credentials from Docker build context.Documentation:
README.mdwith architecture diagrams, API reference, configuration, development, testing, deployment instructions, and project structure.Testing:
cmd/kliops-api/main_test.gowith tests for configuration loading and API route protection/stripping, using fake dependencies for isolation.Summary by CodeRabbit
New Features
Documentation
Chores