From 5ea0486d6d41179f6db2ffe4381fd25cd4dae5a1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 17:02:44 +0000 Subject: [PATCH] feat(add): let the caller name the artifact via --id (REQ-007, #880) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rivet add` chose the ID itself and offered no way to say what it should be. In varve — 89 distinct requirement prefixes, all descriptive of what the requirement is about — that landed a graph-wide release-packaging requirement as `REQ-PIN-003` twice in one session. Title, tags, target file, and `--field id=` all made no difference to the derivation, so the documented workaround was to hand-edit the `id:` line the tool had just written — which is the thing the project's own guidance forbids. `--id ` closes that path: the caller asserts the ID, the tool shape-validates it up front and, via the existing `validate_add`, rejects a duplicate before any file is written. Shape is `PREFIX-NNN` — the convention every existing artifact already follows (`REQ-DRV-COMPONENT-001`, `FIND-DMA-SHM-CANONICAL-001`): uppercase alphanumeric segments dash- separated, ending with a numeric suffix, non-empty prefix carrying at least one letter. Without `--id`, the derived-next-in-series behavior is unchanged. Tests cover the four faces of the contract in one place: - explicit id is honored verbatim (and no derived id is picked instead); - a colliding id is refused with the existing "already exists" message, no second block appended; - nine malformed ids are refused with actionable messages; - the default path still returns the next id in the existing series. Only #880's ask (1) is addressed here. #887's asks (2) and (3) — reject `--field id=` naming a first-class field, and preserve hyphens through `next-id` — are separate PRs. Closes #880. Implements: REQ-007 Verifies: REQ-007 Co-Authored-By: Claude Opus 4.7 Claude-Session: https://claude.ai/code/session_018ozkqbJopvcKoycPR8zHEW --- rivet-cli/src/main.rs | 77 +++++++++++- rivet-cli/tests/cli_commands.rs | 202 ++++++++++++++++++++++++++++++++ 2 files changed, 273 insertions(+), 6 deletions(-) diff --git a/rivet-cli/src/main.rs b/rivet-cli/src/main.rs index aef0ed12..6e2d571a 100644 --- a/rivet-cli/src/main.rs +++ b/rivet-cli/src/main.rs @@ -1140,6 +1140,16 @@ enum Command { #[arg(long)] title: String, + /// Choose the artifact ID explicitly (e.g. `REQ-DRV-GRAPH-001`). Must + /// match `PREFIX-NNN` shape (uppercase letters/digits, dash-separated + /// segments, ending with a numeric suffix) and be unique in the store. + /// When omitted, `rivet add` picks the next ID for the type — that + /// derivation ignores tags, title and target file, so a name that a + /// later reader would need to un-learn is one of the cases this flag + /// exists for (#880). + #[arg(long)] + id: Option, + /// Artifact description #[arg(long)] description: Option, @@ -2885,6 +2895,7 @@ fn run(cli: Cli) -> Result { Command::Add { r#type, title, + id, description, status, tags, @@ -2897,6 +2908,7 @@ fn run(cli: Cli) -> Result { &cli, r#type, title, + id.as_deref(), description.as_deref(), status, tags, @@ -17277,12 +17289,59 @@ fn cmd_next_id( Ok(true) } +/// #880: shape check for an explicitly-chosen artifact ID (`rivet add --id`). +/// +/// Every ID in a rivet store is `PREFIX-NNN` — one or more uppercase +/// alphanumeric segments separated by single dashes, ending with a purely +/// numeric suffix (e.g. `REQ-001`, `REQ-DRV-GRAPH-001`, `FIND-DMA-SHM-CANONICAL-001`). +/// Uniqueness is not checked here — `validate_add` does that against the store. +fn validate_explicit_id(id: &str) -> Result<()> { + if id.is_empty() { + anyhow::bail!("--id must not be empty"); + } + for c in id.chars() { + if !(c.is_ascii_uppercase() || c.is_ascii_digit() || c == '-') { + anyhow::bail!( + "--id '{id}' has invalid character '{c}'. Use uppercase letters, digits, and dashes only (e.g. REQ-DRV-GRAPH-001)" + ); + } + } + if id.starts_with('-') || id.ends_with('-') || id.contains("--") { + anyhow::bail!( + "--id '{id}' must not start or end with '-', or contain '--' (e.g. REQ-DRV-GRAPH-001)" + ); + } + let Some(dash_pos) = id.rfind('-') else { + anyhow::bail!( + "--id '{id}' must have a PREFIX-NNN shape with a numeric suffix (e.g. REQ-DRV-GRAPH-001)" + ); + }; + let (prefix, suffix) = (&id[..dash_pos], &id[dash_pos + 1..]); + if prefix.is_empty() { + anyhow::bail!("--id '{id}' has an empty prefix; use PREFIX-NNN (e.g. REQ-DRV-GRAPH-001)"); + } + if suffix.is_empty() || !suffix.chars().all(|c| c.is_ascii_digit()) { + anyhow::bail!( + "--id '{id}' must end with a numeric suffix after the last '-' (e.g. REQ-DRV-GRAPH-001)" + ); + } + // A prefix must itself contain a letter so a caller cannot register a + // whole-digit "prefix" like "1-2" that would confuse next-id later. + if !prefix.chars().any(|c| c.is_ascii_uppercase()) { + anyhow::bail!( + "--id '{id}' prefix '{prefix}' must contain at least one uppercase letter (e.g. REQ-DRV-GRAPH-001)" + ); + } + Ok(()) +} + /// Add a new artifact to the project. #[allow(clippy::too_many_arguments)] fn cmd_add( cli: &Cli, artifact_type: &str, title: &str, + explicit_id: Option<&str>, description: Option<&str>, status: &str, tags: &[String], @@ -17328,12 +17387,18 @@ fn cmd_add( ctx.ensure_no_parse_skips(cli, "add an artifact")?; let (store, schema) = (ctx.store, ctx.schema); - // Resolve prefix for the type - let prefix = mutate::prefix_for_type(artifact_type, &store); - - // Generate ID (git-aware: never reissue an ID burned by an open branch or - // a reverted commit — #479). - let id = next_id_git_aware(&cli.project, &store, &prefix); + // #880: `--id` lets the caller name the artifact directly. Shape-validated + // here; uniqueness is validated below by `validate_add`. Without `--id`, we + // resolve the prefix for the type and pick the next number (git-aware: + // never reissue an ID burned by an open branch or a reverted commit — + // #479). + let id = if let Some(explicit) = explicit_id { + validate_explicit_id(explicit)?; + explicit.to_string() + } else { + let prefix = mutate::prefix_for_type(artifact_type, &store); + next_id_git_aware(&cli.project, &store, &prefix) + }; // Build fields map let mut fields_map: BTreeMap = BTreeMap::new(); diff --git a/rivet-cli/tests/cli_commands.rs b/rivet-cli/tests/cli_commands.rs index 2caac673..9f1a844a 100644 --- a/rivet-cli/tests/cli_commands.rs +++ b/rivet-cli/tests/cli_commands.rs @@ -10601,3 +10601,205 @@ fn coverage_declares_unmodelled_rules_and_fails_when_stale() { its reason is the defect it replaced; got:\n{stale_text}" ); } + +// ── #880: rivet add --id — the caller names the artifact ──────────────── +// +// Without --id, `rivet add` picks the next number in some pre-existing series, +// and title/tags/target-file/--field id= all fail to influence it. The issue +// documents the workaround (hand-edit the id: line) as the exact thing the +// project's own guidance forbids. --id closes the workaround: the caller +// asserts the ID, and the tool validates shape and uniqueness before writing. + +fn write_min_project(dir: &std::path::Path) { + std::fs::write( + dir.join("rivet.yaml"), + "project:\n name: t\n version: \"0.1.0\"\n schemas: [common, dev]\n\ + sources:\n - path: artifacts\n format: generic-yaml\n", + ) + .unwrap(); + std::fs::create_dir_all(dir.join("artifacts")).unwrap(); + std::fs::write( + dir.join("artifacts").join("r.yaml"), + "artifacts:\n \ + - id: REQ-PIN-001\n type: requirement\n title: seed\n status: draft\n", + ) + .unwrap(); +} + +/// #880 happy path: `--id REQ-DRV-GRAPH-001` is honored verbatim on a repo +/// whose existing series is REQ-PIN-*, defeating the derived-ID surprise the +/// issue documents. +#[test] +fn add_id_flag_honors_explicit_id() { + let tmp = tempfile::tempdir().expect("temp dir"); + let dir = tmp.path(); + write_min_project(dir); + + let out = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "add", + "--type", + "requirement", + "--title", + "The assembler is released, or a layers repository has nothing to consume", + "--status", + "draft", + "--id", + "REQ-DRV-GRAPH-001", + ]) + .output() + .expect("rivet add"); + assert!( + out.status.success(), + "add --id must succeed on a well-formed unique id; stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + stdout.contains("REQ-DRV-GRAPH-001"), + "the printed id must be the caller's; got: {stdout}" + ); + let yaml = std::fs::read_to_string(dir.join("artifacts").join("r.yaml")).unwrap(); + assert!( + yaml.contains("id: REQ-DRV-GRAPH-001"), + "the file must carry the caller's id, not a derived one; got:\n{yaml}" + ); + assert!( + !yaml.contains("REQ-PIN-002"), + "no next-in-series id must be picked when --id is given; got:\n{yaml}" + ); +} + +/// #880 uniqueness: --id collides with an existing artifact -> hard error, +/// nothing written. Piggybacks on the existing validate_add uniqueness check +/// so behavior stays consistent with a hand-edited duplicate. +#[test] +fn add_id_flag_rejects_duplicate_id() { + let tmp = tempfile::tempdir().expect("temp dir"); + let dir = tmp.path(); + write_min_project(dir); + + let out = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "add", + "--type", + "requirement", + "--title", + "collides with the seed", + "--status", + "draft", + "--id", + "REQ-PIN-001", + ]) + .output() + .expect("rivet add"); + assert!( + !out.status.success(), + "duplicate --id must fail; stdout: {}", + String::from_utf8_lossy(&out.stdout) + ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains("already exists"), + "the error must name the uniqueness failure; got: {stderr}" + ); + let yaml = std::fs::read_to_string(dir.join("artifacts").join("r.yaml")).unwrap(); + let count = yaml.matches("REQ-PIN-001").count(); + assert_eq!( + count, 1, + "no second block may be appended on rejection; got:\n{yaml}" + ); +} + +/// #880 shape: a lowercase, whitespace-containing, or ill-shaped id is refused +/// up-front with a message a caller can act on. Bad-id inputs are grouped in +/// one test so the shape contract is discoverable in one place. +/// +/// Values are passed as `--id=VALUE` (equals form) because bare `--id -REQ-001` +/// would be consumed by clap as an unknown short flag before our validator saw +/// it. The equals form is the shape a caller reaches for once they hit that, +/// so testing it is closer to the real recovery path anyway. +#[test] +fn add_id_flag_rejects_malformed_ids() { + let cases: &[(&str, &str)] = &[ + ("REQ-drv-001", "invalid character"), + ("REQ 001", "invalid character"), + ("-REQ-001", "must not start or end with"), + ("REQ-001-", "must not start or end with"), + ("REQ--001", "must not"), + ("REQ", "numeric suffix"), + ("REQ-", "must not start or end with"), + ("-001", "must not start"), + ("001-002", "uppercase letter"), + ]; + for (bad, needle) in cases { + let tmp = tempfile::tempdir().expect("temp dir"); + let dir = tmp.path(); + write_min_project(dir); + + let out = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "add", + "--type", + "requirement", + "--title", + "shape check", + "--status", + "draft", + &format!("--id={bad}"), + ]) + .output() + .expect("rivet add"); + assert!( + !out.status.success(), + "malformed --id '{bad}' must fail; stdout: {}", + String::from_utf8_lossy(&out.stdout) + ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains(needle), + "malformed --id '{bad}' error must mention '{needle}'; got: {stderr}" + ); + } +} + +/// #880 default preserved: without --id, the derived-next-in-series behavior is +/// unchanged. The regression guard here is that the shape check does not fire +/// on the derived id — otherwise the flag would break every existing caller. +#[test] +fn add_without_id_still_derives_next_in_series() { + let tmp = tempfile::tempdir().expect("temp dir"); + let dir = tmp.path(); + write_min_project(dir); + + let out = Command::new(rivet_bin()) + .args([ + "--project", + dir.to_str().unwrap(), + "add", + "--type", + "requirement", + "--title", + "second seed", + "--status", + "draft", + ]) + .output() + .expect("rivet add"); + assert!( + out.status.success(), + "default add must still succeed; stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout).trim().to_string(); + assert_eq!( + stdout, "REQ-PIN-002", + "derived id must continue the existing series" + ); +}