feat: add sandbox.agent.default-ai-credits-pricing to fix BYOK Ollama HTTP 400 - #47687
Conversation
… HTTP 400 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot place this field under the frontmatter models field instead of sandbox.agent |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commit. models:
default-ai-credits-pricing:
input: 0
output: 0Extraction is handled by a new |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds fallback AI-credit pricing for unrecognized BYOK models and configures the Ollama workflow with zero-cost pricing.
Changes:
- Parses and emits
apiProxy.defaultAiCreditsPricing. - Adds schema and unit coverage.
- Recompiles the Ollama workflow with zero pricing.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/workflow_data.go |
Stores fallback pricing. |
pkg/workflow/workflow_builder.go |
Extracts pricing from frontmatter. |
pkg/workflow/sandbox.go |
Defines pricing configuration. |
pkg/workflow/frontmatter_extraction_security.go |
Adds numeric conversion. |
pkg/workflow/frontmatter_extraction_security_test.go |
Tests pricing extraction. |
pkg/workflow/awf_config.go |
Emits AWF proxy pricing. |
pkg/workflow/awf_config_test.go |
Tests AWF JSON output. |
pkg/parser/schemas/main_workflow_schema.json |
Adds frontmatter schema field. |
pkg/parser/schema_test.go |
Tests schema validation. |
.github/workflows/daily-byok-ollama-test.md |
Configures zero-cost pricing. |
.github/workflows/daily-byok-ollama-test.lock.yml |
Recompiles generated workflow. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Medium
| "additionalProperties": false | ||
| } | ||
| }, | ||
| "default-ai-credits-pricing": { |
There was a problem hiding this comment.
The models location is intentional — it was moved from sandbox.agent to models per explicit feedback from @pelikhan. The PR description was written before the relocation and is now stale; the implemented API is models.default-ai-credits-pricing.
|
|
||
| // toFloat64 converts any numeric value from a parsed YAML/JSON frontmatter map to float64. | ||
| // Returns (value, true) on success, or (0, false) if the value is nil or not a numeric type. | ||
| func toFloat64(v any) (float64, bool) { |
There was a problem hiding this comment.
Fixed in be24d82 — toFloat64 is now co-located with extractDefaultAiCreditsPricingFromModels in workflow_builder.go.
There was a problem hiding this comment.
The implementation is clean and correct. The new sandbox.agent.default-ai-credits-pricing → apiProxy.defaultAiCreditsPricing pipeline is well-structured end-to-end (schema → frontmatter extraction → WorkflowData → AWF config), with good test coverage for zero, non-zero, absent, and wrong-type cases. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.8 AIC · ⌖ 5.1 AIC · ⊞ 5K
Documents the architectural decision to expose a per-workflow fallback AI credits pricing field for BYOK/self-hosted models unrecognized by the AWF built-in pricing table.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (347 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report
📊 Metrics (14 tests)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /diagnosing-bugs, and /grill-with-docs — commenting with two correctness observations and one documentation consistency issue.
📋 Key Themes & Highlights
Key Themes
- Fragile substring assertions (
awf_config_test.golines 953–954, 978–979):assert.Contains(jsonStr, "\"input\":0")can match any JSON field, not specificallydefaultAiCreditsPricing. Replace with full-key assertions or struct-level comparison. - Silent zero-default on missing keys (
workflow_builder.golines 424–428): ifinputoroutputis absent the extractor silently returns{0, 0}rather thannil. This is inconsistent with the schema (which requires both fields) and hides misconfigured frontmatter. Consider returningnilwhen either key failstoFloat64. - PR description / YAML nesting mismatch (
daily-byok-ollama-test.mdline 96): the field lives undermodels:in the actual workflow and schema, but the PR description sayssandbox.agent. Cosmetic, but will mislead future authors.
Positive Highlights
- ✅ Excellent test coverage across schema validation, extraction, and AWF config round-trip
- ✅
toFloat64helper handles all numeric YAML types cleanly — good defensive coding - ✅ Lock file correctly regenerated with the new
defaultAiCreditsPricingblob - ✅
omitemptyonDefaultAiCreditsPricingensures backward-compatible JSON output
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 49 AIC · ⌖ 4.92 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/awf_config_test.go:953
[/tdd] Fragile substring assertions — Contains(jsonStr, "\"input\":0") could match any other JSON field that contains input:0, giving false confidence.
<details>
<summary>💡 Suggested fix: target the full nested key</summary>
Check for the full object path to avoid accidental matches:
assert.Contains(t, jsonStr, `"defaultAiCreditsPricing":{"input":0,"output":0}`)Or unmarshal into a struct and compare field values directly. The same issue applies to the "input":3 / `"out…
pkg/workflow/workflow_builder.go:490
[/diagnosing-bugs] Silent zero-default when input/output keys are missing — if either key is absent in the YAML, it silently defaults to 0.0 and a non-nil AiCreditsPricingConfig is still returned. A workflow with only input: 3 and no output would pass schema validation (schema requires both) but the extraction would silently use output: 0 without any log or error.
<details>
<summary>💡 Suggested fix: require both keys or return nil</summary>
inputVal, inputOk := toFloa…
</details>
<details><summary>.github/workflows/daily-byok-ollama-test.md:96</summary>
**[/grill-with-docs]** `default-ai-credits-pricing` is nested under `models:` here but lives under `sandbox.agent` in the PR description. The two sources of truth should be reconciled — the schema also places it under `models` (line 113 of the diff), so the PR description appears to be the outlier, but this mismatch will cause confusion for future authors.
@copilot please address this.
</details>There was a problem hiding this comment.
Non-blocking issues found
The feature logic is sound and the test coverage is decent. Three issues worth addressing before this becomes a pattern:
🔍 Issues (all non-blocking)
1. Silent zero-value return for unparseable pricing fields () — medium
If a default-ai-credits-pricing object exists but both fields fail toFloat64 (non-numeric values), the extractor returns &AiCreditsPricingConfig{0, 0} rather than nil, silently activating free-model pricing. Schema validation prevents this at compile time but not at runtime extraction.
2. Missing json: struct tags on AiCreditsPricingConfig (sandbox.go:440-445) — medium
Only yaml: tags are present. If this struct is ever JSON-serialized (e.g., embedded in WorkflowData going to GH_AW_INFO_*), fields will serialize as "Input"/"Output" (capitalized), breaking any downstream consumer expecting lowercase. Cheap fix: add json:"input" and json:"output" now.
3. toFloat64 missing json.Number case (frontmatter_extraction_security.go:337) — low
Latent data-loss bug if any caller ever switches to json.Decoder.UseNumber(). A case json.Number: with .Float64() is a one-liner.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 67.5 AIC · ⌖ 4.97 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/workflow_builder.go:490
Silent non-nil return when both pricing fields fail to parse: if the default-ai-credits-pricing object exists in the frontmatter but both input and output values fail toFloat64 (e.g., are string-typed), extractDefaultAiCreditsPricingFromModels still returns &AiCreditsPricingConfig{Input:0, Output:0} — indistinguishable from an explicit {input:0, output:0} config — silently activating zero-cost pricing.
<details>
<summary>💡 Suggested fix</summary>
Track whether at least one …
pkg/workflow/sandbox.go:444
Missing json struct tags risks capitalized field names in JSON serialization: AiCreditsPricingConfig has yaml: tags but no json: tags, so Go's encoding/json will serialize fields as "Input" and "Output" (capitalized), not "input"/"output".
<details>
<summary>💡 Suggested fix</summary>
Add json tags to match the yaml tags:
type AiCreditsPricingConfig struct {
Input float64 `yaml:"input" json:"input"`
Output float64 `yaml:"output" json:"output"`
}If this s…
pkg/workflow/frontmatter_extraction_security.go:337
toFloat64 does not handle json.Number: if any frontmatter parsing path uses json.Unmarshal with UseNumber() (or json.Decoder.UseNumber()), numeric values arrive as json.Number type — not float64, int, etc. The current switch statement has no case for json.Number, so it will fall through to the default and silently return (0, false), discarding valid pricing values.
<details>
<summary>💡 Suggested fix</summary>
Add a json.Number case:
case json.Number:
f, e…
</details>|
@copilot run pr-finisher skill |
| "type": "object", | ||
| "description": "Fallback per-token pricing ($/1M tokens) for models not in the built-in pricing table. Required when max-ai-credits is active and the model is self-hosted or unrecognized (e.g. BYOK Ollama). Without this, the AWF API proxy rejects unrecognized models with HTTP 400.", | ||
| "required": ["input", "output"], | ||
| "properties": { |
There was a problem hiding this comment.
@copilot reuse ai-credits ref type (or create one)
There was a problem hiding this comment.
Done in be24d82 — added $defs/ai_credits_pricing to the schema and replaced the inline object definition with "$ref": "#/$defs/ai_credits_pricing".
There was a problem hiding this comment.
Fixed in be24d82 — created $defs/ai_credits_pricing with input and output properties and replaced the inline definition with "$ref": "#/$defs/ai_credits_pricing".
|
@copilot please run the
Once addressed, rerun the relevant checks and report back on the PR. Run: https://github.com/github/gh-aw/actions/runs/30068020977
|
… with extractor Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in be24d82. Two changes made:
Local validation passes: |
| "cost": { | ||
| "type": "object", | ||
| "description": "Per-token cost in USD. Keys are token classes; values are numeric cost-per-token strings or numbers.", | ||
| "properties": { |
There was a problem hiding this comment.
@copilot unify ai credits pricing type information with the default ai credits field so that they use the same type in Go and JSON schema
All review feedback has been addressed in commit be24d82:
Local validation results:
PR Description Update Needed: |
|
🎉 This pull request is included in a new release. Release: |
Adds a new glossary entry for the `models.default-ai-credits-pricing` frontmatter field introduced in #47687. This field allows workflow authors to provide fallback per-token pricing for BYOK/self-hosted models not in the AWF built-in pricing table, preventing HTTP 400 errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Self-hosted Ollama models (
qwen2.5:0.5b) aren't in the AWF built-in pricing table. WhenmaxAiCreditsis active (always, via default expression), the API proxy rejects unrecognized models with HTTP 400unknown_model_ai_creditsunlessapiProxy.defaultAiCreditsPricingis set. TheDaily BYOK Ollama Testhad no fallback configured, causing 8+ consecutive daily failures.New compiler feature:
sandbox.agent.default-ai-credits-pricingMaps directly to
apiProxy.defaultAiCreditsPricingin the generated AWF config JSON. Useinput: 0, output: 0for free self-hosted models.Changes
pkg/workflow/sandbox.go—AiCreditsPricingConfigstruct;DefaultAiCreditsPricing *AiCreditsPricingConfigonAgentSandboxConfigpkg/workflow/awf_config.go—AWFDefaultAiCreditsPricingConfigstruct; field onAWFAPIProxyConfig;extractDefaultAiCreditsPricinghelper wired inBuildAWFConfigJSONpkg/workflow/frontmatter_extraction_security.go—toFloat64helper; parsedefault-ai-credits-pricingfromsandbox.agentpkg/parser/schemas/main_workflow_schema.json—default-ai-credits-pricingproperty undersandbox.agentwithrequired: [input, output]daily-byok-ollama-test.md— sets{input: 0, output: 0}since the Ollama model is free/local;.lock.ymlrecompiled with"defaultAiCreditsPricing":{"input":0,"output":0}in the AWF config blobqwen2.5:0.5bhas no AI-credits pricing; api-proxy returns 400 [Content truncated due to length] #47684