diff --git a/internal/cmd/execution_failure_test.go b/internal/cmd/execution_failure_test.go new file mode 100644 index 0000000..a4fd5ea --- /dev/null +++ b/internal/cmd/execution_failure_test.go @@ -0,0 +1,70 @@ +package cmd + +import ( + "strings" + "testing" + + "github.com/nottelabs/notte-cli/internal/api" +) + +func strPtr(s string) *string { return &s } + +// The API sends the failure twice: `exception` is a bare string rendered in the +// server's ErrorConfig mode (often the generic user-facing sentence), while +// `exception_detail` carries the concrete type and the per-audience messages. +// The CLI must report the structured one when it is there. +func TestExecutionFailureError_PrefersStructuredDetail(t *testing.T) { + detail := &api.SerializedError{ + ErrorType: "ActionExecutionError", + DevMessage: "Failed to execute action: evaluate_js on https://x. Reason: ReferenceError: foo is not defined", + UserMessage: "Sorry, this action cannot be executed at the moment.", + } + + err := executionFailureError(detail, strPtr("Sorry, this action cannot be executed at the moment."), "boom") + + got := err.Error() + if !strings.Contains(got, "ActionExecutionError") { + t.Fatalf("expected the concrete error type, got %q", got) + } + if !strings.Contains(got, "ReferenceError: foo is not defined") { + t.Fatalf("expected the actual reason, got %q", got) + } + if strings.Contains(got, "Sorry, this action cannot be executed") { + t.Fatalf("expected the generic user message to be dropped, got %q", got) + } +} + +func TestExecutionFailureError_FallsBackToUserMessage(t *testing.T) { + detail := &api.SerializedError{ErrorType: "BrowserError", UserMessage: "Something unexpected happened."} + + err := executionFailureError(detail, nil, "") + + if got := err.Error(); got != "BrowserError: Something unexpected happened." { + t.Fatalf("unexpected error: %q", got) + } +} + +// API builds that predate exception_detail send only the legacy string. +func TestExecutionFailureError_LegacyExceptionString(t *testing.T) { + err := executionFailureError(nil, strPtr("boom"), "click failed") + + if got := err.Error(); got != "boom: click failed" { + t.Fatalf("unexpected error: %q", got) + } +} + +func TestExecutionFailureError_MessageOnly(t *testing.T) { + err := executionFailureError(nil, nil, "click failed") + + if got := err.Error(); got != "action failed: click failed" { + t.Fatalf("unexpected error: %q", got) + } +} + +func TestExecutionFailureError_NothingToGoOn(t *testing.T) { + err := executionFailureError(nil, nil, "") + + if got := err.Error(); got != "action failed" { + t.Fatalf("unexpected error: %q", got) + } +} diff --git a/internal/cmd/output_helpers.go b/internal/cmd/output_helpers.go index b361ded..2e202d5 100644 --- a/internal/cmd/output_helpers.go +++ b/internal/cmd/output_helpers.go @@ -227,3 +227,41 @@ func printSessionStatus(resp *api.SessionResponse) error { return nil } + +// executionFailureError builds the error for a failed page action. +// +// The API serializes failures twice: `exception` is a bare string rendered in +// whatever ErrorConfig mode the server ran in (often the generic user-facing +// sentence), while `exception_detail` carries the concrete error type, the +// per-audience messages and the retry/notify flags. Prefer the structured +// field so the CLI reports what actually went wrong, and fall back to the +// legacy string for API builds that predate it. +func executionFailureError(detail *api.SerializedError, exception *string, message string) error { + if detail != nil { + reason := detail.DevMessage + if reason == "" { + reason = detail.UserMessage + } + if reason == "" { + reason = message + } + if reason == "" { + return fmt.Errorf("%s", detail.ErrorType) + } + if detail.ErrorType != "" { + return fmt.Errorf("%s: %s", detail.ErrorType, reason) + } + return fmt.Errorf("%s", reason) + } + + if exception != nil && *exception != "" { + if message != "" && *exception != message { + return fmt.Errorf("%s: %s", *exception, message) + } + return fmt.Errorf("%s", *exception) + } + if message != "" { + return fmt.Errorf("action failed: %s", message) + } + return fmt.Errorf("action failed") +} diff --git a/internal/cmd/page.go b/internal/cmd/page.go index f144e7d..9696656 100644 --- a/internal/cmd/page.go +++ b/internal/cmd/page.go @@ -55,19 +55,7 @@ func printExecuteResponse(resp *api.ApiExecutionResponse) error { } if !resp.Success { - // Build error message with available context - if resp.Exception != nil { - // Include exception and message if both present and different - if resp.Message != "" && *resp.Exception != resp.Message { - return fmt.Errorf("%s: %s", *resp.Exception, resp.Message) - } - return fmt.Errorf("%s", *resp.Exception) - } - // No exception - use message or generic fallback - if resp.Message != "" { - return fmt.Errorf("action failed: %s", resp.Message) - } - return fmt.Errorf("action failed") + return executionFailureError(resp.ExceptionDetail, resp.Exception, resp.Message) } // Print message @@ -718,10 +706,15 @@ var pageEvalJsCmd = &cobra.Command{ The JavaScript code is executed in the context of the page's main frame. +The evaluated value is printed alone on stdout (objects and arrays as JSON, a JS +null as "null"), so it pipes straight into jq or a shell variable; the status +line goes to stderr. Use -o json for the full execution result. + Examples: notte page eval-js "document.title" notte page eval-js "window.location.href" - notte page eval-js "document.querySelectorAll('a').length"`, + notte page eval-js "document.querySelectorAll('a').length" + notte page eval-js "JSON.stringify([...document.links].map(a => a.href))" | jq length`, Args: cobra.ExactArgs(1), RunE: runPageEvalJs, } @@ -765,21 +758,16 @@ func runPageEvalJs(cmd *cobra.Command, args []string) error { } if !result.Success { - if result.Exception != nil { - if result.Message != "" && *result.Exception != result.Message { - return fmt.Errorf("%s: %s", *result.Exception, result.Message) - } - return fmt.Errorf("%s", *result.Exception) - } - if result.Message != "" { - return fmt.Errorf("eval-js failed: %s", result.Message) - } - return fmt.Errorf("eval-js failed") + return executionFailureError(result.ExceptionDetail, result.Exception, result.Message) } - fmt.Println(result.Message) - if result.Data != nil && result.Data.Markdown != "" { - fmt.Printf("Result: %s\n", result.Data.Markdown) + // The evaluated value is the point of the command, so print it bare: it pipes + // into jq or a shell variable without post-processing, and a JS value that is + // legitimately empty still prints an (empty) line instead of vanishing behind + // a success banner. The status line goes to stderr so stdout stays the value. + _, _ = fmt.Fprintln(os.Stderr, result.Message) + if result.Data != nil { + fmt.Println(result.Data.Markdown) } return nil } diff --git a/tests/integration/page_evaljs_test.go b/tests/integration/page_evaljs_test.go new file mode 100644 index 0000000..4afb074 --- /dev/null +++ b/tests/integration/page_evaljs_test.go @@ -0,0 +1,158 @@ +//go:build integration + +package integration + +import ( + "bytes" + "context" + "encoding/json" + "os" + "os/exec" + "strings" + "testing" + "time" +) + +// runCLIText runs the CLI in its default (human) output mode. The shared +// helpers prepend `-o json`, which is exactly what these tests must not do: +// the point is what a shell sees on stdout without --output json. +func runCLIText(t *testing.T, args ...string) CLIResult { + t.Helper() + + ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) + defer cancel() + + cmd := exec.CommandContext(ctx, "go", append([]string{"run", "./cmd/notte", "--yes"}, args...)...) + cmd.Dir = getProjectRoot() + cmd.Env = append(os.Environ(), "NOTTE_API_KEY="+os.Getenv("NOTTE_API_KEY")) + if apiURL := os.Getenv("NOTTE_API_URL"); apiURL != "" { + cmd.Env = append(cmd.Env, "NOTTE_API_URL="+apiURL) + } + + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + exitCode := 0 + if err := cmd.Run(); err != nil { + if exitErr, ok := err.(*exec.ExitError); ok { + exitCode = exitErr.ExitCode() + } else { + exitCode = -1 + } + } + return CLIResult{Stdout: stdout.String(), Stderr: stderr.String(), ExitCode: exitCode} +} + +// evalJs runs `notte page eval-js` in text mode, the way a shell would. +func evalJs(t *testing.T, sessionID string, code string) CLIResult { + t.Helper() + return runCLIText(t, "page", "eval-js", code, "--session-id", sessionID) +} + +// The evaluated value is the point of the command: it must land on stdout with +// nothing else, so `title=$(notte page eval-js "document.title")` captures the +// title alone and a pipe into jq needs no post-processing. The status line goes +// to stderr. +func TestPageEvalJsPrintsOnlyTheValue(t *testing.T) { + sessionID := startTestSession(t) + defer cleanupSession(t, sessionID) + + requireSuccess(t, runCLIWithTimeout(t, 120*time.Second, "page", "goto", "https://example.com", "--session-id", sessionID)) + + result := evalJs(t, sessionID, "document.title") + requireSuccess(t, result) + + // what `title=$(...)` would capture + title := strings.TrimSpace(result.Stdout) + if title != "Example Domain" { + t.Fatalf("expected the title alone on stdout, got %q (stderr: %q)", result.Stdout, result.Stderr) + } + if strings.Contains(result.Stdout, "Result:") || strings.Contains(result.Stdout, "Successfully executed") { + t.Errorf("stdout must carry the value only, got %q", result.Stdout) + } +} + +// `notte page eval-js "JSON.stringify(...)" | jq length` — stdout has to be a +// standalone JSON document for the pipe to work. +func TestPageEvalJsStdoutIsPipeableJSON(t *testing.T) { + sessionID := startTestSession(t) + defer cleanupSession(t, sessionID) + + requireSuccess(t, runCLIWithTimeout(t, 120*time.Second, "page", "goto", "https://example.com", "--session-id", sessionID)) + + result := evalJs(t, sessionID, "JSON.stringify([...document.links].map(a => a.href))") + requireSuccess(t, result) + + var links []string + if err := json.Unmarshal([]byte(strings.TrimSpace(result.Stdout)), &links); err != nil { + t.Fatalf("stdout is not parseable JSON (%v): %q", err, result.Stdout) + } + if len(links) == 0 { + t.Errorf("expected at least one link on example.com, got %v", links) + } +} + +// A JS null is a successful evaluation and must arrive as the string "null", +// not as an empty stdout that a caller would read as "no result". +func TestPageEvalJsNullPrintsNull(t *testing.T) { + sessionID := startTestSession(t) + defer cleanupSession(t, sessionID) + + requireSuccess(t, runCLIWithTimeout(t, 120*time.Second, "page", "goto", "https://example.com", "--session-id", sessionID)) + + result := evalJs(t, sessionID, "null") + requireSuccess(t, result) + + if got := strings.TrimSpace(result.Stdout); got != "null" { + t.Fatalf("expected \"null\" on stdout, got %q", got) + } +} + +// -o json still emits the whole execution result, so machine consumers that +// want the envelope keep working. +func TestPageEvalJsJSONOutputKeepsTheEnvelope(t *testing.T) { + sessionID := startTestSession(t) + defer cleanupSession(t, sessionID) + + requireSuccess(t, runCLIWithTimeout(t, 120*time.Second, "page", "goto", "https://example.com", "--session-id", sessionID)) + + result := runCLIWithTimeout(t, 120*time.Second, "page", "eval-js", "document.title", "--session-id", sessionID) + requireSuccess(t, result) + + var envelope struct { + Success bool `json:"success"` + Data *struct { + Markdown string `json:"markdown"` + } `json:"data"` + } + if err := json.Unmarshal([]byte(result.Stdout), &envelope); err != nil { + t.Fatalf("expected a JSON envelope on stdout (%v): %q", err, result.Stdout) + } + if !envelope.Success { + t.Errorf("expected success=true, got %+v", envelope) + } + if envelope.Data == nil || envelope.Data.Markdown != "Example Domain" { + t.Errorf("expected the title in data.markdown, got %+v", envelope.Data) + } +} + +// A failing script must exit non-zero and name the actual JavaScript error, +// rather than the generic user-facing sentence the API serializes by default. +func TestPageEvalJsFailureReportsTheJavaScriptError(t *testing.T) { + sessionID := startTestSession(t) + defer cleanupSession(t, sessionID) + + requireSuccess(t, runCLIWithTimeout(t, 120*time.Second, "page", "goto", "https://example.com", "--session-id", sessionID)) + + result := evalJs(t, sessionID, "notAFunction()") + requireFailure(t, result) + + combined := result.Stdout + result.Stderr + if !containsString(combined, "notAFunction") { + t.Errorf("expected the JavaScript error in the output, got stdout=%q stderr=%q", result.Stdout, result.Stderr) + } + if containsString(combined, "Sorry, this action cannot be executed") { + t.Errorf("generic user message leaked instead of the real reason: %q", combined) + } +}