Dev web UI implementation , + optimizations in semantic caching - #6
Merged
Conversation
…ix 15s→180.0s assertion
- Initialize Express app with cors, cookie-parser, and dotenv - Configure connection to Postgres via pg Pool - Add startup DB migration for 'users' table - Create users.js database accessor methods - Add /health endpoint
- Add POST /api/search/enqueue to proxy queries to warden - Associate session ID automatically with auth'd username - Add GET /api/admin/users for user management - Add PATCH /api/admin/users/:id/role to change privileges - Add GET /api/admin/health to proxy Warden health check
- Bootstrap Vite project - Establish CSS design system (glassmorphism variables, dark theme) - Create axios API client configured with proxy - Set up React Router for primary routes /login, /register, /search, /admin - Implement Auth context to manage global session state - Create ProtectedRoute and Auth-aware NavBar components
- Create production Dockerfile for Node.js API - Create multi-stage build Dockerfile for React UI with Nginx - Add custom Nginx config for SPA routing and sb_api proxy - Update docker-compose.yml to include sb_api and sb_ui services
…ization - Integrated ConversationTurn persistence in UI via new /api/search/history endpoint. - Updated Search UI to survive 429 Rate Limit errors without aborting search state. - Tuned Warden rate limits to 25 RPS / 100 Burst for better polling support. - Fixed critical AttributeError in Python worker's argument handling. - Integrated log collection for all dockerized services.
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
✅ Created PR with unit tests: #7 |
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 (2)
searchboost_warden/src/relay.rs (1)
53-58:⚠️ Potential issue | 🟡 MinorUpdate stale rate-limit comment to match new values.
Line 53 still documents
10/20, but Lines 57-58 now apply25/100. This can mislead future tuning/debugging.🧹 Proposed fix
- // Configure rate limiting: 10 requests per second, with a burst fallback of 20+ // Configure rate limiting: 25 requests per second, with a burst fallback of 100🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_warden/src/relay.rs` around lines 53 - 58, Update the stale comment above the governor_conf configuration to reflect the actual rate-limit values being set (25 requests per second with a burst size of 100) so it matches the GovernorConfigBuilder usage; locate the comment near governor_conf and replace the "10 requests per second, with a burst fallback of 20" text with a short, accurate description like "25 requests per second, with a burst fallback of 100" referencing per_second and burst_size for clarity.searchboost_service/searchboost_src/configurator.py (1)
204-210:⚠️ Potential issue | 🟠 MajorContainer name mismatch:
sb-searxngvssb_searxng.Line 50 changed the default
SearchSettings.hostto"sb-searxng", but line 206 still checks for"sb_searxng"in the container names list. This means local host remapping won't work for the SearXNG host.🐛 Proposed fix
if not self._is_docker: current_host = final_data.get("host") - container_names = ["sb_redis", "sb_db", "sb_ollama", "sb_warden", "sb_searxng"]+ container_names = ["sb_redis", "sb_db", "sb_ollama", "sb_warden", "sb-searxng"] if current_host in container_names:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_service/searchboost_src/configurator.py` around lines 204 - 210, Configurator's local host remapping fails for SearXNG due to a container name mismatch: the container_names list still contains "sb_searxng" while SearchSettings.host default was changed to "sb-searxng"; update the container_names in the block guarded by self._is_docker (the list assigned to container_names referenced by current_host) to include the hyphenated "sb-searxng" (or replace the underscore variant) so the conditional if current_host in container_names correctly matches and final_data["host"] is remapped to self._host; keep the debug call self._logger.debug(...) intact to log the remapping.
🟠 Major comments (24)
LICENSE-1-661 (1)
1-661:⚠️ Potential issue | 🟠 MajorUpdate package metadata to declare AGPL-3.0 license across all modules.
The project's AGPL-3.0 license is correctly declared in the LICENSE file and source code headers, but package metadata is inconsistent:
- searchboost_api/package.json (line 13): declares
"license": "ISC"— this contradicts the actual AGPL-3.0 license and will be published to npm with wrong license metadata.- searchboost_ui/package.json: missing license field.
- searchboost_warden/Cargo.toml: missing license field.
Update all three manifest files to declare
"license": "AGPL-3.0"(npm/Node) orlicense = "AGPL-3.0"(Cargo) before merge to prevent distribution of conflicting license information.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LICENSE` around lines 1 - 661, Package metadata declares inconsistent licensing: change searchboost_api/package.json's "license" value from "ISC" to "AGPL-3.0", add a "license": "AGPL-3.0" field to searchboost_ui/package.json, and add license = "AGPL-3.0" to searchboost_warden/Cargo.toml so all manifests match the project's AGPL-3.0 LICENSE; update only the respective manifest files and ensure JSON/Cargo syntax remains valid.searchboost_api/src/db/history.js-8-17 (1)
8-17:⚠️ Potential issue | 🟠 MajorEscape
LIKEmetacharacters in username-derived session prefix.Line 8 builds a
LIKEpattern from rawusername. Ifusernamecontains%or_, the query can match other users’ sessions.🔐 Proposed fix
async function getSessions(username) { - const prefix = `SB-SESSION-${username}%`;+ const escapedUsername = username.replace(/[\\%_]/g, '\\$&');+ const prefix = `SB-SESSION-${escapedUsername}-%`; try { const result = await pool.query( `SELECT session_id, MAX(created_at) as last_activity FROM conversation_turns - WHERE session_id LIKE $1+ WHERE session_id LIKE $1 ESCAPE '\\' GROUP BY session_id ORDER BY last_activity DESC`, [prefix] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/src/db/history.js` around lines 8 - 17, The LIKE pattern built into variable prefix in history.js uses raw username so percent/underscore in usernames will act as wildcards; escape backslashes, percent and underscore in username (e.g., replace \ with \\ then % with \% and _ with \_) before building prefix, then pass that escaped prefix to pool.query and update the SQL to use LIKE $1 ESCAPE '\\' (or equivalent ESCAPE clause) so the pattern treats those characters literally when executing the SELECT in pool.query.searchboost_api/Dockerfile-1-17 (1)
1-17:⚠️ Potential issue | 🟠 MajorRun the API container as a non-root user.
Line 1 starts from an image that defaults to root, and no
USERis set. This leaves the app process running as root in-container.🔒 Proposed hardening patch
FROM node:20-alpine WORKDIR /app COPY package*.json ./ RUN npm install --production COPY . . # Ensure we don't bring in dev .env by accident if it exists in context, # although compose will override it. -RUN rm -f .env+RUN rm -f .env && chown -R node:node /app++USER node EXPOSE 3001 CMD ["node", "src/app.js"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/Dockerfile` around lines 1 - 17, The Dockerfile currently runs the Node process as root; create a non-root user and switch to it before starting the app: add steps to create a user/group (e.g., appuser), chown the WORKDIR (/app) and any runtime-owned files to that user, and set USER to that account before CMD so node src/app.js runs without root privileges; ensure npm install and any build steps that require root remain earlier (or use a temporary root stage) and only drop to the non-root user for runtime.searchboost_api/src/db/pool.js-6-8 (1)
6-8:⚠️ Potential issue | 🟠 MajorAvoid hardcoded fallback database credentials in app code.
Lines 6–8 silently fall back to known credentials. This can accidentally enable insecure deployments and make env misconfiguration harder to detect.
🔐 Proposed fix
const { Pool } = require('pg'); +const requiredEnv = ['DB_HOST', 'DB_PORT', 'DB_USER', 'DB_PASSWORD', 'DB_NAME'];+for (const key of requiredEnv) {+ if (!process.env[key]) {+ throw new Error(`Missing required environment variable: ${key}`);+ }+}+ const pool = new Pool({ - host: process.env.DB_HOST || 'localhost',- port: process.env.DB_PORT || 5432,- user: process.env.DB_USER || 'searchboost',- password: process.env.DB_PASSWORD || 'searchboost_pass',- database: process.env.DB_NAME || 'searchboost_db',+ host: process.env.DB_HOST,+ port: Number(process.env.DB_PORT),+ user: process.env.DB_USER,+ password: process.env.DB_PASSWORD,+ database: process.env.DB_NAME, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/src/db/pool.js` around lines 6 - 8, The DB pool configuration currently provides hardcoded fallbacks for user/password/database (the user, password, database fields in the exported config in pool.js); remove these default literals and instead require those values from environment variables (or validate them at startup) so deployments fail fast on missing credentials—update the code that builds the DB config to not silently default to 'searchboost'/'searchboost_pass'/'searchboost_db' and add a startup validation that throws a clear error if process.env.DB_USER, process.env.DB_PASSWORD or process.env.DB_NAME are missing.searchboost_ui/Dockerfile-15-25 (1)
15-25:⚠️ Potential issue | 🟠 MajorRun the runtime image as non-root.
The nginx:stable-alpine image runs as root by default (no USER directive), which violates the DS-0002 security standard and weakens container isolation. Switch to
nginxinc/nginx-unprivileged:stable-alpineand update the port to 8080 (non-root cannot bind to ports below 1024). Updatesearchboost_ui/nginx.confline 2 fromlisten 80;tolisten 8080;.🔧 Proposed fix
-FROM nginx:stable-alpine+FROM nginxinc/nginx-unprivileged:stable-alpine # Copy build output to nginx path COPY --from=build /app/dist /usr/share/nginx/html # Replace default nginx config with our SPA + Proxy config COPY nginx.conf /etc/nginx/conf.d/default.conf -EXPOSE 80+EXPOSE 8080 CMD ["nginx", "-g", "daemon off;"]And in
searchboost_ui/nginx.conf:server { - listen 80;+ listen 8080; server_name localhost;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_ui/Dockerfile` around lines 15 - 25, Change the runtime image to a non‑root nginx image and move the exposed/listen port to 8080: replace the FROM image in the Dockerfile (the runtime stage that currently uses nginx:stable-alpine) with nginxinc/nginx-unprivileged:stable-alpine, update EXPOSE 80 to EXPOSE 8080, and update the nginx configuration (searchboost_ui/nginx.conf) to change the listen directive from 80 to 8080 (keep CMD ["nginx","-g","daemon off;"] as-is).searchboost_service/searchboost_src/service.py-124-127 (1)
124-127:⚠️ Potential issue | 🟠 MajorPost-optimization cache hit bypasses assistant history persistence.
When this branch returns early, the assistant response is never saved, so conversation history becomes incomplete for cached optimized-query hits.
Proposed fix
if post_opt_cache: self.logger.info("--- CACHE HIT (POST-OPTIMIZATION) ---") - print(f"\nFinal Response (Cached via Optimized Query):\n{post_opt_cache}")+ if history_svc and self.session_id:+ await history_svc.save_turn(self.session_id, "assistant", post_opt_cache)+ self.logger.debug("SearchBoostService : Returning cached response (optimized query hit).") return post_opt_cache🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_service/searchboost_src/service.py` around lines 124 - 127, The post-opt cache early return returns post_opt_cache without persisting the assistant message, so update the branch that sees post_opt_cache (the block containing self.logger.info, print and return post_opt_cache) to first invoke the existing assistant-history persistence routine used elsewhere (e.g., the function/method that saves assistant responses such as save_assistant_response, persist_assistant_message, conversation.add_message or similar) with post_opt_cache as the assistant content, then perform the logging/print and finally return the cached response; ensure you use the same parameters/metadata the normal response-persistence code uses so cached hits are recorded identically.configs/master_settings.yml-14-14 (1)
14-14:⚠️ Potential issue | 🟠 MajorAvoid hardcoding passwords in configuration files.
The Redis password (line 14) and database password (line 31) are hardcoded. Configuration files are often committed to version control, which would expose these credentials. Consider using environment variable interpolation or a secrets management solution.
Suggested approach
Use environment variable placeholders that the configuration loader resolves at runtime:
redis: password: ${REDIS_PASSWORD}db: password: ${DB_PASSWORD}Or reference a separate
.envfile / secrets manager that is excluded from version control.Also applies to: 31-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/master_settings.yml` at line 14, The config currently hardcodes credentials for the redis and db password keys (password under the redis and db sections); replace these literal values with environment-variable placeholders (e.g., ${REDIS_PASSWORD} and ${DB_PASSWORD}) or wire them to your secrets manager so the loader resolves them at runtime, and update any .env.example or config docs accordingly to document the required env vars.searchboost_service/searchboost_src/redis_manager.py-64-64 (1)
64-64:⚠️ Potential issue | 🟠 MajorAvoid logging raw query text (PII/privacy risk).
Line [64] logs full user query content; search text can contain sensitive data and should not be persisted verbatim in logs.
🔒 Proposed fix
- self._logger.debug(f"RedisManager: Caching query '{query}' with TTL {actual_ttl}s")+ self._logger.debug(f"RedisManager: Caching query with TTL {actual_ttl}s")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_service/searchboost_src/redis_manager.py` at line 64, The debug log in RedisManager that logs the raw user query should be replaced to avoid persisting PII; update the logging in the method that contains the line self._logger.debug(f"RedisManager: Caching query '{query}' with TTL {actual_ttl}s") to log a non-reversible identifier and metadata instead (e.g., compute and log a SHA-256 hex digest of query and the query length and TTL, or log a redacted/placeholder like "<redacted_query>" plus length/TTL), ensuring you no longer include the raw query text in the log output.searchboost_service/searchboost_src/redis_manager.py-56-61 (1)
56-61:⚠️ Potential issue | 🟠 MajorFix Ruff E701 one-line
ifstatements to keep lint green.Line [56] and Line [61] use multiple statements on one line (
if ...: return), which is currently flagged as Ruff E701.🧹 Proposed fix
- if not self._redis: return None+ if not self._redis:+ return None @@ - if not self._redis: return+ if not self._redis:+ return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_service/searchboost_src/redis_manager.py` around lines 56 - 61, Replace the one-line guarded returns flagged by Ruff E701 with multi-line if blocks: in the method that returns the cached value (the block that currently does "if not self._redis: return None" before "return await self._redis.get(f\"query:{query}\")") and in cache_response (the "if not self._redis: return" guard), expand them to standard two-line form (if not self._redis: newline indent return ...) so the early-return checks use block-style if statements rather than single-line statements.searchboost_tests/timeouts.py-1-90 (1)
1-90:⚠️ Potential issue | 🟠 MajorRename test file to match pytest discovery patterns.
This file is named
timeouts.py, which does not match pytest's default discovery patterns (test_*.pyor*_test.py). With no custom pytest configuration found in the repository (nopytest.ini,pyproject.toml, orsetup.cfgwith pytest options), this test file will not be discovered or executed by pytest in CI.Rename to
test_timeouts.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_tests/timeouts.py` around lines 1 - 90, The test file timeouts.py won't be discovered by pytest; rename the file to test_timeouts.py so pytest picks it up, then ensure any external references (if any) to timeouts.py are updated; the tests inside (test_web_search_timeout_handling and test_ollama_query_timeout_handling) reference WebSearch and OllamaClient and should run unchanged once the file is renamed.searchboost_ui/src/components/SearchBar.jsx-50-76 (1)
50-76:⚠️ Potential issue | 🟠 MajorAdd accessible names for input controls.
Line [50] textarea and Line [72] submit button rely on placeholder/title only; screen readers need explicit accessible names.
♿ Proposed fix
<textarea id="search-input" + aria-label="Search query" ref={textareaRef} onInput={handleInput} onKeyDown={handleKeyDown} disabled={loading} placeholder="Ask anything... (Ctrl+Enter to submit)" @@ <button + aria-label="Submit query" onClick={handleSubmit} disabled={loading} title="Submit"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_ui/src/components/SearchBar.jsx` around lines 50 - 76, The textarea with id "search-input" (using textareaRef, handleInput, handleKeyDown) and the submit button (onClick handleSubmit) lack explicit accessible names; add aria-label or aria-labelledby attributes to both controls (e.g., aria-label="Search query" on the textarea and aria-label="Submit search" on the button or reference visible labels via aria-labelledby) so screen readers get a proper name while keeping existing id/handlers intact and preserve disabled/title behavior..gsd/phases/2/2.6-PLAN.md-117-123 (1)
117-123:⚠️ Potential issue | 🟠 Major
setIntervalwill overlap slow polls.A 2-second interval does not wait for the previous
/result/{job_id}request to finish. Under latency or retries, this stacks duplicate polls for the same job and creates races around completion/error cleanup. A recursivesetTimeoutafter each response is safer here.Also applies to: 131-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gsd/phases/2/2.6-PLAN.md around lines 117 - 123, The current polling design uses setInterval to call GET /api/search/result/{job_id} every 2000ms which can overlap when requests are slow; change to a recursive setTimeout-based poll loop (startPolling -> pollOnce) that issues the next setTimeout only after the previous request completes, so no concurrent polls for the same job occur; ensure the loop still clears on status="complete" or error, and replace any clearInterval calls with clearTimeouts or cancel logic; preserve the 240s max-poll timeout by tracking elapsed time (or a single setTimeout as hard deadline) and aborting the recursive polling with a "Request timed out" error when exceeded.searchboost_api/src/routes/auth.js-61-70 (1)
61-70:⚠️ Potential issue | 🟠 MajorKeep cookie expiry aligned with the configured auth TTL.
The JWT lifetime is configurable, but the cookie always expires after 24 hours. Any non-default
JWT_EXPIRES_INwill desynchronize browser state from server auth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/src/routes/auth.js` around lines 61 - 70, The cookie expiry for 'sb_token' is hardcoded to 24h while the token TTL is configurable via process.env.JWT_EXPIRES_IN; update the res.cookie call (the 'sb_token' cookie code near jwt.sign) to derive maxAge from the same JWT_EXPIRES_IN value used for jwt.sign instead of the fixed 24*60*60*1000. Convert the JWT_EXPIRES_IN string (support numeric seconds or formats like '24h'/'1d' if used) into milliseconds and pass that as maxAge, and fall back to the current 24h constant only if JWT_EXPIRES_IN is missing or unparsable so the cookie and jwt lifetimes remain synchronized.searchboost_api/src/routes/auth.js-24-30 (1)
24-30:⚠️ Potential issue | 🟠 MajorUsername uniqueness needs an atomic write path.
The
findByUsername()precheck is race-prone. Two concurrent registrations for the same username can both pass the lookup, and the loser will then depend on DB error behavior instead of reliably returning the intended 409.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/src/routes/auth.js` around lines 24 - 30, The current precheck using findByUsername is race-prone; instead make the signup flow atomic by relying on a DB-unique constraint for username and handling the insertion error from createUser: ensure a unique index exists for username, remove/keep but do not rely on the precheck alone (findByUsername), and wrap the createUser call in a try/catch that detects the DB unique-violation error (database-specific code/constraint name) and returns res.status(409).json({ error: 'Username already taken' }) for that error while rethrowing/returning 500 for other failures..gsd/phases/2/2.6-PLAN.md-125-129 (1)
125-129:⚠️ Potential issue | 🟠 Major“New conversation” needs a backend reset primitive too.
The plan says follow-ups should include prior context, but this reset only clears local
conversationHistory. If the backend still keys context by user/session, the next search will continue the old conversation even though the UI looks empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gsd/phases/2/2.6-PLAN.md around lines 125 - 129, The UI "New conversation" currently only clears the local conversationHistory variable but not the backend context; add a backend reset primitive (e.g., create an endpoint like POST /api/conversations/reset backed by a method such as ConversationService.resetContext or resetConversationContext(sessionId/userId)) that clears stored conversation state or generates a new conversation ID for that user/session, and update the "New conversation" button handler to call this endpoint before or when clearing conversationHistory so follow-up queries no longer pick up prior backend context..gsd/phases/2/2.10-PLAN.md-42-44 (1)
42-44:⚠️ Potential issue | 🟠 MajorDon't reconstruct history by simple User/Assistant adjacency.
Matching turns purely by sequential role is brittle once a message is missing, retried, or interleaved with another request. This plan should require a stable conversation/thread key plus ordering metadata so history reconstruction cannot attach the wrong answer to the wrong query.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gsd/phases/2/2.10-PLAN.md around lines 42 - 44, The current plan for getHistory(username) is unsafe because it pairs turns by simple User/Assistant adjacency; instead modify the plan to query the conversation_turns table using a stable conversation/thread key (e.g., conversation_id) plus an ordering column (e.g., turn_index or created_at) and reconstruct pairs only within the same conversation_id using ordering metadata. Specifically, query turns filtered by username and conversation_id, sort by the ordering column, and pair a User turn with the next Assistant turn in that same conversation_id (or join User rows to the next Assistant row by sequence number) so missing, retried, or interleaved messages cannot attach the wrong Assistant response to a query.searchboost_ui/src/pages/Admin.jsx-24-33 (1)
24-33:⚠️ Potential issue | 🟠 MajorDon't make the initial dashboard load all-or-nothing.
Promise.allthrows away whichever response did succeed if the other call fails. That means a temporary/admin/usersfailure also blanks health, and vice versa, even though those panels are independent.♻️ Safer partial-load pattern
- const [usersRes, healthRes] = await Promise.all([- client.get('/admin/users'),- client.get('/admin/health')- ]);- setUsers(usersRes.data);- setHealthData(healthRes.data);+ const [usersRes, healthRes] = await Promise.allSettled([+ client.get('/admin/users'),+ client.get('/admin/health'),+ ]);++ if (usersRes.status === 'fulfilled') {+ setUsers(usersRes.value.data);+ }+ if (healthRes.status === 'fulfilled') {+ setHealthData(healthRes.value.data);+ }+ if (usersRes.status === 'rejected' && healthRes.status === 'rejected') {+ setError('Failed to load administrative data. Some services may be unreachable.');+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_ui/src/pages/Admin.jsx` around lines 24 - 33, The current use of Promise.all with client.get('/admin/users') and client.get('/admin/health') causes one failing request to discard the other; change the logic to fetch each independently (e.g., use Promise.allSettled or separate awaits) so you can handle success/failure per request and call setUsers when the users call succeeds and setHealthData when the health call succeeds; keep existing error handling but update it to record which call failed (referencing client.get('/admin/users'), client.get('/admin/health'), usersRes/healthRes, setUsers and setHealthData) so partial data renders instead of an all-or-nothing failure.searchboost_ui/src/pages/Register.jsx-37-48 (1)
37-48:⚠️ Potential issue | 🟠 MajorHandle the “registered but not logged in” path explicitly.
If
/auth/registersucceeds and/auth/loginfails, the account is already created but the user stays on this form. Retrying then flips into “Username already taken,” which is misleading and turns a transient failure into a broken happy path.🩹 One UI-side mitigation
- try {- await client.post('/auth/register', { username, password });+ let registered = false;+ try {+ await client.post('/auth/register', { username, password });+ registered = true; const res = await client.post('/auth/login', { username, password }); login(res.data); navigate('/search'); } catch (err) { + if (registered) {+ navigate('/login', { replace: true });+ return;+ } if (err.response && err.response.status === 409) { setError('Username already taken'); } else if (err.response && err.response.data && err.response.data.error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_ui/src/pages/Register.jsx` around lines 37 - 48, When register succeeds but the subsequent login fails the code currently treats errors (on retry) as "Username already taken"; change the flow in Register.jsx so you separate the two actions: call client.post('/auth/register') first, then attempt client.post('/auth/login'); if the register call succeeds but the login call throws, set a clear UI state via setError('Account created but login failed. Please try logging in or retry.') and surface a retry-login action (or navigate to the login page) instead of mapping that failure to a 409; update handling around client.post('/auth/login'), login(), setError and navigate to reflect this explicit "registered but not logged in" path.searchboost_ui/src/pages/Search.jsx-21-26 (1)
21-26:⚠️ Potential issue | 🟠 MajorReset transient state when the thread changes.
This effect only starts
fetchHistory(currentThreadId)and clears the timer. If the user switches chats while a search is pending,loadingcan staytrueforever, and a failed history fetch leaves the previous thread's transcript/result on screen. Clearloading,result,error, andconversationHistorybefore starting the new fetch.Also applies to: 37-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_ui/src/pages/Search.jsx` around lines 21 - 26, When the current thread changes in the useEffect that calls fetchHistory(currentThreadId) and clears pollIntervalRef, reset transient UI state first: set loading to true/false appropriately and clear result, error, and conversationHistory before starting the new fetch so prior thread data or a stuck loading state cannot persist; update the effect that invokes fetchHistory (and the similar effect at the other location) to explicitly set loading = true, result = null, error = null, conversationHistory = [] (or their initial values) immediately before calling fetchHistory(currentThreadId) and ensure any aborted/failed fetch handlers also clear loading and set error/result consistently..gsd/phases/5/5.1-PLAN.md-30-32 (1)
30-32:⚠️ Potential issue | 🟠 MajorDo not use raw
LIKEon usernames that may contain_.Phase 2.2 explicitly allows underscores in usernames, and
_is a single-character wildcard in SQLLIKE. A pattern such asSB-SESSION-john_doe%will also matchSB-SESSION-johnXdoe..., so this query shape can leak another user's sessions/history. Escape%and_withESCAPE, or stop encoding identity into a prefix string and query by separate columns instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gsd/phases/5/5.1-PLAN.md around lines 30 - 32, history.js currently builds session queries like getSessions(username) and getHistory(sessionId) that rely on a prefix pattern (e.g., "SB-SESSION-{username}%") using SQL LIKE, which misinterprets underscores and % in usernames; change the implementation to avoid raw LIKE on usernames by either (1) stopping identity encoding into the session_id prefix and instead add/query a dedicated username column (use SELECT DISTINCT session_id FROM conversation_turns WHERE username = $1 and SELECT content, role, created_at FROM conversation_turns WHERE session_id = $1), or if you must keep the prefix approach, escape user-supplied '%' and '_' and use an explicit ESCAPE clause in the LIKE expression so getSessions and getHistory do not leak other users' sessions. Ensure updates touch the getSessions and getHistory functions in history.js only.searchboost_ui/src/pages/Search.jsx-64-104 (1)
64-104:⚠️ Potential issue | 🟠 MajorUse recursive
setTimeoutinstead ofsetInterval(async ...)for polling.
setIntervalschedules callbacks at fixed intervals regardless of whether previous callbacks have completed. When the callback is async and contains awaits (network requests), concurrent overlapping executions are possible if the awaited work takes longer than the interval. In this code, ifclient.get()takes >2000ms, the next interval tick fires before the previous completes—both callbacks could then callsetConversationHistory, producing duplicate history entries. Use recursivesetTimeoutwith a generation or abort guard to ensure each search's polling is isolated and only the latest search's result is committed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_ui/src/pages/Search.jsx` around lines 64 - 104, Replace the setInterval-based polling in the block that uses pollIntervalRef and pollCount with a recursive setTimeout loop tied to a per-search generation/abort guard (e.g., a searchGenerationRef that increments on each new search) so only the latest jobId's polling commits state; store the timeout id in pollIntervalRef (rename to pollTimeoutRef if desired), on each tick check generation matches before processing results, await client.get(`/search/result/${jobId}`) serially, increment pollCount and stop by clearing the timeout and setting setError/setLoading as before, and ensure you call setResult, setConversationHistory, and fetchSessions only when the generation matches to prevent duplicate history updates from overlapping polls..gsd/phases/2/2.8-PLAN.md-12-14 (1)
12-14:⚠️ Potential issue | 🟠 MajorMove
.envto repository root to match Docker Compose configuration.Line 13 instructs users to create
searchboost_api/.env, but line 131 configuresenv_file: .env, which Docker Compose resolves relative to the directory containingdocker-compose.yml(the repository root). The mismatch means Docker Compose will look for.envat the repository root and ignore the file the user was told to create, preventingJWT_SECRETfrom loading.📄 Suggested doc fix
- - task: "Copy .env.example to .env in searchboost_api/ and set JWT_SECRET to a long random string"+ - task: "Copy searchboost_api/.env.example to .env at the repository root and set JWT_SECRET to a long random string"Also applies to: 127-132, 160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gsd/phases/2/2.8-PLAN.md around lines 12 - 14, The doc currently instructs creating searchboost_api/.env which conflicts with the Docker Compose setting env_file: .env (resolved relative to the compose file), so update the user_setup task that currently says "Copy .env.example to .env in searchboost_api/ and set JWT_SECRET..." to instruct users to copy .env.example to the repository root .env (and set JWT_SECRET there); also update any other occurrences of the same instruction (the other user_setup entries referenced) so they consistently direct creation of .env at the repo root to match env_file: .env..gsd/phases/2/2.4-PLAN.md-121-129 (1)
121-129:⚠️ Potential issue | 🟠 MajorDon't redirect on every 401 from the shared axios client; it breaks public auth routes.
This plan will call
GET /api/auth/meon AuthContext mount, but the global 401 interceptor will bounce unauthenticated users away from/registerbefore they can access the form. Additionally, login form submissions that receive a 401 (invalid credentials) cannot display error messages because the interceptor consumes the response.📄 Suggested plan adjustment
- - Response interceptor: on 401 response, redirect to /login+ - Response interceptor: on 401 response, redirect to /login only for protected endpoints+ and never for /auth/me, /auth/login, or /auth/register- - On mount: GET /api/auth/me, set user or null+ - On mount: GET /api/auth/me; on 401 set user to null without navigation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gsd/phases/2/2.4-PLAN.md around lines 121 - 129, The shared axios instance in src/api/client.js currently redirects to /login on every 401 which breaks public routes and form error handling; modify the response interceptor to only perform the redirect when the failed request does not opt out (use a request config flag like skipAuthRedirect or allowAuthErrors on error.config), then update callers: in AuthContext.jsx's mount GET /api/auth/me and in login form submissions set skipAuthRedirect (or allowAuthErrors) to true so unauthenticated users or invalid-credential 401s can be handled locally, while protected requests keep the default behavior and still trigger the global redirect; keep logout() behavior as-is (it should explicitly navigate("/login")).searchboost_api/src/routes/search.js-16-20 (1)
16-20:⚠️ Potential issue | 🟠 MajorAdd a timeout-backed Warden client to prevent indefinite request hangs.
The axios calls to Warden at lines 16 and 35 lack a timeout, so a slow or half-open connection will block the request indefinitely and exhaust the Express request pool. The codebase already demonstrates timeout handling in
admin.js(3000ms on health check), so apply the same pattern here.⏱️ Suggested fix
const router = express.Router(); +const wardenClient = axios.create({+ baseURL: process.env.WARDEN_URL,+ timeout: 10000,+}); router.post('/enqueue', verifyToken, async (req, res, next) => { try { @@ - const response = await axios.post(`${process.env.WARDEN_URL}/enqueue`, {+ const response = await wardenClient.post('/enqueue', { query, session_id, options: options || {} @@ router.get('/result/:job_id', verifyToken, async (req, res, next) => { try { const { job_id } = req.params; - const response = await axios.get(`${process.env.WARDEN_URL}/results/${job_id}`);+ const response = await wardenClient.get(`/results/${job_id}`); res.status(response.status).json(response.data);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/src/routes/search.js` around lines 16 - 20, The axios requests to Warden (the axios.post calls that assign to response and the second axios.post later in the same file) lack a timeout and can hang; create a timeout-backed Warden client or pass a timeout option (use the same 3000ms used in admin.js health check) to each axios.post call to `${process.env.WARDEN_URL}/enqueue` so requests fail fast on slow/half-open connections; update both occurrences (the first response = await axios.post(...) and the later axios.post(...)) to use the timeout-backed client or include { timeout: 3000 } in the request options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2192a7c9-7c02-402e-ac9f-6c997df77586
⛔ Files ignored due to path filters (2)
searchboost_api/package-lock.jsonis excluded by!**/package-lock.jsonsearchboost_ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (99)
.gitignore.gsd/ARCHITECTURE.md.gsd/DECISIONS.md.gsd/ROADMAP.md.gsd/SPEC.md.gsd/STACK.md.gsd/STATE.md.gsd/phases/1/1-PLAN.md.gsd/phases/1/1-SUMMARY.md.gsd/phases/1/2-PLAN.md.gsd/phases/1/2-SUMMARY.md.gsd/phases/1/RESEARCH.md.gsd/phases/2/2.1-PLAN.md.gsd/phases/2/2.10-PLAN.md.gsd/phases/2/2.2-PLAN.md.gsd/phases/2/2.3-PLAN.md.gsd/phases/2/2.4-PLAN.md.gsd/phases/2/2.5-PLAN.md.gsd/phases/2/2.6-PLAN.md.gsd/phases/2/2.7-PLAN.md.gsd/phases/2/2.8-PLAN.md.gsd/phases/2/2.9-PLAN.md.gsd/phases/3/3.1-PLAN.md.gsd/phases/3/3.1-SUMMARY.md.gsd/phases/3/3.2-PLAN.md.gsd/phases/3/3.2-SUMMARY.md.gsd/phases/4/4.1-PLAN.md.gsd/phases/4/4.1-SUMMARY.md.gsd/phases/4/4.2-PLAN.md.gsd/phases/4/4.2-SUMMARY.md.gsd/phases/4/VERIFICATION.md.gsd/phases/5/5.1-PLAN.md.gsd/phases/5/5.1-SUMMARY.md.gsd/phases/5/5.2-PLAN.md.gsd/phases/5/5.2-SUMMARY.md.gsd/phases/5/VERIFICATION.mdLICENSEconfigs/cloud_ai.jsonconfigs/db.jsonconfigs/limiter.tomlconfigs/local_ai.jsonconfigs/master.yamlconfigs/master_settings.ymlconfigs/redis.jsonconfigs/searxng_settings.ymlconfigs/service_settings.jsonconfigs/warden.iniconfigs/web_search.jsondocker-compose.ymlnotes/DetailedAudit.mdnotes/SystemDesign.mdnotes/TODO.mdscripts/collect_logs.shsearchboost_api/.env.examplesearchboost_api/Dockerfilesearchboost_api/package.jsonsearchboost_api/src/app.jssearchboost_api/src/db/history.jssearchboost_api/src/db/migrate.jssearchboost_api/src/db/pool.jssearchboost_api/src/db/users.jssearchboost_api/src/middleware/auth.jssearchboost_api/src/routes/admin.jssearchboost_api/src/routes/auth.jssearchboost_api/src/routes/search.jssearchboost_service/requirements.txtsearchboost_service/searchboost_src/configurator.pysearchboost_service/searchboost_src/database.pysearchboost_service/searchboost_src/redis_manager.pysearchboost_service/searchboost_src/service.pysearchboost_service/searchboost_src/worker.pysearchboost_tests/timeouts.pysearchboost_ui/.gitignoresearchboost_ui/Dockerfilesearchboost_ui/index.htmlsearchboost_ui/nginx.confsearchboost_ui/package.jsonsearchboost_ui/src/App.jsxsearchboost_ui/src/api/client.jssearchboost_ui/src/components/AdminUserTable.jsxsearchboost_ui/src/components/NavBar.jsxsearchboost_ui/src/components/ProtectedRoute.jsxsearchboost_ui/src/components/ResultDisplay.jsxsearchboost_ui/src/components/SearchBar.jsxsearchboost_ui/src/components/SystemHealth.jsxsearchboost_ui/src/context/AuthContext.jsxsearchboost_ui/src/index.csssearchboost_ui/src/main.jsxsearchboost_ui/src/pages/Admin.jsxsearchboost_ui/src/pages/Login.jsxsearchboost_ui/src/pages/Register.jsxsearchboost_ui/src/pages/Search.jsxsearchboost_ui/vite.config.jssearchboost_warden/Cargo.tomlsearchboost_warden/Dockerfilesearchboost_warden/src/configurator.rssearchboost_warden/src/main.rssearchboost_warden/src/observer.rssearchboost_warden/src/relay.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
added 5 commits
March 27, 2026 18:27
… strict process booting
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (8)
searchboost_ui/Dockerfile (2)
8-8: Use lockfile-based install for deterministic builds.Replace
npm installwithnpm cito ensure reproducible dependency resolution and prevent lockfile drift in container builds. The requiredpackage-lock.jsonfile is present.Proposed change
-RUN npm install+RUN npm ci🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_ui/Dockerfile` at line 8, The Dockerfile uses a non-deterministic install command: replace the RUN npm install line in the Dockerfile with RUN npm ci so the build uses the lockfile for deterministic installs; ensure package-lock.json is copied into the image before that RUN step and that the CI install is run in the same working directory as package.json/package-lock.json.
15-25: Add explicitUSER 101to make non-root runtime explicit and durable.The
nginxinc/nginx-unprivileged:stable-alpinebase image runs as the nginx user (UID 101) by default. Adding an explicitUSERdirective makes this security intent clear in the Dockerfile, improves durability across base image updates, and reduces scanner noise.Proposed change
FROM nginxinc/nginx-unprivileged:stable-alpine # Copy build output to nginx path COPY --from=build /app/dist /usr/share/nginx/html # Replace default nginx config with our SPA + Proxy config COPY nginx.conf /etc/nginx/conf.d/default.conf +USER 101 EXPOSE 8080 CMD ["nginx", "-g", "daemon off;"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_ui/Dockerfile` around lines 15 - 25, The Dockerfile currently relies on nginxinc/nginx-unprivileged:stable-alpine running as UID 101 implicitly; make the non-root runtime explicit and durable by adding a USER 101 directive (e.g., immediately after the FROM line or after copying files) so the image always runs as the nginx unprivileged user; ensure this USER 101 appears before EXPOSE/CMD so runtime processes inherit the non-root UID.searchboost_service/searchboost_src/configurator.py (1)
231-232: Avoid broad exception catches in YAML loading paths.Line 231 and Line 244 catch
Exception, which can mask real defects and make config failures hard to diagnose. Catch expected parse/IO errors explicitly and keep unexpected exceptions visible.Also applies to: 244-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_service/searchboost_src/configurator.py` around lines 231 - 232, The except blocks that currently catch Exception around YAML/config loading (the handler logging "Config Loader: Error reading {master_filepath}") should be narrowed to only handle expected errors: catch yaml.YAMLError for parse errors and FileNotFoundError/OSError (or IOError) for filesystem issues, log those with self._logger.warning including the filepath and error, and re-raise or let any other unexpected exceptions propagate so they are not masked; update both occurrences (the handler referencing master_filepath and the similar handler at the other location) to follow this pattern and avoid a bare Exception catch.MANUAL_TESTPLAN.md (2)
21-21: Consider simplifying "completely empty" to "empty."The phrase "completely empty" may be redundant. Simply "empty" conveys the same meaning more concisely. As per the static analysis hint, consider the shorter alternative for improved readability.
✍️ Suggested simplification
-**Expected:** The cache MUST be completely empty for this query due to the PII detection.+**Expected:** The cache MUST be empty for this query due to the PII detection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MANUAL_TESTPLAN.md` at line 21, Update the sentence in MANUAL_TESTPLAN.md that reads "The cache MUST be completely empty for this query due to the PII detection." to use the simpler phrasing "The cache MUST be empty for this query due to the PII detection." — locate that exact sentence and replace "completely empty" with "empty" to improve readability.
23-31: Add specific Redis commands for verification.Test 3 mentions checking Redis keys and verifying cache hits but doesn't provide the specific
redis-clicommands needed. This reduces repeatability and clarity for QA engineers executing the test.Consider adding specific commands such as:
redis-cli KEYS '*'to list all cache keys after step 3redis-cli GET <key>to inspect cached values- Specific key patterns to look for (e.g.,
semantic_cache:*)- How to verify a cache hit (e.g., check response time, inspect logs, or verify SearXNG wasn't called)
📝 Suggested enhancement
## Test 3: Post-Optimization Cache Check **Method:** Terminal (`curl` / `redis-cli`) **Objective:** Confirm that two different inputs requesting identically structured semantic outcomes match via the underlying optimized string, avoiding multiple external searches. **Steps:** 1. Clear the Redis cache (`FLUSHALL`). 2. Enqueue Request A: "can you tell me who the current president of france is right now" -3. Wait for LLM optimization and response. Check Redis for keys.+3. Wait for LLM optimization and response. Check Redis for keys: `redis-cli KEYS 'semantic_cache:*'` 4. Enqueue Request B: "who is president france" -**Expected:** The system should instantly flag a post-optimization CACHE HIT on the second query without calling SearXNG again.+**Expected:** The system should instantly flag a post-optimization CACHE HIT on the second query without calling SearXNG again. Verify by checking logs for "CACHE HIT" or by confirming no additional SearXNG requests in network logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MANUAL_TESTPLAN.md` around lines 23 - 31, Update Test 3 in MANUAL_TESTPLAN.md to include concrete redis-cli commands and key patterns: after step 3 instruct QA to run commands like listing keys (e.g., KEYS '*'), filtering by the cache namespace (e.g., semantic_cache:*), and retrieving values (GET <key>) to inspect stored optimized strings; specify checking TTLs (TTL <key>) if relevant, and describe how to verify a cache hit for Request B by measuring response time, confirming the same optimized string/value from the cached key, and/or checking logs to ensure SearXNG was not invoked (search logs for the SearXNG request entry). Ensure the doc references "Test 3: Post-Optimization Cache Check", the key pattern "semantic_cache:*", and the actions GET, KEYS, and TTL so QA can reproduce the verification steps precisely.searchboost_api/src/routes/search.js (1)
13-15: Validatethread_idformat before composingsession_id.
thread_idis concatenated directly intosession_id(Lines 13–15 and 64–65). Add a strict allowlist/length cap to avoid malformed IDs and inconsistent history/session addressing.🔧 Suggested fix
+function normalizeThreadId(raw) {+ if (!raw || raw === 'default') return '';+ if (typeof raw !== 'string' || !/^[a-zA-Z0-9_-]{1,64}$/.test(raw)) return null;+ return `-${raw}`;+} @@ - const thread_id = req.body.thread_id && req.body.thread_id !== 'default' ? `-${req.body.thread_id}` : '';+ const thread_id = normalizeThreadId(req.body.thread_id);+ if (thread_id === null) return res.status(400).json({ error: 'Invalid thread_id' }); @@ - const thread_id = threadParam && threadParam !== 'default' ? `-${threadParam}` : '';+ const thread_id = normalizeThreadId(threadParam);+ if (thread_id === null) return res.status(400).json({ error: 'Invalid thread_id' });Also applies to: 64-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/src/routes/search.js` around lines 13 - 15, thread_id is currently concatenated into session_id without validation; add a strict allowlist and a max length check before forming session_id (validate the value assigned to the thread_id variable and reject or normalize anything outside the allowlist/length). Specifically, update the code that computes thread_id and session_id in search.js so thread_id is first run through a sanitizer function (e.g., allow only alphanumeric, hyphen/underscore if needed, and enforce a max length like 32), fallback to empty string for invalid values, and then build session_id using the sanitized thread_id; apply the same validation/sanitization where thread_id is used later (the occurrences around lines 64–65) to ensure consistent, safe session identifiers.searchboost_api/src/db/history.js (1)
38-38: Make turn ordering deterministic for pair reconstruction.Line 38 orders only by
created_at; equal timestamps can reorder turns and mispair query/result entries.🔧 Suggested fix
- 'SELECT role, content, created_at FROM conversation_turns WHERE session_id = $1 ORDER BY created_at ASC',+ 'SELECT id, role, content, created_at FROM conversation_turns WHERE session_id = $1 ORDER BY created_at ASC, id ASC',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/src/db/history.js` at line 38, The query ordering in the conversation_turns SELECT (the SQL string 'SELECT role, content, created_at FROM conversation_turns WHERE session_id = $1 ORDER BY created_at ASC') is not deterministic when created_at ties; update the ORDER BY to include a unique deterministic tiebreaker (for example append a stable column such as id or turn_index: "ORDER BY created_at ASC, id ASC") so turns are always returned in a consistent order for pair reconstruction.searchboost_warden/src/configurator.rs (1)
82-83: Include root cause details in startup errors.At Line 82 and Line 83,
expect(...)drops useful context. Include the underlying error so ops can distinguish malformed YAML vs missing keys vs merge issues quickly.Suggested patch
- let settings = Config::builder()+ let settings = Config::builder() .add_source(File::new(&master_path, FileFormat::Yaml).required(false)) .add_source(File::new(&discrete_path, FileFormat::Yaml).required(false)) .add_source(config::Environment::with_prefix("WARDEN").separator("__")) .build() - .expect("Warden Error: Could not find config file");+ .unwrap_or_else(|e| panic!("Warden Error: failed to build config: {e}"));- let mut settings: Self = settings.try_deserialize()- .expect("Warden Error: Config file format is invalid");+ let mut settings: Self = settings+ .try_deserialize()+ .unwrap_or_else(|e| panic!("Warden Error: invalid or incomplete config: {e}"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_warden/src/configurator.rs` around lines 82 - 83, The current call that deserializes settings uses settings.try_deserialize().expect(...) which drops the underlying error; change the failure path for try_deserialize() in the configurator to surface the root cause by capturing the error and including it in the panic/log message (e.g., replace the bare expect on try_deserialize() with an explicit match, unwrap_or_else, or expect_with formatted message that includes the error), referencing the deserialization site (settings.try_deserialize()) and the resulting Struct type (Self) so the message reads like "Warden Error: Config file format is invalid: {error}" and preserves the original error details for ops to inspect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MANUAL_TESTPLAN.md`:
- Line 5: MANUAL_TESTPLAN.md currently hardcodes the public IP
"http://35.204.126.60"; replace that literal with an environment-based
placeholder such as "$DEV_UI_URL" or "<dev-environment-url>" and update the line
that reads "**Method:** Browser UI (`http://35.204.126.60`)" to use the
placeholder instead; then add a short note pointing readers to the secure,
environment-specific configuration (e.g., .env.local or deployment
documentation) where the actual URL is stored and documented so the value is not
committed to the repo.
- Around line 4-11: Update Test 1 in MANUAL_TESTPLAN.md to include an
"Expected:" section and precise verification steps: state that each '+ New Chat'
must create a distinct thread_id, e.g., "Expected: Each New Chat creates a
unique thread_id; queries from one thread must not appear in another's history;
two distinct session records with different thread_id values should exist in
PostgreSQL", and add concrete checks such as "Verify sidebar shows thread_id or
session label for each chat; run a SQL query to confirm two distinct thread_id
values exist in the sessions table; perform queries in each chat and confirm
results are isolated."
- Line 17: Add a prominent warning around the FLUSHALL steps (the line
containing "FLUSHALL" and the similar step in Test 3) that this command will
delete all keys in the Redis instance and must only be run against an isolated
test Redis; update the MANUAL_TESTPLAN.md entries at the FLUSHALL lines to
either require a dedicated test Redis or offer alternatives (use a dedicated
test Redis instance, flush only specific key patterns with a scan+DEL pipeline
such as "redis-cli --scan --pattern 'semantic_cache:*' | xargs redis-cli DEL",
or use a non-default Redis DB number like "SELECT 15" for test isolation), and
mirror the same warning and alternatives for the Test 3 FLUSHALL occurrence.
In `@scripts/install.sh`:
- Around line 20-31: The .env creation uses a here-doc via "cat <<EOT >> .env"
to write high-sensitivity values (DB_PASSWORD, REDIS_PASSWORD, JWT_SECRET)
without setting file permissions; ensure the file is created with restrictive
permissions (e.g., use umask 077 before creating the file or create/truncate
.env then chmod 600) so secrets are not group/world-readable. Modify the section
around the here-doc in scripts/install.sh to set restrictive umask or explicitly
chmod the .env after writing, while still writing the same variables
(OLLAMA_PORT, OLLAMA_MODEL, SEARXNG_PORT, DB_USER, DB_NAME, DB_PASSWORD,
REDIS_PASSWORD, JWT_SECRET, JWT_EXPIRES_IN).
In `@searchboost_api/src/db/history.js`:
- Around line 9-17: The session lookup uses a prefix that allows
prefix-collisions (e.g., "alice" matching "alice2"); update the prefix building
and LIKE pattern to enforce a boundary after the username (e.g., change prefix
to include a delimiter like `SB-SESSION-${escapedUsername}-` and then query with
`${prefix}%`) so session_id matches only that user's sessions; update the
pool.query call that selects from conversation_turns (and the similar occurrence
at lines 20-23) to use the new prefix parameter while keeping the ESCAPE
handling and parameterized query.
In `@searchboost_api/src/routes/auth.js`:
- Around line 11-22: The route currently assumes username and password are
strings and performs length/regex checks and later bcrypt operations, which can
throw on non-string types; add an explicit type check (typeof username ===
'string' && typeof password === 'string') immediately after extracting req.body
and return res.status(400).json({ error: 'Username and password must be strings'
}) if they fail, before any length/regex checks or any
bcrypt.hash/bcrypt.compare calls; ensure the same guard is applied in the other
auth flow that references username/password (the login/register handler and any
places calling bcrypt.hash or bcrypt.compare) so non-string payloads return 400
instead of causing 500s.
- Around line 65-73: The route currently sets the JWT as an httpOnly cookie via
res.cookie(...) but then also returns the token in the response body with
res.status(200).json({ ...payload, token }), which exposes the bearer token to
JS; remove the token from the JSON response and only set it as the cookie.
Update the response to send the payload (e.g., user info) or an appropriate
status/message without including the token, keeping res.cookie(...) as-is;
ensure any code that expects the token in the body (client-side) is adjusted to
rely on the cookie instead.
In `@searchboost_api/src/routes/search.js`:
- Around line 36-38: The ownership check using sessionMatch and job_id is
vulnerable because startsWith allows username-prefix collisions; replace the
startsWith logic so it only accepts either an exact match of sessionMatch[1] ===
`SB-SESSION-${req.user.username}` or a hyphen-delimited thread form, e.g.
sessionMatch[1].startsWith(`SB-SESSION-${req.user.username}-`); update the
conditional that currently uses startsWith to require one of those two
conditions before returning 403.
In `@searchboost_service/searchboost_src/configurator.py`:
- Line 202: The merge currently assigns yaml_data highest precedence in
final_data, causing CLI and environment values to be ignored; change the merge
order so defaults come first, then YAML, then environment overrides, and finally
CLI args so runtime/config overrides win — update the final_data construction
(referencing the variables final_data, base_data, yaml_data, manual_env_data,
filtered_cli) to merge in an order like base_data, yaml_data, manual_env_data,
filtered_cli so SEARCHBOOST_* and CLI args take highest priority.
- Around line 239-243: The merging logic in the configurator (where
discrete_data and data are combined) is shallow: when both are dicts the code
uses data[key].update(val) which overwrites nested sibling fields; replace that
with a recursive deep-merge so nested dicts are merged rather than replaced.
Implement or call a helper (e.g., deep_merge_dicts or merge_dict_recursive) and
use it in place of data[key].update(val) inside the loop that iterates
discrete_data so that for keys where both data[key] and val are dicts you merge
recursively, otherwise assign as before.
In `@searchboost_warden/src/configurator.rs`:
- Around line 76-77: The current Config::builder call marks both
File::new(&master_path, ...) and File::new(&discrete_path, ...) as optional
which hides an explicitly provided config; change the logic that builds
master_path/discrete_path to detect if the path was supplied via env (e.g. from
WARDEN_CONFIG_PATH or similar) vs using the default, and call .required(true)
for the File source when the path was explicitly set and .required(false) when
it is the default; update the code around the variables master_path and
discrete_path (and the config builder invocation) to choose required(...)
dynamically based on that explicit-vs-default check.
---
Nitpick comments:
In `@MANUAL_TESTPLAN.md`:
- Line 21: Update the sentence in MANUAL_TESTPLAN.md that reads "The cache MUST
be completely empty for this query due to the PII detection." to use the simpler
phrasing "The cache MUST be empty for this query due to the PII detection." —
locate that exact sentence and replace "completely empty" with "empty" to
improve readability.
- Around line 23-31: Update Test 3 in MANUAL_TESTPLAN.md to include concrete
redis-cli commands and key patterns: after step 3 instruct QA to run commands
like listing keys (e.g., KEYS '*'), filtering by the cache namespace (e.g.,
semantic_cache:*), and retrieving values (GET <key>) to inspect stored optimized
strings; specify checking TTLs (TTL <key>) if relevant, and describe how to
verify a cache hit for Request B by measuring response time, confirming the same
optimized string/value from the cached key, and/or checking logs to ensure
SearXNG was not invoked (search logs for the SearXNG request entry). Ensure the
doc references "Test 3: Post-Optimization Cache Check", the key pattern
"semantic_cache:*", and the actions GET, KEYS, and TTL so QA can reproduce the
verification steps precisely.
In `@searchboost_api/src/db/history.js`:
- Line 38: The query ordering in the conversation_turns SELECT (the SQL string
'SELECT role, content, created_at FROM conversation_turns WHERE session_id = $1
ORDER BY created_at ASC') is not deterministic when created_at ties; update the
ORDER BY to include a unique deterministic tiebreaker (for example append a
stable column such as id or turn_index: "ORDER BY created_at ASC, id ASC") so
turns are always returned in a consistent order for pair reconstruction.
In `@searchboost_api/src/routes/search.js`:
- Around line 13-15: thread_id is currently concatenated into session_id without
validation; add a strict allowlist and a max length check before forming
session_id (validate the value assigned to the thread_id variable and reject or
normalize anything outside the allowlist/length). Specifically, update the code
that computes thread_id and session_id in search.js so thread_id is first run
through a sanitizer function (e.g., allow only alphanumeric, hyphen/underscore
if needed, and enforce a max length like 32), fallback to empty string for
invalid values, and then build session_id using the sanitized thread_id; apply
the same validation/sanitization where thread_id is used later (the occurrences
around lines 64–65) to ensure consistent, safe session identifiers.
In `@searchboost_service/searchboost_src/configurator.py`:
- Around line 231-232: The except blocks that currently catch Exception around
YAML/config loading (the handler logging "Config Loader: Error reading
{master_filepath}") should be narrowed to only handle expected errors: catch
yaml.YAMLError for parse errors and FileNotFoundError/OSError (or IOError) for
filesystem issues, log those with self._logger.warning including the filepath
and error, and re-raise or let any other unexpected exceptions propagate so they
are not masked; update both occurrences (the handler referencing master_filepath
and the similar handler at the other location) to follow this pattern and avoid
a bare Exception catch.
In `@searchboost_ui/Dockerfile`:
- Line 8: The Dockerfile uses a non-deterministic install command: replace the
RUN npm install line in the Dockerfile with RUN npm ci so the build uses the
lockfile for deterministic installs; ensure package-lock.json is copied into the
image before that RUN step and that the CI install is run in the same working
directory as package.json/package-lock.json.
- Around line 15-25: The Dockerfile currently relies on
nginxinc/nginx-unprivileged:stable-alpine running as UID 101 implicitly; make
the non-root runtime explicit and durable by adding a USER 101 directive (e.g.,
immediately after the FROM line or after copying files) so the image always runs
as the nginx unprivileged user; ensure this USER 101 appears before EXPOSE/CMD
so runtime processes inherit the non-root UID.
In `@searchboost_warden/src/configurator.rs`:
- Around line 82-83: The current call that deserializes settings uses
settings.try_deserialize().expect(...) which drops the underlying error; change
the failure path for try_deserialize() in the configurator to surface the root
cause by capturing the error and including it in the panic/log message (e.g.,
replace the bare expect on try_deserialize() with an explicit match,
unwrap_or_else, or expect_with formatted message that includes the error),
referencing the deserialization site (settings.try_deserialize()) and the
resulting Struct type (Self) so the message reads like "Warden Error: Config
file format is invalid: {error}" and preserves the original error details for
ops to inspect.
🪄 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: Pro
Run ID: ea16b447-0f39-4a5c-b58e-934c5351c458
📒 Files selected for processing (22)
.gsd/ROADMAP.md.gsd/STATE.mdMANUAL_TESTPLAN.mdconfigs/master_settings.ymldocker-compose.ymlscripts/install.shsearchboost_api/Dockerfilesearchboost_api/package.jsonsearchboost_api/src/app.jssearchboost_api/src/db/history.jssearchboost_api/src/db/pool.jssearchboost_api/src/middleware/auth.jssearchboost_api/src/routes/auth.jssearchboost_api/src/routes/search.jssearchboost_service/searchboost_src/configurator.pysearchboost_service/searchboost_src/service.pysearchboost_ui/Dockerfilesearchboost_ui/nginx.confsearchboost_ui/package.jsonsearchboost_warden/Cargo.tomlsearchboost_warden/src/configurator.rssearchboost_warden/src/relay.rs
✅ Files skipped from review due to trivial changes (10)
- searchboost_api/Dockerfile
- searchboost_warden/src/relay.rs
- searchboost_api/src/db/pool.js
- searchboost_ui/nginx.conf
- searchboost_api/package.json
- searchboost_api/src/middleware/auth.js
- searchboost_api/src/app.js
- .gsd/ROADMAP.md
- configs/master_settings.yml
- searchboost_ui/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- searchboost_warden/Cargo.toml
- .gsd/STATE.md
- searchboost_service/searchboost_src/service.py
- docker-compose.yml
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
MANUAL_TESTPLAN.md (1)
30-30:⚠️ Potential issue | 🟠 MajorMirror the FLUSHALL safety warning for Test 3.
Line 30 still uses
FLUSHALLwithout the same destructive-operation warning/prerequisite used in Test 2. This leaves a real data-loss footgun in shared Redis environments.🔧 Proposed patch
## Test 3: Post-Optimization Cache Check **Method:** Terminal (`curl` / `redis-cli`) **Objective:** Confirm that two different inputs requesting identically structured semantic outcomes match via the underlying optimized string, avoiding multiple external searches. **Steps:** 1. Clear the Redis cache (`FLUSHALL`). + > [!WARNING]+ > `FLUSHALL` is destructive and wipes ALL keys from the Redis instance. Use ONLY in an isolated test/dev Redis environment. 2. Enqueue Request A: "can you tell me who the current president of france is right now"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MANUAL_TESTPLAN.md` at line 30, Update the FLUSHALL instruction in Test 3 to mirror the safety warning and prerequisite text used in Test 2: locate the line containing "1. Clear the Redis cache (`FLUSHALL`)." and prepend the same destructive-operation warning and required confirmation/environment check wording from Test 2 (e.g., “Do not run in production / ensure you are on a disposable Redis instance / require confirmation”), so Test 3 carries the identical safety guidance as Test 2.
🧹 Nitpick comments (5)
MANUAL_TESTPLAN.md (1)
24-24: Scope the assertion to semantic-cache keys to avoid false failures.“Cache MUST be completely empty” can fail due to unrelated Redis keys. Make the expected outcome pattern-scoped (e.g.,
semantic_cache:*) and add an explicit check command.🧪 Suggested wording
-**Expected:** The cache MUST be completely empty for this query due to the PII detection.+**Expected:** No `semantic_cache:*` keys should be created for this query due to PII detection (other unrelated Redis keys may exist).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MANUAL_TESTPLAN.md` at line 24, Update the test expectation so it scopes the assertion to semantic cache keys instead of all Redis keys: replace the blanket "The cache MUST be completely empty" check with a pattern-scoped check for keys like semantic_cache:* and add an explicit verification command (e.g., scan/keys against pattern semantic_cache:* and assert zero matches). Ensure the test text references the pattern semantic_cache:* and includes the concrete check step to avoid false failures from unrelated Redis keys.searchboost_api/src/db/history.js (1)
46-64: Consider edge cases in turn reconstruction.The reconstruction logic assumes strict user→assistant pairing. If the database contains:
- Consecutive user messages (e.g., retries): second user gets
'...'placeholder- Orphan assistant messages (no preceding user): silently dropped
If these scenarios are possible in production data, consider logging or handling them explicitly. Otherwise, if invariants guarantee strict alternation, this is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/src/db/history.js` around lines 46 - 64, The turn-reconstruction loop in history.js currently assumes strict user→assistant pairs and drops orphan assistant turns or leaves consecutive user turns with '...' placeholders; update the logic around the for loop that iterates over turns (variables: turns, history, query, result) to explicitly handle edge cases: when encountering an assistant turn without a preceding user, log a warning and either attach it to the most recent history entry if its result is empty or skip it; when encountering consecutive user turns, attempt to find the next assistant turn ahead to pair with the earlier user and if none exists, keep the placeholder but log the missing-assistant case; ensure all log messages reference the turn index/role for traceability.searchboost_api/src/routes/auth.js (1)
78-81: Consider matching cookie options inclearCookiefor reliability.Some browsers require matching
path,sameSite, andsecureoptions when clearing cookies. While the default path/typically works, explicitly specifying options ensures consistent behavior.♻️ Proposed fix
router.post('/logout', (req, res) => { - res.clearCookie('sb_token');+ res.clearCookie('sb_token', {+ httpOnly: true,+ sameSite: 'strict',+ secure: process.env.NODE_ENV === 'production'+ }); res.status(200).json({ message: 'Logged out' }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/src/routes/auth.js` around lines 78 - 81, The logout route's clearCookie call in router.post('/logout') can fail in some browsers because cookie attributes must match when clearing; update the clearCookie('sb_token') invocation to pass the same options used when setting the token (e.g., path: '/', sameSite: 'Lax' or 'Strict' as appropriate, secure: true/false matching environment, and domain if used) so the cookie is reliably removed, and keep the rest of the response logic (res.status(200).json(...)) unchanged.searchboost_service/searchboost_src/configurator.py (2)
24-24: Remove unusedjsonimport.The
jsonmodule is imported but no longer used after switching from JSON to YAML configuration loading.🧹 Proposed fix
-import json , logging , os , aiofiles, yaml+import logging, os, aiofiles, yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_service/searchboost_src/configurator.py` at line 24, The import line in configurator.py includes an unused symbol "json"; remove "json" from the import statement (the line importing json , logging , os , aiofiles, yaml) so only the used modules remain (logging, os, aiofiles, yaml) to eliminate the unused import warning and keep imports tidy.
239-240: Consider narrowing exception types for config loading errors.Static analysis flags catching broad
Exception(BLE001). While the resilient fallback behavior is reasonable for configuration loading, narrowing to specific exceptions (e.g.,yaml.YAMLError,IOError) would prevent masking unexpected programming errors.🛡️ Proposed fix
try: if os.path.exists(master_filepath): async with aiofiles.open(master_filepath, 'r') as f: content = await f.read() master_data = yaml.safe_load(content) or {} deep_merge(data, master_data) - except Exception as e:+ except (yaml.YAMLError, IOError, OSError) as e: self._logger.warning(f"Config Loader: Error reading {master_filepath}: {e}") try: if os.path.exists(discrete_filepath): async with aiofiles.open(discrete_filepath, 'r') as f: content = await f.read() discrete_data = yaml.safe_load(content) or {} deep_merge(data, discrete_data) - except Exception as e:+ except (yaml.YAMLError, IOError, OSError) as e: self._logger.warning(f"Config Loader: Error reading {discrete_filepath}: {e}")Also applies to: 248-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_service/searchboost_src/configurator.py` around lines 239 - 240, Narrow the broad except clauses around the config-loading blocks that currently catch "except Exception as e" (the ones that log via self._logger.warning(f'Config Loader: Error reading {master_filepath}: {e}')) to only handle expected I/O and YAML parse errors (e.g., "except (yaml.YAMLError, OSError) as e" or "except (yaml.YAMLError, IOError, OSError) as e"); update both occurrences (the two try/except blocks that reference master_filepath and use self._logger.warning) and ensure yaml is imported where needed so unexpected programming errors are not masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@searchboost_api/src/routes/auth.js`:
- Around line 61-63: The code calls jwt.sign(...) in auth.js to produce token
without verifying process.env.JWT_SECRET, which allows creating forgeable
tokens; add a guard that throws a clear error if JWT_SECRET is missing (fail
fast at startup or immediately before the jwt.sign call that assigns token) —
e.g., validate process.env.JWT_SECRET in the module initialization or a shared
config and throw new Error('JWT_SECRET environment variable is required') so
jwt.sign is never called with an undefined secret.
- Around line 65-70: The cookie maxAge is hardcoded to 24h while the JWT
lifetime is configurable via JWT_EXPIRES_IN; update the res.cookie call that
sets 'sb_token' to derive maxAge from the JWT_EXPIRES_IN value instead of a
fixed 24h (e.g., parse JWT_EXPIRES_IN into milliseconds using a parser like the
ms package or convert numeric seconds to ms) so the cookie and token lifetimes
match; ensure you reference the same env var (JWT_EXPIRES_IN) and set
res.cookie('sb_token', token, { httpOnly: true, sameSite: 'strict', secure:
process.env.NODE_ENV === 'production', maxAge: <parsed JWT_EXPIRES_IN in ms> })
so sessions expire consistently.
In `@searchboost_api/src/routes/search.js`:
- Around line 16-20: The axios calls to Warden (e.g. the axios.post call that
posts to `${process.env.WARDEN_URL}/enqueue` and the other axios call around
line 42) lack timeouts and can hang; fix by adding a timeout (e.g. 30_000 ms) —
either pass a third argument { timeout: 30000 } to the individual
axios.post/axios.get calls (locate the
axios.post(`${process.env.WARDEN_URL}/enqueue`, { query, session_id, options:
options || {} }) invocation) or create a shared axios instance (e.g.
wardenClient = axios.create({ baseURL: process.env.WARDEN_URL, timeout: 30000
})) and replace raw axios usages with wardenClient.post/get for both
occurrences.
In `@searchboost_service/searchboost_src/configurator.py`:
- Line 237: master_data is being created via yaml.safe_load(content) which
leaves ${VAR} placeholders unexpanded; update the loading logic so placeholders
are resolved against environment and manual_env_data following the intended
precedence (Default < YAML < manual_env_data < CLI). Either run
os.path.expandvars on content before yaml.safe_load or post-process the
resulting master_data dict with a small expand_placeholders_in_dict routine that
replaces "${VAR}" values using os.environ and manual_env_data (respecting
manual_env_data overrides) so entries like REDIS_PASSWORD and DB_PASSWORD are
never left as literal "${...}" strings.
In `@searchboost_warden/src/configurator.rs`:
- Around line 61-78: In Settings::load, the Environment source created via
config::Environment::with_prefix("WARDEN").separator("__") needs to preserve the
"WARDEN" prefix so env keys map to the nested warden.* fields; update that call
to add .keep_prefix(true) (i.e.,
config::Environment::with_prefix("WARDEN").separator("__").keep_prefix(true))
when building the Config to restore environment override compatibility for keys
like WARDEN__NETWORK__RELAY_PORT.
---
Duplicate comments:
In `@MANUAL_TESTPLAN.md`:
- Line 30: Update the FLUSHALL instruction in Test 3 to mirror the safety
warning and prerequisite text used in Test 2: locate the line containing "1.
Clear the Redis cache (`FLUSHALL`)." and prepend the same destructive-operation
warning and required confirmation/environment check wording from Test 2 (e.g.,
“Do not run in production / ensure you are on a disposable Redis instance /
require confirmation”), so Test 3 carries the identical safety guidance as Test
2.
---
Nitpick comments:
In `@MANUAL_TESTPLAN.md`:
- Line 24: Update the test expectation so it scopes the assertion to semantic
cache keys instead of all Redis keys: replace the blanket "The cache MUST be
completely empty" check with a pattern-scoped check for keys like
semantic_cache:* and add an explicit verification command (e.g., scan/keys
against pattern semantic_cache:* and assert zero matches). Ensure the test text
references the pattern semantic_cache:* and includes the concrete check step to
avoid false failures from unrelated Redis keys.
In `@searchboost_api/src/db/history.js`:
- Around line 46-64: The turn-reconstruction loop in history.js currently
assumes strict user→assistant pairs and drops orphan assistant turns or leaves
consecutive user turns with '...' placeholders; update the logic around the for
loop that iterates over turns (variables: turns, history, query, result) to
explicitly handle edge cases: when encountering an assistant turn without a
preceding user, log a warning and either attach it to the most recent history
entry if its result is empty or skip it; when encountering consecutive user
turns, attempt to find the next assistant turn ahead to pair with the earlier
user and if none exists, keep the placeholder but log the missing-assistant
case; ensure all log messages reference the turn index/role for traceability.
In `@searchboost_api/src/routes/auth.js`:
- Around line 78-81: The logout route's clearCookie call in
router.post('/logout') can fail in some browsers because cookie attributes must
match when clearing; update the clearCookie('sb_token') invocation to pass the
same options used when setting the token (e.g., path: '/', sameSite: 'Lax' or
'Strict' as appropriate, secure: true/false matching environment, and domain if
used) so the cookie is reliably removed, and keep the rest of the response logic
(res.status(200).json(...)) unchanged.
In `@searchboost_service/searchboost_src/configurator.py`:
- Line 24: The import line in configurator.py includes an unused symbol "json";
remove "json" from the import statement (the line importing json , logging , os
, aiofiles, yaml) so only the used modules remain (logging, os, aiofiles, yaml)
to eliminate the unused import warning and keep imports tidy.
- Around line 239-240: Narrow the broad except clauses around the config-loading
blocks that currently catch "except Exception as e" (the ones that log via
self._logger.warning(f'Config Loader: Error reading {master_filepath}: {e}')) to
only handle expected I/O and YAML parse errors (e.g., "except (yaml.YAMLError,
OSError) as e" or "except (yaml.YAMLError, IOError, OSError) as e"); update both
occurrences (the two try/except blocks that reference master_filepath and use
self._logger.warning) and ensure yaml is imported where needed so unexpected
programming errors are not masked.
🪄 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: Pro
Run ID: b221d803-5047-433f-a049-496e2a34399a
📒 Files selected for processing (8)
MANUAL_TESTPLAN.mdscripts/install.shsearchboost_api/src/db/history.jssearchboost_api/src/routes/auth.jssearchboost_api/src/routes/search.jssearchboost_service/searchboost_src/configurator.pysearchboost_service/searchboost_src/service.pysearchboost_warden/src/configurator.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- searchboost_service/searchboost_src/service.py
- scripts/install.sh
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
61
to
78
| #[derive(Deserialize, Clone)] | ||
| pub struct Settings { | ||
| pub warden: WardenSettings, | ||
| pub redis: RedisSettings, | ||
| } | ||
| impl Settings { | ||
| pub fn load() -> Self { | ||
| let config_path = env::var("WARDEN_CONFIG_PATH") | ||
| .unwrap_or_else(|_| "../configs/warden.ini".to_string()); | ||
| let master_env = env::var("MASTER_CONFIG_PATH"); | ||
| let master_path = master_env.clone().unwrap_or_else(|_| "../configs/master_settings.yml".to_string()); | ||
| let discrete_env = env::var("WARDEN_CONFIG_PATH"); | ||
| let discrete_path = discrete_env.clone().unwrap_or_else(|_| "../configs/warden.yml".to_string()); | ||
| let settings = Config::builder() | ||
| .add_source(File::new(&config_path, FileFormat::Ini)) | ||
| .add_source(File::new(&master_path, FileFormat::Yaml).required(master_env.is_ok())) | ||
| .add_source(File::new(&discrete_path, FileFormat::Yaml).required(discrete_env.is_ok())) | ||
| .add_source(config::Environment::with_prefix("WARDEN").separator("__")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get the full configurator.rs file to understand the context
cat searchboost_warden/src/configurator.rs | head -150Repository: Somnerd/SearchBoost
Length of output: 3605
🏁 Script executed:
# Check the main.rs to see how settings are used
cat searchboost_warden/src/main.rs | head -100Repository: Somnerd/SearchBoost
Length of output: 2644
🏁 Script executed:
# Check Cargo.toml for config crate version
cat searchboost_warden/Cargo.toml | grep -A 5 "config"Repository: Somnerd/SearchBoost
Length of output: 215
🏁 Script executed:
# Check for any tests or examples using environment variables
rg "WARDEN__" searchboost_warden/ -A 2 -B 2Repository: Somnerd/SearchBoost
Length of output: 45
🏁 Script executed:
# Check for any documentation about config overrides
find searchboost_warden -type f \( -name "*.md" -o -name "*.yml" -o -name "*.yaml"\) -exec grep -l "WARDEN" {} \;Repository: Somnerd/SearchBoost
Length of output: 45
🏁 Script executed:
# Verify the config crate behavior by checking if there are other uses of Environment in the codebase
rg "config::Environment" searchboost_warden/Repository: Somnerd/SearchBoost
Length of output: 185
🌐 Web query:
config crate rust keep_prefix Environment with_prefix separator behavior 0.15
💡 Result:
In the Rust config crate version 0.15 (latest 0.15.21 as of 2026-03-12), the Environment struct provides methods to configure how environment variables are mapped to configuration keys, particularly when using a prefix via with_prefix("ENVIRONMENT"). Key methods: - with_prefix(s: &str) or prefix(s: &str): Sets an optional prefix (e.g., "ENVIRONMENT") that filters env vars. Only vars starting with {prefix}{prefix_separator} are considered. Example: "ENVIRONMENT_DEBUG" becomes "debug" key (prefix stripped by default). - prefix_separator(s: &str): Sets separator between prefix and key (defaults to separator or ""). - separator(s: &str): Sets separator for nested keys (e.g., "" for "redis_password" -> "redis.password"). Used as prefix_separator if not explicitly set. - keep_prefix(keep: bool): Controls whether the prefix is retained in the resulting config key. Default is false (strip prefix, so "ENVIRONMENT_debug" -> "debug"). If true, keeps it (e.g., "ENVIRONMENT_debug" -> "ENVIRONMENT.debug"). Usage example: use config::{Environment, Config}; let settings = Config::builder .add_source(Environment::with_prefix("ENVIRONMENT") .prefix_separator("") .separator("") .keep_prefix(false)) // Strip prefix .build?; Behavior notes from docs and issues: - Prefix matching requires {prefix}{prefix_separator} exactly. - Separator hierarchy: prefix_separator defaults to separator, then "". - keep_prefix(true) places the prefix as the top-level key in the config hierarchy. This is consistent in v0.15.x per official docs.rs documentation.
Citations:
- 1: https://docs.rs/crate/config/latest
- 2: https://docs.rs/config/latest/config/
- 3: https://docs.rs/config/latest/config/struct.Environment.html
- 4: Incorrect description of separator usage in Environment::with_prefix() rust-cli/config-rs#539
Environment variable overrides are broken after nesting settings under warden.
With the current code, WARDEN__NETWORK__RELAY_PORT and similar env var overrides map to network.relay_port instead of the expected warden.network.relay_port. The config crate strips the prefix by default; add .keep_prefix(true) to preserve the prefix as part of the generated key path so overrides target the nested structure.
🛠️ Fix: add keep_prefix to restore env override compatibility
let settings = Config::builder()
.add_source(File::new(&master_path, FileFormat::Yaml).required(master_env.is_ok()))
.add_source(File::new(&discrete_path, FileFormat::Yaml).required(discrete_env.is_ok()))
- .add_source(config::Environment::with_prefix("WARDEN").separator("__"))+ .add_source(+ config::Environment::with_prefix("WARDEN")+ .separator("__")+ .keep_prefix(true)+ )📝 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
| #[derive(Deserialize,Clone)] | |
| pubstructSettings{ | |
| pub warden:WardenSettings, | |
| pub redis:RedisSettings, | |
| } | |
| implSettings{ | |
| pubfn load() -> Self{ | |
| let config_path = env::var("WARDEN_CONFIG_PATH") | |
| .unwrap_or_else(|_| "../configs/warden.ini".to_string()); | |
| let master_env = env::var("MASTER_CONFIG_PATH"); | |
| let master_path = master_env.clone().unwrap_or_else(|_| "../configs/master_settings.yml".to_string()); | |
| let discrete_env = env::var("WARDEN_CONFIG_PATH"); | |
| let discrete_path = discrete_env.clone().unwrap_or_else(|_| "../configs/warden.yml".to_string()); | |
| let settings = Config::builder() | |
| .add_source(File::new(&config_path,FileFormat::Ini)) | |
| .add_source(File::new(&master_path,FileFormat::Yaml).required(master_env.is_ok())) | |
| .add_source(File::new(&discrete_path,FileFormat::Yaml).required(discrete_env.is_ok())) | |
| .add_source(config::Environment::with_prefix("WARDEN").separator("__")) | |
| #[derive(Deserialize,Clone)] | |
| pubstructSettings{ | |
| pub warden:WardenSettings, | |
| pub redis:RedisSettings, | |
| } | |
| implSettings{ | |
| pubfn load() -> Self{ | |
| let master_env = env::var("MASTER_CONFIG_PATH"); | |
| let master_path = master_env.clone().unwrap_or_else(|_| "../configs/master_settings.yml".to_string()); | |
| let discrete_env = env::var("WARDEN_CONFIG_PATH"); | |
| let discrete_path = discrete_env.clone().unwrap_or_else(|_| "../configs/warden.yml".to_string()); | |
| let settings = Config::builder() | |
| .add_source(File::new(&master_path,FileFormat::Yaml).required(master_env.is_ok())) | |
| .add_source(File::new(&discrete_path,FileFormat::Yaml).required(discrete_env.is_ok())) | |
| .add_source( | |
| config::Environment::with_prefix("WARDEN") | |
| .separator("__") | |
| .keep_prefix(true) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@searchboost_warden/src/configurator.rs` around lines 61 - 78, In
Settings::load, the Environment source created via
config::Environment::with_prefix("WARDEN").separator("__") needs to preserve the
"WARDEN" prefix so env keys map to the nested warden.* fields; update that call
to add .keep_prefix(true) (i.e.,
config::Environment::with_prefix("WARDEN").separator("__").keep_prefix(true))
when building the Config to restore environment override compatibility for keys
like WARDEN__NETWORK__RELAY_PORT.
added 2 commits
March 28, 2026 01:18
- Add Fail-Closed guard for DB_PASSWORD in Node.js API - Add 5s Axios timeouts to Warden proxy calls - Implement shell variable expansion for YAML configs in Python - Flatten Warden config struct for reliable Environment overrides - Sync HttpOnly cookie maxAge with JWT_EXPIRES_IN
- Update ROADMAP.md with Dynamic LLM Selection and Hybrid Rust Migration - Synchronize notes/ with Phase 6 'Pristine' architecture - Finalize README.md manifest and getting started guide
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
searchboost_service/searchboost_src/configurator.py (1)
233-250:⚠️ Potential issue | 🟠 MajorUnset
${VAR}placeholders still make it through as live config values.This is the same deployment risk as the earlier placeholder comment:
os.path.expandvars()leaves unknown variables unchanged, so an unset${REDIS_PASSWORD}/${DB_PASSWORD}survives YAML parsing as a literal string. Because both load paths also swallowException, the worker can continue instead of failing closed when a required secret was never resolved.#!/bin/bashset -euo pipefail echo"=== YAML placeholders still present in repo configs ===" rg -n '\$\{[A-Z0-9_]+\}' --glob '*.yml' --glob '*.yaml'echoecho"=== Python expandvars behavior for an unset variable ===" python - <<'PY'import ossample = "${REDIS_PASSWORD}"os.environ.pop("REDIS_PASSWORD", None)print("input :", sample)print("output:", os.path.expandvars(sample))PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_service/searchboost_src/configurator.py` around lines 233 - 250, After expanding environment variables with os.path.expandvars in the file-read blocks, detect any remaining unresolved ${VAR} placeholders (e.g., regex r'\$\{[A-Z0-9_]+\}') in expanded_content and fail-fast: raise or log an error and stop loading instead of swallowing the exception; update the try/except around the master/discrete file reads (the blocks using aiofiles.open, os.path.expandvars, yaml.safe_load and deep_merge) to validate expanded_content for unresolved placeholders and, if any are found, call self._logger.error with the file path and the list of unresolved variables and then raise an exception so the worker does not continue with literal placeholder values.searchboost_warden/src/configurator.rs (1)
67-75:⚠️ Potential issue | 🔴 Critical
WARDEN_CONFIG_PATHnow fails closed, but the configured YAML still looks missing.This builder will now panic when
WARDEN_CONFIG_PATHis set and/configs/warden.ymldoes not exist. The repo context still showsdocker-compose.ymlexporting that path while the checked-in Warden config isconfigs/warden.ini, so the default stack will not boot unless a matchingwarden.ymlwas added elsewhere in this PR.#!/bin/bashset -euo pipefail echo"=== Warden config env wiring ===" rg -n 'MASTER_CONFIG_PATH|WARDEN_CONFIG_PATH' docker-compose.yml echoecho"=== Warden config files present in the repo ===" fd -a '^warden\.(ya?ml|ini)$'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_warden/src/configurator.rs` around lines 67 - 75, The current Config::builder call uses .required(discrete_env.is_ok()) which causes a panic if WARDEN_CONFIG_PATH is set but the referenced file doesn't exist; fix this by making the requirement depend on actual file existence instead of just the env var—use std::path::Path::new(&discrete_path).exists() to decide the .required(...) flag (or set .required(false) and explicitly log/warn if the env var is set but the file is missing), updating the File::new(..., FileFormat::Yaml).required(...) invocation for discrete_path (and similarly for master_path/master_env if desired) so the builder only fails when the file truly exists or is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gsd/ARCHITECTURE.md:
- Around line 54-56: The doc incorrectly states that Node dispatches a prebuilt
`job_id` to Rust Warden; update step 4 to state Node dispatches the `session_id`
(not `job_id`) to Warden, reflecting the enqueue route in
searchboost_api/src/routes/search.js that sends `{ query, session_id }`; clarify
the handoff contract by noting the `job_id` is created by Warden after enqueue
and only validated later (step 5) against the request's origin user.
- Around line 74-75: The "## Technical Debt" section currently hard-codes "- [ ]
*None Detected.* The repository possesses 0 unresolved TODO, FIXME, or HACK
comments." which will go stale; update that line in ARCHITECTURE.md by either
removing the absolute "0" claim and leaving a neutral note (e.g., "See current
audit for outstanding TODO/FIXME/HACK items") or replace it with a link or badge
to the audit/CI artifact that generates the count, and ensure the text
references the "## Technical Debt" heading so maintainers know where the
dynamic/generated source lives.
In @.gsd/STACK.md:
- Around line 37-38: Update the STACK.md table to reflect that
MASTER_CONFIG_PATH is a concrete file path (as set in
searchboost_warden/src/configurator.rs) pointing to the master_settings.yml file
rather than a generic "/configs" root, and add a separate entry for
SEARCHBOOST_CONFIG_DIR (the Python worker's directory discovery variable)
documenting it as the directory root (default /configs); reference the symbols
MASTER_CONFIG_PATH and SEARCHBOOST_CONFIG_DIR when editing the table so the rows
correctly describe the distinct knobs.
In @.gsd/STATE.md:
- Around line 1-16: Replace the initial second-level heading "## Current
Position" with a top-level H1 (e.g., "# Current Position") and ensure all
section headings ("## Last Session Summary", "## Next Steps") are preceded and
followed by a single blank line so they comply with markdownlint rules
MD041/MD022; update the file's heading levels and insert blank lines around each
heading (Current Position, Last Session Summary, Next Steps) to eliminate the
lint warnings.
In `@notes/FlowDiagram.pu`:
- Around line 13-15: The diagram uses the bare routes but the express routes in
searchboost_api/src/routes/search.js are mounted under /api/search, so update
the diagram lines (e.g., the POST /enqueue and GET/POST /result/:job_id usages)
to include the /api prefix—change POST /search/enqueue to POST
/api/search/enqueue and any /result/:job_id references to
/api/search/result/:job_id (also update the other occurrences noted at lines
41-42) so the public contract matches the mounted routes.
- Around line 39-43: The diagram shows the worker writing Redis key
`job_result:uuid` but the worker implementation
(searchboost_service/searchboost_src/worker.py) actually writes completed
results to `sb:result:{job_id}`; update the diagram so WKR -> RD uses `SET
sb:result:{job_id}` and the Warden read path (WRD -> RD) uses `GET
sb:result:{job_id}` to match the worker's key contract.
In `@notes/TODO.md`:
- Around line 3-38: Remove the trailing space on the second line of the file and
ensure each Markdown heading has a blank line before and after it: add blank
lines around the headings "🟢 CURRENT STATUS: Phase 6 Fully Hardened", "✅ RECENT
ACHIEVEMENTS (Phase 6)", "🏗️ NEXT OBJECTIVES: Phase 7 & Multi-Tenancy", "🐛
BUGS & DEBT (KINDLING)", "🩹 MAINTENANCE & BUG FIXES (ARCHIVED)", and "🔮
POST-MVP: Intelligence Additions" so linter complaints (lines flagged for
missing surrounding blank lines) are resolved.
In `@README.md`:
- Line 61: Replace the corrupted characters in the README heading "## ��
Technical Roadmap" with the correct emoji (e.g., "🗺️" or "🗺") so the heading
reads "## 🗺️ Technical Roadmap"; update the exact heading string "## ��
Technical Roadmap" to the corrected emoji version and commit the change to the
README.md.
- Around line 20-42: Add a single blank line after each Markdown heading to
satisfy style rules: insert an empty line after the "🛡️ Reliability Features",
"🛠️ Project Structure", and "🚀 Getting Started" headings so there is a blank
line between each heading line and the following content (fix the headings named
exactly "🛡️ Reliability Features", "🛠️ Project Structure", and "🚀 Getting
Started").
In `@searchboost_api/src/app.js`:
- Around line 35-42: The startup env validation omits WARDEN_URL causing the
service to run without that mandatory config; update the requiredEnv array in
the async function start (in app.js) to include 'WARDEN_URL' so missing
WARDEN_URL is detected and the process exits early (same pattern used for
'JWT_SECRET' and 'DB_PASSWORD'); ensure the error message still reports all
missing keys via missing.join(', ') so operators see WARDEN_URL listed when
absent.
In `@searchboost_api/src/routes/auth.js`:
- Around line 61-69: The current parsing of JWT_EXPIRES_IN (expiresInString /
matchTime / maxAge) only accepts formats like "24h" and falls back to 24h for
numeric or space-separated values; update the logic to correctly interpret all
valid jsonwebtoken expiry formats by: if expiresInString is purely numeric treat
it as seconds, accept space-separated values like "2 days" by normalizing
whitespace and units, and preferably use a robust parser such as the "ms"
library (or a small helper) to convert the expiresInString into milliseconds
before assigning maxAge; keep the existing multipliers fallback (s/m/h/d) for
backwards compatibility and ensure maxAge always reflects the computed
milliseconds for token expiry.
---
Duplicate comments:
In `@searchboost_service/searchboost_src/configurator.py`:
- Around line 233-250: After expanding environment variables with
os.path.expandvars in the file-read blocks, detect any remaining unresolved
${VAR} placeholders (e.g., regex r'\$\{[A-Z0-9_]+\}') in expanded_content and
fail-fast: raise or log an error and stop loading instead of swallowing the
exception; update the try/except around the master/discrete file reads (the
blocks using aiofiles.open, os.path.expandvars, yaml.safe_load and deep_merge)
to validate expanded_content for unresolved placeholders and, if any are found,
call self._logger.error with the file path and the list of unresolved variables
and then raise an exception so the worker does not continue with literal
placeholder values.
In `@searchboost_warden/src/configurator.rs`:
- Around line 67-75: The current Config::builder call uses
.required(discrete_env.is_ok()) which causes a panic if WARDEN_CONFIG_PATH is
set but the referenced file doesn't exist; fix this by making the requirement
depend on actual file existence instead of just the env var—use
std::path::Path::new(&discrete_path).exists() to decide the .required(...) flag
(or set .required(false) and explicitly log/warn if the env var is set but the
file is missing), updating the File::new(..., FileFormat::Yaml).required(...)
invocation for discrete_path (and similarly for master_path/master_env if
desired) so the builder only fails when the file truly exists or is required.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c00a4f70-8713-4873-b0f7-944a69f49d66
📒 Files selected for processing (15)
.gsd/ARCHITECTURE.md.gsd/ROADMAP.md.gsd/STACK.md.gsd/STATE.mdREADME.mdnotes/DetailedAudit.mdnotes/ENGINEERING_MANUAL.mdnotes/FlowDiagram.punotes/SystemDesign.mdnotes/TODO.mdsearchboost_api/src/app.jssearchboost_api/src/routes/auth.jssearchboost_api/src/routes/search.jssearchboost_service/searchboost_src/configurator.pysearchboost_warden/src/configurator.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+37
to
+38
| | MASTER_CONFIG_PATH| Base YAML configuration root | No (defaults to /configs) | | ||
| | SB_INSTANCE_IP | Target deployment IP for testplan resolution | No | |
There was a problem hiding this comment.
MASTER_CONFIG_PATH is documented as the wrong kind of setting.
In searchboost_warden/src/configurator.rs, MASTER_CONFIG_PATH is a concrete file path (.../master_settings.yml), not a generic /configs root. The Python worker uses a different variable (SEARCHBOOST_CONFIG_DIR) for directory discovery, so this table currently conflates two separate knobs.
Suggested doc fix
-| MASTER_CONFIG_PATH| Base YAML configuration root | No (defaults to /configs) |+| MASTER_CONFIG_PATH | Warden path to `master_settings.yml` | No (defaults to `../configs/master_settings.yml`) |+| SEARCHBOOST_CONFIG_DIR | Worker config directory override | No |📝 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
| | MASTER_CONFIG_PATH| Base YAML configuration root | No (defaults to /configs) | | |
| | SB_INSTANCE_IP | Target deployment IP for testplan resolution | No | | |
| | MASTER_CONFIG_PATH | Warden path to `master_settings.yml`| No (defaults to `../configs/master_settings.yml`) | | |
| | SEARCHBOOST_CONFIG_DIR | Worker config directory override | No | | |
| | SB_INSTANCE_IP | Target deployment IP for testplan resolution | No | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gsd/STACK.md around lines 37 - 38, Update the STACK.md table to reflect
that MASTER_CONFIG_PATH is a concrete file path (as set in
searchboost_warden/src/configurator.rs) pointing to the master_settings.yml file
rather than a generic "/configs" root, and add a separate entry for
SEARCHBOOST_CONFIG_DIR (the Python worker's directory discovery variable)
documenting it as the directory root (default /configs); reference the symbols
MASTER_CONFIG_PATH and SEARCHBOOST_CONFIG_DIR when editing the table so the rows
correctly describe the distinct knobs.
Comment on lines
+1
to
+16
| ## Current Position | ||
| - **Phase**: 6 (verified) | ||
| - **Task**: Security Hardening & Precedence Overhaul (Phase 6.5) | ||
| - **Status**: ✅ Complete and PR #6 Updated | ||
| ## Last Session Summary | ||
| Phase 6 was transformed into a deep security audit after CodeRabbit flagged critical vulnerabilities. We resolved: | ||
| 1. **Critical IDOR Collision**: Switched to colon-separated session IDs (`SB-SESSION:user:thread`) to block name-prefix attacks. | ||
| 2. **Strict Secrets Enforcement**: Eliminated all default password fallbacks in API, Worker, and Warden. Implemented `chmod 600` on `.env`. | ||
| 3. **Docker Isolation**: API and UI containers now run as unprivileged users (node/nginx-unprivileged). | ||
| 4. **Config Overhaul**: Implemented recursive deep-merge and CLI > ENV > YAML precedence in the Python configurator. | ||
| 5. **History Persistence**: Fixed cache-hit history drops in `SearchBoostService.run()`. | ||
| ## Next Steps | ||
| 1. Merge PR #6 into `dev`. | ||
| 2. Final review of audit results in `.gsd/phases/6/6-VERIFICATION.md`. |
There was a problem hiding this comment.
This file still violates the heading rules from markdownlint.
The file starts with ## instead of an H1, and the section headings are not surrounded by blank lines, so MD041/MD022 will keep firing.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
[warning] 6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 14-14: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gsd/STATE.md around lines 1 - 16, Replace the initial second-level heading
"## Current Position" with a top-level H1 (e.g., "# Current Position") and
ensure all section headings ("## Last Session Summary", "## Next Steps") are
preceded and followed by a single blank line so they comply with markdownlint
rules MD041/MD022; update the file's heading levels and insert blank lines
around each heading (Current Position, Last Session Summary, Next Steps) to
eliminate the lint warnings.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+3
to
+38
| This master roadmap consolidates all architecture phases, stabilizing tasks, and the newest feature mandates into a single path forward. | ||
| --- | ||
| ## 🔴 HIGH PRIORITY: Current Objectives (Phase 2 & UI) | ||
| ## 🟢 CURRENT STATUS: Phase 6 Fully Hardened | ||
| We have successfully completed the **Security & Stability Sweep (Phase 6)**, resolving 11+ critical issues identified by CodeRabbit. | ||
| * [ ] **Governor Implementation**: Add the `governor` and `tower-governor` crates to the Rust Warden to implement strict GCRA rate limiting on the `/enqueue` endpoint, protecting the downstream AI services. | ||
| * [ ] **Web UI & Containerization**: Build a modern, stunning frontend dashboard (e.g. Next.js/Vite) to interact with the Warden's relay API. Write a `Dockerfile` for it and add it to the stack. | ||
| ## 🟠 MEDIUM PRIORITY: Build & Environment Stability | ||
| *The 10-minute build cycle is currently the primary blocker for feature velocity.* | ||
| * [ ] **Docker Cache Optimization**: Refactor `searchboost_warden/Dockerfile` to cache dependencies separately from source (Target: < 2m rebuilds) and implement incremental build volumes (`target/`). | ||
| * [ ] **Dev-Mode Toggle**: Update `docker-compose.yml` to support standard `cargo build` (Debug) instead of `--release` for faster local cycles. | ||
| * [ ] **Configurator Generalization**: | ||
| * [ ] **Unified Settings**: Move to a master `settings.yaml` shared across Rust and Python. | ||
| * [ ] **Pathing Fix**: Implement absolute pathing in `configurator.py` and Warden to ensure settings are picked up regardless of container working directory. | ||
| --- | ||
| ## ✅ Phase 1 & 2: Infrastructure & Abstraction (COMPLETED) | ||
| * [X] Fix Network Bindings, Correct Redis Authentication, Visibility Fix, Log Observation. | ||
| * [X] Warden Result Polling, Orchestrator Redis Purge, Warden ID Authority. | ||
| * [X] Health Check Endpoints (`/health`). | ||
| * [X] **Warden Serialization Fix**: Modified Rust relay to produce perfect Tuple pickle bytes to match Arq's default deserializer. (Completed Phase 1 Fixes). | ||
| --- | ||
| ## 🏗️ Phase 3: Resilience & Production Hardening | ||
| * [ ] **Exponential Backoff**: Replace static polling with jittered backoff logic in `main.py`. | ||
| * [ ] **Request Validation**: Implement strict Pydantic/Rust schema validation for payloads. | ||
| * [ ] **Worker Scaling**: Test horizontal scaling of workers with the new Composite Key locality. | ||
| * [ ] **Log Rotation**: Ensure `collect_logs.sh` handles log purging and archive management. | ||
| * [ ] **Sovereign Handshake**: Finalize the logic where Warden has 100% authority over ID generation and enqueuing. | ||
| * [ ] **Health & Observation**: | ||
| * [ ] Integrate **Grafana & Prometheus** for real-time circuit breaker and latency monitoring. | ||
| --- | ||
| ## 🚀 Phase 4: Strategy & Architecture Finalization | ||
| * [ ] **Folder Separation**: Physical split into `searchboost_client/` and `searchboost_worker/`. | ||
| * [ ] **Refine "Self-Hosted" Config**: Ensure the INI/JSON system remains "Enthusiast-Friendly" while supporting enterprise features. | ||
| * [ ] **Documentation**: Complete the `SystemDesign.md` and `README.md` reflecting the new "Authority" model. | ||
| * [ ] **Test Suite Foundation**: | ||
| * [ ] Create Redis mock for Warden unit tests. | ||
| * [ ] Build a "Sovereign Handshake" integration test script. | ||
| * [ ] Payload injection tests for the Worker. | ||
| ### ✅ RECENT ACHIEVEMENTS (Phase 6) | ||
| * **Critical IDOR Protection**: Implemented colon-delimited session IDs (`SB-SESSION:user:thread`) and strict ownership validation to prevent cross-user data leaks. | ||
| * **Fail-Closed Secrets**: Removed all hardcoded fallback credentials. System now mandates explicit `.env` for boot. | ||
| * **Rootless Containers**: API and UI containers migrated to non-root users (`node`/`nginx-unprivileged`). | ||
| * **Config Precedence**: Fixed Python configurator to prioritize `CLI > ENV > YAML` and implemented recursive deep-merging. | ||
| * **History Synchronization**: Fixed race conditions and logic bypasses in `SearchBoostService.run()` to ensure 100% conversation persistence. | ||
| * **PII-Safe Caching**: Integrated `PIIDetector` with a triple-gate cache strategy. | ||
| --- | ||
| ## 🔮 Phase 5: IO Normalization & Generalization (Post-MVP) | ||
| ## 🏗️ NEXT OBJECTIVES: Phase 7 & Multi-Tenancy | ||
| * **[ ] Multi-Tenancy isolation**: Deep-test the new colon-separated session IDs with concurrent users in a production-like staging environment. | ||
| * **[ ] Deployment Strategy**: Prepare `docker-compose.prod.yml` with proper ACME/SSL termination and Nginx proxying. | ||
| * **[ ] Vector RAG Integration**: Plan `pgvector` migration for the PostgreSQL database to enable semantic history search. | ||
| *Goal: Replace pickle-based serialization with a standardized, language-agnostic format across the entire service module.* | ||
| ## 🐛 BUGS & DEBT (KINDLING) | ||
| * [ ] **Entropy/TTL for Time-Sensitive Queries**: Implement context-aware validation or shorter TTLs for caching time-sensitive answers (e.g., current time/date). | ||
| * [ ] **Warden Observation Timestamps**: Fix `sb_warden` observation logic to ensure container logs are captured with accurate timestamps. | ||
| * [ ] **Exponential Backoff**: Replace static polling with jittered backoff logic in the React UI for result fetching. | ||
| * [ ] **Normalized IO Handshake**: Define a strictly versioned schema (JSON/Protobuf) for Warden ↔ Service comms. | ||
| * [ ] **Translation Adapter**: Implement an adapter in front of the Worker to map standard IO to `arq` inputs. | ||
| * [ ] **User Auth Microservice**: Build standalone Auth service & implement JWT propagation from UI → Warden → Worker for multi-tenant isolation. | ||
| * [ ] **Update Warden Relay**: Remove pickle dependency and use normalized format. | ||
| ## 🩹 MAINTENANCE & BUG FIXES (ARCHIVED) | ||
| * [X] **RFC Compliance (SearXNG)**: Renamed `sb_searxng` to `sb-searxng`. | ||
| * [X] **Governor Implementation**: 25 req/s rate limiting with burstfallback of 100 via `tower-governor`. | ||
| * [X] **Web UI & Containerization**: Full React + Node.js + PostgreSQL stack containerized. | ||
| * [X] **(UI) Session Isolation**: Resolved session leakage through multi-thread sidebar implementation. | ||
| --- | ||
| ## 🟢 FEATURE ROADMAP: Intelligence Additions | ||
| * [ ] **Semantic Caching**: Optimize Ollama for semantic similarity checks to skip redundant web searches. | ||
| * [ ] **Postgres Search**: Implement full-text search and `pgvector` indexing on the local DB. | ||
| * [ ] **SearxNG Local Plugin**: Custom plugin to index and search local project files. | ||
| ## 🔮 POST-MVP: Intelligence Additions |
There was a problem hiding this comment.
Fix markdown formatting issues flagged by linter.
Static analysis detected:
- Line 3: Trailing space
- Lines 7, 10, 20, 25, 30, 38: Headings should be surrounded by blank lines
🔧 Proposed fix
-This master roadmap consolidates all architecture phases, stabilizing tasks, and the newest feature mandates into a single path forward. +This master roadmap consolidates all architecture phases, stabilizing tasks, and the newest feature mandates into a single path forward.
---
## 🟢 CURRENT STATUS: Phase 6 Fully Hardened
+
We have successfully completed the **Security & Stability Sweep (Phase 6)**, resolving 11+ critical issues identified by CodeRabbit.
### ✅ RECENT ACHIEVEMENTS (Phase 6)
+
* **Critical IDOR Protection**: Implemented colon-delimited session IDs...Apply similar blank-line additions after all headings.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 3-3: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 10-10: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 20-20: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 25-25: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 30-30: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 38-38: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@notes/TODO.md` around lines 3 - 38, Remove the trailing space on the second
line of the file and ensure each Markdown heading has a blank line before and
after it: add blank lines around the headings "🟢 CURRENT STATUS: Phase 6 Fully
Hardened", "✅ RECENT ACHIEVEMENTS (Phase 6)", "🏗️ NEXT OBJECTIVES: Phase 7 &
Multi-Tenancy", "🐛 BUGS & DEBT (KINDLING)", "🩹 MAINTENANCE & BUG FIXES
(ARCHIVED)", and "🔮 POST-MVP: Intelligence Additions" so linter complaints
(lines flagged for missing surrounding blank lines) are resolved.
Comment on lines
20
to
42
| ## 🛡️ Reliability Features | ||
| * **Composite Key Locality**: Uses `{session}:uuid` formatting to ensure data stays close to the user in clustered environments. | ||
| * **Circuit Breaking**: Automatically trips when Redis or the Worker fails, routing traffic through an isolated **Fallback Handler**. | ||
| * **Log Observation**: The Warden observes Docker container health in real-time. | ||
| * **Colon-Delimited Identity**: Delimiter-safe session identifiers to prevent prefix collision attacks. | ||
| * **Circuit Breaking**: Automatically trips when downstream microservices fail. | ||
| * **Fail-Closed Security**: Services crash on boot if required secrets (JWT/DB) are missing. | ||
| * **Unprivileged Containers**: All services run as non-root users. | ||
| --- | ||
| ## 🛠️ Project Structure | ||
| ```bash | ||
| <<<<<<< HEAD | ||
| SearchBoost/ | ||
| ├── configs/ # Unified INI/JSON configurations | ||
| ├── logs/ # Aggregated log directory (Worker, Warden, DB) | ||
| ├── configs/ # Unified YAML configurations (master, warden, worker) | ||
| ├── notes/ # Tech specs and Roadmap | ||
| ├── scripts/ # Dev utilities (log collectors, permission fixes) | ||
| ├── searchboost_service/ # Orchestrator & Client logic (Python) | ||
| └── searchboost_warden/ # The Reliability Sidecar (Rust) | ||
| ======= | ||
| git clone https://github.com/Somnerd/SearchBoost.git | ||
| ``` | ||
| 2. **Build the Infrastructure:** | ||
| ```Bash | ||
| docker-compose build --no-cache | ||
| ``` | ||
| 3. **Spin up the Infrastructure:** | ||
| ```Bash | ||
| docker-compose up -d | ||
| ``` | ||
| 4. **Run the Search Pipeline:** | ||
| ```Bash | ||
| cd searchboost_src && python main.py --query "architecture patterns" | ||
| >>>>>>> main | ||
| ``` | ||
| ├── scripts/ # Dev utilities (install.sh, log collectors) | ||
| ├── searchboost_api/ # Express Gateway (Node.js) | ||
| ├── searchboost_service/ # Research Worker (Python) | ||
| ├── searchboost_ui/ # Modern Web Interface (React) | ||
| └── searchboost_warden/ # Reliability Sidecar (Rust) | ||
| --- | ||
| ## 🚀 Getting Started |
There was a problem hiding this comment.
Add blank lines after headings per markdown style.
Static analysis flagged lines 21, 31, 42 as needing blank lines after headings.
🔧 Proposed fix (example for line 21)
## 🛡️ Reliability Features
+
* **Colon-Delimited Identity**: Delimiter-safe session identifiers...Apply similar fixes after headings on lines 31 and 42.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 21-21: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 42-42: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 20 - 42, Add a single blank line after each Markdown
heading to satisfy style rules: insert an empty line after the "🛡️ Reliability
Features", "🛠️ Project Structure", and "🚀 Getting Started" headings so there
is a blank line between each heading line and the following content (fix the
headings named exactly "🛡️ Reliability Features", "🛠️ Project Structure", and
"🚀 Getting Started").
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+35
to
+42
| async function start() { | ||
| const requiredEnv = ['JWT_SECRET', 'DB_PASSWORD']; | ||
| const missing = requiredEnv.filter(k => !process.env[k]); | ||
| if (missing.length > 0) { | ||
| console.error(`FATAL: Missing mandatory environment variables: ${missing.join(', ')}`); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Add WARDEN_URL to startup validation for fail-closed consistency.
WARDEN_URL is used in search.js (lines 16, 42) and admin.js (line 64) but is not validated at startup. While runtime error handling exists, requests will fail with 503 "Could not reach Warden" rather than preventing boot with a clear configuration error.
🔧 Proposed fix
async function start() {
- const requiredEnv = ['JWT_SECRET', 'DB_PASSWORD'];+ const requiredEnv = ['JWT_SECRET', 'DB_PASSWORD', 'WARDEN_URL'];
const missing = requiredEnv.filter(k => !process.env[k]);📝 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
| asyncfunctionstart(){ | |
| constrequiredEnv=['JWT_SECRET','DB_PASSWORD']; | |
| constmissing=requiredEnv.filter(k=>!process.env[k]); | |
| if(missing.length>0){ | |
| console.error(`FATAL: Missing mandatory environment variables: ${missing.join(', ')}`); | |
| process.exit(1); | |
| } | |
| asyncfunctionstart(){ | |
| constrequiredEnv=['JWT_SECRET','DB_PASSWORD','WARDEN_URL']; | |
| constmissing=requiredEnv.filter(k=>!process.env[k]); | |
| if(missing.length>0){ | |
| console.error(`FATAL: Missing mandatory environment variables: ${missing.join(', ')}`); | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@searchboost_api/src/app.js` around lines 35 - 42, The startup env validation
omits WARDEN_URL causing the service to run without that mandatory config;
update the requiredEnv array in the async function start (in app.js) to include
'WARDEN_URL' so missing WARDEN_URL is detected and the process exits early (same
pattern used for 'JWT_SECRET' and 'DB_PASSWORD'); ensure the error message still
reports all missing keys via missing.join(', ') so operators see WARDEN_URL
listed when absent.
Uh oh!
There was an error while loading. Please reload this page.
- Sync /api/ prefixes in FlowDiagram.pu - Clarify MASTER_CONFIG_PATH bootstrap docs in Rust - Upgrade JWT cookie TTL parser to handle pure numeric seconds - Refine session/job ID terminology in ARCHITECTURE.md - Update Technical Debt section with realistic monitoring goals
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
notes/FlowDiagram.pu (1)
39-43:⚠️ Potential issue | 🟡 MinorContract drift remains in result key + polling param naming.
Line 39/Line 43 still use
job_result:uuid, and Line 41 uses:id. If current implementations remain as previously reviewed, this should besb:result:{job_id}and:job_idto match the real API/worker contract.Suggested doc fix
- WKR -> RD : SET job_result:uuid+ WKR -> RD : SET sb:result:{job_id}- UI -> API : GET /api/search/result/:id+ UI -> API : GET /api/search/result/:job_id- WRD -> RD : GET job_result:uuid+ WRD -> RD : GET sb:result:{job_id}Use this read-only check to confirm the current contract in code before merging:
#!/bin/bashset -euo pipefail echo"== Locate search route definitions ==" fd -i 'search.js'| xargs -r rg -nP --type=js -C2 'result\s*/:\w+|enqueue|router\.(get|post)\s*\('echoecho"== Locate worker Redis result key usage ==" fd -i 'worker.py'| xargs -r rg -nP --type=py -C2 'sb:result|job_result'Expected: route shows
/result/:job_id; worker key usage showssb:result:{job_id}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notes/FlowDiagram.pu` around lines 39 - 43, Update the flow diagram labels to match the real API/worker contract: replace the Redis key label "job_result:uuid" (seen on the WKR -> RD and WRD -> RD arrows) with "sb:result:{job_id}" and change the route parameter "/api/search/result/:id" to "/api/search/result/:job_id" (replace ":id" with ":job_id") so the UI -> API and proxy steps match the worker key usage; verify the arrows/labels mentioning WKR, WRD, and RD are updated accordingly and run the provided read-only shell checks to confirm route and worker usages align.searchboost_api/src/routes/auth.js (3)
79-81:⚠️ Potential issue | 🟠 MajorGuard against missing
JWT_SECRETat startup or sign time.This issue was flagged in a previous review and remains unaddressed. If
JWT_SECRETis undefined,jwt.signproduces tokens signed with an empty/undefined secret, making them trivially forgeable.🛡️ Proposed fix — fail fast at module load
Add at the top of the file after imports:
if(!process.env.JWT_SECRET){thrownewError('JWT_SECRET environment variable is required');}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/src/routes/auth.js` around lines 79 - 81, The code calls jwt.sign(...) to create token using process.env.JWT_SECRET but doesn't guard against a missing secret; add a fail-fast check for process.env.JWT_SECRET at module load (immediately after imports in auth.js) and throw a clear Error if it's falsy so jwt.sign cannot run with an undefined secret; reference the JWT_SECRET env var and the jwt.sign usage that produces the token to locate where to add the check.
11-14:⚠️ Potential issue | 🟠 MajorValidate credential types before length checks and bcrypt calls.
This issue was flagged in a previous review and remains unaddressed. Non-string payloads (e.g., arrays, objects, numbers) can bypass the current truthy checks and cause 500 errors when
bcrypt.hash/bcrypt.comparereceive unexpected types.🔧 Proposed fix
+function isNonEmptyString(v) {+ return typeof v === 'string' && v.trim().length > 0;+}+ router.post('/register', async (req, res, next) => { try { const { username, password } = req.body; - if (!username || !password) {+ if (!isNonEmptyString(username) || !isNonEmptyString(password)) { return res.status(400).json({ error: 'Username and password are required' }); }Apply the same check in
/loginat line 41.Also applies to: 40-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/src/routes/auth.js` around lines 11 - 14, Request body credential values are only being checked for truthiness, which allows non-string types (arrays, objects, numbers) to pass and later crash bcrypt.hash/bcrypt.compare; update both the register handler (where const { username, password } = req.body is used) and the login handler (the similar block around line 41) to first verify typeof username === 'string' && typeof password === 'string' (and return 400 with a clear message if not) before performing length checks or calling bcrypt.hash/bcrypt.compare, ensuring invalid types are rejected early.
61-77: 🧹 Nitpick | 🔵 TrivialmaxAge parsing now handles pure numeric seconds — space-separated formats remain unsupported.
The regex update addresses the numeric seconds case from the previous review. However, formats like
"2 days"or"1 hour"(supported by jsonwebtoken via themslibrary internally) will still fall back to the 24h default, causing a mismatch.Consider using the
mspackage for full compatibility if such formats are expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/src/routes/auth.js` around lines 61 - 77, Replace the ad-hoc regex logic in the JWT expiry parsing with the ms package: import/require ms, call ms(expiresInString) and if it returns a number use that value for maxAge (ms returns milliseconds and covers "2 days"/"1 hour"/"24h" etc.); if ms returns undefined but the string is a pure numeric token (expiresInString / matchTime), treat it as seconds and set maxAge = parseInt(...) * 1000; otherwise fall back to the 24h default. Update references in this block (expiresInString, maxAge, matchTime, multipliers) and add ms to dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gsd/ARCHITECTURE.md:
- Line 34: Update the security line that currently reads "HttpOnly cookies; no
client-side JWT access" to avoid implying HttpOnly provides CSRF protection:
state that HttpOnly prevents JavaScript access to cookies but does not mitigate
CSRF, and list proper CSRF mitigations to use instead (e.g., SameSite cookie
attribute, CSRF tokens, and strict Origin/Referer checks). Replace the single
misleading sentence in .gsd/ARCHITECTURE.md (the "HttpOnly cookies; no
client-side JWT access" entry) with this clarified wording.
- Line 44: Update the ARCHITECTURE.md security section to correct where IDOR/job
ownership is validated: remove or relocate the claim that "Rust Warden validates
job_id ownership" and instead state that job ownership checks are performed in
the Node proxy (see searchboost_api/src/routes/search.js handling of GET
/result/:job_id which validates job_id segments), while Warden
(searchboost_warden/src/relay.rs) deterministically builds job_id from
session_id during enqueue; apply the correction to the lines cited (including
the similar statement at line 57).
- Around line 5-27: Fix markdownlint issues: ensure there is a blank line above
and below each heading and code fence (address MD022/MD031) for the System
Diagram and section headings like "React Web Client" and "Node.js REST API"; add
a fence language to the triple-backtick block (change ``` to ```text or
```diagram) to satisfy MD040; remove any trailing spaces (MD009) throughout the
file including around diagram lines; and correct the compound adjective by
changing "High Speed" to "High-Speed" in the Redis table row ("Semantic TTL
Caching + High-Speed Job Queue (Authenticated)").
In @.gsd/ROADMAP.md:
- Line 30: Replace the unverifiable phrase "Resolved 14+ architectural
vulnerabilities" in the roadmap with a neutral or evidence-backed statement:
either reword to something like "Addressed multiple architectural
vulnerabilities" or append a link/reference to a generated audit artifact/CI
report that substantiates the count (e.g., "See audit report: <artifact-URL>").
Update the exact line containing "Resolved 14+ architectural vulnerabilities" so
the roadmap no longer asserts a fixed security-count without a verifiable
source.
- Around line 3-64: The roadmap has markdownlint warnings (MD022: headings
should be surrounded by blank lines) and other style issues around heading
spacing and list structure (e.g., the section headings like "## Phase 1: Fix
Worker Hangs" through "## Phase 9: Knowledge Ingestion & Hybrid RAG (MVP
Hook)"). Fix by adding a single blank line before and after each heading, ensure
checklists and lists are separated from headings by blank lines, remove trailing
spaces, and normalize heading levels and list indentation; then run a
formatter/linter such as markdownlint-cli --fix or prettier --write on
.gsd/ROADMAP.md to auto-apply remaining style fixes.
In `@searchboost_api/src/routes/auth.js`:
- Line 67: The parseInt call that sets const val from matchTime (const val =
parseInt(matchTime[1])) should use an explicit radix to avoid ambiguity; change
the invocation to pass 10 as the second argument (i.e., use
parseInt(matchTime[1], 10)) so the value parsed into val is explicitly base-10.
- Around line 96-98: The logout handler currently calls
res.clearCookie('sb_token') without options, so browsers may not remove the
cookie; update the router.post('/logout', ...) handler to call
res.clearCookie('sb_token', { httpOnly: true, sameSite: 'strict', secure:
process.env.NODE_ENV === 'production' }) (matching the options used when setting
the cookie in the login route) before sending the JSON response so the cookie
will actually be cleared.
---
Duplicate comments:
In `@notes/FlowDiagram.pu`:
- Around line 39-43: Update the flow diagram labels to match the real API/worker
contract: replace the Redis key label "job_result:uuid" (seen on the WKR -> RD
and WRD -> RD arrows) with "sb:result:{job_id}" and change the route parameter
"/api/search/result/:id" to "/api/search/result/:job_id" (replace ":id" with
":job_id") so the UI -> API and proxy steps match the worker key usage; verify
the arrows/labels mentioning WKR, WRD, and RD are updated accordingly and run
the provided read-only shell checks to confirm route and worker usages align.
In `@searchboost_api/src/routes/auth.js`:
- Around line 79-81: The code calls jwt.sign(...) to create token using
process.env.JWT_SECRET but doesn't guard against a missing secret; add a
fail-fast check for process.env.JWT_SECRET at module load (immediately after
imports in auth.js) and throw a clear Error if it's falsy so jwt.sign cannot run
with an undefined secret; reference the JWT_SECRET env var and the jwt.sign
usage that produces the token to locate where to add the check.
- Around line 11-14: Request body credential values are only being checked for
truthiness, which allows non-string types (arrays, objects, numbers) to pass and
later crash bcrypt.hash/bcrypt.compare; update both the register handler (where
const { username, password } = req.body is used) and the login handler (the
similar block around line 41) to first verify typeof username === 'string' &&
typeof password === 'string' (and return 400 with a clear message if not) before
performing length checks or calling bcrypt.hash/bcrypt.compare, ensuring invalid
types are rejected early.
- Around line 61-77: Replace the ad-hoc regex logic in the JWT expiry parsing
with the ms package: import/require ms, call ms(expiresInString) and if it
returns a number use that value for maxAge (ms returns milliseconds and covers
"2 days"/"1 hour"/"24h" etc.); if ms returns undefined but the string is a pure
numeric token (expiresInString / matchTime), treat it as seconds and set maxAge
= parseInt(...) * 1000; otherwise fall back to the 24h default. Update
references in this block (expiresInString, maxAge, matchTime, multipliers) and
add ms to dependencies.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f59c5ff6-1993-4bfb-9d8e-c4acc0fbe8ce
📒 Files selected for processing (5)
.gsd/ARCHITECTURE.md.gsd/ROADMAP.mdnotes/FlowDiagram.pusearchboost_api/src/routes/auth.jssearchboost_warden/src/configurator.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
added 3 commits
March 28, 2026 19:04
- Migrated IDOR ownership checks from the Node.js API to the Rust Warden - Integrated sqlx/PostgreSQL into Warden for independent session authority - Hardened Node.js API with strict runtime type guards and secure session termination - Standardized job_id prefixing (SB-SESSION:user:thread) across all tiers - Fixed Dockerfile security (npm ci, USER 101) and synchronized PlantUML flow diagrams - Resolved 18+ CodeRabbit audit nitpicks across Python, Node, and documentation
- Migrated configs/warden.ini to configs/warden.yml - Exposed PostgreSQL connection metadata to the Warden container - Validated system health: all services UP and 'IDOR Protected' logic active
- Replaced custom regex with 'ms' package for robust cookie maxAge calculation - Added top-level environment validation for JWT_SECRET in auth.js - Updated package.json with ms dependency
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
searchboost_warden/src/relay.rs (1)
118-128:⚠️ Potential issue | 🔴 CriticalCritical: Worker function signature mismatch will cause runtime failure.
The relay passes 3 arguments to the worker (
query,options,job_id), but the worker'srun_taskfunction only accepts 2 parameters (query,args_namespace). This will cause anarqtask invocation failure at runtime.From
searchboost_service/searchboost_src/worker.pyline 57:asyncdefrun_task(self, ctx, query: str, args_namespace):The
job_idis automatically provided by the arq runtime through thectxparameter (see line 75, 82, 96 of worker.py where it's accessed viactx.get('job_id')), so it must not be included as a positional argument in the job data.🐛 Fix: Remove job_id from relay job arguments
let job_data = serde_json::json!({ "t": 1, "f": "Worker.run_task", "a": [ payload.query, - payload.options.unwrap_or_default(),- job_id // Pass job_id for Worker traceability+ payload.options.unwrap_or_default() ], "k": {}, "et": enqueue_time_ms });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_warden/src/relay.rs` around lines 118 - 128, The relay is passing three positional args to the worker (payload.query, payload.options.unwrap_or_default(), job_id) but Worker.run_task expects only (ctx, query, args_namespace) and obtains job_id from ctx; remove the job_id from the job_data "a" array so the "a" list contains only payload.query and payload.options.unwrap_or_default(), leaving "k" and "et" as-is; update the creation of job_data in relay.rs (the json! block building "a") to stop including job_id to match Worker.run_task's signature.
♻️ Duplicate comments (3)
.gsd/ARCHITECTURE.md (1)
5-10:⚠️ Potential issue | 🟡 MinorFix recurring markdownlint and wording issues in one pass.
This section still has the previously flagged formatting issues (blank lines around headings/fences, missing fence language, trailing spaces) and the compound adjective at Line 64 should be “High-Speed”.
Suggested cleanup diff
## Overview + SearchBoost is a decentralized, highly-resilient hybrid-AI search engine pipeline. It is architected as an asynchronous distributed system isolating user authorization (Node.js), high-throughput boundary ingress/caching (Rust), intensive LLM background execution (Python), and frontend client rendering (React). The system is **Secure-by-Default**, enforcing fail-closed configuration and unprivileged container execution. ## System Diagram -```++```text [ User Browser (React Vite) ] @@ [ Ollama ] [ SearXNG ]@@
-### React Web Client
+### React Web Client
@@
-### Node.js REST API
+### Node.js REST API
@@
-| Redis | Memory DB | Semantic TTL Caching + High Speed Job Queue (Authenticated) |
+| Redis | Memory DB | Semantic TTL Caching + High-Speed Job Queue (Authenticated) |</details> Also applies to: 31-31, 36-36, 59-60, 64-64 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.gsd/ARCHITECTURE.md around lines 5 - 10, Fix markdownlint and wording
issues: add a language to the code fence (use ```text), ensure a blank line
before and after fenced blocks and headings (e.g., around the System Diagram and
headings like "React Web Client" and "Node.js REST API"), remove trailing
spaces, and update the compound adjective in the table cell from "High Speed Job
Queue" to "High-Speed Job Queue"; search for the headings and the table row in
the file (symbols: "System Diagram", "React Web Client", "Node.js REST API", and
the table row containing "Redis | Memory DB | Semantic TTL Caching") and make
these changes consistently wherever duplicated (lines referenced in the
comment).</details> </blockquote></details> <details> <summary>.gsd/ROADMAP.md (2)</summary><blockquote> `30-30`: _⚠️ Potential issue_ | _🟡 Minor_ **Replace fixed vulnerability count with evidence-backed wording.** “Resolved 14+ architectural vulnerabilities” is likely to go stale unless tied to a live audit artifact. Prefer neutral phrasing or add an audit/report link. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.gsd/ROADMAP.md at line 30, Replace the hardcoded claim "Resolved 14+ architectural vulnerabilities" with neutral, evidence-backed wording: either rephrase to something like "Addressed multiple architectural vulnerabilities" or append a reference to an audit/report (e.g., "per [audit/report name or link]"). Update the sentence that also mentions "Implemented non-root isolation and fail-closed secret management" to remain factual and, if applicable, add a parenthetical reference to the supporting audit/artifact; ensure you remove the numeric count unless you provide a verifiable link. ``` </details> --- `1-1`: _⚠️ Potential issue_ | _🟡 Minor_ **Run markdownlint cleanup for heading/list spacing and trailing spaces.** The file still has recurring MD022/MD009 warnings in this range. A single formatting pass will keep docs CI clean and reduce noisy future diffs. Also applies to: 4-4, 14-14, 26-26 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.gsd/ROADMAP.md at line 1, Run a markdownlint cleanup to fix MD022/MD009 warnings: normalize heading and list spacing and remove trailing spaces in the document (e.g., ensure the "SearchBoost Roadmap" heading has a blank line after it, list items have a single space after the marker, and no lines end with trailing whitespace); you can run your project's markdownlint autofix or apply a single-pass formatter to the file and commit the cleaned file so CI no longer reports MD022/MD009 for the affected sections. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gsd/ARCHITECTURE.md:
- Line 49: The ARCHITECTURE statement overstates PIIDetector as an enforcement
gate; update the sentence to reflect that PIIDetector (the PIIDetector class in
searchboost_service/searchboost_src/pii_detector.py) performs a triple-pass
advisory/flag-based scan that flags PII risk and informs downstream
handling/cache-safety decisions rather than hard-blocking requests, and keep the
note about the recursive deep-merge configuration precedence (CLI > ENV > YAML)
intact.- Around line 54-55: Update the architecture doc to reflect actual ownership:
change the wording so the Warden (relay module) constructs both session_id and
job_id (the implementation in the Warden code—see relay module where
session_id/job_id are generated) instead of Node constructing session_id;
reference the symbols session_id and job_id and state that Node sends the
request/metadata to Warden, which then generates session_id and appends a UUID
to produce job_id and enqueues the task.In
@docker-compose.yml:
- Around line 19-23: The docker-compose environment variables use DB_* names
that are ignored by the Warden configurator which expects
WARDEN__ style names (the DB struct is named database in
configurator.rs), so change the env keys to WARDEN__DATABASE__USER,
WARDEN__DATABASE__PASSWORD, WARDEN__DATABASE__NAME, WARDEN__DATABASE__HOST, and
WARDEN__DATABASE__PORT to match the WARDEN prefix and nested field convention
used by the configurator (note DB_PASSWORD previously worked only because there
is an explicit override handler).In
@MANUAL_TESTPLAN.md:
- Around line 23-24: Update the MANUAL_TESTPLAN.md verification text to require
an explicit count of zero matching keys rather than the vague "MUST NOT contain
any keys corresponding to the PII-laden query": instruct testers to run
redis-cli KEYS "semantic_cache:" and assert the returned set length is zero for
the specific test run, or if concurrent tests may create noise, show how to
derive the exact key pattern for the PII query (e.g., include the query’s
deterministic identifier or hash used by semantic cache) and use that pattern
(redis-cli KEYS "semantic_cache:<hash_or_id>") or grep the KEYS output to
confirm zero matches for that identifier.- Around line 8-11: Add an explicit PostgreSQL verification step that directs
the tester to connect to the database and query the table that stores chat
threads (look for the thread_id column) to confirm the two sessions created by
"Search 'test query 1'" and "Search 'test query 2' after '+ New Chat'" produce
distinct thread_id records; mention the table name or service used for
persistence and instruct verifying the two thread_id values are different and
tied to the expected session timestamps or user identifiers.- Around line 30-34: Add an explicit verification step after "Enqueue Request B"
to assert a cache hit occurred: query Redis for the semantic_cache:* key(s)
created by Request A (e.g., using redis-cli GET/EXISTS on the key generated for
the optimized query) and/or check the application log for the CACHE HIT or for
absence of a SearXNG request; update MANUAL_TESTPLAN.md to include this
single-line verification (check Redis key exists and confirm no SearXNG call was
made) immediately after step 4.In
@README.md:
- Line 12: Replace the word "Contemporary" with "Modern" in the README bullet
describing the Web Dashboard (the line starting "* Web Dashboard (React):
Contemporary interactive interface for concurrent thread management.") so the
phrase reads "Modern interactive interface for concurrent thread management";
update only that word to improve clarity and conventional wording.In
@searchboost_api/src/routes/auth.js:
- Around line 40-47: Reorder the input validation in the auth route so type
checks run before emptiness checks: first verify req.body.username and
req.body.password are strings (the typeof checks for username and password),
then validate they are non-empty/non-falsy; update the validation logic around
the destructured const { username, password } in the login handler to perform
the typeof checks first and return the 400 error for invalid input types before
returning the 400 error for missing values.In
@searchboost_api/src/routes/search.js:
- Around line 13-20: Validate and sanitize thread_id before using it in
session/job IDs: ensure req.body.thread_id is a string matching an allowed
pattern (e.g., alphanumeric, hyphen/underscore) and does not contain ':' (or
other reserved chars); if it fails validation or is missing, fall back to
'default'. Update the code that builds
SB-SESSION:${req.user.username}:${thread_id} and the axios.post payload (where
thread_id is set) to use the validated/sanitized value (referencing the
thread_id variable and the axios.post call), and coerce non-string inputs to
strings before validation so non-string request bodies cannot be forwarded
unchanged to Warden.- Around line 23-28: The catch blocks in search.js currently send upstream
transport errors back to clients by using error.response.data or error.message;
instead, always return the generic 503 JSON ({ error: 'Could not reach Warden'
}) to clients and log the full upstream error server-side; update both catch
blocks (the ones inspecting error.response and the other branch) to call
res.status(503).json({ error: 'Could not reach Warden' }) and send the detailed
error to your server logger (e.g., console.error or processLogger.error)
including error and error.response for diagnostics, rather than echoing those
details in the HTTP response.In
@searchboost_ui/Dockerfile:
- Around line 1-28: Add a HEALTHCHECK instruction to the Dockerfile (before CMD)
that probes the served SPA via HTTP on port 8080 so orchestrators can detect
unhealthy containers; implement a simple command such as using curl or wget to
GET the root (e.g., curl -f http://localhost:8080/ || exit 1) with sensible
interval, timeout and retries, ensuring it runs in the final image that uses
nginxinc/nginx-unprivileged and respects the unprivileged USER 101.Outside diff comments:
In@searchboost_warden/src/relay.rs:
- Around line 118-128: The relay is passing three positional args to the worker
(payload.query, payload.options.unwrap_or_default(), job_id) but Worker.run_task
expects only (ctx, query, args_namespace) and obtains job_id from ctx; remove
the job_id from the job_data "a" array so the "a" list contains only
payload.query and payload.options.unwrap_or_default(), leaving "k" and "et"
as-is; update the creation of job_data in relay.rs (the json! block building
"a") to stop including job_id to match Worker.run_task's signature.Duplicate comments:
In @.gsd/ARCHITECTURE.md:
- Around line 5-10: Fix markdownlint and wording issues: add a language to the
code fence (use ```text), ensure a blank line before and after fenced blocks and
headings (e.g., around the System Diagram and headings like "React Web Client"
and "Node.js REST API"), remove trailing spaces, and update the compound
adjective in the table cell from "High Speed Job Queue" to "High-Speed Job
Queue"; search for the headings and the table row in the file (symbols: "System
Diagram", "React Web Client", "Node.js REST API", and the table row containing
"Redis | Memory DB | Semantic TTL Caching") and make these changes consistently
wherever duplicated (lines referenced in the comment).In @.gsd/ROADMAP.md:
- Line 30: Replace the hardcoded claim "Resolved 14+ architectural
vulnerabilities" with neutral, evidence-backed wording: either rephrase to
something like "Addressed multiple architectural vulnerabilities" or append a
reference to an audit/report (e.g., "per [audit/report name or link]"). Update
the sentence that also mentions "Implemented non-root isolation and fail-closed
secret management" to remain factual and, if applicable, add a parenthetical
reference to the supporting audit/artifact; ensure you remove the numeric count
unless you provide a verifiable link.- Line 1: Run a markdownlint cleanup to fix MD022/MD009 warnings: normalize
heading and list spacing and remove trailing spaces in the document (e.g.,
ensure the "SearchBoost Roadmap" heading has a blank line after it, list items
have a single space after the marker, and no lines end with trailing
whitespace); you can run your project's markdownlint autofix or apply a
single-pass formatter to the file and commit the cleaned file so CI no longer
reports MD022/MD009 for the affected sections.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: ASSERTIVE **Plan**: Pro **Run ID**: `6908b9e1-cbe6-479c-a5a3-35f17aebdb3a` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between fa19c7068f98094b70d294e2657d554bbe7cd1c1 and f85f79fcbd3dab472b1d919655284a77aea6dc8f. </details> <details> <summary>📒 Files selected for processing (16)</summary> * `.gsd/ARCHITECTURE.md` * `.gsd/ROADMAP.md` * `MANUAL_TESTPLAN.md` * `README.md` * `configs/warden.ini` * `configs/warden.yml` * `docker-compose.yml` * `notes/FlowDiagram.pu` * `searchboost_api/src/routes/auth.js` * `searchboost_api/src/routes/search.js` * `searchboost_service/searchboost_src/configurator.py` * `searchboost_ui/Dockerfile` * `searchboost_warden/Cargo.toml` * `searchboost_warden/src/configurator.rs` * `searchboost_warden/src/main.rs` * `searchboost_warden/src/relay.rs` </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * configs/warden.ini </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| ### Python ARQ Worker | ||
| - **Purpose:** Executes heavy I/O loops against Ollama for semantic reduction. | ||
| - **Location:** `searchboost_service/` | ||
| - **Security:** PII-Gate implemented via `PIIDetector` (triple-pass scan). Recursive deep-merge configuration logic with `CLI > ENV > YAML` precedence. |
There was a problem hiding this comment.
Avoid overstating PIIDetector as an enforcement gate.
The statement implies hard enforcement, but PIIDetector behavior (in searchboost_service/searchboost_src/pii_detector.py, Lines 89-132) is advisory/flag-based for cache safety. Rephrase to indicate it flags PII-risk and informs downstream handling, unless an explicit hard-block path is documented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gsd/ARCHITECTURE.md at line 49, The ARCHITECTURE statement overstates
PIIDetector as an enforcement gate; update the sentence to reflect that
PIIDetector (the PIIDetector class in
searchboost_service/searchboost_src/pii_detector.py) performs a triple-pass
advisory/flag-based scan that flags PII risk and informs downstream
handling/cache-safety decisions rather than hard-blocking requests, and keep the
note about the recursive deep-merge configuration precedence (CLI > ENV > YAML)
intact.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+19
to
+23
| - DB_USER=${DB_USER:-searchboost} | ||
| - DB_PASSWORD=${DB_PASSWORD:-searchboost_pass} | ||
| - DB_NAME=${DB_NAME:-searchboost_db} | ||
| - DB_HOST=db | ||
| - DB_PORT=5432 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -e
echo"=== docker-compose Warden env ==="
sed -n '15,23p' docker-compose.yml
echoecho"=== Warden environment loader ==="
sed -n '85,116p' searchboost_warden/src/configurator.rs
echoecho"=== Warden DB field names ==="
sed -n '54,74p' searchboost_warden/src/configurator.rsRepository: Somnerd/SearchBoost
Length of output: 2538
Fix DB environment variable names for Warden configuration loading.
searchboost_warden/src/configurator.rs, Lines 85-116 loads environment overrides via WARDEN__ prefix pattern, and the DB struct field name is database (line 62). The current docker-compose.yml lines 19-23 set DB_USER, DB_NAME, DB_HOST, and DB_PORT without the WARDEN__ prefix, so these settings are silently ignored. Only DB_PASSWORD works because it has an explicit override handler (lines 107-111).
Suggested fix
- - DB_USER=${DB_USER:-searchboost}+ - WARDEN__DB__USER=${DB_USER:-searchboost}
- DB_PASSWORD=${DB_PASSWORD:-searchboost_pass}
- - DB_NAME=${DB_NAME:-searchboost_db}- - DB_HOST=db- - DB_PORT=5432+ - WARDEN__DB__DATABASE=${DB_NAME:-searchboost_db}+ - WARDEN__DB__HOST=db+ - WARDEN__DB__PORT=5432📝 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
| - DB_USER=${DB_USER:-searchboost} | |
| - DB_PASSWORD=${DB_PASSWORD:-searchboost_pass} | |
| - DB_NAME=${DB_NAME:-searchboost_db} | |
| - DB_HOST=db | |
| - DB_PORT=5432 | |
| - WARDEN__DB__USER=${DB_USER:-searchboost} | |
| - DB_PASSWORD=${DB_PASSWORD:-searchboost_pass} | |
| - WARDEN__DB__DATABASE=${DB_NAME:-searchboost_db} | |
| - WARDEN__DB__HOST=db | |
| - WARDEN__DB__PORT=5432 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yml` around lines 19 - 23, The docker-compose environment
variables use DB_* names that are ignored by the Warden configurator which
expects WARDEN__<struct>__<field> style names (the DB struct is named database
in configurator.rs), so change the env keys to WARDEN__DATABASE__USER,
WARDEN__DATABASE__PASSWORD, WARDEN__DATABASE__NAME, WARDEN__DATABASE__HOST, and
WARDEN__DATABASE__PORT to match the WARDEN__ prefix and nested field convention
used by the configurator (note DB_PASSWORD previously worked only because there
is an explicit override handler).
Comment on lines
+8
to
+11
| 1. Login to the application. | ||
| 2. Search "test query 1". Verify sidebar parses session ID. | ||
| 3. Click '+ New Chat'. | ||
| 4. Search "test query 2". Verify second session appears independent of the first main view. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding explicit PostgreSQL verification step.
The objective mentions confirming "distinct thread_id records in PostgreSQL," but the steps don't include direct database verification. Adding a step to query PostgreSQL would provide stronger evidence of isolation.
📋 Suggested improvement
2. Search "test query 1". Verify sidebar parses session ID.
3. Click '+ New Chat'.
4. Search "test query 2". Verify second session appears independent of the first main view.
+5. Query PostgreSQL to confirm two distinct `thread_id` records exist: + `SELECT thread_id, created_at FROM sessions ORDER BY created_at DESC LIMIT 2;`📝 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
| 1. Login to the application. | |
| 2. Search "test query 1". Verify sidebar parses session ID. | |
| 3. Click '+ New Chat'. | |
| 4. Search "test query 2". Verify second session appears independent of the first main view. | |
| 1. Login to the application. | |
| 2. Search "test query 1". Verify sidebar parses session ID. | |
| 3. Click '+ New Chat'. | |
| 4. Search "test query 2". Verify second session appears independent of the first main view. | |
| 5. Query PostgreSQL to confirm two distinct `thread_id` records exist: | |
| `SELECT thread_id, created_at FROM sessions ORDER BY created_at DESC LIMIT 2;` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MANUAL_TESTPLAN.md` around lines 8 - 11, Add an explicit PostgreSQL
verification step that directs the tester to connect to the database and query
the table that stores chat threads (look for the thread_id column) to confirm
the two sessions created by "Search 'test query 1'" and "Search 'test query 2'
after '+ New Chat'" produce distinct thread_id records; mention the table name
or service used for persistence and instruct verifying the two thread_id values
are different and tied to the expected session timestamps or user identifiers.
Uh oh!
There was an error while loading. Please reload this page.
| * **The Worker (Python/Arq):** Asynchronous task processor that handles LLM embeddings and web engine scaling. | ||
| * **Infrastructure (Redis/Postgres/SearxNG):** The distributed storage and search engine backend. | ||
| * **PostgreSQL Persistence**: Automatic long-term storage of LLM responses and system indexing metadata. | ||
| * **Web Dashboard (React):** Contemporary interactive interface for concurrent thread management. |
There was a problem hiding this comment.
Minor: Consider replacing "Contemporary" with "Modern".
While grammatically correct, "contemporary" is an uncommon word choice in technical documentation. "Modern" is the more conventional term for describing up-to-date UI frameworks.
📝 Suggested wording improvement
-* **Web Dashboard (React):** Contemporary interactive interface for concurrent thread management.+* **Web Dashboard (React):** Modern interactive interface for concurrent thread management.📝 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
| ***Web Dashboard (React):**Contemporary interactive interface for concurrent thread management. | |
| ***Web Dashboard (React):**Modern interactive interface for concurrent thread management. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 12, Replace the word "Contemporary" with "Modern" in the
README bullet describing the Web Dashboard (the line starting "* **Web
Dashboard (React):** Contemporary interactive interface for concurrent thread
management.") so the phrase reads "Modern interactive interface for concurrent
thread management"; update only that word to improve clarity and conventional
wording.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
searchboost_warden/src/relay.rs (1)
118-128:⚠️ Potential issue | 🔴 CriticalRemove
job_idfrom the worker invocation arguments.The Python worker's
run_taskfunction only accepts two parameters:queryandargs_namespace. The Rust code attempts to passjob_idas a third argument, which will cause a parameter mismatch at runtime. The worker already has access tojob_idviactx.get('job_id')through the arq framework, as shown in the existing code (e.g., lines storing results withctx.get('job_id')). Removejob_idfrom the"a"array and let the framework provide it to the context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_warden/src/relay.rs` around lines 118 - 128, The payload sent to the worker includes job_id as a third argument which causes a parameter mismatch: remove job_id from the "a" array in the job_data construction so the arguments become payload.query and payload.options.unwrap_or_default() only, leaving Worker.run_task to receive the two expected params; the worker can still access job_id via ctx.get('job_id') from the arq framework.
♻️ Duplicate comments (2)
.gsd/ARCHITECTURE.md (1)
5-10:⚠️ Potential issue | 🟡 MinorResolve remaining markdownlint/style regressions in one pass.
Several lints are still present (blank lines around headings/fences/tables, missing fence language, trailing spaces), and Line 64 reintroduces “High Speed” instead of “High-Speed”.
Suggested doc cleanup
## Overview + SearchBoost is a decentralized, highly-resilient hybrid-AI search engine pipeline. It is architected as an asynchronous distributed system isolating user authorization (Node.js), high-throughput boundary ingress/caching (Rust), intensive LLM background execution (Python), and frontend client rendering (React). The system is **Secure-by-Default**, enforcing fail-closed configuration and unprivileged container execution. ## System Diagram -```++```text [ User Browser (React Vite) ] @@ [ Ollama ] [ SearXNG ]@@
-### React Web Client
+### React Web Client
@@
-### Node.js REST API
+### Node.js REST API
@@Integration Points
External Service Type Purpose @@ - Redis Memory DB + Redis Memory DB </details> Also applies to: 31-31, 36-36, 59-60, 64-64 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.gsd/ARCHITECTURE.md around lines 5 - 10, Fix remaining markdownlint/style
issues in ARCHITECTURE.md: ensure a blank line before and after all headings and
fenced code blocks, add a fence language identifier (use "text") for the
triple-backtick block containing the system diagram, remove trailing spaces
throughout, normalize header labels to remove extra spaces (change "React Web
Client " -> "React Web Client" and "Node.js REST API " -> "Node.js REST API"),
and change the table cell text "High Speed" to "High-Speed" in the Integration
Points table and update any other occurrences noted (lines around the diagram,
headers, and table).</details> </blockquote></details> <details> <summary>searchboost_api/src/routes/auth.js (1)</summary><blockquote> `17-28`: _⚠️ Potential issue_ | _🟡 Minor_ **Missing type validation in register endpoint.** The login endpoint validates `typeof username !== 'string'` at line 51, but the register endpoint lacks this check. Non-string payloads can cause `bcrypt.hash` to throw or behave unexpectedly. <details> <summary>🔧 Proposed fix</summary> ```diff router.post('/register', async (req, res, next) => { try { const { username, password } = req.body; if (!username || !password) { return res.status(400).json({ error: 'Username and password are required' }); } + + if (typeof username !== 'string' || typeof password !== 'string') { + return res.status(400).json({ error: 'Invalid input types' }); + } // Basic validation if (username.length < 3 || username.length > 32 || !/^[a-zA-Z0-9_]+$/.test(username)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_api/src/routes/auth.js` around lines 17 - 28, The register endpoint currently validates username length/format and password length but misses explicit type checks, which can cause bcrypt.hash to throw on non-string inputs; in the register handler (the function that reads const { username, password } = req.body) add guards that verify typeof username === 'string' and typeof password === 'string' before performing regex/length checks, and return res.status(400).json(...) with a clear error when types are invalid so bcrypt.hash is only called with strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gsd/ARCHITECTURE.md:
- Around line 54-55: Update the architecture doc to reflect the actual
data-flow: Node (search.js) sends username and thread_id to the Warden rather
than constructing session_id, and the Warden (relay.rs) is responsible for
creating the session_id (format SB-SESSION:${username}:${thread_id}) and
deriving job_id by appending a UUID before enqueuing to Redis; modify the
wording of the two steps so they state Node dispatches username/thread_id and
Warden constructs session_id and job_id.
- Line 34: The sentence "**Security:** XSS protection via HttpOnly cookies
(hides JWT from scripts); CSRF mitigation via `Strict` SameSite policy."
overstates HttpOnly as XSS protection—update that phrase so "HttpOnly" is
described as preventing JavaScript access/token exfiltration (not stopping
script execution), and clarify that XSS must be mitigated by other controls
(e.g., input/output sanitization, CSP, escaping), while keeping the note that
`SameSite=Strict` helps CSRF mitigation; modify the wording around the
"HttpOnly" and "SameSite" mentions to reflect these distinctions.
In @.gsd/ROADMAP.md:
- Line 51: Phase 8.2's note about porting PIIDetector to Rust lacks
API-preservation requirements; update the roadmap entry to require that the Rust
PIIDetector preserves the Python public interface and behaviors by documenting
that the Rust type named PIIDetector must expose a scan(text: str) ->
PIIDetectionResult-equivalent function signature, gracefully handle
null/non-string inputs the same way the Python implementation does, accept an
optional logger and emit equivalent warning messages, and maintain the same
exact pattern-matching semantics so existing consumers relying on the current
behavior do not break.
- Line 41: Replace the vague word "Terminal" with "CLI" (or "Command-line
Interface") in Phase 7.4 to be explicit about the command-line scope; update the
line "- [ ] 7.4 **Dynamic LLM Selection**: Allow UI/Terminal overrides for
Ollama model names" to mention "CLI" and, if desirable, note integration points
such as the existing argparser.py and its parse_args/ArgumentParser usage to
clarify this phase will extend the current CLI model-selection flags rather than
a generic terminal UI.
In `@docker-compose.yml`:
- Line 18: The REDIS_PASSWORD variable is inconsistent: the Redis service uses a
default of searchboost_pass while Warden and Worker pass ${REDIS_PASSWORD} with
no default, causing auth failures if .env is missing; fix by making the behavior
consistent — either require REDIS_PASSWORD (fail-closed) by removing any Redis
default and ensuring Warden and Worker use ${REDIS_PASSWORD} (no fallback), or
add the same default to Warden and Worker so all three use
${REDIS_PASSWORD:-searchboost_pass}; locate the REDIS_PASSWORD occurrences in
the docker-compose.yml (the Warden service entry, the Worker service entry, and
the Redis service entry) and update them accordingly.
In `@searchboost_api/package.json`:
- Around line 5-7: The package.json has an inconsistent entry point: "main" is
"index.js" while the "start" script runs "node src/app.js"; update the
package.json so the "main" field matches the actual runtime entry (e.g., set
"main" to "src/app.js") or change the "start" script to use "index.js" so both
agree; ensure the referenced file (src/app.js or index.js) actually exists and
exports the intended module so imports use the correct entry point.
In `@searchboost_ui/Dockerfile`:
- Around line 1-28: Add a HEALTHCHECK instruction to the Dockerfile to let
orchestrators detect an unresponsive container: add a HEALTHCHECK after the COPY
and EXPOSE/USER steps that probes the running nginx on port 8080 (the same port
exposed by EXPOSE and used by CMD/nginx) by performing an HTTP GET against / (or
a lightweight health endpoint configured in nginx.conf) and returning non-zero
on failure; set sensible options such as interval, timeout, start-period and
retries so the probe waits for startup and retries transient failures.
In `@searchboost_warden/Cargo.toml`:
- Line 33: The sqlx dependency in Cargo.toml currently enables runtime-tokio,
postgres, and macros but omits TLS; update the sqlx feature list to include
either tls-rustls or tls-native-tls (e.g., add "tls-rustls") so production
PostgreSQL connections are encrypted. Locate the sqlx entry (sqlx = { version =
"0.8", features = [ "runtime-tokio", "postgres", "macros" ] }) and add the
desired TLS feature to the features array, then run cargo build and, if using
environment-based TLS settings, verify DATABASE_URL and sqlx runtime
configuration in your deployment.
In `@searchboost_warden/src/configurator.rs`:
- Around line 104-114: The docker-compose DB_* env vars are ignored; add
explicit overrides in the configurator to mirror the DB_PASSWORD handling: read
DB_USER, DB_HOST, DB_PORT, and DB_NAME via env::var, check for Ok and non-empty,
assign them into settings.db.user, settings.db.host, settings.db.name, and parse
DB_PORT into the numeric type used by settings.db.port (handle parse errors
gracefully or skip if invalid). Use the same pattern as the existing
REDIS_PASSWORD/DB_PASSWORD block so the unique symbol settings.db (and its
fields user, host, port, name) is updated when those env vars are present.
---
Outside diff comments:
In `@searchboost_warden/src/relay.rs`:
- Around line 118-128: The payload sent to the worker includes job_id as a third
argument which causes a parameter mismatch: remove job_id from the "a" array in
the job_data construction so the arguments become payload.query and
payload.options.unwrap_or_default() only, leaving Worker.run_task to receive the
two expected params; the worker can still access job_id via ctx.get('job_id')
from the arq framework.
---
Duplicate comments:
In @.gsd/ARCHITECTURE.md:
- Around line 5-10: Fix remaining markdownlint/style issues in ARCHITECTURE.md:
ensure a blank line before and after all headings and fenced code blocks, add a
fence language identifier (use "text") for the triple-backtick block containing
the system diagram, remove trailing spaces throughout, normalize header labels
to remove extra spaces (change "React Web Client " -> "React Web Client" and
"Node.js REST API " -> "Node.js REST API"), and change the table cell text "High
Speed" to "High-Speed" in the Integration Points table and update any other
occurrences noted (lines around the diagram, headers, and table).
In `@searchboost_api/src/routes/auth.js`:
- Around line 17-28: The register endpoint currently validates username
length/format and password length but misses explicit type checks, which can
cause bcrypt.hash to throw on non-string inputs; in the register handler (the
function that reads const { username, password } = req.body) add guards that
verify typeof username === 'string' and typeof password === 'string' before
performing regex/length checks, and return res.status(400).json(...) with a
clear error when types are invalid so bcrypt.hash is only called with strings.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66878223-6bb3-46bd-ae18-72b530808b65
📒 Files selected for processing (17)
.gsd/ARCHITECTURE.md.gsd/ROADMAP.mdMANUAL_TESTPLAN.mdREADME.mdconfigs/warden.iniconfigs/warden.ymldocker-compose.ymlnotes/FlowDiagram.pusearchboost_api/package.jsonsearchboost_api/src/routes/auth.jssearchboost_api/src/routes/search.jssearchboost_service/searchboost_src/configurator.pysearchboost_ui/Dockerfilesearchboost_warden/Cargo.tomlsearchboost_warden/src/configurator.rssearchboost_warden/src/main.rssearchboost_warden/src/relay.rs
💤 Files with no reviewable changes (1)
- configs/warden.ini
| ### React Web Client | ||
| - **Purpose:** Interactive GUI for concurrent search threads and authentications. | ||
| - **Location:** `searchboost_ui/` | ||
| - **Security:** XSS protection via HttpOnly cookies (hides JWT from scripts); CSRF mitigation via `Strict` SameSite policy. |
There was a problem hiding this comment.
Correct Line 34: HttpOnly is not XSS protection.
Line 34 currently overstates HttpOnly. It prevents JavaScript cookie reads (token exfiltration mitigation), but it does not stop XSS execution.
Suggested wording
-- **Security:** XSS protection via HttpOnly cookies (hides JWT from scripts); CSRF mitigation via `Strict` SameSite policy.+- **Security:** JWT is stored in HttpOnly cookies to reduce token exfiltration via script access; CSRF is mitigated with `Strict` SameSite policy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gsd/ARCHITECTURE.md at line 34, The sentence "**Security:** XSS protection
via HttpOnly cookies (hides JWT from scripts); CSRF mitigation via `Strict`
SameSite policy." overstates HttpOnly as XSS protection—update that phrase so
"HttpOnly" is described as preventing JavaScript access/token exfiltration (not
stopping script execution), and clarify that XSS must be mitigated by other
controls (e.g., input/output sanitization, CSP, escaping), while keeping the
note that `SameSite=Strict` helps CSRF mitigation; modify the wording around the
"HttpOnly" and "SameSite" mentions to reflect these distinctions.
Uh oh!
There was an error while loading. Please reload this page.
| - [ ] 7.1 **Production Proxy**: Deploy Nginx Reverse Proxy with SSL/ACME. | ||
| - [ ] 7.2 **pgvector Migration**: Initialize `pgvector` in PostgreSQL for vector storage. | ||
| - [ ] 7.3 **Worker Scaling**: Horizontal scaling with Redis task locality. | ||
| - [ ] 7.4 **Dynamic LLM Selection**: Allow UI/Terminal overrides for Ollama model names (e.g., `mistral`, `llama3.2`). |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Description: Search for terminal/CLI interface references in the codebase# Search for CLI/terminal related code
rg -i 'terminal|cli|command.?line' --type md --type js --type py --type rust -C2
# Search for argument parsing libraries that might indicate CLI
rg -i 'argparse|clap|commander|yargs' -g 'package.json' -g 'Cargo.toml' -g 'requirements.txt'Repository: Somnerd/SearchBoost
Length of output: 33703
Use clearer terminology: "CLI" instead of "Terminal" in Phase 7.4.
Phase 7.4 mentions "UI/Terminal overrides" for Ollama model selection. The codebase has a command-line interface (argparser.py) with argument parsing and CLI override support already in place. However, "Terminal" is vague in this context. Consider replacing "Terminal" with "CLI" or "Command-line Interface" to make the scope of Phase 7.4 explicit—whether it extends the existing CLI model selection, adds new CLI arguments, or creates a terminal-based UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gsd/ROADMAP.md at line 41, Replace the vague word "Terminal" with "CLI" (or
"Command-line Interface") in Phase 7.4 to be explicit about the command-line
scope; update the line "- [ ] 7.4 **Dynamic LLM Selection**: Allow UI/Terminal
overrides for Ollama model names" to mention "CLI" and, if desirable, note
integration points such as the existing argparser.py and its
parse_args/ArgumentParser usage to clarify this phase will extend the current
CLI model-selection flags rather than a generic terminal UI.
| **Plans**: | ||
| - [ ] 8.1 **gRPC Handshake**: Replace Pickle with Protobuf for Rust ↔ Python ↔ Node safety. | ||
| - [ ] 8.2 **Rust Logic Migration**: Port `PIIDetector` and "Fast-Path" (cache) logic into the Warden. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Specify API preservation requirements for PIIDetector port.
Phase 8.2 plans to port PIIDetector from Python to Rust, but doesn't document which interface behaviors must be preserved. The Python implementation (see searchboost_service/searchboost_src/pii_detector.py:89-132) has specific characteristics:
scan(text: str) -> PIIDetectionResultsignature- Graceful handling of null/non-string inputs
- Optional logger integration with specific warning messages
- Exact pattern matching behavior
Consider adding a sub-item documenting API compatibility requirements to ensure the Rust port maintains parity with Python consumers expecting the current interface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gsd/ROADMAP.md at line 51, Phase 8.2's note about porting PIIDetector to
Rust lacks API-preservation requirements; update the roadmap entry to require
that the Rust PIIDetector preserves the Python public interface and behaviors by
documenting that the Rust type named PIIDetector must expose a scan(text: str)
-> PIIDetectionResult-equivalent function signature, gracefully handle
null/non-string inputs the same way the Python implementation does, accept an
optional logger and emit equivalent warning messages, and maintain the same
exact pattern-matching semantics so existing consumers relying on the current
behavior do not break.
| - WARDEN_CONFIG_PATH=/configs/warden.ini | ||
| - MASTER_CONFIG_PATH=/configs/master_settings.yml | ||
| - WARDEN_CONFIG_PATH=/configs/warden.yml | ||
| - REDIS_PASSWORD=${REDIS_PASSWORD} |
There was a problem hiding this comment.
REDIS_PASSWORD defaults are inconsistent across services.
The Redis server (line 57) falls back to searchboost_pass if REDIS_PASSWORD is unset, but the Warden (line 18) and Worker (line 40) services pass the variable without a default. If .env is missing or REDIS_PASSWORD is unset, the Redis server will require searchboost_pass while clients receive an empty password, causing authentication failures.
🔧 Option 1 (Recommended): Fail-closed by requiring REDIS_PASSWORD
- command: redis-server --requirepass ${REDIS_PASSWORD:-searchboost_pass} --appendonly yes+ command: redis-server --requirepass ${REDIS_PASSWORD:?REDIS_PASSWORD must be set} --appendonly yes🔧 Option 2: Add consistent defaults to all services
- - REDIS_PASSWORD=${REDIS_PASSWORD}+ - REDIS_PASSWORD=${REDIS_PASSWORD:-searchboost_pass}Apply to both Warden (line 18) and Worker (line 40).
Also applies to: 40-40, 57-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yml` at line 18, The REDIS_PASSWORD variable is inconsistent:
the Redis service uses a default of searchboost_pass while Warden and Worker
pass ${REDIS_PASSWORD} with no default, causing auth failures if .env is
missing; fix by making the behavior consistent — either require REDIS_PASSWORD
(fail-closed) by removing any Redis default and ensuring Warden and Worker use
${REDIS_PASSWORD} (no fallback), or add the same default to Warden and Worker so
all three use ${REDIS_PASSWORD:-searchboost_pass}; locate the REDIS_PASSWORD
occurrences in the docker-compose.yml (the Warden service entry, the Worker
service entry, and the Redis service entry) and update them accordingly.
Comment on lines
+5
to
+7
| "main": "index.js", | ||
| "scripts": { | ||
| "start": "node src/app.js", |
There was a problem hiding this comment.
Inconsistent main entry point.
main is set to index.js but the start script runs node src/app.js. If this package is imported elsewhere, the wrong entry point will be used.
🔧 Proposed fix
- "main": "index.js",+ "main": "src/app.js",📝 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
| "main": "index.js", | |
| "scripts": { | |
| "start": "node src/app.js", | |
| "main": "src/app.js", | |
| "scripts": { | |
| "start": "node src/app.js", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@searchboost_api/package.json` around lines 5 - 7, The package.json has an
inconsistent entry point: "main" is "index.js" while the "start" script runs
"node src/app.js"; update the package.json so the "main" field matches the
actual runtime entry (e.g., set "main" to "src/app.js") or change the "start"
script to use "index.js" so both agree; ensure the referenced file (src/app.js
or index.js) actually exists and exports the intended module so imports use the
correct entry point.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
added 3 commits
March 28, 2026 20:07
- Enforced error masking in search routes to return generic 503 instead of raw Warden errors. - Standardized HEALTHCHECK instructions across UI, API, and Warden Dockerfiles. - Hardened register/login endpoints with strict type validation for usernames and passwords. - Refined architecture documentation for neutral wording and fixed markdown lint regressions. - Updated MANUAL_TESTPLAN.md with PostgreSQL persistence and log-based cache verification tests. - Synchronized Warden task enqueuing logic to match Python Worker's Arq requirements (removed redundant job_id).
- Enabled TLS support (rustls) for sqlx in the Warden gateway. - Implemented environment variable overrides for critical database settings (DB_USER, DB_HOST, DB_NAME, DB_PORT) in Warden. - Standardized more documentation wording for neutrality and professional tone.
- Implemented generic 503 error masking in API search route - Standardized Docker HEALTHCHECKs and fixed Warden wget dependency - Refactored Warden configurator with elegant environment override closures - Completed database migration script with threads and turns tables - Enforced strict input type validation in authentication and search - Refined architecture and roadmap documentation for professional tone - Verified all security findings via end-to-end manual QA
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
searchboost_warden/src/relay.rs (2)
174-181:⚠️ Potential issue | 🟠 MajorTreat Redis read errors as failures, not
pending.
unwrap_or(None)on Line 174 makes a failedGETindistinguishable from a missing result, so callers can poll forever while Redis is unhealthy. Return 503 on command errors instead.🛠️ Proposed fix
- let result: Option<String> = conn.get(&result_key).await.unwrap_or(None);+ let result: Option<String> = match conn.get(&result_key).await {+ Ok(result) => result,+ Err(e) => {+ tracing::error!("RELAY: Failed to fetch result data: {}", e);+ warden.breaker.on_error();+ return (StatusCode::SERVICE_UNAVAILABLE, "Result lookup failed").into_response();+ }+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_warden/src/relay.rs` around lines 174 - 181, The code currently uses conn.get(&result_key).await.unwrap_or(None) which hides Redis command errors by turning them into None (treated as pending); change the call to capture the Result from conn.get(&result_key).await, and if it is Err(e) return a 503 ServiceUnavailable response (e.g., StatusCode::SERVICE_UNAVAILABLE with a JSON body like {"status":"error","message": e.to_string()}) instead of treating it as pending, otherwise proceed to match the Ok(Some(data)) / Ok(None) cases; update references around conn.get(&result_key), result_key, and the match that returns StatusCode::OK / StatusCode::ACCEPTED to handle the Err branch.
132-137:⚠️ Potential issue | 🔴 CriticalAbort the enqueue when Redis job persistence fails.
The
unwrap_or_elseon Line 133 only logs theSETEXfailure and then still runszadd. That can enqueue a job ID without its payload, leaving the worker with an orphaned queue entry.🛠️ Proposed fix
- let _: () = conn.set_ex(&job_key, pickled, 86400).await.unwrap_or_else(|e| {- tracing::error!("RELAY: Failed to set job data: {}", e);- });+ if let Err(e) = conn.set_ex(&job_key, pickled, 86400).await {+ tracing::error!("RELAY: Failed to set job data: {}", e);+ warden.breaker.on_error();+ return (StatusCode::INTERNAL_SERVER_ERROR, "Failed to persist job").into_response();+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@searchboost_warden/src/relay.rs` around lines 132 - 137, The current code logs failures from conn.set_ex (setting job_key) but continues to call conn.zadd, allowing a queue entry without payload; change the logic to abort the enqueue when conn.set_ex fails by returning or propagating the error instead of using unwrap_or_else: check the Result from conn.set_ex(job_key, pickled, 86400) and on Err immediately return Err or short-circuit (so you do not call conn.zadd), or use the ? operator to propagate the error; keep references to job_key, conn.set_ex, pickled and ensure conn.zadd("arq:queue", &job_id, score) only runs after a successful set_ex.
♻️ Duplicate comments (6)
.gsd/ARCHITECTURE.md (3)
54-54:⚠️ Potential issue | 🟠 MajorAlign
PIIDetectorbehavior with actual implementation.Line 54 claims a “triple-pass scan,” but the implementation in
searchboost_service/searchboost_src/pii_detector.pyperforms a single pass over registered patterns viascan().Suggested wording update
-- **Security:** PII-Gate implemented via `PIIDetector` (triple-pass scan). Recursive deep-merge configuration logic with `CLI > ENV > YAML` precedence.+- **Security:** PII risk is evaluated via `PIIDetector.scan()` (pattern-based single-pass detection) to guide cache-safety handling. Recursive deep-merge configuration logic with `CLI > ENV > YAML` precedence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gsd/ARCHITECTURE.md at line 54, Update the ARCHITECTURE.md description to reflect the actual behavior of the PIIDetector: replace the phrase "triple-pass scan" with wording that it performs a single-pass scan over registered patterns via the scan() method (or indicate "single-pass pattern scan"), and ensure the text references PIIDetector/scan() so readers understand the implementation matches the documentation.
36-36:⚠️ Potential issue | 🟠 MajorCorrect the HttpOnly/XSS security statement.
Line 36 overstates HttpOnly as XSS protection. HttpOnly limits token exfiltration from scripts, but does not prevent script execution.
Suggested wording update
-- **Security:** XSS protection via HttpOnly cookies (hides JWT from scripts); CSRF mitigation via `Strict` SameSite policy.+- **Security:** JWT is stored in HttpOnly cookies to reduce token exfiltration via script access; XSS requires separate controls (escaping/sanitization/CSP), and CSRF is mitigated with `SameSite=Strict` plus Origin/Referer validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gsd/ARCHITECTURE.md at line 36, The sentence claiming "HttpOnly cookies (hides JWT from scripts); CSRF mitigation via `Strict` SameSite policy" overstates HttpOnly as XSS protection—update the wording in ARCHITECTURE.md to say that HttpOnly reduces the risk of token exfiltration by preventing JavaScript access to the JWT but does not stop script execution or other XSS impacts, and keep the note that CSRF mitigation is provided via SameSite=`Strict`; reference the tokens/JWT, HttpOnly, XSS, and SameSite=`Strict` terms when making this clarification.
60-61:⚠️ Potential issue | 🟡 MinorFix
session_idownership in the data-flow steps.Lines 60-61 currently assign
session_idconstruction to Node, whilesearchboost_warden/src/relay.rsshows Warden constructs bothsession_idandjob_id.Suggested wording update
-3. Node constructs a **session_id** prefix (`SB-SESSION:${username}:${thread_id}`) and dispatches the query to the Rust `Warden`.-4. Warden generates a unique **job_id** by appending a UUID to the session prefix and enqueues the task in Redis.+3. Node dispatches `{ query, username, thread_id }` to the Rust `Warden`.+4. Warden constructs **session_id** (`SB-SESSION:${username}:${thread_id}`), appends a UUID to form **job_id**, and enqueues the task in Redis.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gsd/ARCHITECTURE.md around lines 60 - 61, The architecture text incorrectly says Node constructs the session_id; update the wording so it states that the Warden (searchboost_warden/src/relay.rs) constructs both the session_id (prefix "SB-SESSION:${username}:${thread_id}") and the job_id (by appending a UUID) and enqueues the task in Redis — i.e., change the step that currently assigns session_id construction to Node to instead attribute both session_id and job_id creation to Warden/relay.rs and keep the Redis enqueue behavior the same..gsd/ROADMAP.md (2)
49-49: 🧹 Nitpick | 🔵 TrivialUse “CLI” instead of “Terminal” for scope clarity.
Line 49 is ambiguous; “CLI” better matches the existing command-line control surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gsd/ROADMAP.md at line 49, Update the roadmap entry titled "7.5 **Dynamic LLM Selection**" to replace the word "Terminal" with "CLI" so the item reads "Allow UI/CLI overrides for Ollama model names"; edit the text within the .gsd/ROADMAP.md file at the 7.5 bullet (the "Dynamic LLM Selection" line) accordingly to maintain consistency with the project's command-line terminology.
60-60: 🧹 Nitpick | 🔵 TrivialDefine API/behavior parity requirements for the PIIDetector port.
Line 60 should state what must be preserved (input handling, match semantics, result shape, and logging behavior) to avoid drift during Python→Rust migration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gsd/ROADMAP.md at line 60, Update the ROADMAP line about "Rust Logic Migration" to explicitly list the API and behavior parity requirements to preserve during the PIIDetector port into the Warden: state that PIIDetector must keep identical input handling (accepted input types, encoding, normalization), match semantics (matching rules, precedence, thresholding, and deterministic tie-breaking), result shape (exact JSON / struct fields, field names, types, optional vs required, and error return formats), logging behavior (log levels, message formats, and emitted metadata), error handling and edge-case behavior (exceptions vs error values, nil/empty inputs), compatibility with the existing "Fast-Path" cache (cache keys, TTL semantics, eviction behavior, and race conditions), and include a requirement to add unit/integration tests that assert parity for these items and performance/regression benchmarks to detect behavioral drift.MANUAL_TESTPLAN.md (1)
9-13: 🧹 Nitpick | 🔵 TrivialConsider adding PostgreSQL verification step for stronger validation.
The objective mentions confirming "distinct
thread_idrecords in PostgreSQL," but the steps focus only on UI verification. Adding a direct database query would provide concrete evidence of proper persistence.📋 Suggested enhancement
3. Click '+ New Chat'. 4. Search "test query 2". Verify second session appears independent of the first main view. +5. Query PostgreSQL to verify distinct records: `SELECT thread_id, created_at FROM sessions ORDER BY created_at DESC LIMIT 2;` **Expected:** The UI sidebar must list two distinct session IDs. Both threads must persist independent conversation states upon page refreshes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MANUAL_TESTPLAN.md` around lines 9 - 13, Add a PostgreSQL verification step that queries the database for distinct thread_id records after creating two chats: connect to the app DB and run a SELECT to fetch thread_id (and associated id/timestamps) from the threads/conversations table to confirm two distinct thread_id values exist and map to the expected sessions; include this DB check as an extra step after UI verification so the manual test verifies both UI sidebar entries and persistent thread_id rows in Postgres.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MANUAL_TESTPLAN.md`:
- Around line 5-6: Add blank lines before and after each markdown heading to
satisfy MD022 and improve readability: insert an empty line above and below "##
Test 1: UI Session Multi-Thread Isolation" and similarly for the "Method:" lines
(e.g., the "Method: Browser UI (`http://${SB_INSTANCE_IP}`)") and repeat the
same spacing fixes for the headings in Test 3 ("## Test 3: ...") and Test 4 ("##
Test 4: ...") as well as the other occurrences flagged (lines referenced around
8-9, 33-34, 40-41) so every heading has a blank line separating it from
surrounding text.
- Around line 25-26: The test plan's Redis verification step for
"semantic_cache:*" is ambiguous about whether a missing key means the PII
detector rejected the query or the query was never cached; update the
MANUAL_TESTPLAN to add an explicit log-based verification: after running the
query and checking redis with `redis-cli KEYS "semantic_cache:*"`, also search
the application logs for a PII rejection entry (look for the PII detector's
rejection message or tag, e.g., "PII detector" / "PII rejected" or similar) to
confirm active rejection rather than absence, and instruct testers to retry
caching the same non-PII query to validate normal caching behavior as a control.
- Line 44: Step 1 is vague about how to obtain the session identifier; update
the MANUAL_TESTPLAN.md step that references "SB-SESSION:username:default" to
provide concrete discovery steps: instruct the tester to open browser DevTools →
Application/Storage to look for a cookie or localStorage key named "SB-SESSION",
or open DevTools → Network, inspect an authenticated request and check request
headers for an "SB-SESSION" header value (or provide the exact CLI/env command
if the session is set server-side), then show an example value format
("SB-SESSION:username:default") and note where to substitute the actual
username/token when running the test.
- Line 35: Update Step 3 to specify exactly what to check in Redis for "Request
A": list the expected key names/patterns (e.g., request:<requestA_id>:status,
request:<requestA_id>:response, embedding:<requestA_id>), the expected values or
states (status == "completed" or "ready", response non-empty), and any TTL or
timing expectations (e.g., keys should appear within X seconds and TTL > 0).
Mention how to obtain the request ID (from Step 1/Request A) and include a
pass/fail criterion (e.g., "pass if all listed keys exist with expected values;
fail otherwise"). Ensure the step text references "Request A" and the exact
Redis key patterns to remove ambiguity.
In `@searchboost_api/Dockerfile`:
- Around line 5-7: Replace the non-deterministic install step that uses "RUN npm
install --production" with a reproducible CI install using the package lock:
update the Dockerfile to use "RUN npm ci --omit=dev" so the image installs
exactly from package-lock.json, removes any existing node_modules, and fails
fast if the lockfile is out of sync; ensure the COPY of package*.json remains so
package-lock.json is available for "npm ci".
- Around line 9-13: Replace the two-step COPY + RUN chown/remove with a single
atomic COPY that sets ownership (use the COPY --chown=node:node ... form) and
remove the RUN line (the broken command "rm -f .env && chown -R node:node
/app"); additionally add a .dockerignore (or update it) to exclude .env, .env.*
and node_modules so the build context never includes those files instead of
deleting .env inside the image.
In `@searchboost_api/src/db/migrate.js`:
- Around line 28-29: Add a composite index to support the history query shape
used in conversation history (the query in searchboost_api/src/db/history.js
that uses WHERE session_id = $1 ORDER BY created_at ASC). Modify the migration
to create an index on conversation_turns(session_id, created_at) (optionally
include id as a third column for stable tie-breaking) instead of relying on the
single-column idx_turns_session_id so that the ORDER BY can be served by the
index and avoid large sorts.
- Around line 20-26: conversation_turns currently lacks a foreign key, so
conversation history can become orphaned when users are deleted (see deleteUser
in searchboost_api/src/db/users.js); modify the schema for conversation_turns to
reference its owning entity (e.g., add a foreign key column referencing
threads.id or users.id depending on your domain model) and enforce referential
integrity with ON DELETE CASCADE (or appropriate action) so that rows in
conversation_turns are removed when the parent thread or user is removed; update
migration and any code that inserts into conversation_turns (referenced symbols:
conversation_turns table, session_id column, threads table, users table,
deleteUser function) to use the new foreign key column.
- Around line 13-17: The threads table allows ownerless rows because
threads.user_id is nullable; change the schema to enforce ownership by making
user_id NOT NULL in the CREATE TABLE definition (update the SQL that defines the
threads table to include "user_id INTEGER NOT NULL REFERENCES users(id) ON
DELETE CASCADE"), and for existing databases add a migration that (1) cleans or
removes any rows where user_id IS NULL (or assigns a valid owner if
appropriate), (2) ALTER TABLE threads ALTER COLUMN user_id SET NOT NULL, and (3)
re-run/replace the current create/seed logic so any future inserts respect the
NOT NULL constraint; update any code that inserts threads (e.g., thread creation
functions) to always supply a user_id.
In `@searchboost_api/src/routes/auth.js`:
- Around line 15-96: Add application-level rate limiting to the authentication
routes to mitigate brute-force and credential-stuffing attacks: install/import
express-rate-limit, create limiter instances (e.g., loginLimiter for
router.post('/login') and registerLimiter for router.post('/register')) with
sensible settings (windowMs, max attempts, and a JSON error message), then
attach the middleware to the route handlers (e.g., router.post('/login',
loginLimiter, async (req, res, next) => { ... }) and similarly for '/register').
Ensure the limiter names (loginLimiter/registerLimiter) are unique and
configurable via environment variables if desired.
In `@searchboost_api/src/routes/search.js`:
- Around line 9-21: Validate and reject malformed inputs before building
payload: ensure req.body.query is a non-empty string (reject other truthy
non-strings) and ensure req.body.options is either undefined or a plain object
(reject arrays/primitives), returning res.status(400).json({ error: ... }) on
invalid input; then use the validated values for payload.query and
payload.options (keep thread_id validation as-is) so searchboost_warden/relay.rs
receives the expected types.
- Line 24: The console.log is dumping the entire proxy payload (variable
payload) which may contain PII; replace this with a safe log that either
omits/redacts sensitive fields or logs only non-PII metadata. Implement or call
a small helper (e.g., redactSensitiveFields(payload) or buildProxyLog(payload))
and use that in place of JSON.stringify(payload) to remove/replace fields like
query/search text, username/user ids and any sensitive options, or log only
stable metadata (request id, route, payload size, allowed option keys). Update
the log call that currently prints payload so it uses the redacted/metadata
object instead.
- Around line 27-38: The catch blocks for the /enqueue and /result/:job_id flows
are masking all upstream responses as 503 because validateStatus forces non-2xx
into throw and the catch always returns 503; update both catch handlers to
detect axios error.response and forward its status and body for client errors
(4xx) instead of replacing with 503: if error.response exists and
error.response.status < 500 then call
res.status(error.response.status).json(error.response.data) (preserving any
Warden error shape), otherwise log the error and return res.status(503).json({
error: 'Service Unavailable', details: 'The search gateway is currently offline
or unable to process this request.' }); ensure references to validateStatus,
axios.post/axios.get and response/error.response are used to locate the
handlers.
In `@searchboost_ui/Dockerfile`:
- Around line 28-29: The HEALTHCHECK line has a typo in the wget flag: replace
the incorrect "-no-verbose" with the correct "--no-verbose" so wget runs
properly (locate the HEALTHCHECK directive using the existing line that calls
wget --spider http://localhost:8080/). Also add a start period to the
HEALTHCHECK (e.g., include --start-period=5s in the HEALTHCHECK options) to give
nginx time to boot before retries begin.
In `@searchboost_warden/Dockerfile`:
- Around line 5-9: The apt-get install commands in the Dockerfile are pulling
recommended packages; update both RUN lines (the ones installing pkg-config
libssl-dev gcc and the later install at lines 27-31) to include
--no-install-recommends after apt-get install -y to avoid installing suggested
packages and reduce image size, preserving the existing && rm -rf
/var/lib/apt/lists/* cleanup.
- Around line 21-22: Remove the unnecessary RUN touch src/main.rs line because
it updates file mtimes and breaks Docker layer caching; delete that RUN
instruction and ensure the subsequent RUN that checks BUILD_PROFILE and runs
cargo build (the `RUN if [ "$BUILD_PROFILE" = "release" ]; then cargo build
--release; else cargo build; fi` step) remains immediately after the COPY steps
that add source files so caching behaves correctly.
- Line 3: ARG BUILD_PROFILE currently accepts any value but COPY uses
target/${BUILD_PROFILE}/, causing failures for unsupported values; add
validation after ARG BUILD_PROFILE to ensure it's exactly "release" or "debug"
(e.g., a RUN sh -c check: if [ "$BUILD_PROFILE" != "release" ] && [
"$BUILD_PROFILE" != "debug" ]; then echo "Invalid BUILD_PROFILE..."; exit 1; fi)
so builds fail fast with a clear error; keep the existing build logic that uses
--release when BUILD_PROFILE=release and plain cargo build for debug.
In `@searchboost_warden/src/configurator.rs`:
- Around line 64-71: The database_url method currently interpolates raw
credentials into a Postgres URI which breaks for passwords with reserved URI
characters; replace this by constructing connection options instead of raw URI:
use sqlx::postgres::PgConnectOptions (build with host via host(), port(),
username(), password() using self.password.as_deref().unwrap_or(""), and
database()) and pass that to sqlx::PgPool::connect_with, or if you must keep a
URI string percent-encode the user/password components (e.g., via
url::percent_encode) before formatting; update the database_url usage to return
or use the safe connection options so special characters are handled correctly.
In `@searchboost_warden/src/relay.rs`:
- Around line 100-110: The tracing::info call currently logs the full user
search text (payload.query); remove the raw query from routine info logs and
instead log only non-sensitive identifiers (username, session_id, job_id) plus a
derived metric such as query length or a deterministic hash (e.g., SHA256 or hex
prefix) if you need traceability. Update the tracing::info invocation in
relay.rs (the block using username = payload.username, session_id = session_id,
job_id = job_id, query = payload.query) to omit payload.query and include either
payload.query.len() or a computed hash/redaction token.
- Around line 83-94: The current use of unwrap_or(false) on the
sqlx::query_scalar call (which produces thread_exists) masks DB errors as
authorization failures; change the handling of the sqlx result from using
unwrap_or(false) to explicitly match the Result from sqlx::query_scalar::<_,
bool>(...)
.bind(&payload.thread_id).bind(&payload.username).fetch_one(&warden.db_pool).await
so that on Err(_) you return a 503 Service Unavailable (e.g.,
StatusCode::SERVICE_UNAVAILABLE) and on Ok(false) you keep returning the 403
Forbidden; ensure you reference thread_exists, the query call,
payload.thread_id/payload.username and warden.db_pool when updating the code so
transient metadata DB failures produce a 503 instead of a 403.
---
Outside diff comments:
In `@searchboost_warden/src/relay.rs`:
- Around line 174-181: The code currently uses
conn.get(&result_key).await.unwrap_or(None) which hides Redis command errors by
turning them into None (treated as pending); change the call to capture the
Result from conn.get(&result_key).await, and if it is Err(e) return a 503
ServiceUnavailable response (e.g., StatusCode::SERVICE_UNAVAILABLE with a JSON
body like {"status":"error","message": e.to_string()}) instead of treating it as
pending, otherwise proceed to match the Ok(Some(data)) / Ok(None) cases; update
references around conn.get(&result_key), result_key, and the match that returns
StatusCode::OK / StatusCode::ACCEPTED to handle the Err branch.
- Around line 132-137: The current code logs failures from conn.set_ex (setting
job_key) but continues to call conn.zadd, allowing a queue entry without
payload; change the logic to abort the enqueue when conn.set_ex fails by
returning or propagating the error instead of using unwrap_or_else: check the
Result from conn.set_ex(job_key, pickled, 86400) and on Err immediately return
Err or short-circuit (so you do not call conn.zadd), or use the ? operator to
propagate the error; keep references to job_key, conn.set_ex, pickled and ensure
conn.zadd("arq:queue", &job_id, score) only runs after a successful set_ex.
---
Duplicate comments:
In @.gsd/ARCHITECTURE.md:
- Line 54: Update the ARCHITECTURE.md description to reflect the actual behavior
of the PIIDetector: replace the phrase "triple-pass scan" with wording that it
performs a single-pass scan over registered patterns via the scan() method (or
indicate "single-pass pattern scan"), and ensure the text references
PIIDetector/scan() so readers understand the implementation matches the
documentation.
- Line 36: The sentence claiming "HttpOnly cookies (hides JWT from scripts);
CSRF mitigation via `Strict` SameSite policy" overstates HttpOnly as XSS
protection—update the wording in ARCHITECTURE.md to say that HttpOnly reduces
the risk of token exfiltration by preventing JavaScript access to the JWT but
does not stop script execution or other XSS impacts, and keep the note that CSRF
mitigation is provided via SameSite=`Strict`; reference the tokens/JWT,
HttpOnly, XSS, and SameSite=`Strict` terms when making this clarification.
- Around line 60-61: The architecture text incorrectly says Node constructs the
session_id; update the wording so it states that the Warden
(searchboost_warden/src/relay.rs) constructs both the session_id (prefix
"SB-SESSION:${username}:${thread_id}") and the job_id (by appending a UUID) and
enqueues the task in Redis — i.e., change the step that currently assigns
session_id construction to Node to instead attribute both session_id and job_id
creation to Warden/relay.rs and keep the Redis enqueue behavior the same.
In @.gsd/ROADMAP.md:
- Line 49: Update the roadmap entry titled "7.5 **Dynamic LLM Selection**" to
replace the word "Terminal" with "CLI" so the item reads "Allow UI/CLI overrides
for Ollama model names"; edit the text within the .gsd/ROADMAP.md file at the
7.5 bullet (the "Dynamic LLM Selection" line) accordingly to maintain
consistency with the project's command-line terminology.
- Line 60: Update the ROADMAP line about "Rust Logic Migration" to explicitly
list the API and behavior parity requirements to preserve during the PIIDetector
port into the Warden: state that PIIDetector must keep identical input handling
(accepted input types, encoding, normalization), match semantics (matching
rules, precedence, thresholding, and deterministic tie-breaking), result shape
(exact JSON / struct fields, field names, types, optional vs required, and error
return formats), logging behavior (log levels, message formats, and emitted
metadata), error handling and edge-case behavior (exceptions vs error values,
nil/empty inputs), compatibility with the existing "Fast-Path" cache (cache
keys, TTL semantics, eviction behavior, and race conditions), and include a
requirement to add unit/integration tests that assert parity for these items and
performance/regression benchmarks to detect behavioral drift.
In `@MANUAL_TESTPLAN.md`:
- Around line 9-13: Add a PostgreSQL verification step that queries the database
for distinct thread_id records after creating two chats: connect to the app DB
and run a SELECT to fetch thread_id (and associated id/timestamps) from the
threads/conversations table to confirm two distinct thread_id values exist and
map to the expected sessions; include this DB check as an extra step after UI
verification so the manual test verifies both UI sidebar entries and persistent
thread_id rows in Postgres.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b1448db-ca82-434d-8b53-b97032944198
📒 Files selected for processing (12)
.gsd/ARCHITECTURE.md.gsd/ROADMAP.mdMANUAL_TESTPLAN.mdsearchboost_api/Dockerfilesearchboost_api/src/db/migrate.jssearchboost_api/src/routes/auth.jssearchboost_api/src/routes/search.jssearchboost_ui/Dockerfilesearchboost_warden/Cargo.tomlsearchboost_warden/Dockerfilesearchboost_warden/src/configurator.rssearchboost_warden/src/relay.rs
Comment on lines
+5
to
+6
| ## Test 1: UI Session Multi-Thread Isolation | ||
| **Method:** Browser UI (`http://${SB_INSTANCE_IP}`) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add blank lines around headings for better readability.
Multiple headings lack surrounding blank lines, which reduces readability. The markdown linter flags this as MD022.
📋 Suggested formatting improvements
**Target Environment:** `dev-web-ui` (Phase 5 & Semantic Caching)
+
## Test 1: UI Session Multi-Thread Isolation
**Method:** Browser UI (`http://${SB_INSTANCE_IP}`)
**Objective:** Confirm that the React component spawns clear isolated `thread_id` records in PostgreSQL and visually renders them simultaneously.
+
**Steps:**
1. Login to the application. ## Test 2: PII-Safe Semantic Caching (Hard Gate)
**Method:** Terminal (`curl` / `redis-cli`)
**Objective:** Confirm that sensitive inputs (e.g. Credit Cards) are actively rejected by the `PIIDetector` and NEVER written to the Redis cache.
+
**Steps:**
1. Clear existing semantic cache keys (`redis-cli KEYS "semantic_cache:*" | xargs redis-cli DEL`).Apply similar fixes to Test 3 (line 33) and Test 4 (line 40).
Also applies to: 8-9, 33-34, 40-41
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 5-5: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MANUAL_TESTPLAN.md` around lines 5 - 6, Add blank lines before and after each
markdown heading to satisfy MD022 and improve readability: insert an empty line
above and below "## Test 1: UI Session Multi-Thread Isolation" and similarly for
the "Method:" lines (e.g., the "Method: Browser UI
(`http://${SB_INSTANCE_IP}`)") and repeat the same spacing fixes for the
headings in Test 3 ("## Test 3: ...") and Test 4 ("## Test 4: ...") as well as
the other occurrences flagged (lines referenced around 8-9, 33-34, 40-41) so
every heading has a blank line separating it from surrounding text.
Comment on lines
+25
to
+26
| 4. Check Redis keys using `redis-cli KEYS "semantic_cache:*"`. | ||
| **Expected:** The `semantic_cache` MUST NOT contain any keys corresponding to the PII-laden query. |
There was a problem hiding this comment.
Clarify verification criteria to distinguish rejection from absence.
The expected outcome states the cache "MUST NOT contain any keys," but this doesn't differentiate between the PII detector actively rejecting the query versus the query simply not being cached yet. Consider adding a verification step that checks application logs for explicit PII rejection.
🔍 Suggested enhancement
4. Check Redis keys using `redis-cli KEYS "semantic_cache:*"`.
-**Expected:** The `semantic_cache` MUST NOT contain any keys corresponding to the PII-laden query.+**Expected:** The `semantic_cache` MUST NOT contain any keys corresponding to the PII-laden query. Verify application logs show explicit PII rejection (e.g., "PII detected, skipping cache" or similar message).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MANUAL_TESTPLAN.md` around lines 25 - 26, The test plan's Redis verification
step for "semantic_cache:*" is ambiguous about whether a missing key means the
PII detector rejected the query or the query was never cached; update the
MANUAL_TESTPLAN to add an explicit log-based verification: after running the
query and checking redis with `redis-cli KEYS "semantic_cache:*"`, also search
the application logs for a PII rejection entry (look for the PII detector's
rejection message or tag, e.g., "PII detector" / "PII rejected" or similar) to
confirm active rejection rather than absence, and instruct testers to retry
caching the same non-PII query to validate normal caching behavior as a control.
| **Steps:** | ||
| 1. Clear existing semantic cache keys (`redis-cli KEYS "semantic_cache:*" | xargs redis-cli DEL`). | ||
| 2. Enqueue Request A: "can you tell me who the current president of france is right now" | ||
| 3. Wait for LLM optimization and response. Check Redis for keys. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify what to verify in Redis after Request A.
Step 3 instructs to "Check Redis for keys" but doesn't specify the expected state or what constitutes success. This makes the test harder to execute consistently.
📝 Suggested clarification
-3. Wait for LLM optimization and response. Check Redis for keys.+3. Wait for LLM optimization and response. Verify Redis contains exactly one `semantic_cache:*` key using `redis-cli KEYS "semantic_cache:*"`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MANUAL_TESTPLAN.md` at line 35, Update Step 3 to specify exactly what to
check in Redis for "Request A": list the expected key names/patterns (e.g.,
request:<requestA_id>:status, request:<requestA_id>:response,
embedding:<requestA_id>), the expected values or states (status == "completed"
or "ready", response non-empty), and any TTL or timing expectations (e.g., keys
should appear within X seconds and TTL > 0). Mention how to obtain the request
ID (from Step 1/Request A) and include a pass/fail criterion (e.g., "pass if all
listed keys exist with expected values; fail otherwise"). Ensure the step text
references "Request A" and the exact Redis key patterns to remove ambiguity.
| **Method:** Terminal (`psql`) | ||
| **Objective:** Confirm that threads and individual user messages are correctly mapped to the PostgreSQL `history` table with proper ownership. | ||
| **Steps:** | ||
| 1. Identify the current user's session identifier (e.g., `SB-SESSION:username:default`). |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Provide concrete guidance for obtaining the session ID.
Step 1 assumes a specific session ID format (SB-SESSION:username:default) without explaining how to discover it. If this format is incorrect or changes, the test will fail.
🔧 Suggested improvement
-1. Identify the current user's session identifier (e.g., `SB-SESSION:username:default`).+1. Obtain the current session identifier from the UI (check browser localStorage/sessionStorage for `session_id` or `thread_id` key) or query PostgreSQL: `SELECT DISTINCT session_id FROM history ORDER BY created_at DESC LIMIT 5;`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MANUAL_TESTPLAN.md` at line 44, Step 1 is vague about how to obtain the
session identifier; update the MANUAL_TESTPLAN.md step that references
"SB-SESSION:username:default" to provide concrete discovery steps: instruct the
tester to open browser DevTools → Application/Storage to look for a cookie or
localStorage key named "SB-SESSION", or open DevTools → Network, inspect an
authenticated request and check request headers for an "SB-SESSION" header value
(or provide the exact CLI/env command if the session is set server-side), then
show an example value format ("SB-SESSION:username:default") and note where to
substitute the actual username/token when running the test.
Comment on lines
+5
to
+7
| COPY package*.json ./ | ||
| RUN npm install --production |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer npm ci --omit=dev for reproducible production builds.
npm ci ensures deterministic installs from package-lock.json, removes any existing node_modules, and fails fast if the lockfile is out of sync—all preferable for CI/CD pipelines.
♻️ Proposed fix
COPY package*.json ./
-RUN npm install --production+RUN npm ci --omit=dev📝 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
| COPY package*.json ./ | |
| RUN npm install --production | |
| COPY package*.json ./ | |
| RUN npm ci --omit=dev |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@searchboost_api/Dockerfile` around lines 5 - 7, Replace the non-deterministic
install step that uses "RUN npm install --production" with a reproducible CI
install using the package lock: update the Dockerfile to use "RUN npm ci
--omit=dev" so the image installs exactly from package-lock.json, removes any
existing node_modules, and fails fast if the lockfile is out of sync; ensure the
COPY of package*.json remains so package-lock.json is available for "npm ci".
Comment on lines
5
to
+9
| RUN apt-get update && apt-get install -y \ | ||
| pkg-config \ | ||
| libssl-dev \ | ||
| gcc \ | ||
| &&rm -rf /var/lib/apt/lists/* | ||
| &&rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
Add --no-install-recommends to both apt installs.
Both install steps currently pull recommended packages unnecessarily, increasing image size and attack surface.
Proposed fix
-RUN apt-get update && apt-get install -y \+RUN apt-get update && apt-get install -y --no-install-recommends \
pkg-config \
libssl-dev \
gcc \
&& rm -rf /var/lib/apt/lists/*
@@
-RUN apt-get update && apt-get install -y \+RUN apt-get update && apt-get install -y --no-install-recommends \
ca-certificates \
libssl3 \
wget \
&& rm -rf /var/lib/apt/lists/*Also applies to: 27-31
🧰 Tools
🪛 Hadolint (2.14.0)
[warning] 5-5: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
[info] 5-5: Avoid additional packages by specifying --no-install-recommends
(DL3015)
🪛 Trivy (0.69.3)
[error] 5-9: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'apt-get update && apt-get install -y pkg-config libssl-dev gcc && rm -rf /var/lib/apt/lists/*'
Rule: DS-0029
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@searchboost_warden/Dockerfile` around lines 5 - 9, The apt-get install
commands in the Dockerfile are pulling recommended packages; update both RUN
lines (the ones installing pkg-config libssl-dev gcc and the later install at
lines 27-31) to include --no-install-recommends after apt-get install -y to
avoid installing suggested packages and reduce image size, preserving the
existing && rm -rf /var/lib/apt/lists/* cleanup.
Comment on lines
+21
to
+22
| RUN touch src/main.rs | ||
| RUN if [ "$BUILD_PROFILE" = "release" ]; then cargo build --release; else cargo build; fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Confirm cache-busting mutation exists.
rg -n 'touch\s+src/main.rs' --iglob 'Dockerfile*'# Confirm real main entrypoint exists in source tree.
fd -i 'main.rs'Repository: Somnerd/SearchBoost
Length of output: 148
Remove touch src/main.rs line; it undermines Docker layer caching.
The touch command on line 21 updates the file's modification time on every build, invalidating Docker's build cache even when source code hasn't changed. This forces unnecessary recompilation of the root crate.
Proposed fix
-RUN touch src/main.rs
RUN if [ "$BUILD_PROFILE" = "release" ]; then cargo build --release; else cargo build; fi📝 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
| RUN touch src/main.rs | |
| RUN if [ "$BUILD_PROFILE" = "release" ]; then cargo build --release; else cargo build; fi | |
| RUN if [ "$BUILD_PROFILE" = "release" ]; then cargo build --release; else cargo build; fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@searchboost_warden/Dockerfile` around lines 21 - 22, Remove the unnecessary
RUN touch src/main.rs line because it updates file mtimes and breaks Docker
layer caching; delete that RUN instruction and ensure the subsequent RUN that
checks BUILD_PROFILE and runs cargo build (the `RUN if [ "$BUILD_PROFILE" =
"release" ]; then cargo build --release; else cargo build; fi` step) remains
immediately after the COPY steps that add source files so caching behaves
correctly.
Comment on lines
+64
to
+71
| pub fn database_url(&self) -> String { | ||
| match &self.password { | ||
| Some(pass) if !pass.is_empty() => { | ||
| format!("postgres://{}:{}@{}:{}/{}", self.user, pass, self.host, self.port, self.database) | ||
| } | ||
| _ => { | ||
| format!("postgres://{}@{}:{}/{}", self.user, self.host, self.port, self.database) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "configurator.rs" -type f 2>/dev/null | head -5Repository: Somnerd/SearchBoost
Length of output: 45
🏁 Script executed:
git ls-files | grep -E "configurator\.rs|Cargo\.toml"Repository: Somnerd/SearchBoost
Length of output: 131
🏁 Script executed:
# Look for the configurator.rs file
find . -name "configurator.rs" -type fRepository: Somnerd/SearchBoost
Length of output: 103
🏁 Script executed:
# Check Cargo.toml for sqlx versionif [ -f"Cargo.toml" ];then cat Cargo.toml;fiRepository: Somnerd/SearchBoost
Length of output: 45
🏁 Script executed:
# Look for database configuration files
git ls-files | grep -E "(config|database|setup)"| head -20Repository: Somnerd/SearchBoost
Length of output: 481
🏁 Script executed:
cat -n searchboost_warden/src/configurator.rs | head -100Repository: Somnerd/SearchBoost
Length of output: 4211
🏁 Script executed:
cat searchboost_warden/Cargo.tomlRepository: Somnerd/SearchBoost
Length of output: 897
🏁 Script executed:
# Check where database_url() is being used in the codebase
rg "database_url" searchboost_warden/src/Repository: Somnerd/SearchBoost
Length of output: 250
🏁 Script executed:
# Search for sqlx usage patterns in the codebase
rg "sqlx::" searchboost_warden/src/ -A 3Repository: Somnerd/SearchBoost
Length of output: 923
🏁 Script executed:
# Check if there are any other database connection patterns
rg "PgPool\|PgConnectOptions\|postgres://" searchboost_warden/src/Repository: Somnerd/SearchBoost
Length of output: 45
🏁 Script executed:
cat -n searchboost_warden/src/main.rs | grep -A 5 -B 5 "database_url"Repository: Somnerd/SearchBoost
Length of output: 534
🏁 Script executed:
# Check the sqlx documentation/API for special character handling
web_search
sqlx postgres connection special characters credentials percent encodingRepository: Somnerd/SearchBoost
Length of output: 156
🌐 Web query:
sqlx postgres connection special characters credentials percent encoding
💡 Result:
In Rust's sqlx crate for PostgreSQL connections, special characters in credentials (username or password) within the DATABASE_URL or connection string must be percent-encoded according to URL standards (RFC 3986). This applies when parsing strings into PgConnectOptions. Key points: - Use the format: postgres://user:password@host:port/dbname or postgresql://... - Percent-encode reserved characters in the userinfo part (before @), such as / -> %2F, ? -> %3F, # -> %23, :, @, etc. - Not all special chars need encoding (e.g., $ often works unencoded), but encode any with URL special meaning to avoid parsing errors. - sqlx follows libpq URI parsing rules strictly; partial encoding fails, as seen in issues where users needed full encoding like /7M+r?E' -> %2F7M%2Br%3FE%27%3AdHz. Example: let password = "/7M+r?E':dHz"; let encoded_pass = urlencoding::encode(password); // or manual: "%2F7M%2Br%3FE'%3AdHz" let url = format!("postgres://sys_d:{}@192.168.10.2/dbkn", encoded_pass); let pool = PgPool::connect(&url).await?; Official docs confirm: "characters not allowed in URLs must be percent-encoded." Use urlencoding crate or online tools for convenience. Alternatively, build PgConnectOptions programmatically with .username and .password to bypass URL parsing entirely: let opts = PgConnectOptions::new .host("192.168.10.2") .port(5432) .dbname("dbkn") .username("sys_d") .password("/7M+r?E':dHz"); // raw password, no encoding needed let pool = PgPool::from(opts).await?; This avoids encoding issues completely for complex credentials.
Citations:
- 1: sqlx problem with passwords containing special chars and % encoding transact-rs/sqlx#3323
- 2: https://docs.rs/sqlx/latest/sqlx/postgres/struct.PgConnectOptions.html
- 3: https://docs.rs/sqlx-postgres/latest/sqlx_postgres/struct.PgConnectOptions.html
🌐 Web query:
sqlx PgConnectOptions vs connection string
💡 Result:
In sqlx for Rust PostgreSQL connections, PgConnectOptions and connection strings are closely related but serve different purposes. Connection strings are PostgreSQL/libpq-style URLs like "postgres://user:pass@localhost:5432/dbname?sslmode=require". They can be used directly with PgConnection::connect or PgPool::connect, which parse the string internally into PgConnectOptions. PgConnectOptions is a builder struct for configuring connections programmatically. It implements FromStr, so it can be parsed directly from a connection string: let opts: PgConnectOptions = "postgres://...".parse?; Key differences and use cases: - Connection string (simple): PgPool::connect("postgres://localhost/mydb").await?; - Parse and modify: let mut opts: PgConnectOptions = "postgres://localhost/mydb".parse?; opts = opts.host("newhost").port(5433); PgPool::connect_with(opts).await?; - Builder from scratch: let opts = PgConnectOptions::new.host("localhost").database("mydb").username("user").password("pass"); PgConnectOptions supports all libpq parameters via URL query params, plus sqlx-specific ones like statement-cache-capacity=100. It also reads environment variables like PGHOST, PGDATABASE by default. Both approaches are equivalent for basic use, but PgConnectOptions offers more flexibility for dynamic configuration, Unix sockets, SSL certs, and avoiding string parsing overhead in complex setups.
Citations:
- 1: https://docs.rs/sqlx/latest/sqlx/postgres/struct.PgConnectOptions.html
- 2: https://docs.rs/sqlx-postgres/latest/sqlx_postgres/struct.PgConnectOptions.html
- 3: https://docs.rs/sqlx-oldapi/latest/sqlx_oldapi/postgres/struct.PgConnectOptions.html
- 4: https://docs.rs/sqlx/latest/sqlx/postgres/struct.PgConnection.html
- 5: https://github.com/launchbadge/sqlx
Use PgConnectOptions builder or percent-encode credentials to handle special characters in passwords.
Line 67 constructs the Postgres URI by direct string interpolation of raw credentials. Passwords containing reserved URI characters (e.g., @, :, /, ?, #) will either fail the connection or misdirect to the wrong host/database. sqlx follows libpq URI parsing rules strictly and requires percent-encoding for any special characters in the userinfo component.
Use one of these approaches:
Recommended: Build
PgConnectOptionsprogrammatically—sqlx will handle raw credentials without parsing:let opts = PgConnectOptions::new().host(&self.host).port(self.port).username(&self.user).password(&self.password.as_deref().unwrap_or("")).database(&self.database); sqlx::PgPool::connect_with(opts).await
Alternative: Percent-encode each component before formatting (use
urlencodingcrate orurl::percent_encode).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@searchboost_warden/src/configurator.rs` around lines 64 - 71, The
database_url method currently interpolates raw credentials into a Postgres URI
which breaks for passwords with reserved URI characters; replace this by
constructing connection options instead of raw URI: use
sqlx::postgres::PgConnectOptions (build with host via host(), port(),
username(), password() using self.password.as_deref().unwrap_or(""), and
database()) and pass that to sqlx::PgPool::connect_with, or if you must keep a
URI string percent-encode the user/password components (e.g., via
url::percent_encode) before formatting; update the database_url usage to return
or use the safe connection options so special characters are handled correctly.
Comment on lines
+83
to
+94
| let thread_exists: bool = sqlx::query_scalar::<_, bool>( | ||
| "SELECT EXISTS(SELECT 1 FROM threads t JOIN users u ON t.user_id = u.id WHERE t.id::text = $1 AND u.username = $2)" | ||
| ) | ||
| .bind(&payload.thread_id) | ||
| .bind(&payload.username) | ||
| .fetch_one(&warden.db_pool) | ||
| .await | ||
| .unwrap_or(false); | ||
| if !thread_exists { | ||
| tracing::warn!("Blocked IDOR Attempt: {} tried to access thread {}", payload.username, payload.thread_id); | ||
| return (StatusCode::FORBIDDEN, "Thread access denied").into_response(); |
There was a problem hiding this comment.
Return a 503 when the ownership lookup fails.
The unwrap_or(false) on Line 90 turns any Postgres error into 403 Thread access denied. A transient metadata DB failure will lock out valid users and look like an authorization failure instead of an infrastructure failure.
🛠️ Proposed fix
- let thread_exists: bool = sqlx::query_scalar::<_, bool>(+ let thread_exists = match sqlx::query_scalar::<_, bool>(
"SELECT EXISTS(SELECT 1 FROM threads t JOIN users u ON t.user_id = u.id WHERE t.id::text = $1 AND u.username = $2)"
)
.bind(&payload.thread_id)
.bind(&payload.username)
.fetch_one(&warden.db_pool)
.await
- .unwrap_or(false);+ {+ Ok(exists) => exists,+ Err(e) => {+ tracing::error!("RELAY: Failed to verify thread ownership: {}", e);+ return (StatusCode::SERVICE_UNAVAILABLE, "Metadata lookup failed").into_response();+ }+ };📝 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
| let thread_exists:bool = sqlx::query_scalar::<_,bool>( | |
| "SELECT EXISTS(SELECT 1 FROM threads t JOIN users u ON t.user_id = u.id WHERE t.id::text = $1 AND u.username = $2)" | |
| ) | |
| .bind(&payload.thread_id) | |
| .bind(&payload.username) | |
| .fetch_one(&warden.db_pool) | |
| .await | |
| .unwrap_or(false); | |
| if !thread_exists { | |
| tracing::warn!("Blocked IDOR Attempt: {} tried to access thread {}", payload.username, payload.thread_id); | |
| return(StatusCode::FORBIDDEN,"Thread access denied").into_response(); | |
| let thread_exists = match sqlx::query_scalar::<_,bool>( | |
| "SELECT EXISTS(SELECT 1 FROM threads t JOIN users u ON t.user_id = u.id WHERE t.id::text = $1 AND u.username = $2)" | |
| ) | |
| .bind(&payload.thread_id) | |
| .bind(&payload.username) | |
| .fetch_one(&warden.db_pool) | |
| .await | |
| { | |
| Ok(exists) => exists, | |
| Err(e) => { | |
| tracing::error!("RELAY: Failed to verify thread ownership: {}", e); | |
| return(StatusCode::SERVICE_UNAVAILABLE,"Metadata lookup failed").into_response(); | |
| } | |
| }; | |
| if !thread_exists { | |
| tracing::warn!("Blocked IDOR Attempt: {} tried to access thread {}", payload.username, payload.thread_id); | |
| return(StatusCode::FORBIDDEN,"Thread access denied").into_response(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@searchboost_warden/src/relay.rs` around lines 83 - 94, The current use of
unwrap_or(false) on the sqlx::query_scalar call (which produces thread_exists)
masks DB errors as authorization failures; change the handling of the sqlx
result from using unwrap_or(false) to explicitly match the Result from
sqlx::query_scalar::<_, bool>(...)
.bind(&payload.thread_id).bind(&payload.username).fetch_one(&warden.db_pool).await
so that on Err(_) you return a 503 Service Unavailable (e.g.,
StatusCode::SERVICE_UNAVAILABLE) and on Ok(false) you keep returning the 403
Forbidden; ensure you reference thread_exists, the query call,
payload.thread_id/payload.username and warden.db_pool when updating the code so
transient metadata DB failures produce a 503 instead of a 403.
Comment on lines
100
to
+110
| tracing::info!( | ||
| "Relaying query for: | ||
| \nsession : {session_id} | ||
| \nJob ID : {job_id} | ||
| \nQuery : {query}", | ||
| session_id = payload.session_id, | ||
| "Validated & Enqueued: | ||
| \nusername : {username} | ||
| \nsession : {session_id} | ||
| \nJob ID : {job_id} | ||
| \nQuery : {query}", | ||
| username = payload.username, | ||
| session_id = session_id, | ||
| job_id = job_id, | ||
| query = payload.query | ||
| ); | ||
| //let mut conn = warden.redis_client.get_async_connection().await.unwrap(); | ||
| ); |
There was a problem hiding this comment.
Stop logging raw search queries at info level.
This log records username, session_id, and the full query for every enqueue. Search text is user input and often sensitive; keep only non-sensitive identifiers or derived metrics in routine logs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@searchboost_warden/src/relay.rs` around lines 100 - 110, The tracing::info
call currently logs the full user search text (payload.query); remove the raw
query from routine info logs and instead log only non-sensitive identifiers
(username, session_id, job_id) plus a derived metric such as query length or a
deterministic hash (e.g., SHA256 or hex prefix) if you need traceability. Update
the tracing::info invocation in relay.rs (the block using username =
payload.username, session_id = session_id, job_id = job_id, query =
payload.query) to omit payload.query and include either payload.query.len() or a
computed hash/redaction token.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Security
Chores