feature/web ui#173
Merged
Merged
Conversation
# Conflicts: # rogue/server/websocket/manager.py
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Vite-based web SPA (client app, routes, components, stores, API layer, WS hooks), integrates its build into CI and packaging (Hatch build hook), migrates Python dev tooling to ruff/ty, adds on-disk JobStore persistence and config/file-picker endpoints, and extends scenario execution with per-scenario attempts and temperature controls. ChangesWeb UI, Server Persistence, and Tooling Migration
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant SPA as Vite SPA
participant API as FastAPI Server
participant JobStore as JobStore (disk)
participant LLM as LiteLLM / Judge
participant WS as WebSocket
Browser->>SPA: load app
SPA->>API: GET /api/v1/config
API->>JobStore: read/merge config.toml
JobStore-->>API: config dict
API-->>SPA: config + env defaults
SPA->>SPA: sync config to Zustand
Browser->>SPA: POST /api/v1/evaluations (start)
SPA->>API: create evaluation
API->>JobStore: save EvaluationJob (pending)
API-->>SPA: { job_id }
SPA->>WS: open ws /api/v1/ws/job/{job_id}
API->>LLM: run evaluation (multi-attempts, temp)
LLM-->>API: progress events
API->>WS: emit timeline events
WS-->>SPA: timeline events
SPA->>SPA: render LiveConversation
API->>JobStore: update job (progress/completed)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major 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 (3)
examples/mcp/tshirt_store_langgraph_mcp/__main__.py (1)
13-26:⚠️ Potential issue | 🟡 MinorUpdate stale URL comments after port default change
Line 13 changes the default port to
10002, but Line 25 and Line 26 still document10001. This will mislead users copying the endpoint URLs.Suggested fix
- # When using "sse", the url will be http://localhost:10001/sse - # When using "streamable-http", the url will be http://localhost:10001/mcp + # When using "sse", the URL will be http://localhost:10002/sse (or http://localhost:{port}/sse) + # When using "streamable-http", the URL will be http://localhost:10002/mcp (or http://localhost:{port}/mcp)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/mcp/tshirt_store_langgraph_mcp/__main__.py` around lines 13 - 26, Update the stale endpoint comments to match the changed default port (10002) in the click option for the main function; locate the main function and its click.option default port setting and modify the two comment lines that currently show "http://localhost:10001/sse" and "http://localhost:10001/mcp" to use "http://localhost:10002/sse" and "http://localhost:10002/mcp" respectively so the documented URLs reflect the actual default port.rogue/prompt_injection_evaluator/run_prompt_injection_evaluator.py (1)
126-197:⚠️ Potential issue | 🟠 MajorWrap dataset and agent setup in error handling.
load_dataset,get_agent_card, andsend_messagecan all fail here; right now any of them will terminate the async generator and drop the entire evaluation run. Please catch the expected failures and surface a structured error/result instead of aborting the stream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rogue/prompt_injection_evaluator/run_prompt_injection_evaluator.py` around lines 126 - 197, The arun_prompt_injection_evaluator function must not let load_dataset, A2ACardResolver.get_agent_card, or RemoteAgentConnections.send_message raise and abort the async generator; wrap the dataset load and agent setup in a try/except and yield a structured error result (e.g., update PromptInjectionResult and yield an "error" or "status" tuple) if load_dataset or card_resolver.get_agent_card fails, and wrap each call to agent_client.send_message inside try/except so failures for a single sample yield a structured error/result and continue to the next sample instead of terminating the stream; reference the existing symbols load_dataset, DatasetDict/Dataset checks, A2ACardResolver.get_agent_card, RemoteAgentConnections.send_message, PromptInjectionResult, and arun_prompt_injection_evaluator when making the changes.rogue/__main__.py (1)
180-185:⚠️ Potential issue | 🟠 MajorDon't pipe the example agent's output unless you also drain it.
Lines 183-184 attach both streams to
PIPE, but this child stays alive for the full session and the parent only reads those pipes on startup failure. Once the example agent writes enough logs to fill the OS buffer, it can block and stop serving requests.💡 Suggested fix
process = subprocess.Popen( # noqa: S603 cmd, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rogue/__main__.py` around lines 180 - 185, The child process started with subprocess.Popen (the process variable in the try block) is created with both stdout and stderr set to PIPE which can deadlock when the agent runs long-lived; change the spawn to not attach pipes (e.g., use stdout=subprocess.DEVNULL and stderr=subprocess.DEVNULL or stderr=subprocess.STDOUT) or implement background readers to continuously drain process.stdout/process.stderr so the OS buffers never fill; update the subprocess.Popen call in the same try block accordingly (refer to the process variable and the Popen(...) invocation).
🟡 Minor comments (15)
rogue/server/red_teaming/report/tui_formatter.py-258-264 (1)
258-264:⚠️ Potential issue | 🟡 MinorHandle
not_testedexplicitly in framework icon mapping.The fallback currently marks every non-
excellent/goodstatus as ❌, sonot_testedappears as a failure in markdown output. Add a neutral icon fornot_testedto avoid misleading report consumers.Proposed fix
icon = ( "✅" if card.status == "excellent" else "⚠️" if card.status == "good" + else "○" + if card.status == "not_tested" else "❌" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rogue/server/red_teaming/report/tui_formatter.py` around lines 258 - 264, The icon mapping for card statuses in the renderer currently treats any status that is not "excellent" or "good" as a failure (setting icon based on card.status in the assignment to icon), which mislabels "not_tested"; update the conditional in rogue/server/red_teaming/report/tui_formatter.py where icon is assigned (referencing card.status) to explicitly handle "not_tested" (choose a neutral icon such as "⏺️" or "⚪") before falling back to the failure icon for true failures, so "not_tested" renders neutrally instead of as ❌.packages/web/src/lib/utils.ts-18-23 (1)
18-23:⚠️ Potential issue | 🟡 MinorGuard invalid and future timestamps in
formatRelative.Line 18/19 can produce
NaNor negative diffs, which leads to bad strings (e.g., negative “ago” values orInvalid Date).Suggested patch
export function formatRelative(iso?: string | null): string { if (!iso) return "—"; const then = new Date(iso).getTime(); + if (!Number.isFinite(then)) return "—"; const diff = (Date.now() - then) / 1000; + if (diff < 0) return "just now"; if (diff < 60) return `${Math.floor(diff)}s ago`; if (diff < 3600) return `${Math.floor(diff / 60)}m ago`; if (diff < 86400) return `${Math.floor(diff / 3600)}h ago`; - return new Date(iso).toLocaleDateString(); + return new Date(then).toLocaleDateString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/lib/utils.ts` around lines 18 - 23, The formatRelative function currently computes "then" and "diff" which can be NaN or negative for invalid or future ISO strings; add validation: parse the iso string (reuse the existing "then" variable) and if Number.isNaN(then) return a sensible fallback (e.g., empty string or the original iso), and if then > Date.now() treat it as a zero diff (return "just now" or "0s ago") instead of producing negative "ago" values; update the logic around the "then" and "diff" variables so subsequent branches (seconds/minutes/hours/dates) only run for valid, non-future timestamps.packages/web/src/lib/protocols.ts-12-23 (1)
12-23:⚠️ Potential issue | 🟡 MinorHandle unsupported protocols explicitly.
This switch currently falls off the end for any runtime value outside the mirrored union, so callers can get
undefinedeven though the function promisesTransport[]. Returning a safe fallback here avoids blowing up the form if the backend adds a protocol first.Suggested fix
+const TRANSPORTS_BY_PROTOCOL: Record<Protocol, Transport[]> = { + a2a: ["http"], + mcp: ["streamable_http", "sse"], + openai_api: ["chat_completions"], + python: [], +}; + export function getTransportsForProtocol(protocol: Protocol): Transport[] { - switch (protocol) { - case "mcp": - return ["streamable_http", "sse"]; - case "a2a": - return ["http"]; - case "openai_api": - return ["chat_completions"]; - case "python": - return []; - } + return TRANSPORTS_BY_PROTOCOL[protocol] ?? []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/lib/protocols.ts` around lines 12 - 23, The getTransportsForProtocol function currently can return undefined for unknown runtime Protocol values; update the switch in getTransportsForProtocol (and/or add a default branch) to return a safe fallback (e.g., an empty array) for unsupported protocols so the function always returns Transport[]; optionally include an exhaustive type guard using the Protocol/Transport union (or a never check) to surface new protocols at compile time while still returning [] at runtime for unknown values.packages/web/src/components/ui/sonner.tsx-5-20 (1)
5-20:⚠️ Potential issue | 🟡 MinorMerge
toastOptions/classNameinstead of letting...propsreplace them.Line 20 currently lets consumer props overwrite the wrapper defaults wholesale. A caller that only sets
toastOptions.durationor a customclassNamewill silently lose the shared toast styling defined on Lines 10-18.💡 Suggested fix
+import { cn } from "@/lib/utils"; import { Toaster as Sonner, type ToasterProps } from "sonner"; import { useTheme } from "@/components/theme-provider"; export function Toaster(props: ToasterProps) { const { theme } = useTheme(); return ( <Sonner + {...props} theme={theme} - className="toaster group" + className={cn("toaster group", props.className)} toastOptions={{ + ...props.toastOptions, classNames: { toast: "group toast group-[.toaster]:bg-popover group-[.toaster]:text-popover-foreground group-[.toaster]:border-border group-[.toaster]:shadow-lg", description: "group-[.toast]:text-muted-foreground", actionButton: "group-[.toast]:bg-primary group-[.toast]:text-primary-foreground", cancelButton: "group-[.toast]:bg-muted group-[.toast]:text-muted-foreground", + ...props.toastOptions?.classNames, }, }} - {...props} /> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/ui/sonner.tsx` around lines 5 - 20, The Toaster component currently spreads {...props} last, allowing callers to replace the default className and toastOptions; update Toaster to merge instead: compute a merged className by concatenating the existing default className on Sonner with props.className (e.g., via clsx or string join), and compute mergedToastOptions by shallow-merging the default toastOptions with props.toastOptions while deep-merging toastOptions.classNames (merge each classNames key so callers can override or add a single key without dropping defaults); then pass these merged values into the Sonner call (and spread the remaining props) so callers cannot wipe out the shared styling defined in Toaster.lefthook.yaml-66-74 (1)
66-74:⚠️ Potential issue | 🟡 MinorModified files may not be re-staged after auto-fix.
prettier --writeandeslint --fixmodify files in place, but unlike theend-of-file-fixerjob (lines 11-16), these don't rungit addafterward. This means fixes won't be included in the commit.Suggested fix to re-stage modified files
- name: prettier-web root: "packages/web/" glob: "*.{ts,tsx,js,jsx,json,css,md}" - run: pnpm exec prettier --write {staged_files} + run: | + pnpm exec prettier --write {staged_files} + git add {staged_files} - name: eslint-web root: "packages/web/" glob: "*.{ts,tsx,js,jsx}" - run: pnpm exec eslint --fix {staged_files} + run: | + pnpm exec eslint --fix {staged_files} + git add {staged_files}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lefthook.yaml` around lines 66 - 74, The prettier-web and eslint-web hooks run auto-fixers but don’t re-stage changed files, so update the lefthook jobs for the hooks named "prettier-web" and "eslint-web" to run git add on the modified files after running pnpm exec prettier --write {staged_files} and pnpm exec eslint --fix {staged_files}; specifically append a step or combine commands to run git add {staged_files} (or an equivalent re-stage command) so fixes are included in the commit.packages/web/src/routes/settings/api-keys.tsx-27-39 (1)
27-39:⚠️ Potential issue | 🟡 MinorAssociate each provider label with its input.
These labels are visual only right now. Without an
htmlFor/idpair, clicking the provider name won't focus the field and assistive tech loses the label/input relationship.Suggested fix
{PROVIDERS.map((p) => ( - <div key={p.id} className="grid grid-cols-[10rem_1fr] items-center gap-3"> - <Label className="text-sm"> + <div key={p.id} className="grid grid-cols-[10rem_1fr] items-center gap-3"> + <Label htmlFor={`api-key-${p.id}`} className="text-sm"> {p.name} <span className="ml-2 text-[10px] uppercase tracking-wider text-muted-foreground"> {p.id} </span> </Label> <Input + id={`api-key-${p.id}`} type="password" value={cfg.apiKeys[p.id as Provider] ?? ""} placeholder={p.fields.find((f) => f.key === "apiKey")?.placeholder ?? "—"} onChange={(e) => cfg.setApiKey(p.id as Provider, e.target.value)} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/routes/settings/api-keys.tsx` around lines 27 - 39, The Label and Input are not associated for accessibility; update the Label (component used around provider name) to include htmlFor and give the Input an id built from the provider id (e.g., `api-key-${p.id}`) so each pair is unique, then keep the existing Input props (type, value, placeholder, onChange) and ensure the id value is a string (use p.id as string or template literal) so cfg.setApiKey(p.id as Provider, ...) still works..github/actions/codestyle/action.yml-19-24 (1)
19-24:⚠️ Potential issue | 🟡 MinorUpdate the formatting guideline to reflect the migration from Black to Ruff.
The CI action now enforces
ruff format, but the repo guideline states "Useblackfor code formatting". While Ruff's formatter is designed as a drop-in replacement with >99.9% compatibility to Black, the documented rule should be updated to keep the contract consistent—either by updating the guideline to "Useruff format" or by adding "or ruff format as a compatible alternative".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/codestyle/action.yml around lines 19 - 24, The repo guideline still says "Use `black` for code formatting" but CI now runs "uv run ruff format --check" and "uv run ruff check"; update the documentation string/guide that currently references "Use `black` for code formatting" to either "Use `ruff format`" or "Use `black` (or `ruff format` as a compatible alternative)" so the written guideline matches the CI steps (search for the exact phrase "Use `black` for code formatting" and replace or augment it accordingly).packages/web/src/components/site-header.tsx-34-48 (1)
34-48:⚠️ Potential issue | 🟡 MinorMark the active breadcrumb as the current page.
The last crumb is still rendered as a normal link, so assistive tech never gets
aria-current="page"and keyboard users can still tab to a link for the page they’re already on.♿ Suggested change
- <Link - to={c.to} - className={cn( - "truncate transition-colors", - i === crumbs.length - 1 - ? "font-medium text-foreground" - : "text-muted-foreground hover:text-foreground", - )} - > - {c.label} - </Link> + {i === crumbs.length - 1 ? ( + <span className="truncate font-medium text-foreground" aria-current="page"> + {c.label} + </span> + ) : ( + <Link + to={c.to} + className={cn("truncate transition-colors", "text-muted-foreground hover:text-foreground")} + > + {c.label} + </Link> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/site-header.tsx` around lines 34 - 48, The last breadcrumb is rendered as a normal Link so assistive tech and keyboard users don't get aria-current semantics; update the crumbs.map render in the site-header component to detect the last item (i === crumbs.length - 1) and render a non-interactive element (e.g., a <span> or <div>) instead of <Link> for that item, add aria-current="page" to that element, keep the same classes applied via cn for visual consistency, and ensure it is not focusable (no tabIndex or set tabIndex={-1}) so keyboard users won't tab to the current page; adjust the code paths around crumbs, IconChevronRight, Link and cn accordingly.packages/web/src/routes/index.tsx-34-41 (1)
34-41:⚠️ Potential issue | 🟡 MinorDon’t report the server as live before the health check resolves.
health.isError ? "Disconnected" : "Live"marks the backend healthy during the initial loading state and while retries are still in flight, so the first paint can briefly show a false green status.🩺 Suggested state split
+ const serverValue = health.isPending + ? "Checking…" + : health.isError + ? "Disconnected" + : "Live"; + const serverTone = health.isPending + ? undefined + : health.isError + ? "destructive" + : "success"; + return ( @@ <MetricCard label="Server" - value={health.isError ? "Disconnected" : "Live"} - tone={health.isError ? "destructive" : "success"} + value={serverValue} + tone={serverTone} description={ health.data?.timestamp ? `Last ping ${formatRelative(health.data.timestamp)}` : "—" } />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/routes/index.tsx` around lines 34 - 41, The status logic in the MetricCard uses health.isError ? "Disconnected" : "Live" which shows "Live" during initial loading/retries; change it to explicitly handle loading/unknown by using health.isSuccess for "Live", health.isError for "Disconnected", and a third state (e.g., "Checking…" or "—") when neither is true (health.isLoading or initial). Update both the value and tone props on the MetricCard used here (MetricCard value, tone, and description rendering that uses health.data?.timestamp and formatRelative) so tone follows the same three-way mapping (success for isSuccess, destructive for isError, neutral/default for loading).packages/web/src/routes/red-team/$jobId.index.tsx-27-38 (1)
27-38:⚠️ Potential issue | 🟡 MinorMake terminal redirects follow the streamed status.
isTerminalonly checksjob.data?.status, so if the WebSocket reports completion before the query cache is patched, the user can stay on the live view longer than intended. Consider deriving the redirect condition fromstream.latest?.statusas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/routes/red-team/`$jobId.index.tsx around lines 27 - 38, The redirect should use the streamed status when available: change how status/isTerminal are computed so they prefer stream.latest?.status over job.data?.status (e.g. derive a latestStatus = stream.latest?.status ?? job.data?.status and then compute isTerminal from that) and use that isTerminal for the Navigate to the report route; update references to status/isTerminal (and keep the Navigate to "/red-team/$jobId/report" with params {{ jobId }}) so a WebSocket-driven completion triggers the redirect immediately.packages/web/src/api/ws.ts-228-229 (1)
228-229:⚠️ Potential issue | 🟡 MinorMissing
kindin the useEffect dependency array.The effect uses
kindto determine the query key (line 111), butkindis not included in the dependency array. Ifkindchanges withoutjobIdchanging, the hook will use a stale query key.🐛 Proposed fix
}; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [jobId]); + }, [jobId, kind]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/api/ws.ts` around lines 228 - 229, The useEffect that currently lists only jobId in its dependency array is missing kind, causing a stale query key when kind changes; update the effect (the useEffect surrounding jobId) to include kind in its dependency array (e.g., [jobId, kind]) or derive the query key inside the effect and depend on both values, and remove the eslint-disable-next-line react-hooks/exhaustive-deps so ESLint can validate dependencies for the useEffect and prevent regressions.rogue/run_web.py-75-77 (1)
75-77:⚠️ Potential issue | 🟡 MinorPotential
ValueErrorifPORTenv var is not a valid integer.If
PORTis set to a non-numeric value,int(os.getenv("PORT", "8000"))will raise an unhandledValueError. Consider wrapping in a try/except or validating the value.🛡️ Proposed fix to handle invalid PORT gracefully
host = getattr(args, "host", os.getenv("HOST", "127.0.0.1")) - port = getattr(args, "port", int(os.getenv("PORT", "8000"))) + try: + port = getattr(args, "port", None) or int(os.getenv("PORT", "8000")) + except ValueError: + logger.error("Invalid PORT environment variable; must be an integer.") + return 1 url = args.server_url or f"http://{host}:{port}/"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rogue/run_web.py` around lines 75 - 77, The current int(os.getenv("PORT", "8000")) call used to set the port variable can raise ValueError for non-numeric PORT env values; update the port assignment in the host/port/url block to validate/parsing logic: attempt to read os.getenv("PORT") and convert to int inside a try/except (or use str.isdigit() validation), fall back to the default 8000 on failure, and optionally log or warn about the invalid value, then use the validated port when building url = args.server_url or f"http://{host}:{port}/". Ensure you update the symbol port (and keep host and url logic) so invalid env input does not crash the process.packages/web/src/routes/red-team/configure.tsx-293-301 (1)
293-301:⚠️ Potential issue | 🟡 MinorMissing validation for attacks-per-vulnerability input.
Number(e.target.value)will produceNaNif the input is empty or non-numeric, and there's no validation that the value stays within themin/maxbounds (1-20). This could send invalid values to the backend.🛡️ Proposed fix with validation
onChange={(e) => rt.setAttacksPerVulnerability(Number(e.target.value))} + onBlur={(e) => { + const val = Number(e.target.value); + if (Number.isNaN(val) || val < 1) { + rt.setAttacksPerVulnerability(1); + } else if (val > 20) { + rt.setAttacksPerVulnerability(20); + } + }}Alternatively, clamp the value in
setAttacksPerVulnerabilitywithin the store.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/routes/red-team/configure.tsx` around lines 293 - 301, The numeric input can produce NaN or out-of-range values; update the onChange handler for the Input (id="apv") to parse the value safely, guard against NaN (e.g., fallback to 1 or the previous rt.attacksPerVulnerability), and clamp the result to the allowed range 1–20 before calling rt.setAttacksPerVulnerability; alternatively implement the clamp inside setAttacksPerVulnerability in the store so that any call (from this Input or elsewhere) enforces Math.max(1, Math.min(20, parsedValue)) and ignores/handles empty/non-numeric inputs.packages/web/src/components/model-picker/dialog.tsx-114-115 (1)
114-115:⚠️ Potential issue | 🟡 MinorDraft reset may trigger unexpectedly on parent re-renders.
The effect resets
draftwheneverinitialchanges. If the parent component creates a newinitialobject on each render (common with inline objects), this will reset the draft state unexpectedly, losing user edits. Consider using a stable reference or comparing the actual values:♻️ Suggested fix using JSON comparison
+ const initialRef = useRef(initial); // Reset when reopened with a new value. - useEffect(() => setDraft(initial), [initial]); + useEffect(() => { + if (JSON.stringify(initial) !== JSON.stringify(initialRef.current)) { + initialRef.current = initial; + setDraft(initial); + } + }, [initial]);Alternatively, the parent (
ModelPickerButton) should memoize theinitialvalue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/model-picker/dialog.tsx` around lines 114 - 115, The effect in ModelPicker dialog uses useEffect(() => setDraft(initial), [initial]) which resets draft when the parent supplies a new object identity; change the hook to only reset when the actual values change (not just reference) by comparing deep equality of initial (e.g., JSON.stringify or a deepEqual) before calling setDraft, or use a deep-compare effect helper; alternatively ensure the parent ModelPickerButton memoizes the initial prop so its reference doesn't change on every render; update the logic around setDraft/initial in the dialog component to guard against identity-only changes.packages/web/src/api/client.ts-47-48 (1)
47-48:⚠️ Potential issue | 🟡 MinorUnsafe type assertion for 204 responses.
Returning
undefined as Tis a type lie—the function signature promisesPromise<T>, but callers receiveundefinedfor 204 responses. This could cause runtime errors if callers don't explicitly check forundefined.Consider using a discriminated return type or a separate function for endpoints that may return 204:
♻️ Option 1: Make return type explicit
-export async function api<T>(path: string, opts: RequestOptions = {}): Promise<T> { +export async function api<T>(path: string, opts: RequestOptions = {}): Promise<T | undefined> { // ... - if (res.status === 204) return undefined as T; + if (res.status === 204) return undefined; return (await res.json()) as T; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/api/client.ts` around lines 47 - 48, The code returns "undefined as T" when res.status === 204 which lies about the Promise<T> return type; update the function that contains the res.status === 204 branch to use an explicit nullable return type (e.g., Promise<T | undefined>) or create a dedicated variant for 204 endpoints, remove the "as T" assertion and return plain undefined, and then update all callers of this function to handle the undefined case (or call the new non-null variant) so types and runtime behavior are consistent; locate the change at the res.status === 204 check and the call to res.json() to implement this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 25967f60-286b-4836-8d35-962561c4c773
⛔ Files ignored due to path filters (20)
packages/web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpackages/web/public/assets/empty-state/alerts-dark.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/alerts-light.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/api-keys-dark.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/api-keys-light.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/exposures-dark.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/exposures-light.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/integrations-dark.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/integrations-light.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/inventory-dark.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/inventory-light.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/logs-dark.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/logs-light.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/red-team-dark.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/red-team-light.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/risks-dark.svgis excluded by!**/*.svgpackages/web/public/assets/empty-state/risks-light.svgis excluded by!**/*.svgpackages/web/public/assets/rogue_logo.svgis excluded by!**/*.svgpackages/web/public/assets/wolf-favicon.svgis excluded by!**/*.svguv.lockis excluded by!**/*.lock
📒 Files selected for processing (186)
.bandit.yaml.github/actions/codestyle/action.yml.github/actions/web-build/action.yml.github/actions/web-codestyle/action.yml.github/pull_request_template.md.github/workflows/build.yml.github/workflows/pr.yml.github/workflows/release.yml.gitignore.mypy.iniexamples/mcp/tshirt_store_langgraph_mcp/__main__.pyhatch_build.pylefthook.yamlpackages/tui/internal/screens/scenarios/edit.gopackages/tui/internal/screens/scenarios/editor.gopackages/tui/internal/screens/scenarios/list.gopackages/tui/internal/screens/scenarios/scenario_types.gopackages/web/.npmrcpackages/web/.prettierignorepackages/web/.prettierrcpackages/web/components.jsonpackages/web/eslint.config.jspackages/web/index.htmlpackages/web/package.jsonpackages/web/src/api/client.tspackages/web/src/api/config-sync.tspackages/web/src/api/queries.tspackages/web/src/api/types.tspackages/web/src/api/ws.tspackages/web/src/components/app-sidebar.tsxpackages/web/src/components/business-context-card.tsxpackages/web/src/components/confirm-dialog.tsxpackages/web/src/components/empty-state.tsxpackages/web/src/components/live-conversation.tsxpackages/web/src/components/model-picker/catalog.tspackages/web/src/components/model-picker/dialog.tsxpackages/web/src/components/nav-main.tsxpackages/web/src/components/nav-secondary.tsxpackages/web/src/components/nav-user.tsxpackages/web/src/components/red-team/catalog-pane.tsxpackages/web/src/components/red-team/frameworks-dialog.tsxpackages/web/src/components/rogue-logo.tsxpackages/web/src/components/rogue-security-suggestion.tsxpackages/web/src/components/settings/settings-nav.tsxpackages/web/src/components/site-header.tsxpackages/web/src/components/target-agent-form.tsxpackages/web/src/components/theme-provider.tsxpackages/web/src/components/theme-toggle.tsxpackages/web/src/components/ui/badge.tsxpackages/web/src/components/ui/button.tsxpackages/web/src/components/ui/card.tsxpackages/web/src/components/ui/collapsible.tsxpackages/web/src/components/ui/dialog.tsxpackages/web/src/components/ui/dropdown-menu.tsxpackages/web/src/components/ui/input.tsxpackages/web/src/components/ui/label.tsxpackages/web/src/components/ui/scroll-area.tsxpackages/web/src/components/ui/select.tsxpackages/web/src/components/ui/separator.tsxpackages/web/src/components/ui/sheet.tsxpackages/web/src/components/ui/sidebar.tsxpackages/web/src/components/ui/sonner.tsxpackages/web/src/components/ui/switch.tsxpackages/web/src/components/ui/tabs.tsxpackages/web/src/components/ui/textarea.tsxpackages/web/src/components/ui/tooltip.tsxpackages/web/src/hooks/use-mobile.tspackages/web/src/lib/models-dev.tspackages/web/src/lib/protocols.tspackages/web/src/lib/red-team-catalog.tspackages/web/src/lib/utils.tspackages/web/src/main.tsxpackages/web/src/routes/__root.tsxpackages/web/src/routes/evaluations/$jobId.index.tsxpackages/web/src/routes/evaluations/$jobId.report.tsxpackages/web/src/routes/evaluations/$jobId.tsxpackages/web/src/routes/evaluations/index.tsxpackages/web/src/routes/evaluations/new.tsxpackages/web/src/routes/help.tsxpackages/web/src/routes/index.tsxpackages/web/src/routes/red-team/$jobId.index.tsxpackages/web/src/routes/red-team/$jobId.report.tsxpackages/web/src/routes/red-team/$jobId.tsxpackages/web/src/routes/red-team/configure.tsxpackages/web/src/routes/red-team/index.tsxpackages/web/src/routes/red-team/new.tsxpackages/web/src/routes/scenarios/index.tsxpackages/web/src/routes/scenarios/interview.tsxpackages/web/src/routes/settings.tsxpackages/web/src/routes/settings/api-keys.tsxpackages/web/src/routes/settings/backup.tsxpackages/web/src/routes/settings/general.tsxpackages/web/src/routes/settings/index.tsxpackages/web/src/routes/settings/models.tsxpackages/web/src/routes/settings/rogue-security.tsxpackages/web/src/stores/config.tspackages/web/src/stores/red-team.tspackages/web/src/stores/scenarios.tspackages/web/src/styles/globals.csspackages/web/tsconfig.jsonpackages/web/vite.config.tspyproject.tomlrogue/__init__.pyrogue/__main__.pyrogue/common/generic_agent_executor.pyrogue/common/generic_task_callback.pyrogue/common/litellm_config.pyrogue/common/logging/config.pyrogue/common/logging/intercept_handler.pyrogue/common/logging/safe_format.pyrogue/common/remote_agent_connection.pyrogue/common/tui_installer.pyrogue/common/update_checker.pyrogue/common/version.pyrogue/common/workdir_utils.pyrogue/evaluator_agent/a2a/a2a_evaluator_agent.pyrogue/evaluator_agent/base_evaluator_agent.pyrogue/evaluator_agent/mcp/mcp_evaluator_agent.pyrogue/evaluator_agent/mcp_utils.pyrogue/evaluator_agent/multi_turn/driver.pyrogue/evaluator_agent/multi_turn/goal_checker.pyrogue/evaluator_agent/multi_turn/run_multi_turn.pyrogue/evaluator_agent/openai_api/openai_api_evaluator_agent.pyrogue/evaluator_agent/policy_evaluation.pyrogue/evaluator_agent/python/python_evaluator_agent.pyrogue/evaluator_agent/red_team/a2a_red_team_attacker_agent.pyrogue/evaluator_agent/red_team/base_red_team_attacker_agent.pyrogue/evaluator_agent/red_team/factory.pyrogue/evaluator_agent/red_team/mcp_red_team_attacker_agent.pyrogue/evaluator_agent/red_team/openai_api_red_team_attacker_agent.pyrogue/evaluator_agent/red_team/python_red_team_attacker_agent.pyrogue/evaluator_agent/run_evaluator_agent.pyrogue/models/cli_input.pyrogue/models/prompt_injection.pyrogue/prompt_injection_evaluator/run_prompt_injection_evaluator.pyrogue/run_cli.pyrogue/run_server.pyrogue/run_tui.pyrogue/run_web.pyrogue/server/api/config.pyrogue/server/api/evaluation.pyrogue/server/api/fs.pyrogue/server/api/interview.pyrogue/server/api/llm.pyrogue/server/api/red_team.pyrogue/server/core/red_team_orchestrator.pyrogue/server/main.pyrogue/server/models/api_format.pyrogue/server/red_teaming/attacks/single_turn/context_poisoning.pyrogue/server/red_teaming/attacks/single_turn/goal_redirection.pyrogue/server/red_teaming/attacks/single_turn/gray_box.pyrogue/server/red_teaming/attacks/single_turn/input_bypass.pyrogue/server/red_teaming/attacks/single_turn/permission_escalation.pyrogue/server/red_teaming/catalog/vulnerabilities.pyrogue/server/red_teaming/models.pyrogue/server/red_teaming/orchestrator.pyrogue/server/red_teaming/report/generator.pyrogue/server/red_teaming/report/key_findings.pyrogue/server/red_teaming/report/tui_formatter.pyrogue/server/red_teaming/utils/dataset_loader.pyrogue/server/red_teaming/vulnerabilities/access_control.pyrogue/server/red_teaming/vulnerabilities/base_vulnerability.pyrogue/server/red_teaming/vulnerabilities/competitors.pyrogue/server/red_teaming/vulnerabilities/content_safety.pyrogue/server/red_teaming/vulnerabilities/intellectual_property.pyrogue/server/red_teaming/vulnerabilities/pii_leakage.pyrogue/server/red_teaming/vulnerabilities/technical_vulnerabilities.pyrogue/server/red_teaming/vulnerabilities/unbounded_consumption.pyrogue/server/services/deckard_service.pyrogue/server/services/evaluation_library.pyrogue/server/services/evaluation_service.pyrogue/server/services/job_store.pyrogue/server/services/llm_service.pyrogue/server/services/red_team_service.pyrogue/server/services/scenario_evaluation_service.pyrogue/server/websocket/manager.pyrogue/tests/models/test_cli_input.pyrogue/tests/models/test_evaluation_result.pyrogue/tests/test_deckard_service.pyrogue/tests/test_multi_turn.pyrogue/tests/test_python_evaluator_kwargs.pyrogue/tests/test_run_cli.pysdks/python/rogue_sdk/client.pysdks/python/rogue_sdk/tests/test_types.pysdks/python/rogue_sdk/types.pysdks/python/rogue_sdk/websocket.py
💤 Files with no reviewable changes (2)
- .bandit.yaml
- .mypy.ini
Comment on lines
+33
to
+43
| if (!res.ok) { | ||
| let detail: unknown = undefined; | ||
| try { | ||
| detail = await res.json(); | ||
| } catch { | ||
| try { | ||
| detail = await res.text(); | ||
| } catch { | ||
| /* ignore */ | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Response body stream consumed twice on error.
When res.json() fails on line 36, the response body stream is already consumed. The subsequent res.text() call on line 39 will fail or return empty because the body can only be read once. Consider cloning the response or reading as text first:
🐛 Proposed fix
if (!res.ok) {
let detail: unknown = undefined;
+ const text = await res.text();
try {
- detail = await res.json();
+ detail = JSON.parse(text);
} catch {
- try {
- detail = await res.text();
- } catch {
- /* ignore */
- }
+ detail = text || undefined;
}
throw new ApiError(res.status, `${res.status} ${res.statusText}`, detail);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/api/client.ts` around lines 33 - 43, The error handling
reads the response body twice (res.json() then res.text()), which fails because
the stream is consumed; modify the error branch to clone the Response before
multiple reads (use res.clone()) or read text first and then attempt JSON parse
from that text, so that the variable detail is populated reliably; update the
logic around the Response object used in this file (the res variable and the
detail assignment) to use the cloned response or text-then-JSON approach and
ensure both fallback attempts succeed.
Comment on lines
+325
to
+328
| export type WebSocketMessage = | ||
| | { type: "job_update"; job_id: string; data: JobUpdate } | ||
| | { type: "chat_update"; job_id: string; data: ChatUpdate } | ||
| | { type: string; job_id: string; data: Record<string, unknown> }; |
Contributor
There was a problem hiding this comment.
The fallback websocket arm breaks discrimination.
Because { type: string; ... } also matches "job_update" and "chat_update", narrowing on message.type won't reliably refine data to JobUpdate or ChatUpdate. Keep unknown payloads in a separate boundary type instead of mixing them into the app-level discriminated union.
Suggested fix
export type WebSocketMessage =
| { type: "job_update"; job_id: string; data: JobUpdate }
- | { type: "chat_update"; job_id: string; data: ChatUpdate }
- | { type: string; job_id: string; data: Record<string, unknown> };
+ | { type: "chat_update"; job_id: string; data: ChatUpdate };
+
+export interface UnknownWebSocketMessage {
+ type: string;
+ job_id: string;
+ data: Record<string, unknown>;
+}📝 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.
Suggested change
| export type WebSocketMessage = | |
| | { type: "job_update"; job_id: string; data: JobUpdate } | |
| | { type: "chat_update"; job_id: string; data: ChatUpdate } | |
| | { type: string; job_id: string; data: Record<string, unknown> }; | |
| export type WebSocketMessage = | |
| | { type: "job_update"; job_id: string; data: JobUpdate } | |
| | { type: "chat_update"; job_id: string; data: ChatUpdate }; | |
| export interface UnknownWebSocketMessage { | |
| type: string; | |
| job_id: string; | |
| data: Record<string, unknown>; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/api/types.ts` around lines 325 - 328, The current
WebSocketMessage union mixes a catch-all arm with the discriminant which
prevents proper type narrowing; split into distinct message types (e.g.,
JobUpdateMessage with type: "job_update" and data: JobUpdate, ChatUpdateMessage
with type: "chat_update" and data: ChatUpdate) and move the fallback into its
own boundary type (e.g., UnknownWebSocketMessage with type: string and data:
Record<string, unknown>), then recompose WebSocketMessage as a union of those
three so narrowing on message.type reliably refines data for job_update and
chat_update.
Comment on lines
+44
to
+52
| <Button variant="ghost" onClick={() => onOpenChange(false)} disabled={pending}> | ||
| {cancelLabel} | ||
| </Button> | ||
| <Button | ||
| variant={destructive ? "destructive" : "default"} | ||
| onClick={() => onConfirm()} | ||
| disabled={pending} | ||
| autoFocus | ||
| > |
Contributor
There was a problem hiding this comment.
Don't autofocus the confirm action.
Opening the dialog with focus already on Confirm means Enter can immediately execute the action, including destructive ones. Focus Cancel instead, or let the dialog manage initial focus.
Suggested fix
- <Button variant="ghost" onClick={() => onOpenChange(false)} disabled={pending}>
+ <Button
+ variant="ghost"
+ onClick={() => onOpenChange(false)}
+ disabled={pending}
+ autoFocus
+ >
{cancelLabel}
</Button>
<Button
variant={destructive ? "destructive" : "default"}
onClick={() => onConfirm()}
disabled={pending}
- autoFocus
>
{pending ? "Working…" : confirmLabel}
</Button>📝 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.
Suggested change
| <Button variant="ghost" onClick={() => onOpenChange(false)} disabled={pending}> | |
| {cancelLabel} | |
| </Button> | |
| <Button | |
| variant={destructive ? "destructive" : "default"} | |
| onClick={() => onConfirm()} | |
| disabled={pending} | |
| autoFocus | |
| > | |
| <Button | |
| variant="ghost" | |
| onClick={() => onOpenChange(false)} | |
| disabled={pending} | |
| autoFocus | |
| > | |
| {cancelLabel} | |
| </Button> | |
| <Button | |
| variant={destructive ? "destructive" : "default"} | |
| onClick={() => onConfirm()} | |
| disabled={pending} | |
| > | |
| {pending ? "Working…" : confirmLabel} | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/components/confirm-dialog.tsx` around lines 44 - 52, Remove
the autoFocus from the confirm Button to avoid executing actions by pressing
Enter on open; instead set initial focus to the cancel Button (the one using
cancelLabel and onOpenChange) or let the dialog manage focus automatically;
update the Button usage that references autoFocus, onConfirm, destructive,
pending, cancelLabel and onOpenChange so the cancel button receives focus (or
remove autoFocus entirely) and confirm no longer auto-focuses.
Comment on lines
+40
to
+53
| <button | ||
| type="button" | ||
| onClick={() => toggleFramework(f.id)} | ||
| className={cn( | ||
| "flex w-full items-center justify-between gap-3 px-3 py-2.5 text-left transition-colors hover:bg-accent", | ||
| on && "bg-accent/60", | ||
| )} | ||
| > | ||
| <span> | ||
| <span className="font-medium">{f.name}</span> | ||
| <span className="ml-2 text-xs text-muted-foreground">{f.description}</span> | ||
| </span> | ||
| {on && <IconCheck className="h-4 w-4 text-primary" />} | ||
| </button> |
Contributor
There was a problem hiding this comment.
Expose the selected state to assistive tech.
These row buttons behave like toggles, but the enabled state is only visual right now. Screen readers will announce the same control before and after selection, which makes multi-selecting frameworks much harder. Add aria-pressed={on} (or use a checkbox/toggle primitive).
Suggested fix
<li key={f.id}>
<button
type="button"
onClick={() => toggleFramework(f.id)}
+ aria-pressed={on}
className={cn(
"flex w-full items-center justify-between gap-3 px-3 py-2.5 text-left transition-colors hover:bg-accent",
on && "bg-accent/60",
)}
>📝 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.
Suggested change
| <button | |
| type="button" | |
| onClick={() => toggleFramework(f.id)} | |
| className={cn( | |
| "flex w-full items-center justify-between gap-3 px-3 py-2.5 text-left transition-colors hover:bg-accent", | |
| on && "bg-accent/60", | |
| )} | |
| > | |
| <span> | |
| <span className="font-medium">{f.name}</span> | |
| <span className="ml-2 text-xs text-muted-foreground">{f.description}</span> | |
| </span> | |
| {on && <IconCheck className="h-4 w-4 text-primary" />} | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => toggleFramework(f.id)} | |
| aria-pressed={on} | |
| className={cn( | |
| "flex w-full items-center justify-between gap-3 px-3 py-2.5 text-left transition-colors hover:bg-accent", | |
| on && "bg-accent/60", | |
| )} | |
| > | |
| <span> | |
| <span className="font-medium">{f.name}</span> | |
| <span className="ml-2 text-xs text-muted-foreground">{f.description}</span> | |
| </span> | |
| {on && <IconCheck className="h-4 w-4 text-primary" />} | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/components/red-team/frameworks-dialog.tsx` around lines 40 -
53, The toggle buttons rendering each framework (inside the button that calls
toggleFramework(f.id) and conditionally shows IconCheck) only update visually;
add an accessible state attribute by setting aria-pressed={on} on that button
element so screen readers announce the pressed state (or replace with a proper
checkbox/toggle primitive if you prefer); ensure the attribute uses the same
`on` boolean used for the visual state and keep the existing onClick handler
toggleFramework(f.id).
Comment on lines
+39
to
+44
| export const Button = forwardRef<HTMLButtonElement, ButtonProps>( | ||
| ({ className, variant, size, asChild = false, ...props }, ref) => { | ||
| const Comp = asChild ? Slot : "button"; | ||
| return ( | ||
| <Comp ref={ref} className={cn(buttonVariants({ variant, size }), className)} {...props} /> | ||
| ); |
Contributor
There was a problem hiding this comment.
Default native buttons to type="button".
When this component is used inside a form, the native <button> path currently defaults to HTML's submit behavior. That makes non-submit action buttons unexpectedly submit forms unless every caller remembers to override type.
Suggested fix
export const Button = forwardRef<HTMLButtonElement, ButtonProps>(
- ({ className, variant, size, asChild = false, ...props }, ref) => {
+ ({ className, variant, size, asChild = false, type, ...props }, ref) => {
const Comp = asChild ? Slot : "button";
return (
- <Comp ref={ref} className={cn(buttonVariants({ variant, size }), className)} {...props} />
+ <Comp
+ ref={ref}
+ className={cn(buttonVariants({ variant, size }), className)}
+ type={asChild ? undefined : (type ?? "button")}
+ {...props}
+ />
);
},
);📝 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.
Suggested change
| export const Button = forwardRef<HTMLButtonElement, ButtonProps>( | |
| ({ className, variant, size, asChild = false, ...props }, ref) => { | |
| const Comp = asChild ? Slot : "button"; | |
| return ( | |
| <Comp ref={ref} className={cn(buttonVariants({ variant, size }), className)} {...props} /> | |
| ); | |
| export const Button = forwardRef<HTMLButtonElement, ButtonProps>( | |
| ({ className, variant, size, asChild = false, type, ...props }, ref) => { | |
| const Comp = asChild ? Slot : "button"; | |
| return ( | |
| <Comp | |
| ref={ref} | |
| className={cn(buttonVariants({ variant, size }), className)} | |
| type={asChild ? undefined : (type ?? "button")} | |
| {...props} | |
| /> | |
| ); | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/components/ui/button.tsx` around lines 39 - 44, The Button
component currently renders a native "button" when asChild is false without
defaulting type, causing forms to be submitted unexpectedly; update the render
so that when Comp is the native "button" and no type is provided in props (i.e.,
asChild is false and props.type is undefined) it supplies type="button" by
default before spreading props. Locate Button, the asChild/Comp logic and the
JSX return (references: Button, asChild, Comp, Slot, buttonVariants, cn) and
implement the conditional defaulting of the type prop for the native button
path.
Comment on lines
+42
to
+45
| def _escape_for_script_literal(raw: str) -> str: | ||
| """Escape ``\\`` and ``"`` so the prompt is a valid AppleScript / PowerShell | ||
| double-quoted literal. Both languages use the same escape rules.""" | ||
| return raw.replace("\\", "\\\\").replace('"', '\\"') |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PowerShell string literals passed via -Command, does backslash escape "inside a double-quoted string, or should quoting use backtick / single-quoted strings? Please use officialabout_Quoting_Rules documentation.
💡 Result:
In PowerShell string literals, including those passed via -Command (after command-line parsing), the backtick () escapes " inside a double-quoted string. Backslash () does not escape " in PowerShell strings; it is treated literally. Official about_Quoting_Rules documentation states: "To force PowerShell to interpret a double quotation mark literally, use a backtick character. This prevents PowerShell from interpreting the quotation mark as a string delimiter." Example from docs: "Use a quotation mark (") to begin a string." # outputs: Use a quotation mark (") to begin a string. When passing via powershell.exe -Command "...", the command-line parser requires \ to escape " for the shell (e.g., cmd.exe), turning " into literal " in the resulting PowerShell code. Then, inside that code, use backtick to escape inner ": e.g., powershell -Command "Write-Output "She said \"hello\"."" Alternatives: Use single-quoted strings ('...') for literals (no expansion, escape ' as ''), or here-strings (@'...'@ or @"..."@).
Citations:
- 1: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules?view=powershell-7.4
- 2: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules?view=powershell-7.6
- 3: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules?view=powershell-7.2
- 4: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules?view=powershell-7.4&viewFallbackFrom=powershell-7.3
- 5: https://github.com/MicrosoftDocs/PowerShell-Docs/blob/main/reference/7.6/Microsoft.PowerShell.Core/About/about_Quoting_Rules.md
- 6: https://stackoverflow.com/questions/74862959/escape-double-quotes-in-powershell-command
- 7: https://stackoverflow.com/questions/34276662/powershell-escaping-string-passed-to-child-process
- 8: https://stackoverflow.com/questions/71576784/powershell-escaping-rules-for-cli-calls
🏁 Script executed:
git ls-files | grep -E "fs\.py|api" | head -20Repository: qualifire-dev/rogue
Length of output: 1852
🏁 Script executed:
cat -n rogue/server/api/fs.py | head -200Repository: qualifire-dev/rogue
Length of output: 1852
Fix PowerShell string escaping in _escape_for_script_literal().
The function incorrectly escapes quotes using \" for both AppleScript and PowerShell. According to PowerShell's official about_Quoting_Rules, backslash does not escape quotes in double-quoted strings—backtick () is the escape character. Additionally, single-quoted strings ('...') are safer for literals as they prevent variable expansion and only require escaping single quotes as ''`.
Split the function into platform-specific variants: _escape_for_applescript_literal() using \" and _escape_for_powershell_literal() using single-quoted strings (which only need to escape ' as ''). Update all PowerShell command construction to use single-quoted strings instead of double-quoted strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rogue/server/api/fs.py` around lines 42 - 45, The current
_escape_for_script_literal() incorrectly uses backslash-quote escapes for
PowerShell; split it into platform-specific functions: implement
_escape_for_applescript_literal(raw: str) that keeps the existing backslash and
\" escaping for AppleScript, and implement _escape_for_powershell_literal(raw:
str) that returns a single-quoted PowerShell-safe literal by escaping single
quotes as two single quotes (raw.replace("'", "''")); then update all places
that previously called _escape_for_script_literal() for PowerShell command
construction to call _escape_for_powershell_literal() and ensure PowerShell
commands are built using single-quoted literals instead of double-quoted ones.
Comment on lines
+178
to
+211
| @router.post("/pick-file", response_model=PickedFile) | ||
| async def pick_file(request: PickFileRequest) -> PickedFile: | ||
| """Open the OS-native file dialog on the server's display and return the | ||
| absolute path the user selected. | ||
|
|
||
| Returns ``{path: null}`` when the user cancels. Errors when the OS picker | ||
| helper isn't available (e.g. headless Linux without zenity) — clients | ||
| should fall back to manual path entry. | ||
| """ | ||
| system = platform.system() | ||
| extensions = _normalise_extensions(request.extensions) | ||
| prompt = request.prompt or "Select a file" | ||
|
|
||
| try: | ||
| if system == "Darwin": | ||
| path = await _pick_macos(prompt, extensions) | ||
| elif system == "Linux": | ||
| path = await _pick_linux(prompt, extensions) | ||
| elif system == "Windows": | ||
| path = await _pick_windows(prompt, extensions) | ||
| else: | ||
| raise HTTPException( | ||
| status_code=501, | ||
| detail=f"Native file picker not implemented for {system}", | ||
| ) | ||
| except FileNotFoundError as exc: | ||
| raise HTTPException( | ||
| status_code=503, | ||
| detail=f"Native picker helper missing: {exc.filename or exc}", | ||
| ) from exc | ||
| except asyncio.TimeoutError: | ||
| return PickedFile(path=None) | ||
|
|
||
| return PickedFile(path=path) |
Contributor
There was a problem hiding this comment.
Restrict this endpoint to loopback or explicit local-only mode.
/fs/pick-file lets the caller open a native dialog on the server desktop and receive an absolute path back. There is no auth or client-locality check here, so if the API is ever reachable beyond localhost this becomes a remote GUI/path-disclosure primitive.
Suggested fix
-from fastapi import APIRouter, HTTPException
+from fastapi import APIRouter, HTTPException, Request
@@
-@router.post("/pick-file", response_model=PickedFile)
-async def pick_file(request: PickFileRequest) -> PickedFile:
+@router.post("/pick-file", response_model=PickedFile)
+async def pick_file(http_request: Request, request: PickFileRequest) -> PickedFile:
@@
+ client_host = http_request.client.host if http_request.client else None
+ if client_host not in {"127.0.0.1", "::1"}:
+ raise HTTPException(
+ status_code=403,
+ detail="Native file picker is only available from localhost",
+ )
+
system = platform.system()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rogue/server/api/fs.py` around lines 178 - 211, The pick_file endpoint
(router.post("/pick-file") -> pick_file) must be restricted to local-only
access: add a check at the start of pick_file that validates the incoming
request is from loopback (e.g. inspect request.client.host or use the FastAPI
Request dependency) or that an explicit local-only server mode flag is enabled;
if the client is not local and local-only mode is not set, raise
HTTPException(403). Also wire this check to the existing server config (a
boolean like local_only or allow_remote_file_picker) so the behavior is
configurable and documented, and ensure tests cover both loopback requests and
remote-denied responses.
Comment on lines
+108
to
+117
| # Configure metric with business context for context-aware detection. | ||
| # `business_context` isn't on every metric subclass; setattr sidesteps | ||
| # ty's protocol-based attribute resolution (which can't see optional | ||
| # subclass fields here). | ||
| if ( | ||
| hasattr(self.metric, "business_context") | ||
| and not self.metric.business_context | ||
| and self.business_context | ||
| self.business_context | ||
| and hasattr(self.metric, "business_context") | ||
| and not getattr(self.metric, "business_context", None) | ||
| ): | ||
| self.metric.business_context = self.business_context | ||
| setattr(self.metric, "business_context", self.business_context) |
Contributor
There was a problem hiding this comment.
Drop the hasattr guard here.
setattr already handles metrics that do not declare business_context. With the current check, those metrics will never receive the injected context, which defeats the compatibility fix this change is trying to make.
Suggested fix
- if (
- self.business_context
- and hasattr(self.metric, "business_context")
- and not getattr(self.metric, "business_context", None)
- ):
+ if self.business_context and not getattr(self.metric, "business_context", None):
setattr(self.metric, "business_context", self.business_context)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rogue/server/red_teaming/vulnerabilities/base_vulnerability.py` around lines
108 - 117, Remove the hasattr guard so metrics that don't declare
business_context still get the injected context: in the block around
self.business_context, metric, and setattr in base_vulnerability.py, change the
condition to check only that self.business_context is truthy and that
getattr(self.metric, "business_context", None) is falsy, then call
setattr(self.metric, "business_context", self.business_context); keep references
to metric, business_context, and setattr to locate and update the code.
Comment on lines
+134
to
153
| max_wait = _ws_wait_seconds() | ||
| if max_wait > 0: | ||
| wait_interval = 0.1 | ||
| waited = 0.0 | ||
| while waited < max_wait: | ||
| if self.websocket_manager.has_connections(job_id): | ||
| logger.info( | ||
| f"WebSocket client connected for job {job_id}, starting scan", | ||
| ) | ||
| break | ||
| await asyncio.sleep(wait_interval) | ||
| waited += wait_interval | ||
| else: | ||
| logger.warning( | ||
| f"No WebSocket client connected after {max_wait}s, " | ||
| f"proceeding anyway for job {job_id}", | ||
| ) | ||
| break | ||
| await asyncio.sleep(wait_interval) | ||
| waited += wait_interval | ||
| else: | ||
| logger.warning( | ||
| f"No WebSocket client connected after {max_wait}s, " | ||
| f"proceeding anyway for job {job_id}", | ||
| ) | ||
|
|
||
| # Create and run orchestrator | ||
| orchestrator = RedTeamOrchestrator( |
Contributor
There was a problem hiding this comment.
Honor cancellation before starting the scan.
The new WS wait window makes pre-start cancellation much more likely, but this code never re-checks whether the job was cancelled before creating the orchestrator. A job can be marked CANCELLED and still run to completion in the background.
Suggested fix
max_wait = _ws_wait_seconds()
if max_wait > 0:
wait_interval = 0.1
waited = 0.0
while waited < max_wait:
+ if job.status == EvaluationStatus.CANCELLED:
+ logger.info("Red team job cancelled before scan start", extra={"job_id": job_id})
+ return
if self.websocket_manager.has_connections(job_id):
logger.info(
f"WebSocket client connected for job {job_id}, starting scan",
)
break
@@
logger.warning(
f"No WebSocket client connected after {max_wait}s, "
f"proceeding anyway for job {job_id}",
)
+
+ if job.status == EvaluationStatus.CANCELLED:
+ logger.info("Red team job cancelled before orchestrator start", extra={"job_id": job_id})
+ return
# Create and run orchestrator
orchestrator = RedTeamOrchestrator(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rogue/server/services/red_team_service.py` around lines 134 - 153, After the
WS wait loop, re-check the job's current status before creating the
RedTeamOrchestrator: call the service/repo method that fetches the job by job_id
(e.g. self.job_repo.get(job_id) or equivalent), check if its status equals the
CANCELLED enum/state, and if so log and exit/return without instantiating or
running RedTeamOrchestrator; place this check immediately after the
websocket_manager.has_connections wait and before the RedTeamOrchestrator(...)
creation to ensure canceled jobs are not started.
Comment on lines
245
to
+248
| def _notify_job_update(self, job: RedTeamJob): | ||
| """Notify websocket clients about job status update.""" | ||
| # Persist on every state change so scans survive restarts. | ||
| self._store.save(job) |
Contributor
There was a problem hiding this comment.
Don't let job persistence failures take down live scans.
_store.save(job) now runs on every update path, but any filesystem/serialization error here will bubble straight into run_job() and can flip a healthy scan to FAILED. This should be isolated behind local error handling so realtime updates still continue.
Suggested fix
def _notify_job_update(self, job: RedTeamJob):
"""Notify websocket clients about job status update."""
# Persist on every state change so scans survive restarts.
- self._store.save(job)
+ try:
+ self._store.save(job)
+ except Exception:
+ logger.exception(
+ "Failed to persist red-team job update",
+ extra={"job_id": job.job_id},
+ )
message = WebSocketMessage(
type=WebSocketEventType.JOB_UPDATE,
job_id=job.job_id,📝 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.
Suggested change
| def _notify_job_update(self, job: RedTeamJob): | |
| """Notify websocket clients about job status update.""" | |
| # Persist on every state change so scans survive restarts. | |
| self._store.save(job) | |
| def _notify_job_update(self, job: RedTeamJob): | |
| """Notify websocket clients about job status update.""" | |
| # Persist on every state change so scans survive restarts. | |
| try: | |
| self._store.save(job) | |
| except Exception: | |
| logger.exception( | |
| "Failed to persist red-team job update", | |
| extra={"job_id": job.job_id}, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rogue/server/services/red_team_service.py` around lines 245 - 248, The call
to self._store.save(job) inside _notify_job_update can raise
filesystem/serialization errors and should not propagate to run_job(); wrap the
save call in a try/except that catches Exception, logs the error using the
service logger (e.g., self._logger or module logger) with context about the job
id/state, and suppresses the exception so notification and runtime state updates
continue; do not re-raise, but consider adding a metric/log level for persistent
failures to surface them later.
yuval-qf
approved these changes
Apr 30, 2026
Comment on lines
+290
to
+293
| _os.environ.setdefault( | ||
| "ROGUE_WORKDIR", | ||
| str(args.workdir.resolve()), | ||
| ) |
Collaborator
There was a problem hiding this comment.
You receive the workdir as a parameter, I'm not sure you need this as env as well
| server_process.join() | ||
| sys.exit(exit_code) | ||
| elif args.mode == "web": | ||
| sys.exit(run_web(args)) |
Collaborator
There was a problem hiding this comment.
consider adding a --with-server so that the server and web-ui can be executed in a single command.
Like the tui and cli have
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rogue/__main__.py (1)
307-315:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame workdir/env ordering bug remains here.
This block still runs after the default
args.mode is Nonepath exits, so the fallback server never seesROGUE_WORKDIR.setdefault()also means an existing environment value can override an explicit--workdir.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rogue/__main__.py` around lines 307 - 315, The ROGUE_WORKDIR env var is being set with os.environ.setdefault after the code path that exits when args.mode is None, and setdefault allows an existing env var to override an explicit --workdir; move the workdir setup earlier (before any early exits based on args.mode) and replace os.environ.setdefault(...) with an unconditional assignment os.environ["ROGUE_WORKDIR"] = str(args.workdir.resolve()), ensuring args.workdir.mkdir(...) still runs before this assignment so the resolved path exists; update the block containing args.workdir.mkdir, _os.environ.setdefault, and the ROGUE_WORKDIR logic accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@rogue/__main__.py`:
- Around line 307-315: The ROGUE_WORKDIR env var is being set with
os.environ.setdefault after the code path that exits when args.mode is None, and
setdefault allows an existing env var to override an explicit --workdir; move
the workdir setup earlier (before any early exits based on args.mode) and
replace os.environ.setdefault(...) with an unconditional assignment
os.environ["ROGUE_WORKDIR"] = str(args.workdir.resolve()), ensuring
args.workdir.mkdir(...) still runs before this assignment so the resolved path
exists; update the block containing args.workdir.mkdir, _os.environ.setdefault,
and the ROGUE_WORKDIR logic accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3448533c-f58e-4b45-ab62-c110fb3e3dd6
📒 Files selected for processing (2)
rogue/__main__.pysdks/python/rogue_sdk/types.py
✅ Files skipped from review due to trivial changes (1)
- sdks/python/rogue_sdk/types.py
CI installs (test, codestyle, rogue, import-time workflows) ran `uv pip install -e .` without first building the web SPA, which caused hatchling to abort with `FileNotFoundError: Forced include not found: packages/web/dist`. The static `[tool.hatch.build.targets.*.force-include]` tables ran independently of the build hook's pnpm-missing fallback, so the graceful warning never had a chance to take effect. Move the force-include into `hatch_build.py` so it's only set when the dist actually exists. The Build workflow still gets the bundled UI (it runs the web build first); the lighter workflows install successfully without pnpm. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hatch_build.py`:
- Line 38: The current staleness check that sets needs_build (the expression
using dist_index.is_file() or _is_stale(dist_index, src_dir)) misses other
pnpm/vite inputs so UI assets can be stale; update the logic to consider
additional inputs (package.json, pnpm-lock.yaml, any vite.config.* files,
index.html, and the public/ tree) by extending _is_stale (or adding a helper
like _is_stale_with_inputs) to accept and check a list of paths/patterns and use
it wherever _is_stale is called (including the other occurrence around the 87-94
block) so needs_build becomes: not dist_index.is_file() or _is_stale(dist_index,
src_dir, extra_inputs) and ensure the implementation recursively checks mtimes
for public/** and globs for vite.config.*.
- Around line 33-35: The code currently sets skip_build =
bool(os.environ.get("ROGUE_SKIP_WEB_BUILD")) which treats any non-empty string
(e.g. "0" or "false") as True; change this to parse the ROGUE_SKIP_WEB_BUILD
value explicitly (e.g. val = os.environ.get("ROGUE_SKIP_WEB_BUILD",
"").strip().lower(); skip_build = val in ("1","true","yes") or use
distutils.util.strtobool on val) so that "0", "false", "no" (and empty) evaluate
to False and "1", "true", "yes" evaluate to True, then keep the existing
self.app.display_info call when skip_build is True.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1d8287cd-b741-40dd-aa1c-2ed9323e725c
📒 Files selected for processing (2)
hatch_build.pypyproject.toml
Comment on lines
+33
to
+35
| skip_build = bool(os.environ.get("ROGUE_SKIP_WEB_BUILD")) | ||
| if skip_build: | ||
| self.app.display_info("ROGUE_SKIP_WEB_BUILD set; skipping web build.") |
Contributor
There was a problem hiding this comment.
Parse ROGUE_SKIP_WEB_BUILD explicitly instead of using bool(...).
bool(os.environ.get(...)) treats "0" and "false" as True, so a caller can accidentally skip the web build and omit the UI.
Suggested fix
- skip_build = bool(os.environ.get("ROGUE_SKIP_WEB_BUILD"))
+ skip_build = os.environ.get("ROGUE_SKIP_WEB_BUILD", "").lower() in {
+ "1",
+ "true",
+ "yes",
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hatch_build.py` around lines 33 - 35, The code currently sets skip_build =
bool(os.environ.get("ROGUE_SKIP_WEB_BUILD")) which treats any non-empty string
(e.g. "0" or "false") as True; change this to parse the ROGUE_SKIP_WEB_BUILD
value explicitly (e.g. val = os.environ.get("ROGUE_SKIP_WEB_BUILD",
"").strip().lower(); skip_build = val in ("1","true","yes") or use
distutils.util.strtobool on val) so that "0", "false", "no" (and empty) evaluate
to False and "1", "true", "yes" evaluate to True, then keep the existing
self.app.display_info call when skip_build is True.
| self.app.display_info("ROGUE_SKIP_WEB_BUILD set; skipping web build.") | ||
|
|
||
| if not skip_build and web_dir.is_dir(): | ||
| needs_build = not dist_index.is_file() or _is_stale(dist_index, src_dir) |
Contributor
There was a problem hiding this comment.
Broaden the staleness check before reusing dist/.
Right now a wheel can reuse stale web assets when files outside packages/web/src change. pnpm build also depends on inputs like package.json, pnpm-lock.yaml, vite.config.*, index.html, and public/**, so local packaging can silently ship an outdated UI.
Suggested fix
- src_dir = web_dir / "src"
+ build_inputs = (
+ web_dir / "src",
+ web_dir / "public",
+ web_dir / "index.html",
+ web_dir / "package.json",
+ web_dir / "pnpm-lock.yaml",
+ web_dir / "tsconfig.json",
+ web_dir / "vite.config.ts",
+ )
skip_build = bool(os.environ.get("ROGUE_SKIP_WEB_BUILD"))
if skip_build:
self.app.display_info("ROGUE_SKIP_WEB_BUILD set; skipping web build.")
if not skip_build and web_dir.is_dir():
- needs_build = not dist_index.is_file() or _is_stale(dist_index, src_dir)
+ needs_build = not dist_index.is_file() or _is_stale(
+ dist_index, build_inputs
+ )-def _is_stale(dist_index: Path, src_dir: Path) -> bool:
- if not src_dir.is_dir():
- return False
+def _is_stale(dist_index: Path, inputs: tuple[Path, ...]) -> bool:
dist_mtime = dist_index.stat().st_mtime
- for path in src_dir.rglob("*"):
- if path.is_file() and path.stat().st_mtime > dist_mtime:
- return True
+ for root in inputs:
+ if root.is_file() and root.stat().st_mtime > dist_mtime:
+ return True
+ if root.is_dir():
+ for path in root.rglob("*"):
+ if path.is_file() and path.stat().st_mtime > dist_mtime:
+ return True
return FalseAlso applies to: 87-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hatch_build.py` at line 38, The current staleness check that sets needs_build
(the expression using dist_index.is_file() or _is_stale(dist_index, src_dir))
misses other pnpm/vite inputs so UI assets can be stale; update the logic to
consider additional inputs (package.json, pnpm-lock.yaml, any vite.config.*
files, index.html, and the public/ tree) by extending _is_stale (or adding a
helper like _is_stale_with_inputs) to accept and check a list of paths/patterns
and use it wherever _is_stale is called (including the other occurrence around
the 87-94 block) so needs_build becomes: not dist_index.is_file() or
_is_stale(dist_index, src_dir, extra_inputs) and ensure the implementation
recursively checks mtimes for public/** and globs for vite.config.*.
After the build-hook fix unblocked the install step, three real failures
surfaced on the PR:
- ruff format: examples/tshirt_store_agent/test_client.py used double-quoted
f-string with double-quoted inner string; reformat to inner single-quotes.
- ruff lint:
- I001 in two examples/tshirt_store_langgraph_agent/* files (auto-fixed
import order).
- S104 in rogue/__main__.py: bandit-style `# nosec B104` is invisible to
ruff; switch to ruff-native `# noqa: S104`.
- pytest: test_deckard_service.test_payload_includes_conversations_array
expected `metadata == {"expected_outcome": ...}` but the deckard payload
now also carries `attempt_index` / `attempts_total` (added so the platform
can render "attempt N of M"). Update the assertion to match the current
payload shape.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rogue/__main__.py (1)
265-315:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ROGUE_WORKDIRstill doesn’t reliably follow--workdirThis remains broken in two ways:
- the env setup is after the default-mode early-exit path, so
args.mode is Nonenever getsROGUE_WORKDIR;setdefaultcan ignore an explicit--workdirwhenROGUE_WORKDIRis already set.💡 Proposed fix
import asyncio +import os import subprocess # noqa: S404 import sys import time @@ configure_logger(args.debug, file_path=log_file_path) + + # Ensure workdir exists and is propagated to server/web flows. + args.workdir.mkdir(exist_ok=True, parents=True) + os.environ["ROGUE_WORKDIR"] = str(args.workdir.resolve()) @@ - # Handle regular modes (ui, cli, server, tui) - args.workdir.mkdir(exist_ok=True, parents=True) - # Make the resolved workdir available to the FastAPI server (for the - # JSON-per-job persistence layer + config sync endpoints). - import os as _os - - _os.environ.setdefault( - "ROGUE_WORKDIR", - str(args.workdir.resolve()), - ) + # Handle regular modes (web, cli, server, tui)As per coding guidelines, "Use
isortconventions for import organization".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rogue/__main__.py` around lines 265 - 315, Default-mode path doesn't get the intended ROGUE_WORKDIR and setdefault can be overridden by an existing env var; move the workdir setup (ensure args.workdir.mkdir(...)) and the environment assignment before the "if args.mode is None" block and replace os.environ.setdefault(...) with an unconditional assignment os.environ["ROGUE_WORKDIR"] = str(args.workdir.resolve()) so the resolved --workdir from args is always used by the server/TUI; keep the import of os (as _os or os) near other imports per isort conventions and refer to args.mode, args.workdir, and the run_server/run_rogue_tui flow when relocating the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@rogue/__main__.py`:
- Around line 265-315: Default-mode path doesn't get the intended ROGUE_WORKDIR
and setdefault can be overridden by an existing env var; move the workdir setup
(ensure args.workdir.mkdir(...)) and the environment assignment before the "if
args.mode is None" block and replace os.environ.setdefault(...) with an
unconditional assignment os.environ["ROGUE_WORKDIR"] =
str(args.workdir.resolve()) so the resolved --workdir from args is always used
by the server/TUI; keep the import of os (as _os or os) near other imports per
isort conventions and refer to args.mode, args.workdir, and the
run_server/run_rogue_tui flow when relocating the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a77101c0-247d-4d68-a758-ddf5c3434a9d
📒 Files selected for processing (5)
examples/tshirt_store_agent/test_client.pyexamples/tshirt_store_langgraph_agent/__init__.pyexamples/tshirt_store_langgraph_agent/shirtify_langgraph_agent.pyrogue/__main__.pyrogue/tests/test_deckard_service.py
✅ Files skipped from review due to trivial changes (3)
- examples/tshirt_store_langgraph_agent/shirtify_langgraph_agent.py
- examples/tshirt_store_langgraph_agent/init.py
- examples/tshirt_store_agent/test_client.py
Lockfile is generated by pnpm and should never be reformatted; CI's `prettier --check .` was flagging it, breaking the Web codestyle job. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`packages/web/src/routeTree.gen.ts` is produced by `tsr generate` and is gitignored, so a fresh CI checkout doesn't have it. The codestyle action ran `tsc --noEmit` directly, which fails to resolve `./routeTree.gen` and collapses every `createFileRoute(...)` into a `(arg: undefined)` signature — hence the avalanche of TS2345 errors on the route files. The web-build action sidesteps this because `pnpm build` is `pnpm routes && tsc --noEmit && vite build`. Use `pnpm typecheck` (which is `pnpm routes && tsc --noEmit`) here for the same effect, matching what a developer would run locally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Motivation and Context
Type of Change
Changes Made
Screenshots/Examples (if applicable)
Checklist
uv run black .to format my codeuv run flake8 .and fixed all issuesuv run mypy --config-file .mypy.ini .and addressed type checking issuesuv run bandit -c .bandit.yaml -r .for security checksuv run pytestand all tests passTesting
Test Configuration:
Test Steps:
1.
2.
3.
Additional Notes
Related Issues/PRs