pr-creation

What this skill does

# Freenet Pull Request Quality Standards

## Core Philosophy

**Our goal is high-quality code that won't require future fixes.** Don't cut corners, be a perfectionist, don't increase tech debt. A quick fix that causes problems later wastes more time than doing it right the first time.

## Before Creating the PR

### Claim the Issue

If your PR fixes a GitHub issue, **verify the issue is unassigned** before starting work. If someone else is already assigned, check with them or the user before proceeding — don't duplicate effort. If the issue is unassigned, assign it to yourself immediately so others know it's being worked on:

```bash
gh issue edit <ISSUE> --repo freenet/<REPO> --add-assignee @me
```

### Sync with Latest Main from GitHub

**CRITICAL:** Always ensure you're working from the latest `main` branch from GitHub, not a stale local copy:

```bash
cd ~/code/freenet/freenet-core/main
git fetch origin
git log --oneline -1 origin/main  # Check what's latest on GitHub
git pull origin main              # Update local main
git log --oneline -1              # Verify you have the latest
```

This prevents:
- Working on outdated code that's already been fixed
- Merge conflicts when the PR is ready
- Basing work on code that's already been superseded

### Assign the Issue

If you're working on a GitHub issue, assign it to yourself so others know it's being worked on:

```bash
gh issue edit <ISSUE> --repo freenet/<REPO> --add-assignee @me
```

### Create a Worktree

Never work directly in the main worktree. Create a dedicated worktree for your branch:

```bash
cd ~/code/freenet/freenet-core/main
git worktree add ../fix-<issue-number> -b fix-<issue-number>
cd ../fix-<issue-number>
```

### Verify Your Environment

```bash
# CRITICAL: Verify you're in a worktree, not the main directory
pwd  # Should be .../freenet-core/<branch-name>, NOT .../freenet-core/main
git branch --show-current  # Should be your feature branch
```

### Run Local Checks

```bash
cargo fmt
cargo clippy --all-targets --all-features
cargo test
```

Fix all warnings and errors before pushing.

### Choosing the Right Test Level

**Simulation tests (primary):** For logic errors in routing, topology, subscriptions, operations, or any behavioral change. Use the direct simulation runner (`run_simulation_direct`) in `crates/core` — it's deterministic, fast, runs in CI, and has zero external dependencies. This should be the default for validating that changes don't regress subscribe rates, GET rates, tree formation, etc.

**Docker NAT simulation:** Only for transport-layer issues that require real network namespaces — firewall hole-punching, NAT traversal, UDP behavior behind iptables rules. In-memory sockets can't reproduce these. Don't use this for testing routing logic or subscription behavior.

**Manual multi-machine testing:** For issues that require real geographic distribution, actual internet latency, or cross-architecture validation (nova/vega/technic).

## PR Title and Description

### Title Format

PR titles **must** follow Conventional Commits - CI fails non-conforming titles:
- `feat:` - New feature
- `fix:` - Bug fix
- `docs:` - Documentation only
- `refactor:` - Code change that neither fixes a bug nor adds a feature
- `test:` - Adding or correcting tests
- `chore:` - Maintenance tasks

### Description Requirements

**Explain WHY, not just WHAT.** Structure your PR description:

```markdown
## Problem
[What's broken? What's the user impact? Why does it matter?]

## Approach
[Why this solution over alternatives? What's the key insight?]

## Testing
[New tests added and what scenarios they validate]
[Local validation steps performed]
[E2E testing results if applicable]

## Fixes
Closes #XXXX
```

**Bad:** "Add observed_addr field to ConnectRequest"
**Good:** "The joiner can't know its public address until observed externally. Previous approach rewrote addresses at transport boundary, but that's a hack. This lets the gateway fill in the observed socket naturally since it already sees the real UDP source."

## Test Quality Standards

### CI Must Be a Reliable Safety Net

**The test harness should be growing faster than the core code.** Quick fixes without test coverage create an illusion of speed — we end up in a patch → break → patch cycle where each fix introduces new regressions. Almost every logical bug can be reproduced in a synthetic test with zero external dependencies. The work to build those tests is harder than a quick fix, but it's the only way to stop the cycle.

**Every PR that changes behavior must include tests that would have caught the problem before merge.** Not just tests that verify the fix works — tests that would have prevented the bug from shipping in the first place. If you're changing routing logic, test that routing actually makes correct decisions under realistic data conditions. If you're changing topology, test that subscribe/GET success rates don't regress. If you're changing subscription handling, test that subscription trees form and survive peer churn.

**CI gap tests go in the same PR, not a follow-up issue.** The pattern of filing "add missing test" issues that never get done is how we end up with 17 bug-fix PRs in 4 days and none of them caught by CI. If the fix PR doesn't include the test that closes the gap, it is not ready to merge.

### A Bug That Made It Past CI Is Also a Bug in CI

When fixing a bug, always ask: **"Why didn't CI catch this?"**

Investigate which test layer should have caught it:
- Unit tests for logic errors
- Integration tests for component interactions
- Network simulations for distributed behavior
- E2E tests for real-world scenarios

Document the gap in your PR description. **Then close the gap.** The fix PR should include the missing test that would have caught this bug class, not just a narrow regression test for the specific symptom.

### Simulation Health Metrics (Required)

For PRs that touch routing, topology, operations, or subscription handling, the PR **must** include simulation tests that assert key health metrics. This is not optional — discovering regressions through production telemetry days later means CI failed:

| Metric | What it catches |
|--------|-----------------|
| Subscribe success rate | Topology regressions, interest TTL issues |
| GET success rate | Routing regressions |
| Subscription tree formation | Tree building/maintenance bugs |
| Interest renewal rate | TTL/renewal bugs |
| Time to first successful operation | Bootstrap/convergence issues |

These should be measured in simulation tests and asserted against thresholds. **Discovering regressions days later through production telemetry is a test infrastructure failure.**

Use the direct simulation runner (`run_simulation_direct`) for these — it supports multi-peer networks with contract operations and is deterministic.

### Don't Discover Logic Errors in Production

If a bug is a **logic error** (wrong threshold, missing data path, incorrect fallback), it should be reproducible in a unit or integration test. Don't settle for "we'll monitor it in production." Examples:

- Router prediction function requires data from 3 estimators but threshold only checks 1 → unit test with realistic data mix
- Topology change breaks subscribe routing → simulation test measuring subscribe success rate
- Interest TTL too aggressive → test that interests survive under congestion

Reserve production telemetry analysis for genuinely environment-specific issues (OOM under specific conditions, real NAT traversal, etc.), not for catching logic errors that a test should find.

### Regression Tests Must Reproduce the Bug

1. Write the test **before** the fix
2. Verify the test **fails** without your fix
3. Verify the test **passes** with your fix

This ensures the test actually catches the bug, not just the happy path.

### Make Tests General

When improving tests, make the new test as general as possible while still catching the specific problem found. A test that