Repository navigation
Preserve AWF ranked choices and request/failure metadata in unified sessions - #69848
Conversation
…essions Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
…erve-candidate-rankings Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
SivaKesava1
added
the
smoke-routing
|
✅ Smoke Copilot SDK Routed Claude completed successfully!
|
|
✅ Smoke Copilot Routed Claude completed successfully!
|
github-actions
Bot
removed
the
smoke-routing
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Smoke Pi Routed Claude completed successfully!
|
|
✅ 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.
|
|
✅ Smoke Copilot Routed Steering completed successfully!
|
|
✅ 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.
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
I didn't find a blocking issue in the changed lines.
Notes
Static review only: I checked the projection changes, schema/type updates, and the new regression coverage around AWF model-routing metadata. I did not execute tests or runtime validation in this review.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 51.8 AIC · ⌖ 6.24 AIC · ⊞ 12.7K
Comment /review to run again
There was a problem hiding this comment.
🔵 Needs a closer look
The published session-contract and privacy-boundary changes warrant maintainer sign-off.
1 open issue
What changed in this PR
Preserves AWF routing metadata in unified sessions so downstream readers can inspect candidate rankings and request/failure context.
Changes:
- Adds ranked choices, pool counts, endpoints, telemetry availability, HTTP status, and failure phase.
- Updates contracts and regression coverage while retaining session format 1.
| File | Description |
|---|---|
| docs/src/content/docs/specs/unified-agent-session-specification.md | Documents metadata and privacy rules in version 1.12.0. |
| docs/public/schemas/unified-session.schema.json | Adds routing fields and ranked-choice schema. |
| actions/setup/js/unified_session_payload.test.cjs | Adds recorded-data and compatibility cases. |
| actions/setup/js/unified_session_payload.cjs | Projects metadata and filters malformed values. |
| actions/setup/js/types/unified_session.d.ts | Declares routing fields and ranked-choice type. |
Review effort: Max
| if (Object.hasOwn(data, "unavailable") && !(Array.isArray(data.unavailable) && data.unavailable.every(value => typeof value === "string"))) delete data.unavailable; | ||
| for (const key of ["eligibleChoices", "catalogueOverlap", "selectedEndpoint", "requestedEndpoint", "upstreamEndpoint", "status", "phase"]) { | ||
| if (data[key] !== null && typeof data[key] === "object") delete data[key]; |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design (via pr-triage: new_feature classification). The core projection logic (normalizeRankedChoices, the malformed-scalar-drop loop, the unavailable guard) is well-structured and the PR description's documented behaviors (absence-stays-absent, malformed-scalar-dropped, private-field exclusion) are mostly backed by the 300-line test suite using real golden fixtures — a strong practice.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps on negative paths: the malformed-scalar-drop loop (7 fields), the unavailable type guard, and normalizeRankedChoices' non-array fallback are all documented behaviors in the PR description but have no corresponding test exercising the "drop" branch. Only the happy-path (valid data) and omitted-field cases are currently tested.
- Test/production alias duplication: SOURCE_FIELDS in the test file duplicates EVENT_FIELDS["firewall.model_routing"] from production code; a future alias change could drift silently between the two.
Positive Highlights
- ✅ Idempotence, JSON round-trip, and input-immutability are checked on every projected record via the shared
project()helper — a solid regression-test pattern. - ✅ Real golden fixtures (recorded AWF runs) are used for both selection and request/failure stages, in both snake_case and camelCase, giving good confidence the projection matches actual AWF output shapes.
- ✅ Privacy exclusions (conversation_sha256, detail, method, pathname, and nested candidate payload fields) are explicitly tested with sentinel values and a JSON.stringify scan.
- ✅ Spec (T-UAS-070), schema, and types are updated consistently alongside the implementation.
Note: investigation was static-only — vitest could not run in this sandbox due to a blocked npm registry (self-signed cert error), so findings are based on code and test-file reading rather than executed assertions.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 133.1 AIC · ⌖ 16.2 AIC · ⊞ 10.1K
Comment /matt to run again
| else data.rankedChoices = rankedChoices; | ||
| } | ||
| // AWF lists unobserved request-telemetry dimensions here, not unavailable models. | ||
| if (Object.hasOwn(data, "unavailable") && !(Array.isArray(data.unavailable) && data.unavailable.every(value => typeof value === "string"))) delete data.unavailable; |
There was a problem hiding this comment.
[/tdd] The guard that drops unavailable unless it is an array of strings has no negative-case test. Current coverage only supplies a valid array or omits the field entirely.
💡 Suggested tests
for (const bad of ["not-an-array", { nested: true }, ["model", 42]]) {
const projected = project(routingEvent({ ...SMOKE_REQUEST, unavailable: bad }));
expect(projected.data).not.toHaveProperty("unavailable");
}The mixed-type array case (["model", 42]) is the most likely regression target since .every(value => typeof value === "string") is easy to accidentally weaken (e.g. to .some) without a failing test catching it.
@copilot please address this.
| // AWF lists unobserved request-telemetry dimensions here, not unavailable models. | ||
| if (Object.hasOwn(data, "unavailable") && !(Array.isArray(data.unavailable) && data.unavailable.every(value => typeof value === "string"))) delete data.unavailable; | ||
| for (const key of ["eligibleChoices", "catalogueOverlap", "selectedEndpoint", "requestedEndpoint", "upstreamEndpoint", "status", "phase"]) { | ||
| if (data[key] !== null && typeof data[key] === "object") delete data[key]; |
There was a problem hiding this comment.
[/tdd] The malformed-scalar-drop loop (dropping eligibleChoices, catalogueOverlap, selectedEndpoint, requestedEndpoint, upstreamEndpoint, status, phase when they are objects) is explicitly documented in the PR description ("Malformed scalars dropped"), but no test in unified_session_payload.test.cjs exercises it for any of these 7 fields.
💡 Suggested test
it.each(["eligibleChoices", "catalogueOverlap", "selectedEndpoint", "requestedEndpoint", "upstreamEndpoint", "status", "phase"])("drops %s when AWF supplies an object instead of a scalar", key => {
const snakeKey = SOURCE_FIELDS[key][0];
const record = { ...SMOKE_SELECTION, [snakeKey]: { nested: true } };
const projected = project(routingEvent(record));
expect(projected.data).not.toHaveProperty(key);
});Without this, a future refactor of the drop loop (e.g. an off-by-one in the field list, or an accidental removal) would ship silently, since nothing currently fails if the loop is deleted entirely.
@copilot please address this.
| * @param {any} choices | ||
| * @returns {import("./types/unified_session").ModelRoutingRankedChoice[] | undefined} | ||
| */ | ||
| function normalizeRankedChoices(choices) { |
There was a problem hiding this comment.
[/tdd] normalizeRankedChoices returns undefined for any non-array input (string, number, object), deleting rankedChoices entirely per the caller at line 576. This fallback path is untested; all current test inputs supply either a valid array or omit the field.
Suggested test
Add cases where ranked_choices is a string, a number, or a plain object, and assert the projected data has no rankedChoices property.
Also worth a quick case for a non-string id (e.g. id: 123) on a candidate entry, to confirm it is silently dropped rather than coerced; the current typeof choice.id === string check suggests this is intended but it is not currently verified.
@copilot please address this.
| const GOLDEN_DIR = fileURLToPath(new URL("../../../pkg/cli/testdata/model_routing_golden/", import.meta.url)); | ||
| const ROUTING_LOG = "sandbox/firewall/logs/api-proxy-logs/model-routing.jsonl"; | ||
| const NEW_FIELDS = ["rankedChoices", "eligibleChoices", "catalogueOverlap", "selectedEndpoint", "requestedEndpoint", "upstreamEndpoint", "unavailable", "status", "phase"]; | ||
| const SOURCE_FIELDS = { |
There was a problem hiding this comment.
[/codebase-design] SOURCE_FIELDS (lines 925-933) duplicates the alias mapping already defined in the production EVENT_FIELDS["firewall.model_routing"] object in unified_session_payload.cjs. If an alias is added or renamed in production, this test fixture will silently drift and keep passing against the stale list rather than the real contract.
Suggestion
Export EVENT_FIELDS (or an alias slice of it) from unified_session_payload.cjs for test use, or derive SOURCE_FIELDS from it at test setup time, so the test fails loudly if production aliases change without a matching test update.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
AWF already records the router's ranked candidates, the pool counts, the request endpoints and HTTP status, and the failure phase. The unified session collector dropped all of it, so a reader of
usage/aw_session.jsonlcouldn't see which candidates were ranked or the endpoint/status context for deviations and failures. This PR adds those fields to the existingfirewall.model_routingprojection; no new event or source is added.Projection (
actions/setup/js/unified_session_payload.cjs)Nine fields are added, accepting both snake_case and camelCase:
rankedChoicesranked_choices{rank, id?, model, effort}.rankis the 1-based array position. An explicitnulleffort is kept.idappears only if supplied (current AWF doesn't persist it). Any other property on an entry is dropped.eligibleChoiceseligible_choicescatalogueOverlapcatalogue_overlapselectedEndpointselected_endpointendpointfield is unchanged.requestedEndpoint/upstreamEndpointrequested_endpoint/upstream_endpointunavailableunavailablemodel,effort), not unavailable models. Kept only if it is an array of strings.statusstatusoutcome.phasephaseprimary/bootstrap), distinct fromprovenance.phase.Absent stays absent: no zero, empty array or
nullis added for missing data. A supplied zero,nulleffort or empty array is kept.Malformed scalars dropped: if the counts, endpoints,
statusorphasehold an object, that field is omitted, so nested payloads can't pass through.Private fields still excluded:
conversation_sha256,detail,methodandpathname, including their camelCase spellings.No per-candidate predictions: nothing beyond the ranking, the selected choice and observed results is recorded.
Example from a recorded selection (run 38984582061):
(Truncated; the record has 12 entries in AWF order.)
Types and schema
ModelRoutingRankedChoicetype toModelRoutingDataintypes/unified_session.d.ts.docs/public/schemas/unified-session.schema.jsonis regenerated with the existing generator (npm run schema:session).Specification
session.formatstays 1.Tests (
unified_session_payload.test.cjs)agent/model-routing.jsonl.codex-routed-provider-unavailableandcopilot-budget-exhausted-steeringgolden logs and sessions, read directly.idon a candidate, the new fields omitted, sentinel values in the private fields.Not changed
gh aw auditoutput is unchanged, as the issue decided.make update-model-routing-goldenchanged no fixtures orexpected*.jsonfiles.Known failures that also occur on main
TestSessionParserSourcesIncludeLocalDependenciescopilot_sdk_mcp_config.test.cjswork_queue_control_performance.test.cjscollect_usage_artifact_files_test.sh