Skip to content

fix: diff→symbol joins return empty results in multi-repo daemons #74

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zzet merged 6 commits into from
Jun 11, 2026
Merged

fix: diff→symbol joins return empty results in multi-repo daemons #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4c04ab3
analysis: repo-prefix-aware diff→symbol join
zzet Jun 11, 2026
0891ac8
analysis: pin git-diff header prefixes against mnemonicPrefix/noprefix
zzet Jun 11, 2026
f391e8f
mcp: resolve diff-handler repo scope from selector, lone repo, or ses…
zzet Jun 11, 2026
39be03a
review: one file-risk row per file, and a headline that explains BLOCK
zzet Jun 11, 2026
ea327a8
review: file-risk rows carry their evidence; coverage tempers the ver…
zzet Jun 11, 2026
45e3d4a
review: coverage blindness is "unknown", not "untested"
zzet Jun 11, 2026
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
83 changes: 18 additions & 65 deletions cmd/gortex/review.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"fmt"
"io"
"os"
"sort"
"strings"

"github.com/spf13/cobra"
Expand Down Expand Up @@ -180,9 +179,12 @@ type reviewPayloadCLI struct {
Source string `json:"source"`
} `json:"comments"`
FileRisk []struct {
File string `json:"file"`
Risk string `json:"risk"`
Findings int `json:"findings"`
File string `json:"file"`
Risk string `json:"risk"`
Findings int `json:"findings"`
Affected int `json:"affected"`
Symbols int `json:"symbols"`
Uncovered int `json:"uncovered"`
} `json:"file_risk"`
Depth string `json:"depth"`
Cost *struct {
Expand Down Expand Up @@ -225,9 +227,12 @@ func reviewReportFromPayload(p reviewPayloadCLI) *review.ReviewReport {
}
for _, fr := range p.FileRisk {
report.FileRisk = append(report.FileRisk, review.FileRisk{
File: fr.File,
Risk: fr.Risk,
Findings: fr.Findings,
File: fr.File,
Risk: fr.Risk,
Findings: fr.Findings,
Affected: fr.Affected,
Symbols: fr.Symbols,
Uncovered: fr.Uncovered,
})
}
if p.Cost != nil {
Expand Down Expand Up @@ -258,67 +263,15 @@ func printReview(cmd *cobra.Command, raw json.RawMessage) error {
return nil
}

// The agent audience renders the terse, machine-first summary from the
// canonical review renderer so the CLI and any sub-agent shelling the verb
// see byte-identical output. The human audience keeps the readable per-file
// packet below.
// Both audiences render through the canonical review renderer so the CLI,
// the MCP text rendering, and any sub-agent shelling the verb see the
// same packet — the human path used to keep a hand-rolled duplicate here,
// which silently dropped every field the renderer learned after it forked
// (coverage evidence, the rulepack-passed line).
if strings.EqualFold(reviewAudience, "agent") {
_, _ = io.WriteString(out, review.RenderSummary(reviewReportFromPayload(p), review.AudienceAgent))
return nil
}

verdict := p.Verdict
if verdict == "" {
verdict = "APPROVE"
}
_, _ = fmt.Fprintf(out, "Verdict: %s\n", verdict)
if p.Summary != "" {
_, _ = fmt.Fprintf(out, "%s\n", p.Summary)
}

if len(p.FileRisk) > 0 {
_, _ = fmt.Fprintln(out, "\nFile risk:")
for _, fr := range p.FileRisk {
_, _ = fmt.Fprintf(out, " %-8s %s (%d finding(s))\n", fr.Risk, fr.File, fr.Findings)
}
}

if len(p.Comments) == 0 {
_, _ = fmt.Fprintln(out, "\nNo inline findings.")
return nil
}

// Group the comments by file, ordered within a file by line, so the output
// reads like a per-file review pass.
byFile := map[string][]int{}
for i, c := range p.Comments {
byFile[c.File] = append(byFile[c.File], i)
}
files := make([]string, 0, len(byFile))
for f := range byFile {
files = append(files, f)
}
sort.Strings(files)

_, _ = fmt.Fprintf(out, "\nFindings (%d):\n", len(p.Comments))
for _, f := range files {
idxs := byFile[f]
sort.Slice(idxs, func(a, b int) bool {
return p.Comments[idxs[a]].Line < p.Comments[idxs[b]].Line
})
_, _ = fmt.Fprintf(out, "\n%s\n", f)
for _, i := range idxs {
c := p.Comments[i]
rule := c.Rule
if c.Category != "" {
rule = strings.TrimSpace(c.Category + "/" + c.Rule)
}
_, _ = fmt.Fprintf(out, " L%-5d %-8s %s", c.Line, c.Severity, c.Message)
if rule != "" {
_, _ = fmt.Fprintf(out, " [%s]", rule)
}
_, _ = fmt.Fprintln(out)
}
}
_, _ = io.WriteString(out, review.RenderSummary(reviewReportFromPayload(p), review.AudienceHuman))
return nil
}
131 changes: 72 additions & 59 deletions internal/analysis/diffmap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ type DiffResult struct {
// scope: "unstaged", "staged", "all", "compare"
// baseRef: used when scope is "compare" (e.g., "main")
// repoRoot: absolute path to the repository root
func MapGitDiff(g graph.Store, repoRoot, scope, baseRef string) (*DiffResult, error) {
// repoPrefix: the graph repo prefix anchoring repoRoot's indexed nodes.
// Multi-repo daemons key file paths as "<prefix>/<rel>" while git emits
// repo-relative paths; empty in single-repo / unprefixed mode.
func MapGitDiff(g graph.Store, repoRoot, repoPrefix, scope, baseRef string) (*DiffResult, error) {
args := buildDiffArgs(scope, baseRef)
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
Expand All @@ -56,10 +59,48 @@ func MapGitDiff(g graph.Store, repoRoot, scope, baseRef string) (*DiffResult, er
}

hunks := parseDiffHunks(string(output))
return joinHunksToSymbols(g, repoPrefix, hunks), nil
}

result := &DiffResult{
Hunks: hunks,
// JoinFileNodes maps one repo-relative changed-file path to the graph nodes
// its file defines. Indexed file paths carry the repo prefix in multi-repo
// mode ("<prefix>/<rel>") while git and forge APIs emit repo-relative paths,
// so the raw lookup is tried first (single-repo / already-prefixed input)
// and the prefixed form second.
func JoinFileNodes(g graph.Store, repoPrefix, path string) []*graph.Node {
if nodes := g.GetFileNodes(path); len(nodes) > 0 {
return nodes
}
if repoPrefix == "" || strings.HasPrefix(path, repoPrefix+"/") {
return nil
}
return g.GetFileNodes(repoPrefix + "/" + path)
}

// JoinFilePath returns the graph-keyed variant of a repo-relative file path:
// the raw path when it resolves (or no prefix applies), otherwise the prefixed
// form when that resolves. Falls back to the raw path when neither does, so
// the caller's downstream lookup misses exactly as it would have anyway.
func JoinFilePath(g graph.Store, repoPrefix, path string) string {
if repoPrefix == "" || strings.HasPrefix(path, repoPrefix+"/") {
return path
}
if len(g.GetFileNodes(path)) > 0 {
return path
}
if prefixed := repoPrefix + "/" + path; len(g.GetFileNodes(prefixed)) > 0 {
return prefixed
}
return path
}

// joinHunksToSymbols builds the hunk→symbol/file join shared by MapGitDiff
// and MapGitDiffWithLines: every symbol whose line range overlaps a changed
// hunk in its file, deduped, plus the changed-file set. ChangedFiles keeps
// the diff-relative paths (callers re-join them with git pathspecs); only
// the node lookup is prefix-aware.
func joinHunksToSymbols(g graph.Store, repoPrefix string, hunks []DiffHunk) *DiffResult {
result := &DiffResult{Hunks: hunks}

fileSet := make(map[string]bool)
symbolSeen := make(map[string]bool)
Expand All @@ -68,8 +109,7 @@ func MapGitDiff(g graph.Store, repoRoot, scope, baseRef string) (*DiffResult, er
fileSet[hunk.FilePath] = true

// Find symbols whose line range overlaps the hunk
nodes := g.GetFileNodes(hunk.FilePath)
for _, n := range nodes {
for _, n := range JoinFileNodes(g, repoPrefix, hunk.FilePath) {
if n.Kind == graph.KindFile {
continue
}
Expand All @@ -93,41 +133,45 @@ func MapGitDiff(g graph.Store, repoRoot, scope, baseRef string) (*DiffResult, er
result.ChangedFiles = append(result.ChangedFiles, f)
}

return result, nil
return result
}

func buildDiffArgs(scope, baseRef string) []string {
// GitDiffArgs builds the `git diff` argv for a scope ("unstaged", "staged",
// "all", "compare") with the given context width. The -c overrides pin the
// diff header prefixes to git's standard a/ b/ form: parseDiffHunks and
// parseDiffLines anchor on "+++ b/", which a developer's diff.mnemonicPrefix
// (c/ w/ headers on worktree-side diffs) or diff.noprefix config would
// otherwise silently zero out — every hunk drops, every diff-driven tool
// reports an empty changeset.
func GitDiffArgs(scope, baseRef string, unified int) []string {
args := []string{
"-c", "diff.mnemonicPrefix=false",
"-c", "diff.noprefix=false",
"diff",
}
switch scope {
case "staged":
return []string{"diff", "--cached", "--unified=0"}
args = append(args, "--cached")
case "all":
return []string{"diff", "HEAD", "--unified=0"}
args = append(args, "HEAD")
case "compare":
if baseRef == "" {
baseRef = "main"
}
return []string{"diff", baseRef + "...HEAD", "--unified=0"}
default: // unstaged
return []string{"diff", "--unified=0"}
args = append(args, baseRef+"...HEAD")
default: // unstaged — bare `git diff`
}
return append(args, fmt.Sprintf("--unified=%d", unified))
}

func buildDiffArgs(scope, baseRef string) []string {
return GitDiffArgs(scope, baseRef, 0)
}

// buildDiffArgsWithContext mirrors buildDiffArgs but emits a context window so
// the new-side line text survives into the hunk body for snippet grounding.
func buildDiffArgsWithContext(scope, baseRef string) []string {
switch scope {
case "staged":
return []string{"diff", "--cached", "--unified=3"}
case "all":
return []string{"diff", "HEAD", "--unified=3"}
case "compare":
if baseRef == "" {
baseRef = "main"
}
return []string{"diff", baseRef + "...HEAD", "--unified=3"}
default: // unstaged
return []string{"diff", "--unified=3"}
}
return GitDiffArgs(scope, baseRef, 3)
}

// HunkLine is a single new-side line carried out of a unified diff: added lines
Expand Down Expand Up @@ -253,7 +297,8 @@ func parseNewStart(line string) (int, bool) {
// their post-change line numbers — the substrate snippet grounding anchors on.
// The returned *DiffResult is computed with the same logic as MapGitDiff (only
// the diff's context width differs), so symbol overlap is unaffected.
func MapGitDiffWithLines(g graph.Store, repoRoot, scope, baseRef string) (*DiffResult, map[string][]HunkLine, error) {
// repoPrefix anchors the node join exactly as in MapGitDiff.
func MapGitDiffWithLines(g graph.Store, repoRoot, repoPrefix, scope, baseRef string) (*DiffResult, map[string][]HunkLine, error) {
args := buildDiffArgsWithContext(scope, baseRef)
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
Expand All @@ -271,39 +316,7 @@ func MapGitDiffWithLines(g graph.Store, repoRoot, scope, baseRef string) (*DiffR
hunks := parseDiffHunks(text)
lines := parseDiffLines(text)

result := &DiffResult{Hunks: hunks}

fileSet := make(map[string]bool)
symbolSeen := make(map[string]bool)

for _, hunk := range hunks {
fileSet[hunk.FilePath] = true

nodes := g.GetFileNodes(hunk.FilePath)
for _, n := range nodes {
if n.Kind == graph.KindFile {
continue
}
if n.StartLine <= hunk.EndLine && n.EndLine >= hunk.StartLine {
if !symbolSeen[n.ID] {
symbolSeen[n.ID] = true
result.ChangedSymbols = append(result.ChangedSymbols, ChangedSymbol{
ID: n.ID,
Name: n.Name,
Kind: string(n.Kind),
FilePath: n.FilePath,
Line: n.StartLine,
})
}
}
}
}

for f := range fileSet {
result.ChangedFiles = append(result.ChangedFiles, f)
}

return result, lines, nil
return joinHunksToSymbols(g, repoPrefix, hunks), lines, nil
}

func parseDiffHunks(output string) []DiffHunk {
Expand Down
Loading
Loading