Agents open a pull request by describing it, rather than running gh themselves (#1567) - #1612
Conversation
…ther than running gh themselves #1567 found that a plan agent's PR body ended "…then comment on and close #1164", which GitHub read as a closing keyword: the plan merged, the issue closed, and the next tickets sync deleted the ticket and its fresh plan. The first fix directions were both wrong. Linting closing keywords out of every PR body breaks the correct case — 8 of the last 40 PRs legitimately close their issue that way. And the body that caused it was the agent's own: it ran `gh pr create` itself, outside the handoff path, where the framework never sees the text. brillout's answer on the issue: "As always, I'd frame it as a capability… We avoid limiting agents — we *enable* agents instead." So there is no prohibition here. An `open-pr` block is a new non-blocking signal, alongside set-session-name and ready-for-merge: the agent writes the description, the framework opens the PR. The agent gets to say what the change turned out to be instead of the PR body reciting the opening prompt; the framework keeps the parts that have to stay consistent — the title from the session name, the ticket's `(fix #N)`, and recording the number on the agent. An agent that prefers to open its own still can. The closing-keyword bug is then fixed where it belongs: a plan agent's description is defused before it is published, because a plan agent's PR lands the plan, not the work. That is the same rule that already keeps `(fix #N)` off a plan agent's title (#1327) — this is its other half. Defusing wraps the issue reference in backticks: the sentence a human reads is unchanged, and GitHub stops acting on it. Suite green: 1502 framework tests, 776 dashboard, typecheck clean.
|
🤖 agent comment SPEC walkthrough — five files, each as intro + Flows, then the diff, then what it means against the rest of the file. 1. Meaning: this is the one place that knows GitHub's issue-closing grammar. It is a new file rather than a branch inside the handoff because the grammar is GitHub's, not the handoff's — and because the second emitter that will want it (#1500's error capability, or any later PR-writing path) should not have to reach into the handoff to find it. The idempotence flow is what makes it safe for a caller that cannot be sure whether the text has already been through it. 2. Diff: -… its chosen session name, ready-for-merge — is a tagged block parsed out of the turn's final message.
+… its chosen session name, ready-for-merge, a pull-request description — is a tagged block parsed out of the turn's final message.
-- … the non-blocking signals (markdown views, session name, ready-for-merge).
+- … the non-blocking signals (markdown views, session name, ready-for-merge, a pull-request description).
-- Signal emission is deduped across a span of turns: ready-for-merge fires once, and a session name re-emits only on a real rename.
+- Signal emission is deduped across a span of turns: ready-for-merge fires once, and a session name or a pull-request description re-emits only on a real change.
+- An agent describes its pull request in a block instead of opening one itself; the description is the body the end-of-agent handoff publishes, and the last one the agent wrote is the one used.And one added rationale: +- The pull-request description is offered as a capability rather than the agent being told not to open its own: an agent that describes the work and lets the framework publish it keeps the title, the ticket's issue reference and the recorded pull-request number, and an agent that prefers its own way is not stopped.Meaning: the file's first Flows line is that the protocols pin how to emit, not when — and the new block joins that list unchanged in kind. It is a non-blocking signal, so it belongs beside session-name and ready-for-merge rather than beside the gate: the agent does not stop for it. The dedupe line already covered "re-emits only on a real rename" for the session name; a description behaves the same way for the same reason, so the two share one sentence rather than getting one each. The rationale is the only new argument in the file, and it is the issue's whole answer: enabling rather than forbidding. 3. Diff: the third bullet is new. Meaning: it sits directly above the arming bullet because it describes the PR's content, where the next lines describe whether it is opened at all and what is recorded about it. The "otherwise" half is the honest statement of today's behavior — the intent is what the framework knows on its own, and it says what was asked at the start rather than what the change became. 4. -… and how to emit the non-blocking signals (show a document, name the session, ready-for-merge);
+… and how to emit the non-blocking signals (show a document, name the session, ready-for-merge, describe the pull request the framework will open);Meaning: the sentence lists the agent's side of the conversation. The wording is deliberately "the pull request the framework will open" rather than "open a pull request" — the agent describes, the framework publishes, and the spec should not imply the block itself is the publishing act. 5. -… ready-for-merge detection, and the single continuation wording shared by every path.
+… ready-for-merge detection, the pull-request description an agent writes for the framework to publish (markdown kept whole, the last block winning, an empty one ignored), and the single continuation wording shared by every path.Meaning: the three parenthesised cases are the three that could silently corrupt a PR body — markdown mangled, a stale first draft published over a revision, or an empty block blanking the body that would otherwise have been written. Plus
+- The PR is described by the agent that did the work, when it wrote a description for itOne spec is deliberately not updated here: |
…run skipped it Live evidence: a Sonnet run on a scratch project got the protocol verbatim in its system prompt (12,686 chars, `open-pr` present), emitted set-session-name and ready-for-merge from that same file, and did not emit open-pr. The other two signals name a concrete moment — the setSessionName() and setReadyForMerge() calls. This one said "when your work is ready to be published", which is not a moment the agent can recognize, and the agent had already emitted ready-for-merge by then. It now hangs off exactly that signal, and says what is lost without it.
|
🤖 agent comment Dogfooded on the real dashboard. The first run failed — that's why there's a second commit. Run 1: the agent ignored the blockA local Sonnet agent, a plain task ("Add a README.md…"), no mention of the capability anywhere in the prompt. Its system prompt did contain the Both of those come from the same protocol file as Why it skipped it. The other two blocks say exactly when to emit them — "when you call setSessionName()", "when you call setReadyForMerge()". Mine said "when your work is ready to be published". That isn't a moment an agent can point at. And by the time it got there, it had already sent The fix ( Run 2: it worksRebuilt, restarted the daemon, new task. The agent emitted the block on its own, and the framework recorded this as the PR description:
Here is the same PR's body as it would have read without the block — it is the prompt, repeated back:
One says what the change is. The other says what was asked for. That difference is the whole feature. The dashboard shows it as a new row, What is still untestedThe last step: handing that description to (Also confirmed in passing: #1604's project-error banner is live and correct on the same page — "The data branch is not syncing · since 11m ago — the repository has no remote".) |
|
🤖 agent comment Ran the capability across model tiers, since signal emission is known to vary by tier (#1334's evidence table: Haiku never emits
Opus produced this, unprompted:
That last line is the argument for the capability in one sentence: it volunteers the negative, which is what a reviewer wants and which no framework-composed body could ever produce, since the framework only knows the prompt it sent. Haiku confirms the prediction, and confirms it is a tier property rather than anything about this block. Its system prompt was 12,801 chars and contained the
So the tier that already fails Worth noting the dashboard already warns about exactly this when Haiku is selected: "Haiku consistently skips the session-finish protocol, so a publishing run ends as an unmerged draft PR and needs hand-holding." This capability lands on the correct side of a line the product had already drawn. |
|
Sounds good.
The issue number won't be clickable and won't cross-reference anymore. How about replacing |
Three specs this branch touches were rewritten by #1613. The additions are kept, re-said in the new voice: - prompts/SPEC.md — the pull-request description joins the non-blocking signal list inside Rom's restructured sentence, not the old one. - agent-handoff.SPEC.md — the description flow leads with what the reader gets ("describes the work in the agent's own words") rather than with what the framework does, and the capability gains a User Story beside the others. - turn-gate.SPEC.md merged cleanly.
Rom on #1612: wrapping the reference in backticks stopped the closing, but it also stopped the reference being a link, and stopped the issue's own timeline recording that a pull request had mentioned it. The ticket lost the backlink that says someone is discussing it. GitHub only obeys a closing keyword when the reference follows it directly, so two words in between are enough: `close #1164` becomes `close the ticket #1164`. The sentence still reads as the agent wrote it, the reference is untouched — clickable, and still cross-referenced onto the issue. Idempotence now falls out of the rewrite instead of needing a guard: after the first pass the keyword is followed by the filler, not by a reference. A reference already inside backticks is still skipped — it is a code sample, GitHub does not act on it, and the filler would corrupt it. Also lands the agent-handoff.test.SPEC.md clause deferred while #1611 was rewriting that sentence.
|
Done — the filler, as you suggested. You were right about the cost, and it was two things rather than one: the reference stopped being a link, and the issue stopped recording that a pull request had mentioned it — so the ticket lost the backlink telling it someone is discussing it. The filler keeps both and removes only the closing. Idempotence got simpler instead of harder: after the rewrite the keyword is followed by "the", not by a reference, so a second pass finds nothing to change. The explicit guard is gone. A reference already inside backticks is still skipped — that one is a code sample, GitHub doesn't act on it, and the filler would corrupt it. Also in this push: merged main, which meant re-saying this branch's three spec additions in #1613's user-story voice rather than the voice they were written in; plus the Suite 1504 + 776, typecheck clean. |
🤖 agent PR
Fix #1567, built to @brillout's answer on the issue:
So there is no prohibition in this PR.
The problem. A plan agent's PR body ended "…then comment on and close #1164." GitHub read that as a closing keyword: the plan merged, #1164 closed even though only the plan had landed, and the next tickets sync saw a closed issue and deleted the ticket and its fresh plan. Both had to be restored by hand.
Why the obvious fixes don't work. Linting closing keywords out of PR bodies breaks the correct case — 8 of the last 40 PRs legitimately close their issue that way (#1604, #1603, #1595). And the body that caused the incident was the agent's own: it ran
gh pr createitself, outside the handoff, where the framework never sees the text at all.What the user sees. Pull requests the framework opens are described by the agent that did the work, instead of repeating the prompt it was given. Today's body is the opening intent plus a session line; now it is what the change turned out to be.
The capability. A new non-blocking block, alongside
set-session-nameandready-for-merge:The agent writes the description; the framework opens the PR and keeps the parts that have to stay consistent — the title from the session name, the ticket's
(fix #N)(#1334), and recording the number on the agent so every surface reads the same one. The block is re-emittable, last one wins, and an agent that prefers to open its own PR still can — it then owns all of the above, which is the trade the issue was really about.Where the original bug is actually fixed. A plan agent's description is defused before publishing, because a plan agent's PR lands the plan, not the work. That rule already exists for the title —
maybeAutoHandoffdeliberately withholds(fix #N)from a plan agent (#1327, "its PR lands the plan, not the work, so the merge must not close the issue") — and this is its other half. Defusing wraps the reference in backticks:close `#1164reads identically to a human and links nowhere.How it is wired, following
ready-for-mergeexactly:parsePullRequestDescriptioninturn-gate.ts→ apull-request-descriptionevent throughcreateTurnSignalEmitter→ the agent journal'sprDescription()→maybeAutoHandoff, which defuses it for a plan agent and hands it toagentPrBody. One new module,src/closing-keywords.ts, owns GitHub's closing grammar.Tests. 13 new: the defuser (the exact #1164 sentence, every keyword form and case, the cross-repo
owner/repo#123form, a keyword-less mention left alone, a word merely ending in a keyword, and idempotence), the block parser (markdown kept whole, last block wins, empty block ignored), and the PR body itself (the description replaces the intent; without one the intent still shows). Suite green: 1502 framework, 776 dashboard, typecheck clean.Not in this PR.
src/dashboard/agent-handoff.test.SPEC.mdshould gain a clause for the two new body tests, but Fix everything the SPEC.md migration's grounding pass flagged #1611 is rewriting that exact sentence right now — it is a one-line follow-up once that lands, rather than a conflict today.