Skip to content

Commit b5c41ad

Browse files
costajohntcostajohnt
authored
fix: resolve 10 audit issues — security, reliability, bugs, consolidation (#534)
* fix: resolve 10 open issues — security, reliability, bugs, consolidation, performance - Fix vetIssuesParallel concurrency bug: replace Promise.race+splice with Map-based tracking (#517) - Fix minor bugs: daysBetween clamping, formatRelativeTime future guard, star count log typo, checklist stats consistency (#525) - Consolidate isRateLimitError/isRateLimitOrAuthError into errors.ts from 4 files (#518) - Replace all console.error with structured logger.warn in dashboard-data.ts and dashboard-server.ts (#530) - Add security headers (X-Content-Type-Options, X-Frame-Options, CSP, Referrer-Policy), Origin validation, request timeout (#519) - Improve dashboard server reliability: replace process.exit with thrown errors, sanitize client error messages (#526) - Add SRI integrity hash and pin Chart.js to v4.5.1 (#528) - Parallelize CONTRIBUTING.md probing with Promise.allSettled (#520) - Extract shared updateMonthlyAnalytics function, eliminating duplicated analytics code (#527) - Consolidate DEFAULT_CONCURRENCY constant across 3 files (#522) * fix: address lint errors — add error causes, prefix unused catch var * style: fix prettier formatting
1 parent 872aa8b commit b5c41ad

18 files changed

+259
-271
lines changed

‎packages/core/src/commands/daily.ts‎

Lines changed: 3 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,10 @@ import {
3131
type PRCheckFailure,
3232
type RepoGroup,
3333
} from '../core/index.js';
34-
import { errorMessage, getHttpStatusCode } from '../core/errors.js';
34+
import { errorMessage, isRateLimitOrAuthError } from '../core/errors.js';
3535
import { warn } from '../core/logger.js';
3636
import { emptyPRCountsResult } from '../core/github-stats.js';
37+
import { updateMonthlyAnalytics } from './dashboard-data.js';
3738
import {
3839
deduplicateDigest,
3940
compactActionableIssues,
@@ -46,17 +47,6 @@ import {
4647

4748
const MODULE = 'daily';
4849

49-
/** Return true for errors that should propagate (not degrade gracefully). */
50-
function isRateLimitOrAuthError(err: unknown): boolean {
51-
const status = getHttpStatusCode(err);
52-
if (status === 401 || status === 429) return true;
53-
if (status === 403) {
54-
const msg = errorMessage(err).toLowerCase();
55-
return msg.includes('rate limit') || msg.includes('abuse detection');
56-
}
57-
return false;
58-
}
59-
6050
// Re-export domain functions so existing consumers (tests, dashboard, startup)
6151
// can continue importing from './daily.js' without changes.
6252
export {
@@ -335,60 +325,6 @@ async function updateRepoScores(
335325
}
336326
}
337327

338-
/**
339-
* Phase 3: Persist monthly chart analytics to state.
340-
* Stores merged, closed, and combined opened counts per month.
341-
* Each metric is isolated so partial failures don't produce inconsistent state.
342-
*/
343-
function updateAnalytics(
344-
prs: FetchedPR[],
345-
monthlyCounts: Record<string, number>,
346-
monthlyClosedCounts: Record<string, number>,
347-
openedFromMerged: Record<string, number>,
348-
openedFromClosed: Record<string, number>,
349-
): void {
350-
const stateManager = getStateManager();
351-
352-
// Store monthly chart data (non-critical — each metric isolated so partial failures don't leave inconsistent state).
353-
// Guard: skip overwriting when the data is empty to avoid wiping existing chart data on transient API failures.
354-
// An empty object means the fetch failed and fell back to emptyPRCountsResult(), so we preserve previous state.
355-
try {
356-
if (Object.keys(monthlyCounts).length > 0) {
357-
stateManager.setMonthlyMergedCounts(monthlyCounts);
358-
}
359-
} catch (error) {
360-
warn(MODULE, `Failed to store monthly merged counts: ${errorMessage(error)}`);
361-
}
362-
363-
try {
364-
if (Object.keys(monthlyClosedCounts).length > 0) {
365-
stateManager.setMonthlyClosedCounts(monthlyClosedCounts);
366-
}
367-
} catch (error) {
368-
warn(MODULE, `Failed to store monthly closed counts: ${errorMessage(error)}`);
369-
}
370-
371-
try {
372-
// Build combined monthly opened counts from merged + closed + currently-open PRs
373-
const combinedOpenedCounts: Record<string, number> = { ...openedFromMerged };
374-
for (const [month, count] of Object.entries(openedFromClosed)) {
375-
combinedOpenedCounts[month] = (combinedOpenedCounts[month] || 0) + count;
376-
}
377-
// Add currently-open PR creation dates
378-
for (const pr of prs) {
379-
if (pr.createdAt) {
380-
const month = pr.createdAt.slice(0, 7);
381-
combinedOpenedCounts[month] = (combinedOpenedCounts[month] || 0) + 1;
382-
}
383-
}
384-
if (Object.keys(combinedOpenedCounts).length > 0) {
385-
stateManager.setMonthlyOpenedCounts(combinedOpenedCounts);
386-
}
387-
} catch (error) {
388-
warn(MODULE, `Failed to compute/store monthly opened counts: ${errorMessage(error)}`);
389-
}
390-
}
391-
392328
/**
393329
* Phase 4: Expire snoozes and partition PRs into active vs shelved buckets.
394330
* Auto-unshelves PRs where maintainers have engaged, generates the digest,
@@ -627,7 +563,7 @@ async function executeDailyCheckInternal(token: string): Promise<DailyCheckResul
627563
await updateRepoScores(prMonitor, prs, mergedCounts, closedCounts);
628564

629565
// Phase 3: Persist monthly analytics
630-
updateAnalytics(prs, monthlyCounts, monthlyClosedCounts, openedFromMerged, openedFromClosed);
566+
updateMonthlyAnalytics(prs, monthlyCounts, monthlyClosedCounts, openedFromMerged, openedFromClosed);
631567

632568
// Phase 4: Expire snoozes, partition PRs, generate and save digest
633569
const { activePRs, shelvedPRs, digest } = partitionPRs(prMonitor, prs, recentlyClosedPRs, recentlyMergedPRs);

‎packages/core/src/commands/dashboard-data.ts‎

Lines changed: 62 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,12 @@
55
*/
66

77
import { getStateManager, PRMonitor, IssueConversationMonitor } from '../core/index.js';
8-
import { errorMessage, getHttpStatusCode } from '../core/errors.js';
8+
import { errorMessage, isRateLimitOrAuthError } from '../core/errors.js';
9+
import { warn } from '../core/logger.js';
910
import { emptyPRCountsResult } from '../core/github-stats.js';
1011
import { toShelvedPRRef } from './daily.js';
12+
13+
const MODULE = 'dashboard-data';
1114
import type { DailyDigest, AgentState, ClosedPR, MergedPR, CommentedIssue } from '../core/types.js';
1215

1316
export interface DashboardStats {
@@ -34,6 +37,53 @@ export function buildDashboardStats(digest: DailyDigest, state: Readonly<AgentSt
3437
};
3538
}
3639

40+
/**
41+
* Persist monthly chart analytics (merged, closed, opened) to state.
42+
* Each metric is isolated so partial failures don't produce inconsistent state.
43+
* Skips overwriting when data is empty to avoid wiping chart data on transient API failures.
44+
*/
45+
export function updateMonthlyAnalytics(
46+
prs: Array<{ createdAt?: string }>,
47+
monthlyCounts: Record<string, number>,
48+
monthlyClosedCounts: Record<string, number>,
49+
openedFromMerged: Record<string, number>,
50+
openedFromClosed: Record<string, number>,
51+
): void {
52+
const stateManager = getStateManager();
53+
54+
try {
55+
if (Object.keys(monthlyCounts).length > 0) {
56+
stateManager.setMonthlyMergedCounts(monthlyCounts);
57+
}
58+
} catch (error) {
59+
warn(MODULE, `Failed to store monthly merged counts: ${errorMessage(error)}`);
60+
}
61+
try {
62+
if (Object.keys(monthlyClosedCounts).length > 0) {
63+
stateManager.setMonthlyClosedCounts(monthlyClosedCounts);
64+
}
65+
} catch (error) {
66+
warn(MODULE, `Failed to store monthly closed counts: ${errorMessage(error)}`);
67+
}
68+
try {
69+
const combinedOpenedCounts: Record<string, number> = { ...openedFromMerged };
70+
for (const [month, count] of Object.entries(openedFromClosed)) {
71+
combinedOpenedCounts[month] = (combinedOpenedCounts[month] || 0) + count;
72+
}
73+
for (const pr of prs) {
74+
if (pr.createdAt) {
75+
const month = pr.createdAt.slice(0, 7);
76+
combinedOpenedCounts[month] = (combinedOpenedCounts[month] || 0) + 1;
77+
}
78+
}
79+
if (Object.keys(combinedOpenedCounts).length > 0) {
80+
stateManager.setMonthlyOpenedCounts(combinedOpenedCounts);
81+
}
82+
} catch (error) {
83+
warn(MODULE, `Failed to store monthly opened counts: ${errorMessage(error)}`);
84+
}
85+
}
86+
3787
export interface DashboardFetchResult {
3888
digest: DailyDigest;
3989
commentedIssues: CommentedIssue[];
@@ -44,16 +94,6 @@ export interface DashboardFetchResult {
4494
* Returns the digest and commented issues, updating state as a side effect.
4595
* Throws if the fetch fails entirely (caller should fall back to cached data).
4696
*/
47-
function isRateLimitOrAuthError(err: unknown): boolean {
48-
const status = getHttpStatusCode(err);
49-
if (status === 401 || status === 429) return true;
50-
if (status === 403) {
51-
const msg = errorMessage(err).toLowerCase();
52-
return msg.includes('rate limit') || msg.includes('abuse detection');
53-
}
54-
return false;
55-
}
56-
5797
export async function fetchDashboardData(token: string): Promise<DashboardFetchResult> {
5898
const stateManager = getStateManager();
5999
const prMonitor = new PRMonitor(token);
@@ -64,30 +104,30 @@ export async function fetchDashboardData(token: string): Promise<DashboardFetchR
64104
prMonitor.fetchUserOpenPRs(),
65105
prMonitor.fetchRecentlyClosedPRs().catch((err): ClosedPR[] => {
66106
if (isRateLimitOrAuthError(err)) throw err;
67-
console.error(`Warning: Failed to fetch recently closed PRs: ${errorMessage(err)}`);
107+
warn(MODULE, `Failed to fetch recently closed PRs: ${errorMessage(err)}`);
68108
return [];
69109
}),
70110
prMonitor.fetchRecentlyMergedPRs().catch((err): MergedPR[] => {
71111
if (isRateLimitOrAuthError(err)) throw err;
72-
console.error(`Warning: Failed to fetch recently merged PRs: ${errorMessage(err)}`);
112+
warn(MODULE, `Failed to fetch recently merged PRs: ${errorMessage(err)}`);
73113
return [];
74114
}),
75115
prMonitor.fetchUserMergedPRCounts().catch((err) => {
76116
if (isRateLimitOrAuthError(err)) throw err;
77-
console.error(`Warning: Failed to fetch merged PR counts: ${errorMessage(err)}`);
117+
warn(MODULE, `Failed to fetch merged PR counts: ${errorMessage(err)}`);
78118
return emptyPRCountsResult<{ count: number; lastMergedAt: string }>();
79119
}),
80120
prMonitor.fetchUserClosedPRCounts().catch((err) => {
81121
if (isRateLimitOrAuthError(err)) throw err;
82-
console.error(`Warning: Failed to fetch closed PR counts: ${errorMessage(err)}`);
122+
warn(MODULE, `Failed to fetch closed PR counts: ${errorMessage(err)}`);
83123
return emptyPRCountsResult<number>();
84124
}),
85125
issueMonitor.fetchCommentedIssues().catch((error) => {
86126
const msg = errorMessage(error);
87127
if (msg.includes('No GitHub username configured')) {
88-
console.error(`[DASHBOARD] Issue conversation tracking requires setup: ${msg}`);
128+
warn(MODULE, `Issue conversation tracking requires setup: ${msg}`);
89129
} else {
90-
console.error(`[DASHBOARD] Issue conversation fetch failed: ${msg}`);
130+
warn(MODULE, `Issue conversation fetch failed: ${msg}`);
91131
}
92132
return {
93133
issues: [] as CommentedIssue[],
@@ -98,49 +138,17 @@ export async function fetchDashboardData(token: string): Promise<DashboardFetchR
98138

99139
const commentedIssues = fetchedIssues.issues;
100140
if (fetchedIssues.failures.length > 0) {
101-
console.error(`[DASHBOARD] ${fetchedIssues.failures.length} issue conversation check(s) failed`);
141+
warn(MODULE, `${fetchedIssues.failures.length} issue conversation check(s) failed`);
102142
}
103143

104144
if (failures.length > 0) {
105-
console.error(`Warning: ${failures.length} PR fetch(es) failed`);
145+
warn(MODULE, `${failures.length} PR fetch(es) failed`);
106146
}
107147

108148
// Store monthly chart data (opened/merged/closed) so charts have data
109149
const { monthlyCounts, monthlyOpenedCounts: openedFromMerged } = mergedResult;
110150
const { monthlyCounts: monthlyClosedCounts, monthlyOpenedCounts: openedFromClosed } = closedResult;
111-
112-
// Guard: skip overwriting when data is empty to avoid wiping chart data on transient API failures.
113-
try {
114-
if (Object.keys(monthlyCounts).length > 0) {
115-
stateManager.setMonthlyMergedCounts(monthlyCounts);
116-
}
117-
} catch (error) {
118-
console.error('[DASHBOARD] Failed to store monthly merged counts:', errorMessage(error));
119-
}
120-
try {
121-
if (Object.keys(monthlyClosedCounts).length > 0) {
122-
stateManager.setMonthlyClosedCounts(monthlyClosedCounts);
123-
}
124-
} catch (error) {
125-
console.error('[DASHBOARD] Failed to store monthly closed counts:', errorMessage(error));
126-
}
127-
try {
128-
const combinedOpenedCounts: Record<string, number> = { ...openedFromMerged };
129-
for (const [month, count] of Object.entries(openedFromClosed)) {
130-
combinedOpenedCounts[month] = (combinedOpenedCounts[month] || 0) + count;
131-
}
132-
for (const pr of prs) {
133-
if (pr.createdAt) {
134-
const month = pr.createdAt.slice(0, 7);
135-
combinedOpenedCounts[month] = (combinedOpenedCounts[month] || 0) + 1;
136-
}
137-
}
138-
if (Object.keys(combinedOpenedCounts).length > 0) {
139-
stateManager.setMonthlyOpenedCounts(combinedOpenedCounts);
140-
}
141-
} catch (error) {
142-
console.error('[DASHBOARD] Failed to store monthly opened counts:', errorMessage(error));
143-
}
151+
updateMonthlyAnalytics(prs, monthlyCounts, monthlyClosedCounts, openedFromMerged, openedFromClosed);
144152

145153
const digest = prMonitor.generateDigest(prs, recentlyClosedPRs, recentlyMergedPRs);
146154

@@ -156,9 +164,9 @@ export async function fetchDashboardData(token: string): Promise<DashboardFetchR
156164
try {
157165
stateManager.save();
158166
} catch (error) {
159-
console.error('Warning: Failed to save dashboard digest to state:', errorMessage(error));
167+
warn(MODULE, `Failed to save dashboard digest to state: ${errorMessage(error)}`);
160168
}
161-
console.error(`Refreshed: ${prs.length} PRs fetched`);
169+
warn(MODULE, `Refreshed: ${prs.length} PRs fetched`);
162170

163171
return { digest, commentedIssues };
164172
}

‎packages/core/src/commands/dashboard-server.test.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ function createMockReq(method: string, url: string, body?: string): IncomingMess
171171
const req = new EventEmitter() as IncomingMessage;
172172
req.method = method;
173173
req.url = url;
174+
req.headers = {};
174175

175176
// For POST requests, simulate body streaming
176177
if (body !== undefined) {
@@ -203,6 +204,11 @@ function createMockRes(): { res: ServerResponse; result: Promise<MockResponseRes
203204

204205
const res = resEmitter as unknown as ServerResponse;
205206

207+
res.setHeader = vi.fn((name: string, value: string | number) => {
208+
headers[name.toLowerCase()] = value;
209+
return res;
210+
});
211+
206212
res.writeHead = vi.fn((code: number, hdrs?: Record<string, string | number>) => {
207213
statusCode = code;
208214
if (hdrs) Object.assign(headers, hdrs);

0 commit comments

Comments
 (0)