SEZ9 commented on PR #13000: URL: https://github.com/apache/seatunnel/pull/13000#issuecomment-6466881043
Thanks @DanielLeens — all three taken, and chasing issue 3 turned up a regression I had introduced, so thank you for that one in particular. **The regression first.** `cli.py`'s interactive loop already merged a clarification reply with the request it answered, before calling in. With the Orchestrator now doing the same, the requirement would have been prepended **twice** in interactive sessions. I removed the duplicate merge from `cli.py` — the Orchestrator owns `pending_question`, so it is the right place, and the programmatic callers never went through `cli.py` at all — and added a test asserting the requirement appears exactly once. This also means the PR description overstated the impact, which I should correct: interactive sessions were **not** losing the requirement, because of that workaround. The defect was in the Orchestrator API, which is what the benchmark and any embedder use. The benchmark trace in the description is real; the implication that a user at the REPL saw it is not. **Issue 1 — `load_history`.** Fixed. It now clears both fields, so resuming a session cannot merge the first message into whatever requirement happened to be open. **Issue 3 — test coverage.** You were right that the name promised more than the body checked, and that removing a reset would not have gone red. Split into a focused question-path test plus a `PendingStateIsClearedTest` covering every exit that serves or abandons the requirement: delivered config, best-effort config after three repair rounds, chat reply, session resume. I verified each one by removing the corresponding reset and confirming exactly one test fails: ``` removed reset: load_history -> FAILED test_resuming_a_session_clears_it removed reset: config_delivered -> FAILED test_a_delivered_config_clears_it removed reset: chat -> FAILED test_a_chat_reply_clears_it ``` **Issue 2 — state left set if `_run_planner` raises.** Left as it is, as you allowed, and I want to give the reason rather than just skip it. The documented stance is that a message arriving while a question is outstanding is treated as an answer. An exception in the planner does not change what the user most likely meant by their next message, and resetting there would silently discard a requirement they can still see on their screen. If a reviewer prefers the reset I will add it, but I did not want to trade a visible merge for an invisible loss. Suite from `seatunnel-cli/`: 346 passed, 3 subtests passed. -- 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]
