Support short-form effective-token limits (100M, 100K) across schema, parsers, and docs#36496
Conversation
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>
100M, 100K) across schema, parsers, and docs
There was a problem hiding this comment.
Pull request overview
This PR standardizes support for short-form effective-token limits (e.g., 100M, 100K) across the frontmatter JSON schema, Go compiler parsing, the setup Action’s JS parsing helpers, and reference documentation—aiming to keep validation and runtime normalization aligned.
Changes:
- Refactors the main workflow JSON schema to reuse shared
$defsformax-effective-tokens/max-daily-effective-tokensand acceptK/Msuffix forms consistently. - Consolidates Go parsing/normalization logic into shared helpers for effective-token limits and wires both ET limit fields through them.
- Updates JS parsing to a shared BigInt-based flow and updates docs/examples to prefer short-form notation (e.g.,
5M,15M,25M).
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/engine.go | Routes max-effective-tokens parsing through shared helper and improves invalid-value logging. |
| pkg/workflow/effective_token_limit_parser.go | Adds shared Go helpers to normalize/parse positive limits and max-effective-tokens semantics. |
| pkg/workflow/daily_effective_workflow.go | Reuses shared normalization for max-daily-effective-tokens runtime string emission. |
| pkg/parser/schemas/main_workflow_schema.json | Introduces shared $defs for ET limit types and rewires both fields to reference them. |
| docs/src/content/docs/reference/glossary.md | Updates examples/default wording to include short-form limits (e.g., 5M, 15M). |
| docs/src/content/docs/reference/frontmatter.md | Updates effective-token limit docs/examples to prefer short-form notation. |
| docs/src/content/docs/reference/frontmatter-full.md | Updates defaults/examples for max-effective-tokens (but one example now conflicts with its stated “integer format”). |
| docs/src/content/docs/reference/engines.md | Updates default max-effective-tokens wording to 25M. |
| docs/src/content/docs/reference/cost-management.md | Updates examples and defaults snippets to short-form K/M strings. |
| docs/src/content/docs/reference/compiler-enterprise-environment-controls.md | Updates org env/variable examples to use short-form K/M values. |
| actions/setup/js/effective_token_limits.test.cjs | Expands tests to cover whitespace trimming and numeric input. |
| actions/setup/js/effective_token_limits.cjs | Refactors parsing around a BigInt helper and shared regex. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 2
| # Format 1: Maximum effective-token (ET) budget for AWF API proxy enforcement. Use | ||
| # a negative value to disable budget enforcement and token steering. | ||
| max-effective-tokens: 1 | ||
| max-effective-tokens: 1M | ||
|
|
| function parsePositiveEffectiveTokenLimitBigInt(value) { | ||
| if (typeof value === "number" && Number.isFinite(value) && Number.isInteger(value) && value > 0) { | ||
| return String(value); | ||
| return BigInt(value); | ||
| } |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality 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 (>100 new lines across 📄 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. This PR introduces a new "single source of truth per layer" convention for limit parsing — exactly the kind of structural choice future contributors will want explained. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /grill-with-docs, and /improve-codebase-architecture — clean refactor overall, 3 minor observations (no blockers).
📋 Key Themes & Highlights
Key Themes
- Missing Go unit tests:
effective_token_limit_parser.gois the new single source of truth for K/M normalization across both enforcement paths, but ships without a test file. The JS side added tests; the Go side should too. - Schema metadata not fully updated: The
default: 25000000and its inlinedescriptionin themax-effective-tokensfield still reference the raw integer, while the PR updates all other occurrences to short-form. $defname subtly misleads:positive_integer_with_km_suffix_or_expressionsounds like the suffix is required, but the regex makes it optional.
Positive Highlights
- ✅ Excellent DRY refactor — duplicated inline
oneOfschema blocks extracted into shared$defscleanly. - ✅ Parallel structure between Go and JS parsers (bigint inner + string outer) makes future updates easier.
- ✅ Improved error log in
engine.gonow includes the value type, which is much more useful for debugging. - ✅ Negative-value /
-1disable semantics are preserved correctly through the refactor in both schema and Go helpers. - ✅ Whitespace trimming added to both the JS and Go paths — good defensive parsing.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.7M
| return 0, false | ||
| } | ||
| return parsed, true | ||
| } |
There was a problem hiding this comment.
[/tdd] No unit tests for the new Go parser functions — this is the single source of truth for K/M suffix normalization used by both engine.go and daily_effective_workflow.go, so a bug here silently corrupts both enforcement paths.
💡 Suggested test coverage
A effective_token_limit_parser_test.go file should cover at minimum:
(go/redacted):build !integration
package workflow
import "testing"
func TestNormalizePositiveEffectiveTokenLimit(t *testing.T) {
tests := []struct{
name string; input any; want string; ok bool
}{
{"plain int", 1000, "1000", true},
{"K suffix", "100K", "100000", true},
{"M suffix", "100M", "100000000", true},
{"lowercase m", "5m", "5000000", true},
{"whitespace", " 5M ", "5000000", true},
{"zero", 0, "", false},
{"negative int", -1, "", false},
{"zero string", "0", "", false},
{"expression", "${{ inputs.limit }}", "", false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, ok := normalizePositiveEffectiveTokenLimit(tt.input)
if ok != tt.ok || got != tt.want {
t.Errorf("got (%q, %v), want (%q, %v)", got, ok, tt.want, tt.ok)
}
})
}
}
func TestParseMaxEffectiveTokenLimitValue(t *testing.T) {
tests := []struct{ input any; want int64; ok bool }{
{25000000, 25000000, true},
{"100M", 100000000, true},
{"-1", -1, true},
{-1, -1, true},
{"0", 0, false},
{0, 0, false},
}
// ...
}The negative-integer path (val != 0 in parseMaxEffectiveTokenLimitValue) also means values like -5 are accepted as valid — worth an explicit test to confirm whether that's intentional.
| "description": "Maximum 24-hour per-workflow effective-token budget as a numeric string or GitHub Actions expression. Use -1 to disable." | ||
| } | ||
| ], | ||
| "$ref": "#/$defs/max_daily_effective_tokens_limit", |
There was a problem hiding this comment.
[/grill-with-docs] The default value and field-level description still reference the raw integer 25000000, while the PR's stated goal is to normalize examples to short-form notation.
💡 Suggested fix
"default": "25M",
"description": "Explicit ET budget control for firewall cost enforcement. Defaults to 25M when omitted. Set to a negative value to disable budget enforcement and token steering."Note that changing default from integer to string may need a type check — if the schema validator or tooling treats default as a sample value, this is safe. If parsers actually use the schema default field to inject a value at runtime, keep it as 25000000 and just update the description.
| "positive_integer_with_km_suffix_or_expression": { | ||
| "type": "string", | ||
| "oneOf": [ | ||
| { |
There was a problem hiding this comment.
[/improve-codebase-architecture] The $def name positive_integer_with_km_suffix_or_expression implies the K/M suffix is required, but the regex ^[1-9][0-9]*([kKmM])?$ makes the suffix optional — plain integers like "5000000" also match.
💡 Suggested rename
Consider positive_integer_with_optional_km_suffix_or_expression or simply positive_token_limit_string to avoid misleading readers of the schema. The current name could cause a future maintainer to think plain-integer strings are rejected by this def.
|
@copilot review all comments, commit and apply copilot-review skill |
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 616.4K
Comments that could not be inline-anchored
pkg/workflow/effective_token_limit_parser.go:51
Bug: any non-zero negative integer is accepted as a valid limit, not just -1. Any caller passing int(-2) or int(-999) will slip through, silently disabling token steering without matching the documented -1 sentinel.
<details>
<summary>💡 Suggested fix</summary>
Change the guard from val != 0 to val > 0 || val == -1:
// Before
if val, ok := typeutil.ParseIntValue(raw); ok && val != 0 {
return int64(val), true
}
// After
if val, ok := typeutil.ParseIntValue(raw); ok && (…
</details>
<details><summary>pkg/workflow/engine.go:144</summary>
**`GetMaxEffectiveTokens()` is now dead code — no callers remain after this refactor.** `awf_config.go` replaced the call with inline logic that also applies the enterprise env-var override (`ResolveDefaultMaxEffectiveTokens`). The exported method silently falls back to the compile-time constant, skipping the env override.
<details>
<summary>💡 Detail</summary>
Before the refactor, `awf_config.go` presumably called `GetMaxEffectiveTokens()`. After the refactor it reads `EngineConfig.MaxEffect…
</details>
<details><summary>pkg/workflow/effective_token_limit_parser.go:23</summary>
**No direct unit tests for either new helper in this file.** The `val != 0` integer-path bug in `parseMaxEffectiveTokenLimitValue` (see other comment) would have been caught immediately by a table test.
<details>
<summary>💡 Suggested tests</summary>
```go
// effective_token_limit_parser_test.go
func TestParseMaxEffectiveTokenLimitValue(t *testing.T) {
tests := []struct{name string; input any; want int64; ok bool}{
{"int positive", int(1000), 1000, true},
{"int -1 s…
</details>Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the in-scope review feedback and pushed commit
|
This updates
max-effective-tokensandmax-daily-effective-tokensto consistently accept short-form limits (for example100M) across frontmatter validation and runtime parsing paths. It also aligns reference docs to use short-form notation by default.Schema refactor (single source of truth)
main_workflow_schema.json.max-effective-tokens,max-daily-effective-tokens) to reuse those defs, preserving existing-1disable semantics and expression support.Go compiler parser refactor
pkg/workflow/effective_token_limit_parser.gofor normalization/parsing of positive values with optionalK/Msuffix.parseMaxEffectiveTokensValue(pkg/workflow/engine.go)parseMaxDailyEffectiveTokensValue(pkg/workflow/daily_effective_workflow.go)JavaScript parser helper refactor
actions/setup/js/effective_token_limits.cjsaround a shared bigint-based parser.Docs normalization to short-form
5M,15M,25M) in:frontmatter.mdglossary.mdcost-management.mdfrontmatter-full.mdengines.mdcompiler-enterprise-environment-controls.md