internal/runtime/builtin/agentstep/convert.go (2)

125-144: Logic is correct; minor simplification possible.

The nested if len(inputs) > 0 wrapper is redundant — for k := range inputs over a nil/empty map is a no-op, and make([]string, 0, len(nil)) is valid. You can flatten the block without changing behavior:

♻️ Optional simplification

-	keys := make([]string, 0, len(inputs))
-	if len(inputs) > 0 {
-		var allowed map[string]struct{}
-		if approval != nil {
-			// Filter to declared input fields only (consistent with env var allowlist).
-			allowed = make(map[string]struct{}, len(approval.Input))
-			for _, key := range approval.Input {
-				allowed[key] = struct{}{}
-			}
-		}
-
-		for k := range inputs {
-			if allowed != nil {
-				if _, ok := allowed[k]; !ok {
-					continue
-				}
-			}
-			keys = append(keys, k)
-		}
-	}
+	var allowed map[string]struct{}
+	if approval != nil {
+		// Filter to declared input fields only (consistent with env var allowlist).
+		allowed = make(map[string]struct{}, len(approval.Input))
+		for _, key := range approval.Input {
+			allowed[key] = struct{}{}
+		}
+	}
+	keys := make([]string, 0, len(inputs))
+	for k := range inputs {
+		if allowed != nil {
+			if _, ok := allowed[k]; !ok {
+				continue
+			}
+		}
+		keys = append(keys, k)
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/runtime/builtin/agentstep/convert.go` around lines 125 - 144, Remove
the redundant `if len(inputs) > 0` wrapper and flatten the block so the keys
slice creation and the loop over `inputs` happen unconditionally; keep the
existing approval-based allowlist logic (the `allowed` map creation when
`approval != nil` and the `for k := range inputs` loop that checks `allowed`)
intact, referencing the `keys` variable, `inputs`, `approval`, and `allowed` so
the behavior is unchanged but the extra conditional is eliminated.

---

119-120: Doc comment is now stale.

The comment still says "filtering to only declared approval input fields", but with the new behavior the allowlist is only applied when approval != nil. Update it to reflect the conditional filtering so future readers don't get misled.

📝 Suggested wording

-// formatPushBackFeedback creates a user message from push-back inputs,
-// filtering to only declared approval input fields.
+// formatPushBackFeedback creates a user message from push-back inputs.
+// When approval is non-nil, the inputs are filtered to its declared Input
+// allowlist (consistent with the env var allowlist). When approval is nil
+// (e.g. push-back originated outside an approval step), all inputs are
+// included.
 func formatPushBackFeedback(inputs map[string]string, approval *core.ApprovalConfig) string {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/runtime/builtin/agentstep/convert.go` around lines 119 - 120, Update
the doc comment for formatPushBackFeedback to reflect the new conditional
behavior: state that the function creates a user message from push-back inputs
and that the approval-field allowlist is applied only when the approval
parameter is non-nil (otherwise inputs are passed through unfiltered); reference
the function name formatPushBackFeedback and its approval parameter in the
comment so readers understand the conditional filtering behavior.

internal/runtime/builtin/agentstep/convert_test.go (1)

232-241: Optional: strengthen the assertion.

The new behavior also emits the Reviewer feedback: header when approval is nil but inputs are non-empty. Adding an explicit assertion would lock in that contract and guard against accidental regressions where the header is omitted but the bullets still rendered (or vice versa).

🧪 Suggested strengthening

 	assert.Contains(t, result, "requested changes")
+	assert.Contains(t, result, "Reviewer feedback:")
 	assert.Contains(t, result, "- KEY: value")
+	assert.Contains(t, result, "revise your response")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/runtime/builtin/agentstep/convert_test.go` around lines 232 - 241,
TestFormatPushBackFeedback_NilApproval currently asserts only for "requested
changes" and the bullet, but the function formatPushBackFeedback also emits a
"Reviewer feedback:" header when approval is nil and inputs are non-empty;
update the test to explicitly assert that the result contains "Reviewer
feedback:" so the contract is locked in. Modify
TestFormatPushBackFeedback_NilApproval to include an assert.Contains(t, result,
"Reviewer feedback:") alongside the existing assertions, referencing the test
name and formatPushBackFeedback to locate the change.

api/v1/api.yaml (1)

8839-8859: Tighten PushBackHistoryEntry validation for stronger client contracts.

Line 8844 should constrain iteration, and Line 8850 should declare date-time format since the description already says RFC3339.

♻️ Proposed OpenAPI schema tightening

     PushBackHistoryEntry:
       type: object
       description: "One push-back event recorded for an approval step"
       properties:
         iteration:
           type: integer
+          minimum: 1
           description: "Push-back iteration number"
         by:
           type: string
           description: "Authenticated user who pushed the step back"
         at:
           type: string
+          format: date-time
           description: "RFC3339 timestamp when the push-back was recorded"
         inputs:
           type: object
           additionalProperties:
             type: string
           description: "Inputs provided for this push-back event"
       required:
         - iteration

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@api/v1/api.yaml` around lines 8839 - 8859, The PushBackHistoryEntry schema
needs stronger validation: add a numeric constraint for the iteration property
(e.g., add "minimum: 1" to the iteration property to enforce positive
iterations) and declare the timestamp format for the at property by adding
"format: date-time" so the RFC3339 description is enforced; update the
PushBackHistoryEntry -> properties -> iteration and PushBackHistoryEntry ->
properties -> at entries accordingly.

internal/core/exec/pushback.go (1)

56-72: Consolidate duplicate clonePushBackHistory implementations.

The exported ClonePushBackHistory here is functionally equivalent to the unexported clonePushBackHistory in internal/runtime/transform/node.go and internal/service/frontend/api/v1/dagruns_edit_retry.go. Consider having those callers reuse exec.ClonePushBackHistory to avoid drift (e.g., one variant uses an explicit loop for Inputs, the other uses maps.Clone/cloneStringMap).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/exec/pushback.go` around lines 56 - 72, Duplicate
implementations of clonePushBackHistory exist; consolidate by making callers
reuse the exported ClonePushBackHistory in exec. Replace the unexported
clonePushBackHistory functions in the other modules with calls to
exec.ClonePushBackHistory (preserving behavior for Inputs via maps.Clone),
remove the duplicate local functions, and update imports to reference the exec
package; ensure signatures match (slice of PushBackEntry) and run tests to
confirm no behavioral changes.

internal/core/exec/env.go (1)

46-48: Consider DAG_PUSH_BACK for consistency with other multi-word env keys.

All other multi-word env keys in this file are snake_case (e.g., DAG_RUN_LOG_FILE, DAG_RUN_STEP_NAME, DAG_RUN_WORK_DIR). DAG_PUSHBACK collapses the words, which is inconsistent. Since this is a user-facing env var that gets baked into DAG scripts, it'll be hard to change once released. Worth confirming the chosen name is intentional.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/exec/env.go` around lines 46 - 48, The constant
EnvKeyDAGPushBack currently defines the env var value as "DAG_PUSHBACK" which is
inconsistent with other multi-word keys; change its value to "DAG_PUSH_BACK" in
the EnvKeyDAGPushBack declaration and update any places that read or write that
env var (templates, DAG scripts, tests, and any usage of EnvKeyDAGPushBack) to
reference the new name; if backward compatibility is required, add code to
accept the old "DAG_PUSHBACK" as a deprecated alias when reading environment
variables and add/update tests and docs to reflect the new canonical name.

internal/core/spec/step.go (1)

1973-1978: Consider trimming whitespace on RewindTo.

Other ID/name fields are trimmed via strings.TrimSpace (e.g., buildStepID, buildStepName, Call). Since RewindTo is matched against step IDs/names by the validator, accidental leading/trailing whitespace in the YAML will likely fail validation with a confusing "not found" message. Consider trimming here for consistency:

Proposed change

 	result.Approval = &core.ApprovalConfig{
 		Prompt:   s.Approval.Prompt,
 		Input:    s.Approval.Input,
 		Required: s.Approval.Required,
-		RewindTo: s.Approval.RewindTo,
+		RewindTo: strings.TrimSpace(s.Approval.RewindTo),
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/spec/step.go` around lines 1973 - 1978, Trim whitespace from
the approval RewindTo value before assigning it to the core.ApprovalConfig to
match behavior used for other ID/name fields; in the block that constructs
result.Approval (where result.Approval = &core.ApprovalConfig{ Prompt:
s.Approval.Prompt, Input: s.Approval.Input, Required: s.Approval.Required,
RewindTo: s.Approval.RewindTo, }), replace the direct assignment of
s.Approval.RewindTo with strings.TrimSpace(s.Approval.RewindTo) and ensure the
strings package is imported so RewindTo comparisons by the validator won’t fail
due to leading/trailing whitespace.

internal/runtime/transform/node.go (1)

56-57: Prefer the shared exec.ClonePushBackHistory helper over a local clone implementation.

This duplicates existing logic and increases drift risk across transformation paths.

♻️ Proposed simplification

@@
-		PushBackHistory:   clonePushBackHistory(n.PushBackHistory),
+		PushBackHistory:   exec.ClonePushBackHistory(n.PushBackHistory),
@@
-		PushBackHistory:   clonePushBackHistory(node.State.PushBackHistory),
+		PushBackHistory:   exec.ClonePushBackHistory(node.State.PushBackHistory),
 	}
 }
-
-func clonePushBackHistory(src []exec.PushBackEntry) []exec.PushBackEntry {
-	if len(src) == 0 {
-		return nil
-	}
-
-	dst := make([]exec.PushBackEntry, len(src))
-	for i, entry := range src {
-		dst[i] = exec.PushBackEntry{
-			Iteration: entry.Iteration,
-			By:        entry.By,
-			At:        entry.At,
-		}
-		if len(entry.Inputs) > 0 {
-			inputs := make(map[string]string, len(entry.Inputs))
-			for key, value := range entry.Inputs {
-				inputs[key] = value
-			}
-			dst[i].Inputs = inputs
-		}
-	}
-	return dst
-}

Also applies to: 100-101, 104-125

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/runtime/transform/node.go` around lines 56 - 57, Replace the local
clone implementation calls to clonePushBackHistory with the shared helper
exec.ClonePushBackHistory to avoid duplicated logic; specifically, update the
struct construction that sets PushBackHistory (and the other occurrences around
the noted blocks) to call exec.ClonePushBackHistory(n.PushBackHistory) instead
of clonePushBackHistory, and remove the now-unused local clonePushBackHistory
function if it becomes unreferenced.

internal/runtime/runner.go (1)

446-478: Recommended refactor: DRY the approval lookup and simplify the warning loop.

node.Step().Approval is fetched twice (lines 452 and 459), and the warning loop rebuilds an allowed set that already corresponds to the keys present in filteredInputs. You can hoist the lookup once and use filteredInputs directly for membership checks:

♻️ Proposed simplification

 if node.State().ApprovalIteration > 0 {
 	state := node.State()
 	env := GetEnv(ctx)
+	approval := node.Step().Approval
 	var allowedInputs []string
-	if approval := node.Step().Approval; approval != nil {
+	if approval != nil {
 		allowedInputs = approval.Input
 	}
 	filteredInputs := exec.FilterPushBackInputs(allowedInputs, state.PushBackInputs)
 	for k, v := range filteredInputs {
 		env = env.WithEnvVars(k, v)
 	}
-	if approval := node.Step().Approval; approval != nil && len(filteredInputs) != len(state.PushBackInputs) {
-		allowed := make(map[string]struct{}, len(approval.Input))
-		for _, key := range approval.Input {
-			allowed[key] = struct{}{}
-		}
+	if approval != nil && len(filteredInputs) != len(state.PushBackInputs) {
 		for k := range state.PushBackInputs {
-			if _, ok := allowed[k]; ok {
+			if _, ok := filteredInputs[k]; ok {
 				continue
 			}
 			logger.Warn(ctx, "Ignoring unexpected push-back input", slog.String("input", k))
 		}
 	}
 	payload, err := marshalPushBackPayload(allowedInputs, state)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/runtime/runner.go` around lines 446 - 478, Hoist the approval lookup
into a single variable (e.g., approval := node.Step().Approval) and reuse it
instead of calling node.Step().Approval twice; keep the existing allowedInputs
derivation from approval.Input, use exec.FilterPushBackInputs(...) as before to
get filteredInputs, and simplify the warning loop by checking membership against
filteredInputs (or build the allowed set once from approval.Input if you prefer)
when iterating state.PushBackInputs to emit logger.Warn; also reuse the hoisted
approval when building the allowed map or deciding to perform the length
comparison and when calling marshalPushBackPayload(allowedInputs, state) so the
code no longer repeats node.Step().Approval.

internal/intg/approval_test.go (1)

370-374: Hoist the repeated indentTestScript(pushBackSnapshotScript(), 6) to a variable.

Calling the same expression four times for the four steps is harmless but a bit noisy. Computing it once and reusing the result keeps the test a touch tidier.

♻️ Optional DRY tweak

-`, dagName,
-		indentTestScript(pushBackSnapshotScript(), 6),
-		indentTestScript(pushBackSnapshotScript(), 6),
-		indentTestScript(pushBackSnapshotScript(), 6),
-		indentTestScript(pushBackSnapshotScript(), 6))
+`, dagName, snippet, snippet, snippet, snippet)

with snippet := indentTestScript(pushBackSnapshotScript(), 6) declared above the fmt.Sprintf call.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/intg/approval_test.go` around lines 370 - 374, Hoist the repeated
call indentTestScript(pushBackSnapshotScript(), 6) into a single variable (e.g.,
snippet) declared just above the fmt.Sprintf call and use that variable in place
of the four repeated expressions; this reduces duplication while leaving
functions like pushBackSnapshotScript() and indentTestScript(...) unchanged and
keeps the fmt.Sprintf invocation concise.

ui/src/features/dags/components/common/PushBackHistory.tsx (1)

50-67: Optional: align metadata block styling with the project's slate palette.

The coding guidelines call for bg-slate-200 dark:bg-slate-700 for metadata/information blocks and the documented text-slate-{800/600/500} hierarchy for primary/secondary/muted text. This component instead uses shadcn theme variables (bg-muted/30, text-foreground/90, text-muted-foreground). Functionally fine, but inconsistent with the convention used in similarly-purposed information blocks elsewhere in the UI.

If the rest of the dags surface follows the slate-palette convention, consider switching for visual consistency in light/dark modes. If shadcn theme vars are the new direction, this is a no-op.

As per coding guidelines: "Use consistent metadata styling with bg-slate-200 dark:bg-slate-700 for similar information blocks to support both light and dark modes" and "Use text color hierarchy with primary text as text-slate-800 dark:text-slate-200, secondary as text-slate-600 dark:text-slate-400, and muted as text-slate-500 dark:text-slate-500".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ui/src/features/dags/components/common/PushBackHistory.tsx` around lines 50 -
67, Update the metadata block's Tailwind classes to use the project's slate
palette instead of shadcn tokens: replace the container classes ("bg-muted/30",
"border-border") on the outer div (the element keyed by
`${entry.iteration}-${entry.at || 'pending'}` in PushBackHistory.tsx) with
"bg-slate-200 dark:bg-slate-700" and keep rounded/border/padding as-is; change
the primary text span (currently "text-foreground/90") to "text-slate-800
dark:text-slate-200", the secondary spans (currently "text-muted-foreground" and
inner "text-foreground/80") to "text-slate-600 dark:text-slate-400" and
"text-slate-500 dark:text-slate-500" respectively so the iteration/by/at
hierarchy matches the documented slate-{800/600/500} scheme while preserving
layout and the use of formatTimestamp(entry.at).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/runtime/transform/node.go (1)

55-55: Consider also cloning PushBackInputs for consistency.

PushBackHistory is now deep-copied, but the adjacent PushBackInputs (a map[string]string) is still assigned by reference here, while the API path in internal/service/frontend/api/v1/dagruns.go (around lines 3379–3381) explicitly uses cloneStringMap(inputs) before assignment. If the runtime↔persistence boundary is meant to fully isolate state, mirroring the same maps.Clone treatment for PushBackInputs (in both ToNode and newNode) would prevent surprises if either side later mutates the map.

Note that ApprovalInputs / OutputVariables exhibit the same pre-existing pattern, so this is purely about matching the new cloning intent — feel free to defer.

Also applies to: 99-99

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/runtime/transform/node.go` at line 55, The code currently
deep-copies PushBackHistory but assigns PushBackInputs by reference; update the
conversions (ToNode and newNode) to clone PushBackInputs as well (e.g., use
maps.Clone or the existing cloneStringMap helper) so runtime↔persistence
boundaries fully isolate state—apply the same cloning treatment where
PushBackInputs is assigned, mirroring the PushBackHistory handling and ensuring
consistency with the API path that calls cloneStringMap(inputs).

api/v1/api.yaml (1)

7699-7701: Disallow empty rewindTo values.

On Line 7700, rewindTo accepts "". Adding minLength: 1 tightens request validation and avoids ambiguous rewind targets.

Proposed diff

         rewindTo:
           type: string
+          minLength: 1
           description: "Optional step name to restart from when the approver pushes the step back. Must reference the step itself or an upstream dependency."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@api/v1/api.yaml` around lines 7699 - 7701, The rewindTo schema currently
allows empty strings; update the OpenAPI property named rewindTo (the string
field with description "Optional step name to restart from...") to disallow
empty values by adding a minLength: 1 constraint to its schema so validation
rejects "" values and only accepts non-empty step identifiers.

internal/service/frontend/api/v1/transformer.go (1)

354-387: Silently dropped invalid timestamps may obscure data corruption.

When time.Parse(time.RFC3339, entry.At) fails (line 381), the parse error is discarded and items[i].At is left nil, which renders as a missing field in the API response. If NormalizePushBackHistory ever emits a malformed timestamp due to a bug or storage corruption, this path will hide it from both clients and operators.

Consider logging the parse error (or surfacing it as a metric) so unexpected formats become visible.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/frontend/api/v1/transformer.go` around lines 354 - 387,
toPushBackHistory currently swallows time.Parse errors for entry.At, hiding
malformed timestamps; update this function to surface parse failures by logging
the error (including entry.At and entry.Iteration and any identifying node info
available) when time.Parse(time.RFC3339, entry.At) returns an error, and
optionally emit a metric; keep the existing behavior of leaving items[i].At nil
on parse failure but ensure the parse error is logged via the project's logger
(or log.Printf if no logger is available) so malformed timestamps from
NormalizePushBackHistory become visible.

internal/intg/approval_test.go (1)

509-547: Consider reusing exec.PushBackEntry for the History field and add DisallowUnknownFields validation.

The requirePushBackPayload helper redeclares the payload struct inline (lines 514–528). The History field's struct definition exactly matches the exported exec.PushBackEntry type from internal/core/exec/node.go, which the production code already uses. Reusing exec.PushBackEntry eliminates duplication and would catch schema drift for history items. Additionally, since json.Unmarshal ignores unknown fields by default, the entire payload is at risk of silently accepting schema changes. Adding json.Decoder.DisallowUnknownFields() during unmarshalling would catch any unexpected schema additions or removals in production code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/intg/approval_test.go` around lines 509 - 547, The inline payload
struct in requirePushBackPayload should reuse the exported exec.PushBackEntry
type for the History field and fail on unknown JSON fields: replace the
anonymous History item type with []exec.PushBackEntry (to match
internal/core/exec/node.go) and parse raw using a json.Decoder with
DisallowUnknownFields() (e.g., create a decoder from the raw string and call
decoder.DisallowUnknownFields() before Decode(&payload)); ensure any necessary
imports (e.g., strings) are added. This removes duplication and causes the test
to error on unexpected schema changes.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes