Skip to content

feat: Semantic Layer UI — full parity with kbagent CLI (4 phases) - #308

Merged
padak merged 6 commits into
mainfrom
feat/semantic-layer-ui
May 18, 2026
Merged

feat: Semantic Layer UI — full parity with kbagent CLI (4 phases)#308
padak merged 6 commits into
mainfrom
feat/semantic-layer-ui

Conversation

@padak

@padak padak commented May 17, 2026

Copy link
Copy Markdown
Member

Summary

Native Semantic Layer surface inside kbagent serve --ui, matching the full
kbagent semantic-layer CLI feature set. Built in four iterative passes
against the keboola-ai project's langsmith_semantic_model (15 metrics,
16 datasets, 142 relationships, 6 constraints) so every code path is
exercised against real production-shape data.

What lands

Phase 1+2 — CRUD scaffolding (commit bcc80b7)

  • New /semantic-layer page in the Insights sidebar group.
  • Schema-driven entity forms (one declaration covers Add + Edit for all
    five entity kinds — metric, dataset, relationship, constraint, glossary).
  • Per-model header with Validate / Export / Delete buttons.

Phase 3 — Workflows + backend hardening (commit 34514ce)

  • New SemanticLayerDialogs.tsx with five drawers: Diff (project↔project,
    project↔file, file↔file), Promote (cross-project with dry-run preview),
    Import (snapshot replay with item-by-item log), Build (heuristic
    greenfield builder with bucket-filtered table picker), Encrypt token
    (project storage token → KBC::ProjectSecure ciphertext for transformation
    user_properties).
  • Filled-keboola primary buttons so the actionable choice no longer looks
    identical to Cancel/Close.
  • Backend: heuristic_generate_model now maps warehouse-native column
    types onto the metastore's lowercase closed set (string/integer/...).
    Previously every legacy untyped table 422'd the build push.
  • Backend: http_base._raise_api_error now surfaces the real error
    message from the metastore's {"error": 422, "errors": [...]} shape
    instead of bare "API error 422: 422".

Phase 4 — Polish for real-data scale (commit 4d7dcb4)

  • tableId/primaryKey/constraintType camelCase → snake normalisation
    at the API boundary so the TABLE column actually populates.
  • Relationships ERD toggle (Mermaid erDiagram; mermaid is already in
    deps). Soft 80-edge cap with banner pointing at the dataset chip filter
    for larger models.
  • Quick search box across every entity table, plus a "filter by dataset"
    chip for relationships.
  • Dataset detail drawer renders fields with role badges + AI keywords
    instead of raw JSON (Raw still one click away).
  • Constraints grouped into collapsible <details> per constraint_type
    (inequality → equality → range → ...) with severity icons
    (🔴 critical · ⚠ warning · ⓘ info).

Architecture / business-logic check

  • All UI calls route through /api/semantic-layer/*zero
    metastore HTTP from JS, zero business logic in the frontend.
  • CLI ↔ API ↔ UI parity is 1:1 (14 routes mirror 11 CLI subcommands +
    15 add/edit/remove leaves; POST /items/{kind} parameterises the
    five kinds in REST style).
  • Backend service layer is the only place fields/types/FQN/push order
    are computed.

Known follow-ups (filed separately, not in this PR)

Test plan

  • Local smoke: kbagent serve --ui against padak-2-0-master and
    keboola-ai
  • Phase 1 — Create model (Snowflake / BigQuery)
  • Phase 2 — Add dataset, Add metric, Validate, Export → snapshot.json
  • Phase 3 — Diff (project↔file), Build (with cleanup of orphan
    datasets), Encrypt token (paste-ready user_properties block)
  • Phase 4 — Dataset detail drawer on fct_conversations (64 fields),
    Relationships ERD with dataset chip filter (142 → 31 edges),
    Constraints grouped by type (inequality/range/composition) with
    severity icons

padak added 5 commits May 17, 2026 16:28
…+ Validate/Export (#300)

First commit on a multi-commit branch implementing full parity for the
`kbagent semantic-layer` CLI surface in the web UI.

What this commit delivers
-------------------------

- New /semantic-layer page reachable from the Insights section in the
  sidebar. Page hub layout: top model picker + Validate / Export
  workflow buttons, below them entity tabs (Metrics / Datasets /
  Relationships / Constraints / Glossary).
- Model lifecycle: list, create (drawer form with name + description +
  sql_dialect), delete (with confirmation, cascades on the API side).
- Per-entity CRUD for all five kinds — metric, dataset, relationship,
  constraint, glossary:
    - Read: table rows with kind-specific columns (e.g. metric shows
      Name / Dataset / SQL; constraint shows Name / Type / Severity /
      Rule with severity-colored pills).
    - Inspect: row click opens a Drawer with the raw JSON for the
      entity, including any unmodelled fields the API returns.
    - Add: '+ Add <kind>' button opens a schema-driven form Drawer.
    - Edit: pencil icon opens the same form pre-filled, sending only
      changed fields as new_* keys per the CLI's edit API.
    - Delete: trash icon with confirm prompt; deletion is destructive
      on the API side, no undo UI.
- Validate: top-bar checkbox toggles 'deep' mode, button opens a
  Drawer showing the validation result JSON. Structural + Snowflake
  column probes when deep is set.
- Export: button triggers a /semantic-layer/export call and saves the
  JSON snapshot to disk as
  `sl-<model>-<UTC-timestamp>.json`. Filename collision-safe across
  successive exports.

Architecture notes
------------------

The form layer is schema-driven: ENTITY_SCHEMAS declares per-kind
field lists (name, type, required flag, options for enums, help text,
the editKey for the API's new_* prefix convention). One generic
EntityFormDrawer + FieldInput renders Add and Edit forms for all
five kinds. Avoids hand-coding five separate form components and
keeps validation rules / field labels in one place.

List-typed fields (primary_key, metrics) are entered as
comma/newline-separated text and serialised back to arrays on submit
so users can paste from either format. Boolean fields render as
checkboxes (e.g. dataset.deep_fields). Select fields render an
"(unset)" option so the edit form can omit fields and let the API
treat them as no-change.

Sidebar entry is in the Insights section next to Lineage — both are
about understanding the data shape rather than writing transformations.

What is NOT in this commit (Phase 3 follow-up)
----------------------------------------------

- Diff dialog (project↔project, project↔file, file↔file with
  side-by-side render).
- Promote dialog (cross-project copy with model UUID rewrite +
  identical/changed/new classification).
- Import dialog (snapshot upload + dry-run preview).
- Build wizard (AI-assisted greenfield via the existing
  /semantic-layer/build endpoint — would benefit from a streaming
  variant; CLI today is blocking).
- Token encrypt UI (the CLI `sl token --encrypt` covers transformation
  container secrets — niche enough to skip in v1).
- Tests for the new page (frontend pages are tested live; backend
  endpoints have existing test coverage in test_semantic_layer_*).

Backend changes: none. All endpoints already shipped in v0.41.0 (PR
#293). TypeScript build clean; 3328 backend tests still passing.
…+ backend hardening

Phase 3 of the Semantic Layer UI brings full parity with the `kbagent
semantic-layer` CLI surface:

UI dialogs (new `pages/SemanticLayerDialogs.tsx`):
- DiffDialog — project↔project / project↔file / file↔file with side-by-side
  added/removed/changed panels per entity kind.
- PromoteDialog — cross-project copy with from→to picker, dry-run preview
  (new/overwritten/identical/failed stats per type) before apply.
- ImportDialog — snapshot file upload + types filter + overwrite toggle,
  dry-run preview, item-by-item result log.
- BuildDialog — heuristic greenfield builder; bucket-filtered table picker,
  preview vs apply, validation + push counts visible inline.
- TokenEncryptDialog — encrypt project storage token into a
  `KBC::Project::*` ciphertext bound to a component, with copy-block /
  copy-just-the-ciphertext helpers.

UX polish in SemanticLayer.tsx:
- New page-level action bar: New model | Build | Import | Encrypt token.
- Per-model header gains Diff + Promote buttons next to Validate/Export.
- Filled primary buttons (bg-keboola) so the actionable choice no longer
  looks identical to Cancel/Close — fixes the "which one is Build?" UX
  hit Vojta reported in earlier rounds.
- Add/Edit drawer now `invalidateQueries(["sl-show", project, model])`
  on success so the entity table refreshes without a manual Reload click.
- SQL dialect dropdown defaults to capitalised "Snowflake"/"BigQuery" —
  metastore is case-sensitive, lowercase tripped a 422 on Create Model.

Backend fixes uncovered during smoke testing on padak-2-0-master:
- _semantic_layer_internals.heuristic_generate_model now normalises
  warehouse-native column types via a new `_normalize_field_type` helper
  onto the metastore's closed vocabulary (string/integer/decimal/
  boolean/date/datetime/json). Untyped legacy buckets — and capitalised
  warehouse types — used to 422 with "value must be one of ...". Empty
  / unknown types fall back to "string".
- http_base._raise_api_error now walks `exception`, `message`,
  `description`, `detail`, `errors` and finally `json.dumps(body)` so the
  Keboola Metastore's HTTP 422 / 500 payload shape ({"error": 422,
  "errors": [{"path": ..., "message": ...}]}) surfaces a readable
  message instead of bare "API error 422: 422".

Known follow-ups (filed as backlog, not in this PR):
- semantic-layer model delete leaves orphan children in the metastore;
  subsequent Build with same dataset names hits per-project name
  conflict. Workaround: delete child datasets manually before retrying.
… + grouped constraints

Smoke-tested against keboola-ai/langsmith_semantic_model (15 metrics,
16 datasets, 142 relationships, 6 constraints).

Five UI improvements landed in a single pass:

1. **Dataset table column actually populates** — backend returns camelCase
   `tableId` / `primaryKey` / `constraintType` from the metastore, but the
   UI schema (and Pydantic add/edit bodies) speak snake_case. New
   `normalizeEntityRow()` bridges the gap at the boundary so render code
   stays in one vocabulary. Previously the entire TABLE column was empty
   for any project that fed the metastore directly.

2. **Relationships ERD view** — Mermaid `erDiagram` toggle next to the
   existing table view. Datasets become entities, relationships become
   labelled edges (`name (type)`). Soft cap at 80 edges with a banner that
   tells the user to drill in via the dataset chip filter when the model
   is larger. Click-through edge list below the SVG keeps every
   relationship reachable for editing even if Mermaid's hit-testing is
   fiddly on dense graphs. Mermaid is already in deps (Lineage / Flows
   use it) so no new dependency.

3. **Quick search + dataset filter** — sticky search box above every
   entity table. For relationships, a `(all datasets)` dropdown filters
   edges where either endpoint matches the selected `tableId`. The
   filtered count surfaces next to the title (`Relationships 31 / 142`).

4. **Dataset detail drawer** — clicking a dataset row no longer dumps
   raw JSON. Renders Storage table / FQN / grain / primary key as a
   labelled grid, AI keywords as chips, then a `fields[]` table with
   name / type / role badge / description. Power-user "Raw response"
   disclosure keeps the JsonView one click away.

5. **Constraint grouping + severity icons** — constraints are now
   grouped into collapsible `<details>` per constraint_type
   (inequality → equality → range → composition → exclusion → temporal
   → conditional, with unknown types alphabetised below). Each row
   leads with a SeverityIcon: XCircle (red) for `critical`/`error`,
   AlertTriangle (amber) for `warning`, Info (zinc) otherwise. Edit /
   delete actions are preserved.

All five features are read-only against the metastore (search/filter)
or use the existing edit drawer path, so no API surface change.
…ally fills

Previously the ER diagram lived in the top quarter of its container — the
mermaid SVG laid out at its intrinsic ~250px height regardless of how big
the pane was, leaving 70% of the 70vh canvas as whitespace. Zoom-in via
CSS `transform: scale()` with top-left origin then pushed the centred
diagram content off-screen entirely (156% zoom showed a blank canvas).

Three fixes land together:

1. Auto-fit on render. After mermaid emits the SVG we read its viewBox
   and compute the scale that fills the active container along whichever
   axis dominates (Math.max(fitX, fitY)), clamped to 1.0–3.0. For the
   typical wide-and-short erDiagram fitX dominates and we land around
   130–150% out of the box, so the diagram fills the canvas without
   scrolling instead of leaving a sea of whitespace. The fit pass is
   gated on userZoomed so manual zoom adjustments aren't snapped back.

2. Zoom via width/height growth, not CSS transform. The SVG host div
   now uses width: zoom*100% and height: zoom*100% (with minWidth/
   minHeight 100%). This makes the surrounding overflow-auto container
   actually scroll when the user zooms in instead of clipping the centred
   SVG content into the top-left corner.

3. Reset = Fit, not 100%. The Reset Zoom button now snaps back to the
   auto-fit zoom, which is the actually-readable default. "100%" was
   the smallest readable form (mermaid's native pixel size) and left
   most of the canvas empty — confusing as a reset target.

Also strips mermaid's inline max-width / width= / height= attrs from
the SVG so the container — not mermaid's pre-computed layout pixels —
decides the diagram's rendered size. preserveAspectRatio="xMidYMid
meet" stays so the diagram doesn't distort.

Fullscreen drawer keeps the same logic with a 85vh / 95vw canvas.
…ensibly

`erDiagram` had no rankdir and laid every model out wide-and-short, wasting
~70% of the canvas. Switch to `flowchart TB`: the hub lands above and its
dimensions / dependents fill the rank below, which uses vertical space well
for the typical Keboola fact-with-dims shape.

- Edge labels drop to just the join type (e.g. `left` / `inner`) — the
  per-relationship name was always `<from>_to_<to>`, duplicated at both
  endpoints. The edge list below the diagram keeps the full names.
- Auto-fit switches `Math.max` → `Math.min(fitX, fitY)` so the whole graph
  fits without scrolling. Floor 0.4 lets a dense 80-edge view shrink down;
  cap 2.5 stops a 3-node mini-graph from blowing up.

Before: see #283 / 836f32d screenshots — wide strip, content in top quarter.
After: 15-relationship hub view auto-fits at ~63%, 80-edge overview at ~40%,
both readable end-to-end.
@padak

padak commented May 17, 2026

Copy link
Copy Markdown
Member Author

Pushed fe49c1a — relationships ERD now uses Mermaid flowchart TB instead of erDiagram.

Why: erDiagram has no rankdir and always lays out wide-and-short. For Keboola's typical hub-and-spoke models (one fact, many dims) that wastes ~70% of the canvas. flowchart TB puts the hub above its dimensions and keeps the diagram readable end-to-end.

Three small changes:

  • erDiagramflowchart TB in the Mermaid source
  • Edge labels shortened from <from>_to_<to> (left) to just left / inner (the full names live in the edge list below the diagram)
  • Auto-fit switched from Math.max to Math.min(fitX, fitY) so the whole graph fits without scrolling; floor 0.4 / cap 2.5

Verified locally with the langsmith_semantic_model (16 datasets, 142 relationships): hub-filter view auto-fits at ~63%, full-model view at ~40%, both fully readable.

@padak padak left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of #308 — feat: Semantic Layer UI — full parity with kbagent CLI (4 phases)

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This PR adds a full Semantic Layer management page to kbagent serve --ui,
mirroring all kbagent semantic-layer CLI operations (model CRUD, five entity
kinds, validate/export/diff/promote/import/build/encrypt-token). Two backend
bugfixes are bundled: _raise_api_error now surfaces real error text from the
Keboola Metastore's non-standard error shapes ({"error": 422, "errors": [...]},
{"description": "..."}) instead of printing a bare HTTP status code; and
heuristic_generate_model now maps warehouse-native column types to the
metastore's closed vocabulary before pushing, which previously caused HTTP 422
on every legacy untyped table. The UI calls zero Metastore endpoints directly —
all traffic routes through /api/semantic-layer/*. Architecture, layer
boundaries, security, and permissions are clean.

Verdict: COMMENT — no blocking findings; four non-blocking findings, most
of which are a missing test for the two backend bugfixes.

Verdict

  • Verdict: COMMENT
  • Blocking findings: 0
  • Non-blocking findings: 4
  • Nits: 1

Blocking findings

(none)

Non-blocking findings

[NB-1] src/keboola_agent_cli/http_base.py:226-231 — duplicate comment block introduced by the diff

The diff inserts a comment block starting with # Real Keboola APIs answer with one of these keys in priority order. at line 226. A second, more precise
version of the same intro sentence starts at line 232 (# Real Keboola APIs answer with one of these keys in priority). Lines 226-231 are a leftover from
an earlier draft; the correct version is the second block (lines 232-240) which
explains the two caveats. Fix: delete the five stale lines (226-231) so the
comment reads as a single cohesive block.

[NB-2] src/keboola_agent_cli/http_base.py:241-255 — no tests for the three new _raise_api_error fallback code paths

The PR introduces three new parser paths in _raise_api_error: (a) error
field that is an int ({"error": 422}) must be rejected in favour of
description/errors; (b) {"description": "..."} (FastAPI default); (c)
{"errors": [...]} / {"detail": [...]} list shapes. test_http_base.py has
no test exercising any of these shapes. These are the exact shapes the Metastore
returns on validation failures (the stated motivation for the fix), so a
regression here would silently bring back the "bare 422" UX. Add three test
cases to tests/test_http_base.py mirroring the real Metastore response shapes.

[NB-3] src/keboola_agent_cli/services/_semantic_layer_internals.py:988-994_normalize_field_type has no dedicated unit test; existing heuristic test does not assert the mapping

_normalize_field_type is the fix for the "legacy untyped table 422" bug.
tests/test_semantic_layer_service.py::TestBuildModel::test_heuristic_fallback
exercises heuristic_generate_model with {"name": "AMOUNT", "type": "NUMBER"}
but asserts only the count of datasets/metrics/glossary — not that the
generated field type was normalized to "decimal". A dedicated parametrized
test (empty string -> "string", "NUMBER(18,2)" -> "decimal",
"VARCHAR(255)" -> "string", unknown type -> "string") would pin the
mapping and prevent regression when the FIELD_TYPE_MAP is extended. Also
extend test_heuristic_fallback to assert result["generated"]["datasets"][0]["fields"][0]["type"] == "decimal".

[NB-4] plugins/kbagent/skills/kbagent/references/gotchas.md:185-210 — field-type normalization fix not documented in the build gotcha entry

The existing ## \semantic-layer build` is a HEURISTIC fallback, not full AI
(since v0.41.0)entry describes the heuristic fields[] synthesis but says nothing about field-type normalization. Prior to this PR,build 422'd on every legacy untyped Storage table (Storage returns empty/""or warehouse- native uppercase types likeVARCHAR, NUMBER; the Metastore's closed vocabulary only accepts lowercase string/integer/decimal/...). An AI agent instructed to buildagainst an untyped project on kbagent < this fix will hit 422s and have no documentation to consult. Add a one-liner with a(since v0.41.10)` tag to the existing entry: e.g. "Field types are normalized
from warehouse-native (VARCHAR, NUMBER, ...) to the metastore's closed
lowercase set ({string, integer, decimal, boolean, date, datetime, json}) —
avoids HTTP 422 on legacy untyped Storage tables (since v0.41.10)."

Nits

  • [NIT-1] web/frontend/src/pages/SemanticLayer.tsx:1 — at 2099 lines this file is well above the soft ceiling for any layer. It's frontend (no hard ceiling defined in CONTRIBUTING.md for TypeScript), but splitting the entity-table section and the model-CRUD header into sibling files would make the next feature addition (e.g. a new entity kind) faster to navigate.

Verification log

  • gh pr view 308 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state → 7 files, +3588/-2, state: OPEN, branch: feat/semantic-layer-ui ✓
  • git rev-parse --abbrev-ref HEADfeat/semantic-layer-ui (matches PR branch) ✓
  • Layer violation greps (typer/click in services; httpx in commands; formatter in clients) → empty output ✓
  • grep -E '^\+(from httpx|import httpx...)' diff → no new HTTP calls in wrong layer ✓
  • grep '*.keboola.com\|fetch\|axios' SemanticLayer.tsx SemanticLayerDialogs.tsx → no direct Metastore calls from JS; all routes through /api/semantic-layer/*
  • grep -E '^\+\s*except\s*:' diff → no bare excepts introduced ✓
  • grep -E '^\+.*error_code\s*=\s*"[A-Z_]+"' diff → no raw error-code strings ✓
  • grep -E '^\+(from typer|import typer...)' diff → no layer violations ✓
  • grep 'OPERATION_REGISTRY' permissions.py → semantic-layer commands already registered (pre-existing from v0.41.0); this PR adds no new CLI commands, so no new entries needed ✓
  • Plugin synchronization map: PR adds no new CLI commands; no AGENT_CONTEXT, CLAUDE.md ## All CLI Commands, or commands-reference.md updates required ✓
  • grep "Real Keboola APIs answer" http_base.py → duplicate comment block at lines 226-231 and 232-240 detected ✗ (NB-1)
  • git diff --stat main...HEAD | grep tests/ → no test files changed in this PR ✗ (NB-2, NB-3)
  • make check → 3339 passed, 7 skipped, exit 0 ✓
  • Behavior verification: could not run kbagent serve --ui against a real project (no E2E credentials in this session). PR author's test plan (4 phases × multiple scenarios including legacy untyped table build) is detailed and appears complete; marking as unverified by reviewer.
  • Token discipline: grep token SemanticLayerDialogs.tsx — token appears only in the encrypt-token dialog (building #metastore_token key) and in UI copy strings; no token is read from JS scope or logged ✓

Open questions for the author

  • The _raise_api_error change uses body.get("description") as one of the fallback keys. The Keboola Manage API also uses description for a success body field on some endpoints. If a success response somehow reaches _raise_api_error (should not happen given the < 400 guard, but defensive question), would the description key from a non-error body produce a confusing error message? The guard appears correct; just flagging for author confirmation.

Four findings from kbagent-pr-reviewer; NIT-1 (SemanticLayer.tsx file size)
deferred — splitting a 2099-line file is a risky follow-up, not bundled here.

NB-1: drop duplicate `_raise_api_error` comment block (lines 226-231 were a
left-over from an earlier draft; the precise version at 232-240 stays).

NB-2: pin the three new `_raise_api_error` fallback paths with regression
tests that reproduce the actual Metastore / FastAPI error shapes:
  - `{"error": 422, "description": "..."}` — int `error` must not shadow
    the real message in `description` (the original bug)
  - `{"description": "..."}` — FastAPI default
  - `{"errors": [{loc, msg}]}` — Metastore 422 list shape
  - `{"detail": [{loc, msg}]}` — canonical FastAPI validation shape
Without these, regressing to "API error 422: 422" would slip through CI.

NB-3: pin the warehouse → metastore type normalization. Add a parametrized
`TestNormalizeFieldType` covering empty / None / parameterised types
(`VARCHAR(255)`, `NUMBER(38,2)`), case variants, every output bucket, and
fall-through-to-`"string"` for unknown UDTs. Also extend the existing
`test_heuristic_fallback` to assert the dataset field type was normalized
to `"decimal"` end-to-end (Storage hands `"NUMBER"`, metastore wants
`"decimal"` — the assertion that motivated the original 422 fix).

NB-4: document the field-type normalization in the `semantic-layer build`
gotcha entry with a `(since v0.41.10)` tag so any AI agent running an older
kbagent has a documented path to the explanation.

make check: 3357 passed (was 3339), 7 skipped, exit 0.

@padak padak left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of #308 — feat: Semantic Layer UI — full parity with kbagent CLI (4 phases)

Generated by kbagent-pr-reviewer subagent (delta re-review after commit b2dbe69).
Verdict and findings below are advisory; the human author retains every veto.
CI-coverable issues (lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This is a delta re-review targeting the fix-up commit b2dbe69 ("test(semantic-layer-ui): address /kbagent:review findings on #308"). All four non-blocking findings from the prior review (NB-1 through NB-4) are now fully addressed. NIT-1 (SemanticLayer.tsx file size) was explicitly deferred by the author and is not re-raised here. No new blocking or non-blocking issues were introduced by the fix-up commit. One new nit is noted regarding a minor type annotation mismatch introduced by NB-3's test. make check passes with 3357 tests (up from 3339).

Verdict: APPROVE — zero blocking findings, zero non-blocking findings.

Verdict

  • Verdict: APPROVE
  • Blocking findings: 0
  • Non-blocking findings: 0
  • Nits: 1

Blocking findings

(none)

Non-blocking findings

(none)

Prior findings — resolution status

Finding Status
NB-1: duplicate comment block in http_base.py:226-231 Resolvedb2dbe69 removes the 6 stale lines; only the precise two-caveat block at lines 226-234 remains.
NB-2: no tests for three new _raise_api_error fallback paths Resolvedtests/test_http_base.py gains four tests: test_int_error_field_falls_back_to_description, test_description_field_used_when_no_message, test_errors_list_shape_serialised, test_detail_list_shape_serialised. Each reproduces the real Metastore/FastAPI error shape.
NB-3: _normalize_field_type has no dedicated unit test; test_heuristic_fallback did not assert field type ResolvedTestNormalizeFieldType parametrizes 14 cases (empty, None, parameterised types, case variants, unknown UDT fallthrough). test_heuristic_fallback now asserts ds_fields[0]["type"] == "decimal".
NB-4: field-type normalization not documented with (since vX.Y.Z) tag Resolvedgotchas.md line 211 adds **Field-type normalization (since v0.41.10)** with the full closed-set description.
NIT-1: SemanticLayer.tsx at 2099 lines above soft ceiling Explicitly deferred — acknowledged in commit message; not re-raised.

Nits

  • [NIT-2] tests/test_semantic_layer_service.py:1924TestNormalizeFieldType.test_normalize_field_type passes None as the basetype argument but _normalize_field_type is annotated basetype: str. The runtime if not basetype: guard handles None correctly and ty does not flag the parametrize call site, but the annotation and the test contract are inconsistent. The production call site (line 1028) cannot produce None via col.get("type", "") or col.get("native_type", "") unless both dict values are explicitly None (not absent), which would bypass the "" default. If the intent is to document that None-safe behavior is guaranteed, change the signature to basetype: str | None. If None is not a real input, drop the parametrize case. Either way is fine; this is cosmetic.

Verification log

  • gh pr view 308 --json title,state → state=OPEN, title matches ✓
  • git rev-parse --abbrev-ref HEADfeat/semantic-layer-ui matches <branch>
  • git log --oneline feat/semantic-layer-ui ^main → 6 commits; top is b2dbe69
  • git show b2dbe69 --stat → 4 files changed: gotchas.md (+8), http_base.py (-6), test_http_base.py (+99), test_semantic_layer_service.py (+49) ✓
  • NB-1: grep -n "Real Keboola APIs" http_base.py → 1 hit (line 226); duplicate gone ✓
  • NB-2: grep -n "def test_" test_http_base.py → 4 new tests for _raise_api_error fallback shapes ✓
  • NB-3: grep -n "class TestNormalizeFieldType" test_semantic_layer_service.py → found; 14 parametrize cases; test_heuristic_fallback extended with ds_fields[0]["type"] == "decimal" assertion ✓
  • NB-4: grep -n "since v0.41.10" gotchas.md → line 211, Field-type normalization entry ✓
  • make check → 3357 passed, 7 skipped, exit 0 ✓
  • make typecheck on modified files (http_base.py, _semantic_layer_internals.py) → 0 error[ diagnostics in those files ✓
  • Bare except:, raw error_code strings, print() in production, token-in-log checks → all clean ✓
  • b2dbe69 http_base.py diff: pure comment removal (6 lines), no logic change ✓

Open questions for the author

(none)

@padak padak left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of #308 — feat: Semantic Layer UI — full parity with kbagent CLI (4 phases)

Generated by kbagent-pr-reviewer subagent (delta re-review after commit b2dbe69).
Verdict and findings below are advisory; the human author retains every veto.
CI-coverable issues (lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This is a delta re-review targeting the fix-up commit b2dbe69 ("test(semantic-layer-ui): address /kbagent:review findings on #308"). All four non-blocking findings from the prior review (NB-1 through NB-4) are now fully addressed. NIT-1 (SemanticLayer.tsx file size) was explicitly deferred by the author and is not re-raised here. No new blocking or non-blocking issues were introduced by the fix-up commit. One new nit is noted regarding a minor type annotation mismatch introduced by NB-3's test. make check passes with 3357 tests (up from 3339).

Verdict: APPROVE — zero blocking findings, zero non-blocking findings.

Verdict

  • Verdict: APPROVE
  • Blocking findings: 0
  • Non-blocking findings: 0
  • Nits: 1

Blocking findings

(none)

Non-blocking findings

(none)

Prior findings — resolution status

Finding Status
NB-1: duplicate comment block in http_base.py:226-231 Resolvedb2dbe69 removes the 6 stale lines; only the precise two-caveat block at lines 226-234 remains.
NB-2: no tests for three new _raise_api_error fallback paths Resolvedtests/test_http_base.py gains four tests: test_int_error_field_falls_back_to_description, test_description_field_used_when_no_message, test_errors_list_shape_serialised, test_detail_list_shape_serialised. Each reproduces the real Metastore/FastAPI error shape.
NB-3: _normalize_field_type has no dedicated unit test; test_heuristic_fallback did not assert field type ResolvedTestNormalizeFieldType parametrizes 14 cases (empty, None, parameterised types, case variants, unknown UDT fallthrough). test_heuristic_fallback now asserts ds_fields[0]["type"] == "decimal".
NB-4: field-type normalization not documented with (since vX.Y.Z) tag Resolvedgotchas.md line 211 adds **Field-type normalization (since v0.41.10)** with the full closed-set description.
NIT-1: SemanticLayer.tsx at 2099 lines above soft ceiling Explicitly deferred — acknowledged in commit message; not re-raised.

Nits

  • [NIT-2] tests/test_semantic_layer_service.py:1924TestNormalizeFieldType.test_normalize_field_type passes None as the basetype argument but _normalize_field_type is annotated basetype: str. The runtime if not basetype: guard handles None correctly and ty does not flag the parametrize call site, but the annotation and the test contract are inconsistent. The production call site (line 1028) cannot produce None via col.get("type", "") or col.get("native_type", "") unless both dict values are explicitly None (not absent), which would bypass the "" default. If the intent is to document that None-safe behavior is guaranteed, change the signature to basetype: str | None. If None is not a real input, drop the parametrize case. Either way is fine; this is cosmetic.

Verification log

  • gh pr view 308 --json title,state → state=OPEN, title matches ✓
  • git rev-parse --abbrev-ref HEADfeat/semantic-layer-ui matches <branch>
  • git log --oneline feat/semantic-layer-ui ^main → 6 commits; top is b2dbe69
  • git show b2dbe69 --stat → 4 files changed: gotchas.md (+8), http_base.py (-6), test_http_base.py (+99), test_semantic_layer_service.py (+49) ✓
  • NB-1: grep -n "Real Keboola APIs" http_base.py → 1 hit (line 226); duplicate gone ✓
  • NB-2: grep -n "def test_" test_http_base.py → 4 new tests for _raise_api_error fallback shapes ✓
  • NB-3: grep -n "class TestNormalizeFieldType" test_semantic_layer_service.py → found; 14 parametrize cases; test_heuristic_fallback extended with ds_fields[0]["type"] == "decimal" assertion ✓
  • NB-4: grep -n "since v0.41.10" gotchas.md → line 211, Field-type normalization entry ✓
  • make check → 3357 passed, 7 skipped, exit 0 ✓
  • make typecheck on modified files (http_base.py, _semantic_layer_internals.py) → 0 error[ diagnostics in those files ✓
  • Bare except:, raw error_code strings, print() in production, token-in-log checks → all clean ✓
  • b2dbe69 http_base.py diff: pure comment removal (6 lines), no logic change ✓

Open questions for the author

(none)

@padak
padak merged commit 2dd9945 into main May 18, 2026
1 check passed
@padak
padak deleted the feat/semantic-layer-ui branch May 18, 2026 06:22
padak added a commit that referenced this pull request May 18, 2026
…dening (#313)

- pyproject.toml 0.42.0 → 0.43.0 (auto-synced into plugin.json +
  marketplace.json via `make version-sync`)
- Changelog entry summarises the three lines of work merged via #308:
  * Full Semantic Layer page in `kbagent serve --ui` — 1:1 mirror of
    `kbagent semantic-layer` CLI surface (model CRUD, 5-entity CRUD,
    validate/export/diff/promote/import/build/encrypt-token, all
    routed through `/api/semantic-layer/*`, zero direct Metastore
    calls from JS). Relationships ERD ships as `flowchart TB` with
    hub-and-spoke layout, edge-label minimisation, and Math.min
    auto-fit so 80-edge overviews shrink to fit (~40%) and 15-edge
    hub drill-downs land at ~63%.
  * `_raise_api_error` walks exception/message/description/detail/
    errors/json.dumps; rejects int `error` field. Surfaces real
    Metastore 422 text instead of "API error 422: 422".
  * Heuristic `build` now maps warehouse-native column types to the
    metastore's closed lowercase vocabulary before push. Closes the
    long-standing 422 on legacy untyped Storage tables.

make lint / format-check / changelog-check / test: all green
(3373 passed, 7 skipped).
padak added a commit that referenced this pull request May 18, 2026
…ty (#312)

PR #311 (v0.42.0, closes #304) placed the keboola.sandboxes
parameters.id → storage_workspace_id resolution in `commands/config.py`,
which meant the annotation only fired on `kbagent config detail` CLI
invocations. HTTP / REST callers (`kbagent serve` web UI, scheduled
agents, third-party clients hitting `GET /configs/...`) hit the same
parameters.id trap David Ešner originally reported in #304.

This PR moves the annotation into `ConfigService.get_config_detail()`
behind a new opt-in parameter so all callers can get it.

Architecture
------------

1. **Pure-function helper extraction.** The workspace[].configurationId
   → workspace.id filter logic moves from
   `WorkspaceService.resolve_sandbox_workspace_id` to a module-level
   `find_storage_workspace_for_sandbox_config(workspaces, config_id)`
   in `services/workspace_service.py`. This lets ConfigService call
   it without taking a circular `ConfigService → WorkspaceService`
   dependency in the DI graph. `WorkspaceService.resolve_sandbox_workspace_id`
   becomes a thin wrapper around the helper (still useful for direct
   programmatic callers).

2. **Opt-in service parameter.**
   `ConfigService.get_config_detail()` gains
   `include_sandbox_annotation: bool = False`. Default off so existing
   programmatic consumers see the unchanged shape -- zero-regression
   contract. When the flag is on AND `component_id == "keboola.sandboxes"`
   AND single-config mode, the service fetches `list_workspaces` once
   and stamps the annotation onto the response.

3. **CLI: switch to service-layer annotation.** `commands/config.py`
   drops its ad-hoc post-fetch enrichment block (which previously
   called `WorkspaceService` directly from the command layer -- a
   layering violation) and instead passes
   `include_sandbox_annotation=True` to `get_config_detail`. Bulk mode
   stays off because it would N+1 the workspace listing endpoint.

4. **HTTP / REST parity.**
   `GET /configs/{project}/{component_id}/{config_id}` on `kbagent serve`
   accepts a new query parameter `?include_sandbox_annotation=true`
   (default false), forwarded verbatim to the service. The FastAPI
   `description=` on the Query annotation renders the rationale inline
   in /docs.

5. **Graceful degradation.** If `list_workspaces` fails (rate limit,
   transient 5xx), the detail call still succeeds and
   `storage_workspace_id` is set to `None`. The annotation is UX, not
   a contract -- the caller still gets the raw detail with the same
   shape they would see if `include_sandbox_annotation=False`.

Tests
-----

- **5 new in `test_services.py::TestConfigServiceSandboxAnnotation`**:
  default-off zero-regression, opt-in resolution to real workspace ID,
  orphan (no matching workspace -> storage_workspace_id=None), non-sandbox
  component is no-op (no list_workspaces fan-out), graceful degradation
  on list_workspaces KeboolaApiError.

- **3 new in `test_serve_ui.py::TestConfigDetailSandboxAnnotation`**:
  HTTP router parameter binding -- default-off, opt-in, non-sandbox
  no-op. Stubs `app.state.registry.config.get_config_detail` to avoid
  real Keboola HTTP and assert the router forwards the flag verbatim.

- **3 existing CLI tests in `test_cli.py::TestConfigDetail`** updated
  to mock the new service-layer call path (`client.list_workspaces`
  instead of `WorkspaceService.resolve_sandbox_workspace_id`).

Test suite: 3381 passed, 104 skipped.

Versioning
----------

v0.43.0 was released yesterday for the Semantic Layer UI (PR #308) but
its changelog entry was missing from `changelog.py` -- the
`changelog-check` make target was failing. This PR backfills the 0.43.0
entry (reconstructed from the GitHub release notes) AND adds 0.43.1 for
the #312 fix. Plugin.json + marketplace.json synced via `make
version-sync`.
padak added a commit that referenced this pull request May 18, 2026
…ty (#312) (#314)

* fix(0.43.1): sandbox annotation in service layer for HTTP / REST parity (#312)

PR #311 (v0.42.0, closes #304) placed the keboola.sandboxes
parameters.id → storage_workspace_id resolution in `commands/config.py`,
which meant the annotation only fired on `kbagent config detail` CLI
invocations. HTTP / REST callers (`kbagent serve` web UI, scheduled
agents, third-party clients hitting `GET /configs/...`) hit the same
parameters.id trap David Ešner originally reported in #304.

This PR moves the annotation into `ConfigService.get_config_detail()`
behind a new opt-in parameter so all callers can get it.

Architecture
------------

1. **Pure-function helper extraction.** The workspace[].configurationId
   → workspace.id filter logic moves from
   `WorkspaceService.resolve_sandbox_workspace_id` to a module-level
   `find_storage_workspace_for_sandbox_config(workspaces, config_id)`
   in `services/workspace_service.py`. This lets ConfigService call
   it without taking a circular `ConfigService → WorkspaceService`
   dependency in the DI graph. `WorkspaceService.resolve_sandbox_workspace_id`
   becomes a thin wrapper around the helper (still useful for direct
   programmatic callers).

2. **Opt-in service parameter.**
   `ConfigService.get_config_detail()` gains
   `include_sandbox_annotation: bool = False`. Default off so existing
   programmatic consumers see the unchanged shape -- zero-regression
   contract. When the flag is on AND `component_id == "keboola.sandboxes"`
   AND single-config mode, the service fetches `list_workspaces` once
   and stamps the annotation onto the response.

3. **CLI: switch to service-layer annotation.** `commands/config.py`
   drops its ad-hoc post-fetch enrichment block (which previously
   called `WorkspaceService` directly from the command layer -- a
   layering violation) and instead passes
   `include_sandbox_annotation=True` to `get_config_detail`. Bulk mode
   stays off because it would N+1 the workspace listing endpoint.

4. **HTTP / REST parity.**
   `GET /configs/{project}/{component_id}/{config_id}` on `kbagent serve`
   accepts a new query parameter `?include_sandbox_annotation=true`
   (default false), forwarded verbatim to the service. The FastAPI
   `description=` on the Query annotation renders the rationale inline
   in /docs.

5. **Graceful degradation.** If `list_workspaces` fails (rate limit,
   transient 5xx), the detail call still succeeds and
   `storage_workspace_id` is set to `None`. The annotation is UX, not
   a contract -- the caller still gets the raw detail with the same
   shape they would see if `include_sandbox_annotation=False`.

Tests
-----

- **5 new in `test_services.py::TestConfigServiceSandboxAnnotation`**:
  default-off zero-regression, opt-in resolution to real workspace ID,
  orphan (no matching workspace -> storage_workspace_id=None), non-sandbox
  component is no-op (no list_workspaces fan-out), graceful degradation
  on list_workspaces KeboolaApiError.

- **3 new in `test_serve_ui.py::TestConfigDetailSandboxAnnotation`**:
  HTTP router parameter binding -- default-off, opt-in, non-sandbox
  no-op. Stubs `app.state.registry.config.get_config_detail` to avoid
  real Keboola HTTP and assert the router forwards the flag verbatim.

- **3 existing CLI tests in `test_cli.py::TestConfigDetail`** updated
  to mock the new service-layer call path (`client.list_workspaces`
  instead of `WorkspaceService.resolve_sandbox_workspace_id`).

Test suite: 3381 passed, 104 skipped.

Versioning
----------

v0.43.0 was released yesterday for the Semantic Layer UI (PR #308) but
its changelog entry was missing from `changelog.py` -- the
`changelog-check` make target was failing. This PR backfills the 0.43.0
entry (reconstructed from the GitHub release notes) AND adds 0.43.1 for
the #312 fix. Plugin.json + marketplace.json synced via `make
version-sync`.

* fix(0.43.1): address PR #314 review findings (#312)

Two follow-ups to the issue #312 fix in response to /kbagent:review:

1. [NON-BLOCKING] plugins/kbagent/agents/keboola-expert.md VERSION GATE
   now distinguishes the 0.42.0+ CLI sandbox annotation from the new
   0.43.1+ HTTP opt-in (`?include_sandbox_annotation=true` on
   `GET /configs/...`). Kept the VERSION GATE entry tight so the agent
   prompt stays under the 60_000-byte budget (`test_agent_prompt_under_token_budget`).

2. [NON-BLOCKING] Each of the 5 `TestConfigServiceSandboxAnnotation`
   tests now asserts `mock_client.close.assert_called_once()`. This
   pins the contract that the finally block runs on every path --
   fast-path / opt-in / orphan / non-sandbox-no-op / exception-mid-try
   -- so an accidental early return or a regression in the bare
   try/except KeboolaApiError swallow would be caught instead of
   leaking an httpx client per call.

Nits (2 in the original report) were already addressed by the
follow-up.

Test suite: 3381 passed, 104 skipped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant