aminghadersohi commented on PR #42659:
URL: https://github.com/apache/superset/pull/42659#issuecomment-5364964015
Correction and update to my previous comment — that comment was accurate
about the SHA, but incomplete, and I should flag what I missed.
**What I got right:** the fix was on the branch; the rebase had rewritten
`8a01ea6fdc` into a new SHA.
**What I missed:** the PR was in a `CONFLICTING` / `DIRTY` merge state
against master. `#43202` ("preserve user-authored result values") landed on the
same MCP dashboard files after this branch was last synced, and it did more
than collide textually:
1. It removed `sanitize_for_llm_context` from the MCP service entirely. This
branch still called it in `permalink.py`, so on current master the module fails
to import outright — `ImportError: cannot import name
'sanitize_for_llm_context'`. A naive merge would have shipped a broken import.
2. It rewrote `_apply_permalink_state` from `model_dump`/`model_validate` to
`model_copy(update=...)`, specifically so results are not re-validated.
Resolving that conflict in this branch's favour would have silently reverted
that fix.
I have rebased onto `22396d504a` and reconciled both rather than taking
either side wholesale:
- Dropped the `sanitize_for_llm_context` call. Sanitization is no longer the
tool layer's job per `#43202`. The privacy redaction
(`user_can_view_data_model_metadata` /
`redact_filter_state_data_model_metadata`) is kept — that is access control,
not value rewriting, and is unaffected.
- Kept master's `model_copy` form of `_apply_permalink_state`.
- Removed two imports in `test_dashboard_tools.py` that `#43202` made dead
(`LLM_CONTEXT_*` delimiters, now that `_wrapped` is identity).
- Master had five `refresh_request_user_for_permalink_access` tests in
`test_dashboard_tools.py`; this branch moved that helper to `permalink.py`.
Four are covered by the parametrized tests in `test_permalink.py`, but
`keeps_user_when_reload_fails` (reload returns `None`, user preserved —
`permalink.py:85-86`) was not. I ported it so no coverage is lost in the move.
New head is `daf1838f93`, five linear commits on master, fixups squashed
into their originating commits so the history stays bisectable. GitHub now
reports `MERGEABLE`.
Verification: 399 dashboard MCP tests pass; full
`tests/unit_tests/mcp_service/` is 3528 passed / 1 failed, and that one failure
(`test_mcp_e2e_smoke.py::test_tools_call_health_check_over_real_asgi_transport`)
reproduces identically on a clean `origin/master` worktree, so it is
pre-existing and not from this branch. `pre-commit` passes on all nine changed
files (mypy, ruff, pylint included).
Worth re-running your harness against `daf1838f93` rather than the old SHA —
the source changed materially in the reconciliation, so the previous build is
not representative.
--
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]
