Skip to content

trail finding: populate selected_text for line/range findings#1444

Open
Soph wants to merge 1 commit into
mainfrom
soph/trail-finding-selected-text
Open

trail finding: populate selected_text for line/range findings#1444
Soph wants to merge 1 commit into
mainfrom
soph/trail-finding-selected-text

Conversation

@Soph

@Soph Soph commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

https://entire.io/gh/entireio/cli/trails/580/trail-finding-populate-selected-text-for-line-range-findings

Problem

entire trail finding add --line / --start-line always failed against the control plane with:

location.granularity=line requires non-empty selected_text

The CLI built a line/range location but never set selected_text, and there was no flag to supply it (TrailReviewLocationCreateRequest.SelectedText existed but nothing ever assigned it). The server requires it for line/range granularity (validateLocationShape in the BFF). Only --file (which maps to file granularity, no selected_text requirement) worked — so line-anchored findings were dead on arrival.

Fix

selected_text is the diff anchor, so it must be the actual source text at those lines. Two ways to provide it:

  1. --selected-text flag — explicit, always wins.

  2. Auto-read from the local working tree when the flag is omitted, behind a safety gate so the anchor text provably matches the trail's reviewed code:

    • finding's branch == checked-out branch,
    • target file has no uncommitted changes,
    • trail head SHA == local HEAD.

    The user-supplied --file is validated (no absolute paths, no .. escape, no .git) via the existing validatePatchPath before any filesystem access. Any gate failure (or out-of-range lines) is a hard error pointing at --selected-text. Lines are joined with \n and capped at 16384 chars, matching the server's own selectedTextFromPatch derivation.

Gate ordering / empty-review caveat

The branch and clean-file checks run before the review is started, so those misconfigurations fail with no server side effects. The trail head SHA is only returned by startTrailReview, so createTrailReviewFinding finalizes the input via a callback that runs after the review is opened — and that's where the HEAD-match check happens. A HEAD mismatch therefore leaves an empty review session behind. This is a rare edge (you'd be on the right branch with a clean tree but at a different commit than the trail tip); pre-checking it would need a way to fetch the trail head without opening a review, which the API doesn't currently expose. Flagged rather than re-architected.

Refactors (no behavior change)

  • compareLocalHead — shared by the new head check and the existing verifyTrailReviewHead.
  • gitStatusDirty(ctx, root, pathspecs...) — now backs both HasUncommittedChanges (empty root → unchanged CWD behavior) and the new file-scoped clean check.

Testing

  • New unit tests: explicit selected-text, file-granularity ignoring it, locationNeedsSelectedText, and the gate (readSelectedTextFromWorktree happy path + branch-mismatch + path-escape + out-of-range + dirty-file; verifyTrailHeadMatchesLocal nil/match/mismatch).
  • mise run fmt && mise run lint clean; full cmd/entire/cli suite green.
  • Verified live: posted a range finding (lines 260–262 of git_operations.go) to this branch's trail via the auto-read path; the server accepted it and stored the correct selected_text — the exact case that previously failed.

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings June 17, 2026 13:11
@Soph Soph requested a review from a team as a code owner June 17, 2026 13:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes entire trail finding add --line/--start-line being rejected by the control plane by ensuring selected_text is populated for line/range locations (via --selected-text or by auto-reading the anchored lines from the local worktree under safety gates). Also refactors git dirty/HEAD comparison utilities and updates unit tests to cover the new behavior.

Changes:

  • Add --selected-text and auto-read selected_text for line/range finding locations (with branch/dirty-file/HEAD safety gating).
  • Refactor finding creation to finalize input after starting a review (to access the review head SHA) and reuse a shared local-HEAD comparator.
  • Introduce gitStatusDirty(ctx, root, pathspecs...) and update tests for the new selected-text and review-finalization flow.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
cmd/entire/cli/trail_review_cmd.go Adds selected-text support + auto-read gate; refactors review/finding submission flow and HEAD comparison.
cmd/entire/cli/trail_review_cmd_test.go Adds unit tests for selected-text behavior, auto-read gates, and updated create-finding flow.
cmd/entire/cli/git_operations.go Adds reusable gitStatusDirty helper and updates HasUncommittedChanges to use it.

Comment thread cmd/entire/cli/trail_review_cmd.go
Comment thread cmd/entire/cli/trail_review_cmd.go
Comment thread cmd/entire/cli/trail_review_cmd.go
@Soph Soph force-pushed the soph/trail-finding-selected-text branch from a4dd3a6 to 7520d4a Compare June 17, 2026 14:20
`entire trail finding add --line/--start-line` always failed against the
control plane with `location.granularity=line requires non-empty
selected_text`: the CLI built a line/range location but never set
`selected_text`, and there was no flag to supply it. Only `--file`
(file-granularity, no selected_text requirement) worked.

selected_text is the diff anchor, so it must be the source text at those
lines. Two ways to provide it now:

- `--selected-text` flag — explicit, always wins.
- Auto-read from the local working tree when the flag is omitted, behind a
  safety gate so the anchor text provably matches the trail's reviewed code:
  the finding's branch must equal the checked-out branch, the target file
  must have no uncommitted changes, and the trail head SHA must equal local
  HEAD. The user-supplied --file is validated (no absolute paths, no `..`
  escape, no .git) and the file is read through os.Root, which confines the
  open to the repo root at the kernel level and rejects symlinks that escape
  it. Any gate failure (or out-of-range lines) is a hard error pointing at
  `--selected-text`. Lines are joined with "\n" and capped at 16384 chars,
  matching the server's own selectedTextFromPatch derivation.

The branch and clean-file checks run before the review is started, so those
misconfigurations fail without side effects. The trail head SHA is only
returned by starting the review, so `createTrailReviewFinding` finalizes the
input via a callback that runs after the review is opened, where the
HEAD-match check happens — a HEAD mismatch there does leave an empty review
session (rare: requires the right branch and a clean tree but a different
commit than the trail tip).

Also factor out two shared helpers while here: `compareLocalHead` (now used
by both the new head check and the existing `verifyTrailReviewHead`) and
`gitStatusDirty` (now backing `HasUncommittedChanges` and the file-scoped
clean check).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 3e51051232cb
@Soph Soph force-pushed the soph/trail-finding-selected-text branch from 7520d4a to 7335c7d Compare June 17, 2026 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants