"This document covers PR creation, test output handling, and quality requirements for the Builder role. For the core builder workflow, see `builder.md`."
Install
npx skillscat add rjwalters/loom/loom-builder-pr Install via the SkillsCat registry.
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
- Rollout check (host-move PRs)
- 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 --statStep 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 typesStep 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 findingsWhen 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:
- Read issue (with comments)
- Review recent main changes (YOU ARE HERE)
- Check dependencies
- Create worktree
- 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 testsOption 2: Tail for Summary
# Get just the final summary
pnpm test 2>&1 | tail -30Option 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:
- Run the targeted tests for what you touched (CI runs the full suite, #11690)
- Analyze the output
- Report only relevant failures in your response
- 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 totalThis 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 warningsStep 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 0Step 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 #123Common 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:
- Check
buildGate.commandin.loom/config.json(may already run
format+lint as part of the project check command). - Check
CONTRIBUTING.md,package.jsonscripts, aMakefile, or the CI
workflow (.github/workflows/*.yml) for the documented format/lint
commands. - 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 checkAdd 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 -- --checkWhy 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 (seejudge.md§ "Test-First (TDD) Claim Verification"). Do not writeyesfor
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, writeTDD: noand say why (e.g. "test
added after implementation for coverage, not written first") — an honestnois advisory-only; a falseyesis 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), useTDD: 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
- Look at your diff, not the issue title — what files changed and what do the changes accomplish?
- Pick the correct prefix based on the nature of the change (bug fix →
fix:, new capability →feat:, etc.) - Summarize the change itself in a few words — a reader should understand the change without opening the PR
- 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 #123WRONG (bare issue number):
Issue #2557WRONG (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 stateCORRECT (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 validationIssue 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:
- Strip the issue title prefix entirely
- Map to the correct conventional commit type using the table above
- 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 aSigned-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: ifCONTRIBUTING.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. Seedefaults/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:
- GitHub to auto-close the issue when the PR merges
- 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 withPart of #Nso the PR is discoverable.
PR Label Rules
When creating a NEW PR:
- Pass only
loom:review-requested:create-pr.shcopies 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:pryourself (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 #123Examples
WRONG - Issue stays open after merge:
## Summary
This PR implements the feature requested in issue #123.
## Changes
- Added new functionality
- Updated testsCORRECT - Issue auto-closes on merge:
## Summary
Implement new feature to improve user experience.
## Changes
- Added new functionality
- Updated tests
Closes #123Why This Matters
GitHub's auto-close feature only works with specific keywords immediately followed by the issue reference:
Closes #XFixes #XResolves #XClosing #XFixed #XResolved #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/resolvedcreates 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 whyFixes issue #123silently fails to close, and why a straythen close #123buried
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 reusedfeature/issue-Nbranch. 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#123Part of #N/Contributes to #Nkeeps the PR discoverable without triggering auto-close, so
the issue survives the merge, and is the form merge-pr.sh parses to fire theloom:building→loom:issuereset (#3667). Do not substituteRefs #N— nothing parses it, so the reset
silently no-ops.Loom-Issue: owner/repo#Nis the machine-readable link (loom-daemon merge-pr-refs loom-issue-trailer-refs), greppable without GitHub's keyword parser or prose matching. Theowner/reposlug is required (a bareLoom-Issue: #123does not parse). Required on
every PR whose body does not close its issue; optional on aCloses #NPR, whose closing
reference already carries the link. Only the final PR usesCloses #N.
This trailer is a coordination link Loom reads back; keep writing it.
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 skippedBoth 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's hands or a ruling (not merely a cross-repo or time-gated wait), add <!-- loom:remainder-hold reason="..." --> to the issue body before merge (parks it loom:operator; see label-state-machine.md). A cross-repo or time-gated remainder gets a park record instead: see the "Cross-repo or time-gated leftover" row in label-state-machine.md "Routing to a human".
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, orthen 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 #Nreference. 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:
- PR title uses conventional commit format - e.g.,
fix: descriptive summary(see "PR Titles" above) - Acceptance criteria verified - Each criterion from issue explicitly checked (see "Acceptance Criteria Verification" above)
- PR description references the issue:
Closes #Xfor a full implementation, orPart of #Xfor a declared partial increment (not "Issue #X" or "Addresses #X") — same reference in the commit message - Issue number is correct
- PR has
loom:review-requestedlabel - 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
- PR description includes verification table for each criterion
- Tests added/updated as needed
- Commits carry a
Signed-off-by:trailer if required (commit.signoff: truein.loom/config.json, or a DCO/sign-offcheck — see "DCO sign-off") ## Test Planincludes aTDD:line for any diff touching executing code (see "Test-First Discipline" above) — omit only for docs/config/ADR-only changes## Rollout checksection present if the PR is a host-move PR (see "Rollout check" above)- Synced with latest
origin/mainimmediately before push — "Pre-Push Rebase: Sync withorigin/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; fiDo 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 landedDIRTY 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:
- Resolve each conflicted file,
git addit, thengit commit --no-edit
(merge) orgit rebase --continue(rebase; repeat until it completes).
A branch already pushed is pushed plain in merge mode — never forced. - 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. - Version-bearing-file sync gate (#7168; moot after #7743): a clean rebase
landsorigin/main's exact version values, so it never fires unless your
branch carries its own version-bearing edit — revert it, neverversion.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 - 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 inbuilder.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) ...
fiThis 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 itshost= still names this host (--host, defaulting to this host's own
identity — same LOOM_HOST_ID > $HOSTNAME > hostname precedencesweep_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#Nwas 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-runsforge check-open-pron your body's referenced issue (closing ORPart of); an open PR
on a different head branch refuses (exit6, 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
holdContents:writewhilePull-requests:writehas not propagated into it yet, so
yourgit pushsucceeds and the very nextgh pr createreturns403 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 afeature/issue-Nbranch 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 plaingh 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/mainStep 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 --noEmitStep 3: If full checks fail on pre-existing issues
- Document in PR description what pre-existing issues exist
- Don't fix unrelated issues - this expands scope
- 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 issueCreating 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 baregh issue create(#5047).gh issue createfails 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(orforge_gh_create_issue_rl_safeinlib/forge-helpers.shif scripting).loom-daemon forge issue createis a byte-identicalgh
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 featureDon't include unrelated fixes in your PR
# WRONG: PR titled "Add login button" that also migrates linter configDon't ignore failures in YOUR code
# WRONG: Introducing new lint errors in the code you wroteWhy 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.