Review and Simplify CLI Integration

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:

Defer:

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:

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:

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:

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.

Yeah, agreed. The [number] UX is backwards.

Right now PR #1266 says:

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

Example:

Then:

makes sense.

Also update entire trail list to include at least:

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:

not “go find a magic number first.”

3. Consider accepting more than numbers

Instead of only [number], support a selector:

Or explicit flags:

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:

Body:

Also probably batch:

with idempotency via client_id / Idempotency-Key, because agents retry.

My suggested simplification

For this PR, I’d prioritize:

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.

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:

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:

and instead frame it as:

Examples:

For posting comments, I’d add a trail-scoped API that does not force callers to know review_id:

Body:

Server can attach it to the active/latest review, or create/reuse one idempotently.

CLI:

For agents, also add 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.”

Implemented the UX/API changes.

Changed:

• entire trail review now uses [<trail>] where trail can be number, id, or branch.
• Added persistent selector:
  • --trail <number|id|branch>
• 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.

Validation:

• env -u PI_CODING_AGENT mise exec -- go test ./... passed
• mise exec -- golangci-lint run passed
• git diff --check passed

Note: git-remote-entire is still untracked in the worktree; I left it untouched.

Done.

Updated:

• PR #1266 description with new targeting model, golden path, command docs, API shape, and test status.
• Added docs/trail-review.md with full command usage.
• Added manual e2e examples:
  • e2e/examples/README.md
  • e2e/examples/trail-review-golden-path.sh
• Updated e2e/README.md to document manual examples.

Validation:

• env -u PI_CODING_AGENT mise exec -- go test ./cmd/entire/cli ./cmd/entire/cli/api
• env -u PI_CODING_AGENT mise exec -- go test ./...
• mise exec -- golangci-lint run
• git diff --check

All passed. E2E example script is manual/service-backed, so I did not run it.

Done.

Changes made:

• Removed redundant entire trail review list.
• Discovery now uses existing:
  • entire trail list --status any
• entire trail review fallback now prints regular trail list when no current-branch trail is found.
• Documented and tested #425 selector support.
  • Noted shell caveat: use '#425', --trail '#425', or --trail=#425.
• Updated PR description again.
• Rewrote docs/trail-review.md with output examples for each command:
  • entire trail list --status any
  • entire trail review
  • fallback output
  • start
  • comments
  • comment add
  • show
  • apply
  • resolve/dismiss/reopen
  • approve/request-changes
  • watch
• Updated golden path e2e example to use entire trail list --status any.

Validation:

• env -u PI_CODING_AGENT mise exec -- go test ./cmd/entire/cli ./cmd/entire/cli/api
• env -u PI_CODING_AGENT mise exec -- go test ./...
• mise exec -- golangci-lint run
• git diff --check

All passed.

Agreed. I cleaned that up.

Changes:

• Removed # selector support. Selectors are now only:
  • number: 425
  • trail id: trl_abc123
  • branch: feat/foo
• Removed the standalone docs/trail-review.md; command docs/output now live concisely in the PR description.
• Removed approve / request-changes commands and API types. You’re right — those were trail verdict actions, not part of the agent-native code review API.
• Updated PR description to be shorter and focused.
• Kept entire trail list --status any as the discovery path instead of adding redundant review-specific list.
• Updated tests accordingly.

Validation passed:

• env -u PI_CODING_AGENT mise exec -- go test ./cmd/entire/cli ./cmd/entire/cli/api
• env -u PI_CODING_AGENT mise exec -- go test ./...
• mise exec -- golangci-lint run
• git diff --check

Addressed.

Changes:

• Removed #425 support entirely.
• Removed approve / request-changes code and API types.
• Removed standalone docs/trail-review.md; kept docs concise in PR description.
• Updated PR description to clarify:
  • entire trail list --status any already exists; PR only improves list output with NUM/TITLE.
  • Code suggestions are created via comment add --patch-file suggestion.diff or --patch-file -.
  • review start is optional/advanced: only for explicitly requesting/rerunning automated review, not needed for list/post/respond.
• Added --patch-file support for code suggestions.
• Removed review start from golden-path e2e example.
• Updated manual e2e docs accordingly.

Validation passed:

• env -u PI_CODING_AGENT mise exec -- go test ./cmd/entire/cli ./cmd/entire/cli/api
• env -u PI_CODING_AGENT mise exec -- go test ./...
• mise exec -- golangci-lint run
• git diff --check

Still untouched: untracked git-remote-entire.

You’re right — if we can’t explain review start clearly, it shouldn’t be in this UX.

I removed it.

Now the flow is just:

Also updated:

• Removed review start code, API types, tests, and PR docs.
• Removed PostWithHeaders that only existed for start.
• Clarified PR description:
  • entire trail list --status any already exists.
  • Code suggestions are via comment add --patch-file suggestion.diff or --patch-file -.
• Golden-path e2e example no longer includes review start.

Validation passed:

• go test ./cmd/entire/cli ./cmd/entire/cli/api
• go test ./...
• golangci-lint run
• git diff --check

Usually no. Better practice is one consistent resource namespace.

Current mixed shape is a bit awkward:

Better canonical shape:

Then aliases can exist for ergonomics:

This matches common CLI patterns better: resource + verb, e.g. gh issue list/view/create, kubectl get/describe, etc.

One caveat: “comment” may be API language while the UI says “finding.” If the user-facing concept is finding, then even better:

But if API/product calls them review comments, use comment. My recommendation: make comment list/show/add/... canonical and keep shorter old forms as aliases if desired.

Yes — agreed. I changed it to:

Also removed:

• entire trail review
• entire trail review start
• approve / request-changes
• #425 support/docs

Updated:

• PR description
• golden-path e2e example renamed to e2e/examples/trail-finding-golden-path.sh
• e2e docs
• output text to say “finding” instead of “comment” where user-facing

Validation passed:

• go test ./cmd/entire/cli ./cmd/entire/cli/api
• go test ./...
• golangci-lint run
• git diff --check

Review complete. Overall the current working tree is in good shape functionally.

Fixed during review:

• Replaced remaining user-facing “comment” output with “finding”, including status/apply output and watch event rendering.
• Updated trail watch wording from code-review/comment language to finding language.
• Fixed removed entire trail review silently falling through to entire trail by adding cobra.NoArgs on trail.
• Added tests verifying finding command surface and removed review/start/comments/approve/request-changes.
• Switched --patch-file - to read from cmd.InOrStdin().
• Updated PR #1266 description examples via gh pr edit.

Validation passed:

• env -u PI_CODING_AGENT mise exec -- go test ./cmd/entire/cli ./cmd/entire/cli/api
• env -u PI_CODING_AGENT mise exec -- go test ./...
• mise exec -- golangci-lint run
• git diff --check

Remaining cautions:

• Code changes are still uncommitted/unpushed, so PR code does not yet reflect them.
• e2e/examples/ is untracked and should be added if committing.
• git-remote-entire is still untracked and appears unrelated; leave it out.
• Two unrelated lint-cleanup edits remain in checkpoint/strategy files because lint currently flags those stale nolint directives.

Committed and pushed.

• Commit: 43067b86d feat: simplify trail finding CLI
• Pushed to: origin/feat/trail-review-cli
• PR head now points to 43067b86d
• Removed untracked git-remote-entire
• Working tree is clean

Note: commit/push hooks printed a missing ./scripts/entire-dev warning, but both operations completed successfully.

Done.

• Fetched latest origin/main
• Merged into current branch pr-1266
• Resolved conflicts in:
  • cmd/entire/cli/checkpoint/committed_reader_resolve.go
  • cmd/entire/cli/strategy/manual_commit.go
• Committed merge: caa7203a5
• Pushed to PR branch: origin/feat/trail-review-cli
• Removed the untracked root git-remote-entire file
• Working tree is clean

Validation passed:

• env -u PI_CODING_AGENT mise exec -- go test ./cmd/entire/cli ./cmd/entire/cli/api
• env -u PI_CODING_AGENT mise exec -- go test ./...
• mise exec -- golangci-lint run
• git diff --check

PR is mergeable; checks are currently running.

Checked out/fetched latest branch state.

Current HEAD:

• edaadf399 refactor: address trail finding CLI review feedback
• Matches origin/feat/trail-review-cli
• Working tree is clean
• PR is mergeable

Validation passed locally:

• env -u PI_CODING_AGENT mise exec -- go test ./cmd/entire/cli ./cmd/entire/cli/api
• env -u PI_CODING_AGENT mise exec -- go test ./...
• mise exec -- golangci-lint run
• git diff --check origin/main...HEAD
• No conflict markers found

Latest changes look valid. CI on GitHub still has some test jobs in progress, but completed jobs are passing so far.

It means the review was valid: entire trail finding ... was resolving the git remote with the lower-level helper, which can return an empty forge for unsupported remotes like GitLab. That would build a bad API path like:

Instead of failing locally with a clear unsupported-forge error.

I fixed it by changing resolveTrailReviewTarget to call resolveTrailRemote(ctx), the guarded helper already used by trail/watch paths, and added a regression test for unsupported forge handling.

Pushed fix:

Validation passed locally:

• go test ./cmd/entire/cli ./cmd/entire/cli/api
• go test ./...
• golangci-lint run
• git diff --check

Working tree is clean; PR checks are queued.