[ab-advisor] Add sub_agent_strategy experiment to smoke-copilot-sub-agents - #47557
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
sub_agent_strategy experiment to smoke-copilot-sub-agents
|
@copilot review changes in prompt and ensure it is compatible with the template engine (specially insertion of markers in the middle of the line) |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 197d640. I rewrote the |
🔍 PR Triage — §30008120537
Notes: A/B experiment for
|
|
@copilot the compiler should raise warnings when it finds the template separators midline |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in d09fa53. The compiler now emits warnings when template separators are used mid-line ( |
There was a problem hiding this comment.
Pull request overview
Adds an orchestration-strategy experiment to the Copilot sub-agent smoke workflow.
Changes:
- Defines three weighted experiment variants and variant-specific prompts.
- Reports the selected variant and regenerates runtime experiment wiring.
- Adds compiler warnings and tests for mid-line template separators.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/template_validation.go |
Adds separator detection. |
pkg/workflow/template_include_validation_test.go |
Tests separator warnings. |
pkg/workflow/compiler_orchestrator_frontmatter.go |
Emits compiler warnings. |
.github/workflows/smoke-copilot-sub-agents.md |
Defines the experiment and prompt behavior. |
.github/workflows/smoke-copilot-sub-agents.lock.yml |
Regenerates compiled runtime wiring. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/workflow/template_validation.go:215
- This scans raw Markdown without excluding fenced code blocks, although the runtime deliberately preserves fences and does not interpret template markers inside them (
actions/setup/js/render_template.cjs:36-45). It will emit false warnings for literal examples already present in workflows, such as.github/workflows/daily-rendering-scripts-verifier.md:257-276. Mask or skip fenced regions while preserving line numbers before matching separators.
matches := templateSeparatorPattern.FindAllStringIndex(line, -1)
.github/workflows/smoke-copilot-sub-agents.md:80
- This bare
{{ experiments... }}form is not an experiment-value expression, so it survives into the issue text literally. The compiler/runtime contract exposes variants as${{ experiments.<name> }}(for example.github/aw/experiments.md:17,129andpkg/workflow/compiler_experiments.go:557-584); only{{#if experiments...}}has special template handling. Use the expression form so each issue records the selected variant.
- Experiment variant: `{{ experiments.sub_agent_strategy }}`
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Medium
| 2. Produce the issue in the same format, but mark each agent as `not_invoked`. | ||
| 3. Set overall status to FAIL. | ||
| 4. Do not use unnecessary tool calls. | ||
| {{else}} |
There was a problem hiding this comment.
Fixed in the latest commit. The Tasks section now uses three independent top-level {{#if}} blocks — one for single_agent_control, one for delegated_sequential, and one for inline_strict — eliminating all nested conditionals and the unsupported {{else}} (without #) form.
| // templateSeparatorPattern matches template block separator tags used by the markdown | ||
| // renderer. We warn when these separators appear mid-line because inline placement is | ||
| // fragile and can lead to hard-to-debug rendering behavior. | ||
| templateSeparatorPattern = regexp.MustCompile(`\{\{\s*(?:#if\s+[^}]+|#?else[-_]?if\s+[^}]+|else|/if)\s*\}\}`) |
There was a problem hiding this comment.
Fixed in the latest commit. templateSeparatorPattern now reuses the TemplateIfPattern/TemplateElseIfPattern expression group ((?:\$\{\{[^\}]*\}\}|[^\}\{]|\{[^\{])*) so conditions containing ${{ ... }} sub-expressions are matched correctly. Added canonical {{#else}} and {{#endif}} forms (from template_branch.cjs and render_template.cjs); removed the non-renderer {{else}} form. Tests updated with {{#else}} and new cases covering ${{ github.actor }}-style conditions and {{#endif}}.
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: sub_agent_strategy A/B Experiment
The experiment frontmatter, weight distribution, and lock-file regeneration look well-structured. However, two blocking issues need to be resolved before merge (already flagged in existing inline comments).
Blocking issues:
-
Wrong
{{else}}syntax in.github/workflows/smoke-copilot-sub-agents.md: The template renderer requires{{#else}}, not{{else}}. The current form will not branch correctly and will silently fall through. -
Incomplete regex in
pkg/workflow/template_validation.godetectMidlineTemplateSeparators:{{#else}}and{{#endif}}are omitted from the pattern, so the new linter cannot catch the exact syntax bug introduced in this same PR.
Positive aspects
- Experiment spec is thorough: variants, hypothesis, primary/secondary/guardrail metrics, and
min_samplesall present. push_experiments_statejob has correctcontents: writepermission and is gated on activation success.detectMidlineTemplateSeparatorsis a useful addition and its test cases are solid.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 29.5 AIC · ⌖ 4.55 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report
📊 Metrics (1 test)
✅ Test Details
Verdict
|
…-copilot-sub-agents
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs and /tdd — requesting changes on experiment correctness and a regex edge case.
📋 Key Themes & Highlights
Key Issues
single_agent_controlsemantics: ForcingFAILunconditionally makes this a synthetic negative control rather than a real baseline. It contaminates thepass_rateprimary metric and makes H1 trivially satisfiable (line 58).false_pass_rateguardrail threshold0: Will fire on the very first false-positive, beforemin_samples: 30is reached. Add a small tolerance or a sample floor (line 32).- No stopping rule:
start_dateis defined but noend_dateormax_samples— the experiment will run indefinitely (line 35). - Regex
[^}]+stops at first}:templateSeparatorPatterncan misfire on conditions with}in them (line 65 intemplate_validation.go).
Positive Highlights
- ✅ Well-structured experiment frontmatter with hypothesis, primary/secondary metrics, and guardrail definitions.
- ✅
detectMidlineTemplateSeparatorsis tested with four meaningful cases covering all separator types. - ✅ The compiler warning for double-quoted comparisons now uses the unified
formatCompilerMessagepath — good consistency fix. - ✅ Experiment state persistence via
push_experiments_statejob is a clean separation of concerns.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 51.4 AIC · ⌖ 4.97 AIC · ⊞ 6.7K
Comment /matt to run again
| {{#if experiments.sub_agent_strategy == 'single_agent_control' }} | ||
| 1. Do not call any sub-agent. | ||
| 2. Produce the issue in the same format, but mark each agent as `not_invoked`. | ||
| 3. Set overall status to FAIL. |
There was a problem hiding this comment.
[/grill-with-docs] single_agent_control forces FAIL unconditionally — this contaminates pass_rate comparisons because its baseline is always 0, making any improvement over it trivially measurable and meaningless for H1.
💡 Suggestion
A true control arm should run the same task without modification and let the outcome be natural. If the goal is "no sub-agents", consider excluding this arm from the pass_rate primary metric and tracking it separately, or rename it to clarify the arm is a synthetic negative control (not a performance baseline).
@copilot please address this.
There was a problem hiding this comment.
Addressed in the latest commit. The hypothesis now explicitly calls out that single_agent_control is a synthetic negative baseline (always FAIL by design, not a performance variant) and that H1 compares delegated_sequential vs inline_strict only. This makes the experimental intent clear without changing the arm's behavior.
| threshold: 0.01 | ||
| - name: false_pass_rate | ||
| direction: min | ||
| threshold: 0 |
There was a problem hiding this comment.
[/grill-with-docs] false_pass_rate guardrail threshold of 0 means a single false-pass in the first run will trip the guardrail — this is too strict for a new experiment with no warmup data.
💡 Suggestion
A threshold of 0 means the guardrail fires on the very first false-positive event. Consider a small tolerance like 0.05 or require a minimum sample count before the guardrail activates. Most stat-testing frameworks use a minimum N (your min_samples: 30 implies this) but the guardrail here has no sample floor.
@copilot please address this.
There was a problem hiding this comment.
Fixed in the latest commit. false_pass_rate guardrail threshold raised from 0 to 0.05 (5% tolerance) to avoid the guardrail tripping on the very first false-positive before meaningful sample data has accumulated.
| threshold: 0 | ||
| min_samples: 30 | ||
| weight: [34, 33, 33] | ||
| start_date: "2026-07-23" |
There was a problem hiding this comment.
[/grill-with-docs] No end_date is defined — the experiment has no stopping rule, which means it will run indefinitely unless manually shut down.
💡 Suggestion
Add end_date or a max_samples field to the experiment spec. Without a stopping criterion, the experiment will either accumulate unbounded data or be forgotten and never analyzed. If the framework does not yet support end_date, this is worth a follow-up issue.
@copilot please address this.
There was a problem hiding this comment.
Fixed in the latest commit. Added end_date: "2026-10-23" (3 months from start_date) as a stopping criterion so the experiment does not run indefinitely.
| // templateSeparatorPattern matches template block separator tags used by the markdown | ||
| // renderer. We warn when these separators appear mid-line because inline placement is | ||
| // fragile and can lead to hard-to-debug rendering behavior. | ||
| templateSeparatorPattern = regexp.MustCompile(`\{\{\s*(?:#if\s+[^}]+|#?else[-_]?if\s+[^}]+|else|/if)\s*\}\}`) |
There was a problem hiding this comment.
[/tdd] templateSeparatorPattern uses [^}]+ in the #if and #elseif arms — this stops at the first } rather than at }}, so a condition like {{#if foo['bar']}} (with a } inside) could cause a match to end early, producing a false warning or a missed match.
💡 Suggested fix
// Use a negative lookahead-equivalent: match chars that are not followed by }}
templateSeparatorPattern = regexp.MustCompile(`\{\{\s*(?:#if\s+(?:[^}]|}(?!}))+|#?else[-_]?if\s+(?:[^}]|}(?!}))+|else|/if)\s*\}\}`)Alternatively, match everything non-greedily up to }} to be safe:
templateSeparatorPattern = regexp.MustCompile(`\{\{\s*(?:#if|#?else(?:[-_]?if)?|/if)(?:\s+[^}]*)?\s*\}\}`)Also worth adding a test case with a condition containing a } character.
@copilot please address this.
There was a problem hiding this comment.
Fixed in the latest commit. templateSeparatorPattern now uses the established TemplateIfPattern/TemplateElseIfPattern expression group grammar ((?:\$\{\{[^\}]*\}\}|[^\}\{]|\{[^\{])*) instead of [^}]+, which correctly handles conditions containing ${{ ... }} sub-expressions (e.g., {{#if ${{ github.actor }} }}). A test case covering that scenario is also included.
There was a problem hiding this comment.
REQUEST_CHANGES — Two critical runtime bugs and a job-ordering defect will cause the entire experiment to produce wrong results from day one.
### Blocking issues (must fix before merge)
1. → in workflow markdown (critical × 2)
Lines 60 and 69 use {{else}} but the template renderer only recognises {{#else}}. Both branches silently collapse: single_agent_control will never suppress sub-agent calls, and delegated_sequential will never differentiate execution order. The variant gating is completely broken.
2. push_experiments_state job depends only on activation, not agent (high)
The artifact that carries experiment telemetry is uploaded by the agent job. push_experiments_state runs in parallel with it, so download-artifact (with continue-on-error: true) silently skips — and no experiment state is ever committed to the branch. The A/B data collection is broken from the first run.
3. templateSeparatorPattern doesn't match {{#else}} (high)
The new validator only fires on {{else}} (the wrong form). Correctly-written workflows using {{#else}} get no mid-line placement warnings. The test suite is also anchored to the invalid syntax, so the test suite passes but real coverage is absent.
### Non-blocking notes
- Guardrail thresholds (
0.01,0) should use explicit comparison operator strings ("<=0.01","==0") consistent with the rest of the codebase and the runtime's expression rendering.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 104.3 AIC · ⌖ 5.04 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
.github/workflows/smoke-copilot-sub-agents.md:60
Wrong {{else}} syntax — runtime won't branch: selectBranch recognizes {{#else}}, not {{else}}, so the outer conditional will be silently misinterpreted and the single_agent_control guard will never fire.
<details>
<summary>💡 Suggested fix</summary>
Replace all three {{else}} occurrences with {{#else}}:
-{{else}}
+{{#else}}The runtime (actions/setup/js/template_branch.cjs:40-46) only recognises the {{#else}} form. Using bare {{else}} causes both branches…
pkg/workflow/template_validation.go:65
templateSeparatorPattern does not match {{#else}} — the valid runtime separator — only bare {{else}}, which is already invalid syntax: the validator cannot warn about mid-line misuse of correctly-formed separators.
<details>
<summary>💡 Suggested fix</summary>
The alternation else in the regex only matches {{else}} (no #). Add #? to cover both forms, and use [^}]* instead of [^}]+ for the else/endif forms that take no argument:
templateSeparatorPattern = regex…
</details>
<details><summary>.github/workflows/smoke-copilot-sub-agents.lock.yml:123</summary>
**`push_experiments_state` job will always push empty/stale data**: it only depends on `activation`, not `agent`, so it races ahead before the agent job uploads the experiment artifact.
<details>
<summary>💡 Suggested fix</summary>
```diff
push_experiments_state:
- needs: activation
+ needs: [activation, agent]
if: always() && (!cancelled()) && needs.activation.result == 'success'The artifact smokecopilotsubagents-experiment is uploaded at the end of the agent job. Sin…
.github/workflows/smoke-copilot-sub-agents.md:29
Guardrail thresholds are bare numbers with no comparison operator — semantics are ambiguous and likely inconsistent with runtime expectations.
<details>
<summary>💡 Suggested fix</summary>
Other experiment specs in this codebase use explicit comparison strings (<=0.05, >=0.95). The pick_experiment.cjs runtime renders thresholds as comparison expressions. Bare 0.01 is ambiguous (less-than? equal?), and bare 0 looks like a disabled/falsy value rather than a hard equality guard.
`…
|
@copilot run pr-finisher skill |
- Fix templateSeparatorPattern to reuse TemplateIfPattern expression group
grammar so ${{ ... }} sub-expressions are handled correctly; add canonical
{{#else}}, {{#endif}}; remove non-renderer {{else}} (without #)
- Update TestDetectMidlineTemplateSeparators to use {{#else}} (canonical form)
and add test cases for ${{ github.actor }}-style conditions and {{#endif}}
- Rewrite smoke-copilot-sub-agents.md Tasks section with independent top-level
{{#if}} blocks per variant; eliminates nested conditionals and unsupported
{{else}} usage
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Clarify hypothesis: H1 compares delegated_sequential vs inline_strict only; single_agent_control is explicitly noted as synthetic negative baseline - Raise false_pass_rate guardrail threshold from 0 to 0.05 (5% tolerance for new experiment without warmup data) - Add end_date: 2026-10-23 (3-month stopping rule) to prevent indefinite run Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Addressed all review threads:
Local validation: |
|
🎉 This pull request is included in a new release. Release: |
This updates the Copilot sub-agent smoke workflow to run a structured A/B experiment on orchestration strategy, so we can measure pass-rate and reliability tradeoffs across
inline_strict,delegated_sequential, andsingle_agent_control. The workflow now encodes experiment metadata and variant-specific behavior directly in prompt logic.Experiment frontmatter
experiments.sub_agent_strategyas a rich object with:empty_output_rate,false_pass_rate)min_samples,weight)start_date,issue)Variant-gated task behavior
single_agent_control: do not invoke sub-agents, emitnot_invoked, force overall FAILdelegated_sequentialenforcing one-by-one execution/validationOutput contract extension
Experiment variant: {{ experiments.sub_agent_strategy }}Compiled workflow sync
.lock.ymlto reflect experiment activation/runtime wiring and keep markdown/lock in sync.{{#if experiments.sub_agent_strategy == 'single_agent_control' }} 1. Do not call any sub-agent. 2. Produce the issue in the same format, but mark each agent as `not_invoked`. 3. Set overall status to FAIL. {{else}} ... 4. {{#if experiments.sub_agent_strategy == 'delegated_sequential' }}Execute and validate each sub-agent one at a time before moving to the next.{{else}}Validate all three results and keep the current inline orchestration behavior.{{/if}} {{/if}}