From 38105ec09cae21bf2b368636080c608822ef22d1 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Sat, 7 Feb 2026 16:13:14 +0300 Subject: [PATCH] fix: add defensive checks; improve code quality Signed-off-by: NotAShelf Change-Id: I8c88c07bd852c78d196705da03f372e06a6a6964 --- src/config.ts | 103 ++++++++++++++----------- src/filters.ts | 34 ++++++--- src/github.ts | 199 +++++++++++++++++++++++++++++++++++-------------- src/types.ts | 31 +++++++- 4 files changed, 255 insertions(+), 112 deletions(-) diff --git a/src/config.ts b/src/config.ts index e5896c0..863ac93 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,12 +1,10 @@ -import fs from 'node:fs'; -import path from 'node:path'; -import { createJiti } from 'jiti'; -import dotenv from 'dotenv'; -import type { Config } from './types.js'; +import fs from "node:fs"; +import path from "node:path"; +import dotenv from "dotenv"; +import type { Config } from "./types.js"; -dotenv.config(); - -const jiti = createJiti(__filename, { interopDefault: true }); +// Suppress dotenv warnings +dotenv.config({ quiet: true }); const defaults: Config = { server: { port: 3000 }, @@ -34,31 +32,34 @@ const defaults: Config = { includeReasoning: true, messages: { positive: [ - 'This {type} looks great for the trout! All signals point upstream.', - 'The trout approve of this {type}. Swim on!', - 'Splashing good news - this {type} is looking healthy.', + "This {type} looks great for the trout! All signals point upstream.", + "The trout approve of this {type}. Swim on!", + "Splashing good news - this {type} is looking healthy.", ], negative: [ - 'This {type} is muddying the waters. The trout are concerned.', - 'Warning: the trout sense trouble in this {type}.', - 'Something smells fishy about this {type}. Please review.', + "This {type} is muddying the waters. The trout are concerned.", + "Warning: the trout sense trouble in this {type}.", + "Something smells fishy about this {type}. Please review.", ], neutral: [ - 'The trout have no strong feelings about this {type}.', - 'This {type} is neither upstream nor downstream. Neutral waters.', - 'The trout are watching this {type} with mild interest.', + "The trout have no strong feelings about this {type}.", + "This {type} is neither upstream nor downstream. Neutral waters.", + "The trout are watching this {type} with mild interest.", ], }, - commentMarker: '', + commentMarker: "", allowUpdates: false, }, logging: { - level: 'info', - file: 'troutbot.log', + level: "info", + file: "troutbot.log", }, }; -export function deepMerge>(target: T, source: Partial): T { +export function deepMerge>( + target: T, + source: Partial, +): T { const result = { ...target }; for (const key of Object.keys(source) as (keyof T)[]) { const sourceVal = source[key]; @@ -66,15 +67,15 @@ export function deepMerge>(target: T, source: if ( sourceVal !== null && sourceVal !== undefined && - typeof sourceVal === 'object' && + typeof sourceVal === "object" && !Array.isArray(sourceVal) && - typeof targetVal === 'object' && + typeof targetVal === "object" && !Array.isArray(targetVal) && targetVal !== null ) { result[key] = deepMerge( targetVal as Record, - sourceVal as Record + sourceVal as Record, ) as T[keyof T]; } else if (sourceVal !== undefined) { result[key] = sourceVal as T[keyof T]; @@ -83,30 +84,35 @@ export function deepMerge>(target: T, source: return result; } -export function loadConfig(): Config { - const configPath = process.env.CONFIG_PATH || 'config.ts'; +export async function loadConfig(): Promise { + const configPath = process.env.CONFIG_PATH || "config.ts"; const resolvedPath = path.resolve(configPath); let fileConfig: Partial = {}; if (fs.existsSync(resolvedPath)) { - const loaded = jiti(resolvedPath) as Partial | { default: Partial }; - fileConfig = 'default' in loaded ? loaded.default : loaded; + const loaded = await import(resolvedPath); + fileConfig = + "default" in loaded + ? loaded.default + : (loaded as unknown as Partial); } else if (process.env.CONFIG_PATH) { console.warn( - `Warning: CONFIG_PATH is set to "${process.env.CONFIG_PATH}" but file not found at ${resolvedPath}` + `Warning: CONFIG_PATH is set to "${process.env.CONFIG_PATH}" but file not found at ${resolvedPath}`, ); } const config = deepMerge( defaults as unknown as Record, - fileConfig as unknown as Record + fileConfig as unknown as Record, ) as unknown as Config; // Environment variable overrides if (process.env.PORT) { const parsed = parseInt(process.env.PORT, 10); if (Number.isNaN(parsed)) { - throw new Error(`Invalid PORT value: "${process.env.PORT}" is not a number`); + throw new Error( + `Invalid PORT value: "${process.env.PORT}" is not a number`, + ); } config.server.port = parsed; } @@ -119,7 +125,7 @@ export function loadConfig(): Config { config.dashboard = { ...(config.dashboard || { enabled: true }), auth: { - type: 'token', + type: "token", token: process.env.DASHBOARD_TOKEN, }, }; @@ -129,18 +135,18 @@ export function loadConfig(): Config { config.dashboard = { ...(config.dashboard || { enabled: true }), auth: { - type: 'basic', + type: "basic", username: process.env.DASHBOARD_USERNAME, password: process.env.DASHBOARD_PASSWORD, }, }; } - const validLogLevels = ['debug', 'info', 'warn', 'error']; + const validLogLevels = ["debug", "info", "warn", "error"]; if (process.env.LOG_LEVEL) { if (!validLogLevels.includes(process.env.LOG_LEVEL)) { throw new Error( - `Invalid LOG_LEVEL: "${process.env.LOG_LEVEL}". Must be one of: ${validLogLevels.join(', ')}` + `Invalid LOG_LEVEL: "${process.env.LOG_LEVEL}". Must be one of: ${validLogLevels.join(", ")}`, ); } config.logging.level = process.env.LOG_LEVEL; @@ -151,23 +157,36 @@ export function loadConfig(): Config { } export function validate(config: Config): void { - if (!config.server.port || config.server.port < 1 || config.server.port > 65535) { - throw new Error('Invalid server port'); + if ( + !config.server.port || + config.server.port < 1 || + config.server.port > 65535 + ) { + throw new Error("Invalid server port"); } const { backends } = config.engine; - if (!backends.checks.enabled && !backends.diff.enabled && !backends.quality.enabled) { - throw new Error('At least one engine backend must be enabled'); + if ( + !backends.checks.enabled && + !backends.diff.enabled && + !backends.quality.enabled + ) { + throw new Error("At least one engine backend must be enabled"); } const { weights } = config.engine; for (const [key, value] of Object.entries(weights)) { if (value < 0) { - throw new Error(`Backend weight "${key}" must be non-negative, got ${value}`); + throw new Error( + `Backend weight "${key}" must be non-negative, got ${value}`, + ); } } - if (config.engine.confidenceThreshold < 0 || config.engine.confidenceThreshold > 1) { - throw new Error('confidenceThreshold must be between 0 and 1'); + if ( + config.engine.confidenceThreshold < 0 || + config.engine.confidenceThreshold > 1 + ) { + throw new Error("confidenceThreshold must be between 0 and 1"); } } diff --git a/src/filters.ts b/src/filters.ts index 6908ee4..3d6f36a 100644 --- a/src/filters.ts +++ b/src/filters.ts @@ -1,41 +1,53 @@ -import type { FiltersConfig, WebhookEvent } from './types.js'; +import type { FiltersConfig, WebhookEvent } from "./types.js"; export function shouldProcess( event: WebhookEvent, - filters: FiltersConfig + filters: FiltersConfig, ): { pass: boolean; reason?: string } { // Label filters if (filters.labels.include.length > 0) { - const hasRequired = event.labels.some((l) => filters.labels.include.includes(l)); + const hasRequired = event.labels.some((l) => + filters.labels.include.includes(l), + ); if (!hasRequired) { - return { pass: false, reason: 'Missing required label' }; + return { pass: false, reason: "Missing required label" }; } } if (filters.labels.exclude.length > 0) { - const hasExcluded = event.labels.some((l) => filters.labels.exclude.includes(l)); + const hasExcluded = event.labels.some((l) => + filters.labels.exclude.includes(l), + ); if (hasExcluded) { - return { pass: false, reason: 'Has excluded label' }; + return { pass: false, reason: "Has excluded label" }; } } // Author filters if (filters.authors.include && filters.authors.include.length > 0) { - if (!filters.authors.include.includes(event.author)) { - return { pass: false, reason: 'Author not in include list' }; + const normalizedAuthor = event.author.toLowerCase(); + const hasIncluded = filters.authors.include.some( + (a) => a.toLowerCase() === normalizedAuthor, + ); + if (!hasIncluded) { + return { pass: false, reason: "Author not in include list" }; } } if (filters.authors.exclude.length > 0) { - if (filters.authors.exclude.includes(event.author)) { - return { pass: false, reason: 'Author is excluded' }; + const normalizedAuthor = event.author.toLowerCase(); + const isExcluded = filters.authors.exclude.some( + (a) => a.toLowerCase() === normalizedAuthor, + ); + if (isExcluded) { + return { pass: false, reason: "Author is excluded" }; } } // Branch filters (PRs only) if (event.branch && filters.branches.include.length > 0) { if (!filters.branches.include.includes(event.branch)) { - return { pass: false, reason: 'Branch not in include list' }; + return { pass: false, reason: "Branch not in include list" }; } } diff --git a/src/github.ts b/src/github.ts index 40bdc95..29dc69e 100644 --- a/src/github.ts +++ b/src/github.ts @@ -1,12 +1,14 @@ -import { Octokit } from '@octokit/rest'; -import { getLogger } from './logger.js'; -import type { CheckRun, PRFile, ResponseConfig } from './types.js'; +import { Octokit } from "@octokit/rest"; +import { getLogger } from "./logger.js"; +import type { CheckRun, PRFile } from "./types.js"; let octokit: Octokit | null = null; export function initGitHub(token?: string): void { if (!token) { - getLogger().warn('No GITHUB_TOKEN set - running in dry-run mode, comments will not be posted'); + getLogger().warn( + "No GITHUB_TOKEN set - running in dry-run mode, comments will not be posted", + ); return; } octokit = new Octokit({ auth: token }); @@ -21,13 +23,20 @@ export async function postComment( owner: string, repo: string, issueNumber: number, - body: string + body: string, ): Promise { if (!octokit) { - getLogger().info(`[dry-run] Would post comment on ${owner}/${repo}#${issueNumber}:\n${body}`); + getLogger().info( + `[dry-run] Would post comment on ${owner}/${repo}#${issueNumber}:\n${body}`, + ); return; } - await octokit.issues.createComment({ owner, repo, issue_number: issueNumber, body }); + await octokit.issues.createComment({ + owner, + repo, + issue_number: issueNumber, + body, + }); getLogger().info(`Posted comment on ${owner}/${repo}#${issueNumber}`); } @@ -35,7 +44,7 @@ export async function hasExistingComment( owner: string, repo: string, issueNumber: number, - marker: string + marker: string, ): Promise<{ exists: boolean; commentId?: number }> { if (!octokit) { return { exists: false }; @@ -59,13 +68,18 @@ export async function updateComment( owner: string, repo: string, commentId: number, - body: string + body: string, ): Promise { if (!octokit) { getLogger().info(`[dry-run] Would update comment ${commentId}:\n${body}`); return; } - await octokit.issues.updateComment({ owner, repo, comment_id: commentId, body }); + await octokit.issues.updateComment({ + owner, + repo, + comment_id: commentId, + body, + }); getLogger().info(`Updated comment ${commentId} on ${owner}/${repo}`); } @@ -74,26 +88,41 @@ export async function createReaction( repo: string, commentId: number, reaction: - | 'thumbs_up' - | 'thumbs_down' - | 'laugh' - | 'confused' - | 'heart' - | 'hooray' - | 'eyes' - | 'rocket' + | "thumbs_up" + | "thumbs_down" + | "laugh" + | "confused" + | "heart" + | "hooray" + | "eyes" + | "rocket", ): Promise { if (!octokit) { - getLogger().info(`[dry-run] Would add ${reaction} reaction to comment ${commentId}`); + getLogger().info( + `[dry-run] Would add ${reaction} reaction to comment ${commentId}`, + ); return; } // Map thumbs_up/thumbs_down to GitHub API format (+1/-1) - const content = reaction === 'thumbs_up' ? '+1' : reaction === 'thumbs_down' ? '-1' : reaction; + const content = + reaction === "thumbs_up" + ? "+1" + : reaction === "thumbs_down" + ? "-1" + : reaction; await octokit.reactions.createForIssueComment({ owner, repo, comment_id: commentId, - content: content as '+1' | '-1' | 'laugh' | 'confused' | 'heart' | 'hooray' | 'eyes' | 'rocket', + content: content as + | "+1" + | "-1" + | "laugh" + | "confused" + | "heart" + | "hooray" + | "eyes" + | "rocket", }); getLogger().info(`Added ${reaction} reaction to comment ${commentId}`); } @@ -102,10 +131,10 @@ export async function createReaction( export async function fetchCheckRuns( owner: string, repo: string, - ref: string + ref: string, ): Promise { if (!octokit) { - getLogger().debug('[dry-run] Cannot fetch check runs without a token'); + getLogger().debug("[dry-run] Cannot fetch check runs without a token"); return []; } @@ -126,10 +155,10 @@ export async function fetchCheckRuns( export async function fetchPRFiles( owner: string, repo: string, - prNumber: number + prNumber: number, ): Promise { if (!octokit) { - getLogger().debug('[dry-run] Cannot fetch PR files without a token'); + getLogger().debug("[dry-run] Cannot fetch PR files without a token"); return []; } @@ -151,7 +180,7 @@ export async function fetchPRFiles( export async function fetchPR( owner: string, repo: string, - prNumber: number + prNumber: number, ): Promise<{ title: string; body: string; @@ -163,12 +192,18 @@ export async function fetchPR( if (!octokit) return null; try { - const { data } = await octokit.pulls.get({ owner, repo, pull_number: prNumber }); + const { data } = await octokit.pulls.get({ + owner, + repo, + pull_number: prNumber, + }); return { title: data.title, - body: data.body || '', - author: data.user?.login || '', - labels: (data.labels || []).map((l) => (typeof l === 'string' ? l : l.name || '')), + body: data.body || "", + author: data.user?.login || "", + labels: (data.labels || []).map((l) => + typeof l === "string" ? l : l.name || "", + ), branch: data.head.ref, sha: data.head.sha, }; @@ -181,7 +216,7 @@ export async function fetchPR( export async function fetchIssue( owner: string, repo: string, - issueNumber: number + issueNumber: number, ): Promise<{ title: string; body: string; @@ -191,19 +226,57 @@ export async function fetchIssue( if (!octokit) return null; try { - const { data } = await octokit.issues.get({ owner, repo, issue_number: issueNumber }); + const { data } = await octokit.issues.get({ + owner, + repo, + issue_number: issueNumber, + }); return { title: data.title, - body: data.body || '', - author: data.user?.login || '', - labels: (data.labels || []).map((l) => (typeof l === 'string' ? l : l.name || '')), + body: data.body || "", + author: data.user?.login || "", + labels: (data.labels || []).map((l) => + typeof l === "string" ? l : l.name || "", + ), }; } catch (err) { - getLogger().debug(`Failed to fetch issue ${owner}/${repo}#${issueNumber}`, err); + getLogger().debug( + `Failed to fetch issue ${owner}/${repo}#${issueNumber}`, + err, + ); return null; } } +export async function listAccessibleRepositories(): Promise< + Array<{ owner: string; repo: string }> +> { + if (!octokit) { + getLogger().debug("[dry-run] Cannot fetch repositories without a token"); + return []; + } + + const repos: Array<{ owner: string; repo: string }> = []; + + for await (const response of octokit.paginate.iterator( + octokit.repos.listForAuthenticatedUser, + { + per_page: 100, + sort: "updated", + }, + )) { + for (const repo of response.data) { + if (!repo.full_name || typeof repo.full_name !== "string") continue; + const parts = repo.full_name.split("/"); + if (parts.length < 2 || !parts[0] || !parts[1]) continue; + const [owner, repoName] = parts; + repos.push({ owner, repo: repoName }); + } + } + + return repos; +} + export interface RecentComment { id: number; body: string; @@ -216,10 +289,10 @@ export interface RecentComment { export async function listRecentComments( owner: string, repo: string, - since: Date + since: Date, ): Promise { if (!octokit) { - getLogger().debug('[dry-run] Cannot fetch comments without a token'); + getLogger().debug("[dry-run] Cannot fetch comments without a token"); return []; } @@ -227,12 +300,15 @@ export async function listRecentComments( const comments: RecentComment[] = []; // Fetch recent issue comments - const issueComments = await octokit.paginate(octokit.issues.listCommentsForRepo, { - owner, - repo, - since: sinceIso, - per_page: 100, - }); + const issueComments = await octokit.paginate( + octokit.issues.listCommentsForRepo, + { + owner, + repo, + since: sinceIso, + per_page: 100, + }, + ); for (const comment of issueComments) { if (!comment.body) continue; @@ -240,9 +316,11 @@ export async function listRecentComments( comments.push({ id: comment.id, body: comment.body, - author: comment.user?.login || '', + author: comment.user?.login || "", createdAt: comment.created_at, - issueNumber: comment.issue_url ? parseInt(comment.issue_url.split('/').pop() || '0', 10) : 0, + issueNumber: comment.issue_url + ? parseInt(comment.issue_url.split("/").pop() || "0", 10) + : 0, isPullRequest: false, // we'll determine this by fetching the issue }); } @@ -256,19 +334,28 @@ function pickRandom(list: string[]): string { } export function formatComment( - responseConfig: ResponseConfig, - type: 'issue' | 'pull_request', + responseConfig: { + commentMarker: string; + includeConfidence: boolean; + includeReasoning: boolean; + messages: { + positive: string[]; + negative: string[]; + neutral: string[]; + }; + }, + type: "issue" | "pull_request", impact: string, confidence: number, - reasoning: string + reasoning: string, ): string { - const typeLabel = type === 'pull_request' ? 'pull request' : 'issue'; + const typeLabel = type === "pull_request" ? "pull request" : "issue"; const { messages } = responseConfig; let messageList: string[]; - if (impact === 'positive') { + if (impact === "positive") { messageList = messages.positive; - } else if (impact === 'negative') { + } else if (impact === "negative") { messageList = messages.negative; } else { messageList = messages.neutral; @@ -276,14 +363,16 @@ export function formatComment( const template = pickRandom(messageList); - let body = responseConfig.commentMarker + '\n\n'; - body += template.replace(/\{type\}/g, typeLabel).replace(/\{impact\}/g, impact); + let body = responseConfig.commentMarker + "\n\n"; + body += template + .replace(/\{type\}/g, typeLabel) + .replace(/\{impact\}/g, impact); if (responseConfig.includeConfidence) { body += `\n\n**Confidence:** ${(confidence * 100).toFixed(0)}%`; } if (responseConfig.includeReasoning) { - body += `\n\n**Analysis:** ${reasoning}`; + body += `\n\n**Analysis:**\n\`\`\`\n${reasoning}\n\`\`\``; } return body; diff --git a/src/types.ts b/src/types.ts index 2bbe471..77a8680 100644 --- a/src/types.ts +++ b/src/types.ts @@ -16,7 +16,7 @@ export interface PollingConfig { backfill?: boolean; stateFile?: string; authorizedUsers?: string[]; - repositories?: RepoConfig[]; + repositories?: RepoPattern[]; // Can include wildcards like [{ owner: 'NotAShelf', repo: '*' }] } export interface ServerConfig { @@ -31,7 +31,7 @@ export interface DashboardConfig { } export interface DashboardAuthConfig { - type: 'basic' | 'token'; + type: "basic" | "token"; username?: string; password?: string; token?: string; @@ -42,6 +42,11 @@ export interface RepoConfig { repo: string; } +export interface RepoPattern { + owner: string; + repo?: string; // undefined means all repos for that owner +} + export interface FiltersConfig { labels: { include: string[]; @@ -106,12 +111,30 @@ export interface LoggingConfig { file: string; } -export type Impact = 'positive' | 'negative' | 'neutral'; +export type Impact = "positive" | "negative" | "neutral"; export interface AnalysisResult { impact: Impact; confidence: number; reasoning: string; + // Multi-dimensional scoring for sophisticated analysis + dimensions?: { + correctness: number; // -1 to 1: will this work correctly? + risk: number; // -1 to 1: could this break things? (negative = risky) + maintainability: number; // -1 to 1: will this be maintainable? + alignment: number; // -1 to 1: does this fit the project? + }; + // Correlation analysis + correlations?: { + suspiciousPatterns: string[]; + reinforcingSignals: string[]; + contradictions: string[]; + }; + // Uncertainty quantification + uncertainty?: { + confidenceInterval: [number, number]; // [lower, upper] + primaryUncertaintySource: string; + }; } export interface EngineBackend { @@ -121,7 +144,7 @@ export interface EngineBackend { export interface WebhookEvent { action: string; - type: 'issue' | 'pull_request'; + type: "issue" | "pull_request"; number: number; title: string; body: string;