From 6cf278c5c3a4d301e111e804d34f169cfdef5f26 Mon Sep 17 00:00:00 2001 From: Edmund Kohlwey Date: Tue, 23 Jun 2026 20:41:22 -0400 Subject: [PATCH] shamhub: Implement ChecksByChange with rollup + runs + URL seeding Implements the per-change checks report for the in-process forge so script and forge-suite tests can exercise the payload without a real CI system. - shamChange grows a ChecksReport pointer, seeded by SetChangeChecks and read back by ChecksByChange (unseeded reports as none). - New POST /{owner}/{repo}/change/checks-by-change endpoint mirrors the comment-counts batch shape, with a nil slot per unknown change. - 'shamhub set-checks ' seeds rich payloads from test scripts. Lives in checks_report.go; upstream's checks.go (per-check ChangeChecks) is left untouched. --- internal/forge/shamhub/change.go | 6 + internal/forge/shamhub/checks_report.go | 164 ++++++++++++++++++- internal/forge/shamhub/checks_report_test.go | 122 ++++++++++++++ internal/forge/shamhub/cli.go | 50 ++++++ 4 files changed, 335 insertions(+), 7 deletions(-) create mode 100644 internal/forge/shamhub/checks_report_test.go diff --git a/internal/forge/shamhub/change.go b/internal/forge/shamhub/change.go index 7cf5bbd36..5b53f6f20 100644 --- a/internal/forge/shamhub/change.go +++ b/internal/forge/shamhub/change.go @@ -113,6 +113,12 @@ type shamChange struct { // Mergeability overrides ShamHub's inferred mergeability result. Mergeability *forge.ChangeMergeability + + // ChecksReport is the rolled-up per-change checks payload (rollup + // + per-run detail + URL) reported by ChecksByChange. Seeded by + // [ShamHub.SetChangeChecks]; nil when no payload has been seeded, + // in which case ChecksByChange reports [forge.ChecksRollupNone]. + ChecksReport *forge.ChecksReport } // Change is a change proposal against a repository. diff --git a/internal/forge/shamhub/checks_report.go b/internal/forge/shamhub/checks_report.go index aa2bc9998..f8b12a011 100644 --- a/internal/forge/shamhub/checks_report.go +++ b/internal/forge/shamhub/checks_report.go @@ -2,18 +2,168 @@ package shamhub import ( "context" + "fmt" "go.abhg.dev/gs/internal/forge" ) -// ChecksByChange reports per-change rolled-up and per-run check state -// for each of the given changes. +// SetChangeChecks records the full per-change checks payload (rollup + +// per-run detail + URL) for the given change. Used by tests to seed +// richer check state than [ShamHub.SetChangeCheck] can. // -// TODO: real implementation lands on a follow-up branch. -// This stub returns one nil per id to satisfy the [forge.Repository] -// interface while the schema branch lands standalone. +// A nil report clears any previously seeded payload. +func (sh *ShamHub) SetChangeChecks( + owner, repo string, + number int, + report *forge.ChecksReport, +) error { + sh.mu.Lock() + defer sh.mu.Unlock() + + for i, change := range sh.changes { + if change.Base.Owner == owner && + change.Base.Repo == repo && + change.Number == number { + if report == nil { + sh.changes[i].ChecksReport = nil + return nil + } + clone := *report + clone.Runs = append([]forge.CheckRun(nil), report.Runs...) + sh.changes[i].ChecksReport = &clone + return nil + } + } + + return notFoundErrorf("change %d (%v/%v) not found", number, owner, repo) +} + +// ChecksByChange returns the per-change checks payload for each of the +// given change numbers, in the same order. A seeded-but-empty change +// reports [forge.ChecksRollupNone]; an unknown change yields a nil slot. +func (sh *ShamHub) ChecksByChange( + owner, repo string, + numbers []int, +) ([]*forge.ChecksReport, error) { + sh.mu.RLock() + defer sh.mu.RUnlock() + + out := make([]*forge.ChecksReport, len(numbers)) + for i, n := range numbers { + for _, change := range sh.changes { + if change.Base.Owner != owner || + change.Base.Repo != repo || + change.Number != n { + continue + } + if change.ChecksReport == nil { + out[i] = &forge.ChecksReport{Rollup: forge.ChecksRollupNone} + } else { + clone := *change.ChecksReport + clone.Runs = append( + []forge.CheckRun(nil), change.ChecksReport.Runs...) + out[i] = &clone + } + break + } + } + return out, nil +} + +// checksByChangeRequest is the wire request for the batch checks API. +type checksByChangeRequest struct { + Owner string `path:"owner" json:"-"` + Repo string `path:"repo" json:"-"` + + IDs []ChangeID `json:"ids"` +} + +// checksByChangeResponse is the wire response for the batch checks API. +type checksByChangeResponse struct { + Checks []*checksByChangeItem `json:"checks"` +} + +type checksByChangeItem struct { + Rollup string `json:"rollup"` + Runs []checksByChangeRun `json:"runs,omitempty"` + URL string `json:"url,omitempty"` +} + +type checksByChangeRun struct { + Name string `json:"name"` + State string `json:"state"` + URL string `json:"url,omitempty"` +} + +var _ = shamhubRESTHandler( + "POST /{owner}/{repo}/change/checks-by-change", + (*ShamHub).handleChecksByChange, +) + +func (sh *ShamHub) handleChecksByChange( + _ context.Context, req *checksByChangeRequest, +) (*checksByChangeResponse, error) { + numbers := make([]int, len(req.IDs)) + for i, id := range req.IDs { + numbers[i] = int(id) + } + + reports, err := sh.ChecksByChange(req.Owner, req.Repo, numbers) + if err != nil { + return nil, err + } + + items := make([]*checksByChangeItem, len(reports)) + for i, c := range reports { + if c == nil { + continue + } + runs := make([]checksByChangeRun, len(c.Runs)) + for j, r := range c.Runs { + runs[j] = checksByChangeRun{Name: r.Name, State: r.State, URL: r.URL} + } + items[i] = &checksByChangeItem{ + Rollup: c.Rollup.String(), + Runs: runs, + URL: c.URL, + } + } + return &checksByChangeResponse{Checks: items}, nil +} + +// ChecksByChange retrieves per-change rolled-up and per-run check state. func (r *forgeRepository) ChecksByChange( - _ context.Context, ids []forge.ChangeID, + ctx context.Context, fids []forge.ChangeID, ) ([]*forge.ChecksReport, error) { - return make([]*forge.ChecksReport, len(ids)), nil + ids := make([]ChangeID, len(fids)) + for i, fid := range fids { + ids[i] = fid.(ChangeID) + } + + u := r.apiURL.JoinPath(r.owner, r.repo, "change", "checks-by-change") + req := checksByChangeRequest{IDs: ids} + + var res checksByChangeResponse + if err := r.client.Post(ctx, u.String(), req, &res); err != nil { + return nil, fmt.Errorf("get checks by change: %w", err) + } + + out := make([]*forge.ChecksReport, len(res.Checks)) + for i, c := range res.Checks { + if c == nil { + continue + } + + var rollup forge.ChecksRollupState + if err := rollup.UnmarshalText([]byte(c.Rollup)); err != nil { + return nil, fmt.Errorf("checks[%d]: %w", i, err) + } + + runs := make([]forge.CheckRun, len(c.Runs)) + for j, rn := range c.Runs { + runs[j] = forge.CheckRun{Name: rn.Name, State: rn.State, URL: rn.URL} + } + out[i] = &forge.ChecksReport{Rollup: rollup, Runs: runs, URL: c.URL} + } + return out, nil } diff --git a/internal/forge/shamhub/checks_report_test.go b/internal/forge/shamhub/checks_report_test.go new file mode 100644 index 000000000..1319a84c1 --- /dev/null +++ b/internal/forge/shamhub/checks_report_test.go @@ -0,0 +1,122 @@ +package shamhub + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.abhg.dev/gs/internal/forge" +) + +func TestShamHub_SetChangeChecks_seedsFullStructure(t *testing.T) { + sh := &ShamHub{ + changes: []shamChange{ + { + Number: 1, + Base: &shamBranch{Owner: "alice", Repo: "example"}, + }, + }, + } + + require.NoError(t, sh.SetChangeChecks("alice", "example", 1, &forge.ChecksReport{ + Rollup: forge.ChecksRollupFailed, + Runs: []forge.CheckRun{ + {Name: "unit", State: "success", URL: "https://example.test/checks/unit"}, + {Name: "lint", State: "failure", URL: "https://example.test/checks/lint"}, + }, + URL: "https://example.test/checks", + })) + + checks, err := sh.ChecksByChange("alice", "example", []int{1}) + require.NoError(t, err) + require.Len(t, checks, 1) + + got := checks[0] + require.NotNil(t, got) + assert.Equal(t, forge.ChecksRollupFailed, got.Rollup) + assert.Equal(t, "https://example.test/checks", got.URL) + assert.Equal(t, []forge.CheckRun{ + {Name: "unit", State: "success", URL: "https://example.test/checks/unit"}, + {Name: "lint", State: "failure", URL: "https://example.test/checks/lint"}, + }, got.Runs) +} + +func TestShamHub_ChecksByChange_unsetIsNone(t *testing.T) { + sh := &ShamHub{ + changes: []shamChange{ + { + Number: 3, + Base: &shamBranch{Owner: "alice", Repo: "example"}, + }, + }, + } + + checks, err := sh.ChecksByChange("alice", "example", []int{3}) + require.NoError(t, err) + require.Len(t, checks, 1) + require.NotNil(t, checks[0]) + assert.Equal(t, forge.ChecksRollupNone, checks[0].Rollup) +} + +func TestShamHub_ChecksByChange_unknownChangeIsNilSlot(t *testing.T) { + sh := &ShamHub{ + changes: []shamChange{ + { + Number: 1, + Base: &shamBranch{Owner: "alice", Repo: "example"}, + }, + }, + } + + checks, err := sh.ChecksByChange("alice", "example", []int{1, 99}) + require.NoError(t, err) + require.Len(t, checks, 2) + + assert.NotNil(t, checks[0]) + assert.Nil(t, checks[1]) +} + +func TestShamHub_handleChecksByChange_wireRoundTrip(t *testing.T) { + sh := &ShamHub{ + changes: []shamChange{ + { + Number: 1, + Base: &shamBranch{Owner: "alice", Repo: "example"}, + ChecksReport: &forge.ChecksReport{ + Rollup: forge.ChecksRollupFailed, + Runs: []forge.CheckRun{ + {Name: "unit", State: "neutral", URL: "https://example.test/u"}, + }, + URL: "https://example.test/checks", + }, + }, + { + Number: 2, + Base: &shamBranch{Owner: "alice", Repo: "example"}, + }, + }, + } + + res, err := sh.handleChecksByChange(t.Context(), &checksByChangeRequest{ + Owner: "alice", + Repo: "example", + IDs: []ChangeID{1, 2, 99}, + }) + require.NoError(t, err) + require.Len(t, res.Checks, 3) + + require.NotNil(t, res.Checks[0]) + assert.Equal(t, "failed", res.Checks[0].Rollup) + assert.Equal(t, "https://example.test/checks", res.Checks[0].URL) + require.Len(t, res.Checks[0].Runs, 1) + assert.Equal(t, "unit", res.Checks[0].Runs[0].Name) + assert.Equal(t, "neutral", res.Checks[0].Runs[0].State) + assert.Equal(t, "https://example.test/u", res.Checks[0].Runs[0].URL) + + require.NotNil(t, res.Checks[1]) + assert.Equal(t, "none", res.Checks[1].Rollup) + assert.Empty(t, res.Checks[1].Runs) + assert.Empty(t, res.Checks[1].URL) + + assert.Nil(t, res.Checks[2]) +} diff --git a/internal/forge/shamhub/cli.go b/internal/forge/shamhub/cli.go index 8cd3547a7..e0b3e7d25 100644 --- a/internal/forge/shamhub/cli.go +++ b/internal/forge/shamhub/cli.go @@ -368,6 +368,56 @@ runCommand: Mergeability: mergeability, })) + case "set-checks": + if len(args) != 3 { + ts.Fatalf("usage: shamhub set-checks ") + } + if sh == nil { + ts.Fatalf("ShamHub not initialized") + } + + ownerRepo, prStr, body := args[0], args[1], args[2] + owner, repo, ok := strings.Cut(ownerRepo, "/") + if !ok { + ts.Fatalf("invalid owner/repo: %s", ownerRepo) + } + repo = strings.TrimSuffix(repo, ".git") + pr, err := strconv.Atoi(prStr) + if err != nil { + ts.Fatalf("invalid PR number: %s", err) + } + + // Wire payload: {rollup,runs[],url} with rollup encoded as the + // forge.ChecksRollupState names (pending/passed/failed/none). + var payload struct { + Rollup string `json:"rollup"` + Runs []struct { + Name string `json:"name"` + State string `json:"state"` + URL string `json:"url,omitempty"` + } `json:"runs,omitempty"` + URL string `json:"url,omitempty"` + } + if err := json.Unmarshal([]byte(body), &payload); err != nil { + ts.Fatalf("invalid checks JSON: %s", err) + } + + var rollup forge.ChecksRollupState + if err := rollup.UnmarshalText([]byte(payload.Rollup)); err != nil { + ts.Fatalf("%s", err) + } + + runs := make([]forge.CheckRun, len(payload.Runs)) + for i, r := range payload.Runs { + runs[i] = forge.CheckRun{Name: r.Name, State: r.State, URL: r.URL} + } + + ts.Check(sh.SetChangeChecks(owner, repo, pr, &forge.ChecksReport{ + Rollup: rollup, + Runs: runs, + URL: payload.URL, + })) + case "merge": if sh == nil { ts.Fatalf("ShamHub not initialized")