Skip to content
Draft
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
3 changes: 3 additions & 0 deletions .changes/unreleased/Added-20260601-142716.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
kind: Added
body: '''branch comment'': Inline comments now report ''outdated'' automatically when the anchored line is modified between the comment''s commit and the change''s current head (shamhub forge); previously a static field.'
time: 2026-06-01T14:27:16.137838-04:00
37 changes: 37 additions & 0 deletions internal/diffmap/mapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,43 @@ func (m *Mapper) Files() []string {
return files
}

// LineModified reports whether the given file:line was changed
// by this diff. side selects which side of the diff to inspect:
// "LEFT" (or "left") tests the NEW range of each hunk (the diff's
// "+" side), "RIGHT" (or "right", and the default) tests the OLD
// range (the diff's "-" side).
//
// The "RIGHT"-tests-OLD convention exists because callers ask
// stale-style questions: "this comment was anchored to line N on
// the RIGHT side of the original PR diff (i.e., the post-commit
// version of the file). I am now looking at the diff between the
// post-commit and the current head — has line N been touched in
// the post-commit's frame of reference?" That frame is the OLD
// side of the post-commit..head diff.
func (m *Mapper) LineModified(file string, line int, side string) bool {
fd, ok := m.files[file]
if !ok {
return false
}
useNewRange := strings.EqualFold(side, "LEFT")
for _, h := range fd.hunks {
if useNewRange {
if h.newCount > 0 &&
line >= h.newStart &&
line < h.newStart+h.newCount {
return true
}
} else {
if h.oldCount > 0 &&
line >= h.oldStart &&
line < h.oldStart+h.oldCount {
return true
}
}
}
return false
}

// parseHunkHeader parses "@@ -old,count +new,count @@".
func parseHunkHeader(line string) (hunk, error) {
// Strip "@@ " prefix and " @@..." suffix.
Expand Down
5 changes: 4 additions & 1 deletion internal/forge/shamhub/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,13 @@ runCommand:
flag := flag.NewFlagSet("shamhub comment edit", flag.ContinueOnError)
flag.SetOutput(logw)
flag.Usage = func() {
fmt.Fprintln(logw, "usage: shamhub comment edit [-resolved=true|false] <id>")
fmt.Fprintln(logw, "usage: shamhub comment edit [-resolved=true|false] [-outdated=true|false] <id>")
}

var resolved optionalBoolFlag
var outdated optionalBoolFlag
flag.Var(&resolved, "resolved", "set whether the comment is resolved")
flag.Var(&outdated, "outdated", "force the comment to be reported as outdated/stale")
ts.Check(flag.Parse(args[1:]))
args = flag.Args()
if len(args) != 1 {
Expand All @@ -135,6 +137,7 @@ runCommand:
ts.Check(sh.EditComment(EditCommentRequest{
ID: id,
Resolved: resolved.Ptr(),
Outdated: outdated.Ptr(),
}))

case "delete":
Expand Down
20 changes: 20 additions & 0 deletions internal/forge/shamhub/comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,12 @@ type EditCommentRequest struct {
// Resolved optionally updates the resolved state.
// Nil leaves the field unchanged.
Resolved *bool

// Outdated optionally forces the outdated/stale flag, bypassing
// the diff-based computation on list. Nil leaves the field
// unchanged; tests use this to assert the stale path renders
// correctly without setting up a real divergent commit.
Outdated *bool
}

// EditComment updates the state of a comment.
Expand All @@ -110,6 +116,9 @@ func (sh *ShamHub) EditComment(req EditCommentRequest) error {
if req.Resolved != nil {
updated.Resolved = *req.Resolved
}
if req.Outdated != nil {
updated.Outdated = *req.Outdated
}

if err := validateCommentState(updated.Resolvable, updated.Resolved); err != nil {
return err
Expand Down Expand Up @@ -157,6 +166,17 @@ type shamComment struct {
// ThreadID groups inline comments into threads.
ThreadID string

// CommitSHA is the head SHA of the change at the time the
// comment was posted. Used to compute whether the comment is
// stale relative to the change's current head.
// Empty for legacy or test-seeded comments.
CommitSHA string

// Outdated, if true, forces the comment to be reported as
// outdated regardless of git state. Tests use this to drive
// the stale path without setting up actual diff history.
Outdated bool

// Author is the comment author username.
Author string

Expand Down
122 changes: 119 additions & 3 deletions internal/forge/shamhub/inline_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@ import (
"context"
"fmt"
"strconv"
"strings"
"time"

"go.abhg.dev/gs/internal/diffmap"
"go.abhg.dev/gs/internal/forge"
"go.abhg.dev/gs/internal/xec"
)

var (
Expand Down Expand Up @@ -69,17 +72,40 @@ type inlineCommentItem struct {
}

func (sh *ShamHub) handleListInlineComments(
_ context.Context,
ctx context.Context,
req *listInlineCommentsRequest,
) (*listInlineCommentsResponse, error) {
sh.mu.RLock()
defer sh.mu.RUnlock()

// Locate the change once so the per-comment stale check has a
// reference to compare each comment's anchor SHA against.
var change *shamChange
for i, c := range sh.changes {
if c.Base.Owner == req.Owner &&
c.Base.Repo == req.Repo &&
c.Number == req.Change {
change = &sh.changes[i]
break
}
}

var items []inlineCommentItem
for _, c := range sh.comments {
if c.Change != req.Change || c.Path == "" {
continue
}
outdated := c.Outdated
if !outdated && change != nil {
stale, err := sh.commentIsStale(ctx, &c, change)
if err != nil {
sh.log.Warn("compute stale",
"comment", c.ID,
"error", err)
} else {
outdated = stale
}
}
items = append(items, inlineCommentItem{
ID: c.ID,
ThreadID: c.ThreadID,
Expand All @@ -88,14 +114,75 @@ func (sh *ShamHub) handleListInlineComments(
Body: c.Body,
Author: c.Author,
Resolved: c.Resolved,
Outdated: false,
Outdated: outdated,
CreatedAt: c.CreatedAt,
})
}

return &listInlineCommentsResponse{Items: items}, nil
}

// commentIsStale reports whether the comment's anchored line has
// been touched between the comment's recorded CommitSHA and the
// change's current head. Comments without a recorded CommitSHA
// (e.g. test-seeded entries) are never considered stale by this
// path; tests can still force stale via the Outdated override.
func (sh *ShamHub) commentIsStale(
ctx context.Context, c *shamComment, change *shamChange,
) (bool, error) {
if c.CommitSHA == "" {
return false, nil
}
headSHA, err := sh.resolveBranchSHA(
ctx, change.Head.Owner, change.Head.Repo, change.Head.Name,
)
if err != nil {
return false, err
}
if headSHA == c.CommitSHA {
return false, nil
}

out, err := xec.Command(ctx, sh.log, sh.gitExe,
"diff", "--unified=0",
c.CommitSHA+".."+headSHA, "--", c.Path).
WithDir(sh.repoDir(change.Head.Owner, change.Head.Repo)).
Output()
if err != nil {
// If the file is gone or the diff fails, treat as stale:
// the comment's anchor can no longer be located cleanly.
return true, nil
}
if len(out) == 0 {
return false, nil
}

mapper, err := diffmap.New(out)
if err != nil {
return false, fmt.Errorf("parse diff: %w", err)
}
side := c.Side
if side == "" {
side = "RIGHT"
}
return mapper.LineModified(c.Path, c.Line, side), nil
}

// resolveBranchSHA returns the current SHA of the named branch in
// the given owner/repo's worktree.
func (sh *ShamHub) resolveBranchSHA(
ctx context.Context, owner, repo, branch string,
) (string, error) {
out, err := xec.Command(ctx, sh.log, sh.gitExe, "rev-parse", branch).
WithDir(sh.repoDir(owner, repo)).
Output()
if err != nil {
return "", fmt.Errorf(
"resolve %s/%s:%s: %w", owner, repo, branch, err)
}
return strings.TrimSpace(string(out)), nil
}

func (r *forgeRepository) ListInlineComments(
ctx context.Context,
id forge.ChangeID,
Expand Down Expand Up @@ -148,9 +235,37 @@ type postInlineCommentResponse struct {
}

func (sh *ShamHub) handlePostInlineComment(
_ context.Context,
ctx context.Context,
req *postInlineCommentRequest,
) (*postInlineCommentResponse, error) {
// Look up the change's head branch BEFORE taking the write lock
// so the rev-parse subprocess doesn't fight other writers.
sh.mu.RLock()
var headOwner, headRepo, headBranch string
for _, c := range sh.changes {
if c.Base.Owner == req.Owner &&
c.Base.Repo == req.Repo &&
c.Number == req.Change {
headOwner = c.Head.Owner
headRepo = c.Head.Repo
headBranch = c.Head.Name
break
}
}
sh.mu.RUnlock()

var commitSHA string
if headBranch != "" {
sha, err := sh.resolveBranchSHA(ctx, headOwner, headRepo, headBranch)
if err != nil {
sh.log.Warn("capture comment commitSHA",
"change", req.Change,
"error", err)
} else {
commitSHA = sha
}
}

sh.mu.Lock()
defer sh.mu.Unlock()

Expand All @@ -168,6 +283,7 @@ func (sh *ShamHub) handlePostInlineComment(
Path: req.Path,
Line: req.Line,
Side: req.Side,
CommitSHA: commitSHA,
ThreadID: threadID,
Resolvable: true,
Author: "test-user",
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading