Repository navigation
Surface model-routing decisions in logs, audits, and unified sessions - #66645
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@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.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two small opportunities to keep the routing reporting surface lean.
net: -20 lines possible.
Generated by ✂️ Ponytail Reviewer for #66645 · codex · gpt56 · 19.4 AIC · ⌖ 7.32 AIC · ⊞ 13.4K
Comment /ponytail to run again
| } | ||
| writeModelRoutingReportLine(w, "\n") | ||
| } | ||
|
|
There was a problem hiding this comment.
L68: delete: best-effort report-write wrapper used only by these renderers. Call fmt.Fprintf directly like the neighboring renderers; removes the helper and logging indirection.
There was a problem hiding this comment.
I kept the wrapper: it preserves best-effort rendering while logging write failures. Removing it in favor of unchecked fmt.Fprintf calls would lose that diagnostic; this is unrelated to the routing correctness fixes.
| if len(choices) == 3 { | ||
| break | ||
| } | ||
| var choice map[string]any |
There was a problem hiding this comment.
L278: yagni: map[string]any plus alternate-key probing for one versioned ranked-choice shape. Unmarshal []ModelRoutingChoice directly and slice the first three; removes the alias helper and dynamic decoding.
There was a problem hiding this comment.
I kept the alternate-key normalization in topModelRoutingChoices; it preserves accepted field aliases across routing records while still limiting output to the top three choices. Dropping it is not needed for the correctness fixes.
🏗️ Design Decision Gate: ADR RequiredThis PR triggered ADR enforcement because it adds 883 lines to business-logic directories ( A draft ADR has been committed to this branch: Decision captured
Evidence used
Your next actionReview the draft ADR — especially the Alternatives Considered and Negative Consequences sections — correct anything that misstates the intent, fill 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.
|
There was a problem hiding this comment.
Verdict
The new routing surfaces are not reliable yet: this patch turns missing routing artifacts into real output, and it also charges unmatched usage into the deviated bucket.
Blocking themes
- Missing
model-routing.jsonlis serialized/rendered as a realmodel_routingsection instead of being omitted, which is a backwards-incompatible behavior change for non-routed and historical runs. - Cost attribution treats unmatched request IDs as deviations, so the selected/deviated split and deviation share can be wrong whenever routing logs are partial or absent.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 53.7 AIC · ⌖ 5.76 AIC · ⊞ 19.6K
Comment /review to run again
| func analyzeModelRouting(runDir string) *ModelRoutingSummary { | ||
| path := findModelRoutingFile(runDir) | ||
| if path == "" { | ||
| return &ModelRoutingSummary{Status: "not_routed"} |
There was a problem hiding this comment.
Returning a non-nil summary when model-routing.jsonl is absent makes every legacy/non-routed run emit a fake model_routing section/JSON block with zeroed costs, which is a breaking output regression for gh aw audit and gh aw logs consumers.
💡 Why this blocks and how to fix it
renderConsoleModelRouting, runData.ModelRouting, and AuditData.ModelRouting all key off nil to decide whether routing data exists. After this change, a run that never produced routing logs now serializes as {"status":"not_routed", ...} and prints model_routing: status=not_routed even though there was no routing artifact at all. That changes the output shape for every historical/non-routed run and makes it impossible for downstream tooling to distinguish “no routing data” from “a routing attempt explicitly concluded not_routed”.
A safer pattern is to return nil when the file is missing and reserve Status: "not_routed" for cases where a routing file actually exists but its records indicate that outcome.
There was a problem hiding this comment.
Fixed in e51d334: a missing routing artifact now returns nil, so not_routed is only reported when a routing artifact exists and records that outcome.
| } | ||
|
|
||
| for _, entry := range usageEntries { | ||
| bucket := &summary.DeviatedTrafficCost |
There was a problem hiding this comment.
Defaulting every non-classifier token-usage entry whose request_id is missing from requestKinds into DeviatedTrafficCost will over-report deviation rate and deviation AIC whenever the routing log is partial or a request was never observed.
💡 Why this blocks and how to fix it
This loop currently treats three very different cases as the same bucket: real deviations, unmatched requests from partial/missing routing logs, and any other inference usage entry whose request never appeared in model-routing.jsonl. The result is that the new selected-vs-deviated split is not trustworthy: a partial artifact or older AWF layout will silently inflate deviated_requests, deviated_traffic_share, and deviated cost.
Please only attribute selected/deviated traffic when there is an explicit request_id match, and either skip unmatched usage or track it as a separate unobserved/unattributed bucket.
There was a problem hiding this comment.
Fixed in e51d334: cost attribution now accepts only explicit as_selected and deviated request records; unmatched and unobserved usage rows are skipped. Added bucket-count and token-metric assertions.
There was a problem hiding this comment.
🟡 Changes recommended
Default logs omit routing data, while attribution and recommendation logic can misreport deviations and priorities.
3 open findings
What changed in this PR
Adds model-routing visibility across audits, logs, cost attribution, artifacts, and unified sessions.
Changes:
- Parses routing decisions and attributes classifier, selected-model, and deviated costs.
- Adds per-run comparisons and cross-run routing aggregates.
- Extends artifacts, schemas, unified sessions, tests, and documentation.
| File | Description |
|---|---|
schemas/logs.schema.json |
Adds routing fields to logs schema. |
schemas/logs-jsonl.schema.json |
Adds routing fields to JSONL schema. |
schemas/audit.schema.json |
Adds routing audit and comparison schema. |
pkg/workflow/compiler_yaml_artifacts.go |
Preserves routing logs in fallback artifacts. |
pkg/workflow/compiler_artifacts_test.go |
Tests fallback routing paths. |
pkg/cli/token_usage_types.go |
Captures token-usage purpose. |
pkg/cli/model_routing.go |
Parses and aggregates routing data. |
pkg/cli/model_routing_test.go |
Tests routing analysis and rendering. |
pkg/cli/logs_run_processor.go |
Attaches routing analysis to runs. |
pkg/cli/logs_report.go |
Defines routing report models. |
pkg/cli/logs_orchestrator_render.go |
Adds routing to cross-run reports. |
pkg/cli/logs_models.go |
Persists routing in run models. |
pkg/cli/audit.go |
Adds routing analysis state. |
pkg/cli/audit_summary_build.go |
Propagates routing into summaries. |
pkg/cli/audit_run_pipeline.go |
Restores routing from summaries. |
pkg/cli/audit_report.go |
Exposes routing in audits. |
pkg/cli/audit_report_render.go |
Renders routing details and changes. |
pkg/cli/audit_cross_run.go |
Adds cross-run routing data. |
pkg/cli/audit_cross_run_render.go |
Renders routing aggregates. |
pkg/cli/audit_comparison.go |
Compares selected routes. |
pkg/cli/audit_analysis_fanout.go |
Runs routing analysis during audits. |
docs/src/content/docs/reference/model-routing.md |
Documents routing reports. |
docs/src/content/docs/reference/cost-management.md |
Documents routing cost attribution. |
docs/src/content/docs/reference/audit.md |
Documents audit routing output. |
docs/src/content/docs/reference/artifacts.md |
Documents routing artifact locations. |
docs/public/schemas/unified-session.schema.json |
Adds routing session events. |
actions/setup/js/unified_session.test.cjs |
Tests routing event collection. |
actions/setup/js/unified_session.cjs |
Normalizes routing events. |
actions/setup/js/unified_session_render.cjs |
Renders routing events. |
actions/setup/js/types/unified_session.d.ts |
Types routing session payloads. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if delta.ModelRouting != nil && delta.ModelRouting.Changed { | ||
| return "Review the model-routing change against the selected successful baseline, including the selected model, effort, mode, and router version." | ||
| } |
There was a problem hiding this comment.
Fixed in e51d334: route recommendations now follow the posture, MCP-failure, and blocked-request checks. Added combined-change priority tests.
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to analyze token usage for run %d: %v", result.Run.DatabaseID, tokenErr))) | ||
| } | ||
| result.TokenUsage = tokenUsage | ||
| result.ModelRouting = analyzeModelRouting(runOutputDir) |
There was a problem hiding this comment.
Fixed in e51d334: the collector stages routing records at usage/agent/model-routing.jsonl and the compact usage artifact uploads that path. Added coverage for analyzing routing and costs from the usage-only artifact.
| if request.Routed == "as_selected" { | ||
| requestKinds[request.RequestID] = "selected" | ||
| } else { | ||
| requestKinds[request.RequestID] = "deviated" | ||
| } | ||
| } | ||
|
|
||
| for _, entry := range usageEntries { | ||
| bucket := &summary.DeviatedTrafficCost | ||
| switch { | ||
| case entry.Purpose == "routing_classification": | ||
| bucket = &summary.ClassifierCost | ||
| case requestKinds[entry.RequestID] == "selected": | ||
| bucket = &summary.SelectedModelCost | ||
| } | ||
| bucket.Requests++ |
There was a problem hiding this comment.
Fixed in e51d334: only explicit selected/deviated statuses enter request attribution; unobserved, empty, unknown, and unmatched usage are excluded from deviation costs.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd to the new routing-aware audit/logs surface. Overall the feature is well-tested (model_routing_test.go covers selection parsing, legacy normalization, comparison deltas, and cross-run aggregation) and fits the existing audit/logs package structure cleanly — approving with a few non-blocking suggestions.
📋 Key Themes & Highlights
Key Themes
- Typed interface inconsistency:
ModelRoutingDatainunified_session.d.tsuses an open index signature while every sibling event-data interface enumerates explicit fields — worth aligning for consistency and discoverability. - Partial cost-attribution test coverage: token/request counts in
ModelRoutingCostbuckets aren't asserted, only.AIC. - A fallback re-parse path in
buildAuditComparisonCandidateFromSummaryre-reads the routing JSONL from disk whensummary.ModelRoutingis nil — minor duplication risk worth a comment or test to confirm it's rarely/never hit. - Minor dead-code nit in
isAWFBefore02839's negative-major check.
Positive Highlights
- ✅ Cost attribution correctly joins routing requests to token usage by
request_id, with clear separation of classifier/selected/deviated buckets. - ✅ Legacy endpoint-only deviation normalization (pre-v0.28.39) is explicit, tested, and surfaced to users via
EndpointOnlyDeviationNormalized. - ✅ Good reuse of existing conventions (
safeModelRoutingTextcontrol-char stripping,findTokenUsageFile-style multi-path file discovery,console:struct tags for structured output).
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 124.3 AIC · ⌖ 14.9 AIC · ⊞ 10.1K
Comment /matt to run again
| requestId?: JsonValue; | ||
| } | ||
|
|
||
| export interface ModelRoutingData { |
There was a problem hiding this comment.
[/codebase-design] ModelRoutingData uses an open index signature ([key: string]: JsonValue) while every sibling event-data interface (FirewallAccessData, UsageReportData, SafeOutputData, ...) enumerates explicit fields. This breaks the deep-module contract the other types establish and loses compile-time safety/autocomplete for consumers of firewall.model_routing events.
💡 Suggested fix
Enumerate the fields actually produced by the Go-side modelRoutingRecord/ModelRoutingSummary (e.g. stage, routed, outcome, selected_model, selected_effort, router, request_id, deviations), matching the convention used by UsageReportData:
export interface ModelRoutingData {
stage?: JsonValue;
routed?: JsonValue;
outcome?: JsonValue;
selected_model?: JsonValue;
selected_effort?: JsonValue;
router?: JsonValue;
request_id?: JsonValue;
}This keeps the type consistent with its neighbors and documents the shape for anyone reading the unified-session schema.
@copilot please address this.
There was a problem hiding this comment.
Fixed in e51d334: ModelRoutingData now enumerates the emitted snake_case fields, and the unified-session schema was regenerated.
| Objective struct { | ||
| Goal string `json:"goal"` | ||
| Mode string `json:"mode"` | ||
| } `json:"objective"` |
There was a problem hiding this comment.
[/tdd] ModelRoutingCost.Requests, InputTokens, OutputTokens, CacheReadTokens, and CacheWriteTokens are accumulated in aggregateModelRoutingCosts but none of them are asserted in TestParseModelRoutingSelectionAndCostAttribution — only .AIC is checked. A regression that mis-buckets token counts (e.g. swapping InputTokens/OutputTokens) would pass CI silently.
💡 Suggested test addition
if summary.SelectedModelCost.Requests != 1 || summary.SelectedModelCost.InputTokens != 100 {
t.Fatalf("selected model token/request counts not attributed: %+v", summary.SelectedModelCost)
}Add similar assertions for classifier and deviated buckets so the cost-attribution logic is fully covered, not just the AIC rollup.
@copilot please address this.
There was a problem hiding this comment.
Fixed in e51d334: added request and input/output/cache token assertions for classifier, selected-model, and deviated cost buckets.
| }, nil | ||
| } | ||
|
|
||
| func buildAuditComparisonCandidateFromSummary(summary *RunSummary, logsPath string) auditComparisonCandidate { |
There was a problem hiding this comment.
[/codebase-design] buildAuditComparisonCandidateFromSummary re-parses the routing JSONL from disk via analyzeModelRouting(logsPath) whenever summary.ModelRouting is nil, but every other field in this function (createdItems, posture, blockedRequests) is read directly from the already-loaded summary/manifest. This re-introduces a disk I/O path (and a second place that must stay in sync with RunSummary.ModelRouting) purely as a fallback.
💡 Why this matters
If RunSummary.ModelRouting is ever renamed or its population logic changes, this silent fallback could mask a real data-loss bug by quietly re-deriving routing from the raw file instead of failing loudly. Consider logging when the fallback path is taken, or confirm (with a test) that summary.ModelRouting is always populated by the time this function runs, making the fallback dead code.
@copilot please address this.
There was a problem hiding this comment.
Kept the fallback for cached summaries created before the routing field existed, documented that compatibility case, and added a test proving routing is recovered from the retained artifact.
| }) | ||
| } | ||
| return choices | ||
| } |
There was a problem hiding this comment.
[/codebase-design] isAWFBefore02839 checks major < 0, but %d in fmt.Sscanf never produces a negative value for this unsigned-looking version string (no leading - is possible from TrimPrefix("v") output in practice), so that branch is effectively dead/confusing. Minor, but worth simplifying to major == 0 && (minor < 28 || ...) for clarity, since a major >= 1 schema should never be treated as legacy regardless of sign.
@copilot please address this.
There was a problem hiding this comment.
Fixed in e51d334 by simplifying the legacy-version predicate to the supported major-zero range.
There was a problem hiding this comment.
Reviewed with the Impeccable audit/harden lenses (backend/CLI feature touching audit-report rendering, console/markdown formatting, and cost-attribution edge states — new feature + hardening of error/edge states across model_routing.go, audit/logs rendering, and unified-session ingestion).
Summary
The model-routing surfacing is comprehensive and well-tested: parsing, legacy endpoint-deviation normalization, cost attribution, console/markdown rendering, cross-run aggregation, and unified-session events all have matching unit tests, and go test ./pkg/cli/... passes locally for the new and existing comparison tests. Text output is consistently sanitized via safeModelRoutingText/modelRoutingMarkdownCell, and generated artifact paths/schemas are updated consistently.
Blocking issue
I found one precedence/logic bug in recommendAuditComparisonAction (see inline comment) where the new model-routing check is checked before the existing posture/MCP-failure/blocked-request checks, so it can silently suppress a more important recommendation (e.g. a first-time read_only→write_capable posture change) whenever routing also changed. The existing test suite has dedicated "priority order" sub-tests for the pre-existing checks, but none cover this interaction, so it wasn't caught. Recommend COMMENT-level visibility since it's a reporting/recommendation-text regression rather than a security hole, but it should be fixed before merge since it can hide genuinely risky behavior changes from auditors.
No other blocking issues found in the diff.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 172.8 AIC · ⌖ 13.3 AIC · ⊞ 8.1K
| } | ||
| if delta.ModelRouting != nil && delta.ModelRouting.Changed { | ||
| return "Review the model-routing change against the selected successful baseline, including the selected model, effort, mode, and router version." | ||
| } |
There was a problem hiding this comment.
Priority regression in recommendAuditComparisonAction: the new model-routing check is inserted before the existing posture / MCP-failure / blocked-request checks, so it silently overrides those higher-priority recommendations whenever a model-routing change also occurs alongside them.
For example, a run that goes read_only → write_capable and has a routing change will now report only the model-routing message, hiding the write-capable-posture warning that this function previously surfaced first. I verified this with a quick ad-hoc test: recommendAuditComparisonAction("risky", "success", &AuditComparisonDelta{Posture: {Before: "read_only", After: "write_capable"}, ModelRouting: {Changed: true}}) returns only "Review the model-routing change...".
The existing TestRecommendAuditComparisonAction suite has explicit "priority order" sub-tests (e.g. "posture change wins over MCP failure") documenting the intended precedence, but none cover the new ModelRouting branch's interaction with those checks, so this slipped through.
Suggested fix: move the delta.ModelRouting check after the posture/MCP-failure/blocked-requests checks (or append it as a secondary note rather than an early return), and add a priority-order test asserting posture/MCP-failure changes still win over a routing change.
@copilot please address this.
There was a problem hiding this comment.
Fixed in e51d334: the route-specific recommendation follows posture, MCP-failure, and blocked-request priorities, with tests covering those simultaneous changes.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The three findings are addressed in e51d334: absent routing data is omitted, routing is collected into the compact usage artifact, and only matched explicit outcomes receive traffic costs.
I left both cleanup suggestions unchanged: the routing renderer wrapper logs best-effort write failures, and alternate-key handling preserves accepted ranked-choice fields. Neither affects the requested correctness fixes. |
|
🎉 This pull request is included in a new release. Release: |
AWF records routing classifications, selections, and per-request deviations, but
gh aw auditandgh aw logsdid not expose those decisions or distinguish classifier cost from agent traffic. This adds route-aware audit summaries and comparisons, cross-run route aggregates, and unified-session support.request_id; separate classifier, selected-model, and deviated-traffic costs. Flag changes in model, effort, mode, or router version. For AWF versions before v0.28.39, count endpoint-only deviations as selected-model traffic and note the normalization.firewall.model_routingevents in unified sessions and add routing logs to the fallback artifact.Example audit output:
gh aw logsandgh aw audit#66638