[Core] BudgetDecision.BLOCK이 provider 호출을 차단하도록 수정 - #54
Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
Walkthrough예산 결정은 확정 사용량과 후보 비용을 포함한 예상 사용량을 분리한다. 평가기는 예외 대신 구조화된 결정을 반환한다. 알림과 샘플 앱은 Changes예산 결정 및 집행
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 77: Update the token-pilot-budget entry in the module-status table to
state that it provides pure budget decisions only, without claiming legacy BLOCK
enforcement. Record provider-boundary enforcement as owned by
token-pilot-spring-ai’s DefaultLedgerAdvisor, while preserving the budget
module’s dependency-light design and noting only its actual implemented
capabilities.
- Line 331: Update the legacy budget-flow wording to replace “already-exhausted
status” with “already-exhausted budget decision at the legacy provider
boundary.” Explicitly state that BudgetDecision.EvaluationType.STATUS is a
candidate-free status lookup, not admission authorization or a basis for
permitting provider invocation.
In
`@token-pilot-budget/src/test/java/io/tokenpilot/budget/internal/DefaultBudgetEvaluatorTest.java`:
- Around line 52-75: Extend the parameterized test
한도_미만의_예상_비용을_포함해_ALLOW와_WARN을_판정한다 to verify that store.addCost(...) is never
called after evaluator.evaluate(...). Match the existing no-call verification
used by the BLOCK test while preserving all current assertions.
In
`@token-pilot-sample-app/src/main/java/io/tokenpilot/sample/SampleController.java`:
- Around line 99-112: Update the decision handling in SampleController so
CURRENCY_MISMATCH is treated as a provider-boundary fail-closed outcome
alongside BLOCK, returning the actual blocked state, projected usage, and limit
instead of "blockedState":"NONE". Add an end-to-end regression test covering the
currency mismatch response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1778ad61-48e8-4a79-9efa-617ce8add5a4
📒 Files selected for processing (14)
AGENTS.mdtoken-pilot-autoconfigure/src/test/java/io/tokenpilot/autoconfigure/TokenPilotAutoConfigurationTest.javatoken-pilot-budget/src/main/java/io/tokenpilot/budget/BudgetDecision.javatoken-pilot-budget/src/main/java/io/tokenpilot/budget/BudgetEvaluator.javatoken-pilot-budget/src/main/java/io/tokenpilot/budget/exception/BudgetExceededException.javatoken-pilot-budget/src/main/java/io/tokenpilot/budget/internal/DefaultBudgetEvaluator.javatoken-pilot-budget/src/test/java/io/tokenpilot/budget/internal/DefaultBudgetEvaluatorTest.javatoken-pilot-notification/src/main/java/io/tokenpilot/notification/BudgetNotificationEvent.javatoken-pilot-notification/src/main/java/io/tokenpilot/notification/BudgetNotificationService.javatoken-pilot-notification/src/test/java/io/tokenpilot/notification/BudgetNotificationServiceTest.javatoken-pilot-sample-app/src/main/java/io/tokenpilot/sample/SampleController.javatoken-pilot-sample-app/src/test/java/io/tokenpilot/sample/SampleApplicationBudgetE2ETest.javatoken-pilot-spring-ai/src/main/java/io/tokenpilot/springai/internal/DefaultLedgerAdvisor.javatoken-pilot-spring-ai/src/test/java/io/tokenpilot/springai/internal/DefaultLedgerAdvisorTest.java
Comment on lines
+52
to
+75
| @ParameterizedTest | ||
| @CsvSource({ | ||
| "10.00, 20.00, 30.00, ALLOW, NONE", | ||
| "70.00, 10.00, 80.00, WARN, WARNING" | ||
| }) | ||
| void 한도_미만의_예상_비용을_포함해_ALLOW와_WARN을_판정한다( | ||
| String committed, | ||
| String candidate, | ||
| String projected, | ||
| BudgetState expectedState, | ||
| BudgetThreshold expectedThreshold | ||
| ) { | ||
| DefaultBudgetEvaluator evaluator = evaluator(policy(null, ZoneOffset.UTC), "2026-07-22T00:00:00Z"); | ||
| when(store.getAccumulatedCost(any(), any())).thenReturn(usd("70.00")); | ||
| when(store.getAccumulatedCost(any(), any())).thenReturn(usd(committed)); | ||
|
|
||
| BudgetDecision result = evaluator.evaluate(TAGS, usd("10.00")); | ||
| BudgetDecision result = evaluator.evaluate(TAGS, usd(candidate)); | ||
|
|
||
| assertThat(result.state()).isEqualTo(BudgetState.WARN); | ||
| assertThat(result.threshold()).isEqualTo(BudgetThreshold.WARNING); | ||
| assertThat(result.state()).isEqualTo(expectedState); | ||
| assertThat(result.threshold()).isEqualTo(expectedThreshold); | ||
| assertThat(result.key()).isEqualTo(key("policy-a", "tenant-a", "2026-07")); | ||
| assertThat(result.currentUsage()).isEqualTo(usd("80.00")); | ||
| assertThat(result.evaluationType()).isEqualTo(EvaluationType.ADMISSION); | ||
| assertThat(result.committedUsage()).isEqualTo(usd(committed)); | ||
| assertThat(result.projectedUsage()).isEqualTo(usd(projected)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
ALLOW/WARN 테스트에 addCost 비호출 검증 누락
BLOCK 테스트(Line 99)에는 verify(store, never()).addCost(...)가 있지만, 이 파라미터라이즈드 테스트에는 동일한 검증이 없습니다. evaluate()가 어떤 상태에서도 store를 변경하지 않는 순수 함수여야 한다는 보장은 ALLOW/WARN 경로에서도 동일하게 중요합니다.
참고로 라인 레인지 변경 요약에는 "두 테스트 모두 ... store.addCost의 비호출을 함께 검증한다"고 되어 있으나, 실제 코드상 이 테스트에는 해당 검증이 빠져 있습니다.
✅ 제안 diff
assertThat(result.committedUsage()).isEqualTo(usd(committed));
assertThat(result.projectedUsage()).isEqualTo(usd(projected));
+ verify(store, never()).addCost(any(), any(), any());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @ParameterizedTest | |
| @CsvSource({ | |
| "10.00, 20.00, 30.00, ALLOW, NONE", | |
| "70.00, 10.00, 80.00, WARN, WARNING" | |
| }) | |
| void 한도_미만의_예상_비용을_포함해_ALLOW와_WARN을_판정한다( | |
| String committed, | |
| String candidate, | |
| String projected, | |
| BudgetState expectedState, | |
| BudgetThreshold expectedThreshold | |
| ) { | |
| DefaultBudgetEvaluator evaluator = evaluator(policy(null, ZoneOffset.UTC), "2026-07-22T00:00:00Z"); | |
| when(store.getAccumulatedCost(any(), any())).thenReturn(usd("70.00")); | |
| when(store.getAccumulatedCost(any(), any())).thenReturn(usd(committed)); | |
| BudgetDecision result = evaluator.evaluate(TAGS, usd("10.00")); | |
| BudgetDecision result = evaluator.evaluate(TAGS, usd(candidate)); | |
| assertThat(result.state()).isEqualTo(BudgetState.WARN); | |
| assertThat(result.threshold()).isEqualTo(BudgetThreshold.WARNING); | |
| assertThat(result.state()).isEqualTo(expectedState); | |
| assertThat(result.threshold()).isEqualTo(expectedThreshold); | |
| assertThat(result.key()).isEqualTo(key("policy-a", "tenant-a", "2026-07")); | |
| assertThat(result.currentUsage()).isEqualTo(usd("80.00")); | |
| assertThat(result.evaluationType()).isEqualTo(EvaluationType.ADMISSION); | |
| assertThat(result.committedUsage()).isEqualTo(usd(committed)); | |
| assertThat(result.projectedUsage()).isEqualTo(usd(projected)); | |
| } | |
| `@ParameterizedTest` | |
| `@CsvSource`({ | |
| "10.00, 20.00, 30.00, ALLOW, NONE", | |
| "70.00, 10.00, 80.00, WARN, WARNING" | |
| }) | |
| void 한도_미만의_예상_비용을_포함해_ALLOW와_WARN을_판정한다( | |
| String committed, | |
| String candidate, | |
| String projected, | |
| BudgetState expectedState, | |
| BudgetThreshold expectedThreshold | |
| ) { | |
| DefaultBudgetEvaluator evaluator = evaluator(policy(null, ZoneOffset.UTC), "2026-07-22T00:00:00Z"); | |
| when(store.getAccumulatedCost(any(), any())).thenReturn(usd(committed)); | |
| BudgetDecision result = evaluator.evaluate(TAGS, usd(candidate)); | |
| assertThat(result.state()).isEqualTo(expectedState); | |
| assertThat(result.threshold()).isEqualTo(expectedThreshold); | |
| assertThat(result.key()).isEqualTo(key("policy-a", "tenant-a", "2026-07")); | |
| assertThat(result.evaluationType()).isEqualTo(EvaluationType.ADMISSION); | |
| assertThat(result.committedUsage()).isEqualTo(usd(committed)); | |
| assertThat(result.projectedUsage()).isEqualTo(usd(projected)); | |
| verify(store, never()).addCost(any(), any(), any()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@token-pilot-budget/src/test/java/io/tokenpilot/budget/internal/DefaultBudgetEvaluatorTest.java`
around lines 52 - 75, Extend the parameterized test
한도_미만의_예상_비용을_포함해_ALLOW와_WARN을_판정한다 to verify that store.addCost(...) is never
called after evaluator.evaluate(...). Match the existing no-call verification
used by the BLOCK test while preserving all current assertions.
Comment on lines
+99
to
+112
| BudgetDecision blockedDecision = evaluator.evaluate(tags, projectedCost); | ||
| if (blockedDecision.state() == BudgetState.BLOCK) { | ||
| return Map.of( | ||
| "enabled", "true", | ||
| "initialState", initialDecision.state().name(), | ||
| "blockedState", blockedDecision.state().name(), | ||
| "currentUsage", CostBoundaryFormatter.format(blockedDecision.currentUsage()), | ||
| "projectedUsage", CostBoundaryFormatter.format(blockedDecision.projectedUsage()), | ||
| "limit", CostBoundaryFormatter.format(blockedDecision.limit()) | ||
| ); | ||
| } | ||
| return Map.of( | ||
| "enabled", "true", | ||
| "initialState", initialDecision.state().name(), | ||
| "blockedState", "NONE" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
통화 불일치를 차단되지 않음으로 표시하지 마세요.
CURRENCY_MISMATCH는 provider 경계에서 fail-closed해야 하는 결정인데, 현재는 "blockedState":"NONE"으로 내려갑니다. BLOCK과 함께 실제 상태·사용량·한도를 반환하고 E2E 회귀 테스트를 추가하세요.
수정 예시
- if (blockedDecision.state() == BudgetState.BLOCK) {
+ if (blockedDecision.state() == BudgetState.BLOCK
+ || blockedDecision.state() == BudgetState.CURRENCY_MISMATCH) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| BudgetDecision blockedDecision = evaluator.evaluate(tags, projectedCost); | |
| if (blockedDecision.state() == BudgetState.BLOCK) { | |
| return Map.of( | |
| "enabled", "true", | |
| "initialState", initialDecision.state().name(), | |
| "blockedState", blockedDecision.state().name(), | |
| "currentUsage", CostBoundaryFormatter.format(blockedDecision.currentUsage()), | |
| "projectedUsage", CostBoundaryFormatter.format(blockedDecision.projectedUsage()), | |
| "limit", CostBoundaryFormatter.format(blockedDecision.limit()) | |
| ); | |
| } | |
| return Map.of( | |
| "enabled", "true", | |
| "initialState", initialDecision.state().name(), | |
| "blockedState", "NONE" | |
| BudgetDecision blockedDecision = evaluator.evaluate(tags, projectedCost); | |
| if (blockedDecision.state() == BudgetState.BLOCK | |
| || blockedDecision.state() == BudgetState.CURRENCY_MISMATCH) { | |
| return Map.of( | |
| "enabled", "true", | |
| "initialState", initialDecision.state().name(), | |
| "blockedState", blockedDecision.state().name(), | |
| "projectedUsage", CostBoundaryFormatter.format(blockedDecision.projectedUsage()), | |
| "limit", CostBoundaryFormatter.format(blockedDecision.limit()) | |
| ); | |
| } | |
| return Map.of( | |
| "enabled", "true", | |
| "initialState", initialDecision.state().name(), | |
| "blockedState", "NONE" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@token-pilot-sample-app/src/main/java/io/tokenpilot/sample/SampleController.java`
around lines 99 - 112, Update the decision handling in SampleController so
CURRENCY_MISMATCH is treated as a provider-boundary fail-closed outcome
alongside BLOCK, returning the actual blocked state, projected usage, and limit
instead of "blockedState":"NONE". Add an end-to-end regression test covering the
currency mismatch response.
HuitaePark
reviewed
Aug 3, 2026
Member
There was a problem hiding this comment.
전체적으로 방향이 좋습니다. evaluator를 순수 decision으로 만들고 provider boundary에서 BLOCK을 집행한 점, 원 decision 보존과 provider/ledger/budget mutation 0을 검증한 점까지 #25의 핵심 계약을 충족합니다.
한 가지 호환성만 확인 부탁드립니다. BudgetNotificationEvent의 record component를 currentUsage에서 projectedUsage로 바꾸면서 외부 BudgetNotificationHandler 구현에서 사용하던 event.currentUsage() accessor가 사라집니다. BudgetDecision에는 migration note가 있지만 notification 공개 API 변경에는 호환 경로나 안내가 없습니다.
가능하면 @Deprecated currentUsage()를 projectedUsage의 호환 accessor로 한 릴리스 유지해 주세요. 의도적인 breaking change라면 최소한 migration note와 release note에 명시해 주면 좋겠습니다.
16 tasks
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 399-401: Update the changelog entry heading associated with the
BudgetNotificationEvent.currentUsage() compatibility note to use the actual
change date, ensuring it is not later than the review date of 2026-08-03; use
2026-08-03 if this records the current change, or omit/defer the entry if the
change occurred on 2026-08-04.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f3e5bc3-509e-45a6-8f4e-42cd5a34db73
📒 Files selected for processing (3)
AGENTS.mdtoken-pilot-notification/src/main/java/io/tokenpilot/notification/BudgetNotificationEvent.javatoken-pilot-notification/src/test/java/io/tokenpilot/notification/BudgetNotificationServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- token-pilot-notification/src/test/java/io/tokenpilot/notification/BudgetNotificationServiceTest.java
Comment on lines
+399
to
+401
| ### 2026-08-04 | ||
|
|
||
| - Preserved the deprecated `BudgetNotificationEvent.currentUsage()` compatibility accessor through 0.1.x while migrating handlers to `projectedUsage()`; removal is planned for 0.2.0. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
업데이트 이력의 날짜를 실제 변경일과 일치시키세요.
현재 리뷰 기준일은 2026년 8월 3일입니다. 2026-08-04는 미래 날짜입니다. 이 항목이 현재 변경을 기록한다면 2026-08-03 또는 실제 변경일로 수정하세요. 실제 변경일이 2026년 8월 4일이라면 해당 날짜 이후에 항목을 추가하세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 399 - 401, Update the changelog entry heading
associated with the BudgetNotificationEvent.currentUsage() compatibility note to
use the actual change date, ensuring it is not later than the review date of
2026-08-03; use 2026-08-03 if this records the current change, or omit/defer the
entry if the change occurred on 2026-08-04.
Contributor
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
배경
Budget evaluator의 판단 책임과 provider 경계의 차단 책임이 섞여 있었고, legacy
DefaultLedgerAdvisor.before()가 BLOCK decision을 집행하지 않아 provider 호출이 진행될 수 있었습니다.변경 내용
BudgetDecision에STATUS / ADMISSION평가 유형과committedUsage / projectedUsage를 분리했습니다.DefaultBudgetEvaluator가 BLOCK과 CURRENCY_MISMATCH에서도 예외를 던지지 않고 구조화된 decision을 반환하도록 수정했습니다.projectedUsage >= limit을 BLOCK 경계로 고정했습니다.DefaultLedgerAdvisor.before()가 BLOCK이면 원래 decision을 보존한BudgetExceededException으로 provider 호출 전에 중단하도록 수정했습니다.projectedUsage로 통일했습니다.제외 범위
Closes #25