From fa408ff740546b8b0dc464edba56a97607bc0257 Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Fri, 28 Aug 2026 15:55:23 -0700 Subject: [PATCH 01/12] [RAPTOR-19538] feat(uidiff): shared unified-diff rendering primitives New internal/uidiff package: an ordered row list in, a unified diff out. It owns only what is generic about that rendering, so the up command's diff mode (and later a component/template adapter) can stay thin. - Row model: Kind (Context | Add | Del | Unmanaged), Path for the redaction hook, Text pre-formatted by the caller. - Render(w, rows, opts) with a fixed default 3-line context window (opts.Context, zero means 3; deliberately no CLI flag), collapsing each run of Context/Unmanaged rows longer than the window into a single "... N identical lines" that counts exactly the rows it hides, correct at both ends of the list. - Styling reuses the tui palette so a diff and the default plan read as one visual language: SuccessStyle for "+", WarnStyle for "-" and the caller's unmanaged marker, HintStyle for context and collapse lines. - Caller-supplied Redact hook suppresses the value portion of every Kind it fires for; redacted rows render a placeholder built from the path ("set" / "changed" / "(redacted)"), never the value. - CODEOWNERS: internal/uidiff/ routes to @datarobot-oss/workload-cli, keeping the package owned beside the code that will consume it first. The package imports nothing under internal/workload (verified via go list -deps); the one-way dependency up -> uidiff is the point. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- .github/CODEOWNERS | 1 + internal/uidiff/render.go | 199 +++++++++++++++++++ internal/uidiff/render_test.go | 351 +++++++++++++++++++++++++++++++++ internal/uidiff/uidiff.go | 78 ++++++++ 4 files changed, 629 insertions(+) create mode 100644 internal/uidiff/render.go create mode 100644 internal/uidiff/render_test.go create mode 100644 internal/uidiff/uidiff.go diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 31de98c99..0949899f2 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -2,6 +2,7 @@ cmd/workload/ @datarobot-oss/workload-cli cmd/artifact/ @datarobot-oss/workload-cli internal/workload/ @datarobot-oss/workload-cli +internal/uidiff/ @datarobot-oss/workload-cli cmd/pipeline/ @datarobot-oss/compute-services internal/pipeline/ @datarobot-oss/compute-services docs/commands/pipeline*.md @datarobot-oss/compute-services diff --git a/internal/uidiff/render.go b/internal/uidiff/render.go new file mode 100644 index 000000000..bba216ee3 --- /dev/null +++ b/internal/uidiff/render.go @@ -0,0 +1,199 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package uidiff + +import ( + "fmt" + "io" + "strconv" + "strings" + + "github.com/charmbracelet/lipgloss" + "github.com/datarobot/cli/tui" +) + +// defaultContextWindow is the context window git taught everyone to read: +// enough surrounding rows to place a change, not so many that the diff stops +// being shorter than the thing it diffs. +const defaultContextWindow = 3 + +// The marker characters are the vocabulary the plan block already prints, so +// a diff and the default plan read as one visual language. Context and +// unmanaged rows add no marker: their text arrives from the caller complete. +const ( + addMarker = "+" + delMarker = "-" +) + +// The words that stand in for a value the redaction hook refused to print. +// "set" and "changed" are the plan's own vocabulary for a value arriving and +// a value moving; the bracketed token marks rows whose value is simply +// withheld, where neither verb is true. +const ( + setPlaceholder = "set" + changedPlaceholder = "changed" + hiddenPlaceholder = "(redacted)" +) + +// Render writes rows as a unified diff to w. +// +// Every change shows, and so does every unchanged row within the context +// window of one. The rest of each unchanged run collapses into a single +// "... N identical lines" line that counts only the rows it hides, so the +// rendering is always shorter than the row list it came from and never +// understates how much it left out. Redacted rows render as a placeholder +// built from their path, whatever their Kind: no value text reaches the +// writer for a path the hook refuses. +func Render(w io.Writer, rows []Row, opts Options) error { + window := opts.Context + if window <= 0 { + window = defaultContextWindow + } + + shown := shownRows(rows, window) + + var b strings.Builder + + for i := 0; i < len(rows); { + if shown[i] { + b.WriteString(renderRow(rows[i], opts.Redact)) + b.WriteString("\n") + + i++ + + continue + } + + hidden := 0 + + for i+hidden < len(rows) && !shown[i+hidden] { + hidden++ + } + + b.WriteString(collapseLine(hidden)) + b.WriteString("\n") + + i += hidden + } + + if _, err := io.WriteString(w, b.String()); err != nil { + return fmt.Errorf("cannot write diff: %w", err) + } + + return nil +} + +// shownRows marks which rows the window keeps: every change, plus every row +// within the window of one. Distance is measured in rows, not in equality, +// because the collapse answers "how much of this did I stop reading?" and +// that question does not care what the hidden rows say. +func shownRows(rows []Row, window int) []bool { + shown := make([]bool, len(rows)) + + for i, row := range rows { + if row.Kind != Add && row.Kind != Del { + continue + } + + lo := max(i-window, 0) + hi := min(i+window, len(rows)-1) + + for j := lo; j <= hi; j++ { + shown[j] = true + } + } + + return shown +} + +// collapseLine is the one line that stands for a hidden run. It counts the +// rows it hides rather than the ones it kept, so a reader can always tell +// how much output did not happen. +func collapseLine(hidden int) string { + return tui.HintStyle.Render("... " + strconv.Itoa(hidden) + " identical lines") +} + +// renderRow renders one shown row: the marker and text its kind calls for, +// with the redaction hook standing in for the text of a row whose value must +// not print. +func renderRow(row Row, redact func(string) bool) string { + marker, text := rowContent(row, redact) + + if marker == "" { + return rowStyle(row.Kind).Render(text) + } + + return rowStyle(row.Kind).Render(marker + " " + text) +} + +// rowContent decides what a row prints. The caller's Text is the answer for +// everything but a redacted path, where the value inside the caller's text +// is replaced by the placeholder its kind calls for: the path stays, the +// value never does. The hook is consulted once per row, here, so a caller +// with an expensive predicate pays for it once. +func rowContent(row Row, redact func(string) bool) (marker, text string) { + text = row.Text + + if redact != nil && redact(row.Path) { + text = redactedText(row) + } + + switch row.Kind { + case Add: + return addMarker, text + case Del: + return delMarker, text + case Context, Unmanaged: + return "", text + } + + // An out-of-range Kind has no marker of its own; it renders as context + // rather than inventing one. + return "", text +} + +// redactedText is what a row prints when its value must not: the path, plus +// the word its kind calls for. "set" and "changed" are the plan's own +// vocabulary for a value arriving and a value moving; the bracketed token +// marks rows whose value is simply withheld, where neither verb is true. +func redactedText(row Row) string { + switch row.Kind { + case Add: + return row.Path + ": " + setPlaceholder + case Del: + return row.Path + ": " + changedPlaceholder + case Context, Unmanaged: + return row.Path + ": " + hiddenPlaceholder + } + + return row.Path + ": " + hiddenPlaceholder +} + +// rowStyle is the palette entry for a row's kind. An addition reads as new, +// a replacement and an unmanaged field as something being moved, and +// everything merely contextual as commentary on it. +func rowStyle(kind Kind) lipgloss.Style { + switch kind { + case Add: + return tui.SuccessStyle + case Del, Unmanaged: + return tui.WarnStyle + case Context: + return tui.HintStyle + } + + // An out-of-range Kind reads as context, quieter than guessing wrong. + return tui.HintStyle +} diff --git a/internal/uidiff/render_test.go b/internal/uidiff/render_test.go new file mode 100644 index 000000000..8442aaca8 --- /dev/null +++ b/internal/uidiff/render_test.go @@ -0,0 +1,351 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package uidiff + +import ( + "errors" + "regexp" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// renderRows runs Render against a buffer, which is how every test below +// reads a rendering. +func renderRows(t *testing.T, rows []Row, opts Options) string { + t.Helper() + + var b strings.Builder + + require.NoError(t, Render(&b, rows, opts)) + + return b.String() +} + +// stripANSI removes lipgloss colour escapes so assertions can match plain +// text. Styling is environment dependent (it varies with terminal colour +// support), so tests assert on content, never on the escape sequences. That +// is also the parseability guarantee: with styles stripped the markers are +// still there, because they are ordinary characters the styling wrapped. +var ansiPattern = regexp.MustCompile(`\x1b\[[0-9;]*m`) + +func stripANSI(text string) string { + return ansiPattern.ReplaceAllString(text, "") +} + +// ctxRow, addRow and delRow build rows whose visible text is the argument +// itself, so an expected rendering reads as the rows it came from. +func ctxRow(text string) Row { + return Row{Kind: Context, Path: text, Text: text} +} + +func addRow(text string) Row { + return Row{Kind: Add, Path: text, Text: text} +} + +func delRow(text string) Row { + return Row{Kind: Del, Path: text, Text: text} +} + +// extraRow is an unmanaged row the way the workload caller emits them: the +// leading marker and the wording are the caller's, carried in Text, and the +// renderer adds nothing of its own. +func extraRow(path string) Row { + return Row{Kind: Unmanaged, Path: path, Text: "~ " + path + ": not managed by this file"} +} + +// VAL-DIFF-001: the window is exactly three unchanged rows either side of a +// change, and everything beyond it on both sides collapses. +func TestRender_ContextWindowIsThreeEachSide(t *testing.T) { + rows := []Row{ + ctxRow("before-1"), ctxRow("before-2"), ctxRow("before-3"), + ctxRow("before-4"), ctxRow("before-5"), ctxRow("before-6"), + addRow("the change"), + ctxRow("after-1"), ctxRow("after-2"), ctxRow("after-3"), + ctxRow("after-4"), ctxRow("after-5"), ctxRow("after-6"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "... 3 identical lines\n"+ + "before-4\nbefore-5\nbefore-6\n"+ + "+ the change\n"+ + "after-1\nafter-2\nafter-3\n"+ + "... 3 identical lines\n", + stripANSI(out)) +} + +// A side that cannot fill the window shows what is there rather than padding. +func TestRender_ChangeWithFewerContextRowsShowsWhatIsThere(t *testing.T) { + rows := []Row{ + ctxRow("only-1"), ctxRow("only-2"), + delRow("the change"), + ctxRow("after"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "only-1\nonly-2\n- the change\nafter\n", + stripANSI(out)) +} + +// VAL-DIFF-002: a run at exactly the window shows in full, no collapse line. +func TestRender_RunAtTheWindowIsShownInFull(t *testing.T) { + rows := []Row{ + addRow("first change"), + ctxRow("between-1"), ctxRow("between-2"), ctxRow("between-3"), + delRow("second change"), + } + + out := stripANSI(renderRows(t, rows, Options{})) + + assert.Equal(t, + "+ first change\nbetween-1\nbetween-2\nbetween-3\n- second change\n", + out) + assert.NotContains(t, out, "...", "a run at the window never collapses") +} + +// One row past the window: the three change-adjacent rows survive and the +// single farthest one collapses, counted exactly. +func TestRender_RunOnePastTheWindowCollapses(t *testing.T) { + rows := []Row{ + ctxRow("far"), ctxRow("near-1"), ctxRow("near-2"), ctxRow("near-3"), + addRow("the change"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "... 1 identical lines\nnear-1\nnear-2\nnear-3\n+ the change\n", + stripANSI(out)) +} + +// Two changes seven rows apart: each keeps its three, and the one middle row +// neither side reaches is the whole hidden count. +func TestRender_SevenBetweenTwoChangesHidesTheMiddleOne(t *testing.T) { + rows := []Row{ + addRow("first"), + ctxRow("c1"), ctxRow("c2"), ctxRow("c3"), + ctxRow("c4"), ctxRow("c5"), ctxRow("c6"), ctxRow("c7"), + delRow("second"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "+ first\nc1\nc2\nc3\n... 1 identical lines\nc5\nc6\nc7\n- second\n", + stripANSI(out)) +} + +// Two long runs in one body collapse separately, each line counting only its +// own run. +func TestRender_TwoLongRunsCollapseIndependently(t *testing.T) { + rows := []Row{ + ctxRow("lead-1"), ctxRow("lead-2"), ctxRow("lead-3"), + ctxRow("lead-4"), ctxRow("lead-5"), + addRow("the change"), + ctxRow("tail-1"), ctxRow("tail-2"), ctxRow("tail-3"), + ctxRow("tail-4"), ctxRow("tail-5"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "... 2 identical lines\nlead-3\nlead-4\nlead-5\n"+ + "+ the change\n"+ + "tail-1\ntail-2\ntail-3\n... 2 identical lines\n", + stripANSI(out)) +} + +// VAL-DIFF-003: runs touching the ends of the list collapse away from the +// change, on the far side of the rows that survive. +func TestRender_ListEdgeRunsCollapseAwayFromTheChange(t *testing.T) { + leading := renderRows(t, []Row{ + ctxRow("c1"), ctxRow("c2"), ctxRow("c3"), ctxRow("c4"), ctxRow("c5"), + addRow("the change"), + }, Options{}) + + assert.Equal(t, + "... 2 identical lines\nc3\nc4\nc5\n+ the change\n", + stripANSI(leading), + "the collapse line sits on the far side of a leading run, never the change side") + + trailing := renderRows(t, []Row{ + delRow("the change"), + ctxRow("c1"), ctxRow("c2"), ctxRow("c3"), ctxRow("c4"), ctxRow("c5"), + }, Options{}) + + assert.Equal(t, + "- the change\nc1\nc2\nc3\n... 2 identical lines\n", + stripANSI(trailing)) +} + +// A body with no change at all is one whole hidden run: the collapse line is +// the only output, and it counts everything. +func TestRender_AllContextCollapsesToOneLine(t *testing.T) { + out := renderRows(t, []Row{ + ctxRow("c1"), ctxRow("c2"), ctxRow("c3"), ctxRow("c4"), + }, Options{}) + + assert.Equal(t, "... 4 identical lines\n", stripANSI(out)) +} + +// VAL-DIFF-005: unmanaged rows run as context, carry the caller's marker +// from Text, and never render as a removal. +func TestRender_UnmanagedRowsRunWithContextAndNeverRenderAsRemovals(t *testing.T) { + rows := []Row{ + ctxRow("c1"), ctxRow("c2"), + extraRow("environmentVars[SIDECAR]"), + extraRow("environmentVars[AUTOSCALE]"), + addRow("runtime.replicaCount: 3"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "... 1 identical lines\nc2\n"+ + "~ environmentVars[SIDECAR]: not managed by this file\n"+ + "~ environmentVars[AUTOSCALE]: not managed by this file\n"+ + "+ runtime.replicaCount: 3\n", + stripANSI(out)) +} + +// The markers are the vocabulary the plan block already prints, so a diff +// and the default plan read as one visual language. +func TestRender_MarkersFollowThePlanVocabulary(t *testing.T) { + rows := []Row{ + ctxRow("unchanged"), + delRow("runtime.replicaCount: 1"), + addRow("runtime.replicaCount: 3"), + extraRow("environmentVars[SIDECAR]"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "unchanged\n"+ + "- runtime.replicaCount: 1\n"+ + "+ runtime.replicaCount: 3\n"+ + "~ environmentVars[SIDECAR]: not managed by this file\n", + stripANSI(out)) +} + +// VAL-DIFF-004: the redaction hook suppresses the text of every Kind it +// fires for, whatever the row was carrying, and never touches the others. +func TestRender_RedactionAppliesToEveryKind(t *testing.T) { + redact := func(path string) bool { + return strings.Contains(path, ".environmentVars[") + } + + rows := []Row{ + ctxRow("port: 9090"), + {Kind: Context, Path: "runtime.environmentVars[LOG_LEVEL]", Text: "runtime.environmentVars[LOG_LEVEL]: debug"}, + {Kind: Add, Path: "runtime.environmentVars[TOKEN]", Text: "runtime.environmentVars[TOKEN]: plaintext-secret"}, + {Kind: Del, Path: "runtime.environmentVars[OLD]", Text: "runtime.environmentVars[OLD]: dr-credential:abc123/client-secret"}, + {Kind: Unmanaged, Path: "runtime.environmentVars[SIDE]", Text: "~ runtime.environmentVars[SIDE]: not managed by this file"}, + addRow("port: 9091"), + } + + out := stripANSI(renderRows(t, rows, Options{Redact: redact})) + + for _, secret := range []string{ + "plaintext-secret", + "dr-credential:abc123/client-secret", + "debug", + "not managed by this file", + } { + assert.NotContains(t, out, secret) + } + + assert.Contains(t, out, "runtime.environmentVars[LOG_LEVEL]: (redacted)") + assert.Contains(t, out, "+ runtime.environmentVars[TOKEN]: set") + assert.Contains(t, out, "- runtime.environmentVars[OLD]: changed") + assert.Contains(t, out, "runtime.environmentVars[SIDE]: (redacted)") + + // Rows the hook does not fire for keep their values, so redaction is a + // property of the path, not of the renderer. + assert.Contains(t, out, "port: 9090") + assert.Contains(t, out, "+ port: 9091") +} + +// Without a hook nothing is redacted; the placeholder machinery is opt-in. +func TestRender_WithoutARedactHookTheValuesPrint(t *testing.T) { + rows := []Row{ + {Kind: Add, Path: "runtime.environmentVars[TOKEN]", Text: "runtime.environmentVars[TOKEN]: secret"}, + addRow("port: 9091"), + } + + out := renderRows(t, rows, Options{}) + + assert.Contains(t, stripANSI(out), "+ runtime.environmentVars[TOKEN]: secret") +} + +// The hook is a per-row decision: once per row, never per line of output. +func TestRender_RedactHookIsConsultedOncePerRow(t *testing.T) { + calls := 0 + + rows := []Row{ctxRow("a"), addRow("b"), delRow("c"), extraRow("d"), ctxRow("e")} + + var b strings.Builder + + require.NoError(t, Render(&b, rows, Options{Redact: func(string) bool { + calls++ + + return false + }})) + + assert.Equal(t, len(rows), calls, "the hook runs once per row") +} + +// VAL-DIFF-001: zero means the default window, and the Context field is how +// a caller asks for a different one. +func TestRender_ZeroContextMeansTheDefaultWindow(t *testing.T) { + rows := []Row{ + ctxRow("c1"), ctxRow("c2"), ctxRow("c3"), ctxRow("c4"), ctxRow("c5"), + addRow("the change"), + } + + assert.Equal(t, + renderRows(t, rows, Options{Context: 3}), + renderRows(t, rows, Options{}), + "zero and three are the same window") + + out := renderRows(t, rows, Options{Context: 1}) + + assert.Equal(t, "... 4 identical lines\nc5\n+ the change\n", stripANSI(out)) +} + +// An empty body is an empty rendering, not a stray newline. +func TestRender_NoRowsWritesNothing(t *testing.T) { + assert.Empty(t, renderRows(t, nil, Options{})) +} + +var errWriteFailed = errors.New("write failed") + +type failingWriter struct{} + +func (failingWriter) Write([]byte) (int, error) { return 0, errWriteFailed } + +// A writer that fails leaves the caller holding the error; a diff that +// reported success while losing lines would be worse than one that failed. +func TestRender_PropagatesWriteErrors(t *testing.T) { + err := Render(failingWriter{}, []Row{ctxRow("c")}, Options{}) + + require.ErrorIs(t, err, errWriteFailed) +} diff --git a/internal/uidiff/uidiff.go b/internal/uidiff/uidiff.go new file mode 100644 index 000000000..d72cafd17 --- /dev/null +++ b/internal/uidiff/uidiff.go @@ -0,0 +1,78 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package uidiff turns ordered rows into unified-diff-flavored output: the +// three-line context window git made conventional, collapsing of unchanged +// runs longer than that window, line styling from the CLI's shared palette, +// and a caller-supplied hook that keeps a value its owner refuses to print +// out of the output entirely. +// +// The package knows nothing about what the rows describe. Workloads, +// manifests and their name-keyed lists are the caller's business: the caller +// hands over rows whose text it has already formatted, and a predicate +// saying which paths carry values that must never reach the screen. What +// makes a row an addition, a replacement, context, or unmanaged field is +// likewise the caller's judgment; this package only decides what such a row +// looks like. +package uidiff + +// Kind classifies one row of a diff body. +type Kind int + +const ( + // Context is a row the caller counts as unchanged. Context rows show + // within the window of a change and collapse into a summary beyond it. + Context Kind = iota + + // Add is a value the caller is introducing, rendered with a "+". + Add + + // Del is a value the caller is replacing, rendered with a "-". + Del + + // Unmanaged is a row the caller wants visible but does not manage, such + // as a field the live object carries that the file never names. It + // renders as context, never as a removal, and the caller's Text carries + // its marker. + Unmanaged +) + +// Row is one line of a diff body. +type Row struct { + // Kind selects the marker and style the renderer applies. + Kind Kind + + // Path identifies what the row describes. It exists for the redaction + // hook and nothing else; the visible text comes from Text. + Path string + + // Text is the line's content, already formatted by the caller. For + // Unmanaged rows the leading marker is part of the Text, since only the + // caller knows the vocabulary the row needs. + Text string +} + +// Options tunes one Render call. The zero value is ready to use: the context +// window defaults to three and nothing is redacted. +type Options struct { + // Context is how many unchanged rows each side of a change stay visible. + // Zero means the default, three. There is no CLI flag for it, by design: + // a diff window is a rendering convention, not a knob. + Context int + + // Redact reports whether a path's value must never reach the output. A + // path it reports true for renders as a placeholder in place of its + // Text, whatever the row's Kind. Nil means nothing is redacted. + Redact func(path string) bool +} From 41781489e4e66141e98ee567ee62fda5fd0e7ff9 Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Fri, 28 Aug 2026 16:00:32 -0700 Subject: [PATCH 02/12] [RAPTOR-19538] feat(workload): add DiffRows, a whole-leaf walker beside Subset --diff needs the leaves that already agree as context, not just the ones that differ. DiffRows(want, have) emits one row per leaf of want and marks each changed or unchanged with the same comparison Subset applies: name-keyed lists matched by name, memory-equivalent sizes read as equal, keys visited in sorted order so two runs over the same inputs read the same way. A leaf the live object lacks is one addition row, a whole element at a time, exactly as Subset reports it: adding a container is one act, not one per field. With no live side at all (a first deploy) every leaf is an addition, walked out to its leaves so the diff can show what will be created rather than summarise it. The unmanaged side stays Extra's question; the caller merges the two. Subset and Extra are untouched, so the default plan keeps its exact shape. --- internal/workload/up/diff.go | 163 ++++++++++++ internal/workload/up/diff_test.go | 411 ++++++++++++++++++++++++++++++ 2 files changed, 574 insertions(+) diff --git a/internal/workload/up/diff.go b/internal/workload/up/diff.go index 493fe860e..ff0552852 100644 --- a/internal/workload/up/diff.go +++ b/internal/workload/up/diff.go @@ -281,6 +281,169 @@ func join(path, key string) string { return path + "." + key } +// DiffRow is one leaf of want as a unified diff needs it: every leaf the +// file asks for gets a row, changed or not, so a diff can draw the context +// around a change and not just the change. Subset stays the source of truth +// for what differs; DiffRows is Subset's walk with the silent leaves kept. +type DiffRow struct { + // Path is where the field sits, in the file's own spelling, with + // name-keyed lists addressed by name rather than by index. It is also + // the address the plan's redaction hook reads, so a value that must + // never print is refused here on the same terms. + Path string + + // Want is the value the manifest asks for. + Want any + + // Have is the live value, nil when Absent. + Have any + + // Absent distinguishes a field the live object does not carry at all + // from one that carries a different value, exactly as it does for + // Change: adding a probe is not the same act as moving one. + Absent bool + + // Changed reports whether the live side already agrees. Unchanged rows + // are the context a diff draws between changes, and a size written two + // ways is unchanged here for the same reason it is not drift in Subset. + Changed bool +} + +// DiffRows walks every leaf of want, not just the differing ones, marking +// each row changed or unchanged with the same comparison Subset applies: +// name-keyed lists matched by name, memory-equivalent sizes read as equal, +// keys visited in sorted order so two runs over the same inputs read the +// same way. +// +// A leaf the live object does not have is one addition row, a whole element +// at a time, exactly as Subset reports it: adding a container is one act, +// not one per field. With no live side at all (a first deploy) every leaf +// is an addition, walked out to its leaves so the diff can show what will +// be created rather than summarise it. +// +// The unmanaged side is not walked here: what the live object carries that +// the file never names is Extra's question, and the caller merges the two. +func DiffRows(want, have map[string]any) []DiffRow { + var rows []DiffRow + + if have == nil { + additionRows("", want, &rows) + + return rows + } + + walkRows("", want, have, true, &rows) + + return rows +} + +// walkRows compares one node, the way walk does, but answers with a row for +// every leaf rather than only the disagreeing ones. present says whether +// have was actually there, so a key holding an explicit null is not confused +// with a key that is missing. +func walkRows(path string, want, have any, present bool, out *[]DiffRow) { + if !present { + *out = append(*out, DiffRow{Path: path, Want: want, Absent: true, Changed: true}) + + return + } + + switch w := want.(type) { + case map[string]any: + walkRowsMap(path, w, have, out) + case []any: + walkRowsList(path, w, have, out) + default: + *out = append(*out, DiffRow{Path: path, Want: want, Have: have, Changed: !equalAt(path, want, have)}) + } +} + +// walkRowsMap recurses into an object, in key order so the rows read the +// same way twice. A have side that is not an object is one changed row: it +// cannot be matched key by key, and guessing at a comparison would report +// less than the truth. +func walkRowsMap(path string, want map[string]any, have any, out *[]DiffRow) { + h, ok := have.(map[string]any) + if !ok { + *out = append(*out, DiffRow{Path: path, Want: want, Have: have, Changed: true}) + + return + } + + for _, key := range slices.Sorted(maps.Keys(want)) { + hv, present := h[key] + + walkRows(join(path, key), want[key], hv, present, out) + } +} + +// walkRowsList compares a list under the same two shapes walkList knows: +// containerGroups, containers and environmentVars are keyed by name and +// matched by it, so a reordered live object moves no row, and everything +// else, a resourceBundles of plain strings or an autoscaling policy with no +// name to key on, is one leaf compared whole. +func walkRowsList(path string, want []any, have any, out *[]DiffRow) { + h, ok := have.([]any) + if !ok { + *out = append(*out, DiffRow{Path: path, Want: want, Have: have, Changed: true}) + + return + } + + if !nameKeyed(want) { + *out = append(*out, DiffRow{Path: path, Want: want, Have: h, Changed: !equal(want, h)}) + + return + } + + live := byName(h) + + for _, item := range want { + element, _ := item.(map[string]any) + name, _ := element["name"].(string) + at := fmt.Sprintf("%s[%s]", path, name) + + counterpart, found := live[name] + if !found { + *out = append(*out, DiffRow{Path: at, Want: element, Absent: true, Changed: true}) + + continue + } + + walkRowsMap(at, element, counterpart, out) + } +} + +// additionRows walks want with no live side to compare against, as a first +// deploy is: every leaf becomes an addition. Objects and name-keyed lists +// are walked through to their leaves, so the diff can show what will be +// created field by field; anything without a name to walk into, an unkeyed +// list or a scalar, is one row as it stands. +func additionRows(path string, want any, out *[]DiffRow) { + switch w := want.(type) { + case map[string]any: + for _, key := range slices.Sorted(maps.Keys(w)) { + additionRows(join(path, key), w[key], out) + } + case []any: + if !nameKeyed(w) { + *out = append(*out, DiffRow{Path: path, Want: w, Absent: true, Changed: true}) + + return + } + + for _, item := range w { + element, _ := item.(map[string]any) + name, _ := element["name"].(string) + at := fmt.Sprintf("%s[%s]", path, name) + + additionRows(at, element, out) + } + default: + *out = append(*out, DiffRow{Path: path, Want: want, Absent: true, Changed: true}) + } +} + // format renders a value for the plan. Composite values are summarised // rather than dumped: a plan is a summary, and a reader who wants the whole // object has the file open next to it. diff --git a/internal/workload/up/diff_test.go b/internal/workload/up/diff_test.go index 372a429c9..490f2cd90 100644 --- a/internal/workload/up/diff_test.go +++ b/internal/workload/up/diff_test.go @@ -481,3 +481,414 @@ func TestSubset_SiblingKeysDoNotShareStorage(t *testing.T) { assert.Equal(t, map[string]bool{"port": true, "cpu": true}, last, "each sibling keeps its own last segment") } + +// rowPaths is the set of walked paths, which is what most DiffRows cases +// care about. +func rowPaths(rows []DiffRow) []string { + out := make([]string, 0, len(rows)) + for _, r := range rows { + out = append(out, r.Path) + } + + return out +} + +// changedRows keeps the rows the two sides disagree about, which is exactly +// the set Subset reports. DiffRows is Subset's walk with the agreeing leaves +// kept, so the two must answer the same question with the same records. +func changedRows(rows []DiffRow) []DiffRow { + out := make([]DiffRow, 0, len(rows)) + for _, r := range rows { + if r.Changed { + out = append(out, r) + } + } + + return out +} + +// assertSameFindings pins the classification to Subset's: for the same two +// sides, the changed rows are the changes, one for one, values and all. A +// diff that disagreed with the plan about what differs would be worse than +// no diff at all. +func assertSameFindings(t *testing.T, want, have map[string]any) { + t.Helper() + + changes := Subset(want, have) + rows := changedRows(DiffRows(want, have)) + + require.Len(t, rows, len(changes)) + + for i, c := range changes { + assert.Equal(t, c.Path, rows[i].Path, "row %d path", i) + assert.Equal(t, c.Want, rows[i].Want, "row %d want", i) + assert.Equal(t, c.Have, rows[i].Have, "row %d have", i) + assert.Equal(t, c.Absent, rows[i].Absent, "row %d absent", i) + } +} + +// TestDiffRows_EmitsOneRowPerLeafChangedOrUnchanged is the difference from +// Subset: a diff needs the leaves that already agree as context, so every +// leaf of want gets a row, in sorted order, marked. +func TestDiffRows_EmitsOneRowPerLeafChangedOrUnchanged(t *testing.T) { + rows := DiffRows( + obj(t, `{"type":"service","port":9090,"probe":{"path":"/health","port":8000}}`), + obj(t, `{"type":"service","port":8080,"probe":{"path":"/health","port":8000}}`), + ) + + require.Len(t, rows, 4, "one row per leaf of want, not one per difference") + assert.Equal(t, []string{"port", "probe.path", "probe.port", "type"}, rowPaths(rows)) + + changed := rows[0] + + assert.True(t, changed.Changed) + assert.False(t, changed.Absent) + assert.InDelta(t, 9090.0, changed.Want, 0) + assert.InDelta(t, 8080.0, changed.Have, 0) + + for _, r := range rows[1:] { + assert.False(t, r.Changed, "leaf %s agrees, so it is context", r.Path) + } +} + +// TestDiffRows_ChangedRowsAgreeWithSubset runs the fixtures Subset's own +// tests use through both walkers: whatever Subset reports, DiffRows must +// mark changed, with the same path, values and absent flag. +func TestDiffRows_ChangedRowsAgreeWithSubset(t *testing.T) { + tests := []struct { + name string + want string + have string + }{ + { + name: "a moved scalar", + want: `{"port":9090}`, + have: `{"port":8080}`, + }, + { + name: "a field the live object lacks", + want: `{"readinessProbe":{"path":"/health"}}`, + have: `{}`, + }, + { + name: "an explicit null on the live side", + want: `{"entrypoint":["uvicorn"]}`, + have: `{"entrypoint":null}`, + }, + { + name: "a port inside a name-keyed list", + want: `{"containerGroups":[{"name":"default","containers":[{"name":"primary","port":9090}]}]}`, + have: `{"containerGroups":[{"name":"default","containers":[{"name":"primary","port":8080}]}]}`, + }, + { + name: "a whole element the live object lacks", + want: `{"containers":[{"name":"primary","port":8080},{"name":"sidecar","port":9000}]}`, + have: `{"containers":[{"name":"primary","port":8080}]}`, + }, + { + name: "an unkeyed list that differs", + want: `{"resourceBundles":["gpu.large"]}`, + have: `{"resourceBundles":["gpu.medium"]}`, + }, + { + name: "a type mismatch", + want: `{"probe":{"path":"/health"}}`, + have: `{"probe":"/health"}`, + }, + { + name: "a genuinely different memory size", + want: `{"resourceAllocation":{"memory":"1GB"}}`, + have: `{"resourceAllocation":{"memory":512000000.0}}`, + }, + { + name: "a rotated credential reference", + want: `{"containers":[{"name":"vllm-server","environmentVars":[ + {"name":"HUGGING_FACE_HUB_TOKEN","source":"dr-credential", + "drCredentialId":"aaaaaaaaaaaaaaaaaaaaaaaa","key":"apiToken"}]}]}`, + have: `{"containers":[{"name":"vllm-server","environmentVars":[ + {"name":"HF_HOME","value":"/tmp/hf"}, + {"name":"HUGGING_FACE_HUB_TOKEN","source":"dr-credential", + "drCredentialId":"66f1a2b3c4d5e6f7a8b9c0d1","key":"apiToken"}]}]}`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assertSameFindings(t, obj(t, tc.want), obj(t, tc.have)) + }) + } + + // And the real spec, where the file moved two things and addressed them + // by name rather than by position. + file := obj(t, `{ + "type": "service", + "containerGroups": [ + { + "name": "default", + "containers": [ + { + "name": "vllm-server", + "port": 8080, + "readinessProbe": {"path": "/ready", "port": 8000} + } + ] + } + ] + }`) + + assertSameFindings(t, file, obj(t, liveSpecJSON)) + + // A file that says less than the live object: every leaf it does name + // agrees, so the diff is all context and Subset is silent. + quiet := obj(t, `{ + "type": "service", + "containerGroups": [ + { + "name": "default", + "containers": [ + { + "name": "vllm-server", + "primary": true, + "port": 8000, + "imageBuildConfig": {"dockerfile": {"source": "provided"}}, + "readinessProbe": {"path": "/health", "port": 8000} + } + ] + } + ] + }`) + + assert.Empty(t, Subset(quiet, obj(t, liveSpecJSON))) + assert.Empty(t, changedRows(DiffRows(quiet, obj(t, liveSpecJSON))), + "nothing the file names differs, so the diff has no changes to show") +} + +// TestDiffRows_NameKeyedListsMatchByNameNotIndex is why the walk matches by +// name: the platform returns container groups in whatever order it likes, +// and a reordered live object must move no row and mark none changed. +func TestDiffRows_NameKeyedListsMatchByNameNotIndex(t *testing.T) { + want := obj(t, `{"containers":[ + {"name":"primary","port":8080}, + {"name":"sidecar","port":9000} + ]}`) + have := obj(t, `{"containers":[ + {"name":"sidecar","port":9000}, + {"name":"primary","port":8080} + ]}`) + sorted := obj(t, `{"containers":[ + {"name":"primary","port":8080}, + {"name":"sidecar","port":9000} + ]}`) + + reordered := DiffRows(want, have) + + assert.Equal(t, DiffRows(want, sorted), reordered, + "the live order of a name-keyed list moves no row") + + for _, r := range reordered { + assert.False(t, r.Changed, "reordering a name-keyed list is not drift") + } +} + +func TestDiffRows_NameKeyedPathUsesTheName(t *testing.T) { + rows := DiffRows( + obj(t, `{"containerGroups":[{"name":"default","containers":[{"name":"primary","port":9090}]}]}`), + obj(t, `{"containerGroups":[{"name":"default","containers":[{"name":"primary","port":8080}]}]}`), + ) + + assert.Equal(t, []string{ + "containerGroups[default].containers[primary].name", + "containerGroups[default].containers[primary].port", + "containerGroups[default].name", + }, rowPaths(rows)) + + changed := changedRows(rows) + + require.Len(t, changed, 1) + assert.Equal(t, "containerGroups[default].containers[primary].port", changed[0].Path) +} + +func TestDiffRows_NameKeyedElementMissingLiveIsOneAddition(t *testing.T) { + rows := DiffRows( + obj(t, `{"containers":[{"name":"primary","port":8080},{"name":"sidecar","port":9000}]}`), + obj(t, `{"containers":[{"name":"primary","port":8080}]}`), + ) + + assert.Equal(t, []string{ + "containers[primary].name", + "containers[primary].port", + "containers[sidecar]", + }, rowPaths(rows)) + + additions := changedRows(rows) + + require.Len(t, additions, 1, "a whole new container is one addition, not one per field") + assert.Equal(t, "containers[sidecar]", additions[0].Path) + assert.True(t, additions[0].Absent) +} + +// TestDiffRows_UnkeyedListsCompareWhole covers resourceBundles (plain +// strings) and autoscaling policies (objects with no name). Neither has +// anything to key on, so each list is one leaf and a partial match would +// mean nothing. +func TestDiffRows_UnkeyedListsCompareWhole(t *testing.T) { + unchanged := DiffRows( + obj(t, `{"resourceBundles":["gpu.medium"]}`), + obj(t, `{"resourceBundles":["gpu.medium"]}`), + ) + require.Len(t, unchanged, 1) + assert.False(t, unchanged[0].Changed) + + changed := DiffRows( + obj(t, `{"resourceBundles":["gpu.large"]}`), + obj(t, `{"resourceBundles":["gpu.medium"]}`), + ) + require.Len(t, changed, 1) + assert.True(t, changed[0].Changed) + + policies := DiffRows( + obj(t, `{"policies":[{"scalingMetric":"gpuCacheUtilization","target":80}]}`), + obj(t, `{"policies":[{"scalingMetric":"gpuCacheUtilization","target":70}]}`), + ) + require.Len(t, policies, 1, "an object with no name to key on is one leaf too") + assert.True(t, policies[0].Changed) +} + +func TestDiffRows_EmptyListsAreCompatible(t *testing.T) { + bothEmpty := DiffRows(obj(t, `{"containers":[]}`), obj(t, `{"containers":[]}`)) + require.Len(t, bothEmpty, 1) + assert.False(t, bothEmpty[0].Changed) + + liveHasMore := DiffRows(obj(t, `{"containers":[]}`), obj(t, `{"containers":[{"name":"primary"}]}`)) + require.Len(t, liveHasMore, 1, "an empty list has nothing to key on, so it is compared whole") + assert.True(t, liveHasMore[0].Changed) +} + +func TestDiffRows_TypeMismatchIsOneChange(t *testing.T) { + rows := DiffRows( + obj(t, `{"probe":{"path":"/health"}}`), + obj(t, `{"probe":"/health"}`), + ) + + require.Len(t, rows, 1, "an object asked of a scalar is one row, not one per field") + assert.Equal(t, "probe", rows[0].Path) + assert.True(t, rows[0].Changed) + assert.False(t, rows[0].Absent) + + listVsScalar := DiffRows( + obj(t, `{"entrypoint":["uvicorn","app:app"]}`), + obj(t, `{"entrypoint":"uvicorn app:app"}`), + ) + require.Len(t, listVsScalar, 1) + assert.True(t, listVsScalar[0].Changed) +} + +// TestDiffRows_ExplicitNullLiveIsAChangeNotAnAddition separates "the key is +// there holding null" from "the key is not there", which the walk tracks +// with its present flag. Both render differently in a diff. +func TestDiffRows_ExplicitNullLiveIsAChangeNotAnAddition(t *testing.T) { + rows := DiffRows( + obj(t, `{"entrypoint":["uvicorn"]}`), + obj(t, `{"entrypoint":null}`), + ) + + require.Len(t, rows, 1) + assert.False(t, rows[0].Absent, "the key is there holding null, which is not the same as missing") + assert.Nil(t, rows[0].Have) + assert.True(t, rows[0].Changed) +} + +func TestDiffRows_EmptyWantAsksForNothing(t *testing.T) { + assert.Empty(t, DiffRows(obj(t, `{}`), obj(t, `{"anything":1}`))) + assert.Empty(t, DiffRows(obj(t, `{"probe":{}}`), obj(t, `{"probe":{"path":"/health"}}`)), + "an object the file leaves empty has no leaves to walk") +} + +// TestDiffRows_MemoryIsTheSameSizeWrittenTwoWays is the diff-mode half of +// the memory tolerance: "512MB" in the file and 512000000 in the live +// workload are one sizing written twice, so the leaf reads as context and +// not as a change no deploy would settle. Reporting it as a change would +// mean every run showed permanent drift in a diff that never shrinks. +func TestDiffRows_MemoryIsTheSameSizeWrittenTwoWays(t *testing.T) { + fileString := DiffRows( + obj(t, `{"resourceAllocation":{"memory":"512MB"}}`), + obj(t, `{"resourceAllocation":{"memory":512000000.0}}`), + ) + require.Len(t, fileString, 1) + assert.False(t, fileString[0].Changed, "512MB and 512000000 are the same sizing") + + fileNumber := DiffRows( + obj(t, `{"resourceAllocation":{"memory":512000000.0}}`), + obj(t, `{"resourceAllocation":{"memory":"512MB"}}`), + ) + require.Len(t, fileNumber, 1) + assert.False(t, fileNumber[0].Changed, "the tolerance does not care which side spells it") +} + +func TestDiffRows_ADifferentMemoryIsStillDrift(t *testing.T) { + // 536870912 is 512MiB, a binary size: the platform reads 512MB as + // 512000000 bytes, so the two are genuinely different sizings. + rows := DiffRows( + obj(t, `{"resourceAllocation":{"memory":"512MB"}}`), + obj(t, `{"resourceAllocation":{"memory":536870912.0}}`), + ) + require.Len(t, rows, 1) + assert.True(t, rows[0].Changed, "512MB is not 536870912 bytes") + + spelled := DiffRows( + obj(t, `{"resourceAllocation":{"memory":"512MB"}}`), + obj(t, `{"resourceAllocation":{"memory":"1GB"}}`), + ) + require.Len(t, spelled, 1) + assert.True(t, spelled[0].Changed, "512MB is not 1GB") +} + +// TestDiffRows_NilHaveMeansEveryLeafIsAnAddition is the first deploy: there +// is no live object to agree with, so every leaf of want is an addition, +// walked out to its leaves so a diff can show what will be created rather +// than summarise it. +func TestDiffRows_NilHaveMeansEveryLeafIsAnAddition(t *testing.T) { + rows := DiffRows( + obj(t, `{"type":"service","port":8080,"probe":{"path":"/health"}, + "containers":[{"name":"primary","port":8080}], + "resourceBundles":["gpu.medium"]}`), + nil, + ) + + assert.Equal(t, []string{ + "containers[primary].name", + "containers[primary].port", + "port", + "probe.path", + "resourceBundles", + "type", + }, rowPaths(rows)) + + for _, r := range rows { + assert.True(t, r.Absent, "leaf %s has nothing live to agree with", r.Path) + assert.True(t, r.Changed, "leaf %s is an addition on a first deploy", r.Path) + assert.Nil(t, r.Have) + } +} + +// TestDiffRows_OrderIsStable keeps a diff from reading differently on two +// runs over the same inputs, which would make a diffed CI log useless. The +// inputs are parsed fresh each round: Go randomises map iteration order, and +// the rows must not care. +func TestDiffRows_OrderIsStable(t *testing.T) { + const ( + wantJSON = `{"zebra":1,"alpha":{"m":2,"a":3}, + "containerGroups":[{"name":"default","containers":[{"name":"primary","port":9090}]}]}` + + haveJSON = `{"zebra":0,"alpha":{"m":0,"a":0}, + "containerGroups":[{"name":"default","containers":[{"name":"primary","port":8080}]}]}` + ) + + first := DiffRows(obj(t, wantJSON), obj(t, haveJSON)) + require.NotEmpty(t, first) + + for range 25 { + assert.Equal(t, first, DiffRows(obj(t, wantJSON), obj(t, haveJSON)), + "the same inputs must read the same way twice") + } +} From a7aea39540fa42ef7baeb85771dc5086ffb1af48 Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Fri, 28 Aug 2026 16:29:09 -0700 Subject: [PATCH 03/12] [RAPTOR-19538] feat(workload): render the deploy plan as a unified diff RenderDiff writes the plan block for --diff: a changed leaf states both sides of itself as - old / + new, agreeing leaves are context that collapses past the three-line window, and nothing is truncated -- the default plan caps its detail list because it is a summary, a diff is the detail. Unmanaged live fields are counted once as "N fields not managed by this file" and never render as removals, and the verdict plus that count still print for an empty plan, where the default mode short-circuits. Build now computes the rows the diff draws: the spec half and the runtime half from the same walks that produce Artifact and Runtime, with the unchanged leaves kept as context; the create path walked against nil so a first deploy is all additions; and the two synthetic changes the walk cannot see (artifactId, artifact.type) merged in, because a change the default plan prints must not be invisible in the diff. Extra's unmanaged paths land on the plan deduplicated across the two halves, since one element unmanaged in both documents is one field to a reader. Redaction is the uidiff hook fed redacted(): no value behind an .environmentVars[ path prints in any line state, while the variable names stay visible. The state, lock, code and artifact entry lines render as action lines in their existing positions; the artifact entry stays because "a new version will be minted" is not visible in any field value. A first deploy gets a header saying so, the create summary line the default plan prints, and an all-additions body. Render itself is untouched; a captured-baseline regression test pins the default output byte for byte. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- internal/workload/up/plan.go | 104 +++++ internal/workload/up/plan_test.go | 148 +++++++ internal/workload/up/render.go | 185 +++++++- internal/workload/up/render_diff_test.go | 517 +++++++++++++++++++++++ 4 files changed, 953 insertions(+), 1 deletion(-) create mode 100644 internal/workload/up/render_diff_test.go diff --git a/internal/workload/up/plan.go b/internal/workload/up/plan.go index ba7ec9126..a7820b116 100644 --- a/internal/workload/up/plan.go +++ b/internal/workload/up/plan.go @@ -104,6 +104,22 @@ type Plan struct { // InheritsImage reports that the new version can take the running image. // What the plan intends, not a promise; the envelope is corrected after. InheritsImage bool + // DiffArtifact and DiffRuntime are the same two halves for the --diff + // rendering: every leaf the file names, changed or not, so a diff can + // draw context around its changes instead of listing only what moves. + // They come from the walks that produce Artifact and Runtime, so the two + // renderings of one plan cannot disagree about what differs. The changes + // below that no walk can produce -- the artifact id and type -- are + // merged into the rows as well, because a change the default plan prints + // must never be invisible in the diff. + DiffArtifact []DiffRow + DiffRuntime []DiffRow + + // Unmanaged lists the name-keyed elements the live object carries that + // the file never names, from Extra. The diff renders them as a count + // rather than as the removals they are not: the file leaving a field out + // is the file declining to manage it, not asking for it to be deleted. + Unmanaged []string // Locked reports that the version now serving is immutable. Its successor // has to be locked too before the platform will take it, so a deploy onto @@ -344,6 +360,16 @@ func Build(loaded Loaded, live Live, code CodeChange, opts Options) (Plan, error // Nothing exists to compare against, so every field is trivially an // addition. Saying so once is a plan; saying it per field is a wall. if plan.Creates { + artifactRows, runtimeRows, err := createRows(loaded) + if err != nil { + return Plan{}, err + } + + // The diff has no live side to draw context from either, so it walks + // the file against nil and shows what will be created, leaf by leaf. + plan.DiffArtifact = artifactRows + plan.DiffRuntime = runtimeRows + return plan, nil } @@ -360,6 +386,20 @@ func Build(loaded Loaded, live Live, code CodeChange, opts Options) (Plan, error plan.Artifact = Subset(spec, live.Spec) plan.Runtime = Subset(runtime, live.Runtime) + // The diff rows are the same two walks with the agreeing leaves kept, so + // the changes above and the context around them come from one comparison + // and cannot disagree about what differs. + plan.DiffArtifact = DiffRows(spec, live.Spec) + plan.DiffRuntime = DiffRows(runtime, live.Runtime) + + // A diff that stayed silent about what the live object carries that the + // file never names would read as an exhaustive account of the workload, + // so the paths land on the plan for the renderer to count. The two + // documents can hold the same unmanaged element -- a sidecar exists in + // the artifact's spec and in the workload's runtime -- and one element is + // one field to a reader, so the list holds each path once. + plan.Unmanaged = dedupePaths(Extra(spec, live.Spec), Extra(runtime, live.Runtime)) + // A file that names an artifact by id describes no spec to compare, so // the walk above has nothing to say about it. Pointing at a different // version than the one running is still the whole plan: it is a roll, and @@ -372,6 +412,13 @@ func Build(loaded Loaded, live Live, code CodeChange, opts Options) (Plan, error Have: live.ArtifactID, Want: bound, }) + + plan.DiffArtifact = append(plan.DiffArtifact, DiffRow{ + Path: keyArtifactID, + Have: live.ArtifactID, + Want: bound, + Changed: true, + }) } kind, err := loaded.ArtifactType() @@ -412,6 +459,13 @@ func Build(loaded Loaded, live Live, code CodeChange, opts Options) (Plan, error Have: running, Want: kind, }) + + plan.DiffArtifact = append(plan.DiffArtifact, DiffRow{ + Path: keyArtifactType, + Have: running, + Want: kind, + Changed: true, + }) } // Last: every drift has to be in hand before RebuildsImage can answer. @@ -419,3 +473,53 @@ func Build(loaded Loaded, live Live, code CodeChange, opts Options) (Plan, error return plan, nil } + +// createRows walks the file against nothing, which is what a create compares +// against: the spec half and the runtime half both come back as additions, +// one row per leaf, so a diff can show what will be created rather than +// summarise it. +func createRows(loaded Loaded) ([]DiffRow, []DiffRow, error) { + spec, err := loaded.Spec() + if err != nil { + return nil, nil, err + } + + runtime, err := loaded.Runtime() + if err != nil { + return nil, nil, err + } + + return DiffRows(spec, nil), DiffRows(runtime, nil), nil +} + +// dedupePaths merges the unmanaged path lists from the two halves of the +// plan, keeping the first sighting of each path. The lists each arrive in a +// deterministic order, and the same element can be reported by both -- a +// sidecar the file never names exists in the artifact's spec and in the +// workload's runtime alike -- so counting it twice would promise a reader +// two fields where there is one name to go look at. +func dedupePaths(lists ...[]string) []string { + var total int + + for _, list := range lists { + total += len(list) + } + + seen := make(map[string]struct{}, total) + + out := make([]string, 0, total) + + for _, list := range lists { + for _, path := range list { + if _, ok := seen[path]; ok { + continue + } + + seen[path] = struct{}{} + + out = append(out, path) + } + } + + return out +} diff --git a/internal/workload/up/plan_test.go b/internal/workload/up/plan_test.go index 11033becf..73dcbc74f 100644 --- a/internal/workload/up/plan_test.go +++ b/internal/workload/up/plan_test.go @@ -762,3 +762,151 @@ func allStates() []State { StateErrored, } } + +// TestBuild_DiffRowsMirrorTheArtifactRuntimeSplit: the diff rows come from +// the same two walks the change lists do, split the same way, so a diff and +// the plan beside it can never disagree about what differs or which block a +// finding belongs to. The agreeing leaves are kept as context, which is what +// makes the rows a diff rather than a second change list. +func TestBuild_DiffRowsMirrorTheArtifactRuntimeSplit(t *testing.T) { + drifted := `{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "spec": {"containerGroups": [ + {"name": "default", "containers": [{"name": "primary", "port": 9090}]} + ]}}, + "runtime": {"containerGroups": [{"name": "default", "replicaCount": 3}]} + }` + + plan, err := Build( + loadedFrom(drifted), + liveFrom(t, StateRunning, planLiveSpec, planLiveRuntime), + builtCode(0), + Options{}, + ) + require.NoError(t, err) + + assert.Equal(t, paths(plan.Artifact), rowPaths(changedRows(plan.DiffArtifact))) + assert.Equal(t, paths(plan.Runtime), rowPaths(changedRows(plan.DiffRuntime))) + + require.Greater(t, len(plan.DiffArtifact), len(plan.Artifact), "the spec rows keep the agreeing leaves") + + var port DiffRow + + for _, r := range plan.DiffArtifact { + if r.Path == "containerGroups[default].containers[primary].port" && !r.Changed { + port = r + } + } + + assert.False(t, port.Changed, "a leaf the file and the workload agree on is context") +} + +// TestBuild_CreatePathComputesAllAdditionRows: a create has no live side to +// compare against, so the diff walks the file against nil and every leaf is +// an addition, walked out per leaf. The default plan's halves stay empty, +// because a create says so once rather than per field. +func TestBuild_CreatePathComputesAllAdditionRows(t *testing.T) { + plan, err := Build( + loadedFrom(planPayload), + Live{State: StateUnbound}, + CodeChange{Applies: true, FirstDeploy: true}, + Options{}, + ) + require.NoError(t, err) + + require.NotEmpty(t, plan.DiffArtifact) + + for _, r := range plan.DiffArtifact { + assert.True(t, r.Absent, "row %s", r.Path) + assert.True(t, r.Changed, "row %s", r.Path) + assert.Nil(t, r.Have, "row %s", r.Path) + } + + require.NotEmpty(t, plan.DiffRuntime) + + for _, r := range plan.DiffRuntime { + assert.True(t, r.Absent, "row %s", r.Path) + assert.True(t, r.Changed, "row %s", r.Path) + } + + assert.Empty(t, plan.Unmanaged, "nothing is live, so nothing is unmanaged") + assert.Empty(t, plan.Artifact) + assert.Empty(t, plan.Runtime) +} + +// TestBuild_UnmanagedComesFromExtra: what the live object carries that the +// file never names lands on the plan as paths, so the diff can count it. +// The same element is unmanaged in both documents here -- the metrics +// sidecar exists in the artifact's spec and in the workload's runtime -- and +// one element is one field to a reader, so the list holds the path once. +func TestBuild_UnmanagedComesFromExtra(t *testing.T) { + plan, err := Build( + loadedFrom(planPayload), + liveFrom(t, StateRunning, planLiveSpec, planLiveRuntime), + builtCode(0), + Options{}, + ) + require.NoError(t, err) + + assert.Equal(t, + []string{"containerGroups[default].containers[metrics]"}, + plan.Unmanaged) +} + +// TestBuild_SyntheticArtifactIDChangeEntersTheDiffRows: the artifact id is +// not a leaf of the spec, so no walk of it can produce the change. It is +// merged into the rows all the same, because a change the default plan +// prints must not be invisible in the diff. +func TestBuild_SyntheticArtifactIDChangeEntersTheDiffRows(t *testing.T) { + loaded := Loaded{Compiled: &manifest.Compiled{ + Payload: json.RawMessage(`{"name": "my-app", "artifactId": "68b0bbbb0000000000000002"}`), + ArtifactID: "68b0bbbb0000000000000002", + }} + + live := liveFrom(t, StateRunning, "", planLiveRuntime) + live.ArtifactID = "68a0000000000000000000a1" + + plan, err := Build(loaded, live, builtCode(0), Options{}) + require.NoError(t, err) + + changed := changedRows(plan.DiffArtifact) + + require.Len(t, changed, 1) + assert.Equal(t, "artifactId", changed[0].Path) + assert.Equal(t, "68a0000000000000000000a1", changed[0].Have) + assert.Equal(t, "68b0bbbb0000000000000002", changed[0].Want) + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "- artifactId: 68a0000000000000000000a1") + assert.Contains(t, out, "+ artifactId: 68b0bbbb0000000000000002") +} + +// The type sits beside the spec for the same reason, and lands in the rows +// the same way, with the defaulted live value on the left of the pair. +func TestBuild_SyntheticArtifactTypeChangeEntersTheDiffRows(t *testing.T) { + loaded := Loaded{Compiled: &manifest.Compiled{ + Payload: json.RawMessage(`{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "type": "agent", "spec": {}} + }`), + }} + + live := liveFrom(t, StateRunning, "", planLiveRuntime) + live.ArtifactType = "service" + + plan, err := Build(loaded, live, builtCode(0), Options{}) + require.NoError(t, err) + + changed := changedRows(plan.DiffArtifact) + + require.Len(t, changed, 1) + assert.Equal(t, "artifact.type", changed[0].Path) + assert.Equal(t, "service", changed[0].Have) + assert.Equal(t, "agent", changed[0].Want) + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "- artifact.type: service") + assert.Contains(t, out, "+ artifact.type: agent") +} diff --git a/internal/workload/up/render.go b/internal/workload/up/render.go index 3dadfaa6d..f28f6086c 100644 --- a/internal/workload/up/render.go +++ b/internal/workload/up/render.go @@ -20,6 +20,7 @@ import ( "strings" "github.com/charmbracelet/lipgloss" + "github.com/datarobot/cli/internal/uidiff" "github.com/datarobot/cli/internal/workload" "github.com/datarobot/cli/internal/workload/manifest" "github.com/datarobot/cli/tui" @@ -87,6 +88,175 @@ func Render(w io.Writer, s Summary, plan Plan) error { return err } +// RenderDiff writes the plan block as a unified diff, for `up --diff`. +// +// Where Render summarises, this lays the plan out leaf by leaf: a changed +// field states both sides of itself, `- old` then `+ new`; an agreeing field +// is context that collapses once it runs past the window; and what the live +// object carries that the file never names is counted once, because none of +// it is a removal. The default plan caps its detail list because a plan is a +// summary, and the file is a better place to read the rest; a diff is the +// detail, so nothing here is capped. +func RenderDiff(w io.Writer, s Summary, plan Plan) error { + var b strings.Builder + + if head := diffHeader(s, plan); head != "" { + b.WriteString(planTitleStyle.Render(head)) + b.WriteString("\n") + } + + if plan.Empty() { + // The three-state rule is the point of --diff, so an empty plan does + // not skip the block the way Render's does: what the live object + // carries that the file never names still gets said. The verdict is + // the line the default mode prints, because the run's outcome, and + // its exit code, are the same run's. + b.WriteString("\n" + settledVerdict(plan) + "\n") + + if note := unmanagedNote(plan.Unmanaged); note != "" { + b.WriteString(note) + b.WriteString("\n") + } + + _, err := io.WriteString(w, b.String()) + + return err + } + + b.WriteString("\n") + + for _, line := range diffActionLines(s, plan) { + b.WriteString(line) + b.WriteString("\n") + } + + if err := uidiff.Render(&b, diffRows(plan), uidiff.Options{Redact: redacted}); err != nil { + return err + } + + if note := unmanagedNote(plan.Unmanaged); note != "" { + b.WriteString(note) + b.WriteString("\n") + } + + _, err := io.WriteString(w, b.String()) + + return err +} + +// diffActionLines are the lines a diff keeps from the default plan: the +// reasons the run will act. None of them is a field change, so each keeps +// the entry form and position the default plan gives it -- rendered as a +// hunk, a stopped workload or a locked version would read as an edit to a +// field nobody wrote. +func diffActionLines(s Summary, plan Plan) []string { + var out []string + + if plan.Creates { + out = append(out, entry("+", "workload", createDetail(s, plan))) + } + + if line := stateLine(plan.State, s.Status); line != "" { + out = append(out, line) + } + + if plan.Code.Changed() { + out = append(out, entry("~", "code", codeDetail(plan.Code))) + } + + out = append(out, lockLines(plan)...) + out = append(out, artifactEntry(plan)...) + + return out +} + +// diffHeader names the plan's subject. A live workload is named exactly as +// the default plan names it. A first deploy has no live side to name, and +// above a diff of the file itself the default plan's silence would leave the +// block unheaded, so the header states the subject instead. A create that +// replaces a dead binding is not a first deploy and still gets no header: +// the create line is the warning, as it is in the default mode. +func diffHeader(s Summary, plan Plan) string { + if !plan.Creates { + return header(s, plan) + } + + if plan.PriorWorkloadID != "" { + return "" + } + + name := s.Name + if name == "" { + name = "workload" + } + + return name + ", first deploy" +} + +// diffRows flattens the plan's two row halves into the order the diff draws +// them, the artifact spec first and the runtime sizing second, one diff +// either way: they are one deploy and the reader reviews them together. +func diffRows(plan Plan) []uidiff.Row { + rows := make([]uidiff.Row, 0, len(plan.DiffArtifact)+len(plan.DiffRuntime)) + + rows = appendLeafRows(rows, plan.DiffArtifact) + rows = appendLeafRows(rows, plan.DiffRuntime) + + return rows +} + +// appendLeafRows turns whole-leaf rows into diff lines. A changed leaf +// becomes two lines, the value it replaces and the value it becomes, because +// that is what a unified diff states; an addition is one line, and an +// agreeing leaf is context. Redaction is left to the uidiff hook, which +// rewrites the whole line for a path it refuses, so a value formatted into +// the text here is never what prints. +func appendLeafRows(rows []uidiff.Row, leaves []DiffRow) []uidiff.Row { + for _, leaf := range leaves { + switch { + case !leaf.Changed: + rows = append(rows, uidiff.Row{ + Kind: uidiff.Context, + Path: leaf.Path, + Text: leafText(leaf.Path, leaf.Want), + }) + case leaf.Absent: + rows = append(rows, uidiff.Row{ + Kind: uidiff.Add, + Path: leaf.Path, + Text: leafText(leaf.Path, leaf.Want), + }) + default: + rows = append(rows, + uidiff.Row{Kind: uidiff.Del, Path: leaf.Path, Text: leafText(leaf.Path, leaf.Have)}, + uidiff.Row{Kind: uidiff.Add, Path: leaf.Path, Text: leafText(leaf.Path, leaf.Want)}, + ) + } + } + + return rows +} + +// leafText renders one leaf the way the plan's detail lines spell a value, +// reusing format so a composite still reads as a summary rather than a dump. +func leafText(path string, v any) string { + return path + ": " + format(v) +} + +// unmanagedNote is the diff's whole account of the live object's unmanaged +// fields, one counted line rather than a marking per field. Each such field +// is live state the file declines to manage, so none of them is a removal, +// and interleaving them with the rows would put them next to edits they have +// nothing to do with. +func unmanagedNote(paths []string) string { + if len(paths) == 0 { + return "" + } + + return tui.HintStyle.Render(fmt.Sprintf("%d %s not managed by this file", + len(paths), plural(len(paths), "field", "fields"))) +} + // settledVerdict is what an empty plan means, which is not always that there // is nothing to do. // @@ -329,6 +499,19 @@ func lockLines(plan Plan) []string { // A version that keeps the running image says so: --dry-run is the whole of the // review a deploy gets. func artifactLines(plan Plan) []string { + head := artifactEntry(plan) + if head == nil { + return nil + } + + return append(head, details(plan.Artifact)...) +} + +// artifactEntry is the artifact line without its detail list, which --diff +// replaces with the full rows underneath. It stays in diff mode because the +// one fact it carries, that a new version will be minted, is not visible in +// any field value. +func artifactEntry(plan Plan) []string { if !plan.RollsArtifact() { return nil } @@ -343,7 +526,7 @@ func artifactLines(plan Plan) []string { reason += "; keeps the running image, so no rebuild" } - return append([]string{entry("+", "artifact", reason)}, details(plan.Artifact)...) + return []string{entry("+", "artifact", reason)} } // runtimeLines describes a sizing change, which needs no new version. A diff --git a/internal/workload/up/render_diff_test.go b/internal/workload/up/render_diff_test.go new file mode 100644 index 000000000..150789145 --- /dev/null +++ b/internal/workload/up/render_diff_test.go @@ -0,0 +1,517 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package up + +import ( + "fmt" + "strings" + "testing" + + "github.com/datarobot/cli/internal/workload/manifest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// renderDiff is the diff-mode twin of render: it runs one plan through +// RenderDiff and hands back the bytes, so the tests below can assert on +// output the way render_test.go does. +func renderDiff(t *testing.T, s Summary, plan Plan) string { + t.Helper() + + var b strings.Builder + + require.NoError(t, RenderDiff(&b, s, plan)) + + return b.String() +} + +// TestRenderDiff_SizingChangeIsAUnifiedDiff is the shape the flag exists for: +// a moved value states both sides of itself, with the file's value leaving +// and the manifest's value arriving, where the default plan would print one +// "have -> want" line. +func TestRenderDiff_SizingChangeIsAUnifiedDiff(t *testing.T) { + plan := Plan{ + State: StateRunning, + Runtime: []Change{ + {Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}, + }, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "- containerGroups[default].replicaCount: 1") + assert.Contains(t, out, "+ containerGroups[default].replicaCount: 3") +} + +// TestRenderDiff_CollapsesUnchangedRunsBeyondTheWindow pins the git +// convention the renderer inherits from uidiff: agreeing leaves stay visible +// within three lines of a change and collapse into a counted summary beyond +// it, so a big spec still renders shorter than itself without ever hiding a +// change. +func TestRenderDiff_CollapsesUnchangedRunsBeyondTheWindow(t *testing.T) { + rows := make([]DiffRow, 0, 13) + + for i := 1; i <= 6; i++ { + rows = append(rows, DiffRow{Path: fmt.Sprintf("spec.leaf%02d", i), Want: float64(i), Have: float64(i)}) + } + + rows = append(rows, DiffRow{Path: "spec.port", Want: 9090.0, Have: 8080.0, Changed: true}) + + for i := 7; i <= 12; i++ { + rows = append(rows, DiffRow{Path: fmt.Sprintf("spec.leaf%02d", i), Want: float64(i), Have: float64(i)}) + } + + // The Change list rides along because Build always fills it beside the + // rows, and plan.Empty() reads it rather than the rows: the diff's + // verdict has to mean what the run's exit code means. + out := renderDiff(t, appSummary, Plan{ + State: StateRunning, + Artifact: []Change{{Path: "spec.port", Have: 8080.0, Want: 9090.0}}, + DiffArtifact: rows, + }) + + // Three unchanged leaves on the far side of each window edge hide; the + // summary counts them out loud rather than leaving the reader to guess. + assert.Equal(t, 2, strings.Count(out, "... 3 identical lines")) + assert.NotContains(t, out, "spec.leaf01", "beyond the window, context collapses") + assert.NotContains(t, out, "spec.leaf03") + assert.Contains(t, out, "spec.leaf04", "the row at the window edge stays") + assert.Contains(t, out, "spec.leaf09", "the last row inside the far window stays") + assert.NotContains(t, out, "spec.leaf10") +} + +// TestRenderDiff_NeverTruncates is the other half of what --diff is bought +// for: the default plan caps its detail list at detailLimit and counts the +// rest, because a plan is a summary. A diff is the detail, so every changed +// leaf appears no matter how many there are. +func TestRenderDiff_NeverTruncates(t *testing.T) { + rows := make([]DiffRow, 0, 10) + + changes := make([]Change, 0, 10) + + for _, name := range []string{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j"} { + rows = append(rows, DiffRow{Path: "spec." + name, Want: 2.0, Have: 1.0, Changed: true}) + + changes = append(changes, Change{Path: "spec." + name, Have: 1.0, Want: 2.0}) + } + + out := renderDiff(t, appSummary, Plan{State: StateRunning, Artifact: changes, DiffArtifact: rows}) + + assert.Contains(t, out, "- spec.j: 1") + assert.Contains(t, out, "+ spec.j: 2") + assert.NotContains(t, out, "more", "nothing is dropped, so nothing is counted out loud") +} + +// TestRenderDiff_NeverPrintsEnvironmentVariableValues is the hard rule, run +// across all three row states at once: a literal that moved, a literal that +// agrees, and a credential reference that rotated. The values behind any +// `.environmentVars[` path stay out of the output entirely -- additions, +// removals and context alike -- while the variable names stay visible, +// because the names are what tell the reader what changed. +func TestRenderDiff_NeverPrintsEnvironmentVariableValues(t *testing.T) { + const ( + literalOld = "sk-live-abc123" + literalNew = "sk-live-xyz789" + literalSame = "hunter2" + credOld = "66f1a2b3c4d5e6f7a8b9c0d1" + credNew = "aaaaaaaaaaaaaaaaaaaaaaaa" + ) + + plan := Plan{ + State: StateRunning, + Artifact: []Change{ + { + Path: "containerGroups[default].containers[primary].environmentVars[OPENAI_API_KEY].value", + Have: literalOld, + Want: literalNew, + }, + { + Path: "containerGroups[default].containers[primary].environmentVars[HUGGING_FACE_HUB_TOKEN].drCredentialId", + Have: credOld, + Want: credNew, + }, + }, + DiffArtifact: []DiffRow{ + { + Path: "containerGroups[default].containers[primary].environmentVars[OPENAI_API_KEY].value", + Have: literalOld, + Want: literalNew, + Changed: true, + }, + { + Path: "containerGroups[default].containers[primary].environmentVars[PLAINTEXT].value", + Have: literalSame, + Want: literalSame, + }, + { + Path: "containerGroups[default].containers[primary].environmentVars[HUGGING_FACE_HUB_TOKEN].drCredentialId", + Have: credOld, + Want: credNew, + Changed: true, + }, + { + Path: "containerGroups[default].containers[primary].environmentVars[FOO].name", + Have: "FOO", + Want: "FOO", + }, + }, + } + + out := renderDiff(t, appSummary, plan) + + for _, secret := range []string{literalOld, literalNew, literalSame, credOld, credNew} { + assert.NotContains(t, out, secret) + } + + assert.Contains(t, out, "environmentVars[OPENAI_API_KEY].value: changed") + assert.Contains(t, out, "environmentVars[OPENAI_API_KEY].value: set") + assert.Contains(t, out, "environmentVars[PLAINTEXT].value: (redacted)") + assert.Contains(t, out, "environmentVars[HUGGING_FACE_HUB_TOKEN].drCredentialId: changed") + assert.Contains(t, out, "environmentVars[FOO].name: (redacted)", + "a name leaf inside the block sits on a redacted path, so its value is withheld with the rest") +} + +// TestRenderDiff_NameLeavesRenderAsContext records the render-time choice +// about the name leaves DiffRows emits mechanically. They stay: inside a +// container's run of rows, the name leaf is the only line that says which +// element the surrounding lines belong to, and dropping it would leave a +// two-container diff ambiguous. They are context, never changes. +func TestRenderDiff_NameLeavesRenderAsContext(t *testing.T) { + plan := Plan{ + State: StateRunning, + Artifact: []Change{ + {Path: "containerGroups[default].containers[primary].port", Have: 8080.0, Want: 9090.0}, + }, + DiffArtifact: []DiffRow{ + {Path: "containerGroups[default].containers[primary].name", Have: "primary", Want: "primary"}, + {Path: "containerGroups[default].containers[primary].port", Have: 8080.0, Want: 9090.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "containerGroups[default].containers[primary].name: primary") + assert.NotContains(t, out, "- containerGroups[default].containers[primary].name") +} + +// TestRenderDiff_MultiBlockChangesRenderInOneDiff: a plan that moves the +// artifact spec and the runtime sizing renders both halves in one diff, +// because they are one deploy and the reader reviews them together. +func TestRenderDiff_MultiBlockChangesRenderInOneDiff(t *testing.T) { + drifted := `{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "spec": { + "type": "service", + "containerGroups": [{"name": "default", "containers": [ + {"name": "primary", "primary": true, "port": 9090, + "readinessProbe": {"path": "/health", "port": 8080}} + ]}] + }}, + "runtime": {"containerGroups": [ + {"name": "default", "replicaCount": 3, + "containers": [{"name": "primary", "resourceAllocation": {"cpu": 0.5, "memory": "512MB"}}]} + ]} + }` + + plan, err := Build( + loadedFrom(drifted), + liveFrom(t, StateRunning, planLiveSpec, planLiveRuntime), + builtCode(0), + Options{}, + ) + require.NoError(t, err) + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "- containerGroups[default].containers[primary].port: 8080") + assert.Contains(t, out, "+ containerGroups[default].containers[primary].port: 9090") + assert.Contains(t, out, "- containerGroups[default].replicaCount: 1") + assert.Contains(t, out, "+ containerGroups[default].replicaCount: 3") +} + +// TestRenderDiff_AlreadyUpToDateIsTheDefaultVerdict: an empty plan with +// nothing unmanaged has nothing for a diff to draw, so --diff prints exactly +// what the default mode prints. The run's outcome, and its exit code, are +// the same run's. +func TestRenderDiff_AlreadyUpToDateIsTheDefaultVerdict(t *testing.T) { + plan := Plan{State: StateRunning, Code: builtCode(0)} + + out := renderDiff(t, appSummary, plan) + + assert.Equal(t, render(t, appSummary, plan), out) + assert.Contains(t, out, "โœ“ Already up to date") +} + +// TestRenderDiff_StateLinesRenderAsActionLines: stopped, errored and +// terminated are reasons the run will act, not field changes, so they keep +// the entry form and position the default plan gives them. Inside a diff +// they must not read as edits to fields nobody wrote. +func TestRenderDiff_StateLinesRenderAsActionLines(t *testing.T) { + tests := []struct { + name string + plan Plan + want string + }{ + {"stopped", Plan{State: StateStopped, Code: builtCode(0)}, "~ workload"}, + {"errored", Plan{State: StateErrored}, "! workload"}, + {"terminated", Plan{State: StateTerminated}, "! workload"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + out := renderDiff(t, appSummary, tc.plan) + + assert.Contains(t, out, " "+tc.want, "an entry line, indented like the default plan's") + assert.NotContains(t, out, "\n- "+tc.want) + assert.NotContains(t, out, "\n+ "+tc.want) + }) + } +} + +// The state line survives alongside the field rows, and the rows still +// render as rows: the start and the sizing move in the same diff because +// they happen in the same deploy. +func TestRenderDiff_StoppedWorkloadRendersStartAndRowsTogether(t *testing.T) { + plan := Plan{ + State: StateStopped, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "started, having been stopped") + assert.Contains(t, out, "- containerGroups[default].replicaCount: 1") + assert.Contains(t, out, "+ containerGroups[default].replicaCount: 3") +} + +// TestRenderDiff_LockLineRendersAsAnActionLine: locking is a consequence of +// the deploy, not an edit to a field, so the ~ entry keeps the form and +// position the default plan gives it and never becomes a hunk. +func TestRenderDiff_LockLineRendersAsAnActionLine(t *testing.T) { + plan := Plan{ + State: StateRunning, + Locked: true, + Code: builtCode(1), + DiffArtifact: []DiffRow{ + {Path: "port", Want: 9090.0, Have: 8080.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "~ lock") + assert.Contains(t, out, "locked to match") + assert.NotContains(t, out, "- lock") + assert.NotContains(t, out, "+ lock") +} + +// TestRenderDiff_UnmanagedNeverBecomesARemoval is the three-state rule's +// sharp edge. What the live object carries that the file never names renders +// once, as a counted summary; it must never appear as a `-` line, because +// the file leaving a field out is the file declining to manage it, not +// asking for it to be deleted. +func TestRenderDiff_UnmanagedNeverBecomesARemoval(t *testing.T) { + plan := Plan{ + State: StateRunning, + Runtime: []Change{ + {Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}, + }, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + Unmanaged: []string{ + "containerGroups[default].containers[metrics]", + "containerGroups[default].containers[primary].environmentVars[FOO]", + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "2 fields not managed by this file") + assert.NotContains(t, out, "- containerGroups[default].containers[metrics]") + assert.NotContains(t, out, "- containerGroups[default].containers[primary].environmentVars[FOO]") + assert.Equal(t, 1, strings.Count(out, "not managed by this file"), + "the summary is stated once, never once per field") +} + +// TestRenderDiff_UnmanagedSummarySaysFieldOnce: the count goes through the +// same plural helper as the rest of the plan, so a lone extra does not read +// like a census. +func TestRenderDiff_UnmanagedSummarySaysFieldOnce(t *testing.T) { + plan := Plan{ + State: StateRunning, + Code: builtCode(0), + Unmanaged: []string{"containerGroups[default].containers[metrics]"}, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "1 field not managed by this file") +} + +// TestRenderDiff_EmptyPlanWithUnmanagedSurfacesThem is where --diff +// deliberately diverges from the default mode: Render short-circuits an +// empty plan and never mentions unmanaged fields, while the diff still says +// what the live object carries that the file never names. The verdict, and +// the exit code behind it, are unchanged. +func TestRenderDiff_EmptyPlanWithUnmanagedSurfacesThem(t *testing.T) { + plan := Plan{ + State: StateRunning, + Code: builtCode(0), + Unmanaged: []string{ + "containerGroups[default].containers[metrics]", + "containerGroups[default].containers[primary].environmentVars[FOO]", + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "โœ“ Already up to date") + assert.Contains(t, out, "2 fields not managed by this file") + assert.NotContains(t, out, "\n- ") + assert.NotContains(t, out, "\n+ ", "no managed field changed, so no change lines exist") +} + +// TestRenderDiff_FirstDeployIsAllAdditions: with no live side to compare +// against, the diff's subject is the compiled manifest itself. A header says +// so, the create summary line the default plan prints still leads, and every +// leaf of the file renders as an addition -- the diff must not be less +// informative than the plan it replaces. +func TestRenderDiff_FirstDeployIsAllAdditions(t *testing.T) { + plan, err := Build( + loadedFrom(planPayload), + Live{State: StateUnbound}, + CodeChange{Applies: true, FirstDeploy: true}, + Options{}, + ) + require.NoError(t, err) + + out := renderDiff(t, Summary{Name: "my-app"}, plan) + + assert.Contains(t, out, "my-app, first deploy") + assert.Contains(t, out, "+ workload my-app will be created, with its first artifact") + assert.Contains(t, out, "~ code all project files, uploaded for the first time") + assert.Contains(t, out, "+ containerGroups[default].containers[primary].port: 8080") + assert.Contains(t, out, "+ containerGroups[default].replicaCount: 1") + assert.NotContains(t, out, "\n- ", "there is nothing live to remove anything from") +} + +// A create that replaces a dead binding is not a first deploy, and the +// header must not claim it is: the create line's dead-binding warning is the +// whole point of that plan, exactly as in the default mode. +func TestRenderDiff_RecreateDoesNotClaimFirstDeploy(t *testing.T) { + plan, err := Build( + loadedFrom(planPayload), + Live{Live: manifest.Live{WorkloadID: "68b0c1d2e3f4a5b6c7d8e9f0"}, State: StateMissing}, + builtCode(0), + Options{}, + ) + require.NoError(t, err) + + out := renderDiff(t, Summary{Name: "my-app"}, plan) + + assert.Contains(t, out, "+ workload my-app will be created: .datarobot.yaml is bound to 68b0c1d2, "+ + "which no longer exists") + assert.NotContains(t, out, "first deploy") +} + +// TestRenderDiff_ArtifactEntryAnnouncesTheRoll: the one fact no field value +// carries is that a new version will be minted and rolled. The default +// plan's artifact entry keeps its place above the diff; its capped detail +// list is what the rows below it replace. +func TestRenderDiff_ArtifactEntryAnnouncesTheRoll(t *testing.T) { + plan := Plan{ + State: StateRunning, + Code: builtCode(0), + DiffArtifact: []DiffRow{ + {Path: "containerGroups[default].containers[primary].port", Want: 9090.0, Have: 8080.0, Changed: true}, + }, + } + + plan.Artifact = []Change{{Path: "containerGroups[default].containers[primary].port", Have: 8080.0, Want: 9090.0}} + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "+ artifact new version, 1 spec change") + assert.NotContains(t, out, " containerGroups[default].containers[primary].port", + "the capped detail list is replaced by the diff, not repeated under it") +} + +// TestRenderDiff_CodeLineKeepsItsPositionUntilTheFileListLands: code drift +// is measured upstream of the renderer, so until the sync file list replaces +// it the bare count renders as the action line it already is. +func TestRenderDiff_CodeLineKeepsItsPositionUntilTheFileListLands(t *testing.T) { + plan := Plan{ + State: StateRunning, + Code: builtCode(14), + DiffArtifact: []DiffRow{ + {Path: "port", Want: 9090.0, Have: 8080.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "~ code 14 files changed since the last deploy") +} + +// TestRender_DefaultModeBaselineIsUnchanged pins the default plan's exact +// bytes against a baseline captured before --diff existed. The renderer grew +// fields that carry the diff rows, and this is the proof that Render neither +// reads them nor moved: same header, same capped detail list with its +// "and 1 more", same line order, byte for byte. +func TestRender_DefaultModeBaselineIsUnchanged(t *testing.T) { + changes := make([]Change, 0, 7) + + for _, name := range []string{"a", "b", "c", "d", "e", "f", "g"} { + changes = append(changes, Change{Path: "spec." + name, Have: 1.0, Want: 2.0}) + } + + plan := Plan{ + State: StateRunning, + Code: builtCode(14), + Locked: true, + Artifact: changes, + Runtime: []Change{{Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}}, + DiffArtifact: []DiffRow{ + {Path: "spec.a", Want: 2.0, Have: 1.0, Changed: true}, + }, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + Unmanaged: []string{"containerGroups[default].containers[metrics]"}, + } + + assert.Equal(t, + "my-app (68b0c1d2), running\n"+ + "\n"+ + " ~ code 14 files changed since the last deploy\n"+ + " ~ lock the running version is locked, so a new one is created and locked to match. "+ + "Locking is permanent\n"+ + " + artifact new version, 7 spec changes\n"+ + " spec.a: 1 -> 2\n"+ + " spec.b: 1 -> 2\n"+ + " spec.c: 1 -> 2\n"+ + " spec.d: 1 -> 2\n"+ + " spec.e: 1 -> 2\n"+ + " spec.f: 1 -> 2\n"+ + " and 1 more\n"+ + " ~ runtime containerGroups[default].replicaCount: 1 -> 3\n", + render(t, appSummary, plan)) +} From 6131b5656add5f3cb8f6d757a0f6650917328ac0 Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Fri, 28 Aug 2026 16:35:14 -0700 Subject: [PATCH 04/12] [RAPTOR-19538] feat(workload): show the sync file list for --diff code drift Code drift in --diff mode printed the same bare "~ code N files changed" count the default plan prints, which is exactly the summary a diff exists to replace. CodeChange now carries the dry-run SyncPlan that defaultCodeChange already computed and threw away, and RenderDiff feeds it to the sync command's own display.PrintPlan -- the renderer `dr artifact code sync --dry-run` uses -- so both commands describe one upload with one format, and the diff names the files instead of counting them. The list keeps the code block's position, ahead of the lock and artifact lines. A first deploy has no plan to list, because nothing was ever uploaded to compare against, so it keeps the all-files wording. A plan measured without its list, which only a test harness wiring the count alone can produce, falls back to the count rather than going silent about drift. Render is untouched: the default mode keeps the bare count byte for byte. The seam is unchanged -- codeChangeFn still hands back a CodeChange, and the plan is data rather than engine state, so it survives the Close that releases the project lock. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- internal/workload/up/plan.go | 10 ++++ internal/workload/up/render.go | 43 +++++++++++++--- internal/workload/up/render_diff_test.go | 64 ++++++++++++++++++++++-- internal/workload/up/run.go | 9 +++- 4 files changed, 114 insertions(+), 12 deletions(-) diff --git a/internal/workload/up/plan.go b/internal/workload/up/plan.go index a7820b116..aa076ac40 100644 --- a/internal/workload/up/plan.go +++ b/internal/workload/up/plan.go @@ -18,6 +18,7 @@ import ( "slices" "github.com/datarobot/cli/internal/workload/manifest" + "github.com/datarobot/cli/internal/workload/sync" ) // Actions are what a run will do, and what the JSON envelope reports. A run @@ -64,6 +65,15 @@ type CodeChange struct { // the link onto it, which the plan says out loud because nothing in the // file asked for it. LinkLocked bool + + // SyncPlan is the dry-run plan the sync engine measured this tree with, + // the same one Files counts. --diff renders it through the sync command's + // own plan printer, so the two commands describe an upload with one + // format instead of two. It is plain data rather than engine state -- the + // lock is released right after measuring -- so carrying it past the + // engine's Close costs nothing. Nil on a first deploy, for a manifest + // that names an image, and when a test harness wires only the count. + SyncPlan *sync.SyncPlan } // Changed reports whether the code needs syncing and rebuilding. diff --git a/internal/workload/up/render.go b/internal/workload/up/render.go index f28f6086c..7ced8716e 100644 --- a/internal/workload/up/render.go +++ b/internal/workload/up/render.go @@ -23,6 +23,7 @@ import ( "github.com/datarobot/cli/internal/uidiff" "github.com/datarobot/cli/internal/workload" "github.com/datarobot/cli/internal/workload/manifest" + "github.com/datarobot/cli/internal/workload/sync/display" "github.com/datarobot/cli/tui" ) @@ -125,7 +126,12 @@ func RenderDiff(w io.Writer, s Summary, plan Plan) error { b.WriteString("\n") - for _, line := range diffActionLines(s, plan) { + actions, err := diffActionLines(s, plan) + if err != nil { + return err + } + + for _, line := range actions { b.WriteString(line) b.WriteString("\n") } @@ -139,7 +145,7 @@ func RenderDiff(w io.Writer, s Summary, plan Plan) error { b.WriteString("\n") } - _, err := io.WriteString(w, b.String()) + _, err = io.WriteString(w, b.String()) return err } @@ -149,7 +155,7 @@ func RenderDiff(w io.Writer, s Summary, plan Plan) error { // the entry form and position the default plan gives it -- rendered as a // hunk, a stopped workload or a locked version would read as an edit to a // field nobody wrote. -func diffActionLines(s Summary, plan Plan) []string { +func diffActionLines(s Summary, plan Plan) ([]string, error) { var out []string if plan.Creates { @@ -160,14 +166,39 @@ func diffActionLines(s Summary, plan Plan) []string { out = append(out, line) } - if plan.Code.Changed() { - out = append(out, entry("~", "code", codeDetail(plan.Code))) + code, err := codeBlock(plan.Code) + if err != nil { + return nil, err } + out = append(out, code...) out = append(out, lockLines(plan)...) out = append(out, artifactEntry(plan)...) - return out + return out, nil +} + +// codeBlock is the diff's code section. A first deploy has no sync plan to +// list, because nothing was ever uploaded to compare the tree against, so it +// keeps the default plan's wording. Otherwise the section is the file list +// the sync would upload, drawn by the sync command's own plan printer and fed +// the dry-run plan the code-change seam carried: two formats for one upload +// would eventually disagree, which is the whole reason the list is borrowed +// rather than redrawn. A plan measured without its list -- no production path +// makes one, but a harness wiring only the count does -- falls back to the +// count rather than printing nothing about real drift. +func codeBlock(code CodeChange) ([]string, error) { + if code.FirstDeploy || code.SyncPlan == nil || code.SyncPlan.IsEmpty() { + return []string{entry("~", "code", codeDetail(code))}, nil + } + + var b strings.Builder + + if err := display.PrintPlan(&b, code.SyncPlan); err != nil { + return nil, fmt.Errorf("render the sync file list: %w", err) + } + + return strings.Split(strings.TrimSuffix(b.String(), "\n"), "\n"), nil } // diffHeader names the plan's subject. A live workload is named exactly as diff --git a/internal/workload/up/render_diff_test.go b/internal/workload/up/render_diff_test.go index 150789145..e30681246 100644 --- a/internal/workload/up/render_diff_test.go +++ b/internal/workload/up/render_diff_test.go @@ -20,6 +20,7 @@ import ( "testing" "github.com/datarobot/cli/internal/workload/manifest" + "github.com/datarobot/cli/internal/workload/sync" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -454,10 +455,65 @@ func TestRenderDiff_ArtifactEntryAnnouncesTheRoll(t *testing.T) { "the capped detail list is replaced by the diff, not repeated under it") } -// TestRenderDiff_CodeLineKeepsItsPositionUntilTheFileListLands: code drift -// is measured upstream of the renderer, so until the sync file list replaces -// it the bare count renders as the action line it already is. -func TestRenderDiff_CodeLineKeepsItsPositionUntilTheFileListLands(t *testing.T) { +// driftedSyncPlan is the dry-run plan the sync engine hands back for a tree +// with two changed files and one deleted one, wired through the codeChangeFn +// seam exactly as defaultCodeChange carries the real one. +func driftedSyncPlan() *sync.SyncPlan { + return &sync.SyncPlan{ + Uploads: []sync.FileAction{ + {Path: "main.py", Classification: sync.ClsLocalModified, Action: sync.ActUploadModify, LocalSize: 1234}, + {Path: "new.go", Classification: sync.ClsLocalAdded, Action: sync.ActUploadAdd, LocalSize: 300}, + }, + Deletes: []sync.FileAction{ + {Path: "old.txt", Action: sync.ActUploadDelete, LocalSize: 15}, + }, + } +} + +// TestRenderDiff_CodeDriftPrintsTheSyncFileList: in diff mode the code block +// is the file list the sync would upload, drawn by the same renderer +// `dr artifact code sync --dry-run` uses and fed the same dry-run plan. +// Naming the files is the whole point of a diff; a count would leave the +// reader guessing which ones move. +func TestRenderDiff_CodeDriftPrintsTheSyncFileList(t *testing.T) { + plan := Plan{ + State: StateRunning, + Locked: true, + Code: CodeChange{ + Applies: true, + Files: 3, + SyncPlan: driftedSyncPlan(), + }, + DiffArtifact: []DiffRow{ + {Path: "port", Want: 9090.0, Have: 8080.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "Sync plan:") + assert.Contains(t, out, "โ†‘ UPLOAD (2):") + assert.Contains(t, out, "main.py 1.2 KiB") + assert.Contains(t, out, "new.go 300 B", "rows align inside a group, the way the sync plan prints them") + assert.Contains(t, out, "โœ• DELETE (1):") + assert.Contains(t, out, "old.txt 15 B") + + assert.Contains(t, out, "~ lock", "the plan below the file list pins the block order") + + assert.NotContains(t, out, "3 files changed since the last deploy", + "the count is the default mode's summary; the diff names the files instead") + assert.NotContains(t, out, "~ code") + + assert.Less(t, strings.Index(out, "Sync plan:"), strings.Index(out, "~ lock"), + "the file list keeps the code block's position, ahead of the lock line") +} + +// TestRenderDiff_CodeCountWithoutAPlanKeepsTheSummaryLine: the file list +// needs the sync plan the seam carries, and a CodeChange built without one -- +// as test harnesses build them, since no production path measures files +// without also measuring the list -- falls back to the count the default +// mode prints rather than going silent about real drift. +func TestRenderDiff_CodeCountWithoutAPlanKeepsTheSummaryLine(t *testing.T) { plan := Plan{ State: StateRunning, Code: builtCode(14), diff --git a/internal/workload/up/run.go b/internal/workload/up/run.go index b3b4e0773..cf0ff905d 100644 --- a/internal/workload/up/run.go +++ b/internal/workload/up/run.go @@ -1267,8 +1267,13 @@ func defaultCodeChange(loaded Loaded, live Live) (change CodeChange, err error) } return CodeChange{ - Applies: true, - Files: len(plan.Uploads) + len(plan.Deletes), + Applies: true, + Files: len(plan.Uploads) + len(plan.Deletes), + // The plan rides along beside its count so --diff can name the files + // rather than only counting them, rendered by the sync command's own + // plan printer. It is data, not engine state, so it survives the + // Close below. + SyncPlan: plan, IgnoreNotice: notice, ImageStale: imageStale(loaded.ProjectDir, live), // The engine says so rather than this asking a second time: it fetched From aaac9e0489d72019e94206115eef4d992ddadcb2 Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Fri, 28 Aug 2026 16:40:28 -0700 Subject: [PATCH 05/12] [RAPTOR-19538] feat(workload): wire the --diff flag through the deploy up.Options gains Diff and Run picks RenderDiff over Render at the one render call site, so --diff swaps how the plan is shown and nothing else: the dry-run return and every apply branch read the plan exactly as they do without it. The shell registers the flag with help text that says the default plan is the summary and the diff is the detail, threads it into the options, and reports it to telemetry under its own key so adoption is readable without guessing from dry_run. Proved by seam tests at both layers: stubRun observes the option, a sized fixture shows the selector swapping the renderer while the default path keeps its have -> want summary with no hunks, and --dry-run --diff prints the identical diff body the wet run prints, with the mutating seams wired to fail the test on the dry leg. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- cmd/workload/up/cmd.go | 10 ++++ cmd/workload/up/cmd_test.go | 49 +++++++++++++++++ internal/workload/up/run.go | 23 +++++++- internal/workload/up/run_test.go | 92 ++++++++++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 1 deletion(-) diff --git a/cmd/workload/up/cmd.go b/cmd/workload/up/cmd.go index 135793a72..5d025ff6c 100644 --- a/cmd/workload/up/cmd.go +++ b/cmd/workload/up/cmd.go @@ -85,6 +85,7 @@ type flags struct { dir string yes bool dryRun bool + diff bool detach bool lock bool force bool @@ -179,6 +180,7 @@ Examples: return map[string]any{ "yes": nonInteractive, "dry_run": f.dryRun, + "diff": f.diff, "detach": f.detach, "lock": f.lock, "force_build": f.force, @@ -195,6 +197,13 @@ func addFlags(cmd *cobra.Command, f *flags, poll *pollflags.Set) { "Do not prompt. With no manifest this is an error rather than a wizard, "+ "and rolling a locked production version is not confirmed.") cmd.Flags().BoolVar(&f.dryRun, "dry-run", false, "Print the plan and change nothing.") + // The help says what changes about the rendering rather than leaving the + // reader to run it to find out: the default plan is the summary, the + // diff is the detail, and neither says anything the other does not. + cmd.Flags().BoolVar(&f.diff, "diff", false, + "Render the plan as a unified diff instead of the changed-fields list: "+ + "every field the file names appears, unchanged ones as context that collapses when it runs long, "+ + "and nothing is truncated. Combine with --dry-run to look without touching.") cmd.Flags().BoolVar(&f.detach, "detach", false, "Return once the deploy is requested; do not wait for it to serve.") cmd.Flags().BoolVar(&f.lock, "lock", false, "Lock whichever artifact ends up live, making it permanent, even when this deploy minted no new "+ @@ -239,6 +248,7 @@ func run(cmd *cobra.Command, f flags, poll pollflags.Set, format outputformat.Ou Dir: dir, NonInteractive: nonInteractive, DryRun: f.dryRun, + Diff: f.diff, Detach: f.detach, Lock: f.lock, Confirm: rollConfirm(cmd, yes), diff --git a/cmd/workload/up/cmd_test.go b/cmd/workload/up/cmd_test.go index 00057358d..765d8e5f2 100644 --- a/cmd/workload/up/cmd_test.go +++ b/cmd/workload/up/cmd_test.go @@ -23,6 +23,7 @@ import ( "strings" "testing" + "github.com/datarobot/cli/internal/telemetry" "github.com/datarobot/cli/internal/workload/manifest" "github.com/datarobot/cli/internal/workload/up" "github.com/stretchr/testify/assert" @@ -612,6 +613,54 @@ func TestCmd_UnchangedRunStillWarnsAboutTheDraft(t *testing.T) { assert.Contains(t, stderr, draftHeadline) } +// TestCmd_DiffIsRegistered keeps the flag discoverable: cobra lists it in +// --help only when it is registered, not hidden, and defaulted off, and the +// help text is what tells a reader what they are about to get. +func TestCmd_DiffIsRegistered(t *testing.T) { + lookup := Cmd().Flags().Lookup("diff") + + require.NotNil(t, lookup, "the flag has to exist for --diff to parse at all") + assert.False(t, lookup.Hidden) + assert.Equal(t, "false", lookup.DefValue, "the diff rendering is opt-in") + assert.Contains(t, lookup.Usage, "unified diff") +} + +// TestCmd_DiffReachesTheDeploy threads the flag into the deploy's options, +// and only when it was given: a run without it must not silently start +// rendering diffs. +func TestCmd_DiffReachesTheDeploy(t *testing.T) { + seen := stubRun(t, deployed(), nil) + + _, _, err := runCmd(t, "--dry-run", "--diff") + require.NoError(t, err) + assert.True(t, seen.Diff) + assert.True(t, seen.DryRun) + + defaults := stubRun(t, deployed(), nil) + + _, _, err = runCmd(t) + require.NoError(t, err) + assert.False(t, defaults.Diff) +} + +// TestCmd_TelemetryRecordsTheDiffFlag: adoption of the flag has to be +// readable without guessing at it from dry_run, so it is reported under its +// own key, reflecting the flag as given. +func TestCmd_TelemetryRecordsTheDiffFlag(t *testing.T) { + withFlag := Cmd() + require.NoError(t, withFlag.ParseFlags([]string{"--diff"})) + + event, ok := telemetry.EventFor(withFlag, nil) + require.True(t, ok, "EventFor must return ok=true for an annotated command") + + assert.Equal(t, true, event.EventProperties["diff"]) + assert.Equal(t, false, event.EventProperties["dry_run"], "the pre-existing keys keep their own values") + + event, ok = telemetry.EventFor(Cmd(), nil) + require.True(t, ok) + assert.Equal(t, false, event.EventProperties["diff"], "an unset flag reports itself as off, not as absent") +} + func TestCmd_IsRegisteredUnderWorkload(t *testing.T) { cmd := Cmd() diff --git a/internal/workload/up/run.go b/internal/workload/up/run.go index cf0ff905d..ea1fb1b82 100644 --- a/internal/workload/up/run.go +++ b/internal/workload/up/run.go @@ -81,6 +81,13 @@ type Options struct { // DryRun stops after the plan. DryRun bool + // Diff renders the plan as a unified diff instead of the summary list: + // the same plan, laid out leaf by leaf with context around what moves. + // It changes how the plan is shown and nothing else -- the dry-run + // return and every apply branch read the plan exactly as they do + // without it, so a diff run and a summary run mutate the same things. + Diff bool + // Detach returns once the apply is requested, skipping the waits. Detach bool @@ -198,7 +205,10 @@ func Run(opts Options) (Result, error) { } summary := Summary{Name: result.Name, WorkloadID: result.WorkloadID, Status: live.Status} - if err := Render(opts.Stderr, summary, plan); err != nil { + + renderPlan := rendererFor(opts.Diff) + + if err := renderPlan(opts.Stderr, summary, plan); err != nil { return result, err } @@ -219,6 +229,17 @@ func Run(opts Options) (Result, error) { return apply(loaded, live, plan, result, opts) } +// rendererFor picks how the plan is shown. --diff swaps the renderer and +// nothing else: the plan goes out laid out as a diff rather than summarised, +// and everything below reads it identically either way. +func rendererFor(diff bool) func(io.Writer, Summary, Plan) error { + if diff { + return RenderDiff + } + + return Render +} + // lockOnly is the whole of a --lock run that found nothing else to do. // // --lock is about the end state rather than about what this run happened to diff --git a/internal/workload/up/run_test.go b/internal/workload/up/run_test.go index dfa942f66..ac6f1f07a 100644 --- a/internal/workload/up/run_test.go +++ b/internal/workload/up/run_test.go @@ -502,6 +502,98 @@ func TestRun_DryRunAppliesNothing(t *testing.T) { assert.Contains(t, stderr, "+ workload") } +// resizedManifest is the bound fixture with the sizing moved one step, which +// is the one-change plan the wiring tests below diff. The binding line rides +// on top, as it does wherever the tests deploy against the live fixtures. +func resizedManifest() string { + return "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + + strings.Replace(boundLiveManifest, "memory: 22GB", "memory: 24GB", 1) +} + +// TestRun_DiffRendersTheUnifiedDiffInsteadOfTheSummary is the selector: the +// flag swaps the renderer and nothing else. With it the plan is laid out as +// a diff stating both sides of what moves; without it the summary keeps its +// own spelling, because a flag nobody passed may not change a byte. +func TestRun_DiffRendersTheUnifiedDiffInsteadOfTheSummary(t *testing.T) { + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + }) + + sized := resizedManifest() + + _, stderr, err := runIn(t, sized, Options{NonInteractive: true, DryRun: true}) + require.NoError(t, err) + + assert.Contains(t, stderr, "resourceAllocation.memory: 22GB -> 24GB", + "the default plan states a change as have -> want") + assert.NotContains(t, stderr, "\n+ ") + assert.NotContains(t, stderr, "\n- ", "the diff is opt-in; the default plan must not grow hunks") + + _, diffStderr, err := runIn(t, sized, Options{NonInteractive: true, DryRun: true, Diff: true}) + require.NoError(t, err) + + assert.Contains(t, diffStderr, "- containerGroups[default].containers[vllm-server].resourceAllocation.memory: 22GB") + assert.Contains(t, diffStderr, "+ containerGroups[default].containers[vllm-server].resourceAllocation.memory: 24GB") + assert.NotContains(t, diffStderr, "-> ", + "a diff states each side of a change on its own line, never the summary's arrow") +} + +// TestRun_DryRunDiffMatchesTheWetDiffAndWritesNothing is the contract that +// makes --dry-run --diff the look-without-touching combination: the dry run +// prints the identical diff body the wet run prints for the same state, and +// stops where the wet run continues into progress. The mutating seams are +// wired to fail the test on the dry leg, so "prints the same body" and +// "performs zero writes" are asserted by the same run. +func TestRun_DryRunDiffMatchesTheWetDiffAndWritesNothing(t *testing.T) { + sized := resizedManifest() + + liveDocs := fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + } + + dry := liveDocs + dry.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + t.Fatal("a dry run must not touch the workload") + + return nil, nil + } + dry.replace = func(string, string, json.RawMessage) (*workload.Replacement, error) { + t.Fatal("a dry run must not roll anything") + + return nil, nil + } + + install(t, dry) + + dryResult, dryStderr, err := runIn(t, sized, Options{NonInteractive: true, DryRun: true, Diff: true}) + require.NoError(t, err) + + assert.Equal(t, ActionUpdated, dryResult.Action, "the dry run's action is the plan's own") + assert.Contains(t, dryStderr, "+ containerGroups[default].containers[vllm-server].resourceAllocation.memory: 24GB") + + // The wet leg runs the same plan far enough to act: the resize is + // requested, and --detach stops there so the leg needs no wait wiring. + var resized bool + + wet := liveDocs + wet.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + resized = true + + return nil, nil + } + + install(t, wet) + + _, wetStderr, err := runIn(t, sized, Options{NonInteractive: true, Diff: true, Detach: true}) + require.NoError(t, err) + assert.True(t, resized, "the wet leg has to reach the mutation for the comparison to mean anything") + + assert.True(t, strings.HasPrefix(wetStderr, dryStderr), + "the wet run must print the identical diff body before its progress;\ndry: %q\nwet: %q", dryStderr, wetStderr) +} + // TestRun_AlreadyUpToDate exits without touching anything, which is what // makes `up` cheap to run on every push. func TestRun_AlreadyUpToDate(t *testing.T) { From 6bcaa446dbe03a9a7aae1ca0c0efe14b082c96a5 Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Fri, 28 Aug 2026 17:08:29 -0700 Subject: [PATCH 06/12] [RAPTOR-19538] feat(workload): carry the diff section in the JSON envelope With --diff the plan object gains a structured `diff` section: one entry per changing leaf ({path, have, want, absent}) in the order the diff body prints them -- artifact rows, then the synthetic artifactId and artifact.type entries, then runtime -- plus the unmanaged path list, so a consumer reads the same changes as data that the human block prints as lines. Entries on an environmentVars path serialise with their values withheld and a redacted marker, the JSON twin of the rule that keeps secrets out of the printed plan: the envelope is the artefact that ends up in CI logs, so redaction happens before marshalling rather than after. The section is present only when --diff was asked for: JSON() keeps its exact shape, a run without the flag gains no diff key, and the legacy artifact/runtime arrays ride along unchanged beside it. The dry run and the run that follows it carry the same section, because both project the one pre-apply plan. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- cmd/workload/up/cmd.go | 14 +- cmd/workload/up/cmd_test.go | 84 ++++++ internal/workload/up/render.go | 113 +++++++ internal/workload/up/render_test.go | 449 ++++++++++++++++++++++++++++ internal/workload/up/run_test.go | 54 ++++ 5 files changed, 713 insertions(+), 1 deletion(-) diff --git a/cmd/workload/up/cmd.go b/cmd/workload/up/cmd.go index 5d025ff6c..600378610 100644 --- a/cmd/workload/up/cmd.go +++ b/cmd/workload/up/cmd.go @@ -409,6 +409,18 @@ func draftIsServing(f flags, result up.Result, failed bool) bool { return f.dryRun || result.WorkloadID != "" } +// planEnvelope picks the plan's machine shape. --diff adds the structured +// diff section to the same document; without the flag the envelope keeps the +// exact shape it had before the flag existed, so a consumer that never asked +// for a diff never has to learn about one. +func planEnvelope(p up.Plan, diff bool) up.PlanJSON { + if diff { + return p.JSONWithDiff() + } + + return p.JSON() +} + func render(cmd *cobra.Command, f flags, format outputformat.OutputFormat, result up.Result, failed bool) error { if format == outputformat.OutputFormatJSON { return outputformat.PrintJSONEnvelope(cmd.OutOrStdout(), "up", upResult{ @@ -420,7 +432,7 @@ func render(cmd *cobra.Command, f flags, format outputformat.OutputFormat, resul BuildID: buildID(result.BuildID), Action: result.Action, Locked: result.Locked, - Plan: result.Plan.JSON(), + Plan: planEnvelope(result.Plan, f.diff), }) } diff --git a/cmd/workload/up/cmd_test.go b/cmd/workload/up/cmd_test.go index 765d8e5f2..9b48b625b 100644 --- a/cmd/workload/up/cmd_test.go +++ b/cmd/workload/up/cmd_test.go @@ -661,6 +661,90 @@ func TestCmd_TelemetryRecordsTheDiffFlag(t *testing.T) { assert.Equal(t, false, event.EventProperties["diff"], "an unset flag reports itself as off, not as absent") } +// diffedResult is a result whose plan carries one change in each half, a +// whole row structure and an unmanaged path, the shape the JSON diff +// envelope tests read through the stubbed deploy. +func diffedResult() up.Result { + return up.Result{ + Plan: up.Plan{ + State: up.StateRunning, + Code: up.CodeChange{}, + Artifact: []up.Change{{Path: "containerGroups[default].containers[primary].port", Have: 8080.0, Want: 9090.0}}, + Runtime: []up.Change{{Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}}, + + DiffArtifact: []up.DiffRow{{ + Path: "containerGroups[default].containers[primary].port", + Have: 8080.0, Want: 9090.0, Changed: true, + }}, + DiffRuntime: []up.DiffRow{{ + Path: "containerGroups[default].replicaCount", + Have: 1.0, Want: 3.0, Changed: true, + }}, + + Unmanaged: []string{"containerGroups[default].containers[metrics]"}, + }, + } +} + +// TestCmd_JSONDiffSectionFollowsTheFlag threads the flag into the envelope +// itself: with it, stdout is still exactly one JSON document and the plan +// inside it gains the structured diff section; without it, the plan has no +// diff key at all, which is what keeps a consumer that never asked for a diff +// from having to learn about one. +func TestCmd_JSONDiffSectionFollowsTheFlag(t *testing.T) { + stubRun(t, diffedResult(), nil) + + stdout, _, err := runCmd(t, "--dry-run", "--output-format", "json", "--diff") + require.NoError(t, err) + + var envelope map[string]any + + require.NoError(t, json.Unmarshal([]byte(stdout), &envelope), + "stdout must be one JSON document and nothing else") + + body := envelope["up"].(map[string]any) + + plan, ok := body["plan"].(map[string]any) + require.True(t, ok) + + diff, ok := plan["diff"].(map[string]any) + require.True(t, ok, "--diff was requested, so the section is present") + + changes := diff["changes"].([]any) + require.Len(t, changes, 2) + + first := changes[0].(map[string]any) + + assert.Equal(t, "containerGroups[default].containers[primary].port", first["path"]) + assert.InDelta(t, 8080.0, first["have"], 0) + assert.InDelta(t, 9090.0, first["want"], 0) + assert.Equal(t, false, first["absent"]) + + unmanaged := diff["unmanaged"].([]any) + require.Len(t, unmanaged, 1) + assert.Equal(t, "containerGroups[default].containers[metrics]", unmanaged[0]) + + // The legacy arrays keep their place beside the new section. + artifact := plan["artifact"].([]any) + require.Len(t, artifact, 1) + assert.Equal(t, "containerGroups[default].containers[primary].port: 8080 -> 9090", artifact[0]) + + stubRun(t, diffedResult(), nil) + + stdout, _, err = runCmd(t, "--dry-run", "--output-format", "json") + require.NoError(t, err) + + envelope = map[string]any{} + + require.NoError(t, json.Unmarshal([]byte(stdout), &envelope), + "stdout must be one JSON document and nothing else") + + body = envelope["up"].(map[string]any) + plan = body["plan"].(map[string]any) + + assert.NotContains(t, plan, "diff", "without the flag the section is absent, not null") +} + func TestCmd_IsRegisteredUnderWorkload(t *testing.T) { cmd := Cmd() diff --git a/internal/workload/up/render.go b/internal/workload/up/render.go index 7ced8716e..2dfcfbbb4 100644 --- a/internal/workload/up/render.go +++ b/internal/workload/up/render.go @@ -687,6 +687,44 @@ type PlanJSON struct { Code CodeJSON `json:"code"` Artifact []string `json:"artifact"` Runtime []string `json:"runtime"` + + // Diff carries the structured form of what --diff renders, and is + // present only when --diff was asked for: a caller that never passed the + // flag reads the same document it always did, and one that did gets the + // changes as data rather than scraping them out of the human block. + Diff *DiffJSON `json:"diff,omitempty"` +} + +// DiffJSON is the plan's diff section. Both of its lists are the plan's own +// accounting, carried so a machine-readable envelope says everything the +// human diff does about what moves and what is left alone. +type DiffJSON struct { + // Changes is one entry per changing leaf of the manifest, in the order + // the diff body prints them. The unchanged leaves the diff draws as + // context are not here: a list of everything the file already agrees + // with would bury the few entries a consumer acts on. + Changes []ChangeJSON `json:"changes"` + + // Unmanaged holds the paths of what the live object carries that the + // file never names, so a caller can see what this deploy leaves alone + // without parsing the count out of the human summary. + Unmanaged []string `json:"unmanaged"` +} + +// ChangeJSON is one changing leaf, the structured twin of the `- old`/`+ new` +// pair the diff prints for it. Have is null and Absent true for a leaf the +// live object does not carry, which is an addition and not a swap. +type ChangeJSON struct { + Path string `json:"path"` + Have any `json:"have"` + Want any `json:"want"` + Absent bool `json:"absent"` + + // Redacted marks a path whose values were withheld, so an entry with + // null sides reads as refused rather than empty. The omitempty keeps the + // marker off the entries that carried their values in plain, which is + // most of them. + Redacted bool `json:"redacted,omitempty"` } // CodeJSON is the working tree's part of the answer. @@ -743,3 +781,78 @@ func describeAll(changes []Change) []string { return out } + +// JSONWithDiff is JSON plus the diff section --diff renders. Both come from +// the one plan, so the envelope and the human block cannot disagree about +// what changes; the plain JSON() keeps the default path's exact shape, which +// is what a caller that never passed the flag is owed. +func (p Plan) JSONWithDiff() PlanJSON { + out := p.JSON() + out.Diff = p.diffJSON() + + return out +} + +// diffJSON projects the diff rows into their machine-readable shape, the +// artifact side first with the synthetic id and type entries where Build +// appended them, then the runtime side: the order the diff body draws, so +// the two renderings of one plan list their changes as one sequence a +// consumer can zip. +func (p Plan) diffJSON() *DiffJSON { + return &DiffJSON{ + Changes: changesJSON(p.DiffArtifact, p.DiffRuntime), + Unmanaged: unmanagedJSON(p.Unmanaged), + } +} + +// changesJSON walks the two row halves keeping only the changing leaves. The +// agreeing ones are context on the way to a reader and noise in a list, and +// the walk they come from already put the changing ones in the body's order. +func changesJSON(artifact, runtime []DiffRow) []ChangeJSON { + leaves := make([]DiffRow, 0, len(artifact)+len(runtime)) + + leaves = append(leaves, artifact...) + leaves = append(leaves, runtime...) + + out := make([]ChangeJSON, 0, len(leaves)) + + for _, leaf := range leaves { + if !leaf.Changed { + continue + } + + out = append(out, changeJSON(leaf)) + } + + return out +} + +// changeJSON serialises one leaf, redacting before it marshals. The values +// of an environment variable are withheld here exactly as the human diff +// withholds them, because a machine-readable plan is the one that ends up in +// CI artifacts; the entry still says which path moved, so the change itself +// is not silently lost to the refusal. +func changeJSON(leaf DiffRow) ChangeJSON { + out := ChangeJSON{ + Path: leaf.Path, + Have: leaf.Have, + Want: leaf.Want, + Absent: leaf.Absent, + } + + if redacted(leaf.Path) { + out.Have = nil + out.Want = nil + out.Redacted = true + } + + return out +} + +// unmanagedJSON copies the unmanaged paths, always into a non-nil slice so +// the JSON carries [] rather than null for "nothing unmanaged". +func unmanagedJSON(paths []string) []string { + out := make([]string, 0, len(paths)) + + return append(out, paths...) +} diff --git a/internal/workload/up/render_test.go b/internal/workload/up/render_test.go index 1c8839158..332e09d10 100644 --- a/internal/workload/up/render_test.go +++ b/internal/workload/up/render_test.go @@ -19,6 +19,7 @@ import ( "strings" "testing" + "github.com/datarobot/cli/internal/workload/manifest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -530,3 +531,451 @@ func TestRender_LinkedProjectOnAPublishedImageStillGetsANewArtifact(t *testing.T assert.Contains(t, out, "with its first artifact") assert.NotContains(t, out, "linked to") } + +// diffDriftPayload is the fixture the JSON diff tests read: drift in both +// halves (two spec leaves, one whole block the live object lacks, two sizing +// leaves) with unchanged and unmanaged fields around them, which is the least +// a section claiming to mirror the diff has to carry. +const diffDriftPayload = `{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "spec": { + "type": "service", + "containerGroups": [{"name": "default", "containers": [ + {"name": "primary", "primary": true, "port": 9090, + "readinessProbe": {"path": "/ready", "port": 8080}, + "startupProbe": {"path": "/started", "port": 8080}} + ]}] + }}, + "runtime": {"containerGroups": [ + {"name": "default", "replicaCount": 3, + "containers": [{"name": "primary", "resourceAllocation": {"cpu": 1, "memory": "512MB"}}]} + ]} +}` + +// driftPlan builds that fixture the way a real run does, through Build, so +// the envelope and the diff read the same walk rather than two constructions. +func driftPlan(t *testing.T) Plan { + t.Helper() + + plan, err := Build( + loadedFrom(diffDriftPayload), + liveFrom(t, StateRunning, planLiveSpec, planLiveRuntime), + builtCode(0), + ) + require.NoError(t, err) + + return plan +} + +// TestPlanJSON_DiffAbsentWithoutTheFlag keeps the pre-feature envelope exact +// for a caller that never asked for a diff: no diff key at all, never a null +// one, on each of the three shapes an envelope can take. +func TestPlanJSON_DiffAbsentWithoutTheFlag(t *testing.T) { + cases := []struct { + name string + plan Plan + }{ + {"nothing changed", Plan{State: StateRunning, Code: builtCode(0)}}, + {"a sizing change", Plan{ + State: StateRunning, + Runtime: []Change{{Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}}, + }}, + {"a first deploy", Plan{State: StateUnbound, Creates: true}}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + encoded, err := json.Marshal(c.plan.JSON()) + require.NoError(t, err) + + var decoded map[string]any + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + assert.NotContains(t, decoded, "diff", + "the section is added only when --diff asked for it, not nulled out") + }) + } +} + +// TestPlanJSON_DiffCarriesTheChangingLeaves: with --diff the plan carries one +// entry per changing leaf, in the walk's order, with the unchanged context +// leaves left out of it and the unmanaged paths listed beside them. The +// legacy artifact/runtime arrays ride along unchanged, because --diff adds a +// section rather than reshaping the envelope a consumer already reads. +func TestPlanJSON_DiffCarriesTheChangingLeaves(t *testing.T) { + plan := driftPlan(t) + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + var decoded map[string]any + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + diff, ok := decoded["diff"].(map[string]any) + require.True(t, ok, "--diff was requested, so the section is an object") + + changes, ok := diff["changes"].([]any) + require.True(t, ok, "changes is an array even when it is empty") + + got := make([]string, 0, len(changes)) + + for _, c := range changes { + entry, ok := c.(map[string]any) + require.True(t, ok) + + path, _ := entry["path"].(string) + got = append(got, path) + + assert.Contains(t, entry, "have") + assert.Contains(t, entry, "want") + assert.Contains(t, entry, "absent") + assert.NotContains(t, entry, "redacted", "nothing on this fixture is an environment variable") + } + + // One entry per changing leaf of the manifest, in the same order the + // walk reports them to the default envelope: the two halves concatenated, + // unchanged-but-managed leaves nowhere among them. + want := make([]string, 0, len(plan.Artifact)+len(plan.Runtime)) + + want = append(want, paths(plan.Artifact)...) + want = append(want, paths(plan.Runtime)...) + + assert.Equal(t, want, got) + assert.Len(t, got, 5, "the fixture drifts in five leaves across the two halves") + assert.Contains(t, got, "containerGroups[default].containers[primary].startupProbe") + assert.NotContains(t, got, "containerGroups[default].containers[primary].port.expectation", + "the walk only answers for leaves the file names") + + // The absent leaf carries no live value, and says so twice: a null have + // and an absent flag, because "not there" is a different act from + // "different value". + var added map[string]any + + for _, c := range changes { + entry, _ := c.(map[string]any) + + if entry["path"] == "containerGroups[default].containers[primary].startupProbe" { + added = entry + + break + } + } + + require.NotNil(t, added) + assert.Nil(t, added["have"]) + assert.Equal(t, true, added["absent"]) + assert.NotNil(t, added["want"]) + + // The unmanaged side is its own list, the plan's deduped Extra paths: + // the sidecar the file never names, counted once however many halves + // carry it, and never mistaken for a change. + unmanaged, ok := diff["unmanaged"].([]any) + require.True(t, ok, "unmanaged is an array even when it is empty") + require.Len(t, unmanaged, 1) + assert.Equal(t, "containerGroups[default].containers[metrics]", unmanaged[0]) + + for _, path := range unmanaged { + assert.NotContains(t, got, path, "unmanaged fields are not changes") + } + + // The arrays a consumer already reads keep their exact content with and + // without the section. + plain := plan.JSON() + + assert.Equal(t, plain.Artifact, plan.JSONWithDiff().Artifact) + assert.Equal(t, plain.Runtime, plan.JSONWithDiff().Runtime) +} + +// TestPlanJSON_DiffRedactsBeforeSerialising is the JSON half of the one hard +// rule. A literal can be a secret pasted in plaintext and a credential ref +// names one; both ride in the diff rows as raw values, so the values are +// withheld here rather than trusted to a marshalling order, and the entry +// carries a marker saying the refusal happened. +func TestPlanJSON_DiffRedactsBeforeSerialising(t *testing.T) { + const ( + literal = "sk-literal-plaintext" + oldLit = "sk-old-plaintext" + rotated = "aaaa222222222222222222aa" + liveID = "66f1a2b3c4d5e6f7a8b9c0d1" + ) + + payload := `{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "spec": { + "type": "service", + "containerGroups": [{"name": "default", "containers": [ + {"name": "primary", "environmentVars": [ + {"name": "OPENAI_API_KEY", "value": "` + literal + `"}, + {"name": "HUGGING_FACE_HUB_TOKEN", "source": "dr-credential", + "drCredentialId": "` + rotated + `", "key": "apiToken"} + ]} + ]}] + }} + }` + + live := `{ + "type": "service", + "containerGroups": [{"name": "default", "containers": [ + {"name": "primary", "environmentVars": [ + {"name": "OPENAI_API_KEY", "value": "` + oldLit + `"}, + {"name": "HUGGING_FACE_HUB_TOKEN", "source": "dr-credential", + "drCredentialId": "` + liveID + `", "key": "apiToken"} + ]} + ]}] + }` + + plan, err := Build(loadedFrom(payload), liveFrom(t, StateRunning, live, planLiveRuntime), builtCode(0)) + require.NoError(t, err) + + // The fixture has to earn its keep: two env-var changes are what the walk + // found, and they are what must not leak. + require.Equal(t, []string{ + "containerGroups[default].containers[primary].environmentVars[OPENAI_API_KEY].value", + "containerGroups[default].containers[primary].environmentVars[HUGGING_FACE_HUB_TOKEN].drCredentialId", + }, paths(plan.Artifact)) + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + document := string(encoded) + + for _, secret := range []string{literal, oldLit, rotated, liveID, "dr-credential:"} { + assert.NotContains(t, document, secret) + } + + var decoded map[string]any + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + diff := decoded["diff"].(map[string]any) + changes := diff["changes"].([]any) + require.Len(t, changes, 2) + + for _, c := range changes { + entry := c.(map[string]any) + + assert.Contains(t, entry["path"], "environmentVars[", "the name is the whole point of the entry") + assert.Nil(t, entry["have"], "the live value never serialises") + assert.Nil(t, entry["want"], "the asked-for value never serialises") + assert.Equal(t, true, entry["redacted"]) + } + + // The legacy arrays were redacted before this section existed, and stay + // that way: they say the change happened, never what it said. + plain := plan.JSON() + + assert.NotContains(t, strings.Join(plain.Artifact, "\n"), literal) + assert.Contains(t, strings.Join(plain.Artifact, "\n"), "changed") + assert.Equal(t, plain.Artifact, plan.JSONWithDiff().Artifact) +} + +// TestPlanJSON_FirstDeployDiffIsAllAdditions: with no live object every leaf +// of the compiled manifest is an addition, absent with no live value, and +// there is nothing for the unmanaged list to hold. +func TestPlanJSON_FirstDeployDiffIsAllAdditions(t *testing.T) { + plan, err := Build( + loadedFrom(planPayload), + Live{State: StateUnbound}, + CodeChange{Applies: true, FirstDeploy: true}, + ) + require.NoError(t, err) + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + var decoded map[string]any + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + assert.Equal(t, true, decoded["creates"]) + assert.Empty(t, decoded["priorWorkloadId"]) + + diff, ok := decoded["diff"].(map[string]any) + require.True(t, ok) + + changes := diff["changes"].([]any) + require.NotEmpty(t, changes, "a first deploy is made of the leaves the manifest names") + + for _, c := range changes { + entry := c.(map[string]any) + + assert.Equal(t, true, entry["absent"], "%v must be an addition", entry["path"]) + assert.Nil(t, entry["have"]) + assert.NotNil(t, entry["want"]) + } + + unmanaged := diff["unmanaged"].([]any) + assert.Empty(t, unmanaged, "there is no live object to carry unmanaged fields") + + plain := plan.JSON() + + assert.Equal(t, plain.Artifact, plan.JSONWithDiff().Artifact) + assert.Equal(t, plain.Runtime, plan.JSONWithDiff().Runtime) +} + +// TestPlanJSON_EmptyPlanDiffIsTwoEmptyLists: --diff still asked, so the +// section is present with empty arrays where a change would sit, and the +// rest of the envelope is the exact document the default path would have +// produced. +func TestPlanJSON_EmptyPlanDiffIsTwoEmptyLists(t *testing.T) { + plan := Plan{State: StateRunning, Code: builtCode(0)} + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + var withDiff map[string]any + + require.NoError(t, json.Unmarshal(encoded, &withDiff)) + + diff, ok := withDiff["diff"].(map[string]any) + require.True(t, ok, "the section is present because --diff asked, even for an empty plan") + + changes, ok := diff["changes"].([]any) + require.True(t, ok, "changes is [] rather than null") + assert.Empty(t, changes) + + unmanaged, ok := diff["unmanaged"].([]any) + require.True(t, ok, "unmanaged is [] rather than null") + assert.Empty(t, unmanaged) + + plainEncoded, err := json.Marshal(plan.JSON()) + require.NoError(t, err) + + var plain map[string]any + + require.NoError(t, json.Unmarshal(plainEncoded, &plain)) + + delete(withDiff, "diff") + assert.Equal(t, plain, withDiff, "aside from the section, the two envelopes are one document") +} + +// textDiffChangePaths pulls the paths of a rendered diff's change lines, in +// order. A changed leaf states both sides of itself, `- old` then `+ new`, +// and is one entry in the JSON list, so a run of same-path lines collapses to +// one. The text and the list then say the same sequence, not just the same set. +func textDiffChangePaths(t *testing.T, body string) []string { + t.Helper() + + var out []string + + for _, line := range strings.Split(body, "\n") { + if len(line) < 2 || (line[0] != '+' && line[0] != '-') { + continue + } + + path, _, _ := strings.Cut(line[2:], ": ") + + if len(out) > 0 && out[len(out)-1] == path { + continue + } + + out = append(out, path) + } + + return out +} + +// TestPlanJSON_DiffOrderMatchesTheTextBody: both renderings draw from the one +// walk the plan was built with, so the Nth entry of the list is the Nth +// change the text prints, artifact side then runtime side, synthetic +// artifact-id change included. A consumer reading both representations +// deserves a sequence it can zip, and a list that reordered itself between +// the two would make every such zip a lie. +func TestPlanJSON_DiffOrderMatchesTheTextBody(t *testing.T) { + plan := driftPlan(t) + + var body strings.Builder + + require.NoError(t, RenderDiff(&body, appSummary, plan)) + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + var decoded PlanJSON + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + require.NotNil(t, decoded.Diff) + require.NotEmpty(t, decoded.Diff.Changes, "the fixture drifts, so both renderings have changes to line up") + + fromJSON := make([]string, 0, len(decoded.Diff.Changes)) + + for _, c := range decoded.Diff.Changes { + fromJSON = append(fromJSON, c.Path) + } + + assert.Equal(t, textDiffChangePaths(t, body.String()), fromJSON) +} + +// TestPlanJSON_DiffSyntheticChangesSurvive: the artifact id is a change no +// walk of the spec can produce, because a file bound by id describes no spec +// at all. The default envelope reports it, so the diff section must too -- +// a version roll invisible in one rendering of the plan it drives is a +// silent regression waiting for a reader. +func TestPlanJSON_DiffSyntheticChangesSurvive(t *testing.T) { + loaded := Loaded{Compiled: &manifest.Compiled{ + Payload: json.RawMessage(`{"name": "my-app", "artifactId": "68b0bbbb0000000000000002"}`), + ArtifactID: "68b0bbbb0000000000000002", + }} + + live := liveFrom(t, StateRunning, "", planLiveRuntime) + live.ArtifactID = "68a0000000000000000000a1" + + plan, err := Build(loaded, live, builtCode(0)) + require.NoError(t, err) + + require.Equal(t, []string{"artifactId"}, paths(plan.Artifact), + "the fixture has to reach Build's synthetic append for the test to mean anything") + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + var decoded PlanJSON + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + require.NotNil(t, decoded.Diff) + + require.Len(t, decoded.Diff.Changes, 1) + + change := decoded.Diff.Changes[0] + + assert.Equal(t, "artifactId", change.Path) + assert.Equal(t, "68a0000000000000000000a1", change.Have) + assert.Equal(t, "68b0bbbb0000000000000002", change.Want) + assert.False(t, change.Absent) +} + +// The type is the same kind of blind spot: the platform reads the +// discriminator off the artifact, so the spec walk never sees it. It rides in +// the synthetic append beside the id, and lands in the list the same way. +func TestPlanJSON_DiffSyntheticTypeChangeSurvives(t *testing.T) { + loaded := Loaded{Compiled: &manifest.Compiled{ + Payload: json.RawMessage(`{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "type": "agent", "spec": {}} + }`), + }} + + live := liveFrom(t, StateRunning, "", planLiveRuntime) + live.ArtifactType = "service" + + plan, err := Build(loaded, live, builtCode(0)) + require.NoError(t, err) + + require.Equal(t, []string{"artifact.type"}, paths(plan.Artifact)) + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + var decoded PlanJSON + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + require.NotNil(t, decoded.Diff) + require.Len(t, decoded.Diff.Changes, 1) + + change := decoded.Diff.Changes[0] + + assert.Equal(t, "artifact.type", change.Path) + assert.Equal(t, "service", change.Have) + assert.Equal(t, "agent", change.Want) +} diff --git a/internal/workload/up/run_test.go b/internal/workload/up/run_test.go index ac6f1f07a..e8069117d 100644 --- a/internal/workload/up/run_test.go +++ b/internal/workload/up/run_test.go @@ -594,6 +594,60 @@ func TestRun_DryRunDiffMatchesTheWetDiffAndWritesNothing(t *testing.T) { "the wet run must print the identical diff body before its progress;\ndry: %q\nwet: %q", dryStderr, wetStderr) } +// TestRun_PlanJSONDiffIsTheSameDryAndWet is that equivalence for the machine +// envelope: the diff section is computed from the pre-apply plan, so the dry +// preview and the run that follows it hand a consumer the same changes and +// the same unmanaged paths, in the same order. The action differs between +// the two; the section must not, or a script diffing the two envelopes would +// read drift into a deploy that fixed it. +func TestRun_PlanJSONDiffIsTheSameDryAndWet(t *testing.T) { + sized := resizedManifest() + + liveDocs := fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + } + + dry := liveDocs + dry.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + t.Fatal("a dry run must not touch the workload") + + return nil, nil + } + dry.replace = func(string, string, json.RawMessage) (*workload.Replacement, error) { + t.Fatal("a dry run must not roll anything") + + return nil, nil + } + + install(t, dry) + + dryResult, _, err := runIn(t, sized, Options{NonInteractive: true, DryRun: true, Diff: true}) + require.NoError(t, err) + + dryDiff := dryResult.Plan.JSONWithDiff().Diff + require.NotNil(t, dryDiff) + require.NotEmpty(t, dryDiff.Changes, + "the fixture has to move something for the comparison to mean anything") + + var resized bool + + wet := liveDocs + wet.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + resized = true + + return nil, nil + } + + install(t, wet) + + wetResult, _, err := runIn(t, sized, Options{NonInteractive: true, Diff: true, Detach: true}) + require.NoError(t, err) + assert.True(t, resized, "the wet leg has to reach the mutation for the comparison to mean anything") + + assert.Equal(t, dryDiff, wetResult.Plan.JSONWithDiff().Diff) +} + // TestRun_AlreadyUpToDate exits without touching anything, which is what // makes `up` cheap to run on every push. func TestRun_AlreadyUpToDate(t *testing.T) { From 9d527e36f156d4e73efc5ba2fa66ac21d91b4dbd Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Fri, 28 Aug 2026 19:16:57 -0700 Subject: [PATCH 07/12] [RAPTOR-19538] fix(workload): gate the --diff code block on code actually changing diffActionLines handed plan.Code to codeBlock unconditionally, so a plan whose drift was elsewhere -- runtime sizing, say, with a tree matching the last deploy -- grew a stray "~ code 0 files changed since the last deploy" line naming a sync the run was never going to perform. The block now exists only when Code.Changed(). The same fix settles which predicate gates the file list. IsEmpty() counts Downloads and Conflicts, which a deploy never pulls, so a plan holding only those would have rendered a file list for a sync that will not happen; the fallback now keys on len(Uploads)+len(Deletes), the same count Files carries, and the block is documented as one predicate throughout. --- internal/workload/up/render.go | 30 ++++++---- internal/workload/up/render_diff_test.go | 76 ++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 10 deletions(-) diff --git a/internal/workload/up/render.go b/internal/workload/up/render.go index 2dfcfbbb4..61c53e7b4 100644 --- a/internal/workload/up/render.go +++ b/internal/workload/up/render.go @@ -178,17 +178,27 @@ func diffActionLines(s Summary, plan Plan) ([]string, error) { return out, nil } -// codeBlock is the diff's code section. A first deploy has no sync plan to -// list, because nothing was ever uploaded to compare the tree against, so it -// keeps the default plan's wording. Otherwise the section is the file list -// the sync would upload, drawn by the sync command's own plan printer and fed -// the dry-run plan the code-change seam carried: two formats for one upload -// would eventually disagree, which is the whole reason the list is borrowed -// rather than redrawn. A plan measured without its list -- no production path -// makes one, but a harness wiring only the count does -- falls back to the -// count rather than printing nothing about real drift. +// codeBlock is the diff's code section, and it exists only for code the +// deploy will push: an entry saying nothing moved, or a file list for a sync +// the run will not perform, would both invent work. The gate is Changed(), +// one predicate for the whole block, because Files counts exactly the plan's +// Uploads and Deletes -- the rows a deploy pushes. IsEmpty() would also count +// Downloads and Conflicts, which the deploy never pulls, and would then print +// a file list for a tree the run leaves alone. A first deploy has no sync +// plan to list, because nothing was ever uploaded to compare the tree +// against, so it keeps the default plan's wording. Otherwise the section is +// the file list the sync would upload, drawn by the sync command's own plan +// printer and fed the dry-run plan the code-change seam carried: two formats +// for one upload would eventually disagree, which is the whole reason the +// list is borrowed rather than redrawn. A plan measured without its list -- +// no production path makes one, but a harness wiring only the count does -- +// falls back to the count rather than printing nothing about real drift. func codeBlock(code CodeChange) ([]string, error) { - if code.FirstDeploy || code.SyncPlan == nil || code.SyncPlan.IsEmpty() { + if !code.Changed() { + return nil, nil + } + + if code.FirstDeploy || code.SyncPlan == nil || len(code.SyncPlan.Uploads)+len(code.SyncPlan.Deletes) == 0 { return []string{entry("~", "code", codeDetail(code))}, nil } diff --git a/internal/workload/up/render_diff_test.go b/internal/workload/up/render_diff_test.go index e30681246..4be20d206 100644 --- a/internal/workload/up/render_diff_test.go +++ b/internal/workload/up/render_diff_test.go @@ -527,6 +527,82 @@ func TestRenderDiff_CodeCountWithoutAPlanKeepsTheSummaryLine(t *testing.T) { assert.Contains(t, out, "~ code 14 files changed since the last deploy") } +// TestRenderDiff_UnchangedCodeRendersNoCodeEntry is the regression for the +// guard the file-list feature dropped: a plan whose drift is elsewhere -- +// here the runtime sizing -- with a tree that matches the last deploy must +// not grow a code entry, because "0 files changed since the last deploy" +// names a sync the run is never going to perform. A manifest naming an +// image has no code to sync at all, and gets the same silence. +func TestRenderDiff_UnchangedCodeRendersNoCodeEntry(t *testing.T) { + sizingOnly := Plan{ + State: StateRunning, + Code: builtCode(0), + Runtime: []Change{ + {Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}, + }, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + } + + publishedImage := Plan{ + State: StateRunning, + Code: CodeChange{Applies: false, Files: 12}, + Runtime: []Change{ + {Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}, + }, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + } + + for name, plan := range map[string]Plan{"unchanged tree": sizingOnly, "published image": publishedImage} { + t.Run(name, func(t *testing.T) { + out := renderDiff(t, appSummary, plan) + + assert.NotContains(t, out, "~ code") + assert.NotContains(t, out, "files changed since the last deploy") + assert.NotContains(t, out, "Sync plan:") + }) + } +} + +// TestRenderDiff_DownloadOnlySyncPlanRendersNoFileList pins the one +// predicate the code block is gated on. The deploy pushes the plan's Uploads +// and Deletes and never pulls the remote side, so a dry-run plan holding +// only downloads and conflicts -- which IsEmpty() counts and Files does not +// -- must not render a file list for a sync the run will not perform. +func TestRenderDiff_DownloadOnlySyncPlanRendersNoFileList(t *testing.T) { + plan := Plan{ + State: StateRunning, + Code: CodeChange{ + Applies: true, + SyncPlan: &sync.SyncPlan{ + Downloads: []sync.FileAction{ + { + Path: "remote-only.txt", + Classification: sync.ClsRemoteModified, + Action: sync.ActDownloadModify, + RemoteSize: 99, + }, + }, + }, + }, + Runtime: []Change{ + {Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}, + }, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.NotContains(t, out, "Sync plan:") + assert.NotContains(t, out, "remote-only.txt") + assert.NotContains(t, out, "~ code") +} + // TestRender_DefaultModeBaselineIsUnchanged pins the default plan's exact // bytes against a baseline captured before --diff existed. The renderer grew // fields that carry the diff rows, and this is the proof that Render neither From 38e3bcbebddd934a950bacd2c60f4f5e3c13a960 Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Fri, 28 Aug 2026 19:20:00 -0700 Subject: [PATCH 08/12] [RAPTOR-19538] fix(workload): scrub env-var values from whole-element JSON diff rows Path-based redaction in changeJSON only caught rows whose own path sits inside an environmentVars list. But DiffRows emits a whole-element row for every NEW name-keyed list element: a new container is one row whose path names the container, with the entire container map as want, so its environmentVars block (plaintext literals and dr-credential refs alike) serialised raw into plan.diff.changes. Rather than dropping the whole entry, which would hide the element's other fields from a consumer that has to review the plan, the subtree is scrubbed before it marshals: variable names survive, every value does not, the copy never mutates the rows the human diff still reads, and the entry carries the redacted marker. The regression pins a new-container fixture carrying both a credential ref and a plaintext value, asserting neither reaches the serialised envelope. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- internal/workload/up/render.go | 119 +++++++++++++++++++++++++++- internal/workload/up/render_test.go | 69 ++++++++++++++-- 2 files changed, 179 insertions(+), 9 deletions(-) diff --git a/internal/workload/up/render.go b/internal/workload/up/render.go index 61c53e7b4..06b58fe63 100644 --- a/internal/workload/up/render.go +++ b/internal/workload/up/render.go @@ -57,7 +57,16 @@ const detailLimit = 6 // environment variable can be a secret someone pasted in plaintext, and a // plan that echoed it would put it in terminal scrollback and CI logs. Names // are enough to see what changed. -const envVarsSegment = ".environmentVars[" +const ( + // envVarsKey is the block's own key in a decoded manifest, the one the + // subtree scrub looks for: a whole-element row's path names the element + // around it, so catching secrets below it means reading the value, not + // the path. + envVarsKey = "environmentVars" + + // envVarsSegment is how envVarsKey reads inside a leaf's path. + envVarsSegment = "." + envVarsKey + "[" +) // Render writes the plan block that `up` prints before it acts, and that // --dry-run prints instead of acting. @@ -842,6 +851,16 @@ func changesJSON(artifact, runtime []DiffRow) []ChangeJSON { // withholds them, because a machine-readable plan is the one that ends up in // CI artifacts; the entry still says which path moved, so the change itself // is not silently lost to the refusal. +// +// Path-based redaction alone is not enough. A NEW name-keyed list element +// arrives as one whole-element row (a new container is one row for the +// container, not one per field), so the row's own path names the container +// and never trips redacted() while its value carries an environmentVars +// block complete with secrets. Of the two ways to close that leak, dropping +// the whole entry would hide the element's other fields from a consumer that +// has to review the plan, so the subtree is scrubbed instead: variable names +// survive, values do not, and the scrub copies rather than mutates because +// the same rows feed the human diff after this. func changeJSON(leaf DiffRow) ChangeJSON { out := ChangeJSON{ Path: leaf.Path, @@ -854,6 +873,104 @@ func changeJSON(leaf DiffRow) ChangeJSON { out.Have = nil out.Want = nil out.Redacted = true + + return out + } + + if carriesEnvVars(leaf.Want) || carriesEnvVars(leaf.Have) { + out.Want = scrubEnvVars(leaf.Want) + out.Have = scrubEnvVars(leaf.Have) + out.Redacted = true + } + + return out +} + +// carriesEnvVars reports whether a value holds an environmentVars block +// anywhere below it, which is what turns a whole-element row into a leak +// however innocent its own path reads. +func carriesEnvVars(v any) bool { + switch typed := v.(type) { + case map[string]any: + if _, ok := typed[envVarsKey]; ok { + return true + } + + for _, child := range typed { + if carriesEnvVars(child) { + return true + } + } + case []any: + for _, child := range typed { + if carriesEnvVars(child) { + return true + } + } + } + + return false +} + +// scrubEnvVars deep-copies a composite value with every environment variable +// reduced to its name. Everything else keeps its structure, so the entry +// still says what the element carries and a consumer can tell a container +// apart from its variables; only the refuse-to-print part is dropped. +func scrubEnvVars(v any) any { + switch typed := v.(type) { + case map[string]any: + out := make(map[string]any, len(typed)) + + for key, child := range typed { + if key == envVarsKey { + out[key] = scrubEnvList(child) + + continue + } + + out[key] = scrubEnvVars(child) + } + + return out + case []any: + out := make([]any, len(typed)) + + for i, child := range typed { + out[i] = scrubEnvVars(child) + } + + return out + default: + return v + } +} + +// scrubEnvList copies one environmentVars block, keeping the names and +// nothing else. A name is the part a reader acts on; every other key of an +// element is a value or the address of one, whether a plaintext literal or a +// dr-credential ref, so none of it survives the copy. An element without a +// usable name has nothing safe to say and is dropped rather than passed +// through. +func scrubEnvList(v any) []any { + list, ok := v.([]any) + if !ok { + return nil + } + + out := make([]any, 0, len(list)) + + for _, item := range list { + element, ok := item.(map[string]any) + if !ok { + continue + } + + name, ok := element["name"].(string) + if !ok || name == "" { + continue + } + + out = append(out, map[string]any{"name": name}) } return out diff --git a/internal/workload/up/render_test.go b/internal/workload/up/render_test.go index 332e09d10..7f42fcfff 100644 --- a/internal/workload/up/render_test.go +++ b/internal/workload/up/render_test.go @@ -692,12 +692,20 @@ func TestPlanJSON_DiffCarriesTheChangingLeaves(t *testing.T) { // names one; both ride in the diff rows as raw values, so the values are // withheld here rather than trusted to a marshalling order, and the entry // carries a marker saying the refusal happened. +// +// The sidecar container pins the whole-element half of the rule: a NEW +// name-keyed list element is one row for the element, not one per field, so +// the row's own path names the container and never trips the path-based +// redaction while its value carries an environmentVars block complete with +// secrets. Nothing a variable holds may reach the document. func TestPlanJSON_DiffRedactsBeforeSerialising(t *testing.T) { const ( - literal = "sk-literal-plaintext" - oldLit = "sk-old-plaintext" - rotated = "aaaa222222222222222222aa" - liveID = "66f1a2b3c4d5e6f7a8b9c0d1" + literal = "sk-literal-plaintext" + oldLit = "sk-old-plaintext" + rotated = "aaaa222222222222222222aa" + liveID = "66f1a2b3c4d5e6f7a8b9c0d1" + sidecarLit = "hunter2-plaintext" + sidecarCredID = "77c2b3c4d5e6f7a8b9c0d199" ) payload := `{ @@ -709,6 +717,11 @@ func TestPlanJSON_DiffRedactsBeforeSerialising(t *testing.T) { {"name": "OPENAI_API_KEY", "value": "` + literal + `"}, {"name": "HUGGING_FACE_HUB_TOKEN", "source": "dr-credential", "drCredentialId": "` + rotated + `", "key": "apiToken"} + ]}, + {"name": "sidecar", "image": "sidecar:2", "environmentVars": [ + {"name": "SIDECAR_PASSWORD", "value": "` + sidecarLit + `"}, + {"name": "SIDECAR_API_TOKEN", "source": "dr-credential", + "drCredentialId": "` + sidecarCredID + `", "key": "sidecarKey"} ]} ]}] }} @@ -728,11 +741,13 @@ func TestPlanJSON_DiffRedactsBeforeSerialising(t *testing.T) { plan, err := Build(loadedFrom(payload), liveFrom(t, StateRunning, live, planLiveRuntime), builtCode(0)) require.NoError(t, err) - // The fixture has to earn its keep: two env-var changes are what the walk - // found, and they are what must not leak. + // The fixture has to earn its keep: two env-var changes on the existing + // container plus the whole-element row the new container emits, and all + // three are what must not leak. require.Equal(t, []string{ "containerGroups[default].containers[primary].environmentVars[OPENAI_API_KEY].value", "containerGroups[default].containers[primary].environmentVars[HUGGING_FACE_HUB_TOKEN].drCredentialId", + "containerGroups[default].containers[sidecar]", }, paths(plan.Artifact)) encoded, err := json.Marshal(plan.JSONWithDiff()) @@ -740,7 +755,7 @@ func TestPlanJSON_DiffRedactsBeforeSerialising(t *testing.T) { document := string(encoded) - for _, secret := range []string{literal, oldLit, rotated, liveID, "dr-credential:"} { + for _, secret := range []string{literal, oldLit, rotated, liveID, sidecarLit, sidecarCredID, "dr-credential:"} { assert.NotContains(t, document, secret) } @@ -750,17 +765,55 @@ func TestPlanJSON_DiffRedactsBeforeSerialising(t *testing.T) { diff := decoded["diff"].(map[string]any) changes := diff["changes"].([]any) - require.Len(t, changes, 2) + require.Len(t, changes, 3) for _, c := range changes { entry := c.(map[string]any) + if entry["path"] == "containerGroups[default].containers[sidecar]" { + continue + } + assert.Contains(t, entry["path"], "environmentVars[", "the name is the whole point of the entry") assert.Nil(t, entry["have"], "the live value never serialises") assert.Nil(t, entry["want"], "the asked-for value never serialises") assert.Equal(t, true, entry["redacted"]) } + // The new container's entry arrives as a whole-element row: the path + // names the container, so the scrub works on the subtree instead. The + // names of its variables stay, because seeing which variables an element + // sets is half the point of the entry; every value, literal or + // credential ref, is gone, and the refusal is marked. + var sidecar map[string]any + + for _, c := range changes { + entry, _ := c.(map[string]any) + + if entry["path"] == "containerGroups[default].containers[sidecar]" { + sidecar = entry + + break + } + } + + require.NotNil(t, sidecar, "the new container is one whole-element change") + assert.Equal(t, true, sidecar["redacted"]) + assert.Nil(t, sidecar["have"], "the container is an addition") + assert.Equal(t, true, sidecar["absent"]) + + want, ok := sidecar["want"].(map[string]any) + require.True(t, ok, "the container's other fields keep their structure") + + assert.Equal(t, "sidecar:2", want["image"], "nothing secret keeps the structure honest") + + vars, ok := want["environmentVars"].([]any) + require.True(t, ok, "the variable names stay") + require.Len(t, vars, 2) + + assert.Equal(t, map[string]any{"name": "SIDECAR_PASSWORD"}, vars[0]) + assert.Equal(t, map[string]any{"name": "SIDECAR_API_TOKEN"}, vars[1]) + // The legacy arrays were redacted before this section existed, and stay // that way: they say the change happened, never what it said. plain := plan.JSON() From eb4183e70b8ad53b3dc463a06ea204fac6cf0bcc Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Fri, 28 Aug 2026 23:51:03 -0700 Subject: [PATCH 09/12] [RAPTOR-19538] feat(workload): add the --confirm gate to dr workload up An interactive run asked with --confirm now prints the plan (or the diff), then asks '? Apply this deploy? (y/N)' on stderr and deploys only on an affirmative; anything else, including an empty answer, declines, and the run returns up.ErrDeclined having touched nothing. The gate fires whenever the run would otherwise mutate -- a pending plan, or an empty one with --lock waiting to make the serving artifact permanent -- and never on a dry run or a wholly empty one, because unmanaged fields are never mutated either. The question is installed only when stdin is a terminal and no non-interactive signal is set (--yes, --output-format json, DATAROBOT_CLI_NON_INTERACTIVE): suppression composes, so there is deliberately no cobra mutual exclusivity, and the flag help documents the matrix. The locked-production typed confirm still fires after an accepted y/N, asking its own question. Both flags are reported to telemetry under their own keys, and the command Long names --dry-run --diff as the look-without-touching combination. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- CHANGELOG.md | 1 + cmd/workload/up/cmd.go | 79 +++- cmd/workload/up/cmd_test.go | 300 ++++++++++++ internal/workload/up/confirm_test.go | 651 +++++++++++++++++++++++++++ internal/workload/up/run.go | 68 +++ 5 files changed, 1092 insertions(+), 7 deletions(-) create mode 100644 internal/workload/up/confirm_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 226f9137f..7f14ff7a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - `dr workload status`, `dr workload get`, `dr workload endpoint` and `dr workload logs` now take the workload id as an optional argument. Left out, it is read from the `workloadId` in the nearest `.datarobot.yaml`, searched upward from the new `--dir` flag (the current directory by default), so the commands `dr workload up` points you at can be run as printed from the project it just deployed. A typed id still wins, and is used without reading any manifest. The workload that was picked is named on stderr, except under `--output-format json`, where anything on stderr would break `2>&1 | jq .`. - `dr workload stop`, `dr workload start` and `dr workload delete` take the workload id optionally too, on the same terms. Because they change something, a workload named by the manifest rather than by you is confirmed first; a typed id is never questioned. `--yes` skips the question, and `stop` and `start` gained that flag for this. `DATAROBOT_CLI_NON_INTERACTIVE=1` also skips it on `stop` and `start`, but not on a manifest-named `delete`: that variable is set once across a pipeline, and deleting something nobody named is not what it was set for. - `dr workload delete --dir `: which project's manifest holds the binding to clear, matching the flag `up` and `config` already take. +- `dr workload up --diff` and `dr workload up --confirm`: the plan can now be reviewed as a unified diff, with context around each change and nothing truncated, and a run asked with `--confirm` deploys only after a `y` to `? Apply this deploy? (y/N)` on stderr, declining having changed nothing. `--dry-run --diff` is the look-without-touching combination, and when the run is non-interactive (`--yes`, `--output-format json`, `DATAROBOT_CLI_NON_INTERACTIVE`, or stdin that is not a terminal) `--confirm` is suppressed and behaves as if it was not given, because a question nobody can answer would hang the run instead of keeping it safe. ## Fixed diff --git a/cmd/workload/up/cmd.go b/cmd/workload/up/cmd.go index 600378610..df1f3cad6 100644 --- a/cmd/workload/up/cmd.go +++ b/cmd/workload/up/cmd.go @@ -82,13 +82,14 @@ func buildID(id string) *string { } type flags struct { - dir string - yes bool - dryRun bool - diff bool - detach bool - lock bool - force bool + dir string + yes bool + dryRun bool + diff bool + confirm bool + detach bool + lock bool + force bool // bindingFlags exist only to be refused. Cobra's own "unknown flag" // message would leave the user guessing where binding lives, and these @@ -149,6 +150,13 @@ allocation, is applied in place instead. Nothing is built and no version is made, because what the workload runs has not changed. A deploy that moves both sends the sizing with the rollout, so the new version comes up with it. +Two flags are for looking before leaping. --diff prints the plan as a unified +diff, with context around each change and nothing truncated, instead of the +changed-fields summary. --confirm asks '? Apply this deploy? (y/N)' on stderr +after the plan is printed and deploys only on yes; when the run is +non-interactive it is suppressed and behaves as if it was not given. To look +without touching, --dry-run --diff is the combination. + A workload that is not ready to be deployed onto is dealt with rather than refused. One still starting or stopping is waited out and then re-read, so the plan is built against where it landed. A stopped one is started and then @@ -181,6 +189,7 @@ Examples: "yes": nonInteractive, "dry_run": f.dryRun, "diff": f.diff, + "confirm": f.confirm, "detach": f.detach, "lock": f.lock, "force_build": f.force, @@ -204,6 +213,15 @@ func addFlags(cmd *cobra.Command, f *flags, poll *pollflags.Set) { "Render the plan as a unified diff instead of the changed-fields list: "+ "every field the file names appears, unchanged ones as context that collapses when it runs long, "+ "and nothing is truncated. Combine with --dry-run to look without touching.") + // The suppression is documented in the flag itself because it is the one + // surprise the flag carries: someone piping output and passing --confirm + // would otherwise wait on a prompt that never comes, with no word of why. + cmd.Flags().BoolVar(&f.confirm, "confirm", false, + "Ask '? Apply this deploy? (y/N)' on stderr after printing the plan, and deploy only on yes; "+ + "anything else, including an empty answer, declines and changes nothing. "+ + "Suppressed when the run is non-interactive: --yes, --output-format json, "+ + "DATAROBOT_CLI_NON_INTERACTIVE, or a stdin that is not a terminal. "+ + "When suppressed it behaves as if the flag was not given.") cmd.Flags().BoolVar(&f.detach, "detach", false, "Return once the deploy is requested; do not wait for it to serve.") cmd.Flags().BoolVar(&f.lock, "lock", false, "Lock whichever artifact ends up live, making it permanent, even when this deploy minted no new "+ @@ -252,6 +270,7 @@ func run(cmd *cobra.Command, f flags, poll pollflags.Set, format outputformat.Ou Detach: f.detach, Lock: f.lock, Confirm: rollConfirm(cmd, yes), + ConfirmApply: applyConfirm(cmd, f.confirm, nonInteractive), ForceBuild: f.force, PollInterval: poll.Interval, PollTimeout: poll.Timeout, @@ -259,6 +278,15 @@ func run(cmd *cobra.Command, f flags, poll pollflags.Set, format outputformat.Ou Spinner: !json && !nonInteractive, }) + if errors.Is(runErr, up.ErrDeclined) { + // The gate said no. Nothing has been touched, so there is no endpoint + // to print and no envelope to emit: the refusal itself is the whole + // outcome, and it reaches stderr through the same path every error + // takes. Falling through to render here would print the endpoint of a + // workload this run deliberately left alone. + return runErr + } + if runErr != nil && !reportable(result) { return runErr } @@ -325,6 +353,43 @@ func rollConfirm(cmd *cobra.Command, yes bool) func(question, want string) (bool return typedConfirm(cmd) } +// applyConfirm is the opt-in y/N gate behind --confirm, and nil when there is +// nobody to answer it. Suppressed, not refused: a run that cannot be asked +// behaves as if the flag was not given, which is the convention every prompt +// on this command follows. --yes is already the answer, -o json and a piped +// stdin mean nobody is reading, and DATAROBOT_CLI_NON_INTERACTIVE says the +// same; refusing the combination would break a scripted caller to protect a +// default that suppression already keeps safe. There is deliberately no cobra +// mutual exclusivity with --yes for the same reason. +// +// The question goes to stderr and the answer comes from stdin, like the typed +// confirm below: stdout is the endpoint, or one JSON document, and a question +// printed into it would break whatever is parsing it. +func applyConfirm(cmd *cobra.Command, confirm, nonInteractive bool) func(string) (bool, error) { + if !confirm || nonInteractive { + return nil + } + + return func(question string) (bool, error) { + fmt.Fprintf(cmd.ErrOrStderr(), "? %s (y/N) ", question) + + scanner := bufio.NewScanner(cmd.InOrStdin()) + if !scanner.Scan() { + if err := scanner.Err(); err != nil { + return false, fmt.Errorf("cannot read the answer: %w", err) + } + + return false, nil + } + + // The default is no, so an enter pressed twice in a row, a stray + // keystroke and a closed pipe can never deploy on their own. + answer := strings.ToLower(strings.TrimSpace(scanner.Text())) + + return answer == "y" || answer == "yes", nil + } +} + // typedConfirm asks a question that only the exact expected word answers. // // It goes to stderr and reads a single line, like everything else the deploy diff --git a/cmd/workload/up/cmd_test.go b/cmd/workload/up/cmd_test.go index 9b48b625b..e5d4d49cc 100644 --- a/cmd/workload/up/cmd_test.go +++ b/cmd/workload/up/cmd_test.go @@ -816,4 +816,304 @@ func TestCmd_FailedDraftRunOmitsTheLockLine(t *testing.T) { assert.NotContains(t, next, "--lock") assert.Contains(t, next, "dr workload logs 68b0c1d2e3f4a5b6c7d8e9f0", "the lines that can name the workload still do") +// TestCmd_ConfirmIsRegistered: the flag exists, is opt-in, and its help says +// the one thing a reader could not guess -- that under non-interactive +// conditions it is suppressed rather than refused, because a flag that fails +// a scripted run helps nobody. +func TestCmd_ConfirmIsRegistered(t *testing.T) { + lookup := Cmd().Flags().Lookup("confirm") + + require.NotNil(t, lookup, "the flag has to exist for --confirm to parse at all") + assert.False(t, lookup.Hidden) + assert.Equal(t, "false", lookup.DefValue, "confirming is opt-in") + + for _, term := range []string{ + "(y/N)", "--yes", "--output-format", "DATAROBOT_CLI_NON_INTERACTIVE", + "terminal", "suppressed", + } { + assert.Contains(t, lookup.Usage, term, "the suppression matrix is documented in the flag itself") + } +} + +// TestCmd_ConfirmReachesTheDeploy threads the flag into the deploy's options, +// and only when it was given: a run without it must not start asking. +func TestCmd_ConfirmReachesTheDeploy(t *testing.T) { + onATerminal(t) + + seen := stubRun(t, deployed(), nil) + + _, _, err := runCmdWithInput(t, "y\n", "--confirm") + require.NoError(t, err) + require.NotNil(t, seen.ConfirmApply, "the gate reaches the deploy wired up") + + agreed, err := seen.ConfirmApply("Apply this deploy?") + require.NoError(t, err) + assert.True(t, agreed, "the answer is read from stdin") + + defaults := stubRun(t, deployed(), nil) + + _, _, err = runCmd(t) + require.NoError(t, err) + assert.Nil(t, defaults.ConfirmApply, "without the flag there is no gate") +} + +// TestCmd_ConfirmGate_AnswersOnlyYes: the question is a y/N with the default +// on the safe side. Every spelling of yes proceeds; bare Enter, any other +// word, and a stdin that ends before an answer all decline. +func TestCmd_ConfirmGate_AnswersOnlyYes(t *testing.T) { + cases := []struct { + typed string + want bool + }{ + {"y\n", true}, + {"Y\n", true}, + {"yes\n", true}, + {"YES\n", true}, + {"Yes\n", true}, + {"yEs\n", true}, + {" y \n", true}, + {"\n", false}, + {"", false}, + {"n\n", false}, + {"N\n", false}, + {"no\n", false}, + {"nope\n", false}, + {"deploy\n", false}, + } + + for _, c := range cases { + t.Run(c.typed, func(t *testing.T) { + cmd := Cmd() + + var errOut bytes.Buffer + + cmd.SetErr(&errOut) + cmd.SetIn(strings.NewReader(c.typed)) + + ask := applyConfirm(cmd, true, false) + require.NotNil(t, ask) + + agreed, err := ask("Apply this deploy?") + require.NoError(t, err) + + assert.Equal(t, c.want, agreed) + assert.Contains(t, errOut.String(), "? Apply this deploy? (y/N)", + "the question goes to stderr with the default shown") + }) + } +} + +// TestCmd_ConfirmGate_NotInstalledWhenSuppressed: the builder itself is nil +// whenever there is nobody to ask, which is the suppression-over-error +// convention the roll confirm follows. +func TestCmd_ConfirmGate_NotInstalledWhenSuppressed(t *testing.T) { + cmd := Cmd() + + assert.Nil(t, applyConfirm(cmd, false, false), "no flag, no gate") + assert.Nil(t, applyConfirm(cmd, true, true), "non-interactive swallows the flag") + assert.Nil(t, applyConfirm(cmd, false, true), "and neither installs the other") +} + +// --yes is already the answer, so --confirm alongside it behaves as if the +// flag was not given: no question, the same deploy. +func TestCmd_ConfirmSuppressed_Yes(t *testing.T) { + onATerminal(t) + + seen := stubRun(t, deployed(), nil) + + stdout, stderr, err := runCmd(t, "--yes", "--confirm") + require.NoError(t, err) + + assert.Nil(t, seen.ConfirmApply, "--yes is the answer, so there is no gate to install") + assert.NotContains(t, stderr, "? Apply") + assert.Equal(t, "https://app.datarobot.com/workloads/68b0/\n", stdout, + "the suppressed run is byte-identical to one without --confirm") +} + +// The non-interactive environment variable says nobody is reading, and a +// question nobody reads would hang a pipeline forever. +func TestCmd_ConfirmSuppressed_NonInteractiveEnv(t *testing.T) { + onATerminal(t) + t.Setenv("DATAROBOT_CLI_NON_INTERACTIVE", "1") + + seen := stubRun(t, deployed(), nil) + + _, stderr, err := runCmd(t, "--confirm") + require.NoError(t, err) + + assert.Nil(t, seen.ConfirmApply) + assert.NotContains(t, stderr, "? Apply") +} + +// -o json implies non-interactive for the y/N gate specifically. The typed +// production confirm is a different question with different rules and is +// covered by TestCmd_JSONOutputStillHandsOverTheQuestion. +func TestCmd_ConfirmSuppressed_JSON(t *testing.T) { + onATerminal(t) + + seen := stubRun(t, deployed(), nil) + + stdout, stderr, err := runCmd(t, "--output-format", "json", "--confirm") + require.NoError(t, err) + + assert.Nil(t, seen.ConfirmApply) + assert.True(t, json.Valid([]byte(stdout)), "stdout stays one JSON document") + assert.NotContains(t, stderr, "Apply this deploy", "the y/N gate is the prompt that is suppressed") +} + +// A piped stdin has nobody behind it, which is the CI case even with no other +// non-interactive signal set. +func TestCmd_ConfirmSuppressed_PipedStdin(t *testing.T) { + seen := stubRun(t, deployed(), nil) + + _, stderr, err := runCmd(t, "--confirm") + require.NoError(t, err) + + assert.True(t, seen.NonInteractive) + assert.Nil(t, seen.ConfirmApply, "a piped stdin has nobody to ask") + assert.NotContains(t, stderr, "? Apply") +} + +// TestCmd_ConfirmInstallKeysOnStdinNotStderr: the gate follows stdin, not +// stderr. A run with a terminal stdin still asks when stderr is redirected, +// and the question lands in whatever the stderr is. +func TestCmd_ConfirmInstallKeysOnStdinNotStderr(t *testing.T) { + onATerminal(t) + decliningRun(t) + + stdout, stderr, err := runCmdWithInput(t, "y\n", "--confirm") + require.NoError(t, err) + + assert.Contains(t, stderr, "? Apply this deploy? (y/N)", "the question is on stderr") + assert.Equal(t, "https://app.datarobot.com/workloads/68b0/\n", stdout, + "stdout stays the endpoint channel") +} + +// decliningRun wires the deploy the way the real one answers the gate, so the +// prompt, the parser and the command's decline mapping are exercised as one +// flow rather than as three unit-tested pieces. +func decliningRun(t *testing.T) { + t.Helper() + + prev := runFn + + runFn = func(opts up.Options) (up.Result, error) { + ok, err := opts.ConfirmApply("Apply this deploy?") + if err != nil { + return up.Result{}, err + } + + if !ok { + // WorkloadID is set on purpose: a declined run of an existing + // workload is exactly the case where falling through to the + // renderer would leak the endpoint onto stdout. + return up.Result{WorkloadID: "68b0c1d2e3f4a5b6c7d8e9f0"}, up.ErrDeclined + } + + return deployed(), nil + } + + t.Cleanup(func() { runFn = prev }) +} + +// TestCmd_ConfirmDecline_NonzeroExit: a decline is a refusal, and the command +// says so with a nonzero exit and one short line on stderr. +func TestCmd_ConfirmDecline_NonzeroExit(t *testing.T) { + onATerminal(t) + decliningRun(t) + + for _, input := range []string{"\n", "n\n", "nope\n", ""} { + stdout, stderr, err := runCmdWithInput(t, input, "--confirm") + + require.Error(t, err, "answering %q must exit nonzero", input) + require.ErrorIs(t, err, up.ErrDeclined) + assert.Empty(t, stdout, "a declined run prints nothing on stdout") + assert.Contains(t, stderr, "declined", "the decline is said on stderr") + assert.Contains(t, stderr, "nothing was deployed") + assert.NotContains(t, stderr, "https://app.datarobot.com/workloads/68b0/", + "the endpoint of a workload the run left alone is not printed") + } +} + +// TestCmd_ConfirmDecline_StdoutEmpty is the stdout half on its own, because +// it is the half a script breaks on: whatever the decline's exit code, stdout +// must be byte-for-byte empty. +func TestCmd_ConfirmDecline_StdoutEmpty(t *testing.T) { + onATerminal(t) + decliningRun(t) + + stdout, _, err := runCmdWithInput(t, "n\n", "--confirm") + require.Error(t, err) + assert.Empty(t, stdout) +} + +// TestCmd_ConfirmAccept_ProceedsToEndOfRun: accepting the y/N runs the normal +// success path, which in the shell means the endpoint lands on stdout as it +// always does. +func TestCmd_ConfirmAccept_ProceedsToEndOfRun(t *testing.T) { + onATerminal(t) + decliningRun(t) + + stdout, stderr, err := runCmdWithInput(t, "y\n", "--confirm") + require.NoError(t, err) + assert.Contains(t, stderr, "? Apply this deploy? (y/N)") + assert.Equal(t, "https://app.datarobot.com/workloads/68b0/\n", stdout) +} + +// TestCmd_ConfirmHelpDocumentsSuppression: the Long paragraph and the flag +// help between them tell a first-time reader what --diff prints, what +// --confirm asks, and when the asking is skipped. +func TestCmd_ConfirmHelpDocumentsSuppression(t *testing.T) { + long := Cmd().Long + + assert.Contains(t, long, "--diff") + assert.Contains(t, long, "--confirm") + assert.Contains(t, long, "--dry-run --diff", "the look-without-touching combination is named") + assert.Contains(t, long, "Only fields the manifest mentions are managed", + "the pre-existing narrative stays") + + lookup := Cmd().Flags().Lookup("confirm") + require.NotNil(t, lookup) + assert.Contains(t, lookup.Usage, "behaves as if", "the suppressed semantics are stated, not implied") +} + +// TestCmd_ConfirmAndDiffCompose: the two flags are independent, and cobra is +// deliberately not taught to refuse the pair -- suppression composes, mutual +// exclusivity does not. +func TestCmd_ConfirmAndDiffCompose(t *testing.T) { + onATerminal(t) + + seen := stubRun(t, deployed(), nil) + + _, _, err := runCmd(t, "--diff", "--confirm", "--dry-run") + require.NoError(t, err, "the flags compose; refusing the pair would break a scripted caller") + assert.True(t, seen.Diff) + assert.True(t, seen.DryRun) + assert.NotNil(t, seen.ConfirmApply) + + for _, name := range []string{"diff", "confirm"} { + lookup := Cmd().Flags().Lookup(name) + require.NotNil(t, lookup) + + assert.NotContains(t, lookup.Annotations, "cobra_annotation_mutually_exclusive", + "--diff and --confirm are never made mutually exclusive") + } +} + +// TestCmd_TelemetryRecordsTheConfirmFlag: adoption of the flag has to be +// readable under its own key, reflecting the flag as given rather than the +// suppressed effective behavior. +func TestCmd_TelemetryRecordsTheConfirmFlag(t *testing.T) { + withFlag := Cmd() + require.NoError(t, withFlag.ParseFlags([]string{"--confirm"})) + + event, ok := telemetry.EventFor(withFlag, nil) + require.True(t, ok, "EventFor must return ok=true for an annotated command") + + assert.Equal(t, true, event.EventProperties["confirm"]) + assert.Equal(t, false, event.EventProperties["diff"], "the pre-existing keys keep their own values") + + event, ok = telemetry.EventFor(Cmd(), nil) + require.True(t, ok) + assert.Equal(t, false, event.EventProperties["confirm"], "an unset flag reports itself as off, not as absent") } diff --git a/internal/workload/up/confirm_test.go b/internal/workload/up/confirm_test.go new file mode 100644 index 000000000..df0d7291d --- /dev/null +++ b/internal/workload/up/confirm_test.go @@ -0,0 +1,651 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package up + +import ( + "bytes" + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/datarobot/cli/internal/workload" + "github.com/datarobot/cli/internal/workload/sync" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// runConfirmIn is runIn with the stderr buffer supplied by the test, so the +// gate's recorder can measure what had been printed by the time the question +// was asked. Placement is most of what the gate has to get right, and offsets +// are how a buffer answers questions about order. +func runConfirmIn(t *testing.T, content string, opts Options, stderr *bytes.Buffer) (Result, error) { + t.Helper() + + dir := t.TempDir() + writeManifest(t, dir, content) + + // The validator refuses a provided-source build with no Dockerfile beside + // the manifest, which is correct and means the build-track fixtures need + // a real one. Writing it unconditionally is harmless for the others. + require.NoError(t, os.WriteFile(filepath.Join(dir, "Dockerfile"), + []byte("FROM scratch\nEXPOSE 8080\n"), 0o600)) + + opts.Dir = dir + opts.Stderr = stderr + + return Run(opts) +} + +// liveDocs is the bound pair the empty-plan and retune fixtures deploy +// against, and the decline tests wire counters on top of it. +func liveDocs(t *testing.T) fakes { + return fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + } +} + +// TestRun_ConfirmPromptsAfterPlan is the go-test half of the prompt contract: +// the gate asks once, after the plan has been rendered, and the question is +// the one the y/N answer is graded against. The decline also pins the run's +// own half of the exit contract: ErrDeclined, from a run that has printed the +// plan and mutated nothing. +func TestRun_ConfirmPromptsAfterPlan(t *testing.T) { + install(t, liveDocs(t)) + + var ( + asked int + question string + printed int + ) + + var stderr bytes.Buffer + + result, err := runConfirmIn(t, resizedManifest(), Options{ + ConfirmApply: func(q string) (bool, error) { + asked++ + question = q + printed = stderr.Len() + + return false, nil + }, + }, &stderr) + + require.ErrorIs(t, err, ErrDeclined) + assert.Equal(t, 1, asked, "the gate is asked once, not once per branch") + assert.Equal(t, "Apply this deploy?", question) + assert.Contains(t, stderr.String()[:printed], "-> ", + "the default plan precedes the question: consent follows the review") + assert.Equal(t, ActionUnchanged, result.Action, "a decline did nothing, so nothing is reported") +} + +// TestRun_ConfirmGate_AfterDryRunReturn: a preview is not a mutation to +// consent to, so the dry-run return comes first and the gate is never armed. +func TestRun_ConfirmGate_AfterDryRunReturn(t *testing.T) { + f := liveDocs(t) + f.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + t.Fatal("a dry run must not touch the workload") + + return nil, nil + } + + install(t, f) + + var asked bool + + result, stderr, err := runIn(t, resizedManifest(), Options{ + NonInteractive: true, + DryRun: true, + ConfirmApply: func(string) (bool, error) { asked = true; return false, nil }, + }) + + require.NoError(t, err) + assert.False(t, asked, "dry-run never prompts") + assert.Equal(t, ActionUpdated, result.Action, "the dry run's action is still the plan's own") + assert.Contains(t, stderr, "-> ", "the plan is printed as usual") +} + +// TestRun_ConfirmGate_AfterNoteUnusedForce: the --force-build note is part of +// what the user reviews, so it is said before the question, not after it. +func TestRun_ConfirmGate_AfterNoteUnusedForce(t *testing.T) { + install(t, liveDocs(t)) + + var printed int + + var stderr bytes.Buffer + + _, err := runConfirmIn(t, resizedManifest(), Options{ + ForceBuild: true, + ConfirmApply: func(string) (bool, error) { printed = stderr.Len(); return false, nil }, + }, &stderr) + + require.ErrorIs(t, err, ErrDeclined) + + note := strings.Index(stderr.String(), "--force-build had no effect") + require.NotEqual(t, -1, note, "the unused-force note is part of the output") + assert.Less(t, note, printed, "the note precedes the question") +} + +// TestRun_ConfirmAccept_Proceeds: yes means the run continues through the +// normal apply path exactly as if the gate had never been installed. The seam +// sequence is captured twice -- once without the gate, once accepting it -- +// and the two must agree, because --confirm is a question, not a new plan. +func TestRun_ConfirmAccept_Proceeds(t *testing.T) { + seams := func(seen *[]string) fakes { + f := liveDocs(t) + f.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + *seen = append(*seen, "resize") + + return nil, nil + } + + return f + } + + var baseline []string + + install(t, seams(&baseline)) + + _, _, err := runIn(t, resizedManifest(), Options{NonInteractive: true, Detach: true}) + require.NoError(t, err) + require.NotEmpty(t, baseline, "the fixture has to reach the resize for the comparison to mean anything") + + var accepted []string + + install(t, seams(&accepted)) + + result, _, err := runIn(t, resizedManifest(), Options{ + Detach: true, + ConfirmApply: func(string) (bool, error) { return true, nil }, + }) + + require.NoError(t, err) + assert.Equal(t, baseline, accepted, "accepting the gate proceeds exactly as a run without it") + assert.Equal(t, ActionUpdated, result.Action) +} + +// countingFakes wires a named marker into every seam the contract counts as a +// mutation, so a test asserting len(calls) == 0 is asserting the run touched +// none of them. +func countingFakes(calls *[]string) func(f fakes) fakes { + mark := func(name string) { *calls = append(*calls, name) } + + return func(f fakes) fakes { + f.cred = func(string) (*workload.Credential, error) { mark("cred"); return nil, nil } + f.findCredential = func(string, int) (*workload.Credential, error) { mark("findCredential"); return nil, nil } + f.guard = func(string) error { mark("guard"); return nil } + f.lock = func(id string) (*workload.Artifact, error) { + mark("lock:" + id) + + return &workload.Artifact{ID: id, Status: workload.ArtifactStatusLocked}, nil + } + f.create = func(any) (*workload.Workload, error) { mark("create"); return running("wl-new"), nil } + f.start = func(string) (*workload.WorkloadOperationResponse, error) { mark("start"); return nil, nil } + f.writeID = func(string, string) error { mark("writeID"); return nil } + f.build = func(string) (*workload.BuildTriggerResponse, error) { mark("build"); return nil, nil } + f.codeRef = func(string, string, string) error { mark("codeRef"); return nil } + f.sync = func(string) (*sync.Result, error) { mark("sync"); return nil, nil } + f.settings = func(string, json.RawMessage) (*workload.Replacement, error) { mark("resize"); return nil, nil } + f.replace = func(string, string, json.RawMessage) (*workload.Replacement, error) { mark("replace"); return nil, nil } + + return f + } +} + +// TestRun_ConfirmDecline_MutatesNothing is the promise the gate exists to +// keep: every decline answer stops the run with ErrDeclined and not one +// mutating seam has fired. The plan is printed first -- the user is declining +// something they read -- and the action stays "nothing". An answer that +// cannot be read at all is a different failure: the run stops too, but it is +// not a decline, so the sentinel stays out of it. +func TestRun_ConfirmDecline_MutatesNothing(t *testing.T) { + cases := []struct { + name string + answer bool + readErr error + declined bool + }{ + {name: "no", answer: false, declined: true}, + {name: "unreadable", readErr: errors.New("stdin closed")}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + var calls []string + + f := countingFakes(&calls)(liveDocs(t)) + + install(t, f) + + result, stderr, err := runIn(t, resizedManifest(), Options{ + ConfirmApply: func(string) (bool, error) { return c.answer, c.readErr }, + }) + + require.Error(t, err) + + if c.declined { + require.ErrorIs(t, err, ErrDeclined) + } else { + require.NotErrorIs(t, err, ErrDeclined, + "a failed read is not a decline, and must not read as one") + assert.Contains(t, err.Error(), "stdin closed") + } + + assert.Empty(t, calls, "neither answer reaches any mutating seam") + assert.Contains(t, stderr, "-> ", "the plan precedes the question") + assert.Equal(t, ActionUnchanged, result.Action) + }) + } +} + +// TestRun_ConfirmDecline_NoApplyMutation names the apply half of the +// non-mutation promise explicitly: on a non-empty plan, declining stops +// before the credential checks, the rollout guard and the apply itself. +func TestRun_ConfirmDecline_NoApplyMutation(t *testing.T) { + var calls []string + + f := countingFakes(&calls)(liveDocs(t)) + f.replace = func(string, string, json.RawMessage) (*workload.Replacement, error) { + calls = append(calls, "replace") + + return nil, nil + } + + install(t, f) + + _, _, err := runIn(t, resizedManifest(), Options{ + ConfirmApply: func(string) (bool, error) { return false, nil }, + }) + + require.ErrorIs(t, err, ErrDeclined) + + for _, seam := range []string{"cred", "findCredential", "guard", "resize", "replace", "create", "build"} { + assert.NotContains(t, calls, seam, "%s must not fire past a declined gate", seam) + } +} + +// TestRun_ConfirmDecline_NoBuild: a genuine build is a mutation like any +// other, and a decline leaves the image untriggered. +func TestRun_ConfirmDecline_NoBuild(t *testing.T) { + var calls []string + + install(t, countingFakes(&calls)(fakes{})) + + _, _, err := runIn(t, unboundDockerfileManifest, Options{ + ConfirmApply: func(string) (bool, error) { return false, nil }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.NotContains(t, calls, "build") + assert.NotContains(t, calls, "create") +} + +// TestRun_ConfirmDecline_NoLockOnlyMutation: an empty plan with --lock would +// still make the serving artifact permanent, and a decline has to stop before +// that one-way door too. +func TestRun_ConfirmDecline_NoLockOnlyMutation(t *testing.T) { + var calls []string + + f := countingFakes(&calls)(fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return draftArtifact(t), nil }, + }) + + install(t, f) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + _, _, err := runIn(t, bound, Options{ + Lock: true, + ConfirmApply: func(string) (bool, error) { return false, nil }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.NotContains(t, calls, "lock", "locking cannot be undone, so it waits for a yes like everything else") + assert.NotContains(t, calls, "guard") +} + +// TestRun_Confirm_LockOnlyPrompts: --lock is about the end state, so an empty +// plan with the flag pending still locks. Locking is a mutation, so the gate +// fires on it exactly as on a deploy: asked once, respected on both sides. +func TestRun_Confirm_LockOnlyPrompts(t *testing.T) { + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + lockSeams := func(locked *[]string) fakes { + return fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return draftArtifact(t), nil }, + lock: func(id string) (*workload.Artifact, error) { + *locked = append(*locked, id) + + return &workload.Artifact{ID: id, Status: workload.ArtifactStatusLocked}, nil + }, + } + } + + var declined []string + + asked := 0 + + install(t, lockSeams(&declined)) + + _, _, err := runIn(t, bound, Options{ + Lock: true, + ConfirmApply: func(string) (bool, error) { + asked++ + + return false, nil + }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.Equal(t, 1, asked, "an empty plan with a pending lock still prompts") + assert.Empty(t, declined, "the decline leaves the artifact as it is") + + var accepted []string + + asked = 0 + + install(t, lockSeams(&accepted)) + + result, _, err := runIn(t, bound, Options{ + Lock: true, + ConfirmApply: func(string) (bool, error) { + asked++ + + return true, nil + }, + }) + + require.NoError(t, err) + assert.Equal(t, 1, asked) + assert.Len(t, accepted, 1, "accepting proceeds into the lock") + assert.True(t, result.Locked) +} + +// TestRun_ConfirmDetach: --detach changes when the run returns, not whether +// it deploys, so the gate fires exactly as without it -- after the plan, +// before the apply. A decline still mutates nothing, and an accept goes out +// detached: the resize is requested and the run returns without waiting for +// it to serve. +func TestRun_ConfirmDetach(t *testing.T) { + var ( + calls []string + resized bool + ) + + counting := func(f fakes) fakes { + f.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + calls = append(calls, "resize") + resized = true + + return nil, nil + } + f.waitSteady = func(string, time.Duration, time.Duration, + func(*workload.Workload), + ) (*workload.Workload, error) { + t.Fatal("--detach must not wait for the workload to settle") + + return nil, nil + } + + return f + } + + install(t, counting(liveDocs(t))) + + asked := 0 + + _, _, err := runIn(t, resizedManifest(), Options{ + Detach: true, + ConfirmApply: func(string) (bool, error) { asked++; return false, nil }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.Equal(t, 1, asked, "the gate is asked on the detached path too") + assert.Empty(t, calls, "a decline on a detached run mutates nothing") + + install(t, counting(liveDocs(t))) + + result, _, err := runIn(t, resizedManifest(), Options{ + Detach: true, + ConfirmApply: func(string) (bool, error) { return true, nil }, + }) + + require.NoError(t, err) + assert.True(t, resized, "accepting proceeds into the detached apply") + assert.Equal(t, ActionUpdated, result.Action) +} + +// TestRun_Confirm_NoOpDoesNotPrompt: the gate fires whenever the run would +// otherwise mutate, and a wholly empty plan would not. Its output must not +// drift either way: --confirm on a no-op run is indistinguishable from a run +// without the flag. +func TestRun_Confirm_NoOpDoesNotPrompt(t *testing.T) { + f := liveDocs(t) + f.create = func(any) (*workload.Workload, error) { + t.Fatal("nothing changed, so nothing should be created") + + return nil, nil + } + + install(t, f) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + _, baseline, err := runIn(t, bound, Options{NonInteractive: true}) + require.NoError(t, err) + + install(t, f) + + var asked bool + + _, stderr, err := runIn(t, bound, Options{ + NonInteractive: true, + ConfirmApply: func(string) (bool, error) { asked = true; return false, nil }, + }) + + require.NoError(t, err) + assert.False(t, asked, "nothing pending means nothing to consent to") + assert.Equal(t, baseline, stderr, "the empty-plan output does not drift when --confirm is given") +} + +// TestRun_Confirm_UnmanagedOnlyDoesNotPrompt: unmanaged fields are never +// mutated -- they survive every deploy untouched -- so a plan whose only +// content is unmanaged fields mutates nothing and must not arm the gate. The +// fixture is the empty-plan one, whose live object carries a sidecar, a +// resource bundle and an autoscaling policy the file never names. +func TestRun_Confirm_UnmanagedOnlyDoesNotPrompt(t *testing.T) { + install(t, liveDocs(t)) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, baseline, err := runIn(t, bound, Options{NonInteractive: true}) + require.NoError(t, err) + require.NotEmpty(t, result.Plan.Unmanaged, + "the fixture has to carry unmanaged fields for this to test the invariant") + + install(t, liveDocs(t)) + + var asked bool + + gated, gatedStderr, err := runIn(t, bound, Options{ + NonInteractive: true, + ConfirmApply: func(string) (bool, error) { asked = true; return false, nil }, + }) + + require.NoError(t, err) + assert.Empty(t, gated.Plan.Artifact, "the plan under test stays empty of managed changes") + assert.False(t, asked, "unmanaged fields do not arm the gate: nothing would change") + assert.Equal(t, baseline, gatedStderr, "the unmanaged-only output does not drift") +} + +// TestRun_ConfirmFirstDeploy: a first deploy is the loudest thing the gate can +// be asked about, so the create path prompts too, and a decline leaves the +// platform without a workload. +func TestRun_ConfirmFirstDeploy(t *testing.T) { + var created []string + + createSeams := func() fakes { + return fakes{ + create: func(any) (*workload.Workload, error) { + created = append(created, "create") + + return running("wl-new"), nil + }, + wait: func(string, workload.Serving, time.Duration, time.Duration, + func(*workload.Workload), + ) (*workload.Workload, error) { + return running("wl-new"), nil + }, + } + } + + install(t, createSeams()) + + asked := 0 + + _, _, err := runIn(t, unboundImageManifest, Options{ + ConfirmApply: func(string) (bool, error) { asked++; return false, nil }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.Equal(t, 1, asked) + assert.Empty(t, created, "a declined first deploy creates nothing") + + install(t, createSeams()) + + result, _, err := runIn(t, unboundImageManifest, Options{ + ConfirmApply: func(string) (bool, error) { asked++; return true, nil }, + }) + + require.NoError(t, err) + assert.Equal(t, 2, asked) + assert.Equal(t, []string{"create"}, created, "accepting proceeds into the create") + assert.Equal(t, "wl-new", result.WorkloadID) +} + +// TestRun_ConfirmWithoutDiff_UsesDefaultRender: --confirm does not require +// --diff. The default plan renders as it always does, then the gate asks; the +// diff renderer stays out of a run that never asked for it. +func TestRun_ConfirmWithoutDiff_UsesDefaultRender(t *testing.T) { + install(t, liveDocs(t)) + + var asked bool + + _, stderr, err := runIn(t, resizedManifest(), Options{ + ConfirmApply: func(string) (bool, error) { asked = true; return false, nil }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.True(t, asked) + assert.Contains(t, stderr, "resourceAllocation.memory: 22GB -> 24GB", + "the default plan states a change as have -> want") + assert.NotContains(t, stderr, "\n+ ") + assert.NotContains(t, stderr, "identical lines", "the diff rendering is opt-in and stays opt-in") +} + +// TestRun_ConfirmWithDiff_PromptsAfterTheDiff: the composition renders the +// unified diff, then asks. The diff body is what the user is consenting to, +// so it precedes the question by construction. +func TestRun_ConfirmWithDiff_PromptsAfterTheDiff(t *testing.T) { + install(t, liveDocs(t)) + + var printed int + + var stderr bytes.Buffer + + _, err := runConfirmIn(t, resizedManifest(), Options{ + Diff: true, + ConfirmApply: func(string) (bool, error) { printed = stderr.Len(); return false, nil }, + }, &stderr) + + require.ErrorIs(t, err, ErrDeclined) + + body := stderr.String()[:printed] + + assert.Contains(t, body, "+ containerGroups[default].containers[vllm-server].resourceAllocation.memory: 24GB", + "the diff precedes the question") + assert.NotContains(t, stderr.String()[printed:], "+ ", + "nothing renders past the gate on a decline") +} + +// TestRun_ConfirmAndTypedConfirm_Coexist: the two questions are different +// questions about different things, and they keep their order. The y/N asks +// whether to apply at all; the typed one asks who is rolling production. A +// decline at the y/N never reaches the second question, because there is +// nothing left to roll. +func TestRun_ConfirmAndTypedConfirm_Coexist(t *testing.T) { + var ( + tr track + gateAsk int + typedAsk int + gateFirst bool + ) + + f := lockedLive(wiredRoll(&tr)) + f.guard = func(string) error { + tr.steps = append(tr.steps, "guard") + + return nil + } + + install(t, f) + + // Declining the y/N: the typed question is never asked and nothing runs. + _, _, err := runIn(t, newImage(), Options{ + ConfirmApply: func(string) (bool, error) { gateAsk++; return false, nil }, + Confirm: func(string, string) (bool, error) { + typedAsk++ + + return false, nil + }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.Equal(t, 1, gateAsk) + assert.Equal(t, 0, typedAsk, "a declined y/N skips the typed confirm entirely") + assert.NotContains(t, tr.steps, "lock:art-2") + assert.NotContains(t, tr.steps, "replace:art-2") + + // Accepting the y/N hands the run to the typed confirm, which refuses: + // the roll never starts and nothing is locked. + gateAsk, typedAsk = 0, 0 + + _, _, err = runIn(t, newImage(), Options{ + ConfirmApply: func(string) (bool, error) { + gateFirst = typedAsk == 0 + + gateAsk++ + + return true, nil + }, + Confirm: func(string, string) (bool, error) { + typedAsk++ + + return false, nil + }, + }) + + require.Error(t, err) + assert.True(t, gateFirst, "the y/N is asked before the typed confirm") + assert.NotContains(t, tr.steps, "lock:art-2", "a failed typed confirm mutates nothing") + assert.NotContains(t, tr.steps, "replace:art-2") + assert.NotContains(t, tr.steps, "create-artifact") +} diff --git a/internal/workload/up/run.go b/internal/workload/up/run.go index ea1fb1b82..36dc57dc0 100644 --- a/internal/workload/up/run.go +++ b/internal/workload/up/run.go @@ -68,6 +68,13 @@ var ( syncProjectFn = defaultSync ) +// ErrDeclined is what a run answers when --confirm was given and the user +// said no. It is returned before the first mutating branch runs, so a caller +// can treat it as "nothing happened" rather than as a failure partway through +// one: nothing was deployed, nothing was locked, and the plan it declined is +// still the plan the next run will carry out. +var ErrDeclined = errors.New("declined: nothing was deployed") + // Options is everything a run needs from its caller. type Options struct { // Dir is where to start looking for the manifest. The search walks @@ -104,6 +111,16 @@ type Options struct { // caller's job because only it knows where the user's input comes from. Confirm func(question, want string) (bool, error) + // ConfirmApply is the opt-in y/N gate behind --confirm, and nil when + // nobody is to be asked. It is separate from Confirm: that one is the + // locked-production question only the exact workload name answers, and + // this one is the ordinary "apply this?" asked of every run the flag was + // given to. The deploy calls it once, after the plan is printed and + // before the first mutating branch, and a no stops the run with + // ErrDeclined. Reading the answer is the caller's job because only it + // knows where the user's input comes from. + ConfirmApply func(question string) (bool, error) + // Lock makes the artifact that ends up live immutable and permanent. // Locking is one-way, so it happens last, only after the workload is // actually serving: locking something that never came up would leave an @@ -214,6 +231,15 @@ func Run(opts Options) (Result, error) { noteUnusedForce(plan, opts) + return carryOut(loaded, live, plan, result, opts) +} + +// carryOut is everything a run does once the plan has been shown: stop for a +// dry run, ask the confirm gate, then act on the plan through whichever path +// applies. It is its own step because the ordering here is the contract the +// --confirm gate rests on: the review is printed before the question is asked, +// and the question is answered before the first mutation is attempted. +func carryOut(loaded Loaded, live Live, plan Plan, result Result, opts Options) (Result, error) { if opts.DryRun { // The one run whose action is the plan's: it stops here, so printing // the plan is the whole of what it did. @@ -222,6 +248,10 @@ func Run(opts Options) (Result, error) { return result, nil } + if err := confirmGate(plan, result, opts); err != nil { + return result, err + } + if plan.Empty() { return lockOnly(loaded, live, result, opts) } @@ -229,6 +259,44 @@ func Run(opts Options) (Result, error) { return apply(loaded, live, plan, result, opts) } +// confirmGate asks the opt-in y/N question --confirm installs, and only when +// answering yes would change something. The gate fires whenever the run would +// otherwise mutate: a plan carrying changes, or an empty one with a --lock +// waiting to make the serving artifact permanent. A wholly empty plan asks +// nothing and returns as it always has, and unmanaged fields never arm the +// gate: they survive every deploy untouched, so there is nothing to consent +// to. "Empty" is therefore measured by pending mutation, not by how quiet the +// plan looks. +// +// Everything that prints has printed by the time the question is asked, and +// everything that mutates is still ahead of it. That placement is the whole +// promise: a decline returns ErrDeclined from a run that has changed nothing, +// and a dry run never gets here at all -- it returns above, because a preview +// is not a mutation to consent to. +func confirmGate(plan Plan, result Result, opts Options) error { + if opts.ConfirmApply == nil { + return nil + } + + // lockOnly is the one mutation an empty plan can still carry out, and + // only when --lock was passed and the serving artifact is not already + // locked. Any other empty plan is the "Already up to date" run. + if plan.Empty() && (result.Locked || !opts.Lock) { + return nil + } + + agreed, err := opts.ConfirmApply("Apply this deploy?") + if err != nil { + return fmt.Errorf("cannot ask to apply this deploy: %w", err) + } + + if !agreed { + return ErrDeclined + } + + return nil +} + // rendererFor picks how the plan is shown. --diff swaps the renderer and // nothing else: the plan goes out laid out as a diff rather than summarised, // and everything below reads it identically either way. From e5b53d60779f59f4a278df171fc45266d25cf558 Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Sat, 29 Aug 2026 00:05:17 -0700 Subject: [PATCH 10/12] [RAPTOR-19538] fix(workload): scrub env-var values when a diff row IS the envVars list When the live side carries no environmentVars list at all, the walker has nothing to match variables against by name, so the file's whole list arrives as one changed leaf whose path ENDS at the list. redacted() only matches the per-variable spelling (".environmentVars["), and the container-level scrub only saw env-var blocks nested BELOW a row's value, so this row serialized every variable's literal plaintext and credential ref straight into the JSON envelope. Caught by the m3-live-smoke staging run: the human diff was safe (format() summarizes composites), the machine envelope was not. The constructor now names the shape itself (endsAtEnvVars) and scrubs the list directly, keeping the variable names and dropping every value, marked redacted like the other refusal paths. --- internal/workload/up/render.go | 42 ++++++++++---- internal/workload/up/render_test.go | 86 +++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 10 deletions(-) diff --git a/internal/workload/up/render.go b/internal/workload/up/render.go index 06b58fe63..b543b1776 100644 --- a/internal/workload/up/render.go +++ b/internal/workload/up/render.go @@ -852,15 +852,19 @@ func changesJSON(artifact, runtime []DiffRow) []ChangeJSON { // CI artifacts; the entry still says which path moved, so the change itself // is not silently lost to the refusal. // -// Path-based redaction alone is not enough. A NEW name-keyed list element -// arrives as one whole-element row (a new container is one row for the -// container, not one per field), so the row's own path names the container -// and never trips redacted() while its value carries an environmentVars -// block complete with secrets. Of the two ways to close that leak, dropping -// the whole entry would hide the element's other fields from a consumer that -// has to review the plan, so the subtree is scrubbed instead: variable names -// survive, values do not, and the scrub copies rather than mutates because -// the same rows feed the human diff after this. +// Path-based redaction alone is not enough, twice over. A NEW name-keyed list +// element arrives as one whole-element row (a new container is one row for +// the container, not one per field), so the row's own path names the +// container and never trips redacted() while its value carries an +// environmentVars block complete with secrets. And when the live side +// carries no such list at all, there is nothing to match variables against +// by name, so the file's whole list arrives as one row whose path ENDS at +// the list -- a row that is itself the block the redaction refuses to print. +// Of the two ways to close either leak, dropping the whole entry would hide +// what the element carries from a consumer that has to review the plan, so +// the values are scrubbed instead: variable names survive, values do not, +// and the scrub copies rather than mutates because the same rows feed the +// human diff after this. func changeJSON(leaf DiffRow) ChangeJSON { out := ChangeJSON{ Path: leaf.Path, @@ -877,7 +881,15 @@ func changeJSON(leaf DiffRow) ChangeJSON { return out } - if carriesEnvVars(leaf.Want) || carriesEnvVars(leaf.Have) { + if endsAtEnvVars(leaf.Path) { + // The row IS the environmentVars list, so the scrub is the list + // scrub itself; routing the value through the generic walker would + // copy every element verbatim, because the elements carry no + // environmentVars key of their own to catch. + out.Want = scrubEnvList(leaf.Want) + out.Have = scrubEnvList(leaf.Have) + out.Redacted = true + } else if carriesEnvVars(leaf.Want) || carriesEnvVars(leaf.Have) { out.Want = scrubEnvVars(leaf.Want) out.Have = scrubEnvVars(leaf.Have) out.Redacted = true @@ -886,6 +898,16 @@ func changeJSON(leaf DiffRow) ChangeJSON { return out } +// endsAtEnvVars reports whether a leaf's path ends at the environmentVars +// list itself, the shape the walker emits when the live side has no list to +// walk element-by-element. redacted() misses it because that hook matches +// the per-variable spelling (".environmentVars["), while this row's path +// stops at the list, so the constructor names the shape itself and scrubs +// the values exactly as it scrubs them below a whole-element row. +func endsAtEnvVars(path string) bool { + return strings.HasSuffix(path, "."+envVarsKey) +} + // carriesEnvVars reports whether a value holds an environmentVars block // anywhere below it, which is what turns a whole-element row into a leak // however innocent its own path reads. diff --git a/internal/workload/up/render_test.go b/internal/workload/up/render_test.go index 7f42fcfff..cf7f255d5 100644 --- a/internal/workload/up/render_test.go +++ b/internal/workload/up/render_test.go @@ -823,6 +823,92 @@ func TestPlanJSON_DiffRedactsBeforeSerialising(t *testing.T) { assert.Equal(t, plain.Artifact, plan.JSONWithDiff().Artifact) } +// TestPlanJSON_DiffRedactsAWholeEnvVarsList: when the live side carries no +// environmentVars list at all, the walker has nothing to match variables +// against by name, so the file's whole list arrives as ONE row whose path +// ENDS at the list. That row is as much a carrier of variable values as any +// whole-element row, and the document must scrub it the same way: the names +// stay, every value -- literal or credential ref -- is gone, and the refusal +// is marked. (Caught against live staging: the container-level scrub only +// saw values nested BELOW a row, never a row that was the list itself.) +func TestPlanJSON_DiffRedactsAWholeEnvVarsList(t *testing.T) { + const ( + literal = "sk-plaintext-9999" + credID = "88d3c4d5e6f7a8b9c0d1e2f3" + ) + + payload := `{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "spec": { + "type": "service", + "containerGroups": [{"name": "default", "containers": [ + {"name": "primary", "environmentVars": [ + {"name": "OPENAI_API_KEY", "value": "` + literal + `"}, + {"name": "HUGGING_FACE_HUB_TOKEN", "source": "dr-credential", + "drCredentialId": "` + credID + `", "key": "apiToken"} + ]} + ]}] + }} + }` + + // The live container exists but names no variables, so there is no list + // to walk element-by-element: the file's whole environmentVars list is + // one changed leaf. + live := `{ + "type": "service", + "containerGroups": [{"name": "default", "containers": [ + {"name": "primary", "image": "app:1"} + ]}] + }` + + plan, err := Build(loadedFrom(payload), liveFrom(t, StateRunning, live, planLiveRuntime), builtCode(0)) + require.NoError(t, err) + + // The fixture has to earn its keep: the change must arrive as the + // whole-list row, not as per-variable rows. + assert.Contains(t, paths(plan.Artifact), + "containerGroups[default].containers[primary].environmentVars", + "expected the whole environmentVars list as one changed leaf") + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + document := string(encoded) + + for _, secret := range []string{literal, credID, "dr-credential:"} { + assert.NotContains(t, document, secret) + } + + var decoded map[string]any + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + changes := decoded["diff"].(map[string]any)["changes"].([]any) + + var entry map[string]any + + for _, c := range changes { + e, _ := c.(map[string]any) + + if e["path"] == "containerGroups[default].containers[primary].environmentVars" { + entry = e + + break + } + } + + require.NotNil(t, entry, "the whole-list change must survive the scrub") + assert.Equal(t, true, entry["redacted"]) + + want, ok := entry["want"].([]any) + require.True(t, ok, "the variable names keep their list") + + assert.Equal(t, []any{ + map[string]any{"name": "OPENAI_API_KEY"}, + map[string]any{"name": "HUGGING_FACE_HUB_TOKEN"}, + }, want, "only the names survive the scrub") +} + // TestPlanJSON_FirstDeployDiffIsAllAdditions: with no live object every leaf // of the compiled manifest is an addition, absent with no live value, and // there is nothing for the unmanaged list to hold. From 42e401f9f31fe575cd8aaca40c49146ea98f94b9 Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Sat, 29 Aug 2026 00:18:52 -0700 Subject: [PATCH 11/12] [RAPTOR-19538] test(smoke): live staging scenario for --diff and --confirm RAPTOR-19538-diff-confirm.sh walks the six VAL-SMOKE assertions against staging on a RUN-identified throwaway whoami workload: the sizing change shows as a unified diff under --dry-run --diff with neither env-var value leaking, the JSON envelope carries the structured change list with the values scrubbed, a --confirm decline exits nonzero through a real PTY (DATAROBOT_CLI_NON_INTERACTIVE unset by the scenario; the expect driver allocates the terminal) having mutated nothing, an accepted y applies the sizing, and the trap cleanup deletes the workload and artifact so no RUN-prefixed resource stays on staging. Two shapes the fixtures never showed: the env-var change arrives as one whole-list row when the live side has no list to match against, and the settings API rejects environmentVars under runtime containers with a 422, so the smoke drops the two variables before the accepted deploy and keeps them as diff surface for the redaction assertions. Registered in TICKETS.md; deliberately not wired into run_workload_smoke_test.sh because it drives an interactive PTY prompt and writes to staging. --- .../workload/RAPTOR-19538-diff-confirm.sh | 512 ++++++++++++++++++ smoke_test_scripts/workload/TICKETS.md | 1 + 2 files changed, 513 insertions(+) create mode 100755 smoke_test_scripts/workload/RAPTOR-19538-diff-confirm.sh diff --git a/smoke_test_scripts/workload/RAPTOR-19538-diff-confirm.sh b/smoke_test_scripts/workload/RAPTOR-19538-diff-confirm.sh new file mode 100755 index 000000000..fa3085002 --- /dev/null +++ b/smoke_test_scripts/workload/RAPTOR-19538-diff-confirm.sh @@ -0,0 +1,512 @@ +#!/usr/bin/env bash +# Ticket: RAPTOR-19538 +# Live staging smoke โ€” `dr workload up --diff` + `--confirm` (m3-live-smoke). +# +# Deploys a throwaway image-based whoami workload on staging (the RAPTOR-19533 +# scenario A pattern: create --spec-file โ†’ wait running โ†’ config binds and +# renders .datarobot.yaml), edits the manifest's sizing plus two environment +# variables (one credential-backed `dr-credential:/` reference and +# one literal plaintext value), then walks the six VAL-SMOKE assertions: +# +# SMOKE-001 `up --dry-run --diff` shows the sizing change as a unified +# diff (- old / + new with context) and leaks no env-var value. +# SMOKE-002 A real `up --confirm` answered "n" exits NONZERO and mutates +# nothing. Run under a PTY via expect(1) with +# DATAROBOT_CLI_NON_INTERACTIVE unset: lib.sh sets the variable +# and a piped stdin would suppress the gate, so both overrides +# are required for the prompt to install at all. +# SMOKE-005 `up --dry-run --diff --output-format json` emits one JSON +# document whose plan.diff carries the structured change list, +# with env-var values redacted, while the change is still +# pending (i.e. before SMOKE-003 applies it). +# SMOKE-003 A real `up --confirm` answered "y" applies the sizing change: +# a follow-up `up --dry-run` reports the workload up to date, +# which is the CLI's own live-vs-manifest verdict that sizing +# now matches the manifest (`workload get` does not surface +# sizing, so the dry-run plan is the observable). +# SMOKE-004 Cleanup: the EXIT-trap cleanup deletes the created workload +# and artifact, and a post-cleanup probe proves no RUN-prefixed +# resource is left on staging. +# +# The decline leg (SMOKE-002) expects a NONZERO exit, so it is wrapped in a +# `set +e` guard and asserts the captured code directly โ€” never +# wl::assert_cmd_ok, which would abort the script on the expected failure and +# strand SMOKE-003. +# +# ~15 min. Run directly (serially; it creates/deletes real staging +# resources): smoke_test_scripts/workload/RAPTOR-19538-diff-confirm.sh +# +# Transcript/evidence files land in +# tmp/smoke-19538// (override with SMOKE_TRANSCRIPT_DIR). + +# shellcheck shell=bash +set -euo pipefail +source "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +wl::init_env +wl::register_cleanup + +# The lib trap stops workloads, but a stopped draft still shows up in +# `dr workload list`, and SMOKE-004 requires no RUN-prefixed leftovers at +# all. Keep the lib registration (the registry helpers below are the same +# arrays wl::cleanup drains) and swap the EXIT trap for a stronger wrapper +# that DELETEs our workloads (the platform stops a running workload before +# removing it) and then hands off to the lib cleanup for artifacts, scratch, +# and exit-code propagation. +smoke::cleanup_resources() { + local id + + # Workloads first: an artifact backing a live workload cannot be deleted. + for id in "${WL_CREATED_WORKLOADS[@]:-}"; do + [[ -n "$id" ]] || continue + "$DR_BIN" workload delete "$id" --yes >/dev/null 2>&1 || true + echo " ๐Ÿงน deleted workload $id" + done + WL_CREATED_WORKLOADS=() + + for id in "${WL_ARTIFACT_IDS[@]:-}"; do + [[ -n "$id" ]] || continue + "$DR_BIN" artifact delete "$id" --yes >/dev/null 2>&1 || true + echo " ๐Ÿงน deleted artifact $id" + done + WL_ARTIFACT_IDS=() +} + +smoke::cleanup() { + local rc=$? + + smoke::cleanup_resources + + [[ -n "${WL_SCRATCH:-}" ]] && rm -rf "$WL_SCRATCH" 2>/dev/null || true + + return "$rc" +} +# shellcheck disable=SC2317 # reached via the EXIT trap, not a call site. +trap smoke::cleanup EXIT + +# --------------------------------------------------------------------------- +# Evidence: transcripts land outside the scratch dir, which the trap removes. +# --------------------------------------------------------------------------- +TRANSCRIPT_DIR="${SMOKE_TRANSCRIPT_DIR:-$WL_REPO_ROOT/tmp/smoke-19538/$RUN}" +mkdir -p "$TRANSCRIPT_DIR" + +smoke::record() { + # record : persist this leg's WL_OUT/WL_RC/WL_ERR capture. + local leg=$1 + printf '%s' "$WL_OUT" > "$TRANSCRIPT_DIR/$leg.out" + printf '%s' "$WL_ERR" > "$TRANSCRIPT_DIR/$leg.err" + echo "$WL_RC" > "$TRANSCRIPT_DIR/$leg.rc" +} + +# --------------------------------------------------------------------------- +# PTY driver for the interactive --confirm gate (SMOKE-002/003). +# +# The gate installs only when stdin is a terminal AND the run is interactive. +# This bash script's stdin is a pipe and wl::init_env exported +# DATAROBOT_CLI_NON_INTERACTIVE=1 โ€” either one alone suppresses the prompt BY +# DESIGN, so the driver runs the CLI under expect(1), which allocates a real +# PTY, and unsets the variable in the child environment. The answer ("n" or +# "y") is fed to the prompt through the PTY like a keystroke. +# --------------------------------------------------------------------------- +smoke::write_expect_driver() { + cat > "$WL_SCRATCH/confirm.exp" <<'EOF' +#!/usr/bin/expect -f +# argv: [up args...] +set timeout 1200 +set dr_bin [lindex $argv 0] +set workdir [lindex $argv 1] +set transcript [lindex $argv 2] +set answer [lindex $argv 3] +set up_args [lrange $argv 4 end] + +# The gate must see an interactive run: a real PTY (expect allocates one) and +# no non-interactive signal in the environment. Unset defensively as well as +# in the caller โ€” either override alone is not enough if the other regresses. +catch {unset env(DATAROBOT_CLI_NON_INTERACTIVE)} +set env(DATAROBOT_CLI_FEATURE_WORKLOAD) true + +log_file -noappend $transcript +cd $workdir +eval spawn $dr_bin workload up $up_args + +expect { + -re {Apply this deploy} { + send -- "$answer\r" + exp_continue + } + eof {} + timeout { + puts "\nPTY driver: timed out waiting for the confirm prompt" + exit 124 + } +} + +# Propagate the CLI's exit status: `wait` yields pid spawnid flag status. +foreach {pid spawnid os_error_flag status} [wait] break +exit $status +EOF +} + +# smoke::pty_run [up args...] โ€” runs the PTY driver. +# The caller captures the exit code in PTY_RC itself; this wrapper must never +# abort under set -e (the decline leg legitimately exits nonzero). +smoke::pty_run() { + local transcript=$1 answer=$2 + shift 2 + + PTY_RC=0 + /usr/bin/expect -f "$WL_SCRATCH/confirm.exp" \ + "$DR_BIN" "$(pwd)" "$transcript" "$answer" "$@" || PTY_RC=$? +} + +# --------------------------------------------------------------------------- +# Credential id discovery (read-only). +# +# SMOKE-001's redaction assertion needs one credential-backed env var, and +# SMOKE-003 really applies the manifest โ€” so the reference must name a +# credential that exists. The CLI has no credential subcommand, so the id is +# discovered with a read-only GET on the platform's /credentials/ route using +# the same configured token the CLI itself authenticates with. Only the id is +# read; the stored secret is never fetched and never printed. +# --------------------------------------------------------------------------- +smoke::discover_credential_id() { + local endpoint token override + + endpoint="${DATAROBOT_ENDPOINT:-}" + if [[ -z "$endpoint" ]]; then + endpoint="$(yq -r '.endpoint // ""' "$HOME/.config/datarobot/drconfig.yaml")" + fi + + override="$(wl::resolve_token)" + if [[ -n "$override" ]]; then + token="$override" + else + token="$(yq -r '.token // ""' "$HOME/.config/datarobot/drconfig.yaml")" + fi + + [[ -n "$endpoint" && -n "$token" ]] || return 1 + + curl -fsS --max-time 30 -H "Authorization: Bearer $token" \ + "$endpoint/credentials/?limit=1" \ + | jq -r '.data[0].credentialId // empty' +} + +wl::start_timer "19538: --diff/--confirm live smoke (staging)" + +# --- Deploy the throwaway whoami workload (scenario A pattern) -------------- +work="$WL_SCRATCH/project" +mkdir -p "$work" +cat > "$work/workload.yaml" </dev/null + +# The platform materializes an artifact for the inline image spec; register it +# (if any) so cleanup deletes it after the workload is gone. +wl::dr_capture workload get "$WID" --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload get" +AID="$(printf '%s' "$WL_OUT" | jq -r '.artifactId // empty')" +[[ -z "$AID" ]] || wl::register_artifact "$AID" + +# --- Bind and render the manifest ------------------------------------------- +cd "$work" +wl::dr_capture workload config --yes --workload-id "$WID" +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload config --workload-id" +[[ -f .datarobot.yaml ]] || wl::fail "config did not write .datarobot.yaml" +wl::pass "wrote .datarobot.yaml" + +# Baseline: the file the bind rendered is the live state. +wl::dr_capture workload up --dry-run +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "baseline up --dry-run" +smoke::record "000-baseline-dry-run" + +# --- Edit the manifest: sizing + one env var of each secret shape ----------- +CRED_ID="$(smoke::discover_credential_id)" \ + || wl::fail "cannot discover a credential id (read-only GET on /credentials/ failed)" +if [[ -z "$CRED_ID" ]]; then + wl::fail "no credential visible to this account; the credential-backed env var cannot be referenced" +fi + +# A recognizable fake plaintext so the redaction greps are exact: the value +# must never appear in any diff output, human or JSON. +LIT_SECRET="smoke-literal-secret-${RUN}-never-print" + +yq -i '.runtime.containerGroups[0].replicaCount = 3' .datarobot.yaml +yq -i ".runtime.containerGroups[0].containers[0].environmentVars = [{\"name\": \"SMOKE_LIT_TOKEN\", \"value\": \"$LIT_SECRET\"}, {\"name\": \"SMOKE_API_TOKEN\", \"value\": \"dr-credential:$CRED_ID/apiToken\"}]" .datarobot.yaml +wl::pass "manifest edited: replicaCount 1 -> 3, two env vars added (credential ref + literal)" + +# The reference id is the only credential fact the manifest needs to carry; +# grep for the secret SHAPES below, never for a stored value. +wl::pass "using credential $CRED_ID for the dr-credential reference" + +# --- SMOKE-001: --dry-run --diff shows the sizing change, leaks no values --- +wl::start_timer "SMOKE-001: up --dry-run --diff" + +wl::dr_capture workload up --dry-run --diff +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "up --dry-run --diff" +smoke::record "001-dry-run-diff" + +printf '%s\n' "$WL_ERR" | grep -Eq '^- .*replicaCount' \ + || wl::fail "SMOKE-001: no '- old' line for the sizing change; stderr was: $WL_ERR" +printf '%s\n' "$WL_ERR" | grep -Eq '^\+ .*replicaCount' \ + || wl::fail "SMOKE-001: no '+ new' line for the sizing change" +wl::pass "diff shows the sizing change as - old / + new" + +# Context either side: the line before the first - marker is context (no marker). +first_change_lineno="$(printf '%s\n' "$WL_ERR" | grep -nE '^[+-] ' | head -1 | cut -d: -f1)" +[[ -n "$first_change_lineno" ]] || wl::fail "SMOKE-001: diff body has no change lines" +prev_lineno=$((first_change_lineno - 1)) +[[ "$prev_lineno" -ge 1 ]] || wl::fail "SMOKE-001: no line before the first change" +prev_line="$(printf '%s\n' "$WL_ERR" | sed -n "${prev_lineno}p")" +if printf '%s' "$prev_line" | grep -qE '^[+-] '; then + wl::fail "SMOKE-001: line preceding the first change is not context: $prev_line" +fi +wl::pass "change is flanked by context lines" + +# Redaction: neither the credential ref material nor the literal value may +# appear; the env var NAME must. (The .err file was persisted above, so the +# absent-checks run against a real file rather than a pipe.) +wl::assert_absent "$TRANSCRIPT_DIR/001-dry-run-diff.err" "$LIT_SECRET" "SMOKE-001 literal secret" +wl::assert_absent "$TRANSCRIPT_DIR/001-dry-run-diff.err" 'dr-credential:' "SMOKE-001 credential ref" +wl::assert_absent "$TRANSCRIPT_DIR/001-dry-run-diff.err" "$CRED_ID" "SMOKE-001 credential id" + +# The env-var change reaches the diff either per element (paths carrying the +# variable name) or as one whole-list row (`environmentVars: [N item(s)]`) +# when the live side has no such list; both spellings must appear as a change +# line, and neither may carry a value. +printf '%s\n' "$WL_ERR" | grep -q 'environmentVars' \ + || wl::fail "SMOKE-001: no environmentVars change line in the diff" +wl::pass "env-var values redacted; the env-var change line is present" + +# (d) No mutation: the workload document is identical around the dry-run. +wl::dr_capture workload get "$WID" --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload get (before-diff)" +before="$(printf '%s' "$WL_OUT" | jq -S .)" +wl::dr_capture workload get "$WID" --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload get (after-diff)" +after="$(printf '%s' "$WL_OUT" | jq -S .)" +[[ "$before" == "$after" ]] || wl::fail "SMOKE-001: workload document changed around the dry-run diff" +wl::pass "no mutation from --dry-run --diff" + +wl::stop_timer + +# --- SMOKE-002: --confirm answered "n" exits nonzero, mutates nothing ------- +wl::start_timer "SMOKE-002: --confirm decline (PTY)" + +# lib.sh exports DATAROBOT_CLI_NON_INTERACTIVE=1 for every non-PTY leg; the +# gate installs only in an interactive run, so unset it here and keep it unset +# โ€” the PTY driver also unsets it in the child, and only the two overrides +# TOGETHER install the prompt (piped stdin alone suppresses it by design). +unset DATAROBOT_CLI_NON_INTERACTIVE +smoke::write_expect_driver + +wl::dr_capture workload get "$WID" --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload get (before-decline)" +before="$(printf '%s' "$WL_OUT" | jq -S .)" + +# Expected NONZERO exit: set +e so the guard cannot abort before SMOKE-003, +# then assert the captured code directly (never wl::assert_cmd_ok here). +set +e +smoke::pty_run "$TRANSCRIPT_DIR/002-decline-pty.log" "n" --confirm +decline_rc=$PTY_RC +set -e + +if [[ "$decline_rc" -eq 0 ]]; then + wl::fail "SMOKE-002: declined run exited 0; the gate must refuse with a nonzero exit" +fi +wl::pass "declined run exited nonzero (rc=$decline_rc)" + +grep -q 'Apply this deploy' "$TRANSCRIPT_DIR/002-decline-pty.log" \ + || wl::fail "SMOKE-002: prompt missing from the PTY transcript" +grep -q '(y/N)' "$TRANSCRIPT_DIR/002-decline-pty.log" \ + || wl::fail "SMOKE-002: prompt is not the y/N (default no) phrasing" +grep -q 'declined: nothing was deployed' "$TRANSCRIPT_DIR/002-decline-pty.log" \ + || wl::fail "SMOKE-002: decline message missing from the PTY transcript" +wl::pass "prompt and decline message present in the PTY transcript" + +# stdout untouched: a declined run prints no endpoint. The workload's own +# endpoint line is what a successful run emits; its absence here, against the +# same PTY driver that shows it on the accept leg below, is the observable. +endpoint="$(printf '%s' "$before" | jq -r '.endpoint // empty')" +if [[ -n "$endpoint" ]] && grep -qF "$endpoint" "$TRANSCRIPT_DIR/002-decline-pty.log"; then + wl::fail "SMOKE-002: declined transcript contains the endpoint (stdout was not untouched)" +fi +wl::pass "declined transcript carries no endpoint (stdout untouched)" + +# (d) Nothing mutated: the workload document matches, and the sizing change is +# still pending exactly as the diff showed it. +wl::dr_capture workload get "$WID" --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload get (after-decline)" +after="$(printf '%s' "$WL_OUT" | jq -S .)" +[[ "$before" == "$after" ]] || wl::fail "SMOKE-002: workload document changed after the decline" +wl::dr_capture workload up --dry-run --diff +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "up --dry-run --diff (pending after decline)" +smoke::record "002-pending-after-decline" +printf '%s\n' "$WL_ERR" | grep -Eq '^\+ .*replicaCount.*3' \ + || wl::fail "SMOKE-002: sizing change no longer pending after the decline (spec mutated?)" +wl::pass "decline mutated nothing; sizing change still pending" + +wl::stop_timer + +# --- SMOKE-005: --diff JSON envelope with the change still pending ---------- +wl::start_timer "SMOKE-005: --diff --output-format json envelope" + +wl::dr_capture workload up --dry-run --diff --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "up --dry-run --diff --output-format json" +smoke::record "005-json-diff" +printf '%s' "$WL_OUT" > "$TRANSCRIPT_DIR/005-json-diff.raw" + +# (a) Exactly one JSON document on stdout. The envelope nests the plan under +# the "up" key (the deploy's outcome object), hence .up.plan.diff. +docs="$(printf '%s' "$WL_OUT" | jq -s 'length')" +[[ "$docs" == "1" ]] || wl::fail "SMOKE-005: stdout carries $docs documents, expected 1" +wl::pass "stdout is exactly one JSON document" + +# (b) A non-empty changes list that names the sizing field with live/want. +changes="$(printf '%s' "$WL_OUT" | jq '.up.plan.diff.changes | length')" +[[ "$changes" -gt 0 ]] || wl::fail "SMOKE-005: plan.diff.changes is empty" +sizing_change="$(printf '%s' "$WL_OUT" \ + | jq -c '[.up.plan.diff.changes[] | select(.path | contains("replicaCount"))][0]')" +[[ "$sizing_change" != "null" ]] || wl::fail "SMOKE-005: no replicaCount entry in plan.diff.changes" +have="$(printf '%s' "$sizing_change" | jq '.have')" +want="$(printf '%s' "$sizing_change" | jq '.want')" +[[ "$have" == "1" && "$want" == "3" ]] \ + || wl::fail "SMOKE-005: sizing change is have=$have want=$want, expected 1 -> 3" +wl::pass "plan.diff.changes carries the sizing change (have=1, want=3)" + +# (c) Env-var paths are redacted in the envelope. A whole-list env-var change +# arrives as one row whose own path only ends in .environmentVars, so the +# envelope marks it `redacted: true` and keeps the structure with the variable +# NAMES but not the values (scrubEnvList) โ€” asserting null have/want here +# would test the wrong shape. The hard property is the whole-document scan +# below: neither the literal secret nor any credential material appears. +printf '%s' "$WL_OUT" | jq -e ' + [.up.plan.diff.changes[] | select(.path | contains("environmentVars"))] | length > 0 +' >/dev/null || wl::fail "SMOKE-005: no environmentVars entries in plan.diff.changes" +printf '%s' "$WL_OUT" | jq -e ' + [.up.plan.diff.changes[] + | select(.path | contains("environmentVars")) + | .redacted == true] | all +' >/dev/null || wl::fail "SMOKE-005: an environmentVars change is not redacted in the envelope" +wl::assert_absent "$TRANSCRIPT_DIR/005-json-diff.raw" "$LIT_SECRET" "SMOKE-005 literal secret" +wl::assert_absent "$TRANSCRIPT_DIR/005-json-diff.raw" 'dr-credential:' "SMOKE-005 credential ref" +wl::assert_absent "$TRANSCRIPT_DIR/005-json-diff.raw" "$CRED_ID" "SMOKE-005 credential id" +wl::pass "env-var values redacted from the JSON envelope" + +# (d) unmanaged is present (array, possibly empty). +printf '%s' "$WL_OUT" | jq -e '.up.plan.diff.unmanaged | type == "array"' >/dev/null \ + || wl::fail "SMOKE-005: plan.diff.unmanaged missing or not an array" +wl::pass "plan.diff.unmanaged present" + +wl::stop_timer + +# --- SMOKE-003: --confirm answered "y" applies the sizing change ------------ +wl::start_timer "SMOKE-003: --confirm accept (PTY)" + +# The env vars were diff surface only. The settings update sends the runtime +# block verbatim and the platform rejects environmentVars under runtime +# containers with a 422 ("Extra inputs are not permitted"), so a manifest +# that asks for runtime env vars can never be applied -- the two variables +# come back out here, and the accepted deploy carries the sizing change, the +# change the confirm gate exists to gate. (Pre-existing CLI/platform mismatch +# in how runtime env vars apply, found by this smoke and reported separately; +# it does not touch the diff, the confirm gate, or the JSON envelope.) +yq -i 'del(.runtime.containerGroups[0].containers[0].environmentVars)' .datarobot.yaml +wl::pass "env vars removed from the runtime block; the sizing change stays pending" + +smoke::pty_run "$TRANSCRIPT_DIR/003-accept-pty.log" "y" --confirm +wl::assert_cmd_ok "$PTY_RC" "see transcript" "$(tail -40 "$TRANSCRIPT_DIR/003-accept-pty.log")" \ + "up --confirm answered y" +wl::pass "accepted run exited 0" + +grep -q 'Apply this deploy' "$TRANSCRIPT_DIR/003-accept-pty.log" \ + || wl::fail "SMOKE-003: prompt missing from the PTY transcript" +wl::pass "prompt preceded the apply in the PTY transcript" + +# (b) The sizing change was applied. `workload get` does not surface sizing, +# so the CLI's own live-vs-manifest verdict is the observable: with the +# manifest still asking for replicaCount 3, an up-to-date dry-run proves the +# live runtime now matches the manifest's new value. +wl::dr_capture workload up --dry-run +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "up --dry-run (after accept)" +smoke::record "003-dry-run-after-accept" +printf '%s\n' "$WL_ERR" | grep -q 'Already up to date' \ + || wl::fail "SMOKE-003: workload not up to date after the accepted deploy; stderr was: $WL_ERR" +wl::pass "live runtime now matches the manifest (up-to-date verdict)" + +# (d) stdout carries the endpoint: the accepted PTY transcript shows the +# endpoint line the declined one deliberately lacked. +wl::dr_capture workload get "$WID" --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload get (after-accept)" +endpoint="$(printf '%s' "$WL_OUT" | jq -r '.endpoint // empty')" +[[ -n "$endpoint" ]] || wl::fail "SMOKE-003: workload has no endpoint" +grep -qF "$endpoint" "$TRANSCRIPT_DIR/003-accept-pty.log" \ + || wl::fail "SMOKE-003: accepted transcript does not carry the endpoint" +wl::pass "accepted transcript carries the endpoint" + +wl::wait_for_status "$WID" running 300 >/dev/null + +wl::stop_timer + +# --- SMOKE-004: cleanup leaves no RUN-prefixed resources -------------------- +wl::start_timer "SMOKE-004: cleanup" + +# Run the same cleanup the EXIT trap would run, while the script can still +# assert on the result; the trap runs again on exit as a verified no-op. +smoke::cleanup_resources +wl::pass "cleanup deleted workload $WID (and artifact ${AID:-none})" + +wl::dr_capture workload list --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload list (post-cleanup)" +smoke::record "004-workload-list-post-cleanup" +leftover_wl="$(printf '%s' "$WL_OUT" \ + | jq -r --arg run "$RUN" '[.workloads[]? | select(.name | contains($run))] | length')" +[[ "$leftover_wl" == "0" ]] \ + || wl::fail "SMOKE-004: $leftover_wl RUN-prefixed workload(s) remain on staging" +wl::pass "no RUN-prefixed workloads remain (dr workload list)" + +if [[ -n "$AID" ]]; then + wl::dr_capture artifact get "$AID" --output-format json + if [[ "$WL_RC" -eq 0 ]]; then + wl::fail "SMOKE-004: artifact $AID still exists after cleanup" + fi + wl::pass "artifact $AID deleted (get fails as expected)" +fi + +wl::stop_timer + +wl::stop_timer +echo "โœ… RAPTOR-19538 diff/confirm smoke passed โ€” transcripts in $TRANSCRIPT_DIR" diff --git a/smoke_test_scripts/workload/TICKETS.md b/smoke_test_scripts/workload/TICKETS.md index 92b680e75..6e0701513 100644 --- a/smoke_test_scripts/workload/TICKETS.md +++ b/smoke_test_scripts/workload/TICKETS.md @@ -12,6 +12,7 @@ scenario script (grep with `rg '^# Ticket:' smoke_test_scripts/workload`). | C โ€” re-bind tuned | `RAPTOR-19533-C-rebind.sh` | RAPTOR-19533 | re-bind preserves live tuning; FileExists guard; delete-rebind restore | | D โ€” built-workload rebuild | `RAPTOR-19533-D-built.sh` | RAPTOR-19533 | no ErrImagePull after re-bind; platform rebuilds from imageBuildConfig | | Artifact lifecycle | `artifact-lifecycle.sh` | none โ€” basic acceptance | `dr artifact` create/get/list/code sync/versions/del CLI-side state | +| diff/confirm live smoke | `RAPTOR-19538-diff-confirm.sh` | RAPTOR-19538 | staging: `--dry-run --diff` redaction, `--confirm` decline/accept (PTY), JSON diff envelope, cleanup. Run directly: interactive PTY + staging writes | ## Adding a scenario for a new ticket From bfdc0aa67b0256d7cce8bd3bb5aed7f6c41a91eb Mon Sep 17 00:00:00 2001 From: AJ Alon Date: Mon, 31 Aug 2026 13:10:47 -0700 Subject: [PATCH 12/12] [RAPTOR-19538] polish(uidiff): consolidate redaction tests, pin per-kind styles --- cmd/workload/up/cmd_test.go | 2 + go.mod | 2 +- internal/uidiff/render.go | 4 + internal/uidiff/render_test.go | 133 +++++++++++++++++++++------- internal/workload/up/plan_test.go | 4 +- internal/workload/up/render_test.go | 10 ++- 6 files changed, 115 insertions(+), 40 deletions(-) diff --git a/cmd/workload/up/cmd_test.go b/cmd/workload/up/cmd_test.go index e5d4d49cc..5606fba0f 100644 --- a/cmd/workload/up/cmd_test.go +++ b/cmd/workload/up/cmd_test.go @@ -816,6 +816,8 @@ func TestCmd_FailedDraftRunOmitsTheLockLine(t *testing.T) { assert.NotContains(t, next, "--lock") assert.Contains(t, next, "dr workload logs 68b0c1d2e3f4a5b6c7d8e9f0", "the lines that can name the workload still do") +} + // TestCmd_ConfirmIsRegistered: the flag exists, is opt-in, and its help says // the one thing a reader could not guess -- that under non-interactive // conditions it is suppressed rather than refused, because a flag that fails diff --git a/go.mod b/go.mod index 6dfdfc288..87e549c70 100644 --- a/go.mod +++ b/go.mod @@ -22,6 +22,7 @@ require ( github.com/jeandeaual/go-locale v0.0.0-20250612000132-0ef82f21eade github.com/joho/godotenv v1.5.1 github.com/muesli/cancelreader v0.2.2 + github.com/muesli/termenv v0.16.0 github.com/sabhiram/go-gitignore v0.0.0-20210923224102-525f6e181f06 github.com/sergi/go-diff v1.4.0 github.com/spf13/cobra v1.10.2 @@ -69,7 +70,6 @@ require ( github.com/microcosm-cc/bluemonday v1.0.27 // indirect github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 // indirect github.com/muesli/reflow v0.3.0 // indirect - github.com/muesli/termenv v0.16.0 // indirect github.com/pelletier/go-toml/v2 v2.4.2 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/sagikazarmark/locafero v0.12.0 // indirect diff --git a/internal/uidiff/render.go b/internal/uidiff/render.go index bba216ee3..992390e28 100644 --- a/internal/uidiff/render.go +++ b/internal/uidiff/render.go @@ -178,6 +178,10 @@ func redactedText(row Row) string { return row.Path + ": " + hiddenPlaceholder } + // An out-of-range Kind falls through the switch the way Context does, + // and for the same reason: "set" and "changed" are claims about a + // transition the renderer recognizes, and it makes neither for a Kind it + // does not. return row.Path + ": " + hiddenPlaceholder } diff --git a/internal/uidiff/render_test.go b/internal/uidiff/render_test.go index 8442aaca8..76a10ce29 100644 --- a/internal/uidiff/render_test.go +++ b/internal/uidiff/render_test.go @@ -20,8 +20,12 @@ import ( "strings" "testing" + "github.com/charmbracelet/lipgloss" + "github.com/muesli/termenv" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/datarobot/cli/tui" ) // renderRows runs Render against a buffer, which is how every test below @@ -246,54 +250,117 @@ func TestRender_MarkersFollowThePlanVocabulary(t *testing.T) { stripANSI(out)) } -// VAL-DIFF-004: the redaction hook suppresses the text of every Kind it -// fires for, whatever the row was carrying, and never touches the others. +// VAL-DIFF-004: the redaction hook suppresses the value of every Kind it +// fires for, whatever the row was carrying, and touches nothing else. This +// table is the canonical Kind x redaction-state matrix: every Kind appears +// in each state the hook admits -- fired, passed over, and absent (nil +// hook) -- so no combination ships untested and the placeholder vocabulary +// has exactly one home. func TestRender_RedactionAppliesToEveryKind(t *testing.T) { - redact := func(path string) bool { - return strings.Contains(path, ".environmentVars[") + const ( + path = "runtime.environmentVars[TOKEN]" + secret = "plaintext-secret" + ) + + // The marker and placeholder each Kind earns: verbs for a value arriving + // and a value moving, the bracketed token where neither verb is true, + // and a marker only for the kinds that announce a change. + kinds := []struct { + name string + kind Kind + marker string + placeholder string + }{ + {name: "context", kind: Context, placeholder: hiddenPlaceholder}, + {name: "add", kind: Add, marker: addMarker, placeholder: setPlaceholder}, + {name: "del", kind: Del, marker: delMarker, placeholder: changedPlaceholder}, + {name: "unmanaged", kind: Unmanaged, placeholder: hiddenPlaceholder}, } - rows := []Row{ - ctxRow("port: 9090"), - {Kind: Context, Path: "runtime.environmentVars[LOG_LEVEL]", Text: "runtime.environmentVars[LOG_LEVEL]: debug"}, - {Kind: Add, Path: "runtime.environmentVars[TOKEN]", Text: "runtime.environmentVars[TOKEN]: plaintext-secret"}, - {Kind: Del, Path: "runtime.environmentVars[OLD]", Text: "runtime.environmentVars[OLD]: dr-credential:abc123/client-secret"}, - {Kind: Unmanaged, Path: "runtime.environmentVars[SIDE]", Text: "~ runtime.environmentVars[SIDE]: not managed by this file"}, - addRow("port: 9091"), + states := []struct { + name string + hook func(string) bool + redacted bool + }{ + // The firing hook is path-scoped, the way the workload caller's is: + // it refuses the row's path, not the whole rendering, so the anchor + // row keeps its value even here. + {name: "hook-fires", hook: func(p string) bool { return p == path }, redacted: true}, + {name: "hook-passes", hook: func(string) bool { return false }, redacted: false}, + {name: "no-hook", hook: nil, redacted: false}, } - out := stripANSI(renderRows(t, rows, Options{Redact: redact})) + for _, k := range kinds { + for _, s := range states { + t.Run(k.name+"/"+s.name, func(t *testing.T) { + // Context and Unmanaged rows only show inside the window of + // a change, so every case anchors its row to one; the + // anchor's line is part of each expectation. + rows := []Row{ + {Kind: k.kind, Path: path, Text: path + ": " + secret}, + {Kind: Add, Path: "anchor", Text: "anchor"}, + } - for _, secret := range []string{ - "plaintext-secret", - "dr-credential:abc123/client-secret", - "debug", - "not managed by this file", - } { - assert.NotContains(t, out, secret) - } + var opts Options + + if s.hook != nil { + opts.Redact = s.hook + } + + out := stripANSI(renderRows(t, rows, opts)) + + marker := k.marker + if marker != "" { + marker += " " + } - assert.Contains(t, out, "runtime.environmentVars[LOG_LEVEL]: (redacted)") - assert.Contains(t, out, "+ runtime.environmentVars[TOKEN]: set") - assert.Contains(t, out, "- runtime.environmentVars[OLD]: changed") - assert.Contains(t, out, "runtime.environmentVars[SIDE]: (redacted)") + want := marker + path + ": " + secret + if s.redacted { + want = marker + path + ": " + k.placeholder - // Rows the hook does not fire for keep their values, so redaction is a - // property of the path, not of the renderer. - assert.Contains(t, out, "port: 9090") - assert.Contains(t, out, "+ port: 9091") + assert.NotContains(t, out, secret, "a fired hook never lets the value through") + } + + assert.Equal(t, want+"\n+ anchor\n", out) + }) + } + } } -// Without a hook nothing is redacted; the placeholder machinery is opt-in. -func TestRender_WithoutARedactHookTheValuesPrint(t *testing.T) { +// The style mapping is invisible to the other tests because they strip ANSI: +// they prove what a row says, never what it wears. This one forces a color +// profile so the escapes survive, then checks each Kind against the palette +// entry it is documented to carry -- additions read as success, removals and +// unmanaged fields as warnings, and context as a hint. The expected strings +// come from the same styles the renderer uses, so the assertion holds +// whatever colors the profile downsamples to. +func TestRender_PerKindStylesWithForcedColorProfile(t *testing.T) { + original := lipgloss.ColorProfile() + + lipgloss.SetColorProfile(termenv.ANSI) + + t.Cleanup(func() { + lipgloss.SetColorProfile(original) + }) + rows := []Row{ - {Kind: Add, Path: "runtime.environmentVars[TOKEN]", Text: "runtime.environmentVars[TOKEN]: secret"}, - addRow("port: 9091"), + {Kind: Context, Path: "ctx", Text: "ctx"}, + {Kind: Add, Path: "added", Text: "added"}, + {Kind: Del, Path: "removed", Text: "removed"}, + {Kind: Unmanaged, Path: "extra", Text: "~ extra: not managed"}, } out := renderRows(t, rows, Options{}) - assert.Contains(t, stripANSI(out), "+ runtime.environmentVars[TOKEN]: secret") + // A profile that failed to engage would leave both sides of the checks + // below plain text and the whole test vacuous, so first prove that the + // rendering actually carries escapes. + assert.NotEqual(t, stripANSI(out), out, "forcing a color profile must leave ANSI escapes in the rendering") + + assert.Contains(t, out, tui.SuccessStyle.Render("+ added"), "Add wears the success style") + assert.Contains(t, out, tui.WarnStyle.Render("- removed"), "Del wears the warn style") + assert.Contains(t, out, tui.WarnStyle.Render("~ extra: not managed"), "Unmanaged wears the warn style, like Del") + assert.Contains(t, out, tui.HintStyle.Render("ctx"), "Context wears the hint style") } // The hook is a per-row decision: once per row, never per line of output. diff --git a/internal/workload/up/plan_test.go b/internal/workload/up/plan_test.go index 73dcbc74f..65d0530df 100644 --- a/internal/workload/up/plan_test.go +++ b/internal/workload/up/plan_test.go @@ -781,7 +781,7 @@ func TestBuild_DiffRowsMirrorTheArtifactRuntimeSplit(t *testing.T) { loadedFrom(drifted), liveFrom(t, StateRunning, planLiveSpec, planLiveRuntime), builtCode(0), - Options{}, + Options{}, ) require.NoError(t, err) @@ -844,7 +844,7 @@ func TestBuild_UnmanagedComesFromExtra(t *testing.T) { loadedFrom(planPayload), liveFrom(t, StateRunning, planLiveSpec, planLiveRuntime), builtCode(0), - Options{}, + Options{}, ) require.NoError(t, err) diff --git a/internal/workload/up/render_test.go b/internal/workload/up/render_test.go index cf7f255d5..613c1f0ae 100644 --- a/internal/workload/up/render_test.go +++ b/internal/workload/up/render_test.go @@ -561,6 +561,7 @@ func driftPlan(t *testing.T) Plan { loadedFrom(diffDriftPayload), liveFrom(t, StateRunning, planLiveSpec, planLiveRuntime), builtCode(0), + Options{}, ) require.NoError(t, err) @@ -738,7 +739,7 @@ func TestPlanJSON_DiffRedactsBeforeSerialising(t *testing.T) { ]}] }` - plan, err := Build(loadedFrom(payload), liveFrom(t, StateRunning, live, planLiveRuntime), builtCode(0)) + plan, err := Build(loadedFrom(payload), liveFrom(t, StateRunning, live, planLiveRuntime), builtCode(0), Options{}) require.NoError(t, err) // The fixture has to earn its keep: two env-var changes on the existing @@ -861,7 +862,7 @@ func TestPlanJSON_DiffRedactsAWholeEnvVarsList(t *testing.T) { ]}] }` - plan, err := Build(loadedFrom(payload), liveFrom(t, StateRunning, live, planLiveRuntime), builtCode(0)) + plan, err := Build(loadedFrom(payload), liveFrom(t, StateRunning, live, planLiveRuntime), builtCode(0), Options{}) require.NoError(t, err) // The fixture has to earn its keep: the change must arrive as the @@ -917,6 +918,7 @@ func TestPlanJSON_FirstDeployDiffIsAllAdditions(t *testing.T) { loadedFrom(planPayload), Live{State: StateUnbound}, CodeChange{Applies: true, FirstDeploy: true}, + Options{}, ) require.NoError(t, err) @@ -1060,7 +1062,7 @@ func TestPlanJSON_DiffSyntheticChangesSurvive(t *testing.T) { live := liveFrom(t, StateRunning, "", planLiveRuntime) live.ArtifactID = "68a0000000000000000000a1" - plan, err := Build(loaded, live, builtCode(0)) + plan, err := Build(loaded, live, builtCode(0), Options{}) require.NoError(t, err) require.Equal(t, []string{"artifactId"}, paths(plan.Artifact), @@ -1098,7 +1100,7 @@ func TestPlanJSON_DiffSyntheticTypeChangeSurvives(t *testing.T) { live := liveFrom(t, StateRunning, "", planLiveRuntime) live.ArtifactType = "service" - plan, err := Build(loaded, live, builtCode(0)) + plan, err := Build(loaded, live, builtCode(0), Options{}) require.NoError(t, err) require.Equal(t, []string{"artifact.type"}, paths(plan.Artifact))