fix: restore explicit usage for `job run-update` parser by ayeshurun · Pull Request #99 · ayeshurun/fabric-cli

ayeshurun · 2026-08-27T09:24:48Z

Resolves [BUG] fab-cli job run-update --help command just shows errors microsoft/fabric-cli#277 ([BUG] fab-cli job run-update --help command just shows errors microsoft/fabric-cli#277)

Problem

fab job run-update --help raises an AssertionError from inside argparse instead of printing help:

File "argparse.py", line 342, in _format_usage
    assert ' '.join(opt_parts) == opt_usage
AssertionError

Root cause

run_update_parser is the only argument-bearing leaf parser in the codebase that never sets an explicit usage string. (extension and version are also leaves without one, but declare no arguments of their own, so their generated usage is short and never wraps.) Every other argument-bearing leaf sets it:

X_parser.usage = f"{utils_error_parser.get_usage_prog(X_parser)}"

When usage is set, argparse takes the usage % dict(prog=...) branch and skips the generate-and-wrap block entirely. Without it, argparse builds the usage itself:

--id is declared required=True with metavar="".
_format_actions_usage formats it as '%s %s' % ('--id', '') -> '--id ' (trailing space). Because it is required, it is not wrapped in [...].
argparse's cleanup regexes only strip spaces adjacent to brackets, so the dangling space survives: ... --id [-i] [--enable] ... (double space).
On the wrapping path, _format_usage re-splits with re.findall, which collapses the double space, then asserts the join round-trips. It doesn't -> AssertionError.

Other parsers (e.g. run-cancel) have the same required=True + metavar="" combination but are shielded by their explicit usage.

Fix

One line, restoring the convention the rest of the file already follows:

run_update_parser.usage = f"{utils_error_parser.get_usage_prog(run_update_parser)}"

Also corrects the first example in the help text: --disabled -> --disable. The --disabled flag does not exist and is rejected by the parser. This was invisible before because the help never rendered.

Honest limitations

This does not fix the underlying argparse bug, it avoids it. A formatter-level fix in CustomHelpFormatter (e.g. overriding _format_actions_usage, or auto-assigning usage when None) would harden all parsers, but touches private argparse APIs that were refactored between 3.12 and 3.13, and auto-assigning usage would change output for the root parser, group parsers, version, and extension. Happy to go that route if maintainers prefer it.
The crash is terminal-width dependent. It only fires when the usage line is long enough to wrap. On a wide terminal run-update --help would have worked.
The crash is Python-version dependent (confirmed). CPython 3.13 replaced the regex re-split with _get_actions_usage_parts_with_split and dropped the assertion, so the AssertionError does not reproduce there. I could only test 3.12.3 myself; this was independently verified on 3.13.15 in review. CI covers 3.10-3.13, so on the 3.13 job test_format_help_does_not_raise no longer reproduces [BUG] fab-cli job run-update --help command just shows errors microsoft/fabric-cli#277 specifically -- but the explicit-usage invariant test still fails pre-fix on every version, so the suite is not a silent no-op.

Tests

New tests/test_parsers/test_fab_parser_help.py:

test_format_help_does_not_raise -- walks all 59 parsers, asserts format_help() doesn't raise. Uses a narrow_terminal fixture (COLUMNS=60) to force the wrapping path; without it the test passes even on broken code, since pytest's captured terminal is wide.
test_leaf_parsers_declaring_arguments_set_explicit_usage -- asserts every leaf parser with its own arguments sets usage. This is the durable guard: it holds regardless of Python version and prevents the class of bug rather than the instance.
test_job_run_update_help_lists_flags -- asserts the rendered help contains the expected usage line and flags.

All three fail on the unfixed tree; all pass with the fix.

Verification

pytest tests/test_core tests/test_utils tests/test_parsers -> 661 passed, 9 skipped
pytest tests/test_commands -k job -> 78 passed, 1 pre-existing unrelated failure (test_import.py::test_import_create_new_item_success[SparkJobDefinition], also fails on a clean tree)
mypy --ignore-missing-imports clean on both changed files
Manual: COLUMNS=60 fab job run-update --help now renders correctly and matches sibling formatting

I deliberately did not run black across fab_jobs_parser.py -- the file isn't black-formatted on main, so reformatting it would have added ~16 lines of unrelated diff noise to a 2-line fix.

To be precise about why that's safe: CI does run Black (fab-build.yml -> tox -e lint -> black .), but without --check or --diff. Black reformats in the ephemeral CI workspace and exits 0, so the job cannot fail on formatting and the rewrite is discarded. Formatting is therefore not gated. black --check is clean on the new test file. (An earlier revision of this description claimed there was no Black in CI at all -- that was wrong, corrected here. Separately: the lint job being unable to fail is arguably its own bug, but out of scope.)

Out of scope (noticed, not changed)

--disable uses action="store_false", so args.disable defaults to True. The consuming logic in fab_jobs_run_update.py is confusing but correct. Verified in review across all four flag combinations -- neither: (False, True), --enable: (True, True), --disable: (False, False), both: (True, False); with no config neither/both error and enable/disable set state, with config neither preserves state and both errors.
run-update's help= string is past tense ("Updated the schedule...") unlike its siblings, and differs from the description at the top of the file.

Review round 1

Independently reviewed in an isolated session (Senior Software Engineer persona, no exposure to the implementation reasoning). Verdict: approve with changes. Applied:

Added type annotations to the fixture and all three test functions. Repo guidance requires type hints on all functions; these were unannotated, which meant mypy was skipping their bodies entirely. Now annotated and re-verified clean.
Corrected the Black rationale above (my original claim that CI has no Black check was false).

Reviewer independently confirmed, rather than taking my word for: the root cause and the double-space mechanism; that run-update is the only affected registered parser out of 59; that exactly 3 tests fail pre-fix; that Python 3.13 is immune; that COLUMNS is honoured cross-platform by shutil.get_terminal_size() and restored by monkeypatch; and the enable/disable truth table.

Deferred as follow-up rather than expanding this PR:

get_usage_prog() brackets every optional regardless of action.required, so the rendered usage shows [--id] even though --id is required. This affects all commands equally, and fixing it here would change user-visible help output across the CLI.
The "every leaf parser must remember to set usage" invariant is a footgun (~49 manual call sites; 10 parser nodes / 11 actions have the same required=True + metavar="" shape, and 9 of those nodes are shielded only by explicit usage). Centralising leaf-parser finalisation in a registration/factory layer would remove the class of bug structurally. The new invariant test guards against recurrence in the meantime.

Review round 2

Second independent review (Staff Python Engineer persona, fresh session). Verdict was request changes. It independently reproduced the crash on Python 3.10.20 / 3.11.16 / 3.12.3 and confirmed 3.13.15 is immune, and confirmed all new tests fail against the pre-fix tree.

Applied:

Required flags no longer render as optional. get_usage_prog() bracketed every optional action regardless of action.required, so required flags displayed as [--id] although argparse rejects omitting them. Fixed in a separate commit (0c6f296) so it can be dropped independently of the crash fix. This affects 18 required flags across 16 commands; verified all 59 parsers still format cleanly at COLUMNS 30/40/60/80/120/200, since an explicit usage string bypasses argparse's generate-and-wrap path entirely.
Docs corrected -- docs/commands/jobs/index.md bracketed the required --id for run-cancel/run-status/run-update/run-rm, and omitted --interval/--start/--end/--days from the run-update synopsis.
run-sch help example fixed -- one entry under sch_examples invoked job run-update ... --id <schedule_id> instead of job run-sch, i.e. the wrong command plus a flag run-sch does not accept. Same class as the --disabled typo already fixed here.
Real --help dispatch is now tested -- previous tests only called format_help(). The new test drives parse_args(["job", "run-update", "--help"]) and asserts SystemExit(0) plus stdout. It also fails against the pre-fix tree, so it genuinely covers [BUG] fab-cli job run-update --help command just shows errors microsoft/fabric-cli#277 rather than merely adding coverage.
Test assertions tightened -- required --id is asserted unbracketed (optional --enable still bracketed), and the flag list no longer omits --interval/--start/--end.
Description facts corrected -- see the two edits above ("only argument-bearing leaf", and 10 nodes / 11 actions rather than 9).

Where I disagreed, with evidence:

The reviewer framed [--id] as a regression introduced by this PR. It is not. Bracketing required flags is pre-existing CLI-wide behaviour from get_usage_prog(), already affecting 18 required flags across 16 commands on main. Before this PR run-update did not use that helper, so its generated usage did show --id unbracketed -- but that same generated line also carried the malformed double space that causes [BUG] fab-cli job run-update --help command just shows errors microsoft/fabric-cli#277, leaked the raw prog (-c), and ordered <path> last. So it was a mixed trade, not a pure regression. It is fixed regardless, which makes the objection moot.
The reviewer proposed replacing the explicit-usage invariant test with a behaviour test. Kept as-is: it is cheap, it fails against the pre-fix tree, and on Python 3.13 -- where the crash cannot reproduce -- it is one of the assertions that still detects the defect. Centralising leaf-parser finalisation remains the right structural fix and is still out of scope for a crash fix.

Not done (deliberate):

Value metavars in usage (--id <schedule_id> rather than --id). Broader user-visible change across every command, and orthogonal to both the crash and the requiredness bug.

Review round 3

Third independent review (Senior CLI / Developer-Experience Engineer persona, fresh session). Verdict: approve with changes, no blocking defects. It reproduced the defect from a standalone argparse repro with no fabric-cli code in the loop, and produced a version/width matrix confirming the crash on 3.11/3.12 at COLUMNS 40-100 and immunity on 3.13.

I verified every material claim rather than accepting it. All held, and it corrected one of my own counts.

Applied:

Removed a double space from get_usage_prog(). Its final return interpolated an empty positionals section, so commands with no positionals rendered deploy [--output_format] .... A double space is precisely what trips argparse's usage round-trip assertion, so shipping one in the shared helper -- in the PR that fixes [BUG] fab-cli job run-update --help command just shows errors microsoft/fabric-cli#277 -- was an avoidable hazard. Harmless today only because an explicit usage is never re-parsed. Affected 9 commands; now 0.
Documented that parser.usage is load-bearing. The line at fab_jobs_parser.py:279 reads as a cosmetic consistency edit but is the entire crash fix: it diverts argparse away from the asserting code path. A future cleanup removing "redundant" usage assignments would silently reintroduce the crash on Python <= 3.12. Comments added there and on get_usage_prog.
Added direct unit tests for get_usage_prog (tests/test_utils/test_fab_error_parser.py). The helper has 49 call sites and changed behaviour, but the only assertion on the new behaviour went through a single command. 8 of the 9 new tests fail against the previous implementation; the ninth pins optional-flag rendering, which is correct in both.
Disclosed the usage change in the changelog. The existing entry mentioned only the crash. A second entry now names all 16 affected commands. Note .changie.yaml has no changed kind (breaking, new-items, added, fixed, optimization, docs), so this is filed as fixed -- defensible, since the old output labelled required flags as optional.
Docs: added the missing ### run-update section (this PR is about run-update discoverability, and the command still had no docs section), removed a stray * from the run-sch row, and replaced [--enable/--disable] with [--enable] [--disable] -- the slash implied a mutual exclusivity that does not exist anywhere in the CLI.

Corrections to earlier statements in this description:

16 commands / 18 required flags, not 17 commands. The reviewer's enumeration was right and mine was wrong. Affected: 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, job run-rm.
The strongest justification for the get_usage_prog change was missing from this description. The hand-written docs already use unbracketed required flags everywhere except the jobs page (fs/export.md, fs/import.md, fs/deploy.md, fs/set.md, fs/ln.md, fs/assign.md, fs/unassign.md, tables/index.md, acls/index.md, labels/index.md). So the change converges runtime output onto an already-published convention -- it closes a docs/runtime divergence rather than inventing a new style. It remains isolated in commit 0c6f296 and can still be dropped independently.
The Windows attribution in [BUG] fab-cli job run-update --help command just shows errors microsoft/fabric-cli#277 is incidental. The causal variables are terminal width and Python version; this PR touches no OS-specific code. Worth stating so nobody hunts a Windows-specific bug that does not exist. Separately -- and not this PR's to fix -- fab-build.yml runs on ubuntu-latest only, so a Windows-reported bug is being closed with no Windows CI at all.
Narrow-terminal help is now non-crashing, not "correct". Because an explicit usage is never wrapped, run-update's 128-character usage line overflows a 40-column terminal instead of crashing. That is strictly better and consistent with the other 49 parsers, which already behave this way, but it is not the same as rendering correctly.
The test_import.py[SparkJobDefinition] failure noted above did not recur. The reviewer could not reproduce it either (1073 passed / 2 skipped on both trees), and a full re-run here is now green. It appears environmental or ordering-dependent, not a real failure.

Noted, deliberately not done:

test_format_help_does_not_raise passes vacuously on Python 3.13. Three of four CI legs still exercise it, and test_leaf_parsers_declaring_arguments_set_explicit_usage is the durable version-independent guard, as already stated above.
metavar="<id>" instead of metavar="" would eliminate the double space at source and is the more robust repair, but it changes output across every command that uses the pattern. The reviewer agreed it does not belong in a bug-fix PR.
tox.toml:43 runs black . without --check (lint cannot fail on formatting), and [tool.black] at tox.toml:69 is dead config since black reads pyproject.toml only. Both pre-existing and out of scope.

Verification after these changes: 672 passed / 9 skipped on tests/test_core tests/test_utils tests/test_parsers (up from 663, +9 new); tests/test_commands 1073 passed / 2 skipped, byte-identical to the pre-change baseline; mypy src tests clean across 348 files; black --check clean on all changed Python files.

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

chore: Draft Azure CLI authentication design AB#1694265

`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 microsoft#277 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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

`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

The `job run-sch --help` examples included an entry that invoked `job run-update --id <schedule_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 microsoft#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

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

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

ayeshurun · 2026-08-27T11:19:23Z

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


@copilot remove this comment

Removed in 5366ef1.

ayeshurun · 2026-08-27T11:19:44Z

+    """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.
+    """


@copilot remove docstring

Removed in 5366ef1.

ayeshurun · 2026-08-27T11:20:45Z

+    # Required: an explicit usage string keeps argparse out of its own
+    # usage-wrapping path, which crashes on this parser's `metavar=""` args.


@copilot remove comment

Removed in 5366ef1.

ayeshurun · 2026-08-27T11:22:27Z

@copilot remove this file

Removed in 5366ef1.

ayeshurun · 2026-08-27T11:23:08Z

@copilot remove this file

Removed in 5366ef1.

ayeshurun · 2026-08-27T11:23:20Z

@copilot remove this file

Removed in 5366ef1.

Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com>

ayeshurun · 2026-08-29T04:29:09Z

+    # 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
    ]

-    # Combine parts for the final usage string
-    return f"{command_part} {' '.join(pos_args)} {' '.join(opt_args)}"
+    sections = [command_part, " ".join(pos_args), " ".join(opt_args)]
+    return " ".join(section for section in sections if section)


@copilot revert this change.

Reverted in 1956c51.

Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com>

…ev/alonyeshurun/automatic-disco

Alon Yeshurun and others added 9 commits August 2, 2026 14:34

Merge pull request #94 from ayeshurun/dev/alonyeshurun/tg-poc

370cd5f

chore: Draft Azure CLI authentication design AB#1694265

Merge branch 'main' of https://github.com/ayeshurun/fabric-cli

0ff40f7

ayeshurun commented Aug 27, 2026

View reviewed changes

Copilot started work on behalf of ayeshurun August 27, 2026 11:23 View session

chore: address PR review cleanup

5366ef1

Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com>

Copilot finished work on behalf of ayeshurun August 27, 2026 11:28

ayeshurun commented Aug 29, 2026

View reviewed changes

Copilot started work on behalf of ayeshurun August 29, 2026 04:29 View session

fix: revert usage helper change

1956c51

Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com>

Copilot finished work on behalf of ayeshurun August 29, 2026 04:33

ayeshurun force-pushed the main branch from b136c3e to 09e53e5 Compare August 30, 2026 09:01

Alon Yeshurun added 2 commits August 30, 2026 12:02

Merge branch 'main' of https://github.com/ayeshurun/fabric-cli into d…

cbb7521

…ev/alonyeshurun/automatic-disco

Remove

1b814ea

		# Required: an explicit usage string keeps argparse out of its own
		# usage-wrapping path, which crashes on this parser's `metavar=""` args.

Conversation

ayeshurun commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root cause

Fix

Honest limitations

Tests

Verification

Out of scope (noticed, not changed)

Review round 1

Review round 2

Review round 3

Uh oh!

ayeshurun Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

ayeshurun Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

ayeshurun Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

ayeshurun Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

ayeshurun Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

ayeshurun Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

ayeshurun Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ayeshurun commented Aug 27, 2026 •

edited

Loading