From 8f0b7064fcc6d7425c608f2ec7304ad9cd941926 Mon Sep 17 00:00:00 2001 From: Hare Date: Sat, 6 Jun 2026 05:56:55 +0900 Subject: [PATCH] review: approve yoi ticket cli --- .../artifacts/implementation-report.md | 81 +++++++++ .../artifacts/review.md | 62 +++++++ .../item.md | 2 +- .../thread.md | 159 ++++++++++++++++++ 4 files changed, 303 insertions(+), 1 deletion(-) create mode 100644 work-items/open/20260605-203006-yoi-ticket-cli-parity/artifacts/implementation-report.md create mode 100644 work-items/open/20260605-203006-yoi-ticket-cli-parity/artifacts/review.md diff --git a/work-items/open/20260605-203006-yoi-ticket-cli-parity/artifacts/implementation-report.md b/work-items/open/20260605-203006-yoi-ticket-cli-parity/artifacts/implementation-report.md new file mode 100644 index 00000000..20506362 --- /dev/null +++ b/work-items/open/20260605-203006-yoi-ticket-cli-parity/artifacts/implementation-report.md @@ -0,0 +1,81 @@ +# Implementation report: yoi-ticket-cli-parity + +## Worktree / branch + +- Worktree: `/home/hare/Projects/yoi/.worktree/yoi-ticket-cli-parity` +- Branch: `work/yoi-ticket-cli-parity` + +## Commit + +- `4d5068b feat: add yoi ticket CLI` + +## Summary + +Added `yoi ticket ...` subcommands to the product `yoi` binary crate, backed directly by the Rust Ticket backend APIs. + +The implementation does not shell out to `tickets.sh`. Active storage remains unchanged for this ticket: when `.yoi/ticket.config.toml` is absent, the CLI uses the current default `/work-items`. + +## Implemented command surface + +- `yoi ticket create --title [--slug <slug>] [--kind <kind>] [--priority P2] [--label a,b]` +- `yoi ticket list [--status open|pending|closed|all]` +- `yoi ticket show <id-or-slug>` +- `yoi ticket comment <id-or-slug> [--role comment|plan|decision|implementation_report] (--file <path>|--message <text>)` +- `yoi ticket review <id-or-slug> (--approve|--request-changes) (--file <path>|--message <text>)` +- `yoi ticket status <id-or-slug> <open|pending|closed>` +- `yoi ticket close <id-or-slug> (--resolution <text>|--file <path>)` +- `yoi ticket doctor` + +`yoi ticket status ... closed` is intentionally rejected with guidance to use `yoi ticket close`, because status-only close would not write `resolution.md`. + +## Backend behavior + +- Uses `ticket::config::TicketConfig::load_workspace(<cwd>)` to resolve backend root. +- Missing `.yoi/ticket.config.toml` defaults to `<cwd>/work-items`. +- Relative configured roots resolve against the workspace. +- Malformed config fails through the existing config loader. +- No storage migration to `.yoi/tickets` is included. + +## Changed files + +- `Cargo.lock` +- `crates/ticket/src/lib.rs` +- `crates/yoi/Cargo.toml` +- `crates/yoi/src/main.rs` +- `crates/yoi/src/ticket_cli.rs` +- `package.nix` + +## Review status + +External sibling reviewer approved with no blockers. + +Non-blocker follow-ups: + +- `yoi ticket doctor` currently hides warning-only diagnostics because it prints `doctor: ok` when `error_count() == 0`. +- `show`, `list`, and doctor diagnostic output are not explicitly bounded. +- Body-source error text is generic and can mention `--resolution` for comment/review commands. + +## Validation + +Coder-reported validation passed: + +- `cargo test -p yoi ticket` +- `cargo test -p ticket` +- `cargo check --workspace --all-targets` +- `cargo fmt --check` +- `git diff --check` +- `git diff --cached --check` +- `./tickets.sh doctor` +- `cargo build -p yoi` +- `target/debug/yoi ticket doctor` +- `nix build .#yoi --no-link` + +Reviewer-rerun validation passed: + +- `git diff --check develop...HEAD` +- `./tickets.sh doctor` +- production CLI grep found no shell-out to `tickets.sh`. + +## Ready for merge + +Yes. diff --git a/work-items/open/20260605-203006-yoi-ticket-cli-parity/artifacts/review.md b/work-items/open/20260605-203006-yoi-ticket-cli-parity/artifacts/review.md new file mode 100644 index 00000000..2c43a27f --- /dev/null +++ b/work-items/open/20260605-203006-yoi-ticket-cli-parity/artifacts/review.md @@ -0,0 +1,62 @@ +# External review: yoi-ticket-cli-parity + +## 1. Result + +approve + +## 2. Summary of implementation + +The implementation adds a new `yoi ticket ...` command surface in the product `yoi` binary crate, with parsing and dispatch in `crates/yoi/src/main.rs` and the command implementation in `crates/yoi/src/ticket_cli.rs`. + +The CLI resolves the local Ticket backend through `ticket::config::TicketConfig::load_workspace`, defaults to `<cwd>/work-items` when no `.yoi/ticket.config.toml` is present, and then calls `LocalTicketBackend` / `TicketBackend` operations directly. I found no production-path shell-out to `tickets.sh`; the only `tickets.sh` process invocations found are in `crates/ticket` compatibility tests. + +The changed files are scoped to the `yoi` CLI crate, the `ticket` backend doctor reporting shape, and necessary dependency/package hash updates. I did not find storage migration, `tickets.sh` removal, TUI changes, scheduler/lease work, or broad refactoring. + +## 3. Requirement-by-requirement assessment + +- `yoi ticket` subcommands cover create/list/show/comment/review/status/close/doctor: satisfied. `TicketCommand` contains all requested operations and `help_text()` documents them. +- Product CLI ownership in the `yoi` binary crate: satisfied. `main.rs` adds `Mode::Ticket` and dispatches `ticket` before normal TUI argument handling. +- Uses Rust Ticket backend APIs directly, no `tickets.sh` shell-out: satisfied for the product CLI path. `ticket_cli.rs` calls `LocalTicketBackend` and `TicketBackend` methods directly. +- Backend root resolution and active storage: satisfied for this ticket. The CLI uses `.yoi/ticket.config.toml` when present and otherwise defaults to `<cwd>/work-items`, preserving current local storage rather than moving to `.yoi/tickets`. +- `status closed` safety: satisfied. `status closed` is rejected with guidance to use `yoi ticket close <ticket> --resolution <text>`. +- `comment`, `review`, and `close` body source validation: satisfied. The parser requires exactly one body source and rejects conflicting/missing inputs; `review` also requires exactly one result flag. +- Doctor success/failure behavior: satisfied for errors. `TicketCliStatus::Failure` maps to a failing process exit code, and diagnostics are printed when errors are present. +- Human-useful output: broadly satisfied. Output is concise tabular/plain text for create/list/status and readable Markdown-like output for show. +- Bounded output: partially satisfied only by the natural size of the local backend; `show`, `list`, and doctor diagnostics do not impose explicit limits. I classify this as a follow-up because the requested CLI parity is still implemented and the current compatibility CLI is also not explicitly bounded. +- Tests in temp roots/fixtures: satisfied in implementation. `ticket_cli.rs` exercises core operations in `TempDir`, including configured backend root behavior and validation edge cases; `crates/ticket` keeps compatibility tests. +- `Cargo.lock` / `package.nix`: necessary and safe on inspection. Adding `ticket` to the `yoi` crate requires the lockfile package dependency update, and the Nix cargo hash update is expected from the Cargo metadata/source change. +- Non-goals: satisfied. I found no `.yoi/tickets` migration, `tickets.sh` removal, TUI change, scheduler/lease addition, or broad refactor. + +## 4. Blockers + +None. + +## 5. Non-blockers / follow-ups + +- `yoi ticket doctor` suppresses warning-only diagnostics because `doctor()` returns early with `doctor: ok` when `report.error_count() == 0`. If backend warnings are intended to be user-visible, the CLI should print warnings while still exiting successfully. This is not a merge blocker because the old `tickets.sh doctor` only had errors and the required failure behavior for errors is present. +- The CLI does not explicitly bound `show`, `list`, or diagnostic output. Consider adding limits later if this command is expected to be safe for very large Ticket stores or oversized thread bodies. +- The generic body-source error text says `--message/--resolution` for all commands, so comment/review errors mention `--resolution` even though that flag is only for close. The validation is correct; the wording can be improved in follow-up. + +## 6. Validation assessed or rerun + +Rerun/read-only validation: + +- `git diff --check develop...HEAD` — passed with no output. +- `./tickets.sh doctor` — passed: `doctor: ok`. +- `git status --short && git branch --show-current && git rev-parse --short=12 HEAD` — confirmed branch `work/yoi-ticket-cli-parity` at `4d5068ba3baf`; no worktree dirt was reported before this review artifact was written. +- `git grep -n "tickets.sh\|std::process::Command\|Command::new" -- crates/yoi crates/ticket` — no production CLI shell-out found; only `crates/ticket` compatibility tests invoke `tickets.sh`. + +Inspected: + +- Ticket and delegation intent. +- `crates/yoi/src/ticket_cli.rs`. +- `crates/yoi/src/main.rs`. +- `crates/ticket/src/lib.rs` diff. +- `crates/ticket/src/config.rs` backend default/config behavior. +- `crates/yoi/Cargo.toml`, `Cargo.lock` diff, and `package.nix` hash change. + +Not rerun: `cargo test`, `cargo check`, `cargo fmt --check`, built `yoi ticket doctor`, or `nix build .#yoi`; I stayed to read-only validation commands as requested for this external sibling review. + +## 7. Residual risk + +The main residual risk is not semantic parity but operational validation: I did not rerun the Rust or Nix builds, so the final merge owner should rely on the coder's validation evidence or rerun the full acceptance suite before merging. There is also minor UX risk around unbounded `show/list` output and warning-only doctor output being hidden. diff --git a/work-items/open/20260605-203006-yoi-ticket-cli-parity/item.md b/work-items/open/20260605-203006-yoi-ticket-cli-parity/item.md index f491d242..8249fdd0 100644 --- a/work-items/open/20260605-203006-yoi-ticket-cli-parity/item.md +++ b/work-items/open/20260605-203006-yoi-ticket-cli-parity/item.md @@ -7,7 +7,7 @@ kind: task priority: P1 labels: [ticket, cli, backend] created_at: 2026-06-05T20:30:06Z -updated_at: 2026-06-05T20:34:35Z +updated_at: 2026-06-05T20:56:42Z assignee: null legacy_ticket: null --- diff --git a/work-items/open/20260605-203006-yoi-ticket-cli-parity/thread.md b/work-items/open/20260605-203006-yoi-ticket-cli-parity/thread.md index 00cc6ba0..e5358d67 100644 --- a/work-items/open/20260605-203006-yoi-ticket-cli-parity/thread.md +++ b/work-items/open/20260605-203006-yoi-ticket-cli-parity/thread.md @@ -106,4 +106,163 @@ Report: - whether ready for external review. +--- + +<!-- event: review author: hare at: 2026-06-05T20:56:41Z status: approve --> + +## Review: approve + +# External review: yoi-ticket-cli-parity + +## 1. Result + +approve + +## 2. Summary of implementation + +The implementation adds a new `yoi ticket ...` command surface in the product `yoi` binary crate, with parsing and dispatch in `crates/yoi/src/main.rs` and the command implementation in `crates/yoi/src/ticket_cli.rs`. + +The CLI resolves the local Ticket backend through `ticket::config::TicketConfig::load_workspace`, defaults to `<cwd>/work-items` when no `.yoi/ticket.config.toml` is present, and then calls `LocalTicketBackend` / `TicketBackend` operations directly. I found no production-path shell-out to `tickets.sh`; the only `tickets.sh` process invocations found are in `crates/ticket` compatibility tests. + +The changed files are scoped to the `yoi` CLI crate, the `ticket` backend doctor reporting shape, and necessary dependency/package hash updates. I did not find storage migration, `tickets.sh` removal, TUI changes, scheduler/lease work, or broad refactoring. + +## 3. Requirement-by-requirement assessment + +- `yoi ticket` subcommands cover create/list/show/comment/review/status/close/doctor: satisfied. `TicketCommand` contains all requested operations and `help_text()` documents them. +- Product CLI ownership in the `yoi` binary crate: satisfied. `main.rs` adds `Mode::Ticket` and dispatches `ticket` before normal TUI argument handling. +- Uses Rust Ticket backend APIs directly, no `tickets.sh` shell-out: satisfied for the product CLI path. `ticket_cli.rs` calls `LocalTicketBackend` and `TicketBackend` methods directly. +- Backend root resolution and active storage: satisfied for this ticket. The CLI uses `.yoi/ticket.config.toml` when present and otherwise defaults to `<cwd>/work-items`, preserving current local storage rather than moving to `.yoi/tickets`. +- `status closed` safety: satisfied. `status closed` is rejected with guidance to use `yoi ticket close <ticket> --resolution <text>`. +- `comment`, `review`, and `close` body source validation: satisfied. The parser requires exactly one body source and rejects conflicting/missing inputs; `review` also requires exactly one result flag. +- Doctor success/failure behavior: satisfied for errors. `TicketCliStatus::Failure` maps to a failing process exit code, and diagnostics are printed when errors are present. +- Human-useful output: broadly satisfied. Output is concise tabular/plain text for create/list/status and readable Markdown-like output for show. +- Bounded output: partially satisfied only by the natural size of the local backend; `show`, `list`, and doctor diagnostics do not impose explicit limits. I classify this as a follow-up because the requested CLI parity is still implemented and the current compatibility CLI is also not explicitly bounded. +- Tests in temp roots/fixtures: satisfied in implementation. `ticket_cli.rs` exercises core operations in `TempDir`, including configured backend root behavior and validation edge cases; `crates/ticket` keeps compatibility tests. +- `Cargo.lock` / `package.nix`: necessary and safe on inspection. Adding `ticket` to the `yoi` crate requires the lockfile package dependency update, and the Nix cargo hash update is expected from the Cargo metadata/source change. +- Non-goals: satisfied. I found no `.yoi/tickets` migration, `tickets.sh` removal, TUI change, scheduler/lease addition, or broad refactor. + +## 4. Blockers + +None. + +## 5. Non-blockers / follow-ups + +- `yoi ticket doctor` suppresses warning-only diagnostics because `doctor()` returns early with `doctor: ok` when `report.error_count() == 0`. If backend warnings are intended to be user-visible, the CLI should print warnings while still exiting successfully. This is not a merge blocker because the old `tickets.sh doctor` only had errors and the required failure behavior for errors is present. +- The CLI does not explicitly bound `show`, `list`, or diagnostic output. Consider adding limits later if this command is expected to be safe for very large Ticket stores or oversized thread bodies. +- The generic body-source error text says `--message/--resolution` for all commands, so comment/review errors mention `--resolution` even though that flag is only for close. The validation is correct; the wording can be improved in follow-up. + +## 6. Validation assessed or rerun + +Rerun/read-only validation: + +- `git diff --check develop...HEAD` — passed with no output. +- `./tickets.sh doctor` — passed: `doctor: ok`. +- `git status --short && git branch --show-current && git rev-parse --short=12 HEAD` — confirmed branch `work/yoi-ticket-cli-parity` at `4d5068ba3baf`; no worktree dirt was reported before this review artifact was written. +- `git grep -n "tickets.sh\|std::process::Command\|Command::new" -- crates/yoi crates/ticket` — no production CLI shell-out found; only `crates/ticket` compatibility tests invoke `tickets.sh`. + +Inspected: + +- Ticket and delegation intent. +- `crates/yoi/src/ticket_cli.rs`. +- `crates/yoi/src/main.rs`. +- `crates/ticket/src/lib.rs` diff. +- `crates/ticket/src/config.rs` backend default/config behavior. +- `crates/yoi/Cargo.toml`, `Cargo.lock` diff, and `package.nix` hash change. + +Not rerun: `cargo test`, `cargo check`, `cargo fmt --check`, built `yoi ticket doctor`, or `nix build .#yoi`; I stayed to read-only validation commands as requested for this external sibling review. + +## 7. Residual risk + +The main residual risk is not semantic parity but operational validation: I did not rerun the Rust or Nix builds, so the final merge owner should rely on the coder's validation evidence or rerun the full acceptance suite before merging. There is also minor UX risk around unbounded `show/list` output and warning-only doctor output being hidden. + + +--- + +<!-- event: implementation_report author: hare at: 2026-06-05T20:56:42Z --> + +## Implementation report + +# Implementation report: yoi-ticket-cli-parity + +## Worktree / branch + +- Worktree: `/home/hare/Projects/yoi/.worktree/yoi-ticket-cli-parity` +- Branch: `work/yoi-ticket-cli-parity` + +## Commit + +- `4d5068b feat: add yoi ticket CLI` + +## Summary + +Added `yoi ticket ...` subcommands to the product `yoi` binary crate, backed directly by the Rust Ticket backend APIs. + +The implementation does not shell out to `tickets.sh`. Active storage remains unchanged for this ticket: when `.yoi/ticket.config.toml` is absent, the CLI uses the current default `<workspace>/work-items`. + +## Implemented command surface + +- `yoi ticket create --title <title> [--slug <slug>] [--kind <kind>] [--priority P2] [--label a,b]` +- `yoi ticket list [--status open|pending|closed|all]` +- `yoi ticket show <id-or-slug>` +- `yoi ticket comment <id-or-slug> [--role comment|plan|decision|implementation_report] (--file <path>|--message <text>)` +- `yoi ticket review <id-or-slug> (--approve|--request-changes) (--file <path>|--message <text>)` +- `yoi ticket status <id-or-slug> <open|pending|closed>` +- `yoi ticket close <id-or-slug> (--resolution <text>|--file <path>)` +- `yoi ticket doctor` + +`yoi ticket status ... closed` is intentionally rejected with guidance to use `yoi ticket close`, because status-only close would not write `resolution.md`. + +## Backend behavior + +- Uses `ticket::config::TicketConfig::load_workspace(<cwd>)` to resolve backend root. +- Missing `.yoi/ticket.config.toml` defaults to `<cwd>/work-items`. +- Relative configured roots resolve against the workspace. +- Malformed config fails through the existing config loader. +- No storage migration to `.yoi/tickets` is included. + +## Changed files + +- `Cargo.lock` +- `crates/ticket/src/lib.rs` +- `crates/yoi/Cargo.toml` +- `crates/yoi/src/main.rs` +- `crates/yoi/src/ticket_cli.rs` +- `package.nix` + +## Review status + +External sibling reviewer approved with no blockers. + +Non-blocker follow-ups: + +- `yoi ticket doctor` currently hides warning-only diagnostics because it prints `doctor: ok` when `error_count() == 0`. +- `show`, `list`, and doctor diagnostic output are not explicitly bounded. +- Body-source error text is generic and can mention `--resolution` for comment/review commands. + +## Validation + +Coder-reported validation passed: + +- `cargo test -p yoi ticket` +- `cargo test -p ticket` +- `cargo check --workspace --all-targets` +- `cargo fmt --check` +- `git diff --check` +- `git diff --cached --check` +- `./tickets.sh doctor` +- `cargo build -p yoi` +- `target/debug/yoi ticket doctor` +- `nix build .#yoi --no-link` + +Reviewer-rerun validation passed: + +- `git diff --check develop...HEAD` +- `./tickets.sh doctor` +- production CLI grep found no shell-out to `tickets.sh`. + +## Ready for merge + +Yes. + + ---