Skip to content

fix(browser): avoid attachment menu coordinate clicks - #431

Open
oraclexing wants to merge 1 commit into
steipete:mainfrom
oraclexing:fix/browser-attachment-plus-safety
Open

fix(browser): avoid attachment menu coordinate clicks#431
oraclexing wants to merge 1 commit into
steipete:mainfrom
oraclexing:fix/browser-attachment-plus-safety

Conversation

@oraclexing

Copy link
Copy Markdown

Summary

  • activate ChatGPT's attachment menu through the exact composer + control
  • replace trusted page-coordinate clicks with focused trusted keyboard activation
  • abort before prompt submission if activation enters Work or another conversation
  • document and test the Work-home attachment regression path

Why

#315 / #316 normalize ChatGPT Work to Chat during startup, but attachment upload later used broad fallback selectors plus a trusted mouse click at a computed page coordinate. On the current Work home, suggestion cards sit close to the composer.

In a sanitized live failure, an attachment-bearing Oracle run had promptSubmitted: false; an unrelated Work suggestion became a new task about 18 seconds after the run began, and Oracle then failed one attachment-readiness timeout later. No separate browser-control call occurred in the invoking turn. The timing strongly implicates attachment-menu activation, although the exact misdirected pointer event was not captured.

This patch removes that coordinate-click hazard. It targets only #composer-plus-btn / button[data-testid="composer-plus-btn"], focuses the control, sends trusted Enter key events, and keeps only an exact-selector synthetic fallback. A post-activation guard rejects Work or unexpected conversation navigation.

Validation

Exact head: 7b753df7975eaed9bf8914ef6670c9f5640f942b.

  • pnpm exec vitest run tests/browser/pageActions.test.ts — 98 passed, 1 skipped
  • pnpm run lint — passed
  • pnpm test — 1,873 passed, 53 skipped
  • pnpm run build — passed
  • pnpm run docs:check — passed (77 flags, 6 files)

Signed-in browser smoke

A compiled exact-head overlay ran an Instant GPT-5.6 Sol consult with one uploaded LICENSE file:

  • session: attachment-safety-final-smoke-20260828
  • attachment UI became ready and the prompt submitted to an ordinary /c/... conversation
  • answer: ORACLE_ATTACHMENT_SAFETY_FINAL_OK
  • completed in 39.2s and archived the smoke conversation
  • post-run sidebar inspection still showed only the two pre-existing Work entries; no new Work task appeared

After restarting Codex, the installed MCP overlay was also verified through detached consult plus a single blocking wait:

  • session: oracle-mcp-attachment-restart-smoke
  • answer: ORACLE_MCP_ATTACHMENT_RESTART_OK
  • completed with one real attachment and archived successfully
  • post-run sidebar inspection again showed no new Work task

An explicit navigation to an existing Work conversation also failed closed before attachment upload or prompt submission.

@clawsweeper

clawsweeper Bot commented Aug 28, 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: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 28, 2026
@clawsweeper

clawsweeper Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed August 28, 2026, 9:26 AM ET / 13:26 UTC.

ClawSweeper review

What this changes

The PR replaces broad coordinate-based attachment-menu clicks with focused activation of the exact ChatGPT composer control and adds a Work/navigation safety guard.

Regression provenance

Possible regression — probable (reviewed change; known regression link). No predecessor PR is attributed.

Merge readiness

Blocked by patch quality or review findings - 4 items remain

The branch has a concrete safety gap: its new post-activation guard does not recognize project-scoped ChatGPT conversations, so it can miss navigation into another project conversation.

Priority: P1
Reviewed head: 7b753df7975eaed9bf8914ef6670c9f5640f942b

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) PR readiness rating was derived from proof quality, review findings, security review, and reviewer confidence.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): ClawSweeper live verification passed on the reviewed terminal surface.
Patch quality 🧂 unranked krab (1/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): ClawSweeper live verification passed on the reviewed terminal surface.
Evidence reviewed 3 items Introduced guard defect: The new guard anchors its URL pattern at /c/, so distinct project routes such as /g/.../c/a and /g/.../c/b both yield null and bypass the unexpected-conversation rejection.
Supported project-route behavior: Current browser navigation code intentionally extracts a conversation ID from any /c/ path segment, including project-scoped routes.
Feature provenance: The merged Work-mode feature explicitly covered project-scoped conversations.
Findings 1 actionable finding [P1] Handle project-scoped conversation URLs in the navigation guard
Security None None.

Live Verification

Command: pnpm exec vitest run tests/browser/pageActions.test.ts

Result: PASS (completed)


 RUN  v4.1.11 /tmp/clawsweeper-live-proof-431-Yufj1E/target

 ✓ tests/browser/pageActions.test.ts (99 tests | 1 skipped) 8581ms
     ✓ weak-only evidence that resolves into the app shell is NOT a challenge  1001ms
     ✓ weak-only evidence that persists through the grace window IS a challenge  1502ms
     ✓ treats welcome-back navigation during account click as login progress  1502ms
     ✓ falls back to snapshot when observer fails  1204ms
     ✓ throws when file input missing  354ms
     ✓ skips upload when attachment already present (ellipsis-aware detection)  353ms
     ✓ skips reupload when file already queued in input  352ms
     ✓ skips upload when file count already satisfies expected count  352ms
     ✓ skips upload when input count already satisfies expected count  352ms
     ✓ avoids retrying other inputs once upload shows progress  356ms
     ✓ defers data transfer fallback when attachment signals appear after setFileInputFiles  352ms

 Test Files  1 passed (1)
      Tests  98 passed | 1 skipped (99)
   Start at  13:27:22
   Duration  9.13s (transform 279ms, setup 37ms, import 373ms, tests 8.58s, environment 0ms)






























Assertions:

  • PASS expect_output: Tests

How this fits together

Oracle’s browser mode drives a signed-in ChatGPT tab through Chrome DevTools Protocol. Attachment upload opens the composer menu before transferring a local file and then waits for the upload UI to become ready.

flowchart LR
A[Oracle browser consult] --> B[ChatGPT composer]
B --> C[Exact attachment control]
C --> D[Attachment menu activation]
D --> E[Navigation safety check]
E --> F[File upload and prompt submission]
Loading

Before merge

  • Handle project-scoped conversation URLs in the navigation guard (P1) - The new guard only extracts IDs from paths beginning with /c/. Oracle supports project chats at /g/.../c/<id>, so a transition between project conversations yields null for both URLs and unexpectedConversation stays false. The upload can then continue despite this PR’s stop-before-submit guarantee. Use the project-aware /c/ extraction already used by browser navigation and add that regression case.
  • Resolve merge risk (P1) - Project-scoped browser chats can still move to another conversation after attachment activation and continue toward upload or submission despite the intended fail-closed guarantee.
  • Complete next step (P2) - A narrow introduced P1 guard defect has a clear source-level repair and regression-test boundary.
  • Improve patch quality - Address the highest-priority review finding and re-run the changed-surface validation.

Findings

  • [P1] Handle project-scoped conversation URLs in the navigation guard — src/browser/actions/attachments.ts:187-198
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed surface 5 files; production +187/-66, tests +74 Most of the change is browser-automation safety logic, with focused unit coverage and operator documentation.

Merge-risk options

Maintainer options:

  1. Cover project conversation navigation (recommended)
    Make the post-activation guard compare IDs in root and project-scoped URLs, then prove it rejects a changed project conversation before upload.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Use the shared project-aware conversation-ID extraction for the attachment navigation guard and add root and project route regression coverage.

Technical review

Best possible solution:

Use the repository’s project-aware conversation-ID extraction in the post-activation guard and add a regression test for a changed project conversation.

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

Yes, from source: give the new guard distinct project-scoped /g/.../c/<id> start and current URLs; its anchored parser returns null for both and does not reject.

Is this the best way to solve the issue?

No; exact composer activation is appropriate, but the guard must use project-aware route handling to fulfill its fail-closed behavior.

Full review comments:

  • [P1] Handle project-scoped conversation URLs in the navigation guard — src/browser/actions/attachments.ts:187-198
    The new guard only extracts IDs from paths beginning with /c/. Oracle supports project chats at /g/.../c/<id>, so a transition between project conversations yields null for both URLs and unexpectedConversation stays false. The upload can then continue despite this PR’s stop-before-submit guarantee. Use the project-aware /c/ extraction already used by browser navigation and add that regression case.
    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 fa0e6fd669ad.

Labels

Label changes:

  • add P1: The new safeguard can let an attachment-bearing browser run proceed in an unintended project conversation.
  • add merge-risk: 🚨 message-delivery: An unrecognized project-route navigation can send an attachment-bearing prompt into the wrong ChatGPT conversation.
  • add proof: sufficient: Contributor real behavior proof is sufficient. ClawSweeper live verification passed on the reviewed terminal surface.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦞 diamond lobster and patch quality is 🧂 unranked krab.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): ClawSweeper live verification passed on the reviewed terminal surface.

Label justifications:

  • P1: The new safeguard can let an attachment-bearing browser run proceed in an unintended project conversation.
  • merge-risk: 🚨 message-delivery: An unrecognized project-route navigation can send an attachment-bearing prompt into the wrong ChatGPT conversation.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦞 diamond lobster and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): ClawSweeper live verification passed on the reviewed terminal surface.
  • proof: sufficient: Contributor real behavior proof is sufficient. ClawSweeper live verification passed on the reviewed terminal surface.

Evidence

Acceptance criteria:

  • [P1] pnpm exec vitest run tests/browser/pageActions.test.ts.
  • [P1] pnpm run lint.

What I checked:

Likely related people:

  • DragonFSKY: Authored the merged ChatGPT Work-to-Chat feature that established project-scoped conversation handling. (role: feature owner; confidence: high; commits: f7ab9d39f11f; files: src/browser/actions/navigation.ts, tests/browser/pageActions.test.ts)
  • Peter Steinberger: Merged the Work-mode feature and authored the current main attachment-related fix beneath this PR’s base. (role: recent area contributor; confidence: medium; commits: f7ab9d39f11f, fa0e6fd669ad; files: src/browser/actions/attachments.ts)

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: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant