From a5ee8d83dd89248fdc4109d4141d019df53b39eb Mon Sep 17 00:00:00 2001 From: Patrick Bertsch Date: Wed, 4 Nov 2026 22:06:30 -0600 Subject: [PATCH] fix(studio): refuse an empty run scope, and resolve a run once before it starts A line of 0 or an empty tag is no longer read as the whole file: run_file and the MCP tool refuse it, and a line or tag that is left out is not sent. The run slot is now claimed before runFileAsync returns, so a second start is refused by the call instead of being dropped in the background, and the scope is resolved once. Tests: a failed test that has been renamed or removed is left out of a failed-only re-run; a composite test is selected by its name only when the run names it; an explicit line of 0 and an empty tag are refused; a start while a run holds the slot returns the refusal. --- CHANGELOG.md | 2 +- internal/mcp/studio.go | 18 +++++---- internal/mcp/studio_run_scope_test.go | 13 ++++++ internal/runner/filter_test.go | 23 +++++++++++ studio/app.go | 33 ++++++++------- studio/automation.go | 25 +++++++++--- studio/run_scope_test.go | 51 +++++++++++++++++++++++ studio/runtrack.go | 58 ++++++++++++++++++++++----- 8 files changed, 180 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ebcb117a..d09d9eb2 100644 --- a/CHANGELOG.md +++ b/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 `` 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. diff --git a/internal/mcp/studio.go b/internal/mcp/studio.go index a8f109d3..a17ff0fe 100644 --- a/internal/mcp/studio.go +++ b/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 diff --git a/internal/mcp/studio_run_scope_test.go b/internal/mcp/studio_run_scope_test.go index 77a0c5c3..9d9bb5e7 100644 --- a/internal/mcp/studio_run_scope_test.go +++ b/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) + } +} diff --git a/internal/runner/filter_test.go b/internal/runner/filter_test.go index 4d6c0732..aa7158bf 100644 --- a/internal/runner/filter_test.go +++ b/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) + } + } +} diff --git a/studio/app.go b/studio/app.go index 973e633d..9922d098 100644 --- a/studio/app.go +++ b/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) diff --git a/studio/automation.go b/studio/automation.go index 8042daee..b3bc817a 100644 --- a/studio/automation.go +++ b/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 } diff --git a/studio/run_scope_test.go b/studio/run_scope_test.go index ca83f238..b45baba4 100644 --- a/studio/run_scope_test.go +++ b/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) diff --git a/studio/runtrack.go b/studio/runtrack.go index 00f3aafb..58a790c7 100644 --- a/studio/runtrack.go +++ b/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,8 +95,8 @@ 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 @@ -100,20 +104,52 @@ func (a *App) runFileAsync(path string, scope RunScope) error { 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() +}