Repository navigation
Surface sub-agent failures and replace unpriced kiwi workflow models - #69788
Conversation
Preserve Actions success rates while degrading current-revision health when fresh usage summaries record failed sub-agent invocations. Include affected agents, models, and runs without leaking raw diagnostics. Document the observed kiwi pricing rejection separately from alias resolution. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. PR Code Quality ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
Use the advertised lightweight gpt-6-luna model for all five active kiwi selections. Its token prices are present in the embedded catalog and provider metadata. Preserve workflow prompts, permissions, budgets, and historical audit fixtures, and regenerate the three affected locks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (348 new lines under 📄 Draft ADR committed: Decision captured:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and check that the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
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.
|
![github-actions[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
I didn't find a changed-line issue here that is strong enough to block merge.
Reviewed areas
- status propagation for cached sub-agent failures
- stale-cache guards and revision scoping
- fleet summary / detailed output behavior
- regression coverage for duplicate, stale, and historical runs
Static review only; I did not execute the code in this pass.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 88.1 AIC · ⌖ 6.16 AIC · ⊞ 12.7K
Comment /review to run again
| pr := ProcessedRun{ | ||
| Run: cached, JobDetails: summary.JobDetails, MCPFailures: summary.MCPFailures, | ||
| MissingTools: summary.MissingTools, MissingData: summary.MissingData, FirewallAnalysis: summary.FirewallAnalysis, | ||
| cached.Repository != run.Repository || cached.Attempt != run.Attempt || !cached.UpdatedAt.Equal(run.UpdatedAt) { |
There was a problem hiding this comment.
This repository check is case-sensitive, so --repo Owner/Repo can make a valid cache look stale and silently suppress the very health evidence this change is trying to surface.
💡 Why this matters
GitHub repository slugs are case-insensitive, and other cache-validation paths in this codebase already use strings.EqualFold for that reason. Using raw string equality here means a casing difference between the cached summary and the run source will drop both failure-cluster enrichment and sub-agent-failure reporting as false negatives.
Prefer normalizing or case-folding the repository comparison, for example:
!strings.EqualFold(cached.Repository, run.Repository)That keeps the stale-cache guard strict without rejecting equivalent repository identities.
| if health.SubagentFailureCount == 0 { | ||
| return | ||
| } | ||
| health.Status = "subagent_failures_observed" |
There was a problem hiding this comment.
This unconditionally overwrites failures_observed and insufficient_evidence, so a workflow with real run failures or too little data now gets reported only as subagent_failures_observed.
💡 Why this blocks the change
calculateCurrentWorkflowHealth already derives a status from Actions outcomes and sample sufficiency. Replacing that status here throws away the primary signal whenever cached sub-agent failures exist, which means both the CLI and JSON can tell callers the wrong reason a workflow is unhealthy.
A safer pattern is to preserve the existing status and surface sub-agent failures as an additional dimension, or only promote to subagent_failures_observed from statuses like healthy / recovery_observed.
if health.SubagentFailureCount > 0 {
switch health.Status {
case "healthy", "recovery_observed":
health.Status = "subagent_failures_observed"
}
}That keeps failed or insufficient revisions from being misclassified while still exposing failed delegation.
|
🎉 This pull request is included in a new release. Release: |
There was a problem hiding this comment.
🟡 Changes recommended
Contribution Check’s replacement sub-agents remain endpoint-incompatible when its CLI session selects a Claude parent.
3 open issues
What changed in this PR
Adds cached sub-agent failure reporting to gh-aw health, exposing failed delegation even when Actions runs succeed.
Changes:
- Report failed invocations, affected runs, and agent/model attribution.
- Validate cached evidence and document its limits.
- Replace five
kiwiselections withgpt-6-lunaand refresh three locks.
| File | Description |
|---|---|
pkg/cli/health_subagents.go |
Aggregates and renders cached delegation failures. |
pkg/cli/health_subagents_test.go |
Tests counts, output, cache validation, and exclusions. |
pkg/cli/health_revisions.go |
Integrates delegation evidence into revision health. |
pkg/cli/health_metrics.go |
Adds workflow and fleet failure counters. |
pkg/cli/health_failure_clusters.go |
Shares cache loading and identity checks. |
pkg/cli/health_command.go |
Displays failure totals and warnings. |
docs/src/content/docs/setup/cli.md |
Documents health fields and cache limitations. |
.github/workflows/dead-code-remover.md |
Updates the discovery sub-agent model. |
.github/workflows/dead-code-remover.lock.yml |
Refreshes the source body hash. |
.github/workflows/daily-malicious-code-scan.md |
Updates two scanning sub-agent models. |
.github/workflows/daily-malicious-code-scan.lock.yml |
Refreshes model metadata and body hash. |
.github/workflows/contribution-check.md |
Updates formatter and dispatcher models. |
.github/workflows/contribution-check.lock.yml |
Refreshes model metadata and body hash. |
.github/aw/debug-model-engine-misconfiguration.md |
Distinguishes pricing rejection from routing failures. |
Review effort: Max
| --- | ||
| description: Groups PR verdict JSONs into Ready/Needs-look/Off-guidelines tables and returns the markdown body for the contribution check report issue | ||
| model: kiwi | ||
| model: gpt-6-luna |
| if health.SubagentFailureCount > 0 { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("%d sub-agent invocation(s) failed across %d run(s); Actions success rate does not establish healthy delegation", health.SubagentFailureCount, health.SubagentFailureRuns))) | ||
| } else if health.BelowThresh { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Success rate (%.1f%%) is below threshold (%.1f%%)", health.SuccessRate, threshold))) | ||
| } else if health.Status == "healthy" || health.Status == "recovery_observed" { | ||
| fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Success rate (%.1f%%) is above threshold (%.1f%%)", health.SuccessRate, threshold))) | ||
| } else { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Current revision has insufficient evidence for a healthy status")) | ||
| } |
| health.Status = "subagent_failures_observed" | ||
| for i := range health.Revisions { | ||
| revision := &health.Revisions[i] | ||
| for _, id := range revision.Current.RunIDs { | ||
| revision.Current.SubagentFailures += failuresByRun[id] | ||
| } | ||
| if revision.Current.SubagentFailures > 0 { | ||
| revision.Status = health.Status | ||
| } | ||
| } |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs (per pr-triage), with a /codebase-design lens on the shared cache-loading path. Overall: solid feature with strong test coverage and careful preservation of Actions truth; two non-blocking findings posted inline.
📋 Key Themes & Highlights
Key Themes
- Duplicated cache reads (
/codebase-design):health_subagents.goandhealth_failure_clusters.goeach independently callloadHealthRunSummaryfor the same run set, re-reading/re-parsingrun_summary.jsontwice per health check. Minor perf/architecture nit, not a bug. - Status precedence not documented (
/grill-with-docs):applyHealthSubagentFailuresunconditionally overwriteshealth.Status, discarding the revision-derived status (healthy/recovery_observed/insufficient_evidence). Likely intentional (subagent failures should dominate messaging) but worth a short comment explaining the precedence rule so future readers do not need to reverse-engineer it.
Positive Highlights
- ✅ Excellent test coverage in
health_subagents_test.go: 11-field stale-cache rejection matrix, missing/corrupt cache fallback, intentional-failure exclusion from fleet counts, and explicit assertion that historical-revision failures do not degrade the current revision. - ✅ Careful double-count avoidance — explicit comment and test proving
DeclaredSubagentModelsis not summed alongsideSubagentModelRequests. - ✅ No raw diagnostic leakage — test explicitly asserts error text (e.g. "private diagnostic detail") never reaches JSON/table output.
- ✅ Docs (
cli.md) clearly explain the newsubagent_failures_observedstatus, cache-only limitation, and new JSON fields — good/grill-with-docshygiene.
Scope note
Triage ran successfully (pr-triage); no fallback heuristic was needed. Investigation stayed within budget — no open uncertainty beyond the two inline findings.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 211.7 AIC · ⌖ 14 AIC · ⊞ 10.1K
Comment /matt to run again
| func applyHealthSubagentFailures(health *WorkflowHealth, runs []WorkflowRun, config HealthConfig) { | ||
| failuresByRun := make(map[int64]int) | ||
| for _, run := range runs { | ||
| summary, _ := loadHealthRunSummary(run, config) |
There was a problem hiding this comment.
[/codebase-design] applyHealthSubagentFailures re-reads and re-parses the same run_summary.json via loadHealthRunSummary for every run, duplicating the load already done in enrichHealthFailureFromCache (health_failure_clusters.go) for the same run set during the same health check.
💡 Why this matters
Both buildHealthFailureClusters and calculateCurrentWorkflowHealth call loadHealthRunSummary(run, config) independently against overlapping run sets, so each cached summary file is read and JSON-unmarshalled twice per health check. It's not a correctness bug today, but as more health features consume the same cache (this is now the second consumer), the duplicated I/O will keep compounding and the two load paths can drift in validation rules over time.
Consider threading a shared map[int64]*RunSummary (or memoizing loadHealthRunSummary) through displayDetailedHealth/calculateHealthWorkflowReports so both consumers share one load per run.
@copilot please address this.
| if health.SubagentFailureCount == 0 { | ||
| return | ||
| } | ||
| health.Status = "subagent_failures_observed" |
There was a problem hiding this comment.
[/grill-with-docs] health.Status = "subagent_failures_observed" unconditionally overwrites whatever revision-derived status was just computed (healthy, recovery_observed, insufficient_evidence, etc.), discarding that information rather than composing it.
💡 Why this matters
calculateCurrentWorkflowHealth sets health.Status from revision classification, then applyHealthSubagentFailures clobbers it with a single flat string whenever any sub-agent failure is observed — even if the underlying revision evidence was insufficient_evidence or recovery_observed. The JSON/table output downstream (isHealthEvidenceInsufficient, displayDetailedHealthStatus) only sees the final string, so a caller can no longer tell “this workflow has insufficient run evidence AND a subagent failure” from “healthy but degraded by one subagent failure” — both collapse to the same status string. The per-revision loop does something closer to the right thing (if revision.Current.SubagentFailures > 0 { revision.Status = health.Status }), but the top-level WorkflowHealth.Status doesn't get the same nuance.
Worth confirming this is the intended design (it may be — subagent failures are meant to take precedence in messaging) and, if so, documenting it inline with a short comment explaining the precedence rule, since it's not obvious from the code alone.
@copilot please address this.
Why
A successful Actions run can hide failed sub-agents. Contribution Check run 39068401375 stayed green after its
report-formattersub-agent received akiwiAI-credit pricing rejection.Changes
subagent_failures_observed, invocation and affected-run counts, and run links with agent/model attribution.kiwisub-agent selections withgpt-6-lunain Contribution Check, Daily Malicious Code Scan, and Dead Code Remover. Regenerate the three affected locks while preserving prompts, permissions, budgets, and workflow intent.The original run contained a
kiwialias, but no verified pricing or canonical model mapping was found. Per the user's follow-up direction, use an advertised, priced model instead of inventing rates. GPT-6 Luna is present in the live Copilot model listing, gh-aw's embedded catalog, and the original run's firewall pricing metadata. Its published default-tier prices are $0.10/M input and $0.50/M output (GitHub pricing). Historicalkiwifailure fixtures and alias compatibility remain intact. Unrelated red CI is outside this PR.Validation
go test ./pkg/cli -run '^TestHealth' -count=1python3 -m unittest discover -s .github/skills/agentic-workflows/tests -vmake fmtandmake buildmake agent-report-progress. After resolving changed-file lint failures, the remaining lint, schema, and workflow-drift gates passed throughmake agent-report-progress-no-test, using modern Bash and an isolated temporary directory for lint locking. The no-test gates were rerun after the workflow replacements without repeating the PR's unit-test phase.make recompile: all 344 workflows compiled successfully; only the three intended locks changed..github/skills/operational-value-designer/scripts/verify-operational-value-contract-change.sh origin/mainmodel: kiwi, and correct generated sub-agent model metadata. Source changes are limited to model substitutions.report-formatter/kiwiinvocation,subagent_failures_observed, and an unchanged 100% Actions success rate.Health does not download artifacts; missing cached evidence is not proof of successful delegation.