.github/workflows/docker.yml (2)

9-40: Good structure for version extraction with validation.

The extract-version job correctly validates that both version and chart_name are non-empty before proceeding. However, note that yq returns the literal string null (not empty) when a key is missing.

🔎 Suggested fix to handle null values from yq

      - name: Extract version from .cz.yaml
        id: version
        run: |
          VERSION=$(yq '.commitizen.version' .cz.yaml)
-          if [[ -z "$VERSION" ]]; then
+          if [[ -z "$VERSION" || "$VERSION" == "null" ]]; then
            echo "Error: Could not extract version from .cz.yaml"
            exit 1
          fi
          echo "version=$VERSION" >> "$GITHUB_OUTPUT"

      - name: Helm Chart Name
        id: chartName
        run: |
          CHART_NAME=$(yq '.name' helm/Chart.yaml)
-          if [[ -z "$CHART_NAME" ]]; then
+          if [[ -z "$CHART_NAME" || "$CHART_NAME" == "null" ]]; then
            echo "Error: Could not extract chart name from helm/Chart.yaml"
            exit 1
          fi
          echo "chart_name=$CHART_NAME" >> $GITHUB_OUTPUT

---

116-116: Dependency chain may cause build-images to be skipped unintentionally.

When extract-version is skipped (non-bump commits), create-manifests will also be skipped because it depends on extract-version. However, build-images will still run, consuming CI resources without creating manifests or deploying.

Consider either:

1. Adding the same if condition to build-images, or
2. Using if: always() && needs.build-images.result == 'success' pattern if you want builds to run independently

helm/templates/deployment.yaml (1)

139-149: startupProbe uses readiness probe values instead of dedicated startup values.

The startupProbe configuration reuses .Values.probes.readiness.* defaults. For startup probes, you typically want longer initialDelaySeconds and higher failureThreshold to allow for slower cold starts without the probe configuration being counterintuitive.

Consider adding dedicated .Values.probes.startup.* configuration values:

          startupProbe:
            httpGet:
              path: /health
              port: management
-            initialDelaySeconds: {{ .Values.probes.readiness.initialDelaySeconds | default 10 }}
-            periodSeconds: {{ .Values.probes.readiness.periodSeconds | default 10 }}
-            timeoutSeconds: {{ .Values.probes.readiness.timeoutSeconds | default 5 }}
-            successThreshold: {{ .Values.probes.readiness.successThreshold | default 1 }}
-            failureThreshold: {{ .Values.probes.readiness.failureThreshold | default 3 }}
+            initialDelaySeconds: {{ .Values.probes.startup.initialDelaySeconds | default 5 }}
+            periodSeconds: {{ .Values.probes.startup.periodSeconds | default 5 }}
+            timeoutSeconds: {{ .Values.probes.startup.timeoutSeconds | default 5 }}
+            successThreshold: {{ .Values.probes.startup.successThreshold | default 1 }}
+            failureThreshold: {{ .Values.probes.startup.failureThreshold | default 30 }}

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

helm/templates/deployment.yaml (1)

91-99: Issue from previous review has been resolved.

The environment variable ordering issue identified in the previous review has been fixed. DB_PASSWORD (lines 92-96) now correctly appears before DATABASE_URL (lines 97-98) in the else branch, allowing Kubernetes to properly substitute $(DB_PASSWORD) in the DATABASE_URL value.

helm/templates/deployment.yaml (1)

139-149: Consider dedicated startup probe configuration.

The startupProbe reuses all timing values from .Values.probes.readiness.* (lines 144-148). While this keeps the configuration DRY, startup probes typically benefit from different thresholds—particularly a higher failureThreshold to allow slow-starting applications more time before being marked as failed.

If the readiness timing is appropriate for startup checks, this is fine. Otherwise, consider adding .Values.probes.startup.* to values.yaml for independent tuning.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

helm/values.yaml (1)

72-72: LGTM!

The comment now clearly indicates this configuration section is for OSS mode only and not used when management.enabled=true. This addresses the previous review feedback.

helm/templates/migration-job.yaml (1)

49-55: Add default for existingSecret for consistency with deployment.yaml.

Line 52 uses .Values.management.database.existingSecret without a fallback, unlike the else branch pattern in deployment.yaml. This was flagged in a previous review and should be addressed for defensive templating.

🔎 Proposed fix

            - name: DB_PASSWORD
              valueFrom:
                secretKeyRef:
-                  name: {{ .Values.management.database.existingSecret }}
+                  name: {{ .Values.management.database.existingSecret | default (printf "%s-db-secret" (include "app.fullname" .)) }}
                   key: {{ .Values.management.database.passwordKey }}

helm/templates/deployment.yaml (1)

92-98: Add default for existingSecret for defensive templating.

Similar to migration-job.yaml, line 95 uses .Values.management.database.existingSecret without a fallback. While values.yaml provides a default, the template should be defensive in case of misconfiguration.

🔎 Proposed fix

            - name: DB_PASSWORD
              valueFrom:
                secretKeyRef:
-                  name: {{ .Values.management.database.existingSecret }}
+                  name: {{ .Values.management.database.existingSecret | default (printf "%s-db-secret" (include "app.fullname" .)) }}
                   key: {{ .Values.management.database.passwordKey }}

helm/values.yaml (1)

78-92: YAML structure issue: Example comments placed after empty list declarations.

The example comments appear after models: [] and pipelines: [], which is syntactically correct but unconventional. Users might expect the examples to be commented-out entries within the list. Consider restructuring:

🔎 Suggested restructure for clarity

   # Define models available in the hub
-  models: []
-    # Example:
-    # - key: gpt-4o
-    #   type: gpt-4o
-    #   provider: openai
+  models:
+    # - key: gpt-4o
+    #   type: gpt-4o
+    #   provider: openai
   # Define pipelines for routing
-  pipelines: []
-    # Example:
-    # - name: default
-    #   type: chat
-    #   plugins:
-    #     - model-router:
-    #         models:
-    #           - gpt-4o
+  pipelines:
+    # - name: default
+    #   type: chat
+    #   plugins:
+    #     - model-router:
+    #         models:
+    #           - gpt-4o

helm/templates/migration-job.yaml (1)

58-59: Consider using values from values.yaml instead of hardcoded values.

backoffLimit and activeDeadlineSeconds are hardcoded to 3 and 300, but values.yaml defines configurable values at management.migrations.backoffLimit and management.migrations.activeDeadlineSeconds. Use the template references for consistency.

🔎 Proposed fix

-  backoffLimit: 3
-  activeDeadlineSeconds: 300
+  backoffLimit: {{ .Values.management.migrations.backoffLimit }}
+  activeDeadlineSeconds: {{ .Values.management.migrations.activeDeadlineSeconds }}

helm/templates/deployment.yaml (3)

138-148: startupProbe reuses readiness probe configuration values.

The startupProbe uses .Values.probes.readiness.* values instead of dedicated startup probe configuration. This works but may cause confusion when tuning probes independently. Consider either:

1. Adding a dedicated probes.startup section in values.yaml, or
2. Adding a comment explaining the intentional reuse of readiness values.

---

156-156: Remove trailing comment or add content.

This comment line # Management mode doesn't need config file volume is orphaned after the {{- end }} block. It appears after the volumeMounts section ends, which is slightly confusing. Consider moving it inside the conditional block or removing it.

---

168-168: Remove trailing comment or add content.

Similar to line 156, this comment # Management mode doesn't need config volumes appears after the conditional block ends. Consider moving it inside the {{- if not .Values.management.enabled }} block or removing it for cleaner template output.

Cargo.toml (2)

75-75: Consider aligning dev-dependencies with production TLS configuration.

The dev-dependencies sqlx uses runtime-tokio while production uses runtime-tokio-native-tls. This creates an environment inconsistency where tests run without TLS but production enforces it.

If this is intentional (e.g., testcontainers don't require TLS), consider documenting it. Otherwise, aligning both to use runtime-tokio-native-tls would better match production behavior.

🔎 Proposed alignment with production configuration

-sqlx = { version = "0.8", features = ["runtime-tokio", "postgres", "migrate"] }
+sqlx = { version = "0.8", features = ["runtime-tokio-native-tls", "postgres", "migrate"] }

---

54-54: Consider using the split TLS feature approach for sqlx 0.8.

The runtime-tokio-native-tls feature is available in sqlx 0.8, but it's soft-deprecated. The recommended approach is to use runtime-tokio combined with tls-native-tls instead:

sqlx = { version = "0.8", features = ["runtime-tokio", "tls-native-tls", "postgres", "macros", "chrono", "uuid", "json", "migrate"] }

Additionally, consider whether dev-dependencies (line 75) should also include TLS features for consistency with your production environment, or if the current test-only configuration (runtime-tokio without TLS) is intentional for testcontainers usage.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

helm/templates/deployment.yaml (1)

145-155: Consider dedicated configuration for startupProbe.

The startupProbe currently reuses values from .Values.probes.readiness.* (e.g., line 150), which may not be optimal for startup scenarios. Startup probes typically require different timing characteristics than readiness probes.

💡 Suggested improvement

Define dedicated startup probe configuration in values.yaml:

probes:
  startup:
    initialDelaySeconds: 0
    periodSeconds: 5
    timeoutSeconds: 5
    successThreshold: 1
    failureThreshold: 30  # Allow longer startup time
  readiness:
    # ... existing config

Then reference these in the template:

 {{- if .Values.management.enabled }}
 startupProbe:
   httpGet:
     path: /health
     port: management
-  initialDelaySeconds: {{ .Values.probes.readiness.initialDelaySeconds | default 10 }}
-  periodSeconds: {{ .Values.probes.readiness.periodSeconds | default 10 }}
-  timeoutSeconds: {{ .Values.probes.readiness.timeoutSeconds | default 5 }}
-  successThreshold: {{ .Values.probes.readiness.successThreshold | default 1 }}
-  failureThreshold: {{ .Values.probes.readiness.failureThreshold | default 3 }}
+  initialDelaySeconds: {{ .Values.probes.startup.initialDelaySeconds | default 0 }}
+  periodSeconds: {{ .Values.probes.startup.periodSeconds | default 5 }}
+  timeoutSeconds: {{ .Values.probes.startup.timeoutSeconds | default 5 }}
+  successThreshold: {{ .Values.probes.startup.successThreshold | default 1 }}
+  failureThreshold: {{ .Values.probes.startup.failureThreshold | default 30 }}
 {{- end }}

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: avivhalfon
Repo: traceloop/hub PR: 80
File: helm/templates/migration-job.yaml:25-55
Timestamp: 2026-01-01T12:50:14.957Z
Learning: In helm/templates/migration-job.yaml for the traceloop/hub project, the `.Values.management.database.existingSecret` field is intentionally required without a default value, as it's a mandatory configuration in values.yaml for database credentials.

helm/templates/migration-job.yaml (1)

25-60: Consider a helper template to reduce duplication.

The ConfigMap-or-value pattern is repeated four times for POSTGRES_DATABASE_HOST, PORT, NAME, and USER. This creates maintenance overhead when the pattern needs to change. doronkopit5's past feedback noted: "you still define twice POSTGRES_DATABASE_HOST, POSTGRES_DATABASE_PORT, POSTGRES_DATABASE_NAME."

While Helm doesn't support looping over environment variables directly, you could define a named template in _helpers.tpl that takes the field name and config key as parameters to generate the conditional block. This would centralize the pattern in one place.

💡 Example helper template approach

In _helpers.tpl:

{{- define "app.dbEnvVar" -}}
{{- $field := .field -}}
{{- $configKey := .configKey -}}
{{- $root := .root -}}
- name: POSTGRES_DATABASE_{{ upper $field }}
  {{- if $root.Values.management.database.configMapName }}
  valueFrom:
    configMapKeyRef:
      name: {{ $root.Values.management.database.configMapName }}
      key: {{ index $root.Values.management.database.configMapKeys $configKey }}
  {{- else }}
  value: {{ index $root.Values.management.database $field | quote }}
  {{- end }}
{{- end -}}

Then use it:

{{- include "app.dbEnvVar" (dict "field" "host" "configKey" "host" "root" .) }}
{{- include "app.dbEnvVar" (dict "field" "port" "configKey" "port" "root" .) }}
{{- include "app.dbEnvVar" (dict "field" "name" "configKey" "name" "root" .) }}
{{- include "app.dbEnvVar" (dict "field" "user" "configKey" "user" "root" .) }}

helm/templates/deployment.yaml (2)

72-107: Consider a helper template to reduce duplication (consistent with migration-job.yaml).

This file has the same ConfigMap-or-value pattern repeated four times, identical to helm/templates/migration-job.yaml lines 25-60. Keeping these duplications in sync across files increases maintenance burden. As noted in the migration-job review, a helper template in _helpers.tpl could centralize this pattern for both files.

---

154-164: Confirm management /health endpoint and add dedicated startup probe configuration.

The management API correctly exposes a /health endpoint on the management port (verified in src/management/mod.rs), so the endpoint path in line 157 is correct.

However, the startup probe reuses readiness probe values (lines 159-163) without dedicated configuration. Consider adding probes.startup.* values in values.yaml with higher failureThreshold and periodSeconds to allow sufficient time for initial startup (database connection, configuration polling, etc.).

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: avivhalfon
Repo: traceloop/hub PR: 80
File: helm/templates/migration-job.yaml:25-55
Timestamp: 2026-01-01T12:50:14.957Z
Learning: In helm/templates/migration-job.yaml for the traceloop/hub project, the `.Values.management.database.existingSecret` field is intentionally required without a default value, as it's a mandatory configuration in values.yaml for database credentials.