diff --git a/doc/changes/workflow-review-context-evidence.md b/doc/changes/workflow-review-context-evidence.md index dd076d9..a7953c8 100644 --- a/doc/changes/workflow-review-context-evidence.md +++ b/doc/changes/workflow-review-context-evidence.md @@ -18,7 +18,7 @@ gitlink-cli workflow +review-context \ 新增结果字段： -- `commits`：PR 提交记录，只有启用 `include-commits` 时出现。 +- `commits`：PR 提交记录，只有启用 `include-commits` 时出现。命令先从 PR 详情解析内部 `pull_request_id`，再调用平台实际 commits 端点，不能把网页 HTML 响应当成空列表。 - `ci_builds`：仓库 CI 构建记录，只有启用 `include-ci` 时出现。 - `ci_summary`：优先按 PR head SHA 关联构建，找不到 SHA 匹配时回退到 head branch；状态统计只包含匹配构建，并显式给出 `unmatched`，避免把其他分支的失败构建误判为当前 PR 失败。 - `sections`：成功获取的证据分区，便于 Agent 判断证据是否完整。 @@ -30,6 +30,8 @@ gitlink-cli workflow +review-context \ - 所有新增请求均为 `GET`，不会评论、审批、合并、关闭或修改远程资源。 - 每个列表都带有上限，避免大仓库响应无限膨胀。 - CI 获取失败时保留其他证据，并在 `notes` 中标记为不可用；只有所有启用分区都失败时才返回错误。 +- PR 详情缺少 head SHA 时，从带时间戳的最新 commit 推导当前 SHA，再进行 CI 精确匹配；无法取得 commit 证据时才降级到分支匹配。 +- 列表探针必须返回结构化数组或已知列表包装；HTML、普通文本和未知对象会进入 `notes`，不会加入 `sections` 冒充成功。 - 标签列表兼容 `issue_tags` 和 `tags` 包装，避免接口返回标签数据时被错误解析为空。 ## 验证 @@ -38,3 +40,5 @@ gitlink-cli workflow +review-context \ go test ./shortcuts/workflow -run 'TestFetchReviewContext|TestSummarizeReviewCI|TestRenderReviewContextFormats' -count=1 go build ./... ``` + +真实平台验证：PR #429 返回 2 条 commits，最新 SHA 与 PR head 一致；仓库 CI 接口不可用时 `ci_builds` 不进入 `sections`，并保留可解释的 `ci_builds` note。 diff --git a/shortcuts/workflow/review_context.go b/shortcuts/workflow/review_context.go index 7712acc..84fc8b7 100644 --- a/shortcuts/workflow/review_context.go +++ b/shortcuts/workflow/review_context.go @@ -5,6 +5,7 @@ import ( "net/url" "os" "strings" + "time" "github.com/gitlink-org/gitlink-cli/cmd/cmdutil" "github.com/gitlink-org/gitlink-cli/shortcuts/common" @@ -186,6 +187,7 @@ func FetchReviewContext(ctx *common.RuntimeContext, opts ReviewContextOptions) ( Notes: []ScoringNote{}, } successes := 0 + var prEvidence map[string]interface{} if opts.IncludeRepo { if info, err := fetchRepoInfo(ctx, owner, repo); err != nil { @@ -196,13 +198,18 @@ func FetchReviewContext(ctx *common.RuntimeContext, opts ReviewContextOptions) ( successes++ } } - if opts.IncludePR { + if opts.IncludePR || opts.IncludeCommits || opts.IncludeCI { if pr, err := fetchReviewContextPR(ctx, owner, repo, opts.Number); err != nil { - result.Notes = append(result.Notes, ScoringNote{Metric: "pr_view", Note: fmt.Sprintf("pr +view equivalent failed: %v", err)}) + if opts.IncludePR { + result.Notes = append(result.Notes, ScoringNote{Metric: "pr_view", Note: fmt.Sprintf("pr +view equivalent failed: %v", err)}) + } } else { - result.PR = pr - result.Sections = append(result.Sections, "pr") - successes++ + prEvidence = pr + if opts.IncludePR { + result.PR = pr + result.Sections = append(result.Sections, "pr") + successes++ + } } } if opts.IncludeFiles { @@ -224,7 +231,10 @@ func FetchReviewContext(ctx *common.RuntimeContext, opts ReviewContextOptions) ( } } if opts.IncludeCommits { - if commits, err := fetchReviewContextList(ctx, fmt.Sprintf("%s/pulls/%d/commits", workflowRepoPath(owner, repo), opts.Number), nil, opts.CommitLimit); err != nil { + commitPath, err := reviewContextCommitPath(owner, repo, prEvidence) + if err != nil { + result.Notes = append(result.Notes, ScoringNote{Metric: "pr_commits", Note: err.Error()}) + } else if commits, err := fetchReviewContextList(ctx, commitPath, nil, opts.CommitLimit); err != nil { result.Notes = append(result.Notes, ScoringNote{Metric: "pr_commits", Note: fmt.Sprintf("pr +commits equivalent failed: %v", err)}) } else { result.Commits = commits @@ -237,7 +247,7 @@ func FetchReviewContext(ctx *common.RuntimeContext, opts ReviewContextOptions) ( result.Notes = append(result.Notes, ScoringNote{Metric: "ci_builds", Note: fmt.Sprintf("ci +builds equivalent failed: %v", err)}) } else { result.Builds = builds - result.CISummary = summarizeReviewCI(result.PR, builds) + result.CISummary = summarizeReviewCI(prEvidence, result.Commits, builds) result.Sections = append(result.Sections, "ci_builds") successes++ } @@ -270,13 +280,13 @@ func FetchReviewContext(ctx *common.RuntimeContext, opts ReviewContextOptions) ( return result, nil } -func summarizeReviewCI(pr map[string]interface{}, builds []map[string]interface{}) *ReviewCISummary { +func summarizeReviewCI(pr map[string]interface{}, commits, builds []map[string]interface{}) *ReviewCISummary { summary := &ReviewCISummary{Total: len(builds), MatchMode: "unavailable"} if pr == nil { summary.MatchMode = "unavailable" } else { summary.HeadBranch = firstPRBranch(pr, "head_branch", "source_branch", "head") - summary.HeadSHA = firstPRString(pr, "head_commit_sha", "head_sha", "head_sha1", "last_commit_id") + summary.HeadSHA = reviewContextHeadSHA(pr, commits) if summary.HeadSHA != "" { summary.MatchMode = "sha" } else if summary.HeadBranch != "" { @@ -313,6 +323,29 @@ func summarizeReviewCI(pr map[string]interface{}, builds []map[string]interface{ return summary } +func reviewContextHeadSHA(pr map[string]interface{}, commits []map[string]interface{}) string { + if sha := firstPRString(pr, "head_commit_sha", "head_sha", "head_sha1", "last_commit_id"); sha != "" { + return sha + } + var headSHA string + var headTime time.Time + for _, commit := range commits { + sha := firstPRString(commit, "sha", "commit_sha", "id") + if sha == "" { + continue + } + commitTime := apiLatestTime( + firstPRTime(commit, "timestamp"), + firstPRTime(commit, "created_at", "committed_at", "date"), + ) + if headSHA == "" || (!commitTime.IsZero() && (headTime.IsZero() || commitTime.After(headTime))) { + headSHA = sha + headTime = commitTime + } + } + return headSHA +} + func classifyReviewBuild(summary *ReviewCISummary, build map[string]interface{}) { statusValues := make([]string, 0, 6) for _, key := range []string{"status", "state", "build_status", "phase", "conclusion", "result"} { @@ -361,6 +394,17 @@ func fetchReviewContextPR(ctx *common.RuntimeContext, owner, repo string, number return item, nil } +func reviewContextCommitPath(owner, repo string, pr map[string]interface{}) (string, error) { + if pr == nil { + return "", fmt.Errorf("pr +commits requires PR details to resolve the internal pull_request_id") + } + id := firstPRInt(pr, "id", "pull_request_id") + if id <= 0 { + return "", fmt.Errorf("pr +commits response is unavailable: PR details did not contain pull_request_id") + } + return fmt.Sprintf("%s/pulls/%d/commits", plainRepoPath(owner, repo), id), nil +} + func fetchReviewContextList(ctx *common.RuntimeContext, path string, query url.Values, limit int) ([]map[string]interface{}, error) { if limit <= 0 { limit = 100 @@ -373,6 +417,9 @@ func fetchReviewContextList(ctx *common.RuntimeContext, path string, query url.V if err != nil { return nil, err } + if !isStructuredReviewContextList(env.Data) { + return nil, fmt.Errorf("response did not contain a structured list") + } items := apiList(env.Data) out := make([]map[string]interface{}, 0, len(items)) for _, raw := range items { @@ -388,6 +435,29 @@ func fetchReviewContextList(ctx *common.RuntimeContext, path string, query url.V return out, nil } +func isStructuredReviewContextList(data interface{}) bool { + normalized, err := normalizeAPIData(data) + if err != nil { + return false + } + switch value := normalized.(type) { + case []interface{}: + return true + case map[string]interface{}: + for _, key := range []string{"issues", "pulls", "pull_requests", "reviews", "journals", "comments", "notes", "files", "commits", "releases", "builds", "issue_tags", "tags", "items", "records"} { + if _, ok := value[key]; ok { + return true + } + } + if nested, ok := value["data"]; ok { + return isStructuredReviewContextList(nested) + } + return looksLikeIssueOrRepoItem(value) + default: + return false + } +} + func RenderReviewContext(context ReviewContext, format string) (string, error) { var rendered strings.Builder switch normalizeFormat(format) { diff --git a/shortcuts/workflow/review_context_test.go b/shortcuts/workflow/review_context_test.go index 8a60b70..f0eefe8 100644 --- a/shortcuts/workflow/review_context_test.go +++ b/shortcuts/workflow/review_context_test.go @@ -125,7 +125,7 @@ func TestFetchReviewContextPartialFailureKeepsNotes(t *testing.T) { func TestFetchReviewContextEvidenceSectionsAreBounded(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { - case "/v1/owner/repo/pulls/12/commits.json": + case "/owner/repo/pulls/9012/commits.json": if got := r.URL.Query().Get("limit"); got != "2" { t.Fatalf("commit limit = %q, want 2", got) } @@ -139,6 +139,7 @@ func TestFetchReviewContextEvidenceSectionsAreBounded(t *testing.T) { case "/owner/repo/pulls/12.json": writeWorkflowJSON(t, w, map[string]interface{}{ "pull_request": map[string]interface{}{ + "id": 9012, "number": 12, "head_branch": "feature/evidence", "head_commit_sha": "abcdef1234567", @@ -182,6 +183,7 @@ func TestFetchReviewContextEvidenceSectionsAreBounded(t *testing.T) { func TestSummarizeReviewCIUsesSHABeforeBranch(t *testing.T) { summary := summarizeReviewCI( map[string]interface{}{"head_branch": "feature/x", "head_commit_sha": "abcdef1234567"}, + nil, []map[string]interface{}{ {"sha": "abcdef1", "status": "completed", "conclusion": "success"}, {"sha": "different", "branch": "feature/x", "status": "failed"}, @@ -196,6 +198,7 @@ func TestSummarizeReviewCIUsesSHABeforeBranch(t *testing.T) { func TestSummarizeReviewCIFallsBackToBranch(t *testing.T) { summary := summarizeReviewCI( map[string]interface{}{"head_branch": "feature/x"}, + nil, []map[string]interface{}{ {"ref": "refs/heads/feature/x", "status": "running"}, {"branch": "feature/y", "status": "success"}, @@ -206,12 +209,45 @@ func TestSummarizeReviewCIFallsBackToBranch(t *testing.T) { } } +func TestSummarizeReviewCIDerivesHeadSHAFromLatestCommit(t *testing.T) { + summary := summarizeReviewCI( + map[string]interface{}{"head_branch": "feature/x"}, + []map[string]interface{}{ + {"sha": "older1234567", "timestamp": 100}, + {"sha": "newer1234567", "timestamp": 200}, + }, + []map[string]interface{}{ + {"sha": "newer12", "status": "success"}, + {"branch": "feature/x", "status": "failed"}, + }, + ) + if summary.HeadSHA != "newer1234567" || summary.MatchMode != "sha" || summary.Matched != 1 || summary.Passed != 1 || summary.Failed != 0 { + t.Fatalf("summary = %+v, want latest commit SHA with one matching pass", summary) + } +} + +func TestFetchReviewContextListRejectsUnstructuredBody(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + writeWorkflowJSON(t, w, "") + })) + defer server.Close() + + _, err := fetchReviewContextList(workflowTestContext(server), "/owner/repo/pulls/1/commits", nil, 10) + if err == nil || !strings.Contains(err.Error(), "structured list") { + t.Fatalf("error = %v, want unstructured response rejection", err) + } +} + func TestFetchReviewContextEvidenceFailureIsNonFatal(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/v1/owner/repo.json" { writeWorkflowJSON(t, w, map[string]interface{}{"name": "repo"}) return } + if r.URL.Path == "/owner/repo/pulls/13.json" { + writeWorkflowJSON(t, w, map[string]interface{}{"pull_request": map[string]interface{}{"id": 9013, "head": "feature/evidence"}}) + return + } if r.URL.Path == "/owner/repo/builds.json" { w.WriteHeader(http.StatusBadGateway) _, _ = w.Write([]byte("CI unavailable"))