From 5bc9cb2c14f529a98d38c20c91700d20ea9db7a5 Mon Sep 17 00:00:00 2001 From: Alon Yeshurun Date: Sun, 2 Aug 2026 14:34:18 +0300 Subject: [PATCH 01/10] Draft Azure CLI authentication design AB#1694265 ## Summary Scaffold feature 1694265 and add the Fabric CLI-specific design and seven-slice implementation plan for Azure CLI authentication. ## Prompting Intent Draft the repo design spec from the Feature Registry artifacts, create an implementation plan and task breakdown, and prepare the design readiness gate required before syncing tasks to ADO. ## Linked Sources - Requirements spec: https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/requirements-spec.md - Engineering design: https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/engineering-design.md - Implementation handoff: https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/implementation-handoff.md - Test plan: https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/test-plan.md ## Rationale Translate the locked cross-cutting contract into concrete Fabric CLI modules and reviewable workstreams while leaving security, host-integration, and rollout decisions as explicit gates. Task work items are intentionally deferred until this design is merged, as required by the feature readiness policy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9c368147-9689-45f9-9f29-fc882429424b --- .github/hooks/hooks.json | 15 ++ Features/1694265/design-spec.md | 229 ++++++++++++++++++++++++ Features/1694265/implementation-plan.md | 71 ++++++++ Features/1694265/registry.md | 20 +++ 4 files changed, 335 insertions(+) create mode 100644 .github/hooks/hooks.json create mode 100644 Features/1694265/design-spec.md create mode 100644 Features/1694265/implementation-plan.md create mode 100644 Features/1694265/registry.md diff --git a/.github/hooks/hooks.json b/.github/hooks/hooks.json new file mode 100644 index 00000000..f94d97b6 --- /dev/null +++ b/.github/hooks/hooks.json @@ -0,0 +1,15 @@ +{ + "version": 1, + "hooks": { + "sessionStart": [ + { + "type": "command", + "bash": "b=\"${HOME:-${USERPROFILE:-}}/.copilot/hooks\"; s=\"$b/.last-copilot-toolkit-plugin-update.$(date -u +%Y%m%d)\"; if mkdir -p \"$b\" 2>/dev/null && ( set -C; : > \"$s\" ) 2>/dev/null; then copilot plugin install https://dev.azure.com/msdata/A365/_git/copilot-toolkit 2>/dev/null || echo 'Plugin already installed or unavailable' >&2; fi", + "powershell": "try { $b = Join-Path $HOME '.copilot/hooks'; New-Item -ItemType Directory -Force -Path $b -ErrorAction Stop | Out-Null; $s = Join-Path $b ('.last-copilot-toolkit-plugin-update.' + [DateTime]::UtcNow.ToString('yyyyMMdd')); $fs = [System.IO.File]::Open($s, [System.IO.FileMode]::CreateNew); $fs.Close(); copilot plugin install https://dev.azure.com/msdata/A365/_git/copilot-toolkit 2>$null; if ($LASTEXITCODE -ne 0) { Write-Host 'Plugin already installed or unavailable' -ForegroundColor Yellow } } catch { }; exit 0", + "cwd": ".", + "timeoutSec": 120, + "comment": "Install/update Copilot Toolkit plugins at most once per day (daily stamp guard) to avoid Azure DevOps throttling" + } + ] + } +} diff --git a/Features/1694265/design-spec.md b/Features/1694265/design-spec.md new file mode 100644 index 00000000..c6a33752 --- /dev/null +++ b/Features/1694265/design-spec.md @@ -0,0 +1,229 @@ +# Design Spec — Feature 1694265 (fabric-cli) + +> Repo-specific design for adding Azure CLI as an explicit Fabric CLI authentication source. +> The parent [engineering design](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/engineering-design.md), [implementation handoff](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/implementation-handoff.md), and [test plan](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/test-plan.md) define the product contract. + +## Scope + +Fabric CLI owns the complete client-side implementation: + +- Add `azure-cli` as an explicit authentication source while preserving all existing direct Fabric CLI sources. +- Introduce a shared pre-dispatch authentication coordinator for command-line, batch, auth, and REPL execution. +- Acquire Azure CLI tokens noninteractively through `AzureCliCredential`. +- Persist a versioned source and identity binding, but never persist delegated Azure CLI tokens. +- Add passive and active authentication status, stable error/exit behavior, source-local logout, and source-aware SDK/deploy integration. +- Preserve the existing route labels and scopes: `fabric` and `powerbi` use the Fabric scope, `storage` uses the OneLake scope, and `azure` uses the ARM scope. +- Add feature gating, telemetry, documentation, and the cross-platform regression matrix required for rollout. + +### Non-goals + +- Running or wrapping `az login`, `az logout`, `az account set`, or any Azure CLI context mutation. +- Using `DefaultAzureCredential` or merging Azure CLI tokens into the MSAL cache. +- Persisting Azure CLI access or refresh tokens. +- Supporting arbitrary scopes, claims challenges, SQL, XMLA, Kusto, or non-Public Azure clouds in the first release. +- Normalizing every existing identity mode under the new `--source` syntax. + +## Current Architecture + +Authentication currently spans several independent paths: + +- `fabric_cli.main` special-cases auth commands and uses `_execute_command` only for other one-shot commands. +- `InteractiveCLI.handle_command` parses and invokes handlers independently. +- `FabAuth` combines persistent state, environment loading, provider selection, MSAL acquisition, and interactive renewal. +- `fab_api_client.do_request` acquires tokens during request execution, which can trigger interactive renewal. +- `fab_auth.status` requests three tokens, and `FabAuth.logout` resets unrelated configuration. +- `MsalTokenCredential` supports only the current Fabric CLI provider and remains headless. +- Config-file deploy creates the credential inside a catch-all that maps failures to `DeploymentFailed`. + +The implementation must separate policy, coordination, provider behavior, and persistence without regressing existing authentication modes. + +## Proposed Design + +### Shared parsed-command executor + +Create one executor used by one-shot, batch, auth, and REPL surfaces. It will: + +1. Classify the parsed command as local, passive auth, active auth, or authenticated. +2. Resolve interaction policy from command flags, output mode, host capability, CI/batch/pipe context, and `FAB_INTERACTION`. +3. Resolve the effective source using runtime environment overrides before the configured source. +4. Run authentication readiness before handler dispatch when required. +5. Invoke the parsed handler at most once and return its exit code without replay. + +Local commands such as help, version, passive status, and logout bypass token acquisition. Batch execution fails fast and reports executed, failed, and skipped counts. + +### Authentication coordinator + +Add a coordinator responsible for source resolution, interaction eligibility, chooser orchestration, candidate validation, atomic binding, and exactly-once continuation. Provider classes remain noninteractive. + +Source precedence: + +1. Runtime environment credentials for the current process. +2. Persisted configured source. +3. Shared chooser only when no source exists and interaction is allowed. +4. `AuthenticationRequired` for unattended or deferred execution. + +The coordinator exposes a small result model containing configured source, effective source, principal capability, readiness state, and optional checked-audience expiration. It does not expose token values. + +### Provider boundary + +Define a provider protocol used by `FabAuth`, HTTP requests, status checks, and the SDK bridge: + +- Acquire exactly one allowlisted audience. +- Return token and expiration metadata. +- Validate tenant and principal against the active binding. +- Clear only process-local cached data for a requested audience. +- Report stable Fabric CLI errors without raw SDK or process output. + +Existing MSAL user, service-principal, managed-identity, federation, certificate, and raw-token behavior remain behind the direct `fabric-cli` provider. Interactive MSAL renewal moves out of ordinary provider acquisition and is initiated only by the coordinator when policy allows it. + +### Azure CLI provider + +Add `azure-identity` as a dependency and implement the provider with `AzureCliCredential`: + +- Resolve only a trusted Azure CLI executable through Azure Identity. +- Use disconnected standard input, no shell, a safe working directory, and a bounded 10-second acquisition. +- Pass one resolved `.default` scope per call. +- Allow only `fabric`, `storage`, `azure`, and `powerbi` route labels. +- Keep `powerbi` mapped to the existing Fabric scope. +- Cache successful tokens in process by source, tenant, principal, and audience. +- Coalesce concurrent misses for the same key; refresh inside the configured buffer; never cache failures. +- Clear and reject only the affected token when a claims challenge is returned. + +The provider never invokes Azure CLI login, logout, account selection, tenant selection, or subscription selection. + +### Command contract + +Extend `fab auth` with: + +```text +fab auth login --source azure-cli [--tenant ] [--no-prompt] +fab auth status [--check] [--audience fabric|storage|azure|powerbi] +``` + +Rules: + +- Azure CLI source flags conflict with direct source flags. +- Unattended Azure CLI login requires `--tenant` and `--no-prompt`. +- Bare attended login and eligible progressive first use share the existing chooser. +- Progressive discovery offers Azure CLI only for a supported user identity. +- Workload identities require explicit login in the first release. +- Default chooser action is defer; defer returns `AuthenticationRequired` and exit 4 without writing state. +- Cancellation returns exit 2 without writing state. + +### Persistent state + +Evolve `auth.json` to a versioned source record: + +```json +{ + "version": 2, + "source": "azure-cli", + "cloud": "AzureCloud", + "tenant_id": "", + "principal_id": "", + "principal_type": "user", + "account": "", + "subscription_id": null, + "subscription_name": null, + "app_id": "", + "bound_at": "" +} +``` + +Legacy records migrate idempotently to source `fabric-cli` without deleting the MSAL cache. Candidate source replacement follows validate-then-commit semantics: + +1. Validate Fabric readiness and identity. +2. Acquire the interprocess state lock. +3. Recheck the current state. +4. Atomically replace `auth.json`. +5. Preserve unrelated configuration and the prior state if any step fails. + +The configuration directory remains owner-only (`0700`) and auth state remains owner-only (`0600`). Runtime environment overrides never modify persistent state. + +### Identity binding + +The binding pins cloud, canonical tenant ID, stable principal ID, and principal type. Subscription metadata is display-only and nullable. Every fresh token must match the binding before a service request. + +The implementation mechanism for establishing stable tenant and principal identity is blocked on security decision Q4. If token-derived validation is approved, it must verify issuer, signature, audience, expiration, `tid`, and `oid`; otherwise the provider must use the approved metadata contract. Missing stable identity fails closed. + +### Status and logout + +Plain `fab auth status` remains exit 0 and becomes passive: no provider call, no Azure CLI process, and readiness `unknown` when local state is insufficient. + +`fab auth status --check --audience ` performs one active check and returns exit 0 when ready or exit 4 for readiness failures. Text output retains the current leading status line and legacy field order. Structured output retains the current envelope and legacy token keys with `"N/A"` during the deprecation window. + +Logout clears only Fabric-owned state for the configured source: + +- Azure CLI source: binding and in-process token cache. +- Direct source: current Fabric CLI auth state and its MSAL cache. +- All sources: Fabric CLI memory and context caches as required. + +Unrelated CLI configuration and Azure CLI state remain unchanged. + +### Errors and output + +Add structured error definitions through `fabric_cli.errors` and constants through `fab_constant`; do not hardcode user-facing messages in handlers. Readiness errors map to exit 4, usage/conflict/cancellation errors map to exit 2, and unexpected errors remain exit 1. + +All output uses existing `fab_ui` text and JSON renderers. JSON stdout contains one document; prompts and diagnostics use the diagnostic stream. Logs and telemetry exclude tokens, claims challenges, process output, command arguments, and identity identifiers. + +### HTTP, SDK, deploy, and user capability integration + +- `fab_api_client.do_request` requests tokens from the effective provider without allowing interaction or replay. +- Generalize `create_fabric_token_credential` behind its existing public factory so `fabric-cicd` receives a headless credential for the effective source. +- Run Fabric readiness preflight before entering deploy's catch-all so readiness failures preserve exit 4 and are not wrapped as `DeploymentFailed`. +- Replace source-specific `identity_type == "user"` checks with principal-capability checks for browser-open and personal-workspace behavior. + +## Dependencies + +- `azure-identity` for `AzureCliCredential`. +- Existing `azure-core`, MSAL, secure file utilities, output renderers, and command parser infrastructure. +- Security ownership and approval for executable resolution, principal binding, state, errors, and telemetry. +- Fabric agent-experience decision for the attended-host interaction channel. +- Fabric CLI engineering decision for the feature flag and rollout rings. + +## Rollout and Compatibility + +- Gate Azure CLI source selection and progressive offers independently where possible. +- Enable explicit unattended login before progressive offers. +- Preserve every existing direct authentication syntax and route mapping. +- Rollback disables Azure CLI selection and offers without destructively rewriting a recoverable source marker. +- Existing scripts consuming status retain legacy token keys as `"N/A"` for one deprecation window. + +## Testing Strategy + +### Unit tests + +- Parser conflicts, source selection, interaction classification, error mapping, and output models. +- Azure CLI executable absence, timeout, sanitized failures, scope allowlist, tenant/principal mismatch, refresh, cache coalescing, and failure non-caching. +- State migration, locking, atomic replacement, permissions, failed replacement preservation, and runtime-only environment precedence. +- Passive status zero-call behavior, active single-audience behavior, and source-local logout. + +### Integration tests + +- Shared execution across command-line, auth, REPL, JSON, pipe, CI, callbacks, and batch paths. +- Exactly-once handler and request continuation after consent. +- HTTP route-to-scope mapping and no request replay. +- Headless SDK bridge and deploy preflight error preservation. +- Existing direct user, service-principal, certificate, federation, managed-identity, and raw-token suites. + +### End-to-end and release tests + +- Windows, Linux, and macOS with Azure CLI installed, absent, signed out, user signed in, workload identity, guest tenant, and no subscription. +- Identity drift, Azure CLI context race, timeout, refresh, concurrency, and claims challenge. +- Representative Fabric Skills runs using a prepared Azure CLI identity without a second interactive login. +- Feature-gate enablement, disablement, and rollback. + +The parent test plan remains the acceptance evidence ledger for all 59 requirements and 40 mapped tests. + +## Open Gates + +| Gate | Owner | Blocks | +| --- | --- | --- | +| Q2: Select feature flag and rollout rings | Fabric CLI engineering | Rollout implementation | +| Q3: Assign final security approval owner | Fabric security and Fabric CLI leadership | Design lock | +| Q4: Approve principal identity validation mechanism | Fabric security | Binding implementation | +| Q5: Select attended-host interaction channel | Fabric agent experience and Fabric CLI | Attended-agent release | + +## Implementation Plan + +See [implementation-plan.md](implementation-plan.md) for the sequenced workstreams, dependencies, and proposed ADO task breakdown. diff --git a/Features/1694265/implementation-plan.md b/Features/1694265/implementation-plan.md new file mode 100644 index 00000000..74fcaebb --- /dev/null +++ b/Features/1694265/implementation-plan.md @@ -0,0 +1,71 @@ +# Implementation Plan — Feature 1694265 (fabric-cli) + +This plan decomposes the repo-specific [design spec](design-spec.md) into independently reviewable workstreams. The parent ADO item is User Story 1694265. + +## Delivery Principles + +- Land policy and compatibility tests before changing provider behavior. +- Keep all providers noninteractive below the coordinator. +- Preserve existing direct authentication while adding Azure CLI. +- Do not merge slices that depend on unresolved security or host-integration gates. +- Track requirement and test-plan evidence from the first implementation pull request. + +## Proposed Task Breakdown + +| Slice | Proposed ADO Task | Implementation scope | Primary files | Depends on | Exit criteria | +| ---: | --- | --- | --- | --- | --- | +| 1 | Add shared executor, authentication coordinator, and interaction policy | Unify command-line, auth, REPL, and batch dispatch; classify interaction; resolve effective source; map readiness exits; guarantee exactly-once handler execution | `main.py`, `core/fab_interactive.py`, new coordinator/executor modules, `core/fab_decorators.py` | None | All execution surfaces use one policy; local commands bypass auth; unattended commands do not prompt; readiness errors exit 4 | +| 2 | Add Azure CLI provider and explicit login contract | Add `azure-identity`; parser flags and conflicts; provider protocol; trusted executable behavior; timeout; sanitization; one-scope allowlist | `pyproject.toml`, `parsers/fab_auth_parser.py`, `commands/auth/fab_auth.py`, new provider modules, `errors/auth.py` | Slice 1 interface alignment | Explicit attended and unattended login paths validate Fabric without invoking Azure CLI login or accepting arbitrary scopes | +| 3 | Implement identity binding and atomic auth state | Versioned source state; legacy migration; runtime-only environment override; locking; atomic writes; permissions; pinned identity; process cache and concurrency | `core/fab_auth.py`, `core/fab_state_config.py`, `utils/fab_secure_io.py`, new state/cache modules | Slices 1-2; Q4 for principal validation | Failed replacement preserves prior state; fresh tokens match binding; no delegated token reaches disk | +| 4 | Implement shared chooser and exactly-once continuation | Progressive eligible-user discovery; default defer; alternate direct options; direct-terminal chooser; attended-host abstraction; separate-login fallback | Coordinator, `commands/auth/fab_auth.py`, `utils/fab_ui.py`, host interaction abstraction | Slices 1 and 3; Q5 for host transport | Default Enter defers; cancellation and defer write no state; successful consent invokes one handler/request | +| 5 | Implement passive/active status, stable errors, and source-local logout | Passive status; `--check`; audience selection; compatibility output; all stable errors; stream separation; capability checks; scoped logout | Auth parser/commands, `core/fab_constant.py`, `errors/auth.py`, output models, `commands/fs/fab_fs_open.py` | Slices 1-3 | Passive status makes zero provider calls; active status checks one audience; logout preserves unrelated and Azure CLI state | +| 6 | Integrate HTTP, SDK bridge, deploy, and batch paths | Source-aware request acquisition; headless credential factory; deploy preflight before catch-all; claims behavior; fail-fast batch counts; Power BI route compatibility | `client/fab_api_client.py`, `core/fab_msal_bridge.py`, deploy command, `main.py` | Slices 1-5 | No replay; SDK callbacks stay headless; deploy readiness exits 4; Power BI remains mapped to Fabric scope | +| 7 | Complete release matrix, docs, telemetry, and Skills pilot | Full regression matrix; telemetry safety; docs/examples; feature gates; rollout and rollback evidence; representative Skills runs | Tests, docs, telemetry integration, release configuration | Slices 1-6; Q2-Q3 | All parent test-plan rows have evidence; direct sources regress cleanly; rollout and rollback are approved | + +## Dependency Graph + +```text +Slice 1 ──┬──> Slice 2 ──> Slice 3 ──┬──> Slice 4 ──┐ + │ └──> Slice 5 ──┼──> Slice 6 ──> Slice 7 + └─────────────────────────────────────────┘ + +Q4 gates identity validation in Slice 3. +Q5 gates attended-host integration in Slice 4. +Q2 and Q3 gate rollout completion in Slice 7. +``` + +## Pull Request Sequence + +1. **Policy and compatibility harness:** interaction classifier, executor contract, current-behavior regression tests, and stable error categories. +2. **Provider foundation:** provider protocol, Azure CLI parser contract, allowlist, timeout, sanitization, and mocked provider tests. +3. **State and binding:** versioned migration, runtime override model, locking, atomic write, cache, and approved principal validation. +4. **User interaction:** direct chooser and continuation first; attended-host transport only after Q5. +5. **Status and lifecycle:** passive/active status, compatibility output, capability checks, and source-local logout. +6. **Integration surfaces:** HTTP, SDK bridge, deploy, batch, claims, and Power BI route regression. +7. **Release hardening:** platform matrix, docs, telemetry, Skills pilot, flags, and rollback. + +Each pull request should link to its ADO task and update the requirement/test evidence ledger. + +## Validation Matrix + +| Layer | Required coverage | +| --- | --- | +| Parser | New flags, conflicts, required tenant/no-prompt combinations, audience allowlist | +| Policy | Direct terminal, approved attended host, undeclared host, JSON, pipe, batch, CI, callback, status, logout | +| Provider | Installed/signed-in states, timeout, sanitized error, audience mapping, refresh, concurrency, drift | +| State | Migration, permissions, atomicity, lock contention, failed replacement, environment precedence | +| Execution | CLI, auth commands, REPL, batch fail-fast, exactly-once continuation | +| Integration | HTTP routes, SDK credential, deploy preflight, user capabilities, claims handling | +| Regression | MSAL user, SPN secret/certificate/federation, managed identity, raw tokens | +| Release | Windows/Linux/macOS, guest/no-subscription, Skills pilot, feature gates, rollback | + +## Task Creation Readiness + +Before creating ADO work items: + +- The Feature Registry artifacts must be present on its default branch. +- This repo's `Features/1694265/design-spec.md` must be reviewed and merged to the code repo's default branch. +- The ADO organization, project, hierarchy, area path, iteration, and owners must be confirmed. +- Existing closed Task 1728448 remains decision history and is not reused. + +After the readiness gate passes, create the seven work items through the `manage-tasks` workflow so each ADO ID is used to generate its local `task-.md` file. diff --git a/Features/1694265/registry.md b/Features/1694265/registry.md new file mode 100644 index 00000000..68458234 --- /dev/null +++ b/Features/1694265/registry.md @@ -0,0 +1,20 @@ +# Feature 1694265 — Registry Pointer + +This folder contains area-specific artifacts for Feature 1694265 in the **fabric-cli** repo. + +## Feature Registry (Parent) + +- **Registry Repo:** [fabric-cli](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry) +- **Feature Folder:** `Features/active/1694265/` (moves to `Features/done/1694265/` when archived) +- **ADO Work Item:** [#1694265](https://powerbi@dev.azure.com/powerbi/Trident/_workitems/edit/1694265) *(stable canonical link)* + +> **Note:** The Feature Registry uses an `active/done` folder hierarchy. After a feature is archived, the folder path changes from `Features/active/` to `Features/done/`. Use the ADO Work Item link above as the stable reference. + +## Navigation + +> **Tip:** These links point to the `active/` path and will break after archival. Use the ADO Work Item link above to find the feature regardless of its current folder. + +- [Requirements Spec](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/requirements-spec.md) (in Feature Registry — active only) +- [Engineering Design](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/engineering-design.md) (in Feature Registry — active only) +- [Implementation Handoff](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/implementation-handoff.md) (in Feature Registry — active only) +- [Test Plan](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/test-plan.md) (in Feature Registry — active only) From 922e5a949ea8957a1d594309de7ea795bcd183c4 Mon Sep 17 00:00:00 2001 From: Alon Yeshurun Date: Wed, 26 Aug 2026 20:19:22 +0300 Subject: [PATCH 02/10] fix: restore explicit usage for job run-update parser `fab job run-update --help` raised an AssertionError from inside argparse instead of printing help. It was the only leaf parser that did not set an explicit `usage`, so argparse generated and wrapped one itself. Its `--id` flag is `required=True` with `metavar=""`, which produces a double space in the generated usage; argparse's `_format_usage` re-splits that string with a regex and asserts the result round-trips, which it does not (Python <= 3.12). Also corrects the `--disabled` example flag to `--disable`, which is the flag the parser actually defines. Resolves #277 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../unreleased/fixed-20260826-192501.yaml | 7 ++ src/fabric_cli/parsers/fab_jobs_parser.py | 3 +- tests/test_parsers/test_fab_parser_help.py | 115 ++++++++++++++++++ 3 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 .changes/unreleased/fixed-20260826-192501.yaml create mode 100644 tests/test_parsers/test_fab_parser_help.py diff --git a/.changes/unreleased/fixed-20260826-192501.yaml b/.changes/unreleased/fixed-20260826-192501.yaml new file mode 100644 index 00000000..6ee72d1d --- /dev/null +++ b/.changes/unreleased/fixed-20260826-192501.yaml @@ -0,0 +1,7 @@ +kind: fixed +body: Fixed `job run-update --help` crashing with an `AssertionError` instead of printing + help +time: 2026-08-26T19:25:01.000000000+03:00 +custom: + Author: ayeshurun + AuthorLink: https://github.com/ayeshurun diff --git a/src/fabric_cli/parsers/fab_jobs_parser.py b/src/fabric_cli/parsers/fab_jobs_parser.py index c3c333ba..3b835028 100644 --- a/src/fabric_cli/parsers/fab_jobs_parser.py +++ b/src/fabric_cli/parsers/fab_jobs_parser.py @@ -221,7 +221,7 @@ def register_parser(subparsers: _SubParsersAction) -> None: # Subcommand for 'run_update' update_examples = [ "# disable pipeline schedule", - "$ job run-update pip1.DataPipeline --id --disabled \n", + "$ job run-update pip1.DataPipeline --id --disable \n", "# update pipeline schedule to run every 10 minutes and enable it", "$ job run-update pip1.DataPipeline --id --type cron --interval 10 --start 2024-11-15T09:00:00 --end 2024-12-15T10:00:00 --enable \n", "# update pipeline schedule to run every day at 10:00 and 16:00 (maintain the existing enabled state)", @@ -276,6 +276,7 @@ def register_parser(subparsers: _SubParsersAction) -> None: run_update_parser.add_argument( "--days", metavar="", help="Days of the week. Optional" ) + run_update_parser.usage = f"{utils_error_parser.get_usage_prog(run_update_parser)}" run_update_parser.set_defaults(func=lazy_command(_jobs_module_path, 'run_update_command')) # Subcommand for 'run_rm' diff --git a/tests/test_parsers/test_fab_parser_help.py b/tests/test_parsers/test_fab_parser_help.py new file mode 100644 index 00000000..f147519f --- /dev/null +++ b/tests/test_parsers/test_fab_parser_help.py @@ -0,0 +1,115 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""Tests that every registered parser can render its help output.""" + +import argparse +from typing import Iterator, Tuple + +import pytest + +from fabric_cli.core import fab_parser_setup + +# Flags injected into every parser by `fab_global_params.add_global_flags`. +_GLOBAL_FLAG_DESTS = {"help", "output_format"} + + +def _walk_parsers( + parser: argparse.ArgumentParser, prog: str +) -> Iterator[Tuple[str, argparse.ArgumentParser, bool]]: + """Yield (command, parser, is_group) for a parser and all of its subparsers.""" + subparser_actions = [ + action + for action in parser._actions + if isinstance(action, argparse._SubParsersAction) + ] + yield prog, parser, bool(subparser_actions) + + for action in subparser_actions: + visited: set[int] = set() + for name, subparser in action.choices.items(): + # Aliases point at the same parser instance; only walk it once. + if id(subparser) in visited: + continue + visited.add(id(subparser)) + yield from _walk_parsers(subparser, f"{prog} {name}") + + +def _all_parsers() -> list[Tuple[str, argparse.ArgumentParser, bool]]: + parser, _ = fab_parser_setup.create_parser_and_subparsers() + return list(_walk_parsers(parser, "fab")) + + +def _declares_own_arguments(parser: argparse.ArgumentParser) -> bool: + for action in parser._actions: + if isinstance(action, argparse._SubParsersAction): + continue + if action.dest in _GLOBAL_FLAG_DESTS: + continue + return True + return False + + +_PARSERS = _all_parsers() + + +@pytest.fixture +def narrow_terminal(monkeypatch): + """Force argparse to wrap usage lines. + + `argparse.HelpFormatter` derives its width from `shutil.get_terminal_size()`, + which honours `COLUMNS`. The #277 crash only happens on the wrapping path, so a + wide test terminal would hide it. + """ + monkeypatch.setenv("COLUMNS", "60") + + +@pytest.mark.parametrize( + "command, parser", + [(command, parser) for command, parser, _ in _PARSERS], + ids=[command for command, _, _ in _PARSERS], +) +def test_format_help_does_not_raise(command, parser, narrow_terminal): + """` --help` must render instead of crashing. + + Regression test for #277: `job run-update` had no explicit `usage`, so argparse + built and wrapped one itself. A required flag with an empty metavar produced a + double space in the usage string, tripping the internal assertion in + `argparse.HelpFormatter._format_usage` (Python <= 3.12). + """ + assert parser.format_help() + + +@pytest.mark.parametrize( + "command, parser", + [(command, parser) for command, parser, is_group in _PARSERS if not is_group], + ids=[command for command, _, is_group in _PARSERS if not is_group], +) +def test_leaf_parsers_declaring_arguments_set_explicit_usage(command, parser): + """Leaf commands with their own arguments must set `usage` explicitly. + + Relying on argparse's generated usage is what triggered #277. This check is + Python-version independent, unlike the crash itself. + """ + if not _declares_own_arguments(parser): + pytest.skip(f"'{command}' declares no arguments of its own") + + assert parser.usage, f"'{command}' must set an explicit usage string" + + +def _find_parser(command: str) -> argparse.ArgumentParser: + for name, parser, _ in _PARSERS: + if name == command: + return parser + raise AssertionError(f"parser '{command}' not found") + + +def test_job_run_update_help_lists_flags(): + """Regression test for #277.""" + run_update = _find_parser("fab job run-update") + + help_message = run_update.format_help() + + assert "Usage: job run-update " in help_message + for flag in ("--id", "--input", "--enable", "--disable", "--type", "--days"): + assert flag in help_message From 51e409fc4bed1aaa64cd674182a3de5582c7b169 Mon Sep 17 00:00:00 2001 From: Alon Yeshurun Date: Thu, 27 Aug 2026 12:32:04 +0300 Subject: [PATCH 03/10] test: add type annotations to parser help tests Repo guidance requires type hints on all functions. The fixture and the three test functions were unannotated, so mypy skipped their bodies. Annotating them means mypy now actually checks these bodies; verified still clean. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8fd56781-ee24-4eaf-9221-08e51ce89669 --- tests/test_parsers/test_fab_parser_help.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/test_parsers/test_fab_parser_help.py b/tests/test_parsers/test_fab_parser_help.py index f147519f..357720a8 100644 --- a/tests/test_parsers/test_fab_parser_help.py +++ b/tests/test_parsers/test_fab_parser_help.py @@ -54,7 +54,7 @@ def _declares_own_arguments(parser: argparse.ArgumentParser) -> bool: @pytest.fixture -def narrow_terminal(monkeypatch): +def narrow_terminal(monkeypatch: pytest.MonkeyPatch) -> None: """Force argparse to wrap usage lines. `argparse.HelpFormatter` derives its width from `shutil.get_terminal_size()`, @@ -69,7 +69,9 @@ def narrow_terminal(monkeypatch): [(command, parser) for command, parser, _ in _PARSERS], ids=[command for command, _, _ in _PARSERS], ) -def test_format_help_does_not_raise(command, parser, narrow_terminal): +def test_format_help_does_not_raise( + command: str, parser: argparse.ArgumentParser, narrow_terminal: None +) -> None: """` --help` must render instead of crashing. Regression test for #277: `job run-update` had no explicit `usage`, so argparse @@ -85,7 +87,9 @@ def test_format_help_does_not_raise(command, parser, narrow_terminal): [(command, parser) for command, parser, is_group in _PARSERS if not is_group], ids=[command for command, _, is_group in _PARSERS if not is_group], ) -def test_leaf_parsers_declaring_arguments_set_explicit_usage(command, parser): +def test_leaf_parsers_declaring_arguments_set_explicit_usage( + command: str, parser: argparse.ArgumentParser +) -> None: """Leaf commands with their own arguments must set `usage` explicitly. Relying on argparse's generated usage is what triggered #277. This check is @@ -104,7 +108,7 @@ def _find_parser(command: str) -> argparse.ArgumentParser: raise AssertionError(f"parser '{command}' not found") -def test_job_run_update_help_lists_flags(): +def test_job_run_update_help_lists_flags() -> None: """Regression test for #277.""" run_update = _find_parser("fab job run-update") From 0c6f2966f3bcbf4e8bc053f1e1067e2caab11b1e Mon Sep 17 00:00:00 2001 From: Alon Yeshurun Date: Thu, 27 Aug 2026 12:46:30 +0300 Subject: [PATCH 04/10] fix: render required flags unbracketed in usage strings `get_usage_prog` wrapped every optional action in brackets regardless of `action.required`, so required flags such as `job run-update --id` were displayed as `[--id]` even though argparse rejects their omission. Brackets denote optionality, so required flags are now left unbracketed. This affects 18 required flags across 17 commands, all of which were previously mis-rendered as optional. Also corrects docs/commands/jobs/index.md, which bracketed the required `--id` for run-cancel/run-status/run-update/run-rm and omitted --interval/--start/--end/--days from the run-update synopsis. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8fd56781-ee24-4eaf-9221-08e51ce89669 --- docs/commands/jobs/index.md | 8 ++++---- src/fabric_cli/utils/fab_error_parser.py | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/commands/jobs/index.md b/docs/commands/jobs/index.md index c434ca90..198fcc75 100644 --- a/docs/commands/jobs/index.md +++ b/docs/commands/jobs/index.md @@ -15,12 +15,12 @@ The `job` commands provide tools for starting, running, monitoring, and scheduli |-----------------|---------------------------|-----------------------------------------------------------------------| | `job start` | Start an item (async) | `job start [-P ] [-C ] [-i ]` | | `job run` | Run an item (sync) | `job run [-P ] [-C ] [-i ] [--timeout ]` | -| `job run-cancel`| Cancel an item run | `job run-cancel [--id ] [--wait]` | +| `job run-cancel`| Cancel an item run | `job run-cancel --id [--wait]` | | `job run-list` | List item or scheduled job runs | `job run-list [--schedule]` | -| `job run-status`| Get job run details | `job run-status [--id ] [--schedule]` | +| `job run-status`| Get job run details | `job run-status --id [--schedule]` | | `job run-sch` | Schedule a job | `job run-sch [-i *] [--type ] [--interval ] [--days ]` | -| `job run-update`| Update a scheduled job | `job run-update [--id ] [-i ] [--type ] [--enable/--disable]` | -| `job run-rm` | Delete a scheduled job | `job run-rm [--id ] [--force]` | +| `job run-update`| Update a scheduled job | `job run-update --id [-i ] [--type ] [--interval ] [--start ] [--end ] [--days ] [--enable/--disable]` | +| `job run-rm` | Delete a scheduled job | `job run-rm --id [--force]` | --- diff --git a/src/fabric_cli/utils/fab_error_parser.py b/src/fabric_cli/utils/fab_error_parser.py index 74c3cac4..0cbfebf2 100644 --- a/src/fabric_cli/utils/fab_error_parser.py +++ b/src/fabric_cli/utils/fab_error_parser.py @@ -74,9 +74,10 @@ def get_usage_prog(parser: argparse.ArgumentParser) -> str: # Collect positional arguments in `<...>` pos_args = [f"<{arg.dest}>" for arg in parser._get_positional_actions()] - # Collect optional (flag) arguments in `[...]` + # Collect optional (flag) arguments in `[...]`. Required flags are left + # unbracketed, since brackets denote optionality. opt_args = [ - f"[{arg.option_strings[0]}]" + (arg.option_strings[0] if arg.required else f"[{arg.option_strings[0]}]") for arg in parser._get_optional_actions() if arg.option_strings ] From e650bff0b71ecd339631167688802e332ce683a1 Mon Sep 17 00:00:00 2001 From: Alon Yeshurun Date: Thu, 27 Aug 2026 12:46:30 +0300 Subject: [PATCH 05/10] fix: correct run-sch help example and harden help regression tests The `job run-sch --help` examples included an entry that invoked `job run-update --id ` instead of `job run-sch`, showing users the wrong command (and a flag run-sch does not accept). Test hardening: - add a test driving the real `parse_args(["job", "run-update", "--help"])` dispatch path, rather than only calling `format_help()` directly; this also fails without the crash fix, so it genuinely covers #277 - assert required `--id` renders unbracketed while optional flags stay bracketed - complete the flag assertion, which previously omitted --interval, --start and --end Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8fd56781-ee24-4eaf-9221-08e51ce89669 --- src/fabric_cli/parsers/fab_jobs_parser.py | 2 +- tests/test_parsers/test_fab_parser_help.py | 47 +++++++++++++++++++++- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/fabric_cli/parsers/fab_jobs_parser.py b/src/fabric_cli/parsers/fab_jobs_parser.py index 3b835028..f7343497 100644 --- a/src/fabric_cli/parsers/fab_jobs_parser.py +++ b/src/fabric_cli/parsers/fab_jobs_parser.py @@ -176,7 +176,7 @@ def register_parser(subparsers: _SubParsersAction) -> None: "# set up pipeline schedule to run every 10 minutes and enable it", "$ job run-sch pip1.DataPipeline --type cron --interval 10 --start 2024-11-15T09:00:00 --end 2024-12-15T10:00:00 --enable \n", "# set up pipeline schedule to run every day at 10:00 and 16:00 (disabled by default)", - "$ job run-update pip1.DataPipeline --id --type daily --interval 10:00,16:00 --start 2024-11-15T09:00:00 --end 2024-12-16T10:00:00 \n", + "$ job run-sch pip1.DataPipeline --type daily --interval 10:00,16:00 --start 2024-11-15T09:00:00 --end 2024-12-16T10:00:00 \n", "# set up pipeline schedule to run every Monday and Friday at 10:00 and 16:00, disabled by default", "$ job run-sch pip1.DataPipeline --type weekly --interval 10:00,16:00 --days Monday,Friday --start 2024-11-15T09:00:00 --end 2024-12-16T10:00:00 \n", "# set up pipeline schedule with custom input", diff --git a/tests/test_parsers/test_fab_parser_help.py b/tests/test_parsers/test_fab_parser_help.py index 357720a8..b5afd44b 100644 --- a/tests/test_parsers/test_fab_parser_help.py +++ b/tests/test_parsers/test_fab_parser_help.py @@ -108,6 +108,20 @@ def _find_parser(command: str) -> argparse.ArgumentParser: raise AssertionError(f"parser '{command}' not found") +# Every flag `job run-update` defines itself, excluding global flags. +_RUN_UPDATE_FLAGS = ( + "--id", + "--input", + "--enable", + "--disable", + "--type", + "--interval", + "--start", + "--end", + "--days", +) + + def test_job_run_update_help_lists_flags() -> None: """Regression test for #277.""" run_update = _find_parser("fab job run-update") @@ -115,5 +129,34 @@ def test_job_run_update_help_lists_flags() -> None: help_message = run_update.format_help() assert "Usage: job run-update " in help_message - for flag in ("--id", "--input", "--enable", "--disable", "--type", "--days"): - assert flag in help_message + for flag in _RUN_UPDATE_FLAGS: + assert flag in help_message, f"'{flag}' missing from help" + + +def test_required_flags_are_not_bracketed_in_usage() -> None: + """Brackets denote optionality, so a required flag must not be bracketed.""" + run_update = _find_parser("fab job run-update") + + usage_line = run_update.format_help().splitlines()[0] + + assert "--id" in usage_line + assert "[--id]" not in usage_line, "required '--id' must not look optional" + # A genuinely optional flag on the same command stays bracketed. + assert "[--enable]" in usage_line + + +def test_help_flag_renders_through_real_dispatch( + capsys: pytest.CaptureFixture[str], narrow_terminal: None +) -> None: + """`fab job run-update --help` must exit 0 and print help. + + The other tests call `format_help()` directly; this drives the actual + `parse_args` -> `print_help` path a user hits from the command line. + """ + root, _ = fab_parser_setup.create_parser_and_subparsers() + + with pytest.raises(SystemExit) as excinfo: + root.parse_args(["job", "run-update", "--help"]) + + assert excinfo.value.code == 0 + assert "job run-update" in capsys.readouterr().out From b133e9c42dd063df0f1e4e445f88771427c0e9da Mon Sep 17 00:00:00 2001 From: Alon Yeshurun Date: Thu, 27 Aug 2026 12:48:53 +0300 Subject: [PATCH 06/10] docs: complete run-update flag list in AI skill reference The reference syntax omitted --start, --end and --days, matching the gap just corrected in docs/commands/jobs/index.md. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8fd56781-ee24-4eaf-9221-08e51ce89669 --- .ai-assets/skills/fabric-cli-core/references/reference.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.ai-assets/skills/fabric-cli-core/references/reference.md b/.ai-assets/skills/fabric-cli-core/references/reference.md index a575c80a..91d8cbec 100644 --- a/.ai-assets/skills/fabric-cli-core/references/reference.md +++ b/.ai-assets/skills/fabric-cli-core/references/reference.md @@ -680,7 +680,7 @@ fab job run-sch "Production.Workspace/Pipeline.DataPipeline" -i '{"enabled": tru #### Syntax ```bash -fab job run-update --id [--type ] [--interval ] [--enable] [--disable] [-i ] +fab job run-update --id [--type ] [--interval ] [--start ] [--end ] [--days ] [--enable] [--disable] [-i ] ``` #### Examples From e9b64e90e0cd2114f1613e91471fe7931f46387d Mon Sep 17 00:00:00 2001 From: Alon Yeshurun Date: Thu, 27 Aug 2026 13:15:49 +0300 Subject: [PATCH 07/10] fix: drop double space in usage strings and document the crash bypass Address independent review feedback on the job run-update help fix. - get_usage_prog no longer emits a double space for commands without positionals (affected 9 commands, e.g. `deploy`). A double space is precisely what trips argparse's usage round-trip assertion, so leaving one in the shared helper was an avoidable hazard. - Document that assigning parser.usage is load-bearing rather than cosmetic: it diverts argparse away from the asserting usage-wrapping path. Without this note a future cleanup removing "redundant" usage assignments would silently reintroduce the crash on Python <= 3.12. - Add direct unit tests for get_usage_prog, which had 49 call sites and no direct coverage. 8 of 9 fail against the previous implementation. - Disclose the user-visible usage change in the changelog: required flags now render unbracketed across 16 commands. - Docs: add the missing run-update section, drop a stray `*`, and replace the invented `[--enable/--disable]` notation, which implied a mutual exclusivity that does not exist. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8fd56781-ee24-4eaf-9221-08e51ce89669 --- .../unreleased/fixed-20260827-130500.yaml | 10 +++ docs/commands/jobs/index.md | 29 ++++++- src/fabric_cli/parsers/fab_jobs_parser.py | 2 + src/fabric_cli/utils/fab_error_parser.py | 13 ++- tests/test_utils/test_fab_error_parser.py | 80 +++++++++++++++++++ 5 files changed, 130 insertions(+), 4 deletions(-) create mode 100644 .changes/unreleased/fixed-20260827-130500.yaml create mode 100644 tests/test_utils/test_fab_error_parser.py diff --git a/.changes/unreleased/fixed-20260827-130500.yaml b/.changes/unreleased/fixed-20260827-130500.yaml new file mode 100644 index 00000000..6859c527 --- /dev/null +++ b/.changes/unreleased/fixed-20260827-130500.yaml @@ -0,0 +1,10 @@ +kind: fixed +body: Required flags are no longer shown bracketed (as if optional) in `--help` usage + lines, matching the convention already used in the documentation. Affects `export`, + `bulk-export`, `import`, `deploy`, `set`, `ln`, `assign`, `unassign`, `table load`, + `acl rm`, `acl set`, `label set`, `job run-cancel`, `job run-status`, `job run-update` + and `job run-rm` +time: 2026-08-27T13:05:00.000000000+03:00 +custom: + Author: ayeshurun + AuthorLink: https://github.com/ayeshurun diff --git a/docs/commands/jobs/index.md b/docs/commands/jobs/index.md index 198fcc75..3ecd3c25 100644 --- a/docs/commands/jobs/index.md +++ b/docs/commands/jobs/index.md @@ -18,8 +18,8 @@ The `job` commands provide tools for starting, running, monitoring, and scheduli | `job run-cancel`| Cancel an item run | `job run-cancel --id [--wait]` | | `job run-list` | List item or scheduled job runs | `job run-list [--schedule]` | | `job run-status`| Get job run details | `job run-status --id [--schedule]` | -| `job run-sch` | Schedule a job | `job run-sch [-i *] [--type ] [--interval ] [--days ]` | -| `job run-update`| Update a scheduled job | `job run-update --id [-i ] [--type ] [--interval ] [--start ] [--end ] [--days ] [--enable/--disable]` | +| `job run-sch` | Schedule a job | `job run-sch [-i ] [--type ] [--interval ] [--days ]` | +| `job run-update`| Update a scheduled job | `job run-update --id [-i ] [--type ] [--interval ] [--start ] [--end ] [--days ] [--enable] [--disable]` | | `job run-rm` | Delete a scheduled job | `job run-rm --id [--force]` | --- @@ -162,6 +162,31 @@ fab job run-cancel --id [--wait] --- +### run-update + +Update an existing scheduled job. + +**Usage:** + +``` +fab job run-update --id [-i ] [--enable] [--disable] [--type ] [--interval ] [--start ] [--end ] [--days ] +``` + +**Parameters:** + +- ``: Path to the resource. +- `--id`: Schedule ID to update. +- `-i, --input`: JSON payload, inline or path. Optional. +- `--enable`: Enable the schedule. Optional. +- `--disable`: Disable the schedule. Optional. +- `--type`: Type of schedule (`cron`, `daily`, `weekly`). Optional. +- `--interval`: Interval in minutes or time list. Optional. +- `--start`: Start date and time in UTC. Optional. +- `--end`: End date and time in UTC. Optional. +- `--days`: Days of the week. Optional. + +--- + ### run-rm Remove a scheduled job. diff --git a/src/fabric_cli/parsers/fab_jobs_parser.py b/src/fabric_cli/parsers/fab_jobs_parser.py index f7343497..ee6f0a96 100644 --- a/src/fabric_cli/parsers/fab_jobs_parser.py +++ b/src/fabric_cli/parsers/fab_jobs_parser.py @@ -276,6 +276,8 @@ def register_parser(subparsers: _SubParsersAction) -> None: run_update_parser.add_argument( "--days", metavar="", help="Days of the week. Optional" ) + # Required: an explicit usage string keeps argparse out of its own + # usage-wrapping path, which crashes on this parser's `metavar=""` args. run_update_parser.usage = f"{utils_error_parser.get_usage_prog(run_update_parser)}" run_update_parser.set_defaults(func=lazy_command(_jobs_module_path, 'run_update_command')) diff --git a/src/fabric_cli/utils/fab_error_parser.py b/src/fabric_cli/utils/fab_error_parser.py index 0cbfebf2..a154d05a 100644 --- a/src/fabric_cli/utils/fab_error_parser.py +++ b/src/fabric_cli/utils/fab_error_parser.py @@ -67,6 +67,13 @@ def invalid_for_command_line_mode() -> None: def get_usage_prog(parser: argparse.ArgumentParser) -> str: + """Build an explicit usage string for a parser. + + Assigning the result to `parser.usage` is load-bearing, not cosmetic: it + diverts argparse away from generating and wrapping its own usage line. + That generation path asserts on a round-trip re-split which fails for + arguments declared with `metavar=""`, crashing help on Python <= 3.12. + """ # Removes 'fab' from %(prog)s # Start with the command (like "acl ls" or "acl dir") command_part = " ".join(parser.prog.split()[1:]) @@ -82,8 +89,10 @@ def get_usage_prog(parser: argparse.ArgumentParser) -> str: if arg.option_strings ] - # Combine parts for the final usage string - return f"{command_part} {' '.join(pos_args)} {' '.join(opt_args)}" + # Combine parts for the final usage string. Empty sections are dropped so + # commands without positionals don't render a double space. + sections = [command_part, " ".join(pos_args), " ".join(opt_args)] + return " ".join(section for section in sections if section) def map_http_status_code_to_error_code(status_code: int) -> str: diff --git a/tests/test_utils/test_fab_error_parser.py b/tests/test_utils/test_fab_error_parser.py new file mode 100644 index 00000000..280e60c0 --- /dev/null +++ b/tests/test_utils/test_fab_error_parser.py @@ -0,0 +1,80 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""Tests for the usage-string helper shared by every parser.""" + +import argparse + +import pytest + +from fabric_cli.utils import fab_error_parser as utils_error_parser + + +def _parser(prog: str = "fab demo") -> argparse.ArgumentParser: + """Build a bare parser without argparse's implicit help flag.""" + return argparse.ArgumentParser(prog=prog, add_help=False) + + +def test_required_flags_render_unbracketed() -> None: + parser = _parser() + parser.add_argument("--config", metavar="", required=True) + + assert utils_error_parser.get_usage_prog(parser) == "demo --config" + + +def test_optional_flags_render_bracketed() -> None: + parser = _parser() + parser.add_argument("--force", metavar="", required=False) + + assert utils_error_parser.get_usage_prog(parser) == "demo [--force]" + + +def test_short_form_is_preferred_when_declared_first() -> None: + """`option_strings[0]` wins, so declaration order decides the rendering.""" + parser = _parser() + parser.add_argument("-o", "--output", metavar="", required=True) + parser.add_argument("--format", "-F", metavar="", required=False) + + assert utils_error_parser.get_usage_prog(parser) == "demo -o [--format]" + + +def test_positionals_precede_flags() -> None: + parser = _parser() + parser.add_argument("path") + parser.add_argument("-f", metavar="", required=False) + + assert utils_error_parser.get_usage_prog(parser) == "demo [-f]" + + +def test_no_positionals_does_not_produce_a_double_space() -> None: + """A double space is exactly what breaks argparse's usage round-trip.""" + parser = _parser() + parser.add_argument("--config", metavar="", required=True) + + usage = utils_error_parser.get_usage_prog(parser) + + assert " " not in usage + + +def test_parser_without_arguments_renders_only_the_command() -> None: + assert utils_error_parser.get_usage_prog(_parser("fab auth status")) == ( + "auth status" + ) + + +@pytest.mark.parametrize( + "prog, expected", + [("fab demo", "demo"), ("fab job run-update", "job run-update")], +) +def test_root_program_name_is_stripped(prog: str, expected: str) -> None: + assert utils_error_parser.get_usage_prog(_parser(prog)) == expected + + +def test_mixed_arguments_render_in_declaration_order() -> None: + parser = _parser("fab acl set") + parser.add_argument("path") + parser.add_argument("-I", metavar="", required=True) + parser.add_argument("-R", metavar="", required=True) + parser.add_argument("-f", metavar="", required=False) + + assert utils_error_parser.get_usage_prog(parser) == "acl set -I -R [-f]" From 5366ef1a77f666848e6f3862b015c6316f47e51a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:26:37 +0000 Subject: [PATCH 08/10] chore: address PR review cleanup Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com> --- .../unreleased/fixed-20260827-130500.yaml | 10 -- src/fabric_cli/parsers/fab_jobs_parser.py | 2 - src/fabric_cli/utils/fab_error_parser.py | 9 - tests/test_parsers/test_fab_parser_help.py | 162 ------------------ tests/test_utils/test_fab_error_parser.py | 80 --------- 5 files changed, 263 deletions(-) delete mode 100644 .changes/unreleased/fixed-20260827-130500.yaml delete mode 100644 tests/test_parsers/test_fab_parser_help.py delete mode 100644 tests/test_utils/test_fab_error_parser.py diff --git a/.changes/unreleased/fixed-20260827-130500.yaml b/.changes/unreleased/fixed-20260827-130500.yaml deleted file mode 100644 index 6859c527..00000000 --- a/.changes/unreleased/fixed-20260827-130500.yaml +++ /dev/null @@ -1,10 +0,0 @@ -kind: fixed -body: Required flags are no longer shown bracketed (as if optional) in `--help` usage - lines, matching the convention already used in the documentation. Affects `export`, - `bulk-export`, `import`, `deploy`, `set`, `ln`, `assign`, `unassign`, `table load`, - `acl rm`, `acl set`, `label set`, `job run-cancel`, `job run-status`, `job run-update` - and `job run-rm` -time: 2026-08-27T13:05:00.000000000+03:00 -custom: - Author: ayeshurun - AuthorLink: https://github.com/ayeshurun diff --git a/src/fabric_cli/parsers/fab_jobs_parser.py b/src/fabric_cli/parsers/fab_jobs_parser.py index ee6f0a96..f7343497 100644 --- a/src/fabric_cli/parsers/fab_jobs_parser.py +++ b/src/fabric_cli/parsers/fab_jobs_parser.py @@ -276,8 +276,6 @@ def register_parser(subparsers: _SubParsersAction) -> None: run_update_parser.add_argument( "--days", metavar="", help="Days of the week. Optional" ) - # Required: an explicit usage string keeps argparse out of its own - # usage-wrapping path, which crashes on this parser's `metavar=""` args. run_update_parser.usage = f"{utils_error_parser.get_usage_prog(run_update_parser)}" run_update_parser.set_defaults(func=lazy_command(_jobs_module_path, 'run_update_command')) diff --git a/src/fabric_cli/utils/fab_error_parser.py b/src/fabric_cli/utils/fab_error_parser.py index a154d05a..27bc15a4 100644 --- a/src/fabric_cli/utils/fab_error_parser.py +++ b/src/fabric_cli/utils/fab_error_parser.py @@ -67,13 +67,6 @@ def invalid_for_command_line_mode() -> None: def get_usage_prog(parser: argparse.ArgumentParser) -> str: - """Build an explicit usage string for a parser. - - Assigning the result to `parser.usage` is load-bearing, not cosmetic: it - diverts argparse away from generating and wrapping its own usage line. - That generation path asserts on a round-trip re-split which fails for - arguments declared with `metavar=""`, crashing help on Python <= 3.12. - """ # Removes 'fab' from %(prog)s # Start with the command (like "acl ls" or "acl dir") command_part = " ".join(parser.prog.split()[1:]) @@ -89,8 +82,6 @@ def get_usage_prog(parser: argparse.ArgumentParser) -> str: if arg.option_strings ] - # Combine parts for the final usage string. Empty sections are dropped so - # commands without positionals don't render a double space. sections = [command_part, " ".join(pos_args), " ".join(opt_args)] return " ".join(section for section in sections if section) diff --git a/tests/test_parsers/test_fab_parser_help.py b/tests/test_parsers/test_fab_parser_help.py deleted file mode 100644 index b5afd44b..00000000 --- a/tests/test_parsers/test_fab_parser_help.py +++ /dev/null @@ -1,162 +0,0 @@ -# Copyright (c) Microsoft Corporation. -# Licensed under the MIT License. - -"""Tests that every registered parser can render its help output.""" - -import argparse -from typing import Iterator, Tuple - -import pytest - -from fabric_cli.core import fab_parser_setup - -# Flags injected into every parser by `fab_global_params.add_global_flags`. -_GLOBAL_FLAG_DESTS = {"help", "output_format"} - - -def _walk_parsers( - parser: argparse.ArgumentParser, prog: str -) -> Iterator[Tuple[str, argparse.ArgumentParser, bool]]: - """Yield (command, parser, is_group) for a parser and all of its subparsers.""" - subparser_actions = [ - action - for action in parser._actions - if isinstance(action, argparse._SubParsersAction) - ] - yield prog, parser, bool(subparser_actions) - - for action in subparser_actions: - visited: set[int] = set() - for name, subparser in action.choices.items(): - # Aliases point at the same parser instance; only walk it once. - if id(subparser) in visited: - continue - visited.add(id(subparser)) - yield from _walk_parsers(subparser, f"{prog} {name}") - - -def _all_parsers() -> list[Tuple[str, argparse.ArgumentParser, bool]]: - parser, _ = fab_parser_setup.create_parser_and_subparsers() - return list(_walk_parsers(parser, "fab")) - - -def _declares_own_arguments(parser: argparse.ArgumentParser) -> bool: - for action in parser._actions: - if isinstance(action, argparse._SubParsersAction): - continue - if action.dest in _GLOBAL_FLAG_DESTS: - continue - return True - return False - - -_PARSERS = _all_parsers() - - -@pytest.fixture -def narrow_terminal(monkeypatch: pytest.MonkeyPatch) -> None: - """Force argparse to wrap usage lines. - - `argparse.HelpFormatter` derives its width from `shutil.get_terminal_size()`, - which honours `COLUMNS`. The #277 crash only happens on the wrapping path, so a - wide test terminal would hide it. - """ - monkeypatch.setenv("COLUMNS", "60") - - -@pytest.mark.parametrize( - "command, parser", - [(command, parser) for command, parser, _ in _PARSERS], - ids=[command for command, _, _ in _PARSERS], -) -def test_format_help_does_not_raise( - command: str, parser: argparse.ArgumentParser, narrow_terminal: None -) -> None: - """` --help` must render instead of crashing. - - Regression test for #277: `job run-update` had no explicit `usage`, so argparse - built and wrapped one itself. A required flag with an empty metavar produced a - double space in the usage string, tripping the internal assertion in - `argparse.HelpFormatter._format_usage` (Python <= 3.12). - """ - assert parser.format_help() - - -@pytest.mark.parametrize( - "command, parser", - [(command, parser) for command, parser, is_group in _PARSERS if not is_group], - ids=[command for command, _, is_group in _PARSERS if not is_group], -) -def test_leaf_parsers_declaring_arguments_set_explicit_usage( - command: str, parser: argparse.ArgumentParser -) -> None: - """Leaf commands with their own arguments must set `usage` explicitly. - - Relying on argparse's generated usage is what triggered #277. This check is - Python-version independent, unlike the crash itself. - """ - if not _declares_own_arguments(parser): - pytest.skip(f"'{command}' declares no arguments of its own") - - assert parser.usage, f"'{command}' must set an explicit usage string" - - -def _find_parser(command: str) -> argparse.ArgumentParser: - for name, parser, _ in _PARSERS: - if name == command: - return parser - raise AssertionError(f"parser '{command}' not found") - - -# Every flag `job run-update` defines itself, excluding global flags. -_RUN_UPDATE_FLAGS = ( - "--id", - "--input", - "--enable", - "--disable", - "--type", - "--interval", - "--start", - "--end", - "--days", -) - - -def test_job_run_update_help_lists_flags() -> None: - """Regression test for #277.""" - run_update = _find_parser("fab job run-update") - - help_message = run_update.format_help() - - assert "Usage: job run-update " in help_message - for flag in _RUN_UPDATE_FLAGS: - assert flag in help_message, f"'{flag}' missing from help" - - -def test_required_flags_are_not_bracketed_in_usage() -> None: - """Brackets denote optionality, so a required flag must not be bracketed.""" - run_update = _find_parser("fab job run-update") - - usage_line = run_update.format_help().splitlines()[0] - - assert "--id" in usage_line - assert "[--id]" not in usage_line, "required '--id' must not look optional" - # A genuinely optional flag on the same command stays bracketed. - assert "[--enable]" in usage_line - - -def test_help_flag_renders_through_real_dispatch( - capsys: pytest.CaptureFixture[str], narrow_terminal: None -) -> None: - """`fab job run-update --help` must exit 0 and print help. - - The other tests call `format_help()` directly; this drives the actual - `parse_args` -> `print_help` path a user hits from the command line. - """ - root, _ = fab_parser_setup.create_parser_and_subparsers() - - with pytest.raises(SystemExit) as excinfo: - root.parse_args(["job", "run-update", "--help"]) - - assert excinfo.value.code == 0 - assert "job run-update" in capsys.readouterr().out diff --git a/tests/test_utils/test_fab_error_parser.py b/tests/test_utils/test_fab_error_parser.py deleted file mode 100644 index 280e60c0..00000000 --- a/tests/test_utils/test_fab_error_parser.py +++ /dev/null @@ -1,80 +0,0 @@ -# Copyright (c) Microsoft Corporation. -# Licensed under the MIT License. - -"""Tests for the usage-string helper shared by every parser.""" - -import argparse - -import pytest - -from fabric_cli.utils import fab_error_parser as utils_error_parser - - -def _parser(prog: str = "fab demo") -> argparse.ArgumentParser: - """Build a bare parser without argparse's implicit help flag.""" - return argparse.ArgumentParser(prog=prog, add_help=False) - - -def test_required_flags_render_unbracketed() -> None: - parser = _parser() - parser.add_argument("--config", metavar="", required=True) - - assert utils_error_parser.get_usage_prog(parser) == "demo --config" - - -def test_optional_flags_render_bracketed() -> None: - parser = _parser() - parser.add_argument("--force", metavar="", required=False) - - assert utils_error_parser.get_usage_prog(parser) == "demo [--force]" - - -def test_short_form_is_preferred_when_declared_first() -> None: - """`option_strings[0]` wins, so declaration order decides the rendering.""" - parser = _parser() - parser.add_argument("-o", "--output", metavar="", required=True) - parser.add_argument("--format", "-F", metavar="", required=False) - - assert utils_error_parser.get_usage_prog(parser) == "demo -o [--format]" - - -def test_positionals_precede_flags() -> None: - parser = _parser() - parser.add_argument("path") - parser.add_argument("-f", metavar="", required=False) - - assert utils_error_parser.get_usage_prog(parser) == "demo [-f]" - - -def test_no_positionals_does_not_produce_a_double_space() -> None: - """A double space is exactly what breaks argparse's usage round-trip.""" - parser = _parser() - parser.add_argument("--config", metavar="", required=True) - - usage = utils_error_parser.get_usage_prog(parser) - - assert " " not in usage - - -def test_parser_without_arguments_renders_only_the_command() -> None: - assert utils_error_parser.get_usage_prog(_parser("fab auth status")) == ( - "auth status" - ) - - -@pytest.mark.parametrize( - "prog, expected", - [("fab demo", "demo"), ("fab job run-update", "job run-update")], -) -def test_root_program_name_is_stripped(prog: str, expected: str) -> None: - assert utils_error_parser.get_usage_prog(_parser(prog)) == expected - - -def test_mixed_arguments_render_in_declaration_order() -> None: - parser = _parser("fab acl set") - parser.add_argument("path") - parser.add_argument("-I", metavar="", required=True) - parser.add_argument("-R", metavar="", required=True) - parser.add_argument("-f", metavar="", required=False) - - assert utils_error_parser.get_usage_prog(parser) == "acl set -I -R [-f]" From 1956c51d1334490a448d3959c9ff691d4259820e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 29 Aug 2026 04:31:17 +0000 Subject: [PATCH 09/10] fix: revert usage helper change Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com> --- src/fabric_cli/utils/fab_error_parser.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/fabric_cli/utils/fab_error_parser.py b/src/fabric_cli/utils/fab_error_parser.py index 27bc15a4..74c3cac4 100644 --- a/src/fabric_cli/utils/fab_error_parser.py +++ b/src/fabric_cli/utils/fab_error_parser.py @@ -74,16 +74,15 @@ def get_usage_prog(parser: argparse.ArgumentParser) -> str: # Collect positional arguments in `<...>` pos_args = [f"<{arg.dest}>" for arg in parser._get_positional_actions()] - # Collect optional (flag) arguments in `[...]`. Required flags are left - # unbracketed, since brackets denote optionality. + # Collect optional (flag) arguments in `[...]` opt_args = [ - (arg.option_strings[0] if arg.required else f"[{arg.option_strings[0]}]") + f"[{arg.option_strings[0]}]" for arg in parser._get_optional_actions() if arg.option_strings ] - sections = [command_part, " ".join(pos_args), " ".join(opt_args)] - return " ".join(section for section in sections if section) + # Combine parts for the final usage string + return f"{command_part} {' '.join(pos_args)} {' '.join(opt_args)}" def map_http_status_code_to_error_code(status_code: int) -> str: From 1b814ea0575e216f84e2e3ae54aeb80198262812 Mon Sep 17 00:00:00 2001 From: Alon Yeshurun Date: Sun, 30 Aug 2026 12:03:01 +0300 Subject: [PATCH 10/10] Remove --- .github/hooks/hooks.json | 15 -- Features/1694265/design-spec.md | 229 ------------------------ Features/1694265/implementation-plan.md | 71 -------- Features/1694265/registry.md | 20 --- 4 files changed, 335 deletions(-) delete mode 100644 .github/hooks/hooks.json delete mode 100644 Features/1694265/design-spec.md delete mode 100644 Features/1694265/implementation-plan.md delete mode 100644 Features/1694265/registry.md diff --git a/.github/hooks/hooks.json b/.github/hooks/hooks.json deleted file mode 100644 index f94d97b6..00000000 --- a/.github/hooks/hooks.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "version": 1, - "hooks": { - "sessionStart": [ - { - "type": "command", - "bash": "b=\"${HOME:-${USERPROFILE:-}}/.copilot/hooks\"; s=\"$b/.last-copilot-toolkit-plugin-update.$(date -u +%Y%m%d)\"; if mkdir -p \"$b\" 2>/dev/null && ( set -C; : > \"$s\" ) 2>/dev/null; then copilot plugin install https://dev.azure.com/msdata/A365/_git/copilot-toolkit 2>/dev/null || echo 'Plugin already installed or unavailable' >&2; fi", - "powershell": "try { $b = Join-Path $HOME '.copilot/hooks'; New-Item -ItemType Directory -Force -Path $b -ErrorAction Stop | Out-Null; $s = Join-Path $b ('.last-copilot-toolkit-plugin-update.' + [DateTime]::UtcNow.ToString('yyyyMMdd')); $fs = [System.IO.File]::Open($s, [System.IO.FileMode]::CreateNew); $fs.Close(); copilot plugin install https://dev.azure.com/msdata/A365/_git/copilot-toolkit 2>$null; if ($LASTEXITCODE -ne 0) { Write-Host 'Plugin already installed or unavailable' -ForegroundColor Yellow } } catch { }; exit 0", - "cwd": ".", - "timeoutSec": 120, - "comment": "Install/update Copilot Toolkit plugins at most once per day (daily stamp guard) to avoid Azure DevOps throttling" - } - ] - } -} diff --git a/Features/1694265/design-spec.md b/Features/1694265/design-spec.md deleted file mode 100644 index c6a33752..00000000 --- a/Features/1694265/design-spec.md +++ /dev/null @@ -1,229 +0,0 @@ -# Design Spec — Feature 1694265 (fabric-cli) - -> Repo-specific design for adding Azure CLI as an explicit Fabric CLI authentication source. -> The parent [engineering design](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/engineering-design.md), [implementation handoff](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/implementation-handoff.md), and [test plan](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/test-plan.md) define the product contract. - -## Scope - -Fabric CLI owns the complete client-side implementation: - -- Add `azure-cli` as an explicit authentication source while preserving all existing direct Fabric CLI sources. -- Introduce a shared pre-dispatch authentication coordinator for command-line, batch, auth, and REPL execution. -- Acquire Azure CLI tokens noninteractively through `AzureCliCredential`. -- Persist a versioned source and identity binding, but never persist delegated Azure CLI tokens. -- Add passive and active authentication status, stable error/exit behavior, source-local logout, and source-aware SDK/deploy integration. -- Preserve the existing route labels and scopes: `fabric` and `powerbi` use the Fabric scope, `storage` uses the OneLake scope, and `azure` uses the ARM scope. -- Add feature gating, telemetry, documentation, and the cross-platform regression matrix required for rollout. - -### Non-goals - -- Running or wrapping `az login`, `az logout`, `az account set`, or any Azure CLI context mutation. -- Using `DefaultAzureCredential` or merging Azure CLI tokens into the MSAL cache. -- Persisting Azure CLI access or refresh tokens. -- Supporting arbitrary scopes, claims challenges, SQL, XMLA, Kusto, or non-Public Azure clouds in the first release. -- Normalizing every existing identity mode under the new `--source` syntax. - -## Current Architecture - -Authentication currently spans several independent paths: - -- `fabric_cli.main` special-cases auth commands and uses `_execute_command` only for other one-shot commands. -- `InteractiveCLI.handle_command` parses and invokes handlers independently. -- `FabAuth` combines persistent state, environment loading, provider selection, MSAL acquisition, and interactive renewal. -- `fab_api_client.do_request` acquires tokens during request execution, which can trigger interactive renewal. -- `fab_auth.status` requests three tokens, and `FabAuth.logout` resets unrelated configuration. -- `MsalTokenCredential` supports only the current Fabric CLI provider and remains headless. -- Config-file deploy creates the credential inside a catch-all that maps failures to `DeploymentFailed`. - -The implementation must separate policy, coordination, provider behavior, and persistence without regressing existing authentication modes. - -## Proposed Design - -### Shared parsed-command executor - -Create one executor used by one-shot, batch, auth, and REPL surfaces. It will: - -1. Classify the parsed command as local, passive auth, active auth, or authenticated. -2. Resolve interaction policy from command flags, output mode, host capability, CI/batch/pipe context, and `FAB_INTERACTION`. -3. Resolve the effective source using runtime environment overrides before the configured source. -4. Run authentication readiness before handler dispatch when required. -5. Invoke the parsed handler at most once and return its exit code without replay. - -Local commands such as help, version, passive status, and logout bypass token acquisition. Batch execution fails fast and reports executed, failed, and skipped counts. - -### Authentication coordinator - -Add a coordinator responsible for source resolution, interaction eligibility, chooser orchestration, candidate validation, atomic binding, and exactly-once continuation. Provider classes remain noninteractive. - -Source precedence: - -1. Runtime environment credentials for the current process. -2. Persisted configured source. -3. Shared chooser only when no source exists and interaction is allowed. -4. `AuthenticationRequired` for unattended or deferred execution. - -The coordinator exposes a small result model containing configured source, effective source, principal capability, readiness state, and optional checked-audience expiration. It does not expose token values. - -### Provider boundary - -Define a provider protocol used by `FabAuth`, HTTP requests, status checks, and the SDK bridge: - -- Acquire exactly one allowlisted audience. -- Return token and expiration metadata. -- Validate tenant and principal against the active binding. -- Clear only process-local cached data for a requested audience. -- Report stable Fabric CLI errors without raw SDK or process output. - -Existing MSAL user, service-principal, managed-identity, federation, certificate, and raw-token behavior remain behind the direct `fabric-cli` provider. Interactive MSAL renewal moves out of ordinary provider acquisition and is initiated only by the coordinator when policy allows it. - -### Azure CLI provider - -Add `azure-identity` as a dependency and implement the provider with `AzureCliCredential`: - -- Resolve only a trusted Azure CLI executable through Azure Identity. -- Use disconnected standard input, no shell, a safe working directory, and a bounded 10-second acquisition. -- Pass one resolved `.default` scope per call. -- Allow only `fabric`, `storage`, `azure`, and `powerbi` route labels. -- Keep `powerbi` mapped to the existing Fabric scope. -- Cache successful tokens in process by source, tenant, principal, and audience. -- Coalesce concurrent misses for the same key; refresh inside the configured buffer; never cache failures. -- Clear and reject only the affected token when a claims challenge is returned. - -The provider never invokes Azure CLI login, logout, account selection, tenant selection, or subscription selection. - -### Command contract - -Extend `fab auth` with: - -```text -fab auth login --source azure-cli [--tenant ] [--no-prompt] -fab auth status [--check] [--audience fabric|storage|azure|powerbi] -``` - -Rules: - -- Azure CLI source flags conflict with direct source flags. -- Unattended Azure CLI login requires `--tenant` and `--no-prompt`. -- Bare attended login and eligible progressive first use share the existing chooser. -- Progressive discovery offers Azure CLI only for a supported user identity. -- Workload identities require explicit login in the first release. -- Default chooser action is defer; defer returns `AuthenticationRequired` and exit 4 without writing state. -- Cancellation returns exit 2 without writing state. - -### Persistent state - -Evolve `auth.json` to a versioned source record: - -```json -{ - "version": 2, - "source": "azure-cli", - "cloud": "AzureCloud", - "tenant_id": "", - "principal_id": "", - "principal_type": "user", - "account": "", - "subscription_id": null, - "subscription_name": null, - "app_id": "", - "bound_at": "" -} -``` - -Legacy records migrate idempotently to source `fabric-cli` without deleting the MSAL cache. Candidate source replacement follows validate-then-commit semantics: - -1. Validate Fabric readiness and identity. -2. Acquire the interprocess state lock. -3. Recheck the current state. -4. Atomically replace `auth.json`. -5. Preserve unrelated configuration and the prior state if any step fails. - -The configuration directory remains owner-only (`0700`) and auth state remains owner-only (`0600`). Runtime environment overrides never modify persistent state. - -### Identity binding - -The binding pins cloud, canonical tenant ID, stable principal ID, and principal type. Subscription metadata is display-only and nullable. Every fresh token must match the binding before a service request. - -The implementation mechanism for establishing stable tenant and principal identity is blocked on security decision Q4. If token-derived validation is approved, it must verify issuer, signature, audience, expiration, `tid`, and `oid`; otherwise the provider must use the approved metadata contract. Missing stable identity fails closed. - -### Status and logout - -Plain `fab auth status` remains exit 0 and becomes passive: no provider call, no Azure CLI process, and readiness `unknown` when local state is insufficient. - -`fab auth status --check --audience ` performs one active check and returns exit 0 when ready or exit 4 for readiness failures. Text output retains the current leading status line and legacy field order. Structured output retains the current envelope and legacy token keys with `"N/A"` during the deprecation window. - -Logout clears only Fabric-owned state for the configured source: - -- Azure CLI source: binding and in-process token cache. -- Direct source: current Fabric CLI auth state and its MSAL cache. -- All sources: Fabric CLI memory and context caches as required. - -Unrelated CLI configuration and Azure CLI state remain unchanged. - -### Errors and output - -Add structured error definitions through `fabric_cli.errors` and constants through `fab_constant`; do not hardcode user-facing messages in handlers. Readiness errors map to exit 4, usage/conflict/cancellation errors map to exit 2, and unexpected errors remain exit 1. - -All output uses existing `fab_ui` text and JSON renderers. JSON stdout contains one document; prompts and diagnostics use the diagnostic stream. Logs and telemetry exclude tokens, claims challenges, process output, command arguments, and identity identifiers. - -### HTTP, SDK, deploy, and user capability integration - -- `fab_api_client.do_request` requests tokens from the effective provider without allowing interaction or replay. -- Generalize `create_fabric_token_credential` behind its existing public factory so `fabric-cicd` receives a headless credential for the effective source. -- Run Fabric readiness preflight before entering deploy's catch-all so readiness failures preserve exit 4 and are not wrapped as `DeploymentFailed`. -- Replace source-specific `identity_type == "user"` checks with principal-capability checks for browser-open and personal-workspace behavior. - -## Dependencies - -- `azure-identity` for `AzureCliCredential`. -- Existing `azure-core`, MSAL, secure file utilities, output renderers, and command parser infrastructure. -- Security ownership and approval for executable resolution, principal binding, state, errors, and telemetry. -- Fabric agent-experience decision for the attended-host interaction channel. -- Fabric CLI engineering decision for the feature flag and rollout rings. - -## Rollout and Compatibility - -- Gate Azure CLI source selection and progressive offers independently where possible. -- Enable explicit unattended login before progressive offers. -- Preserve every existing direct authentication syntax and route mapping. -- Rollback disables Azure CLI selection and offers without destructively rewriting a recoverable source marker. -- Existing scripts consuming status retain legacy token keys as `"N/A"` for one deprecation window. - -## Testing Strategy - -### Unit tests - -- Parser conflicts, source selection, interaction classification, error mapping, and output models. -- Azure CLI executable absence, timeout, sanitized failures, scope allowlist, tenant/principal mismatch, refresh, cache coalescing, and failure non-caching. -- State migration, locking, atomic replacement, permissions, failed replacement preservation, and runtime-only environment precedence. -- Passive status zero-call behavior, active single-audience behavior, and source-local logout. - -### Integration tests - -- Shared execution across command-line, auth, REPL, JSON, pipe, CI, callbacks, and batch paths. -- Exactly-once handler and request continuation after consent. -- HTTP route-to-scope mapping and no request replay. -- Headless SDK bridge and deploy preflight error preservation. -- Existing direct user, service-principal, certificate, federation, managed-identity, and raw-token suites. - -### End-to-end and release tests - -- Windows, Linux, and macOS with Azure CLI installed, absent, signed out, user signed in, workload identity, guest tenant, and no subscription. -- Identity drift, Azure CLI context race, timeout, refresh, concurrency, and claims challenge. -- Representative Fabric Skills runs using a prepared Azure CLI identity without a second interactive login. -- Feature-gate enablement, disablement, and rollback. - -The parent test plan remains the acceptance evidence ledger for all 59 requirements and 40 mapped tests. - -## Open Gates - -| Gate | Owner | Blocks | -| --- | --- | --- | -| Q2: Select feature flag and rollout rings | Fabric CLI engineering | Rollout implementation | -| Q3: Assign final security approval owner | Fabric security and Fabric CLI leadership | Design lock | -| Q4: Approve principal identity validation mechanism | Fabric security | Binding implementation | -| Q5: Select attended-host interaction channel | Fabric agent experience and Fabric CLI | Attended-agent release | - -## Implementation Plan - -See [implementation-plan.md](implementation-plan.md) for the sequenced workstreams, dependencies, and proposed ADO task breakdown. diff --git a/Features/1694265/implementation-plan.md b/Features/1694265/implementation-plan.md deleted file mode 100644 index 74fcaebb..00000000 --- a/Features/1694265/implementation-plan.md +++ /dev/null @@ -1,71 +0,0 @@ -# Implementation Plan — Feature 1694265 (fabric-cli) - -This plan decomposes the repo-specific [design spec](design-spec.md) into independently reviewable workstreams. The parent ADO item is User Story 1694265. - -## Delivery Principles - -- Land policy and compatibility tests before changing provider behavior. -- Keep all providers noninteractive below the coordinator. -- Preserve existing direct authentication while adding Azure CLI. -- Do not merge slices that depend on unresolved security or host-integration gates. -- Track requirement and test-plan evidence from the first implementation pull request. - -## Proposed Task Breakdown - -| Slice | Proposed ADO Task | Implementation scope | Primary files | Depends on | Exit criteria | -| ---: | --- | --- | --- | --- | --- | -| 1 | Add shared executor, authentication coordinator, and interaction policy | Unify command-line, auth, REPL, and batch dispatch; classify interaction; resolve effective source; map readiness exits; guarantee exactly-once handler execution | `main.py`, `core/fab_interactive.py`, new coordinator/executor modules, `core/fab_decorators.py` | None | All execution surfaces use one policy; local commands bypass auth; unattended commands do not prompt; readiness errors exit 4 | -| 2 | Add Azure CLI provider and explicit login contract | Add `azure-identity`; parser flags and conflicts; provider protocol; trusted executable behavior; timeout; sanitization; one-scope allowlist | `pyproject.toml`, `parsers/fab_auth_parser.py`, `commands/auth/fab_auth.py`, new provider modules, `errors/auth.py` | Slice 1 interface alignment | Explicit attended and unattended login paths validate Fabric without invoking Azure CLI login or accepting arbitrary scopes | -| 3 | Implement identity binding and atomic auth state | Versioned source state; legacy migration; runtime-only environment override; locking; atomic writes; permissions; pinned identity; process cache and concurrency | `core/fab_auth.py`, `core/fab_state_config.py`, `utils/fab_secure_io.py`, new state/cache modules | Slices 1-2; Q4 for principal validation | Failed replacement preserves prior state; fresh tokens match binding; no delegated token reaches disk | -| 4 | Implement shared chooser and exactly-once continuation | Progressive eligible-user discovery; default defer; alternate direct options; direct-terminal chooser; attended-host abstraction; separate-login fallback | Coordinator, `commands/auth/fab_auth.py`, `utils/fab_ui.py`, host interaction abstraction | Slices 1 and 3; Q5 for host transport | Default Enter defers; cancellation and defer write no state; successful consent invokes one handler/request | -| 5 | Implement passive/active status, stable errors, and source-local logout | Passive status; `--check`; audience selection; compatibility output; all stable errors; stream separation; capability checks; scoped logout | Auth parser/commands, `core/fab_constant.py`, `errors/auth.py`, output models, `commands/fs/fab_fs_open.py` | Slices 1-3 | Passive status makes zero provider calls; active status checks one audience; logout preserves unrelated and Azure CLI state | -| 6 | Integrate HTTP, SDK bridge, deploy, and batch paths | Source-aware request acquisition; headless credential factory; deploy preflight before catch-all; claims behavior; fail-fast batch counts; Power BI route compatibility | `client/fab_api_client.py`, `core/fab_msal_bridge.py`, deploy command, `main.py` | Slices 1-5 | No replay; SDK callbacks stay headless; deploy readiness exits 4; Power BI remains mapped to Fabric scope | -| 7 | Complete release matrix, docs, telemetry, and Skills pilot | Full regression matrix; telemetry safety; docs/examples; feature gates; rollout and rollback evidence; representative Skills runs | Tests, docs, telemetry integration, release configuration | Slices 1-6; Q2-Q3 | All parent test-plan rows have evidence; direct sources regress cleanly; rollout and rollback are approved | - -## Dependency Graph - -```text -Slice 1 ──┬──> Slice 2 ──> Slice 3 ──┬──> Slice 4 ──┐ - │ └──> Slice 5 ──┼──> Slice 6 ──> Slice 7 - └─────────────────────────────────────────┘ - -Q4 gates identity validation in Slice 3. -Q5 gates attended-host integration in Slice 4. -Q2 and Q3 gate rollout completion in Slice 7. -``` - -## Pull Request Sequence - -1. **Policy and compatibility harness:** interaction classifier, executor contract, current-behavior regression tests, and stable error categories. -2. **Provider foundation:** provider protocol, Azure CLI parser contract, allowlist, timeout, sanitization, and mocked provider tests. -3. **State and binding:** versioned migration, runtime override model, locking, atomic write, cache, and approved principal validation. -4. **User interaction:** direct chooser and continuation first; attended-host transport only after Q5. -5. **Status and lifecycle:** passive/active status, compatibility output, capability checks, and source-local logout. -6. **Integration surfaces:** HTTP, SDK bridge, deploy, batch, claims, and Power BI route regression. -7. **Release hardening:** platform matrix, docs, telemetry, Skills pilot, flags, and rollback. - -Each pull request should link to its ADO task and update the requirement/test evidence ledger. - -## Validation Matrix - -| Layer | Required coverage | -| --- | --- | -| Parser | New flags, conflicts, required tenant/no-prompt combinations, audience allowlist | -| Policy | Direct terminal, approved attended host, undeclared host, JSON, pipe, batch, CI, callback, status, logout | -| Provider | Installed/signed-in states, timeout, sanitized error, audience mapping, refresh, concurrency, drift | -| State | Migration, permissions, atomicity, lock contention, failed replacement, environment precedence | -| Execution | CLI, auth commands, REPL, batch fail-fast, exactly-once continuation | -| Integration | HTTP routes, SDK credential, deploy preflight, user capabilities, claims handling | -| Regression | MSAL user, SPN secret/certificate/federation, managed identity, raw tokens | -| Release | Windows/Linux/macOS, guest/no-subscription, Skills pilot, feature gates, rollback | - -## Task Creation Readiness - -Before creating ADO work items: - -- The Feature Registry artifacts must be present on its default branch. -- This repo's `Features/1694265/design-spec.md` must be reviewed and merged to the code repo's default branch. -- The ADO organization, project, hierarchy, area path, iteration, and owners must be confirmed. -- Existing closed Task 1728448 remains decision history and is not reused. - -After the readiness gate passes, create the seven work items through the `manage-tasks` workflow so each ADO ID is used to generate its local `task-.md` file. diff --git a/Features/1694265/registry.md b/Features/1694265/registry.md deleted file mode 100644 index 68458234..00000000 --- a/Features/1694265/registry.md +++ /dev/null @@ -1,20 +0,0 @@ -# Feature 1694265 — Registry Pointer - -This folder contains area-specific artifacts for Feature 1694265 in the **fabric-cli** repo. - -## Feature Registry (Parent) - -- **Registry Repo:** [fabric-cli](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry) -- **Feature Folder:** `Features/active/1694265/` (moves to `Features/done/1694265/` when archived) -- **ADO Work Item:** [#1694265](https://powerbi@dev.azure.com/powerbi/Trident/_workitems/edit/1694265) *(stable canonical link)* - -> **Note:** The Feature Registry uses an `active/done` folder hierarchy. After a feature is archived, the folder path changes from `Features/active/` to `Features/done/`. Use the ADO Work Item link above as the stable reference. - -## Navigation - -> **Tip:** These links point to the `active/` path and will break after archival. Use the ADO Work Item link above to find the feature regardless of its current folder. - -- [Requirements Spec](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/requirements-spec.md) (in Feature Registry — active only) -- [Engineering Design](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/engineering-design.md) (in Feature Registry — active only) -- [Implementation Handoff](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/implementation-handoff.md) (in Feature Registry — active only) -- [Test Plan](https://powerbi@dev.azure.com/powerbi/Trident/_git/PlatformDevFeatureRegistry?path=/Features/active/1694265/test-plan.md) (in Feature Registry — active only)