codeant-ai-for-open-source[bot] commented on code in PR #41606:
URL: https://github.com/apache/superset/pull/41606#discussion_r3598845622
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1083,6 +1083,385 @@ class UpdateDashboardResponse(BaseModel):
)
+class ManageDashboardOwnersRequest(BaseModel):
+ """Request schema for explicit add/remove dashboard owner management.
+
+ Unlike ``update_dashboard``'s dropped ``owners`` field (a full-replacement
+ list with no safety guard, so an empty or partial list could silently
+ orphan a dashboard), this tool takes explicit add/remove operations and
+ rejects any change that would leave the dashboard with zero owners.
+
+ "Owners" here means USER-type entries in the dashboard's Subject-based
+ ``editors`` list (the ownership model apache/superset#38831 introduced,
+ replacing the legacy ``owners`` relationship). Any ROLE- or GROUP-type
+ editors already on the dashboard are left untouched by this tool.
+ """
+
+ identifier: int | str = Field(
+ ...,
+ description=(
+ "Dashboard ID (integer), UUID, or slug. Same identifier shape "
+ "accepted by ``get_dashboard_info``."
+ ),
+ )
+ add_owner_ids: list[int] = Field(
+ default_factory=list,
+ description=(
+ "User IDs to add as dashboard owners. Discover IDs with
``find_users``."
+ ),
+ )
+ remove_owner_ids: list[int] = Field(
+ default_factory=list,
+ description=(
+ "User IDs to remove from dashboard owners. Rejected if it would "
+ "leave the dashboard with zero owners, or if an ID is not "
+ "currently an owner."
+ ),
+ )
+
+ @field_validator("identifier", mode="before")
+ @classmethod
+ def reject_bool_identifier(cls, value: object) -> object:
+ """bool is a subclass of int, so identifier=true would coerce to
+ dashboard ID 1 and mutate the wrong dashboard; reject it outright."""
+ if isinstance(value, bool):
+ raise ValueError("identifier must be an integer ID, UUID, or slug
string")
+ return value
+
+ @field_validator("add_owner_ids", "remove_owner_ids", mode="before")
+ @classmethod
+ def reject_bool_owner_ids(cls, value: object) -> object:
+ """bool is a subclass of int, so a `true`/`false` list element would
+ coerce to owner ID 1/0 and add/remove the wrong owner; reject it."""
+ if isinstance(value, list) and any(isinstance(item, bool) for item in
value):
+ raise ValueError("owner ID list items must be integers, not
booleans")
+ return value
+
+ @model_validator(mode="after")
+ def _validate_operations(self) -> "ManageDashboardOwnersRequest":
+ if not self.add_owner_ids and not self.remove_owner_ids:
+ raise ValueError(
+ "At least one of add_owner_ids or remove_owner_ids is
required."
+ )
+ overlap: list[int] = sorted(
+ set(self.add_owner_ids) & set(self.remove_owner_ids)
+ )
+ if overlap:
+ raise ValueError(
+ "User IDs cannot appear in both add_owner_ids and "
+ f"remove_owner_ids: {overlap}."
+ )
+ return self
+
+
+class DashboardMutationErrorFields(BaseModel):
+ """Shared ``error``/``permission_denied`` fields for dashboard governance
+ mutation responses (owners/roles/certification), including the
+ validator that wraps ``error`` before it is exposed to LLM context.
+ """
+
+ error: str | None = Field(None, description="Error message, if operation
failed")
+ permission_denied: bool = Field(
+ default=False,
+ description=("True when the user lacks edit rights on the target
dashboard."),
+ )
+
+ @field_validator("error")
+ @classmethod
+ def sanitize_error_for_llm_context(cls, value: str | None) -> str | None:
+ """Wrap error text before it is exposed to LLM context."""
+ if value is None:
+ return value
+ return sanitize_for_llm_context(value, field_path=("error",))
+
+
+class ManageDashboardOwnersResponse(DashboardMutationErrorFields):
+ """Response schema for ``manage_dashboard_owners``."""
+
+ owners: list[SubjectInfo] = Field(
+ default_factory=list,
+ description=(
+ "Full list of USER-type editor subjects (dashboard owners) "
+ "after the operation. Any ROLE/GROUP-type editors on the "
+ "dashboard are not included here."
+ ),
+ )
+ dashboard_url: str | None = Field(None, description="URL to view the
dashboard")
+ added_owner_ids: list[int] = Field(
+ default_factory=list,
+ description="User IDs actually added as owners by this call.",
+ )
+ removed_owner_ids: list[int] = Field(
+ default_factory=list,
+ description="User IDs actually removed from owners by this call.",
+ )
+ warnings: list[str] = Field(
+ default_factory=list,
+ description=(
+ "Non-fatal advisory messages, e.g. that a non-admin caller was "
+ "automatically re-added as owner after trying to remove "
+ "themselves."
+ ),
+ )
+
+ @field_validator("owners", mode="after")
+ @classmethod
+ def sanitize_owners_for_llm_context(
+ cls, value: list[SubjectInfo]
+ ) -> list[SubjectInfo]:
+ """Wrap owner labels before LLM exposure; owner display names are
+ user-controlled and render as plain text in this response, so an
+ unsanitized label could inject content into LLM context (CWE-79
+ analog for LLM-facing output). Entries that sanitize to an empty
+ label are dropped rather than surfaced with a blank identity."""
+ sanitized: list[SubjectInfo] = []
+ for subject in value:
+ if subject.label is None:
+ sanitized.append(subject)
+ continue
+ clean_label = sanitize_for_llm_context(
+ subject.label, field_path=("owners", "label")
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation for this local variable to
satisfy the rule requiring type hints on relevant variables. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is new Python code in the PR, and the local variable `clean_label` is
inferred rather than explicitly typed. The repository rule requires type hints
for relevant variables that can be annotated, so this is a real rule violation.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=32497155bc37410589d7d9f70257902f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=32497155bc37410589d7d9f70257902f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/schemas.py
**Line:** 1222:1224
**Comment:**
*Custom Rule: Add an explicit type annotation for this local variable
to satisfy the rule requiring type hints on relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=cf2c3428800fb8fb9d8400c367c074682394fddb1388f8aee256d21ba7662b30&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=cf2c3428800fb8fb9d8400c367c074682394fddb1388f8aee256d21ba7662b30&reaction=dislike'>๐</a>
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1083,6 +1083,385 @@ class UpdateDashboardResponse(BaseModel):
)
+class ManageDashboardOwnersRequest(BaseModel):
+ """Request schema for explicit add/remove dashboard owner management.
+
+ Unlike ``update_dashboard``'s dropped ``owners`` field (a full-replacement
+ list with no safety guard, so an empty or partial list could silently
+ orphan a dashboard), this tool takes explicit add/remove operations and
+ rejects any change that would leave the dashboard with zero owners.
+
+ "Owners" here means USER-type entries in the dashboard's Subject-based
+ ``editors`` list (the ownership model apache/superset#38831 introduced,
+ replacing the legacy ``owners`` relationship). Any ROLE- or GROUP-type
+ editors already on the dashboard are left untouched by this tool.
+ """
+
+ identifier: int | str = Field(
+ ...,
+ description=(
+ "Dashboard ID (integer), UUID, or slug. Same identifier shape "
+ "accepted by ``get_dashboard_info``."
+ ),
+ )
+ add_owner_ids: list[int] = Field(
+ default_factory=list,
+ description=(
+ "User IDs to add as dashboard owners. Discover IDs with
``find_users``."
+ ),
+ )
+ remove_owner_ids: list[int] = Field(
+ default_factory=list,
+ description=(
+ "User IDs to remove from dashboard owners. Rejected if it would "
+ "leave the dashboard with zero owners, or if an ID is not "
+ "currently an owner."
+ ),
+ )
+
+ @field_validator("identifier", mode="before")
+ @classmethod
+ def reject_bool_identifier(cls, value: object) -> object:
+ """bool is a subclass of int, so identifier=true would coerce to
+ dashboard ID 1 and mutate the wrong dashboard; reject it outright."""
+ if isinstance(value, bool):
+ raise ValueError("identifier must be an integer ID, UUID, or slug
string")
+ return value
+
+ @field_validator("add_owner_ids", "remove_owner_ids", mode="before")
+ @classmethod
+ def reject_bool_owner_ids(cls, value: object) -> object:
+ """bool is a subclass of int, so a `true`/`false` list element would
+ coerce to owner ID 1/0 and add/remove the wrong owner; reject it."""
+ if isinstance(value, list) and any(isinstance(item, bool) for item in
value):
+ raise ValueError("owner ID list items must be integers, not
booleans")
+ return value
+
+ @model_validator(mode="after")
+ def _validate_operations(self) -> "ManageDashboardOwnersRequest":
+ if not self.add_owner_ids and not self.remove_owner_ids:
+ raise ValueError(
+ "At least one of add_owner_ids or remove_owner_ids is
required."
+ )
+ overlap: list[int] = sorted(
+ set(self.add_owner_ids) & set(self.remove_owner_ids)
+ )
+ if overlap:
+ raise ValueError(
+ "User IDs cannot appear in both add_owner_ids and "
+ f"remove_owner_ids: {overlap}."
+ )
+ return self
+
+
+class DashboardMutationErrorFields(BaseModel):
+ """Shared ``error``/``permission_denied`` fields for dashboard governance
+ mutation responses (owners/roles/certification), including the
+ validator that wraps ``error`` before it is exposed to LLM context.
+ """
+
+ error: str | None = Field(None, description="Error message, if operation
failed")
+ permission_denied: bool = Field(
+ default=False,
+ description=("True when the user lacks edit rights on the target
dashboard."),
+ )
+
+ @field_validator("error")
+ @classmethod
+ def sanitize_error_for_llm_context(cls, value: str | None) -> str | None:
+ """Wrap error text before it is exposed to LLM context."""
+ if value is None:
+ return value
+ return sanitize_for_llm_context(value, field_path=("error",))
+
+
+class ManageDashboardOwnersResponse(DashboardMutationErrorFields):
+ """Response schema for ``manage_dashboard_owners``."""
+
+ owners: list[SubjectInfo] = Field(
+ default_factory=list,
+ description=(
+ "Full list of USER-type editor subjects (dashboard owners) "
+ "after the operation. Any ROLE/GROUP-type editors on the "
+ "dashboard are not included here."
+ ),
+ )
+ dashboard_url: str | None = Field(None, description="URL to view the
dashboard")
+ added_owner_ids: list[int] = Field(
+ default_factory=list,
+ description="User IDs actually added as owners by this call.",
+ )
+ removed_owner_ids: list[int] = Field(
+ default_factory=list,
+ description="User IDs actually removed from owners by this call.",
+ )
+ warnings: list[str] = Field(
+ default_factory=list,
+ description=(
+ "Non-fatal advisory messages, e.g. that a non-admin caller was "
+ "automatically re-added as owner after trying to remove "
+ "themselves."
+ ),
+ )
+
+ @field_validator("owners", mode="after")
+ @classmethod
+ def sanitize_owners_for_llm_context(
+ cls, value: list[SubjectInfo]
+ ) -> list[SubjectInfo]:
+ """Wrap owner labels before LLM exposure; owner display names are
+ user-controlled and render as plain text in this response, so an
+ unsanitized label could inject content into LLM context (CWE-79
+ analog for LLM-facing output). Entries that sanitize to an empty
+ label are dropped rather than surfaced with a blank identity."""
+ sanitized: list[SubjectInfo] = []
+ for subject in value:
+ if subject.label is None:
+ sanitized.append(subject)
+ continue
+ clean_label = sanitize_for_llm_context(
+ subject.label, field_path=("owners", "label")
+ )
+ if not clean_label:
+ continue
+ sanitized.append(subject.model_copy(update={"label": clean_label}))
+ return sanitized
+
+
+class ManageDashboardRolesRequest(BaseModel):
+ """Request schema for explicit add/remove dashboard RBAC role management.
+
+ Unlike ``update_dashboard``'s dropped ``roles`` field (a full-replacement
+ access-control list), this tool takes explicit add/remove operations.
+
+ "Roles" here means ROLE-type entries in the dashboard's Subject-based
+ ``viewers`` list (the access model apache/superset#38831 introduced,
+ replacing the legacy ``roles``/``DASHBOARD_RBAC`` relationship). Any
+ USER- or GROUP-type viewers already on the dashboard are left untouched
+ by this tool.
+ """
+
+ identifier: int | str = Field(
+ ...,
+ description=(
+ "Dashboard ID (integer), UUID, or slug. Same identifier shape "
+ "accepted by ``get_dashboard_info``."
+ ),
+ )
+ add_role_ids: list[int] = Field(
+ default_factory=list,
+ description=(
+ "Role IDs to grant dashboard access to. Discover IDs with
``list_roles``."
+ ),
+ )
+ remove_role_ids: list[int] = Field(
+ default_factory=list,
+ description=(
+ "Role IDs to revoke dashboard access from. Rejected if an ID is "
+ "not currently assigned to the dashboard."
+ ),
+ )
+
+ @field_validator("identifier", mode="before")
+ @classmethod
+ def reject_bool_identifier(cls, value: object) -> object:
+ """bool is a subclass of int, so identifier=true would coerce to
+ dashboard ID 1 and mutate the wrong dashboard; reject it outright."""
+ if isinstance(value, bool):
+ raise ValueError("identifier must be an integer ID, UUID, or slug
string")
+ return value
+
+ @field_validator("add_role_ids", "remove_role_ids", mode="before")
+ @classmethod
+ def reject_bool_role_ids(cls, value: object) -> object:
+ """bool is a subclass of int, so a `true`/`false` list element would
+ coerce to role ID 1/0 and grant/revoke the wrong role; reject it."""
+ if isinstance(value, list) and any(isinstance(item, bool) for item in
value):
+ raise ValueError("role ID list items must be integers, not
booleans")
+ return value
+
+ @model_validator(mode="after")
+ def _validate_operations(self) -> "ManageDashboardRolesRequest":
+ if not self.add_role_ids and not self.remove_role_ids:
+ raise ValueError(
+ "At least one of add_role_ids or remove_role_ids is required."
+ )
+ overlap: list[int] = sorted(set(self.add_role_ids) &
set(self.remove_role_ids))
+ if overlap:
+ raise ValueError(
+ "Role IDs cannot appear in both add_role_ids and "
+ f"remove_role_ids: {overlap}."
+ )
+ return self
+
+
+class ManageDashboardRolesResponse(DashboardMutationErrorFields):
+ """Response schema for ``manage_dashboard_roles``."""
+
+ roles: list[SubjectInfo] = Field(
+ default_factory=list,
+ description=(
+ "Full list of ROLE-type viewer subjects (dashboard access "
+ "roles) after the operation. Any USER/GROUP-type viewers on "
+ "the dashboard are not included here."
+ ),
+ )
+ dashboard_url: str | None = Field(None, description="URL to view the
dashboard")
+ added_role_ids: list[int] = Field(
+ default_factory=list,
+ description="Role IDs actually added by this call.",
+ )
+ removed_role_ids: list[int] = Field(
+ default_factory=list,
+ description="Role IDs actually removed by this call.",
+ )
+ viewers_enabled: bool = Field(
+ default=False,
+ description=(
+ "Whether the ENABLE_VIEWERS feature flag is enabled on this "
+ "instance. When False, dashboard viewers are stored but have no "
+ "effect on access control โ access still follows normal "
+ "Superset permissions/editorship."
+ ),
+ )
+ warnings: list[str] = Field(
+ default_factory=list, description="Non-fatal advisory messages."
+ )
+
+ @field_validator("roles", mode="after")
+ @classmethod
+ def sanitize_roles_for_llm_context(
+ cls, value: list[SubjectInfo]
+ ) -> list[SubjectInfo]:
+ """Wrap role labels before LLM exposure; role display names are
+ user-controlled and render as plain text in this response, so an
+ unsanitized label could inject content into LLM context (CWE-79
+ analog for LLM-facing output). Entries that sanitize to an empty
+ label are dropped rather than surfaced with a blank identity."""
+ sanitized: list[SubjectInfo] = []
+ for subject in value:
+ if subject.label is None:
+ sanitized.append(subject)
+ continue
+ clean_label = sanitize_for_llm_context(
+ subject.label, field_path=("roles", "label")
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation for this local variable to
comply with the projectโs type-hint rule for relevant variables. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is also new Python code in the PR, and the local variable `clean_label`
is not annotated even though it can be. That matches the custom type-hint rule,
so the violation is real.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4cf56cf0c0824a58944fed0cefd1240d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4cf56cf0c0824a58944fed0cefd1240d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/schemas.py
**Line:** 1346:1348
**Comment:**
*Custom Rule: Add an explicit type annotation for this local variable
to comply with the projectโs type-hint rule for relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=5d1fe783a346fd8c74a21c31a1241a65e84dfe2a3b58254bafcc29560c15d4e9&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=5d1fe783a346fd8c74a21c31a1241a65e84dfe2a3b58254bafcc29560c15d4e9&reaction=dislike'>๐</a>
--
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]
