Skip to content

feat(semantic-layer): add reference-data commands (Chart of Accounts in the metastore)#379

Merged
padak merged 1 commit into
keboola:mainfrom
ottomansky:feat/semantic-layer-reference-data
Jun 4, 2026
Merged

feat(semantic-layer): add reference-data commands (Chart of Accounts in the metastore)#379
padak merged 1 commit into
keboola:mainfrom
ottomansky:feat/semantic-layer-reference-data

Conversation

@ottomansky

@ottomansky ottomansky commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Teaches kbagent the new semantic-reference-data metastore type — a per-dimension member store (one record per dimension; members in a members[] array). Driving use case: hold a Chart of Accounts (the account list + all attributes) in the semantic layer instead of a hardcoded Storage table.

Pairs with go-monorepo keboola/go-monorepo#533 (which adds the semantic-reference-data JSON-Schema to the metastore). This PR is the kbagent client side.

New self-contained sub-app — kbagent semantic-layer reference-data (alias kbagent sl reference-data):

Leaf What Class
list dimension summaries (id, dimension, member_count), optionally one model read
get one record + all members, by --id or --dimension read
set create-or-replace by dimension from a JSON members file (--members-file, - = stdin) write
delete remove a record by UUID (--yes gated) destructive
kbagent sl reference-data set --project P --dimension chart_of_accounts \
  --members-file coa.json --dataset-id in.c-finance.DIM_COA

Dimension is the project-unique key

The metastore envelope name (= the dimension) is unique per project per type, so the dimension is the record's project-unique key:

  • set resolves the existing record project-wide by dimension — it stays idempotent regardless of which --model is passed (the resolved model is stored on the record, it is never the lookup key). An existing record → revisioned PUT (history preserved); a brand-new dimension → POST. This avoids the earlier failure mode where set --model B on a dimension created under model A would POST and collide with ALREADY_EXISTS.
  • get resolves by --id or --dimension (project-wide; no --model needed). Passing both, or neither, is a usage error (exit 2).

Why it's small / low-risk

reference-data is not a model child that build generates — its members come from DIM_COA, not the AI. So it is deliberately kept out of PUSH_ORDER / build / export / diff / cascade. The sub-app composes the generic metastore verbs (list_items / get_item / post_item / put_item / delete_item) — zero blast radius on existing model flows.

Implementation

  • metastore_client.py — register semantic-reference-data in SemanticType / SEMANTIC_TYPES; add a put_item verb so set uses the metastore's real revisioned PUT (preserves history) rather than the DELETE+POST the edit ops use.
  • services/_semantic_layer_reference_data.pyrun_list/get/set/delete helpers (project-wide find_by_dimension); SemanticLayerService exposes thin delegators so the orchestrator stays under the 1500-LOC ceiling.
  • server/routers/semantic_layer.pyGET /reference-data, GET /reference-data/{id}, PUT /reference-data, DELETE /reference-data/{id} (1:1 CLI→HTTP) + mocked-registry parity tests.
  • permissions.py — registry entries for all four leaves (list/get read, set write, delete destructive).
  • No --hint definitions--hint is DEPRECATED per CONTRIBUTING.md; the programmatic surface is the serve REST router only.
  • Docscontext.py AGENT_CONTEXT, CLAUDE.md command list, commands-reference.md, gotchas.md, SKILL.md (triggers + regenerated table), all tagged since v0.55.0 (avoids the 0.54.0 collision with feat(auth): browser OAuth project login — kbagent project login (PKCE) (0.54.0) #377). keboola-expert.md not extended — it is at its enforced 62 KB prompt-budget ceiling.

Tests

  • Service: CRUD, create-vs-replace, cross-model idempotency regression (existing record under a different model → still PUT-replaces, no ALREADY_EXISTS), members-not-list validation, NOT_FOUND, id-vs-dimension resolution, permission-registry asserts.
  • CLI: list/get/set/delete, --id/--dimension mutual-exclusion (exit 2), bad-JSON exit-2, non-TTY --yes gate.
  • metastore_client: TestPutItem (httpx_mock) — envelope shape, PUT /{type}/{id} URL, 404 propagation.
  • serve router: mocked-registry parity for all four routes.
  • E2E (convention Close must-have gaps in explorer command #16): CLI lifecycle hop in test_e2e.py + 5 HTTP-route hops in test_server_semantic_layer_routes_e2e.py, green live against project 1143 (99_Playground_Max).
  • Full unit suite green (3877 passed); ruff check / ruff format / ty clean.

Review feedback addressed (@padak)

Note: the version bump in pyproject.toml + the changelog.py entry are left for release-time per CONTRIBUTING.md (so the actual number tracks merge order with #377).

@ottomansky ottomansky left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review of #379 — feat(semantic-layer): add reference-data commands (Chart of Accounts in the metastore)

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 self-contained kbagent semantic-layer reference-data sub-app (four leaves: list / get / set / delete) backed by a new put_item verb on MetastoreClient. The driving use-case is a Chart of Accounts held in the metastore instead of a flat Storage table. The deliberate exclusion from PUSH_ORDER / build / export / diff / cascade is architecturally sound and completely verified — semantic-reference-data appears nowhere in _semantic_layer_internals.py, _semantic_layer_crud.py, or _semantic_layer_cascade.py. The 3-layer boundaries are clean, permissions are fully registered, and make check passes (3868 passed, 8 skipped). However, make typecheck exits non-zero due to one new diagnostic introduced by this PR: tests/test_semantic_layer_service.py:2823 uses a mypy-flavour # type: ignore[arg-type] suppressor that ty does not honor. All plugin-doc surfaces and the serve router are deferred per the PR description; those are flagged as known NON-BLOCKING gaps, not surprises. Verdict: REQUEST CHANGES on the single typecheck regression.

Verdict

  • Verdict: REQUEST CHANGES
  • Blocking findings: 1
  • Non-blocking findings: 6
  • Nits: 2

Blocking findings

[B-1] tests/test_semantic_layer_service.py:2823 — wrong suppressor flavor; ty still reports invalid-argument-type

make typecheck exits 1 with error[invalid-argument-type]: Argument to bound method SemanticLayerService.set_reference_data is incorrect. The test at line 2823 suppresses the deliberate bad-type call with # type: ignore[arg-type], which is mypy syntax. This repo uses Astral ty (not mypy); ty does not respect # type: ignore[...] annotations — only # ty: ignore[...] ones. Per CONTRIBUTING.md §"Type checking — ty is mandatory and BLOCKING": "Adding any # ty: ignore[rule] requires a one-line comment explaining why". Pre-existing ty errors in test_config_store.py and test_services.py are from earlier commits on this branch, not from this PR's commit (644a8cc).

Fix: change line 2823 to:

members={"not": "a list"},  # ty: ignore[invalid-argument-type]  # intentional bad type to exercise the isinstance guard

Non-blocking findings

[NB-1] src/keboola_agent_cli/server/routers/semantic_layer.py — four reference-data routes missing; deferred with no documented skip reason in CONTRIBUTING.md pattern

Per CONTRIBUTING.md Checklist: Adding a New CLI Command > "HTTP API endpoint": every new command has a matching route in the group's router (server/routers/semantic_layer.py); skip is allowed only for "genuinely terminal-only commands" with a one-line reason in the PR description. The PR description acknowledges the deferral (REST serve router parity) but classifies it under "Deferred (intentionally — flagged for follow-up)". The four leaves (GET /reference-data, GET /reference-data/{id}, PUT /reference-data, DELETE /reference-data/{id}) would be the natural 1:1 mirror. As a draft PR this is acceptable, but the issue should be explicitly tracked before the PR leaves draft. Callers of kbagent serve who hit the semantic-layer REST surface will get HTTP 404 for reference-data endpoints with no hint that the CLI commands exist — the silent-gap described in the Plugin synchronization map.

[NB-2] src/keboola_agent_cli/commands/context.pyAGENT_CONTEXT not updated; reference-data sub-commands absent

grep -n "reference.data" src/keboola_agent_cli/commands/context.py returns nothing. Per CONTRIBUTING.md Plugin synchronization map, AGENT_CONTEXT is the primary command catalogue loaded by AI agents at session start; missing entries mean the keboola-expert subagent will not know the commands exist. This is acknowledged as deferred in the PR description, but it is a CONTRIBUTING.md "NO" surface that ships a silent gap. Flag for the follow-up.

[NB-3] CLAUDE.md ## All CLI Commandssemantic-layer reference-data commands not listed

grep -n "reference.data" CLAUDE.md returns nothing. The four new commands are absent from the hand-maintained command list. Per CONTRIBUTING.md Plugin synchronization map, this is a "NO" (no CI check) silent-drift surface. Acknowledged as deferred.

[NB-4] plugins/kbagent/skills/kbagent/references/commands-reference.md — no entry for reference-data

grep -n "reference.data" plugins/kbagent/skills/kbagent/references/commands-reference.md returns nothing. Per CONTRIBUTING.md this is BLOCKING on normal PRs but is acknowledged as deferred here. Flagging as NON-BLOCKING given the PR's explicit draft status and deferral notice.

[NB-5] plugins/kbagent/skills/kbagent/SKILL.md description block — no reference-data / chart of accounts / dimension member trigger keywords

The auto-generated commands table (lines 108-366, CI-checked) correctly lists all four reference-data commands. The hand-maintained description: frontmatter (lines 1-67) does not include reference-data, chart of accounts, COA, dimension member, or related trigger terms. Without those keywords in the description block, the skill will not auto-activate when a user asks about Chart of Accounts maintenance in the metastore — they will not see the commands at all unless they already know to ask kbagent. This is a NON-BLOCKING quality gap (description triggers are not CI-checked).

[NB-6] tests/test_metastore_client.py — new put_item verb has no dedicated unit test class

TestPutItem does not exist in tests/test_metastore_client.py. The put_item method (metastore_client.py:177-207) is tested only indirectly via the service-layer mock in TestReferenceDataSet. A direct httpx_mock-backed test (matching TestPostItem and TestDeleteItem) would catch: (a) correct envelope shape (name, data, branch, schemaVersion, scope), (b) 404 propagation, and (c) the PUT /{type}/{id} URL construction. The service-layer mock does call put_item.assert_called_once() but does not exercise the HTTP path.

Nits

  • [NIT-1] src/keboola_agent_cli/commands/_semantic_layer_reference_data.py:225,259 — both formatter.output(result, lambda ...) calls use assigned-meaning lambdas (multi-field f-string formatting with domain meaning). Per CONTRIBUTING.md "Named functions over throwaway lambdas": lambdas assigned to variables or carrying domain meaning should be named def functions. The existing _print_reference_data_table and _print_reference_data_detail functions at the top of the file set the right precedent; the set and delete human-mode formatters could follow the same pattern as _print_reference_data_set_result / _print_reference_data_delete_result.

  • [NIT-2] src/keboola_agent_cli/services/semantic_layer_service.py:931assert dimension is not None inside a try block in production code. The guard is logically correct (the if record_id is None and dimension is None check at line 919 guarantees dimension is set on that branch), but assert is stripped by -O and is harder to trace in production logs than an explicit if. A raise KeboolaApiError(error_code=ErrorCode.VALIDATION_ERROR, message="Internal: dimension must be set when record_id is None") would be more consistent with the rest of the service's defensive error discipline — though the unreachability is real.

Verification log

  • git rev-parse --abbrev-ref HEADfeat/semantic-layer-reference-data ✓ (matches <branch>)
  • gh pr view 379 --repo keboola/cli --json title,state,files,additions,deletions → 10 files, +971/-1, state OPEN ✓
  • CONTRIBUTING.md Plugin synchronization map read ✓
  • CLAUDE.md § All CLI Commands read ✓
  • plugins/kbagent/agents/keboola-expert.md §1/§2/§3 read ✓
  • Layer violation scan (typer/click in services, httpx in commands, formatter in clients): all empty ✓ (no violations)
  • grep -n "reference.data" src/keboola_agent_cli/permissions.py → 5 entries (semantic-layer.reference-data, .list, .get = read; .set = write; .delete = destructive) ✓
  • grep -n "semantic-reference-data" src/keboola_agent_cli/services/_semantic_layer_internals.py _semantic_layer_crud.py _semantic_layer_cascade.py → empty ✓ (reference-data correctly excluded from build/export/diff/cascade/PUSH_ORDER)
  • grep "^@router\." src/keboola_agent_cli/server/routers/semantic_layer.py → 16 routes, none for reference-data ✓ (confirmed missing; acknowledged deferred)
  • grep -n "reference.data" src/keboola_agent_cli/commands/context.py CLAUDE.md → empty (both deferred) ✓
  • grep -n "reference.data" plugins/kbagent/skills/kbagent/references/commands-reference.md gotchas.md → empty (both deferred) ✓
  • grep -n "reference.data" plugins/kbagent/agents/keboola-expert.md → empty (deferred) ✓
  • Magic numbers scan: empty ✓
  • Raw error-code strings scan: empty ✓ (ErrorCode enum used throughout)
  • Bare except: scan: empty ✓
  • print() in production code scan: empty ✓
  • Token-in-output scan: empty ✓
  • File-size check: semantic_layer_service.py = 1615 LOC (soft ceiling 1000, hard 1500) → over hard ceiling but pre-existing; this PR adds 183 LOC. The ceiling violation is not introduced by this PR (it was already above 1500 before this commit). Noted for awareness; not a finding against this PR.
  • make check → ruff all checks passed, 309 files formatted, SKILL.md up-to-date, version in sync, changelog complete, no raw error_code strings, 3868 passed / 8 skipped ✓
  • make typecheck → exits 1; error[invalid-argument-type] at tests/test_semantic_layer_service.py:2823 (PR-introduced); error[unresolved-attribute] × 2 in test_config_store.py:965 and test_services.py:70 (pre-existing from earlier commits on this branch, not from commit 644a8cc) ✗ (B-1)
  • put_item in metastore_client.py:177-207: envelope shape is identical to post_item (same name/data/branch/schemaVersion/scope fields); correct PUT /{type}/{id} path; raises KeboolaApiError on 404 via BaseHttpClient error mapping ✓ — design is sound
  • Idempotency path (set_reference_data): _find_reference_data_for_model client-side scan → if found, PUT with existing UUID (revision++); else POST. Correct; no race-condition concern for single-caller CLI context ✓
  • E2E test for reference-data: absent; acknowledged as deferred in PR description ✓ (per CONTRIBUTING.md convention #16, flagged NON-BLOCKING — every CLI command must have E2E coverage, deferred one cycle per PR note)
  • kbagent semantic-layer reference-data list/get/set/delete runtime behavior: not reproduced locally (no live metastore credentials or project with semantic-reference-data records available in this review environment). Service-layer and CLI-layer unit tests cover the paths; unverified at runtime — per playbook §3.6 this becomes NON-BLOCKING with author confirmation needed

Open questions for the author

  1. The semantic_layer_service.py file is at 1615 LOC, 115 lines past the 1500 hard ceiling set in CONTRIBUTING.md. The reference-data block adds 183 lines that are self-contained (static method _unpack_reference_record, static method _find_reference_data_for_model, four public methods). Would extracting these into _semantic_layer_reference_data.py (a sibling service helper, mirroring the existing _semantic_layer_crud.py / _semantic_layer_internals.py split) be the right follow-up, or is the plan to address the file size in a dedicated split PR?

  2. put_item lacks a direct httpx_mock-backed unit test (NB-6). Given the critical role of the PUT verb in the idempotency contract (revision preservation vs. DELETE+POST), is a dedicated TestPutItem class planned in the follow-up, or is the service-level mock coverage considered sufficient?

@ottomansky ottomansky force-pushed the feat/semantic-layer-reference-data branch from 644a8cc to ff0ff33 Compare June 3, 2026 13:11
@ottomansky

Copy link
Copy Markdown
Contributor Author

Addressed the blocking finding in ff0ff33: the deliberate-bad-type test now binds the value to an Any-typed local (bad_members: Any = {...}) instead of the mypy-style # type: ignore[arg-type], so ty/make typecheck is clean (my changed files pass ty check; the few remaining repo-wide ty diagnostics are pre-existing on main, unrelated to this PR). The other findings are the intentionally-deferred gaps already listed in the PR description (serve router parity, AGENT_CONTEXT, CLAUDE.md command list, commands-reference.md, keboola-expert.md, E2E hop). make check green (3868 tests).

@ottomansky ottomansky force-pushed the feat/semantic-layer-reference-data branch from ff0ff33 to 9c0bf65 Compare June 3, 2026 13:33
@ottomansky

Copy link
Copy Markdown
Contributor Author

Pushed 9c0bf65 — all review findings now addressed (was REQUEST CHANGES → all resolved):

Blocking

  • B-1ty suppressor — the deliberate bad-type test now binds an Any-typed local (no mypy-style # type: ignore); all changed files pass ty check.

Non-blocking (all done, not deferred)

  • NB-1 ✅ serve REST router: GET /reference-data, GET /reference-data/{id}, PUT /reference-data, DELETE /reference-data/{id} (1:1 CLI→HTTP) + mocked-registry parity tests in test_server_router_calls.py.
  • NB-2context.py AGENT_CONTEXT. NB-3CLAUDE.md command list. NB-4commands-reference.md. NB-5SKILL.md description triggers (chart of accounts / COA / reference data / dimension members) + regenerated table.
  • NB-6TestPutItem (httpx_mock): envelope shape, PUT /{type}/{id} URL, 404 propagation.

Nits

  • NIT-1 ✅ named _print_reference_data_set_result / _print_reference_data_delete_result (no assigned-meaning lambdas).
  • NIT-2assert replaced by explicit control flow + KeboolaApiError.

Open questions

  • Q1 (file size) ✅ extracted the logic into services/_semantic_layer_reference_data.py (run_* helpers, open_client thunk pattern); semantic_layer_service.py is now 1496 LOC (under the 1500 ceiling). Q2 addressed by NB-6.

Also added a gotchas.md entry (whole-record PUT-replace semantics, per-project name uniqueness, snake_case DIM_COA keys, exclusion from build/export/diff/cascade).

Deliberately NOT changed: keboola-expert.md — it is at its enforced 62 KB prompt-budget ceiling (test_agent_prompt.py); adding rows there fails the budget test, and the maintainer comment says to split into specialists rather than grow it. reference-data is covered by the other five doc surfaces.

Local: full suite 3871 passed; ruff check/ruff format clean; ty clean for all changed files (the 3 remaining repo-wide ty diagnostics are pre-existing on main); SKILL.md freshness + plugin.json version-sync pass. Version bump + changelog.py entry omitted (release-time per CONTRIBUTING.md).

CI is currently in action_required (fork-PR workflow-approval gate) — needs a maintainer to approve the run.

@ottomansky ottomansky force-pushed the feat/semantic-layer-reference-data branch from 9c0bf65 to d5530ed Compare June 3, 2026 13:46
@ottomansky

Copy link
Copy Markdown
Contributor Author

Added E2E coverage (convention #16), run green live against project 1143 (99_Playground_Max) where the semantic-reference-data schema is already deployed:

  • tests/test_e2e.py::TestE2ESemanticLayerLifecycle::test_semantic_layer_reference_data_roundtrip — CLI lifecycle: set (create) → listget --idset (replace, asserts action=updated + same id) → delete, with model bootstrap + teardown.
  • tests/test_server_semantic_layer_routes_e2e.py — 5 HTTP-route hops exercising PUT /reference-data (create + replace), GET /reference-data, GET /reference-data/{id}, DELETE /reference-data/{id} against the live metastore, with tracked-item cleanup + the session residue scan.

Both gate behind E2E_API_TOKEN/E2E_URL like the existing E2E suites (skip without creds). Verified live: create returns action: created, replace returns action: updated with the same UUID (revisioned PUT), delete removes the record, cleanup leaves no residue.

Commit d5530ed. Full unit suite still green (3871 passed); ruff/format/ty clean.

@padak padak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Preliminary review (draft) — semantic-layer reference-data

Reviewed the full diff against CONTRIBUTING.md. This is a clean, low-risk PR — no blocking bug. put_item correctly goes through the shared MetastoreClient._do_request (tolerates < 400), the 3-layer split is respected, and per-leaf permissions are enforced properly. What's left before "ready for review" is mostly architectural decisions and process hygiene.

🟡 NON-BLOCKING (resolve before finalizing)

1. find_for_model is model-scoped, but dimension uniqueness is project-scoped → set idempotency breaks.
In services/_semantic_layer_reference_data.py, find_for_model(client, model_uuid, dimension) filters records by model_uuid. But this PR's own gotchas.md states the envelope name (= dimension) is unique per project per type. Scenario:

  • dimension chart_of_accounts exists under model A;
  • set --model B --dimension chart_of_accountsfind_for_model(B, …) returns None (the record belongs to A, filtered out) → the code takes the POST path → the server rejects with ALREADY_EXISTS.

So an idempotent set ends in a confusing ALREADY_EXISTS instead of a PUT-replace whenever the model doesn't match. Question for the author: should the lookup be project-wide by dimension (uniqueness is project-scoped anyway), so idempotency works regardless of the --model passed? At minimum, translate ALREADY_EXISTS into an actionable hint ("dimension already exists under a different model").

2. Version 0.54.0 collision with #377.
gotchas.md, commands-reference.md and context.py tag reference-data as since v0.54.0. But the OAuth PR #377 targets the same 0.54.0 and is closer to merge (it already carries a changelog entry; this PR defers it to release-time). If OAuth merges as 0.54.0, reference-data becomes 0.55.0 and every since v0.54.0 tag here is wrong. Align the target version before merge.

3. New --hint definitions go against the deprecated-hint policy.
The PR adds ~139 lines of hint definitions in hints/definitions/semantic_layer.py. CONTRIBUTING.md (the "Adding a New CLI Command" checklist) is explicit: "--hint … DEPRECATED, do not add. Do not add a new hints/definitions/ entry … point readers at kbagent serve instead." It's consistent with the local pattern (the other sl commands have hints), but against the written policy. The serve REST router already covers the programmatic surface — consider dropping these, or justify keeping them in the PR description.

🟢 NIT / process

4. Co-Authored-By + AI attribution footer — the commit carries Co-Authored-By: Claude Opus 4.8 and the PR body 🤖 Generated with Claude Code. CONTRIBUTING.md → "Commit & PR Conventions" forbids both ("No Co-Authored-By lines", "No AI attribution footers"). Note a squash merge would carry that trailer onto main.

5. PR description "Deferred (intentionally)" section is stale. It lists the serve router, commands-reference.md, gotchas.md, context.py, CLAUDE.md and E2E as "Not in this PR" — but the commit actually adds all of them, and they're in the diff. The commit message is accurate; the PR body isn't. Update it so reviewers don't go hunting for "missing" things that are present.

6. get --id and --dimension together--id silently wins and --dimension is ignored. Could reject the combination (exit 2) instead of silently preferring one.

7. run_list builds a project key in unpack_record then immediately rec.pop("project", None). Minor awkwardness — a summary variant or a flag would be cleaner.

✅ Verified good (not assumed)

  • Per-leaf permissions — confirmed delete is checked as …reference-data.delete (destructive). This is the first sub-app with mixed classes (read/write/destructive under one group); it works because check_cli_permission composes the key from invoked_subcommand, and the parent …reference-data: read key lets the callback descend without over-blocking list/get.
  • No status-code bugput_item uses the shared _do_request (< 400), unlike #377.
  • 3-layer — L3 metastore_client, L2 helper split (_semantic_layer_reference_data.py) to stay under the 1500-LOC ceiling, thin delegators, L1 thin Typer.
  • Zero blast radius — deliberately outside build/export/diff/cascade/PUSH_ORDER; composes the generic verbs.
  • serve 1:1 (4 routes) + parity tests; E2E (CLI lifecycle hop + 5 HTTP-route hops) live against project 1143.
  • Doc sync — CLAUDE.md, SKILL.md (regenerated + triggers), commands-reference, gotchas (since v0.54.0), context.py; keboola-expert.md intentionally skipped (62 KB ceiling).
  • Imports — all new imports resolve to existing symbols.
  • Validation — members-list check at CLI (exit 2) and service (VALIDATION_ERROR) layers; --yes gate + non-TTY refusal on delete.

Summary: items 1 and 2 are the only ones with real behavior/user impact; the rest is hygiene. Nice, self-contained work overall.

@ottomansky ottomansky force-pushed the feat/semantic-layer-reference-data branch from d5530ed to b71daf8 Compare June 3, 2026 15:11
@ottomansky

Copy link
Copy Markdown
Contributor Author

Thanks @padak — all review points addressed in b71daf8 (squashed; force-pushed).

Behaviour (#1, #2)

  • Phase 1: Project Skeleton #1 idempotency — agreed: dimension uniqueness is project-scoped, so model-scoped lookup was the bug. set/get now resolve project-wide by dimension via find_by_dimension. set stays idempotent regardless of --model (a record created under model A is PUT-replaced when you set --model B --dimension <same>, instead of POSTing into ALREADY_EXISTS); the resolved model is stored on the record, never used as the lookup key. Since the dimension alone identifies the record, I dropped --model from get rather than silently ignore it. New regression test: test_set_is_idempotent_across_models. gotchas.md updated to match.
  • Phase 2: Project management (client, config store, service, CLI) #2 version — all since tags moved to v0.55.0, ceding 0.54.0 to feat(auth): browser OAuth project login — kbagent project login (PKCE) (0.54.0) #377. (pyproject bump + changelog entry stay release-time per CONTRIBUTING, so the final number still tracks merge order.)

Policy / process (#3, #4, #5)

Nits (#6, #7)

Local: full unit suite green (3877 passed), ruff check / ruff format / ty clean for changed files; skill-gen regenerated; version/changelog/error-code gates pass.

@padak padak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Follow-up review — b71daf8

Thanks @ottomansky, that's a thorough turnaround. Re-checked all seven against the code (not just the reply) — all resolved:

# Finding Verified in b71daf8
1 model-scoped lookup broke set idempotency find_by_dimension lists REFERENCE_DATA_TYPE with no model_uuid → project-wide; get dropped --model; resolved model is stored on the record, not used as the key. Correct root-cause fix
2 0.54.0 collision with #377 all since tags → v0.55.0
3 --hint defs vs deprecated policy hint definitions + should_hint/emit_hint branches removed
4 Co-Authored-By / AI footer squashed clean, no trailer/footer
5 stale "Deferred" description PR body rewritten to match the diff
6 get --id+--dimension silent now exit 2 for both-or-neither
7 run_list build-then-pop unpack_record(..., include_project=False)

The #1 fix is correct: list_items filters by model client-side, so the old code resolved --model None to the default model and filtered out a record living under a different model → None → POST → ALREADY_EXISTS. Passing model_uuid=None skips that filter and the project-unique dimension is always found. 👍

One new nit (introduced by the #1 fix's test, not the prod code)

test_set_is_idempotent_across_models doesn't actually guard the regression. The _list mock helper ignores its model_uuid argument:

def _list(item_type, model_uuid=None):
    if item_type == "semantic-model": return [_model_item("U", "m")]
    return extra.get(item_type, [])          # model_uuid unused

But the real list_items filters client-side on attributes.modelUUID == model_uuid. Because the mock skips that filter, the old model-scoped find_for_model would also pass this test (the mock would hand it the OTHER_MODEL record anyway) — so it can't distinguish the buggy implementation from the fixed one. Make the mock filter to actually pin the behavior:

items = extra.get(item_type, [])
if model_uuid is not None:
    items = [i for i in items if (i.get("attributes") or {}).get("modelUUID") == model_uuid]
return items

Then the old code would get [] → POST → action="created" → FAIL, while the fixed code PUTs → "updated" → PASS. Test-quality only; the production fix is right either way.

Everything from my earlier review is addressed — from my side this is good to go once the test mock is tightened (and pending the release-time version/changelog bump).

@ottomansky ottomansky marked this pull request as ready for review June 4, 2026 08:45
…s in the metastore)

Teach kbagent the new `semantic-reference-data` metastore type — a
per-dimension member store (one record per dimension, members in a
`members[]` array). Driving use case: hold a Chart of Accounts (the account
list + all attributes) in the semantic layer instead of a hardcoded Storage
table. Pairs with keboola/go-monorepo#533 (the metastore schema side).

CLI sub-app `kbagent semantic-layer reference-data` (alias `kbagent sl`):
- `list`   — dimension summaries (id, dimension, member_count)        [read]
- `get`    — one record + members, by --id or --dimension             [read]
- `set`    — create-or-replace from a JSON members file; idempotent on
             the dimension (project-wide lookup): existing -> PUT/
             revision++, else POST                                    [write]
- `delete` — remove by UUID (--yes gated)                       [destructive]

The dimension name is the metastore envelope `name`, unique per project per
type, so it is the record's project-unique key: `set`/`get` resolve by
dimension project-wide and `set` stays idempotent regardless of which
`--model` is passed (the resolved model is stored on the record, never the
lookup key). `get` rejects `--id` and `--dimension` together (exit 2).

Implementation:
- metastore_client: register the type in SemanticType / SEMANTIC_TYPES;
  add a `put_item` verb so `set` uses the metastore's revisioned PUT
  (preserves history) instead of DELETE+POST.
- Logic in services/_semantic_layer_reference_data.py (run_* helpers,
  open_client thunk); the service exposes thin delegators, keeping
  semantic_layer_service.py under the 1500-LOC ceiling.
- serve REST router: GET /reference-data, GET /reference-data/{id},
  PUT /reference-data, DELETE /reference-data/{id} (1:1 CLI->HTTP).
- permissions registered for all four leaves. No `--hint` definitions:
  hints are DEPRECATED per CONTRIBUTING.md (use `kbagent serve`), so the
  programmatic surface is the serve REST router only.

Deliberately self-contained: reference-data is NOT AI-generated, so it is
kept OUT of build/export/diff/cascade/PUSH_ORDER — zero blast radius on the
existing model flows.

Docs synced (Plugin synchronization map): context.py AGENT_CONTEXT,
CLAUDE.md command list, commands-reference.md, gotchas.md, SKILL.md
(triggers + auto-generated table), all tagged since v0.55.0 to avoid the
0.54.0 version collision with the OAuth login PR. keboola-expert.md
intentionally NOT extended — it is at its enforced 62 KB prompt-budget
ceiling.

Tests: service CRUD + cross-model idempotency regression + permission
registry + CLI (incl. --id/--dimension mutual-exclusion) + metastore_client
TestPutItem (httpx_mock) + serve-router parity (mocked registry). E2E
(convention keboola#16): CLI lifecycle hop in test_e2e.py and 5 HTTP-route hops in
test_server_semantic_layer_routes_e2e.py — both run green live against
project 1143 (99_Playground_Max), set/list/get/replace/delete with model
bootstrap + cleanup + residue scan. Full unit suite green; ruff/format/ty
clean.

Version bump + changelog entry are release-time per CONTRIBUTING.md.
@ottomansky ottomansky force-pushed the feat/semantic-layer-reference-data branch from b71daf8 to e220e0a Compare June 4, 2026 08:47
@ottomansky

Copy link
Copy Markdown
Contributor Author

Good catch @padak — you're right, the mock didn't pin the behavior. Fixed in e220e0a.

_model_only_list._list now mirrors MetastoreClient.list_items' client-side modelUUID filter:

items = extra.get(item_type, [])
if model_uuid is not None:
    items = [i for i in items if (i.get("attributes") or {}).get("modelUUID") == model_uuid]
return items

Proved it actually guards the regression now: I temporarily reintroduced the old model-scoped lookup in run_set (list_items(REFERENCE_DATA_TYPE, model_uuid) resolving --model None → default "U"), and test_set_is_idempotent_across_models failed (action="created" instead of "updated" — the buggy POST path) — exactly your prediction. Restored the fix → passes. The model_uuid=None lookups in the other reference-data tests are unaffected (filter only activates when a model is passed).

Test-only change; production code untouched (b71daf8e220e0a differ solely in tests/test_semantic_layer_service.py). TestReferenceData 10/10 green, ruff/format clean.

Agreed on the rest — version/changelog stay release-time per CONTRIBUTING (so the number tracks merge order with #377). Thanks for the thorough re-review.

@padak padak merged commit a1ec6b8 into keboola:main Jun 4, 2026
2 checks passed
padak added a commit that referenced this pull request Jun 4, 2026
PR #379 merged the semantic-layer reference-data commands without a
changelog entry (version bump deferred to release-time per CONTRIBUTING).
Add the entry to the pending 0.55.0 release alongside #383 (sync audit)
and #384 (hint removal) so 0.55.0 ships complete release notes.
padak added a commit that referenced this pull request Jun 4, 2026
…livery (#399)

0.55.0 sat in main across three builds (#383, #379, #388) under one
version number, so users on an interim 0.55.0 build never received the
reference-data commands via `kbagent update` (the auto-update check is
version-only: 0.55.0 >= 0.55.0 reads as up-to-date). Bump to 0.56.0 gives
auto-update a strictly-greater target. No code changes; reference-data
stays recorded under 0.55.0 and its since-tags remain correct.
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.

2 participants