loom-builder-pr

Builder: PR Creation and Quality

This document covers PR creation, test output handling, and quality requirements for the Builder role. For the core builder workflow, see builder.md.

Contents

• Pre-Implementation Review: Check Recent Main Changes
• Test Output: Truncate for Token Efficiency
• Acceptance Criteria Verification: REQUIRED Before PR Creation
• Test-First Discipline (TDD line, required in PR body)
• PR Titles: Conventional Commit Style Required
• Commit Messages: Same Rules as PR Titles
• Creating Pull Requests: Label and Auto-Close Requirements
• Handling Pre-existing Lint/Build Failures
• Raising Concerns

Pre-Implementation Review: Check Recent Main Changes

CRITICAL: Before implementing, review recent changes to main to avoid conflicts with recent architectural decisions.

Why This Matters

The codebase evolves while you work. Recent PRs may have:

• Introduced new utilities or helper functions you should use
• Changed authentication/authorization patterns
• Updated API conventions or response formats
• Added shared components or abstractions
• Modified configuration or environment handling

Without this review: You may implement using outdated patterns, leading to merge conflicts, inconsistent code, or duplicated functionality.

Required Commands

Step 1: Review recent commits to main

# Show last 20 commits to main
git fetch origin main
git log --oneline -20 origin/main

# Show files changed in recent commits
git diff HEAD~20..origin/main --stat

Step 2: Check changes in your feature area

# Check recent changes in directories related to your feature
git log --oneline -10 origin/main -- "src/relevant/path"
git log --oneline -10 origin/main -- "*.ts"  # or relevant file types

Step 3: Look for these architectural changes

Change Type	What to Look For	Why It Matters
Authentication	New auth middleware, session handling, token patterns	Use the new auth approach, not old patterns
API Patterns	Response formats, error handling, validation	Match existing conventions
Utilities	New helper functions, shared modules	Reuse instead of reimplementing
Shared Components	Common UI elements, base classes	Extend rather than duplicate
Configuration	New env vars, config patterns	Follow established patterns

Example Workflow

# 1. Fetch latest main
git fetch origin main

# 2. See what changed recently
git log --oneline -20 origin/main
# 5b55cb7 Add dependency unblocking to Guide role (#997)
# cc41f95 Add guidance for handling pre-existing lint/build failures (#982)
# 6b55a3e Add rebase step before PR creation (#980)
# ...

# 3. If you see relevant changes, investigate
git show 5b55cb7 --stat  # See what files changed
git show 5b55cb7          # See the actual changes

# 4. Check changes in your feature area
git log --oneline -10 origin/main -- "src/lib/auth"
# -> If you see auth changes, read them before implementing!

# 5. Adapt your implementation plan based on findings

When to Skip This Step

• Trivial fixes: Typos, documentation, obvious bugs
• Isolated changes: Changes that don't interact with other code
• Fresh main: You just pulled main and no time has passed

Integration with Worktree Workflow

This review happens BEFORE creating your worktree:

1. Read issue (with comments)
2. Review recent main changes (YOU ARE HERE)
3. Check dependencies
4. Create worktree
5. Implement (using patterns learned from review)

Test Output: Truncate for Token Efficiency

IMPORTANT: When running tests, truncate verbose output to conserve tokens in long-running sessions.

Why Truncate?

Test output can easily exceed 10,000+ lines, consuming significant context:

• Full test suites dump every passing test
• Stack traces repeat for related failures
• Coverage reports add thousands of lines
• This wastes tokens and pollutes context for subsequent work

Truncation Strategies

Option 1: Failures + Summary Only (Recommended)

# Run tests, capture only failures and summary
pnpm test 2>&1 | grep -E "(FAIL|PASS|Error|Summary|Tests:)" | head -100

# Or use test runner's built-in options
pnpm test --reporter=dot          # Minimal output (dots for pass/fail)
pnpm test --silent                # Suppress console.log from tests
pnpm test --onlyFailures          # Re-run only failed tests

Option 2: Tail for Summary

# Get just the final summary
pnpm test 2>&1 | tail -30

Option 3: Head + Tail

# First 20 lines (test start) + last 30 lines (summary)
pnpm test 2>&1 | (head -20; echo "... [truncated] ..."; tail -30)

Option 4: Grep for Failures

# Show only failing tests and their immediate context
pnpm test 2>&1 | grep -A 5 -B 2 "FAIL\|Error"

When Full Output Is Needed

Sometimes you need full output for debugging:

• First run after major changes (to see all failures)
• Investigating intermittent failures
• Understanding test coverage gaps

In these cases, run full output but don't include it all in your response. Instead:

1. Run the full test suite
2. Analyze the output
3. Report only relevant failures in your response
4. Include actionable summary, not raw dumps

Example: Good Test Reporting

Instead of dumping 500 lines of output:

Test Results: 3 failures

1. `src/lib/state.test.ts` - "should update terminal config"
   - Expected: { name: "Builder" }
   - Received: undefined
   - Likely cause: Missing null check in updateTerminal()

2. `src/lib/worktree.test.ts` - "should create worktree"
   - Error: ENOENT: no such file or directory
   - Likely cause: Test cleanup not running

3. `src/main.test.ts` - "should initialize app"
   - Timeout after 5000ms
   - Likely cause: Async setup not awaited

Summary: 47 passed, 3 failed, 50 total

This gives you all the information needed to fix issues without wasting tokens on verbose output.

Acceptance Criteria Verification: REQUIRED Before PR Creation

CRITICAL: Before creating a PR, you MUST explicitly verify that ALL acceptance criteria from the issue are met. This prevents incomplete PRs that require manual intervention during review.

Why This Matters

During orchestration, incomplete PRs cause:

• CI failures when criteria are missed
• Manual intervention mid-workflow
• Wasted review cycles
• Sweep/Judge time spent on fixable issues

Example failure: Issue #1441 listed 4 shellcheck warnings to fix. Builder fixed 3/4, missed cli/loom-start.sh:47, requiring manual fixes after CI failed.

Step 1: Extract Acceptance Criteria

Before starting implementation, extract ALL acceptance criteria from the issue:

Look for these patterns in issue body and comments:

Pattern	Example
Checkbox items	- [ ] Fix shellcheck warning in file.sh
Numbered requirements	1. Add validation to input
"must"/"should"/"required" statements	Must handle edge case X
Explicit test conditions	Verify that Y works when Z
File-specific changes	Update config.json to include...

Create a working checklist:

## My Acceptance Criteria Checklist

From issue #123:
- [ ] Fix shellcheck warning in `scripts/build.sh:42`
- [ ] Fix shellcheck warning in `scripts/test.sh:15`
- [ ] Fix shellcheck warning in `scripts/deploy.sh:88`
- [ ] All `find ... -exec shellcheck` returns 0 warnings

Step 2: Verify Each Criterion During Implementation

As you complete each item, explicitly verify it works:

# Example: Issue says "Fix all shellcheck warnings in scripts/"
# DON'T just fix what you find - verify the COMPLETE list from the issue

# If issue lists specific files:
shellcheck scripts/build.sh    # Check file 1
shellcheck scripts/test.sh     # Check file 2
shellcheck scripts/deploy.sh   # Check file 3
# etc. - check EVERY file mentioned

# If issue says "all shellcheck warnings":
find scripts -name "*.sh" -exec shellcheck {} \; 2>&1 | grep -c "error\|warning"
# Must return 0

Step 3: Pre-PR Verification Checklist

BEFORE opening the PR (./.loom/scripts/create-pr.sh, see "Creating the PR"), complete this checklist:

## Pre-PR Verification

Issue #123 acceptance criteria:

1. [ ] Criterion A - Verified by: [describe how you checked]
2. [ ] Criterion B - Verified by: [describe how you checked]
3. [ ] Criterion C - Verified by: [describe how you checked]

Root cause verification (for process/behavior issues):
- [ ] Changes address root cause, not just surface symptom
- [ ] Fix is structural (enforcement, validation, inlining) not just documentation
- [ ] If documentation-only: justified why docs will change behavior this time

Local verification:
- [ ] The project's check command passes (see `buildGate.command` in `.loom/config.json`, or the repo's documented CI command, e.g. `pnpm check:ci`)
- [ ] Formatter + linter run on changed files (see "Format and Lint Changed Files" below) — a format-only CI failure is a guaranteed Judge rejection
- [ ] Commits are signed off if required (`commit.signoff: true` in `.loom/config.json`, or a DCO/`sign-off` requirement — `git commit --signoff`; see "DCO sign-off" above)
- [ ] Relevant tests pass
- [ ] Each criterion has explicit verification (not "I think it works")
- [ ] Ran the "defaults/ Version-Bearing-Files Gate" command block below (not just read it) — exited 0. It fails if this PR hand-edits any version-bearing file's value (`package.json`, `mcp-loom/package.json`, `Cargo.toml`, `VERSION`) — those are now bumped automatically at merge time (#7743), never by hand in a feature PR.

Run the defaults/ Version-Bearing-Files Gate locally — an actual command, not a checklist bullet to read (#6675, recurring Judge rejection: #6598, #6599, #6610, #6611, #6630, #6668 all hit this in CI because it was never run pre-PR). A single automated workflow (.github/workflows/version-bump-on-merge.yml, #7743) now owns bumping VERSION and the other version-bearing files, once, right after any merge that touched defaults/ — a feature PR must not carry its own edit to any of them. ./.loom/scripts/create-pr.sh also runs a version-bearing-file consistency check (version-check-gate.sh, #6730) before creating the PR, so running the block below by hand is defense-in-depth, not the only line of defense; still run it locally to catch a hand-edit before pushing rather than at CI time:

# Run from your worktree, AFTER your last commit, BEFORE
# ./.loom/scripts/create-pr.sh. Mirrors the CI job "PRs Must Not Hand-Edit
# Version-Bearing Files" (.github/workflows/ci.yml) — no PR exists yet at
# Builder time, so use the merge-base with origin/main as --base instead of
# a PR base sha. (`--forbid-bump` also narrows to merge-base(base, head)
# internally since #7823, so passing one here is idempotent, not redundant
# belt-and-braces you could drop: it keeps this block correct on an older
# installed copy of the script too.)
MERGE_BASE="$(git merge-base origin/main HEAD)"

# Exit 0 = no version-bearing file's VALUE changed anywhere in your diff;
# exit 1 = your PR hand-edits one of package.json/mcp-loom/package.json/
# Cargo.toml/VERSION. Do NOT "fix" a failure here by running
# ./scripts/version.sh bump patch -- that command is now exclusively the
# post-merge workflow's job; a Builder should never run it. Revert the
# edit(s) to the flagged file(s) instead.
if ! bash defaults/scripts/check-defaults-version-bump.sh --forbid-bump --base "$MERGE_BASE" --head HEAD; then
  echo "BLOCKER: this PR hand-edits a version-bearing file's value (see the diff above)." >&2
  echo "Fix: revert the change(s) to that file -- version bumps happen automatically at merge (#7743), never in a feature PR." >&2
  exit 1
fi

echo "OK: no version-bearing file's value was hand-edited."

Treat a non-zero exit above as a hard local blocker — revert your edit to the flagged file(s) and re-run the block until it prints the final OK: line before calling create-pr.sh. Do not proceed on the strength of having merely read the checklist bullet.

Step 4: Document Verification in PR Description

Include criterion verification in your PR description:

## Summary
Fix shellcheck warnings in deployment scripts.

## Acceptance Criteria Verification

| Criterion | Status | Verification |
|-----------|--------|--------------|
| Fix `scripts/build.sh:42` | ✅ | `shellcheck scripts/build.sh` returns no warnings |
| Fix `scripts/test.sh:15` | ✅ | `shellcheck scripts/test.sh` returns no warnings |
| Fix `scripts/deploy.sh:88` | ✅ | `shellcheck scripts/deploy.sh` returns no warnings |
| All warnings resolved | ✅ | `find scripts -name "*.sh" -exec shellcheck {} \;` returns 0 |

Closes #123

Common Verification Commands

Criterion Type	Verification Command
Shellcheck fixes	shellcheck <file> or find ... -exec shellcheck {} \;
TypeScript errors	pnpm tsc --noEmit
Lint issues	pnpm lint or scoped biome check <file>
Rust compilation	cargo check (see Language-Specific section below)
Rust linting	cargo clippy (see Language-Specific section below)
Rust formatting	cargo fmt --all -- --check (see Language-Specific section below)
Test passes	pnpm test -- <pattern>
File exists/content	cat <file> or grep <pattern> <file>
Config changes	Read file and verify expected content

Format and Lint Changed Files: MANDATORY Before Committing

Run the project's formatter and linter on your changed files before every commit, not just before opening the PR. A format-only CI failure (e.g. cargo fmt --check / ruff format --check / biome format --check catching an unformatted file with otherwise-correct code) is a guaranteed Judge rejection — Judge never approves with a failing required check (see "PR Creation Checklist" below), so a one-command mechanical fix costs a full Doctor -> Judge cycle (an extra dispatch, re-review, and CI wait). This happened repeatedly in production (kicad-tools PRs #4532/#4533/#4535, #4882): otherwise approve-ready PRs rejected solely on ruff format --check.

Discover the commands from repo convention — don't skip this because the language or toolchain is unfamiliar:

1. Check buildGate.command in .loom/config.json (may already run format+lint as part of the project check command).
2. Check CONTRIBUTING.md, package.json scripts, a Makefile, or the CI workflow (.github/workflows/*.yml) for the documented format/lint commands.
3. Fall back to the language's standard tool:

Language	Format	Lint
Rust	cargo fmt	cargo clippy
Python	ruff format <files> (or black <files>)	ruff check <files>
TypeScript/JavaScript	biome format --write <files> / prettier --write <files>	biome check <files> / eslint <files>
Go	gofmt -w <files>	go vet ./...
Shell	—	shellcheck <file>

Scope to your changed files (same pattern as "Handling Pre-existing Lint/Build Failures" below) so you don't pull unrelated files into your PR:

# Example: Python project
git diff --name-only origin/main -- '*.py' | xargs -r uv run ruff format
git diff --name-only origin/main -- '*.py' | xargs -r uv run ruff check

Add to your pre-PR checklist:

Local verification:
- [ ] Formatter run on changed files (no remaining diff / `--check` passes)
- [ ] Linter run on changed files (0 errors)

Language-Specific Verification

Rust Code Changes

If you modified any .rs files, run these checks before committing:

# Compile check - catches type errors, borrow issues, async Send violations
cargo check

# Lint - catches common mistakes, anti-patterns, correctness issues
cargo clippy

# Format all Rust files (applies formatting)
cargo fmt

# Verify formatting (check only, no changes - returns non-zero if unformatted)
cargo fmt --all -- --check

Why check compilation before commit (not just rely on CI)?

1. Defense in depth - Pre-commit hooks can fail silently in worktrees or with PATH issues
2. Early feedback - Catch errors immediately instead of after CI failure
3. Save a Doctor cycle - the project's check command (buildGate.command in .loom/config.json, e.g. pnpm check:ci) includes compilation; catching it early avoids a fix cycle
4. Async pitfalls - Common Rust async errors (e.g., holding MutexGuard across .await) are only caught by the compiler, not by reading code

Add to your pre-PR checklist when modifying Rust:

Local verification:
- [ ] The project's check command passes (`buildGate.command` in `.loom/config.json`, e.g. `pnpm check:ci`)
- [ ] `cargo check` returns 0 (Rust files only)
- [ ] `cargo clippy` returns 0 (Rust files only)
- [ ] `cargo fmt --all -- --check` returns 0 (Rust files only)

Red Flags: Don't Create PR Yet

STOP and verify if:

• You haven't explicitly checked each criterion from the issue
• You're unsure if a criterion is met ("it should work")
• The issue mentions files you haven't touched
• CI might fail on something you didn't test locally

Instead:

1. Go back to Step 1 and re-extract criteria
2. Verify each one explicitly
3. Only then create the PR

---

Test-First Discipline (TDD line, required in PR body)

Why: Loom's cross-session Builder → Judge cycle is the between-turn half of an evaluator-optimizer loop (CLAUDE.md § "Sweep Lifecycle"). What's missing is a checkable trace of the in-Builder half — did the test exist before the fix, not just after it. This section is the concrete signal ADR-0015 (docs/adr/0015-builder-test-first-checkpoint.md) decided on, adapted from damusix/atomic-claude's maker/checker split (issue #5849, evaluation docs/research/atomic-claude-evaluation.md).

When your diff touches executing code (anything Judge would run tests against — not a docs-only, ADR-only, or pure-config change), your PR body's ## Test Plan section MUST include one TDD: line:

TDD: yes — <test path> written first; failed for the right reason before the fix, passes after
TDD: no — <reason, e.g. "pure refactor, existing coverage in tests/foo_test.rs already exercises this path">

How to earn a TDD: yes line — practice this while implementing, not retroactively while writing the PR body:

• New behavior: write the test first, run it, confirm it fails for the right reason (not a typo/syntax error) — then implement until it passes.
• Bug fix: write a test that reproduces the bug (fails on the pre-fix code) — then fix — then confirm it passes.
• If you did this, the TDD: line's <test path> must be a real path in your diff — Judge checks it against the changed-files list (see judge.md § "Test-First (TDD) Claim Verification"). Do not write yes for a test you added after confirming the fix already worked; that is exactly the unverified self-report this checkpoint exists to catch. If you didn't actually write the test first, write TDD: no and say why (e.g. "test added after implementation for coverage, not written first") — an honest no is advisory-only; a false yes is a blocking Judge finding.
• When there is nothing to test-first (design/investigation issues like this one, docs, ADRs, .github/labels.yml, config-only changes), use TDD: no — <reason> or omit the line entirely — both are advisory, never blocking. Do not invent a placeholder test to satisfy this checkpoint.

This is advisory on absence, blocking only on contradiction — a missing line or a plausible TDD: no never blocks approval; a TDD: yes claim the diff does not corroborate does. Full decision and rationale (including why this isn't a buildGate check): ADR-0015.

---

PR Titles: Conventional Commit Style Required

CRITICAL: PR titles MUST describe the actual change. Never use generic or issue-referencing titles.

Format

<type>: <concise summary of what the code change does>

Allowed Prefixes

Prefix	When to Use
fix:	Bug fixes
feat:	New features or capabilities
refactor:	Code restructuring without behavior change
docs:	Documentation-only changes
test:	Adding or updating tests
chore:	Build, config, or tooling changes
perf:	Performance improvements

How to Derive the Title

1. Look at your diff, not the issue title — what files changed and what do the changes accomplish?
2. Pick the correct prefix based on the nature of the change (bug fix → fix:, new capability → feat:, etc.)
3. Summarize the change itself in a few words — a reader should understand the change without opening the PR
4. Keep it under 70 characters total (prefix + description)

Ask yourself: "If someone reads only this title in git log --oneline, will they understand what changed?" If the answer is no, rewrite it.

Examples

WRONG (generic body that just references the issue):

feat: implement changes for issue #2584
fix: address issue #2557
feat: implement feature from issue #123

WRONG (bare issue number):

Issue #2557

WRONG (raw issue title copied as PR title):

Builder should generate descriptive PR titles instead of generic 'Issue #N'

WRONG (double prefix — issue title's own prefix copied and another prefix prepended):

feat: bug: MCP status bar noise misclassifies builder failures as MCP failures
fix: feat: add workspace snapshot caching for daemon state

CORRECT (describes what the code change actually does):

fix: standardize timestamp format to ISO 8601 UTC across log scripts
feat: add workspace snapshot caching for daemon state
refactor: rename instant-exit to low-output terminology
docs: update troubleshooting guide for worktree cleanup
fix: prevent duplicate label transitions in sweep phase validation

Issue Title Prefix Mapping

Issue titles sometimes use non-standard prefixes (like bug:) that are not valid conventional commit types. If you're tempted to use an issue title as inspiration for your PR title, be aware that you must strip and remap the issue prefix — never copy it verbatim.