From 6ed6f47b65ba201a1c71012963a7f45bfe316946 Mon Sep 17 00:00:00 2001 From: Martin Trojer Date: Thu, 3 Sep 2026 09:32:41 +0100 Subject: [PATCH] fix(cli): warn when review add --type is not a configured type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 #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 --- docs/CONFIG.md | 7 +++ src/cli.rs | 2 + src/review_cli.rs | 143 +++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 143 insertions(+), 9 deletions(-) diff --git a/docs/CONFIG.md b/docs/CONFIG.md index c86fe6ce..5be4c0d1 100644 --- a/docs/CONFIG.md +++ b/docs/CONFIG.md @@ -205,6 +205,13 @@ and it is appended to the end of the Tab cycle when they are, so you can always untyped. An untyped comment never renders a `[TYPE]` tag, a badge, or a legend entry (file-level comments still keep their `File-level:` marker on submit). +### `tuicr review add --type` + +When `comment_types` is configured, `tuicr review add --type ` warns on stderr if `` is not +one of them, listing the ids that are. The comment is still stored and the command still exits `0`, +so scripted callers keep working. With `comment_types` unset there is nothing to check against and +no warning is emitted. + ### Replacement semantics `comment_types` is a full replacement of the _configured_ types. If you define 2 types, those 2 — diff --git a/src/cli.rs b/src/cli.rs index 41503092..8404fd39 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -214,6 +214,8 @@ pub enum ReviewCommand { /// Comment classification. Defaults to `none` (no type, no `[TYPE]` /// prefix); pass a type configured via `comment_types` to classify. + /// An id absent from a configured `comment_types` warns on stderr but + /// is still stored. #[arg(long = "type", value_name = "TYPE", default_value = "none", value_parser = non_empty_comment_type)] comment_type: String, diff --git a/src/review_cli.rs b/src/review_cli.rs index ee2035c9..133dfabf 100644 --- a/src/review_cli.rs +++ b/src/review_cli.rs @@ -92,7 +92,14 @@ fn add_comment( let request_parts = build_add_request_parts(options)?; let target = request_parts.target; let comment_type = CommentType::from_id(&request_parts.comment_type); - let author = resolve_cli_author(request_parts.username); + // One config read serves both the type check and the author fallback. + let config = config::load_config() + .ok() + .and_then(|outcome| outcome.config); + if let Some(warning) = unknown_comment_type_warning(&comment_type, config.as_ref()) { + eprintln!("{warning}"); + } + let author = resolve_cli_author(request_parts.username, config.as_ref()); let comment = store.add_comment( &session_ref, AddCommentRequest { @@ -121,23 +128,53 @@ struct AddRequestParts { /// Priority: explicit `--username` / JSON `username` ► config `username` ► /// `Comment::DEFAULT_AUTHOR`. Trims whitespace so `--username " "` doesn't /// produce an awkward all-whitespace badge. -fn resolve_cli_author(explicit: Option) -> String { +fn resolve_cli_author(explicit: Option, config: Option<&config::AppConfig>) -> String { if let Some(name) = explicit.as_deref().map(str::trim).filter(|s| !s.is_empty()) { return name.to_string(); } - if let Ok(outcome) = config::load_config() - && let Some(name) = outcome - .config - .as_ref() - .and_then(|cfg| cfg.username.as_deref()) - .map(str::trim) - .filter(|s| !s.is_empty()) + if let Some(name) = config + .and_then(|cfg| cfg.username.as_deref()) + .map(str::trim) + .filter(|s| !s.is_empty()) { return name.to_string(); } comment::DEFAULT_AUTHOR.to_string() } +/// Warn when `--type` names a type that no `comment_types` entry defines. +/// +/// `CommentType::from_id` turns any non-empty id into `Custom`, so a typo is +/// indistinguishable from a configured type: it is stored, exported as +/// `**[TYPO]**`, and shown with no badge in the TUI. Warning rather than +/// rejecting keeps the CLI usable when `comment_types` is unset — the default, +/// under which every id but `none` is technically undefined — and keeps +/// scripted callers from breaking on a config change they don't control. +fn unknown_comment_type_warning( + comment_type: &CommentType, + config: Option<&config::AppConfig>, +) -> Option { + if comment_type.is_none() { + return None; + } + let configured = config.and_then(|cfg| cfg.comment_types.as_deref())?; + if configured + .iter() + .any(|definition| definition.id == comment_type.id()) + { + return None; + } + let known: Vec<&str> = configured + .iter() + .map(|definition| definition.id.as_str()) + .collect(); + Some(format!( + "Warning: comment type '{}' is not configured; known types: {}", + comment_type.id(), + known.join(", ") + )) +} + fn build_add_request_parts(options: AddCommentOptions) -> Result { let mut comment_type = options.comment_type; let mut content = options.content; @@ -866,4 +903,92 @@ mod tests { assert_eq!(value[0]["location"], "src/main.rs:42"); assert_eq!(value[0]["content"], "check this"); } + + fn config_with_types(ids: &[&str]) -> config::AppConfig { + config::AppConfig { + comment_types: Some( + ids.iter() + .map(|id| config::CommentTypeConfig { + id: (*id).to_string(), + ..Default::default() + }) + .collect(), + ), + ..Default::default() + } + } + + #[test] + fn should_warn_when_type_is_not_configured() { + // given a config declaring note and issue + let config = config_with_types(&["note", "issue"]); + + // when a typo slips through + let warning = unknown_comment_type_warning(&CommentType::from_id("isue"), Some(&config)); + + // then the id and the valid ids are both named + let warning = warning.expect("an unconfigured type should warn"); + assert!(warning.contains("'isue'"), "got {warning}"); + assert!(warning.contains("note, issue"), "got {warning}"); + } + + #[test] + fn should_not_warn_for_a_configured_type() { + let config = config_with_types(&["note", "issue"]); + assert!( + unknown_comment_type_warning(&CommentType::from_id("issue"), Some(&config)).is_none() + ); + } + + #[test] + fn should_not_warn_when_comment_types_are_unconfigured() { + // `comment_types` is unset by default, which leaves every id but + // `none` undefined. Warning there would fire on every typed comment + // the agent skill documents, so an absent list means no opinion. + assert!(unknown_comment_type_warning(&CommentType::from_id("issue"), None).is_none()); + assert!( + unknown_comment_type_warning( + &CommentType::from_id("issue"), + Some(&config::AppConfig::default()) + ) + .is_none() + ); + } + + #[test] + fn should_not_warn_for_the_untyped_default() { + // `--type none` is always valid and never carries a badge. + let config = config_with_types(&["issue"]); + assert!( + unknown_comment_type_warning(&CommentType::from_id("none"), Some(&config)).is_none() + ); + } + + #[test] + fn should_still_store_a_comment_whose_type_is_unconfigured() { + // given a session and a type no config declares + let dir = tempdir().unwrap(); + let store = ReviewStore::with_reviews_dir(dir.path()); + let session = test_session(PathBuf::from("/tmp/repo")); + let session_ref = store.save_review(&session).unwrap(); + + // when + let comment = store + .add_comment( + &session_ref, + AddCommentRequest { + target: CommentTarget::File { + path: PathBuf::from("src/main.rs"), + }, + content: "body".to_string(), + comment_type: CommentType::from_id("isue"), + author: "Codex".to_string(), + commit_id: None, + }, + ) + .expect("an unconfigured type must not block the write"); + + // then the warning is advisory only — the comment is still stored + assert_eq!(comment.comment_type.id(), "isue"); + } }