Repository navigation
[Epic] Cost visibility: failed-attempt spend, cost export, anomaly alerts, cost by effort, escalation after failure - #3458
propr-dev[bot] wants to merge 15 commits into
Conversation
…on, and analytics reports it separately from successful spend. Typecheck passes for root, core, api and propr-ui, and eslint passes with `--max-warnings 0` on every changed file. In the related tests (28 root suites, the API analytics suites and the Analytics page tests) everything passes. The `processGoalJob` suite only failed when I ran suites four at a time, because they share a SQLite file; it passes 18/18 alone. `test/llmMetrics.test.ts` needs a real Redis, so I didn't run it; the new tests use a mocked Redis that behaves like the real one for these calls.
1. Failed attempts get one execution row
- Usage limits: `parseStreamJsonOutput` now attaches what it had parsed so far (turns, tokens, cost, session) to the `UsageLimitError`. When `ClaudeAgent` or `CodexAgent` catches one, it writes one `llm_logs` row and one `llm_executions` row with `success = false`, the error message and the observed tokens and cost (zero if none), then rethrows. Nothing downstream ever gets a result for this case, so the agent has to record it.
- Thrown errors: the agent returns a failed result as before, and the job records it as before. The difference is that it now also writes the missing `llm_logs` row. If the agent run had finished and a later step threw, the tokens and cost from that run are kept instead of being reset to zero.
- Cancellations: the issue and PR-comment jobs now record the run before checking whether the task was cancelled. A cancelled run's spend used to go unrecorded.
- The shared logic is in a new file, `agents/impl/utils/failedAgentExecution.ts`. Recording waits at most 10 seconds, so an unreachable store can't hold up a failing run.
2. `recordLLMMetrics` can't count an execution twice
- Each agent result now carries an `executionId`, assigned after the agent runs and passed through every result conversion in the issue, PR-comment, merge-conflict and goal jobs.
- The first record of an id claims it in Redis (`SET NX`, kept 30 days, with a small in-process backup set). Any later record with that id inserts no row and adds nothing to the `llm:metrics:*` counters. Results without an id are recorded as before. This is in a new file, `utils/llmMetricsExecutionClaim.ts`.
3. Analytics splits spend by outcome
- `loadRecordedSpend` now returns `{ total, successful, failed }` instead of a single number; `total` means the same as before. Its two callers in `statsRoutes.ts` are updated.
- `loadModelUsage` adds `successful_cost_usd`, `failed_cost_usd` and `failed_runs` to each model.
- `/stats/overview` adds `usage.successful_cost_usd` and `usage.failed_cost_usd`. `/stats/dashboard` adds `recordedSpendSuccessful` and `recordedSpendFailed`, including under `previous`. No existing field was renamed or removed.
- An execution with no recorded outcome is not counted as failed. A database without the `success` column reports zero failed spend.
- The Analytics page shows "$X on failed attempts" under Total spend for the selected window, and shows nothing when the server doesn't send the split. I shortened the wording from "spend on failed attempts" because it was cut off on a 390px phone.
Tests
- New `test/llmMetricsFailedExecutions.test.ts`: recording an id twice gives one row and one counter increment; an id claimed by another worker is skipped; usage limit with and without streamed tokens; thrown errors.
- New `test/llmMetricsFailureHandlerIdempotency.test.ts`: the issue and PR-comment failure handlers pass on the same id the run recorded. This test fails against the old code.
- The API tests cover the split, missing outcomes and the old schema. The Analytics page test covers the new line.
- I updated existing exact-match test expectations to include the new fields. The shared dashboard test database now has a nullable `success` column.
Two behaviour changes to check:
- The idempotency guard relies on Redis. If Redis is down, recording already fails as it did before.
- A thrown error before the agent did any work still writes a failed row with zero spend, as the issue asks. Those rows add to the run counts on the Analytics page.
I captured desktop and phone previews of the totals band using stubbed data.
…on's claim, so a later record of the same result can still write its row. A completed record is still never written twice. Typecheck passes for core and root, and eslint `--max-warnings 0` is clean on both changed source files. The new and existing metrics tests pass (25/25), and the other suites that record metrics through a mocked Redis pass (289/289). I fixed F1: a write that fails no longer uses up an execution's claim, so a later record of the same result can still write its row. A completed record is still never written twice. Typecheck passes for core and root, and eslint `--max-warnings 0` is clean on both changed source files. The new and existing metrics tests pass (25/25), and the other suites that record metrics through a mocked Redis pass (289/289). **How the claim works now** (`packages/core/src/utils/llmMetricsExecutionClaim.ts`) - Recording an execution now has two stages, each claimed separately with Redis `SET NX`: the `llm:metrics:*` counters (existing key `llm:metrics:recorded:<id>`) and the `llm_executions` row (new key `llm:metrics:persisted:<id>`). - Each claim stores a random token. A claim is released only if it still holds that token, so one worker can't remove another worker's claim. Releasing also clears the in-process backup entry. - A release happens only when the stage verifiably wrote nothing: - **Counters:** released if the attempt fails before any counter is incremented (a cost calculation or `setex` failure). Once incrementing has started the claim is kept, so a partial write is never counted again. - **Row:** `persistToDatabase` now returns whether the row was inserted, and the claim is released only if it wasn't. If the row went in but its detail rows failed, the claim stays, so nothing is inserted twice. - A record whose counters are already claimed now skips only the counters and still writes the row if that stage is unclaimed. That is how the failure handler's second record recovers a missing row. **Tests** - New `test/llmMetricsClaimRecovery.test.ts`, which injects failures. Each case records the same result again afterwards and checks the outcome: | First attempt fails at | Second attempt | |---|---| | Row insert | Exactly one row, counters not incremented again; a third record still writes nothing | | `setex` | Both claims were released; one row, one count | | A counter increment | One row; the counters keep the first attempt's value | | Detail rows | Still treated as recorded, no second row | A fifth case checks that a claim held by another worker is never released. The first three cases fail against the previous code. - In `test/llmMetricsFailedExecutions.test.ts`, the cross-worker test now marks both stages as claimed by the other worker. **Separate from this fix (not changed)** - One race can't be avoided: if the database inserts the row but the reply is lost, the claim is released and a retry could insert a second row. Redis being unreachable during a release leaves that stage claimed, so it's lost but not doubled. - As before, a Redis failure during the counters stage still skips the database write within that same attempt; the next record of the same result writes it. There are no visual changes, so I made no previews. PR: #3453 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
…-report-spend-o-20261111-2200-41r [3437 by Claude Opus 5.5] Record and report spend on failed and errored attempts
…. It was a leftover wait loop from an earlier test run. It never exited because its `pgrep -f "costExport.test"` matched its own command line. The actual test runs finished separately and passed (8/8 for the API tests), so I didn't restart it. The implementation and test results in my previous summary still stand.
…me from the one log call matched to each execution, and stay null when no call matches. All 9 cost-export tests pass on SQLite, the API typecheck is clean, and eslint `--max-warnings 0` passes on both changed files. I fixed F1 and F2 together: agent and execution type now come from the one log call matched to each execution, and stay null when no call matches. All 9 cost-export tests pass on SQLite, the API typecheck is clean, and eslint `--max-warnings 0` passes on both changed files. **F1 didn't reproduce.** The old correlated `ORDER BY` query works on this SQLite (3.53.4, better-sqlite3): the existing agent/type test passed before I changed anything, run with `PROPR_DEMO_MODE=true`. I replaced the query anyway, because fixing F2 needs one ranked match per execution. **What changed** (`loggedRuns` in `packages/api/routes/costExport.ts`): - The export now joins each execution in the window to candidate log calls and ranks them with `row_number()`: same session first, then same model, then nearest start time, then `log_id`. The best candidate is the only row joined, and both agent and execution type are read from it. A null `agent_alias` on that row stays null and isn't taken from another call. - A call counts as a match only if: - **Execution with a task:** the call has the same task and the same session. If neither has a session, it needs the same task and the same model instead. Calls that only share the task, or that come from a different session, no longer match. - **Execution without a task:** the call has the same session (unchanged). - The task and session paths both use the new query. The join only runs when `agent` or `execution_type` is requested. One rule is stricter than the finding asked for. A session-less execution no longer matches a call that has a session, even on the same model. I did this because an execution and its log call take their session from the same result, so a log with a session belongs to some other run. Otherwise the test would pass only because of the `log_id` tie-break. **New test:** "agent and execution type come from one matching call; nothing is borrowed from other runs of the task". It covers three cases: - **F2's own case:** the matching call has a type but no agent, and another review call on the task has `codex`. The result is `null / implementation`. - **No matching call:** an execution whose session no call recorded gets `null / null`. - **No session:** the nearer of two session-less same-model calls wins, and a nearer call from another session is never picked. I checked that this test fails on the previous code, which gives `codex / implementation` and `codex / review`. I found nothing else outside the selected findings to report. PR: #3459 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
…t and execution type only when there's real evidence that a log call belongs to that run. If there isn't, both stay null, so the later run in the finding's example is no longer credited to the earlier run's call. I fixed F3. An execution without a session now gets an agent and execution type only when there's real evidence that a log call belongs to that run. If there isn't, both stay null, so the later run in the finding's example is no longer credited to the earlier run's call. **What changed** (`loggedRuns` in `packages/api/routes/costExport.ts`): a session-less execution and a session-less call on the same task and model now pair up only when: - **Their time spans overlap.** Both rows come from the same agent run, so the real call always overlaps its execution. A missing end time counts as the start time. - **They are each other's nearest overlapping match.** One call can therefore never be credited to two executions. - **The match is decided across all executions, not just the requested period.** If a call's own execution started before the period, the call isn't credited to a later overlapping execution inside it. Executions with a session, and executions with no task, still match exactly as before. The HTTP JSON, CSV, CLI and MCP outputs all share this loader, so they all get the fix. **Tests** (`packages/api/test/costExport.test.ts`): - **New regression test** with three cases: - **The finding's case:** two session-less runs of one task on one model, with a call recorded only for the earlier run. The later run is counted as unknown, and its $4 is no longer credited to `claude / implementation`. - **Two runs overlapping one call:** only the run nearest the call gets it. The other stays unknown. - **Call's own run outside the period:** the call isn't credited to the overlapping run inside the period. - **One existing test fixture adjusted.** In the earlier test's session-less case, the matching call started an hour before the execution and recorded no end time, so it never overlapped the run. I gave it an end time so that it covers the run. That case still checks that the matching call wins over a call from another session. **Verification:** - All 10 cost-export tests pass. - The new test fails on the previous `costExport.ts`, and that is the only test that fails there. - The `packages/api` typecheck is clean. - eslint `--max-warnings 0` passes on both changed files. One thing to know: the overlap rule assumes an execution row's time span actually covers its agent run. The execution's start is computed when it is saved, as save time minus the run's duration. If a run were saved long after it ended, its spans wouldn't overlap and its agent and type would show as unknown rather than wrong. I found nothing else outside F3 to report. This change has no visual effect, so I made no preview. PR: #3459 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
…both cost export endpoints now return 400 instead of exporting every repository. I fixed F4: when a repository filter isn't a plain string, both cost export endpoints now return 400 instead of exporting every repository. **The fix** (`packages/api/routes/costExport.ts`, in `readExport`): `validateRepositoryFilter` now checks the raw `req.query.repository` before anything else. A missing or empty value still means "all repositories". Repeated parameters (`?repository=acme/alpha&repository=acme/beta`) and nested ones (`?repository[name]=…`) get a 400 with "Repository must be a string". Both `/api/stats/cost` and `/api/stats/cost.csv` go through this one function, so one change covers both. **New test** (`packages/api/test/costExport.test.ts`): for both endpoints, it sends two repeated values, one value in array form, and an object. It checks each gets a 400 with that error. **Verification:** - The cost-export suite passes 11/11. I ran it with `PROPR_DEMO_MODE=true` because without GitHub auth configured, the test file fails before any test runs. - With the original route code put back, the new test fails, so it does catch the bug. - The API typecheck is clean, and eslint `--max-warnings 0` passes on both changed files. **Other places I checked:** - The MCP tool `get_cost_breakdown` isn't affected: its input schema already requires `repository` to be a single repository handle, so it can't receive an array or object. - **Not fixed, separate issue:** `readSummary` in `packages/api/routes/reviewScoreStats.ts` (around line 388) has the same pattern. It turns any non-string `repository` into `'all'` before validating, so repeated parameters widen the review-score summary to every repository. This PR only exported `csvCell` from that file and didn't touch this code, so I left it out of scope. PR: #3459 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
…d-llm-cost-expo-20261111-2357-0bs [3438 by Claude Opus 5.5] Metrics and LLM-cost export (CSV, JSON, CLI, MCP)
… compared with the usual tokens for the same repository, model and execution type. A run far above that usual level raises a `warning` Inbox notification on its task. The new tests and the related suites pass. How the check works - Baseline: the median tokens of the last N successful runs (default 20) for the same repository, model and execution type. A run is never part of its own baseline. No alert fires until at least 8 such runs exist. - Execution type: older call sites don't pass one, so I fall back to the job type (`issue`, `pr_comment`, …). - Token total: input + output + cache writes + cache reads, the same total the cost estimate uses. - When it alerts: the run must be above the multiple (default 4×) and above an absolute floor (default 500,000 tokens), so small runs never alert. - Notification: reuses the existing `task` kind, links to the task, and states observed versus baseline. For example: "Used 6M tokens, 6× the baseline of 1M (median of the last 8 successful implementation runs of Claude Opus 5.5 in acme/widgets; alert above 4×)." - It goes to all instance members, the same way the spend-cap notification does. - It is skipped for repositories with notifications turned off. - Each task alerts at most once, so later runs of the same task stay quiet. - Runs with no task still count toward baselines but cannot alert, since there is nothing to link to. - Speed: one row per run goes into a new table, `llm_execution_token_samples`, added by migration `20261112000000`. A composite index makes the baseline query a single indexed lookup. The check is loaded only when first used, and any error in it is logged without affecting the recording. Automation pause (off by default) With `token_outlier_pause_alerts` set to M, a repository with M alerts inside the window has its scheduled agent runs skipped, with a reason in the run history. This happens in the existing agent-run cost gate. Only scheduled runs are affected; manual, API, MCP and CLI triggers still run. The pause lifts by itself once the window passes, and if the check itself fails the run goes ahead. Settings | Setting | Default | Range | |---|---|---| | `token_outlier_multiplier` | 4 | 1.5–100 | | `token_outlier_min_tokens` | 500,000 | from 1,000 | | `token_outlier_baseline_runs` | 20 | 8–200 | | `token_outlier_pause_alerts` | 0 (off) | 0–1,000 | | `token_outlier_pause_window_hours` | 24 | 1–720 | - They follow the standard pattern: loader, validation, `SPECIALIZED_SETTING_NAMES`, the settings API, the settings catalog, MCP `update_execution_settings`, and the CLI (`propr setting update …`). - The definitions live in `@propr/shared` so the CLI can use them. - The API imports them from shared rather than from `@propr/core`. Several existing tests replace `@propr/core` with a fixed list of exports and broke when the API imported the new constants from core. Old high-cost Redis list: removed. That covers `checkCostThreshold`, the `llm:metrics:alerts:highcost` list, `HighCostAlert`, and the `recentHighCostAlerts` summary field in core and the API. Nothing in the UI, CLI or OpenAPI used that field. `LLM_COST_THRESHOLD_USD` no longer does anything, and the docs now describe the new alert instead: `metrics.md`, `agents.md`, `execution-safety.md`, plus a regenerated `settings-locations.md`. Tests - New: `test/tokenOutlierAlert.test.ts`, 18 tests on in-memory SQLite. They cover the baseline (median, last-N, failed runs excluded, other model/type/repository excluded, case-insensitive repository), the threshold and floor, the 8-run minimum, once-per-task deduplication, the pause and its window, the gate, and the settings (loader, API validation, CLI parsing). I broke the deduplication and the 8-run minimum on purpose to confirm the tests catch it. - Existing: I re-ran 30 related suites (settings, config routes, MCP, OpenAPI, cost export, and the existing metrics-recording suites). In `test/configRoutes.followup.test.ts`, the expected settings responses needed the new defaults added; it now passes 103/103. - `test/llmMetrics.test.ts`: I rewrote its old high-cost test to check that the list is gone. It needs a live Redis and timed out in the first run because this container has none. With a Redis 7.2 built locally it passes 5/5. - Typecheck and lint: clean for shared, core, API and CLI; the changed core and API files pass `eslint --max-warnings 0`. There are no visual previews: the alert uses the existing Inbox notification UI, and everything else is backend.
…d the automation pause, only after its Inbox notification has actually been saved. All 21 tests in `test/tokenOutlierAlert.test.ts` pass, including three new ones. I fixed F1: a task's alert is now marked, and counted toward the automation pause, only after its Inbox notification has actually been saved. All 21 tests in `test/tokenOutlierAlert.test.ts` pass, including three new ones. **Changes in `packages/core/src/utils/tokenOutlierAlert.ts`** - **Three outcomes:** `notifyTokenOutlier` now reports `delivered` once the event is saved, or `suppressed` when nothing is raised (repository notifications off, no recipients, no task). A failed save throws as before. The new `TokenOutlierNotificationOutcome` type is also exported from `packages/core/src/index.ts`. - **Marker after delivery:** `recordTokenUsageSample` writes `alerted_at` only for `delivered`. - If the notification is suppressed, nothing is marked and the result is `alerted: false`. - If the save fails, the error reaches the caller's existing best-effort handler in `llmMetrics.ts` with nothing marked. The task's next outlier run tries again, and the existing `token-outlier:<taskId>` deduplication key stops a retry from creating a second Inbox event. - **Pause count:** since the current alert isn't in the table yet when the pause is worked out, the count adds 1 for it. The notification text still states the pause that this alert causes. - **Two runs at once:** two outlier runs of the same task could both pass the "already alerted" check while waiting on the notification, then both mark and count twice toward the pause. The marker update now skips the row if another run of the task has been marked in the meantime, so only one run reports `alerted: true`. The original code had the same gap, and it could only get wider once marking waits for delivery. **New tests in `test/tokenOutlierAlert.test.ts`** - **Notifications off:** with the pause set to 1, a suppressed alert leaves no marker and no pause. Once delivery works again, the same task's next outlier alerts and starts the pause. - **Save failure:** the error propagates, there is no marker or pause, the next outlier run of the task alerts, and the one after that stays quiet. - **Two runs at once:** both runs get past the "already alerted" check before delivery finishes, and only one ends up marked. All three new tests fail when run against the previous version of the file, and the other 18 still pass there. The core typecheck and `eslint --max-warnings 0` on the changed core files are clean. `test/llmMetrics.test.ts` was not run successfully: it needs Redis, which isn't in this container, so the attempt hung and I stopped it. The caller it would cover, `checkTokenOutlier`, is unchanged. There are no visual previews, because this change is backend-only. PR: #3467 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
|
/merge |
|
❌ Failed to resolve merge conflicts from Push rejectedReason: Non-fast-forward ( The remote branch moved while the task ran (non-fast-forward). Fetch the branch, integrate the new commits, and push again. Recovery: The commits were pushed to Remote outputSystem-triggered merge conflict resolution |
…-a-run-s-token--20261112-1031-e9d [3439 by Claude Opus 5.5] Alert when a run's token use is far above the baseline for its repository and model
|
🔀 Resolved merge conflicts from Conflicting Files
Resolution SummaryThe merge conflict is resolved. What the conflict was: both branches added a new entry at the top of
How I resolved it: I kept everything from both sides and only removed the conflict markers. Under Why: these are separate features, so a changelog needs every entry and neither side should replace the other. Putting them one after another keeps the PR's entry and brings in main's changes unchanged. 🤖 Resolution Details
System-triggered merge conflict resolution |
Automatically resolved merge conflicts after merging target branch into PR branch. Resolved conflicts in: - CHANGELOG.md PR: #3458 Model: Claude Opus 5.5
…s down by implementer model and effort. Only the API typechecks; the CLI and UI typecheck too, and lint passes for the API and UI files I changed with `--max-warnings 0` (the CLI package has no ESLint config, so nothing lints its files). The new and existing tests pass: - API: the review-score suites plus the nearby analytics, stats, cost-export and MCP overview suites. - CLI: 4/4. - UI: the Vitest tests and the 15 Playwright tests in `analytics-timeframe.pw.ts`. API (`packages/api/routes/reviewScoreStats.ts`) - The summary has a new `by_effort` list. Each row has the same figures as a `models` row, for one model at one effort level, plus `implementer_reasoning_level`. - A PR's effort comes from the same score row that gives its model. PRs with no recorded level, including empty strings and historical data, go under an explicit `unknown` value and are never dropped. Each model's effort rows add up to its `models` row. - Rows follow the order of `models`, then effort from lowest to highest, with `unknown` last. - `models` keeps its meaning, so the per-model totals and the overview's score figures are unchanged. - If a database doesn't have the `implementer_reasoning_level` column yet, every PR is reported as `unknown` instead of the query failing. - CSV format change: the CSV now has one row per model and effort, with a new `implementer_reasoning_level` column after `implementer_agent`. Anything that reads this CSV expecting one row per model will need updating; the per-model totals are still in the JSON `models` list. CLI (`packages/cli`) - The TypeScript type now includes `score_delta`, `runs_to_merge` and the effort rows. - `propr stats review-scores` prints one row per model and effort, with new Effort, Merged, and Runs to merge columns. If the server is older and doesn't send effort rows, it falls back to per-model rows with the effort shown as `(all)`. UI (`ReviewQualityByModel.tsx`) - A model with more than one effort level gets an indented row per level under its totals, with "Unknown effort" last. - A model with exactly one known level gets a small tag next to its name instead, such as "Medium effort". - Untracked work and older servers look the same as before. - I didn't add a cost-per-merged-PR column to this table; it didn't have one before, and cost is in the CSV and CLI. The issue only asked for effort as a secondary grouping here. Tests and docs - New tests cover grouping, ordering, the `unknown` bucket, the missing-column case, the CSV columns, and the CLI rows and table. The existing fixtures now include effort levels, and the e2e mock includes effort rows. - The API tests for the effort split are in a new file, `packages/api/test/reviewScoreEffort.test.ts`, because adding them to the existing test file would have pushed it over the 400-line lint limit. - `metrics.md`, `propr-cli.md` and `CHANGELOG.md` are updated. A few of the related API suites only run with `PROPR_DEMO_MODE=true` in this container. Without it they stop at the GitHub-auth message, which is an environment issue, not caused by this change. The preview is a screenshot of the Analytics pane with mocked data, captured by the existing e2e test.
…ame.pw.ts` had 405 counted lines against the 400-line `max-lines` limit, because this branch added effort rows to its mock data. It now passes. I fixed the UI lint failure. `propr-ui/e2e/analytics-timeframe.pw.ts` had 405 counted lines against the 400-line `max-lines` limit, because this branch added effort rows to its mock data. It now passes. I shortened the file without changing any data or what the tests check: - **Review-score mock:** the model and agent fields are now defined once, as `claude` and `gpt`, and a small `effort()` helper builds the four effort rows. The `models` and `by_effort` lists now take five lines instead of nine. - **Drill-down test:** three `expect(...)` calls that were split across two lines are now on one line each. Checks I ran: - `eslint e2e/analytics-timeframe.pw.ts --max-warnings 0` and the full `npm run lint` in `propr-ui` both pass. - `tsc --noEmit` passes. - All 15 Playwright tests in `analytics-timeframe.pw.ts` pass. The "Slow Test" note about `creation-short-window.pw.ts` in the CI summary is only a timing report, not a failure, and this branch doesn't touch that file. Nothing changes on screen, so there is no new visual preview. PR: #3494 Comment by: @github-actions[bot] (ID: 6828733695) Comment by: @propr-dev[bot] (ID: 6828733526) Model: Claude Opus 5.5
…t-per-merged-pr-20261112-1447-a37 [3440 by Claude Opus 5.5] Report cost per merged PR by model and effort
Cost visibility: failed-attempt spend, cost export, anomaly alerts, cost by effort, escalation after failure
This Epic PR aggregates all changes for the plan: Cost visibility: failed-attempt spend, cost export, anomaly alerts, cost by effort, escalation after failure
Issues in this Epic
Auto-close
When this PR is merged, the following issues will be automatically closed:
Fixes #3437
Fixes #3438
Fixes #3439
Fixes #3440
Fixes #3441
Created automatically by ProPR