Repository navigation
Stabilize smoke-copilot-sub-agents: mandatory delegation, experiment-independent contract checks - #70944
Conversation
…he experiment Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
|
Review findings (round 2): Small round 2 for this PR (#70944, issue #70938):
|
…ection Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
Done in 2883a29:
All CI checks on this PR are green.
I left those failures alone and noted them in the PR description. I reverted everything |
|
✅ 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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: 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.
|
|
✅ 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.
|
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
One correctness gap remains in the smoke contract: the post-step validates the three required children but does not reject additional sub-agent invocations. I left an inline comment requesting an exact-name/count assertion so unintended calls cannot silently pass or skew the experiment.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 2.06 AIC · ⌖ 0.556 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — this is a well-targeted flakiness fix with solid new regression coverage. Two non-blocking findings posted inline, plus one acceptance-gate note below.
📋 Key Themes & Highlights
Key Themes
- Reordering resolution checks before the
SMOKE_EXECUTIONassert is a textbook/diagnosing-bugsroot-cause fix: a dead-pinned child now reports its actualmodel.resolutioncause instead of a generic execution-failure message. The newsmoke_copilot_sub_agents.test.cjscase (reports a dead child pin's model.resolution cause...) directly pins this regression — good/tdddiscipline. - Removing
single_agent_controleliminates a test design smell (a variant that was designed to always fail was polluting the smoke signal with experiment noise). Scoring viametric: eval:sub_agent_strategy_goal_metinstead ofpass_ratecorrectly decouples the orchestration-style experiment from the mandatory contract gate, following the existingcopilot-pr-nlp-analysis.mdprecedent foreval:-prefixed metrics. - The "required first step" phrasing mirrors the established pattern in
smoke-copilot.md(#70387), keeping prompt conventions consistent across the smoke-test family. - Flagged inline: the
end_date: 2026-10-23experiment window looks stale givenmin_samples: 30and zero dispatches so far per the PR body — worth a bump before this starts collecting live data. - Flagged inline: the new "first tool call must be
haiku-whoami" prompt rule has no corresponding ordering assertion in the post-step script or tests, so regressions in call order would pass silently.
Positive Highlights
- ✅
servedModelsbecoming aMapkeyed by agent name (was aSetof models) is a clean refactor that both preserves the existing "models must be distinct across children" invariant and enables the per-agent lookup needed for the reordered proxy-log checks. - ✅
interpolate_prompt.test.cjsadds a dedicated per-variant "Experiment variant:" output-line test, closing a gap where only the Tasks-section instructions were previously verified. - ✅ PR description is transparent about outstanding acceptance work (live dispatches, dead-pin scratch run) rather than claiming it's done.
Acceptance note: the PR body's own "Acceptance not yet done" section says no workflows have been dispatched yet. Per the repo's live-execution rules, this should gate merge until the ten dispatches and the dead-child-pin scratch run are completed and linked.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 103.6 AIC · ⌖ 15.1 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
🟡 Changes recommended
Experiment scoring and startup diagnostics need corrections, and required hosted acceptance validation remains outstanding.
5 open issues
What changed in this PR
Updates gh-aw’s Copilot SDK sub-agent smoke workflow to require delegation in every run and separate experiment scoring from the smoke contract.
Changes:
- Remove the negative-control variant and require all three child agents.
- Prioritize resolution diagnostics while preserving lifecycle, served-model, and endpoint checks.
- Add dead-pin and variant-rendering regression coverage, and regenerate the workflow lock.
| File | Description |
|---|---|
actions/setup/js/smoke_copilot_sub_agents.test.cjs |
Replace negative-control coverage with a dead-pin diagnostic case. |
actions/setup/js/interpolate_prompt.test.cjs |
Check mandatory delegation and variant-specific report rendering. |
.github/workflows/smoke-copilot-sub-agents.md |
Update delegation requirements, experiment scoring, diagnostics, and reporting. |
.github/workflows/smoke-copilot-sub-agents.lock.yml |
Reflect the authored workflow changes in generated Actions configuration. |
Review effort: Max
… pin cause first, and rename sub-agent eval ID Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
Round 3 is in 7592c28. Changes per thread:
The lock file is regenerated, and the 17 smoke tests pass.
I didn't run these on main, so I haven't confirmed they also fail there; the latest CI run on main passes. I reverted the regenerated changes the run made to unrelated files to keep this PR to the one fix. |
|
Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. @copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 7592c28
|
…ke-copilot-sub-agents Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Committed and pushed |
|
Verified at
|

smoke-copilot-sub-agentswas failing for reasons unrelated to the sub-agent/model contract. Itssingle_agent_controlarm banned delegation and was designed to always FAIL. Whether delegation happened at all depended on which experiment variant the run drew. This PR makes the contract exercise mandatory in every run, and scores the experiment separately from the smoke result. Template evaluator, compiler, harness and AWF are unchanged; the quoted-comparison fix already merged in #70747.Prompt
taskcall tohaiku-whoamiwithwho am i?. This follows the required-first-step pattern used insmoke-copilot(Resolve experiment references in inline sub-agent frontmatter before writing the agent file; fail smoke-copilot whenfile-summarizerdoesn't run #70387).taskcall each togpt5-whoamiandterra-whoami.Experiment
single_agent_control. Variants are now[inline_strict, delegated_sequential], weighted[50, 50].pass_ratetoeval:sub_agent_strategy_goal_met, so the experiment is scored separately from the smoke pass/fail. The eval question now describes the orchestration objective.Post-step assertions
SMOKE_VARIANTenv./chat/completions(Haiku) or/responses(GPT)Variant interpolation
${{ experiments.sub_agent_strategy }}fails to compile onmain:validateSingleExpressionrejectsexperiments.*in the prompt body. The documented${{ experiments.style }}example fails the same way. Fixing that would mean a compiler change, which is out of scope here. The report renders the variant with{{#if experiments.sub_agent_strategy == '…' }}blocks instead.Tests (outside the two scoped files)
Two existing tests asserted the removed control arm and would fail without changes:
smoke_copilot_sub_agents.test.cjs: the control test is replaced by a dead-pin test that checks themodel.resolutioncause is reported.interpolate_prompt.test.cjs: dropped the control case, and both remaining variants must render thehaiku-whoamicall.Acceptance not yet done
No workflows have been dispatched for this branch. Still needed, under the repo's live-execution rules:
model.resolutioncause.