Skip to content

fix(cli): warn when review add --type is not a configured type - #682

Merged
agavra merged 1 commit into
agavra:mainfrom
martintrojer:fix/cli-warn-unknown-comment-type
Sep 7, 2026
Merged

agavra merged 1 commit into
agavra:mainfrom
martintrojer:fix/cli-warn-unknown-comment-type

Conversation

@martintrojer

Copy link
Copy Markdown
Collaborator

Fixes #681.

CommentType::from_id maps any non-empty id to Custom. That is right for the TUI, where ids come from validated comment_types config, but the CLI had nothing to check against — so --type isue was stored, exported as **[ISUE]**, and badged as an unknown type, all at exit 0.

Why warn instead of reject

Two reasons, and I would rather flag them than have them found in review.

comment_types is unset by default, and under that default every id but none is undefined. Rejecting unknown ids would make --type issue an error on a stock install — which is exactly what the agent skill tells agents to pass. Second, a scripted caller should not start failing because someone edited a config it does not control.

So: an absent or empty comment_types means no opinion and no warning. A configured list that omits the id gets a warning naming the valid ids. The comment is written either way and the exit code stays 0.

If you would rather have a hard error, or a --strict-type flag, say so and I will rework it — the placement is the same either way.

The warning goes to stderr, so stdout stays parseable JSON.

add already read config for the username fallback, so the check reuses that read rather than adding a second one; resolve_cli_author now takes the config instead of loading it.

Test plan

  • cargo test: 1585 passed, 0 failed. cargo fmt --check and clippy --all-targets clean (one pre-existing bitbucket/bkt.rs warning, present on main)
  • Five new unit tests: warns and names the valid ids for an unconfigured id, silent for a configured one, silent when comment_types is absent or the config is default, silent for none, and the comment is still written when the type is unconfigured
  • Manually, with comment_types = [note, issue]:
    • --type issue → nothing on stderr
    • --type isue → Warning: comment type 'isue' is not configured; known types: note, issue on stderr, stdout still valid JSON (verified with json.load), exit 0
    • No config file: both --type issue and the none default stay silent

`CommentType::from_id` maps any non-empty id to `Custom`, which is right
for the TUI, where ids come from validated `comment_types` config. The
CLI had nothing to check against, so `--type isue` was stored, exported
as `**[ISUE]**`, and badged as an unknown type, all at exit 0. Agents
seeding comments are the ones who notice least.

Warns rather than rejects, for two reasons. `comment_types` is unset by
default, and under that default every id but `none` is undefined —
rejecting would break `--type issue`, which the agent skill documents.
And a scripted caller should not start failing because someone edited a
config it does not control. So an absent list means no opinion, and a
present one that omits the id gets a warning naming the valid ids.

The warning goes to stderr, leaving stdout parseable JSON.

`add` already read config for the `username` fallback, so the check
reuses that read instead of adding a second one; `resolve_cli_author`
now takes the config rather than loading it.

Fixes agavra#681

Test plan:
- cargo test: 1585 passed, 0 failed. cargo fmt --check, clippy clean
  (one pre-existing bkt.rs warning)
- Five new unit tests: warns and names valid ids for an unconfigured id,
  silent for a configured one, silent when comment_types is absent or
  the config is default, silent for `none`, and the comment is still
  written when the type is unconfigured
- Manually, with comment_types = [note, issue]: `--type issue` prints
  nothing on stderr; `--type isue` prints the warning to stderr, keeps
  stdout as valid JSON (checked with json.load), and exits 0. With no
  config file, both `--type issue` and the `none` default stay silent

@agavra agavra left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you @martintrojer for the contribution! This looks good to me.

@agavra
agavra merged commit 45406b4 into agavra:main Sep 7, 2026
5 checks passed
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.

review add accepts any --type, silently creating an unconfigured comment type

2 participants