Repository navigation
feat(args): one short flag per long option, on every option - #902
Conversation
keboola-pr-reviewer-bot
left a comment
There was a problem hiding this comment.
Verdict: needs_human (risk 3/5) · profile _default
Documented breaking change to a published CLI: removed short flags now exit 2, which policy names as an escalation class.
Concerns:
docs/migrate-to-1.0.md: Removed short flags (-c/-r/-d/-f/-l/-m/-n/-v) now exit 2; breaking for scripts/agents.src/keboola_agent_cli/commands/changelog.py: changelog --full loses -v and -n; verify no CI/agent relies on them.
zajca
left a comment
There was a problem hiding this comment.
Automated code review
Reviewed by Opus + Sonnet, full review of 178acc353a7d.
Opus: Trusted base: e8bf703. I read the repository guidance (CLAUDE.md, docs/cli-interface-spec.md, docs/migrate-to-1.0.md) from that base commit.
The PR applies spec rule A2 in a stricter form. Each short flag now maps to one long option, and every option with that long name declares it. It adds -p, -n, -t, -L, -o and -e to every --project, --name, --type, --limit, --output and --email option. It removes the short flags that meant a different option (-c, -r; -d on --body, --deny and --description; -f on --format; -l/-n on --limit; -m on --mode; changelog -v). It also drops RESERVED_SHORT_FLAGS, adds the blocking checker rule args.short-flag-on-every-option, removes the baseline entries, and regenerates the manifest and the context text. Docs (spec, migration row 36, gotchas, CLAUDE.md, plugin references) are updated to match.
How I reviewed it:
- I read the full diff of the checker, the baseline, the tests, the docs and representative command modules.
- I loaded the full Click command tree at the base and at the head from read-only copies in /tmp and compared every leaf command:
- No command at the head declares the same option string twice.
- No command option takes over a root short flag (only the expected hidden
-jroot-flag copies appear). - No long option was added or removed.
- No removed short flag now selects another option of the same command. All 16 removed short flags (for example
changelog -n,kai history -n,search -l/-r,lineage show -c/-f,http post/patch -d) are simply gone and give a usage error. This matches the PR description.
- I searched src/, plugins/, docs/, tests/, scripts/ and web/src for remaining uses of the removed short flags. The only hit is a historical release-note string in changelog.py, which is fine.
- I ran tests/test_check_cli_spec.py, tests/test_changelog_render.py and tests/test_lineage_view.py on the head copy: 64 passed, 2 skipped. The full checker script could not finish locally because fastapi was missing from the ad-hoc environment. That is a limit of my local setup, not a defect in the PR.
Tests: the new rule is proven by test_an_option_without_its_short_flag_fails. It would go red if r_short_on_every_option stopped looking up SHORT_FLAGS by long name, and the reworked test_a_short_flag_with_another_meaning_fails keeps covering the one-meaning rule. The registry and spec table are compared by test_short_flag_table_matches_the_registry, and the CLI change itself is enforced by the blocking checker rule with an empty baseline. The rewritten tests keep their contracts.
Compatibility: removing the short flags breaks scripts and agents that use them. This is intentional: it ships in the 1.0.0 major release, is documented as row 36 of docs/migrate-to-1.0.md and in the plugin gotchas with a version gate, and the plugin documents switch to long option names that work on both majors. The new short flags are additive. I found no break without a documented compatibility path.
Reuse: the only new helper is r_short_on_every_option in scripts/check_cli_spec.py. It complements r_short_one_meaning instead of duplicating it, and the base has nothing else covering that responsibility.
No actionable correctness, security, compatibility, test or maintainability issues were found in the trusted review scope.
Sonnet: PR #902 (one commit on top of e8bf703, 63 files) implements spec rule A2 of the 1.0 CLI plan (#791). Every option with a long name in the SHORT_FLAGS registry now carries its short flag (-p --project, -n --name, -t --type, -L --limit, -o --output, -e --email and others). The conflicting short flags are removed: -c on --category and --column, -r on --regex and --role, -d on --body, --deny and --description, -f on --format, -l and -n on --limit, -m on --mode, and changelog -v. The reserved-flag registry is dropped. scripts/check_cli_spec.py gains a blocking rule, args.short-flag-on-every-option, and its baseline is updated. The spec, CLAUDE.md, the migration guide (new row 36), gotchas.md and the plugin docs are updated, along with a few tests that used the removed flags.
I read the full diff of src/, scripts/, tests/ and the docs. I checked the new-flag additions for duplicate short flags within one command with a static AST pass over the head sources. The only hits were register helper functions that define several separate subcommands, so they are not real collisions. I could not run the CLI or the test suite (no environment installed, and installs are out of scope), so runtime option parsing was not exercised. The trusted base has no AGENTS.md, no .agents/skills/test-audit, and no docs/ conventions beyond the CLI spec. I applied CLAUDE.md and the surrounding conventions.
Tests: the new rule has unit tests for both the pass and fail cases, and test_v_is_not_a_short_flag_of_full proves that a removed flag is rejected with exit 2. A change to the registry or to the rule logic would turn those tests red. The other removed flags have no direct usage-error test, and the 200-odd added -p flags are covered only by the blocking spec-check rule. That is a non-actionable gap here, because the ratchet enforces it.
Compatibility: this removes short flags that scripts may use, and it is a breaking change in an externally visible CLI surface. It is deliberate and gated to the 1.0 line. It is tagged vNEXT and listed as migration row 36 with exit-code-2 behavior, and the plugin gotchas tell agents to use long names, which work on both majors. Added short flags are additive. I report no finding, since the break follows the repository's documented 1.0 migration plan.
Reuse: no new production helpers were added apart from one rule function, r_short_on_every_option, which follows the pattern of the existing r_short_one_meaning rule and reuses its helpers. There is no duplication.
No actionable correctness, security, compatibility, test, or maintainability issues were found in the trusted review scope.
No blocking findings were identified.
🤖 Written by AI: Hermes pr-review (posted from zajca's GitHub account, not by a human)
Main moved by 8 commits (short flags for every option, #902, among them). The merge is clean, `--governed` has no short flag in the registry, and the CLI spec gate, command sync, lint and the workspace tests pass on the result. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Main now requires the short flag of every option whose long name is in SHORT_FLAGS (#902, spec A2). The rls, cls and binding commands declared `--project` without `-p`; the CLI spec gate failed on 17 leaves. The manifest is regenerated. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Why
-pwas on 24 of the 228--projectoptions.-nwas on 2 of the 40--nameoptions,-ton 1 of the 20--typeoptions, and-oon 3 of the 7--outputoptions.-c,-d,-f,-l,-m,-nand-r. For example,-dwas--directoryin 10 commands,--bodyinhttp post,--denyinpermissions setand--descriptionintoken create.-pto existing options, and it reserved-c,-nand-r.What changes
Rule A2: one short flag maps to one long option, and every option with that long name has it. A long name that is not in
SHORT_FLAGShas no short flag.New short flags on every option of their long name:
-p--project(228 options),-n--name(40, as in the Azure CLI),-t--type(20),-L--limit(14, as in the GitHub CLI),-o--output(7) and-e--email(8).Removed short flags that meant another option. This is a breaking change, row 36 of
docs/migrate-to-1.0.md:-c:permissions list --category,lineage show --column-r:searchandconfig search --regex,project inviteandproject member-set-role --role-donhttp postandhttp patch --body,permissions set --denyandtoken create --description-fonlineage show --format-lonsearch --limit, and-nonchangelogandkai history --limit(use-L)-monpermissions set --mode-vonchangelog --full(-vis the global--verbose)A removed short flag is now a usage error (exit 2). No removed short flag selects another option of the same command.
scripts/check_cli_spec.py:-Land-nare inSHORT_FLAGS,RESERVED_SHORT_FLAGSis removed, and the new blocking ruleargs.short-flag-on-every-optionfails on an option without its short flag. The baseline has no short-flag entries left.A script changed the
typer.Optiondeclarations through the AST, thenruff formatran.docs/cli-manifest.jsonand the command list ofkbagent contextare regenerated.Docs: spec rule A2, the migration page (row 36 and the plugin forms),
gotchas.md(avNEXTentry and a bullet for both major versions), andCLAUDE.md. The help oflineage show,kbagent context, the plugin documents and the tests use the long names where they used a removed short flag (lineage show -cand-f,token create -d,changelog -n).Checks
make checkpasses.make cli-spec-checkreports no short-flag violation and no baseline entry.Part of #791.