diff --git a/services/libs/data-access-layer/src/project-catalog/types.ts b/services/libs/data-access-layer/src/project-catalog/types.ts index 9729ab94a7..3700e5e6b9 100644 --- a/services/libs/data-access-layer/src/project-catalog/types.ts +++ b/services/libs/data-access-layer/src/project-catalog/types.ts @@ -64,7 +64,7 @@ export interface IRequestHints { segmentId?: string } -export type RequestQuestionKind = 'did_you_mean' | 'lf_conflict' | 'pick_project' +export type RequestQuestionKind = 'did_you_mean' | 'lf_conflict' | 'lf_confirm' | 'pick_project' export interface IRequestQuestionOption { id: string diff --git a/services/libs/project-onboarding/src/botCopy.test.ts b/services/libs/project-onboarding/src/botCopy.test.ts index a973fb569e..c61f7945ed 100644 --- a/services/libs/project-onboarding/src/botCopy.test.ts +++ b/services/libs/project-onboarding/src/botCopy.test.ts @@ -29,6 +29,11 @@ describe('botCopy', () => { whatsNeeded: 'Connect GitHub to the project with this repo', action: 'connect_github', }) + expect(describeReviewCase('not_in_pcc', 'Acme')).toEqual({ + insightsState: 'Not in the foundation project list', + whatsNeeded: 'Check Acme with PCC, then create it in CDP, add this repo and mark as done', + action: 'manual', + }) expect(describeReviewCase('legacy_connection', 'TiKV')).toEqual({ insightsState: 'Uses the old GitHub connection', whatsNeeded: 'Add the repo by hand in CDP, then mark as done', diff --git a/services/libs/project-onboarding/src/botCopy.ts b/services/libs/project-onboarding/src/botCopy.ts index 5b2a7d6c24..89b937eb8c 100644 --- a/services/libs/project-onboarding/src/botCopy.ts +++ b/services/libs/project-onboarding/src/botCopy.ts @@ -39,6 +39,7 @@ export type ReviewCase = | 'connect_github' | 'legacy_connection' | 'project_not_set_up' + | 'not_in_pcc' | 'owner_only_match' | 'handed_off' | 'unknown' @@ -83,6 +84,12 @@ const REVIEW_CASE_COPY: Record = { whatsNeeded: (project) => `Create ${project} in CDP, add this repo, then mark as done`, action: 'manual', }, + not_in_pcc: { + insightsState: 'Not in the foundation project list', + whatsNeeded: (project) => + `Check ${project} with PCC, then create it in CDP, add this repo and mark as done`, + action: 'manual', + }, owner_only_match: { insightsState: 'Only the GitHub owner matches LF projects', whatsNeeded: () => diff --git a/services/libs/project-onboarding/src/botThreadReply.test.ts b/services/libs/project-onboarding/src/botThreadReply.test.ts index 40a6d0bcb8..7c775a7da1 100644 --- a/services/libs/project-onboarding/src/botThreadReply.test.ts +++ b/services/libs/project-onboarding/src/botThreadReply.test.ts @@ -158,6 +158,18 @@ describe('botThreadReply', () => { reviewReason: "Acme is an LF project, but it isn't set up in LFX Insights yet", }), ).toContain("I've asked the CDP team to create it and add the repo; they'll let you know here.") + expect( + buildBotReviewReply({ + ...project, + classificationKind: 'lf_not_in_pcc', + proposedAction: 'human_review', + matchedSegmentId: null, + reviewReason: + "Acme is described as an LF project, but it isn't in the foundation project list yet", + }), + ).toBe( + `👀 ${REPO_LABEL}: Acme is described as an LF project, but it isn't in the foundation project list yet. I've asked the CDP team to check it with PCC and add it; they'll let you know here.`, + ) expect( buildBotReviewReply({ ...project, diff --git a/services/libs/project-onboarding/src/botThreadReply.ts b/services/libs/project-onboarding/src/botThreadReply.ts index 25cde72356..18ca7f6881 100644 --- a/services/libs/project-onboarding/src/botThreadReply.ts +++ b/services/libs/project-onboarding/src/botThreadReply.ts @@ -78,6 +78,8 @@ function reviewNextStep(project: BotReviewProject): string { return "I've asked the CDP team to add it by hand; they'll mark it done here once it's in." case 'project_not_set_up': return "I've asked the CDP team to create it and add the repo; they'll let you know here." + case 'not_in_pcc': + return "I've asked the CDP team to check it with PCC and add it; they'll let you know here." case 'owner_only_match': case 'handed_off': case 'unknown': diff --git a/services/libs/project-onboarding/src/classifyCatalogRow.test.ts b/services/libs/project-onboarding/src/classifyCatalogRow.test.ts index 00770dffba..b705d3d55b 100644 --- a/services/libs/project-onboarding/src/classifyCatalogRow.test.ts +++ b/services/libs/project-onboarding/src/classifyCatalogRow.test.ts @@ -156,10 +156,51 @@ describe('classifyCatalogRow', () => { expect(result.trace.parsed).toMatchObject({ declaredLf: true }) expect(result.classification).toMatchObject({ classificationKind: 'lf_not_in_pcc', + proposedAction: 'human_review', + action: 'review', + reviewReason: + "Acme is described as an LF project, but it isn't in the foundation project list yet", + requestQuestion: null, + }) + }) + + it('asks a bot requester whether an unknown project is LF before evaluating it', async () => { + const bot = { provenance: 'slack-bot' as const, requestedBy: 'U1' } + + const asked = await classifyCatalogRow(row(bot), deps()) + expect(asked.classification).toMatchObject({ + classificationKind: 'ambiguous', proposedAction: null, - action: 'skip', + action: 'ask', + requestQuestion: { kind: 'lf_confirm' }, + }) + + const discovered = await classifyCatalogRow(row(), deps()) + expect(discovered.classification).toMatchObject({ + classificationKind: 'non_lf_new_project', + proposedAction: 'create_segment', + action: 'evaluate', requestQuestion: null, }) + + const confirmedLf = await classifyCatalogRow( + row({ ...bot, requestHints: { declaredLf: true } }), + deps(), + ) + expect(confirmedLf.classification).toMatchObject({ + classificationKind: 'lf_not_in_pcc', + proposedAction: 'human_review', + action: 'review', + }) + + const notLf = await classifyCatalogRow( + row({ ...bot, requestHints: { treatAsNew: true } }), + deps(), + ) + expect(notLf.classification).toMatchObject({ + classificationKind: 'non_lf_new_project', + action: 'evaluate', + }) }) it('stores the project name the LLM extracted from the request text', async () => { @@ -343,10 +384,26 @@ describe('toCatalogClassification', () => { }) }) + it('asks for human review of an LF project missing from the foundation project list', () => { + expect( + toCatalogClassification({ kind: 'lf_not_in_pcc', projectName: 'Acme' }, { autoApply: true }), + ).toEqual({ + classificationKind: 'lf_not_in_pcc', + proposedAction: 'human_review', + pccProjectId: null, + matchedProjectName: null, + matchedSegmentId: null, + action: 'review', + requestQuestion: null, + skipReason: null, + reviewReason: + "Acme is described as an LF project, but it isn't in the foundation project list yet", + }) + }) + it.each([ { kind: 'already_tracked', repoUrls: ['https://github.com/acme/one'] }, { kind: 'non_github_source', nonGithubRepoUrls: ['https://gitlab.com/acme/one'] }, - { kind: 'lf_not_in_pcc', projectName: 'Acme' }, { kind: 'ambiguous', reason: 'x', candidates: [] }, ])('proposes no action for $kind', (resolution) => { expect(toCatalogClassification(resolution, { autoApply: true })).toMatchObject({ diff --git a/services/libs/project-onboarding/src/classifyCatalogRow.ts b/services/libs/project-onboarding/src/classifyCatalogRow.ts index 8aeb3c0d65..bf89eecee9 100644 --- a/services/libs/project-onboarding/src/classifyCatalogRow.ts +++ b/services/libs/project-onboarding/src/classifyCatalogRow.ts @@ -135,10 +135,16 @@ function toProposal(resolution: OnboardingResolution): IProposal { pccProjectId: null, ...NO_MATCH, } + case 'lf_not_in_pcc': + return { + classificationKind: resolution.kind, + proposedAction: 'human_review', + pccProjectId: null, + ...NO_MATCH, + } case 'already_tracked': case 'ambiguous': case 'non_github_source': - case 'lf_not_in_pcc': return { classificationKind: resolution.kind, proposedAction: null, @@ -169,11 +175,13 @@ export async function classifyCatalogRow( deps: CatalogRowClassificationDeps, ): Promise { const trace = createClassificationTrace() + const canAsk = isSlackBotProvenance(row.provenance ?? null) && !!row.requestedBy const resolution = await resolveParsedRequest( parsedRequestSource(row, deps), deps.lookups, trace, row.requestHints ?? {}, + { canAsk }, ) return { @@ -181,10 +189,7 @@ export async function classifyCatalogRow( trace, classification: { projectName: resolvedProjectName(row, trace), - ...toCatalogClassification(resolution, { - autoApply: deps.autoApply, - canAsk: isSlackBotProvenance(row.provenance ?? null) && !!row.requestedBy, - }), + ...toCatalogClassification(resolution, { autoApply: deps.autoApply, canAsk }), }, } } diff --git a/services/libs/project-onboarding/src/classifyRequest.ts b/services/libs/project-onboarding/src/classifyRequest.ts index da3168587e..6fd9ec78ed 100644 --- a/services/libs/project-onboarding/src/classifyRequest.ts +++ b/services/libs/project-onboarding/src/classifyRequest.ts @@ -15,6 +15,7 @@ import { } from './requestParser' import { IOnboardingRequestLookups, + IResolveOptions, OnboardingResolution, resolveOnboardingRequest, } from './requestResolver' @@ -47,6 +48,7 @@ export async function resolveParsedRequest( lookups: IOnboardingRequestLookups, trace: IClassificationTrace, hints: IRequestHints = {}, + options: IResolveOptions = {}, ): Promise { const parsed = await parse() if (parsed.ok === false) { @@ -55,7 +57,7 @@ export async function resolveParsedRequest( } trace.parsed = parsed.request - return resolveOnboardingRequest(parsed.request, traceLookups(lookups, trace), hints) + return resolveOnboardingRequest(parsed.request, traceLookups(lookups, trace), hints, options) } export async function resolveWithTrace( diff --git a/services/libs/project-onboarding/src/requestQuestion.test.ts b/services/libs/project-onboarding/src/requestQuestion.test.ts index 10af348649..404e409468 100644 --- a/services/libs/project-onboarding/src/requestQuestion.test.ts +++ b/services/libs/project-onboarding/src/requestQuestion.test.ts @@ -45,6 +45,15 @@ const conflictQuestion = { { id: 'not_sure', label: 'Not sure, ask the CDP team', handOff: true }, ], } +const confirmQuestion = { + kind: 'lf_confirm' as const, + text: "I couldn't find Acme in the foundation project list. Is it a Linux Foundation project?", + options: [ + { id: 'lf', label: "Yes, it's an LF project", hints: { declaredLf: true } }, + { id: 'not_lf', label: "No, it's not LF", hints: { treatAsNew: true } }, + { id: 'not_sure', label: 'Not sure, ask the CDP team', handOff: true }, + ], +} const row = { id: '8f1c2a44-5b7e-4d2f-9c1a-3e6f7a8b9c0d', repoUrl: 'https://github.com/acme/widget', @@ -125,6 +134,9 @@ describe('answer values', () => { expect(handOffReason(named, conflictQuestion)).toBe( "the requester isn't sure whether Acme Widgets is the LF project Acme Labs", ) + expect(handOffReason(named, confirmQuestion)).toBe( + "the requester isn't sure whether Acme Widgets is an LF project", + ) }) it('finds options and describes an answered question', () => { diff --git a/services/libs/project-onboarding/src/requestQuestion.ts b/services/libs/project-onboarding/src/requestQuestion.ts index 597a76c695..30be556285 100644 --- a/services/libs/project-onboarding/src/requestQuestion.ts +++ b/services/libs/project-onboarding/src/requestQuestion.ts @@ -108,6 +108,8 @@ export function handOffReason( return suggested ? `the requester isn't sure whether ${name ?? 'this repo'} is the LF project ${suggested}` : `the requester isn't sure whether ${name ?? 'this repo'} is an LF project` + case 'lf_confirm': + return `the requester isn't sure whether ${name ?? 'this project'} is an LF project` } } diff --git a/services/libs/project-onboarding/src/requestResolver.test.ts b/services/libs/project-onboarding/src/requestResolver.test.ts index 59f7fa8a06..df4d7e2598 100644 --- a/services/libs/project-onboarding/src/requestResolver.test.ts +++ b/services/libs/project-onboarding/src/requestResolver.test.ts @@ -62,6 +62,51 @@ describe('resolveOnboardingRequest', () => { expect(result).toEqual({ kind: 'non_lf_new_project', projectName: 'Acme' }) }) + it('asks the requester whether an unknown project is LF when it can', async () => { + const result = await resolveOnboardingRequest(request(), lookups([]), {}, { canAsk: true }) + + expect(result).toEqual({ + kind: 'needs_input', + candidates: [], + fallbackReason: + "Acme isn't in the foundation project list and the request doesn't say whether it is an LF project", + question: { + kind: 'lf_confirm', + text: "I couldn't find Acme in the foundation project list. Is it a Linux Foundation project?", + options: [ + { id: 'lf', label: "Yes, it's an LF project", hints: { declaredLf: true } }, + { id: 'not_lf', label: "No, it's not LF", hints: { treatAsNew: true } }, + { id: 'not_sure', label: 'Not sure, ask the CDP team', handOff: true }, + ], + }, + }) + }) + + it('does not ask about the LF status when the request already says it or has no name', async () => { + const ask = { canAsk: true } + + expect( + await resolveOnboardingRequest(request({ declaredLf: false }), lookups([]), {}, ask), + ).toEqual({ kind: 'non_lf_new_project', projectName: 'Acme' }) + expect( + await resolveOnboardingRequest(request({ declaredLf: true }), lookups([]), {}, ask), + ).toEqual({ kind: 'lf_not_in_pcc', projectName: 'Acme' }) + expect( + await resolveOnboardingRequest(request({ projectName: null }), lookups([]), {}, ask), + ).toEqual({ kind: 'non_lf_new_project', projectName: null }) + }) + + it('follows the LF answer on the next run', async () => { + const ask = { canAsk: true } + + expect( + await resolveOnboardingRequest(request(), lookups([]), { declaredLf: true }, ask), + ).toEqual({ kind: 'lf_not_in_pcc', projectName: 'Acme' }) + expect( + await resolveOnboardingRequest(request(), lookups([]), { treatAsNew: true }, ask), + ).toEqual({ kind: 'non_lf_new_project', projectName: 'Acme' }) + }) + it('alerts when the request is declared LF but PCC has no match', async () => { const result = await resolveOnboardingRequest(request({ declaredLf: true }), lookups([])) diff --git a/services/libs/project-onboarding/src/requestResolver.ts b/services/libs/project-onboarding/src/requestResolver.ts index 269cf58e88..d6de202520 100644 --- a/services/libs/project-onboarding/src/requestResolver.ts +++ b/services/libs/project-onboarding/src/requestResolver.ts @@ -45,6 +45,10 @@ export interface IOnboardingRequestLookups { findExclusivelyLfOwners?: (owners: string[]) => Promise> } +export interface IResolveOptions { + canAsk?: boolean +} + export type CdpIntegrationAction = 'create_integration' | 'update_integration' | 'human_review' export type PccMatchLevel = 'exact' | 'strong' | 'weak' | 'none' @@ -271,6 +275,21 @@ function lfConflict( ) } +function lfConfirm(projectName: string): OnboardingResolution { + return needsInput( + { + kind: 'lf_confirm', + text: `I couldn't find ${projectName} in the foundation project list. Is it a Linux Foundation project?`, + options: [ + { id: 'lf', label: "Yes, it's an LF project", hints: { declaredLf: true } }, + { id: 'not_lf', label: "No, it's not LF", hints: { treatAsNew: true } }, + NOT_SURE_OPTION, + ], + }, + `${projectName} isn't in the foundation project list and the request doesn't say whether it is an LF project`, + ) +} + function repositoriesLabel(count: number | undefined): string { if (count === undefined) { return '' @@ -426,6 +445,7 @@ async function resolveWithoutPccMatch( request: IParsedOnboardingRequest, lookups: IOnboardingRequestLookups, pccConfigured: boolean, + canAsk: boolean, ): Promise { const byOwner = await resolveByOwnerHeuristic(request, lookups) if (byOwner) { @@ -444,6 +464,10 @@ async function resolveWithoutPccMatch( : ambiguous("I couldn't tell which project this repository belongs to") } + if (request.declaredLf === null && request.projectName && canAsk) { + return lfConfirm(request.projectName) + } + return { kind: 'non_lf_new_project', projectName: request.projectName } } @@ -451,6 +475,7 @@ export async function resolveOnboardingRequest( original: IParsedOnboardingRequest, lookups: IOnboardingRequestLookups, hints: IRequestHints = {}, + { canAsk = false }: IResolveOptions = {}, ): Promise { const request = withHints(original, hints) if (hasNoRepositories(request)) { @@ -488,7 +513,7 @@ export async function resolveOnboardingRequest( const { projectName } = request const pccConfigured = lookups.findPccCandidates !== undefined if (!projectName || !lookups.findPccCandidates) { - return resolveWithoutPccMatch(request, lookups, pccConfigured) + return resolveWithoutPccMatch(request, lookups, pccConfigured, canAsk) } const assessment = assessPccCandidates(projectName, await lookups.findPccCandidates(projectName)) @@ -501,5 +526,5 @@ export async function resolveOnboardingRequest( return didYouMean(projectName, assessment.best) } - return resolveWithoutPccMatch(request, lookups, pccConfigured) + return resolveWithoutPccMatch(request, lookups, pccConfigured, canAsk) } diff --git a/services/libs/project-onboarding/src/reviewCard.test.ts b/services/libs/project-onboarding/src/reviewCard.test.ts index 4a4a7ce20b..7e0ac60211 100644 --- a/services/libs/project-onboarding/src/reviewCard.test.ts +++ b/services/libs/project-onboarding/src/reviewCard.test.ts @@ -171,6 +171,14 @@ describe('resolveReviewCase', () => { matchedSegmentId: null, }), ).toBe('project_not_set_up') + expect( + resolveReviewCase({ + ...row, + classificationKind: 'lf_not_in_pcc', + proposedAction: 'human_review', + matchedSegmentId: null, + }), + ).toBe('not_in_pcc') expect( resolveReviewCase({ ...row, proposedAction: 'human_review', matchedSegmentId: null }), ).toBe('owner_only_match') diff --git a/services/libs/project-onboarding/src/reviewCard.ts b/services/libs/project-onboarding/src/reviewCard.ts index efa3b5662a..e966c458f1 100644 --- a/services/libs/project-onboarding/src/reviewCard.ts +++ b/services/libs/project-onboarding/src/reviewCard.ts @@ -66,6 +66,9 @@ export function resolveReviewCase(row: ReviewCaseRow): ReviewCase { if (row.classificationKind === 'lf_not_in_cdp') { return 'project_not_set_up' } + if (row.classificationKind === 'lf_not_in_pcc') { + return 'not_in_pcc' + } if (row.classificationKind === 'ambiguous') { return 'handed_off' } @@ -100,6 +103,15 @@ export function toReviewRepoContext(metrics: IPublicRepoMetrics): IReviewRepoCon } } +function reviewCaseProjectName( + reviewCase: ReviewCase, + row: Pick, +): string | null { + return reviewCase === 'new_project' || reviewCase === 'not_in_pcc' + ? row.projectName + : row.matchedProjectName +} + export function toReviewRequestCard( row: ReviewCardRow, { cdpUrl, repoContext }: IReviewCardExtras, @@ -114,10 +126,7 @@ export function toReviewRequestCard( requestedName: row.projectName, matchedName: row.matchedProjectName, mismatch: namesDiffer(row.projectName, row.matchedProjectName), - ...describeReviewCase( - reviewCase, - reviewCase === 'new_project' ? row.projectName : row.matchedProjectName, - ), + ...describeReviewCase(reviewCase, reviewCaseProjectName(reviewCase, row)), evaluationNote: reviewCase === 'new_project' ? row.evaluationReason : null, autoApplyHold: isAutoApplyHold(row), cdpUrl, diff --git a/services/libs/project-onboarding/src/routing.test.ts b/services/libs/project-onboarding/src/routing.test.ts index 481bca74d0..72539a7df7 100644 --- a/services/libs/project-onboarding/src/routing.test.ts +++ b/services/libs/project-onboarding/src/routing.test.ts @@ -69,6 +69,10 @@ describe('routeResolution', () => { { kind: 'lf_owner_in_cdp', owners: ['acme', 'acme-labs'] }, "the GitHub owners acme, acme-labs only have LF projects in LFX Insights, so I couldn't tell which one this repo belongs to", ], + [ + { kind: 'lf_not_in_pcc', projectName: 'Acme' }, + "Acme is described as an LF project, but it isn't in the foundation project list yet", + ], ])('routes %j to review regardless of auto apply', (resolution, reviewReason) => { expect(routeResolution(resolution, { autoApply: false })).toMatchObject({ action: 'review', @@ -91,10 +95,6 @@ describe('routeResolution', () => { { kind: 'non_github_source', nonGithubRepoUrls: ['https://gitlab.com/acme/one'] }, 'only GitHub repositories can be added', ], - [ - { kind: 'lf_not_in_pcc', projectName: 'Acme' }, - "it is described as an LF project, but I couldn't find it in the foundation project list", - ], [ { kind: 'ambiguous', reason: "that's a question I can't decide", candidates: [] }, "that's a question I can't decide", diff --git a/services/libs/project-onboarding/src/routing.ts b/services/libs/project-onboarding/src/routing.ts index 46a71ab015..4fe30b9a4a 100644 --- a/services/libs/project-onboarding/src/routing.ts +++ b/services/libs/project-onboarding/src/routing.ts @@ -22,8 +22,6 @@ export type RoutedResolution = Pick< export const ALREADY_TRACKED_SKIP_REASON = 'it is already in LFX Insights' export const NON_GITHUB_SOURCE_SKIP_REASON = 'only GitHub repositories can be added' -export const LF_NOT_IN_PCC_SKIP_REASON = - "it is described as an LF project, but I couldn't find it in the foundation project list" function route(action: ProjectCatalogRoutedAction): RoutedResolution { return { action, skipReason: null, reviewReason: null, requestQuestion: null } @@ -41,6 +39,10 @@ function ask(question: IRequestQuestion): RoutedResolution { return { action: 'ask', skipReason: null, reviewReason: null, requestQuestion: question } } +function lfNotInPccReviewReason(projectName: string): string { + return `${projectName} is described as an LF project, but it isn't in the foundation project list yet` +} + function describeOwners(owners: string[]): string { return owners.length === 1 ? `the GitHub owner ${owners[0]} only has` @@ -82,7 +84,7 @@ export function routeResolution( case 'non_github_source': return skip(NON_GITHUB_SOURCE_SKIP_REASON) case 'lf_not_in_pcc': - return skip(LF_NOT_IN_PCC_SKIP_REASON) + return review(lfNotInPccReviewReason(resolution.projectName)) case 'ambiguous': return skip(resolution.reason) }