#164 feat(cost): loop-anomaly detector over the cost ledger (CB.3)

[Anonymous]

Originally created by: harshitagrawal2O

Pull Request

Slice

• Repo: doberman-core
• Feature / Slice: CB.3 — loop-anomaly detector over the cost ledger (part of [#143])
• Plan reference: doberman_implementation_plan.md (Slice CB.3)

What this PR does

Rebased onto current main and narrowed to the two net-new files — the CI/CD-config glob work is already carried forward in [#203], so paths.py / README.md / CHANGELOG.md are dropped here.

Adds detect_loop_anomaly() over the append-only cost_events ledger. Over a rolling window (optionally scoped to one entity_id) it flags a runaway/looping burn:

• call_burst — number of tool_call events exceeds max_calls
• token_burst — summed token units (tokens_in + tokens_out) exceed max_units

It returns an advisory LoopAnomaly (counts + a coarse signal label + a redaction-safe explanation). Infra-first slice: detect_loop_anomaly / LoopAnomaly are not wired into any caller yet — the CostObserver seam that gives it a home is [#143].

Review feedback addressed (from the earlier pass on this PR)

• The TypeError in the "never raises" path is fixed. A tz-naive now (or a legacy naive-timestamp row) compared against the ledger's aware timestamps used to raise outside the failure boundary. Now: a naive now returns calm up front, now is optional (defaults to datetime.now(timezone.utc)) so omitting it can't raise, and naive rows are skipped in-loop.
• Signals calibrated to single, well-defined quantities. One agent action emits several CostEvent rows, so call_burst now counts only tool_call rows and token_burst sums only token-kind units (previously it counted all rows / summed mixed units).
• Bounded read is a real indexed scan. Capped by the monotonic id PK (part of idx_cost_events) instead of the unindexed TEXT ts, which also removes the mixed-UTC-offset string-ordering hazard.
• Kept the parameterized SQL (# noqa: S608 justified), the _ANOMALY_ROW_CAP DoS bound, the broken-DB fail-safe test, the not-a-verdict structural test, and the redaction test.
• Named the per-entity_id burst-scoping ceiling in a comment so it's on record before anything depends on it.

Tests added (run in CI)

• tests/unit/test_cost_loop_anomaly.py — 18 tests: empty/calm, call_burst, token_burst, precedence, window/future boundaries, exact calls == max_calls boundary, per-entity scoping, kind-scoping of each signal, now default, naive now, legacy naive DB row, broken-DB fail-safe, not-a-verdict, and explanation redaction.

Public-release safety (doberman-core only)

• [x] Contains nothing from the "not allowed" list: no enterprise/hosted code, no proprietary detection, no customer data, no secrets, no commercial-license code
• [x] Core still builds/tests/runs with NO enterprise package installed

Security checklist

• [x] Fails closed on error / uncertainty — every error path (bad now, unparseable/naive row, DB failure) returns a calm readout; the detector never raises into its caller
• [x] No secret, full file, or unredacted prompt logged or committed — LoopAnomaly holds counts + a coarse label only; a test asserts the raw entity_id never appears in the explanation
• [x] Any guardrail/learning change is raise-only (no silent loosening) — N/A: this is advisory and off the decision path; test_loop_anomaly_is_not_a_verdict asserts it structurally
• [x] Every BLOCK/AUTH carries reason codes + a human explanation — N/A: emits no verdict
• [x] doberman-core does not import doberman_enterprise

Edge cases covered / Deviations from plan / Risks introduced

• Deviation (agreed on the thread): landed as the advisory detector half of [#143]; the sliding-window/tag/AUTH wiring belongs to the CostObserver seam and is deferred to [#143].
• Known ceiling (commented): burst signals are per-entity_id, so a caller able to rotate the entity id resets the counter — harmless while advisory with no production caller; the [#143] wiring must bound it.
• Known limitation (commented): the id-ordered row cap suits the live "is this entity looping now" read; a far-historical now on a ledger with more than the cap of newer rows can under-count — an accepted trade-off for an advisory readout.

[Anonymous]

Originally posted by: fu351

Hi @harshitagrawal2O — thanks again for the CI/CD-config protection half of this. The GitLab/Jenkins/CircleCI/Azure globs are solid and well tested, and all four CI checks are green.

I want to make sure this doesn't stall, so here's where things stand and two clear ways forward — both genuinely fine with me.

Context first, so you're not doing double work: I'm going to land the other half of [#92] (the role-boundary matcher) myself in a separate, small PR. src/doberman/roles/builtin_roles.yaml still only has .github/workflows/** in its five role-boundary entries, with no GitLab/Jenkins/CircleCI/Azure equivalents, so that side isn't covered yet. Your glob work isn't being thrown away — I'll carry it forward with Co-Authored-By: harshitagrawal2O credit on that PR.

That leaves this PR free to become a focused CB.3 slice. As written, detect_loop_anomaly is advisory cost-row aggregation with no wiring into the decision path — the plan (Slice CB.3) calls for deterministic HMAC action/turn fingerprint counting in a sliding window, a loop_anomaly tag below the sustained threshold, an AUTH contribution above it, mode-scaled thresholds, and combine/reason-code wiring. There's also a real bug in the module's "never raises" guarantee: the ts < cutoff / ts > now comparisons sit outside the try/except that guards datetime.fromisoformat(), so a naive (no-tzinfo) timestamp compared against an aware cutoff raises an uncaught TypeError.

So the choice is yours:

(a) Rescope this PR to CB.3 only — drop the CI/CD glob commits (they'll already be covered elsewhere), rebase onto current main, and build the loop-anomaly detector to the plan's spec, including the TypeError fix and a regression test for it. Or

(b) Close this one and pick up something fresh and self-contained instead — [#198] (engine: escalate bulk-delete of gitignored, uncommitted directories, level-7) or [#199] (engine: decide + fix force-push risk classification beyond the default protected-branch list, level-8) are both good next steps.

Either is genuinely welcome — just let me know which way you'd like to go.

[Anonymous]

Originally posted by: fu351

Separate from the review above — here's where your work fits in the roadmap, because I don't want the rebase note to read as "start over."

Your cost detector is the beginning of a roadmap item we actually want: #143 — Cost-observability seam: CostObserver plugin + loop-anomaly detector (level-9, help wanted, roadmap).

That issue asks for exactly the two halves you've been building toward:

1. a CostObserver plugin seam — an interface plus entry-point registration, so cost observers can be registered without core importing them by name, and
2. a loop-anomaly detector built on top of it — which is essentially what your src/doberman/storage/cost.py already is.

So the path I'd suggest: rebase this PR down to just cost.py + its tests as described in the review, land it as the detector half, then take [#143] for the seam that makes it pluggable and wires it to a real caller. That also resolves the "ships as inert library code" note — right now nothing calls detect_loop_anomaly, and [#143] is the issue that gives it a home.

Worth flagging one design constraint before you start on the seam, since it's the thing most likely to cause rework: cost observation must stay strictly off the decision path. Logging and telemetry may never alter, gate, or crash a decision. Your current code already respects this — test_loop_anomaly_is_not_a_verdict asserts it structurally, which was good to see — and the seam needs to preserve that property, not just the detector.

If [#143] is more than you want to take on right now, two smaller ones in the same area:

• #212 — built-in Detector for oversized base64 blobs in tool arguments (level-7) — detector work against the same interface, much narrower scope.
• #198 — escalate bulk-delete of gitignored, uncommitted directories (level-7) — closest to the path-rule work you did in [#164], and that work is already merged and credited to you, so you know this code.

Comment on whichever you'd like and I'll make sure the context is there before you start. Thanks for sticking with this one.

[Anonymous]

Originally posted by: fu351

Checking in, with no pressure attached. Your last push here was 25 July and my review notes went up on the 30th, so I mostly want to be sure the ball isn't sitting in my court by mistake.

Nothing here has gone stale or lost its place. The cost-detector half is still wanted, and [#143] is still open and still yours to take once this lands.

If the rebase-and-narrow I asked for is more of a slog than it looked, say so and I'll get specific about exactly what to keep and what to drop. That request took me one paragraph to type and is genuinely fiddly to carry out, which is a bad trade I'd rather fix than let sit. If you've moved on or things simply got busy, that's completely fine too — say the word and I'll close it with no hard feelings, and your #152 work stays credited either way.

If a faster back-and-forth would help, we have a Discord: https://discord.gg/Sfy5XGNqty. I'm happy to walk through the rebase there or right here on this thread, whichever suits you.

[Anonymous]

Originally posted by: fu351

Checking in — no pressure. This branch has conflicts with main now and the earlier review feedback is still open. If you're still on it, happy to answer questions here or on Discord. If you've moved on, no problem at all — say the word and I'll return it to the pool, no hard feelings.

[Anonymous]

Originally posted by: harshitagrawal2O

Rebased onto current main and narrowed this to the two net-new files (src/doberman/storage/cost.py + tests/unit/test_cost_loop_anomaly.py) — the CI/CD glob hunks are dropped since [#203] already carries that work. Thanks for landing it and the credit.

Addressed the review:

• TypeError in the "never raises" path — fixed. A tz-naive now returns calm up front, now is now optional (defaults to datetime.now(timezone.utc)) so omitting it can't raise, and a legacy naive-timestamp row is skipped in-loop rather than crashing the comparison. Regression tests added for a naive now, a legacy naive DB row, and the now default.
• Exact threshold boundary — added test_exact_threshold_is_not_an_anomaly covering calls == max_calls (not an anomaly) and calls == max_calls + 1 (is).
• Infra-first — stated explicitly in the PR body: nothing calls detect_loop_anomaly yet; [#143] is where the seam + a real caller live.
• Per-entity_id scoping ceiling — named in a comment.

While I was in there I also tightened two things from a self-review pass:

• Signal calibration — one action emits several CostEvent rows, so counting every row mis-labeled "runaway tool-call loop" and mixed call-counts into token burn. call_burst now counts only tool_call rows; token_burst sums only token-kind units. Added kind-scoping tests.
• Bounded read — capped by the monotonic id PK (in idx_cost_events) instead of the unindexed TEXT ts, so it's a real indexed read and immune to mixed UTC offsets in stored timestamps.

Kept everything you liked (parameterized SQL, _ANOMALY_ROW_CAP bound, the broken-DB fail-safe test, the not-a-verdict structural test, the redaction test). Green locally: ruff, ruff format, lint-imports (both contracts kept), and the full pytest suite. Ready for re-review — and happy to take [#143] next for the CostObserver seam that wires this to a caller.

[Anonymous]

Ticket changed by: fu351

• status: open --> closed

[Anonymous]

Originally posted by: fu351

Two in one day — nice. The datetime fix is done properly rather than patched over: a naive now returns calm before any comparison, and naive ledger rows are skipped, so by the time ts < cutoff runs both sides are guaranteed tz-aware and the TypeError can't recur. I also like the two calibration fixes you added on your own initiative — kind-scoping the burst counts, and switching the bounded read to the indexed monotonic id instead of the unindexed ts. The detector stays fully off the decision path (nothing wires it in yet), which matches the de-scope: land the detector here, the engine wiring goes to [#143]. And test_exact_threshold_is_not_an_anomaly pins the calls == max_calls off-by-one exactly. Merging.

You've now landed two detectors. [#321] (PII / financial-data-class exfil) is still the natural next one from the earlier suggestion, and [#143] is where this loop-anomaly detector eventually gets wired into the engine, if you want to follow it all the way through. Happy to assign either.

Thanks again, harshitagrawal2O.