Ver código fonte

fix(#952): carry arguments when rerunning an action

The Rerun button ignored the original arguments and always started
actions with an empty argument set, so actions that take arguments
threw instead of re-running.

Rerun now reuses the arguments stored on the log entry: complete
runs restart directly, and incomplete ones (missing values, or
password / very_dangerous_raw_string fields) open the argument form
pre-filled via history.state. Justification-required actions always
re-prompt so each execution is explicitly justified.

Storable arguments are copied onto the log entry only after argument
parsing succeeds, so failed executions no longer persist arguments
that could be replayed.

Co-authored-by: Cursor <cursoragent@cursor.com>
jamesread 1 mês atrás
pai
commit
e2bf82ff9c

+ 32 - 14
frontend/resources/vue/utils/rerunArguments.js

@@ -10,6 +10,24 @@ function isNonStorableArgumentType (type) {
   return nonStorableArgumentTypes.has(type)
 }
 
+function argumentSkipsValidation (type) {
+  return type === 'confirmation' || type === 'html'
+}
+
+/**
+ * Mirrors backend restartArgumentRequired: an argument needs a stored value
+ * when it is validated and has no default. Proto ActionArgument has no
+ * `required` field — only `defaultValue`.
+ */
+function rerunArgumentRequired (arg) {
+  if (argumentSkipsValidation(arg?.type)) {
+    return false
+  }
+
+  const defaultValue = arg?.defaultValue ?? ''
+  return defaultValue === ''
+}
+
 export function logEntryArgumentsToStartActionArgs (logEntry) {
   return (logEntry?.arguments ?? []).map((arg) => ({
     name: arg.name,
@@ -18,7 +36,9 @@ export function logEntryArgumentsToStartActionArgs (logEntry) {
 }
 
 export function rerunNeedsArgumentForm (action, logEntry) {
-  if (actionRequiresJustification(action?.justification) && !logEntry?.justification) {
+  // Always re-prompt when justification is required so each execution is
+  // explicitly justified rather than silently reusing a prior reason.
+  if (actionRequiresJustification(action?.justification)) {
     return true
   }
 
@@ -37,7 +57,7 @@ export function hasMissingRerunArguments (action, storedArgs) {
       return true
     }
 
-    if (arg.required && !stored.has(arg.name)) {
+    if (rerunArgumentRequired(arg) && !stored.has(arg.name)) {
       return true
     }
   }
@@ -45,25 +65,23 @@ export function hasMissingRerunArguments (action, storedArgs) {
   return false
 }
 
-export function buildArgumentFormQuery (logEntry) {
-  const query = {}
+/**
+ * Builds history.state.prefilledArguments for ArgumentForm (not URL query),
+ * matching ActionButton's prefill pattern and keeping values out of the URL.
+ */
+export function buildRerunPrefilledArguments (logEntry) {
+  const prefilled = {}
 
   for (const arg of logEntry?.arguments ?? []) {
-    query[arg.name] = arg.value
+    prefilled[arg.name] = arg.value
   }
 
-  return query
+  return prefilled
 }
 
-export function buildRerunStartActionArgs (bindingId, logEntry, action) {
-  const startActionArgs = {
+export function buildRerunStartActionArgs (bindingId, logEntry) {
+  return {
     bindingId,
     arguments: logEntryArgumentsToStartActionArgs(logEntry)
   }
-
-  if (actionRequiresJustification(action?.justification) && logEntry?.justification) {
-    startActionArgs.justification = logEntry.justification
-  }
-
-  return startActionArgs
 }

+ 59 - 22
frontend/resources/vue/utils/rerunArguments.test.mjs

@@ -1,7 +1,7 @@
 import test from 'node:test'
 import assert from 'node:assert/strict'
 import {
-  buildArgumentFormQuery,
+  buildRerunPrefilledArguments,
   buildRerunStartActionArgs,
   hasMissingRerunArguments,
   logEntryArgumentsToStartActionArgs,
@@ -26,8 +26,8 @@ test('logEntryArgumentsToStartActionArgs maps proto arguments for StartAction',
 test('hasMissingRerunArguments requires password fields to be re-entered', () => {
   const action = {
     arguments: [
-      { name: 'user', type: 'ascii_identifier', required: true },
-      { name: 'pass', type: 'password', required: true }
+      { name: 'user', type: 'ascii_identifier' },
+      { name: 'pass', type: 'password' }
     ]
   }
 
@@ -40,8 +40,8 @@ test('hasMissingRerunArguments requires password fields to be re-entered', () =>
 test('hasMissingRerunArguments requires very_dangerous_raw_string fields to be re-entered', () => {
   const action = {
     arguments: [
-      { name: 'host', type: 'ascii_identifier', required: true },
-      { name: 'payload', type: 'very_dangerous_raw_string', required: false }
+      { name: 'host', type: 'ascii_identifier' },
+      { name: 'payload', type: 'very_dangerous_raw_string' }
     ]
   }
 
@@ -51,9 +51,10 @@ test('hasMissingRerunArguments requires very_dangerous_raw_string fields to be r
   )
 })
 
-test('hasMissingRerunArguments detects missing required stored arguments', () => {
+test('hasMissingRerunArguments detects missing stored args without relying on required flag', () => {
+  // Proto ActionArgument has no `required` field — only defaultValue.
   const action = {
-    arguments: [{ name: 'host', type: 'ascii_identifier', required: true }]
+    arguments: [{ name: 'host', type: 'ascii_identifier' }]
   }
 
   assert.equal(hasMissingRerunArguments(action, []), true)
@@ -63,9 +64,40 @@ test('hasMissingRerunArguments detects missing required stored arguments', () =>
   )
 })
 
+test('hasMissingRerunArguments treats empty defaultValue as required', () => {
+  const action = {
+    arguments: [{ name: 'host', type: 'ascii_identifier', defaultValue: '' }]
+  }
+
+  assert.equal(hasMissingRerunArguments(action, []), true)
+})
+
+test('hasMissingRerunArguments allows args that have a defaultValue', () => {
+  const action = {
+    arguments: [{ name: 'host', type: 'ascii_identifier', defaultValue: 'example.com' }]
+  }
+
+  assert.equal(hasMissingRerunArguments(action, []), false)
+})
+
+test('hasMissingRerunArguments ignores confirmation and html args', () => {
+  const action = {
+    arguments: [
+      { name: 'confirm', type: 'confirmation' },
+      { name: 'help', type: 'html' },
+      { name: 'host', type: 'ascii_identifier' }
+    ]
+  }
+
+  assert.equal(
+    hasMissingRerunArguments(action, [{ name: 'host', value: 'db-1' }]),
+    false
+  )
+})
+
 test('rerunNeedsArgumentForm can start directly when stored args are complete', () => {
   const action = {
-    arguments: [{ name: 'host', type: 'ascii_identifier', required: true }]
+    arguments: [{ name: 'host', type: 'ascii_identifier' }]
   }
   const logEntry = {
     arguments: [{ name: 'host', value: 'db-1' }]
@@ -74,36 +106,36 @@ test('rerunNeedsArgumentForm can start directly when stored args are complete',
   assert.equal(rerunNeedsArgumentForm(action, logEntry), false)
 })
 
-test('rerunNeedsArgumentForm opens the form when justification is missing', () => {
-  const action = { justification: ' ', arguments: [] }
+test('rerunNeedsArgumentForm always opens the form when justification is required', () => {
+  const action = {
+    justification: ' ',
+    arguments: [{ name: 'host', type: 'ascii_identifier' }]
+  }
+  const logEntry = {
+    arguments: [{ name: 'host', value: 'db-1' }],
+    justification: 'approved change'
+  }
 
+  assert.equal(rerunNeedsArgumentForm(action, logEntry), true)
   assert.equal(rerunNeedsArgumentForm(action, {}), true)
-  assert.equal(
-    rerunNeedsArgumentForm(action, { justification: 'approved change' }),
-    false
-  )
 })
 
-test('buildRerunStartActionArgs includes stored justification', () => {
+test('buildRerunStartActionArgs uses stored arguments without justification', () => {
   assert.deepEqual(
     buildRerunStartActionArgs('binding-1', {
       arguments: [{ name: 'host', value: 'db-1' }],
       justification: 'maintenance window'
-    }, {
-      justification: ' ',
-      arguments: [{ name: 'host', type: 'ascii_identifier' }]
     }),
     {
       bindingId: 'binding-1',
-      arguments: [{ name: 'host', value: 'db-1' }],
-      justification: 'maintenance window'
+      arguments: [{ name: 'host', value: 'db-1' }]
     }
   )
 })
 
-test('buildArgumentFormQuery prefills non-password stored arguments', () => {
+test('buildRerunPrefilledArguments maps stored args for history.state', () => {
   assert.deepEqual(
-    buildArgumentFormQuery({
+    buildRerunPrefilledArguments({
       arguments: [
         { name: 'host', value: 'db-1' },
         { name: 'port', value: '5432' }
@@ -115,3 +147,8 @@ test('buildArgumentFormQuery prefills non-password stored arguments', () => {
     }
   )
 })
+
+test('buildRerunPrefilledArguments returns empty object when nothing was stored', () => {
+  assert.deepEqual(buildRerunPrefilledArguments({}), {})
+  assert.deepEqual(buildRerunPrefilledArguments(undefined), {})
+})

+ 11 - 7
frontend/resources/vue/views/ExecutionView.vue

@@ -100,7 +100,11 @@ import { WorkoutRunIcon, Cancel02Icon, ArrowLeftIcon, DashboardSquare01Icon, Cop
 import { useRouter } from 'vue-router'
 import { buttonResults } from '../stores/buttonResults'
 import { requestReconnectNow } from '../../../js/websocket.js'
-import { needsArgumentForm } from '../utils/needsArgumentForm.js'
+import {
+  buildRerunPrefilledArguments,
+  buildRerunStartActionArgs,
+  rerunNeedsArgumentForm
+} from '../utils/rerunArguments.js'
 
 const router = useRouter()
 
@@ -224,16 +228,16 @@ async function rerunAction() {
 
   try {
     const binding = await window.client.getActionBinding({ bindingId })
-    if (needsArgumentForm(binding.action)) {
-      router.push(`/actionBinding/${bindingId}/argumentForm`)
+    if (rerunNeedsArgumentForm(binding.action, logEntry.value)) {
+      router.push({
+        path: `/actionBinding/${bindingId}/argumentForm`,
+        state: { prefilledArguments: buildRerunPrefilledArguments(logEntry.value) }
+      })
       return
     }
 
     requestReconnectNow()
-    const startActionArgs = {
-      bindingId: bindingId,
-      arguments: []
-    }
+    const startActionArgs = buildRerunStartActionArgs(bindingId, logEntry.value)
 
     const res = await window.client.startAction(startActionArgs)
     router.push(`/logs/${res.executionTrackingId}`)

+ 19 - 3
service/internal/executor/executor.go

@@ -846,6 +846,19 @@ func stepACLCheck(req *ExecutionRequest) bool {
 }
 
 func stepParseArgs(req *ExecutionRequest) bool {
+	if !prepareArgumentsForExecution(req) {
+		return false
+	}
+
+	ok := parseActionForExecution(req)
+	if ok {
+		copyStorableArgumentsToLogEntry(req)
+	}
+
+	return ok
+}
+
+func prepareArgumentsForExecution(req *ExecutionRequest) bool {
 	ensureArgumentMap(req)
 
 	if !hasBindingAndAction(req) {
@@ -856,14 +869,17 @@ func stepParseArgs(req *ExecutionRequest) bool {
 	if err := injectSystemArgs(req); err != nil {
 		return fail(req, err)
 	}
+
 	mangleInvalidArgumentValues(req)
-	copyStorableArgumentsToLogEntry(req)
+	return true
+}
 
+func parseActionForExecution(req *ExecutionRequest) bool {
 	if hasExec(req) {
 		return handleExecBranch(req)
-	} else {
-		return handleShellBranch(req)
 	}
+
+	return handleShellBranch(req)
 }
 
 func handleExecBranch(req *ExecutionRequest) bool {