Skip to content

fix(gh-comment-attach-files): retry uploads and fix URL detection#457

Open
shunk031 wants to merge 4 commits into
mainfrom
fix/gh-comment-attach-files-retry-and-url-detection
Open

fix(gh-comment-attach-files): retry uploads and fix URL detection#457
shunk031 wants to merge 4 commits into
mainfrom
fix/gh-comment-attach-files-retry-and-url-detection

Conversation

@shunk031

@shunk031 shunk031 commented May 26, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add up to 3 retries for file uploads
  • Fix false-positive bug in URL detection logic
  • Refactor perform_upload() to return results instead of raising immediately
  • Improve robustness of run_playwright_value()
  • Default to headless browser; add --headed opt-in flag
  • Detect GitHub login redirects and surface a clear error message

Changes

1. Retry logic (upload_files)

Handles upload failures in unstable environments (CI, slow networks, etc.).
Retries up to 3 times with appropriate recovery per failure type:

Failure Recovery
CalledProcessError (crash) Wait 30s → reopen browser
upload_result.ok == false Wait 60s → reload page
URL not found Wait 30s → retry

2. URL detection false-positive fix

Problem: GitHub inserts a temporary placeholder ![Uploading stagedName…]() into the textarea immediately after receiving a file. The old implementation matched on stagedName and incorrectly treated this as a completed upload.

Fix: Drop the stagedName check; instead compare the count of URL hint occurrences before and after the upload:

// Before
return value.includes(stagedName) || urlHints.some(h => value.includes(h));

// After
const countHints = (text) => urlHints.reduce((sum, h) => sum + (text.split(h).length - 1), 0);
return countHints(value) > countHints(previous);

3. Headless-by-default with --headed opt-in

Previously the browser always launched in headed (visible) mode, which interrupted other work with unexpected windows.

  • Default: headless — no browser window appears during normal use.
  • --headed: opt-in flag for when a GitHub login is needed.
  • Login detection: if wait_for_comment_composer detects a /login or /session redirect, it exits immediately with a clear message pointing the user to --headed.

4. Other

  • perform_upload(): return result instead of raising; retry control consolidated in caller
  • run_playwright_value(): handle "undefined" stdout and JSONDecodeError
  • wait_for_comment_composer(): unify error message to a single line
  • import sys moved to top-level; remove __import__("sys") workaround

🤖 Generated with Claude Code

- upload_files() に最大 3 回のリトライを追加。アップロードの
  クラッシュ・失敗・URL 未検出をそれぞれハンドリングし、
  ブラウザの再起動も行う
- URL 検出ロジックを修正: stagedName チェックは GitHub が挿入する
  "![Uploading stagedName…]()" プレースホルダーで誤検知するため、
  URL ヒントの出現回数を数える方式に変更
- perform_upload() は即時 raise せず結果を返すように変更
  (リトライはcallerのupload_files()が制御)
- run_playwright_value() で "undefined" stdout と
  JSONDecodeError を適切にハンドリング
- wait_for_comment_composer() のエラーメッセージを 1 行に統一

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@codecov

codecov Bot commented May 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.79%. Comparing base (520d850) to head (810ed77).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #457      +/-   ##
==========================================
- Coverage   88.93%   88.79%   -0.15%     
==========================================
  Files           9        9              
  Lines         235      232       -3     
==========================================
- Hits          209      206       -3     
  Misses         26       26              
Flag Coverage Δ
macos-14-client 41.66% <ø> (ø)
ubuntu-latest-client 88.36% <ø> (+1.66%) ⬆️
ubuntu-latest-server 88.74% <ø> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

shunk031 and others added 3 commits May 26, 2026 23:54
…in and login detection

- Default to headless browser (removes always-on --headed flag)
- Add --headed CLI flag for cases where login is required
- Detect /login or /session redirect in wait_for_comment_composer and raise a clear error directing user to --headed
- Thread headed param through upload_files and open_browser call in retry path
- Move import sys to top-level (was inline in upload_files); clean up __import__("sys") hack

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…aded is set

When running with --headed, the user may be in the middle of logging in.
Skipping the login detection error in headed mode lets the polling continue
until the user completes authentication and lands on the target page.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant