fix: address PR bot review comments

Critical fixes:
- Fix duplicated isPolling/pollLastTouched in events route (missing closing brace)
- Add missing path import to realtime.ts (path.basename was used without import)
- Fix error.message leak in sessions and beads/read routes (security)
- Add missing NextResponse import to activity route
- Fix diffDependencies to use composite key (type:target) for accurate tracking

Code quality:
- Fix beadCounts computation in kanban-controls (was counting epic's own deps, not child issues)
- Replace require('path') with ES module imports throughout

Tests: 13/15 passing (2 contract tests remain brittle)

Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
openhands 2026-02-14 09:34:10 +00:00
parent 6b0e76330e
commit 664ef2892b
8 changed files with 51 additions and 23 deletions

View file

@ -1,9 +1,11 @@
import { NextResponse } from 'next/server';
import path from 'node:path';
import { activityEventBus } from '../../../lib/realtime'; import { activityEventBus } from '../../../lib/realtime';
function isValidProjectRoot(root: string): boolean { function isValidProjectRoot(root: string): boolean {
try { try {
const resolved = require('path').resolve(root); const resolved = path.resolve(root);
return require('path').isAbsolute(resolved); return path.isAbsolute(resolved);
} catch { } catch {
return false; return false;
} }

View file

@ -27,12 +27,13 @@ export async function GET(request: Request): Promise<Response> {
const issues = await readIssuesFromDisk({ projectRoot, preferBd: true }); const issues = await readIssuesFromDisk({ projectRoot, preferBd: true });
return NextResponse.json({ ok: true, issues }); return NextResponse.json({ ok: true, issues });
} catch (error) { } catch (error) {
console.error('[API/BeadsRead] Failed to read issues:', error);
return NextResponse.json( return NextResponse.json(
{ {
ok: false, ok: false,
error: { error: {
classification: 'unknown', classification: 'internal_error',
message: error instanceof Error ? error.message : 'Failed to read issues.', message: 'An internal error occurred while reading issues.',
}, },
}, },
{ status: 500 }, { status: 500 },

View file

@ -96,6 +96,10 @@ export async function GET(request: Request): Promise<Response> {
lastTouchedVersion = nextVersion; lastTouchedVersion = nextVersion;
write(toSseFrame(issuesEventBus.emit(projectRoot, lastTouchedPath, 'changed'))); write(toSseFrame(issuesEventBus.emit(projectRoot, lastTouchedPath, 'changed')));
} }
} finally {
isPolling = false;
}
};
const touchedPoll = setInterval(() => { const touchedPoll = setInterval(() => {
void pollLastTouched(); void pollLastTouched();

View file

@ -1,4 +1,5 @@
import { NextResponse } from 'next/server'; import { NextResponse } from 'next/server';
import path from 'node:path';
import { readIssuesFromDisk } from '../../../lib/read-issues'; import { readIssuesFromDisk } from '../../../lib/read-issues';
import { activityEventBus } from '../../../lib/realtime'; import { activityEventBus } from '../../../lib/realtime';
import { buildSessionTaskFeed, getCommunicationSummary } from '../../../lib/agent-sessions'; import { buildSessionTaskFeed, getCommunicationSummary } from '../../../lib/agent-sessions';
@ -7,8 +8,8 @@ function isValidProjectRoot(root: string): boolean {
// Basic validation: path should not contain traversal patterns // Basic validation: path should not contain traversal patterns
// and should resolve to an absolute path // and should resolve to an absolute path
try { try {
const resolved = require('path').resolve(root); const resolved = path.resolve(root);
return require('path').isAbsolute(resolved); return path.isAbsolute(resolved);
} catch { } catch {
return false; return false;
} }
@ -42,8 +43,8 @@ export async function GET(request: Request): Promise<Response> {
{ {
ok: false, ok: false,
error: { error: {
classification: 'unknown', classification: 'internal_error',
message: error instanceof Error ? error.message : 'Failed to load session feed.', message: 'An internal error occurred while loading the session feed.',
}, },
}, },
{ status: 500 }, { status: 500 },

View file

@ -1,5 +1,6 @@
'use client'; 'use client';
import { useMemo } from 'react';
import { motion } from 'framer-motion'; import { motion } from 'framer-motion';
import type { KanbanFilterOptions, KanbanStats } from '../../lib/kanban'; import type { KanbanFilterOptions, KanbanStats } from '../../lib/kanban';
@ -12,6 +13,7 @@ interface KanbanControlsProps {
filters: KanbanFilterOptions; filters: KanbanFilterOptions;
stats: KanbanStats; stats: KanbanStats;
epics: BeadIssue[]; epics: BeadIssue[];
issues: BeadIssue[];
onFiltersChange: (filters: KanbanFilterOptions) => void; onFiltersChange: (filters: KanbanFilterOptions) => void;
onNextActionable: () => void; onNextActionable: () => void;
nextActionableFeedback?: string | null; nextActionableFeedback?: string | null;
@ -21,6 +23,7 @@ export function KanbanControls({
filters, filters,
stats, stats,
epics, epics,
issues,
onFiltersChange, onFiltersChange,
onNextActionable, onNextActionable,
nextActionableFeedback = null, nextActionableFeedback = null,
@ -29,12 +32,24 @@ export function KanbanControls({
'ui-field rounded-xl px-3 py-2.5 text-sm outline-none transition'; 'ui-field rounded-xl px-3 py-2.5 text-sm outline-none transition';
// Build bead counts map for EpicChipStrip // Build bead counts map for EpicChipStrip
const beadCounts = new Map<string, number>(); // Count non-epic issues that have this epic as their parent
for (const epic of epics) { const beadCounts = useMemo(() => {
// Count non-epic issues that belong to this epic const counts = new Map<string, number>();
const count = epic.dependencies?.filter(d => d.type === 'parent' && d.target === epic.id).length ?? 0; for (const epic of epics) {
beadCounts.set(epic.id, count); let count = 0;
} for (const issue of issues) {
if (issue.issue_type === 'epic') continue;
const parentDep = issue.dependencies.find(d => d.type === 'parent');
const inferredParent = issue.id.includes('.') ? issue.id.split('.')[0] : null;
const parentEpicId = parentDep?.target ?? inferredParent;
if (parentEpicId === epic.id) {
count++;
}
}
counts.set(epic.id, count);
}
return counts;
}, [epics, issues]);
return ( return (
<section className="grid gap-3"> <section className="grid gap-3">

View file

@ -230,6 +230,7 @@ export function KanbanPage({
filters={filters} filters={filters}
stats={stats} stats={stats}
epics={localIssues.filter((issue) => issue.issue_type === 'epic')} epics={localIssues.filter((issue) => issue.issue_type === 'epic')}
issues={localIssues}
onFiltersChange={setFilters} onFiltersChange={setFilters}
onNextActionable={handleNextActionable} onNextActionable={handleNextActionable}
nextActionableFeedback={nextActionableFeedback} nextActionableFeedback={nextActionableFeedback}

View file

@ -1,3 +1,4 @@
import path from 'node:path';
import { canonicalizeWindowsPath, windowsPathKey } from './pathing'; import { canonicalizeWindowsPath, windowsPathKey } from './pathing';
import type { ActivityEvent } from './activity'; import type { ActivityEvent } from './activity';

View file

@ -78,7 +78,7 @@ export function diffSnapshots(
// 5. Collection Changes (Dependencies) // 5. Collection Changes (Dependencies)
diffDependencies(prev.dependencies, curr.dependencies).forEach(kindAndTarget => { diffDependencies(prev.dependencies, curr.dependencies).forEach(kindAndTarget => {
events.push(createEvent(kindAndTarget.kind, curr, now, { to: kindAndTarget.target })); events.push(createEvent(kindAndTarget.kind, curr, now, { to: kindAndTarget.target, field: kindAndTarget.type }));
}); });
}); });
@ -119,25 +119,28 @@ function areArraysEqual(a: string[], b: string[]): boolean {
/** /**
* Detects added and removed dependencies. * Detects added and removed dependencies.
* Uses composite key `${type}:${target}` to detect type changes as well.
*/ */
function diffDependencies( function diffDependencies(
prev: BeadDependency[], prev: BeadDependency[],
curr: BeadDependency[] curr: BeadDependency[]
): { kind: 'dependency_added' | 'dependency_removed', target: string }[] { ): { kind: 'dependency_added' | 'dependency_removed', target: string, type: string }[] {
const changes: { kind: 'dependency_added' | 'dependency_removed', target: string }[] = []; const changes: { kind: 'dependency_added' | 'dependency_removed', target: string, type: string }[] = [];
const prevTargets = new Set(prev.map(d => d.target)); const prevKeys = new Set(prev.map(d => `${d.type}:${d.target}`));
const currTargets = new Set(curr.map(d => d.target)); const currKeys = new Set(curr.map(d => `${d.type}:${d.target}`));
curr.forEach(d => { curr.forEach(d => {
if (!prevTargets.has(d.target)) { const key = `${d.type}:${d.target}`;
changes.push({ kind: 'dependency_added', target: d.target }); if (!prevKeys.has(key)) {
changes.push({ kind: 'dependency_added', target: d.target, type: d.type });
} }
}); });
prev.forEach(d => { prev.forEach(d => {
if (!currTargets.has(d.target)) { const key = `${d.type}:${d.target}`;
changes.push({ kind: 'dependency_removed', target: d.target }); if (!currKeys.has(key)) {
changes.push({ kind: 'dependency_removed', target: d.target, type: d.type });
} }
}); });