Skip to content

feat(semantic-layer): metastore scope/ACL support (targeted + org-wide) [AI-3790] - #715

Merged
Matovidlo merged 6 commits into
mainfrom
martinvasko-ai-3790-verify-that-the-new-semantic-models-and-datasets
Oct 5, 2026
Merged

Matovidlo merged 6 commits into
mainfrom
martinvasko-ai-3790-verify-that-the-new-semantic-models-and-datasets

Conversation

@Matovidlo

@Matovidlo Matovidlo commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds kbagent CLI and kbagent serve support for the new metastore ACL scope model (PSGO-140): items can now be shared with specific projects or made organization-wide, instead of only ever being project-private.

  • --scope project|organization|targeted and --target-project ALIAS|ID (repeatable or comma-separated) on semantic-layer model create and every semantic-layer add <kind>. With --scope omitted, model create makes a project model and add <kind> inherits its model's scope and target projects, so an org-level model does not show up in consumer projects with no datasets or metrics.
  • New semantic-layer scope command group, with verbs following the CLI spec (#791): get, add, remove, set, request-create, request-delete, request-list.
    • scope set takes exactly one of --scope organization (elevation, irreversible, org-admin only), --target-project (replaces the whole list) or --clear, with --dry-run and --yes.
    • scope add|remove merge into the grants and are refused (exit 2) from a project that does not own the item, because the server hides the grants from a non-owner and a merge would overwrite them.
    • request-list returns items, limit, offset and has_more.
  • --target-project takes a registered alias or a numeric project ID, so a target need not be registered. An alias must be on the owner project's stack.
  • Usage errors exit 2: an unknown alias, a bad --scope/--type, --target-project without --scope targeted, a mixed scope set request.
  • With --scope targeted and no --target-project, a real terminal runs an interactive picker (projects on the owner's stack only); --json/non-interactive fails fast (exit 2).
  • Permissions: --scope organization is destructive-class through FLAG_ESCALATIONS on scope set (also with --dry-run), model create and every add <kind>. An add <kind> that would inherit organization from its model is gated the same way. get and request-list are read; add, remove, set, request-create and request-delete are write.
  • kbagent serve: 7 scope routes (GET|PUT /semantic-layer/scope/{context_id}, POST|DELETE .../target-projects, PUT|DELETE .../elevation-request, GET /semantic-layer/scope/elevation-requests), plus scope and target_projects on POST /semantic-layer/models and /items/{kind}.
  • keboola-expert.md has an explicit rule: never widen an item's visibility without the user having named the target project(s) first.

Bugs fixed along the way

  • The metastore envelope hardcoded schemaVersion: "1.0.0", which only supports scope=project. kbagent no longer sends schemaVersion on create: the server stores the stack's default schema, and a later elevation is checked against that stored version, so a pinned 1.0.0 would make every item created without --scope impossible to elevate.
  • edit, import --overwrite and promote used DELETE+POST, which reset an organization/targeted item to project-only visibility, dropped a pending elevation request and changed the item ID. They now update in place with PUT: the item keeps its ID, scope, grants and pending request, and a failed update changes nothing (no rollback is needed any more; the rollback field is always null). A rename to a taken name maps to ALREADY_EXISTS.

PSGO-282 (master-token wording, 6 doc files plus metastore_client.py): the metastore accepts any valid token for reads and needs a project-admin token only for writes. Docs and the 401 message now say that, and the 401 message keeps the session-aware remedy from main.

Impact analysis

  • metastore_client.py: scope/grant/elevation primitives; post_item gains scope/target_project_ids and sends no schemaVersion; put_item sends only name and data and maps 409 to ALREADY_EXISTS.
  • services/_semantic_layer_scope.py (new), semantic_layer_service.py, _semantic_layer_crud.py, _semantic_layer_internals.py: scope logic, scope inheritance, child_scope(), in-place edit/overwrite paths.
  • commands/_semantic_layer_scope.py (new), _semantic_layer_crud.py, _semantic_layer_helpers.py, semantic_layer.py: scope sub-app, flags, permission gates.
  • server/routers/semantic_layer.py, _serve_command_map.py, docs/web-server-endpoints.md: scope routes.
  • permissions.py: semantic-layer.scope.* entries and the --scope organization escalations.
  • Docs: CLAUDE.md, commands/context.py, commands-reference.md, gotchas.md, SKILL.md (regenerated, plus a workflow-table row), metastore-scope-workflow.md, semantic-layer-workflow.md, keboola-expert.md.
  • Behavior changes: add <kind> without --scope now inherits the model's scope (it used to always be project); the scope commands are renamed (not released yet, so no deprecation needed).

Test plan

  • New and rewritten tests: tests/test_metastore_client.py, tests/test_semantic_layer_scope.py (target resolution, grant merge with the non-owner guard, elevation, inheritance, child_scope, in-place edit/import/promote), tests/test_semantic_layer_scope_cli.py (flags, exit codes, permission matrix incl. inherited scope and dry-run), tests/test_server_router_calls.py (scope routes and body validation), tests/test_semantic_layer_service.py, and a new block in TestE2ESemanticLayerLifecycle (needs a live stack).
  • Full suite: pytest tests/ gives 7357 passed, 189 skipped (e2e need E2E_API_TOKEN/E2E_URL), 0 failed (excluding tests/test_build_hook.py, which also fails without these changes in my environment). ruff check, ruff format --check and ty check src are clean; check_command_sync, check_sentinel_guards, check_version_gates, check_error_codes and check_file_size pass.
  • Not yet verified against a live stack with PSGO-140 deployed (grounded in go-monorepo source, schema files and route definitions). To run once this PR is green: create targeted, add a target, create a request, elevate, edit, list the requests, including one item created without --scope and one created by an earlier kbagent. Open questions that depend on it: whether a rename through PUT is accepted, whether a create without schemaVersion stores the stack default, and whether items pinned to 1.0.0 by an older kbagent can be elevated.

Related issues

Linear: AI-3790

🤖 Generated with Claude Code

@linear-code

linear-code Bot commented Aug 28, 2026

Copy link
Copy Markdown

AI-3790

@Matovidlo
Matovidlo requested a lite review from Copilot August 28, 2026 12:51
@Matovidlo
Matovidlo marked this pull request as ready for review August 28, 2026 12:52
@Matovidlo
Matovidlo requested a review from soustruh August 28, 2026 12:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds CLI + service support for the metastore’s new semantic-layer visibility model (PSGO-140): items can be project-private (default), shared with specific projects (targeted grants), or elevated to organization-wide visibility, including new scope management subcommands and scope preservation across DELETE+POST edits.

Changes:

  • Add --scope project|organization|targeted + --target-project to semantic-layer creation commands, with an interactive picker fallback for targeted scope.
  • Introduce semantic-layer scope subcommands (status/grant/request-elevation/withdraw-elevation/elevate/pending) plus corresponding service/client primitives.
  • Update metastore envelope schemaVersion to 1.1.0 and preserve scope/grants during edit flows.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/test_semantic_layer_scope.py Unit tests for scope helpers + service orchestration + edit-preserves-scope regression coverage.
tests/test_semantic_layer_scope_cli.py CLI tests for --scope/--target-project and semantic-layer scope subcommands + permission gating.
tests/test_metastore_client.py Metastore client tests updated for schemaVersion 1.1.0 and new scope/grant endpoints.
src/keboola_agent_cli/services/semantic_layer_service.py Service layer wiring for scope/grants/elevation + passing scope/grants on creates.
src/keboola_agent_cli/services/_semantic_layer_scope.py New helper module: alias→project_id resolution and scope/grant/elevation orchestration logic.
src/keboola_agent_cli/services/_semantic_layer_crud.py Preserve scope/grants across DELETE+POST edits (including rollback).
src/keboola_agent_cli/permissions.py Register permission categories for semantic-layer.scope.* operations.
src/keboola_agent_cli/metastore_client.py Add scope/grant/elevation primitives; bump create envelope schemaVersion to 1.1.0.
src/keboola_agent_cli/commands/semantic_layer.py Wire scope sub-app and add --scope/--target-project to model create.
src/keboola_agent_cli/commands/context.py Update generated context docs for new scope flags and scope subcommands.
src/keboola_agent_cli/commands/_semantic_layer_scope.py New Typer sub-app implementing semantic-layer scope ... CLI surface.
src/keboola_agent_cli/commands/_semantic_layer_helpers.py Add resolve_scope_targets helper with interactive picker / non-interactive fail-fast behavior.
src/keboola_agent_cli/commands/_semantic_layer_crud.py Add --scope/--target-project plumbing to semantic-layer add <kind>.
plugins/kbagent/skills/kbagent/SKILL.md Add command table entries for new semantic-layer scope commands (and sl alias).
plugins/kbagent/skills/kbagent/references/semantic-layer-workflow.md Cross-link to the new scope workflow guidance.
plugins/kbagent/skills/kbagent/references/metastore-scope-workflow.md New workflow doc for sharing/elevation with strong “ask user first” safety rules.
plugins/kbagent/skills/kbagent/references/gotchas.md Add PSGO-140 gotchas (schemaVersion 1.1.0 requirement, replace semantics, 403 vs 404, etc.).
plugins/kbagent/skills/kbagent/references/commands-reference.md Document new scope flags and semantic-layer scope subcommands.
plugins/kbagent/agents/keboola-expert.md Add explicit rule preventing agents from widening scope without user-specified targets.
CLAUDE.md Update command inventory and add scope feature notes + new commands.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/keboola_agent_cli/commands/_semantic_layer_scope.py Outdated
Comment thread src/keboola_agent_cli/metastore_client.py Outdated
Comment thread src/keboola_agent_cli/commands/_semantic_layer_scope.py
@Matovidlo
Matovidlo force-pushed the martinvasko-ai-3790-verify-that-the-new-semantic-models-and-datasets branch from e5c7c64 to 8c8fd94 Compare August 31, 2026 12:57
@Matovidlo
Matovidlo requested a review from padak September 18, 2026 12:59
@Matovidlo

Copy link
Copy Markdown
Contributor Author

@soustruh or @padak can you check this ? I will rebase it after but it sits here for a while and everything is done except of CLI part

@soustruh

soustruh commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Command-line interface: align the new commands with the planned spec

#791 (CLI-20) plans one specification for the kbagent command-line interface and a CI check (make cli-spec-check). The spec is not merged yet. The rules below come from the current draft of that spec. I suggest that we align the new commands with it now, for two reasons:

  • The scope commands are not released yet, so a rename costs nothing now.
  • After a release, the change policy in the draft spec requires a deprecation for each rename. The old form warns for at least 30 days and 3 releases. Then it fails with a hint for at least 30 more days.

Commands

The draft spec uses a fixed set of verbs: get/set read or write one attribute, add/remove attach or detach a relationship, create/delete create or destroy a server object, and list lists. A compound name puts the noun before the verb, as in metadata-get. The group name scope already gives the context, so the elevation request is only request.

Now Proposed Reason
scope status scope get Reads one attribute
scope grant --target-project X scope add --target-project X Attaches a target project
scope grant --remove-target-project X scope remove --target-project X Detaches a target project. The --remove-target-project option is not necessary
scope grant --replace / --clear scope set --target-project X / scope set --clear Writes the whole target-project list
scope elevate scope set --scope organization Writes the scope. If the metastore later supports more scope changes, such as a downgrade, the command accepts more values and no new verb is necessary
scope request-elevation scope request-create Creates the elevation request
scope withdraw-elevation scope request-delete Deletes the elevation request
scope pending scope request-list Lists the elevation requests

scope set accepts --scope organization or --target-project/--clear, not both. A combination exits 2. The metastore changes the scope only to organization, and it accepts a target-project list only for a targeted item. One command thus covers both server calls and refuses an invalid combination before it sends a request. With separate add, remove and set commands, the invalid flag combinations of scope grant do not exist. Today scope grant --replace --remove-target-project X silently ignores --remove-target-project.

Options

  • --id becomes --context-id. For a new command, the draft spec requires the form --<noun>-id, with one name for one identifier. semantic-layer get-context --context-id already uses this name for the UUID of any semantic entity.
  • --target-project accepts an alias or a project ID, as --project does since CLI-22 (v0.96.1). Then a target project does not have to be registered in kbagent. The option is repeatable and also accepts a comma list, because the draft spec requires this for a list option that takes IDs.
  • --scope and --type use click.Choice, because the draft spec requires this for a fixed set of values. A bad value then exits 2.

Permissions

  • scope get and scope request-list are read.
  • scope add, scope remove, scope set, scope request-create and scope request-delete are write.
  • scope set --scope organization is destructive through FLAG_ESCALATIONS, as the draft spec describes for a flag that raises the class of its command. The same escalation applies to model create and to every add <kind> with --scope organization, because these commands give the same org-wide visibility. Today --deny-destructive blocks scope elevate, but it does not block add dataset --scope organization.

Behavior fixes

Rule in the draft spec Now in this PR Fix
A bad input value is a usage error (exit 2) that names the option, never a traceback An unknown --target-project alias on model create and on the five add commands gives a traceback and exit 1 Resolve the targets inside _handle_service_call and return INVALID_ARGUMENT with exit 2
Same rule, and one meaning per option --target-project without --scope targeted is silently ignored, and the command exits 0 Exit 2 with INVALID_ARGUMENT
A destructive command has --dry-run and --yes scope elevate has --yes only scope set --scope organization has --dry-run and --yes
A limited list reports limit, offset and has_more scope pending returns a bare list scope request-list returns the three keys
Every command has a kbagent serve route, or a skip with a reason. CONTRIBUTING.md requires this already today No routes for the six scope commands, and no scope field on POST /semantic-layer/models and POST /semantic-layer/items/{kind} Add the routes, or write the skip reason in the PR description

Examples

# Create with a visibility scope
kbagent semantic-layer model create --project dev --name sales --scope targeted --target-project prod,5678
kbagent semantic-layer add dataset --project dev --model sales --name orders --table-id out.c-sales.orders --scope organization

# Show the scope of one item
kbagent semantic-layer scope get --project dev --type dataset --context-id 7f3c...

# Target projects: add, remove, replace the list, clear the list
kbagent semantic-layer scope add --project dev --type dataset --context-id 7f3c... --target-project prod --target-project 5678
kbagent semantic-layer scope remove --project dev --type dataset --context-id 7f3c... --target-project 5678
kbagent semantic-layer scope set --project dev --type dataset --context-id 7f3c... --target-project prod,5678
kbagent semantic-layer scope set --project dev --type dataset --context-id 7f3c... --clear

# Elevation request (project admin): create, delete
kbagent semantic-layer scope request-create --project dev --type dataset --context-id 7f3c...
kbagent semantic-layer scope request-delete --project dev --type dataset --context-id 7f3c...

# Org admin: list the requests, preview the elevation, elevate
kbagent semantic-layer scope request-list --project org-admin --type dataset --limit 50
kbagent semantic-layer scope set --project org-admin --type dataset --context-id 7f3c... --scope organization --dry-run
kbagent semantic-layer scope set --project org-admin --type dataset --context-id 7f3c... --scope organization --yes

For #791, not for this PR

  • --scope and --target-project already exist with other meanings: search --scope selects a part of the configuration body, and config clone --target-project names the destination project. The draft spec requires one meaning per long option name in the whole tree. The spec will decide this. Until then, the two options go to the baseline.
  • The draft spec defines destructive as "deletes, drops or invalidates". An irreversible widening of visibility is not in that definition. The spec must list scope set --scope organization as a declared case, or define a class for it.

@soustruh

soustruh commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Review: correctness and rebase

The new metastore calls match the go-monorepo source: verbs, paths, bodies, the 204 on PUT .../target-projects, and the org-admin requirement of PATCH and GET /{type}/organization. Every semantic-* type has a 1.1.0 schema there. The full test suite passes on the PR head, and ruff and ty are clean. make check fails only at changelog-check, because the branch starts before the 0.93.0–0.96.1 releases. Nobody ran the new commands against a live stack, so the findings that depend on server behavior come from the source code only.

The traceback for an unknown --target-project alias is the one blocking item. It is in the command-line interface comment above, with the other interface fixes.

Findings

  1. import --overwrite and promote reset the scope to project. Their overwrite branches (services/_semantic_layer_internals.py:445 and :518) do DELETE+POST and call post_item without scope/target_project_ids. This PR fixes the same reset in edit only. Example: a target dataset has scope=organization. semantic-layer promote finds a changed attribute, deletes the dataset and creates it again as project-only. The consumer projects lose access, and the command reports no error. compare_attrs compares attributes only, so the diff does not show the scope change either.
  2. The edit rollback can lose the item. delete_then_post (services/_semantic_layer_crud.py:104) sends the original scope on the primary POST and on the rollback POST. If the POST fails because of the scope (for example a 403, when the caller is not an org admin), the rollback fails in the same way after the DELETE. The item is then gone.
  3. An org admin outside the owner project can remove all grants. The server returns meta.targetProjectIds only when the caller's project owns the item (attachTargetProjectIDs(..., &callerProjectID)). PUT .../target-projects is allowed for the owner and for an org admin. If an org admin adds a target project from another project, kbagent reads an empty list and sends only the new project. The server then replaces the whole list. edit from a non-owner project has the same problem. Suggestion: in merge mode, refuse when meta.projectId is not the caller's project, or treat a missing targetProjectIds on a targeted item as unknown, not as empty.
  4. edit removes a pending elevation request and changes the item ID. DELETE+POST does not keep meta.scopeElevationRequestedAt, so the request disappears from the org admin's list. MetastoreClient.put_item already exists, and its docstring says that a PUT does not change the scope or the grants. If edit uses put_item, findings 2 and 4 and the scope copy in edit are not necessary.
  5. Elevation of items created before this PR can fail. The server checks PATCH {"scope":"organization"} and PUT .../scope-elevation-request against the stored schemaVersion of the item (scopeSupported(...) in meta_object_repository.go). Items that earlier kbagent versions created have 1.0.0, which supports only project. The docs promise elevation of existing items with no condition. Please check this on a live stack. If it is true, document it, or make the error message say how to continue.
  6. Target aliases can resolve to a project on a different stack. resolve_target_project_ids (services/_semantic_layer_scope.py:37) does not check that the target project is on the same stack as the owner project, and the interactive picker shows projects from all stacks. A project ID from a different stack then means a different project, or no project, on the owner's stack.
  7. Child items do not get the scope of their model. Every add <kind> uses project unless --scope is given. The UI release note says that sub-objects of an org-level object are also created at org level. With kbagent, consumer projects see an org-level model with no datasets or metrics.
  8. schemaVersion 1.1.0 on every write. The bump also applies to project-scope writes. If a stack does not have the 1.1.0 schemas yet, every model create, add, edit, import and build fails there. Please confirm that all stacks have the schemas before the release.

Rebase and docs

  • The PR conflicts with main in 7 files: CLAUDE.md, gotchas.md, commands/_semantic_layer_crud.py, commands/context.py, metastore_client.py, services/semantic_layer_service.py and tests/test_metastore_client.py. In metastore_client.py, keep the session-auth changes from main (http_auth, BEARER_REFRESH_ON_401 = False and the session 401 message) and add the PSGO-282 wording. After PSGO-282, the comment on BEARER_REFRESH_ON_401 ("a valid non-master token gets 401") is out of date.
  • _serve_command_map.py exists only on main. After the rebase, new routes need entries there (test_command_map_matches_every_route_exactly) and a new docs/web-server-endpoints.md (make endpoints-gen).
  • The PR description does not mention the PSGO-282 changes (the master-token text in 6 files).
  • metastore-scope-workflow.md has no row in the workflow table of SKILL.md.
  • metastore-scope-workflow.md:9 says "Any normal project token can create at this scope". A targeted create needs a project-admin token. The end of Workflow 3 says that "a non-admin token" can call scope elevate if it "holds the organization-admin role". That should be "an org-admin token".
  • A live run would close most of the open questions: create targeted, add a target project, create a request, elevate, edit, list the requests. Please include one item that an earlier kbagent version created.

Matovidlo and others added 4 commits October 2, 2026 12:15
…semantic layer

Adds kbagent CLI support for PSGO-140's metastore scope model: `--scope
project|organization|targeted` and `--target-project` on `model create` /
`add <kind>`, plus a new `semantic-layer scope` command group
(status/grant/request-elevation/withdraw-elevation/elevate/pending) for
managing target-project grants and organization-wide elevation on existing
items.

Bumps the metastore envelope schema version from 1.0.0 to 1.1.0 -- every
semantic-* schema only supports scope="project" at 1.0.0; 1.1.0 is what adds
organization/targeted support (purely additive ACL block, verified against
go-monorepo). Without this the whole feature would 400 server-side.

The DELETE+POST `edit` path now reads and re-applies an item's original
scope/target-project grants, so editing an organization/targeted-scope item
no longer silently resets it to project scope.

Widening visibility always requires an explicit --target-project (or an
interactive picker on a real terminal; hard fail in --json/non-interactive)
-- never a silent default. keboola-expert.md gets an explicit rule to always
ask the user before passing --scope organization|targeted.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…oint response

`scope request-elevation` / `withdraw-elevation` / `elevate` rendered the
mutating endpoint's own response body. That body omits
`meta.targetProjectIds`, so the commands printed `target_project_ids: null`
for an item whose grants were fully intact.

Verified live against metastore.us-east4 (project keboola-ai, model with
scope=targeted, targets=[5024]):

    scope status         -> targets=[5024]
    request-elevation    -> targets=None      <-- wrong, grants untouched
    scope status         -> targets=[5024]

An operator reading that output would reasonably conclude that requesting
elevation had just wiped every grant on the object.

`grant_target_projects` already re-read the item after its PUT for exactly
this reason; the three elevation helpers now do the same. Re-verified live:
command output matches `scope status` at every step.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…en for every call (PSGO-282)

go-monorepo#596 (PSGO-282) fixed the metastore to accept any valid,
non-disabled, non-expired Storage token for reads -- writes still need a
project-admin token. Every doc this repo carries about the old behavior
(added in #711/#717) still claimed the *whole* semantic-layer family
needed a master token, which is now false and would make agents refuse a
plain read or hunt for a master token they don't need.

Updates CLAUDE.md, keboola-expert.md, commands-reference.md, gotchas.md,
semantic-layer-workflow.md and docs/error-codes.md to state the real
split (reads: any valid token: writes: project-admin token), and softens
metastore_client.py's 401-reclassification message so it no longer
overclaims a blanket master-token requirement -- kept as a safety net for
a deployment that predates the fix. New gate entries use the vNEXT
placeholder per this repo's release convention (check_version_gates.py).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…CLI redesign, serve routes)

Correctness (review by Martin Struzsky):
- edit, import --overwrite and promote update in place with put_item
  instead of DELETE+POST: the item keeps its id, scope, grants and pending
  elevation request, and a failed update changes nothing. Removes
  delete_then_post.
- scope add/remove refuse from a project that does not own the item (the
  server hides the grants from a non-owner, so a merge would wipe them).
- --target-project accepts an alias or numeric project ID (repeatable or
  comma-separated); an alias must be on the owner's stack.
- add <kind> without --scope inherits the model's scope and targets.
- schemaVersion 1.1.0 is sent only for non-project scopes.
- post_item rejects target_project_ids=[] without targeted scope.

CLI spec alignment (#791):
- scope verbs: get/add/remove/set/request-create/request-delete/request-list,
  --id -> --context-id, scope set takes exactly one of --scope organization,
  --target-project or --clear, with --dry-run/--yes.
- --scope/--type are fixed choices; bad values, unknown aliases and
  --target-project without --scope targeted exit 2.
- --scope organization is destructive-class via FLAG_ESCALATIONS.
- request-list returns limit/offset/has_more.

Serve: 7 scope routes plus scope/target_projects on POST /models and
/items/{kind}, with command-map entries and regenerated endpoint docs.

Docs: CLAUDE.md, context.py, commands-reference, gotchas, scope workflow,
expert prompt, SKILL.md (regenerated + workflow row); PSGO-282 wording.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@Matovidlo
Matovidlo force-pushed the martinvasko-ai-3790-verify-that-the-new-semantic-models-and-datasets branch from 8c8fd94 to ffb8f04 Compare October 2, 2026 10:46

Copy link
Copy Markdown
Contributor Author

Thanks, all adopted in ffb8f04.

  • Commands: scope get|add|remove|set|request-create|request-delete|request-list. scope set takes exactly one of --scope organization, --target-project or --clear, and has --dry-run/--yes. Any other combination exits 2.
  • Options: --id is now --context-id. --scope and --type are fixed choices. --target-project takes an alias or a numeric project ID, repeated or comma-separated. I resolve it in the service instead of adding it to ALIAS_OPTIONS, because a target need not be registered; it is listed in NOT_AN_ALIAS in test_project_ref.py with that reason.
  • Exit 2 cases: an unknown alias, a bad --scope/--type, and --target-project without --scope targeted all exit 2 now.
  • Permissions: --scope organization is destructive-class through FLAG_ESCALATIONS on scope set, model create and every add <kind>. --dry-run isn't blocked.
  • request-list: returns limit, offset and has_more.
  • Serve routes: 7 scope routes, plus scope and target_projects on POST /models and /items/{kind}, with command-map entries and a regenerated endpoint reference.
  • Left for Specify the complete kbagent command-line interface #791: the shared option names (--scope/--target-project) and whether an irreversible elevation counts as "destructive".

Matovidlo commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor Author

Rebased on main (7 conflicts) and addressed in ffb8f04. In metastore_client.py I kept the session-auth 401 handling and the PSGO-282 wording, and updated the stale BEARER_REFRESH_ON_401 comment.

  • Findings 1, 2 and 4 (import --overwrite / promote reset the scope; edit rollback can lose the item; edit drops the elevation request and changes the ID): edit, import --overwrite and promote now use put_item (PUT in place). The item keeps its ID, scope, grants and pending request, and a failed PUT changes nothing. delete_then_post and the scope copy in edit are gone.
  • Finding 3 (org admin outside the owner project wipes grants): scope add|remove refuse (exit 2) when the caller's project doesn't own the item or the grants are unreadable, and point to scope set --target-project.
  • Finding 5 (elevating older items): not verified, because nothing ran on a live stack yet. The gotchas and the workflow doc flag it.
  • Finding 6 (target on another stack): an alias on a different stack is rejected, and the picker only lists projects on the owner's stack. A bare ID can't be checked and is trusted.
  • Finding 7 (children don't get the model's scope): add <kind> without --scope now inherits the model's scope and target projects. model create still defaults to project.
  • Finding 8 (schemaVersion 1.1.0 on every write): it is sent only for non-project scopes, or when editing a non-project item. Project-scope writes keep 1.0.0.

Docs: added the SKILL.md row for metastore-scope-workflow.md, fixed the two wording errors you listed (targeted create needs a project-admin token; scope set --scope organization needs an org-admin token), replaced the stale "master token" and "DELETE+POST" text, and the PR description now covers PSGO-282.

Full suite passes (7346 passed, 189 skipped); ruff and ty are clean.

Still open until the live run, which I'll do once the PR is green: whether a rename through PUT works, and finding 5.

@keboola-pr-reviewer-bot keboola-pr-reviewer-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verdict: needs_human (risk 4/5) · profile keboola-mcp-server

Needs human: large semantic-layer ACL feature that also changes existing edit/import/promote to in-place PUT, on a contract the author says is unverified against a live stack.

Impact flags: possible rollback re-introduction — see Check Run summary.

Concerns:

  • src/keboola_agent_cli/services/_semantic_layer_crud.py: edit/import/promote switched from DELETE+POST-with-rollback to in-place PUT on existing commands
  • src/keboola_agent_cli/metastore_client.py: new org-wide/cross-project ACL visibility elevation; one-way, no downgrade endpoint
  • PR description: feature not verified against a live stack with PSGO-140; grounded only in go-monorepo source

@soustruh

soustruh commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Re-review of ffb8f04

Thanks. I checked the fixes against the code, not only against the reply. These are fixed: the in-place PUT for edit, import --overwrite and promote, the non-owner guard on scope add|remove, the cross-stack alias check and the picker filter, the scope inheritance of add <kind>, and every item of the command-line interface comment. make check passes on the head.

There are two new blocking items. The first one comes from my own finding 8.

Blocking

  1. New items without --scope cannot be elevated. Finding 8 of my earlier comment was not verified, and the fix for it causes this problem.

    • Project-scope creates now send schemaVersion 1.0.0, and Create stores the version that the client sends.
    • PromoteToOrganizationScope and setScopeElevationRequest check scopeSupported(...) against the stored version of the item (meta_object_repository.go, about lines 695–699 and 844–848). At 1.0.0, only project is supported.
    • Put keeps the stored version (currentObject.SchemaVersion), so edit does not change it.
    • Result: scope request-create and scope set --scope organization fail with ErrScopeNotSupported for every new item that was created without --scope. This continues until the schemasweep job moves the item to the default version.
    • The risk in finding 8 was probably not real. Migration 20260818101500_add_remaining_semantic_layer_schemas_1_1_0 makes 1.1.0 the default schema, and 20261002120000_add_semantic_layer_schemas_1_2_0 makes 1.2.0 the default for the child types. I did not check which stacks have these migrations.

    Fix: do not send schemaVersion on create. For an empty version, getByObjectType uses the default schema of the stack (version = $2 OR ($2 = '' AND is_default = true)). The create handler (fetchPolicy) and Create both resolve the schema through this function, and CreateRequest.SchemaVersion has no required tag. Create then stores the resolved default version. Then _schema_version() is not necessary. With 1.2.0, a create with a modelUUID that does not exist returns 422. I read only the source, so please check this on a live stack.

  2. An inherited organization scope bypasses --deny-destructive. add <kind> without --scope takes the scope of the model in the service layer. The FLAG_ESCALATIONS check runs only when the user types --scope organization (commands/_semantic_layer_helpers.py:99), and the service takes the scope of the model later (services/semantic_layer_service.py:702). Example: kbagent --deny-destructive semantic-layer add dataset --project prod --model orgmodel ... creates an org-wide dataset and exits 0. The same command with --scope organization exits 6. Fix: when the model scope is not project, require an explicit --scope, and refuse with INVALID_ARGUMENT that names the scope of the model. This also covers non-blocking item 2.

Non-blocking

  1. scope add|remove refuses the owner of a targeted item with no target projects. In _merge_targets (services/_semantic_layer_scope.py:126), the second condition treats "no target projects" as "cannot read". The server omits targetProjectIds when the list is empty (attachTargetProjectIDs sets a nil slice, and JSONAPIMeta omits nil). So after scope set --clear, the owner gets "This project does not own the item". The first condition already catches a non-owner, so the second one can go. When project_id is missing in the config, the message should say that the own project ID is unknown and name kbagent project refresh.
  2. A project admin who adds a child to an org-level model now gets a bare 403. The x-metastore.acl.create rule allows an organization-scope create only for an org admin. Before this change, the child was created at project scope. The fix of blocking item 2 covers this. Without that fix, the error needs a hint: pass --scope project, or use an org-admin token.
  3. put_item sends a schemaVersion that the server ignores. MetaObjectUpdatePutRequest has only name and data. So the scope parameter of put_item and its docstring describe no behavior. A rename to a name that exists returns 409 "Object with this name already exists in this project" (or "... in organization scope"). post_item maps this to ALREADY_EXISTS, but put_item does not.
  4. No E2E test for the scope commands or for the PUT edit path. The live run that you plan should include an item created without --scope, because that is the case of blocking item 1.

Nits

  • scope set --scope organization --dry-run skips the escalation check. sync push --force checks before its dry-run branch (commands/sync.py:1048). Either way works, but the two should match.
  • Some texts still say "DELETE+POST": the docstrings at services/semantic_layer_service.py:1210, 1246, 1276, 1326, 1365, 1500, services/_semantic_layer_internals.py:481 and metastore_client.py:374, and comments in tests/test_e2e.py (4482, 4493, 11600, 11617, 11639) and tests/test_server_semantic_layer_routes_e2e.py (345, 353). These E2E tests refresh their ID tracking because they expect a new ID after an edit, so check them in the live run.
  • edit always returns "rollback": None now (services/_semantic_layer_crud.py:161, 298).

…d org scope)

Blocking:
- Create no longer sends schemaVersion. The metastore stores the version it
  resolves and checks elevation against the STORED version; pinning 1.0.0 on
  a plain create made every item created without --scope impossible to
  elevate. Removes _schema_version() and the version constants.
- An inherited organization scope (add <kind> without --scope under an
  org-level model) is now permission-gated like a typed --scope organization,
  so --deny-destructive no longer lets it through. A non-org-admin 403 on an
  inherited scope gets a hint to pass --scope project.

Non-blocking / nits:
- scope add|remove: ownership is decided by meta.projectId only, so the owner
  of a targeted item with no grants can add again; an unknown own project ID
  says to run `kbagent project refresh`.
- put_item no longer sends schemaVersion or takes a scope argument; a rename
  to a taken name maps to ALREADY_EXISTS.
- scope set --scope organization --dry-run is gated too (same as
  sync push --force).
- E2E: scope get/request-create/request-delete/dry-run on an item created
  without --scope, and an edit that keeps the item id.
- Remove stale DELETE+POST wording in docstrings, comments and docs.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Copy link
Copy Markdown
Contributor Author

Thanks for checking the code. Addressed in 1052004.

Blocking

  • Item 1, new items cannot be elevated: create no longer sends schemaVersion, so the server stores the stack's default schema. I removed _schema_version() and the version constants and rewrote the comment in metastore_client.py and the gotchas. This reverses my fix for your finding 8, which caused the problem. I took the empty-version behaviour from your reading of getByObjectType; it is untested on a live stack.
  • Item 2, inherited organization bypasses --deny-destructive: I kept the inherit default and gated it instead of requiring an explicit --scope. add <kind> without --scope asks for the model's scope (new child_scope()), and an inherited organization goes through the same FLAG_ESCALATIONS check as a typed --scope organization. The lookup only runs when a permission policy is active. A project admin who isn't an org admin and gets the 403 now sees a hint to pass --scope project or use an org-admin token. I covered it with a CLI test for organization, targeted and project models. If you prefer the explicit---scope rule, say so.

Non-blocking

  • 1: _merge_targets decides ownership by meta.projectId only, so the owner of a targeted item with no grants can add again. If the caller's own project ID is unknown, the message names kbagent project refresh.
  • 2: covered by the hint above.
  • 3: put_item no longer sends schemaVersion and no longer takes a scope argument. A 409 on a rename now maps to ALREADY_EXISTS.
  • 4: I added an E2E block to TestE2ESemanticLayerLifecycle: scope get, request-create, request-delete and set --scope organization --dry-run on an item created without --scope, and an edit that keeps the item ID. I haven't run it, because it needs a live stack. The live run should also cover a rename through PUT.

Nits

  • scope set --scope organization --dry-run now checks the escalation first, like sync push --force. I updated the docs and tests that said otherwise.
  • I replaced the "DELETE+POST" text in the service and internals docstrings, the E2E comments and the docs. The E2E ID-tracking code still works when IDs don't change.
  • I left "rollback": None in the edit result so consumers of that key keep working.

Full suite passes (7357 passed, 189 skipped, excluding tests/test_build_hook.py, which also fails without these changes in my environment). ruff, ty and the sync gates are clean.

@soustruh

soustruh commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Re-review of 1052004

Thanks. Both blocking items are fixed. I checked them against the code:

  • Create sends no schemaVersion. The metastore resolves an empty version to the default schema since versioned schema retrieval was added (June 2025), so this also works on older deployments. The current defaults support organization: 1.2.0 for the child types and 1.1.0 for semantic-model, which has no 1.2.0.
  • An inherited organization scope now goes through FLAG_ESCALATIONS. Under --deny-destructive, add dataset without --scope on an org-level model exits 6.
  • put_item, _merge_targets and the dry-run check of scope set behave as described in your reply.

The full test suite passes on the head, and ruff, ty and the sync gates pass.

One small item: services/semantic_layer_service.py:721 adds the org-admin hint for every inherited scope that is not project, so also for targeted. The ACL of the 1.1.0 and 1.2.0 schemas allows a project admin to create a targeted item, so a 403 there has a different cause, and the hint gives wrong advice. Please change the condition to scope == "organization".

The live run is still open: the new E2E block, a rename through PUT, and an elevation of an item created without --scope.

A project admin may create a targeted item, so a 403 on an inherited targeted
scope has another cause and the "needs an org-admin token" hint was wrong
advice. Only add it when the inherited scope is organization.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Copy link
Copy Markdown
Contributor Author

Thanks for checking 1052004 against the code. Fixed the hint in 3724194: it is now only added when the inherited scope is organization, so an inherited targeted scope that gets a 403 keeps the original message. I added a test for the targeted case next to the organization one.

The live run is still open, as you listed: the new E2E block, a rename through PUT, and an elevation of an item created without --scope. I'll run it once I have a stack with PSGO-140 deployed and will post the results here.

@soustruh soustruh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for all the changes! 🙏

@Matovidlo
Matovidlo merged commit bea2b49 into main Oct 5, 2026
4 checks passed
@Matovidlo
Matovidlo deleted the martinvasko-ai-3790-verify-that-the-new-semantic-models-and-datasets branch October 5, 2026 04:25
Matovidlo added a commit that referenced this pull request Oct 7, 2026
…ect ids in REST

Replaces the merge commit a121636 that the rebase onto main flattened:
the rls/cls services use the scope client #715 shipped on main, and the
REST bodies accept integer project ids (Copilot review).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Matovidlo added a commit that referenced this pull request Oct 7, 2026
…s, destructive deletes

Address the review of #778:
- default to targeted scope (the schema default); --scope organization
  is explicit and destructive-class (FLAG_ESCALATIONS, also over REST)
- --dialect defaults to and must match the project backend; duplicate
  principals are refused across the visible policies on the table
- --target-project takes alias or ID (#715 resolver); grants go in the
  create request only; on update they change before the rules
- update sends a PATCH of the changed keys and re-reads the result
- rls/cls delete are destructive and have --dry-run
- own 409 message, owner_project_id in list/detail
- command shape per #791: --table-id, --dialect choice, REST table_id /
  target_projects and PATCH; one router builder for rls and cls
- rls setup --json returns a JSON error envelope; typed wizard values

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants