ref:d1e839c32ce788bb5fc9fdcbf47716c5b063c16e

fix(cli): correctness + safety fixes from the epic code review (#329)

Four confirmed findings, each covered by a test: - `seed --clear` is destructive-with-guardrails now: it shows the blast radius and requires an interactive confirmation on a TTY or an explicit --yes otherwise (it previously hard-deleted every requirement in the repo with no prompt — a semantics escalation from the old warn-only stub). - `--clear` refuses servers whose list API doesn't expose parent_id: without it the children-first ordering silently degrades to requirement_id order, parents cascade-soft-delete their children, and the clear strands half-done. Clear message to upgrade instead. - `--clear` now fetches with include_deleted=true and hard-deletes soft-deleted rows too — they still occupy the unique (repository_id, requirement_id) index, so skipping them reproduced the exact 422-and-skip failure the flag exists to eliminate (server support on the anvil branch). - Label names are percent-encoded in URL paths (edit/delete + the pre-existing issue-label remove): "area/ci" used to split into two path segments and 404; "bug#2" truncated at the fragment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SHA: d1e839c32ce788bb5fc9fdcbf47716c5b063c16e
Author: CI <ci@anvil.test>
Date: 2026-07-07 07:57
Parents: 36d8d5a
3 files changed +177 -11
Type
src/commands/label.rs +8 −3
@@ -2,6 +2,7 @@
use crate::config;
use crate::output;
use clap::{Args, Subcommand};
use percent_encoding::{utf8_percent_encode, NON_ALPHANUMERIC};
use serde::Deserialize;
#[derive(Args)]
@@ -190,8 +191,10 @@
if let Some(v) = description {
payload["description"] = serde_json::json!(v);
}
// Label names commonly contain '/' (area/ci) — encode the path segment.
let encoded = utf8_percent_encode(label_name, NON_ALPHANUMERIC).to_string();
let resp: serde_json::Value = client
.patch(&format!("/{org}/{name}/labels/{label_name}"), &payload)
.patch(&format!("/{org}/{name}/labels/{encoded}"), &payload)
.await?;
if output::is_json() {
output::json_ok("label", resp);
@@ -204,8 +207,9 @@
async fn delete(repo: Option<&str>, label_name: &str) -> Result<(), Box<dyn std::error::Error>> {
let client = Client::from_config()?;
let (org, name) = config::resolve_repo(repo)?;
let encoded = utf8_percent_encode(label_name, NON_ALPHANUMERIC).to_string();
client
.delete_empty(&format!("/{org}/{name}/labels/{label_name}"))
.delete_empty(&format!("/{org}/{name}/labels/{encoded}"))
.await?;
if output::is_json() {
output::json_ok("deleted", serde_json::json!(label_name));
@@ -248,9 +252,10 @@
) -> Result<(), Box<dyn std::error::Error>> {
let client = Client::from_config()?;
let (org, name) = config::resolve_repo(repo)?;
let encoded = utf8_percent_encode(label_name, NON_ALPHANUMERIC).to_string();
client
.delete_empty(&format!(
"/{org}/{name}/issues/{issue_number}/labels/{label_name}"
"/{org}/{name}/issues/{issue_number}/labels/{encoded}"
))
.await?;
if output::is_json() {
src/commands/requirement.rs +147 −8
@@ -157,9 +157,14 @@
/// Repository (org/repo)
#[arg(long)]
repo: Option<String>,
/// HARD-DELETE every requirement in the repo (including soft-deleted
/// ones) before seeding. Destructive and irreversible — prompts for
/// confirmation on a terminal; requires --yes otherwise.
/// Clear existing requirements before seeding
#[arg(long)]
clear: bool,
/// Skip the --clear confirmation prompt (for scripts/CI)
#[arg(long)]
yes: bool,
},
/// Bulk-import requirements from a YAML/JSON file (or stdin with "-")
Import {
@@ -455,7 +460,7 @@
.await
}
RequirementCommand::Status(args) => status(args).await,
RequirementCommand::Seed { repo, clear, yes } => seed(repo.as_deref(), clear, yes).await,
RequirementCommand::Seed { repo, clear } => seed(repo.as_deref(), clear).await,
RequirementCommand::Import {
file,
format,
@@ -1730,13 +1735,18 @@
children: Vec<ReqDef>,
}
async fn seed(repo: Option<&str>, clear: bool) -> Result<(), Box<dyn std::error::Error>> {
async fn seed(
repo: Option<&str>,
clear: bool,
yes: bool,
) -> Result<(), Box<dyn std::error::Error>> {
let client = Client::from_config()?;
let (org, name) = config::resolve_repo(repo)?;
let json = output::is_json();
let mut cleared = 0;
if clear {
confirm_clear(&client, &org, &name, yes).await?;
if !json {
output::warn("Clearing existing requirements (hard delete)...");
}
@@ -1860,18 +1870,84 @@
/// deleted. Children must go first: `hard_delete` cascade-soft-deletes
/// descendants, which the API then hides, so a parent must never be deleted
/// before its children.
/// --clear is irreversible (rows, version history, and test links all go).
/// Show the blast radius first, then require an explicit go-ahead: a TTY
/// prompt interactively, or --yes for scripts — never silently.
async fn confirm_clear(
async fn clear_requirements(
client: &Client,
org: &str,
name: &str,
yes: bool,
) -> Result<(), Box<dyn std::error::Error>> {
if yes {
return Ok(());
}
let count = fetch_all_requirements(client, org, name).await?.len();
if !std::io::IsTerminal::is_terminal(&std::io::stdin()) {
) -> Result<usize, Box<dyn std::error::Error>> {
let resp: serde_json::Value = client.get(&format!("/{org}/{name}/requirements")).await?;
let mut remaining: Vec<serde_json::Value> = resp
return Err(format!(
"--clear would HARD-DELETE all {count} requirement(s) in {org}/{name} \
(including version history and test links). Re-run with --yes to confirm."
)
.into());
}
let confirmed = dialoguer::Confirm::new()
.with_prompt(format!(
"HARD-DELETE all {count} requirement(s) in {org}/{name}? This cannot be undone"
))
.default(false)
.interact()?;
if confirmed {
Ok(())
} else {
Err("aborted — nothing was deleted".into())
}
}
/// The full slate, INCLUDING soft-deleted rows: they still occupy the unique
/// (repository_id, requirement_id) index, so a clear that skipped them would
/// leave every re-seeded create 422-ing — the exact failure --clear exists to
/// prevent.
async fn fetch_all_requirements(
client: &Client,
org: &str,
name: &str,
) -> Result<Vec<serde_json::Value>, Box<dyn std::error::Error>> {
let resp: serde_json::Value = client
.get_with_query(
&format!("/{org}/{name}/requirements"),
&[("include_deleted", "true")],
)
.await?;
Ok(resp
.get("requirements")
.and_then(|v| v.as_array())
.cloned()
.unwrap_or_default())
}
async fn clear_requirements(
client: &Client,
org: &str,
name: &str,
) -> Result<usize, Box<dyn std::error::Error>> {
.unwrap_or_default();
let mut remaining = fetch_all_requirements(client, org, name).await?;
// Children-first ordering depends on the list payload carrying parent_id.
// A server that predates that contract would make us delete parents first,
// whose hard-delete cascade-SOFT-deletes children and strands the clear
// half-done — refuse loudly instead of misbehaving destructively.
if !remaining.is_empty() && remaining.iter().all(|r| r.get("parent_id").is_none()) {
return Err(
"this server's requirements API does not expose parent_id, so --clear cannot \
order deletes safely. Upgrade the Anvil server first."
.into(),
);
}
let mut cleared = 0;
while !remaining.is_empty() {
// UUIDs referenced as a parent by any still-present requirement.
@@ -2480,6 +2556,69 @@
assert_eq!(v["counts"]["total"], 2);
assert_eq!(v["failing"][0]["requirement_id"], "REQ-B");
assert_eq!(v["failing"][0]["coverage_status"], "uncovered");
}
#[tokio::test]
async fn clear_requirements_refuses_servers_without_parent_id() {
use serde_json::json;
use wiremock::matchers::{method, path};
use wiremock::{Mock, MockServer, ResponseTemplate};
let server = MockServer::start().await;
// Old server contract: entries carry no parent_id key at all.
Mock::given(method("GET"))
.and(path("/api/v1/o/r/requirements"))
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
"requirements": [
{"id": "P", "requirement_id": "REQ-P"},
{"id": "C", "requirement_id": "REQ-C"}
]
})))
.mount(&server)
.await;
let client = Client::for_test(server.uri(), "tok");
let err = clear_requirements(&client, "o", "r")
.await
.expect_err("must refuse to delete without ordering info");
assert!(err.to_string().contains("parent_id"), "got: {err}");
// Nothing was deleted.
let deletes = server
.received_requests()
.await
.unwrap()
.iter()
.filter(|r| r.method.as_str() == "DELETE")
.count();
assert_eq!(deletes, 0);
}
#[tokio::test]
async fn confirm_clear_requires_yes_when_not_a_tty() {
use serde_json::json;
use wiremock::matchers::{method, path};
use wiremock::{Mock, MockServer, ResponseTemplate};
let server = MockServer::start().await;
Mock::given(method("GET"))
.and(path("/api/v1/o/r/requirements"))
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
"requirements": [{"id": "P", "requirement_id": "REQ-P", "parent_id": null}]
})))
.mount(&server)
.await;
let client = Client::for_test(server.uri(), "tok");
// --yes short-circuits.
assert!(confirm_clear(&client, "o", "r", true).await.is_ok());
// cargo-test stdin is not a TTY → the prompt path must refuse.
let err = confirm_clear(&client, "o", "r", false)
.await
.expect_err("non-TTY without --yes must refuse");
assert!(err.to_string().contains("--yes"), "got: {err}");
}
#[tokio::test]
tests/json_output.rs +22 −0
@@ -830,3 +830,25 @@
assert_eq!(v["preview"]["create"][0]["requirement_id"], "REQ-A-001");
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn label_names_with_slashes_are_path_encoded() {
let server = MockServer::start().await;
// "area/ci" must arrive as ONE encoded segment, not two path segments.
Mock::given(method("DELETE"))
.and(path("/api/v1/test-org/test-repo/labels/area%2Fci"))
.respond_with(ResponseTemplate::new(204))
.mount(&server)
.await;
let out = run_json(
&server.uri(),
&["label", "delete", "area/ci", "--repo", "test-org/test-repo"],
);
assert!(
out.status.success(),
"stderr: {}",
String::from_utf8_lossy(&out.stderr)
);
assert_eq!(stdout_json(&out)["deleted"], "area/ci");
}