feat(api): add Fabric skill telemetry by shirasassoon · Pull Request #282 · microsoft/fabric-cli

shirasassoon · 2026-08-30T09:33:45Z

✨ Description of new changes

Add an internal, global --skill argument for command-scoped Fabric skill attribution.
Store valid skill attribution in command context so internal requests retain it; malformed values are silently omitted, consistent with existing telemetry.
Add x-ms-fabric-skill centrally to Fabric control-plane requests while excluding OneLake, Azure, and Power BI audiences.
Exclude skill attribution from debug logs, consistent with existing User-Agent telemetry.
Keep the focused attribution coverage in test_fab_skill_attribution.py.

Validation

py -3.13 -m pytest -q tests\test_core\test_fab_skill_attribution.py (20 passed)
py -3.13 -m mypy tests\test_core\test_fab_skill_attribution.py --ignore-missing-imports
Targeted Black check passes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Adds first-class Fabric skill attribution to the CLI by introducing a global --skill flag / FABRIC_SKILL env var, validating the skill name once per command, storing it in the process-wide command Context, and injecting x-ms-fabric-skill into Fabric control-plane requests (but not OneLake/Azure/Power BI) so retries, pagination, and LRO polling retain attribution.

Changes:

Add global --skill parameter (with env var fallback) and persist the resolved value in Context.
Validate skill names and surface a structured invalid-skill error message.
Inject x-ms-fabric-skill centrally in the API client for Fabric-audience requests; update docs, changelog entry, and add/extend tests.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/test_parsers/test_fab_global_params.py	Adds parser-level coverage for `--skill` and verifies it survives placement across the command tree.
tests/test_core/test_fab_decorators.py	Validates precedence/clearing behavior for skill resolution in `set_command_context`.
tests/test_core/test_fab_context.py	Adds unit tests for skill name validation logic on `Context.fabric_skill`.
tests/test_core/test_fab_api_client.py	Adds API-client tests to ensure header injection is scoped and overrides user-provided variants.
tests/conftest.py	Ensures singleton `Context` test fixture resets the new `_fabric_skill` state.
src/fabric_cli/parsers/fab_global_params.py	Introduces the global `--skill` flag definition.
src/fabric_cli/errors/common.py	Adds a shared error message for invalid Fabric skill names.
src/fabric_cli/core/fab_decorators.py	Resolves skill from args/env and stores it into `Context` at command start.
src/fabric_cli/core/fab_context.py	Adds `fabric_skill` property with validation and typed storage.
src/fabric_cli/core/fab_constant.py	Adds `FABRIC_SKILL` env var name and `x-ms-fabric-skill` header constant.
src/fabric_cli/client/fab_api_client.py	Injects `x-ms-fabric-skill` for Fabric control-plane calls and strips conflicting header spellings.
docs/essentials/parameters.md	Documents `--skill` behavior, precedence, and audience exclusions.
docs/commands/index.md	Lists `--skill` among global parameters.
.changes/unreleased/added-20260830-122126.yaml	Adds changie entry for the new attribution feature.

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

 from fabric_cli.core import fab_constant
 from fabric_cli.core.fab_auth import FabAuth
+from fabric_cli.core.fab_context import Context
 from fabric_cli.core.fab_exceptions import FabricAPIError


Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/test_core/test_fab_api_client.py:17

The Context import is unused in this test module and will trigger unused-import linting (and adds noise to the import section). Remove it unless you intend to use it in the new test cases.

from fabric_cli.core.fab_context import Context

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/test_core/test_fab_api_client.py:18

Context is imported but never referenced in this test module (the later @patch("fabric_cli.core.fab_context.Context") uses a string and does not require the import). This will typically fail linting (unused import).

from fabric_cli.core import fab_constant
from fabric_cli.core.fab_auth import FabAuth
from fabric_cli.core.fab_context import Context
from fabric_cli.core.fab_exceptions import FabricAPIError

Copilot

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/test_core/test_fab_api_client.py:18

Unused import: Context is imported but never referenced in this test module (no Context usage and @patch("fabric_cli.core.fab_context.Context") uses a string path). Removing it avoids unused-import lint warnings and keeps the test file tidy.

from fabric_cli.core import fab_constant
from fabric_cli.core.fab_auth import FabAuth
from fabric_cli.core.fab_context import Context
from fabric_cli.core.fab_exceptions import FabricAPIError

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/fabric_cli/parsers/fab_global_params.py:7

add_global_flags now touches argparse types, but the parser parameter is still untyped. This repo expects type annotations on functions; adding the concrete argparse.ArgumentParser type here will improve correctness and static checking.

def add_global_flags(parser) -> None:

tests/test_core/test_fab_api_client.py:18

Context is imported but never used in this test module (the patch uses a string path). Keeping an unused import adds noise and may trip linters.

from fabric_cli.core import fab_constant
from fabric_cli.core.fab_auth import FabAuth
from fabric_cli.core.fab_context import Context
from fabric_cli.core.fab_exceptions import FabricAPIError

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

tests/test_core/test_fab_logger.py:108

This test sets FAB_DEBUG_ENABLED to "1", but log_debug_http_request only logs when the config value is exactly "true". As written, the function returns early and the assertion passes without exercising the header-filtering logic; set the mocked config to "true" so the test actually validates the behavior.

    monkeypatch.setattr(fab_state_config, "get_config", lambda x: "1")

src/fabric_cli/core/fab_context.py:88

Context.fabric_skill setter can raise TypeError if a non-string value is assigned (e.g., bytes/int), because re.fullmatch expects a str when the pattern is a str. Since fabric_skill can be set from argparse Namespace/external callers, it should defensively coerce non-str values to None before running the regex.

        if fabric_skill is not None and not re.fullmatch(
            r"[A-Za-z0-9][A-Za-z0-9._-]{0,127}", fabric_skill
        ):
            fabric_skill = None
        self._fabric_skill = fabric_skill

Copilot

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/fabric_cli/core/fab_context.py:88

Context.fabric_skill setter calls re.fullmatch(..., fabric_skill) for any non-None value. If a non-string is ever assigned (e.g., via internal code or tests), re.fullmatch will raise TypeError instead of silently omitting the malformed value as intended. Guard with an isinstance(fabric_skill, str) check before running the regex and treat non-strings as invalid (set to None).

        if fabric_skill is not None and not re.fullmatch(
            r"[A-Za-z0-9][A-Za-z0-9._-]{0,127}", fabric_skill
        ):
            fabric_skill = None
        self._fabric_skill = fabric_skill

tests/test_core/test_fab_api_client.py:18

Unused import: Context is imported but not referenced anywhere in this test module (the Context mocking uses string-based @patch(...)). Removing it avoids lint noise and keeps imports minimal.

from fabric_cli.core import fab_constant
from fabric_cli.core.fab_auth import FabAuth
from fabric_cli.core.fab_context import Context
from fabric_cli.core.fab_exceptions import FabricAPIError

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…to add-fabric-skill-attribution

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Alon Yeshurun (ayeshurun) · 2026-08-30T10:09:12Z

+    parser.add_argument(
+        "--skill",
+        required=False,
+        default=argparse.SUPPRESS,
+        help=argparse.SUPPRESS,
+    )


do we need both arg and envar?

As discussed we will only support arg for P0

Alon Yeshurun (ayeshurun) · 2026-08-31T07:11:37Z

+    @property
+    def fabric_skill(self) -> Optional[str]:
+        return self._fabric_skill
+
+    @fabric_skill.setter
+    def fabric_skill(self, fabric_skill: Optional[str]) -> None:
+        if fabric_skill is not None and not re.fullmatch(
+            r"[A-Za-z0-9][A-Za-z0-9._-]{0,127}", fabric_skill
+        ):
+            fabric_skill = None
+        self._fabric_skill = fabric_skill


not sure it is necessary, we can just read the args.skill in do_request instead, making the changes in this PR even smaller. (also no need the fab_decorator change).

I removed the regex validation, but the context storage and decorator need to be kept ensuring that attribution gets propagated to internal Fabric requests. Because --skills exists only on the original command's args, internal requests often use newly created Namespace() objects without skill. The decorator copies the value into shared command context, then every do_request() can retrieve it, including path lookups and LRO polling. It also clears the previous value when a later command has no --skill, preventing incorrect attribution in interactive sessions.

Alon Yeshurun (ayeshurun) · 2026-08-31T07:12:47Z

    continuation_token=None,
    hostname=None,
 ) -> ApiResponse:
    json_file = getattr(args, "json_file", None)


Suggested change

fabric_skill = getattr(args, "skill", None)

json_file = getattr(args, "json_file", None)

fabric_skill = getattr(args, "skill", None) only works for the initial request. Internal lookups and LRO polling create fresh Namespace() objects without skill, so attribution would be lost. That's why fabric_skill must read from shared command context instead.

got it. thanks for the explanation

Alon Yeshurun (ayeshurun) · 2026-08-31T07:13:23Z

                fab_constant.ERROR_INVALID_OPERATION,
            )

+    if audience_value not in ("storage", "azure", "powerbi") and fabric_skill:


Suggested change

if audience_value not in ("storage", "azure", "powerbi") and fabric_skill:

if fabric_skill and audience_value == "fabric":

Fabric requests can use audience=None as well, so we can switch the exclusion logic to:
if fabric_skill and audience_value in (None, "fabric"):
more accurately or leave what we have

Alon Yeshurun (ayeshurun) · 2026-08-31T07:14:37Z

+        for header_name in list(headers):
+            if header_name.lower() == fab_constant.FABRIC_SKILL_HEADER:
+                del headers[header_name]
+        headers[fab_constant.FABRIC_SKILL_HEADER] = fabric_skill


why is it necessary?

aren't we set the skill header anways?

We can remove the case normalization logic and just keep as:

headers[fab_constant.FABRIC_SKILL_HEADER] = fabric_skill

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

+    @property
+    def fabric_skill(self) -> Optional[str]:
+        return self._fabric_skill
+
+    @fabric_skill.setter
+    def fabric_skill(self, value: Optional[str]) -> None:
+        self._fabric_skill = value
+


+    if fabric_skill and audience_value not in ("storage", "azure", "powerbi"):
+        headers[fab_constant.FABRIC_SKILL_HEADER] = fabric_skill


Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

    if headers_value is not None:
        if isinstance(args.headers, dict):
            headers.update(args.headers)
        else:
            raise FabricCLIError(
                ErrorMessages.Common.invalid_headers_format(),
                fab_constant.ERROR_INVALID_OPERATION,
            )

+    # The fabric skills header is only applicable for the Fabric audience (None or "fabric")
+    if fabric_skill and audience_value in (None, "fabric"):
+        headers[fab_constant.FABRIC_SKILL_HEADER] = fabric_skill
+


Alon Yeshurun (ayeshurun) · 2026-08-31T12:10:42Z

    continuation_token=None,
    hostname=None,
 ) -> ApiResponse:
    json_file = getattr(args, "json_file", None)


got it. thanks for the explanation

Alon Yeshurun (ayeshurun) · 2026-08-31T12:15:18Z

+        ["job", "run", "ws.Notebook", "--skill", "semantic-model-authoring"],
+    ],
+)
+def test_skill_argument_is_preserved_across_command_tree(command):


please add suffix to tests '_success' and '_failure'

Alon Yeshurun (ayeshurun) · 2026-08-31T12:16:40Z

+    assert "X-MS-FABRIC-SKILL" not in request_headers
+
+
+def test_skill_header_is_not_logged(monkeypatch):


please add tests for cases when --skill isn't a string

Alon Yeshurun (ayeshurun) · 2026-08-31T12:17:47Z

+    if not isinstance(fabric_skill, str):
+        fabric_skill = None


in what scenarios it wont be a string? if feasible, perhaps we should ensure that inside the setter?

feat(api): add Fabric skill attribution

25a4126

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI lite review requested due to automatic review settings August 30, 2026 09:33

shirasassoon requested a review from a team as a code owner August 30, 2026 09:33

Copilot started reviewing on behalf of shirasassoon August 30, 2026 09:34 View session

shirasassoon added the skip-changelog label Aug 30, 2026