Reject consecutive hyphen GitHub logins

[jtc268]

Summary

• reject GitHub login values with consecutive hyphens across public account normalization, wallet linking/registration, and deploy admin/labeler validation
• add regressions for account API/page/MCP balance, wallet registration, and deploy readiness

Bounty #406.

Evidence

The existing GitHub login regex allowed bad--login anywhere login validation was used. GitHub username guidance for managed usernames treats names with two consecutive dashes as not creatable, so accepting them locally lets invalid github:* accounts pass public account, wallet-link, and admin config validation.

Before the fix, /api/v1/accounts/github:bad--login and MCP get_balance accepted the account shape instead of rejecting it as an invalid GitHub login.

Validation

• uv run --extra dev python -m pytest tests/test_account_validation.py::test_account_views_reject_consecutive_hyphen_github_login tests/test_wallets.py::test_wallet_registration_rejects_consecutive_hyphen_github_login tests/test_deploy_readiness.py::test_deploy_readiness_rejects_invalid_admin_or_labeler_logins -q -> 3 passed
• uv run --extra dev python -m pytest tests/test_account_validation.py tests/test_account_routes.py tests/test_wallets.py tests/test_deploy_readiness.py -q -> 67 passed
• uv run --extra dev ruff check app/accounts.py app/config.py app/ledger/service.py tests/test_account_validation.py tests/test_wallets.py tests/test_deploy_readiness.py -> passed
• uv run --extra dev ruff format --check app/accounts.py app/config.py app/ledger/service.py tests/test_account_validation.py tests/test_wallets.py tests/test_deploy_readiness.py -> 6 files already formatted
• uv run --extra dev python -m mypy app -> success
• uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok
• git diff --check -> clean

Out of scope

No auth, payout, wallet signature, ledger balance, or database migration behavior changed.

Summary by CodeRabbit

• Bug Fixes
  • Enhanced GitHub login validation to reject usernames containing consecutive hyphens, affecting account creation, wallet registration, and deployment config checks.
• Tests
  • Added and updated tests to verify rejection of logins with consecutive hyphens across API, HTML views, wallet registration, and deploy-readiness validation.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 78e74d5b-ca7a-4907-b4d0-3d87bdb3dbdb

📥 Commits

Reviewing files that changed from the base of the PR and between 0f89e7c and a7620dd.

📒 Files selected for processing (6)

• app/accounts.py
• app/config.py
• app/ledger/service.py
• tests/test_account_validation.py
• tests/test_deploy_readiness.py
• tests/test_wallets.py

---

📝 Walkthrough

Walkthrough

The PR tightens GitHub login validation by adding a negative lookahead to reject logins containing consecutive hyphens. The regex pattern is updated identically across three modules (accounts, config, ledger), and test coverage verifies the constraint is enforced across account validation, deploy readiness, and wallet registration flows.

Changes

GitHub login validation: consecutive hyphen constraint

Layer / File(s)	Summary
Validation regex constraint update app/accounts.py, app/config.py, app/ledger/service.py	GITHUB_LOGIN_RE pattern updated to add (?!.*--) negative lookahead, rejecting logins with consecutive hyphens while preserving existing character and length constraints.
Test coverage for consecutive hyphen rejection tests/test_account_validation.py, tests/test_deploy_readiness.py, tests/test_wallets.py	Three test cases verify the constraint: account/page endpoints reject with HTTP 400 and "github login" error; MCP endpoint returns HTTP 200 with error code -32602; wallet registration raises LedgerError with "invalid github login" message.

🚥 Pre-merge checks | ✅ 6

✅ Passed checks (6 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title directly and concretely names the changed surface: rejecting consecutive hyphen GitHub logins, which matches the main change across multiple regex updates.
Description check	✅ Passed	Description covers summary, evidence of the problem, validation results for all required checks, and clearly states out-of-scope items, meeting the template structure.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Mergework Public Artifact Hygiene	✅ Passed	PR modifies only technical code. No README, docs, or public artifacts changed. No prohibited investment, price, cash-out, or payout claims detected.
Bounty Pr Focus	✅ Passed	Diff matches stated files: three modules updated with (?!.*--) regex, three test files added covering account/wallet/deploy validation. Scope focused on Bounty #406 with no unrelated changes.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[er1c-cartman]

Reviewed PR #480 at current head 0f89e7c8dc43bd5520980a5a25793135afb8df21. No blocker found in this focused pass.

Evidence checked:

• Diff is scoped to six files: app/accounts.py, app/config.py, app/ledger/service.py, tests/test_account_validation.py, tests/test_deploy_readiness.py, and tests/test_wallets.py.
• The new ^(?!.*--)... login regex consistently applies to public account normalization, deploy admin/labeler validation, and wallet registration, so github:bad--login is rejected across the surfaces that previously accepted it.
• Existing valid-login behavior is preserved by the original character/length/start/end constraints; the change only adds the consecutive-hyphen exclusion.
• Regression coverage exercises API account lookup, public account page lookup, MCP get_balance, wallet registration, and deploy readiness validation.
• GitHub CI run #635 completed successfully on this head.

Validation run locally:

• PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ~/.local/bin/uv run --extra dev python -m pytest tests/test_account_validation.py::test_account_views_reject_consecutive_hyphen_github_login tests/test_wallets.py::test_wallet_registration_rejects_consecutive_hyphen_github_login tests/test_deploy_readiness.py::test_deploy_readiness_rejects_invalid_admin_or_labeler_logins -q -> 3 passed
• PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ~/.local/bin/uv run --extra dev python -m pytest tests/test_account_validation.py tests/test_account_routes.py tests/test_wallets.py tests/test_deploy_readiness.py -q -> 67 passed
• ~/.local/bin/uv run --extra dev ruff check app/accounts.py app/config.py app/ledger/service.py tests/test_account_validation.py tests/test_wallets.py tests/test_deploy_readiness.py -> passed
• ~/.local/bin/uv run --extra dev ruff format --check app/accounts.py app/config.py app/ledger/service.py tests/test_account_validation.py tests/test_wallets.py tests/test_deploy_readiness.py -> 6 files already formatted
• ~/.local/bin/uv run --extra dev mypy app -> success
• PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ~/.local/bin/uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok
• git diff --check origin/main...HEAD -> clean
• changed-diff secret scan -> no matches

Assessment: this is a good focused hardening move for MergeWork because it keeps account identities closer to GitHub-createable login rules before those values enter public pages, MCP responses, deploy settings, or wallet-link records.

[akmhatey-ai]

Reviewed PR #480 at current head 0f89e7c8dc43bd5520980a5a25793135afb8df21.

I would make one small test-coverage change before merge. The runtime behavior looks fixed, but the deploy-readiness regression does not isolate the new consecutive-hyphen case.

tests/test_deploy_readiness.py::test_deploy_readiness_rejects_invalid_admin_or_labeler_logins now checks both bad_login and bad--login in the same admin_logins / github_accepted_labelers tuples. Since bad_login was already invalid before this PR, that test would still pass if app/config.py regressed to the old regex that accepted consecutive hyphens.

Local proof on this head, without changing files, by monkeypatching app.config.GITHUB_LOGIN_RE to the old regex:

bad--login only -> []
current test shape -> ['MERGEWORK_ADMIN_LOGINS must contain valid GitHub logins', 'MERGEWORK_GITHUB_ACCEPTED_LABELERS must contain valid GitHub logins']

So the account/API/MCP and wallet coverage prove the new rule, but the deploy-readiness surface is only incidentally covered by an older invalid underscore login. Please split or parameterize the deploy-readiness test so bad--login is tested alone for admin_logins and github_accepted_labelers.

Validation I ran:

• uv run --extra dev python -m pytest tests/test_account_validation.py::test_account_views_reject_consecutive_hyphen_github_login tests/test_wallets.py::test_wallet_registration_rejects_consecutive_hyphen_github_login tests/test_deploy_readiness.py::test_deploy_readiness_rejects_invalid_admin_or_labeler_logins -q -> 3 passed
• uv run --extra dev python -m pytest tests/test_account_validation.py tests/test_account_routes.py tests/test_wallets.py tests/test_deploy_readiness.py -q -> 67 passed
• uv run --extra dev python -m pytest -q -> 416 passed
• uv run --extra dev ruff check app/accounts.py app/config.py app/ledger/service.py tests/test_account_validation.py tests/test_wallets.py tests/test_deploy_readiness.py -> passed
• uv run --extra dev ruff format --check app/accounts.py app/config.py app/ledger/service.py tests/test_account_validation.py tests/test_wallets.py tests/test_deploy_readiness.py -> 6 files already formatted
• uv run --extra dev python -m mypy app -> success
• uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok
• git diff --check origin/main...HEAD -> clean
• git diff origin/main...HEAD | gitleaks stdin --no-banner --redact --exit-code 1 -> no leaks found

This is a coverage gap, not a claim that the current production behavior is still accepting bad--login.

[jtc268]

Updated in a7620dd to address the deploy-readiness coverage gap.

What changed:

• Added test_deploy_readiness_rejects_consecutive_hyphen_admin_or_labeler_logins() where bad--login is the only admin/labeler value, so the deploy-readiness test now fails under the old regex and proves the new consecutive-hyphen constraint directly.

Fresh validation:

• PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_deploy_readiness.py::test_deploy_readiness_rejects_invalid_admin_or_labeler_logins tests/test_deploy_readiness.py::test_deploy_readiness_rejects_consecutive_hyphen_admin_or_labeler_logins -q -> 2 passed
• PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_deploy_readiness.py tests/test_account_validation.py tests/test_wallets.py -q -> 65 passed
• uv run --extra dev ruff check tests/test_deploy_readiness.py -> passed
• uv run --extra dev ruff format --check tests/test_deploy_readiness.py -> 1 file already formatted
• git diff --check -> clean

[akmhatey-ai]

Follow-up on my earlier changes-requested review for head 0f89e7c8dc43bd5520980a5a25793135afb8df21: current head a7620dde2b9fa73aa889b6990197bf94113da26d resolves the deploy-readiness coverage gap I flagged.

The new test_deploy_readiness_rejects_consecutive_hyphen_admin_or_labeler_logins() checks bad--login as the only admin/labeler value, so the deploy-readiness path now directly proves the consecutive-hyphen rule instead of relying on the already-invalid bad_login case.

I revalidated the current head:

• focused consecutive-hyphen/deploy/account/wallet tests: 4 passed
• related deploy/account/wallet suites: 65 passed
• full pytest: 417 passed
• Ruff check and format check: passed
• mypy app: success
• docs smoke: ok
• git diff --check origin/main...HEAD: clean
• diff-scoped Gitleaks: no leaks

Approved for the current head. This is a follow-up to the existing review thread, not a new bounty claim.

[rebel117]

Reviewed PR #480 at current head for Bounty #447.

Checked files: app/accounts.py, app/config.py, app/ledger/service.py, tests/test_account_validation.py, tests/test_deploy_readiness.py, tests/test_wallets.py.

Evidence:

• Inspected the regex change in all 3 locations: r"^(?!.*--)[a-z0-9](?:[a-z0-9-]{0,37}[a-z0-9])?$". The negative lookahead (?!.*--) rejects any login containing consecutive hyphens.
• Confirmed GitHub actually forbids consecutive hyphens in usernames (per their signup rules), so this aligns the validation with the real constraint.
• Verified the test covers all three entry points: API (/api/v1/accounts/github:bad--login), HTML page (/accounts/github:bad--login), and MCP (get_balance with github:bad--login). All three return error responses.
• Checked test_deploy_readiness.py changes: the existing test_deploy_readiness_rejects_invalid_admin_or_labeler_logins now includes bad--login alongside bad_login, and a new dedicated test covers consecutive-hyphen-only case.
• Confirmed test_wallets.py verifies wallet registration also rejects bad--login through register_wallet.
• Cross-referenced: the same GITHUB_LOGIN_RE pattern is defined in 3 separate modules (accounts.py, config.py, ledger/service.py). This PR updates all 3 consistently.

Verdict: Correct and well-tested. One minor observation: having the same regex in 3 files is a DRY concern, but that is pre-existing and out of scope for this PR. No blockers.