fix: restore manager source flow and add safe hire recovery - #12
Conversation
Allow demo-mode hire access for manager source workflows, add double-confirm archive behavior, and provide archived-hire restore actions to prevent accidental irreversible removals. 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 47 minutes and 26 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 (5)
📝 WalkthroughWalkthroughThis PR introduces Google Drive URL ingestion, refactors document retrieval with hire-scoped visibility and progressive threshold backoff, implements optional unauthenticated hire access via environment variable, adds active/archived hire distinction with archival workflows, and improves document scoring in chat endpoints. Changes
Sequence DiagramsequenceDiagram
participant Client as Chat Client
participant Route as Chat Route
participant Auth as Auth Layer
participant Retrieve as Retrieval Pipeline
participant VectorDB as Vector DB
participant Keyword as Keyword Fallback
participant Rank as Ranker
participant Response as Response
Client->>Route: POST /api/chat (question, hireId?)
Route->>Auth: requireHireAccess(hireId)
Auth-->>Route: ✓ (userId from auth or demo-user)
Route->>Retrieve: retrieveDocs(question, hireId)
Retrieve->>Retrieve: tokenizeQuestion()
Retrieve->>Retrieve: matchesRetrievalScope(hireId)
Retrieve->>VectorDB: matchDocumentsWithBackoff()<br/>(progressive threshold decrease)
loop Lower threshold & increase match_count
VectorDB-->>Retrieve: scoped results
Retrieve->>Retrieve: merge & check sufficiency
end
alt embeddings insufficient
Retrieve->>Keyword: fetchHireKeywordFallback(hireId)
Keyword-->>Retrieve: external_id prefix + title matches
Retrieve->>Retrieve: merge vector + keyword results
end
Retrieve->>Rank: rankHireDocuments(hireId)<br/>(title keyword re-weighting)
Rank-->>Retrieve: re-ranked documents
Retrieve-->>Route: filtered & ranked docs
Route->>Route: sort by score descending
Route-->>Response: documents for context
Response-->>Client: chat response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
Sync hire knowledge immediately after source creation and improve hire-scoped retrieval recall so newly added docs are available in chat without manual sync friction. Made-with: Cursor
…allback; stop filtering non-positive similarity in chat API Made-with: Cursor
… sources surface Made-with: Cursor
… web UI HTML) Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/manager/tasks/page.tsx (1)
184-210:⚠️ Potential issue | 🟡 MinorDisable the "Add source" submit while a sync is in flight.
addSourceflipssetSyncing(true)after POSTing the source, but the submit button at line 296 only checks!selectedHireId. A user can submit a second source mid-sync, which races a parallel/api/sync/knowledgeand produces stalesetMessageresults from the earlier handler clobbering the later one (or vice versa). Either disable the submit or guard re-entry.♻️ Suggested guard
- <AppButton variant="secondary" type="submit" disabled={!selectedHireId}>Add source</AppButton> + <AppButton variant="secondary" type="submit" disabled={!selectedHireId || syncing}>Add source</AppButton>src/lib/apiAuth.ts (1)
11-42:⚠️ Potential issue | 🟠 MajorFail-open auth default — invert to fail-closed.
allowUnauthedDemoAccessevaluates totruewheneverRUNBOOK_ALLOW_UNAUTH_HIRE_ACCESSis unset or set to any value except the literal string"false". The.env.exampledefaults this totrue, so any deployment that forgets to set this variable (CI, staging, a fresh prod env) silently allows unauthenticated callers to archive/restore hires, add sources, trigger knowledge sync, and chat against hire-scoped data — all attributed to a synthetic"demo-user". That's a regression from the previous 401 behavior.Recommended fix: opt-in to the demo bypass. Treat any value other than the explicit allow-list (
"true"/"1") as "require auth."🔒 Suggested change
-const allowUnauthedDemoAccess = - process.env.RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS !== "false"; +const allowUnauthedDemoAccess = ["true", "1"].includes( + (process.env.RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS ?? "").toLowerCase(), +);Pair this with flipping
.env.exampletofalseand, ideally, gate the bypass onprocess.env.NODE_ENV !== "production"so it cannot be enabled in prod even by misconfiguration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/apiAuth.ts` around lines 11 - 42, The current allowUnauthedDemoAccess defaults to true; change it to opt-in by evaluating process.env.RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS only as enabled when it equals "true" or "1" (e.g., allowUnauthedDemoAccess = ["true","1"].includes(...)); update requireHireAccess to use this new flag and additionally prevent the bypass in production by requiring allowUnauthedDemoAccess && process.env.NODE_ENV !== "production" when deciding to return the demo-user path; keep getCurrentUserId and isUserAuthorizedForHire unchanged.
🧹 Nitpick comments (3)
src/app/api/chat/route.ts (1)
60-65: Nit: comment mislabels the score as cosine "distance".Sort is descending by
scoreand retrieval thresholds (0.45, 0.35, …) only make sense if higher is better — i.e., cosine similarity, not distance. The change to drop the> 0filter is still correct (similarity can be slightly negative), but the inline note will mislead future readers.♻️ Wording fix
- // NOTE: `match_documents` uses cosine *distance* semantics; similarity scores are not guaranteed to be > 0. + // NOTE: `match_documents` returns cosine similarity; values can be slightly negative, so don't filter on `score > 0`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/chat/route.ts` around lines 60 - 65, The inline comment near retrieved/validDocs incorrectly calls the metric "cosine distance"; update the comment around retrieveDocs/retrieved/validDocs (and mention match_documents) to say the scores are cosine similarity (higher is better) and that similarity can be slightly negative, so thresholds like 0.45/0.35 assume larger-is-better — keep the note that removing the > 0 filter is intentional.src/lib/ingestion/gdrive.ts (1)
196-218: Folder ingest does an O(n²)lines.joinper iteration.Inside the folder loop,
lines.join("\n").lengthis recomputed on every file to test the cap. Bounded byMAX_FOLDER_ITEMS = 45andMAX_TOTAL_FOLDER_CHARS = 350_000, the worst-case work is roughly45 × 350K ≈ 15 MBof string copies — not fatal, but easy to drop.♻️ Track running length
- for (const f of files) { + let totalLen = lines.reduce((n, s) => n + s.length + 1, 0); + for (const f of files) { const id = f.id; const name = f.name || "Untitled"; const mime = f.mimeType || ""; if (!id) continue; - lines.push(`━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━`); - lines.push(`## ${name}`); - lines.push(`MIME: ${mime}`); - lines.push(""); + const header = [`━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━`, `## ${name}`, `MIME: ${mime}`, ""]; + for (const h of header) { lines.push(h); totalLen += h.length + 1; } try { const chunk = await extractDriveFileText(drive, id, name, mime); lines.push(chunk); + totalLen += chunk.length + 1; } catch (err) { - lines.push(`(Failed to read this item: ${err instanceof Error ? err.message : String(err)})`); + const msg = `(Failed to read this item: ${err instanceof Error ? err.message : String(err)})`; + lines.push(msg); + totalLen += msg.length + 1; } lines.push(""); - if (lines.join("\n").length > MAX_TOTAL_FOLDER_CHARS) { + totalLen += 1; + if (totalLen > MAX_TOTAL_FOLDER_CHARS) { lines.push(`...[stopped: folder content capped at ${MAX_TOTAL_FOLDER_CHARS} characters]...`); break; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ingestion/gdrive.ts` around lines 196 - 218, The code currently recomputes lines.join("\n").length inside the files loop causing O(n²) string work; replace that with a running length counter: introduce a numeric variable (e.g., runningLen) initialized to the initial content length (or 0), and whenever you push to lines in the loop (the header separator, `## ${name}`, `MIME: ${mime}`, the empty line, the chunk or failure message, and the trailing empty line) increment runningLen by the exact number of characters those pushes will add including the "\n" separators, then check runningLen > MAX_TOTAL_FOLDER_CHARS to break and push the capped message; update code paths that call extractDriveFileText and the catch branch to calculate the chunk length before pushing so you can update runningLen atomically and avoid calling lines.join anywhere in the loop.src/lib/retrieval.ts (1)
184-218: Optional: short-circuit backoff when batch is already smaller thanmatch_count.If iteration
ireturns fewer rows thanmatch_count[i], lowering the threshold further on iterationi+1cannot surface new candidates — it just adds an RPC round-trip per chat. Worth bailing early to keep p95 latency tight on cold/empty corpora.♻️ Sketch
const batch = (documents || []) as MatchedDocument[]; const merged = new Map<string, MatchedDocument>(); for (const doc of [...best, ...batch]) merged.set(doc.id, doc); best = Array.from(merged.values()).sort((a, b) => b.similarity - a.similarity); const scopedCount = best.filter((d) => matchesRetrievalScope(d.content, hireId)).length; if (scopedCount >= TOP_K) break; + // No point widening the net if the DB has fewer rows than the current ceiling. + if (batch.length < match_count) break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/retrieval.ts` around lines 184 - 218, In matchDocumentsWithBackoff, add a short-circuit: after the RPC returns and you have const batch = (documents || []) as MatchedDocument[], if batch.length < match_count then merge batch into best (same Map merge logic) and break the loop, because requesting a lower threshold cannot produce more rows than match_count; this avoids the extra RPC round-trip for the next iteration. Reference symbols: matchDocumentsWithBackoff, thresholds, matchCounts, match_count, batch, best, and the Map merge block.
🤖 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:
- Around line 13-16: Change the permissive default in the .env.example by
setting RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS to false (so the example defaults to
requiring auth) and add a trailing newline at the end of the file; this aligns
the example with the principle of least privilege and resolves the dotenv-linter
warning — check references to RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS and related logic
in src/lib/apiAuth.ts to ensure the example matches expected behavior.
In `@src/lib/ingestion/gdrive.ts`:
- Around line 40-59: parseDriveGoogleUrl currently only accepts host
"drive.google.com", so docs.google.com links (Docs/Sheets/Slides) are missed;
update parseDriveGoogleUrl to also accept "docs.google.com" and detect the docs
paths by adding a pathname regex like
/\/(?:document|spreadsheets|presentation)\/d\/([a-zA-Z0-9_-]+)/ (returning {
kind: "file", id }) in addition to the existing /file\/d\/... and /folders\/...
checks; this will allow fetchGoogleDriveUrlContent / ingestSingleDriveFile /
driveFileMeta to resolve the real mimeType and exports correctly for
docs.google.com links.
---
Outside diff comments:
In `@src/lib/apiAuth.ts`:
- Around line 11-42: The current allowUnauthedDemoAccess defaults to true;
change it to opt-in by evaluating process.env.RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS
only as enabled when it equals "true" or "1" (e.g., allowUnauthedDemoAccess =
["true","1"].includes(...)); update requireHireAccess to use this new flag and
additionally prevent the bypass in production by requiring
allowUnauthedDemoAccess && process.env.NODE_ENV !== "production" when deciding
to return the demo-user path; keep getCurrentUserId and isUserAuthorizedForHire
unchanged.
---
Nitpick comments:
In `@src/app/api/chat/route.ts`:
- Around line 60-65: The inline comment near retrieved/validDocs incorrectly
calls the metric "cosine distance"; update the comment around
retrieveDocs/retrieved/validDocs (and mention match_documents) to say the scores
are cosine similarity (higher is better) and that similarity can be slightly
negative, so thresholds like 0.45/0.35 assume larger-is-better — keep the note
that removing the > 0 filter is intentional.
In `@src/lib/ingestion/gdrive.ts`:
- Around line 196-218: The code currently recomputes lines.join("\n").length
inside the files loop causing O(n²) string work; replace that with a running
length counter: introduce a numeric variable (e.g., runningLen) initialized to
the initial content length (or 0), and whenever you push to lines in the loop
(the header separator, `## ${name}`, `MIME: ${mime}`, the empty line, the chunk
or failure message, and the trailing empty line) increment runningLen by the
exact number of characters those pushes will add including the "\n" separators,
then check runningLen > MAX_TOTAL_FOLDER_CHARS to break and push the capped
message; update code paths that call extractDriveFileText and the catch branch
to calculate the chunk length before pushing so you can update runningLen
atomically and avoid calling lines.join anywhere in the loop.
In `@src/lib/retrieval.ts`:
- Around line 184-218: In matchDocumentsWithBackoff, add a short-circuit: after
the RPC returns and you have const batch = (documents || []) as
MatchedDocument[], if batch.length < match_count then merge batch into best
(same Map merge logic) and break the loop, because requesting a lower threshold
cannot produce more rows than match_count; this avoids the extra RPC round-trip
for the next iteration. Reference symbols: matchDocumentsWithBackoff,
thresholds, matchCounts, match_count, batch, best, and the Map merge block.
🪄 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: bf290671-500e-4378-a8e6-c146157d3517
📒 Files selected for processing (7)
.env.examplesrc/app/api/chat/route.tssrc/app/manager/tasks/page.tsxsrc/lib/apiAuth.tssrc/lib/ingestion/gdrive.tssrc/lib/retrieval.tssrc/lib/vectorizer.ts
| # Optional: used by manager control plane in hybrid mode. | ||
| # Managers can always add links in the UI; provider credentials unlock richer ingestion. No newline at end of file | ||
| # Managers can always add links in the UI; provider credentials unlock richer ingestion. | ||
| # Set to "false" to require real auth on hire-scoped manager/sync/chat endpoints. | ||
| RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS=true No newline at end of file |
There was a problem hiding this comment.
Permissive default + missing trailing newline.
Two small things:
- The example sets
RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS=true, which is the unsafe configuration. Consider shipping.env.examplewithfalseso the principle of least surprise is "this needs auth unless you opt out for demo." (Root-cause concern lives insrc/lib/apiAuth.ts.) - dotenv-linter flags missing trailing newline at line 16.
🛡️ Suggested change
# Optional: used by manager control plane in hybrid mode.
# Managers can always add links in the UI; provider credentials unlock richer ingestion.
-# Set to "false" to require real auth on hire-scoped manager/sync/chat endpoints.
-RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS=true
+# Set to "true" ONLY for local/demo to skip auth on hire-scoped manager/sync/chat endpoints.
+RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS=false
+📝 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.
| # Optional: used by manager control plane in hybrid mode. | |
| # Managers can always add links in the UI; provider credentials unlock richer ingestion. | |
| \ No newline at end of file | |
| # Managers can always add links in the UI; provider credentials unlock richer ingestion. | |
| # Set to "false" to require real auth on hire-scoped manager/sync/chat endpoints. | |
| RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS=true | |
| # Optional: used by manager control plane in hybrid mode. | |
| # Managers can always add links in the UI; provider credentials unlock richer ingestion. | |
| # Set to "true" ONLY for local/demo to skip auth on hire-scoped manager/sync/chat endpoints. | |
| RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS=false | |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 16-16: [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 around lines 13 - 16, Change the permissive default in the
.env.example by setting RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS to false (so the
example defaults to requiring auth) and add a trailing newline at the end of the
file; this aligns the example with the principle of least privilege and resolves
the dotenv-linter warning — check references to RUNBOOK_ALLOW_UNAUTH_HIRE_ACCESS
and related logic in src/lib/apiAuth.ts to ensure the example matches expected
behavior.
| export function parseDriveGoogleUrl(rawUrl: string): { kind: "folder" | "file" | "open"; id: string } | null { | ||
| try { | ||
| const url = new URL(rawUrl.trim()); | ||
| const host = url.hostname.toLowerCase(); | ||
| if (host !== "drive.google.com") return null; | ||
|
|
||
| const folderMatch = url.pathname.match(/\/folders\/([a-zA-Z0-9_-]+)/); | ||
| if (folderMatch?.[1]) return { kind: "folder", id: folderMatch[1] }; | ||
|
|
||
| const fileMatch = url.pathname.match(/\/file\/d\/([a-zA-Z0-9_-]+)/); | ||
| if (fileMatch?.[1]) return { kind: "file", id: fileMatch[1] }; | ||
|
|
||
| const idParam = url.searchParams.get("id"); | ||
| if (idParam && /^[a-zA-Z0-9_-]+$/.test(idParam)) return { kind: "open", id: idParam }; | ||
|
|
||
| return null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
docs.google.com URLs are not detected — most Google Docs links will fall back to web HTML.
parseDriveGoogleUrl requires host === "drive.google.com", so Google Docs/Sheets/Slides share URLs (which use docs.google.com/{document,spreadsheets,presentation}/d/<ID>/...) silently take the fetchUrlDocument path in vectorizer.ts, scraping the auth-walled viewer shell instead of the real content. Given that managers will overwhelmingly paste docs.google.com links, this is a meaningful gap in the new Drive ingestion path.
🐛 Suggested extension
export function parseDriveGoogleUrl(rawUrl: string): { kind: "folder" | "file" | "open"; id: string } | null {
try {
const url = new URL(rawUrl.trim());
const host = url.hostname.toLowerCase();
- if (host !== "drive.google.com") return null;
+ if (host !== "drive.google.com" && host !== "docs.google.com") return null;
const folderMatch = url.pathname.match(/\/folders\/([a-zA-Z0-9_-]+)/);
if (folderMatch?.[1]) return { kind: "folder", id: folderMatch[1] };
const fileMatch = url.pathname.match(/\/file\/d\/([a-zA-Z0-9_-]+)/);
if (fileMatch?.[1]) return { kind: "file", id: fileMatch[1] };
+ // docs.google.com/{document,spreadsheets,presentation}/d/<ID>/...
+ const docsMatch = url.pathname.match(/\/(?:document|spreadsheets|presentation)\/d\/([a-zA-Z0-9_-]+)/);
+ if (docsMatch?.[1]) return { kind: "file", id: docsMatch[1] };
+
const idParam = url.searchParams.get("id");
if (idParam && /^[a-zA-Z0-9_-]+$/.test(idParam)) return { kind: "open", id: idParam };
return null;
} catch {
return null;
}
}fetchGoogleDriveUrlContent already routes kind: "file" through ingestSingleDriveFile, which uses driveFileMeta to resolve the real mimeType and pick the right export — so adding the docs.google.com regex is sufficient.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/ingestion/gdrive.ts` around lines 40 - 59, parseDriveGoogleUrl
currently only accepts host "drive.google.com", so docs.google.com links
(Docs/Sheets/Slides) are missed; update parseDriveGoogleUrl to also accept
"docs.google.com" and detect the docs paths by adding a pathname regex like
/\/(?:document|spreadsheets|presentation)\/d\/([a-zA-Z0-9_-]+)/ (returning {
kind: "file", id }) in addition to the existing /file\/d\/... and /folders\/...
checks; this will allow fetchGoogleDriveUrlContent / ingestSingleDriveFile /
driveFileMeta to resolve the real mimeType and exports correctly for
docs.google.com links.
…ngs, lists, bold); cleaner source excerpts Made-with: Cursor
Allow demo-mode hire access for manager source workflows, add double-confirm archive behavior, and provide archived-hire restore actions to prevent accidental irreversible removals.
Made-with: Cursor
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes