## Double check this claim.  
I thought empty body errors?

**(question)** Empty-string clears. `--body ""` and `--severity ""` produce `req.Body/req.Severity = &""`, which pass `trailReviewCommentPatchHasFields` and get sent as explicit clears. If the server treats empty body/severity as valid, a finding can be blanked out. Confirm that's intended server-side, or reject empty  
`--body` the way add does (buildTrailReviewCommentInput requires a non-empty body).

---

4w ago·3.8s·9 calls

Confirmed: the claim is correct for `update`.

In `cmd/entire/cli/trail_review_cmd.go`:

- `--body ""` sets `opts.BodyChanged = true`, then:

```go
    req.Body = stringPtr(strings.TrimSpace(opts.Body))
    ```

So it becomes `*""`.

- `--severity ""` sets `opts.SeverityChanged = true`, then:

```go
    severity := strings.ToLower(strings.TrimSpace(opts.Severity))
    req.Severity = stringPtr(severity)
    ```

So it also becomes `*""`.

- `trailReviewCommentPatchHasFields` only checks pointer presence:

```go
    return req.Body != nil || req.Severity != nil || ...
    ```

So empty strings pass.

- The JSON tags are `omitempty`, but because these are non-nil pointers, they will still marshal as:

```json
    {"body": ""}
    ```

or

```json
    {"severity": ""}
    ```

By contrast, `add` does reject empty body:

```go
    body := strings.TrimSpace(opts.Body)
    if body == "" {
        return ..., errors.New("finding body is required (pass --body)")
    }
    ```

So: empty body errors on `add`, but not on `update`.

If empty-body clearing is not intended, `buildTrailReviewCommentPatchRequest` should reject `BodyChanged && strings.TrimSpace(opts.Body) == ""`. For severity, current behavior intentionally permits empty as a “clear” unless we decide to reject it too.

---

Update should also reject.

4w ago·3.0s·4 calls

Implemented update rejection for empty values.

Changes:

- `cmd/entire/cli/trail_review_cmd.go`
    - `--body ""` / whitespace now errors: `finding body is required (pass --body)`
    - `--severity ""` / whitespace now errors as invalid severity
- `cmd/entire/cli/trail_review_cmd_test.go`
    - Added coverage for empty body and empty severity on update

Validation:

- `go test ./cmd/entire/cli` passes.

---

**Entire Trail review comment**  
**ID:** 019ee023-54d4-7f48-bcad-d359e9624b93  
**URL:** [Link](/content/gh/entireio/cli/trails/619/clean-up-trail-finding-commands/index.html)  
**Severity:** high  
**Context:** `cmd/entire/cli/trail_review_cmd.go:1271`  
**Selected context:**

```go
fmt.Fprintln(w, "  entire trail watch")  
```

The help text on line 1271 suggests running `entire trail watch`, but the `watch` command was removed from the finding subcommands in this PR and no replacement at the trail level is visible in this diff. This creates a broken user-facing reference.

---

Why was it removed from "entire trail" help? Should still be there, just not on trail finding...

4w ago·4.7s·4 calls

It wasn’t removed from `entire trail`.

Confirmed:
- `newTrailCmd()` still has `cmd.AddCommand(newTrailWatchCmd())`
- `entire trail watch --help` works
- The PR only removed `watch` from `entire trail finding`

I added a regression assertion that `entire trail watch` remains registered while `entire trail finding watch` stays removed.

---

**Entire Trail review comment**  
**ID:** 019ee04b-e048-7154-b198-75b06fa9f3d4  
**URL:** [Link](/content/gh/entireio/cli/trails/619/clean-up-trail-finding-commands/index.html)  
**Severity:** medium  
**Context:** `cmd/entire/cli/trail_review_cmd.go`

`buildTrailReviewCommentPatchRequest (~line 703)` treats an explicitly-empty flag as a clear: `--body ""` or `--severity ""` (flag Changed) sends an empty string to the server. Severity validation only runs on non-empty values, so `""` slips through. Since `add` requires a non-empty body, blanking a body via `update` is inconsistent. Confirm the server treats `""` as clear vs. persisting an empty finding; if clearing isn't intended, skip empty strings like unchanged fields are skipped.

4w ago·5.1s·1 call

This is already fixed on the branch.

Current behavior in `buildTrailReviewCommentPatchRequest`:

- `--body ""` / whitespace returns: `finding body is required (pass --body)`  
- `--severity ""` / whitespace returns invalid severity  
- Empty strings are no longer sent to the server as clears

Added tests cover both cases. Commit: `99ba8655f Reject empty finding update fields`

---

**Entire Trail review comment**  
**ID:** 019ee04b-eed2-75f3-a041-f87ba717e27e  
**URL:** [Link](/content/gh/entireio/cli/trails/619/clean-up-trail-finding-commands/index.html)  
**Severity:** low  
**Context:** `cmd/entire/cli/trail_review_cmd.go`

Stylistic: in `newTrailReviewApplyCmd`, `cmd.Long (~line 258)` is assigned between two `cmd.Flags()` calls. Harmless, but tidier to set Long next to Use/Short.

4w ago·2.0s·3 calls

Done. Moved `Long` into the `cobra.Command` literal next to `Use`/`Short`, and kept flags together.

Validation:

- `gofmt`  
- `go test ./cmd/entire/cli -run TestTrailCommandSurfaceUsesFindings -count=1` passes.

---

**Entire Trail review comment**  
**ID:** 019ee04b-fe42-7d75-a92e-781902040bbb  
**URL:** [Link](/content/gh/entireio/cli/trails/619/clean-up-trail-finding-commands/index.html)  
**Severity:** low  
**Context:** `cmd/entire/cli/trail_review_cmd.go`

`printTrailReviewCommentsTable (~line 1285)` hard-codes a two-space indent into the tabwriter header line (" ID\t...") and each row. Works, but indenting via tabwriter padding/prefix would be cleaner if this pattern spreads.

4w ago·2.0s·4 calls

Done. Updated `printTrailReviewCommentsTable` to render the tabwriter output into a buffer without embedded indentation, then apply indentation via a helper.

Validation:

- `gofmt`  
- Targeted test passes.

---

Resolve all "entire trail finding" once done some might have been addressed already and some are nits.