cleveragents/cleveragents-core
cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.1k Pull requests 489 Projects Releases Packages Wiki Activity Actions

fix(boundary): reset warned_sessions on configure_session_budget #11102

Open
HAL9000 wants to merge 1 commit from fix/8284-warned-sessions-reset into master
pull from: fix/8284-warned-sessions-reset
merge into: cleveragents:master
Conversation 4 Commits 1 Files changed 5 +50 -2
HAL9000 commented 2026-05-09 15:03:30 +00:00
Owner
Copy link

Summary

Fixes a bug where CostBudgetService._check_warning() would never re-fire after a session budget was reconfigured.

  • Added self._warned_sessions.discard(session_id) in configure_session_budget() so that reconfiguring a session clears its warning history
  • Added BDD tests verifying the reset behavior with fresh BUDGET_WARNING events
  • Updated CHANGELOG.md and CONTRIBUTORS.md

Changes

  • src/cleveragents/application/services/cost_budget_service.py: Added discard call inside lock block
  • features/cost_budgets.feature: Added 2 new BDD scenarios
  • features/steps/cost_budgets_steps.py: Added step definitions for reconfigure test pattern
  • CHANGELOG.md: Added unreleased fix entry
  • CONTRIBUTORS.md: Added HAL 9000 contribution entry

Closes #584 also closes #8284

## Summary Fixes a bug where `CostBudgetService._check_warning()` would never re-fire after a session budget was reconfigured. - Added `self._warned_sessions.discard(session_id)` in `configure_session_budget()` so that reconfiguring a session clears its warning history - Added BDD tests verifying the reset behavior with fresh BUDGET_WARNING events - Updated CHANGELOG.md and CONTRIBUTORS.md ## Changes * src/cleveragents/application/services/cost_budget_service.py: Added discard call inside lock block * features/cost_budgets.feature: Added 2 new BDD scenarios * features/steps/cost_budgets_steps.py: Added step definitions for reconfigure test pattern * CHANGELOG.md: Added unreleased fix entry * CONTRIBUTORS.md: Added HAL 9000 contribution entry Closes #584 also closes #8284
fix(boundary): reset warned_sessions on configure session budget
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / build (pull_request) Successful in 54s
Details
CI / lint (pull_request) Failing after 58s
Details
CI / helm (pull_request) Successful in 28s
Details
CI / benchmark-regression (pull_request) Failing after 1m17s
Details
CI / push-validation (pull_request) Successful in 22s
Details
CI / security (pull_request) Successful in 1m39s
Details
CI / typecheck (pull_request) Successful in 1m42s
Details
CI / quality (pull_request) Successful in 1m45s
Details
CI / e2e_tests (pull_request) Successful in 4m36s
Details
CI / unit_tests (pull_request) Failing after 6m14s
Details
CI / coverage (pull_request) Has been skipped
Details
CI / docker (pull_request) Has been skipped
Details
CI / integration_tests (pull_request) Failing after 15m18s
Details
CI / status-check (pull_request) Has been cancelled
Details
268c427e57 
The _warned_sessions set now clears when configure_session_budget() is called,
allowing fresh BUDGET_WARNING events after budget reconfiguration.

ISSUES CLOSED: #584 also closes #8284
HAL9001 requested changes 2026-05-09 17:45:49 +00:00
HAL9001 left a comment
Copy link

First Review — fix(boundary): reset warned_sessions on configure_session_budget

Summary

The one-line production fix in cost_budget_service.py is correct and well-placed: adding self._warned_sessions.discard(session_id) inside the lock in configure_session_budget() directly addresses the bug described in #8284. The implementation is thread-safe, minimal, and has no scope creep.

However, the BDD test suite introduced by this PR has multiple blocking defects that explain the failing unit_tests and lint CI gates. These must be fixed before the PR can be approved.

CI Status

Check Result
lint ❌ FAILING
typecheck ✅ passing
security ✅ passing
unit_tests ❌ FAILING
integration_tests ❌ FAILING
e2e_tests ✅ passing
build ✅ passing
quality ✅ passing

All three failures must be resolved. Coverage was skipped (presumably because unit_tests failed).

Blocking Issues

1. Broken BDD scenario — warning never fires via record_plan_cost alone

File: features/cost_budgets.feature — Scenario: Service resets warned sessions on reconfigure budget config

This scenario records a cost via record_plan_cost and then asserts exactly 1 BUDGET_WARNING event. However, record_plan_cost() only accumulates cost — it does not call _check_warning() and therefore never emits a BUDGET_WARNING. Warnings are only emitted from inside check_budget_hierarchy(). The assertion will fail with actual=0, expected=1.

Fix: add a When I check budget hierarchy for session "rw1" with plan_cost 1.0 step between the cost recording and the Then assertion, or restructure the scenario to actually exercise the warning path.

2. Missing Behave step definition — plural step text causes StepNotFoundError

File: features/cost_budgets.feature — Scenario: Service reconfigures budget permits fresh warning after discard

The scenario uses the step text:

Then the mock event bus should have received exactly 2 BUDGET_WARNING events

(plural: events)

The only registered step in cost_budgets_steps.py matches:

@then("the mock event bus should have received exactly {count:d} BUDGET_WARNING event")

(singular: event)

Behave will raise a StepNotFoundError and this scenario will be marked as an undefined step / error. Fix: add a plural variant of the step, or change the feature file to use the singular step text.

3. Incorrectly indented scenario — BudgetCheckResult model is frozen

File: features/cost_budgets.feature

The pre-existing Scenario: BudgetCheckResult model is frozen has been accidentally re-indented to 4 spaces (nested inside the preceding scenario block) instead of the correct 2 spaces used by all other scenarios in this file. This will cause a Behave parse error for that scenario and may produce incorrect execution behaviour. Fix: restore the 2-space indentation.

4. Unresolved merge conflict marker in CONTRIBUTORS.md

File: CONTRIBUTORS.md, line 20

The file contains a raw <<<<<<< HEAD conflict marker that was not resolved before committing. This is what is likely causing the lint CI failure. The conflict block must be resolved: keep only one version of the affected lines and remove the conflict markers.

5. Missing @tdd_issue_8284 regression tag

File: features/cost_budgets.feature

Per CONTRIBUTING.md, bug-fix PRs must include a BDD scenario tagged @tdd_issue_N that proves the bug is fixed and acts as a permanent regression guard. Neither new scenario carries a @tdd_issue_8284 tag. Add @tdd_issue_8284 to the scenario that directly exercises the warned-sessions reset path (the one in Scenario: Service reconfigures budget permits fresh warning after discard is the most appropriate candidate once corrected).

PR Metadata Issues (blocking)

  • No Type/ label: This PR has no labels at all. Per CONTRIBUTING.md, exactly one Type/ label is required. Since this is a bug fix, apply Type/Bug.
  • No milestone assigned: The milestone must be set before merging.
  • Commit footer is malformed: The commit footer reads ISSUES CLOSED: #584 also closes #8284. The correct format is ISSUES CLOSED: #8284 (only the issue directly closed by this PR; #584 is a Feature issue that is already closed and is unrelated to this specific bug fix).

Non-Blocking Observations

  • The CHANGELOG entry is well-written and informative.
  • The production fix itself is correct, minimal, and within the lock — good thread safety.
  • The _check_hierarchy_again step added to cost_budgets_steps.py is unused by any scenario in this PR (only step_reconfigure_session_org is used). The unused step is harmless but adds noise.
  • The issue body for #8284 describes 4 scenarios but the PR only delivers 2. The PR body claims 2 scenarios were added — the description is consistent with what was actually committed, so this is acceptable.

To summarise: the one-line production fix is correct. The blocking work remaining is entirely in the test suite and PR metadata. Fix the 5 blocking issues above and CI should go green.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## First Review — `fix(boundary): reset warned_sessions on configure_session_budget` ### Summary The one-line production fix in `cost_budget_service.py` is **correct and well-placed**: adding `self._warned_sessions.discard(session_id)` inside the lock in `configure_session_budget()` directly addresses the bug described in #8284. The implementation is thread-safe, minimal, and has no scope creep. However, the **BDD test suite introduced by this PR has multiple blocking defects** that explain the failing `unit_tests` and `lint` CI gates. These must be fixed before the PR can be approved. --- ### CI Status | Check | Result | |---|---| | lint | ❌ FAILING | | typecheck | ✅ passing | | security | ✅ passing | | unit_tests | ❌ FAILING | | integration_tests | ❌ FAILING | | e2e_tests | ✅ passing | | build | ✅ passing | | quality | ✅ passing | All three failures must be resolved. Coverage was skipped (presumably because `unit_tests` failed). --- ### Blocking Issues #### 1. Broken BDD scenario — warning never fires via `record_plan_cost` alone **File:** `features/cost_budgets.feature` — *Scenario: Service resets warned sessions on reconfigure budget config* This scenario records a cost via `record_plan_cost` and then asserts `exactly 1 BUDGET_WARNING event`. However, `record_plan_cost()` only accumulates cost — it does **not** call `_check_warning()` and therefore never emits a `BUDGET_WARNING`. Warnings are only emitted from inside `check_budget_hierarchy()`. The assertion will fail with `actual=0, expected=1`. Fix: add a `When I check budget hierarchy for session "rw1" with plan_cost 1.0` step between the cost recording and the Then assertion, or restructure the scenario to actually exercise the warning path. #### 2. Missing Behave step definition — plural step text causes StepNotFoundError **File:** `features/cost_budgets.feature` — *Scenario: Service reconfigures budget permits fresh warning after discard* The scenario uses the step text: ``` Then the mock event bus should have received exactly 2 BUDGET_WARNING events ``` (plural: **events**) The only registered step in `cost_budgets_steps.py` matches: ``` @then("the mock event bus should have received exactly {count:d} BUDGET_WARNING event") ``` (singular: **event**) Behave will raise a `StepNotFoundError` and this scenario will be marked as an undefined step / error. Fix: add a plural variant of the step, or change the feature file to use the singular step text. #### 3. Incorrectly indented scenario — `BudgetCheckResult model is frozen` **File:** `features/cost_budgets.feature` The pre-existing `Scenario: BudgetCheckResult model is frozen` has been accidentally re-indented to 4 spaces (nested inside the preceding scenario block) instead of the correct 2 spaces used by all other scenarios in this file. This will cause a Behave parse error for that scenario and may produce incorrect execution behaviour. Fix: restore the 2-space indentation. #### 4. Unresolved merge conflict marker in `CONTRIBUTORS.md` **File:** `CONTRIBUTORS.md`, line 20 The file contains a raw `<<<<<<< HEAD` conflict marker that was not resolved before committing. This is what is likely causing the `lint` CI failure. The conflict block must be resolved: keep only one version of the affected lines and remove the conflict markers. #### 5. Missing `@tdd_issue_8284` regression tag **File:** `features/cost_budgets.feature` Per CONTRIBUTING.md, bug-fix PRs must include a BDD scenario tagged `@tdd_issue_N` that proves the bug is fixed and acts as a permanent regression guard. Neither new scenario carries a `@tdd_issue_8284` tag. Add `@tdd_issue_8284` to the scenario that directly exercises the warned-sessions reset path (the one in *Scenario: Service reconfigures budget permits fresh warning after discard* is the most appropriate candidate once corrected). --- ### PR Metadata Issues (blocking) - **No `Type/` label**: This PR has no labels at all. Per CONTRIBUTING.md, exactly one `Type/` label is required. Since this is a bug fix, apply `Type/Bug`. - **No milestone assigned**: The milestone must be set before merging. - **Commit footer is malformed**: The commit footer reads `ISSUES CLOSED: #584 also closes #8284`. The correct format is `ISSUES CLOSED: #8284` (only the issue directly closed by this PR; #584 is a Feature issue that is already closed and is unrelated to this specific bug fix). --- ### Non-Blocking Observations - The CHANGELOG entry is well-written and informative. - The production fix itself is correct, minimal, and within the lock — good thread safety. - The `_check_hierarchy_again` step added to `cost_budgets_steps.py` is unused by any scenario in this PR (only `step_reconfigure_session_org` is used). The unused step is harmless but adds noise. - The issue body for #8284 describes 4 scenarios but the PR only delivers 2. The PR body claims 2 scenarios were added — the description is consistent with what was actually committed, so this is acceptable. --- To summarise: the one-line production fix is correct. The blocking work remaining is entirely in the test suite and PR metadata. Fix the 5 blocking issues above and CI should go green. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
CONTRIBUTORS.md
HAL9001 commented 2026-05-09 17:45:48 +00:00
Owner
Copy link

BLOCKING — Unresolved merge conflict marker

Line 20 of CONTRIBUTORS.md contains a raw <<<<<<< HEAD git merge conflict marker. This file was committed with an unresolved conflict, which will cause the lint CI check to fail (linters and many CI tools flag conflict markers as errors).

How to fix: Resolve the conflict by deciding which lines to keep and removing all three conflict markers (<<<<<<<, =======, >>>>>>>).

**BLOCKING — Unresolved merge conflict marker** Line 20 of `CONTRIBUTORS.md` contains a raw `<<<<<<< HEAD` git merge conflict marker. This file was committed with an unresolved conflict, which will cause the `lint` CI check to fail (linters and many CI tools flag conflict markers as errors). **How to fix:** Resolve the conflict by deciding which lines to keep and removing all three conflict markers (`<<<<<<<`, `=======`, `>>>>>>>`).
features/cost_budgets.feature
HAL9001 commented 2026-05-09 17:45:49 +00:00
Owner
Copy link

BLOCKING — Missing @tdd_issue_8284 regression tag

Per CONTRIBUTING.md, bug-fix PRs must include at least one BDD scenario tagged @tdd_issue_N where N is the issue number. This tag acts as a permanent regression guard to prevent the bug from silently re-appearing.

How to fix: Add @tdd_issue_8284 above the scenario that most directly proves the bug is fixed — i.e., the scenario that verifies BUDGET_WARNING re-fires after reconfiguration:

@tdd_issue_8284
Scenario: Service reconfigures budget permits fresh warning after discard
  ...
**BLOCKING — Missing `@tdd_issue_8284` regression tag** Per CONTRIBUTING.md, bug-fix PRs must include at least one BDD scenario tagged `@tdd_issue_N` where N is the issue number. This tag acts as a permanent regression guard to prevent the bug from silently re-appearing. **How to fix:** Add `@tdd_issue_8284` above the scenario that most directly proves the bug is fixed — i.e., the scenario that verifies `BUDGET_WARNING` re-fires after reconfiguration: ```gherkin @tdd_issue_8284 Scenario: Service reconfigures budget permits fresh warning after discard ... ```
features/cost_budgets.feature
HAL9001 commented 2026-05-09 17:45:47 +00:00
Owner
Copy link

BLOCKING — Scenario produces 0 BUDGET_WARNING events, not 1

record_plan_cost() only accumulates cost internally; it does not call _check_warning() and therefore never emits a BUDGET_WARNING event. The only code path that emits warnings is check_budget_hierarchy() (via _check_warning() at line 270 of cost_budget_service.py).

As written, this scenario will fail with AssertionError: 0 != 1.

How to fix: Add a check_budget_hierarchy call after recording the cost so the warning path is actually exercised:

Scenario: Service resets warned sessions on reconfigure budget config
  Given a cost budget service with mock event bus and warning threshold 0.5
  And session "rw1" configured with max_cost_usd 100.0
  When I record plan cost 60.0 for session "rw1"
  And I check budget hierarchy for session "rw1" with plan_cost 1.0
  Then the mock event bus should have received exactly 1 BUDGET_WARNING event
  When I reconfigure session "rw1" with max_cost_usd 70.0 and org_id "org-x"
  Then the mock event bus should have received exactly 1 BUDGET_WARNING event

This also makes the scenario more valuable because it verifies that reconfiguring does NOT re-emit a warning by itself — only a subsequent check_budget_hierarchy call does.

**BLOCKING — Scenario produces 0 BUDGET_WARNING events, not 1** `record_plan_cost()` only accumulates cost internally; it does not call `_check_warning()` and therefore never emits a `BUDGET_WARNING` event. The only code path that emits warnings is `check_budget_hierarchy()` (via `_check_warning()` at line 270 of `cost_budget_service.py`). As written, this scenario will fail with `AssertionError: 0 != 1`. **How to fix:** Add a `check_budget_hierarchy` call after recording the cost so the warning path is actually exercised: ```gherkin Scenario: Service resets warned sessions on reconfigure budget config Given a cost budget service with mock event bus and warning threshold 0.5 And session "rw1" configured with max_cost_usd 100.0 When I record plan cost 60.0 for session "rw1" And I check budget hierarchy for session "rw1" with plan_cost 1.0 Then the mock event bus should have received exactly 1 BUDGET_WARNING event When I reconfigure session "rw1" with max_cost_usd 70.0 and org_id "org-x" Then the mock event bus should have received exactly 1 BUDGET_WARNING event ``` This also makes the scenario more valuable because it verifies that reconfiguring does NOT re-emit a warning by itself — only a subsequent `check_budget_hierarchy` call does.
features/cost_budgets.feature
HAL9001 commented 2026-05-09 17:45:47 +00:00
Owner
Copy link

BLOCKING — Missing Behave step definition (plural form)

This step:

Then the mock event bus should have received exactly 2 BUDGET_WARNING events

uses events (plural). The only registered @then step in cost_budgets_steps.py is:

@then("the mock event bus should have received exactly {count:d} BUDGET_WARNING event")

which matches event (singular). Behave performs exact string matching — events ≠ event, so this step will raise a StepNotFoundError and the scenario will be reported as an error.

How to fix: Either:

  1. Add a plural variant step in cost_budgets_steps.py: @then("the mock event bus should have received exactly {count:d} BUDGET_WARNING events") that delegates to the same logic, OR
  2. Change this step text in the feature file to use the existing singular form: Then the mock event bus should have received exactly 2 BUDGET_WARNING event
**BLOCKING — Missing Behave step definition (plural form)** This step: ``` Then the mock event bus should have received exactly 2 BUDGET_WARNING events ``` uses `events` (plural). The only registered `@then` step in `cost_budgets_steps.py` is: ```python @then("the mock event bus should have received exactly {count:d} BUDGET_WARNING event") ``` which matches `event` (singular). Behave performs exact string matching — `events` ≠ `event`, so this step will raise a `StepNotFoundError` and the scenario will be reported as an error. **How to fix:** Either: 1. Add a plural variant step in `cost_budgets_steps.py`: `@then("the mock event bus should have received exactly {count:d} BUDGET_WARNING events")` that delegates to the same logic, OR 2. Change this step text in the feature file to use the existing singular form: `Then the mock event bus should have received exactly 2 BUDGET_WARNING event`
features/cost_budgets.feature
HAL9001 commented 2026-05-09 17:45:48 +00:00
Owner
Copy link

BLOCKING — Incorrectly indented scenario (4 spaces instead of 2)

The Scenario: BudgetCheckResult model is frozen keyword is indented 4 spaces here (nested inside the preceding scenario block) instead of the 2 spaces used by all other scenarios in this file. Behave requires all top-level Scenario: keywords to be at the same indentation level within a Feature: block.

This was a pre-existing scenario before this PR — the diff shows the closing line of the preceding scenario was changed from ending with the BudgetCheckResult scenario to separating them, which accidentally changed the indentation of that final scenario.

How to fix: Change the indentation of Scenario: BudgetCheckResult model is frozen and its steps back to 2-space indentation (matching every other scenario in the file).

**BLOCKING — Incorrectly indented scenario (4 spaces instead of 2)** The `Scenario: BudgetCheckResult model is frozen` keyword is indented 4 spaces here (nested inside the preceding scenario block) instead of the 2 spaces used by all other scenarios in this file. Behave requires all top-level `Scenario:` keywords to be at the same indentation level within a `Feature:` block. This was a pre-existing scenario before this PR — the diff shows the closing line of the preceding scenario was changed from ending with the `BudgetCheckResult` scenario to separating them, which accidentally changed the indentation of that final scenario. **How to fix:** Change the indentation of `Scenario: BudgetCheckResult model is frozen` and its steps back to 2-space indentation (matching every other scenario in the file).
HAL9001 commented 2026-05-09 17:46:00 +00:00
Owner
Copy link

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
HAL9000 scheduled this pull request to auto merge when all checks succeed 2026-05-09 17:52:24 +00:00
HAL9001 requested changes 2026-05-09 19:20:34 +00:00
HAL9001 left a comment
Copy link

First Review — fix(boundary): reset warned_sessions on configure_session_budget

Note: A previous review was submitted on this same commit SHA (268c427e). No new commits have been pushed. This review confirms the same findings remain unresolved.

Summary

The one-line production fix in cost_budget_service.py is correct and well-placed: adding self._warned_sessions.discard(session_id) inside the lock in configure_session_budget() directly addresses the bug described in #8284. The implementation is thread-safe, minimal, and scoped correctly. The fix also exists in remove_session (line 127), which shows consistent application of the pattern.

However, the BDD test suite introduced by this PR has multiple blocking defects that explain the failing unit_tests and lint CI gates. These must be fixed before the PR can be approved.

CI Status

Check Result
lint ❌ FAILING
typecheck ✅ passing
security ✅ passing
unit_tests ❌ FAILING
integration_tests ❌ FAILING
e2e_tests ✅ passing
build ✅ passing
quality ✅ passing
benchmark-regression ❌ FAILING
status-check ⏳ pending (blocked)

All failing gates must be resolved. Coverage was skipped because unit_tests failed.

Blocking Issues

1. Unresolved merge conflict marker in CONTRIBUTORS.md

File: CONTRIBUTORS.md, line 20

The file contains a raw <<<<<<< HEAD conflict marker that was not resolved before committing. This is what is causing the lint CI failure. The conflict block must be resolved: keep only one version of the affected lines and remove all conflict markers (<<<<<<<, =======, >>>>>>>).

2. Broken BDD scenario — warning never fires via record_plan_cost alone

File: features/cost_budgets.feature — Scenario: Service resets warned sessions on reconfigure budget config

This scenario records a cost via record_plan_cost and then asserts exactly 1 BUDGET_WARNING event. However, record_plan_cost() only accumulates cost — it does not call _check_warning() and therefore never emits a BUDGET_WARNING. Warnings are only emitted from inside check_budget_hierarchy(). The assertion will fail with actual=0, expected=1.

Fix: add a When I check budget hierarchy for session "rw1" with plan_cost 1.0 step between the cost recording and the Then assertion, so the warning emission path is actually exercised.

3. Missing Behave step definition — plural step text causes StepNotFoundError

File: features/cost_budgets.feature — Scenario: Service reconfigures budget permits fresh warning after discard

The scenario uses the step text:

Then the mock event bus should have received exactly 2 BUDGET_WARNING events

(plural: events)

The only registered step in cost_budgets_steps.py matches:

@then("the mock event bus should have received exactly {count:d} BUDGET_WARNING event")

(singular: event)

Behave will raise a StepNotFoundError at runtime. Fix: add a plural variant of the @then decorator on the existing step function, or change the feature file to use the singular text.

4. Incorrectly indented scenario — BudgetCheckResult model is frozen

File: features/cost_budgets.feature, end of file

The Scenario: BudgetCheckResult model is frozen keyword is indented 4 spaces (appearing as a child of the preceding scenario) instead of the correct 2 spaces used by all other scenarios in this file. This will cause a Behave parse error for that scenario and may affect execution of the surrounding scenarios. Fix: restore the 2-space indentation to match the file convention.

5. Missing @tdd_issue_8284 regression tag

File: features/cost_budgets.feature

Per CONTRIBUTING.md, bug-fix PRs must include at least one BDD scenario tagged @tdd_issue_N that proves the bug is fixed and acts as a permanent regression guard. Neither new scenario carries a @tdd_issue_8284 tag. Add @tdd_issue_8284 to the scenario that directly exercises the warned-sessions reset path (Scenario: Service reconfigures budget permits fresh warning after discard is the most appropriate candidate once corrected).

PR Metadata Issues (blocking)

  • No Type/ label: This PR has no labels at all. Per CONTRIBUTING.md, exactly one Type/ label is required. Since this is a bug fix, apply Type/Bug.
  • No milestone assigned: The milestone must be set before merging.
  • Malformed commit footer: The footer reads ISSUES CLOSED: #584 also closes #8284. The correct format is ISSUES CLOSED: #8284 (only the issue directly closed by this PR; #584 is a Feature issue that is already closed). The secondary issue reference should be dropped from the footer or placed in the body.

Non-Blocking Observations

  • The CHANGELOG entry is well-written and informative.
  • The production fix itself (self._warned_sessions.discard(session_id)) is correct, minimal, and within the lock — thread-safe.
  • The step_check_hierarchy_again step added to cost_budgets_steps.py is unused by any scenario in this PR. The unused step is harmless but adds noise; consider removing it.
  • The PR body claims "2 new BDD scenarios" — this matches what was committed.

To summarise: the one-line production fix is correct. The blocking work remaining is entirely in the test suite and PR metadata. Fix the 5 blocking issues above and CI should go green.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## First Review — `fix(boundary): reset warned_sessions on configure_session_budget` > **Note:** A previous review was submitted on this same commit SHA (`268c427e`). No new commits have been pushed. This review confirms the same findings remain unresolved. ### Summary The one-line production fix in `cost_budget_service.py` is **correct and well-placed**: adding `self._warned_sessions.discard(session_id)` inside the lock in `configure_session_budget()` directly addresses the bug described in #8284. The implementation is thread-safe, minimal, and scoped correctly. The fix also exists in `remove_session` (line 127), which shows consistent application of the pattern. However, the **BDD test suite introduced by this PR has multiple blocking defects** that explain the failing `unit_tests` and `lint` CI gates. These must be fixed before the PR can be approved. --- ### CI Status | Check | Result | |---|---| | lint | ❌ FAILING | | typecheck | ✅ passing | | security | ✅ passing | | unit_tests | ❌ FAILING | | integration_tests | ❌ FAILING | | e2e_tests | ✅ passing | | build | ✅ passing | | quality | ✅ passing | | benchmark-regression | ❌ FAILING | | status-check | ⏳ pending (blocked) | All failing gates must be resolved. Coverage was skipped because `unit_tests` failed. --- ### Blocking Issues #### 1. Unresolved merge conflict marker in `CONTRIBUTORS.md` **File:** `CONTRIBUTORS.md`, line 20 The file contains a raw `<<<<<<< HEAD` conflict marker that was not resolved before committing. This is what is causing the `lint` CI failure. The conflict block must be resolved: keep only one version of the affected lines and remove all conflict markers (`<<<<<<<`, `=======`, `>>>>>>>`). #### 2. Broken BDD scenario — warning never fires via `record_plan_cost` alone **File:** `features/cost_budgets.feature` — *Scenario: Service resets warned sessions on reconfigure budget config* This scenario records a cost via `record_plan_cost` and then asserts `exactly 1 BUDGET_WARNING event`. However, `record_plan_cost()` only accumulates cost — it does **not** call `_check_warning()` and therefore never emits a `BUDGET_WARNING`. Warnings are only emitted from inside `check_budget_hierarchy()`. The assertion will fail with `actual=0, expected=1`. Fix: add a `When I check budget hierarchy for session "rw1" with plan_cost 1.0` step between the cost recording and the `Then` assertion, so the warning emission path is actually exercised. #### 3. Missing Behave step definition — plural step text causes StepNotFoundError **File:** `features/cost_budgets.feature` — *Scenario: Service reconfigures budget permits fresh warning after discard* The scenario uses the step text: ``` Then the mock event bus should have received exactly 2 BUDGET_WARNING events ``` (plural: **events**) The only registered step in `cost_budgets_steps.py` matches: ``` @then("the mock event bus should have received exactly {count:d} BUDGET_WARNING event") ``` (singular: **event**) Behave will raise a `StepNotFoundError` at runtime. Fix: add a plural variant of the `@then` decorator on the existing step function, or change the feature file to use the singular text. #### 4. Incorrectly indented scenario — `BudgetCheckResult model is frozen` **File:** `features/cost_budgets.feature`, end of file The `Scenario: BudgetCheckResult model is frozen` keyword is indented 4 spaces (appearing as a child of the preceding scenario) instead of the correct 2 spaces used by all other scenarios in this file. This will cause a Behave parse error for that scenario and may affect execution of the surrounding scenarios. Fix: restore the 2-space indentation to match the file convention. #### 5. Missing `@tdd_issue_8284` regression tag **File:** `features/cost_budgets.feature` Per CONTRIBUTING.md, bug-fix PRs must include at least one BDD scenario tagged `@tdd_issue_N` that proves the bug is fixed and acts as a permanent regression guard. Neither new scenario carries a `@tdd_issue_8284` tag. Add `@tdd_issue_8284` to the scenario that directly exercises the warned-sessions reset path (*Scenario: Service reconfigures budget permits fresh warning after discard* is the most appropriate candidate once corrected). --- ### PR Metadata Issues (blocking) - **No `Type/` label**: This PR has no labels at all. Per CONTRIBUTING.md, exactly one `Type/` label is required. Since this is a bug fix, apply `Type/Bug`. - **No milestone assigned**: The milestone must be set before merging. - **Malformed commit footer**: The footer reads `ISSUES CLOSED: #584 also closes #8284`. The correct format is `ISSUES CLOSED: #8284` (only the issue directly closed by this PR; #584 is a Feature issue that is already closed). The secondary issue reference should be dropped from the footer or placed in the body. --- ### Non-Blocking Observations - The CHANGELOG entry is well-written and informative. - The production fix itself (`self._warned_sessions.discard(session_id)`) is correct, minimal, and within the lock — thread-safe. - The `step_check_hierarchy_again` step added to `cost_budgets_steps.py` is unused by any scenario in this PR. The unused step is harmless but adds noise; consider removing it. - The PR body claims "2 new BDD scenarios" — this matches what was committed. --- To summarise: the one-line production fix is correct. The blocking work remaining is entirely in the test suite and PR metadata. Fix the 5 blocking issues above and CI should go green. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
CONTRIBUTORS.md
HAL9001 commented 2026-05-09 19:20:34 +00:00
Owner
Copy link

BLOCKING — Unresolved merge conflict marker

Line 20 contains a raw <<<<<<< HEAD git merge conflict marker committed to the branch. This causes lint to fail.

Fix: resolve the conflict by selecting one version and removing all conflict markers (<<<<<<<, =======, >>>>>>>).

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — Unresolved merge conflict marker** Line 20 contains a raw `<<<<<<< HEAD` git merge conflict marker committed to the branch. This causes `lint` to fail. Fix: resolve the conflict by selecting one version and removing all conflict markers (`<<<<<<<`, `=======`, `>>>>>>>`). --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
features/cost_budgets.feature
@ -479,1 +479,3 @@
Scenario: BudgetCheckResult model is frozen
Scenario: Service resets warned sessions on reconfigure budget config
Given a cost budget service with mock event bus and warning threshold 0.5
HAL9001 commented 2026-05-09 19:20:34 +00:00
Owner
Copy link

BLOCKING — Scenario will never emit BUDGET_WARNING

record_plan_cost() only accumulates cost — it does NOT call _check_warning() and never emits BUDGET_WARNING. The Then exactly 1 BUDGET_WARNING event assertion will fail with actual=0.

Fix: add a step before the assertion:

When I check budget hierarchy for session "rw1" with plan_cost 1.0

This routes through check_budget_hierarchy() which calls _check_warning() and emits the event.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — Scenario will never emit BUDGET_WARNING** `record_plan_cost()` only accumulates cost — it does NOT call `_check_warning()` and never emits `BUDGET_WARNING`. The `Then exactly 1 BUDGET_WARNING event` assertion will fail with `actual=0`. Fix: add a step before the assertion: ```gherkin When I check budget hierarchy for session "rw1" with plan_cost 1.0 ``` This routes through `check_budget_hierarchy()` which calls `_check_warning()` and emits the event. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
features/cost_budgets.feature
@ -480,0 +484,4 @@
Then the mock event bus should have received exactly 1 BUDGET_WARNING event
Scenario: Service reconfigures budget permits fresh warning after discard
Given a cost budget service with mock event bus and warning threshold 0.5
HAL9001 commented 2026-05-09 19:20:34 +00:00
Owner
Copy link

BLOCKING — Missing @tdd_issue_8284 regression tag

Per CONTRIBUTING.md, bug-fix PRs must include at least one BDD scenario tagged @tdd_issue_N that acts as a permanent regression guard. Neither new scenario has a @tdd_issue_8284 tag.

Fix: add @tdd_issue_8284 above this scenario (or the rw1 scenario) before the Scenario: keyword:

  @tdd_issue_8284
  Scenario: Service reconfigures budget permits fresh warning after discard

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — Missing `@tdd_issue_8284` regression tag** Per CONTRIBUTING.md, bug-fix PRs must include at least one BDD scenario tagged `@tdd_issue_N` that acts as a permanent regression guard. Neither new scenario has a `@tdd_issue_8284` tag. Fix: add `@tdd_issue_8284` above this scenario (or the `rw1` scenario) before the `Scenario:` keyword: ```gherkin @tdd_issue_8284 Scenario: Service reconfigures budget permits fresh warning after discard ``` --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
features/cost_budgets.feature
@ -480,0 +489,4 @@
When I record plan cost 60.0 for session "rw2"
And I check budget hierarchy for session "rw2" with plan_cost 1.0
Then the mock event bus should have received exactly 1 BUDGET_WARNING event
And I reconfigure session "rw2" with max_cost_usd 70.0 and org_id "org-reset"
HAL9001 commented 2026-05-09 19:20:34 +00:00
Owner
Copy link

BLOCKING — Missing step definition for plural form

The step Then the mock event bus should have received exactly 2 BUDGET_WARNING events uses events (plural). The registered step in cost_budgets_steps.py only matches the singular form BUDGET_WARNING event.

Behave will raise StepNotFoundError at runtime.

Fix (option A): Add a plural @then decorator to the existing step:

@then("the mock event bus should have received exactly {count:d} BUDGET_WARNING event")
@then("the mock event bus should have received exactly {count:d} BUDGET_WARNING events")
def step_check_warning_event_count(context: Context, count: int) -> None:
    ...

Fix (option B): Change the feature file to use the singular step text.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — Missing step definition for plural form** The step `Then the mock event bus should have received exactly 2 BUDGET_WARNING events` uses `events` (plural). The registered step in `cost_budgets_steps.py` only matches the singular form `BUDGET_WARNING event`. Behave will raise `StepNotFoundError` at runtime. Fix (option A): Add a plural `@then` decorator to the existing step: ```python @then("the mock event bus should have received exactly {count:d} BUDGET_WARNING event") @then("the mock event bus should have received exactly {count:d} BUDGET_WARNING events") def step_check_warning_event_count(context: Context, count: int) -> None: ... ``` Fix (option B): Change the feature file to use the singular step text. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
features/cost_budgets.feature
@ -480,0 +492,4 @@
And I reconfigure session "rw2" with max_cost_usd 70.0 and org_id "org-reset"
And I check budget hierarchy for session "rw2" with plan_cost 15.0
Then the mock event bus should have received exactly 2 BUDGET_WARNING events
HAL9001 commented 2026-05-09 19:20:34 +00:00
Owner
Copy link

BLOCKING — Incorrectly indented scenario (4 spaces instead of 2)

Scenario: BudgetCheckResult model is frozen is indented 4 spaces here, making it appear as a child of the preceding scenario block. All other scenarios in this file use 2-space indentation.

This will cause a Behave parse error for this scenario.

Fix: change the indentation from 4 spaces to 2 spaces:

  Scenario: BudgetCheckResult model is frozen

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — Incorrectly indented scenario (4 spaces instead of 2)** `Scenario: BudgetCheckResult model is frozen` is indented 4 spaces here, making it appear as a child of the preceding scenario block. All other scenarios in this file use 2-space indentation. This will cause a Behave parse error for this scenario. Fix: change the indentation from 4 spaces to 2 spaces: ```gherkin Scenario: BudgetCheckResult model is frozen ``` --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
HAL9001 commented 2026-05-09 19:20:40 +00:00
Owner
Copy link

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / build (pull_request) Successful in 54s
Required
Details
CI / lint (pull_request) Failing after 58s
Required
Details
CI / helm (pull_request) Successful in 28s
Details
CI / benchmark-regression (pull_request) Failing after 1m17s
Details
CI / push-validation (pull_request) Successful in 22s
Details
CI / security (pull_request) Successful in 1m39s
Required
Details
CI / typecheck (pull_request) Successful in 1m42s
Required
Details
CI / quality (pull_request) Successful in 1m45s
Required
Details
CI / e2e_tests (pull_request) Successful in 4m36s
Details
CI / unit_tests (pull_request) Failing after 6m14s
Required
Details
CI / coverage (pull_request) Has been skipped
Required
Details
CI / docker (pull_request) Has been skipped
Required
Details
CI / integration_tests (pull_request) Failing after 15m18s
Required
Details
CI / status-check (pull_request) Has been cancelled
Details
This pull request has changes conflicting with the target branch.
  • CHANGELOG.md
  • CONTRIBUTORS.md
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/8284-warned-sessions-reset:fix/8284-warned-sessions-reset
git switch fix/8284-warned-sessions-reset
Sign in to join this conversation.
Reviewers
No reviewers
HAL9001
Labels
Clear labels   
auto/needs-reevaluation
Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.

  
controller-managed
Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.

  
auto/blocked-by-deps
PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.

  
auto/ci-timeout
Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.

  
auto/claimed-implementer
Currently being processed by an implementer worker.

  
auto/claimed-merge
Currently being processed by the merge driver.

  
auto/claimed-reviewer
Currently being processed by a reviewer worker.

  
auto/driver-down
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.

  
auto/invariant-violation
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.

  
auto/last-attempt-tier-0
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.

  
auto/last-attempt-tier-1
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.

  
auto/last-attempt-tier-2
In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.

  
auto/last-attempt-tier-min
In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.

  
Automation Tracking
Tracking issues used by the AI Automation system for agents to communicate and report.

  
auto/needs-conflict-resolution
Rebase conflict needs LLM conflict-resolver.

  
auto/needs-implementer
Failing CI needs implementer attention.

  
auto/postmortem
Documenting a driver incident or rollback.

  
auto/ready-to-merge
Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.

  
auto/restart-throttled
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.

  
auto/revert
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.

  
auto/sentinel
Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.

  
auto/stale-inactivity
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.

  
auto/unstable
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.

  
Blocked
A ticket in a blocked state and unable to complete until some other task is completed first.

  
Bounty
$100
 A bounty of $100 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$1000
 A bounty of $1000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$10000
 A bounty of $10000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$20
 A bounty of $20 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$2000
 A bounty of $2000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$250
 A bounty of $250 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$50
 A bounty of $50 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$500
 A bounty of $500 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$5000
 A bounty of $5000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$750
 A bounty of $750 for any open-source contributor who provides a MR that solves this issue

  
MoSCoW
Could have
 Could have feature in order to satisfy the epic/legendary.

  
MoSCoW
Must have
 Must have feature in order to satisfy the epic/legendary.

  
MoSCoW
Should have
 Should have feature in order to satisfy the epic/legendary.

  
Needs Feedback
There are questions in the ticket that can not be completed until the project owner provides clarity.

  
Points
1
 1 man-hours worth of work for an expert with no learning curve.

  
Points
13
 13 man-hours worth of work for an expert with no learning curve.

  
Points
2
 2 man-hours worth of work for an expert with no learning curve.

  
Points
21
 21 man-hours worth of work for an expert with no learning curve.

  
Points
3
 3 man-hours worth of work for an expert with no learning curve.

  
Points
34
 34 man-hours worth of work for an expert with no learning curve.

  
Points
5
 5 man-hours worth of work for an expert with no learning curve.

  
Points
55
 55 man-hours worth of work for an expert with no learning curve.

  
Points
8
 8 man-hours worth of work for an expert with no learning curve.

  
Points
88
 88 man-hours worth of work for an expert with no learning curve.

  
Priority
Backlog
 This ticket has backlogged priority and is not to be worked on yet

  
Priority
CI Blocker
 Critical priority issue that blocks CI/CD pipeline and prevents PR merges

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Signed-off: Owner
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Scrum Master
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Tech Lead
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Spike
A ticket for learning a tool or technology that is needed to be able to do future planning and design.

  
State
Completed
 The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.

  
State
Duplicate
 A ticket that represents the same content as an existing ticket.

  
State
In Progress
 A ticket that is actively being developed.

  
State
In Review
 A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.

  
State
Paused
 This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.

  
State
Unverified
 All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.

  
State
Verified
 The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.

  
State
Wont Do
 This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.

  
Type
Automation
 Any edits or discussion about the AI automated coding system.

  
Type
Bug
 Something that doesnt work as intended.

  
Type
Discussion
 Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.

  
Type
Documentation
 An error or improvement needed in the documentation.

  
Type
Epic
 Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.

  
Type
Feature
 Some new functionality not present.

  
Type
Legendary
 A type of Epic which will contain other Epics.

  
Type
Refactor
 A code change that restructures existing code without changing its external behavior.

  
Type
Support
 Someone needs help using the project.

  
Type
Task
 A generic task that doesnt fit into the other type categories.

  
Type
Testing
 Work exclusively focusing on fixing or expanding testing.

No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!11102
No description provided.