I3eka commented on PR #43136: URL: https://github.com/apache/superset/pull/43136#issuecomment-6433551472
Reviewed all 12 inline threads and all findings in Copilot review [5194449807](https://github.com/apache/superset/pull/43136#pullrequestreview-5194449807), including the eight suppressed comments. Replies are attached to each inline thread. The search fix and its whitespace/hyphen regressions are preserved. Apache master `1220ec01ef856de09fc9397f429f9fe1feb3e2ef` is merged without rewriting published history. | Summary/suppressed finding | Verification and change | | --- | --- | | Helper directives dropped | Fixed in `518560dbe8`: bounded user-role instructions reach both provider paths, before the current question. | | Idempotency overwrites accepted context | Fixed in `8f26cec7dc`: stable run metadata, atomic claim, enqueue/retry coverage. | | Cancel ID not associated with the owned thread | Fixed in `ff539ee45b`: missing/cross-thread IDs return 404 before any flag write. | | Unbounded raw page context | Fixed in `98b5124363`: server-side compact JSON size and nesting bounds before persistence on message/suggestion paths. | | Wrong feedback exception | Fixed in `986f75da00`: invalid target returns 422; missing/foreign target remains 404. | | Terminal reconnect starts inference again | Fixed in `8f26cec7dc`: complete/error/cancelled outcomes replay from storage. | | Unbounded pagination and per-thread message loading | Fixed in `bbd255c301`: validated limit/offset, one aggregate count query for the owned page, no transcript loading. HTTP/SQLite query-count regressions cover it. | | Concrete model ignored | Fixed in `fb5625d2f2`: selected model reaches runtime and provider requests. | | Worker cancellation uses a local cache | Fixed in `31677921e6`: event-bus Redis backend and TTL. A real-encoder regression additionally caught and fixed raw Redis rejecting boolean `True`; the flag is now bytes. | | Untrusted page text crosses the prompt boundary | Fixed in `5b5b0f621c`: existing framing helper and delimiter escaping, complete frames under truncation. | | Retry/circuit middleware unused | Fixed in `bb2a92ab50`: complete and stream paths honor provider retry policy; streaming retries stop after the first provider event. Regressions cover recovery, permanent failures, exhaustion, breaker limits and no replay after partial output. | | Suggestions leak provider resources | Fixed in `0476f4aab4`: close provider before closing its loop on success and failure, including cleanup failure. | | Chart-level authorization missing | Not reproduced: `_load_chart` uses the scoped DAO and `raise_for_access(chart=chart)`. The existing denied-object and missing-chart regressions pass. This does not replace the separate review of capability grants on other tools. | | Duplicate SQL labels break normalization | Fixed in `e982eb1cea`: reuse SQL Lab's collision-safe `dedup`, preserve every value and leave the input frame unchanged. | | Object-wrapped suggestions fail | Not reproduced: the parser extracts the array before decoding; the cited object-wrapper regression passes. | Also addressed the human retention and empty-chat findings, removed the unwired knowledge-provider contract using the alternative offered in review (`774482609c`), verified actual Redis/Sentinel construction, and checked the single migration head. The exact history cutoff in `c4eb9ff729` prevents a queued question from seeing later messages. Validation: **809 backend tests; 108 frontend tests in 9 suites; PR-file pre-commit checks**. These are local checks, with no live provider/warehouse/Redis/broker, deployed migration, browser or deployment test. A disappeared whole worker/host is not automatically reconciled. @rusackas @sadpandajoe: this remains **Draft / ON HOLD**, dependent on #42805 and SIP #42806. The review fixes do not request merging ahead of that process. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
