From cd625e8b2a796d702f8bbeaa49ff89ec74d006a0 Mon Sep 17 00:00:00 2001 From: Pedro Date: Fri, 11 Sep 2026 16:46:58 -0600 Subject: [PATCH 01/39] feat(frontend): add AI workflow fixer with LLM-powered diff panel - AiWorkflowFixerService: error classification, LLM call via LiteLLM proxy - AiFixFrameComponent: diff view with approve/discard actions - Wires into result-panel on explicit button trigger - Reads Python UDF errors from ConsoleUpdateEvent path Co-Authored-By: Claude Opus 5 --- demo-workflows/README.md | 52 ++++ demo-workflows/combined.json | 90 +++++++ demo-workflows/missing_column.json | 90 +++++++ demo-workflows/model_not_found.json | 87 ++++++ demo-workflows/null_error.json | 90 +++++++ demo-workflows/type_error.json | 90 +++++++ .../ai-fix/ai-fix-frame.component.html | 111 ++++++++ .../ai-fix/ai-fix-frame.component.scss | 77 ++++++ .../ai-fix/ai-fix-frame.component.spec.ts | 250 ++++++++++++++++++ .../ai-fix/ai-fix-frame.component.ts | 132 +++++++++ .../result-panel/ai-fix/ai-fix-prompt.ts | 69 +++++ .../ai-fix/ai-workflow-fixer.service.spec.ts | 234 ++++++++++++++++ .../ai-fix/ai-workflow-fixer.service.ts | 222 ++++++++++++++++ .../result-panel/result-panel.component.ts | 14 +- 14 files changed, 1607 insertions(+), 1 deletion(-) create mode 100644 demo-workflows/README.md create mode 100644 demo-workflows/combined.json create mode 100644 demo-workflows/missing_column.json create mode 100644 demo-workflows/model_not_found.json create mode 100644 demo-workflows/null_error.json create mode 100644 demo-workflows/type_error.json create mode 100644 frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.html create mode 100644 frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.scss create mode 100644 frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.spec.ts create mode 100644 frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.ts create mode 100644 frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts create mode 100644 frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts create mode 100644 frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts diff --git a/demo-workflows/README.md b/demo-workflows/README.md new file mode 100644 index 00000000000..8542b9ba934 --- /dev/null +++ b/demo-workflows/README.md @@ -0,0 +1,52 @@ +# Demo workflows for the AI Workflow Fixer + +Five workflows that fail on purpose, one per error pattern the fixer handles. +Each file is a bare `WorkflowContent` (operators / links / positions / settings), +the same shape as `bin/single-node/examples/workflows/`. + +## Prerequisites + +All five read the example movies dataset, so bring the stack up with the +examples loaded first: + +```sh +bin/single-node.sh up --with-examples +``` + +That publishes `/dataset/texera/popular-movies-of-imdb/v1/TMDb_updated.csv` +(columns: `id`, `title`, `overview`, `original_language`, `vote_count`, +`vote_average`), which every workflow here scans. + +## Loading them + +Import a file from the workflow list in the GUI, or POST it the way +`bin/single-node/examples/load-examples.sh` does — the content travels as a +**string**, not as a nested object: + +```sh +curl -s -X POST "$TEXERA_DASHBOARD_SERVICE_URL/workflow/create" \ + -H "Authorization: Bearer $TOKEN" -H "Content-Type: application/json" \ + -d "{\"name\":\"missing_column\", \"content\": $(jq -Rs < demo-workflows/missing_column.json)}" +``` + +## What each one does + +| File | Operator | Expected failure | Fix the LLM should propose | +| --- | --- | --- | --- | +| `missing_column.json` | Python UDF | `KeyError: 'vote_counts'` | use `vote_count`, the closest column in the schema | +| `type_error.json` | Python UDF | `TypeError: unsupported operand type(s) for -: 'str' and 'int'` | cast `title`/`vote_count` explicitly | +| `null_error.json` | Python UDF | `ValueError: Input contains NaN.` | `.dropna()` (or `fillna`) before scaling | +| `model_not_found.json` | HuggingFace | provider 404 for the model | correct `modelId` to `Qwen/Qwen2.5-72B-Instruct` | +| `combined.json` | Python UDF | `KeyError: 'vote_avg'` first, `ValueError: Input contains NaN.` after | one fix per round: the missing column first | + +Notes: + +- The fixer classifies the **raw** error text, so the Python traceback that + arrives as an ERROR console message is what it reads — not the trimmed text + shown in the error tab. +- `model_not_found.json` needs a Hugging Face API token in the operator's + `hfApiToken` field to reach the provider and get the 404; the field is left + empty here on purpose so no credential is committed. +- The NaN cases inject the missing values themselves + (`table.loc[table.index % 7 == 0, ...] = float("nan")`) so the failure is + deterministic rather than dependent on gaps in the source data. diff --git a/demo-workflows/combined.json b/demo-workflows/combined.json new file mode 100644 index 00000000000..558baaec2fa --- /dev/null +++ b/demo-workflows/combined.json @@ -0,0 +1,90 @@ +{ + "operators": [ + { + "operatorID": "CSVFileScan-operator-demo-combined", + "operatorType": "CSVFileScan", + "operatorVersion": "N/A", + "operatorProperties": { + "fileEncoding": "UTF_8", + "customDelimiter": ",", + "hasHeader": true, + "fileName": "/dataset/texera/popular-movies-of-imdb/v1/TMDb_updated.csv" + }, + "inputPorts": [], + "outputPorts": [ + { + "portID": "output-0", + "displayName": "", + "allowMultiInputs": false, + "isDynamicPort": false + } + ], + "showAdvanced": false, + "isDisabled": false, + "customDisplayName": "Read Movies", + "dynamicInputPorts": false, + "dynamicOutputPorts": false, + "viewResult": false + }, + { + "operatorID": "PythonUDFV2-operator-demo-combined", + "operatorType": "PythonUDFV2", + "operatorVersion": "N/A", + "operatorProperties": { + "code": "from pytexera import *\nfrom sklearn.preprocessing import StandardScaler\n\n\nclass ProcessTableOperator(UDFTableOperator):\n\n @overrides\n def process_table(self, table: Table, port: int) -> Iterator[Optional[TableLike]]:\n # TWO BUGS ON PURPOSE, to check that only the first one is fixed per round.\n #\n # Bug 1 (raised first): the column is \"vote_average\", not \"vote_avg\".\n # Raises: KeyError: 'vote_avg'\n ratings = table[[\"vote_avg\"]]\n\n # Bug 2 (only reachable once bug 1 is fixed): the ratings still contain\n # missing values, so scaling fails.\n # Raises: ValueError: Input contains NaN.\n ratings.loc[ratings.index % 7 == 0] = float(\"nan\")\n table[\"rating_z\"] = StandardScaler().fit_transform(ratings)\n\n yield table\n", + "workers": 1, + "retainInputColumns": true, + "outputColumns": [ + { + "attributeName": "rating_z", + "attributeType": "double" + } + ] + }, + "inputPorts": [ + { + "portID": "input-0", + "displayName": "", + "allowMultiInputs": true, + "isDynamicPort": false, + "dependencies": [] + } + ], + "outputPorts": [ + { + "portID": "output-0", + "displayName": "", + "allowMultiInputs": false, + "isDynamicPort": false + } + ], + "showAdvanced": false, + "isDisabled": false, + "customDisplayName": "Standardize ratings (two bugs)", + "dynamicInputPorts": true, + "dynamicOutputPorts": true, + "viewResult": true + } + ], + "operatorPositions": { + "CSVFileScan-operator-demo-combined": { "x": 80, "y": 200 }, + "PythonUDFV2-operator-demo-combined": { "x": 340, "y": 200 } + }, + "links": [ + { + "linkID": "link-demo-combined", + "source": { + "operatorID": "CSVFileScan-operator-demo-combined", + "portID": "output-0" + }, + "target": { + "operatorID": "PythonUDFV2-operator-demo-combined", + "portID": "input-0" + } + } + ], + "commentBoxes": [], + "settings": { + "dataTransferBatchSize": 400 + } +} diff --git a/demo-workflows/missing_column.json b/demo-workflows/missing_column.json new file mode 100644 index 00000000000..4eb2d1e6938 --- /dev/null +++ b/demo-workflows/missing_column.json @@ -0,0 +1,90 @@ +{ + "operators": [ + { + "operatorID": "CSVFileScan-operator-demo-missing-column", + "operatorType": "CSVFileScan", + "operatorVersion": "N/A", + "operatorProperties": { + "fileEncoding": "UTF_8", + "customDelimiter": ",", + "hasHeader": true, + "fileName": "/dataset/texera/popular-movies-of-imdb/v1/TMDb_updated.csv" + }, + "inputPorts": [], + "outputPorts": [ + { + "portID": "output-0", + "displayName": "", + "allowMultiInputs": false, + "isDynamicPort": false + } + ], + "showAdvanced": false, + "isDisabled": false, + "customDisplayName": "Read Movies", + "dynamicInputPorts": false, + "dynamicOutputPorts": false, + "viewResult": false + }, + { + "operatorID": "PythonUDFV2-operator-demo-missing-column", + "operatorType": "PythonUDFV2", + "operatorVersion": "N/A", + "operatorProperties": { + "code": "from pytexera import *\n\n\nclass ProcessTupleOperator(UDFOperatorV2):\n\n @overrides\n def process_tuple(self, tuple_: Tuple, port: int) -> Iterator[Optional[TupleLike]]:\n # BROKEN ON PURPOSE: the column is called \"vote_count\", not \"vote_counts\".\n # Raises: KeyError: 'vote_counts'\n popularity = tuple_[\"vote_counts\"]\n\n tuple_[\"popularity_bucket\"] = \"high\" if popularity > 1000 else \"low\"\n yield tuple_\n", + "workers": 1, + "retainInputColumns": true, + "outputColumns": [ + { + "attributeName": "popularity_bucket", + "attributeType": "string" + } + ] + }, + "inputPorts": [ + { + "portID": "input-0", + "displayName": "", + "allowMultiInputs": true, + "isDynamicPort": false, + "dependencies": [] + } + ], + "outputPorts": [ + { + "portID": "output-0", + "displayName": "", + "allowMultiInputs": false, + "isDynamicPort": false + } + ], + "showAdvanced": false, + "isDisabled": false, + "customDisplayName": "Bucket by popularity (missing column)", + "dynamicInputPorts": true, + "dynamicOutputPorts": true, + "viewResult": true + } + ], + "operatorPositions": { + "CSVFileScan-operator-demo-missing-column": { "x": 80, "y": 200 }, + "PythonUDFV2-operator-demo-missing-column": { "x": 340, "y": 200 } + }, + "links": [ + { + "linkID": "link-demo-missing-column", + "source": { + "operatorID": "CSVFileScan-operator-demo-missing-column", + "portID": "output-0" + }, + "target": { + "operatorID": "PythonUDFV2-operator-demo-missing-column", + "portID": "input-0" + } + } + ], + "commentBoxes": [], + "settings": { + "dataTransferBatchSize": 400 + } +} diff --git a/demo-workflows/model_not_found.json b/demo-workflows/model_not_found.json new file mode 100644 index 00000000000..b38aac716c5 --- /dev/null +++ b/demo-workflows/model_not_found.json @@ -0,0 +1,87 @@ +{ + "operators": [ + { + "operatorID": "CSVFileScan-operator-demo-model-not-found", + "operatorType": "CSVFileScan", + "operatorVersion": "N/A", + "operatorProperties": { + "fileEncoding": "UTF_8", + "customDelimiter": ",", + "hasHeader": true, + "fileName": "/dataset/texera/popular-movies-of-imdb/v1/TMDb_updated.csv" + }, + "inputPorts": [], + "outputPorts": [ + { + "portID": "output-0", + "displayName": "", + "allowMultiInputs": false, + "isDynamicPort": false + } + ], + "showAdvanced": false, + "isDisabled": false, + "customDisplayName": "Read Movies", + "dynamicInputPorts": false, + "dynamicOutputPorts": false, + "viewResult": false + }, + { + "operatorID": "HuggingFace-operator-demo-model-not-found", + "operatorType": "HuggingFace", + "operatorVersion": "N/A", + "operatorProperties": { + "hfApiToken": "", + "task": "text-generation", + "modelId": "Qwen/Qwen2.5-72B-Instrukt", + "promptColumn": "overview", + "systemPrompt": "Summarize the movie plot in one sentence.", + "resultColumn": "summary" + }, + "inputPorts": [ + { + "portID": "input-0", + "displayName": "", + "allowMultiInputs": true, + "isDynamicPort": false, + "dependencies": [] + } + ], + "outputPorts": [ + { + "portID": "output-0", + "displayName": "", + "allowMultiInputs": false, + "isDynamicPort": false + } + ], + "showAdvanced": false, + "isDisabled": false, + "customDisplayName": "Summarize plots (model typo)", + "dynamicInputPorts": false, + "dynamicOutputPorts": false, + "viewResult": true + } + ], + "operatorPositions": { + "CSVFileScan-operator-demo-model-not-found": { "x": 80, "y": 200 }, + "HuggingFace-operator-demo-model-not-found": { "x": 340, "y": 200 } + }, + "links": [ + { + "linkID": "link-demo-model-not-found", + "source": { + "operatorID": "CSVFileScan-operator-demo-model-not-found", + "portID": "output-0" + }, + "target": { + "operatorID": "HuggingFace-operator-demo-model-not-found", + "portID": "input-0" + } + } + ], + "commentBoxes": [], + "settings": { + "dataTransferBatchSize": 400 + } +} diff --git a/demo-workflows/null_error.json b/demo-workflows/null_error.json new file mode 100644 index 00000000000..31665ac3d5c --- /dev/null +++ b/demo-workflows/null_error.json @@ -0,0 +1,90 @@ +{ + "operators": [ + { + "operatorID": "CSVFileScan-operator-demo-null-error", + "operatorType": "CSVFileScan", + "operatorVersion": "N/A", + "operatorProperties": { + "fileEncoding": "UTF_8", + "customDelimiter": ",", + "hasHeader": true, + "fileName": "/dataset/texera/popular-movies-of-imdb/v1/TMDb_updated.csv" + }, + "inputPorts": [], + "outputPorts": [ + { + "portID": "output-0", + "displayName": "", + "allowMultiInputs": false, + "isDynamicPort": false + } + ], + "showAdvanced": false, + "isDisabled": false, + "customDisplayName": "Read Movies", + "dynamicInputPorts": false, + "dynamicOutputPorts": false, + "viewResult": false + }, + { + "operatorID": "PythonUDFV2-operator-demo-null-error", + "operatorType": "PythonUDFV2", + "operatorVersion": "N/A", + "operatorProperties": { + "code": "from pytexera import *\nfrom sklearn.preprocessing import StandardScaler\n\n\nclass ProcessTableOperator(UDFTableOperator):\n\n @overrides\n def process_table(self, table: Table, port: int) -> Iterator[Optional[TableLike]]:\n # Stand-in for a real source with gaps: every 7th rating is missing.\n table.loc[table.index % 7 == 0, \"vote_average\"] = float(\"nan\")\n\n # BROKEN ON PURPOSE: StandardScaler rejects missing values.\n # The fix is a .dropna() (or a fillna) before scaling.\n # Raises: ValueError: Input contains NaN.\n table[\"rating_z\"] = StandardScaler().fit_transform(table[[\"vote_average\"]])\n\n yield table\n", + "workers": 1, + "retainInputColumns": true, + "outputColumns": [ + { + "attributeName": "rating_z", + "attributeType": "double" + } + ] + }, + "inputPorts": [ + { + "portID": "input-0", + "displayName": "", + "allowMultiInputs": true, + "isDynamicPort": false, + "dependencies": [] + } + ], + "outputPorts": [ + { + "portID": "output-0", + "displayName": "", + "allowMultiInputs": false, + "isDynamicPort": false + } + ], + "showAdvanced": false, + "isDisabled": false, + "customDisplayName": "Standardize ratings (NaN)", + "dynamicInputPorts": true, + "dynamicOutputPorts": true, + "viewResult": true + } + ], + "operatorPositions": { + "CSVFileScan-operator-demo-null-error": { "x": 80, "y": 200 }, + "PythonUDFV2-operator-demo-null-error": { "x": 340, "y": 200 } + }, + "links": [ + { + "linkID": "link-demo-null-error", + "source": { + "operatorID": "CSVFileScan-operator-demo-null-error", + "portID": "output-0" + }, + "target": { + "operatorID": "PythonUDFV2-operator-demo-null-error", + "portID": "input-0" + } + } + ], + "commentBoxes": [], + "settings": { + "dataTransferBatchSize": 400 + } +} diff --git a/demo-workflows/type_error.json b/demo-workflows/type_error.json new file mode 100644 index 00000000000..3cb12a74fb2 --- /dev/null +++ b/demo-workflows/type_error.json @@ -0,0 +1,90 @@ +{ + "operators": [ + { + "operatorID": "CSVFileScan-operator-demo-type-error", + "operatorType": "CSVFileScan", + "operatorVersion": "N/A", + "operatorProperties": { + "fileEncoding": "UTF_8", + "customDelimiter": ",", + "hasHeader": true, + "fileName": "/dataset/texera/popular-movies-of-imdb/v1/TMDb_updated.csv" + }, + "inputPorts": [], + "outputPorts": [ + { + "portID": "output-0", + "displayName": "", + "allowMultiInputs": false, + "isDynamicPort": false + } + ], + "showAdvanced": false, + "isDisabled": false, + "customDisplayName": "Read Movies", + "dynamicInputPorts": false, + "dynamicOutputPorts": false, + "viewResult": false + }, + { + "operatorID": "PythonUDFV2-operator-demo-type-error", + "operatorType": "PythonUDFV2", + "operatorVersion": "N/A", + "operatorProperties": { + "code": "from pytexera import *\n\n\nclass ProcessTupleOperator(UDFOperatorV2):\n\n @overrides\n def process_tuple(self, tuple_: Tuple, port: int) -> Iterator[Optional[TupleLike]]:\n # BROKEN ON PURPOSE: \"title\" is a string column and \"vote_count\" is an int,\n # so the subtraction has no valid operands. The fix is an explicit cast.\n # Raises: TypeError: unsupported operand type(s) for -: 'str' and 'int'\n score = tuple_[\"title\"] - tuple_[\"vote_count\"]\n\n tuple_[\"score\"] = score\n yield tuple_\n", + "workers": 1, + "retainInputColumns": true, + "outputColumns": [ + { + "attributeName": "score", + "attributeType": "double" + } + ] + }, + "inputPorts": [ + { + "portID": "input-0", + "displayName": "", + "allowMultiInputs": true, + "isDynamicPort": false, + "dependencies": [] + } + ], + "outputPorts": [ + { + "portID": "output-0", + "displayName": "", + "allowMultiInputs": false, + "isDynamicPort": false + } + ], + "showAdvanced": false, + "isDisabled": false, + "customDisplayName": "Compute score (type error)", + "dynamicInputPorts": true, + "dynamicOutputPorts": true, + "viewResult": true + } + ], + "operatorPositions": { + "CSVFileScan-operator-demo-type-error": { "x": 80, "y": 200 }, + "PythonUDFV2-operator-demo-type-error": { "x": 340, "y": 200 } + }, + "links": [ + { + "linkID": "link-demo-type-error", + "source": { + "operatorID": "CSVFileScan-operator-demo-type-error", + "portID": "output-0" + }, + "target": { + "operatorID": "PythonUDFV2-operator-demo-type-error", + "portID": "input-0" + } + } + ], + "commentBoxes": [], + "settings": { + "dataTransferBatchSize": 400 + } +} diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.html b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.html new file mode 100644 index 00000000000..0f63313ab4e --- /dev/null +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.html @@ -0,0 +1,111 @@ + + + +
+ + {{ state.status === "analyzing" ? "Analyzing error with AI..." : "Applying fix..." }} +
+ + +
+ +
+ + + + + + + + +
🔍 Root cause
+

{{ fix.explanation }}

+ +
+ 📝 Proposed fix + {{ fix.confidence }} confidence +
+ +
+ Configuration field: {{ fix.fieldName }} +
+ +
+
+ - {{ line }} +
+
+ + {{ line }} +
+
+ +
+ + +
+ + diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.scss b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.scss new file mode 100644 index 00000000000..068fa64ab12 --- /dev/null +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.scss @@ -0,0 +1,77 @@ +/** + * 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. + */ + +:host { + display: block; + width: 100%; + padding: 8px; +} + +.ai-fix-status { + align-items: center; + display: flex; + gap: 8px; + padding: 8px 0; +} + +.ai-fix-section-title { + font-weight: bold; + margin-top: 8px; +} + +.ai-fix-explanation { + margin: 4px 0 8px; + user-select: text; +} + +.ai-fix-field { + margin-bottom: 4px; + user-select: text; +} + +/* Same monospace treatment the other result-panel frames use for code. */ +.ai-fix-diff { + border: 1px solid #e8e8e8; + border-radius: 2px; + font-family: monaco, serif; + font-size: 11px; + line-height: 16px; + margin: 4px 0 8px; + overflow-x: auto; + white-space: pre; + user-select: text; +} + +.ai-fix-removed { + background-color: #ffeef0; + color: #82071e; + padding: 0 6px; +} + +.ai-fix-added { + background-color: #e6ffed; + color: #116329; + padding: 0 6px; +} + +.ai-fix-actions { + display: flex; + gap: 8px; + margin-top: 8px; +} diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.spec.ts new file mode 100644 index 00000000000..7cbfc4181ec --- /dev/null +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.spec.ts @@ -0,0 +1,250 @@ +/** + * 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 { ComponentFixture, TestBed } from "@angular/core/testing"; +import { By } from "@angular/platform-browser"; +import { BehaviorSubject } from "rxjs"; +import { HttpClientTestingModule } from "@angular/common/http/testing"; +import { AiFixFrameComponent } from "./ai-fix-frame.component"; +import { AiWorkflowFixerService, FixState, UNSUPPORTED_MESSAGE } from "./ai-workflow-fixer.service"; +import { ExecuteWorkflowService } from "../../../service/execute-workflow/execute-workflow.service"; +import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; +import { WorkflowCompilingService } from "../../../service/compile-workflow/workflow-compiling.service"; +import { WorkflowConsoleService } from "../../../service/workflow-console/workflow-console.service"; +import { commonTestProviders } from "../../../../common/testing/test-utils"; +import { WorkflowFatalError } from "../../../types/workflow-websocket.interface"; +import { ConsoleMessage } from "../../../types/workflow-common.interface"; + +const IDLE: FixState = { operatorId: "", errorMessage: "", errorType: "unsupported", status: "idle" }; + +const READY_CODE_FIX: FixState = { + operatorId: "op1", + errorMessage: "KeyError: 'email'", + errorType: "missing_column", + status: "ready", + suggestedFix: { + type: "code_change", + original: "yield t['email']", + suggested: "yield t['user_email']\nyield t", + explanation: "the column is user_email", + confidence: "high", + }, +}; + +const READY_PROPERTY_FIX: FixState = { + ...READY_CODE_FIX, + errorType: "model_not_found", + suggestedFix: { + type: "property_change", + original: "gpt-4-turb", + suggested: "gpt-4-turbo", + explanation: "the model name has a typo", + confidence: "medium", + fieldName: "model", + }, +}; + +const fatalError = (over: Partial = {}) => + ({ + message: "KeyError: 'email'", + details: "Traceback ...", + operatorId: "op1", + workerId: "w1", + type: { name: "EXECUTION_FAILURE" }, + timestamp: { nanos: 0, seconds: 0 }, + ...over, + }) as WorkflowFatalError; + +const consoleMessage = (name: string, message: string) => + ({ + workerId: "w1", + timestamp: { nanos: 0, seconds: 0 }, + msgType: { name }, + source: "udf.py:process_tuple:7", + title: "KeyError: 'email'", + message, + }) as ConsoleMessage; + +describe("AiFixFrameComponent", () => { + let component: AiFixFrameComponent; + let fixture: ComponentFixture; + let state$: BehaviorSubject; + let analyzeError: ReturnType; + let applyFix: ReturnType; + let discardFix: ReturnType; + let getErrorMessages: ReturnType; + let getConsoleMessages: ReturnType; + let getPortInputSchema: ReturnType; + let operatorProperties: Record; + + const render = (state: FixState) => { + state$.next(state); + fixture.detectChanges(); + }; + const query = (css: string) => fixture.debugElement.query(By.css(css)); + // Angular surrounds an interpolation with layout whitespace, so compare the content itself. + const texts = (css: string) => + fixture.debugElement.queryAll(By.css(css)).map(el => el.nativeElement.textContent.trim()); + + beforeEach(async () => { + state$ = new BehaviorSubject(IDLE); + analyzeError = vi.fn().mockResolvedValue(undefined); + applyFix = vi.fn(); + discardFix = vi.fn(); + getErrorMessages = vi.fn().mockReturnValue([]); + getConsoleMessages = vi.fn().mockReturnValue(undefined); + getPortInputSchema = vi.fn().mockReturnValue([{ attributeName: "user_email", attributeType: "string" }]); + operatorProperties = { code: "yield t['email']", workers: 1 }; + + await TestBed.configureTestingModule({ + imports: [AiFixFrameComponent, HttpClientTestingModule], + providers: [ + { + provide: AiWorkflowFixerService, + useValue: { getState$: () => state$.asObservable(), analyzeError, applyFix, discardFix }, + }, + { provide: ExecuteWorkflowService, useValue: { getErrorMessages } }, + { + provide: WorkflowActionService, + useValue: { getTexeraGraph: () => ({ getOperator: () => ({ operatorProperties }) }) }, + }, + { provide: WorkflowCompilingService, useValue: { getPortInputSchema } }, + { provide: WorkflowConsoleService, useValue: { getConsoleMessages } }, + ...commonTestProviders, + ], + }).compileComponents(); + + fixture = TestBed.createComponent(AiFixFrameComponent); + component = fixture.componentInstance; + component.operatorId = "op1"; + fixture.detectChanges(); + }); + + afterEach(() => vi.restoreAllMocks()); + + it("should create", () => { + expect(component).toBeTruthy(); + }); + + describe("the analyze button", () => { + it("passes the fatal error, schema, code and properties to the service", () => { + getErrorMessages.mockReturnValue([fatalError({ operatorId: "other" }), fatalError()]); + + query(".ai-fix-analyze").nativeElement.click(); + + expect(analyzeError).toHaveBeenCalledWith( + "op1", + "KeyError: 'email'\nTraceback ...", + [{ attributeName: "user_email", attributeType: "string" }], + "yield t['email']", + operatorProperties + ); + }); + + it("falls back to the Python traceback, which never becomes a fatal error", () => { + getConsoleMessages.mockReturnValue([ + consoleMessage("PRINT", "hello"), + consoleMessage("ERROR", "Traceback (most recent call last):\nKeyError: 'email'"), + ]); + + query(".ai-fix-analyze").nativeElement.click(); + + expect(analyzeError.mock.calls[0][1]).toEqual("Traceback (most recent call last):\nKeyError: 'email'"); + }); + + it("does nothing when the operator has no error at all", () => { + query(".ai-fix-analyze").nativeElement.click(); + expect(analyzeError).not.toHaveBeenCalled(); + }); + + it("is disabled without an operator, and hidden once a suggestion is on screen", () => { + component.operatorId = undefined; + fixture.detectChanges(); + expect(query(".ai-fix-analyze").nativeElement.disabled).toBe(true); + + component.operatorId = "op1"; + render(READY_CODE_FIX); + expect(query(".ai-fix-analyze")).toBeNull(); + }); + }); + + it("shows a spinner while analyzing", () => { + render({ ...READY_CODE_FIX, status: "analyzing", suggestedFix: undefined }); + + expect(query("nz-spin")).toBeTruthy(); + expect(query(".ai-fix-status").nativeElement.textContent).toContain("Analyzing error with AI"); + expect(query(".ai-fix-actions")).toBeNull(); + }); + + it("shows the not-supported message for an out-of-scope error", () => { + render({ ...READY_CODE_FIX, errorType: "unsupported", suggestedFix: undefined }); + + expect(query("nz-alert")).toBeTruthy(); + expect(fixture.nativeElement.textContent).toContain(UNSUPPORTED_MESSAGE); + expect(query(".ai-fix-actions")).toBeNull(); + }); + + it("renders the root cause, a confidence badge, and one diff line per line of the fix", () => { + render(READY_CODE_FIX); + + expect(query(".ai-fix-explanation").nativeElement.textContent).toContain("the column is user_email"); + expect(query("nz-tag").nativeElement.textContent).toContain("high"); + expect(texts(".ai-fix-removed")).toEqual(["- yield t['email']"]); + // The suggestion spans two lines, so it renders two added rows. + expect(texts(".ai-fix-added")).toEqual(["+ yield t['user_email']", "+ yield t"]); + }); + + it("renders the field name and values instead of code for a property change", () => { + render(READY_PROPERTY_FIX); + + expect(query(".ai-fix-field").nativeElement.textContent).toContain("model"); + expect(texts(".ai-fix-removed")).toEqual(["- gpt-4-turb"]); + expect(texts(".ai-fix-added")).toEqual(["+ gpt-4-turbo"]); + }); + + it("delegates the apply and discard buttons to the service", () => { + render(READY_CODE_FIX); + + query(".ai-fix-apply").nativeElement.click(); + query(".ai-fix-discard").nativeElement.click(); + + expect(applyFix).toHaveBeenCalledTimes(1); + expect(discardFix).toHaveBeenCalledTimes(1); + }); + + it("reports the applied state and offers a fresh analysis", () => { + render({ ...READY_CODE_FIX, status: "applied" }); + + expect(fixture.nativeElement.textContent).toContain("Fix applied"); + expect(query(".ai-fix-actions")).toBeNull(); + expect(query(".ai-fix-analyze")).toBeTruthy(); + }); + + it("reports a failed analysis", () => { + render({ ...READY_CODE_FIX, status: "error", suggestedFix: undefined }); + + expect(fixture.nativeElement.textContent).toContain("could not suggest a fix"); + }); + + it("maps confidence to a badge colour", () => { + expect(component.confidenceColor("high")).toEqual("green"); + expect(component.confidenceColor("medium")).toEqual("gold"); + expect(component.confidenceColor("low")).toEqual("red"); + }); +}); diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.ts new file mode 100644 index 00000000000..1aba2bd772f --- /dev/null +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.ts @@ -0,0 +1,132 @@ +/** + * 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 { Component, Input } from "@angular/core"; +import { AsyncPipe, NgFor, NgIf } from "@angular/common"; +import { Observable } from "rxjs"; +import { NzAlertComponent } from "ng-zorro-antd/alert"; +import { NzSpinComponent } from "ng-zorro-antd/spin"; +import { NzTagComponent } from "ng-zorro-antd/tag"; +import { NzButtonComponent } from "ng-zorro-antd/button"; +import { NzWaveDirective } from "ng-zorro-antd/core/wave"; +import { NzIconDirective } from "ng-zorro-antd/icon"; +import { ExecuteWorkflowService } from "../../../service/execute-workflow/execute-workflow.service"; +import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; +import { WorkflowCompilingService } from "../../../service/compile-workflow/workflow-compiling.service"; +import { WorkflowConsoleService } from "../../../service/workflow-console/workflow-console.service"; +import { AiWorkflowFixerService, FixConfidence, FixState, UNSUPPORTED_MESSAGE } from "./ai-workflow-fixer.service"; + +/** + * Result-panel frame that asks the LLM to fix the failed operator. + * + * The analysis is started by an explicit button: ResultPanelComponent rebuilds + * every frame on each re-render (clearResultPanel), so analysing on init would + * fire a model request per re-render. For the same reason all state lives in + * AiWorkflowFixerService and this component keeps none. + */ +@Component({ + selector: "texera-ai-fix-frame", + templateUrl: "./ai-fix-frame.component.html", + styleUrls: ["./ai-fix-frame.component.scss"], + imports: [ + NgIf, + NgFor, + AsyncPipe, + NzAlertComponent, + NzSpinComponent, + NzTagComponent, + NzButtonComponent, + NzWaveDirective, + NzIconDirective, + ], +}) +export class AiFixFrameComponent { + @Input() operatorId?: string; + + public readonly state$: Observable; + public readonly unsupportedMessage = UNSUPPORTED_MESSAGE; + + constructor( + private aiWorkflowFixerService: AiWorkflowFixerService, + private executeWorkflowService: ExecuteWorkflowService, + private workflowActionService: WorkflowActionService, + private workflowCompilingService: WorkflowCompilingService, + private workflowConsoleService: WorkflowConsoleService + ) { + this.state$ = aiWorkflowFixerService.getState$(); + } + + /** Collects the operator's error, code, schema and config, then asks for a fix. */ + public onAnalyze(): void { + const operatorId = this.operatorId; + if (!operatorId) { + return; + } + const errorMessage = this.rawErrorMessage(operatorId); + if (!errorMessage) { + return; + } + const properties = this.workflowActionService.getTexeraGraph().getOperator(operatorId).operatorProperties ?? {}; + void this.aiWorkflowFixerService.analyzeError( + operatorId, + errorMessage, + this.workflowCompilingService.getPortInputSchema(operatorId, 0), + properties.code as string | undefined, + properties + ); + } + + public onApply(): void { + this.aiWorkflowFixerService.applyFix(); + } + + public onDiscard(): void { + this.aiWorkflowFixerService.discardFix(); + } + + public confidenceColor(confidence: FixConfidence): string { + if (confidence === "high") { + return "green"; + } + return confidence === "medium" ? "gold" : "red"; + } + + /** Splits a snippet so the template can prefix each line with - or +. */ + public splitLines(snippet: string): string[] { + return snippet.split("\n"); + } + + /** + * The untrimmed error text for this operator. + * + * Engine failures arrive as fatal errors, but a Python UDF exception never + * becomes one: it is delivered as an ERROR console message whose `message` + * holds the whole traceback, which is what the classifier needs. + */ + private rawErrorMessage(operatorId: string): string | undefined { + const fatal = this.executeWorkflowService.getErrorMessages().find(err => err.operatorId === operatorId); + if (fatal) { + return [fatal.message, fatal.details].filter(part => part).join("\n"); + } + const consoleError = (this.workflowConsoleService.getConsoleMessages(operatorId) ?? []) + .filter(msg => msg.msgType.name === "ERROR") + .pop(); + return consoleError ? consoleError.message || consoleError.title : undefined; + } +} diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts new file mode 100644 index 00000000000..ac4e9d7bb91 --- /dev/null +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts @@ -0,0 +1,69 @@ +/** + * 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 { PortSchema } from "../../../types/workflow-compiling.interface"; + +/** + * Builds the single user message sent to the LLM for one failed operator. + * + * The model is asked for exactly one minimal fix and for raw JSON. The reply is + * still parsed defensively (fenced blocks are unwrapped) because "no markdown" + * is a request, not a guarantee. + */ +export function buildFixPrompt( + errorMessage: string, + operatorCode: string | undefined, + schema: PortSchema | undefined, + operatorProperties: Readonly> +): string { + return `You are a debugging assistant for a Python data processing operator in a workflow engine. + +Error +${errorMessage} + +Operator code +\`\`\`python +${operatorCode ?? "(this operator has no user code)"} +\`\`\` + +Input schema (column names and types) +${JSON.stringify(schema ?? [], null, 2)} + +Operator configuration +${JSON.stringify(operatorProperties, null, 2)} + +Task +Diagnose the root cause of the error. Then propose exactly ONE fix. + +Rules: +- If it is a missing column error: suggest the closest column name from the schema. +- If it is a type error: add an explicit cast (e.g., int(), str(), float()) at the point of failure. +- If it is a null/NaN error: add a .dropna() call or an explicit null guard. +- If it is a model name error: correct the model field in the configuration. +- Do NOT rewrite the whole function. Change the minimum number of lines. + +Respond ONLY with valid JSON, no markdown, no explanation outside the JSON: +{ + "explanation": "one sentence describing the root cause", + "fix_type": "code_change" | "property_change", + "original_snippet": "exact lines to replace (for code_change) or field name (for property_change)", + "suggested_snippet": "replacement lines or new field value", + "confidence": "high" | "medium" | "low" +}`; +} diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts new file mode 100644 index 00000000000..32ecb63e0b2 --- /dev/null +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts @@ -0,0 +1,234 @@ +/** + * 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 { TestBed } from "@angular/core/testing"; +import { firstValueFrom } from "rxjs"; +import { AiWorkflowFixerService, classifyError, FixState } from "./ai-workflow-fixer.service"; +import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; +import { ExecuteWorkflowService } from "../../../service/execute-workflow/execute-workflow.service"; +import { commonTestProviders } from "../../../../common/testing/test-utils"; +import { PortSchema } from "../../../types/workflow-compiling.interface"; + +const OP = "PythonUDFV2-op-1"; +const SCHEMA: PortSchema = [ + { attributeName: "user_email", attributeType: "string" }, + { attributeName: "follower_count", attributeType: "integer" }, +]; +const CODE = + "class ProcessTupleOperator(UDFOperatorV2):\n def process_tuple(self, t, port):\n yield t['email']\n"; + +// A UDF traceback as the Python worker sends it: the whole stack in one string, +// so the regexes must match a line inside it rather than the start of the message. +const KEY_ERROR = `Traceback (most recent call last):\n File "udf.py", line 3\n yield t['email']\nKeyError: 'email'`; + +describe("AiWorkflowFixerService", () => { + let service: AiWorkflowFixerService; + let setOperatorProperty: ReturnType; + let executeWorkflow: ReturnType; + let operatorProperties: Record; + + // Stub the transport at the class seam (callModel), as migration-llm.spec.ts does: + // mocking the "ai" module leaks across specs sharing the import and hits the network. + const stubModel = (raw: string) => vi.spyOn(service as any, "callModel").mockResolvedValue({ text: raw }); + const state = () => (service as any).stateSubject.getValue() as FixState; + + const suggestion = (over: object = {}) => + JSON.stringify({ + explanation: "the column is user_email", + fix_type: "code_change", + original_snippet: "yield t['email']", + suggested_snippet: "yield t['user_email']", + confidence: "high", + ...over, + }); + + beforeEach(() => { + setOperatorProperty = vi.fn(); + executeWorkflow = vi.fn(); + operatorProperties = { code: CODE, workers: 1 }; + + TestBed.configureTestingModule({ + providers: [ + AiWorkflowFixerService, + { + provide: WorkflowActionService, + useValue: { + setOperatorProperty, + getTexeraGraph: () => ({ getOperator: () => ({ operatorProperties }) }), + }, + }, + { provide: ExecuteWorkflowService, useValue: { executeWorkflow } }, + ...commonTestProviders, + ], + }); + service = TestBed.inject(AiWorkflowFixerService); + }); + + describe("classifyError", () => { + it("detects each supported pattern", () => { + expect(classifyError(KEY_ERROR)).toEqual("missing_column"); + expect(classifyError("TypeError: unsupported operand type(s) for *: 'str' and 'int'")).toEqual("type_error"); + expect(classifyError("ValueError: input contains NaN")).toEqual("null_error"); + expect(classifyError("java.lang.NullPointerException: boom")).toEqual("null_error"); + expect(classifyError("ModelNotFound: no such model")).toEqual("model_not_found"); + expect(classifyError("Error code: 404 - model does not exist")).toEqual("model_not_found"); + }); + + it("returns unsupported for out-of-scope, empty, and near-miss messages", () => { + expect(classifyError("ZeroDivisionError: division by zero")).toEqual("unsupported"); + expect(classifyError("")).toEqual("unsupported"); + expect(classifyError("TypeError: 'int' object is not iterable")).toEqual("unsupported"); + }); + }); + + describe("analyzeError", () => { + it("moves idle -> analyzing -> ready and maps the LLM response into the fix", async () => { + stubModel(suggestion()); + const seen: string[] = []; + service.getState$().subscribe(s => seen.push(s.status)); + + await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + + expect(seen).toEqual(["idle", "analyzing", "ready"]); + expect(state().errorType).toEqual("missing_column"); + expect(state().suggestedFix).toEqual({ + type: "code_change", + original: "yield t['email']", + suggested: "yield t['user_email']", + explanation: "the column is user_email", + confidence: "high", + fieldName: undefined, + }); + }); + + it("tolerates a fenced JSON response even though the prompt forbids markdown", async () => { + stubModel("```json\n" + suggestion() + "\n```"); + await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + expect(state().status).toEqual("ready"); + }); + + it("shows the current field value as the original for a property change", async () => { + operatorProperties = { model: "gpt-4-turb" }; + stubModel( + suggestion({ fix_type: "property_change", original_snippet: "model", suggested_snippet: "gpt-4-turbo" }) + ); + + await service.analyzeError(OP, "Error code: 404 - model not found", SCHEMA, undefined, operatorProperties); + + expect(state().suggestedFix?.fieldName).toEqual("model"); + expect(state().suggestedFix?.original).toEqual("gpt-4-turb"); + expect(state().suggestedFix?.suggested).toEqual("gpt-4-turbo"); + }); + + it("never calls the model for an out-of-scope error", async () => { + const callModel = vi.spyOn(service as any, "callModel"); + await service.analyzeError(OP, "ZeroDivisionError: division by zero", SCHEMA, CODE, operatorProperties); + + expect(callModel).not.toHaveBeenCalled(); + expect(state().errorType).toEqual("unsupported"); + expect(state().status).toEqual("ready"); + expect(state().suggestedFix).toBeUndefined(); + }); + + it("ends in error on transport failure, malformed JSON, or a missing field", async () => { + vi.spyOn(service as any, "callModel").mockRejectedValue(new Error("502")); + await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + expect(state().status).toEqual("error"); + + stubModel("I cannot help with that."); + await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + expect(state().status).toEqual("error"); + + stubModel(JSON.stringify({ explanation: "e", fix_type: "code_change", confidence: "low" })); + await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + expect(state().status).toEqual("error"); + }); + }); + + describe("applyFix", () => { + it("replaces only the snippet, keeps other properties, and re-runs", async () => { + stubModel(suggestion()); + await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + + service.applyFix(); + + expect(setOperatorProperty).toHaveBeenCalledWith(OP, { + code: CODE.replace("yield t['email']", "yield t['user_email']"), + workers: 1, + }); + expect(executeWorkflow).toHaveBeenCalledWith(""); + expect(state().status).toEqual("applied"); + }); + + it("patches the named field for a property change", async () => { + operatorProperties = { model: "gpt-4-turb", temperature: 0 }; + stubModel( + suggestion({ fix_type: "property_change", original_snippet: "model", suggested_snippet: "gpt-4-turbo" }) + ); + await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + + service.applyFix(); + + expect(setOperatorProperty).toHaveBeenCalledWith(OP, { model: "gpt-4-turbo", temperature: 0 }); + }); + + it("does nothing without a ready suggestion", () => { + service.applyFix(); + expect(setOperatorProperty).not.toHaveBeenCalled(); + expect(executeWorkflow).not.toHaveBeenCalled(); + expect(state().status).toEqual("idle"); + }); + + it("ends in error, without re-running, when the snippet is no longer in the code", async () => { + stubModel(suggestion({ original_snippet: "a line the user already deleted" })); + await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + + service.applyFix(); + + expect(setOperatorProperty).not.toHaveBeenCalled(); + expect(executeWorkflow).not.toHaveBeenCalled(); + expect(state().status).toEqual("error"); + }); + + it("ends in error when the graph rejects the change", async () => { + setOperatorProperty.mockImplementation(() => { + throw new Error("read-only workflow"); + }); + stubModel(suggestion()); + await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + + service.applyFix(); + + expect(state().status).toEqual("error"); + expect(executeWorkflow).not.toHaveBeenCalled(); + }); + }); + + it("discards to idle and keeps state across re-subscription", async () => { + stubModel(suggestion()); + await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + + // A frame rebuilt by clearResultPanel() re-subscribes and must still see the fix. + expect((await firstValueFrom(service.getState$())).status).toEqual("ready"); + + service.discardFix(); + const after = await firstValueFrom(service.getState$()); + expect(after).toEqual({ operatorId: "", errorMessage: "", errorType: "unsupported", status: "idle" }); + }); +}); diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts new file mode 100644 index 00000000000..e0d68193de0 --- /dev/null +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts @@ -0,0 +1,222 @@ +/** + * 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 { Injectable } from "@angular/core"; +import { BehaviorSubject, Observable } from "rxjs"; +import { createOpenAI } from "@ai-sdk/openai"; +import { generateText, type ModelMessage } from "ai"; +import { AppSettings } from "../../../../common/app-setting"; +import { AuthService } from "../../../../common/service/user/auth.service"; +import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; +import { ExecuteWorkflowService } from "../../../service/execute-workflow/execute-workflow.service"; +import { PortSchema } from "../../../types/workflow-compiling.interface"; +import { buildFixPrompt } from "./ai-fix-prompt"; + +export type FixErrorType = "missing_column" | "type_error" | "null_error" | "model_not_found" | "unsupported"; +export type FixConfidence = "high" | "medium" | "low"; + +export interface SuggestedFix { + type: "code_change" | "property_change"; + original: string; + suggested: string; + explanation: string; + // Rendered as the confidence badge; `fieldName` names the patched property + // (property_change only) so applyFix knows which field to write. + confidence: FixConfidence; + fieldName?: string; +} + +export interface FixState { + operatorId: string; + errorMessage: string; + errorType: FixErrorType; + originalCode?: string; + originalProperties?: object; + suggestedFix?: SuggestedFix; + status: "idle" | "analyzing" | "ready" | "applying" | "applied" | "error"; +} + +// Model id as LiteLLM exposes it (bin/single-node/litellm-config.yaml), not the provider's id. +export const AI_FIXER_MODEL = "claude-haiku-4.5"; +export const AI_FIXER_TIMEOUT_MS = 120_000; +export const UNSUPPORTED_MESSAGE = "This error type is not yet supported for automatic fixing."; + +const IDLE_STATE: FixState = { operatorId: "", errorMessage: "", errorType: "unsupported", status: "idle" }; + +// Each pattern matches a line anywhere inside a traceback, not the whole message. +const PATTERNS: ReadonlyArray<[RegExp, FixErrorType]> = [ + [/KeyError:\s*['"][^'"]+['"]/, "missing_column"], + [/TypeError:\s*unsupported operand/i, "type_error"], + [/ValueError:[^\n]*\bNaN\b|NullPointerException/i, "null_error"], + [/ModelNotFound|\b404\b/i, "model_not_found"], +]; + +export function classifyError(errorMessage: string): FixErrorType { + return PATTERNS.find(([pattern]) => pattern.test(errorMessage))?.[1] ?? "unsupported"; +} + +/** + * Holds the AI-fix loop for one failed operator: classify -> ask the LLM -> apply -> re-run. + * + * The state lives here rather than in the frame component because + * ResultPanelComponent.clearResultPanel() destroys and rebuilds the frames on + * every re-render, which would otherwise discard an in-flight suggestion. + */ +@Injectable({ providedIn: "root" }) +export class AiWorkflowFixerService { + private readonly stateSubject = new BehaviorSubject(IDLE_STATE); + private model: any; + + constructor( + private workflowActionService: WorkflowActionService, + private executeWorkflowService: ExecuteWorkflowService + ) {} + + public getState$(): Observable { + return this.stateSubject.asObservable(); + } + + public async analyzeError( + operatorId: string, + errorMessage: string, + schema: PortSchema | undefined, + code: string | undefined, + properties: Readonly> + ): Promise { + const errorType = classifyError(errorMessage); + const base: FixState = { + operatorId, + errorMessage, + errorType, + originalCode: code, + originalProperties: properties, + status: "analyzing", + }; + + // Out of scope: report it without spending a model call. + if (errorType === "unsupported") { + this.stateSubject.next({ ...base, status: "ready" }); + return; + } + + this.stateSubject.next(base); + try { + const { text } = await this.callModelWithTimeout(buildFixPrompt(errorMessage, code, schema, properties)); + this.stateSubject.next({ ...base, status: "ready", suggestedFix: this.toFix(text, properties) }); + } catch (err) { + console.error("AI workflow fixer: analysis failed", err); + this.stateSubject.next({ ...base, status: "error" }); + } + } + + /** + * Applies the suggestion to the live operator and re-runs the workflow. + * Properties are re-read from the graph so a concurrent edit is not clobbered. + */ + public applyFix(): void { + const current = this.stateSubject.getValue(); + const fix = current.suggestedFix; + if (!fix || current.status !== "ready") { + return; + } + this.stateSubject.next({ ...current, status: "applying" }); + try { + const properties: Record = { + ...this.workflowActionService.getTexeraGraph().getOperator(current.operatorId).operatorProperties, + }; + if (fix.type === "code_change") { + const code = String(properties.code ?? ""); + if (!code.includes(fix.original)) { + throw new Error("the code changed since the suggestion was generated"); + } + properties.code = code.replace(fix.original, fix.suggested); + } else { + properties[String(fix.fieldName)] = fix.suggested; + } + this.workflowActionService.setOperatorProperty(current.operatorId, properties); + this.stateSubject.next({ ...current, status: "applied" }); + // Same empty execution name the operator menu uses for an ad-hoc run. + this.executeWorkflowService.executeWorkflow(""); + } catch (err) { + console.error("AI workflow fixer: apply failed", err); + this.stateSubject.next({ ...current, status: "error" }); + } + } + + public discardFix(): void { + this.stateSubject.next(IDLE_STATE); + } + + // Seam over the `ai` transport: specs spy this instead of mocking the "ai" module, + // which leaks across specs sharing the import and hangs on a real network call. + protected callModel(messages: ModelMessage[], abortSignal?: AbortSignal): Promise<{ text: string }> { + if (!this.model) { + // The /api/chat/* LiteLLM proxy authenticates with the Texera JWT and swaps in + // the master key upstream, so the user's access token is the only credential sent. + this.model = createOpenAI({ + baseURL: new URL(`${AppSettings.getApiEndpoint()}`, document.baseURI).toString(), + apiKey: AuthService.getAccessToken() ?? "", + }).chat(AI_FIXER_MODEL); + } + return generateText({ model: this.model, messages, abortSignal }); + } + + private callModelWithTimeout(prompt: string): Promise<{ text: string }> { + const controller = new AbortController(); + let timer: ReturnType; + const timeout = new Promise((_resolve, reject) => { + timer = setTimeout(() => { + controller.abort(); + reject(new Error("AI fix request timed out")); + }, AI_FIXER_TIMEOUT_MS); + }); + return Promise.race([this.callModel([{ role: "user", content: prompt }], controller.signal), timeout]).finally(() => + clearTimeout(timer) + ); + } + + /** Maps the model's JSON contract onto SuggestedFix, rejecting incomplete replies. */ + private toFix(raw: string, properties: Readonly>): SuggestedFix { + let text = raw.trim(); + const fenced = text.match(/```(?:[a-zA-Z]+)?\s*([\s\S]*?)```/); + if (fenced) { + text = fenced[1].trim(); + } + let parsed: any; + try { + parsed = JSON.parse(text); + } catch (err) { + throw new Error(`model did not return JSON: ${(err as Error).message}`); + } + const { explanation, fix_type, original_snippet, suggested_snippet, confidence } = parsed ?? {}; + if (!explanation || !original_snippet || !suggested_snippet) { + throw new Error("model response is missing a required field"); + } + const isProperty = fix_type === "property_change"; + return { + type: isProperty ? "property_change" : "code_change", + // For a property change the model returns the field name, so show its current value. + original: isProperty ? String(properties[original_snippet] ?? "") : original_snippet, + suggested: suggested_snippet, + explanation, + confidence: confidence === "high" || confidence === "low" ? confidence : "medium", + fieldName: isProperty ? original_snippet : undefined, + }; + } +} diff --git a/frontend/src/app/workspace/component/result-panel/result-panel.component.ts b/frontend/src/app/workspace/component/result-panel/result-panel.component.ts index d548bae4c77..0de7f461c3f 100644 --- a/frontend/src/app/workspace/component/result-panel/result-panel.component.ts +++ b/frontend/src/app/workspace/component/result-panel/result-panel.component.ts @@ -40,6 +40,8 @@ import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; import { isPythonUdf, isSink } from "../../service/workflow-graph/model/workflow-graph"; import { WorkflowVersionService } from "../../../dashboard/service/user/workflow-version/workflow-version.service"; import { ErrorFrameComponent } from "./error-frame/error-frame.component"; +import { AiFixFrameComponent } from "./ai-fix/ai-fix-frame.component"; +import { GuiConfigService } from "../../../common/service/gui-config.service"; import { WorkflowConsoleService } from "../../service/workflow-console/workflow-console.service"; import { NzResizeEvent, NzResizableDirective, NzResizeHandlesComponent } from "ng-zorro-antd/resizable"; import { VisualizationFrameContentComponent } from "../visualization-panel-content/visualization-frame-content.component"; @@ -118,7 +120,8 @@ export class ResultPanelComponent implements OnInit, OnDestroy { private changeDetectorRef: ChangeDetectorRef, private workflowConsoleService: WorkflowConsoleService, private resizeService: PanelResizeService, - private panelService: PanelService + private panelService: PanelService, + private config: GuiConfigService ) { this.width = 0; this.height = Number(localStorage.getItem("result-panel-height")) || this.height; @@ -314,6 +317,15 @@ export class ResultPanelComponent implements OnInit, OnDestroy { component: ErrorFrameComponent, componentInputs: { operatorId }, }); + // The AI tab only offers the analysis: the model is called when the user asks for it, + // never on render, because this method runs again on every re-render of the panel. + // The LiteLLM proxy rejects the call outright when the copilot feature is off. + if (this.config.env.copilotEnabled) { + this.frameComponentConfigs.set("AI Fix", { + component: AiFixFrameComponent, + componentInputs: { operatorId }, + }); + } } displayResult(operatorId: string) { From a79eebf7d34f872128a1b735685d406786c27c24 Mon Sep 17 00:00:00 2001 From: Pedro Date: Sat, 12 Sep 2026 09:56:32 -0600 Subject: [PATCH 02/39] fix(demo-workflows): add required defaultEnv to Python UDF operators PythonUDFV2 declares defaultEnv as a required property, so a workflow whose UDF omits it is rejected by the frontend's schema validation: the canvas marks the operator red and the Run button reads "Invalid Workflow". Every demo workflow hit this on import. Also adds the envName counterpart, which the operator reads when defaultEnv is false. Co-Authored-By: Claude Opus 5 --- demo-workflows/combined.json | 2 + demo-workflows/missing_column.json | 2 + demo-workflows/missing_column_textinput.json | 91 ++++++++++++++++++++ demo-workflows/null_error.json | 2 + demo-workflows/type_error.json | 2 + 5 files changed, 99 insertions(+) create mode 100644 demo-workflows/missing_column_textinput.json diff --git a/demo-workflows/combined.json b/demo-workflows/combined.json index 558baaec2fa..99f450d0bd8 100644 --- a/demo-workflows/combined.json +++ b/demo-workflows/combined.json @@ -34,6 +34,8 @@ "code": "from pytexera import *\nfrom sklearn.preprocessing import StandardScaler\n\n\nclass ProcessTableOperator(UDFTableOperator):\n\n @overrides\n def process_table(self, table: Table, port: int) -> Iterator[Optional[TableLike]]:\n # TWO BUGS ON PURPOSE, to check that only the first one is fixed per round.\n #\n # Bug 1 (raised first): the column is \"vote_average\", not \"vote_avg\".\n # Raises: KeyError: 'vote_avg'\n ratings = table[[\"vote_avg\"]]\n\n # Bug 2 (only reachable once bug 1 is fixed): the ratings still contain\n # missing values, so scaling fails.\n # Raises: ValueError: Input contains NaN.\n ratings.loc[ratings.index % 7 == 0] = float(\"nan\")\n table[\"rating_z\"] = StandardScaler().fit_transform(ratings)\n\n yield table\n", "workers": 1, "retainInputColumns": true, + "defaultEnv": true, + "envName": "", "outputColumns": [ { "attributeName": "rating_z", diff --git a/demo-workflows/missing_column.json b/demo-workflows/missing_column.json index 4eb2d1e6938..006b22d48d4 100644 --- a/demo-workflows/missing_column.json +++ b/demo-workflows/missing_column.json @@ -34,6 +34,8 @@ "code": "from pytexera import *\n\n\nclass ProcessTupleOperator(UDFOperatorV2):\n\n @overrides\n def process_tuple(self, tuple_: Tuple, port: int) -> Iterator[Optional[TupleLike]]:\n # BROKEN ON PURPOSE: the column is called \"vote_count\", not \"vote_counts\".\n # Raises: KeyError: 'vote_counts'\n popularity = tuple_[\"vote_counts\"]\n\n tuple_[\"popularity_bucket\"] = \"high\" if popularity > 1000 else \"low\"\n yield tuple_\n", "workers": 1, "retainInputColumns": true, + "defaultEnv": true, + "envName": "", "outputColumns": [ { "attributeName": "popularity_bucket", diff --git a/demo-workflows/missing_column_textinput.json b/demo-workflows/missing_column_textinput.json new file mode 100644 index 00000000000..46925b9c28c --- /dev/null +++ b/demo-workflows/missing_column_textinput.json @@ -0,0 +1,91 @@ +{ + "operators": [ + { + "operatorID": "TextInput-operator-demo-missing-column", + "operatorType": "TextInput", + "operatorVersion": "N/A", + "operatorProperties": { + "textInput": "Ad Astra\nThe Irishman\nJoker\nParasite\nKnives Out", + "attributeName": "line", + "attributeType": "string" + }, + "inputPorts": [], + "outputPorts": [ + { + "portID": "output-0", + "displayName": "", + "allowMultiInputs": false, + "isDynamicPort": false + } + ], + "showAdvanced": false, + "isDisabled": false, + "customDisplayName": "Movie titles", + "dynamicInputPorts": false, + "dynamicOutputPorts": false, + "viewResult": false + }, + { + "operatorID": "PythonUDFV2-operator-demo-missing-column-textinput", + "operatorType": "PythonUDFV2", + "operatorVersion": "N/A", + "operatorProperties": { + "code": "from pytexera import *\n\n\nclass ProcessTupleOperator(UDFOperatorV2):\n\n @overrides\n def process_tuple(self, tuple_: Tuple, port: int) -> Iterator[Optional[TupleLike]]:\n # BROKEN ON PURPOSE: the input column is \"line\", not \"lines\".\n # Raises: KeyError: 'lines'\n title = tuple_[\"lines\"]\n\n tuple_[\"title_length\"] = len(title)\n yield tuple_\n", + "workers": 1, + "retainInputColumns": true, + "defaultEnv": true, + "envName": "", + "outputColumns": [ + { + "attributeName": "title_length", + "attributeType": "integer" + } + ] + }, + "inputPorts": [ + { + "portID": "input-0", + "displayName": "", + "allowMultiInputs": true, + "isDynamicPort": false, + "dependencies": [] + } + ], + "outputPorts": [ + { + "portID": "output-0", + "displayName": "", + "allowMultiInputs": false, + "isDynamicPort": false + } + ], + "showAdvanced": false, + "isDisabled": false, + "customDisplayName": "Title length (missing column)", + "dynamicInputPorts": true, + "dynamicOutputPorts": true, + "viewResult": true + } + ], + "operatorPositions": { + "TextInput-operator-demo-missing-column": { "x": 80, "y": 200 }, + "PythonUDFV2-operator-demo-missing-column-textinput": { "x": 340, "y": 200 } + }, + "links": [ + { + "linkID": "link-demo-missing-column-textinput", + "source": { + "operatorID": "TextInput-operator-demo-missing-column", + "portID": "output-0" + }, + "target": { + "operatorID": "PythonUDFV2-operator-demo-missing-column-textinput", + "portID": "input-0" + } + } + ], + "commentBoxes": [], + "settings": { + "dataTransferBatchSize": 400 + } +} diff --git a/demo-workflows/null_error.json b/demo-workflows/null_error.json index 31665ac3d5c..05ee6d2543f 100644 --- a/demo-workflows/null_error.json +++ b/demo-workflows/null_error.json @@ -34,6 +34,8 @@ "code": "from pytexera import *\nfrom sklearn.preprocessing import StandardScaler\n\n\nclass ProcessTableOperator(UDFTableOperator):\n\n @overrides\n def process_table(self, table: Table, port: int) -> Iterator[Optional[TableLike]]:\n # Stand-in for a real source with gaps: every 7th rating is missing.\n table.loc[table.index % 7 == 0, \"vote_average\"] = float(\"nan\")\n\n # BROKEN ON PURPOSE: StandardScaler rejects missing values.\n # The fix is a .dropna() (or a fillna) before scaling.\n # Raises: ValueError: Input contains NaN.\n table[\"rating_z\"] = StandardScaler().fit_transform(table[[\"vote_average\"]])\n\n yield table\n", "workers": 1, "retainInputColumns": true, + "defaultEnv": true, + "envName": "", "outputColumns": [ { "attributeName": "rating_z", diff --git a/demo-workflows/type_error.json b/demo-workflows/type_error.json index 3cb12a74fb2..c6a491b8fad 100644 --- a/demo-workflows/type_error.json +++ b/demo-workflows/type_error.json @@ -34,6 +34,8 @@ "code": "from pytexera import *\n\n\nclass ProcessTupleOperator(UDFOperatorV2):\n\n @overrides\n def process_tuple(self, tuple_: Tuple, port: int) -> Iterator[Optional[TupleLike]]:\n # BROKEN ON PURPOSE: \"title\" is a string column and \"vote_count\" is an int,\n # so the subtraction has no valid operands. The fix is an explicit cast.\n # Raises: TypeError: unsupported operand type(s) for -: 'str' and 'int'\n score = tuple_[\"title\"] - tuple_[\"vote_count\"]\n\n tuple_[\"score\"] = score\n yield tuple_\n", "workers": 1, "retainInputColumns": true, + "defaultEnv": true, + "envName": "", "outputColumns": [ { "attributeName": "score", From c286bcc9402cab4d0ce870062ed369844c932a4b Mon Sep 17 00:00:00 2001 From: Pedro Date: Sat, 12 Sep 2026 10:27:13 -0600 Subject: [PATCH 03/39] fix(frontend): register the AI fix tab for Python UDF console errors A Python UDF exception never becomes a WorkflowFatalError: the worker pauses on the exception and the traceback arrives as an ERROR console message while the execution stays Running. The tab was registered only from displayError(), which runs on the Failed and compilation-failed states, so it never appeared for the very case the panel exists to fix. Register it next to the console frame, where that case is already handled, still gated on the copilot feature flag. Co-Authored-By: Claude Opus 5 --- .../result-panel.component.spec.ts | 57 ++++++++++++++++++- .../result-panel/result-panel.component.ts | 16 ++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts b/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts index 339c772b521..0d459ac6eb0 100644 --- a/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts @@ -52,7 +52,9 @@ import { WorkflowConsoleService } from "../../service/workflow-console/workflow- import { CompilationState } from "../../types/workflow-compiling.interface"; import { WorkflowFatalError } from "../../types/workflow-websocket.interface"; import { PYTHON_UDF_V2_OP_TYPE } from "../../service/workflow-graph/model/workflow-graph"; -import { OperatorPredicate } from "../../types/workflow-common.interface"; +import { ConsoleMessage, OperatorPredicate } from "../../types/workflow-common.interface"; +import { AiFixFrameComponent } from "./ai-fix/ai-fix-frame.component"; +import { GuiConfigService } from "../../../common/service/gui-config.service"; describe("ResultPanelComponent", () => { let component: ResultPanelComponent; @@ -111,6 +113,15 @@ describe("ResultPanelComponent", () => { timestamp: { nanos: 0, seconds: 0 }, }); + const consoleMessage = (msgTypeName: string): ConsoleMessage => ({ + workerId: "w1", + timestamp: { nanos: 0, seconds: 0 }, + msgType: { name: msgTypeName }, + source: "udf.py:process_tuple:10", + title: "KeyError: 'lines'", + message: "Traceback (most recent call last):\nKeyError: 'lines'", + }); + it("should create", () => expect(component).toBeTruthy()); it("should show nothing by default", () => { @@ -404,6 +415,50 @@ describe("ResultPanelComponent", () => { expect(errorConfig?.componentInputs).toEqual({ operatorId: "3" }); }); + it("registers the AI fix frame for a console ERROR while the execution is still running", () => { + // A Python UDF exception never becomes a fatal error: the worker pauses and the + // traceback arrives as an ERROR console message with the execution still Running, + // so the Failed branch never fires for the case the AI panel exists to fix. + workflowActionService.addOperator(mockResultPredicate, mockPoint); + const consoleService = TestBed.inject(WorkflowConsoleService); + const config = TestBed.inject(GuiConfigService); + (config.env as any).copilotEnabled = true; + vi.spyOn(workflowActionService.getJointGraphWrapper(), "getCurrentHighlightedOperatorIDs").mockReturnValue(["3"]); + vi.spyOn(consoleService, "getConsoleMessages").mockReturnValue([consoleMessage("ERROR")]); + + component.rerenderResultPanel(); + + const aiConfig = component.frameComponentConfigs.get("AI Fix"); + expect(aiConfig?.component).toBe(AiFixFrameComponent); + expect(aiConfig?.componentInputs).toEqual({ operatorId: "3" }); + }); + + it("registers no AI fix frame when the operator's console holds no ERROR", () => { + workflowActionService.addOperator(mockResultPredicate, mockPoint); + const consoleService = TestBed.inject(WorkflowConsoleService); + const config = TestBed.inject(GuiConfigService); + (config.env as any).copilotEnabled = true; + vi.spyOn(workflowActionService.getJointGraphWrapper(), "getCurrentHighlightedOperatorIDs").mockReturnValue(["3"]); + vi.spyOn(consoleService, "getConsoleMessages").mockReturnValue([consoleMessage("PRINT")]); + + component.rerenderResultPanel(); + + expect(component.frameComponentConfigs.has("AI Fix")).toBe(false); + }); + + it("registers no AI fix frame while the copilot feature is disabled", () => { + workflowActionService.addOperator(mockResultPredicate, mockPoint); + const consoleService = TestBed.inject(WorkflowConsoleService); + const config = TestBed.inject(GuiConfigService); + (config.env as any).copilotEnabled = false; + vi.spyOn(workflowActionService.getJointGraphWrapper(), "getCurrentHighlightedOperatorIDs").mockReturnValue(["3"]); + vi.spyOn(consoleService, "getConsoleMessages").mockReturnValue([consoleMessage("ERROR")]); + + component.rerenderResultPanel(); + + expect(component.frameComponentConfigs.has("AI Fix")).toBe(false); + }); + it("displays a console frame when the selected operator has console messages", () => { workflowActionService.addOperator(mockResultPredicate, mockPoint); const consoleService = TestBed.inject(WorkflowConsoleService); diff --git a/frontend/src/app/workspace/component/result-panel/result-panel.component.ts b/frontend/src/app/workspace/component/result-panel/result-panel.component.ts index 0de7f461c3f..df38ca89f6a 100644 --- a/frontend/src/app/workspace/component/result-panel/result-panel.component.ts +++ b/frontend/src/app/workspace/component/result-panel/result-panel.component.ts @@ -298,9 +298,25 @@ export class ResultPanelComponent implements OnInit, OnDestroy { if (this.workflowConsoleService.hasConsoleMessages(this.currentOperatorId) || isPythonUdf(operator)) { this.displayConsole(this.currentOperatorId, isPythonUdf(operator)); } + // A Python UDF exception never becomes a fatal error: the worker pauses and the + // traceback arrives as an ERROR console message while the execution stays Running, + // so the Failed branch above never fires for the case this panel exists to fix. + if (this.config.env.copilotEnabled && this.hasConsoleError(this.currentOperatorId)) { + this.frameComponentConfigs.set("AI Fix", { + component: AiFixFrameComponent, + componentInputs: { operatorId: this.currentOperatorId }, + }); + } } } + /** True when the operator reported an ERROR console message (e.g. a Python UDF traceback). */ + private hasConsoleError(operatorId: string): boolean { + return (this.workflowConsoleService.getConsoleMessages(operatorId) ?? []).some( + message => message.msgType.name === "ERROR" + ); + } + clearResultPanel(): void { this.frameComponentConfigs.clear(); } From 0eb0a3f60e8769715bbc112aee3ac85af392ae1a Mon Sep 17 00:00:00 2001 From: Pedro Date: Wed, 23 Sep 2026 14:42:07 -0600 Subject: [PATCH 04/39] fix(frontend): guard property fixes against a stale suggestion applyFix() only checked staleness on the code branch: the property branch wrote the suggested value unconditionally, so a model chosen after the analysis was silently overwritten and the workflow re-ran on it. Re-reading the operator's properties preserves unrelated fields but not the one being replaced. Compare the live field with the value captured when the suggestion was generated and refuse when they differ, exactly as the code branch does with its snippet: nothing is written, no re-run, and the panel asks for a fresh analysis. Co-Authored-By: Claude Opus 5 --- .../ai-fix/ai-workflow-fixer.service.spec.ts | 20 +++++++++++++++++++ .../ai-fix/ai-workflow-fixer.service.ts | 9 ++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts index 32ecb63e0b2..f9128e1469c 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts @@ -186,6 +186,26 @@ describe("AiWorkflowFixerService", () => { service.applyFix(); expect(setOperatorProperty).toHaveBeenCalledWith(OP, { model: "gpt-4-turbo", temperature: 0 }); + expect(executeWorkflow).toHaveBeenCalledWith(""); + expect(state().status).toEqual("applied"); + }); + + it("ends in error, without re-running, when the target field changed since the analysis", async () => { + operatorProperties = { model: "gpt-4-turb", temperature: 0 }; + stubModel( + suggestion({ fix_type: "property_change", original_snippet: "model", suggested_snippet: "gpt-4-turbo" }) + ); + await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + + // A coeditor picks a different model after the suggestion was generated: applying the + // stale suggestion would silently discard their choice and re-run on it. + operatorProperties = { model: "claude-haiku-4.5", temperature: 0 }; + + service.applyFix(); + + expect(setOperatorProperty).not.toHaveBeenCalled(); + expect(executeWorkflow).not.toHaveBeenCalled(); + expect(state().status).toEqual("error"); }); it("does nothing without a ready suggestion", () => { diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts index e0d68193de0..14f134b83b5 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts @@ -147,7 +147,14 @@ export class AiWorkflowFixerService { } properties.code = code.replace(fix.original, fix.suggested); } else { - properties[String(fix.fieldName)] = fix.suggested; + const field = String(fix.fieldName); + // Same staleness guard as the code branch: re-reading the properties keeps the + // unrelated fields, but the field being replaced can itself have moved on since + // the suggestion was generated, and overwriting it would discard that newer value. + if (String(properties[field] ?? "") !== fix.original) { + throw new Error(`${field} changed since the suggestion was generated`); + } + properties[field] = fix.suggested; } this.workflowActionService.setOperatorProperty(current.operatorId, properties); this.stateSubject.next({ ...current, status: "applied" }); From 19d21b6be626b208e2025bebe614d3e40a8d1e21 Mon Sep 17 00:00:00 2001 From: Pedro Date: Thu, 24 Sep 2026 08:28:00 -0600 Subject: [PATCH 05/39] fix(frontend): don't report a re-run the deployment will refuse When a warehouse is required and none is selected, ExecuteWorkflowService refuses the run before it starts, so the AI fix panel reporting "re-running" contradicted the toast the user actually saw. The fix is still applied to the operator; the panel now reports that separately and asks for a warehouse. Co-Authored-By: Claude Opus 5 --- .../ai-fix/ai-fix-frame.component.html | 12 ++++++- .../ai-fix/ai-workflow-fixer.service.spec.ts | 34 +++++++++++++++++++ .../ai-fix/ai-workflow-fixer.service.ts | 24 +++++++++++-- 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.html b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.html index 0f63313ab4e..566ee51e8b2 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.html +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.html @@ -28,7 +28,12 @@
+ *ngIf=" + state.status === 'idle' || + state.status === 'applied' || + state.status === 'applied_without_run' || + state.status === 'error' + ">
-
+
Date: Thu, 22 Oct 2026 16:57:38 -0600 Subject: [PATCH 27/39] fix(frontend): refuse a bad field name before it reaches the diff Found by self-audit, same class as the invented-snippet finding. `original` for a property change is the field's CURRENT value, read so the diff has a "before". Naming a field the operator does not have rendered an empty one, and naming `code` rendered the entire UDF source as a single unwrapped line in a `white-space: pre` box. Apply refused both afterwards, which is too late: the panel had already shown it. The analysis rejects both now, so no suggestion that cannot be written reaches the screen. The apply-side check stays as the second barrier, and is now reachable only when a field present at the analysis is gone by the time Apply re-reads the properties. Its test asserted exactly that -- and passed with the check deleted, because the staleness guard covered the case on its own: an absent field reads as "" and no longer matched the captured original. With the field empty at the analysis both read "" and only the presence check can refuse, so the test now fails without it. Co-Authored-By: Claude Opus 5 --- .../ai-fix/ai-workflow-fixer.service.spec.ts | 36 ++++++++++++++++--- .../ai-fix/ai-workflow-fixer.service.ts | 10 ++++++ 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts index af051f29d94..6504d1301eb 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts @@ -865,25 +865,51 @@ describe("AiWorkflowFixerService", () => { // real setting stays wrong. operatorProperties = { modelId: "gpt-4-turb", workers: 1 }; stubModel(suggestion({ fix_type: "property_change", original_snippet: "modelName", suggested_snippet: "Qwen" })); + + await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + + expect(state().status).toEqual("error"); + await service.applyFix(); + expect(setOperatorProperty).not.toHaveBeenCalled(); + }); + + it("refuses a field that disappeared between the analysis and the write", async () => { + // The analysis rejects a field the operator does not have, so this is the only way the + // apply-side check is still reachable: a coeditor retypes the operator while the + // suggestion is on screen, and the field it names is gone by the time Apply re-reads + // the properties. Writing then would create a dead key and leave the real setting wrong. + // + // The field is empty rather than set, because the staleness guard would otherwise cover + // this on its own: an absent field reads as "" and would no longer match the captured + // original. With both reading "", only the presence check can refuse. + operatorProperties = { modelId: "", workers: 1 }; + stubModel( + suggestion({ fix_type: "property_change", original_snippet: "modelId", suggested_snippet: "gpt-4-turbo" }) + ); await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + expect(state().status).toEqual("ready"); + operatorProperties = { workers: 1 }; await service.applyFix(); expect(setOperatorProperty).not.toHaveBeenCalled(); + expect(executeWorkflow).not.toHaveBeenCalled(); expect(state().status).toEqual("apply_failed"); }); - it("never rewrites the UDF source through a property change", async () => { - // Naming "code" would hand the whole operator source to the property branch, which - // shows a one-line diff and has no snippet guard. + it("never puts the UDF source in the diff through a property change", async () => { + // Naming "code" hands the whole operator source to the property branch, which reads + // the field's current value as the diff's "before". Refusing only at Apply still let + // the panel render the entire source first, as one unwrapped line. operatorProperties = { code: "print(1)", workers: 1 }; stubModel(suggestion({ fix_type: "property_change", original_snippet: "code", suggested_snippet: "print(2)" })); + await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + expect(state().status).toEqual("error"); + expect(state().suggestedFix).toBeUndefined(); await service.applyFix(); - expect(setOperatorProperty).not.toHaveBeenCalled(); - expect(state().status).toEqual("apply_failed"); }); it("keeps a numeric setting numeric", async () => { diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts index a2a2efa9c9a..2d32ca40bb0 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts @@ -573,6 +573,16 @@ export class AiWorkflowFixerService { } } const isProperty = fix_type === "property_change"; + // Checked here, not only at Apply: `original` for a property change is the field's + // CURRENT value, so an unknown name renders an empty "before" and `code` renders the + // entire UDF source as one unwrapped diff line. A suggestion the write would refuse + // should never reach the screen in the first place. + if ( + isProperty && + (original_snippet === "code" || !Object.prototype.hasOwnProperty.call(properties, original_snippet)) + ) { + throw new Error(`${original_snippet} is not a configuration field this fix may change`); + } return { type: isProperty ? "property_change" : "code_change", // For a property change the model returns the field name, so show its current value. From 2057c038d6ed8440c6c697b3c9a9809b71034ba2 Mon Sep 17 00:00:00 2001 From: Pedro Date: Thu, 22 Oct 2026 18:14:06 -0600 Subject: [PATCH 28/39] docs(frontend): put the execution-state comments back on their own sets LIVE_STATES was inserted above TERMINAL_STATES and absorbed its comment, so one set carried a description of the other and the other carried none. TERMINAL_STATES also now says why `Uninitialized` is absent from it although updateWorkflowActionLock counts it as a run that is over: a kill lands there, and a kill is how Apply clears the lock itself, so resetting on it would throw away a suggestion that is still the right one for the error on screen. Co-Authored-By: Claude Opus 5 --- .../result-panel/ai-fix/ai-workflow-fixer.service.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts index 2d32ca40bb0..6244fede9d4 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts @@ -96,8 +96,7 @@ export const AI_FIXER_MODEL = "claude-haiku-4.5"; export const AI_FIXER_TIMEOUT_MS = 120_000; export const UNLOCK_TIMEOUT_MS = 15_000; -// A run that reached one of these is over, so anything the panel still shows predates it. -// A run in one of these is live enough to be worth stopping; the lock coming from anything +// A run in one of these is live enough to be worth stopping; a lock coming from anything // else is not something killing an execution can clear. const LIVE_STATES: ReadonlySet = new Set([ ExecutionState.Initializing, @@ -108,12 +107,17 @@ const LIVE_STATES: ReadonlySet = new Set([ ExecutionState.Recovering, ]); +// A run that reached one of these is over, so anything the panel still shows predates it. +// `Uninitialized` is deliberately absent even though updateWorkflowActionLock treats it as +// over: a kill lands there, and a kill is how Apply itself clears the lock, so resetting on +// it would throw away a suggestion that is still the right one for the error on screen. const TERMINAL_STATES: ReadonlySet = new Set([ ExecutionState.Completed, ExecutionState.Failed, ExecutionState.Killed, ExecutionState.Terminated, ]); + export const UNSUPPORTED_MESSAGE = "This error type is not yet supported for automatic fixing."; export const IDLE_STATE: FixState = { operatorId: "", status: "idle" }; From 711b3280405ec3815ece6a0853b6b04a64dda50b Mon Sep 17 00:00:00 2001 From: Pedro Date: Thu, 22 Oct 2026 18:38:45 -0600 Subject: [PATCH 29/39] fix(frontend): count the snippet at Apply, not just find it Found by a full review of the branch, which is the pass Copilot's run did not finish. The occurrence count behind the "changes N places" badge is taken at the analysis, and Apply replaces every match while checking only that the snippet is still present. So a copy added or removed in between made the write touch a different number of lines than the panel promised -- and the badge is hidden at one occurrence, so gaining a second copy meant an extra edit with no warning at all. Reachable because a kill lifts the modification lock without resetting the panel: `Uninitialized` is deliberately not a terminal state, since that is where Apply's own kill lands. Stop the run with the suggestion on screen and the UDF is editable under it. It refuses in both directions. A count that dropped is not an oversight in the comparison: the badge said two places, so that is the edit the user approved, and patching whichever copy is left is a different change from the one on screen. Co-Authored-By: Claude Opus 5 --- .../ai-fix/ai-workflow-fixer.service.spec.ts | 37 +++++++++++++++++++ .../ai-fix/ai-workflow-fixer.service.ts | 17 ++++++++- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts index 6504d1301eb..64bdcb73809 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts @@ -615,6 +615,43 @@ describe("AiWorkflowFixerService", () => { expect(state().status).toEqual("idle"); }); + it("refuses when the snippet now appears fewer times than the panel promised", async () => { + // Deliberate, not an oversight in the comparison: the badge said two places, so the + // user approved an edit to two. Patching the one that is left is a different change + // from the one on screen, and the diff for it is one model call away. + const twice = CODE + " yield t['email']\n"; + stubModel(suggestion()); + await service.analyzeError(OP, KEY_ERROR, SCHEMA, twice, { ...operatorProperties, code: twice }); + expect(state().suggestedFix?.occurrences).toEqual(2); + + operatorProperties.code = CODE; + + await service.applyFix(); + + expect(setOperatorProperty).not.toHaveBeenCalled(); + expect(state().applyError).toContain("1 place instead of 2 places"); + }); + + it("refuses when the snippet now appears more times than the panel promised", async () => { + // The occurrence count is captured at the analysis and drives the "changes N places" + // badge, which stays hidden at one. Apply only checked that the snippet was still + // THERE, so a copy added in between was patched too -- an edit the user was never + // warned about. Reachable because a kill lifts the modification lock without + // resetting the panel: `Uninitialized` is deliberately not a terminal state. + stubModel(suggestion()); + await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + expect(state().suggestedFix?.occurrences).toEqual(1); + + operatorProperties.code = CODE + " yield t['email']\n"; + + await service.applyFix(); + + expect(setOperatorProperty).not.toHaveBeenCalled(); + expect(executeWorkflow).not.toHaveBeenCalled(); + expect(state().status).toEqual("apply_failed"); + expect(state().applyError).toContain("2 places"); + }); + it("reports an apply failure, without re-running, when the snippet is no longer in the code", async () => { stubModel(suggestion()); await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts index 6244fede9d4..5f672d9b96f 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts @@ -368,10 +368,23 @@ export class AiWorkflowFixerService { }; if (fix.type === "code_change") { const code = String(properties.code ?? ""); - if (!code.includes(fix.original)) { + const patched = replaceAllIndented(code, fix.original, fix.suggested); + if (patched.count === 0) { throw new Error("the code changed since the suggestion was generated"); } - properties.code = replaceAllIndented(code, fix.original, fix.suggested).code; + // Counted, not just found. The count behind the "changes N places" badge was taken at + // the analysis, and this replaces every match -- so a copy added or removed in between + // means the write touches a different number of lines than the panel promised. A kill + // lifts the modification lock without resetting the panel, so the user can edit the + // UDF with the suggestion still on screen. + if (fix.occurrences !== undefined && patched.count !== fix.occurrences) { + const places = (n: number) => `${n} place${n === 1 ? "" : "s"}`; + throw new Error( + `the snippet is now in ${places(patched.count)} instead of ${places(fix.occurrences)}, ` + + "so the fix was not applied" + ); + } + properties.code = patched.code; } else { // Only the model-not-found case asks for a configuration change; a property fix for // any other error is the model leaving the four shapes this panel supports. From 68c65b1adbcf653e30b058626fec3a8042198de9 Mon Sep 17 00:00:00 2001 From: Pedro Date: Fri, 23 Oct 2026 10:03:03 -0600 Subject: [PATCH 30/39] fix(frontend): key the frame tabs by their config, not by their name @kunwp1 on the last round: keying by name also keeps every other frame alive across re-renders and operator switches, because NgComponentOutlet only recreates a component when its type changes. That is broader than the problem. The config object is the right key. A re-render re-`set`s each entry with a fresh object, so those frames are recreated exactly as they were before this PR, while syncAiFixFrame touching only its own entry leaves the others' identity -- and so their components -- untouched. It also settles what I could not test. The DOM assertion was out of reach because rendering the frames needs the panel open, and that mounts components jsdom cannot draw. But "toggles only the AI Fix entry on a console update" already asserts that the Console config object survives by identity, which is exactly what the trackBy now keys on. Co-Authored-By: Claude Opus 5 --- .../result-panel/result-panel.component.html | 2 +- .../result-panel/result-panel.component.ts | 19 +++++++++++++------ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/result-panel.component.html b/frontend/src/app/workspace/component/result-panel/result-panel.component.html index 37e01d01f58..0ad9f54eb9a 100644 --- a/frontend/src/app/workspace/component/result-panel/result-panel.component.html +++ b/frontend/src/app/workspace/component/result-panel/result-panel.component.html @@ -103,7 +103,7 @@

No results available to display.
-
+
Date: Fri, 23 Oct 2026 10:03:21 -0600 Subject: [PATCH 31/39] fix(frontend): validate the fix before the kill, not only after it @kunwp1: every refusal in applyFix ran after ensureModifiable(), which stops the paused execution to clear the modification lock. So a fix that was never going to be written cost the user their run -- and the test for it shows the rest of the cost: fifteen seconds of unlock wait before the refusal even reached the panel. The checks and the patch move into patchedProperties(), which reads only the graph and returns a fresh object. It runs as a pre-flight before the kill, and again after it, since the operator can move during the wait and the pre-flight result describes the operator as it was before. The other half of the same comment: refusing a configuration change for anything but model_not_found was Apply-only, so the panel showed a diff and then turned it down -- which is what the comment in toFix says must not happen. The analysis refuses it now, and the check stays in patchedProperties as the second barrier. Co-Authored-By: Claude Opus 5 --- .../ai-fix/ai-workflow-fixer.service.spec.ts | 23 +++- .../ai-fix/ai-workflow-fixer.service.ts | 116 +++++++++++------- 2 files changed, 92 insertions(+), 47 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts index 64bdcb73809..444113e811c 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts @@ -615,6 +615,23 @@ describe("AiWorkflowFixerService", () => { expect(state().status).toEqual("idle"); }); + it("does not stop the user's run for a fix it was going to refuse anyway", async () => { + // Every refusal used to run after ensureModifiable(), which kills the paused execution + // to clear the modification lock. So a fix that could never be written cost the user + // their run: the panel reported the refusal over a workflow it had just stopped. + modificationEnabled = false; + stubModel(suggestion()); + await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + + operatorProperties.code = "class ProcessTupleOperator(UDFOperatorV2):\n pass\n"; + + await service.applyFix(); + + expect(killWorkflow).not.toHaveBeenCalled(); + expect(setOperatorProperty).not.toHaveBeenCalled(); + expect(state().status).toEqual("apply_failed"); + }); + it("refuses when the snippet now appears fewer times than the panel promised", async () => { // Deliberate, not an oversight in the comparison: the badge said two places, so the // user approved an edit to two. Patching the one that is left is a different change @@ -858,8 +875,12 @@ describe("AiWorkflowFixerService", () => { ); await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); - await service.applyFix(); + // Refused by the analysis, not by Apply: a diff the write will never honour should + // never be on screen, and refusing at the click costs the paused run as well. + expect(state().status).toEqual("error"); + expect(state().suggestedFix).toBeUndefined(); + await service.applyFix(); expect(setOperatorProperty).not.toHaveBeenCalled(); }); diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts index 5f672d9b96f..f21b6873ebf 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts @@ -315,6 +315,12 @@ export class AiWorkflowFixerService { return; } const suggestedFix = this.toFix(text, visible); + // The prompt asks for a configuration change in exactly one case, so a property fix + // for any other error is the model going off script. Refused here rather than only at + // Apply: the panel must not show a diff the write will never honour. + if (suggestedFix.type === "property_change" && errorType !== "model_not_found") { + throw new Error("a configuration change is not the fix for this error"); + } // Counted here so the panel can say it before Apply: a short snippet matched as plain // text can appear in lines the user never saw in the diff. if (suggestedFix.type === "code_change") { @@ -359,56 +365,17 @@ export class AiWorkflowFixerService { this.stateSubject.next({ ...current, status: "applying" }); try { this.assertSameWorkflow(current); + // Run before the kill as well as after it. ensureModifiable stops the paused execution + // to clear the modification lock, so a fix that was going to be refused anyway cost the + // user their run -- and then the unlock wait before the refusal even reached the panel. + this.patchedProperties(current, fix); await this.ensureModifiable(); // Checked again: ensureModifiable can wait seconds for a kill to clear the lock, and // the user is free to open another workflow while it does. this.assertSameWorkflow(current); - const properties: Record = { - ...this.workflowActionService.getTexeraGraph().getOperator(current.operatorId).operatorProperties, - }; - if (fix.type === "code_change") { - const code = String(properties.code ?? ""); - const patched = replaceAllIndented(code, fix.original, fix.suggested); - if (patched.count === 0) { - throw new Error("the code changed since the suggestion was generated"); - } - // Counted, not just found. The count behind the "changes N places" badge was taken at - // the analysis, and this replaces every match -- so a copy added or removed in between - // means the write touches a different number of lines than the panel promised. A kill - // lifts the modification lock without resetting the panel, so the user can edit the - // UDF with the suggestion still on screen. - if (fix.occurrences !== undefined && patched.count !== fix.occurrences) { - const places = (n: number) => `${n} place${n === 1 ? "" : "s"}`; - throw new Error( - `the snippet is now in ${places(patched.count)} instead of ${places(fix.occurrences)}, ` + - "so the fix was not applied" - ); - } - properties.code = patched.code; - } else { - // Only the model-not-found case asks for a configuration change; a property fix for - // any other error is the model leaving the four shapes this panel supports. - if (current.errorType !== "model_not_found") { - throw new Error("a configuration change is not the fix for this error"); - } - const field = String(fix.fieldName); - // Checked against the fields the model was actually shown, not the raw properties: - // those still hold the credentials withoutSecrets kept out of the prompt, so a - // guessed `hfApiToken` would otherwise pass here and be overwritten. - const visible = withoutSecrets(properties, this.schemaProperties(current.operatorId)); - if (field === "code" || !Object.prototype.hasOwnProperty.call(visible, field)) { - throw new Error(`${field} is not a configuration field this fix may change`); - } - // Same staleness guard as the code branch: re-reading the properties keeps the - // unrelated fields, but the field being replaced can itself have moved on since - // the suggestion was generated, and overwriting it would discard that newer value. - if (String(properties[field] ?? "") !== fix.original) { - throw new Error(`${field} changed since the suggestion was generated`); - } - // The model always replies with a string; writing it raw would turn a numeric or - // boolean setting into one, which the operator then rejects at validation. - properties[field] = coerceToFieldType(properties[field], fix.suggested); - } + // Re-read and re-validated rather than reusing the pre-flight result, which describes + // the operator as it was before that wait. + const properties = this.patchedProperties(current, fix); this.workflowActionService.setOperatorProperty(current.operatorId, properties); // Asked rather than predicted: mirroring ExecuteWorkflowService's refusal conditions // here means every one it gains has to be repeated, and missing one reports @@ -435,6 +402,63 @@ export class AiWorkflowFixerService { } } + /** + * The operator's properties with the fix written into them, or a refusal. + * + * Reads only the graph and returns a fresh object, so it is safe to run twice: once as a + * pre-flight before ensureModifiable, so a refusal does not cost the user the execution + * that call kills, and once after it, because the operator can move during the wait. + */ + private patchedProperties(current: FixState, fix: SuggestedFix): Record { + const properties: Record = { + ...this.workflowActionService.getTexeraGraph().getOperator(current.operatorId).operatorProperties, + }; + if (fix.type === "code_change") { + const code = String(properties.code ?? ""); + const patched = replaceAllIndented(code, fix.original, fix.suggested); + if (patched.count === 0) { + throw new Error("the code changed since the suggestion was generated"); + } + // Counted, not just found. The count behind the "changes N places" badge was taken at + // the analysis, and this replaces every match -- so a copy added or removed in between + // means the write touches a different number of lines than the panel promised. A kill + // lifts the modification lock without resetting the panel, so the user can edit the + // UDF with the suggestion still on screen. + if (fix.occurrences !== undefined && patched.count !== fix.occurrences) { + const places = (n: number) => `${n} place${n === 1 ? "" : "s"}`; + throw new Error( + `the snippet is now in ${places(patched.count)} instead of ${places(fix.occurrences)}, ` + + "so the fix was not applied" + ); + } + properties.code = patched.code; + } else { + // Only the model-not-found case asks for a configuration change; a property fix for + // any other error is the model leaving the four shapes this panel supports. + if (current.errorType !== "model_not_found") { + throw new Error("a configuration change is not the fix for this error"); + } + const field = String(fix.fieldName); + // Checked against the fields the model was actually shown, not the raw properties: + // those still hold the credentials withoutSecrets kept out of the prompt, so a + // guessed `hfApiToken` would otherwise pass here and be overwritten. + const visible = withoutSecrets(properties, this.schemaProperties(current.operatorId)); + if (field === "code" || !Object.prototype.hasOwnProperty.call(visible, field)) { + throw new Error(`${field} is not a configuration field this fix may change`); + } + // Same staleness guard as the code branch: re-reading the properties keeps the + // unrelated fields, but the field being replaced can itself have moved on since + // the suggestion was generated, and overwriting it would discard that newer value. + if (String(properties[field] ?? "") !== fix.original) { + throw new Error(`${field} changed since the suggestion was generated`); + } + // The model always replies with a string; writing it raw would turn a numeric or + // boolean setting into one, which the operator then rejects at validation. + properties[field] = coerceToFieldType(properties[field], fix.suggested); + } + return properties; + } + public discardFix(): void { this.reset(); } From 114b5822c6966f3794aba122e59ec1534b37ea1f Mon Sep 17 00:00:00 2001 From: Pedro Date: Fri, 23 Oct 2026 10:16:19 -0600 Subject: [PATCH 32/39] fix(frontend): classify on the exception the source already separated @kunwp1 was right that JVM errors still did not classify, and the reason is worse than the pattern: on Java 9+ the JDK's own frames carry a module prefix (`java.base/java.lang.Thread.run(Thread.java:840)`) and native ones read `(Native Method)`. mkConsoleMessage sends `getStackTrace.mkString` with no `at ` prefix either, so a real worker trace ends on a frame nothing here recognised -- the backward scan returned that frame and `NullPointerException` classified as unsupported. The fix in 7ee6339 did not work in production. So stop deducing it. Both sources hand the exception over on its own: mkConsoleMessage puts `err.toString` in `title`, the Python worker puts `format_exception[-1]` there, and a fatal error carries `evt.e.toString` in `message` with the stack in `details`. analyzeError now takes both parts -- `header` to classify, `full` for the prompt -- and FRAME_LINE and finalExceptionLine are gone. That also retires the chained-traceback logic rather than reimplementing it: `format_exception[-1]` is already the exception that stopped the worker, which is what scanning backwards was trying to recover. The regression test moves to the frame spec, where the title is actually extracted, since that is where this can break again. Two service tests that fed a whole traceback to classifyError went with the heuristic -- passing a blob now only proves the pattern matches somewhere in it, not that the header was read. Co-Authored-By: Claude Opus 5 --- .../ai-fix/ai-fix-frame.component.spec.ts | 34 ++-- .../ai-fix/ai-fix-frame.component.ts | 32 ++-- .../ai-fix/ai-workflow-fixer.service.spec.ts | 173 +++++++++--------- .../ai-fix/ai-workflow-fixer.service.ts | 62 +++---- 4 files changed, 157 insertions(+), 144 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.spec.ts index 5ae7604e172..b8ec74ad659 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.spec.ts @@ -157,7 +157,8 @@ describe("AiFixFrameComponent", () => { expect(analyzeError).toHaveBeenCalledWith( "op1", - "KeyError: 'email'\nTraceback ...", + // A fatal error already separates them: `message` is the exception, `details` the stack. + { header: "KeyError: 'email'", full: "KeyError: 'email'\nTraceback ..." }, // Every input port, not just port 0: a UDF downstream of a join has more than one. { "0_false": [{ attributeName: "user_email", attributeType: "string" }] }, "yield t['email']", @@ -173,24 +174,33 @@ describe("AiFixFrameComponent", () => { query(".ai-fix-analyze").nativeElement.click(); - // Title first, then the body: the exception is in `title` for both workers, and for a - // JVM operator it is the only place it appears. - expect(analyzeError.mock.calls[0][1]).toEqual( - "KeyError: 'email'\nTraceback (most recent call last):\nKeyError: 'email'" - ); + // The exception is handed over on its own, for classification, and again inside the + // full text, which is what the model is shown. + expect(analyzeError.mock.calls[0][1]).toEqual({ + header: "KeyError: 'email'", + full: "KeyError: 'email'\nTraceback (most recent call last):\nKeyError: 'email'", + }); }); - it("sends the exception even when it only lives in the title", () => { - // ErrorUtils.mkConsoleMessage puts `err.toString()` in title and only the stack - // frames in message, so reading message alone left JVM operators sending the model - // a bare stack trace -- and the NullPointerException pattern could never match. + it("classifies a JVM error on its title, never on the frames below it", () => { + // ErrorUtils.mkConsoleMessage puts `err.toString()` in title and only + // `getStackTrace.mkString` in message -- no `at ` prefix, and on Java 9+ the JDK's own + // frames carry a module name. A real worker trace ends on one of those, so anything + // that tried to find the exception inside the joined text picked the frame and left + // NullPointerException unclassifiable. The title is handed over directly instead. + const frames = [ + "org.apache.texera.udf.Op.process(Op.scala:31)", + "java.base/java.lang.Thread.run(Thread.java:840)", + ].join("\n"); getConsoleMessages.mockReturnValue([ - consoleMessage("ERROR", "org.apache.texera.Foo.run(Foo.scala:12)", "java.lang.NullPointerException: boom"), + consoleMessage("ERROR", frames, "java.lang.NullPointerException: value is null"), ]); query(".ai-fix-analyze").nativeElement.click(); - expect(analyzeError.mock.calls[0][1]).toContain("java.lang.NullPointerException: boom"); + const passed = analyzeError.mock.calls[0][1]; + expect(passed.header).toEqual("java.lang.NullPointerException: value is null"); + expect(passed.full).toContain("java.base/java.lang.Thread.run"); }); it("does nothing when the operator has no error at all", () => { diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.ts index f2a1c388483..8cc3b7ef93d 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.ts @@ -36,6 +36,7 @@ import { FixConfidence, FixState, IDLE_STATE, + OperatorError, UNSUPPORTED_MESSAGE, } from "./ai-workflow-fixer.service"; @@ -104,14 +105,14 @@ export class AiFixFrameComponent { if (!operatorId) { return; } - const errorMessage = this.rawErrorMessage(operatorId); - if (!errorMessage) { + const error = this.operatorError(operatorId); + if (!error) { return; } const properties = this.workflowActionService.getTexeraGraph().getOperator(operatorId).operatorProperties ?? {}; void this.aiWorkflowFixerService.analyzeError( operatorId, - errorMessage, + error, this.workflowCompilingService.getOperatorInputSchemaMap(operatorId), properties.code as string | undefined, properties @@ -141,16 +142,17 @@ export class AiFixFrameComponent { } /** - * The untrimmed error text for this operator. + * This operator's error, split the way its source already splits it. * - * Engine failures arrive as fatal errors, but a Python UDF exception never - * becomes one: it is delivered as an ERROR console message whose `message` - * holds the whole traceback, which is what the classifier needs. + * Engine failures arrive as fatal errors, but a Python UDF exception never becomes one: it + * is delivered as an ERROR console message while the execution stays `Running`. Either way + * the exception is in a field of its own -- `message` on a fatal error, `title` on a console + * message -- so the classifier is handed it directly instead of searching the joined text. */ - private rawErrorMessage(operatorId: string): string | undefined { + private operatorError(operatorId: string): OperatorError | undefined { const fatal = this.executeWorkflowService.getErrorMessages().find(err => err.operatorId === operatorId); if (fatal) { - return [fatal.message, fatal.details].filter(part => part).join("\n"); + return { header: fatal.message, full: [fatal.message, fatal.details].filter(part => part).join("\n") }; } const consoleError = (this.workflowConsoleService.getConsoleMessages(operatorId) ?? []) .filter(msg => msg.msgType.name === "ERROR") @@ -158,11 +160,11 @@ export class AiFixFrameComponent { if (!consoleError) { return undefined; } - // Both parts, title first. ErrorUtils.mkConsoleMessage puts the exception in `title` and - // only the stack frames in `message`, so reading `message` alone sent the model a bare - // stack trace for every JVM operator -- and left NullPointerException unmatchable. The - // Python worker puts the final exception line in `title` too, so this order gives the - // classifier the exception first in both cases. - return [consoleError.title, consoleError.message].filter(part => part).join("\n"); + // `message` holds only the frames for a JVM operator and the whole traceback for a Python + // one, so the model is sent both parts with the exception first in either case. + return { + header: consoleError.title, + full: [consoleError.title, consoleError.message].filter(part => part).join("\n"), + }; } } diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts index 444113e811c..68425ef002b 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts @@ -25,6 +25,7 @@ import { coerceToFieldType, FixState, replaceAllIndented, + OperatorError, } from "./ai-workflow-fixer.service"; import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; import { ExecuteWorkflowService } from "../../../service/execute-workflow/execute-workflow.service"; @@ -50,6 +51,13 @@ const CODE = const KEY_ERROR = "Traceback (most recent call last):\n File \"udf.py\", line 3\n yield t['email']\nKeyError: 'email'"; +const QUOTE = String.fromCharCode(39); + +// Both sources hand the exception over on its own, so a test supplies it the same way: +// `header` is what classification reads, `full` is what the model is shown. +const err = (header: string, full: string = header): OperatorError => ({ header, full }); +const KEY_ERROR_EVENT = err("KeyError: " + QUOTE + "email" + QUOTE, KEY_ERROR); + describe("AiWorkflowFixerService", () => { let service: AiWorkflowFixerService; let setOperatorProperty: ReturnType; @@ -149,7 +157,7 @@ describe("AiWorkflowFixerService", () => { describe("classifyError", () => { it("detects each supported pattern", () => { - expect(classifyError(KEY_ERROR)).toEqual("missing_column"); + expect(classifyError(KEY_ERROR_EVENT.header)).toEqual("missing_column"); expect(classifyError("TypeError: unsupported operand type(s) for *: 'str' and 'int'")).toEqual("type_error"); expect(classifyError("ValueError: input contains NaN")).toEqual("null_error"); expect(classifyError("java.lang.NullPointerException: boom")).toEqual("null_error"); @@ -174,7 +182,7 @@ describe("AiWorkflowFixerService", () => { const seen: string[] = []; service.getState$().subscribe(s => seen.push(s.status)); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); expect(seen).toEqual(["idle", "analyzing", "ready"]); expect(state().errorType).toEqual("missing_column"); @@ -197,7 +205,7 @@ describe("AiWorkflowFixerService", () => { // happened. stubModel(suggestion({ original_snippet: "yield t['address']" })); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); expect(state().status).toEqual("error"); expect(state().suggestedFix).toBeUndefined(); @@ -205,7 +213,7 @@ describe("AiWorkflowFixerService", () => { it("tolerates a fenced JSON response even though the prompt forbids markdown", async () => { stubModel("```json\n" + suggestion() + "\n```"); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); expect(state().status).toEqual("ready"); }); @@ -215,7 +223,7 @@ describe("AiWorkflowFixerService", () => { suggestion({ fix_type: "property_change", original_snippet: "model", suggested_snippet: "gpt-4-turbo" }) ); - await service.analyzeError(OP, "Error code: 404 - model not found", SCHEMA, undefined, operatorProperties); + await service.analyzeError(OP, err("Error code: 404 - model not found"), SCHEMA, undefined, operatorProperties); expect(state().suggestedFix?.fieldName).toEqual("model"); expect(state().suggestedFix?.original).toEqual("gpt-4-turb"); @@ -224,7 +232,7 @@ describe("AiWorkflowFixerService", () => { it("never calls the model for an out-of-scope error", async () => { const callModel = vi.spyOn(service as any, "callModel"); - await service.analyzeError(OP, "ZeroDivisionError: division by zero", SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, err("ZeroDivisionError: division by zero"), SCHEMA, CODE, operatorProperties); expect(callModel).not.toHaveBeenCalled(); expect(state().status).toEqual("unsupported"); @@ -233,15 +241,15 @@ describe("AiWorkflowFixerService", () => { it("ends in error on transport failure, malformed JSON, or a missing field", async () => { vi.spyOn(service as any, "callModel").mockRejectedValue(new Error("502")); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); expect(state().status).toEqual("error"); stubModel("I cannot help with that."); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); expect(state().status).toEqual("error"); stubModel(JSON.stringify({ explanation: "e", fix_type: "code_change", confidence: "low" })); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); expect(state().status).toEqual("error"); }); }); @@ -256,8 +264,8 @@ describe("AiWorkflowFixerService", () => { .mockReturnValueOnce(slowFirst) .mockResolvedValueOnce({ text: suggestion({ explanation: "for the second operator" }) }); - const first = service.analyzeError("op-first", KEY_ERROR, SCHEMA, CODE, operatorProperties); - await service.analyzeError("op-second", KEY_ERROR, SCHEMA, CODE, operatorProperties); + const first = service.analyzeError("op-first", KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError("op-second", KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); expect(state().operatorId).toEqual("op-second"); resolveFirst({ text: suggestion({ explanation: "for the first operator" }) }); @@ -274,7 +282,7 @@ describe("AiWorkflowFixerService", () => { operatorProperties = { hfApiToken: "hf_live_abc", password: "hunter2", modelId: "gpt-4-turb" }; const callModel = vi.spyOn(service as any, "callModel").mockResolvedValue({ text: suggestion() }); - await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); const prompt = JSON.stringify(callModel.mock.calls[0][0]); expect(prompt).not.toContain("hf_live_abc"); @@ -283,47 +291,40 @@ describe("AiWorkflowFixerService", () => { }); describe("classifying a real traceback", () => { - it("reads the exception at the end, not the frames above it", () => { - // The frame lines carry paths and line numbers that look like patterns on their own. - const traceback = [ - "Traceback (most recent call last):", - ' File "/usr/lib/python3/sklearn/model_selection/_split.py", line 404, in split', - " n = len(indices) // 0", - "ZeroDivisionError: integer division or modulo by zero", - ].join("\n"); - - expect(classifyError(traceback)).toEqual("unsupported"); + it("classifies a JVM exception whose frames no pattern could recognise", () => { + // mkConsoleMessage puts `err.toString` in `title` and only `getStackTrace.mkString` in + // `message`, so the frames arrive with no `at ` prefix -- and on Java 9+ the JDK's own + // carry a module name. A real worker trace ends on exactly such a frame, which is why + // deducing the exception from the joined text classified the frame instead. + expect(classifyError("java.lang.NullPointerException: value is null")).toEqual("null_error"); }); it("classifies a chained traceback on the exception that stopped the worker", () => { - const chained = [ - "Traceback (most recent call last):", - "KeyError: " + String.fromCharCode(39) + "email" + String.fromCharCode(39), - "", - "During handling of the above exception, another exception occurred:", - "", - "Traceback (most recent call last):", - "ZeroDivisionError: integer division or modulo by zero", - ].join("\n"); - - expect(classifyError(chained)).toEqual("unsupported"); + // Nothing here has to unpick the chain: the Python worker sets `title` from + // `format_exception[-1]`, which is already the final exception. + expect(classifyError("ZeroDivisionError: integer division or modulo by zero")).toEqual("unsupported"); + expect(classifyError("KeyError: " + QUOTE + "email" + QUOTE)).toEqual("missing_column"); }); - it("classifies a JVM error whose frames are not Python-shaped", () => { - // mkConsoleMessage sends `err.toString()` then the stack frames, and Java frames look - // nothing like Python ones, so the frame filter must not mistake the last frame for - // the exception. - const jvm = [ - "java.lang.NullPointerException: boom", - "org.apache.texera.Foo.run(Foo.scala:12)", - "org.apache.texera.Bar.go(Bar.scala:40)", - ].join("\n"); + it("never sees the frame lines that read like patterns on their own", () => { + // A path with "model" above a `line 404` entry used to look like a model 404. Those + // lines live in `full`, which only the prompt is given. + const event = err( + "ZeroDivisionError: integer division or modulo by zero", + [ + "Traceback (most recent call last):", + ' File "/usr/lib/python3/sklearn/model_selection/_split.py", line 404, in split', + " n = len(indices) // 0", + "ZeroDivisionError: integer division or modulo by zero", + ].join("\n") + ); - expect(classifyError(jvm)).toEqual("null_error"); + expect(classifyError(event.header)).toEqual("unsupported"); + expect(event.full).toContain("line 404"); }); it("still classifies the plain case the panel is built for", () => { - expect(classifyError(KEY_ERROR)).toEqual("missing_column"); + expect(classifyError(KEY_ERROR_EVENT.header)).toEqual("missing_column"); }); }); @@ -350,8 +351,14 @@ describe("AiWorkflowFixerService", () => { const slowFirst = new Promise<{ text: string }>(resolve => (resolveFirst = resolve)); vi.spyOn(service as any, "callModel").mockReturnValueOnce(slowFirst); - const first = service.analyzeError("op-first", KEY_ERROR, SCHEMA, CODE, operatorProperties); - await service.analyzeError("op-second", "ZeroDivisionError: division by zero", SCHEMA, CODE, operatorProperties); + const first = service.analyzeError("op-first", KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError( + "op-second", + err("ZeroDivisionError: division by zero"), + SCHEMA, + CODE, + operatorProperties + ); expect(state().operatorId).toEqual("op-second"); expect(state().status).toEqual("unsupported"); @@ -375,7 +382,7 @@ describe("AiWorkflowFixerService", () => { }) ); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); expect(state().status).toEqual("error"); expect(state().suggestedFix).toBeUndefined(); @@ -386,7 +393,7 @@ describe("AiWorkflowFixerService", () => { it("drops the applied message once the re-run is over", async () => { stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); await service.applyFix(); expect(state().status).toEqual("applied"); @@ -398,7 +405,7 @@ describe("AiWorkflowFixerService", () => { it("drops a suggestion the user never applied", async () => { stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); expect(state().status).toEqual("ready"); ended("Failed"); @@ -414,7 +421,7 @@ describe("AiWorkflowFixerService", () => { const slow = new Promise<{ text: string }>(resolve => (resolveAnalysis = resolve)); vi.spyOn(service as any, "callModel").mockReturnValueOnce(slow); - const analysing = service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + const analysing = service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); ended("Killed"); expect(state().status).toEqual("idle"); @@ -430,7 +437,7 @@ describe("AiWorkflowFixerService", () => { const slow = new Promise<{ text: string }>(resolve => (resolveAnalysis = resolve)); vi.spyOn(service as any, "callModel").mockReturnValueOnce(slow); - const analysing = service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + const analysing = service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); service.discardFix(); resolveAnalysis({ text: suggestion() }); @@ -446,7 +453,7 @@ describe("AiWorkflowFixerService", () => { // the panel drops to idle and offers Analyze while the apply is still running. modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); const seen: string[] = []; const watching = service.getState$().subscribe(current => seen.push(current.status)); @@ -524,7 +531,7 @@ describe("AiWorkflowFixerService", () => { // it entirely and the tab came back saying "re-running" over a run stuck on an error. consoleMessages = [errorAt(100)]; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); await service.applyFix(); expect(state().status).toEqual("applied"); @@ -537,7 +544,7 @@ describe("AiWorkflowFixerService", () => { it("leaves the result alone while the error is still the one it was made for", async () => { consoleMessages = [errorAt(100)]; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); await service.applyFix(); consoleUpdates$.next(); @@ -549,7 +556,7 @@ describe("AiWorkflowFixerService", () => { describe("applyFix", () => { it("replaces only the snippet, keeps other properties, and re-runs", async () => { stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); await service.applyFix(); @@ -567,7 +574,7 @@ describe("AiWorkflowFixerService", () => { const twice = CODE + " yield t['email']\n"; operatorProperties = { code: twice, workers: 1 }; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, twice, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, twice, operatorProperties); await service.applyFix(); @@ -581,7 +588,7 @@ describe("AiWorkflowFixerService", () => { stubModel( suggestion({ fix_type: "property_change", original_snippet: "model", suggested_snippet: "gpt-4-turbo" }) ); - await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); await service.applyFix(); @@ -595,7 +602,7 @@ describe("AiWorkflowFixerService", () => { stubModel( suggestion({ fix_type: "property_change", original_snippet: "model", suggested_snippet: "gpt-4-turbo" }) ); - await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); // A coeditor picks a different model after the suggestion was generated: applying the // stale suggestion would silently discard their choice and re-run on it. @@ -621,7 +628,7 @@ describe("AiWorkflowFixerService", () => { // their run: the panel reported the refusal over a workflow it had just stopped. modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); operatorProperties.code = "class ProcessTupleOperator(UDFOperatorV2):\n pass\n"; @@ -638,7 +645,7 @@ describe("AiWorkflowFixerService", () => { // from the one on screen, and the diff for it is one model call away. const twice = CODE + " yield t['email']\n"; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, twice, { ...operatorProperties, code: twice }); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, twice, { ...operatorProperties, code: twice }); expect(state().suggestedFix?.occurrences).toEqual(2); operatorProperties.code = CODE; @@ -656,7 +663,7 @@ describe("AiWorkflowFixerService", () => { // warned about. Reachable because a kill lifts the modification lock without // resetting the panel: `Uninitialized` is deliberately not a terminal state. stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); expect(state().suggestedFix?.occurrences).toEqual(1); operatorProperties.code = CODE + " yield t['email']\n"; @@ -671,7 +678,7 @@ describe("AiWorkflowFixerService", () => { it("reports an apply failure, without re-running, when the snippet is no longer in the code", async () => { stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); // The user edits the UDF while reading the suggestion -- the snippet it quoted was in // the code when it was generated and is gone by the time Apply runs. A snippet that @@ -691,7 +698,7 @@ describe("AiWorkflowFixerService", () => { // says the run was refused. executeWorkflow.mockReturnValue(false); stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); await service.applyFix(); @@ -702,7 +709,7 @@ describe("AiWorkflowFixerService", () => { it("reports the re-run once the deployment accepts it", async () => { executeWorkflow.mockReturnValue(true); stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); await service.applyFix(); @@ -717,7 +724,7 @@ describe("AiWorkflowFixerService", () => { // avoids by checking first. modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); const applying = service.applyFix(); expect(killWorkflow).toHaveBeenCalled(); @@ -737,7 +744,7 @@ describe("AiWorkflowFixerService", () => { vi.useFakeTimers(); modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); const applying = service.applyFix(); await vi.advanceTimersByTimeAsync(20_000); @@ -754,7 +761,7 @@ describe("AiWorkflowFixerService", () => { // button has to work: refusing silently because the status is no longer `ready` // leaves the user clicking something that does nothing. stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); operatorProperties.code = "something else entirely"; await service.applyFix(); @@ -773,7 +780,7 @@ describe("AiWorkflowFixerService", () => { // Two imports of the same JSON carry the same operator ids, and this service outlives // the workspace, so the operator id alone would let the write land on the wrong graph. stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); openWorkflowId = 12; await service.applyFix(); @@ -788,7 +795,7 @@ describe("AiWorkflowFixerService", () => { // free to open another workflow while it does. modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); const applying = service.applyFix(); openWorkflowId = 12; @@ -805,7 +812,7 @@ describe("AiWorkflowFixerService", () => { const slow = new Promise<{ text: string }>(resolve => (resolveAnalysis = resolve)); vi.spyOn(service as any, "callModel").mockReturnValueOnce(slow); - const analysing = service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + const analysing = service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); openWorkflowId = 12; resolveAnalysis({ text: suggestion() }); await analysing; @@ -820,7 +827,7 @@ describe("AiWorkflowFixerService", () => { stubModel( suggestion({ fix_type: "property_change", original_snippet: "outputColumns", suggested_snippet: "[]" }) ); - await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); await service.applyFix(); @@ -836,10 +843,10 @@ describe("AiWorkflowFixerService", () => { // now looking at. modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); const applying = service.applyFix(); - await service.analyzeError("other-op", KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError("other-op", KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); expect(state().operatorId).toEqual("other-op"); modificationEnabled = true; @@ -859,7 +866,7 @@ describe("AiWorkflowFixerService", () => { suggestion({ fix_type: "property_change", original_snippet: "hfApiToken", suggested_snippet: "hf_new" }) ); - await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); expect(JSON.stringify(state())).not.toContain("hf_live_abc"); await service.applyFix(); @@ -874,7 +881,7 @@ describe("AiWorkflowFixerService", () => { suggestion({ fix_type: "property_change", original_snippet: "modelId", suggested_snippet: "gpt-4-turbo" }) ); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); // Refused by the analysis, not by Apply: a diff the write will never honour should // never be on screen, and refusing at the click costs the paused run as well. @@ -892,7 +899,7 @@ describe("AiWorkflowFixerService", () => { workflowReadonly = true; modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); await service.applyFix(); @@ -908,7 +915,7 @@ describe("AiWorkflowFixerService", () => { modificationEnabled = false; executionState = ExecutionState.Uninitialized; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); await service.applyFix(); @@ -924,7 +931,7 @@ describe("AiWorkflowFixerService", () => { operatorProperties = { modelId: "gpt-4-turb", workers: 1 }; stubModel(suggestion({ fix_type: "property_change", original_snippet: "modelName", suggested_snippet: "Qwen" })); - await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); expect(state().status).toEqual("error"); await service.applyFix(); @@ -944,7 +951,7 @@ describe("AiWorkflowFixerService", () => { stubModel( suggestion({ fix_type: "property_change", original_snippet: "modelId", suggested_snippet: "gpt-4-turbo" }) ); - await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); expect(state().status).toEqual("ready"); operatorProperties = { workers: 1 }; @@ -962,7 +969,7 @@ describe("AiWorkflowFixerService", () => { operatorProperties = { code: "print(1)", workers: 1 }; stubModel(suggestion({ fix_type: "property_change", original_snippet: "code", suggested_snippet: "print(2)" })); - await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); expect(state().status).toEqual("error"); expect(state().suggestedFix).toBeUndefined(); @@ -975,7 +982,7 @@ describe("AiWorkflowFixerService", () => { // current value from the operator itself. operatorProperties = { temperature: 0, workers: 1 }; stubModel(suggestion({ fix_type: "property_change", original_snippet: "temperature", suggested_snippet: "1" })); - await service.analyzeError(OP, "404 model not found", SCHEMA, undefined, operatorProperties); + await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); await service.applyFix(); @@ -987,7 +994,7 @@ describe("AiWorkflowFixerService", () => { throw new Error("read-only workflow"); }); stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); await service.applyFix(); @@ -998,7 +1005,7 @@ describe("AiWorkflowFixerService", () => { it("discards to idle and keeps state across re-subscription", async () => { stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); // A frame rebuilt by clearResultPanel() re-subscribes and must still see the fix. expect((await firstValueFrom(service.getState$())).status).toEqual("ready"); diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts index f21b6873ebf..1a955f6e4a5 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts @@ -33,6 +33,19 @@ import { OperatorMetadataService } from "../../../service/operator-metadata/oper import { WorkflowConsoleService } from "../../../service/workflow-console/workflow-console.service"; export type FixErrorType = "missing_column" | "type_error" | "null_error" | "model_not_found" | "unsupported"; +/** + * One operator error, kept as the two parts its source already distinguishes. + * + * Classification reads `header` and the prompt gets `full`: collapsing them would put the + * burden of finding the exception inside the joined text back on this service. + */ +export interface OperatorError { + /** The exception line on its own -- a console message's `title`, a fatal error's `message`. */ + header: string; + /** Exception and stack together, which is what the model is shown. */ + full: string; +} + export type FixConfidence = "high" | "medium" | "low"; export interface SuggestedFix { @@ -186,40 +199,21 @@ export function coerceToFieldType(currentValue: unknown, suggested: string): unk return suggested; } -// What a stack frame looks like, so it is never mistaken for the exception. Python writes -// File "udf.py", line 7, in process_tuple -// and the JVM writes -// org.apache.texera.Foo.run(Foo.scala:12) -// which carries no colon-separated exception name, so a Python-only pattern lets a Java -// frame through and classifies on it. -const FRAME_LINE = /^File\s+"|^Traceback \(|^at\s|^[\w$.]+\([\w.]+:\d+\)$/; - /** - * The exception a traceback ends on. + * Classifies on the exception alone, never on the stack around it. * - * Matching the whole message would classify on whatever appears first, which is wrong twice - * over: the frame lines carry paths and line numbers that read like patterns -- a path - * containing "model" above a `line 404` entry looks like a model 404 -- and a chained - * traceback reports the handled exception before the one that actually stopped the worker. - * Python puts the real exception last. + * Both sources already separate the exception from its frames, so nothing here has to find + * it: a console message carries `err.toString` / the traceback's last line in `title`, and a + * fatal error carries it in `message`. Deducing it from the joined text instead meant + * guessing at frame shapes, and the JDK's own are unguessable -- on Java 9+ they carry a + * module prefix (`java.base/java.lang.Thread.run(Thread.java:840)`) and native ones read + * `(Native Method)`, so a real worker trace ended on a frame no pattern recognised and + * `NullPointerException` never classified. Python's `title` is the LAST line of the formatted + * traceback, so a chained one arrives already reduced to the exception that stopped the + * worker. */ -export function finalExceptionLine(errorMessage: string): string { - const lines = errorMessage - .split("\n") - .map(line => line.trim()) - .filter(line => line.length > 0); - // Frame lines are the `File "...", line N` entries and the source line under them; the - // exception is the last line that is neither that nor the traceback header. - for (let i = lines.length - 1; i >= 0; i--) { - if (!FRAME_LINE.test(lines[i])) { - return lines[i]; - } - } - return errorMessage; -} - -export function classifyError(errorMessage: string): FixErrorType { - return PATTERNS.find(([pattern]) => pattern.test(finalExceptionLine(errorMessage)))?.[1] ?? "unsupported"; +export function classifyError(exceptionHeader: string): FixErrorType { + return PATTERNS.find(([pattern]) => pattern.test(exceptionHeader))?.[1] ?? "unsupported"; } /** @@ -279,12 +273,12 @@ export class AiWorkflowFixerService { public async analyzeError( operatorId: string, - errorMessage: string, + error: OperatorError, inputSchemas: OperatorPortSchemaMap | undefined, code: string | undefined, properties: Readonly> ): Promise { - const errorType = classifyError(errorMessage); + const errorType = classifyError(error.header); const workflowId = this.workflowActionService.getOpenWorkflowId(); const base: FixState = { operatorId, @@ -310,7 +304,7 @@ export class AiWorkflowFixerService { // field's current value to show in the diff, and reading it from the raw properties // put a credential on screen whenever the model guessed one of those names. const visible = withoutSecrets(properties, this.schemaProperties(operatorId)); - const { text } = await this.callModelWithTimeout(buildFixPrompt(errorMessage, code, inputSchemas, visible)); + const { text } = await this.callModelWithTimeout(buildFixPrompt(error.full, code, inputSchemas, visible)); if (seq !== this.analysisSeq || workflowId !== this.workflowActionService.getOpenWorkflowId()) { return; } From e35d09725541b84bf983f44fd173b31d0f434c37 Mon Sep 17 00:00:00 2001 From: Pedro Date: Fri, 23 Oct 2026 10:36:42 -0600 Subject: [PATCH 33/39] fix(frontend): ask for whole indented lines instead of guessing indentation @kunwp1: the previous fix only covered a multi-line original. A single-line one whose suggestion already carried absolute indentation was still indented twice -- `return` landing at 20 spaces instead of 12 -- because the result depended on which convention the model happened to use and the prompt never stated one. So the prompt states one. original_snippet and suggested_snippet must be whole lines, copied from the start of a line with the leading indentation included, each already at the column it should have. startsAtLineBoundary refuses a reply that quotes from mid-line, checking every occurrence rather than the first, since one mid-line match is enough to make the result depend on a guess. replaceAllIndented is then just a replacement, and is named replaceAll. The last heuristic in the file is gone. Verified against claude-haiku-4.5 through the local LiteLLM proxy, on the three code-change shapes (KeyError, TypeError cast, NaN guard). The new prompt: 28 of 28 replies accepted. The old prompt, as a control: 11 of 12 -- and the one refusal is exactly a mid-line snippet with no indentation, the case this is about. The model's own multi-line guard applies to 0/4/8/12 nesting, valid Python, nothing doubled. The test fixture had to change with it, which says the same thing: its original_snippet was quoted mid-line, so the contract now turns it away. Co-Authored-By: Claude Opus 5 --- .../result-panel/ai-fix/ai-fix-prompt.ts | 8 +- .../ai-fix/ai-workflow-fixer.service.spec.ts | 94 +++++++++++-------- .../ai-fix/ai-workflow-fixer.service.ts | 61 +++++++----- 3 files changed, 97 insertions(+), 66 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts index a3ae8250138..2bf0f333af4 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts @@ -117,13 +117,17 @@ Rules: - If it is a null/NaN error: add a .dropna() call or an explicit null guard. - If it is a model name error: correct the model field in the configuration. - Do NOT rewrite the whole function. Change the minimum number of lines. +- original_snippet and suggested_snippet must be WHOLE LINES, copied starting at the + beginning of a line, with the leading indentation included exactly as it appears. Do not + start a snippet in the middle of a line, and do not re-indent: every line you return must + already carry the indentation it should have in the file. Respond ONLY with valid JSON, no markdown, no explanation outside the JSON: { "explanation": "one sentence describing the root cause", "fix_type": "code_change" | "property_change", - "original_snippet": "exact lines to replace (for code_change) or field name (for property_change)", - "suggested_snippet": "replacement lines or new field value", + "original_snippet": "whole lines to replace, indentation included (for code_change), or the field name (for property_change)", + "suggested_snippet": "replacement lines, each already indented as it should appear, or the new field value", "confidence": "high" | "medium" | "low" }`; } diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts index 68425ef002b..f0da5939048 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts @@ -24,7 +24,8 @@ import { classifyError, coerceToFieldType, FixState, - replaceAllIndented, + replaceAll, + startsAtLineBoundary, OperatorError, } from "./ai-workflow-fixer.service"; import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; @@ -87,8 +88,9 @@ describe("AiWorkflowFixerService", () => { JSON.stringify({ explanation: "the column is user_email", fix_type: "code_change", - original_snippet: "yield t['email']", - suggested_snippet: "yield t['user_email']", + // Whole lines with their indentation, which is what the prompt requires. + original_snippet: " yield t['email']", + suggested_snippet: " yield t['user_email']", confidence: "high", ...over, }); @@ -188,8 +190,8 @@ describe("AiWorkflowFixerService", () => { expect(state().errorType).toEqual("missing_column"); expect(state().suggestedFix).toEqual({ type: "code_change", - original: "yield t['email']", - suggested: "yield t['user_email']", + original: " yield t['email']", + suggested: " yield t['user_email']", explanation: "the column is user_email", confidence: "high", fieldName: undefined, @@ -198,12 +200,24 @@ describe("AiWorkflowFixerService", () => { }); }); + it("refuses a snippet quoted from the middle of a line", async () => { + // The prompt asks for whole lines carrying their indentation. A mid-line snippet says + // nothing about the column its continuation lines belong at, and inferring one is the + // guess that put `yield` at sixteen spaces in an eight-space body. + stubModel(suggestion({ original_snippet: "yield t['email']", suggested_snippet: "yield t['user_email']" })); + + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + + expect(state().status).toEqual("error"); + expect(state().suggestedFix).toBeUndefined(); + }); + it("reports a snippet that is not in the code instead of offering an Apply that cannot work", async () => { // The panel only shows the occurrence count when it is above one, so a snippet the // model invented looked like any other suggestion. Apply then failed with "the code // changed since the suggestion was generated", which blames an edit that never // happened. - stubModel(suggestion({ original_snippet: "yield t['address']" })); + stubModel(suggestion({ original_snippet: " yield t['address']" })); await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); @@ -376,8 +390,8 @@ describe("AiWorkflowFixerService", () => { JSON.stringify({ explanation: "the column is user_email", fix_type: "code_change", - original_snippet: ["yield t['email']"], - suggested_snippet: "yield t['user_email']", + original_snippet: [" yield t['email']"], + suggested_snippet: " yield t['user_email']", confidence: "high", }) ); @@ -469,52 +483,54 @@ describe("AiWorkflowFixerService", () => { }); }); - describe("replaceAllIndented", () => { - it("indents the continuation lines to the line the match was found on", () => { - // Without this the first line lands correctly and the rest start at column 0, which - // is an IndentationError the moment the fix is applied. - const code = "def f():\n x = read()\n"; + describe("replaceAll", () => { + it("writes the suggestion verbatim, adding no indentation of its own", () => { + // The snippet is a whole line carrying its own eight spaces, and so is the + // replacement. Adding the context indentation on top is what put `yield` at sixteen. + const code = "def f():\n a = read()\n yield a\n"; - const out = replaceAllIndented(code, "x = read()", "x = read()\nx = x.strip()").code; + const out = replaceAll(code, " a = read()", " a = read().strip()").code; - expect(out).toEqual("def f():\n x = read()\n x = x.strip()\n"); + expect(out).toEqual("def f():\n a = read().strip()\n yield a\n"); }); - it("keeps the relative indentation the suggestion itself has", () => { - const code = " y = 0\n"; + it("places a multi-line suggestion at the column each of its lines asks for", () => { + const code = "def f():\n x = read()\n"; - const out = replaceAllIndented(code, "y = 0", "if True:\n y = 0").code; + const out = replaceAll(code, " x = read()", " x = read()\n x = x.strip()").code; - expect(out).toEqual(" if True:\n y = 0\n"); + expect(out).toEqual("def f():\n x = read()\n x = x.strip()\n"); }); - it("indents each occurrence at its own depth", () => { - const code = " a()\n a()\n"; - - const out = replaceAllIndented(code, "a()", "a()\nb()").code; + it("counts the places it changed", () => { + const out = replaceAll("p()\np()\n", "p()", "q()"); - expect(out).toEqual(" a()\n b()\n a()\n b()\n"); + expect(out.code).toEqual("q()\nq()\n"); + expect(out.count).toEqual(2); }); + }); - it("does not indent twice when the match itself spans lines", () => { - // A multi-line `original` is quoted from the code, so its continuation lines already - // carry the absolute indentation -- and the model writes the replacement the same - // way. Adding the match indentation on top put `yield` at 16 spaces in an 8-space - // body, an IndentationError the moment the fix was "applied". - const code = "def f():\n a = read()\n yield a\n"; - const original = "a = read()\n yield a"; - const suggested = "a = read().strip()\n yield a"; - - const out = replaceAllIndented(code, original, suggested).code; + describe("startsAtLineBoundary", () => { + it("accepts a snippet that begins a line, including the first one", () => { + expect(startsAtLineBoundary(" a()\n b()\n", " a()")).toBe(true); + expect(startsAtLineBoundary("a()\n", "a()")).toBe(true); + }); - expect(out).toEqual("def f():\n a = read().strip()\n yield a\n"); + it("rejects a snippet quoted from the middle of a line", () => { + // Without its leading indentation the snippet says nothing about what column the + // lines after it belong at, which is exactly what used to be guessed. + expect(startsAtLineBoundary(" a()\n", "a()")).toBe(false); }); - it("counts the places it changed, and leaves a single-line fix alone", () => { - const out = replaceAllIndented("p()\np()\n", "p()", "q()"); + it("rejects when any one occurrence is mid-line, not just the first", () => { + // Every match is replaced, so one mid-line occurrence is enough to make the result + // depend on a guess -- and the first match looking fine is what would hide it. + expect(startsAtLineBoundary("a()\nif x: a()\n", "a()")).toBe(false); + }); - expect(out.code).toEqual("q()\nq()\n"); - expect(out.count).toEqual(2); + it("says nothing about a snippet the code does not contain", () => { + // Absence is a separate refusal, with its own message. + expect(startsAtLineBoundary("a()\n", "zzz()")).toBe(true); }); }); diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts index 1a955f6e4a5..bb7ffb1de19 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts @@ -144,34 +144,39 @@ const PATTERNS: ReadonlyArray<[RegExp, FixErrorType]> = [ ]; /** - * Replaces every occurrence, keeping each one at the indentation it was found at. + * Replaces every occurrence of a snippet, and reports how many there were. * - * A single-line match starts mid-line, after the existing indentation, so a multi-line - * replacement would put its first line in the right place and every line after it at column - * 0 -- an IndentationError the moment the fix was applied. Those continuation lines take the - * indentation of the line the match was found on, and keep whatever relative indentation the - * suggestion itself had. - * - * A match that spans lines already carries the absolute indentation of the region it came - * from, and the model's reply mirrors it, so there is nothing to add: prefixing the context - * indentation on top would double it. + * No indentation work: the prompt requires whole lines carrying the indentation they should + * have, and `startsAtLineBoundary` turns away a reply that does not, so each line of the + * suggestion already sits at the right column. Re-indenting continuation lines here instead + * meant guessing which convention the model had used -- absolute or relative -- and the + * prompt never stated one, so either guess doubled the indentation for the other. */ -export function replaceAllIndented(code: string, original: string, suggested: string): { code: string; count: number } { +export function replaceAll(code: string, original: string, suggested: string): { code: string; count: number } { const parts = code.split(original); - const count = parts.length - 1; - if (count === 0 || !suggested.includes("\n") || original.includes("\n")) { - return { code: parts.join(suggested), count }; - } + return { code: parts.join(suggested), count: parts.length - 1 }; +} - const [head, ...rest] = suggested.split("\n"); - let out = parts[0]; - for (const tail of parts.slice(1)) { - // Whitespace since the last newline is what this occurrence sits at. - const lineStart = out.lastIndexOf("\n") + 1; - const indent = (out.slice(lineStart).match(/^[ \t]*/) ?? [""])[0]; - out += [head, ...rest.map(line => (line.length > 0 ? indent + line : line))].join("\n") + tail; +/** + * Whether every occurrence of the snippet begins a line. + * + * This is what makes the replacement unambiguous: a snippet that starts at column 0 of its + * line carries its own leading indentation, so the suggestion replacing it must carry the + * indentation it wants and nothing has to be inferred. A snippet starting mid-line leaves + * the indentation of its continuation lines undetermined. + */ +export function startsAtLineBoundary(code: string, original: string): boolean { + let from = 0; + for (;;) { + const at = code.indexOf(original, from); + if (at === -1) { + return true; + } + if (at !== 0 && code[at - 1] !== "\n") { + return false; + } + from = at + original.length; } - return { code: out, count }; } /** Writes the model's string back in the type the field already held. */ @@ -318,7 +323,13 @@ export class AiWorkflowFixerService { // Counted here so the panel can say it before Apply: a short snippet matched as plain // text can appear in lines the user never saw in the diff. if (suggestedFix.type === "code_change") { - const occurrences = replaceAllIndented(code ?? "", suggestedFix.original, suggestedFix.suggested).count; + // The prompt asks for whole lines with their indentation; a snippet starting mid-line + // means the model ignored that, and applying it would put the lines after the first + // at a column nothing here can work out. + if (!startsAtLineBoundary(code ?? "", suggestedFix.original)) { + throw new Error("the model did not quote whole lines, so the fix cannot be placed"); + } + const occurrences = replaceAll(code ?? "", suggestedFix.original, suggestedFix.suggested).count; // Zero means the model quoted something the operator does not contain. The panel // only surfaces this count when it is above one, so such a suggestion is // indistinguishable from any other, and Apply turns it away as a stale snippet -- @@ -409,7 +420,7 @@ export class AiWorkflowFixerService { }; if (fix.type === "code_change") { const code = String(properties.code ?? ""); - const patched = replaceAllIndented(code, fix.original, fix.suggested); + const patched = replaceAll(code, fix.original, fix.suggested); if (patched.count === 0) { throw new Error("the code changed since the suggestion was generated"); } From b3f3606dac0d51e67840995d089d49ab640cb105 Mon Sep 17 00:00:00 2001 From: Pedro Date: Fri, 23 Oct 2026 11:47:33 -0600 Subject: [PATCH 34/39] fix(frontend): decide the AI Fix tab from the map, not a remembered key Found by running the stack, not by the tests. The tab never came back when an operator failed twice. The console stream fed a `distinctUntilChanged` keyed on operator + whether the tab is wanted, but rerenderResultPanel writes to the same map without going through that pipe. So: the operator fails (key `true`), a new run starts, the rerender drops the tab -- and the pipe still remembers `true`. The next traceback maps to `true` again, is read as "no change", and the tab stays gone. That is the fix-and-re-run loop this panel exists for. The live trace, with the component instrumented in the browser: rerenderResultPanel() syncAiFixFrame(offer=false) <- run starts, tab dropped STREAM emits -> key=op:true <- traceback arrives, key is right <- syncAiFixFrame never called syncAiFixFrame now compares against the entry that is actually in the map, including which operator it points at, and reports whether it changed anything so the caller can skip change detection. Nothing is remembered outside the map, so it no longer matters which path wrote to it last. The existing test passed because it drives the stream directly, with no rerender interleaved -- distinctUntilChanged's first emission always gets through. The new test follows the live trace instead, and fails without this change. Co-Authored-By: Claude Opus 5 --- .../result-panel.component.spec.ts | 32 ++++++++++++++ .../result-panel/result-panel.component.ts | 42 ++++++++++++------- 2 files changed, 60 insertions(+), 14 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts b/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts index a11967ab3eb..dc57d9fae8c 100644 --- a/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts @@ -506,6 +506,38 @@ describe("ResultPanelComponent", () => { expect(component.frameComponentConfigs.get("Console")).toBe(consoleFrame); }); + it("brings the tab back when the same operator fails again on a later run", () => { + // The map is written by two paths -- this stream and rerenderResultPanel -- but only + // this one fed the distinctUntilChanged key. Starting a run clears the console and the + // rerender drops the tab, while the pipe still remembers `true`, so the next error maps + // to `true` again, is read as "no change", and the tab never comes back. That is the + // fix-and-re-run loop this panel exists for. + workflowActionService.addOperator(mockResultPredicate, mockPoint); + const consoleService = TestBed.inject(WorkflowConsoleService); + (TestBed.inject(GuiConfigService).env as any).copilotEnabled = true; + vi.spyOn(workflowActionService.getJointGraphWrapper(), "getCurrentHighlightedOperatorIDs").mockReturnValue(["3"]); + vi.spyOn(consoleService, "hasConsoleMessages").mockReturnValue(true); + + // First run fails and the error reaches this stream, so the pipe now remembers `true`. + vi.spyOn(consoleService, "getConsoleMessages").mockReturnValue([consoleMessage("ERROR")]); + component.rerenderResultPanel(); + consoleService.clearConsoleMessages(); + expect(component.frameComponentConfigs.has("AI Fix")).toBe(true); + + // A new run starts. The console is emptied without notifying this stream -- the engine + // clears it on its own -- and the rerender takes the tab down, so the pipe's memory is + // now stale. + vi.spyOn(consoleService, "getConsoleMessages").mockReturnValue([]); + component.rerenderResultPanel(); + expect(component.frameComponentConfigs.has("AI Fix")).toBe(false); + + // It fails again, with the operator still selected. + vi.spyOn(consoleService, "getConsoleMessages").mockReturnValue([consoleMessage("ERROR")]); + consoleService.clearConsoleMessages(); + + expect(component.frameComponentConfigs.has("AI Fix")).toBe(true); + }); + it("registers the display-name handler once rather than on every rerender", () => { // It used to be called from the rerender subscriber, adding a subscription each time // without releasing the previous one, so handlers piled up for the panel's lifetime. diff --git a/frontend/src/app/workspace/component/result-panel/result-panel.component.ts b/frontend/src/app/workspace/component/result-panel/result-panel.component.ts index 256fac0ffb6..040ca9d414d 100644 --- a/frontend/src/app/workspace/component/result-panel/result-panel.component.ts +++ b/frontend/src/app/workspace/component/result-panel/result-panel.component.ts @@ -35,7 +35,7 @@ import { ResultTableFrameComponent } from "./result-table-frame/result-table-fra import { ConsoleFrameComponent } from "./console-frame/console-frame.component"; import { WorkflowResultService } from "../../service/workflow-result/workflow-result.service"; import { PanelResizeService } from "../../service/workflow-result/panel-resize/panel-resize.service"; -import { distinctUntilChanged, filter, map } from "rxjs/operators"; +import { filter } from "rxjs/operators"; import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; import { isPythonUdf, isSink } from "../../service/workflow-graph/model/workflow-graph"; import { WorkflowVersionService } from "../../../dashboard/service/user/workflow-version/workflow-version.service"; @@ -267,16 +267,16 @@ export class ResultPanelComponent implements OnInit, OnDestroy { // position and debug input, and the Result table, whenever any operator prints. this.workflowConsoleService .getConsoleMessageUpdateStream() - .pipe( - // Keyed by operator as well as by the flag: comparing the flag alone would block a - // second operator's error after the first one's, since both map to `true`. - map(() => `${this.currentOperatorId ?? ""}:${this.shouldOfferAiFix()}`), - distinctUntilChanged(), - untilDestroyed(this) - ) + .pipe(untilDestroyed(this)) .subscribe(() => { - this.syncAiFixFrame(); - this.changeDetectorRef.detectChanges(); + // Nothing is remembered between emissions here. syncAiFixFrame compares against the + // map itself, which rerenderResultPanel writes to as well: a key remembered by the + // pipe went stale the moment the rerender dropped the tab, so the next error mapped + // to the same value, read as "no change", and the tab never came back -- in the + // fix-and-re-run loop this panel is for. + if (this.syncAiFixFrame()) { + this.changeDetectorRef.detectChanges(); + } }); } @@ -307,12 +307,25 @@ export class ResultPanelComponent implements OnInit, OnDestroy { return config.value; } - /** Adds or removes the AI Fix entry, leaving every other frame untouched. */ - private syncAiFixFrame(): void { - if (this.shouldOfferAiFix()) { + /** + * Adds or removes the AI Fix entry, leaving every other frame untouched. + * + * Returns whether it changed anything, so a caller can skip change detection. The decision + * is made against the entry that is actually there, including which operator it points at: + * this runs from the console stream and from rerenderResultPanel, and anything remembered + * outside the map goes stale as soon as the other path writes to it. + */ + private syncAiFixFrame(): boolean { + const operatorId = this.shouldOfferAiFix() ? this.currentOperatorId : undefined; + const existing = this.frameComponentConfigs.get("AI Fix"); + const shown = existing ? (existing.componentInputs as { operatorId?: string }).operatorId : undefined; + if (operatorId === shown) { + return false; + } + if (operatorId) { this.frameComponentConfigs.set("AI Fix", { component: AiFixFrameComponent, - componentInputs: { operatorId: this.currentOperatorId }, + componentInputs: { operatorId }, }); } else { // Mirrors the Static Error frame's teardown: once the operator no longer reports an @@ -320,6 +333,7 @@ export class ResultPanelComponent implements OnInit, OnDestroy { // -- a tab offering to fix it has nothing to act on. this.frameComponentConfigs.delete("AI Fix"); } + return true; } rerenderResultPanel(): void { From 10d37758943a7fc934fb252f25c48651005951e7 Mon Sep 17 00:00:00 2001 From: Pedro Date: Mon, 26 Oct 2026 19:09:18 -0600 Subject: [PATCH 35/39] fix(frontend): require the snippet to end a line, and drop a check that cannot fail Two from @kunwp1's latest round. A match that begins a line but stops inside it still replaces cleanly, so ` n = 1` matched ` n = 10` once -- a count of 1, which hides the occurrence badge -- and Apply left the trailing `0` behind. The diff offered `n = 1` -> `n = 2` and the operator was left saying `n = 20`. Checking only the start was half the contract: starting a line is what makes the indentation unambiguous, ending one is what keeps the replacement honest. The function says what it does now and is called matchesWholeLines. The errorType check in the apply path is gone. I had kept it as a second barrier, the way the field checks are kept, but the two are not alike: a field can disappear between the analysis and the write, while errorType is fixed when the state is published and the analysis already refuses a property fix for anything but model_not_found. It was unreachable, so the comment now says why there is nothing there. Co-Authored-By: Claude Opus 5 --- .../ai-fix/ai-workflow-fixer.service.spec.ts | 29 ++++++++++++---- .../ai-fix/ai-workflow-fixer.service.ts | 34 +++++++++++-------- 2 files changed, 41 insertions(+), 22 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts index f0da5939048..d5385998a8a 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts @@ -25,7 +25,7 @@ import { coerceToFieldType, FixState, replaceAll, - startsAtLineBoundary, + matchesWholeLines, OperatorError, } from "./ai-workflow-fixer.service"; import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; @@ -510,27 +510,42 @@ describe("AiWorkflowFixerService", () => { }); }); - describe("startsAtLineBoundary", () => { + describe("matchesWholeLines", () => { it("accepts a snippet that begins a line, including the first one", () => { - expect(startsAtLineBoundary(" a()\n b()\n", " a()")).toBe(true); - expect(startsAtLineBoundary("a()\n", "a()")).toBe(true); + expect(matchesWholeLines(" a()\n b()\n", " a()")).toBe(true); + expect(matchesWholeLines("a()\n", "a()")).toBe(true); }); it("rejects a snippet quoted from the middle of a line", () => { // Without its leading indentation the snippet says nothing about what column the // lines after it belong at, which is exactly what used to be guessed. - expect(startsAtLineBoundary(" a()\n", "a()")).toBe(false); + expect(matchesWholeLines(" a()\n", "a()")).toBe(false); }); it("rejects when any one occurrence is mid-line, not just the first", () => { // Every match is replaced, so one mid-line occurrence is enough to make the result // depend on a guess -- and the first match looking fine is what would hide it. - expect(startsAtLineBoundary("a()\nif x: a()\n", "a()")).toBe(false); + expect(matchesWholeLines("a()\nif x: a()\n", "a()")).toBe(false); }); + it("rejects a whole line that is only the start of a longer one", () => { + // The dangerous half: the match begins a line, so the indentation is unambiguous and + // the count is 1, which hides the badge -- but it stops mid-line. Replacing it leaves + // the rest of the line stuck on the end, so `n = 1` becoming `n = 2` silently rewrites + // `n = 10` as `n = 20` while the diff only ever showed `n = 1`. + expect(matchesWholeLines(" n = 10\n", " n = 1")).toBe(false); + }); + + it("accepts a match that ends at the end of the code, with no trailing newline", () => { + expect(matchesWholeLines(" a()", " a()")).toBe(true); + }); + + it("rejects when only one of several occurrences stops mid-line", () => { + expect(matchesWholeLines(" a()\n a() + 1\n", " a()")).toBe(false); + }); it("says nothing about a snippet the code does not contain", () => { // Absence is a separate refusal, with its own message. - expect(startsAtLineBoundary("a()\n", "zzz()")).toBe(true); + expect(matchesWholeLines("a()\n", "zzz()")).toBe(true); }); }); diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts index bb7ffb1de19..89f73acd7c4 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts @@ -147,7 +147,7 @@ const PATTERNS: ReadonlyArray<[RegExp, FixErrorType]> = [ * Replaces every occurrence of a snippet, and reports how many there were. * * No indentation work: the prompt requires whole lines carrying the indentation they should - * have, and `startsAtLineBoundary` turns away a reply that does not, so each line of the + * have, and `matchesWholeLines` turns away a reply that does not, so each line of the * suggestion already sits at the right column. Re-indenting continuation lines here instead * meant guessing which convention the model had used -- absolute or relative -- and the * prompt never stated one, so either guess doubled the indentation for the other. @@ -158,24 +158,29 @@ export function replaceAll(code: string, original: string, suggested: string): { } /** - * Whether every occurrence of the snippet begins a line. + * Whether every occurrence of the snippet covers whole lines. * - * This is what makes the replacement unambiguous: a snippet that starts at column 0 of its - * line carries its own leading indentation, so the suggestion replacing it must carry the - * indentation it wants and nothing has to be inferred. A snippet starting mid-line leaves - * the indentation of its continuation lines undetermined. + * Both ends matter, for different reasons. Starting a line is what makes the indentation + * unambiguous: the snippet carries its own, so the suggestion carries the indentation it + * wants and nothing is inferred. Ending a line is what keeps the replacement honest -- a + * snippet that begins a line but stops inside it still replaces cleanly, so ` n = 1` + * matches ` n = 10` once, shows no occurrence badge, and leaves the trailing `0` + * behind: the diff offers `n = 1` -> `n = 2` and the operator is left saying `n = 20`. */ -export function startsAtLineBoundary(code: string, original: string): boolean { +export function matchesWholeLines(code: string, original: string): boolean { let from = 0; for (;;) { const at = code.indexOf(original, from); if (at === -1) { return true; } - if (at !== 0 && code[at - 1] !== "\n") { + const end = at + original.length; + const startsLine = at === 0 || code[at - 1] === "\n"; + const endsLine = end === code.length || code[end] === "\n"; + if (!startsLine || !endsLine) { return false; } - from = at + original.length; + from = end; } } @@ -326,7 +331,7 @@ export class AiWorkflowFixerService { // The prompt asks for whole lines with their indentation; a snippet starting mid-line // means the model ignored that, and applying it would put the lines after the first // at a column nothing here can work out. - if (!startsAtLineBoundary(code ?? "", suggestedFix.original)) { + if (!matchesWholeLines(code ?? "", suggestedFix.original)) { throw new Error("the model did not quote whole lines, so the fix cannot be placed"); } const occurrences = replaceAll(code ?? "", suggestedFix.original, suggestedFix.suggested).count; @@ -438,11 +443,10 @@ export class AiWorkflowFixerService { } properties.code = patched.code; } else { - // Only the model-not-found case asks for a configuration change; a property fix for - // any other error is the model leaving the four shapes this panel supports. - if (current.errorType !== "model_not_found") { - throw new Error("a configuration change is not the fix for this error"); - } + // No errorType check here. The analysis refuses a property fix for anything but + // model_not_found, and errorType is fixed when the state is published, so by this + // point it can only be that -- unlike the field checks below, which guard against the + // operator moving between the analysis and the write. const field = String(fix.fieldName); // Checked against the fields the model was actually shown, not the raw properties: // those still hold the credentials withoutSecrets kept out of the prompt, so a From 0935e669edba112d5a205a946f9854c9618ba799 Mon Sep 17 00:00:00 2001 From: Pedro Date: Mon, 26 Oct 2026 20:05:53 -0600 Subject: [PATCH 36/39] refactor(frontend): keep only the fixes this panel can actually write @kunwp1 asked whether the supported list could be narrowed to what works end to end. Checking the repo, two of the four shapes have no reachable producer: - **model_not_found.** The HuggingFace operator never raises for an unknown model. A 404 from the inference API is retried as a chat completion, and when every provider fails the text goes into the result rows via `_format_error` -- no exception, no ERROR console message, no tab. A malformed id does raise, but as `ValueError: Invalid Hugging Face model ID ...`, which these patterns do not match. The sklearn operators have a model field and no remote lookup to 404 on. - **NullPointerException.** Only a JVM operator raises it, and a JVM operator has no `code` for Apply to patch, so Analyze could only spend a model call to reach a refusal. So the panel now says what it is: a code fixer for Python UDFs. Three shapes, one kind of fix. The tab is offered only where `isPythonUdf` holds, and toFix turns away any reply that is not a code change. That left the configuration path with no way in, and dead code is what the last round was about, so it goes: the property branch of patchedProperties, coerceToFieldType, `type` and `fieldName` on SuggestedFix, `errorType` on FixState (its only reader was that branch), the field row in the diff, and the model rule in the prompt. Fourteen test blocks went with it. One of those tests was about to become tautological rather than fail: the credential test asked for a model error, which is now unsupported, so the model was never called and the assertion had nothing to inspect. It asks for a supported error now, so it still proves what it claims -- that the service redacts before building the prompt. Co-Authored-By: Claude Opus 5 --- .../ai-fix/ai-fix-frame.component.html | 6 - .../ai-fix/ai-fix-frame.component.spec.ts | 27 +-- .../result-panel/ai-fix/ai-fix-prompt.ts | 7 +- .../ai-fix/ai-workflow-fixer.service.spec.ts | 213 ++---------------- .../ai-fix/ai-workflow-fixer.service.ts | 157 ++++--------- .../result-panel.component.spec.ts | 52 +++++ .../result-panel/result-panel.component.ts | 16 +- 7 files changed, 132 insertions(+), 346 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.html b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.html index 7862d0968e7..ffaec57a458 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.html +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.html @@ -87,12 +87,6 @@ >
-
- Configuration field: {{ fix.fieldName }} -
-
= {}) => ({ message: "KeyError: 'email'", @@ -267,7 +252,7 @@ describe("AiFixFrameComponent", () => { // Without the button this state is a dead end: nothing resets it, so after the user // fixes the code and re-runs into a supported error the tab keeps showing the stale // message until some other operator is analyzed. - render({ ...READY_CODE_FIX, status: "unsupported", errorType: undefined, suggestedFix: undefined }); + render({ ...READY_CODE_FIX, status: "unsupported", suggestedFix: undefined }); expect(fixture.nativeElement.textContent).toContain(UNSUPPORTED_MESSAGE); expect(query(".ai-fix-analyze")).toBeTruthy(); @@ -332,7 +317,7 @@ describe("AiFixFrameComponent", () => { }); it("shows the not-supported message for an out-of-scope error", () => { - render({ ...READY_CODE_FIX, status: "unsupported", errorType: undefined, suggestedFix: undefined }); + render({ ...READY_CODE_FIX, status: "unsupported", suggestedFix: undefined }); expect(query("nz-alert")).toBeTruthy(); expect(fixture.nativeElement.textContent).toContain(UNSUPPORTED_MESSAGE); @@ -349,14 +334,6 @@ describe("AiFixFrameComponent", () => { expect(texts(".ai-fix-added")).toEqual(["+ yield t['user_email']", "+ yield t"]); }); - it("renders the field name and values instead of code for a property change", () => { - render(READY_PROPERTY_FIX); - - expect(query(".ai-fix-field").nativeElement.textContent).toContain("model"); - expect(texts(".ai-fix-removed")).toEqual(["- gpt-4-turb"]); - expect(texts(".ai-fix-added")).toEqual(["+ gpt-4-turbo"]); - }); - it("delegates the apply and discard buttons to the service", () => { render(READY_CODE_FIX); diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts index 2bf0f333af4..96519095d53 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts @@ -115,7 +115,6 @@ Rules: - If it is a missing column error: suggest the closest column name from the schema. - If it is a type error: add an explicit cast (e.g., int(), str(), float()) at the point of failure. - If it is a null/NaN error: add a .dropna() call or an explicit null guard. -- If it is a model name error: correct the model field in the configuration. - Do NOT rewrite the whole function. Change the minimum number of lines. - original_snippet and suggested_snippet must be WHOLE LINES, copied starting at the beginning of a line, with the leading indentation included exactly as it appears. Do not @@ -125,9 +124,9 @@ Rules: Respond ONLY with valid JSON, no markdown, no explanation outside the JSON: { "explanation": "one sentence describing the root cause", - "fix_type": "code_change" | "property_change", - "original_snippet": "whole lines to replace, indentation included (for code_change), or the field name (for property_change)", - "suggested_snippet": "replacement lines, each already indented as it should appear, or the new field value", + "fix_type": "code_change", + "original_snippet": "whole lines to replace, indentation included", + "suggested_snippet": "replacement lines, each already indented as it should appear", "confidence": "high" | "medium" | "low" }`; } diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts index d5385998a8a..0f6e0812ed9 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts @@ -22,7 +22,6 @@ import { firstValueFrom, Subject } from "rxjs"; import { AiWorkflowFixerService, classifyError, - coerceToFieldType, FixState, replaceAll, matchesWholeLines, @@ -162,10 +161,6 @@ describe("AiWorkflowFixerService", () => { expect(classifyError(KEY_ERROR_EVENT.header)).toEqual("missing_column"); expect(classifyError("TypeError: unsupported operand type(s) for *: 'str' and 'int'")).toEqual("type_error"); expect(classifyError("ValueError: input contains NaN")).toEqual("null_error"); - expect(classifyError("java.lang.NullPointerException: boom")).toEqual("null_error"); - expect(classifyError("ModelNotFound: no such model")).toEqual("model_not_found"); - expect(classifyError("Error code: 404 - model does not exist")).toEqual("model_not_found"); - expect(classifyError("404 Client Error: Not Found for url: .../models/Qwen")).toEqual("model_not_found"); // A bare 404 is a line number, an unrelated HTTP status, or data -- not a model lookup. expect(classifyError(' File "udf.py", line 404, in process_tuple')).toEqual("unsupported"); expect(classifyError("requests.HTTPError: 404 for https://example.com/report")).toEqual("unsupported"); @@ -173,6 +168,8 @@ describe("AiWorkflowFixerService", () => { it("returns unsupported for out-of-scope, empty, and near-miss messages", () => { expect(classifyError("ZeroDivisionError: division by zero")).toEqual("unsupported"); + // Only a JVM operator raises this, and a JVM operator has no code for Apply to patch. + expect(classifyError("java.lang.NullPointerException: boom")).toEqual("unsupported"); expect(classifyError("")).toEqual("unsupported"); expect(classifyError("TypeError: 'int' object is not iterable")).toEqual("unsupported"); }); @@ -187,14 +184,11 @@ describe("AiWorkflowFixerService", () => { await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); expect(seen).toEqual(["idle", "analyzing", "ready"]); - expect(state().errorType).toEqual("missing_column"); expect(state().suggestedFix).toEqual({ - type: "code_change", original: " yield t['email']", suggested: " yield t['user_email']", explanation: "the column is user_email", confidence: "high", - fieldName: undefined, // CODE holds the snippet once, and the panel only shows this when it is more. occurrences: 1, }); @@ -225,25 +219,25 @@ describe("AiWorkflowFixerService", () => { expect(state().suggestedFix).toBeUndefined(); }); + it("refuses a reply that proposes a configuration change", async () => { + // The panel writes `code` and nothing else. Nothing in the repo raises an error a + // configuration change could fix -- a HuggingFace 404 is retried internally and ends + // up in the result rows, never as an exception -- so a property fix can only be the + // model going off script, and it should not reach the screen. + stubModel(suggestion({ fix_type: "property_change", original_snippet: "modelId", suggested_snippet: "gpt-4" })); + + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + + expect(state().status).toEqual("error"); + expect(state().suggestedFix).toBeUndefined(); + }); + it("tolerates a fenced JSON response even though the prompt forbids markdown", async () => { stubModel("```json\n" + suggestion() + "\n```"); await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); expect(state().status).toEqual("ready"); }); - it("shows the current field value as the original for a property change", async () => { - operatorProperties = { model: "gpt-4-turb" }; - stubModel( - suggestion({ fix_type: "property_change", original_snippet: "model", suggested_snippet: "gpt-4-turbo" }) - ); - - await service.analyzeError(OP, err("Error code: 404 - model not found"), SCHEMA, undefined, operatorProperties); - - expect(state().suggestedFix?.fieldName).toEqual("model"); - expect(state().suggestedFix?.original).toEqual("gpt-4-turb"); - expect(state().suggestedFix?.suggested).toEqual("gpt-4-turbo"); - }); - it("never calls the model for an out-of-scope error", async () => { const callModel = vi.spyOn(service as any, "callModel"); await service.analyzeError(OP, err("ZeroDivisionError: division by zero"), SCHEMA, CODE, operatorProperties); @@ -293,10 +287,10 @@ describe("AiWorkflowFixerService", () => { // Operators keep secrets beside ordinary settings. This asserts the wiring, not the // filter: ai-fix-prompt.spec covers withoutSecrets itself, and the bug worth catching // here is the service forgetting to call it. - operatorProperties = { hfApiToken: "hf_live_abc", password: "hunter2", modelId: "gpt-4-turb" }; + operatorProperties = { code: CODE, hfApiToken: "hf_live_abc", password: "hunter2", modelId: "gpt-4-turb" }; const callModel = vi.spyOn(service as any, "callModel").mockResolvedValue({ text: suggestion() }); - await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); const prompt = JSON.stringify(callModel.mock.calls[0][0]); expect(prompt).not.toContain("hf_live_abc"); @@ -305,12 +299,14 @@ describe("AiWorkflowFixerService", () => { }); describe("classifying a real traceback", () => { - it("classifies a JVM exception whose frames no pattern could recognise", () => { + it("reads a JVM exception from the header rather than the frames below it", () => { // mkConsoleMessage puts `err.toString` in `title` and only `getStackTrace.mkString` in // `message`, so the frames arrive with no `at ` prefix -- and on Java 9+ the JDK's own - // carry a module name. A real worker trace ends on exactly such a frame, which is why - // deducing the exception from the joined text classified the frame instead. - expect(classifyError("java.lang.NullPointerException: value is null")).toEqual("null_error"); + // carry a module name. Nothing has to find the exception any more, because the header + // is handed over separately. It classifies as unsupported, which is the honest answer: + // the fix this panel writes is a change to `code`, and a JVM operator has none. + expect(classifyError("java.lang.NullPointerException: value is null")).toEqual("unsupported"); + expect(classifyError("java.base/java.lang.Thread.run(Thread.java:840)")).toEqual("unsupported"); }); it("classifies a chained traceback on the exception that stopped the worker", () => { @@ -342,22 +338,6 @@ describe("AiWorkflowFixerService", () => { }); }); - describe("coerceToFieldType", () => { - it("keeps a numeric field numeric", () => { - expect(coerceToFieldType(0, "4")).toEqual(4); - expect(() => coerceToFieldType(0, "four")).toThrow(); - }); - - it("keeps a boolean field boolean", () => { - expect(coerceToFieldType(false, "true")).toEqual(true); - expect(() => coerceToFieldType(false, "yes")).toThrow(); - }); - - it("leaves a string field alone", () => { - expect(coerceToFieldType("gpt-4-turb", "gpt-4-turbo")).toEqual("gpt-4-turbo"); - }); - }); - it("lets an unsupported error supersede an analysis already in flight", async () => { // The unsupported branch answers without calling the model, but it still replaces the // panel state, so a slower analysis started earlier must not land on top of it. @@ -614,38 +594,6 @@ describe("AiWorkflowFixerService", () => { expect(patched.match(/t\['user_email'\]/g)).toHaveLength(2); }); - it("patches the named field for a property change", async () => { - operatorProperties = { model: "gpt-4-turb", temperature: 0 }; - stubModel( - suggestion({ fix_type: "property_change", original_snippet: "model", suggested_snippet: "gpt-4-turbo" }) - ); - await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); - - await service.applyFix(); - - expect(setOperatorProperty).toHaveBeenCalledWith(OP, { model: "gpt-4-turbo", temperature: 0 }); - expect(executeWorkflow).toHaveBeenCalledWith(""); - expect(state().status).toEqual("applied"); - }); - - it("reports an apply failure, without re-running, when the target field changed since the analysis", async () => { - operatorProperties = { model: "gpt-4-turb", temperature: 0 }; - stubModel( - suggestion({ fix_type: "property_change", original_snippet: "model", suggested_snippet: "gpt-4-turbo" }) - ); - await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); - - // A coeditor picks a different model after the suggestion was generated: applying the - // stale suggestion would silently discard their choice and re-run on it. - operatorProperties = { model: "claude-haiku-4.5", temperature: 0 }; - - await service.applyFix(); - - expect(setOperatorProperty).not.toHaveBeenCalled(); - expect(executeWorkflow).not.toHaveBeenCalled(); - expect(state().status).toEqual("apply_failed"); - }); - it("does nothing without a ready suggestion", async () => { await service.applyFix(); expect(setOperatorProperty).not.toHaveBeenCalled(); @@ -851,22 +799,6 @@ describe("AiWorkflowFixerService", () => { expect(state().suggestedFix).toBeUndefined(); }); - it("leaves a structured setting alone rather than writing a string into it", async () => { - // The model always replies with a string; writing it into an array field replaced - // outputColumns with the text "[]". - operatorProperties = { outputColumns: [{ attributeName: "x", attributeType: "integer" }], workers: 1 }; - stubModel( - suggestion({ fix_type: "property_change", original_snippet: "outputColumns", suggested_snippet: "[]" }) - ); - await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); - - await service.applyFix(); - - expect(setOperatorProperty).not.toHaveBeenCalled(); - expect(executeWorkflow).not.toHaveBeenCalled(); - expect(state().applyError).toContain("structured data"); - }); - it("does not report an apply over an analysis started while it waited", async () => { // Reachable: Apply on one operator, then switch to another whose frame shows idle and // click Analyze there. The order is forced here so the newer analysis settles first, @@ -888,40 +820,6 @@ describe("AiWorkflowFixerService", () => { expect(state().status).toEqual("ready"); }); - it("refuses to touch a field the model was never shown", async () => { - // withoutSecrets keeps credentials out of the prompt, but the apply path checked the - // raw properties, so a guessed name like hfApiToken passed the check -- and toFix read - // the current value from the same raw set, putting the secret on screen in the diff. - operatorProperties = { hfApiToken: "hf_live_abc", modelId: "gpt-4-turb", workers: 1 }; - stubModel( - suggestion({ fix_type: "property_change", original_snippet: "hfApiToken", suggested_snippet: "hf_new" }) - ); - - await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); - - expect(JSON.stringify(state())).not.toContain("hf_live_abc"); - await service.applyFix(); - expect(setOperatorProperty).not.toHaveBeenCalled(); - }); - - it("only accepts a property change for the error type that calls for one", async () => { - // The prompt asks for a configuration change in exactly one case; a property fix for - // any other error is the model going off script. - operatorProperties = { modelId: "gpt-4-turb", workers: 1 }; - stubModel( - suggestion({ fix_type: "property_change", original_snippet: "modelId", suggested_snippet: "gpt-4-turbo" }) - ); - - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); - - // Refused by the analysis, not by Apply: a diff the write will never honour should - // never be on screen, and refusing at the click costs the paused run as well. - expect(state().status).toEqual("error"); - expect(state().suggestedFix).toBeUndefined(); - await service.applyFix(); - expect(setOperatorProperty).not.toHaveBeenCalled(); - }); - it("refuses on a read-only workflow instead of killing someone else's run", async () => { // enableWorkflowModification() is a no-op while the workflow is read-only, so the // unlock this waits for never arrives. Running only needs write access to the @@ -955,71 +853,6 @@ describe("AiWorkflowFixerService", () => { expect(state().applyError).toContain("other than a running execution"); }); - it("refuses a field the operator does not have", async () => { - // The staleness guard does not catch this one: an absent field reads as "" and the - // captured original is "" too, so without the check a dead key is written and the - // real setting stays wrong. - operatorProperties = { modelId: "gpt-4-turb", workers: 1 }; - stubModel(suggestion({ fix_type: "property_change", original_snippet: "modelName", suggested_snippet: "Qwen" })); - - await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); - - expect(state().status).toEqual("error"); - await service.applyFix(); - expect(setOperatorProperty).not.toHaveBeenCalled(); - }); - - it("refuses a field that disappeared between the analysis and the write", async () => { - // The analysis rejects a field the operator does not have, so this is the only way the - // apply-side check is still reachable: a coeditor retypes the operator while the - // suggestion is on screen, and the field it names is gone by the time Apply re-reads - // the properties. Writing then would create a dead key and leave the real setting wrong. - // - // The field is empty rather than set, because the staleness guard would otherwise cover - // this on its own: an absent field reads as "" and would no longer match the captured - // original. With both reading "", only the presence check can refuse. - operatorProperties = { modelId: "", workers: 1 }; - stubModel( - suggestion({ fix_type: "property_change", original_snippet: "modelId", suggested_snippet: "gpt-4-turbo" }) - ); - await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); - expect(state().status).toEqual("ready"); - - operatorProperties = { workers: 1 }; - await service.applyFix(); - - expect(setOperatorProperty).not.toHaveBeenCalled(); - expect(executeWorkflow).not.toHaveBeenCalled(); - expect(state().status).toEqual("apply_failed"); - }); - - it("never puts the UDF source in the diff through a property change", async () => { - // Naming "code" hands the whole operator source to the property branch, which reads - // the field's current value as the diff's "before". Refusing only at Apply still let - // the panel render the entire source first, as one unwrapped line. - operatorProperties = { code: "print(1)", workers: 1 }; - stubModel(suggestion({ fix_type: "property_change", original_snippet: "code", suggested_snippet: "print(2)" })); - - await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); - - expect(state().status).toEqual("error"); - expect(state().suggestedFix).toBeUndefined(); - await service.applyFix(); - expect(setOperatorProperty).not.toHaveBeenCalled(); - }); - - it("keeps a numeric setting numeric", async () => { - // original_snippet carries the field name for a property change; toFix reads the - // current value from the operator itself. - operatorProperties = { temperature: 0, workers: 1 }; - stubModel(suggestion({ fix_type: "property_change", original_snippet: "temperature", suggested_snippet: "1" })); - await service.analyzeError(OP, err("404 model not found"), SCHEMA, undefined, operatorProperties); - - await service.applyFix(); - - expect(setOperatorProperty.mock.calls[0][1].temperature).toEqual(1); - }); - it("reports an apply failure when the graph rejects the change", async () => { setOperatorProperty.mockImplementation(() => { throw new Error("read-only workflow"); diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts index 89f73acd7c4..df3641d3948 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts @@ -32,7 +32,7 @@ import { buildFixPrompt, withoutSecrets } from "./ai-fix-prompt"; import { OperatorMetadataService } from "../../../service/operator-metadata/operator-metadata.service"; import { WorkflowConsoleService } from "../../../service/workflow-console/workflow-console.service"; -export type FixErrorType = "missing_column" | "type_error" | "null_error" | "model_not_found" | "unsupported"; +export type FixErrorType = "missing_column" | "type_error" | "null_error" | "unsupported"; /** * One operator error, kept as the two parts its source already distinguishes. * @@ -51,14 +51,12 @@ export type FixConfidence = "high" | "medium" | "low"; export interface SuggestedFix { /** How many places Apply will touch; the diff only ever shows one -/+ pair. */ occurrences?: number; - type: "code_change" | "property_change"; + /** Whole lines quoted from the operator's code, and what to put in their place. */ original: string; suggested: string; explanation: string; - // Rendered as the confidence badge; `fieldName` names the patched property - // (property_change only) so applyFix knows which field to write. + /** Rendered as the confidence badge. */ confidence: FixConfidence; - fieldName?: string; } export interface FixState { @@ -76,14 +74,8 @@ export interface FixState { * switched workflows could be applied to the wrong one. */ workflowId?: number; - /** - * Which of the four supported shapes the error was classified as. Only the apply path - * reads it, to refuse a configuration change for anything but `model_not_found`; an error - * outside those shapes is the `unsupported` status, not a value here. - */ - errorType?: FixErrorType; suggestedFix?: SuggestedFix; - // `unsupported`: the error is outside the four shapes this panel handles, reported without + // `unsupported`: the error is outside the three shapes this panel handles, reported without // spending a model call. `applied_without_run`: the fix was written but the deployment // refused to start a run (a warehouse is required and none is selected), so the panel must // not claim one. `apply_failed` keeps the suggestion on screen: the analysis was fine, the @@ -139,8 +131,7 @@ export const IDLE_STATE: FixState = { operatorId: "", status: "idle" }; const PATTERNS: ReadonlyArray<[RegExp, FixErrorType]> = [ [/KeyError:\s*['"][^'"]+['"]/, "missing_column"], [/TypeError:\s*unsupported operand/i, "type_error"], - [/ValueError:[^\n]*\bNaN\b|NullPointerException/i, "null_error"], - [/ModelNotFound|\bmodel[^\n]*\b404\b|\b404\b[^\n]*\bmodel/i, "model_not_found"], + [/ValueError:[^\n]*\bNaN\b/i, "null_error"], ]; /** @@ -184,31 +175,6 @@ export function matchesWholeLines(code: string, original: string): boolean { } } -/** Writes the model's string back in the type the field already held. */ -export function coerceToFieldType(currentValue: unknown, suggested: string): unknown { - // Arrays and objects are out of scope: the model replies with a string, and writing it - // into a structured field replaced e.g. an outputColumns array with the text "[]". - // Validating a replacement against the operator's schema is a bigger change than the four - // error shapes this panel handles need, so it refuses instead of guessing. - if (currentValue !== null && typeof currentValue === "object") { - throw new Error("this setting holds structured data, which an automatic fix does not edit"); - } - if (typeof currentValue === "number") { - const parsed = Number(suggested); - if (Number.isNaN(parsed)) { - throw new Error(`${suggested} is not a number`); - } - return parsed; - } - if (typeof currentValue === "boolean") { - if (suggested !== "true" && suggested !== "false") { - throw new Error(`${suggested} is not a boolean`); - } - return suggested === "true"; - } - return suggested; -} - /** * Classifies on the exception alone, never on the stack around it. * @@ -293,7 +259,6 @@ export class AiWorkflowFixerService { const base: FixState = { operatorId, workflowId, - errorType, errorKey: this.latestErrorKey(operatorId), status: "analyzing", }; @@ -319,31 +284,23 @@ export class AiWorkflowFixerService { return; } const suggestedFix = this.toFix(text, visible); - // The prompt asks for a configuration change in exactly one case, so a property fix - // for any other error is the model going off script. Refused here rather than only at - // Apply: the panel must not show a diff the write will never honour. - if (suggestedFix.type === "property_change" && errorType !== "model_not_found") { - throw new Error("a configuration change is not the fix for this error"); + // The prompt asks for whole lines with their indentation; a snippet starting mid-line + // means the model ignored that, and applying it would put the lines after the first + // at a column nothing here can work out. + if (!matchesWholeLines(code ?? "", suggestedFix.original)) { + throw new Error("the model did not quote whole lines, so the fix cannot be placed"); } // Counted here so the panel can say it before Apply: a short snippet matched as plain // text can appear in lines the user never saw in the diff. - if (suggestedFix.type === "code_change") { - // The prompt asks for whole lines with their indentation; a snippet starting mid-line - // means the model ignored that, and applying it would put the lines after the first - // at a column nothing here can work out. - if (!matchesWholeLines(code ?? "", suggestedFix.original)) { - throw new Error("the model did not quote whole lines, so the fix cannot be placed"); - } - const occurrences = replaceAll(code ?? "", suggestedFix.original, suggestedFix.suggested).count; - // Zero means the model quoted something the operator does not contain. The panel - // only surfaces this count when it is above one, so such a suggestion is - // indistinguishable from any other, and Apply turns it away as a stale snippet -- - // blaming an edit that never happened. - if (occurrences === 0) { - throw new Error(`the code does not contain ${JSON.stringify(suggestedFix.original)}`); - } - suggestedFix.occurrences = occurrences; + const occurrences = replaceAll(code ?? "", suggestedFix.original, suggestedFix.suggested).count; + // Zero means the model quoted something the operator does not contain. The panel + // only surfaces this count when it is above one, so such a suggestion is + // indistinguishable from any other, and Apply turns it away as a stale snippet -- + // blaming an edit that never happened. + if (occurrences === 0) { + throw new Error(`the code does not contain ${JSON.stringify(suggestedFix.original)}`); } + suggestedFix.occurrences = occurrences; this.stateSubject.next({ ...base, status: "ready", suggestedFix }); } catch (err) { console.error("AI workflow fixer: analysis failed", err); @@ -423,48 +380,24 @@ export class AiWorkflowFixerService { const properties: Record = { ...this.workflowActionService.getTexeraGraph().getOperator(current.operatorId).operatorProperties, }; - if (fix.type === "code_change") { - const code = String(properties.code ?? ""); - const patched = replaceAll(code, fix.original, fix.suggested); - if (patched.count === 0) { - throw new Error("the code changed since the suggestion was generated"); - } - // Counted, not just found. The count behind the "changes N places" badge was taken at - // the analysis, and this replaces every match -- so a copy added or removed in between - // means the write touches a different number of lines than the panel promised. A kill - // lifts the modification lock without resetting the panel, so the user can edit the - // UDF with the suggestion still on screen. - if (fix.occurrences !== undefined && patched.count !== fix.occurrences) { - const places = (n: number) => `${n} place${n === 1 ? "" : "s"}`; - throw new Error( - `the snippet is now in ${places(patched.count)} instead of ${places(fix.occurrences)}, ` + - "so the fix was not applied" - ); - } - properties.code = patched.code; - } else { - // No errorType check here. The analysis refuses a property fix for anything but - // model_not_found, and errorType is fixed when the state is published, so by this - // point it can only be that -- unlike the field checks below, which guard against the - // operator moving between the analysis and the write. - const field = String(fix.fieldName); - // Checked against the fields the model was actually shown, not the raw properties: - // those still hold the credentials withoutSecrets kept out of the prompt, so a - // guessed `hfApiToken` would otherwise pass here and be overwritten. - const visible = withoutSecrets(properties, this.schemaProperties(current.operatorId)); - if (field === "code" || !Object.prototype.hasOwnProperty.call(visible, field)) { - throw new Error(`${field} is not a configuration field this fix may change`); - } - // Same staleness guard as the code branch: re-reading the properties keeps the - // unrelated fields, but the field being replaced can itself have moved on since - // the suggestion was generated, and overwriting it would discard that newer value. - if (String(properties[field] ?? "") !== fix.original) { - throw new Error(`${field} changed since the suggestion was generated`); - } - // The model always replies with a string; writing it raw would turn a numeric or - // boolean setting into one, which the operator then rejects at validation. - properties[field] = coerceToFieldType(properties[field], fix.suggested); + const code = String(properties.code ?? ""); + const patched = replaceAll(code, fix.original, fix.suggested); + if (patched.count === 0) { + throw new Error("the code changed since the suggestion was generated"); + } + // Counted, not just found. The count behind the "changes N places" badge was taken at + // the analysis, and this replaces every match -- so a copy added or removed in between + // means the write touches a different number of lines than the panel promised. A kill + // lifts the modification lock without resetting the panel, so the user can edit the + // UDF with the suggestion still on screen. + if (fix.occurrences !== undefined && patched.count !== fix.occurrences) { + const places = (n: number) => `${n} place${n === 1 ? "" : "s"}`; + throw new Error( + `the snippet is now in ${places(patched.count)} instead of ${places(fix.occurrences)}, ` + + "so the fix was not applied" + ); } + properties.code = patched.code; return properties; } @@ -622,25 +555,17 @@ export class AiWorkflowFixerService { throw new Error(`model returned ${typeof value} for ${name}, expected a string`); } } - const isProperty = fix_type === "property_change"; - // Checked here, not only at Apply: `original` for a property change is the field's - // CURRENT value, so an unknown name renders an empty "before" and `code` renders the - // entire UDF source as one unwrapped diff line. A suggestion the write would refuse - // should never reach the screen in the first place. - if ( - isProperty && - (original_snippet === "code" || !Object.prototype.hasOwnProperty.call(properties, original_snippet)) - ) { - throw new Error(`${original_snippet} is not a configuration field this fix may change`); + // This panel edits code and nothing else, so any other kind of fix is the model going + // off script -- and refusing here keeps it off the screen rather than only out of the + // write. + if (fix_type !== "code_change") { + throw new Error(`${fix_type} is not a kind of fix this panel applies`); } return { - type: isProperty ? "property_change" : "code_change", - // For a property change the model returns the field name, so show its current value. - original: isProperty ? String(properties[original_snippet] ?? "") : original_snippet, + original: original_snippet, suggested: suggested_snippet, explanation, confidence: confidence === "high" || confidence === "low" ? confidence : "medium", - fieldName: isProperty ? original_snippet : undefined, }; } } diff --git a/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts b/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts index dc57d9fae8c..3f388a6eae3 100644 --- a/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts @@ -433,6 +433,13 @@ describe("ResultPanelComponent", () => { // traceback arrives as an ERROR console message with the execution still Running, // so the Failed branch never fires for the case the AI panel exists to fix. workflowActionService.addOperator(mockResultPredicate, mockPoint); + // The fix this panel writes is a change to `code`, so it only offers itself on a + // Python UDF. The test metadata does not know the V2 types, so the graph is told. + vi.spyOn(workflowActionService.getTexeraGraph(), "getOperator").mockReturnValue({ + ...mockResultPredicate, + operatorID: "3", + operatorType: PYTHON_UDF_V2_OP_TYPE, + }); const consoleService = TestBed.inject(WorkflowConsoleService); const config = TestBed.inject(GuiConfigService); (config.env as any).copilotEnabled = true; @@ -453,6 +460,13 @@ describe("ResultPanelComponent", () => { // of this test -- the wiring, not the branch, was what was missing. vi.useFakeTimers(); workflowActionService.addOperator(mockResultPredicate, mockPoint); + // The fix this panel writes is a change to `code`, so it only offers itself on a + // Python UDF. The test metadata does not know the V2 types, so the graph is told. + vi.spyOn(workflowActionService.getTexeraGraph(), "getOperator").mockReturnValue({ + ...mockResultPredicate, + operatorID: "3", + operatorType: PYTHON_UDF_V2_OP_TYPE, + }); const consoleService = TestBed.inject(WorkflowConsoleService); (TestBed.inject(GuiConfigService).env as any).copilotEnabled = true; vi.spyOn(workflowActionService.getJointGraphWrapper(), "getCurrentHighlightedOperatorIDs").mockReturnValue(["3"]); @@ -470,6 +484,13 @@ describe("ResultPanelComponent", () => { // A re-run clears the console. Without the teardown the tab lingered, offering to fix // an error that is no longer there. workflowActionService.addOperator(mockResultPredicate, mockPoint); + // The fix this panel writes is a change to `code`, so it only offers itself on a + // Python UDF. The test metadata does not know the V2 types, so the graph is told. + vi.spyOn(workflowActionService.getTexeraGraph(), "getOperator").mockReturnValue({ + ...mockResultPredicate, + operatorID: "3", + operatorType: PYTHON_UDF_V2_OP_TYPE, + }); const consoleService = TestBed.inject(WorkflowConsoleService); (TestBed.inject(GuiConfigService).env as any).copilotEnabled = true; vi.spyOn(workflowActionService.getJointGraphWrapper(), "getCurrentHighlightedOperatorIDs").mockReturnValue(["3"]); @@ -488,6 +509,13 @@ describe("ResultPanelComponent", () => { // position and debug input and the Result table were reset whenever any operator // printed. The console stream must touch the AI Fix entry and nothing else. workflowActionService.addOperator(mockResultPredicate, mockPoint); + // The fix this panel writes is a change to `code`, so it only offers itself on a + // Python UDF. The test metadata does not know the V2 types, so the graph is told. + vi.spyOn(workflowActionService.getTexeraGraph(), "getOperator").mockReturnValue({ + ...mockResultPredicate, + operatorID: "3", + operatorType: PYTHON_UDF_V2_OP_TYPE, + }); const consoleService = TestBed.inject(WorkflowConsoleService); (TestBed.inject(GuiConfigService).env as any).copilotEnabled = true; vi.spyOn(workflowActionService.getJointGraphWrapper(), "getCurrentHighlightedOperatorIDs").mockReturnValue(["3"]); @@ -513,6 +541,13 @@ describe("ResultPanelComponent", () => { // to `true` again, is read as "no change", and the tab never comes back. That is the // fix-and-re-run loop this panel exists for. workflowActionService.addOperator(mockResultPredicate, mockPoint); + // The fix this panel writes is a change to `code`, so it only offers itself on a + // Python UDF. The test metadata does not know the V2 types, so the graph is told. + vi.spyOn(workflowActionService.getTexeraGraph(), "getOperator").mockReturnValue({ + ...mockResultPredicate, + operatorID: "3", + operatorType: PYTHON_UDF_V2_OP_TYPE, + }); const consoleService = TestBed.inject(WorkflowConsoleService); (TestBed.inject(GuiConfigService).env as any).copilotEnabled = true; vi.spyOn(workflowActionService.getJointGraphWrapper(), "getCurrentHighlightedOperatorIDs").mockReturnValue(["3"]); @@ -549,6 +584,23 @@ describe("ResultPanelComponent", () => { expect(displayNameStream).not.toHaveBeenCalled(); }); + it("registers no AI fix frame on an operator whose fix would be unwritable", () => { + // The only fix this panel writes is a change to `code`. A JVM operator can report an + // error the classifier recognises and still have nothing to patch, so offering the tab + // there spends a model call to reach a refusal. mockResultPredicate is not a UDF. + workflowActionService.addOperator(mockResultPredicate, mockPoint); + const consoleService = TestBed.inject(WorkflowConsoleService); + (TestBed.inject(GuiConfigService).env as any).copilotEnabled = true; + vi.spyOn(workflowActionService.getJointGraphWrapper(), "getCurrentHighlightedOperatorIDs").mockReturnValue(["3"]); + vi.spyOn(consoleService, "hasConsoleMessages").mockReturnValue(true); + vi.spyOn(consoleService, "getConsoleMessages").mockReturnValue([consoleMessage("ERROR")]); + + component.rerenderResultPanel(); + + expect(component.frameComponentConfigs.has("Console")).toBe(true); + expect(component.frameComponentConfigs.has("AI Fix")).toBe(false); + }); + it("registers no AI fix frame when the operator's console holds no ERROR", () => { workflowActionService.addOperator(mockResultPredicate, mockPoint); const consoleService = TestBed.inject(WorkflowConsoleService); diff --git a/frontend/src/app/workspace/component/result-panel/result-panel.component.ts b/frontend/src/app/workspace/component/result-panel/result-panel.component.ts index 040ca9d414d..692dae2e78d 100644 --- a/frontend/src/app/workspace/component/result-panel/result-panel.component.ts +++ b/frontend/src/app/workspace/component/result-panel/result-panel.component.ts @@ -282,11 +282,17 @@ export class ResultPanelComponent implements OnInit, OnDestroy { /** True when the selected operator reports an error the AI panel can be opened on. */ private shouldOfferAiFix(): boolean { - return ( - this.config.env.copilotEnabled && - this.currentOperatorId !== undefined && - this.hasConsoleError(this.currentOperatorId) - ); + if (!this.config.env.copilotEnabled || this.currentOperatorId === undefined) { + return false; + } + if (!this.hasConsoleError(this.currentOperatorId)) { + return false; + } + // Python UDFs only, because the fix this panel writes is a change to `code`. A JVM + // operator can report an error the classifier recognises and still have nothing to + // patch, so offering the tab there spends a model call to reach a refusal. + const graph = this.workflowActionService.getTexeraGraph(); + return graph.hasOperator(this.currentOperatorId) && isPythonUdf(graph.getOperator(this.currentOperatorId)); } /** From 7873a6ba38909dbcc5cfa0100a1151e1301c3b07 Mon Sep 17 00:00:00 2001 From: Pedro Date: Mon, 26 Oct 2026 20:28:03 -0600 Subject: [PATCH 37/39] refactor(frontend): drop a parameter the deleted branch was the only reader of toFix still took the redacted properties, which it read to turn a field name into the field's current value for the diff. That branch went with the configuration path, so the parameter has been along for the ride since. Found by auditing what the deletion left behind rather than by a test or the linter, neither of which minds an unused parameter. Co-Authored-By: Claude Opus 5 --- .../result-panel/ai-fix/ai-workflow-fixer.service.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts index df3641d3948..a7e96b22b3a 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts @@ -283,7 +283,7 @@ export class AiWorkflowFixerService { if (seq !== this.analysisSeq || workflowId !== this.workflowActionService.getOpenWorkflowId()) { return; } - const suggestedFix = this.toFix(text, visible); + const suggestedFix = this.toFix(text); // The prompt asks for whole lines with their indentation; a snippet starting mid-line // means the model ignored that, and applying it would put the lines after the first // at a column nothing here can work out. @@ -527,7 +527,7 @@ export class AiWorkflowFixerService { } /** Maps the model's JSON contract onto SuggestedFix, rejecting incomplete replies. */ - private toFix(raw: string, properties: Readonly>): SuggestedFix { + private toFix(raw: string): SuggestedFix { let text = raw.trim(); const fenced = text.match(/```(?:[a-zA-Z]+)?\s*([\s\S]*?)```/); if (fenced) { From d4b8068e15d9b8c17c1c20045837e01232d8749c Mon Sep 17 00:00:00 2001 From: Pedro Date: Mon, 26 Oct 2026 21:28:37 -0600 Subject: [PATCH 38/39] fix(frontend): stop sending operator configuration, and stop refusing good replies @kunwp1's three follow-ups. **A credential could still reach the model.** withoutSecrets filters top-level field names, but a Python UDF's `uiParameters` is a list of {attribute, value} pairs, so the name says nothing about whether a value is an API key the user typed in -- and the whole list went into the configuration block. Reproduced before fixing: an `hf_live_...` value appears verbatim in the built prompt. Filtering harder was not the fix. None of the three remaining shapes reads configuration, so the block served nothing, and it is gone -- taking withoutSecrets, withoutCode, schemaProperties and the OperatorMetadataService dependency with it. analyzeError no longer takes the properties either. The PR body claimed credentials were kept out behind two barriers; now there is nothing to keep out. **The whole-line check refused valid replies.** A snippet quoted with its trailing newline left `code[end]` pointing at the next line's first character, so the same reply was refused in the middle of a file and accepted on its last line -- the verdict depended on where the line sat. One trailing newline comes off each snippet now, independently, so the replacement leaves the line's own newline in place and merges nothing. The prompt says the newline is optional rather than leaving it to chance. **`fix_type` is gone.** With one legal value it could only ever refuse a good answer; a reply naming a configuration field is already turned away by the occurrence count, which is what that test now says it is relying on. The `properties` parameter in the same comment went in 7873a6ba, and the stale "four supported fixes" wording went with the configuration block. Co-Authored-By: Claude Opus 5 --- .../ai-fix/ai-fix-frame.component.spec.ts | 5 +- .../ai-fix/ai-fix-frame.component.ts | 3 +- .../result-panel/ai-fix/ai-fix-prompt.spec.ts | 57 +------ .../result-panel/ai-fix/ai-fix-prompt.ts | 56 +----- .../ai-fix/ai-workflow-fixer.service.spec.ts | 159 +++++++++++------- .../ai-fix/ai-workflow-fixer.service.ts | 48 ++---- 6 files changed, 130 insertions(+), 198 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.spec.ts index d24966ac4b3..d983909b779 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.spec.ts @@ -135,7 +135,7 @@ describe("AiFixFrameComponent", () => { }); describe("the analyze button", () => { - it("passes the fatal error, schema, code and properties to the service", () => { + it("passes the fatal error, schema and code to the service", () => { getErrorMessages.mockReturnValue([fatalError({ operatorId: "other" }), fatalError()]); query(".ai-fix-analyze").nativeElement.click(); @@ -146,8 +146,7 @@ describe("AiFixFrameComponent", () => { { header: "KeyError: 'email'", full: "KeyError: 'email'\nTraceback ..." }, // Every input port, not just port 0: a UDF downstream of a join has more than one. { "0_false": [{ attributeName: "user_email", attributeType: "string" }] }, - "yield t['email']", - operatorProperties + "yield t['email']" ); }); diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.ts index 8cc3b7ef93d..acfb357d475 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.ts @@ -114,8 +114,7 @@ export class AiFixFrameComponent { operatorId, error, this.workflowCompilingService.getOperatorInputSchemaMap(operatorId), - properties.code as string | undefined, - properties + properties.code as string | undefined ); } diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.spec.ts index fbb00ea4375..d5776751411 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.spec.ts @@ -18,59 +18,18 @@ */ import { describe, expect, it } from "vitest"; -import { buildFixPrompt, formatInputSchema, withoutSecrets } from "./ai-fix-prompt"; +import { buildFixPrompt, formatInputSchema } from "./ai-fix-prompt"; import { PortSchema } from "../../../types/workflow-compiling.interface"; -describe("withoutSecrets", () => { - const passwordWidget = { widget: { formlyConfig: { templateOptions: { type: "password" } } } }; - - it("drops a field the operator's schema renders as a password widget", () => { - const kept = withoutSecrets({ host: "db.example.com", pw: "hunter2" }, { host: {}, pw: passwordWidget }); - - expect(kept).toEqual({ host: "db.example.com" }); - }); - - it("drops credential-looking names even without a schema", () => { - const kept = withoutSecrets({ - hfApiToken: "hf_live_abc", - password: "hunter2", - apiKey: "sk-live", - api_key: "sk-live", - clientSecret: "shh", - modelId: "Qwen/Qwen2.5", - }); - - expect(kept).toEqual({ modelId: "Qwen/Qwen2.5" }); - }); - - it("keeps the ordinary settings a fix actually needs", () => { - const kept = withoutSecrets({ modelId: "gpt-4-turb", temperature: 0, workers: 1 }, { modelId: {} }); - - expect(kept).toEqual({ modelId: "gpt-4-turb", temperature: 0, workers: 1 }); - }); - - it("never lets a secret reach the built prompt", () => { - const properties = { hfApiToken: "hf_live_abc", password: "hunter2", modelId: "gpt-4-turb" }; - - const prompt = buildFixPrompt("404 model not found", undefined, {}, withoutSecrets(properties)); - - expect(prompt).not.toContain("hf_live_abc"); - expect(prompt).not.toContain("hunter2"); - expect(prompt).toContain("gpt-4-turb"); - }); -}); - describe("buildFixPrompt", () => { - it("sends the UDF source once, not again inside the configuration", () => { - // `code` is a property like any other, so the whole operator source was rendered twice: - // fenced under "Operator code" and JSON-escaped under the configuration. The escaped copy - // is the one the model must not quote from, since no snippet taken from it matches. - const code = "class ProcessTupleOperator(UDFOperatorV2):\n pass\n"; + it("sends no operator configuration at all, so a UI parameter cannot leak", () => { + // The configuration block is not sent at all. Filtering it by field name could never be + // enough: a Python UDF's uiParameters is a list of {attribute, value} pairs, so the name + // says nothing about whether a value is an API key the user typed in. + const prompt = buildFixPrompt("KeyError: 'email'", "print(1)", {}); - const prompt = buildFixPrompt("KeyError: 'email'", code, {}, { code, workers: 1 }); - - expect(prompt.split("class ProcessTupleOperator").length - 1).toEqual(1); - expect(prompt).toContain('"workers": 1'); + expect(prompt).not.toContain("hf_live_abc"); + expect(prompt).not.toContain("Operator configuration"); }); }); diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts index 96519095d53..cbc9bca6199 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-prompt.ts @@ -19,48 +19,6 @@ import { OperatorPortSchemaMap } from "../../../types/workflow-compiling.interface"; -// Names that carry a credential even when the operator forgot the password widget. -const SECRET_NAME = /token|password|secret|credential|api[-_]?key/i; - -/** - * Drops operator properties whose values are credentials, so they never reach the model. - * - * Operators keep secrets alongside ordinary settings -- `hfApiToken` on HuggingFaceInference, - * `password` on the SQL sources -- and the configuration block is serialized whole, so without - * this the credential travels with it. The operator's own schema marks those fields with the - * password widget, which is the reliable signal; the name pattern is a second barrier so a - * schema that cannot be read, or an operator missing the annotation, does not leak anyway. - * - * Dropped rather than masked: none of the four supported fixes touches a credential, so the - * model has no reason to know the field is there. - */ -export function withoutSecrets( - operatorProperties: Readonly>, - schemaProperties?: Readonly> -): Record { - const isPasswordWidget = (fieldSchema: unknown): boolean => - (fieldSchema as any)?.widget?.formlyConfig?.templateOptions?.type === "password"; - - return Object.fromEntries( - Object.entries(operatorProperties).filter( - ([name]) => !SECRET_NAME.test(name) && !isPasswordWidget(schemaProperties?.[name]) - ) - ); -} - -/** - * Drops the UDF source from the configuration block. - * - * `code` is a property like any other, so the whole operator source went into the prompt - * twice -- once fenced under "Operator code" and once JSON-escaped inside the configuration. - * The escaped copy is the one the model should never quote from, and an apply refuses a - * property change named `code` anyway. - */ -function withoutCode(properties: Readonly>): Record { - const { code, ...rest } = properties; - return rest; -} - /** * Renders every input port, not just the first. * @@ -82,6 +40,11 @@ export function formatInputSchema(inputSchemas: OperatorPortSchemaMap | undefine /** * Builds the single user message sent to the LLM for one failed operator. * + * The operator's configuration is not among the things sent. Every fix this panel writes is + * a change to `code`, so the block served nothing -- and it could not be made safe by + * filtering field names: a Python UDF's `uiParameters` is a list of attribute/value pairs, + * so the name says nothing about whether a value is an API key the user typed in. + * * The model is asked for exactly one minimal fix and for raw JSON. The reply is still parsed * defensively (fenced blocks are unwrapped) because "no markdown" is a request, not a * guarantee. @@ -89,8 +52,7 @@ export function formatInputSchema(inputSchemas: OperatorPortSchemaMap | undefine export function buildFixPrompt( errorMessage: string, operatorCode: string | undefined, - inputSchemas: OperatorPortSchemaMap | undefined, - operatorProperties: Readonly> + inputSchemas: OperatorPortSchemaMap | undefined ): string { return `You are a debugging assistant for a Python data processing operator in a workflow engine. @@ -105,8 +67,6 @@ ${operatorCode ?? "(this operator has no user code)"} Input schema (column names and types) ${formatInputSchema(inputSchemas)} -Operator configuration -${JSON.stringify(withoutCode(operatorProperties), null, 2)} Task Diagnose the root cause of the error. Then propose exactly ONE fix. @@ -119,12 +79,12 @@ Rules: - original_snippet and suggested_snippet must be WHOLE LINES, copied starting at the beginning of a line, with the leading indentation included exactly as it appears. Do not start a snippet in the middle of a line, and do not re-indent: every line you return must - already carry the indentation it should have in the file. + already carry the indentation it should have in the file. Ending a snippet with a newline + is fine; just end both snippets the same way. Respond ONLY with valid JSON, no markdown, no explanation outside the JSON: { "explanation": "one sentence describing the root cause", - "fix_type": "code_change", "original_snippet": "whole lines to replace, indentation included", "suggested_snippet": "replacement lines, each already indented as it should appear", "confidence": "high" | "medium" | "low" diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts index 0f6e0812ed9..b5c82437eb1 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.spec.ts @@ -52,6 +52,7 @@ const KEY_ERROR = "Traceback (most recent call last):\n File \"udf.py\", line 3\n yield t['email']\nKeyError: 'email'"; const QUOTE = String.fromCharCode(39); +const NL = String.fromCharCode(10); // Both sources hand the exception over on its own, so a test supplies it the same way: // `header` is what classification reads, `full` is what the model is shown. @@ -181,7 +182,7 @@ describe("AiWorkflowFixerService", () => { const seen: string[] = []; service.getState$().subscribe(s => seen.push(s.status)); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); expect(seen).toEqual(["idle", "analyzing", "ready"]); expect(state().suggestedFix).toEqual({ @@ -194,13 +195,42 @@ describe("AiWorkflowFixerService", () => { }); }); + it("accepts snippets quoted with their trailing newline", async () => { + // Whether the model includes the final newline is not something the prompt settles, + // and with it `code[end]` is the first character of the next line -- so the same valid + // reply was refused mid-file and accepted on the last line. One trailing newline is + // dropped from each snippet, independently, so neither the check nor the replacement + // sees it and no lines are merged. + // A line in the MIDDLE of the file: quoted with its newline, the next character is the + // start of the following line rather than a newline. The last line of a file passes + // either way, which is what hid this. + const code = "def f():" + NL + " a = read()" + NL + " yield a" + NL; + stubModel(suggestion({ original_snippet: " a = read()" + NL, suggested_snippet: " a = f()" + NL })); + + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, code); + + expect(state().status).toEqual("ready"); + expect(state().suggestedFix?.original).toEqual(" a = read()"); + expect(state().suggestedFix?.suggested).toEqual(" a = f()"); + }); + + it("still refuses a mid-line match quoted with a newline", async () => { + // Tolerating the newline must not tolerate what the end-of-line check was added for: + // once it is dropped, `n = 1` is again only the start of `n = 10`. + const code = "def f():\n n = 10\n"; + stubModel(suggestion({ original_snippet: " n = 1\n", suggested_snippet: " n = 2\n" })); + + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, code); + + expect(state().status).toEqual("error"); + }); it("refuses a snippet quoted from the middle of a line", async () => { // The prompt asks for whole lines carrying their indentation. A mid-line snippet says // nothing about the column its continuation lines belong at, and inferring one is the // guess that put `yield` at sixteen spaces in an eight-space body. stubModel(suggestion({ original_snippet: "yield t['email']", suggested_snippet: "yield t['user_email']" })); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); expect(state().status).toEqual("error"); expect(state().suggestedFix).toBeUndefined(); @@ -213,34 +243,44 @@ describe("AiWorkflowFixerService", () => { // happened. stubModel(suggestion({ original_snippet: " yield t['address']" })); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); expect(state().status).toEqual("error"); expect(state().suggestedFix).toBeUndefined(); }); - it("refuses a reply that proposes a configuration change", async () => { - // The panel writes `code` and nothing else. Nothing in the repo raises an error a - // configuration change could fix -- a HuggingFace 404 is retried internally and ends - // up in the result rows, never as an exception -- so a property fix can only be the - // model going off script, and it should not reach the screen. + it("refuses a reply that names a configuration field instead of quoting code", async () => { + // Not by reading fix_type -- that field is gone, since one legal value could only ever + // refuse a good reply. A field name is not in the code, so the occurrence count turns + // it away on its own. stubModel(suggestion({ fix_type: "property_change", original_snippet: "modelId", suggested_snippet: "gpt-4" })); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); expect(state().status).toEqual("error"); expect(state().suggestedFix).toBeUndefined(); }); + it("accepts a reply that omits fix_type", async () => { + // There is one kind of fix, so demanding the field only added a way for a good answer + // to be thrown away. + const { fix_type, ...withoutKind } = JSON.parse(suggestion()); + stubModel(JSON.stringify(withoutKind)); + + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); + + expect(state().status).toEqual("ready"); + }); + it("tolerates a fenced JSON response even though the prompt forbids markdown", async () => { stubModel("```json\n" + suggestion() + "\n```"); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); expect(state().status).toEqual("ready"); }); it("never calls the model for an out-of-scope error", async () => { const callModel = vi.spyOn(service as any, "callModel"); - await service.analyzeError(OP, err("ZeroDivisionError: division by zero"), SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, err("ZeroDivisionError: division by zero"), SCHEMA, CODE); expect(callModel).not.toHaveBeenCalled(); expect(state().status).toEqual("unsupported"); @@ -249,15 +289,15 @@ describe("AiWorkflowFixerService", () => { it("ends in error on transport failure, malformed JSON, or a missing field", async () => { vi.spyOn(service as any, "callModel").mockRejectedValue(new Error("502")); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); expect(state().status).toEqual("error"); stubModel("I cannot help with that."); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); expect(state().status).toEqual("error"); stubModel(JSON.stringify({ explanation: "e", fix_type: "code_change", confidence: "low" })); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); expect(state().status).toEqual("error"); }); }); @@ -272,8 +312,8 @@ describe("AiWorkflowFixerService", () => { .mockReturnValueOnce(slowFirst) .mockResolvedValueOnce({ text: suggestion({ explanation: "for the second operator" }) }); - const first = service.analyzeError("op-first", KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); - await service.analyzeError("op-second", KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + const first = service.analyzeError("op-first", KEY_ERROR_EVENT, SCHEMA, CODE); + await service.analyzeError("op-second", KEY_ERROR_EVENT, SCHEMA, CODE); expect(state().operatorId).toEqual("op-second"); resolveFirst({ text: suggestion({ explanation: "for the first operator" }) }); @@ -283,19 +323,24 @@ describe("AiWorkflowFixerService", () => { expect(state().suggestedFix?.explanation).toEqual("for the second operator"); }); - it("never sends a credential property to the model", async () => { - // Operators keep secrets beside ordinary settings. This asserts the wiring, not the - // filter: ai-fix-prompt.spec covers withoutSecrets itself, and the bug worth catching - // here is the service forgetting to call it. - operatorProperties = { code: CODE, hfApiToken: "hf_live_abc", password: "hunter2", modelId: "gpt-4-turb" }; + it("sends the operator's configuration nowhere, so a credential cannot ride along", async () => { + // Operators keep secrets beside ordinary settings, and filtering by field name was never + // enough: a Python UDF's uiParameters is a list of attribute/value pairs, so the name + // says nothing about the value. None of the three fixes reads configuration, so none is + // sent -- this asserts the wiring, and ai-fix-prompt.spec covers the prompt itself. + operatorProperties = { + code: CODE, + hfApiToken: "hf_live_abc", + uiParameters: [{ attribute: { attributeName: "hf_token" }, value: "hf_ui_xyz" }], + }; const callModel = vi.spyOn(service as any, "callModel").mockResolvedValue({ text: suggestion() }); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); const prompt = JSON.stringify(callModel.mock.calls[0][0]); expect(prompt).not.toContain("hf_live_abc"); - expect(prompt).not.toContain("hunter2"); - expect(prompt).toContain("gpt-4-turb"); + expect(prompt).not.toContain("hf_ui_xyz"); + expect(prompt).not.toContain("Operator configuration"); }); describe("classifying a real traceback", () => { @@ -345,14 +390,8 @@ describe("AiWorkflowFixerService", () => { const slowFirst = new Promise<{ text: string }>(resolve => (resolveFirst = resolve)); vi.spyOn(service as any, "callModel").mockReturnValueOnce(slowFirst); - const first = service.analyzeError("op-first", KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); - await service.analyzeError( - "op-second", - err("ZeroDivisionError: division by zero"), - SCHEMA, - CODE, - operatorProperties - ); + const first = service.analyzeError("op-first", KEY_ERROR_EVENT, SCHEMA, CODE); + await service.analyzeError("op-second", err("ZeroDivisionError: division by zero"), SCHEMA, CODE); expect(state().operatorId).toEqual("op-second"); expect(state().status).toEqual("unsupported"); @@ -376,7 +415,7 @@ describe("AiWorkflowFixerService", () => { }) ); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); expect(state().status).toEqual("error"); expect(state().suggestedFix).toBeUndefined(); @@ -387,7 +426,7 @@ describe("AiWorkflowFixerService", () => { it("drops the applied message once the re-run is over", async () => { stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); await service.applyFix(); expect(state().status).toEqual("applied"); @@ -399,7 +438,7 @@ describe("AiWorkflowFixerService", () => { it("drops a suggestion the user never applied", async () => { stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); expect(state().status).toEqual("ready"); ended("Failed"); @@ -415,7 +454,7 @@ describe("AiWorkflowFixerService", () => { const slow = new Promise<{ text: string }>(resolve => (resolveAnalysis = resolve)); vi.spyOn(service as any, "callModel").mockReturnValueOnce(slow); - const analysing = service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + const analysing = service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); ended("Killed"); expect(state().status).toEqual("idle"); @@ -431,7 +470,7 @@ describe("AiWorkflowFixerService", () => { const slow = new Promise<{ text: string }>(resolve => (resolveAnalysis = resolve)); vi.spyOn(service as any, "callModel").mockReturnValueOnce(slow); - const analysing = service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + const analysing = service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); service.discardFix(); resolveAnalysis({ text: suggestion() }); @@ -447,7 +486,7 @@ describe("AiWorkflowFixerService", () => { // the panel drops to idle and offers Analyze while the apply is still running. modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); const seen: string[] = []; const watching = service.getState$().subscribe(current => seen.push(current.status)); @@ -542,7 +581,7 @@ describe("AiWorkflowFixerService", () => { // it entirely and the tab came back saying "re-running" over a run stuck on an error. consoleMessages = [errorAt(100)]; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); await service.applyFix(); expect(state().status).toEqual("applied"); @@ -555,7 +594,7 @@ describe("AiWorkflowFixerService", () => { it("leaves the result alone while the error is still the one it was made for", async () => { consoleMessages = [errorAt(100)]; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); await service.applyFix(); consoleUpdates$.next(); @@ -567,7 +606,7 @@ describe("AiWorkflowFixerService", () => { describe("applyFix", () => { it("replaces only the snippet, keeps other properties, and re-runs", async () => { stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); await service.applyFix(); @@ -585,7 +624,7 @@ describe("AiWorkflowFixerService", () => { const twice = CODE + " yield t['email']\n"; operatorProperties = { code: twice, workers: 1 }; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, twice, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, twice); await service.applyFix(); @@ -607,7 +646,7 @@ describe("AiWorkflowFixerService", () => { // their run: the panel reported the refusal over a workflow it had just stopped. modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); operatorProperties.code = "class ProcessTupleOperator(UDFOperatorV2):\n pass\n"; @@ -624,7 +663,7 @@ describe("AiWorkflowFixerService", () => { // from the one on screen, and the diff for it is one model call away. const twice = CODE + " yield t['email']\n"; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, twice, { ...operatorProperties, code: twice }); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, twice); expect(state().suggestedFix?.occurrences).toEqual(2); operatorProperties.code = CODE; @@ -642,7 +681,7 @@ describe("AiWorkflowFixerService", () => { // warned about. Reachable because a kill lifts the modification lock without // resetting the panel: `Uninitialized` is deliberately not a terminal state. stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); expect(state().suggestedFix?.occurrences).toEqual(1); operatorProperties.code = CODE + " yield t['email']\n"; @@ -657,7 +696,7 @@ describe("AiWorkflowFixerService", () => { it("reports an apply failure, without re-running, when the snippet is no longer in the code", async () => { stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); // The user edits the UDF while reading the suggestion -- the snippet it quoted was in // the code when it was generated and is gone by the time Apply runs. A snippet that @@ -677,7 +716,7 @@ describe("AiWorkflowFixerService", () => { // says the run was refused. executeWorkflow.mockReturnValue(false); stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); await service.applyFix(); @@ -688,7 +727,7 @@ describe("AiWorkflowFixerService", () => { it("reports the re-run once the deployment accepts it", async () => { executeWorkflow.mockReturnValue(true); stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); await service.applyFix(); @@ -703,7 +742,7 @@ describe("AiWorkflowFixerService", () => { // avoids by checking first. modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); const applying = service.applyFix(); expect(killWorkflow).toHaveBeenCalled(); @@ -723,7 +762,7 @@ describe("AiWorkflowFixerService", () => { vi.useFakeTimers(); modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); const applying = service.applyFix(); await vi.advanceTimersByTimeAsync(20_000); @@ -740,7 +779,7 @@ describe("AiWorkflowFixerService", () => { // button has to work: refusing silently because the status is no longer `ready` // leaves the user clicking something that does nothing. stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); operatorProperties.code = "something else entirely"; await service.applyFix(); @@ -759,7 +798,7 @@ describe("AiWorkflowFixerService", () => { // Two imports of the same JSON carry the same operator ids, and this service outlives // the workspace, so the operator id alone would let the write land on the wrong graph. stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); openWorkflowId = 12; await service.applyFix(); @@ -774,7 +813,7 @@ describe("AiWorkflowFixerService", () => { // free to open another workflow while it does. modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); const applying = service.applyFix(); openWorkflowId = 12; @@ -791,7 +830,7 @@ describe("AiWorkflowFixerService", () => { const slow = new Promise<{ text: string }>(resolve => (resolveAnalysis = resolve)); vi.spyOn(service as any, "callModel").mockReturnValueOnce(slow); - const analysing = service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + const analysing = service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); openWorkflowId = 12; resolveAnalysis({ text: suggestion() }); await analysing; @@ -806,10 +845,10 @@ describe("AiWorkflowFixerService", () => { // now looking at. modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); const applying = service.applyFix(); - await service.analyzeError("other-op", KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError("other-op", KEY_ERROR_EVENT, SCHEMA, CODE); expect(state().operatorId).toEqual("other-op"); modificationEnabled = true; @@ -828,7 +867,7 @@ describe("AiWorkflowFixerService", () => { workflowReadonly = true; modificationEnabled = false; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); await service.applyFix(); @@ -844,7 +883,7 @@ describe("AiWorkflowFixerService", () => { modificationEnabled = false; executionState = ExecutionState.Uninitialized; stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); await service.applyFix(); @@ -858,7 +897,7 @@ describe("AiWorkflowFixerService", () => { throw new Error("read-only workflow"); }); stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); await service.applyFix(); @@ -869,7 +908,7 @@ describe("AiWorkflowFixerService", () => { it("discards to idle and keeps state across re-subscription", async () => { stubModel(suggestion()); - await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE, operatorProperties); + await service.analyzeError(OP, KEY_ERROR_EVENT, SCHEMA, CODE); // A frame rebuilt by clearResultPanel() re-subscribes and must still see the fix. expect((await firstValueFrom(service.getState$())).status).toEqual("ready"); diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts index a7e96b22b3a..6bbbd2b8e94 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts @@ -28,8 +28,7 @@ import { WorkflowActionService } from "../../../service/workflow-graph/model/wor import { ExecuteWorkflowService } from "../../../service/execute-workflow/execute-workflow.service"; import { OperatorPortSchemaMap } from "../../../types/workflow-compiling.interface"; import { ExecutionState } from "../../../types/execute-workflow.interface"; -import { buildFixPrompt, withoutSecrets } from "./ai-fix-prompt"; -import { OperatorMetadataService } from "../../../service/operator-metadata/operator-metadata.service"; +import { buildFixPrompt } from "./ai-fix-prompt"; import { WorkflowConsoleService } from "../../../service/workflow-console/workflow-console.service"; export type FixErrorType = "missing_column" | "type_error" | "null_error" | "unsupported"; @@ -206,7 +205,6 @@ export class AiWorkflowFixerService { constructor( private workflowActionService: WorkflowActionService, private executeWorkflowService: ExecuteWorkflowService, - private operatorMetadataService: OperatorMetadataService, private workflowConsoleService: WorkflowConsoleService ) { // The panel holds whatever it last showed until something clears it, and after Apply @@ -251,8 +249,7 @@ export class AiWorkflowFixerService { operatorId: string, error: OperatorError, inputSchemas: OperatorPortSchemaMap | undefined, - code: string | undefined, - properties: Readonly> + code: string | undefined ): Promise { const errorType = classifyError(error.header); const workflowId = this.workflowActionService.getOpenWorkflowId(); @@ -275,11 +272,7 @@ export class AiWorkflowFixerService { this.stateSubject.next(base); try { - // One redacted view, used for both the prompt and the suggestion: toFix reads the - // field's current value to show in the diff, and reading it from the raw properties - // put a credential on screen whenever the model guessed one of those names. - const visible = withoutSecrets(properties, this.schemaProperties(operatorId)); - const { text } = await this.callModelWithTimeout(buildFixPrompt(error.full, code, inputSchemas, visible)); + const { text } = await this.callModelWithTimeout(buildFixPrompt(error.full, code, inputSchemas)); if (seq !== this.analysisSeq || workflowId !== this.workflowActionService.getOpenWorkflowId()) { return; } @@ -509,23 +502,6 @@ export class AiWorkflowFixerService { } } - /** - * The operator's own JSON schema, used to spot the fields it renders as password widgets. - * Returns nothing when the operator or its schema cannot be resolved -- `withoutSecrets` - * still drops credential-looking names, so a miss here narrows the filter, never removes it. - */ - private schemaProperties(operatorId: string): Record | undefined { - try { - const operatorType = this.workflowActionService.getTexeraGraph().getOperator(operatorId).operatorType; - return this.operatorMetadataService.getOperatorSchema(operatorType).jsonSchema.properties as Record< - string, - unknown - >; - } catch { - return undefined; - } - } - /** Maps the model's JSON contract onto SuggestedFix, rejecting incomplete replies. */ private toFix(raw: string): SuggestedFix { let text = raw.trim(); @@ -539,7 +515,7 @@ export class AiWorkflowFixerService { } catch (err) { throw new Error(`model did not return JSON: ${(err as Error).message}`); } - const { explanation, fix_type, original_snippet, suggested_snippet, confidence } = parsed ?? {}; + const { explanation, original_snippet, suggested_snippet, confidence } = parsed ?? {}; if (!explanation || !original_snippet || !suggested_snippet) { throw new Error("model response is missing a required field"); } @@ -555,15 +531,15 @@ export class AiWorkflowFixerService { throw new Error(`model returned ${typeof value} for ${name}, expected a string`); } } - // This panel edits code and nothing else, so any other kind of fix is the model going - // off script -- and refusing here keeps it off the screen rather than only out of the - // write. - if (fix_type !== "code_change") { - throw new Error(`${fix_type} is not a kind of fix this panel applies`); - } + // One trailing newline off each, independently. The prompt asks for whole lines and does + // not settle whether the final newline is part of the quote, so both forms arrive. Kept, + // it makes the verdict depend on where the line sits: `code[end]` is the next line's + // first character everywhere except the last line of the file, which passes. Dropped + // from the replacement too, so the line's own newline stays put and nothing is merged. + const withoutTrailingNewline = (snippet: string) => (snippet.endsWith("\n") ? snippet.slice(0, -1) : snippet); return { - original: original_snippet, - suggested: suggested_snippet, + original: withoutTrailingNewline(original_snippet), + suggested: withoutTrailingNewline(suggested_snippet), explanation, confidence: confidence === "high" || confidence === "low" ? confidence : "medium", }; From 7d3ccda52ba1c2a44369f1bef7e8c7826abaae3b Mon Sep 17 00:00:00 2001 From: Pedro Date: Tue, 27 Oct 2026 08:03:30 -0600 Subject: [PATCH 39/39] feat(config): read the AI fix model from configuration @Ma77Ball, from the first round: AI_FIXER_MODEL was a compile-time constant holding `claude-haiku-4.5`, which is the alias this deployment's LiteLLM proxy publishes, not a provider id. A proxy serving a different alias could not be reached without rebuilding the bundle. It now travels the same route as every other workspace setting: `gui.workflow-workspace.ai-fix-model` in gui.conf, overridable by GUI_WORKFLOW_WORKSPACE_AI_FIX_MODEL, read by GuiConfig, served by ConfigResource beside copilotEnabled, and consumed from GuiConfigService. Deliberately not in llm.conf, which the frontend never sees: the panel is what names the model, and the proxy's base-url and master key stay backend-only. No unit test for the wiring. The one I wrote asserted the value I had just set and that callModel was called, which is true whether or not the id reaches `.chat()` -- callModel is the seam every other test stubs, so from a unit test there is nothing to see. Deleted rather than kept green, and verified against the running config endpoint instead. Co-Authored-By: Claude Opus 5 --- common/config/src/main/resources/gui.conf | 5 +++++ .../org/apache/texera/common/config/GuiConfig.scala | 2 ++ .../texera/service/resource/ConfigResource.scala | 1 + .../src/app/common/service/gui-config.service.mock.ts | 2 ++ frontend/src/app/common/type/gui-config.ts | 1 + .../result-panel/ai-fix/ai-workflow-fixer.service.ts | 11 ++++++----- 6 files changed, 17 insertions(+), 5 deletions(-) diff --git a/common/config/src/main/resources/gui.conf b/common/config/src/main/resources/gui.conf index 7cb7fff6555..d770b4ffd20 100644 --- a/common/config/src/main/resources/gui.conf +++ b/common/config/src/main/resources/gui.conf @@ -120,6 +120,11 @@ gui { copilot-enabled = false copilot-enabled = ${?GUI_WORKFLOW_WORKSPACE_COPILOT_ENABLED} + # the model the AI fix panel asks for, named as the LiteLLM proxy exposes it + # (see llm.conf and the proxy's own model list), not as the provider names it + ai-fix-model = "claude-haiku-4.5" + ai-fix-model = ${?GUI_WORKFLOW_WORKSPACE_AI_FIX_MODEL} + # the limit of columns to be displayed in the result table limit-columns = 15 limit-columns = ${?GUI_WORKFLOW_WORKSPACE_LIMIT_COLUMNS} diff --git a/common/config/src/main/scala/org/apache/texera/common/config/GuiConfig.scala b/common/config/src/main/scala/org/apache/texera/common/config/GuiConfig.scala index d5295024dc8..6283a8c225d 100644 --- a/common/config/src/main/scala/org/apache/texera/common/config/GuiConfig.scala +++ b/common/config/src/main/scala/org/apache/texera/common/config/GuiConfig.scala @@ -73,6 +73,8 @@ object GuiConfig { conf.getInt("gui.workflow-workspace.active-time-in-minutes") val guiWorkflowWorkspaceCopilotEnabled: Boolean = conf.getBoolean("gui.workflow-workspace.copilot-enabled") + val guiWorkflowWorkspaceAiFixModel: String = + conf.getString("gui.workflow-workspace.ai-fix-model") val guiWorkflowWorkspaceLimitColumns: Int = conf.getInt("gui.workflow-workspace.limit-columns") val guiAttributionEnabled: Boolean = diff --git a/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala b/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala index d682fe8d026..e3dff08575b 100644 --- a/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala +++ b/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala @@ -101,6 +101,7 @@ class ConfigResource { "pythonLanguageServerPort" -> GuiConfig.guiWorkflowWorkspacePythonLanguageServerPort, "activeTimeInMinutes" -> GuiConfig.guiWorkflowWorkspaceActiveTimeInMinutes, "copilotEnabled" -> GuiConfig.guiWorkflowWorkspaceCopilotEnabled, + "aiFixModel" -> GuiConfig.guiWorkflowWorkspaceAiFixModel, "limitColumns" -> GuiConfig.guiWorkflowWorkspaceLimitColumns, "pythonNotebookMigrationEnabled" -> GuiConfig.guiWorkflowWorkspacePythonNotebookMigrationEnabled, "pythonNotebookMigrationTimeoutMinutes" -> GuiConfig.guiWorkflowWorkspacePythonNotebookMigrationTimeoutMinutes diff --git a/frontend/src/app/common/service/gui-config.service.mock.ts b/frontend/src/app/common/service/gui-config.service.mock.ts index 8591fc727a9..e108856beda 100644 --- a/frontend/src/app/common/service/gui-config.service.mock.ts +++ b/frontend/src/app/common/service/gui-config.service.mock.ts @@ -54,6 +54,8 @@ export class MockGuiConfigService { defaultLocalUser: { username: "", password: "" }, activeTimeInMinutes: 15, copilotEnabled: false, + + aiFixModel: "claude-haiku-4.5", limitColumns: 15, attributionEnabled: false, pythonNotebookMigrationEnabled: false, diff --git a/frontend/src/app/common/type/gui-config.ts b/frontend/src/app/common/type/gui-config.ts index 5b8b23db03d..c84c104416a 100644 --- a/frontend/src/app/common/type/gui-config.ts +++ b/frontend/src/app/common/type/gui-config.ts @@ -45,6 +45,7 @@ export interface GuiConfig { defaultLocalUser?: { username?: string; password?: string }; activeTimeInMinutes: number; copilotEnabled: boolean; + aiFixModel: string; limitColumns: number; attributionEnabled: boolean; pythonNotebookMigrationEnabled: boolean; diff --git a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts index 6bbbd2b8e94..306ba4df35a 100644 --- a/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts +++ b/frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts @@ -30,6 +30,7 @@ import { OperatorPortSchemaMap } from "../../../types/workflow-compiling.interfa import { ExecutionState } from "../../../types/execute-workflow.interface"; import { buildFixPrompt } from "./ai-fix-prompt"; import { WorkflowConsoleService } from "../../../service/workflow-console/workflow-console.service"; +import { GuiConfigService } from "../../../../common/service/gui-config.service"; export type FixErrorType = "missing_column" | "type_error" | "null_error" | "unsupported"; /** @@ -93,10 +94,6 @@ export interface FixState { applyError?: string; } -// Model id as LiteLLM exposes it (bin/single-node/litellm-config.yaml), not the provider's -// id. A deployment whose proxy publishes a different alias has to change this: there is no -// GuiConfig field for it today, and adding one is a backend change. -export const AI_FIXER_MODEL = "claude-haiku-4.5"; export const AI_FIXER_TIMEOUT_MS = 120_000; export const UNLOCK_TIMEOUT_MS = 15_000; @@ -203,6 +200,7 @@ export class AiWorkflowFixerService { private readonly stateSubject = new BehaviorSubject(IDLE_STATE); constructor( + private config: GuiConfigService, private workflowActionService: WorkflowActionService, private executeWorkflowService: ExecuteWorkflowService, private workflowConsoleService: WorkflowConsoleService @@ -420,7 +418,10 @@ export class AiWorkflowFixerService { const model = createOpenAI({ baseURL: new URL(`${AppSettings.getApiEndpoint()}`, document.baseURI).toString(), apiKey: AuthService.getAccessToken() ?? "", - }).chat(AI_FIXER_MODEL); + // Named as the LiteLLM proxy exposes it, not as the provider does, and read from the + // deployment's own config: a proxy publishing a different alias would otherwise need a + // rebuild to be reachable. + }).chat(this.config.env.aiFixModel); return generateText({ model, messages, abortSignal }); }