chore: merge deep-agent into main - #76
Conversation
- Remove mpp deployment configuration - Rename email-dispatcher to publisher - Add tool icons and improve card UI with vertical scrolling - Rename "Tool execution" to "Tool" for clearer labeling - Add TodoListRenderer component for in-place todo list handling
The key used message.id || 'tc' as fallback, but toggleExpand and expandedItems.has checks used raw message.id. When message.id was null/undefined, these never matched, breaking expand/collapse behavior. Added stableId variable to ensure consistent fallback across all uses.
Replace hardcoded string check with isWriteTodosResult() helper that uses the TOOL_NAME constant. Ensures consistency and prevents missed updates if the tool name changes.
Add type guard to filter out malformed todo items from backend. Ensures each item has required content and status fields before rendering to prevent blank list items or unexpected behavior.
Add comment explaining that the latest todo state is displayed at the first write_todos position while hiding intermediate updates. This provides a single live-updating todo list rather than scattered snapshots throughout the conversation.
Remove hardcoded analyst, trainer, publisher, and dietician entries to make template more generic. Keep only common tools (ls, read_file, execute) and add comment for customization.
getToolLabel returns empty for standard tools; Subagent suffix remains when configured. Chat and stream views only show the extra label when non-empty. Made-with: Cursor
ChatMessagesView: - Add explicit null return for message type fall-through - Return null before wrapper when renderMessage is null - Move width wrapper inside AIMessageRenderer - Check content before rendering parent wrapper divs StreamEventRenderer: - Guard against empty/undefined tool_calls array - Filter null values from rendered events - Skip wrapper div when all events render as null - Add braces to case blocks for ESLint Ensures no empty divs with spacing are rendered, eliminating blank lines.
Remove subagentNames set and getToolLabel function as they're no longer used after removing tool/subagent labels from the UI. Clean up all imports and usages across ChatMessagesView and StreamEventRenderer.
Each tool call in an event now has a unique key and independent expand/collapse state. Co-authored-by: mimran-khan <mimran-khan@users.noreply.github.com>
Co-authored-by: NP-compete <NP-compete@users.noreply.github.com>
- Add /v1/users/:userId/threads endpoint for fetching thread list - Update history endpoint from /v1/history/:threadId to /v1/users/:userId/history/:threadId - Require userId parameter in gethistoryByThreadId function - Add error handling with proper error messages in ChatContext - Remove debug console.log statements
Previously, the UI showed only one todo list for the entire conversation at the first write_todos position. In multi-turn conversations, this meant users couldn't see the task progression for each individual message. Now each user message gets its own todo card displaying the latest state for that specific turn, making it clear which tasks were completed in response to each question.
- Add feedback modal with thumbs up/down buttons on AI messages - Implement submitFeedback API using trace_id instead of run_id - Update API format to match backend spec (trace_id, name: "user-rating", value) - Add comprehensive trace_id validation at UI, handler, and API layers - Hide feedback buttons when trace_id is missing - Swap Submit/Cancel button positions in modal
- Strike out all todos from previous conversation turns when new user message arrives - Hide pending/in_progress icons for struck out todos, keep completed/cancelled visible - Fix trace_id preservation during token streaming to enable feedback icons in active chat
FEAT: UI improvements: tool icons, todo list rendering
…, feedback UX (#35) * feat: Phase 1 — foundation & architecture Upgrade dependencies, add Redux state management, BFF proxy layer, structured error types, and development tooling. Dependencies (MR-01 to MR-04): - Upgrade @langchain/langgraph-sdk from ^0.0.74 to ^1.9.2 - Add @reduxjs/toolkit, react-redux for state management - Add vitest, @testing-library/react, jsdom for testing - Add ioredis for optional Redis session backing - Add test/test:run/test:coverage scripts State Management (MR-05 to MR-08): - Add Redux store with chats + userSettings slices - Chats slice: full CRUD, streaming state tracking, tool result merging - UserSettings slice: theme, memory, debug mode (persisted to localStorage) - Migrate App, AppLayout, HomePage, ChatPage from ChatContext to Redux - ChatContext remains in tree for gradual removal in Phase 2 BFF Proxy Layer (MR-09 to MR-11): - Add /api/proxy/agent/* routes with Bearer token auth forwarding - SSE stream proxy with Node.js pipeline() for zero-copy streaming - Client disconnect detection via AbortController - Trace ID propagation (X-Trace-ID header) - Agent health check endpoint (/api/health/agent) - One-time token endpoint (/api/auth/generate-one-time-token) - Feedback proxy endpoint (/api/proxy/agent/feedback) - Update agent controller to use Authorization: Bearer (not X-Token) - Stop exposing AGENT_HOST to frontend (appData.apiUrl now empty) - Frontend falls back to /api/proxy/agent/* when apiUrl is empty Infrastructure (MR-12): - Add Redis connection utility with graceful fallback - Add REDIS_HOST/PORT/PASSWORD/TLS to env.template - Add CORS_ORIGIN to env.template - Make session cookie secure in production - Simplify vite dev proxy config New files: - src/frontend/redux/{store,hooks}.ts - src/frontend/redux/slices/{chats,userSettings}.ts - src/frontend/types/errors.ts - src/frontend/utils/errorHandler.ts - src/frontend/services/authenticated-fetch.ts - src/server/router/proxy.router.ts - src/server/utils/redis.ts - vitest.config.ts * fix: Use Aegra threads/search API for chat history loading The frontend was calling GET /v1/threads/:userId which doesn't exist in the Aegra (LangGraph Platform) API, causing 404s and a crash on .map() over the error response. - Replace N+1 getThreadIds + getHistory pattern with single POST /threads/search filtered by user_identity metadata - Return empty array on non-OK responses instead of crashing - Remove stale console.log from ChatContext * feat(proxy): translate UI stream requests to Aegra LangGraph Platform API The proxy now creates threads via POST /threads (idempotent) and streams runs via POST /threads/{thread_id}/runs/stream, translating Aegra's messages-mode SSE events into the {type, content, chunk_id} format the frontend useDataStream hook expects. * fix(chat): route streaming through BFF proxy instead of direct agent calls The fallback apiUrl was 'http://localhost:5002' which bypassed the proxy translation layer entirely. Empty string correctly triggers the relative /api/proxy/agent/* paths through the Fastify BFF. * fix(proxy): stop request.raw.on('close') from aborting stream immediately Fastify parses the POST body before the handler runs, so the request 'close' event fires as soon as the listener is attached — killing the agent fetch within 2ms. Move disconnect detection to reply.raw 'close' which only fires when the SSE connection actually drops. * feat: use Redis as session store to survive container restarts In-memory @fastify/session store lost all sessions on rebuild, forcing users to re-authenticate. Wire buildSessionStore() using the existing ioredis client so sessions persist in Redis. Falls back to in-memory if REDIS_HOST is not set. * fix: refresh OAuth token before agent calls + handle array content blocks Two fixes: 1. ensureFreshToken() now refreshes the access token via SSO plugin when it is expired or within 30s of expiry, before every proxy call to the agent. Prevents stale-token 401s. 2. extractText() handles Gemini-style content blocks where content is [{type:"text", text:"..."}] instead of a plain string. This was causing chunkId:0 (zero translated SSE events) in streaming. * fix: fetch thread state separately since search omits values threads/search doesn't return message values in the Aegra API. After searching for thread IDs, fetch GET /threads/{id}/state for each thread to load messages. Fixes "Chat Not Found" on the chat page after streaming completes. * fix: normalize array content blocks + avoid frozen-object mutations 1. normalizeContent() extracts text from Gemini-style content blocks [{type:"text",text:"..."}] so the UI renders plain strings. 2. combineToolCallandResult() now creates new objects via spread instead of mutating frozen React/SDK objects (fixes "Cannot assign to read only property 'content'" during streaming). 3. useDataStream tool-result handler uses .map() immutably instead of mutating tool_calls on frozen message references. * fix: deep-clone messages entering Redux to prevent frozen-object mutations Messages from React useState and API responses are frozen (Object.freeze). When pushed into Redux state, Immer can't create writable drafts for them. deepClone on setChats and appendMessageToChat ensures all messages in the store are mutable, preventing "Cannot assign to read only property" errors during streaming and tool-result merging. * fix: eliminate remaining frozen-object mutations in streaming and Redux sync Two mutation sites caused "Cannot assign to read only property 'content'": 1. useDataStream.tsx: token streaming appended to last message in-place via `.content += content` on a frozen Redux reference. Now creates a new message object via spread. 2. ChatPage.tsx: transferred Redux messages (frozen by Immer) directly into useState without cloning. Now deep-clones on both directions (Redux -> useState, useState -> Redux dispatch). * fix: send token deltas instead of cumulative content in stream proxy Aegra's messages/partial SSE events contain the FULL message content accumulated so far, not incremental deltas. The proxy was forwarding the full text as token chunks, causing the UI to duplicate content (each token was appended to the growing string, snowballing). Now tracks prevPartial state and computes the delta (new text only) before forwarding to the frontend. * fix: disable stream_subgraphs to prevent duplicate AI responses With stream_subgraphs: true, the same message streams from both the sub-agent and parent graph, producing two copies of every AI response. Removing the flag (defaults to false) gives a single clean stream from the top-level graph only. * fix: stream completion, token extraction, and refresh token forwarding - useDataStream: break outer while loop on [DONE] signal via streamDone flag to prevent UI hanging when TCP close is delayed - proxy.router: extractText now handles plain string elements in Gemini's mixed content arrays (structured object + continuation string) - proxy.router: emit final token delta on messages/complete for plain AI messages to prevent silent content drops - proxy.router: hijack Fastify reply, flush headers, atomic [DONE] write to ensure SSE termination reaches client through proxy layers - proxy.router: forward refresh_token to agent via X-Refresh-Token header - ChatPage: gate setMessages on thread loaded + use chat id as dep key * fix: prevent duplicate AI text in multi-turn tool-call streams messages/complete for AI messages with tool_calls was re-emitting the full text content that messages/partial had already streamed as tokens. Set content to empty string since tokens already delivered the text; only the tool_calls array is new information in that event. * fix: eliminate duplicate AI text in multi-turn tool-call streams Stop emitting token deltas from messages/partial events entirely. Instead, buffer the cumulative text and only emit content at messages/complete time: - AI with tool_calls (intermediate): emit tool_calls only, discard text - AI without tool_calls (final): emit full text in one shot - Tool messages: emit as before This prevents intermediate AI messages in multi-turn agent flows from producing separate visible text bubbles that duplicate the final response. * fix: flush buffered partial text when stream ends When messages/complete never fires for the final AI message (e.g. simple responses with no tool calls), the buffered prevPartial text was silently discarded. Now the proxy flushes any remaining buffered text as a final token chunk before sending [DONE]. * fix: streaming tool calls, auth hardening, and multi-turn follow-up fixes - Stream tool calls in real-time by detecting them in messages/partial events (additional_kwargs.function_call) with deduplication - Reset isStreamingTokensRef between submissions to prevent AI response tokens from appending to human messages on follow-up turns - Show tool call and sub-agent invocation UI in chat history - Harden auth refresh endpoint with null-check and try-catch for 401s - Fix normalizeContent to handle mixed content arrays (string + object) - Use React Router navigate for New Chat SPA routing - Remove stray console.log from production * fix: prevent duplicate AI response in history by not double-emitting token The messages/complete handler for AI messages was sending the full text as a token, while prevPartial already accumulated the same text from messages/partial events. Both got concatenated into one message, causing the response to appear twice in localStorage and on reload. Fix: let messages/complete for AI messages only update prevPartial (same as partials do), so the text is flushed exactly once at stream end. * feat: PatternFly 6 integration, streaming engine, and theme system - Add PatternFly 6 deps (@patternfly/react-core, react-icons, patternfly, chatbot) - Migrate AppLayout to PF Page + Masthead + PageSidebar (responsive toggle) - Migrate Sidebar to PF Nav/NavList/NavItem with SearchInput filtering - Add ThemeToggle (PF Switch wired to Redux) + useThemeSync hook - Add FOUC prevention script in index.html (reads localStorage before paint) - Replace global.css with Red Hat brand tokens + semantic light/dark theme - Build streaming engine: SSEProcessor, StreamingManager, useStreamingAPI hook - Wire ChatPage to new useStreamingAPI (drop-in replacement for useDataStream) - Update ChatMessagesView, InputForm, ErrorBoundary with theme-aware colors - Auto-scroll chat via bottomRef instead of Radix scroll-area query * feat: UI overhaul with Red Hat branding, lazy-load perf fix, and auth token refresh - Redesign homepage, sidebar, chat views with Red Hat brand styling - Add Red Hat logo component, rename branding to Deep Agent - Fix viewport overflow with PatternFly layout overrides - Lazy-load thread state to eliminate slow sequential API calls - Merge API threads with local chats to prevent data loss - Fix auth plugin encapsulation so proxy can refresh expired tokens * fix: auto-send prompt cards, no-response retry UI, and duplicate message prevention - Pass initial prompt via route state so ChatPage auto-sends on navigation from prompt cards - Show "agent didn't respond" UI with retry button when stream ends without an AI response - Retry re-submits existing messages instead of creating duplicates - Guard auto-send against chats that already have messages * feat: complete PatternFly integration — migrate all UI primitives, add dialogs and alerts Replace all Radix UI primitives with PatternFly 6 components: - Button → PF Button (ChatPage, Sidebar, ErrorBoundary, ChatErrorBoundary) - Card/ScrollArea → PF Card/CardBody (ActivityTimeline) - Badge → PF Label (ChatMessagesView) - Add PF Modal for delete-chat confirmation (Sidebar) - Add PF Alert + ExpandableSection in error boundaries - Delete 8 unused Radix/Tailwind wrappers (components/ui/) - Remove @radix-ui/*, class-variance-authority from dependencies (-48 packages) * feat: add toast notification system and delete-all-chats MR-24: Toast notifications via PF AlertGroup (isToast + isLiveRegion) - New Redux slice (toasts) with addToast/removeToast actions - ToastNotifications component with auto-dismiss (6s) and manual close - Wired to ChatPage (send/retry failures), AppLayout (history load failure, delete success) MR-28: Delete all conversations - clearAllChats reducer in chats slice - Delete All button in sidebar footer with PF Modal confirmation - Clears Redux state + localStorage, navigates to home * feat: add sub-agent rendering with dedicated indicator, BFF name rewrite, and sidebar status * feat: add interrupt HITL handler, file artifacts viewer, task progress stepper, and debug mode * feat: add TodoStrip above chat input with live task tracking Parse write_todos tool call args to extract structured todo items and display them in a compact strip above the chat input area, updating in real-time as the agent works through tasks. * fix: hide write_todos from tool call UI since TodoStrip handles it Filter write_todos from chat message tool cards, task progress stepper, and tasks sidebar to avoid redundant display. * fix: only show right sidebar when debug mode is enabled Tasks & Tools and Debug panels now both appear together in the right sidebar, but only when the debug toggle is active. * fix: remove scrollbar from debug sidebar * fix: constrain debug sidebar to viewport height with internal scroll Use self-stretch and overflow-y-auto so the sidebar matches the parent flex row height and scrolls internally instead of overflowing. * fix: split debug sidebar into equal halves for Tasks and Debug * feat: add Personalization & Settings (Phase 4) - Settings page with 4 tabs: Profile, Memories, Custom Rules, Appearance - Personalization Redux slice with localStorage persistence - MemoryList: add/delete/clear memories with info callout - RulesEditor: add/toggle/delete rules with PF Switch - AppearanceSettings: theme card picker (light/dark) - ProfileSection: SSO user info + Danger Zone (delete all chats) - /settings route, Settings link in sidebar footer - ThemeToggle removed from masthead (now in Settings > Appearance) - Delete all chats moved from sidebar to Settings > Profile - StreamingManager + BFF proxy forward memories/rules via configurable * fix: resolve streaming duplication, false no-response, and selector memoization - Emit delta tokens incrementally in BFF proxy instead of flushing entire response at end (fixes text duplication and enables real-time streaming) - Debounce 'agent didn't respond' banner by 1.5s to avoid false alarm during stream end transitions - Memoize selectActiveRules with createSelector to prevent unnecessary re-renders from .filter() creating new array references - Skip thread state hydration for new local chats to avoid 404 race condition with thread creation - Add memories/activeRules to submit callback dependency array to fix stale closure * fix: prevent React StrictMode from aborting streams and 404 on sidebar new chat - Defer StreamingManager cancel in cleanup effect using isActiveRef pattern so StrictMode double-mount does not abort in-flight streams - Pass { newChat: true } in location.state from sidebar New Chat button - Guard thread state hydration for both initialPrompt and newChat flows * fix: remove strikethrough from completed tasks in TodoStrip * fix: prune ghost chats by reconciling localStorage with backend threads Local chats that have messages but no matching backend thread are now removed on load, preventing stale sidebar entries after DB resets. * fix: suppress duplicate text when supervisor echoes sub-agent response Track completed AI message texts and skip delta emission when a new partial is merely a prefix of already-streamed content. Prevents the supervisor's echo from being re-emitted after tool messages reset prevPartial. * fix: move settings to inline gear icon next to user name in sidebar * fix: dedup supervisor echo in hydrated messages and fix stale newChat state - Remove newChat guard from hydration effect — history.state persists across reloads, permanently blocking getThreadState for chats created from the sidebar - Reset hydrating state in cleanup to prevent StrictMode double-mount from stalling hydration - Add deduplicateEcho() to normalizeContent: detects and strips repeated regions (≥80 chars, ≥15% of text) caused by supervisor echoing sub-agent responses in the stored LangGraph checkpoint * feat: resilience & error handling (MRs 55-66) - ErrorRecovery shared component with retry counter, expandable details - Exponential backoff retry in streaming (3 retries, jitter, 30s cap) - ChatErrorBoundary + global ErrorBoundary wired to ErrorRecovery - Session expired modal with auth callback (replaces hard redirect) - BFF returns 401 on token refresh failure (was passing stale tokens) - Rate limiting UI with countdown timer on 429 - Logout flow: POST /auth/logout, Redux clear, localStorage clear - 30s stale stream watchdog (no auto-cancel, surfaces boolean) - MCP status event forwarding + McpStatusPanel component - Stream interrupted detection + beforeunload graceful shutdown * feat: UX polish — feedback, editing, copy, thinking blocks (MRs 67-74) - Feedback buttons (thumbs up/down) on AI messages with Langfuse score storage - Feedback API service capturing trace_id from stream metadata - Per-message feedback state tracking in Redux - Message editing (last human message, re-submit) - Thinking/reasoning blocks in collapsible sections - Copy actions on messages and code blocks - Response latency indicator (time to first token, total duration) - Custom data renderer (tables, JSON, lists) * feat: persist feedback to Postgres + hydrate on history load - Send thread_id, message_id, user_id with feedback POST - Fetch feedback from backend on chat history hydration - BFF forwards query string to agent for GET feedback * feat: navigation, keyboard shortcuts, export, accessibility (MRs 75-80) - Keyboard shortcuts: /, Esc, Ctrl+N, Ctrl+Shift+S, ?, Ctrl+Shift+E - Keyboard shortcuts help dialog (PatternFly Modal) - Export conversation as Markdown/JSON with download - Agent health indicator in sidebar (30s polling) - WCAG 2.1 AA: skip link, focus management, listbox sidebar, focus outlines - ARIA: role=log, aria-live, aria-label on all interactive elements, aria-pressed on feedback, sr-only live region for stream status * fix: remove nav wrapper and nested main breaking PF layout PageSidebar must be direct child of Page sidebar prop — wrapping in <nav> broke the grid. PatternFly Page provides its own <main>, so the extra <main id=main-content> created invalid nesting. Changed to <div>. * fix: BFF health check uses /health not /ok Agent serves health at /health, not /ok. The old /ok path returned 404, causing the sidebar to permanently show "Agent: offline". * fix: PF6+Tailwind button layout, textbox styling, selector memoization - Add global CSS to force PF6 buttons to inline-flex (prevents icon+text stacking) - Restore native form element styles stripped by Tailwind v4 reset - Use PF icon prop on Add/Delete buttons in MemoryList, RulesEditor, ProfileSection - Style chat input and home page textarea to match Shadowbot aesthetic - Fix selectStreamingState returning new object ref on every call - BFF server refactoring and OTEL tracing support * fix: CSP font-src data: URIs, PF6 button text span flex layout - Add 'data:' to font-src CSP to allow PF6 inline woff2 fonts - Add flex layout to .pf-v6-c-button__text so icon+text render inline (PF6 wraps all children in a single __text span, not separate __icon) * fix: separate PF6 overrides from Tailwind to survive CSS purging Tailwind v4's build purges PF6 class selectors from global.css. Move all .pf-v6-* overrides to a standalone patternfly-overrides.css imported after both PF6 and Tailwind, ensuring they survive the build and override PF6's default design tokens (button alignment, layout). * fix: inject PF6 overrides via inline <style> in HTML template @tailwindcss/vite purges all PF6 class selectors regardless of which CSS file they're in. Move the overrides to an inline <style> block in the BFF's HTML template, loaded after template-ui.css, guaranteeing they survive the build and override PF6 defaults. * fix: show copy/feedback buttons only on last AI message per turn Instead of rendering copy, thumbs-up, and thumbs-down on every assistant message, only show them on the final AI message before the next user message (or end of conversation). This reduces visual clutter and makes the interaction clearer. * fix: graceful Redis session degradation on connection drops Session store callbacks now swallow Redis errors and degrade gracefully (empty session / lost write) instead of propagating errors that trigger ERR_HTTP_HEADERS_SENT crashes in Fastify. Also increase retry tolerance so transient drops recover. * fix: wire thread deletion to agent backend Previously deletes were client-only (Redux + localStorage). On reload, loadUserHistory reconciled from /threads/search and re-added the "deleted" threads. Now handleDeleteChat and handleDeleteAllChats call DELETE /threads/:id on the agent so threads are permanently removed server-side. * fix: guard session store callbacks against ERR_HTTP_HEADERS_SENT The @fastify/session callback itself can throw ERR_HTTP_HEADERS_SENT synchronously when invoked after the reply is already committed. Wrapping all cb() calls in try/catch prevents unhandled rejections that crash-loop the UI container. * fix: resolve pending tool calls when stream completes Tool cards showed spinners forever when mergeToolResult failed to match IDs (e.g. tool_call_id vs tool_calls[].id mismatch). Now when the stream finishes, resolveAllPendingToolCalls marks any tool call still missing content as complete, clearing stuck spinners. * fix: eagerly connect Redis before server accepts requests Previously lazyConnect deferred the TCP connection until the first session operation, causing ERR_HTTP_HEADERS_SENT on early requests. Now connectRedis() is awaited during setupServer() so the connection is established before the server starts listening. * fix: mark todos as completed when stream finishes TodoStrip now accepts isLoading and treats all in_progress/pending todos as completed once the stream is done. This prevents stuck spinners when the agent forgets to emit a final write_todos call. * fix: remove dead FeedbackModal and radix dialog components FeedbackModal was never imported anywhere and depended on missing ui/button, ui/textarea modules and uninstalled @radix-ui/react-dialog. This broke tsc -b and prevented make local / make dev from working.
Support org-scoped path prefixes on a shared platform host, populate session from gateway X-Auth headers when AUTH_ENABLED=false, and centralize prefixed route and API URL construction for chat, health, streaming, and MCP proxying.
Replaces raw APP_DATA.basePath access with the shared path utility and standardizes globalThis usage.
fix(ui): path-scoped agent UI with gateway SSO passthrough
new Date("").toISOString() throws RangeError: Invalid time value.
This occurred in two places in AppLayout:
1. Loading localStorage chats — if the stored timestamp was a Date
object (from chatStorage) or an unparseable value, the conversion
to ISO string would throw.
2. Building sidebar chat items — backendTimestampMap defaulted missing
updatedAt to empty string "". new Date("") creates an Invalid Date
that throws on any formatting call.
Fix: introduce toSafeDate/toSafeISOString helpers that validate the
Date before calling toISOString(), falling back to Date.now(). Also
filter out threads without updatedAt instead of mapping them to "".
…rangerror fix(ui): guard against invalid timestamps causing RangeError
When AUTH_ENABLED=false, the auth-check plugin reads gateway headers and populates request.session so ensureFreshTokens() can forward tokens to the agent API. It was only registered on clientRoutes, causing 401s on /api/proxy/agent/* calls.
Fix: register auth-check plugin on proxy routes
- Add workflow to build and push UI image to ghcr.io - Triggers on push to main/deep-agent-conf-ext branches and version tags - Builds multi-platform image (amd64, arm64) Template-UI has no per-agent customization, so one base image serves all agents. No config volume mount needed. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…/api/health/agent`
Signed-off-by: Anish701 <anish2sinha@gmail.com>
Signed-off-by: Anish701 <anish2sinha@gmail.com>
Signed-off-by: Anish701 <anish2sinha@gmail.com>
…governance Align CI workflows with template-agent standards: - Trivy vulnerability scanning on container images (block on CRITICAL, report HIGH) - Cosign keyless image signing with signature verification - SBOM generation (CycloneDX) and build provenance attestation - Dependency review on PRs, license compliance scanning - PR governance: draft check, conventional commit titles, linked issue requirement - CODEOWNERS for template-ui-maintainers, PR and issue templates - Dependabot for npm/Docker/GitHub Actions, release-please for automated releases - OpenSSF Scorecard analysis - Pin all actions to SHA digests, add job timeouts, concurrency control - Update concurrently and OPA to resolve critical CVEs - Update Containerfile base image Closes #87 Signed-off-by: Soham Dutta <19648293+NP-compete@users.noreply.github.com> Signed-off-by: Anish701 <anish2sinha@gmail.com>
Signed-off-by: Anish701 <anish2sinha@gmail.com>
Signed-off-by: Anish701 <anish2sinha@gmail.com>
Signed-off-by: Anish701 <anish2sinha@gmail.com>
…ponses Signed-off-by: Anish701 <anish2sinha@gmail.com>
Signed-off-by: Anish701 <anish2sinha@gmail.com>
Signed-off-by: Anish701 <anish2sinha@gmail.com>
This reverts commit 6f75220. Signed-off-by: Anish701 <anish2sinha@gmail.com>
Signed-off-by: Anish701 <anish2sinha@gmail.com>
📝 WalkthroughWalkthroughThe PR adds a deep-agent application platform with runtime configuration, Redux state, streaming and HITL workflows, PatternFly UI, server proxying, authentication, OPA, Redis sessions, deployment resources, CI workflows, and automated tests. ChangesApplication platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Anish701 <anish2sinha@gmail.com>
Signed-off-by: Anish701 <anish2sinha@gmail.com>
Signed-off-by: Anish701 <anish2sinha@gmail.com>
Keep deep-agent's detailed /version endpoint over main's simpler one. Signed-off-by: Soham Dutta <sodutta@redhat.com> Signed-off-by: Soham Dutta <19648293+NP-compete@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
1-77: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd an explicit
permissions:block.No job in this workflow needs more than read access to repository contents. Without a
permissions:key, theGITHUB_TOKENuses the repository or organization default, which can be broader than necessary. Add a workflow-levelpermissions: contents: readto apply least privilege across all jobs.🔒️ Proposed fix
name: CI +permissions: + contents: read + on: push: branches: [main, deep-agent]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 1 - 77, Add a workflow-level permissions block near the top-level name/on configuration, granting only read access to repository contents with contents: read. Apply this setting across all jobs without changing the existing job steps or other permissions.Source: Linters/SAST tools
🟡 Minor comments (43)
.github/workflows/ci.yml-14-14 (1)
14-14: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSet
persist-credentials: falseonactions/checkoutsteps that don't need git push access. None of these jobs push commits or tags, so the checked-out credential should not persist in the local git config for the rest of the job.actions/checkoutdefaults topersist-credentials: true, which leaves theGITHUB_TOKENaccessible to subsequent steps, including third-party code executed bynpm ci/npm run.
.github/workflows/ci.yml#L14-L14: addwith: persist-credentials: falseto the checkout in thelintjob..github/workflows/ci.yml#L26-L26: addwith: persist-credentials: falseto the checkout in theunit-testjob..github/workflows/ci.yml#L38-L38: addwith: persist-credentials: falseto the checkout in thebuildjob..github/workflows/ci.yml#L60-L60: addwith: persist-credentials: falseto the checkout in thee2ejob..github/workflows/build-base-image.yml#L35-L36: addwith: persist-credentials: falseto the "Checkout code" step..github/workflows/dependency-review.yml#L18-L19: addwith: persist-credentials: falseto the "Checkout code" step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 14, Set persist-credentials to false on every actions/checkout step in the lint, unit-test, build, and e2e jobs at .github/workflows/ci.yml lines 14, 26, 38, and 60, plus the “Checkout code” steps at .github/workflows/build-base-image.yml lines 35-36 and .github/workflows/dependency-review.yml lines 18-19. No checkout step requires push access.Source: Linters/SAST tools
Makefile-34-39 (1)
34-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not terminate unrelated developer processes.
Lines 37 and 38 match every current-user process whose command line matches
node.*dist/serverornpm.*start.make cleancan stop another checkout or service. Track the PID started bymake local, then terminate only that PID. Otherwise, remove thesepkillcommands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 34 - 39, Update the clean target to avoid terminating unrelated developer processes: remove the broad pkill commands, or replace them with termination of only the specific process PID recorded when make local starts the service. Preserve cleanup of containers, volumes, node_modules, and dist.README.md-102-104 (1)
102-104: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language identifier to the validation-error block.
Line 102 triggers markdownlint rule MD040. Mark the output as text.
Proposed fix
-``` +```text Config validation error: branding.colors.light.primary must be a valid hex color (got 'not-a-color')</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@README.mdaround lines 102 - 104, Update the validation-error fenced code
block in README.md to specify the text language identifier, changing the opening
fence to ```text while preserving the existing error output.</details> <!-- cr-comment:v1:251974e52f4714393c1a8f33 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>src/server/utils/settings.ts-220-245 (1)</summary><blockquote> `220-245`: _🔒 Security & Privacy_ | _🟡 Minor_ | _⚡ Quick win_ **Reject prototype keys in `deepMerge`.** `result[key] = oVal` assigns any key that appears in the parsed YAML. `js-yaml` keeps `__proto__` as a mapping key, so a config file that contains a `__proto__` mapping pollutes `Object.prototype` through this assignment. The config file is operator-supplied, so this is a hardening gap rather than a remote attack path, but the guard costs one line. <details> <summary>🔒 Proposed fix</summary> ```diff const result = { ...base } as Record<string, unknown>; for (const key of Object.keys(override)) { + if (key === "__proto__" || key === "constructor" || key === "prototype") continue; const bVal = result[key]; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/utils/settings.ts` around lines 220 - 245, Update deepMerge to reject prototype-related keys, especially __proto__, before reading or assigning override entries. Keep normal configuration keys and recursive object merging unchanged, while ensuring rejected keys cannot reach result[key] or mutate Object.prototype. ``` </details> <!-- cr-comment:v1:6a987e4649aebc671ff47e63 --> </blockquote></details> <details> <summary>src/server/router/client.router.ts-73-79 (1)</summary><blockquote> `73-79`: _🔒 Security & Privacy_ | _🟡 Minor_ | _⚡ Quick win_ **Escape `agentName` before you interpolate it into the HTML.** `getAgentName()` in `src/server/utils/settings.ts` (Line 539-557) reads `name` from the agent `/info` response. The value goes into `<title>` without escaping. An agent that returns `</title><img src=x onerror=...>` injects markup into every page. The CSP `script_src: ["'self'"]` blocks inline scripts, so the impact is markup injection rather than script execution, but the value still needs escaping. <details> <summary>🔒 Proposed fix</summary> ```diff +const escapeHtml = (s: string) => + s.replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">") + .replace(/"/g, """).replace(/'/g, "&`#39`;"); + ``` Then use it in the template: ```diff - <title>${agentName}</title> + <title>${escapeHtml(agentName)}</title> ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/router/client.router.ts` around lines 73 - 79, Escape the value returned by getAgentName before interpolating agentName into the HTML template in the client router response. Use the project’s existing HTML-escaping utility if available, and preserve the escaped value in the title while leaving the surrounding page markup unchanged. ``` </details> <!-- cr-comment:v1:151ef33e899f961b0d887f67 --> </blockquote></details> <details> <summary>src/server/utils/config-edge-cases.test.ts-86-95 (1)</summary><blockquote> `86-95`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Test the missing implicit config path** With `UI_CONFIG_PATH` unset, `getSettings()` loads and merges `config/ui/settings.yaml`. This test therefore checks the shipped YAML values, not the missing-file fallback. Rename the test or inject/mock the default path so it uses a nonexistent temporary file. Do not set `UI_CONFIG_PATH`; missing explicit paths throw. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/utils/config-edge-cases.test.ts` around lines 86 - 95, Update the test around getSettings so it genuinely exercises the missing implicit config path: keep UI_CONFIG_PATH unset and inject or mock the default config path to a nonexistent temporary file. Rename the test to reflect the scenario, while preserving the existing default-value assertions; do not set UI_CONFIG_PATH because missing explicit paths must still throw. ``` </details> <!-- cr-comment:v1:2f397cfe2b1cc2bb7a826b22 --> </blockquote></details> <details> <summary>src/server/plugins/trace.plugin.ts-23-28 (1)</summary><blockquote> `23-28`: _🔒 Security & Privacy_ | _🟡 Minor_ | _⚡ Quick win_ **Validate the `x-trace-id` fallback value.** The `traceparent` path is validated by `TRACEPARENT_RE`, but the `x-trace-id` path accepts any non-empty string. The value is stored on the request, written to every response header, and reflected into logs. Restrict it to a safe charset and a maximum length, and generate a UUID otherwise. <details> <summary>🛡️ Proposed fix</summary> ```diff if (!traceId) { const xt = request.headers["x-trace-id"]; - traceId = - typeof xt === "string" && xt.length > 0 ? xt : randomUUID(); + traceId = + typeof xt === "string" && /^[\w.-]{1,64}$/.test(xt) + ? xt + : randomUUID(); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/plugins/trace.plugin.ts` around lines 23 - 28, Update the x-trace-id fallback logic in the trace plugin around request.headers["x-trace-id"] to validate incoming values against a safe character set and maximum length before accepting them. Reuse the existing randomUUID fallback for missing or invalid values, while preserving the current traceparent validation path and subsequent request-header assignment. ``` </details> <!-- cr-comment:v1:3a798b90d202185f9d823d7b --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>src/server/server.ts-163-203 (1)</summary><blockquote> `163-203`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Update the comparison baseline after each reload.** `cfg` is captured once, before the watcher starts. `watchConfig` calls `resetSettings()` and `getSettings()`, so `newSettings` is a new object on every reload, but `cfg` still holds the original snapshot. After the second reload, the diff is computed against the original configuration instead of the previous one. The watcher then reports settings as changed that did not change in that reload, and it misses a value that was reverted. <details> <summary>🐛 Proposed fix</summary> ```diff export function startConfigWatcher(configPath: string, server: FastifyInstance) { - const cfg = getSettings(); + let cfg = getSettings(); const cleanup = watchConfig(configPath, (newSettings) => { server.log.info('[ConfigWatcher] Settings reloaded'); @@ if (autoApplied.length > 0) { server.log.info({ settings: autoApplied }, '[ConfigWatcher] Settings applied without restart:'); } + + cfg = newSettings; }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/server.ts` around lines 163 - 203, Update startConfigWatcher so its comparison baseline is mutable and replaced with newSettings after each watchConfig reload. Perform all restartRequired and autoApplied comparisons against the previous snapshot, then assign the reloaded settings as the baseline before the callback completes, preserving accurate change and revert detection across successive reloads. ``` </details> <!-- cr-comment:v1:cc38e4df5d9ae2e88dcd453c --> </blockquote></details> <details> <summary>src/server/utils/opa.ts-108-111 (1)</summary><blockquote> `108-111`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Handle non-string policy results.** `String(d)` produces `"[object Object]"` when a Rego rule yields an object, for example `{"msg": "..."}`. The violation list is returned to clients by `/config/compliance`. Extract a message field when the entry is an object. <details> <summary>🐛 Proposed fix</summary> ```diff - return denials.map((d) => ({ message: String(d) })); + return denials.map((d) => ({ + message: + typeof d === "string" + ? d + : typeof (d as { msg?: unknown })?.msg === "string" + ? (d as { msg: string }).msg + : JSON.stringify(d), + })); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/utils/opa.ts` around lines 108 - 111, Update the denial mapping in the OPA result handling to extract the message field from object entries before converting values to strings, so objects such as {"msg":"..."} return their meaningful message rather than "[object Object]". Preserve string handling for primitive entries and the existing response shape used by /config/compliance. ``` </details> <!-- cr-comment:v1:9c0a3571ce0fa36a35f3bd90 --> </blockquote></details> <details> <summary>src/frontend/components/InterruptBanner.tsx-136-141 (1)</summary><blockquote> `136-141`: _🩺 Stability & Availability_ | _🟡 Minor_ | _⚡ Quick win_ **Report a blocked popup to the user.** `window.open` returns `null` when the browser blocks the popup. The code ignores the return value, so `connecting` clears and no error appears. The user sees an unchanged `Authenticate` button and has no explanation. <details> <summary>🐛 Proposed fix</summary> ```diff - window.open(body.authorize_url, 'mcp-oauth', 'width=600,height=700'); + const popup = window.open(body.authorize_url, 'mcp-oauth', 'width=600,height=700'); + if (!popup) { + throw new Error('The browser blocked the authentication window. Allow popups and try again.'); + } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/InterruptBanner.tsx` around lines 136 - 141, Update the popup-opening flow in InterruptBanner to capture the return value of window.open and treat a null result as a connection failure. Set the same user-visible connect error used by the catch path before clearing connecting, while preserving successful popup behavior. ``` </details> <!-- cr-comment:v1:7b155646cce37eadaa102c54 --> </blockquote></details> <details> <summary>src/frontend/hooks/useDataStream.tsx-209-213 (1)</summary><blockquote> `209-213`: _🩺 Stability & Availability_ | _🟡 Minor_ | _⚡ Quick win_ **Guard the last-message update against an empty or non-AI message list.** `isStreamingTokensRef.current` stays `true` for the rest of the token stream. `setMessages` is returned from the hook at Line 244, so a consumer can replace the message list while the stream runs. If the list becomes empty, `last` is `undefined` and `last.content` throws. If the last entry is a human message, the token text is appended to the wrong message. <details> <summary>🐛 Proposed fix</summary> ```diff setMessages(prev => { const last = prev[prev.length - 1]; + if (!last || last.type !== 'ai') return prev; const updated = { ...last, content: ((last.content as string) || '') + content }; return [...prev.slice(0, -1), updated]; }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/hooks/useDataStream.tsx` around lines 209 - 213, Update the setMessages callback in useDataStream to handle an empty list and verify the last message is an AI message before appending streamed content. Return the existing message list unchanged when no valid AI message is present; otherwise preserve the current append behavior. ``` </details> <!-- cr-comment:v1:6af79888017e6800e3552580 --> </blockquote></details> <details> <summary>src/frontend/services/agent-rest.ts-131-143 (1)</summary><blockquote> `131-143`: _🩺 Stability & Availability_ | _🟡 Minor_ | _⚡ Quick win_ **`response.json()` sits outside the `try` block, so a malformed body rejects the promise.** The doc comment and the `catch` at Line 127 state that request failures and invalid payloads return an empty list. The `try` covers only the `authenticatedFetch` call. If the endpoint returns HTTP 200 with a non-JSON body, `response.json()` throws and the rejection propagates to the caller in `src/frontend/components/layout/AppLayout.tsx` at Line 119. The behavior then differs from `getThreadState`, which wraps the parse. <details> <summary>🐛 Proposed fix</summary> ```diff if (!response.ok) return []; - const results = await response.json(); - if (!Array.isArray(results)) return []; + let results: unknown; + try { + results = await response.json(); + } catch { + return []; + } + if (!Array.isArray(results)) return []; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/services/agent-rest.ts` around lines 131 - 143, Move the response parsing and result validation in the thread-fetching method into the existing try block that currently handles authenticatedFetch, so response.json() failures are caught and return an empty list through the existing catch path. Preserve the current filtering and mapping behavior for valid array payloads, matching getThreadState’s error-handling behavior. ``` </details> <!-- cr-comment:v1:7fd3e55ee98b454614821fba --> </blockquote></details> <details> <summary>e2e/accessibility/wcag.spec.ts-158-158 (1)</summary><blockquote> `158-158`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Avoid the undeclared `axe-core` type import.** Declare `axe-core` in `devDependencies`, or derive the type from `AxeBuilder['analyze']`. The current import can resolve to an unrelated hoisted version or fail under strict dependency layouts. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/accessibility/wcag.spec.ts` at line 158, Update formatViolations to avoid the undeclared axe-core type import by deriving the violation type from AxeBuilder['analyze'] or by declaring axe-core as a devDependency; prefer the existing AxeBuilder type path and preserve the function’s current behavior. ``` </details> <!-- cr-comment:v1:4ec3590816a9ee2617a855a2 --> </blockquote></details> <details> <summary>e2e/chaos/resilience.spec.ts-257-269 (1)</summary><blockquote> `257-269`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Pass an explicit argument before the timeout options.** `page.waitForFunction` takes `arg` as its second parameter and `options` as its third. Add `undefined` before the options object in both calls at Lines 257-269 and 388-393. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/chaos/resilience.spec.ts` around lines 257 - 269, Update both page.waitForFunction calls in the resilience test to pass an explicit undefined argument before the timeout options object, preserving the existing predicate and timeout configuration. ``` </details> <!-- cr-comment:v1:087f35bc39f86497574c4fc9 --> </blockquote></details> <details> <summary>src/frontend/hooks/useStreamingAPI.ts-596-611 (1)</summary><blockquote> `596-611`: _🔒 Security & Privacy_ | _🟡 Minor_ | _⚡ Quick win_ **Handle empty `action_requests` interrupts** When `action_requests` is empty, `ChatPage.tsx` returns before resuming, and `AIMessageRenderer` renders no approval controls. Clear or dismiss the pending interrupt, or resume it explicitly, so the run does not remain pending. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/hooks/useStreamingAPI.ts` around lines 596 - 611, Update checkAndAutoApprove to explicitly handle an empty interruptValue.action_requests array by clearing or dismissing the pending interrupt, or resuming it immediately. Ensure this path does not leave the run pending while preserving the existing approval decision behavior for non-empty requests. ``` </details> <!-- cr-comment:v1:739a02635bcb5da527949232 --> </blockquote></details> <details> <summary>src/frontend/components/InterruptBanner.test.tsx-134-141 (1)</summary><blockquote> `134-141`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Unstub globals after each MCP auth test.** `vi.restoreAllMocks()` does not undo `vi.stubGlobal()`, so `fetch` and `open` remain replaced after this block. Add `vi.unstubAllGlobals()` to `afterEach`; each `beforeEach` creates new `vi.fn()` values, so queued implementations do not carry between these tests. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/InterruptBanner.test.tsx` around lines 134 - 141, Update the afterEach cleanup in the InterruptBanner test setup to call vi.unstubAllGlobals() alongside vi.restoreAllMocks(), ensuring the fetch and open stubs created by beforeEach do not leak between MCP auth tests. ``` </details> <!-- cr-comment:v1:b766b7b79f532f9153a32f26 --> </blockquote></details> <details> <summary>src/frontend/services/export-chat.ts-35-45 (1)</summary><blockquote> `35-45`: _🩺 Stability & Availability_ | _🟡 Minor_ | _⚡ Quick win_ **Defer `URL.revokeObjectURL` until after the download starts.** Line 44 revokes the blob URL in the same synchronous task as `a.click()`. Some browsers (notably Safari) then cancel the download because the URL is already invalid when they fetch it. Revoke on a later task instead. <details> <summary>🛠 Proposed fix</summary> ```diff document.body.appendChild(a); a.click(); a.remove(); - URL.revokeObjectURL(url); + setTimeout(() => URL.revokeObjectURL(url), 0); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/services/export-chat.ts` around lines 35 - 45, Update downloadFile so URL.revokeObjectURL runs in a later task after a.click(), rather than synchronously in the same task; preserve the existing blob URL creation, anchor download setup, click, and cleanup flow. ``` </details> <!-- cr-comment:v1:efc24d0f1b888b39d518ab8a --> </blockquote></details> <details> <summary>src/frontend/components/ReconnectingBanner.tsx-19-23 (1)</summary><blockquote> `19-23`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Add a polite live region to `ReconnectingBanner`.** `Alert` does not create a live region by default. Add `role="status"` and `aria-live="polite"` so assistive technology announces reconnect updates. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/ReconnectingBanner.tsx` around lines 19 - 23, Add role="status" and aria-live="polite" to the Alert element returned by ReconnectingBanner, preserving the existing warning variant, title, and message rendering. ``` </details> <!-- cr-comment:v1:eac32fc662fb1515f35eb07d --> </blockquote></details> <details> <summary>src/frontend/services/feedback-api.ts-37-42 (1)</summary><blockquote> `37-42`: _🔒 Security & Privacy_ | _🟡 Minor_ | _⚡ Quick win_ **Keep `user_id` out of the query string.** The proxy logs the complete `agentUrl`, including `user_id`. Pass the identifier in a header or redact it before logging the URL. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/services/feedback-api.ts` around lines 37 - 42, Update the feedback request in the fetch call to remove user_id from the query string; pass the identifier through an appropriate request header instead, while preserving the encoded threadId URL and existing credentials behavior. ``` </details> <!-- cr-comment:v1:931fb1c580c7ccdd7dc92588 --> </blockquote></details> <details> <summary>src/frontend/components/ErrorRecovery.tsx-133-144 (1)</summary><blockquote> `133-144`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Drop the `aria-label` that hides the visible retry label.** `aria-label="Retry operation"` replaces the accessible name. When `retryLabel` is `Retry (attempt 2/3)`, screen readers and voice control still get "Retry operation", so the attempt counter is never announced and the spoken name does not match the visible text. The visible text alone is a sufficient accessible name. <details> <summary>♿ Proposed fix</summary> ```diff <Button variant={retryVariant} onClick={onRetry} isDisabled={maxRetriesReached || isRetrying} isLoading={isRetrying} spinnerAriaValueText="Retrying" - aria-label="Retry operation" > {retryLabel} </Button> ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/ErrorRecovery.tsx` around lines 133 - 144, Remove the aria-label prop from the retry Button in ErrorRecovery’s onRetry rendering so its accessible name comes from the visible retryLabel, preserving attempt-specific text for screen readers and voice control. ``` </details> <!-- cr-comment:v1:cb6a8f5b6fcc18f8811e3eff --> </blockquote></details> <details> <summary>src/frontend/components/layout/AppLayout.tsx-361-378 (1)</summary><blockquote> `361-378`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Use a `main` landmark for the skip-link target.** `global.css` already makes `.skip-to-main` visible on focus. Do not add `focus:not-sr-only`. Replace the `div#main-content` with `main#main-content`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/layout/AppLayout.tsx` around lines 361 - 378, Replace the `div` with id `main-content` inside `AppLayout`’s `ErrorBoundary` with a `main` element, preserving its existing classes and children so the skip link targets a semantic main landmark. Do not add `focus:not-sr-only` to the skip-link anchor. ``` </details> <!-- cr-comment:v1:43d9c441e9925c60b78cec5c --> </blockquote></details> <details> <summary>src/frontend/global.css-198-205 (1)</summary><blockquote> `198-205`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Resolve the Stylelint keyframe naming errors.** Stylelint reports `keyframes-name-pattern` errors for `slideInRight` (line 198) and `pulseGlow` (line 202). The names use camelCase, and the rule expects kebab-case. The pre-existing keyframes in this file use camelCase too, so pick one resolution and apply it consistently: - Rename the keyframes to `slide-in-right` and `pulse-glow`, and update the class definitions at lines 219-220. Rename the pre-existing camelCase keyframes as well. - Or relax `keyframes-name-pattern` in the Stylelint configuration. Otherwise the lint job fails. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/global.css` around lines 198 - 205, Resolve the keyframes-name-pattern violations by standardizing all keyframe identifiers in global.css to kebab-case, including slideInRight and pulseGlow and the pre-existing camelCase keyframes. Update every animation or animation-name reference in the related class definitions and elsewhere in the file to use the renamed identifiers. ``` </details> <!-- cr-comment:v1:a352c4ce57242f1003daeb0c --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>src/frontend/global.css-171-177 (1)</summary><blockquote> `171-177`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Include the remaining input types in the focus rule.** The reset restoration at lines 151-156 covers `text`, `search`, `email`, `password`, and `url` inputs. The focus rule at lines 171-173 covers only `textarea`, `text`, and `search`. Email, password, and URL inputs therefore keep the unfocused border and box shadow. <details> <summary>🎨 Proposed fix</summary> ```diff textarea:focus, input[type="text"]:focus, -input[type="search"]:focus { +input[type="search"]:focus, +input[type="email"]:focus, +input[type="password"]:focus, +input[type="url"]:focus { border-color: var(--primary); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/global.css` around lines 171 - 177, Extend the focus selector list in the global focus rule to include input[type="email"], input[type="password"], and input[type="url"], preserving the existing border-color, box-shadow, and outline declarations. ``` </details> <!-- cr-comment:v1:a9a8ff26430cd77e5b18ec1f --> </blockquote></details> <details> <summary>src/components/AnnouncementBanner.tsx-60-65 (1)</summary><blockquote> `60-65`: _🩺 Stability & Availability_ | _🟡 Minor_ | _⚡ Quick win_ **Guard the `sessionStorage.setItem` call.** `sessionStorage.setItem` throws when storage is unavailable or the quota is exceeded. The exception escapes the click handler, so `setDismissed(true)` never runs and the banner stays visible. The read path at line 47 is already inside a `try` block. Apply the same protection here. <details> <summary>🛡️ Proposed fix</summary> ```diff const handleClose = useCallback(() => { - if (payload?.message) { - sessionStorage.setItem(STORAGE_PREFIX + messageHash(payload.message), '1'); - } + if (payload?.message) { + try { + sessionStorage.setItem(STORAGE_PREFIX + messageHash(payload.message), '1'); + } catch { + // storage unavailable; dismiss for this render only + } + } setDismissed(true); }, [payload?.message]); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AnnouncementBanner.tsx` around lines 60 - 65, Wrap the sessionStorage.setItem call in handleClose with try/catch so storage failures do not escape the click handler; keep setDismissed(true) outside the guarded operation so the banner is dismissed whether storage is available or not. ``` </details> <!-- cr-comment:v1:a47895c3657c32ee92856eb1 --> </blockquote></details> <details> <summary>index.html-9-20 (1)</summary><blockquote> `9-20`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Default to dark when `theme` is missing from stored settings.** If `template-ui-settings` contains `{}`, `JSON.parse(stored).theme` is `undefined`. The script removes the dark classes, while the application defaults to dark. Use a dark fallback for missing `theme`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.html` around lines 9 - 20, Update the theme initialization around localStorage key template-ui-settings so a missing or undefined parsed theme defaults to 'dark', preserving explicitly stored non-dark themes and the existing catch fallback. ``` </details> <!-- cr-comment:v1:9cf7f57ad468c7b8f1031037 --> </blockquote></details> <details> <summary>src/frontend/hooks/useRateLimitState.ts-46-59 (1)</summary><blockquote> `46-59`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Derive the countdown from `resetTime`, and move `clearTick` out of the state updater.** Two problems exist in this interval callback: 1. The countdown decrements a counter once per tick. Browsers throttle timers in background tabs, so ticks are dropped. `retryAfterSeconds` then drifts above the true remaining time, and the UI reports a rate limit after the limit expired. Compute the remaining seconds from `resetTime` instead. 2. `clearTick()` runs inside the `setState` updater at line 50. React may invoke an updater more than once, so the side effect can repeat. `clearInterval` is idempotent, so the current impact is limited, but the pattern is unsafe. Both problems disappear if the tick computes the remaining time from the stored `resetTime` and clears the interval from an effect. <details> <summary>🛠️ Proposed change</summary> ```diff - tickRef.current = setInterval(() => { - setState((prev) => { - const next = prev.retryAfterSeconds - 1; - if (next <= 0) { - clearTick(); - return { isRateLimited: false, retryAfterSeconds: 0, resetTime: null }; - } - return { - isRateLimited: true, - retryAfterSeconds: next, - resetTime: prev.resetTime, - }; - }); - }, 1000); + tickRef.current = setInterval(() => { + const remaining = Math.ceil((resetTime.getTime() - Date.now()) / 1000); + if (remaining <= 0) { + clearTick(); + setState({ isRateLimited: false, retryAfterSeconds: 0, resetTime: null }); + return; + } + setState({ isRateLimited: true, retryAfterSeconds: remaining, resetTime }); + }, 1000); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/hooks/useRateLimitState.ts` around lines 46 - 59, Update the interval callback in the rate-limit state hook to derive remaining seconds from the stored resetTime and the current time, rather than decrementing prev.retryAfterSeconds, so throttled timers cannot cause drift. Keep the state updater pure by removing clearTick() from it, and move interval cleanup to an effect that clears the timer once the computed remaining time reaches zero. ``` </details> <!-- cr-comment:v1:eb49fd994ec7a3a4f60ab216 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>package.json-80-95 (1)</summary><blockquote> `80-95`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Raise the minimum supported Node.js version to 22.13.0.** `jsdom@29.1.1` requires Node.js `^20.19.0 || ^22.13.0 || >=24.0.0`, but the project documents Node.js `>=22.0.0`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 80 - 95, Update the project’s documented and enforced minimum Node.js version from 22.0.0 to 22.13.0, including the relevant package.json engines or version metadata and any repository configuration that enforces it. Keep the existing Node.js major-version support unchanged while ensuring all declarations consistently require at least 22.13.0. ``` </details> <!-- cr-comment:v1:bae5793d4453170ab1d9abb3 --> </blockquote></details> <details> <summary>src/components/AnnouncementBanner.tsx-74-74 (1)</summary><blockquote> `74-74`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Use the project border token.** `--pf-v5-global--BorderColor--200` is not available with PatternFly 6. Use the defined `--border` token instead. <details> <summary>🎨 Proposed fix</summary> ```diff - <div className="w-full shrink-0 border-b border-[var(--pf-v5-global--BorderColor--200)]"> + <div className="w-full shrink-0 border-b border-border"> ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AnnouncementBanner.tsx` at line 74, Update the border class on the outer div in AnnouncementBanner to use the project’s --border token instead of the unavailable --pf-v5-global--BorderColor--200 token, preserving the existing border-b styling. ``` </details> <!-- cr-comment:v1:72eed07b9067b83d66d10fc4 --> </blockquote></details> <details> <summary>src/frontend/services/authenticated-fetch.ts-59-59 (1)</summary><blockquote> `59-59`: _🩺 Stability & Availability_ | _🟡 Minor_ | _⚡ Quick win_ **Use `same-origin` credentials and add a client-side timeout.** The BFF already aborts upstream agent requests after 30 seconds and returns `502`. A client-side timeout still bounds browser-to-BFF stalls. Compose it with `init.signal`, including an already-aborted signal. All `agent-rest` URLs use the same-origin `/api/proxy/agent` BFF. Replace `credentials: 'include'` with `credentials: 'same-origin'`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/services/authenticated-fetch.ts` at line 59, Update the fetch logic in authenticated-fetch to use credentials: 'same-origin' for the same-origin agent-rest BFF requests. Add a client-side timeout that composes with init.signal, correctly handling an already-aborted caller signal, and aborts the request when the timeout expires while preserving the existing fetch behavior. ``` </details> <!-- cr-comment:v1:330c8c818d6cb0a247950bda --> </blockquote></details> <details> <summary>src/frontend/components/ActivityTimeline.tsx-61-61 (1)</summary><blockquote> `61-61`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Tailwind v3 leading `!` important modifiers remain after the upgrade to Tailwind 4.1.5.** In Tailwind v4 the important modifier is a suffix, so every class written as `!utility` produces no CSS and the intended override silently does not apply. - `src/frontend/components/ActivityTimeline.tsx#L61-L61`: change `!border-none` to `border-none!` and `!bg-neutral-700` to `bg-neutral-700!`. - `src/frontend/components/ChatMessagesView.tsx#L397-L397`: change `[&_p]:!mb-1.5` to `[&_p]:mb-1.5!` and `[&_p:last-child]:!mb-0` to `[&_p:last-child]:mb-0!`. Run the verification script attached to the `ActivityTimeline.tsx` comment to find any remaining occurrences across the repository. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/ActivityTimeline.tsx` at line 61, Update the Tailwind important modifiers in ActivityTimeline.tsx at lines 61-61, changing border-none and bg-neutral-700 to suffix notation. Also update ChatMessagesView.tsx at lines 397-397, converting both paragraph margin modifiers to suffix notation. Run the attached verification script to locate and update any remaining leading-! Tailwind utilities across the repository. ``` </details> <!-- cr-comment:v1:e7d92fc3dc4a7f078d6043ba --> </blockquote></details> <details> <summary>src/frontend/components/ChatMessagesView.tsx-819-825 (1)</summary><blockquote> `819-825`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Remove the duplicate live region for assistant responses.** Line 819 declares a dedicated `aria-live="polite"` region that receives the full last assistant response. Line 860 declares the message container as `role="log" aria-live="polite"`, and the assistant message is also appended there. A screen reader announces the same response twice. Set the message container to `aria-live="off"` and keep the single dedicated announcer, or drop the dedicated announcer and rely on the log. <details> <summary>♿ Proposed fix</summary> ```diff - <div role="log" aria-label="Chat messages" aria-live="polite" aria-busy={isLoading} className="p-4 md:p-6 space-y-5 max-w-3xl mx-auto pt-8"> + <div role="log" aria-label="Chat messages" aria-live="off" aria-busy={isLoading} className="p-4 md:p-6 space-y-5 max-w-3xl mx-auto pt-8"> ``` </details> Also applies to: 860-860 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/ChatMessagesView.tsx` around lines 819 - 825, Remove the duplicate live-region announcement in ChatMessagesView by retaining only one announcement mechanism: either set the message container’s `role="log"` live behavior at the referenced message-container symbol to `aria-live="off"` while keeping the dedicated `srAnnouncement` region, or remove the dedicated announcer and rely on the log. Ensure each assistant response is announced exactly once. ``` </details> <!-- cr-comment:v1:e7b781ac23169e1b60f0b32a --> </blockquote></details> <details> <summary>src/frontend/components/McpStatusPanel.tsx-36-36 (1)</summary><blockquote> `36-36`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Give the labelled container a role.** `aria-label` on a generic `div` is ignored, because the element exposes no role. Assistive technology does not announce "MCP tool status". `TodoStrip.tsx` line 34 already pairs `role="region"` with `aria-label`; use the same pattern here. <details> <summary>♿ Proposed fix</summary> ```diff - <div className="max-w-3xl mx-auto px-3 pt-2" aria-label="MCP tool status"> + <div className="max-w-3xl mx-auto px-3 pt-2" role="region" aria-label="MCP tool status"> ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/McpStatusPanel.tsx` at line 36, Update the labelled container in McpStatusPanel to include role="region" alongside its existing aria-label, matching the established TodoStrip pattern so assistive technology announces “MCP tool status.” ``` </details> <!-- cr-comment:v1:110b1ba78fec18fb21d1db38 --> </blockquote></details> <details> <summary>src/frontend/lib/streaming/StreamingManager.ts-146-165 (1)</summary><blockquote> `146-165`: _🩺 Stability & Availability_ | _🟡 Minor_ | _⚡ Quick win_ **Add a connection timeout to the stream request.** The `fetch` call has no timeout. If the proxy accepts the connection and then stalls before the first byte, `stream()` stays in `'connecting'` forever and no callback fires. Add a timer that aborts the controller if the response headers do not arrive, and clear it once the reader is obtained. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/lib/streaming/StreamingManager.ts` around lines 146 - 165, Add a connection-timeout timer around the fetch call in the stream() flow, aborting the request controller if response headers do not arrive before the timeout. Clear the timer immediately after response.body?.getReader() successfully returns in the existing reader acquisition path, while preserving current HTTP error and missing-reader handling. ``` </details> <!-- cr-comment:v1:19792151ac54ce1e10578767 --> </blockquote></details> <details> <summary>src/frontend/components/ArtifactViewer.tsx-43-43 (1)</summary><blockquote> `43-43`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Tailwind v4 important modifier is a suffix, not a prefix.** Both files use the v3 `!utility` form. The project declares Tailwind CSS 4.1.5, where the modifier must follow the utility name, so neither override is generated. - `src/frontend/components/ArtifactViewer.tsx#L43-L43`: change `className="!p-1"` to `className="p-1!"`. - `src/frontend/components/TasksSidebar.tsx#L81-L81`: change `!pt-0` to `pt-0!` in the `CardBody` class list. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/ArtifactViewer.tsx` at line 43, Update the Tailwind important modifiers in ArtifactViewer.tsx:43-43 and TasksSidebar.tsx:81-81 to Tailwind v4 suffix syntax, changing the padding utilities to p-1! and pt-0! respectively while preserving the surrounding class lists. ``` </details> <!-- cr-comment:v1:aade95db95b19374e9df3091 --> </blockquote></details> <details> <summary>src/frontend/components/ArtifactViewer.tsx-26-30 (1)</summary><blockquote> `26-30`: _🩺 Stability & Availability_ | _🟡 Minor_ | _⚡ Quick win_ **Handle clipboard failures.** `navigator.clipboard` is undefined in non-secure contexts, and `writeText` rejects when the permission is denied. The rejection is unhandled here, so the copy silently fails and `copied` never changes. <details> <summary>🛡️ Proposed fix</summary> ```diff const handleCopy = async () => { - await navigator.clipboard.writeText(content); - setCopied(true); - setTimeout(() => setCopied(false), 2000); + try { + await navigator.clipboard?.writeText(content); + setCopied(true); + setTimeout(() => setCopied(false), 2000); + } catch { + // clipboard unavailable or permission denied + } }; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/ArtifactViewer.tsx` around lines 26 - 30, Update handleCopy to guard against navigator.clipboard being unavailable and catch failures from writeText, ensuring rejected or unsupported clipboard operations do not remain unhandled and copied is only set after a successful write. Preserve the existing success reset behavior. ``` </details> <!-- cr-comment:v1:ec493e8c42003039e43508fb --> </blockquote></details> <details> <summary>src/frontend/types/user.ts-22-23 (1)</summary><blockquote> `22-23`: _🗄️ Data Integrity & Integration_ | _🟡 Minor_ | _⚡ Quick win_ **Fix development `AppData` injection.** The server payload and test setup provide `agentName` and `basePath`, but `index.html` provides neither. `main.tsx` then replaces the inline `window.APP_DATA` with `{}` because `#root` has no `data-app`. Preserve the inline payload or add a complete `data-app` payload with the required fields. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/types/user.ts` around lines 22 - 23, Update the frontend bootstrap flow around main.tsx and AppData so index.html does not overwrite the inline window.APP_DATA with an empty object when `#root` lacks data-app. Preserve the existing inline payload, or provide a complete data-app value containing the required agentName and basePath fields, while keeping server and test payload handling intact. ``` </details> <!-- cr-comment:v1:9553dbdfe045873c909e65f3 --> </blockquote></details> <details> <summary>src/frontend/utils/errorHandler.ts-66-72 (1)</summary><blockquote> `66-72`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Handle Safari and plain abort errors** `fromFetchError` returns `UNKNOWN_ERROR` for Safari’s `TypeError: Load failed` and for a plain `Error` with `name === 'AbortError'`. Check the abort name on `Error` instances first, then extend the network-message check to include `Load failed`. Do not classify every `TypeError`, because generic `TypeError` values can indicate programming errors. This helper currently has no callers, so the retry-path impact is not present. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/utils/errorHandler.ts` around lines 66 - 72, Update ErrorHandler.fromFetchError to check Error instances with name === 'AbortError' before the existing DOMException handling, returning STREAM_INTERRUPTED for those cancellations. Extend the network-error message check to recognize Safari’s TypeError message “Load failed” while retaining the existing fetch check, and do not classify unrelated TypeError instances as network errors. ``` </details> <!-- cr-comment:v1:b8abf271d02412c7adff8248 --> </blockquote></details> <details> <summary>src/frontend/lib/streaming/SSEProcessor.ts-142-147 (1)</summary><blockquote> `142-147`: _🗄️ Data Integrity & Integration_ | _🟡 Minor_ | _⚡ Quick win_ **Bound incomplete SSE input.** `feed` retains input without a `\n\n` delimiter indefinitely. Add a maximum buffer size and reset or report an error when the limit is exceeded. The proxy emits LF delimiters, so CRLF normalization is not required in this frontend path. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/lib/streaming/SSEProcessor.ts` around lines 142 - 147, Update SSEProcessor.feed to enforce a maximum size for the retained incomplete this.buffer after splitting on LF delimiters. When the buffer exceeds that limit without a complete \n\n event boundary, reset it or report an error using the class’s existing error-handling approach; preserve normal parsing for valid input and do not add CRLF normalization. ``` </details> <!-- cr-comment:v1:8a592e453d3bc0128e6f772e --> </blockquote></details> <details> <summary>src/frontend/pages/HomePage.tsx-8-13 (1)</summary><blockquote> `8-13`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Move `QUICK_PROMPTS` into `HomePage`.** The server injects `agentName` into `root[data-app]`, but `main.tsx` assigns that data to `window.APP_DATA` only after imported modules evaluate. `QUICK_PROMPTS` can therefore capture `'Agent'`, while render-time reads use the configured name. Derive `agentName` once inside `HomePage` and build `quickPrompts` from it. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/pages/HomePage.tsx` around lines 8 - 13, Move the QUICK_PROMPTS definition into the HomePage component, derive agentName once from window.APP_DATA there, and build the component-local quickPrompts from that value so it reflects the server-injected name at render time. ``` </details> <!-- cr-comment:v1:5e077e2147ca64be89657aaf --> </blockquote></details> <details> <summary>src/frontend/test-utils/setup.ts-90-98 (1)</summary><blockquote> `90-98`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Add `preferred_username` to the `window.USER_DATA` fixture.** Application code reads `window.USER_DATA.preferred_username`, for example in `src/frontend/contexts/ChatContext.tsx` line 102 and in `AppLayout`. The fixture omits that field, so those paths receive `undefined` in tests and can hide real failures. <details> <summary>🐛 Proposed fix</summary> ```diff Object.defineProperty(window, 'USER_DATA', { value: { displayName: 'Test User', given_name: 'Test', + preferred_username: 'test.user', email: 'test@example.com', }, ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/test-utils/setup.ts` around lines 90 - 98, Update the window.USER_DATA fixture in the test setup to include a representative preferred_username value alongside the existing user fields, so ChatContext and AppLayout tests receive the same property as application code. ``` </details> <!-- cr-comment:v1:1b6922e7c5e8594d3bca6e5b --> </blockquote></details> <details> <summary>src/frontend/components/Sidebar.tsx-145-162 (1)</summary><blockquote> `145-162`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Reject empty rename input.** `handleSaveRename` forwards `editTitle` unchanged. If the user clears the input and presses Enter or blurs the field, the chat title becomes an empty string and the list entry renders blank. Trim the value and keep the previous title when the result is empty. <details> <summary>🐛 Proposed fix</summary> ```diff const handleSaveRename = (chatId: string) => { - onRenameChat(chatId, editTitle); + const next = editTitle.trim(); + if (next) onRenameChat(chatId, next); setEditingChat(null); setEditTitle(''); }; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/Sidebar.tsx` around lines 145 - 162, Update handleSaveRename in Sidebar so it trims editTitle before saving and preserves the chat’s existing title when the trimmed result is empty. Ensure both Enter and blur paths use this validation, while non-empty renamed titles are saved normally. ``` </details> <!-- cr-comment:v1:d163a2a88c4013fa87abf053 --> </blockquote></details> <details> <summary>src/frontend/components/Sidebar.tsx-86-109 (1)</summary><blockquote> `86-109`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Reset `searchQuery` when the search input is hidden.** The search input renders only when `chatHistory.length > 3`. If the user types a query and then deletes chats until 3 or fewer remain, the input unmounts but `searchQuery` stays in state. `filteredChats` then keeps filtering, and the user sees "No matching threads" with no control to clear the filter. <details> <summary>🐛 Proposed fix</summary> ```diff const filteredChats = useMemo(() => { - const q = searchQuery.trim().toLowerCase(); + const q = chatHistory.length > 3 ? searchQuery.trim().toLowerCase() : ''; if (!q) return chatHistory; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/Sidebar.tsx` around lines 86 - 109, Reset searchQuery whenever chatHistory.length is 3 or fewer, so hiding the SearchInput also clears the active filter. Update the state/effect logic associated with filteredChats or the chat history in Sidebar, while preserving search behavior when more than three chats remain. ``` </details> <!-- cr-comment:v1:ae740411982483ccaeee95b6 --> </blockquote></details> <details> <summary>src/frontend/services/logout.ts-9-16 (1)</summary><blockquote> `9-16`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Prefix client auth URLs with `buildAppPath`.** With a non-root `APP_DATA.basePath`, `/auth/logout` and `/auth/login` target the origin root instead of the application route. Apply the same fix to `SessionExpiredModal` and `/auth/refresh` in `useRefreshableToken`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/services/logout.ts` around lines 9 - 16, Prefix the `/auth/login` URL in `buildGatewayLoginUrl` and the `/auth/logout` request in `logout` with `buildAppPath`. Apply the same `buildAppPath` wrapping to the login/logout-related URLs in `SessionExpiredModal` and the `/auth/refresh` request in `useRefreshableToken`, preserving their existing query and request behavior. ``` </details> <!-- cr-comment:v1:776c1e4f1d6ed5c65f08b203 --> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| const patterns: RegExp[] = [ | ||
| /<think>([\s\S]*?)<\/redacted_thinking>/gi, | ||
| /<thinking>([\s\S]*?)<\/thinking>/gi, | ||
| ]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix the mismatched thinking-tag regex.
Line 52 opens with <think> and closes with </redacted_thinking>. That tag pair does not exist. Two defects follow:
<think>…</think>blocks are never extracted, so raw reasoning text renders inside the assistant Markdown body.- If a
<think>open tag and a later</redacted_thinking>close tag both appear in one message, the pattern spans both and deletes all answer text between them.
🐛 Proposed fix
const patterns: RegExp[] = [
- /<think>([\s\S]*?)<\/redacted_thinking>/gi,
+ /<think>([\s\S]*?)<\/think>/gi,
/<thinking>([\s\S]*?)<\/thinking>/gi,
+ /<redacted_thinking>([\s\S]*?)<\/redacted_thinking>/gi,
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const patterns: RegExp[] = [ | |
| /<think>([\s\S]*?)<\/redacted_thinking>/gi, | |
| /<thinking>([\s\S]*?)<\/thinking>/gi, | |
| ]; | |
| const patterns: RegExp[] = [ | |
| /<think>([\s\S]*?)<\/think>/gi, | |
| /<thinking>([\s\S]*?)<\/thinking>/gi, | |
| /<redacted_thinking>([\s\S]*?)<\/redacted_thinking>/gi, | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/components/ChatMessagesView.tsx` around lines 51 - 54, Fix the
first pattern in the patterns array used by ChatMessagesView so it matches a
<think> opening tag with a corresponding </think> closing tag. Preserve the
existing separate <thinking>…</thinking> pattern and ensure the extractor does
not span or remove answer text between mismatched tags.
| if (response.status === 401) { | ||
| notifySessionExpired(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Stop the refresh interval after a 401.
The return on line 58 skips the token update but does not stop the timer. expiresAt is only reassigned on a successful refresh, so after a 401 it stays in the past and line 52 stays true. The interval on line 48 keeps firing every second. Each tick issues another /auth/refresh?forceRefresh=true request and calls notifySessionExpired() again.
Two consequences follow: one request per second per open tab against the auth endpoint for as long as the tab stays open, and repeated session-expired notifications that re-trigger the registered onAuthExpired handler.
Clear the interval when the session is unauthorized.
🐛 Proposed fix
const response = await fetch("/auth/refresh?forceRefresh=true");
if (response.status === 401) {
+ if (keepTokenRefreshedIntervalRef.current) {
+ clearInterval(keepTokenRefreshedIntervalRef.current);
+ keepTokenRefreshedIntervalRef.current = null;
+ }
notifySessionExpired();
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (response.status === 401) { | |
| notifySessionExpired(); | |
| return; | |
| } | |
| if (response.status === 401) { | |
| if (keepTokenRefreshedIntervalRef.current) { | |
| clearInterval(keepTokenRefreshedIntervalRef.current); | |
| keepTokenRefreshedIntervalRef.current = null; | |
| } | |
| notifySessionExpired(); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/hooks/useRefreshableToken.tsx` around lines 56 - 59, Update the
401 branch in the token refresh interval callback within useRefreshableToken to
clear the active refresh timer before calling notifySessionExpired and
returning. Reuse the interval handle established by the hook so unauthorized
sessions stop scheduling further refresh requests and notifications.
- Validate threadId param to prevent SSRF in history endpoint - Add rate limiting to auth plugin routes - Override @grpc/grpc-js to 1.14.4 to fix high-severity vulnerabilities Signed-off-by: Soham Dutta <sodutta@redhat.com> Signed-off-by: Soham Dutta <19648293+NP-compete@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
docs/deployment-patterns.md (1)
113-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language identifiers to the fenced code blocks.
markdownlintreports MD040 for these fences. Usetextfor precedence and log-output examples.Also applies to: 247-252, 351-353, 372-374, 392-395
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/deployment-patterns.md` around lines 113 - 118, Update the fenced code blocks in the deployment patterns documentation, including the precedence and log-output examples, to specify the text language identifier after each opening fence. Apply this consistently to all referenced blocks so markdownlint MD040 passes.Source: Linters/SAST tools
config/compliance/README.md (1)
28-39: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin and verify the OPA binary.
downloads/latestinstalls a mutable version without checksum or signature verification. Pin a tested OPA version and verify its published digest before installing it. This makes policy compilation reproducible and reduces supply-chain risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/compliance/README.md` around lines 28 - 39, Update the OPA installation instructions in the compliance README to use a specific tested version instead of the mutable downloads/latest URL, and add verification of the vendor-published checksum or signature before making the binary executable or using opa version. Apply the pinned, verified installation flow consistently to the documented platform commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/compliance/README.md`:
- Around line 72-75: Update the fenced code examples in the compliance README,
including the OPA output, input-field list, and file-structure blocks, to
specify the text language tag. Preserve their contents while ensuring every
referenced fence passes Markdownlint MD040.
- Around line 151-171: Update the sample OPA eval command in the local testing
section to load the complete config/compliance directory instead of only
policy.rego. Replace the --data target while preserving the existing input
payload and deny query.
In `@config/ui/examples/minimal.yaml`:
- Around line 1-2: Update the header comments in the minimal configuration
example to document that both FEATURE_AUTH_ENABLED and AUTH_ENABLED must be
unset or false, while preserving the local-development/auth-disabled guidance
and noting that environment variables override YAML.
In `@config/ui/examples/production.yaml`:
- Around line 8-10: Update the configuration validation flow that evaluates OPA
and resolves agent endpoints so AGENT_HOST is parsed into a hostname before the
suffix check, rather than validating the raw URL; ensure the resolved hostname
is used for policy evaluation while preserving AGENT_ENDPOINT’s assignment to
agent.endpoint and correctly handling ports and paths.
In `@docs/deployment-patterns.md`:
- Around line 191-196: Remove the agent.endpoint entry from the
environment-driven Kind configuration example, leaving the timeout and
AGENT_HOST guidance intact so the environment variable is actually used
according to the priority order.
- Around line 44-48: Update the restart step in the deployment instructions to
match the documented hot-reload behavior: instruct users to save the title
change and refresh the browser instead of running npm start, unless the
documentation explicitly limits restarts to a separately defined condition.
- Around line 217-225: Update the Agent A and Agent B shell examples in the
deployment documentation, including the corresponding example at the other
referenced section, so BRANDING_TITLE and AGENT_ENDPOINT are exported or
assigned directly before the server command. Ensure the variables are available
to the child server process while preserving the existing values.
- Around line 108-118: Update the features.auth_enabled documentation to
describe environment variables as overrides rather than the only configuration
source. Revise the table wording to acknowledge settings.yaml, and ensure the
precedence section and related local/Kind examples consistently show environment
values taking priority over YAML.
- Around line 120-124: Update the authentication-disabled documentation around
auth_enabled to state that COOKIE_SIGN remains required and must contain at
least 32 characters, while SSO_CLIENT_ID and SSO_CLIENT_SECRET remain optional;
align the local configuration guidance accordingly.
---
Nitpick comments:
In `@config/compliance/README.md`:
- Around line 28-39: Update the OPA installation instructions in the compliance
README to use a specific tested version instead of the mutable downloads/latest
URL, and add verification of the vendor-published checksum or signature before
making the binary executable or using opa version. Apply the pinned, verified
installation flow consistently to the documented platform commands.
In `@docs/deployment-patterns.md`:
- Around line 113-118: Update the fenced code blocks in the deployment patterns
documentation, including the precedence and log-output examples, to specify the
text language identifier after each opening fence. Apply this consistently to
all referenced blocks so markdownlint MD040 passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Enterprise
Run ID: a7c5626b-426f-4539-a7ff-0973df54d080
📒 Files selected for processing (8)
README.mdconfig/compliance/README.mdconfig/ui/README.mdconfig/ui/examples/blue-theme.yamlconfig/ui/examples/minimal.yamlconfig/ui/examples/production.yamlconfig/ui/examples/red-hat-branding.yamldocs/deployment-patterns.md
| ``` | ||
| OPA policy violation: agent endpoint 'https://public-api.example.com/agent' | ||
| does not match any approved internal suffix [".svc.cluster.local", ".internal.example.com"] | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language tags to the fenced examples.
Markdownlint reports bare fences for the OPA output, input-field list, and file-structure examples. Use text for these blocks so MD040 passes.
Also applies to: 142-149, 178-183
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 72-72: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/compliance/README.md` around lines 72 - 75, Update the fenced code
examples in the compliance README, including the OPA output, input-field list,
and file-structure blocks, to specify the text language tag. Preserve their
contents while ensuring every referenced fence passes Markdownlint MD040.
Source: Linters/SAST tools
| **Test your policy locally before deploying:** | ||
|
|
||
| ```bash | ||
| # Evaluate the deny rule against a sample input | ||
| opa eval \ | ||
| --data config/compliance/policy.rego \ | ||
| --input <(echo '{ | ||
| "features": {"auth_enabled": true, "debug_mode_default": false}, | ||
| "agent": {"endpoint": "https://public.example.com"}, | ||
| "security": {"session": {"max_age_days": 30}, "rate_limit": {"max": 100}}, | ||
| "auth_provider": "oidc", | ||
| "platform": {"opa": { | ||
| "approved_auth_providers": ["oidc"], | ||
| "internal_endpoint_suffixes": [".svc.cluster.local"], | ||
| "max_session_ttl_days": 7, | ||
| "restrict_debug_mode": true, | ||
| "restricted_features": [], | ||
| "max_rate_limit": 500 | ||
| }} | ||
| }') \ | ||
| "data.compliance.ui.deny" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '120,190p' config/compliance/README.md
printf '\n--- compliance files ---\n'
git ls-files config/compliance
printf '\n--- policy references ---\n'
rg -n --glob '!*dist*' 'data\.json|opa eval|policy\.rego|config/compliance' config/compliance .github 2>/dev/null | head -200Repository: redhat-data-and-ai/template-ui
Length of output: 2931
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
for p in [Path("config/compliance/README.md"), Path("config/compliance/policy.rego"), Path("config/compliance/data.json")]:
print(f"\n--- {p} ---")
if not p.exists():
print("MISSING")
continue
lines = p.read_text().splitlines()
if p.name == "README.md":
for i in range(1, len(lines)+1):
if 1 <= i <= 220:
print(f"{i:4}: {lines[i-1]}")
else:
for i, line in enumerate(lines, 1):
print(f"{i:4}: {line}")
PY
printf '\n--- all compliance entries ---\n'
git ls-files config/complianceRepository: redhat-data-and-ai/template-ui
Length of output: 9417
🏁 Script executed:
set -eu
printf '%s\n' '--- README lines 151-171 ---'
nl -ba config/compliance/README.md | sed -n '151,171p'
printf '%s\n' '--- compliance files ---'
git ls-files config/compliance
printf '%s\n' '--- data.json references ---'
rg -n 'data\.json|data\.|opa eval|--data' config/complianceRepository: redhat-data-and-ai/template-ui
Length of output: 1046
🏁 Script executed:
printf '%s\n' '--- OPA runtime integration ---'
rg -n -C 4 'policy_path|compile|bundle|\.rego|\.json|opa' --glob '!config/compliance/README.md' --glob '!package-lock.json' --glob '!pnpm-lock.yaml' --glob '!yarn.lock' .
printf '%s\n' '--- OPA availability ---'
if command -v opa >/dev/null 2>&1; then
opa version
else
printf '%s\n' 'opa is not available in the sandbox'
fiRepository: redhat-data-and-ai/template-ui
Length of output: 50388
🌐 Web query:
Open Policy Agent opa eval --data directory loads Rego and JSON files official documentation
💡 Result:
Yes, the --data flag in the Open Policy Agent (OPA) CLI is used to recursively load policy and data files [1]. When a directory is provided to the --data flag, OPA will automatically and recursively load all files with the extensions.rego,.json, and.yaml contained within that directory and its subdirectories [1][2]. Key details regarding this behavior include: - File Types: The command supports Rego policies as well as JSON and YAML data files [1][3]. - Recursive Loading: When a directory is specified, the loader traverses the directory tree to find and include all matching file types [1][4]. - Flag Usage: The --data flag can be repeated to specify multiple files or directories [1][5]. - Distinction from --bundle: While --data recursively loads files to make them available under the data namespace, the --bundle flag is used to load policy bundles, which follow specific conventions regarding directory structure and manifest files [1][2]. Official documentation confirms that without the --bundle flag, OPA defaults to this recursive loading behavior for the provided paths [1].
Citations:
- 1: https://openpolicyagent.org/docs/cli
- 2: https://v0-66-0--opa-docs.netlify.app/docs/v0.66.0/cli/
- 3: https://v1-0-1--opa-docs.netlify.app/docs/v1.0.1/cli/
- 4: https://v0-55-0--opa-docs.netlify.app/docs/v0.55.0/cli/
- 5: https://openpolicyagent.org/docs
Load the complete policy directory in the local OPA test.
The runtime compiles config/compliance, but the command loads only policy.rego. Use --data config/compliance to include data.json and additional .rego files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/compliance/README.md` around lines 151 - 171, Update the sample OPA
eval command in the local testing section to load the complete config/compliance
directory instead of only policy.rego. Replace the --data target while
preserving the existing input payload and deny query.
| # Minimal configuration — auth disabled, suitable for local development. | ||
| # Copy to config/ui/settings.yaml and ensure AUTH_ENABLED is not set (or set to false). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document both authentication environment variables.
FEATURE_AUTH_ENABLED is the preferred override, and environment variables take precedence over YAML. The current instructions mention only legacy AUTH_ENABLED. If FEATURE_AUTH_ENABLED=true exists in the environment, copying this file still enables SSO.
Update the comments to require both FEATURE_AUTH_ENABLED and AUTH_ENABLED to be unset or false.
Proposed documentation fix
-# Copy to config/ui/settings.yaml and ensure AUTH_ENABLED is not set (or set to false).
+# Copy to config/ui/settings.yaml and ensure FEATURE_AUTH_ENABLED and AUTH_ENABLED are not set (or set to false).
...
- auth_enabled: false # Bypasses SSO; uses dummy user developer@redhat.com
+ auth_enabled: false # Bypasses SSO; keep FEATURE_AUTH_ENABLED and AUTH_ENABLED unset or falseAlso applies to: 12-14
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/ui/examples/minimal.yaml` around lines 1 - 2, Update the header
comments in the minimal configuration example to document that both
FEATURE_AUTH_ENABLED and AUTH_ENABLED must be unset or false, while preserving
the local-development/auth-disabled guidance and noting that environment
variables override YAML.
| # SSO_ISSUER_HOST — e.g. https://sso.example.com/realms/myrealm | ||
| # SSO_CALLBACK_URL — e.g. https://myagent.example.com/auth/callback/oidc | ||
| # AGENT_HOST — e.g. http://my-agent-engine:8082 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'AGENT_HOST|AGENT_ENDPOINT|agent\.endpoint|internal_endpoint_suffixes|input\.agent' \
src/server/router/proxy.router.ts \
src/server/utils/settings.ts \
src/server/utils/opa.ts \
config/compliance/policy.regoRepository: redhat-data-and-ai/template-ui
Length of output: 6932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- production config ---'
cat -n config/ui/examples/production.yaml | sed -n '1,80p'
printf '%s\n' '--- policy ---'
cat -n config/compliance/policy.rego | sed -n '1,140p'
printf '%s\n' '--- OPA call sites and settings lifecycle ---'
rg -n -C 8 'evaluate|OPA|opa|fail_on_violation|getSettings|loadSettings|validateConfig|input\.agent|agent:\s*\{' \
src/server config --glob '!**/node_modules/**' --glob '!**/dist/**' | sed -n '1,320p'
printf '%s\n' '--- settings sections ---'
cat -n src/server/utils/settings.ts | sed -n '160,235p;280,420p;480,570p'Repository: redhat-data-and-ai/template-ui
Length of output: 38863
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- OPA plugin ---'
cat -n src/server/plugins/opa.plugin.ts | sed -n '1,260p'
printf '%s\n' '--- server startup and config loading ---'
rg -n -C 10 'setupServer|startConfigWatcher|listen\(|getSettings\(|UI_CONFIG_PATH|AGENT_HOST|AGENT_ENDPOINT' \
src/server src --glob '*.ts' --glob '*.js' | sed -n '1,360p'
printf '%s\n' '--- settings interface and environment mappings ---'
cat -n src/server/utils/settings.ts | sed -n '1,165p;420,490p'
printf '%s\n' '--- deterministic precedence probe ---'
python3 - <<'PY'
from urllib.parse import urlsplit
suffixes = [".svc.cluster.local", ".internal.example.com"]
def opa_denies(endpoint):
return endpoint != "" and not any(endpoint.endswith(s) for s in suffixes)
def settings_endpoint(yaml_endpoint="", agent_endpoint=None, agent_host=None):
# Mirrors the source paths: AGENT_ENDPOINT is applied to config; AGENT_HOST
# is consumed later by proxy/getAgentName, not applied to config.
endpoint = yaml_endpoint
if agent_endpoint:
endpoint = agent_endpoint
return endpoint
cases = [
("YAML empty + AGENT_HOST example", "", None, "http://my-agent-engine:8082"),
("YAML empty + AGENT_ENDPOINT approved URL", "", "https://agent.svc.cluster.local", None),
("YAML empty + AGENT_ENDPOINT approved URL with port", "", "https://agent.svc.cluster.local:8082", None),
("YAML empty + AGENT_ENDPOINT approved URL with path", "", "https://agent.svc.cluster.local/agent", None),
("YAML empty + AGENT_ENDPOINT example", "", "http://my-agent-engine:8082", None),
]
for name, yaml_endpoint, agent_endpoint, agent_host in cases:
input_endpoint = settings_endpoint(yaml_endpoint, agent_endpoint, agent_host)
runtime_endpoint = input_endpoint or agent_host or "http://localhost:5002"
print({
"case": name,
"opa_input_agent_endpoint": input_endpoint,
"opa_denies_endpoint_rule": opa_denies(input_endpoint),
"runtime_endpoint": runtime_endpoint,
"hostname": urlsplit(runtime_endpoint).hostname,
})
PYRepository: redhat-data-and-ai/template-ui
Length of output: 31537
Resolve AGENT_HOST before OPA evaluation. AGENT_ENDPOINT is applied to agent.endpoint, but AGENT_HOST is only used by the proxy. With the empty YAML value, the documented AGENT_HOST bypasses the endpoint rule and can target an unapproved host. Parse the URL hostname before applying the suffix check so ports and paths do not cause false violations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/ui/examples/production.yaml` around lines 8 - 10, Update the
configuration validation flow that evaluates OPA and resolves agent endpoints so
AGENT_HOST is parsed into a hostname before the suffix check, rather than
validating the raw URL; ensure the resolved hostname is used for policy
evaluation while preserving AGENT_ENDPOINT’s assignment to agent.endpoint and
correctly handling ports and paths.
| 3. **Restart the server** (only the title is needed; for logo changes the file is served immediately but the browser may cache the old one): | ||
|
|
||
| ```bash | ||
| npm start | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the restart instruction with the documented hot-reload behavior.
This step says that a title change requires a server restart. Line 70 says that branding changes, including the title, are hot-reloaded. Replace the restart instruction with save-and-refresh steps, or document the exact condition that requires a restart.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/deployment-patterns.md` around lines 44 - 48, Update the restart step in
the deployment instructions to match the documented hot-reload behavior:
instruct users to save the title change and refresh the browser instead of
running npm start, unless the documentation explicitly limits restarts to a
separately defined condition.
| | `features.auth_enabled` | `bool` | `true` | `FEATURE_AUTH_ENABLED` (or legacy `AUTH_ENABLED`) | `restricted_features`, `approved_auth_providers` | When `false`, bypasses SSO entirely and injects a dummy user (`developer@redhat.com`). Controlled by agent-engine during deployment — set via env var, not YAML. | | ||
| | `features.debug_mode_default` | `bool` | `false` | `FEATURE_DEBUG_MODE_DEFAULT` | `restrict_debug_mode: true` | Default open/closed state of the debug panel. Users can toggle it. Never enable in production — OPA can enforce this. | | ||
|
|
||
| ### Precedence | ||
|
|
||
| ``` | ||
| FEATURE_AUTH_ENABLED env var | ||
| > AUTH_ENABLED env var (legacy) | ||
| > features.auth_enabled in settings.yaml | ||
| > built-in default (true) | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe features.auth_enabled as an environment override, not an environment-only setting.
The table says this value is set via environment variables and not YAML. The precedence section and the local and Kind examples use YAML values. Document that environment variables override YAML instead.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 113-113: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/deployment-patterns.md` around lines 108 - 118, Update the
features.auth_enabled documentation to describe environment variables as
overrides rather than the only configuration source. Revise the table wording to
acknowledge settings.yaml, and ensure the precedence section and related
local/Kind examples consistently show environment values taking priority over
YAML.
| ### When to use each flag | ||
|
|
||
| **`auth_enabled: false`** — local development only. Skips the SSO plugin registration entirely. The dummy user is always `developer@redhat.com`. The `COOKIE_SIGN`, `SSO_CLIENT_ID`, and `SSO_CLIENT_SECRET` env vars are not required. | ||
|
|
||
| **`debug_mode_default: true`** — useful when iterating on agent responses locally to see raw tool calls and streaming chunks. Should always be `false` in production (OPA `restrict_debug_mode: true` enforces this). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not document COOKIE_SIGN as optional when authentication is disabled.
src/server/server.ts requires COOKIE_SIGN to contain at least 32 characters before it checks cfg.features.auth_enabled. Therefore, the local configuration in Lines 155-176 still needs COOKIE_SIGN, even when SSO credentials are not required.
Proposed wording
- The `COOKIE_SIGN`, `SSO_CLIENT_ID`, and `SSO_CLIENT_SECRET` env vars are not required.
+ SSO credentials are not required when authentication is disabled. `COOKIE_SIGN` is still required.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### When to use each flag | |
| **`auth_enabled: false`** — local development only. Skips the SSO plugin registration entirely. The dummy user is always `developer@redhat.com`. The `COOKIE_SIGN`, `SSO_CLIENT_ID`, and `SSO_CLIENT_SECRET` env vars are not required. | |
| **`debug_mode_default: true`** — useful when iterating on agent responses locally to see raw tool calls and streaming chunks. Should always be `false` in production (OPA `restrict_debug_mode: true` enforces this). | |
| ### When to use each flag | |
| **`auth_enabled: false`** — local development only. Skips the SSO plugin registration entirely. The dummy user is always `developer@redhat.com`. SSO credentials are not required when authentication is disabled. `COOKIE_SIGN` is still required. | |
| **`debug_mode_default: true`** — useful when iterating on agent responses locally to see raw tool calls and streaming chunks. Should always be `false` in production (OPA `restrict_debug_mode: true` enforces this). |
🧰 Tools
🪛 LanguageTool
[style] ~124-~124: To form a complete sentence, be sure to include a subject.
Context: ...ee raw tool calls and streaming chunks. Should always be false in production (OPA `r...
(MISSING_IT_THERE)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/deployment-patterns.md` around lines 120 - 124, Update the
authentication-disabled documentation around auth_enabled to state that
COOKIE_SIGN remains required and must contain at least 32 characters, while
SSO_CLIENT_ID and SSO_CLIENT_SECRET remain optional; align the local
configuration guidance accordingly.
| agent: | ||
| endpoint: "http://template-agent.default.svc.cluster.local:8082" | ||
| timeout_ms: 30000 | ||
| ``` | ||
|
|
||
| Set `AGENT_HOST=http://template-agent.default.svc.cluster.local:8082` in the Kind ConfigMap instead if you prefer env-driven config. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the YAML endpoint from the environment-driven Kind example.
This example defines agent.endpoint and then says to use AGENT_HOST instead. The priority order in Lines 245-252 gives the YAML endpoint higher priority, so AGENT_HOST has no effect while the YAML value remains present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/deployment-patterns.md` around lines 191 - 196, Remove the
agent.endpoint entry from the environment-driven Kind configuration example,
leaving the timeout and AGENT_HOST guidance intact so the environment variable
is actually used according to the priority order.
| ```bash | ||
| # Agent A | ||
| BRANDING_TITLE="Code Review Agent" AGENT_ENDPOINT=http://code-review-agent:8082 | ||
|
|
||
| # Agent B | ||
| BRANDING_TITLE="Data Assistant" AGENT_ENDPOINT=http://data-agent:8082 | ||
| ``` | ||
|
|
||
| No `settings.yaml` file is needed — env vars alone are sufficient. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Export shell variables or attach them to a command.
The shell examples assign variables without export and without a command. A child process running the server will not receive these variables. Use export BRANDING_TITLE=... and export AGENT_ENDPOINT=..., or place the assignments before the server command.
Also applies to: 266-269
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/deployment-patterns.md` around lines 217 - 225, Update the Agent A and
Agent B shell examples in the deployment documentation, including the
corresponding example at the other referenced section, so BRANDING_TITLE and
AGENT_ENDPOINT are exported or assigned directly before the server command.
Ensure the variables are available to the child server process while preserving
the existing values.
Summary
deep-agentdevelopment branch intomainCloses #113
Summary by CodeRabbit