docs/WEEK2_GITHUB_INTEGRATION.md

# Week 2: GitHub Integration — Detailed Documentation

> **Goal:** Receive GitHub webhooks, validate signatures, fetch PR data, post comments.
> **Status:** Complete — End-to-end tested with live PR
> **Date:** 2026-03-19
> **Test PR:** github.com/ninjacode911/codeguard-test/pull/1

---

## What We Built

This week we built the **communication layer** between Ninja Code Guard and GitHub —
the nervous system that listens for events, authenticates, fetches data, and responds.

**End-to-end flow achieved:**
```
PR opened on GitHub (21:54:52)
    → Webhook POST to our ngrok tunnel
    → HMAC-SHA256 signature validated
    → Redis cache checked (not previously reviewed)
    → Background task enqueued, 200 returned to GitHub
    → JWT signed with .pem, installation token obtained
    → PR diff + file contents fetched via GitHub API
    → Bot comment posted to PR #1
    → Commit SHA cached in Upstash Redis (7-day TTL)
    → Total time: ~5 seconds
```

---

## Step-by-Step Implementation Log

### Step 1: Webhook Signature Validation (app/github/webhook.py)

**What:** A FastAPI dependency that validates the HMAC-SHA256 signature on every
incoming webhook request.

**The problem it solves:** Our `/webhook/github` endpoint is publicly accessible. Without
validation, anyone could send fake webhook payloads to trigger bogus reviews, waste our
Groq API quota, or spam PRs with fake comments.

**How HMAC-SHA256 works:**

```
                    Shared Secret
                    (GITHUB_WEBHOOK_SECRET)
                         │
          ┌──────────────┼──────────────┐
          │              │              │
      GitHub's side      │        Our server's side
          │              │              │
    request body ──→ HMAC-SHA256   HMAC-SHA256 ←── request body
          │              │              │
          ▼              │              ▼
    computed hash        │        computed hash
          │              │              │
    sent as header ──────┼──────→ compared with
    X-Hub-Signature-256  │        received header
                         │
                    Must match!
```

**Key implementation details:**

1. **Raw bytes, not parsed JSON:** We compute the HMAC on the raw request bytes, not
   parsed JSON. Even a single whitespace difference would produce a completely different
   hash. This is why we use `await request.body()` before any JSON parsing.

2. **Constant-time comparison:** We use `hmac.compare_digest()` instead of `==`.
   A regular `==` short-circuits on the first different byte — an attacker could measure
   response time for different guesses and reconstruct the signature byte by byte.
   `compare_digest()` always takes the same time regardless of where the mismatch is.
   This is called a **timing attack** and is a real-world vulnerability (CVE-2013-0338, etc.).

3. **FastAPI dependency injection:** The validation is implemented as a `Depends()` function.
   FastAPI calls it automatically before the endpoint handler runs. If validation fails,
   the endpoint never executes. This ensures we can't accidentally forget to validate.

```python
# How it's used in the endpoint — validation happens automatically via Depends()
@app.post("/webhook/github")
async def webhook_github(
    body: bytes = Depends(validate_webhook_signature),  # ← runs first
):
    payload = json.loads(body)  # Only reached if signature is valid
```

**Signature format from GitHub:**
```
X-Hub-Signature-256: sha256=5d7230d4d964e5c12a7e4e94c...
                     ^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                     prefix   hex-encoded HMAC digest
```

**Interview talking point:** "We validate webhook authenticity using HMAC-SHA256 with
constant-time comparison to prevent timing attacks. The validation is implemented as a
FastAPI dependency so it's impossible to skip — the endpoint function only executes
after successful validation."

---

### Step 2: GitHub App JWT Authentication (app/github/auth.py)

**What:** Two-step authentication flow — sign a JWT, exchange it for a scoped token.

**The problem it solves:** We need to call GitHub's API (fetch PR data, post comments)
on behalf of our installed app. GitHub needs to verify that API calls are coming from
the registered "Ninja Code Guard" app, not from an impersonator.

**Step 1 — JWT Generation:**

A JWT (JSON Web Token) is a signed token with three parts:
```
eyJhbGciOiJSUzI1NiJ9.eyJpYXQiOjE3MTEuLi4sImV4cCI6MTcxMS4uLiwiX.SflKxwRJ...
└──────── Header ────────┘└────────── Payload ───────────┘└── Signature ──┘

Header:  {"alg": "RS256", "typ": "JWT"}
Payload: {"iat": <issued_at>, "exp": <expires_at>, "iss": "3133457"}
Signature: RSA-SHA256(header + "." + payload, private_key)
```

**Why RS256 (RSA + SHA-256)?**
- This is **asymmetric** cryptography: we sign with our private key (.pem), GitHub
  verifies with the matching public key (stored when we registered the app)
- Even if someone intercepts a JWT, they can't create new ones without the .pem file
- This is the same algorithm used by Google Cloud, AWS Cognito, and Auth0

**Code walkthrough:**
```python
def _generate_jwt() -> str:
    now = int(time.time())

    # Read the RSA private key from our .pem file
    project_root = Path(__file__).resolve().parent.parent.parent
    private_key_path = project_root / settings.github_app_private_key_path
    private_key = private_key_path.read_text()

    payload = {
        "iat": now - 60,        # Issued 60s ago (clock drift tolerance)
        "exp": now + (9 * 60),  # Expires in 9 minutes (GitHub max: 10min)
        "iss": settings.github_app_id,  # "I am app 3133457"
    }

    return jwt.encode(payload, private_key, algorithm="RS256")
```

**Path resolution bug we hit and fixed:**
- Original: `Path(settings.github_app_private_key_path)` → resolved relative to CWD
- Problem: When uvicorn runs, CWD might not be the project root
- Fix: `Path(__file__).resolve().parent.parent.parent / settings.github_app_private_key_path`
- This resolves relative to `auth.py`'s location → up to `app/github/` → `app/` → project root

**Step 2 — Installation Access Token:**

```python
async def get_installation_token(installation_id: int) -> str:
    # Check in-memory cache first
    cached = _token_cache.get(installation_id)
    if cached and cached["expires_at"] > time.time() + 60:
        return cached["token"]

    # Generate JWT and exchange for installation token
    app_jwt = _generate_jwt()
    response = await httpx.AsyncClient().post(
        f"https://api.github.com/app/installations/{installation_id}/access_tokens",
        headers={"Authorization": f"Bearer {app_jwt}"},
    )

    # Cache the token (valid for ~1 hour)
    _token_cache[installation_id] = {
        "token": response.json()["token"],
        "expires_at": time.time() + 3500,
    }
```

**Why cache the token?** Installation tokens last 1 hour. Without caching, we'd generate
a new JWT and make a token exchange API call for every single GitHub API request. Caching
reduces latency and API calls from ~10 per PR review to ~1 per hour.

**Interview talking point:** "GitHub Apps use a two-step auth flow — JWT for app identity,
installation tokens for repo-scoped access. We cache installation tokens in memory with
TTL-based expiry to avoid redundant token exchanges. This is the same client credentials
pattern used in OAuth2."

---

### Step 3: GitHub API Client (app/github/client.py)

**What:** An async HTTP client that fetches PR data and posts review comments.

**The problem it solves:** We need to:
1. Get the PR diff (what changed)
2. Get full file contents (for context — the diff alone isn't enough)
3. Post inline review comments (anchored to specific file+line)
4. Post a summary comment (health score, findings overview)

**Key design decisions:**

#### Why a class instead of standalone functions?
```python
class GitHubClient:
    def __init__(self, installation_id: int):
        self.installation_id = installation_id
        self._token = None  # Lazily fetched on first API call
```
The installation_id and token are shared across all API calls for one webhook event.
A class groups related operations with shared state. It's also easy to mock in tests.

#### Fetching the diff — two formats

```python
# JSON format (structured data about each file)
GET /repos/{owner}/{repo}/pulls/{pr_number}/files
→ [{filename: "app.py", status: "modified", additions: 5, patch: "..."}, ...]

# Raw diff format (the unified diff, same as `git diff`)
GET /repos/{owner}/{repo}/pulls/{pr_number}
Accept: application/vnd.github.diff
→ "diff --git a/app.py b/app.py\n--- a/app.py\n+++ b/app.py\n@@ -1,3 +1,8 @@..."
```

We fetch BOTH. The raw diff is sent to agents for analysis. The structured file list
tells us which files to fetch full contents for.

#### Why we fetch full file contents (not just the diff)

Consider this diff:
```diff
+ result = db.query(f"SELECT * FROM users WHERE id = {user_id}")
```

Questions an agent needs to answer:
- Is `user_id` sanitized upstream? → Need to see the function signature
- Is `db.query()` a safe ORM method or raw SQL? → Need to see the import
- Is this in a public-facing endpoint? → Need to see the route decorator

**Without full file:** Agent sees one line, guesses wildly, produces false positives.
**With full file:** Agent sees imports, class context, function scope — makes informed judgments.

```python
# How we fetch file contents
response = await http.get(
    f"{GITHUB_API}/repos/{repo}/contents/{filepath}",
    params={"ref": commit_sha},  # At the exact commit, not HEAD
)
# GitHub returns content as base64 (because JSON can't hold binary)
content_b64 = response.json()["content"]
source_code = base64.b64decode(content_b64).decode("utf-8")
```

#### Posting reviews — two types of comments

```
PR #1 conversation:
┌─────────────────────────────────────────────┐
│ 📋 Summary Comment (post_comment)           │  ← Top-level, in the conversation
│ Health Score: 65/100, 1 critical finding     │
└─────────────────────────────────────────────┘

Files changed tab:
┌─────────────────────────────────────────────┐
│ app.py                                      │
│ ...                                         │
│ + result = db.query(f"SELECT * FROM...")     │
│   🚨 [CRITICAL] SQL Injection Risk          │  ← Inline, anchored to this line
│   User input directly embedded...            │     (post_review with comments)
│ ...                                         │
└─────────────────────────────────────────────┘
```

```python
# Summary comment — uses Issues API (PRs are issues in GitHub's data model)
await http.post(
    f"{GITHUB_API}/repos/{repo}/issues/{pr_number}/comments",
    json={"body": "## Health Score: 65/100\n..."},
)

# Inline review — uses Pull Request Reviews API
await http.post(
    f"{GITHUB_API}/repos/{repo}/pulls/{pr_number}/reviews",
    json={
        "commit_id": commit_sha,
        "body": "Summary text",
        "event": "COMMENT",  # Don't approve/block — just comment
        "comments": [
            {"path": "app.py", "line": 5, "body": "🚨 SQL Injection..."},
        ],
    },
)
```

**Interview talking point:** "We fetch full file contents via GitHub's Contents API, not just
diffs, because our agents need surrounding context — imports, class definitions, function
signatures — to make accurate assessments. This is the same approach used by Sourcery and
CodeRabbit, but we go further by embedding this context into a vector store for semantic retrieval."

---

### Step 4: Comment Formatter (app/github/comment_formatter.py)

**What:** Converts our internal `Finding` objects into GitHub-flavored Markdown.

**Two output formats:**

#### Inline comment (per finding):
```markdown
🚨 **[CRITICAL — Security] SQL Injection Risk**

The query on line 5 constructs SQL via string interpolation.
User input is directly embedded without sanitization.

**Suggested fix:**
```python
cursor.execute('SELECT * FROM users WHERE id = %s', (user_id,))
```

> 🔒 Security · [CWE-89](https://cwe.mitre.org/data/definitions/89.html) · Confidence: 0.92
```

#### Summary comment (per PR):
```markdown
## ✅ Ninja Code Guard Review — Health Score: 85/100

`████████████████░░░░` **85**/100 — Healthy

### Findings Summary
| Severity | Count |
|----------|-------|
| 🚨 Critical | 0 |
| 🟠 High | 1 |
| 🟡 Medium | 2 |
| ℹ️ Low | 0 |

✅ **Recommendation: Approve** — No critical issues found.
```

**Design decisions:**
- Emoji prefixes for quick scanning (devs skim reviews)
- CWE IDs are hyperlinked (so devs can learn about vulnerabilities)
- Suggested fixes use fenced code blocks (easy copy-paste)
- Health bar uses Unicode block characters (works everywhere, no images needed)

---

### Step 5: Redis Cache (app/db/redis_cache.py)

**What:** Prevents re-analyzing the same PR commit that we've already reviewed.

**The problem it solves:** When a developer pushes multiple commits quickly, or force-pushes,
GitHub sends a webhook for each push. Without caching, we'd burn Groq API quota
re-analyzing the same code and spam the PR with duplicate comments.

**How it works:**
```
Webhook received with commit SHA "0c8ec514"
    │
    ├─ Check Redis: EXISTS ninjacg:reviewed:0c8ec514
    │   │
    │   ├─ Key exists → return "already reviewed" (skip)
    │   │
    │   └─ Key missing → proceed with analysis
    │                      │
    │                      ▼
    │                   Run agents...
    │                   Post comments...
    │                      │
    │                      ▼
    └─ Set Redis: SET ninjacg:reviewed:0c8ec514 "1" EX 604800
                                                      ^^^^^^
                                                   7 days TTL
```

**Key design decision — "Fail Open" pattern:**
```python
async def is_already_reviewed(commit_sha: str) -> bool:
    try:
        client = _get_redis_client()
        result = await client.exists(_cache_key(commit_sha))
        return bool(result)
    except Exception:
        # If Redis is DOWN, return False → proceed with analysis
        return False  # ← This is "fail open"
```

**Fail open vs. fail closed:**
- **Fail open:** If the check fails, allow the operation (may duplicate)
- **Fail closed:** If the check fails, block the operation (may miss reviews)

For a code review tool, **missing a review is worse than reviewing twice**, so we fail open.
This is the same pattern used by rate limiters and circuit breakers in production systems.

**Why Upstash Redis instead of in-memory cache?**
- Render's free tier restarts the server frequently (cold starts every 15 min)
- In-memory dict would be wiped on every restart
- Redis persists across restarts
- If we ever scale to multiple workers, they share the same cache

**Interview talking point:** "Our cache uses a fail-open pattern — if Redis is unavailable,
we proceed with analysis rather than blocking. This prioritizes availability over exact-once
semantics, which is correct for a non-critical review tool. The TTL-based expiry ensures
stale entries are automatically cleaned without manual maintenance."

---

### Step 6: Webhook Endpoint (app/main.py)

**What:** The FastAPI endpoint that receives GitHub webhooks and orchestrates the response.

**The full request lifecycle:**

```python
@app.post("/webhook/github")
async def webhook_github(
    request: Request,
    background_tasks: BackgroundTasks,
    x_github_event: str = Header(..., alias="X-GitHub-Event"),
    body: bytes = Depends(validate_webhook_signature),  # ← Runs FIRST
):
```

**Step-by-step:**

1. **HMAC validation** (via `Depends`): If signature is invalid → 401, endpoint never runs
2. **Parse payload**: `json.loads(body)` — we know the body is authentic now
3. **Filter events**: Only process `pull_request` events with actions: opened, synchronize, reopened, ready_for_review
4. **Skip drafts**: Draft PRs aren't ready for review
5. **Check cache**: `await is_already_reviewed(commit_sha)` — skip if already done
6. **Get installation ID**: Extracted from the webhook payload — needed for auth
7. **Enqueue background task**: `background_tasks.add_task(_process_pr_review, ...)`
8. **Return 200 immediately**: GitHub gets a fast response, processing continues in background

**Why background tasks?**

GitHub has a **10-second webhook timeout**. If we don't respond in time:
- GitHub marks the delivery as failed
- GitHub retries (up to 3 times at increasing intervals)
- We'd get duplicate reviews

Our actual review pipeline takes 15-20 seconds (agent calls + synthesis). So we:
1. Return 200 immediately (~50ms)
2. Process the review in FastAPI's background task queue
3. GitHub is happy, we have unlimited time to process

```python
# This returns 200 to GitHub immediately
background_tasks.add_task(
    _process_pr_review,
    repo_full_name=repo_full_name,
    pr_number=pr_number,
    commit_sha=commit_sha,
    installation_id=installation_id,
)
return {"status": "accepted", "pr": pr_number}
# ↑ GitHub gets this response in ~50ms
# ↓ Meanwhile, _process_pr_review runs in the background
```

**The background task (_process_pr_review):**
```python
async def _process_pr_review(...):
    client = GitHubClient(installation_id)
    pr_data = await client.fetch_pr_data(repo_full_name, pr_number)

    # TODO (Week 3-7): Run agents here
    # For now: post a dummy comment proving the pipeline works

    await client.post_comment(repo_full_name, pr_number, summary)
    await mark_as_reviewed(commit_sha)
```

**Interview talking point:** "We use FastAPI's background tasks to acknowledge webhooks within
GitHub's 10-second timeout, then process asynchronously. The webhook handler follows a
filter-then-dispatch pattern — irrelevant events are filtered early (wrong event type, draft PR,
already cached), and only valid PR events trigger the expensive analysis pipeline."

---

### Step 7: Unit Tests

**What:** 20 tests covering all critical paths.

#### Test Suite: Webhook Validation (5 tests)
```
test_valid_signature_accepted     — Correctly signed payload → 200 ✅
test_invalid_signature_rejected   — Wrong secret → 401 ✅
test_tampered_payload_rejected    — Valid sig for different payload → 401 ✅
test_missing_signature_rejected   — No header → 422 ✅
test_malformed_signature_rejected — No "sha256=" prefix → 401 ✅
```

**How the tests work:**
```python
# We create a minimal FastAPI app just for testing
test_app = FastAPI()
TEST_SECRET = "test_webhook_secret_for_unit_tests"

@test_app.post("/webhook-endpoint")
async def webhook_endpoint(body: bytes = Depends(validate_webhook_signature)):
    return {"status": "ok"}

# monkeypatch overrides the real secret with our test secret
@pytest.fixture
def client(monkeypatch):
    monkeypatch.setattr(
        "app.github.webhook.settings.github_webhook_secret",
        TEST_SECRET,
    )
    return TestClient(test_app)

# Then we compute the expected signature ourselves
def _compute_signature(payload: bytes, secret: str) -> str:
    sig = hmac.new(secret.encode(), payload, hashlib.sha256).hexdigest()
    return f"sha256={sig}"
```

**Key testing pattern:** `monkeypatch` temporarily overrides the real webhook secret
so tests are deterministic and don't depend on `.env` values. This is standard
practice — tests should never use real credentials.

#### Test Suite: Redis Cache (7 tests)
```
test_returns_false_for_new_commit        — New SHA → not reviewed ✅
test_returns_true_for_cached_commit      — Cached SHA → already reviewed ✅
test_redis_failure_returns_false          — Redis down → fail open (False) ✅
test_sets_key_with_ttl                   — SET with 7-day expiry ✅
test_redis_failure_does_not_raise        — Redis SET fails → no crash ✅
test_deletes_key                         — Cache invalidation works ✅
test_redis_failure_does_not_raise (del)  — Redis DELETE fails → no crash ✅
```

**How the tests work:**
```python
@pytest.fixture
def mock_redis():
    mock = AsyncMock()  # Python's built-in mock for async functions
    with patch("app.db.redis_cache._get_redis_client", return_value=mock):
        yield mock

# Example: testing fail-open behavior
async def test_redis_failure_returns_false(mock_redis):
    mock_redis.exists.side_effect = ConnectionError("Redis unavailable")
    result = await is_already_reviewed("abc123")
    assert result is False  # Fail open — proceed with analysis
```

**Key testing pattern:** `AsyncMock` simulates Redis responses without a real Redis
connection. Tests run in milliseconds, offline, and are deterministic.

#### Test Suite: Schema Validation (8 tests)
```
test_valid_finding                           — Valid data → accepted ✅
test_finding_rejects_invalid_agent          — "invalid" agent → ValidationError ✅
test_finding_rejects_invalid_severity       — "urgent" severity → ValidationError ✅
test_finding_confidence_bounds              — 1.5 and -0.1 → ValidationError ✅
test_finding_optional_cwe_id               — None cwe_id → accepted ✅
test_valid_review                           — Valid review → accepted ✅
test_review_health_score_bounds            — 101 and -1 → ValidationError ✅
test_review_rejects_invalid_recommendation — "maybe" → ValidationError ✅
```

---

### Step 8: End-to-End Test with ngrok

**What:** Tested the full pipeline live — from GitHub PR to bot comment.

**Setup:**
1. Started FastAPI server: `uvicorn app.main:app --reload --port 8000`
2. Started ngrok tunnel: `ngrok http 8000` → got public URL
3. Updated GitHub App webhook URL to the ngrok URL
4. Created test repo: github.com/ninjacode911/codeguard-test
5. Installed "Ninja Code Guard" app on the test repo
6. Created a PR with a SQL injection vulnerability in app.py

**Test code in the PR (intentionally vulnerable):**
```python
import sqlite3

def get_user(user_id):
    conn = sqlite3.connect("users.db")
    query = f"SELECT * FROM users WHERE id = {user_id}"  # SQL injection!
    return conn.execute(query).fetchone()

def delete_user(name):
    conn = sqlite3.connect("users.db")
    conn.execute(f"DELETE FROM users WHERE name = '{name}'")  # SQL injection!
```

**What happened (from server logs):**
```
22:01:19  Webhook received — review enqueued  (action=opened, pr=1, sha=0c8ec514)
22:01:19  Starting PR review                   (HMAC validated ✅)
22:01:23  Fetched PR data                      (1 changed file, 1 file with content)
22:01:24  Posted PR comment                    (Bot comment appeared on PR)
22:01:24  Cached review result                 (TTL 7 days in Upstash Redis)
22:01:24  PR review completed                  (Total: ~5 seconds)
```

**Bugs encountered and fixed:**

| Bug | Cause | Fix |
|-----|-------|-----|
| `TypeError: meth() got multiple values for argument 'event'` | structlog reserves `event` as a keyword | Changed `event=x_github_event` to `github_event=x_github_event` |
| `FileNotFoundError: 'keys\\app.pem'` | .pem filename didn't match .env path | Updated .env to use actual filename: `ninja-s-code-guard.2026-03-19.private-key.pem` |
| Same .pem error after .env fix | `Path("./keys/app.pem")` resolves relative to CWD, not project root | Changed to `Path(__file__).resolve().parent.parent.parent / path` |

**Result:** Bot comment posted successfully to PR #1 at github.com/ninjacode911/codeguard-test/pull/1

---

## Files Created/Modified in Week 2

| File | Type | Purpose |
|------|------|---------|
| `app/github/webhook.py` | **New** | HMAC-SHA256 webhook signature validation |
| `app/github/auth.py` | **New** | GitHub App JWT + installation token authentication |
| `app/github/client.py` | **New** | GitHub REST API client (fetch PR data, post comments) |
| `app/github/comment_formatter.py` | **New** | Finding → GitHub Markdown conversion |
| `app/db/redis_cache.py` | **New** | Commit SHA deduplication cache (Upstash Redis) |
| `app/main.py` | **Modified** | Added webhook endpoint + background task processing |
| `requirements.txt` | **Modified** | Added PyJWT[crypto] dependency |
| `tests/unit/test_webhook_validation.py` | **New** | 5 tests for HMAC validation |
| `tests/unit/test_redis_cache.py` | **New** | 7 tests for cache logic |
| `docs/WEEK2_GITHUB_INTEGRATION.md` | **New** | This documentation file |

---

## Dependencies Added

| Package | Version | Purpose |
|---------|---------|---------|
| `PyJWT[crypto]` | >=2.9.0 | JWT generation with RS256 (includes cryptography backend) |
| `httpx` | >=0.28.0 | Async HTTP client for GitHub API calls |
| `redis` | >=5.2.0 | Async Redis client for Upstash |
| `structlog` | >=24.4.0 | Structured logging (JSON-formatted, key-value pairs) |

---

## Architecture Patterns Used (Interview Reference)

| Pattern | Where Used | What It Means |
|---------|------------|---------------|
| **HMAC authentication** | webhook.py | Symmetric key message authentication |
| **Asymmetric JWT auth** | auth.py | RSA private key signing, public key verification |
| **Token caching** | auth.py | In-memory cache with TTL for installation tokens |
| **Dependency injection** | main.py | FastAPI Depends() for webhook validation |
| **Background tasks** | main.py | Async processing after immediate HTTP response |
| **Fail-open pattern** | redis_cache.py | If cache check fails, proceed (don't block) |
| **Separation of concerns** | All files | Each module has a single responsibility |

---

## What's Next (Week 3)

The dummy comment will be replaced with the real **Security Agent** output.
The agent will use Semgrep, Bandit, and Groq's Llama-3.1-70B to find the SQL injection
vulnerabilities in our test PR's `app.py`.

---

*Documentation written 2026-03-19 as part of Week 2 completion.*