diff --git a/docs/gotchas/tooling.md b/docs/gotchas/tooling.md index 4bcd9ecc25c..6027299b44a 100644 --- a/docs/gotchas/tooling.md +++ b/docs/gotchas/tooling.md @@ -33,6 +33,10 @@ When a section grows to 10+ items, graduate it to its own doc. - **The watch service and the lock file do not share a version vocabulary.** A watch's `baselineVersion`, and the `currentVersion` it reports back, are the *asset-delivery content hash* for the place — that's what the service's Roblox driver reads, and it compares them by string equality. `deploy.nevermore.lock.json` holds the *Open Cloud place version* (an integer). They are never equal, so handing the lock's number over as a baseline reads as drift on the very first poll and dispatches a rebuild of the build that just shipped. The CLI therefore sends no baseline at all — the service's first poll adopts what it sees, which is what a baseline was for — and treats every version the service reports as an opaque "something moved" token, asking Open Cloud what the place is actually at before deciding to rebuild. Anything comparing a service version against a lock version is wrong even when the types line up. +- **Open Cloud truncates long engine logs, so anything that must be read back exactly belongs in the script's return value.** A script printing 20,001 lines came back with 7,627 of them — the last contiguous block, with the head dropped — and the size of the window varies run to run (6,715 and 3,236 lines on other runs), so there is no line count to stay under. A Luau execution task's return value is not subject to this: it arrives on the task as `output.results`, a flat array of the returned values (`return t, "s", 42` → three entries), and Roblox serializes them itself, so a returned Lua table is real nested JSON. Do **not** `HttpService:JSONEncode` the result — that lands as a double-encoded string. + +- **An oversize return value annihilates the task rather than truncating the value.** A ~2.1MB return value arrives complete and intact; a ~4.2MB one fails the whole task — state `FAILED`, no `output`, and no error message saying why. So code reading a return value has to treat "the task reported no output" as *unknown* rather than empty, and fall back to the logs; `getTaskReturnValues` in `open-cloud-client.ts` draws exactly that line (`undefined` = nothing came back, `[]` = the script returned nothing). + - **`--script-text` loses everything after the first line when invoked through `npx` on Windows**: the `npx.cmd` shim truncates a multi-line argument, so `nevermore test --cloud --script-text '\n'` silently runs only line 1 (and prints `(no output)` when line 1 produced none). Either write the script as a single line with `;` separators, or bypass the shim: `node tools/nevermore-cli/dist/nevermore.js test --cloud --script-text '...'`, which passes newlines through intact. ## Claude Code hooks diff --git a/tools/nevermore-cli/src/utils/job-context/batch-script-job-context.ts b/tools/nevermore-cli/src/utils/job-context/batch-script-job-context.ts index c4bc5a56303..bf0fe6e2c4b 100644 --- a/tools/nevermore-cli/src/utils/job-context/batch-script-job-context.ts +++ b/tools/nevermore-cli/src/utils/job-context/batch-script-job-context.ts @@ -135,6 +135,9 @@ export class BatchScriptJobContext implements JobContext { success: result.success, durationMs: result.durationMs, errorMessage: result.error, + // returnValues stays absent: one execution covers every package, so the + // single return value it produces has to be split per package before any + // of it can surface here. }; } diff --git a/tools/nevermore-cli/src/utils/job-context/cloud-job-context.test.ts b/tools/nevermore-cli/src/utils/job-context/cloud-job-context.test.ts new file mode 100644 index 00000000000..70a3e1d6635 --- /dev/null +++ b/tools/nevermore-cli/src/utils/job-context/cloud-job-context.test.ts @@ -0,0 +1,112 @@ +/** + * Unit tests for CloudJobContext.runScriptAsync — validates what a finished + * Open Cloud task reports back as a ScriptRunResult, in particular that a task + * which produced no output stays distinguishable from one that returned nothing. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { type Reporter } from '@quenty/cli-output-helpers/reporting'; +import { CloudJobContext } from './cloud-job-context.js'; +import { + type LuauTask, + type OpenCloudClient, +} from '../open-cloud/open-cloud-client.js'; +import { type Deployment } from './job-context.js'; + +function createReporter(): Reporter { + return { + onPackagePhaseChange: vi.fn(), + onPackageProgressUpdate: vi.fn(), + onPackageStart: vi.fn(), + onPackageResult: vi.fn(), + } as unknown as Reporter; +} + +/** + * A client whose task completes as `completedTask`. The real deployment handle + * is private to the context, so the test passes the three fields runScriptAsync + * reads off it. + */ +function createContext(completedTask: Partial) { + const task = { + path: 'universes/1/places/2/versions/3/luau-execution-session-tasks/4', + createTime: '2026-01-01T00:00:00Z', + updateTime: '2026-01-01T00:01:00Z', + user: 'users/1', + state: 'COMPLETE', + script: 'return 1', + ...completedTask, + } as LuauTask; + + const client = { + createExecutionTaskAsync: vi.fn(async () => task), + pollTaskCompletionAsync: vi.fn(async () => task), + } as unknown as OpenCloudClient; + + const context = new CloudJobContext(createReporter(), client); + const deployment = { + universeId: 1, + placeId: 2, + version: 3, + } as unknown as Deployment; + + return { context, deployment }; +} + +describe('CloudJobContext.runScriptAsync', () => { + afterEach(() => { + vi.useRealTimers(); + }); + + it('reports the values the script returned', async () => { + // Fake timers keep the client-side timeout race from leaving a live timer + // behind; nothing in the run itself waits on one. + vi.useFakeTimers(); + const { context, deployment } = createContext({ + state: 'COMPLETE', + output: { results: [{ slug: 'maid', counts: { passed: 1014 } }] }, + }); + + const result = await context.runScriptAsync(deployment, { + scriptContent: 'return results', + packageName: 'maid', + }); + + expect(result.success).toBe(true); + expect(result.returnValues).toEqual([ + { slug: 'maid', counts: { passed: 1014 } }, + ]); + }); + + it('reports an empty result when the task returned nothing', async () => { + vi.useFakeTimers(); + const { context, deployment } = createContext({ + state: 'COMPLETE', + output: {}, + }); + + const result = await context.runScriptAsync(deployment, { + scriptContent: 'print("hi")', + packageName: 'maid', + }); + + expect(result.returnValues).toEqual([]); + }); + + it('leaves returnValues absent when a failed task carried no output', async () => { + vi.useFakeTimers(); + const { context, deployment } = createContext({ + state: 'FAILED', + output: undefined, + }); + + const result = await context.runScriptAsync(deployment, { + scriptContent: 'return huge', + packageName: 'maid', + }); + + expect(result.success).toBe(false); + expect(result.taskState).toBe('FAILED'); + expect(result.returnValues).toBeUndefined(); + }); +}); diff --git a/tools/nevermore-cli/src/utils/job-context/cloud-job-context.ts b/tools/nevermore-cli/src/utils/job-context/cloud-job-context.ts index fe5455e60c2..2375bee5e5a 100644 --- a/tools/nevermore-cli/src/utils/job-context/cloud-job-context.ts +++ b/tools/nevermore-cli/src/utils/job-context/cloud-job-context.ts @@ -1,5 +1,6 @@ import { type Reporter } from '@quenty/cli-output-helpers/reporting'; import { + getTaskReturnValues, type LuauTask, type OpenCloudClient, } from '../open-cloud/open-cloud-client.js'; @@ -134,6 +135,7 @@ export class CloudJobContext extends BaseJobContext { success: completedTask.state === 'COMPLETE', taskState: completedTask.state, errorMessage, + returnValues: getTaskReturnValues(completedTask), }; } diff --git a/tools/nevermore-cli/src/utils/job-context/job-context.ts b/tools/nevermore-cli/src/utils/job-context/job-context.ts index c5b95969442..7d9009807a8 100644 --- a/tools/nevermore-cli/src/utils/job-context/job-context.ts +++ b/tools/nevermore-cli/src/utils/job-context/job-context.ts @@ -28,6 +28,27 @@ export interface ScriptRunResult { taskState?: string; /** Error message from the execution backend, if any. */ errorMessage?: string; + /** + * Everything the executed script returned, in order — the structured channel + * out of a run, as opposed to its printed output. Engine logs are truncated + * by Open Cloud on long runs, so anything a caller must read back exactly + * belongs here rather than in the log text. + * + * `undefined` means the transport never delivered a return channel: a cloud + * task that ended without an `output` (a FAILED task carries none, and an + * oversize return value fails the task rather than truncating the value), a + * bridge run that timed out or disconnected, or a context that does not + * carry return values at all. That is deliberately distinct from `[]`, which + * means the script ran and returned nothing — a caller that needs the value + * can fall back to parsing logs in the first case but not the second. + * + * Values are JSON-shaped, but the two transports spell exotic Luau types + * differently: Open Cloud auto-serializes them, while the Studio bridge + * marshals them into `{ type, value }` wrappers (`SerializedReturnValue`). + * Plain tables of strings, numbers and booleans come back identically on + * both, so structured results should stay inside that subset. + */ + returnValues?: unknown[]; } /** diff --git a/tools/nevermore-cli/src/utils/job-context/local-job-context.test.ts b/tools/nevermore-cli/src/utils/job-context/local-job-context.test.ts new file mode 100644 index 00000000000..2f5883f0ed5 --- /dev/null +++ b/tools/nevermore-cli/src/utils/job-context/local-job-context.test.ts @@ -0,0 +1,89 @@ +/** + * Unit tests for LocalJobContext.runScriptAsync — validates that a bridge run + * hands its script return values on as a ScriptRunResult, and that a run which + * never completed reports none rather than an empty result. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { type Reporter } from '@quenty/cli-output-helpers/reporting'; +import { LocalJobContext } from './local-job-context.js'; +import { type Deployment } from './job-context.js'; + +function createReporter(): Reporter { + return { + onPackagePhaseChange: vi.fn(), + onPackageProgressUpdate: vi.fn(), + onPackageStart: vi.fn(), + onPackageResult: vi.fn(), + } as unknown as Reporter; +} + +/** + * The deployment handle is private to the context, so the test stands in a + * bridge with the one method runScriptAsync calls on it. + */ +function createDeployment( + executeAsync: () => Promise<{ + success: boolean; + logs: string; + returnValues?: unknown[]; + }> +): Deployment { + return { + bridge: { executeAsync }, + cachedLogs: '', + } as unknown as Deployment; +} + +describe('LocalJobContext.runScriptAsync', () => { + it('reports the values the script returned', async () => { + const context = new LocalJobContext(createReporter()); + const deployment = createDeployment(async () => ({ + success: true, + logs: 'ran', + returnValues: [{ counts: { passed: 7 } }], + })); + + const result = await context.runScriptAsync(deployment, { + scriptContent: 'return results', + packageName: 'maid', + }); + + expect(result.success).toBe(true); + expect(result.returnValues).toEqual([{ counts: { passed: 7 } }]); + expect(await context.getLogsAsync(deployment)).toBe('ran'); + }); + + it('leaves returnValues absent when the bridge reported none', async () => { + const context = new LocalJobContext(createReporter()); + const deployment = createDeployment(async () => ({ + success: false, + logs: '[StudioBridge] Timed out after 200ms', + })); + + const result = await context.runScriptAsync(deployment, { + scriptContent: 'while true do end', + packageName: 'maid', + }); + + expect(result.returnValues).toBeUndefined(); + }); + + it('leaves returnValues absent when the bridge throws', async () => { + const context = new LocalJobContext(createReporter()); + const deployment = createDeployment(async () => { + throw new Error('no connected client'); + }); + + const result = await context.runScriptAsync(deployment, { + scriptContent: 'return results', + packageName: 'maid', + }); + + expect(result.success).toBe(false); + expect(result.returnValues).toBeUndefined(); + expect(await context.getLogsAsync(deployment)).toContain( + 'no connected client' + ); + }); +}); diff --git a/tools/nevermore-cli/src/utils/job-context/local-job-context.ts b/tools/nevermore-cli/src/utils/job-context/local-job-context.ts index 3c113823826..bbc268dad9b 100644 --- a/tools/nevermore-cli/src/utils/job-context/local-job-context.ts +++ b/tools/nevermore-cli/src/utils/job-context/local-job-context.ts @@ -69,7 +69,7 @@ export class LocalJobContext extends BaseJobContext { timeoutMs, }); localDeployment.cachedLogs = result.logs; - return { success: result.success }; + return { success: result.success, returnValues: result.returnValues }; } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); diff --git a/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.test.ts b/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.test.ts index e5148aa1a7c..ed1c5687aa3 100644 --- a/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.test.ts +++ b/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.test.ts @@ -1,6 +1,10 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import * as fs from 'fs/promises'; -import { OpenCloudClient } from './open-cloud-client.js'; +import { + OpenCloudClient, + getTaskReturnValues, + type LuauTask, +} from './open-cloud-client.js'; import type { RateLimiter } from './rate-limiter.js'; vi.mock('@quenty/cli-output-helpers', () => ({ @@ -367,3 +371,47 @@ describe('OpenCloudClient.resolveLatestPlaceVersionAsync', () => { ).rejects.toThrowError(/unparseable version path/); }); }); + +describe('getTaskReturnValues', () => { + function makeTask(overrides: Partial): LuauTask { + return { + path: 'universes/1/places/2/versions/3/luau-execution-session-tasks/4', + createTime: '2026-01-01T00:00:00Z', + updateTime: '2026-01-01T00:01:00Z', + user: 'users/1', + state: 'COMPLETE', + script: 'return 1', + ...overrides, + }; + } + + it('returns the values natively typed, one entry per returned value', () => { + // Roblox serializes the return value itself, so a returned table arrives as + // real nested JSON — there is no JSON string to parse a second time. + const task = makeTask({ + output: { + results: [{ slug: 'maid', counts: { passed: 1014 } }, 'str', 42, true], + }, + }); + + expect(getTaskReturnValues(task)).toEqual([ + { slug: 'maid', counts: { passed: 1014 } }, + 'str', + 42, + true, + ]); + }); + + it('reports an empty result when the task returned nothing', () => { + expect(getTaskReturnValues(makeTask({ output: {} }))).toEqual([]); + }); + + it('reports undefined when a failed task carried no output at all', () => { + // An oversize return value fails the task with no output and no error + // message, so "nothing came back to read" has to stay distinguishable from + // "the script returned nothing" — only the former can fall back to logs. + const task = makeTask({ state: 'FAILED', output: undefined }); + + expect(getTaskReturnValues(task)).toBeUndefined(); + }); +}); diff --git a/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.ts b/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.ts index 3deaf3169e1..f04b3ce7299 100644 --- a/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.ts +++ b/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.ts @@ -22,12 +22,38 @@ export interface LuauTask { | 'FAILED'; script: string; timeout?: string; - /** Script return value (populated on COMPLETE). */ - output?: { results?: Array<{ value?: string }> }; + /** + * What the script returned, populated on COMPLETE. `results` is a flat array + * of the returned values — `return t, "s", 42` arrives as three entries — + * natively typed: Roblox serializes them itself, so a returned Lua table is + * real nested JSON here, not a JSON string. (Which is also why a script must + * not JSONEncode its result: that lands as a double-encoded string.) + * + * Absent whenever the task produced no result. A FAILED task carries no + * `output` at all, with no error message explaining why — and an oversize + * return value (~4MB observed; ~2MB still arrives complete) fails the task + * exactly that way rather than truncating the value. + */ + output?: { results?: unknown[] }; /** Error details (populated on FAILED). */ error?: { code?: string; message?: string }; } +/** + * The values a finished task's script returned, or `undefined` when the task + * reported no result at all. + * + * The distinction matters: `undefined` means nothing came back to read, so a + * caller can fall back to the task's logs, while `[]` means the task did report + * a result and the script returned nothing — no fallback will find more. + */ +export function getTaskReturnValues(task: LuauTask): unknown[] | undefined { + if (!task.output) { + return undefined; + } + return task.output.results ?? []; +} + export interface OpenCloudClientOptions { apiKey: string | (() => Promise); rateLimiter: RateLimiter; diff --git a/tools/studio-bridge/src/bridge/bridge-session.test.ts b/tools/studio-bridge/src/bridge/bridge-session.test.ts index b93f39ec6ca..f7afd7bc447 100644 --- a/tools/studio-bridge/src/bridge/bridge-session.test.ts +++ b/tools/studio-bridge/src/bridge/bridge-session.test.ts @@ -174,6 +174,51 @@ describe('BridgeSession', () => { expect(result.success).toBe(false); expect(result.error).toBe('boom'); }); + + it('surfaces the values the script returned', async () => { + const handle = new MockTransportHandle(); + handle.sendActionAsync.mockResolvedValueOnce({ + type: 'scriptComplete', + sessionId: 'session-1', + payload: { + success: true, + returnValues: [{ counts: { passed: 3 } }, 42], + }, + }); + + const session = new BridgeSession(createSessionInfo(), handle); + const result = await session.execAsync('return results, 42'); + + expect(result.returnValues).toEqual([{ counts: { passed: 3 } }, 42]); + }); + + it('leaves returnValues undefined when the plugin reported none', async () => { + const handle = new MockTransportHandle(); + handle.sendActionAsync.mockResolvedValueOnce({ + type: 'scriptComplete', + sessionId: 'session-1', + payload: { success: true }, + }); + + const session = new BridgeSession(createSessionInfo(), handle); + const result = await session.execAsync('print("hello")'); + + expect(result.returnValues).toBeUndefined(); + }); + + it('leaves returnValues undefined on an error response', async () => { + const handle = new MockTransportHandle(); + handle.sendActionAsync.mockResolvedValueOnce({ + type: 'error', + sessionId: 'session-1', + payload: { code: 'SCRIPT_RUNTIME_ERROR', message: 'boom' }, + }); + + const session = new BridgeSession(createSessionInfo(), handle); + const result = await session.execAsync('error("boom")'); + + expect(result.returnValues).toBeUndefined(); + }); }); describe('queryStateAsync', () => { diff --git a/tools/studio-bridge/src/bridge/bridge-session.ts b/tools/studio-bridge/src/bridge/bridge-session.ts index f26a3cbdffe..c488ed51e3d 100644 --- a/tools/studio-bridge/src/bridge/bridge-session.ts +++ b/tools/studio-bridge/src/bridge/bridge-session.ts @@ -118,6 +118,7 @@ export class BridgeSession extends EventEmitter { success: result.payload.success, output, error: result.payload.error, + returnValues: result.payload.returnValues, }; } diff --git a/tools/studio-bridge/src/bridge/types.ts b/tools/studio-bridge/src/bridge/types.ts index 200ccfc6cd5..6480f29b905 100644 --- a/tools/studio-bridge/src/bridge/types.ts +++ b/tools/studio-bridge/src/bridge/types.ts @@ -11,10 +11,17 @@ import type { Capability, DataModelInstance, OutputLevel, + SerializedReturnValue, } from '../server/web-socket-protocol.js'; // Re-export protocol types used in the public API -export type { StudioState, Capability, DataModelInstance, OutputLevel }; +export type { + StudioState, + Capability, + DataModelInstance, + OutputLevel, + SerializedReturnValue, +}; export type SessionContext = 'edit' | 'client' | 'server'; export type SessionOrigin = 'user' | 'managed'; @@ -47,6 +54,12 @@ export interface ExecResult { success: boolean; output: Array<{ level: OutputLevel; body: string }>; error?: string; + /** + * Everything the script returned, in order. Absent when the plugin reported + * no return channel at all (older plugin build, or a run that never reached + * a return) — distinct from `[]`, which means it returned nothing. + */ + returnValues?: SerializedReturnValue[]; } export interface StateResult { diff --git a/tools/studio-bridge/src/commands/console/exec/execute.luau b/tools/studio-bridge/src/commands/console/exec/execute.luau index 734a588c2b9..f023dbaae73 100644 --- a/tools/studio-bridge/src/commands/console/exec/execute.luau +++ b/tools/studio-bridge/src/commands/console/exec/execute.luau @@ -10,6 +10,12 @@ - Distinct error codes: SCRIPT_LOAD_ERROR, SCRIPT_RUNTIME_ERROR. - Sequential queueing: concurrent execute requests are processed one at a time in FIFO order. + - Return values: everything the script returns is marshalled onto + `payload.returnValues`, so a caller can read a result as a value + instead of scraping it back out of printed output. + - Failure isolation: a request that throws while marshalling or + reporting its result is answered with an error rather than left to + latch the queue for the rest of the session. This module has no Roblox dependencies and is testable under Lune. ]] @@ -24,6 +30,149 @@ local _queue: { { payload: { [string]: any }, requestId: string?, sessionId: str {} local _processing = false +-- --------------------------------------------------------------------------- +-- Return value serialization +-- --------------------------------------------------------------------------- + +-- Produces the SerializedReturnValue shapes declared in +-- web-socket-protocol.ts. Action modules are loadstring'd standalone inside +-- the plugin and cannot require anything, so this cannot be shared with the +-- equivalent marshaller in query-data-model.luau. + +local MAX_DEPTH = 64 + +-- The depth cap alone does not bound the work. `seen` is cleared as the walk +-- leaves a table -- correct, since a value reachable by two separate paths is +-- not a cycle -- which means a shared reference is expanded once per edge. +-- `for i = 1, 30 do x = { a = x, b = x } end` is only 30 levels deep but 2^30 +-- nodes, so a total node budget is what actually keeps Studio alive. +local MAX_NODES = 100_000 + +type MarshalState = { seen: { [any]: boolean }, budget: number } + +local function unsupported(typeName: string, text: string): { [string]: any } + return { type = "Unsupported", typeName = typeName, toString = text } +end + +local serializeValue: (value: any, depth: number, state: MarshalState) -> any + +-- An array-like table becomes a JSON array, anything else a JSON object with +-- string keys. Mixed tables take the object branch on purpose: JSONEncode +-- rejects them, and a rejected encode drops the whole scriptComplete message. +-- +-- Traversal is raw (`next`, `rawlen`, `rawget`) so `__iter`, `__len` and +-- `__index` cannot make a returned table report contents it does not have -- +-- and cannot throw from inside the walk. A metatable is not part of the value, +-- so what gets marshalled is the table as it actually is. +local function serializeTable(value: { [any]: any }, depth: number, state: MarshalState): any + local count = 0 + for _ in next, value do + count += 1 + end + + if count == rawlen(value) then + local array = {} + for index = 1, count do + array[index] = serializeValue(rawget(value :: any, index), depth + 1, state) + end + return array + end + + local map: { [string]: any } = {} + for key, item in next, value do + local stringKey = if type(key) == "string" then key else tostring(key) + if map[stringKey] ~= nil then + -- Two distinct Luau keys collapsed onto one JSON key (`[1]` and + -- `"1"`). Marking it keeps the loss visible rather than quietly + -- serving whichever one the traversal happened to reach last. + map[stringKey] = unsupported("collision", ``) + else + map[stringKey] = serializeValue(item, depth + 1, state) + end + end + return map +end + +function serializeValue(value: any, depth: number, state: MarshalState): any + local valueType = typeof(value) + + state.budget -= 1 + if state.budget < 0 then + return unsupported(valueType, "") + end + + if valueType == "nil" then + -- A JSON array cannot hold a hole, and dropping the entry would shift + -- every later value into the wrong position. + return { type = "Nil" } + elseif valueType == "string" or valueType == "boolean" then + return value + elseif valueType == "number" then + -- inf and nan have no JSON spelling, and encoding one produces a + -- document the server cannot decode -- losing the entire message. + if value ~= value or value == math.huge or value == -math.huge then + return unsupported("number", tostring(value)) + end + return value + elseif valueType == "table" then + if depth > MAX_DEPTH then + return unsupported("table", "") + end + if state.seen[value] then + return unsupported("table", "") + end + state.seen[value] = true + local serialized = serializeTable(value, depth, state) + state.seen[value] = nil + return serialized + elseif valueType == "Vector3" then + return { type = "Vector3", value = { value.X, value.Y, value.Z } } + elseif valueType == "Vector2" then + return { type = "Vector2", value = { value.X, value.Y } } + elseif valueType == "CFrame" then + return { type = "CFrame", value = { value:GetComponents() } } + elseif valueType == "Color3" then + return { type = "Color3", value = { value.R, value.G, value.B } } + elseif valueType == "UDim2" then + return { type = "UDim2", value = { value.X.Scale, value.X.Offset, value.Y.Scale, value.Y.Offset } } + elseif valueType == "UDim" then + return { type = "UDim", value = { value.Scale, value.Offset } } + elseif valueType == "BrickColor" then + return { type = "BrickColor", name = value.Name, value = value.Number } + elseif valueType == "EnumItem" then + return { type = "EnumItem", enum = tostring(value.EnumType), name = value.Name, value = value.Value } + elseif valueType == "Instance" then + return { type = "Instance", className = value.ClassName, path = value:GetFullName() } + else + return unsupported(valueType, tostring(value)) + end +end + +-- Marshal the values a script returned, in order. The depth cap, cycle set and +-- node budget are shared across all of them, so one run cannot be talked into +-- unbounded work by spreading it over several return values. +local function serializeReturnValues(packed: { n: number, [number]: any }): { any } + local state: MarshalState = { seen = {}, budget = MAX_NODES } + local returnValues = {} + for index = 2, packed.n do + returnValues[index - 1] = serializeValue(packed[index], 1, state) + end + return returnValues +end + +-- Marshalling walks values the script chose, so a hostile metatable can still +-- throw from somewhere the raw traversal cannot avoid -- a table key whose +-- `__tostring` errors, say. Degrading to a marker keeps one bad value from +-- failing the run, and keeps it from throwing out of _handleExecute, which in +-- queue mode would latch _processing and strand every later request. +local function trySerializeReturnValues(packed: { n: number, [number]: any }): { any } + local ok, returnValues = pcall(serializeReturnValues, packed) + if ok then + return returnValues + end + return { unsupported("unknown", ``) } +end + -- --------------------------------------------------------------------------- -- Core execution logic -- --------------------------------------------------------------------------- @@ -108,17 +257,27 @@ function ExecuteAction._handleExecute( originalWarn(...) end - local success, runtimeError = pcall(fn) + local returned = table.pack(pcall(fn)) + local success = returned[1] -- Restore originals env.print = originalPrint env.warn = originalWarn if not success then - return sendResult({ success = false, error = tostring(runtimeError), code = "SCRIPT_RUNTIME_ERROR", output = captured }) + return sendResult({ + success = false, + error = tostring(returned[2]), + code = "SCRIPT_RUNTIME_ERROR", + output = captured, + }) end - return sendResult({ success = true, output = captured }) + return sendResult({ + success = true, + output = captured, + returnValues = trySerializeReturnValues(returned), + }) end -- --------------------------------------------------------------------------- @@ -133,7 +292,36 @@ local function _processQueue() while #_queue > 0 do local item = table.remove(_queue, 1) - ExecuteAction._handleExecute(item.payload, item.requestId, item.sessionId, item.sendMessage) + -- A throw here must not escape the loop. _processing would stay latched + -- for the rest of the Studio session, every later execute request would + -- pile up in _queue with nothing left to drain it, and the plugin never + -- re-registers this action on its own (syncActions sees an unchanged + -- hash), so only restarting Studio would recover. The most likely + -- thrower is sendMessage itself: it JSONEncodes a payload built partly + -- out of values the script chose. + local ok, err = + pcall(ExecuteAction._handleExecute, item.payload, item.requestId, item.sessionId, item.sendMessage) + + if not ok and item.sendMessage then + -- Tell the requester, or the CLI waits out its whole timeout with no + -- idea the request was dropped. Small fixed payload, so the encode + -- that just failed cannot fail again for the same reason -- but it + -- is still guarded, since it is the last thing standing between a + -- bad response and the latch above. + local response: { [string]: any } = { + type = "scriptComplete", + sessionId = item.sessionId, + payload = { + success = false, + error = `Failed to report script result: {tostring(err)}`, + code = "INTERNAL_ERROR", + }, + } + if item.requestId ~= nil and item.requestId ~= "" then + response.requestId = item.requestId + end + pcall(item.sendMessage, response) + end end _processing = false diff --git a/tools/studio-bridge/src/index.ts b/tools/studio-bridge/src/index.ts index dcde2a4af88..7408adf1852 100644 --- a/tools/studio-bridge/src/index.ts +++ b/tools/studio-bridge/src/index.ts @@ -55,6 +55,7 @@ export type { DataModelInstance, ErrorCode, SerializedValue, + SerializedReturnValue, } from './server/web-socket-protocol.js'; // Lower-level exports for advanced usage / testing diff --git a/tools/studio-bridge/src/server/studio-bridge-server.test.ts b/tools/studio-bridge/src/server/studio-bridge-server.test.ts index 61064734284..f4e6d16c57f 100644 --- a/tools/studio-bridge/src/server/studio-bridge-server.test.ts +++ b/tools/studio-bridge/src/server/studio-bridge-server.test.ts @@ -285,6 +285,55 @@ describe('StudioBridgeServer', () => { expect(result.logs).toContain('Script threw: boom'); }); + it('carries the script return values off the scriptComplete', async () => { + const ready = await createReadyServer(); + server = ready.server; + client = ready.client; + + const resultPromise = server.executeAsync({ + scriptContent: 'return { counts = { passed = 7 } }', + }); + + await new Promise((resolve) => { + client!.on('message', (raw) => { + const data = JSON.parse( + typeof raw === 'string' ? raw : raw.toString('utf-8') + ); + if (data.type === 'execute') resolve(); + }); + }); + + client!.send( + JSON.stringify({ + type: 'scriptComplete', + sessionId: ready.sessionId, + payload: { + success: true, + returnValues: [{ counts: { passed: 7 } }], + }, + }) + ); + + const result = await resultPromise; + expect(result.returnValues).toEqual([{ counts: { passed: 7 } }]); + }); + + it('leaves returnValues undefined when the run times out', async () => { + // Nothing reported a return channel, so the value is unknown rather than + // empty — a caller can still fall back to whatever output arrived. + const ready = await createReadyServer(); + server = ready.server; + client = ready.client; + + const result = await server.executeAsync({ + scriptContent: 'while true do end', + timeoutMs: 200, + }); + + expect(result.success).toBe(false); + expect(result.returnValues).toBeUndefined(); + }); + it('returns failure when client disconnects during execution', async () => { const ready = await createReadyServer(); server = ready.server; diff --git a/tools/studio-bridge/src/server/studio-bridge-server.ts b/tools/studio-bridge/src/server/studio-bridge-server.ts index 814b37a6e60..1e1824cec50 100644 --- a/tools/studio-bridge/src/server/studio-bridge-server.ts +++ b/tools/studio-bridge/src/server/studio-bridge-server.ts @@ -26,6 +26,7 @@ import { type Capability, type PluginMessage, type ServerMessage, + type SerializedReturnValue, encodeMessage, decodePluginMessage, } from './web-socket-protocol.js'; @@ -106,6 +107,14 @@ export interface ExecuteOptions { export interface StudioBridgeResult { success: boolean; logs: string; + /** + * Everything the script returned, in order, marshalled by the plugin. + * Absent whenever no `scriptComplete` carrying a return channel arrived — + * a timeout, a disconnect, a plugin error, or an older plugin build — which + * is what separates "we never learned what it returned" from `[]`, "it + * returned nothing". + */ + returnValues?: SerializedReturnValue[]; } type BridgeState = @@ -797,6 +806,7 @@ export class StudioBridgeServer { finish({ success: msg.payload.success, logs: logLines.join('\n'), + returnValues: msg.payload.returnValues, }); break; } diff --git a/tools/studio-bridge/src/server/web-socket-protocol.test.ts b/tools/studio-bridge/src/server/web-socket-protocol.test.ts index 853cf4e8016..21bda5b0ebe 100644 --- a/tools/studio-bridge/src/server/web-socket-protocol.test.ts +++ b/tools/studio-bridge/src/server/web-socket-protocol.test.ts @@ -47,6 +47,70 @@ describe('decodePluginMessage', () => { }); expect(msg).not.toHaveProperty('requestId'); }); + + it('carries returnValues through unchanged, nesting included', () => { + const msg = roundTripPlugin({ + type: 'scriptComplete', + sessionId: 'sess-1', + payload: { + success: true, + returnValues: [ + { slug: 'maid', counts: { passed: 1014, failed: 0 } }, + 'str', + 42, + true, + { type: 'Vector3', value: [1, 2, 3] }, + [1, 2], + ], + }, + }); + expect(msg?.type).toBe('scriptComplete'); + expect( + (msg as { payload: { returnValues?: unknown[] } }).payload.returnValues + ).toEqual([ + { slug: 'maid', counts: { passed: 1014, failed: 0 } }, + 'str', + 42, + true, + { type: 'Vector3', value: [1, 2, 3] }, + [1, 2], + ]); + }); + + it('leaves returnValues undefined when the plugin sent none', () => { + // Distinguishable from an empty array: nothing reported a return channel, + // so a caller can still fall back to the run's printed output. + const msg = roundTripPlugin({ + type: 'scriptComplete', + sessionId: 'sess-1', + payload: { success: true }, + }); + expect( + (msg as { payload: { returnValues?: unknown[] } }).payload.returnValues + ).toBeUndefined(); + }); + + it('preserves an empty returnValues array', () => { + const msg = roundTripPlugin({ + type: 'scriptComplete', + sessionId: 'sess-1', + payload: { success: true, returnValues: [] }, + }); + expect( + (msg as { payload: { returnValues?: unknown[] } }).payload.returnValues + ).toEqual([]); + }); + + it('ignores a returnValues field that is not an array', () => { + const msg = roundTripPlugin({ + type: 'scriptComplete', + sessionId: 'sess-1', + payload: { success: true, returnValues: 'nope' }, + }); + expect( + (msg as { payload: { returnValues?: unknown[] } }).payload.returnValues + ).toBeUndefined(); + }); }); describe('register', () => { diff --git a/tools/studio-bridge/src/server/web-socket-protocol.ts b/tools/studio-bridge/src/server/web-socket-protocol.ts index 99eb17a2fe0..4587b34e3da 100644 --- a/tools/studio-bridge/src/server/web-socket-protocol.ts +++ b/tools/studio-bridge/src/server/web-socket-protocol.ts @@ -69,6 +69,22 @@ export type SerializedValue = | { type: 'Instance'; className: string; path: string } | { type: 'Unsupported'; typeName: string; toString: string }; +/** + * A value returned by an executed script, marshalled for the wire: everything + * `SerializedValue` covers, plus tables, which the plugin walks recursively + * (array-like tables stay arrays, anything else becomes an object with string + * keys). + * + * `{ type: 'Nil' }` stands in for a `nil` return value: a JSON array cannot + * hold a hole, and dropping the entry would silently shift every value after + * it, misreporting which value was returned where. + */ +export type SerializedReturnValue = + | SerializedValue + | { type: 'Nil' } + | SerializedReturnValue[] + | { [key: string]: SerializedReturnValue }; + export interface DataModelInstance { name: string; className: string; @@ -99,6 +115,13 @@ export interface ScriptCompleteMessage extends BaseMessage { success: boolean; error?: string; output?: Array<{ level: string; body: string; timestamp: number }>; + /** + * Everything the script returned, in order, marshalled by the plugin. + * Absent when the plugin never reported a return channel (an older plugin + * build, or a script that failed before returning) — distinct from `[]`, + * which means the script ran and returned nothing. + */ + returnValues?: SerializedReturnValue[]; }; } @@ -354,6 +377,11 @@ export function decodePluginMessage(raw: string): PluginMessage | null { timestamp: typeof e.timestamp === 'number' ? e.timestamp : 0, })) : undefined; + // The values themselves are whatever the script returned, so they are + // carried through as-is; only the array wrapper is checked. + const returnValues = Array.isArray(payload.returnValues) + ? (payload.returnValues as SerializedReturnValue[]) + : undefined; return { type: 'scriptComplete', sessionId, @@ -363,6 +391,7 @@ export function decodePluginMessage(raw: string): PluginMessage | null { error: typeof payload.error === 'string' ? payload.error : undefined, output, + returnValues, }, }; } diff --git a/tools/studio-bridge/templates/studio-bridge-plugin-test/test/execute-handler.test.luau b/tools/studio-bridge/templates/studio-bridge-plugin-test/test/execute-handler.test.luau index 30c3e5eb169..dc39d8ee465 100644 --- a/tools/studio-bridge/templates/studio-bridge-plugin-test/test/execute-handler.test.luau +++ b/tools/studio-bridge/templates/studio-bridge-plugin-test/test/execute-handler.test.luau @@ -338,6 +338,89 @@ table.insert(tests, { end, }) +table.insert(tests, { + name = "sequential queue: a request that throws while reporting is answered, not latched", + fn = function() + ExecuteAction._resetQueue() + local sentMessages = {} + local failNextSend = true + local sendMessage = function(msg) + if failNextSend then + failNextSend = false + error("encode failed") + end + table.insert(sentMessages, msg) + end + + local router = ActionRouter.new() + ExecuteAction.register(router, sendMessage) + + router:dispatch({ + type = "execute", + sessionId = "sess-wedge", + requestId = "req-w1", + payload = { code = "return 1" }, + }) + + assertEqual(#sentMessages, 1, "the failed request is reported rather than dropped") + assertEqual(sentMessages[1].requestId, "req-w1") + assertFalse(sentMessages[1].payload.success) + assertEqual(sentMessages[1].payload.code, "INTERNAL_ERROR") + + -- The queue must still drain: a latched _processing would leave this + -- request, and every later one, sitting in the queue forever. + router:dispatch({ + type = "execute", + sessionId = "sess-wedge", + requestId = "req-w2", + payload = { code = "print('innocent'); return 2" }, + }) + + assertEqual(#sentMessages, 2, "the next request still runs") + assertEqual(sentMessages[2].requestId, "req-w2") + assertTrue(sentMessages[2].payload.success) + assertEqual(sentMessages[2].payload.returnValues[1], 2) + end, +}) + +table.insert(tests, { + name = "sequential queue: drains even when the failure itself cannot be reported", + fn = function() + ExecuteAction._resetQueue() + + local deadRouter = ActionRouter.new() + ExecuteAction.register(deadRouter, function() + error("encode failed") + end) + + deadRouter:dispatch({ + type = "execute", + sessionId = "sess-wedge2", + requestId = "req-w3", + payload = { code = "return 1" }, + }) + + -- Nothing could be delivered for req-w3, so the only observable question + -- is whether the queue was released for whoever comes next. + local sentMessages = {} + local liveRouter = ActionRouter.new() + ExecuteAction.register(liveRouter, function(msg) + table.insert(sentMessages, msg) + end) + + liveRouter:dispatch({ + type = "execute", + sessionId = "sess-wedge2", + requestId = "req-w4", + payload = { code = "return 4" }, + }) + + assertEqual(#sentMessages, 1, "a later request still gets a response") + assertEqual(sentMessages[1].requestId, "req-w4") + assertTrue(sentMessages[1].payload.success) + end, +}) + -- =========================================================================== -- sendMessage callback integration -- =========================================================================== @@ -403,4 +486,180 @@ table.insert(tests, { end, }) +-- =========================================================================== +-- Return value marshalling +-- =========================================================================== + +table.insert(tests, { + name = "returnValues: empty when the script returns nothing", + fn = function() + local result = ExecuteAction.handleExecute({ code = "local x = 1" }, "req-rv0", "sess-rv0") + assertNotNil(result.returnValues, "returnValues should be present on a successful run") + assertEqual(#result.returnValues, 0, "no values returned") + end, +}) + +table.insert(tests, { + name = "returnValues: carries every returned value in order", + fn = function() + local result = ExecuteAction.handleExecute({ code = "return 'str', 42, true" }, "req-rv1", "sess-rv1") + assertEqual(#result.returnValues, 3, "three values returned") + assertEqual(result.returnValues[1], "str") + assertEqual(result.returnValues[2], 42) + assertEqual(result.returnValues[3], true) + end, +}) + +table.insert(tests, { + name = "returnValues: nested tables survive as tables", + fn = function() + local result = ExecuteAction.handleExecute( + { code = "return { slug = 'maid', counts = { passed = 1014, failed = 0 } }" }, + "req-rv2", + "sess-rv2" + ) + assertEqual(#result.returnValues, 1, "one value returned") + local value = result.returnValues[1] + assertEqual(value.slug, "maid") + assertEqual(value.counts.passed, 1014) + assertEqual(value.counts.failed, 0) + end, +}) + +table.insert(tests, { + name = "returnValues: array-like tables stay arrays", + fn = function() + local result = ExecuteAction.handleExecute({ code = "return { 10, 20, 30 }" }, "req-rv3", "sess-rv3") + local value = result.returnValues[1] + assertEqual(#value, 3, "array length") + assertEqual(value[1], 10) + assertEqual(value[3], 30) + end, +}) + +table.insert(tests, { + name = "returnValues: non-string keys become strings so the table can be encoded", + fn = function() + local result = ExecuteAction.handleExecute({ code = "return { [1] = 'a', name = 'b' }" }, "req-rv4", "sess-rv4") + local value = result.returnValues[1] + assertEqual(value["1"], "a", "numeric key stringified") + assertEqual(value.name, "b") + end, +}) + +table.insert(tests, { + name = "returnValues: a returned nil keeps its position", + fn = function() + local result = ExecuteAction.handleExecute({ code = "return nil, 7" }, "req-rv5", "sess-rv5") + assertEqual(#result.returnValues, 2, "two values returned") + assertEqual(result.returnValues[1].type, "Nil", "nil marker") + assertEqual(result.returnValues[2], 7) + end, +}) + +table.insert(tests, { + name = "returnValues: a cycle is reported instead of recursing forever", + fn = function() + local result = + ExecuteAction.handleExecute({ code = "local t = {}; t.self = t; return t" }, "req-rv6", "sess-rv6") + local value = result.returnValues[1] + assertEqual(value.self.type, "Unsupported", "cycle is marked unsupported") + assertContains(value.self.toString, "cycle") + end, +}) + +table.insert(tests, { + name = "returnValues: non-finite numbers are marked, not emitted as invalid JSON", + fn = function() + local result = ExecuteAction.handleExecute({ code = "return math.huge, 0 / 0" }, "req-rv7", "sess-rv7") + assertEqual(result.returnValues[1].type, "Unsupported", "inf is marked") + assertEqual(result.returnValues[1].typeName, "number") + assertEqual(result.returnValues[2].type, "Unsupported", "nan is marked") + end, +}) + +table.insert(tests, { + name = "returnValues: functions are marked unsupported rather than dropped", + fn = function() + local result = ExecuteAction.handleExecute({ code = "return function() end" }, "req-rv8", "sess-rv8") + assertEqual(#result.returnValues, 1, "one value returned") + assertEqual(result.returnValues[1].type, "Unsupported") + assertEqual(result.returnValues[1].typeName, "function") + end, +}) + +table.insert(tests, { + name = "returnValues: a hostile key metamethod degrades to a marker instead of throwing", + fn = function() + local result = ExecuteAction.handleExecute({ + code = "local k = setmetatable({}, { __tostring = function() error('bad tostring') end }); return { [k] = 1, name = 2 }", + }, "req-rv10", "sess-rv10") + assertTrue(result.success, "the run itself still succeeded") + assertEqual(#result.returnValues, 1, "one marker in place of the values") + assertEqual(result.returnValues[1].type, "Unsupported") + assertContains(result.returnValues[1].toString, "not serializable") + end, +}) + +table.insert(tests, { + name = "returnValues: __iter is bypassed, so real contents are marshalled", + fn = function() + local result = ExecuteAction.handleExecute({ + code = "return setmetatable({ real = 'x' }, { __iter = function() error('bad iter') end })", + }, "req-rv11", "sess-rv11") + assertTrue(result.success, "should succeed") + assertEqual(result.returnValues[1].real, "x", "raw contents, not whatever __iter yields") + end, +}) + +table.insert(tests, { + name = "returnValues: shared references cannot explode past the node budget", + fn = function() + -- Only 30 levels deep, but 2^30 nodes if every shared edge is expanded. + local result = ExecuteAction.handleExecute( + { code = "local x = {}; for _ = 1, 30 do x = { a = x, b = x } end; return x" }, + "req-rv12", + "sess-rv12" + ) + assertTrue(result.success, "should succeed") + + local function findBudgetMarker(value: any): boolean + if type(value) ~= "table" then + return false + end + if value.type == "Unsupported" and string.find(tostring(value.toString), "budget", 1, true) then + return true + end + for _, item in value do + if findBudgetMarker(item) then + return true + end + end + return false + end + + assertTrue(findBudgetMarker(result.returnValues[1]), "walk stopped at the budget") + end, +}) + +table.insert(tests, { + name = "returnValues: keys that collapse onto one JSON key are marked, not silently dropped", + fn = function() + local result = + ExecuteAction.handleExecute({ code = "return { [1] = 'a', ['1'] = 'b' }" }, "req-rv13", "sess-rv13") + local value = result.returnValues[1] + assertEqual(value["1"].type, "Unsupported", "collision is marked") + assertEqual(value["1"].typeName, "collision") + end, +}) + +table.insert(tests, { + name = "returnValues: absent on a failed run", + fn = function() + local result = ExecuteAction.handleExecute({ code = "error('boom')" }, "req-rv9", "sess-rv9") + assertFalse(result.success) + assertNil(result.returnValues, "a failed run reports no return values") + end, +}) + return tests