feat: ship interactive Runbook embed demo flow - #16
Merged
Merged
Conversation
Turn the embed demo into a functional product path with real chat retrieval, Studio-driven assistant configuration, and resilient LLM fallbacks so the end-to-end onboarding demo works reliably without required external integrations. Made-with: Cursor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 53 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request introduces a complete embedded AI onboarding assistant system with GitHub OAuth authentication, project management APIs, document indexing capabilities, and a configurable Studio interface. The implementation includes a JavaScript embed widget, chat API endpoints, session signing, rate limiting, and demo knowledge fallbacks. Changes
Sequence DiagramssequenceDiagram
participant User as User Browser
participant Start as /api/embed/github/start
participant Callback as /api/embed/github/callback
participant GitHub as GitHub OAuth
User->>Start: GET /api/embed/github/start
Start->>Start: Generate state, set cookies
Start->>User: Redirect to GitHub authorize
User->>GitHub: User clicks authorize
GitHub->>Callback: Redirect with code & state
Callback->>Callback: Verify state, exchange code
Callback->>GitHub: POST /token
GitHub->>Callback: Access token
Callback->>Callback: Fetch user, sign session
Callback->>User: Set embed_session cookie<br/>Redirect to /studio
User->>User: Authenticated
sequenceDiagram
participant Widget as Embed Widget
participant API as /api/embed/chat
participant Retrieval as Document Retrieval
participant LLM as Gemini API
Widget->>Widget: User sends message
Widget->>API: POST { projectId, message, pageContext }
API->>API: Verify auth (if non-demo)
API->>Retrieval: retrieveDocsForEmbed(question)
Retrieval->>Retrieval: Generate embedding<br/>Vector search
Retrieval->>API: Return top 6 documents
API->>LLM: POST with system prompt<br/>+ indexed excerpts
LLM->>API: Response with steps JSON
API->>API: Extract steps, format answer
API->>Widget: { answer, sources, steps }
Widget->>Widget: Render response
sequenceDiagram
participant User as User (Studio)
participant Index as /api/embed/projects/[id]/index
participant GitHub as GitHub API
participant Indexer as indexGitHubRepo
participant Embed as generateEmbedding
participant DB as Supabase
User->>Index: POST (trigger index)
Index->>Index: Verify session & ownership
Index->>GitHub: Fetch repo metadata<br/>Get file tree
GitHub->>Indexer: Tree + file contents
Indexer->>Indexer: Filter files, chunk content
loop Per Chunk
Indexer->>Embed: generateEmbedding(text)
Embed->>Embed: Call Gemini Embedding API
Indexer->>DB: Upsert document<br/>+ embedding vector
end
Indexer->>Index: { files, chunks, bytes }
Index->>User: { ok: true, result }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Remove effect-driven initialization in Studio and clean up unused catch variables/constants so eslint passes consistently in CI. Made-with: Cursor
Replace fast SHA-256 hashing for embed API keys with scrypt-based derivation to address CodeQL weak password hash findings. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (15)
src/app/api/embed/me/route.ts (1)
6-16: Consider returning 200 (not 401) for the unauthenticated probe.
/api/embed/meis a "who am I?" probe rather than an authorization-required resource. Returning401will:
- Show as a red error in DevTools network/console on every signed-out page load.
- Trip generic fetch interceptors / monitoring tools that treat 401 as an auth failure to surface.
Most
/me-style endpoints return200 { authenticated: false }for the unauthenticated case and reserve401for endpoints that genuinely require auth.♻️ Suggested change
const session = verifyEmbedSession(req.cookies.get("embed_session")?.value); if (!session) { - return NextResponse.json({ authenticated: false }, { status: 401 }); + return NextResponse.json({ authenticated: false }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/embed/me/route.ts` around lines 6 - 16, The GET handler currently returns a 401 when verifyEmbedSession(req.cookies.get("embed_session")?.value) yields no session; change this to return a 200 with { authenticated: false } instead so /api/embed/me behaves as a probe rather than an auth-gated resource—update the early return in the GET function that uses NextResponse.json(...) to use a 200 status (or omit the status) while keeping the same response body and leave the authenticated:true branch unchanged.src/lib/studioDemoStorage.ts (1)
59-67: Silently swallowing quota errors hides save failures from Studio users.If a user pastes a large manual source and
localStoragerejects withQuotaExceededError, Studio will appear to "save" but the next reload will revert. Consider returning a boolean / throwing so the caller can surface a toast, or at least log toconsole.warn.♻️ Minimal change
-export function saveDemoBundle(bundle: DemoBundle): void { +export function saveDemoBundle(bundle: DemoBundle): boolean { if (typeof window === "undefined") return; try { localStorage.setItem(RUNBOOK_DEMO_BUNDLE_KEY, JSON.stringify(bundle)); window.dispatchEvent(new Event("runbook-demo-update")); + return true; } catch { - /* quota */ + console.warn("[runbook] Failed to persist demo bundle (quota?)"); + return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/studioDemoStorage.ts` around lines 59 - 67, The current saveDemoBundle function silently swallows storage quota errors; update saveDemoBundle to surface failures by changing its signature to return a boolean (true on success, false on failure) and in the catch block log the error with console.warn including the caught error and the RUNBOOK_DEMO_BUNDLE_KEY (or rethrow if caller expects exceptions). Keep the existing behavior of setting localStorage and dispatching the "runbook-demo-update" event on success, but on failure ensure you log the full error (e.g., mentioning QuotaExceededError) and return false so callers can show a toast or handle the failure.src/lib/embedKeywordRetrieval.ts (1)
23-31: Clarify scoring expression and avoidsplitfor counting.
hay.split(t).length - 1 + 2is the per-term contribution: match-count plus a flat+2bonus per matched term. The intent isn't obvious at a glance, andsplitallocates an array of substrings just to count occurrences. A small refactor makes intent explicit and is cheaper:♻️ Suggested rewrite
const scored = docs.map((doc) => { const hay = `${doc.title}\n${doc.content}`.toLowerCase(); let score = 0; for (const t of terms) { - if (hay.includes(t)) score += hay.split(t).length - 1 + 2; + let occurrences = 0; + let idx = hay.indexOf(t); + while (idx !== -1) { + occurrences += 1; + idx = hay.indexOf(t, idx + t.length); + } + if (occurrences > 0) score += occurrences + 2; // +2 per matched term } if (doc.title.toLowerCase().split(/\s+/).some((w) => terms.includes(w))) score += 4; return { doc, score }; });Also note: substring matching causes "go" to match inside "google" — fine for a demo, but worth a
\b…\bregex if you ever want stricter matching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/embedKeywordRetrieval.ts` around lines 23 - 31, The scoring code inside the docs.map that builds scored is unclear and inefficient: replace the expression using hay.split(t).length - 1 + 2 with an explicit occurrence count and a separate per-term bonus so the intent (count occurrences + flat +2 bonus per matched term) is obvious; inside the map over docs compute occurrences for each term using a simple indexOf loop or a global RegExp to count matches (or use word-boundary RegExp /\bterm\b/ if stricter matching is desired), add occurrences to score and then add the +2 bonus only when occurrences > 0, and keep the title-word exact match bonus logic (the doc.title.toLowerCase().split...) unchanged.src/lib/prompts.ts (1)
1-8: System prompt should document theRUNBOOK_STEPS_JSONcontract for consistency.The
RUNBOOK_STEPS_JSONinstruction is already provided in the user message template atsrc/app/api/embed/chat/route.ts(line 153), so the system prompt insrc/lib/prompts.tsis not functionally broken. However, this system prompt should explicitly mention the contract to avoid confusion and ensure clarity about the expected output format. Consider adding:- After your answer, on a new final line: RUNBOOK_STEPS_JSON: ["imperative step 1", "step 2", ...] (3-6 steps, or empty array [] if not applicable).This aligns the system prompt documentation with the actual parsing logic and the contract enforced via the user message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/prompts.ts` around lines 1 - 8, Update the EMBED_CHAT_SYSTEM_PROMPT constant to explicitly document the RUNBOOK_STEPS_JSON output contract: state that after the answer the assistant must append a final line starting with RUNBOOK_STEPS_JSON: followed by a JSON array of 3–6 imperative steps (or [] if none). Modify the string in EMBED_CHAT_SYSTEM_PROMPT so it includes that exact instruction text, ensuring consistency with the user message template and parsing logic used in route handler (referencing EMBED_CHAT_SYSTEM_PROMPT and RUNBOOK_STEPS_JSON to locate where to change).src/app/api/embed/github/start/route.ts (1)
26-37: Setsecure: trueon OAuth helper cookies in production.The callback route conditionally sets
securebased on the request protocol; these helper cookies should follow the same pattern so the OAuthstateandreturnToare not transmitted over plain HTTP in mixed environments.♻️ Proposed fix
+ const secure = req.nextUrl.protocol === "https:"; res.cookies.set("embed_oauth_state", state, { httpOnly: true, sameSite: "lax", + secure, path: "/", maxAge: 600 }); res.cookies.set("embed_oauth_return", returnTo, { httpOnly: true, sameSite: "lax", + secure, path: "/", maxAge: 600 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/embed/github/start/route.ts` around lines 26 - 37, The helper cookies "embed_oauth_state" and "embed_oauth_return" are missing the secure flag; update the two res.cookies.set calls that set those cookies so they include secure: true when the request is secure (same condition used in the callback route), e.g. mirror the protocol-check logic used elsewhere in this file to compute a boolean (request.secure or protocol === "https") and pass secure: isSecure to both res.cookies.set invocations so the cookies are only sent over HTTPS in mixed/production environments.src/lib/embedNorthstarChat.ts (1)
33-50:parseStepsis brittle to common LLM formatting drift.Despite the system prompt forbidding code fences, Gemini occasionally emits
```json ... ```or trailing prose after the JSON array, in which caseJSON.parsethrows andstepssilently falls back. A small amount of normalization would make this a lot more reliable:♻️ Proposed hardening
if (idx >= 0) { - const jsonPart = answer.slice(idx + "RUNBOOK_STEPS_JSON:".length).trim(); + let jsonPart = answer + .slice(idx + "RUNBOOK_STEPS_JSON:".length) + .trim() + .replace(/^```(?:json)?\s*/i, "") + .replace(/```$/i, "") + .trim(); + // Keep only the first JSON array if the model added trailing prose. + const arrMatch = jsonPart.match(/\[[\s\S]*\]/); + if (arrMatch) jsonPart = arrMatch[0]; answer = answer.slice(0, idx).trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/embedNorthstarChat.ts` around lines 33 - 50, The parseSteps function is brittle to LLM formatting drift: modify parseSteps to normalize jsonPart before JSON.parse by removing surrounding code fences and optional language tags (e.g., leading ```json or ```), trimming trailing closing fences, and extracting only the first JSON array using a regex like /\[[\s\S]*?\]/ so any trailing prose is ignored; then parse that cleaned array and proceed with the existing Array.isArray and string filtering logic, keeping the existing try/catch behavior for safety.src/app/api/embed/github/repos/route.ts (1)
19-27: Silent truncation at 500 repos; consideroctokit.paginate.The hard cap of 5 pages × 100 silently drops anything beyond 500 repositories with no signal to the caller. Users with many repos would never see older ones (mitigated only by
sort: "updated").octokit.paginate(octokit.repos.listForAuthenticatedUser, { per_page: 100, sort: "updated" })is the idiomatic Octokit pattern and avoids the manual loop; if you want to keep a cap, return atruncated: trueflag so the UI can warn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/embed/github/repos/route.ts` around lines 19 - 27, The current manual paging loop that collects into repos using octokit.repos.listForAuthenticatedUser with a fixed 5-page cap silently truncates results beyond 500 items; replace the loop with octokit.paginate(octokit.repos.listForAuthenticatedUser, { per_page: 100, sort: "updated" }) to fetch all pages reliably and map each item into the existing repos array (preserving full_name and default_branch), or if you intentionally want a cap keep the manual loop but add a truncated: true boolean in the response when you hit the cap so callers can surface a warning.src/lib/embedIndexer.ts (1)
168-219: Per-chunk sequential embedding will be the long pole.Each chunk awaits
generateEmbeddingthen awaits the Supabase upsert before the next chunk starts. With up to 80 files × multiple chunks each, latency dominates — this is the main reason the indexing call is at risk of timing out on the request thread (see the related comment on the route). If/when you move this to a background job, batching embeddings (Gemini supports batchembedContent) andPromise.all-bounded concurrency for upserts (e.g., 4–8 in flight) will cut runtime by an order of magnitude. Optional for now if reliability is addressed first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/embedIndexer.ts` around lines 168 - 219, The loop currently calls generateEmbedding and then performs the Supabase upsert sequentially per chunk (see chunkContent, generateEmbedding, and supabaseAdmin.from("runbook_documents").upsert), causing high latency; refactor to collect chunks per file (or across files), call Gemini's batch embedding API (e.g., embedContent) once for many contents to get embeddings in bulk, then upsert rows concurrently with a bounded concurrency pool (Promise.all with a limiter such as p-limit set to ~4–8) instead of awaiting each upsert serially; ensure you preserve external_id/title/content construction (externalId, titled, content) and error handling for failed embeddings/upserts and accumulate errors as before.README.md (1)
19-22: Optional copy polish.Static analysis flagged the three consecutive “Open …” sentences in the demo flow. Trivial and safe to ignore, but a small variation (e.g., “Visit
/studio…”, “Then go to/embed-demo…”) reads a bit better.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 19 - 22, Change the three consecutive "Open …" lines to vary phrasing for better flow: update the lines referencing `/`, `/studio`, and `/embed-demo` (and the Runbook example) to use mixed verbs such as "Visit `/` — marketing landing.", "Go to `/studio` — knowledge sources (...)", and "Then open `/embed-demo` — sample 'Northstar' docs page..." so the sequence reads less repetitive while keeping the same destinations and descriptions.src/app/api/embed/chat/route.ts (1)
187-196: Step parsing diverges fromparseStepsinembedNorthstarChat.
embedNorthstarChat.parseStepsfilters empty strings and trims each step; this route only filters bytypeof x === "string", so blank/whitespace entries from the model can leak through and render as empty<li>s. Align the two paths so the demo and keyed routes produce the same shape.♻️ Proposed alignment
- if (Array.isArray(parsed)) { - steps = parsed.filter((x): x is string => typeof x === "string"); - } + if (Array.isArray(parsed)) { + steps = parsed + .filter((x): x is string => typeof x === "string" && x.trim().length > 0) + .map((s) => s.trim()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/embed/chat/route.ts` around lines 187 - 196, The route's step-parsing currently only checks typeof on parsed values, allowing blank/whitespace strings to pass; update the parsing in route.ts to mirror embedNorthstarChat.parseSteps by trimming each string and filtering out empty results (e.g., call .map(s => s.trim()) and then filter(Boolean) or equivalent) so that variable steps contains only non-empty trimmed strings and matches the demo/keyed behavior.public/runbook-embed.js (1)
230-243: Rename thebodypayload variable to avoid shadowing the message-list element.The outer
body(line 144) is the chat container DOM node used byaddBot/addUser. Declaringvar body = { … }here is function-scoped todoSend, so behavior is correct, but the name collision is a footgun if anyone later moves logic between scopes or refactors to ESM/let. A trivial rename removes the trap.♻️ Proposed rename
- var body = { + var payload = { projectId: projectId, message: q, pageContext: pageContext() }; if (projectId === DEMO_ID) { var custom = manualSourcesFromBundle(bundle); - if (custom.length) body.customSources = custom; + if (custom.length) payload.customSources = custom; } var res = await fetch(base + "/api/embed/chat", { method: "POST", headers: headers, - body: JSON.stringify(body) + body: JSON.stringify(payload) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/runbook-embed.js` around lines 230 - 243, The request payload object named "body" inside doSend shadows the outer DOM variable used as the chat container (message-list) and risks future refactor bugs; rename this local payload (e.g., to "payload" or "reqBody") wherever it's created and used — update the var body = { ... } to var payload = { projectId: projectId, message: q, pageContext: pageContext() }, change any conditional assignment body.customSources to payload.customSources, and change JSON.stringify(body) to JSON.stringify(payload) in the fetch to base + "/api/embed/chat"; keep other logic (manualSourcesFromBundle, addBot/addUser, headers) unchanged.src/lib/embedStore.ts (2)
26-29:writeJsonis non-atomic — a crash mid-write can leave a corrupt JSON file.
fs.writeFiletruncates and streams; an interrupted process can leaveembed-projects.json/embed-api-keys.jsonhalf-written, which then breaks every subsequentreadJson. Write to a temp file in the same directory andrenameto swap atomically.♻️ Proposed fix
async function writeJson<T>(file: string, data: T): Promise<void> { await ensureDir(); - await fs.writeFile(file, JSON.stringify(data, null, 2), "utf-8"); + const tmp = `${file}.${process.pid}.${Date.now()}.tmp`; + await fs.writeFile(tmp, JSON.stringify(data, null, 2), "utf-8"); + await fs.rename(tmp, file); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/embedStore.ts` around lines 26 - 29, The writeJson function is non-atomic and can leave corrupt JSON if interrupted; modify writeJson<T>(file: string, data: T) to first ensureDir(), then write the JSON to a temporary file in the same directory (e.g., using path.dirname(file) + a unique temp suffix or pid/random), fsync/close the temp file if you use low-level handles, and finally fs.rename(tempFile, file) to atomically replace the target (this keeps existing permissions and ensures embed-projects.json / embed-api-keys.json are never half-written); keep ensureDir() as-is and reference writeJson when implementing the temp->rename flow.
6-15: File-backed persistence won’t survive a serverless deployment.Writing JSON under
process.cwd()/.runbook-dataworks locally but breaks the moment this ships to Vercel/Lambda/Cloud Run: the working dir is read-only (only/tmpis writable, and only per-instance/ephemeral), andrunMutationis process-local — two warm Lambdas writing concurrently will race and clobber each other regardless of the queue. For the demo this is acceptable; before any production embed onboarding, plan to back this with a real store (Supabase/Postgres, Upstash, KV, etc.). Calling out so this doesn’t silently regress when promoted from the demo path.Also applies to: 39-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/embedStore.ts` around lines 6 - 15, The current file-backed persistence using DATA_DIR = path.join(process.cwd(), ".runbook-data") will fail in serverless/read-only environments and race across instances; change DATA_DIR to be configurable via an environment variable (e.g., process.env.EMBED_DATA_DIR) and fall back to the system temp directory (require('os').tmpdir()) so the code using PROJECTS_FILE / KEYS_FILE / TOKENS_FILE and functions like ensureDir and the mutationQueue still work locally for the demo but won’t try to write into a read-only CWD; also add a TODO comment in this module to replace the file-backed store with a real external store (Supabase/Postgres/Upstash/KV) before production deployment.src/components/EmbeddedRunbookAssistant.tsx (2)
82-100:dangerouslySetInnerHTMLis fine here, but lock downescapeHtmlfor future-proofing.ast-grep flags line 181, but every dynamic value flowing in is wrapped with
escapeHtml, and all of them land inside element-text positions (no attribute interpolation), so this is currently safe. That said,escapeHtmldoesn’t escape'— if anyone later puts an escaped value inside a single-quoted attribute, it becomes XSS. Add'(and`) for defense in depth.🛡️ Proposed fix
function escapeHtml(s: string): string { return s .replace(/&/g, "&") .replace(/</g, "<") .replace(/>/g, ">") - .replace(/"/g, """); + .replace(/"/g, """) + .replace(/'/g, "'") + .replace(/`/g, "`"); }Also applies to: 217-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/EmbeddedRunbookAssistant.tsx` around lines 82 - 100, The injected HTML in EmbeddedRunbookAssistant.tsx currently relies on escapeHtml but that function does not escape single-quote (') or backtick (`), which could allow XSS if values are later used inside single-quoted attributes; update the escapeHtml implementation (the shared escapeHtml helper used by the HTML-building block that appends to setMessages in EmbeddedRunbookAssistant.tsx) to also replace ' with ' and ` with ` (in addition to &, <, >, " ) so all dynamic values inserted into the template strings are fully escaped; ensure the same updated escapeHtml is used for the other usages noted (around the other HTML-building block mentioned in the comment).
67-76:pageContextdiffers frompublic/runbook-embed.js.The shipped widget includes
<meta name="description">content inpageContext; this in-app preview only sends URL +document.title. If you’re using this component to validate Studio-configured behavior, the model will see less context here than in the production embed and produce divergent answers between preview and live.♻️ Proposed fix
- pageContext: typeof window !== "undefined" ? `${window.location.href}\n${document.title}` : "", + pageContext: + typeof window !== "undefined" + ? [ + window.location.href, + document.title, + document + .querySelector('meta[name="description"]') + ?.getAttribute("content") || "" + ] + .filter(Boolean) + .join("\n") + : "",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/EmbeddedRunbookAssistant.tsx` around lines 67 - 76, The pageContext sent from EmbeddedRunbookAssistant.tsx to `${base}/api/embed/chat` currently includes only URL and document.title, which differs from the production embed and causes preview/live divergence; update the pageContext construction in the fetch body to also collect and include the page meta description (the content of <meta name="description">) — locate the pageContext creation near the fetch call in EmbeddedRunbookAssistant.tsx and concatenate the meta description (if present) along with window.location.href and document.title before JSON.stringify so the API receives the same context as public/runbook-embed.js.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Line 30: Add a trailing newline at the end of the .env.example file so the
final line "RUNBOOK_EMBED_SESSION_SECRET=" is terminated with a newline
character; this resolves the dotenv-linter EndingBlankLine warning for the
RUNBOOK_EMBED_SESSION_SECRET entry.
In `@public/runbook-embed.js`:
- Around line 38-40: Replace the unused catch binding "catch (e) { return null;
}" with an optional catch binding "catch { return null; }" in the two locations
where that pattern appears (the catch blocks currently declaring variable "e");
this removes the unused-variable warnings from `@typescript-eslint/no-unused-vars`
while preserving behavior—ensure both occurrences are updated to use "catch {
... }".
In `@src/app/api/embed/chat/route.ts`:
- Around line 26-29: The originAllowed function uses naive substring checks that
are exploitable; update originAllowed to parse both inputs as URLs (or extract
canonical hostnames), normalize to lowercase and strip ports, then compare
either exact hostname equality or perform a suffix match anchored on dot
boundaries (e.g., ensure projectSiteHost === originHost or originHost
endsWith('.' + projectSiteHost)) instead of using String.includes; this ensures
attacks like "evil-example.com.attacker.com" do not pass.
- Around line 94-106: The current demo uses a single global bucket via
checkEmbedRateLimit("northstar-demo-public"), allowing one noisy client to
exhaust the quota; change the call to scope rate limits per remote address by
extracting the caller IP (use the first hop of the x-forwarded-for header or
request.ip as available) and pass a key like
`northstar-demo-public:${remoteAddr}` to checkEmbedRateLimit; ensure you
fallback to the original static key if no remote address is found and keep the
rest of the flow (sanitizeCustomSources, runNorthstarEmbedChat,
NextResponse.json, corsHeaders) unchanged.
In `@src/app/api/embed/github/callback/route.ts`:
- Line 74: The redirect logic using returnTo allows protocol-relative URLs like
//attacker.com because returnTo.startsWith("/") returns true; update the
callback handler that computes dest (the returnTo variable and new URL(...)
usage in route.ts) to first reject inputs with backslashes or leading
double-slashes (e.g., /^\\|^\/\/+/), then parse returnTo with new URL(returnTo,
req.nextUrl.origin) and verify the resulting URL.origin === req.nextUrl.origin
before honoring it; if any check fails, fall back to the safe default (new
URL("/studio", req.nextUrl.origin)).
- Around line 55-61: The GitHub API calls are missing a required User-Agent
header and the JSON parsing of tokenRes and userRes can throw on non-JSON
responses; update the fetch options used when exchanging the token (tokenRes)
and when calling the user endpoint (userRes) to include a User-Agent header
(e.g., "User-Agent": "your-app-name"), and guard the JSON parsing for tokenJson
and user by checking response.ok and wrapping await tokenRes.json() and await
userRes.json() in try/catch (or conditional parsing) so failures fall back to
the existing safe redirect path instead of throwing; locate these changes around
tokenRes, tokenJson, userRes, and user in the route handler and ensure the error
paths use the same redirect logic already implemented.
In `@src/app/api/embed/github/start/route.ts`:
- Line 17: Validate the returnTo value obtained from
req.nextUrl.searchParams.get("return") before using it: in the route handler
where returnTo is assigned, reject and fallback to the default "/studio" if the
value starts with "//", contains "\" (backslash), or parses as an absolute URL
(has a scheme/host); i.e., perform a defensive check on returnTo and only allow
a safe relative path, otherwise set returnTo = "/studio" so the malicious value
never reaches the cookie or callback.
In `@src/app/api/embed/projects/`[projectId]/index/route.ts:
- Around line 38-48: The current POST handler calls indexGitHubRepo
synchronously, which can block the request thread and cause timeouts and
concurrent runs; change the route handler to enqueue a background job (or call
an async worker/queue) instead of awaiting indexGitHubRepo directly, return a
202 Accepted immediately from the route, and implement a per-project running
guard that checks and sets project.lastIndexStatus === "running" (the same
status checked/updated in embedIndexer.ts around where lastIndexStatus is set)
before enqueuing, rejecting new starts if it is already running; ensure the
worker pulls the job and invokes indexGitHubRepo(project, gh.accessToken) and
updates lastIndexStatus on completion/failure so deletes/upserts and embeddings
happen off-request with proper locking.
In `@src/app/api/embed/projects/route.ts`:
- Around line 26-40: The code currently accepts siteUrl raw and persists it via
createProject (see siteUrl variable and createProject call in route.ts), which
allows unsafe schemes (e.g., javascript:, data:) leading to XSS when rendered as
links; validate siteUrl after trimming by parsing or checking its scheme and
only allow empty or http/https (e.g., new URL(siteUrl).protocol === 'http:' ||
'https:') and return a 400 JSON error if the scheme is disallowed, then pass
siteUrl || undefined to createProject as before.
In `@src/app/embed-demo/page.tsx`:
- Line 8: The state initializer currently calls loadDemoBundle() during SSR and
client render causing hydration mismatch; change the useState call to initialize
with DEFAULT_DEMO_BUNDLE (not loadDemoBundle), then in a useEffect invoke
loadDemoBundle() and call setBundle(...) with its result so the real stored
bundle is loaded only on the client; update the code around
useState<DemoBundle>(() => loadDemoBundle()) and add a useEffect that runs once
to replace the bundle for EmbeddedRunbookAssistant props.
In `@src/app/studio/page.tsx`:
- Around line 30-39: The effect in useEffect currently calls multiple setState
functions (setOrigin, setAssistantName, setWelcome, setPrimaryColor,
setSuggestedRaw, setManualSources, setHydrated) which triggers the eslint
react-hooks/set-state-in-effect error; fix by batching these values into a
single state update (e.g., create a boot state like boot and setBoot({ origin,
assistantName, welcome, primaryColor, suggestedRaw, manualSources, hydrated:
true }) and update component usages to read from boot or provide per-field
setters that spread into setBoot), or if you prefer the minimal change add a
localized eslint-disable-next-line comment above the useEffect with a brief
rationale mentioning window/localStorage hydration is post-mount and
intentional; update references to loadDemoBundle(), setSuggestedRaw logic, and
usages of origin/assistantName/etc. accordingly so only one setter is called
inside the effect.
In `@src/lib/embedDemoKnowledge.ts`:
- Around line 16-18: The function excerptFromContent currently normalizes
whitespace with content.replace(...).trim() for slicing but compares the
original content.length to max, causing incorrect ellipses; change
excerptFromContent to first compute a single normalized string (e.g., let
normalized = content.replace(/\s+/g, " ").trim()), then use normalized.slice(0,
max) and check normalized.length > max to decide whether to append "…", ensuring
both slicing and the ellipsis decision use the same normalized text.
In `@src/lib/embedIndexer.ts`:
- Around line 76-84: The current deleteExistingEmbedDocs function deletes all
existing project chunks before writing new ones; change the flow so you
upsert/write new chunks first (use
supabaseAdmin.from("runbook_documents").upsert(...) or insert with on_conflict)
and only afterwards delete stale rows for the project by deleting rows whose
external_id starts with the project prefix but is NOT in the set of external_ids
you just wrote; implement this in deleteExistingEmbedDocs (or a new cleanup
function) by accepting the array of newly-written external_ids, skipping
deletion entirely if that array is empty (to avoid wiping on partial failures),
and using a NOT IN filter (e.g., .not("external_id", "in", "(id1,id2,...)") or
equivalent) to remove only stale rows; apply same change to the other similar
delete usage mentioned.
- Around line 200-208: The fallback insert after a failed upsert in
embedIndexer.ts is incorrect: if
supabaseAdmin.from("runbook_documents").upsert(row, { onConflict:
"provider,external_id" }) returns an error it won’t be fixed by re-running
insert (unique conflict or other issues persist); change the logic in the block
handling the upsert result so that on error you record the error (push to errors
with insErr.message or error.message) and do not attempt the duplicate insert,
or implement a proper retry/backoff for transient failures; update the uses of
variables row, stored, errors and the upsert/insert call sites so stored is only
set true on successful upsert/insert and ensure the logged error comes from the
original upsert error when present.
In `@src/lib/embedNorthstarChat.ts`:
- Around line 57-62: The code maps all entries in input.customSources into
extraDocs without limiting the number of items, which lets a caller submit
arbitrarily many docs and exhaust CPU/LLM tokens; before mapping, cap the array
(e.g. const capped = input.customSources.slice(0, 32)) and map capped instead of
input.customSources so extraDocs has a fixed upper bound; update any downstream
references (retrieveKeywordSources and prompt-building that consume extraDocs)
to use the capped list and document the chosen max items constant near the
extraDocs creation.
In `@src/lib/embedRateLimit.ts`:
- Around line 3-19: The in-memory Map-based limiter (buckets, WINDOW_MS,
MAX_PER_WINDOW, and function checkEmbedRateLimit) causes per-instance limits and
unbounded growth; replace the module-scoped Map with a shared store (e.g.,
Upstash/Redis) using INCR + EXPIRE per keyId to enforce a global MAX_PER_WINDOW
across instances and correct TTL-based resets, and if you must keep a local
fallback keep a bounded Map size (e.g., MAX_BUCKETS constant) and implement
opportunistic eviction in checkEmbedRateLimit to prune expired entries and
remove oldest/newest entries when the cap is exceeded to avoid memory leaks.
In `@src/lib/embedRetrieval.ts`:
- Around line 35-50: The current call to supabaseAdmin.rpc("match_documents",
...) requests TOP_K * 8 candidates and then filters client-side via
matchesEmbedScope(d.content, projectId), which can drop all results if
other-project docs dominate; fix by either (preferred) extending the
match_documents RPC to accept a project_id (or embed scope) and apply the filter
in SQL and then call it from this code (update the rpc parameters where
match_documents is invoked), or (stopgap) implement paging: repeatedly call
supabaseAdmin.rpc("match_documents", { query_embedding:
`[${embedding.join(",")}]`, match_threshold: DEFAULT_MATCH_THRESHOLD,
match_count: pageSize, page_offset: offset }) aggregating unique documents,
apply matchesEmbedScope(projectId) to the accumulated set, and stop when
scoped.length >= TOP_K or a safe max iterations is reached, then sort by
similarity and return the top TOP_K; ensure you update uses of TOP_K,
match_count, match_documents, matchesEmbedScope, embedding, and projectId
accordingly.
In `@src/lib/embedSession.ts`:
- Around line 5-11: The sessionSecret() function must not silently fall back to
a public hardcoded secret or reuse SUPABASE_SERVICE_ROLE_KEY in production;
change it so it returns RUNBOOK_EMBED_SESSION_SECRET if present, and otherwise
throws (fail-closed) when not running in development (e.g., NODE_ENV !==
'development' or VERCEL_ENV !== 'development'); allow an explicit, documented
fallback only for local/dev runs (optionally use SUPABASE_SERVICE_ROLE_KEY or
the dev literal only when NODE_ENV === 'development') and add a clear error
message referencing sessionSecret() so the app fails to start if the dedicated
env is missing in non-dev environments.
---
Nitpick comments:
In `@public/runbook-embed.js`:
- Around line 230-243: The request payload object named "body" inside doSend
shadows the outer DOM variable used as the chat container (message-list) and
risks future refactor bugs; rename this local payload (e.g., to "payload" or
"reqBody") wherever it's created and used — update the var body = { ... } to var
payload = { projectId: projectId, message: q, pageContext: pageContext() },
change any conditional assignment body.customSources to payload.customSources,
and change JSON.stringify(body) to JSON.stringify(payload) in the fetch to base
+ "/api/embed/chat"; keep other logic (manualSourcesFromBundle, addBot/addUser,
headers) unchanged.
In `@README.md`:
- Around line 19-22: Change the three consecutive "Open …" lines to vary
phrasing for better flow: update the lines referencing `/`, `/studio`, and
`/embed-demo` (and the Runbook example) to use mixed verbs such as "Visit `/` —
marketing landing.", "Go to `/studio` — knowledge sources (...)", and "Then open
`/embed-demo` — sample 'Northstar' docs page..." so the sequence reads less
repetitive while keeping the same destinations and descriptions.
In `@src/app/api/embed/chat/route.ts`:
- Around line 187-196: The route's step-parsing currently only checks typeof on
parsed values, allowing blank/whitespace strings to pass; update the parsing in
route.ts to mirror embedNorthstarChat.parseSteps by trimming each string and
filtering out empty results (e.g., call .map(s => s.trim()) and then
filter(Boolean) or equivalent) so that variable steps contains only non-empty
trimmed strings and matches the demo/keyed behavior.
In `@src/app/api/embed/github/repos/route.ts`:
- Around line 19-27: The current manual paging loop that collects into repos
using octokit.repos.listForAuthenticatedUser with a fixed 5-page cap silently
truncates results beyond 500 items; replace the loop with
octokit.paginate(octokit.repos.listForAuthenticatedUser, { per_page: 100, sort:
"updated" }) to fetch all pages reliably and map each item into the existing
repos array (preserving full_name and default_branch), or if you intentionally
want a cap keep the manual loop but add a truncated: true boolean in the
response when you hit the cap so callers can surface a warning.
In `@src/app/api/embed/github/start/route.ts`:
- Around line 26-37: The helper cookies "embed_oauth_state" and
"embed_oauth_return" are missing the secure flag; update the two res.cookies.set
calls that set those cookies so they include secure: true when the request is
secure (same condition used in the callback route), e.g. mirror the
protocol-check logic used elsewhere in this file to compute a boolean
(request.secure or protocol === "https") and pass secure: isSecure to both
res.cookies.set invocations so the cookies are only sent over HTTPS in
mixed/production environments.
In `@src/app/api/embed/me/route.ts`:
- Around line 6-16: The GET handler currently returns a 401 when
verifyEmbedSession(req.cookies.get("embed_session")?.value) yields no session;
change this to return a 200 with { authenticated: false } instead so
/api/embed/me behaves as a probe rather than an auth-gated resource—update the
early return in the GET function that uses NextResponse.json(...) to use a 200
status (or omit the status) while keeping the same response body and leave the
authenticated:true branch unchanged.
In `@src/components/EmbeddedRunbookAssistant.tsx`:
- Around line 82-100: The injected HTML in EmbeddedRunbookAssistant.tsx
currently relies on escapeHtml but that function does not escape single-quote
(') or backtick (`), which could allow XSS if values are later used inside
single-quoted attributes; update the escapeHtml implementation (the shared
escapeHtml helper used by the HTML-building block that appends to setMessages in
EmbeddedRunbookAssistant.tsx) to also replace ' with ' and ` with ` (in
addition to &, <, >, " ) so all dynamic values inserted into the template
strings are fully escaped; ensure the same updated escapeHtml is used for the
other usages noted (around the other HTML-building block mentioned in the
comment).
- Around line 67-76: The pageContext sent from EmbeddedRunbookAssistant.tsx to
`${base}/api/embed/chat` currently includes only URL and document.title, which
differs from the production embed and causes preview/live divergence; update the
pageContext construction in the fetch body to also collect and include the page
meta description (the content of <meta name="description">) — locate the
pageContext creation near the fetch call in EmbeddedRunbookAssistant.tsx and
concatenate the meta description (if present) along with window.location.href
and document.title before JSON.stringify so the API receives the same context as
public/runbook-embed.js.
In `@src/lib/embedIndexer.ts`:
- Around line 168-219: The loop currently calls generateEmbedding and then
performs the Supabase upsert sequentially per chunk (see chunkContent,
generateEmbedding, and supabaseAdmin.from("runbook_documents").upsert), causing
high latency; refactor to collect chunks per file (or across files), call
Gemini's batch embedding API (e.g., embedContent) once for many contents to get
embeddings in bulk, then upsert rows concurrently with a bounded concurrency
pool (Promise.all with a limiter such as p-limit set to ~4–8) instead of
awaiting each upsert serially; ensure you preserve external_id/title/content
construction (externalId, titled, content) and error handling for failed
embeddings/upserts and accumulate errors as before.
In `@src/lib/embedKeywordRetrieval.ts`:
- Around line 23-31: The scoring code inside the docs.map that builds scored is
unclear and inefficient: replace the expression using hay.split(t).length - 1 +
2 with an explicit occurrence count and a separate per-term bonus so the intent
(count occurrences + flat +2 bonus per matched term) is obvious; inside the map
over docs compute occurrences for each term using a simple indexOf loop or a
global RegExp to count matches (or use word-boundary RegExp /\bterm\b/ if
stricter matching is desired), add occurrences to score and then add the +2
bonus only when occurrences > 0, and keep the title-word exact match bonus logic
(the doc.title.toLowerCase().split...) unchanged.
In `@src/lib/embedNorthstarChat.ts`:
- Around line 33-50: The parseSteps function is brittle to LLM formatting drift:
modify parseSteps to normalize jsonPart before JSON.parse by removing
surrounding code fences and optional language tags (e.g., leading ```json or
```), trimming trailing closing fences, and extracting only the first JSON array
using a regex like /\[[\s\S]*?\]/ so any trailing prose is ignored; then parse
that cleaned array and proceed with the existing Array.isArray and string
filtering logic, keeping the existing try/catch behavior for safety.
In `@src/lib/embedStore.ts`:
- Around line 26-29: The writeJson function is non-atomic and can leave corrupt
JSON if interrupted; modify writeJson<T>(file: string, data: T) to first
ensureDir(), then write the JSON to a temporary file in the same directory
(e.g., using path.dirname(file) + a unique temp suffix or pid/random),
fsync/close the temp file if you use low-level handles, and finally
fs.rename(tempFile, file) to atomically replace the target (this keeps existing
permissions and ensures embed-projects.json / embed-api-keys.json are never
half-written); keep ensureDir() as-is and reference writeJson when implementing
the temp->rename flow.
- Around line 6-15: The current file-backed persistence using DATA_DIR =
path.join(process.cwd(), ".runbook-data") will fail in serverless/read-only
environments and race across instances; change DATA_DIR to be configurable via
an environment variable (e.g., process.env.EMBED_DATA_DIR) and fall back to the
system temp directory (require('os').tmpdir()) so the code using PROJECTS_FILE /
KEYS_FILE / TOKENS_FILE and functions like ensureDir and the mutationQueue still
work locally for the demo but won’t try to write into a read-only CWD; also add
a TODO comment in this module to replace the file-backed store with a real
external store (Supabase/Postgres/Upstash/KV) before production deployment.
In `@src/lib/prompts.ts`:
- Around line 1-8: Update the EMBED_CHAT_SYSTEM_PROMPT constant to explicitly
document the RUNBOOK_STEPS_JSON output contract: state that after the answer the
assistant must append a final line starting with RUNBOOK_STEPS_JSON: followed by
a JSON array of 3–6 imperative steps (or [] if none). Modify the string in
EMBED_CHAT_SYSTEM_PROMPT so it includes that exact instruction text, ensuring
consistency with the user message template and parsing logic used in route
handler (referencing EMBED_CHAT_SYSTEM_PROMPT and RUNBOOK_STEPS_JSON to locate
where to change).
In `@src/lib/studioDemoStorage.ts`:
- Around line 59-67: The current saveDemoBundle function silently swallows
storage quota errors; update saveDemoBundle to surface failures by changing its
signature to return a boolean (true on success, false on failure) and in the
catch block log the error with console.warn including the caught error and the
RUNBOOK_DEMO_BUNDLE_KEY (or rethrow if caller expects exceptions). Keep the
existing behavior of setting localStorage and dispatching the
"runbook-demo-update" event on success, but on failure ensure you log the full
error (e.g., mentioning QuotaExceededError) and return false so callers can show
a toast or handle the failure.
🪄 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: 73f2e324-1c7c-4589-8ef9-d3043be412a0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
.env.exampleREADME.mdpackage.jsonpublic/runbook-embed.jssrc/app/api/embed/chat/route.tssrc/app/api/embed/github/callback/route.tssrc/app/api/embed/github/repos/route.tssrc/app/api/embed/github/start/route.tssrc/app/api/embed/me/route.tssrc/app/api/embed/projects/[projectId]/index/route.tssrc/app/api/embed/projects/[projectId]/route.tssrc/app/api/embed/projects/route.tssrc/app/embed-demo/page.tsxsrc/app/layout.tsxsrc/app/page.tsxsrc/app/studio/page.tsxsrc/components/EmbeddedRunbookAssistant.tsxsrc/lib/embedDemoKnowledge.tssrc/lib/embedIndexer.tssrc/lib/embedKeywordRetrieval.tssrc/lib/embedNorthstarChat.tssrc/lib/embedRateLimit.tssrc/lib/embedRetrieval.tssrc/lib/embedSession.tssrc/lib/embedStore.tssrc/lib/embedTypes.tssrc/lib/prompts.tssrc/lib/studioDemoStorage.ts
| # Must match the Authorization callback URL in the GitHub OAuth App (e.g. http://127.0.0.1:3000/api/embed/github/callback) | ||
| GITHUB_OAUTH_REDIRECT_URL= | ||
| # HMAC secret for signed HttpOnly `embed_session` cookie (generate a long random string) | ||
| RUNBOOK_EMBED_SESSION_SECRET= No newline at end of file |
There was a problem hiding this comment.
Add a trailing newline.
dotenv-linter flagged EndingBlankLine. Easy fix:
# HMAC secret for signed HttpOnly `embed_session` cookie (generate a long random string)
RUNBOOK_EMBED_SESSION_SECRET=
+📝 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
| RUNBOOK_EMBED_SESSION_SECRET= | |
| # HMAC secret for signed HttpOnly `embed_session` cookie (generate a long random string) | |
| RUNBOOK_EMBED_SESSION_SECRET= | |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 30-30: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example at line 30, Add a trailing newline at the end of the
.env.example file so the final line "RUNBOOK_EMBED_SESSION_SECRET=" is
terminated with a newline character; this resolves the dotenv-linter
EndingBlankLine warning for the RUNBOOK_EMBED_SESSION_SECRET entry.
Comment on lines
+26
to
+29
| function originAllowed(projectSite: string | undefined, origin: string | null): boolean { | ||
| if (!projectSite || !origin) return true; | ||
| return origin.includes(projectSite) || projectSite.includes(origin.replace(/^https?:\/\//, "")); | ||
| } |
There was a problem hiding this comment.
Origin allowlist substring match is exploitable.
originAllowed uses naive String.includes in both directions. An attacker-controlled origin like https://evil-example.com.attacker.com will satisfy origin.includes("example.com") and pass the check, defeating the per-project origin restriction. Parse both values to a canonical hostname and compare with strict equality (or a suffix match anchored on . boundaries).
🔒 Proposed fix
-function originAllowed(projectSite: string | undefined, origin: string | null): boolean {
- if (!projectSite || !origin) return true;
- return origin.includes(projectSite) || projectSite.includes(origin.replace(/^https?:\/\//, ""));
-}
+function hostFrom(value: string): string | null {
+ try {
+ return new URL(value.includes("://") ? value : `https://${value}`).hostname.toLowerCase();
+ } catch {
+ return null;
+ }
+}
+
+function originAllowed(projectSite: string | undefined, origin: string | null): boolean {
+ if (!projectSite || !origin) return true;
+ const allowed = hostFrom(projectSite);
+ const got = hostFrom(origin);
+ if (!allowed || !got) return false;
+ return got === allowed || got.endsWith(`.${allowed}`);
+}📝 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
| function originAllowed(projectSite: string | undefined, origin: string | null): boolean { | |
| if (!projectSite || !origin) return true; | |
| return origin.includes(projectSite) || projectSite.includes(origin.replace(/^https?:\/\//, "")); | |
| } | |
| function hostFrom(value: string): string | null { | |
| try { | |
| return new URL(value.includes("://") ? value : `https://${value}`).hostname.toLowerCase(); | |
| } catch { | |
| return null; | |
| } | |
| } | |
| function originAllowed(projectSite: string | undefined, origin: string | null): boolean { | |
| if (!projectSite || !origin) return true; | |
| const allowed = hostFrom(projectSite); | |
| const got = hostFrom(origin); | |
| if (!allowed || !got) return false; | |
| return got === allowed || got.endsWith(`.${allowed}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/embed/chat/route.ts` around lines 26 - 29, The originAllowed
function uses naive substring checks that are exploitable; update originAllowed
to parse both inputs as URLs (or extract canonical hostnames), normalize to
lowercase and strip ports, then compare either exact hostname equality or
perform a suffix match anchored on dot boundaries (e.g., ensure projectSiteHost
=== originHost or originHost endsWith('.' + projectSiteHost)) instead of using
String.includes; this ensures attacks like "evil-example.com.attacker.com" do
not pass.
Comment on lines
+94
to
+106
| if (projectId === NORTHSTAR_DEMO_PROJECT_ID) { | ||
| const rl = checkEmbedRateLimit("northstar-demo-public"); | ||
| if (!rl.ok) { | ||
| return NextResponse.json( | ||
| { error: "Rate limited", retryAfter: rl.retryAfter }, | ||
| { status: 429, headers: { ...corsHeaders(origin), "Retry-After": String(rl.retryAfter) } } | ||
| ); | ||
| } | ||
|
|
||
| const customSources = sanitizeCustomSources(body.customSources); | ||
| const payload = await runNorthstarEmbedChat({ message, pageContext, customSources }); | ||
| return NextResponse.json(payload, { headers: corsHeaders(origin) }); | ||
| } |
There was a problem hiding this comment.
Public demo rate limit is shared by every visitor.
checkEmbedRateLimit("northstar-demo-public") uses one global bucket, so a single noisy client can exhaust the quota and 429 every other demo user (the comment even claims it’s “IP-ish” but no IP is mixed in). Bucket per remote address (e.g. x-forwarded-for first hop) so this scales by caller.
🛡️ Proposed fix
- if (projectId === NORTHSTAR_DEMO_PROJECT_ID) {
- const rl = checkEmbedRateLimit("northstar-demo-public");
+ if (projectId === NORTHSTAR_DEMO_PROJECT_ID) {
+ const ip =
+ (req.headers.get("x-forwarded-for") || "").split(",")[0]?.trim() ||
+ req.headers.get("x-real-ip") ||
+ "anon";
+ const rl = checkEmbedRateLimit(`northstar-demo:${ip}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/embed/chat/route.ts` around lines 94 - 106, The current demo uses
a single global bucket via checkEmbedRateLimit("northstar-demo-public"),
allowing one noisy client to exhaust the quota; change the call to scope rate
limits per remote address by extracting the caller IP (use the first hop of the
x-forwarded-for header or request.ip as available) and pass a key like
`northstar-demo-public:${remoteAddr}` to checkEmbedRateLimit; ensure you
fallback to the original static key if no remote address is found and keep the
rest of the flow (sanitizeCustomSources, runNorthstarEmbedChat,
NextResponse.json, corsHeaders) unchanged.
Comment on lines
+55
to
+61
| const userRes = await fetch("https://api.github.com/user", { | ||
| headers: { | ||
| Authorization: `Bearer ${tokenJson.access_token}`, | ||
| Accept: "application/vnd.github+json" | ||
| } | ||
| }); | ||
| const user = (await userRes.json()) as { id?: number; login?: string }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does GitHub's REST API require a User-Agent header for unauthenticated /user requests using a bearer token?
💡 Result:
Yes. GitHub’s REST API requires a valid User-Agent header on all API requests (including requests to GET /user made with a Bearer token). Requests with no User-Agent header are rejected with 403 Forbidden, regardless of whether you authenticate. Separately, whether you include the Bearer token affects rate limits: authenticated requests have much higher primary rate limits than unauthenticated ones.
Citations:
- 1: https://docs.github.com/en/rest/overview/resources-in-the-rest-api
- 2: https://docs.github.com/en/rest/using-the-rest-api/getting-started-with-the-rest-api
- 3: https://docs.github.com/en/rest/overview/media-types/
- 4: https://docs.github.com/en/rest/users/users
🏁 Script executed:
# First, let's check if the file exists and read the relevant sections
cat -n src/app/api/embed/github/callback/route.ts | head -70Repository: dfed25/RunBook
Length of output: 3090
Add missing User-Agent headers and harden JSON parsing against failures.
GitHub's REST API requires a User-Agent header on all requests; requests without one are rejected with 403 Forbidden. Both the token-exchange request (lines 40–47) and the user API request (lines 55–61) lack this header. Additionally, tokenRes.json() and userRes.json() will throw on non-JSON responses (e.g., 5xx HTML errors), causing unhandled errors instead of the safer redirect path used elsewhere in this handler.
♻️ Suggested hardening
const tokenRes = await fetch("https://github.com/login/oauth/access_token", {
method: "POST",
headers: {
Accept: "application/json",
- "Content-Type": "application/x-www-form-urlencoded"
+ "Content-Type": "application/x-www-form-urlencoded",
+ "User-Agent": "runbook-embed"
},
body: body.toString()
});
const userRes = await fetch("https://api.github.com/user", {
headers: {
Authorization: `Bearer ${tokenJson.access_token}`,
- Accept: "application/vnd.github+json"
+ Accept: "application/vnd.github+json",
+ "User-Agent": "runbook-embed"
}
});
- const user = (await userRes.json()) as { id?: number; login?: string };
+ let user: { id?: number; login?: string } = {};
+ try {
+ user = (await userRes.json()) as { id?: number; login?: string };
+ } catch {
+ return NextResponse.redirect(new URL("/studio?error=github_user", req.nextUrl.origin));
+ }Consider wrapping tokenRes.json() in a try-catch as well for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/embed/github/callback/route.ts` around lines 55 - 61, The GitHub
API calls are missing a required User-Agent header and the JSON parsing of
tokenRes and userRes can throw on non-JSON responses; update the fetch options
used when exchanging the token (tokenRes) and when calling the user endpoint
(userRes) to include a User-Agent header (e.g., "User-Agent": "your-app-name"),
and guard the JSON parsing for tokenJson and user by checking response.ok and
wrapping await tokenRes.json() and await userRes.json() in try/catch (or
conditional parsing) so failures fall back to the existing safe redirect path
instead of throwing; locate these changes around tokenRes, tokenJson, userRes,
and user in the route handler and ensure the error paths use the same redirect
logic already implemented.
Comment on lines
+200
to
+208
| const { error } = await supabaseAdmin.from("runbook_documents").upsert(row, { onConflict: "provider,external_id" }); | ||
| let stored = false; | ||
| if (error) { | ||
| const { error: insErr } = await supabaseAdmin.from("runbook_documents").insert(row); | ||
| if (insErr) errors.push(`${filePath}#${cIdx}: ${insErr.message}`); | ||
| else stored = true; | ||
| } else { | ||
| stored = true; | ||
| } |
There was a problem hiding this comment.
Fallback insert after a failed upsert won't recover the conflict case.
upsert(..., { onConflict: "provider,external_id" }) already handles the duplicate-key path by updating, so a non-null error here means something else went wrong (RLS, schema, network, embedding column shape, etc.). A plain insert of the same row will hit the same UNIQUE constraint and fail again. Either drop the fallback and just record the error, or replace it with a more meaningful retry/backoff:
♻️ Proposed simplification
- const { error } = await supabaseAdmin.from("runbook_documents").upsert(row, { onConflict: "provider,external_id" });
- let stored = false;
- if (error) {
- const { error: insErr } = await supabaseAdmin.from("runbook_documents").insert(row);
- if (insErr) errors.push(`${filePath}#${cIdx}: ${insErr.message}`);
- else stored = true;
- } else {
- stored = true;
- }
+ const { error } = await supabaseAdmin
+ .from("runbook_documents")
+ .upsert(row, { onConflict: "provider,external_id" });
+ const stored = !error;
+ if (error) errors.push(`${filePath}#${cIdx}: ${error.message}`);📝 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
| const { error } = await supabaseAdmin.from("runbook_documents").upsert(row, { onConflict: "provider,external_id" }); | |
| let stored = false; | |
| if (error) { | |
| const { error: insErr } = await supabaseAdmin.from("runbook_documents").insert(row); | |
| if (insErr) errors.push(`${filePath}#${cIdx}: ${insErr.message}`); | |
| else stored = true; | |
| } else { | |
| stored = true; | |
| } | |
| const { error } = await supabaseAdmin | |
| .from("runbook_documents") | |
| .upsert(row, { onConflict: "provider,external_id" }); | |
| const stored = !error; | |
| if (error) errors.push(`${filePath}#${cIdx}: ${error.message}`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/embedIndexer.ts` around lines 200 - 208, The fallback insert after a
failed upsert in embedIndexer.ts is incorrect: if
supabaseAdmin.from("runbook_documents").upsert(row, { onConflict:
"provider,external_id" }) returns an error it won’t be fixed by re-running
insert (unique conflict or other issues persist); change the logic in the block
handling the upsert result so that on error you record the error (push to errors
with insErr.message or error.message) and do not attempt the duplicate insert,
or implement a proper retry/backoff for transient failures; update the uses of
variables row, stored, errors and the upsert/insert call sites so stored is only
set true on successful upsert/insert and ensure the logged error comes from the
original upsert error when present.
Comment on lines
+57
to
+62
| const extraDocs: SourceDoc[] = input.customSources.map((s, i) => ({ | ||
| id: `manual-${i}-${s.title.slice(0, 20)}`, | ||
| title: s.title.trim().slice(0, 200), | ||
| content: s.content.trim().slice(0, 12_000), | ||
| sourceType: "text" as const | ||
| })); |
There was a problem hiding this comment.
Cap customSources count, not just per-item size.
Each item is truncated to 12k characters, but there's no bound on input.customSources.length. A malicious or buggy caller can submit thousands of entries, which then flow into retrieveKeywordSources and the prompt-building below, blowing up CPU and (potentially) the LLM token budget. Consider input.customSources.slice(0, 32) (or whatever ceiling fits the demo) before mapping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/embedNorthstarChat.ts` around lines 57 - 62, The code maps all
entries in input.customSources into extraDocs without limiting the number of
items, which lets a caller submit arbitrarily many docs and exhaust CPU/LLM
tokens; before mapping, cap the array (e.g. const capped =
input.customSources.slice(0, 32)) and map capped instead of input.customSources
so extraDocs has a fixed upper bound; update any downstream references
(retrieveKeywordSources and prompt-building that consume extraDocs) to use the
capped list and document the chosen max items constant near the extraDocs
creation.
Comment on lines
+3
to
+19
| const buckets = new Map<string, Bucket>(); | ||
| const WINDOW_MS = 60_000; | ||
| const MAX_PER_WINDOW = 40; | ||
|
|
||
| export function checkEmbedRateLimit(keyId: string): { ok: true } | { ok: false; retryAfter: number } { | ||
| const now = Date.now(); | ||
| let b = buckets.get(keyId); | ||
| if (!b || now > b.reset) { | ||
| b = { count: 0, reset: now + WINDOW_MS }; | ||
| buckets.set(keyId, b); | ||
| } | ||
| if (b.count >= MAX_PER_WINDOW) { | ||
| return { ok: false, retryAfter: Math.ceil((b.reset - now) / 1000) }; | ||
| } | ||
| b.count += 1; | ||
| return { ok: true }; | ||
| } |
There was a problem hiding this comment.
In-memory limiter has two production hazards: per-instance state and unbounded growth.
- State lives in a module-scoped
Map, so on Vercel/serverless or any horizontally-scaled deploy each runtime instance has its own buckets. The effective limit becomesMAX_PER_WINDOW ×#instances``, which materially weakens abuse protection on the public embed/chat path. - Buckets are never evicted — only overwritten when the same
keyIdis seen again after expiry. Distinct keys (incl. thedemopath mixed with random/spoofed ones) accumulate forever, leaking memory in long-lived Node processes.
For the demo this is probably acceptable, but please at least bound the map and consider a shared store (Upstash/Redis) before public traffic.
♻️ Suggested minimal mitigation (opportunistic eviction + size cap)
const buckets = new Map<string, Bucket>();
const WINDOW_MS = 60_000;
const MAX_PER_WINDOW = 40;
+const MAX_TRACKED_KEYS = 10_000;
export function checkEmbedRateLimit(keyId: string): { ok: true } | { ok: false; retryAfter: number } {
const now = Date.now();
+ // Opportunistic cleanup to avoid unbounded growth.
+ if (buckets.size > MAX_TRACKED_KEYS) {
+ for (const [k, v] of buckets) {
+ if (now > v.reset) buckets.delete(k);
+ }
+ // If still oversized, drop oldest insertion-order entries.
+ while (buckets.size > MAX_TRACKED_KEYS) {
+ const firstKey = buckets.keys().next().value;
+ if (firstKey === undefined) break;
+ buckets.delete(firstKey);
+ }
+ }
let b = buckets.get(keyId);For multi-instance correctness, swap this for a shared backing store (e.g., Upstash Redis INCR+EXPIRE) keyed by keyId.
📝 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
| const buckets = new Map<string, Bucket>(); | |
| const WINDOW_MS = 60_000; | |
| const MAX_PER_WINDOW = 40; | |
| export function checkEmbedRateLimit(keyId: string): { ok: true } | { ok: false; retryAfter: number } { | |
| const now = Date.now(); | |
| let b = buckets.get(keyId); | |
| if (!b || now > b.reset) { | |
| b = { count: 0, reset: now + WINDOW_MS }; | |
| buckets.set(keyId, b); | |
| } | |
| if (b.count >= MAX_PER_WINDOW) { | |
| return { ok: false, retryAfter: Math.ceil((b.reset - now) / 1000) }; | |
| } | |
| b.count += 1; | |
| return { ok: true }; | |
| } | |
| const buckets = new Map<string, Bucket>(); | |
| const WINDOW_MS = 60_000; | |
| const MAX_PER_WINDOW = 40; | |
| const MAX_TRACKED_KEYS = 10_000; | |
| export function checkEmbedRateLimit(keyId: string): { ok: true } | { ok: false; retryAfter: number } { | |
| const now = Date.now(); | |
| // Opportunistic cleanup to avoid unbounded growth. | |
| if (buckets.size > MAX_TRACKED_KEYS) { | |
| for (const [k, v] of buckets) { | |
| if (now > v.reset) buckets.delete(k); | |
| } | |
| // If still oversized, drop oldest insertion-order entries. | |
| while (buckets.size > MAX_TRACKED_KEYS) { | |
| const firstKey = buckets.keys().next().value; | |
| if (firstKey === undefined) break; | |
| buckets.delete(firstKey); | |
| } | |
| } | |
| let b = buckets.get(keyId); | |
| if (!b || now > b.reset) { | |
| b = { count: 0, reset: now + WINDOW_MS }; | |
| buckets.set(keyId, b); | |
| } | |
| if (b.count >= MAX_PER_WINDOW) { | |
| return { ok: false, retryAfter: Math.ceil((b.reset - now) / 1000) }; | |
| } | |
| b.count += 1; | |
| return { ok: true }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/embedRateLimit.ts` around lines 3 - 19, The in-memory Map-based
limiter (buckets, WINDOW_MS, MAX_PER_WINDOW, and function checkEmbedRateLimit)
causes per-instance limits and unbounded growth; replace the module-scoped Map
with a shared store (e.g., Upstash/Redis) using INCR + EXPIRE per keyId to
enforce a global MAX_PER_WINDOW across instances and correct TTL-based resets,
and if you must keep a local fallback keep a bounded Map size (e.g., MAX_BUCKETS
constant) and implement opportunistic eviction in checkEmbedRateLimit to prune
expired entries and remove oldest/newest entries when the cap is exceeded to
avoid memory leaks.
Comment on lines
+35
to
+50
| const { data: documents, error } = await supabaseAdmin.rpc("match_documents", { | ||
| query_embedding: `[${embedding.join(",")}]`, | ||
| match_threshold: DEFAULT_MATCH_THRESHOLD, | ||
| match_count: TOP_K * 8 | ||
| }); | ||
|
|
||
| if (error) { | ||
| console.error("Embed vector search error:", error); | ||
| return []; | ||
| } | ||
|
|
||
| const batch = (documents || []) as MatchedDocument[]; | ||
| const scoped = batch.filter((d) => matchesEmbedScope(d.content, projectId)); | ||
| return scoped | ||
| .sort((a, b) => b.similarity - a.similarity) | ||
| .slice(0, TOP_K) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether match_documents accepts a project filter parameter
fd -e sql . supabase | xargs rg -nC2 'match_documents'Repository: dfed25/RunBook
Length of output: 510
🏁 Script executed:
# Get full match_documents function definition
sed -n '19,80p' supabase/migrations/00000000000000_init_vector_db.sqlRepository: dfed25/RunBook
Length of output: 895
🏁 Script executed:
# Find TOP_K constant and matchesEmbedScope implementation
rg -n 'TOP_K|matchesEmbedScope' src/lib/embedRetrieval.ts -A 3 -B 3Repository: dfed25/RunBook
Length of output: 1151
🏁 Script executed:
# Check the entire embedRetrieval.ts file to understand the context
wc -l src/lib/embedRetrieval.tsRepository: dfed25/RunBook
Length of output: 87
Client-side project filtering may starve results in multi-tenant deployments.
The match_documents SQL function has no project filter parameter and returns the top match_count documents from all projects by similarity. You then filter these 48 candidates (TOP_K * 8 where TOP_K = 6) client-side by checking for the [embed:${projectId}] marker in content. If documents from other projects dominate the top-48 nearest neighbors, the scoped result set can be empty even when relevant docs exist deeper in the full ranking.
Consider either (a) modifying match_documents to accept and filter by projectId at the SQL level (most robust), or (b) raising match_count substantially. As a stopgap, paginate via additional RPC calls until scoped.length >= TOP_K or a hard limit is reached.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/embedRetrieval.ts` around lines 35 - 50, The current call to
supabaseAdmin.rpc("match_documents", ...) requests TOP_K * 8 candidates and then
filters client-side via matchesEmbedScope(d.content, projectId), which can drop
all results if other-project docs dominate; fix by either (preferred) extending
the match_documents RPC to accept a project_id (or embed scope) and apply the
filter in SQL and then call it from this code (update the rpc parameters where
match_documents is invoked), or (stopgap) implement paging: repeatedly call
supabaseAdmin.rpc("match_documents", { query_embedding:
`[${embedding.join(",")}]`, match_threshold: DEFAULT_MATCH_THRESHOLD,
match_count: pageSize, page_offset: offset }) aggregating unique documents,
apply matchesEmbedScope(projectId) to the accumulated set, and stop when
scoped.length >= TOP_K or a safe max iterations is reached, then sort by
similarity and return the top TOP_K; ensure you update uses of TOP_K,
match_count, match_documents, matchesEmbedScope, embedding, and projectId
accordingly.
Comment on lines
+5
to
+11
| function sessionSecret(): string { | ||
| return ( | ||
| process.env.RUNBOOK_EMBED_SESSION_SECRET || | ||
| process.env.SUPABASE_SERVICE_ROLE_KEY?.slice(0, 48) || | ||
| "runbook-embed-dev-secret-change-in-production" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Don't silently fall back to a hardcoded session secret in production.
If RUNBOOK_EMBED_SESSION_SECRET and SUPABASE_SERVICE_ROLE_KEY are both unset, the HMAC key becomes the literal string "runbook-embed-dev-secret-change-in-production". That secret is in the public repo, so anyone can mint a valid embed_session token for any githubId/login and bypass verifyEmbedSession — i.e., full impersonation against GET /api/embed/projects, POST /api/embed/projects/:id/index, etc. Reusing SUPABASE_SERVICE_ROLE_KEY as a signing key is also undesirable (key reuse across purposes; service-role keys often live in client-readable config in some setups).
Recommend: require the dedicated env in non-dev and fail closed.
🛡️ Proposed fix
function sessionSecret(): string {
- return (
- process.env.RUNBOOK_EMBED_SESSION_SECRET ||
- process.env.SUPABASE_SERVICE_ROLE_KEY?.slice(0, 48) ||
- "runbook-embed-dev-secret-change-in-production"
- );
+ const secret =
+ process.env.RUNBOOK_EMBED_SESSION_SECRET?.trim() ||
+ process.env.SUPABASE_SERVICE_ROLE_KEY?.slice(0, 48);
+ if (!secret) {
+ if (process.env.NODE_ENV === "production") {
+ throw new Error("RUNBOOK_EMBED_SESSION_SECRET is required in production");
+ }
+ return "runbook-embed-dev-secret-change-in-production";
+ }
+ return secret;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/embedSession.ts` around lines 5 - 11, The sessionSecret() function
must not silently fall back to a public hardcoded secret or reuse
SUPABASE_SERVICE_ROLE_KEY in production; change it so it returns
RUNBOOK_EMBED_SESSION_SECRET if present, and otherwise throws (fail-closed) when
not running in development (e.g., NODE_ENV !== 'development' or VERCEL_ENV !==
'development'); allow an explicit, documented fallback only for local/dev runs
(optionally use SUPABASE_SERVICE_ROLE_KEY or the dev literal only when NODE_ENV
=== 'development') and add a clear error message referencing sessionSecret() so
the app fails to start if the dedicated env is missing in non-dev environments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Turn the embed demo into a functional product path with real chat retrieval, Studio-driven assistant configuration, and resilient LLM fallbacks so the end-to-end onboarding demo works reliably without required external integrations.
Made-with: Cursor
Summary by CodeRabbit
Release Notes
New Features
Documentation