-
Notifications
You must be signed in to change notification settings - Fork 48
feat(code-reviewer) add council review type data model #4500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c0e1937
1e01fe6
6f1504c
c7cf98a
34a03ae
84ffb2c
534b244
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| import { TRPCError } from '@trpc/server'; | ||
|
|
||
| const mockGetOrganizationById = jest.fn(); | ||
| const mockGetMostRecentSeatPurchase = jest.fn(); | ||
| const mockIsLocalCodeReviewDevelopmentEnabled = jest.fn(); | ||
|
|
||
| jest.mock('@/lib/organizations/organizations', () => ({ | ||
| getOrganizationById: (...args: unknown[]) => mockGetOrganizationById(...args), | ||
| })); | ||
| jest.mock('@/lib/organizations/organization-seats', () => ({ | ||
| getMostRecentSeatPurchase: (...args: unknown[]) => mockGetMostRecentSeatPurchase(...args), | ||
| })); | ||
| jest.mock('@/lib/config.server', () => ({ | ||
| isLocalCodeReviewDevelopmentEnabled: () => mockIsLocalCodeReviewDevelopmentEnabled(), | ||
| })); | ||
|
|
||
| // Real classifyOrganizationEntitlement is used (not mocked) so the tests exercise the | ||
| // canonical active-entitlement logic layered on top of the plan tier. | ||
| import { | ||
| isCouncilEntitledForOrganization, | ||
| isCouncilEntitledForOwner, | ||
| assertCouncilCreationAllowed, | ||
| } from './council-entitlement'; | ||
|
|
||
| const DAY_MS = 24 * 60 * 60 * 1000; | ||
| const hardExpiredTrialEnd = new Date(Date.now() - 5 * DAY_MS).toISOString(); | ||
|
|
||
| function enterpriseOrg(overrides: Record<string, unknown> = {}) { | ||
| return { | ||
| id: 'org-1', | ||
| plan: 'enterprise', | ||
| created_at: new Date(Date.now() - 60 * DAY_MS).toISOString(), | ||
| free_trial_end_at: hardExpiredTrialEnd, | ||
| require_seats: true, | ||
| settings: {}, | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| mockIsLocalCodeReviewDevelopmentEnabled.mockReturnValue(false); | ||
| mockGetMostRecentSeatPurchase.mockResolvedValue(null); | ||
| }); | ||
|
|
||
| describe('isCouncilEntitledForOrganization', () => { | ||
| it('returns true for an enterprise org with an active paid subscription', async () => { | ||
| mockGetOrganizationById.mockResolvedValue(enterpriseOrg()); | ||
| mockGetMostRecentSeatPurchase.mockResolvedValue({ subscription_status: 'active' }); | ||
|
|
||
| await expect(isCouncilEntitledForOrganization('org-1')).resolves.toBe(true); | ||
| }); | ||
|
|
||
| it('returns false for a lapsed enterprise org (plan still enterprise, subscription ended, trial hard-expired)', async () => { | ||
| // Regression: plan is NOT downgraded on cancellation, so plan alone must not grant council. | ||
| mockGetOrganizationById.mockResolvedValue(enterpriseOrg()); | ||
| mockGetMostRecentSeatPurchase.mockResolvedValue({ subscription_status: 'ended' }); | ||
|
|
||
| await expect(isCouncilEntitledForOrganization('org-1')).resolves.toBe(false); | ||
| }); | ||
|
|
||
| it('returns false for a non-enterprise plan without checking subscription status', async () => { | ||
| mockGetOrganizationById.mockResolvedValue(enterpriseOrg({ plan: 'teams' })); | ||
|
|
||
| await expect(isCouncilEntitledForOrganization('org-1')).resolves.toBe(false); | ||
| expect(mockGetMostRecentSeatPurchase).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('returns false when the organization does not exist or id is missing', async () => { | ||
| mockGetOrganizationById.mockResolvedValue(null); | ||
| await expect(isCouncilEntitledForOrganization('org-1')).resolves.toBe(false); | ||
| await expect(isCouncilEntitledForOrganization(null)).resolves.toBe(false); | ||
| expect(mockGetOrganizationById).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); | ||
|
|
||
| describe('isCouncilEntitledForOwner', () => { | ||
| it('is never entitled for personal (user) owners', async () => { | ||
| await expect(isCouncilEntitledForOwner({ type: 'user', id: 'u1', userId: 'u1' })).resolves.toBe( | ||
| false | ||
| ); | ||
| expect(mockGetOrganizationById).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('assertCouncilCreationAllowed', () => { | ||
| const orgOwner = { type: 'org', id: 'org-1', userId: 'u1' } as const; | ||
|
|
||
| it('is a no-op for non-council reviews', async () => { | ||
| await expect( | ||
| assertCouncilCreationAllowed({ owner: orgOwner, reviewType: 'standard' }) | ||
| ).resolves.toBeUndefined(); | ||
| expect(mockGetOrganizationById).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('throws FORBIDDEN when a council review is requested without entitlement', async () => { | ||
| mockGetOrganizationById.mockResolvedValue(enterpriseOrg()); | ||
| mockGetMostRecentSeatPurchase.mockResolvedValue({ subscription_status: 'ended' }); | ||
|
|
||
| await expect( | ||
| assertCouncilCreationAllowed({ owner: orgOwner, reviewType: 'council' }) | ||
| ).rejects.toThrow(TRPCError); | ||
| }); | ||
|
|
||
| it('allows a council review for an entitled enterprise org', async () => { | ||
| mockGetOrganizationById.mockResolvedValue(enterpriseOrg()); | ||
| mockGetMostRecentSeatPurchase.mockResolvedValue({ subscription_status: 'active' }); | ||
|
|
||
| await expect( | ||
| assertCouncilCreationAllowed({ owner: orgOwner, reviewType: 'council' }) | ||
| ).resolves.toBeUndefined(); | ||
| }); | ||
|
|
||
| it('bypasses entitlement in local development', async () => { | ||
| mockIsLocalCodeReviewDevelopmentEnabled.mockReturnValue(true); | ||
|
|
||
| await expect( | ||
| assertCouncilCreationAllowed({ owner: orgOwner, reviewType: 'council' }) | ||
| ).resolves.toBeUndefined(); | ||
| expect(mockGetOrganizationById).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| import 'server-only'; | ||
|
|
||
| import { TRPCError } from '@trpc/server'; | ||
| import { getOrganizationById } from '@/lib/organizations/organizations'; | ||
| import { getMostRecentSeatPurchase } from '@/lib/organizations/organization-seats'; | ||
| import { classifyOrganizationEntitlement } from '@/lib/organizations/trial-utils'; | ||
| import { isLocalCodeReviewDevelopmentEnabled } from '@/lib/config.server'; | ||
| import type { CodeReviewType } from '@kilocode/db/schema-types'; | ||
| import type { Owner } from './schemas'; | ||
|
|
||
| /** | ||
| * Which plan tier grants council. Council is enterprise-only. | ||
| * | ||
| * NOTE: this is only the tier label. `organizations.plan` stays `'enterprise'` even | ||
| * after an enterprise subscription ends (it is not downgraded on cancellation), so plan | ||
| * alone is NOT proof of active entitlement. Active-subscription/trial status is layered | ||
| * on separately in `isCouncilEntitledForOrganization`. Staged rollout (which entitled | ||
| * users see council yet) is a separate client-side PostHog concern handled elsewhere. | ||
| */ | ||
| export function isCouncilEntitledPlan(plan: string | null | undefined): boolean { | ||
| return plan === 'enterprise'; | ||
| } | ||
|
|
||
| /** | ||
| * Council reviews are an enterprise-only paid feature. An organization is entitled only | ||
| * when it is on the enterprise tier AND currently has active entitlement (active paid | ||
| * subscription or non-expired trial). This mirrors the canonical entitlement check used | ||
| * by `requireActiveSubscriptionOrTrial`, so a lapsed enterprise org (plan still | ||
| * `'enterprise'`, seat purchase `ended`, trial hard-expired) cannot create paid council | ||
| * reviews. Personal owners are never entitled. | ||
| */ | ||
| export async function isCouncilEntitledForOrganization( | ||
| organizationId: string | null | undefined | ||
| ): Promise<boolean> { | ||
| if (!organizationId) return false; | ||
| const organization = await getOrganizationById(organizationId); | ||
| if (!organization || !isCouncilEntitledPlan(organization.plan)) return false; | ||
|
|
||
| const latestPurchase = await getMostRecentSeatPurchase(organizationId); | ||
| const classification = classifyOrganizationEntitlement({ | ||
| organization, | ||
| latestSeatPurchaseStatus: latestPurchase?.subscription_status ?? null, | ||
| now: new Date(), | ||
| }); | ||
| return classification.hasEntitlement; | ||
| } | ||
|
|
||
| export async function isCouncilEntitledForOwner(owner: Owner): Promise<boolean> { | ||
| return owner.type === 'org' ? isCouncilEntitledForOrganization(owner.id) : false; | ||
| } | ||
|
|
||
| /** | ||
| * Single enforcement point for council entitlement, invoked at the review-creation | ||
| * boundary. Any path that persists a `council` review (manual or automated) passes | ||
| * through here, so a non-entitled owner can never create one. Local dev bypasses so | ||
| * the feature can be exercised without an enterprise org. | ||
| * | ||
| * Throws FORBIDDEN when a council review is requested without entitlement. | ||
| */ | ||
| export async function assertCouncilCreationAllowed(params: { | ||
| owner: Owner; | ||
| reviewType?: CodeReviewType; | ||
| }): Promise<void> { | ||
| if (params.reviewType !== 'council') return; | ||
| if (isLocalCodeReviewDevelopmentEnabled()) return; | ||
| if (await isCouncilEntitledForOwner(params.owner)) return; | ||
| throw new TRPCError({ | ||
| code: 'FORBIDDEN', | ||
| message: 'Council reviews require an Enterprise plan.', | ||
| }); | ||
| } | ||
|
|
||
| // NOTE: a config-save guard (`assertCouncilConfigAllowed`) will be added alongside the | ||
| // council config-save path (PR #5) that actually accepts a `council` config, so it lands | ||
| // with its first caller rather than as an unreferenced export. The creation-boundary | ||
| // guard above already prevents a non-entitled owner from *running* a council review. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| ALTER TABLE "cloud_agent_code_reviews" ADD COLUMN "review_type" text DEFAULT 'standard' NOT NULL;--> statement-breakpoint | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. **[WARNING]: Migration can block the live reviews table Each Reply with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not addressing this one, by design. A few reasons:
If we want lock-safe migrations, that's a separate change to the migration tooling/convention that should apply to all migrations, tracked on its own. |
||
| ALTER TABLE "cloud_agent_code_reviews" ADD COLUMN "trigger_source" text; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WARNING: New reviews discard their known trigger provenance
Every current production caller omits
triggerSource, including manual jobs and the GitHub, GitLab, and Bitbucket webhook paths, so this fallback writesNULLfor all new reviews. That makes known manual/webhook origins indistinguishable from legacy rows and leaves the new field unpopulated; have those creation paths passmanualorwebhookexplicitly.Reply with
@kilocode-bot fix itto have Kilo Code address this issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in
34a03ae. All five code-review creation paths now pass an explicittriggerSource, so new rows record their origin instead of writingNULL:lib/code-reviews/manual-code-review-jobs.ts→manuallib/integrations/platforms/github/webhook-handlers/pull-request-handler.ts→webhooklib/integrations/platforms/gitlab/webhook-handlers/merge-request-handler.ts→webhooklib/integrations/platforms/bitbucket/manual-code-review-trigger.ts→manualapp/api/webhooks/bitbucket/[integrationId]/route.ts→webhookThe
?? nullfallback incodeReviewInsertValuesstays only as a safety net for any future/unknown caller. Regression coverage added: the GitHub and GitLab handler tests assertcreateCodeReviewis called withtriggerSource: 'webhook', and the Bitbucket webhook route test asserts the persisted row hastrigger_source: 'webhook'. The latest review reflects this file as 0 issues.