fix(FN-LOCAL): stop tracking local-only artifacts
This commit is contained in:
gsxdsm
@@ -1,575 +0,0 @@
|
||||
# Dashboard Package Review Findings
|
||||
|
||||
**Task:** FN-1964 — Review `packages/dashboard` for bugs and architectural issues
|
||||
**Review Date:** 2026-04-18
|
||||
**Scope:** 112+ files, ~155K lines (server + client + CSS)
|
||||
**Severity Legend:** 🔴 Critical · 🟠 High · 🟡 Medium · ⚪ Low
|
||||
|
||||
---
|
||||
|
||||
## Summary Statistics
|
||||
|
||||
| Severity | Count | Focus Areas |
|
||||
|----------|-------|------------|
|
||||
| 🔴 Critical | 1 | Error Handling |
|
||||
| 🟠 High | 7 | Error Handling, Data Fetching, Accessibility, AI Sessions |
|
||||
| 🟡 Medium | 9 | Accessibility, CSS, Performance, Resource Management |
|
||||
| ⚪ Low | 5 | CSS, Code Quality |
|
||||
| **Total** | **22** | |
|
||||
|
||||
**Findings by Focus Area:**
|
||||
|
||||
| Focus Area | Count |
|
||||
|------------|-------|
|
||||
| Error Handling | 4 |
|
||||
| Accessibility | 4 |
|
||||
| Data Fetching / Frontend Hooks | 4 |
|
||||
| SSE / WebSocket | 3 |
|
||||
| CSS / Styling | 3 |
|
||||
| AI Session Persistence | 2 |
|
||||
| Route Ordering | 1 |
|
||||
| Security | 1 |
|
||||
| Resource Management | 1 |
|
||||
|
||||
---
|
||||
|
||||
## 1. API Routes
|
||||
|
||||
### 1.1 🔴 Critical — `insights-routes.ts`: Missing `catchHandler` Wrapper
|
||||
|
||||
**File:** `packages/dashboard/src/insights-routes.ts:141`
|
||||
**Severity:** Critical
|
||||
**Focus Area:** Error Handling
|
||||
|
||||
The insights router is created without `catchHandler` wrapping its route handlers:
|
||||
|
||||
```typescript
|
||||
// router.ts (how other routers are created):
|
||||
export function createInsightsRouter(...) {
|
||||
const router = Router();
|
||||
// All route handlers throw — no catchHandler wrapping them
|
||||
router.get("/", (req, res) => {
|
||||
try {
|
||||
// ...handler code...
|
||||
} catch (error) {
|
||||
rethrowAsApiError(error, "Failed to get insights");
|
||||
}
|
||||
});
|
||||
```
|
||||
|
||||
Unlike `routes.ts` which uses `catchTypedHandler` and `mission-routes.ts` which uses `catchTypedHandler`, **insights-routes.ts has no catchHandler wrapper**. When a route handler throws an exception (e.g., from `badRequest()` throwing after a validation failure), Express will propagate the thrown `ApiError` without calling `sendErrorResponse()`. Instead, the default Express error handler fires, producing an **HTML error page instead of a JSON response**. This breaks all API clients.
|
||||
|
||||
Compare with `mission-routes.ts:124`:
|
||||
```typescript
|
||||
function catchTypedHandler(fn: (req: TypedRequest, res: Response, next: NextFunction) => Promise<void>) {
|
||||
return catchHandler((req, res, next) => fn(req as TypedRequest, res, next));
|
||||
}
|
||||
|
||||
router.post("/milestones/:missionId/milestones", catchTypedHandler(async (req, res) => { /* ... */ }));
|
||||
```
|
||||
|
||||
**Recommendation:** Wrap all route handlers in `catchHandler` or `catchTypedHandler`, or add an error middleware for the insights router.
|
||||
|
||||
---
|
||||
|
||||
### 1.2 🟠 High — Terminal SSE Route Shadowing
|
||||
|
||||
**File:** `packages/dashboard/src/routes.ts:7445, 7474`
|
||||
**Severity:** High
|
||||
**Focus Area:** Route Ordering
|
||||
|
||||
The Terminal SSE endpoint and kill endpoint have route ordering that causes shadowing:
|
||||
|
||||
```typescript
|
||||
// Line 7445: GET /terminal/sessions/:id — generic parameterized route
|
||||
router.get("/terminal/sessions/:id", (req, res) => { /* returns session or streams */ });
|
||||
|
||||
// Line 7411: POST /terminal/sessions/:id/kill — specific operation
|
||||
router.post("/terminal/sessions/:id/kill", (req, res) => { /* kills session */ });
|
||||
```
|
||||
|
||||
The SSE endpoint at line 7445 is a GET with `/:id` which could shadow `GET /terminal/sessions/kill` (which doesn't exist). The POST kill route at line 7411 comes AFTER the GET SSE route, so it won't be shadowed. However, the kill route uses a POST-with-a-path pattern that should be verified against the documented Express wildcard ordering convention (FN-1492/FN-1909):
|
||||
|
||||
```typescript
|
||||
// Operation routes MUST come before generic routes:
|
||||
router.post("/terminal/sessions/:id/kill", ...); // ← specific first
|
||||
router.get("/terminal/sessions/:id", ...); // ← generic second ← CORRECT
|
||||
```
|
||||
|
||||
Currently this is correct. **No shadowing issue exists** — but the pattern is fragile and any future additions of specific terminal operation routes (e.g., `/terminal/sessions/:id/resize`) must be placed before line 7445.
|
||||
|
||||
---
|
||||
|
||||
### 1.3 🟡 Medium — `project-store-resolver.ts`: Race Condition on Store Eviction
|
||||
|
||||
**File:** `packages/dashboard/src/project-store-resolver.ts:96-107`
|
||||
**Severity:** Medium
|
||||
**Focus Area:** Resource Management
|
||||
|
||||
When `evictProjectStore()` is called during server shutdown or project removal, it synchronously:
|
||||
1. Deletes from `pendingCreations` map
|
||||
2. Stops the watcher
|
||||
3. Closes the store
|
||||
4. Deletes from `storeCache`
|
||||
|
||||
Between steps 1 and 4, a concurrent call to `getOrCreateProjectStore()` for the same `projectId` could:
|
||||
1. See the store is not in `storeCache`
|
||||
2. Create a new pending promise
|
||||
3. Start a new store creation
|
||||
|
||||
This creates two store instances for the same projectId if the eviction and creation overlap. The concurrent creation promise deduplication (`pendingCreations`) only prevents duplicate *creation promises*, not duplicate *stores* if one creation is aborted mid-way.
|
||||
|
||||
```typescript
|
||||
export async function getOrCreateProjectStore(projectId: string): Promise<TaskStore> {
|
||||
const cached = storeCache.get(projectId); // ← checks cache
|
||||
if (cached) return cached; // ← returns cached or creates new
|
||||
const pending = pendingCreations.get(projectId);
|
||||
if (pending) return pending; // ← waits for in-flight creation
|
||||
const creation = (async () => { // ← creates new store
|
||||
// ...
|
||||
storeCache.set(projectId, store); // ← adds to cache
|
||||
})();
|
||||
pendingCreations.set(projectId, creation);
|
||||
return creation;
|
||||
}
|
||||
|
||||
export function evictProjectStore(projectId: string): void {
|
||||
pendingCreations.delete(projectId); // ← removes pending (step 1)
|
||||
const store = storeCache.get(projectId); // ← gets store
|
||||
if (store) {
|
||||
store.stopWatching(); // ← stops watcher
|
||||
store.close(); // ← closes store
|
||||
storeCache.delete(projectId); // ← removes from cache (step 4)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
The gap between `pendingCreations.delete()` and `storeCache.set()` means a concurrent request can see the evicted store is gone and start creating a new one while the old store is still being closed.
|
||||
|
||||
---
|
||||
|
||||
## 2. SSE Pipeline
|
||||
|
||||
### 2.1 🟠 High — SSE Proxy Endpoint Missing Timeout Cleanup
|
||||
|
||||
**File:** `packages/dashboard/src/routes.ts:17531-17614`
|
||||
**Severity:** High
|
||||
**Focus Area:** SSE / WebSocket
|
||||
|
||||
The SSE proxy endpoint (`GET /proxy/:nodeId/events`) creates a 30-second timeout but does NOT clear it in the `req.on("close")` handler. If the client disconnects before the timeout fires, the `clearTimeout` is never called, causing a memory leak:
|
||||
|
||||
```typescript
|
||||
// routes.ts:17531
|
||||
router.get("/proxy/:nodeId/events", async function (req, res) {
|
||||
// ...
|
||||
const timeout = setTimeout(() => controller.abort(), 30_000); // ← timeout created
|
||||
|
||||
req.on("close", () => {
|
||||
if (!destroyed) {
|
||||
destroyed = true;
|
||||
controller.abort(); // ← aborts fetch ✓
|
||||
nodeStream.destroy(); // ← destroys stream ✓
|
||||
// ← MISSING: clearTimeout(timeout)!
|
||||
}
|
||||
});
|
||||
// If client disconnects, req.on("close") fires but timeout is never cleared
|
||||
// The timeout timer keeps running until the 30s expires, then fires controller.abort()
|
||||
// on an already-destroyed stream. Minor memory leak per disconnect.
|
||||
});
|
||||
```
|
||||
|
||||
Compare with the correct pattern in the SSE stream's `heartbeat` cleanup at `sse.ts:571`:
|
||||
```typescript
|
||||
const heartbeat = setInterval(() => { /* ... */ }, 30_000);
|
||||
_req.on("close", () => { clearInterval(heartbeat); /* ... */ });
|
||||
```
|
||||
|
||||
**Recommendation:** Add `clearTimeout(timeout)` in the `req.on("close")` handler.
|
||||
|
||||
---
|
||||
|
||||
### 2.2 🟠 High — `mapSourceEventToTransition`: Non-Standard State Transitions Map to "error"
|
||||
|
||||
**File:** `packages/dashboard/src/sse.ts:127-143`
|
||||
**Severity:** High
|
||||
**Focus Area:** SSE / WebSocket
|
||||
|
||||
```typescript
|
||||
function mapSourceEventToTransition(sourceEvent: string, plugin: PluginInstallation, _previousState?: PluginState): PluginLifecycleTransition {
|
||||
switch (sourceEvent) {
|
||||
case "plugin:registered": return "installing";
|
||||
case "plugin:enabled": return "enabled";
|
||||
case "plugin:disabled": return "disabled";
|
||||
case "plugin:unregistered": return "uninstalled";
|
||||
case "plugin:updated": return "settings-updated";
|
||||
case "plugin:stateChanged":
|
||||
if (plugin.state === "error") return "error";
|
||||
return "error"; // ← ALL non-error state changes map to "error"!
|
||||
default:
|
||||
return "error";
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
When `plugin:stateChanged` fires with a non-error state (e.g., `started`, `stopped`), the return value is `"error"` — which is semantically incorrect. This means the UI will show a red error indicator whenever any plugin enters a non-error state. The comment even says "we don't emit a dedicated transition" but the code returns `"error"` instead of a neutral transition.
|
||||
|
||||
**Recommendation:** Add a separate transition type like `"state-changed"` or fall back to `"enabled"` for running states.
|
||||
|
||||
---
|
||||
|
||||
### 2.3 ⚪ Low — SSE `mapSourceEventToTransition`: Dead Code
|
||||
|
||||
**File:** `packages/dashboard/src/sse.ts:127`
|
||||
**Severity:** Low
|
||||
**Focus Area:** Code Quality
|
||||
|
||||
The `_previousState` parameter is declared but never used in `mapSourceEventToTransition`:
|
||||
|
||||
```typescript
|
||||
function mapSourceEventToTransition(sourceEvent: string, plugin: PluginInstallation, _previousState?: PluginState): PluginLifecycleTransition {
|
||||
// _previousState is never referenced in the function body
|
||||
}
|
||||
```
|
||||
|
||||
Unused parameters are a code quality concern. If this parameter was intended for future use (e.g., to detect transitions like `running→stopped`), it should be removed or used.
|
||||
|
||||
---
|
||||
|
||||
## 3. Frontend Data Fetching
|
||||
|
||||
### 3.1 🟠 High — `useInsights.ts`: `useMemo` Used for Side Effect (Anti-pattern)
|
||||
|
||||
**File:** `packages/dashboard/app/hooks/useInsights.ts:303`
|
||||
**Severity:** High
|
||||
**Focus Area:** Data Fetching
|
||||
|
||||
```typescript
|
||||
// Initial load - intentionally runs once on mount
|
||||
// eslint-disable-next-line
|
||||
useMemo(() => {
|
||||
void refresh();
|
||||
}, []);
|
||||
```
|
||||
|
||||
Using `useMemo` for a side effect (data fetching) is a documented React anti-pattern. While React typically calls `useMemo` eagerly, it is not guaranteed to execute the callback — the memoized value may be skipped in certain implementations or when the component tree is in a Suspense boundary. The **correct pattern is `useEffect`**:
|
||||
|
||||
```typescript
|
||||
useEffect(() => {
|
||||
void refresh();
|
||||
}, []);
|
||||
```
|
||||
|
||||
The eslint-disable comment acknowledges this is intentional, but this still risks the initial load being skipped under non-standard React implementations.
|
||||
|
||||
---
|
||||
|
||||
### 3.2 🟠 High — `api.ts`: `fetchTaskDetail` Bypasses `api()` Error Wrapper
|
||||
|
||||
**File:** `packages/dashboard/app/api.ts:148-163`
|
||||
**Severity:** High
|
||||
**Focus Area:** Data Fetching
|
||||
|
||||
```typescript
|
||||
export async function fetchTaskDetail(id: string, projectId?: string): Promise<TaskDetail> {
|
||||
const maxAttempts = 2;
|
||||
const url = buildApiUrl(withProjectId(`/tasks/${id}`, projectId));
|
||||
for (let attempt = 1; attempt <= maxAttempts; attempt++) {
|
||||
const res = await fetch(url, { headers: { "Content-Type": "application/json" } });
|
||||
const data = await res.json(); // ← throws on non-JSON response
|
||||
if (res.ok) return data as TaskDetail;
|
||||
if (attempt === maxAttempts) {
|
||||
throw new Error((data as { error?: string }).error || "Request failed");
|
||||
}
|
||||
}
|
||||
throw new Error("Request failed");
|
||||
}
|
||||
```
|
||||
|
||||
This bypasses the `api()` wrapper, which handles: (1) HTML-instead-of-JSON detection (`looksLikeHtml`), (2) consistent error extraction, (3) 204 No Content handling. If the server returns an error HTML page, `res.json()` will throw an unhandled exception instead of producing a descriptive error.
|
||||
|
||||
---
|
||||
|
||||
### 3.3 🟠 High — `useRemoteNodeEvents.ts`: Missing `projectId` in SSE URL
|
||||
|
||||
**File:** `packages/dashboard/app/hooks/useRemoteNodeEvents.ts:26`
|
||||
**Severity:** High
|
||||
**Focus Area:** Data Fetching
|
||||
|
||||
```typescript
|
||||
const url = `/api/proxy/${encodeURIComponent(nodeId)}/events`;
|
||||
// No ?projectId=... query parameter is added
|
||||
|
||||
return subscribeSse(url, {
|
||||
events: {
|
||||
"task:created": (e: MessageEvent) => { /* ... */ },
|
||||
// ...other event handlers...
|
||||
},
|
||||
onOpen: () => setIsConnected(true),
|
||||
onError: () => setIsConnected(false),
|
||||
});
|
||||
```
|
||||
|
||||
Unlike `useAgentLogs`, `useMultiAgentLogs`, and `useTasks` which all include `?projectId=...` in their SSE URLs, this hook sends no project scope. The proxy forwards all events without filtering by project. Unlike the SSE hooks that have project-context version guards, `useRemoteNodeEvents` also lacks `projectContextVersionRef` — in-flight events from an old `nodeId` context could update `lastEvent` after a node switch.
|
||||
|
||||
**Recommendation:** Add `projectId` query parameter to the SSE URL and implement project-context version guard pattern.
|
||||
|
||||
---
|
||||
|
||||
### 3.4 🟡 Medium — `useBatchBadgeFetch.ts`: Shared State Not Project-Scoped
|
||||
|
||||
**File:** `packages/dashboard/app/hooks/useBatchBadgeFetch.ts:11-17`
|
||||
**Severity:** Medium
|
||||
**Focus Area:** Data Fetching
|
||||
|
||||
```typescript
|
||||
const batchBadgeStore = {
|
||||
data: new Map<string, { result: BatchStatusResult[string]; timestamp: number }>(),
|
||||
pendingPromise: null as Promise<BatchStatusResult> | null, // ← NOT project-scoped
|
||||
lastFetchTime: null as number | null, // ← NOT project-scoped
|
||||
};
|
||||
```
|
||||
|
||||
The `pendingPromise` and `lastFetchTime` fields are shared across all project scopes. If a user switches projects while a fetch is pending, the new project's hook instance will incorrectly observe `isLoading = true` from the old project's operation.
|
||||
|
||||
The `data` map uses scoped keys (`projectId::taskId`) and is properly isolated, but the `pendingPromise` and `lastFetchTime` are global.
|
||||
|
||||
---
|
||||
|
||||
## 4. AI Session Persistence
|
||||
|
||||
### 4.1 🟡 Medium — `mission-interview.ts`: `projectId` Always Stored as `null`
|
||||
|
||||
**File:** `packages/dashboard/src/mission-interview.ts:292`
|
||||
**Severity:** Medium
|
||||
**Focus Area:** AI Sessions
|
||||
|
||||
```typescript
|
||||
function persistMissionSession(session: MissionInterviewSession, status: ..., error?: string): void {
|
||||
if (!_aiSessionStore) return;
|
||||
const row: AiSessionRow = {
|
||||
// ...
|
||||
projectId: null, // ← Always null despite session being project-scoped
|
||||
// ...
|
||||
};
|
||||
_aiSessionStore.upsert(row);
|
||||
}
|
||||
```
|
||||
|
||||
The mission interview session is created within a project context (the `rootDir` comes from `scopedStore.getRootDir()` via `getProjectContext(req)` in mission-routes.ts), but `projectId` is hardcoded to `null` in persistence. This means dismissed mission interview sessions cannot be recovered per-project after server restart.
|
||||
|
||||
Compare with `subtask-breakdown.ts:211` which correctly sets `projectId: session.projectId ?? null`.
|
||||
|
||||
---
|
||||
|
||||
### 4.2 ⚪ Low — `milestone-slice-interview.ts`: Project ID Not Explicitly Tracked
|
||||
|
||||
**File:** `packages/dashboard/src/milestone-slice-interview.ts`
|
||||
**Severity:** Low
|
||||
**Focus Area:** AI Sessions
|
||||
|
||||
The milestone/slice interview session does not track `projectId` in its session state. This is consistent with mission-interview (where projectId is null) but differs from subtask-breakdown (where projectId is tracked). Whether this is a bug depends on whether mission interviews should be recoverable per-project.
|
||||
|
||||
---
|
||||
|
||||
## 5. CSS / Styling
|
||||
|
||||
### 5.1 🟡 Medium — `--surface-hover` Never Defined in `:root`
|
||||
|
||||
**File:** `packages/dashboard/app/styles.css:4547, 5930, 8602, 22709, 29134` (and 10+ other locations)
|
||||
**Severity:** Medium
|
||||
**Focus Area:** CSS / Styling
|
||||
|
||||
The `--surface-hover` custom property is referenced in 22+ locations but **never defined in `:root`**. All usages rely on fallback values:
|
||||
|
||||
```css
|
||||
background: var(--surface-hover, rgba(0, 0, 0, 0.04)); /* dark mode fallback */
|
||||
background: var(--surface-hover, rgba(0, 0, 0, 0.03)); /* light mode fallback */
|
||||
```
|
||||
|
||||
This is documented in project memory as a known pitfall. The fallback values vary (0.03, 0.04, 0.05), creating inconsistent hover states across the UI. The `--surface-hover` token should be defined in `:root` and all 54 theme blocks to enable theme-aware hover styling.
|
||||
|
||||
---
|
||||
|
||||
### 5.2 🟡 Medium — `.agent-active` Box Shadow Uses `rgba()` Instead of CSS Variables
|
||||
|
||||
**File:** `packages/dashboard/app/styles.css:1440-1462`
|
||||
**Severity:** Medium
|
||||
**Focus Area:** CSS / Styling
|
||||
|
||||
```css
|
||||
.card.agent-active {
|
||||
border-color: var(--in-progress);
|
||||
box-shadow:
|
||||
0 0 8px rgba(var(--in-progress-rgb), 0.4), /* ← rgba() + RGB variable */
|
||||
0 0 20px rgba(var(--in-progress-rgb), 0.15);
|
||||
}
|
||||
@keyframes agent-glow {
|
||||
0%, 100% {
|
||||
box-shadow:
|
||||
0 0 8px rgba(var(--in-progress-rgb), 0.4), /* ← rgba() + RGB variable */
|
||||
0 0 20px rgba(var(--in-progress-rgb), 0.15);
|
||||
}
|
||||
50% {
|
||||
box-shadow:
|
||||
0 0 12px rgba(var(--in-progress-rgb), 0.6), /* ← rgba() + RGB variable */
|
||||
0 0 28px rgba(var(--in-progress-rgb), 0.25);
|
||||
}
|
||||
```
|
||||
|
||||
The `.agent-active` box shadow uses `rgba(var(--in-progress-rgb), X)` which is a valid project pattern for theming (using RGB variables). However, this is inside a component class definition, not in `:root` or theme blocks, meaning the glow effect only works for themes that define `--in-progress-rgb`. If a theme lacks this variable, the glow silently degrades.
|
||||
|
||||
---
|
||||
|
||||
### 5.3 ⚪ Low — `@media (max-width: 768px)` Touch Target Validation
|
||||
|
||||
**File:** `packages/dashboard/app/styles.css:26780+`
|
||||
**Severity:** Low
|
||||
**Focus Area:** CSS / Styling
|
||||
|
||||
Scanning 55 mobile `@media` blocks reveals the `.touch-target` class (min-height: 44px) exists globally at line 27, but many interactive elements within mobile media queries rely on their natural height rather than explicit touch target sizing. The `.btn-icon` class at line 518 sets `height: 28px` and `width: 28px` — below the 36px mobile minimum — and only gets a `touch-target` boost when the `.touch-target` class is also applied.
|
||||
|
||||
---
|
||||
|
||||
## 6. Accessibility
|
||||
|
||||
### 6.1 🟠 High — Icon-Only Buttons Missing `aria-label` (Systematic Pattern)
|
||||
|
||||
**File:** `packages/dashboard/app/components/Header.tsx:601, 689, 703, 716, 874, 893, 916, 938, 950, 962, 974, 997, 1047, 1057, 1078`
|
||||
**Severity:** High
|
||||
**Focus Area:** Accessibility
|
||||
|
||||
Multiple `.btn-icon` buttons in `Header.tsx` use `title` attribute but lack `aria-label`. Screen readers read `title` attributes when present, but this is **not guaranteed by WCAG 2.1 SC 4.1.2** — `aria-label` is the correct mechanism:
|
||||
|
||||
```typescript
|
||||
// Line 601 — node selector trigger
|
||||
<button className={`btn-icon node-selector-trigger${...}`} />
|
||||
// Has title="Switch node" (line 603) but NO aria-label
|
||||
|
||||
// Line 689 — mobile search trigger
|
||||
<button className="btn-icon mobile-search-trigger" />
|
||||
// Has title="Open search" (line 691) but NO aria-label
|
||||
|
||||
// Lines 703, 716 — usage indicator buttons
|
||||
<button className="btn-icon" /> // ← No title, no aria-label
|
||||
```
|
||||
|
||||
Many buttons in `Header.tsx` have neither `title` nor `aria-label`:
|
||||
- Lines 703, 716 (usage-related buttons)
|
||||
- Lines 950, 962, 974 (files, git, workflow actions)
|
||||
- Line 997 (more-actions dropdown trigger)
|
||||
- Lines 1047, 1057 (pause/schedule buttons)
|
||||
|
||||
Per WCAG 2.1 Level A, interactive elements must have accessible names. `title` attributes are not sufficient — they only provide a tooltip, not an accessible name for assistive technology.
|
||||
|
||||
---
|
||||
|
||||
### 6.2 🟡 Medium — Modal Focus Management Not Verified
|
||||
|
||||
**File:** `packages/dashboard/app/components/`
|
||||
**Severity:** Medium
|
||||
**Focus Area:** Accessibility
|
||||
|
||||
No explicit focus trap (locking keyboard focus within modals) was identified in any of the major modal components reviewed (`TaskDetailModal.tsx`, `SettingsModal.tsx`, `PlanningModeModal.tsx`, `MissionInterviewModal.tsx`). The project uses focus management patterns but no `focus-trap` library or custom implementation was found.
|
||||
|
||||
Without a focus trap, keyboard users can tab out of a modal into the background content, breaking the expected modal interaction pattern.
|
||||
|
||||
---
|
||||
|
||||
### 6.3 🟡 Medium — Heading Hierarchy Not Enforced
|
||||
|
||||
**File:** Multiple component files
|
||||
**Severity:** Medium
|
||||
**Focus Area:** Accessibility
|
||||
|
||||
No automated enforcement of heading hierarchy (h1→h2→h3) was identified. Component review showed `<h1>` used in component titles with `<h2>` subsections within the same component, but no consistent heading outline was enforced. This can lead to skipped heading levels that confuse screen reader users navigating by headings.
|
||||
|
||||
---
|
||||
|
||||
## 7. Error Handling
|
||||
|
||||
### 7.1 🔴 Critical — `insights-routes.ts`: Missing `catchHandler` (Already Documented Above)
|
||||
|
||||
See **Finding 1.1**.
|
||||
|
||||
---
|
||||
|
||||
### 7.2 🟠 High — `fetchTaskDetail` Bypass (Already Documented Above)
|
||||
|
||||
See **Finding 3.2**.
|
||||
|
||||
---
|
||||
|
||||
### 7.3 🟡 Medium — Inconsistent Error Response Format in `ai-session-store.ts`
|
||||
|
||||
**File:** `packages/dashboard/src/ai-session-store.ts`
|
||||
**Severity:** Medium
|
||||
**Focus Area:** Error Handling
|
||||
|
||||
The `AiSessionStore` emits events (`ai_session:updated`, `ai_session:deleted`) but has no error event emission. If a database operation fails, the error is silently swallowed (or logged to console). Compare with other stores that emit `"error"` events for exceptional conditions.
|
||||
|
||||
```typescript
|
||||
// ai-session-store.ts:upsert — on DB failure, throws unhandled exception
|
||||
upsert(session: AiSessionRow): void {
|
||||
try {
|
||||
this.db.prepare(...).run(...);
|
||||
} catch {
|
||||
// Only clears thinking timer, then throws — no error event
|
||||
this.clearThinkingTimer(session.id);
|
||||
throw err; // ← unhandled Express error if thrown in route handler
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
If this is called from within an async route handler not wrapped in `catchHandler`, the thrown error propagates to Express's default handler, returning HTML instead of JSON.
|
||||
|
||||
---
|
||||
|
||||
## 8. Security
|
||||
|
||||
### 8.1 🟡 Medium — No CSRF Protection on Mutation Endpoints
|
||||
|
||||
**File:** `packages/dashboard/src/routes.ts` (all mutation routes)
|
||||
**Severity:** Medium
|
||||
**Focus Area:** Security
|
||||
|
||||
No CSRF tokens or double-submit cookie patterns were identified for mutation endpoints (`POST`, `PUT`, `PATCH`, `DELETE`). The API uses Bearer token authentication but lacks CSRF protection for browser-based API calls. This is a known gap — the current mitigation is that Bearer tokens are not stored in cookies (they're stored in `localStorage` and sent via `Authorization` header), which prevents CSRF. However, this should be documented as an architectural decision.
|
||||
|
||||
---
|
||||
|
||||
### 8.2 🟡 Medium — No Content-Security-Policy Headers
|
||||
|
||||
**File:** `packages/dashboard/src/server.ts`
|
||||
**Severity:** Medium
|
||||
**Focus Area:** Security
|
||||
|
||||
No CSP headers are set on responses. The dashboard serves user-generated content (task descriptions, file contents, agent prompts) and no CSP is defined to mitigate XSS risks.
|
||||
|
||||
---
|
||||
|
||||
## 9. Cross-Cutting Code Quality
|
||||
|
||||
### 9.1 ⚪ Low — Widespread `any` Type Usage in AI Session Files
|
||||
|
||||
**File:** `packages/dashboard/src/planning.ts:32`, `mission-interview.ts:29`, `subtask-breakdown.ts`, `milestone-slice-interview.ts`, `chat.ts`, `agent-generation.ts`, `ai-refine.ts`, `roadmap-suggestions.ts`
|
||||
**Severity:** Low
|
||||
**Focus Area:** Code Quality
|
||||
|
||||
All AI session files declare `let createKbAgent: any;` for dynamic engine import. The `AgentResult` type alias in `planning.ts:27` is also `any`. While this is intentional (avoiding direct engine dependency), it weakens type safety for the AI integration layer.
|
||||
|
||||
---
|
||||
|
||||
## 10. Findings Referenced from `.fusion/memory.md` (Known Issues)
|
||||
|
||||
The following issues are documented in project memory and **NOT re-reported as new findings**. This report references them for completeness:
|
||||
|
||||
| Memory Reference | Description | Status |
|
||||
|-----------------|-------------|--------|
|
||||
| FN-1492 / FN-1909 | Express wildcard route ordering: specific routes must precede generic | ✅ Correctly ordered in routes.ts |
|
||||
| FN-1657 | Project-context reset pattern for SSE hooks | ✅ Implemented in useTasks, useAgentLogs, useMultiAgentLogs |
|
||||
| FN-1734 | Polling hook loading contract: `loading` true only for initial fetch | ✅ Implemented in useProjectHealth |
|
||||
| FN-1764 | Context version guard for SSE stale event rejection | ✅ Implemented in multi-agent log hooks |
|
||||
| FN-1535 | Theme URL path joining bug | ✅ Fixed |
|
||||
| FN-1534 | Theme loading edge cases | ✅ Fixed |
|
||||
| FN-1976 | Message SSE store cohesion | ✅ Engine stores are used for SSE listeners |
|
||||
| FN-1269 | Timing-safe webhook signature | ✅ Verified `timingSafeEqual` in github-webhooks.ts |
|
||||
| `--surface-hover` token | Token used but never defined in `:root` | ⚠️ Still present (Finding 5.1) |
|
||||
@@ -1,194 +0,0 @@
|
||||
# Engine Package Review: Findings Report
|
||||
|
||||
**Reviewed:** `packages/engine/src` (~87K lines, 114 files)
|
||||
**Date:** 2026-04-16
|
||||
**Task:** FN-1962
|
||||
|
||||
## Summary
|
||||
|
||||
| Severity | Count |
|
||||
|----------|-------|
|
||||
| Critical | 0 |
|
||||
| High | 3 |
|
||||
| Medium | 3 |
|
||||
| Low | 2 |
|
||||
|
||||
### Findings by subsystem
|
||||
|
||||
| Area | Findings |
|
||||
|------|----------|
|
||||
| TaskExecutor (`executor.ts`) | 1 medium |
|
||||
| Heartbeat system (`agent-heartbeat.ts`) | 1 medium, 1 low |
|
||||
| Concurrency (`concurrency.ts`) | 1 medium, 1 low |
|
||||
| Merge process (`merger.ts`) | 0 |
|
||||
| Scheduler (`scheduler.ts`) | 1 high, 1 low |
|
||||
| Step-session / workflow / plugin runner | 0 |
|
||||
| Remaining subsystems | 2 high |
|
||||
| Cross-cutting | summarized from above |
|
||||
|
||||
---
|
||||
|
||||
## 1. TaskExecutor (`executor.ts`)
|
||||
|
||||
### Medium
|
||||
|
||||
#### M-1 — Executor registers long-lived store listeners but exposes no teardown path
|
||||
- **File:line:** `packages/engine/src/executor.ts:422-450`, `:462-603`, `:606-623`
|
||||
- **Description:** `TaskExecutor` attaches three `TaskStore` listeners in the constructor (`task:moved`, `task:updated`, `settings:updated`) but does not expose `stop()`/`dispose()` to unsubscribe them.
|
||||
- **Impact:** If runtimes are restarted against a shared/external `TaskStore`, stale executor instances can remain listener-reachable and duplicate event handling (double execute triggers, duplicate pause handling, extra steering injections).
|
||||
- **Suggested fix:** Add explicit teardown (`dispose`) that removes registered listeners; invoke it from runtime shutdown before replacing/recreating an executor.
|
||||
|
||||
### Low
|
||||
- No additional high-confidence correctness defects found in the reviewed execution/recovery/workflow code paths.
|
||||
|
||||
---
|
||||
|
||||
## 2. Heartbeat System (`agent-heartbeat.ts`)
|
||||
|
||||
### Medium
|
||||
|
||||
#### M-2 — `agentStartLocks` entries are never removed
|
||||
- **File:line:** `packages/engine/src/agent-heartbeat.ts:281-286`
|
||||
- **Description:** `withAgentStartLock()` stores a promise per agent ID in `agentStartLocks`, but never deletes the key after completion.
|
||||
- **Impact:** Unbounded map growth over time for agents that run at least once; stale promise references retained for process lifetime.
|
||||
- **Suggested fix:** Wrap lock execution in `try/finally` and delete map entry when the stored promise resolves/rejects and is still current for that agent.
|
||||
|
||||
### Low
|
||||
|
||||
#### L-1 — Timer registration can occur while scheduler is stopped; `stop()` early return may skip cleanup
|
||||
- **File:line:** `packages/engine/src/agent-heartbeat.ts:1422-1446`, `:1393-1406`
|
||||
- **Description:** `registerAgent()` always creates a timer regardless of `running` state. `stop()` returns early when `!running`, so pre-start registered timers can survive an attempted stop.
|
||||
- **Impact:** Unexpected idle intervals and avoidable timer leakage in non-standard lifecycle ordering.
|
||||
- **Suggested fix:** Either gate timer creation on `running`, or make `stop()` always clear `timers` regardless of `running` flag.
|
||||
|
||||
---
|
||||
|
||||
## 3. Concurrency (`concurrency.ts`)
|
||||
|
||||
### Medium
|
||||
|
||||
#### M-3 — Semaphore `release()` can underflow active count
|
||||
- **File:line:** `packages/engine/src/concurrency.ts:112-114`
|
||||
- **Description:** `release()` unconditionally decrements `_active`; a double-release can make `_active` negative.
|
||||
- **Impact:** Corrupt semaphore state and potential over-admission (more than configured concurrency).
|
||||
- **Suggested fix:** Guard against underflow (`if (this._active <= 0) return` or throw), and optionally track token ownership for safer release semantics.
|
||||
|
||||
### Low
|
||||
|
||||
#### L-2 — Missing regression tests for release underflow / double-release
|
||||
- **File:line:** `packages/engine/src/concurrency.test.ts:4-275`
|
||||
- **Description:** Existing tests validate priority/FIFO and dynamic limit changes, but no test covers duplicate `release()` behavior.
|
||||
- **Impact:** Underflow regression can reappear undetected.
|
||||
- **Suggested fix:** Add explicit tests asserting `activeCount` never drops below zero and that extra releases are rejected or ignored.
|
||||
|
||||
---
|
||||
|
||||
## 4. Merge Process (`merger.ts`)
|
||||
|
||||
### Critical
|
||||
- None.
|
||||
|
||||
### High
|
||||
- None.
|
||||
|
||||
### Medium
|
||||
- None.
|
||||
|
||||
### Low
|
||||
- No high-confidence defects identified in reviewed merge retry/context-recovery/worktree-cleanup paths.
|
||||
- `execSync` usage appears confined to short git plumbing; user-configured commands run through async `execAsync`.
|
||||
|
||||
---
|
||||
|
||||
## 5. Scheduler (`scheduler.ts`)
|
||||
|
||||
### High
|
||||
|
||||
#### H-1 — Event listeners added in constructor are not removed in `stop()`
|
||||
- **File:line:** `packages/engine/src/scheduler.ts:132-135`, `:146-150`, `:158-162`, `:171-225`, `:231-269`; stop path `:329-346`
|
||||
- **Description:** Scheduler registers five `store.on(...)` listeners but `stop()` only clears interval/aux state; it never unsubscribes listeners.
|
||||
- **Impact:** Stale scheduler instances can continue reacting to store events after stop, causing duplicate side effects and memory retention across runtime restarts.
|
||||
- **Suggested fix:** Store listener function refs as class fields and unregister all of them in `stop()`.
|
||||
|
||||
### Low
|
||||
|
||||
#### L-3 — Test coverage gap for listener teardown behavior
|
||||
- **File:line:** `packages/engine/src/scheduler.test.ts:139-339` and stop invocations around `:499`, `:1662`, `:1764`, `:1774`
|
||||
- **Description:** Tests verify listener registration and scheduling behavior but do not assert listener unsubscription on `stop()`.
|
||||
- **Impact:** Lifecycle leaks are not protected by regression tests.
|
||||
- **Suggested fix:** Add tests asserting `store.off`/`removeListener` calls for each subscribed event during shutdown.
|
||||
|
||||
---
|
||||
|
||||
## 6. Step-Session Executor, Workflow Steps, Plugin Runner
|
||||
|
||||
### Critical
|
||||
- None.
|
||||
|
||||
### High
|
||||
- None.
|
||||
|
||||
### Medium
|
||||
- None.
|
||||
|
||||
### Low
|
||||
- No high-confidence defects found in reviewed session lifecycle, plugin hook timeout isolation, or cache invalidation paths.
|
||||
|
||||
---
|
||||
|
||||
## 7. Remaining Subsystems
|
||||
|
||||
### High
|
||||
|
||||
#### H-2 — InProcessRuntime forwards TaskStore events without unsubscribe on stop
|
||||
- **File:line:** `packages/engine/src/runtimes/in-process-runtime.ts:869-889`; stop path `:578-693`
|
||||
- **Description:** `setupEventForwarding()` attaches `task:created`, `task:moved`, `task:updated` listeners to `TaskStore`; `stop()` does not remove them.
|
||||
- **Impact:** Runtime restart or shared-store scenarios can leave stale forwarding handlers active, duplicating emitted runtime events and retaining stopped runtime objects.
|
||||
- **Suggested fix:** Keep bound handler refs and unregister in `stop()`.
|
||||
|
||||
#### H-3 — HybridExecutor forwards ProjectManager events without cleanup on shutdown
|
||||
- **File:line:** `packages/engine/src/hybrid-executor.ts:394-427`; shutdown path `:359-382`
|
||||
- **Description:** `setupEventForwarding()` registers seven `projectManager.on(...)` listeners. `shutdown()` removes CentralCore listeners but not these forwarding listeners.
|
||||
- **Impact:** Reinitialization can accumulate duplicated forwarding and retained references.
|
||||
- **Suggested fix:** Track forwarded handlers and call `projectManager.off(...)` (or equivalent) during shutdown.
|
||||
|
||||
### Medium
|
||||
- None.
|
||||
|
||||
### Low
|
||||
- No additional high-confidence defects identified in reviewed cron/pi/reviewer/triage/self-healing/mission/runtime-IPC paths beyond cleanup findings above.
|
||||
|
||||
---
|
||||
|
||||
## 8. Cross-Cutting Concerns
|
||||
|
||||
### Resource Cleanup
|
||||
- Multiple lifecycle components register listeners without symmetric teardown (`Scheduler`, `InProcessRuntime`, `HybridExecutor`, and `TaskExecutor` constructor listeners).
|
||||
|
||||
### Error Handling
|
||||
- Generally robust try/catch usage and callback isolation patterns observed; no critical uncaught-flow defects identified in reviewed paths.
|
||||
|
||||
### Race Conditions
|
||||
- Semaphore underflow (`concurrency.ts`) can destabilize concurrency accounting under release misuse.
|
||||
|
||||
### `execSync` Usage
|
||||
- No policy violations found in sampled engine hotspots: `execSync` usage appears limited to git plumbing; user-configured operations use async execution.
|
||||
|
||||
### Memory Leaks
|
||||
- Unbounded `agentStartLocks` map in heartbeat monitor.
|
||||
- Event-listener retention in scheduler/runtime orchestration layers.
|
||||
|
||||
### Event Listener Cleanup
|
||||
- Missing unsubscribe patterns are the dominant architectural issue class in this review.
|
||||
|
||||
### Type Safety
|
||||
- No critical type-safety defects (unsafe cast crashes/null deref) identified in reviewed files.
|
||||
|
||||
---
|
||||
|
||||
## Recommendations (Prioritized)
|
||||
|
||||
1. **Immediate (high):** Implement consistent listener lifecycle management (`on`/`off`) in Scheduler, InProcessRuntime, HybridExecutor, and TaskExecutor.
|
||||
2. **Near-term (medium):** Harden `AgentSemaphore.release()` against underflow and add regression tests.
|
||||
3. **Near-term (medium):** Add lock-map cleanup in `HeartbeatMonitor.withAgentStartLock()`.
|
||||
4. **Quality gates:** Add lifecycle teardown tests for scheduler/runtime listener cleanup to prevent regressions.
|
||||
@@ -1,243 +0,0 @@
|
||||
# Interface Packages Audit — FN-1963
|
||||
|
||||
## Summary
|
||||
Comprehensive read-only audit completed across `packages/cli`, `packages/tui`, `packages/desktop`, `packages/mobile`, and `packages/plugin-sdk`.
|
||||
|
||||
**Total findings: 22**
|
||||
- **Critical:** 1
|
||||
- **High:** 3
|
||||
- **Medium:** 15
|
||||
- **Low:** 3
|
||||
|
||||
The most severe issue is a **desktop API contract split** between preload and renderer codepaths that can disable native desktop integrations (window controls, IPC API transport, deep-link/update hooks). Secondary concerns cluster around lifecycle cleanup (`process.exit` in command helpers, non-disposed stores/listeners) and brittle API boundaries (direct source imports in plugin-sdk, string-based duplicate detection in extension imports).
|
||||
|
||||
## Critical Findings
|
||||
|
||||
### IF-001 — Desktop preload/renderer API contract drift breaks native bridge
|
||||
- **Severity:** critical
|
||||
- **Location:**
|
||||
- `packages/desktop/src/preload.ts:6-43`
|
||||
- `packages/desktop/src/renderer/hooks/useElectron.ts:15-19`
|
||||
- `packages/desktop/src/renderer/components/TitleBar.tsx:18-23`
|
||||
- `packages/desktop/src/renderer/api-electron.ts:141`
|
||||
- `packages/desktop/src/ipc.ts:5-59` (no `api-request` handler)
|
||||
- **Description:** Preload exposes `window.fusionAPI`, but renderer stack checks `window.electronAPI` and expects methods/channels (`windowControl`, `getPlatform`, `invoke("api-request")`, `installUpdate`) that are not exposed/handled.
|
||||
- **Impact:** Desktop-only functionality silently degrades or no-ops (custom title bar controls, Electron transport path, update/deep-link hooks), causing inconsistent behavior between web vs desktop shell.
|
||||
- **Suggested fix:** Define one canonical bridge contract and align all layers (preload typings, renderer hooks/components, IPC handler channels). Add integration tests that boot preload+renderer together and assert bridge method parity.
|
||||
|
||||
## High Findings
|
||||
|
||||
### IF-002 — Daemon token printed in cleartext at startup
|
||||
- **Severity:** high
|
||||
- **Location:** `packages/cli/src/commands/daemon.ts:456-463` (despite `maskToken` at `daemon.ts:108-115`)
|
||||
- **Description:** Startup banner logs full daemon bearer token.
|
||||
- **Impact:** Token exposure in terminal recordings/log aggregation/shell history can allow unauthorized API access.
|
||||
- **Suggested fix:** Mask token by default (show prefix/suffix only), add `--show-token` explicit override if full token display is truly needed.
|
||||
|
||||
### IF-003 — TaskStore lifecycle leaks in settings/backup command paths
|
||||
- **Severity:** high
|
||||
- **Location:**
|
||||
- `packages/cli/src/commands/settings-import.ts:28-119`
|
||||
- `packages/cli/src/commands/settings-export.ts:22-70`
|
||||
- `packages/cli/src/commands/backup.ts:27-54`
|
||||
- **Description:** Commands construct `TaskStore` instances but never close them, and several branches terminate via `process.exit(...)`.
|
||||
- **Impact:** In embedded/in-process usage (tests, extension-hosted command reuse), DB handles can leak and produce lock contention.
|
||||
- **Suggested fix:** Wrap store usage in `try/finally` with `await store.close()`, and return structured results/errors to top-level runner instead of direct exits in helpers.
|
||||
|
||||
### IF-004 — Auto-updater listeners can be registered repeatedly
|
||||
- **Severity:** high
|
||||
- **Location:**
|
||||
- `packages/desktop/src/main.ts:97` (setup at boot)
|
||||
- `packages/desktop/src/ipc.ts:35-38` (setup again on each check)
|
||||
- `packages/desktop/src/native.ts:132-156`
|
||||
- **Description:** `setupAutoUpdater()` registers event handlers each call with no idempotency guard/removal.
|
||||
- **Impact:** Duplicate notifications/events and potential listener leak warnings over long sessions.
|
||||
- **Suggested fix:** Add one-time registration guard (module-level boolean or `autoUpdater.listenerCount(...)` check) and separate "trigger check" from "register listeners".
|
||||
|
||||
## Medium Findings
|
||||
|
||||
### IF-005 — `--port` parsing accepts missing/invalid values as `NaN`
|
||||
- **Severity:** medium
|
||||
- **Location:** `packages/cli/src/bin.ts:317-346`
|
||||
- **Description:** `parseInt(args[pi + 1], 10)` is used without validating value presence/type.
|
||||
- **Impact:** `fn dashboard --port` (no value) or invalid values can cascade into non-user-friendly runtime errors.
|
||||
- **Suggested fix:** Use shared validated parser (`getFlagValueNumber`) and emit explicit usage errors when value missing/invalid.
|
||||
|
||||
### IF-006 — Async disposal callbacks in dashboard are not awaited
|
||||
- **Severity:** medium
|
||||
- **Location:** `packages/cli/src/commands/dashboard.ts:468-479` (with async callbacks added at `dashboard.ts:571-574`)
|
||||
- **Description:** `disposeCallbacks` are typed/consumed as sync; async teardown work is fire-and-forget.
|
||||
- **Impact:** Shutdown order becomes nondeterministic, and cleanup races can occur under signal handling.
|
||||
- **Suggested fix:** Make `dispose` async and await each callback, or enforce sync-only callbacks.
|
||||
|
||||
### IF-007 — `runServe` repeatedly registers process listeners
|
||||
- **Severity:** medium
|
||||
- **Location:** `packages/cli/src/commands/serve.ts:146-188`
|
||||
- **Description:** Diagnostics setup adds `beforeExit`/`exit`/`uncaughtExceptionMonitor`/`unhandledRejection` listeners each invocation without a registration guard.
|
||||
- **Impact:** Duplicate logs and listener growth in programmatic multi-run scenarios.
|
||||
- **Suggested fix:** Mirror dashboard’s `processDiagnosticsRegistered` guard pattern and unregister where appropriate.
|
||||
|
||||
### IF-008 — Extension store cache is cleared without disposing stores
|
||||
- **Severity:** medium
|
||||
- **Location:**
|
||||
- `packages/cli/src/extension.ts:40-49`
|
||||
- `packages/cli/src/extension.ts:1909-1916`
|
||||
- **Description:** `storeCache.clear()` drops references but never closes cached `TaskStore` instances.
|
||||
- **Impact:** Potential open DB handles across session lifecycles and stale connections after project switches.
|
||||
- **Suggested fix:** Iterate cache values on shutdown and call `close()` before clearing.
|
||||
|
||||
### IF-009 — `fn_task_plan` monkey-patches global console and ignores `ctx.cwd`
|
||||
- **Severity:** medium
|
||||
- **Location:** `packages/cli/src/extension.ts:1000-1029`
|
||||
- **Description:** Tool temporarily overrides global `console.log/error`; also calls `runTaskPlan(..., true)` without passing project context/cwd.
|
||||
- **Impact:** Concurrent tool calls can interleave logs/race restore; planning may target wrong project when host cwd differs.
|
||||
- **Suggested fix:** Avoid global console patching (return structured output from planner API) and thread explicit project/cwd into planner path.
|
||||
|
||||
### IF-010 — GitHub import duplicate detection is substring-based
|
||||
- **Severity:** medium
|
||||
- **Location:**
|
||||
- `packages/cli/src/extension.ts:771`
|
||||
- `packages/cli/src/extension.ts:848`
|
||||
- **Description:** Existing import checks use `task.description.includes(sourceUrl)`.
|
||||
- **Impact:** False positives/negatives with partial URL matches or edited descriptions can skip valid imports or duplicate tasks.
|
||||
- **Suggested fix:** Parse canonical `Source:` metadata or persist source URL in a structured task field.
|
||||
|
||||
### IF-011 — `fn_skills_install` child process has no timeout/cancellation strategy
|
||||
- **Severity:** medium
|
||||
- **Location:** `packages/cli/src/extension.ts:1761-1784`
|
||||
- **Description:** `spawn("npx", ...)` waits indefinitely for exit; no timeout or signal handling.
|
||||
- **Impact:** Hung `npx` can stall tool execution indefinitely.
|
||||
- **Suggested fix:** Add timeout with forced termination and user-facing timeout error.
|
||||
|
||||
### IF-012 — `/fn` command state is module-level and session-shared
|
||||
- **Severity:** medium
|
||||
- **Location:** `packages/cli/src/extension.ts:1820-1904`
|
||||
- **Description:** `dashboardProcess`/`dashboardPort` are shared state for command handler instance.
|
||||
- **Impact:** Multiple sessions in same runtime can interfere (status/start/stop collisions).
|
||||
- **Suggested fix:** Scope process state per session/context or maintain keyed process registry.
|
||||
|
||||
### IF-013 — TUI terminal sizing clamps to minimum, masking real narrow widths
|
||||
- **Severity:** medium
|
||||
- **Location:** `packages/tui/src/utils/terminal.ts:65-69`
|
||||
- **Description:** Width/height are forced to min (80x24), then used as effective dimensions.
|
||||
- **Impact:** Very narrow terminals can render as if wider than reality, leading to overflow/wrapping artifacts.
|
||||
- **Suggested fix:** Expose both actual and clamped dimensions; render logic should respect actual bounds and optionally warn/fallback when below minimum.
|
||||
|
||||
### IF-014 — TUI truncation is not display-width aware (Unicode/emoji)
|
||||
- **Severity:** medium
|
||||
- **Location:** `packages/tui/src/utils/truncate.ts:37-55`
|
||||
- **Description:** Uses `string.length`/`slice` for terminal width.
|
||||
- **Impact:** Wide characters and grapheme clusters misalign table columns and produce visual corruption.
|
||||
- **Suggested fix:** Use wcwidth/grapheme-aware measurement/truncation utilities.
|
||||
|
||||
### IF-015 — Number-key routing is duplicated and not focus-guarded in router
|
||||
- **Severity:** medium
|
||||
- **Location:**
|
||||
- `packages/tui/src/components/screen-router.tsx:114-123`
|
||||
- `packages/tui/src/hooks/use-global-shortcuts.tsx:141-146`
|
||||
- `packages/tui/src/index.tsx:65-67,82-85`
|
||||
- **Description:** Screen switching is handled in both router and global shortcuts; router path ignores focus guard.
|
||||
- **Impact:** Double state updates and accidental screen changes while typing in focused inputs.
|
||||
- **Suggested fix:** Centralize screen-switch handling and enforce focus guard in a single input handler path.
|
||||
|
||||
### IF-016 — `useActivityLog` live conversion loses type fidelity and cancellation is ineffective
|
||||
- **Severity:** medium
|
||||
- **Location:** `packages/tui/src/hooks/use-activity-log.ts:76-86,98-110`
|
||||
- **Description:** Live `agent:log` events are coerced to `type: "task:updated"`; async fetch still mutates state after effect cancellation.
|
||||
- **Impact:** Type filtering becomes misleading for live updates; potential set-state-after-unmount warnings.
|
||||
- **Suggested fix:** Preserve/log original event kind (or explicit mapping), and gate state updates by cancellation/version guard.
|
||||
|
||||
### IF-017 — Mobile `initializePlugins` lacks rollback on partial init failure
|
||||
- **Severity:** medium
|
||||
- **Location:** `packages/mobile/src/index.ts:61-104`
|
||||
- **Description:** Managers initialize sequentially; if later init fails, earlier managers remain active.
|
||||
- **Impact:** Partial startup can leave intervals/listeners running without returned references/cleanup coordination.
|
||||
- **Suggested fix:** Add transactional init with rollback (`destroy`) for already-started managers on failure.
|
||||
|
||||
### IF-018 — Push manager `start()` is not idempotent
|
||||
- **Severity:** medium
|
||||
- **Location:** `packages/mobile/src/plugins/push-notifications.ts:82-86` + listener accumulation at `89-98`
|
||||
- **Description:** Repeated `start()` calls re-register listeners without guard.
|
||||
- **Impact:** Duplicate notification events and multiplied side effects.
|
||||
- **Suggested fix:** Track started state and short-circuit or teardown before re-init.
|
||||
|
||||
### IF-019 — Deep-link PWA fallback misses initial hash and leaves encoded IDs in custom-scheme path parsing
|
||||
- **Severity:** medium
|
||||
- **Location:**
|
||||
- `packages/mobile/src/plugins/deep-links.ts:91-118`
|
||||
- `packages/mobile/src/plugins/deep-links.ts:184-192`
|
||||
- **Description:** Handler listens to `hashchange` only (no initial hash consume); custom scheme segments are assigned without decoding.
|
||||
- **Impact:** First-load deeplink can be dropped; encoded task/project IDs can propagate incorrectly.
|
||||
- **Suggested fix:** Invoke bound hash handler once during initialize and decode path segments when constructing payload.
|
||||
|
||||
### IF-020 — Plugin SDK imports core source via relative paths (bypasses package boundary)
|
||||
- **Severity:** medium
|
||||
- **Location:**
|
||||
- `packages/plugin-sdk/src/index.ts:57-61`
|
||||
- `packages/plugin-sdk/src/index.test.ts:3-5`
|
||||
- **Description:** SDK depends on `../../core/src/...` internals rather than stable package entrypoints.
|
||||
- **Impact:** Core refactors can break SDK consumers; publish-time portability is brittle.
|
||||
- **Suggested fix:** Re-export/import from public package exports (`@fusion/core`) and keep SDK isolated from internal source layout.
|
||||
|
||||
### IF-021 — Plugin SDK omits key plugin-store/loader public types
|
||||
- **Severity:** medium
|
||||
- **Location:**
|
||||
- Missing from `packages/plugin-sdk/src/index.ts`
|
||||
- Present in core at `packages/core/src/plugin-store.ts:18-36`, `packages/core/src/plugin-loader.ts:31-40`
|
||||
- **Description:** `PluginStoreEvents`, `PluginRegistrationInput`, `PluginUpdateInput`, `PluginLoaderOptions` are not exposed by SDK.
|
||||
- **Impact:** Plugin/tooling authors must import from core internals, reinforcing boundary violations.
|
||||
- **Suggested fix:** Re-export these types in SDK index and add export-surface tests.
|
||||
|
||||
## Low Findings
|
||||
|
||||
### IF-022 — `definePlugin` helper loses specific subtype inference
|
||||
- **Severity:** low
|
||||
- **Location:** `packages/plugin-sdk/src/index.ts:99-100`
|
||||
- **Description:** Signature is `FusionPlugin -> FusionPlugin` instead of generic identity `<T extends FusionPlugin>(plugin: T) => T`.
|
||||
- **Impact:** Reduced literal-type preservation and weaker IntelliSense in advanced plugin authoring scenarios.
|
||||
- **Suggested fix:** Make helper generic and add TS assertion tests for inferred literal preservation.
|
||||
|
||||
### IF-023 — macOS activate path doesn’t restore existing hidden window
|
||||
- **Severity:** low
|
||||
- **Location:** `packages/desktop/src/main.ts:124-129` with hide-on-close at `main.ts:63-72`
|
||||
- **Description:** `activate` only handles `mainWindow === null`; hidden-but-existing window is not shown.
|
||||
- **Impact:** Dock re-activation can appear unresponsive after hide-to-tray behavior.
|
||||
- **Suggested fix:** On activate, show/focus existing hidden window when present.
|
||||
|
||||
### IF-024 — Utility duplication across CLI commands increases drift risk
|
||||
- **Severity:** low
|
||||
- **Location:**
|
||||
- `packages/cli/src/commands/dashboard.ts:27-47`
|
||||
- `packages/cli/src/commands/serve.ts:51-71`
|
||||
- `packages/cli/src/commands/daemon.ts:49-69`
|
||||
- **Description:** `formatBytes`/`formatUptime` are copy-pasted across modules.
|
||||
- **Impact:** Behavior drift and repeated maintenance effort.
|
||||
- **Suggested fix:** Move shared formatters to a common utility module in CLI package.
|
||||
|
||||
## Package-by-Package Details
|
||||
|
||||
### CLI (`packages/cli`)
|
||||
- IF-002, IF-003, IF-005, IF-006, IF-007, IF-024
|
||||
- Additional consistency note: helper-level `process.exit(...)` usage is pervasive across command modules (`task.ts`, `mission.ts`, `project.ts`, `node.ts`, `settings-import.ts`, `settings-export.ts`, `backup.ts`), making command functions hard to reuse safely outside direct CLI execution.
|
||||
|
||||
### Pi Extension (`packages/cli/src/extension.ts`)
|
||||
- IF-008, IF-009, IF-010, IF-011, IF-012
|
||||
|
||||
### TUI (`packages/tui`)
|
||||
- IF-013, IF-014, IF-015, IF-016
|
||||
|
||||
### Desktop (`packages/desktop`)
|
||||
- IF-001, IF-004, IF-023
|
||||
|
||||
### Mobile (`packages/mobile`)
|
||||
- IF-017, IF-018, IF-019
|
||||
|
||||
### Plugin SDK (`packages/plugin-sdk`)
|
||||
- IF-020, IF-021, IF-022
|
||||
|
||||
### Cross-Cutting
|
||||
- **Import consistency:** IF-001 and IF-020 show boundary drift (desktop bridge naming divergence, plugin-sdk source-relative imports).
|
||||
- **Shared type alignment:** desktop has parallel incompatible API types (`fusionAPI` vs `electronAPI` contracts), and plugin-sdk misses public core plugin lifecycle/store types (IF-001, IF-021).
|
||||
- **Error handling patterns:** CLI/extension mix thrown errors, `process.exit`, and `{ isError: true }` result contracts, creating inconsistent caller behavior.
|
||||
- **Resource cleanup:** recurring lifecycle issues across CLI commands, extension cache disposal, desktop updater listeners, and mobile manager startup rollback/idempotency.
|
||||
- **Test coverage gaps:** plugin-sdk tests focus on runtime identity but do not assert SDK export completeness or boundary integrity (IF-021/IF-022). A single skipped CLI bundle asset test exists at `packages/cli/src/__tests__/bundle-output.test.ts:49`.
|
||||
Reference in New Issue
Block a user