Add model policy frontmatter + import unioning + env policy overrides#41824
Add model policy frontmatter + import unioning + env policy overrides#41824Copilot wants to merge 28 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds end-to-end “model policy” support to gh-aw workflows, allowing authors (and centralized operators via env vars) to control which models are permitted/blocked, and ensuring policies compose safely across imported workflows before being emitted into the generated AWF config.
Changes:
- Extended workflow frontmatter
modelsto supportallowed,disallowed, andblockedpolicy lists (alongside optional pricing providers). - Propagated model policy through import extraction and merged policies across imports + main workflow using union semantics.
- Emitted effective model policy to AWF config (
apiProxy.allowedModels/apiProxy.disallowedModels) with env override precedence.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_builder.go | Extracts main workflow model policy and unions it with imported policy sets into WorkflowData. |
| pkg/workflow/workflow_builder_model_policy_test.go | Adds unit tests for policy extraction and union merge behavior. |
| pkg/workflow/model_aliases_test.go | Verifies frontmatter parsing populates parsed model policy lists. |
| pkg/workflow/frontmatter_types.go | Adds parsed frontmatter fields for model policy lists. |
| pkg/workflow/frontmatter_parsing.go | Parses model policy lists from raw frontmatter into typed config. |
| pkg/workflow/compilerenv/manager.go | Adds env-driven policy overrides for allowed/blocked model sets. |
| pkg/workflow/compilerenv/manager_test.go | Tests env override parsing and “unset” behavior. |
| pkg/workflow/compiler_types.go | Plumbs merged model policy into WorkflowData. |
| pkg/workflow/awf_config.go | Maps effective model policy (with env precedence) into AWF apiProxy config. |
| pkg/workflow/awf_config_test.go | Tests AWF config emission and env override precedence. |
| pkg/parser/schemas/main_workflow_schema.json | Updates schema for models to include policy fields and make providers optional. |
| pkg/parser/import_processor.go | Extends ImportsResult to carry extracted model policy sets. |
| pkg/parser/import_field_extractor.go | Extracts model policy from imported workflows and avoids treating policy keys as aliases. |
| pkg/parser/import_field_extractor_test.go | Adds tests ensuring model policy is extracted (and not misinterpreted as aliases) and can coexist with model costs. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/parser/import_field_extractor.go:639
- When an imported workflow has
models.providersplus model policy keys (allowed/disallowed/blocked), this appends the entirerawModelsobject intoacc.modelCosts. That will later flow intoWorkflowData.ModelCostsand into GH_AW_INFO_MODEL_COSTS, leaking policy keys into a payload that is expected to match the models.json pricing structure (providers-only). This can break downstream cost merging/parsing.
if _, hasProviders := rawModels["providers"]; hasProviders {
acc.modelCosts = append(acc.modelCosts, rawModels)
if providers, ok := rawModels["providers"].(map[string]any); ok {
parserLog.Printf("Extracted model costs from import: providers=%d", len(providers))
} else {
pkg/workflow/workflow_builder.go:166
- Now that
modelsfrontmatter can contain policy keys (allowed/disallowed/blocked) withoutproviders,toolsResult.parsedFrontmatter.ModelCostsmay be non-empty even when there is no pricing data (because it unmarshals the wholemodelsobject).extractMainModelCostsOverlaycurrently returns that map as a cost overlay, which can cause policy-onlymodelsto be emitted as GH_AW_INFO_MODEL_COSTS and/or pollute the providers overlay with policy keys.
}
return workflowData
}
- Files reviewed: 14/14 changed files
- Comments generated: 0
- Review effort level: Low
github-actions
Bot
added
enhancement
🤖 PR Triage
Score breakdown: Impact 35 · Urgency 5 · Quality 10 New model policy frontmatter controls (
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hey The PR is well-structured:
This looks ready for review. 🚀
|
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
/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.
Skills-Based Review 🧠
Applied /tdd, /grill-with-docs, and /improve-codebase-architecture — requesting changes on test coverage gaps and a correctness concern.
📋 Key Themes & Highlights
Key Themes
- Asymmetric wildcard behaviour (correctness):
modelConflictsWithDisallowedPolicyonly applies pattern matching when the disallowed side has wildcards — wildcard entries in allowed are never expanded. The behaviour is non-obvious and untested. - Silent drops without warnings: two paths silently discard data — (a)
parseModelPolicyListignores non-string frontmatter entries without a warning (unlike the import path which warns), and (b)appendModelsFielddrops model aliases whenprovidersis present. - Test fragility: index-based warning assertions (
acc.warnings[0]) panic on re-ordering instead of surfacing a clear failure message. - Missing maintainability comments: two functions whose logic is non-obvious (
resolveModelPolicyForAWFConfig,extractMainModelPolicyOverlay) lack the inline comment that the equivalent cost-overlay functions already carry.
Positive Highlights
- ✅ Solid three-layer policy design (frontmatter → import union → env override) with clean precedence semantics
- ✅ Good test coverage for the core merge/conflict/env-override paths
- ✅ Deterministic output via
sort.Stringson both result slices - ✅ Policy keys correctly excluded from cost overlays in both import and main paths
- ✅
smoke-claude.mdused as a live integration smoke test for the new feature
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 141.4 AIC · ⌖ 12.3 AIC · ⊞ 6.6K
Comment /matt to run again
|
@copilot please run the Unresolved review feedback includes the asymmetric wildcard-policy correctness gap, missing test coverage, warning-handling cleanup, and the env-override/commentary follow-ups.
|
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>
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>
Addressed in Fixed and covered:
Local validation passed: |
🤖 PR Triage — §28342769269
Score breakdown: Impact 42 · Urgency 18 · Quality 9 Rationale: Model policy enforcement (allowed/blocked model lists, frontmatter schema, env overrides). ~48h old. Test quality APPROVED (82/100). CHANGES_REQUESTED citing 3 correctness bugs: (1) allowed+blocked conflict handling, (2) policy key leakage into cost data, (3) silent parse failure. CI pending.
|
|
@copilot please run the Unresolved review feedback:
|
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>
Done in |
🤖 PR Triage — §28357644191
|
🤖 PR Triage — §28376613466
Score breakdown: Impact 35 + Urgency 12 + Quality 4 Rationale: Significant feature (model policy frontmatter + import unioning + env overrides), but blocked by Batch: Groups with #42235 and #42226 (all have
|
|
@copilot please run the Unresolved review:
|
…er-models-fields # Conflicts: # .github/extensions/agentic-workflows-dashboard/extension.mjs # .github/workflows/smoke-claude.lock.yml # pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden # pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden # pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden # pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden # pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden # pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden # pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden # pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden # pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in
The branch is current and locally validated. CI on this new HEAD still needs a maintainer-triggered rerun. |
This change introduces model policy controls in workflow frontmatter (
models.allowed,models.disallowed) and maps them to AWF’sallowedModels/disallowedModelsconfig. It also makes policy behavior import-safe by unioning model sets across composed workflows, with centralized environment overrides taking precedence.Frontmatter + schema support
modelsfrontmatter schema to support policy fields alongside optional pricing providers.models.allowedmodels.disallowedImport compatibility (union semantics)
disallowedtakes precedence when the same model appears in both allowed and disallowed sets.Cost data cleanliness + parse warnings
models.providersobjects.models.allowed/models.disallowed/models.providersshapes and invalid entries, while safely skipping bad values.Centralized policy overrides
GHAW_POLICY_MODELS_ALLOWEDGHAW_POLICY_MODELS_DISALLOWEDAWF config mapping
apiProxy.allowedModelsapiProxy.disallowedModelspr-sous-chef https://github.com/github/gh-aw/actions/runs/28333890571
pr-sous-chef: requested branch update via https://github.com/github/gh-aw/actions/runs/28336535351
pr-sous-chef: refresh branch for workflow run 28338043309
pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/28381497646