feat: Standardize permission checks with x-authz OpenAPI metadata#2089
feat: Standardize permission checks with x-authz OpenAPI metadata#2089omar-inkeep merged 12 commits intomainfrom
Conversation
Introduce a consistent authorization metadata system for all API routes
using a new x-authz OpenAPI extension. This makes authorization requirements
machine-readable and self-documenting in the OpenAPI spec.
Key changes:
- Add createProtectedRoute() helper that attaches x-authz metadata and
middleware to route definitions in a single declaration
- Add AuthzMeta type with optional resource/permission and required
description fields
- Standardize resource/permission taxonomy:
- project/{view,edit,use} — SpiceDB permission checks
- organization/{admin,member} — org-level role checks
- description-only for complex/inherited auth patterns
- noAuth() registers no metadata (routes appear with security: [])
- inheritedAuth() for routes where parent middleware handles auth
- Fix dev-session route ordering in createApp.ts (must be registered
before the catch-all /api/auth/* handler)
- Update OpenAPI snapshot with new x-authz metadata on all 240 routes
- Consolidate tenant/member -> organization/member,
organization/authenticated -> organization/member,
workApp/orgAdmin -> organization/admin
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 9267b9a The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(8) Total Issues | Risk: High
🔴❗ Critical (2) ❗🔴
🔴 1) oauth.ts Deleted OAuth login route breaks MCP packages and docs
files: agents-api/src/domains/manage/routes/oauth.ts, packages/agents-mcp/src/funcs/oAuthInitiateOauthLoginPublic.ts, packages/agents-manage-mcp/src/funcs/oAuthInitiateOauthLoginPublic.ts, agents-docs/content/api-reference/(openapi)/oauth.mdx
Issue: The route /manage/oauth/login (operationId: initiate-oauth-login-public) has been deleted and replaced with /manage/tenants/{tenantId}/projects/{projectId}/tools/{id}/oauth/login. The new route has a different path structure, different operationId (initiate-tool-oauth-login), uses path params instead of query params, and requires project view permission instead of being public.
Why: This is a breaking change with immediate downstream impact:
- MCP SDK packages are broken —
packages/agents-mcpandpackages/agents-manage-mcpstill reference the old path atpathToFunc('/manage/oauth/login'). Any MCP client using the OAuth tool will get 404 errors. - Documentation references deleted route —
agents-docs/content/api-reference/(openapi)/oauth.mdxstill includes the old operationId. - UI was updated —
mcp-urls.tswas updated to the new path, but external consumers were not.
Fix: Choose one of:
- Regenerate MCP packages — Run the Speakeasy SDK generation workflow (
.github/workflows/speakeasy-sdk.yml) after this PR merges, then update documentation. - Add backward compatibility — Keep the old route as a redirect to the new route for a deprecation period.
- Mark as breaking — Add a changeset with major version bump and migration guide.
Refs:
🔴 2) createApp.ts:243-244 Dead middleware for deleted route
Issue: oauthRefMiddleware and branchScopedDbMiddleware are still registered for /manage/oauth/login but this route has been deleted.
Why: The middleware will never execute. This is dead code that adds confusion about which routes exist.
Fix: Remove lines 243-244 from createApp.ts:
app.use('/manage/oauth/login', async (c, next) => oauthRefMiddleware(c, next));
app.use('/manage/oauth/login', async (c, next) => branchScopedDbMiddleware(c, next));Refs:
🟠⚠️ Major (3) ⚠️ 🟠
🟠 1) package.json Missing changeset for new public export
file: packages/agents-core/package.json:97-100
Issue: This PR adds a new public export ./middleware to @inkeep/agents-core. This is a customer-facing API surface change exposing createProtectedRoute, inheritedAuth, noAuth, and related utilities.
Why: Since @inkeep/agents-core is a published npm package, any new export constitutes a minor semver change and requires a changeset per AGENTS.md guidelines.
Fix: Create a changeset:
pnpm bump minor --pkg agents-core "Add middleware export with createProtectedRoute for standardized route permission handling"Refs:
Inline Comments:
- 🟠 Major:
requirePermission.ts:65Hardcoded error message doesn't reflect actual required permission - 🟠 Major:
requirePermission.ts:97-101x-authz metadata doesn't reflect actual permission being checked
🟡 Minor (3) 🟡
🟡 1) AGENTS.md Should document the createProtectedRoute pattern
Issue: This PR introduces a significant new routing pattern enforced via Biome lint rules. AGENTS.md has no mention of createProtectedRoute() or the authorization metadata system.
Why: AI coding agents and contributors will encounter lint errors when creating new routes without guidance on the pattern.
Fix: Add a section to AGENTS.md documenting:
- Always use
createProtectedRoute()from@inkeep/agents-core/middleware - Pass a
permissionmiddleware (e.g.,requireProjectPermission('view')) - For public routes use
noAuth() - For routes with inherited auth use
inheritedAuth()variants
Refs:
Inline Comments:
- 🟡 Minor:
sessionAuth.ts:38-40Missing resource/permission fields unlike sibling middleware - 🟡 Minor:
inherited-auth.ts:47-51inheritedRunApiKeyAuth lacks structured metadata - 🟡 Minor:
biome.jsonc:67-74Lint rule message could mention noAuth()
💭 Consider (3) 💭
💭 1) AuthzMeta type could use discriminated union
Issue: The AuthzMeta type uses optional fields for resource and permission which allows ambiguous states like { resource: 'project', description: '...' } (permission undefined).
Fix: Consider a discriminated union to make the patterns explicit:
type AuthzMeta =
| { resource: string; permission: string; description: string }
| { resource?: never; permission?: never; description: string };Refs:
Inline Comments:
- 💭 Consider:
create-protected-route.ts:39Silent fallback to security: [] when no metadata - 💭 Consider:
manageAuth.ts:197as anycast for middleware delegation
🕐 Pending Recommendations (0)
No prior unresolved feedback.
🚫 REQUEST CHANGES
Summary: This PR introduces a well-designed authorization metadata system that improves API documentation and enforces consistent permission patterns. However, there are two critical issues that need to be addressed before merging:
-
Breaking change — The OAuth login route deletion breaks the MCP SDK packages and documentation. Either regenerate the packages post-merge, add backward compatibility, or explicitly mark this as a breaking change with a migration path.
-
Missing changeset — The new
@inkeep/agents-core/middlewareexport is a user-facing API change that requires a changeset.
The requirePermission middleware also has issues with hardcoded error messages and metadata that don't reflect the actual permissions being checked — this creates incorrect OpenAPI documentation and confusing error messages.
Discarded (12)
| Location | Issue | Reason Discarded |
|---|---|---|
projects.ts:30-31 |
Import path style inconsistency | Valid but pre-existing pattern — uses src/ prefix consistently throughout the file |
workflowProcess.ts:15 |
Public endpoint security concern | Intentional design — cron endpoint with infrastructure-level security |
| Security-IAM findings | All INFO-level observations | All 7 findings confirmed existing patterns are correct and safe |
apiKeys.ts:33 |
Permission field ordering | Cosmetic inconsistency with very low impact |
evalsAuth.ts:91-93 |
Description-only metadata | Intentional for service-to-service auth — documenting is acceptable |
| Taxonomy consolidation | Observation about tenant→organization rename | Positive observation, no action needed |
| Phantom type limitations | TypeScript structural typing concerns | Acceptable trade-off for developer ergonomics |
| x-authz documentation-only nature | Metadata doesn't participate in runtime | Expected design — documentation feature only |
Reviewers (8)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-breaking-changes |
6 | 2 | 0 | 0 | 0 | 0 | 4 |
pr-review-product |
5 | 2 | 0 | 0 | 0 | 0 | 3 |
pr-review-devops |
5 | 2 | 0 | 0 | 1 | 0 | 2 |
pr-review-consistency |
8 | 0 | 0 | 0 | 3 | 0 | 5 |
pr-review-architecture |
8 | 0 | 1 | 0 | 0 | 0 | 7 |
pr-review-standards |
4 | 0 | 1 | 0 | 2 | 0 | 1 |
pr-review-types |
4 | 0 | 1 | 0 | 0 | 0 | 3 |
pr-review-security-iam |
7 | 0 | 0 | 0 | 0 | 0 | 7 |
| Total | 47 | 6 | 3 | 0 | 6 | 0 | 32 |
| throw createApiError({ | ||
| code: 'forbidden', | ||
| message: `Permission denied. Required: ${requiredPermissions.join(', ')}`, | ||
| message: 'Permission denied. Required: organization admin.', |
There was a problem hiding this comment.
🟠 MAJOR: Hardcoded error message doesn't reflect actual required permission
Issue: The error message says "Required: organization admin" regardless of the actual permission being checked. This middleware is called with different permissions like { project: ['create'] } and { project: ['delete'] }, but users will always see "organization admin" in the error.
Why: This misleads users about what permission they actually need. For example, when creating a project fails, users see "Required: organization admin" when the actual requirement is project:create.
Fix: Derive the error message from the permissions argument:
message: `Permission denied. Required: ${Object.entries(permissions)
.map(([r, p]) => `${r}:${Array.isArray(p) ? p.join(',') : p}`)
.join(', ')}.`,Refs:
| registerAuthzMeta(mw, { | ||
| resource: 'organization', | ||
| permission: 'admin', | ||
| description: 'Requires organization admin role', | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR: x-authz metadata doesn't reflect actual permission being checked
Issue: This hardcodes { resource: 'organization', permission: 'admin' } but the middleware is parameterized — it's called with different permissions like { project: ['create'] } and { project: ['delete'] }. The OpenAPI spec will incorrectly document all routes using this middleware as requiring "organization admin".
Why: OpenAPI consumers (SDKs, docs, tooling) will see incorrect authorization requirements. Routes that actually check project:create permission appear to require organization:admin in the API spec.
Fix: Derive metadata from the permissions argument:
const [[resource, action]] = Object.entries(permissions);
registerAuthzMeta(mw, {
resource,
permission: Array.isArray(action) ? action.join('/') : action,
description: `Requires ${resource} ${Array.isArray(action) ? action.join('/') : action} permission`,
});Refs:
| registerAuthzMeta(mw, { | ||
| description: 'Requires an active user session (cookie-based)', | ||
| }); |
There was a problem hiding this comment.
🟡 Minor: Missing resource/permission fields unlike sibling middleware
Issue: sessionAuth registers metadata with only description, while the closely-related manageApiKeyOrSessionAuth uses the full structured form with resource: 'organization', permission: 'member'. This creates inconsistency in the taxonomy.
Why: Consumers parsing x-authz for structured authorization data will find some session-auth routes missing the machine-readable fields.
Fix:
| registerAuthzMeta(mw, { | |
| description: 'Requires an active user session (cookie-based)', | |
| }); | |
| registerAuthzMeta(mw, { | |
| resource: 'organization', | |
| permission: 'member', | |
| description: 'Requires an active user session (cookie-based)', | |
| }); |
Refs:
| ...routeConfig, | ||
| middleware: [permission, ...toArray(routeConfig.middleware)], | ||
| ...(meta && { 'x-authz': meta }), | ||
| ...(!meta && !('security' in config) && { security: [] }), |
There was a problem hiding this comment.
💭 Consider: Silent fallback to security: [] when no metadata is found
Issue: When a permission middleware doesn't have registered metadata AND no explicit security is provided, this silently sets security: [] (public access). This fail-open behavior could mask misconfigured routes.
Why: If someone creates custom middleware and forgets to call registerAuthzMeta(), the route silently becomes public in the OpenAPI spec with no warning.
Fix: Consider logging a warning in development when this fallback is triggered:
if (!meta && !('security' in config)) {
if (process.env.NODE_ENV === 'development') {
console.warn(`[createProtectedRoute] No authz metadata found for permission middleware. Route will appear as public in OpenAPI.`);
}
}Refs:
| export const inheritedRunApiKeyAuth = () => | ||
| inheritedAuth({ | ||
| description: | ||
| 'Requires a valid API key (Bearer token). Auth is enforced by runApiKeyAuth middleware in createApp.ts.', | ||
| }); |
There was a problem hiding this comment.
🟡 Minor: inheritedRunApiKeyAuth lacks structured metadata unlike inheritedManageTenantAuth
Issue: This helper uses description-only metadata while the sibling inheritedManageTenantAuth above (lines 33-39) includes resource: 'organization', permission: 'member'. This creates asymmetry in the inherited auth markers.
Why: Consumers expecting structured x-authz metadata for programmatic parsing will find run API routes inconsistent with manage routes.
Fix: Consider adding structured metadata if there's an appropriate resource/permission pairing (e.g., resource: 'project', permission: 'use'), or document that description-only is intentional for run API key auth patterns.
Refs:
|
|
||
| const authHeader = c.req.header('Authorization'); | ||
| if (authHeader?.startsWith('Bearer ')) { | ||
| return manageApiKeyAuth()(c as any, next); |
There was a problem hiding this comment.
💭 Consider: as any cast for middleware delegation
Issue: Uses c as any to bypass TypeScript type checking when delegating to manageApiKeyAuth(). This pattern appears twice in this function (lines 197 and 200).
Why: While this works at runtime, it defeats type safety and could hide type incompatibilities between the outer context c and what manageApiKeyAuth expects.
Fix: Consider using a more specific type assertion if the types are compatible, or restructure to avoid the cast. If the types genuinely cannot be unified, a comment explaining why the cast is safe would help future maintainers.
Refs:
| "react": { | ||
| "importNames": ["useContext"], | ||
| "message": "Use the `use()` hook instead." | ||
| }, | ||
| "@hono/zod-openapi": { | ||
| "importNames": ["createRoute"], | ||
| "message": "Use createProtectedRoute from @inkeep/agents-core/middleware instead. If this route intentionally has no permission check, add a biome-ignore comment explaining why." | ||
| } |
There was a problem hiding this comment.
🟡 Minor: Lint rule message could mention noAuth() for intentionally public routes
Issue: The error message guides developers to use createProtectedRoute but doesn't mention that noAuth() is the pattern for intentionally public routes like health checks and OAuth callbacks.
Why: Developers encountering this error for a new public route may not know about noAuth() and might incorrectly add a biome-ignore comment instead.
Fix: Consider updating the message:
| "react": { | |
| "importNames": ["useContext"], | |
| "message": "Use the `use()` hook instead." | |
| }, | |
| "@hono/zod-openapi": { | |
| "importNames": ["createRoute"], | |
| "message": "Use createProtectedRoute from @inkeep/agents-core/middleware instead. If this route intentionally has no permission check, add a biome-ignore comment explaining why." | |
| } | |
| "@hono/zod-openapi": { | |
| "importNames": ["createRoute"], | |
| "message": "Use createProtectedRoute from @inkeep/agents-core/middleware instead. For intentionally public routes, use createProtectedRoute with noAuth() as the permission. If neither applies, add a biome-ignore comment explaining why." | |
| } |
Refs:
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (7) Pending from Prior Review | Risk: High
Delta Review Scope
This is a re-review scoped to changes since commit f04adb3b1860. The delta consists of 25 commits merged from main, including:
- Deletion of
agents-manage-mcppackage (partially addresses prior Critical #1) - Dev-session security hardening
- Various unrelated UI/docs improvements
The PR's own code (middleware, route changes) is unchanged since the last review.
🔴❗ Critical (0 new) ❗🔴
No new critical issues introduced in the delta.
🟠⚠️ Major (0 new) 🟠⚠️
No new major issues introduced in the delta.
🟡 Minor (0 new) 🟡
No new minor issues introduced in the delta.
🕐 Pending Recommendations (7)
The following issues from the prior automated review remain unaddressed:
Critical:
- 🔴
agents-mcp/oAuthInitiateOauthLoginPublic.ts:89— OAuth route path mismatch:agents-mcpstill references deleted/manage/oauth/loginpath - 🔴
createApp.ts:250-251— Dead middleware for deleted OAuth route still registered
Major:
- 🟠
requirePermission.ts:65— Hardcoded error message doesn't reflect actual required permission - 🟠
requirePermission.ts:97-101—x-authzmetadata hardcodesorganization/adminregardless of actual permission - 🟠
packages/agents-core— Missing changeset for new./middlewarepublic export
Minor:
- 🟡
biome.jsonc:62-69— Lint rule message could mentionnoAuth()for intentionally public routes - 🟡
sessionAuth.ts:38-40— Missingresource/permissionfields unlike sibling middleware - 🟡
inherited-auth.ts:47-51—inheritedRunApiKeyAuthlacks structured metadata unlikeinheritedManageTenantAuth
Consider:
- 💭
create-protected-route.ts:39— Silent fallback tosecurity: []when no metadata is found - 💭
manageAuth.ts:197—as anycast for middleware delegation
Delta Changes from Main
The merge from main deleted packages/agents-manage-mcp, which partially resolves Critical #1 (that package no longer exists). However, packages/agents-mcp remains and still contains the broken OAuth path reference at line 89.
🚫 REQUEST CHANGES
Summary: The delta from main does not introduce new issues, but 7 prior review findings remain unaddressed — including 2 Critical issues. The agents-mcp package still references the deleted /manage/oauth/login route (will cause 404 errors), and dead middleware remains in createApp.ts. The requirePermission.ts issues cause incorrect error messages and OpenAPI documentation. These should be addressed before merge.
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-breaking-changes |
1 | 0 | 0 | 0 | 0 | 1 | 0 |
pr-review-devops |
3 | 0 | 0 | 0 | 0 | 2 | 1 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 4 | 0 | 0 | 0 | 0 | 3 | 1 |
Note: All 4 returned findings matched issues already raised in the prior review — routed to Pending Recommendations to avoid duplication.
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
packages/agents-core/src/middleware/index.ts |
Export structure observation | Not an issue — informational observation about correct export structure |
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (5) Pending from Prior Review | Risk: High
Delta Review Scope
This is a re-review scoped to changes since commit 0dbf64e25ef0. The delta consists of 1 commit (51d789b17 address comments) that changes 9 files:
| File | Change | Prior Issue Addressed |
|---|---|---|
AGENTS.md |
Added createProtectedRoute documentation |
✅ Minor — AGENTS.md should document pattern |
biome.jsonc |
Updated lint rule message to mention noAuth() |
✅ Minor — Lint message could mention noAuth() |
createApp.ts |
Removed dead OAuth middleware | ✅ Critical #2 — Dead middleware for deleted route |
middleware/ref.ts |
Removed oauthRefMiddleware export |
✅ Supports Critical #2 fix |
oauth.mdx |
Removed deleted route from operations | Documentation update |
tools.mdx |
Added new OAuth login route | Documentation update |
project-members.mdx |
Removed "requires authz" language | Documentation cleanup |
user-project-memberships.mdx |
Removed "requires authz" language | Documentation cleanup |
The delta addresses 3 prior review findings and introduces no new issues. The documentation and lint rule changes are well-done.
🔴❗ Critical (0 new) ❗🔴
No new critical issues introduced in the delta.
🟠⚠️ Major (0 new) 🟠⚠️
No new major issues introduced in the delta.
🟡 Minor (0 new) 🟡
No new minor issues introduced in the delta.
🕐 Pending Recommendations (5)
The following issues from prior automated reviews remain unaddressed:
Critical:
- 🔴
agents-mcp/oAuthInitiateOauthLoginPublic.ts:89— OAuth route path mismatch:agents-mcppackage still references deleted/manage/oauth/loginpath. Will cause 404 errors for MCP clients using OAuth.- Suggested fix: Run the Speakeasy SDK generation workflow (
.github/workflows/speakeasy-sdk.yml) after this PR merges to regenerate theagents-mcppackage, or mark this as a known breaking change requiring post-merge action.
- Suggested fix: Run the Speakeasy SDK generation workflow (
Major:
- 🟠
requirePermission.ts:65— Hardcoded error message says "Required: organization admin" regardless of actual permission being checked - 🟠
requirePermission.ts:97-101—x-authzmetadata hardcodesorganization/adminregardless of actual permission, causing incorrect OpenAPI documentation - 🟠
packages/agents-core— Missing changeset for new./middlewarepublic export (user-facing API change requires semver bump)
Minor:
- 🟡
sessionAuth.ts:38-40— Missingresource/permissionfields unlike sibling middleware (inconsistency in x-authz metadata)
🚫 REQUEST CHANGES
Summary: The delta commit successfully addresses 3 prior findings (dead middleware removal, AGENTS.md documentation, lint rule message improvement). However, 5 prior issues remain unaddressed:
-
agents-mcppackage has a stale OAuth path — This will break MCP clients post-merge. Either regenerate the package or document as known follow-up. -
requirePermission.tshas hardcoded metadata — Both the error message (line 65) and x-authz metadata (lines 97-101) say "organization admin" regardless of actual permission. This causes incorrect error messages and incorrect OpenAPI docs. -
Missing changeset — The new
@inkeep/agents-core/middlewareexport is a user-facing API change.
These issues have been flagged across 3 automated reviews. The requirePermission.ts fixes would be straightforward (derive message/metadata from the permissions argument).
Discarded (0)
No findings discarded in this delta review.
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-docs |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta was clean — reviewers confirmed prior fixes were adequate. All pending items are from prior review runs.
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (4) Pending from Prior Review | Risk: High
Delta Review Scope
This is the 5th automated re-review, scoped to changes since commit 1f388c87776d. The delta consists of 1 commit (74acec900 changeset) adding a single file:
| File | Change | Prior Issue Status |
|---|---|---|
.changeset/spicy-ravens-see.md |
Added changeset for @inkeep/agents-core, @inkeep/agents-api, @inkeep/agents-work-apps, @inkeep/agents-manage-ui |
✅ Addresses prior Major issue about missing changeset for new ./middleware export |
The delta introduces no new issues. The changeset correctly covers @inkeep/agents-core (which has the new ./middleware export) with a patch version bump.
🔴❗ Critical (0 new) ❗🔴
No new critical issues introduced in the delta.
🟠⚠️ Major (0 new) 🟠⚠️
No new major issues introduced in the delta.
🟡 Minor (0 new) 🟡
No new minor issues introduced in the delta.
🕐 Pending Recommendations (4)
The following issues from prior automated reviews remain unaddressed:
Critical:
- 🔴
agents-mcp/oAuthInitiateOauthLoginPublic.ts:89— OAuth route path mismatch:agents-mcppackage still references deleted/manage/oauth/loginpath (line 89:pathToFunc("/manage/oauth/login")). Will cause 404 errors for MCP clients using OAuth.- Suggested fix: The
agents-mcppackage is auto-generated by Speakeasy (.github/workflows/speakeasy-sdk.yml). Either:- Run the SDK regeneration workflow after this PR merges, OR
- Acknowledge this as a known breaking change requiring post-merge action
- Suggested fix: The
Major:
- 🟠
requirePermission.ts:65— Hardcoded error message says "Required: organization admin" regardless of actual permission being checked (e.g.,project:create,project:delete) - 🟠
requirePermission.ts:97-101—x-authzmetadata hardcodes{ resource: 'organization', permission: 'admin' }regardless of actual permission, causing incorrect OpenAPI documentation for routes that check different permissions
Minor:
- 🟡
sessionAuth.ts:38-40— Missingresource/permissionfields unlike sibling middleware (inconsistency in x-authz metadata structure)
🚫 REQUEST CHANGES
Summary: The delta successfully adds the missing changeset for the new @inkeep/agents-core/middleware export — this addresses one of the prior Major issues. However, 4 prior findings remain unaddressed across 5 automated review runs:
-
agents-mcppackage has a stale OAuth path — This auto-generated package will cause 404 errors for MCP OAuth clients post-merge. The path at line 89 still references/manage/oauth/loginwhich was deleted in this PR. -
requirePermission.tshardcoded metadata — Both the error message (line 65) and x-authz OpenAPI metadata (lines 97-101) say "organization admin" regardless of actual permission. This creates incorrect error messages for users and incorrect OpenAPI documentation for SDK/tooling consumers. The fix is straightforward: derive the message and metadata from thepermissionsargument.
The requirePermission.ts fixes have been suggested with code snippets in the inline comments. The agents-mcp issue can be addressed post-merge via the Speakeasy SDK regeneration workflow.
Discarded (0)
No findings discarded in this delta review.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| (none dispatched) | — | — | — | — | — | — | — |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta consisted of a single changeset file — no domain reviewers were needed. All pending items are from prior review runs.
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (4) Pending from Prior Review | Risk: High
Delta Review Scope
This is the 6th automated re-review, scoped to changes since commit 74acec90094e. The delta consists of 7 commits (18 files):
| Source | Change | Assessment |
|---|---|---|
PR commit f4f4038bc |
Removed test case from devSession.test.ts |
✅ Appropriate — test was checking 404 when auth: null, but the route is now guarded by if (auth) block (line 61 of createApp.ts), making the test redundant |
| Main merge (6 commits) | GitHub MCP reactions, docs warnings, UI fixes, docker-compose | ✅ Unrelated to this PR's scope |
The delta introduces no new issues. The test removal is a valid cleanup.
🔴❗ Critical (0 new) ❗🔴
No new critical issues introduced in the delta.
🟠⚠️ Major (0 new) 🟠⚠️
No new major issues introduced in the delta.
🟡 Minor (0 new) 🟡
No new minor issues introduced in the delta.
🕐 Pending Recommendations (4)
The following issues from prior automated reviews remain unaddressed across 6 review runs:
Critical:
- 🔴
agents-mcp/oAuthInitiateOauthLoginPublic.ts:89— OAuth route path mismatch:agents-mcppackage still references deleted/manage/oauth/loginpath. The Speakeasy-generated SDK will cause 404 errors for MCP clients using OAuth.- Suggested fix: The
agents-mcppackage is auto-generated by Speakeasy (.github/workflows/speakeasy-sdk.yml). Either:- Run the SDK regeneration workflow after this PR merges, OR
- Acknowledge this as a known breaking change requiring post-merge action
- Suggested fix: The
Major:
- 🟠
requirePermission.ts:65— Hardcoded error message says "Required: organization admin" regardless of actual permission being checked (e.g.,project:create,project:delete) - 🟠
requirePermission.ts:97-101—x-authzmetadata hardcodes{ resource: 'organization', permission: 'admin' }regardless of actual permission, causing incorrect OpenAPI documentation for routes that check different permissions
Minor:
- 🟡
sessionAuth.ts:38-40— Missingresource/permissionfields unlike sibling middleware (inconsistency in x-authz metadata structure)
🚫 REQUEST CHANGES
Summary: The delta from this synchronize event is clean — the test removal is appropriate and the main branch merge commits are unrelated. However, 4 prior review findings remain unaddressed across 6 automated reviews:
-
agents-mcpOAuth path mismatch — This Speakeasy-generated package still references the deleted/manage/oauth/loginroute. MCP clients will get 404 errors post-merge. The fix requires regenerating the SDK via the Speakeasy workflow. -
requirePermission.tshardcoded metadata — Both the error message (line 65) andx-authzOpenAPI metadata (lines 97-101) say "organization admin" regardless of actual permission. This creates:- Incorrect error messages for users (e.g., "Required: organization admin" when the actual requirement is
project:create) - Incorrect OpenAPI documentation for SDK/tooling consumers
- Incorrect error messages for users (e.g., "Required: organization admin" when the actual requirement is
The requirePermission.ts fixes are straightforward — derive the message and metadata from the permissions argument (see inline comments for suggested implementation).
Discarded (0)
No findings discarded in this delta review.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| (none dispatched) | — | — | — | — | — | — | — |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta consisted of test cleanup and unrelated main merge commits — no domain reviewers were needed. All pending items are from prior review runs.
|
Claude encountered an error —— View job PR Review in Progress
|
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Inkeep Agent Framework Docs' |
1 participant
Summary
Introduces a consistent, machine-readable authorization metadata system (
x-authz) for all API routes via a new OpenAPI extension. Every route now self-documents its authorization requirements directly in the OpenAPI spec.What changed
New middleware infrastructure (
packages/agents-core/src/middleware/)authz-meta.tsAuthzMetatype +registerAuthzMeta()/getAuthzMeta()— WeakMap-based metadata registrycreate-protected-route.tscreateProtectedRoute()— wrapscreateRouteto attach both middleware andx-authzmetadata in one declarationinherited-auth.tsinheritedAuth()+ specialized variants (inheritedManageTenantAuth,inheritedRunApiKeyAuth,inheritedWorkAppsAuth) for routes where auth is handled by parent middlewareno-auth.tsnoAuth()— pass-through middleware that registers no metadata (route appears withsecurity: [])Standardized resource/permission taxonomy
resourcepermissionprojectviewprojecteditprojectuseorganizationadminorganizationmember240 of 247 routes now carry
x-authzmetadata. The remaining 7 are public (security: []): health checks, readiness, OpenAPI spec, OAuth callbacks, and webhooks.Taxonomy consolidation
tenant/member→organization/memberorganization/authenticated→organization/memberworkApp/orgAdmin→organization/adminBug fix: dev-session route ordering
Moved the
/api/auth/dev-sessionroute registration before the catch-all/api/auth/*handler increateApp.ts. Hono uses first-match-wins routing, so the previous ordering caused the catch-all to always intercept dev-session requests. Also guarded both routes with anauthnull check.OpenAPI spec examples
1. Project-scoped with SpiceDB permission (resource + permission + description)
2. Organization-level role check
3. Description-only (complex/inherited auth patterns)
4. Public routes (no x-authz, security: [])
Files changed (91 files)
packages/agents-core/src/middleware/(5 new files)createProtectedRoute()with appropriate permission middlewaremanageAuth,sessionAuth,evalsAuth,requirePermission,projectAccess,tenantAccessto registerx-authzmetadataorganization/admintaxonomyx-authzmetadata across all routesModelFactory.test.ts(addedmockprovider),devSession.test.ts(route ordering fix)