aminghadersohi commented on PR #44146:
URL: https://github.com/apache/superset/pull/44146#issuecomment-5756198180
Review-body follow-up in 5fa47d9bcb5975f9d3455e476b892ff4dab5c88b (no
routing behavior changes):
- Documented `mcp_auth_hook(tool_name=...)`, its
registered/extension-prefixed identity, and the `None` resource/prompt
routing-skip contract; explained the deferred scope import.
- Documented the distinction between routing disabled (`None`) and enabled
with no user's role grants (empty scope).
- Added the operator note from the APPROVED review: none of the default
`MCP_GUEST_ALLOWED_TOOLS` supports dataset-scoped mode, so those calls refuse
when it is enabled.
- Added the explicit asyncio marker, hoisted the scope test's imports,
annotated its locals, and corrected two docstrings that repeated the incorrect
claim that stringifying True produces a valid UUID.
Other body suggestions were evaluated rather than treated as blockers:
- The two sync/async gate calls already delegate all enforcement to
`enforce_call_dataset_scope`; both run after RBAC and inside the
exception-cleanup block. Factoring the identical two-line calls is optional, so
I avoided an unrelated runtime refactor.
- `list_datasets` locals retain their inferred types from the annotated
scope helper, request model, UUID constructor and bound-filter builder.
Branch-file mypy and pylint both pass; no new Any workaround was added just to
annotate inferred locals.
- The scope module's call-local model/lookup imports remain deferred to
their data-access paths rather than being made startup dependencies merely for
style. `security_manager` at module scope is the application's proxy, not
equivalent to importing the dataset model.
- The claimed F541 failure does not reproduce: `ruff check
superset/mcp_service/dataset_scope.py --select F541` and the full pre-commit
Ruff hook both pass. The adjacent f-string literals form one expression
containing `{_NO_SUBSTITUTE}`.
Validation before push: `PYTHONPATH="$PWD:$PWD/superset-core/src" pytest -q
tests/unit_tests/mcp_service/test_dataset_scope.py
tests/unit_tests/mcp_service/dataset
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py
tests/unit_tests/mcp_service/test_auth*.py` => **488 passed, 1 skipped**.
Strict asyncio-mode startup/query-scope subset => **17 passed, 153
deselected**. `pre-commit run` passed on explicit staged paths; `pre-commit run
--files <all 14 PR files>` also passed (including mypy, Ruff, pylint). Initial
collection had a problem finding `superset_core`; using this checkout's
`superset-core/src` in PYTHONPATH fixed it without dependency changes. No dev
server was started.
All four specified threads have individual closing-commit replies and are
resolved. Re-requested sadpandajoe via the API and confirmed the request is
live. Did not re-request gabotorresruiz, dismiss any review, merge, or change
draft state.
Approval applicability: I inspected `a0f75225c...2ad5158f3` and the final
head, and range-diffed the PR commits. All eight approved PR patches are
identical after rebase; the additions are the reviewer's UUID-doc correction
plus this docs/comments/test-only follow-up. Thus the approval remains
applicable to the PR-specific work. **It is not sufficient for the entire
updated head under the requested strict criterion**, because the rebase also
brings upstream behavior changes, other files, and dependency changes (e.g.
PyJWT 2.13.0→2.14.0 and psycopg2-binary 2.9.12→2.9.13). The recorded approval
remains untouched.
--
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]
