feat(templates): detect PR review feedback in Ralph's work-check cycle - #4
Conversation
Ralph reads reviewDecision, which reports CHANGES_REQUESTED and APPROVED. A COMMENTED review leaves reviewDecision null, and so does an inline review thread — so a reviewer could ask for a change on a specific line and Ralph would report the PR as clean. New "PR Review Feedback Detection" category keyed on reviewThreads.isResolved, plus COMMENTED reviews and PR conversation comments, across every repo in commentWatch.pullRequestRepos (one GraphQL request, aliased). Sorts ahead of approved PRs so Ralph cannot merge past outstanding feedback. Review threads need no seen= field: isResolved is already the high-water mark, so a second one would only compete with it. seen= stays on COMMENTED review bodies and PR conversation comments, which have no resolution state. Bot filtering is asymmetric, and measured: review bots post a top-level review or PR comment on effectively every PR but open an inline thread on roughly one PR in sixty. Inline threads keep every author; top-level surfaces drop author.__typename == "Bot". On a live six-repo scan that filter is the difference between 12 reported items and 0, all 12 of them bot boilerplate. Agents reply to review threads but do not resolve them. Resolving is the natural ack, but an agent resolving a thread it only partly addressed drops that feedback silently, which is the failure this mechanism exists to prevent; leaving it open only nags. The queue-cleanliness argument is answered instead by an "awaiting confirmation" state — a thread whose newest comment is a signed agent reply is demoted rather than re-routed — so the reviewer keeps the resolve button. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🟠 Impact Analysis — PR #4Risk tier: 🟠 HIGH 📊 Summary
🎯 Risk Factors
📦 Modules Affectedci-workflows (1 file)
crew-cli (3 files)
crew-sdk (3 files)
root (4 files)
templates (3 files)
This report is generated automatically for every PR. See #733 for details. |
🛫 PR Readiness Check
PR Scope: 🔧 Infrastructure
|
| Status | Check | Details |
|---|---|---|
| ❌ | Single commit | 2 commits — consider squashing before review |
| ✅ | Not in draft | Ready for review |
| ❌ | Branch up to date | dev is 1 commit(s) ahead — rebase recommended |
| ❌ | Copilot review | No Copilot review yet — it may still be processing |
| ✅ | Changeset present | Changeset file found |
| ✅ | Scope clean | No .crew/ or docs/proposals/ files |
| ✅ | No merge conflicts | Merge status unknown — GitHub is still computing |
| ✅ | Copilot threads resolved | No Copilot review threads |
| ❌ | CI passing | 1 check(s) failing: Policy Gates |
Files Changed (14 files, +1053 −26)
| File | +/− |
|---|---|
.changeset/pr-review-feedback-detection.md |
+14 −0 |
.crew-templates/crew.agent.md |
+23 −2 |
.crew-templates/issue-lifecycle.md |
+47 −3 |
.crew-templates/ralph-reference.md |
+184 −1 |
.github/agents/crew.agent.md |
+23 −2 |
packages/crew-cli/templates/crew.agent.md.template |
+23 −2 |
packages/crew-cli/templates/issue-lifecycle.md |
+47 −3 |
packages/crew-cli/templates/ralph-reference.md |
+184 −1 |
packages/crew-sdk/templates/crew.agent.md.template |
+23 −2 |
packages/crew-sdk/templates/issue-lifecycle.md |
+47 −3 |
packages/crew-sdk/templates/ralph-reference.md |
+184 −1 |
templates/crew.agent.md.template |
+23 −2 |
templates/issue-lifecycle.md |
+47 −3 |
templates/ralph-reference.md |
+184 −1 |
Total: +1053 −26
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
…comments Advancing commentWatch.since to merge time when the adoption PR sits is a guard against unmarked agent comments landing in the gap. Applied blindly it does the opposite of its job. Found while doing exactly that on a live estate: the gap held four comments, all genuine operator replies and zero agent comments — because the crew was already signing by then. Three were already suppressed by seen=; the fourth was an unanswered instruction that raising the floor would have silenced permanently. Enumerate the window first. Advance the floor only over unmarked agent comments, never over an unanswered human one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Estate-side companion is up: david-driscoll/home-operations#627 (config + operative local spec). Parent issue: david-driscoll/vault#105. Also pushed a second commit here, found while validating bradygaster#627: don't advance an adoption floor over an unanswered human comment. Applying the "reset |
Follow-on to #3. That PR made human comments on issues detectable. This one closes the
other half of the same gap: review feedback on pull requests.
Parent issue: david-driscoll/vault#105 · Estate-side companion: david-driscoll/home-operations#627
The gap
Ralph's PR scan reads
reviewDecision, which reportsCHANGES_REQUESTEDandAPPROVED.But a
COMMENTEDreview leavesreviewDecision: null, and so does an inline reviewthread. A reviewer can ask for a change on a specific line and Ralph reports the PR as
clean — so requested changes sit unread on work that is otherwise ready to merge.
What this does not reuse, and why
The
seen=marker machinery from #3 exists because GitHub tracks nothing about whether acomment was answered. Review threads carry
isResolved, which is exactly that. So theyneed no marker-based high-water mark, no
commentWatch.sincecutoff, and have noretroactive adoption problem — a thread resolved before any of this existed is already
resolved, and one left open is genuinely still open. Adding
seen=there would create asecond high-water mark that can disagree with GitHub's own.
seen=still applies to the surfaces with no resolution state: top-levelCOMMENTEDreview bodies and PR conversation comments.
isResolvedCOMMENTEDreview bodyseen=on an agent replyseen=on an agent replyThe bot filter is asymmetric, and that part is measured
This is the correction that fell out of validating against live data. Keying the category on
reviews(states:[COMMENTED])as originally sketched makes it fire on every PR forever:quality/CI bots post a top-level review or PR comment unconditionally. Inline threads are
the opposite — rare and specific.
Sampled across the four code repos: a review bot had posted a top-level
COMMENTEDreviewor PR comment on every PR sampled, but had opened an inline thread on one PR in sixty.
So:
carry a native ack. A bot pointing at a real line is real feedback.
author.__typename == "Bot". Otherwise thecategory is permanently lit and becomes noise the operator learns to ignore.
On a live scan of six repos / eight open PRs, that filter is the difference between 12
reported items and 0 — all 12 bot boilerplate. Filtering on
__typenamerather than alogin allowlist needs no per-installation maintenance.
Agents do not resolve threads
The design decision that most affects whether this is trustworthy. Both directions have a
real argument, so compare the failure modes:
view the reviewer uses. Fails silently, and nothing surfaces it later.
noisily, and self-corrects.
For a mechanism whose whole purpose is that feedback stops sitting unread, a silent-drop
failure defeats the thing being built. A noisy one merely annoys.
That asymmetry is only decisive because the queue-cleanliness argument has another answer:
a thread whose newest comment is a signed
<!-- crew:agent= -->reply is demoted toawaiting confirmationrather than re-reported as new work. The queue stays clean, thereviewer keeps the resolve button, and a further reviewer reply returns the thread to the
queue automatically. This is the one place the marker earns its keep on the PR side — it is
what distinguishes "an agent has responded here" from "nobody has".
Explicit operator instruction to resolve remains an exception.
Config
Repos are configurable rather than hardcoded — review feedback spans the repos the crew
opens PRs in, not the single issue tracker:
One GraphQL request covers all of them via aliases, matching #3's one-request-per-cycle
principle. Absent/empty
pullRequestReposis a no-op, so this is inert for existing crewsuntil an operator opts in.
Verification
Read-only throughout — no comments posted, no threads resolved on any real PR.
open PR; all 11 PR comments are bot-authored and suppressed).
vault#40, read-only): fires with.mise.toml:39and the gist.one, a resolved twin of the same thread, which is correctly suppressed:
unresolvedisResolved: trueawaiting-confirmunresolved(reply is stale)COMMENTEDreviewCOMMENTEDreview, unsignedreview-commentedisOutdatedunresolved, flagged outdatedisOutdatedis reported, never filtered on: it means the diff hunk moved, not that thepoint was addressed.
Only
states:OPENPRs are scanned — unresolved threads survive a merge, so dropping thestate filter would resurrect feedback on shipped work.
Files
.crew-templates/{ralph-reference,issue-lifecycle,crew.agent}.md, mirrored to the threetemplate targets and
.github/agents/vianode scripts/sync-templates.mjs --sync.Changeset included. Same 13-file footprint as #3.