Repository navigation
Avoid duplicate PR sous-chef nudges and reduce finisher costs - #65411
Conversation
Prefilter meaningful PR state before model execution, skip stale/dependency/active-agent work, and use sanitizer-safe fingerprints. Prioritize concrete blockers and direct cleanup over generic nudges, branch churn, or formatting. Correct finisher review snapshots and reuse validation on unchanged heads. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — found one concrete runtime bug in the new deterministic prefilter script; the rest of the refactor (shell logic → scripts/pr-sous-chef.mjs + pr-sous-chef.test.mjs, 30 passing tests) is well-structured and a clear improvement over the inline-shell approach.
📋 Key Themes & Highlights
Key Themes
- Untested code path has a bug:
main()'sGITHUB_STEP_SUMMARYbranch references a free variableskippedthat doesn't exist (should beoutput.skipped), which will throwReferenceErrorwhenever this script runs in real CI with a step summary enabled. This logic lives inline inmain()and isn't covered by the otherwise-thoroughpr-sous-chef.test.mjssuite, since those tests only exercise the exportedclassify/buildQueue/activeAgentfunctions.
Positive Highlights
- ✅ Moving deduplication/eligibility logic from inline YAML shell into a real, unit-testable Node module (
scripts/pr-sous-chef.mjs) is a strong/codebase-designwin — much easier to reason about and extend than the prior shell-heavy implementation. - ✅ 30 regression tests cover a good spread of edge cases: fingerprint stability across comment reordering/marker stripping, bot/human activity distinction, staleness, active-agent detection across status values, and dependency-bot exclusion.
- ✅
pr-finisherSKILL.md updates clearly document the GraphQLreviewThreadsworkaround and the rationale for reusing prior validation by SHA — good traceability for future readers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 36.5 AIC · ⌖ 14.4 AIC · ⊞ 10K
Comment /matt to run again
| } | ||
| if (process.env.GITHUB_STEP_SUMMARY) { | ||
| const counts = new Map(); | ||
| for (const item of skipped) counts.set(item.skip_reason, (counts.get(item.skip_reason) ?? 0) + 1); |
There was a problem hiding this comment.
[/tdd] ReferenceError: skipped is not defined — this references a free variable that does not exist in main(); it should be output.skipped. This path only runs when GITHUB_STEP_SUMMARY is set (i.e., in real CI), so it will throw at runtime and is not covered by any existing unit test.
💡 Fix
if (process.env.GITHUB_STEP_SUMMARY) {
const counts = new Map();
for (const item of output.skipped) counts.set(item.skip_reason, (counts.get(item.skip_reason) ?? 0) + 1);Also consider adding a unit test (or extracting this summary-building logic into an exported, testable function) so regressions like this are caught before make test — this logic currently has zero test coverage because it lives inline in main(), which is only exercised as a CLI entrypoint.
@copilot please address this.
There was a problem hiding this comment.
Fixed the summary crash by using the queue output and extracted the summary formatter with regression coverage. Commit: 8b964c2.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Request changes
The duplicate-suppression rewrite is headed in the right direction, but the legacy fallback is still broad enough to hide new actionable blockers after an old markerless sous-chef comment. The prefilter also still performs a repo-wide active-run sweep up front, which weakens the cost-saving goal of this change.
Blocking themes
unchanged_legacycan suppress freshBEHIND/CONFLICTINGstates or changed failing checks instead of producing a new targeted nudge.- The new prefilter still does five paginated Actions queries before it knows which PRs matter, so it is not actually lightweight on busy repositories.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 59.3 AIC · ⌖ 7.47 AIC · ⊞ 20.2K
Comment /review to run again
| return { skip_reason: "unchanged" }; | ||
| } | ||
| if (latest && !statePattern.test(latest.body) && activity <= epoch(latest.created_at)) { | ||
| return { skip_reason: "unchanged_legacy" }; |
There was a problem hiding this comment.
This comment did not include an actionable concern, so no code change was warranted. Commit 8b964c2 addresses the substantive prefilter feedback.
| ? [gh(["pr", "view", checkNumber, "--repo", repo, "--json", prFields])] | ||
| : gh(["pr", "list", "--repo", repo, "--state", "open", "--search", "is:pr is:open -is:draft -author:app/dependabot -author:app/renovate -label:broccoli sort:updated-desc", "--limit", "200", "--json", prFields]); | ||
| if (!Array.isArray(candidates)) throw new Error("PR queue must be an array"); | ||
| const runs = candidates.length ? fetchActiveRuns(repo) : []; |
There was a problem hiding this comment.
main() still fan-outs five repository-wide workflow-run queries before it knows whether any PR is eligible, so the new "cheap prefilter" can spend most of its time scanning Actions history instead of reducing cost.
💡 Why this is risky
fetchActiveRuns() paginates queued, in_progress, pending, requested, and waiting runs for the entire repository, and main() does that on every scheduled run whenever the candidate list is non-empty. On a busy repo, that is a lot of extra API traffic and latency before you even inspect the first PR, which undercuts the stated goal of reducing repeated PR-sous-chef cost and makes rate-limit failures more likely.
Please defer the run lookup until after the cheap per-PR filters, or narrow it to the branches / PR numbers you are actually considering in this pass. At minimum, the single-PR slash-command path should not need a repo-wide active-run sweep.
There was a problem hiding this comment.
Active-run lookup now occurs only after dependency/staleness filters pass, and explicit single-PR checks filter runs by the PR branch. Commit: 8b964c2.
| return { skip_reason: "unchanged" }; | ||
| } | ||
| if (latest && !statePattern.test(latest.body) && activity <= epoch(latest.created_at)) { | ||
| return { skip_reason: "unchanged_legacy" }; |
There was a problem hiding this comment.
unchanged_legacy still suppresses follow-up nudges after the actionable blocker changes, so a PR with an old markerless sous-chef comment can stay muted forever once it becomes BEHIND/CONFLICTING or picks up different failing checks.
💡 Why this blocks merge
This fallback only checks activity <= latest.created_at, but activity is derived from commits, trusted review feedback, and human comments. It does not move when the same SHA becomes BEHIND, when the merge state flips to CONFLICTING, or when failed checks change because of a rerun. For any PR that still has a legacy markerless nudge, those state changes will keep hitting unchanged_legacy, which means the workflow never emits the new targeted nudge even though the reason to wake Copilot changed.
A safer fallback is to apply unchanged_legacy only when the newly computed actionable state is still effectively empty/no-op, instead of using it as a blanket suppression path for every later state transition.
There was a problem hiding this comment.
Legacy nudges now suppress only an otherwise no-op state; new branch conflicts, behind states, and failed checks can proceed. Added regression coverage. Commit: 8b964c2.
|
There was a problem hiding this comment.
🧪 Test Quality Sentinel Report
✅ Test Quality Score: 96/100 — Excellent
Analyzed 30 test(s): 30 design, 0 implementation, 0 violation(s).
Note: Pre-fetch scoping only matched*.test.cjs/*.test.js; this PR's new test file uses the.mjsextension with Node's built-innode:testrunner (scripts/pr-sous-chef.test.mjs), so it was analyzed manually from the PR diff instead of the prefetch artifacts.
📊 Metrics (30 tests)
| Metric | Value |
|---|---|
| Analyzed | 30 (Go: 0, JS: 30) |
| ✅ Design | 30 (100%) |
| 0 (0%) | |
| Edge/error coverage | 27 (90%) |
| Duplicate clusters | 0 |
| Inflation | No (test 319 lines : prod 257 lines ≈ 1.24:1) |
| 🚨 Violations | 0 |
| Test | File | Classification | Issues |
|---|---|---|---|
unchanged conflicts never get a repeat poke, even after cooldown |
pr-sous-chef.test.mjs | design_test / high_value | none |
visible fingerprint survives HTML marker removal and arbitrary comment order |
pr-sous-chef.test.mjs | design_test / high_value | none |
new HEAD and changed blockers can advance again |
pr-sous-chef.test.mjs | design_test / high_value | none |
bot replies and updatedAt churn are not progress |
pr-sous-chef.test.mjs | design_test / high_value | none |
legacy provenance without a hidden marker suppresses unchanged PRs |
pr-sous-chef.test.mjs | design_test / high_value | none |
latest legacy nudge is selected by timestamp, not REST array order |
pr-sous-chef.test.mjs | design_test / high_value | none |
dependency bots are ignored using all known login formats |
pr-sous-chef.test.mjs | design_test / high_value | none |
bot comments cannot revive a completely stale PR |
pr-sous-chef.test.mjs | design_test / high_value | none |
fresh human feedback revives an older PR |
pr-sous-chef.test.mjs | design_test / high_value | none |
an active coding agent blocks even an old run on a previous SHA |
pr-sous-chef.test.mjs | design_test / high_value | none |
active agents match PR number even without a branch match |
pr-sous-chef.test.mjs | design_test / high_value | none |
long-running agent checks never age out |
pr-sous-chef.test.mjs | design_test / high_value | none |
short pending CI gates, WAITING CI remains directly approvable |
pr-sous-chef.test.mjs | design_test / high_value | none |
maintainer Copilot requests also enforce startup cooldown |
pr-sous-chef.test.mjs | design_test / high_value | none |
DIRTY or BLOCKED alone is not an implementation blocker |
pr-sous-chef.test.mjs | design_test / high_value | none |
check reruns with identical failures do not change the fingerprint |
pr-sous-chef.test.mjs | design_test / high_value | none |
author replies alone do not re-poke an unresolved thread |
pr-sous-chef.test.mjs | design_test / high_value | none |
truncated review data fails closed |
pr-sous-chef.test.mjs | design_test / high_value | none |
resolved feedback permits direct dismissal of stale automation reviews |
pr-sous-chef.test.mjs | design_test / high_value | none |
GraphQL bot logins without REST suffixes permit review cleanup |
pr-sous-chef.test.mjs | design_test / high_value | none |
external discussion does not change the trusted-state fingerprint |
pr-sous-chef.test.mjs | design_test / high_value | none |
informational sous-chef acknowledgements made with a maintainer token are not progress |
pr-sous-chef.test.mjs | design_test / high_value | none |
external replies cannot change a trusted review fingerprint |
pr-sous-chef.test.mjs | design_test / high_value | none |
out-of-scope unresolved feedback does not wake an implementation agent or dismiss reviews |
pr-sous-chef.test.mjs | design_test / high_value | none |
active-run lookup paginates every nonterminal status |
pr-sous-chef.test.mjs | design_test / high_value | uses a plain injected fake for the gh call boundary (external I/O), not a mocking library — acceptable |
deduplication finds a nudge beyond the first hundred comments |
pr-sous-chef.test.mjs | design_test / high_value | none |
queue selects exactly four PRs with deterministic blocker priority |
pr-sous-chef.test.mjs | design_test / high_value | none |
dependency bots and clearly stale queue entries require no detail reads |
pr-sous-chef.test.mjs | design_test / high_value | none |
invalid metadata and failed reads are errors, not an empty success queue |
pr-sous-chef.test.mjs | design_test / high_value | none |
eligible context excludes raw successful checks and comment bodies |
pr-sous-chef.test.mjs | design_test / high_value | none |
Verdict
✅ Passed. 0% implementation tests (threshold: 30%). All 30 tests in
scripts/pr-sous-chef.test.mjsexercise exported public functions (classify,buildQueue,activeAgent,fetchActiveRuns) against observable return values (skip reasons, fingerprints, queue contents, thrown errors) rather than internal call counts. Edge/error paths (stale PRs, truncated data, invalid timestamps, bot noise, pagination boundaries) are well covered. No mock libraries, no missing build tags (N/A for JS), and the test file closely tracks the newscripts/pr-sous-chef.mjsproduction module (1.24:1 line ratio, no inflation). Other changed files (SKILL.md, workflow.md/.lock.yml,Makefile) are non-test and out of scope.
Verdict Summary
✅ Test Quality Sentinel: 96/100. 0% implementation tests (threshold: 30%).
🧪 Test quality analysis by Test Quality Sentinel · copilot · sonnet50 · 70.5 AIC · ⌖ 18.3 AIC · ⊞ 11.1K
Comment /review to run again
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The prefilter currently crashes while writing its step summary and misses some active reviewer workflows.
Review effort: Balanced
Findings: 1
Open (4)
What changed in this PR
Adds deterministic PR sous-chef filtering to prevent duplicate agent work and reduce finisher costs.
Changes:
- Adds fingerprint-based eligibility, pagination, prioritization, and active-agent checks.
- Streamlines the sous-chef workflow and generated configuration.
- Updates pr-finisher review collection and validation guidance.
| File | Description |
|---|---|
scripts/pr-sous-chef.mjs |
Implements queue filtering and deduplication. |
scripts/pr-sous-chef.test.mjs |
Adds regression coverage. |
Makefile |
Runs the new tests. |
.github/workflows/pr-sous-chef.md |
Redesigns workflow behavior. |
.github/workflows/pr-sous-chef.lock.yml |
Regenerates the compiled workflow. |
.github/workflows/agentic_commands.yml |
Updates command metadata. |
.github/skills/pr-finisher/SKILL.md |
Fixes review retrieval and validation guidance. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| if (process.env.GITHUB_STEP_SUMMARY) { | ||
| const counts = new Map(); | ||
| for (const item of skipped) counts.set(item.skip_reason, (counts.get(item.skip_reason) ?? 0) + 1); |
There was a problem hiding this comment.
Fixed the undefined summary variable and added a formatter regression test. Commit: 8b964c2.
| const marker = "<!-- gh-aw-pr-sous-chef-nudge -->"; | ||
| const statePattern = /Sous-chef state: ([a-f0-9]{64})/; | ||
| const activeStatuses = new Set(["queued", "in_progress", "pending", "requested", "waiting"]); | ||
| const agentPattern = /copilot|coding.agent|claude|codex|gemini|opencode|(^|[/ -])agent([/ -]|$)|^Q$/i; |
There was a problem hiding this comment.
Added reviewer workflow names to active-run detection and regression cases for both PR Code Quality Reviewer and Ponytail Reviewer. Commit: 8b964c2.
There was a problem hiding this comment.
Reviewed with Impeccable harden + audit modes (new deterministic script replacing inline bash — classified as bug_fix/refactor_cleanup).
One blocking issue found: scripts/pr-sous-chef.mjs's main() references an undefined skipped variable when writing the step summary (should be output.skipped), which throws a ReferenceError on every real Actions run and crashes the prefilter job before it can write pr-sous-chef-candidates-compact.json or the job outputs. See inline comment for details and fix.
Everything else reviewed (prefilter/classify logic, workflow YAML restructuring, SKILL.md doc updates, and the 30 new regression tests) looks solid and well-tested.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 56.6 AIC · ⌖ 13 AIC · ⊞ 8.1K
| } | ||
| if (process.env.GITHUB_STEP_SUMMARY) { | ||
| const counts = new Map(); | ||
| for (const item of skipped) counts.set(item.skip_reason, (counts.get(item.skip_reason) ?? 0) + 1); |
There was a problem hiding this comment.
skipped is never defined in main()'s scope — only output.skipped exists (as returned by buildQueue). This ReferenceError: skipped is not defined will throw on every real GitHub Actions run, since GITHUB_STEP_SUMMARY is always set, crashing the prefilter job before the queue/output files are written. None of the 30 tests in pr-sous-chef.test.mjs exercise this main() path, so the regression wasn't caught.
Fix: iterate output.skipped instead of the undefined skipped.
for (const item of output.skipped) counts.set(item.skip_reason, (counts.get(item.skip_reason) ?? 0) + 1);@copilot please address this.
There was a problem hiding this comment.
Fixed the undefined summary variable by iterating the queue result and added formatter regression coverage. Commit: 8b964c2.
|
@copilot run the PR finisher skill. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Why
PR sous-chef repeatedly woke Copilot for unchanged or already-addressed feedback. A sample of 30 recent runs consumed 364.95 AI credits and 5.76M tokens across six model executions; one model run only created a report issue. Finisher replies on #65165 confirmed repeat passes on feedback that was already resolved.
The existing prefilter read the oldest ten issue comments because that endpoint does not support its sort parameters. Its hidden nudge marker was also stripped from posted comments by safe-output sanitization.
Approach
gh pr view --json reviewThreadsquery with a paginated GraphQL snapshot. Batch validation and reuse prior results only when their recorded SHA matches the current head; retain broader coverage when affected code or prior CI failures require it.Slash-command acknowledgements remain mandatory but do not bypass eligibility. Workflow runs are serialized, writes remain safe-output-only, and the CJS/CGO/CWI approval allowlist is preserved. The large lock-file diff is generated; centralized slash-command metadata is regenerated as well.
Validation
node --test scripts/pr-sous-chef.test.mjs: 30 passing regression tests covering deduplication, pagination, stale/bot/active-agent gates, trusted feedback, queue limits, compact output, and explicit read failures.make recompile: all 304 workflows compiled../gh-aw compile pr-sous-chef --strict --schedule-seed github/gh-aw.make agent-report-progress: passed using installed Bash 5; generated workflows are in sync.No live workflow runs were triggered. Post-change production cost savings are not yet measured.