pedroreyesalvarado commented on PR #8637: URL: https://github.com/apache/texera/pull/8637#issuecomment-6370016685
@kunwp1 — all four, one commit each: `68c65b1`, `03cd234`, `114b582`, `e35d097`. **The JVM one was a regression of my own fix, not a gap in it.** You were right about the module prefix, and I checked `FRAME_LINE` against six real frame shapes before touching anything: `java.base/java.lang.Thread.run(Thread.java:840)`, the ThreadPoolExecutor frame, `(Native Method)` and `<init>` all fall through — only the two shapes I had imagined match. And `mkConsoleMessage` sends `getStackTrace.mkString` with no `at ` prefix, so the one pattern I added for the JVM never applied to console messages at all. A real worker trace ends on exactly one of those frames, so the backward scan returned the frame and `NullPointerException` classified as unsupported. What I shipped in `7ee6339` did not work in production. So I took your suggestion rather than patching the regex. I checked all three sources first: `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 retires the chained-traceback handling too, instead of reimplementing it: `format_exception[-1]` is already the exception that stopped the worker, which is what scanning backwards was trying to recover. **Validating after the kill was the worst of the four in practice**, and the test makes the cost visible: it took 15 seconds to fail. The run was already stopped, and then the user waited out the unlock timeout to be told nothing had been written. The checks and the patch moved into `patchedProperties()`, which reads only the graph, so it runs as a pre-flight before the kill and again after it — the operator can still move during the wait. The `model_not_found` check moved to the analysis, where the rest of them already were. **On the indentation, you were right that the prompt never stated a convention, so I stated one** and measured it rather than guessing. The prompt now requires whole lines copied from the start of a line with their indentation; `startsAtLineBoundary` refuses a reply that quotes 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`. I ran it against `claude-haiku-4.5` through the local LiteLLM proxy on the three code-change shapes, with the previous prompt as a control: | | accepted | |---|---| | new prompt | 28 / 28 | | old prompt (control) | 11 / 12 | The single refusal under the old prompt is exactly a mid-line snippet with no indentation — the case this is about. So the model was usually including the indentation on its own, which is why this looked fine end to end and still broke the way you found. Applying the model's real multi-line guard now yields 0/4/8/12 nesting, valid Python, nothing doubled. The test fixture had to change with it, which says the same thing from the other side: its `original_snippet` was quoted mid-line, so the new contract turns the old fixture away. **`trackBy`** — keying by the config object, as you suggested. It also settles what I admitted I could not test: "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, so the claim is no longer resting on my reading of `NgComponentOutlet`. One more that nobody reported. Reviewing the branch myself I found that Apply checked only whether the snippet was still *present*, never whether it was still in the same number of places — so a copy added between the analysis and the click was patched too, with the "changes N places" badge hidden at one occurrence. It refuses in both directions now (`711b328`). Suite: 6751 passing, `eslint ./src`, `prettier --check` and `tsc --noEmit` clean. **One ask.** The approved re-run on `711b328` passed everything (Required Checks was cancelled by my next push, not failed), but every push since is back to `action_required`, so none of these four fixes has actually been through CI. Could you approve a run when you next look? Four changes went in off the back of unit tests alone, and the classification one touches a path I got wrong once already — I would rather CI contradict me now than after a merge. The same goes for Copilot, whose review ran out of runners last time. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
