feat(api): add Fabric skill telemetry

[shirasassoon]

✨ Description of new changes

• Add an internal, global --skill argument for command-scoped Fabric skill attribution.
• Store skill attribution in command context so internal requests retain it; non-string values are omitted.
• 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.

[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.

[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

[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

[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

[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

[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 2 comments.

[Copilot]

Pull request overview

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

[Copilot]

🔵 Needs a closer look

The implementation and tests don’t yet fully enforce the PR-stated behavior of silently omitting malformed skill values and one test currently patches auth in a way that can instantiate real singleton state at import time.

Review details

Suppressed comments (3)

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

src/fabric_cli/core/fab_context.py:84

• The PR description says malformed skill attribution should be silently omitted, but Context.fabric_skill currently stores any value without validation. That allows invalid header values to propagate into requests (and import re is currently unused). Consider validating/sanitizing here so only safe skill tokens are persisted in the singleton Context.
  tests/test_core/test_fab_skill_attribution.py:97
• @patch.object(FabAuth(), ...) instantiates the singleton FabAuth at import time (before fixtures run), which can make the test less hermetic by touching real auth/config state. Patch within the test after applying the existing auth isolation fixture, and avoid import-time construction.
  tests/test_core/test_fab_skill_attribution.py:109
• The tests assert the header is added for a valid skill, but they don’t cover the stated behavior that malformed skill values are silently omitted. Adding a small negative test case will prevent regressions once validation is in place.

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new fabric_skill accepts any string, which can cause request failures (e.g., invalid header values) and does not reliably “silently omit malformed values” as described without additional normalization/validation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The implementation does not currently omit malformed string skill values as described, so validation/sanitization should be added before propagating the value into an HTTP header.

Review details

Suppressed comments (1)

src/fabric_cli/core/fab_context.py:84

• The Context.fabric_skill setter currently only normalizes non-string values; any malformed string (e.g., whitespace-only, contains spaces/control chars, overly long) will be preserved and then propagated into the x-ms-fabric-skill request header. This doesn’t match the PR description’s requirement that malformed values are silently omitted (and it’s safer to sanitize before using as an HTTP header value).

    @fabric_skill.setter
    def fabric_skill(self, value: Optional[str]) -> None:
        self._fabric_skill = value if isinstance(value, str) else None

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new header value should be validated to avoid invalid/injectable HTTP headers, and the new tests should not instantiate the FabAuth singleton during module import.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

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

tests/test_core/test_fab_skill_attribution.py:133

• This test patches FabAuth() at decoration time, which instantiates the singleton during module import (running _load_auth() / _load_env()), making the unit test slower and potentially flaky depending on environment/filesystem state. Prefer patching the FabAuth callable in the module so no real auth initialization happens.

This issue also appears on line 149 of the same file.

tests/test_core/test_fab_skill_attribution.py:149

• This test uses @patch.object(FabAuth(), ...), which forces singleton initialization at import time. Patch the FabAuth callable instead to avoid reading auth files/env during test collection.

@patch.object(FabAuth(), "get_access_token", return_value="dummy-token")

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite