fix(deploy): guard GitHub OAuth callback route registration (#1146)#1185
fix(deploy): guard GitHub OAuth callback route registration (#1146)#1185yanyishuai wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 |
JeremyZeng77
left a comment
There was a problem hiding this comment.
The current head is not merge-ready because the new public link checker imports a helper module that is not present in this PR.
Evidence checked:
- Reviewed the changed files:
scripts/check_public_mrwk_links.py,tests/test_check_public_mrwk_links.py,tests/test_oauth_deploy_smoke.py,app/oauth_deploy_smoke.py, deploy-ready wiring, runbook notes, and the public link fixture. - CI run
28415796267fails during pytest collection before the suite can run. - Both
tests/test_check_public_mrwk_links.pyandtests/test_oauth_deploy_smoke.pyimportscripts.check_public_mrwk_links; that module importsGH_TIMEOUT_SECONDSfromscripts.gh_cli_constants. scripts/gh_cli_constants.pyis not included in this PR's changed-file list, so collection stops withModuleNotFoundError: No module named 'scripts.gh_cli_constants'.
Suggested fix: include the shared scripts/gh_cli_constants.py helper in this branch, or keep this PR self-contained by defining a local timeout constant for the public link checker.
b1cf950 to
6cdd8c9
Compare
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed updated head 6cdd8c9f4b86267882fd305d83df16b4fa9f7779.
This still needs changes before merge. Hosted pytest fails during collection because scripts/check_public_mrwk_links.py imports GH_TIMEOUT_SECONDS from scripts.gh_cli_constants, but this PR still does not add that module. The import blocks both tests/test_check_public_mrwk_links.py and tests/test_oauth_deploy_smoke.py before the new guardrails can run.
There is also a functional gap in the deploy link check: --input still appends load_input_rows(args.input) directly to rows, so the runbook fixture analyzes cached status_code/body values instead of live-probing the published bounty/proposal/proof/OAuth URLs. The post-deploy command should normalize input rows and call probe_url() for each URL, with regression coverage for that path.
665edb8 to
670d115
Compare
670d115 to
9356b27
Compare
|
OAuth deploy smoke guard for #1146 is green on Includes Wallet: |
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed updated head 9356b27fe0dc39ab015d15014238d1e7ad6f34e3.
The import blocker is fixed and hosted checks are green, but the deploy health check still needs one functional change before merge: the runbook --input fixtures/public_mrwk_links.json --fail-on-issues path still does not live-probe those URLs. main() extends rows directly with load_input_rows(args.input), so the deploy gate analyzes cached status_code/body values from the fixture instead of checking the current public OAuth/bounty/proposal/proof endpoints.
Please normalize input rows to URL/type/source and call probe_url() for each --input URL before analyze_probe_results(). The fixture should describe targets, not pre-bake healthy results. Add focused regression coverage proving --input invokes probing and that --fail-on-issues exits nonzero on unhealthy live-probe results.
Summary
Production
/auth/github/callbackwas returning an ExpressCannot GETshell,which means the public host was not serving the MergeWork FastAPI app. This PR
adds deploy-time and link-health guardrails so that regression is caught before
contributor sign-in breaks again. Closes #1146.
Changes
app/oauth_deploy_smoke.py— verify login/callback routes are registered(503/422 from FastAPI, not 404 / Express shell)
scripts/check_deploy_ready.py— run OAuth route registration gate on deployscripts/check_public_mrwk_links.py— OAuth-specific health rules (422/503 OK)fixtures/public_mrwk_links.json— representative public URLs incl. OAuthWhy
Issue #1146 blocked
/meGitHub sign-in when production served the wrong appfor the OAuth callback path. The routes already exist in
app/auth.py; thischange makes a bad deploy fail fast instead of silently breaking payouts.
Test plan
Wallet
Do4v7foHJvRJLpRRoGaVPWX6DDEjX3yTK7J91gpwUQpECloses #1146