diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a7f84c9b..b4d7363fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **The Commits tab says which GitHub account each commit actually landed + under.** A project's GitHub account governs what lich asks `gh`; it has never + governed the push, which still signs with your global `user.email`. So a pull + request could be read by one account and its commits land under another, or + under no account at all, and nothing said a word. Each commit row now shows + the account GitHub itself resolved the author's email to — GitHub checks it + against every account's verified addresses, so noreply forms, vanity domains + and organisation aliases all resolve correctly — and a commit belonging to no + account is marked as one. When the commits are not all the pull request + author's, a line above the list names the accounts they landed under and the + addresses no account owns, while the branch can still be fixed. + ### Changed - **The two sandbox grants say what they hand over.** In Settings › Sandbox, the diff --git a/docs/ceilings.md b/docs/ceilings.md index bd530e9dc..caa582aba 100644 --- a/docs/ceilings.md +++ b/docs/ceilings.md @@ -8,9 +8,25 @@ work when nobody knows it and that the call site never shows. The mechanism and - **A project's gh account governs gh, not git**: `vcs.account` (`internal/project/ghaccount.go`) puts one account's token in `GH_TOKEN` for every gh call lich makes for that project. A push still rides the remote's ssh key and signs with the global `user.email`, so a PR can be *read* by one account and its commits *land* - under another, with no error anywhere. The Version Control settings print both identities and never compare - them: noreply forms, vanity domains and org aliases make a mismatch warning a false-positive farm. lich never - writes `user.email`. + under another. lich never writes `user.email`, and the Version Control settings still print both identities + without comparing them — noreply forms, vanity domains and org aliases make a string comparison a + false-positive farm. + The Commits tab of the Pulls screen is where the two are finally reconciled, and only there + (`frontend/src/lib/pulls/commit-authors.ts`): GitHub has already resolved each commit's author email against + the verified addresses of every account, so its answer is set membership, not a guess. **The window is after + the push and before the merge.** Nothing reads before a push — the resolution does not exist until GitHub has + the commit — so a branch can still be built entirely under the wrong identity and only say so once its pull + request is open. Two things keep it there: `gh api user/emails` would answer for an unpushed commit, but it + needs the `user` scope that gh's own login does not request (measured: `repo, read:org, gist`) and its refusal + is an HTTP 404 indistinguishable from any other not-found, on a path where gh's stderr never reaches the UI; + and a commit's *signature* is a second identity this compares nothing about, so a commit signed by a key the + account does not list reads as clean. + The reading is also about the pull request, not about you: it compares each commit against whoever opened the + PR, the one identity the payload already carries. So a branch somebody else built collaboratively — a + teammate's commit, a Copilot-authored branch — reads as commits under another account on a PR you are only + reviewing. The statement is true and the tone is factual rather than an alarm, but the line does speak on pull + requests where nothing is wrong. Narrowing it to *your* pull requests means resolving which account lich runs + gh as, which `vcs.account` only answers when the project has named one. - **`LICH_WORKTREE_PORT` is reserved, never held** (`internal/terminal/worktreeport.go`): the number is a name the checkout owns, nothing binds it, and anything on the machine can take the port before the dev server starts. A Run card shortens that window rather than closing it — the process it starts is what binds the port, and diff --git a/frontend/src/components/pulls/PullRequestView.tsx b/frontend/src/components/pulls/PullRequestView.tsx index 5f44d9430..9a0f166f4 100644 --- a/frontend/src/components/pulls/PullRequestView.tsx +++ b/frontend/src/components/pulls/PullRequestView.tsx @@ -493,7 +493,9 @@ export function PullRequestView({ ) : (
{tab === "checks" && } - {tab === "commits" && } + {tab === "commits" && ( + + )} {tab === "conversation" && ( No commits. } + const notice = commitAuthorNotice(commits, authorLogin) return (
+ {notice && ( +

+ {notice} +

+ )} {commits.map((commit) => ( ))} @@ -40,7 +58,7 @@ function CommitRow({ commit }: { commit: PullRequestCommit }) { {commit.headline} - {commitMeta(commit)} + {commitMetaLine(commit)} {body !== "" && ( ) } - -// Who landed it and when, in git's own phrasing. Either half can be missing — a -// merge commit from the web flow carries no author — so the line is built from -// what is there instead of printing an empty "committed on". -function commitMeta(commit: PullRequestCommit): string { - const at = new Date(commit.date) - const date = Number.isNaN(at.getTime()) ? "" : at.toLocaleDateString() - if (commit.author && date) { - return `${commit.author} committed ${date}` - } - return commit.author || (date && `Committed ${date}`) || "" -} diff --git a/frontend/src/components/pulls/pulls-open-session.test.tsx b/frontend/src/components/pulls/pulls-open-session.test.tsx index 0cea15ed9..8886d96ce 100644 --- a/frontend/src/components/pulls/pulls-open-session.test.tsx +++ b/frontend/src/components/pulls/pulls-open-session.test.tsx @@ -26,6 +26,7 @@ const DETAIL: PullRequestDetail = { title: "A pull request", body: "", author: "someone", + authorLogin: "someone", state: "OPEN", isDraft: false, mergeable: "MERGEABLE", diff --git a/frontend/src/lib/api-types.ts b/frontend/src/lib/api-types.ts index 4fc43d4be..b309f9f10 100644 --- a/frontend/src/lib/api-types.ts +++ b/frontend/src/lib/api-types.ts @@ -107,8 +107,14 @@ export interface PullRequestCommit { headline: string /** The rest of the message; "" for a one-line commit. */ body: string - /** The first author's login, or their name; "" when gh reports none. */ - author: string + /** The GitHub account GitHub itself resolved the author's email to, and "" + * when it resolved none: the email belongs to no account, so the commit is + * attributed to nobody. Never falls back to the name — that is the fact. */ + login: string + /** git's author name: free text, and no evidence of who landed the commit. */ + name: string + /** git's author email, which is what GitHub did or did not resolve. */ + email: string /** gh's ISO committedDate. */ date: string } @@ -140,6 +146,9 @@ export interface PullRequestDetail { body: string /** Who opened it: a login, or a display name when gh reports no login. */ author: string + /** The same account without that fallback: "" rather than a display name, so + * it can be compared against a commit's login without inventing a mismatch. */ + authorLogin: string /** gh: OPEN | CLOSED | MERGED. Only a number-addressed lookup returns a * non-OPEN one; the branch lookup still hides them. */ state: string diff --git a/frontend/src/lib/pulls/commit-authors.test.ts b/frontend/src/lib/pulls/commit-authors.test.ts new file mode 100644 index 000000000..192221b7e --- /dev/null +++ b/frontend/src/lib/pulls/commit-authors.test.ts @@ -0,0 +1,133 @@ +import { describe, expect, it } from "vitest" +import type { PullRequestCommit } from "@/lib/api-types" +import { commitAuthorNotice, commitMetaLine } from "./commit-authors" + +const DATE = new Date("2026-07-25T22:32:47Z").toLocaleDateString() + +function commit(fields: Partial): PullRequestCommit { + return { + oid: "a4dbc1f", + headline: "feat: the panel", + body: "", + login: "", + name: "", + email: "", + date: "2026-07-25T22:32:47Z", + ...fields, + } +} + +describe("commitMetaLine", () => { + it("shows GitHub's own answer as an account", () => { + expect(commitMetaLine(commit({ login: "omartelo", name: "martelo" }))).toBe( + `@omartelo committed ${DATE}`, + ) + }) + + // The whole failure: without the mark, this row reads exactly like the one + // above it, and a commit belonging to nobody reaches a merge unremarked. + it("marks a commit GitHub could not attribute", () => { + expect(commitMetaLine(commit({ name: "Ada Lovelace", email: "ada@analytical.example" }))).toBe( + `Ada Lovelace committed ${DATE} · linked to no GitHub account`, + ) + }) + + // A merge commit from the web flow carries no author at all. Nobody's + // identity is wrong there, so the mark would be a false alarm. + it("leaves a commit with no author alone", () => { + expect(commitMetaLine(commit({}))).toBe(`Committed ${DATE}`) + }) + + it("keeps the line whole when gh reports no date", () => { + expect(commitMetaLine(commit({ login: "omartelo", date: "" }))).toBe("@omartelo") + expect(commitMetaLine(commit({ date: "" }))).toBe("") + }) +}) + +describe("commitAuthorNotice", () => { + it("stays quiet when every commit is the pull request author's", () => { + const commits = [commit({ login: "omartelo" }), commit({ oid: "f55ebe9", login: "omartelo" })] + expect(commitAuthorNotice(commits, "omartelo")).toBe("") + }) + + it("stays quiet with no commits at all", () => { + expect(commitAuthorNotice(null, "omartelo")).toBe("") + expect(commitAuthorNotice([], "omartelo")).toBe("") + }) + + // The ceiling this exists for: read by one account, landed under another. + it("names the account the commits actually landed under", () => { + const commits = [ + commit({ login: "marcelo-filho_snk" }), + commit({ oid: "f55ebe9", login: "marcelo-filho_snk" }), + ] + expect(commitAuthorNotice(commits, "omartelo")).toBe( + "2 commits landed under @marcelo-filho_snk, not @omartelo.", + ) + }) + + it("names each stray account once, however many commits it landed", () => { + const commits = [ + commit({ login: "omartelo" }), + commit({ oid: "b1", login: "marcelo-filho_snk" }), + commit({ oid: "b2", login: "marcelo-filho_snk" }), + commit({ oid: "b3", login: "octocat" }), + ] + expect(commitAuthorNotice(commits, "omartelo")).toBe( + "3 commits landed under @marcelo-filho_snk, @octocat, not @omartelo.", + ) + }) + + it("folds a long list of accounts into a count", () => { + const commits = ["a", "b", "c", "d", "e"].map((login, i) => commit({ oid: `oid-${i}`, login })) + expect(commitAuthorNotice(commits, "omartelo")).toBe( + "5 commits landed under @a, @b, @c and 2 more, not @omartelo.", + ) + }) + + it("names the address no account owns, which is what has to be fixed", () => { + const commits = [commit({ name: "Ada Lovelace", email: "ada@analytical.example" })] + expect(commitAuthorNotice(commits, "omartelo")).toBe( + "1 commit is linked to no GitHub account: ada@analytical.example.", + ) + }) + + it("reports both readings when a branch carries both", () => { + const commits = [ + commit({ login: "marcelo-filho_snk" }), + commit({ oid: "b1", name: "Ada", email: "ada@analytical.example" }), + ] + expect(commitAuthorNotice(commits, "omartelo")).toBe( + "1 commit landed under @marcelo-filho_snk, not @omartelo. " + + "1 commit is linked to no GitHub account: ada@analytical.example.", + ) + }) + + // A commit gh reports with no email at all is still unattributed; the line + // says so without an empty colon trailing it. + it("drops the address list when there is no address to name", () => { + expect(commitAuthorNotice([commit({ name: "Ada" })], "omartelo")).toBe( + "1 commit is linked to no GitHub account.", + ) + }) + + // Same reason the row leaves it alone: gh reported no author, so there is no + // identity to call wrong. + it("never counts an authorless merge commit as unattributed", () => { + const commits = [commit({ login: "omartelo" }), commit({ oid: "b1" })] + expect(commitAuthorNotice(commits, "omartelo")).toBe("") + }) + + // gh reports no login for a deleted account. Comparing against "" would call + // every commit a stray, so the comparison is withheld — the unattributed + // reading needs no second identity and still stands. + it("withholds the comparison when the pull request author has no login", () => { + const commits = [ + commit({ login: "omartelo" }), + commit({ oid: "b1", name: "Ada", email: "ada@analytical.example" }), + ] + expect(commitAuthorNotice(commits, "")).toBe( + "1 commit is linked to no GitHub account: ada@analytical.example.", + ) + }) +}) diff --git a/frontend/src/lib/pulls/commit-authors.ts b/frontend/src/lib/pulls/commit-authors.ts new file mode 100644 index 000000000..10858b2ea --- /dev/null +++ b/frontend/src/lib/pulls/commit-authors.ts @@ -0,0 +1,87 @@ +import type { PullRequestCommit } from "@/lib/api-types" + +// How many addresses or accounts the line names before it folds the rest into a +// count. Three is enough to recognise the mistake; a branch that made it under +// four different identities has a bigger problem than a list can render. +const NAMED = 3 + +const NO_ACCOUNT = "linked to no GitHub account" + +// Whether gh reported an author for this commit at all. A web-flow merge commit +// carries none, and calling that one unattributed would be the false alarm this +// whole reading exists to avoid — nobody's identity is wrong, there is nobody. +function hasAuthor(commit: PullRequestCommit): boolean { + return commit.email !== "" || commit.name !== "" +} + +function unattributed(commit: PullRequestCommit): boolean { + return commit.login === "" && hasAuthor(commit) +} + +/** The line under a commit's subject: who landed it, when, and — when GitHub + * resolved the author's email to no account — that nobody owns it. A login is + * GitHub's own answer and is shown as one; without the mark, a commit belonging + * to nobody prints the git name and reads exactly like one that does. */ +export function commitMetaLine(commit: PullRequestCommit): string { + const at = new Date(commit.date) + const date = Number.isNaN(at.getTime()) ? "" : at.toLocaleDateString() + const who = commit.login ? `@${commit.login}` : commit.name + const landed = + who && date ? `${who} committed ${date}` : who || (date && `Committed ${date}`) || "" + if (!unattributed(commit)) { + return landed + } + return landed ? `${landed} · ${NO_ACCOUNT}` : `Committed by an address ${NO_ACCOUNT}` +} + +/** The one line above the commit list, or "" when there is nothing to say. + * + * Nothing here guesses. GitHub resolved every author email against the verified + * addresses of every account before answering, so a login that differs is a + * different account and an empty one is an email no account owns — the two + * readings a noreply form, a vanity domain or an org alias used to make + * unsayable. What the line cannot see is a commit that has not been pushed: + * before that, nobody has resolved anything. */ +export function commitAuthorNotice( + commits: PullRequestCommit[] | null, + authorLogin: string, +): string { + if (!commits || commits.length === 0) { + return "" + } + const strays = commits.filter((c) => c.login !== "" && c.login !== authorLogin) + const orphans = commits.filter(unattributed) + return [ + // Only when the pull request's own author is a login: gh reports none for a + // deleted account, and comparing against "" would call every commit a stray. + authorLogin && strays.length > 0 + ? `${count(strays.length, "commit")} landed under ${list(distinct(strays.map((c) => `@${c.login}`)))}, not @${authorLogin}.` + : "", + orphans.length > 0 + ? `${count(orphans.length, "commit")} ${orphans.length === 1 ? "is" : "are"} ${NO_ACCOUNT}${emails(orphans)}.` + : "", + ] + .filter(Boolean) + .join(" ") +} + +function emails(commits: PullRequestCommit[]): string { + const addresses = distinct(commits.map((c) => c.email).filter(Boolean)) + return addresses.length > 0 ? `: ${list(addresses)}` : "" +} + +function distinct(values: string[]): string[] { + return Array.from(new Set(values)) +} + +// Up to NAMED of them, then how many were held back — so a long list stays one +// line and still says it is not the whole list. +function list(values: string[]): string { + const hidden = values.length - NAMED + const shown = values.slice(0, NAMED).join(", ") + return hidden > 0 ? `${shown} and ${hidden} more` : shown +} + +function count(n: number, noun: string): string { + return `${n} ${noun}${n === 1 ? "" : "s"}` +} diff --git a/frontend/src/lib/pulls/merge-gate.test.ts b/frontend/src/lib/pulls/merge-gate.test.ts index 105340062..a8bd93502 100644 --- a/frontend/src/lib/pulls/merge-gate.test.ts +++ b/frontend/src/lib/pulls/merge-gate.test.ts @@ -23,6 +23,7 @@ const detail = (over: Partial = {}): PullRequestDetail => ({ title: "feat: something", body: "", author: "omartelo", + authorLogin: "omartelo", state: "OPEN", isDraft: false, mergeable: "MERGEABLE", diff --git a/frontend/src/lib/pulls/pr-handoff.test.ts b/frontend/src/lib/pulls/pr-handoff.test.ts index 8ce501a0a..263a3f099 100644 --- a/frontend/src/lib/pulls/pr-handoff.test.ts +++ b/frontend/src/lib/pulls/pr-handoff.test.ts @@ -8,6 +8,7 @@ const detail = (over: Partial = {}): PullRequestDetail => ({ title: "feat: something", body: "", author: "omartelo", + authorLogin: "omartelo", state: "OPEN", isDraft: false, mergeable: "MERGEABLE", diff --git a/internal/project/pr.go b/internal/project/pr.go index 1c770d41a..380fd1ad2 100644 --- a/internal/project/pr.go +++ b/internal/project/pr.go @@ -98,6 +98,11 @@ type PRDetail struct { // reports no login. GitHub refuses a review request addressed to them, which // is what keeps them out of the picker. Author string `json:"author"` + // AuthorLogin is the same account as Author, minus the fallback to a display + // name — the value a commit's login is compared against. Author cannot serve: + // a name matches no login, so a PR whose author gh reports without one would + // read as every commit landing under the wrong account. + AuthorLogin string `json:"authorLogin"` // State is gh's OPEN | CLOSED | MERGED. Only a number-addressed lookup can // return a non-OPEN one; the branch lookup still gates them out. State string `json:"state"` @@ -133,11 +138,18 @@ type PRDetail struct { // PRCommit is one commit the pull request would land, split the way git itself // splits a message: the subject line, then the body (empty for a one-liner). +// +// Login and Name are kept apart because only one of them is evidence: Name is +// free text, Login is the account GitHub resolved the author's email to against +// every account's verified addresses — an answer lich cannot compute, and one +// whose "" means the email belongs to nobody rather than "unknown". type PRCommit struct { OID string `json:"oid"` Headline string `json:"headline"` Body string `json:"body"` - Author string `json:"author"` + Login string `json:"login"` + Name string `json:"name"` + Email string `json:"email"` Date string `json:"date"` // gh's ISO committedDate } @@ -178,6 +190,10 @@ type ghCommit struct { type ghCommitAuthor struct { Login string `json:"login"` Name string `json:"name"` + // Email is the address git recorded and GitHub resolved (or failed to). It + // is carried for the unresolved case alone: "this commit belongs to nobody" + // is only actionable next to the address that has to be added or corrected. + Email string `json:"email"` } // PullRequestDetail returns one open pull request in full: the given number, or @@ -218,6 +234,7 @@ func parsePRDetail(out []byte, openOnly bool) (*PRDetail, error) { Title: v.Title, Body: v.Body, Author: firstNonEmpty(v.Author.Login, v.Author.Name), + AuthorLogin: v.Author.Login, State: v.State, IsDraft: v.IsDraft, Mergeable: v.Mergeable, @@ -244,15 +261,20 @@ func toCommits(commits []ghCommit) []PRCommit { } out := make([]PRCommit, 0, len(commits)) for _, c := range commits { - author := "" + // Never collapsed to one display string: both kinds of commit carry a + // name, so falling back to it makes an unattributed commit read exactly + // like an attributed one. + var author ghCommitAuthor if len(c.Authors) > 0 { - author = firstNonEmpty(c.Authors[0].Login, c.Authors[0].Name) + author = c.Authors[0] } out = append(out, PRCommit{ OID: c.OID, Headline: c.MessageHeadline, Body: c.MessageBody, - Author: author, + Login: author.Login, + Name: author.Name, + Email: author.Email, Date: c.CommittedDate, }) } diff --git a/internal/project/pr_test.go b/internal/project/pr_test.go index 73011e622..c7f24d7cd 100644 --- a/internal/project/pr_test.go +++ b/internal/project/pr_test.go @@ -57,6 +57,24 @@ func TestParsePRDetail(t *testing.T) { } }) + // AuthorLogin is what a commit's login is compared against, so it must stay + // empty rather than borrow the display name: a name can never equal a login, + // and a borrowed one would report every commit as landing under another + // account. + t.Run("the author login never falls back to a display name", func(t *testing.T) { + out := []byte(`{"number":4,"state":"OPEN","author":{"name":"Ada Lovelace"}}`) + pr, err := parsePRDetail(out, true) + if err != nil || pr == nil { + t.Fatalf("unexpected parse result: %+v, %v", pr, err) + } + if pr.Author != "Ada Lovelace" { + t.Errorf("the printed author still falls back: %q", pr.Author) + } + if pr.AuthorLogin != "" { + t.Errorf("AuthorLogin borrowed the name: %q", pr.AuthorLogin) + } + }) + t.Run("draft flag survives", func(t *testing.T) { pr, err := parsePRDetail([]byte(`{"number":9,"state":"OPEN","isDraft":true}`), true) if err != nil { @@ -258,7 +276,9 @@ func TestToCommits(t *testing.T) { MessageHeadline: "fix: retire the badge", MessageBody: "The footer badge only looked up again on HEAD moves.", CommittedDate: "2026-07-25T22:32:47Z", - Authors: []ghCommitAuthor{{Login: "omartelo", Name: "omartelo"}}, + Authors: []ghCommitAuthor{ + {Login: "omartelo", Name: "omartelo", Email: "meopedevts@proton.me"}, + }, }, {OID: "f55ebe9", MessageHeadline: "docs: changelog"}, }) @@ -269,26 +289,39 @@ func TestToCommits(t *testing.T) { if first.OID != "a4dbc1f" || first.Headline != "fix: retire the badge" { t.Errorf("wrong header: %+v", first) } - if !strings.Contains(first.Body, "HEAD moves") || first.Author != "omartelo" { - t.Errorf("lost body or author: %+v", first) + if !strings.Contains(first.Body, "HEAD moves") || first.Login != "omartelo" { + t.Errorf("lost body or login: %+v", first) + } + if first.Email != "meopedevts@proton.me" { + t.Errorf("lost the author email: %q", first.Email) } if first.Date != "2026-07-25T22:32:47Z" { t.Errorf("wrong date: %q", first.Date) } // A one-line commit has no body and a web-flow merge commit no author; // both are rows, not errors. - if got[1].Body != "" || got[1].Author != "" { + if got[1].Body != "" || got[1].Login != "" || got[1].Name != "" { t.Errorf("second commit should be bare: %+v", got[1]) } }) - t.Run("a nameless login falls back to the name", func(t *testing.T) { + // Contract change: the login used to fall back to the name, which made a + // commit GitHub could not attribute read exactly like one it could. The two + // now stay apart, and an empty login is the answer "no account owns this + // email" rather than a missing value. + t.Run("an unattributed author keeps its name and reports no login", func(t *testing.T) { got := toCommits([]ghCommit{{ OID: "c0ffee0", - Authors: []ghCommitAuthor{{Name: "Ada Lovelace"}}, + Authors: []ghCommitAuthor{{Name: "Ada Lovelace", Email: "ada@analytical.example"}}, }}) - if len(got) != 1 || got[0].Author != "Ada Lovelace" { - t.Errorf("author fallback failed: %+v", got) + if len(got) != 1 { + t.Fatalf("want 1 commit, got %+v", got) + } + if got[0].Login != "" { + t.Errorf("a name must never be reported as a login: %+v", got[0]) + } + if got[0].Name != "Ada Lovelace" || got[0].Email != "ada@analytical.example" { + t.Errorf("lost the git author: %+v", got[0]) } }) }