Add max-daily-ai-credits frontmatter guardrail and deprecate max-daily-effective-tokens#37101
Add max-daily-ai-credits frontmatter guardrail and deprecate max-daily-effective-tokens#37101Copilot wants to merge 6 commits into
max-daily-ai-credits frontmatter guardrail and deprecate max-daily-effective-tokens#37101Conversation
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>
max-daily-ai-credits frontmatter guardrail and deprecate max-daily-effective-tokens
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces max-daily-ai-credits as the new daily per-workflow guardrail field, deprecates max-daily-effective-tokens, and switches the runtime daily guardrail metric from Effective Tokens (ET) to AI Credits (AIC) while preserving compatibility paths.
Changes:
- Added
max-daily-ai-creditsto frontmatter types + JSON schema and markedmax-daily-effective-tokensas deprecated with migration guidance. - Updated compiler/import resolution to prefer the new field and fall back to the deprecated one.
- Updated runtime guardrail reporting/aggregation to compute and display AI Credits (AIC), alongside docs and tests.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/frontmatter_types.go | Adds new frontmatter field for max-daily-ai-credits. |
| pkg/workflow/daily_effective_workflow_guardrail_test.go | Updates guardrail resolution tests for new field + deprecated fallback. |
| pkg/workflow/daily_effective_workflow.go | Prefers new frontmatter field, adds deprecated fallback, updates validation/disable detection. |
| pkg/workflow/compiler_validators.go | Updates validator log message to reflect AIC guardrail. |
| pkg/parser/schemas/main_workflow_schema.json | Adds new schema field/definition and deprecates old field with message. |
| pkg/parser/schema_test.go | Updates schema validation tests to use max-daily-ai-credits. |
| pkg/parser/import_processor.go | Updates import merge comment to reflect new/old field behavior. |
| pkg/parser/import_field_extractor_test.go | Updates import precedence test to ensure new field wins with fallback. |
| pkg/parser/import_field_extractor.go | Extracts max-daily-ai-credits first, then deprecated field into same accumulator slot. |
| docs/src/content/docs/reference/glossary.md | Renames glossary entry and examples to AIC field. |
| docs/src/content/docs/reference/frontmatter.md | Updates frontmatter docs and adds deprecation note. |
| docs/src/content/docs/reference/cost-management.md | Updates cost-management guidance to AIC field + deprecation note. |
| actions/setup/md/daily_effective_workflow_exceeded.md | Updates user-facing exceeded message and guidance to AIC terminology. |
| actions/setup/js/handle_agent_failure.cjs | Updates guardrail terminology in logs/docs to AIC. |
| actions/setup/js/daily_effective_workflow_helpers.cjs | Adds AIC summation/stat helpers and formatting. |
| actions/setup/js/check_daily_effective_workflow_guardrail.test.cjs | Updates tests to validate AIC aggregation/stats/summary rendering. |
| actions/setup/js/check_daily_effective_workflow_guardrail.cjs | Switches runtime aggregation and summary rendering from ET to AIC. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 17/17 changed files
- Comments generated: 6
| for _, field := range []string{maxDailyAICreditsField, maxDailyEffectiveTokensField} { | ||
| raw, ok := data.RawFrontmatter[field] | ||
| if !ok { | ||
| continue | ||
| } | ||
| if val, ok := typeutil.ParseIntValue(raw); ok && val < 0 { | ||
| return fmt.Errorf("%s must be at least 0, got %d", field, val) | ||
| } |
| on: | ||
| workflow_dispatch: | ||
| stale-check: false | ||
| max-daily-effective-tokens: -1 | ||
| max-daily-ai-credits: -1 |
| err := compiler.CompileWorkflow(workflowFile) | ||
| if err == nil { | ||
| t.Fatal("expected compile to fail for negative max-daily-effective-tokens") | ||
| t.Fatal("expected compile to fail for negative max-daily-ai-credits") |
| } | ||
|
|
||
| core.setOutput("daily_effective_workflow_total_effective_tokens", String(totalEffectiveTokens)); | ||
| core.setOutput("daily_effective_workflow_total_effective_tokens", String(totalAIC)); |
| "max_daily_ai_credits_limit": { | ||
| "description": "Maximum AI Credits budget allowed across the last 24 hours for runs of this workflow by the triggering user. Accepts positive integers (including K/M suffix strings and expressions). Set to -1 to disable.", | ||
| "oneOf": [ | ||
| { | ||
| "$ref": "#/$defs/positive_effective_tokens_limit" | ||
| }, |
| * Build a context string when the 24-hour per-workflow AIC guardrail prevented the agent from | ||
| * starting in the activation job. | ||
| * @param {boolean} hasDailyEffectiveWorkflowExceeded - Whether the daily workflow quota was exceeded | ||
| * @param {string} totalEffectiveTokens - Aggregated ET usage across the last 24 hours | ||
| * @param {string} totalEffectiveTokens - Aggregated AIC usage across the last 24 hours |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #37101 does not have the 'implementation' label (has_implementation_label=false) and has 94 new lines in business logic directories, below the 100-line threshold (requires_adr_by_default_volume=false). No custom config present. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
…deprecated var detection Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics & Test Classification (12 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📎 Notable observations (non-blocking)JavaScript unit-change precision (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 95/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 12 changed tests enforce behavioral contracts, covering schema validation error paths, backward-compatibility edge cases, suffix/expression boundary conditions, and statistical computation invariants. No guideline violations detected.
Done in commit
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /zoom-out, /grill-with-docs, and /improve-codebase-architecture — requesting changes on a correctness issue and test coverage gaps.
📋 Key Themes & Highlights
Blocking issues
- Cross-import field priority bug (
import_field_extractor.go:363): the new field only wins within a single frontmatter — if an earlier import usesmax-daily-effective-tokensand a later import usesmax-daily-ai-credits, the deprecated field silently wins. This contradicts the migration intent. computeInferenceAICfallback untested (daily_effective_workflow_helpers.cjs:124): every log entry written before this feature ships lacks anaicfield and will hit the computed fallback during the 24-hour transition window. No test covers this path.- No backward-compat schema test (
schema_test.go:527): all tests were migrated to the new field; there is nothing ensuringmax-daily-effective-tokensstill passes schema validation.
Important, non-blocking
- Output key unit mismatch (
check_daily_effective_workflow_guardrail.cjs:443):daily_effective_workflow_total_effective_tokensnow carries AIC — downstream consumers receiving this output will silently get the wrong unit. - No compile-time deprecation warning (
daily_effective_workflow.go:79): thedeprecated: trueschema annotation is IDE-only;gh aw compilenever tells authors to migrate. - Template placeholder not renamed (
daily_effective_workflow_exceeded.md:3):{total_effective_tokens}in a user-facing error message clashes with the surrounding AIC terminology. - Missing precedence contract tests: no test for both fields in the same frontmatter (
daily_effective_workflow_guardrail_test.go) or across imports (import_field_extractor_test.go).
Positive highlights
- ✅ Solid first-wins semantics within a single frontmatter (correct within-doc ordering).
- ✅ The
supports deprecated field fallbacktest is a great addition — exactly the kind of explicit contract test this feature needs. - ✅
resolveMaxDailyEffectiveTokensis clean and easy to follow; new-field-first then deprecated fallback is the right pattern. - ✅ Good coverage of the explicit
aicfield path insumAICFromTokenUsageFile. - ✅ PR description is thorough and clearly explains the motivation.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 6.8M · 813 AIC
| acc.extractFirstWinsJSONField(fm, fullPath, "max-turns", &acc.mergedMaxTurns) | ||
| acc.extractFirstWinsJSONField(fm, fullPath, "max-runs", &acc.mergedMaxRuns) | ||
| acc.extractFirstWinsJSONField(fm, fullPath, "max-effective-tokens", &acc.mergedMaxEffectiveTokens) | ||
| acc.extractFirstWinsJSONField(fm, fullPath, "max-daily-ai-credits", &acc.mergedMaxDailyEffectiveTokens) |
There was a problem hiding this comment.
[/zoom-out] Field priority only holds within a single frontmatter; the new field does not win over the deprecated field when they come from different imports.
💡 Bug scenario
With the current implementation, extractFirstWinsJSONField is called for each frontmatter in import order. If import A (processed first) has max-daily-effective-tokens: 200 and import B has max-daily-ai-credits: 100, import A’s deprecated value wins — the accumulator is already set when import B is processed.
A safe fix is to track both fields in separate accumulators and merge with explicit priority at the end of import processing:
// Track separately:
acc.extractFirstWinsJSONField(fm, fullPath, "max-daily-ai-credits", &acc.mergedMaxDailyAICredits)
acc.extractFirstWinsJSONField(fm, fullPath, "max-daily-effective-tokens", &acc.mergedMaxDailyEffectiveTokens)
// In finalize: prefer new field over deprecated| total += explicit; | ||
| continue; | ||
| } | ||
| const computed = computeInferenceAIC({ |
There was a problem hiding this comment.
[/tdd] The computed-fallback path — triggered when a JSONL entry has no aic field — has no test. Every log entry written before this feature ships will hit this path during the 24-hour transition window.
💡 Suggested additional test case
it("falls back to computeInferenceAIC when aic field is absent", () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "daily-guardrail-fallback-"));
const filePath = path.join(tmpDir, "token-usage.jsonl");
// Entry without aic — should compute AIC from token counts
fs.writeFileSync(
filePath,
JSON.stringify({ model: "gpt-4o", input_tokens: 1000, output_tokens: 500 }),
"utf8"
);
expect(exports.sumAICFromTokenUsageFile(filePath)).toBeGreaterThan(0);
});Also worth a negative test: unknown model → 0 AIC, run is not counted.
| } | ||
| if value, found := resolveMaxDailyEffectiveTokensFromRaw(frontmatter[maxDailyEffectiveTokensField]); found { | ||
| dailyEffectiveWorkflowLog.Print("Resolved max-daily-effective-tokens from workflow frontmatter") | ||
| dailyEffectiveWorkflowLog.Print("Resolved deprecated max-daily-effective-tokens from workflow frontmatter") |
There was a problem hiding this comment.
[/grill-with-docs] The deprecated field is silently accepted — users who compile a workflow with max-daily-effective-tokens get no actionable feedback. The PR description says "explicit migration guidance" is added, but this log line is internal-only and not surfaced in gh aw compile output.
💡 Suggestion
Emit a compiler-level warning here so authors see the message at compile time:
// After logging the internal debug message:
compilerWarnings.Add(fmt.Sprintf(
"%s is deprecated and will be removed in a future version; use %s instead",
maxDailyEffectiveTokensField, maxDailyAICreditsField,
))The deprecated: true annotation in the JSON schema only helps IDE tooling; the CLI itself never reads it.
| @@ -443,7 +443,7 @@ async function main() { | |||
| } | |||
There was a problem hiding this comment.
[/zoom-out] The output key daily_effective_workflow_total_effective_tokens now carries an AIC value, not an effective-token count. Any downstream step that reads this output and interprets it as effective tokens will silently receive a different unit.
💡 Options
- Breaking (clean): Rename to
daily_effective_workflow_total_aic; update all.lock.ymlconsumers. - Non-breaking: Emit both the legacy key (for backward compat) and a new
daily_effective_workflow_total_aickey, then deprecate the old one. - At minimum, add a comment on this line and on the agentic_commands.yml step that reads it so the unit change is documented in-place.
|
|
||
| - **24h ET usage:** `{total_effective_tokens}` effective tokens | ||
| - **Configured threshold:** `{threshold}` effective tokens | ||
| - **24h AIC usage:** `{total_effective_tokens}` AI Credits |
There was a problem hiding this comment.
[/grill-with-docs] The label now says "AIC usage" but the template placeholder {total_effective_tokens} still reads "effective_tokens" — inconsistent terminology in a user-facing message.
💡 Suggestion
-**24h AIC usage:** `{total_effective_tokens}` AI Credits
+**24h AIC usage:** `{total_aic}` AI CreditsUpdate the interpolation call site (in handle_agent_failure.cjs) to pass total_aic instead of total_effective_tokens.
| @@ -678,7 +680,7 @@ func TestExtractConfigFields_FirstWinsAndAccumulates(t *testing.T) { | |||
| assert.Equal(t, "10", acc.mergedMaxTurns, "max-turns should be first-wins") | |||
There was a problem hiding this comment.
[/tdd] This test only validates same-frontmatter precedence (both fields in the same fm map). The cross-import case — import A has max-daily-effective-tokens, import B has max-daily-ai-credits — is not covered and currently produces the wrong result (see the ordering issue on import_field_extractor.go:363).
💡 Suggested additional sub-test
acc2 := &importAccumulator{}
acc2.extractConfigFields(map[string]any{"max-daily-effective-tokens": 200}, "/import-a.md")
acc2.extractConfigFields(map[string]any{"max-daily-ai-credits": 100}, "/import-b.md")
assert.Equal(t, "100", acc2.mergedMaxDailyEffectiveTokens,
"new field from a later import should win over deprecated field from an earlier import")| * @param {Array<{aic:number}>} runs | ||
| * @returns {{count:number,total:number,average:number,min:number,max:number,stddev:number}} | ||
| */ | ||
| function calculateDailyAICStats(runs) { |
There was a problem hiding this comment.
[/improve-codebase-architecture] calculateDailyAICStats is structurally identical to calculateDailyEffectiveWorkflowStats — the only difference is the field name (aic vs effective_tokens). Duplicating the stats logic means any future fix (e.g. handling NaN differently) must be applied in two places.
💡 Refactor suggestion
/**
* `@param` {number[]} values
* `@returns` {{count:number,total:number,average:number,min:number,max:number,stddev:number}}
*/
function calculateStats(values) {
const filtered = values.filter(v => Number.isFinite(v) && v > 0);
if (filtered.length === 0) return { count: 0, total: 0, average: 0, min: 0, max: 0, stddev: 0 };
const total = filtered.reduce((s, v) => s + v, 0);
const average = total / filtered.length;
const variance = filtered.length > 1
? filtered.reduce((s, v) => s + (v - average) ** 2, 0) / (filtered.length - 1)
: 0;
return { count: filtered.length, total, average,
min: Math.min(...filtered), max: Math.max(...filtered), stddev: Math.sqrt(variance) };
}
const calculateDailyAICStats = runs => calculateStats(runs.map(r => Number(r?.aic || 0)));
const calculateDailyEffectiveWorkflowStats = runs => calculateStats(runs.map(r => Number(r?.effective_tokens || 0)));| * starting in the activation job. | ||
| * @param {boolean} hasDailyEffectiveWorkflowExceeded - Whether the daily workflow quota was exceeded | ||
| * @param {string} totalEffectiveTokens - Aggregated ET usage across the last 24 hours | ||
| * @param {string} totalEffectiveTokens - Aggregated AIC usage across the last 24 hours |
There was a problem hiding this comment.
[/grill-with-docs] The @param name totalEffectiveTokens is stale — this parameter now carries an AIC value. The description was updated but the name was not, so callers and readers must track the discrepancy mentally.
💡 Suggestion
- * `@param` {string} totalEffectiveTokens - Aggregated AIC usage across the last 24 hours
+ * `@param` {string} totalAIC - Aggregated AIC usage across the last 24 hoursAlso rename the parameter in the function signature and all call sites to remove the misleading EffectiveTokens name.
| "max-effective-tokens": "${{ inputs.max-effective-tokens }}", | ||
| "max-daily-effective-tokens": "${{ inputs.max-daily-effective-tokens }}", | ||
| "on": "push", | ||
| "max-runs": "${{ inputs.max-runs }}", |
There was a problem hiding this comment.
[/tdd] All schema tests were migrated to max-daily-ai-credits. There are no tests verifying that max-daily-effective-tokens is still accepted by the schema after being marked deprecated: true. If the deprecated definition is ever accidentally removed or broken, no test would catch it.
💡 Suggested backward-compat test
func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_DeprecatedMaxDailyEffectiveTokensStillAccepted(t *testing.T) {
t.Parallel()
fm := map[string]any{
"on": "push",
"max-daily-effective-tokens": 1000,
}
err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(fm, "/tmp/deprecated-field-compat-test.md")
if err != nil {
t.Fatalf("expected deprecated field to still pass schema validation, got: %v", err)
}
}| t.Run("prefers top-level literal value", func(t *testing.T) { | ||
| t.Parallel() | ||
| got := resolveMaxDailyEffectiveTokens(map[string]any{"max-daily-effective-tokens": 1234}, `"999"`) | ||
| got := resolveMaxDailyEffectiveTokens(map[string]any{"max-daily-ai-credits": 1234}, `"999"`) |
There was a problem hiding this comment.
[/tdd] No test covers the case where both max-daily-ai-credits and max-daily-effective-tokens are present in the same frontmatter — i.e., that the new field is preferred and the deprecated one ignored.
💡 Suggested sub-test
t.Run("new field wins over deprecated when both present", func(t *testing.T) {
t.Parallel()
got := resolveMaxDailyEffectiveTokens(
map[string]any{
"max-daily-ai-credits": 5000,
"max-daily-effective-tokens": 9999, // should be ignored
},
"",
)
if got == nil || *got != "5000" {
t.Fatalf("expected new field to win, got %v", got)
}
})This also documents the intended precedence contract for future readers.
This PR introduces
max-daily-ai-creditsas the new daily frontmatter guardrail field and deprecatesmax-daily-effective-tokens. Daily guardrail evaluation now tracks AI Credits (AIC) rather than effective tokens, while preserving backward compatibility for existing workflows.Schema + deprecation path
max-daily-ai-creditsto main workflow frontmatter schema withmax_daily_ai_credits_limit.max-daily-effective-tokensas deprecated with explicit migration guidance.integer,K/Msuffix strings, expressions,-1disable).Compiler/import resolution behavior
max-daily-ai-credits(first-wins), then fall back to deprecatedmax-daily-effective-tokens.Runtime guardrail metric switch (ET ➜ AIC)
token-usage.jsonl.Enterprise environment variable support
GH_AW_DEFAULT_MAX_DAILY_AI_CREDITSas the new enterprise env var override for the daily AIC guardrail default.ResolveDefaultMaxDailyAICreditschecksGH_AW_DEFAULT_MAX_DAILY_AI_CREDITSfirst, then falls back to the deprecatedGH_AW_DEFAULT_MAX_DAILY_EFFECTIVE_TOKENSwith a deprecation warning log, then the 500k built-in default.default_max_daily_ai_creditsinto thegh aw env get/updateYAML defaults manager alongside the deprecated field.Docs + examples
max-daily-ai-credits.