Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added

- The Pulls screen shows a changed image in a pull request's Files tab, before and after side by side, as the Review tab does, where it used to say only "Binary file". An edited Markdown file there has the preview button too, where only an added one had it. lich reads the file from your clone when it holds the commit and from GitHub otherwise.
- `lich mcp` answers MCP clients on protocol revision 2026-07-28, which name the version on each request instead of opening with `initialize`, and serves their `server/discover`. Clients that open with `initialize` are answered exactly as before.

### Fixed
Expand Down
17 changes: 10 additions & 7 deletions docs/ceilings.md
Original file line number Diff line number Diff line change
Expand Up @@ -439,11 +439,13 @@ work when nobody knows it and that the call site never shows. The mechanism and
And there is no expanding *past the last hunk*: a unified diff carries
no file length, so nothing here knows whether anything follows it, and an affordance drawn there would
be a no-op on every file whose change reaches the end.
- **An image preview reads only what this clone holds** (`internal/project/blob.go`): `/blob` serves the
working tree, `HEAD` or a tree object, and has no GitHub fallback the way `FileLines` does, so the Pulls
screen still prints "Binary file". Adding it there takes two reads this endpoint does not make: the head blob
from the contents API, and the old side at the merge base, which gh does not report (`baseRefOid` is the base
branch's tip, and an image the base also changed would show the wrong "before"). SVG is refused on purpose:
- **A pull request's image costs GitHub calls the clone could not answer** (`internal/project/blob.go`): `/blob`
reads an object this clone lacks from the contents API, the way `FileLines` does, and the old side stands on
the merge base, which gh does not report (`baseRefOid` is the base branch's tip, wrong once the base moved).
`PullRequestMergeBase` asks GitHub for both tips, and computes the merge base locally only when the clone
holds both, else asks the compare API too. The window asks it once per pull request head, shared by every
image card, and again only after a failure; each image side the clone lacks is still one contents call, and
a GitHub read is checked against the size cap only after the whole body arrived. SVG is refused on purpose:
git diffs it as text, and the window has no SVG renderer. One side
is capped at `maxPreviewBytes` (20 MiB) and held whole in memory, and a Git LFS file shows the pointer's
text diff, never the image. The window decodes in Go (`native/cmd/lich-native/blob_preview.go`) and `/blob` serves only what it
Expand All @@ -463,8 +465,9 @@ work when nobody knows it and that the call site never shows. The mechanism and
- **A Markdown preview shows the file's new version, never the change** (`native/cmd/lich-native/dock_review_markdown.go`):
the Review card's preview renders the whole file as the Pulls screen renders a PR body, with nothing marking
what was added or removed, no line to hang a review comment on and no selection to inject. It reads the
working tree (`project.ReadFile`, as the Code tab does), so on the Pulls screen only an added file, whose patch is the whole file, has a
preview; an edited one has no toggle for the same reason an image there has no preview. A relative image
working tree (`project.ReadFile`, as the Code tab does); on the Pulls screen an added file renders its patch,
and an edited one is read at the pull request's head (`project.ReadFileAt`), from GitHub when the clone does
not hold it, under ReadFile's size cap. A relative image
shows its alt text (the renderer fetches nothing) and raw HTML is left out. A new version of the file puts
the card back on its diff, as it reopens or folds a card by its size.

Expand Down
66 changes: 62 additions & 4 deletions internal/project/blob.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,15 @@ type Blob struct {
ContentType string
}

// githubBlobTimeout bounds one image read from the contents API: up to
// maxPreviewBytes over the network, well past what prReadTimeout allows a
// JSON answer.
const githubBlobTimeout = 30 * time.Second

// Blob reads rel at ref for an image preview. ref is "" for the working tree,
// "HEAD" for the commit a checkout's uncommitted diff stands on, or an object
// this clone holds. A pull request's head is
// not reached: unlike FileLines there is no GitHub fallback.
// id. An object this clone does not hold, a pull request's head or merge base,
// is read from GitHub, the way FileLines reads it.
//
// A path absent at ref is fs.ErrNotExist: the side of an added or deleted file
// that has nothing to show.
Expand All @@ -66,10 +71,13 @@ func (s *Service) Blob(dir, rel, ref string) (Blob, error) {
}
var data []byte
var err error
if ref == "" {
switch {
case ref == "":
data, err = workTreeBlob(dir, rel)
} else {
case ref == "HEAD" || holdsObject(dir, ref):
data, err = objectBlob(dir, rel, ref)
default:
data, err = s.githubBlob(dir, rel, ref)
}
if err != nil {
return Blob{}, err
Expand Down Expand Up @@ -132,6 +140,56 @@ func objectBlob(dir, rel, ref string) ([]byte, error) {
return []byte(out), nil
}

// holdsObject reports whether this clone has the object ref names.
func holdsObject(dir, ref string) bool {
_, ok := gitQuiet(dir, "cat-file", "-e", ref)
return ok
}

// githubBlob reads rel at a commit this clone does not hold. The size is only
// known once the bytes are here: the raw contents answer carries no length
// worth trusting ahead of the body.
func (s *Service) githubBlob(dir, rel, ref string) ([]byte, error) {
out, err := s.gh(githubBlobTimeout, dir, "api", "-H", ghRawAccept, contentsPath(rel, ref))
if err != nil {
return nil, err
}
if err := checkPreviewSize(int64(len(out))); err != nil {
return nil, err
}
return out, nil
}

// PullRequestMergeBase is the commit pull request number's diff is taken
// from, the old side of every file GitHub shows. The base branch's tip is not
// it once the base moved past where the branch left it. The tips come from
// GitHub; their merge base is computed here when the clone holds both, and
// asked of GitHub otherwise.
func (s *Service) PullRequestMergeBase(path string, number int) (string, error) {
out, err := s.gh(prReadTimeout, path, "api", fmt.Sprintf("repos/{owner}/{repo}/pulls/%d", number),
"--jq", `.base.sha + " " + .head.sha`)
if err != nil {
return "", err
}
base, head, _ := strings.Cut(strings.TrimSpace(string(out)), " ")
if !isOID(base) || !isOID(head) {
return "", errUnreadableAnswer(fmt.Errorf("pull request tips %q", out))
}
if local, ok := gitQuiet(path, "merge-base", base, head); ok {
return strings.TrimSpace(local), nil
}
out, err = s.gh(prReadTimeout, path, "api", "repos/{owner}/{repo}/compare/"+base+"..."+head,
"--jq", ".merge_base_commit.sha")
if err != nil {
return "", err
}
mergeBase := strings.TrimSpace(string(out))
if !isOID(mergeBase) {
return "", errUnreadableAnswer(fmt.Errorf("merge base %q", out))
}
return mergeBase, nil
}

func checkPreviewSize(size int64) error {
if size > maxPreviewBytes {
return fmt.Errorf("%.1f MB, %w", float64(size)/(1<<20), errPreviewTooLarge)
Expand Down
128 changes: 128 additions & 0 deletions internal/project/blob_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"net/http"
"net/http/httptest"
"net/url"
"slices"
"strings"
"testing"
)
Expand Down Expand Up @@ -68,6 +69,133 @@ func TestBlobMissingSideIsNotExist(t *testing.T) {
}
}

// A pull request's commits need not be in the clone; GitHub answers for them.
func TestBlobMissingObjectAsksGitHub(t *testing.T) {
t.Parallel()
repo, _ := initRepo(t)
missing := strings.Repeat("b", 40)

gh := &fakeGH{out: []byte(pngBytes)}
blob, err := withGH(gh).Blob(repo, "img/logo.png", missing)
if err != nil {
t.Fatalf("Blob: %v", err)
}
if string(blob.Data) != pngBytes || blob.ContentType != "image/png" {
t.Errorf("got %q as %q, want GitHub's image", blob.Data, blob.ContentType)
}
want := []string{"api", "-H", ghRawAccept, contentsPath("img/logo.png", missing)}
if !slices.Equal(gh.args, want) {
t.Errorf("gh args = %v, want %v", gh.args, want)
}
}

// A commit the clone holds is read locally, even when the path is absent at
// it: that answer is the clone's to give, not GitHub's.
func TestBlobLocalObjectNeverAsksGitHub(t *testing.T) {
t.Parallel()
repo, git := initRepo(t)
write(t, repo, "logo.png", pngBytes)
git("add", ".")
git("commit", "-m", "logo")
head := strings.TrimSpace(git("rev-parse", "HEAD"))

gh := &fakeGH{err: errors.New("gh must not run")}
svc := withGH(gh)
if blob, err := svc.Blob(repo, "logo.png", head); err != nil || string(blob.Data) != pngBytes {
t.Errorf("present: %q, %v; want the committed image", blob.Data, err)
}
if _, err := svc.Blob(repo, "gone.png", head); !errors.Is(err, fs.ErrNotExist) {
t.Errorf("absent: err = %v, want fs.ErrNotExist", err)
}
if gh.calls != 0 {
t.Errorf("gh ran %d times, want 0", gh.calls)
}
}

func TestBlobGitHubSizeLimit(t *testing.T) {
t.Parallel()
repo, _ := initRepo(t)
gh := &fakeGH{out: make([]byte, maxPreviewBytes+1)}
if _, err := withGH(gh).Blob(repo, "big.png", strings.Repeat("c", 40)); !errors.Is(err, errPreviewTooLarge) {
t.Errorf("err = %v, want errPreviewTooLarge", err)
}
}

func TestBlobGitHubFailureSurfaces(t *testing.T) {
t.Parallel()
repo, _ := initRepo(t)
gh := &fakeGH{err: errors.New("gh: Not Found (HTTP 404)")}
if _, err := withGH(gh).Blob(repo, "x.png", strings.Repeat("c", 40)); err == nil || errors.Is(err, fs.ErrNotExist) {
t.Errorf("err = %v, want gh's failure, not a missing side", err)
}
}

// Both tips in the clone: git computes the merge base, GitHub is asked only
// for the tips.
func TestPullRequestMergeBaseLocal(t *testing.T) {
t.Parallel()
repo, git := initRepo(t)
write(t, repo, "fork.txt", "fork\n")
git("add", ".")
git("commit", "-m", "fork point")
fork := strings.TrimSpace(git("rev-parse", "HEAD"))
git("checkout", "-q", "-b", "topic")
write(t, repo, "b.txt", "topic\n")
git("add", ".")
git("commit", "-m", "topic")
head := strings.TrimSpace(git("rev-parse", "HEAD"))
git("checkout", "-q", "-")
write(t, repo, "c.txt", "base moved\n")
git("add", ".")
git("commit", "-m", "base")
base := strings.TrimSpace(git("rev-parse", "HEAD"))

gh := &fakeGH{out: []byte(base + " " + head + "\n")}
got, err := withGH(gh).PullRequestMergeBase(repo, 7)
if err != nil {
t.Fatalf("PullRequestMergeBase: %v", err)
}
if got != fork {
t.Errorf("merge base = %s, want the fork point %s", got, fork)
}
if want := []string{"api", "repos/{owner}/{repo}/pulls/7", "--jq", `.base.sha + " " + .head.sha`}; gh.calls != 1 || !slices.Equal(gh.args, want) {
t.Errorf("gh calls = %v, want one: %v", gh.history, want)
}
}

func TestPullRequestMergeBaseAsksGitHub(t *testing.T) {
t.Parallel()
repo, _ := initRepo(t)
base, head, mergeBase := strings.Repeat("a", 40), strings.Repeat("b", 40), strings.Repeat("c", 40)

gh := &fakeGH{outs: [][]byte{[]byte(base + " " + head + "\n"), []byte(mergeBase + "\n")}}
got, err := withGH(gh).PullRequestMergeBase(repo, 7)
if err != nil {
t.Fatalf("PullRequestMergeBase: %v", err)
}
if got != mergeBase {
t.Errorf("merge base = %s, want %s", got, mergeBase)
}
want := []string{"api", "repos/{owner}/{repo}/compare/" + base + "..." + head, "--jq", ".merge_base_commit.sha"}
if !slices.Equal(gh.args, want) {
t.Errorf("gh args = %v, want %v", gh.args, want)
}
}

func TestPullRequestMergeBaseRejectsAnUnreadableAnswer(t *testing.T) {
t.Parallel()
repo, _ := initRepo(t)
tips := []byte(strings.Repeat("a", 40) + " " + strings.Repeat("b", 40))
for name, outs := range map[string][][]byte{
"tips": {[]byte("null null")},
"merge base": {tips, []byte("null")},
} {
if _, err := withGH(&fakeGH{outs: outs}).PullRequestMergeBase(repo, 7); err == nil {
t.Errorf("%s: want an error, got nil", name)
}
}
}

func TestBlobRefusals(t *testing.T) {
t.Parallel()
repo, git := initRepo(t)
Expand Down
10 changes: 10 additions & 0 deletions internal/project/filelines.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,16 @@ func (s *Service) FileLines(path, rel, ref string, from, to int) ([]string, erro
return sliceLines(text, from, to), nil
}

// ReadFileAt is ReadFile at ref, for a preview of a file's new version that
// the working tree does not hold: a pull request's head. See FileLines for
// what a ref can be and where it is read from.
func (s *Service) ReadFileAt(path, rel, ref string) (string, error) {
if err := relpath.Validate(rel); err != nil {
return "", err
}
return s.fileAt(path, rel, ref)
}

// fileAt reads one file whole at ref. See FileLines for what a ref can be.
func (s *Service) fileAt(path, rel, ref string) (string, error) {
if ref == "" {
Expand Down
26 changes: 26 additions & 0 deletions internal/project/filelines_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,32 @@ func TestFileLinesMissingObjectAsksGitHub(t *testing.T) {
}
}

func TestReadFileAt(t *testing.T) {
t.Parallel()
repo, git := initRepo(t)
write(t, repo, "notes.md", "# old\n")
git("add", ".")
git("commit", "-m", "notes")
head := strings.TrimSpace(git("rev-parse", "HEAD"))
write(t, repo, "notes.md", "# new\n")

gh := &fakeGH{out: []byte("# from GitHub\n")}
svc := withGH(gh)
if text, err := svc.ReadFileAt(repo, "notes.md", head); err != nil || text != "# old\n" {
t.Errorf("local commit: %q, %v; want the committed text", text, err)
}
if gh.calls != 0 {
t.Errorf("gh ran %d times for a local commit, want 0", gh.calls)
}
missing := strings.Repeat("e", 40)
if text, err := svc.ReadFileAt(repo, "notes.md", missing); err != nil || text != "# from GitHub\n" {
t.Errorf("absent commit: %q, %v; want GitHub's text", text, err)
}
if _, err := svc.ReadFileAt(repo, "../x.md", head); err == nil {
t.Error("traversal: want an error, got nil")
}
}

func TestFileLinesGitHubRejectsOversize(t *testing.T) {
t.Parallel()
repo, _ := initRepo(t)
Expand Down
22 changes: 22 additions & 0 deletions native/cmd/lich-native/blob_preview.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
_ "image/jpeg"
_ "image/png"
"path"
"strconv"
"strings"

"gioui.org/layout"
Expand Down Expand Up @@ -155,6 +156,27 @@ func (v *blobView) want(client *lichclient.Client, checkout, rel, ref, version s
})
}

// wantMergeBase reads rel at pull request pr's merge base, the commit its
// old side stands on.
func (v *blobView) wantMergeBase(client *lichclient.Client, checkout, rel string, pr *prFiles, version string) {
number := pr.number
key := strings.Join([]string{checkout, rel, strconv.Itoa(number), version}, "\x00")
baseKey := strings.Join([]string{checkout, strconv.Itoa(number), pr.head}, "\x00")
v.read.want(key, checkout, func(ctx context.Context) (*previewImage, error) {
base, err := pr.mergeBase.get(ctx, baseKey, func() (string, error) {
return client.PullRequestMergeBase(context.Background(), checkout, number)
})
if err != nil {
return nil, err
}
data, err := client.Blob(ctx, checkout, rel, base)
if err != nil {
return nil, err
}
return decodePreview(data)
})
}

// layout is the image over its caption, or the line saying why there is none.
func (v *blobView) layout(gtx C, th *ui.Theme) D {
img, loading, err := v.read.get()
Expand Down
7 changes: 5 additions & 2 deletions native/cmd/lich-native/dock_review.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,11 @@ type reviewPanel struct {
// checkout's working tree: nothing to revert or discard, its gaps read at the
// PR's head, and each file tickable as viewed.
type prFiles struct {
head string
url string // the pull request's page
number int
head string
// mergeBase is the old side every image card reads, asked once per head.
mergeBase mergeBaseMemo
url string // the pull request's page
// viewed is each ticked file's diff text when it was ticked: a commit
// that changes the file unticks it, one that does not leaves it.
viewed map[string]string
Expand Down
Loading
Loading