diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f90fbcf..7eae35d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,8 @@ first added in the same Unreleased section is not an entry: edit that entry inst ### Fixed - The starter tour and the site footer link to https://x.com/smarzban. A tour already on your board keeps its notes. +- `tsk list -p` and `tsk project archive|unarchive` refuse a name two projects share, like `tsk add`, instead of matching nothing. Thanks @s3rgiosan (#249). +- `tsk list --all --group project` labels projects that share a name by their shortest distinct path, such as `site-a/wordpress/wp-content`. Thanks @s3rgiosan (#249). - A bare token in an https backup URL is removed and masked even when the URL does not start at the scheme. - An open board shows the update notice as soon as the release check finishes, and a board left open checks again once a day. - A token in a backup URL stays out of git's command line and the backup's local clone, so other local users cannot see it. diff --git a/site/src/content/docs/docs/cli.md b/site/src/content/docs/docs/cli.md index a3931173..33a7abd8 100644 --- a/site/src/content/docs/docs/cli.md +++ b/site/src/content/docs/docs/cli.md @@ -71,7 +71,7 @@ Rows the board paints as `N1`, `N2`… are notices: starter tasks and release no - A bare name for `add` must match exactly one project the board knows: one with tasks, an archived one, or the repository you are in. Otherwise it refuses with `unknown-project`. - Only an absolute path to an existing directory creates a new project. Relative and missing paths refuse. -- `tsk list -p ` never refuses: an unknown name lists nothing. +- `tsk list -p ` lists nothing for an unknown name. A name that two or more projects share refuses with `unknown-project` and lists their paths: pass one of them instead. ## add @@ -182,7 +182,7 @@ tsk list --all --thread release --json - Tasks group by status in the order `STARTED`, `READY`, `OPEN`, `BLOCKED`, `REVIEW`, and keep the board's order inside each status. - A row reads `- 12 Fix login timeout @claude ⎇ main #auth`. A blocked row adds `blocked: ` beneath. - With `--all`, each status groups its rows by project and desk. A sort other than `recent` lists the rows flat instead, each ending with its scope. -- With `--group`, sub-headings name each group as the board paints it. +- With `--group`, sub-headings name each group as the board paints it. Projects that share a name read as their shortest distinct path, such as `site-a/wordpress/wp-content`. - One task in full adds its notes, steps, `@assignee`, `⎇ base`, `after` and `before` tasks, `#thread`, and its latest five [paper trail](/docs/task-page/#paper-trail) entries. - Text wraps to the terminal width. Redirected output keeps each line whole. @@ -593,7 +593,7 @@ tsk project archive [--state-dir ] tsk project unarchive [--state-dir ] ``` -Output is `archived project ` or `unarchived project `. Repeating it is safe. A name with no tasks refuses: `no project named has tasks`. +Output is `archived project ` or `unarchived project `. Repeating it is safe. A name with no tasks refuses: `no project named has tasks`. A name that two or more projects share refuses with `project is ambiguous: `: pass one of the paths. **Refusals:** a message with no code, exit 1 ([exit contract](#exit-contract)). @@ -777,6 +777,7 @@ A refusal prints `tsk : : ` on stderr, such as `tsk stat | Command | Refusal codes | | --- | --- | | `add` | `empty-title`, `invalid-title`, `unknown-project`, `project-archived`, `unknown-base`; in a plan also `invalid-item`, `invalid-thread`, `invalid-assignee`, `invalid-after` | +| `list` | `unknown-project` | | `status` | `unknown-task`, `soft-deleted-task`, `text-too-long`, `invalid-blocker` | | `status … started` | `after-not-done`, `agent-gone`, and the `dispatch` codes `other-host`, `needs-git-project`, `unknown-agent`, `agent-config`, `unknown-base`, `no-default-base`, `host-failed`, `unsafe-state-dir`; on Windows `unsupported-platform` | | `status … done --clean` | The `clean` codes except `not-dispatched` and `already-cleaned` | diff --git a/skills/tsk-cli/SKILL.md b/skills/tsk-cli/SKILL.md index bbc2bd3f..5b8a1394 100644 --- a/skills/tsk-cli/SKILL.md +++ b/skills/tsk-cli/SKILL.md @@ -59,7 +59,8 @@ task's running agent, in Herdr or Rex, add `--send`: it sends only that reply an `dispatch`, `edit --append-notes`, `steps add`, `steps toggle`, and `steps remove` are not: read the task first. 4. **Use a known project name or a full path.** For `tsk add -p `, - unknown or ambiguous names refuse; `tsk list -p` lists nothing for them. Only an + unknown or ambiguous names refuse; `tsk list -p` lists nothing for an unknown name and + refuses an ambiguous one. Only an existing absolute directory creates a project ([scope](https://gettsk.sh/docs/cli.md#scope)). 5. **Never pass `--state-dir`** unless the user asked. It points at a different board. 6. **Ignore notice rows.** Rows the board paints as `N1`… are for the user; no command reaches them. diff --git a/src/cli/archive.rs b/src/cli/archive.rs index a171051d..7fc88d90 100644 --- a/src/cli/archive.rs +++ b/src/cli/archive.rs @@ -65,11 +65,19 @@ pub fn run_project( // Project verbs keep the old permissive path contract so outside-Git invocation // directories, removed directories, and legacy relative scopes do not alter how an // existing project is addressed. - let resolved = crate::scope::resolve_permissive_project_path( + let resolved = match crate::scope::resolve_permissive_project_path( &name, domain, Some(&crate::context::snapshot_from_env()), - ); + ) { + Ok(resolved) => resolved, + Err(error) => { + return Ok(( + Err(ArchiveCliError::UnknownProject(error.message(&name))), + false, + )) + } + }; let result = if archive { domain.archive_project(&resolved) } else { diff --git a/src/cli/list.rs b/src/cli/list.rs index 76b99d44..0bdbec15 100644 --- a/src/cli/list.rs +++ b/src/cli/list.rs @@ -59,6 +59,8 @@ pub enum ListError { Store(String), /// A well-formed task address that addresses no task in the store. UnknownTask, + /// A bare `-p` name that matches more than one project; the refusal message. + UnknownProject(String), } /// One task visible to the list command. @@ -439,13 +441,15 @@ pub fn run(input: ListInput) -> Result { }), }); } - let scope = (!input.all).then(|| match input.project.as_deref() { - Some(project) => TaskScope::Project { - path: resolve_permissive_project_path(project, &domain, Some(&snapshot_from_env())), - }, - None if input.global => TaskScope::Global, - None => snapshot_from_env().default_scope, - }); + let scope = match (input.all, input.project.as_deref()) { + (true, _) => None, + (false, Some(project)) => Some(TaskScope::Project { + path: resolve_permissive_project_path(project, &domain, Some(&snapshot_from_env())) + .map_err(|error| ListError::UnknownProject(error.message(project)))?, + }), + (false, None) if input.global => Some(TaskScope::Global), + (false, None) => Some(snapshot_from_env().default_scope), + }; let view = if input.deleted { ListView::Deleted } else if input.done { @@ -533,6 +537,9 @@ pub fn run(input: ListInput) -> Result { arranged.extend(rows.into_iter().map(|task| (label.clone(), task))); } } + if group == GroupBy::Project { + distinguish_project_groups(&mut arranged); + } arranged }; // Group labels name you as `[board] me` for the text listing; JSON keeps the stored @@ -673,6 +680,40 @@ fn is_open(status: HumanStatus) -> bool { ) } +/// Relabel `--group project` groups whose basenames collide with the shortest unique path +/// suffix, so two `wp-content` projects read as two groups. Equivalent spellings of one +/// project (`/tmp/x`, `/private/tmp/x`) share one group and so one label. +fn distinguish_project_groups(arranged: &mut [(Option, &crate::domain::Task)]) { + let identities = PathIdentityCache::default(); + // One representative spelling per project identity, the first one listed. + let mut representatives: Vec = Vec::new(); + let mut representative_of = |path: &str| -> String { + match representatives + .iter() + .find(|known| identities.equivalent(known, path)) + { + Some(known) => known.clone(), + None => { + representatives.push(path.to_string()); + path.to_string() + } + } + }; + let keys = arranged + .iter() + .map(|(_, task)| match &task.scope { + TaskScope::Project { path } => Some(representative_of(path)), + TaskScope::Global => None, + }) + .collect::>(); + let labels = crate::cli::presenter::distinct_scope_labels(keys.iter().cloned()); + for ((label, _), key) in arranged.iter_mut().zip(&keys) { + if key.is_some() { + *label = labels.get(key).cloned(); + } + } +} + fn status_group_rank(status: HumanStatus) -> u8 { match status { HumanStatus::Started => 0, diff --git a/src/cli/presenter.rs b/src/cli/presenter.rs index 98c78273..da97459d 100644 --- a/src/cli/presenter.rs +++ b/src/cli/presenter.rs @@ -374,8 +374,12 @@ pub fn list_help(_terminal_width: Option) -> CliOutput { "tsk list --all --group project --sort title".into(), "tsk list --archived --all".into(), ], - refusals: Vec::new(), - exit: exit_line("tasks listed", None, true), + refusals: vec!["unknown-project: a -p name shared by two or more projects".into()], + exit: exit_line( + "tasks listed", + Some("a -p name matches more than one project"), + true, + ), }) } @@ -1273,26 +1277,30 @@ fn append_wrapped( } fn scope_labels(rows: &[ListRow]) -> BTreeMap, String> { + distinct_scope_labels(rows.iter().map(|row| row.project.clone())) +} + +/// One distinct label per scope (`None` is the desk): a project's basename, widened to +/// the shortest unique path suffix where basenames collide (`site-a/wordpress/wp-content`). +pub(crate) fn distinct_scope_labels( + scopes: impl IntoIterator>, +) -> BTreeMap, String> { let mut entries = Vec::new(); let mut empty_projects = 0; - for row in rows { + for scope in scopes { if entries .iter() - .any(|entry: &ScopeLabel| entry.scope == row.project) + .any(|entry: &ScopeLabel| entry.scope == scope) { continue; } - let empty_number = if row - .project - .as_deref() - .is_some_and(|path| path.trim().is_empty()) - { + let empty_number = if scope.as_deref().is_some_and(|path| path.trim().is_empty()) { empty_projects += 1; empty_projects } else { 0 }; - entries.push(ScopeLabel::new(row.project.clone(), empty_number)); + entries.push(ScopeLabel::new(scope, empty_number)); } loop { @@ -2475,10 +2483,13 @@ pub fn project_help() -> CliOutput { "tsk project archive atlas".into(), "tsk project unarchive atlas".into(), ], - refusals: vec!["no project named has tasks".into()], + refusals: vec![ + "no project named has tasks".into(), + "project is ambiguous: ".into(), + ], exit: exit_line( "project archived or unarchived, or already was", - Some("no project of that name has tasks"), + Some("no project of that name has tasks, or the name is ambiguous"), true, ), }) @@ -2559,6 +2570,14 @@ pub fn list_rejected(error: ListError, terminal_width: Option) -> CliOutp }, // A well-formed address that addresses no task: the invocation is wrong, not the store. ListError::UnknownTask => list_usage("unknown task", terminal_width), + ListError::UnknownProject(detail) => CliOutput { + stdout: String::new(), + stderr: wrap_list_document( + &format!("tsk list: unknown-project: {}\n", terminal_text(&detail)), + terminal_width, + ), + code: 1, + }, } } diff --git a/src/scope.rs b/src/scope.rs index 5f8495e3..a0111f0e 100644 --- a/src/scope.rs +++ b/src/scope.rs @@ -506,16 +506,23 @@ pub fn adopt_stored_identity(snapshot: &mut InvocationSnapshot, domain: &DomainS } /// Resolve with the pre-T82 permissive rules used by read and project archive verbs. -/// Slash paths stay literal, and missing or ambiguous bare names stay literal. +/// Slash paths and bare names matching no project stay literal, so legacy relative +/// scopes and removed directories stay addressable. A bare name matching two or more +/// distinct projects refuses like `add` does, rather than silently matching nothing. pub(crate) fn resolve_permissive_project_path( token: &str, domain: &DomainState, snapshot: Option<&InvocationSnapshot>, -) -> String { +) -> Result { if has_path_separator(token) { - return token.to_string(); + return Ok(token.to_string()); } let stored = stored_project_paths(domain); + // A legacy bare scope (`atlas`) is addressable only by its own spelling, so an exact + // stored match wins before a same-basename `/…/atlas` can make it ambiguous. + if stored.contains(token) { + return Ok(token.to_string()); + } let mut candidates = stored.clone(); if let Some(snapshot) = snapshot { // Outside Git, the invocation directory was not historically a basename alias. @@ -540,8 +547,12 @@ pub(crate) fn resolve_permissive_project_path( } } match matches.as_slice() { - [path] => stored_spelling(domain, path).unwrap_or_else(|| path.clone()), - _ => token.to_string(), + [path] => Ok(stored_spelling(domain, path).unwrap_or_else(|| path.clone())), + [] => Ok(token.to_string()), + _ => { + matches.sort(); + Err(ProjectResolveError::Ambiguous(matches)) + } } } @@ -692,7 +703,48 @@ mod portable_path_tests { let domain = DomainState::new(); assert_eq!( resolve_permissive_project_path(r"C:\work\tsk", &domain, None), - r"C:\work\tsk" + Ok(r"C:\work\tsk".to_string()) + ); + } + + fn domain_with_scopes(paths: &[&str]) -> DomainState { + let mut domain = DomainState::new(); + for path in paths { + domain + .create( + "fixture", + None, + TaskScope::Project { + path: (*path).to_string(), + }, + crate::domain::ProvenanceOrigin::Manual, + None, + ) + .expect("create project fixture"); + } + domain + } + + #[test] + fn permissive_resolution_keeps_a_legacy_bare_scope_addressable_beside_a_namesake() { + let domain = domain_with_scopes(&["atlas", "/projects/tsk-missing-namesake/atlas"]); + assert_eq!( + resolve_permissive_project_path("atlas", &domain, None), + Ok("atlas".to_string()) + ); + } + + #[test] + fn permissive_resolution_refuses_a_basename_two_absolute_projects_share() { + let domain = domain_with_scopes(&[ + "/projects/tsk-missing-b/atlas", + "/projects/tsk-missing-a/atlas", + ]); + let error = resolve_permissive_project_path("atlas", &domain, None) + .expect_err("two absolute namesakes are ambiguous"); + assert_eq!( + error.message("atlas"), + "project atlas is ambiguous: /projects/tsk-missing-a/atlas, /projects/tsk-missing-b/atlas" ); } diff --git a/src/setup_agent.rs b/src/setup_agent.rs index 9dc77cd4..8dc38def 100644 --- a/src/setup_agent.rs +++ b/src/setup_agent.rs @@ -1309,7 +1309,7 @@ mod tests { hash = hash.wrapping_mul(0x0000_0100_0000_01b3); } assert_eq!( - hash, 0x0b2e_4132_6342_cac0, + hash, 0xaa9c_141f_5d1a_30fb, "skills/tsk-cli/SKILL.md changed: refresh this hash pin, and bump `version:` only if this is the first skill edit since the last release" ); } diff --git a/tests/cli_archive.rs b/tests/cli_archive.rs index e4085215..b52bc08b 100644 --- a/tests/cli_archive.rs +++ b/tests/cli_archive.rs @@ -342,3 +342,58 @@ fn unarchive_refusals_name_the_unarchive_verb() { unknown.stderr ); } + +#[test] +fn project_archive_refuses_a_basename_shared_by_two_projects() { + let dir = temp_state_dir("project-ambiguous"); + let _guard = TempDirGuard(dir.clone()); + let mut paths = Vec::new(); + for site in ["site-a", "site-b"] { + let project = dir.join(site).join("wordpress").join("wp-content"); + fs::create_dir_all(&project).expect("create project directory"); + let path = project.to_string_lossy().into_owned(); + let added = cli(vec![ + "tsk".into(), + "add".into(), + "-t".into(), + format!("{site} task"), + "-p".into(), + path.clone(), + "--state-dir".into(), + dir.to_string_lossy().into_owned(), + ]); + assert_eq!(added.code, 0, "{:?}", added.stderr); + paths.push(path); + } + let project = |verb: &str, name: &str| { + cli(vec![ + "tsk".into(), + "project".into(), + verb.into(), + name.into(), + "--state-dir".into(), + dir.to_string_lossy().into_owned(), + ]) + }; + + for verb in ["archive", "unarchive"] { + let out = project(verb, "wp-content"); + assert_eq!(out.code, 1, "{verb}: {out:?}"); + assert!(out.stdout.is_empty(), "{verb}: {:?}", out.stdout); + assert_eq!( + out.stderr, + format!( + "tsk project: project wp-content is ambiguous: {}, {}\n", + paths[0], paths[1] + ), + "{verb}" + ); + } + let store = TaskStore::new(&dir).load().expect("load store"); + assert!(store.projects().is_empty(), "nothing archived"); + + // A full path still addresses one of them. + let one = project("archive", &paths[0]); + assert_eq!(one.code, 0, "{one:?}"); + assert_eq!(one.stdout, "archived project wp-content\n"); +} diff --git a/tests/cli_list.rs b/tests/cli_list.rs index 3e9fbe73..ace549fa 100644 --- a/tests/cli_list.rs +++ b/tests/cli_list.rs @@ -3817,3 +3817,166 @@ fn list_group_and_sort_by_assignee_match_the_board() { let _ = std::fs::remove_dir_all(dir); } + +/// Two real project directories sharing the basename `wp-content`, one task in each. +fn seed_colliding_basenames(label: &str) -> (PathBuf, String, String) { + let dir = temp_state_dir(label); + let mut paths = Vec::new(); + for site in ["site-a", "site-b"] { + let project = dir.join(site).join("wordpress").join("wp-content"); + std::fs::create_dir_all(&project).expect("create project directory"); + let path = project.to_string_lossy().into_owned(); + let added = list(&[ + "tsk".into(), + "add".into(), + "-t".into(), + format!("{site} task"), + "-p".into(), + path.clone(), + "--state-dir".into(), + state_dir_arg(&dir), + ]); + assert_eq!(added.code, 0, "{}", added.stderr); + paths.push(path); + } + let b = paths.pop().expect("site-b"); + let a = paths.pop().expect("site-a"); + (dir, a, b) +} + +#[test] +fn list_project_refuses_a_basename_shared_by_two_projects() { + let _env = env_lock(); + let (dir, a, b) = seed_colliding_basenames("ambiguous-p"); + for args in [vec!["-p", "wp-content"], vec!["-p", "WP-CONTENT", "--json"]] { + let output = list_at_width(&list_args(&dir, &args), 400); + assert_eq!(output.code, 1, "{args:?}: {output:?}"); + assert!(output.stdout.is_empty(), "{args:?}: {}", output.stdout); + assert_eq!( + output.stderr, + format!( + "tsk list: unknown-project: project {} is ambiguous: {a}, {b}\n", + args[1] + ), + "{args:?}" + ); + } + + // A full path still lists one project, and an unknown name still lists nothing. + let one = json_rows(&list(&list_args(&dir, &["-p", a.as_str(), "--json"]))); + assert_eq!(titles(&one), vec!["site-a task"]); + let none = json_rows(&list(&list_args(&dir, &["-p", "nothing-here", "--json"]))); + assert!(none.is_empty()); + + let _ = std::fs::remove_dir_all(dir); +} + +#[test] +fn list_group_project_labels_colliding_basenames_by_their_shortest_unique_suffix() { + let _env = env_lock(); + let (dir, _a, _b) = seed_colliding_basenames("ambiguous-group"); + + let rows = json_rows(&list(&list_args( + &dir, + &["--all", "--group", "project", "--sort", "title", "--json"], + ))); + let mut labelled = rows + .iter() + .map(|row| { + ( + row["title"].as_str().expect("title").to_owned(), + row["group"].as_str().expect("group").to_owned(), + ) + }) + .collect::>(); + labelled.sort(); + assert_eq!( + labelled, + vec![ + ( + "site-a task".to_string(), + "site-a/wordpress/wp-content".to_string() + ), + ( + "site-b task".to_string(), + "site-b/wordpress/wp-content".to_string() + ), + ] + ); + + let human = list_at_width(&list_args(&dir, &["--all", "--group", "project"]), 400); + assert_eq!(human.code, 0, "{}", human.stderr); + for site in ["site-a", "site-b"] { + assert!( + human + .stdout + .contains(&format!(" {site}/wordpress/wp-content\n - ")), + "{}", + human.stdout + ); + } + assert!( + !human.stdout.contains("\n wp-content\n"), + "{}", + human.stdout + ); + + let _ = std::fs::remove_dir_all(dir); +} + +#[cfg(unix)] +#[test] +fn list_group_project_keeps_equivalent_spellings_in_one_labelled_group() { + let _env = env_lock(); + let dir = temp_state_dir("equivalent-group"); + let real = dir.join("real").join("wp-content"); + let other = dir.join("other").join("wp-content"); + std::fs::create_dir_all(&real).expect("create real project"); + std::fs::create_dir_all(&other).expect("create other project"); + let alias_parent = dir.join("alias"); + std::fs::create_dir(&alias_parent).expect("create alias parent"); + let alias = alias_parent.join("wp-content"); + std::os::unix::fs::symlink(&real, &alias).expect("symlink alias"); + + let mut state = DomainState::new(); + for (title, path) in [ + ("real task", &real), + ("alias task", &alias), + ("other task", &other), + ] { + create_task( + &mut state, + title, + TaskScope::Project { + path: path.to_string_lossy().into_owned(), + }, + HumanStatus::Open, + ); + } + TaskStore::new(&dir).save(&state).expect("seed store"); + + let rows = json_rows(&list(&list_args( + &dir, + &["--all", "--group", "project", "--json"], + ))); + let group_of = |title: &str| { + rows.iter() + .find(|row| row["title"] == title) + .map(|row| row["group"].as_str().expect("group").to_owned()) + .expect("row") + }; + assert_eq!(group_of("real task"), group_of("alias task"), "{rows:?}"); + assert_ne!(group_of("real task"), group_of("other task"), "{rows:?}"); + assert_eq!(group_of("other task"), "other/wp-content"); + + let human = list_at_width(&list_args(&dir, &["--all", "--group", "project"]), 400); + assert_eq!(human.code, 0, "{}", human.stderr); + let headings = human + .stdout + .lines() + .filter(|line| line.starts_with(" ") && !line.starts_with(" ")) + .collect::>(); + assert_eq!(headings.len(), 2, "{}", human.stdout); + + let _ = std::fs::remove_dir_all(dir); +}