Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these settings in your CodeRabbit configuration.

components/package-template/src/etc/clp-config.yml (1)

118-122: Add a brief usage note to reduce misconfiguration (when to enable, required storage engine, container reachability)

This mirrors prior feedback and helps users wiring the web UI to an external Presto.

Apply:

 ## Presto client config
 #presto:
 #  host: "localhost"
 #  port: 8889
+#
+# Note:
+# - Enable this block only when `package.query_engine` is "presto".
+# - Presto queries require `package.storage_engine` to be "clp-s".
+# - Ensure host/port are reachable from the web UI container and match your Presto endpoint.

components/webui/server/src/plugins/app/Presto.ts (2)

10-12: Allow an explicit env override and prefer nullish coalescing

Support CLP_PRESTO_USER for ops overrides, then fall back to process.env.USER, then the default. Using ?? avoids treating empty strings as falsy.

Apply:

-// Use process.env.USER if defined, otherwise fallback to "clp-webui"
-const CLP_PRESTO_USER_FALLBACK = "clp-webui";
+// Resolve Presto user in priority order:
+// 1) CLP_PRESTO_USER (explicit override for ops)
+// 2) USER (typical Linux/Docker env)
+// 3) "clp-webui" (safe fallback when no user is available, e.g., some containers)
+const CLP_PRESTO_USER_FALLBACK = "clp-webui";

---

42-42: Use explicit user resolution chain (env override → USER → fallback)

This matches the documented intent and prior feedback.

Apply:

-            user: process.env.USER || CLP_PRESTO_USER_FALLBACK,
+            user: process.env.CLP_PRESTO_USER ?? process.env.USER ?? CLP_PRESTO_USER_FALLBACK,

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/src/glt/SQLitePreparedStatement.hpp:4-4
Timestamp: 2025-07-01T14:49:07.333Z
Learning: In the CLP codebase, standard headers like `<cstdint>` and `<string>` in alphabetical order (as seen in components/core/src/glt/SQLitePreparedStatement.hpp) are correctly ordered and do not need adjustment.

components/package-template/src/etc/clp-config.yml (1)

118-122: LGTM: sane defaults and placement

Presto block is placed after the garbage collector section, matching the Python config’s declaration order. Defaults align with the new model. No functional concerns here.

components/webui/server/src/plugins/app/Presto.ts (1)

37-42: Env-driven Presto options: LGTM, and this addresses prior feedback.

Using fastify.config.USER, PRESTO_CATALOG, and PRESTO_SCHEMA is the right direction and integrates with the env plugin cleanly.

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

components/clp-py-utils/clp_py_utils/clp_config.py (1)

780-793: Presto.port should be > 0 and reject unknown keys

Port 0 isn’t valid for a remote Presto service; also forbid extraneous keys to catch typos early.

 class Presto(BaseModel):
     host: str
     port: int
 
     @validator("host")
     def validate_host(cls, field):
         _validate_host(cls, field)
         return field
 
     @validator("port")
     def validate_port(cls, field):
         _validate_port(cls, field)
+        if field <= 0:
+            raise ValueError(f"{cls.__name__}.port must be greater than 0")
         return field
+
+    class Config:
+        extra = "forbid"

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: davemarco
PR: y-scope/clp#1198
File: components/webui/server/src/plugins/app/Presto.ts:38-43
Timestamp: 2025-08-25T16:27:50.549Z
Learning: In the CLP webui Presto configuration, host and port are set via package settings (configurable), while user, catalog, and schema are set via environment variables (environment-specific). This mixed approach is intentional - settings are typically set by package and some values don't need to be package-configurable.

Learnt from: davemarco
PR: y-scope/clp#1198
File: components/webui/server/src/plugins/app/Presto.ts:38-43
Timestamp: 2025-08-25T16:27:50.549Z
Learning: In the CLP webui Presto configuration, host and port are set via package settings (configurable), while user, catalog, and schema are set via environment variables (environment-specific). This mixed approach is intentional - settings are typically set by package and some values don't need to be package-configurable.

Learnt from: haiqi96
PR: y-scope/clp#743
File: components/clp-py-utils/clp_py_utils/clp_config.py:339-365
Timestamp: 2025-04-21T16:21:07.371Z
Learning: When validating enum values in Pydantic models with a pre=True root_validator, convert string values to enum instances using EnumClass(string_value) within a try/except block before comparison, as direct comparison between strings and enum instances will always return False.

components/package-template/src/etc/clp-config.yml (1)

119-121: Tighten the Presto section: remove stray “#” and add brief usage guidance

The lone “#” looks accidental. Adding a short note prevents misconfiguration when enabling Presto.

-## Presto client config
-#presto: null
-#
+## Presto client config
+# Use only when `package.query_engine` is "presto" and `package.storage_engine` is "clp-s".
+# Ensure host/port are reachable from the WebUI container.
+#presto: null

Optional: also include a commented example block to guide users without setting defaults:

 #presto: null
+#presto:
+#  host: "localhost"
+#  port: 8889

components/webui/server/src/plugins/external/env.ts (3)

43-47: Disallow blank USER to avoid emitting an empty Presto user

Empty strings can override the default when injected by containers; enforce non-empty.

 // System
 USER: {
-    type: "string",
-    default: "clp-webui",
+    type: "string",
+    minLength: 1,
+    default: "clp-webui",
 },

---

10-10: Scope user to Presto; prefer PRESTO_USER with USER fallback

A generic USER key is ambiguous. Add PRESTO_USER now and let the Presto plugin prefer it over USER.

 declare module "fastify" {
     export interface FastifyInstance {
         config: {
             PORT: number;
             HOST: string;
             USER: string;
+            PRESTO_USER?: string;
             CLP_DB_USER: string;
             CLP_DB_PASS: string;
             PRESTO_CATALOG: string;
             PRESTO_SCHEMA: string;
             RATE_LIMIT: number;
         };
     }
 }

 // Presto
+PRESTO_USER: {
+    type: "string",
+    minLength: 1,
+},
 PRESTO_CATALOG: {
     type: "string",
     default: "clp",
 },
 PRESTO_SCHEMA: {
     type: "string",
     default: "default",
 },

Follow-up (in Presto.ts, not shown here): use fastify.config.PRESTO_USER ?? fastify.config.USER.

Also applies to: 59-67

---

59-67: Validate catalog/schema are non-empty

Empty strings may behave differently than omitting the field; enforce non-empty.

 PRESTO_CATALOG: {
-    type: "string",
-    default: "clp",
+    type: "string",
+    minLength: 1,
+    default: "clp",
 },
 PRESTO_SCHEMA: {
-    type: "string",
-    default: "default",
+    type: "string",
+    minLength: 1,
+    default: "default",
 },

components/clp-py-utils/clp_py_utils/clp_config.py (1)

787-800: Presto.port should be > 0 and extra keys should be rejected

Port 0 is invalid for a remote service. Also forbid unexpected keys to catch typos early.

 class Presto(BaseModel):
     host: str
     port: int
 
     @validator("host")
     def validate_host(cls, field):
         _validate_host(cls, field)
         return field
 
     @validator("port")
     def validate_port(cls, field):
         _validate_port(cls, field)
+        if field <= 0:
+            raise ValueError(f"{cls.__name__}.port must be greater than 0")
         return field
+
+    class Config:
+        extra = "forbid"

components/clp-package-utils/clp_package_utils/scripts/start_clp.py (2)

891-892: Ensure webui server/client settings.json schemas include these keys.

Because update_settings_object rejects unknown keys, adding ClpQueryEngine (and below, PrestoHost/PrestoPort) will hard-fail if the settings schemas don’t already define them. Verify the templates were updated, or relax the updater for explicitly allowed new keys.

Run:

#!/bin/bash
set -euo pipefail
echo "Scanning settings.json files for required keys..."
fd -a -H 'settings.json' components/webui | while read -r p; do
  echo "== $p"
  ok=1
  rg -nP '"ClpQueryEngine"\s*:' "$p" || { echo "MISSING: ClpQueryEngine"; ok=0; }
  rg -nP '"PrestoHost"\s*:' "$p" || { echo "MISSING: PrestoHost"; ok=0; }
  rg -nP '"PrestoPort"\s*:' "$p" || { echo "MISSING: PrestoPort"; ok=0; }
  [[ $ok -eq 1 ]] && echo "OK"
done

---

917-924: PRESTO branch likely never executes; normalise to enum, add guard, and log.

clp_config.package.query_engine can be a string; comparing to an enum member will fail, leaving PrestoHost/Port unset. Also add a defensive guard and a single info log to aid troubleshooting.

Apply:

-    query_engine = clp_config.package.query_engine
-    if QueryEngine.PRESTO == query_engine:
-        server_settings_json_updates["PrestoHost"] = clp_config.presto.host
-        server_settings_json_updates["PrestoPort"] = clp_config.presto.port
+    query_engine = clp_config.package.query_engine
+    qe = QueryEngine(query_engine)  # normalise to enum
+    if qe == QueryEngine.PRESTO:
+        if clp_config.presto is None:
+            raise ValueError("presto config is required when ClpQueryEngine is 'presto'")
+        server_settings_json_updates["PrestoHost"] = clp_config.presto.host
+        server_settings_json_updates["PrestoPort"] = clp_config.presto.port
+        logger.info(
+            f"Configuring web UI Presto client -> host: {clp_config.presto.host}, port: {clp_config.presto.port}"
+        )
     else:
         server_settings_json_updates["PrestoHost"] = None
         server_settings_json_updates["PrestoPort"] = None

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: davemarco
PR: y-scope/clp#1198
File: components/webui/server/src/plugins/app/Presto.ts:38-43
Timestamp: 2025-08-25T16:27:50.549Z
Learning: In the CLP webui Presto configuration, host and port are set via package settings (configurable), while user, catalog, and schema are set via environment variables (environment-specific). This mixed approach is intentional - settings are typically set by package and some values don't need to be package-configurable.

Learnt from: davemarco
PR: y-scope/clp#1198
File: components/webui/server/src/plugins/app/Presto.ts:38-43
Timestamp: 2025-08-25T16:27:50.549Z
Learning: In the CLP webui Presto configuration, host and port are set via package settings (configurable), while user, catalog, and schema are set via environment variables (environment-specific). This mixed approach is intentional - settings are typically set by package and some values don't need to be package-configurable.

Learnt from: haiqi96
PR: y-scope/clp#743
File: components/clp-py-utils/clp_py_utils/clp_config.py:339-365
Timestamp: 2025-04-21T16:21:07.371Z
Learning: When validating enum values in Pydantic models with a pre=True root_validator, convert string values to enum instances using EnumClass(string_value) within a try/except block before comparison, as direct comparison between strings and enum instances will always return False.

Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/src/glt/SQLitePreparedStatement.hpp:4-4
Timestamp: 2025-07-01T14:49:07.333Z
Learning: In the CLP codebase, standard headers like `<cstdint>` and `<string>` in alphabetical order (as seen in components/core/src/glt/SQLitePreparedStatement.hpp) are correctly ordered and do not need adjustment.