Record bounty details when a PR with a bounty is merged#5468
Record bounty details when a PR with a bounty is merged#5468SuyashJain17 wants to merge 14 commits intoOWASP-BLT:mainfrom
Conversation
|
👋 Hi @SuyashJain17! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces live GitHub Sponsors payouts with a recording workflow: the GitHub Action posts bounty details to a BLT API; backend validates input, locks the issue row, and records a Changes
Sequence Diagram(s)sequenceDiagram
participant GH_Action as GH Action
participant BLT_API as BLT API
participant Backend as Website Backend
participant DB as Database
GH_Action->>BLT_API: POST /bounty_payout (issue_number, repo, owner, amount, contributor, pr_number) + X-BLT-API-TOKEN
BLT_API->>Backend: Forward request (validate token & payload)
Backend->>Backend: Validate input (amount bounds, owner/repo/username formats)
Backend->>DB: SELECT ... FOR UPDATE (lock issue row)
Backend->>DB: UPDATE issue set sponsors_tx_id = "BOUNTY:<contributor>:<amount>:<pr_number>"
Backend-->>BLT_API: 200 OK (recorded status, issue_number, amount, recipient)
BLT_API-->>GH_Action: Workflow success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
📊 Monthly LeaderboardHi @SuyashJain17! Here's how you rank for January 2026:
Leaderboard based on contributions in January 2026. Keep up the great work! 🚀 |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
.github/workflows/bounty-payout.yml
Outdated
| throw new Error(`Request failed: ${res.statusCode}`); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const errorText = await response.text(); | ||
| throw new Error(`Bounty payout request failed: ${response.status} - ${errorText}`); | ||
| } | ||
| req.on('error', (e) => { | ||
| throw new Error(`Request error: ${e.message}`); | ||
| }); | ||
|
|
||
| const result = await response.json(); | ||
| console.log('Bounty payout request successful:', result); | ||
| req.write(data); | ||
| req.end(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/views/bounty.py (1)
74-89: Potential race condition in duplicate check.There's a TOCTOU (time-of-check-time-of-use) gap between checking
sponsors_tx_idand saving. Two concurrent requests could both pass the check. In practice, this is low risk since workflow triggers are per-PR-merge, but for robustness consider usingselect_for_update():♻️ Optional fix using select_for_update
- github_issue = GitHubIssue.objects.filter(issue_id=issue_number, repo=repo).first() + github_issue = GitHubIssue.objects.select_for_update().filter(issue_id=issue_number, repo=repo).first()Note: This requires the view to be wrapped in a transaction (e.g.,
@transaction.atomic).
🤖 Fix all issues with AI agents
In @.github/workflows/bounty-payout.yml:
- Around line 127-144: The https.request callback is asynchronous and its throw
statements inside res.on('end') and req.on('error') won’t surface to the Actions
step; wrap the whole request sequence in a Promise, return that Promise, call
reject(new Error(...)) instead of throw when res.statusCode >= 400 or on
req.on('error'), and call resolve() when the response is successful (e.g., 2xx)
after res.on('end'); ensure you perform req.write(data) and req.end() inside the
Promise executor. Use the existing https.request(options, (res) => { ... })
callback, res.on('data'), res.on('end'), req.on('error'), req.write(data) and
req.end() but convert their error/success handling to reject/resolve and return
the Promise so the GitHub Actions step can await failures.
🧹 Nitpick comments (3)
.github/workflows/bounty-payout.yml (1)
122-122: UseBuffer.byteLength()for accurate Content-Length.
data.lengthreturns character count, not byte count. While GitHub usernames are ASCII-only (so this works in practice), usingBuffer.byteLength(data)is more correct for HTTP headers.♻️ Suggested fix
- 'Content-Length': data.length, + 'Content-Length': Buffer.byteLength(data),website/tests/test_bounty.py (2)
66-68: Consider asserting thesponsors_tx_idformat.The test only checks
sponsors_tx_idis not None. Given the view stores a specific format (BOUNTY:username:amount:pr), consider asserting the expected value for stronger test coverage.♻️ Suggested enhancement
# Verify database was updated self.issue.refresh_from_db() - self.assertIsNotNone(self.issue.sponsors_tx_id) + self.assertEqual(self.issue.sponsors_tx_id, "BOUNTY:testuser:5000:456")
10-11: Consider adding tests for edge cases.The test suite covers main paths well but could benefit from:
- Invalid numeric fields (e.g.,
"bounty_amount": "not_a_number") to test the ValueError handling at lines 51-57 inbounty.py.- Invalid JSON body to test the JSONDecodeError handling.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
.github/workflows/bounty-payout.ymlwebsite/tests/test_bounty.pywebsite/views/bounty.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (7)
website/tests/test_bounty.py (2)
70-94: LGTM!The duplicate detection test correctly pre-populates
sponsors_tx_idand verifies the warning response. The test aligns well with the view's duplicate handling logic.
96-171: LGTM!The error handling tests provide good coverage for authentication failures, missing fields, and entity lookup failures. The test cases align with the view's error handling paths.
website/views/bounty.py (4)
26-35: LGTM!Good security practices: constant-time comparison via
secrets.compare_digestprevents timing attacks, and proper handling of missing configuration returns a 500 without leaking details.
86-94: LGTM!The bounty recording logic is clean. The
sponsors_tx_idformat includes all necessary information for manual processing, and the logging provides good audit trail without exposing sensitive data.
44-61: LGTM!Input validation is thorough: required field checking, numeric type validation, and proper error responses. The extracted fields are used safely without injection risks.
74-84: LGTM!Good idempotency handling - returning a 200 with a warning status for duplicates ensures the workflow doesn't fail on retries while clearly indicating the bounty was already recorded.
.github/workflows/bounty-payout.yml (1)
69-78: Bounty amount regex correctly requires two decimal places for consistency.The regex
/\$(\d+(?:\.\d{2})?)/intentionally requires exactly two decimal places (or no decimals), matching patterns like$50and$25.50but not$25.5. This aligns with the code comment explicitly documenting both formats and standard currency representation. The cents conversion (* 100) confirms this is designed for standard two-decimal currency amounts. No evidence of single-decimal label formats (e.g.,$25.5) being used in the codebase.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
SuyashJain17
force-pushed
the
feat/bounty-recording
branch
from
f818915 to
a738e65
Compare
github-actions
bot
added
changes-requested
❌ Console Statements DetectedFound 2 file(s) with Why remove console statements?
Files with violations:website/static/js/issue.js: website/static/organization/js/hunt_controller.js: How to fix:
Note:
Please remove all console statements and push your changes. |
github-actions
bot
added
has-console-statements
website/views/bounty.py
Outdated
| status=200, | ||
| ) | ||
|
|
||
| # Process payment via GitHub Sponsors API | ||
| transaction_id = process_github_sponsors_payment( | ||
| username=contributor_username, | ||
| amount=bounty_amount, | ||
| note=f"Bounty for PR #{pr_number} resolving issue #{issue_number} in {owner_name}/{repo_name}", | ||
| ) | ||
|
|
||
| if not transaction_id: | ||
| logger.error(f"Failed to process GitHub Sponsors payment for issue #{issue_number}") | ||
| return JsonResponse({"status": "error", "message": "Payment processing failed"}, status=500) | ||
| # Record the bounty for manual payment via GitHub Sponsors | ||
| # Format: BOUNTY:<contributor>:<amount_cents>:<pr> | ||
| github_issue.sponsors_tx_id = f"BOUNTY:{contributor_username}:{bounty_amount}:{pr_number}" | ||
| github_issue.save() | ||
|
|
||
| logger.info( | ||
| f"Successfully processed bounty payment: ${bounty_amount / 100:.2f} to {contributor_username} " | ||
| f"Recorded bounty: ${bounty_amount / 100:.2f} to {contributor_username} " |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/static/js/issue.js (1)
387-389: Remove console statement flagged by CI pipeline.The pipeline failure indicates
console.errorusage at line 388. Consider using a structured logging approach or removing the console statement to pass the CI check.Suggested fix
} catch (error) { - console.error('Error fetching issues:', error); if (!append) { hideSuggestionBox(); } else {Alternatively, if error visibility is needed, consider using a centralized error reporting mechanism similar to
safeLogingsoc_pr_report.js.
🤖 Fix all issues with AI agents
In @.github/workflows/bounty-payout.yml:
- Around line 120-124: The Content-Length header is computed using data.length
which counts UTF-16 code units and will be wrong for non-ASCII
contributor_username; update the headers object to compute the byte length with
Buffer.byteLength(data, 'utf8') (replace data.length with
Buffer.byteLength(data, 'utf8')) so the Content-Length value reflects actual
bytes sent.
♻️ Duplicate comments (1)
.github/workflows/bounty-payout.yml (1)
127-144: Critical: Asynchttps.requestis not awaited - workflow will complete before request finishes.The callback-based
https.requestis asynchronous. The script exits immediately afterreq.end()without waiting for the response. Errors thrown in callbacks (res.on('end'),req.on('error')) won't propagate to the workflow step - the step will always succeed regardless of the actual HTTP outcome.🔧 Wrap in Promise and await
- const req = https.request(options, (res) => { - let body = ''; - res.on('data', chunk => body += chunk); - res.on('end', () => { - console.log(`Status: ${res.statusCode}`); - console.log(`Response: ${body}`); - if (res.statusCode >= 400) { - throw new Error(`Request failed: ${res.statusCode}`); - } - }); - }); - - req.on('error', (e) => { - throw new Error(`Request error: ${e.message}`); - }); - - req.write(data); - req.end(); + await new Promise((resolve, reject) => { + const req = https.request(options, (res) => { + let body = ''; + res.on('data', chunk => body += chunk); + res.on('end', () => { + console.log(`Status: ${res.statusCode}`); + console.log(`Response: ${body}`); + if (res.statusCode >= 400) { + reject(new Error(`Request failed: ${res.statusCode} - ${body}`)); + } else { + resolve(body); + } + }); + }); + + req.on('error', (e) => { + reject(new Error(`Request error: ${e.message}`)); + }); + + req.write(data); + req.end(); + });
🧹 Nitpick comments (6)
website/documents/BltTerms.md (1)
94-94: Trailing newline fix looks good.The static analysis tool flags the email as a bare URL (MD034). Optionally, you could wrap it in angle brackets to create a proper markdown autolink:
<blt-support@owasp.org>. This is a minor stylistic preference and not blocking.📝 Optional markdown style fix
-Questions about the Terms of Service should be sent to blt-support@owasp.org. +Questions about the Terms of Service should be sent to <blt-support@owasp.org>.website/documents/BltWeeklyActivityOfContributor.md (1)
44-44: LGTM – Documentation formatting change.This appears to be a formatting-only adjustment (likely an end-of-file newline addition) with no functional impact.
Optional style nitpick: The static analysis tool suggests replacing "a large number of" with "many" or "numerous" to reduce wordiness, though the current phrasing is perfectly acceptable for technical documentation.
website/views/bounty.py (1)
50-57: Consider validating thatbounty_amountis positive.The numeric validation ensures the value is an integer but doesn't reject zero or negative amounts, which would be invalid for a bounty.
🛠️ Suggested fix
try: issue_number = int(data["issue_number"]) pr_number = int(data["pr_number"]) bounty_amount = int(data["bounty_amount"]) + if bounty_amount <= 0: + raise ValueError("bounty_amount must be positive") except (ValueError, TypeError): logger.warning("Invalid numeric fields in request") return JsonResponse({"status": "error", "message": "Invalid numeric fields"}, status=400)website/tests/test_bounty.py (3)
66-68: Consider verifying thesponsors_tx_idformat.The test only checks that
sponsors_tx_idis not None. Per the new flow, this field should follow the formatBOUNTY:<contributor>:<amount>:<pr>. Verifying the format would catch regressions in the recording logic.💡 Suggested enhancement
# Verify database was updated self.issue.refresh_from_db() self.assertIsNotNone(self.issue.sponsors_tx_id) + self.assertTrue( + self.issue.sponsors_tx_id.startswith("BOUNTY:testuser:5000:456"), + f"Unexpected sponsors_tx_id format: {self.issue.sponsors_tx_id}" + )
70-94: Add assertion to verifysponsors_tx_idremains unchanged.The duplicate test should verify that the existing
sponsors_tx_idis not overwritten. This ensures the idempotency guarantee is properly tested.💡 Suggested enhancement
self.assertEqual(response.status_code, 200) response_data = response.json() self.assertEqual(response_data["status"], "warning") + + # Verify the existing transaction ID was not overwritten + self.issue.refresh_from_db() + self.assertEqual(self.issue.sponsors_tx_id, "EXISTING_TX")
10-171: Consider adding test coverage for edge cases.The test suite covers the main paths well, but consider adding tests for:
- Issue without bounty tag: What happens when
has_dollar_tag=False? Per PR objectives, only issues with a bounty label should be processed.- Invalid bounty amounts: Zero or negative
bounty_amountvalues.- Missing API token header: Request without
HTTP_X_BLT_API_TOKENheader at all (vs. wrong token).💡 Example test for issue without bounty tag
`@patch.dict`(os.environ, {"BLT_API_TOKEN": "test_token_12345"}) def test_bounty_payout_issue_without_bounty_tag(self): """Test that issues without bounty tag are rejected.""" # Create issue without bounty tag non_bounty_issue = GitHubIssue.objects.create( issue_id=999, title="Non-bounty Issue", body="No bounty here", state="open", created_at="2025-01-01T00:00:00Z", updated_at="2025-01-01T00:00:00Z", url="https://github.com/TestOrg/TestRepo/issues/999", has_dollar_tag=False, repo=self.repo, ) payload = { "issue_number": 999, "repo": "TestRepo", "owner": "TestOrg", "contributor_username": "testuser", "pr_number": 456, "bounty_amount": 5000, } response = self.client.post( "/bounty_payout/", data=json.dumps(payload), content_type="application/json", HTTP_X_BLT_API_TOKEN=self.api_token, ) # Should reject or handle appropriately self.assertIn(response.status_code, [400, 404])
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (8)
website/static/images/tech/ml.svgis excluded by!**/*.svgwebsite/static/images/tech/nlp.svgis excluded by!**/*.svgwebsite/static/images/tech/oauth.svgis excluded by!**/*.svgwebsite/static/images/tech/python.svgis excluded by!**/*.svgwebsite/static/images/tech/sql.svgis excluded by!**/*.svgwebsite/static/images/tech/tailwind.svgis excluded by!**/*.svgwebsite/static/img/tomato-svgrepo-com.svgis excluded by!**/*.svgwebsite/static/js/jquery.sparkline.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (41)
.github/codeql-config.yml.github/workflows/add-files-changed-label.yml.github/workflows/auto-fix-main-precommit.yml.github/workflows/auto-fix-pr-precommit.yml.github/workflows/bounty-payout.yml.lgtm.ymlBACON/bacon-etch.yamlBACON/ord-server/example-split.yamlBACON/regtest/bitcoin.confProcfiledocs/features.mdwebsite/documents/BltAboutUs.mdwebsite/documents/BltBLTV.mdwebsite/documents/BltChangePassword.mdwebsite/documents/BltCommunityMembers.mdwebsite/documents/BltCompanyDashboard.mdwebsite/documents/BltCompanyListingPage.mdwebsite/documents/BltCompanyScoreboard.mdwebsite/documents/BltDetails.mdwebsite/documents/BltDetailsFromOwasp.mdwebsite/documents/BltGlobalLeaderboard.mdwebsite/documents/BltInvite.mdwebsite/documents/BltLoginPage.mdwebsite/documents/BltSignUpPage.mdwebsite/documents/BltStartaBughunt.mdwebsite/documents/BltStats.mdwebsite/documents/BltTerms.mdwebsite/documents/BltTrademarksSearch.mdwebsite/documents/BltTrademarksSearchResults.mdwebsite/documents/BltUserProfile.mdwebsite/documents/BltWeeklyActivityOfContributor.mdwebsite/static/css/app.min.csswebsite/static/css/checkInModal.csswebsite/static/js/debug-panel.jswebsite/static/js/gsoc_pr_report.jswebsite/static/js/issue.jswebsite/static/js/messages.jswebsite/static/js/reminder_settings.jswebsite/static/organization/js/hunt_controller.jswebsite/tests/test_bounty.pywebsite/views/bounty.py
✅ Files skipped from review due to trivial changes (24)
- website/static/js/debug-panel.js
- website/documents/BltInvite.md
- .lgtm.yml
- website/documents/BltChangePassword.md
- website/documents/BltCompanyDashboard.md
- .github/workflows/auto-fix-main-precommit.yml
- .github/codeql-config.yml
- website/documents/BltSignUpPage.md
- website/documents/BltGlobalLeaderboard.md
- website/static/css/app.min.css
- website/documents/BltStartaBughunt.md
- website/documents/BltLoginPage.md
- website/static/organization/js/hunt_controller.js
- website/documents/BltCompanyListingPage.md
- .github/workflows/auto-fix-pr-precommit.yml
- website/documents/BltCompanyScoreboard.md
- website/static/js/reminder_settings.js
- docs/features.md
- BACON/ord-server/example-split.yaml
- website/documents/BltDetailsFromOwasp.md
- website/documents/BltAboutUs.md
- website/documents/BltDetails.md
- website/documents/BltStats.md
- website/static/css/checkInModal.css
🧰 Additional context used
🪛 GitHub Actions: Check Console Statements
website/static/js/issue.js
[error] 388-388: Found console statements in 2 file(s)
🪛 LanguageTool
website/documents/BltWeeklyActivityOfContributor.md
[style] ~44-~44: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...lity**: The table is designed to handle a large number of users and activity metrics, maintaining...
(LARGE_NUMBER_OF)
🪛 markdownlint-cli2 (0.18.1)
website/documents/BltTerms.md
94-94: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run pre-commit
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (21)
BACON/regtest/bitcoin.conf (1)
12-13: Verify the exposure scope of this regtest RPC configuration.The combination of
rpcbind=0.0.0.0:18443andrpcallowip=0.0.0.0/0allows RPC connections from any IP address to this Bitcoin regtest node. While permissive settings are typical for local regtest environments, ensure this container is not exposed to untrusted networks—especially given the default credentials (bitcoin:bitcoin).If this is strictly for isolated CI/local development, this is acceptable. Otherwise, consider restricting
rpcallowipto specific trusted subnets (e.g., Docker network ranges)..github/workflows/add-files-changed-label.yml (1)
130-130: LGTM!Trailing newline normalization at EOF - no functional change.
website/documents/BltCommunityMembers.md (1)
42-42: LGTM!Markdown list formatting fix - corrects list item syntax.
Procfile (1)
3-3: LGTM!Trailing newline normalization at EOF - no functional change. The active uvicorn configuration remains intact.
website/static/js/issue.js (1)
735-735: LGTM!Trailing newline formatting change aligns with project-wide consistency standards.
website/static/js/gsoc_pr_report.js (1)
928-939: LGTM!The
viewRawDatafunction is properly structured with correct closing brace. The implementation safely handles popup blocking and properly escapes JSON content viaJSON.stringify.website/documents/BltBLTV.md (1)
43-43: LGTM!Formatting adjustment in documentation. No content changes.
website/documents/BltTrademarksSearchResults.md (1)
42-42: LGTM!Bullet point formatting standardized for consistency with other list items.
website/static/js/messages.js (1)
111-111: LGTM!Trailing newline formatting change for file consistency. The
createMessagefunction implementation is solid with proper type-based styling and auto-dismiss handling.BACON/bacon-etch.yaml (1)
11-12: LGTM!The YAML syntax fix is correct. The
- file:is proper list item syntax under theinscriptions:key.website/documents/BltUserProfile.md (1)
35-35: LGTM!EOF newline normalization.
website/documents/BltTrademarksSearch.md (1)
39-39: LGTM!Formatting adjustment with no semantic change.
website/views/bounty.py (3)
25-35: LGTM!Authentication uses constant-time comparison via
secrets.compare_digestto prevent timing attacks. The error handling properly distinguishes between missing configuration (500) and invalid tokens (403).
74-84: LGTM!Duplicate detection is appropriate. Returning 200 with a "warning" status allows idempotent behavior while signaling that no new action was taken.
86-104: LGTM!The bounty recording format is clear and parseable. The response provides useful information for confirmation. The log message correctly converts cents to dollars for readability.
.github/workflows/bounty-payout.yml (1)
15-88: LGTM!The issue detection logic correctly parses closing keywords, validates bounty labels, and handles both integer and decimal dollar amounts with proper conversion to cents.
website/tests/test_bounty.py (5)
10-39: LGTM!Test setup properly creates the necessary fixtures (Organization, Repo, GitHubIssue with bounty tag) and establishes the API token for authentication tests.
96-115: LGTM!The unauthorized test correctly verifies that an invalid API token results in a 403 response.
117-129: LGTM!The missing fields test correctly verifies that incomplete payloads are rejected with a 400 status.
131-150: LGTM!The repo not found test correctly verifies that requests for non-existent repositories return 404.
152-171: LGTM!The issue not found test correctly verifies that requests for non-existent issues return 404.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Jayant2908
left a comment
Jayant2908
left a comment
There was a problem hiding this comment.
Hey Do fix the bot issues and remove all the console log statements present. Thank You!
- Wrap https.request in Promise so errors fail workflow step - Use Buffer.byteLength for correct Content-Length with non-ASCII - Use organization__github_org for repo lookup to match GitHub data - Update tests to include github_org field
github-actions
bot
added
pre-commit: failed
github-actions
bot
added
last-active: 4d
DonnieBLT
left a comment
DonnieBLT
left a comment
There was a problem hiding this comment.
lets make this a simple github action please
|
Hi OWASP-BLT team! 👋 I can help with the bounty details recording feature! My Plan:
Timeline: 2-3 days Why I am a good fit:
Ready to help!
|
DonnieBLT
left a comment
DonnieBLT
left a comment
There was a problem hiding this comment.
I think we are getting closer to this working. I'm also interested if we can make this more standalone and not require the BLT API. Let's work on this one step at a time. I think there is now a comment that is left when there is a bounty. Let's make sure that's working.
| const result = await response.json(); | ||
| console.log('Bounty payout request successful:', result); |
There was a problem hiding this comment.
Bug: The code attempts to call .json() on an undefined variable response. The Promise resolves with a string, which should be captured and parsed instead.
Severity: CRITICAL
Suggested Fix
Assign the result of the awaited Promise to a variable like body. Then, parse the JSON from this string using JSON.parse(body). Alternatively, call resolve(JSON.parse(body)) from within the Promise's end event handler.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/workflows/bounty-payout.yml#L155-L156
Potential issue: In the bounty payout workflow, an `https` request is wrapped in a
`Promise`. This `Promise` resolves with the response body as a string. However, the code
immediately following the `await` call attempts to use `response.json()`. The variable
`response` is never defined in this scope, which will lead to a `ReferenceError`. This
error will cause the GitHub Action step to crash every time it runs, preventing bounty
payout information from being recorded in the database.
Did we get this right? 👍 / 👎 to inform future reviews.
💬 Unresolved Conversations DetectedHi @SuyashJain17! This pull request currently has 5 unresolved conversations that need to be addressed. Action Required
The label on this PR will automatically update as conversations are resolved. Thank you! 🙏 |
|
💬 Reminder: Unresolved Conversations Hi @SuyashJain17! This pull request has 1 unresolved conversation that need to be addressed. Please review and resolve the pending discussions so we can move forward with merging this PR. Thank you! 🙏 |
| BLT_API_TOKEN: ${{ secrets.BLT_API_TOKEN }} | ||
| API_BASE_URL: ${{ secrets.API_BASE_URL || 'https://owasp.org' }} | ||
| uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 https://github.com/actions/github-script/commit/f28e40c7f34bde8b3046d885e986cb6290c5673b | ||
| API_BASE_URL: ${{ secrets.API_BASE_URL || 'https://owaspblt.org' }} |
There was a problem hiding this comment.
Bug: The default API_BASE_URL in the bounty payout workflow is set to https://owaspblt.org instead of the correct API domain, https://blt.owasp.org.
Severity: HIGH
Suggested Fix
In the .github/workflows/bounty-payout.yml file, change the default value for API_BASE_URL from 'https://owaspblt.org' to the correct API endpoint, 'https://blt.owasp.org'. This will ensure that bounty payouts are correctly processed if the API_BASE_URL secret is not set.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/workflows/bounty-payout.yml#L104
Potential issue: The bounty payout workflow sets a default `API_BASE_URL` to
`'https://owaspblt.org'`. However, the application's internal API is hosted at
`blt.owasp.org`. If the `API_BASE_URL` secret is not configured in the environment, the
workflow will send requests to the wrong domain. Due to `continue-on-error: true` being
set for this step, these requests will fail silently, and bounty payouts will not be
recorded in the database. This contradicts the PR description, which states the intent
to use `blt.owasp.org`.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/bounty-payout.yml (1)
95-97:⚠️ Potential issue | 🟠 MajorGate notification comments on confirmed successful bounty recording.
The workflow posts bounty notification comments whenever
has-bountyis true (line 159), regardless of whether the "Send bounty data" POST succeeded or what status the backend returned. Withcontinue-on-error: true(line 97), step failures are silently ignored. Additionally, the backend returns HTTP 200 with"status": "warning"for duplicate bounties (website/views/bounty.py:117-124), which are treated as successful responses and trigger public comments.There's also a critical code bug: line 155 references an undefined
responsevariable (const result = await response.json();), which will throw a ReferenceError. The Promise resolves with a string body, not a response object. This error is masked bycontinue-on-error: true.Add step output validation and gate the notification step on a confirmed successful recording:
Suggested fix
- name: Send bounty data to BLT + id: send-bounty if: steps.check-issues.outputs['has-bounty'] == 'true' continue-on-error: true env: ISSUE_NUMBER: ${{ steps.check-issues.outputs.issue-number }} BOUNTY_AMOUNT: ${{ steps.check-issues.outputs.bounty-amount }} CONTRIBUTOR_USERNAME: ${{ steps.check-issues.outputs.contributor-username }} PR_NUMBER: ${{ github.event.pull_request.number }} BLT_API_TOKEN: ${{ secrets.BLT_API_TOKEN }} API_BASE_URL: ${{ secrets.API_BASE_URL || 'https://owaspblt.org' }} uses: actions/github-script@v6.1.1 with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | const core = require('@actions/core'); const https = require('https'); ... - const result = await response.json(); - console.log('Bounty payout request successful:', result); + const result = JSON.parse(body); + core.setOutput('recorded', result.status === 'success' ? 'true' : 'false'); + if (result.status !== 'success') { + core.warning(`Bounty was not newly recorded: ${result.message}`); + } + console.log('Bounty payout request completed:', result); - name: Post bounty notification comments - if: steps.check-issues.outputs['has-bounty'] == 'true' + if: steps.check-issues.outputs['has-bounty'] == 'true' && steps.send-bounty.outputs.recorded == 'true'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bounty-payout.yml around lines 95 - 97, The "Send bounty data to BLT" step currently swallows errors (continue-on-error: true), references an undefined response variable (const result = await response.json()), and doesn't surface backend "status" (success vs warning); fix by removing or disabling continue-on-error for that step, correctly capture the POST result (use the actual variable returned by the fetch/HTTP call — e.g., assign to `responseBody` or `fetchResponse` then parse JSON via JSON.parse(responseBody) or await fetchResponse.json()), validate the parsed JSON has status === "success" before setting a step output like `bounty_posted=true`, and gate the downstream "post bounty notification" step on that output (if: steps.<send-step-id>.outputs.bounty_posted == 'true') so only confirmed successful recordings trigger public comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/bounty-payout.yml:
- Around line 133-156: The code still calls await response.json() even though
the fetch-style response variable no longer exists: after the https.request
block that resolves with the response body (the local variable body inside that
promise), remove the reference to response.json() and instead use the resolved
body from the promise returned by the https.request wrapper (parse it with
JSON.parse if you expect JSON) before logging; update the logging to use the
parsed result (e.g., parsedResult) and ensure the promise resolves/rejects from
the https.request wrapper so the subsequent code uses that resolved value rather
than a non-existent response variable.
- Around line 133-153: The outbound https.request has no timeout; update the
Promise wrapper around https.request to call req.setTimeout(TIMEOUT_MS, ...)
(e.g., 30_000) and abort the request by calling req.destroy(new Error('Request
timeout')) when the timer fires so the existing req.on('error', ...) will reject
the Promise; ensure the timeout is applied to the same request object created by
https.request (referencing req) and that you do not swallow the error so callers
get a clear timeout error.
---
Outside diff comments:
In @.github/workflows/bounty-payout.yml:
- Around line 95-97: The "Send bounty data to BLT" step currently swallows
errors (continue-on-error: true), references an undefined response variable
(const result = await response.json()), and doesn't surface backend "status"
(success vs warning); fix by removing or disabling continue-on-error for that
step, correctly capture the POST result (use the actual variable returned by the
fetch/HTTP call — e.g., assign to `responseBody` or `fetchResponse` then parse
JSON via JSON.parse(responseBody) or await fetchResponse.json()), validate the
parsed JSON has status === "success" before setting a step output like
`bounty_posted=true`, and gate the downstream "post bounty notification" step on
that output (if: steps.<send-step-id>.outputs.bounty_posted == 'true') so only
confirmed successful recordings trigger public comments.
🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 46d6180f-5de2-4615-bec2-ed285b45633c
📒 Files selected for processing (1)
.github/workflows/bounty-payout.yml
| await new Promise((resolve, reject) => { | ||
| const req = https.request(options, (res) => { | ||
| let body = ''; | ||
| res.on('data', chunk => body += chunk); | ||
| res.on('end', () => { | ||
| console.log(`Status: ${res.statusCode}`); | ||
| console.log(`Response: ${body}`); | ||
| if (res.statusCode >= 400) { | ||
| reject(new Error(`Request failed: ${res.statusCode} - ${body}`)); | ||
| } else { | ||
| resolve(body); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const errorText = await response.text(); | ||
| throw new Error(`Bounty payout request failed: ${response.status} - ${errorText}`); | ||
| } | ||
| req.on('error', (e) => { | ||
| reject(new Error(`Request error: ${e.message}`)); | ||
| }); | ||
|
|
||
| req.write(data); | ||
| req.end(); | ||
| }); | ||
| const result = await response.json(); | ||
| console.log('Bounty payout request successful:', result); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
wc -l .github/workflows/bounty-payout.ymlRepository: OWASP-BLT/BLT
Length of output: 96
🏁 Script executed:
# Read the relevant section around lines 133-156
sed -n '130,160p' .github/workflows/bounty-payout.ymlRepository: OWASP-BLT/BLT
Length of output: 1296
🏁 Script executed:
# Search for any assignment to 'response' variable in the workflow file
rg 'response\s*=' .github/workflows/bounty-payout.ymlRepository: OWASP-BLT/BLT
Length of output: 39
Remove the leftover fetch response handling.
After switching to https.request, the variable response no longer exists. Line 155 attempts to call await response.json(), which throws a ReferenceError on every successful POST, causing the step to fail.
🧩 Suggested fix
- await new Promise((resolve, reject) => {
+ const responseBody = await new Promise((resolve, reject) => {
const req = https.request(options, (res) => {
let body = '';
res.on('data', chunk => body += chunk);
res.on('end', () => {
console.log(`Status: ${res.statusCode}`);
console.log(`Response: ${body}`);
if (res.statusCode >= 400) {
reject(new Error(`Request failed: ${res.statusCode} - ${body}`));
} else {
resolve(body);
}
});
});
req.on('error', (e) => {
reject(new Error(`Request error: ${e.message}`));
});
req.write(data);
req.end();
});
- const result = await response.json();
- console.log('Bounty payout request successful:', result);
+ console.log('Bounty payout request successful:', responseBody);📝 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.
| await new Promise((resolve, reject) => { | |
| const req = https.request(options, (res) => { | |
| let body = ''; | |
| res.on('data', chunk => body += chunk); | |
| res.on('end', () => { | |
| console.log(`Status: ${res.statusCode}`); | |
| console.log(`Response: ${body}`); | |
| if (res.statusCode >= 400) { | |
| reject(new Error(`Request failed: ${res.statusCode} - ${body}`)); | |
| } else { | |
| resolve(body); | |
| } | |
| }); | |
| }); | |
| if (!response.ok) { | |
| const errorText = await response.text(); | |
| throw new Error(`Bounty payout request failed: ${response.status} - ${errorText}`); | |
| } | |
| req.on('error', (e) => { | |
| reject(new Error(`Request error: ${e.message}`)); | |
| }); | |
| req.write(data); | |
| req.end(); | |
| }); | |
| const result = await response.json(); | |
| console.log('Bounty payout request successful:', result); | |
| const responseBody = await new Promise((resolve, reject) => { | |
| const req = https.request(options, (res) => { | |
| let body = ''; | |
| res.on('data', chunk => body += chunk); | |
| res.on('end', () => { | |
| console.log(`Status: ${res.statusCode}`); | |
| console.log(`Response: ${body}`); | |
| if (res.statusCode >= 400) { | |
| reject(new Error(`Request failed: ${res.statusCode} - ${body}`)); | |
| } else { | |
| resolve(body); | |
| } | |
| }); | |
| }); | |
| req.on('error', (e) => { | |
| reject(new Error(`Request error: ${e.message}`)); | |
| }); | |
| req.write(data); | |
| req.end(); | |
| }); | |
| console.log('Bounty payout request successful:', responseBody); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/bounty-payout.yml around lines 133 - 156, The code still
calls await response.json() even though the fetch-style response variable no
longer exists: after the https.request block that resolves with the response
body (the local variable body inside that promise), remove the reference to
response.json() and instead use the resolved body from the promise returned by
the https.request wrapper (parse it with JSON.parse if you expect JSON) before
logging; update the logging to use the parsed result (e.g., parsedResult) and
ensure the promise resolves/rejects from the https.request wrapper so the
subsequent code uses that resolved value rather than a non-existent response
variable.
| await new Promise((resolve, reject) => { | ||
| const req = https.request(options, (res) => { | ||
| let body = ''; | ||
| res.on('data', chunk => body += chunk); | ||
| res.on('end', () => { | ||
| console.log(`Status: ${res.statusCode}`); | ||
| console.log(`Response: ${body}`); | ||
| if (res.statusCode >= 400) { | ||
| reject(new Error(`Request failed: ${res.statusCode} - ${body}`)); | ||
| } else { | ||
| resolve(body); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const errorText = await response.text(); | ||
| throw new Error(`Bounty payout request failed: ${response.status} - ${errorText}`); | ||
| } | ||
| req.on('error', (e) => { | ||
| reject(new Error(`Request error: ${e.message}`)); | ||
| }); | ||
|
|
||
| req.write(data); | ||
| req.end(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant lines
if [ -f ".github/workflows/bounty-payout.yml" ]; then
echo "=== File found ==="
wc -l ".github/workflows/bounty-payout.yml"
echo "=== Lines 130-160 ==="
sed -n '130,160p' ".github/workflows/bounty-payout.yml" | cat -n
else
echo "File not found at .github/workflows/bounty-payout.yml"
echo "Searching for similar files..."
find . -name "*bounty*" -type f 2>/dev/null
fiRepository: OWASP-BLT/BLT
Length of output: 1594
🏁 Script executed:
# Let's check Node.js documentation and examples for https.request timeout handling
# First, let's see the Node.js version used in this workflow
grep -n "node" ".github/workflows/bounty-payout.yml" | head -20Repository: OWASP-BLT/BLT
Length of output: 39
🌐 Web query:
Node.js https.request setTimeout timeout handling best practice
💡 Result:
What to know (core behavior)
https.request()’stimeoutoption andreq.setTimeout(ms)do not stop the request—they only cause a'timeout'event to be emitted. You must abort/destroy the request yourself. (nodejs.org)- The
ClientRequest'timeout'event means socket inactivity, not “total request took too long”. (nodejs.org) req.setTimeout(ms)ultimately callssocket.setTimeout()after the socket is assigned and connected. (nodejs.org)- If you pass an
AbortSignaltohttp(s).request, callingabort()behaves likereq.destroy()(it will emit an'error'withABORT_ERR). (nodejs.org)
Best-practice pattern (manual timeout => destroy)
import https from "node:https";
export function requestWithTimeout(options, ms) {
return new Promise((resolve, reject) => {
const req = https.request(options, (res) => resolve(res));
req.on("error", reject);
req.setTimeout(ms, () => {
// IMPORTANT: timeout only notifies; you must stop the request.
req.destroy(new Error(`Request timed out after ${ms}ms`));
});
req.end();
});
}Preferred modern pattern (AbortSignal deadline)
If you want a “real” deadline (and not just an idle-socket timeout), use an abort signal and set your own timer, or (in newer Node.js) AbortSignal.timeout(ms):
import https from "node:https";
const req = https.request(
{ hostname: "example.com", path: "/", method: "GET", signal: AbortSignal.timeout(5000) },
(res) => { /* ... */ }
);
req.on("error", (err) => { /* handle ABORT_ERR vs others */ });
req.end();AbortSignal.timeout() is a convenience that creates a signal that aborts after ms. (betterstack.com)
Citations:
- 1: https://nodejs.org/api/http.html
- 2: https://nodejs.org/api/http.html
- 3: https://nodejs.org/api/http.html
- 4: https://nodejs.org/api/http.html
- 5: https://betterstack.com/community/guides/scaling-nodejs/nodejs-timeouts/?utm_source=openai
Add a timeout to the outbound HTTP request.
This https.request has no timeout, so a stalled BLT API can leave the Promise pending until the runner times out. The suggested fix correctly uses req.setTimeout() with req.destroy() to abort the request, which is the documented best-practice pattern for Node.js HTTP clients.
⏱️ Suggested fix
req.on('error', (e) => {
reject(new Error(`Request error: ${e.message}`));
});
+ req.setTimeout(10000, () => {
+ req.destroy(new Error('Request timed out after 10s'));
+ });
req.write(data);
req.end();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/bounty-payout.yml around lines 133 - 153, The outbound
https.request has no timeout; update the Promise wrapper around https.request to
call req.setTimeout(TIMEOUT_MS, ...) (e.g., 30_000) and abort the request by
calling req.destroy(new Error('Request timeout')) when the timer fires so the
existing req.on('error', ...) will reject the Promise; ensure the timeout is
applied to the same request object created by https.request (referencing req)
and that you do not swallow the error so callers get a clear timeout error.
| API_BASE_URL: ${{ secrets.API_BASE_URL || 'https://owasp.org' }} | ||
| uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 https://github.com/actions/github-script/commit/f28e40c7f34bde8b3046d885e986cb6290c5673b | ||
| API_BASE_URL: ${{ secrets.API_BASE_URL || 'https://owaspblt.org' }} | ||
| uses: actions/github-script@v6.1.1 |
There was a problem hiding this comment.
| uses: actions/github-script@v6.1.1 | |
| uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 https://github.com/actions/github-script/commit/f28e40c7f34bde8b3046d885e986cb6290c5673b |
|
lets go more simple than this - you can continue this on the rewards repo we'll handle rewards there |
5 participants
Resolves #3941
What this PR does
This adds a small flow to record bounty details when a merged pull request closes an issue that has a
$bounty label.When such a PR is merged, the bounty information is sent to the backend and stored so it can be paid out manually via GitHub Sponsors.
Why this change
There have been a few attempts to fully automate bounty payouts, but those changes are hard to review safely.
This keeps things simple by only recording the bounty, without automating any payments.
Scope
Notes
Actual payout is handled manually for now.
This keeps the flow easy to reason about and avoids touching financial automation at this stage.
Summary by CodeRabbit
Refactor
Bug Fixes
Chores
Tests