fix(github-work-app): Make gitHub MCP access schema project scoped#2574
fix(github-work-app): Make gitHub MCP access schema project scoped#2574miles-kt-inkeep merged 10 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 7050d4e The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Scopes GitHub MCP tool access control to
|
There was a problem hiding this comment.
Medium urgency. The schema migration and auth layer look correct. The main concern: several data-access functions on the sibling workAppGitHubMcpToolRepositoryAccess table (getMcpToolRepositoryAccess, clearMcpToolRepositoryAccess, and the 'selected' mode path in getMcpToolRepositoryAccessWithDetails) still filter by toolId only — inconsistent with the project-scoping this PR establishes. If tool IDs are globally unique today this is functionally safe, but it undermines the defense-in-depth intent of the change. Test coverage for cross-project isolation is also missing.
| @@ -960,7 +968,7 @@ export const getMcpToolRepositoryAccessWithDetails = | |||
| workAppGitHubInstallations, | |||
| eq(workAppGitHubRepositories.installationDbId, workAppGitHubInstallations.id) | |||
There was a problem hiding this comment.
Inconsistent scoping in 'selected' mode path. This query still filters workAppGitHubMcpToolRepositoryAccess by scope.toolId only, while every other query in this PR now filters by the full (tenantId, projectId, toolId) compound key. The workAppGitHubMcpToolRepositoryAccess table has tenantId and projectId columns — this WHERE clause should include them for consistency and defense-in-depth.
The same issue applies to:
getMcpToolRepositoryAccess(~line 900) — filters bytoolIdonlyclearMcpToolRepositoryAccess(~line 984) — deletes bytoolIdonlysetMcpToolRepositoryAccess(~line 860) — deletes bytoolIdonly before re-insertingcascadeDeleteByToolincascade-delete.ts(~line 458) — deletes repository access bytoolIdonly
If tool IDs are globally unique today, this is functionally safe, but it contradicts the scoping model this PR establishes.
| }, | ||
| (table) => [ | ||
| primaryKey({ columns: [table.toolId] }), | ||
| primaryKey({ columns: [table.tenantId, table.projectId, table.toolId] }), |
There was a problem hiding this comment.
The PK change on this table is correct. Worth checking: the sibling workAppGitHubMcpToolRepositoryAccess table (~line 808) still has its unique constraint scoped as (toolId, repositoryDbId) without tenant/project. If the same toolId can now legitimately appear in different projects, that unique constraint would need updating too.
| tenantId: ctx.tenantId, | ||
| projectId, | ||
| toolId, | ||
| }); |
There was a problem hiding this comment.
Missing cross-project isolation test. The whole point of making this project-scoped is that access configured for project A should not be visible when querying with project B's scope. Add a test that sets access for { tenantId, projectId: 'project-A', toolId }, then queries with { tenantId, projectId: 'project-B', toolId } and asserts an empty result.
| tenantId: ctx.tenantId, | ||
| projectId, | ||
| toolId, | ||
| }); |
There was a problem hiding this comment.
getMcpToolAccessMode and deleteMcpToolAccessMode were both changed to accept a { tenantId, projectId, toolId } scope object, but neither has direct test coverage in this file. Consider adding tests that verify: (1) correct mode returned after setMcpToolAccessMode, (2) deleteMcpToolAccessMode clears it, and (3) querying with a different project scope returns the default 'selected' mode.
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
🟠 1) github-work-app-installations.ts:969 Incomplete scope filtering in 'selected' mode repository query
Issue: When mode is 'selected', the query only filters by scope.toolId without also filtering by scope.tenantId and scope.projectId. This inconsistency means the repository access query doesn't use the full composite scope that was the purpose of this PR.
Why: For defense-in-depth and consistency with the access mode query (lines 930-932 which correctly use all three scope fields), this query should also scope by tenantId and projectId. While the workAppGitHubMcpToolRepositoryAccess table does have these columns, not filtering by them could lead to edge cases in multi-tenant scenarios.
Fix: See inline comment with suggestion block.
Refs:
🟠 2) system Missing changeset for schema migration and API contract change
Issue: This PR introduces a database schema migration that changes the primary key structure of work_app_github_mcp_tool_access_mode and modifies function signatures in @inkeep/agents-core (e.g., getMcpToolAccessMode, getMcpToolRepositoryAccessWithDetails, deleteMcpToolAccessMode now require a scope object instead of just toolId). These are changes to exported functions in published packages.
Why: Per AGENTS.md: "Any bug fix, feature, or behavior change to a published package... needs a changeset." This applies to @inkeep/agents-core, @inkeep/agents-work-apps, and @inkeep/agents-api.
Fix: Run: pnpm bump patch --pkg agents-core --pkg agents-work-apps --pkg agents-api "Fix GitHub MCP tool access to be project-scoped instead of globally scoped by toolId"
Refs:
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
auth.ts:84-88API key validation inconsistent with Slack MCP pattern (timing-safe comparison + null check) - 🟡 Minor:
0021_careful_xorn.sql:2Migration file missing trailing newline
💭 Consider (2) 💭
💭 1) github/mcp/index.ts:83-85 Use createApiError for 'No repository access' error
Issue: The getServer function throws a generic Error instead of using createApiError. This will propagate as an unstructured 500 error rather than a properly formatted API error response.
Why: Consistency with the auth middleware error handling pattern and better client-side error handling.
Fix: Replace with createApiError({ code: 'forbidden', message: 'No repository access configured for this tool' })
Refs:
💭 2) github/mcp/auth.ts Missing unit tests for new header validation
Issue: The new header validation for x-inkeep-tenant-id and x-inkeep-project-id (lines 27-53) lacks unit tests. The Slack MCP auth middleware has comprehensive tests in /packages/agents-work-apps/src/__tests__/slack/mcp/auth.test.ts.
Why: Test coverage for authentication boundary changes ensures regressions are caught.
Fix: Create a test file /packages/agents-work-apps/src/__tests__/github/mcp/auth.test.ts mirroring the Slack MCP auth test structure.
Refs:
🚫 REQUEST CHANGES
Summary: This PR correctly aligns GitHub MCP tool access with the multi-tenant architecture pattern, matching the Slack MCP implementation from PR #2545. The schema change and most data access updates are well-implemented. However, there's an incomplete scope filter in the repository access query for 'selected' mode (line 969) that should be addressed for consistency and defense-in-depth. Additionally, please add a changeset for the published package changes.
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
index.ts:1632-1656 |
POST handler lacks error handling around getServer/connect | Pre-existing code structure, not introduced by this PR |
index.ts:1633-1634 |
Environment variable check format | Pre-existing, not in PR scope |
index.ts:72 |
Local ToolScope type duplicates ToolScopeConfig | Matches existing Slack MCP pattern, intentional codebase convention |
migration |
Potential duplicate key risk | Medium confidence, toolIds are UUIDs and unlikely to conflict; risk is theoretical |
cascade-delete.ts |
Missing cross-project isolation test | Low severity edge case, main functionality is tested |
githubMcpToolAccess.test.ts |
Missing tests for getMcpToolAccessMode scoping | Tests exist for getMcpToolRepositoryAccessWithDetails which covers the scoping pattern |
auth.ts:27-53 |
New required headers breaking internal API contract | Both known callers are updated in this PR |
| Architecture positive | Correct alignment with multi-tenant architecture | Positive observation, no action needed |
Reviewers (9)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
1 | 1 | 0 | 0 | 1 | 0 | 0 |
pr-review-devops |
4 | 1 | 0 | 0 | 1 | 0 | 2 |
pr-review-tests |
3 | 0 | 1 | 0 | 0 | 0 | 2 |
pr-review-consistency |
6 | 0 | 0 | 0 | 1 | 0 | 4 |
pr-review-breaking-changes |
2 | 0 | 0 | 0 | 1 | 0 | 1 |
pr-review-errors |
3 | 0 | 1 | 0 | 0 | 0 | 2 |
pr-review-architecture |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-types |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 21 | 2 | 2 | 0 | 4 | 0 | 13 |
🤖 Generated with Claude Code
|
@claude add a changeset |
|
Claude finished @miles-kt-inkeep's task in 1m 22s —— View job Adding Changeset
Added changeset |
Co-authored-by: miles-kt-inkeep <[email protected]>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta re-review scoped to changes since the last automated review (commit 497ad36c2).
✅ Prior Issues Addressed
The previous review flagged two issues — both have been resolved:
| Prior Issue | Status |
|---|---|
| 🟠 Incomplete scope filtering in 'selected' mode repository query (line 969) | ✅ Fixed — Query now filters by (tenantId, projectId, toolId) at lines 971-976 |
| 🟠 Missing changeset | ✅ Added — .changeset/project-scoped-github-mcp.md properly covers agents-core, agents-work-apps, agents-api |
Delta Verification
The fix at lines 971-976 correctly implements the full composite scope filter:
.where(
and(
eq(workAppGitHubMcpToolRepositoryAccess.tenantId, scope.tenantId),
eq(workAppGitHubMcpToolRepositoryAccess.projectId, scope.projectId),
eq(workAppGitHubMcpToolRepositoryAccess.toolId, scope.toolId)
)
)This pattern now matches the access mode query (lines 930-935), providing consistent tenant/project isolation throughout the getMcpToolRepositoryAccessWithDetails function.
🕐 Pending Recommendations (1)
- 🟡 Sibling functions still use toolId-only filtering —
getMcpToolRepositoryAccessandclearMcpToolRepositoryAccessstill filter bytoolIdonly. As noted in the prior pullfrog review, this is pre-existing and these functions are only used in tests, so it's functionally safe but inconsistent with the project-scoping model. Consider addressing in a follow-up PR for defense-in-depth.
✅ APPROVE
Summary: The delta correctly addresses all feedback from the previous review. The scope filtering fix is properly implemented, matching the existing pattern in the file. The changeset is complete. Ship it! 🚀
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta re-review with minimal scope — both reviewers confirmed the fix is correct with no new issues.
🤖 Generated with Claude Code
Ito Test Report ❌16 test cases ran. 15 passed, 1 failed. 🔍 The run shows broad coverage of GitHub MCP access controls, scoping, and validation behavior with most checks passing. One reproducible defect remains in rapid-save handling: overlapping save requests can persist an older repository selection instead of the final intended state. ✅ Passed (15)
❌ Failed (1)
Rapid Save spam does not corrupt final access config – Failed
📋 View Recording |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta re-review scoped to changes since the last automated review (commit d5633672bc8d).
✅ Delta Analysis
The delta since the last APPROVE consists of 6 merge commits integrating changes from main:
| Commit | Description |
|---|---|
db5fe6584 |
Merge branch 'main' |
d7c10011f |
chore: add new models [model-sync] |
396f858c2 |
update model sync |
afa607507 |
ci: skip duplicate merge-queue push jobs |
959d5b49f |
updated claude prompt again |
0451e1d93 |
chore: add new models from provider APIs [model-sync] |
Conclusion: These changes are unrelated to the PR's feature scope (project-scoped GitHub MCP access). The core PR files have not been modified since the prior APPROVE.
✅ Core PR Verification
The project-scoping implementation remains correctly implemented:
- Schema migration changes PK to
(tenant_id, project_id, tool_id)✓ getMcpToolRepositoryAccessWithDetailsfilters by full scope (lines 930-935, 971-976) ✓getMcpToolAccessModefilters by full scope (lines 1134-1140) ✓deleteMcpToolAccessModefilters by full scope (lines 1157-1163) ✓cascadeDeleteByToolfilters by full scope for access mode (lines 464-470) ✓- Auth middleware validates
x-inkeep-tenant-idandx-inkeep-project-idheaders ✓ - Changeset properly covers agents-core, agents-work-apps, agents-api ✓
🕐 Pending Recommendations (1)
- 🟡 Sibling functions still use toolId-only filtering —
getMcpToolRepositoryAccess(line 900),clearMcpToolRepositoryAccess(line 994), and the repository access delete incascadeDeleteByTool(line 458) still filter bytoolIdonly. As noted in prior reviews, these functions are only used in tests and the change is functionally safe since tool IDs are globally unique, but it's inconsistent with the project-scoping model. Consider addressing in a follow-up PR for defense-in-depth.
📋 Ito Test Report Note
The Ito test failure (ADV-3: rapid save race condition) is a pre-existing UI concern unrelated to this PR's schema changes. The issue involves concurrent save requests in the UI dialog that can produce nondeterministic results — this is a client-side concurrency issue, not a scoping bug.
✅ APPROVE
Summary: The delta contains only merge commits from main (model sync, CI updates) with no changes to the PR's feature scope. The prior APPROVE stands — the project-scoping implementation is correct and complete. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Delta re-review with minimal scope — no subagent reviewers dispatched as the delta contains only merge commits unrelated to the PR's feature scope.
🤖 Generated with Claude Code
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta re-review scoped to changes since the last automated review (commit db5fe65847c0).
✅ Delta Analysis
The delta consists of a single commit (5a613473f) that completes the project-scoping implementation by updating the remaining data access functions:
| Function | Change |
|---|---|
getMcpToolRepositoryAccess |
Signature: (toolId) → (scope: { tenantId, projectId, toolId }); query now filters by all three |
clearMcpToolRepositoryAccess |
Signature: (toolId) → (scope: { tenantId, projectId, toolId }); query now filters by all three |
setMcpToolRepositoryAccess |
Delete clause now filters by (tenantId, projectId, toolId) |
cascadeDeleteByTool |
Repository access delete now filters by (tenantId, projectId, toolId) |
All test call sites in githubMcpToolAccess.test.ts and githubProjectAccess.test.ts have been updated to pass the full scope object.
✅ Prior Feedback Fully Addressed
The pending recommendation from previous reviews flagged that sibling functions still used toolId-only filtering. This delta fully resolves that feedback:
- ✅
getMcpToolRepositoryAccess— Now filters by(tenantId, projectId, toolId) - ✅
clearMcpToolRepositoryAccess— Now filters by(tenantId, projectId, toolId) - ✅
setMcpToolRepositoryAccess— Delete clause uses full scope - ✅
cascadeDeleteByTool— Repository access delete uses full scope
✅ Verification
-
Tenant/Project Isolation: All data access functions now properly enforce tenant/project boundaries, matching the schema's composite primary key
(tenant_id, project_id, tool_id). -
Test Coverage: The existing test
"should not affect tools in other projects"ingithubProjectAccess.test.tsvalidates cross-project isolation. The test"should only clear access for the specified tool"validates tool isolation within the same project. -
Cascade Delete: The
cascadeDeleteByToolfunction now properly scopes both the access mode and repository access deletes by(tenantId, projectId, toolId).
✅ APPROVE
Summary: The delta correctly addresses all pending recommendations from prior reviews. All MCP tool access functions now filter by the full (tenantId, projectId, toolId) scope, completing the defense-in-depth pattern for GitHub MCP tool access. The implementation is consistent, test coverage is comprehensive, and the project-scoping effort is now complete. Ship it! 🚀
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta re-review with minimal scope — all reviewers confirmed the delta correctly addresses prior feedback with no new issues.
🤖 Generated with Claude Code
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta re-review scoped to changes since the last automated review (commit 7611381f1da3).
✅ Delta Analysis
The delta since the last APPROVE consists of a merge commit integrating changes from main:
| Commit | Description |
|---|---|
7050d4ea9 |
Merge branch 'main' into efix/project-scoped-github-work-app |
d7827b34f |
add <FlowButton />, fix svg icon color for Ship and Agent Settings toolbar buttons (#2582) |
All changes in the delta are UI-related (FlowButton component, toolbar refactoring in agents-manage-ui). These are unrelated to the PR's feature scope (project-scoped GitHub MCP access).
✅ Core PR Verification
The project-scoping implementation remains correctly implemented:
| Function | Status |
|---|---|
Schema PK changed to (tenant_id, project_id, tool_id) |
✅ |
getMcpToolRepositoryAccess |
✅ Filters by full scope |
getMcpToolRepositoryAccessWithDetails |
✅ Filters by full scope |
clearMcpToolRepositoryAccess |
✅ Filters by full scope |
getMcpToolAccessMode |
✅ Filters by full scope |
deleteMcpToolAccessMode |
✅ Filters by full scope |
cascadeDeleteByTool |
✅ Filters by full scope for both tables |
Auth middleware (x-inkeep-tenant-id, x-inkeep-project-id) |
✅ |
| Changeset | ✅ Covers agents-core, agents-work-apps, agents-api |
📋 Ito Test Report Note
The Ito test failure (ADV-3: rapid save race condition) is a pre-existing UI concern unrelated to this PR's schema changes. The issue involves concurrent save requests in the UI dialog that can produce nondeterministic results — this is a client-side concurrency issue in the WorkAppGitHubAccessEditDialog, not a scoping bug introduced by this PR.
✅ APPROVE
Summary: The delta contains only merge commits from main (UI component refactoring) with no changes to the PR's feature scope. The prior APPROVE stands — the project-scoping implementation is correct and complete. All data access functions properly filter by the full (tenantId, projectId, toolId) composite scope. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta re-review with minimal scope — no subagent reviewers dispatched as the delta contains only merge commits unrelated to the PR's feature scope.
🤖 Generated with Claude Code
Ito Test Report ❌18 test cases ran. 17 passed, 1 failed. ✅ Most GitHub MCP access-scoping, authorization-header validation, isolation, and resilience checks behaved as expected. 🔍 One create-flow regression remains: a GitHub MCP server created in selected-repository mode can land on the details page with empty repository scope, indicating selected access is not reliably persisted/read after creation. ✅ Passed (17)
❌ Failed (1)
Create GitHub MCP server from Work Apps catalog – Failed
📋 View Recording |
No description provided.