rjwalters

loom-builder-pr

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

rjwalters 72 10 Updated 1w ago
GitHub

Install

npx skillscat add rjwalters/kicad-tools/loom-builder-pr

Install via the SkillsCat registry.

SKILL.md

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

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 targeted tests for what you touched (CI runs the full suite, #11690)
  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:
- [ ] Fast-tier pre-flight passes (`loom-daemon preflight --issue N`); CI runs the suite
- [ ] 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` or a DCO requirement — `git commit --signoff`; see "DCO sign-off" above)
- [ ] Relevant tests pass
- [ ] Diff adds/moves a direct forge caller? Rebase onto the landing base; no `defaults/forge/manifest.toml` there = no inventory command. Else declare it under its `[[operation.callers]]` (`defaults/forge/README.md`; a move must drop the old path, which no command flags), then run `loom-daemon forge-inventory validate --manifest-dir defaults/forge` and `gate --manifest-dir defaults/forge --baseline defaults/forge/call-bypass-baseline.toml` (bare forms read the binary's embedded copy). Nonzero blocks review, even if pre-existing: fix, rerun; never add baseline debt or weaken the gate.
- [ ] Each criterion has explicit verification
- [ ] Ran the "defaults/ Version-Bearing-Files Gate" block below (not just read it) — exited 0 (version-bearing files bump at merge, #7743).

Run the defaults/ Version-Bearing-Files Gate locally — an actual command, not a bullet to read (#6675; #6598, #6599, #6610, #6611, #6630, #6668 hit this in CI). .github/workflows/version-bump-on-merge.yml (#7743) owns bumping VERSION and the other version-bearing files after any merge touching defaults/; a feature PR must not edit them. create-pr.sh also runs version-check-gate.sh (#6730); this is defense-in-depth before CI:

# 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 (kept for older installed copies of the script; #7823).
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. Never "fix" it with ./scripts/version.sh bump (the
# post-merge workflow's job only); revert the flagged edit(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.

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)

Pre-PR gate (pre-flight, #10476, #11690). With buildGate set, run loom-daemon preflight --issue N before create-pr.sh. It runs the fast tier (compile + smoke, never the suite) and never defers on load. Exit 1 = failed: fix the tail, commit, re-run (max preflightMaxAttempts). Exit 4 (preflight_unresolved) = stop, NO PR (claim released). Exit 5 = a hung compile timed out (claim kept): re-run; at the timeout cap, NO PR. create-pr.sh refuses (exit 7) an ungated HEAD. Then open the PR without waiting for CI.

Language-Specific Verification

Rust Code Changes

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

# Compile check
cargo check

# Lint
cargo clippy

# Format (applies)
cargo fmt

# Verify formatting (check only)
cargo fmt --all -- --check

Why check before commit? Pre-commit hooks can fail silently in worktrees, and a Doctor cycle costs far more than a local check.

When modifying Rust, tick off before the PR: pre-flight, cargo check, cargo clippy, and cargo fmt --all -- --check all return 0.

Red Flags: Don't Create PR Yet

STOP and verify if you have not checked each issue criterion, are unsure one is met ("it should work"), the issue mentions files you have not touched, or CI might fail on something untested locally. Then re-extract criteria (Step 1), verify each explicitly, and only then create the PR.


Rollout check (host-move PRs)

A PR that moves work between hosts or changes who emits a fleet signal (authority, captain, singleton jobs, gating, capability routing) needs a ## Rollout check section: the production signal (e.g. a SigNoz query) and its expected value after the fleet rolls. Others omit it. See .loom/docs/rollout-check.md.


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.

Issue Title Prefix Correct PR Prefix Notes
bug: fix: bug: is not a conventional commit type
feature: feat: Abbreviate to feat:
feat: feat: Already valid — but still rewrite the description
fix: fix: Already valid — but still rewrite the description
docs: docs: Already valid — but still rewrite the description
chore: chore: Already valid — but still rewrite the description

CRITICAL: Never prepend a new prefix to a title that already has one. feat: bug: MCP status bar... is malformed. Instead:

  1. Strip the issue title prefix entirely
  2. Map to the correct conventional commit type using the table above
  3. Rewrite the summary to describe what your code change does, not what the issue says

Remember: even if an issue title has a valid prefix, the PR title should describe your diff — not copy the issue title.

Why This Matters

  • gh pr list, git log --oneline, and release notes become readable at a glance
  • Generic titles like "implement changes for issue #N" provide zero information
  • Aligns with the existing commit style in this repository
  • Enables automated changelog generation

PR Title Checklist

Before creating a PR, verify your title:

  • Starts with a conventional commit prefix (fix:, feat:, etc.)
  • Describes what the PR does, not what issue it addresses
  • Does NOT contain generic phrases like "implement changes for", "address issue", or "implement feature from"
  • Does NOT reference an issue number in the title (issue references go in the PR body)
  • Does NOT contain double prefixes (e.g., feat: bug:, fix: feat:) — strip any prefix from the issue title before composing your own
  • Is under 70 characters
  • A reader can understand the change from the title alone without looking at the diff

Commit Messages: Same Rules as PR Titles

Commit messages follow the exact same rules as PR titles above. This repo merges with merge commits (#9105), so each commit message survives onto main verbatim — write each one as durable history.

How to Write Commit Messages

# Step 1: Review your diff BEFORE writing the commit message
git diff --stat
git diff

# Step 2: Describe what the code change does
git commit -m "fix: validate PR title format in sweep phase validation"

# NOT: git commit -m "feat: implement changes for issue #2678"
# NOT: git commit -m "fix: address issue #2557"

Commit Message Anti-Patterns

These patterns are WRONG — sweep phase validation will reject PRs with titles matching them:

Anti-Pattern Why It's Wrong
feat: implement changes for issue #N Describes the issue, not the code change
fix: address issue #N Says nothing about what was fixed
feat: implement feature from issue #N Generic — could be any feature
<copy of issue title> Issue titles describe problems; commits describe solutions

DCO sign-off

If commit.signoff is true in .loom/config.json, pass --signoff on every
commit you author (including git commit --amend) so each carries a
Signed-off-by: trailer — DCO-requiring repos gate PRs on it:

git commit --signoff -m "fix: validate PR title format in sweep phase validation"

If the knob is unset, do a one-time check before your first commit: if
CONTRIBUTING.md/a DCO file mentions Signed-off-by/DCO or a required status
check name matches dco/sign-?off, use --signoff too and note it in the PR
body. --signoff is harmless when not required. See
defaults/docs/commit-signoff.md.


Creating Pull Requests: Label and Auto-Close Requirements

CRITICAL: PRs MUST include an issue reference in the body — either a closing keyword
(Closes #N / Fixes #N / Resolves #N) for a full implementation, or a non-closing
reference (Part of #N / Contributes to #N) for a declared partial increment of a
family/epic issue (see "Multi-PR landings" below).
A closing keyword is required for:

  1. GitHub to auto-close the issue when the PR merges
  2. Sweep orchestration to detect your PR during phase validation

A partial-increment PR intentionally omits the closing keyword so the family/epic issue
stays open — it still references the issue with Part of #N so the PR is discoverable.

PR Label Rules

When creating a NEW PR:

  • Pass only loom:review-requested: create-pr.sh copies the closing issue's
    priority labels (loom:important / loom:very-important) itself (#10518); a role never invents one
  • This is the ONLY time you add labels to a PR

After PR creation:

  • NEVER remove loom:review-requested (Judge does this)
  • NEVER remove loom:pr (Judge adds this, Champion uses it)
  • NEVER add loom:pr yourself (only Judge can approve)
  • NEVER modify any labels on PRs you didn't create

Why? PR labels are review-pipeline signals (loom:review-requested -> Judge
-> loom:pr -> Champion merges). Touching them breaks the pipeline.

GitHub Auto-Close Requirements

IMPORTANT: When creating PRs, you MUST use GitHub's magic keywords to ensure issues auto-close when PRs merge.

The Problem

If you write "Issue #123" or "Fixes issue #123", GitHub will NOT auto-close the issue. This leads to:

  • Orphaned open issues that appear incomplete
  • Manual cleanup work for maintainers
  • Confusion about what's actually done

The Solution: Use Magic Keywords

ALWAYS use one of these exact formats in your PR description:

Closes #123
Fixes #123
Resolves #123

Examples

WRONG - Issue stays open after merge:

## Summary
This PR implements the feature requested in issue #123.

## Changes
- Added new functionality
- Updated tests

CORRECT - Issue auto-closes on merge:

## Summary
Implement new feature to improve user experience.

## Changes
- Added new functionality
- Updated tests

Closes #123

Why This Matters

GitHub's auto-close feature only works with specific keywords immediately followed by the issue reference:

  • Closes #X
  • Fixes #X
  • Resolves #X
  • Closing #X
  • Fixed #X
  • Resolved #X

Any other phrasing will NOT trigger auto-close.

The keyword does NOT have to start a line — GitHub scans the ENTIRE body.
close/closes/closed/fix/fixes/fixed/resolve/resolves/resolved creates a
real closing reference wherever it appears — mid-sentence, inside a numbered list, in a
"follow-up" checklist. What breaks the link is a word between the keyword and the reference
(close issue #X, Fixes issue #X), not its position in the body. This cuts both ways: it
is why Fixes issue #123 silently fails to close, and why a stray then close #123 buried
in prose silently does close — see "Multi-PR landings" below (#4569).

Multi-PR landings: every PR declares its issue (#9465)

The auto-close guidance above assumes one issue → one PR → close. Two shapes break that, and
the reference requirement applies to both
— not only to declared families:

  • a family/epic issue (loom:epic / loom:epic-phase, or a body scoping a family such as
    "~346 conformance failures across N files") landed deliberately in slices;
  • an ordinary issue that happens to land through more than one merged PR — Doctor rework that
    opened a fresh PR, a Judge-rejected-then-refixed cycle, a first slice shipped on a reused
    feature/issue-N branch. This is the majority case and had no documented requirement at all:
    19 of 329 sweep-landing PRs (2026-08-15..09-29) were invisible to GitHub's issue↔PR link, and
    one issue landed through 47 PRs.

For a PR that does NOT complete the issue, use a non-closing reference and the
machine-readable trailer:

Part of #123
Loom-Issue: rjwalters/loom#123
  • Part of #N / Contributes to #N keeps the PR discoverable without triggering auto-close, so
    the issue survives the merge, and is the form merge-pr.sh parses to fire the loom:building →
    loom:issue reset (#3667). Do not substitute Refs #N — nothing parses it, so the reset
    silently no-ops.
  • Loom-Issue: owner/repo#N is the machine-readable link (loom-daemon merge-pr-refs loom-issue-trailer-refs), greppable without GitHub's keyword parser or prose matching. The
    owner/repo slug is required (a bare Loom-Issue: #123 does not parse). Required on
    every PR whose body does not close its issue
    ; optional on a Closes #N PR, whose closing
    reference already carries the link. Only the final PR uses Closes #N.

Write both as PLAIN TEXT — never in backticks (#8796). A trailer inside an inline code span
(or a fenced block) is silently ignored — merge-pr.sh blanks code spans before matching,
deliberately (#5234), so the PR looks correct to a reviewer and to Judge while the #3667 reset
never fires and the issue is stranded at loom:building with nothing logged.

Part of #123              <- declaration: parsed, the label reset fires on merge
`Part of #123`            <- code span: NOT a declaration, reset silently skipped

Both the PR body and the commit messages must carry the same reference. GitHub harvests closing keywords from the PR body and the merge commit's subject (this repo merges with merge commits, #9105). Prefer Part of #N.

If a Part of #N remainder needs a human, add <!-- loom:remainder-hold reason="..." --> to the issue body before merge (parks it loom:operator; see label-state-machine.md).

A stray closing keyword ANYWHERE in the body defeats Part of #N (#4569)

Part of #N / Contributes to #N is not a shield. GitHub does not weigh the two references
against each other — one closing keyword adjacent to #N anywhere in the body is enough
to close the issue on merge, no matter how explicitly the rest of the body says otherwise. This
is observed, not hypothetical (a Contributes to #2 PR whose operator-handoff step read "then
close #2" closed #2 on merge, and it had to be reopened by hand).

Rules for a non-closing PR body:

  • Never write a closing keyword immediately before the tracked issue's number. Write
    then close the issue, or then close issue #2 (the intervening word breaks the link).

  • The tracked issue number should appear exactly once with a keyword: the Part of #N /
    Contributes to #N reference. Scan the whole body — including checklists, test plans, and
    operator-handoff sections — and every commit message on the branch (#4595), before pushing:

    # Must print nothing for the tracked issue N:
    grep -inE '\b(close[sd]?|fix(e[sd])?|resolve[sd]?)[[:space:]]+#N\b' <<<"$PR_BODY"
    git log --format=%B origin/main..HEAD \
      | grep -inE '\b(close[sd]?|fix(e[sd])?|resolve[sd]?)[[:space:]]+#N\b'

merge-pr.sh backstops some of this — it warns on a whole-line backticked Part of trailer
or an unparseable Loom-Issue: one, warns naming which source carries a contradicting closing
keyword, and reopens the issue if GitHub closed it anyway — but every warning reaches only
whoever runs the merge, and a reopen leaves close/reopen churn. Get it right in the body. Full rationale,
incidents, and the trailer's parser contract: .loom/docs/issue-pr-linking.md.

PR Creation Checklist

When creating a PR, verify:

  1. PR title uses conventional commit format - e.g., fix: descriptive summary (see "PR Titles" above)
  2. Acceptance criteria verified - Each criterion from issue explicitly checked (see "Acceptance Criteria Verification" above)
  3. PR description references the issue: Closes #X for a full implementation, or Part of #X for a declared partial increment (not "Issue #X" or "Addresses #X") — same reference in the commit message
  4. Issue number is correct
  5. PR has loom:review-requested label
  6. It compiles (pre-flight) and targeted tests pass — including the formatter/linter on changed files (see "Format and Lint Changed Files" above); a format-only failure is a guaranteed Judge rejection. CI runs the rest
  7. PR description includes verification table for each criterion
  8. Tests added/updated as needed
  9. Commits carry a Signed-off-by: trailer if required (commit.signoff: true in .loom/config.json, or a DCO/sign-off check — see "DCO sign-off")
  10. ## Test Plan includes a TDD: line for any diff touching executing code (see "Test-First Discipline" above) — omit only for docs/config/ADR-only changes
  11. ## Rollout check section present if the PR is a host-move PR (see "Rollout check" above)
  12. Synced with latest origin/main immediately before push — "Pre-Push Rebase: Sync with origin/main" below

Pre-Push Rebase: Sync with origin/main (#7668)

Immediately before git push + opening the PR — after the checklist above
passes, right before Lease Fencing below — sync with main's tip (a merge
unless the repo allows rebase, #10771):

BRANCH_UPDATE=$(loom-daemon branch-update strategy) || BRANCH_UPDATE=merge  # unknown = merge
git fetch origin main
if [ "$BRANCH_UPDATE" = rebase ]; then git rebase origin/main; else git merge --no-ff -m "Merge origin/main (Builder: sync before push)" origin/main; fi

Do this unconditionally, not only when your diff touches a file you know
is contested: a branch cut hours ago against a busy repo can be stale by push
time. Three consecutive PRs (example-org/tool-repo#164, #165, #166) each landed
DIRTY with CI never run because each was cut before the previous one merged
and touched the same README paragraph; each cost a Judge-rejection + Doctor
round-trip this step absorbs up front.

If the update conflicts, resolve it now, not after a review round-trip:

  1. Resolve each conflicted file, git add it, then git commit --no-edit
    (merge) or git rebase --continue (rebase; repeat until it completes).
    A branch already pushed is pushed plain in merge mode — never forced.
  2. Re-run pre-flight (a new commit voids its receipt) and the targeted tests
    — a rebase can silently change behavior even when it resolves cleanly.
  3. Version-bearing-file sync gate (#7168; moot after #7743): a clean rebase
    lands origin/main's exact version values, so it never fires unless your
    branch carries its own version-bearing edit — revert it, never version.sh bump:
    if [ -x ./.loom/scripts/version-check-gate.sh ] && ! ./.loom/scripts/version-check-gate.sh --fix-hint "before push."; then
      echo "Aborting: version-bearing files out of sync after rebase (see BLOCKER: above) -- revert, never bump." >&2
      exit 1
    fi
  4. If you cannot resolve the conflict confidently (e.g. it touches code you
    don't understand well enough to merge correctly), stop and report rather
    than force a resolution — this is the same bar as any other blocked-work
    outcome in builder.md.

This only moves an update that would otherwise happen reactively (Doctor,
after loom:changes-requested) to before the PR opens; no gate is weakened.

Lease Fencing: Confirm You Still Own the Claim (Epic #6165 Phase 3, #6309)

Immediately before git push + opening the PR — the one irreversible,
externally-visible action of this whole Builder run — run the sweep-side
fencing check:

./.loom/scripts/sweep-lease-fence.sh check "$N" --branch "$(git rev-parse --abbrev-ref HEAD)"
FENCE_RC=$?
if [[ "$FENCE_RC" -eq 3 ]]; then
  echo "Lease fence: EXPIRED — MY OWN claim's lease record is stale on the forge's own clock (my renewal loop died). Aborting before push/PR-open; NOT pushing, NOT opening a PR." >&2
  # Stop here for issue $N. Do not push, do not create a PR, do not touch
  # the loom:building label or contest any peer's claim — report this issue
  # as not-contributed-this-run. (#6783: an expired lease of a DIFFERENT,
  # abandoned host is not an abort; it folds into FENCE_RC == 0 below.)
elif [[ "$FENCE_RC" -eq 4 ]]; then
  echo "Lease fence: SUPERSEDED — a different host's lease is now the freshest for issue $N. Aborting before push/PR-open; NOT pushing, NOT opening a PR." >&2
  # Same stop-here handling as the EXPIRED branch above.
elif [[ "$FENCE_RC" -ne 0 ]]; then
  # 5: yours -> adopt via create-pr.sh, else stand down; NEVER a suffix
  # branch. 6: loom-daemon too old/missing (#10027).
  echo "Lease fence: exit $FENCE_RC (5 BRANCH_COLLISION / 6 BRANCH_PROBE_UNAVAILABLE) — NOT pushing." >&2
else
  # FENCE_RC == 0 (fresh & own host, OR no lease evidence to fence against —
  # fail-open, see the script's own header doc — OR an EXPIRED lease owned
  # by a DIFFERENT, abandoned host, Issue #6783) -> proceed exactly as
  # before.
  git push -u origin "$(git rev-parse --abbrev-ref HEAD)"
  # ... then open the PR (see "Creating the PR" below) ...
fi

This is fencing, not a lock — it bounds the cost of a race to one wasted
build, it does not prevent the race (Phase 1, defaults/docs/lease-renewal.md,
keeps the lease alive for the whole run; Phase 2,
loom-daemon/src/claim_reconciliation.rs, is the daemon's symmetric check
before reclaiming a peer's claim). Reads the freshest
<!-- loom:lease host=… sweep=… --> comment on issue $N and confirms BOTH:
the comment is still fresh (now - updated_at <= LEASE_TTL_MINUTES, default
15, override with --ttl-minutes or LOOM_LEASE_TTL_MINUTES) and its
host= still names this host (--host, defaulting to this host's own
identity — same LOOM_HOST_ID > $HOSTNAME > hostname precedence
sweep_registry::host_identity() uses). It aborts (exit 3 = expired-and-
own-host, 4 = superseded, 5 = branch collision)
before doing anything externally-visible: no push, no PR. It
never touches a peer's claim — the loom:building label is
left as-is. Since #6320 an in-session run
(manual /loom:sweep, GH Actions cron, --no-daemon) publishes its own
lease at pre-flight (sweep-lease-publish.sh, sweep.md Step 1b), so this
check is now meaningful on both dispatch paths rather than only the
daemon-dispatched one. When there genuinely is no lease comment — a run
predating those writers, or one whose lease write failed — the check fails
open (exit 0), same as every other lease-record reader in this repo treats
"no lease" as "no evidence", never as "not fresh".

Issue #6783: an EXPIRED lease owned by a DIFFERENT host now PASSes, not
aborts.
Exit 3 (EXPIRED) fires only when the freshest lease is stale AND
belongs to this sweep's own host — a sweep whose own renewal loop died
should not trust its claim. An expired lease belonging to a different host
is an abandoned record, not a live peer, and is now treated as no fencing
evidence (fail-open, exit 0) rather than a permanent abort — a dead sweep's
never-renewed, never-yielded lease would otherwise fence out every successor
dispatch forever (observed on issue #6694 / PR #6773). Exit 4 (SUPERSEDED
— a fresh lease from a live peer) is unaffected: that case still means a
live peer genuinely holds the claim.

Creating the PR

Open the PR with ./.loom/scripts/create-pr.sh, never a bare gh pr create (#6074).
The flags are a subset of gh pr create's, so the call below reads the same — but four
things a bare gh pr create cannot do are load-bearing here:

  • It adopts an already-open PR for your branch (prints that PR's URL, exits 0, creates
    nothing). So if a previous attempt on this issue already pushed and opened a PR, you
    converge on it instead of failing or duplicating.
  • It re-verifies the target issue's freshness immediately before opening the PR
    (#6277).
    When the body carries a closing keyword (Closes/Fixes/Resolves #N), the
    script re-checks whether #N was already closed by a different, already-merged PR — the
    two-workers-race scenario that otherwise isn't caught until Judge review. On a detected
    supersede it refuses to open a duplicate PR (names the superseding PR, exits non-zero,
    does not push further, does not delete the branch). Part of #N / Contributes to #N
    partial-increment references are exempt by construction — see "Multi-PR landings" below.
  • It enforces a 1:1 issue-to-PR review-gate (#9453 phase 5) — the race adopt-first
    above can't catch: two builders on different branches for the same issue. Re-runs
    forge check-open-pr on your body's referenced issue (closing OR Part of); an open PR
    on a different head branch refuses (exit 6, names it). If refused: stand down —
    close your branch, or escalate to Judge/a human if yours genuinely supersedes it.
  • It survives the GitHub App permission window. A cached App installation token can
    hold Contents:write while Pull-requests:write has not propagated into it yet, so
    your git push succeeds and the very next gh pr create returns 403 Resource not accessible by integration. The script force-mints a fresh installation token (bypassing
    the ~1h cache) and then falls back to a personal token, instead of dying. Before this,
    that 403 killed the sweep with no PR and the next dispatch rebuilt the identical
    work
    , orphaning a feature/issue-N branch each pass.

If it still fails, do NOT rebuild and do NOT delete the branch — your commits are
pushed. Re-run the script (idempotent), or report the branch name for a hand-opened PR.
On an install predating this script (no .loom/scripts/create-pr.sh — use plain
gh pr create), a 403 … not accessible by integration after a successful push is
still a transient credential window, never a signal to redo the work.

# CORRECT way to create PR
# Title MUST use conventional commit format: "fix:", "feat:", "refactor:", etc.
# create-pr.sh adds the closing issue's priority labels itself (#10518)
./.loom/scripts/create-pr.sh --title "fix: descriptive summary of the change" --label "loom:review-requested" --body "$(cat <<'EOF'
## Summary
Brief description of what this PR does and why.

## Changes
- Change 1
- Change 2
- Change 3

## Acceptance Criteria Verification

| Criterion | Status | Verification |
|-----------|--------|--------------|
| Criterion 1 from issue | ✅ | How you verified it |
| Criterion 2 from issue | ✅ | How you verified it |

## Test Plan
How you verified the changes work.

TDD: yes — tests/foo_test.rs written first, failed for the right reason, now passes
(or: TDD: no — <reason>, e.g. docs-only / config-only / refactor with existing coverage)

Closes #123
EOF
)"

Remember:

  • Put "Closes #123" (or "Part of #123" for a partial increment) on its own line in the PR description, and use the same reference in the commit message
  • Include the acceptance criteria verification table showing each criterion was checked

Handling Pre-existing Lint/Build Failures

IMPORTANT: When the target codebase has pre-existing issues, don't let them block your focused work.

The Problem

Target codebases may have pre-existing failures that are unrelated to your issue:

  • Deprecated linter configurations
  • CSS classes not defined in newer framework versions
  • A11y warnings in unrelated files
  • Type errors in untouched code

These are NOT your responsibility to fix when implementing a specific feature.

Strategy: Focus on Your Changes

Step 1: Identify what you changed

# Get list of files you modified
git diff --name-only origin/main

Step 2: Run scoped checks on your changes only

# Lint only changed files (Biome)
git diff --name-only origin/main -- '*.ts' '*.tsx' '*.js' '*.jsx' | xargs npx biome check

# Lint only changed files (ESLint)
git diff --name-only origin/main -- '*.ts' '*.tsx' '*.js' '*.jsx' | xargs npx eslint

# Type-check affected files (TypeScript will check dependencies automatically)
npx tsc --noEmit

Step 3: If full checks fail on pre-existing issues

  1. Document in PR description what pre-existing issues exist
  2. Don't fix unrelated issues - this expands scope
  3. Optionally create a follow-up issue for the tech debt

PR Documentation Template

When pre-existing issues exist, add this to your PR:

## Pre-existing Issues (Not Addressed)

The following issues exist in the codebase but are outside the scope of this PR:

- [ ] `biome.json` uses deprecated v1 schema (needs migration to v2)
- [ ] `DashboardPage.tsx` has a11y warnings (unrelated to this feature)
- [ ] Tailwind 4 CSS class `border-border` not defined

These should be addressed in separate PRs to maintain focused scope.

Decision Tree

Lint/Build fails
|
Is the failure in YOUR changed files?
|-- YES -> Fix it (your responsibility)
+-- NO -> Pre-existing issue
         |-- Document in PR description
         |-- Continue with your implementation
         +-- Optionally create follow-up issue

Creating Follow-up Issues (Optional)

If you want to track pre-existing issues for future cleanup:

File issues with ./.loom/scripts/create-issue.sh, never a bare gh issue create (#5047).
gh issue create fails outright when GraphQL quota is exhausted, while the independent REST
pool sits ~99% unused. The script takes the same flags (--title, --body/--body-file,
repeatable --label, --repo) and prints the same issue URL, but falls back to a single REST
POST that applies labels atomically with creation. Recipe and rationale:
.loom/docs/gh-issue-create-rest-fallback.md (or forge_gh_create_issue_rl_safe in
lib/forge-helpers.sh if scripting). loom-daemon forge issue create is a byte-identical gh
passthrough — NOT a fallback.

./.loom/scripts/create-issue.sh --title "Tech debt: Migrate biome.json to v2 schema" --body "$(cat <<'EOF'
## Problem

`biome.json` uses deprecated v1 schema which causes warnings on every lint run.

## Discovery

Found while working on #969. Not fixed there to maintain focused scope.

## Solution

Run `npx @biomejs/biome migrate` to update configuration.

## Impact

- Removes deprecation warnings
- Enables new linter rules
- Estimated: 30 minutes
EOF
)"

What NOT to Do

Don't block your PR on unrelated failures

# WRONG: Spending hours fixing biome config for an unrelated feature

Don't include unrelated fixes in your PR

# WRONG: PR titled "Add login button" that also migrates linter config

Don't ignore failures in YOUR code

# WRONG: Introducing new lint errors in the code you wrote

Why This Matters

Scope creep kills productivity:

  • Issue #921 spent 2+ hours on biome migration (unrelated to feature)
  • Issue #922 fixed a11y warnings in files not touched by the feature
  • Each detour adds risk and delays the actual goal

Focused PRs are better:

  • Easier to review (one concern per PR)
  • Faster to merge (no surprises)
  • Clearer git history (each commit has one purpose)
  • Lower risk (smaller blast radius)

Raising Concerns

After completing your assigned work, you can suggest improvements by creating unlabeled issues. The Architect will triage them and the user decides priority.

Example of post-work suggestion:

./.loom/scripts/create-issue.sh --title "Refactor terminal state management to use reducer pattern" --body "$(cat <<'EOF'
## Problem

While implementing #42, I noticed that terminal state updates are scattered across multiple files with inconsistent patterns.

## Current Code

- State mutations in: `src/lib/state.ts`, `src/main.ts`, `src/lib/terminal-manager.ts`
- No single source of truth for state transitions
- Hard to debug state-related issues

## Proposed Refactor

- Single `terminalReducer` function handling all state transitions
- Action types for each state change
- Easier to test and debug

## Impact

- **Files**: ~5 files affected
- **Complexity**: Medium (2-3 days)
- **Risk**: Low if we add comprehensive tests first

Discovered while working on #42
EOF
)"

Note: For out-of-scope work discovered during implementation, use the Scope Management section in builder-complexity.md - pause immediately and create an issue before continuing.