Fix Trail Command Behavior and API Alignment

I inspected the full entire trail command tree and ran the relevant tests.

Commands present:

• entire trail
• entire trail list
• entire trail create
• entire trail update
• entire trail watch
• entire trail finding
• entire trail finding list|add|show|apply|resolve|dismiss|reopen|watch

Validation run:

• go test ./cmd/entire/cli -run 'Trail|trail' ✅
• go test ./cmd/entire/cli/... ✅
• Help output renders for the command tree ✅

I did not live-hit the real API, so this is code/test investigation.

Findings:

1. entire trail is hidden from top-level help

   • cmd/entire/cli/trail_cmd.go:45
   • Direct invocation works, but discoverability is intentionally/accidentally disabled.

2. trail create mutates git before auth/API succeeds

   • cmd/entire/cli/trail_cmd.go:508+
   • It may create a local branch and push it before runAuthenticatedDataAPI.
   • If the user is not logged in or the API rejects the request, the repo may already be changed.

3. trail create / trail update silently accept extra positional args

   • They do not set Args: cobra.NoArgs.
   • Example: entire trail create junk --title x would ignore junk.

4. trail create --status is not locally validated

   • trail update validates status; trail create sends invalid statuses to the server.

5. Trail lookup only searches the first 200 trails

   • cmd/entire/cli/trail_cmd.go:895
   • Affects: default trail, update, watch, finding, selectors by branch/number/id.
   • Code comment already acknowledges trails beyond 200 are invisible.

6. trail update --body "" cannot clear a body

   • buildTrailUpdateRequest only sends body/title if non-empty.
   • Interactive update also cannot clear body/title cleanly because empty means “do not send”.

7. Finding filters are weakly validated

   • trail finding/list --status, --severity, --stale are mostly passed through to the API.
   • Invalid values are not caught locally.
   • --include-dismissed is potentially misleading because default --status open may still exclude dismissed unless combined with --status any.

8. SSE watch treats clean EOF as io.ErrUnexpectedEOF

   • cmd/entire/cli/trail_watch_cmd.go:378
   • Probably okay if the server always sends event: reconnect, but --once may fail on a clean close without a reconnect frame.

Overall: the command tree is wired and tests pass, but there are real functional/UX bugs around create side effects, lookup pagination, update clearing fields, arg/status validation, and watch EOF handling.

Implemented fixes for items 2–8.

Changed:

• trail create

  • Now validates status locally.
  • Defaults empty status to draft.
  • Rejects unexpected positional args.
  • Creates the remote/API trail before local branch creation/push, so auth/API failures don’t leave local mutations.

• trail update

  • Rejects unexpected positional args.
  • Can now send explicit empty --body "" / --title "" values instead of treating them as “unchanged”.

• Trail lookup

  • findTrail now paginates through all trail-list pages instead of only searching the first 200.

• trail finding filters

  • Added local validation for status/severity/stale/limit/offset.
  • Normalizes comma-separated filters.
  • --include-dismissed now broadens the default open-only filter unless --status was explicitly set.

• SSE watch

  • Clean EOF now returns a clean streamCloseDone instead of io.ErrUnexpectedEOF.

Added/updated tests covering pagination, update clearing, arg rejection, filter validation, include-dismissed behavior, and clean SSE EOF.

Validation:

• go test ./cmd/entire/cli -run 'Trail|trail' ✅
• go test ./cmd/entire/cli/... ✅

Done.

Additional fixes:

• Title is now enforced as required:

  • trail create errors if the resolved title is empty.
  • trail update --title "" is rejected.
  • Body can still be cleared with --body "".

• Added API/model support for trail phase:

  • api.TrailResource
  • api.TrailCreateRequest
  • api.TrailUpdateRequest
  • trail.Metadata

• Surfaced phase in CLI:

  • entire trail detail view prints Phase.
  • entire trail list shows a PHASE column when any listed trail has phase data.
  • entire trail create --phase <value>
  • entire trail update --phase <value>
  • Interactive update includes phase input.

• Added tests for phase propagation/display and required-title validation.

Validation:

• go test ./cmd/entire/cli/... ✅

I’d recommend making the data-fetching behavior explicit.

Best shape:

• entire trail → prints help/overview only
• entire trail show [<trail>] → shows one trail
  • default: current branch’s trail
  • selector: number, id, or branch
• entire trail list → lists trails
• No implicit “show current, else list” fallback at the root

Why:

• It matches normal CLI command patterns: noun root = namespace/help, verb subcommands do work.
• It avoids surprising API calls from just typing entire trail.
• It avoids ambiguous behavior where the same command sometimes shows details and sometimes lists.
• It gives agents/users a deterministic command: entire trail show.

Migration path I’d use:

1. Add entire trail show.
2. Change injected agent hint to:
   Use entire trail show/list/create/update/watch...
3. For one release, keep entire trail as a compatibility alias for show, but print a short stderr notice: entire trail now maps to 'entire trail show'; use that explicitly.
4. Later, make root print help only.

I would not keep the “if no current trail, list recent trails” behavior for show; I’d make it say:

No trail found for branch "x". Run 'entire trail create' or 'entire trail list --status any'.

That’s clearer and easier to script around.

Agreed. I implemented that shape:

• entire trail now prints help/overview only; no API/auth call.
• Added entire trail show [<trail>].
  • No selector: current branch’s trail.
  • Selector: number, id, or branch.
• Removed implicit root behavior of “show current trail, else list”.
• Did not change agent hint.
• No migration alias/warning.

Validation:

• go test ./cmd/entire/cli -run 'Trail|trail' ✅
• go test ./cmd/entire/cli/... ✅

Done.

• Branch: fix-trail-command-behavior
• Commit: a5e8ef66c — Fix trail command behavior
• Pushed to: origin/fix-trail-command-behavior
• Trail created:
  • Number: 579
  • ID: 019ed585-7b97-745b-9b55-aaf146031970
  • Status: open
  • Phase: building

PR URL: https://github.com/entireio/cli/pull/new/fix-trail-command-behavior

Done — I documented the changes on the trail.

Note: entire trail update --body currently does not persist to the API’s body_document; the trail body remained empty after update attempts. I added the full documentation as a trail discussion comment instead so it’s recorded on the trail.

Investigated. The body does not persist because the server no longer treats body as a PATCH-able trail field.

Evidence:

• PATCH /api/v1/trails/gh/entireio/cli/579 with only {"body": "..."} returns:
  • 400 {"error":"At least one field is required"}
• PATCH with {"title": "...", "body": "..."} succeeds, but only title/metadata changes; body remains empty.
• GET /api/v1/trails/gh/entireio/cli/579 returns body as:
  • trail.body_document.text_snapshot
  • not trail.body
• The web app updates the trail body through a collaborative document websocket:
  • /api/v1/trails/gh/{org}/{repo}/{number}/body/ws

So the CLI’s current TrailUpdateRequest.Body is stale API shape. It serializes body, but the server ignores/rejects it because trail body is now body_document.

Recommendation:

1. Do not claim entire trail update --body works until we implement the document API/websocket path.
2. Either:
   • remove/disable --body with a clear error, or
   • implement proper body-document updates against the server-supported mechanism.
3. Also parse body_document.text_snapshot in trail detail responses so CLI display can show existing web-authored body text.

For now, I documented the changes in a trail discussion comment because that endpoint does persist.

No.

Our commit only touched trail-related files:

• cmd/entire/cli/api/trail_types.go
• cmd/entire/cli/api/trail_types_test.go
• cmd/entire/cli/trail/trail.go
• cmd/entire/cli/trail_cmd.go
• cmd/entire/cli/trail_cmd_test.go
• cmd/entire/cli/trail_review_cmd.go
• cmd/entire/cli/trail_review_cmd_test.go
• cmd/entire/cli/trail_watch_cmd.go
• cmd/entire/cli/trail_watch_cmd_test.go

No line attribution/blame files were touched. Also working tree is clean.

Reviewed again. No changes made.

Main concerns before merge:

1. trail create new-branch flow may now be broken

   • cmd/entire/cli/trail_cmd.go:625-680
   • We now call the API before creating/pushing the local branch.
   • Original code pushed first because “API can reference it”. If the API requires the branch to exist remotely, entire trail create --branch new-branch will fail.

2. SSE watch may stop instead of reconnecting

   • cmd/entire/cli/trail_watch_cmd.go:378
   • Clean EOF now returns streamCloseDone, which makes normal watch exit. For --once that’s good; for continuous watch, a proxy/server clean-close without event: reconnect should probably reconnect.

3. --include-dismissed may include too much

   • cmd/entire/cli/trail_review_cmd.go:539-544
   • It changes default status=open to status=any, so resolved findings are included too. If the intent is “open + dismissed”, this is too broad.

4. Trail body support is still stale

   • cmd/entire/cli/trail_cmd.go:908-910
   • --body still serializes body, but API currently uses body_document. So body update can fail or silently not persist depending on other fields.
   • Tests around body clearing are only unit-level and don’t reflect actual API behavior.

5. Phase may be server-derived, not user-editable

   • cmd/entire/cli/trail_cmd.go:560, cmd/entire/cli/trail_cmd.go:810-812
   • Since phase represents whether the trail has code, exposing --phase on create/update may be wrong if the server computes it from branch/code state. Display support is good; mutation support is questionable.

Everything else looks consistent with the intended root/help + explicit show command shape.

Reviewed and live-tested against the current API. No code changes made.

Findings:

1. Bare entire trail behavior is good

   • Prints help only.
   • No auth/API call.

2. entire trail show works, but does not display body

   • Body now persists in API as body_document.text_snapshot.
   • CLI show resolves via list endpoint, which returns body: "", not body_document.
   • Result: persisted body is invisible in CLI.

3. trail update --body is still partially broken

   • --body alone fails:
     • API error: At least one field is required
   • --title ... --body ... succeeds and body persists.
   • So API accepts/processes body only when another recognized PATCH field is present.

4. trail create for a new branch is broken

   • Current code creates the API trail before creating/pushing the branch.
   • Live test with a non-existing branch failed with API 500.
   • Creating/pushing the branch first, then running entire trail create, works.

5. --phase mutation does not work

   • trail update --phase building fails if it’s the only field.
   • trail update --title ... --phase building succeeds but phase remains unchanged.
   • trail create --phase building also produced a trail with phase: null.
   • So phase appears read-only/server-derived from the API side. Display support is fine; create/update flags are misleading.

6. Finding filter validation works

   • Invalid --status, --severity, --stale fail locally as expected.
   • --include-dismissed command runs.

7. Watch connects

   • entire trail watch 579 --once --show-pings connected and streamed events.
   • I didn’t wait for server close/reconnect behavior.

8. Tests still pass

   • go test ./cmd/entire/cli/... ✅

Main things I’d fix before merge:

• Restore local branch create/push before API trail create, or server must support non-existing branches.
• Remove/disable --phase mutation unless API supports it.
• Fix body update semantics/API, and make show fetch/display body_document.text_snapshot.

Fixed the confirmed findings and pushed.

What changed:

• Restored trail create branch workflow:

  • Auth/remote resolution happens first.
  • Then local branch creation + push happens before API trail creation.
  • Live-tested with a new branch; it now succeeds.

• Removed misleading phase mutation support:

  • Removed trail create --phase.
  • Removed trail update --phase.
  • Removed phase from create/update request bodies.
  • Kept phase parsing/display from API responses.

• Left body behavior alone, per your note that you’re investigating API/platform side.

Also: I noticed the remote branch had a merge from main that my first force-push would have overwritten. I restored that history and pushed the corrected branch with the merge preserved.

Validation:

• Live entire trail create new branch test ✅
• go test ./cmd/entire/cli/... ✅
• Branch pushed: fix-trail-command-behavior
• Current tip: ad9b4cd7d

Validated successfully.

Checks run:

• go test ./cmd/entire/cli/... ✅
• entire trail root prints help only, no data/auth output ✅
• entire trail create/update --help no longer expose --phase ✅
• entire trail show fix-trail-command-behavior works and displays phase from API ✅
• entire trail list --json includes the current trail and phase ✅
• Live entire trail create with a new branch:
  • created local branch ✅
  • pushed remote branch ✅
  • created trail ✅
  • body persisted into API body_document.text_snapshot on create ✅
  • cleaned up validation trail and branch afterward ✅
• entire trail update --title "" rejects empty title ✅
• entire trail update --status nope rejects invalid status ✅
• entire trail finding list --status nope rejects invalid finding filter ✅
• entire trail finding list --include-dismissed works ✅
• entire trail watch 579 --once --show-pings connects and streams events ✅

Final repo state:

• Working tree clean ✅
• Branch tracks origin/fix-trail-command-behavior ✅

Fixed the review finding and pushed.

What changed:

• Removed branch creation/push from the API callback.
• trail create now:
  1. Authenticates / constructs API client.
  2. Resolves trail remote.
  3. Creates local branch if needed.
  4. Pushes branch if needed.
  5. Creates the trail via API.
• If push fails after creating a local branch, it cleans up the local branch.
• If the trail API create fails after pushing a newly-created branch, it best-effort deletes the pushed remote branch and local branch to avoid orphan branches.

Validation:

• Live create test with new branch ✅
• Cleanup of test branch/trail ✅
• go test ./cmd/entire/cli/... ✅
• Working tree clean ✅
• Pushed to PR branch: eb8b098ab

Fixed that review comment.

Change:

• Added cleanupCreatedTrailBranch(...) before returning the api.DecodeJSON error in trail create.

Validation:

• go test ./cmd/entire/cli -run 'Trail|trail' ✅
• go test ./cmd/entire/cli/... ✅
• Pushed updated branch: df1921f81

Remote checks have started and are currently queued/in progress.

That was just me polling GitHub Actions until the PR checks finished.

The script:

• calls gh pr view 1447 --json statusCheckRollup
• prints pending/failed counts
• sleeps 15 seconds between polls
• stops when all checks are complete or if one fails

The timeout=1800 is the tool-level max runtime: 1800 seconds / 30 minutes, so the polling command doesn’t run forever if GitHub Actions hangs. It doesn’t affect the repo or PR; it only limits my local wait loop.

Understood — I’ll stop force-pushing.

Current state:

• Working tree clean.
• Branch is synced with origin/fix-trail-command-behavior.
• All GitHub PR review threads are resolved.
• Latest PR head: 1a6c8a505
• Remote checks:
  • lint ✅
  • license ✅
  • binary-size ✅
  • tests still in progress

I won’t push again unless there’s a new actual code change required.

Yes — entire trail finding has been addressed.

Verified:

• Command surface uses finding with subcommands: list, show, add, apply, resolve, dismiss, reopen, watch.
• Removed/old review naming is covered by tests.
• Filter validation is in place:
  • invalid status rejects with clear error.
  • severity/stale/limit/offset normalization is tested.
• --include-dismissed behavior is implemented: when no explicit status is provided, it changes default open filtering to any.
• Relevant tests pass:
  • go test ./cmd/entire/cli -run 'TrailCommandSurfaceUsesFindings|NormalizeTrailReviewListOptions|Invalid|TrailFinding|trail finding'

Also checked PR status:

• All GitHub checks are green on current head 1a6c8a505.
• Working tree clean.
• No force-push performed after your instruction.

Resolved the open entire trail finding.

Verified:

• entire trail finding list 579 --status open → No findings found.
• Working tree clean.

Merged latest origin/main into fix-trail-command-behavior and pushed normally.

Validation:

• go test ./cmd/entire/cli/... ✅
• No force-push used.
• New branch tip: 60792ef1c

Addressed and pushed normally (no force push).

Changes:

• Moved findTrailBySelector into trail_cmd.go near the other trail lookup helpers.
• Updated findTrail pagination:
  • no longer stops after only 2 pages when Total == 0
  • still detects repeated pages
  • bounded to 10 pages to avoid runaway loops
• Improved cleanup warnings with explicit manual recovery commands.

Validation:

• go test ./cmd/entire/cli/... ✅
• GitHub review threads: none unresolved ✅
• Trail open findings: none open ✅
• Remote checks are currently running on 271673602; binary-size already passed.

Yes — it’s a reasonable and technically accurate comment. I traced the code paths:

The core claim holds.

• branchNeedsCreation only checks refs/heads/<branch> locally (repo.Reference(NewBranchReferenceName, true)). A branch that exists only on the remote (never fetched, no remote-tracking ref) → needsCreation == true.
• We then createBranch at local HEAD and pushBranchToOrigin with git push -u origin <branch>.
• On success remoteBranchPushed = true, and a later API-create failure runs git push --delete origin <branch>.

So the "is this branch ours to delete?" decision is indeed gated on a local-only signal, and the deletion is irreversible remote state. That part is correct.

The danger is real but narrower than "push is a no-op/fast-forward" implies:

• Identical commit (remote == local HEAD): push is a no-op, succeeds → remoteBranchPushed=true → cleanup deletes a branch we didn't create. ✅ data loss.
• Remote behind local HEAD (fast-forward): push succeeds and advances the remote, then cleanup deletes it entirely. ✅ data loss (and we also moved it first).
• Remote diverged/ahead: git push is rejected (non-fast-forward) → pushBranchToOrigin errors → cleanup called with remotePushed=false → no remote deletion. ✅ safe.

So the failure mode requires: remote-only branch + push that fast-forwards or is a no-op + API-create failure. The comment’s own "realistically narrow" caveat is fair.

Severity: Medium is defensible because the outcome is irreversible remote deletion, though given the conjunction of conditions, low would also be reasonable.

Suggested fix is sound. The cleanest guard is to check remote existence before pushing (e.g. git ls-remote --heads origin <branch>); if it already exists remotely, it isn’t ours, so don’t set remoteBranchPushed for cleanup purposes. Note we previously had a branchExistsOnOrigin helper but removed it for an unrelated lint reason — a pre-push existence check would address this more directly than the post-failure check we removed.

One thing I’d push back on slightly: it overlaps conceptually with the earlier resolved finding about deleting a branch "from a previous attempt," but it’s a distinct trigger (teammate’s remote branch + local-only needsCreation), so it’s worth tracking separately rather than dismissing as a duplicate.

I haven’t changed any code, since you only asked for an assessment. Want me to implement the pre-push existence guard?

Fixed.

The comment was valid, so I added a pre-push guard:

• Before pushing in trail create, branchExistsOnOrigin(branch) checks git ls-remote --heads origin <branch>.
• remoteBranchPushed is now set to !existedOnOrigin, so cleanup only deletes the remote branch when our push is what introduced it.
• If the existence check itself errors (e.g. network), we conservatively treat the branch as pre-existing and never delete it, emitting a warning.

This closes the data-loss path: a teammate’s remote-only branch can no longer be deleted by cleanup just because needsCreation was derived from local-only state.

Validation:

• go build ./... ✅
• go test ./cmd/entire/cli/... ✅
• Committed and pushed normally (no force push): 57a22a335