Repository navigation
Make PR Sous Chef poll faster and start agents only for actionable work - #65650
Conversation
Keep per-run credit limits and deterministic eligibility gates unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Skip low API budgets without model startup, filter metadata exclusions before detail reads, and require a new nudge, guarded CI approval, or resolved-review cleanup. Preserve deduplication while allowing independent cleanup and approvals without repeat nudges. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🔍 Design Decision Gate 🏗️ is checking for design decision records on this pull request... |
🧪 Test Quality Sentinel Report
📊 Metrics (66 tests)
Test Coverage HighlightsStrong design invariants verified:
All 66 tests use strict assertions; 45+ include edge cases:
No code patterns flagged:
Inflation Analysis
Ratio justification: The PR introduces 8 major behavioral changes (5-min polling vs 15-min, disabled daily cap, approval-only mode, new queue filtering, fingerprint migration, etc.), each requiring multiple scenarios. Average 3.2 lines per test is lean for behavioral test coverage. Verdict
Pass criteria:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 74/100 Acceptable. Implementation ratio: 0% (threshold ≤30%). All 66 tests are design-driven with strong edge-case coverage and no violations. Inflation ratio 2.78:1 is justified by 8 major behavior changes in PR scope (faster polling, approval workflows, rate limit gates, etc.).
🧪 Test quality analysis by Test Quality Sentinel · copilot · haiku45 · 25.4 AIC · ⌖ 6.69 AIC · ⊞ 8.2K
Comment /review to run again
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Verdict
I did not find a blocking issue in the changed lines.
Highlights
The prefilter tightening is internally consistent with the workflow prompt: low-budget and empty-queue runs now avoid model startup, approval-only and review-cleanup work stays actionable without re-nudging, and the new tests cover the main edge cases around stale/not-idle filtering, approval-run validation, and duplicate-nudge suppression.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 46.5 AIC · ⌖ 7.5 AIC · ⊞ 19.4K
Comment /review to run again
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The pre-activation search can prevent valid /souschef acknowledgements for opted-out PRs.
Review effort: Balanced
Findings: 1
What changed in this PR
Updates PR Sous Chef to poll faster while using deterministic checks to avoid unnecessary agent runs.
Changes:
- Adds early metadata, budget, active-run, and actionable-work filtering.
- Introduces guarded CI approval candidates and independent review cleanup.
- Updates workflow scheduling, prompts, tests, and generated configuration.
| File | Description |
|---|---|
scripts/pr-sous-chef.mjs |
Implements prefiltering and approval validation. |
scripts/pr-sous-chef.test.mjs |
Adds regression coverage for new behavior. |
.github/workflows/pr-sous-chef.md |
Updates schedule, activation gate, and guidance. |
.github/workflows/pr-sous-chef.lock.yml |
Regenerates the compiled workflow. |
| name: souschef | ||
| events: [pull_request_comment] | ||
| skip-if-no-match: "is:pr is:open -is:draft -author:app/dependabot -author:app/renovate" | ||
| skip-if-no-match: "is:pr is:open -is:draft -author:app/dependabot -author:app/renovate -label:broccoli" |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design (via pr-triage recommendation: refactor_cleanup, skills /codebase-design, /tdd) — no blocking issues, two readability suggestions.
📋 Key Themes & Highlights
Key Themes
- Ternary readability:
priorityscoring inclassify()repeats the!nudgeSkipguard three times inline; a small named helper would make the ranking rule easier to audit. - Gate composition seam:
metadataSkip/checkSkipare invoked with different compositions across four call sites (classify,fetchCandidate,buildQueue,fetchQueue). This works today and is well pinned by tests, but the "which gate applies before which fetch" rule lives implicitly in each call site rather than in one place.
Positive Highlights
- ✅ Splitting cheap metadata gates (
metadataSkip) from expensive-fetch gates (checkSkip) is a solid efficiency win — it avoids GraphQL/REST detail calls for draft/stale/opted-out PRs before the costly reads. - ✅
approvableRuns()validates workflow path, head SHA/branch, PR membership, and run state together — a defensively written seam for an operation (auto-approving CI) that must fail closed. - ✅ Excellent regression coverage (66 tests, including targeted-refresh, metadata-recheck, and API-call-count assertions) for a change that touches scheduling cadence, credit guardrails, and the core classify/nudge logic simultaneously.
- ✅
/tddcheck: tests exercise edge cases directly tied to the new behavior (empty reviewer comments, approval candidate validation across 12 negative cases, human-only blockers not suppressing genuine work).
No regression or correctness risk found in the reviewed diff — recommending COMMENT since the two findings are maintainability suggestions, not blockers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 106.6 AIC · ⌖ 14.6 AIC · ⊞ 10K
Comment /matt to run again
| nudge_needed: !nudgeSkip, | ||
| zero_diff_stalled: zeroDiff, | ||
| priority: branchAction === "CONFLICTING" ? 0 : failedChecks.length ? 1 : feedback.length ? 2 : zeroDiff ? 3 : 4, | ||
| priority: branchAction === "CONFLICTING" ? 0 : !nudgeSkip && failedChecks.some(check => check.conclusion !== "ACTION_REQUIRED") ? 1 : !nudgeSkip && feedback.length ? 2 : !nudgeSkip && zeroDiff ? 3 : 4, |
There was a problem hiding this comment.
[/codebase-design] The priority ternary repeats the !nudgeSkip && guard three times (failedChecks... ? 1 : !nudgeSkip && feedback.length ? 2 : !nudgeSkip && zeroDiff ? 3 : 4), forcing the reader to re-derive that nudgeSkip gates everything except the conflict case.
💡 Suggested refactor
function priorityFor(branchAction, nudgeSkip, failedChecks, feedback, zeroDiff) {
if (branchAction === "CONFLICTING") return 0;
if (nudgeSkip) return 4;
if (failedChecks.some(check => check.conclusion !== "ACTION_REQUIRED")) return 1;
if (feedback.length) return 2;
if (zeroDiff) return 3;
return 4;
}Stating the nudgeSkip gate once makes the ranking rule a readable spec instead of a repeated-guard ternary chain.
@copilot please address this.
| if (rateLimit.low) return { skip_reason: "rate_limit" }; | ||
| const current = checkNumber ? pr : fetchCandidate(repo, pr.number, api); | ||
| const current = checkNumber ? pr : fetchCandidate(repo, pr.number, api, now); | ||
| const skip = metadataSkip(current, now) || checkSkip(current, now); |
There was a problem hiding this comment.
[/codebase-design] metadataSkip/checkSkip are now called from four separate sites (classify, fetchCandidate, buildQueue, fetchQueue) with slightly different compositions (metadataSkip alone, metadataSkip || checkSkip, both). This is an easy-to-miss seam: a future caller that forgets checkSkip will silently skip the pending/active-agent gate.
💡 Suggested consolidation
Consider a single cheapSkip(pr, now, { checks = true } = {}) entry point that internally decides whether checkSkip applies (it can't run before check data exists, as in fetchCandidate), so there is one authoritative place encoding "which gates are safe before which fetch" instead of four call sites each re-deciding the composition. The tests already pin this behavior well (66 cases), but the production code's seam isn't as explicit as the test coverage implies.
@copilot please address this.
There was a problem hiding this comment.
Reviewed with the Impeccable skills reviewer. This PR is a backend/config change (Node.js prefilter logic + GitHub Actions workflow YAML) with no UI surface, so none of the Impeccable UI-audit modes (audit/critique/harden/distill/extract/clarify) apply. Applied a standard correctness/security review instead.
Findings: No blocking issues.
- The new
metadataSkip/checkSkipsplit,approvableRunsguard (exact workflow allowlist, PR/branch/SHA match,action_requiredconclusion handling), andnudge_needed/approve_runs/dismiss_reviewsgating are all covered by new unit tests. - Ran
node --test scripts/pr-sous-chef.test.mjs: 66/66 passing, including the new workflow-config assertions (schedule: every 5m,max-daily-ai-credits: -1, updatedif:gate,-label:broccoliskip filter). - The
.github/workflows/pr-sous-chef.mdprompt text was cross-checked against the script behavior (idle/stale/opted-out/approval-required semantics, slash-command acknowledgement rules) and is internally consistent. max-daily-ai-credits: -1is a supported, documented disable value inpkg/workflow/daily_aic_workflow.go, so disabling the daily guardrail here is intentional and correctly recognized by the compiler.
No inline comments added — nothing met the bar for actionable, blocking feedback.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 159.9 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
|
🎉 This pull request is included in a new release. Release: |
Why
Investigation of PR #65562 showed that daily-credit accounting blocked scheduled nudges, while some eligibility paths could still wake the agent only to no-op. Poll more frequently without starting the model unless the deterministic prefilter finds executable progress actions.
Changes
max-daily-ai-credits: -1. Per-run credit limits and GitHub API-budget checks remain unchanged.broccoliPRs before expensive detail reads.action_requiredconclusion.Explicit
/souschefrequests still start the agent for acknowledgement when API budgets are healthy. Disabling the daily cap permits more aggregate AI spending; the tighter startup gate and existing per-run limits remain in place. The workflow source, prompt/eval guidance, and generated lock file are updated together.Validation
node --test scripts/pr-sous-chef.test.mjs: 66 regression tests passed.npx --no-install prettier --check scripts/pr-sous-chef.mjs scripts/pr-sous-chef.test.mjs.make recompile: all 320 workflows compiled successfully../gh-aw compile pr-sous-chef --strict --no-check-update --schedule-seed github/gh-aw.PATH=/opt/homebrew/bin:$PATH make agent-report-progress: passed, including impacted tests and workflow drift checks.