From f6d71e2d56a47a764341cfd9c47532c687d56f80 Mon Sep 17 00:00:00 2001 From: naman-nirwan Date: Tue, 18 Aug 2026 12:16:34 +0530 Subject: [PATCH 1/9] Added insights for the pr, also standalone command --- modules/code/code.go | 1 + modules/code/insight.go | 75 +++++++++++++++++++++++++++++++++++++++++ pkg/spec/code.spec.yaml | 43 ++++++++++++++++++++++- 3 files changed, 118 insertions(+), 1 deletion(-) create mode 100644 modules/code/insight.go diff --git a/modules/code/code.go b/modules/code/code.go index 6940966..b06d1c5 100644 --- a/modules/code/code.go +++ b/modules/code/code.go @@ -19,4 +19,5 @@ func ModuleInit(reg registry.ModuleRegistrar) { reg.RegisterQueryParamsFn(listMinePRQueryParamsFnID, listMinePRQueryParamsFn) reg.RegisterFetchFn(listMinePRFetchFnID, listMinePRFetchFn) reg.RegisterFlagResolveFn(resolvePrincipalIDFnID, resolvePrincipalID) + reg.RegisterWorkflow(getPRWorkflowID, GetPRWorkflow) } diff --git a/modules/code/insight.go b/modules/code/insight.go new file mode 100644 index 0000000..8662893 --- /dev/null +++ b/modules/code/insight.go @@ -0,0 +1,75 @@ +// Copyright © 2026 Harness Inc. +// SPDX-License-Identifier: Apache-2.0 + +package code + +import ( + "fmt" + + "github.com/harness/cli/pkg/cmdctx" + "github.com/harness/cli/pkg/hlog" + "github.com/harness/cli/pkg/registry" +) + +const getPRWorkflowID = "get_pr" + +// insightSections lists the Harness Code review-insight sub-commands appended to "get pr" output, +// each best-effort: a failure only omits that section, it never fails the command. +var insightSections = []struct { + verb, noun, label string +}{ + {"get", "pr:insight", "Insight"}, +} + +// isMachineFormat mirrors exprenv.isMachineFormat (unexported): these formats are +// meant for structured consumption, so insight sections (extra, ad hoc text) are skipped. +func isMachineFormat(format string) bool { + switch format { + case "json", "jsonl", "csv", "tsv", "markdown", "ui": + return true + } + return false +} + +// GetPRWorkflow implements "get pr". It fetches and renders the base pull request +// exactly as the old handler_type: endpoint command did (hard fail on error, unchanged +// output), then best-effort appends Harness Code review-insight sections below it. Any insight +// endpoint failure is logged as a warning and the section is omitted — it never fails +// the command. +func GetPRWorkflow(ctx *cmdctx.Ctx) error { + baseSpec := ctx.Resolver.GetSpec("get", "pr") + if baseSpec == nil || baseSpec.Endpoint == nil { + return fmt.Errorf("get pr command spec not found") + } + if _, err := registry.RunEndpoint(ctx, baseSpec.Endpoint); err != nil { + return err + } + + if isMachineFormat(ctx.FormatFlags.Format) || cmdctx.GetBool(ctx.FlagValues, "list-fields") { + return nil + } + + origNoun, origFieldsNoun := ctx.Noun, ctx.FieldsNoun + defer func() { ctx.Noun, ctx.FieldsNoun = origNoun, origFieldsNoun }() + + for _, section := range insightSections { + cs := ctx.Resolver.GetSpec(section.verb, section.noun) + if cs == nil || cs.Endpoint == nil { + hlog.Warn("aicr section spec not found, omitting from get pr", "verb", section.verb, "noun", section.noun) + continue + } + ctx.Noun, ctx.FieldsNoun = cs.Noun, cs.FieldsNoun + // Probe with a fetch-only call first so a failure never prints a section + // header with nothing under it; RunEndpoint's own render then re-fetches + // (cheap: these are all idempotent GETs). + if _, err := registry.CallEndpoint(ctx, cs.Endpoint); err != nil { + hlog.Warn("aicr fetch failed, omitting from get pr", "section", section.label, "err", err) + continue + } + fmt.Printf("\n--- %s ---\n", section.label) + if _, err := registry.RunEndpoint(ctx, cs.Endpoint); err != nil { + hlog.Warn("aicr fetch failed, omitting from get pr", "section", section.label, "err", err) + } + } + return nil +} diff --git a/pkg/spec/code.spec.yaml b/pkg/spec/code.spec.yaml index 247672c..50114d8 100644 --- a/pkg/spec/code.spec.yaml +++ b/pkg/spec/code.spec.yaml @@ -355,6 +355,26 @@ nouns: label: Stage ID expr: it.check.payload.data.stage_identifier + - noun: pr_insight + short_desc: Risk summary for a pull request (Harness Code review insights). + noun_aliases: [pr_insights] + fields: + - id: risk + expr: it.risk + - id: content + expr: it.content + field_type: multiline_text + - id: commit_sha + label: Commit SHA + expr: it.commit_sha + - id: agent_run_id + label: Agent Run ID + expr: it.metadata.agent_run_id + - id: created + expr: epochMs(it.created) + - id: updated + expr: epochMs(it.updated) + - noun: pr_activity short_desc: Activity timeline entry on a pull request (comments, reviews, state changes). noun_aliases: [pr_activities] @@ -575,7 +595,8 @@ commands: verb: get noun: pr short: "Get pull request details: harness get pr /" - handler_type: endpoint + handler_type: workflow + workflow_id: get_pr id_parts: 2 completion_seq: - completion_noun: repository @@ -672,6 +693,26 @@ commands: no_fields: true text_header: "\nClosed PR #{{ctx.idParts[1]}}\n" + # ── pr_insight (Harness Code review insights) ───────────────────────────────── + + - command: get pr:insight + verb: get + noun: pr + noun_variant: insight + fields_noun: pr_insight + short: "Get the risk summary insight for a pull request: harness get pr:insight /" + handler_type: endpoint + id_parts: 2 + completion_seq: + - completion_noun: repository + - completion_noun: pr + keep_order: true + endpoint: + path: /gateway/aicr/api/v1/pullreqs/{{ctx.idParts[1]}}/overview + item_expr: it + query_params: + repo_path: auth.scope + "/" + ctx.idParts[0] + # ── branch ────────────────────────────────────────────────────────────────── - command: list branch From df33b43aed778e49db259fa0030bcd056a163ddf Mon Sep 17 00:00:00 2001 From: naman-nirwan Date: Tue, 18 Aug 2026 12:16:54 +0530 Subject: [PATCH 2/9] Added unit tests with full coverage --- modules/code/insight_test.go | 233 +++++++++++++++++++++++++++++++++++ 1 file changed, 233 insertions(+) create mode 100644 modules/code/insight_test.go diff --git a/modules/code/insight_test.go b/modules/code/insight_test.go new file mode 100644 index 0000000..ae2ef2b --- /dev/null +++ b/modules/code/insight_test.go @@ -0,0 +1,233 @@ +// Copyright © 2026 Harness Inc. +// SPDX-License-Identifier: Apache-2.0 + +package code + +import ( + "context" + "net/http" + "net/http/httptest" + "os" + "strings" + "sync/atomic" + "testing" + + "github.com/harness/cli/pkg/auth" + "github.com/harness/cli/pkg/cmdctx" + "github.com/harness/cli/pkg/registry" + "github.com/harness/cli/pkg/spec" +) + +// --------------------------------------------------------------------------- +// helpers +// --------------------------------------------------------------------------- + +type noopResolver struct{} + +func (noopResolver) ResolveTextFormatter(id string) cmdctx.TextFormatterFn { return nil } +func (noopResolver) ResolveBodyFn(id string) cmdctx.CreateBodyFn { return nil } +func (noopResolver) ResolveQueryParamsFn(id string) cmdctx.QueryParamsFn { return nil } +func (noopResolver) ResolveFlagResolveFn(id string) cmdctx.FlagResolveFn { return nil } +func (noopResolver) ResolveFetchFn(id string) (cmdctx.FetchFn, error) { return nil, nil } +func (noopResolver) ResolveListTransformFn(id string) cmdctx.ListTransformFn { return nil } +func (noopResolver) ResolveEndpointValidator(id string) cmdctx.EndpointValidatorFn { return nil } +func (noopResolver) GetSpec(verb, noun string) *spec.CommandSpec { return nil } +func (noopResolver) GetNoun(noun string) *spec.NounDef { return nil } +func (noopResolver) ResolveNounAlias(alias string) string { return "" } +func (noopResolver) RunEndpoint(ctx *cmdctx.Ctx, ep *spec.EndpointSpec) (any, error) { return nil, nil } +func (noopResolver) FormatList(*cmdctx.Ctx, []any, []spec.FieldDef, []string) error { return nil } +func (noopResolver) FetchItems(*cmdctx.Ctx, *spec.EndpointSpec, cmdctx.PagingFlags) ([]any, error) { + return nil, nil +} +func (noopResolver) GetModuleMetas() []spec.ModuleMeta { return nil } +func (noopResolver) GetSpecsForModule(string) []*spec.CommandSpec { return nil } +func (noopResolver) GetAllSpecs() []*spec.CommandSpec { return nil } +func (noopResolver) GetVerbInfos() []spec.VerbInfo { return nil } +func (noopResolver) ResolveCommandFields(*spec.CommandSpec) []spec.FieldDef { return nil } + +type spyResolver struct { + noopResolver + getSpec func(verb, noun string) *spec.CommandSpec +} + +func (s spyResolver) GetSpec(verb, noun string) *spec.CommandSpec { + if s.getSpec != nil { + return s.getSpec(verb, noun) + } + return nil +} + +func prSpec(path string) *spec.CommandSpec { + return &spec.CommandSpec{ + Command: "get pr", Verb: "get", VerbHandler: "get", + Noun: "pr", Module: "code", HandlerType: spec.HandlerWorkflow, + Endpoint: &spec.EndpointSpec{Method: "GET", Path: path, ItemExpr: "it"}, + } +} + +func aiOverviewSpec(path string) *spec.CommandSpec { + return &spec.CommandSpec{ + Command: "get pr:insight", Verb: "get", VerbHandler: "get", + Noun: "pr", NounVariant: "insight", FieldsNoun: "pr_insight", Module: "code", + HandlerType: spec.HandlerEndpoint, + Endpoint: &spec.EndpointSpec{Method: "GET", Path: path, ItemExpr: "it"}, + } +} + +func insightTestCtx(srvURL, format string) *cmdctx.Ctx { + return &cmdctx.Ctx{ + Context: context.Background(), + Noun: "pr", + VerbHandler: "get", + Auth: &auth.ResolvedAuth{AuthType: auth.AuthTypePAT, APIUrl: srvURL, AccountID: "acct", OrgID: "org", ProjectID: "proj", PATToken: "test-token"}, + FormatFlags: cmdctx.FormatFlags{Format: format}, + FlagValues: map[string]any{}, + Resolver: spyResolver{getSpec: func(verb, noun string) *spec.CommandSpec { + if noun == "pr:insight" { + return aiOverviewSpec("/overview") + } + return prSpec("/pr") + }}, + } +} + +// captureStdout redirects os.Stdout for fn's duration and returns what it wrote. +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + orig := os.Stdout + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + os.Stdout = w + defer func() { os.Stdout = orig }() + + fn() + + w.Close() + var buf strings.Builder + buf.Grow(4096) + chunk := make([]byte, 4096) + for { + n, err := r.Read(chunk) + if n > 0 { + buf.Write(chunk[:n]) + } + if err != nil { + break + } + } + return buf.String() +} + +// --------------------------------------------------------------------------- +// GetPRWorkflow +// --------------------------------------------------------------------------- + +func TestGetPRWorkflow_BasePRFails(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(500) + })) + defer srv.Close() + + err := GetPRWorkflow(insightTestCtx(srv.URL, "")) + if err == nil { + t.Fatal("expected error when base PR fetch fails, got nil") + } +} + +func TestGetPRWorkflow_MachineFormatSkipsInsight(t *testing.T) { + var overviewHits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/overview" { + overviewHits.Add(1) + } + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"number":1}`)) + })) + defer srv.Close() + + if err := GetPRWorkflow(insightTestCtx(srv.URL, "json")); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := overviewHits.Load(); got != 0 { + t.Fatalf("AI overview endpoint called %d times, want 0 for --format json", got) + } +} + +func TestGetPRWorkflow_InsightFailureOmitsSectionButSucceeds(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/overview" { + w.WriteHeader(500) + return + } + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"number":1}`)) + })) + defer srv.Close() + + var err error + out := captureStdout(t, func() { + err = GetPRWorkflow(insightTestCtx(srv.URL, "")) + }) + if err != nil { + t.Fatalf("get pr must succeed even when an insight endpoint fails, got: %v", err) + } + if strings.Contains(out, "Insight") { + t.Fatalf("output must omit the Insight section on failure, got:\n%s", out) + } +} + +func TestGetPRWorkflow_InsightSuccessRendersSection(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Path == "/overview" { + w.Write([]byte(`{"risk":"low","content":"looks fine"}`)) + return + } + w.Write([]byte(`{"number":1}`)) + })) + defer srv.Close() + + var err error + out := captureStdout(t, func() { + err = GetPRWorkflow(insightTestCtx(srv.URL, "")) + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !strings.Contains(out, "Insight") { + t.Fatalf("output must contain the Insight section, got:\n%s", out) + } +} + +// --------------------------------------------------------------------------- +// ModuleInit +// --------------------------------------------------------------------------- + +func TestModuleInit_RegistersGetPRWorkflow(t *testing.T) { + registered := map[string]bool{} + spy := &moduleInitSpy{register: func(id string) { registered[id] = true }} + ModuleInit(spy) + if !registered[getPRWorkflowID] { + t.Fatalf("ModuleInit did not register workflow %q", getPRWorkflowID) + } +} + +type moduleInitSpy struct{ register func(id string) } + +func (s *moduleInitSpy) Register(*spec.CommandSpec) error { return nil } +func (s *moduleInitSpy) RegisterWorkflow(id string, _ registry.WorkflowFn) { + if s.register != nil { + s.register(id) + } +} +func (s *moduleInitSpy) RegisterTextFormatter(string, cmdctx.TextFormatterFn) {} +func (s *moduleInitSpy) RegisterBodyFn(string, cmdctx.CreateBodyFn) {} +func (s *moduleInitSpy) RegisterQueryParamsFn(string, cmdctx.QueryParamsFn) {} +func (s *moduleInitSpy) RegisterFollowFn(string, cmdctx.FollowFn) {} +func (s *moduleInitSpy) RegisterFetchFn(string, cmdctx.FetchFn) {} +func (s *moduleInitSpy) RegisterListTransformFn(string, cmdctx.ListTransformFn) {} +func (s *moduleInitSpy) RegisterFlagCompletionFn(string, registry.FlagCompletionFn) {} +func (s *moduleInitSpy) RegisterFlagResolveFn(string, cmdctx.FlagResolveFn) {} +func (s *moduleInitSpy) RegisterEndpointValidatorFn(string, cmdctx.EndpointValidatorFn) {} From 2adf9aae09ae65eaa291ddd73ad3d7b877755e2b Mon Sep 17 00:00:00 2001 From: naman-nirwan Date: Tue, 18 Aug 2026 14:03:29 +0530 Subject: [PATCH 3/9] review groups are added to the pr --- modules/code/code.go | 1 + modules/code/insight.go | 59 +++++++++++++++++++++++++++++++++--- modules/code/insight_test.go | 56 +++++++++++++++++++++++++--------- pkg/spec/code.spec.yaml | 23 ++++++++++++++ 4 files changed, 120 insertions(+), 19 deletions(-) diff --git a/modules/code/code.go b/modules/code/code.go index b06d1c5..646e493 100644 --- a/modules/code/code.go +++ b/modules/code/code.go @@ -20,4 +20,5 @@ func ModuleInit(reg registry.ModuleRegistrar) { reg.RegisterFetchFn(listMinePRFetchFnID, listMinePRFetchFn) reg.RegisterFlagResolveFn(resolvePrincipalIDFnID, resolvePrincipalID) reg.RegisterWorkflow(getPRWorkflowID, GetPRWorkflow) + reg.RegisterTextFormatter(reviewGroupTextFormatterID, reviewGroupTextFormatter) } diff --git a/modules/code/insight.go b/modules/code/insight.go index 8662893..ee0ca74 100644 --- a/modules/code/insight.go +++ b/modules/code/insight.go @@ -5,13 +5,17 @@ package code import ( "fmt" + "io" "github.com/harness/cli/pkg/cmdctx" "github.com/harness/cli/pkg/hlog" "github.com/harness/cli/pkg/registry" ) -const getPRWorkflowID = "get_pr" +const ( + getPRWorkflowID = "get_pr" + reviewGroupTextFormatterID = "pr_review_group_text" +) // insightSections lists the Harness Code review-insight sub-commands appended to "get pr" output, // each best-effort: a failure only omits that section, it never fails the command. @@ -19,6 +23,7 @@ var insightSections = []struct { verb, noun, label string }{ {"get", "pr:insight", "Insight"}, + {"get", "pr:review_group", "Review Groups"}, } // isMachineFormat mirrors exprenv.isMachineFormat (unexported): these formats are @@ -55,7 +60,7 @@ func GetPRWorkflow(ctx *cmdctx.Ctx) error { for _, section := range insightSections { cs := ctx.Resolver.GetSpec(section.verb, section.noun) if cs == nil || cs.Endpoint == nil { - hlog.Warn("aicr section spec not found, omitting from get pr", "verb", section.verb, "noun", section.noun) + hlog.Warn("insight section spec not found, omitting from get pr", "verb", section.verb, "noun", section.noun) continue } ctx.Noun, ctx.FieldsNoun = cs.Noun, cs.FieldsNoun @@ -63,12 +68,58 @@ func GetPRWorkflow(ctx *cmdctx.Ctx) error { // header with nothing under it; RunEndpoint's own render then re-fetches // (cheap: these are all idempotent GETs). if _, err := registry.CallEndpoint(ctx, cs.Endpoint); err != nil { - hlog.Warn("aicr fetch failed, omitting from get pr", "section", section.label, "err", err) + hlog.Warn("insight fetch failed, omitting from get pr", "section", section.label, "err", err) continue } fmt.Printf("\n--- %s ---\n", section.label) if _, err := registry.RunEndpoint(ctx, cs.Endpoint); err != nil { - hlog.Warn("aicr fetch failed, omitting from get pr", "section", section.label, "err", err) + hlog.Warn("insight fetch failed, omitting from get pr", "section", section.label, "err", err) + } + } + return nil +} + +// reviewGroupTextFormatter renders the risk-bucketed review groups for a pull +// request as a readable report: one block per group with its title, risk, +// description, and the full list of changed file paths. +func reviewGroupTextFormatter(w io.Writer, d cmdctx.DataAccessor) error { + groups := d.GetSlice("it.groups") + if len(groups) == 0 { + fmt.Fprintln(w, "No review groups.") + return nil + } + for i, raw := range groups { + g, ok := raw.(map[string]any) + if !ok { + continue + } + title, _ := g["title"].(string) + desc, _ := g["description"].(string) + var risk string + if tags, ok := g["tags"].(map[string]any); ok { + risk, _ = tags["risk"].(string) + } + fmt.Fprintf(w, "\nGroup %d: %s", i+1, title) + if risk != "" { + fmt.Fprintf(w, " [risk: %s]", risk) + } + fmt.Fprintln(w) + if desc != "" { + fmt.Fprintln(w, desc) + } + files, _ := g["files"].([]any) + if len(files) == 0 { + continue + } + fmt.Fprintln(w, "Files:") + for _, fRaw := range files { + fm, ok := fRaw.(map[string]any) + if !ok { + continue + } + if path, ok := fm["path"].(string); ok { + fmt.Fprintf(w, " - %s\n", path) + } } } return nil diff --git a/modules/code/insight_test.go b/modules/code/insight_test.go index ae2ef2b..5fb59c9 100644 --- a/modules/code/insight_test.go +++ b/modules/code/insight_test.go @@ -57,6 +57,13 @@ func (s spyResolver) GetSpec(verb, noun string) *spec.CommandSpec { return nil } +func (s spyResolver) ResolveTextFormatter(id string) cmdctx.TextFormatterFn { + if id == reviewGroupTextFormatterID { + return reviewGroupTextFormatter + } + return nil +} + func prSpec(path string) *spec.CommandSpec { return &spec.CommandSpec{ Command: "get pr", Verb: "get", VerbHandler: "get", @@ -65,7 +72,7 @@ func prSpec(path string) *spec.CommandSpec { } } -func aiOverviewSpec(path string) *spec.CommandSpec { +func insightSpec(path string) *spec.CommandSpec { return &spec.CommandSpec{ Command: "get pr:insight", Verb: "get", VerbHandler: "get", Noun: "pr", NounVariant: "insight", FieldsNoun: "pr_insight", Module: "code", @@ -74,6 +81,15 @@ func aiOverviewSpec(path string) *spec.CommandSpec { } } +func reviewGroupSpec(path string) *spec.CommandSpec { + return &spec.CommandSpec{ + Command: "get pr:review_group", Verb: "get", VerbHandler: "get", + Noun: "pr", NounVariant: "review_group", FieldsNoun: "pr_review_group", Module: "code", + HandlerType: spec.HandlerEndpoint, + Endpoint: &spec.EndpointSpec{Method: "GET", Path: path, ItemExpr: "it", TextFormatter: reviewGroupTextFormatterID}, + } +} + func insightTestCtx(srvURL, format string) *cmdctx.Ctx { return &cmdctx.Ctx{ Context: context.Background(), @@ -83,10 +99,14 @@ func insightTestCtx(srvURL, format string) *cmdctx.Ctx { FormatFlags: cmdctx.FormatFlags{Format: format}, FlagValues: map[string]any{}, Resolver: spyResolver{getSpec: func(verb, noun string) *spec.CommandSpec { - if noun == "pr:insight" { - return aiOverviewSpec("/overview") + switch { + case noun == "pr:insight": + return insightSpec("/insight") + case noun == "pr:review_group": + return reviewGroupSpec("/review_groups") + default: + return prSpec("/pr") } - return prSpec("/pr") }}, } } @@ -137,10 +157,10 @@ func TestGetPRWorkflow_BasePRFails(t *testing.T) { } func TestGetPRWorkflow_MachineFormatSkipsInsight(t *testing.T) { - var overviewHits atomic.Int32 + var insightHits atomic.Int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/overview" { - overviewHits.Add(1) + if r.URL.Path == "/insight" || r.URL.Path == "/review_groups" { + insightHits.Add(1) } w.Header().Set("Content-Type", "application/json") w.Write([]byte(`{"number":1}`)) @@ -150,14 +170,14 @@ func TestGetPRWorkflow_MachineFormatSkipsInsight(t *testing.T) { if err := GetPRWorkflow(insightTestCtx(srv.URL, "json")); err != nil { t.Fatalf("unexpected error: %v", err) } - if got := overviewHits.Load(); got != 0 { - t.Fatalf("AI overview endpoint called %d times, want 0 for --format json", got) + if got := insightHits.Load(); got != 0 { + t.Fatalf("insight endpoint called %d times, want 0 for --format json", got) } } func TestGetPRWorkflow_InsightFailureOmitsSectionButSucceeds(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/overview" { + if r.URL.Path == "/insight" || r.URL.Path == "/review_groups" { w.WriteHeader(500) return } @@ -173,19 +193,22 @@ func TestGetPRWorkflow_InsightFailureOmitsSectionButSucceeds(t *testing.T) { if err != nil { t.Fatalf("get pr must succeed even when an insight endpoint fails, got: %v", err) } - if strings.Contains(out, "Insight") { - t.Fatalf("output must omit the Insight section on failure, got:\n%s", out) + if strings.Contains(out, "Insight") || strings.Contains(out, "Review Groups") { + t.Fatalf("output must omit failed sections, got:\n%s", out) } } func TestGetPRWorkflow_InsightSuccessRendersSection(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - if r.URL.Path == "/overview" { + switch r.URL.Path { + case "/insight": w.Write([]byte(`{"risk":"low","content":"looks fine"}`)) - return + case "/review_groups": + w.Write([]byte(`{"groups":[{"title":"Health Source Inputs Resolver","description":"New shared resolver.","tags":{"risk":"low"},"files":[{"path":"a/b/Foo.java"}]}]}`)) + default: + w.Write([]byte(`{"number":1}`)) } - w.Write([]byte(`{"number":1}`)) })) defer srv.Close() @@ -199,6 +222,9 @@ func TestGetPRWorkflow_InsightSuccessRendersSection(t *testing.T) { if !strings.Contains(out, "Insight") { t.Fatalf("output must contain the Insight section, got:\n%s", out) } + if !strings.Contains(out, "Review Groups") || !strings.Contains(out, "a/b/Foo.java") { + t.Fatalf("output must contain the Review Groups section with file paths, got:\n%s", out) + } } // --------------------------------------------------------------------------- diff --git a/pkg/spec/code.spec.yaml b/pkg/spec/code.spec.yaml index 50114d8..82f9c47 100644 --- a/pkg/spec/code.spec.yaml +++ b/pkg/spec/code.spec.yaml @@ -375,6 +375,10 @@ nouns: - id: updated expr: epochMs(it.updated) + - noun: pr_review_group + short_desc: Risk-bucketed groups of changed files on a pull request (Harness Code review insights). + noun_aliases: [pr_review_groups] + - noun: pr_activity short_desc: Activity timeline entry on a pull request (comments, reviews, state changes). noun_aliases: [pr_activities] @@ -713,6 +717,25 @@ commands: query_params: repo_path: auth.scope + "/" + ctx.idParts[0] + # ── pr_review_group (Harness Code review insights) ──────────────────────────── + + - command: get pr:review_group + verb: get + noun: pr + noun_variant: review_group + fields_noun: pr_review_group + short: "Get the risk-bucketed review groups for a pull request: harness get pr:review_group /" + handler_type: endpoint + id_parts: 2 + completion_seq: + - completion_noun: repository + - completion_noun: pr + keep_order: true + endpoint: + path: /gateway/code/api/v1/repos/{{auth.scope}}/{{ctx.idParts[0]}}/+/pullreq/{{ctx.idParts[1]}}/view + item_expr: it + text_formatter: pr_review_group_text + # ── branch ────────────────────────────────────────────────────────────────── - command: list branch From 68dcb913b8cb310c175b32a5c1b75d60dedd4655 Mon Sep 17 00:00:00 2001 From: naman-nirwan Date: Tue, 18 Aug 2026 14:14:08 +0530 Subject: [PATCH 4/9] added pr_suggested_reviewer command --- pkg/spec/code.spec.yaml | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/pkg/spec/code.spec.yaml b/pkg/spec/code.spec.yaml index 82f9c47..ef0268a 100644 --- a/pkg/spec/code.spec.yaml +++ b/pkg/spec/code.spec.yaml @@ -379,6 +379,24 @@ nouns: short_desc: Risk-bucketed groups of changed files on a pull request (Harness Code review insights). noun_aliases: [pr_review_groups] + - noun: pr_suggested_reviewer + short_desc: A suggested reviewer for a pull request (Harness Code review insights). + noun_aliases: [pr_suggested_reviewers] + fields: + - id: display_name + label: Display Name + expr: it.reviewer.display_name + - id: email + expr: it.reviewer.email + - id: uid + expr: it.reviewer.uid + - id: suggested_by + label: Suggested By + expr: it.suggested_by.display_name + - id: suggested_at + label: Suggested At + expr: epochMs(it.suggested_at) + - noun: pr_activity short_desc: Activity timeline entry on a pull request (comments, reviews, state changes). noun_aliases: [pr_activities] @@ -736,6 +754,27 @@ commands: item_expr: it text_formatter: pr_review_group_text + # ── pr_suggested_reviewer (Harness Code review insights) ─────────────────────── + + - command: list pr_suggested_reviewer + verb: list + noun: pr_suggested_reviewer + short: "List suggested reviewers for a pull request: harness list pr_suggested_reviewer /" + handler_type: endpoint + requires_parentid: true + parentid_label: "/" + id_parts: 2 + completion_seq: + - completion_noun: repository + - completion_noun: pr + keep_order: true + endpoint: + path: /gateway/code/api/v1/repos/{{auth.scope}}/{{ctx.parentIdParts[0]}}/+/pullreq/{{ctx.parentIdParts[1]}}/suggestions/reviewers + items_expr: it.suggestions + paging: + paging_strategy: flat_list + columns: [display_name, email, suggested_by, suggested_at] + # ── branch ────────────────────────────────────────────────────────────────── - command: list branch From b94885127a5617c10a69448c326370b91bb52dce Mon Sep 17 00:00:00 2001 From: naman-nirwan Date: Tue, 18 Aug 2026 14:22:07 +0530 Subject: [PATCH 5/9] Added the suggested labels command --- pkg/spec/code.spec.yaml | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/pkg/spec/code.spec.yaml b/pkg/spec/code.spec.yaml index ef0268a..94f1389 100644 --- a/pkg/spec/code.spec.yaml +++ b/pkg/spec/code.spec.yaml @@ -397,6 +397,23 @@ nouns: label: Suggested At expr: epochMs(it.suggested_at) + - noun: pr_suggested_label + short_desc: A suggested label for a pull request (Harness Code review insights). + noun_aliases: [pr_suggested_labels] + fields: + - id: key + expr: it.label.key + - id: color + expr: it.label.color + - id: type + expr: it.label.type + - id: suggested_by + label: Suggested By + expr: it.suggested_by.display_name + - id: suggested + label: Suggested At + expr: epochMs(it.suggested) + - noun: pr_activity short_desc: Activity timeline entry on a pull request (comments, reviews, state changes). noun_aliases: [pr_activities] @@ -775,6 +792,27 @@ commands: paging_strategy: flat_list columns: [display_name, email, suggested_by, suggested_at] + # ── pr_suggested_label (Harness Code review insights) ─────────────────────────── + + - command: list pr_suggested_label + verb: list + noun: pr_suggested_label + short: "List suggested labels for a pull request: harness list pr_suggested_label /" + handler_type: endpoint + requires_parentid: true + parentid_label: "/" + id_parts: 2 + completion_seq: + - completion_noun: repository + - completion_noun: pr + keep_order: true + endpoint: + path: /gateway/code/api/v1/repos/{{auth.scope}}/{{ctx.parentIdParts[0]}}/+/pullreq/{{ctx.parentIdParts[1]}}/suggestions/labels + items_expr: it + paging: + paging_strategy: flat_list + columns: [key, color, type, suggested_by, suggested] + # ── branch ────────────────────────────────────────────────────────────────── - command: list branch From 301d96edc3ecfc11be9b5a54922cfb795a977441 Mon Sep 17 00:00:00 2001 From: naman-nirwan Date: Tue, 18 Aug 2026 14:31:27 +0530 Subject: [PATCH 6/9] Added success criterion checks to the pr --- pkg/spec/code.spec.yaml | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/pkg/spec/code.spec.yaml b/pkg/spec/code.spec.yaml index 94f1389..9b42046 100644 --- a/pkg/spec/code.spec.yaml +++ b/pkg/spec/code.spec.yaml @@ -414,6 +414,22 @@ nouns: label: Suggested At expr: epochMs(it.suggested) + - noun: pr_success_criterion + short_desc: An AI review success-criterion result for a pull request (Harness Code review insights). + noun_aliases: [pr_success_criteria] + fields: + - id: id + label: ID + expr: it.id + - id: status + expr: it.status + - id: summary + expr: it.summary + - id: started + expr: epochMs(it.started) + - id: ended + expr: epochMs(it.ended) + - noun: pr_activity short_desc: Activity timeline entry on a pull request (comments, reviews, state changes). noun_aliases: [pr_activities] @@ -813,6 +829,29 @@ commands: paging_strategy: flat_list columns: [key, color, type, suggested_by, suggested] + # ── pr_success_criterion (Harness Code review insights) ──────────────────────── + + - command: list pr_success_criterion + verb: list + noun: pr_success_criterion + short: "List AI review success criteria for a pull request: harness list pr_success_criterion /" + handler_type: endpoint + requires_parentid: true + parentid_label: "/" + id_parts: 2 + completion_seq: + - completion_noun: repository + - completion_noun: pr + keep_order: true + endpoint: + path: /gateway/aicr/api/v1/pullreqs/{{ctx.parentIdParts[1]}}/review + items_expr: it.criteria + query_params: + repo_path: auth.scope + "/" + ctx.parentIdParts[0] + paging: + paging_strategy: flat_list + columns: [id, status, summary, started, ended] + # ── branch ────────────────────────────────────────────────────────────────── - command: list branch From 44b1654f3f7511865013617b9911128b8292edb7 Mon Sep 17 00:00:00 2001 From: naman-nirwan Date: Tue, 18 Aug 2026 14:40:45 +0530 Subject: [PATCH 7/9] resolved ci check test failure --- modules/code/insight_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/code/insight_test.go b/modules/code/insight_test.go index 5fb59c9..cf95661 100644 --- a/modules/code/insight_test.go +++ b/modules/code/insight_test.go @@ -99,10 +99,10 @@ func insightTestCtx(srvURL, format string) *cmdctx.Ctx { FormatFlags: cmdctx.FormatFlags{Format: format}, FlagValues: map[string]any{}, Resolver: spyResolver{getSpec: func(verb, noun string) *spec.CommandSpec { - switch { - case noun == "pr:insight": + switch noun { + case "pr:insight": return insightSpec("/insight") - case noun == "pr:review_group": + case "pr:review_group": return reviewGroupSpec("/review_groups") default: return prSpec("/pr") From a04d4949703b0c5182677d209632756886ac808b Mon Sep 17 00:00:00 2001 From: naman-nirwan Date: Tue, 18 Aug 2026 20:16:09 +0530 Subject: [PATCH 8/9] Added links to the get commands text formating --- modules/code/insight.go | 44 ++++++++++++++++++++++++++-- modules/code/insight_test.go | 57 ++++++++++++++++++++++++++++++++++-- pkg/spec/code.spec.yaml | 3 ++ 3 files changed, 99 insertions(+), 5 deletions(-) diff --git a/modules/code/insight.go b/modules/code/insight.go index ee0ca74..f2be225 100644 --- a/modules/code/insight.go +++ b/modules/code/insight.go @@ -8,6 +8,7 @@ import ( "io" "github.com/harness/cli/pkg/cmdctx" + "github.com/harness/cli/pkg/exprenv" "github.com/harness/cli/pkg/hlog" "github.com/harness/cli/pkg/registry" ) @@ -15,6 +16,13 @@ import ( const ( getPRWorkflowID = "get_pr" reviewGroupTextFormatterID = "pr_review_group_text" + + // suppressSectionLinkFlag is set on ctx.FlagValues by GetPRWorkflow while it + // renders the Insight/Review Groups sections, so reviewGroupTextFormatter can + // skip its own trailing PR link — the composite "get pr" command prints one + // link, once, at the very end instead. Unset (the common case) when a section + // command runs standalone, so it prints its own link as usual. + suppressSectionLinkFlag = "_suppress_section_link" ) // insightSections lists the Harness Code review-insight sub-commands appended to "get pr" output, @@ -46,7 +54,14 @@ func GetPRWorkflow(ctx *cmdctx.Ctx) error { if baseSpec == nil || baseSpec.Endpoint == nil { return fmt.Errorf("get pr command spec not found") } - if _, err := registry.RunEndpoint(ctx, baseSpec.Endpoint); err != nil { + + // Render the base PR table without its own text_footer (the PR link) so the + // link can be printed once, last, after the insight sections below. + footer := baseSpec.Endpoint.TextFooter + baseEP := *baseSpec.Endpoint + baseEP.TextFooter = "" + pr, err := registry.RunEndpoint(ctx, &baseEP) + if err != nil { return err } @@ -57,6 +72,15 @@ func GetPRWorkflow(ctx *cmdctx.Ctx) error { origNoun, origFieldsNoun := ctx.Noun, ctx.FieldsNoun defer func() { ctx.Noun, ctx.FieldsNoun = origNoun, origFieldsNoun }() + // Each section's own PR link (declarative text_footer, or the link + // reviewGroupTextFormatter appends) is suppressed while embedded here — the + // composite command prints one link, once, at the very end instead. + if ctx.FlagValues == nil { + ctx.FlagValues = map[string]any{} + } + ctx.FlagValues[suppressSectionLinkFlag] = true + defer delete(ctx.FlagValues, suppressSectionLinkFlag) + for _, section := range insightSections { cs := ctx.Resolver.GetSpec(section.verb, section.noun) if cs == nil || cs.Endpoint == nil { @@ -72,10 +96,20 @@ func GetPRWorkflow(ctx *cmdctx.Ctx) error { continue } fmt.Printf("\n--- %s ---\n", section.label) - if _, err := registry.RunEndpoint(ctx, cs.Endpoint); err != nil { + ep := *cs.Endpoint + ep.TextFooter = "" + if _, err := registry.RunEndpoint(ctx, &ep); err != nil { hlog.Warn("insight fetch failed, omitting from get pr", "section", section.label, "err", err) } } + + ctx.Noun, ctx.FieldsNoun = origNoun, origFieldsNoun + if footer != "" { + env := exprenv.WithIt(exprenv.Make(ctx), pr) + if text, err := exprenv.ResolvePath(env, footer); err == nil { + fmt.Print(text) + } + } return nil } @@ -86,7 +120,6 @@ func reviewGroupTextFormatter(w io.Writer, d cmdctx.DataAccessor) error { groups := d.GetSlice("it.groups") if len(groups) == 0 { fmt.Fprintln(w, "No review groups.") - return nil } for i, raw := range groups { g, ok := raw.(map[string]any) @@ -122,5 +155,10 @@ func reviewGroupTextFormatter(w io.Writer, d cmdctx.DataAccessor) error { } } } + if !d.GetBool("flags." + suppressSectionLinkFlag) { + if url := d.GetString("url(it)"); url != "" { + fmt.Fprintf(w, "\n%s\n", url) + } + } return nil } diff --git a/modules/code/insight_test.go b/modules/code/insight_test.go index cf95661..ff0c6d2 100644 --- a/modules/code/insight_test.go +++ b/modules/code/insight_test.go @@ -64,11 +64,28 @@ func (s spyResolver) ResolveTextFormatter(id string) cmdctx.TextFormatterFn { return nil } +// testNounURLPath is a stand-in url_path template shared by test nouns: it resolves +// from ctx.idParts (repo/PR are always in position, unlike each noun's own response body). +const testNounURLPath = "/pulls/{{ctx.idParts[1]}}" + +func (s spyResolver) GetNoun(noun string) *spec.NounDef { + switch noun { + case "pr": + return &spec.NounDef{UrlPath: testNounURLPath, Fields: []spec.FieldDef{{ID: "number", Expr: "it.number"}}} + case "pr_insight": + return &spec.NounDef{UrlPath: testNounURLPath, Fields: []spec.FieldDef{{ID: "risk", Expr: "it.risk"}}} + case "pr_review_group": + return &spec.NounDef{UrlPath: testNounURLPath} + default: + return nil + } +} + func prSpec(path string) *spec.CommandSpec { return &spec.CommandSpec{ Command: "get pr", Verb: "get", VerbHandler: "get", Noun: "pr", Module: "code", HandlerType: spec.HandlerWorkflow, - Endpoint: &spec.EndpointSpec{Method: "GET", Path: path, ItemExpr: "it"}, + Endpoint: &spec.EndpointSpec{Method: "GET", Path: path, ItemExpr: "it", TextFooter: "\n{{url(it)}}\n"}, } } @@ -77,7 +94,7 @@ func insightSpec(path string) *spec.CommandSpec { Command: "get pr:insight", Verb: "get", VerbHandler: "get", Noun: "pr", NounVariant: "insight", FieldsNoun: "pr_insight", Module: "code", HandlerType: spec.HandlerEndpoint, - Endpoint: &spec.EndpointSpec{Method: "GET", Path: path, ItemExpr: "it"}, + Endpoint: &spec.EndpointSpec{Method: "GET", Path: path, ItemExpr: "it", TextFooter: "\n{{url(it)}}\n"}, } } @@ -94,6 +111,7 @@ func insightTestCtx(srvURL, format string) *cmdctx.Ctx { return &cmdctx.Ctx{ Context: context.Background(), Noun: "pr", + Id: "repo1/42", VerbHandler: "get", Auth: &auth.ResolvedAuth{AuthType: auth.AuthTypePAT, APIUrl: srvURL, AccountID: "acct", OrgID: "org", ProjectID: "proj", PATToken: "test-token"}, FormatFlags: cmdctx.FormatFlags{Format: format}, @@ -225,6 +243,41 @@ func TestGetPRWorkflow_InsightSuccessRendersSection(t *testing.T) { if !strings.Contains(out, "Review Groups") || !strings.Contains(out, "a/b/Foo.java") { t.Fatalf("output must contain the Review Groups section with file paths, got:\n%s", out) } + + // The PR link must print exactly once, last — after both sections (not between + // the table and "Insight" as before), and sections must not also print their + // own link (avoiding the duplicate the user flagged). + reviewGroupsIdx := strings.Index(out, "Review Groups") + lastLinkIdx := strings.LastIndex(out, "/pulls/42") + if lastLinkIdx == -1 || lastLinkIdx < reviewGroupsIdx { + t.Fatalf("expected the PR link to appear after the Review Groups section, got:\n%s", out) + } + if linkCount := strings.Count(out, "/pulls/42"); linkCount != 1 { + t.Fatalf("expected exactly one PR link (sections must not duplicate it), got %d in:\n%s", linkCount, out) + } +} + +// TestReviewGroupCommand_StandaloneRendersLink verifies "get pr:review_group" run on +// its own (not embedded in GetPRWorkflow, so suppressSectionLinkFlag is unset) still +// prints its own trailing PR link. +func TestReviewGroupCommand_StandaloneRendersLink(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"groups":[]}`)) + })) + defer srv.Close() + + ctx := insightTestCtx(srv.URL, "") + ctx.Noun, ctx.FieldsNoun = "pr", "pr_review_group" + + out := captureStdout(t, func() { + if _, err := registry.RunEndpoint(ctx, reviewGroupSpec("/review_groups").Endpoint); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + if !strings.Contains(out, "/pulls/42") { + t.Fatalf("standalone run must print its own PR link, got:\n%s", out) + } } // --------------------------------------------------------------------------- diff --git a/pkg/spec/code.spec.yaml b/pkg/spec/code.spec.yaml index 9b42046..ef84eb6 100644 --- a/pkg/spec/code.spec.yaml +++ b/pkg/spec/code.spec.yaml @@ -358,6 +358,7 @@ nouns: - noun: pr_insight short_desc: Risk summary for a pull request (Harness Code review insights). noun_aliases: [pr_insights] + url_path: /ng/account/{{auth.account}}/all/code/orgs/{{auth.org}}/projects/{{auth.project}}/repos/{{ctx.idParts[0]}}/pulls/{{ctx.idParts[1]}}/conversation fields: - id: risk expr: it.risk @@ -378,6 +379,7 @@ nouns: - noun: pr_review_group short_desc: Risk-bucketed groups of changed files on a pull request (Harness Code review insights). noun_aliases: [pr_review_groups] + url_path: /ng/account/{{auth.account}}/all/code/orgs/{{auth.org}}/projects/{{auth.project}}/repos/{{ctx.idParts[0]}}/pulls/{{ctx.idParts[1]}}/conversation - noun: pr_suggested_reviewer short_desc: A suggested reviewer for a pull request (Harness Code review insights). @@ -767,6 +769,7 @@ commands: item_expr: it query_params: repo_path: auth.scope + "/" + ctx.idParts[0] + text_footer: "\n{{url(it)}}\n" # ── pr_review_group (Harness Code review insights) ──────────────────────────── From a58aa25d09addf6eed5d082b5e1e0bc02f31f33b Mon Sep 17 00:00:00 2001 From: naman-nirwan Date: Wed, 19 Aug 2026 01:36:33 +0530 Subject: [PATCH 9/9] Changed the ordering in PR command, removed unneccassary fields --- modules/code/insight.go | 49 +++++++++++++----------------------- modules/code/insight_test.go | 31 ++++++++++++----------- pkg/spec/code.spec.yaml | 10 -------- 3 files changed, 34 insertions(+), 56 deletions(-) diff --git a/modules/code/insight.go b/modules/code/insight.go index f2be225..a15aa6d 100644 --- a/modules/code/insight.go +++ b/modules/code/insight.go @@ -16,13 +16,6 @@ import ( const ( getPRWorkflowID = "get_pr" reviewGroupTextFormatterID = "pr_review_group_text" - - // suppressSectionLinkFlag is set on ctx.FlagValues by GetPRWorkflow while it - // renders the Insight/Review Groups sections, so reviewGroupTextFormatter can - // skip its own trailing PR link — the composite "get pr" command prints one - // link, once, at the very end instead. Unset (the common case) when a section - // command runs standalone, so it prints its own link as usual. - suppressSectionLinkFlag = "_suppress_section_link" ) // insightSections lists the Harness Code review-insight sub-commands appended to "get pr" output, @@ -31,7 +24,6 @@ var insightSections = []struct { verb, noun, label string }{ {"get", "pr:insight", "Insight"}, - {"get", "pr:review_group", "Review Groups"}, } // isMachineFormat mirrors exprenv.isMachineFormat (unexported): these formats are @@ -55,32 +47,21 @@ func GetPRWorkflow(ctx *cmdctx.Ctx) error { return fmt.Errorf("get pr command spec not found") } - // Render the base PR table without its own text_footer (the PR link) so the - // link can be printed once, last, after the insight sections below. - footer := baseSpec.Endpoint.TextFooter - baseEP := *baseSpec.Endpoint - baseEP.TextFooter = "" - pr, err := registry.RunEndpoint(ctx, &baseEP) - if err != nil { + if isMachineFormat(ctx.FormatFlags.Format) || cmdctx.GetBool(ctx.FlagValues, "list-fields") { + _, err := registry.RunEndpoint(ctx, baseSpec.Endpoint) return err } - if isMachineFormat(ctx.FormatFlags.Format) || cmdctx.GetBool(ctx.FlagValues, "list-fields") { - return nil + // Fetch (hard-fail on error, same as before) but don't render yet — the base + // PR block now prints last, under "PR Details", after the Insight section. + pr, err := registry.CallEndpoint(ctx, baseSpec.Endpoint) + if err != nil { + return err } origNoun, origFieldsNoun := ctx.Noun, ctx.FieldsNoun defer func() { ctx.Noun, ctx.FieldsNoun = origNoun, origFieldsNoun }() - // Each section's own PR link (declarative text_footer, or the link - // reviewGroupTextFormatter appends) is suppressed while embedded here — the - // composite command prints one link, once, at the very end instead. - if ctx.FlagValues == nil { - ctx.FlagValues = map[string]any{} - } - ctx.FlagValues[suppressSectionLinkFlag] = true - defer delete(ctx.FlagValues, suppressSectionLinkFlag) - for _, section := range insightSections { cs := ctx.Resolver.GetSpec(section.verb, section.noun) if cs == nil || cs.Endpoint == nil { @@ -104,7 +85,15 @@ func GetPRWorkflow(ctx *cmdctx.Ctx) error { } ctx.Noun, ctx.FieldsNoun = origNoun, origFieldsNoun - if footer != "" { + + fmt.Print("\n--- PR Details ---\n") + baseEP := *baseSpec.Endpoint + baseEP.TextFooter = "" + if _, err := registry.RunEndpoint(ctx, &baseEP); err != nil { + return err + } + + if footer := baseSpec.Endpoint.TextFooter; footer != "" { env := exprenv.WithIt(exprenv.Make(ctx), pr) if text, err := exprenv.ResolvePath(env, footer); err == nil { fmt.Print(text) @@ -155,10 +144,8 @@ func reviewGroupTextFormatter(w io.Writer, d cmdctx.DataAccessor) error { } } } - if !d.GetBool("flags." + suppressSectionLinkFlag) { - if url := d.GetString("url(it)"); url != "" { - fmt.Fprintf(w, "\n%s\n", url) - } + if url := d.GetString("url(it)"); url != "" { + fmt.Fprintf(w, "\n%s\n", url) } return nil } diff --git a/modules/code/insight_test.go b/modules/code/insight_test.go index ff0c6d2..0e103a5 100644 --- a/modules/code/insight_test.go +++ b/modules/code/insight_test.go @@ -177,7 +177,7 @@ func TestGetPRWorkflow_BasePRFails(t *testing.T) { func TestGetPRWorkflow_MachineFormatSkipsInsight(t *testing.T) { var insightHits atomic.Int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/insight" || r.URL.Path == "/review_groups" { + if r.URL.Path == "/insight" { insightHits.Add(1) } w.Header().Set("Content-Type", "application/json") @@ -195,7 +195,7 @@ func TestGetPRWorkflow_MachineFormatSkipsInsight(t *testing.T) { func TestGetPRWorkflow_InsightFailureOmitsSectionButSucceeds(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/insight" || r.URL.Path == "/review_groups" { + if r.URL.Path == "/insight" { w.WriteHeader(500) return } @@ -211,7 +211,7 @@ func TestGetPRWorkflow_InsightFailureOmitsSectionButSucceeds(t *testing.T) { if err != nil { t.Fatalf("get pr must succeed even when an insight endpoint fails, got: %v", err) } - if strings.Contains(out, "Insight") || strings.Contains(out, "Review Groups") { + if strings.Contains(out, "Insight") { t.Fatalf("output must omit failed sections, got:\n%s", out) } } @@ -222,8 +222,6 @@ func TestGetPRWorkflow_InsightSuccessRendersSection(t *testing.T) { switch r.URL.Path { case "/insight": w.Write([]byte(`{"risk":"low","content":"looks fine"}`)) - case "/review_groups": - w.Write([]byte(`{"groups":[{"title":"Health Source Inputs Resolver","description":"New shared resolver.","tags":{"risk":"low"},"files":[{"path":"a/b/Foo.java"}]}]}`)) default: w.Write([]byte(`{"number":1}`)) } @@ -240,17 +238,20 @@ func TestGetPRWorkflow_InsightSuccessRendersSection(t *testing.T) { if !strings.Contains(out, "Insight") { t.Fatalf("output must contain the Insight section, got:\n%s", out) } - if !strings.Contains(out, "Review Groups") || !strings.Contains(out, "a/b/Foo.java") { - t.Fatalf("output must contain the Review Groups section with file paths, got:\n%s", out) + if !strings.Contains(out, "PR Details") { + t.Fatalf("output must contain the PR Details section, got:\n%s", out) } - // The PR link must print exactly once, last — after both sections (not between - // the table and "Insight" as before), and sections must not also print their - // own link (avoiding the duplicate the user flagged). - reviewGroupsIdx := strings.Index(out, "Review Groups") + // Insight must render first, PR Details last (right before the link), and the + // PR link must print exactly once, at the very end. + insightIdx := strings.Index(out, "Insight") + prDetailsIdx := strings.Index(out, "PR Details") + if insightIdx == -1 || prDetailsIdx == -1 || prDetailsIdx < insightIdx { + t.Fatalf("expected Insight to render before PR Details, got:\n%s", out) + } lastLinkIdx := strings.LastIndex(out, "/pulls/42") - if lastLinkIdx == -1 || lastLinkIdx < reviewGroupsIdx { - t.Fatalf("expected the PR link to appear after the Review Groups section, got:\n%s", out) + if lastLinkIdx == -1 || lastLinkIdx < prDetailsIdx { + t.Fatalf("expected the PR link to appear after the PR Details section, got:\n%s", out) } if linkCount := strings.Count(out, "/pulls/42"); linkCount != 1 { t.Fatalf("expected exactly one PR link (sections must not duplicate it), got %d in:\n%s", linkCount, out) @@ -258,8 +259,8 @@ func TestGetPRWorkflow_InsightSuccessRendersSection(t *testing.T) { } // TestReviewGroupCommand_StandaloneRendersLink verifies "get pr:review_group" run on -// its own (not embedded in GetPRWorkflow, so suppressSectionLinkFlag is unset) still -// prints its own trailing PR link. +// its own (it's no longer embedded in GetPRWorkflow) still prints its own trailing +// PR link. func TestReviewGroupCommand_StandaloneRendersLink(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") diff --git a/pkg/spec/code.spec.yaml b/pkg/spec/code.spec.yaml index ef84eb6..b28740e 100644 --- a/pkg/spec/code.spec.yaml +++ b/pkg/spec/code.spec.yaml @@ -365,16 +365,6 @@ nouns: - id: content expr: it.content field_type: multiline_text - - id: commit_sha - label: Commit SHA - expr: it.commit_sha - - id: agent_run_id - label: Agent Run ID - expr: it.metadata.agent_run_id - - id: created - expr: epochMs(it.created) - - id: updated - expr: epochMs(it.updated) - noun: pr_review_group short_desc: Risk-bucketed groups of changed files on a pull request (Harness Code review insights).