refactor: tool credentials configuration#590
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces scoped tool credentials (supporting user, instance, and shared scopes) to replace the legacy tool configuration credentials. It adds a new scoped_tool_credentials database table, corresponding migration scripts, backend API endpoints, and a frontend ToolCredentialsPanel component to manage these credentials. The review feedback highlights critical issues with the use of sqlite_where in SQLAlchemy index definitions and Alembic migrations, which is silently ignored on SQLite and leads to IntegrityErrors. Additionally, the feedback points out a bug where an admin's personal credentials are leaked into the instance default view, and a UI state inconsistency when bulk credential deletion fails midway.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| __table_args__ = ( | ||
| Index( | ||
| "uq_scoped_tool_credential_scoped", | ||
| "scope_type", | ||
| "scope_id", | ||
| "tool_name", | ||
| "field_name", | ||
| unique=True, | ||
| sqlite_where=scope_id.is_not(None), | ||
| postgresql_where=scope_id.is_not(None), | ||
| ), | ||
| Index( | ||
| "uq_scoped_tool_credential_instance", | ||
| "tool_name", | ||
| "field_name", | ||
| unique=True, | ||
| sqlite_where=(scope_type == "instance") & scope_id.is_(None), | ||
| postgresql_where=(scope_type == "instance") & scope_id.is_(None), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
In SQLAlchemy, sqlite_where is not a valid parameter for Index and is silently ignored by the SQLite compiler. Because it is ignored, the unique index uq_scoped_tool_credential_instance is created as a global unique index on (tool_name, field_name) without any WHERE clause on SQLite. This causes an IntegrityError when any user tries to configure a personal credential for a tool that already has an instance default configured.
To fix this, remove sqlite_where and rely solely on postgresql_where, which SQLAlchemy's SQLite dialect also uses to compile partial indexes.
__table_args__ = (
Index(
"uq_scoped_tool_credential_scoped",
"scope_type",
"scope_id",
"tool_name",
"field_name",
unique=True,
postgresql_where=scope_id.is_not(None),
),
Index(
"uq_scoped_tool_credential_instance",
"tool_name",
"field_name",
unique=True,
postgresql_where=(scope_type == "instance") & scope_id.is_(None),
),
)There was a problem hiding this comment.
Checked this against SQLAlchemy 2.0.47. sqlite_where is the SQLite dialect option for partial indexes; it is not ignored. Compiling this index for SQLite emits a partial unique index with the expected WHERE clause. postgresql_where alone is ignored by SQLite and would create the global (tool_name, field_name) unique index described here.
Since the model declares both sqlite_where and postgresql_where, it supports both SQLite and PostgreSQL. No change needed here.
| if "uq_scoped_tool_credential_scoped" not in indexes: | ||
| op.create_index( | ||
| "uq_scoped_tool_credential_scoped", | ||
| "scoped_tool_credentials", | ||
| ["scope_type", "scope_id", "tool_name", "field_name"], | ||
| unique=True, | ||
| sqlite_where=sa.column("scope_id").is_not(None), | ||
| postgresql_where=sa.column("scope_id").is_not(None), | ||
| ) | ||
| if "uq_scoped_tool_credential_instance" not in indexes: | ||
| op.create_index( | ||
| "uq_scoped_tool_credential_instance", | ||
| "scoped_tool_credentials", | ||
| ["tool_name", "field_name"], | ||
| unique=True, | ||
| sqlite_where=and_( | ||
| sa.column("scope_type") == "instance", | ||
| sa.column("scope_id").is_(None), | ||
| ), | ||
| postgresql_where=and_( | ||
| sa.column("scope_type") == "instance", | ||
| sa.column("scope_id").is_(None), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
In SQLAlchemy/Alembic, sqlite_where is not a valid parameter for op.create_index and is silently ignored by the SQLite compiler. Because it is ignored, the unique index uq_scoped_tool_credential_instance is created as a global unique index on (tool_name, field_name) without any WHERE clause on SQLite. This causes an IntegrityError when any user tries to configure a personal credential for a tool that already has an instance default configured.
To fix this, remove sqlite_where and rely solely on postgresql_where, which SQLAlchemy's SQLite dialect also uses to compile partial indexes.
if "uq_scoped_tool_credential_scoped" not in indexes:
op.create_index(
"uq_scoped_tool_credential_scoped",
"scoped_tool_credentials",
["scope_type", "scope_id", "tool_name", "field_name"],
unique=True,
postgresql_where=sa.column("scope_id").is_not(None),
)
if "uq_scoped_tool_credential_instance" not in indexes:
op.create_index(
"uq_scoped_tool_credential_instance",
"scoped_tool_credentials",
["tool_name", "field_name"],
unique=True,
postgresql_where=and_(
sa.column("scope_type") == "instance",
sa.column("scope_id").is_(None),
),
)There was a problem hiding this comment.
Same verification for the Alembic migration. op.create_index(..., sqlite_where=...) passes the SQLite predicate through and emits the expected partial index. Removing sqlite_where and relying only on postgresql_where would remove the SQLite predicate and create a global unique index.
The current migration includes both dialect-specific predicates, so SQLite and PostgreSQL are both covered. No change needed here.
| if not bool(getattr(current_user, "is_admin", False)): | ||
| raise HTTPException(status_code=403, detail="Admin privileges required") | ||
| return { | ||
| "scope_type": "instance", | ||
| "scope_id": None, | ||
| "user_id": user_id, | ||
| "user": current_user, | ||
| "include_instance": True, | ||
| } |
There was a problem hiding this comment.
When retrieving the credential view for the instance scope, passing the admin's personal user_id and user to the context causes the resolution fallback logic to check and display the admin's personal credentials as the resolved values in the instance default view. This is highly confusing as it pollutes the instance configuration view with personal credentials.
Setting user_id and user to None when scope == "instance" ensures that the instance default view only resolves and displays instance-level and environment-level credentials.
| if not bool(getattr(current_user, "is_admin", False)): | |
| raise HTTPException(status_code=403, detail="Admin privileges required") | |
| return { | |
| "scope_type": "instance", | |
| "scope_id": None, | |
| "user_id": user_id, | |
| "user": current_user, | |
| "include_instance": True, | |
| } | |
| if not bool(getattr(current_user, "is_admin", False)): | |
| raise HTTPException(status_code=403, detail="Admin privileges required") | |
| return { | |
| "scope_type": "instance", | |
| "scope_id": None, | |
| "user_id": None, | |
| "user": None, | |
| "include_instance": True, | |
| } |
| const failedResponse = responses.find((response) => !response.ok) | ||
| if (failedResponse) { | ||
| const err = await failedResponse.json() | ||
| toast.error(err.detail || t("tools.credentials.deleteAllFailed")) | ||
| return | ||
| } |
There was a problem hiding this comment.
If some of the deletions succeed but one fails, the function returns early without reloading the credentials. This leaves the UI in an inconsistent state where some credentials are deleted in the backend but still shown as configured in the frontend.
Calling await loadCredentials() before returning on failure ensures that the UI is always refreshed to reflect the actual state of successfully deleted credentials.
| const failedResponse = responses.find((response) => !response.ok) | |
| if (failedResponse) { | |
| const err = await failedResponse.json() | |
| toast.error(err.detail || t("tools.credentials.deleteAllFailed")) | |
| return | |
| } | |
| const failedResponse = responses.find((response) => !response.ok) | |
| if (failedResponse) { | |
| const err = await failedResponse.json() | |
| toast.error(err.detail || t("tools.credentials.deleteAllFailed")) | |
| await loadCredentials() | |
| return | |
| } |
fix(tool-credentials): show database as normal tool fix(tool-credentials): remove database standalone section fix(tool-credentials): use full-width database fields fix(tool-credentials): resolve scoped runtime credentials fix(tool-credentials): hide unavailable instance message feat(tool-credentials): delete all tool credentials feat(tool-credentials): show credential sources ref(tool-credentials): decouple team scope fix(tool-credentials): translate inherited sources fix(tool-credentials): localize credential tool labels
638c4ae to
b4c9caa
Compare
|
Is this ready for review? |
No description provided.