Skip to content

feat(github): add required_signatures branch-protection tools (#178)#180

Merged
MementoRC merged 1 commit into
developmentfrom
feat/issue-178-required-signatures
May 22, 2026
Merged

feat(github): add required_signatures branch-protection tools (#178)#180
MementoRC merged 1 commit into
developmentfrom
feat/issue-178-required-signatures

Conversation

@MementoRC
Copy link
Copy Markdown
Owner

Summary

GitHub exposes commit-signature enforcement on a separate REST endpoint (.../protection/required_signatures), so it cannot be set through github_update_branch_protection's payload. Closes that gap by adding a dedicated enable/disable/get trio, mirroring the existing github_*_vulnerability_alerts pattern.

New tools:

  • github_get_required_signatures(repo_owner, repo_name, branch)
  • github_enable_required_signatures(repo_owner, repo_name, branch)
  • github_disable_required_signatures(repo_owner, repo_name, branch)

Endpoint: PUT|DELETE|GET /repos/{owner}/{repo}/branches/{branch}/protection/required_signatures

Changes

  • github/security.py — three async functions using the existing github_client_context(). The PUT returns 200+JSON (unlike the vulnerability_alerts PUT which returns 204), DELETE returns 204, GET returns 200 with {enabled, url} or 404 when branch protection is absent.
  • github/models.py — three Pydantic models, each (repo_owner, repo_name, branch).
  • lean/registry_github.pyToolDefinition trio added after the vulnerability_alerts block, using model_json_schema().
  • github/api.py — untouched; the existing from .security import * wildcard re-export picks up the new functions automatically.
  • README.md — lean interface counts: 52 → 55 (total), 51 → 54 / 22 → 25 (GitHub bucket).
  • tests/unit/github/test_github_required_signatures.py — 7 tests via AsyncMock + patch(github_client_context).

Test plan

  • pixi run -e quality ruff check <5 changed files> — clean
  • pixi run -e quality ruff format --check <5 changed files> — clean (pre-existing format debt on unrelated files is not introduced here)
  • pixi run -e quality pytest tests/unit/github/test_github_required_signatures.py -v — 7/7 pass
  • pixi run -e quality pytest tests/unit/github/ -v — 266/266 pass, no regressions

Test coverage

Test Verifies
test_get_returns_enabled_when_status_200_and_enabled_true GET 200 + enabled:true body → ✅ Required signatures: enabled
test_get_returns_disabled_when_status_200_and_enabled_false GET 200 + enabled:false body → ❌ Required signatures: disabled
test_get_returns_not_found_when_status_404 GET 404 → ❌ Branch protection or branch not found:
test_enable_returns_success_when_status_200 PUT 200 → success line
test_enable_returns_error_when_status_404 PUT 404 → error line
test_disable_returns_success_when_status_204 DELETE 204 → success line
test_disable_returns_error_when_status_404 DELETE 404 → error line

All tests verify the correct URL /repos/foo/bar/branches/main/protection/required_signatures is hit on the right HTTP verb.

Why dedicated tools, not a parameter on update_branch_protection

The issue notes the dedicated-tools approach is “probably the cleaner fit” because:

  • The required_signatures endpoint is a separate REST resource with its own PUT/DELETE/GET verbs.
  • It matches the existing pattern for github_enable_vulnerability_alerts / github_enable_automated_security_fixes.
  • It keeps github_update_branch_protection from silently making a second API call hidden inside its payload.

Closes #178

GitHub exposes commit-signature enforcement on a separate endpoint
(.../protection/required_signatures), so it could not be set through
github_update_branch_protection. Closes that gap by adding a dedicated
enable/disable/get trio, mirroring the existing
github_*_vulnerability_alerts pattern.

New tools:
  - github_get_required_signatures(repo_owner, repo_name, branch)
  - github_enable_required_signatures(repo_owner, repo_name, branch)
  - github_disable_required_signatures(repo_owner, repo_name, branch)

Endpoint: PUT/DELETE/GET /repos/{owner}/{repo}/branches/{branch}/protection/required_signatures

- security.py: three async functions using github_client_context().
  PUT returns 200+JSON (not 204 like vulnerability_alerts PUT);
  DELETE returns 204; GET returns 200 with {enabled, url} or 404
  when branch protection is absent.
- models.py: three Pydantic models with (repo_owner, repo_name, branch).
- registry_github.py: ToolDefinition trio added after vulnerability_alerts.
- api.py untouched (wildcard re-export of security.py covers new funcs).
- README.md tool counts: 52 -> 55 and 51/22 -> 54/25.
- tests/unit/github/test_github_required_signatures.py: 7 tests via
  AsyncMock+patch(github_client_context) covering get (enabled/disabled/
  404), enable (200/404), disable (204/404).

Closes #178
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 22, 2026

Claude finished @MementoRC's task —— View job


PR Review: GitHub Required Signatures Branch Protection Tools

I've completed my comprehensive review of this PR. Here's my assessment:

Todo List:

  • Read and analyze all changed files
  • Review code quality and best practices
  • Check for potential bugs and issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage and quality
  • Provide comprehensive feedback

Overall Assessment: ✅ EXCELLENT IMPLEMENTATION

This is a very well-implemented PR that follows established patterns and maintains high code quality. The implementation is clean, consistent, and comprehensive.

Code Quality & Best Practices ✅

Excellent adherence to existing patterns:

  • Consistent API structure: All 3 functions follow the exact same pattern as existing github_*_vulnerability_alerts functions in the same file
  • Proper error handling: Comprehensive exception handling covering ValueError, ConnectionError, and general exceptions
  • Logging consistency: Uses the same logging pattern (logger.debug, logger.info, logger.error) as existing functions
  • Return format: Follows established emoji + message format (/ prefix with descriptive messages)

Strong model design:

  • Clean Pydantic models at lines 673-695 in models.py follow the exact same pattern as existing models
  • Proper field naming: Uses consistent repo_owner, repo_name, branch parameters
  • No unnecessary complexity: Models are appropriately simple with just the required fields

Security Considerations ✅

Appropriate security implementation:

  • Correct GitHub API endpoint: Uses the proper /repos/{owner}/{repo}/branches/{branch}/protection/required_signatures endpoint
  • Expected HTTP status codes:
    • GET: 200 (with JSON body), 404 (not found)
    • PUT: 200 (with JSON response, unlike vulnerability_alerts which returns 204)
    • DELETE: 204 (success), 404 (not found)
  • Branch validation: Models inherit from the same validation as other branch protection tools
  • No security vulnerabilities: No exposed secrets, proper authentication handling

Performance Considerations ✅

Efficient implementation:

  • Minimal API calls: Each function makes exactly one HTTP request
  • Async/await pattern: Follows established async patterns for non-blocking operations
  • Context management: Proper use of github_client_context() for connection management
  • No unnecessary processing: Direct request/response handling without extra computation

Test Coverage & Quality ✅

Comprehensive test suite (tests/unit/github/test_github_required_signatures.py):

  • 100% function coverage: All 3 functions have dedicated test cases
  • Status code coverage: Tests both success and error scenarios for each function
  • Proper assertions: Tests verify correct URLs, response handling, and return messages
  • Good test organization: Clear test names and proper grouping by function
  • Mocking strategy: Appropriate use of AsyncMock and patch following established patterns

Test Cases Coverage:

  • github_get_required_signatures: Tests 200+enabled, 200+disabled, and 404 scenarios
  • github_enable_required_signatures: Tests 200 success and 404 error
  • github_disable_required_signatures: Tests 204 success and 404 error

Architecture & Integration ✅

Seamless integration with existing codebase:

  • Registry integration: Properly registered in lean/registry_github.py at lines 457-479
  • Import structure: Functions automatically exported via wildcard import in api.py:16
  • Documentation count update: README correctly updated from 52→55 total tools
  • Logical grouping: Placed appropriately in the Security & Compliance section

Minor Observations

Slight inconsistency noted (not a bug):

  • The PUT endpoint returns 200+JSON unlike vulnerability_alerts which returns 204, but this is correct per GitHub's API documentation for required_signatures vs vulnerability_alerts endpoints

No issues found:

  • No potential bugs identified
  • No security vulnerabilities
  • No performance concerns
  • No missing edge cases
  • No architectural problems

Recommendation: ✅ APPROVE

This PR is ready for merge. It demonstrates excellent engineering practices:

  1. Follows established patterns perfectly
  2. Comprehensive test coverage
  3. Proper error handling
  4. Clean, maintainable code
  5. Accurate documentation updates
  6. No security or performance issues

The implementation correctly fills the gap mentioned in the issue description by providing dedicated tools for GitHub's separate required_signatures endpoint, maintaining consistency with the existing vulnerability_alerts pattern.


@MementoRC MementoRC merged commit be85d37 into development May 22, 2026
16 checks passed
@MementoRC MementoRC deleted the feat/issue-178-required-signatures branch May 22, 2026 17:03
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.

github_update_branch_protection is missing required_signatures parameter

1 participant