Repository navigation
Fix routing smoke assertions: attribute agent traffic by model instead of nonexistent purpose values - #67595
Fix routing smoke assertions: attribute agent traffic by model instead of nonexistent purpose values#67595SivaKesava1 with Copilot wants to merge 3 commits into
purpose values#67595Conversation
… purpose Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
purpose values
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🔍 Design Decision Gate 🏗️ is checking for design decision records on this pull request... |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
SivaKesava1
added
the
smoke-routing
|
✅ Smoke Copilot Routed completed successfully!
|
github-actions
Bot
removed
the
smoke-routing
|
✅ Smoke Pi Routed completed successfully!
|
|
✅ Smoke Copilot SDK Routed completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
One removable speculative attribution layer found.
net: -20 lines possible.
Generated by ✂️ Ponytail Reviewer for #67595 · codex · gpt56 · 12 AIC · ⌖ 5.16 AIC · ⊞ 13.5K
Comment /ponytail to run again
| const startedEvents = subagentEvents.filter(event => event.type === "subagent.started"); | ||
| const observedNames = [...new Set(startedEvents.map(event => event.data?.agentName ?? "<none>"))]; | ||
|
|
||
| // Attribute agent traffic to declared sub-agents: prefer session correlation |
There was a problem hiding this comment.
L235: yagni: speculative request-ID/agent-ID attribution for links the PR says AWF does not emit. Attribute sub-agent traffic by declared model until real correlated records exist.
There was a problem hiding this comment.
Leaving this as is. Issue #67594 asked for the request-ID/agent-ID correlation so that a real correlation from AWF or gh-aw takes precedence over the model fallback as soon as one is recorded. Tests cover it. Until correlated records exist, traffic is attributed by declared model, and C1 now makes that attribution unambiguous (d4d3b24).
There was a problem hiding this comment.
🟡 Changes recommended
Duplicate sub-agent models remain ambiguous and can let one request satisfy multiple S3 checks.
1 open finding
What changed in this PR
Fixes routing smoke assertions to classify real AWF traffic correctly and updates the Pi smoke configuration.
Changes:
- Attributes non-classifier requests by correlation or model.
- Adds real-shape fixtures and negative coverage.
- Restricts Pi routing candidates and recompiles its lock file.
| File | Description |
|---|---|
actions/setup/js/smoke_model_routing_assertions.cjs |
Revises request attribution and checks. |
actions/setup/js/smoke_model_routing_assertions.test.cjs |
Expands realistic assertion coverage. |
.github/workflows/smoke-pi-routed.md |
Restricts routed models to Luna. |
.github/workflows/smoke-pi-routed.lock.yml |
Regenerates the compiled workflow. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if (subAgents.length) { | ||
| const overlapping = subAgents.filter(agent => allowed.has(servedModel(agent.model))); | ||
| if (overlapping.length) |
There was a problem hiding this comment.
Fixed in d4d3b24. C1 now also fails when two declared sub-agents share a normalized model, for example FAIL C1 declared sub-agents share a model: claude-haiku-4.5 (haiku-whoami, mini-whoami); each sub-agent must declare a distinct model. When a config both overlaps allowed-models and has a duplicate model, both problems are reported in the same C1 line. A new test covers duplicates that only match after normalization (claude-haiku-4.5 vs copilot/claude-haiku-4-5-20251001).
There was a problem hiding this comment.
Unable to submit full review body from the sandbox.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 45.8 AIC · ⌖ 5.33 AIC · ⊞ 21.2K
Comment /review to run again
|
Ran the three routing smokes on this PR's code (
On the two open review threads:
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs (root-cause bug fix) and /tdd (test rebuild). The fix correctly diagnoses the root cause — AWF never writes purpose: "agent"/"subagent", only purpose: "routing_classification" for the classifier — and switches to model-based attribution with session-correlation fallback. Comprehensive negative-variant tests were rebuilt from real production fixtures (runs 38097181601/38096990663/38100426674), covering dated model IDs, query-string endpoints, empty-request failure modes, and the new C1 disjointness check.
📋 Key Themes & Highlights
Key Themes
- One actionable correctness gap: model-based attribution (
attributedinsmoke_model_routing_assertions.cjs) doesn't guard against two declared sub-agents sharing the same model — left an inline comment with a concrete fix suggestion (extend C1, or add a regression test documenting the behavior).
Positive Highlights
- ✅ Root cause correctly diagnosed against real AWF record shapes, not just symptom patched
- ✅ Test suite rebuilt against actual production fixtures rather than synthetic approximations, closing the gap that let the original bug ship
- ✅ New C1 check and empty-request failure modes (R3/R4/M1/S3) add real coverage that was previously silently passing on zero requests
- ✅ Doc comment at the top of the file clearly explains the new attribution model for future maintainers
No blocking issues; approving with one actionable suggestion for the author to consider.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 99.8 AIC · ⌖ 14.6 AIC · ⊞ 10.3K
Comment /matt to run again
| }); | ||
| const correlatedRequests = new Set(subAgentEvidence.flatMap(evidence => evidence.correlated)); | ||
| const attributed = subAgentEvidence.map(evidence => (evidence.correlated.length ? evidence.correlated : agentTraffic.filter(request => !correlatedRequests.has(request) && servedModel(request.model) === evidence.model))); | ||
| const subagentRequests = new Set(attributed.flat()); |
There was a problem hiding this comment.
[/diagnosing-bugs] Model-based attribution doesn't guard against two declared sub-agents sharing the same model — both would independently claim the same uncorrelated request via servedModel(request.model) === evidence.model, so each sub-agent's S3 check could pass on a request that actually belongs to the other.
💡 Why this matters and a suggested fix
subAgentEvidence is built per-agent (line 237), and when correlation is empty each entry independently filters agentTraffic by its own declared model (line 247). If two subAgents entries declare the same model, the same proxy record(s) would appear in both attributed[i] and attributed[j], letting S3 "pass" for both even though only one sub-agent actually made that call. C1 only checks sub-agent models against allowedModels, not sub-agent models against each other, so this slips through today.
Current workflows (smoke-pi-routed.md) happen to declare disjoint sub-agent models, so this doesn't manifest yet, but the new attribution logic makes it a latent correctness gap in a check that's meant to be deterministic evidence. Consider either:
- extending C1 to also fail when two declared sub-agents share a model, or
- adding a regression test that exercises duplicate sub-agent models and documents the current (ambiguous) behavior if it's intentionally out of scope.
@copilot please address this.
There was a problem hiding this comment.
Addressed in d4d3b24. C1 now fails when two declared sub-agents share a normalized model, so the same request can no longer count toward two sub-agents' S3 checks. A new regression test covers this.
Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
Important
Before merging, a maintainer must add the
smoke-routinglabel. That runs Smoke Copilot Routed, Smoke Pi Routed and Smoke Copilot SDK Routed on this PR's code. They have not run on it yet.All three routing smokes fail on main in the post-step only. Routing and sub-agent delegation were correct.
smoke_model_routing_assertions.cjscounted only token-usage records withpurpose: "agent"or"subagent", but AWF never writes those values. The onlypurposeAWF records isrouting_classification. As a result R3, M1 and S3 had nothing to match, and R4 passed on zero requests.Request classification (
smoke_model_routing_assertions.cjs)purpose: "routing_classification"is the classifier (R2). Every other record is agent traffic.x_initiatoris only the billing class, so it is not used.subagent.*events or a sub-agent ID on proxy records. Today there is none, so model matching is what runs.allowedModels, because model-based attribution would then be ambiguous.claude-haiku-4-5-20251001becomesclaude-haiku-4.5, and/v1/messages?beta=truebecomes/v1/messages.Records as AWF writes them (from Smoke Pi Routed):
{"model":"gpt-5.6-luna","path":"/responses","status":200,"purpose":"routing_classification","x_initiator":"agent"} {"model":"gpt-5.6-luna","path":"/responses","status":200,"x_initiator":"agent"} {"model":"claude-haiku-4-5-20251001","path":"/v1/messages?beta=true","status":200,"x_initiator":"agent"} {"model":"gpt-5.4-mini-2026-03-17","path":"/responses","status":200,"x_initiator":"agent"}These now resolve to: the classifier (R2), the main agent (R3, R4),
haiku-whoami(S3) andmini-whoami(S3).smoke-pi-routed.mdallowed-modelsand the post-stepallowedModelsare reduced to[gpt-5.6-luna], matchingsmoke-copilot-sdk-routed. The sub-agent models can no longer be routing selections, which satisfies C1. The lock file is recompiled.smoke-copilot-routed.mdandsmoke-copilot-sdk-routed.mdare unchanged.Tests
agentartifact'stoken-usage.jsonland events fromusage/aw_session.jsonl. They include:purposeagentId, with a secondsubagent.completedmarkedcancelled: true. S2 is deliberately not tightened.purposecannot satisfy R3.Scope
Only the assertion helper, its tests and the Pi smoke config change. Routing, harness, AWF configuration and audit behavior are untouched.