Skip to content

feat(CLI-17): add kbagent rls command group + guided setup skill - #778

Open
Matovidlo wants to merge 28 commits into
mainfrom
martinvasko-cli-17-kbagent-rls-command-group-guided-setup-skill-for-metastore
Open

Matovidlo wants to merge 28 commits into
mainfrom
martinvasko-cli-17-kbagent-rls-command-group-guided-setup-skill-for-metastore

Conversation

@Matovidlo

@Matovidlo Matovidlo commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Implements the kbagent side of RLS v2 (row-level security), companion to keboola-mcp-server PR #709, which redesigns RLS around metastore-backed rls-policy / cls-policy objects (declarative condition primitives instead of hand-written SQL). kbagent only authors the policies; enforcement is query_data in keboola-mcp-server.

  • New rls command group: list, detail, schema, create, update, delete, and a guided setup wizard (checkbox table picker + interactive condition builder, interactive-terminal-only).
  • New cls command group (column-level security): list, detail, schema, create, update, delete. Each rule is {principal|principals, visible_columns} -- an allowlist projection. ClsService subclasses RlsService; no setup wizard.
  • Scope: targeted by default (the metastore schema's own default: the owning project plus --target-project grants). --scope organization governs the table in every project of the organization; it is explicit and destructive-class (rls|cls.create --scope organization in FLAG_ESCALATIONS, also checked over REST). There is no project scope (the schema does not support it). --target-project takes an alias or a project ID (the feat(semantic-layer): metastore scope/ACL support (targeted + org-wide) [AI-3790] #715 resolver).
  • Who may write (metastore schema ACL): a project admin may create/update/delete targeted policies of its own project without grants; grants and organization scope need the organization-admin role. Policies are enforced only where the project has the row-level-security feature.
  • Policies that would block every query are refused before any write. The enforcement refuses reads when a policy's dialect differs from the project backend (--dialect defaults to the backend), and kbagent refuses that mismatch locally. A null comparison (renders col = NULL, matches nothing) and principals with whitespace are refused too.
  • Schema 1.1.0 (go-monorepo keboola/go-monorepo#617, AI-4010; the metastore default once it ships): rules select identities by principal, principals or IdP groups; every rule matching one identity applies (RLS conditions OR, CLS columns union), so the old duplicate-principal refusal is gone; rls --default (CLI and REST) sets the condition for an identified reader no rule matches, and {"false": true} means no rows; value {"$identity": "email"} / values {"$identity": "groups"} placeholders are accepted and previewed. A default cannot be removed by update (recreate the policy).
  • update is a partial PATCH (new MetastoreClient.patch_item) of only the changed keys. The policy is read first and the MERGED result is validated, because the metastore PATCH validates only the keys it receives; grants change before the rules; the result is re-read after the write. The scope support in metastore_client.py (post_item scope, put_target_projects) comes from feat(semantic-layer): metastore scope/ACL support (targeted + org-wide) [AI-3790] #715.
  • --dry-run preview follows the enforcement's rendering (columns quoted per dialect, TRUE/FALSE), but is not the enforcement.
  • Permissions: create/update/setup are admin; delete is destructive (and has --dry-run).
  • REST: /rls and /cls routers built from one builder, every route permission-gated: POST /{group}/{project} (table_id, rules, dialect?, scope, target_projects), PATCH /{group}/{project}/{policy_id}, DELETE ...?dry_run=true.
  • Command shape follows the CLI spec draft (Specify the complete kbagent command-line interface #791): --table-id, --dialect choice, --policy-id, --target-project alias or ID.
  • Full CONTRIBUTING.md doc tax: CLAUDE.md, context.py AGENT_CONTEXT, commands-reference.md, gotchas.md, keboola-expert.md, SKILL.md (regenerated), new rls-workflow.md, docs/error-codes.md, docs/web-server-endpoints.md (regenerated).

Backend availability: the rls-policy and cls-policy object types are registered by the metastore's 2026-09-29 schema migrations (go-monorepo, released as metastore-v0.11.0). A stack whose metastore predates them answers rls schema / cls schema with a classified NOT_FOUND (see gotchas.md).

Known enforcement-side hazard (for #709): an organization policy is loaded by every project in the organization, whatever its backend, so in a mixed Snowflake/BigQuery organization it blocks every project on the other backend. kbagent can only check the owner's backend; the docs recommend targeted there.

No version bump, no changelog.py entry -- per this repo's binding convention, feature PRs never bump the version; (since vNEXT) placeholders are resolved by the release PR.

Test Plan

  • tests/test_rls_service.py, tests/test_cls_service.py (scope default and combinations, backend dialect, schema 1.1.0 (groups, OR-combined rules, default, $identity), PATCH of changed keys, grants-before-rules, re-read, 409 message, delete dry-run, preview rendering per dialect).
  • tests/test_rls_cli.py, tests/test_cls_cli.py (flags, --scope organization and delete blocked by --deny-destructive, setup --json JSON error envelope).
  • tests/test_server_rls.py, tests/test_server_cls.py (PATCH route, target_projects + organization = 422, permission classes per route).
  • tests/test_metastore_client.py, tests/test_semantic_layer_scope.py (target project 0 rejected).
  • tests/test_e2e.py: authors a targeted policy with no grants and the project's own dialect, updates its rules, delete --dry-run, deletes it (not run locally: needs E2E_API_TOKEN).
  • Full suite after the rebase onto 0.97.0: 7686 passed, 66 skipped. ruff check, ruff format --check, ty check, loc-check, check-sentinel-guards, check_command_sync.py, check_version_gates.py, check_error_codes.py, skill-frontmatter length and the SKILL.md / web-server-endpoints.md generators all pass.

Related Issues

Linear: CLI-17

Companion PR: keboola/mcp-server#709 (RFC + PLAN + the enforcement engine). Follow-up: #833 (semantic-layer edit → PATCH).

🤖 Generated with Claude Code

@Matovidlo
Matovidlo requested a lite review from Copilot September 17, 2026 12:52
@linear-code

linear-code Bot commented Sep 17, 2026

Copy link
Copy Markdown

CLI-17

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.

🟡 Changes recommended

Local validation/preview currently treats any {"true": ...} as always-true (even when the value is falsey), which can silently accept and display an invalid condition shape when the live schema is unavailable.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds first-class Row-Level Security (RLS v2) support to kbagent by introducing a new rls command group and a REST router backed by a metastore rls-policy object type, including validation + preview tooling and the required permissions/doc/test integration across the CLI, server, and plugin skill surfaces.

Changes:

  • Implemented kbagent rls {list,detail,schema,create,update,delete,setup} with RlsService orchestration and local condition/rules validation + preview rendering.
  • Extended metastore client write envelopes to support organization / targeted scopes and added a target-projects grant management call for targeted RLS policies.
  • Added /rls server router with permission gating, updated operation registry classifications, and refreshed docs/plugin skill references + test coverage.
File summaries
File Description
tests/test_server_rls.py Verifies /rls router → service kwarg parity and permission gating behavior.
tests/test_rls_service.py Unit-tests RlsService CRUD orchestration, scope handling, validation, and preview rendering.
tests/test_rls_cli.py CLI-level tests for kbagent rls JSON/human output and exit-code/error mapping (incl. non-TTY setup refusal).
tests/test_metastore_client.py Adds coverage for new metastore scope envelope fields and put_target_projects().
tests/test_e2e.py Adds an E2E “clean failure / skip” check for rls schema and rls list while backend type is unregistered.
src/keboola_agent_cli/services/rls_service.py New service composing metastore primitives into rls-policy CRUD + preview/dry-run behavior.
src/keboola_agent_cli/services/_rls_condition.py New pure helpers for local rule validation, op validation, Draft7 schema validation, and condition preview rendering.
src/keboola_agent_cli/server/routers/rls.py New FastAPI router exposing REST equivalents for rls commands (except interactive setup).
src/keboola_agent_cli/server/dependencies.py Wires RlsService into ServiceRegistry.
src/keboola_agent_cli/server/app.py Registers the rls router and documents it in the server router list.
src/keboola_agent_cli/server/_serve_command_map.py Maps /rls endpoints back to CLI command names.
src/keboola_agent_cli/permissions.py Registers rls.* operations and classifies writes as admin.
src/keboola_agent_cli/metastore_client.py Adds scoped write support (scope, targetProjectIds) and put_target_projects() helper.
src/keboola_agent_cli/errors.py Introduces ErrorCode.INVALID_RLS_POLICY.
src/keboola_agent_cli/commands/rls.py New Typer command group implementing the rls CLI UX, including guided setup.
src/keboola_agent_cli/commands/context.py Updates AGENT_CONTEXT with RLS command help/recipes.
src/keboola_agent_cli/cli.py Registers rls Typer app and constructs RlsService in the CLI context.
plugins/kbagent/skills/kbagent/SKILL.md Updates skill trigger/decision table to include RLS commands and workflow link.
plugins/kbagent/skills/kbagent/references/rls-workflow.md New workflow doc for authoring/validating/sharing RLS policies.
plugins/kbagent/skills/kbagent/references/gotchas.md Adds version-gated RLS gotchas and “backend not registered yet” guidance.
plugins/kbagent/skills/kbagent/references/commands-reference.md Adds reference section for kbagent rls commands and flags.
plugins/kbagent/agents/keboola-expert.md Updates expert agent guidance to include RLS authoring and constraints.
docs/web-server-endpoints.md Regenerates endpoint list to include the new rls router.
docs/error-codes.md Documents the new INVALID_RLS_POLICY error code.
CLAUDE.md Updates the hand-maintained command list to include the new rls group.
Review details

Suppressed comments (1)

src/keboola_agent_cli/services/_rls_condition.py:139

  • compile_condition_preview() currently returns "TRUE" whenever the key 'true' is present, regardless of its value. Even with improved validation, this function can be called independently (e.g., future reuse) and should not render {"true": false} as an always-true predicate.
    if "true" in condition:
        return "TRUE"
  • Files reviewed: 25/25 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 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/services/_rls_condition.py Outdated
Comment thread src/keboola_agent_cli/commands/rls.py Outdated
@Matovidlo
Matovidlo force-pushed the martinvasko-cli-17-kbagent-rls-command-group-guided-setup-skill-for-metastore branch from a8901e5 to d6807d3 Compare October 2, 2026 08:07
@Matovidlo
Matovidlo requested a lite review from Copilot October 2, 2026 16:03

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.

Comment thread src/keboola_agent_cli/commands/rls.py
Comment thread src/keboola_agent_cli/services/_rls_condition.py
Comment thread src/keboola_agent_cli/services/rls_service.py Outdated
Comment thread src/keboola_agent_cli/services/rls_service.py Outdated
Comment thread src/keboola_agent_cli/commands/cls.py
Comment thread src/keboola_agent_cli/commands/rls.py Outdated
Comment thread src/keboola_agent_cli/metastore_client.py
Comment thread src/keboola_agent_cli/services/rls_service.py Outdated
Comment thread tests/test_e2e.py
Comment thread plugins/kbagent/skills/kbagent/references/rls-workflow.md
@Matovidlo
Matovidlo force-pushed the martinvasko-cli-17-kbagent-rls-command-group-guided-setup-skill-for-metastore branch from 59872a5 to 5ae3644 Compare October 5, 2026 04:04
@Matovidlo
Matovidlo requested a lite review from Copilot October 5, 2026 04:06

Copilot AI left a comment

Copy link
Copy Markdown

Comment thread src/keboola_agent_cli/services/rls_service.py
Comment thread tests/test_e2e.py
Comment thread plugins/kbagent/skills/kbagent/references/gotchas.md Outdated
Comment thread src/keboola_agent_cli/metastore_client.py Outdated
Comment thread src/keboola_agent_cli/metastore_client.py Outdated

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.

Copilot review overview

🟡 Changes recommended

Unresolved targeted-scope, validation, error-handling, setup, and skill-trigger findings remain.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (5)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Preserve existing trigger phrases when adding new ones

plugins/​kbagent/​skills/​kbagent/​SKILL.md:21

This replacement removes the existing trigger phrases zero-copy clone and workspace load type while adding rls, cls. That narrows activation for already-supported workspace workflows; append the new triggers instead of dropping the old ones.

Medium severity Fail on table-listing errors instead of reporting no tables

src/​keboola_agent_cli/​commands/​rls.py:606

StorageService.list_tables() returns per-project failures in tables_result["errors"] instead of raising. This path ignores that field and reports No tables found with exit 0 when table listing failed (for example, an invalid token), so rls setup can appear successful without creating anything. Surface the errors and fail before treating the list as empty.

Medium severity Validate policy operation type before set membership check

src/​keboola_agent_cli/​services/​_rls_condition.py:104

op comes directly from arbitrary JSON, so a malformed rule such as {"column": "x", "op": []} makes op not in RLS_CONDITION_OPS raise TypeError because lists are unhashable. With the live schema unavailable, this escapes the CLI's handled exceptions as a traceback instead of INVALID_RLS_POLICY; guard that op is a string before the set-membership check.

Comment thread src/keboola_agent_cli/services/rls_service.py Outdated
Comment thread src/keboola_agent_cli/services/rls_service.py Outdated

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.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain in validation, setup error handling, project-ID handling, and REST request models.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (2)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Accept integer-compatible project IDs in CLS models

src/​keboola_agent_cli/​server/​routers/​cls.py:35

The CLS REST models have the same list[str] contract, so numeric project IDs in a JSON request are rejected before reaching the shared project_ids_as_ints validation. This makes the REST API inconsistent with the metastore wire contract and the existing auth/sharing project-ID models; use an integer-compatible input type for both CLS request models.

Medium severity Accept integer project IDs in REST request models

src/​keboola_agent_cli/​server/​routers/​rls.py:47

The REST body declares target project IDs as strings, but the metastore contract implemented in this PR serializes targetProjectIds as positive integers (and the other REST models use list[int]). A normal REST request such as {"target_project_ids": [999]} is therefore rejected by Pydantic with 422 before the service can normalize it; accept integer IDs (or both integer/string inputs) here and in the update model.

Medium severity Validate local principal types and non-empty values

src/​keboola_agent_cli/​services/​_rls_condition.py:130

The local principal check uses truthiness and only checks that principals is a list. When live-schema validation is unavailable (a documented degraded path), values such as {"principal": {"email": "a@x.com"}} or {"principals": [123]} pass and are sent as policy rules even though the enforcement contract requires principal strings. Validate the selected field's type and non-empty string members locally, while still rejecting both fields being present.

Comment thread src/keboola_agent_cli/services/_rls_condition.py

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.

Copilot review overview

🟡 Changes recommended

Unresolved policy-validation, REST payload/error handling, and skill-trigger findings remain.

Review effort: Lite
Findings: 2 High severity

Open (2)
Resolved since last review (1)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Accept numeric target project IDs in REST models

src/​keboola_agent_cli/​server/​routers/​cls.py:34

The REST body models target project IDs as strings, but these are numeric project IDs on the wire (targetProjectIds: [int]) and the existing /sharing REST model uses list[int] for the same concept. A normal JSON request such as {"target_project_ids": [123]} can therefore be rejected by Pydantic before reaching the service. Use a numeric (or explicitly int-or-string) model for both create and update bodies, then normalize before calling the service.

Medium severity Accept numeric target project IDs in REST models

src/​keboola_agent_cli/​server/​routers/​rls.py:46

The REST body models target project IDs as strings, but these are numeric project IDs on the wire (targetProjectIds: [int]) and the existing /sharing REST model uses list[int] for the same concept. A normal JSON request such as {"target_project_ids": [123]} can therefore be rejected by Pydantic before reaching the service. Use a numeric (or explicitly int-or-string) model for both create and update bodies, then normalize before calling the service.

Low severity Return named validation results instead of tuples

src/​keboola_agent_cli/​services/​rls_service.py:182

This new helper returns two semantically distinct values as a positional tuple, which violates the repository's binding convention that multi-value returns use a dataclass. A named result would make the errors/warnings contract explicit and avoid positional coupling as validation grows.

Comment thread src/keboola_agent_cli/services/_rls_condition.py Outdated
Comment thread src/keboola_agent_cli/services/rls_service.py

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.

Copilot review overview

🟡 Changes recommended

Critical REST target-project ID handling and concurrency issues, along with additional validation and error-mapping findings, remain unresolved.

Review effort: Lite
Findings: 3 High severity

Open (3)
Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Validate operation type before set membership testing

src/​keboola_agent_cli/​services/​_rls_condition.py:110

op can come from arbitrary JSON, including an array or object. Testing an unhashable value with op not in RLS_CONDITION_OPS raises TypeError, so malformed rules can escape the intended INVALID_RLS_POLICY validation and produce a traceback (and rls setup does not catch TypeError). Check that op is a string before membership testing.

Low severity Preserve existing skill trigger phrases

plugins/​kbagent/​skills/​kbagent/​SKILL.md:21

This replacement drops the existing zero-copy clone and workspace load type trigger phrases from the skill description while adding rls, cls. That weakens the skill's activation for those already-supported workflows; retain the old triggers and append the new ones instead.

Comment thread src/keboola_agent_cli/server/routers/cls.py Outdated
Comment thread src/keboola_agent_cli/server/routers/rls.py Outdated
Comment thread src/keboola_agent_cli/services/rls_service.py Outdated

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.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings affect permission enforcement, target-grant updates, validation, and error handling.

Review effort: Lite
Findings: 2 High severity · 2 Medium severity · 1 Low severity

Open (5)
Resolved since last review (3)

Comment thread src/keboola_agent_cli/commands/rls.py
Comment thread src/keboola_agent_cli/services/rls_service.py Outdated
Comment thread src/keboola_agent_cli/commands/rls.py Outdated
Comment thread src/keboola_agent_cli/services/_rls_condition.py
Comment thread tests/test_e2e.py

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.

Copilot review overview

🟡 Changes recommended

Unresolved validation, scope, REST project-reference, error-reporting, and grant-handling issues remain.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (5)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Reject unsupported project-scoped policy updates

src/​keboola_agent_cli/​services/​rls_service.py:375

The update preserves whatever meta["scope"] contains, including "project", so an externally-created or legacy project-scoped policy can be modified through this command even though the service contract says RLS/CLS writes must never use project scope. Reject an existing unsupported scope before issuing put_item (defaulting only when the metadata is absent).

Comment thread src/keboola_agent_cli/metastore_client.py Outdated
Comment thread src/keboola_agent_cli/commands/rls.py Outdated
Matovidlo and others added 12 commits October 7, 2026 17:25
…t ids in --dry-run

- update: only an explicit, non-empty --target-project list derives a new scope. When the
  option is omitted or the grants are being cleared, the policy keeps its current scope. A
  targeted policy whose grants were cleared is legitimately "targeted" with an empty list;
  an unrelated update used to turn it into "organization".
- create/update: validate the target project ids before any network call, so --dry-run runs
  the same validation as the real write (INVALID_ARGUMENT) instead of reporting a successful
  preview for an id the write would reject.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… keys (Copilot review)

validate_condition_ops handled `{"and": [...], "or": [...]}` by validating only the "and" branch
and returning, so with the live schema unavailable the policy no longer matched the submitted
tree. An and/or condition must now have exactly one of those keys and no others, and a leaf
condition may carry only its op's own keys (value for comparisons, values for in/not_in), as the
schema's additionalProperties:false requires. compile_condition_preview is equally strict.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ved version listing (Copilot review)

- validate_principal_fields decided "exactly one of principal/principals" by truthiness, so
  {"principal": "a", "principals": []} or {"principal": 1} passed when the live schema was
  unavailable and the service wrote a rule that does not satisfy the documented shape. Key
  presence now decides "exactly one", and the value is checked: principal must be a non-empty
  string, principals a non-empty list of non-empty strings. (Shared with the cls service.)
- fetch_resolved_schema returns the bare {"versions": [...]} listing when it cannot pick a
  version. That is a valid, constraint-free JSON Schema, so validating against it silently
  skipped every structural check without the documented warning. It is now treated as
  unavailable (schema=None with a reason, surfaced as a warning on create/update).

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…pdate re-sends no grants; non-string op (Copilot review)

- `rls setup` performs `create`'s writes but was only gated as `rls.setup`; an exact `rls.create`
  denial now blocks it too (checked before any API call).
- When every table of a `setup` fails, the exit code now follows the error class of the failures
  (auth -> 3, config -> 5, ...) like `rls create`, instead of a synthetic general failure; a mix of
  classes stays 1.
- `update` omitting --target-project re-sent the grant snapshot it had just read through
  put_target_projects, which could overwrite a concurrent grant change. Only an explicit list (or
  the clear path) touches grants now.
- validate_condition_ops tested `op in <set>` with the raw value, so an unhashable op such as `[]`
  raised TypeError. A non-string op is reported as an unknown op.
- E2E: create/detail/update/delete round-trip for rls and cls (skips on a stack or token that cannot
  author policies; the policy is always deleted) and the `rls setup` refusal under --json.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… error code in the setup envelope (Copilot review)

- project_ids_as_ints used int(), which truncates 1.5 to 1 and turns True into 1, so a REST or
  programmatic caller could grant a policy to a different project than the one named. Booleans and
  non-integral floats are rejected (2.0 is still accepted as 2).
- When every table of `rls setup` fails with the same error class, the final error now carries that
  class's code (INVALID_TOKEN, CONFIG_ERROR, ...) instead of a fixed API_ERROR, matching its exit
  code; a mix of classes stays API_ERROR.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The services took `list[int | str]`, which `list[int]` from the REST bodies is not assignable to
(lists are invariant). They now take `Sequence[int | str]`, and project_ids_as_ints takes a
Sequence. The test helper's id list is typed loosely. No behaviour change.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…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>
PATCH merges only the given top-level data keys under a row lock
(go-monorepo simpleJSONMerge), so unknown keys and concurrent changes
to other keys survive a policy update. The server validates only the
patched keys, so callers validate the merged record themselves.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… enforcement

The enforcement refuses all queries of a project when one policy names
a principal twice on a table, or has a principal with whitespace; a null
comparison renders col = NULL and matches nothing. Refuse all three
locally. The preview quotes columns per dialect and renders booleans as
TRUE/FALSE, as the enforcement does. Target project 0 is rejected (the
metastore requires positive IDs).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…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>
Every listed policy applies to the project; organization scope reaches
every project of the organization. Document the schema ACL (project
admin vs organization admin), the row-level-security feature, the
checks that keep one policy from blocking all queries, and the new
flags. Restore the SKILL.md trigger keywords; regenerate the endpoint
reference.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An organization-scope policy in the shared E2E organization would reach
every project there, and switching its dialect would block their
queries. Use the default targeted scope and the project's own dialect.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Matovidlo
Matovidlo force-pushed the martinvasko-cli-17-kbagent-rls-command-group-guided-setup-skill-for-metastore branch from 051a08b to 32cc0d0 Compare October 7, 2026 16:11
@Matovidlo

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. Rebased on main (0.97.0) and addressed everything below. I checked each point against go-monorepo origin/main (services/metastore: the rls-policy/cls-policy 1.0.0 schemas, meta_object_repository.go, repository.go, metaobject_helpers.go, security_policy_read.go) and the enforcement in mcp-server#709.

Blocking

  1. Default scope → the default is now targeted, the schema's own x-metastore.scope.default. Without --target-project, a policy governs only the owning project. --scope organization must be passed explicitly and is gated as destructive (rls.create --scope organization / cls.create --scope organization in FLAG_ESCALATIONS, also checked on POST /rls|cls/{project}). --target-project with --scope organization is exit 2. I did not make --scope mandatory: targeted with no grants governs only the owning project, so it is the safe default you asked for. The five doc locations (and every other doc surface) now say that every listed policy applies, and that organization reaches every project with the feature. list/detail show owner_project_id (meta.projectId, (organization) when absent).

  2. One invalid policy blocks every query → kbagent now refuses, before any write:

    • a dialect other than the project backend. --dialect defaults to the backend (verify_token().default_backend); a backend other than snowflake/bigquery is refused.
    • a principal named twice on one table, case-folded, within the policy and across the other policies the token can list on the same table key (lowercased on Snowflake, exact on BigQuery, like rls.py). The cross-policy check is best effort: a project admin lists only the policies its project owns (security_policy_read.go), which the docs say.
    • principals with whitespace or control characters (rls.py's _PRINCIPAL_RE, which would also raise).

    Remaining hazard, which is on the enforcement side: an organization policy is loaded by every project in the organization, whatever its backend. In a mixed Snowflake/BigQuery organization it blocks every project on the other backend. kbagent can only check the owner's backend. The docs say to prefer targeted there. I'll raise it on docs(rfc): rewrite the Layer 1 RFC after review [DMD-1900] #709.

Same as #715

  • --target-project → resolve_target_project_ids from feat(semantic-layer): metastore scope/ACL support (targeted + org-wide) [AI-3790] #715: alias or ID, comma lists, de-duplicated, same-stack check, exit 2. I also made it reject 0 (the metastore validates gt=0), which affects the semantic layer too.
  • update result → re-read after the write.
  • 409 message → its own message for policies ("already exists for table X in this project… rls update --policy-id / rls delete").
  • Permission class → rls.delete / cls.delete are destructive; --deny-destructive blocks them.

Other findings

  • Grants twice on create → the POST carries targetProjectIds (stored in the same transaction, meta_object_repository.go:270-274); the follow-up put_target_projects is gone.
  • Rules before grants → the grants change first, so a 403 on manageGrants writes nothing.
  • PUT → PATCH → new MetastoreClient.patch_item(item_type, item_id, *, name=None, data=None). update keeps the GET and sends only the changed keys (name too when the table changes). One finding while checking simpleJSONMerge: the PATCH validates only the patched keys, against a schema without required/additionalProperties (meta_object_repository.go:1669-1728). kbagent therefore validates the merged policy locally (and against the live schema) before it sends the PATCH.
  • Preview vs enforcement → columns are quoted per dialect ("col" / `col`), booleans render TRUE/FALSE, and a null comparison value (or null in values) is refused with a pointer to is_null, because the enforcement renders col = NULL.
  • rls setup --json → a JSON error envelope (INVALID_ARGUMENT, exit 2). Wizard values are parsed as JSON scalars when they are one (42, true); quoting keeps a string. The dialect defaults to the backend.
  • Prerequisites → documented from the schema ACL: a project admin may create/update/delete targeted policies of its own project without grants; grants and organization need the organization-admin role; nothing is enforced without the row-level-security feature.
  • SKILL.md → zero-copy clone / workspace load type restored.

Command shape (#791)

All six rows applied: --table-id; --dialect is a click.Choice (and optional); --target-project takes alias or ID; rls|cls delete --dry-run; REST bodies use table_id / target_projects; update is PATCH /rls|cls/{project}/{policy_id} (the two routers now share one builder).

Nits

  • The rls_service docstring is updated (scope default, who may write).
  • _validate_policy returns a PolicyValidation dataclass.
  • The E2E test creates a targeted policy with no grants and the project's own dialect, and changes the rules instead of the dialect.

Follow-up (semantic layer)

Agreed: semantic-layer edit → patch_item (changed keys only, the cascade only metrics), while import --overwrite / promote keep put_item. That's a separate change, tracked in #833; patch_item from this PR is available for it.

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.

🟡 Changes recommended

Unresolved moderate issues remain in setup error handling, input validation, and target-project enforcement.

2 open findings

🧠 Review effort: Lite

Comment thread src/keboola_agent_cli/commands/rls.py Outdated
Comment thread src/keboola_agent_cli/services/_rls_condition.py
… scalar operands only (Copilot review)

rls setup listed tables and prompted before the service refused
--scope organization with --target-project, and a declined prompt then
exited 0; the conflict is now a usage error before any call (create too).
Comparison and membership operands must be strings, numbers or booleans,
so an object or array is refused even when the live schema is unavailable.

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

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.

🔵 Needs a closer look

Unresolved moderate issues remain in PATCH coverage, organization-scope validation, Unicode target-project handling, and E2E policy-name isolation.

0 open findings

2 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Restrict target project IDs to ASCII digits

src/​keboola_agent_cli/​services/​_semantic_layer_scope.py:55

str.isdigit() is true for Unicode digit characters such as ², but int(target) then raises ValueError. An invalid --target-project ² therefore escapes the normal INVALID_ARGUMENT path and can produce a traceback in both the new RLS/CLS commands and semantic-layer scope resolution. Restrict this check to ASCII decimal IDs (or catch the conversion) before calling int().

🧠 Review effort: Lite

… unique E2E policy table (Copilot review)

str.isdigit() is also true for characters like '²', which int() rejects,
so --target-project ² escaped INVALID_ARGUMENT with a traceback (rls/cls
and semantic-layer scope). Cover MetastoreClient.patch_item directly, and
give the E2E policy a unique table: policies are named by table, so
concurrent runs would collide with a 409.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…xisting helpers

- rls/cls commands share list/detail/schema/create/update/delete and the
  printers through a PolicyGroup descriptor; cls.py keeps only its options
- errors go through the existing _handle_service_call (JSON errors keep
  details); _is_interactive and the unused project_ids_as_ints are gone
- MetastoreClient builds ALREADY_EXISTS in one place with a caller hint;
  the service no longer re-wraps it
- json_utils.draft7_errors replaces the copied Draft7 formatting in flow
  and rls validation; one comparison-operator map
- the schema fetch reuses the open metastore client and the project
  backend is looked up once per project, not on every write

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

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Matovidlo and others added 3 commits October 7, 2026 19:33
… drop backend memo

- exit_code_for() in commands/_helpers.py, used by handle_service_call
  (moved there; the semantic layer keeps its alias) and by rls setup, so
  a usage error exits 2 there too, as in rls create
- Dialect / PolicyScope enums live in the service layer; the CLI options
  and REST bodies import them instead of re-typing the values
- drop the per-service backend memo: under kbagent serve it grew without
  bound, keyed by tokens
- drop the REST target/scope validator: the service refuses the
  combination and REST answers 400
- remove dead loggers and the validate_policy_structural wrapper, derive
  PolicyGroup.label, explicit delete parameters, stale docstrings

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…, default, $identity)

Schema 1.1.0 becomes the metastore default (go-monorepo draft PR), so new
writes are 1.1.0 without sending a version.
- rules select identities by principal, principals or IdP groups
- every rule matching one identity applies (RLS conditions OR, CLS columns
  union), so the duplicate-principal refusal and the cross-policy listing
  it needed are removed
- rls --default (CLI and REST) sets the condition for identities no rule
  matches; {"false": true} is a new sentinel (no rows)
- value {"$identity": "email"} / values {"$identity": "groups"} placeholders
  are accepted and previewed as <identity.email> / (<identity.groups>)
- a stored policy without dialect (optional in 1.1.0) merges with the
  project backend on update; a CLS policy refuses a default
- docs: CLAUDE.md, AGENT_CONTEXT, commands-reference, gotchas, rls-workflow
  (new 1.1.0 workflow), keboola-expert

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…irement, vNEXT tags (review)

The engine now combines several policies on one table with AND (CLS:
intersection), so an added policy can only narrow access, and a reader
with no identity is always refused (security review on
keboola/mcp-server#709). The docs said rules combine with OR across
policies. Also: the CLS section no longer mentions the removed
duplicate-principal check, 1.1.0 passages carry (since vNEXT), and the
docs say a 1.1.0 policy needs an MCP engine that reads 1.1.0.

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

This branch has not been deployed

No deployments
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.

4 participants