gitea_admin/pragent
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

feat: add adaptive review effort budgets #19

Merged
masi merged 4 commits from feat/review-budget-governor into main 2026-09-01 12:15:17 +00:00
Conversation 2 Commits 4 Files Changed 16
+627 -23
masi commented 2026-09-01 11:52:15 +00:00
Contributor
Copy Link
Copy Source

Summary

  • track per-iteration tokens, tool calls, steps, and equivalent cost
  • enforce hard review limits for steps, tokens, output, duration, lenses, and cost
  • terminate budgeted subprocesses after a completed iteration crosses a limit
  • adapt default headroom for broad diffs while preserving explicit repository budgets
  • expose usage and cap metadata in review output and Langfuse
  • document the current architecture, budget controls, and adaptive profiles

Calibration

Reviewed active PR context from netcracker/interview and techspark/suaspark-dashboard to keep focused reviews inexpensive while allowing larger TypeScript/Go changes to complete.

Verification

  • python3 -m pytest tests/pilot -q
  • 525 tests passed
  • git diff --check passed
## Summary - track per-iteration tokens, tool calls, steps, and equivalent cost - enforce hard review limits for steps, tokens, output, duration, lenses, and cost - terminate budgeted subprocesses after a completed iteration crosses a limit - adapt default headroom for broad diffs while preserving explicit repository budgets - expose usage and cap metadata in review output and Langfuse - document the current architecture, budget controls, and adaptive profiles ## Calibration Reviewed active PR context from netcracker/interview and techspark/suaspark-dashboard to keep focused reviews inexpensive while allowing larger TypeScript/Go changes to complete. ## Verification - python3 -m pytest tests/pilot -q - 525 tests passed - git diff --check passed
masi added 3 commits 2026-09-01 11:52:16 +00:00
feat: add review effort accounting and budget governor 3a10deef06
docs: document review budget defaults f9eef969e6
feat: adapt review budget to diff size 4d51e14a1a
pragent-bot bot reviewed 2026-09-01 11:58:49 +00:00
pragent-bot bot left a comment
Copy Link
Copy Source

🤖 AI Review · pragent pilot · headroom/MiniMax-M2.7 · 4d51e14a · Merge confidence: 3/5 🟡

PR-Agent pilot on this repo. Comments are LLM-generated; treat as suggestions, not mandates.

Summary of Changes

  • pilot/review/budget.py (new): Budget dataclass with from_config/for_review, BudgetState thread-safe accumulator, equivalent_cost helper with cost_model import.
  • pilot/review/opencode_runtime.py: _run_process gains budget/budget_state params, streams events line-by-line, terminates on cap hit, returns partial output or capped-message fallback.
  • pilot/review/opencode.py: Budget and BudgetState threaded through triage, run_lenses_review, run, and run_opencode; tool_calls and iterations added to usage dict.
  • pilot/review/configuration.py: _parse_budget sanitizes max_steps/max_total_tokens/max_output_tokens/max_duration_seconds/max_lenses/max_equivalent_cost_usd from .pr-review.json.

Key Risks & Concerns

  • equivalent_cost silently returns 0.0 on ImportError, making max_equivalent_cost_usd budget non-functional in any deployment where cost_model is not on the Python path.
  • _run_process crashes with TypeError if parse_events returns None usage before the duration budget fires, leaving the review process dead.
  • The runner parameter in _run_process is ignored in the budget path — a caller expecting a custom runner for testability would get Popen with no indication.
  • If opencode emits no step_finish events before the duration budget fires, budget_state.cap_reason is never set and run_opencode returns an empty uncapped response instead of the capped fallback.

Findings Overview

2 inline comment(s); 4 total.

Severity Location Finding
🔴 [HIGH] pilot/review/opencode_runtime.py:730 If parse_events returns (text, None) — which it does when no step_finish event has been seen yet — usage is None and usage.get(...) raises TypeError: 'NoneType' object is not callable. This crashes the review process when the subprocess is terminated by the duration budget before emitting any step event.
🔴 [HIGH] pilot/review/budget.py:176 equivalent_cost wraps the entire cost_model import and cost() call in 'except Exception', which silently catches ImportError/ModuleNotFoundError. If cost_model is not on the Python path at runtime (any deployment where pilot/ is not in sys.path), equivalent_cost returns 0.0 and max_equivalent_cost_usd budget enforcement is completely non-functional — the cost cap can never be reached.
🟡 [MEDIUM] pilot/review/opencode_runtime.py:124 The runner parameter is accepted and documented but is never used when budget is provided — _run_process always calls subprocess.Popen directly in that path. A caller passing a custom runner (e.g., for testability) would have it silently ignored with no error or warning.
🟡 [MEDIUM] tests/pilot/review_tests/budget_test.py:922 The test passes runner=None to _run_process expecting it to be used, but since budget is also provided, the runner argument is ignored (Popen is used unconditionally). The test exercises Popen + budget enforcement correctly but does not exercise the runner code path it declares it is testing.

Unanchored Notes

  • 🔴 [HIGH] pilot/review/opencode_runtime.py:730 — If parse_events returns (text, None) — which it does when no step_finish event has been seen yet — usage is None and usage.get(...) raises TypeError: 'NoneType' object is not callable. This crashes the review process when the subprocess is terminated by the duration budget before emitting any step event.
    • Fix: Guard the budget-record block with 'if usage is not None:' before calling usage.get(...).
  • 🟡 [MEDIUM] tests/pilot/review_tests/budget_test.py:922 — The test passes runner=None to _run_process expecting it to be used, but since budget is also provided, the runner argument is ignored (Popen is used unconditionally). The test exercises Popen + budget enforcement correctly but does not exercise the runner code path it declares it is testing.
    • Fix: Either split this into two tests — one with budget (uses Popen) and one without budget (uses runner) — or document that runner is not exercised in this test.
🔋 AI Usage & Run Details
  • Model / Engine: headroom/MiniMax-M2.7 · opencode · 19 steps · 391.4s
  • Total Tokens: 29,844 (29.8K) in / 15,287 (15.3K) out (0 reasoning, cache 612,644 (612.6K) read / 104,408 (104.4K) write, 762,183 (762.2K) total)
  • Actual: $0.00 (headroom/MiniMax-M2.7 — free tier)
  • Scope: Whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff. Per-comment output is attributed (one model pass produces all findings; output split by each finding's body weight).
🤖 **AI Review** · pragent pilot · headroom/MiniMax-M2.7 · `4d51e14a` · Merge confidence: 3/5 🟡 > PR-Agent pilot on this repo. Comments are LLM-generated; treat as suggestions, not mandates. ### Summary of Changes - pilot/review/budget.py (new): Budget dataclass with from_config/for_review, BudgetState thread-safe accumulator, equivalent_cost helper with cost_model import. - pilot/review/opencode_runtime.py: _run_process gains budget/budget_state params, streams events line-by-line, terminates on cap hit, returns partial output or capped-message fallback. - pilot/review/opencode.py: Budget and BudgetState threaded through triage, run_lenses_review, run, and run_opencode; tool_calls and iterations added to usage dict. - pilot/review/configuration.py: _parse_budget sanitizes max_steps/max_total_tokens/max_output_tokens/max_duration_seconds/max_lenses/max_equivalent_cost_usd from .pr-review.json. ### Key Risks & Concerns - equivalent_cost silently returns 0.0 on ImportError, making max_equivalent_cost_usd budget non-functional in any deployment where cost_model is not on the Python path. - _run_process crashes with TypeError if parse_events returns None usage before the duration budget fires, leaving the review process dead. - The runner parameter in _run_process is ignored in the budget path — a caller expecting a custom runner for testability would get Popen with no indication. - If opencode emits no step_finish events before the duration budget fires, budget_state.cap_reason is never set and run_opencode returns an empty uncapped response instead of the capped fallback. ### Findings Overview _2 inline comment(s); 4 total._ | Severity | Location | Finding | |---|---|---| | 🔴 [HIGH] | `pilot/review/opencode_runtime.py:730` | If parse_events returns (text, None) — which it does when no step_finish event has been seen yet — usage is None and usage.get(...) raises TypeError: 'NoneType' object is not callable. This crashes the review process when the subprocess is terminated by the duration budget before emitting any step event. | | 🔴 [HIGH] | `pilot/review/budget.py:176` | equivalent_cost wraps the entire cost_model import and cost() call in 'except Exception', which silently catches ImportError/ModuleNotFoundError. If cost_model is not on the Python path at runtime (any deployment where pilot/ is not in sys.path), equivalent_cost returns 0.0 and max_equivalent_cost_usd budget enforcement is completely non-functional — the cost cap can never be reached. | | 🟡 [MEDIUM] | `pilot/review/opencode_runtime.py:124` | The runner parameter is accepted and documented but is never used when budget is provided — _run_process always calls subprocess.Popen directly in that path. A caller passing a custom runner (e.g., for testability) would have it silently ignored with no error or warning. | | 🟡 [MEDIUM] | `tests/pilot/review_tests/budget_test.py:922` | The test passes runner=None to _run_process expecting it to be used, but since budget is also provided, the runner argument is ignored (Popen is used unconditionally). The test exercises Popen + budget enforcement correctly but does not exercise the runner code path it declares it is testing. | ### Unanchored Notes - 🔴 [HIGH] `pilot/review/opencode_runtime.py:730` — If parse_events returns (text, None) — which it does when no step_finish event has been seen yet — usage is None and usage.get(...) raises TypeError: 'NoneType' object is not callable. This crashes the review process when the subprocess is terminated by the duration budget before emitting any step event. - **Fix:** Guard the budget-record block with 'if usage is not None:' before calling usage.get(...). - 🟡 [MEDIUM] `tests/pilot/review_tests/budget_test.py:922` — The test passes runner=None to _run_process expecting it to be used, but since budget is also provided, the runner argument is ignored (Popen is used unconditionally). The test exercises Popen + budget enforcement correctly but does not exercise the runner code path it declares it is testing. - **Fix:** Either split this into two tests — one with budget (uses Popen) and one without budget (uses runner) — or document that runner is not exercised in this test. <details> <summary>🔋 AI Usage & Run Details</summary> - **Model / Engine**: `headroom/MiniMax-M2.7` · opencode · 19 steps · 391.4s - **Total Tokens**: 29,844 (29.8K) in / 15,287 (15.3K) out (0 reasoning, cache 612,644 (612.6K) read / 104,408 (104.4K) write, 762,183 (762.2K) total) - **Actual**: $0.00 (headroom/MiniMax-M2.7 — free tier) - **Scope**: Whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff. Per-comment output is *attributed* (one model pass produces all findings; output split by each finding's body weight). </details> <!-- pragent:sha=4d51e14a1abad488c8c69eddae5bbfe4270acfd4 -->
pilot/review/budget.py Outdated
@@ -0,0 +173,4 @@
cache_writes=int(usage.get("cache_write") or 0),
output=int(usage.get("output") or 0),
), price)
except Exception:
pragent-bot bot commented 2026-09-01 11:58:49 +00:00
Copy Link
Copy Source

🔴 [HIGH] equivalent_cost wraps the entire cost_model import and cost() call in 'except Exception', which silently catches ImportError/ModuleNotFoundError. If cost_model is not on the Python path at runtime (any deployment where pilot/ is not in sys.path), equivalent_cost returns 0.0 and max_equivalent_cost_usd budget enforcement is completely non-functional — the cost cap can never be reached.

Fix: Catch only ImportError/ModuleNotFoundError and let other exceptions propagate; or validate at startup that cost_model is importable when max_equivalent_cost_usd is configured.

except (ImportError, ModuleNotFoundError):
        return 0.0

🪙 ~2,951 (3.0K) tok (19% · attributed output)

🔴 [HIGH] equivalent_cost wraps the entire cost_model import and cost() call in 'except Exception', which silently catches ImportError/ModuleNotFoundError. If cost_model is not on the Python path at runtime (any deployment where pilot/ is not in sys.path), equivalent_cost returns 0.0 and max_equivalent_cost_usd budget enforcement is completely non-functional — the cost cap can never be reached. **Fix:** Catch only ImportError/ModuleNotFoundError and let other exceptions propagate; or validate at startup that cost_model is importable when max_equivalent_cost_usd is configured. ```suggestion except (ImportError, ModuleNotFoundError): return 0.0 ``` 🪙 ~2,951 (3.0K) tok (19% · attributed output)
masi marked this conversation as resolved
pilot/review/opencode_runtime.py
@@ -95,2 +121,4 @@
raise RuntimeError(last_err or "opencode produced no output")
def _run_process(
pragent-bot bot commented 2026-09-01 11:58:49 +00:00
Copy Link
Copy Source

🟡 [MEDIUM] The runner parameter is accepted and documented but is never used when budget is provided — _run_process always calls subprocess.Popen directly in that path. A caller passing a custom runner (e.g., for testability) would have it silently ignored with no error or warning.

Fix: Either remove the runner parameter from the budget path, or raise ValueError if runner is not callable when budget is provided.

🪙 ~1,885 (1.9K) tok (12% · attributed output)

🟡 [MEDIUM] The runner parameter is accepted and documented but is never used when budget is provided — _run_process always calls subprocess.Popen directly in that path. A caller passing a custom runner (e.g., for testability) would have it silently ignored with no error or warning. **Fix:** Either remove the runner parameter from the budget path, or raise ValueError if runner is not callable when budget is provided. 🪙 ~1,885 (1.9K) tok (12% · attributed output)
masi marked this conversation as resolved
masi added 1 commit 2026-09-01 12:14:35 +00:00
fix: resolve review budget feedback 592747d98d
masi merged commit 0adf7cdf2f into main 2026-09-01 12:15:17 +00:00
pragent-bot bot reviewed 2026-09-01 12:21:07 +00:00
pragent-bot bot left a comment
Copy Link
Copy Source

🤖 AI Review · pragent pilot · headroom/MiniMax-M2.7 · 592747d9 · Merge confidence: 4/5 🟢

PR-Agent pilot on this repo. Comments are LLM-generated; treat as suggestions, not mandates.

Summary of Changes

  • New budget subsystem: Budget dataclass, BudgetState accumulator, and equivalent_cost() for per-iteration cost estimation
  • Streaming _run_process() in opencode_runtime: reads Popen stdout line-by-line, checks budget after each step event, terminates if a limit is crossed
  • Adaptive headroom: diffs with 200/800/2000+ changed lines automatically scale from 20→40→60→80 steps and 120K→400K→800K→1.2M tokens unless an explicit repo budget is set
  • Budget metadata in Langfuse: cap_hit/cap_reason/iterations/tool_calls exposed in trace metadata; per-lens budget enforcement in opencode_lenses

Key Risks & Concerns

  • The delta passed to equivalent_cost() in _run_process never includes tool_calls (previous is never updated with it), so the tool_calls count accumulated by parse_events is silently dropped from BudgetState. The max_equivalent_cost_usd cap is unaffected, but Langfuse tool_calls metadata will be incorrect.
  • No test covers the equivalent_cost() function itself, leaving the max_equivalent_cost_usd enforcement path untested in the happy case.

Findings Overview

1 inline comment(s); 1 total.

Severity Location Finding
🟡 [MEDIUM] pilot/review/opencode_runtime.py:181 previous.update() (lines 181-189) never includes tool_calls, so the tool_calls count accumulated by parse_events is silently discarded and never propagated to BudgetState. The max_equivalent_cost_usd cap itself is unaffected (it uses only steps/total_tokens/output_tokens/equivalent_cost_usd), but metadata fields derived from BudgetState.snapshot() and the per-iteration accumulation will undercount tool calls in Langfuse.
🔋 AI Usage & Run Details
  • Model / Engine: headroom/MiniMax-M2.7 · opencode · 14 steps · 386.4s
  • Total Tokens: 26,910 (26.9K) in / 14,610 (14.6K) out (0 reasoning, cache 401,115 (401.1K) read / 67,612 (67.6K) write, 510,247 (510.2K) total)
  • Actual: $0.00 (headroom/MiniMax-M2.7 — free tier)
  • Scope: Whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff. Per-comment output is attributed (one model pass produces all findings; output split by each finding's body weight).
🤖 **AI Review** · pragent pilot · headroom/MiniMax-M2.7 · `592747d9` · Merge confidence: 4/5 🟢 > PR-Agent pilot on this repo. Comments are LLM-generated; treat as suggestions, not mandates. ### Summary of Changes - New budget subsystem: Budget dataclass, BudgetState accumulator, and equivalent_cost() for per-iteration cost estimation - Streaming _run_process() in opencode_runtime: reads Popen stdout line-by-line, checks budget after each step event, terminates if a limit is crossed - Adaptive headroom: diffs with 200/800/2000+ changed lines automatically scale from 20→40→60→80 steps and 120K→400K→800K→1.2M tokens unless an explicit repo budget is set - Budget metadata in Langfuse: cap_hit/cap_reason/iterations/tool_calls exposed in trace metadata; per-lens budget enforcement in opencode_lenses ### Key Risks & Concerns - The delta passed to equivalent_cost() in _run_process never includes tool_calls (previous is never updated with it), so the tool_calls count accumulated by parse_events is silently dropped from BudgetState. The max_equivalent_cost_usd cap is unaffected, but Langfuse tool_calls metadata will be incorrect. - No test covers the equivalent_cost() function itself, leaving the max_equivalent_cost_usd enforcement path untested in the happy case. ### Findings Overview _1 inline comment(s); 1 total._ | Severity | Location | Finding | |---|---|---| | 🟡 [MEDIUM] | `pilot/review/opencode_runtime.py:181` | previous.update() (lines 181-189) never includes tool_calls, so the tool_calls count accumulated by parse_events is silently discarded and never propagated to BudgetState. The max_equivalent_cost_usd cap itself is unaffected (it uses only steps/total_tokens/output_tokens/equivalent_cost_usd), but metadata fields derived from BudgetState.snapshot() and the per-iteration accumulation will undercount tool calls in Langfuse. | <details> <summary>🔋 AI Usage & Run Details</summary> - **Model / Engine**: `headroom/MiniMax-M2.7` · opencode · 14 steps · 386.4s - **Total Tokens**: 26,910 (26.9K) in / 14,610 (14.6K) out (0 reasoning, cache 401,115 (401.1K) read / 67,612 (67.6K) write, 510,247 (510.2K) total) - **Actual**: $0.00 (headroom/MiniMax-M2.7 — free tier) - **Scope**: Whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff. Per-comment output is *attributed* (one model pass produces all findings; output split by each finding's body weight). </details> <!-- pragent:sha=592747d98dd9bf862fb43c30a95be7e3b743a235 -->
pilot/review/opencode_runtime.py
@@ -97,0 +178,4 @@
"output": usage.get("output", 0) - previous["output"],
}
cost = float(usage.get("cost", 0.0)) - previous["cost"]
previous.update({
pragent-bot bot commented 2026-09-01 12:21:07 +00:00
Copy Link
Copy Source

🟡 [MEDIUM] previous.update() (lines 181-189) never includes tool_calls, so the tool_calls count accumulated by parse_events is silently discarded and never propagated to BudgetState. The max_equivalent_cost_usd cap itself is unaffected (it uses only steps/total_tokens/output_tokens/equivalent_cost_usd), but metadata fields derived from BudgetState.snapshot() and the per-iteration accumulation will undercount tool calls in Langfuse.

Fix: Add 'tool_calls': usage.get('tool_calls', 0) to both the delta dict (lines 172-179) and the previous.update() call (lines 181-189), and track previous_tool_calls = 0 similarly.

previous = {"steps": 0, "total": 0, "output": 0, "cost": 0.0, "tool_calls": 0}
...
                delta = {
                    "steps": usage.get("steps", 0) - previous["steps"],
                    "input": usage.get("input", 0) - previous.get("input", 0),
                    "cache_read": usage.get("cache_read", 0) - previous.get("cache_read", 0),
                    "cache_write": usage.get("cache_write", 0) - previous.get("cache_write", 0),
                    "total": usage.get("total", 0) - previous["total"],
                    "output": usage.get("output", 0) - previous["output"],
                    "tool_calls": usage.get("tool_calls", 0) - previous.get("tool_calls", 0),
                }
...
                previous.update({
                    "steps": usage.get("steps", 0),
                    "input": usage.get("input", 0),
                    "cache_read": usage.get("cache_read", 0),
                    "cache_write": usage.get("cache_write", 0),
                    "total": usage.get("total", 0),
                    "output": usage.get("output", 0),
                    "cost": float(usage.get("cost", 0.0)),
                    "tool_calls": usage.get("tool_calls", 0),
                })

🪙 ~14,610 (14.6K) tok (100% · attributed output)

🟡 [MEDIUM] previous.update() (lines 181-189) never includes tool_calls, so the tool_calls count accumulated by parse_events is silently discarded and never propagated to BudgetState. The max_equivalent_cost_usd cap itself is unaffected (it uses only steps/total_tokens/output_tokens/equivalent_cost_usd), but metadata fields derived from BudgetState.snapshot() and the per-iteration accumulation will undercount tool calls in Langfuse. **Fix:** Add 'tool_calls': usage.get('tool_calls', 0) to both the delta dict (lines 172-179) and the previous.update() call (lines 181-189), and track previous_tool_calls = 0 similarly. ```suggestion previous = {"steps": 0, "total": 0, "output": 0, "cost": 0.0, "tool_calls": 0} ... delta = { "steps": usage.get("steps", 0) - previous["steps"], "input": usage.get("input", 0) - previous.get("input", 0), "cache_read": usage.get("cache_read", 0) - previous.get("cache_read", 0), "cache_write": usage.get("cache_write", 0) - previous.get("cache_write", 0), "total": usage.get("total", 0) - previous["total"], "output": usage.get("output", 0) - previous["output"], "tool_calls": usage.get("tool_calls", 0) - previous.get("tool_calls", 0), } ... previous.update({ "steps": usage.get("steps", 0), "input": usage.get("input", 0), "cache_read": usage.get("cache_read", 0), "cache_write": usage.get("cache_write", 0), "total": usage.get("total", 0), "output": usage.get("output", 0), "cost": float(usage.get("cost", 0.0)), "tool_calls": usage.get("tool_calls", 0), }) ``` 🪙 ~14,610 (14.6K) tok (100% · attributed output)
Sign in to join this conversation.
Reviewers
No Reviewers
pragent-bot
Labels
Clear labels
AI-REVIEW
Trigger pragent-bot AI review
 AI-USAGE
Opt-in: append AI token-usage summary + per-comment attribution to the review
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
pragent-bot (pragent AI review bot) gitea_admin
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gitea_admin/pragent#19
Delete Branch "feat/review-budget-governor"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?