Include threat-detection credits in forecast totals, expose monthly low/high/stdev, and fix formal-verifier tool denials - #39101
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the forecast pipeline to (1) correctly account for AI credits spent in threat-detection by aggregating all usage JSONL inputs from the compact usage artifact, and (2) expand the forecast issue report to surface a fuller monthly distribution summary (Low/P50/High/Stdev) while keeping ranking and totals centered on Monthly P50.
Changes:
- Add support for discovering and summing AIC across all
usage/**/*.jsonlfiles (including explicitai_creditsand recomputed AIC from token-usage records). - Expand the forecast issue table to include Monthly (Low/P50/High/Stdev) columns, plus an updated “How to read this report” section.
- Add/extend tests covering usage-artifact aggregation and the widened forecast table output.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/token_usage.go | Adds usage-artifact JSONL discovery and AIC summation to include threat-detection spend in forecast cost computation. |
| pkg/cli/token_usage_test.go | Adds tests for mixed usage-artifact inputs and helper parsing utilities used by the new aggregation path. |
| actions/setup/js/create_forecast_issue.cjs | Expands forecast issue markdown generation to show Monthly Low/P50/High/Stdev and updates guidance copy. |
| actions/setup/js/create_forecast_issue.test.cjs | Updates expectations for the widened forecast table and revised guidance strings. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
| "- **Monthly (P50)** is the Monte Carlo median of total AIC over 30 days.", | ||
| "- **Monthly (Low/P50/High)** are the Monte Carlo P10 / P50 / P90 total-AIC bounds over 30 days.", | ||
| "- **Monthly (Stdev)** is the Monte Carlo standard deviation of the 30-day total-AIC distribution.", | ||
| "- Monthly values are distribution medians, not a direct `P50/Run × runs` multiplication.", |
| expect(body).toContain("### How to read this report"); | ||
| expect(body).toContain("Monte Carlo P10 / P50 / P90 total-AIC bounds"); | ||
| expect(body).toContain("Monte Carlo standard deviation"); | ||
| expect(body).toContain("Monthly values are distribution medians"); |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (242 new lines, mostly 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 MatterADRs create a searchable, permanent record of why the codebase looks the way it does — here, why forecast cost is summed across the whole 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
ADRs are stored in
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 263.7 AIC · ⌖ 13.9 AIC · ⊞ 29.4K
| tokenUsageLog.Printf("Analyzing token usage (AIC only) in: %s", runDir) | ||
|
|
||
| usageJSONLFiles := findUsageJSONLFiles(runDir) | ||
| if len(usageJSONLFiles) > 0 { |
There was a problem hiding this comment.
[/tdd] Silent short-circuit: if usage/ contains JSONL files that yield zero AIC (empty content, all-zero records, or an unrecognised schema), analyzeTokenUsageAICOnly returns 0 and never tries the legacy findTokenUsageFile path — potentially undercounting cost silently.
💡 Suggestion
Consider falling back to the legacy path when totalAIC == 0 and the JSONL scan found no valid records:
usageJSONLFiles := findUsageJSONLFiles(runDir)
if len(usageJSONLFiles) > 0 {
totalAIC, err := sumAICFromUsageJSONLFiles(usageJSONLFiles)
if err != nil {
return nil, err
}
if totalAIC > 0 {
return &TokenUsageSummary{TotalAIC: totalAIC}, nil
}
// fall through to legacy path if JSONL produced nothing
}Or, if the intent is that the presence of the usage/ directory is definitive (no fallback), add a test that exercises the zero-AIC-from-JSONL path and add a code comment that the short-circuit is intentional.
| return parsedNum | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[/tdd] usageNumericValue coerces five distinct Go types (float64, json.Number, int, int64, string) across two maps and has no direct unit test. A regression in any branch would only surface through the integration test in TestSumAICFromUsageJSONLFiles.
💡 Suggested tests
func TestUsageNumericValue(t *testing.T) {
cases := []struct{
name string
parsed map[string]any
usage map[string]any
want float64
}{
{"float64 in top-level", map[string]any{"aic": float64(1.5)}, nil, 1.5},
{"json.Number in usage sub-map", nil, map[string]any{"aic": json.Number("2.0")}, 2.0},
{"int value", map[string]any{"aic": int(3)}, nil, 3.0},
{"string value", map[string]any{"aic": "4.25"}, nil, 4.25},
{"NaN skipped, fallback to usage", map[string]any{"aic": math.NaN()}, map[string]any{"aic": float64(5.0)}, 5.0},
{"all zero/missing", nil, nil, 0},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
got := usageNumericValue(tc.parsed, tc.usage, "aic")
assert.InDelta(t, tc.want, got, 1e-9)
})
}
}|
|
||
| total, err := sumAICFromUsageJSONLFiles([]string{fileOne, fileTwo}) | ||
| require.NoError(t, err) | ||
| assert.Greater(t, total, 1.25) |
There was a problem hiding this comment.
[/tdd] The assertion assert.Greater(t, total, 1.25) only confirms the computed AIC is non-zero. A bug that returned 1.26 (off by 1 token) or computed the wrong model rate would still pass this test.
💡 Suggestion
Pin the expected computed AIC using the same computeModelInferenceAIC helper, or compute the expected value once and assert with InDelta:
expected := 1.25 + computeModelInferenceAIC("anthropic", "claude-sonnet-4-6", 1000, 0, 0, 0, 0)
assert.InDelta(t, expected, total, 1e-6, "should be explicit AIC plus computed AIC")This catches regressions in the computed-AIC path rather than just confirming the path was entered.
| } | ||
| closeErr := file.Close() | ||
| if err := scanner.Err(); err != nil { | ||
| return 0, fmt.Errorf("error reading usage JSONL file %s: %w", filePath, err) |
There was a problem hiding this comment.
[/diagnose] closeErr is silently discarded when scanner.Err() is non-nil: the function returns the scan error but never reports the close error. If both fail, only the scanner error is visible. Using defer would also be safer here against any future early-return path.
💡 Suggestion
defer func() {
if cerr := file.Close(); cerr != nil && err == nil {
err = fmt.Errorf("failed to close usage JSONL file %s: %w", filePath, cerr)
}
}()
// ...scanner loop...
if err := scanner.Err(); err != nil {
return 0, fmt.Errorf("error reading usage JSONL file %s: %w", filePath, err)
}This keeps the close error visible when nothing else failed, and the named-return approach lets you propagate it cleanly.
| ); | ||
| if (tableRows.length > 1) { | ||
| dataRows.push(`| **TOTAL** | | | **${formatAIC(totalMonthly)}** |`); | ||
| dataRows.push(`| **TOTAL** | | | | **${formatAIC(totalMonthly)}** | | |`); |
There was a problem hiding this comment.
[/grill-with-docs] The TOTAL row intentionally sums only monthlyP50 (column 4) and leaves Low/High/Stdev blank — a subtle convention that is only documented in the PR description, not in the code. Anyone maintaining this later might "fix" the blank cells, breaking the intentional asymmetry.
💡 Suggestion
Add a brief inline comment:
// TOTAL intentionally shows only P50 (the sortable key); Low/High/Stdev totals are omitted
// because summing percentiles across workflows produces a statistically misleading number.
dataRows.push(`| **TOTAL** | | | | **${formatAIC(totalMonthly)}** | | |`);| ); | ||
|
|
||
| expect(body).toContain("| wf-round | 1 | 2 | 5 |"); | ||
| expect(body).toContain("| wf-round | 1 | 2 | 5 | 5 | 5 | 0 |"); |
There was a problem hiding this comment.
[/tdd] The wf-round test covers the fallback when monthly_monte_carlo is entirely absent, which is good. However there is no test for a partially populated monthly_monte_carlo — e.g., an object that has p50_projected_aic but is missing std_dev_aic or p10_projected_aic. getMonthlyForecastStats uses optional chaining so undefined keys fall through to 0, but a test would make this explicit.
💡 Suggested test case
it("handles partially populated monthly_monte_carlo", async () => {
const module = await import("./create_forecast_issue.cjs");
const body = module.buildForecastIssueBody(
{
period: "month",
workflows: [{
workflow_id: "wf-partial",
sampled_runs: 1,
p50_aic_per_run: 10,
monthly_monte_carlo: { p50_projected_aic: 50 }, // no p10, no p90, no std_dev
}],
},
{ owner: "o", repo: "r", serverUrl: "https://github.com", generatedAtISO: "2026-01-01T00:00:00.000Z" }
);
// low and high fall back to p50; stddev falls back to 0
expect(body).toContain("| wf-partial | 1 | 10 | 50 | 50 | 50 | 0 |");
});|
@copilot run pr-reviewer skill |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 87/100 — Excellent
📊 Metrics & Test Classification (7 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in 755cca1. I investigated the linked CGO failure job, identified the flaky |
The forecast report was undercounting workflows that spend AI credits in threat-detection, and it only surfaced the monthly median. This updates the forecast pipeline to include detection usage in per-run cost computation and exposes the monthly low / P50 / high / standard deviation range per workflow.
Forecast cost aggregation
usageartifact, not just the main agent usage.ai_creditsrecords and raw token-usage records that need AIC recomputation.Forecast report shape
Monthly (Low)= Monte Carlo P10Monthly (P50)= Monte Carlo medianMonthly (High)= Monte Carlo P90Monthly (Stdev)= Monte Carlo standard deviationReport guidance
Focused coverage
Workflow guardrail follow-up
Daily Formal Spec Verifiertool permissions to allow reading CLI Go sources viacat pkg/cli/*.go.