Copilot commented on code in PR #6436: URL: https://github.com/apache/texera/pull/6436#discussion_r3600302871
########## agent-service/src/server.recommend.spec.ts: ########## @@ -0,0 +1,80 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { describe, expect, test } from "bun:test"; +import { buildApp } from "./server"; +import { env } from "./config/env"; +import type { RecommendationResponse } from "./recommender/recommender-types"; + +const API = env.API_PREFIX; +const app = buildApp(); + +async function postRecommend(body: unknown): Promise<Response> { + return app.handle( + new Request(`http://localhost${API}/recommend`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }) + ); +} + +describe("POST /recommend", () => { + test("returns a hardcoded ranked list for a valid operator", async () => { + const res = await postRecommend({ operatorType: "Filter" }); + expect(res.status).toBe(200); + const data = (await res.json()) as RecommendationResponse; + expect(data.strategy).toBe("hardcoded"); + expect(data.recommendations.length).toBeGreaterThan(0); + for (const rec of data.recommendations) { + expect(typeof rec.operatorType).toBe("string"); + expect(rec.operatorType.length).toBeGreaterThan(0); + expect(rec.score).toBeGreaterThan(0); + expect(rec.reason.length).toBeGreaterThan(0); + } + }); + + test("honors the requested limit", async () => { + const res = await postRecommend({ operatorType: "Filter", limit: 1 }); + expect(res.status).toBe(200); + const data = (await res.json()) as RecommendationResponse; + expect(data.recommendations).toHaveLength(1); + }); + + test("rejects a request with no operatorType as 400", async () => { + const res = await postRecommend({ limit: 3 }); + expect(res.status).toBe(400); + }); + + test("rejects an empty operatorType as 400", async () => { + const res = await postRecommend({ operatorType: "" }); + expect(res.status).toBe(400); + }); + Review Comment: The route tests cover missing and empty-string `operatorType`, but they don’t cover the whitespace-only case (e.g. " ") which currently slips past the schema. Adding a regression test for whitespace-only input will prevent the endpoint from accidentally returning 200 with an empty recommendation list for effectively-empty operator types. ########## agent-service/src/server.ts: ########## @@ -388,6 +390,33 @@ const agentsRouter = new Elysia({ prefix: "/agents" }) } ); +// Stateless ambient operator recommender (apache/texera#5240). Given the +// operator a user just added, returns a short ranked list of likely next +// operators for the canvas to render as ghost suggestions. Version 1 uses a +// hardcoded rule table with no LLM call, so it needs no user token and no +// per-request model configuration; it only reads the process-wide operator +// catalog to validate and label its suggestions. +const recommendRouter = new Elysia({ prefix: "/recommend" }) + // Local-scoped error handler, mirroring agentsRouter: body-schema and + // malformed-JSON failures are client errors (400), not 500s. + .onError(({ code, error, set }) => { + log.error({ err: error }, "recommend request error"); + const errorMessage = error instanceof Error ? error.message : String(error); + if (code === "VALIDATION" || code === "PARSE") { + set.status = 400; + return { error: errorMessage || "Invalid request body" }; + } + set.status = 500; + return { error: errorMessage || "Internal server error" }; + }) + .post("/", ({ body }) => recommendOperators(body as RecommendationRequest, WorkflowSystemMetadata.getInstance()), { + body: t.Object({ + operatorType: t.String({ minLength: 1 }), + existingOperatorTypes: t.Optional(t.Array(t.String())), + limit: t.Optional(t.Number()), + }), + }); Review Comment: `operatorType` consisting only of whitespace currently passes the body schema (`minLength: 1`) and then gets `.trim()`'d inside `recommendOperators`, resulting in a 200 response with an empty recommendations list. If the endpoint is expected to treat missing/empty `operatorType` as a client error, this should return 400 for whitespace-only values too. ########## agent-service/src/server.ts: ########## @@ -388,6 +390,33 @@ const agentsRouter = new Elysia({ prefix: "/agents" }) } ); +// Stateless ambient operator recommender (apache/texera#5240). Given the +// operator a user just added, returns a short ranked list of likely next +// operators for the canvas to render as ghost suggestions. Version 1 uses a +// hardcoded rule table with no LLM call, so it needs no user token and no +// per-request model configuration; it only reads the process-wide operator +// catalog to validate and label its suggestions. +const recommendRouter = new Elysia({ prefix: "/recommend" }) + // Local-scoped error handler, mirroring agentsRouter: body-schema and + // malformed-JSON failures are client errors (400), not 500s. + .onError(({ code, error, set }) => { + log.error({ err: error }, "recommend request error"); + const errorMessage = error instanceof Error ? error.message : String(error); + if (code === "VALIDATION" || code === "PARSE") { + set.status = 400; + return { error: errorMessage || "Invalid request body" }; + } + set.status = 500; + return { error: errorMessage || "Internal server error" }; Review Comment: For non-validation errors, the handler currently returns the raw `errorMessage` to the client even when responding with HTTP 500. This can leak internal implementation details; it’s safer to log the error server-side and return a generic message for 5xx responses. ########## agent-service/src/recommender/recommender-types.ts: ########## @@ -0,0 +1,81 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Wire types for the ambient operator recommender (discussion apache/texera#5240). + * + * The recommender is a stateless endpoint that, given the operator a user just + * added to the canvas, returns a short ranked list of likely "next" operators + * to render as faded ghost suggestions on the output port. + * + * Version 1 (this file's consumer, `recommendOperators`) produces suggestions + * from a hardcoded rule table — no LLM call, no persistent state — so it can + * ship and validate the full canvas pipeline at zero API cost. Version 2 will + * swap the ranking strategy for a small LLM behind the same request/response + * shape; the `strategy` discriminator on the response lets clients tell the two + * apart without a breaking change. + */ + +/** Ranking strategy that produced a response. `llm` is reserved for V2. */ +export type RecommendationStrategy = "hardcoded" | "llm"; + +/** Hard ceiling on how many suggestions a single request may ask for. */ +export const MAX_RECOMMENDATION_LIMIT = 5; + +/** Default number of suggestions when the request does not specify a limit. */ +export const DEFAULT_RECOMMENDATION_LIMIT = 3; + +export interface RecommendationRequest { + /** + * The operator type that was just added and whose output port we are + * suggesting successors for (e.g. `"CSVFileScan"`). Primary ranking signal. + */ + operatorType: string; + + /** + * Optional: operator types already present on the canvas. Unused by the V1 + * hardcoded ranker; reserved so V2 can bias suggestions with graph context + * without a request-shape change. + */ + existingOperatorTypes?: string[]; + + /** + * Optional: maximum number of suggestions to return. Defaults to + * {@link DEFAULT_RECOMMENDATION_LIMIT}, clamped to + * `[1, {@link MAX_RECOMMENDATION_LIMIT}]`. + */ + limit?: number; +} + +export interface OperatorRecommendation { + /** Recommended operator type; always a real, catalog-known type. */ + operatorType: string; Review Comment: The docstring claims `operatorType` is “always a real, catalog-known type”, but `recommendOperators` explicitly returns unvalidated raw candidates when the catalog is missing/uninitialized. This comment should be relaxed to match the actual behavior (or the implementation should be changed to always validate). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
