Repository navigation
Preserve AWF model inventories with null endpoints and live reasoning metadata (G-7) - #70013
Conversation
…data Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
SivaKesava1
added
the
smoke-routing
|
✅ Smoke Copilot Routed Steering completed successfully!
|
github-actions
Bot
removed
the
smoke-routing
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ 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.
|
|
✅ Smoke Copilot Routed Claude completed successfully!
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Smoke Copilot SDK Routed Claude completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Smoke Pi Routed Claude 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 opportunity to reduce test-only complexity.
net: -190 lines possible.
Generated by ✂️ Ponytail Reviewer for #70013 · codex · gpt56 · 12 AIC · ⌖ 6.35 AIC · ⊞ 14.6K
Comment /ponytail to run again
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.
Request changes
This patch ships two blocking contract regressions: the release marker says patch even though the PR intentionally removes the old model.inventory guarantee, and the regenerated schema no longer enforces the positive-safe-integer constraint for contextWindowTokens.
Themes
- release semantics no longer match the documented 2.0.0 / breaking contract change
- published schema diverges from the normalizer and T-UAS-075, so downstream validators can accept payloads the collector rejects
Static review only; I did not run code or tests under this workflow.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 41.3 AIC · ⌖ 6.04 AIC · ⊞ 12.7K
Comment /review to run again
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "gh-aw": patch | |||
There was a problem hiding this comment.
Marking this changeset as patch understates a breaking model.inventory contract change, so downstream consumers can absorb a schema break without any semver warning.
💡 Why this blocks
The PR explicitly removes the old guarantee that every endpoint publishes models and even bumps the spec to 2.0.0. Releasing that under a patch version means automated upgraders and release notes will advertise a non-breaking update while parsers that still require models can fail at runtime.
Suggested fix:
---
"gh-aw": major
---If this repo uses another release channel for breaking changes, use that instead of patch, but the current marker is inconsistent with the change being shipped.
There was a problem hiding this comment.
Done in 4010fc3. I renamed the changeset to .changeset/major-preserve-awf-model-inventory-null-endpoints.md and changed it to "gh-aw": major. The body now says that model.inventory endpoint models/routingModels may be absent, meaning unknown, and that readers must not treat absence as an empty list (unified session specification 2.0.0).
| "modelId": { "type": "string" }, | ||
| "supportedEndpoints": { "type": "array", "items": { "type": "string" } }, | ||
| "supportedReasoningEfforts": { "type": "array", "items": { "type": "string" }, "description": "Identifier-shaped reasoning efforts; absent means unknown; a supplied empty array stays empty." }, | ||
| "contextWindowTokens": { "type": "number", "description": "Positive context window size in tokens; absent means unknown." } |
There was a problem hiding this comment.
contextWindowTokens is documented and implemented as a positive safe integer, but the published JSON schema now accepts any number, so downstream validators will bless payloads this collector rejects.
💡 Why this blocks
The normalizer rejects 0, 1.5, strings, and values above Number.MAX_SAFE_INTEGER, and T-UAS-075 says this field is a positive safe integer. With "type": "number" here, generated validators still accept fractional or oversized values, which reintroduces a contract mismatch between the shipped schema and the code.
Suggested fix:
"contextWindowTokens": {
"type": "integer",
"minimum": 1,
"maximum": 9007199254740991,
"description": "Positive context window size in tokens; absent means unknown."
}There was a problem hiding this comment.
Done in 4010fc3. I added @asType integer, @minimum 1 and @maximum 9007199254740991 to contextWindowTokens in unified_session.d.ts and regenerated with make session-schemas. The schema now emits "type": "integer", "minimum": 1 and "maximum": 9007199254740991, and make check-session-schemas passes. I also added a schema-validation test: it accepts 1 and MAX_SAFE_INTEGER and rejects 0, 1.5 and 9007199254740992. Against the old number schema, that test fails.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs. No actionable issues found — approving.
📋 Verification performed
- Manually traced
normalizeModelInventoryDataagainst ~20 cases (null-vs-empty model/routing inheritance,configuredvalidation, reasoning-effort regex, context-window bounds,providers-map path) — all match the documented semantics. - Verified SHA-256 digests of both recorded AWF fixtures against the README provenance table — exact match.
- Confirmed no secrets in fixtures (
"Bearer"is a scheme name, not a token value). - Confirmed idempotency: the normalizer accepts its own output and survives a JSON round trip on the real 40-model fixture.
- Confirmed the 2.0.0 major spec bump is correctly justified: endpoint
modelsmoves from required to optional, which is a genuine reader-guarantee removal per this repou2019’s breaking-change rules. - Test coverage is strong: acceptance tests, malformed-variant tests, a golden-case non-regression test, and legacy-fixture pinning are all present and consistent with the implementation.
Positive Highlights
- ✅ Careful distinction between “unknown” (omitted) and “empty” ([]) inventories, directly fixing the reported bug (#69754) without weakening validation elsewhere.
- ✅ Byte-exact, SHA-pinned fixtures with clear provenance documentation — excellent reproducibility.
- ✅ Spec (
T-UAS-075), schema, and changelog are all updated in lockstep with the code change.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 130 AIC · ⌖ 13.8 AIC · ⊞ 10.1K
Comment /matt to run again
There was a problem hiding this comment.
🔵 Needs a closer look
The incompatible session-output contract needs maintainer sign-off, with schema constraints and release guidance still unresolved.
1 open issue
What changed in this PR
Preserves AWF model inventories in unified sessions when endpoints report unknown model lists, while retaining live capability metadata.
Changes:
- Omits null lists without discarding valid endpoints.
- Retains configuration, reasoning efforts, and context windows.
- Updates contracts and adds recorded-fixture regression coverage.
| File | Description |
|---|---|
| docs/src/content/docs/specs/unified-agent-session-specification.md | Defines the 2.0.0 inventory contract. |
| docs/public/schemas/unified-session.schema.json | Adds capability fields and optional model lists. |
| actions/setup/js/unified_session.test.cjs | Updates collector acceptance and rejection cases. |
| actions/setup/js/unified_session_payload.test.cjs | Adds projection and recorded-inventory regressions. |
| actions/setup/js/unified_session_payload.cjs | Preserves unknown lists and capability metadata. |
| actions/setup/js/types/unified_session.d.ts | Updates inventory payload declarations. |
| actions/setup/js/test_data/awf_reflect/run-38995410124-awf-reflect.json | Adds the primary recorded reproduction. |
| actions/setup/js/test_data/awf_reflect/run-29812420778-legacy-awf-reflect.json | Adds a legacy compatibility fixture. |
| actions/setup/js/test_data/awf_reflect/README.md | Records fixture provenance and digests. |
| .prettierignore | Protects byte-exact fixture formatting. |
| .changeset/preserve-awf-model-inventory-null-endpoints.md | Adds a release note. |
Review effort: Max
| --- | ||
| "gh-aw": patch | ||
| --- | ||
|
|
||
| Keep AWF model inventories in unified sessions when an unconfigured endpoint reports null `models` or `routing_models`. Null lists are now omitted as unknown instead of becoming empty arrays or rejecting the whole snapshot, and endpoint `configured`, routing `supportedReasoningEfforts`, and `contextWindowTokens` are retained (unified session specification 2.0.0). |
There was a problem hiding this comment.
Updated in 90677d8. The changeset is marked major and now states that omitted model lists are unknown while an explicit [] remains a known-empty inventory, with reader guidance not to treat absence as empty.
There was a problem hiding this comment.
Updated in 90677d8. The changeset is marked major and now states that omitted model lists are unknown while an explicit [] remains a known-empty inventory, with reader guidance not to treat absence as empty.
…chema to positive safe integer Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 4010fc3
|
…awf-model-inventories Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
The unified session collector rejected a whole AWF
/reflectsnapshot whenever any endpoint hadrouting_models: null. Unconfigured endpoints therefore discarded the configured endpoint's live model list, and the collector recordedinvalid_model_inventorywith nomodel.inventoryevent. All six sampled smoke runs show this. When the collector did accept a snapshot, the projection lost information:[];configuredwas dropped;Normalizer (
unified_session_payload.cjs)modelsorrouting_modelsis now omitted, and the rest of the inventory is kept.modelsis accepted only on an endpoint that supplies a booleanconfigured, so the{ provider }-only inputs that were malformed before are still rejected.routing_models: nulldoes not inherit the top-level routing list. Only an absent endpoint field does, as before.configured(bothtrueandfalse);supportedReasoningEfforts: identifier tokens matching^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$;contextWindowTokens: positive safe integers only, and omitted when null.configured, and invalid efforts or context windows. Each still producesinvalid_model_inventoryand no event.sourceandcandidate_metadata_completeare excluded.Recorded run 38995410124 now projects as:
{ "endpoints": [ { "provider": "openai", "configured": false }, // previously: whole snapshot rejected { "provider": "anthropic", "configured": false }, { "provider": "copilot", "configured": true, "models": [/* 40 */], "routingModels": [{ "modelId": "claude-haiku-5.5", "supportedEndpoints": ["/v1/messages", "/chat/completions"], "supportedReasoningEfforts": ["low", "medium", "high", "xhigh", "max"], "contextWindowTokens": 1000000 } /* … 40 */] }, { "provider": "gemini", "configured": false }, { "provider": "vertex", "configured": false } ], "modelsFetchComplete": true }Contract
ModelInventoryDataintypes/unified_session.d.tsis updated, andunified-session.schema.jsonis regenerated. Endpointmodelsis no longer required.modelsis now optional, which removes an existing reader guarantee.session.formatstays at 1.Fixtures and tests
actions/setup/js/test_data/awf_reflect/holds byte-exact, SHA-pinned recorded reflections, with provenance in its README. They are added to.prettierignoresomake fmtcannot rewrite them.run-38995410124-awf-reflect.jsonis the primary fixture from the issue.run-29812420778-legacy-awf-reflect.jsoncomes from an older AWF version withoutrouting_models.unified_session_payload.test.cjsadds the five named acceptance tests, plus malformed-capability variants. The existing null-model expectations that encoded the lossy[]output are updated.Notes for review
/reflectversion (gh-aw-firewall Add OIDC authentication with API key fallback - enabled by default for Claude #2253 onward) suppliesconfigured, so a recorded reflection without it does not exist. The golden source runs are private or expired; run 37886511291 returns 404.routing_models.modelsinstead of reporting[], andconfiguredis retained.expected*.jsondiffs.pkg/clicode consumesmodel.inventoryevents.audit_model_availability.goreads the raw reflection file and already treats a nilmodelsas unknown.awf_reflect_summary.test.cjscan fail when it runs in the same vitest invocation as thecopilot_harnessandawf_reflecttests, because they share/tmp/gh-aw. It passes on its own and does not use the changed code.