refactor(workspaces): simplify extraction and report readiness - #696
Conversation
Delete the Firecrawl PDF extraction provider and the stub provider factory: nothing routed to either, and mistral_ocr existed only as a union member backed by a throwing stub. Uploads route to llama_parse and workers_ai_to_markdown only. Removing Firecrawl also removes its private mode enum, so the shared extraction mode union loses the members no route could reach. Each provider previously re-narrowed that union with a silent fallback, so an unrecognised mode became the provider default rather than an error. Collapse MarkdownExtractionProviderId and MarkdownExtractionProviderMode into the canonical WorkspaceFile* names they aliased, and drop the markdownExtractionProviders re-export, which had no consumers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A read of a file whose extraction had not finished returned a bare pending status, and every extraction failure collapsed into one projection_failed code, so the model could not tell a transient problem from one that will never resolve, nor how long a wait had already run. Classify the projection row in resolveWorkspaceProjectionReadiness so these cases are decided in one pure function rather than as branches inside the R2 read path. Pending results now carry the phase, elapsed time, and a retry hint that backs off as the wait grows. Failures split into extraction_failed and extraction_stalled, and carry the recorded reason where the row has one. A projection stuck in processing for longer than any healthy run could take is now reported as stalled rather than pending forever: nothing detected a dead workflow before, so the item stayed pending indefinitely. The threshold sits above the worst case a retrying extraction can reach, so slow runs are not mislabelled. Ready reads report whether content came from the fast first pass and which returned pages extracted no text, so a partially scanned PDF is no longer read as a document that happens to be blank in places. Handling guidance rides on the results that need it rather than in the tool description, which every request pays for whether or not a file is ever read. Drop the not_started and queued projection statuses: nothing has ever written either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWorkspace extraction now supports two providers, classifies projection readiness, exposes provisional and empty-page results, maps extraction states into content read responses, and adds model-visible guidance for pending, transient, failed, and provisional reads. ChangesWorkspace extraction and read behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
React Doctor found 2 new issues in 2 files · 2 warnings · score 90 / 100 (Great) · 0 fixed · vs 2 warnings
Reviewed by React Doctor for commit |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de869b1ae6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!projection.objectKey || !projection.sourceHash) { | ||
| return { state: "unreadable" }; |
There was a problem hiding this comment.
When this deploy reads a projection row written by an older release with status equal to queued or not_started, it skips both explicit status branches and reaches this missing-object check, returning projection_failed. Earlier versions of requestWorkspaceFileExtraction persisted queued, and the previous reader explicitly treated both statuses as pending, so existing Durable Object rows can now be reported as storage failures while extraction is merely waiting. Continue recognizing these legacy values at runtime or migrate the persisted rows before removing their handling.
Useful? React with 👍 / 👎.
| if (result.type === "file" && (result.provisional || result.emptyPages?.length)) { | ||
| situations.add("provisional"); |
There was a problem hiding this comment.
For a completed enhanced PDF containing a genuinely blank physical page, readFile sets emptyPages but omits provisional; this condition still emits guidance saying those pages are extracting and should be read again later. That causes pointless repeat reads and misrepresents final blank pages, so the provisional guidance should depend on result.provisional (with emptyPages only qualifying which provisional pages are incomplete).
Useful? React with 👍 / 👎.
| return { | ||
| state: "ready", | ||
| manifestObjectKey: projection.objectKey, | ||
| sourceHash: projection.sourceHash, | ||
| provisional: projection.metadataJson.provisional === true, |
There was a problem hiding this comment.
When LiteParse succeeds but the enhanced extraction later fails, WorkspaceFileExtractionWorkflow returns the partial result without updating the projection row, leaving the fast projection's metadataJson.provisional set to true. This new mapping therefore reports that a higher-quality pass is still running forever, and the accompanying guidance tells the model to retry empty pages even though the workflow has terminated. The partial-failure path should publish a terminal fast projection or otherwise clear the provisional marker.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/features/workspaces/content/workspace-content-reader.ts (1)
280-282: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSurface
elapsedSecondswhen reporting a stalled extraction.
resolveWorkspaceProjectionReadinesscomputes how long the row has been stuck ({ state: "stalled"; elapsedSeconds }), butdescribeUnreadableProjectiondrops it here. Themessagefield is optional on the failed-variant schema, so including the duration would help downstream guidance/support triage without changing the contract.♻️ Proposed fix
if (projection.state === "stalled") { - return { code: "extraction_stalled", path, status: "failed", type: "file" }; + return { + code: "extraction_stalled", + message: `Extraction has been stuck in processing for ${projection.elapsedSeconds} seconds.`, + path, + status: "failed", + type: "file", + }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/workspaces/content/workspace-content-reader.ts` around lines 280 - 282, Update the stalled branch in describeUnreadableProjection to include projection.elapsedSeconds in the returned failed result, using the optional message field to surface the duration while preserving the existing extraction_stalled code, path, status, and type values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/workspaces/content/workspace-read-references.ts`:
- Around line 61-64: Update the pending guidance in workspaceReadGuidance so it
instructs the model to use the numeric retryAfterSeconds value reported by each
pending result, rather than repeating the literal token name. Preserve the
existing instructions about not waiting and limiting repeated reads.
In `@src/features/workspaces/extraction/workspace-projection-readiness.ts`:
- Around line 78-80: Sanitize or normalize projection.errorMessage before
returning the failed read result from the projection readiness flow. Update the
failed branch in the workspace projection readiness function to pass the message
through the existing sanitization mechanism, preserving the "failed" state while
preventing raw stack traces, URLs, or request IDs from reaching model-visible
guidance.
---
Nitpick comments:
In `@src/features/workspaces/content/workspace-content-reader.ts`:
- Around line 280-282: Update the stalled branch in describeUnreadableProjection
to include projection.elapsedSeconds in the returned failed result, using the
optional message field to surface the duration while preserving the existing
extraction_stalled code, path, status, and type values.
🪄 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 Plus
Run ID: 97d1800a-7f0d-48c5-beb7-b2f1d45812d6
📒 Files selected for processing (16)
src/features/workspaces/content/workspace-content-contract.tssrc/features/workspaces/content/workspace-content-reader.tssrc/features/workspaces/content/workspace-read-references.test.tssrc/features/workspaces/content/workspace-read-references.tssrc/features/workspaces/extraction/providers/firecrawl.tssrc/features/workspaces/extraction/providers/index.tssrc/features/workspaces/extraction/providers/stubs.tssrc/features/workspaces/extraction/types.tssrc/features/workspaces/extraction/workspace-file-extraction-observability.tssrc/features/workspaces/extraction/workspace-file-extraction-workflow.tssrc/features/workspaces/extraction/workspace-page-projection.test.tssrc/features/workspaces/extraction/workspace-page-projection.tssrc/features/workspaces/extraction/workspace-projection-readiness.test.tssrc/features/workspaces/extraction/workspace-projection-readiness.tssrc/features/workspaces/kernel/workspace-kernel-types.tssrc/features/workspaces/model/workspace-file/types.ts
💤 Files with no reviewable changes (2)
- src/features/workspaces/extraction/providers/stubs.ts
- src/features/workspaces/extraction/providers/firecrawl.ts
| const workspaceReadGuidance = { | ||
| pending: | ||
| "Some paths are still extracting. Never sleep, poll, or otherwise stall waiting for them, including inside compute, sandbox_bash, or orchestrate. Either do other work and read those paths again later in this reply, or tell the user they are still processing and to ask again in about retryAfterSeconds. Never read the same pending path more than twice in one reply.", | ||
| unrecoverable: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the reported retry delay rather than a literal token.
retryAfterSeconds is emitted verbatim here, so the model may tell users to retry “in about retryAfterSeconds” instead of using the numeric value on each pending result.
Proposed fix
pending:
- "Some paths are still extracting. Never sleep, poll, or otherwise stall waiting for them, including inside compute, sandbox_bash, or orchestrate. Either do other work and read those paths again later in this reply, or tell the user they are still processing and to ask again in about retryAfterSeconds. Never read the same pending path more than twice in one reply.",
+ "Some paths are still extracting. Never sleep, poll, or otherwise stall waiting for them, including inside compute, sandbox_bash, or orchestrate. Either do other work and read those paths again later in this reply, or tell the user they are still processing and can retry after the retryAfterSeconds value reported for that path. Never read the same pending path more than twice in one reply.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const workspaceReadGuidance = { | |
| pending: | |
| "Some paths are still extracting. Never sleep, poll, or otherwise stall waiting for them, including inside compute, sandbox_bash, or orchestrate. Either do other work and read those paths again later in this reply, or tell the user they are still processing and to ask again in about retryAfterSeconds. Never read the same pending path more than twice in one reply.", | |
| unrecoverable: | |
| const workspaceReadGuidance = { | |
| pending: | |
| "Some paths are still extracting. Never sleep, poll, or otherwise stall waiting for them, including inside compute, sandbox_bash, or orchestrate. Either do other work and read those paths again later in this reply, or tell the user they are still processing and can retry after the retryAfterSeconds value reported for that path. Never read the same pending path more than twice in one reply.", | |
| unrecoverable: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/workspaces/content/workspace-read-references.ts` around lines 61
- 64, Update the pending guidance in workspaceReadGuidance so it instructs the
model to use the numeric retryAfterSeconds value reported by each pending
result, rather than repeating the literal token name. Preserve the existing
instructions about not waiting and limiting repeated reads.
| if (projection.status === "failed") { | ||
| return { state: "failed", message: projection.errorMessage }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect where errorMessage is populated for the file-projection row to confirm it's already a sanitized/curated string.
rg -n -B3 -A3 'errorMessage' src/features/workspaces/extraction/workspace-file-extraction-workflow.ts src/features/workspaces/extraction/workspace-file-extraction-observability.ts 2>/dev/nullRepository: ThinkEx-OSS/thinkex
Length of output: 2013
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline src/features/workspaces/extraction/workspace-file-extraction-workflow.ts src/features/workspaces/extraction/workspace-projection-readiness.ts src/features/workspaces/extraction/workspace-content-reader.ts 2>/dev/null || true
echo
echo "== workflow getErrorMessage definitions/usages =="
rg -n -B5 -A15 'function getErrorMessage|const getErrorMessage|getErrorMessage\(' src/features/workspaces/extraction/workspace-file-extraction-workflow.ts src/features/workspaces 2>/dev/null
echo
echo "== readiness and reader relevant slices =="
sed -n '40,95p' src/features/workspaces/extraction/workspace-projection-readiness.ts
sed -n '1,220p' src/features/workspaces/extraction/workspace-content-reader.tsRepository: ThinkEx-OSS/thinkex
Length of output: 30191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate reader files =="
fd -a 'workspace-content-reader|workspace-content-contract|workspace-file-extraction-workflow' src 2>/dev/null || true
echo
echo "== search WorkspaceContentReadResult / message fields =="
rg -n -B4 -A6 'WorkspaceContentReadResult|describeUnreadableProjection|projection\.errorMessage|errorMessage' src 2>/dev/null
echo
echo "== workflow surrounding failed extraction details =="
sed -n '140,205p' src/features/workspaces/extraction/workspace-file-extraction-workflow.ts
sed -n '256,272p' src/features/workspaces/extraction/workspace-file-extraction-workflow.tsRepository: ThinkEx-OSS/thinkex
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== read references guidance implementation =="
sed -n '70,175p' src/features/workspaces/content/workspace-read-references.ts
sed -n '256,305p' src/features/workspaces/content/workspace-content-reader.ts
echo
echo "== contract schemas for failed statuses =="
sed -n '1,220p' src/features/workspaces/content/workspace-content-contract.ts
echo
echo "== failed projection callsites and possible source details =="
rg -n -B8 -A8 'projection_failed|extraction_failed|code: "projection_failed"|extraction_failed' src/features/workspaces src 2>/dev/null | head -n 220Repository: ThinkEx-OSS/thinkex
Length of output: 29789
Sanitize errorMessage before emitting read guidance with extraction failures.
getErrorMessage() forwards error.message/String(error) directly into the failed projection, and failed read results carry message in the contract. If extraction failures store raw stack traces, URLs, or request IDs, strip or normalize that text before adding the guidance so it does not leak details to model-visible output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/workspaces/extraction/workspace-projection-readiness.ts` around
lines 78 - 80, Sanitize or normalize projection.errorMessage before returning
the failed read result from the projection readiness flow. Update the failed
branch in the workspace projection readiness function to pass the message
through the existing sanitization mechanism, preserving the "failed" state while
preventing raw stack traces, URLs, or request IDs from reaching model-visible
guidance.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (result.type === "file" && (result.provisional || result.emptyPages?.length)) { | ||
| situations.add("provisional"); | ||
| } |
There was a problem hiding this comment.
emptyPages describes pages that returned no extracted content, including genuinely blank or gap-filled pages after the enhanced projection is complete. Treating a non-empty emptyPages value as provisional makes a ready final result tell the model that those pages are still extracting and should be read again later.
Only add the provisional situation when result.provisional is true. Keep emptyPages as descriptive metadata for final blank pages rather than inferring that extraction remains in progress.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
Narrow runtime repro source for a final blank PDF page
- Authored TypeScript repro constructs a ready PDF result with `emptyPages: [7]` and no provisional flag, then asserts the parent and PR model outputs; it captures the exact disputed path.
Parent commit output for a final blank PDF page
- Executed the repro against the parent commit; the ready final blank page returned without guidance, establishing the before behavior.
PR output falsely says a final blank PDF page is still extracting
- Executed the repro against the PR commit; the same final blank page emitted retry/extraction guidance, proving the misleading model-visible behavior.
Focused workspace read-reference test run
- Ran the focused existing Vitest suite successfully with 9 passing tests; the current tests do not cover a final `emptyPages` result without `provisional`.
Greptile SummaryThis change streamlines extraction providers and adds projection-readiness, failed-extraction, provisional-page, and model-guidance details to workspace reads. A ready file result with final blank or gap-filled pages is incorrectly described to the model as still extracting. The reproduced path is in Do not merge until provisional guidance is emitted only when the result is explicitly provisional. Confidence Score: 3/5Not safe to merge until T-Rex findings are addressed. T-Rex reproduced 2 failing behaviors at runtime in src/features/workspaces/content/workspace-read-references.ts; the change needs fixes before it is safe to merge. Files Needing Attention: src/features/workspaces/content/workspace-read-references.ts
T-Rex LogsWhat T-Rex did
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/content/workspace-content-reader.ts">
<violation number="1" location="src/features/workspaces/content/workspace-content-reader.ts:236">
P2: Completed projections with intentionally blank pages are reported as still extracting. Only expose `emptyPages` for provisional projections (or change downstream guidance) so a final blank page is not retried indefinitely.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return { | ||
| assetKind: fileType.assetKind, | ||
| content: pageRead.content, | ||
| ...(pageRead.emptyPages.length > 0 ? { emptyPages: pageRead.emptyPages } : {}), |
There was a problem hiding this comment.
P2: Completed projections with intentionally blank pages are reported as still extracting. Only expose emptyPages for provisional projections (or change downstream guidance) so a final blank page is not retried indefinitely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/content/workspace-content-reader.ts, line 236:
<comment>Completed projections with intentionally blank pages are reported as still extracting. Only expose `emptyPages` for provisional projections (or change downstream guidance) so a final blank page is not retried indefinitely.</comment>
<file context>
@@ -241,6 +233,7 @@ async function readFile(input: {
return {
assetKind: fileType.assetKind,
content: pageRead.content,
+ ...(pageRead.emptyPages.length > 0 ? { emptyPages: pageRead.emptyPages } : {}),
format: "markdown",
itemId: input.item.id,
</file context>
| ...(pageRead.emptyPages.length > 0 ? { emptyPages: pageRead.emptyPages } : {}), | |
| ...(projection.provisional && pageRead.emptyPages.length > 0 ? { emptyPages: pageRead.emptyPages } : {}), |
emptyPages on a final, non-provisional read describes genuinely blank physical pages. The guidance trigger treated any non-empty emptyPages as provisional, so a completed enhanced PDF with a blank page told the model those pages were still extracting and to read them again later, causing endless retries of a finished read. Gate the provisional guidance on result.provisional alone; emptyPages stays as descriptive metadata. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| return (Object.keys(workspaceReadGuidance) as Array<keyof typeof workspaceReadGuidance>) |
There was a problem hiding this comment.
React Doctor · react-doctor/js-combine-iterations (warning)
This loops over your list twice because .filter().map() makes two passes, so do it in one pass with .reduce() or a for...of loop
Fix → Combine .map().filter() style chains into one pass with .reduce() or a for...of loop, so you only loop over the list once
| content: pages.map(formatProjectionPage).join("\n\n"), | ||
| // Surfaced so callers can distinguish a genuinely blank page from one the | ||
| // fast extraction pass could not read yet. | ||
| emptyPages: pages.filter((page) => page.markdown.length === 0).map((page) => page.pageNumber), |
There was a problem hiding this comment.
React Doctor · react-doctor/js-combine-iterations (warning)
This loops over your list twice because .filter().map() makes two passes, so do it in one pass with .reduce() or a for...of loop
Fix → Combine .map().filter() style chains into one pass with .reduce() or a for...of loop, so you only loop over the list once
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
1 participant
Summary
Why
Workspace reads need to distinguish content that is ready, still processing, or failed. The unused provider paths also obscured the active extraction architecture.
Changes
Testing
pnpm checkpnpm test— 184 tests passedpnpm test:workers— 24 tests passedReview Notes
Start with
workspace-projection-readiness.tsandworkspace-content-reader.ts. This PR contains only the two extraction commits and targetsmain.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes
Refactor