feat(github): add GHAS forensics tools — rulesets, code & secret scanning (#186)

[MementoRC]

Closes #186.

Summary

Adds 7 read-only GitHub tools so a mergeable: true, merge_state: blocked PR — all checks green, required_approving_review_count: 0 — can be root-caused from MCP git alone. The blocker for that class (e.g. Claire-s-Monster/ci-framework#207, a Dependabot bump where CodeQL passes in 2s but the PR sits blocked) is invariably a repository ruleset or a code-scanning requirement, neither of which classic github_get_branch_protection exposes.

New tools

Rulesets

• github_list_rulesets(repo_owner, repo_name) — GET /repos/{o}/{r}/rulesets
• github_get_ruleset(repo_owner, repo_name, ruleset_id) — GET /repos/{o}/{r}/rulesets/{id} (full config: required_status_checks, code_scanning, bypass actors)
• github_get_branch_rules(repo_owner, repo_name, branch) — GET /repos/{o}/{r}/rules/branches/{branch} (which classic+ruleset rules apply to a branch)

Code scanning

• github_list_code_scanning_alerts(..., state, severity, tool_name, ref) — GET /repos/{o}/{r}/code-scanning/alerts
• github_list_code_scanning_analyses(..., ref, tool_name) — GET /repos/{o}/{r}/code-scanning/analyses (ref supports refs/pull/{n}/head)
• github_get_code_scanning_default_setup(repo_owner, repo_name) — GET /repos/{o}/{r}/code-scanning/default-setup

Secret scanning

• github_list_secret_scanning_alerts(repo_owner, repo_name, state) — GET /repos/{o}/{r}/secret-scanning/alerts

Implementation

• New module github/scanning.py (kept out of security.py, which was at 516 lines — adding here would have breached the 500-line organism limit). Wired via api.py's wildcard re-export.
• Functions use the existing github_client_context() auth pattern; 404 returns a structured ❌ … not found string mirroring github_get_required_signatures. Optional filters are sent as query params, omitted when None.
• 7 Pydantic models in github/models.py; 7 ToolDefinitions in lean/registry_github.py.
• tests/unit/github/test_github_ghas_forensics.py: 18 tests (success + 404 per tool, plus query-param forwarding / None-omission for the filtered list tools).
• README counts: 56→63 upfront, 55→62 accessible, GitHub 25→32.

Diagnosis path this unlocks (the #207 case)

github_get_branch_rules(development) → ruleset id → github_get_ruleset(id) → check code_scanning requirement → github_list_code_scanning_analyses(ref=refs/pull/207/head) → confirm empty/insufficient results.

Test plan

• pixi run -e quality test-unit — passes (exit 0)
• pixi run -e quality lint (ruff F,E9) — clean

⚠️ Read-only tools; require security_events: read scope on the token (the existing PAT that powers github_get_security_analysis already has it). Requires an MCP server restart to expose the new tools.

🤖 Generated with Claude Code

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR Review: feat(github): add GHAS forensics tools

• Gather context and read changed files
• Review code quality and best practices
• Check for bugs or issues
• Review test coverage
• Security considerations
• Post final review

---

Overall this is a clean, well-structured addition. The implementation follows the existing patterns in the codebase consistently. Below are observations ranging from functional issues to style improvements.

Bugs / Functional Issues

1. _build_params silently converts False and 0 to strings (scanning.py:18-20)

def _build_params(**kwargs: Any) -> dict[str, str]:
    return {k: str(v) for k, v in kwargs.items() if v is not None}

The condition if v is not None will include False and 0 as params (correct), but it will also inadvertently include empty strings "". If a caller ever passes state="" they'd get state= in the query string. Consider if v is not None and v != "" or document the contract. Not a current bug since all callers use None as the sentinel, but it's a subtle footgun.

2. Missing __all__ in scanning.py

Every other module (security.py, repos.py, etc.) uses wildcard re-exports via api.py. Without __all__, from .scanning import * will export everything including logger, _build_params, and any future private helpers. Adding __all__ would explicitly control the public surface:

__all__ = [
    "github_list_rulesets",
    "github_get_ruleset",
    "github_get_branch_rules",
    "github_list_code_scanning_alerts",
    "github_list_code_scanning_analyses",
    "github_get_code_scanning_default_setup",
    "github_list_secret_scanning_alerts",
]

3. No pagination support (scanning.py)

github_list_rulesets, github_list_code_scanning_alerts, github_list_code_scanning_analyses, and github_list_secret_scanning_alerts all call list endpoints that GitHub paginates (default 30 items, max 100). A repo with many alerts or rulesets will silently return only the first page. Other list tools in the codebase (e.g., GitHubListPullRequests) expose per_page/page parameters. This is worth adding, at minimum for the alerts endpoints.

Code Quality / Style

4. Inconsistent logging style with the rest of the codebase (scanning.py)

security.py uses f-strings in logger.debug/error calls:

logger.debug(f"🔍 Getting vulnerability alerts status for {repo_owner}/{repo_name}")

scanning.py uses %-style formatting (which is technically preferred by the Python logging docs and avoids unnecessary string formatting when the log level is inactive). This is a minor inconsistency — worth picking one style and sticking to it. The new code's %-style is actually the better practice; consider noting it as a deliberate improvement.

5. Pydantic models lack input validation for enum-like string fields (models.py:1026-1029)

state, severity, tool_name in GitHubListCodeScanningAlerts are free-form strings. The GitHub API will return a 422 for invalid values. Other models in the codebase use field_validator for constrained strings (e.g. branch name validation at models.py:37-57). Adding a Literal type or validator for state and severity would give earlier, clearer error messages:

from typing import Literal
state: Literal["open", "closed", "dismissed", "fixed"] | None = None
severity: Literal["critical", "high", "medium", "low", "warning", "note", "error"] | None = None

6. Redundant params if params else None pattern (scanning.py:167, 224, 330)

_build_params already returns an empty dict {} when all args are None. The callers then pass params=None when the dict is empty. An empty dict {} as params would be equivalent (aiohttp ignores empty params). But the tests specifically assert params=None for the no-filter case (test line 196, 285, 409), which means the current logic is load-bearing for the tests. The code and tests are consistent — just note this as a deliberate design choice to keep None semantics clear. ✓

Test Coverage

7. No error-path tests for non-200/non-404 responses (test file)

All 18 tests cover 200 (success) and 404 (not found). There are no tests for, e.g., 403 (insufficient scope), 422 (invalid params), 500 (server error), or the ValueError/ConnectionError exception paths. Looking at the pattern from similar test files in the repo, the exception paths are likely worth covering. The 403 case is particularly relevant since these endpoints require security_events: read.

8. test_list_alerts_passes_filters_as_query_params accesses kwargs by positional index (test line 216)

call_kwargs = mock_client.get.call_args
params = call_kwargs[1]["params"]

call_args[1] accesses keyword arguments by index — this works but is fragile. The idiomatic way is mock_client.get.call_args.kwargs["params"] which is more readable and won't break if positional/keyword argument ordering changes.

Summary

Category	Finding	Severity
Bug	_build_params includes empty strings	Low
Missing	__all__ not defined	Medium
Missing	No pagination support	Medium
Style	Logging style inconsistency	Low
Quality	No enum validation on state/severity	Low
Tests	No error-path / exception tests	Medium
Tests	Fragile call_args[1] indexing	Low

The core logic is correct, the 404 handling mirrors existing patterns faithfully, and the diagnosis workflow described in the PR description is well thought out. Addressing pagination and __all__ would be the highest-value pre-merge changes.

[MementoRC]

Thanks for the thorough review — addressed in c03c369:

Functional

• 🚨 CRITICAL HOTFIX: Prevent MCP server crashes during GPG operations #3 pagination (most substantive): added per_page (ge=1, le=100) and page (ge=1) to the four list tools — github_list_rulesets, github_list_code_scanning_alerts, github_list_code_scanning_analyses, github_list_secret_scanning_alerts — forwarded as query params, omitted when None. The three single-object GETs are untouched. +tests asserting forwarding and omission.
• feat: add dotenv #1 _build_params empty-string footgun: now drops "" too (v is not None and v != ""); False/0 are still included (they stringify to "False"/"0").
• feat: enhance git_add with robust error handling and detailed feedback #2 missing __all__: added, naming only the 7 public tool functions so from .scanning import * no longer leaks logger/_build_params.

Quality

• feat: LLM-Optimized Code Compliance Transformation #5 enum validation: state/severity on the code-scanning-alerts model and state on secret-scanning are now Literal[...] for schema-documented validation. Kept the scanning.py function signatures as str | None since the lean dispatch passes raw kwargs and bypasses Pydantic — the Literal is schema-level. tool_name stays free-form.

Tests

• feat: add E2E verification tests for MCP git server functionality #7 error paths: added 403 (insufficient security_events scope) tests and exception-path (ConnectionError / generic) tests.
• fix: resolve MCP server crash from logging stdout conflicts #8 fragile access: replaced call_args[1]["params"] with call_args.kwargs["params"] at all sites.

Intentionally not changed (you flagged these as fine):

• feat: add comprehensive technical documentation #4 %-style logging — left as-is; it's the better practice (lazy formatting), as you noted.
• fix: resolve git server import errors preventing ClaudeCode loading #6 params if params else None — deliberate, tests depend on the None semantics.

scanning.py is 389 lines (within the 500 limit). test-unit green (exit 0), lint clean.

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR Review: feat(github): add GHAS forensics tools (follow-up on c03c369)

• Gather context and read changed files
• Review fixes from previous round
• Check for new issues
• Post final review

---

All 7 items from the previous review have been addressed. Here's the verification:

Fixes Verified ✓

Previous Finding	Status
#1 _build_params empty-string footgun	Fixed — v is not None and v != "" (scanning.py:30)
#2 Missing __all__	Fixed — 7 public names listed (scanning.py:12-20)
#3 No pagination	Fixed — per_page/page added to all 4 list tools with ge=1, le=100 / ge=1 constraints in models
#5 No Literal enum validation	Fixed — Literal["open", "closed", "dismissed", "fixed"] on GitHubListCodeScanningAlerts.state, Literal["open", "resolved"] on GitHubListSecretScanningAlerts.state, Literal[...] on severity (models.py:1030-1031, 1061)
#7 No 403/exception path tests	Fixed — 403 tests for github_list_rulesets, github_list_code_scanning_alerts, github_list_secret_scanning_alerts; ConnectionError/RuntimeError tests added
#8 Fragile call_args[1] indexing	Fixed — all sites use .call_args.kwargs["params"]
#4 / #6 (intentionally deferred)	Acknowledged — %-style logging and params if params else None left as-is, both defensible

Remaining Observations (minor, not blocking)

1. Inconsistent 403 test coverage

github_get_ruleset and github_get_branch_rules have 403 error handling in the implementation (the else branch at lines 97-99 and 134-136) but no corresponding 403 test. github_list_code_scanning_analyses and github_get_code_scanning_default_setup are also missing 403 tests. The PR adds 403 tests only for the 3 list tools where the scope issue is most salient — this is a reasonable triage, but worth noting for completeness.

2. _build_params stringifies False and 0 → "False" / "0"

The docstring says False/0 are intentionally included. This is noted in the PR response. However, the GitHub API doesn't currently use boolean query params in these endpoints, so this is purely a forward-compatibility note. No action needed, just worth keeping in mind.

3. tool_name parameter name may conflict with GitHub API

The GitHub code scanning API uses tool_name as the query parameter name (confirmed in the docs). _build_params passes kwargs by name, so tool_name= in the dict becomes ?tool_name= in the query string — this is correct. ✓

4. No test for github_get_code_scanning_default_setup exception paths

The 200 and 404 cases are covered but not the exception path. Minor given the straightforward implementation.

Summary

The implementation is clean, correct, and follows all existing patterns. All substantive issues from the first review have been properly resolved. The remaining observations are minor and non-blocking. This PR is ready to merge.

[MementoRC]

Closed out the remaining minor observations in b349cc2 (test-only):

• feat: add dotenv #1 uniform 403 coverage: added 403 tests for github_get_ruleset, github_get_branch_rules, github_list_code_scanning_analyses, and github_get_code_scanning_default_setup — every GHAS tool now has a 403 path test.
• feat: add comprehensive technical documentation #4 exception path: added a ConnectionError exception-path test for github_get_code_scanning_default_setup.
• feat: enhance git_add with robust error handling and detailed feedback #2 / 🚨 CRITICAL HOTFIX: Prevent MCP server crashes during GPG operations #3: no action — the False/0 stringify is a forward-compat note, and tool_name → ?tool_name= is already correct. ✓

test-unit green (exit 0), lint clean.

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR Review: feat(github): add GHAS forensics tools (round 3 — b349cc2)

• Gather context and read changed files
• Review current state of all files
• Check for remaining issues
• Post final review

---

All three rounds of review are now complete. The implementation is in excellent shape. Here's a final verification pass against the full set of files.

All Previous Issues Resolved ✓

Finding	Resolution
_build_params empty-string footgun	Fixed — v is not None and v != "" (scanning.py:30)
Missing __all__	Fixed — 7 public names listed (scanning.py:12-20)
No pagination	Fixed — per_page/page on all 4 list tools with ge=1, le=100 / ge=1 constraints
No Literal enum validation	Fixed — Literal[...] on state/severity in GitHubListCodeScanningAlerts and state in GitHubListSecretScanningAlerts (models.py:1030-1031, 1061)
Missing 403 tests	Fixed — all 7 tools now have 403 coverage
Missing exception path tests	Fixed — ConnectionError on github_list_rulesets and github_get_code_scanning_default_setup, RuntimeError on github_list_code_scanning_alerts
Fragile call_args[1] indexing	Fixed — all sites use .call_args.kwargs["params"]

Final Code Quality Assessment

scanning.py — Clean and consistent. The three-tier exception handling (ValueError → auth, ConnectionError → network, Exception → catch-all) is applied uniformly across all 7 functions. __all__ properly limits the public surface to the 7 tool functions. _build_params is now defensively written.

models.py — The 7 new Pydantic models are well-scoped. Literal types on enum fields give early validation with schema-level documentation. per_page/page constraints (ge=1, le=100 / ge=1) prevent obvious API misuse. The free-form tool_name: str | None is appropriate — GitHub doesn't constrain that field.

registry_github.py — New ToolDefinition entries follow the exact pattern of existing entries (domain="github", complexity="focused"). Imports are alphabetically integrated into the existing block.

Test file — 30+ tests across 7 tools covering: 200 success, 404 not-found, 403 forbidden, exception paths, filter param forwarding, None param omission, and pagination forwarding. Test isolation via _mock_client helper and _PATCH_CTX constant is clean. All call_args access uses .kwargs["params"].

One Observation (non-blocking)

test_list_secret_alerts_omits_none_state (test line 623) only passes state=None but not per_page=None, page=None. Since _build_params receives all three as None and the assertion is params is None, the test implicitly validates all three. This is correct but slightly non-obvious — the test name (_omits_none_state) understates what it's actually verifying. A minor naming note, not worth a change.

Summary

The implementation is complete, correct, and thoroughly tested. The diagnosis workflow described in the PR (github_get_branch_rules → github_get_ruleset → github_list_code_scanning_analyses) is well-supported by the new tools. This PR is ready to merge.