Critically review this integration. [PR #1266](https://github.com/entireio/cli/pull/1266): What are options to simplify? I fetched PR #1266 locally. High-level: it’s a big, useful integration, but it front-loads too much product/API complexity into the CLI: a 1.2k-line command file handles target resolution, API paths, pagination, hydration, dashboard aggregation, status mutation, review submission, SSE delegation, patch safety, and git apply. ## Main concerns - **Scope is too wide for one CLI integration** - Dashboard/list/show - Start/watch - Resolve/dismiss/reopen - Approve/request-changes - Apply server-provided patches safely These are multiple feature slices, not one integration. - **CLI is compensating for missing API shape** - Dashboard counts require fetching _all_ comments separately. - `show/apply/resolve` must scan paginated comments to find a comment ID, then fetch review state to hydrate suggestions. - Pagination ignores `NextOffset` and assumes `offset += limit`. - A direct comment detail/update endpoint would remove a lot of client code. - **Patch apply is the highest-risk part** - It adds git HEAD checks, custom patch path validation, atomic apply logic, and temp repo tests. - Even with current hardening, path validation is subtle across quoted paths, case-insensitive filesystems, symlinks, etc. - This is probably not needed for the first CLI version. - **Some UX combinations are awkward** - `trail review start --json --watch` prints JSON then enters human watch mode. - `--status`, `--stale`, and `--include-dismissed` create confusing combinations. - `trail watch` and `trail review watch` duplicate the same behavior. ## Best simplification options ### Option 1: MVP cut Ship only: ``` entire trail review [number] entire trail review comments [number] entire trail review show [number] entire trail review start [number] entire trail review watch [number] ``` Defer: ``` apply resolve/dismiss/reopen approve/request-changes ``` This likely removes the riskiest 30–40% of the PR, especially patch application. ### Option 2: Move orchestration server-side Add API endpoints shaped for the CLI: ``` GET /trails/{id}/reviews/dashboard GET /trails/{id}/reviews/comments/{comment_id} PATCH /trails/{id}/reviews/comments/{comment_id} ``` Then the CLI avoids: - scanning all comments for prefix matches - fetching review state just to hydrate one comment - computing counts locally - managing list pagination for dashboard totals This preserves functionality while simplifying the client. ### Option 3: Keep features, but refactor structure Split `trail_review_cmd.go` into smaller files: ``` trail_review_cmd.go cobra wiring only trail_review_api.go typed API calls/path builders trail_review_print.go rendering trail_review_apply.go patch/git apply logic trail_review_target.go shared trail resolution ``` Also add a shared JSON helper in `api.Client` to reduce repeated `Get/Post/Patch -> check -> decode` boilerplate. ### Option 4: Simplify UX flags Replace the filter matrix with presets: ``` --all --closed --stale --severity high ``` instead of exposing `--status`, `--stale`, and `--include-dismissed` independently. ## My recommendation For fastest safe merge: **ship the read/watch/start slice first**, drop `apply` and verdict submission from this PR, and add a server-side dashboard/comment-detail endpoint before reintroducing mutations. That would make this feel like a clean CLI integration instead of a full review client implemented in one command file. But would also love to be able to create a review comment via API? I always see `entire trail review [number]` but wtf. I want lists, no one knows a fucking number before not having a list to pick from? Yeah, agreed. The `[number]` UX is backwards. Right now PR #1266 says: ``` entire trail review [] ``` but the discoverability path is weak because `entire trail list` currently prints branch/author/time, not the trail number/title. So users are asked for a number they probably have no way to know. I’d change the integration around this: ## 1. Add a real review/trail list first ``` entire trail review list ``` Example: ``` NUM STATUS BRANCH TITLE OPEN HIGH UPDATED 425 in_review feat/trail-review-cli Add trail review CLI 6 2 1h ago 421 open fix/auth-refresh Fix token refresh 1 1 2d ago ``` Then: ``` entire trail review 425 ``` makes sense. Also update `entire trail list` to include at least: ``` NUM STATUS BRANCH TITLE UPDATED ``` If trail number is central to the CLI, every list must show it. ## 2. No args should list or pick `entire trail review` should do: 1. If current branch has a trail: open that review dashboard. 2. Else if TTY: show an interactive picker. 3. Else: print the review list and say “pass a number”. So the happy path is: ``` entire trail review ``` not “go find a magic number first.” ## 3. Consider accepting more than numbers Instead of only `[number]`, support a selector: ``` entire trail review 425 entire trail review trl_abc123 entire trail review feat/trail-review-cli ``` Or explicit flags: ``` entire trail review --number 425 entire trail review --id trl_abc123 entire trail review --branch feat/trail-review-cli ``` ## 4. Yes: add create-comment API This is missing from the integration. The CLI can read/review/apply, but agents need to **produce** review comments too. I’d want an API like: ``` POST /api/v1/trails/{trail_id}/reviews/{review_id}/comments ``` Body: ```json { "client_id": "agent-run-123:finding-7", "title": "Missing expiry skew handling", "body": "Token refresh should allow clock skew before treating the session as expired.", "severity": "high", "confidence": 0.94, "location": { "file_path": "src/auth/session.ts", "start_line": 88, "end_line": 91 }, "suggested_changes": [ { "change_type": "unified_diff", "patch": "diff --git a/src/auth/session.ts b/src/auth/session.ts\n..." } ] } ``` Also probably batch: ``` POST /api/v1/trails/{trail_id}/reviews/{review_id}/comments:batch ``` with idempotency via `client_id` / `Idempotency-Key`, because agents retry. ## My suggested simplification For this PR, I’d prioritize: ``` entire trail review list entire trail review entire trail review entire trail review comments entire trail review show API: create review comment / batch create comments ``` Defer risky stuff like `apply`, `approve`, `request-changes`, maybe even status mutation. First make the loop discoverable and let agents create comments. Then layer mutations/apply on top. Ok, but the user might already be on a branch within a repo that has a certain trail ID and so it would be good to just get them the comments for that or allow to post for that? Yes — that should be the default path. The CLI should treat “current branch’s trail” as the primary target, and trail number as an override. Good UX: ``` entire trail review entire trail review comments entire trail review comment add --file foo.go --line 42 --body "..." ``` All should resolve: 1. current git repo 2. current branch 3. trail for that branch 4. trail ID 5. latest/active review for that trail Only if that fails should it ask for/list trails. So I’d avoid advertising: ``` entire trail review [number] ``` and instead frame it as: ``` entire trail review [--trail ] ``` Examples: ``` entire trail review comments # current branch trail entire trail review comments --trail 425 # explicit trail entire trail review comments --trail trl_abc # explicit trail id ``` For posting comments, I’d add a trail-scoped API that does not force callers to know `review_id`: ``` POST /api/v1/trails/{trail_id}/reviews/comments ``` Body: ```json { "title": "Potential nil dereference", "body": "This can panic when user is nil.", "severity": "high", "location": { "file_path": "internal/auth/session.go", "start_line": 42 } } ``` Server can attach it to the active/latest review, or create/reuse one idempotently. CLI: ``` entire trail review comment add \ --file internal/auth/session.go \ --line 42 \ --severity high \ --title "Potential nil dereference" \ --body "This can panic when user is nil." ``` For agents, also add batch: ``` POST /api/v1/trails/{trail_id}/reviews/comments:batch ``` with `client_id` per finding for idempotency. PR #1266 already partially does current-branch resolution when number is omitted, but the command shape/docs make the number feel central. I’d change the mental model to: > “Current branch trail by default. List/pick only when ambiguous. Number/id/branch only as explicit override.” Yes — exactly that. Make the relevant changes. Implemented the UX/API changes. Changed: - `entire trail review` now uses `[]` where trail can be number, id, or branch. - Added persistent selector: - `--trail ` - Current branch remains the default target for: - `entire trail review` - `entire trail review comments` - `entire trail review comment add` - status/show/apply/watch/start/submit flows - If `entire trail review` can’t find a current-branch trail, it now falls back to showing a repo trail list. - Added: - `entire trail review list` - `entire trail review comment add` - `entire trail review comments add` alias - Added trail-scoped create-comment API client path: - `POST /api/v1/trails/{trail_id}/reviews/comments` - Added create request API types for review comments, locations, and suggested changes. - Updated `entire trail list` rows to include `NUM`, `BRANCH`, `TITLE`, etc., so numbers are discoverable. - Refactored watch slightly so review watch can stream by resolved trail ID, not just number. - Added tests for selector parsing, review list output, create comment request building, and create-comment POST path. Update the PR description and document the command usage properly. Also make e2e examples for the golden path.