[ab-advisor] Add model_size A/B experiment to test-quality-sentinel#43551
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds an A/B “model_size” experiment to the test-quality-sentinel agentic workflow, wiring the selected experiment variant into the Copilot engine model selection so the workflow can compare review quality vs cost across two model options.
Changes:
- Wires
engine.modelintest-quality-sentinel.mdtoneeds.activation.outputs.model_size. - Adds an
experiments.model_sizespec (variants, weights, min samples, metrics/guardrails, start date). - Regenerates
test-quality-sentinel.lock.ymlto include experiment state restore/pick/upload and push-back to an experiments git branch.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/test-quality-sentinel.md | Adds engine.model wiring and defines the model_size experiment campaign metadata. |
| .github/workflows/test-quality-sentinel.lock.yml | Recompiled workflow output, including experiment state handling and model wiring. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
| variants: [small, large] | ||
| description: "Tests whether a smaller model can preserve test-review decision quality at lower cost versus a larger reasoning-capable model." | ||
| hypothesis: "H0: model-size variant does not improve review usefulness acceptance rate. H1: a larger reasoning-capable model improves review usefulness acceptance rate by >=15 percentage points without materially increasing false-positive change requests." |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
❌ Test Quality Sentinel failed during test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ REQUEST_CHANGES — Two correctness bugs and two experiment-design issues that will produce invalid data must be fixed before merging.
### Blocking issues (must fix)
1. Variant names are not valid model identifiers (critical — agent will fail or silently default)
Variants [small, large] are awf-config firewall pool aliases, not model IDs recognized by the Copilot harness CLI. COPILOT_MODEL will receive the literal string "small" or "large", which the harness does not understand. Every run of the experiment will use an undetermined fallback model, making arm assignment meaningless. (See comment on lock.yml:979)
2. No fallback when model_size output is empty (critical — first run will fail)
engine.model: "${{ needs.activation.outputs.model_size }}" has no || guard. On first run (experiment branch not yet created), pick-experiment emits an empty string and the agent job fails immediately. (See comment on lock.yml:152)
3. Primary metric has no instrumentation path (high — experiment collects zero data)
review_usefulness_acceptance_rate is not written anywhere in the compiled workflow. The experiment will run indefinitely without ever reaching a conclusion. (See comment on sentinel.md:134)
4. min_samples: 70 delivers only ~25% power for the 15pp MDE (high — results will be statistical noise)
Each arm gets 35 samples. The 15pp MDE at 80% power requires 169 samples per arm. (See comment on sentinel.md:141)
🔎 Code quality review by PR Code Quality Reviewer · 138.7 AIC · ⌖ 11.1 AIC · ⊞ 5.4K
Comment /review to run again
| COPILOT_DUMMY_BYOK: dummy-byok-key-for-offline-mode | ||
| COPILOT_GITHUB_TOKEN: ${{ github.token }} | ||
| COPILOT_MODEL: ${{ vars.GH_AW_MODEL_AGENT_COPILOT || vars.GH_AW_DEFAULT_MODEL_COPILOT || 'claude-sonnet-4.6' }} | ||
| COPILOT_MODEL: ${{ needs.activation.outputs.model_size }} |
There was a problem hiding this comment.
COPILOT_MODEL receives the raw variant alias small/large instead of a concrete model identifier, which will cause the harness to crash or silently default on every run.
💡 Details and fix
In the compiled lock.yml the agent step sets:
COPILOT_MODEL: ${{ needs.activation.outputs.model_size }}This passes the literal string "small" or "large" (the experiment variant names) straight to the Copilot harness CLI. The harness expects a concrete model identifier like claude-haiku-4.5 or claude-sonnet-4.6. The alias-to-model-pool resolution (small → haiku/flash-lite, large → sonnet/gpt-5-pro) lives in the awf-config firewall layer, not in the harness — passing a symbolic alias to the harness is unsupported.
Compare with the detection job on line 1627 which correctly uses real model names:
COPILOT_MODEL: ${{ vars.GH_AW_MODEL_DETECTION_COPILOT || vars.GH_AW_DEFAULT_MODEL_COPILOT || 'claude-sonnet-4.6' }}Fix: Use concrete model identifiers as the variant values in test-quality-sentinel.md:
experiments:
model_size:
variants: [claude-haiku-4.5, claude-sonnet-4.6]With this, pick-experiment emits the real model name directly and no alias translation is needed at the harness layer.
| GH_AW_INFO_ENGINE_ID: "copilot" | ||
| GH_AW_INFO_ENGINE_NAME: "GitHub Copilot CLI" | ||
| GH_AW_INFO_MODEL: ${{ vars.GH_AW_MODEL_AGENT_COPILOT || vars.GH_AW_DEFAULT_MODEL_COPILOT || 'claude-sonnet-4.6' }} | ||
| GH_AW_INFO_MODEL: "${{ needs.activation.outputs.model_size }}" |
There was a problem hiding this comment.
Empty model_size output (e.g. on first run before the experiment branch exists) passes an empty string to COPILOT_MODEL with no fallback, crashing or silently corrupting the agent run.
💡 Details and fix
The restore-experiment-state step (load_experiment_state_from_repo.cjs) has no continue-on-error: true. If the experiment branch experiments/testqualitysentinel doesn't exist yet (first ever run), the step will fail. Even if it doesn't hard-fail, the downstream pick-experiment step may emit an empty model_size output, which then flows through:
GH_AW_INFO_MODEL: "${{ needs.activation.outputs.model_size }}" # line 152
...
COPILOT_MODEL: ${{ needs.activation.outputs.model_size }} # line 979No || fallback exists on either expression. Every other model reference in this workflow (detection job, before this PR) uses a safe triple-fallback:
${{ vars.GH_AW_MODEL_DETECTION_COPILOT || vars.GH_AW_DEFAULT_MODEL_COPILOT || 'claude-sonnet-4.6' }}Fix: Add restore-experiment-state as continue-on-error: true and add a fallback to the model expression:
engine:
model: "${{ needs.activation.outputs.model_size || 'claude-sonnet-4.6' }}"| variants: [small, large] | ||
| description: "Tests whether a smaller model can preserve test-review decision quality at lower cost versus a larger reasoning-capable model." | ||
| hypothesis: "H0: model-size variant does not improve review usefulness acceptance rate. H1: a larger reasoning-capable model improves review usefulness acceptance rate by >=15 percentage points without materially increasing false-positive change requests." | ||
| metric: review_usefulness_acceptance_rate |
There was a problem hiding this comment.
The primary metric review_usefulness_acceptance_rate has no instrumentation path in the compiled workflow — this experiment will never collect any primary data.
💡 Details
review_usefulness_acceptance_rate is set as the metric (primary KPI) for the experiment. However, nothing in the compiled test-quality-sentinel.lock.yml writes this value to the experiment state artifact. The framework-collected metrics available automatically are: run_success_rate, run_duration_ms, ai_credits_total, effective_tokens. All of these appear in other experiments in the repo.
A "usefulness acceptance rate" that tracks whether PR reviewers found the AI review helpful requires one of:
- A reaction-harvesting job that polls PR reactions/comments for thumbs-up signals after the fact
- A human-in-the-loop labelling step
- An explicit write to the experiment state artifact from within the agent prompt
Without any of these, the experiment branch will accumulate runs with review_usefulness_acceptance_rate always null. The ab-advisor will never reach a conclusion.
Also affected: false_positive_change_request_rate (secondary metric + guardrail) has the same problem — no automatic collection mechanism — making the <=0.20 guardrail permanently inert.
Fix: Either replace the primary metric with one the framework collects automatically (e.g., run_success_rate as a proxy for now), or add a post-processing job that harvests and writes the acceptance signal. If proceeding with a manual metric, document the collection SLA.
| threshold: "<=0.20" | ||
| - name: run_success_rate | ||
| threshold: ">=0.90" | ||
| min_samples: 70 |
There was a problem hiding this comment.
min_samples: 70 gives ~25% statistical power for the claimed 15pp MDE — the experiment will almost certainly produce inconclusive results.
💡 Power calculation
With min_samples: 70 and weight: [50, 50], each arm gets 35 samples. For a two-proportion z-test (alpha=0.05, two-tailed), detecting a 15 percentage-point difference (the H1 threshold stated in the hypothesis) from a baseline of 0.50 requires approximately 169 samples per arm (338 total) at 80% power.
At n=35 per arm, the achieved power is approximately 25% — meaning there is a 75% chance of incorrectly accepting H0 and shipping no model change even if the large model is genuinely 15pp better. Conversely, any positive result at this sample size is highly likely to be noise.
Fix: Raise min_samples to at least 340, or narrow the MDE to a level that is achievable within the traffic budget. If the workflow fires infrequently (e.g. ~5 PRs/day), document the expected time to reach sample size.
| description: "Tests whether a smaller model can preserve test-review decision quality at lower cost versus a larger reasoning-capable model." | ||
| hypothesis: "H0: model-size variant does not improve review usefulness acceptance rate. H1: a larger reasoning-capable model improves review usefulness acceptance rate by >=15 percentage points without materially increasing false-positive change requests." | ||
| metric: review_usefulness_acceptance_rate | ||
| secondary_metrics: [ai_credits_total, false_positive_change_request_rate] |
There was a problem hiding this comment.
false_positive_change_request_rate appears in both secondary_metrics and guardrail_metrics — the duplication is contradictory and signals unclear intent.
💡 Details
Line 135: secondary_metrics: [ai_credits_total, false_positive_change_request_rate]
Lines 136–138:
guardrail_metrics:
- name: false_positive_change_request_rate
threshold: "<=0.20"A metric should be either a passive observation (secondary) or an active gate (guardrail), not both simultaneously. If the framework processes both lists independently, the metric is evaluated under two different semantics in the same run. If it deduplicates, one entry is silently dropped — and it's unclear which takes precedence.
Fix: Remove false_positive_change_request_rate from secondary_metrics and leave it only in guardrail_metrics where the threshold is operative:
secondary_metrics: [ai_credits_total]
Skills-Based Review 🧠Applied 📋 Key Themes & HighlightsKey Themes
Positive Highlights
@copilot please address the review comments above.
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 166.7 AIC · ⌖ 11.1 AIC · ⊞ 6.7K
Comment /matt to run again
| if: ${{ contains(toJSON(vars), '"GH_AW_RUNTIME_FEATURES":') }} | ||
| run: bash "${RUNNER_TEMP}/gh-aw/actions/log_runtime_features_summary.sh" | ||
| - name: Restore experiment state from git | ||
| id: restore-experiment-state |
There was a problem hiding this comment.
[/tdd] The restore-experiment-state step has no continue-on-error: true guard — if the experiments branch doesn't exist yet (first run) or a transient API error occurs, the entire activation job fails and model_size is never output, blocking the whole workflow.
💡 Suggested fix
The sibling Download experiment artifact step at line 177 already uses continue-on-error: true. Apply the same pattern here:
- name: Restore experiment state from git
id: restore-experiment-state
continue-on-error: true # first-run: branch doesn't exist yet
uses: actions/github-script@...The load_experiment_state_from_repo.cjs module itself documents that it "falls back gracefully to an empty state when the branch or file does not yet exist" — but that only works if the step itself is allowed to soft-fail at the runner level.
@copilot please address this.
| const { main } = require('${{ runner.temp }}/gh-aw/actions/load_experiment_state_from_repo.cjs'); | ||
| await main(); | ||
| - name: Pick experiment variants | ||
| id: pick-experiment |
There was a problem hiding this comment.
[/tdd] The pick-experiment step also lacks continue-on-error: true. If it fails, steps.pick-experiment.outputs.model_size is empty, and COPILOT_MODEL (line 979) and GH_AW_ENGINE_MODEL (line 1838) receive empty strings — the agent runs with an unresolved model, likely causing a hard API error.
💡 Suggested fix
- name: Pick experiment variants
id: pick-experiment
continue-on-error: true # fall back to control variant on failure
uses: actions/github-script@...If the step fails, downstream steps that reference steps.pick-experiment.outputs.model_size will emit an empty string. Add a fallback at the activation job output level:
model_size: ${{ steps.pick-experiment.outputs.model_size || vars.GH_AW_MODEL_AGENT_COPILOT || vars.GH_AW_DEFAULT_MODEL_COPILOT || 'claude-sonnet-4.6' }}This restores the original defensive fallback chain that was removed by this PR.
@copilot please address this.
| GH_AW_EFFECTIVE_TOKENS: ${{ needs.agent.outputs.effective_tokens }} | ||
| GH_AW_ENGINE_ID: "copilot" | ||
| GH_AW_ENGINE_MODEL: ${{ needs.agent.outputs.model }} | ||
| GH_AW_ENGINE_MODEL: "${{ needs.activation.outputs.model_size }}" |
There was a problem hiding this comment.
[/grill-with-docs] GH_AW_ENGINE_MODEL is now set to the raw experiment variant name (small or large) instead of a resolved model ID. generate_footer.cjs and messages_footer.cjs both render this value directly in the run footer (model: small appears in PR comments), which will be confusing to authors reading run history.
💡 Suggested fix
The previous value ${{ needs.agent.outputs.model }} returned the actual model identifier resolved during the run (e.g. claude-sonnet-4.6). Consider using the resolved model from agent outputs instead:
GH_AW_ENGINE_MODEL: ${{ needs.agent.outputs.model || needs.activation.outputs.model_size }}This way the footer shows the real model when it is known, falling back to the experiment variant alias only when the agent output is unavailable.
@copilot please address this.
| threshold: ">=0.90" | ||
| min_samples: 70 | ||
| weight: [50, 50] | ||
| start_date: "2026-07-05" |
There was a problem hiding this comment.
[/grill-with-docs] The experiment specifies metric: review_usefulness_acceptance_rate (a 0–1 proportion) but omits analysis_type. Without it, the ab-testing reporting workflow can't automatically select the correct statistical test — proportion_test (two-proportion z-test) is the appropriate choice here and is one of the four valid values in the schema.
💡 Suggested addition
experiments:
model_size:
...
metric: review_usefulness_acceptance_rate
analysis_type: proportion_test # 0–1 bounded rate metric → two-proportion z-testWithout this, any automated analysis reporter defaults to an unspecified test, which may produce misleading significance results for a proportion metric.
@copilot please address this.
| threshold: ">=0.90" | ||
| min_samples: 70 | ||
| weight: [50, 50] | ||
| start_date: "2026-07-05" |
There was a problem hiding this comment.
[/grill-with-docs] No end_date is set. Without a time bound, the experiment runs indefinitely — if statistical significance is never reached (e.g. review acceptance data is sparse), the sentinel agent silently keeps running with 50/50 assignment forever. The ab-testing-advisor guidance recommends setting an explicit end_date to cap experiment duration.
💡 Suggested addition
Based on min_samples: 70 per variant and typical PR review throughput, estimate when 140 samples will accumulate and set that as the bound:
start_date: "2026-07-05"
end_date: "2026-10-05" # 3-month cap; revisit if still underpoweredAfter end_date passes, pick_experiment.cjs will assign all runs to the control variant (small), providing a safe fallback.
@copilot please address this.
Implements the A/B experiment campaign from the ab-testing-advisor for
test-quality-sentinel: testing whether thesmall(baseline) model can matchlargemodel decision quality at lower cost for PR test review tasks.Changes
test-quality-sentinel.mdengine.modelto the activation output:"${{ needs.activation.outputs.model_size }}"experiments.model_sizewith[small, large]variants, 50/50 weight, 70 min samples per variant (calibrated for ≥15 pp MDE at 80% power), and guardrails on false-positive change request rate (≤20%) and run success rate (≥90%)test-quality-sentinel.lock.yml— recompiled from updated source