feat: invert token limits priority to .waza.yaml first #59#64
feat: invert token limits priority to .waza.yaml first #59#64
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates token limits configuration precedence to support the ongoing config consolidation work: .waza.yaml tokens.limits is now the primary source, with .token-limits.json treated as a legacy fallback and accompanied by a deprecation warning when used.
Changes:
- Inverted
resolveLimitsConfig()precedence to prefer.waza.yaml tokens.limits, falling back to.token-limits.json, then built-in defaults. - Updated
checkandsuggestflows to handle the new(cfg, usedLegacy)return and emit a deprecation warning when legacy config is active. - Added unit + integration tests and new fixtures for
.waza.yamllimits and “both present” precedence.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/waza/tokens/helpers.go | Changes limits resolution order and returns a legacy-usage flag. |
| cmd/waza/tokens/check.go | Uses new helper return signature; emits legacy deprecation warning. |
| cmd/waza/tokens/suggest.go | Uses new helper return signature; emits legacy deprecation warning. |
| cmd/waza/tokens/helpers_test.go | Adds unit tests for precedence scenarios. |
| cmd/waza/tokens/check_test.go | Adds integration tests validating .waza.yaml precedence in check. |
| cmd/waza/tokens/testdata/check/waza-yaml-limits/* | New fixtures for .waza.yaml-based limits. |
| cmd/waza/tokens/testdata/check/both-configs/* | New fixtures for “both configs present; .waza.yaml wins”. |
| .squad/agents/linus/history.md | Documents the change and testing summary. |
|
Rusty architecture review summary:
I cannot submit an approve review on this PR because GitHub blocks approving your own pull request, but this is architecturally good to merge. |
|
@copilot, are there any relevant spots to add documentation for this? I think users will need to know how all the configs and overrides intersect. |
|
@richardpark-msft I've opened a new pull request, #70, to work on those changes. Once the pull request is ready, I'll request review from you. |
- Fix gofmt indentation in resolveLimitsConfig (helpers.go) - Handle overrides-only edge case in resolveLimitsConfig (helpers.go) - Route deprecation warning through io.Writer/cmd.ErrOrStderr() (check.go) - Route deprecation warning through cmd.ErrOrStderr() (suggest.go) - Fix json.Marshal error handling in tests (helpers_test.go) - Add TestResolveLimitsConfig_OverridesOnly test (helpers_test.go) - Add TestCheck_LegacyWarningEmitted and TestCheck_NoWarningWithWazaYaml (check_test.go) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
site/src/content/docs/guides/token-limits.mdx:57
- This section says
fallbackLimitis “Limit applied to files that match no pattern”, but token limit fallback forwaza tokens check/suggestis currently hard-coded to 2000 viainternal/checks.FallbackLimit, and.waza.yaml tokens.fallbackLimitisn’t used in the token limits checker. Please clarify whatfallbackLimitactually affects today (or implement support for it).
| Field | Type | Default | Description |
|-------|------|---------|-------------|
| `warningThreshold` | integer | `2500` | Token count at which a soft warning is shown |
| `fallbackLimit` | integer | `1000` | Limit for files that match no pattern |
| `limits.defaults` | map | (built-in) | Glob patterns → token limits |
| `limits.overrides` | map | `{}` | Exact file paths → token limits (take precedence over `defaults`) |
Both `limits.defaults` and `limits.overrides` are optional — you can set only one of them. If `tokens.limits` is present in `.waza.yaml` but neither map is set, `.token-limits.json` is **not** consulted as a fallback; `.waza.yaml` is the definitive source.
spboyer
left a comment
There was a problem hiding this comment.
Rusty (Opus 4.6) — Lint Still Failing
Last commit: \ix: gofmt helpers.go\ (March 4) — but the Lint check is still ❌. Tests pass on both ubuntu and windows, but golangci-lint fails.
Action needed: Run \golangci-lint run\ locally, fix the remaining lint issue, and push.
|
Fixed lint errors and rebased on main. |
- Fix gofmt indentation in resolveLimitsConfig (helpers.go) - Handle overrides-only edge case in resolveLimitsConfig (helpers.go) - Route deprecation warning through io.Writer/cmd.ErrOrStderr() (check.go) - Route deprecation warning through cmd.ErrOrStderr() (suggest.go) - Fix json.Marshal error handling in tests (helpers_test.go) - Add TestResolveLimitsConfig_OverridesOnly test (helpers_test.go) - Add TestCheck_LegacyWarningEmitted and TestCheck_NoWarningWithWazaYaml (check_test.go) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
resolveLimitsConfig() now checks .waza.yaml tokens.limits as the primary source. .token-limits.json is only used as a legacy fallback when .waza.yaml has no limits configured, and emits a deprecation warning when active. Priority order: 1. .waza.yaml tokens.limits (primary) 2. .token-limits.json (legacy fallback with deprecation warning) 3. Built-in defaults Tests cover all four scenarios: waza-only, legacy-only, both-present (.waza.yaml wins), and neither-present. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Initial plan * docs: add token limits configuration guide and update schema/CLI references Co-authored-by: richardpark-msft <51494936+richardpark-msft@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: richardpark-msft <51494936+richardpark-msft@users.noreply.github.com>
- Fix gofmt indentation in resolveLimitsConfig (helpers.go) - Handle overrides-only edge case in resolveLimitsConfig (helpers.go) - Route deprecation warning through io.Writer/cmd.ErrOrStderr() (check.go) - Route deprecation warning through cmd.ErrOrStderr() (suggest.go) - Fix json.Marshal error handling in tests (helpers_test.go) - Add TestResolveLimitsConfig_OverridesOnly test (helpers_test.go) - Add TestCheck_LegacyWarningEmitted and TestCheck_NoWarningWithWazaYaml (check_test.go) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assign fmt.Fprintf return values to _ in check.go and suggest.go to satisfy golangci-lint errcheck. These are warning messages to stderr where ignoring the error is the standard Go pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e6eedcb to
f36fedf
Compare
Pull request was closed
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
087617e to
c7e2d06
Compare
Closes #59
Part of #20
What changed
resolveLimitsConfig()now checks.waza.yaml tokens.limitsfirst.token-limits.jsonis legacy fallback only.token-limits.jsonis the active sourceTesting
TestCheck_ConfigLimitsandTestCheck_DefaultLimitsWhenNoConfigpassgo test ./...green