fix: full pricing snippet and suppress engine error on missing model pricing - #48594
Conversation
…rror Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot ensure the snippet shown in the message has correct yaml syntax |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The model name key was indented at 10 spaces — same level as models:
providers:
anthropic:
models:
claude-3-5-sonnet:
cost:
input: "0e0" # $0.00 per million input tokens
output: "0e0" # $0.00 per million output tokens
cache_read: "0e0" # $0.00 per million cache-read tokens
cache_write: "0e0" # $0.00 per million cache-write tokens |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (default_business_additions=0, file_count=2). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Improves missing-model-pricing failure reports.
Changes:
- Adds all four pricing fields to fallback YAML.
- Suppresses redundant engine errors.
- Extends regression tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/handle_agent_failure.cjs |
Updates pricing snippets and error suppression. |
actions/setup/js/handle_agent_failure.test.cjs |
Tests the updated behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| */ | ||
| function shouldBuildEngineFailureContext(agentConclusion, hasToolDenialsExceeded, isTimedOut) { | ||
| return agentConclusion === "failure" && !hasToolDenialsExceeded && !isTimedOut; | ||
| function shouldBuildEngineFailureContext(agentConclusion, hasToolDenialsExceeded, isTimedOut, hasMissingModelPricingError) { |
There was a problem hiding this comment.
Fixed in 45a7c8c by updating the helper JSDoc to include @param {boolean} hasMissingModelPricingError so the signature and contract are aligned.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes look correct and well-tested.
shouldBuildEngineFailureContextcorrectly suppresses the redundant engine block when pricing is the root cause, with the new parameter wired in both call sites.buildManualModelPricingFrontmatterSnippetnow emits all four cost fields — the YAML indentation fix for${quotedModelName}is also a valid correctness improvement.- Tests added for both behaviours.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.6 AIC · ⌖ 5.19 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review
Applied /diagnosing-bugs and /tdd — this is a clean, well-scoped bug fix with good test coverage. No blocking issues found.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 23.5 AIC · ⌖ 4.52 AIC · ⊞ 6.7K
Comment /matt to run again
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (2 tests)
Verdict
|
There was a problem hiding this comment.
Small, correct fix; no blocking issues found — two minor non-blocking notes left inline.
Themes
- The
shouldBuildEngineFailureContextsuppression logic andbuildManualModelPricingFrontmatterSnippetcache-field addition are both logically sound and covered by new tests. - Left two low/medium-severity comments: (1) manual pricing fallback presents cache_read/cache_write as confirmed $0 rather than a placeholder, and (2) the new boolean parameter lacks an explicit default, relying on implicit undefined-to-falsy coercion.
- Neither issue is a correctness bug in the current call sites; both are hardening suggestions.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 51.4 AIC · ⌖ 4.39 AIC · ⊞ 7.4K
Comment /review to run again
| */ | ||
| function buildManualModelPricingFrontmatterSnippet(modelName, engineId) { | ||
| return buildModelPricingFrontmatterSnippet(modelName, engineId, { input: 0, output: 0 }); | ||
| return buildModelPricingFrontmatterSnippet(modelName, engineId, { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }); |
There was a problem hiding this comment.
Manual pricing fallback now always shows cache_read/cache_write at $0.00, which could mislead users into thinking $0 is a confirmed rate rather than a placeholder for engines/models that do not support prompt caching at all.
💡 Details
All four fields are now hardcoded as { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }. For engines/models with no caching support, presenting cache_read: "0e0" # $0.00 per million cache-read tokens reads as an authoritative confirmed price rather than a fill-in placeholder. Consider a comment noting these are placeholders, or omitting cache fields when the engine is known not to support caching.
There was a problem hiding this comment.
Addressed in 45a7c8c by marking the manual zero-value pricing block as placeholders in the generated YAML snippet, so the values are clearly guidance to replace.
| */ | ||
| function shouldBuildEngineFailureContext(agentConclusion, hasToolDenialsExceeded, isTimedOut) { | ||
| return agentConclusion === "failure" && !hasToolDenialsExceeded && !isTimedOut; | ||
| function shouldBuildEngineFailureContext(agentConclusion, hasToolDenialsExceeded, isTimedOut, hasMissingModelPricingError) { |
There was a problem hiding this comment.
The new hasMissingModelPricingError parameter has no default value, so it silently relies on JS undefined-to-falsy coercion for any caller that omits it.
💡 Details
shouldBuildEngineFailureContext(agentConclusion, hasToolDenialsExceeded, isTimedOut, hasMissingModelPricingError) works today only because both call sites and the pre-existing 3-arg unit tests happen to pass undefined which coerces to falsy. This is accidentally-correct rather than defensively-correct: a future refactor (param reordering, or inserting a 5th flag before this one) could silently change behavior with no type error. Suggest hasMissingModelPricingError = false as an explicit default.
There was a problem hiding this comment.
Addressed in 45a7c8c by defaulting hasMissingModelPricingError = false in shouldBuildEngineFailureContext so omitted arguments are handled explicitly.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit 72b5398. I addressed the reported typecheck failures by updating the safe output validator JSDoc type definition for data fields and adjusting the schema variable initialization in safe output handlers; local typecheck and targeted tests now pass. |
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
When a model has no pricing configured, the failure issue showed an incomplete YAML snippet (missing cache fields) and a noisy engine error block that restated the same root cause.
Changes
Full pricing snippet:
buildManualModelPricingFrontmatterSnippetnow emits all four cost fields so users can fill in actual values without guessing the schema:Engine error suppressed:
shouldBuildEngineFailureContextnow returnsfalsewhenhasMissingModelPricingErroris true. The engine terminates because of the HTTP 400 pricing rejection — the raw stdio dump adds no actionable signal on top of the pricing remediation block.