diff --git a/internal/cli/cluster.go b/internal/cli/cluster.go index dc3847a5..eb0cfcc7 100644 --- a/internal/cli/cluster.go +++ b/internal/cli/cluster.go @@ -119,8 +119,6 @@ func runClusterInfo( kubeconfigPath, contextOverride, nsOverride string, tokenExpiry int64, ) error { - p.Banner("tracebloc", "cluster diagnostics") - // Bind the active client's namespace exactly like the data commands do, // so this pre-flight targets what `data ingest` will actually target — // and so the multi-client "set your active client" remediation works diff --git a/internal/cli/cluster_info_test.go b/internal/cli/cluster_info_test.go index a3e5a6cb..3997265d 100644 --- a/internal/cli/cluster_info_test.go +++ b/internal/cli/cluster_info_test.go @@ -61,7 +61,7 @@ func mintTokenReactor(cs *fake.Clientset, token string, expiresAt time.Time) { // A reached cluster hosting no tracebloc client exits 4 (distinct from the // kubeconfig exit-3), still errors.Is-identifiable as ErrNoParentRelease. The -// banner + Kubeconfig section print before discovery fails. +// Kubeconfig section prints before discovery fails. func TestRunClusterInfo_NoClientExit4(t *testing.T) { out, err := runInfo(t, fake.NewSimpleClientset(), 600) if got := ExitCodeFromError(err); got != 4 { @@ -70,7 +70,7 @@ func TestRunClusterInfo_NoClientExit4(t *testing.T) { if !errors.Is(err, cluster.ErrNoParentRelease) { t.Errorf("want errors.Is(ErrNoParentRelease), got %v", err) } - for _, want := range []string{"cluster diagnostics", "test-ctx"} { + for _, want := range []string{"test-ctx"} { if !strings.Contains(out, want) { t.Errorf("output missing %q:\n%s", want, out) } diff --git a/internal/cli/data_delete_json_test.go b/internal/cli/data_delete_json_test.go index 33ab4a24..520a55ff 100644 --- a/internal/cli/data_delete_json_test.go +++ b/internal/cli/data_delete_json_test.go @@ -226,7 +226,7 @@ func TestDataDeleteCmd_OutputJSONNeverPrompts(t *testing.T) { if got["status"] != "error" { t.Errorf("got %+v, want status=error", got) } - // The human banner went to stderr, not stdout. + // The human output went to stderr, not stdout. if strings.Contains(out.String(), "tracebloc") && !strings.Contains(out.String(), `"error"`) { t.Errorf("human output leaked to stdout:\n%s", out.String()) } diff --git a/internal/cli/resources.go b/internal/cli/resources.go index 8c311938..ddc9d168 100644 --- a/internal/cli/resources.go +++ b/internal/cli/resources.go @@ -83,10 +83,10 @@ Exit codes: // the jobs-manager env — the same source `cluster doctor` parses, so the two // never disagree. func runResourcesShow(ctx context.Context, p *ui.Printer, opts cluster.KubeconfigOptions) error { - p.Banner("tracebloc", "machine resources") + p.Newline() binding := bindActiveClientNamespace(&opts) - target, err := resolveClusterTarget(ctx, p, opts, binding, false) + target, err := resolveClusterTargetFn(ctx, p, opts, binding, false) if err != nil { return binding.explain(err) } diff --git a/internal/cli/resources_set.go b/internal/cli/resources_set.go index 5772888a..2df0733b 100644 --- a/internal/cli/resources_set.go +++ b/internal/cli/resources_set.go @@ -131,8 +131,6 @@ Exit codes: // clientset + fake helm Runner without the real kubeconfig path (the seam // renderResources / ingestion_run_test already use). func runResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, opts cluster.KubeconfigOptions, req setReq) error { - p.Banner("tracebloc", "machine resources") - // Pure request checks first — no cluster needed, so a bad invocation fails // fast with exit 2 (never touching a live cluster). if err := validateRequestShape(req, pr != nil); err != nil { @@ -234,6 +232,7 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target * // is still validated below, before anything mutates. ceilingUnchanged := sameCeiling(desired, current) if ceilingUnchanged && !phantomGPU { + p.Newline() p.Successf("Each training run already uses up to %s — nothing to change.", perRunSize(desired)) return nil } @@ -243,6 +242,7 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target * // clean no-op — fall through to persist so BuildEnvSpec's explicit-empty // GPU override lands and clears it; otherwise runs stay unschedulable / // fall back to CPU while the heartbeat keeps advertising a GPU. + p.Newline() p.Infof("Your CPU and memory budget is unchanged — but this machine has no GPU while the cluster still requests one, so I'll clear that stale GPU setting so runs can schedule.") } @@ -266,7 +266,10 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target * return &exitError{code: exitFailure, err: fmt.Errorf( "refusing to change the ceiling without confirmation: pass --yes, or run on a terminal")} } - p.Newline() + // PromptHint self-leads with a blank line, so this opens with a single + // blank — no preceding Newline() (that stacked two: the #375 banner- + // removal regression Bugbot caught). Mirrors the dry-run path, which + // leans on Section's own leading newline. p.PromptHint("tracebloc keeps about 1 core and 3 GiB for itself on top of this — it fits on this machine.") proceed, cerr := pr.Confirm(fmt.Sprintf("Let each training run use up to %s?", perRunSize(desired)), true) if cerr != nil { @@ -535,7 +538,6 @@ func persistCeiling(ctx context.Context, p *ui.Printer, target *clusterTarget, o } if dryRun { - p.Newline() p.Section("Dry run — nothing was changed") p.Field("would set each run to", perRunSize(d)) if gpuRemoved { diff --git a/internal/cli/resources_set_test.go b/internal/cli/resources_set_test.go index 99324909..75103c63 100644 --- a/internal/cli/resources_set_test.go +++ b/internal/cli/resources_set_test.go @@ -670,3 +670,41 @@ func TestSet_UntouchedGPUIsKept(t *testing.T) { } func boolPtr(b bool) *bool { return &b } + +// proceedingPrompter answers the final confirm "yes"; the wizard prompts are +// unused on the flag-driven path. +type proceedingPrompter struct{} + +func (proceedingPrompter) Input(string, string, string, func(string) error) (string, error) { + return "", errInteractiveCancelled +} +func (proceedingPrompter) Select(string, string, []string, string) (string, error) { + return "", errInteractiveCancelled +} +func (proceedingPrompter) Confirm(string, bool) (bool, error) { return true, nil } + +// TestSet_ConfirmOpensWithSingleBlank: after the banner removal (#375) the +// flag-driven confirm path must still open with exactly ONE blank line. +// PromptHint self-leads with a newline, so a preceding Newline() stacked two — +// the command opened with a double blank (Bugbot #375). Pins the single-blank +// opening so the redundant Newline() can't creep back. +func TestSet_ConfirmOpensWithSingleBlank(t *testing.T) { + fakeHelm(t) + cs := csWith("8", "32Gi", map[string]string{"RESOURCE_LIMITS": "cpu=2,memory=8Gi"}) + out, err := runSet(t, cs, proceedingPrompter{}, setReq{cores: "4", coresSet: true}) + if err != nil { + t.Fatalf("flag-driven confirm + proceed should succeed: %v\n%s", err, out) + } + head := out + if len(head) > 48 { + head = head[:48] + } + // PromptHint emits "\n \n": exactly one leading newline, then two + // spaces. A double blank ("\n\n…") is the regression. + if !strings.HasPrefix(out, "\n ") { + t.Errorf("confirm path must open with a single blank line then the hint, got %q", head) + } + if strings.HasPrefix(out, "\n\n") { + t.Errorf("confirm path opens with a DOUBLE blank line (banner-removal regression): %q", head) + } +} diff --git a/internal/cli/resources_test.go b/internal/cli/resources_test.go index 1dbbfc93..5281c71d 100644 --- a/internal/cli/resources_test.go +++ b/internal/cli/resources_test.go @@ -85,6 +85,39 @@ func TestRenderResources_ShowsMachineAndTrainingCeiling(t *testing.T) { } } +// TestShow_OpensWithSingleBlank: after the banner removal (#375), the outer +// runResourcesShow must open with exactly ONE blank line before the view. The +// leading Newline() lives in runResourcesShow (before resolve, so a resolve-time +// redirect line also gets a blank) — a spot every renderResources-level test +// skips — so pin it by driving the outer function through the resolve seam. +// Mirrors TestSet_ConfirmOpensWithSingleBlank. (Asad review, #375.) +func TestShow_OpensWithSingleBlank(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + orig := resolveClusterTargetFn + t.Cleanup(func() { resolveClusterTargetFn = orig }) + cs := csWith("8", "32Gi", map[string]string{"RESOURCE_LIMITS": "cpu=4,memory=16Gi"}) + resolveClusterTargetFn = func(_ context.Context, _ *ui.Printer, _ cluster.KubeconfigOptions, _ activeClientBinding, _ bool) (*clusterTarget, error) { + return resTarget(cs), nil + } + + var buf bytes.Buffer + if err := runResourcesShow(context.Background(), ui.New(&buf, ui.WithColor(false)), cluster.KubeconfigOptions{Context: "my-ctx"}); err != nil { + t.Fatalf("runResourcesShow: %v\n%s", err, buf.String()) + } + out := buf.String() + head := out + if len(head) > 48 { + head = head[:48] + } + // Stat does not self-lead, so the single leading Newline() is the only blank. + if !strings.HasPrefix(out, "\n ") { + t.Errorf("show must open with a single blank line then the view, got %q", head) + } + if strings.HasPrefix(out, "\n\n") { + t.Errorf("show opens with a DOUBLE blank line: %q", head) + } +} + // TestRenderResources_ChartDefaultWhenEnvUnset: with no RESOURCE_* env, the // ceiling reported is the chart default (cpu=2,memory=8Gi), not "unknown". func TestRenderResources_ChartDefaultWhenEnvUnset(t *testing.T) { diff --git a/internal/ui/ui.go b/internal/ui/ui.go index 088915d1..0dfd29ba 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -1,7 +1,7 @@ // Package ui renders installer-style terminal output for the tracebloc -// CLI — colored step headers, ✔/⚠/· status lines, dim hints, and a -// branded banner — matching the look of the tracebloc/client one-line -// installer (scripts/lib/common.sh). +// CLI — colored step headers, ✔/⚠/· status lines, and dim hints — +// matching the look of the tracebloc/client one-line installer +// (scripts/lib/common.sh). // // Everything goes through a Printer, constructed with New. A Printer // colorizes only when its writer is a real terminal and NO_COLOR is @@ -243,19 +243,8 @@ func (p *Printer) out(format string, a ...any) { _, _ = fmt.Fprintf(p.w, format, a...) } -// Banner prints the branded intro block: a bold-cyan title, a dim rule, -// and an optional subtitle. Mirrors common.sh print_banner. -func (p *Printer) Banner(title, subtitle string) { - p.out("\n %s\n", p.hue(title, toneHeading)) - p.out(" %s\n", p.paint("────────────────────────────────────────", color.Faint)) - if subtitle != "" { - p.out(" %s\n", subtitle) - } - p.out("\n") -} - // Para prints a normal-weight paragraph, each line indented to match -// Banner/Section bodies. It splits on embedded newlines so multi-line +// Section bodies. It splits on embedded newlines so multi-line // prose keeps the indent. Use for explanatory prose — distinct from // Hintf (dim one-liners) and Infof (· bullets). func (p *Printer) Para(text string) { diff --git a/internal/ui/ui_test.go b/internal/ui/ui_test.go index 2c2cf741..a53aac40 100644 --- a/internal/ui/ui_test.go +++ b/internal/ui/ui_test.go @@ -35,7 +35,6 @@ func TestNew_BufferDefaultsToPlain(t *testing.T) { func TestWithColorFalse_OmitsANSI(t *testing.T) { var buf bytes.Buffer p := New(&buf, WithColor(false)) - p.Banner("tracebloc", "declarative ingestion") p.Step(1, 3, "Discover cluster") p.Warnf("PVC is %s", "ReadWriteOnce") p.Hintf("pass --namespace to override") @@ -43,7 +42,7 @@ func TestWithColorFalse_OmitsANSI(t *testing.T) { if strings.Contains(buf.String(), esc) { t.Errorf("WithColor(false) still emitted ANSI: %q", buf.String()) } - for _, want := range []string{"tracebloc", "Step 1/3", "Discover cluster", "ReadWriteOnce"} { + for _, want := range []string{"Step 1/3", "Discover cluster", "ReadWriteOnce"} { if !strings.Contains(buf.String(), want) { t.Errorf("output missing %q: %q", want, buf.String()) }