Skip to content

fix(docs): pin command-reference metavar format as a stable contract (#513) - #517

Merged
padak merged 2 commits into
mainfrom
claude/issue-513-pin-metavar-contract
Aug 22, 2026
Merged

fix(docs): pin command-reference metavar format as a stable contract (#513)#517
padak merged 2 commits into
mainfrom
claude/issue-513-pin-metavar-contract

Conversation

@padak

@padak padak commented Jul 22, 2026

Copy link
Copy Markdown
Member

What

The release-asset command reference (scripts/gen_command_reference.py, consumed by help.keboola.com and the connection-docs freshness gate) rendered its option metavar column via Click's make_metavar(). That default is not stable across Click versions — it already drifted once: bare `TEXT` / `INTEGER` at v0.70.1 (Click 8.x), `<str>` / `<int>` at v0.72.0.

This PR pins the option metavar shape to a documented convention independent of the installed Click:

  • Derive from ParamType.name (a version-stable token: text/integer/float/path/choice) instead of make_metavar(), via a new _stable_option_metavar().
  • Value-taking options always render as a <...> span — <str>, <int>, <float>, <path>, and <a|b|c> for choices (literal case preserved, e.g. <admin|guest|readOnly|share>, since choice values are real CLI tokens). Flags carry no metavar span.
  • Author-set metavars are wrapped and lowercased (ALIAS<alias>).
  • The column contract is documented in the module header.
  • Positional arguments are unchanged — they render as `NAME` (positional) and are not part of the | `--flag` `<type>` | shape the downstream gate parses.

Why

The connection-docs freshness gate (keboola/connection-docs#1037, PRDCT-556) detects whether an option takes a value by matching /^<.*>$/ on the metavar span. That works today only because 0.72.0 happens to emit the <…> form. A future Typer/Click bump reverting to bare TEXT would silently make the checker misparse value-taking options, turning valid commands into false-positive "unknown command" errors and reddening the docs build on correct content. Pinning the format at the source makes the published asset a contract by intent, not by accident of the dependency version.

How it was tested

  • New TestMetavarContract in tests/test_gen_command_reference.py: asserts scalar types map to the documented <...> forms, choices preserve literal case, explicit metavars are wrapped/lowercased, every value option satisfies the <...> span contract, flags carry no span, no bare uppercase TEXT/INTEGER/PATH/FLOAT leaks onto an option row, and the documented scalar/choice forms are present in the generated asset. A Click/Typer bump that changes make_metavar() now fails CI here instead of downstream.
  • make check green (lint + format + changelog-check + typecheck + full suite: 4620 passed, 8 skipped).
  • Verified the generated reference: all option metavars are <...>-wrapped; no bare uppercase metavars remain.

Note: no version bump / changelog entry — this only stabilizes the generated asset's shape (already <str> in the last release), it does not change the CLI surface. The immediate mitigation on the consumer side (accept <str> | TEXT | [a|b]) is still recommended in the linked issue; this is the durable source-side fix.

Fixes #513

…513)

The release-asset reference (help.keboola.com, connection-docs freshness
gate) is a published contract, but its option metavar column was rendered
via Click's make_metavar(), whose default drifted between releases (bare
`TEXT`/`INTEGER` at Click 8.x vs `<str>`/`<int>` later). The downstream
connection-docs gate (keboola/connection-docs#1037) detects value-taking
options by matching /^<.*>$/ on the metavar span, so a future dependency
bump reverting to bare `TEXT` would silently break the docs build.

Derive option metavars from Click's version-stable `ParamType.name`
instead of `make_metavar()`: value-taking options always render as a
`<...>` span (`<str>`, `<int>`, `<path>`, `<a|b|c>` for choices with
literal case preserved), flags carry none. Document the column contract
in the module header and add tests that fail CI on a Click/Typer bump
that would change the shape, instead of letting the drift surface
downstream.

@padak padak left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second-opinion review (OpenAI Codex, gpt-5.6-sol, high reasoning) — verdict: NEEDS CHANGES.

Context check first: the reviewer inspected 922 value options and 226 flags in the current CLI — all existing types produce the intended spans, and no current command uses count or tuple options. So nothing breaks today; the findings are about valid Click parameter shapes the stable contract should survive, which is the whole point of this PR.

  1. High — scripts/gen_command_reference.py:114 — count flags render as value-taking options. In Click, click.Option(..., count=True) has count=True but is_flag=False; the code would emit --verbose <int>, making the downstream gate think the option consumes a value. Suppress the metavar when is_flag or count is true, and add a _format_param() test with a count option.
  2. Medium — scripts/gen_command_reference.py:102-104 — tuple/composite and unknown-type fallback can produce malformed spans. click.Tuple([STRING, INT]) has name "<text integer>" → rendered as <<text-integer>>. A nameless custom type silently becomes <str>. Add explicit composite handling via ptype.types (e.g. <str,int>) and a neutral <value> fallback for unknown types.
  3. Medium — scripts/gen_command_reference.py:99-101str(choice) is not always the CLI token. With click.Choice(SomeEnum), Click accepts member names (RED|BLUE) while this emits Color.RED|Color.BLUE. Handle Enum members via .name (or Click's normalize_choice path) and add an Enum-backed choice test.
  4. Low — tests/test_gen_command_reference.py:113-133 — tests overstate coverage. They exercise five synthetic external-Click options while the app uses Typer's vendored Click objects, and integration assertions check token presence, not every row. Add focused tests for count, tuple/nargs, multiple, Enum choice, custom/nameless types, plus a walk of the live Typer command tree validating every emitted option row.

Int ranges, float ranges, paths/files, scalar choices, regular boolean flags, and value-taking boolean options are handled consistently. The main remaining drift risk is composite/custom types and choices, not make_metavar() itself.

…options (#513)

Review follow-up on the stable-metavar contract. Three valid Click parameter
shapes could still emit an off-contract span; none is used by the CLI today,
so the generated reference is byte-identical -- the point is that it stays
that way when one of them appears.

- Count options no longer get a value metavar. `click.Option(count=True)`
  leaves `is_flag` False, so keying only on `is_flag` published
  `--verbose <int>` and would make the downstream connection-docs gate demand
  a value the CLI does not accept. Suppression now goes through
  `_takes_a_value()` (is_flag OR count).
- Composite types render their members. `click.Tuple([STRING, INT])` has
  `name == "<text integer>"`, which the sanitizer turned into the nested
  `<<text-integer>>`; composites are now expanded via `ptype.types` to
  `<str,int>`, and a scalar type with `nargs > 1` repeats the same way.
- Enum-backed choices render member names. Click parses `RED` off the command
  line while `str(member)` is `Color.RED`; `_choice_token()` uses `.name`.
  Click's own `normalize_choice()` is deliberately not used -- it casefolds a
  `case_sensitive=False` choice, which would rewrite real tokens like
  `readOnly`.
- Nameless/unrecognised custom types fall back to a neutral `<value>` instead
  of asserting `<str>`, and every token is folded into the documented
  alphabet so no span can be malformed.

Tests: `_format_param()`-level cases for count/flag/value options, composite
and multi-value nargs, `multiple=True`, nameless and unmapped custom types,
and Enum-backed plus case-insensitive choices. Added a walk of the LIVE Typer
tree (1072 option rows) validating every emitted row against the option-cell
grammar, cross-checking metavar presence against `_takes_a_value()`, and
rejecting nested angle brackets -- the synthetic cases use plain `click`,
while the app renders Typer's vendored Click, so this is the test that
actually catches vendored-Click drift.
@padak

padak commented Aug 22, 2026

Copy link
Copy Markdown
Member Author

All four findings addressed in 0de2827. Confirming the reviewer's context check first: none of these shapes exists in the CLI today, so the generated reference is byte-identical before and after (diffed scripts/gen_command_reference.py --output across the change — no diff). The point is that it stays that way when one of these shapes does appear.

1. High — count flags rendered as value-taking. Fixed. Metavar suppression moved out of the inline is_flag check into _takes_a_value(param) = not (is_flag or count). New _format_param()-level tests cover a count option (asserting is_flag is False and count is True explicitly, so the trap is documented in the test), a plain boolean flag, and — guarding the opposite mistake — a value option that must still get its span.

2. Medium — composite/unknown-type fallback. Fixed. _composite_types() detects is_composite and expands ptype.types, so click.Tuple([STRING, INT]) renders <str,int> instead of the nested <<text-integer>>. A scalar type with nargs > 1 repeats the same way (<int,int>); composites are excluded from that path since Click already sets nargs == len(types). A nameless custom ParamType now falls back to a neutral <value> rather than asserting <str>, and every token is folded through _sanitize_token() into the documented alphabet, so an unmapped named type degrades to e.g. <my-weird-type> and no span can be malformed.

3. Medium — Enum-backed click.Choice. Fixed. _choice_token() renders Enum members via .name, so click.Choice(Color) gives <RED|BLUE>. Click's own normalize_choice() is deliberately not used: it casefolds a case_sensitive=False choice, which would silently rewrite real CLI tokens like readOnly in the published asset. A test pins that too.

4. Low — tests overstate coverage. Added TestLiveCommandTreeGrammar, which walks the real Typer tree (Typer's vendored Click, not plain click) and validates every emitted option row — 1072 of them — against the option-cell grammar: names are backticked option tokens, the optional metavar matches ^<[A-Za-z0-9][A-Za-z0-9|,_-]*>$. It also cross-checks metavar presence against _takes_a_value() per live param, asserts no row nests angle brackets, and refuses to pass on a scan of fewer than 500 rows so a broken walker can't produce a silent green. Plus the requested focused cases: count, tuple, multi-value nargs, multiple=True, Enum choice, nameless and unmapped custom types.

Docstring contract in the module header updated to state the new rules (no span for is_flag or count, comma-joined composites, <value> fallback).

Verification: tests/test_gen_command_reference.py 27 passed; full suite 4633 passed / 140 skipped; ruff check, ruff format --check and ty check clean on both touched files; check_command_sync.py OK. Still no version bump / changelog entry — the published asset's bytes are unchanged.

@padak
padak marked this pull request as ready for review August 22, 2026 21:05
@padak
padak merged commit d6c4f11 into main Aug 22, 2026
4 of 5 checks passed
@padak
padak deleted the claude/issue-513-pin-metavar-contract branch August 22, 2026 21:05

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Open in Devin Review

Comment on lines +84 to +85
"filename": "path",
"file": "file",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 File-typed option renders as

_METAVAR_BY_TYPE_NAME maps "filename": "path", and Click's File type has name == "filename", so a click.File() option renders <path>, not <file>. The "file": "file" entry is unreachable for stock Click. Cosmetic, but worth confirming intent.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

padak added a commit that referenced this pull request Aug 22, 2026
pyproject/plugin.json/marketplace.json were already renumbered to 0.89.0 by
v0.88.0.

Changelog: adds 0.89.0 entries for #645 (describe-batch --from-file shape
validation, issue #640), #642 (table-detail human column descriptions), #620
(sync-action forwards root authorization/runtime), #517 (stable metavar
contract, issue #513), #586 (documented prompt budget gated against the
enforced one, issue #585) and #641 (docs-only), and decorates the existing

Silent-drift surfaces:

* gotchas.md -- resolves both "(Release step: ... tag this sentence)"
  placeholders. Both were left by commits AFTER the v0.88.0 tag (#642 and
  #645), so both are tagged (since v0.89.0), not 0.88.0. Adds the #620 gotcha:
  below 0.89.0 a sync action on an OAuth / Service-Account component died with
  an opaque empty-body 400 because the broker reference was never forwarded.
* #620 shipped with no doc surfaces at all -- CLAUDE.md, AGENT_CONTEXT and
  commands-reference.md now carry the forwarding rule (root only, never
  row-overridden, only when non-empty) with its version gate.
* #645 never reached CLAUDE.md -- the describe-batch shape check and its
  behaviour change are recorded there now; commands-reference gains the
  version tag.
* #642's human Description column is version-tagged in CLAUDE.md,
  commands-reference.md, AGENT_CONTEXT and storage-describe-workflow.md.
* #643 was otherwise complete; adds the two surfaces it did not touch --
  safe-write-workflow.md (delete is reversible; never blind-retry on <= 0.88.x)
  and a keboola-expert.md matrix row for delete/restore/trash-list.
  keboola-expert.md is 49 774 B, well inside the 70 000 B budget.

make check green: 5934 passed, 12 skipped. version-gate-check resolves all 438
markers across 72 versions.
padak added a commit that referenced this pull request Aug 23, 2026
pyproject/plugin.json/marketplace.json were already renumbered to 0.89.0 by
v0.88.0.

Changelog: adds 0.89.0 entries for #645 (describe-batch --from-file shape
validation, issue #640), #642 (table-detail human column descriptions), #620
(sync-action forwards root authorization/runtime), #517 (stable metavar
contract, issue #513), #586 (documented prompt budget gated against the
enforced one, issue #585) and #641 (docs-only), and decorates the existing

Silent-drift surfaces:

* gotchas.md -- resolves both "(Release step: ... tag this sentence)"
  placeholders. Both were left by commits AFTER the v0.88.0 tag (#642 and
  #645), so both are tagged (since v0.89.0), not 0.88.0. Adds the #620 gotcha:
  below 0.89.0 a sync action on an OAuth / Service-Account component died with
  an opaque empty-body 400 because the broker reference was never forwarded.
* #620 shipped with no doc surfaces at all -- CLAUDE.md, AGENT_CONTEXT and
  commands-reference.md now carry the forwarding rule (root only, never
  row-overridden, only when non-empty) with its version gate.
* #645 never reached CLAUDE.md -- the describe-batch shape check and its
  behaviour change are recorded there now; commands-reference gains the
  version tag.
* #642's human Description column is version-tagged in CLAUDE.md,
  commands-reference.md, AGENT_CONTEXT and storage-describe-workflow.md.
* #643 was otherwise complete; adds the two surfaces it did not touch --
  safe-write-workflow.md (delete is reversible; never blind-retry on <= 0.88.x)
  and a keboola-expert.md matrix row for delete/restore/trash-list.
  keboola-expert.md is 49 774 B, well inside the 70 000 B budget.

make check green: 5934 passed, 12 skipped. version-gate-check resolves all 438
markers across 72 versions.
padak added a commit that referenced this pull request Aug 23, 2026
…es (#651)

Release prep for 0.89.0: adds the changelog entries for everything merged since v0.88.0 (#620, #642, #643, #644, #645, #646, #647, #648, #649, #650, #517, #586, #641), resolves every vNEXT placeholder left by feature PRs to v0.89.0 per the new #648 release process, closes the 10 gaps a full doc-surface audit found across the kbagent plugin (SKILL.md triggers, commands-reference, gotchas, workflow files, keboola-expert.md, AGENT_CONTEXT, CLAUDE.md), and records the live e2e verification evidence. Version files were already at 0.89.0 (bumped by #643); make version-sync is a no-op.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pin command-reference.md metavar format as a stable contract (docs gate depends on it)

1 participant