Uh oh!
There was an error while loading. Please reload this page.
Fix allocation-size overflow risk in model pricing merge - #43468
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43468 does not have the 'implementation' label and has only 51 new lines of code in business logic directories (threshold is 100). |
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This pull request addresses a CodeQL-reported allocation-size overflow risk in the workflow compiler’s model pricing merge by removing overflow-prone map pre-sizing and adding a regression test to prevent reintroduction of capacity arithmetic.
Changes:
- Stop pre-sizing the top-level
resultmap inmergeModelPricingIntoModelCoststo avoid capacity arithmetic in a security-sensitive path. - Add an AST-based regression test that asserts the top-level clone allocation does not pass a capacity argument.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_model_pricing.go | Removes make(..., len(...)) pre-sizing for the top-level clone map to avoid overflow-prone allocation math. |
| pkg/workflow/compiler_model_pricing_test.go | Adds a regression test that parses the source and checks the result map allocation does not include a capacity argument. |
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: Low
| for _, stmt := range fn.Body.List { | ||
| assign, ok := stmt.(*ast.AssignStmt) | ||
| if !ok || len(assign.Lhs) != 1 || len(assign.Rhs) != 1 { | ||
| continue | ||
| } | ||
| lhs, ok := assign.Lhs[0].(*ast.Ident) | ||
| if !ok || lhs.Name != "result" { | ||
| continue | ||
| } | ||
| call, ok := assign.Rhs[0].(*ast.CallExpr) | ||
| if !ok { | ||
| continue | ||
| } | ||
| fun, ok := call.Fun.(*ast.Ident) | ||
| if !ok || fun.Name != "make" { | ||
| continue | ||
| } | ||
| foundResultMake = true | ||
| assert.Len(t, call.Args, 1, "result map allocation should not pre-size capacity") | ||
| return false | ||
| } | ||
| return false |
🧪 Test Quality Sentinel Report
📊 Metrics (13 tests)
|
There was a problem hiding this comment.
Review: Fix allocation-size overflow risk in model pricing merge
This is a clean, focused security fix. The change is minimal, correct, and well-tested.
What was fixed: CodeQL flagged make(map[string]any, len(modelCosts)) in mergeModelPricingIntoModelCosts. The old code even had a comment acknowledging the overflow risk (Do not add +1 here...) but still passed len(modelCosts) as a capacity hint — the new fix removes it entirely, letting the map grow naturally via maps.Copy.
Test quality: The AST-based regression guard is an excellent choice. It verifies the actual source at the syntax level so any future re-introduction of a capacity argument will be caught at test time, regardless of the value passed.
Scope is appropriate: The fix targets only the CodeQL-flagged allocation. The cost := make(map[string]string, len(pricing)) on line 159 uses a similar pattern but is bounded by trusted internal data (pricing is a typed map[string]float64 argument, not externally sized), so excluding it is reasonable.
✅ Correctness: removing the capacity hint does not change observable behavior — Go maps grow on demand.
✅ Security: allocation-size arithmetic derived from the modelCosts input is eliminated.
✅ Tests: existing merge-correctness tests are unchanged; new AST guard prevents regression.
No blocking issues found. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 42.1 AIC · ⌖ 5.98 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two small but meaningful issues.
📋 Key Themes & Highlights
Key Themes
- Incomplete scope of the fix: the
costmap on line 159 still useslen(pricing)capacity hint — the same pattern CodeQL flagged onresult. Both allocations live in the same security-sensitive function and should be treated consistently. - Fragile test infrastructure: the regression guard relies on
runtime.Caller(0)to locate the source file, which breaks in environments where source and binary paths diverge (Docker, module cache,go install). It also only scans top-level statements in the function body, so a future refactor that moves the assignment inside any compound block would silently pass the test.
Positive Highlights
- ✅ The core fix is minimal and correct — removing the capacity argument eliminates the overflow risk for the top-level clone
- ✅ Adding an AST-based regression test is an excellent pattern for enforcement of security invariants — the intent is right even if the implementation needs tightening
- ✅ The PR description is clear about root cause, scope, and what behavior is preserved
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 62.9 AIC · ⌖ 6.33 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_model_pricing.go:159
[/diagnosing-bugs] The cost map still pre-sizes with len(pricing) — the same allocation-size pattern CodeQL flagged on result. Since pricing comes from parsed model data (potentially untrusted), this is the same category of risk.
<details>
<summary>💡 Suggested change</summary>
// beforecost:=make(map[string]string, len(pricing))
// aftercost:=make(map[string]string)For consistency with the fix applied to result, removing the capacity hint here ensures the who…
pkg/workflow/compiler_model_pricing_test.go:97
[/tdd]runtime.Caller(0) returns the path to the compiled test binary, not the source file — this test will fail or produce wrong results in environments where source paths differ from build paths (e.g., Docker, CI with module cache, or after go install).
<details>
<summary>💡 Safer alternative: embed the source via go:embed</summary>
Using //go:embed compiler_model_pricing.go instead of runtime.Caller ensures the source is always available regardless of build environment:
…
</details><details><summary>pkg/workflow/compiler_model_pricing_test.go:108</summary>**[/tdd]**TheASTwalkonlyiteratestop-levelstatementsin`fn.Body.List` — itwillmissa`result`assignmentthatappearsinsidean`if`block, a `switch`, or any other nested compound statement, silently setting `foundResultMake=false` and causing the test to pass without actually verifying anything.<details><summary>💡 Use recursive AST inspection instead</summary>Replace the manual loop with a nested `ast.Inspect` call scoped to the function body:```go
ast.Inspect(fn.Body, func…
</details>There was a problem hiding this comment.
COMMENT — Security fix correct; two issues need attention
The core fix (removing the capacity hint from the top-level map allocation) correctly addresses the CodeQL finding. The logic of the function and its mutation-isolation guarantees are intact.
Findings
1. Medium — runtime.Caller(0) breaks under -trimpath (test file, line 97)
The new AST regression guard resolves the source file path using runtime.Caller(0). Under -trimpath builds this returns a module-relative path, not an absolute filesystem path; parser.ParseFile then fails and require.NoError kills the test with a confusing error. The guard should check filepath.IsAbs and skip, or use //go:embed to avoid the runtime path lookup entirely.
2. Low — Inconsistent capacity removal (production file, line 158)make(map[string]string, len(pricing)) retains a capacity hint derived from a resolver-supplied map. The PR's stated goal is to eliminate allocation-size arithmetic in this path; this line is the next candidate for the same CodeQL rule. Removing the hint is a one-line change with negligible performance impact.
The first finding is a real latent flake; the second is a correctness-of-intent gap. Neither blocks the security fix from being correct, but both should be addressed before merge.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 204.6 AIC · ⌖ 5.73 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_model_pricing_test.go:97
runtime.Caller(0) breaks under -trimpath builds — this test will panic/fail in any CI step that strips file paths, producing a confusing no such file or directory error rather than a meaningful test failure.
<details>
<summary>💡 Details and suggested fix</summary>
Under -trimpath (used in release builds and many CI pipelines), runtime.Caller(0) returns a module-relative path such as github.com/github/gh-aw/pkg/workflow/compiler_model_pricing_test.go, not an absolute filesyste…
pkg/workflow/compiler_model_pricing.go:158
Inconsistent capacity removal: make(map[string]string, len(pricing)) still uses a capacity hint from the caller-controlled pricing map, which CodeQL or a future audit may flag by the same logic that motivated this PR.
<details>
<summary>💡 Details</summary>
The PR description says the goal is to eliminate "allocation-size arithmetic in this security-sensitive path". The production code removes make(map[string]any, len(modelCosts)) (correct) but leaves:
cost:=make(map[string…
</details><details><summary>pkg/workflow/compiler_model_pricing.go:187</summary>**Shallowclonedoesn'tprotectdeepmutation**: thefunction'sdoccommentclaimsit"returnsanewmaptoavoidmutatingtheinput", butexistingmodel-entrysub-mapsarealiased — notcloned — sodeepmutationsviathereturnedmapsilentlycorrupttheinput.
<details>
<summary>💡 Details</summary>
Thefunctioncarefullyclonesthetop-level, `providers`, per-provider, and`models`layers:
```gomaps.Copy(modelsMap, mm) // copies key→value pairs from original models mapBut `maps.C…
pkg/workflow/compiler_model_pricing_test.go:90
DoesNotMutateInput test uses an empty models map — the aliasing bug it claims to test is unexercised.
<details>
<summary>💡 Details</summary>
The test constructs:
base:=map[string]any{
"providers": map[string]any{
"openai": map[string]any{
"models": map[string]any{}, // empty!
},
},
}Because openai.models is empty, maps.Copy(modelsMap, mm) copies zero entries, so no aliasing ever occurs and the assertion passes vacuously regardless…
pelikhan
commented
Jul 5, 2026
@copilot invoke pr-finisher skill |
gh-aw-bot
commented
Jul 5, 2026
@copilot please run the
|
🤖 PR Triage — Run #7
Score breakdown: impact 35 + urgency 22 + quality 13 Fixes CodeQL-flagged allocation-size overflow in
|
Uh oh!
There was an error while loading. Please reload this page.
🎉 This pull request is included in a new release. Release: |
CodeQL flagged
pkg/workflow/compiler_model_pricing.gofor allocation-size overflow in the workflow compiler’s model-pricing path. The issue was in capacity arithmetic used while cloning the top-levelModelCostsmap.Remove overflow-prone allocation math
mergeModelPricingIntoModelCosts.Add a regression guard
compiler_model_pricing.goand asserts the top-levelresultmap allocation inmergeModelPricingIntoModelCostsdoes not reintroduce a capacity argument.Preserve existing merge behavior
Example of the change: