diff --git a/modules/code/code.go b/modules/code/code.go index 646e493..30bbb0e 100644 --- a/modules/code/code.go +++ b/modules/code/code.go @@ -21,4 +21,5 @@ func ModuleInit(reg registry.ModuleRegistrar) { reg.RegisterFlagResolveFn(resolvePrincipalIDFnID, resolvePrincipalID) reg.RegisterWorkflow(getPRWorkflowID, GetPRWorkflow) reg.RegisterTextFormatter(reviewGroupTextFormatterID, reviewGroupTextFormatter) + reg.RegisterTextFormatter(insightTextFormatterID, insightTextFormatter) } diff --git a/modules/code/insight.go b/modules/code/insight.go index a15aa6d..b7bb337 100644 --- a/modules/code/insight.go +++ b/modules/code/insight.go @@ -6,102 +6,17 @@ package code import ( "fmt" "io" + "strings" "github.com/harness/cli/pkg/cmdctx" - "github.com/harness/cli/pkg/exprenv" - "github.com/harness/cli/pkg/hlog" - "github.com/harness/cli/pkg/registry" + "github.com/harness/cli/pkg/console" ) const ( - getPRWorkflowID = "get_pr" reviewGroupTextFormatterID = "pr_review_group_text" + insightTextFormatterID = "pr_insight_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. -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 isMachineFormat(ctx.FormatFlags.Format) || cmdctx.GetBool(ctx.FlagValues, "list-fields") { - _, err := registry.RunEndpoint(ctx, baseSpec.Endpoint) - return err - } - - // 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 }() - - for _, section := range insightSections { - cs := ctx.Resolver.GetSpec(section.verb, section.noun) - if cs == nil || cs.Endpoint == nil { - 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 - // 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("insight fetch failed, omitting from get pr", "section", section.label, "err", err) - continue - } - fmt.Printf("\n--- %s ---\n", section.label) - 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 - - 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) - } - } - 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. @@ -110,7 +25,7 @@ func reviewGroupTextFormatter(w io.Writer, d cmdctx.DataAccessor) error { if len(groups) == 0 { fmt.Fprintln(w, "No review groups.") } - for i, raw := range groups { + for _, raw := range groups { g, ok := raw.(map[string]any) if !ok { continue @@ -121,11 +36,15 @@ func reviewGroupTextFormatter(w io.Writer, d cmdctx.DataAccessor) error { if tags, ok := g["tags"].(map[string]any); ok { risk, _ = tags["risk"].(string) } - fmt.Fprintf(w, "\nGroup %d: %s", i+1, title) + riskTag := "" if risk != "" { - fmt.Fprintf(w, " [risk: %s]", risk) + riskTag = fmt.Sprintf(" [%s]", risk) + } + line := fmt.Sprintf("● %s%s", title, riskTag) + if c := riskColor(risk); c != 0 { + line = console.WithColor(c, line) } - fmt.Fprintln(w) + fmt.Fprintf(w, "\n%s\n", line) if desc != "" { fmt.Fprintln(w, desc) } @@ -149,3 +68,43 @@ func reviewGroupTextFormatter(w io.Writer, d cmdctx.DataAccessor) error { } return nil } + +// riskColor maps a risk bucket ("low"/"medium"/"high", case-insensitive) to the +// color it's displayed in. Returns 0 (no color) for any other value, including empty. +func riskColor(risk string) console.Color { + switch strings.ToLower(risk) { + case "low": + return console.ColorGreen + case "medium": + return console.ColorYellow + case "high": + return console.ColorRed + default: + return 0 + } +} + +// insightTextFormatter renders the AI code-review overview for a pull request as a +// colorized (by risk) header/footer section marker around the review content. +// The content is printed verbatim: wrapping is left to the terminal so words and +// URLs are never split, and the output stays copy-paste clean. +func insightTextFormatter(w io.Writer, d cmdctx.DataAccessor) error { + renderInsight(w, d.GetString("it.risk"), d.GetString("it.content")) + return nil +} + +// renderInsight is the shared rendering step behind insightTextFormatter and +// DebugRenderInsightHandler, so the debug command exercises the exact same +// styling without needing a fake cmdctx.DataAccessor. +func renderInsight(w io.Writer, risk, content string) { + heading := "AI Code Review" + if risk != "" { + heading += fmt.Sprintf(" [%s risk]", risk) + } + console.RenderTextBox(w, console.TextBox{ + Icon: "✨", + Header: heading, + HeaderColor: riskColor(risk), + Text: strings.TrimSpace(content), + }) +} diff --git a/modules/code/insight_test.go b/modules/code/insight_test.go index 0e103a5..2e9e74c 100644 --- a/modules/code/insight_test.go +++ b/modules/code/insight_test.go @@ -58,10 +58,14 @@ func (s spyResolver) GetSpec(verb, noun string) *spec.CommandSpec { } func (s spyResolver) ResolveTextFormatter(id string) cmdctx.TextFormatterFn { - if id == reviewGroupTextFormatterID { + switch id { + case reviewGroupTextFormatterID: return reviewGroupTextFormatter + case insightTextFormatterID: + return insightTextFormatter + default: + return nil } - return nil } // testNounURLPath is a stand-in url_path template shared by test nouns: it resolves @@ -94,7 +98,10 @@ 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", TextFooter: "\n{{url(it)}}\n"}, + Endpoint: &spec.EndpointSpec{ + Method: "GET", Path: path, ItemExpr: "it", TextFooter: "\n{{url(it)}}\n", + TextFormatter: insightTextFormatterID, + }, } } @@ -211,7 +218,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") { + if strings.Contains(out, "AI Code Review") { t.Fatalf("output must omit failed sections, got:\n%s", out) } } @@ -235,23 +242,23 @@ func TestGetPRWorkflow_InsightSuccessRendersSection(t *testing.T) { 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) + if !strings.Contains(out, "AI Code Review") { + t.Fatalf("output must contain the AI Code Review heading, got:\n%s", out) } - if !strings.Contains(out, "PR Details") { - t.Fatalf("output must contain the PR Details section, got:\n%s", out) + if !strings.Contains(out, "Number:") { + t.Fatalf("output must contain the PR's labeled fields, got:\n%s", out) } - // Insight must render first, PR Details last (right before the link), and the + // Labeled fields must render first, Insight 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) + numberIdx := strings.Index(out, "Number:") + insightIdx := strings.Index(out, "AI Code Review") + if insightIdx == -1 || numberIdx == -1 || insightIdx < numberIdx { + t.Fatalf("expected labeled fields to render before Insight, got:\n%s", out) } lastLinkIdx := strings.LastIndex(out, "/pulls/42") - if lastLinkIdx == -1 || lastLinkIdx < prDetailsIdx { - t.Fatalf("expected the PR link to appear after the PR Details section, got:\n%s", out) + if lastLinkIdx == -1 || lastLinkIdx < insightIdx { + t.Fatalf("expected the PR link to appear after the Insight 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) @@ -281,6 +288,62 @@ func TestReviewGroupCommand_StandaloneRendersLink(t *testing.T) { } } +// --------------------------------------------------------------------------- +// insightTextFormatter +// --------------------------------------------------------------------------- + +type fakeDataAccessor struct { + values map[string]string + slices map[string][]any +} + +func (f fakeDataAccessor) GetString(path string) string { return f.values[path] } +func (f fakeDataAccessor) GetInt64(string) int64 { return 0 } +func (f fakeDataAccessor) GetBool(string) bool { return false } +func (f fakeDataAccessor) GetTs(string) string { return "" } +func (f fakeDataAccessor) GetData() any { return nil } +func (f fakeDataAccessor) GetSlice(path string) []any { return f.slices[path] } + +// --------------------------------------------------------------------------- +// reviewGroupTextFormatter +// --------------------------------------------------------------------------- + +func TestReviewGroupTextFormatter_ColorizesBulletAndRiskTag(t *testing.T) { + groups := []any{ + map[string]any{ + "title": "Auth middleware changes", + "description": "Touches token validation.", + "tags": map[string]any{"risk": "high"}, + "files": []any{ + map[string]any{"path": "auth/middleware.go"}, + }, + }, + } + out := captureStdout(t, func() { + err := reviewGroupTextFormatter(os.Stdout, fakeDataAccessor{ + slices: map[string][]any{"it.groups": groups}, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + if strings.Contains(out, "Group 1") || strings.Contains(out, "Group ") { + t.Fatalf("output must not contain a numbered \"Group\" label, got:\n%s", out) + } + if !strings.Contains(out, "●") { + t.Fatalf("output must contain the risk bullet, got:\n%s", out) + } + if !strings.Contains(out, "Auth middleware changes") { + t.Fatalf("output must contain the group title, got:\n%s", out) + } + if !strings.Contains(out, "[high]") { + t.Fatalf("output must contain the risk tag, got:\n%s", out) + } + if !strings.Contains(out, "auth/middleware.go") { + t.Fatalf("output must still list the file path, got:\n%s", out) + } +} + // --------------------------------------------------------------------------- // ModuleInit // --------------------------------------------------------------------------- diff --git a/modules/code/pr.go b/modules/code/pr.go index b752b92..0475513 100644 --- a/modules/code/pr.go +++ b/modules/code/pr.go @@ -8,8 +8,110 @@ import ( "strings" "github.com/harness/cli/pkg/cmdctx" + "github.com/harness/cli/pkg/exprenv" + "github.com/harness/cli/pkg/extractutil" + "github.com/harness/cli/pkg/format" + "github.com/harness/cli/pkg/hlog" + "github.com/harness/cli/pkg/registry" + "github.com/harness/cli/pkg/spec" ) +const getPRWorkflowID = "get_pr" + +// isMachineFormat mirrors exprenv.isMachineFormat (unexported): these formats are +// meant for structured consumption, so the insight section (extra, ad hoc text) is 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 the base pull request (hard fail on +// error) and the "pr:insight" section (best-effort — a failure only omits it, it never +// fails the command), then hands everything fetched to renderPR. +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 isMachineFormat(ctx.FormatFlags.Format) || cmdctx.GetBool(ctx.FlagValues, "list-fields") { + _, err := registry.RunEndpoint(ctx, baseSpec.Endpoint) + return err + } + + pr, err := registry.CallEndpoint(ctx, baseSpec.Endpoint) + if err != nil { + return err + } + + insightSpec := ctx.Resolver.GetSpec("get", "pr:insight") + var insightData any + if insightSpec == nil || insightSpec.Endpoint == nil { + hlog.Warn("insight section spec not found, omitting from get pr") + insightSpec = nil + } else { + insightData, err = registry.CallEndpoint(ctx, insightSpec.Endpoint) + if err != nil { + hlog.Warn("insight fetch failed, omitting from get pr", "err", err) + insightSpec = nil + } + } + + return renderPR(ctx, baseSpec, pr, insightSpec, insightData) +} + +// renderPR prints "get pr" output in three parts so the AI Code Review (Insight) +// section can sit between them: labeled fields, then the insight section (best-effort +// — a failure here still only omits it), then the description text block and footer. +func renderPR(ctx *cmdctx.Ctx, baseSpec *spec.CommandSpec, pr any, insightSpec *spec.CommandSpec, insightData any) error { + w, closeW, err := format.OpenWriter(ctx.FormatFlags.OutFile) + if err != nil { + return err + } + defer closeW() + + nounForFields := ctx.Noun + if ctx.FieldsNoun != "" { + nounForFields = ctx.FieldsNoun + } + var labeledFields []spec.FieldDef + if nd := ctx.Resolver.GetNoun(nounForFields); nd != nil { + for _, f := range nd.Fields { + if f.FieldType == "multiline_text" || f.FieldType == "yaml" { + continue + } + labeledFields = append(labeledFields, f) + } + } + + exprEnv := exprenv.Make(ctx) + interpolate := func(tmpl string, item any) string { + s, _ := exprenv.ResolvePath(exprenv.WithIt(exprEnv, item), tmpl) + return s + } + data := extractutil.MakeDataAccessor(exprEnv, pr) + + if err := format.BuildTextFieldFormatter(labeledFields, "", "", interpolate)(w, data); err != nil { + return err + } + + if insightSpec != nil { + sectionData := extractutil.MakeDataAccessor(exprEnv, insightData) + if err := insightTextFormatter(w, sectionData); err != nil { + hlog.Warn("insight render failed, omitting from get pr", "err", err) + } + } + + if desc := strings.TrimSpace(data.GetString("it.description")); desc != "" { + fmt.Fprintf(w, "\n%s\n", desc) + } + _, err = fmt.Fprint(w, interpolate(baseSpec.Endpoint.TextFooter, pr)) + return err +} + // createPRBodyFn builds the pull request create body. // Required: --set title= source_branch=<branch> target_branch=<branch> // Optional: --set description=<text> OR -f <file> for multi-line description. diff --git a/pkg/console/console.go b/pkg/console/console.go index e91f166..7608ab5 100644 --- a/pkg/console/console.go +++ b/pkg/console/console.go @@ -93,6 +93,26 @@ func WithColor(c Color, text string) string { return fmt.Sprintf("\x1b[%dm%s\x1b[0m", int(c), text) } +// WithBoldColor wraps text in a bold ANSI code, additionally tinted with c when +// c != 0, when stdout is a TTY. +func WithBoldColor(c Color, text string) string { + if !ensureStdoutTTY() { + return text + } + if c != 0 { + return fmt.Sprintf("\x1b[1;%dm%s\x1b[0m", int(c), text) + } + return fmt.Sprintf("\x1b[1m%s\x1b[0m", text) +} + +// WithBold wraps text in a bold ANSI code (no color) when stdout is a TTY. +func WithBold(text string) string { + if !ensureStdoutTTY() { + return text + } + return fmt.Sprintf("\x1b[1m%s\x1b[0m", text) +} + // ReadSecret prints prompt to stderr and reads a masked line from the terminal. func ReadSecret(prompt string) (string, error) { if !ensureTTY() { diff --git a/pkg/console/textbox.go b/pkg/console/textbox.go new file mode 100644 index 0000000..cdd01fe --- /dev/null +++ b/pkg/console/textbox.go @@ -0,0 +1,32 @@ +// Copyright © 2026 Harness Inc. +// SPDX-License-Identifier: Apache-2.0 + +package console + +import ( + "fmt" + "io" +) + +// TextBox is a lightweight section marker: a colorized header line (optionally +// preceded by Icon, suppressed when stdout isn't a TTY), then Text verbatim, +// then a dimmed fixed-width closing rule. +type TextBox struct { + Icon string + Header string + HeaderColor Color + Text string +} + +// RenderTextBox writes box to w. Text is written verbatim — callers must not +// wrap it themselves; wrapping is left to the terminal so words and URLs are +// never split, and the output stays copy-paste clean. +func RenderTextBox(w io.Writer, box TextBox) { + header := box.Header + ":" + if box.Icon != "" && ensureStdoutTTY() { + header = box.Icon + " " + header + } + fmt.Fprintln(w, WithBoldColor(box.HeaderColor, header)) + fmt.Fprintln(w, box.Text) + fmt.Fprintln(w, WithColor(ColorBrightBlack, "──────────")) +} diff --git a/pkg/spec/code.spec.yaml b/pkg/spec/code.spec.yaml index b28740e..8d4fcd4 100644 --- a/pkg/spec/code.spec.yaml +++ b/pkg/spec/code.spec.yaml @@ -760,6 +760,7 @@ commands: query_params: repo_path: auth.scope + "/" + ctx.idParts[0] text_footer: "\n{{url(it)}}\n" + text_formatter: pr_insight_text # ── pr_review_group (Harness Code review insights) ────────────────────────────