SEZ9 commented on PR #13001: URL: https://github.com/apache/seatunnel/pull/13001#issuecomment-6466874991
Thanks @DanielLeens. Taking the three in turn, one is answered by the stack and two I have left as follow-ups with reasoning rather than changed here. **Issue 2 — question not recorded in history.** I checked the caller rather than assuming. `cli.py`'s interactive loop *did* handle this, at the point where it merged `_pending_request` with the next message before calling in. Looking at it alongside #13000 showed that merge is now a duplicate: the Orchestrator does the same thing, so the requirement would have been prepended twice in interactive sessions. I removed the `cli.py` merge in the #13000 follow-up commit and added a test asserting the requirement appears exactly once. So the question text is still not appended to `conversation_history`, and the planner does see two consecutive user turns — but it no longer lacks the context you were worried about, because the reply now arrives merged with the request it answers. The existing missing-slot question has always behaved this way, so this is consistent rather than new. Appending the assistant turn as well would be tidier and I am happy to do it, but it is a change to the history contract that the session writer and `_trimmed_history` both read, so I would rather not fold it into this PR. **Issue 1 — unbounded clarification loop.** Agreed, and I have deliberately not added a counter here. The loop is bounded in the two cases I can reason about: the interactive user can stop, and a programmatic caller like the benchmark replies once and then gives up. What I cannot bound is a planner that keeps emitting text without both `source:` and `sink:` while an automated caller keeps answering. A round counter is the right fix for that, but it needs a decision about what to do when it trips — error, or generate with a documented warning — and that is a behaviour choice worth its own issue rather than a number I pick here. I will file it. **Issue 3 — duplicated test file.** Correct, it is the stacking. #13000 now has a follow-up commit, so this branch has been rebuilt on top of it; the file resolves away when #13000 merges. CI and the suite from `seatunnel-cli/` are green on the new head: 354 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]
