-
Notifications
You must be signed in to change notification settings - Fork 11
refactor(workspaces): simplify extraction and report readiness #696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -51,12 +51,70 @@ export function createWorkspaceReadReferences( | |||||||||||||||||
| return createWorkspaceReferenceRecords(locations); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Guidance for read outcomes the model has to react to, keyed by situation. | ||||||||||||||||||
| * | ||||||||||||||||||
| * These live here rather than in the tool description because none of them | ||||||||||||||||||
| * change how a read is issued, and background extraction states are rare enough | ||||||||||||||||||
| * that every request should not carry the instructions for handling them. | ||||||||||||||||||
| */ | ||||||||||||||||||
| 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: | ||||||||||||||||||
|
Comment on lines
+61
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Use the reported retry delay rather than a literal token.
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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| "Extraction will not finish for some paths. Report the code and any message to the user; do not retry those reads and do not suggest re-uploading the file.", | ||||||||||||||||||
| transient: | ||||||||||||||||||
| "Some paths failed on a transient storage problem. One repeat read is reasonable; if it fails again, tell the user.", | ||||||||||||||||||
| provisional: | ||||||||||||||||||
| "Some content came from a fast first pass. Pages listed in emptyPages are still extracting, so read them again later rather than reporting them as blank.", | ||||||||||||||||||
| } as const; | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Collects the handling guidance a set of read results calls for. | ||||||||||||||||||
| * | ||||||||||||||||||
| * Emitted once per situation rather than once per result so a batch of pending | ||||||||||||||||||
| * reads does not repeat the same paragraph for every path. | ||||||||||||||||||
| * | ||||||||||||||||||
| * @param results - Ordered workspace read results. | ||||||||||||||||||
| * @returns Guidance lines for the situations present, most actionable first. | ||||||||||||||||||
| */ | ||||||||||||||||||
| function createWorkspaceReadGuidance(results: readonly WorkspaceContentReadResult[]): string[] { | ||||||||||||||||||
| const situations = new Set<keyof typeof workspaceReadGuidance>(); | ||||||||||||||||||
|
|
||||||||||||||||||
| for (const result of results) { | ||||||||||||||||||
| if (result.status === "pending") { | ||||||||||||||||||
| situations.add("pending"); | ||||||||||||||||||
| continue; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| if (result.status === "failed") { | ||||||||||||||||||
| if (result.code === "extraction_failed" || result.code === "extraction_stalled") { | ||||||||||||||||||
| situations.add("unrecoverable"); | ||||||||||||||||||
| } else if (result.code === "projection_failed") { | ||||||||||||||||||
| situations.add("transient"); | ||||||||||||||||||
| } | ||||||||||||||||||
| continue; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // Gate on provisional only: emptyPages on a final (non-provisional) read | ||||||||||||||||||
| // describes genuinely blank pages, which must not be reported as still | ||||||||||||||||||
| // extracting or the model retries a completed read forever. | ||||||||||||||||||
| if (result.type === "file" && result.provisional) { | ||||||||||||||||||
| situations.add("provisional"); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| return (Object.keys(workspaceReadGuidance) as Array<keyof typeof workspaceReadGuidance>) | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. React Doctor · 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 |
||||||||||||||||||
| .filter((situation) => situations.has(situation)) | ||||||||||||||||||
| .map((situation) => workspaceReadGuidance[situation]); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Projects a rich workspace read result into compact model-visible JSON. | ||||||||||||||||||
| * | ||||||||||||||||||
| * Raw item IDs and durable locations remain in the persisted tool result. | ||||||||||||||||||
| * Model-visible content receives only opaque refs next to the content they | ||||||||||||||||||
| * identify. | ||||||||||||||||||
| * identify, plus guidance for any outcome that needs handling. | ||||||||||||||||||
| * | ||||||||||||||||||
| * @param output - Validated rich workspace read output. | ||||||||||||||||||
| * @returns JSON-safe results annotated with short workspace refs. | ||||||||||||||||||
|
|
@@ -65,8 +123,10 @@ export function createWorkspaceReadItemsModelOutput(output: WorkspaceReadItemsOu | |||||||||||||||||
| const refsByLocation = new Map( | ||||||||||||||||||
| output.references.map((record) => [getWorkspaceLocationKey(record.location), record.ref]), | ||||||||||||||||||
| ); | ||||||||||||||||||
| const guidance = createWorkspaceReadGuidance(output.results); | ||||||||||||||||||
|
|
||||||||||||||||||
| return { | ||||||||||||||||||
| ...(guidance.length > 0 ? { guidance } : {}), | ||||||||||||||||||
| results: output.results.map((result) => { | ||||||||||||||||||
| if (result.status !== "ready") { | ||||||||||||||||||
| return result; | ||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Completed projections with intentionally blank pages are reported as still extracting. Only expose
emptyPagesfor provisional projections (or change downstream guidance) so a final blank page is not retried indefinitely.Prompt for AI agents