[PR #67] [MERGED] feat(groom): builder authors the PR body per the comfy-pr convention (BE-4346) #76

New issue

Closed

opened 2026-10-26 22:35:59 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:35:59 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/67
Author: @mattmillerai
Created: 7/24/2026
Status: ✅ Merged
Merged: 7/24/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-4346-groom-builder-pr-body

📝 Commits (2)

9c7ae2b feat(groom): builder authors the PR body per the comfy-pr convention (BE-4346)
6fa063b fix(groom): harden builder PR-body assembly against crash, overflow & markup injection (BE-4346)

📊 Changes

4 files changed (+299 additions, -17 deletions)

View changed files

📝 .github/groom/builder.md (+9 -1)
📝 .github/groom/ledger.py (+119 -0)
📝 .github/groom/tests/test_ledger.py (+113 -0)
📝 .github/workflows/groom.yml (+58 -16)

📄 Description

ELI-5

When the groom sweep auto-builds a fix, it opens a pull request. Until now the PR description was machine-assembled — a loud banner followed by the reviewer-bot's raw "here's the problem" paragraph, with the text hard-wrapped so GitHub rendered it as a wall of mid-sentence line breaks. It didn't follow the team's PR convention (a plain-language ## ELI-5 first, then a structured what/why, one line per paragraph). This change hands the PR-writing job to the agent that actually made the change — it now writes a proper ELI-5-first, structured description — while the workflow keeps prepending the "review-only, never auto-merged" banner and appending the ledger marker.

What changed

.github/groom/builder.md — the builder brief now instructs the agent to write the human-facing PR body to a new {{PR_BODY_OUT}} file (when it patches): ## ELI-5 first, then ## What changed / ## Why, with the "never hard-wrap — one line per paragraph, let GitHub soft-wrap" rule embedded directly (the builder runs claude -p --bare --setting-sources "" and cannot invoke the comfy-pr skill, so the convention is spelled out in the brief). On a bail it writes no body.
.github/groom/ledger.py — new builder_pr_body(banner, eli5_body, verifier_rationale, signature) helper assembles the final body: banner first, the builder's ELI-5 body, the verifier rationale demoted to a secondary <details> section, and the signature_marker LAST. Falls back to the original banner + ## Verifier rationale + marker template when the body is empty or doesn't lead with an ELI-5 heading.
.github/workflows/groom.yml — adds the PR_BODY_OUT env var and the builder's Edit grant + prompt substitution for it (mirroring the proven BUILDER_OUT write path); the pre-publish secret scan now covers the PR body alongside the patch and summary (fail-closed bail if the key appears); the capture step hands a size-capped, scanned body to build_pr via the build artifact; build_pr calls builder_pr_body instead of assembling the string inline.
.github/groom/tests/test_ledger.py — BuilderPrBodyTest covers the happy path (banner first / marker last / rationale in <details>), the empty-body and non-ELI-5 fallbacks, ELI-5 heading variants, a whitespace-only body, and the spoofed-marker injection guard.

Why

Groom builder PRs are read by humans, so they should follow the same PR convention as hand-written ones (evidence: comfy-cloud-mcp-server #837 opened with the raw rationale and no ELI-5). The builder agent knows exactly what it changed, so it's the right author. The banner and ledger marker stay load-bearing (the banner sets review expectations; the ledger keys dedup on the marker), so they remain workflow-controlled — the marker is appended LAST so an embedded/spoofed marker in the model body can't shadow it (extract_signature reads the last marker). Behavior is preserved on every non-happy path: the fallback template is byte-identical to today's output.

Acceptance criteria

PR opens with ## ELI-5 first, structured what/why, verifier rationale as a secondary <details> section (enforced: build_pr falls back to the plain template unless the body leads with an ELI-5 heading).
Prose not hard-wrapped — one line per paragraph/bullet (embedded rule in builder.md).
Banner first, signature_marker last (both in builder_pr_body; unit-tested).
Builder bail / empty / oversized body → fallback to the existing banner + rationale template; never an empty-body PR.
Pre-publish secret scan covers the builder-authored body before gh pr create (extended the capture-step scan; build_pr holds no model key, so scanning must and does happen in the credential-free build job before the artifact is uploaded).
.github/groom/tests covers the new body path incl. the fallback (54 tests pass).

Judgment calls

Separate PR_BODY_OUT file rather than a pr_body JSON field. The ticket offered either; a markdown file mirrors the finder/verifier *_OUT pattern and avoids the model having to emit a large multiline markdown blob as an escaped JSON string. It uses the identical Edit(//<path>) grant that the shipped BUILDER_OUT write relies on.
The verifier rationale is assembled by build_pr (deterministic), not by the builder. build_pr already has the verifier text in scope; keeping it out of the model's hands means the rationale is never rewritten and its own upstream secret-scan still stands. The builder only authors the ELI-5 + what/why.
Body cap of 20000 bytes — well under GitHub's 65536-char PR-body limit, leaving headroom for the wrapped banner/rationale/marker; oversized bodies are dropped (→ fallback), never truncated.
Negative-claim falsification: N/A — this change adds a capability (builder-authored bodies) and its fallbacks produce a more complete PR body; it denies no user-facing capability, so there is no dead-end to falsify.

Out of scope (per ticket): the groom issue body, groom-security findings (filed as issues), and the finder/verifier briefs.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/67 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/24/2026 **Status:** ✅ Merged **Merged:** 7/24/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4346-groom-builder-pr-body` --- ### 📝 Commits (2) - [`9c7ae2b`](https://github.com/Comfy-Org/github-workflows/commit/9c7ae2bce570c42e1a461253d23ca8277fee45a1) feat(groom): builder authors the PR body per the comfy-pr convention (BE-4346) - [`6fa063b`](https://github.com/Comfy-Org/github-workflows/commit/6fa063b00d5c3f93302b553d92ba9b217a888f29) fix(groom): harden builder PR-body assembly against crash, overflow & markup injection (BE-4346) ### 📊 Changes **4 files changed** (+299 additions, -17 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/builder.md` (+9 -1) 📝 `.github/groom/ledger.py` (+119 -0) 📝 `.github/groom/tests/test_ledger.py` (+113 -0) 📝 `.github/workflows/groom.yml` (+58 -16) </details> ### 📄 Description ## ELI-5 When the groom sweep auto-builds a fix, it opens a pull request. Until now the PR description was machine-assembled — a loud banner followed by the reviewer-bot's raw "here's the problem" paragraph, with the text hard-wrapped so GitHub rendered it as a wall of mid-sentence line breaks. It didn't follow the team's PR convention (a plain-language `## ELI-5` first, then a structured what/why, one line per paragraph). This change hands the PR-writing job to the agent that actually made the change — it now writes a proper ELI-5-first, structured description — while the workflow keeps prepending the "review-only, never auto-merged" banner and appending the ledger marker. ## What changed - `.github/groom/builder.md` — the builder brief now instructs the agent to write the human-facing PR body to a new `{{PR_BODY_OUT}}` file (when it patches): `## ELI-5` first, then `## What changed` / `## Why`, with the "never hard-wrap — one line per paragraph, let GitHub soft-wrap" rule embedded directly (the builder runs `claude -p --bare --setting-sources ""` and cannot invoke the comfy-pr skill, so the convention is spelled out in the brief). On a bail it writes no body. - `.github/groom/ledger.py` — new `builder_pr_body(banner, eli5_body, verifier_rationale, signature)` helper assembles the final body: banner first, the builder's ELI-5 body, the verifier rationale demoted to a secondary `<details>` section, and the `signature_marker` LAST. Falls back to the original banner + `## Verifier rationale` + marker template when the body is empty or doesn't lead with an ELI-5 heading. - `.github/workflows/groom.yml` — adds the `PR_BODY_OUT` env var and the builder's `Edit` grant + prompt substitution for it (mirroring the proven `BUILDER_OUT` write path); the pre-publish secret scan now covers the PR body alongside the patch and summary (fail-closed bail if the key appears); the capture step hands a size-capped, scanned body to `build_pr` via the build artifact; `build_pr` calls `builder_pr_body` instead of assembling the string inline. - `.github/groom/tests/test_ledger.py` — `BuilderPrBodyTest` covers the happy path (banner first / marker last / rationale in `<details>`), the empty-body and non-ELI-5 fallbacks, ELI-5 heading variants, a whitespace-only body, and the spoofed-marker injection guard. ## Why Groom builder PRs are read by humans, so they should follow the same PR convention as hand-written ones (evidence: comfy-cloud-mcp-server #837 opened with the raw rationale and no ELI-5). The builder agent knows exactly what it changed, so it's the right author. The banner and ledger marker stay load-bearing (the banner sets review expectations; the ledger keys dedup on the marker), so they remain workflow-controlled — the marker is appended LAST so an embedded/spoofed marker in the model body can't shadow it (`extract_signature` reads the last marker). Behavior is preserved on every non-happy path: the fallback template is byte-identical to today's output. ## Acceptance criteria - [x] PR opens with `## ELI-5` first, structured what/why, verifier rationale as a secondary `<details>` section (enforced: `build_pr` falls back to the plain template unless the body leads with an ELI-5 heading). - [x] Prose not hard-wrapped — one line per paragraph/bullet (embedded rule in `builder.md`). - [x] Banner first, `signature_marker` last (both in `builder_pr_body`; unit-tested). - [x] Builder bail / empty / oversized body → fallback to the existing banner + rationale template; never an empty-body PR. - [x] Pre-publish secret scan covers the builder-authored body before `gh pr create` (extended the capture-step scan; `build_pr` holds no model key, so scanning must and does happen in the credential-free build job before the artifact is uploaded). - [x] `.github/groom/tests` covers the new body path incl. the fallback (54 tests pass). ## Judgment calls - **Separate `PR_BODY_OUT` file rather than a `pr_body` JSON field.** The ticket offered either; a markdown file mirrors the finder/verifier `*_OUT` pattern and avoids the model having to emit a large multiline markdown blob as an escaped JSON string. It uses the identical `Edit(//<path>)` grant that the shipped `BUILDER_OUT` write relies on. - **The verifier rationale is assembled by `build_pr` (deterministic), not by the builder.** `build_pr` already has the verifier text in scope; keeping it out of the model's hands means the rationale is never rewritten and its own upstream secret-scan still stands. The builder only authors the ELI-5 + what/why. - **Body cap of 20000 bytes** — well under GitHub's 65536-char PR-body limit, leaving headroom for the wrapped banner/rationale/marker; oversized bodies are dropped (→ fallback), never truncated. - **Negative-claim falsification:** N/A — this change *adds* a capability (builder-authored bodies) and its fallbacks produce a *more* complete PR body; it denies no user-facing capability, so there is no dead-end to falsify. Out of scope (per ticket): the groom *issue* body, groom-security findings (filed as issues), and the finder/verifier briefs. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:35:59 +00:00