colibri/docs/wiki/pull-requests.md

Pull Requests

A pull request (PR) is how we propose and review changes to the codebase. It's the primary way we collaborate on code - we branch off, make changes, and request those changes be pulled into the main codebase.

What is a PR?

A PR is a proposal to change code. It shows:

• What changed - the diff of modified files
• Why it changed - commit messages explaining the reasoning
• How it works - tests proving it works correctly

Every code change goes through a PR before being merged to main. Even small fixes.

Our Workflow

1. Create a Branch

# From main branch
git checkout main
git pull origin main
git checkout -b descriptive-branch-name

# Example branch names:
# - fix/format-drift
# - feat/eval-harness
# - docs/model-selection
# - refactor/remove-legacy

2. Make Changes

# Edit files, run tests
cargo fmt
cargo clippy
cargo test --workspace

# Commit with clear messages
git add -A
git commit -m "fix: resolve format drift in daemon.rs

Ran cargo fmt to fix method chain formatting.
No functional changes - just formatting compliance.

Sam & Claude"

3. Push Branch

git push -u origin descriptive-branch-name

4. Create PR

Go to Forgejo web UI or use the link from the push output:

https://forgejo.smilepowered.org/clawdie/colibri/compare/main...descriptive-branch-name

Fill out the PR description:

• What: What does this PR do?
• Why: Why is this change needed?
• How: How was it implemented?
• Testing: What tests prove it works?

5. Review

Another agent (or human) reviews the PR:

• Checks that tests pass
• Checks that formatting/clippy are clean
• Reviews the code for correctness
• May request changes

6. Merge

Once approved:

• Squash merge to main (keeps history clean)
• Or regular merge if commits tell a story worth preserving
• Delete the branch after merge

7. Clean Up

# Back on main
git checkout main
git pull origin main

# Delete local branch (optional)
git branch -d descriptive-branch-name

When to Create a PR

Always create a PR for:

• Any code changes (even small fixes)
• Refactors
• New features
• Bug fixes
• Documentation updates

Never commit directly to main. Main is protected - you can't anyway.

Branch Naming

Use descriptive, prefix-style names:

• fix/ - bug fixes (e.g., fix/format-drift)
• feat/ - new features (e.g., feat/eval-harness)
• docs/ - documentation (e.g., docs/model-selection)
• refactor/ - code refactoring (e.g., refactor/remove-legacy)
• test/ - test additions (e.g., test/integration-eval)

Commit Messages

Write clear, specific commit messages:

Good:

fix: resolve format drift in daemon.rs

Ran cargo fmt to fix method chain formatting.
No functional changes - just formatting compliance.

Sam & Claude

Bad:

fixed stuff

Structure:

1. Type prefix: fix:, feat:, docs:, refactor:, test:
2. One-line summary: What changed and why (imperative mood)
3. Body: Details if needed (blank line after summary)
4. Attribution: Sam & Claude for collaborative work

Review Checklist

Before requesting review (or when reviewing):

• cargo fmt passes (formatting is clean)
• cargo clippy passes (no warnings)
• cargo test --workspace passes (all tests green)
• ./scripts/wiki-lint passes (no dead links)
• Code is readable and well-commented
• Commit messages are clear and descriptive
• No unrelated changes in the PR

Common Patterns

Small Fix PR

One commit, one clear fix:

git commit -m "fix: resolve cargo fmt drift

Ran cargo fmt on daemon.rs to fix method chain formatting.

Sam & Claude"

Feature PR

Multiple commits telling a story:

feat: add eval harness for task completion

Phase 1 implementation:
- task_evals table schema
- write_task_eval/read_task_eval methods
- record_task_completion hook in daemon
- colibri_get_task_eval MCP tool

Tests:
- 6 new tests covering schema, CRUD, roundtrip
- All existing tests still pass

Sam & Claude

Refactor PR

Rename/reorganize with clear rationale:

refactor: rename golden tests to fixtures

Replace jargon "golden tests" with clearer "fixtures tests".
Updates all documentation and comments.

Preserved intentional "golden line" metaphor in tokenomics doc.

Sam & Claude

What Makes a Good PR?

✅ Clear purpose: One focused change, not a grab-bag ✅ Tests pass: All gates green before requesting review ✅ Small scope: Easier to review, easier to revert if needed ✅ Clear commits: Each commit tells part of the story ✅ Good description: Explains what, why, and how ✅ Self-contained: Doesn't break other things

❌ Multiple unrelated changes: Split into separate PRs ❌ Failing tests: Fix tests before requesting review ❌ Unclear commits: "fixed stuff" doesn't explain anything ❌ Huge scope: Break large changes into smaller PRs ❌ Missing description: Reviewer shouldn't have to guess why

Merging Strategy

Squash merge when:

• PR has messy commit history
• Commits are interdependent
• Want clean linear history

Regular merge when:

• Each commit tells part of a story
• Commits are independently reviewable
• Want to preserve development history

When in doubt, squash. Main stays cleaner.

After Merge

# Pull latest main
git checkout main
git pull origin main

# Delete local branch
git branch -d my-feature-branch

# Optional: delete remote branch (Forgejo usually does this)
git push origin --delete my-feature-branch

Troubleshooting

PR has merge conflicts

git checkout my-feature-branch
git pull origin main
# Resolve conflicts
git add .
git commit -m "merge main into my-feature-branch"
git push

Tests fail after push

• Check CI logs
• Run tests locally: cargo test --workspace
• Fix issues, push again

Reviewer requests changes

• Make requested changes
• Commit with clear message: fix: address review feedback
• Push and re-request review

Examples from Our Project

Recent PRs in colibri:

• feat/eval-harness-phase1 - Added evaluation harness (732 lines changed)
• refactor/remove-legacy - Removed legacy references (11 files)
• rename/golden-to-fixtures - Renamed test terminology (10 files)
• fix/smoke-to-test - Renamed smoke tests to unit tests

These all followed the pattern: branch → commit → push → PR → review → merge → cleanup.

Summary

PRs are how we collaborate on code. They provide:

• Visibility: Everyone sees what's changing
• Review: Catch bugs before they hit main
• History: Clear record of why changes were made
• Quality: Gates ensure code meets standards

Always use PRs. Never commit directly to main. Write clear descriptions. Keep tests green.