Skip to content

fix(browser): bundle multiple text uploads - #420

Open
gwelinder wants to merge 1 commit into
steipete:mainfrom
gwelinder:fix/frog-browser-upload-bundle-20260826
Open

fix(browser): bundle multiple text uploads#420
gwelinder wants to merge 1 commit into
steipete:mainfrom
gwelinder:fix/frog-browser-upload-bundle-20260826

Conversation

@gwelinder

Copy link
Copy Markdown

Summary

  • Automatically combine multiple uploaded text files into one ordered, source-labelled text bundle while keeping a single text file under its original attachment name.
  • Preserve explicit attachment/bundle modes and formats. Mixed raw inputs remain byte-preserving direct uploads unless explicit bundling or the platform attachment limit requires ZIP.
  • Include expected names, missing UI/input names, send state, upload state, file count, and observed input names in direct-upload timeout errors.

Root cause

The automatic policy bundled only after the 10-attachment platform limit. Nine Markdown files therefore took nine sequential upload/readiness paths. Completion correctly required all expected names and a ready send control, but if the ninth attachment never appeared, Oracle waited out the entire budget and then returned a generic timeout with no stalled filename or final readiness state.

The new boundary is semantic rather than incident-specific: one text file remains one attachment; multiple text files form one lossless labelled corpus.

Validation

  • pnpm run check
  • pnpm vitest run tests/browser/policies.test.ts tests/browser/prompt.test.ts tests/browser/attachmentsCompletion.test.ts tests/browser/pageActions.test.ts tests/browser/promptComposerExpressions.test.ts tests/browser/index.test.ts tests/browser/sessionRunner.test.ts — 277 passed, 1 skipped
  • pnpm test — 1,795 passed, 43 skipped
  • pnpm run build
  • pnpm docs:check

The mocked browser regression reproduces an 8-of-9 Markdown readiness stall, verifies the exact missing ninth filename/state, and proves the generated one-bundle upload reaches ready state. No Chrome smoke was run because Frog 20260826151633 explicitly requires this fix without launching Chrome.

Frog: 20260826151633

@clawsweeper

clawsweeper Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 26, 2026
@clawsweeper

clawsweeper Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed August 26, 2026, 9:56 AM ET / 13:56 UTC.

ClawSweeper review

What this changes

The PR bundles multiple text uploads into one ordered, source-labeled text attachment, retains single-file and raw-file behavior, and improves attachment-timeout diagnostics.

Regression provenance

Possible regression — probable (reviewed change; failure trace). No predecessor PR is attributed.

Merge readiness

Blocked until real behavior proof from a real setup is added - 5 items remain

Keep this PR open: automatic fallback bundling now writes a temporary corpus for ordinary successful inline multi-file runs, but no cleanup path exists; it also needs real browser proof before merge.

Priority: P2
Reviewed head: 2f3b43ec9e8be1ea3098a79537503347010bab00

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch has focused tests, but a concrete temporary-artifact leak and missing real-browser proof keep it from merge-ready quality.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The PR supplies mocked browser tests and CI-style commands but no after-fix run in a real browser setup; add redacted live output, logs, or a recording showing the ready composer state. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR supplies mocked browser tests and CI-style commands but no after-fix run in a real browser setup; add redacted live output, logs, or a recording showing the ready composer state. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 4 items Fallback bundle is materialized eagerly: For automatic inline prompts, the new upload plan treats two or more text files as bundle-worthy; fallback assembly immediately writes that bundle even before the inline submission outcome is known.
Generated bundle has no production cleanup owner: Bundles are created under os.tmpdir(), and repository-wide search found no production cleanup path for oracle-browser-bundle artifacts; the new test manually removes its fallback bundle, confirming the artifact persists otherwise.
Browser attachment history: Current feature history identifies Peter Steinberger as the main long-term contributor to prompt assembly and original attachment bundling, including the original bundle-over-limit work.
Findings 1 actionable finding [P2] Defer writing the unused fallback bundle
Security None None.

How this fits together

Oracle browser mode converts CLI file inputs into pasted composer text or uploaded attachments before submitting a ChatGPT prompt. The attachment policy determines the upload shape, and readiness checks decide when the composer can send.

flowchart LR
  A[CLI file inputs] --> B[Prompt assembly]
  B --> C[Inline or attachment policy]
  C --> D[Generated text bundle]
  C --> E[Browser composer]
  D --> E
  E --> F[Readiness check]
  F --> G[Prompt submission]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR supplies mocked browser tests and CI-style commands but no after-fix run in a real browser setup; add redacted live output, logs, or a recording showing the ready composer state. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Defer writing the unused fallback bundle (P2) - With two small text files in auto mode, the initial inline submission normally succeeds, but uploadPlan.shouldBundle now makes this fallback path write a new oracle-browser-bundle-* file first. Nothing in production removes that directory, so every such successful run leaves a copy of the source corpus in temporary storage. Materialize it only when fallback is selected, or add lifecycle cleanup and a regression test.
  • Resolve merge risk (P2) - Successful auto-mode runs with two or more small text files now create a source-content bundle in the temporary directory even when fallback upload is never used; repeated runs can retain user content and consume disk space.
  • Resolve merge risk (P1) - The PR has no after-fix real-browser evidence for the changed ChatGPT composer upload path.
  • Complete next step (P2) - The concrete cleanup fix is clear, but the external contributor must also provide real-browser evidence before a maintainer can merge it.

Findings

  • [P2] Defer writing the unused fallback bundle — src/browser/prompt.ts:473
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Code and test delta production +94/-20, tests +192/-47, docs +5/-4 The implementation changes prompt assembly, upload policy, and readiness diagnostics with substantial focused regression coverage.

Merge-risk options

Maintainer options:

  1. Defer unused fallback materialization (recommended)
    Create the fallback bundle only when the inline submission actually fails, or clean it up on every non-fallback completion path.

Technical review

Best possible solution:

Create the fallback bundle only when fallback submission is selected, or give generated fallback artifacts a verified cleanup lifecycle, then add redacted real-browser proof that the bundled upload reaches a ready composer.

Do we have a high-confidence way to reproduce the issue?

No real-browser reproduction was provided; however, source establishes that ordinary two-file auto-mode fallback construction writes a bundle before fallback selection and no cleanup owner exists.

Is this the best way to solve the issue?

No; preserve semantic bundling but defer fallback file creation until needed or clean generated artifacts deterministically after submission.

Full review comments:

  • [P2] Defer writing the unused fallback bundle — src/browser/prompt.ts:473
    With two small text files in auto mode, the initial inline submission normally succeeds, but uploadPlan.shouldBundle now makes this fallback path write a new oracle-browser-bundle-* file first. Nothing in production removes that directory, so every such successful run leaves a copy of the source corpus in temporary storage. Materialize it only when fallback is selected, or add lifecycle cleanup and a regression test.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 79e483bd9dc8.

Labels

Label changes:

  • add P2: This is a user-visible browser upload reliability change with bounded impact.
  • add merge-risk: 🚨 availability: The new routine fallback path can leave generated bundles in temporary storage after successful inline submissions.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR supplies mocked browser tests and CI-style commands but no after-fix run in a real browser setup; add redacted live output, logs, or a recording showing the ready composer state. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: This is a user-visible browser upload reliability change with bounded impact.
  • merge-risk: 🚨 availability: The new routine fallback path can leave generated bundles in temporary storage after successful inline submissions.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR supplies mocked browser tests and CI-style commands but no after-fix run in a real browser setup; add redacted live output, logs, or a recording showing the ready composer state. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Fallback bundle is materialized eagerly: For automatic inline prompts, the new upload plan treats two or more text files as bundle-worthy; fallback assembly immediately writes that bundle even before the inline submission outcome is known. (src/browser/prompt.ts:473, 2f3b43ec9e8b)
  • Generated bundle has no production cleanup owner: Bundles are created under os.tmpdir(), and repository-wide search found no production cleanup path for oracle-browser-bundle artifacts; the new test manually removes its fallback bundle, confirming the artifact persists otherwise. (src/browser/prompt.ts:248, 2f3b43ec9e8b)
  • Browser attachment history: Current feature history identifies Peter Steinberger as the main long-term contributor to prompt assembly and original attachment bundling, including the original bundle-over-limit work. (src/browser/prompt.ts:243, 1d7c3fb8782b)
  • Contributor evidence is mock-only: The PR body reports mocked browser regressions and test suites, and explicitly says no Chrome smoke ran; that does not demonstrate after-fix behavior in a real browser setup. (2f3b43ec9e8b)

Likely related people:

  • Peter Steinberger: Long-term primary contributor to browser prompt assembly and author of the original attachment-bundling history. (role: feature-history owner; confidence: high; commits: 1d7c3fb8782b, 2b8290a489a3; files: src/browser/prompt.ts, src/browser/policies.ts)
  • postoso: Authored the merged duplicate-attachment-basename handling that this PR replaces with automatic text bundling. (role: adjacent contributor; confidence: medium; commits: c48aa1de0a94; files: src/browser/prompt.ts, tests/browser/prompt.test.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Defer or clean up generated fallback bundles and add a regression test for a successful inline multi-file run.
  • Add redacted after-fix real-browser proof that multiple text files upload as one ready bundle.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant