# Contributing to SF

We're glad you're here. SF is an open project and contributions are welcome across the entire codebase. We hold a high bar for what gets merged — not to be gatekeepers, but because every change ships to real users and stability matters.

Read [VISION.md](VISION.md) before contributing. It defines what SF is, what it isn't, and what we won't accept.

## Before you start

1. **Check existing issues.** Someone may already be working on it.
2. **Claim the issue.** Comment on the issue to get it assigned to you before writing code. This prevents duplicate work and wasted effort.
3. **No issue? Create one first** for new features. Bug fixes for obvious problems can skip this step.
4. **Architectural changes require an RFC.** If your change touches core systems (auto-mode, agent-core, orchestration), open an issue describing your approach and get approval before writing code. We use Architecture Decision Records (ADRs) for significant decisions.

## Branching and commits

Always work on a dedicated branch. Never push directly to `main`.

**Branch naming:** `<type>/<short-description>`

| Type | When to use |
|------|-------------|
| `feat/` | New functionality |
| `fix/` | Bug or defect correction |
| `refactor/` | Code restructuring, no behavior change |
| `test/` | Adding or updating tests |
| `docs/` | Documentation only |
| `chore/` | Dependencies, tooling, housekeeping |
| `ci/` | CI/CD configuration |

**Commit messages** must follow [Conventional Commits](https://www.conventionalcommits.org/). The commit-msg hook enforces this locally; CI enforces it on push.

```
<type>(<scope>): <short summary>
```

Valid types: `feat` `fix` `docs` `chore` `refactor` `test` `infra` `ci` `perf` `build` `revert`

```
feat(pi-agent-core): add streaming output for long-running tasks
fix(pi-ai): resolve null pointer on empty provider response
chore(deps): bump typescript from 5.3.0 to 5.4.2
```

Keep branches current by rebasing onto `main` — do not merge `main` into your feature branch:

```bash
git fetch origin
git rebase origin/main
```

## Working with SF (team workflow)

SF uses worktree-based isolation for multi-developer work. If you're contributing with SF running, enable team mode in your project preferences:

```yaml
# .sf/PREFERENCES.md
---
version: 1
mode: team
---
```

This enables unique milestone IDs, branch pushing, and pre-merge checks — preventing milestone ID collisions when multiple contributors run auto-mode simultaneously. Each developer gets their own isolated worktree; squash merges to `main` happen independently.

For full details see [docs/working-in-teams.md](docs/working-in-teams.md) and [docs/git-strategy.md](docs/git-strategy.md).

## Opening a pull request

### PR description format

Every PR needs a **TL;DR** and a **detailed explanation**. Use this structure:

```
## TL;DR

**What:** One sentence — what does this change?
**Why:** One sentence — why is it needed?
**How:** One sentence — what's the approach?

## What

Detailed description of the change. What files, modules, or systems are affected?

## Why

The motivation. What problem does this solve? What was broken, missing, or suboptimal?
Link issues where applicable: `Closes #123`

## How

The approach. How does the implementation work? What were the key decisions?
If this is a non-trivial change, explain the design and any alternatives you considered.
```

### Requirements

- **CI must pass.** If your PR breaks tests, fix them before requesting review.
- **One concern per PR.** A bug fix is a bug fix. A feature is a feature. Don't bundle unrelated changes.
- **No drive-by formatting.** Don't reformat code you didn't change. Don't reorder imports in files you're not modifying.
- **Link issues when relevant.** Not mandatory for every PR, but if an issue exists, reference it.

### Change type checklist

Include in your PR:

- [ ] `feat` — New feature or capability
- [ ] `fix` — Bug fix
- [ ] `refactor` — Code restructuring (no behavior change)
- [ ] `test` — Adding or updating tests
- [ ] `docs` — Documentation only
- [ ] `chore` — Build, CI, or tooling changes

### Breaking changes

If your PR changes any public API, CLI behavior, config format, or file structure, say so explicitly. Breaking changes need extra scrutiny and may need migration guidance.

## AI-assisted contributions

AI-generated PRs are first-class citizens here. We welcome them. We just ask for transparency:

- **Disclose it.** Note that the PR is AI-assisted in your description. Do not credit the AI tool as an author or co-author in the commit or PR.
- **Test it.** AI-generated code must be tested to the same standard as human-written code. "The AI said it works" is not a test plan.
- **Understand it.** You should be able to explain what the code does and why. If a reviewer asks a question, "I'll ask the AI" is not an answer.

AI agents opening PRs must follow the same workflow as human contributors: clean working tree, new branch per task, CI passing before requesting review. Multi-phase work should start as a Draft PR and only move to Ready when complete.

AI PRs go through the same review process as any other PR. No special treatment in either direction.

## Architecture guidelines

Before writing code, understand these principles:

- **Extension-first.** Can this be an extension instead of a core change? If yes, build it as an extension.
- **Simplicity wins.** Don't add abstractions, helpers, or utilities for one-time operations. Don't design for hypothetical future requirements.
- **Tests are the contract.** Changed behavior? The test suite tells you what you broke.
- **Future-shape compatibility.** Every slice answers two questions before merge: (a) "does it work for today's step?" and (b) "is the shape it leaves consistent with where the documented next step goes?" If (b) is no, the small refactor that makes it yes is *part of the slice*, not a follow-up. This is not future-proofing for hypothetical needs — it's keeping today's slice aligned with documented next milestones (e.g. M048 firewall, M028 federation, A2A external boundary). Concrete examples: when adding a feature flag, route it through a config file rather than hardcoding so the next slice can toggle without a rebuild; when building a state aggregator, wrap state-source behind an interface so swapping providers later doesn't require rewriting the consumer.

See [VISION.md](VISION.md) for the full list of what we won't accept.

## Scope areas

The codebase is organized into these areas. All are open to contributions:

| Area | Path | Notes |
|------|------|-------|
| Terminal UI | `packages/pi-tui` | Components, themes, rendering |
| AI/LLM layer | `packages/pi-ai` | Provider integrations, model handling |
| Agent core | `packages/pi-agent-core` | Agent orchestration — RFC required for changes |
| Coding agent | `packages/pi-coding-agent` | The main coding agent |
| SF extension | `src/resources/extensions/sf/` | SF workflow — RFC required for auto-mode |
| MCP client | `src/resources/extensions/mcp-client/` | External MCP tool-server integration only |
| Other extensions | `src/resources/extensions/` | Browser, search, voice, MCP client, etc. |
| Native engine | `rust-engine/` | Rust N-API modules (grep, git, AST, etc.) |
| VS Code extension | `vscode-extension/` | Chat participant, sidebar, RPC integration |
| Web interface | `web/` | Browser-based dashboard |
| CI/Build | `.github/`, `scripts/` | Workflows, build scripts |
| Documentation | `docs/` | User guides, ADRs, SDK docs |

## Review process

PRs go through automated review first, then human review. To help us review efficiently:

- Keep PRs focused and reasonably sized. Massive PRs take longer to review and are more likely to be sent back.
- Respond to review comments. If you disagree, explain why — discussion is welcome.
- If your PR has been open for a while without review, ping in Discord. We're a small team and things slip.

### What reviewers verify

Reading a diff is not the same as verifying a change. Our review standard is execution-based, not static-analysis-based.

**What reviewers do:**

1. **Check out the branch** — check out the PR branch locally (or in a worktree). Don't review from the diff view alone.
2. **Build the branch** — run `npm run build`. A diff that doesn't compile is not reviewable.
3. **Run the test suite** — run `npm test`. CI status is a signal, not a substitute for local verification.
4. **Trace root cause for bug fixes** — confirm the diff addresses the root cause described in the issue, not just the symptom.
5. **Check for a regression test** — bug fixes must include a test that would have caught the original bug. If it's absent, the fix is incomplete.

Only after completing these steps should a reviewer make claims about correctness.

**What "looks right" means:**

"Looks right" is the starting point for review, not the conclusion. "The tests pass" only means the tests pass — not that the claimed bug is fixed or the feature works as described. A well-written commit message on a broken change is still a broken change.

### What contributors must provide to unblock review

- **Bug fixes** — include a regression test. A fix without a test is an assertion, not a proof.
- **Features** — include tests covering the primary success path and at least one failure path.
- **Behavior changes** — update or replace any existing tests that cover the changed behavior. Don't leave passing-but-wrong tests in place.

If your PR claims to fix issue #N, reviewers will verify the fix addresses the root cause described in #N — not just that CI is green.

## Testing standards

This project uses Node.js built-in `node:test` as the test runner. All new tests must follow these patterns:

### Use `node:test` and `node:assert/strict`

```typescript
import { describe, test, beforeEach, afterEach } from "node:test";
import assert from "node:assert/strict";
```

Do not use `createTestContext()` from `test-helpers.ts` (legacy, being removed). Do not introduce Jest, Vitest, or other test frameworks.

### Use `beforeEach`/`afterEach` or `t.after()` for cleanup — never `try`/`finally`

```typescript
// ✅ CORRECT — shared fixture with beforeEach/afterEach
describe("feature", () => {
  let tmp: string;
  beforeEach(() => { tmp = mkdtempSync(join(tmpdir(), "test-")); });
  afterEach(() => { rmSync(tmp, { recursive: true, force: true }); });

  test("case", () => { /* clean test body */ });
});

// ✅ CORRECT — per-test cleanup with t.after()
test("case", (t) => {
  const tmp = mkdtempSync(join(tmpdir(), "test-"));
  t.after(() => { rmSync(tmp, { recursive: true, force: true }); });
  // test body
});

// ❌ WRONG — inline try/finally
test("case", () => {
  const tmp = mkdtempSync(join(tmpdir(), "test-"));
  try {
    // test body
  } finally {
    rmSync(tmp, { recursive: true, force: true });
  }
});
```

**When to use which:**
- `beforeEach`/`afterEach` — when all tests in a `describe` block share the same setup/teardown pattern
- `t.after()` — when each test has unique cleanup (different fixtures, env vars, etc.)
- `try`/`finally` — only inside standalone helper functions that don't have access to the test context `t` (e.g., `withEnv()`, `capture()`)

### Template literal fixture data

When constructing multi-line fixture content (markdown, YAML, etc.) inside indented test blocks, use array join to avoid unintended leading whitespace:

```typescript
// ✅ CORRECT — no indentation leakage
const content = [
  "## Slices",
  "- [x] **S01: First slice**",
  "- [ ] **S02: Second slice**",
].join("\n");

// ❌ WRONG — template literal inside describe/test adds leading spaces
const content = `
  ## Slices
  - [x] **S01: First slice**
`;
// Each line now has 2 leading spaces, breaking ^## regex anchors
```

### Test-first for bug fixes

Bug fixes must include a regression test that fails before the fix and passes after. Write the test first, confirm it fails, then apply the fix. See the `test-first-bugfix` skill.

## Local development

```bash
# Install dependencies
npm ci

# Install git hooks (secret scanning + commit message validation)
npm run secret-scan:install-hook

# Build
npm run build

# Run tests
npm test

# Type check
npx tsc --noEmit
```

Run `npm run secret-scan:install-hook` once after cloning. It installs two hooks:
- **pre-commit** — blocks commits containing hardcoded secrets or credentials
- **commit-msg** — validates Conventional Commits format before the commit lands

CI must pass before your PR will be reviewed. Run these locally to save time.

## Security

If you find a security vulnerability, **do not open a public issue.** Email the maintainers directly or use GitHub's private vulnerability reporting.

## Questions?

Open a discussion on GitHub or ask in the Discord `#maintainers` channel.
