Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export const OrganizationSettings = ({
releaseDetectionMethod: string
releaseDetectionKey: string
isActive: number
excludedUsers: string
timezone: string
language: string
}
Expand All @@ -60,7 +59,6 @@ export const OrganizationSettings = ({
releaseDetectionMethod: organizationSetting?.releaseDetectionMethod,
releaseDetectionKey: organizationSetting?.releaseDetectionKey,
isActive: organizationSetting?.isActive ? '1' : undefined,
excludedUsers: organizationSetting?.excludedUsers,
timezone: organizationSetting?.timezone ?? DEFAULT_TIMEZONE,
language: organizationSetting?.language,
},
Expand Down Expand Up @@ -173,21 +171,6 @@ export const OrganizationSettings = ({
<div className="text-destructive">{fields.isActive.errors}</div>
</fieldset>

<fieldset className="space-y-1">
<Label htmlFor={fields.excludedUsers.id}>
Excluded Users (comma separated)
</Label>
<Input
{...getInputProps(fields.excludedUsers, { type: 'text' })}
placeholder="e.g. my-org-bot, some-other-bot"
/>
<p className="text-muted-foreground text-sm">
Copilot is excluded by default. Add additional usernames to exclude
from cycle time calculations.
</p>
<div className="text-destructive">{fields.excludedUsers.errors}</div>
</fieldset>

{form.errors && (
<Alert variant="destructive">
<AlertTitle>System Error</AlertTitle>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export const updateOrganizationSetting = async (
| 'releaseDetectionMethod'
| 'releaseDetectionKey'
| 'isActive'
| 'excludedUsers'
| 'timezone'
| 'language'
>,
Expand Down
1 change: 0 additions & 1 deletion app/routes/$orgSlug/settings/_index/+schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ export const organizationSettingsSchema = z.object({
.literal('on')
.optional()
.transform((val) => (val === 'on' ? 1 : 0)),
excludedUsers: z.string().max(2000).default(''),
timezone: z.string().refine((v) => VALID_TIMEZONES.has(v), {
message: 'Invalid timezone',
}),
Expand Down
2 changes: 0 additions & 2 deletions app/routes/$orgSlug/settings/_index/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ export const action = async ({ request, context }: Route.ActionArgs) => {
releaseDetectionMethod,
releaseDetectionKey,
isActive,
excludedUsers,
timezone,
language,
} = submission.value
Expand All @@ -55,7 +54,6 @@ export const action = async ({ request, context }: Route.ActionArgs) => {
releaseDetectionMethod,
releaseDetectionKey,
isActive,
excludedUsers,
timezone,
language,
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
upsertPullRequestReview,
upsertPullRequestReviewers,
} from '~/batch/db/mutations'
import { getBotLogins } from '~/batch/db/queries'
import { createFetcher } from '~/batch/github/fetcher'
import type { ShapedGitHubPullRequest } from '~/batch/github/model'
import { buildPullRequests } from '~/batch/github/pullrequest'
Expand Down Expand Up @@ -161,15 +162,18 @@ export const action = async ({
timelineItems,
})

// 4. Get organization settings for build config
const settings = await getOrganizationSettings(organization.id)
// 4. Get organization settings and bot logins for build config
const [settings, botLogins] = await Promise.all([
getOrganizationSettings(organization.id),
getBotLogins(organization.id),
])

// 5. Build pull request data (analyze)
const result = await buildPullRequests(
{
organizationId: organization.id,
repositoryId,
excludedUsers: settings?.excludedUsers ?? '',
botLogins,
releaseDetectionMethod: settings?.releaseDetectionMethod ?? 'branch',
releaseDetectionKey: settings?.releaseDetectionKey ?? '',
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export const getOrganizationSettings = async (
const tenantDb = getTenantDb(organizationId)
return await tenantDb
.selectFrom('organizationSettings')
.select(['releaseDetectionMethod', 'releaseDetectionKey', 'excludedUsers'])
.select(['releaseDetectionMethod', 'releaseDetectionKey'])
.executeTakeFirst()
}

Expand Down
4 changes: 2 additions & 2 deletions app/services/jobs/analyze-worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ interface WorkerInput {
repositoryId: string
releaseDetectionMethod: string
releaseDetectionKey: string
excludedUsers: string
botLogins: string[]
filterPrNumbers?: number[]
}

Expand Down Expand Up @@ -47,7 +47,7 @@ const result: Awaited<ReturnType<typeof buildPullRequests>> =
repositoryId: input.repositoryId,
releaseDetectionMethod: input.releaseDetectionMethod,
releaseDetectionKey: input.releaseDetectionKey,
excludedUsers: input.excludedUsers,
botLogins: new Set(input.botLogins),
},
await store.loader.pullrequests(),
store.loader,
Expand Down
1 change: 1 addition & 0 deletions app/services/jobs/crawl.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export const crawlJob = defineJob({
}
return {
organizationSetting: org.organizationSetting,
botLogins: org.botLogins,
repositories: org.repositories,
exportSetting: org.exportSetting,
}
Expand Down
1 change: 1 addition & 0 deletions app/services/jobs/recalculate.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export const recalculateJob = defineJob({
}
return {
organizationSetting: org.organizationSetting,
botLogins: org.botLogins,
repositories: org.repositories,
exportSetting: org.exportSetting,
}
Expand Down
2 changes: 1 addition & 1 deletion app/services/jobs/run-in-worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ interface AnalyzeWorkerInput {
repositoryId: string
releaseDetectionMethod: string
releaseDetectionKey: string
excludedUsers: string
botLogins: string[]
filterPrNumbers?: number[]
}

Expand Down
5 changes: 3 additions & 2 deletions app/services/jobs/shared-steps.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ interface AnalyzeResult {
interface OrganizationData {
organizationSetting: Pick<
Selectable<TenantDB.OrganizationSettings>,
'releaseDetectionMethod' | 'releaseDetectionKey' | 'excludedUsers'
'releaseDetectionMethod' | 'releaseDetectionKey'
>
botLogins: string[]
repositories: Selectable<TenantDB.Repositories>[]
exportSetting?: Selectable<TenantDB.ExportSettings> | null
}
Expand Down Expand Up @@ -127,7 +128,7 @@ export async function analyzeAndFinalizeSteps(
repo.releaseDetectionMethod ?? orgSetting.releaseDetectionMethod,
releaseDetectionKey:
repo.releaseDetectionKey ?? orgSetting.releaseDetectionKey,
excludedUsers: orgSetting.excludedUsers,
botLogins: organization.botLogins,
filterPrNumbers: prNumbers ? [...prNumbers] : undefined,
},
{
Expand Down
1 change: 0 additions & 1 deletion app/services/tenant-type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ export interface Integrations {

export interface OrganizationSettings {
createdAt: Generated<string>;
excludedUsers: Generated<string>;
id: string;
isActive: Generated<0 | 1>;
language: Generated<"en" | "ja">;
Expand Down
13 changes: 13 additions & 0 deletions batch/db/mutations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,18 @@ import type { OrganizationId } from '~/app/types/organization'
import type { AnalyzedReview, AnalyzedReviewer } from '../github/types'
import { logger } from '../helper/logger'

/** GitHub API で [bot] 接尾辞なしで記録される well-known bot (lowercase) */
const KNOWN_BOTS = new Set([
'copilot',
'copilot-pull-request-reviewer',
'copilot-swe-agent',
'github-actions',
'dependabot',
'renovate',
'coderabbitai',
'devin-ai-integration',
])

export function upsertPullRequest(
organizationId: OrganizationId,
data: Insertable<TenantDB.PullRequests>,
Expand Down Expand Up @@ -240,6 +252,7 @@ export async function upsertCompanyGithubUsers(
uniqueLogins.map((login) => ({
login,
displayName: login,
type: KNOWN_BOTS.has(login) ? 'Bot' : null,
isActive: 0,
updatedAt: now,
})),
Expand Down
102 changes: 60 additions & 42 deletions batch/db/queries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,56 +14,74 @@ export const getPullRequestReport = async (organizationId: OrganizationId) => {

async function getTenantData(organizationId: OrganizationId) {
const tenantDb = getTenantDb(organizationId)
const [organizationSetting, integration, repositories, exportSetting] =
await Promise.all([
tenantDb
.selectFrom('organizationSettings')
.select([
'releaseDetectionMethod',
'releaseDetectionKey',
'isActive',
'excludedUsers',
])
.executeTakeFirst(),
tenantDb
.selectFrom('integrations')
.select(['id', 'method', 'provider', 'privateToken'])
.executeTakeFirst(),
tenantDb
.selectFrom('repositories')
.select([
'id',
'repo',
'owner',
'integrationId',
'provider',
'releaseDetectionKey',
'releaseDetectionMethod',
'teamId',
'updatedAt',
'createdAt',
])
.execute(),
tenantDb
.selectFrom('exportSettings')
.select([
'id',
'sheetId',
'clientEmail',
'privateKey',
'updatedAt',
'createdAt',
])
.executeTakeFirst(),
])
const [
organizationSetting,
integration,
repositories,
exportSetting,
botLoginRows,
] = await Promise.all([
tenantDb
.selectFrom('organizationSettings')
.select(['releaseDetectionMethod', 'releaseDetectionKey', 'isActive'])
.executeTakeFirst(),
tenantDb
.selectFrom('integrations')
.select(['id', 'method', 'provider', 'privateToken'])
.executeTakeFirst(),
tenantDb
.selectFrom('repositories')
.select([
'id',
'repo',
'owner',
'integrationId',
'provider',
'releaseDetectionKey',
'releaseDetectionMethod',
'teamId',
'updatedAt',
'createdAt',
])
.execute(),
tenantDb
.selectFrom('exportSettings')
.select([
'id',
'sheetId',
'clientEmail',
'privateKey',
'updatedAt',
'createdAt',
])
.executeTakeFirst(),
tenantDb
.selectFrom('companyGithubUsers')
.select('login')
.where('type', '=', 'Bot')
.execute(),
])
return {
organizationSetting: organizationSetting ?? null,
integration: integration ?? null,
repositories,
exportSetting: exportSetting ?? null,
botLogins: botLoginRows.map((r) => r.login),
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

botLogins を小文字正規化しないと Bot フィルタが漏れます。

下流の照合は user.toLowerCase() 前提です。ここで生値を返すと、DBに Dependabot などが入っていた場合に一致しません。
getTenantData / getBotLogins の両方で lower-case 正規化してください。

💡 修正案
   return {
     organizationSetting: organizationSetting ?? null,
     integration: integration ?? null,
     repositories,
     exportSetting: exportSetting ?? null,
-    botLogins: botLoginRows.map((r) => r.login),
+    botLogins: botLoginRows.map((r) => r.login.toLowerCase()),
   }
 }
@@
 export async function getBotLogins(
   organizationId: OrganizationId,
 ): Promise<Set<string>> {
@@
-  return new Set(rows.map((r) => r.login))
+  return new Set(rows.map((r) => r.login.toLowerCase()))
 }

Also applies to: 73-83

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@batch/db/queries.ts` around lines 69 - 70, botLogins is returned without
lower-case normalization causing downstream matches (which call
user.toLowerCase()) to miss entries like "Dependabot"; update the mapping in
getTenantData (the botLogins: botLoginRows.map(...) expression) to normalize
with .toLowerCase(), and also ensure getBotLogins returns lower-cased logins too
so both entry points consistently yield lower-case bot login strings.

}

export async function getBotLogins(
organizationId: OrganizationId,
): Promise<Set<string>> {
const tenantDb = getTenantDb(organizationId)
const rows = await tenantDb
.selectFrom('companyGithubUsers')
.select('login')
.where('type', '=', 'Bot')
.execute()
return new Set(rows.map((r) => r.login))
}

export const listAllOrganizations = async () => {
const orgs = await db
.selectFrom('organizations')
Expand Down
2 changes: 1 addition & 1 deletion batch/github/__tests__/buildPullRequests-filter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ const config = {
repositoryId: 'repo-1',
releaseDetectionMethod: 'branch',
releaseDetectionKey: 'main',
excludedUsers: '',
botLogins: new Set<string>(),
}

// --- テスト ---
Expand Down
20 changes: 5 additions & 15 deletions batch/github/pullrequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,6 @@ import type {
PullRequestLoaders,
} from './types'

// デフォルトで除外するユーザー (GitHub API で [bot] 接尾辞なしで記録されるbot)
const DEFAULT_EXCLUDED_USERS = ['Copilot']

/** PR に関連するアーティファクトの型 */
interface PrArtifacts {
commits: ShapedGitHubCommit[]
Expand All @@ -55,7 +52,7 @@ interface BuildConfig {
repositoryId: string
releaseDetectionMethod: string
releaseDetectionKey: string
excludedUsers: string
botLogins: Set<string>
}

const nullOrDate = (dateStr?: Date | string | null) => {
Expand All @@ -82,21 +79,21 @@ async function loadPrArtifacts(
function filterActors(
artifacts: PrArtifacts,
pr: ShapedGitHubPullRequest,
excludedUsers: string[],
botLogins: Set<string>,
): PrArtifacts {
return {
commits: artifacts.commits,
reviews: artifacts.reviews.filter(
(r) =>
!r.isBot &&
r.user !== pr.author &&
!excludedUsers.includes(r.user ?? ''),
!botLogins.has((r.user ?? '').toLowerCase()),
),
discussions: artifacts.discussions.filter(
(d) =>
!d.isBot &&
d.user !== pr.author &&
!excludedUsers.includes(d.user ?? ''),
!botLogins.has((d.user ?? '').toLowerCase()),
),
}
}
Expand Down Expand Up @@ -188,13 +185,6 @@ export const buildPullRequests = async (
loaders: PullRequestLoaders,
filterPrNumbers?: Set<number>,
) => {
// カンマ区切りの除外ユーザーリストをパース
const customExcludedUsers = config.excludedUsers
.split(',')
.map((u) => u.trim())
.filter((u) => u.length > 0)
const excludedUsers = [...DEFAULT_EXCLUDED_USERS, ...customExcludedUsers]

// リリース日ルックアップを事前構築
// 注: filterPrNumbers に関係なく全 PR から構築する(リリースPR自体がフィルタ外でも必要)
let branchReleaseLookup: Map<number, string> | null = null
Expand Down Expand Up @@ -234,7 +224,7 @@ export const buildPullRequests = async (
const rawArtifacts = await loadPrArtifacts(pr, loaders)

// 2. アクター除外フィルタ(純粋関数)
const artifacts = filterActors(rawArtifacts, pr, excludedUsers)
const artifacts = filterActors(rawArtifacts, pr, config.botLogins)

// 3. レビューレスポンス解析
reviewResponses.push(
Expand Down
Loading