feat(mcp): sync_custom_registry tool with org-scoped service refactor#2568
Merged
feat(mcp): sync_custom_registry tool with org-scoped service refactor#2568
Conversation
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
|
✅ No security or compliance issues detected. Reviewed everything up to dfa7e14. Security OverviewDetected Code Changes
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 8 files
Confidence score: 3/5
- There is some merge risk because
resolve_org_role_for_requestintracecat/mcp/auth.pyappears to ignore token org scoping, which can incorrectly reject valid single-org tokens for users with multiple org memberships. tracecat/registry/repositories/router.pycurrently surfaces raw exception text in 500 responses; sanitizing this to a generic message is important to avoid leaking internal details at API boundaries.- Pay close attention to
tracecat/mcp/auth.pyandtracecat/registry/repositories/router.py- auth role resolution may block legitimate access, and error handling should avoid exposing unexpected internal exception content.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tracecat/registry/repositories/router.py">
<violation number="1" location="tracecat/registry/repositories/router.py:129">
P2: Do not include raw exception messages in 500 responses; return a sanitized generic error message instead.
(Based on your team's feedback about sanitizing unexpected error responses at router boundaries.) [FEEDBACK_USED]</violation>
</file>
<file name="tracecat/mcp/auth.py">
<violation number="1" location="tracecat/mcp/auth.py:1330">
P1: `resolve_org_role_for_request` ignores token org scoping and resolves ambiguity from all memberships, which can reject valid single-org tokens for multi-org users.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
|
Found 8 test failures on Blacksmith runners: Failures
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd9d6e5b49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae11ef3119
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
…ctor
Introduce a workspace-less MCP tool for syncing the org's custom action
registry, and consolidate the sync pipeline onto RegistryReposService so
both the HTTP route and MCP tool share a single source of truth gated by
@require_scope("org:registry:update").
- Move full sync pipeline (entitlement check, force-version delete, SSH
context, action sync, repo update) into RegistryReposService.sync_repository
- Slim sync_registry_repository route to exception-to-HTTP translation; pre-
fetch repo once and reuse for the 422 validation-error path
- Add sync_custom_registry MCP tool that resolves org context from the token
via new resolve_org_role_for_request helper (mirrors API's
_resolve_org_for_regular_user multi-org error handling)
- Filter both DEFAULT_REGISTRY_ORIGIN and DEFAULT_LOCAL_REGISTRY_ORIGIN so
deployments running both still resolve to a single custom repo
- Gate list_repositories on org:registry:read (matches its semantics)
- Log repository_id rather than origin to avoid theoretical credential leakage
- Add docs entry, service-level scope tests, and full MCP tool coverage
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Honor token org scoping in resolve_org_role_for_request, sanitize the sync route's 500 detail, and replace the heavy fakes in the new MCP tests with stdlib AsyncMock. - resolve_org_role_for_request now intersects identity.organization_ids with the user's OrganizationMembership rows before the ambiguity check, mirroring list_user_workspaces. Single-org-scoped tokens resolve cleanly even when the user is a member of multiple orgs. - sync_registry_repository's 500 path returns a generic detail; the exception is still logged with full context. - tests/unit/test_mcp_server.py drops _UNSET, three handwritten fake classes, and two patch helpers; the nine sync_custom_registry tests now use AsyncMock + a single _patch_sync_custom_registry helper. - tests/unit/test_mcp_auth.py adds four focused tests for the new org-scoping intersection (scoped+multi-org, unscoped+single-org, unscoped+multi-org, token scoped to unowned org). Verified: ruff/format/basedpyright clean; 306 affected unit tests pass; 7 of the 8 CI temporal failures pass locally on this branch (test_workflow_table_actions_in_loop has an httpx ConnectError to the executor service that is environmental and unrelated to this changeset). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Single-tenant platform superusers don't carry OrganizationMembership rows, so the previous code rejected them with "no organization memberships" the moment sync_custom_registry routed through resolve_org_role_for_request. Mirror the HTTP API's _resolve_org_for_superuser pattern: when the user is a superuser and TRACECAT__EE_MULTI_TENANT is off, fall back to get_default_organization_id instead of querying memberships. Add a focused test covering the fallback path. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Pull get_default_organization_id (in mcp/auth.py) and get_setting/ssh_context/Entitlement/check_entitlement (in registry/repositories/service.py) up to module level. Verified non-circular by inspecting the import graph. Two imports remain lazy with a comment explaining why: - tracecat.git.utils.parse_git_url - tracecat.registry.actions.service.RegistryActionsService Both transitively import RegistryReposService back via tracecat.registry.repository, which would cycle if pulled to module scope. Update the superuser-fallback test to monkeypatch mcp_auth.get_default_organization_id directly now that the symbol is bound at module level. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The service method now accepts a RegistryRepository instance and trusts
the caller to have done the org-scoped lookup. Both real callers (the
HTTP route's get_repository_by_id and the MCP tool's list_repositories)
do filter by org, so no exploit exists today, but a future caller that
bypasses those lookups would expose IDOR.
Add an inline assertion on repository.organization_id == self.organization_id
inside sync_repository. On mismatch raise the existing domain exception
RegistryNotFound("Registry repository not found"); the route catches it
before the broader RegistryError clause and maps it to the same 404 it
already returns for missing IDs. Cross-org probing is therefore
indistinguishable from a missing repository.
Add a focused unit test covering the cross-org rejection.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
topher-lo
force-pushed
the
feat/registry-sync-mcp-2
branch
from
3428def to
dfa7e14
Compare
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfa7e14f9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
workflows_sync_custom_registryMCP tool so org admins can sync their custom action registry from any MCP client without picking a workspace.RegistryReposService.sync_repository, gated by@require_scope("org:registry:update"), so the existing HTTP route and the new MCP tool share one source of truth instead of duplicating entitlement / SSH / force-version-delete / action-sync logic.resolve_org_role_for_requesthelper that queriesOrganizationMembershipdirectly (same pattern as_resolve_org_for_regular_user).Why
#2565 added the same MCP tool but left the existing
POST /registry/repos/{id}/syncroute owning the full sync pipeline inline, with the service method re-implementing it next door. This PR keeps the new MCP capability while turning the route into a thin caller of the service so the two paths can't drift.Behavior changes
RegistryReposService.sync_repository(repository, sync_params)now takes aRegistryRepository(notrepository_id) so callers reuse the org-scoped lookup they already did. The route fetches once at the top for 404 handling and reuses the same instance for the 422 validation-error body.RegistryReposService.list_repositoriesis gated onorg:registry:read— it had no other callers, and read scope matches the operation's intent.sync_custom_registryfilters out bothDEFAULT_REGISTRY_ORIGINandDEFAULT_LOCAL_REGISTRY_ORIGINso a deployment running both built-ins still resolves to a single custom repo. The "exactly one custom registry" check stays as a defensive guard documented inline.workspace_id; the org is resolved from the auth token. Multi-org tokens get a clear error matching the HTTP API's behavior.sync_repositoryand the newUpdated repositorylog line key offrepository_idrather thanoriginto remove a theoretical credential-leak path.Test plan
uv run pytest tests/unit/test_registry_repositories_service.py tests/unit/test_registry_repositories_router.py tests/unit/api/test_api_registry_repositories_entitlements.py tests/unit/test_route_scope_guards.py tests/unit/test_mcp_server.py(256 passed)uv run ruff checkanduv run ruff format --checkon changed filesuv run basedpyright --warnings tracecat/registry/repositories tracecat/mcp/server.py tracecat/mcp/auth.py(0 errors, 0 warnings)POST /registry/repos/{id}/syncagainst a registered git+ssh repo viajust cluster up -dand confirm response shape is unchanged.workflows_sync_custom_registryfrom an MCP client against an org with one custom registry; confirm success and per-repo result fields.No custom registry repository foundToolError.🤖 Generated with Claude Code
Summary by cubic
Adds the
workflows_sync_custom_registryMCP tool to sync an org’s custom action registry without a workspace, and consolidates the sync flow intoRegistryReposService.sync_repositorywith org-scoped scopes and entitlements. Honors token org scoping (with single-tenant superuser default‑org fallback), adds a cross‑org 404, and returns generic 500s.New Features
workflows_sync_custom_registry: sync to HEAD ortarget_commit_sha, optionalforce, and clear ToolErrors for missing scope or org context; filters outDEFAULT_REGISTRY_ORIGINandDEFAULT_LOCAL_REGISTRY_ORIGINand reports per‑repo results (version,commit_sha,actions_count, errors).resolve_org_role_for_request: intersects token org scope with memberships; clear errors for multi‑org tokens and unowned scopes; single‑tenant superusers fall back to the default org.Refactors
RegistryReposService.sync_repository, gated by@require_scope("org:registry:update"), with entitlement checks for non‑default repos; route now delegates and returns a generic 500 detail.RegistryReposService.list_repositoriesnow requiresorg:registry:read;sync_repositoryrejects cross‑org repositories withRegistryNotFound(mapped to 404); logs userepository_idinstead of origin.Written for commit dfa7e14. Summary will update on new commits.