-
-
Notifications
You must be signed in to change notification settings - Fork 90
feat(mcp): gittensory_check_before_start (pre-start duplicate/solvability check) #621
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
Changes from all commits
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 |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { createMcpHandler } from "agents/mcp"; | ||
|
Check warning on line 1 in src/mcp/server.ts
|
||
| import type { Context } from "hono"; | ||
| import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; | ||
| import type { RequestHandlerExtra } from "@modelcontextprotocol/sdk/shared/protocol.js"; | ||
|
|
@@ -69,6 +69,7 @@ | |
| buildLinkedIssueValidation, | ||
| buildLocalDiffPreflightResult, | ||
| buildPreflightResult, | ||
| buildPreStartCheck, | ||
| buildQueueHealth, | ||
| buildRegistryChangeReport, | ||
| buildRoleContext, | ||
|
|
@@ -125,6 +126,14 @@ | |
| .optional(), | ||
| }; | ||
|
|
||
| const checkBeforeStartShape = { | ||
| owner: z.string().min(1), | ||
| repo: z.string().min(1), | ||
| issueNumber: z.number().int().positive().optional(), | ||
| title: z.string().min(1).max(PREFLIGHT_LIMITS.titleChars).optional(), | ||
| plannedPaths: z.array(z.string().max(PREFLIGHT_LIMITS.changedFileChars)).max(PREFLIGHT_LIMITS.changedFiles).optional(), | ||
| }; | ||
|
|
||
| const preflightShape = { | ||
| repoFullName: z.string().min(3).max(PREFLIGHT_LIMITS.repoFullNameChars), | ||
| contributorLogin: z.string().min(1).max(PREFLIGHT_LIMITS.contributorLoginChars).optional(), | ||
|
|
@@ -393,6 +402,18 @@ | |
| report: z.unknown().optional(), | ||
| }; | ||
|
|
||
| const checkBeforeStartOutputSchema = { | ||
| status: z.string().optional(), | ||
| repoFullName: z.string().optional(), | ||
| found: z.boolean().optional(), | ||
| claimStatus: z.string().optional(), | ||
| duplicateClusterRisk: z.string().optional(), | ||
| recommendation: z.string().optional(), | ||
| reasons: z.unknown().optional(), | ||
| blockers: z.unknown().optional(), | ||
| report: z.unknown().optional(), | ||
| }; | ||
|
|
||
| export async function handleMcpRequest(c: AppContext): Promise<Response> { | ||
| if (c.req.method === "OPTIONS") return new Response(null, { status: 204 }); | ||
| const identity = await authenticateMcpRequest(c); | ||
|
|
@@ -594,6 +615,17 @@ | |
| async (input) => this.toolResult(await this.validateLinkedIssue(input)), | ||
| ); | ||
|
|
||
|
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. Double-check the access control logic to ensure that only authorized users can perform pre-start checks. 🤖 Prompt for AI agents |
||
| server.registerTool( | ||
| "gittensory_check_before_start", | ||
| { | ||
| description: | ||
| "Before any code is written, check whether an issue is already claimed or solved, whether a duplicate cluster is forming, and whether it is a valid target. Returns a go/raise/avoid recommendation with public-safe reasons from cached metadata. No GitHub writes.", | ||
| inputSchema: checkBeforeStartShape, | ||
| outputSchema: checkBeforeStartOutputSchema, | ||
| }, | ||
| async (input) => this.toolResult(await this.checkBeforeStart(input)), | ||
| ); | ||
|
|
||
| server.registerTool( | ||
| "gittensory_preflight_local_diff", | ||
| { | ||
|
|
@@ -974,6 +1006,41 @@ | |
| }; | ||
| } | ||
|
|
||
| private async checkBeforeStart(input: { owner: string; repo: string; issueNumber?: number | undefined; title?: string | undefined; plannedPaths?: string[] | undefined }): Promise<ToolPayload> { | ||
| const fullName = `${input.owner}/${input.repo}`; | ||
| if (!(await this.canAccessRepo(fullName))) { | ||
| return { | ||
| summary: `Forbidden: session cannot access pre-start checks for ${fullName}.`, | ||
| data: { status: "forbidden", repoFullName: fullName }, | ||
| }; | ||
| } | ||
| const [repo, issues, pullRequests, recentMergedPullRequests] = await Promise.all([ | ||
| getRepository(this.env, fullName), | ||
| listIssueSignalSample(this.env, fullName), | ||
| listOpenPullRequests(this.env, fullName), | ||
| listRecentMergedPullRequests(this.env, fullName), | ||
| ]); | ||
| const report = buildPreStartCheck(repo, issues, pullRequests, recentMergedPullRequests, fullName, { | ||
| issueNumber: input.issueNumber, | ||
| title: input.title, | ||
| plannedPaths: input.plannedPaths, | ||
| }); | ||
| return { | ||
| summary: `Gittensory pre-start check for ${fullName}: ${report.recommendation.toUpperCase()}.`, | ||
| data: { | ||
| status: "ok", | ||
| repoFullName: fullName, | ||
| found: report.found, | ||
| claimStatus: report.claimStatus, | ||
| duplicateClusterRisk: report.duplicateClusterRisk, | ||
| recommendation: report.recommendation, | ||
| reasons: report.reasons, | ||
| blockers: report.blockers, | ||
| report: report as unknown as Record<string, unknown>, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| private async canAccessRepo(fullName: string): Promise<boolean> { | ||
| if (this.identity.kind !== "session") return true; | ||
| const [scope, repo] = await Promise.all([this.loadSessionAccessScope(), getRepository(this.env, fullName)]); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||
| import type { | ||||||
|
Check warning on line 1 in src/signals/engine.ts
|
||||||
| AdvisoryFinding, | ||||||
| BountyRecord, | ||||||
| CheckSummaryRecord, | ||||||
|
|
@@ -2919,6 +2919,190 @@ | |||||
| }; | ||||||
| } | ||||||
|
|
||||||
| export type PreStartCheckTarget = { | ||||||
| issueNumber?: number | undefined; | ||||||
| title?: string | undefined; | ||||||
| plannedPaths?: string[] | undefined; | ||||||
| }; | ||||||
|
|
||||||
| export type PreStartCheckClaimStatus = "unclaimed" | "claimed" | "solved" | "unknown"; | ||||||
| export type PreStartCheckRecommendation = "go" | "raise" | "avoid"; | ||||||
| export type DuplicateClusterRisk = "none" | "low" | "medium" | "high"; | ||||||
|
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. The title matching logic may not handle very short or very long titles correctly. Ensure that edge cases are tested. 🤖 Prompt for AI agents |
||||||
|
|
||||||
| export type PreStartCheckReport = { | ||||||
| repoFullName: string; | ||||||
| generatedAt: string; | ||||||
| lane: LaneAdvice; | ||||||
| target: { | ||||||
| requested: { issueNumber?: number | undefined; title?: string | undefined; plannedPaths?: string[] | undefined }; | ||||||
| matchedBy: "issue_number" | "title" | "planned_paths" | "none"; | ||||||
| resolvedIssueNumber?: number | undefined; | ||||||
| resolvedIssueTitle?: string | undefined; | ||||||
| }; | ||||||
| found: boolean; | ||||||
| claimStatus: PreStartCheckClaimStatus; | ||||||
| lifecycle?: IssueDiscoveryLifecycleState | undefined; | ||||||
| issueQualityStatus?: "ready" | "needs_proof" | "hold" | "do_not_use" | undefined; | ||||||
| duplicateClusterRisk: DuplicateClusterRisk; | ||||||
| recommendation: PreStartCheckRecommendation; | ||||||
| reasons: string[]; | ||||||
| blockers: string[]; | ||||||
| summary: string; | ||||||
|
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. 🛠️ Refactor suggestion · Simplify Title Matching Logic Consider refactoring the title matching logic to improve readability and maintainability. 🤖 Prompt for AI agents |
||||||
| }; | ||||||
|
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. The tokenization and matching logic for issue titles could be optimized to handle large datasets more efficiently. 🤖 Prompt for AI agents |
||||||
|
|
||||||
| const DUPLICATE_RISK_RANK: Record<DuplicateClusterRisk, number> = { none: 0, low: 1, medium: 2, high: 3 }; | ||||||
| // Minimum Jaccard token overlap for a supplied title to resolve to a cached open issue. | ||||||
| const TITLE_MATCH_MIN_JACCARD = 0.5; | ||||||
| // Cap the title-matching scan so it stays cheap on repos with very large open-issue counts | ||||||
| // (matches the bound used by the issue lifecycle report). | ||||||
| const TITLE_MATCH_MAX_ISSUES = 300; | ||||||
|
|
||||||
| /** | ||||||
| * Pre-start duplicate/solvability check. Answers, before any branch exists, whether an issue is | ||||||
| * already claimed/solved, whether a duplicate cluster is forming, and whether it is a valid target — | ||||||
| * composing the existing collision, issue-quality, and lifecycle reports. Public-safe by construction: | ||||||
| * every reason/blocker is routed through {@link sanitizePublicComment}; no reward/score/trust language. | ||||||
| */ | ||||||
| export function buildPreStartCheck( | ||||||
| repo: RepositoryRecord | null, | ||||||
| issues: IssueRecord[], | ||||||
| pullRequests: PullRequestRecord[], | ||||||
| recentMergedPullRequests: RecentMergedPullRequestRecord[], | ||||||
| fullName: string, | ||||||
| target: PreStartCheckTarget, | ||||||
| ): PreStartCheckReport { | ||||||
| const lane = buildLaneAdvice(repo, fullName); | ||||||
| const collisions = buildCollisionReport(fullName, issues, pullRequests, recentMergedPullRequests); | ||||||
|
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. 🛠️ Refactor suggestion · Extract Magic Numbers The magic number 0.5 used for title matching should be extracted to a named constant for better readability and maintainability.
Suggested change
🤖 Prompt for AI agents |
||||||
| const quality = buildIssueQualityReport(repo, issues, pullRequests, fullName, [], collisions, recentMergedPullRequests); | ||||||
| const lifecycle = buildIssueDiscoveryLifecycleReport(repo, issues, pullRequests, fullName, recentMergedPullRequests); | ||||||
| const openIssues = issues.filter((issue) => issue.state === "open"); | ||||||
|
|
||||||
| let resolvedIssue: IssueRecord | undefined; | ||||||
| let matchedBy: PreStartCheckReport["target"]["matchedBy"] = "none"; | ||||||
| if (typeof target.issueNumber === "number") { | ||||||
| resolvedIssue = openIssues.find((issue) => issue.number === target.issueNumber); | ||||||
| if (resolvedIssue) matchedBy = "issue_number"; | ||||||
| } else if (target.title) { | ||||||
| const wanted = new Set(tokenize(target.title)); | ||||||
| let best: { number: number; score: number } | undefined; | ||||||
| // An all-stopword/short title has no meaningful tokens to match against. Bound the scan to a | ||||||
| // fixed number of open issues so title matching stays cheap on repos with very large queues. | ||||||
| if (wanted.size > 0) { | ||||||
| for (const issue of openIssues.slice(0, TITLE_MATCH_MAX_ISSUES)) { | ||||||
| const have = new Set(tokenize(issue.title)); | ||||||
| const shared = [...wanted].filter((term) => have.has(term)).length; | ||||||
| const score = shared / new Set([...wanted, ...have]).size; | ||||||
| if (!best || score > best.score) best = { number: issue.number, score }; | ||||||
| } | ||||||
| } | ||||||
| if (best && best.score >= TITLE_MATCH_MIN_JACCARD) { | ||||||
| resolvedIssue = openIssues.find((issue) => issue.number === best!.number); | ||||||
| matchedBy = "title"; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| const resolvedNumber = resolvedIssue?.number; | ||||||
| const qualityEntry = resolvedNumber == null ? undefined : quality.issues.find((entry) => entry.number === resolvedNumber); | ||||||
| const lifecycleEntry = resolvedNumber == null ? undefined : lifecycle.states.find((entry) => entry.number === resolvedNumber); | ||||||
|
|
||||||
| const plannedPaths = (target.plannedPaths ?? []).map((path) => path.toLowerCase()); | ||||||
| if (matchedBy === "none" && plannedPaths.length > 0) matchedBy = "planned_paths"; | ||||||
|
|
||||||
| const issueClusters = | ||||||
| resolvedNumber == null ? [] : collisions.clusters.filter((cluster) => cluster.items.some((item) => item.type === "issue" && item.number === resolvedNumber)); | ||||||
| // Open PR records carry no file metadata in the cache, so planned-path overlap is evaluated against recently merged work. | ||||||
| const pathOverlapMergedPullRequests = | ||||||
| plannedPaths.length === 0 ? [] : recentMergedPullRequests.filter((pr) => pr.changedFiles.some((file) => plannedPaths.includes(file.toLowerCase()))); | ||||||
|
|
||||||
| let duplicateClusterRisk: DuplicateClusterRisk = "none"; | ||||||
| const riskCandidates: DuplicateClusterRisk[] = [...issueClusters.map((cluster) => cluster.risk), ...(pathOverlapMergedPullRequests.length > 0 ? (["medium"] as const) : [])]; | ||||||
| for (const risk of riskCandidates) { | ||||||
| if (DUPLICATE_RISK_RANK[risk] > DUPLICATE_RISK_RANK[duplicateClusterRisk]) duplicateClusterRisk = risk; | ||||||
|
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. Verify that the planned paths logic correctly identifies overlaps with recently merged work. 🤖 Prompt for AI agents |
||||||
| } | ||||||
|
|
||||||
| const found = resolvedNumber != null || matchedBy === "planned_paths"; | ||||||
|
|
||||||
| let claimStatus: PreStartCheckClaimStatus = "unknown"; | ||||||
| if (resolvedNumber != null) { | ||||||
| const linkageStatus = qualityEntry?.linkage?.status; | ||||||
| const state = lifecycleEntry?.state; | ||||||
| if (state === "solved" || state === "valid_solved" || linkageStatus === "validated") claimStatus = "solved"; | ||||||
| else if (linkageStatus === "plausible") claimStatus = "claimed"; | ||||||
| else claimStatus = "unclaimed"; | ||||||
| } else if (matchedBy === "planned_paths") { | ||||||
| claimStatus = pathOverlapMergedPullRequests.length > 0 ? "claimed" : "unclaimed"; | ||||||
| } | ||||||
|
|
||||||
| const reasons: string[] = []; | ||||||
| const blockers: string[] = []; | ||||||
|
|
||||||
| if (!found) { | ||||||
| blockers.push( | ||||||
| target.issueNumber != null | ||||||
| ? `Issue #${target.issueNumber} was not found in cached open-issue metadata; confirm it exists and is open before starting.` | ||||||
| : "No matching open issue or overlapping work was found in cached metadata; confirm the target before starting.", | ||||||
| ); | ||||||
| } | ||||||
| if (claimStatus === "solved") blockers.push("This issue already has merged or validated solving work; new work would likely duplicate it."); | ||||||
| if (claimStatus === "claimed") { | ||||||
| blockers.push( | ||||||
| resolvedNumber != null | ||||||
| ? "Open PR work already references this issue; coordinate or pick a different target to avoid a collision." | ||||||
|
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. 🛠️ Refactor suggestion · Optimize Duplicate Cluster Risk Calculation The duplicate cluster risk calculation can be optimized for better performance. 🤖 Prompt for AI agents |
||||||
| : "Recently merged work already touched one or more of these paths; confirm this is not a duplicate before starting.", | ||||||
| ); | ||||||
| } | ||||||
|
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. 🛠️ Refactor suggestion · Use Enums for Statuses The status strings like 'go', 'raise', 'avoid' should be replaced with enum values for type safety and better code readability.
Suggested change
🤖 Prompt for AI agents |
||||||
| if (duplicateClusterRisk === "high") blockers.push("A high-risk duplicate or overlapping work cluster already exists for this target."); | ||||||
| if (lifecycleEntry?.state === "duplicate") blockers.push("This issue is classified as a duplicate in cached metadata."); | ||||||
| if (lifecycleEntry?.state === "invalid") blockers.push("This issue is classified as invalid in cached metadata."); | ||||||
| // Issue quality is "uncertain" when the cached report places it anywhere short of ready (needs_proof/hold), but not at the do_not_use floor (handled as an avoid blocker). | ||||||
| const qualityUncertain = qualityEntry != null && qualityEntry.status !== "ready" && qualityEntry.status !== "do_not_use"; | ||||||
| if (duplicateClusterRisk === "medium") reasons.push("A possible duplicate or overlapping work cluster exists; confirm it before starting."); | ||||||
| if (qualityUncertain) reasons.push("Issue quality is not yet a confident go; verify the scope and proof before committing effort."); | ||||||
| if (lane.lane === "direct_pr") reasons.push("This repository is direct-PR first; issue filing is not its primary contribution path."); | ||||||
|
|
||||||
| let recommendation: PreStartCheckRecommendation; | ||||||
| if (claimStatus === "solved" || qualityEntry?.status === "do_not_use" || lifecycleEntry?.state === "duplicate" || lifecycleEntry?.state === "invalid" || duplicateClusterRisk === "high") { | ||||||
| recommendation = "avoid"; | ||||||
| } else if (!found || duplicateClusterRisk === "medium" || claimStatus === "claimed" || qualityUncertain || lane.lane === "direct_pr") { | ||||||
| recommendation = "raise"; | ||||||
| } else { | ||||||
| recommendation = "go"; | ||||||
| } | ||||||
| if (recommendation === "go") reasons.push("No claim, duplicate, or solvability blocker was detected in cached metadata; this looks safe to start."); | ||||||
|
|
||||||
| const summary = | ||||||
| recommendation === "go" | ||||||
| ? "Go: no blocking claim, duplicate, or solvability signal in cached metadata." | ||||||
| : recommendation === "raise" | ||||||
| ? "Raise: proceed only after confirming the flagged concerns." | ||||||
| : "Avoid: this target is already claimed, solved, duplicate, or high-risk."; | ||||||
|
|
||||||
| return { | ||||||
| repoFullName: fullName, | ||||||
| generatedAt: nowIso(), | ||||||
| lane, | ||||||
| target: { | ||||||
| requested: { | ||||||
| ...(target.issueNumber != null ? { issueNumber: target.issueNumber } : {}), | ||||||
| ...(target.title ? { title: target.title } : {}), | ||||||
| ...(plannedPaths.length > 0 ? { plannedPaths: target.plannedPaths } : {}), | ||||||
| }, | ||||||
| matchedBy, | ||||||
| resolvedIssueNumber: resolvedNumber, | ||||||
| resolvedIssueTitle: resolvedIssue?.title, | ||||||
| }, | ||||||
| found, | ||||||
| claimStatus, | ||||||
| lifecycle: lifecycleEntry?.state, | ||||||
| issueQualityStatus: qualityEntry?.status, | ||||||
| duplicateClusterRisk, | ||||||
| recommendation, | ||||||
| reasons: [...new Set(reasons)].map((reason) => sanitizePublicComment(reason)), | ||||||
| blockers: [...new Set(blockers)].map((blocker) => sanitizePublicComment(blocker)), | ||||||
| summary: sanitizePublicComment(summary), | ||||||
| }; | ||||||
| } | ||||||
|
|
||||||
| function buildIssueLinkageRecord( | ||||||
| issue: IssueRecord, | ||||||
| lifecycleEntry: IssueDiscoveryLifecycleReport["states"][number] | undefined, | ||||||
|
|
||||||
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.
🧹 Nitpick · Duplicate schema definition
The
checkBeforeStartShapeis defined both in this file and in the CLI script, risking divergence.🤖 Prompt for AI agents