fix(studio): refuse an empty run scope, and resolve a run once before it starts

CHANGELOG.md

@@ -12,7 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 - Secret text is masked more thoroughly in warnings, run output and recordings. See the repository security advisories for details.
 
 ### Added
-- **Studio runs part of a file.** Next to **Run**, a scope control picks the whole file (the default), the test at the cursor, the tests with one tag, or the tests that failed the last time the file ran in this session. Results group failures first when a run has more than one outcome. Automation's `run_file` takes `line`, `tag` or `failedOnly`, and the MCP tool `studio_run_file` takes `line`, `tag` or `failed_only`; give at most one. The file must be inside an open workspace or an automation folder, the same check Run File makes. A scope that matches no test is an error and nothing runs. Two tests that share a name refuse a scope that names one of them. The failed tests are kept in memory, so a restart starts with none. The CLI has no per-test filter yet, so `probe test` is unchanged.
+- **Studio runs part of a file.** Next to **Run**, a scope control picks the whole file (the default), the test at the cursor, the tests with one tag, or the tests that failed the last time the file ran in this session. Results group failures first when a run has more than one outcome. Automation's `run_file` takes `line`, `tag` or `failedOnly`, and the MCP tool `studio_run_file` takes `line`, `tag` or `failed_only`; give at most one; a line of 0 or an empty tag is refused rather than run as the whole file. The file must be inside an open workspace or an automation folder, the same check Run File makes. A scope that matches no test is an error and nothing runs. Two tests that share a name refuse a scope that names one of them. The failed tests are kept in memory, so a restart starts with none. The CLI has no per-test filter yet, so `probe test` is unchanged.
 - **MCP: `start_tests`, `test_status`, `test_result` and `cancel_tests` run tests in the background.** `run_tests` blocks until `probe test` exits, so a multi-device run (`--parallel`, `composite_devices`, several devices) could outlast the MCP client's tool timeout and be reported as failed while it kept running. `start_tests` takes the same arguments, returns a `job_id` at once and runs the command in its own process group; `test_status` returns the state, elapsed time and the last lines of output; `test_result` returns the retained output (the last 1 MB) and the exit code; `cancel_tests` sends SIGTERM to the run and kills it after 5 seconds. Jobs live in the `probe-mcp` process and are killed when it exits. The hosted-upload flags are refused here exactly as in `run_tests`. `run_tests` is unchanged. The MCP server now has 40 tools.
 - **`probe lint` checks the references a run would resolve.** Lint reports each step and recipe call that matches no built-in step and no recipe in the file, its `use` statements or the recipes folder (the check `probe test --dry-run` makes), each selector that can never match, and each `<name>` placeholder that no example column, recipe parameter or stored value defines. It runs without a device, gives the line of each finding, and checks hook bodies and the device blocks of composite tests too.
 - **Recordings show the element each step acted on.** A step that taps, double taps, long presses, types, clears, toggles, or swipes or scrolls with a selector records its selector, and its bounds and tap point when the element is on screen as the step starts. The viewer shows them under "Element". Text typed into a field is never recorded, and a failed lookup is an advisory warning that does not change the step.

internal/mcp/studio.go

@@ -170,22 +170,24 @@ func studioCall(method string, params any) (json.RawMessage, error) {
 // from the raw arguments: the string map the other tools read would hold an empty string for a number or a boolean.
 func studioRunParams(raw json.RawMessage) (map[string]any, error) {
 	var a struct {
-		Path       string `json:"path"`
-		Line       int    `json:"line"`
-		Tag        string `json:"tag"`
-		FailedOnly bool   `json:"failed_only"`
+		Path       string  `json:"path"`
+		Line       *int    `json:"line"`
+		Tag        *string `json:"tag"`
+		FailedOnly bool    `json:"failed_only"`
 	}
 	if len(raw) > 0 {
 		if err := json.Unmarshal(raw, &a); err != nil {
 			return nil, fmt.Errorf("studio_run_file takes path (text), line (number), tag (text) and failed_only (true or false): %w", err)
 		}
 	}
+	// A line or tag the caller sent is passed on as sent, so Studio refuses an explicit 0 or an empty tag instead of running
+	// the whole file. A line or tag left out is not sent.
 	params := map[string]any{"path": a.Path}
-	if a.Line != 0 {
-		params["line"] = a.Line
+	if a.Line != nil {
+		params["line"] = *a.Line
 	}
-	if a.Tag != "" {
-		params["tag"] = a.Tag
+	if a.Tag != nil {
+		params["tag"] = *a.Tag
 	}
 	if a.FailedOnly {
 		params["failedOnly"] = true

internal/mcp/studio_run_scope_test.go

@@ -26,3 +26,16 @@ func TestStudioRunFileSendsTheScope(t *testing.T) {
 		}
 	}
 }
+
+// An explicit line of 0 is passed on as sent, so Studio refuses it rather than running the whole file.
+func TestStudioRunFileForwardsAnExplicitZeroLine(t *testing.T) {
+	calls := fakeStudio(t, map[string]any{"run_file": map[string]any{"started": true}})
+	callTool(t, "studio_run_file", map[string]any{"path": "/w/a.probe", "line": 0})
+	if len(*calls) != 1 {
+		t.Fatalf("got %d calls, want 1", len(*calls))
+	}
+	want := map[string]any{"path": "/w/a.probe", "line": float64(0)}
+	if got := (*calls)[0]["params"]; !reflect.DeepEqual(got, want) {
+		t.Errorf("params = %v, want %v", got, want)
+	}
+}

internal/runner/filter_test.go

@@ -66,3 +66,26 @@ func TestTestResultDefName(t *testing.T) {
 		}
 	}
 }
+
+// selectedName is the composite-test filter: every test is selected unless TestNames is set, and then only the named ones.
+func TestSelectedNameFollowsTestNames(t *testing.T) {
+	all := &Runner{opts: RunOptions{}}
+	named := &Runner{opts: RunOptions{TestNames: []string{"checkout"}}}
+	none := &Runner{opts: RunOptions{TestNames: []string{}}}
+	cases := []struct {
+		label string
+		r     *Runner
+		name  string
+		want  bool
+	}{
+		{"no filter selects a composite test", all, "payments", true},
+		{"a named composite test is selected", named, "checkout", true},
+		{"an unnamed composite test is not", named, "payments", false},
+		{"an empty name list selects nothing", none, "checkout", false},
+	}
+	for _, c := range cases {
+		if got := c.r.selectedName(c.name); got != c.want {
+			t.Errorf("%s: selectedName(%q) = %v, want %v", c.label, c.name, got, c.want)
+		}
+	}
+}

studio/app.go

@@ -957,28 +957,27 @@ func (a *App) runScoped(requested string, scope RunScope) ([]RunResult, error) {
 	if conn == nil {
 		return nil, errNotConnected
 	}
-
-	// One run at a time; CancelRun stops it.
-	runCtx, cancel := context.WithCancel(a.ctx)
-	a.runMu.Lock()
-	if a.runCancel != nil {
-		a.runMu.Unlock()
-		cancel()
-		return nil, errRunInProgress
+	runCtx, cancel, err := a.claimRun()
+	if err != nil {
+		return nil, err
 	}
-	a.runCancel = cancel
-	a.runMu.Unlock()
-	defer func() {
-		a.runMu.Lock()
-		a.runCancel = nil
-		a.runMu.Unlock()
-		cancel()
-	}()
-
+	defer a.releaseRun(cancel)
 	sel, plan, err := a.resolveRunScope(path, scope)
 	if err != nil {
 		return nil, err
 	}
+	return a.executeRun(runCtx, path, sel, plan)
+}
+
+// executeRun runs the tests sel picks from path, whose scope is already resolved to plan. The caller holds the run slot
+// (see claimRun) and releases it. It emits the run events and records the results.
+func (a *App) executeRun(runCtx context.Context, path string, sel runSelection, plan RunPlan) ([]RunResult, error) {
+	a.mu.Lock()
+	conn := a.conn
+	a.mu.Unlock()
+	if conn == nil {
+		return nil, errNotConnected
+	}
 	a.track.begin(path, plan)
 	a.emit("run:started", path)
 	a.emit("run:plan", plan)

studio/automation.go

@@ -385,10 +385,10 @@ func (s *automationServer) call(method string, raw json.RawMessage) (any, error)
 		return b.Status(), nil
 	case "run_file":
 		var p struct {
-			Path       string `json:"path"`
-			Line       int    `json:"line"`
-			Tag        string `json:"tag"`
-			FailedOnly bool   `json:"failedOnly"`
+			Path       string  `json:"path"`
+			Line       *int    `json:"line"`
+			Tag        *string `json:"tag"`
+			FailedOnly bool    `json:"failedOnly"`
 		}
 		if err := param(raw, &p); err != nil || p.Path == "" {
 			return nil, fmt.Errorf("run_file needs {\"path\": ...}")
@@ -397,8 +397,21 @@ func (s *automationServer) call(method string, raw json.RawMessage) (any, error)
 		if err != nil {
 			return nil, err
 		}
-		// line, tag and failedOnly pick part of the file, as the Run control's scope does: at most one of them.
-		scope := RunScope{Line: p.Line, Tag: p.Tag, FailedOnly: p.FailedOnly}
+		// line, tag and failedOnly pick part of the file, as the Run control's scope does: at most one of them. A line or
+		// tag that is sent must name something: an explicit 0 or an empty tag is refused, not read as the whole file.
+		scope := RunScope{FailedOnly: p.FailedOnly}
+		if p.Line != nil {
+			if *p.Line < 1 {
+				return nil, fmt.Errorf("line must be 1 or more; leave line out to run the whole file")
+			}
+			scope.Line = *p.Line
+		}
+		if p.Tag != nil {
+			if tagName(*p.Tag) == "" {
+				return nil, fmt.Errorf("tag needs a name, such as smoke; leave tag out to run the whole file")
+			}
+			scope.Tag = *p.Tag
+		}
 		if err := b.runFileAsync(real, scope); err != nil {
 			return nil, err
 		}

studio/run_scope_test.go

@@ -248,6 +248,57 @@ func TestRunFileAsyncRefusesAFileOutsideTheRootsBeforeItsScope(t *testing.T) {
 	}
 }
 
+// A run that is already in progress is refused by the call itself, not by a background run that silently does nothing.
+func TestRunFileAsyncRefusesAStartWhileARunHoldsTheSlot(t *testing.T) {
+	path := writeScopeFile(t, scopeFixture)
+	dir, _ := filepath.EvalSymlinks(filepath.Dir(path))
+	a := &App{conn: &connection{}, humanWorkspace: dir}
+	a.runCancel = func() {}
+	if err := a.runFileAsync(path, RunScope{}); err != errRunInProgress {
+		t.Errorf("runFileAsync while a run holds the slot: err %v; want errRunInProgress", err)
+	}
+}
+
+// A test that failed and has since been renamed or removed is left out of the re-run; the tests the file still has run.
+func TestFailedOnlyLeavesOutATestThatIsGone(t *testing.T) {
+	path := writeScopeFile(t, "test \"login\"\n  wait 1 seconds\n")
+	a := &App{}
+	a.failures.record(path, []runner.TestResult{
+		{TestName: "login", Row: -1, Passed: false},
+		{TestName: "renamed away", Row: -1, Passed: false},
+	})
+	sel, plan, err := a.resolveRunScope(path, RunScope{FailedOnly: true})
+	if err != nil {
+		t.Fatalf("failed-only with one failed test gone: err %v; want the test that still exists", err)
+	}
+	if len(sel.names) != 1 || sel.names[0] != "login" || len(plan.Tests) != 1 {
+		t.Errorf("failed-only: names %v, plan %+v; want only login", sel.names, plan.Tests)
+	}
+}
+
+// An explicit line of 0 or an empty tag names nothing, so run_file refuses it rather than running the whole file.
+func TestRunFileRefusesAnExplicitEmptyScope(t *testing.T) {
+	s, fb, _ := startTest(t)
+	dir, _ := filepath.EvalSymlinks(t.TempDir())
+	fb.roots = []string{dir}
+	file := filepath.Join(dir, "x.probe")
+	if err := os.WriteFile(file, []byte("test \"x\"\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	for _, params := range []map[string]any{
+		{"path": file, "line": 0},
+		{"path": file, "tag": ""},
+		{"path": file, "tag": "  @  "},
+	} {
+		if _, out := rpc(t, s, s.token, "run_file", params, nil); out["error"] == nil {
+			t.Errorf("run_file %v: started, want a refusal", params)
+		}
+	}
+	if fb.scope != (RunScope{}) {
+		t.Errorf("a refused run_file still reached the run with scope %+v", fb.scope)
+	}
+}
+
 // run_file passes its optional line, tag and failedOnly fields on to the run as its scope.
 func TestRunFileSendsItsScope(t *testing.T) {
 	s, fb, _ := startTest(t)

studio/runtrack.go

@@ -1,6 +1,10 @@
 package main
 
-import "sync"
+import (
+	"context"
+	"errors"
+	"sync"
+)
 
 // maxTrackedSteps bounds the step history kept for a run (the newest are kept).
 const maxTrackedSteps = 2000
@@ -91,29 +95,61 @@ func (a *App) RunFileAsync(path string) error {
 	return a.runFileAsync(path, RunScope{})
 }
 
-// runFileAsync is RunFileAsync for a scope. The scope is resolved before the run starts in the background, so a scope
-// that cannot run is returned to the caller rather than lost.
+// runFileAsync is RunFileAsync for a scope. The run slot is taken and the scope resolved before this returns, so a run
+// that cannot start is returned to the caller rather than lost in the background; the background run uses that resolution.
 func (a *App) runFileAsync(path string, scope RunScope) error {
 	a.mu.Lock()
 	connected := a.conn != nil
 	a.mu.Unlock()
 	if !connected {
 		return errNotConnected
 	}
-	a.runMu.Lock()
-	busy := a.runCancel != nil
-	a.runMu.Unlock()
-	if busy {
-		return errRunInProgress
-	}
 	// Refuse a path outside the allowed folders now, not inside the goroutine, so the caller hears about it.
 	checked, err := probeFileWithinRoots(a.automationRoots(), path)
 	if err != nil {
 		return err
 	}
-	if _, _, err := a.resolveRunScope(checked, scope); err != nil {
+	runCtx, cancel, err := a.claimRun()
+	if err != nil {
+		return err
+	}
+	sel, plan, err := a.resolveRunScope(checked, scope)
+	if err != nil {
+		a.releaseRun(cancel)
 		return err
 	}
-	go func() { _, _ = a.runScoped(checked, scope) }()
+	go func() {
+		defer a.releaseRun(cancel)
+		// A connection that drops after the checks above is reported in the run state: the caller has already returned.
+		if _, err := a.executeRun(runCtx, checked, sel, plan); errors.Is(err, errNotConnected) {
+			a.track.finish(nil, err)
+		}
+	}()
 	return nil
 }
+
+// claimRun takes the run slot for a run that is about to start. It fails at once when another run holds the slot, so a
+// caller learns that before it returns. The caller gives the slot back with releaseRun.
+func (a *App) claimRun() (context.Context, context.CancelFunc, error) {
+	parent := a.ctx
+	if parent == nil {
+		parent = context.Background()
+	}
+	runCtx, cancel := context.WithCancel(parent)
+	a.runMu.Lock()
+	defer a.runMu.Unlock()
+	if a.runCancel != nil {
+		cancel()
+		return nil, nil, errRunInProgress
+	}
+	a.runCancel = cancel
+	return runCtx, cancel, nil
+}
+
+// releaseRun gives back the run slot that claimRun took and stops the run's context.
+func (a *App) releaseRun(cancel context.CancelFunc) {
+	a.runMu.Lock()
+	a.runCancel = nil
+	a.runMu.Unlock()
+	cancel()
+}